* [PATCH 0/2] Add an initial DT binding doc for ina3221 @ 2018-09-21 0:07 Nicolin Chen 2018-09-21 0:07 ` [PATCH 1/2] dt-bindings: hwmon: Add ina3221 documentation Nicolin Chen 2018-09-21 0:07 ` [PATCH 2/2] hwmon: ina3221: Get channel names from DT node Nicolin Chen 0 siblings, 2 replies; 10+ messages in thread From: Nicolin Chen @ 2018-09-21 0:07 UTC (permalink / raw) To: jdelvare, linux, robh+dt, mark.rutland Cc: afd, linux-hwmon, devicetree, linux-kernel This series adds a initial DT binding doc for ina3221. It adds channel names (which should be according to board schematics) and implements the corresponding actions (disabling channels) in the driver. Nicolin Chen (2): dt-bindings: hwmon: Add ina3221 documentation hwmon: ina3221: Get channel names from DT node .../devicetree/bindings/hwmon/ina3221.txt | 23 +++++ drivers/hwmon/ina3221.c | 88 +++++++++++++++++-- 2 files changed, 103 insertions(+), 8 deletions(-) create mode 100644 Documentation/devicetree/bindings/hwmon/ina3221.txt -- 2.17.1 ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 1/2] dt-bindings: hwmon: Add ina3221 documentation 2018-09-21 0:07 [PATCH 0/2] Add an initial DT binding doc for ina3221 Nicolin Chen @ 2018-09-21 0:07 ` Nicolin Chen 2018-09-21 0:45 ` Guenter Roeck 2018-09-21 0:07 ` [PATCH 2/2] hwmon: ina3221: Get channel names from DT node Nicolin Chen 1 sibling, 1 reply; 10+ messages in thread From: Nicolin Chen @ 2018-09-21 0:07 UTC (permalink / raw) To: jdelvare, linux, robh+dt, mark.rutland Cc: afd, linux-hwmon, devicetree, linux-kernel Texas Instruments INA3221 is a triple-channel shunt and bus voltage monitor. This patch adds a DT binding doc for it. Signed-off-by: Nicolin Chen <nicoleotsuka@gmail.com> --- .../devicetree/bindings/hwmon/ina3221.txt | 23 +++++++++++++++++++ 1 file changed, 23 insertions(+) create mode 100644 Documentation/devicetree/bindings/hwmon/ina3221.txt diff --git a/Documentation/devicetree/bindings/hwmon/ina3221.txt b/Documentation/devicetree/bindings/hwmon/ina3221.txt new file mode 100644 index 000000000000..266c9586c9b1 --- /dev/null +++ b/Documentation/devicetree/bindings/hwmon/ina3221.txt @@ -0,0 +1,23 @@ +ina3221 properties + +Required properties: +- compatible: Must be "ti,ina3221" +- reg: I2C address + +Optional properties: + +- ti,channel1-name: +- ti,channel2-name: +- ti,channel3-name: + The names of the input sources (described in the schematics) + Set the names with "NC" to indicate not-connected channels + +Example: + +ina3221@40 { + compatible = "ti,ina3221"; + reg = <0x40>; + ti,channel1-name = "NC"; + ti,channel2-name = "VDD_5V0_EXT"; + ti,channel3-name = "VDD_19V"; +}; -- 2.17.1 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] dt-bindings: hwmon: Add ina3221 documentation 2018-09-21 0:07 ` [PATCH 1/2] dt-bindings: hwmon: Add ina3221 documentation Nicolin Chen @ 2018-09-21 0:45 ` Guenter Roeck 2018-09-21 1:24 ` Nicolin Chen 0 siblings, 1 reply; 10+ messages in thread From: Guenter Roeck @ 2018-09-21 0:45 UTC (permalink / raw) To: Nicolin Chen, jdelvare, robh+dt, mark.rutland Cc: afd, linux-hwmon, devicetree, linux-kernel On 09/20/2018 05:07 PM, Nicolin Chen wrote: > Texas Instruments INA3221 is a triple-channel shunt and bus > voltage monitor. This patch adds a DT binding doc for it. > > Signed-off-by: Nicolin Chen <nicoleotsuka@gmail.com> > --- > .../devicetree/bindings/hwmon/ina3221.txt | 23 +++++++++++++++++++ > 1 file changed, 23 insertions(+) > create mode 100644 Documentation/devicetree/bindings/hwmon/ina3221.txt > > diff --git a/Documentation/devicetree/bindings/hwmon/ina3221.txt b/Documentation/devicetree/bindings/hwmon/ina3221.txt > new file mode 100644 > index 000000000000..266c9586c9b1 > --- /dev/null > +++ b/Documentation/devicetree/bindings/hwmon/ina3221.txt > @@ -0,0 +1,23 @@ > +ina3221 properties > + > +Required properties: > +- compatible: Must be "ti,ina3221" > +- reg: I2C address > + > +Optional properties: > + > +- ti,channel1-name: > +- ti,channel2-name: > +- ti,channel3-name: > + The names of the input sources (described in the schematics) > + Set the names with "NC" to indicate not-connected channels > + I don't really think this is a good idea - first to specify sensor names this way, and much less specifying that "NC" means that a sensor shall be disconnected/disabled. Also, if we define devicetree support for this chip, it should include all configuration options required to configure it. This should at the very least include shunt resistor values. Thanks, Guenter > +Example: > + > +ina3221@40 { > + compatible = "ti,ina3221"; > + reg = <0x40>; > + ti,channel1-name = "NC"; > + ti,channel2-name = "VDD_5V0_EXT"; > + ti,channel3-name = "VDD_19V"; > +}; > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] dt-bindings: hwmon: Add ina3221 documentation 2018-09-21 0:45 ` Guenter Roeck @ 2018-09-21 1:24 ` Nicolin Chen 0 siblings, 0 replies; 10+ messages in thread From: Nicolin Chen @ 2018-09-21 1:24 UTC (permalink / raw) To: Guenter Roeck Cc: jdelvare, robh+dt, mark.rutland, afd, linux-hwmon, devicetree, linux-kernel On Thu, Sep 20, 2018 at 05:45:07PM -0700, Guenter Roeck wrote: > On 09/20/2018 05:07 PM, Nicolin Chen wrote: > > +- ti,channel1-name: > > +- ti,channel2-name: > > +- ti,channel3-name: > > + The names of the input sources (described in the schematics) > > + Set the names with "NC" to indicate not-connected channels > > + > > I don't really think this is a good idea - first to specify sensor > names this way, and much less specifying that "NC" means that a sensor > shall be disconnected/disabled. I will replace the NC with a bool property for disconnection. > Also, if we define devicetree support for this chip, it should include > all configuration options required to configure it. This should at > the very least include shunt resistor values. I plan to add them too but am doing it incrementally. I can try to include them in the next version. Thanks Nicolin ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 2/2] hwmon: ina3221: Get channel names from DT node 2018-09-21 0:07 [PATCH 0/2] Add an initial DT binding doc for ina3221 Nicolin Chen 2018-09-21 0:07 ` [PATCH 1/2] dt-bindings: hwmon: Add ina3221 documentation Nicolin Chen @ 2018-09-21 0:07 ` Nicolin Chen 2018-09-21 0:41 ` Guenter Roeck 1 sibling, 1 reply; 10+ messages in thread From: Nicolin Chen @ 2018-09-21 0:07 UTC (permalink / raw) To: jdelvare, linux, robh+dt, mark.rutland Cc: afd, linux-hwmon, devicetree, linux-kernel The connection of channels are usually descirbed in the schematics, which then should be indicated in DT binding as well, and further should get exposed to sysfs so as to help driver users understand what channels are really monitoring respectively. Meanwhile, channels could be left unconnected based on the hardware design. So the channel name should support NC so the driver could disable the channel accordingly. Signed-off-by: Nicolin Chen <nicoleotsuka@gmail.com> --- drivers/hwmon/ina3221.c | 88 +++++++++++++++++++++++++++++++++++++---- 1 file changed, 80 insertions(+), 8 deletions(-) diff --git a/drivers/hwmon/ina3221.c b/drivers/hwmon/ina3221.c index e6b49500c52a..5d487e205260 100644 --- a/drivers/hwmon/ina3221.c +++ b/drivers/hwmon/ina3221.c @@ -41,9 +41,12 @@ #define INA3221_CONFIG_MODE_SHUNT BIT(1) #define INA3221_CONFIG_MODE_BUS BIT(2) #define INA3221_CONFIG_MODE_CONTINUOUS BIT(3) +#define INA3221_CONFIG_CHx_EN(x) BIT(14 - (x)) #define INA3221_RSHUNT_DEFAULT 10000 +#define INA3221_NOT_CONNECTED "NC" + enum ina3221_fields { /* Configuration */ F_RST, @@ -91,11 +94,20 @@ static const unsigned int register_channel[] = { * @regmap: Register map of the device * @fields: Register fields of the device * @shunt_resistors: Array of resistor values per channel + * @attr_group: attribute groups for sysfs node + * (leave one space at the end for NULL termination) + * @channel_name: channel names + * @enable: enable or disable channels */ struct ina3221_data { struct regmap *regmap; struct regmap_field *fields[F_MAX_FIELDS]; int shunt_resistors[INA3221_NUM_CHANNELS]; + + const struct attribute_group *attr_group[INA3221_NUM_CHANNELS + 1]; + const char *channel_name[INA3221_NUM_CHANNELS]; + + bool enable[INA3221_NUM_CHANNELS]; }; static int ina3221_read_value(struct ina3221_data *ina, unsigned int reg, @@ -253,6 +265,24 @@ static ssize_t ina3221_show_alert(struct device *dev, return snprintf(buf, PAGE_SIZE, "%d\n", regval); } +static ssize_t ina3221_show_name(struct device *dev, + struct device_attribute *attr, char *buf) +{ + struct sensor_device_attribute *sd_attr = to_sensor_dev_attr(attr); + struct ina3221_data *ina = dev_get_drvdata(dev); + unsigned int channel = sd_attr->index; + + return snprintf(buf, PAGE_SIZE, "%s\n", ina->channel_name[channel]); +} + +/* channel name */ +static SENSOR_DEVICE_ATTR(name1_input, 0444, + ina3221_show_name, NULL, INA3221_CHANNEL1); +static SENSOR_DEVICE_ATTR(name2_input, 0444, + ina3221_show_name, NULL, INA3221_CHANNEL2); +static SENSOR_DEVICE_ATTR(name3_input, 0444, + ina3221_show_name, NULL, INA3221_CHANNEL3); + /* bus voltage */ static SENSOR_DEVICE_ATTR(in1_input, S_IRUGO, ina3221_show_bus_voltage, NULL, INA3221_BUS1); @@ -317,8 +347,8 @@ static SENSOR_DEVICE_ATTR(in5_input, S_IRUGO, static SENSOR_DEVICE_ATTR(in6_input, S_IRUGO, ina3221_show_shunt_voltage, NULL, INA3221_SHUNT3); -static struct attribute *ina3221_attrs[] = { - /* channel 1 */ +static struct attribute *ina3221_channel1_attrs[] = { + &sensor_dev_attr_name1_input.dev_attr.attr, &sensor_dev_attr_in1_input.dev_attr.attr, &sensor_dev_attr_curr1_input.dev_attr.attr, &sensor_dev_attr_shunt1_resistor.dev_attr.attr, @@ -328,7 +358,11 @@ static struct attribute *ina3221_attrs[] = { &sensor_dev_attr_curr1_max_alarm.dev_attr.attr, &sensor_dev_attr_in4_input.dev_attr.attr, - /* channel 2 */ + NULL, +}; + +static struct attribute *ina3221_channel2_attrs[] = { + &sensor_dev_attr_name2_input.dev_attr.attr, &sensor_dev_attr_in2_input.dev_attr.attr, &sensor_dev_attr_curr2_input.dev_attr.attr, &sensor_dev_attr_shunt2_resistor.dev_attr.attr, @@ -338,7 +372,11 @@ static struct attribute *ina3221_attrs[] = { &sensor_dev_attr_curr2_max_alarm.dev_attr.attr, &sensor_dev_attr_in5_input.dev_attr.attr, - /* channel 3 */ + NULL, +}; + +static struct attribute *ina3221_channel3_attrs[] = { + &sensor_dev_attr_name3_input.dev_attr.attr, &sensor_dev_attr_in3_input.dev_attr.attr, &sensor_dev_attr_curr3_input.dev_attr.attr, &sensor_dev_attr_shunt3_resistor.dev_attr.attr, @@ -350,7 +388,12 @@ static struct attribute *ina3221_attrs[] = { NULL, }; -ATTRIBUTE_GROUPS(ina3221); + +static const struct attribute_group ina3221_group[INA3221_NUM_CHANNELS] = { + { .attrs = ina3221_channel1_attrs, }, + { .attrs = ina3221_channel2_attrs, }, + { .attrs = ina3221_channel3_attrs, }, +}; static const struct regmap_range ina3221_yes_ranges[] = { regmap_reg_range(INA3221_SHUNT1, INA3221_BUS3), @@ -373,10 +416,14 @@ static const struct regmap_config ina3221_regmap_config = { static int ina3221_probe(struct i2c_client *client, const struct i2c_device_id *id) { + const struct device_node *np = client->dev.of_node; struct device *dev = &client->dev; struct ina3221_data *ina; struct device *hwmon_dev; - int i, ret; + u16 mask = 0, val = 0; + const char *str; + char prop[32]; + int i, g, ret; ina = devm_kzalloc(dev, sizeof(*ina), GFP_KERNEL); if (!ina) @@ -407,9 +454,34 @@ static int ina3221_probe(struct i2c_client *client, return ret; } + /* Fetch hardware information from Device Tree */ + for (i = 0, g = 0; i < INA3221_NUM_CHANNELS; i++) { + /* Fetch the channel name */ + sprintf(prop, "ti,channel%d-name", i + 1); + /* Set a default name on failure */ + if (of_property_read_string(np, prop, &str)) + str = "unknown"; + /* Ignore unconnected channels */ + if (!strcmp(str, INA3221_NOT_CONNECTED)) + continue; + /* Log connected channels */ + ina->attr_group[g++] = &ina3221_group[i]; + ina->channel_name[i] = str; + ina->enable[i] = true; + } + + /* Disable unconnected channels */ + for (i = 0; i < INA3221_NUM_CHANNELS; i++) { + mask |= INA3221_CONFIG_CHx_EN(i); + val |= ina->enable[i] ? INA3221_CONFIG_CHx_EN(i) : 0; + } + ret = regmap_update_bits(ina->regmap, INA3221_CONFIG, mask, val); + if (ret) + return ret; + hwmon_dev = devm_hwmon_device_register_with_groups(dev, - client->name, - ina, ina3221_groups); + client->name, ina, + ina->attr_group); if (IS_ERR(hwmon_dev)) { dev_err(dev, "Unable to register hwmon device\n"); return PTR_ERR(hwmon_dev); -- 2.17.1 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] hwmon: ina3221: Get channel names from DT node 2018-09-21 0:07 ` [PATCH 2/2] hwmon: ina3221: Get channel names from DT node Nicolin Chen @ 2018-09-21 0:41 ` Guenter Roeck 2018-09-21 1:20 ` Nicolin Chen 0 siblings, 1 reply; 10+ messages in thread From: Guenter Roeck @ 2018-09-21 0:41 UTC (permalink / raw) To: Nicolin Chen, jdelvare, robh+dt, mark.rutland Cc: afd, linux-hwmon, devicetree, linux-kernel On 09/20/2018 05:07 PM, Nicolin Chen wrote: > The connection of channels are usually descirbed in the > schematics, which then should be indicated in DT binding > as well, and further should get exposed to sysfs so as > to help driver users understand what channels are really > monitoring respectively. > > Meanwhile, channels could be left unconnected based on > the hardware design. So the channel name should support > NC so the driver could disable the channel accordingly. > I am not in favor of such indirect settings. If a channel is to be disconnected, define a property for it. I am personally also not in favor of using devicetree to define channel names like this, much less so for a single driver. > Signed-off-by: Nicolin Chen <nicoleotsuka@gmail.com> > --- > drivers/hwmon/ina3221.c | 88 +++++++++++++++++++++++++++++++++++++---- > 1 file changed, 80 insertions(+), 8 deletions(-) > > diff --git a/drivers/hwmon/ina3221.c b/drivers/hwmon/ina3221.c > index e6b49500c52a..5d487e205260 100644 > --- a/drivers/hwmon/ina3221.c > +++ b/drivers/hwmon/ina3221.c > @@ -41,9 +41,12 @@ > #define INA3221_CONFIG_MODE_SHUNT BIT(1) > #define INA3221_CONFIG_MODE_BUS BIT(2) > #define INA3221_CONFIG_MODE_CONTINUOUS BIT(3) > +#define INA3221_CONFIG_CHx_EN(x) BIT(14 - (x)) > > #define INA3221_RSHUNT_DEFAULT 10000 > > +#define INA3221_NOT_CONNECTED "NC" > + > enum ina3221_fields { > /* Configuration */ > F_RST, > @@ -91,11 +94,20 @@ static const unsigned int register_channel[] = { > * @regmap: Register map of the device > * @fields: Register fields of the device > * @shunt_resistors: Array of resistor values per channel > + * @attr_group: attribute groups for sysfs node > + * (leave one space at the end for NULL termination) > + * @channel_name: channel names > + * @enable: enable or disable channels > */ > struct ina3221_data { > struct regmap *regmap; > struct regmap_field *fields[F_MAX_FIELDS]; > int shunt_resistors[INA3221_NUM_CHANNELS]; > + > + const struct attribute_group *attr_group[INA3221_NUM_CHANNELS + 1]; > + const char *channel_name[INA3221_NUM_CHANNELS]; > + > + bool enable[INA3221_NUM_CHANNELS]; > }; > > static int ina3221_read_value(struct ina3221_data *ina, unsigned int reg, > @@ -253,6 +265,24 @@ static ssize_t ina3221_show_alert(struct device *dev, > return snprintf(buf, PAGE_SIZE, "%d\n", regval); > } > > +static ssize_t ina3221_show_name(struct device *dev, > + struct device_attribute *attr, char *buf) > +{ > + struct sensor_device_attribute *sd_attr = to_sensor_dev_attr(attr); > + struct ina3221_data *ina = dev_get_drvdata(dev); > + unsigned int channel = sd_attr->index; > + > + return snprintf(buf, PAGE_SIZE, "%s\n", ina->channel_name[channel]); > +} > + > +/* channel name */ > +static SENSOR_DEVICE_ATTR(name1_input, 0444, > + ina3221_show_name, NULL, INA3221_CHANNEL1); > +static SENSOR_DEVICE_ATTR(name2_input, 0444, > + ina3221_show_name, NULL, INA3221_CHANNEL2); > +static SENSOR_DEVICE_ATTR(name3_input, 0444, > + ina3221_show_name, NULL, INA3221_CHANNEL3); > + > /* bus voltage */ > static SENSOR_DEVICE_ATTR(in1_input, S_IRUGO, > ina3221_show_bus_voltage, NULL, INA3221_BUS1); > @@ -317,8 +347,8 @@ static SENSOR_DEVICE_ATTR(in5_input, S_IRUGO, > static SENSOR_DEVICE_ATTR(in6_input, S_IRUGO, > ina3221_show_shunt_voltage, NULL, INA3221_SHUNT3); > > -static struct attribute *ina3221_attrs[] = { > - /* channel 1 */ > +static struct attribute *ina3221_channel1_attrs[] = { > + &sensor_dev_attr_name1_input.dev_attr.attr, > &sensor_dev_attr_in1_input.dev_attr.attr, > &sensor_dev_attr_curr1_input.dev_attr.attr, > &sensor_dev_attr_shunt1_resistor.dev_attr.attr, > @@ -328,7 +358,11 @@ static struct attribute *ina3221_attrs[] = { > &sensor_dev_attr_curr1_max_alarm.dev_attr.attr, > &sensor_dev_attr_in4_input.dev_attr.attr, > > - /* channel 2 */ > + NULL, > +}; > + > +static struct attribute *ina3221_channel2_attrs[] = { > + &sensor_dev_attr_name2_input.dev_attr.attr, > &sensor_dev_attr_in2_input.dev_attr.attr, > &sensor_dev_attr_curr2_input.dev_attr.attr, > &sensor_dev_attr_shunt2_resistor.dev_attr.attr, > @@ -338,7 +372,11 @@ static struct attribute *ina3221_attrs[] = { > &sensor_dev_attr_curr2_max_alarm.dev_attr.attr, > &sensor_dev_attr_in5_input.dev_attr.attr, > > - /* channel 3 */ > + NULL, > +}; > + > +static struct attribute *ina3221_channel3_attrs[] = { > + &sensor_dev_attr_name3_input.dev_attr.attr, > &sensor_dev_attr_in3_input.dev_attr.attr, > &sensor_dev_attr_curr3_input.dev_attr.attr, > &sensor_dev_attr_shunt3_resistor.dev_attr.attr, > @@ -350,7 +388,12 @@ static struct attribute *ina3221_attrs[] = { > > NULL, > }; > -ATTRIBUTE_GROUPS(ina3221); > + > +static const struct attribute_group ina3221_group[INA3221_NUM_CHANNELS] = { > + { .attrs = ina3221_channel1_attrs, }, > + { .attrs = ina3221_channel2_attrs, }, > + { .attrs = ina3221_channel3_attrs, }, > +}; > > static const struct regmap_range ina3221_yes_ranges[] = { > regmap_reg_range(INA3221_SHUNT1, INA3221_BUS3), > @@ -373,10 +416,14 @@ static const struct regmap_config ina3221_regmap_config = { > static int ina3221_probe(struct i2c_client *client, > const struct i2c_device_id *id) > { > + const struct device_node *np = client->dev.of_node; > struct device *dev = &client->dev; > struct ina3221_data *ina; > struct device *hwmon_dev; > - int i, ret; > + u16 mask = 0, val = 0; > + const char *str; > + char prop[32]; > + int i, g, ret; > > ina = devm_kzalloc(dev, sizeof(*ina), GFP_KERNEL); > if (!ina) > @@ -407,9 +454,34 @@ static int ina3221_probe(struct i2c_client *client, > return ret; > } > > + /* Fetch hardware information from Device Tree */ > + for (i = 0, g = 0; i < INA3221_NUM_CHANNELS; i++) { > + /* Fetch the channel name */ > + sprintf(prop, "ti,channel%d-name", i + 1); > + /* Set a default name on failure */ > + if (of_property_read_string(np, prop, &str)) > + str = "unknown"; So if there are no devicetree entries, the user now gets a sequence of "unknown" sensors ? I don't think so. Please keep in mind that there are users of this chip who don't have devicetree systems, and other users may not want to specify any specific name properties (or they use sensors3.conf to do it). > + /* Ignore unconnected channels */ > + if (!strcmp(str, INA3221_NOT_CONNECTED)) > + continue; Sorry, I won't accept this. I am sure we can come up with some useful means to define in devicetree if individual channels of a hardware monitoring chip are enabled or not, but a channel name of "NC" won't be it. > + /* Log connected channels */ > + ina->attr_group[g++] = &ina3221_group[i]; > + ina->channel_name[i] = str; > + ina->enable[i] = true; I also don't see the need for defining the group dynamically. The is_visible callback is very well suited for handling optional sysfs attributes. > + } > + > + /* Disable unconnected channels */ > + for (i = 0; i < INA3221_NUM_CHANNELS; i++) { > + mask |= INA3221_CONFIG_CHx_EN(i); > + val |= ina->enable[i] ? INA3221_CONFIG_CHx_EN(i) : 0; > + } > + ret = regmap_update_bits(ina->regmap, INA3221_CONFIG, mask, val); > + if (ret) > + return ret; > + > hwmon_dev = devm_hwmon_device_register_with_groups(dev, > - client->name, > - ina, ina3221_groups); > + client->name, ina, > + ina->attr_group); > if (IS_ERR(hwmon_dev)) { > dev_err(dev, "Unable to register hwmon device\n"); > return PTR_ERR(hwmon_dev); > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] hwmon: ina3221: Get channel names from DT node 2018-09-21 0:41 ` Guenter Roeck @ 2018-09-21 1:20 ` Nicolin Chen 2018-09-21 9:18 ` Nicolin Chen 0 siblings, 1 reply; 10+ messages in thread From: Nicolin Chen @ 2018-09-21 1:20 UTC (permalink / raw) To: Guenter Roeck Cc: jdelvare, robh+dt, mark.rutland, afd, linux-hwmon, devicetree, linux-kernel Hi Guenter, Thanks for the review. On Thu, Sep 20, 2018 at 05:41:48PM -0700, Guenter Roeck wrote: > > Meanwhile, channels could be left unconnected based on > > the hardware design. So the channel name should support > > NC so the driver could disable the channel accordingly. > > > > I am not in favor of such indirect settings. If a channel is > to be disconnected, define a property for it. OK. I can add a bool property for it instead. > I am personally also not in favor of using devicetree to define > channel names like this, much less so for a single driver. As DT is used to describe hardware, I felt plausible to put them in since the names are mentioned in the schematics. Do you have any advise to handle this better? > > + /* Fetch hardware information from Device Tree */ > > + for (i = 0, g = 0; i < INA3221_NUM_CHANNELS; i++) { > > + /* Fetch the channel name */ > > + sprintf(prop, "ti,channel%d-name", i + 1); > > + /* Set a default name on failure */ > > + if (of_property_read_string(np, prop, &str)) > > + str = "unknown"; > > So if there are no devicetree entries, the user now gets a sequence of > "unknown" sensors ? I don't think so. Please keep in mind that there are > users of this chip who don't have devicetree systems, and other users > may not want to specify any specific name properties (or they use sensors3.conf > to do it). Being enlightened by your comments below, maybe adding a separate group for channel names by attaching is_visible to it could be acceptable? Then, name nodes can hide from those who don't want to specify. > > + /* Ignore unconnected channels */ > > + if (!strcmp(str, INA3221_NOT_CONNECTED)) > > + continue; > > Sorry, I won't accept this. I am sure we can come up with some useful means > to define in devicetree if individual channels of a hardware monitoring chip > are enabled or not, but a channel name of "NC" won't be it. I will try the bool property as you mentioned earlier. > > + /* Log connected channels */ > > + ina->attr_group[g++] = &ina3221_group[i]; > > + ina->channel_name[i] = str; > > + ina->enable[i] = true; > > I also don't see the need for defining the group dynamically. The > is_visible callback is very well suited for handling optional sysfs > attributes. I will add an is_visible callback. Thanks Nicolin ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] hwmon: ina3221: Get channel names from DT node 2018-09-21 1:20 ` Nicolin Chen @ 2018-09-21 9:18 ` Nicolin Chen 2018-09-21 12:56 ` Guenter Roeck 0 siblings, 1 reply; 10+ messages in thread From: Nicolin Chen @ 2018-09-21 9:18 UTC (permalink / raw) To: Guenter Roeck Cc: jdelvare, robh+dt, mark.rutland, afd, linux-hwmon, devicetree, linux-kernel Hi, On Thu, Sep 20, 2018 at 06:20:38PM -0700, Nicolin Chen wrote: > > So if there are no devicetree entries, the user now gets a sequence of > > "unknown" sensors ? I don't think so. Please keep in mind that there are > > users of this chip who don't have devicetree systems, and other users > > may not want to specify any specific name properties (or they use sensors3.conf > > to do it). > > Being enlightened by your comments below, maybe adding a > separate group for channel names by attaching is_visible > to it could be acceptable? Then, name nodes can hide from > those who don't want to specify. > > > + /* Log connected channels */ > > > + ina->attr_group[g++] = &ina3221_group[i]; > > > + ina->channel_name[i] = str; > > > + ina->enable[i] = true; > > > > I also don't see the need for defining the group dynamically. The > > is_visible callback is very well suited for handling optional sysfs > > attributes. I tried is_visible but it looks like it won't be that neat to implement as some attributes of this driver don't really pass the channel index to the store()/show() but some other indexes. If you are very against the dynamical group, I can drop it to leave the sysfs node as it was. And for the name nodes that I was talking about above, I will add an sysfs store() function so non-DT users can set them, and I also removed the confusing "unknown" default name. I have been rewriting the DT binding and it should make a lot of sense now comparing to this version. Will send v2 tomrrow. Thanks Nicolin ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] hwmon: ina3221: Get channel names from DT node 2018-09-21 9:18 ` Nicolin Chen @ 2018-09-21 12:56 ` Guenter Roeck 2018-09-21 17:43 ` Nicolin Chen 0 siblings, 1 reply; 10+ messages in thread From: Guenter Roeck @ 2018-09-21 12:56 UTC (permalink / raw) To: Nicolin Chen Cc: jdelvare, robh+dt, mark.rutland, afd, linux-hwmon, devicetree, linux-kernel On 09/21/2018 02:18 AM, Nicolin Chen wrote: > Hi, > > On Thu, Sep 20, 2018 at 06:20:38PM -0700, Nicolin Chen wrote: >>> So if there are no devicetree entries, the user now gets a sequence of >>> "unknown" sensors ? I don't think so. Please keep in mind that there are >>> users of this chip who don't have devicetree systems, and other users >>> may not want to specify any specific name properties (or they use sensors3.conf >>> to do it). >> >> Being enlightened by your comments below, maybe adding a >> separate group for channel names by attaching is_visible >> to it could be acceptable? Then, name nodes can hide from >> those who don't want to specify. > >>>> + /* Log connected channels */ >>>> + ina->attr_group[g++] = &ina3221_group[i]; >>>> + ina->channel_name[i] = str; >>>> + ina->enable[i] = true; >>> >>> I also don't see the need for defining the group dynamically. The >>> is_visible callback is very well suited for handling optional sysfs >>> attributes. > > I tried is_visible but it looks like it won't be that neat to > implement as some attributes of this driver don't really pass > the channel index to the store()/show() but some other indexes. > The channel index is not the only means that can be used to determine the channel index. Many drivers use the position in the attrs[] array to determine the channel index. I don't see why this would not be possible here. > If you are very against the dynamical group, I can drop it to > leave the sysfs node as it was. > > And for the name nodes that I was talking about above, I will > add an sysfs store() function so non-DT users can set them, > and I also removed the confusing "unknown" default name. > The label attributes are RO. Please follow the ABI. temp[1-*]_label Suggested temperature channel label. Text string Should only be created if the driver has hints about what this temperature channel is being used for, and user-space doesn't. In all other cases, the label is provided by user-space. RO Guenter ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] hwmon: ina3221: Get channel names from DT node 2018-09-21 12:56 ` Guenter Roeck @ 2018-09-21 17:43 ` Nicolin Chen 0 siblings, 0 replies; 10+ messages in thread From: Nicolin Chen @ 2018-09-21 17:43 UTC (permalink / raw) To: Guenter Roeck Cc: jdelvare, robh+dt, mark.rutland, afd, linux-hwmon, devicetree, linux-kernel On Fri, Sep 21, 2018 at 05:56:00AM -0700, Guenter Roeck wrote: > > I tried is_visible but it looks like it won't be that neat to > > implement as some attributes of this driver don't really pass > > the channel index to the store()/show() but some other indexes. > > > > The channel index is not the only means that can be used to determine > the channel index. Many drivers use the position in the attrs[] array > to determine the channel index. I don't see why this would not be > possible here. Hmmm, that should simply work...I didn't mean not possible though. > > If you are very against the dynamical group, I can drop it to > > leave the sysfs node as it was. > > > > And for the name nodes that I was talking about above, I will > > add an sysfs store() function so non-DT users can set them, > > and I also removed the confusing "unknown" default name. > > > > The label attributes are RO. Please follow the ABI. > > temp[1-*]_label Suggested temperature channel label. Thanks a lot for the hint. I looked up the doc and feel that this one probably fits my situation more: in[0-*]_label Suggested voltage channel label. Will follow it in my next version. Thank you Nicolin ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2018-09-21 17:44 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-09-21 0:07 [PATCH 0/2] Add an initial DT binding doc for ina3221 Nicolin Chen 2018-09-21 0:07 ` [PATCH 1/2] dt-bindings: hwmon: Add ina3221 documentation Nicolin Chen 2018-09-21 0:45 ` Guenter Roeck 2018-09-21 1:24 ` Nicolin Chen 2018-09-21 0:07 ` [PATCH 2/2] hwmon: ina3221: Get channel names from DT node Nicolin Chen 2018-09-21 0:41 ` Guenter Roeck 2018-09-21 1:20 ` Nicolin Chen 2018-09-21 9:18 ` Nicolin Chen 2018-09-21 12:56 ` Guenter Roeck 2018-09-21 17:43 ` Nicolin Chen
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).