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

[Jonathan Cameron <jic23@kernel.org>]

> > > +                             *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?

Prefer the helper - but not a strong preference.



> 
> > > 
> > > 
> > > +
> > > +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
Huh. I wonder why my search didn't find that.  Indeed!

Jonathan


> 
> 
> 
> 
> Best regards,
> Ariana
>