Message-ID: <20250907113609.6060e557@jic23-huawei>
Date: Sun, 7 Sep 2025 11:36:09 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: Andy Shevchenko <andy.shevchenko@...il.com>
Cc: Duje Mihanović <duje@...emihanovic.xyz>, David
 Lechner <dlechner@...libre.com>, Nuno Sá
 <nuno.sa@...log.com>, Andy Shevchenko <andy@...nel.org>, Karel Balej
 <balejk@...fyz.cz>, Lee Jones <lee@...nel.org>, Rob Herring
 <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
 <conor+dt@...nel.org>, David Wronek <david@...nlining.org>,
 phone-devel@...r.kernel.org, ~postmarketos/upstreaming@...ts.sr.ht,
 linux-kernel@...r.kernel.org, linux-iio@...r.kernel.org,
 devicetree@...r.kernel.org, Mark Brown <broonie@...nel.org>, Jean Delvare
 <jdelvare@...e.com>, Guenter Roeck <linux@...ck-us.net>
Subject: Re: [PATCH v3 2/3] iio: adc: Add driver for Marvell 88PM886 PMIC
 ADC

On Fri, 5 Sep 2025 16:39:05 +0300
Andy Shevchenko <andy.shevchenko@...il.com> wrote:

> On Fri, Sep 5, 2025 at 2:01 PM Duje Mihanović <duje@...emihanovic.xyz> wrote:
> >
> > Marvell's 88PM886 PMIC has a so-called General Purpose ADC used for
> > monitoring various system voltages and temperatures. Add the relevant
> > register definitions to the MFD header and a driver for the ADC.  
> 
> Jonathan et al. The Q here is do we want to somehow make values
> spelling (in the comments and documentation) be unified with the
> Scientific Style [1]? Personally I find the article very useful and
> one style for the whole subsystem would be good to enforce (in my
> humble opinion). Thoughts?
> 
> [1]: https://poynton.ca/notes/units/

Consistency is indeed good, but I'm always careful about putting up
barriers to submission. So I'd prefer starting with guidance and general
review comments about consistent style, but probably not pushing back too
hard initially at least on new code where a consistent slightly different
style is applied. e.g. there is inconsistency in this patch as Volt and
Ampere get capitals but not Ohm (which should given all 3 are named
after people)

To actually move to such a style we'd need to fix up at least some existing
comments but that would be a trade off against churn.

I've been useless for years about putting an IIO maintainer entry profile in place.
https://docs.kernel.org/doc-guide/maintainer-profile.html but that is where
probably where this sort of extra documentation of specific requirements would belong
unless we can get broad agreement across subsystems where comments with units
are common.

Before moving to any standard on this I think we'd want to make sure
we at least don't 'clash' with what is accepted in other subsystems where
these units are common.  +CC some relevant maintainers.

Jonathan

> 
> ...
> 
> > +static int gpadc_get_raw(struct iio_dev *iio, enum pm886_gpadc_channel chan)
> > +{
> > +       struct pm886_gpadc *gpadc = iio_priv(iio);
> > +       __be16 buf;
> > +       int ret;
> > +
> > +       ret = regmap_bulk_read(gpadc->map, pm886_gpadc_regs[chan], &buf, sizeof(buf));
> > +       if (ret)
> > +               return ret;
> > +
> > +       return be16_to_cpu(buf) >> 4;  
> 
> + asm/byteorder.h (after linux/* but before linux/iio/*)
> 
> > +}  
> 
> ...
> 
> > +               /*
> > +                * Vendor kernel errors out above 1.25V, but testing shows that  
> 
> 1.25 V
> 
> > +                * the resistance of the battery detection channel (GPADC2 on
> > +                * coreprimevelte) reaches about 1.4Mohm when the battery is  
> 
> 1.4 MOhm or even 1.4 MΩ
> 
> 
> > +                * removed, which can't be measured with such a low upper
> > +                * limit. Therefore, to be able to detect the battery without
> > +                * ugly externs as used in the vendor fuelgauge driver,  
> 
> fuel gauge
> 
> > +                * increase this limit a bit.
> > +                */
> > +               if (WARN_ON(*raw_uV > 1500 * (MICRO / MILLI)))  
> 
> + bug.h
> 
> > +                       return -EIO;  
> 
> ...
> 
> > +               /*
> > +                * Vendor kernel errors out under 300mV, but for the same  
> 
> 300 mV
> 
> > +                * reason as above (except the channel hovers around 3.5kohm  
> 
> 3.5 kOhm / 3.5 kΩ
> 
> > +                * with battery present) reduce this limit.
> > +                */
> > +               if (*raw_uV < 200 * (MICRO / MILLI)) {
> > +                       dev_dbg(&iio->dev, "bad bias for chan %d: %duA @ %duV\n", chan->channel,  
> 
> Also add spaces before units.
> 
> > +                               *raw_uA, *raw_uV);
> > +                       continue;
> > +               }
> > +
> > +               dev_dbg(&iio->dev, "good bias for chan %d: %duA @ %duV\n", chan->channel,
> > +                       *raw_uA, *raw_uV);  
> 
> Ditto.
> 
> > +               return 0;
> > +       }
> > +
> > +       dev_err(&iio->dev, "failed to find good bias for chan %d\n", chan->channel);
> > +       return -EINVAL;
> > +}  
> 
> ...
> 
> > +       ret = DIV_ROUND_CLOSEST(raw_uV, raw_uA);  
> 
> + math.h
> 
> ...
> 
> > +       page = devm_i2c_new_dummy_device(dev, client->adapter,
> > +                                        client->addr + PM886_PAGE_OFFSET_GPADC);
> > +       if (IS_ERR(page))  
> 
> + err.h
> 
> > +               return dev_err_probe(dev, PTR_ERR(page), "Failed to initialize GPADC page\n");  
> 
> 

Powered by blists - more mailing lists