Re: [PATCH 2/2] regulator: max20339: add Maxim MAX20339 regulator driver

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

Hi Krzysztof,

On Mon, 2024-09-16 at 22:06 +0200, Krzysztof Kozlowski wrote:
> 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?

You unplug the USB cable :-), but yes, I'll come up with something better.

> [...]
> > +}
> > +
> > +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.

Do you mean it doesn't enforce existence of this property? (It does and
binding check appropriately complains if it's missing). Otherwise, can
you please point me to the problem you're seeing?

From the binding:

+properties:
+  [...]
+  regulators:
+    type: object
+    [...]
+    patternProperties:
+      "^lsw[12]$":
+        [...]
+        properties:
+          [...]
+          shunt-resistor-micro-ohms:
+            [...]
+        required:
+          - shunt-resistor-micro-ohms
+
+        unevaluatedProperties: false
+
+    required:
+      - lsw1
+      - lsw2
+
+    additionalProperties: false
+
+[...]
+
+required:
+  [...]
+  - regulators

Anything wrong or missing in the above?

> > [...]
> > +	},                                                        \
> > +	.ovp_mask = _ovp_mask,                                    \
> > +	.status_reg = _status_reg,                                \
> > +}
> > +
> > +
> 
> Here and in few other places - just one blank line.

OK.

> > +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@linaro.org/

Thanks!

> [...]
> > +
> > +	irq_flags = IRQF_ONESHOT | IRQF_SHARED;
> 
> Why shared?

Just to be nice in case somebody puts it on a shared line. Not actually
required in my case.

> > +	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.

I wasn't aware of this, thanks. I'll drop the shared and somebody can
revisit it in the future if required. BTW, a naive grep returned +400
drivers that use shared together with devm.

> > [...]
> > +
> > +	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.

OK.

> > [...]
> > +
> > +	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

Oops, sure.

> > +			      "regmap init failed\n");
> > +		return PTR_ERR(regmap);
> > +	}

> > [...]
> > +
> > +#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.

OK.

Thanks Krzysztof!
Andre

> 
> Best regards,
> Krzysztof
>