* [PATCH 0/2] hwmon: adc128d818: Support operation mode 1
@ 2016-03-29 19:03 Alexander Koch
2016-03-29 19:03 ` [PATCH 1/2] hwmon: adc128d818: Implement chip mode selection Alexander Koch
` (2 more replies)
0 siblings, 3 replies; 8+ messages in thread
From: Alexander Koch @ 2016-03-29 19:03 UTC (permalink / raw)
To: Jean Delvare
Cc: Guenter Roeck, lm-sensors, linux-kernel, Michael Hornung, Alexander Koch
The ADC128D818 offers four different chip operation modes which vary in the
number and measurement types of the available input signals (see datasheet
sec. 8.4.1).
The current version of the driver only supports the default chip operation mode
(mode 0), providing seven analog values and a temperature reading.
This patch series adds support for operation mode 1 (eight analog inputs but no
temperature), selectable through the device tree attribute 'mode':
adc1: adc128d818@1d {
compatible = "ti,adc128d818";
reg = <0x1d>;
mode = /bits/ 8 <1>;
};
The changes are transparent as 'mode' defaults to mode 0 which yields the
previous behaviour. Code has been tested on real hardware using an evaluation
board.
Alexander Koch (2):
hwmon: adc128d818: Implement chip mode selection
hwmon: adc128d818: Implement operation mode 1
drivers/hwmon/adc128d818.c | 113 +++++++++++++++++++++++++++++++++++++--------
1 file changed, 95 insertions(+), 18 deletions(-)
--
2.7.4
^ permalink raw reply [flat|nested] 8+ messages in thread* [PATCH 1/2] hwmon: adc128d818: Implement chip mode selection 2016-03-29 19:03 [PATCH 0/2] hwmon: adc128d818: Support operation mode 1 Alexander Koch @ 2016-03-29 19:03 ` Alexander Koch 2016-03-29 19:53 ` Guenter Roeck 2016-03-29 19:03 ` [PATCH 2/2] hwmon: adc128d818: Implement operation mode 1 Alexander Koch 2016-03-29 20:02 ` [PATCH 0/2] hwmon: adc128d818: Support " Guenter Roeck 2 siblings, 1 reply; 8+ messages in thread From: Alexander Koch @ 2016-03-29 19:03 UTC (permalink / raw) To: Jean Delvare Cc: Guenter Roeck, lm-sensors, linux-kernel, Michael Hornung, Alexander Koch The ADC128D818 offers four operation modes (see datasheet sec. 8.4.1) which vary in the number of available input signals and their types (differential vs. absolute). The current version of this driver only implements mode 0, this patch prepares implementation of the others. * Introduce device tree property 'mode' for selection of chip operation mode, default to mode 0 * Extend device data structure to cover all eight input channels Example of 'mode' usage: adc1: adc128d818@1d { compatible = "ti,adc128d818"; reg = <0x1d>; mode = /bits/ 8 <0>; }; Signed-off-by: Alexander Koch <mail@alexanderkoch.net> Acked-by: Michael Hornung <mhornung.linux@gmail.com> --- drivers/hwmon/adc128d818.c | 67 +++++++++++++++++++++++++++++++++------------- 1 file changed, 49 insertions(+), 18 deletions(-) diff --git a/drivers/hwmon/adc128d818.c b/drivers/hwmon/adc128d818.c index ad2b47e..7505d4e 100644 --- a/drivers/hwmon/adc128d818.c +++ b/drivers/hwmon/adc128d818.c @@ -28,6 +28,7 @@ #include <linux/regulator/consumer.h> #include <linux/mutex.h> #include <linux/bitops.h> +#include <linux/of.h> /* Addresses to scan * The chip also supports addresses 0x35..0x37. Don't scan those addresses @@ -63,10 +64,11 @@ struct adc128_data { struct regulator *regulator; int vref; /* Reference voltage in mV */ struct mutex update_lock; + u8 mode; /* Operation mode */ bool valid; /* true if following fields are valid */ unsigned long last_updated; /* In jiffies */ - u16 in[3][7]; /* Register value, normalized to 12 bit + u16 in[3][8]; /* Register value, normalized to 12 bit * 0: input voltage * 1: min limit * 2: max limit @@ -82,12 +84,14 @@ static struct adc128_data *adc128_update_device(struct device *dev) struct adc128_data *data = dev_get_drvdata(dev); struct i2c_client *client = data->client; struct adc128_data *ret = data; - int i, rv; + int i, rv, no_in; + + no_in = (data->mode == 1 || data->mode == 2) ? 8 : 7; mutex_lock(&data->update_lock); if (time_after(jiffies, data->last_updated + HZ) || !data->valid) { - for (i = 0; i < 7; i++) { + for (i = 0; i < no_in; i++) { rv = i2c_smbus_read_word_swapped(client, ADC128_REG_IN(i)); if (rv < 0) @@ -107,20 +111,25 @@ static struct adc128_data *adc128_update_device(struct device *dev) data->in[2][i] = rv << 4; } - rv = i2c_smbus_read_word_swapped(client, ADC128_REG_TEMP); - if (rv < 0) - goto abort; - data->temp[0] = rv >> 7; + if (data->mode != 1) { + rv = i2c_smbus_read_word_swapped(client, + ADC128_REG_TEMP); + if (rv < 0) + goto abort; + data->temp[0] = rv >> 7; - rv = i2c_smbus_read_byte_data(client, ADC128_REG_TEMP_MAX); - if (rv < 0) - goto abort; - data->temp[1] = rv << 1; + rv = i2c_smbus_read_byte_data(client, + ADC128_REG_TEMP_MAX); + if (rv < 0) + goto abort; + data->temp[1] = rv << 1; - rv = i2c_smbus_read_byte_data(client, ADC128_REG_TEMP_HYST); - if (rv < 0) - goto abort; - data->temp[2] = rv << 1; + rv = i2c_smbus_read_byte_data(client, + ADC128_REG_TEMP_HYST); + if (rv < 0) + goto abort; + data->temp[2] = rv << 1; + } rv = i2c_smbus_read_byte_data(client, ADC128_REG_ALARM); if (rv < 0) @@ -304,7 +313,7 @@ static SENSOR_DEVICE_ATTR(in5_alarm, S_IRUGO, adc128_show_alarm, NULL, 5); static SENSOR_DEVICE_ATTR(in6_alarm, S_IRUGO, adc128_show_alarm, NULL, 6); static SENSOR_DEVICE_ATTR(temp1_max_alarm, S_IRUGO, adc128_show_alarm, NULL, 7); -static struct attribute *adc128_attrs[] = { +static struct attribute *adc128m0_attrs[] = { &sensor_dev_attr_in0_min.dev_attr.attr, &sensor_dev_attr_in1_min.dev_attr.attr, &sensor_dev_attr_in2_min.dev_attr.attr, @@ -339,7 +348,7 @@ static struct attribute *adc128_attrs[] = { &sensor_dev_attr_temp1_max_alarm.dev_attr.attr, NULL }; -ATTRIBUTE_GROUPS(adc128); +ATTRIBUTE_GROUPS(adc128m0); static int adc128_detect(struct i2c_client *client, struct i2c_board_info *info) { @@ -387,6 +396,15 @@ static int adc128_init_client(struct adc128_data *data) if (err) return err; + /* Set operation mode, if non-default */ + if (data->mode != 0) { + err = i2c_smbus_write_byte_data(client, + ADC128_REG_CONFIG_ADV, + data->mode << 1); + if (err) + return err; + } + /* Start monitoring */ err = i2c_smbus_write_byte_data(client, ADC128_REG_CONFIG, 0x01); if (err) @@ -410,6 +428,7 @@ static int adc128_probe(struct i2c_client *client, struct regulator *regulator; struct device *hwmon_dev; struct adc128_data *data; + const struct attribute_group **groups; int err, vref; data = devm_kzalloc(dev, sizeof(struct adc128_data), GFP_KERNEL); @@ -433,6 +452,18 @@ static int adc128_probe(struct i2c_client *client, data->vref = 2560; /* 2.56V, in mV */ } + /* Operation mode is optional and defaults to mode 0 */ + data->mode = 0; + groups = adc128m0_groups; + if (of_property_read_u8(dev->of_node, "mode", &data->mode) == 0) { + if (data->mode != 0) { + dev_err(dev, "unsupported operation mode %d", + data->mode); + err = -EINVAL; + goto error; + } + } + data->client = client; i2c_set_clientdata(client, data); mutex_init(&data->update_lock); @@ -443,7 +474,7 @@ static int adc128_probe(struct i2c_client *client, goto error; hwmon_dev = devm_hwmon_device_register_with_groups(dev, client->name, - data, adc128_groups); + data, groups); if (IS_ERR(hwmon_dev)) { err = PTR_ERR(hwmon_dev); goto error; -- 2.7.4 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] hwmon: adc128d818: Implement chip mode selection 2016-03-29 19:03 ` [PATCH 1/2] hwmon: adc128d818: Implement chip mode selection Alexander Koch @ 2016-03-29 19:53 ` Guenter Roeck 2016-12-22 13:02 ` Alexander Koch 0 siblings, 1 reply; 8+ messages in thread From: Guenter Roeck @ 2016-03-29 19:53 UTC (permalink / raw) To: Alexander Koch; +Cc: Jean Delvare, lm-sensors, linux-kernel, Michael Hornung On Tue, Mar 29, 2016 at 09:03:38PM +0200, Alexander Koch wrote: > The ADC128D818 offers four operation modes (see datasheet sec. 8.4.1) > which vary in the number of available input signals and their types > (differential vs. absolute). > > The current version of this driver only implements mode 0, this patch > prepares implementation of the others. > > * Introduce device tree property 'mode' for selection of chip operation > mode, default to mode 0 > > * Extend device data structure to cover all eight input channels > > Example of 'mode' usage: > > adc1: adc128d818@1d { > compatible = "ti,adc128d818"; > reg = <0x1d>; > mode = /bits/ 8 <0>; This will require devicetree maintainer review (and bindings document). The property will probably require a ti, prefix. The bindings document also needs to list the valid modes (all of them, not only the ones supported by the driver). > }; > > Signed-off-by: Alexander Koch <mail@alexanderkoch.net> > Acked-by: Michael Hornung <mhornung.linux@gmail.com> > --- > drivers/hwmon/adc128d818.c | 67 +++++++++++++++++++++++++++++++++------------- > 1 file changed, 49 insertions(+), 18 deletions(-) > > diff --git a/drivers/hwmon/adc128d818.c b/drivers/hwmon/adc128d818.c > index ad2b47e..7505d4e 100644 > --- a/drivers/hwmon/adc128d818.c > +++ b/drivers/hwmon/adc128d818.c > @@ -28,6 +28,7 @@ > #include <linux/regulator/consumer.h> > #include <linux/mutex.h> > #include <linux/bitops.h> > +#include <linux/of.h> > > /* Addresses to scan > * The chip also supports addresses 0x35..0x37. Don't scan those addresses > @@ -63,10 +64,11 @@ struct adc128_data { > struct regulator *regulator; > int vref; /* Reference voltage in mV */ > struct mutex update_lock; > + u8 mode; /* Operation mode */ > bool valid; /* true if following fields are valid */ > unsigned long last_updated; /* In jiffies */ > > - u16 in[3][7]; /* Register value, normalized to 12 bit > + u16 in[3][8]; /* Register value, normalized to 12 bit > * 0: input voltage > * 1: min limit > * 2: max limit > @@ -82,12 +84,14 @@ static struct adc128_data *adc128_update_device(struct device *dev) > struct adc128_data *data = dev_get_drvdata(dev); > struct i2c_client *client = data->client; > struct adc128_data *ret = data; > - int i, rv; > + int i, rv, no_in; > + > + no_in = (data->mode == 1 || data->mode == 2) ? 8 : 7; > mode=2 has four voltages plus temperature sensor. > mutex_lock(&data->update_lock); > > if (time_after(jiffies, data->last_updated + HZ) || !data->valid) { > - for (i = 0; i < 7; i++) { > + for (i = 0; i < no_in; i++) { > rv = i2c_smbus_read_word_swapped(client, > ADC128_REG_IN(i)); > if (rv < 0) > @@ -107,20 +111,25 @@ static struct adc128_data *adc128_update_device(struct device *dev) > data->in[2][i] = rv << 4; > } > > - rv = i2c_smbus_read_word_swapped(client, ADC128_REG_TEMP); > - if (rv < 0) > - goto abort; > - data->temp[0] = rv >> 7; > + if (data->mode != 1) { > + rv = i2c_smbus_read_word_swapped(client, > + ADC128_REG_TEMP); > + if (rv < 0) > + goto abort; > + data->temp[0] = rv >> 7; > > - rv = i2c_smbus_read_byte_data(client, ADC128_REG_TEMP_MAX); > - if (rv < 0) > - goto abort; > - data->temp[1] = rv << 1; > + rv = i2c_smbus_read_byte_data(client, > + ADC128_REG_TEMP_MAX); > + if (rv < 0) > + goto abort; > + data->temp[1] = rv << 1; > > - rv = i2c_smbus_read_byte_data(client, ADC128_REG_TEMP_HYST); > - if (rv < 0) > - goto abort; > - data->temp[2] = rv << 1; > + rv = i2c_smbus_read_byte_data(client, > + ADC128_REG_TEMP_HYST); > + if (rv < 0) > + goto abort; > + data->temp[2] = rv << 1; > + } > > rv = i2c_smbus_read_byte_data(client, ADC128_REG_ALARM); > if (rv < 0) > @@ -304,7 +313,7 @@ static SENSOR_DEVICE_ATTR(in5_alarm, S_IRUGO, adc128_show_alarm, NULL, 5); > static SENSOR_DEVICE_ATTR(in6_alarm, S_IRUGO, adc128_show_alarm, NULL, 6); > static SENSOR_DEVICE_ATTR(temp1_max_alarm, S_IRUGO, adc128_show_alarm, NULL, 7); > > -static struct attribute *adc128_attrs[] = { > +static struct attribute *adc128m0_attrs[] = { > &sensor_dev_attr_in0_min.dev_attr.attr, > &sensor_dev_attr_in1_min.dev_attr.attr, > &sensor_dev_attr_in2_min.dev_attr.attr, > @@ -339,7 +348,7 @@ static struct attribute *adc128_attrs[] = { > &sensor_dev_attr_temp1_max_alarm.dev_attr.attr, > NULL > }; > -ATTRIBUTE_GROUPS(adc128); > +ATTRIBUTE_GROUPS(adc128m0); > > static int adc128_detect(struct i2c_client *client, struct i2c_board_info *info) > { > @@ -387,6 +396,15 @@ static int adc128_init_client(struct adc128_data *data) > if (err) > return err; > > + /* Set operation mode, if non-default */ > + if (data->mode != 0) { > + err = i2c_smbus_write_byte_data(client, > + ADC128_REG_CONFIG_ADV, > + data->mode << 1); > + if (err) > + return err; > + } > + > /* Start monitoring */ > err = i2c_smbus_write_byte_data(client, ADC128_REG_CONFIG, 0x01); > if (err) > @@ -410,6 +428,7 @@ static int adc128_probe(struct i2c_client *client, > struct regulator *regulator; > struct device *hwmon_dev; > struct adc128_data *data; > + const struct attribute_group **groups; > int err, vref; > > data = devm_kzalloc(dev, sizeof(struct adc128_data), GFP_KERNEL); > @@ -433,6 +452,18 @@ static int adc128_probe(struct i2c_client *client, > data->vref = 2560; /* 2.56V, in mV */ > } > > + /* Operation mode is optional and defaults to mode 0 */ > + data->mode = 0; > + groups = adc128m0_groups; > + if (of_property_read_u8(dev->of_node, "mode", &data->mode) == 0) { > + if (data->mode != 0) { > + dev_err(dev, "unsupported operation mode %d", > + data->mode); > + err = -EINVAL; > + goto error; > + } I am sure you are goint to introduce mode support with the next patch, but you also added some mode support in this patch. Patches should be logically consistent, which is not the case here. Most of your changes are not even testable because mode !=0 is rejected here, and some of the changes in this patch don't make sense without the next patch. It might make more sense to introduce the bindings document in the first patch, and the code changes in the second patch. > + } > + > data->client = client; > i2c_set_clientdata(client, data); > mutex_init(&data->update_lock); > @@ -443,7 +474,7 @@ static int adc128_probe(struct i2c_client *client, > goto error; > > hwmon_dev = devm_hwmon_device_register_with_groups(dev, client->name, > - data, adc128_groups); > + data, groups); > if (IS_ERR(hwmon_dev)) { > err = PTR_ERR(hwmon_dev); > goto error; > -- > 2.7.4 > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] hwmon: adc128d818: Implement chip mode selection 2016-03-29 19:53 ` Guenter Roeck @ 2016-12-22 13:02 ` Alexander Koch 2016-12-22 19:09 ` Guenter Roeck 0 siblings, 1 reply; 8+ messages in thread From: Alexander Koch @ 2016-12-22 13:02 UTC (permalink / raw) To: Guenter Roeck Cc: Jean Delvare, lm-sensors, linux-kernel, Michael Hornung, linux-hwmon On Tue, Mar 29, 2016 at 21:53, Guenter Roeck wrote: > On Tue, Mar 29, 2016 at 09:03:38PM +0200, Alexander Koch wrote: >> The ADC128D818 offers four operation modes (see datasheet sec. 8.4.1) >> which vary in the number of available input signals and their types >> (differential vs. absolute). >> >> The current version of this driver only implements mode 0, this patch >> prepares implementation of the others. >> >> * Introduce device tree property 'mode' for selection of chip operation >> mode, default to mode 0 >> >> * Extend device data structure to cover all eight input channels >> >> Example of 'mode' usage: >> >> adc1: adc128d818@1d { >> compatible = "ti,adc128d818"; >> reg = <0x1d>; >> mode = /bits/ 8 <0>; > This will require devicetree maintainer review (and bindings document). > The property will probably require a ti, prefix. The bindings document also > needs to list the valid modes (all of them, not only the ones supported > by the driver). Okay so I will create a bindings document 'devicetree/bindings/hwmon/adc128d818.txt' describing the current properties along with the new 'mode' property. Concerning the prefix I have been a bit indecisive as there are currently examples with (e.g. 'w1/omap-hdq.txt') and without (e.g. 'input/ti,drv260x.txt') that. >> mutex_lock(&data->update_lock); >> >> if (time_after(jiffies, data->last_updated + HZ) || !data->valid) { >> - for (i = 0; i < 7; i++) { >> + for (i = 0; i < no_in; i++) { >> rv = i2c_smbus_read_word_swapped(client, >> ADC128_REG_IN(i)); >> if (rv < 0) >> @@ -107,20 +111,25 @@ static struct adc128_data *adc128_update_device(struct device *dev) >> data->in[2][i] = rv << 4; >> } >> >> - rv = i2c_smbus_read_word_swapped(client, ADC128_REG_TEMP); >> - if (rv < 0) >> - goto abort; >> - data->temp[0] = rv >> 7; >> + if (data->mode != 1) { >> + rv = i2c_smbus_read_word_swapped(client, >> + ADC128_REG_TEMP); >> + if (rv < 0) >> + goto abort; >> + data->temp[0] = rv >> 7; >> >> - rv = i2c_smbus_read_byte_data(client, ADC128_REG_TEMP_MAX); >> - if (rv < 0) >> - goto abort; >> - data->temp[1] = rv << 1; >> + rv = i2c_smbus_read_byte_data(client, >> + ADC128_REG_TEMP_MAX); >> + if (rv < 0) >> + goto abort; >> + data->temp[1] = rv << 1; >> >> - rv = i2c_smbus_read_byte_data(client, ADC128_REG_TEMP_HYST); >> - if (rv < 0) >> - goto abort; >> - data->temp[2] = rv << 1; >> + rv = i2c_smbus_read_byte_data(client, >> + ADC128_REG_TEMP_HYST); >> + if (rv < 0) >> + goto abort; >> + data->temp[2] = rv << 1; >> + } >> >> rv = i2c_smbus_read_byte_data(client, ADC128_REG_ALARM); >> if (rv < 0) >> @@ -304,7 +313,7 @@ static SENSOR_DEVICE_ATTR(in5_alarm, S_IRUGO, adc128_show_alarm, NULL, 5); >> static SENSOR_DEVICE_ATTR(in6_alarm, S_IRUGO, adc128_show_alarm, NULL, 6); >> static SENSOR_DEVICE_ATTR(temp1_max_alarm, S_IRUGO, adc128_show_alarm, NULL, 7); >> >> -static struct attribute *adc128_attrs[] = { >> +static struct attribute *adc128m0_attrs[] = { >> &sensor_dev_attr_in0_min.dev_attr.attr, >> &sensor_dev_attr_in1_min.dev_attr.attr, >> &sensor_dev_attr_in2_min.dev_attr.attr, >> @@ -339,7 +348,7 @@ static struct attribute *adc128_attrs[] = { >> &sensor_dev_attr_temp1_max_alarm.dev_attr.attr, >> NULL >> }; >> -ATTRIBUTE_GROUPS(adc128); >> +ATTRIBUTE_GROUPS(adc128m0); >> >> static int adc128_detect(struct i2c_client *client, struct i2c_board_info *info) >> { >> @@ -387,6 +396,15 @@ static int adc128_init_client(struct adc128_data *data) >> if (err) >> return err; >> >> + /* Set operation mode, if non-default */ >> + if (data->mode != 0) { >> + err = i2c_smbus_write_byte_data(client, >> + ADC128_REG_CONFIG_ADV, >> + data->mode << 1); >> + if (err) >> + return err; >> + } >> + >> /* Start monitoring */ >> err = i2c_smbus_write_byte_data(client, ADC128_REG_CONFIG, 0x01); >> if (err) >> @@ -410,6 +428,7 @@ static int adc128_probe(struct i2c_client *client, >> struct regulator *regulator; >> struct device *hwmon_dev; >> struct adc128_data *data; >> + const struct attribute_group **groups; >> int err, vref; >> >> data = devm_kzalloc(dev, sizeof(struct adc128_data), GFP_KERNEL); >> @@ -433,6 +452,18 @@ static int adc128_probe(struct i2c_client *client, >> data->vref = 2560; /* 2.56V, in mV */ >> } >> >> + /* Operation mode is optional and defaults to mode 0 */ >> + data->mode = 0; >> + groups = adc128m0_groups; >> + if (of_property_read_u8(dev->of_node, "mode", &data->mode) == 0) { >> + if (data->mode != 0) { >> + dev_err(dev, "unsupported operation mode %d", >> + data->mode); >> + err = -EINVAL; >> + goto error; >> + } > I am sure you are goint to introduce mode support with the next patch, but you also > added some mode support in this patch. Patches should be logically consistent, > which is not the case here. Most of your changes are not even testable because > mode !=0 is rejected here, and some of the changes in this patch don't make > sense without the next patch. I'm not sure I got your point there. Taking a step back from my (admittedly crappy) implementation I had the intention of preserving two logical units: one being refactoring to introduce the 'mode' property (but keeping the functional level of the driver, i.e. mode-0-only) and two being the addition of new functionality (mode 1 support). I understand that step 1 does not make much sense without step 2, but what would be the alternative then? Combining both steps in a single patch? I'd consider this a bit too much for a single patch, and separating refactoring from the addition of new functionality is, to my understanding, generally advised (e.g. [1]). Concerning testability, could you please explain why the rejection of mode !=0 renders my changes untestable? This check is performed only if the 'mode' attribute is present in the device tree, so testing the default (no definition at all) is still possible. Would I have to implement all possible values of 'mode' in order to make my changes testable? > It might make more sense to introduce the bindings document in the first patch, > and the code changes in the second patch. In my daily work I am used to committing new functional implementations along with their documentation in a single commit. The reason for this being that the alternative would be two separate patches, and that if someone accidentally checks out the first patch the resulting code base would either have documentation for something that isn't implemented or an undocumented function. Heeding to your advice I assume in Kernel development it is preferred to have changes to documentation and code in separate commits. So what would you think of the following patch set structure? 1) Add device tree binding document (including 'mode') 2) Refactor the code to reflect the 'mode' property (but still support mode 0 only) 3) Implement mode 1 support Another point would be updating 'Documentation/hwmon/adc128d818', which is missing the other operation modes as well. Would you see this as 4) or between 1) and 2)? Kind regards and thank you very much for your detailed feedback Alex [1] https://kernelnewbies.org/PatchPhilosophy ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] hwmon: adc128d818: Implement chip mode selection 2016-12-22 13:02 ` Alexander Koch @ 2016-12-22 19:09 ` Guenter Roeck 0 siblings, 0 replies; 8+ messages in thread From: Guenter Roeck @ 2016-12-22 19:09 UTC (permalink / raw) To: Alexander Koch Cc: Jean Delvare, lm-sensors, linux-kernel, Michael Hornung, linux-hwmon On Thu, Dec 22, 2016 at 02:02:54PM +0100, Alexander Koch wrote: > On Tue, Mar 29, 2016 at 21:53, Guenter Roeck wrote: > > On Tue, Mar 29, 2016 at 09:03:38PM +0200, Alexander Koch wrote: > >> The ADC128D818 offers four operation modes (see datasheet sec. 8.4.1) > >> which vary in the number of available input signals and their types > >> (differential vs. absolute). > >> > >> The current version of this driver only implements mode 0, this patch > >> prepares implementation of the others. > >> > >> * Introduce device tree property 'mode' for selection of chip operation > >> mode, default to mode 0 > >> > >> * Extend device data structure to cover all eight input channels > >> > >> Example of 'mode' usage: > >> > >> adc1: adc128d818@1d { > >> compatible = "ti,adc128d818"; > >> reg = <0x1d>; > >> mode = /bits/ 8 <0>; > > This will require devicetree maintainer review (and bindings document). > > The property will probably require a ti, prefix. The bindings document also > > needs to list the valid modes (all of them, not only the ones supported > > by the driver). > > Okay so I will create a bindings document > 'devicetree/bindings/hwmon/adc128d818.txt' describing the current > properties along with the new 'mode' property. > > Concerning the prefix I have been a bit indecisive as there are > currently examples with (e.g. 'w1/omap-hdq.txt') and without (e.g. > 'input/ti,drv260x.txt') that. > > > >> mutex_lock(&data->update_lock); > >> > >> if (time_after(jiffies, data->last_updated + HZ) || !data->valid) { > >> - for (i = 0; i < 7; i++) { > >> + for (i = 0; i < no_in; i++) { > >> rv = i2c_smbus_read_word_swapped(client, > >> ADC128_REG_IN(i)); > >> if (rv < 0) > >> @@ -107,20 +111,25 @@ static struct adc128_data *adc128_update_device(struct device *dev) > >> data->in[2][i] = rv << 4; > >> } > >> > >> - rv = i2c_smbus_read_word_swapped(client, ADC128_REG_TEMP); > >> - if (rv < 0) > >> - goto abort; > >> - data->temp[0] = rv >> 7; > >> + if (data->mode != 1) { > >> + rv = i2c_smbus_read_word_swapped(client, > >> + ADC128_REG_TEMP); > >> + if (rv < 0) > >> + goto abort; > >> + data->temp[0] = rv >> 7; > >> > >> - rv = i2c_smbus_read_byte_data(client, ADC128_REG_TEMP_MAX); > >> - if (rv < 0) > >> - goto abort; > >> - data->temp[1] = rv << 1; > >> + rv = i2c_smbus_read_byte_data(client, > >> + ADC128_REG_TEMP_MAX); > >> + if (rv < 0) > >> + goto abort; > >> + data->temp[1] = rv << 1; > >> > >> - rv = i2c_smbus_read_byte_data(client, ADC128_REG_TEMP_HYST); > >> - if (rv < 0) > >> - goto abort; > >> - data->temp[2] = rv << 1; > >> + rv = i2c_smbus_read_byte_data(client, > >> + ADC128_REG_TEMP_HYST); > >> + if (rv < 0) > >> + goto abort; > >> + data->temp[2] = rv << 1; > >> + } > >> > >> rv = i2c_smbus_read_byte_data(client, ADC128_REG_ALARM); > >> if (rv < 0) > >> @@ -304,7 +313,7 @@ static SENSOR_DEVICE_ATTR(in5_alarm, S_IRUGO, adc128_show_alarm, NULL, 5); > >> static SENSOR_DEVICE_ATTR(in6_alarm, S_IRUGO, adc128_show_alarm, NULL, 6); > >> static SENSOR_DEVICE_ATTR(temp1_max_alarm, S_IRUGO, adc128_show_alarm, NULL, 7); > >> > >> -static struct attribute *adc128_attrs[] = { > >> +static struct attribute *adc128m0_attrs[] = { > >> &sensor_dev_attr_in0_min.dev_attr.attr, > >> &sensor_dev_attr_in1_min.dev_attr.attr, > >> &sensor_dev_attr_in2_min.dev_attr.attr, > >> @@ -339,7 +348,7 @@ static struct attribute *adc128_attrs[] = { > >> &sensor_dev_attr_temp1_max_alarm.dev_attr.attr, > >> NULL > >> }; > >> -ATTRIBUTE_GROUPS(adc128); > >> +ATTRIBUTE_GROUPS(adc128m0); > >> > >> static int adc128_detect(struct i2c_client *client, struct i2c_board_info *info) > >> { > >> @@ -387,6 +396,15 @@ static int adc128_init_client(struct adc128_data *data) > >> if (err) > >> return err; > >> > >> + /* Set operation mode, if non-default */ > >> + if (data->mode != 0) { > >> + err = i2c_smbus_write_byte_data(client, > >> + ADC128_REG_CONFIG_ADV, > >> + data->mode << 1); > >> + if (err) > >> + return err; > >> + } > >> + > >> /* Start monitoring */ > >> err = i2c_smbus_write_byte_data(client, ADC128_REG_CONFIG, 0x01); > >> if (err) > >> @@ -410,6 +428,7 @@ static int adc128_probe(struct i2c_client *client, > >> struct regulator *regulator; > >> struct device *hwmon_dev; > >> struct adc128_data *data; > >> + const struct attribute_group **groups; > >> int err, vref; > >> > >> data = devm_kzalloc(dev, sizeof(struct adc128_data), GFP_KERNEL); > >> @@ -433,6 +452,18 @@ static int adc128_probe(struct i2c_client *client, > >> data->vref = 2560; /* 2.56V, in mV */ > >> } > >> > >> + /* Operation mode is optional and defaults to mode 0 */ > >> + data->mode = 0; > >> + groups = adc128m0_groups; > >> + if (of_property_read_u8(dev->of_node, "mode", &data->mode) == 0) { > >> + if (data->mode != 0) { > >> + dev_err(dev, "unsupported operation mode %d", > >> + data->mode); > >> + err = -EINVAL; > >> + goto error; > >> + } > > I am sure you are goint to introduce mode support with the next patch, but you also > > added some mode support in this patch. Patches should be logically consistent, > > which is not the case here. Most of your changes are not even testable because > > mode !=0 is rejected here, and some of the changes in this patch don't make > > sense without the next patch. > > I'm not sure I got your point there. Taking a step back from my > (admittedly crappy) implementation I had the intention of preserving two > logical units: one being refactoring to introduce the 'mode' property > (but keeping the functional level of the driver, i.e. mode-0-only) and > two being the addition of new functionality (mode 1 support). > > I understand that step 1 does not make much sense without step 2, but > what would be the alternative then? Combining both steps in a single > patch? I'd consider this a bit too much for a single patch, and > separating refactoring from the addition of new functionality is, to my > understanding, generally advised (e.g. [1]). > > Concerning testability, could you please explain why the rejection of > mode !=0 renders my changes untestable? This check is performed only if > the 'mode' attribute is present in the device tree, so testing the > default (no definition at all) is still possible. Would I have to > implement all possible values of 'mode' in order to make my changes > testable? > > > > It might make more sense to introduce the bindings document in the first patch, > > and the code changes in the second patch. > > In my daily work I am used to committing new functional implementations > along with their documentation in a single commit. The reason for this > being that the alternative would be two separate patches, and that if > someone accidentally checks out the first patch the resulting code base > would either have documentation for something that isn't implemented or > an undocumented function. > > Heeding to your advice I assume in Kernel development it is preferred to > have changes to documentation and code in separate commits. > Specifically for devicetree changes, this is correct and make sense, since the people reviewing and approving devicetree properties in most cases will not review the actual code that results from it. Since both documentation and code are usually pushed into the kernel at the same time, the situation above does not really apply. One would, on purpose, have to check out the SHA which has one but not the other. I would call that self-inflicted damage. Anyone checking out code at a specific SHA has to accept that it is not necessarily "complete" in any sense. Any large patch series will have the documentation patch (if it exists) not 100% in sync with all the other changes being made. Either one ot the other will be out of date. > > So what would you think of the following patch set structure? > > 1) Add device tree binding document (including 'mode') > 2) Refactor the code to reflect the 'mode' property (but still support > mode 0 only) > 3) Implement mode 1 support > Ok with me. > > Another point would be updating 'Documentation/hwmon/adc128d818', which > is missing the other operation modes as well. Would you see this as 4) > or between 1) and 2)? > You can add this to 3). 1) only applies to devicetree changes. Guenter ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 2/2] hwmon: adc128d818: Implement operation mode 1 2016-03-29 19:03 [PATCH 0/2] hwmon: adc128d818: Support operation mode 1 Alexander Koch 2016-03-29 19:03 ` [PATCH 1/2] hwmon: adc128d818: Implement chip mode selection Alexander Koch @ 2016-03-29 19:03 ` Alexander Koch 2016-03-29 20:01 ` Guenter Roeck 2016-03-29 20:02 ` [PATCH 0/2] hwmon: adc128d818: Support " Guenter Roeck 2 siblings, 1 reply; 8+ messages in thread From: Alexander Koch @ 2016-03-29 19:03 UTC (permalink / raw) To: Jean Delvare Cc: Guenter Roeck, lm-sensors, linux-kernel, Michael Hornung, Alexander Koch Implement chip operation mode 1 (see datasheet sec. 8.4.1) which offers an additional analog input signal on channel 7 but no temperature reading. Signed-off-by: Alexander Koch <mail@alexanderkoch.net> Michael Hornung <mhornung.linux@gmail.com> --- drivers/hwmon/adc128d818.c | 48 +++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 47 insertions(+), 1 deletion(-) diff --git a/drivers/hwmon/adc128d818.c b/drivers/hwmon/adc128d818.c index 7505d4e..d29ba36 100644 --- a/drivers/hwmon/adc128d818.c +++ b/drivers/hwmon/adc128d818.c @@ -298,6 +298,13 @@ static SENSOR_DEVICE_ATTR_2(in6_min, S_IWUSR | S_IRUGO, static SENSOR_DEVICE_ATTR_2(in6_max, S_IWUSR | S_IRUGO, adc128_show_in, adc128_set_in, 6, 2); +static SENSOR_DEVICE_ATTR_2(in7_input, S_IRUGO, + adc128_show_in, NULL, 7, 0); +static SENSOR_DEVICE_ATTR_2(in7_min, S_IWUSR | S_IRUGO, + adc128_show_in, adc128_set_in, 7, 1); +static SENSOR_DEVICE_ATTR_2(in7_max, S_IWUSR | S_IRUGO, + adc128_show_in, adc128_set_in, 7, 2); + static SENSOR_DEVICE_ATTR(temp1_input, S_IRUGO, adc128_show_temp, NULL, 0); static SENSOR_DEVICE_ATTR(temp1_max, S_IWUSR | S_IRUGO, adc128_show_temp, adc128_set_temp, 1); @@ -311,6 +318,7 @@ static SENSOR_DEVICE_ATTR(in3_alarm, S_IRUGO, adc128_show_alarm, NULL, 3); static SENSOR_DEVICE_ATTR(in4_alarm, S_IRUGO, adc128_show_alarm, NULL, 4); static SENSOR_DEVICE_ATTR(in5_alarm, S_IRUGO, adc128_show_alarm, NULL, 5); static SENSOR_DEVICE_ATTR(in6_alarm, S_IRUGO, adc128_show_alarm, NULL, 6); +static SENSOR_DEVICE_ATTR(in7_alarm, S_IRUGO, adc128_show_alarm, NULL, 7); static SENSOR_DEVICE_ATTR(temp1_max_alarm, S_IRUGO, adc128_show_alarm, NULL, 7); static struct attribute *adc128m0_attrs[] = { @@ -349,6 +357,42 @@ static struct attribute *adc128m0_attrs[] = { NULL }; ATTRIBUTE_GROUPS(adc128m0); +static struct attribute *adc128m1_attrs[] = { + &sensor_dev_attr_in0_min.dev_attr.attr, + &sensor_dev_attr_in1_min.dev_attr.attr, + &sensor_dev_attr_in2_min.dev_attr.attr, + &sensor_dev_attr_in3_min.dev_attr.attr, + &sensor_dev_attr_in4_min.dev_attr.attr, + &sensor_dev_attr_in5_min.dev_attr.attr, + &sensor_dev_attr_in6_min.dev_attr.attr, + &sensor_dev_attr_in7_min.dev_attr.attr, + &sensor_dev_attr_in0_max.dev_attr.attr, + &sensor_dev_attr_in1_max.dev_attr.attr, + &sensor_dev_attr_in2_max.dev_attr.attr, + &sensor_dev_attr_in3_max.dev_attr.attr, + &sensor_dev_attr_in4_max.dev_attr.attr, + &sensor_dev_attr_in5_max.dev_attr.attr, + &sensor_dev_attr_in6_max.dev_attr.attr, + &sensor_dev_attr_in7_max.dev_attr.attr, + &sensor_dev_attr_in0_input.dev_attr.attr, + &sensor_dev_attr_in1_input.dev_attr.attr, + &sensor_dev_attr_in2_input.dev_attr.attr, + &sensor_dev_attr_in3_input.dev_attr.attr, + &sensor_dev_attr_in4_input.dev_attr.attr, + &sensor_dev_attr_in5_input.dev_attr.attr, + &sensor_dev_attr_in6_input.dev_attr.attr, + &sensor_dev_attr_in7_input.dev_attr.attr, + &sensor_dev_attr_in0_alarm.dev_attr.attr, + &sensor_dev_attr_in1_alarm.dev_attr.attr, + &sensor_dev_attr_in2_alarm.dev_attr.attr, + &sensor_dev_attr_in3_alarm.dev_attr.attr, + &sensor_dev_attr_in4_alarm.dev_attr.attr, + &sensor_dev_attr_in5_alarm.dev_attr.attr, + &sensor_dev_attr_in6_alarm.dev_attr.attr, + &sensor_dev_attr_in7_alarm.dev_attr.attr, + NULL +}; +ATTRIBUTE_GROUPS(adc128m1); static int adc128_detect(struct i2c_client *client, struct i2c_board_info *info) { @@ -456,7 +500,9 @@ static int adc128_probe(struct i2c_client *client, data->mode = 0; groups = adc128m0_groups; if (of_property_read_u8(dev->of_node, "mode", &data->mode) == 0) { - if (data->mode != 0) { + if (data->mode == 1) { + groups = adc128m1_groups; + } else if (data->mode != 0) { dev_err(dev, "unsupported operation mode %d", data->mode); err = -EINVAL; -- 2.7.4 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] hwmon: adc128d818: Implement operation mode 1 2016-03-29 19:03 ` [PATCH 2/2] hwmon: adc128d818: Implement operation mode 1 Alexander Koch @ 2016-03-29 20:01 ` Guenter Roeck 0 siblings, 0 replies; 8+ messages in thread From: Guenter Roeck @ 2016-03-29 20:01 UTC (permalink / raw) To: Alexander Koch; +Cc: Jean Delvare, lm-sensors, linux-kernel, Michael Hornung On Tue, Mar 29, 2016 at 09:03:39PM +0200, Alexander Koch wrote: > Implement chip operation mode 1 (see datasheet sec. 8.4.1) which offers an > additional analog input signal on channel 7 but no temperature reading. > > Signed-off-by: Alexander Koch <mail@alexanderkoch.net> > Michael Hornung <mhornung.linux@gmail.com> > --- > drivers/hwmon/adc128d818.c | 48 +++++++++++++++++++++++++++++++++++++++++++++- > 1 file changed, 47 insertions(+), 1 deletion(-) > > diff --git a/drivers/hwmon/adc128d818.c b/drivers/hwmon/adc128d818.c > index 7505d4e..d29ba36 100644 > --- a/drivers/hwmon/adc128d818.c > +++ b/drivers/hwmon/adc128d818.c > @@ -298,6 +298,13 @@ static SENSOR_DEVICE_ATTR_2(in6_min, S_IWUSR | S_IRUGO, > static SENSOR_DEVICE_ATTR_2(in6_max, S_IWUSR | S_IRUGO, > adc128_show_in, adc128_set_in, 6, 2); > > +static SENSOR_DEVICE_ATTR_2(in7_input, S_IRUGO, > + adc128_show_in, NULL, 7, 0); > +static SENSOR_DEVICE_ATTR_2(in7_min, S_IWUSR | S_IRUGO, > + adc128_show_in, adc128_set_in, 7, 1); > +static SENSOR_DEVICE_ATTR_2(in7_max, S_IWUSR | S_IRUGO, > + adc128_show_in, adc128_set_in, 7, 2); > + > static SENSOR_DEVICE_ATTR(temp1_input, S_IRUGO, adc128_show_temp, NULL, 0); > static SENSOR_DEVICE_ATTR(temp1_max, S_IWUSR | S_IRUGO, > adc128_show_temp, adc128_set_temp, 1); > @@ -311,6 +318,7 @@ static SENSOR_DEVICE_ATTR(in3_alarm, S_IRUGO, adc128_show_alarm, NULL, 3); > static SENSOR_DEVICE_ATTR(in4_alarm, S_IRUGO, adc128_show_alarm, NULL, 4); > static SENSOR_DEVICE_ATTR(in5_alarm, S_IRUGO, adc128_show_alarm, NULL, 5); > static SENSOR_DEVICE_ATTR(in6_alarm, S_IRUGO, adc128_show_alarm, NULL, 6); > +static SENSOR_DEVICE_ATTR(in7_alarm, S_IRUGO, adc128_show_alarm, NULL, 7); > static SENSOR_DEVICE_ATTR(temp1_max_alarm, S_IRUGO, adc128_show_alarm, NULL, 7); > > static struct attribute *adc128m0_attrs[] = { > @@ -349,6 +357,42 @@ static struct attribute *adc128m0_attrs[] = { > NULL > }; > ATTRIBUTE_GROUPS(adc128m0); > +static struct attribute *adc128m1_attrs[] = { > + &sensor_dev_attr_in0_min.dev_attr.attr, > + &sensor_dev_attr_in1_min.dev_attr.attr, > + &sensor_dev_attr_in2_min.dev_attr.attr, > + &sensor_dev_attr_in3_min.dev_attr.attr, > + &sensor_dev_attr_in4_min.dev_attr.attr, > + &sensor_dev_attr_in5_min.dev_attr.attr, > + &sensor_dev_attr_in6_min.dev_attr.attr, > + &sensor_dev_attr_in7_min.dev_attr.attr, > + &sensor_dev_attr_in0_max.dev_attr.attr, > + &sensor_dev_attr_in1_max.dev_attr.attr, > + &sensor_dev_attr_in2_max.dev_attr.attr, > + &sensor_dev_attr_in3_max.dev_attr.attr, > + &sensor_dev_attr_in4_max.dev_attr.attr, > + &sensor_dev_attr_in5_max.dev_attr.attr, > + &sensor_dev_attr_in6_max.dev_attr.attr, > + &sensor_dev_attr_in7_max.dev_attr.attr, > + &sensor_dev_attr_in0_input.dev_attr.attr, > + &sensor_dev_attr_in1_input.dev_attr.attr, > + &sensor_dev_attr_in2_input.dev_attr.attr, > + &sensor_dev_attr_in3_input.dev_attr.attr, > + &sensor_dev_attr_in4_input.dev_attr.attr, > + &sensor_dev_attr_in5_input.dev_attr.attr, > + &sensor_dev_attr_in6_input.dev_attr.attr, > + &sensor_dev_attr_in7_input.dev_attr.attr, > + &sensor_dev_attr_in0_alarm.dev_attr.attr, > + &sensor_dev_attr_in1_alarm.dev_attr.attr, > + &sensor_dev_attr_in2_alarm.dev_attr.attr, > + &sensor_dev_attr_in3_alarm.dev_attr.attr, > + &sensor_dev_attr_in4_alarm.dev_attr.attr, > + &sensor_dev_attr_in5_alarm.dev_attr.attr, > + &sensor_dev_attr_in6_alarm.dev_attr.attr, > + &sensor_dev_attr_in7_alarm.dev_attr.attr, > + NULL > +}; > +ATTRIBUTE_GROUPS(adc128m1); > No, this is not how to do it. You have two options: - define multiple groups, and add groups as needed. For example, you would have one group for in0..in3, one for in4..in5, one for in6, and one for in7, plus one for temp1. This would cover all operational modes. Then use a bitmap for supported voltages in a given mode, and use that bit map to determine if a voltage sensor is supported or not. For temperature you can use a boolean, or declare that the temperature sensor is available if in7 is not available. This way you don't always have to check the operational mode when updating sensor values. - Use a single group and is_visible() to determine is a sensor is visible or not (in combination with the bit map suggested above). This would probably be the simpler solution. Guenter > static int adc128_detect(struct i2c_client *client, struct i2c_board_info *info) > { > @@ -456,7 +500,9 @@ static int adc128_probe(struct i2c_client *client, > data->mode = 0; > groups = adc128m0_groups; > if (of_property_read_u8(dev->of_node, "mode", &data->mode) == 0) { > - if (data->mode != 0) { > + if (data->mode == 1) { > + groups = adc128m1_groups; > + } else if (data->mode != 0) { > dev_err(dev, "unsupported operation mode %d", > data->mode); > err = -EINVAL; > -- > 2.7.4 > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 0/2] hwmon: adc128d818: Support operation mode 1 2016-03-29 19:03 [PATCH 0/2] hwmon: adc128d818: Support operation mode 1 Alexander Koch 2016-03-29 19:03 ` [PATCH 1/2] hwmon: adc128d818: Implement chip mode selection Alexander Koch 2016-03-29 19:03 ` [PATCH 2/2] hwmon: adc128d818: Implement operation mode 1 Alexander Koch @ 2016-03-29 20:02 ` Guenter Roeck 2 siblings, 0 replies; 8+ messages in thread From: Guenter Roeck @ 2016-03-29 20:02 UTC (permalink / raw) To: Alexander Koch; +Cc: Jean Delvare, lm-sensors, linux-kernel, Michael Hornung On Tue, Mar 29, 2016 at 09:03:37PM +0200, Alexander Koch wrote: > The ADC128D818 offers four different chip operation modes which vary in the > number and measurement types of the available input signals (see datasheet > sec. 8.4.1). > > The current version of the driver only supports the default chip operation mode > (mode 0), providing seven analog values and a temperature reading. > > This patch series adds support for operation mode 1 (eight analog inputs but no > temperature), selectable through the device tree attribute 'mode': > > adc1: adc128d818@1d { > compatible = "ti,adc128d818"; > reg = <0x1d>; > mode = /bits/ 8 <1>; > }; > > The changes are transparent as 'mode' defaults to mode 0 which yields the > previous behaviour. Code has been tested on real hardware using an evaluation > board. > > Alexander Koch (2): > hwmon: adc128d818: Implement chip mode selection > hwmon: adc128d818: Implement operation mode 1 > > drivers/hwmon/adc128d818.c | 113 +++++++++++++++++++++++++++++++++++++-------- > 1 file changed, 95 insertions(+), 18 deletions(-) > Please send follow-up patches to linux-hwmon@vger.kernel.org. Thanks, Guenter ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2016-12-22 19:09 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-03-29 19:03 [PATCH 0/2] hwmon: adc128d818: Support operation mode 1 Alexander Koch 2016-03-29 19:03 ` [PATCH 1/2] hwmon: adc128d818: Implement chip mode selection Alexander Koch 2016-03-29 19:53 ` Guenter Roeck 2016-12-22 13:02 ` Alexander Koch 2016-12-22 19:09 ` Guenter Roeck 2016-03-29 19:03 ` [PATCH 2/2] hwmon: adc128d818: Implement operation mode 1 Alexander Koch 2016-03-29 20:01 ` Guenter Roeck 2016-03-29 20:02 ` [PATCH 0/2] hwmon: adc128d818: Support " Guenter Roeck
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).