regulator: new drivers for AD5398 and AD5821
in [Kernel]
|
Prev: block: push down BKL into .locked_ioctl
Next: [PATCHv2 10/11] OMAP2: Devkit8000: Remove unused omap_board_config
From: Mark Brown on 2 Jun 2010 06:40 On Wed, Jun 02, 2010 at 04:51:34PM +0800, sonic zhang wrote: > +static int ad5398_read_reg(struct i2c_client *client, unsigned short *data) > +{ > + unsigned short val; > + int ret; > + > + ret = i2c_master_recv(client, (char *)&val, 2); > + if (ret < 0) { > + dev_err(&client->dev, "I2C read error\n"); > + return ret; > + } > + *data = swab16(val); Should this not be a be16_to_cpu() or similar rather than an unconditional byte swap? Presumably the byte swap is not going to be needed if the CPU has the same endianness as the CPU that the system is using. > + /* read chip enable bit */ > + ret = ad5398_read_reg(client, &data); > + if (ret < 0) > + return ret; > + /* prepare register data */ > + selector = (selector << chip->current_offset) & chip->current_mask; > + selector |= (data & AD5398_CURRENT_EN_MASK); The reason I was querying this code on the original submission is that it is more normal to write this as something like data = selector | (data & ~chip->current_mask); ie, to write the code as "set these bits" rather than "preserve these bits". This is more clearly robust to the reader since it's clear that there aren't other register bits which should be set. > + chip->min_uA = init_data->constraints.min_uA; > + chip->max_uA = init_data->constraints.max_uA; This looks very wrong, especially given that you use the min_uA and max_uA settings to scale the register values being written in to the chip. I would expect that either the limits would be fixed in the silicon or (if they depend on things like the associated passives which can be configured per-board) that there would be some other setting in the platform data which specifies what's actually being changed. The constraints being specified by the platform should not influence the physical properties of the chip, they control which values are allowed in a particular design (for example, saying that values over a given limit are not allowed due to the limits of the hardware connected to the regulator) and are separate to what the chip is capable of. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Sonic Zhang on 2 Jun 2010 23:00 On Wed, Jun 2, 2010 at 6:33 PM, Mark Brown <broonie(a)opensource.wolfsonmicro.com> wrote: > On Wed, Jun 02, 2010 at 04:51:34PM +0800, sonic zhang wrote: > >> +static int ad5398_read_reg(struct i2c_client *client, unsigned short *data) >> +{ >> + � � unsigned short val; >> + � � int ret; >> + >> + � � ret = i2c_master_recv(client, (char *)&val, 2); >> + � � if (ret < 0) { >> + � � � � � � dev_err(&client->dev, "I2C read error\n"); >> + � � � � � � return ret; >> + � � } >> + � � *data = swab16(val); > > Should this not be a be16_to_cpu() or similar rather than an > unconditional byte swap? �Presumably the byte swap is not going to be > needed if the CPU has the same endianness as the CPU that the system is > using. I made a mistake to mix simple i2c transfer and smbus i2c transfer here. For smbus i2c transfer, byte swap is unconditional. > >> + � � /* read chip enable bit */ >> + � � ret = ad5398_read_reg(client, &data); >> + � � if (ret < 0) >> + � � � � � � return ret; > >> + � � /* prepare register data */ >> + � � selector = (selector << chip->current_offset) & chip->current_mask; >> + � � selector |= (data & AD5398_CURRENT_EN_MASK); > > The reason I was querying this code on the original submission is that > it is more normal to write this as something like > > � �data = selector | (data & ~chip->current_mask); > > ie, to write the code as "set these bits" rather than "preserve these > bits". �This is more clearly robust to the reader since it's clear that > there aren't other register bits which should be set. OK. > >> + � � � chip->min_uA = init_data->constraints.min_uA; >> + � � � chip->max_uA = init_data->constraints.max_uA; > > This looks very wrong, especially given that you use the min_uA and > max_uA settings to scale the register values being written in to the > chip. �I would expect that either the limits would be fixed in the > silicon or (if they depend on things like the associated passives which > can be configured per-board) that there would be some other setting in > the platform data which specifies what's actually being changed. > > The constraints being specified by the platform should not influence the > physical properties of the chip, they control which values are allowed > in a particular design (for example, saying that values over a given > limit are not allowed due to the limits of the hardware connected to the > regulator) and are separate to what the chip is capable of. > I will predefine the chip physical min max values in the driver and add user defined limitation based on both initial constraints and chip spec. Sonic -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Mark Brown on 3 Jun 2010 13:10 On Thu, Jun 03, 2010 at 11:27:20AM +0800, sonic zhang wrote: > The AD5398 and AD5821 are single 10-bit DAC with 120 mA output current > sink capability. They feature an internal reference and operates from > a single 2.7 V to 5.5 V supply. > This driver supports both the AD5398 and the AD5821. It adapts into the > voltage and current framework. Looks good, just a few fairly small things. > + if (chip->min_uA < init_data->constraints.min_uA) > + chip->user_min_uA = init_data->constraints.min_uA; > + else > + chip->user_min_uA = chip->min_uA; > + if (chip->max_uA > init_data->constraints.max_uA) > + chip->user_max_uA = init_data->constraints.max_uA; > + else > + chip->user_max_uA = chip->max_uA; Your driver should not be worrying about the constraints, the regulator core will take care of enforcing them. > +MODULE_DESCRIPTION("AD5398 and AD5821 current regulator driver"); > +MODULE_AUTHOR("Sonic Zhang"); > +MODULE_LICENSE("GPL"); > +MODULE_ALIAS("platform:ad5398-regulator"); This is an I2C device, not a platform device. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Mark Brown on 7 Jun 2010 08:30 On Fri, Jun 04, 2010 at 12:40:46PM +0800, sonic zhang wrote: > The AD5398 and AD5821 are single 10-bit DAC with 120 mA output current > sink capability. They feature an internal reference and operates from > a single 2.7 V to 5.5 V supply. > This driver supports both the AD5398 and the AD5821. It adapts into the > voltage and current framework. > Signed-off-by: Sonic Zhang <sonic.zhang(a)analog.com> > Signed-off-by: Mike Frysinger <vapier(a)gentoo.org> Acked-by: Mark Brown <broonie(a)opensource.wolfsonmicro.com> but > +static int __init ad5398_init(void) > +{ > + return i2c_add_driver(&ad5398_driver); > +} > +module_init(ad5398_init); at least some systems are likely to want this to be subsys_initcall() to make sure the regulator is available prior to the consumers. Please send an incremental change for this rather than repostnig the full thing. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Mark Brown on 7 Jun 2010 08:50
On Mon, Jun 07, 2010 at 08:39:28AM -0400, Mike Frysinger wrote: > i guess the Blackfin I2C driver should have its initcall bumped up as well Yes, probably. SPI might be worth it also. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/ |