Re: [PATCH v9 4/6] mfd: max77759: add Maxim MAX77759 core mfd driver

[André Draszik]

Hi Lee,

On Thu, 2025-05-08 at 15:02 +0100, Lee Jones wrote:
> 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@xxxxxxxxxx>
> > 
> > ---
> > 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.

Thanks :-)

> I assume the other patches depend on this one?

Yes, that is correct.
> 
> [...]
> 
> > 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.

I guess I might as well send another version, given this series is still
waiting for Srini's ACK.

Cheers,
Andre'