Re: [PATCH v4 3/3] iio: adc: add support for PAC1711

[Ariana.Lazar]

Hi Jonathan,

Thank you for the review.

> > +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];
> > +}
> Neil Amstrong has a patch on list adding a generic version of this.
> https://lore.kernel.org/all/20261013-topic-sm8x50-iio-max34417-adc-v3-2-d3604ae4a229@xxxxxxxxxx/
> 
> If you can add that to start of your series, then I'll deal with
> picking it up with whoever's series lands first.
> +CC Neil for info on this.


Sure, I will include be56 helper patch in the series and add Neil to
CC.


> > +
> > +static int pac1711_read_raw(struct iio_dev *indio_dev, struct
> > iio_chan_spec const *chan,
> > +                         int *val, int *val2, long mask)
> > +{
> > +     struct pac1711_chip_info *info = iio_priv(indio_dev);
> > +     u64 tmp = 0;
> > +     int ret;
> > +
> > +     switch (mask) {
> > +     case IIO_CHAN_INFO_RAW:
> > +             ret = pac1711_retrieve_data(info,
> > PAC1711_MIN_UPDATE_WAIT_TIME_US);
> > +             if (ret)
> > +                     return ret;
> > +
> > +             switch (chan->type) {
> > +             case IIO_VOLTAGE:
> > +                     *val = info->chip_reg_data.vbus;
> > +                     return IIO_VAL_INT;
> > +             case IIO_CURRENT:
> > +                     *val = info->chip_reg_data.vsense;
> > +                     return IIO_VAL_INT;
> > +             case IIO_POWER:
> > +                     scoped_guard(mutex, &info->lock) {
> > +                             *val = (u32)info-
> > >chip_reg_data.vpower;
> > +                             *val2 = (u32)(info-
> > >chip_reg_data.vpower >> 32);
> > +                     }
> > +                     return IIO_VAL_INT_64;
> > +             case IIO_ENERGY:
> > +             case IIO_CHARGE:
> > +                     scoped_guard(mutex, &info->lock) {
> > +                             *val = (u32)info-
> > >chip_reg_data.acc_val;
> 
> Could use lower_32_bits / upper_32_bits.
> Maybe we should just have a helper for this given it is
> becoming rather more common.
>                                 iio_fill_val_int_64(info-
> >chip_reg_data.acc_val, val, val2);
> 
> I don't mind if you want to leave that a follow up for someone else
> to do though.


About this, would you prefer me to add iio_fill_val_int_64() as a
precursor patch in this series, with PAC1711 as its first user? Or
should I use lower_32_bits() and upper_32_bits() in the driver
implementation and leave the generic helper for a separate follow-up
series?

> > 
> > 
> > +
> > +static int pac1711_parse_fw(struct i2c_client *client, struct
> > pac1711_chip_info *info)
> > +{
> > +     struct device *dev = &client->dev;
> > +     const char *acc_mode;
> > +     int ret;
> > +
> > +     ret = device_property_read_u32(dev, "shunt-resistor-micro-
> > ohms", &info->shunt);
> > +     if (ret)
> > +             return dev_err_probe(dev, ret, "Shunt resistor
> > property error\n");
> > +
> > +     if (!info->shunt)
> > +             return dev_err_probe(dev, -EINVAL, "Invalid value for
> > shunt resistor\n");
> > +
> > +     ret = pac1711_init_vbus_vsense_ranges(info, true);
> > +     if (ret)
> > +             return ret;
> > +
> > +     ret = pac1711_init_vbus_vsense_ranges(info, false);
> > +     if (ret)
> > +             return ret;
> > +
> > +     if (device_property_present(dev, "microchip,accumulation-
> > source")) {
> > +             ret = device_property_read_string(dev,
> > "microchip,accumulation-source", &acc_mode);
> > +             if (ret)
> > +                     return dev_err_probe(dev, ret, "Cannot read
> > accumulation-source\n");
> > +
> > +             if (!strcmp(acc_mode, PAC1711_ACC_ENERGY_STR))
> > +                     info->accumulation_mode =
> > PAC1711_ACCMODE_VPOWER;
> > +             else if (!strcmp(acc_mode, PAC1711_ACC_CHARGE_STR))
> > +                     info->accumulation_mode =
> > PAC1711_ACCMODE_VSENSE;
> 
> Can we use fwnode_property_match_property_string() for this?
> Could add a device variant for that to make it simpler still.
> 
> 

Regarding this comment, I was wondering if
device_property_match_property_string() is the function you want me to
use in the driver implementation instead of strcmp():

https://elixir.bootlin.com/linux/v7.3-rc7/C/ident/device_property_match_property_string




Best regards,
Ariana