> -----Original Message-----
> From: Mark Brown <broonie@xxxxxxxxxx>
> Sent: Tuesday, February 25, 2025 9:55 PM
> To: Torreno, Alexis Czezar <AlexisCzezar.Torreno@xxxxxxxxxx>
> Cc: Liam Girdwood <lgirdwood@xxxxxxxxx>; Rob Herring <robh@xxxxxxxxxx>;
> Krzysztof Kozlowski <krzk+dt@xxxxxxxxxx>; Conor Dooley
> <conor+dt@xxxxxxxxxx>; linux-kernel@xxxxxxxxxxxxxxx;
> devicetree@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH 2/2] regulator: adp5055: Add driver for adp5055
>
> [External]
>
> 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.
(Addressed on the dt binding patch email)
>
> > +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.
Will fix, it seems my I had used space rather than tabs for those
>
> > @@ -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.
Am not familiar with this, is this where each line use // rather than /**/?
>
> > +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.
Will simplify/merge
>
> > +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.
Will edit to 0xe0. Will also check on cache
>
> > + 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?
My mistake, this should be -EINVAL instead
>
> > +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.
Confused on this, I thought these were standard 'regmap_update_bits' and
'gpiod_set_value_cansleep'
