On 09/21/2018 02:18 AM, Nicolin Chen wrote:

Hi,

On Thu, Sep 20, 2018 at 06:20:38PM -0700, Nicolin Chen wrote:

quoted

quoted

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.

quoted

quoted

quoted

+		/* 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