linux-kernel - Re: [PATCH 1/2] regulator: lt8722: Add driver for LT8722

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: <2cce3ad8-7a3a-47db-a18c-33c32e96c009@sirena.org.uk>
Date: Fri, 8 Nov 2024 12:51:07 +0000
From: Mark Brown <broonie@...nel.org>
To: "Ramon Cristopher M. Calam" <ramoncristopher.calam@...log.com>
Cc: linux-kernel@...r.kernel.org, devicetree@...r.kernel.org,
	Liam Girdwood <lgirdwood@...il.com>, Rob Herring <robh@...nel.org>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>
Subject: Re: [PATCH 1/2] regulator: lt8722: Add driver for LT8722

On Fri, Nov 08, 2024 at 05:35:43PM +0800, Ramon Cristopher M. Calam wrote:
> Add ADI LT8722 full bridge DC/DC converter driver.

> +++ b/drivers/regulator/lt8722-regulator.c
> @@ -0,0 +1,701 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Analog Devices LT8722 Ultracompact Full Bridge Driver with SPI driver
> + *

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

> +static int lt8722_reg_read(struct spi_device *spi, u8 reg, u32 *val)
> +{

> +static int lt8722_reg_write(struct spi_device *spi, u8 reg, u32 val)
> +{

You can use these as reg_read() and reg_write() operations in regmap
which will allow the driver to use all the standard helpers and vastly
reduce the size of the driver.

> +static int lt8722_parse_fw(struct lt8722_chip_info *chip,
> +			   struct regulator_init_data *init_data)
> +{
> +	int ret;
> +
> +	/* Override the min_uV constraint with the minimum output voltage */
> +	init_data->constraints.min_uV = LT8722_MIN_VOUT;

Any modification of the constraints by the driver is a bug.  Adjust the
information the driver provides about the voltages it supports if you
need to do this.

> +static int lt8722_is_enabled(struct regulator_dev *rdev)
> +{
> +	struct lt8722_chip_info *chip = rdev_get_drvdata(rdev);
> +	int ret;
> +	u32 reg_val;
> +	bool en_req, en_pin;
> +
> +	ret = lt8722_reg_read(chip->spi, LT8722_SPIS_COMMAND, &reg_val);
> +	if (ret < 0)
> +		return ret;
> +
> +	en_req = FIELD_GET(LT8722_EN_REQ_MASK, reg_val);
> +	en_pin = gpiod_get_value(chip->en_gpio);
> +
> +	return en_req && en_pin;
> +}

Always adjusting both the GPIO and register all the time like this is
pointless, it turns the GPIO into just pure overhead.  Just use the
standard support for setting enables via registrers and GPIOs.  When
there's a GPIO leave the register permanently enabld.

> +	chip->en_gpio = devm_gpiod_get(&spi->dev, "enable", GPIOD_OUT_LOW);
> +	if (IS_ERR(chip->en_gpio))
> +		return PTR_ERR(chip->en_gpio);

Presumably this is optional, it could just be tied off.

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