[i2c] [PATCH] [2.6.21] PA Semi SMBus driver

Olof Johansson olof at lixom.net
Mon Jan 29 05:09:31 CET 2007


On Fri, Jan 26, 2007 at 08:27:50PM +0100, Jean Delvare wrote:
> Hi Olof,
> 
> On Thu, 25 Jan 2007 01:15:09 -0600, Olof Johansson wrote:
> > Driver for the SMBus interface on PA Semi's PWRficient line of processors.
> > 
> > Signed-off-by: Olof Johansson <olof at lixom.net>
> 
> Here's my review:

Hi,

Good review -- thanks!

I've addressed your comments (mostly were just misses by me, some must have come
from whatever driver it was I used for framework -- I no longer remember which one
that was).

I've done one more change since submission: Added native i2c master
transfers, and taken out some of the complex/block transfer types from
the smbus side. No need to keep them in there when they can be emulated,
and I needed the i2c transfers to use with the ds1307 RTC driver.

Comments to some of the comments below.


> > --- linux-2.6.orig/drivers/i2c/busses/Kconfig
> > +++ linux-2.6/drivers/i2c/busses/Kconfig
> > @@ -341,6 +341,12 @@ config I2C_PARPORT_LIGHT
> >  	  This support is also available as a module.  If so, the module 
> >  	  will be called i2c-parport-light.
> >  
> > +config I2C_PASEMI
> > +	tristate "PA Semi SMBus interface"
> > +	depends on PPC_PASEMI && I2C && PCI
> > +	help
> > +	  Supports the PA Semi SoC on-chip SMBus interfaces.
> 
> It's a new driver, don't you feel like having it depend on EXPERIMENTAL?

Not in this case. The whole platform is new, and it's depending on it
to even be configured. If it had been a driver that can be selected and
probed on PC hardware, then sure.

> Aren't "SoC" and "on-chip" redundant terms?
> s/interfaces/interface/?

Yep, fixed. The chips have multiple smbus controllers, thus the plural form.

> > +/*
> > + * Copyright (C) 2006 PA Semi, Inc
> 
> You might want to add 2007.
> 
> > + *
> > + * Maintained by: Olof Johansson <olof at lixom.net>
> 
> Maintainer information goes into file MAINTAINERS instead.

We've taken a habit of adding it to files as well in arch/powerpc, so I was used to it.
It makes less sense doing it that way for drivers though, I'll add a MAINTAINERS entry.

> > +#define CTL_MSW_M	0x07ff0000
> > +#define CTL_MRR		0x00000400
> > +#define CTL_MTR		0x00000200
> > +#define CTL_UJM		0x00000100
> > +#define CTL_CLK_M	0x000000ff
> 
> CTL_MSW_M and CTL_UJM aren't used, is this expected? You might as well
> not define them then.

Yep, I just brought over a set of header defines, and they were included. I've taken
them out.

> > +static int reg_read(struct pasemi_smbus *smbus, int reg)
> > +{
> > +	int ret;
> > +	ret = inl(smbus->base + reg);
> > +	pr_debug("smbus read reg %x val %08x\n", smbus->base + reg, ret);
> > +	return ret;
> > +}
> 
> You could inline both functions for better performance and a smaller
> driver.
> 
> In both pr_debug() you should use %lx instead of %x to print
> smbus->base + reg, as "base" is a unsigned long.

Odd, that was fixed in my patch here right before submission. I must
have missed a quilt refresh.

> BTW you should use
> dev_dbg() instead of pr_debug(), otherwise the user has no idea what
> device these debug messages are related to.

Good point.

> > +static unsigned int pasemi_smb_waitready(struct pasemi_smbus *smbus)
> > +{
> > +	int timeout = 500;
> > +	unsigned int status;
> > +
> > +	status = reg_read(smbus, SMSTA);
> > +
> > +	while (timeout-- && !(status & SMSTA_XEN)) {
> 
> You should test the status first, otherwise you might report a timeout
> while the bus was finally ready on the last wait cycle.

:-) Very unlikely but still a good habit.

> > +	/* All our ops take 8-bit shifted addresses */
> > +	addr <<= 1;
> > +	read = read_write == I2C_SMBUS_READ;
> 
> In practice this is equivalent to "read = read_write", so you could use
> read_write directly. The I2C_SMBUS_READ and I2C_SMBUS_WRITE constants
> were wisely chosen ;)

I was thinking about this for a bit when I wrote it. I didn't want my
register fields depend on higher-level driver API defines. If you're
comfortable with it, then so will I be.

> Please use I2C_SMBUS_BLOCK_MAX instead of hardcoding 32. Same below.

I took out the block ops when I added the I2C master transfer modes instead.

> > +
> > +	if (~pci_resource_flags(dev, 0) & IORESOURCE_IO)
> > +		return -ENODEV;
> 
> Strange construct, using a binary not (rather than bitwise) would make
> your intent much clearer.

Yep, I have no idea where that came from. Must have been from the driver
I based mine on.

> > +	error = i2c_add_adapter(&smbus->adapter);
> > +	if (error)
> > +		goto out_release_region;
> > +
> > +	reg_write(smbus, CTL, (CTL_MTR | CTL_MRR | (CLK_100K_DIV & CTL_CLK_M)));
> 
> You have a race condition here: you register the adapter before it is
> ready. You should swap the reg_write() and the adapter registration.

Good catch.

> You should define your vendor ID in include/linux/pci_ids.h, and use it.

Chicken and egg problem; I'm submitting separate patches for pci_ids.h,
but I don't want this to not build until that's in there. I expect to
follow up and replace later, if that's ok.

> Otherwise this is a very clean driver, good work!

...and nice and detailed review. Thanks!

> Care to address my comments and resubmit it for inclusion?

See follow-up.


-Olof



More information about the i2c mailing list