| Message ID | 1361187031-3679-7-git-send-email-wni@nvidia.com |
|---|---|
| State | RFC, archived |
| Headers | show |
On Mon, Feb 18, 2013 at 07:30:28PM +0800, Wei Ni wrote: > Register the remote sensor to the thermal framework. > It can support to show the temperature and read/write threshold. > > Signed-off-by: Wei Ni <wni@nvidia.com> > --- > arch/arm/boot/dts/tegra30-cardhu.dtsi | 1 + > drivers/hwmon/lm90.c | 182 ++++++++++++++++++++++++++++++++- > 2 files changed, 182 insertions(+), 1 deletion(-) > > diff --git a/arch/arm/boot/dts/tegra30-cardhu.dtsi b/arch/arm/boot/dts/tegra30-cardhu.dtsi > index 15ad1ad..3f6ab89 100644 > --- a/arch/arm/boot/dts/tegra30-cardhu.dtsi > +++ b/arch/arm/boot/dts/tegra30-cardhu.dtsi > @@ -279,6 +279,7 @@ > reg = <0x4c>; > interrupt-parent = <&gpio>; > interrupts = <226 0x08>; /* gpio PCC2 */ > + #sensor-cells = <1>; > }; > }; > > diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c > index de5a476..0abdedc 100644 > --- a/drivers/hwmon/lm90.c > +++ b/drivers/hwmon/lm90.c > @@ -91,6 +91,7 @@ > #include <linux/sysfs.h> > #include <linux/interrupt.h> > #include <linux/of_irq.h> > +#include <linux/thermal.h> > > /* > * Addresses to scan > @@ -182,6 +183,15 @@ enum chips { lm90, adm1032, lm99, lm86, max6657, max6659, adt7461, max6680, > #define LM90_HAVE_BROKEN_ALERT (1 << 7) /* Broken alert */ > > /* > + * Thermal framework > + */ > +enum lm90_thresholds { > + LM90_LOW_THRESHOLDS = 0, /* threshold 0: low limits */ > + LM90_HIGH_THRESHOLDS, /* threshold 1: high limits */ > + LM90_NUM_THRESHOLDS > +}; > + > +/* > * Driver data (common to all clients) > */ > > @@ -377,6 +387,9 @@ struct lm90_data { > s16 temp11[TEMP11_REG_NUM]; > u8 temp_hyst; > u16 alarms; /* bitvector (upper 8 bits for max6695/96) */ > + > + struct thermal_sensor *ts_remote; > + struct thermal_sensor *ts_local; > }; > > /* > @@ -1493,12 +1506,151 @@ static irqreturn_t lm90_irq(int irq, void *dev_id) > return IRQ_HANDLED; > } > > +static int lm90_read_remote_temp(struct thermal_sensor *ts, long *temp) > +{ > + struct i2c_client *client = ts->devdata; > + struct device *dev = &client->dev; > + > + _show_temp11(dev, TEMP11_REMOTE_TEMP, (int *)temp); > + > + return 0; > +} > + > +static int lm90_read_remote_threshold(struct thermal_sensor *ts, int th_index, > + long *val) > +{ > + struct i2c_client *client = ts->devdata; > + struct device *dev = &client->dev; > + int index; > + > + switch (th_index) { > + case LM90_LOW_THRESHOLDS: > + /* remote low limit */ > + index = TEMP11_REMOTE_LOW; > + break; > + case LM90_HIGH_THRESHOLDS: > + /* remote high limit */ > + index = TEMP11_REMOTE_HIGH; > + break; > + default: > + dev_err(dev, "read remote threshold failed.\n"); > + return -EINVAL; > + } > + > + _show_temp11(dev, index, (int *)val); Typecasting a pointer like this doesn't work. Try this on a big-endian system with sizeof(int) != sizeof(long). Note that you would not have the problem if you would pass the value instead of the pointer. > + > + return 0; > +} > + > +static int lm90_write_remote_threshold(struct thermal_sensor *ts, int th_index, > + long val) > +{ > + struct i2c_client *client = ts->devdata; > + struct device *dev = &client->dev; > + int nr, index; > + > + switch (th_index) { > + case LM90_LOW_THRESHOLDS: > + /* remote low limit */ > + nr = NR_CHAN_0_REMOTE_LOW; > + index = TEMP11_REMOTE_LOW; > + break; > + case LM90_HIGH_THRESHOLDS: > + /* remote high limit */ > + nr = NR_CHAN_0_REMOTE_HIGH; > + index = TEMP11_REMOTE_HIGH; > + break; > + default: > + dev_err(dev, "write remote threshold failed.\n"); > + return -EINVAL; > + } > + > + _set_temp11(dev, nr, index, val); > + > + return 0; > +} > + > +static struct thermal_sensor_ops remote_ops = { > + .get_temp = lm90_read_remote_temp, > + .get_threshold = lm90_read_remote_threshold, > + .set_threshold = lm90_write_remote_threshold, > +}; > + > +static int lm90_read_local_temp(struct thermal_sensor *ts, long *temp) > +{ > + struct i2c_client *client = ts->devdata; > + struct device *dev = &client->dev; > + > + _show_temp11(dev, TEMP11_LOCAL_TEMP, (int *)temp); > + Same here and everywhere else. > + return 0; > +} > + > +static int lm90_read_local_threshold(struct thermal_sensor *ts, int th_index, > + long *val) > +{ > + struct i2c_client *client = ts->devdata; > + struct device *dev = &client->dev; > + int index; > + > + switch (th_index) { > + case LM90_LOW_THRESHOLDS: > + /* local low limit */ > + index = TEMP8_LOCAL_LOW; > + break; > + case LM90_HIGH_THRESHOLDS: > + /* local high limit */ > + index = TEMP8_LOCAL_HIGH; > + break; > + default: > + dev_err(dev, "read local threshold failed.\n"); > + return -EINVAL; > + } > + > + _show_temp8(dev, index, (int *)val); > + > + return 0; > +} > + > +static int lm90_write_local_threshold(struct thermal_sensor *ts, int th_index, > + long val) > +{ > + struct i2c_client *client = ts->devdata; > + struct device *dev = &client->dev; > + int index; > + > + switch (th_index) { > + case LM90_LOW_THRESHOLDS: > + /* local low limit */ > + index = TEMP8_LOCAL_LOW; > + break; > + case LM90_HIGH_THRESHOLDS: > + /* local high limit */ > + index = TEMP8_LOCAL_HIGH; > + break; > + default: > + dev_err(dev, "write local threshold failed.\n"); > + return -EINVAL; > + } > + > + _set_temp8(dev, index, val); > + > + return 0; > +} > + > +static struct thermal_sensor_ops local_ops = { > + .get_temp = lm90_read_local_temp, > + .get_threshold = lm90_read_local_threshold, > + .set_threshold = lm90_write_local_threshold, > +}; > + > static int lm90_probe(struct i2c_client *client, > const struct i2c_device_id *id) > { > struct device *dev = &client->dev; > struct i2c_adapter *adapter = to_i2c_adapter(dev->parent); > struct lm90_data *data; > + struct node_args np_args; > int err; > > data = devm_kzalloc(&client->dev, sizeof(struct lm90_data), GFP_KERNEL); > @@ -1576,12 +1728,38 @@ static int lm90_probe(struct i2c_client *client, > "lm90", data); > if (err < 0) { > dev_err(dev, "cannot request interrupt\n"); > - goto exit_remove_files; > + goto exit_unregister_hwmon; > } > } > > + np_args.np = dev->of_node; > + np_args.index = 0; > + data->ts_remote = thermal_sensor_register("lm90_remote", > + LM90_NUM_THRESHOLDS, > + &np_args, > + &remote_ops, client); > + if (IS_ERR(data->ts_remote)) { > + dev_err(dev, "cannot register sensor to thermal framework\n"); > + err = -EINVAL; > + goto exit_unregister_hwmon; > + } > + > + np_args.index = 1; > + data->ts_local = thermal_sensor_register("lm90_local", > + LM90_NUM_THRESHOLDS, > + &np_args, > + &local_ops, client); > + > + if (IS_ERR(data->ts_local)) { > + dev_err(dev, "cannot register sensor to thermal framework\n"); > + err = -EINVAL; > + goto exit_unregister_hwmon; > + } > + How about the second remote sensor ? Granted, not all chips supported by this driver have it, but if we are adding this we might as well add it for all sensors on all chips. > return 0; > > +exit_unregister_hwmon: > + hwmon_device_unregister(data->hwmon_dev); > exit_remove_files: > lm90_remove_files(client, data); > exit_restore: > @@ -1594,6 +1772,8 @@ static int lm90_remove(struct i2c_client *client) > struct lm90_data *data = i2c_get_clientdata(client); > > free_irq(client->irq, data); > + thermal_sensor_unregister(data->ts_remote); > + thermal_sensor_unregister(data->ts_local); > hwmon_device_unregister(data->hwmon_dev); > lm90_remove_files(client, data); > lm90_restore_conf(client, data); > -- > 1.7.9.5 > > > _______________________________________________ > lm-sensors mailing list > lm-sensors@lm-sensors.org > http://lists.lm-sensors.org/mailman/listinfo/lm-sensors > -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 02/18/2013 08:30 PM, Wei Ni wrote: > Register the remote sensor to the thermal framework. > It can support to show the temperature and read/write threshold. > > Signed-off-by: Wei Ni <wni@nvidia.com> > --- > arch/arm/boot/dts/tegra30-cardhu.dtsi | 1 + > drivers/hwmon/lm90.c | 182 ++++++++++++++++++++++++++++++++- > 2 files changed, 182 insertions(+), 1 deletion(-) Making changes to a driver *and* a board file in the same patch? I think this should be separated, and the board file change preferably squashed with the first patch of this series, and moved right after this one. > > diff --git a/arch/arm/boot/dts/tegra30-cardhu.dtsi b/arch/arm/boot/dts/tegra30-cardhu.dtsi > index 15ad1ad..3f6ab89 100644 > --- a/arch/arm/boot/dts/tegra30-cardhu.dtsi > +++ b/arch/arm/boot/dts/tegra30-cardhu.dtsi > @@ -279,6 +279,7 @@ > reg = <0x4c>; > interrupt-parent = <&gpio>; > interrupts = <226 0x08>; /* gpio PCC2 */ > + #sensor-cells = <1>; > }; > }; > > diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c > index de5a476..0abdedc 100644 > --- a/drivers/hwmon/lm90.c > +++ b/drivers/hwmon/lm90.c > @@ -91,6 +91,7 @@ > #include <linux/sysfs.h> > #include <linux/interrupt.h> > #include <linux/of_irq.h> > +#include <linux/thermal.h> > > /* > * Addresses to scan > @@ -182,6 +183,15 @@ enum chips { lm90, adm1032, lm99, lm86, max6657, max6659, adt7461, max6680, > #define LM90_HAVE_BROKEN_ALERT (1 << 7) /* Broken alert */ > > /* > + * Thermal framework > + */ > +enum lm90_thresholds { > + LM90_LOW_THRESHOLDS = 0, /* threshold 0: low limits */ > + LM90_HIGH_THRESHOLDS, /* threshold 1: high limits */ > + LM90_NUM_THRESHOLDS > +}; > + > +/* > * Driver data (common to all clients) > */ > > @@ -377,6 +387,9 @@ struct lm90_data { > s16 temp11[TEMP11_REG_NUM]; > u8 temp_hyst; > u16 alarms; /* bitvector (upper 8 bits for max6695/96) */ > + > + struct thermal_sensor *ts_remote; > + struct thermal_sensor *ts_local; > }; > > /* > @@ -1493,12 +1506,151 @@ static irqreturn_t lm90_irq(int irq, void *dev_id) > return IRQ_HANDLED; > } > > +static int lm90_read_remote_temp(struct thermal_sensor *ts, long *temp) > +{ > + struct i2c_client *client = ts->devdata; > + struct device *dev = &client->dev; > + > + _show_temp11(dev, TEMP11_REMOTE_TEMP, (int *)temp); As Guenter pointed, this might break. Since you introduced _show_temp11 in a previous patch, you should revise it to take a long * as third argument (or even better, return a long). Or if you cannot do that for some reason, use a temporary int and affect temp properly (*temp = temp_int). > + > + return 0; > +} > + > +static int lm90_read_remote_threshold(struct thermal_sensor *ts, int th_index, > + long *val) > +{ > + struct i2c_client *client = ts->devdata; > + struct device *dev = &client->dev; > + int index; > + > + switch (th_index) { > + case LM90_LOW_THRESHOLDS: > + /* remote low limit */ > + index = TEMP11_REMOTE_LOW; > + break; > + case LM90_HIGH_THRESHOLDS: > + /* remote high limit */ > + index = TEMP11_REMOTE_HIGH; > + break; > + default: > + dev_err(dev, "read remote threshold failed.\n"); > + return -EINVAL; > + } > + > + _show_temp11(dev, index, (int *)val); > + > + return 0; > +} > + > +static int lm90_write_remote_threshold(struct thermal_sensor *ts, int th_index, > + long val) > +{ > + struct i2c_client *client = ts->devdata; > + struct device *dev = &client->dev; > + int nr, index; > + > + switch (th_index) { > + case LM90_LOW_THRESHOLDS: > + /* remote low limit */ > + nr = NR_CHAN_0_REMOTE_LOW; > + index = TEMP11_REMOTE_LOW; > + break; > + case LM90_HIGH_THRESHOLDS: > + /* remote high limit */ > + nr = NR_CHAN_0_REMOTE_HIGH; > + index = TEMP11_REMOTE_HIGH; > + break; > + default: > + dev_err(dev, "write remote threshold failed.\n"); > + return -EINVAL; > + } > + > + _set_temp11(dev, nr, index, val); > + > + return 0; > +} > + > +static struct thermal_sensor_ops remote_ops = { > + .get_temp = lm90_read_remote_temp, > + .get_threshold = lm90_read_remote_threshold, > + .set_threshold = lm90_write_remote_threshold, > +}; > + > +static int lm90_read_local_temp(struct thermal_sensor *ts, long *temp) > +{ > + struct i2c_client *client = ts->devdata; > + struct device *dev = &client->dev; > + > + _show_temp11(dev, TEMP11_LOCAL_TEMP, (int *)temp); > + > + return 0; > +} > + > +static int lm90_read_local_threshold(struct thermal_sensor *ts, int th_index, > + long *val) > +{ > + struct i2c_client *client = ts->devdata; > + struct device *dev = &client->dev; > + int index; > + > + switch (th_index) { > + case LM90_LOW_THRESHOLDS: > + /* local low limit */ > + index = TEMP8_LOCAL_LOW; > + break; > + case LM90_HIGH_THRESHOLDS: > + /* local high limit */ > + index = TEMP8_LOCAL_HIGH; > + break; I think the comments are unneeded here, the macro name should be explicit enough. > + default: > + dev_err(dev, "read local threshold failed.\n"); > + return -EINVAL; > + } > + > + _show_temp8(dev, index, (int *)val); > + > + return 0; > +} > + > +static int lm90_write_local_threshold(struct thermal_sensor *ts, int th_index, > + long val) > +{ > + struct i2c_client *client = ts->devdata; > + struct device *dev = &client->dev; > + int index; > + > + switch (th_index) { > + case LM90_LOW_THRESHOLDS: > + /* local low limit */ > + index = TEMP8_LOCAL_LOW; > + break; > + case LM90_HIGH_THRESHOLDS: > + /* local high limit */ > + index = TEMP8_LOCAL_HIGH; > + break; > + default: > + dev_err(dev, "write local threshold failed.\n"); > + return -EINVAL; > + } > + > + _set_temp8(dev, index, val); > + > + return 0; > +} > + > +static struct thermal_sensor_ops local_ops = { > + .get_temp = lm90_read_local_temp, > + .get_threshold = lm90_read_local_threshold, > + .set_threshold = lm90_write_local_threshold, > +}; > + > static int lm90_probe(struct i2c_client *client, > const struct i2c_device_id *id) > { > struct device *dev = &client->dev; > struct i2c_adapter *adapter = to_i2c_adapter(dev->parent); > struct lm90_data *data; > + struct node_args np_args; > int err; > > data = devm_kzalloc(&client->dev, sizeof(struct lm90_data), GFP_KERNEL); > @@ -1576,12 +1728,38 @@ static int lm90_probe(struct i2c_client *client, > "lm90", data); > if (err < 0) { > dev_err(dev, "cannot request interrupt\n"); > - goto exit_remove_files; > + goto exit_unregister_hwmon; > } > } > > + np_args.np = dev->of_node; > + np_args.index = 0; > + data->ts_remote = thermal_sensor_register("lm90_remote", > + LM90_NUM_THRESHOLDS, > + &np_args, > + &remote_ops, client); > + if (IS_ERR(data->ts_remote)) { > + dev_err(dev, "cannot register sensor to thermal framework\n"); > + err = -EINVAL; When don't you return the error code provided by thermal_sensor_register, e.g. err = PTR_ERR(data->ts_remote) ? > + goto exit_unregister_hwmon; > + } > + > + np_args.index = 1; > + data->ts_local = thermal_sensor_register("lm90_local", > + LM90_NUM_THRESHOLDS, > + &np_args, > + &local_ops, client); > + > + if (IS_ERR(data->ts_local)) { > + dev_err(dev, "cannot register sensor to thermal framework\n"); > + err = -EINVAL; Same thing here. > + goto exit_unregister_hwmon; > + } > + > return 0; > > +exit_unregister_hwmon: > + hwmon_device_unregister(data->hwmon_dev); > exit_remove_files: > lm90_remove_files(client, data); > exit_restore: > @@ -1594,6 +1772,8 @@ static int lm90_remove(struct i2c_client *client) > struct lm90_data *data = i2c_get_clientdata(client); > > free_irq(client->irq, data); > + thermal_sensor_unregister(data->ts_remote); > + thermal_sensor_unregister(data->ts_local); Ideally you would unregister your sensors in the reverse order they have been registered, but I'm being picky here. Alex. -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 02/19/2013 11:42 AM, Guenter Roeck wrote: > On Mon, Feb 18, 2013 at 07:30:28PM +0800, Wei Ni wrote: >> Register the remote sensor to the thermal framework. >> It can support to show the temperature and read/write threshold. >> >> Signed-off-by: Wei Ni <wni@nvidia.com> >> --- >> arch/arm/boot/dts/tegra30-cardhu.dtsi | 1 + >> drivers/hwmon/lm90.c | 182 ++++++++++++++++++++++++++++++++- >> 2 files changed, 182 insertions(+), 1 deletion(-) >> >> diff --git a/arch/arm/boot/dts/tegra30-cardhu.dtsi b/arch/arm/boot/dts/tegra30-cardhu.dtsi >> index 15ad1ad..3f6ab89 100644 >> --- a/arch/arm/boot/dts/tegra30-cardhu.dtsi >> +++ b/arch/arm/boot/dts/tegra30-cardhu.dtsi >> @@ -279,6 +279,7 @@ >> reg = <0x4c>; >> interrupt-parent = <&gpio>; >> interrupts = <226 0x08>; /* gpio PCC2 */ >> + #sensor-cells = <1>; >> }; >> }; >> >> diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c >> index de5a476..0abdedc 100644 >> --- a/drivers/hwmon/lm90.c >> +++ b/drivers/hwmon/lm90.c >> @@ -91,6 +91,7 @@ >> #include <linux/sysfs.h> >> #include <linux/interrupt.h> >> #include <linux/of_irq.h> >> +#include <linux/thermal.h> >> >> /* >> * Addresses to scan >> @@ -182,6 +183,15 @@ enum chips { lm90, adm1032, lm99, lm86, max6657, max6659, adt7461, max6680, >> #define LM90_HAVE_BROKEN_ALERT (1 << 7) /* Broken alert */ >> >> /* >> + * Thermal framework >> + */ >> +enum lm90_thresholds { >> + LM90_LOW_THRESHOLDS = 0, /* threshold 0: low limits */ >> + LM90_HIGH_THRESHOLDS, /* threshold 1: high limits */ >> + LM90_NUM_THRESHOLDS >> +}; >> + >> +/* >> * Driver data (common to all clients) >> */ >> >> @@ -377,6 +387,9 @@ struct lm90_data { >> s16 temp11[TEMP11_REG_NUM]; >> u8 temp_hyst; >> u16 alarms; /* bitvector (upper 8 bits for max6695/96) */ >> + >> + struct thermal_sensor *ts_remote; >> + struct thermal_sensor *ts_local; >> }; >> >> /* >> @@ -1493,12 +1506,151 @@ static irqreturn_t lm90_irq(int irq, void *dev_id) >> return IRQ_HANDLED; >> } >> >> +static int lm90_read_remote_temp(struct thermal_sensor *ts, long *temp) >> +{ >> + struct i2c_client *client = ts->devdata; >> + struct device *dev = &client->dev; >> + >> + _show_temp11(dev, TEMP11_REMOTE_TEMP, (int *)temp); >> + >> + return 0; >> +} >> + >> +static int lm90_read_remote_threshold(struct thermal_sensor *ts, int th_index, >> + long *val) >> +{ >> + struct i2c_client *client = ts->devdata; >> + struct device *dev = &client->dev; >> + int index; >> + >> + switch (th_index) { >> + case LM90_LOW_THRESHOLDS: >> + /* remote low limit */ >> + index = TEMP11_REMOTE_LOW; >> + break; >> + case LM90_HIGH_THRESHOLDS: >> + /* remote high limit */ >> + index = TEMP11_REMOTE_HIGH; >> + break; >> + default: >> + dev_err(dev, "read remote threshold failed.\n"); >> + return -EINVAL; >> + } >> + >> + _show_temp11(dev, index, (int *)val); > > Typecasting a pointer like this doesn't work. Try this on a big-endian system with > sizeof(int) != sizeof(long). Note that you would not have the problem if you > would pass the value instead of the pointer. Yes, you are right, it will cause problems. I will return the value for _show_temp11() and _show_temp8(). > >> + >> + return 0; >> +} >> + >> +static int lm90_write_remote_threshold(struct thermal_sensor *ts, int th_index, >> + long val) >> +{ >> + struct i2c_client *client = ts->devdata; >> + struct device *dev = &client->dev; >> + int nr, index; >> + >> + switch (th_index) { >> + case LM90_LOW_THRESHOLDS: >> + /* remote low limit */ >> + nr = NR_CHAN_0_REMOTE_LOW; >> + index = TEMP11_REMOTE_LOW; >> + break; >> + case LM90_HIGH_THRESHOLDS: >> + /* remote high limit */ >> + nr = NR_CHAN_0_REMOTE_HIGH; >> + index = TEMP11_REMOTE_HIGH; >> + break; >> + default: >> + dev_err(dev, "write remote threshold failed.\n"); >> + return -EINVAL; >> + } >> + >> + _set_temp11(dev, nr, index, val); >> + >> + return 0; >> +} >> + >> +static struct thermal_sensor_ops remote_ops = { >> + .get_temp = lm90_read_remote_temp, >> + .get_threshold = lm90_read_remote_threshold, >> + .set_threshold = lm90_write_remote_threshold, >> +}; >> + >> +static int lm90_read_local_temp(struct thermal_sensor *ts, long *temp) >> +{ >> + struct i2c_client *client = ts->devdata; >> + struct device *dev = &client->dev; >> + >> + _show_temp11(dev, TEMP11_LOCAL_TEMP, (int *)temp); >> + > Same here and everywhere else. > >> + return 0; >> +} >> + >> +static int lm90_read_local_threshold(struct thermal_sensor *ts, int th_index, >> + long *val) >> +{ >> + struct i2c_client *client = ts->devdata; >> + struct device *dev = &client->dev; >> + int index; >> + >> + switch (th_index) { >> + case LM90_LOW_THRESHOLDS: >> + /* local low limit */ >> + index = TEMP8_LOCAL_LOW; >> + break; >> + case LM90_HIGH_THRESHOLDS: >> + /* local high limit */ >> + index = TEMP8_LOCAL_HIGH; >> + break; >> + default: >> + dev_err(dev, "read local threshold failed.\n"); >> + return -EINVAL; >> + } >> + >> + _show_temp8(dev, index, (int *)val); >> + >> + return 0; >> +} >> + >> +static int lm90_write_local_threshold(struct thermal_sensor *ts, int th_index, >> + long val) >> +{ >> + struct i2c_client *client = ts->devdata; >> + struct device *dev = &client->dev; >> + int index; >> + >> + switch (th_index) { >> + case LM90_LOW_THRESHOLDS: >> + /* local low limit */ >> + index = TEMP8_LOCAL_LOW; >> + break; >> + case LM90_HIGH_THRESHOLDS: >> + /* local high limit */ >> + index = TEMP8_LOCAL_HIGH; >> + break; >> + default: >> + dev_err(dev, "write local threshold failed.\n"); >> + return -EINVAL; >> + } >> + >> + _set_temp8(dev, index, val); >> + >> + return 0; >> +} >> + >> +static struct thermal_sensor_ops local_ops = { >> + .get_temp = lm90_read_local_temp, >> + .get_threshold = lm90_read_local_threshold, >> + .set_threshold = lm90_write_local_threshold, >> +}; >> + >> static int lm90_probe(struct i2c_client *client, >> const struct i2c_device_id *id) >> { >> struct device *dev = &client->dev; >> struct i2c_adapter *adapter = to_i2c_adapter(dev->parent); >> struct lm90_data *data; >> + struct node_args np_args; >> int err; >> >> data = devm_kzalloc(&client->dev, sizeof(struct lm90_data), GFP_KERNEL); >> @@ -1576,12 +1728,38 @@ static int lm90_probe(struct i2c_client *client, >> "lm90", data); >> if (err < 0) { >> dev_err(dev, "cannot request interrupt\n"); >> - goto exit_remove_files; >> + goto exit_unregister_hwmon; >> } >> } >> >> + np_args.np = dev->of_node; >> + np_args.index = 0; >> + data->ts_remote = thermal_sensor_register("lm90_remote", >> + LM90_NUM_THRESHOLDS, >> + &np_args, >> + &remote_ops, client); >> + if (IS_ERR(data->ts_remote)) { >> + dev_err(dev, "cannot register sensor to thermal framework\n"); >> + err = -EINVAL; >> + goto exit_unregister_hwmon; >> + } >> + >> + np_args.index = 1; >> + data->ts_local = thermal_sensor_register("lm90_local", >> + LM90_NUM_THRESHOLDS, >> + &np_args, >> + &local_ops, client); >> + >> + if (IS_ERR(data->ts_local)) { >> + dev_err(dev, "cannot register sensor to thermal framework\n"); >> + err = -EINVAL; >> + goto exit_unregister_hwmon; >> + } >> + > > How about the second remote sensor ? Granted, not all chips supported by this > driver have it, but if we are adding this we might as well add it for all > sensors on all chips. Ok, I will improve it. > >> return 0; >> >> +exit_unregister_hwmon: >> + hwmon_device_unregister(data->hwmon_dev); >> exit_remove_files: >> lm90_remove_files(client, data); >> exit_restore: >> @@ -1594,6 +1772,8 @@ static int lm90_remove(struct i2c_client *client) >> struct lm90_data *data = i2c_get_clientdata(client); >> >> free_irq(client->irq, data); >> + thermal_sensor_unregister(data->ts_remote); >> + thermal_sensor_unregister(data->ts_local); >> hwmon_device_unregister(data->hwmon_dev); >> lm90_remove_files(client, data); >> lm90_restore_conf(client, data); >> -- >> 1.7.9.5 >> >> >> _______________________________________________ >> lm-sensors mailing list >> lm-sensors@lm-sensors.org >> http://lists.lm-sensors.org/mailman/listinfo/lm-sensors >> -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 02/19/2013 01:22 PM, Alex Courbot wrote: > On 02/18/2013 08:30 PM, Wei Ni wrote: >> Register the remote sensor to the thermal framework. >> It can support to show the temperature and read/write threshold. >> >> Signed-off-by: Wei Ni <wni@nvidia.com> >> --- >> arch/arm/boot/dts/tegra30-cardhu.dtsi | 1 + >> drivers/hwmon/lm90.c | 182 ++++++++++++++++++++++++++++++++- >> 2 files changed, 182 insertions(+), 1 deletion(-) > > Making changes to a driver *and* a board file in the same patch? I think > this should be separated, and the board file change preferably squashed > with the first patch of this series, and moved right after this one. > >> >> diff --git a/arch/arm/boot/dts/tegra30-cardhu.dtsi b/arch/arm/boot/dts/tegra30-cardhu.dtsi >> index 15ad1ad..3f6ab89 100644 >> --- a/arch/arm/boot/dts/tegra30-cardhu.dtsi >> +++ b/arch/arm/boot/dts/tegra30-cardhu.dtsi >> @@ -279,6 +279,7 @@ >> reg = <0x4c>; >> interrupt-parent = <&gpio>; >> interrupts = <226 0x08>; /* gpio PCC2 */ >> + #sensor-cells = <1>; >> }; >> }; >> >> diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c >> index de5a476..0abdedc 100644 >> --- a/drivers/hwmon/lm90.c >> +++ b/drivers/hwmon/lm90.c >> @@ -91,6 +91,7 @@ >> #include <linux/sysfs.h> >> #include <linux/interrupt.h> >> #include <linux/of_irq.h> >> +#include <linux/thermal.h> >> >> /* >> * Addresses to scan >> @@ -182,6 +183,15 @@ enum chips { lm90, adm1032, lm99, lm86, max6657, max6659, adt7461, max6680, >> #define LM90_HAVE_BROKEN_ALERT (1 << 7) /* Broken alert */ >> >> /* >> + * Thermal framework >> + */ >> +enum lm90_thresholds { >> + LM90_LOW_THRESHOLDS = 0, /* threshold 0: low limits */ >> + LM90_HIGH_THRESHOLDS, /* threshold 1: high limits */ >> + LM90_NUM_THRESHOLDS >> +}; >> + >> +/* >> * Driver data (common to all clients) >> */ >> >> @@ -377,6 +387,9 @@ struct lm90_data { >> s16 temp11[TEMP11_REG_NUM]; >> u8 temp_hyst; >> u16 alarms; /* bitvector (upper 8 bits for max6695/96) */ >> + >> + struct thermal_sensor *ts_remote; >> + struct thermal_sensor *ts_local; >> }; >> >> /* >> @@ -1493,12 +1506,151 @@ static irqreturn_t lm90_irq(int irq, void *dev_id) >> return IRQ_HANDLED; >> } >> >> +static int lm90_read_remote_temp(struct thermal_sensor *ts, long *temp) >> +{ >> + struct i2c_client *client = ts->devdata; >> + struct device *dev = &client->dev; >> + >> + _show_temp11(dev, TEMP11_REMOTE_TEMP, (int *)temp); > > As Guenter pointed, this might break. Since you introduced _show_temp11 > in a previous patch, you should revise it to take a long * as third > argument (or even better, return a long). Or if you cannot do that for > some reason, use a temporary int and affect temp properly (*temp = > temp_int). yes, the pointer will cause problems here. I will follow Guenter suggestion to return the value simply for _show_temp11 and _show_temp8 > >> + >> + return 0; >> +} >> + >> +static int lm90_read_remote_threshold(struct thermal_sensor *ts, int th_index, >> + long *val) >> +{ >> + struct i2c_client *client = ts->devdata; >> + struct device *dev = &client->dev; >> + int index; >> + >> + switch (th_index) { >> + case LM90_LOW_THRESHOLDS: >> + /* remote low limit */ >> + index = TEMP11_REMOTE_LOW; >> + break; >> + case LM90_HIGH_THRESHOLDS: >> + /* remote high limit */ >> + index = TEMP11_REMOTE_HIGH; >> + break; >> + default: >> + dev_err(dev, "read remote threshold failed.\n"); >> + return -EINVAL; >> + } >> + >> + _show_temp11(dev, index, (int *)val); >> + >> + return 0; >> +} >> + >> +static int lm90_write_remote_threshold(struct thermal_sensor *ts, int th_index, >> + long val) >> +{ >> + struct i2c_client *client = ts->devdata; >> + struct device *dev = &client->dev; >> + int nr, index; >> + >> + switch (th_index) { >> + case LM90_LOW_THRESHOLDS: >> + /* remote low limit */ >> + nr = NR_CHAN_0_REMOTE_LOW; >> + index = TEMP11_REMOTE_LOW; >> + break; >> + case LM90_HIGH_THRESHOLDS: >> + /* remote high limit */ >> + nr = NR_CHAN_0_REMOTE_HIGH; >> + index = TEMP11_REMOTE_HIGH; >> + break; >> + default: >> + dev_err(dev, "write remote threshold failed.\n"); >> + return -EINVAL; >> + } >> + >> + _set_temp11(dev, nr, index, val); >> + >> + return 0; >> +} >> + >> +static struct thermal_sensor_ops remote_ops = { >> + .get_temp = lm90_read_remote_temp, >> + .get_threshold = lm90_read_remote_threshold, >> + .set_threshold = lm90_write_remote_threshold, >> +}; >> + >> +static int lm90_read_local_temp(struct thermal_sensor *ts, long *temp) >> +{ >> + struct i2c_client *client = ts->devdata; >> + struct device *dev = &client->dev; >> + >> + _show_temp11(dev, TEMP11_LOCAL_TEMP, (int *)temp); >> + >> + return 0; >> +} >> + >> +static int lm90_read_local_threshold(struct thermal_sensor *ts, int th_index, >> + long *val) >> +{ >> + struct i2c_client *client = ts->devdata; >> + struct device *dev = &client->dev; >> + int index; >> + >> + switch (th_index) { >> + case LM90_LOW_THRESHOLDS: >> + /* local low limit */ >> + index = TEMP8_LOCAL_LOW; >> + break; >> + case LM90_HIGH_THRESHOLDS: >> + /* local high limit */ >> + index = TEMP8_LOCAL_HIGH; >> + break; > > I think the comments are unneeded here, the macro name should be > explicit enough. OK. > >> + default: >> + dev_err(dev, "read local threshold failed.\n"); >> + return -EINVAL; >> + } >> + >> + _show_temp8(dev, index, (int *)val); >> + >> + return 0; >> +} >> + >> +static int lm90_write_local_threshold(struct thermal_sensor *ts, int th_index, >> + long val) >> +{ >> + struct i2c_client *client = ts->devdata; >> + struct device *dev = &client->dev; >> + int index; >> + >> + switch (th_index) { >> + case LM90_LOW_THRESHOLDS: >> + /* local low limit */ >> + index = TEMP8_LOCAL_LOW; >> + break; >> + case LM90_HIGH_THRESHOLDS: >> + /* local high limit */ >> + index = TEMP8_LOCAL_HIGH; >> + break; >> + default: >> + dev_err(dev, "write local threshold failed.\n"); >> + return -EINVAL; >> + } >> + >> + _set_temp8(dev, index, val); >> + >> + return 0; >> +} >> + >> +static struct thermal_sensor_ops local_ops = { >> + .get_temp = lm90_read_local_temp, >> + .get_threshold = lm90_read_local_threshold, >> + .set_threshold = lm90_write_local_threshold, >> +}; >> + >> static int lm90_probe(struct i2c_client *client, >> const struct i2c_device_id *id) >> { >> struct device *dev = &client->dev; >> struct i2c_adapter *adapter = to_i2c_adapter(dev->parent); >> struct lm90_data *data; >> + struct node_args np_args; >> int err; >> >> data = devm_kzalloc(&client->dev, sizeof(struct lm90_data), GFP_KERNEL); >> @@ -1576,12 +1728,38 @@ static int lm90_probe(struct i2c_client *client, >> "lm90", data); >> if (err < 0) { >> dev_err(dev, "cannot request interrupt\n"); >> - goto exit_remove_files; >> + goto exit_unregister_hwmon; >> } >> } >> >> + np_args.np = dev->of_node; >> + np_args.index = 0; >> + data->ts_remote = thermal_sensor_register("lm90_remote", >> + LM90_NUM_THRESHOLDS, >> + &np_args, >> + &remote_ops, client); >> + if (IS_ERR(data->ts_remote)) { >> + dev_err(dev, "cannot register sensor to thermal framework\n"); >> + err = -EINVAL; > > When don't you return the error code provided by > thermal_sensor_register, e.g. err = PTR_ERR(data->ts_remote) ? I didn't consider it, I will change it. > >> + goto exit_unregister_hwmon; >> + } >> + >> + np_args.index = 1; >> + data->ts_local = thermal_sensor_register("lm90_local", >> + LM90_NUM_THRESHOLDS, >> + &np_args, >> + &local_ops, client); >> + >> + if (IS_ERR(data->ts_local)) { >> + dev_err(dev, "cannot register sensor to thermal framework\n"); >> + err = -EINVAL; > > Same thing here. > >> + goto exit_unregister_hwmon; >> + } >> + >> return 0; >> >> +exit_unregister_hwmon: >> + hwmon_device_unregister(data->hwmon_dev); >> exit_remove_files: >> lm90_remove_files(client, data); >> exit_restore: >> @@ -1594,6 +1772,8 @@ static int lm90_remove(struct i2c_client *client) >> struct lm90_data *data = i2c_get_clientdata(client); >> >> free_irq(client->irq, data); >> + thermal_sensor_unregister(data->ts_remote); >> + thermal_sensor_unregister(data->ts_local); > > Ideally you would unregister your sensors in the reverse order they have > been registered, but I'm being picky here. Yes, it's better in reverse order. I really appreciate you reviewing my patches so carefully :) Wei. > > Alex. > -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 02/18/2013 04:30 AM, Wei Ni wrote: > Register the remote sensor to the thermal framework. > It can support to show the temperature and read/write threshold. > arch/arm/boot/dts/tegra30-cardhu.dtsi | 1 + > drivers/hwmon/lm90.c | 182 ++++++++++++++++++++++++++++++++- There shouldn't be any need to edit both those files in the same patch. This patch is making multiple separate logical changes, which should be separate patches. > diff --git a/arch/arm/boot/dts/tegra30-cardhu.dtsi b/arch/arm/boot/dts/tegra30-cardhu.dtsi > + #sensor-cells = <1>; Again, in order to allow that, you need some documentation in Documentation/devicetree/bindings/. Related to that, you need to Cc the devicetree maintainers on at least the patches that add that documentation. > diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c > @@ -1576,12 +1728,38 @@ static int lm90_probe(struct i2c_client *client, ... > + np_args.np = dev->of_node; > + np_args.index = 0; As I mentioned before, that'd be better replaced by an explicit of_xlate function. -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 02/20/2013 07:18 AM, Stephen Warren wrote: > On 02/18/2013 04:30 AM, Wei Ni wrote: >> Register the remote sensor to the thermal framework. >> It can support to show the temperature and read/write threshold. > >> arch/arm/boot/dts/tegra30-cardhu.dtsi | 1 + >> drivers/hwmon/lm90.c | 182 ++++++++++++++++++++++++++++++++- > > There shouldn't be any need to edit both those files in the same patch. > This patch is making multiple separate logical changes, which should be > separate patches. I will follow Matthew's suggestion to split this serial patch into 3 separate sets. > >> diff --git a/arch/arm/boot/dts/tegra30-cardhu.dtsi b/arch/arm/boot/dts/tegra30-cardhu.dtsi > >> + #sensor-cells = <1>; > > Again, in order to allow that, you need some documentation in > Documentation/devicetree/bindings/. Related to that, you need to Cc the > devicetree maintainers on at least the patches that add that documentation. Ok, I will do it. > >> diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c > >> @@ -1576,12 +1728,38 @@ static int lm90_probe(struct i2c_client *client, > ... >> + np_args.np = dev->of_node; >> + np_args.index = 0; > > As I mentioned before, that'd be better replaced by an explicit of_xlate > function. Yes, I will try to use this func. > -- > To unsubscribe from this list: send the line "unsubscribe linux-pm" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/arch/arm/boot/dts/tegra30-cardhu.dtsi b/arch/arm/boot/dts/tegra30-cardhu.dtsi index 15ad1ad..3f6ab89 100644 --- a/arch/arm/boot/dts/tegra30-cardhu.dtsi +++ b/arch/arm/boot/dts/tegra30-cardhu.dtsi @@ -279,6 +279,7 @@ reg = <0x4c>; interrupt-parent = <&gpio>; interrupts = <226 0x08>; /* gpio PCC2 */ + #sensor-cells = <1>; }; }; diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c index de5a476..0abdedc 100644 --- a/drivers/hwmon/lm90.c +++ b/drivers/hwmon/lm90.c @@ -91,6 +91,7 @@ #include <linux/sysfs.h> #include <linux/interrupt.h> #include <linux/of_irq.h> +#include <linux/thermal.h> /* * Addresses to scan @@ -182,6 +183,15 @@ enum chips { lm90, adm1032, lm99, lm86, max6657, max6659, adt7461, max6680, #define LM90_HAVE_BROKEN_ALERT (1 << 7) /* Broken alert */ /* + * Thermal framework + */ +enum lm90_thresholds { + LM90_LOW_THRESHOLDS = 0, /* threshold 0: low limits */ + LM90_HIGH_THRESHOLDS, /* threshold 1: high limits */ + LM90_NUM_THRESHOLDS +}; + +/* * Driver data (common to all clients) */ @@ -377,6 +387,9 @@ struct lm90_data { s16 temp11[TEMP11_REG_NUM]; u8 temp_hyst; u16 alarms; /* bitvector (upper 8 bits for max6695/96) */ + + struct thermal_sensor *ts_remote; + struct thermal_sensor *ts_local; }; /* @@ -1493,12 +1506,151 @@ static irqreturn_t lm90_irq(int irq, void *dev_id) return IRQ_HANDLED; } +static int lm90_read_remote_temp(struct thermal_sensor *ts, long *temp) +{ + struct i2c_client *client = ts->devdata; + struct device *dev = &client->dev; + + _show_temp11(dev, TEMP11_REMOTE_TEMP, (int *)temp); + + return 0; +} + +static int lm90_read_remote_threshold(struct thermal_sensor *ts, int th_index, + long *val) +{ + struct i2c_client *client = ts->devdata; + struct device *dev = &client->dev; + int index; + + switch (th_index) { + case LM90_LOW_THRESHOLDS: + /* remote low limit */ + index = TEMP11_REMOTE_LOW; + break; + case LM90_HIGH_THRESHOLDS: + /* remote high limit */ + index = TEMP11_REMOTE_HIGH; + break; + default: + dev_err(dev, "read remote threshold failed.\n"); + return -EINVAL; + } + + _show_temp11(dev, index, (int *)val); + + return 0; +} + +static int lm90_write_remote_threshold(struct thermal_sensor *ts, int th_index, + long val) +{ + struct i2c_client *client = ts->devdata; + struct device *dev = &client->dev; + int nr, index; + + switch (th_index) { + case LM90_LOW_THRESHOLDS: + /* remote low limit */ + nr = NR_CHAN_0_REMOTE_LOW; + index = TEMP11_REMOTE_LOW; + break; + case LM90_HIGH_THRESHOLDS: + /* remote high limit */ + nr = NR_CHAN_0_REMOTE_HIGH; + index = TEMP11_REMOTE_HIGH; + break; + default: + dev_err(dev, "write remote threshold failed.\n"); + return -EINVAL; + } + + _set_temp11(dev, nr, index, val); + + return 0; +} + +static struct thermal_sensor_ops remote_ops = { + .get_temp = lm90_read_remote_temp, + .get_threshold = lm90_read_remote_threshold, + .set_threshold = lm90_write_remote_threshold, +}; + +static int lm90_read_local_temp(struct thermal_sensor *ts, long *temp) +{ + struct i2c_client *client = ts->devdata; + struct device *dev = &client->dev; + + _show_temp11(dev, TEMP11_LOCAL_TEMP, (int *)temp); + + return 0; +} + +static int lm90_read_local_threshold(struct thermal_sensor *ts, int th_index, + long *val) +{ + struct i2c_client *client = ts->devdata; + struct device *dev = &client->dev; + int index; + + switch (th_index) { + case LM90_LOW_THRESHOLDS: + /* local low limit */ + index = TEMP8_LOCAL_LOW; + break; + case LM90_HIGH_THRESHOLDS: + /* local high limit */ + index = TEMP8_LOCAL_HIGH; + break; + default: + dev_err(dev, "read local threshold failed.\n"); + return -EINVAL; + } + + _show_temp8(dev, index, (int *)val); + + return 0; +} + +static int lm90_write_local_threshold(struct thermal_sensor *ts, int th_index, + long val) +{ + struct i2c_client *client = ts->devdata; + struct device *dev = &client->dev; + int index; + + switch (th_index) { + case LM90_LOW_THRESHOLDS: + /* local low limit */ + index = TEMP8_LOCAL_LOW; + break; + case LM90_HIGH_THRESHOLDS: + /* local high limit */ + index = TEMP8_LOCAL_HIGH; + break; + default: + dev_err(dev, "write local threshold failed.\n"); + return -EINVAL; + } + + _set_temp8(dev, index, val); + + return 0; +} + +static struct thermal_sensor_ops local_ops = { + .get_temp = lm90_read_local_temp, + .get_threshold = lm90_read_local_threshold, + .set_threshold = lm90_write_local_threshold, +}; + static int lm90_probe(struct i2c_client *client, const struct i2c_device_id *id) { struct device *dev = &client->dev; struct i2c_adapter *adapter = to_i2c_adapter(dev->parent); struct lm90_data *data; + struct node_args np_args; int err; data = devm_kzalloc(&client->dev, sizeof(struct lm90_data), GFP_KERNEL); @@ -1576,12 +1728,38 @@ static int lm90_probe(struct i2c_client *client, "lm90", data); if (err < 0) { dev_err(dev, "cannot request interrupt\n"); - goto exit_remove_files; + goto exit_unregister_hwmon; } } + np_args.np = dev->of_node; + np_args.index = 0; + data->ts_remote = thermal_sensor_register("lm90_remote", + LM90_NUM_THRESHOLDS, + &np_args, + &remote_ops, client); + if (IS_ERR(data->ts_remote)) { + dev_err(dev, "cannot register sensor to thermal framework\n"); + err = -EINVAL; + goto exit_unregister_hwmon; + } + + np_args.index = 1; + data->ts_local = thermal_sensor_register("lm90_local", + LM90_NUM_THRESHOLDS, + &np_args, + &local_ops, client); + + if (IS_ERR(data->ts_local)) { + dev_err(dev, "cannot register sensor to thermal framework\n"); + err = -EINVAL; + goto exit_unregister_hwmon; + } + return 0; +exit_unregister_hwmon: + hwmon_device_unregister(data->hwmon_dev); exit_remove_files: lm90_remove_files(client, data); exit_restore: @@ -1594,6 +1772,8 @@ static int lm90_remove(struct i2c_client *client) struct lm90_data *data = i2c_get_clientdata(client); free_irq(client->irq, data); + thermal_sensor_unregister(data->ts_remote); + thermal_sensor_unregister(data->ts_local); hwmon_device_unregister(data->hwmon_dev); lm90_remove_files(client, data); lm90_restore_conf(client, data);
Register the remote sensor to the thermal framework. It can support to show the temperature and read/write threshold. Signed-off-by: Wei Ni <wni@nvidia.com> --- arch/arm/boot/dts/tegra30-cardhu.dtsi | 1 + drivers/hwmon/lm90.c | 182 ++++++++++++++++++++++++++++++++- 2 files changed, 182 insertions(+), 1 deletion(-)