lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-Id: <DCGUXTSZ8B9G.2S2Q2JXYMBSRY@matfyz.cz>
Date: Sun, 31 Aug 2025 21:24:54 +0200
From: "Karel Balej" <balejk@...fyz.cz>
To: Duje Mihanović <duje@...emihanovic.xyz>,
        "Jonathan
 Cameron" <jic23@...nel.org>,
        "David Lechner" <dlechner@...libre.com>,
        Nuno Sá <nuno.sa@...log.com>,
        "Andy Shevchenko"
 <andy@...nel.org>, "Lee Jones" <lee@...nel.org>,
        "Rob Herring"
 <robh@...nel.org>,
        "Krzysztof Kozlowski" <krzk+dt@...nel.org>,
        "Conor
 Dooley" <conor+dt@...nel.org>
Cc: "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>
Subject: Re: [PATCH v2 2/3] iio: adc: Add driver for Marvell 88PM886 PMIC
 ADC

Duje Mihanović, 2025-08-31T12:33:05+02:00:
> 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.
>
> Signed-off-by: Duje Mihanović <duje@...emihanovic.xyz>
> ---
> v2:
> - default MFD_88PM886_PMIC
> - u8[2] -> __be16
> - Drop kernel.h include
> - Add pm886_gpadc struct
> - Reorder channel enum
> - Drop GPADC voltage channels
> - Drop unnecessary masking in gpadc_get_raw()
> - Extend gpadc_enable_bias() to allow disabling bias
> - usleep_range() -> fsleep()
> - PM wrapper for pm886_gpadc_read_raw()
> - Proper channel info: voltage is RAW | SCALE, temperature is RAW |
>   OFFSET | SCALE, resistance is PROCESSED
> - Explicitly define channels to en/disable in pm886_gpadc_setup()
> - Don't explicitly set iio->dev.parent
> - Miscellaneous style changes
> ---
>  MAINTAINERS                     |   5 +
>  drivers/iio/adc/88pm886-gpadc.c | 383 ++++++++++++++++++++++++++++++++++++++++
>  drivers/iio/adc/Kconfig         |  13 ++
>  drivers/iio/adc/Makefile        |   1 +
>  include/linux/mfd/88pm886.h     |  54 ++++++
>  5 files changed, 456 insertions(+)
>

[...]

> diff --git a/drivers/iio/adc/88pm886-gpadc.c b/drivers/iio/adc/88pm886-gpadc.c
> new file mode 100644
> index 0000000000000000000000000000000000000000..4622d2525e0edeed89c6e6d43336b177590aa885
> --- /dev/null
> +++ b/drivers/iio/adc/88pm886-gpadc.c

[...]

> +static int pm886_gpadc_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct pm886_chip *chip = dev_get_drvdata(dev->parent);
> +	struct i2c_client *client = chip->client;
> +	struct pm886_gpadc *gpadc;
> +	struct i2c_client *page;
> +	struct iio_dev *iio;
> +	int ret;
> +
> +	iio = devm_iio_device_alloc(dev, sizeof(*gpadc));
> +	if (!iio)
> +		return -ENOMEM;
> +
> +	gpadc = iio_priv(iio);
> +	dev_set_drvdata(dev, iio);
> +
> +	page = devm_i2c_new_dummy_device(dev, client->adapter,
> +					 client->addr + PM886_PAGE_OFFSET_GPADC);
> +	if (IS_ERR(page))
> +		return dev_err_probe(dev, PTR_ERR(page), "Failed to initialize GPADC page\n");
> +
> +	gpadc->map = devm_regmap_init_i2c(page, &pm886_gpadc_regmap_config);
> +	if (IS_ERR(gpadc->map))
> +		return dev_err_probe(dev, PTR_ERR(gpadc->map),
> +				     "Failed to initialize GPADC regmap\n");
> +
> +	iio->name = "88pm886-gpadc";
> +	iio->dev.of_node = dev->parent->of_node;

Didn't you mean to drop this since Jonathan pointed out that it's done
by the core?

> +	iio->modes = INDIO_DIRECT_MODE;
> +	iio->info = &pm886_gpadc_iio_info;
> +	iio->channels = pm886_gpadc_channels;
> +	iio->num_channels = ARRAY_SIZE(pm886_gpadc_channels);
> +
> +	pm_runtime_set_autosuspend_delay(dev, 50);
> +	pm_runtime_use_autosuspend(dev);
> +	ret = devm_pm_runtime_enable(dev);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to enable runtime PM\n");
> +
> +	ret = devm_iio_device_register(dev, iio);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to register ADC\n");
> +
> +	return 0;
> +}

[...]

