On Saturday, 30 August 2025 06:37:27 Central European Summer Time Andy
Shevchenko wrote:
> On Fri, Aug 29, 2025 at 2:41 AM David Lechner <dlech...@baylibre.com> wrote:
> > On 8/28/25 5:17 PM, Duje Mihanović wrote:
...
> > + if (adcnum < 0 || adcnum > 3)
> > + return -EINVAL;
>
> in_range()
Not sure how relevant that check is anymore; in my current local
version this can definitely only be called for one of the GPADCx
channels. Should I drop it then?
> > > + for (int i = 0; i < 16; i++) {
>
> Why signed? What is the magic value here?
No magic to my understanding, it's that the bias generator can output
16 different current levels.
> > > + ret = regmap_update_bits(*map, reg, 0xf, i);
>
> GENMASK() or even better to have a definitive constant.
While at it, could
> > + *amp = 1 + i * 5;
use a macro too?
> > > + raw = gpadc_get_raw(iio, chan->channel);
> > > + if (raw < 0) {
> > > + ret = raw;
> > > + goto out;
> > > + }
>
> Instead just assign to ret and if okay, reassign to raw.
I've refactored that function greatly since and now have:
*val = gpadc_get_raw(iio, chan->channel);
if (*val < 0)
return *val;
Is assigning to *val immediately alright?
> > > + const u8 config[] = {0xff, 0xfd, 0x1};
> >
> > IIRC, IIO subsystem prefers spaces around the braces.
> >
> > { 0xff, 0xfd, 0x1 };
>
> Also make them fixed width, i.e. 0x01
I've replaced these with macros as David suggested.
> > > +config 88PM886_GPADC
> > > + tristate "Marvell 88PM886 GPADC driver"
> > > + depends on MFD_88PM886_PMIC
> > > + default y
>
> Really? Why tristate then?
> I would expect default MFD_88PM886_PMIC instead,
>
> > > + help
> > > + Say Y here to enable support for the GPADC (General Purpose ADC)
> > > + found on the Marvell 88PM886 PMIC. The GPADC measures various
> > > + internal voltages and temperatures, including (but not limited to)
> > > + system, battery and USB.
>
> Please, add a line about the module name if one chooses 'm'. Or see
> above — drop the "tristate" and explain why this driver may not be a
> module in the commit message.
'default MFD_88PM886_PMIC' would make it y as that one is a bool. How
about 'default m if MFD_88PM886_PMIC' or, since this already depends on
_PMIC, 'default m'?
Regards,
--
Duje
