On Sun, Oct 03, 2010 at 06:18:07PM -0700, Guenter Roeck wrote:
> Hi Ira,
>
> On Sun, Oct 03, 2010 at 05:17:42PM -0400, Ira W. Snyder wrote:
> > On Sun, Oct 03, 2010 at 06:29:34PM +0200, Jean Delvare wrote:
> > > On Fri, 17 Sep 2010 21:32:41 -0700, Guenter Roeck wrote:
> > > > This driver adds support for Linear Technology LTC4261 I2C Negative
> > > > Voltage Hot Swap Controller.
> > > >
> > > > Signed-off-by: Guenter Roeck <guenter.roeck@xxxxxxxxxxxx>
> > >
> > > Ira, you are familiar with Linear Technology hardware monitoring
> > > devices, would you mind reviewing this driver? Thanks.
> > >
> >
> > Sure, not a problem.
> >
> > Guenter, there are only a few minor nitpicks below. Please fix them and
> > then add this after your Signed-off-by line:
> > Reviewed-by: Ira W. Snyder <iws@xxxxxxxxxxxxxxxx>
> >
> > Overall, the driver looks ready for inclusion.
> >
> Thanks a lot - feedback inline
>
> > Ira
>
> [...]
> >
> > > > +
> > > > +/* chip registers */
> > > > +#define LTC4261_STATUS 0x00 /* readonly */
> > > > +#define LTC4261_FAULT 0x01
> > > > +#define LTC4261_ALERT 0x02
> > > > +#define LTC4261_CONTROL 0x03
> > > > +#define LTC4261_SENSE_H 0x04
> > > > +#define LTC4261_SENSE_L 0x05
> > > > +#define LTC4261_ADIN2_H 0x06
> > > > +#define LTC4261_ADIN2_L 0x07
> > > > +#define LTC4261_ADIN_H 0x08
> > > > +#define LTC4261_ADIN_L 0x09
> > > > +
> >
> > Tabs after #define here should be spaces. Keep the tabs just before the
> > numbers to keep things lined up. You got it right on the fault register
> > bits below.
> >
> Yes, I know. Always happens to me. Fixed that already right after I submitted the patch.
>
> > > > +/*
> > > > + * Fault register bits
> > > > + */
> > > > +#define FAULT_OV (1<<0)
> > > > +#define FAULT_UV (1<<1)
> > > > +#define FAULT_OC (1<<2)
> > > > +
> > > > +struct ltc4261_data {
> > > > + struct device *hwmon_dev;
> > > > +
> > > > + struct mutex update_lock;
> > > > + bool valid;
> > > > + unsigned long last_updated; /* in jiffies */
> > > > +
> > > > + /* Registers */
> > > > + u8 regs[10];
> > > > +};
> > > > +
> > > > +static struct ltc4261_data *ltc4261_update_device(struct device *dev)
> > > > +{
> > > > + struct i2c_client *client = to_i2c_client(dev);
> > > > + struct ltc4261_data *data = i2c_get_clientdata(client);
> > > > + struct ltc4261_data *ret = data;
> > > > +
> > > > + mutex_lock(&data->update_lock);
> > > > +
> > > > + if (time_after(jiffies, data->last_updated + HZ) || !data->valid) {
> >
> > The datasheet claims that registers E-J (SENSE and ADIN/OV registers)
> > are updated at 7.3 Hz. See page 21: Data Converter. A 1 Hz update rate
> > is awfully slow for an application that wants to do fast monitoring. I
> > think (HZ / 4) or (HZ / 6) would be better.
> >
> Makes sense. I'll use HZ/4.
>
> > > > + int i;
> > > > +
> > > > + /* Read registers -- 0x00 to 0x09 */
> > > > + for (i = 0; i < ARRAY_SIZE(data->regs); i++) {
> > > > + int val;
> > > > +
> >
> > I'd move the "int i" and "int val" variable declarations to the top of
> > the function. I don't have a strong preference, though.
> >
> Common tendency is that a variable should be defined in the innermost block
> of its use. I know I am not always strict with that rule, but try to follow
> the idea.
>
> [...]
>
> > > > +MODULE_DEVICE_TABLE(i2c, ltc4261_id);
> > > > +
> > > > +/* This is the driver that will be inserted */
> > > > +static struct i2c_driver ltc4261_driver = {
> > > > + .driver = {
> > > > + .name = "ltc4261",
> > > > + },
> > > > + .probe = ltc4261_probe,
> > > > + .remove = ltc4261_remove,
> > > > + .id_table = ltc4261_id,
> > > > +};
> > > > +
> >
> > I'd align the equal signs, like the ltc4245 driver does. No strong
> > preference, though. Like this:
> >
> > static struct i2c_driver ltc4261_driver = {
> > .driver = {
> > .name = "ltc4261",
> > },
> > .probe = ltc4261_probe,
> > .remove = ltc4261_remove,
> > .id_table = ltc4261_id,
> > };
> >
> The formatting is the result of Lindent, which doesn't seem to like
> the extra blanks. In questions like this - where much of it is
> personal preference - I prefer to go with the Lindent results.
>
For both this and the variable declarations above, I don't have a strong
preference. They're just things I would personally do differently. Like
Jean commented below, I don't use Lindent either. Please still add my
Reviewed-by on the next version. You've fixed the stuff I was concerned
about.
Thanks,
Ira
_______________________________________________
lm-sensors mailing list
lm-sensors@xxxxxxxxxxxxxx
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
