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: <a7f7d4dc-283a-40b9-bb1b-0bc8aceb99c1@sirena.org.uk>
Date: Tue, 25 Feb 2025 13:54:41 +0000
From: Mark Brown <broonie@...nel.org>
To: Alexis Czezar Torreno <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

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.

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)