> diff --git a/include/linux/mfd/88pm886.h b/include/linux/mfd/88pm886.h
> index 85eca44f39ab58ba4cb9ec4216118ee9604d021f..85c3c16fb10b7ee6aafdd6e68fd9135d8009eef8 100644
> --- a/include/linux/mfd/88pm886.h
> +++ b/include/linux/mfd/88pm886.h
> @@ -10,6 +10,7 @@
>  #define PM886_IRQ_ONKEY			0
>  
>  #define PM886_PAGE_OFFSET_REGULATORS	1
> +#define PM886_PAGE_OFFSET_GPADC		2
>  
>  #define PM886_REG_ID			0x00
>  
> @@ -67,6 +68,59 @@
>  #define PM886_REG_BUCK4_VOUT		0xcf
>  #define PM886_REG_BUCK5_VOUT		0xdd
>  
> +/* GPADC enable/disable registers */
> +#define PM886_REG_GPADC_CONFIG(n)	(n)
> +
> +/* GPADC channel registers */
> +#define PM886_REG_GPADC_VSC		0x40
> +#define PM886_REG_GPADC_VCHG_PWR	0x4c
> +#define PM886_REG_GPADC_VCF_OUT		0x4e
> +#define PM886_REG_GPADC_TINT		0x50
> +#define PM886_REG_GPADC_GPADC0		0x54
> +#define PM886_REG_GPADC_GPADC1		0x56
> +#define PM886_REG_GPADC_GPADC2		0x58
> +#define PM886_REG_GPADC_VBAT		0xa0
> +#define PM886_REG_GPADC_GND_DET1	0xa4
> +#define PM886_REG_GPADC_GND_DET2	0xa6
> +#define PM886_REG_GPADC_VBUS		0xa8
> +#define PM886_REG_GPADC_GPADC3		0xaa
> +#define PM886_REG_GPADC_MIC_DET		0xac
> +#define PM886_REG_GPADC_VBAT_SLP	0xb0
> +
> +/* GPADC channel enable bits */
> +#define PM886_GPADC_VSC_EN	BIT(0)
> +#define PM886_GPADC_VBAT_EN	BIT(1)
> +#define PM886_GPADC_GNDDET1_EN	BIT(3)
> +#define PM886_GPADC_VBUS_EN	BIT(4)
> +#define PM886_GPADC_VCHG_PWR_EN	BIT(5)
> +#define PM886_GPADC_VCF_OUT_EN	BIT(6)
> +#define PM886_GPADC_CONFIG1_EN_ALL (PM886_GPADC_VSC_EN | \
> +				    PM886_GPADC_VBAT_EN | \
> +				    PM886_GPADC_GNDDET1_EN | \
> +				    PM886_GPADC_VBUS_EN | \
> +				    PM886_GPADC_VCHG_PWR_EN | \
> +				    PM886_GPADC_VCF_OUT_EN)
> +
> +#define PM886_GPADC_TINT_EN	BIT(0)
> +#define PM886_GPADC_PMODE_EN	BIT(1)
> +#define PM886_GPADC_GPADC0_EN	BIT(2)
> +#define PM886_GPADC_GPADC1_EN	BIT(3)
> +#define PM886_GPADC_GPADC2_EN	BIT(4)
> +#define PM886_GPADC_GPADC3_EN	BIT(5)
> +#define PM886_GPADC_MIC_DET_EN	BIT(6)
> +#define PM886_GPADC_CONFIG2_EN_ALL (PM886_GPADC_TINT_EN | \
> +				    PM886_GPADC_GPADC0_EN | \
> +				    PM886_GPADC_GPADC1_EN | \
> +				    PM886_GPADC_GPADC2_EN | \
> +				    PM886_GPADC_GPADC3_EN | \
> +				    PM886_GPADC_MIC_DET_EN)
> +
> +/* No CONFIG3_EN_ALL because this is the only bit there */
> +#define PM886_GPADC_GND_DET2_EN	BIT(0)

I have previously ordered the definitions here to always have the
individual bits follow the register definition (see above REG_STATUS1
for instance). I don't know if this is a common practice, but it seemed
useful to me to make it clear which register the bits are found in
without having to look at their usage (even though in your case it is
clear from the _EN_ALL definition following them) and without making
them long by stuffing the register name in their names. So if you wanted
to follow this logic, the preceding three paragraphs should be moved
after the GPADC_CONFIG macro (since you used that to condense the
individual register definitions).

Also a nit, the above comment is I believe a full sentence, so it should
have a period at the end (I wouldn't mention it, but I seem to recall
that Lee was keen on this :-).

> +
> +#define PM886_GPADC_BIAS_LEVELS		16
> +#define PM886_GPADC_INDEX_TO_BIAS_UA(i)	(1 + (i) * 5)
> +
>  #define PM886_LDO_VSEL_MASK		0x0f
>  #define PM886_BUCK_VSEL_MASK		0x7f

It would also seem logical to me to keep these last two grouped with the
other regulators-related definitions.

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