Message-ID: <20230312164255.13f444b2@jic23-huawei>
Date:   Sun, 12 Mar 2023 16:42:55 +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


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

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

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

> 
> >   
> > > +                     case PAC193X_VPOWER_4_ADDR:
> > > +                             *val = chip_info-  
> > > >chip_reg_data.vpower[channel];  
> > > +                             return IIO_VAL_INT;
> > > +                     default:
> > > +                             return -EINVAL;
> > > +                     }
> > > +                     break;
> > > +             case IIO_ENERGY:
> > > +                     switch (chan->address) {
> > > +                     case PAC193X_VPOWER_ACC_1_ADDR:
> > > +                     case PAC193X_VPOWER_ACC_2_ADDR:
> > > +                     case PAC193X_VPOWER_ACC_3_ADDR:
> > > +                     case PAC193X_VPOWER_ACC_4_ADDR:
> > > +                             /*
> > > +                              * expresses the 64 bit energy value
> > > as a 32 bit integer part and
> > > +                              * 32 bits (representing 8 digits)
> > > fractional value  
> > 
> > I'm lost.  So it's a 64 bit value?
> > We have IIO_VAL_INT_64. Does that not work here?
> >   
> 
> Yes, we have the IIO_VAL_INT_64 and it will work here. The "issue" was
> that the IIO_VAL_INT_64 is not available before kernel v6.0 and we
> already release something to customers supporting Linux kernel v4.7 and
> v5.15. Also our latest release of Linux4Sam is based on the Linux
> kernel v5.15 (we are working to have a release based on v6.1).
> 
> Maybe I'm totally wrong here but my idea was to have something "more
> generic" that could be easily backported to an older kernel and to
> change this to IIO_VAL_INT_64 later. 

As a general rule, we don't do this because it stops us from ever
improving things like this.  If you need IIO_VAL_INT_64 on
an older kernel then backport that as well.  Or provide a backported
version of your driver doing something different from upstream version.


> 
> 
> > > +                              */
> > > +                             curr_energy = chip_info-  
> > > >chip_reg_data.energy_sec_acc[channel];  
> > > +                             int_part = div_s64_rem(curr_energy,
> > > 100000000, &rem);
> > > +
> > > +                             /* rescale reminder to be printed as
> > > "nano" value */
> > > +                             rem = rem * 10;
> > > +
> > > +                             *val = int_part;
> > > +                             *val2 = rem;
> > > +                             return IIO_VAL_INT_PLUS_NANO;
> > > +                     default:
> > > +                             return -EINVAL;
> > > +                     }
> > > +                     break;
> > > +             default:
> > > +                     return -EINVAL;
> > > +             }
> > > +             break;
> > > +     case IIO_CHAN_INFO_AVERAGE_RAW:  
> > 
> > I'd forgotten we had this ABI and had to look up the docs.   Hmm.
> > It's a bit clunky but I guess
> > still useful in this case.  Longer term we should think about how to
> > describe that and how it
> > is different from oversampling. The problem with oversampling is it
> > affects the main channel
> > so we can't easily associate both an averaged value  and a raw one
> > like you have here.
> > 
> > Given lots of other changes requested and early in this kernel cycle,
> > perhaps it will make sense to revisit
> > this question at a later version of this patch (or maybe just leave
> > it alone as too hard
> > to do better!)  
> 
> 
> Here I was trying to map the IIO ABI to the PAC internal registers. We
> have some hardware registers that will do an average for us. Usually
> with oversampling someone could increase the bit number of the
> measurement, the average is just the sum of the numbers divided by how
> many numbers are and it can eliminate some noise out of the
> measurements.
> 
> 
> Here the device has registers that will keep the current values (and
> will calculate the power based on those values) and also some average
> measurements registers (keeping the same precision) that could help the
> user to identifies trends.

Understood. It was similar hardware that lead to this ABI existing
in the first place.  Whilst you are right that the purpose is somewhat
different.  In some cases oversampling in hardware doesn't increase the
bit depth and ends up looking much the same as an average.

