Message-ID: <20230325180604.2bee86c2@jic23-huawei>
Date:   Sat, 25 Mar 2023 18:06:04 +0000
From:   Jonathan Cameron <jic23@...nel.org>
To:     <Marius.Cristea@...rochip.com>
Cc:     <devicetree@...r.kernel.org>, <lars@...afoo.de>,
        <linux-iio@...r.kernel.org>, <robh+dt@...nel.org>,
        <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v1 2/2] iio: adc: adding support for pac193x

On Thu, 23 Mar 2023 15:15:18 +0000
<Marius.Cristea@...rochip.com> wrote:

> On Sun, 2023-03-12 at 16:42 +0000, Jonathan Cameron wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you
> > know the content is safe
> >   
> > >   
> > > > 
> > > > 
> > > >   
> > > > > +
> > > > > +     len += sprintf(buf, "%u\n", chip_info->shunts[target]);
> > > > > +
> > > > > +     return len;
> > > > > +}
> > > > > +
> > > > > +static ssize_t channel_name_show(struct device *dev,  
> > > > 
> > > > Looks like per channel label support provided by the read_label
> > > > iio_info callback.
> > > >   
> > > 
> > > I was trying to use the read_label iio_info callback but I end it
> > > up
> > > into a sysfs error related to duplicated labels.  
> > 
> > That's interesting.  Can you provide more info on that. I'd like to
> > understand why that's happening to you.
> >   
> 
> I have add the ".read_label" and the function asociated with it to the
> driver but here is the error:
> 
> root@...a5d27-wlsom1-ek-sd:~/work/pac193x# insmod pac1934.ko
> pac193x 1-001c: :pac193x_prep_iio_channels: Channel 2 active
> pac193x 1-001c: :pac193x_prep_iio_channels: Channel 3 active
> pac193x 1-001c: :pac193x_prep_iio_channels: Channel 4 active
> iio iio:device1: tried to double register : in_voltage2_label
> pac193x 1-001c: Failed to register sysfs interfaces
> iio iio:device1: error -EBUSY: Can't register IIO device
> pac193x: probe of 1-001c failed with error -16
> 

There can only be one registered iio_chan_spec entry for a given
group of direction, type, channel, modifier (channel and modifier can
be optional). This comes from inherent design restrictions of the
events.  Now I think what has happened here is that you are
registering both _VBUS_CHANNEL and _VBUS_AVG channel with the
same index. You can't do that.  Whilst you don't currently have events,
if they come along later there will be no way to distinguish those
two.

So either roll them into one iio_chan_spec, or give them different
index values.

> 
> 
> Here are the list of files in case I don't use read_label (only
> physical channels 2 to 4 are available):
> 
> root@...a5d27-wlsom1-ek-sd:~/work/pac193x# ls
> /sys/bus/iio/devices/iio\:device1/
> channel_name_2        in_current4_mean_raw  in_power2_raw         
> in_voltage2_scale     power
> channel_name_3        in_current4_raw       in_power2_scale       
> in_voltage3_mean_raw  reset_accumulators
> channel_name_4        in_current4_scale     in_power3_raw         
> in_voltage3_raw       sampling_frequency_available
> in_current2_mean_raw  in_energy2_raw        in_power3_scale       
> in_voltage3_scale     shunt_value_2
> in_current2_raw       in_energy2_scale      in_power4_raw         
> in_voltage4_mean_raw  shunt_value_3
> in_current2_scale     in_energy3_raw        in_power4_scale       
> in_voltage4_raw       shunt_value_4
> in_current3_mean_raw  in_energy3_scale      in_sampling_frequency 
> in_voltage4_scale     subsystem
> in_current3_raw       in_energy4_raw        in_voltage2_mean_raw   name
> uevent
> in_current3_scale     in_energy4_scale      in_voltage2_raw       
> of_node               waiting_for_supplier
> 
> 
> 
> > > 
> > > Also I'm not sure if this read_label will help me in the end. What
> > > I
> > > was aiming to obtain here with the "channel_name_show" and the
> > > custom
> > > IIO attribute was to have "an umbrella" name/label for multiple IIO
> > > channels. For example on physical "channel 1" we will measure the
> > > voltage, the current and the power because all of those IIO
> > > channels
> > > will point to one board power supply (like VDD, VIO, V_GPU, V_DDR,
> > > etc).  
> > 
> > That should be possible with read_label as it's free form text.
> > Perhaps I'm missing why that doesn't provide enough information for
> > what
> > you need.
> >   
> > >   
> 
> It seems it does more that I need. My concern is that the read_label
> will do a label for all available channels (like _raw or scale_ will
> do) and I want to  have just up to a maximum of 4 labels each one will
> cover the multiple entries (e.g.: channel_name_2 will be a umbrela
> "label" for:
> in_power2_raw         in_voltage2_scale  in_power2_scale        
> in_current2_mean_raw  in_energy2_raw     shunt_value_2
> in_current2_raw       in_energy2_scale   in_current2_scale     
> in_voltage2_mean_raw  in_voltage2_raw

Whilst it might not be ideal, label is standard ABI that normal code
is going to know how to use.  channel_name_2 is custom ABI that only
code written against this one driver will use.

So go with the label version and don't worry about the duplication.

The association you are building between channel indexes is also unique
to this driver, so you should not rely on that as generic code will not
know that all the things numbered 2 refer to a particular set of inputs.

If each channel has a _label and those match, that association is explicit.

Channel numbers shouldn't really mean anything on their own.

> 
> > > 
> > > 
> > > 
> > >   
> > > > > +                              struct device_attribute *attr,
> > > > > +                              char *buf)
> > > > > +{
> > > > > +     struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> > > > > +     struct pac193x_chip_info *chip_info =
> > > > > iio_priv(indio_dev);
> > > > > +     int len = 0;
> > > > > +     int target = (int)(attr->attr.name[strlen(attr-  
> > > > > >attr.name) -  
> > > > > 1] - '0') - 1;
> > > > > +
> > > > > +     len += sprintf(buf, "%s\n", chip_info-  
> > > > > > channel_names[target]);  
> > > > > +
> > > > > +     return len;
> > > > > +}
> > > > > +
> > > > > +static ssize_t shunt_value_store(struct device *dev,  
> > > > 
> > > > Shunt values aren't normally dynamic.  Why do you need to write
> > > > it
> > > > from
> > > > userspace? I'd expect that to come from firmware.
> > > > 
> > > > If it's needed the ext_info framework might be a better fit than
> > > > direct implementation of the attribute.  
> > > 
> > > 
> > > Yes, usually the shunt aren't normally dynamic, but what I'm aiming
> > > to
> > > get here is to let the user to overwrite the value after a
> > > calibration
> > > of the system. This will improve the accuracy of the reading even
> > > in
> > > the case the shunts are not of high precision ones.  
> > 
> > I'll go with maybe on this.  Perhaps not a feature for the initial
> > version of the driver, but one that is better to discuss in a follow
> > up thread along with details of the expected calibration process etc.  
> > >   
> 
> Calibration process could be easly be done. Just think that we have a
> linux board that measure some dinamic load that uses a connector. The
> load could be easily changed by the user with a "calibrated" load, read
> the measurement do the math expected versus read values and update the
> shunt value. Those values needs to be stored by the user somewhere to
> be reused after a reboot. After the board is calibrated a "normal" load
> could pluged in (one example is to monitor the charge/discharge current
> of a battery).

OK. I'd still advise postponing control of this.  It's unusual and likely
to delay / distract from review of the rest of the driver.

Jonathan

Powered by blists - more mailing lists