On 16/09/2024 18:48, André Draszik wrote:
> The MAX20339 is an overvoltage protection (OVP) device which also
> integrates two load switches with resistor programmable current
> limiting and includes ESD protection for the USB Type-C signal pins.
>
> This driver exposes the main path and the two the load switches via the
...
> +
> +
> +static irqreturn_t max20339_irq(int irqno, void *data)
> +{
> + struct max20339_irq_data *max20339 = data;
> + struct device *dev = max20339->dev;
> + struct regmap *regmap = max20339->regmap;
> + u8 status[6];
> + int ret;
> +
> + ret = regmap_bulk_read(regmap, MAX20339_STATUS1, status,
> + ARRAY_SIZE(status));
> + if (ret) {
> + dev_err(dev, "Failed to read IRQ status: %d\n", ret);
> + return IRQ_NONE;
> + }
> +
> + dev_dbg(dev,
> + "INT1 2 3: %#.2x %#.2x %#.2x STATUS1 2 3: %#.2x %#.2x %#.2x\n",
> + status[3], status[4], status[5], status[0], status[1],
> + status[2]);
You should not have prints, even debugs, in interrupt handlers. This can
flood the dmesg.
> +
> + if (!status[3] && !status[4] && !status[5])
> + return IRQ_NONE;
> +
> + /* overall status */
> + if (status[3] & status[0] & MAX20339_THMFAULT) {
> + dev_warn(dev, "Thermal fault\n");
> + for (int i = 0; i < ARRAY_SIZE(max20339->rdevs); ++i)
> + regulator_notifier_call_chain(max20339->rdevs[i],
> + REGULATOR_EVENT_OVER_TEMP,
> + NULL);
> + }
> +
> + /* INSW status */
> + if ((status[3] & MAX20339_VINVALID)
> + && !(status[0] & MAX20339_VINVALID)) {
> + dev_warn(dev, "Vin over- or undervoltage\n");
Same with all these. What happens if interrupt is triggered constantly?
> + regulator_notifier_call_chain(max20339->rdevs[MAX20339_REGULATOR_INSW],
> + (REGULATOR_EVENT_UNDER_VOLTAGE_WARN
> + | REGULATOR_EVENT_OVER_VOLTAGE_WARN),
> + NULL);
> + }
> +
> + if (status[3] & status[0] & MAX20339_INOVFAULT) {
> + dev_warn(dev, "Over voltage on INput\n");
> + regulator_notifier_call_chain(max20339->rdevs[MAX20339_REGULATOR_INSW],
> + REGULATOR_EVENT_OVER_VOLTAGE_WARN,
> + NULL);
> + }
> +
...
> +
> +static int max20339_lsw_get_error_flags(struct regulator_dev *rdev,
> + unsigned int *flags)
> +{
> + struct max20339_regulator *data = rdev_get_drvdata(rdev);
> + unsigned int val;
> + int ret;
> +
> + ret = regmap_read(rdev_get_regmap(rdev), data->status_reg, &val);
> + if (ret) {
> + dev_err(rdev_get_dev(rdev),
> + "Failed to read MAX20339_STATUS%d: %d\n",
> + data->status_reg, ret);
> + return ret;
> + }
> +
> + *flags = 0;
> +
> + if (val & MAX20339_LSWxSHORTFAULT)
> + *flags |= REGULATOR_ERROR_OVER_CURRENT;
> +
> + if (val & MAX20339_LSWxOVFAULT)
> + *flags |= REGULATOR_ERROR_OVER_VOLTAGE_WARN;
> +
> + if (val & MAX20339_LSWxOCFAULT)
> + *flags |= REGULATOR_ERROR_OVER_CURRENT;
> +
> + return 0;
> +}
> +
> +static int max20339_lsw_dt_parse(struct device_node *np,
> + const struct regulator_desc *desc,
> + struct regulator_config *cfg)
> +{
> + struct max20339_regulator *data = cfg->driver_data;
> +
> + /* we turn missing properties into a fatal issue during probe() */
Your binding does not look in sync with above.
> + return of_property_read_u32(np, "shunt-resistor-micro-ohms",
> + &data->shunt_micro_ohm);
> +}
> +
> +static const struct regulator_ops max20339_lsw_ops = {
> + .enable = regulator_enable_regmap,
> + .disable = regulator_disable_regmap,
> + .is_enabled = max20339_lsw_is_enabled,
> +
> + .set_over_current_protection = max20339_lsw_set_ocp,
> + .set_over_voltage_protection = max20339_lsw_set_ovp,
> +
> + .get_error_flags = max20339_lsw_get_error_flags,
> +};
> +
> +#define MAX20339_LSW_DESC(_sfx, _enable_mask, _csel_mask, _ovp_mask, _status_reg) { \
> + .desc = { \
> + .name = "max20339-" _sfx, \
> + \
> + .ops = &max20339_lsw_ops, \
> + .type = REGULATOR_VOLTAGE, \
> + .supply_name = _sfx, \
> + \
> + .enable_reg = MAX20339_SWCNTL, \
> + .enable_mask = _enable_mask, \
> + \
> + .csel_reg = MAX20339_SWILIMDIV, \
> + .csel_mask = _csel_mask, \
> + \
> + .regulators_node = of_match_ptr("regulators"), \
> + .of_match = of_match_ptr(_sfx), \
> + .of_parse_cb = max20339_lsw_dt_parse, \
> + \
> + .enable_time = 11000 + 1100, \
> + .off_on_delay = 13, \
> + .poll_enabled_time = 2420, \
> + \
> + .owner = THIS_MODULE, \
> + }, \
> + .ovp_mask = _ovp_mask, \
> + .status_reg = _status_reg, \
> +}
> +
> +
Here and in few other places - just one blank line.
> +static struct max20339_regulator max20339_regulators[MAX20339_N_REGULATORS] = {
This can be const and then use container_of instead of rdev_get_drvdata().
See:
https://lore.kernel.org/all/20240909-regulator-const-v1-17-8934704a5787@xxxxxxxxxx/
> + [MAX20339_REGULATOR_INSW] = {
> + .desc = {
> + .name = "max20339-insw",
> +
> + .ops = &max20339_insw_ops,
> + .type = REGULATOR_VOLTAGE,
> + .supply_name = "insw",
> +
> + .volt_table = max20339_insw_volt_table,
> + .n_voltages = ARRAY_SIZE(max20339_insw_volt_table),
> +
> + .enable_reg = MAX20339_IN_CTR,
> + .enable_mask = MAX20339_IN_CTR_INSWEN,
> + .enable_val = FIELD_PREP_CONST(MAX20339_IN_CTR_INSWEN,
> + MAX20339_IN_CTR_INSWEN_AUTO),
> +
> + .active_discharge_reg = MAX20339_IN_CTR,
> + .active_discharge_mask = MAX20339_IN_CTR_INDISEN,
> + .active_discharge_on = MAX20339_IN_CTR_INDISEN,
> + .active_discharge_off = 0,
> +
> + .regulators_node = of_match_ptr("regulators"),
> + .of_match = of_match_ptr("insw"),
> +
> + .enable_time = 15000,
> + .off_on_delay = 1,
> + .poll_enabled_time = 3000,
> +
> + .owner = THIS_MODULE,
> + }
> + },
> + [MAX20339_REGULATOR_LSW1] = MAX20339_LSW_DESC("lsw1",
> + MAX20339_SWCNTL_LSW1EN,
> + MAX20339_SWILIMDIV_LSW1ILIMDIV,
> + MAX20339_SWCNTL_LSW1OVEN,
> + MAX20339_STATUS2),
> + [MAX20339_REGULATOR_LSW2] = MAX20339_LSW_DESC("lsw2",
> + MAX20339_SWCNTL_LSW2EN,
> + MAX20339_SWILIMDIV_LSW2ILIMDIV,
> + MAX20339_SWCNTL_LSW2OVEN,
> + MAX20339_STATUS3),
> +};
> +
> +static int max20339_setup_irq(struct i2c_client *client,
> + struct regmap *regmap,
> + struct regulator_dev *rdevs[])
> +{
> + u8 enabled_irqs[3];
> + struct max20339_irq_data *max20339;
> + int ret;
> + unsigned long irq_flags;
> +
> + /* the IRQ is optional */
> + if (!client->irq) {
> + enabled_irqs[0] = enabled_irqs[1] = enabled_irqs[2] = 0;
> + } else {
> + enabled_irqs[0] = (MAX20339_VINVALID | MAX20339_THMFAULT
> + | MAX20339_INOVFAULT);
> + enabled_irqs[1] = (MAX20339_LSWxSHORTFAULT
> + | MAX20339_LSWxOVFAULT
> + | MAX20339_LSWxOCFAULT);
> + enabled_irqs[2] = (MAX20339_LSWxSHORTFAULT
> + | MAX20339_LSWxOVFAULT
> + | MAX20339_LSWxOCFAULT);
> +
> + max20339 = devm_kzalloc(&client->dev, sizeof(*max20339),
> + GFP_KERNEL);
> + if (!max20339)
> + return -ENOMEM;
> + }
> +
> + ret = regmap_bulk_write(regmap, MAX20339_INTMASK1,
> + enabled_irqs, ARRAY_SIZE(enabled_irqs));
> + if (ret)
> + return dev_err_probe(&client->dev, ret,
> + "error configuring INTMASK1..3\n");
> +
> + if (!client->irq)
> + return 0;
> +
> + max20339->dev = &client->dev;
> + max20339->regmap = regmap;
> + memcpy(max20339->rdevs, rdevs, sizeof(max20339->rdevs));
> +
> + irq_flags = IRQF_ONESHOT | IRQF_SHARED;
Why shared?
> + irq_flags |= irqd_get_trigger_type(irq_get_irq_data(client->irq));
> +
> + ret = devm_request_threaded_irq(&client->dev, client->irq,
Shared interrupts should not be devm. It leads to tricky cases during
removal. If you investigated the code and you are 100% sure there is no
issue, please write a short comment in the code confirming that. Or just
don't use devm.
> + NULL, max20339_irq, irq_flags,
> + "max20339", max20339);
> + if (ret)
> + return dev_err_probe(&client->dev, ret,
> + "IRQ setup failed\n");
> +
> + return 0;
> +}
> +
> +#if IS_ENABLED(CONFIG_GPIO_REGMAP)
> +static int max20339_gpio_regmap_xlate(struct gpio_regmap *const gpio,
> + unsigned int base, unsigned int offset,
> + unsigned int *const reg,
> + unsigned int *const mask)
> +{
> + if (offset != 0)
> + return -EINVAL;
> +
> + *reg = base;
> + *mask = MAX20339_VINVALID;
> + return 0;
> +}
> +
> +static int max20339_setup_gpio(struct i2c_client *client,
> + struct regmap *regmap)
> +{
> + struct fwnode_handle *fwnode;
> + static const char * const names[] = { "vin" };
> + int ret;
> +
> + fwnode = gpiochip_node_get_first(&client->dev);
> + if (!fwnode) {
> + dev_info(&client->dev, "Skipping gpio chip initialization\n");
> + return 0;
> + }
> +
> + ret = PTR_ERR_OR_ZERO(devm_gpio_regmap_register(&client->dev,
> + &(struct gpio_regmap_config) {
> + .parent = &client->dev,
> + .regmap = regmap,
> + .fwnode = fwnode,
> + .ngpio = ARRAY_SIZE(names),
> + .names = names,
> + .reg_dat_base = MAX20339_STATUS1,
> + .reg_mask_xlate = max20339_gpio_regmap_xlate
> + }));
That's not really readable. Please split PTR_ERR_OR_ZERO.
> + fwnode_handle_put(fwnode);
> + if (ret)
> + return dev_err_probe(&client->dev, ret,
> + "failed to initialize gpio chip\n");
> +
> + return 0;
> +}
> +#else /* CONFIG_GPIO_REGMAP */
> +static int max20339_setup_gpio(struct i2c_client *client,
> + struct regmap *regmap)
> +{
> + return 0;
> +}
> +#endif /* CONFIG_GPIO_REGMAP */
> +
> +static int max20339_probe(struct i2c_client *client)
> +{
> + int ret;
> + struct regmap *regmap;
> + struct regulator_config config = {};
> + struct regulator_dev *rdev;
> + struct regulator_dev *rdevs[MAX20339_N_REGULATORS];
> +
> + /*
> + * either "dig-supply" is needed, or alternatively "in-supply" will
> + * supply power
> + */
> + ret = devm_regulator_get_enable_optional(&client->dev, "dig");
> + if (ret) {
> + if (ret == -ENODEV)
> + ret = devm_regulator_get_enable_optional(&client->dev,
> + "insw");
> + if (ret)
> + return dev_err_probe(&client->dev, ret,
> + "failed to get regulator");
> + }
> +
> + regmap = devm_regmap_init_i2c(client, &max20339_regmap_config);
> + if (IS_ERR(regmap)) {
> + dev_err_probe(&client->dev, PTR_ERR(regmap),
return dev_err_probe
> + "regmap init failed\n");
> + return PTR_ERR(regmap);
> + }
> +
> + for (int i = 0; i < ARRAY_SIZE(max20339_regulators); ++i) {
> + config.dev = &client->dev;
> + config.regmap = regmap;
> + config.driver_data = &max20339_regulators[i];
> +
> + rdev = devm_regulator_register(config.dev,
> + &max20339_regulators[i].desc,
> + &config);
> + if (IS_ERR(rdev))
> + return dev_err_probe(&client->dev, PTR_ERR(rdev),
> + "failed to register MAX20339 regulator %s\n",
> + max20339_regulators[i].desc.name);
> +
> + /*
> + * For the LSWs, we really need to know the shunts' values
> + * (from DT). Fail if missing.
> + */
> + if (max20339_regulators[i].desc.csel_mask
> + && !max20339_regulators[i].shunt_micro_ohm)
> + return dev_err_probe(&client->dev, -EINVAL,
> + "property 'shunt-resistor-micro-ohms' not found\n");
> +
> + rdevs[i] = rdev;
> +
> + dev_info(&client->dev, "registered MAX20339 regulator %s\n",
> + max20339_regulators[i].desc.name);
> + }
> +
> + ret = max20339_setup_irq(client, regmap, rdevs);
> + if (ret < 0)
> + return ret;
> +
> + return max20339_setup_gpio(client, regmap);
> +}
> +
> +static const struct i2c_device_id max20339_i2c_id[] = {
> + { "max20339", 0 },
> + { }
> +};
> +MODULE_DEVICE_TABLE(i2c, max20339_i2c_id);
> +
> +#ifdef CONFIG_OF
> +static const struct of_device_id max20339_of_id[] = {
> + { .compatible = "maxim,max20339", },
> + {},
> +};
> +MODULE_DEVICE_TABLE(of, max20339_of_id);
> +#endif
> +
> +static struct i2c_driver max20339_i2c_driver = {
> + .driver = {
> + .name = "max20339",
> + .probe_type = PROBE_PREFER_ASYNCHRONOUS,
> + .of_match_table = of_match_ptr(max20339_of_id),
Drop of_match_ptr and earlier #ifdef. Not much benefits and limits usage
to OF-systems.
Best regards,
Krzysztof
