Hi Jean,
On Tue, Jan 24, 2012 at 03:20:41PM -0500, Jean Delvare wrote:
> On Mon, 23 Jan 2012 18:49:25 -0800, Guenter Roeck wrote:
> > From: Guenter Roeck <linux@xxxxxxxxxxxx>
> >
> > Fixed:
> > ERROR: code indent should use tabs where possible
> > ERROR: do not use assignment in if condition
> > ERROR: space prohibited before that close parenthesis ')'
> > ERROR: space required after that ',' (ctx:VxV)
> > ERROR: spaces required around that '<' (ctx:VxV)
> > ERROR: spaces required around that '==' (ctx:VxV)
> > ERROR: trailing statements should be on next line
> > ERROR: trailing whitespace
> > WARNING: simple_strtol is obsolete, use kstrtol instead
> > WARNING: simple_strtoul is obsolete, use kstrtoul instead
> >
> > Modify multi-line comments to follow Documentation/CodingStyle.
> >
> > Not fixed (false positive):
> > ERROR: Macros with multiple statements should be enclosed in a do - while loop
> >
> > Cc: Jean Delvare <khali@xxxxxxxxxxxx>
> > Signed-off-by: Guenter Roeck <linux@xxxxxxxxxxxx>
> > ---
> > drivers/hwmon/lm78.c | 215 +++++++++++++++++++++++++++++++++-----------------
> > 1 files changed, 141 insertions(+), 74 deletions(-)
> >
> > diff --git a/drivers/hwmon/lm78.c b/drivers/hwmon/lm78.c
> > index 6df0b46..6a69d1e 100644
> > --- a/drivers/hwmon/lm78.c
> > +++ b/drivers/hwmon/lm78.c
> > @@ -1,23 +1,23 @@
> > /*
> > - lm78.c - Part of lm_sensors, Linux kernel modules for hardware
> > - monitoring
> > - Copyright (c) 1998, 1999 Frodo Looijaard <frodol@xxxxxx>
> > - Copyright (c) 2007, 2011 Jean Delvare <khali@xxxxxxxxxxxx>
> > -
> > - This program is free software; you can redistribute it and/or modify
> > - it under the terms of the GNU General Public License as published by
> > - the Free Software Foundation; either version 2 of the License, or
> > - (at your option) any later version.
> > -
> > - This program is distributed in the hope that it will be useful,
> > - but WITHOUT ANY WARRANTY; without even the implied warranty of
> > - MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> > - GNU General Public License for more details.
> > -
> > - You should have received a copy of the GNU General Public License
> > - along with this program; if not, write to the Free Software
> > - Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
> > -*/
> > + * lm78.c - Part of lm_sensors, Linux kernel modules for hardware
> > + * monitoring
> > + * Copyright (c) 1998, 1999 Frodo Looijaard <frodol@xxxxxx>
> > + * Copyright (c) 2007, 2011 Jean Delvare <khali@xxxxxxxxxxxx>
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License as published by
> > + * the Free Software Foundation; either version 2 of the License, or
> > + * (at your option) any later version.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> > + * GNU General Public License for more details.
> > + *
> > + * You should have received a copy of the GNU General Public License
> > + * along with this program; if not, write to the Free Software
> > + * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
> > + */
> >
> > #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> >
> > @@ -74,11 +74,15 @@ enum chips { lm78, lm79 };
> > #define LM78_REG_I2C_ADDR 0x48
> >
> >
> > -/* Conversions. Rounding and limit checking is only done on the TO_REG
> > - variants. */
> > +/*
> > + * Conversions. Rounding and limit checking is only done on the TO_REG
> > + * variants.
> > + */
> >
> > -/* IN: mV, (0V to 4.08V)
> > - REG: 16mV/bit */
> > +/*
> > + * IN: mV, (0V to 4.08V)
>
> That comma could go away, methinks.
>
> > + * REG: 16mV/bit
> > + */
> > static inline u8 IN_TO_REG(unsigned long val)
> > {
> > unsigned long nval = SENSORS_LIMIT(val, 0, 4080);
> > (...)
> > @@ -669,11 +721,13 @@ static struct i2c_driver lm78_driver = {
> > .address_list = normal_i2c,
> > };
> >
> > -/* The SMBus locks itself, but ISA access must be locked explicitly!
> > - We don't want to lock the whole ISA bus, so we lock each client
> > - separately.
> > - We ignore the LM78 BUSY flag at this moment - it could lead to deadlocks,
> > - would slow down the LM78 access and should not be necessary. */
> > +/*
> > + * The SMBus locks itself, but ISA access must be locked explicitly!
> > + * We don't want to lock the whole ISA bus, so we lock each client
> > + * separately.
> > + * We ignore the LM78 BUSY flag at this moment - it could lead to deadlocks,
> > + * would slow down the LM78 access and should not be necessary.
> > + */
> > static int lm78_read_value(struct lm78_data *data, u8 reg)
> > {
> > struct i2c_client *client = data->client;
> > @@ -691,13 +745,15 @@ static int lm78_read_value(struct lm78_data *data, u8 reg)
> > return i2c_smbus_read_byte_data(client, reg);
> > }
> >
> > -/* The SMBus locks itself, but ISA access muse be locked explicitly!
> > - We don't want to lock the whole ISA bus, so we lock each client
> > - separately.
> > - We ignore the LM78 BUSY flag at this moment - it could lead to deadlocks,
> > - would slow down the LM78 access and should not be necessary.
> > - There are some ugly typecasts here, but the good new is - they should
> > - nowhere else be necessary! */
> > +/*
> > + * The SMBus locks itself, but ISA access muse be locked explicitly!
> > + * We don't want to lock the whole ISA bus, so we lock each client
> > + * separately.
> > + * We ignore the LM78 BUSY flag at this moment - it could lead to deadlocks,
> > + * would slow down the LM78 access and should not be necessary.
>
> This is the exact same comment as for the previous function...
>
> > + * There are some ugly typecasts here, but the good new is - they should
> > + * nowhere else be necessary!
>
> ... and this one no longer applies. So maybe you can kill the whole block?
>
> > + */
> > static int lm78_write_value(struct lm78_data *data, u8 reg, u8 value)
> > {
> > struct i2c_client *client = data->client;
> > @@ -823,8 +879,11 @@ static int __devinit lm78_isa_probe(struct platform_device *pdev)
> > lm78_init_device(data);
> >
> > /* Register sysfs hooks */
> > - if ((err = sysfs_create_group(&pdev->dev.kobj, &lm78_group))
> > - || (err = device_create_file(&pdev->dev, &dev_attr_name)))
> > + err = sysfs_create_group(&pdev->dev.kobj, &lm78_group);
> > + if (err)
> > + goto exit_remove_files;
> > + err = device_create_file(&pdev->dev, &dev_attr_name);
> > + if (err)
> > goto exit_remove_files;
> >
> > data->hwmon_dev = hwmon_device_register(&pdev->dev);
>
> Doesn't this change increase the binary size?
>
Actually, no. Binary size turns out to be exactly the same.
> The rest looks good.
>
I'll send an updated patch in a minute.
Thanks,
Guenter
_______________________________________________
lm-sensors mailing list
lm-sensors@xxxxxxxxxxxxxx
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
