Message-ID: <0e6ed004f296cd0a756711d5576d8a6d8129cbc0.camel@microchip.com>
Date:   Thu, 23 Mar 2023 15:15:18 +0000
From:   <Marius.Cristea@...rochip.com>
To:     <jic23@...nel.org>
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 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



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

> > 
> > 
> > 
> > 
> > > > +                              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).


> > 
> > > 
> > 
> > > 
> > > > shunt_value_store, 0);
> > > > +static IIO_DEVICE_ATTR(shunt_value_2, 0644, shunt_value_show,
> > > > shunt_value_store, 0);
> > > > +static IIO_DEVICE_ATTR(shunt_value_3, 0644, shunt_value_show,
> > > > shunt_value_store, 0);
> > > > +static IIO_DEVICE_ATTR(shunt_value_4, 0644, shunt_value_show,
> > > > shunt_value_store, 0);
> > > > +
> > > > +static IIO_DEVICE_ATTR(channel_name_1, 0444,
> > > > channel_name_show,
> > > > NULL, 0);
> > > > +static IIO_DEVICE_ATTR(channel_name_2, 0444,
> > > > channel_name_show,
> > > > NULL, 0);
> > > > +static IIO_DEVICE_ATTR(channel_name_3, 0444,
> > > > channel_name_show,
> > > > NULL, 0);
> > > > +static IIO_DEVICE_ATTR(channel_name_4, 0444,
> > > > channel_name_show,
> > > > NULL, 0);
> > > > +
> > > > +static IIO_DEVICE_ATTR(reset_accumulators, 0200, NULL,
> > > > reset_accumulators_store, 0);
> > > > +
> > > > +static struct attribute *pac193x_all_attributes[] = {
> > > > +     PAC193X_DEV_ATTR(shunt_value_1),
> > > 
> > > These all need ABI documentation so that we can easily review
> > > what
> > > they do.
> > > Documenation/ABI/testing/sysfs-bus-iio-pac1931
> > > Note that if they overlap with ABI used elsewhere we may need to
> > > move
> > > it to a more
> > > generic place (there can't be two lots of docs for the same ABI
> > > element)
> > > 
> > 
> > I will add comments into the code. The "shunt_value_show" will
> > print
> > the shunt value used to calculate current and power.
> I'd prefer handling shut calibration as a future patch because it
> needs
> some thought and discussion + may delay the rest of the driver. 
> Without that
> ability to tweak it I'm not sure it provides value to be able to read
> it now.
> 
> > The "channel_name_show" is the name of the device channel used to
> > easily identify what we are measuring (V_DDR, V_GPU, V_IO, etc) and
> > it's an "umbrella" for multiple IIO measurements (voltage, current
> > and
> > power)
> 
> As above. I'd like to think a bit more on this one.
> 

OK.


> > 
> > "reset_accumulators" will help the user to reinitialize the power
> > on
> > all channels.
> 
> This sounds like the rare case where the ENABLE IIO ABI may fit.
> We use that for things like step counters that also accumulate over
> time.  If we can use that, I'd prefer it to custom ABI.
> 
> > 
> > 
> > > > +     if (!indio_dev) {
> > > > +             dev_err_probe(&indio_dev->dev,
> > > > PTR_ERR(indio_dev),
> > > > +                           "Can't allocate iio device\n");
> > > > +             return -ENOMEM;
> > > > +     }
> > > > +
> > > > +     chip_info = iio_priv(indio_dev);
> > > > +
> > > > +     i2c_set_clientdata(client, indio_dev);
> > > 
> > > Assuming you follow suggestion to go fully devm managed handling
> > > of
> > > remove, I don't htink
> > > you will need this.
> > > 
> > > > +     chip_info->client = client;
> > > > +
> > > > +     memset(&chip_info->chip_reg_data, 0, sizeof(chip_info-
> > > > > chip_reg_data));
> > > 
> > > The iio_priv space is allocated with kzalloc so no need to zero
> > > here.
> > > 
> > > > +
> > > > +     if (ACPI_HANDLE(&client->dev)) {
> > > > +             pac193x_get_variant(chip_info);
> > > 
> > > Why is this ACPI specific?
> > 
> > Not really. I will change it to work in both cases.
> > 
> > > 
> > > If you can query the variant from the hardware, that is the right
> > > thing to use
> > > for all registration types.  It can be helpful to call out if
> > > there
> > > is
> > > a mismatch though with a warning print, so comparing the result
> > > of
> > > the i2c call with the data is fine.
> > > 
> > > > +     } else {
> > > > +             dev_id = id->driver_data;
> > > > +
> > > > +             /* store the type of chip */
> > > > +             chip_info->chip_variant = dev_id;
> > > > +
> > > > +             /* get the maximum number of channels for the
> > > > given
> > > > chip id */
> > > > +             chip_info->phys_channels =
> > > > pac193x_chip_config[dev_id].phys_channels;
> > > > +     }
> > > > +
> > > > +     /*
> > > > +      * load default settings - all channels disabled,
> > > > +      * uni directional flow, default shunt values
> > > 
> > > The concept of a default shunt value bothers me a little if this
> > > is
> > > an external resistor.  How is there in any real sense a default?
> > > Can we just fail if it's not provided - or wrap the default up
> > > for
> > > just ACPI case where I guess there might be firmware that doesn't
> > > specify it?
> > 
> > I'm doing some math with that resistor later in code and I was just
> > making sure that I will not divide by 0, but indeed I could just
> > exit
> > in case the value is not correctly set.
> 
> Exit is best thing to do I think.
> 
> 
OK.


> > 
Thanks,
Marius

Powered by blists - more mailing lists