Hi Jean,
> Hi Juerg,
>
> On Sun, 26 Aug 2007 20:23:27 -0700, Juerg Haefliger wrote:
> > This patch cleans up and prepares the dme1737 driver for support of the sch311x
> > chips. (Almost) no functional changes.
> >
> > - Replaced whitespaces with tabs.
> > - Removed empty lines.
> > - Added _i2c_ to names of functions that are strictly I2C related.
> > - Added 4 new functions: dme1737_create_files, dme1737_remove_files,
> > dme1737_sio_enter, and dme1737_sio_exit.
> > - Added error messages in case client attach/detach fails.
> >
> > Signed-off-by: Juerg Haefliger <juergh at gmail.com>
> >
> > Jean: I'd really appreciate it if you take a look at the patches.
>
> Sorry, I just noticed this request. Next time please include it in the
> patch 0/n instead, so that I'm more likely to read it.
>
> Overall I'm fine with this patch, with the suggestions below:
Thanks for the review. I'll incorporate your suggestions and resubmit.
...juerg
> > --- linux-2.6.orig/drivers/hwmon/dme1737.c 2007-08-23 22:23:50.000000000 -0700
> > +++ linux-2.6/drivers/hwmon/dme1737.c 2007-08-26 20:08:01.000000000 -0700
> > @@ -493,8 +493,8 @@
> >
> > static struct dme1737_data *dme1737_update_device(struct device *dev)
> > {
> > - struct i2c_client *client = to_i2c_client(dev);
> > - struct dme1737_data *data = i2c_get_clientdata(client);
> > + struct dme1737_data *data = dev_get_drvdata(dev);
> > + struct i2c_client *client = &data->client;
> > int ix;
> > u8 lsb[5];
> >
> > @@ -674,7 +674,7 @@
> > break;
> > default:
> > res = 0;
> > - dev_dbg(dev, "Unknown attr fetch (%d)\n", fn);
> > + dev_dbg(dev, "Unknown function %d.\n", fn);
> > }
> >
> > return sprintf(buf, "%d\n", res);
>
> I would back out this family of changes. This is only a debug message
> and it will never show up anyway so why bother changing it? And your
> patch is quite big so making it a bit smaller can't hurt. But it's up
> to you.
>
> > (...)
> > -static int dme1737_detect(struct i2c_adapter *adapter, int address,
> > - int kind)
> > +static int dme1737_i2c_detect(struct i2c_adapter *adapter, int address,
> > + int kind)
> > {
> > u8 company, verstep = 0;
> > struct i2c_client *client;
> > struct dme1737_data *data;
> > - int ix, err = 0;
> > + struct device *dev;
> > + int err = 0;
> > const char *name;
> >
> > if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA)) {
> > @@ -1886,6 +1982,7 @@
> > }
> >
> > if (!(data = kzalloc(sizeof(struct dme1737_data), GFP_KERNEL))) {
> > + dev_err(&adapter->dev, "Failed to allocate memory.\n");
>
> i2c-core will already emit an error message in this unlikely case, so I
> don't think it's worth adding something here.
>
> > err = -ENOMEM;
> > goto exit;
> > }
> > @@ -1894,7 +1991,8 @@
> > i2c_set_clientdata(client, data);
> > client->addr = address;
> > client->adapter = adapter;
> > - client->driver = &dme1737_driver;
> > + client->driver = &dme1737_i2c_driver;
> > + dev = &client->dev;
> >
> > /* A negative kind means that the driver was loaded with no force
> > * parameter (default), so we must identify the chip. */
> > @@ -1919,95 +2017,37 @@
> >
> > /* Tell the I2C layer a new client has arrived */
> > if ((err = i2c_attach_client(client))) {
> > + dev_err(&adapter->dev, "Failed to attach client.\n");
> > goto exit_kfree;
> > }
>
> i2c_attach_client logs a message in case of error already, so this is
> redundant too.
>
> > (...)
> > -static int dme1737_detach_client(struct i2c_client *client)
> > +static int dme1737_i2c_detach_client(struct i2c_client *client)
> > {
> > struct dme1737_data *data = i2c_get_clientdata(client);
> > - int ix, err;
> > + int err;
> >
> > hwmon_device_unregister(data->class_dev);
> > -
> > - for (ix = 0; ix < ARRAY_SIZE(dme1737_fan_group); ix++) {
> > - if (data->has_fan & (1 << ix)) {
> > - sysfs_remove_group(&client->dev.kobj,
> > - &dme1737_fan_group[ix]);
> > - }
> > - }
> > - for (ix = 0; ix < ARRAY_SIZE(dme1737_pwm_group); ix++) {
> > - if (data->has_pwm & (1 << ix)) {
> > - sysfs_remove_group(&client->dev.kobj,
> > - &dme1737_pwm_group[ix]);
> > - }
> > - }
> > - sysfs_remove_group(&client->dev.kobj, &dme1737_group);
> > + dme1737_remove_files(&client->dev);
> >
> > if ((err = i2c_detach_client(client))) {
> > + dev_err(&client->dev, "Failed to detach client.\n");
> > return err;
> > }
> >
>
> Again, i2c_detach_client already logs a message if it fails. I did
> remove these redundant messages from all drivers some times ago, so
> please don't add them back!
>
> http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=7bef559455fc71f66f8573cc1aafe1dd33966c1c
>
> --
> Jean Delvare
>
