Re: RFC: Alchemy Au1550 SMBus driver on PSC

In article <[email protected]>
[email protected] wrote:

>   sys-arch-mips.diff:	diffs for sys/arch/mips from -current.
>   ausmbus_psc.c:	smbus driver.
>   smbusreg.c:		definitions for SMBus registers

Mostly looks good, but I'd suggest some cosmetics again:

> RCS file: /cvsroot/src/sys/arch/mips/alchemy/dev/aupscreg.h,v
 :
> @@ -124,5 +127,6 @@
>  #define	AUPSC_SMBEVNT			0x18	/* R/W */
>  #define	AUPSC_SMBTXRX			0x1c	/* R/W */
>  #define	AUPSC_SMBTMR			0x20	/* R/W */
> +#include <mips/alchemy/dev/smbusreg.h>
>  
>  #endif	/* _MIPS_ALCHEMY_DEV_AUPSCREG_H_ */

This #include is needed here?

> RCS file: /cvsroot/src/sys/arch/mips/conf/files.alchemy,v
>  # On-chip PSC
> -device	aupsc
> +device	aupsc { [addr=-1] }

This should be { [ addr = -1 ] } ?

In ausmbus_psc.c:
> #include <sys/lock.h>

sys/lock.h is still needed?

> 	/* Setup SMBus Protocol Timing register */
> 	v = SMBUS_TMR_TH_SET(0)
> 		| SMBUS_TMR_PS_SET(0xf) | SMBUS_TMR_PU_SET(0xf)
> 		| SMBUS_TMR_SH_SET(0xf) | SMBUS_TMR_SU_SET(0xf)
> 		| SMBUS_TMR_CL_SET(0xf) | SMBUS_TMR_CH_SET(0xf);

Hmm, can't these "0xf" magic numbers be defined in somewhere?

> 	ausmbus_reg_write(sc, AUPSC_SMBTMR, v);
 :
> 	ausmbus_reg_write(sc, AUPSC_SMBMSK, v);
 :
> 	v = ausmbus_reg_read(sc, AUPSC_SMBCFG);
 :
> 	ausmbus_reg_write(sc, AUPSC_SMBCFG, v);
> 	v = ausmbus_reg_read(sc, AUPSC_SMBSTAT);
 :
> static uint32_t
> ausmbus_reg_read(struct ausmbus_softc *sc, int reg)
> {
> 
> 	return bus_space_read_4(sc->sc_ctrl.psc_bust, sc->sc_ctrl.psc_bush,
> 		reg);
> }
> 
> static void
> ausmbus_reg_write(struct ausmbus_softc *sc, int reg, uint32_t val)
> {
> 
> 	bus_space_write_4(sc->sc_ctrl.psc_bust, sc->sc_ctrl.psc_bush,
> 		reg, val);
> }

If ausmbus_reg_{read,write}() are used only here, they should
be macros rather than static functions.

> #if 0
> 	printf("config: 0x%08x\n", ausmbus_reg_read(sc, AUPSC_SMBCFG));
> 	printf("status: 0x%08x\n", ausmbus_reg_read(sc, AUPSC_SMBSTAT));
> 	printf("tmr   : 0x%08x\n", ausmbus_reg_read(sc, AUPSC_SMBTMR));
> 	printf("mask  : 0x%08x\n", ausmbus_reg_read(sc, AUPSC_SMBMSK));
> #endif

If you use these printfs for debug, this could be
#ifdef AUSMBUS_DEBUG or something.
---
Izumi Tsutsui