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);
>
