Re: [PATCH] hwmon: LTC4261 Hardware monitoring driver

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Mon, 2010-10-04 at 11:21 -0400, Ira W. Snyder wrote:
> 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!

As for Lindent ... working in an environment where everyone insists in a
personal coding style, I find it quite useful in helping to enforce a
common coding style without much argument about it. At the very least,
it establishes a common baseline. Of course, that only works if one
gives a good example. 

Also, I don't take the output of Lindent as written in stone (sometimes
its output is a bit odd), but tend to rely on it as common baseline if
there is a disagreement.

Guenter



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


[Index of Archives]     [Linux Kernel]     [Linux Hardware Monitoring]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]

  Powered by Linux