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@sama5d27-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@sama5d27-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
