linux-kernel - RE: [PATCH v2 2/3] regulator: max77675: Add MAX77675 regulator driver

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] [day] [month] [year] [list]

Message-ID:
 <SN6PR03MB3903849DDFA8F4CC8EAB512C941AA@SN6PR03MB3903.namprd03.prod.outlook.com>
Date: Tue, 30 Sep 2025 01:35:10 +0000
From: "Na, Joan" <Joan.Na@...log.com>
To: Mark Brown <broonie@...nel.org>, Joan-Na-adi <joan.na.devcode@...il.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" <linux-kernel@...r.kernel.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        kernel test robot
	<lkp@...el.com>
Subject: RE: [PATCH v2 2/3] regulator: max77675: Add MAX77675 regulator driver

Dear Mark,

Thank you very much for reviewing the patches.
I really appreciate the valuable feedback from you. I'm currently working on incorporating all the comments and will prepare a v3 patch accordingly.
Thanks again for your time and support.

Best regards,  
Joan

-----Original Message-----
From: Mark Brown <broonie@...nel.org> 
Sent: Tuesday, September 30, 2025 2:39 AM
To: Joan-Na-adi <joan.na.devcode@...il.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; Na, Joan <Joan.Na@...log.com>; kernel test robot <lkp@...el.com>
Subject: Re: [PATCH v2 2/3] regulator: max77675: Add MAX77675 regulator driver

[External]

On Mon, Sep 29, 2025 at 07:56:17PM +0900, Joan-Na-adi wrote:
> This patch adds support for the Maxim Integrated MAX77675 PMIC regulator.
> 
> The MAX77675 is a compact, highly efficient SIMO (Single Inductor 
> Multiple Output)

This looks basically good, there's some review comments below but they're mostly cosmetic rather than substantial.

> Reported-by: kernel test robot <lkp@...el.com>
> Closes: 
> https://lore.kernel.org/oe-kbuild-all/20250927.cU4bEADk-lkp@intel.com/

There's no need to do this when you fixed a bug in a patch that was never applied, it's only relevant when fixing a bug in code that was merged.

> +static const struct regmap_config max77675_regmap_config = {
> +	.reg_bits = 8,
> +	.val_bits = 8,
> +	.max_register = MAX77675_MAX_REGISTER,
> +	.cache_type = REGCACHE_NONE,
> +};

_NONE is the default, no need to specify it.  Though it'll generally improve performance to have a cache so _MAPLE (plus a volatile_reg() op for the interrupt/status registers), it'll cut down on I2C traffic which is slow.

> +	/* Debug print all parsed values */
> +	pr_info("MAX77675 config parsed:\n"
> +		 "  dvs_slew_rate: %u\n"
> +		 "  latency_mode: %u\n"
> +		 "  drv_sbb_strength: %u\n"
> +		 "  manual_reset_time: %u\n"
> +		 "  en_pullup_disable: %u\n"
> +		 "  bias_low_power_request: %u\n"
> +		 "  simo_int_ldo_always_on: %u\n"
> +		 "  en_mode: %u\n"
> +		 "  en_debounce_time: %u\n",
> +		 config->drv_slew_rate,
> +		 config->latency_mode,
> +		 config->drv_sbb_strength,
> +		 config->manual_reset_time,
> +		 config->en_pullup_disable,
> +		 config->bias_low_power_request,
> +		 config->simo_int_ldo_always_on,
> +		 config->en_mode,
> +		 config->en_debounce_time);

This is a bit noisy, we don't tend to print the entire config out during boot.  It's also going to be formatted weirdly (eg, only a timestamp at the start).  I'd tend to drop this, or at most make it debug or vdebug level.

> +static void max77675_regulator_remove(struct i2c_client *client) {
> +	struct max77675_regulator *maxreg = i2c_get_clientdata(client);
> +
> +	dev_info(maxreg->dev, "MAX77675 regulators removed\n"); }

This is a bit noisy again.  In general the driver should be silent for non-error stuff or parsing ID information from the hardware, it makes it easier to find important informtion and with serial consoles lots of prints from many drivers can end up slowing boot noticably.