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.
Thanks,
Guenter
_______________________________________________
lm-sensors mailing list
lm-sensors@xxxxxxxxxxxxxx
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
