Re: [PATCH 2/2] regulator: adp5055: Add driver for adp5055

[Mark Brown]

On Tue, Feb 25, 2025 at 05:08:34PM +0800, Alexis Czezar Torreno wrote:

> Add ADI ADP5055 driver support. The device consists
> of 3 buck regulators able to connect to high input voltages of up to 18V
> with no preregulators.

There's a bunch of stuff here which should be using core features, I'll
comment some of those on the DT binding patch.

> +config REGULATOR_ADP5055
> +	tristate "Analog Devices ADP5055 Triple Buck Regulator"
> +	depends on I2C
> +        select REGMAP_I2C
> +	help
> +	  This driver controls an Analog Devices ADP5055 with triple buck
> +          regulators using an I2C interface.

The indentation for the select and the second line of the description is
weird.

> @@ -0,0 +1,490 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Regulator driver for Analog Devices ADP5055
> + *
> + * Copyright (C) 2025 Analog Devices, Inc.
> + */

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

> +static const struct regmap_range adp5055_reg_ranges[] = {
> +	regmap_reg_range(0xD1, 0xE0),
> +};
> +
> +static const struct regmap_access_table adp5055_write_ranges_table = {
> +	.yes_ranges	= adp5055_reg_ranges,
> +	.n_yes_ranges	= ARRAY_SIZE(adp5055_reg_ranges),
> +};
> +
> +static const struct regmap_access_table adp5055_read_ranges_table = {
> +	.yes_ranges	= adp5055_reg_ranges,
> +	.n_yes_ranges	= ARRAY_SIZE(adp5055_reg_ranges),
> +};

The read and write ranges could just be one variable.

> +static const struct regmap_config adp5055_regmap_config = {
> +	.reg_bits = 8,
> +	.val_bits = 8,
> +	.max_register = 0xFF,

max_register is set to 0xff but the largest read or write register is
0xe0.  Might be worth considering adding cache support too, there's
read/modify/write cycles here.

> +	if (!adp5055->hw_en_array_gpios)
> +		if (adp5055->hw_en_array_gpios->ndescs != ADP5055_NUM_CH)
> +			return dev_err_probe(dev, adp5055->hw_en_array_gpios->ndescs,
> +				     "Invalid amount of channels described\n");

Are we sure that ndescs is going to be an error code?

> +static int adp5055_en_func(struct regulator_dev *dev, int en_val)
> +{
> +	struct adp5055 *adp5055 = rdev_get_drvdata(dev);
> +	int id;
> +	int mask;
> +
> +	id = rdev_get_id(dev);
> +	mask = BIT(id);
> +
> +	if (!adp5055->hw_en_array_gpios)
> +		return regmap_update_bits(adp5055->regmap, ADP5055_CTRL_MODE1, mask, en_val);
> +
> +	gpiod_set_value_cansleep(adp5055->hw_en_array_gpios->desc[id], en_val);

Just use the standard GPIO and regmap helpers for this.

Attachment: signature.asc
Description: PGP signature