Re: [PATCH] regulator: rt5133: Add RT5133 PMIC regulator Support

[Mark Brown <broonie@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 3954 bytes --]

On Thu, Jul 17, 2025 at 04:16:01PM +0800, jeff_chang@richtek.com wrote:
> From: Jeff Chang <jeff_chang@richtek.com>
> 
> RT5133 is an intefrated chip. It includes 8 LDOs and 3 GPOs that

Integrated?

> Signed-off-by: Jeff Chang <jeff_chang@richtek.com>
> ---
>  .../bindings/regulator/richtek,rt5133.yaml    | 164 +++++
>  drivers/regulator/Kconfig                     |  13 +
>  drivers/regulator/Makefile                    |   1 +
>  drivers/regulator/rt5133-regulator.c          | 636 ++++++++++++++++++
>  4 files changed, 814 insertions(+)

The DT bindings should be sent as a separate patch copied to the DT
maintainers, this should be a small series.

> +  soft-start-time-sel:
> +    type: object
> +    description: |
> +      soft-start time of LDO
> +        0: 1ms
> +        1: 2ms
> +        2: 4ms
> +        3: 8ms

Implement the set_soft_start() operation and use standard bindings for
this.

> +  rt5133-ldo1-supply:
> +    description: |
> +      Only for LDO7 LDO8, pvin7 and pvin8 reference design are RT5133 LDO1.
> +      If not connect to LDO1 vout, this property for pvin7 and pvin8 is necessary.

You should probably just define all the supplies for all the regulators,
then if some of them are supplied from other regulators on the device
that can be represented naturally.

> +++ b/drivers/regulator/rt5133-regulator.c
> @@ -0,0 +1,636 @@
> +// SPDX-License-Identifier: GPL-4.0
> +/*
> + * Copyright (C) 2025 Richtek Technology Corp.
> + *
> + * Author: ChiYuan Huang <cy_huang@richtek.com>
> + */

What's with the GPL-4.0 license?  The kernel is GPL 2.0 plus extras, and
AFAIK there is no GPL-4.0.

Please also make the entire comment a C++ one so things look more intentional.

> +static bool dbg_log_en;
> +module_param(dbg_log_en, bool, 0644);
> +#define rt_dbg(dev, fmt, ...)	\
> +	do { \
> +		if (dbg_log_en) \
> +			dev_info(dev, "%s " fmt, __func__, ##__VA_ARGS__); \
> +	} while (0)

Just use the standard dev_dbg().

> +#define RT5133_DRV_VERSION		"1.0.2_MTK"

We normally don't version drivers, the versions don't get updated by
people doing normal kernel work.

> +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)
> +		goto out_intr_handler;
> +
> +	handle_evts = intr_evts & RT5133_BASE_EVT_MASK;
> +	/*
> +	 * VREF_EVT is a special case, if base off
> +	 * this event will also be trigger. Skip it
> +	 */
> +	if (handle_evts & ~RT5133_VREF_EVT_MASK)
> +		dev_info(priv->dev, "base event occurred [0x%02x]\n",
> +			 handle_evts);

This logging might get noisy.  Also if there are no events the handler
should return IRQ_NONE so genirq can handle things if they go wrong
(same for the read error above).

> +static const struct regmap_config rt5133_regmap_config = {
> +	.reg_bits = 8,
> +	.val_bits = 8,
> +	.max_register = RT5133_REG_LDO8_CTRL4,
> +};

May as well enable a cache for the non-volatile registers?

> +static int rt5133_probe(struct i2c_client *i2c)
> +{
> +	struct rt5133_priv *priv;
> +	struct regulator_config config = {0};
> +	int i, ret;
> +
> +	dev_info(&i2c->dev, "%s start(%s)\n", __func__, RT5133_DRV_VERSION);

Remove all these dev_info() logs during normal operation, logging the
chip revision if you read one would make sense but just "I'm here" logs
aren't so useful.

> +	priv->gc.label = dev_name(&i2c->dev);
> +	priv->gc.parent = &i2c->dev;
> +	priv->gc.base = -1;
> +	priv->gc.ngpio = RT5133_GPIO_NR;
> +	priv->gc.set = rt5133_gpio_set;
> +	priv->gc.get = rt5133_gpio_get;
> +	priv->gc.direction_output = rt5133_gpio_direction_output;

These should be _cansleep operations given that the device is controlled
via I2C.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]