linux-kernel - Re: [PATCH v9 4/6] mfd: max77759: add Maxim MAX77759 core mfd 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: <20250508140259.GN3865826@google.com>
Date: Thu, 8 May 2025 15:02:59 +0100
From: Lee Jones <lee@...nel.org>
To: André Draszik <andre.draszik@...aro.org>
Cc: Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>,
	Linus Walleij <linus.walleij@...aro.org>,
	Bartosz Golaszewski <brgl@...ev.pl>,
	Srinivas Kandagatla <srini@...nel.org>, Kees Cook <kees@...nel.org>,
	"Gustavo A. R. Silva" <gustavoars@...nel.org>,
	Peter Griffin <peter.griffin@...aro.org>,
	Tudor Ambarus <tudor.ambarus@...aro.org>,
	Will McVicker <willmcvicker@...gle.com>, kernel-team@...roid.com,
	linux-kernel@...r.kernel.org, devicetree@...r.kernel.org,
	linux-gpio@...r.kernel.org, linux-hardening@...r.kernel.org
Subject: Re: [PATCH v9 4/6] mfd: max77759: add Maxim MAX77759 core mfd driver

On Wed, 30 Apr 2025, André Draszik wrote:

> The Maxim MAX77759 is a companion PMIC for USB Type-C applications and
> includes Battery Charger, Fuel Gauge, temperature sensors, USB Type-C
> Port Controller (TCPC), NVMEM, and a GPIO expander.
> 
> Fuel Gauge and TCPC have separate and independent I2C addresses,
> register maps, and interrupt lines and are therefore excluded from the
> MFD core device driver here.
> 
> The GPIO and NVMEM interfaces are accessed via specific commands to the
> built-in microprocessor. This driver implements an API that client
> drivers can use for accessing those.
> 
> Signed-off-by: André Draszik <andre.draszik@...aro.org>
> 
> ---
> v6: really use postinc
> 
> v5:
> * update all (I hope) of Lee's comments:
> * file header C comment (not C++)
> * drop references to 'MFD'
> * extra indent register bit definitions
> * make 'struct max77759' public
> * drop comments that were used for visual separation only
> * drop MAX77759_*_REG_LAST_REGISTER defines
> * add comments to regmap ranges
> * use longer lines
> * sort local variable in reverse christmas tree order
> * update comments in max77759_maxq_command()
> * drop BUILD_BUG_ON()
> * use dev_err() in max77759_maxq_command()
> * reflow max77759_create_i2c_subdev() slightly and update error messages
> * drop useless comment in max77759_add_chained_maxq()
> * reflow max77759_probe()
> * consistent upper-/lower-case in messages
> 
> v4:
> * add missing build_bug.h include
> * update an irq chip comment
> * fix a whitespace in register definitions
> 
> v2:
> * add kernel doc for max77759_maxq_command() and related structs
> * fix an msec / usec typo
> * add missing error handling of devm_mutex_init() (Christophe)
> * align sentinel in max77759_of_id[] with max77759_i2c_id[]
>   (Christophe)
> * some tidy-ups in max77759_maxq_command() (Christophe)
> 
> max77759 Lee's updates
> ---
>  MAINTAINERS                  |   2 +
>  drivers/mfd/Kconfig          |  20 ++
>  drivers/mfd/Makefile         |   1 +
>  drivers/mfd/max77759.c       | 690 +++++++++++++++++++++++++++++++++++++++++++
>  include/linux/mfd/max77759.h | 165 +++++++++++
>  5 files changed, 878 insertions(+)

This looks okay to me now.

I assume the other patches depend on this one?

[...]

> diff --git a/drivers/mfd/max77759.c b/drivers/mfd/max77759.c
> new file mode 100644
> index 0000000000000000000000000000000000000000..15723ac3ef49771eafd5c2e9984abc550eec7aa1
> --- /dev/null
> +++ b/drivers/mfd/max77759.c
> @@ -0,0 +1,690 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright 2020 Google Inc
> + * Copyright 2025 Linaro Ltd.
> + *
> + * Core driver for Maxim MAX77759 companion PMIC for USB Type-C
> + */

[...]

> +int max77759_maxq_command(struct max77759 *max77759,
> +			  const struct max77759_maxq_command *cmd,
> +			  struct max77759_maxq_response *rsp)
> +{
> +	DEFINE_FLEX(struct max77759_maxq_response, _rsp, rsp, length, 1);
> +	struct device *dev = regmap_get_device(max77759->regmap_maxq);
> +	static const unsigned int timeout_ms = 200;
> +	int ret;
> +
> +	if (cmd->length > MAX77759_MAXQ_OPCODE_MAXLENGTH)
> +		return -EINVAL;
> +
> +	/*
> +	 * As a convenience for API users when issuing simple commands, rsp is
> +	 * allowed to be NULL. In that case we need a temporary here to write
> +	 * the response to, as we need to verify that the command was indeed
> +	 * completed correctly.
> +	 */
> +	if (!rsp)
> +		rsp = _rsp;
> +
> +	if (!rsp->length || rsp->length > MAX77759_MAXQ_OPCODE_MAXLENGTH)
> +		return -EINVAL;
> +
> +	guard(mutex)(&max77759->maxq_lock);
> +
> +	reinit_completion(&max77759->cmd_done);
> +
> +	/*
> +	 * MaxQ latches the message when the DATAOUT32 register is written. If
> +	 * cmd->length is shorter we still need to write 0 to it.
> +	 */
> +	ret = regmap_bulk_write(max77759->regmap_maxq,
> +				MAX77759_MAXQ_REG_AP_DATAOUT0, cmd->cmd,
> +				cmd->length);
> +	if (!ret && cmd->length < MAX77759_MAXQ_OPCODE_MAXLENGTH)
> +		ret = regmap_write(max77759->regmap_maxq,
> +				   MAX77759_MAXQ_REG_AP_DATAOUT32, 0);
> +	if (ret) {
> +		dev_err(dev, "writing command failed: %d\n", ret);
> +		return ret;
> +	}
> +
> +	/* wait for response from MaxQ */

If you have to respin this patch, please s/wait/Wait/.

If not, please send a subsequent patch.

-- 
Lee Jones [李琼斯]