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
