| 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
| ||
|
Message-ID: <614644f9-71f7-41af-af90-7ae892581646@wanadoo.fr>
Date: Mon, 24 Feb 2025 21:20:11 +0100
From: Christophe JAILLET <christophe.jaillet@...adoo.fr>
To: André Draszik <andre.draszik@...aro.org>
Cc: brgl@...ev.pl, conor+dt@...nel.org, devicetree@...r.kernel.org,
gustavoars@...nel.org, kees@...nel.org, kernel-team@...roid.com,
krzk+dt@...nel.org, lee@...nel.org, linus.walleij@...aro.org,
linux-gpio@...r.kernel.org, linux-hardening@...r.kernel.org,
linux-kernel@...r.kernel.org, peter.griffin@...aro.org, robh@...nel.org,
srinivas.kandagatla@...aro.org, tudor.ambarus@...aro.org,
willmcvicker@...gle.com
Subject: Re: [PATCH 4/6] mfd: max77759: add Maxim MAX77759 core mfd driver
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?
> + "write data failed: %d\n", ret);
> + if (ret)
> + return ret;
Merge with the if (ret) just above? (as done a few lines below)
> +
> + /* 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?
> +
> + i2c_set_clientdata(client, max77759_mfd);
Harmless, but is it needed? (there is no i2c_get_clientdata() in the flile)
> +
> + for (int i = 0; i < ARRAY_SIZE(max77759_i2c_subdevs); ++i) {
Unusual. Maybe declare 'i' at the beginning of the function?
> + 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?
> +};
> +MODULE_DEVICE_TABLE(of, max77759_of_id);
Powered by blists - more mailing lists