linux-kernel - Re: [PATCH v1 2/2] regulator: pf0900: Add PMIC PF0900 support

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: <386754b3-8c66-4d20-84ed-87a6052b979f@sirena.org.uk>
Date: Tue, 17 Jun 2025 12:44:43 +0100
From: Mark Brown <broonie@...nel.org>
To: Joy Zou <joy.zou@....com>
Cc: lgirdwood@...il.com, robh@...nel.org, krzk+dt@...nel.org,
	conor+dt@...nel.org, linux-kernel@...r.kernel.org,
	devicetree@...r.kernel.org, imx@...ts.linux.dev, frank.li@....com,
	ye.li@....com, ping.bai@....com, aisheng.dong@....com
Subject: Re: [PATCH v1 2/2] regulator: pf0900: Add PMIC PF0900 support

On Tue, Jun 17, 2025 at 06:20:25PM +0800, Joy Zou wrote:

> @@ -0,0 +1,1033 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright 2025 NXP.
> + * NXP PF0900 pmic driver
> + */
> +

Please make the entire comment block a C++ one so things look more
consistent.

> +static int pf0900_pmic_write(struct pf0900 *pf0900, unsigned int reg,
> +			     unsigned int val, uint8_t mask)
> +{
> +	unsigned int rxBuf;
> +	uint8_t data[2];
> +	int ret;
> +
> +	if (!pf0900 || !pf0900->dev)
> +		return -EINVAL;
> +
> +	if (reg >= PF0900_MAX_REGISTER) {
> +		dev_err(pf0900->dev, "Invalid register address: 0x%x\n", reg);
> +		return -EINVAL;
> +	}
> +
> +	/* If not updating entire register, perform a read-mod-write */
> +	data[0] = val;

Having a write operation that includes update_bits() functionality is a
bit confusing.  In general there's a lot of register I/O code in the
driver, and open coded copies of the generic regulator regmap helpers.
You'd save a lot of code by providing a regmap that implements
reg_read() and reg_write() operations, either stack another regmap
inside for the physical I/O or just use I2C SMBus operations.  You'd
also be able to use a cache then, and you'd get all the regmap
diagnostic infrastructure.

> +static int find_closest_bigger(unsigned int target, const unsigned int *table,
> +			       unsigned int num_sel, unsigned int *sel)

This should not be open coded in a specific driver.

> +static irqreturn_t pf0900_irq_handler(int irq, void *data)
> +{

> +	ret = pf0900_pmic_read(pf0900, PF0900_REG_SYSTEM_INT, &system);
> +	if (ret < 0) {
> +		dev_err(pf0900->dev, "Failed to read SYSTEM_INT(%d)\n", ret);
> +		return IRQ_NONE;
> +	}
> +
> +	ret = pf0900_pmic_read(pf0900, PF0900_REG_STATUS1_INT, &status1);

This smells a lot like the system interrupt might tell you if there's
any need to read the specific status interrupts?

> +	ret = pf0900_pmic_write(pf0900, PF0900_REG_STATUS1_INT, status1, status1);
> +	if (ret < 0) {
> +		dev_err(pf0900->dev, "Failed to write STATUS1_INT(%d)\n", ret);
> +		return IRQ_NONE;
> +	}

We're unconditionally acking any interrupt we see even if we didn't
understand them, limiting the ability of the genirq core to manage
unknown interrupts.

> +	if (system & IRQ_EWARN)
> +		dev_warn(pf0900->dev, "EWARN interrupt.\n");

It's not clear what this is but it should probably generate a regulator
notification?

> +	if (system & IRQ_GPIO)
> +		dev_warn(pf0900->dev, "GPIO interrupt.\n");

This should be a normal interrupt, though you didn't wire up the GPIOs
as GPIOs.

> +	if (system & IRQ_OV)
> +		dev_warn(pf0900->dev, "OV interrupt.\n");
> +
> +	if (system & IRQ_UV)
> +		dev_warn(pf0900->dev, "UV interrupt.\n");
> +
> +	if (system & IRQ_ILIM)
> +		dev_warn(pf0900->dev, "ILIM interrupt.\n");

These should definitely be generating regulator notifications, as should
some of the others probably (eg, the OV and thermal ones).

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)