On Sunday 20 April 2008 13:33, Jean Delvare wrote:
> Hi David,
>
> On Wed, 16 Apr 2008 10:30:12 -0700, David Brownell wrote:
> > Teach the LM75 driver how to use new-style driver binding:
> >
> > - Create a second driver struct, using new-style driver binding methods
> > cribbed from the legacy code.
> >
> > - Make the legacy probe logic delegate all its work to this new code.
> >
> > - The old driver struct was renamed as "lm75_legacy".
> >
> > - Sensor limits are chip-specific; record them, and use them in new
> > routines to interconvert temperature and register values.
> >
> > - More careful initialization. Chips are put into 9-bit mode so
> > the current interconversion routines will never fail.
> >
> > - Save the original chip configuration, and restore it on exit.
> >
> > A later patch updates those interconversion routines to support
> > higher resolution measurements on chips which support them.
> >
> > Signed-off-by: David Brownell <dbrownell at users.sourceforge.net>
> > ---
> > drivers/hwmon/Kconfig | 9 +-
> > drivers/hwmon/lm75.c | 191
+++++++++++++++++++++++++++++++++++++-------------
> > 2 files changed, 149 insertions(+), 51 deletions(-)
> >
>
> Out of the various comments I made back in September 2007, there's
> really only one which I have to repeat here. Let's say that the other
> ones were only nitpicking on my side.
>
> > (...)
> > --- a/drivers/hwmon/lm75.c
> > +++ b/drivers/hwmon/lm75.c
> > @@ -54,18 +54,19 @@ static const u8 LM75_REG_TEMP[3] = {
> >
> > /* Each client has this additional data */
> > struct lm75_data {
> > - struct i2c_client client;
> > + struct i2c_client *client;
> > struct device *hwmon_dev;
> > struct mutex update_lock;
> > + long min, max;
> > + u8 orig_conf;
> > char valid; /* !=0 if registers are valid */
> > unsigned long last_updated; /* In jiffies */
> > - u16 temp[3]; /* Register values,
> > + s16 temp[3]; /* Register values,
> > 0 = input
> > 1 = max
> > 2 = hyst */
> > };
> >
> > -static void lm75_init_client(struct i2c_client *client);
> > static int lm75_read_value(struct i2c_client *client, u8 reg);
> > static int lm75_write_value(struct i2c_client *client, u8 reg, u16
value);
> > static struct lm75_data *lm75_update_device(struct device *dev);
> > @@ -75,13 +76,28 @@ static struct lm75_data *lm75_update_dev
> >
> > /* sysfs attributes for hwmon */
> >
> > +static inline s16 temp_to_reg(struct lm75_data *lm75, long temp)
> > +{
> > + if (temp < lm75->min)
> > + temp = lm75->min;
> > + else if (temp > lm75->max)
> > + temp = lm75->max;
> > +
> > + return LM75_TEMP_TO_REG(temp);
>
> This is inefficient. Your have already done half of what
> LM75_TEMP_TO_REG() does, so you're better copying the rest in your
> function.
>
> Alternatively, you could just move the addition of the min and max
> settings the next patch in the series, as I can't really see how they
> are related to converting the driver to new-style i2c. This would be in
> line with your own comment that my complaint is "addressed in the third
> patch".
Moving min and max support to the third patch seems indeed better.
> > +}
> > +
> > +static inline int reg_to_temp(s16 reg)
> > +{
> > + return LM75_TEMP_FROM_REG(reg);
> > +}
> > +
> > static ssize_t show_temp(struct device *dev, struct device_attribute *da,
> > char *buf)
> > {
> > struct sensor_device_attribute *attr = to_sensor_dev_attr(da);
> > struct lm75_data *data = lm75_update_device(dev);
> > - return sprintf(buf, "%d\n",
> > - LM75_TEMP_FROM_REG(data->temp[attr->index]));
> > +
> > + return sprintf(buf, "%d\n", reg_to_temp(data->temp[attr->index]));
> > }
> >
> > static ssize_t set_temp(struct device *dev, struct device_attribute *da,
> > @@ -94,7 +110,7 @@ static ssize_t set_temp(struct device *d
> > long temp = simple_strtol(buf, NULL, 10);
> >
> > mutex_lock(&data->update_lock);
> > - data->temp[nr] = LM75_TEMP_TO_REG(temp);
> > + data->temp[nr] = temp_to_reg(data, temp);
> > lm75_write_value(client, LM75_REG_TEMP[nr], data->temp[nr]);
> > mutex_unlock(&data->update_lock);
> > return count;
David, could you please resubmit the rest of the patch set rebased on the
latest version of your first patch ? I will apply 2/5 manually for now for
test purpose.
Best regards,
--
Laurent Pinchart
CSE Semaphore Belgium
Chaussee de Bruxelles, 732A
B-1410 Waterloo
Belgium
T +32 (2) 387 42 59
F +32 (2) 387 42 75
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: not available
Url : http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20080421/f8c3aa95/attachment.bin
