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

Message-ID: <aNrEIZBhh6PllyOy@finisterre.sirena.org.uk>
Date: Mon, 29 Sep 2025 18:38:41 +0100
From: Mark Brown <broonie@...nel.org>
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, Joan Na <joan.na@...log.com>,
	kernel test robot <lkp@...el.com>
Subject: Re: [PATCH v2 2/3] regulator: max77675: Add MAX77675 regulator driver

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.

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