> 
> 
> >   
> 
> 
> 
> > > +             ctrl_reg = PAC193X_CRTL_SAMPLE_RATE_SET(chip_info-  
> > > >crt_samp_spd_bitfield) |  
> > > +                     PAC193X_CHAN_ALERT_EN;
> > > +             ret = pac193x_i2c_write_byte(client,
> > > PAC193X_CTRL_REG, ctrl_reg);
> > > +             if (ret) {
> > > +                     dev_err(&client->dev, "%s - can't update
> > > sample rate\n", __func__);
> > > +                     chip_info->sample_rate_value = old_samp_rate;
> > > +                     mutex_unlock(&chip_info->lock);  
> > 
> > Fun thing about i2c bus write failures. You have no idea what failed.
> > The value might be
> > the old value or the new one after the failed write... So normally we
> > don't bother too much if our values are
> > out of sync. and that leads to easier unwinding.  However, if you
> > want to keep this as it
> > is then that's fine.
> >   
> 
> 
> I would like to keep it, maybe there are some corner cases when it will
> help debugging (good to know in case of intermittent issues)

I'm fine seeing the error print, but unless you succesfully hammer back in
the old value with a new write, you have no guarantee of what the device
thinks the value is.


> > 
> > 
> >   
> > > +                                                  int revision,
> > > +                                                  int function)
> > > +{
> > > +     acpi_status status;
> > > +     struct acpi_buffer buffer = {ACPI_ALLOCATE_BUFFER, NULL};
> > > +     union acpi_object args[PAC193X_ACPI_ARG_COUNT];
> > > +     struct acpi_object_list args_list;
> > > +     uuid_t uuid;
> > > +
> > > +     uuid_parse(PAC193X_DSM_UUID, &uuid);
> > > +
> > > +     args[0].type = ACPI_TYPE_BUFFER;
> > > +     args[0].buffer.length = sizeof(struct pac193x_uuid_format);
> > > +     args[0].buffer.pointer = (u8 *)&uuid;
> > > +
> > > +     args[1].type = ACPI_TYPE_INTEGER;
> > > +     args[1].integer.value = revision;
> > > +
> > > +     args[2].type = ACPI_TYPE_INTEGER;
> > > +     args[2].integer.value = function;
> > > +
> > > +     args[3].type = ACPI_TYPE_PACKAGE;
> > > +     args[3].package.count = 0;
> > > +
> > > +     args_list.count = PAC193X_ACPI_ARG_COUNT;
> > > +     args_list.pointer = &args[0];
> > > +
> > > +     status = acpi_evaluate_object(handle, "_DSM", &args_list,
> > > &buffer);
> > > +
> > > +     if (ACPI_FAILURE(status)) {
> > > +             kfree(buffer.pointer);
> > > +             return NULL;
> > > +     }
> > > +
> > > +     return buffer.pointer;
> > > +}
> > > +
> > > +static char *pac193x_acpi_get_acpi_match_entry(acpi_handle handle)
> > > +{
> > > +     acpi_status status;
> > > +     union acpi_object *name_object;
> > > +     struct acpi_buffer buffer = {ACPI_ALLOCATE_BUFFER, NULL};
> > > +
> > > +     status = acpi_evaluate_object(handle, "_HID", NULL, &buffer);
> > > +     name_object = buffer.pointer;
> > > +
> > > +     return name_object->string.pointer;
> > > +}
> > > +
> > > +static const char *pac193x_match_acpi_device(struct i2c_client
> > > *client,
> > > +                                          struct pac193x_chip_info
> > > *chip_info)
> > > +{
> > > +     char *name;
> > > +     acpi_handle handle;
> > > +     union acpi_object *rez;
> > > +     unsigned short bi_dir_mask;
> > > +     int idx, i;
> > > +  
> > 
> > Any docs for the ACPI stuff?
> > It looks plausible, but an example DSDT blob would make it easier to
> > review.
> >   
> 
> Here we have:
> https://ww1.microchip.com/downloads/aemDocuments/documents/OTH/ApplicationNotes/ApplicationNotes/PAC193X-Integration-Notes-for-Microsoft-Windows-10-and-Windows-11-Driver-Support-DS00002534.pdf
Yikes that's a long file name ;)  Exactly what I was looking for. Thanks.

Please add that link to the driver somewhere.  + ignore the inevitable
checkpatch warning on line length.

> 
> 
> > 
> >   
> > > +     /*
> > > +      * count how many channels are enabled and store
> > > +      * this information within the driver data
> > > +      */
> > > +     cnt = 0;
> > > +     chip_info->chip_reg_data.num_enabled_channels = 0;
> > > +     while (cnt < chip_info->phys_channels) {
> > > +             if (chip_info->active_channels[cnt])
> > > +                     chip_info-  
> > > >chip_reg_data.num_enabled_channels++;  
> > > +             cnt++;
> > > +     }
> > > +
> > > +     /*
> > > +      * read whatever information was gathered before the driver
> > > was loaded
> > > +      * establish which channels are enabled/disabled and then
> > > establish the
> > > +      * information retrieval mode (using SKIP or no).  
> > 
> > It's unusual to have a driver carry on with existing values.  Much
> > more common
> > to either have it reset the device (to known default state) or write
> > all the
> > registers if such a reset doesn't exist.
> > 
> > One reason for that flow is that we really don't trust other software
> > :)
> > Another is that often we don't know if the device had any power until
> > we turned it
> > on.  So it's much easier to review code where we control all the
> > state.
> > 
> > If there is a strong reason for carrying on with existing settings
> > please add
> > comments here and something to the patch description to point out
> > this unusual
> > behaviour.
> >   
> 
> There are some customers asking to measure the total power exchanged
> from the power-up of the system (like if you are spending a lot of time
> into the bootloader and drain the battery).

Ok. Add a comment on that.


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

> 
> "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.

Jonathan

> 
> >   
> 
> Thanks,
> Marius
> 

Powered by blists - more mailing lists