Re: [PATCH 2/2] iio: adc: adding support for PAC1711

[<Ariana.Lazar@microchip.com>]

Hi Jonathan,

Thank you for the feedback, please see my comments bellow:

> > Best regards,
Ariana
> > 
> 
> > +};
> > +
> > +static inline u64 pac1711_get_unaligned_be56(u8 *p)
> > +{
> > +     return (u64)p[0] << 48 | (u64)p[1] << 40 | (u64)p[2] << 32 |
> > +             (u64)p[3] << 24 | p[4] << 16 | p[5] << 8 | p[6];
> 
> Hmm.  Maybe this one is reasonable to add to the main unaligned
> function set.
> Perhaps not yet as a grep didn't find multiple of this size.
> 

There has been an unsuccessful attempt to submit this function to the
unaligned set. It was rejected because of the lack of current possible
shared uses. Please see the discussion thread below:

https://yhbt.net/lore/lkml/20240927083543.80275-1-
marius.cristea@microchip.com/T/#u

> 
> 
...
> 
> > +
> > +static struct iio_chan_spec pac1711_single_channel[] = {
> 
> As below. Clearly not a single channel.
> 
> > +     PAC1711_VPOWER_CHANNEL(0, 0, PAC1711_VPOWER_ADDR),
> > +     PAC1711_VBUS_CHANNEL(0, 0, PAC1711_VBUS_ADDR),
> > +     PAC1711_VSENSE_CHANNEL(0, 0, PAC1711_VSENSE_ADDR),
> > +     PAC1711_VBUS_AVG_CHANNEL(0, 0, PAC1711_VBUS_AVG_ADDR),
> > +     PAC1711_VSENSE_AVG_CHANNEL(0, 0, PAC1711_VSENSE_AVG_ADDR),
> > +};
> > +
> > +static IIO_DEVICE_ATTR(in_energy_raw, 0444,
> > +                    pac1711_in_power_acc_raw_show, NULL, 0);
> > +
> > +static IIO_DEVICE_ATTR(in_energy_scale, 0444,
> > +                    pac1711_in_power_acc_scale_show, NULL, 0);
> > +
> > +static IIO_DEVICE_ATTR(in_energy_en, 0644,
> > +                    pac1711_in_enable_acc_show,
> > pac1711_in_enable_acc_store, 0);
> > +
> > +static IIO_DEVICE_ATTR(in_current_acc_raw, 0444,
> > +                    pac1711_in_current_acc_raw_show, NULL, 0);
> > +
> > +static IIO_DEVICE_ATTR(in_current_acc_scale, 0444,
> > +                    pac1711_in_current_acc_scale_show, NULL, 0);
> > +
> > +static IIO_DEVICE_ATTR(in_current_acc_en, 0644,
> > +                    pac1711_in_enable_acc_show,
> > pac1711_in_enable_acc_store, 0);
> > +
> > +static IIO_DEVICE_ATTR(in_voltage_acc_raw, 0444,
> > +                    pac1711_in_voltage_acc_raw_show, NULL, 0);
> > +
> > +static IIO_DEVICE_ATTR(in_voltage_acc_scale, 0444,
> > +                   pac1711_in_voltage_acc_scale_show, NULL, 0);
> > +
> > +static IIO_DEVICE_ATTR(in_voltage_acc_en, 0644,
> > +                    pac1711_in_enable_acc_show,
> > pac1711_in_enable_acc_store, 0);
> > +
> > +static struct attribute *pac1711_power_acc_attr[] = {
> > +     PAC1711_DEV_ATTR(in_energy_raw),
> > +     PAC1711_DEV_ATTR(in_energy_scale),
> > +     PAC1711_DEV_ATTR(in_energy_en),
> > +     NULL
> > +};
> > +
> > +static struct attribute *pac1711_current_acc_attr[] = {
> > +     PAC1711_DEV_ATTR(in_current_acc_raw),
> > +     PAC1711_DEV_ATTR(in_current_acc_scale),
> > +     PAC1711_DEV_ATTR(in_current_acc_en),
> > +     NULL
> > +};
> > +
> > +static struct attribute *pac1711_voltage_acc_attr[] = {
> > +     PAC1711_DEV_ATTR(in_voltage_acc_raw),
> > +     PAC1711_DEV_ATTR(in_voltage_acc_scale),
> > +     PAC1711_DEV_ATTR(in_voltage_acc_en),
> 
> If these did make sense (see other comments) then they should be
> done as ext_info for the given channels rather than going though
> the complexity of custom attributes.
> 
> Custom attributes make most sense when not associated with any
> channels.
> A lot of drivers have them for historical reasons as well that we
> haven't
> yet cleaned up.
> 
> Even in_shunt_resistor is can be a shared_by_all extended attribute
> as it's still associated with a bunch of channels.
> 
> 

As for switching accumulator channels to ext_info instead of custom
attributes, only _en channel would be eligible. Both _raw and _scale do
not have constant values, therefore cannot be defined with consts at
startup. Scale can be modified during runtime by changing shunt value
and raw constantly accumulates the read values. It is not recommended
to switch accumulation mode during runtime, but that does not make
possible defining as consts the _raw or _scale attributes. If there
were an workaround which does not need the definition of items and
num_items fields, I would switch to using ext_info.