Re: [PATCH v2] hwmon: Driver for Maxim MAX6697 and compatibles
On Mon, Jan 14, 2013 at 10:24:04PM +0100, Jean Delvare wrote:
> Hi Guenter,
>
> Sorry for the late review, originally I planned to do a quick review
> but apparently I am simply unable to do that. So here comes a complete
> review. As usual, pick what you agree with and feel free to ignore the
> rest :)
>
Hi Jean,
as always an excellent and thorough review.
Couple of comments inline.
> On Sun, 16 Dec 2012 21:33:09 -0800, Guenter Roeck wrote:
> > Add support for MAX6581, MAX6602, MAX6622, MAX6636, MAX6689, MAX6693,
> > MAX6694, MAX6697, MAX6698, and MAX6699 temperature sensors
> >
> > Signed-off-by: Guenter Roeck
> > ---
> > v2:
> > - Add suppport for platform data and devicetree based chip initialization
> > - Drop S_IRUGOWU macro: s/S_IRUGOWU/S_IRUGO | S_IWUSR/
> >
> > Documentation/devicetree/bindings/i2c/max6697.txt | 45 ++
> > Documentation/hwmon/max6697 | 57 ++
> > drivers/hwmon/Kconfig | 11 +
> > drivers/hwmon/Makefile|1 +
> > drivers/hwmon/max6697.c | 634
> > +
> > include/linux/platform_data/max6697.h | 25 +
> > 6 files changed, 773 insertions(+)
> > create mode 100644 Documentation/devicetree/bindings/i2c/max6697.txt
> > create mode 100644 Documentation/hwmon/max6697
> > create mode 100644 drivers/hwmon/max6697.c
> > create mode 100644 include/linux/platform_data/max6697.h
> >
> > diff --git a/Documentation/devicetree/bindings/i2c/max6697.txt
> > b/Documentation/devicetree/bindings/i2c/max6697.txt
> > new file mode 100644
> > index 000..3e867e2
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/i2c/max6697.txt
> > @@ -0,0 +1,45 @@
> > +max6697 properties
> > +
> > +Required properties:
> > +- compatible:
> > + Should be one of
> > + maxim,max6581
> > + maxim,max6602
> > + maxim,max6622
> > + maxim,max6636
> > + maxim,max6689
> > + maxim,max6693
> > + maxim,max6694
> > + maxim,max6697
> > + maxim,max6698
> > + maxim,max6699
> > +- reg: I2C address
> > +
> > +Optional properties:
>
> Your definition of "optional" is questionable. Setting _any_ of these
> properties will cause _all_ others to be considered as 0 and the chip
> will be reprogrammed with these settings. I'd say this is unexpected and
Actually, not just "any", but the values have to be specified if OF data is
present and a non-default configuration is wanted. I clarified that in the
text.
> confusing. I'd expect struct max6697_platform_data to be able to store
> "don't change" for every setting, so that only the properties actually
> provided are written to the chip.
>
> If you really don't want to do that, then you should make it prominently
> visible both here and in max6697.h that one should either set all
> properties or none. Beware though that this may still cause trouble if
> you ever have to add one property to the set in the future.
>
I tried to find a better wording. I do want to set all values, so that part is
on purpose.
> > +- smbus-timeout-disable
> > + Set to enable SMBus timeout
s/enable/disable/
> > +- extended-range-enable
> > + Only valid for MAX6581. Set to enable extended temperature range.
> > +- alert-mask
> > + Alert bit mask. Alert disabled for bits set.
> > +- over-temperature-mask
> > + Over temperature bit mask. Over temperature reporting disabled for
> > + bits set.
> > +- resistance-cancellation
> > + Boolean for all chips other than MAX6581. Enabled if set.
> > + For MAX6581, resistance cancellation enabled for all channels if
> > + specified as boolean, otherwise as per bit mask specified.
> > +- transistor-ideality
> > + For MAX6581 only. Two values; first is bit mask, second is ideality
> > + select value as per MAX6581 data sheet.
>
> I'm not familiar with OF properties... Is there any standard for the
> properties above? If not, and other drivers implement similar
> properties, did you make sure to follow existing common practice?
>
No, not for the optional properties; I did not find anything for those.
I had tried to submit a generic means to configure i2c chips via OF,
but that was rejected even though a similar approach is used for some
PHY chips.
[ ... ]
> > +
> > +static ssize_t show_temp11(struct device *dev,
> > + struct device_attribute *devattr, char *buf)
> > +{
> > + int nr = to_sensor_dev_attr_2(devattr)->nr;
> > + int index = to_sensor_dev_attr_2(devattr)->index;
> > + struct max6697_data *data = max6697_update_device(dev);
> > +
> > + if (IS_ERR(data))
> > + return PTR_ERR(data);
> > +
> > + return sprintf(buf, "%d\n",
> > + ((data->temp[nr][index] << 3) |
> > + (data->temp[nr][index + 1] >> 5)) * 125);
>
> I can't see this code dealing properly with the negative temperature
> values supported by th
Re: [PATCH v2] hwmon: Driver for Maxim MAX6697 and compatibles
Hi Guenter,
Sorry for the late review, originally I planned to do a quick review
but apparently I am simply unable to do that. So here comes a complete
review. As usual, pick what you agree with and feel free to ignore the
rest :)
On Sun, 16 Dec 2012 21:33:09 -0800, Guenter Roeck wrote:
> Add support for MAX6581, MAX6602, MAX6622, MAX6636, MAX6689, MAX6693,
> MAX6694, MAX6697, MAX6698, and MAX6699 temperature sensors
>
> Signed-off-by: Guenter Roeck
> ---
> v2:
> - Add suppport for platform data and devicetree based chip initialization
> - Drop S_IRUGOWU macro: s/S_IRUGOWU/S_IRUGO | S_IWUSR/
>
> Documentation/devicetree/bindings/i2c/max6697.txt | 45 ++
> Documentation/hwmon/max6697 | 57 ++
> drivers/hwmon/Kconfig | 11 +
> drivers/hwmon/Makefile|1 +
> drivers/hwmon/max6697.c | 634
> +
> include/linux/platform_data/max6697.h | 25 +
> 6 files changed, 773 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/i2c/max6697.txt
> create mode 100644 Documentation/hwmon/max6697
> create mode 100644 drivers/hwmon/max6697.c
> create mode 100644 include/linux/platform_data/max6697.h
>
> diff --git a/Documentation/devicetree/bindings/i2c/max6697.txt
> b/Documentation/devicetree/bindings/i2c/max6697.txt
> new file mode 100644
> index 000..3e867e2
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/i2c/max6697.txt
> @@ -0,0 +1,45 @@
> +max6697 properties
> +
> +Required properties:
> +- compatible:
> + Should be one of
> + maxim,max6581
> + maxim,max6602
> + maxim,max6622
> + maxim,max6636
> + maxim,max6689
> + maxim,max6693
> + maxim,max6694
> + maxim,max6697
> + maxim,max6698
> + maxim,max6699
> +- reg: I2C address
> +
> +Optional properties:
Your definition of "optional" is questionable. Setting _any_ of these
properties will cause _all_ others to be considered as 0 and the chip
will be reprogrammed with these settings. I'd say this is unexpected and
confusing. I'd expect struct max6697_platform_data to be able to store
"don't change" for every setting, so that only the properties actually
provided are written to the chip.
If you really don't want to do that, then you should make it prominently
visible both here and in max6697.h that one should either set all
properties or none. Beware though that this may still cause trouble if
you ever have to add one property to the set in the future.
> +- smbus-timeout-disable
> + Set to enable SMBus timeout
> +- extended-range-enable
> + Only valid for MAX6581. Set to enable extended temperature range.
> +- alert-mask
> + Alert bit mask. Alert disabled for bits set.
> +- over-temperature-mask
> + Over temperature bit mask. Over temperature reporting disabled for
> + bits set.
> +- resistance-cancellation
> + Boolean for all chips other than MAX6581. Enabled if set.
> + For MAX6581, resistance cancellation enabled for all channels if
> + specified as boolean, otherwise as per bit mask specified.
> +- transistor-ideality
> + For MAX6581 only. Two values; first is bit mask, second is ideality
> + select value as per MAX6581 data sheet.
I'm not familiar with OF properties... Is there any standard for the
properties above? If not, and other drivers implement similar
properties, did you make sure to follow existing common practice?
> +
> +Example:
> +
> +temp-sensor@1a {
> + compatible = "maxim,max6697";
> + reg = <0x1a>;
> + smbus-timeout-disable;
> + resistance-cancellation;
> + alert-mask = <0xff>;
> + over-temperature-mask = <0xff>;
> +};
For a 7-channel chip, mask values of 0xff make little sense IMHO.
> diff --git a/Documentation/hwmon/max6697 b/Documentation/hwmon/max6697
> new file mode 100644
> index 000..35fc2e9
> --- /dev/null
> +++ b/Documentation/hwmon/max6697
> @@ -0,0 +1,57 @@
> +Kernel driver max6697
> +=
> +
> +Supported chips:
> + * Maxim MAX6581
> +Prefix: 'max6581'
> +Datasheet: http://datasheets.maximintegrated.com/en/ds/MAX6581.pdf
> + * Maxim MAX6602
> +Prefix: 'max6602'
> +Datasheet: http://datasheets.maximintegrated.com/en/ds/MAX6602.pdf
> + * Maxim MAX6622
> +Prefix: 'max6622'
> +Datasheet: http://datasheets.maximintegrated.com/en/ds/MAX6622.pdf
> + * Maxim MAX6636
> +Prefix: 'max6636'
> +Datasheet: http://datasheets.maximintegrated.com/en/ds/MAX6636.pdf
> + * Maxim MAX6689
> +Prefix: 'max6689'
> +Datasheet: http://datasheets.maximintegrated.com/en/ds/MAX6689.pdf
> + * Maxim MAX6693
> +Prefix: 'max6693'
> +Datasheet: http://datasheets.maximintegrated.com/en/ds/MAX6693.pdf
> + * Maxim MAX6694
> +Prefix: 'max6694'
> +Datasheet: http://datasheets.maximintegrated.com/en/ds/MAX6694.pdf
> + * Maxim
Re: [PATCH v2] hwmon: Driver for Maxim MAX6697 and compatibles
Hi Guenter, On Fri, 11 Jan 2013 09:41:05 -0800, Guenter Roeck wrote: > On Sun, Dec 16, 2012 at 09:33:09PM -0800, Guenter Roeck wrote: > > Add support for MAX6581, MAX6602, MAX6622, MAX6636, MAX6689, MAX6693, > > MAX6694, MAX6697, MAX6698, and MAX6699 temperature sensors > > > > Signed-off-by: Guenter Roeck > > I plan to line this driver up for 3.9. Only change from the code below is to > replace SENSORS_LIMIT with clamp_val. Any objections, make yourself heard. I am in the process of reviewing this driver, I do have some comments and hope to be done by tomorrow evening. -- Jean Delvare ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH v2] hwmon: Driver for Maxim MAX6697 and compatibles
On Sun, Dec 16, 2012 at 09:33:09PM -0800, Guenter Roeck wrote:
> Add support for MAX6581, MAX6602, MAX6622, MAX6636, MAX6689, MAX6693,
> MAX6694, MAX6697, MAX6698, and MAX6699 temperature sensors
>
> Signed-off-by: Guenter Roeck
I plan to line this driver up for 3.9. Only change from the code below is to
replace SENSORS_LIMIT with clamp_val. Any objections, make yourself heard.
Thanks,
Guenter
> ---
> v2:
> - Add suppport for platform data and devicetree based chip initialization
> - Drop S_IRUGOWU macro: s/S_IRUGOWU/S_IRUGO | S_IWUSR/
>
> Documentation/devicetree/bindings/i2c/max6697.txt | 45 ++
> Documentation/hwmon/max6697 | 57 ++
> drivers/hwmon/Kconfig | 11 +
> drivers/hwmon/Makefile|1 +
> drivers/hwmon/max6697.c | 634
> +
> include/linux/platform_data/max6697.h | 25 +
> 6 files changed, 773 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/i2c/max6697.txt
> create mode 100644 Documentation/hwmon/max6697
> create mode 100644 drivers/hwmon/max6697.c
> create mode 100644 include/linux/platform_data/max6697.h
>
> diff --git a/Documentation/devicetree/bindings/i2c/max6697.txt
> b/Documentation/devicetree/bindings/i2c/max6697.txt
> new file mode 100644
> index 000..3e867e2
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/i2c/max6697.txt
> @@ -0,0 +1,45 @@
> +max6697 properties
> +
> +Required properties:
> +- compatible:
> + Should be one of
> + maxim,max6581
> + maxim,max6602
> + maxim,max6622
> + maxim,max6636
> + maxim,max6689
> + maxim,max6693
> + maxim,max6694
> + maxim,max6697
> + maxim,max6698
> + maxim,max6699
> +- reg: I2C address
> +
> +Optional properties:
> +- smbus-timeout-disable
> + Set to enable SMBus timeout
> +- extended-range-enable
> + Only valid for MAX6581. Set to enable extended temperature range.
> +- alert-mask
> + Alert bit mask. Alert disabled for bits set.
> +- over-temperature-mask
> + Over temperature bit mask. Over temperature reporting disabled for
> + bits set.
> +- resistance-cancellation
> + Boolean for all chips other than MAX6581. Enabled if set.
> + For MAX6581, resistance cancellation enabled for all channels if
> + specified as boolean, otherwise as per bit mask specified.
> +- transistor-ideality
> + For MAX6581 only. Two values; first is bit mask, second is ideality
> + select value as per MAX6581 data sheet.
> +
> +Example:
> +
> +temp-sensor@1a {
> + compatible = "maxim,max6697";
> + reg = <0x1a>;
> + smbus-timeout-disable;
> + resistance-cancellation;
> + alert-mask = <0xff>;
> + over-temperature-mask = <0xff>;
> +};
> diff --git a/Documentation/hwmon/max6697 b/Documentation/hwmon/max6697
> new file mode 100644
> index 000..35fc2e9
> --- /dev/null
> +++ b/Documentation/hwmon/max6697
> @@ -0,0 +1,57 @@
> +Kernel driver max6697
> +=
> +
> +Supported chips:
> + * Maxim MAX6581
> +Prefix: 'max6581'
> +Datasheet: http://datasheets.maximintegrated.com/en/ds/MAX6581.pdf
> + * Maxim MAX6602
> +Prefix: 'max6602'
> +Datasheet: http://datasheets.maximintegrated.com/en/ds/MAX6602.pdf
> + * Maxim MAX6622
> +Prefix: 'max6622'
> +Datasheet: http://datasheets.maximintegrated.com/en/ds/MAX6622.pdf
> + * Maxim MAX6636
> +Prefix: 'max6636'
> +Datasheet: http://datasheets.maximintegrated.com/en/ds/MAX6636.pdf
> + * Maxim MAX6689
> +Prefix: 'max6689'
> +Datasheet: http://datasheets.maximintegrated.com/en/ds/MAX6689.pdf
> + * Maxim MAX6693
> +Prefix: 'max6693'
> +Datasheet: http://datasheets.maximintegrated.com/en/ds/MAX6693.pdf
> + * Maxim MAX6694
> +Prefix: 'max6694'
> +Datasheet: http://datasheets.maximintegrated.com/en/ds/MAX6694.pdf
> + * Maxim MAX6697
> +Prefix: 'max6697'
> +Datasheet: http://datasheets.maximintegrated.com/en/ds/MAX6697.pdf
> + * Maxim MAX6698
> +Prefix: 'max6698'
> +Datasheet: http://datasheets.maximintegrated.com/en/ds/MAX6698.pdf
> + * Maxim MAX6699
> +Prefix: 'max6699'
> +Datasheet: http://datasheets.maximintegrated.com/en/ds/MAX6699.pdf
> +
> +Author:
> +Guenter Roeck
> +
> +Description
> +---
> +
> +This driver implements support for several MAX6697 compatible temperature
> sensor
> +chips. The chips support one local temperature sensor plus four or six remote
> +temperature sensors. Remote temperature sensors are diode-connected thermal
> +transitors, except for MAX6698 which supports three diode-connected thermal
> +transistors plus three thermistors in addition to the local temperature
> sensor.
> +
> +The driver provides the following sysfs attributes. temp1 is the local (chip)
> +temperature, temp[2..n] are remote temperatures. The actually s
[PATCH v2] hwmon: Driver for Maxim MAX6697 and compatibles
Add support for MAX6581, MAX6602, MAX6622, MAX6636, MAX6689, MAX6693,
MAX6694, MAX6697, MAX6698, and MAX6699 temperature sensors
Signed-off-by: Guenter Roeck
---
v2:
- Add suppport for platform data and devicetree based chip initialization
- Drop S_IRUGOWU macro: s/S_IRUGOWU/S_IRUGO | S_IWUSR/
Documentation/devicetree/bindings/i2c/max6697.txt | 45 ++
Documentation/hwmon/max6697 | 57 ++
drivers/hwmon/Kconfig | 11 +
drivers/hwmon/Makefile|1 +
drivers/hwmon/max6697.c | 634 +
include/linux/platform_data/max6697.h | 25 +
6 files changed, 773 insertions(+)
create mode 100644 Documentation/devicetree/bindings/i2c/max6697.txt
create mode 100644 Documentation/hwmon/max6697
create mode 100644 drivers/hwmon/max6697.c
create mode 100644 include/linux/platform_data/max6697.h
diff --git a/Documentation/devicetree/bindings/i2c/max6697.txt
b/Documentation/devicetree/bindings/i2c/max6697.txt
new file mode 100644
index 000..3e867e2
--- /dev/null
+++ b/Documentation/devicetree/bindings/i2c/max6697.txt
@@ -0,0 +1,45 @@
+max6697 properties
+
+Required properties:
+- compatible:
+ Should be one of
+ maxim,max6581
+ maxim,max6602
+ maxim,max6622
+ maxim,max6636
+ maxim,max6689
+ maxim,max6693
+ maxim,max6694
+ maxim,max6697
+ maxim,max6698
+ maxim,max6699
+- reg: I2C address
+
+Optional properties:
+- smbus-timeout-disable
+ Set to enable SMBus timeout
+- extended-range-enable
+ Only valid for MAX6581. Set to enable extended temperature range.
+- alert-mask
+ Alert bit mask. Alert disabled for bits set.
+- over-temperature-mask
+ Over temperature bit mask. Over temperature reporting disabled for
+ bits set.
+- resistance-cancellation
+ Boolean for all chips other than MAX6581. Enabled if set.
+ For MAX6581, resistance cancellation enabled for all channels if
+ specified as boolean, otherwise as per bit mask specified.
+- transistor-ideality
+ For MAX6581 only. Two values; first is bit mask, second is ideality
+ select value as per MAX6581 data sheet.
+
+Example:
+
+temp-sensor@1a {
+ compatible = "maxim,max6697";
+ reg = <0x1a>;
+ smbus-timeout-disable;
+ resistance-cancellation;
+ alert-mask = <0xff>;
+ over-temperature-mask = <0xff>;
+};
diff --git a/Documentation/hwmon/max6697 b/Documentation/hwmon/max6697
new file mode 100644
index 000..35fc2e9
--- /dev/null
+++ b/Documentation/hwmon/max6697
@@ -0,0 +1,57 @@
+Kernel driver max6697
+=
+
+Supported chips:
+ * Maxim MAX6581
+Prefix: 'max6581'
+Datasheet: http://datasheets.maximintegrated.com/en/ds/MAX6581.pdf
+ * Maxim MAX6602
+Prefix: 'max6602'
+Datasheet: http://datasheets.maximintegrated.com/en/ds/MAX6602.pdf
+ * Maxim MAX6622
+Prefix: 'max6622'
+Datasheet: http://datasheets.maximintegrated.com/en/ds/MAX6622.pdf
+ * Maxim MAX6636
+Prefix: 'max6636'
+Datasheet: http://datasheets.maximintegrated.com/en/ds/MAX6636.pdf
+ * Maxim MAX6689
+Prefix: 'max6689'
+Datasheet: http://datasheets.maximintegrated.com/en/ds/MAX6689.pdf
+ * Maxim MAX6693
+Prefix: 'max6693'
+Datasheet: http://datasheets.maximintegrated.com/en/ds/MAX6693.pdf
+ * Maxim MAX6694
+Prefix: 'max6694'
+Datasheet: http://datasheets.maximintegrated.com/en/ds/MAX6694.pdf
+ * Maxim MAX6697
+Prefix: 'max6697'
+Datasheet: http://datasheets.maximintegrated.com/en/ds/MAX6697.pdf
+ * Maxim MAX6698
+Prefix: 'max6698'
+Datasheet: http://datasheets.maximintegrated.com/en/ds/MAX6698.pdf
+ * Maxim MAX6699
+Prefix: 'max6699'
+Datasheet: http://datasheets.maximintegrated.com/en/ds/MAX6699.pdf
+
+Author:
+Guenter Roeck
+
+Description
+---
+
+This driver implements support for several MAX6697 compatible temperature
sensor
+chips. The chips support one local temperature sensor plus four or six remote
+temperature sensors. Remote temperature sensors are diode-connected thermal
+transitors, except for MAX6698 which supports three diode-connected thermal
+transistors plus three thermistors in addition to the local temperature sensor.
+
+The driver provides the following sysfs attributes. temp1 is the local (chip)
+temperature, temp[2..n] are remote temperatures. The actually supported
+per-channel attributes are chip type and channel dependent.
+
+tempX_input ro remote temperature
+tempX_maxrw remote temperature maximum threshold for alarm
+tempX_max_alarm ro remote temperature maximum threshold alarm
+tempX_crit rw remote temperature critical threshold for alarm
+tempX_crit_alarm ro remote temperature critical threshold alarm
+tempX_fault ro remote temperature diode fault
diff --git a/d
