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:
 <BLAPR03MB5604248B31390D68AB180D8393242@BLAPR03MB5604.namprd03.prod.outlook.com>
Date: Fri, 15 Nov 2024 01:00:20 +0000
From: "Calam, Ramon Cristopher" <RamonCristopher.Calam@...log.com>
To: Mark Brown <broonie@...nel.org>
CC: "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "devicetree@...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

> -----Original Message-----
> From: Mark Brown <broonie@...nel.org>
> Sent: Friday, November 8, 2024 8:51 PM
> To: Calam, Ramon Cristopher <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
> 
> [External]
> 
> 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.

I will implement this.

> 
> > +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.

I will implement reg_read() and reg_write() operations in regmap to utilize
the standard helper.

> 
> > +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.

The device features UV/OC clamp registers for setting the maximum negative/positive output voltage. I've defined these values in the `adi,uv/ov-clamp-microvolt` property within the device tree, which necessitates adjusting the constraints in the driver. My idea is to utilize the `regulator-min/max-microvolt` property instead, thus eliminating the need for manual constraint modifications. Would this approach be appropriate? I'm also considering applying this method to the minimum and maximum output currents.

> 
> > +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.

I will implement this.

> 
> > +	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.

This is currently not optional but from the comment above, yes it could just be
tied off.

Thank you for reviewing the patch,

Best regards,
RC