Re: [lm-sensors] [PATCH] hwmon: LTC4261 Hardware monitoring driver

[Guenter Roeck <guenter.roeck@ericsson.com>]

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@ericsson.com>
> >
> > 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@ovro.caltech.edu>
> 
> 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.

Thanks,
Guenter


_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors