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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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.

Attachment: signature.asc
Description: PGP signature


[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux