Re: [PATCH V2 3/6] mfd: nvvrs: add NVVRS PSEQ MFD driver

[Krzysztof Kozlowski <krzk@kernel.org>]

On Tue, May 20, 2025 at 09:08:29AM GMT, Shubhi Garg wrote:
> Add support for NVIDIA VRS (Voltage Regulator Specification) power
> sequencer device driver. This driver manages ON/OFF and suspend/resume
> power sequencing of system power rails for NVIDIA Tegra234 SoC. It also
> provides 32kHz RTC clock support with backup battery for system timing.
> 
> Signed-off-by: Shubhi Garg <shgarg@nvidia.com>
> ---
> 
> v2:
> - removed unnecessary error logs
> - changed dev_info to dev_dbg
> - changed dev_err to dev_err_probe
> - fixed "of_match_table" assignment
> 
>  drivers/mfd/Kconfig                 |  12 ++
>  drivers/mfd/Makefile                |   1 +
>  drivers/mfd/nvidia-vrs-pseq.c       | 270 ++++++++++++++++++++++++++++
>  include/linux/mfd/nvidia-vrs-pseq.h | 127 +++++++++++++
>  4 files changed, 410 insertions(+)
>  create mode 100644 drivers/mfd/nvidia-vrs-pseq.c
>  create mode 100644 include/linux/mfd/nvidia-vrs-pseq.h
> 
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index 6fb3768e3d71..3144b8f3eb9b 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -1437,6 +1437,18 @@ config MFD_SC27XX_PMIC
>  	  This driver provides common support for accessing the SC27xx PMICs,
>  	  and it also adds the irq_chip parts for handling the PMIC chip events.
>  
> +config MFD_NVVRS_PSEQ
> +	tristate "NVIDIA Voltage Regulator Specification Power Sequencer"
> +	depends on I2C=y

Why I2C cannot be a module? This is a module.

> +	select MFD_CORE
> +	select REGMAP_I2C
> +	select REGMAP_IRQ
> +	help
> +	  Say Y here to add support for NVIDIA Voltage Regulator Specification
> +	  Power Sequencer. NVVRS_PSEQ supports ON/OFF, suspend/resume sequence of
> +	  system power rails. It provides 32kHz RTC clock support with backup
> +	  battery for system timing.
> +

...

> +static int nvvrs_pseq_irq_clear(void *irq_drv_data)
> +{
> +	struct nvvrs_pseq_chip *chip = (struct nvvrs_pseq_chip *)irq_drv_data;
> +	struct i2c_client *client = chip->client;
> +	u8 reg, val;
> +	unsigned int i;
> +	int ret = 0;
> +
> +	/* Write 1 to clear the interrupt bit in the Interrupt
> +	 * Source Register, writing 0 has no effect, writing 1 to a bit
> +	 * which is already at 0 has no effect
> +	 */
> +
> +	for (i = 0; i < chip->irq_chip->num_regs; i++) {
> +		reg = (u8)(chip->irq_chip->status_base + i);
> +		ret = i2c_smbus_read_byte_data(client, reg);
> +		if (ret) {
> +			val = (u8)ret;
> +			dev_dbg(chip->dev, "Clearing interrupts! Interrupt status reg 0x%02x = 0x%02x\n",
> +				reg, val);

ratelimit

...

> +
> +static int nvvrs_pseq_probe(struct i2c_client *client)
> +{
> +	const struct regmap_config *rmap_config;
> +	struct nvvrs_pseq_chip *nvvrs_chip;
> +	const struct mfd_cell *mfd_cells;
> +	int n_mfd_cells;
> +	int ret;
> +
> +	nvvrs_chip = devm_kzalloc(&client->dev, sizeof(*nvvrs_chip), GFP_KERNEL);
> +	if (!nvvrs_chip)
> +		return -ENOMEM;
> +
> +	/* Set PEC flag for SMBUS transfer with PEC enabled */
> +	client->flags |= I2C_CLIENT_PEC;
> +
> +	i2c_set_clientdata(client, nvvrs_chip);
> +	nvvrs_chip->client = client;
> +	nvvrs_chip->dev = &client->dev;
> +	nvvrs_chip->chip_irq = client->irq;
> +	mfd_cells = nvvrs_pseq_children;
> +	n_mfd_cells = ARRAY_SIZE(nvvrs_pseq_children);
> +	rmap_config = &nvvrs_pseq_regmap_config;
> +	nvvrs_chip->irq_chip = &nvvrs_pseq_irq_chip;
> +
> +	nvvrs_chip->rmap = devm_regmap_init_i2c(client, rmap_config);
> +	if (IS_ERR(nvvrs_chip->rmap)) {
> +		ret = PTR_ERR(nvvrs_chip->rmap);

Useless assignment

> +		return dev_err_probe(nvvrs_chip->dev, ret,
> +				     "Failed to initialise regmap\n");
> +	}

Drop }

> +
> +	ret = nvvrs_pseq_vendor_info(nvvrs_chip);
> +	if (ret < 0)
> +		return ret;
> +
> +	nvvrs_pseq_irq_chip.irq_drv_data = nvvrs_chip;
> +	ret = devm_regmap_add_irq_chip(nvvrs_chip->dev, nvvrs_chip->rmap, client->irq,
> +				       IRQF_ONESHOT | IRQF_SHARED, 0,
> +				       &nvvrs_pseq_irq_chip,
> +				       &nvvrs_chip->irq_data);
> +	if (ret < 0) {
> +		return dev_err_probe(nvvrs_chip->dev, ret,
> +				     "Failed to add regmap irq\n");
> +	}

Drop }

Your entire code is full of that.

Please run scripts/checkpatch.pl on the patches and fix reported
warnings. After that, run also 'scripts/checkpatch.pl --strict' on the
patches and (probably) fix more warnings. Some warnings can be ignored,
especially from --strict run, but the code here looks like it needs a
fix. Feel free to get in touch if the warning is not clear.

Best regards,
Krzysztof