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

[Mark Brown]

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).

Attachment: signature.asc
Description: PGP signature