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

["André Draszik" <andre.draszik@linaro.org>]

Hi Christophe,

Thanks for your review!

On Mon, 2025-02-24 at 21:20 +0100, Christophe JAILLET wrote:
> Le 24/02/2025 à 11:28, André Draszik a écrit :
> > The Maxim MAX77759 is a companion Power Management IC for USB Type-C
> > applications with Battery Charger, Fuel Gauge, temperature sensors, USB
> > Type-C Port Controller (TCPC), NVMEM, and additional GPIO interfaces.
> > 
> > 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.
> 
> Hi,
> 
> ...
> 
> > +int max77759_maxq_command(struct max77759_mfd *max77759_mfd,
> > +			  const struct max77759_maxq_command *cmd,
> > +			  struct max77759_maxq_response *rsp)
> > +{
> > +	DEFINE_FLEX(struct max77759_maxq_response, _rsp, rsp, length, 1);
> > +	int ret;
> > +	static const unsigned int timeout_ms = 200;
> > +
> > +	if (cmd->length > MAX77759_MAXQ_REG_AP_MESSAGESZ_MAX)
> > +		return -EINVAL;
> > +
> > +	/* rsp is allowed to be NULL. In that case we do need a temporary. */
> > +	if (!rsp)
> > +		rsp = _rsp;
> > +
> > +	BUILD_BUG_ON(MAX77759_MAXQ_OPCODE_MAXLENGTH
> > +		     != MAX77759_MAXQ_REG_AP_MESSAGESZ_MAX);
> > +	if (!rsp->length || rsp->length > MAX77759_MAXQ_REG_AP_MESSAGESZ_MAX)
> > +		return -EINVAL;
> > +
> > +	guard(mutex)(&max77759_mfd->maxq_lock);
> > +
> > +	reinit_completion(&max77759_mfd->cmd_done);
> > +
> > +	/* write the opcode and data */
> > +	ret = regmap_bulk_write(max77759_mfd->regmap_maxq,
> > +				MAX77759_MAXQ_REG_AP_DATAOUT0, cmd->cmd,
> > +				cmd->length);
> > +	if (!ret && cmd->length < MAX77759_MAXQ_REG_AP_MESSAGESZ_MAX) {
> > +		/* writing the last byte triggers MaxQ */
> > +		ret = regmap_write(max77759_mfd->regmap_maxq,
> > +				   MAX77759_MAXQ_REG_AP_DATAOUT32, 0);
> > +	}
> > +	if (ret)
> > +		dev_warn(regmap_get_device(max77759_mfd->regmap_maxq),
> 
> Maybe regmap_get_device(max77759_mfd->regmap_maxq) could be assigned to 
> a variable to simplify its usage?

Sure, can do.

> 
> > +			 "write data failed: %d\n", ret);
> > +	if (ret)
> > +		return ret;
> 
> Merge with the if (ret) just above? (as done a few lines below)

Definitely, that was an oversight somehow :-(

> 
> > +
> > +	/* wait for response from MaxQ */
> > +	if (!wait_for_completion_timeout(&max77759_mfd->cmd_done,
> > +					 usecs_to_jiffies(timeout_ms))) {
> > +		dev_err(regmap_get_device(max77759_mfd->regmap_maxq),
> > +			"timed out waiting for data\n");
> > +		return -ETIMEDOUT;
> > +	}
> > +
> > +	ret = regmap_bulk_read(max77759_mfd->regmap_maxq,
> > +			       MAX77759_MAXQ_REG_AP_DATAIN0,
> > +			       rsp->rsp, rsp->length);
> > +	if (ret) {
> > +		dev_warn(regmap_get_device(max77759_mfd->regmap_maxq),
> > +			 "read data failed: %d\n", ret);
> > +		return ret;
> > +	}
> > +
> > +	/*
> > +	 * As per the protocol, the first byte of the reply will match the
> > +	 * request.
> > +	 */
> > +	if (rsp->rsp[0] != cmd->cmd[0]) {
> > +		dev_warn(regmap_get_device(max77759_mfd->regmap_maxq),
> > +			 "unexpected opcode response for %#.2x: %*ph\n",
> > +			 cmd->cmd[0], (int)rsp->length, rsp->rsp);
> > +		return -EIO;
> > +	}
> > +
> > +	return 0;
> > +}
> 
> ...
> 
> > +static int max77759_probe(struct i2c_client *client)
> > +{
> > +	struct regmap *regmap_top;
> > +	unsigned int pmic_id;
> > +	int ret;
> > +	struct irq_data *irq_data;
> > +	struct max77759_mfd *max77759_mfd;
> > +	unsigned long irq_flags;
> > +	struct regmap_irq_chip_data *irq_chip_data_pmic;
> > +
> > +	regmap_top = devm_regmap_init_i2c(client, &max77759_regmap_config_top);
> > +	if (IS_ERR(regmap_top))
> > +		return dev_err_probe(&client->dev, PTR_ERR(regmap_top),
> > +				     "regmap init failed\n");
> > +
> > +	ret = regmap_read(regmap_top, MAX77759_PMIC_REG_PMIC_ID, &pmic_id);
> > +	if (ret)
> > +		return dev_err_probe(&client->dev, ret,
> > +				     "Unable to read Device ID\n");
> > +
> > +	if (pmic_id != MAX77759_PMIC_REG_PMIC_ID_MAX77759)
> > +		return dev_err_probe(&client->dev, -ENODEV,
> > +				     "Unsupported Device ID %#.2x (%d)\n",
> > +				     pmic_id, pmic_id);
> > +
> > +	irq_data = irq_get_irq_data(client->irq);
> > +	if (!irq_data)
> > +		return dev_err_probe(&client->dev, -EINVAL,
> > +				     "Invalid IRQ: %d\n", client->irq);
> > +
> > +	max77759_mfd = devm_kzalloc(&client->dev, sizeof(*max77759_mfd),
> > +				    GFP_KERNEL);
> > +	if (!max77759_mfd)
> > +		return -ENOMEM;
> > +
> > +	max77759_mfd->regmap_top = regmap_top;
> > +	devm_mutex_init(&client->dev, &max77759_mfd->maxq_lock);
> 
> Error handling?

Thanks! There's a similar one in the gpio driver which
also needs fixing.

> 
> > +
> > +	i2c_set_clientdata(client, max77759_mfd);
> 
> Harmless, but is it needed? (there is no i2c_get_clientdata() in the flile)

Yes, this is needed. The leaf drivers need to access it, see
dev_get_drvdata(pdev->dev.parent) in those.

> 
> > +
> > +	for (int i = 0; i < ARRAY_SIZE(max77759_i2c_subdevs); ++i) {
> 
> Unusual. Maybe declare 'i' at the beginning of the function?

A naive grep returned > 1000 statements like this in the kernel
tree. It doesn't look like it's that unusual these days anymore.

> 
> > +		ret = max77759_create_i2c_subdev(client,
> > +						 max77759_mfd,
> > +						 &max77759_i2c_subdevs[i]);
> > +		if (ret)
> > +			return ret;
> > +	}
> > +
> > +	irq_flags = IRQF_ONESHOT | IRQF_SHARED;
> > +	irq_flags |= irqd_get_trigger_type(irq_data);
> > +
> > +	ret = devm_regmap_add_irq_chip(&client->dev, max77759_mfd->regmap_top,
> > +				       client->irq, irq_flags, 0,
> > +				       &max77759_pmic_irq_chip,
> > +				       &irq_chip_data_pmic);
> > +	if (ret)
> > +		return dev_err_probe(&client->dev, ret,
> > +				     "Failed to add IRQ chip\n");
> > +
> > +	/* INTSRC - MaxQ & children */
> > +	ret = max77759_add_chained_maxq(client, max77759_mfd,
> > +					irq_chip_data_pmic);
> > +	if (ret)
> > +		return ret;
> > +
> > +	/* INTSRC - topsys & children */
> > +	ret = max77759_add_chained_topsys(client, max77759_mfd,
> > +					  irq_chip_data_pmic);
> > +	if (ret)
> > +		return ret;
> > +
> > +	/* INTSRC - charger & children */
> > +	ret = max77759_add_chained_charger(client, max77759_mfd,
> > +					   irq_chip_data_pmic);
> > +	if (ret)
> > +		return ret;
> > +
> > +	return devm_mfd_add_devices(&client->dev, PLATFORM_DEVID_AUTO,
> > +				    max77759_cells, ARRAY_SIZE(max77759_cells),
> > +				    NULL, 0,
> > +				    regmap_irq_get_domain(irq_chip_data_pmic));
> > +}
> > +
> > +static const struct i2c_device_id max77759_i2c_id[] = {
> > +	{ "max77759", 0 },
> > +	{ }
> > +};
> > +MODULE_DEVICE_TABLE(i2c, max77759_i2c_id);
> > +
> > +static const struct of_device_id max77759_of_id[] = {
> > +	{ .compatible = "maxim,max77759", },
> > +	{},
> 
> Unneeded trailing comma after a terminator.
> Maybe { }  to match the style used in max77759_i2c_id?

OK. I'll also fix the similar leaf drivers.

Cheers,
Andre'


> 
> > +};
> > +MODULE_DEVICE_TABLE(of, max77759_of_id);
>