Re: [PATCH v3 1/5] drivers: mfd: Add ADI MAX77541/MAX77540 PMIC Support

From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
To: Okan Sahin <okan.sahin@analog.com>
Cc: Lee Jones <lee@kernel.org>, Rob Herring <robh+dt@kernel.org>,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
	Liam Girdwood <lgirdwood@gmail.com>,
	Mark Brown <broonie@kernel.org>,
	Jonathan Cameron <jic23@kernel.org>,
	Lars-Peter Clausen <lars@metafoo.de>,
	Caleb Connolly <caleb.connolly@linaro.org>,
	Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>,
	William Breathitt Gray <william.gray@linaro.org>,
	Ramona Bolboaca <ramona.bolboaca@analog.com>,
	ChiYuan Huang <cy_huang@richtek.com>,
	linux-kernel@vger.kernel.org, devicetree@vger.kernel.org,
	linux-iio@vger.kernel.org
Subject: Re: [PATCH v3 1/5] drivers: mfd: Add ADI MAX77541/MAX77540 PMIC Support
Date: Wed, 18 Jan 2023 10:16:17 +0200	[thread overview]
Message-ID: <Y8eq0GtVZfVdNKYn@smile.fi.intel.com> (raw)
In-Reply-To: <20230118063822.14521-2-okan.sahin@analog.com>

On Wed, Jan 18, 2023 at 09:38:08AM +0300, Okan Sahin wrote:
> MFD driver for MAX77541/MAX77540 to enable its sub
> devices.
> 
> The MAX77541 is a multi-function devices. It includes
> buck converter and ADC.
> 
> The MAX77540 is a high-efficiency buck converter
> with two 3A switching phases.
> 
> They have same regmap except for ADC part of MAX77541.

...

> +/*
> + * Copyright (c) 2022 Analog Devices, Inc.

Happy New Year!

> + * Mfd core driver for the MAX77540 and MAX77541

MFD

> + */

...

> +	ret = devm_regmap_add_irq_chip(dev, max77541->regmap, irq,
> +				       IRQF_ONESHOT | IRQF_SHARED, 0,
> +				       &max77541_buck_irq_chip,
> +				       &max77541->irq_buck);
> +	if (ret)
> +		return ret;
> +
> +	if (max77541->id == MAX77541) {
> +		ret = devm_regmap_add_irq_chip(dev, max77541->regmap, irq,
> +					       IRQF_ONESHOT | IRQF_SHARED, 0,
> +					       &max77541_adc_irq_chip,
> +					       &max77541->irq_adc);
> +		if (ret)
> +			return ret;
> +	}

> +	return ret;

return 0;

...

> +		return devm_mfd_add_devices(dev, -1, max77540_devs,

PLATFORM_DEVID_NONE ?

> +					    ARRAY_SIZE(max77540_devs),
> +					    NULL, 0, NULL);

...

> +		return devm_mfd_add_devices(dev, -1, max77541_devs,

Ditto.

> +					    ARRAY_SIZE(max77541_devs),
> +					    NULL, 0, NULL);

...

> +static int max77541_i2c_probe(struct i2c_client *client,
> +			      const struct i2c_device_id *id)

No id, please. I.o.w. you should use ->probe_new().

> +{
> +	struct device *dev = &client->dev;
> +	struct max77541 *max77541;
> +	const void *match;
> +
> +	max77541 = devm_kzalloc(&client->dev, sizeof(*max77541), GFP_KERNEL);
> +	if (!max77541)
> +		return -ENOMEM;
> +
> +	i2c_set_clientdata(client, max77541);
> +	max77541->i2c = client;
> +
> +	match = device_get_match_data(dev);
> +	if (match)
> +		max77541->id = (enum max7754x_ids)match;

This is dangerous if your enum has 0 as a valid value.
Instead, use pointers in the driver_data, like

	&chip_info chip[MAX77540]

> +	else if (id)
> +		max77541->id = id->driver_data;

> +	else

It's better to check the ID range here.
Or since the change recommended above, check for NULL.

> +		return -ENODEV;
> +
> +	max77541->chip = &chip[max77541->id];
> +
> +	max77541->regmap = devm_regmap_init_i2c(client,
> +						&max77541_regmap_config);
> +	if (IS_ERR(max77541->regmap))
> +		return dev_err_probe(dev, PTR_ERR(max77541->regmap),
> +				     "Failed to allocate register map\n");
> +
> +	return max77541_pmic_setup(dev);
> +}

...

> +/*BITS OF REGISTERS*/

Missing spaces.

-- 
With Best Regards,
Andy Shevchenko