Re: [PATCH v3 1/2] regulator: rt5133: Add RT5133 PMIC regulator Support

[Mark Brown]

On Mon, Jul 21, 2025 at 02:01:34PM +0800, jeff_chang@xxxxxxxxxxx wrote:

> RT5133 is an intefrated chip. It includes 8 LDOs and 3 GPOs that
> can be used to drive output high/low purpose. The dependency of the
> GPO block is internally LDO1 Voltage. If LDO1 voltage output disabled,
> GPO cannot be used to drive output high. It need to pay more attention
> on the usage.

This looks mostly fine, a couple of very small nits below:

> +static irqreturn_t rt5133_intr_handler(int irq_number, void *data)
> +{
> +	struct rt5133_priv *priv = data;
> +	u32 intr_evts = 0, handle_evts;
> +	int i, ret;
> +
> +	ret = regmap_bulk_read(priv->regmap, RT5133_REG_BASE_EVT, &intr_evts,
> +			       RT5133_INTR_BYTE_NR);
> +	if (ret) {
> +		dev_err(priv->dev, "%s, read event failed\n", __func__);
> +		return IRQ_HANDLED;
> +	}
> +
> +	handle_evts = intr_evts & RT5133_BASE_EVT_MASK;

> +
> +	ret = regmap_bulk_write(priv->regmap, RT5133_REG_BASE_EVT, &intr_evts,
> +				RT5133_INTR_BYTE_NR);
> +	if (ret)
> +		dev_err(priv->dev, "%s, clear event failed\n", __func__);
> +
> +	return IRQ_HANDLED;

If there weren't any events flagged that we handled we should return
IRQ_NONE here, this supports shared interrupts and lets the error
handling in the interrupt core handle errors where the interrupt line
gets stuck.

> +// Due to crc, cache is not necessary
> +static const struct regmap_config rt5133_regmap_config = {
> +	.reg_bits = 8,
> +	.val_bits = 8,
> +	.max_register = RT5133_REG_LDO8_CTRL4,
> +};

Might be worth enabling a cache?

Attachment: signature.asc
Description: PGP signature