linux-kernel - RE: [PATCH 2/2] regulator: adp5055: Add driver for adp5055

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID:
 <SA1PR03MB634020464A151651A08ECAACF1C22@SA1PR03MB6340.namprd03.prod.outlook.com>
Date: Wed, 26 Feb 2025 02:24:58 +0000
From: "Torreno, Alexis Czezar" <AlexisCzezar.Torreno@...log.com>
To: Mark Brown <broonie@...nel.org>
CC: Liam Girdwood <lgirdwood@...il.com>, Rob Herring <robh@...nel.org>,
        Krzysztof Kozlowski <krzk+dt@...nel.org>,
        Conor Dooley <conor+dt@...nel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>
Subject: RE: [PATCH 2/2] regulator: adp5055: Add driver for adp5055



> -----Original Message-----
> From: Mark Brown <broonie@...nel.org>
> Sent: Tuesday, February 25, 2025 9:55 PM
> To: Torreno, Alexis Czezar <AlexisCzezar.Torreno@...log.com>
> Cc: Liam Girdwood <lgirdwood@...il.com>; Rob Herring <robh@...nel.org>;
> Krzysztof Kozlowski <krzk+dt@...nel.org>; Conor Dooley
> <conor+dt@...nel.org>; linux-kernel@...r.kernel.org;
> devicetree@...r.kernel.org
> 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'