Re: [PATCH 4/4] regulator: Convert tps65023 to use regmap API

[Jean Delvare]

On Fri, 15 Jul 2011 21:16:50 +0900, Mark Brown wrote:
> On Fri, Jul 15, 2011 at 12:31:21PM +0200, Jean Delvare wrote:
> 
> > The driver was previously using the SMBus API and could thus work with
> > SMBus (non-I2C) controllers. After your change, an I2C controller is
> > mandatory. Are you sure this is OK for all users?
> 
> I'd be *utterly* astonished if it ever caused any issues, embedded
> systems like those that have regulators just don't have anything less
> than full I2C support, it's exceptionally rare to see any direct SMBus
> support in the hardware at all - the only example I can see in the tree
> is Blackfin and that is a proper I2C controller too, it just seems that
> someone overdesigned the hardware a little.
> 
> Basically, for something like a regulator it's just not an issue.

OK, alright then.

> > As a general comment, this requirement will considerably limit the
> > interest of regmap for I2C devices (at least in its current form.) Many
> > systems out there only have SMBus controllers, and more importantly,
> > most I2C device drivers are meant to be portable across systems and
> > thus rely on the SMBus API. The i2c documentation encourages driver
> > authors to do this.
> 
> Right, so for pretty much anything except PCs the main reason for using
> the SMBus API is that it provides the sort of data mangling to the bus
> functionality that the regmap API provides.  The main thing it's missing
> for drivers like this one is that it doesn't have a read/modify/write
> operation, otherwise there would be no current value in switching to the
> regmap API.  As far as I can tell this isn't really a big deal except
> for those device classes like hwmon which are frequently deployed in
> PCs, anything else just won't care.

Mostly correct. There are a few other devices which can also be found
on PCs: multiplexers and EEPROMs at least.

> This should be reasonably simple to handle in the regmap API, just teach
> the I2C module to fall back to using the SMBus operations if the
> controller is limited to that.  This should be done incrementally as it
> adds complexity, and ideally would be done by someone with some access
> to hardware that can use the SMBus API (I don't have any myself) so they
> can test.

You do have access ;) The i2c core will translate SMBus to I2C as
needed, so all you have to do is temporarily alter the regmap core to
unconditionally translate to SMBus. This will be inefficient (regmap ->
SMBus -> I2C) but should be suitable for testing.

> Does that sound reasonable to you?

Yes, no objection.

> > >  static int tps_65023_set_bits(struct tps_pmic *tps, u8 reg, u8 mask)
> > >  {
> > > -	int err, data;
> 
> *Please* could people delete unneeded context from mails?

? There were more comments from me in the rest of the code, which is
why I left it.

-- 
Jean Delvare
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/