* [PATCH v3 0/4] Lm90 Enhancements @ 2013-07-12 7:48 Wei Ni 2013-07-12 7:48 ` [PATCH v3 1/4] hwmon: (lm90) split set&show temp as common codes Wei Ni ` (3 more replies) 0 siblings, 4 replies; 48+ messages in thread From: Wei Ni @ 2013-07-12 7:48 UTC (permalink / raw) To: khali, linux, thierry.reding Cc: lm-sensors, linux-kernel, linux-tegra, Wei Ni This patch set enhance the lm90 driver, it make the driver more readable and easier to use thermal framework. This series is v2, previous version patches: [RFC]: http://thread.gmane.org/gmane.linux.power-management.general/31056 [v1]: http://thread.gmane.org/gmane.linux.ports.tegra/11710/ [v2]: http://www.mail-archive.com/linux-kernel@vger.kernel.org/msg465555.html Changes from v2: 1. update the defines for status bit, and go into a separate patch. 2. introduce the new lm90_is_tripped() for lm90_irq_thread and lm90_alert. Changes from v1: 1. change the string "irq" to "IRQ" 2. add macro defines for the alarm status 3. consider the shared IRQ. Changes from RFC: 1. change _show_temp() to read_temp(), _set_temp() to write_temp(). 2. simply return value for the read_temp(), not use pointer. 3. use devm_request_threaded_irq() to request irq and set flag IRQF_ONESHOT. Wei Ni (4): hwmon: (lm90) split set&show temp as common codes hwmon: (lm90) use macro defines for the status bit hwmon: (lm90) add support to handle IRQ hwmon: (lm90) use enums for the indexes of temp8 and temp11 drivers/hwmon/lm90.c | 387 +++++++++++++++++++++++++++++++++----------------- 1 file changed, 256 insertions(+), 131 deletions(-) -- 1.7.9.5 ^ permalink raw reply [flat|nested] 48+ messages in thread
* [PATCH v3 1/4] hwmon: (lm90) split set&show temp as common codes 2013-07-12 7:48 [PATCH v3 0/4] Lm90 Enhancements Wei Ni @ 2013-07-12 7:48 ` Wei Ni 2013-07-12 13:26 ` Jean Delvare 2013-07-12 7:48 ` [PATCH v3 2/4] hwmon: (lm90) use macro defines for the status bit Wei Ni ` (2 subsequent siblings) 3 siblings, 1 reply; 48+ messages in thread From: Wei Ni @ 2013-07-12 7:48 UTC (permalink / raw) To: khali, linux, thierry.reding Cc: lm-sensors, linux-kernel, linux-tegra, Wei Ni Split set&show temp codes as common functions, so we can use it directly when implement linux thermal framework. Signed-off-by: Wei Ni <wni@nvidia.com> --- drivers/hwmon/lm90.c | 112 +++++++++++++++++++++++++++++++------------------- 1 file changed, 69 insertions(+), 43 deletions(-) diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c index 8eeb141..5f30f90 100644 --- a/drivers/hwmon/lm90.c +++ b/drivers/hwmon/lm90.c @@ -702,29 +702,34 @@ static u16 temp_to_u16_adt7461(struct lm90_data *data, long val) * Sysfs stuff */ -static ssize_t show_temp8(struct device *dev, struct device_attribute *devattr, - char *buf) +static int read_temp8(struct device *dev, int index) { - struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr); struct lm90_data *data = lm90_update_device(dev); int temp; if (data->kind == adt7461) - temp = temp_from_u8_adt7461(data, data->temp8[attr->index]); + temp = temp_from_u8_adt7461(data, data->temp8[index]); else if (data->kind == max6646) - temp = temp_from_u8(data->temp8[attr->index]); + temp = temp_from_u8(data->temp8[index]); else - temp = temp_from_s8(data->temp8[attr->index]); + temp = temp_from_s8(data->temp8[index]); /* +16 degrees offset for temp2 for the LM99 */ - if (data->kind == lm99 && attr->index == 3) + if (data->kind == lm99 && index == 3) temp += 16000; - return sprintf(buf, "%d\n", temp); + return temp; } -static ssize_t set_temp8(struct device *dev, struct device_attribute *devattr, - const char *buf, size_t count) +static ssize_t show_temp8(struct device *dev, struct device_attribute *devattr, + char *buf) +{ + struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr); + + return sprintf(buf, "%d\n", read_temp8(dev, attr->index)); +} + +static void write_temp8(struct device *dev, int index, long val) { static const u8 reg[8] = { LM90_REG_W_LOCAL_LOW, @@ -737,60 +742,73 @@ static ssize_t set_temp8(struct device *dev, struct device_attribute *devattr, MAX6659_REG_W_REMOTE_EMERG, }; - struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr); struct i2c_client *client = to_i2c_client(dev); struct lm90_data *data = i2c_get_clientdata(client); - int nr = attr->index; - long val; - int err; - - err = kstrtol(buf, 10, &val); - if (err < 0) - return err; /* +16 degrees offset for temp2 for the LM99 */ - if (data->kind == lm99 && attr->index == 3) + if (data->kind == lm99 && index == 3) val -= 16000; mutex_lock(&data->update_lock); if (data->kind == adt7461) - data->temp8[nr] = temp_to_u8_adt7461(data, val); + data->temp8[index] = temp_to_u8_adt7461(data, val); else if (data->kind == max6646) - data->temp8[nr] = temp_to_u8(val); + data->temp8[index] = temp_to_u8(val); else - data->temp8[nr] = temp_to_s8(val); + data->temp8[index] = temp_to_s8(val); - lm90_select_remote_channel(client, data, nr >= 6); - i2c_smbus_write_byte_data(client, reg[nr], data->temp8[nr]); + lm90_select_remote_channel(client, data, index >= 6); + i2c_smbus_write_byte_data(client, reg[index], data->temp8[index]); lm90_select_remote_channel(client, data, 0); mutex_unlock(&data->update_lock); +} + +static ssize_t set_temp8(struct device *dev, struct device_attribute *devattr, + const char *buf, size_t count) +{ + struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr); + int index = attr->index; + long val; + int err; + + err = kstrtol(buf, 10, &val); + if (err < 0) + return err; + + write_temp8(dev, index, val); + return count; } -static ssize_t show_temp11(struct device *dev, struct device_attribute *devattr, - char *buf) +static int read_temp11(struct device *dev, int index) { - struct sensor_device_attribute_2 *attr = to_sensor_dev_attr_2(devattr); struct lm90_data *data = lm90_update_device(dev); int temp; if (data->kind == adt7461) - temp = temp_from_u16_adt7461(data, data->temp11[attr->index]); + temp = temp_from_u16_adt7461(data, data->temp11[index]); else if (data->kind == max6646) - temp = temp_from_u16(data->temp11[attr->index]); + temp = temp_from_u16(data->temp11[index]); else - temp = temp_from_s16(data->temp11[attr->index]); + temp = temp_from_s16(data->temp11[index]); /* +16 degrees offset for temp2 for the LM99 */ - if (data->kind == lm99 && attr->index <= 2) + if (data->kind == lm99 && index <= 2) temp += 16000; - return sprintf(buf, "%d\n", temp); + return temp; } -static ssize_t set_temp11(struct device *dev, struct device_attribute *devattr, - const char *buf, size_t count) +static ssize_t show_temp11(struct device *dev, struct device_attribute *devattr, + char *buf) +{ + struct sensor_device_attribute_2 *attr = to_sensor_dev_attr_2(devattr); + + return sprintf(buf, "%d\n", read_temp11(dev, attr->index)); +} + +static void write_temp11(struct device *dev, int nr, int index, long val) { struct { u8 high; @@ -804,17 +822,8 @@ static ssize_t set_temp11(struct device *dev, struct device_attribute *devattr, { LM90_REG_W_REMOTE_HIGHH, LM90_REG_W_REMOTE_HIGHL, 1 } }; - struct sensor_device_attribute_2 *attr = to_sensor_dev_attr_2(devattr); struct i2c_client *client = to_i2c_client(dev); struct lm90_data *data = i2c_get_clientdata(client); - int nr = attr->nr; - int index = attr->index; - long val; - int err; - - err = kstrtol(buf, 10, &val); - if (err < 0) - return err; /* +16 degrees offset for temp2 for the LM99 */ if (data->kind == lm99 && index <= 2) @@ -839,6 +848,23 @@ static ssize_t set_temp11(struct device *dev, struct device_attribute *devattr, lm90_select_remote_channel(client, data, 0); mutex_unlock(&data->update_lock); +} + +static ssize_t set_temp11(struct device *dev, struct device_attribute *devattr, + const char *buf, size_t count) +{ + struct sensor_device_attribute_2 *attr = to_sensor_dev_attr_2(devattr); + int nr = attr->nr; + int index = attr->index; + long val; + int err; + + err = kstrtol(buf, 10, &val); + if (err < 0) + return err; + + write_temp11(dev, nr, index, val); + return count; } -- 1.7.9.5 ^ permalink raw reply related [flat|nested] 48+ messages in thread
* Re: [PATCH v3 1/4] hwmon: (lm90) split set&show temp as common codes 2013-07-12 7:48 ` [PATCH v3 1/4] hwmon: (lm90) split set&show temp as common codes Wei Ni @ 2013-07-12 13:26 ` Jean Delvare 2013-07-12 13:50 ` Guenter Roeck 2013-07-15 6:05 ` Wei Ni 0 siblings, 2 replies; 48+ messages in thread From: Jean Delvare @ 2013-07-12 13:26 UTC (permalink / raw) To: Wei Ni; +Cc: linux, thierry.reding, lm-sensors, linux-kernel, linux-tegra Hi Wei, Guenter, Guenter, thanks for reviewing the previous version of this patch. Wei, thanks for incorporating review feedback and posting updated patches so quickly, this is very appreciated, even though I'm too busy these days to be that fast on my end, sorry about that. On Fri, 12 Jul 2013 15:48:04 +0800, Wei Ni wrote: > Split set&show temp codes as common functions, so we can use it directly when > implement linux thermal framework. Can I see a recent version of the code which will need this change? It makes no sense to apply this first part which makes the code more complex with no benefit, without the second part which needs it, so they should be applied together or not at all. One thing I am a little worried about (but maybe I'm wrong) is that I seem to understand you want to register every LM90-like chip as both a hwmon device and two thermal devices. I seem to recall that every thermal device is also exposed automatically as a virtual hwmon device, is that correct? If so we will be presenting the same values twice to libsensors, which would be confusing. > Signed-off-by: Wei Ni <wni@nvidia.com> > --- > drivers/hwmon/lm90.c | 112 +++++++++++++++++++++++++++++++------------------- > 1 file changed, 69 insertions(+), 43 deletions(-) The code changes look good, however I have one suggestion for improvement (plus a minor cleanup request): > diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c > index 8eeb141..5f30f90 100644 > --- a/drivers/hwmon/lm90.c > +++ b/drivers/hwmon/lm90.c > (...) > -static ssize_t set_temp8(struct device *dev, struct device_attribute *devattr, > - const char *buf, size_t count) > (...) > +static void write_temp8(struct device *dev, int index, long val) > { > static const u8 reg[8] = { > LM90_REG_W_LOCAL_LOW, > @@ -737,60 +742,73 @@ static ssize_t set_temp8(struct device *dev, struct device_attribute *devattr, > MAX6659_REG_W_REMOTE_EMERG, > }; > > - struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr); > struct i2c_client *client = to_i2c_client(dev); > struct lm90_data *data = i2c_get_clientdata(client); > - int nr = attr->index; > - long val; > - int err; > - > - err = kstrtol(buf, 10, &val); > - if (err < 0) > - return err; > > /* +16 degrees offset for temp2 for the LM99 */ > - if (data->kind == lm99 && attr->index == 3) > + if (data->kind == lm99 && index == 3) > val -= 16000; > > mutex_lock(&data->update_lock); > if (data->kind == adt7461) > - data->temp8[nr] = temp_to_u8_adt7461(data, val); > + data->temp8[index] = temp_to_u8_adt7461(data, val); > else if (data->kind == max6646) > - data->temp8[nr] = temp_to_u8(val); > + data->temp8[index] = temp_to_u8(val); > else > - data->temp8[nr] = temp_to_s8(val); > + data->temp8[index] = temp_to_s8(val); > > - lm90_select_remote_channel(client, data, nr >= 6); > - i2c_smbus_write_byte_data(client, reg[nr], data->temp8[nr]); > + lm90_select_remote_channel(client, data, index >= 6); > + i2c_smbus_write_byte_data(client, reg[index], data->temp8[index]); This write could fail. So far the lm90 driver has failed to handle register write errors at all, and I will take the blame for that. But if we want to integrate properly with the thermal subsystem, I suspect we will have to properly report errors. So it might be the right time to catch and return write errors here. Then set_temp8() below could return it to user-space (either in this patch or in a separate patch, as you prefer.) And then as a next step, lm90_select_remote_channel should return errors as they happen as well, so that they can be transmitted to the caller. > lm90_select_remote_channel(client, data, 0); > > mutex_unlock(&data->update_lock); > +} > + > +static ssize_t set_temp8(struct device *dev, struct device_attribute *devattr, > + const char *buf, size_t count) > +{ > + struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr); > + int index = attr->index; > + long val; > + int err; > + > + err = kstrtol(buf, 10, &val); > + if (err < 0) > + return err; > + > + write_temp8(dev, index, val); > + > return count; > } > > -static ssize_t show_temp11(struct device *dev, struct device_attribute *devattr, > - char *buf) > +static int read_temp11(struct device *dev, int index) > { > - struct sensor_device_attribute_2 *attr = to_sensor_dev_attr_2(devattr); > struct lm90_data *data = lm90_update_device(dev); > int temp; > > if (data->kind == adt7461) > - temp = temp_from_u16_adt7461(data, data->temp11[attr->index]); > + temp = temp_from_u16_adt7461(data, data->temp11[index]); > else if (data->kind == max6646) > - temp = temp_from_u16(data->temp11[attr->index]); > + temp = temp_from_u16(data->temp11[index]); > else > - temp = temp_from_s16(data->temp11[attr->index]); > + temp = temp_from_s16(data->temp11[index]); > > /* +16 degrees offset for temp2 for the LM99 */ > - if (data->kind == lm99 && attr->index <= 2) > + if (data->kind == lm99 && index <= 2) There's a doubled space on this line. It isn't added by your patch, it was already there before, but please fix it while you're here. > temp += 16000; > > - return sprintf(buf, "%d\n", temp); > + return temp; > } > > -static ssize_t set_temp11(struct device *dev, struct device_attribute *devattr, > - const char *buf, size_t count) > (...) > +static void write_temp11(struct device *dev, int nr, int index, long val) Here too I would suggest returning errors from the I2C layer, and handling them in set_temp11() now or later. > { > struct { > u8 high; > @@ -804,17 +822,8 @@ static ssize_t set_temp11(struct device *dev, struct device_attribute *devattr, > { LM90_REG_W_REMOTE_HIGHH, LM90_REG_W_REMOTE_HIGHL, 1 } > }; > > - struct sensor_device_attribute_2 *attr = to_sensor_dev_attr_2(devattr); > struct i2c_client *client = to_i2c_client(dev); > struct lm90_data *data = i2c_get_clientdata(client); > - int nr = attr->nr; > - int index = attr->index; > - long val; > - int err; > - > - err = kstrtol(buf, 10, &val); > - if (err < 0) > - return err; > > /* +16 degrees offset for temp2 for the LM99 */ > if (data->kind == lm99 && index <= 2) > @@ -839,6 +848,23 @@ static ssize_t set_temp11(struct device *dev, struct device_attribute *devattr, > lm90_select_remote_channel(client, data, 0); > > mutex_unlock(&data->update_lock); > +} > + > +static ssize_t set_temp11(struct device *dev, struct device_attribute *devattr, > + const char *buf, size_t count) > +{ > + struct sensor_device_attribute_2 *attr = to_sensor_dev_attr_2(devattr); > + int nr = attr->nr; > + int index = attr->index; > + long val; > + int err; > + > + err = kstrtol(buf, 10, &val); > + if (err < 0) > + return err; > + > + write_temp11(dev, nr, index, val); > + > return count; > } > -- Jean Delvare ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v3 1/4] hwmon: (lm90) split set&show temp as common codes 2013-07-12 13:26 ` Jean Delvare @ 2013-07-12 13:50 ` Guenter Roeck 2013-07-12 14:30 ` Jean Delvare 2013-07-15 6:05 ` Wei Ni 1 sibling, 1 reply; 48+ messages in thread From: Guenter Roeck @ 2013-07-12 13:50 UTC (permalink / raw) To: Jean Delvare Cc: Wei Ni, thierry.reding, lm-sensors, linux-kernel, linux-tegra On Fri, Jul 12, 2013 at 03:26:15PM +0200, Jean Delvare wrote: > Hi Wei, Guenter, > > Guenter, thanks for reviewing the previous version of this patch. > > Wei, thanks for incorporating review feedback and posting updated > patches so quickly, this is very appreciated, even though I'm too busy > these days to be that fast on my end, sorry about that. > > On Fri, 12 Jul 2013 15:48:04 +0800, Wei Ni wrote: > > Split set&show temp codes as common functions, so we can use it directly when > > implement linux thermal framework. > > Can I see a recent version of the code which will need this change? It > makes no sense to apply this first part which makes the code more > complex with no benefit, without the second part which needs it, so > they should be applied together or not at all. > > One thing I am a little worried about (but maybe I'm wrong) is that I > seem to understand you want to register every LM90-like chip as both a > hwmon device and two thermal devices. I seem to recall that every > thermal device is also exposed automatically as a virtual hwmon > device, is that correct? If so we will be presenting the same values > twice to libsensors, which would be confusing. > Not sure if that is a good idea, but if I recall correctly, the thermal folks plan to remove that path. Guenter > > Signed-off-by: Wei Ni <wni@nvidia.com> > > --- > > drivers/hwmon/lm90.c | 112 +++++++++++++++++++++++++++++++------------------- > > 1 file changed, 69 insertions(+), 43 deletions(-) > > The code changes look good, however I have one suggestion for > improvement (plus a minor cleanup request): > > > diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c > > index 8eeb141..5f30f90 100644 > > --- a/drivers/hwmon/lm90.c > > +++ b/drivers/hwmon/lm90.c > > (...) > > -static ssize_t set_temp8(struct device *dev, struct device_attribute *devattr, > > - const char *buf, size_t count) > > (...) > > +static void write_temp8(struct device *dev, int index, long val) > > { > > static const u8 reg[8] = { > > LM90_REG_W_LOCAL_LOW, > > @@ -737,60 +742,73 @@ static ssize_t set_temp8(struct device *dev, struct device_attribute *devattr, > > MAX6659_REG_W_REMOTE_EMERG, > > }; > > > > - struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr); > > struct i2c_client *client = to_i2c_client(dev); > > struct lm90_data *data = i2c_get_clientdata(client); > > - int nr = attr->index; > > - long val; > > - int err; > > - > > - err = kstrtol(buf, 10, &val); > > - if (err < 0) > > - return err; > > > > /* +16 degrees offset for temp2 for the LM99 */ > > - if (data->kind == lm99 && attr->index == 3) > > + if (data->kind == lm99 && index == 3) > > val -= 16000; > > > > mutex_lock(&data->update_lock); > > if (data->kind == adt7461) > > - data->temp8[nr] = temp_to_u8_adt7461(data, val); > > + data->temp8[index] = temp_to_u8_adt7461(data, val); > > else if (data->kind == max6646) > > - data->temp8[nr] = temp_to_u8(val); > > + data->temp8[index] = temp_to_u8(val); > > else > > - data->temp8[nr] = temp_to_s8(val); > > + data->temp8[index] = temp_to_s8(val); > > > > - lm90_select_remote_channel(client, data, nr >= 6); > > - i2c_smbus_write_byte_data(client, reg[nr], data->temp8[nr]); > > + lm90_select_remote_channel(client, data, index >= 6); > > + i2c_smbus_write_byte_data(client, reg[index], data->temp8[index]); > > This write could fail. So far the lm90 driver has failed to handle > register write errors at all, and I will take the blame for that. But > if we want to integrate properly with the thermal subsystem, I suspect > we will have to properly report errors. So it might be the right time > to catch and return write errors here. Then set_temp8() below could > return it to user-space (either in this patch or in a separate patch, > as you prefer.) > > And then as a next step, lm90_select_remote_channel should return > errors as they happen as well, so that they can be transmitted to the > caller. > > > lm90_select_remote_channel(client, data, 0); > > > > mutex_unlock(&data->update_lock); > > +} > > + > > +static ssize_t set_temp8(struct device *dev, struct device_attribute *devattr, > > + const char *buf, size_t count) > > +{ > > + struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr); > > + int index = attr->index; > > + long val; > > + int err; > > + > > + err = kstrtol(buf, 10, &val); > > + if (err < 0) > > + return err; > > + > > + write_temp8(dev, index, val); > > + > > return count; > > } > > > > -static ssize_t show_temp11(struct device *dev, struct device_attribute *devattr, > > - char *buf) > > +static int read_temp11(struct device *dev, int index) > > { > > - struct sensor_device_attribute_2 *attr = to_sensor_dev_attr_2(devattr); > > struct lm90_data *data = lm90_update_device(dev); > > int temp; > > > > if (data->kind == adt7461) > > - temp = temp_from_u16_adt7461(data, data->temp11[attr->index]); > > + temp = temp_from_u16_adt7461(data, data->temp11[index]); > > else if (data->kind == max6646) > > - temp = temp_from_u16(data->temp11[attr->index]); > > + temp = temp_from_u16(data->temp11[index]); > > else > > - temp = temp_from_s16(data->temp11[attr->index]); > > + temp = temp_from_s16(data->temp11[index]); > > > > /* +16 degrees offset for temp2 for the LM99 */ > > - if (data->kind == lm99 && attr->index <= 2) > > + if (data->kind == lm99 && index <= 2) > > There's a doubled space on this line. It isn't added by your patch, it > was already there before, but please fix it while you're here. > > > temp += 16000; > > > > - return sprintf(buf, "%d\n", temp); > > + return temp; > > } > > > > -static ssize_t set_temp11(struct device *dev, struct device_attribute *devattr, > > - const char *buf, size_t count) > > (...) > > +static void write_temp11(struct device *dev, int nr, int index, long val) > > Here too I would suggest returning errors from the I2C layer, and > handling them in set_temp11() now or later. > > > { > > struct { > > u8 high; > > @@ -804,17 +822,8 @@ static ssize_t set_temp11(struct device *dev, struct device_attribute *devattr, > > { LM90_REG_W_REMOTE_HIGHH, LM90_REG_W_REMOTE_HIGHL, 1 } > > }; > > > > - struct sensor_device_attribute_2 *attr = to_sensor_dev_attr_2(devattr); > > struct i2c_client *client = to_i2c_client(dev); > > struct lm90_data *data = i2c_get_clientdata(client); > > - int nr = attr->nr; > > - int index = attr->index; > > - long val; > > - int err; > > - > > - err = kstrtol(buf, 10, &val); > > - if (err < 0) > > - return err; > > > > /* +16 degrees offset for temp2 for the LM99 */ > > if (data->kind == lm99 && index <= 2) > > @@ -839,6 +848,23 @@ static ssize_t set_temp11(struct device *dev, struct device_attribute *devattr, > > lm90_select_remote_channel(client, data, 0); > > > > mutex_unlock(&data->update_lock); > > +} > > + > > +static ssize_t set_temp11(struct device *dev, struct device_attribute *devattr, > > + const char *buf, size_t count) > > +{ > > + struct sensor_device_attribute_2 *attr = to_sensor_dev_attr_2(devattr); > > + int nr = attr->nr; > > + int index = attr->index; > > + long val; > > + int err; > > + > > + err = kstrtol(buf, 10, &val); > > + if (err < 0) > > + return err; > > + > > + write_temp11(dev, nr, index, val); > > + > > return count; > > } > > > > -- > Jean Delvare > ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v3 1/4] hwmon: (lm90) split set&show temp as common codes 2013-07-12 13:50 ` Guenter Roeck @ 2013-07-12 14:30 ` Jean Delvare 2013-07-12 14:40 ` Guenter Roeck 0 siblings, 1 reply; 48+ messages in thread From: Jean Delvare @ 2013-07-12 14:30 UTC (permalink / raw) To: Guenter Roeck Cc: Wei Ni, thierry.reding, lm-sensors, linux-kernel, linux-tegra Hi Guenter, On Fri, 12 Jul 2013 06:50:00 -0700, Guenter Roeck wrote: > On Fri, Jul 12, 2013 at 03:26:15PM +0200, Jean Delvare wrote: > > One thing I am a little worried about (but maybe I'm wrong) is that I > > seem to understand you want to register every LM90-like chip as both a > > hwmon device and two thermal devices. I seem to recall that every > > thermal device is also exposed automatically as a virtual hwmon > > device, is that correct? If so we will be presenting the same values > > twice to libsensors, which would be confusing. > > Not sure if that is a good idea, but if I recall correctly, the thermal folks > plan to remove that path. If that means that for example the ACPI thermal zone is no longer displayed by "sensors", then I strongly object - unless it is explicitly registered as a separate hwmon device from now on, of course. My idea was to make the bridge optional - you decide when you register a thermal device if it should be exposed as hwmon or not. I don't have a strong opinion on the implementation, as long as each input is listed by "sensors" once and only once. -- Jean Delvare ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v3 1/4] hwmon: (lm90) split set&show temp as common codes 2013-07-12 14:30 ` Jean Delvare @ 2013-07-12 14:40 ` Guenter Roeck 2013-07-15 6:25 ` Wei Ni 0 siblings, 1 reply; 48+ messages in thread From: Guenter Roeck @ 2013-07-12 14:40 UTC (permalink / raw) To: Jean Delvare Cc: Wei Ni, thierry.reding, lm-sensors, linux-kernel, linux-tegra On Fri, Jul 12, 2013 at 04:30:34PM +0200, Jean Delvare wrote: > Hi Guenter, > > On Fri, 12 Jul 2013 06:50:00 -0700, Guenter Roeck wrote: > > On Fri, Jul 12, 2013 at 03:26:15PM +0200, Jean Delvare wrote: > > > One thing I am a little worried about (but maybe I'm wrong) is that I > > > seem to understand you want to register every LM90-like chip as both a > > > hwmon device and two thermal devices. I seem to recall that every > > > thermal device is also exposed automatically as a virtual hwmon > > > device, is that correct? If so we will be presenting the same values > > > twice to libsensors, which would be confusing. > > > > Not sure if that is a good idea, but if I recall correctly, the thermal folks > > plan to remove that path. > > If that means that for example the ACPI thermal zone is no longer > displayed by "sensors", then I strongly object - unless it is > explicitly registered as a separate hwmon device from now on, of course. > If I recall correctly that was the idea. Of course, in practice that will mean that devices will _not_ get exposed as hwmon devices, as implementers won't bother doing both. > My idea was to make the bridge optional - you decide when you register > a thermal device if it should be exposed as hwmon or not. > Yes, that would be a much better solution. > I don't have a strong opinion on the implementation, as long as each > input is listed by "sensors" once and only once. > Agreed. Guenter ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v3 1/4] hwmon: (lm90) split set&show temp as common codes 2013-07-12 14:40 ` Guenter Roeck @ 2013-07-15 6:25 ` Wei Ni 2013-07-15 7:24 ` Jean Delvare 0 siblings, 1 reply; 48+ messages in thread From: Wei Ni @ 2013-07-15 6:25 UTC (permalink / raw) To: Guenter Roeck, rui.zhang, Jean Delvare Cc: thierry.reding, lm-sensors, linux-pm, linux-kernel, linux-tegra On 07/12/2013 10:40 PM, Guenter Roeck wrote: > On Fri, Jul 12, 2013 at 04:30:34PM +0200, Jean Delvare wrote: >> Hi Guenter, >> >> On Fri, 12 Jul 2013 06:50:00 -0700, Guenter Roeck wrote: >>> On Fri, Jul 12, 2013 at 03:26:15PM +0200, Jean Delvare wrote: >>>> One thing I am a little worried about (but maybe I'm wrong) is that I >>>> seem to understand you want to register every LM90-like chip as both a >>>> hwmon device and two thermal devices. I seem to recall that every >>>> thermal device is also exposed automatically as a virtual hwmon >>>> device, is that correct? If so we will be presenting the same values >>>> twice to libsensors, which would be confusing. >>> >>> Not sure if that is a good idea, but if I recall correctly, the thermal folks >>> plan to remove that path. Hi, Rui As Jean said, if we want to register the lm90 as thermal device, it will have two hwmon devices in the sysfs, one is registered by the lm90 driver, another one is registered by the thermal_zone_device_register(), this would be confusing. Do you have any ideas for it? Thanks. Wei. >> >> If that means that for example the ACPI thermal zone is no longer >> displayed by "sensors", then I strongly object - unless it is >> explicitly registered as a separate hwmon device from now on, of course. >> > If I recall correctly that was the idea. Of course, in practice that will mean > that devices will _not_ get exposed as hwmon devices, as implementers won't > bother doing both. > >> My idea was to make the bridge optional - you decide when you register >> a thermal device if it should be exposed as hwmon or not. >> > Yes, that would be a much better solution. I think we can decide it in the DT, we can add a dt property in the lm90 device node, such as: sys-interface = SYS_HWMON; or sys-interface = SYS_THERMAL; So we register it as the hwmon or thermal device > >> I don't have a strong opinion on the implementation, as long as each >> input is listed by "sensors" once and only once. >> > Agreed. > > Guenter > ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v3 1/4] hwmon: (lm90) split set&show temp as common codes 2013-07-15 6:25 ` Wei Ni @ 2013-07-15 7:24 ` Jean Delvare 2013-07-15 9:14 ` Wei Ni 2013-07-17 4:26 ` Thierry Reding 0 siblings, 2 replies; 48+ messages in thread From: Jean Delvare @ 2013-07-15 7:24 UTC (permalink / raw) To: Wei Ni Cc: Guenter Roeck, rui.zhang, thierry.reding, lm-sensors, linux-pm, linux-kernel, linux-tegra On Mon, 15 Jul 2013 14:25:29 +0800, Wei Ni wrote: > On 07/12/2013 10:40 PM, Guenter Roeck wrote: > > On Fri, Jul 12, 2013 at 04:30:34PM +0200, Jean Delvare wrote: > >> If that means that for example the ACPI thermal zone is no longer > >> displayed by "sensors", then I strongly object - unless it is > >> explicitly registered as a separate hwmon device from now on, of course. > > > > If I recall correctly that was the idea. Of course, in practice that will mean > > that devices will _not_ get exposed as hwmon devices, as implementers won't > > bother doing both. > > > >> My idea was to make the bridge optional - you decide when you register > >> a thermal device if it should be exposed as hwmon or not. > > > > Yes, that would be a much better solution. > > I think we can decide it in the DT, we can add a dt property in the lm90 > device node, such as: > sys-interface = SYS_HWMON; > or > sys-interface = SYS_THERMAL; > So we register it as the hwmon or thermal device This is an option, but please keep in mind that DT is not the only way to instantiate LM90-like devices, and we should not expose duplicate inputs by default. It is fine with me if the default is to create only a HWMON device (as the lm90 driver was doing so far), and only DT-instantiated devices have the choice. Another option, as discussed before, would be that the thermal devices registered by lm90 are specifically tagged as "do not expose as hwmon". This would avoid the duplicate hwmon inputs in all cases. Again - no strong opinion on the implementation, as long as it does the right thing. Oh, and we'll have to be careful with the Kconfig dependencies. I do not want the lm90 driver to depend on the thermal framework. -- Jean Delvare ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v3 1/4] hwmon: (lm90) split set&show temp as common codes 2013-07-15 7:24 ` Jean Delvare @ 2013-07-15 9:14 ` Wei Ni 2013-07-15 17:52 ` Jean Delvare 2013-07-17 4:26 ` Thierry Reding 1 sibling, 1 reply; 48+ messages in thread From: Wei Ni @ 2013-07-15 9:14 UTC (permalink / raw) To: Jean Delvare, rui.zhang Cc: Guenter Roeck, thierry.reding, lm-sensors, linux-pm, linux-kernel, linux-tegra On 07/15/2013 03:24 PM, Jean Delvare wrote: > On Mon, 15 Jul 2013 14:25:29 +0800, Wei Ni wrote: >> On 07/12/2013 10:40 PM, Guenter Roeck wrote: >>> On Fri, Jul 12, 2013 at 04:30:34PM +0200, Jean Delvare wrote: >>>> If that means that for example the ACPI thermal zone is no longer >>>> displayed by "sensors", then I strongly object - unless it is >>>> explicitly registered as a separate hwmon device from now on, of course. >>> >>> If I recall correctly that was the idea. Of course, in practice that will mean >>> that devices will _not_ get exposed as hwmon devices, as implementers won't >>> bother doing both. >>> >>>> My idea was to make the bridge optional - you decide when you register >>>> a thermal device if it should be exposed as hwmon or not. >>> >>> Yes, that would be a much better solution. >> >> I think we can decide it in the DT, we can add a dt property in the lm90 >> device node, such as: >> sys-interface = SYS_HWMON; >> or >> sys-interface = SYS_THERMAL; >> So we register it as the hwmon or thermal device > > This is an option, but please keep in mind that DT is not the only way > to instantiate LM90-like devices, and we should not expose duplicate > inputs by default. It is fine with me if the default is to create only a > HWMON device (as the lm90 driver was doing so far), and only > DT-instantiated devices have the choice. Yes, we should not expose duplicate inputs, we may have tree permutation: 1. only hwmon device: for this items, we just need to call hwmon_device_register(). 2. only thermal device + virtual hwmon device: for this items, we just need to call thermal_zone_device_register(). We can set #1 as the default, and if use DT, we provide option to choice #1 or #2. 3. hwmon device + thermal device: for this items, we doesn't need the virtual hwmon which registered by the thermal fw, how to handle this one? Add flag when register as thermal device to indicate if want virtual hwmon device or not, something like your below another option. > > Another option, as discussed before, would be that the thermal devices > registered by lm90 are specifically tagged as "do not expose as hwmon". > This would avoid the duplicate hwmon inputs in all cases. It seems in current thermal fw, it can't be tagged as "do not expose as hwmon", we need to add flag when register thermal device. Rui, what do you think for it? > > Again - no strong opinion on the implementation, as long as it does the > right thing. I'm working on the Tegra platform, we uses nct1008 as the temperature sensor, and we want to register it as thermal device, so that we can run the throttle function. So I prepared these patches to enhance this driver. > > Oh, and we'll have to be careful with the Kconfig dependencies. I do > not want the lm90 driver to depend on the thermal framework. Yes, absolutely agree, lm90 should be independent. Indeed, the thermal folks is trying to restructure the thermal fw, and in this new fw, it's more easy to add the generic sensors to the thermal fw. If you interest it, please refer http://thread.gmane.org/gmane.linux.power-management.general/30692. Thanks. Wei. > ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v3 1/4] hwmon: (lm90) split set&show temp as common codes 2013-07-15 9:14 ` Wei Ni @ 2013-07-15 17:52 ` Jean Delvare 0 siblings, 0 replies; 48+ messages in thread From: Jean Delvare @ 2013-07-15 17:52 UTC (permalink / raw) To: Wei Ni Cc: rui.zhang, Guenter Roeck, thierry.reding, lm-sensors, linux-pm, linux-kernel, linux-tegra Hi Wei, On Mon, 15 Jul 2013 17:14:07 +0800, Wei Ni wrote: > On 07/15/2013 03:24 PM, Jean Delvare wrote: > > On Mon, 15 Jul 2013 14:25:29 +0800, Wei Ni wrote: > >> I think we can decide it in the DT, we can add a dt property in the lm90 > >> device node, such as: > >> sys-interface = SYS_HWMON; > >> or > >> sys-interface = SYS_THERMAL; > >> So we register it as the hwmon or thermal device > > > > This is an option, but please keep in mind that DT is not the only way > > to instantiate LM90-like devices, and we should not expose duplicate > > inputs by default. It is fine with me if the default is to create only a > > HWMON device (as the lm90 driver was doing so far), and only > > DT-instantiated devices have the choice. > > Yes, we should not expose duplicate inputs, we may have tree permutation: > 1. only hwmon device: > for this items, we just need to call hwmon_device_register(). > 2. only thermal device + virtual hwmon device: > for this items, we just need to call thermal_zone_device_register(). > > We can set #1 as the default, and if use DT, we provide option to choice > #1 or #2. #2 makes little sense IMHO, for a driver which properly implements hwmon support. The point of the virtual hwmon device created for thermal zones was to not put an extra burden on thermal driver authors by asking them to additionally implement the hwmon interface. But the hwmon interface it richer than the thermal interface in some respects so native hwmon implementations are preferred when available. Thus I think your option #3 below is what we want in addition to #1, and we don't need #2. > 3. hwmon device + thermal device: > for this items, we doesn't need the virtual hwmon which registered by > the thermal fw, how to handle this one? Add flag when register as > thermal device to indicate if want virtual hwmon device or not, > something like your below another option. -- Jean Delvare ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v3 1/4] hwmon: (lm90) split set&show temp as common codes 2013-07-15 7:24 ` Jean Delvare 2013-07-15 9:14 ` Wei Ni @ 2013-07-17 4:26 ` Thierry Reding 2013-07-17 5:14 ` Guenter Roeck 1 sibling, 1 reply; 48+ messages in thread From: Thierry Reding @ 2013-07-17 4:26 UTC (permalink / raw) To: Jean Delvare Cc: Wei Ni, Guenter Roeck, rui.zhang, lm-sensors, linux-pm, linux-kernel, linux-tegra [-- Attachment #1: Type: text/plain, Size: 1830 bytes --] On Mon, Jul 15, 2013 at 09:24:15AM +0200, Jean Delvare wrote: > On Mon, 15 Jul 2013 14:25:29 +0800, Wei Ni wrote: > > On 07/12/2013 10:40 PM, Guenter Roeck wrote: > > > On Fri, Jul 12, 2013 at 04:30:34PM +0200, Jean Delvare wrote: > > >> If that means that for example the ACPI thermal zone is no longer > > >> displayed by "sensors", then I strongly object - unless it is > > >> explicitly registered as a separate hwmon device from now on, of course. > > > > > > If I recall correctly that was the idea. Of course, in practice that will mean > > > that devices will _not_ get exposed as hwmon devices, as implementers won't > > > bother doing both. > > > > > >> My idea was to make the bridge optional - you decide when you register > > >> a thermal device if it should be exposed as hwmon or not. > > > > > > Yes, that would be a much better solution. > > > > I think we can decide it in the DT, we can add a dt property in the lm90 > > device node, such as: > > sys-interface = SYS_HWMON; > > or > > sys-interface = SYS_THERMAL; > > So we register it as the hwmon or thermal device > > This is an option, but please keep in mind that DT is not the only way > to instantiate LM90-like devices, and we should not expose duplicate > inputs by default. It is fine with me if the default is to create only a > HWMON device (as the lm90 driver was doing so far), and only > DT-instantiated devices have the choice. I don't think this information belongs in the device tree. Whether the device is exposed as hwmon or thermal device (or both) is entirely a software issue whereas DT is a means to describe the hardware. It seems to me that the earlier proposal of communicating to the bridge whether or not a device should be exposed as hwmon device is the right thing to do here. Thierry [-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --] ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v3 1/4] hwmon: (lm90) split set&show temp as common codes 2013-07-17 4:26 ` Thierry Reding @ 2013-07-17 5:14 ` Guenter Roeck 2013-07-17 6:26 ` Wei Ni 0 siblings, 1 reply; 48+ messages in thread From: Guenter Roeck @ 2013-07-17 5:14 UTC (permalink / raw) To: Thierry Reding Cc: Jean Delvare, Wei Ni, rui.zhang, lm-sensors, linux-pm, linux-kernel, linux-tegra On Wed, Jul 17, 2013 at 06:26:20AM +0200, Thierry Reding wrote: > On Mon, Jul 15, 2013 at 09:24:15AM +0200, Jean Delvare wrote: > > On Mon, 15 Jul 2013 14:25:29 +0800, Wei Ni wrote: > > > On 07/12/2013 10:40 PM, Guenter Roeck wrote: > > > > On Fri, Jul 12, 2013 at 04:30:34PM +0200, Jean Delvare wrote: > > > >> If that means that for example the ACPI thermal zone is no longer > > > >> displayed by "sensors", then I strongly object - unless it is > > > >> explicitly registered as a separate hwmon device from now on, of course. > > > > > > > > If I recall correctly that was the idea. Of course, in practice that will mean > > > > that devices will _not_ get exposed as hwmon devices, as implementers won't > > > > bother doing both. > > > > > > > >> My idea was to make the bridge optional - you decide when you register > > > >> a thermal device if it should be exposed as hwmon or not. > > > > > > > > Yes, that would be a much better solution. > > > > > > I think we can decide it in the DT, we can add a dt property in the lm90 > > > device node, such as: > > > sys-interface = SYS_HWMON; > > > or > > > sys-interface = SYS_THERMAL; > > > So we register it as the hwmon or thermal device > > > > This is an option, but please keep in mind that DT is not the only way > > to instantiate LM90-like devices, and we should not expose duplicate > > inputs by default. It is fine with me if the default is to create only a > > HWMON device (as the lm90 driver was doing so far), and only > > DT-instantiated devices have the choice. > > I don't think this information belongs in the device tree. Whether the > device is exposed as hwmon or thermal device (or both) is entirely a > software issue whereas DT is a means to describe the hardware. > Correct; this is exactly the type of information which does _not_ belong int devicetree. > It seems to me that the earlier proposal of communicating to the bridge > whether or not a device should be exposed as hwmon device is the right > thing to do here. > Agreed.. Guenter ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v3 1/4] hwmon: (lm90) split set&show temp as common codes 2013-07-17 5:14 ` Guenter Roeck @ 2013-07-17 6:26 ` Wei Ni 2013-07-17 9:11 ` Jean Delvare 0 siblings, 1 reply; 48+ messages in thread From: Wei Ni @ 2013-07-17 6:26 UTC (permalink / raw) To: Guenter Roeck Cc: Thierry Reding, Jean Delvare, rui.zhang, lm-sensors, linux-pm, linux-kernel, linux-tegra On 07/17/2013 01:14 PM, Guenter Roeck wrote: > On Wed, Jul 17, 2013 at 06:26:20AM +0200, Thierry Reding wrote: >> On Mon, Jul 15, 2013 at 09:24:15AM +0200, Jean Delvare wrote: >>> On Mon, 15 Jul 2013 14:25:29 +0800, Wei Ni wrote: >>>> On 07/12/2013 10:40 PM, Guenter Roeck wrote: >>>>> On Fri, Jul 12, 2013 at 04:30:34PM +0200, Jean Delvare wrote: >>>>>> If that means that for example the ACPI thermal zone is no longer >>>>>> displayed by "sensors", then I strongly object - unless it is >>>>>> explicitly registered as a separate hwmon device from now on, of course. >>>>> >>>>> If I recall correctly that was the idea. Of course, in practice that will mean >>>>> that devices will _not_ get exposed as hwmon devices, as implementers won't >>>>> bother doing both. >>>>> >>>>>> My idea was to make the bridge optional - you decide when you register >>>>>> a thermal device if it should be exposed as hwmon or not. >>>>> >>>>> Yes, that would be a much better solution. >>>> >>>> I think we can decide it in the DT, we can add a dt property in the lm90 >>>> device node, such as: >>>> sys-interface = SYS_HWMON; >>>> or >>>> sys-interface = SYS_THERMAL; >>>> So we register it as the hwmon or thermal device >>> >>> This is an option, but please keep in mind that DT is not the only way >>> to instantiate LM90-like devices, and we should not expose duplicate >>> inputs by default. It is fine with me if the default is to create only a >>> HWMON device (as the lm90 driver was doing so far), and only >>> DT-instantiated devices have the choice. >> >> I don't think this information belongs in the device tree. Whether the >> device is exposed as hwmon or thermal device (or both) is entirely a >> software issue whereas DT is a means to describe the hardware. >> > Correct; this is exactly the type of information which does _not_ belong int > devicetree. > >> It seems to me that the earlier proposal of communicating to the bridge >> whether or not a device should be exposed as hwmon device is the right >> thing to do here. >> > Agreed.. Sorry, what's the "bridge" mean, does it mean we need to add a flag in the thermal_zone_device_register() to indicate if we want to register virtual hwmon device or not? Thanks. Wei. > > Guenter > ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v3 1/4] hwmon: (lm90) split set&show temp as common codes 2013-07-17 6:26 ` Wei Ni @ 2013-07-17 9:11 ` Jean Delvare 2013-07-17 9:54 ` Wei Ni 0 siblings, 1 reply; 48+ messages in thread From: Jean Delvare @ 2013-07-17 9:11 UTC (permalink / raw) To: Wei Ni Cc: Guenter Roeck, Thierry Reding, rui.zhang, lm-sensors, linux-pm, linux-kernel, linux-tegra On Wed, 17 Jul 2013 14:26:54 +0800, Wei Ni wrote: > On 07/17/2013 01:14 PM, Guenter Roeck wrote: > > On Wed, Jul 17, 2013 at 06:26:20AM +0200, Thierry Reding wrote: > >> On Mon, Jul 15, 2013 at 09:24:15AM +0200, Jean Delvare wrote: > >>> On Mon, 15 Jul 2013 14:25:29 +0800, Wei Ni wrote: > >>>> I think we can decide it in the DT, we can add a dt property in the lm90 > >>>> device node, such as: > >>>> sys-interface = SYS_HWMON; > >>>> or > >>>> sys-interface = SYS_THERMAL; > >>>> So we register it as the hwmon or thermal device > >>> > >>> This is an option, but please keep in mind that DT is not the only way > >>> to instantiate LM90-like devices, and we should not expose duplicate > >>> inputs by default. It is fine with me if the default is to create only a > >>> HWMON device (as the lm90 driver was doing so far), and only > >>> DT-instantiated devices have the choice. > >> > >> I don't think this information belongs in the device tree. Whether the > >> device is exposed as hwmon or thermal device (or both) is entirely a > >> software issue whereas DT is a means to describe the hardware. > >> > > Correct; this is exactly the type of information which does _not_ belong int > > devicetree. > > > >> It seems to me that the earlier proposal of communicating to the bridge > >> whether or not a device should be exposed as hwmon device is the right > >> thing to do here. > > > > Agreed.. > > Sorry, what's the "bridge" mean, The code which creates a virtual hwmon input when a new thermal zone is registered (this code is in thermal_core.c.) > does it mean we need to add a flag in > the thermal_zone_device_register() to indicate if we want to register > virtual hwmon device or not? I think so, yes. Alternatively the flag could be added to struct thermal_zone_device_ops, so that you don't have to update all the callers. But I admit it's a hack as the flag doesn't really belong there, so I suppose we don't really want to do that. I have been thinking of an automatic approach, based on comparing the type string passed to thermal_zone_device_register() with already registered hwmon devices, but I couldn't come up with something good and robust enough, so let's forget about it. -- Jean Delvare ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v3 1/4] hwmon: (lm90) split set&show temp as common codes 2013-07-17 9:11 ` Jean Delvare @ 2013-07-17 9:54 ` Wei Ni 0 siblings, 0 replies; 48+ messages in thread From: Wei Ni @ 2013-07-17 9:54 UTC (permalink / raw) To: Jean Delvare Cc: Guenter Roeck, Thierry Reding, rui.zhang, lm-sensors, linux-pm, linux-kernel, linux-tegra On 07/17/2013 05:11 PM, Jean Delvare wrote: > On Wed, 17 Jul 2013 14:26:54 +0800, Wei Ni wrote: >> On 07/17/2013 01:14 PM, Guenter Roeck wrote: >>> On Wed, Jul 17, 2013 at 06:26:20AM +0200, Thierry Reding wrote: >>>> On Mon, Jul 15, 2013 at 09:24:15AM +0200, Jean Delvare wrote: >>>>> On Mon, 15 Jul 2013 14:25:29 +0800, Wei Ni wrote: >>>>>> I think we can decide it in the DT, we can add a dt property in the lm90 >>>>>> device node, such as: >>>>>> sys-interface = SYS_HWMON; >>>>>> or >>>>>> sys-interface = SYS_THERMAL; >>>>>> So we register it as the hwmon or thermal device >>>>> >>>>> This is an option, but please keep in mind that DT is not the only way >>>>> to instantiate LM90-like devices, and we should not expose duplicate >>>>> inputs by default. It is fine with me if the default is to create only a >>>>> HWMON device (as the lm90 driver was doing so far), and only >>>>> DT-instantiated devices have the choice. >>>> >>>> I don't think this information belongs in the device tree. Whether the >>>> device is exposed as hwmon or thermal device (or both) is entirely a >>>> software issue whereas DT is a means to describe the hardware. >>>> >>> Correct; this is exactly the type of information which does _not_ belong int >>> devicetree. >>> >>>> It seems to me that the earlier proposal of communicating to the bridge >>>> whether or not a device should be exposed as hwmon device is the right >>>> thing to do here. >>> >>> Agreed.. >> >> Sorry, what's the "bridge" mean, > > The code which creates a virtual hwmon input when a new thermal zone is > registered (this code is in thermal_core.c.) > >> does it mean we need to add a flag in >> the thermal_zone_device_register() to indicate if we want to register >> virtual hwmon device or not? > > I think so, yes. > > Alternatively the flag could be added to struct > thermal_zone_device_ops, so that you don't have to update all the > callers. But I admit it's a hack as the flag doesn't really belong > there, so I suppose we don't really want to do that. I think it's better to add it to struct thermal_zone_params, the thermal_zone_device_ops is for the callback functions. And I ask it with thermal fw engineers in http://thread.gmane.org/gmane.linux.power-management.general/35874. May be you can look it. > > I have been thinking of an automatic approach, based on comparing the > type string passed to thermal_zone_device_register() with already > registered hwmon devices, but I couldn't come up with something good > and robust enough, so let's forget about it. > ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v3 1/4] hwmon: (lm90) split set&show temp as common codes 2013-07-12 13:26 ` Jean Delvare 2013-07-12 13:50 ` Guenter Roeck @ 2013-07-15 6:05 ` Wei Ni 2013-07-15 7:29 ` Jean Delvare 1 sibling, 1 reply; 48+ messages in thread From: Wei Ni @ 2013-07-15 6:05 UTC (permalink / raw) To: Jean Delvare; +Cc: linux, thierry.reding, lm-sensors, linux-kernel, linux-tegra On 07/12/2013 09:26 PM, Jean Delvare wrote: > Hi Wei, Guenter, > > Guenter, thanks for reviewing the previous version of this patch. > > Wei, thanks for incorporating review feedback and posting updated > patches so quickly, this is very appreciated, even though I'm too busy > these days to be that fast on my end, sorry about that. > > On Fri, 12 Jul 2013 15:48:04 +0800, Wei Ni wrote: >> Split set&show temp codes as common functions, so we can use it directly when >> implement linux thermal framework. > > Can I see a recent version of the code which will need this change? It > makes no sense to apply this first part which makes the code more > complex with no benefit, without the second part which needs it, so > they should be applied together or not at all. In my RFC patches, there had many codes about thermal fw, which need this patch, so I put them together. And now I split the RFC patches, this series is preparing to use the thermal fw. As you said, I want to register lm90 as the thermal zone device, it need to hook some callback, such as .get_temp. if apply this patch, I can write the .get_temp simply, something like: +static int lm90_read_temp2_temp(struct thermal_zone_device *thz, unsigned long *temp) +{ + struct lm90_data *data = thz->devdata; + struct i2c_client *client = to_i2c_client(data->hwmon_dev->parent); + struct device *dev = &client->dev;+ + + *temp = (long)read_temp11(dev, TEMP11_REMOTE_TEMP); + + return 0; +} + +static struct thermal_zone_device_ops remote_ops = { + .get_temp = lm90_read_temp2_temp, +}; If without this patch, I have to rewrite the lm90_read_temp2_temp(), which almost same as the show_temp11(), I think it's not good. When use this patch and following 3/3 patch, the code will be more readable and clear. Anyway, if you want, I can send this patch as a separate one. :) > > One thing I am a little worried about (but maybe I'm wrong) is that I > seem to understand you want to register every LM90-like chip as both a > hwmon device and two thermal devices. I seem to recall that every > thermal device is also exposed automatically as a virtual hwmon > device, is that correct? If so we will be presenting the same values > twice to libsensors, which would be confusing. > >> Signed-off-by: Wei Ni <wni@nvidia.com> >> --- >> drivers/hwmon/lm90.c | 112 +++++++++++++++++++++++++++++++------------------- >> 1 file changed, 69 insertions(+), 43 deletions(-) > > The code changes look good, however I have one suggestion for > improvement (plus a minor cleanup request): > >> diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c >> index 8eeb141..5f30f90 100644 >> --- a/drivers/hwmon/lm90.c >> +++ b/drivers/hwmon/lm90.c >> (...) >> -static ssize_t set_temp8(struct device *dev, struct device_attribute *devattr, >> - const char *buf, size_t count) >> (...) >> +static void write_temp8(struct device *dev, int index, long val) >> { >> static const u8 reg[8] = { >> LM90_REG_W_LOCAL_LOW, >> @@ -737,60 +742,73 @@ static ssize_t set_temp8(struct device *dev, struct device_attribute *devattr, >> MAX6659_REG_W_REMOTE_EMERG, >> }; >> >> - struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr); >> struct i2c_client *client = to_i2c_client(dev); >> struct lm90_data *data = i2c_get_clientdata(client); >> - int nr = attr->index; >> - long val; >> - int err; >> - >> - err = kstrtol(buf, 10, &val); >> - if (err < 0) >> - return err; >> >> /* +16 degrees offset for temp2 for the LM99 */ >> - if (data->kind == lm99 && attr->index == 3) >> + if (data->kind == lm99 && index == 3) >> val -= 16000; >> >> mutex_lock(&data->update_lock); >> if (data->kind == adt7461) >> - data->temp8[nr] = temp_to_u8_adt7461(data, val); >> + data->temp8[index] = temp_to_u8_adt7461(data, val); >> else if (data->kind == max6646) >> - data->temp8[nr] = temp_to_u8(val); >> + data->temp8[index] = temp_to_u8(val); >> else >> - data->temp8[nr] = temp_to_s8(val); >> + data->temp8[index] = temp_to_s8(val); >> >> - lm90_select_remote_channel(client, data, nr >= 6); >> - i2c_smbus_write_byte_data(client, reg[nr], data->temp8[nr]); >> + lm90_select_remote_channel(client, data, index >= 6); >> + i2c_smbus_write_byte_data(client, reg[index], data->temp8[index]); > > This write could fail. So far the lm90 driver has failed to handle > register write errors at all, and I will take the blame for that. But > if we want to integrate properly with the thermal subsystem, I suspect > we will have to properly report errors. So it might be the right time > to catch and return write errors here. Then set_temp8() below could > return it to user-space (either in this patch or in a separate patch, > as you prefer.) Ok, I will add error handler in my next version. > > And then as a next step, lm90_select_remote_channel should return > errors as they happen as well, so that they can be transmitted to the > caller. > >> lm90_select_remote_channel(client, data, 0); >> >> mutex_unlock(&data->update_lock); >> +} >> + >> +static ssize_t set_temp8(struct device *dev, struct device_attribute *devattr, >> + const char *buf, size_t count) >> +{ >> + struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr); >> + int index = attr->index; >> + long val; >> + int err; >> + >> + err = kstrtol(buf, 10, &val); >> + if (err < 0) >> + return err; >> + >> + write_temp8(dev, index, val); >> + >> return count; >> } >> >> -static ssize_t show_temp11(struct device *dev, struct device_attribute *devattr, >> - char *buf) >> +static int read_temp11(struct device *dev, int index) >> { >> - struct sensor_device_attribute_2 *attr = to_sensor_dev_attr_2(devattr); >> struct lm90_data *data = lm90_update_device(dev); >> int temp; >> >> if (data->kind == adt7461) >> - temp = temp_from_u16_adt7461(data, data->temp11[attr->index]); >> + temp = temp_from_u16_adt7461(data, data->temp11[index]); >> else if (data->kind == max6646) >> - temp = temp_from_u16(data->temp11[attr->index]); >> + temp = temp_from_u16(data->temp11[index]); >> else >> - temp = temp_from_s16(data->temp11[attr->index]); >> + temp = temp_from_s16(data->temp11[index]); >> >> /* +16 degrees offset for temp2 for the LM99 */ >> - if (data->kind == lm99 && attr->index <= 2) >> + if (data->kind == lm99 && index <= 2) > > There's a doubled space on this line. It isn't added by your patch, it > was already there before, but please fix it while you're here. Oh, you are so carefully, I will fix it :) > >> temp += 16000; >> >> - return sprintf(buf, "%d\n", temp); >> + return temp; >> } >> >> -static ssize_t set_temp11(struct device *dev, struct device_attribute *devattr, >> - const char *buf, size_t count) >> (...) >> +static void write_temp11(struct device *dev, int nr, int index, long val) > > Here too I would suggest returning errors from the I2C layer, and > handling them in set_temp11() now or later. > >> { >> struct { >> u8 high; >> @@ -804,17 +822,8 @@ static ssize_t set_temp11(struct device *dev, struct device_attribute *devattr, >> { LM90_REG_W_REMOTE_HIGHH, LM90_REG_W_REMOTE_HIGHL, 1 } >> }; >> >> - struct sensor_device_attribute_2 *attr = to_sensor_dev_attr_2(devattr); >> struct i2c_client *client = to_i2c_client(dev); >> struct lm90_data *data = i2c_get_clientdata(client); >> - int nr = attr->nr; >> - int index = attr->index; >> - long val; >> - int err; >> - >> - err = kstrtol(buf, 10, &val); >> - if (err < 0) >> - return err; >> >> /* +16 degrees offset for temp2 for the LM99 */ >> if (data->kind == lm99 && index <= 2) >> @@ -839,6 +848,23 @@ static ssize_t set_temp11(struct device *dev, struct device_attribute *devattr, >> lm90_select_remote_channel(client, data, 0); >> >> mutex_unlock(&data->update_lock); >> +} >> + >> +static ssize_t set_temp11(struct device *dev, struct device_attribute *devattr, >> + const char *buf, size_t count) >> +{ >> + struct sensor_device_attribute_2 *attr = to_sensor_dev_attr_2(devattr); >> + int nr = attr->nr; >> + int index = attr->index; >> + long val; >> + int err; >> + >> + err = kstrtol(buf, 10, &val); >> + if (err < 0) >> + return err; >> + >> + write_temp11(dev, nr, index, val); >> + >> return count; >> } >> > ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v3 1/4] hwmon: (lm90) split set&show temp as common codes 2013-07-15 6:05 ` Wei Ni @ 2013-07-15 7:29 ` Jean Delvare 0 siblings, 0 replies; 48+ messages in thread From: Jean Delvare @ 2013-07-15 7:29 UTC (permalink / raw) To: Wei Ni; +Cc: linux, thierry.reding, lm-sensors, linux-kernel, linux-tegra Hi Wei, On Mon, 15 Jul 2013 14:05:08 +0800, Wei Ni wrote: > On 07/12/2013 09:26 PM, Jean Delvare wrote: > > Can I see a recent version of the code which will need this change? It > > makes no sense to apply this first part which makes the code more > > complex with no benefit, without the second part which needs it, so > > they should be applied together or not at all. > > In my RFC patches, there had many codes about thermal fw, which need > this patch, so I put them together. > And now I split the RFC patches, this series is preparing to use the > thermal fw. > As you said, I want to register lm90 as the thermal zone device, it need > to hook some callback, such as .get_temp. if apply this patch, I can > write the .get_temp simply, something like: > > +static int lm90_read_temp2_temp(struct thermal_zone_device *thz, > unsigned long *temp) > +{ > + struct lm90_data *data = thz->devdata; > + struct i2c_client *client = to_i2c_client(data->hwmon_dev->parent); > + struct device *dev = &client->dev;+ > + > + *temp = (long)read_temp11(dev, TEMP11_REMOTE_TEMP); > + > + return 0; > +} > + > +static struct thermal_zone_device_ops remote_ops = { > + .get_temp = lm90_read_temp2_temp, > +}; > > If without this patch, I have to rewrite the lm90_read_temp2_temp(), > which almost same as the show_temp11(), I think it's not good. When use > this patch and following 3/3 patch, the code will be more readable and > clear. I understand the idea. > Anyway, if you want, I can send this patch as a separate one. :) Yes please, I think it would help me do a better code review and testing as well. -- Jean Delvare ^ permalink raw reply [flat|nested] 48+ messages in thread
* [PATCH v3 2/4] hwmon: (lm90) use macro defines for the status bit 2013-07-12 7:48 [PATCH v3 0/4] Lm90 Enhancements Wei Ni 2013-07-12 7:48 ` [PATCH v3 1/4] hwmon: (lm90) split set&show temp as common codes Wei Ni @ 2013-07-12 7:48 ` Wei Ni 2013-07-15 16:57 ` Jean Delvare 2013-07-12 7:48 ` [PATCH v3 3/4] hwmon: (lm90) add support to handle IRQ Wei Ni 2013-07-12 7:48 ` [PATCH v3 4/4] hwmon: (lm90) use enums for the indexes of temp8 and temp11 Wei Ni 3 siblings, 1 reply; 48+ messages in thread From: Wei Ni @ 2013-07-12 7:48 UTC (permalink / raw) To: khali, linux, thierry.reding Cc: lm-sensors, linux-kernel, linux-tegra, Wei Ni Add bit defines for the status register. Signed-off-by: Wei Ni <wni@nvidia.com> --- drivers/hwmon/lm90.c | 72 ++++++++++++++++++++++++++++++++++---------------- 1 file changed, 49 insertions(+), 23 deletions(-) diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c index 5f30f90..c90037f 100644 --- a/drivers/hwmon/lm90.c +++ b/drivers/hwmon/lm90.c @@ -179,6 +179,19 @@ enum chips { lm90, adm1032, lm99, lm86, max6657, max6659, adt7461, max6680, #define LM90_HAVE_TEMP3 (1 << 6) /* 3rd temperature sensor */ #define LM90_HAVE_BROKEN_ALERT (1 << 7) /* Broken alert */ +/* LM90 status */ +#define LM90_STATUS_LTHRM (1 << 0) /* local THERM limit tripped */ +#define LM90_STATUS_RTHRM (1 << 1) /* remote THERM limit tripped */ +#define LM90_STATUS_OPEN (1 << 2) /* remote is an open circuit */ +#define LM90_STATUS_RLOW (1 << 3) /* remote low temp limit tripped */ +#define LM90_STATUS_RHIGH (1 << 4) /* remote high temp limit tripped */ +#define LM90_STATUS_LLOW (1 << 5) /* local low temp limit tripped */ +#define LM90_STATUS_LHIGH (1 << 6) /* local high temp limit tripped */ +#define LM90_STATUS_BUSY (1 << 7) /* ADC is converting */ + +#define MAX6696_STATUS2_RLOW (1 << 3) /* remote2 low temp limit tripped */ +#define MAX6696_STATUS2_RHIGH (1 << 4) /* remote2 high temp limit tripped */ + /* * Driver data (common to all clients) */ @@ -1417,6 +1430,36 @@ static void lm90_init_client(struct i2c_client *client) i2c_smbus_write_byte_data(client, LM90_REG_W_CONFIG1, config); } +static bool lm90_is_tripped(struct i2c_client *client) +{ + struct lm90_data *data = i2c_get_clientdata(client); + u8 status, status2 = 0; + + lm90_read_reg(client, LM90_REG_R_STATUS, &status); + + if (data->kind == max6696) + lm90_read_reg(client, MAX6696_REG_R_STATUS2, &status2); + + if ((status & 0x7f) == 0 && (status2 & 0xfe) == 0) + return false; + + if (status & (LM90_STATUS_LLOW | LM90_STATUS_LHIGH | LM90_STATUS_LTHRM)) + dev_warn(&client->dev, + "temp%d out of range, please check!\n", 1); + if (status & (LM90_STATUS_RLOW | LM90_STATUS_RHIGH | LM90_STATUS_RTHRM)) + dev_warn(&client->dev, + "temp%d out of range, please check!\n", 2); + if (status & LM90_STATUS_OPEN) + dev_warn(&client->dev, + "temp%d diode open, please check!\n", 2); + + if (status2 & (MAX6696_STATUS2_RLOW | MAX6696_STATUS2_RHIGH)) + dev_warn(&client->dev, + "temp%d out of range, please check!\n", 3); + + return true; +} + static int lm90_probe(struct i2c_client *client, const struct i2c_device_id *id) { @@ -1515,36 +1558,19 @@ static int lm90_remove(struct i2c_client *client) static void lm90_alert(struct i2c_client *client, unsigned int flag) { - struct lm90_data *data = i2c_get_clientdata(client); - u8 config, alarms, alarms2 = 0; - - lm90_read_reg(client, LM90_REG_R_STATUS, &alarms); - - if (data->kind == max6696) - lm90_read_reg(client, MAX6696_REG_R_STATUS2, &alarms2); - - if ((alarms & 0x7f) == 0 && (alarms2 & 0xfe) == 0) { + if (!lm90_is_tripped(client)) { dev_info(&client->dev, "Everything OK\n"); } else { - if (alarms & 0x61) - dev_warn(&client->dev, - "temp%d out of range, please check!\n", 1); - if (alarms & 0x1a) - dev_warn(&client->dev, - "temp%d out of range, please check!\n", 2); - if (alarms & 0x04) - dev_warn(&client->dev, - "temp%d diode open, please check!\n", 2); - - if (alarms2 & 0x18) - dev_warn(&client->dev, - "temp%d out of range, please check!\n", 3); - /* * Disable ALERT# output, because these chips don't implement * SMBus alert correctly; they should only hold the alert line * low briefly. */ + struct lm90_data *data = i2c_get_clientdata(client); + u8 config, alarms; + + lm90_read_reg(client, LM90_REG_R_STATUS, &alarms); + if ((data->flags & LM90_HAVE_BROKEN_ALERT) && (alarms & data->alert_alarms)) { dev_dbg(&client->dev, "Disabling ALERT#\n"); -- 1.7.9.5 ^ permalink raw reply related [flat|nested] 48+ messages in thread
* Re: [PATCH v3 2/4] hwmon: (lm90) use macro defines for the status bit 2013-07-12 7:48 ` [PATCH v3 2/4] hwmon: (lm90) use macro defines for the status bit Wei Ni @ 2013-07-15 16:57 ` Jean Delvare 2013-07-15 17:33 ` Guenter Roeck 2013-07-17 7:03 ` Wei Ni 0 siblings, 2 replies; 48+ messages in thread From: Jean Delvare @ 2013-07-15 16:57 UTC (permalink / raw) To: Wei Ni, Guenter Roeck Cc: thierry.reding, lm-sensors, linux-kernel, linux-tegra Hi Wei, Guenter, On Fri, 12 Jul 2013 15:48:05 +0800, Wei Ni wrote: > Add bit defines for the status register. Regarding the subject: for me these are constants, not macros. AFAIK the term "macro" refers to defines with parameters only. > Signed-off-by: Wei Ni <wni@nvidia.com> > --- > drivers/hwmon/lm90.c | 72 ++++++++++++++++++++++++++++++++++---------------- > 1 file changed, 49 insertions(+), 23 deletions(-) > > diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c > index 5f30f90..c90037f 100644 > --- a/drivers/hwmon/lm90.c > +++ b/drivers/hwmon/lm90.c > @@ -179,6 +179,19 @@ enum chips { lm90, adm1032, lm99, lm86, max6657, max6659, adt7461, max6680, > #define LM90_HAVE_TEMP3 (1 << 6) /* 3rd temperature sensor */ > #define LM90_HAVE_BROKEN_ALERT (1 << 7) /* Broken alert */ > > +/* LM90 status */ > +#define LM90_STATUS_LTHRM (1 << 0) /* local THERM limit tripped */ > +#define LM90_STATUS_RTHRM (1 << 1) /* remote THERM limit tripped */ > +#define LM90_STATUS_OPEN (1 << 2) /* remote is an open circuit */ > +#define LM90_STATUS_RLOW (1 << 3) /* remote low temp limit tripped */ > +#define LM90_STATUS_RHIGH (1 << 4) /* remote high temp limit tripped */ > +#define LM90_STATUS_LLOW (1 << 5) /* local low temp limit tripped */ > +#define LM90_STATUS_LHIGH (1 << 6) /* local high temp limit tripped */ > +#define LM90_STATUS_BUSY (1 << 7) /* ADC is converting */ LM90_STATUS_BUSY is never used anywhere so please don't define it. > + > +#define MAX6696_STATUS2_RLOW (1 << 3) /* remote2 low temp limit tripped */ > +#define MAX6696_STATUS2_RHIGH (1 << 4) /* remote2 high temp limit tripped */ > + > /* > * Driver data (common to all clients) > */ > @@ -1417,6 +1430,36 @@ static void lm90_init_client(struct i2c_client *client) > i2c_smbus_write_byte_data(client, LM90_REG_W_CONFIG1, config); > } > > +static bool lm90_is_tripped(struct i2c_client *client) > +{ > + struct lm90_data *data = i2c_get_clientdata(client); > + u8 status, status2 = 0; > + > + lm90_read_reg(client, LM90_REG_R_STATUS, &status); > + > + if (data->kind == max6696) > + lm90_read_reg(client, MAX6696_REG_R_STATUS2, &status2); > + > + if ((status & 0x7f) == 0 && (status2 & 0xfe) == 0) > + return false; It's a bit disappointing to not use the freshly introduced constants. That being said I agree it would make the code hard to read, so you can leave it as is. Unrelated to this patch, but Guenter, I am worried about the MAX6696 handling here. I realize that I am the one who accepted your code, but now it looks wrong. Specifically: * We check for (status2 & 0xfe) i.e. 7 alarm bits, but the code below only reports 2 alarms bits. So if any of the 5 other alarm bits in STATUS2 are, we may return true (chip is tripped) but not print the cause. * At least bits 1 and 2 of STATUS 2 fit totally fine in the driver as it currently exists, so I can't think of any reason for not handling them. Why are we not? Ideally we should print a message for every alarm bit so that we never return "true" without printing a message. Even though OT2 limits aren't handled by the driver... * If you think this piece of code shouldn't deal with OT/THERM limits because they do not trigger an SMBus alarm, this can be discussed, but all chips should be handled the same in this respect then. * Why in the first place is max6696's data->alert_alarms set to 0x187c and not 0x1c7c? Including 1OPEN but not 2OPEN makes no sense. > + > + if (status & (LM90_STATUS_LLOW | LM90_STATUS_LHIGH | LM90_STATUS_LTHRM)) > + dev_warn(&client->dev, > + "temp%d out of range, please check!\n", 1); > + if (status & (LM90_STATUS_RLOW | LM90_STATUS_RHIGH | LM90_STATUS_RTHRM)) > + dev_warn(&client->dev, > + "temp%d out of range, please check!\n", 2); > + if (status & LM90_STATUS_OPEN) > + dev_warn(&client->dev, > + "temp%d diode open, please check!\n", 2); > + > + if (status2 & (MAX6696_STATUS2_RLOW | MAX6696_STATUS2_RHIGH)) > + dev_warn(&client->dev, > + "temp%d out of range, please check!\n", 3); > + > + return true; > +} > + > static int lm90_probe(struct i2c_client *client, > const struct i2c_device_id *id) > { > @@ -1515,36 +1558,19 @@ static int lm90_remove(struct i2c_client *client) > > static void lm90_alert(struct i2c_client *client, unsigned int flag) > { > - struct lm90_data *data = i2c_get_clientdata(client); > - u8 config, alarms, alarms2 = 0; > - > - lm90_read_reg(client, LM90_REG_R_STATUS, &alarms); > - > - if (data->kind == max6696) > - lm90_read_reg(client, MAX6696_REG_R_STATUS2, &alarms2); > - > - if ((alarms & 0x7f) == 0 && (alarms2 & 0xfe) == 0) { > + if (!lm90_is_tripped(client)) { You could swap the success and failure cases to avoid this negation. > dev_info(&client->dev, "Everything OK\n"); > } else { > - if (alarms & 0x61) > - dev_warn(&client->dev, > - "temp%d out of range, please check!\n", 1); > - if (alarms & 0x1a) > - dev_warn(&client->dev, > - "temp%d out of range, please check!\n", 2); > - if (alarms & 0x04) > - dev_warn(&client->dev, > - "temp%d diode open, please check!\n", 2); > - > - if (alarms2 & 0x18) > - dev_warn(&client->dev, > - "temp%d out of range, please check!\n", 3); > - > /* > * Disable ALERT# output, because these chips don't implement > * SMBus alert correctly; they should only hold the alert line > * low briefly. > */ > + struct lm90_data *data = i2c_get_clientdata(client); > + u8 config, alarms; > + > + lm90_read_reg(client, LM90_REG_R_STATUS, &alarms); You end up reading LM90_REG_R_STATUS, which is not OK. This register contains self-clearing bits, so there is no guarantee that the second read will return the same value as the first read. You'll have to come up with a different approach that reads LM90_REG_R_STATUS only once. > + > if ((data->flags & LM90_HAVE_BROKEN_ALERT) > && (alarms & data->alert_alarms)) { > dev_dbg(&client->dev, "Disabling ALERT#\n"); -- Jean Delvare ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v3 2/4] hwmon: (lm90) use macro defines for the status bit 2013-07-15 16:57 ` Jean Delvare @ 2013-07-15 17:33 ` Guenter Roeck 2013-10-30 15:33 ` Jean Delvare 2013-07-17 7:03 ` Wei Ni 1 sibling, 1 reply; 48+ messages in thread From: Guenter Roeck @ 2013-07-15 17:33 UTC (permalink / raw) To: Jean Delvare Cc: Wei Ni, thierry.reding, lm-sensors, linux-kernel, linux-tegra On Mon, Jul 15, 2013 at 06:57:27PM +0200, Jean Delvare wrote: > Hi Wei, Guenter, > > On Fri, 12 Jul 2013 15:48:05 +0800, Wei Ni wrote: > > Add bit defines for the status register. > > Regarding the subject: for me these are constants, not macros. AFAIK > the term "macro" refers to defines with parameters only. > > > Signed-off-by: Wei Ni <wni@nvidia.com> > > --- > > drivers/hwmon/lm90.c | 72 ++++++++++++++++++++++++++++++++++---------------- > > 1 file changed, 49 insertions(+), 23 deletions(-) > > > > diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c > > index 5f30f90..c90037f 100644 > > --- a/drivers/hwmon/lm90.c > > +++ b/drivers/hwmon/lm90.c > > @@ -179,6 +179,19 @@ enum chips { lm90, adm1032, lm99, lm86, max6657, max6659, adt7461, max6680, > > #define LM90_HAVE_TEMP3 (1 << 6) /* 3rd temperature sensor */ > > #define LM90_HAVE_BROKEN_ALERT (1 << 7) /* Broken alert */ > > > > +/* LM90 status */ > > +#define LM90_STATUS_LTHRM (1 << 0) /* local THERM limit tripped */ > > +#define LM90_STATUS_RTHRM (1 << 1) /* remote THERM limit tripped */ > > +#define LM90_STATUS_OPEN (1 << 2) /* remote is an open circuit */ > > +#define LM90_STATUS_RLOW (1 << 3) /* remote low temp limit tripped */ > > +#define LM90_STATUS_RHIGH (1 << 4) /* remote high temp limit tripped */ > > +#define LM90_STATUS_LLOW (1 << 5) /* local low temp limit tripped */ > > +#define LM90_STATUS_LHIGH (1 << 6) /* local high temp limit tripped */ > > +#define LM90_STATUS_BUSY (1 << 7) /* ADC is converting */ > > LM90_STATUS_BUSY is never used anywhere so please don't define it. > > > + > > +#define MAX6696_STATUS2_RLOW (1 << 3) /* remote2 low temp limit tripped */ > > +#define MAX6696_STATUS2_RHIGH (1 << 4) /* remote2 high temp limit tripped */ > > + > > /* > > * Driver data (common to all clients) > > */ > > @@ -1417,6 +1430,36 @@ static void lm90_init_client(struct i2c_client *client) > > i2c_smbus_write_byte_data(client, LM90_REG_W_CONFIG1, config); > > } > > > > +static bool lm90_is_tripped(struct i2c_client *client) > > +{ > > + struct lm90_data *data = i2c_get_clientdata(client); > > + u8 status, status2 = 0; > > + > > + lm90_read_reg(client, LM90_REG_R_STATUS, &status); > > + > > + if (data->kind == max6696) > > + lm90_read_reg(client, MAX6696_REG_R_STATUS2, &status2); > > + > > + if ((status & 0x7f) == 0 && (status2 & 0xfe) == 0) > > + return false; > > It's a bit disappointing to not use the freshly introduced constants. > That being said I agree it would make the code hard to read, so you can > leave it as is. > > Unrelated to this patch, but Guenter, I am worried about the MAX6696 > handling here. I realize that I am the one who accepted your code, but > now it looks wrong. Specifically: > * We check for (status2 & 0xfe) i.e. 7 alarm bits, but the code below > only reports 2 alarms bits. So if any of the 5 other alarm bits in > STATUS2 are, we may return true (chip is tripped) but not print the > cause. > * At least bits 1 and 2 of STATUS 2 fit totally fine in the driver as > it currently exists, so I can't think of any reason for not handling > them. Why are we not? Ideally we should print a message for every > alarm bit so that we never return "true" without printing a message. > Even though OT2 limits aren't handled by the driver... > * If you think this piece of code shouldn't deal with OT/THERM limits > because they do not trigger an SMBus alarm, this can be discussed, > but all chips should be handled the same in this respect then. > * Why in the first place is max6696's data->alert_alarms set to 0x187c > and not 0x1c7c? Including 1OPEN but not 2OPEN makes no sense. > I am about to leave for vacation, so this will have to wait for a couple of weeks. I'll look at it after I am back. Guenter > > + > > + if (status & (LM90_STATUS_LLOW | LM90_STATUS_LHIGH | LM90_STATUS_LTHRM)) > > + dev_warn(&client->dev, > > + "temp%d out of range, please check!\n", 1); > > + if (status & (LM90_STATUS_RLOW | LM90_STATUS_RHIGH | LM90_STATUS_RTHRM)) > > + dev_warn(&client->dev, > > + "temp%d out of range, please check!\n", 2); > > + if (status & LM90_STATUS_OPEN) > > + dev_warn(&client->dev, > > + "temp%d diode open, please check!\n", 2); > > + > > + if (status2 & (MAX6696_STATUS2_RLOW | MAX6696_STATUS2_RHIGH)) > > + dev_warn(&client->dev, > > + "temp%d out of range, please check!\n", 3); > > + > > + return true; > > +} > > + > > static int lm90_probe(struct i2c_client *client, > > const struct i2c_device_id *id) > > { > > @@ -1515,36 +1558,19 @@ static int lm90_remove(struct i2c_client *client) > > > > static void lm90_alert(struct i2c_client *client, unsigned int flag) > > { > > - struct lm90_data *data = i2c_get_clientdata(client); > > - u8 config, alarms, alarms2 = 0; > > - > > - lm90_read_reg(client, LM90_REG_R_STATUS, &alarms); > > - > > - if (data->kind == max6696) > > - lm90_read_reg(client, MAX6696_REG_R_STATUS2, &alarms2); > > - > > - if ((alarms & 0x7f) == 0 && (alarms2 & 0xfe) == 0) { > > + if (!lm90_is_tripped(client)) { > > You could swap the success and failure cases to avoid this negation. > > > dev_info(&client->dev, "Everything OK\n"); > > } else { > > - if (alarms & 0x61) > > - dev_warn(&client->dev, > > - "temp%d out of range, please check!\n", 1); > > - if (alarms & 0x1a) > > - dev_warn(&client->dev, > > - "temp%d out of range, please check!\n", 2); > > - if (alarms & 0x04) > > - dev_warn(&client->dev, > > - "temp%d diode open, please check!\n", 2); > > - > > - if (alarms2 & 0x18) > > - dev_warn(&client->dev, > > - "temp%d out of range, please check!\n", 3); > > - > > /* > > * Disable ALERT# output, because these chips don't implement > > * SMBus alert correctly; they should only hold the alert line > > * low briefly. > > */ > > + struct lm90_data *data = i2c_get_clientdata(client); > > + u8 config, alarms; > > + > > + lm90_read_reg(client, LM90_REG_R_STATUS, &alarms); > > You end up reading LM90_REG_R_STATUS, which is not OK. This register > contains self-clearing bits, so there is no guarantee that the second > read will return the same value as the first read. You'll have to come > up with a different approach that reads LM90_REG_R_STATUS only once. > > > + > > if ((data->flags & LM90_HAVE_BROKEN_ALERT) > > && (alarms & data->alert_alarms)) { > > dev_dbg(&client->dev, "Disabling ALERT#\n"); > > > -- > Jean Delvare > ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v3 2/4] hwmon: (lm90) use macro defines for the status bit 2013-07-15 17:33 ` Guenter Roeck @ 2013-10-30 15:33 ` Jean Delvare 2013-10-30 16:11 ` Guenter Roeck 2013-10-30 16:56 ` Guenter Roeck 0 siblings, 2 replies; 48+ messages in thread From: Jean Delvare @ 2013-10-30 15:33 UTC (permalink / raw) To: Guenter Roeck Cc: Wei Ni, thierry.reding, lm-sensors, linux-kernel, linux-tegra Hi Guenter, On Mon, 15 Jul 2013 10:33:22 -0700, Guenter Roeck wrote: > On Mon, Jul 15, 2013 at 06:57:27PM +0200, Jean Delvare wrote: > > Unrelated to this patch, but Guenter, I am worried about the MAX6696 > > handling here. I realize that I am the one who accepted your code, but > > now it looks wrong. Specifically: > > * We check for (status2 & 0xfe) i.e. 7 alarm bits, but the code below > > only reports 2 alarms bits. So if any of the 5 other alarm bits in > > STATUS2 are, we may return true (chip is tripped) but not print the > > cause. > > * At least bits 1 and 2 of STATUS 2 fit totally fine in the driver as > > it currently exists, so I can't think of any reason for not handling > > them. Why are we not? Ideally we should print a message for every > > alarm bit so that we never return "true" without printing a message. > > Even though OT2 limits aren't handled by the driver... > > * If you think this piece of code shouldn't deal with OT/THERM limits > > because they do not trigger an SMBus alarm, this can be discussed, > > but all chips should be handled the same in this respect then. > > * Why in the first place is max6696's data->alert_alarms set to 0x187c > > and not 0x1c7c? Including 1OPEN but not 2OPEN makes no sense. > > I am about to leave for vacation, so this will have to wait for a couple of > weeks. I'll look at it after I am back. Are you back now? ;-) -- Jean Delvare ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v3 2/4] hwmon: (lm90) use macro defines for the status bit 2013-10-30 15:33 ` Jean Delvare @ 2013-10-30 16:11 ` Guenter Roeck 2013-10-30 16:56 ` Guenter Roeck 1 sibling, 0 replies; 48+ messages in thread From: Guenter Roeck @ 2013-10-30 16:11 UTC (permalink / raw) To: Jean Delvare Cc: Wei Ni, thierry.reding, lm-sensors, linux-kernel, linux-tegra On Wed, Oct 30, 2013 at 04:33:26PM +0100, Jean Delvare wrote: > Hi Guenter, > > On Mon, 15 Jul 2013 10:33:22 -0700, Guenter Roeck wrote: > > On Mon, Jul 15, 2013 at 06:57:27PM +0200, Jean Delvare wrote: > > > Unrelated to this patch, but Guenter, I am worried about the MAX6696 > > > handling here. I realize that I am the one who accepted your code, but > > > now it looks wrong. Specifically: > > > * We check for (status2 & 0xfe) i.e. 7 alarm bits, but the code below > > > only reports 2 alarms bits. So if any of the 5 other alarm bits in > > > STATUS2 are, we may return true (chip is tripped) but not print the > > > cause. > > > * At least bits 1 and 2 of STATUS 2 fit totally fine in the driver as > > > it currently exists, so I can't think of any reason for not handling > > > them. Why are we not? Ideally we should print a message for every > > > alarm bit so that we never return "true" without printing a message. > > > Even though OT2 limits aren't handled by the driver... > > > * If you think this piece of code shouldn't deal with OT/THERM limits > > > because they do not trigger an SMBus alarm, this can be discussed, > > > but all chips should be handled the same in this respect then. > > > * Why in the first place is max6696's data->alert_alarms set to 0x187c > > > and not 0x1c7c? Including 1OPEN but not 2OPEN makes no sense. > > > > I am about to leave for vacation, so this will have to wait for a couple of > > weeks. I'll look at it after I am back. > > Are you back now? ;-) > Yes, only I completely forgot about this :-). I'll add it to my task list. Guenter ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v3 2/4] hwmon: (lm90) use macro defines for the status bit 2013-10-30 15:33 ` Jean Delvare 2013-10-30 16:11 ` Guenter Roeck @ 2013-10-30 16:56 ` Guenter Roeck 1 sibling, 0 replies; 48+ messages in thread From: Guenter Roeck @ 2013-10-30 16:56 UTC (permalink / raw) To: Jean Delvare Cc: Wei Ni, thierry.reding, lm-sensors, linux-kernel, linux-tegra On Wed, Oct 30, 2013 at 04:33:26PM +0100, Jean Delvare wrote: > Hi Guenter, > > On Mon, 15 Jul 2013 10:33:22 -0700, Guenter Roeck wrote: > > On Mon, Jul 15, 2013 at 06:57:27PM +0200, Jean Delvare wrote: > > > Unrelated to this patch, but Guenter, I am worried about the MAX6696 > > > handling here. I realize that I am the one who accepted your code, but > > > now it looks wrong. Specifically: > > > * We check for (status2 & 0xfe) i.e. 7 alarm bits, but the code below > > > only reports 2 alarms bits. So if any of the 5 other alarm bits in > > > STATUS2 are, we may return true (chip is tripped) but not print the > > > cause. Agreed, that doesn't make much sense, especially since we already check for R1OT1 and display a message if it is set. I'll add checks for the other bits. > > > * At least bits 1 and 2 of STATUS 2 fit totally fine in the driver as > > > it currently exists, so I can't think of any reason for not handling > > > them. Why are we not? Ideally we should print a message for every > > > alarm bit so that we never return "true" without printing a message. > > > Even though OT2 limits aren't handled by the driver... They actually are, through MAX6659_REG_R_REMOTE_EMERG and LM90_HAVE_EMERGENCY. > > > * If you think this piece of code shouldn't deal with OT/THERM limits > > > because they do not trigger an SMBus alarm, this can be discussed, Yes, that was the logic. Presumably OT1 and OT2 are separate interrupts (if connected to interrupt pins) and would have to be handled separately. > > > but all chips should be handled the same in this respect then. Agreed. > > > * Why in the first place is max6696's data->alert_alarms set to 0x187c > > > and not 0x1c7c? Including 1OPEN but not 2OPEN makes no sense. Oversight. 2OPEN does trigger ALERT, so the bit should be set. I'll send a patch in a minute. Guenter ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v3 2/4] hwmon: (lm90) use macro defines for the status bit 2013-07-15 16:57 ` Jean Delvare 2013-07-15 17:33 ` Guenter Roeck @ 2013-07-17 7:03 ` Wei Ni 2013-07-17 7:09 ` Wei Ni 2013-07-17 8:28 ` Jean Delvare 1 sibling, 2 replies; 48+ messages in thread From: Wei Ni @ 2013-07-17 7:03 UTC (permalink / raw) To: Jean Delvare Cc: Guenter Roeck, thierry.reding, lm-sensors, linux-kernel, linux-tegra On 07/16/2013 12:57 AM, Jean Delvare wrote: > Hi Wei, Guenter, > > On Fri, 12 Jul 2013 15:48:05 +0800, Wei Ni wrote: >> Add bit defines for the status register. > > Regarding the subject: for me these are constants, not macros. AFAIK > the term "macro" refers to defines with parameters only. How about "Introduce status bits" > >> Signed-off-by: Wei Ni <wni@nvidia.com> >> --- >> drivers/hwmon/lm90.c | 72 ++++++++++++++++++++++++++++++++++---------------- >> 1 file changed, 49 insertions(+), 23 deletions(-) >> >> diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c >> index 5f30f90..c90037f 100644 >> --- a/drivers/hwmon/lm90.c >> +++ b/drivers/hwmon/lm90.c >> @@ -179,6 +179,19 @@ enum chips { lm90, adm1032, lm99, lm86, max6657, max6659, adt7461, max6680, >> #define LM90_HAVE_TEMP3 (1 << 6) /* 3rd temperature sensor */ >> #define LM90_HAVE_BROKEN_ALERT (1 << 7) /* Broken alert */ >> >> +/* LM90 status */ >> +#define LM90_STATUS_LTHRM (1 << 0) /* local THERM limit tripped */ >> +#define LM90_STATUS_RTHRM (1 << 1) /* remote THERM limit tripped */ >> +#define LM90_STATUS_OPEN (1 << 2) /* remote is an open circuit */ >> +#define LM90_STATUS_RLOW (1 << 3) /* remote low temp limit tripped */ >> +#define LM90_STATUS_RHIGH (1 << 4) /* remote high temp limit tripped */ >> +#define LM90_STATUS_LLOW (1 << 5) /* local low temp limit tripped */ >> +#define LM90_STATUS_LHIGH (1 << 6) /* local high temp limit tripped */ >> +#define LM90_STATUS_BUSY (1 << 7) /* ADC is converting */ > > LM90_STATUS_BUSY is never used anywhere so please don't define it. Ok, I will remove it. > >> + >> +#define MAX6696_STATUS2_RLOW (1 << 3) /* remote2 low temp limit tripped */ >> +#define MAX6696_STATUS2_RHIGH (1 << 4) /* remote2 high temp limit tripped */ >> + >> /* >> * Driver data (common to all clients) >> */ >> @@ -1417,6 +1430,36 @@ static void lm90_init_client(struct i2c_client *client) >> i2c_smbus_write_byte_data(client, LM90_REG_W_CONFIG1, config); >> } >> >> +static bool lm90_is_tripped(struct i2c_client *client) >> +{ >> + struct lm90_data *data = i2c_get_clientdata(client); >> + u8 status, status2 = 0; >> + >> + lm90_read_reg(client, LM90_REG_R_STATUS, &status); >> + >> + if (data->kind == max6696) >> + lm90_read_reg(client, MAX6696_REG_R_STATUS2, &status2); >> + >> + if ((status & 0x7f) == 0 && (status2 & 0xfe) == 0) >> + return false; > > It's a bit disappointing to not use the freshly introduced constants. > That being said I agree it would make the code hard to read, so you can > leave it as is. Sorry, I forgot it. How about to define: #define LM90_STATUS_MASK 0x7f #define MAX6696_STATUS2 0xfe Or since Guenter is for vacation, I can just leave it as is, and wait him back to talk about below issue. > > Unrelated to this patch, but Guenter, I am worried about the MAX6696 > handling here. I realize that I am the one who accepted your code, but > now it looks wrong. Specifically: > * We check for (status2 & 0xfe) i.e. 7 alarm bits, but the code below > only reports 2 alarms bits. So if any of the 5 other alarm bits in > STATUS2 are, we may return true (chip is tripped) but not print the > cause. > * At least bits 1 and 2 of STATUS 2 fit totally fine in the driver as > it currently exists, so I can't think of any reason for not handling > them. Why are we not? Ideally we should print a message for every > alarm bit so that we never return "true" without printing a message. > Even though OT2 limits aren't handled by the driver... > * If you think this piece of code shouldn't deal with OT/THERM limits > because they do not trigger an SMBus alarm, this can be discussed, > but all chips should be handled the same in this respect then. > * Why in the first place is max6696's data->alert_alarms set to 0x187c > and not 0x1c7c? Including 1OPEN but not 2OPEN makes no sense. > >> + >> + if (status & (LM90_STATUS_LLOW | LM90_STATUS_LHIGH | LM90_STATUS_LTHRM)) >> + dev_warn(&client->dev, >> + "temp%d out of range, please check!\n", 1); >> + if (status & (LM90_STATUS_RLOW | LM90_STATUS_RHIGH | LM90_STATUS_RTHRM)) >> + dev_warn(&client->dev, >> + "temp%d out of range, please check!\n", 2); >> + if (status & LM90_STATUS_OPEN) >> + dev_warn(&client->dev, >> + "temp%d diode open, please check!\n", 2); >> + >> + if (status2 & (MAX6696_STATUS2_RLOW | MAX6696_STATUS2_RHIGH)) >> + dev_warn(&client->dev, >> + "temp%d out of range, please check!\n", 3); >> + >> + return true; >> +} >> + >> static int lm90_probe(struct i2c_client *client, >> const struct i2c_device_id *id) >> { >> @@ -1515,36 +1558,19 @@ static int lm90_remove(struct i2c_client *client) >> >> static void lm90_alert(struct i2c_client *client, unsigned int flag) >> { >> - struct lm90_data *data = i2c_get_clientdata(client); >> - u8 config, alarms, alarms2 = 0; >> - >> - lm90_read_reg(client, LM90_REG_R_STATUS, &alarms); >> - >> - if (data->kind == max6696) >> - lm90_read_reg(client, MAX6696_REG_R_STATUS2, &alarms2); >> - >> - if ((alarms & 0x7f) == 0 && (alarms2 & 0xfe) == 0) { >> + if (!lm90_is_tripped(client)) { > > You could swap the success and failure cases to avoid this negation. Yes, I will change it. > >> dev_info(&client->dev, "Everything OK\n"); >> } else { >> - if (alarms & 0x61) >> - dev_warn(&client->dev, >> - "temp%d out of range, please check!\n", 1); >> - if (alarms & 0x1a) >> - dev_warn(&client->dev, >> - "temp%d out of range, please check!\n", 2); >> - if (alarms & 0x04) >> - dev_warn(&client->dev, >> - "temp%d diode open, please check!\n", 2); >> - >> - if (alarms2 & 0x18) >> - dev_warn(&client->dev, >> - "temp%d out of range, please check!\n", 3); >> - >> /* >> * Disable ALERT# output, because these chips don't implement >> * SMBus alert correctly; they should only hold the alert line >> * low briefly. >> */ >> + struct lm90_data *data = i2c_get_clientdata(client); >> + u8 config, alarms; >> + >> + lm90_read_reg(client, LM90_REG_R_STATUS, &alarms); > > You end up reading LM90_REG_R_STATUS, which is not OK. This register > contains self-clearing bits, so there is no guarantee that the second > read will return the same value as the first read. You'll have to come > up with a different approach that reads LM90_REG_R_STATUS only once. Oh, yes, this is a problem, I didn't noticed it. How about to use this: bool lm90_alarms_tripped(*client, *status); bool lm90_alarms2_tripped(*client, *status2); So we can read the status only once and pass it. > >> + >> if ((data->flags & LM90_HAVE_BROKEN_ALERT) >> && (alarms & data->alert_alarms)) { >> dev_dbg(&client->dev, "Disabling ALERT#\n"); > > ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v3 2/4] hwmon: (lm90) use macro defines for the status bit 2013-07-17 7:03 ` Wei Ni @ 2013-07-17 7:09 ` Wei Ni 2013-07-17 8:28 ` Jean Delvare 1 sibling, 0 replies; 48+ messages in thread From: Wei Ni @ 2013-07-17 7:09 UTC (permalink / raw) To: Jean Delvare Cc: Guenter Roeck, thierry.reding, lm-sensors, linux-kernel, linux-tegra On 07/17/2013 03:03 PM, Wei Ni wrote: > On 07/16/2013 12:57 AM, Jean Delvare wrote: >> Hi Wei, Guenter, >> >>> + >>> + if ((status & 0x7f) == 0 && (status2 & 0xfe) == 0) >>> + return false; >> >> It's a bit disappointing to not use the freshly introduced constants. >> That being said I agree it would make the code hard to read, so you can >> leave it as is. > > Sorry, I forgot it. > How about to define: > #define LM90_STATUS_MASK 0x7f > #define MAX6696_STATUS2 0xfe Sorry, it should be "#define MAX6696_STATUS2_MASK 0xfe". > > Or since Guenter is for vacation, I can just leave it as is, and wait > him back to talk about below issue. > >> > ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v3 2/4] hwmon: (lm90) use macro defines for the status bit 2013-07-17 7:03 ` Wei Ni 2013-07-17 7:09 ` Wei Ni @ 2013-07-17 8:28 ` Jean Delvare 2013-07-17 9:29 ` Wei Ni 1 sibling, 1 reply; 48+ messages in thread From: Jean Delvare @ 2013-07-17 8:28 UTC (permalink / raw) To: Wei Ni Cc: Guenter Roeck, thierry.reding, lm-sensors, linux-kernel, linux-tegra Hi Wei, On Wed, 17 Jul 2013 15:03:35 +0800, Wei Ni wrote: > On 07/16/2013 12:57 AM, Jean Delvare wrote: > > On Fri, 12 Jul 2013 15:48:05 +0800, Wei Ni wrote: > >> Add bit defines for the status register. > > > > Regarding the subject: for me these are constants, not macros. AFAIK > > the term "macro" refers to defines with parameters only. > > How about "Introduce status bits" I'd say "Define status bits" as this is exactly what you're doing ;-) That being said, your patch actually does more than this, as you are moving code around and to a separate function. The patch description should say that and explain why. > >> (...) > >> + if ((status & 0x7f) == 0 && (status2 & 0xfe) == 0) > >> + return false; > > > > It's a bit disappointing to not use the freshly introduced constants. > > That being said I agree it would make the code hard to read, so you can > > leave it as is. > > Sorry, I forgot it. > How about to define: > #define LM90_STATUS_MASK 0x7f > #define MAX6696_STATUS2_MASK 0xfe I wouldn't bother. I suspect that this code will have to be reworked soon anyway and these constants may no longer be needed then. > Or since Guenter is for vacation, I can just leave it as is, and wait > him back to talk about below issue. I do maintain the lm90 driver, so the decision is up to me. Guenter did a preliminary review of your patches and I am grateful for that, but I do not intend to wait for his return to continue with your patches. Otherwise he will have to do the same when he returns and I am gone, and this may end up delaying your patches by one kernel version. > >> (...) > >> + struct lm90_data *data = i2c_get_clientdata(client); > >> + u8 config, alarms; > >> + > >> + lm90_read_reg(client, LM90_REG_R_STATUS, &alarms); > > > > You end up reading LM90_REG_R_STATUS, which is not OK. This register > > contains self-clearing bits, so there is no guarantee that the second > > read will return the same value as the first read. You'll have to come > > up with a different approach that reads LM90_REG_R_STATUS only once. > > Oh, yes, this is a problem, I didn't noticed it. > How about to use this: > bool lm90_alarms_tripped(*client, *status); > bool lm90_alarms2_tripped(*client, *status2); > So we can read the status only once and pass it. This is a good idea but you only need status, not status2, so it can be made simpler: bool lm90_is_tripped(*client, *status); (handling both status and status 2 as you already do.) -- Jean Delvare ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v3 2/4] hwmon: (lm90) use macro defines for the status bit 2013-07-17 8:28 ` Jean Delvare @ 2013-07-17 9:29 ` Wei Ni 2013-07-17 9:46 ` Jean Delvare 0 siblings, 1 reply; 48+ messages in thread From: Wei Ni @ 2013-07-17 9:29 UTC (permalink / raw) To: Jean Delvare Cc: Guenter Roeck, thierry.reding, lm-sensors, linux-kernel, linux-tegra On 07/17/2013 04:28 PM, Jean Delvare wrote: > Hi Wei, > > On Wed, 17 Jul 2013 15:03:35 +0800, Wei Ni wrote: >> On 07/16/2013 12:57 AM, Jean Delvare wrote: >>> On Fri, 12 Jul 2013 15:48:05 +0800, Wei Ni wrote: >>>> Add bit defines for the status register. >>> >>> Regarding the subject: for me these are constants, not macros. AFAIK >>> the term "macro" refers to defines with parameters only. >> >> How about "Introduce status bits" > > I'd say "Define status bits" as this is exactly what you're doing ;-) > That being said, your patch actually does more than this, as you are > moving code around and to a separate function. The patch description > should say that and explain why. ok, I will update it in my next version. > >>>> (...) >>>> + if ((status & 0x7f) == 0 && (status2 & 0xfe) == 0) >>>> + return false; >>> >>> It's a bit disappointing to not use the freshly introduced constants. >>> That being said I agree it would make the code hard to read, so you can >>> leave it as is. >> >> Sorry, I forgot it. >> How about to define: >> #define LM90_STATUS_MASK 0x7f >> #define MAX6696_STATUS2_MASK 0xfe > > I wouldn't bother. I suspect that this code will have to be reworked > soon anyway and these constants may no longer be needed then. Ok, let's leave it as is. > >> Or since Guenter is for vacation, I can just leave it as is, and wait >> him back to talk about below issue. > > I do maintain the lm90 driver, so the decision is up to me. Guenter did > a preliminary review of your patches and I am grateful for that, but I > do not intend to wait for his return to continue with your patches. > Otherwise he will have to do the same when he returns and I am gone, > and this may end up delaying your patches by one kernel version. I will send out patches soon :) > >>>> (...) >>>> + struct lm90_data *data = i2c_get_clientdata(client); >>>> + u8 config, alarms; >>>> + >>>> + lm90_read_reg(client, LM90_REG_R_STATUS, &alarms); >>> >>> You end up reading LM90_REG_R_STATUS, which is not OK. This register >>> contains self-clearing bits, so there is no guarantee that the second >>> read will return the same value as the first read. You'll have to come >>> up with a different approach that reads LM90_REG_R_STATUS only once. >> >> Oh, yes, this is a problem, I didn't noticed it. >> How about to use this: >> bool lm90_alarms_tripped(*client, *status); >> bool lm90_alarms2_tripped(*client, *status2); >> So we can read the status only once and pass it. > > This is a good idea but you only need status, not status2, so it can be > made simpler: > bool lm90_is_tripped(*client, *status); > (handling both status and status 2 as you already do.) Yes this is simpler, but I think in the future we may need to handle the status2, how to handle it ? Or we can define the status as bit[0~7]->status and bit[8~15]->status2 . > ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v3 2/4] hwmon: (lm90) use macro defines for the status bit 2013-07-17 9:29 ` Wei Ni @ 2013-07-17 9:46 ` Jean Delvare 0 siblings, 0 replies; 48+ messages in thread From: Jean Delvare @ 2013-07-17 9:46 UTC (permalink / raw) To: Wei Ni Cc: Guenter Roeck, thierry.reding, lm-sensors, linux-kernel, linux-tegra Hi Wei, On Wed, 17 Jul 2013 17:29:32 +0800, Wei Ni wrote: > On 07/17/2013 04:28 PM, Jean Delvare wrote: > > I do maintain the lm90 driver, so the decision is up to me. Guenter did > > a preliminary review of your patches and I am grateful for that, but I > > do not intend to wait for his return to continue with your patches. > > Otherwise he will have to do the same when he returns and I am gone, > > and this may end up delaying your patches by one kernel version. > > I will send out patches soon :) You may want to wait until I'm done reviewing the whole v3 patch set. I hope to have the time to do that today. > >> (...) > >> Oh, yes, this is a problem, I didn't noticed it. > >> How about to use this: > >> bool lm90_alarms_tripped(*client, *status); > >> bool lm90_alarms2_tripped(*client, *status2); > >> So we can read the status only once and pass it. > > > > This is a good idea but you only need status, not status2, so it can be > > made simpler: > > bool lm90_is_tripped(*client, *status); > > (handling both status and status 2 as you already do.) > > Yes this is simpler, but I think in the future we may need to handle the > status2, how to handle it ? Or we can define the status as > bit[0~7]->status and bit[8~15]->status2 . I hope we will never have to. We need it to work around a hardware implementation bug, and I hope that newer chips implementing status2 will not have this bug. That being said, yes, returning status and status2 combined in a 16-bit value would make sense. We end up comparing with data->alert_alarms which has exactly this format anyway (and so does data->alarms too.) -- Jean Delvare ^ permalink raw reply [flat|nested] 48+ messages in thread
* [PATCH v3 3/4] hwmon: (lm90) add support to handle IRQ 2013-07-12 7:48 [PATCH v3 0/4] Lm90 Enhancements Wei Ni 2013-07-12 7:48 ` [PATCH v3 1/4] hwmon: (lm90) split set&show temp as common codes Wei Ni 2013-07-12 7:48 ` [PATCH v3 2/4] hwmon: (lm90) use macro defines for the status bit Wei Ni @ 2013-07-12 7:48 ` Wei Ni 2013-07-18 15:58 ` Jean Delvare 2013-07-12 7:48 ` [PATCH v3 4/4] hwmon: (lm90) use enums for the indexes of temp8 and temp11 Wei Ni 3 siblings, 1 reply; 48+ messages in thread From: Wei Ni @ 2013-07-12 7:48 UTC (permalink / raw) To: khali, linux, thierry.reding Cc: lm-sensors, linux-kernel, linux-tegra, Wei Ni When the temperature exceed the limit range value, the driver can handle the interrupt. Signed-off-by: Wei Ni <wni@nvidia.com> --- drivers/hwmon/lm90.c | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c index c90037f..1cc3d19 100644 --- a/drivers/hwmon/lm90.c +++ b/drivers/hwmon/lm90.c @@ -89,6 +89,7 @@ #include <linux/err.h> #include <linux/mutex.h> #include <linux/sysfs.h> +#include <linux/interrupt.h> /* * Addresses to scan @@ -1460,6 +1461,17 @@ static bool lm90_is_tripped(struct i2c_client *client) return true; } +static irqreturn_t lm90_irq_thread(int irq, void *dev_id) +{ + struct lm90_data *data = dev_id; + struct i2c_client *client = to_i2c_client(data->hwmon_dev->parent); + + if (lm90_is_tripped(client)) + return IRQ_HANDLED; + else + return IRQ_NONE; +} + static int lm90_probe(struct i2c_client *client, const struct i2c_device_id *id) { @@ -1536,6 +1548,18 @@ static int lm90_probe(struct i2c_client *client, goto exit_remove_files; } + if (client->irq >= 0) { + dev_dbg(dev, "lm90 IRQ: %d\n", client->irq); + err = devm_request_threaded_irq(dev, client->irq, + NULL, lm90_irq_thread, + IRQF_TRIGGER_LOW | IRQF_ONESHOT, + "lm90", data); + if (err < 0) { + dev_err(dev, "cannot request interrupt\n"); + goto exit_remove_files; + } + } + return 0; exit_remove_files: -- 1.7.9.5 ^ permalink raw reply related [flat|nested] 48+ messages in thread
* Re: [PATCH v3 3/4] hwmon: (lm90) add support to handle IRQ 2013-07-12 7:48 ` [PATCH v3 3/4] hwmon: (lm90) add support to handle IRQ Wei Ni @ 2013-07-18 15:58 ` Jean Delvare 2013-07-19 6:41 ` Wei Ni 0 siblings, 1 reply; 48+ messages in thread From: Jean Delvare @ 2013-07-18 15:58 UTC (permalink / raw) To: Wei Ni; +Cc: linux, thierry.reding, lm-sensors, linux-kernel, linux-tegra Hi Wei, On Fri, 12 Jul 2013 15:48:06 +0800, Wei Ni wrote: > When the temperature exceed the limit range value, > the driver can handle the interrupt. > > Signed-off-by: Wei Ni <wni@nvidia.com> > --- > drivers/hwmon/lm90.c | 24 ++++++++++++++++++++++++ > 1 file changed, 24 insertions(+) > > diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c > index c90037f..1cc3d19 100644 > --- a/drivers/hwmon/lm90.c > +++ b/drivers/hwmon/lm90.c > @@ -89,6 +89,7 @@ > #include <linux/err.h> > #include <linux/mutex.h> > #include <linux/sysfs.h> > +#include <linux/interrupt.h> > > /* > * Addresses to scan > @@ -1460,6 +1461,17 @@ static bool lm90_is_tripped(struct i2c_client *client) > return true; > } > > +static irqreturn_t lm90_irq_thread(int irq, void *dev_id) > +{ > + struct lm90_data *data = dev_id; > + struct i2c_client *client = to_i2c_client(data->hwmon_dev->parent); Why are you passing data as the dev_id in the first place, instead of client? It's easier to get the data when you have the client (i2c_get_clientdata) than the other way around. > + > + if (lm90_is_tripped(client)) > + return IRQ_HANDLED; > + else > + return IRQ_NONE; > +} > + > static int lm90_probe(struct i2c_client *client, > const struct i2c_device_id *id) > { > @@ -1536,6 +1548,18 @@ static int lm90_probe(struct i2c_client *client, > goto exit_remove_files; > } > > + if (client->irq >= 0) { I though you had agreed to just check for (client->irq)? > + dev_dbg(dev, "lm90 IRQ: %d\n", client->irq); The "lm90" is redundant, dev_dbg will use the chip name as a prefix. > + err = devm_request_threaded_irq(dev, client->irq, > + NULL, lm90_irq_thread, > + IRQF_TRIGGER_LOW | IRQF_ONESHOT, > + "lm90", data); > + if (err < 0) { > + dev_err(dev, "cannot request interrupt\n"); You should include the IRQ number in the error message, it is useful for investigating the issue. Not everyone will have the debugging message above enabled. > + goto exit_remove_files; > + } > + } > + > return 0; > > exit_remove_files: That's it for the code. Now I'm not sure I understand what you are trying to do and what this is all good for. First of all, how is the chip wired on your system? You are using an NCT1008, right? Which output of the chip is connected to your interrupt line, THERM or ALERT/THERM2? ALERT is normally used for SMBus Alert but I suppose it could be used for an interrupt too. THERM and THERM2 OTOH are comparator outputs, they stay low as long as the temperature are above the therm limits. Reading the status register won't bring them back up as I understand it, and contrary to the ALERT output, they can't be masked. Won't this result in an interrupt flood if using IRQF_TRIGGER_LOW? Have you tested your code already? Also I don't think just logging an error message is the right thing to do in the case of overheating. The code to handle SMBus alerts is from me, and it does indeed just log the problems, but it was really only meant as a proof of concept when I first added support for SMBus Alert. Today we could do better than this, starting with issuing sysfs notifications to the relevant alarm files (several other hwmon drivers are doing that already.) For a real system, if the THERM output is connected to an interrupt line (instead of directly to a fan controller) I would expect the platform to provide a callback to handle thermal events and take whatever appropriate measure is needed (e.g. throttling.) Just logging the problem won't save the system, by the time someone looks at the log it might be too late. -- Jean Delvare ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v3 3/4] hwmon: (lm90) add support to handle IRQ 2013-07-18 15:58 ` Jean Delvare @ 2013-07-19 6:41 ` Wei Ni 2013-07-24 7:46 ` Wei Ni 2013-07-27 15:02 ` Jean Delvare 0 siblings, 2 replies; 48+ messages in thread From: Wei Ni @ 2013-07-19 6:41 UTC (permalink / raw) To: Jean Delvare; +Cc: linux, thierry.reding, lm-sensors, linux-kernel, linux-tegra On 07/18/2013 11:58 PM, Jean Delvare wrote: > Hi Wei, > > On Fri, 12 Jul 2013 15:48:06 +0800, Wei Ni wrote: >> When the temperature exceed the limit range value, >> the driver can handle the interrupt. >> >> Signed-off-by: Wei Ni <wni@nvidia.com> >> --- >> drivers/hwmon/lm90.c | 24 ++++++++++++++++++++++++ >> 1 file changed, 24 insertions(+) >> >> diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c >> index c90037f..1cc3d19 100644 >> --- a/drivers/hwmon/lm90.c >> +++ b/drivers/hwmon/lm90.c >> @@ -89,6 +89,7 @@ >> #include <linux/err.h> >> #include <linux/mutex.h> >> #include <linux/sysfs.h> >> +#include <linux/interrupt.h> >> >> /* >> * Addresses to scan >> @@ -1460,6 +1461,17 @@ static bool lm90_is_tripped(struct i2c_client *client) >> return true; >> } >> >> +static irqreturn_t lm90_irq_thread(int irq, void *dev_id) >> +{ >> + struct lm90_data *data = dev_id; >> + struct i2c_client *client = to_i2c_client(data->hwmon_dev->parent); > > Why are you passing data as the dev_id in the first place, instead of > client? It's easier to get the data when you have the client > (i2c_get_clientdata) than the other way around. Oh, I'm stupid :) I will pass the client. > >> + >> + if (lm90_is_tripped(client)) >> + return IRQ_HANDLED; >> + else >> + return IRQ_NONE; >> +} >> + >> static int lm90_probe(struct i2c_client *client, >> const struct i2c_device_id *id) >> { >> @@ -1536,6 +1548,18 @@ static int lm90_probe(struct i2c_client *client, >> goto exit_remove_files; >> } >> >> + if (client->irq >= 0) { > > I though you had agreed to just check for (client->irq)? Oh, yes, I forgot to change it, thanks, I will update it. > >> + dev_dbg(dev, "lm90 IRQ: %d\n", client->irq); > > The "lm90" is redundant, dev_dbg will use the chip name as a prefix. Ok, I will remove it. > >> + err = devm_request_threaded_irq(dev, client->irq, >> + NULL, lm90_irq_thread, >> + IRQF_TRIGGER_LOW | IRQF_ONESHOT, >> + "lm90", data); >> + if (err < 0) { >> + dev_err(dev, "cannot request interrupt\n"); > > You should include the IRQ number in the error message, it is useful > for investigating the issue. Not everyone will have the debugging > message above enabled. Yes, you are right, I will add the IRQ number. > >> + goto exit_remove_files; >> + } >> + } >> + >> return 0; >> >> exit_remove_files: > > That's it for the code. Now I'm not sure I understand what you are > trying to do and what this is all good for. > > First of all, how is the chip wired on your system? You are using an > NCT1008, right? Which output of the chip is connected to your interrupt > line, THERM or ALERT/THERM2? ALERT is normally used for SMBus Alert but > I suppose it could be used for an interrupt too. THERM and THERM2 OTOH > are comparator outputs, they stay low as long as the temperature are > above the therm limits. Reading the status register won't bring them > back up as I understand it, and contrary to the ALERT output, they > can't be masked. Won't this result in an interrupt flood if using > IRQF_TRIGGER_LOW? Have you tested your code already? Let me explain why I want to add the IRQ thread. In our tegra30 platform, we use an NCT1008, and in our downstream tree, it programmed as following: 1. The pin THERM is connected to the PMIC, we will set the THERM limit in the initialization, once the this limit is tripped, it will trigged the PMIC, and the PMIC will shutdown the system immediately. 2. The pin ALERT/THERM2 is configured as ALERT, and it is connected to the interrupt line. In the platform init, we will prepare some trip temps, such as 20C, 40C,60C, 80C, and we will set 20C to the remote-low-temp-limit, and set 40C to remote-high-temp-limit. When the temperature exceed this rang, it will interrupt the system, then we will update the low/high limit to next rang, for example, if the temp raise to 50C, we will set 40C to low-limit, and set 60C to hight-limit, then we will run the throttle functions, and update cooling state. We wish to upstream these codes, but as you know, it's difficult to use current lm90.c and thermal framework to implement it, and these codes related many other things, such as throttling cpufreq, other clock freq. So at first, I want to update the lm90.c, so that I can add it to the thermal fw and use interrupt handler easily. And at the same time I followed the thermal framework thread, there has new infrastructure posted, which is more easy to add lm90 to thermal fw. > > Also I don't think just logging an error message is the right thing to > do in the case of overheating. The code to handle SMBus alerts is from > me, and it does indeed just log the problems, but it was really only > meant as a proof of concept when I first added support for SMBus Alert. > Today we could do better than this, starting with issuing sysfs > notifications to the relevant alarm files (several other hwmon drivers > are doing that already.) yes, I can try to use sysfs_notify() for the alarm. > > For a real system, if the THERM output is connected to an interrupt line > (instead of directly to a fan controller) I would expect the platform > to provide a callback to handle thermal events and take whatever > appropriate measure is needed (e.g. throttling.) Just logging the > problem won't save the system, by the time someone looks at the log it > might be too late. In our downstream tree, we write a new driver nct1008.c, whci can handle these interrupts and all other things, but as you know this driver can't be upstreamed, because there already has the lm90.c :). Anyway I think this patch is the first step of the implementation. > ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v3 3/4] hwmon: (lm90) add support to handle IRQ 2013-07-19 6:41 ` Wei Ni @ 2013-07-24 7:46 ` Wei Ni 2013-07-24 8:08 ` Jean Delvare 2013-07-27 15:02 ` Jean Delvare 1 sibling, 1 reply; 48+ messages in thread From: Wei Ni @ 2013-07-24 7:46 UTC (permalink / raw) To: Jean Delvare; +Cc: linux, thierry.reding, lm-sensors, linux-kernel, linux-tegra Hi, Jean On 07/19/2013 02:41 PM, Wei Ni wrote: > On 07/18/2013 11:58 PM, Jean Delvare wrote: >> Hi Wei, >> >> On Fri, 12 Jul 2013 15:48:06 +0800, Wei Ni wrote: >>> When the temperature exceed the limit range value, >>> the driver can handle the interrupt. >>> >>> Signed-off-by: Wei Ni <wni@nvidia.com> >>> --- >>> drivers/hwmon/lm90.c | 24 ++++++++++++++++++++++++ >>> 1 file changed, 24 insertions(+) >>> >>> diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c >>> index c90037f..1cc3d19 100644 >>> --- a/drivers/hwmon/lm90.c >>> +++ b/drivers/hwmon/lm90.c >>> @@ -89,6 +89,7 @@ >>> #include <linux/err.h> >>> #include <linux/mutex.h> >>> #include <linux/sysfs.h> >>> +#include <linux/interrupt.h> >>> >>> /* >>> * Addresses to scan >>> @@ -1460,6 +1461,17 @@ static bool lm90_is_tripped(struct i2c_client *client) >>> return true; >>> } >>> >>> +static irqreturn_t lm90_irq_thread(int irq, void *dev_id) >>> +{ >>> + struct lm90_data *data = dev_id; >>> + struct i2c_client *client = to_i2c_client(data->hwmon_dev->parent); >> >> Why are you passing data as the dev_id in the first place, instead of >> client? It's easier to get the data when you have the client >> (i2c_get_clientdata) than the other way around. > > Oh, I'm stupid :) > I will pass the client. > >> >>> + >>> + if (lm90_is_tripped(client)) >>> + return IRQ_HANDLED; >>> + else >>> + return IRQ_NONE; >>> +} >>> + >>> static int lm90_probe(struct i2c_client *client, >>> const struct i2c_device_id *id) >>> { >>> @@ -1536,6 +1548,18 @@ static int lm90_probe(struct i2c_client *client, >>> goto exit_remove_files; >>> } >>> >>> + if (client->irq >= 0) { >> >> I though you had agreed to just check for (client->irq)? > > Oh, yes, I forgot to change it, thanks, I will update it. > >> >>> + dev_dbg(dev, "lm90 IRQ: %d\n", client->irq); >> >> The "lm90" is redundant, dev_dbg will use the chip name as a prefix. > > Ok, I will remove it. > >> >>> + err = devm_request_threaded_irq(dev, client->irq, >>> + NULL, lm90_irq_thread, >>> + IRQF_TRIGGER_LOW | IRQF_ONESHOT, >>> + "lm90", data); >>> + if (err < 0) { >>> + dev_err(dev, "cannot request interrupt\n"); >> >> You should include the IRQ number in the error message, it is useful >> for investigating the issue. Not everyone will have the debugging >> message above enabled. > > Yes, you are right, I will add the IRQ number. > >> >>> + goto exit_remove_files; >>> + } >>> + } >>> + >>> return 0; >>> >>> exit_remove_files: >> >> That's it for the code. Now I'm not sure I understand what you are >> trying to do and what this is all good for. >> >> First of all, how is the chip wired on your system? You are using an >> NCT1008, right? Which output of the chip is connected to your interrupt >> line, THERM or ALERT/THERM2? ALERT is normally used for SMBus Alert but >> I suppose it could be used for an interrupt too. THERM and THERM2 OTOH >> are comparator outputs, they stay low as long as the temperature are >> above the therm limits. Reading the status register won't bring them >> back up as I understand it, and contrary to the ALERT output, they >> can't be masked. Won't this result in an interrupt flood if using >> IRQF_TRIGGER_LOW? Have you tested your code already? > > Let me explain why I want to add the IRQ thread. > In our tegra30 platform, we use an NCT1008, and in our downstream tree, > it programmed as following: > 1. The pin THERM is connected to the PMIC, we will set the THERM limit > in the initialization, once the this limit is tripped, it will trigged > the PMIC, and the PMIC will shutdown the system immediately. > 2. The pin ALERT/THERM2 is configured as ALERT, and it is connected to > the interrupt line. In the platform init, we will prepare some trip > temps, such as 20C, 40C,60C, 80C, and we will set 20C to the > remote-low-temp-limit, and set 40C to remote-high-temp-limit. When the > temperature exceed this rang, it will interrupt the system, then we will > update the low/high limit to next rang, for example, if the temp raise > to 50C, we will set 40C to low-limit, and set 60C to hight-limit, then > we will run the throttle functions, and update cooling state. > > We wish to upstream these codes, but as you know, it's difficult to use > current lm90.c and thermal framework to implement it, and these codes > related many other things, such as throttling cpufreq, other clock freq. > So at first, I want to update the lm90.c, so that I can add it to the > thermal fw and use interrupt handler easily. And at the same time I > followed the thermal framework thread, there has new infrastructure > posted, which is more easy to add lm90 to thermal fw. > >> >> Also I don't think just logging an error message is the right thing to >> do in the case of overheating. The code to handle SMBus alerts is from >> me, and it does indeed just log the problems, but it was really only >> meant as a proof of concept when I first added support for SMBus Alert. >> Today we could do better than this, starting with issuing sysfs >> notifications to the relevant alarm files (several other hwmon drivers >> are doing that already.) > > yes, I can try to use sysfs_notify() for the alarm. > >> >> For a real system, if the THERM output is connected to an interrupt line >> (instead of directly to a fan controller) I would expect the platform >> to provide a callback to handle thermal events and take whatever >> appropriate measure is needed (e.g. throttling.) Just logging the >> problem won't save the system, by the time someone looks at the log it >> might be too late. > > In our downstream tree, we write a new driver nct1008.c, whci can handle > these interrupts and all other things, but as you know this driver can't > be upstreamed, because there already has the lm90.c :). > Anyway I think this patch is the first step of the implementation. Do you have any more suggestions for this series, if no, I will prepare v4 patches. Thanks. Wei. > >> > ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v3 3/4] hwmon: (lm90) add support to handle IRQ 2013-07-24 7:46 ` Wei Ni @ 2013-07-24 8:08 ` Jean Delvare 0 siblings, 0 replies; 48+ messages in thread From: Jean Delvare @ 2013-07-24 8:08 UTC (permalink / raw) To: Wei Ni; +Cc: linux, thierry.reding, lm-sensors, linux-kernel, linux-tegra Hi Wei, On Wed, 24 Jul 2013 15:46:46 +0800, Wei Ni wrote: > Do you have any more suggestions for this series, if no, I will prepare > v4 patches. Sorry, I have a couple more replies "in progress" but had friends at home last week end so I did not have the time to finish and send them. I'll try to do that today, thanks for your patience. -- Jean Delvare ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v3 3/4] hwmon: (lm90) add support to handle IRQ 2013-07-19 6:41 ` Wei Ni 2013-07-24 7:46 ` Wei Ni @ 2013-07-27 15:02 ` Jean Delvare 2013-07-29 10:14 ` Wei Ni 1 sibling, 1 reply; 48+ messages in thread From: Jean Delvare @ 2013-07-27 15:02 UTC (permalink / raw) To: Wei Ni; +Cc: linux, thierry.reding, lm-sensors, linux-kernel, linux-tegra Hi Wei, Sorry for the late reply. On Fri, 19 Jul 2013 14:41:54 +0800, Wei Ni wrote: > On 07/18/2013 11:58 PM, Jean Delvare wrote: > > First of all, how is the chip wired on your system? You are using an > > NCT1008, right? Which output of the chip is connected to your interrupt > > line, THERM or ALERT/THERM2? ALERT is normally used for SMBus Alert but > > I suppose it could be used for an interrupt too. THERM and THERM2 OTOH > > are comparator outputs, they stay low as long as the temperature are > > above the therm limits. Reading the status register won't bring them > > back up as I understand it, and contrary to the ALERT output, they > > can't be masked. Won't this result in an interrupt flood if using > > IRQF_TRIGGER_LOW? Have you tested your code already? > > Let me explain why I want to add the IRQ thread. > In our tegra30 platform, we use an NCT1008, and in our downstream tree, > it programmed as following: > 1. The pin THERM is connected to the PMIC, we will set the THERM limit > in the initialization, once the this limit is tripped, it will trigged > the PMIC, and the PMIC will shutdown the system immediately. OK, this is what the chip is designed for, good. > 2. The pin ALERT/THERM2 is configured as ALERT, and it is connected to > the interrupt line. Why don't you use the SMBus alert mechanism then? It is already implemented and allows you to reuse the interrupt of the SMBus controller. Of course this is a question for the hardware designers, not you... If the board uses a separate interrupt pin for the NCT1008's ALERT output, then the driver has to handle that interrupt explicitly, as done in your patch. One thing which worries me though is this explanation in the NCT1008 datasheet: "The ALERT interrupt latch is not reset by reading the status register. It resets when the ALERT output has been serviced by the master reading the device address, provided the error condition has gone away and the status register flag bits are reset." This suggests that connecting the ALERT output to a separate interrupt pin will not work, as the ALERT output will never be de-asserted even if the fault conditions are gone and the status register was read. But as you say this is how your system is supposed to work, can you explain how it can work? > In the platform init, we will prepare some trip > temps, such as 20C, 40C,60C, 80C, and we will set 20C to the > remote-low-temp-limit, and set 40C to remote-high-temp-limit. When the > temperature exceed this rang, it will interrupt the system, then we will > update the low/high limit to next rang, for example, if the temp raise > to 50C, we will set 40C to low-limit, and set 60C to hight-limit, then > we will run the throttle functions, and update cooling state. Hu ho, I have seen this kind of design in the past, and I must say I don't quite like it. Moving critical thermal management handling to the software makes me feel unsafe. System thermal safety should not rely on the OS IMHO. It is best handled by the hardware, or if that can't be done, by the BIOS. And these limit updates are tricky, they could fail and then you're in trouble. Imagine for example that another chip on the SMBus hugs the bus and delays or even plain prevents the change of limits... But once again I guess you're not responsible for this. Another problem with this design, even if no such problem happens, is that the limits are user(-space)-writable in the lm90 driver, while with your design these limits are under full control of the platform management code. You definitely don't want the user to come and adjust the limits, this could result in overheating of the system. So, do you have a plan to optionally switch limits to read only in the lm90 driver? If not, how do you intend to solve the problem? The more I think about it, the more I wonder if a custom thermal driver wouldn't be better for your case. Exposing a few read-only values to user-space through the thermal/hwmon bridge would IMHO make more sense than cluttering the lm90 driver with conditionals to limit what it exposes to user-space. > We wish to upstream these codes, but as you know, it's difficult to use > current lm90.c and thermal framework to implement it, and these codes > related many other things, such as throttling cpufreq, other clock freq. > So at first, I want to update the lm90.c, so that I can add it to the > thermal fw and use interrupt handler easily. And at the same time I > followed the thermal framework thread, there has new infrastructure > posted, which is more easy to add lm90 to thermal fw. Don't get me wrong, I'm very happy with your efforts to upstream your code, this is very welcome. And I am also very happy that you split it into small chunks which are easier to review. But I also need to know the big picture to see where you're ultimately going. > > (...) > > For a real system, if the THERM output is connected to an interrupt line > > (instead of directly to a fan controller) I would expect the platform > > to provide a callback to handle thermal events and take whatever > > appropriate measure is needed (e.g. throttling.) Just logging the > > problem won't save the system, by the time someone looks at the log it > > might be too late. > > In our downstream tree, we write a new driver nct1008.c, whci can handle > these interrupts and all other things, but as you know this driver can't > be upstreamed, because there already has the lm90.c :). > Anyway I think this patch is the first step of the implementation. I'm curious how the nct1008 driver "handles these interrupts." IMHO only the platform code knows what needs to be done upon reception of an interrupt, in particular in your case where you'll do some magic with the limit registers. There's no way this can be hard-coded in a hwmon driver, lm90 or any other. -- Jean Delvare ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v3 3/4] hwmon: (lm90) add support to handle IRQ 2013-07-27 15:02 ` Jean Delvare @ 2013-07-29 10:14 ` Wei Ni 2013-07-29 15:58 ` Jean Delvare 0 siblings, 1 reply; 48+ messages in thread From: Wei Ni @ 2013-07-29 10:14 UTC (permalink / raw) To: Jean Delvare Cc: linux, thierry.reding, lm-sensors, linux-kernel, linux-tegra, sw-mobile-therm On 07/27/2013 11:02 PM, Jean Delvare wrote: > Hi Wei, > > Sorry for the late reply. > > On Fri, 19 Jul 2013 14:41:54 +0800, Wei Ni wrote: >> On 07/18/2013 11:58 PM, Jean Delvare wrote: >>> First of all, how is the chip wired on your system? You are using an >>> NCT1008, right? Which output of the chip is connected to your interrupt >>> line, THERM or ALERT/THERM2? ALERT is normally used for SMBus Alert but >>> I suppose it could be used for an interrupt too. THERM and THERM2 OTOH >>> are comparator outputs, they stay low as long as the temperature are >>> above the therm limits. Reading the status register won't bring them >>> back up as I understand it, and contrary to the ALERT output, they >>> can't be masked. Won't this result in an interrupt flood if using >>> IRQF_TRIGGER_LOW? Have you tested your code already? >> >> Let me explain why I want to add the IRQ thread. >> In our tegra30 platform, we use an NCT1008, and in our downstream tree, >> it programmed as following: >> 1. The pin THERM is connected to the PMIC, we will set the THERM limit >> in the initialization, once the this limit is tripped, it will trigged >> the PMIC, and the PMIC will shutdown the system immediately. > > OK, this is what the chip is designed for, good. > >> 2. The pin ALERT/THERM2 is configured as ALERT, and it is connected to >> the interrupt line. > > Why don't you use the SMBus alert mechanism then? It is already > implemented and allows you to reuse the interrupt of the SMBus > controller. > > Of course this is a question for the hardware designers, not you... If > the board uses a separate interrupt pin for the NCT1008's ALERT output, > then the driver has to handle that interrupt explicitly, as done in your > patch. > > One thing which worries me though is this explanation in the NCT1008 > datasheet: > > "The ALERT interrupt latch is not reset by reading the > status register. It resets when the ALERT output has been > serviced by the master reading the device address, provided > the error condition has gone away and the status register flag > bits are reset." > > This suggests that connecting the ALERT output to a separate interrupt > pin will not work, as the ALERT output will never be de-asserted even > if the fault conditions are gone and the status register was read. But > as you say this is how your system is supposed to work, can you explain > how it can work? Yes, we had met this problems, to fix this issue, we enabled one-shot mode in the bottom half handler of nct interrupts to force a conversion/comparison. This effectively stops repeated nct interrupts. We will do following things in the IRQ thread: 1. stand by the nct1008. (set configure register bit 6) 2. update the limit value if needed. 3. write to one-shot resister. 4. give hardware necessary time to finish conversion 5. run the nct1008 (clear configure register bit 6) > >> In the platform init, we will prepare some trip >> temps, such as 20C, 40C,60C, 80C, and we will set 20C to the >> remote-low-temp-limit, and set 40C to remote-high-temp-limit. When the >> temperature exceed this rang, it will interrupt the system, then we will >> update the low/high limit to next rang, for example, if the temp raise >> to 50C, we will set 40C to low-limit, and set 60C to hight-limit, then >> we will run the throttle functions, and update cooling state. > > Hu ho, I have seen this kind of design in the past, and I must say I > don't quite like it. Moving critical thermal management handling to the > software makes me feel unsafe. System thermal safety should not rely on > the OS IMHO. It is best handled by the hardware, or if that can't be > done, by the BIOS. And these limit updates are tricky, they could fail > and then you're in trouble. Imagine for example that another chip on > the SMBus hugs the bus and delays or even plain prevents the change of > limits... > > But once again I guess you're not responsible for this. These trip-temps are not critical temperature, we used these temps to update cooling states. For the critical-temp, we handle it like my mentioned in #1. Yes, it's not safe to rely on the software, so on our tegra114, we will have soc thermal, which can handle these trip-temps on hardware. > > Another problem with this design, even if no such problem happens, is > that the limits are user(-space)-writable in the lm90 driver, while > with your design these limits are under full control of the platform > management code. You definitely don't want the user to come and adjust > the limits, this could result in overheating of the system. So, do you > have a plan to optionally switch limits to read only in the lm90 > driver? If not, how do you intend to solve the problem? Yes, I had consider it, but didn't have good solution yet, because the lm90 is a generic driver, it's difficult to add a new feature to switch to read only or not. > > The more I think about it, the more I wonder if a custom thermal driver > wouldn't be better for your case. Exposing a few read-only values to > user-space through the thermal/hwmon bridge would IMHO make more sense > than cluttering the lm90 driver with conditionals to limit what it > exposes to user-space. > >> We wish to upstream these codes, but as you know, it's difficult to use >> current lm90.c and thermal framework to implement it, and these codes >> related many other things, such as throttling cpufreq, other clock freq. >> So at first, I want to update the lm90.c, so that I can add it to the >> thermal fw and use interrupt handler easily. And at the same time I >> followed the thermal framework thread, there has new infrastructure >> posted, which is more easy to add lm90 to thermal fw. > > Don't get me wrong, I'm very happy with your efforts to upstream your > code, this is very welcome. And I am also very happy that you split it > into small chunks which are easier to review. But I also need to know > the big picture to see where you're ultimately going. Actually, we tried to submit the nct1008.c very long ago, which included all we want to do, and you had reviewed it :) http://lists.lm-sensors.org/pipermail/lm-sensors/2011-April/032024.html > >>> (...) >>> For a real system, if the THERM output is connected to an interrupt line >>> (instead of directly to a fan controller) I would expect the platform >>> to provide a callback to handle thermal events and take whatever >>> appropriate measure is needed (e.g. throttling.) Just logging the >>> problem won't save the system, by the time someone looks at the log it >>> might be too late. >> >> In our downstream tree, we write a new driver nct1008.c, whci can handle >> these interrupts and all other things, but as you know this driver can't >> be upstreamed, because there already has the lm90.c :). >> Anyway I think this patch is the first step of the implementation. > > I'm curious how the nct1008 driver "handles these interrupts." IMHO > only the platform code knows what needs to be done upon reception of an > interrupt, in particular in your case where you'll do some magic with the > limit registers. There's no way this can be hard-coded in a hwmon > driver, lm90 or any other. Yes, absolutely agree, we can't add any private codes to the generic driver. I had talked about it with Durgadoss in his "[PATCH] Thermal Framework Enhancements" series, and in his v3 series, it introduced threshold concept, which used to set limit value, and the "framework is notified about this interrupt to take appropriate action". But this function still didn't be completed yet. I think the thermal fw can expose callback like thermal_zone->alert, and in the irq_thread, we can notify the thermal fw to call this alert callback function, then the platform code can do anything in this callback. Thanks. Wei. > ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v3 3/4] hwmon: (lm90) add support to handle IRQ 2013-07-29 10:14 ` Wei Ni @ 2013-07-29 15:58 ` Jean Delvare 2013-07-30 8:18 ` Wei Ni 0 siblings, 1 reply; 48+ messages in thread From: Jean Delvare @ 2013-07-29 15:58 UTC (permalink / raw) To: Wei Ni Cc: linux, thierry.reding, lm-sensors, linux-kernel, linux-tegra, sw-mobile-therm Hi Wei, On Mon, 29 Jul 2013 18:14:56 +0800, Wei Ni wrote: > On 07/27/2013 11:02 PM, Jean Delvare wrote: > > On Fri, 19 Jul 2013 14:41:54 +0800, Wei Ni wrote: > >> 2. The pin ALERT/THERM2 is configured as ALERT, and it is connected to > >> the interrupt line. > > > > Why don't you use the SMBus alert mechanism then? It is already > > implemented and allows you to reuse the interrupt of the SMBus > > controller. > > > > Of course this is a question for the hardware designers, not you... If > > the board uses a separate interrupt pin for the NCT1008's ALERT output, > > then the driver has to handle that interrupt explicitly, as done in your > > patch. > > > > One thing which worries me though is this explanation in the NCT1008 > > datasheet: > > > > "The ALERT interrupt latch is not reset by reading the > > status register. It resets when the ALERT output has been > > serviced by the master reading the device address, provided > > the error condition has gone away and the status register flag > > bits are reset." > > > > This suggests that connecting the ALERT output to a separate interrupt > > pin will not work, as the ALERT output will never be de-asserted even > > if the fault conditions are gone and the status register was read. But > > as you say this is how your system is supposed to work, can you explain > > how it can work? > > Yes, we had met this problems, to fix this issue, we enabled one-shot > mode in the bottom half handler of nct interrupts to force a > conversion/comparison. This effectively stops repeated nct interrupts. > We will do following things in the IRQ thread: > 1. stand by the nct1008. (set configure register bit 6) > 2. update the limit value if needed. > 3. write to one-shot resister. > 4. give hardware necessary time to finish conversion > 5. run the nct1008 (clear configure register bit 6) Doh, this is so ugly :( Why don't you configure the pin as THERM2 instead of ALERT then? I'd expect this to make things easier. > (...) > These trip-temps are not critical temperature, we used these temps to > update cooling states. For the critical-temp, we handle it like my > mentioned in #1. I understand. But even if these interrupts are only used for managing cooling states, a misbehavior could still have annoying consequences, such as causing the thermal shutdown to trigger when this could have been avoided, or throttling to stay enabled even though the system has cooled down enough. > Yes, it's not safe to rely on the software, so on our tegra114, we will > have soc thermal, which can handle these trip-temps on hardware. OK, good :) > (...) > Yes, absolutely agree, we can't add any private codes to the generic driver. > I had talked about it with Durgadoss in his "[PATCH] Thermal Framework > Enhancements" series, and in his v3 series, it introduced threshold > concept, which used to set limit value, and the "framework is notified > about this interrupt to take appropriate action". But this function > still didn't be completed yet. > I think the thermal fw can expose callback like thermal_zone->alert, and > in the irq_thread, we can notify the thermal fw to call this alert > callback function, then the platform code can do anything in this callback. I didn't follow the discussions closely, but something like this would be needed, yes. -- Jean Delvare ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v3 3/4] hwmon: (lm90) add support to handle IRQ 2013-07-29 15:58 ` Jean Delvare @ 2013-07-30 8:18 ` Wei Ni 2013-09-16 12:34 ` Jean Delvare 0 siblings, 1 reply; 48+ messages in thread From: Wei Ni @ 2013-07-30 8:18 UTC (permalink / raw) To: Jean Delvare Cc: linux, thierry.reding, lm-sensors, linux-kernel, linux-tegra, sw-mobile-therm On 07/29/2013 11:58 PM, Jean Delvare wrote: > Hi Wei, > > On Mon, 29 Jul 2013 18:14:56 +0800, Wei Ni wrote: >> On 07/27/2013 11:02 PM, Jean Delvare wrote: >>> On Fri, 19 Jul 2013 14:41:54 +0800, Wei Ni wrote: >>>> 2. The pin ALERT/THERM2 is configured as ALERT, and it is connected to >>>> the interrupt line. >>> >>> Why don't you use the SMBus alert mechanism then? It is already >>> implemented and allows you to reuse the interrupt of the SMBus >>> controller. >>> >>> Of course this is a question for the hardware designers, not you... If >>> the board uses a separate interrupt pin for the NCT1008's ALERT output, >>> then the driver has to handle that interrupt explicitly, as done in your >>> patch. >>> >>> One thing which worries me though is this explanation in the NCT1008 >>> datasheet: >>> >>> "The ALERT interrupt latch is not reset by reading the >>> status register. It resets when the ALERT output has been >>> serviced by the master reading the device address, provided >>> the error condition has gone away and the status register flag >>> bits are reset." >>> >>> This suggests that connecting the ALERT output to a separate interrupt >>> pin will not work, as the ALERT output will never be de-asserted even >>> if the fault conditions are gone and the status register was read. But >>> as you say this is how your system is supposed to work, can you explain >>> how it can work? >> >> Yes, we had met this problems, to fix this issue, we enabled one-shot >> mode in the bottom half handler of nct interrupts to force a >> conversion/comparison. This effectively stops repeated nct interrupts. >> We will do following things in the IRQ thread: >> 1. stand by the nct1008. (set configure register bit 6) >> 2. update the limit value if needed. >> 3. write to one-shot resister. >> 4. give hardware necessary time to finish conversion >> 5. run the nct1008 (clear configure register bit 6) > > Doh, this is so ugly :( > > Why don't you configure the pin as THERM2 instead of ALERT then? I'd > expect this to make things easier. If configure as THERM2, only the high temperature limits are relevant, so when the temperature reduced, it will not trigger interrupt, and we can't update the cooling state. Or do you mean that we can configure the pin to THERM2 in the irq_thread to avoid the repeated interrupt ? I tried it, but no help, the nct1008 will not run the conversion/comparison immediately, so the status register will not be cleared. > >> (...) >> These trip-temps are not critical temperature, we used these temps to >> update cooling states. For the critical-temp, we handle it like my >> mentioned in #1. > > I understand. But even if these interrupts are only used for managing > cooling states, a misbehavior could still have annoying consequences, > such as causing the thermal shutdown to trigger when this could have > been avoided, or throttling to stay enabled even though the system has > cooled down enough. I think our driver are trying best to avoid these troubles. As I know in our downstream codes, we didn't met these things. I think since the lm90 support interrupt mode, then the driver should have related interface to handle it, and it can call the callback function to do what the platform driver want. Thanks. Wei. > >> Yes, it's not safe to rely on the software, so on our tegra114, we will >> have soc thermal, which can handle these trip-temps on hardware. > > OK, good :) > >> (...) >> Yes, absolutely agree, we can't add any private codes to the generic driver. >> I had talked about it with Durgadoss in his "[PATCH] Thermal Framework >> Enhancements" series, and in his v3 series, it introduced threshold >> concept, which used to set limit value, and the "framework is notified >> about this interrupt to take appropriate action". But this function >> still didn't be completed yet. >> I think the thermal fw can expose callback like thermal_zone->alert, and >> in the irq_thread, we can notify the thermal fw to call this alert >> callback function, then the platform code can do anything in this callback. > > I didn't follow the discussions closely, but something like this would > be needed, yes. > ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v3 3/4] hwmon: (lm90) add support to handle IRQ 2013-07-30 8:18 ` Wei Ni @ 2013-09-16 12:34 ` Jean Delvare 0 siblings, 0 replies; 48+ messages in thread From: Jean Delvare @ 2013-09-16 12:34 UTC (permalink / raw) To: Wei Ni Cc: linux, thierry.reding, lm-sensors, linux-kernel, linux-tegra, sw-mobile-therm Hi Wei, Sorry for the late reply, catching up with the discussions from before my vacation... On Tue, 30 Jul 2013 16:18:35 +0800, Wei Ni wrote: > On 07/29/2013 11:58 PM, Jean Delvare wrote: > > On Mon, 29 Jul 2013 18:14:56 +0800, Wei Ni wrote: > >> Yes, we had met this problems, to fix this issue, we enabled one-shot > >> mode in the bottom half handler of nct interrupts to force a > >> conversion/comparison. This effectively stops repeated nct interrupts. > >> We will do following things in the IRQ thread: > >> 1. stand by the nct1008. (set configure register bit 6) > >> 2. update the limit value if needed. > >> 3. write to one-shot resister. > >> 4. give hardware necessary time to finish conversion > >> 5. run the nct1008 (clear configure register bit 6) > > > > Doh, this is so ugly :( > > > > Why don't you configure the pin as THERM2 instead of ALERT then? I'd > > expect this to make things easier. > > If configure as THERM2, only the high temperature limits are relevant, > so when the temperature reduced, it will not trigger interrupt, and we > can't update the cooling state. Ah, indeed, I had not noticed this restriction. > Or do you mean that we can configure the pin to THERM2 in the irq_thread > to avoid the repeated interrupt ? I tried it, but no help, the nct1008 > will not run the conversion/comparison immediately, so the status > register will not be cleared. No, I didn't mean to suggest anything like that. > >> (...) > >> These trip-temps are not critical temperature, we used these temps to > >> update cooling states. For the critical-temp, we handle it like my > >> mentioned in #1. > > > > I understand. But even if these interrupts are only used for managing > > cooling states, a misbehavior could still have annoying consequences, > > such as causing the thermal shutdown to trigger when this could have > > been avoided, or throttling to stay enabled even though the system has > > cooled down enough. > > I think our driver are trying best to avoid these troubles. As I know in > our downstream codes, we didn't met these things. > I think since the lm90 support interrupt mode, then the driver should > have related interface to handle it, and it can call the callback > function to do what the platform driver want. Yes, fair enough. I do not object to it, I was only trying to understand how you were using it. -- Jean Delvare ^ permalink raw reply [flat|nested] 48+ messages in thread
* [PATCH v3 4/4] hwmon: (lm90) use enums for the indexes of temp8 and temp11 2013-07-12 7:48 [PATCH v3 0/4] Lm90 Enhancements Wei Ni ` (2 preceding siblings ...) 2013-07-12 7:48 ` [PATCH v3 3/4] hwmon: (lm90) add support to handle IRQ Wei Ni @ 2013-07-12 7:48 ` Wei Ni 2013-07-27 15:38 ` Jean Delvare 3 siblings, 1 reply; 48+ messages in thread From: Wei Ni @ 2013-07-12 7:48 UTC (permalink / raw) To: khali, linux, thierry.reding Cc: lm-sensors, linux-kernel, linux-tegra, Wei Ni Using enums for the indexes and nrs of temp8 and temp11. This make the code much more readable. Signed-off-by: Wei Ni <wni@nvidia.com> --- drivers/hwmon/lm90.c | 179 ++++++++++++++++++++++++++++++++------------------ 1 file changed, 114 insertions(+), 65 deletions(-) diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c index 1cc3d19..8cb5dd0 100644 --- a/drivers/hwmon/lm90.c +++ b/drivers/hwmon/lm90.c @@ -310,6 +310,59 @@ static const struct lm90_params lm90_params[] = { }; /* + * TEMP8 register index + */ +enum lm90_temp8_reg_index { + TEMP8_LOCAL_LOW = 0, /* 0: local low limit */ + TEMP8_LOCAL_HIGH, /* 1: local high limit */ + TEMP8_LOCAL_CRIT, /* 2: local critical limit */ + TEMP8_REMOTE_CRIT, /* 3: remote critical limit */ + TEMP8_LOCAL_EMERG, /* 4: local emergency limit + * (max6659 and max6695/96) + */ + TEMP8_REMOTE_EMERG, /* 5: remote emergency limit + * (max6659 and max6695/96) + */ + TEMP8_REMOTE2_CRIT, /* 6: remote 2 critical limit + * (max6695/96 only) + */ + TEMP8_REMOTE2_EMERG, /* 7: remote 2 emergency limit + * (max6695/96 only) + */ + TEMP8_REG_NUM +}; + +/* + * TEMP11 register index + */ +enum lm90_temp11_reg_index { + TEMP11_REMOTE_TEMP = 0, /* 0: remote input */ + TEMP11_REMOTE_LOW, /* 1: remote low limit */ + TEMP11_REMOTE_HIGH, /* 2: remote high limit */ + TEMP11_REMOTE_OFFSET, /* 3: remote offset + * (except max6646, max6657/58/59, + * and max6695/96) + */ + TEMP11_LOCAL_TEMP, /* 4: local input */ + TEMP11_REMOTE2_TEMP, /* 5: remote 2 input (max6695/96 only) */ + TEMP11_REMOTE2_LOW, /* 6: remote 2 low limit (max6695/96 only) */ + TEMP11_REMOTE2_HIGH, /* 7: remote 2 high limit (max6695/96 only) */ + TEMP11_REG_NUM +}; + +/* + * TEMP11 register NR + */ +enum lm90_temp11_reg_nr { + NR_CHAN_0_REMOTE_LOW = 0, /* 0: channel 0, remote low limit */ + NR_CHAN_0_REMOTE_HIGH, /* 1: channel 0, remote high limit */ + NR_CHAN_0_REMOTE_OFFSET, /* 2: channel 0, remote offset */ + NR_CHAN_1_REMOTE_LOW, /* 3: channel 1, remote low limit */ + NR_CHAN_1_REMOTE_HIGH, /* 4: channel 1, remote high limit */ + NR_NUM /* number of the NRs for temp11 */ +}; + +/* * Client data (each client gets its own) */ @@ -331,25 +384,8 @@ struct lm90_data { u8 reg_local_ext; /* local extension register offset */ /* registers values */ - s8 temp8[8]; /* 0: local low limit - * 1: local high limit - * 2: local critical limit - * 3: remote critical limit - * 4: local emergency limit (max6659 and max6695/96) - * 5: remote emergency limit (max6659 and max6695/96) - * 6: remote 2 critical limit (max6695/96 only) - * 7: remote 2 emergency limit (max6695/96 only) - */ - s16 temp11[8]; /* 0: remote input - * 1: remote low limit - * 2: remote high limit - * 3: remote offset (except max6646, max6657/58/59, - * and max6695/96) - * 4: local input - * 5: remote 2 input (max6695/96 only) - * 6: remote 2 low limit (max6695/96 only) - * 7: remote 2 high limit (max6695/96 only) - */ + s8 temp8[TEMP8_REG_NUM]; + s16 temp11[TEMP11_REG_NUM]; u8 temp_hyst; u16 alarms; /* bitvector (upper 8 bits for max6695/96) */ }; @@ -491,37 +527,42 @@ static struct lm90_data *lm90_update_device(struct device *dev) u8 alarms; dev_dbg(&client->dev, "Updating lm90 data.\n"); - lm90_read_reg(client, LM90_REG_R_LOCAL_LOW, &data->temp8[0]); - lm90_read_reg(client, LM90_REG_R_LOCAL_HIGH, &data->temp8[1]); - lm90_read_reg(client, LM90_REG_R_LOCAL_CRIT, &data->temp8[2]); - lm90_read_reg(client, LM90_REG_R_REMOTE_CRIT, &data->temp8[3]); + lm90_read_reg(client, LM90_REG_R_LOCAL_LOW, + &data->temp8[TEMP8_LOCAL_LOW]); + lm90_read_reg(client, LM90_REG_R_LOCAL_HIGH, + &data->temp8[TEMP8_LOCAL_HIGH]); + lm90_read_reg(client, LM90_REG_R_LOCAL_CRIT, + &data->temp8[TEMP8_LOCAL_CRIT]); + lm90_read_reg(client, LM90_REG_R_REMOTE_CRIT, + &data->temp8[TEMP8_REMOTE_CRIT]); lm90_read_reg(client, LM90_REG_R_TCRIT_HYST, &data->temp_hyst); if (data->reg_local_ext) { lm90_read16(client, LM90_REG_R_LOCAL_TEMP, data->reg_local_ext, - &data->temp11[4]); + &data->temp11[TEMP11_LOCAL_TEMP]); } else { if (lm90_read_reg(client, LM90_REG_R_LOCAL_TEMP, &h) == 0) - data->temp11[4] = h << 8; + data->temp11[TEMP11_LOCAL_TEMP] = h << 8; } lm90_read16(client, LM90_REG_R_REMOTE_TEMPH, - LM90_REG_R_REMOTE_TEMPL, &data->temp11[0]); + LM90_REG_R_REMOTE_TEMPL, + &data->temp11[TEMP11_REMOTE_TEMP]); if (lm90_read_reg(client, LM90_REG_R_REMOTE_LOWH, &h) == 0) { - data->temp11[1] = h << 8; + data->temp11[TEMP11_REMOTE_LOW] = h << 8; if ((data->flags & LM90_HAVE_REM_LIMIT_EXT) && lm90_read_reg(client, LM90_REG_R_REMOTE_LOWL, &l) == 0) - data->temp11[1] |= l; + data->temp11[TEMP11_REMOTE_LOW] |= l; } if (lm90_read_reg(client, LM90_REG_R_REMOTE_HIGHH, &h) == 0) { - data->temp11[2] = h << 8; + data->temp11[TEMP11_REMOTE_HIGH] = h << 8; if ((data->flags & LM90_HAVE_REM_LIMIT_EXT) && lm90_read_reg(client, LM90_REG_R_REMOTE_HIGHL, &l) == 0) - data->temp11[2] |= l; + data->temp11[TEMP11_REMOTE_HIGH] |= l; } if (data->flags & LM90_HAVE_OFFSET) { @@ -529,13 +570,14 @@ static struct lm90_data *lm90_update_device(struct device *dev) &h) == 0 && lm90_read_reg(client, LM90_REG_R_REMOTE_OFFSL, &l) == 0) - data->temp11[3] = (h << 8) | l; + data->temp11[TEMP11_REMOTE_OFFSET] = + (h << 8) | l; } if (data->flags & LM90_HAVE_EMERGENCY) { lm90_read_reg(client, MAX6659_REG_R_LOCAL_EMERG, - &data->temp8[4]); + &data->temp8[TEMP8_LOCAL_EMERG]); lm90_read_reg(client, MAX6659_REG_R_REMOTE_EMERG, - &data->temp8[5]); + &data->temp8[TEMP8_REMOTE_EMERG]); } lm90_read_reg(client, LM90_REG_R_STATUS, &alarms); data->alarms = alarms; /* save as 16 bit value */ @@ -543,15 +585,16 @@ static struct lm90_data *lm90_update_device(struct device *dev) if (data->kind == max6696) { lm90_select_remote_channel(client, data, 1); lm90_read_reg(client, LM90_REG_R_REMOTE_CRIT, - &data->temp8[6]); + &data->temp8[TEMP8_REMOTE2_CRIT]); lm90_read_reg(client, MAX6659_REG_R_REMOTE_EMERG, - &data->temp8[7]); + &data->temp8[TEMP8_REMOTE2_EMERG]); lm90_read16(client, LM90_REG_R_REMOTE_TEMPH, - LM90_REG_R_REMOTE_TEMPL, &data->temp11[5]); + LM90_REG_R_REMOTE_TEMPL, + &data->temp11[TEMP11_REMOTE2_TEMP]); if (!lm90_read_reg(client, LM90_REG_R_REMOTE_LOWH, &h)) - data->temp11[6] = h << 8; + data->temp11[TEMP11_REMOTE2_LOW] = h << 8; if (!lm90_read_reg(client, LM90_REG_R_REMOTE_HIGHH, &h)) - data->temp11[7] = h << 8; + data->temp11[TEMP11_REMOTE2_HIGH] = h << 8; lm90_select_remote_channel(client, data, 0); if (!lm90_read_reg(client, MAX6696_REG_R_STATUS2, @@ -745,7 +788,7 @@ static ssize_t show_temp8(struct device *dev, struct device_attribute *devattr, static void write_temp8(struct device *dev, int index, long val) { - static const u8 reg[8] = { + static const u8 reg[TEMP8_REG_NUM] = { LM90_REG_W_LOCAL_LOW, LM90_REG_W_LOCAL_HIGH, LM90_REG_W_LOCAL_CRIT, @@ -828,7 +871,7 @@ static void write_temp11(struct device *dev, int nr, int index, long val) u8 high; u8 low; int channel; - } reg[5] = { + } reg[NR_NUM] = { { LM90_REG_W_REMOTE_LOWH, LM90_REG_W_REMOTE_LOWL, 0 }, { LM90_REG_W_REMOTE_HIGHH, LM90_REG_W_REMOTE_HIGHL, 0 }, { LM90_REG_W_REMOTE_OFFSH, LM90_REG_W_REMOTE_OFFSL, 0 }, @@ -919,11 +962,12 @@ static ssize_t set_temphyst(struct device *dev, struct device_attribute *dummy, mutex_lock(&data->update_lock); if (data->kind == adt7461) - temp = temp_from_u8_adt7461(data, data->temp8[2]); + temp = temp_from_u8_adt7461(data, + data->temp8[TEMP8_LOCAL_CRIT]); else if (data->kind == max6646) - temp = temp_from_u8(data->temp8[2]); + temp = temp_from_u8(data->temp8[TEMP8_LOCAL_CRIT]); else - temp = temp_from_s8(data->temp8[2]); + temp = temp_from_s8(data->temp8[TEMP8_LOCAL_CRIT]); data->temp_hyst = hyst_to_reg(temp - val); i2c_smbus_write_byte_data(client, LM90_REG_W_TCRIT_HYST, @@ -977,25 +1021,28 @@ static ssize_t set_update_interval(struct device *dev, return count; } -static SENSOR_DEVICE_ATTR_2(temp1_input, S_IRUGO, show_temp11, NULL, 0, 4); -static SENSOR_DEVICE_ATTR_2(temp2_input, S_IRUGO, show_temp11, NULL, 0, 0); +static SENSOR_DEVICE_ATTR_2(temp1_input, S_IRUGO, show_temp11, NULL, + NR_CHAN_0_REMOTE_LOW, TEMP11_LOCAL_TEMP); +static SENSOR_DEVICE_ATTR_2(temp2_input, S_IRUGO, show_temp11, NULL, + NR_CHAN_0_REMOTE_LOW, TEMP11_REMOTE_TEMP); static SENSOR_DEVICE_ATTR(temp1_min, S_IWUSR | S_IRUGO, show_temp8, - set_temp8, 0); + set_temp8, TEMP8_LOCAL_LOW); static SENSOR_DEVICE_ATTR_2(temp2_min, S_IWUSR | S_IRUGO, show_temp11, - set_temp11, 0, 1); + set_temp11, NR_CHAN_0_REMOTE_LOW, TEMP11_REMOTE_LOW); static SENSOR_DEVICE_ATTR(temp1_max, S_IWUSR | S_IRUGO, show_temp8, - set_temp8, 1); + set_temp8, TEMP8_LOCAL_HIGH); static SENSOR_DEVICE_ATTR_2(temp2_max, S_IWUSR | S_IRUGO, show_temp11, - set_temp11, 1, 2); + set_temp11, NR_CHAN_0_REMOTE_HIGH, TEMP11_REMOTE_HIGH); static SENSOR_DEVICE_ATTR(temp1_crit, S_IWUSR | S_IRUGO, show_temp8, - set_temp8, 2); + set_temp8, TEMP8_LOCAL_CRIT); static SENSOR_DEVICE_ATTR(temp2_crit, S_IWUSR | S_IRUGO, show_temp8, - set_temp8, 3); + set_temp8, TEMP8_REMOTE_CRIT); static SENSOR_DEVICE_ATTR(temp1_crit_hyst, S_IWUSR | S_IRUGO, show_temphyst, - set_temphyst, 2); -static SENSOR_DEVICE_ATTR(temp2_crit_hyst, S_IRUGO, show_temphyst, NULL, 3); + set_temphyst, TEMP8_LOCAL_CRIT); +static SENSOR_DEVICE_ATTR(temp2_crit_hyst, S_IRUGO, show_temphyst, NULL, + TEMP8_REMOTE_CRIT); static SENSOR_DEVICE_ATTR_2(temp2_offset, S_IWUSR | S_IRUGO, show_temp11, - set_temp11, 2, 3); + set_temp11, NR_CHAN_0_REMOTE_OFFSET, TEMP11_REMOTE_OFFSET); /* Individual alarm files */ static SENSOR_DEVICE_ATTR(temp1_crit_alarm, S_IRUGO, show_alarm, NULL, 0); @@ -1043,13 +1090,13 @@ static const struct attribute_group lm90_group = { * Additional attributes for devices with emergency sensors */ static SENSOR_DEVICE_ATTR(temp1_emergency, S_IWUSR | S_IRUGO, show_temp8, - set_temp8, 4); + set_temp8, TEMP8_LOCAL_EMERG); static SENSOR_DEVICE_ATTR(temp2_emergency, S_IWUSR | S_IRUGO, show_temp8, - set_temp8, 5); + set_temp8, TEMP8_REMOTE_EMERG); static SENSOR_DEVICE_ATTR(temp1_emergency_hyst, S_IRUGO, show_temphyst, - NULL, 4); + NULL, TEMP8_LOCAL_EMERG); static SENSOR_DEVICE_ATTR(temp2_emergency_hyst, S_IRUGO, show_temphyst, - NULL, 5); + NULL, TEMP8_REMOTE_EMERG); static struct attribute *lm90_emergency_attributes[] = { &sensor_dev_attr_temp1_emergency.dev_attr.attr, @@ -1079,18 +1126,20 @@ static const struct attribute_group lm90_emergency_alarm_group = { /* * Additional attributes for devices with 3 temperature sensors */ -static SENSOR_DEVICE_ATTR_2(temp3_input, S_IRUGO, show_temp11, NULL, 0, 5); +static SENSOR_DEVICE_ATTR_2(temp3_input, S_IRUGO, show_temp11, NULL, + NR_CHAN_0_REMOTE_LOW, TEMP11_REMOTE2_TEMP); static SENSOR_DEVICE_ATTR_2(temp3_min, S_IWUSR | S_IRUGO, show_temp11, - set_temp11, 3, 6); + set_temp11, NR_CHAN_1_REMOTE_LOW, TEMP11_REMOTE2_LOW); static SENSOR_DEVICE_ATTR_2(temp3_max, S_IWUSR | S_IRUGO, show_temp11, - set_temp11, 4, 7); + set_temp11, NR_CHAN_1_REMOTE_HIGH, TEMP11_REMOTE2_HIGH); static SENSOR_DEVICE_ATTR(temp3_crit, S_IWUSR | S_IRUGO, show_temp8, - set_temp8, 6); -static SENSOR_DEVICE_ATTR(temp3_crit_hyst, S_IRUGO, show_temphyst, NULL, 6); + set_temp8, TEMP8_REMOTE2_CRIT); +static SENSOR_DEVICE_ATTR(temp3_crit_hyst, S_IRUGO, show_temphyst, NULL, + TEMP8_REMOTE2_CRIT); static SENSOR_DEVICE_ATTR(temp3_emergency, S_IWUSR | S_IRUGO, show_temp8, - set_temp8, 7); + set_temp8, TEMP8_REMOTE2_EMERG); static SENSOR_DEVICE_ATTR(temp3_emergency_hyst, S_IRUGO, show_temphyst, - NULL, 7); + NULL, TEMP8_REMOTE2_EMERG); static SENSOR_DEVICE_ATTR(temp3_crit_alarm, S_IRUGO, show_alarm, NULL, 9); static SENSOR_DEVICE_ATTR(temp3_fault, S_IRUGO, show_alarm, NULL, 10); -- 1.7.9.5 ^ permalink raw reply related [flat|nested] 48+ messages in thread
* Re: [PATCH v3 4/4] hwmon: (lm90) use enums for the indexes of temp8 and temp11 2013-07-12 7:48 ` [PATCH v3 4/4] hwmon: (lm90) use enums for the indexes of temp8 and temp11 Wei Ni @ 2013-07-27 15:38 ` Jean Delvare 2013-07-29 11:15 ` Wei Ni 0 siblings, 1 reply; 48+ messages in thread From: Jean Delvare @ 2013-07-27 15:38 UTC (permalink / raw) To: Wei Ni; +Cc: linux, thierry.reding, lm-sensors, linux-kernel, linux-tegra Hi Wei, On Fri, 12 Jul 2013 15:48:07 +0800, Wei Ni wrote: > Using enums for the indexes and nrs of temp8 and temp11. > This make the code much more readable. I can't say I'm thrilled by this patch. The improved readability is questionable. In the original code, each line already had one constant which made it clear what the code was doing. So the gain is marginal, I'd say, and this costs almost 50 lines of code. Let me review it nevertheless: > > Signed-off-by: Wei Ni <wni@nvidia.com> > --- > drivers/hwmon/lm90.c | 179 ++++++++++++++++++++++++++++++++------------------ > 1 file changed, 114 insertions(+), 65 deletions(-) > > diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c > index 1cc3d19..8cb5dd0 100644 > --- a/drivers/hwmon/lm90.c > +++ b/drivers/hwmon/lm90.c > @@ -310,6 +310,59 @@ static const struct lm90_params lm90_params[] = { > }; > > /* > + * TEMP8 register index > + */ > +enum lm90_temp8_reg_index { > + TEMP8_LOCAL_LOW = 0, /* 0: local low limit */ > + TEMP8_LOCAL_HIGH, /* 1: local high limit */ > + TEMP8_LOCAL_CRIT, /* 2: local critical limit */ > + TEMP8_REMOTE_CRIT, /* 3: remote critical limit */ > + TEMP8_LOCAL_EMERG, /* 4: local emergency limit > + * (max6659 and max6695/96) > + */ > + TEMP8_REMOTE_EMERG, /* 5: remote emergency limit > + * (max6659 and max6695/96) > + */ > + TEMP8_REMOTE2_CRIT, /* 6: remote 2 critical limit > + * (max6695/96 only) > + */ > + TEMP8_REMOTE2_EMERG, /* 7: remote 2 emergency limit > + * (max6695/96 only) > + */ > + TEMP8_REG_NUM > +}; > + > +/* > + * TEMP11 register index > + */ > +enum lm90_temp11_reg_index { > + TEMP11_REMOTE_TEMP = 0, /* 0: remote input */ > + TEMP11_REMOTE_LOW, /* 1: remote low limit */ > + TEMP11_REMOTE_HIGH, /* 2: remote high limit */ > + TEMP11_REMOTE_OFFSET, /* 3: remote offset > + * (except max6646, max6657/58/59, > + * and max6695/96) > + */ > + TEMP11_LOCAL_TEMP, /* 4: local input */ > + TEMP11_REMOTE2_TEMP, /* 5: remote 2 input (max6695/96 only) */ > + TEMP11_REMOTE2_LOW, /* 6: remote 2 low limit (max6695/96 only) */ > + TEMP11_REMOTE2_HIGH, /* 7: remote 2 high limit (max6695/96 only) */ > + TEMP11_REG_NUM > +}; The "TEMP8_" and "TEMP11_" prefixes aren't really needed, as there is no overlapping between both sets. Removing these prefixes (except for the terminators, where they are needed and make sense) would make the rest of the code more readable IMHO, as it would avoid redundant information on most lines, and avoid line splitting in some cases. Also, the comments are mostly useless now, they were there to document what each number was referring to, but now this is exactly what the new constants are doing. > + > +/* > + * TEMP11 register NR > + */ > +enum lm90_temp11_reg_nr { > + NR_CHAN_0_REMOTE_LOW = 0, /* 0: channel 0, remote low limit */ > + NR_CHAN_0_REMOTE_HIGH, /* 1: channel 0, remote high limit */ > + NR_CHAN_0_REMOTE_OFFSET, /* 2: channel 0, remote offset */ > + NR_CHAN_1_REMOTE_LOW, /* 3: channel 1, remote low limit */ > + NR_CHAN_1_REMOTE_HIGH, /* 4: channel 1, remote high limit */ The conventions used in the descriptions diverge from the ones used above. "channel 0 remote" here is just "remove" above, and "channel 1 remote" is "remote 2" above. This is quite confusing. > + NR_NUM /* number of the NRs for temp11 */ The fact that you were unable to come up with a proper name for this number is a clear indication that this enum should not exist in the first place. These numbers are used only once, to pass specific information to set_temp11. This was easy enough when these were just numbers and I really had no reason not to do that. If you now want to use clean constants, this should be done with logic and consistency. Your proposal makes sense for the data->temp8 and data->temp11 array indexing, because they are used more than once. But introducing a new set of constants with weird names just for one use case doesn't help readability, it makes things worse actually. So if you insist of making the code more readable by the means of named constants, then you should simply adjust the reg array in write_temp11 so that it can be indexed with your TEMP11_* constants above (as is data->temp11.) This will leave some holes in the array but I don't think we care about a few lost bytes here. > +}; > + > +/* > * Client data (each client gets its own) > */ > > @@ -331,25 +384,8 @@ struct lm90_data { > u8 reg_local_ext; /* local extension register offset */ > > /* registers values */ > - s8 temp8[8]; /* 0: local low limit > - * 1: local high limit > - * 2: local critical limit > - * 3: remote critical limit > - * 4: local emergency limit (max6659 and max6695/96) > - * 5: remote emergency limit (max6659 and max6695/96) > - * 6: remote 2 critical limit (max6695/96 only) > - * 7: remote 2 emergency limit (max6695/96 only) > - */ > - s16 temp11[8]; /* 0: remote input > - * 1: remote low limit > - * 2: remote high limit > - * 3: remote offset (except max6646, max6657/58/59, > - * and max6695/96) > - * 4: local input > - * 5: remote 2 input (max6695/96 only) > - * 6: remote 2 low limit (max6695/96 only) > - * 7: remote 2 high limit (max6695/96 only) > - */ > + s8 temp8[TEMP8_REG_NUM]; > + s16 temp11[TEMP11_REG_NUM]; > u8 temp_hyst; > u16 alarms; /* bitvector (upper 8 bits for max6695/96) */ > }; > @@ -491,37 +527,42 @@ static struct lm90_data *lm90_update_device(struct device *dev) > u8 alarms; > > dev_dbg(&client->dev, "Updating lm90 data.\n"); > - lm90_read_reg(client, LM90_REG_R_LOCAL_LOW, &data->temp8[0]); > - lm90_read_reg(client, LM90_REG_R_LOCAL_HIGH, &data->temp8[1]); > - lm90_read_reg(client, LM90_REG_R_LOCAL_CRIT, &data->temp8[2]); > - lm90_read_reg(client, LM90_REG_R_REMOTE_CRIT, &data->temp8[3]); > + lm90_read_reg(client, LM90_REG_R_LOCAL_LOW, > + &data->temp8[TEMP8_LOCAL_LOW]); > + lm90_read_reg(client, LM90_REG_R_LOCAL_HIGH, > + &data->temp8[TEMP8_LOCAL_HIGH]); > + lm90_read_reg(client, LM90_REG_R_LOCAL_CRIT, > + &data->temp8[TEMP8_LOCAL_CRIT]); > + lm90_read_reg(client, LM90_REG_R_REMOTE_CRIT, > + &data->temp8[TEMP8_REMOTE_CRIT]); > lm90_read_reg(client, LM90_REG_R_TCRIT_HYST, &data->temp_hyst); > > if (data->reg_local_ext) { > lm90_read16(client, LM90_REG_R_LOCAL_TEMP, > data->reg_local_ext, > - &data->temp11[4]); > + &data->temp11[TEMP11_LOCAL_TEMP]); > } else { > if (lm90_read_reg(client, LM90_REG_R_LOCAL_TEMP, > &h) == 0) > - data->temp11[4] = h << 8; > + data->temp11[TEMP11_LOCAL_TEMP] = h << 8; > } > lm90_read16(client, LM90_REG_R_REMOTE_TEMPH, > - LM90_REG_R_REMOTE_TEMPL, &data->temp11[0]); > + LM90_REG_R_REMOTE_TEMPL, > + &data->temp11[TEMP11_REMOTE_TEMP]); Please don't break alignment. > > if (lm90_read_reg(client, LM90_REG_R_REMOTE_LOWH, &h) == 0) { > - data->temp11[1] = h << 8; > + data->temp11[TEMP11_REMOTE_LOW] = h << 8; > if ((data->flags & LM90_HAVE_REM_LIMIT_EXT) > && lm90_read_reg(client, LM90_REG_R_REMOTE_LOWL, > &l) == 0) > - data->temp11[1] |= l; > + data->temp11[TEMP11_REMOTE_LOW] |= l; > } > if (lm90_read_reg(client, LM90_REG_R_REMOTE_HIGHH, &h) == 0) { > - data->temp11[2] = h << 8; > + data->temp11[TEMP11_REMOTE_HIGH] = h << 8; > if ((data->flags & LM90_HAVE_REM_LIMIT_EXT) > && lm90_read_reg(client, LM90_REG_R_REMOTE_HIGHL, > &l) == 0) > - data->temp11[2] |= l; > + data->temp11[TEMP11_REMOTE_HIGH] |= l; > } > > if (data->flags & LM90_HAVE_OFFSET) { > @@ -529,13 +570,14 @@ static struct lm90_data *lm90_update_device(struct device *dev) > &h) == 0 > && lm90_read_reg(client, LM90_REG_R_REMOTE_OFFSL, > &l) == 0) > - data->temp11[3] = (h << 8) | l; > + data->temp11[TEMP11_REMOTE_OFFSET] = > + (h << 8) | l; > } > if (data->flags & LM90_HAVE_EMERGENCY) { > lm90_read_reg(client, MAX6659_REG_R_LOCAL_EMERG, > - &data->temp8[4]); > + &data->temp8[TEMP8_LOCAL_EMERG]); > lm90_read_reg(client, MAX6659_REG_R_REMOTE_EMERG, > - &data->temp8[5]); > + &data->temp8[TEMP8_REMOTE_EMERG]); > } > lm90_read_reg(client, LM90_REG_R_STATUS, &alarms); > data->alarms = alarms; /* save as 16 bit value */ > @@ -543,15 +585,16 @@ static struct lm90_data *lm90_update_device(struct device *dev) > if (data->kind == max6696) { > lm90_select_remote_channel(client, data, 1); > lm90_read_reg(client, LM90_REG_R_REMOTE_CRIT, > - &data->temp8[6]); > + &data->temp8[TEMP8_REMOTE2_CRIT]); > lm90_read_reg(client, MAX6659_REG_R_REMOTE_EMERG, > - &data->temp8[7]); > + &data->temp8[TEMP8_REMOTE2_EMERG]); > lm90_read16(client, LM90_REG_R_REMOTE_TEMPH, > - LM90_REG_R_REMOTE_TEMPL, &data->temp11[5]); > + LM90_REG_R_REMOTE_TEMPL, > + &data->temp11[TEMP11_REMOTE2_TEMP]); Please don't break alignment. > if (!lm90_read_reg(client, LM90_REG_R_REMOTE_LOWH, &h)) > - data->temp11[6] = h << 8; > + data->temp11[TEMP11_REMOTE2_LOW] = h << 8; > if (!lm90_read_reg(client, LM90_REG_R_REMOTE_HIGHH, &h)) > - data->temp11[7] = h << 8; > + data->temp11[TEMP11_REMOTE2_HIGH] = h << 8; > lm90_select_remote_channel(client, data, 0); > > if (!lm90_read_reg(client, MAX6696_REG_R_STATUS2, > @@ -745,7 +788,7 @@ static ssize_t show_temp8(struct device *dev, struct device_attribute *devattr, > > static void write_temp8(struct device *dev, int index, long val) > { > - static const u8 reg[8] = { > + static const u8 reg[TEMP8_REG_NUM] = { > LM90_REG_W_LOCAL_LOW, > LM90_REG_W_LOCAL_HIGH, > LM90_REG_W_LOCAL_CRIT, > @@ -828,7 +871,7 @@ static void write_temp11(struct device *dev, int nr, int index, long val) > u8 high; > u8 low; > int channel; > - } reg[5] = { > + } reg[NR_NUM] = { > { LM90_REG_W_REMOTE_LOWH, LM90_REG_W_REMOTE_LOWL, 0 }, > { LM90_REG_W_REMOTE_HIGHH, LM90_REG_W_REMOTE_HIGHL, 0 }, > { LM90_REG_W_REMOTE_OFFSH, LM90_REG_W_REMOTE_OFFSL, 0 }, > @@ -919,11 +962,12 @@ static ssize_t set_temphyst(struct device *dev, struct device_attribute *dummy, > > mutex_lock(&data->update_lock); > if (data->kind == adt7461) > - temp = temp_from_u8_adt7461(data, data->temp8[2]); > + temp = temp_from_u8_adt7461(data, > + data->temp8[TEMP8_LOCAL_CRIT]); Please align on opening parenthesis. > else if (data->kind == max6646) > - temp = temp_from_u8(data->temp8[2]); > + temp = temp_from_u8(data->temp8[TEMP8_LOCAL_CRIT]); > else > - temp = temp_from_s8(data->temp8[2]); > + temp = temp_from_s8(data->temp8[TEMP8_LOCAL_CRIT]); > > data->temp_hyst = hyst_to_reg(temp - val); > i2c_smbus_write_byte_data(client, LM90_REG_W_TCRIT_HYST, > @@ -977,25 +1021,28 @@ static ssize_t set_update_interval(struct device *dev, > return count; > } > > -static SENSOR_DEVICE_ATTR_2(temp1_input, S_IRUGO, show_temp11, NULL, 0, 4); > -static SENSOR_DEVICE_ATTR_2(temp2_input, S_IRUGO, show_temp11, NULL, 0, 0); > +static SENSOR_DEVICE_ATTR_2(temp1_input, S_IRUGO, show_temp11, NULL, > + NR_CHAN_0_REMOTE_LOW, TEMP11_LOCAL_TEMP); > +static SENSOR_DEVICE_ATTR_2(temp2_input, S_IRUGO, show_temp11, NULL, > + NR_CHAN_0_REMOTE_LOW, TEMP11_REMOTE_TEMP); NR_CHAN_0_REMOTE_LOW makes no sense for read-only attributes, as the number is only used in write_temp11(). The original "0" was only because we can't leave the parameter empty. > static SENSOR_DEVICE_ATTR(temp1_min, S_IWUSR | S_IRUGO, show_temp8, > - set_temp8, 0); > + set_temp8, TEMP8_LOCAL_LOW); > static SENSOR_DEVICE_ATTR_2(temp2_min, S_IWUSR | S_IRUGO, show_temp11, > - set_temp11, 0, 1); > + set_temp11, NR_CHAN_0_REMOTE_LOW, TEMP11_REMOTE_LOW); > static SENSOR_DEVICE_ATTR(temp1_max, S_IWUSR | S_IRUGO, show_temp8, > - set_temp8, 1); > + set_temp8, TEMP8_LOCAL_HIGH); > static SENSOR_DEVICE_ATTR_2(temp2_max, S_IWUSR | S_IRUGO, show_temp11, > - set_temp11, 1, 2); > + set_temp11, NR_CHAN_0_REMOTE_HIGH, TEMP11_REMOTE_HIGH); > static SENSOR_DEVICE_ATTR(temp1_crit, S_IWUSR | S_IRUGO, show_temp8, > - set_temp8, 2); > + set_temp8, TEMP8_LOCAL_CRIT); > static SENSOR_DEVICE_ATTR(temp2_crit, S_IWUSR | S_IRUGO, show_temp8, > - set_temp8, 3); > + set_temp8, TEMP8_REMOTE_CRIT); > static SENSOR_DEVICE_ATTR(temp1_crit_hyst, S_IWUSR | S_IRUGO, show_temphyst, > - set_temphyst, 2); > -static SENSOR_DEVICE_ATTR(temp2_crit_hyst, S_IRUGO, show_temphyst, NULL, 3); > + set_temphyst, TEMP8_LOCAL_CRIT); > +static SENSOR_DEVICE_ATTR(temp2_crit_hyst, S_IRUGO, show_temphyst, NULL, > + TEMP8_REMOTE_CRIT); > static SENSOR_DEVICE_ATTR_2(temp2_offset, S_IWUSR | S_IRUGO, show_temp11, > - set_temp11, 2, 3); > + set_temp11, NR_CHAN_0_REMOTE_OFFSET, TEMP11_REMOTE_OFFSET); > > /* Individual alarm files */ > static SENSOR_DEVICE_ATTR(temp1_crit_alarm, S_IRUGO, show_alarm, NULL, 0); > @@ -1043,13 +1090,13 @@ static const struct attribute_group lm90_group = { > * Additional attributes for devices with emergency sensors > */ > static SENSOR_DEVICE_ATTR(temp1_emergency, S_IWUSR | S_IRUGO, show_temp8, > - set_temp8, 4); > + set_temp8, TEMP8_LOCAL_EMERG); > static SENSOR_DEVICE_ATTR(temp2_emergency, S_IWUSR | S_IRUGO, show_temp8, > - set_temp8, 5); > + set_temp8, TEMP8_REMOTE_EMERG); > static SENSOR_DEVICE_ATTR(temp1_emergency_hyst, S_IRUGO, show_temphyst, > - NULL, 4); > + NULL, TEMP8_LOCAL_EMERG); > static SENSOR_DEVICE_ATTR(temp2_emergency_hyst, S_IRUGO, show_temphyst, > - NULL, 5); > + NULL, TEMP8_REMOTE_EMERG); > > static struct attribute *lm90_emergency_attributes[] = { > &sensor_dev_attr_temp1_emergency.dev_attr.attr, > @@ -1079,18 +1126,20 @@ static const struct attribute_group lm90_emergency_alarm_group = { > /* > * Additional attributes for devices with 3 temperature sensors > */ > -static SENSOR_DEVICE_ATTR_2(temp3_input, S_IRUGO, show_temp11, NULL, 0, 5); > +static SENSOR_DEVICE_ATTR_2(temp3_input, S_IRUGO, show_temp11, NULL, > + NR_CHAN_0_REMOTE_LOW, TEMP11_REMOTE2_TEMP); Here again this NR_CHAN_0_REMOTE_LOW makes no sense for a read-only attribute, it was really "0" for "we don't care". > static SENSOR_DEVICE_ATTR_2(temp3_min, S_IWUSR | S_IRUGO, show_temp11, > - set_temp11, 3, 6); > + set_temp11, NR_CHAN_1_REMOTE_LOW, TEMP11_REMOTE2_LOW); > static SENSOR_DEVICE_ATTR_2(temp3_max, S_IWUSR | S_IRUGO, show_temp11, > - set_temp11, 4, 7); > + set_temp11, NR_CHAN_1_REMOTE_HIGH, TEMP11_REMOTE2_HIGH); > static SENSOR_DEVICE_ATTR(temp3_crit, S_IWUSR | S_IRUGO, show_temp8, > - set_temp8, 6); > -static SENSOR_DEVICE_ATTR(temp3_crit_hyst, S_IRUGO, show_temphyst, NULL, 6); > + set_temp8, TEMP8_REMOTE2_CRIT); > +static SENSOR_DEVICE_ATTR(temp3_crit_hyst, S_IRUGO, show_temphyst, NULL, > + TEMP8_REMOTE2_CRIT); > static SENSOR_DEVICE_ATTR(temp3_emergency, S_IWUSR | S_IRUGO, show_temp8, > - set_temp8, 7); > + set_temp8, TEMP8_REMOTE2_EMERG); > static SENSOR_DEVICE_ATTR(temp3_emergency_hyst, S_IRUGO, show_temphyst, > - NULL, 7); > + NULL, TEMP8_REMOTE2_EMERG); > > static SENSOR_DEVICE_ATTR(temp3_crit_alarm, S_IRUGO, show_alarm, NULL, 9); > static SENSOR_DEVICE_ATTR(temp3_fault, S_IRUGO, show_alarm, NULL, 10); Will all these changes done, I may consider apply the modified patch, but no guarantee, as I'm still not sure I like it. I'll make my mind when I see the result. -- Jean Delvare ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v3 4/4] hwmon: (lm90) use enums for the indexes of temp8 and temp11 2013-07-27 15:38 ` Jean Delvare @ 2013-07-29 11:15 ` Wei Ni 2013-07-29 15:48 ` Jean Delvare 0 siblings, 1 reply; 48+ messages in thread From: Wei Ni @ 2013-07-29 11:15 UTC (permalink / raw) To: Jean Delvare; +Cc: linux, thierry.reding, lm-sensors, linux-kernel, linux-tegra On 07/27/2013 11:38 PM, Jean Delvare wrote: > Hi Wei, > > On Fri, 12 Jul 2013 15:48:07 +0800, Wei Ni wrote: >> Using enums for the indexes and nrs of temp8 and temp11. >> This make the code much more readable. > > I can't say I'm thrilled by this patch. The improved readability is > questionable. In the original code, each line already had one constant > which made it clear what the code was doing. So the gain is marginal, > I'd say, and this costs almost 50 lines of code. > > Let me review it nevertheless: > >> >> Signed-off-by: Wei Ni <wni@nvidia.com> >> --- >> drivers/hwmon/lm90.c | 179 ++++++++++++++++++++++++++++++++------------------ >> 1 file changed, 114 insertions(+), 65 deletions(-) >> >> diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c >> index 1cc3d19..8cb5dd0 100644 >> --- a/drivers/hwmon/lm90.c >> +++ b/drivers/hwmon/lm90.c >> @@ -310,6 +310,59 @@ static const struct lm90_params lm90_params[] = { >> }; >> >> /* >> + * TEMP8 register index >> + */ >> +enum lm90_temp8_reg_index { >> + TEMP8_LOCAL_LOW = 0, /* 0: local low limit */ >> + TEMP8_LOCAL_HIGH, /* 1: local high limit */ >> + TEMP8_LOCAL_CRIT, /* 2: local critical limit */ >> + TEMP8_REMOTE_CRIT, /* 3: remote critical limit */ >> + TEMP8_LOCAL_EMERG, /* 4: local emergency limit >> + * (max6659 and max6695/96) >> + */ >> + TEMP8_REMOTE_EMERG, /* 5: remote emergency limit >> + * (max6659 and max6695/96) >> + */ >> + TEMP8_REMOTE2_CRIT, /* 6: remote 2 critical limit >> + * (max6695/96 only) >> + */ >> + TEMP8_REMOTE2_EMERG, /* 7: remote 2 emergency limit >> + * (max6695/96 only) >> + */ >> + TEMP8_REG_NUM >> +}; >> + >> +/* >> + * TEMP11 register index >> + */ >> +enum lm90_temp11_reg_index { >> + TEMP11_REMOTE_TEMP = 0, /* 0: remote input */ >> + TEMP11_REMOTE_LOW, /* 1: remote low limit */ >> + TEMP11_REMOTE_HIGH, /* 2: remote high limit */ >> + TEMP11_REMOTE_OFFSET, /* 3: remote offset >> + * (except max6646, max6657/58/59, >> + * and max6695/96) >> + */ >> + TEMP11_LOCAL_TEMP, /* 4: local input */ >> + TEMP11_REMOTE2_TEMP, /* 5: remote 2 input (max6695/96 only) */ >> + TEMP11_REMOTE2_LOW, /* 6: remote 2 low limit (max6695/96 only) */ >> + TEMP11_REMOTE2_HIGH, /* 7: remote 2 high limit (max6695/96 only) */ >> + TEMP11_REG_NUM >> +}; > > The "TEMP8_" and "TEMP11_" prefixes aren't really needed, as there is no > overlapping between both sets. Removing these prefixes (except for the > terminators, where they are needed and make sense) would make the rest > of the code more readable IMHO, as it would avoid redundant information > on most lines, and avoid line splitting in some cases. Yes, make sense, I will change it. > > Also, the comments are mostly useless now, they were there to document > what each number was referring to, but now this is exactly what the new > constants are doing. Yes, we can remove these comments, but I think it's better to remain those exception and only things. > >> + >> +/* >> + * TEMP11 register NR >> + */ >> +enum lm90_temp11_reg_nr { >> + NR_CHAN_0_REMOTE_LOW = 0, /* 0: channel 0, remote low limit */ >> + NR_CHAN_0_REMOTE_HIGH, /* 1: channel 0, remote high limit */ >> + NR_CHAN_0_REMOTE_OFFSET, /* 2: channel 0, remote offset */ >> + NR_CHAN_1_REMOTE_LOW, /* 3: channel 1, remote low limit */ >> + NR_CHAN_1_REMOTE_HIGH, /* 4: channel 1, remote high limit */ > > The conventions used in the descriptions diverge from the ones used > above. "channel 0 remote" here is just "remove" above, and "channel 1 > remote" is "remote 2" above. This is quite confusing. > >> + NR_NUM /* number of the NRs for temp11 */ > > The fact that you were unable to come up with a proper name for this > number is a clear indication that this enum should not exist in the > first place. > > These numbers are used only once, to pass specific information to > set_temp11. This was easy enough when these were just numbers and I > really had no reason not to do that. Ok, so how about to remove these changes, and keep the original codes to use numbers. > > If you now want to use clean constants, this should be done with logic > and consistency. Your proposal makes sense for the data->temp8 and > data->temp11 array indexing, because they are used more than once. But > introducing a new set of constants with weird names just for one use > case doesn't help readability, it makes things worse actually. > > So if you insist of making the code more readable by the means of named > constants, then you should simply adjust the reg array in write_temp11 > so that it can be indexed with your TEMP11_* constants above (as is > data->temp11.) This will leave some holes in the array but I don't > think we care about a few lost bytes here. > >> +}; >> + >> +/* >> * Client data (each client gets its own) >> */ >> >> @@ -331,25 +384,8 @@ struct lm90_data { >> u8 reg_local_ext; /* local extension register offset */ >> >> /* registers values */ >> - s8 temp8[8]; /* 0: local low limit >> - * 1: local high limit >> - * 2: local critical limit >> - * 3: remote critical limit >> - * 4: local emergency limit (max6659 and max6695/96) >> - * 5: remote emergency limit (max6659 and max6695/96) >> - * 6: remote 2 critical limit (max6695/96 only) >> - * 7: remote 2 emergency limit (max6695/96 only) >> - */ >> - s16 temp11[8]; /* 0: remote input >> - * 1: remote low limit >> - * 2: remote high limit >> - * 3: remote offset (except max6646, max6657/58/59, >> - * and max6695/96) >> - * 4: local input >> - * 5: remote 2 input (max6695/96 only) >> - * 6: remote 2 low limit (max6695/96 only) >> - * 7: remote 2 high limit (max6695/96 only) >> - */ >> + s8 temp8[TEMP8_REG_NUM]; >> + s16 temp11[TEMP11_REG_NUM]; >> u8 temp_hyst; >> u16 alarms; /* bitvector (upper 8 bits for max6695/96) */ >> }; >> @@ -491,37 +527,42 @@ static struct lm90_data *lm90_update_device(struct device *dev) >> u8 alarms; >> >> dev_dbg(&client->dev, "Updating lm90 data.\n"); >> - lm90_read_reg(client, LM90_REG_R_LOCAL_LOW, &data->temp8[0]); >> - lm90_read_reg(client, LM90_REG_R_LOCAL_HIGH, &data->temp8[1]); >> - lm90_read_reg(client, LM90_REG_R_LOCAL_CRIT, &data->temp8[2]); >> - lm90_read_reg(client, LM90_REG_R_REMOTE_CRIT, &data->temp8[3]); >> + lm90_read_reg(client, LM90_REG_R_LOCAL_LOW, >> + &data->temp8[TEMP8_LOCAL_LOW]); >> + lm90_read_reg(client, LM90_REG_R_LOCAL_HIGH, >> + &data->temp8[TEMP8_LOCAL_HIGH]); >> + lm90_read_reg(client, LM90_REG_R_LOCAL_CRIT, >> + &data->temp8[TEMP8_LOCAL_CRIT]); >> + lm90_read_reg(client, LM90_REG_R_REMOTE_CRIT, >> + &data->temp8[TEMP8_REMOTE_CRIT]); >> lm90_read_reg(client, LM90_REG_R_TCRIT_HYST, &data->temp_hyst); >> >> if (data->reg_local_ext) { >> lm90_read16(client, LM90_REG_R_LOCAL_TEMP, >> data->reg_local_ext, >> - &data->temp11[4]); >> + &data->temp11[TEMP11_LOCAL_TEMP]); >> } else { >> if (lm90_read_reg(client, LM90_REG_R_LOCAL_TEMP, >> &h) == 0) >> - data->temp11[4] = h << 8; >> + data->temp11[TEMP11_LOCAL_TEMP] = h << 8; >> } >> lm90_read16(client, LM90_REG_R_REMOTE_TEMPH, >> - LM90_REG_R_REMOTE_TEMPL, &data->temp11[0]); >> + LM90_REG_R_REMOTE_TEMPL, >> + &data->temp11[TEMP11_REMOTE_TEMP]); > > Please don't break alignment. Yes, I will do it. > >> >> if (lm90_read_reg(client, LM90_REG_R_REMOTE_LOWH, &h) == 0) { >> - data->temp11[1] = h << 8; >> + data->temp11[TEMP11_REMOTE_LOW] = h << 8; >> if ((data->flags & LM90_HAVE_REM_LIMIT_EXT) >> && lm90_read_reg(client, LM90_REG_R_REMOTE_LOWL, >> &l) == 0) >> - data->temp11[1] |= l; >> + data->temp11[TEMP11_REMOTE_LOW] |= l; >> } >> if (lm90_read_reg(client, LM90_REG_R_REMOTE_HIGHH, &h) == 0) { >> - data->temp11[2] = h << 8; >> + data->temp11[TEMP11_REMOTE_HIGH] = h << 8; >> if ((data->flags & LM90_HAVE_REM_LIMIT_EXT) >> && lm90_read_reg(client, LM90_REG_R_REMOTE_HIGHL, >> &l) == 0) >> - data->temp11[2] |= l; >> + data->temp11[TEMP11_REMOTE_HIGH] |= l; >> } >> >> if (data->flags & LM90_HAVE_OFFSET) { >> @@ -529,13 +570,14 @@ static struct lm90_data *lm90_update_device(struct device *dev) >> &h) == 0 >> && lm90_read_reg(client, LM90_REG_R_REMOTE_OFFSL, >> &l) == 0) >> - data->temp11[3] = (h << 8) | l; >> + data->temp11[TEMP11_REMOTE_OFFSET] = >> + (h << 8) | l; >> } >> if (data->flags & LM90_HAVE_EMERGENCY) { >> lm90_read_reg(client, MAX6659_REG_R_LOCAL_EMERG, >> - &data->temp8[4]); >> + &data->temp8[TEMP8_LOCAL_EMERG]); >> lm90_read_reg(client, MAX6659_REG_R_REMOTE_EMERG, >> - &data->temp8[5]); >> + &data->temp8[TEMP8_REMOTE_EMERG]); >> } >> lm90_read_reg(client, LM90_REG_R_STATUS, &alarms); >> data->alarms = alarms; /* save as 16 bit value */ >> @@ -543,15 +585,16 @@ static struct lm90_data *lm90_update_device(struct device *dev) >> if (data->kind == max6696) { >> lm90_select_remote_channel(client, data, 1); >> lm90_read_reg(client, LM90_REG_R_REMOTE_CRIT, >> - &data->temp8[6]); >> + &data->temp8[TEMP8_REMOTE2_CRIT]); >> lm90_read_reg(client, MAX6659_REG_R_REMOTE_EMERG, >> - &data->temp8[7]); >> + &data->temp8[TEMP8_REMOTE2_EMERG]); >> lm90_read16(client, LM90_REG_R_REMOTE_TEMPH, >> - LM90_REG_R_REMOTE_TEMPL, &data->temp11[5]); >> + LM90_REG_R_REMOTE_TEMPL, >> + &data->temp11[TEMP11_REMOTE2_TEMP]); > > Please don't break alignment. > >> if (!lm90_read_reg(client, LM90_REG_R_REMOTE_LOWH, &h)) >> - data->temp11[6] = h << 8; >> + data->temp11[TEMP11_REMOTE2_LOW] = h << 8; >> if (!lm90_read_reg(client, LM90_REG_R_REMOTE_HIGHH, &h)) >> - data->temp11[7] = h << 8; >> + data->temp11[TEMP11_REMOTE2_HIGH] = h << 8; >> lm90_select_remote_channel(client, data, 0); >> >> if (!lm90_read_reg(client, MAX6696_REG_R_STATUS2, >> @@ -745,7 +788,7 @@ static ssize_t show_temp8(struct device *dev, struct device_attribute *devattr, >> >> static void write_temp8(struct device *dev, int index, long val) >> { >> - static const u8 reg[8] = { >> + static const u8 reg[TEMP8_REG_NUM] = { >> LM90_REG_W_LOCAL_LOW, >> LM90_REG_W_LOCAL_HIGH, >> LM90_REG_W_LOCAL_CRIT, >> @@ -828,7 +871,7 @@ static void write_temp11(struct device *dev, int nr, int index, long val) >> u8 high; >> u8 low; >> int channel; >> - } reg[5] = { >> + } reg[NR_NUM] = { >> { LM90_REG_W_REMOTE_LOWH, LM90_REG_W_REMOTE_LOWL, 0 }, >> { LM90_REG_W_REMOTE_HIGHH, LM90_REG_W_REMOTE_HIGHL, 0 }, >> { LM90_REG_W_REMOTE_OFFSH, LM90_REG_W_REMOTE_OFFSL, 0 }, >> @@ -919,11 +962,12 @@ static ssize_t set_temphyst(struct device *dev, struct device_attribute *dummy, >> >> mutex_lock(&data->update_lock); >> if (data->kind == adt7461) >> - temp = temp_from_u8_adt7461(data, data->temp8[2]); >> + temp = temp_from_u8_adt7461(data, >> + data->temp8[TEMP8_LOCAL_CRIT]); > > Please align on opening parenthesis. > >> else if (data->kind == max6646) >> - temp = temp_from_u8(data->temp8[2]); >> + temp = temp_from_u8(data->temp8[TEMP8_LOCAL_CRIT]); >> else >> - temp = temp_from_s8(data->temp8[2]); >> + temp = temp_from_s8(data->temp8[TEMP8_LOCAL_CRIT]); >> >> data->temp_hyst = hyst_to_reg(temp - val); >> i2c_smbus_write_byte_data(client, LM90_REG_W_TCRIT_HYST, >> @@ -977,25 +1021,28 @@ static ssize_t set_update_interval(struct device *dev, >> return count; >> } >> >> -static SENSOR_DEVICE_ATTR_2(temp1_input, S_IRUGO, show_temp11, NULL, 0, 4); >> -static SENSOR_DEVICE_ATTR_2(temp2_input, S_IRUGO, show_temp11, NULL, 0, 0); >> +static SENSOR_DEVICE_ATTR_2(temp1_input, S_IRUGO, show_temp11, NULL, >> + NR_CHAN_0_REMOTE_LOW, TEMP11_LOCAL_TEMP); >> +static SENSOR_DEVICE_ATTR_2(temp2_input, S_IRUGO, show_temp11, NULL, >> + NR_CHAN_0_REMOTE_LOW, TEMP11_REMOTE_TEMP); > > NR_CHAN_0_REMOTE_LOW makes no sense for read-only attributes, as the > number is only used in write_temp11(). The original "0" was only > because we can't leave the parameter empty. > >> static SENSOR_DEVICE_ATTR(temp1_min, S_IWUSR | S_IRUGO, show_temp8, >> - set_temp8, 0); >> + set_temp8, TEMP8_LOCAL_LOW); >> static SENSOR_DEVICE_ATTR_2(temp2_min, S_IWUSR | S_IRUGO, show_temp11, >> - set_temp11, 0, 1); >> + set_temp11, NR_CHAN_0_REMOTE_LOW, TEMP11_REMOTE_LOW); >> static SENSOR_DEVICE_ATTR(temp1_max, S_IWUSR | S_IRUGO, show_temp8, >> - set_temp8, 1); >> + set_temp8, TEMP8_LOCAL_HIGH); >> static SENSOR_DEVICE_ATTR_2(temp2_max, S_IWUSR | S_IRUGO, show_temp11, >> - set_temp11, 1, 2); >> + set_temp11, NR_CHAN_0_REMOTE_HIGH, TEMP11_REMOTE_HIGH); >> static SENSOR_DEVICE_ATTR(temp1_crit, S_IWUSR | S_IRUGO, show_temp8, >> - set_temp8, 2); >> + set_temp8, TEMP8_LOCAL_CRIT); >> static SENSOR_DEVICE_ATTR(temp2_crit, S_IWUSR | S_IRUGO, show_temp8, >> - set_temp8, 3); >> + set_temp8, TEMP8_REMOTE_CRIT); >> static SENSOR_DEVICE_ATTR(temp1_crit_hyst, S_IWUSR | S_IRUGO, show_temphyst, >> - set_temphyst, 2); >> -static SENSOR_DEVICE_ATTR(temp2_crit_hyst, S_IRUGO, show_temphyst, NULL, 3); >> + set_temphyst, TEMP8_LOCAL_CRIT); >> +static SENSOR_DEVICE_ATTR(temp2_crit_hyst, S_IRUGO, show_temphyst, NULL, >> + TEMP8_REMOTE_CRIT); >> static SENSOR_DEVICE_ATTR_2(temp2_offset, S_IWUSR | S_IRUGO, show_temp11, >> - set_temp11, 2, 3); >> + set_temp11, NR_CHAN_0_REMOTE_OFFSET, TEMP11_REMOTE_OFFSET); >> >> /* Individual alarm files */ >> static SENSOR_DEVICE_ATTR(temp1_crit_alarm, S_IRUGO, show_alarm, NULL, 0); >> @@ -1043,13 +1090,13 @@ static const struct attribute_group lm90_group = { >> * Additional attributes for devices with emergency sensors >> */ >> static SENSOR_DEVICE_ATTR(temp1_emergency, S_IWUSR | S_IRUGO, show_temp8, >> - set_temp8, 4); >> + set_temp8, TEMP8_LOCAL_EMERG); >> static SENSOR_DEVICE_ATTR(temp2_emergency, S_IWUSR | S_IRUGO, show_temp8, >> - set_temp8, 5); >> + set_temp8, TEMP8_REMOTE_EMERG); >> static SENSOR_DEVICE_ATTR(temp1_emergency_hyst, S_IRUGO, show_temphyst, >> - NULL, 4); >> + NULL, TEMP8_LOCAL_EMERG); >> static SENSOR_DEVICE_ATTR(temp2_emergency_hyst, S_IRUGO, show_temphyst, >> - NULL, 5); >> + NULL, TEMP8_REMOTE_EMERG); >> >> static struct attribute *lm90_emergency_attributes[] = { >> &sensor_dev_attr_temp1_emergency.dev_attr.attr, >> @@ -1079,18 +1126,20 @@ static const struct attribute_group lm90_emergency_alarm_group = { >> /* >> * Additional attributes for devices with 3 temperature sensors >> */ >> -static SENSOR_DEVICE_ATTR_2(temp3_input, S_IRUGO, show_temp11, NULL, 0, 5); >> +static SENSOR_DEVICE_ATTR_2(temp3_input, S_IRUGO, show_temp11, NULL, >> + NR_CHAN_0_REMOTE_LOW, TEMP11_REMOTE2_TEMP); > > Here again this NR_CHAN_0_REMOTE_LOW makes no sense for a read-only > attribute, it was really "0" for "we don't care". > >> static SENSOR_DEVICE_ATTR_2(temp3_min, S_IWUSR | S_IRUGO, show_temp11, >> - set_temp11, 3, 6); >> + set_temp11, NR_CHAN_1_REMOTE_LOW, TEMP11_REMOTE2_LOW); >> static SENSOR_DEVICE_ATTR_2(temp3_max, S_IWUSR | S_IRUGO, show_temp11, >> - set_temp11, 4, 7); >> + set_temp11, NR_CHAN_1_REMOTE_HIGH, TEMP11_REMOTE2_HIGH); >> static SENSOR_DEVICE_ATTR(temp3_crit, S_IWUSR | S_IRUGO, show_temp8, >> - set_temp8, 6); >> -static SENSOR_DEVICE_ATTR(temp3_crit_hyst, S_IRUGO, show_temphyst, NULL, 6); >> + set_temp8, TEMP8_REMOTE2_CRIT); >> +static SENSOR_DEVICE_ATTR(temp3_crit_hyst, S_IRUGO, show_temphyst, NULL, >> + TEMP8_REMOTE2_CRIT); >> static SENSOR_DEVICE_ATTR(temp3_emergency, S_IWUSR | S_IRUGO, show_temp8, >> - set_temp8, 7); >> + set_temp8, TEMP8_REMOTE2_EMERG); >> static SENSOR_DEVICE_ATTR(temp3_emergency_hyst, S_IRUGO, show_temphyst, >> - NULL, 7); >> + NULL, TEMP8_REMOTE2_EMERG); >> >> static SENSOR_DEVICE_ATTR(temp3_crit_alarm, S_IRUGO, show_alarm, NULL, 9); >> static SENSOR_DEVICE_ATTR(temp3_fault, S_IRUGO, show_alarm, NULL, 10); > > Will all these changes done, I may consider apply the modified patch, > but no guarantee, as I'm still not sure I like it. I'll make my mind > when I see the result. I really appreciate you reviewed my patches so carefully. I will update changes in my next version. Thanks. Wei. > > -- > Jean Delvare > ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v3 4/4] hwmon: (lm90) use enums for the indexes of temp8 and temp11 2013-07-29 11:15 ` Wei Ni @ 2013-07-29 15:48 ` Jean Delvare 0 siblings, 0 replies; 48+ messages in thread From: Jean Delvare @ 2013-07-29 15:48 UTC (permalink / raw) To: Wei Ni; +Cc: linux, thierry.reding, lm-sensors, linux-kernel, linux-tegra Hi Wei, On Mon, 29 Jul 2013 19:15:12 +0800, Wei Ni wrote: > On 07/27/2013 11:38 PM, Jean Delvare wrote: > > On Fri, 12 Jul 2013 15:48:07 +0800, Wei Ni wrote: > >> +/* > >> + * TEMP11 register index > >> + */ > >> +enum lm90_temp11_reg_index { > >> + TEMP11_REMOTE_TEMP = 0, /* 0: remote input */ > >> + TEMP11_REMOTE_LOW, /* 1: remote low limit */ > >> + TEMP11_REMOTE_HIGH, /* 2: remote high limit */ > >> + TEMP11_REMOTE_OFFSET, /* 3: remote offset > >> + * (except max6646, max6657/58/59, > >> + * and max6695/96) > >> + */ > >> + TEMP11_LOCAL_TEMP, /* 4: local input */ > >> + TEMP11_REMOTE2_TEMP, /* 5: remote 2 input (max6695/96 only) */ > >> + TEMP11_REMOTE2_LOW, /* 6: remote 2 low limit (max6695/96 only) */ > >> + TEMP11_REMOTE2_HIGH, /* 7: remote 2 high limit (max6695/96 only) */ > >> + TEMP11_REG_NUM > >> +}; > > (...) > > Also, the comments are mostly useless now, they were there to document > > what each number was referring to, but now this is exactly what the new > > constants are doing. > > Yes, we can remove these comments, but I think it's better to remain > those exception and only things. Yes, I agree. > >> + > >> +/* > >> + * TEMP11 register NR > >> + */ > >> +enum lm90_temp11_reg_nr { > >> + NR_CHAN_0_REMOTE_LOW = 0, /* 0: channel 0, remote low limit */ > >> + NR_CHAN_0_REMOTE_HIGH, /* 1: channel 0, remote high limit */ > >> + NR_CHAN_0_REMOTE_OFFSET, /* 2: channel 0, remote offset */ > >> + NR_CHAN_1_REMOTE_LOW, /* 3: channel 1, remote low limit */ > >> + NR_CHAN_1_REMOTE_HIGH, /* 4: channel 1, remote high limit */ > > > > The conventions used in the descriptions diverge from the ones used > > above. "channel 0 remote" here is just "remove" above, and "channel 1 > > remote" is "remote 2" above. This is quite confusing. > > > >> + NR_NUM /* number of the NRs for temp11 */ > > > > The fact that you were unable to come up with a proper name for this > > number is a clear indication that this enum should not exist in the > > first place. > > > > These numbers are used only once, to pass specific information to > > set_temp11. This was easy enough when these were just numbers and I > > really had no reason not to do that. > > Ok, so how about to remove these changes, and keep the original codes to > use numbers. Fine with me. We can always change the code later to use the TEMP11 index values instead if anyone cares, this can be done separately. -- Jean Delvare ^ permalink raw reply [flat|nested] 48+ messages in thread
* [PATCH v3 0/4] expose lm90 to thermal fw @ 2014-08-25 6:29 Wei Ni 2014-08-25 6:29 ` [PATCH v3 1/4] hwmon: (lm90) split set&show temp as common codes Wei Ni 0 siblings, 1 reply; 48+ messages in thread From: Wei Ni @ 2014-08-25 6:29 UTC (permalink / raw) To: edubezval, khali, linux, swarren Cc: lm-sensors, linux-tegra, linux-kernel, Wei Ni Expose lm90 to thermal framework via DT nodes. This series is v3, previous version patches are: [v2]: https://lkml.org/lkml/2014/3/4/194 Changes from v2: add more description in documentation, per Stephen's comment. Changes from v1: 1. remove the unnecessary log messages, per Guenter's request. 2. add thermal zones node for nct1008 on dalmore. Wei Ni (1): thermal: add more description for thermal-zones lightning314 (3): hwmon: (lm90) split set&show temp as common codes hwmon: lm90: expose to thermal fw via DT nodes ARM: tegra: dalmore: add thermal zones for nct1008 .../devicetree/bindings/thermal/thermal.txt | 10 +- arch/arm/boot/dts/tegra114-dalmore.dts | 20 +- drivers/hwmon/lm90.c | 222 ++++++++++++++++----- 3 files changed, 192 insertions(+), 60 deletions(-) -- 1.8.1.5 ^ permalink raw reply [flat|nested] 48+ messages in thread
* [PATCH v3 1/4] hwmon: (lm90) split set&show temp as common codes 2014-08-25 6:29 [PATCH v3 0/4] expose lm90 to thermal fw Wei Ni @ 2014-08-25 6:29 ` Wei Ni 2014-08-25 12:23 ` Mikko Perttunen 0 siblings, 1 reply; 48+ messages in thread From: Wei Ni @ 2014-08-25 6:29 UTC (permalink / raw) To: edubezval, khali, linux, swarren Cc: lm-sensors, linux-tegra, linux-kernel, lightning314 From: lightning314 <wni@nvidia.com> Split set&show temp codes as common functions, so we can use it directly when implement linux thermal framework. And handle error return value for the lm90_select_remote_channel and write_tempx, then set_temp8 and set_temp11 could return it to user-space. Signed-off-by: Wei Ni <wni@nvidia.com> Signed-off-by: Jean Delvare <khali@linux-fr.org> --- drivers/hwmon/lm90.c | 164 ++++++++++++++++++++++++++++++++++----------------- 1 file changed, 109 insertions(+), 55 deletions(-) diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c index c9ff08d..cb33dcf 100644 --- a/drivers/hwmon/lm90.c +++ b/drivers/hwmon/lm90.c @@ -473,20 +473,23 @@ static int lm90_read16(struct i2c_client *client, u8 regh, u8 regl, u16 *value) * various registers have different meanings as a result of selecting a * non-default remote channel. */ -static inline void lm90_select_remote_channel(struct i2c_client *client, - struct lm90_data *data, - int channel) +static inline int lm90_select_remote_channel(struct i2c_client *client, + struct lm90_data *data, + int channel) { u8 config; + int err = 0; if (data->kind == max6696) { lm90_read_reg(client, LM90_REG_R_CONFIG1, &config); config &= ~0x08; if (channel) config |= 0x08; - i2c_smbus_write_byte_data(client, LM90_REG_W_CONFIG1, - config); + err = i2c_smbus_write_byte_data(client, LM90_REG_W_CONFIG1, + config); } + + return err; } /* @@ -759,29 +762,34 @@ static u16 temp_to_u16_adt7461(struct lm90_data *data, long val) * Sysfs stuff */ -static ssize_t show_temp8(struct device *dev, struct device_attribute *devattr, - char *buf) +static int read_temp8(struct device *dev, int index) { - struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr); struct lm90_data *data = lm90_update_device(dev); int temp; if (data->kind == adt7461 || data->kind == tmp451) - temp = temp_from_u8_adt7461(data, data->temp8[attr->index]); + temp = temp_from_u8_adt7461(data, data->temp8[index]); else if (data->kind == max6646) - temp = temp_from_u8(data->temp8[attr->index]); + temp = temp_from_u8(data->temp8[index]); else - temp = temp_from_s8(data->temp8[attr->index]); + temp = temp_from_s8(data->temp8[index]); /* +16 degrees offset for temp2 for the LM99 */ - if (data->kind == lm99 && attr->index == 3) + if (data->kind == lm99 && index == 3) temp += 16000; - return sprintf(buf, "%d\n", temp); + return temp; } -static ssize_t set_temp8(struct device *dev, struct device_attribute *devattr, - const char *buf, size_t count) +static ssize_t show_temp8(struct device *dev, struct device_attribute *devattr, + char *buf) +{ + struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr); + + return sprintf(buf, "%d\n", read_temp8(dev, attr->index)); +} + +static int write_temp8(struct device *dev, int index, long val) { static const u8 reg[TEMP8_REG_NUM] = { LM90_REG_W_LOCAL_LOW, @@ -794,60 +802,79 @@ static ssize_t set_temp8(struct device *dev, struct device_attribute *devattr, MAX6659_REG_W_REMOTE_EMERG, }; - struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr); struct lm90_data *data = dev_get_drvdata(dev); struct i2c_client *client = data->client; - int nr = attr->index; - long val; int err; - err = kstrtol(buf, 10, &val); - if (err < 0) - return err; - /* +16 degrees offset for temp2 for the LM99 */ - if (data->kind == lm99 && attr->index == 3) + if (data->kind == lm99 && index == 3) val -= 16000; mutex_lock(&data->update_lock); if (data->kind == adt7461 || data->kind == tmp451) - data->temp8[nr] = temp_to_u8_adt7461(data, val); + data->temp8[index] = temp_to_u8_adt7461(data, val); else if (data->kind == max6646) - data->temp8[nr] = temp_to_u8(val); + data->temp8[index] = temp_to_u8(val); else - data->temp8[nr] = temp_to_s8(val); - - lm90_select_remote_channel(client, data, nr >= 6); - i2c_smbus_write_byte_data(client, reg[nr], data->temp8[nr]); - lm90_select_remote_channel(client, data, 0); + data->temp8[index] = temp_to_s8(val); + if ((err = lm90_select_remote_channel(client, data, index >= 6)) || + (err = i2c_smbus_write_byte_data(client, reg[index], + data->temp8[index])) || + (err = lm90_select_remote_channel(client, data, 0))) + dev_err(dev, "write_temp8 failed, err %d\n", err); mutex_unlock(&data->update_lock); + + return err; +} + +static ssize_t set_temp8(struct device *dev, struct device_attribute *devattr, + const char *buf, size_t count) +{ + struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr); + int index = attr->index; + long val; + int err; + + err = kstrtol(buf, 10, &val); + if (err < 0) + return err; + + err = write_temp8(dev, index, val); + if (err < 0) + return err; + return count; } -static ssize_t show_temp11(struct device *dev, struct device_attribute *devattr, - char *buf) +static int read_temp11(struct device *dev, int index) { - struct sensor_device_attribute_2 *attr = to_sensor_dev_attr_2(devattr); struct lm90_data *data = lm90_update_device(dev); int temp; if (data->kind == adt7461 || data->kind == tmp451) - temp = temp_from_u16_adt7461(data, data->temp11[attr->index]); + temp = temp_from_u16_adt7461(data, data->temp11[index]); else if (data->kind == max6646) - temp = temp_from_u16(data->temp11[attr->index]); + temp = temp_from_u16(data->temp11[index]); else - temp = temp_from_s16(data->temp11[attr->index]); + temp = temp_from_s16(data->temp11[index]); /* +16 degrees offset for temp2 for the LM99 */ - if (data->kind == lm99 && attr->index <= 2) + if (data->kind == lm99 && index <= 2) temp += 16000; - return sprintf(buf, "%d\n", temp); + return temp; } -static ssize_t set_temp11(struct device *dev, struct device_attribute *devattr, - const char *buf, size_t count) +static ssize_t show_temp11(struct device *dev, struct device_attribute *devattr, + char *buf) +{ + struct sensor_device_attribute_2 *attr = to_sensor_dev_attr_2(devattr); + + return sprintf(buf, "%d\n", read_temp11(dev, attr->index)); +} + +static int write_temp11(struct device *dev, int nr, int index, long val) { struct { u8 high; @@ -861,18 +888,10 @@ static ssize_t set_temp11(struct device *dev, struct device_attribute *devattr, { LM90_REG_W_REMOTE_HIGHH, LM90_REG_W_REMOTE_HIGHL, 1 } }; - struct sensor_device_attribute_2 *attr = to_sensor_dev_attr_2(devattr); struct lm90_data *data = dev_get_drvdata(dev); struct i2c_client *client = data->client; - int nr = attr->nr; - int index = attr->index; - long val; int err; - err = kstrtol(buf, 10, &val); - if (err < 0) - return err; - /* +16 degrees offset for temp2 for the LM99 */ if (data->kind == lm99 && index <= 2) val -= 16000; @@ -887,15 +906,50 @@ static ssize_t set_temp11(struct device *dev, struct device_attribute *devattr, else data->temp11[index] = temp_to_s8(val) << 8; - lm90_select_remote_channel(client, data, reg[nr].channel); - i2c_smbus_write_byte_data(client, reg[nr].high, - data->temp11[index] >> 8); - if (data->flags & LM90_HAVE_REM_LIMIT_EXT) - i2c_smbus_write_byte_data(client, reg[nr].low, - data->temp11[index] & 0xff); - lm90_select_remote_channel(client, data, 0); + err = lm90_select_remote_channel(client, data, reg[nr].channel); + if (err) + goto error; + + err = i2c_smbus_write_byte_data(client, reg[nr].high, + data->temp11[index] >> 8); + if (err) + goto error; + + if (data->flags & LM90_HAVE_REM_LIMIT_EXT) { + err = i2c_smbus_write_byte_data(client, reg[nr].low, + data->temp11[index] & 0xff); + if (err) + goto error; + } + + err = lm90_select_remote_channel(client, data, 0); + +error: + if (err) + dev_err(dev, "write_temp11 failed, err %d\n", err); mutex_unlock(&data->update_lock); + + return err; +} + +static ssize_t set_temp11(struct device *dev, struct device_attribute *devattr, + const char *buf, size_t count) +{ + struct sensor_device_attribute_2 *attr = to_sensor_dev_attr_2(devattr); + int nr = attr->nr; + int index = attr->index; + long val; + int err; + + err = kstrtol(buf, 10, &val); + if (err < 0) + return err; + + err = write_temp11(dev, nr, index, val); + if (err < 0) + return err; + return count; } -- 1.8.1.5 ^ permalink raw reply related [flat|nested] 48+ messages in thread
* Re: [PATCH v3 1/4] hwmon: (lm90) split set&show temp as common codes 2014-08-25 6:29 ` [PATCH v3 1/4] hwmon: (lm90) split set&show temp as common codes Wei Ni @ 2014-08-25 12:23 ` Mikko Perttunen 2014-08-26 2:27 ` Wei Ni 0 siblings, 1 reply; 48+ messages in thread From: Mikko Perttunen @ 2014-08-25 12:23 UTC (permalink / raw) To: Wei Ni, edubezval, khali, linux, swarren Cc: lm-sensors, linux-tegra, linux-kernel FWIW, please fix the authorship information for next version. Cheers, Mikko On 25/08/14 09:29, Wei Ni wrote: > From: lightning314 <wni@nvidia.com> > > Split set&show temp codes as common functions, so we can use it > directly when implement linux thermal framework. > And handle error return value for the lm90_select_remote_channel > and write_tempx, then set_temp8 and set_temp11 could return it > to user-space. > > Signed-off-by: Wei Ni <wni@nvidia.com> > Signed-off-by: Jean Delvare <khali@linux-fr.org> > --- > drivers/hwmon/lm90.c | 164 ++++++++++++++++++++++++++++++++++----------------- > 1 file changed, 109 insertions(+), 55 deletions(-) > > diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c > index c9ff08d..cb33dcf 100644 > --- a/drivers/hwmon/lm90.c > +++ b/drivers/hwmon/lm90.c > @@ -473,20 +473,23 @@ static int lm90_read16(struct i2c_client *client, u8 regh, u8 regl, u16 *value) > * various registers have different meanings as a result of selecting a > * non-default remote channel. > */ > -static inline void lm90_select_remote_channel(struct i2c_client *client, > - struct lm90_data *data, > - int channel) > +static inline int lm90_select_remote_channel(struct i2c_client *client, > + struct lm90_data *data, > + int channel) > { > u8 config; > + int err = 0; > > if (data->kind == max6696) { > lm90_read_reg(client, LM90_REG_R_CONFIG1, &config); > config &= ~0x08; > if (channel) > config |= 0x08; > - i2c_smbus_write_byte_data(client, LM90_REG_W_CONFIG1, > - config); > + err = i2c_smbus_write_byte_data(client, LM90_REG_W_CONFIG1, > + config); > } > + > + return err; > } > > /* > @@ -759,29 +762,34 @@ static u16 temp_to_u16_adt7461(struct lm90_data *data, long val) > * Sysfs stuff > */ > > -static ssize_t show_temp8(struct device *dev, struct device_attribute *devattr, > - char *buf) > +static int read_temp8(struct device *dev, int index) > { > - struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr); > struct lm90_data *data = lm90_update_device(dev); > int temp; > > if (data->kind == adt7461 || data->kind == tmp451) > - temp = temp_from_u8_adt7461(data, data->temp8[attr->index]); > + temp = temp_from_u8_adt7461(data, data->temp8[index]); > else if (data->kind == max6646) > - temp = temp_from_u8(data->temp8[attr->index]); > + temp = temp_from_u8(data->temp8[index]); > else > - temp = temp_from_s8(data->temp8[attr->index]); > + temp = temp_from_s8(data->temp8[index]); > > /* +16 degrees offset for temp2 for the LM99 */ > - if (data->kind == lm99 && attr->index == 3) > + if (data->kind == lm99 && index == 3) > temp += 16000; > > - return sprintf(buf, "%d\n", temp); > + return temp; > } > > -static ssize_t set_temp8(struct device *dev, struct device_attribute *devattr, > - const char *buf, size_t count) > +static ssize_t show_temp8(struct device *dev, struct device_attribute *devattr, > + char *buf) > +{ > + struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr); > + > + return sprintf(buf, "%d\n", read_temp8(dev, attr->index)); > +} > + > +static int write_temp8(struct device *dev, int index, long val) > { > static const u8 reg[TEMP8_REG_NUM] = { > LM90_REG_W_LOCAL_LOW, > @@ -794,60 +802,79 @@ static ssize_t set_temp8(struct device *dev, struct device_attribute *devattr, > MAX6659_REG_W_REMOTE_EMERG, > }; > > - struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr); > struct lm90_data *data = dev_get_drvdata(dev); > struct i2c_client *client = data->client; > - int nr = attr->index; > - long val; > int err; > > - err = kstrtol(buf, 10, &val); > - if (err < 0) > - return err; > - > /* +16 degrees offset for temp2 for the LM99 */ > - if (data->kind == lm99 && attr->index == 3) > + if (data->kind == lm99 && index == 3) > val -= 16000; > > mutex_lock(&data->update_lock); > if (data->kind == adt7461 || data->kind == tmp451) > - data->temp8[nr] = temp_to_u8_adt7461(data, val); > + data->temp8[index] = temp_to_u8_adt7461(data, val); > else if (data->kind == max6646) > - data->temp8[nr] = temp_to_u8(val); > + data->temp8[index] = temp_to_u8(val); > else > - data->temp8[nr] = temp_to_s8(val); > - > - lm90_select_remote_channel(client, data, nr >= 6); > - i2c_smbus_write_byte_data(client, reg[nr], data->temp8[nr]); > - lm90_select_remote_channel(client, data, 0); > + data->temp8[index] = temp_to_s8(val); > > + if ((err = lm90_select_remote_channel(client, data, index >= 6)) || > + (err = i2c_smbus_write_byte_data(client, reg[index], > + data->temp8[index])) || > + (err = lm90_select_remote_channel(client, data, 0))) > + dev_err(dev, "write_temp8 failed, err %d\n", err); > mutex_unlock(&data->update_lock); > + > + return err; > +} > + > +static ssize_t set_temp8(struct device *dev, struct device_attribute *devattr, > + const char *buf, size_t count) > +{ > + struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr); > + int index = attr->index; > + long val; > + int err; > + > + err = kstrtol(buf, 10, &val); > + if (err < 0) > + return err; > + > + err = write_temp8(dev, index, val); > + if (err < 0) > + return err; > + > return count; > } > > -static ssize_t show_temp11(struct device *dev, struct device_attribute *devattr, > - char *buf) > +static int read_temp11(struct device *dev, int index) > { > - struct sensor_device_attribute_2 *attr = to_sensor_dev_attr_2(devattr); > struct lm90_data *data = lm90_update_device(dev); > int temp; > > if (data->kind == adt7461 || data->kind == tmp451) > - temp = temp_from_u16_adt7461(data, data->temp11[attr->index]); > + temp = temp_from_u16_adt7461(data, data->temp11[index]); > else if (data->kind == max6646) > - temp = temp_from_u16(data->temp11[attr->index]); > + temp = temp_from_u16(data->temp11[index]); > else > - temp = temp_from_s16(data->temp11[attr->index]); > + temp = temp_from_s16(data->temp11[index]); > > /* +16 degrees offset for temp2 for the LM99 */ > - if (data->kind == lm99 && attr->index <= 2) > + if (data->kind == lm99 && index <= 2) > temp += 16000; > > - return sprintf(buf, "%d\n", temp); > + return temp; > } > > -static ssize_t set_temp11(struct device *dev, struct device_attribute *devattr, > - const char *buf, size_t count) > +static ssize_t show_temp11(struct device *dev, struct device_attribute *devattr, > + char *buf) > +{ > + struct sensor_device_attribute_2 *attr = to_sensor_dev_attr_2(devattr); > + > + return sprintf(buf, "%d\n", read_temp11(dev, attr->index)); > +} > + > +static int write_temp11(struct device *dev, int nr, int index, long val) > { > struct { > u8 high; > @@ -861,18 +888,10 @@ static ssize_t set_temp11(struct device *dev, struct device_attribute *devattr, > { LM90_REG_W_REMOTE_HIGHH, LM90_REG_W_REMOTE_HIGHL, 1 } > }; > > - struct sensor_device_attribute_2 *attr = to_sensor_dev_attr_2(devattr); > struct lm90_data *data = dev_get_drvdata(dev); > struct i2c_client *client = data->client; > - int nr = attr->nr; > - int index = attr->index; > - long val; > int err; > > - err = kstrtol(buf, 10, &val); > - if (err < 0) > - return err; > - > /* +16 degrees offset for temp2 for the LM99 */ > if (data->kind == lm99 && index <= 2) > val -= 16000; > @@ -887,15 +906,50 @@ static ssize_t set_temp11(struct device *dev, struct device_attribute *devattr, > else > data->temp11[index] = temp_to_s8(val) << 8; > > - lm90_select_remote_channel(client, data, reg[nr].channel); > - i2c_smbus_write_byte_data(client, reg[nr].high, > - data->temp11[index] >> 8); > - if (data->flags & LM90_HAVE_REM_LIMIT_EXT) > - i2c_smbus_write_byte_data(client, reg[nr].low, > - data->temp11[index] & 0xff); > - lm90_select_remote_channel(client, data, 0); > + err = lm90_select_remote_channel(client, data, reg[nr].channel); > + if (err) > + goto error; > + > + err = i2c_smbus_write_byte_data(client, reg[nr].high, > + data->temp11[index] >> 8); > + if (err) > + goto error; > + > + if (data->flags & LM90_HAVE_REM_LIMIT_EXT) { > + err = i2c_smbus_write_byte_data(client, reg[nr].low, > + data->temp11[index] & 0xff); > + if (err) > + goto error; > + } > + > + err = lm90_select_remote_channel(client, data, 0); > + > +error: > + if (err) > + dev_err(dev, "write_temp11 failed, err %d\n", err); > > mutex_unlock(&data->update_lock); > + > + return err; > +} > + > +static ssize_t set_temp11(struct device *dev, struct device_attribute *devattr, > + const char *buf, size_t count) > +{ > + struct sensor_device_attribute_2 *attr = to_sensor_dev_attr_2(devattr); > + int nr = attr->nr; > + int index = attr->index; > + long val; > + int err; > + > + err = kstrtol(buf, 10, &val); > + if (err < 0) > + return err; > + > + err = write_temp11(dev, nr, index, val); > + if (err < 0) > + return err; > + > return count; > } > > ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v3 1/4] hwmon: (lm90) split set&show temp as common codes 2014-08-25 12:23 ` Mikko Perttunen @ 2014-08-26 2:27 ` Wei Ni 2014-08-26 12:17 ` Eduardo Valentin 0 siblings, 1 reply; 48+ messages in thread From: Wei Ni @ 2014-08-26 2:27 UTC (permalink / raw) To: Mikko Perttunen, edubezval, khali, linux, swarren Cc: lm-sensors, linux-tegra, linux-kernel On 08/25/2014 08:23 PM, Mikko Perttunen wrote: > FWIW, please fix the authorship information for next version. Sorry, I didn't get your point, did you mean I should remove the line "From: lightning314 <wni@nvidia.com>" ? Yes I made a mistake on it, I will remove it in next version. Thanks. Wei. > > Cheers, > Mikko > > On 25/08/14 09:29, Wei Ni wrote: >> From: lightning314 <wni@nvidia.com> >> >> Split set&show temp codes as common functions, so we can use it >> directly when implement linux thermal framework. >> And handle error return value for the lm90_select_remote_channel >> and write_tempx, then set_temp8 and set_temp11 could return it >> to user-space. >> >> Signed-off-by: Wei Ni <wni@nvidia.com> >> Signed-off-by: Jean Delvare <khali@linux-fr.org> >> --- >> drivers/hwmon/lm90.c | 164 >> ++++++++++++++++++++++++++++++++++----------------- >> 1 file changed, 109 insertions(+), 55 deletions(-) >> >> diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c >> index c9ff08d..cb33dcf 100644 >> --- a/drivers/hwmon/lm90.c >> +++ b/drivers/hwmon/lm90.c >> @@ -473,20 +473,23 @@ static int lm90_read16(struct i2c_client >> *client, u8 regh, u8 regl, u16 *value) >> * various registers have different meanings as a result of selecting a >> * non-default remote channel. >> */ >> -static inline void lm90_select_remote_channel(struct i2c_client *client, >> - struct lm90_data *data, >> - int channel) >> +static inline int lm90_select_remote_channel(struct i2c_client *client, >> + struct lm90_data *data, >> + int channel) >> { >> u8 config; >> + int err = 0; >> >> if (data->kind == max6696) { >> lm90_read_reg(client, LM90_REG_R_CONFIG1, &config); >> config &= ~0x08; >> if (channel) >> config |= 0x08; >> - i2c_smbus_write_byte_data(client, LM90_REG_W_CONFIG1, >> - config); >> + err = i2c_smbus_write_byte_data(client, LM90_REG_W_CONFIG1, >> + config); >> } >> + >> + return err; >> } >> >> /* >> @@ -759,29 +762,34 @@ static u16 temp_to_u16_adt7461(struct lm90_data >> *data, long val) >> * Sysfs stuff >> */ >> >> -static ssize_t show_temp8(struct device *dev, struct device_attribute >> *devattr, >> - char *buf) >> +static int read_temp8(struct device *dev, int index) >> { >> - struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr); >> struct lm90_data *data = lm90_update_device(dev); >> int temp; >> >> if (data->kind == adt7461 || data->kind == tmp451) >> - temp = temp_from_u8_adt7461(data, data->temp8[attr->index]); >> + temp = temp_from_u8_adt7461(data, data->temp8[index]); >> else if (data->kind == max6646) >> - temp = temp_from_u8(data->temp8[attr->index]); >> + temp = temp_from_u8(data->temp8[index]); >> else >> - temp = temp_from_s8(data->temp8[attr->index]); >> + temp = temp_from_s8(data->temp8[index]); >> >> /* +16 degrees offset for temp2 for the LM99 */ >> - if (data->kind == lm99 && attr->index == 3) >> + if (data->kind == lm99 && index == 3) >> temp += 16000; >> >> - return sprintf(buf, "%d\n", temp); >> + return temp; >> } >> >> -static ssize_t set_temp8(struct device *dev, struct device_attribute >> *devattr, >> - const char *buf, size_t count) >> +static ssize_t show_temp8(struct device *dev, struct device_attribute >> *devattr, >> + char *buf) >> +{ >> + struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr); >> + >> + return sprintf(buf, "%d\n", read_temp8(dev, attr->index)); >> +} >> + >> +static int write_temp8(struct device *dev, int index, long val) >> { >> static const u8 reg[TEMP8_REG_NUM] = { >> LM90_REG_W_LOCAL_LOW, >> @@ -794,60 +802,79 @@ static ssize_t set_temp8(struct device *dev, >> struct device_attribute *devattr, >> MAX6659_REG_W_REMOTE_EMERG, >> }; >> >> - struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr); >> struct lm90_data *data = dev_get_drvdata(dev); >> struct i2c_client *client = data->client; >> - int nr = attr->index; >> - long val; >> int err; >> >> - err = kstrtol(buf, 10, &val); >> - if (err < 0) >> - return err; >> - >> /* +16 degrees offset for temp2 for the LM99 */ >> - if (data->kind == lm99 && attr->index == 3) >> + if (data->kind == lm99 && index == 3) >> val -= 16000; >> >> mutex_lock(&data->update_lock); >> if (data->kind == adt7461 || data->kind == tmp451) >> - data->temp8[nr] = temp_to_u8_adt7461(data, val); >> + data->temp8[index] = temp_to_u8_adt7461(data, val); >> else if (data->kind == max6646) >> - data->temp8[nr] = temp_to_u8(val); >> + data->temp8[index] = temp_to_u8(val); >> else >> - data->temp8[nr] = temp_to_s8(val); >> - >> - lm90_select_remote_channel(client, data, nr >= 6); >> - i2c_smbus_write_byte_data(client, reg[nr], data->temp8[nr]); >> - lm90_select_remote_channel(client, data, 0); >> + data->temp8[index] = temp_to_s8(val); >> >> + if ((err = lm90_select_remote_channel(client, data, index >= 6)) || >> + (err = i2c_smbus_write_byte_data(client, reg[index], >> + data->temp8[index])) || >> + (err = lm90_select_remote_channel(client, data, 0))) >> + dev_err(dev, "write_temp8 failed, err %d\n", err); >> mutex_unlock(&data->update_lock); >> + >> + return err; >> +} >> + >> +static ssize_t set_temp8(struct device *dev, struct device_attribute >> *devattr, >> + const char *buf, size_t count) >> +{ >> + struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr); >> + int index = attr->index; >> + long val; >> + int err; >> + >> + err = kstrtol(buf, 10, &val); >> + if (err < 0) >> + return err; >> + >> + err = write_temp8(dev, index, val); >> + if (err < 0) >> + return err; >> + >> return count; >> } >> >> -static ssize_t show_temp11(struct device *dev, struct >> device_attribute *devattr, >> - char *buf) >> +static int read_temp11(struct device *dev, int index) >> { >> - struct sensor_device_attribute_2 *attr = >> to_sensor_dev_attr_2(devattr); >> struct lm90_data *data = lm90_update_device(dev); >> int temp; >> >> if (data->kind == adt7461 || data->kind == tmp451) >> - temp = temp_from_u16_adt7461(data, data->temp11[attr->index]); >> + temp = temp_from_u16_adt7461(data, data->temp11[index]); >> else if (data->kind == max6646) >> - temp = temp_from_u16(data->temp11[attr->index]); >> + temp = temp_from_u16(data->temp11[index]); >> else >> - temp = temp_from_s16(data->temp11[attr->index]); >> + temp = temp_from_s16(data->temp11[index]); >> >> /* +16 degrees offset for temp2 for the LM99 */ >> - if (data->kind == lm99 && attr->index <= 2) >> + if (data->kind == lm99 && index <= 2) >> temp += 16000; >> >> - return sprintf(buf, "%d\n", temp); >> + return temp; >> } >> >> -static ssize_t set_temp11(struct device *dev, struct device_attribute >> *devattr, >> - const char *buf, size_t count) >> +static ssize_t show_temp11(struct device *dev, struct >> device_attribute *devattr, >> + char *buf) >> +{ >> + struct sensor_device_attribute_2 *attr = >> to_sensor_dev_attr_2(devattr); >> + >> + return sprintf(buf, "%d\n", read_temp11(dev, attr->index)); >> +} >> + >> +static int write_temp11(struct device *dev, int nr, int index, long val) >> { >> struct { >> u8 high; >> @@ -861,18 +888,10 @@ static ssize_t set_temp11(struct device *dev, >> struct device_attribute *devattr, >> { LM90_REG_W_REMOTE_HIGHH, LM90_REG_W_REMOTE_HIGHL, 1 } >> }; >> >> - struct sensor_device_attribute_2 *attr = >> to_sensor_dev_attr_2(devattr); >> struct lm90_data *data = dev_get_drvdata(dev); >> struct i2c_client *client = data->client; >> - int nr = attr->nr; >> - int index = attr->index; >> - long val; >> int err; >> >> - err = kstrtol(buf, 10, &val); >> - if (err < 0) >> - return err; >> - >> /* +16 degrees offset for temp2 for the LM99 */ >> if (data->kind == lm99 && index <= 2) >> val -= 16000; >> @@ -887,15 +906,50 @@ static ssize_t set_temp11(struct device *dev, >> struct device_attribute *devattr, >> else >> data->temp11[index] = temp_to_s8(val) << 8; >> >> - lm90_select_remote_channel(client, data, reg[nr].channel); >> - i2c_smbus_write_byte_data(client, reg[nr].high, >> - data->temp11[index] >> 8); >> - if (data->flags & LM90_HAVE_REM_LIMIT_EXT) >> - i2c_smbus_write_byte_data(client, reg[nr].low, >> - data->temp11[index] & 0xff); >> - lm90_select_remote_channel(client, data, 0); >> + err = lm90_select_remote_channel(client, data, reg[nr].channel); >> + if (err) >> + goto error; >> + >> + err = i2c_smbus_write_byte_data(client, reg[nr].high, >> + data->temp11[index] >> 8); >> + if (err) >> + goto error; >> + >> + if (data->flags & LM90_HAVE_REM_LIMIT_EXT) { >> + err = i2c_smbus_write_byte_data(client, reg[nr].low, >> + data->temp11[index] & 0xff); >> + if (err) >> + goto error; >> + } >> + >> + err = lm90_select_remote_channel(client, data, 0); >> + >> +error: >> + if (err) >> + dev_err(dev, "write_temp11 failed, err %d\n", err); >> >> mutex_unlock(&data->update_lock); >> + >> + return err; >> +} >> + >> +static ssize_t set_temp11(struct device *dev, struct device_attribute >> *devattr, >> + const char *buf, size_t count) >> +{ >> + struct sensor_device_attribute_2 *attr = >> to_sensor_dev_attr_2(devattr); >> + int nr = attr->nr; >> + int index = attr->index; >> + long val; >> + int err; >> + >> + err = kstrtol(buf, 10, &val); >> + if (err < 0) >> + return err; >> + >> + err = write_temp11(dev, nr, index, val); >> + if (err < 0) >> + return err; >> + >> return count; >> } >> >> ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v3 1/4] hwmon: (lm90) split set&show temp as common codes 2014-08-26 2:27 ` Wei Ni @ 2014-08-26 12:17 ` Eduardo Valentin 2014-08-26 16:37 ` Stephen Warren 0 siblings, 1 reply; 48+ messages in thread From: Eduardo Valentin @ 2014-08-26 12:17 UTC (permalink / raw) To: Wei Ni Cc: Mikko Perttunen, khali, linux, swarren, lm-sensors, linux-tegra, linux-kernel On Tue, Aug 26, 2014 at 10:27:43AM +0800, Wei Ni wrote: > On 08/25/2014 08:23 PM, Mikko Perttunen wrote: > > FWIW, please fix the authorship information for next version. > > Sorry, I didn't get your point, did you mean I should remove the line > "From: lightning314 <wni@nvidia.com>" ? Yes I made a mistake on it, I > will remove it in next version. No Wei, don't remove the author line. Well, based on email, it is your patch, so, removing may satisfy. But the point is you should use meaninful names in tags like From and Signed off by. "lightning314" sounds quite cryptic or even a nick name. You must use real names there, such as "Wei Ni". > > Thanks. > Wei. > > > > > Cheers, > > Mikko > > > > On 25/08/14 09:29, Wei Ni wrote: > >> From: lightning314 <wni@nvidia.com> > >> > >> Split set&show temp codes as common functions, so we can use it > >> directly when implement linux thermal framework. > >> And handle error return value for the lm90_select_remote_channel > >> and write_tempx, then set_temp8 and set_temp11 could return it > >> to user-space. > >> > >> Signed-off-by: Wei Ni <wni@nvidia.com> > >> Signed-off-by: Jean Delvare <khali@linux-fr.org> > >> --- > >> drivers/hwmon/lm90.c | 164 > >> ++++++++++++++++++++++++++++++++++----------------- > >> 1 file changed, 109 insertions(+), 55 deletions(-) > >> > >> diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c > >> index c9ff08d..cb33dcf 100644 > >> --- a/drivers/hwmon/lm90.c > >> +++ b/drivers/hwmon/lm90.c > >> @@ -473,20 +473,23 @@ static int lm90_read16(struct i2c_client > >> *client, u8 regh, u8 regl, u16 *value) > >> * various registers have different meanings as a result of selecting a > >> * non-default remote channel. > >> */ > >> -static inline void lm90_select_remote_channel(struct i2c_client *client, > >> - struct lm90_data *data, > >> - int channel) > >> +static inline int lm90_select_remote_channel(struct i2c_client *client, > >> + struct lm90_data *data, > >> + int channel) > >> { > >> u8 config; > >> + int err = 0; > >> > >> if (data->kind == max6696) { > >> lm90_read_reg(client, LM90_REG_R_CONFIG1, &config); > >> config &= ~0x08; > >> if (channel) > >> config |= 0x08; > >> - i2c_smbus_write_byte_data(client, LM90_REG_W_CONFIG1, > >> - config); > >> + err = i2c_smbus_write_byte_data(client, LM90_REG_W_CONFIG1, > >> + config); > >> } > >> + > >> + return err; > >> } > >> > >> /* > >> @@ -759,29 +762,34 @@ static u16 temp_to_u16_adt7461(struct lm90_data > >> *data, long val) > >> * Sysfs stuff > >> */ > >> > >> -static ssize_t show_temp8(struct device *dev, struct device_attribute > >> *devattr, > >> - char *buf) > >> +static int read_temp8(struct device *dev, int index) > >> { > >> - struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr); > >> struct lm90_data *data = lm90_update_device(dev); > >> int temp; > >> > >> if (data->kind == adt7461 || data->kind == tmp451) > >> - temp = temp_from_u8_adt7461(data, data->temp8[attr->index]); > >> + temp = temp_from_u8_adt7461(data, data->temp8[index]); > >> else if (data->kind == max6646) > >> - temp = temp_from_u8(data->temp8[attr->index]); > >> + temp = temp_from_u8(data->temp8[index]); > >> else > >> - temp = temp_from_s8(data->temp8[attr->index]); > >> + temp = temp_from_s8(data->temp8[index]); > >> > >> /* +16 degrees offset for temp2 for the LM99 */ > >> - if (data->kind == lm99 && attr->index == 3) > >> + if (data->kind == lm99 && index == 3) > >> temp += 16000; > >> > >> - return sprintf(buf, "%d\n", temp); > >> + return temp; > >> } > >> > >> -static ssize_t set_temp8(struct device *dev, struct device_attribute > >> *devattr, > >> - const char *buf, size_t count) > >> +static ssize_t show_temp8(struct device *dev, struct device_attribute > >> *devattr, > >> + char *buf) > >> +{ > >> + struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr); > >> + > >> + return sprintf(buf, "%d\n", read_temp8(dev, attr->index)); > >> +} > >> + > >> +static int write_temp8(struct device *dev, int index, long val) > >> { > >> static const u8 reg[TEMP8_REG_NUM] = { > >> LM90_REG_W_LOCAL_LOW, > >> @@ -794,60 +802,79 @@ static ssize_t set_temp8(struct device *dev, > >> struct device_attribute *devattr, > >> MAX6659_REG_W_REMOTE_EMERG, > >> }; > >> > >> - struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr); > >> struct lm90_data *data = dev_get_drvdata(dev); > >> struct i2c_client *client = data->client; > >> - int nr = attr->index; > >> - long val; > >> int err; > >> > >> - err = kstrtol(buf, 10, &val); > >> - if (err < 0) > >> - return err; > >> - > >> /* +16 degrees offset for temp2 for the LM99 */ > >> - if (data->kind == lm99 && attr->index == 3) > >> + if (data->kind == lm99 && index == 3) > >> val -= 16000; > >> > >> mutex_lock(&data->update_lock); > >> if (data->kind == adt7461 || data->kind == tmp451) > >> - data->temp8[nr] = temp_to_u8_adt7461(data, val); > >> + data->temp8[index] = temp_to_u8_adt7461(data, val); > >> else if (data->kind == max6646) > >> - data->temp8[nr] = temp_to_u8(val); > >> + data->temp8[index] = temp_to_u8(val); > >> else > >> - data->temp8[nr] = temp_to_s8(val); > >> - > >> - lm90_select_remote_channel(client, data, nr >= 6); > >> - i2c_smbus_write_byte_data(client, reg[nr], data->temp8[nr]); > >> - lm90_select_remote_channel(client, data, 0); > >> + data->temp8[index] = temp_to_s8(val); > >> > >> + if ((err = lm90_select_remote_channel(client, data, index >= 6)) || > >> + (err = i2c_smbus_write_byte_data(client, reg[index], > >> + data->temp8[index])) || > >> + (err = lm90_select_remote_channel(client, data, 0))) > >> + dev_err(dev, "write_temp8 failed, err %d\n", err); > >> mutex_unlock(&data->update_lock); > >> + > >> + return err; > >> +} > >> + > >> +static ssize_t set_temp8(struct device *dev, struct device_attribute > >> *devattr, > >> + const char *buf, size_t count) > >> +{ > >> + struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr); > >> + int index = attr->index; > >> + long val; > >> + int err; > >> + > >> + err = kstrtol(buf, 10, &val); > >> + if (err < 0) > >> + return err; > >> + > >> + err = write_temp8(dev, index, val); > >> + if (err < 0) > >> + return err; > >> + > >> return count; > >> } > >> > >> -static ssize_t show_temp11(struct device *dev, struct > >> device_attribute *devattr, > >> - char *buf) > >> +static int read_temp11(struct device *dev, int index) > >> { > >> - struct sensor_device_attribute_2 *attr = > >> to_sensor_dev_attr_2(devattr); > >> struct lm90_data *data = lm90_update_device(dev); > >> int temp; > >> > >> if (data->kind == adt7461 || data->kind == tmp451) > >> - temp = temp_from_u16_adt7461(data, data->temp11[attr->index]); > >> + temp = temp_from_u16_adt7461(data, data->temp11[index]); > >> else if (data->kind == max6646) > >> - temp = temp_from_u16(data->temp11[attr->index]); > >> + temp = temp_from_u16(data->temp11[index]); > >> else > >> - temp = temp_from_s16(data->temp11[attr->index]); > >> + temp = temp_from_s16(data->temp11[index]); > >> > >> /* +16 degrees offset for temp2 for the LM99 */ > >> - if (data->kind == lm99 && attr->index <= 2) > >> + if (data->kind == lm99 && index <= 2) > >> temp += 16000; > >> > >> - return sprintf(buf, "%d\n", temp); > >> + return temp; > >> } > >> > >> -static ssize_t set_temp11(struct device *dev, struct device_attribute > >> *devattr, > >> - const char *buf, size_t count) > >> +static ssize_t show_temp11(struct device *dev, struct > >> device_attribute *devattr, > >> + char *buf) > >> +{ > >> + struct sensor_device_attribute_2 *attr = > >> to_sensor_dev_attr_2(devattr); > >> + > >> + return sprintf(buf, "%d\n", read_temp11(dev, attr->index)); > >> +} > >> + > >> +static int write_temp11(struct device *dev, int nr, int index, long val) > >> { > >> struct { > >> u8 high; > >> @@ -861,18 +888,10 @@ static ssize_t set_temp11(struct device *dev, > >> struct device_attribute *devattr, > >> { LM90_REG_W_REMOTE_HIGHH, LM90_REG_W_REMOTE_HIGHL, 1 } > >> }; > >> > >> - struct sensor_device_attribute_2 *attr = > >> to_sensor_dev_attr_2(devattr); > >> struct lm90_data *data = dev_get_drvdata(dev); > >> struct i2c_client *client = data->client; > >> - int nr = attr->nr; > >> - int index = attr->index; > >> - long val; > >> int err; > >> > >> - err = kstrtol(buf, 10, &val); > >> - if (err < 0) > >> - return err; > >> - > >> /* +16 degrees offset for temp2 for the LM99 */ > >> if (data->kind == lm99 && index <= 2) > >> val -= 16000; > >> @@ -887,15 +906,50 @@ static ssize_t set_temp11(struct device *dev, > >> struct device_attribute *devattr, > >> else > >> data->temp11[index] = temp_to_s8(val) << 8; > >> > >> - lm90_select_remote_channel(client, data, reg[nr].channel); > >> - i2c_smbus_write_byte_data(client, reg[nr].high, > >> - data->temp11[index] >> 8); > >> - if (data->flags & LM90_HAVE_REM_LIMIT_EXT) > >> - i2c_smbus_write_byte_data(client, reg[nr].low, > >> - data->temp11[index] & 0xff); > >> - lm90_select_remote_channel(client, data, 0); > >> + err = lm90_select_remote_channel(client, data, reg[nr].channel); > >> + if (err) > >> + goto error; > >> + > >> + err = i2c_smbus_write_byte_data(client, reg[nr].high, > >> + data->temp11[index] >> 8); > >> + if (err) > >> + goto error; > >> + > >> + if (data->flags & LM90_HAVE_REM_LIMIT_EXT) { > >> + err = i2c_smbus_write_byte_data(client, reg[nr].low, > >> + data->temp11[index] & 0xff); > >> + if (err) > >> + goto error; > >> + } > >> + > >> + err = lm90_select_remote_channel(client, data, 0); > >> + > >> +error: > >> + if (err) > >> + dev_err(dev, "write_temp11 failed, err %d\n", err); > >> > >> mutex_unlock(&data->update_lock); > >> + > >> + return err; > >> +} > >> + > >> +static ssize_t set_temp11(struct device *dev, struct device_attribute > >> *devattr, > >> + const char *buf, size_t count) > >> +{ > >> + struct sensor_device_attribute_2 *attr = > >> to_sensor_dev_attr_2(devattr); > >> + int nr = attr->nr; > >> + int index = attr->index; > >> + long val; > >> + int err; > >> + > >> + err = kstrtol(buf, 10, &val); > >> + if (err < 0) > >> + return err; > >> + > >> + err = write_temp11(dev, nr, index, val); > >> + if (err < 0) > >> + return err; > >> + > >> return count; > >> } > >> > >> > ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v3 1/4] hwmon: (lm90) split set&show temp as common codes 2014-08-26 12:17 ` Eduardo Valentin @ 2014-08-26 16:37 ` Stephen Warren 2014-08-27 2:25 ` Wei Ni 0 siblings, 1 reply; 48+ messages in thread From: Stephen Warren @ 2014-08-26 16:37 UTC (permalink / raw) To: Eduardo Valentin, Wei Ni Cc: Mikko Perttunen, khali, linux, lm-sensors, linux-tegra, linux-kernel On 08/26/2014 06:17 AM, Eduardo Valentin wrote: > On Tue, Aug 26, 2014 at 10:27:43AM +0800, Wei Ni wrote: >> On 08/25/2014 08:23 PM, Mikko Perttunen wrote: >>> FWIW, please fix the authorship information for next version. >> >> Sorry, I didn't get your point, did you mean I should remove the line >> "From: lightning314 <wni@nvidia.com>" ? Yes I made a mistake on it, I >> will remove it in next version. > > No Wei, don't remove the author line. Well, based on email, it is your > patch, so, removing may satisfy. The patch authorship should be fixed to have the correct value; "lightning314" is not a valid real name. In turn, depending on your git email configuration, this might automatically remove that line from the message, since it might then exactly match the email's from address. ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v3 1/4] hwmon: (lm90) split set&show temp as common codes 2014-08-26 16:37 ` Stephen Warren @ 2014-08-27 2:25 ` Wei Ni 0 siblings, 0 replies; 48+ messages in thread From: Wei Ni @ 2014-08-27 2:25 UTC (permalink / raw) To: Stephen Warren, Eduardo Valentin Cc: Mikko Perttunen, khali, linux, lm-sensors, linux-tegra, linux-kernel On 08/27/2014 12:37 AM, Stephen Warren wrote: > On 08/26/2014 06:17 AM, Eduardo Valentin wrote: >> On Tue, Aug 26, 2014 at 10:27:43AM +0800, Wei Ni wrote: >>> On 08/25/2014 08:23 PM, Mikko Perttunen wrote: >>>> FWIW, please fix the authorship information for next version. >>> >>> Sorry, I didn't get your point, did you mean I should remove the line >>> "From: lightning314 <wni@nvidia.com>" ? Yes I made a mistake on it, I >>> will remove it in next version. >> >> No Wei, don't remove the author line. Well, based on email, it is your >> patch, so, removing may satisfy. > > The patch authorship should be fixed to have the correct value; > "lightning314" is not a valid real name. In turn, depending on your git > email configuration, this might automatically remove that line from the > message, since it might then exactly match the email's from address. Got it, thanks for the comments. I get this patch from the patchwork, so it use my username "lightning314". Anyway, I will fix it in next version. Wei. ^ permalink raw reply [flat|nested] 48+ messages in thread
end of thread, other threads:[~2014-08-27 2:26 UTC | newest] Thread overview: 48+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2013-07-12 7:48 [PATCH v3 0/4] Lm90 Enhancements Wei Ni 2013-07-12 7:48 ` [PATCH v3 1/4] hwmon: (lm90) split set&show temp as common codes Wei Ni 2013-07-12 13:26 ` Jean Delvare 2013-07-12 13:50 ` Guenter Roeck 2013-07-12 14:30 ` Jean Delvare 2013-07-12 14:40 ` Guenter Roeck 2013-07-15 6:25 ` Wei Ni 2013-07-15 7:24 ` Jean Delvare 2013-07-15 9:14 ` Wei Ni 2013-07-15 17:52 ` Jean Delvare 2013-07-17 4:26 ` Thierry Reding 2013-07-17 5:14 ` Guenter Roeck 2013-07-17 6:26 ` Wei Ni 2013-07-17 9:11 ` Jean Delvare 2013-07-17 9:54 ` Wei Ni 2013-07-15 6:05 ` Wei Ni 2013-07-15 7:29 ` Jean Delvare 2013-07-12 7:48 ` [PATCH v3 2/4] hwmon: (lm90) use macro defines for the status bit Wei Ni 2013-07-15 16:57 ` Jean Delvare 2013-07-15 17:33 ` Guenter Roeck 2013-10-30 15:33 ` Jean Delvare 2013-10-30 16:11 ` Guenter Roeck 2013-10-30 16:56 ` Guenter Roeck 2013-07-17 7:03 ` Wei Ni 2013-07-17 7:09 ` Wei Ni 2013-07-17 8:28 ` Jean Delvare 2013-07-17 9:29 ` Wei Ni 2013-07-17 9:46 ` Jean Delvare 2013-07-12 7:48 ` [PATCH v3 3/4] hwmon: (lm90) add support to handle IRQ Wei Ni 2013-07-18 15:58 ` Jean Delvare 2013-07-19 6:41 ` Wei Ni 2013-07-24 7:46 ` Wei Ni 2013-07-24 8:08 ` Jean Delvare 2013-07-27 15:02 ` Jean Delvare 2013-07-29 10:14 ` Wei Ni 2013-07-29 15:58 ` Jean Delvare 2013-07-30 8:18 ` Wei Ni 2013-09-16 12:34 ` Jean Delvare 2013-07-12 7:48 ` [PATCH v3 4/4] hwmon: (lm90) use enums for the indexes of temp8 and temp11 Wei Ni 2013-07-27 15:38 ` Jean Delvare 2013-07-29 11:15 ` Wei Ni 2013-07-29 15:48 ` Jean Delvare 2014-08-25 6:29 [PATCH v3 0/4] expose lm90 to thermal fw Wei Ni 2014-08-25 6:29 ` [PATCH v3 1/4] hwmon: (lm90) split set&show temp as common codes Wei Ni 2014-08-25 12:23 ` Mikko Perttunen 2014-08-26 2:27 ` Wei Ni 2014-08-26 12:17 ` Eduardo Valentin 2014-08-26 16:37 ` Stephen Warren 2014-08-27 2:25 ` Wei Ni
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).