| Message ID | 20180921000753.21846-1-nicoleotsuka@gmail.com |
|---|---|
| Headers | show |
| Series | Add an initial DT binding doc for ina3221 | expand |
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); >
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
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
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
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