Message-ID: <5ad81ed43d8bb2426c9ae7d22fdb4c7aeb905129.camel@linaro.org>
Date: Tue, 17 Sep 2024 12:41:16 +0100
From: André Draszik <andre.draszik@...aro.org>
To: Mark Brown <broonie@...nel.org>
Cc: Liam Girdwood <lgirdwood@...il.com>, Rob Herring <robh@...nel.org>, 
 Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
 <conor+dt@...nel.org>, Michael Walle <mwalle@...nel.org>, Peter Griffin
 <peter.griffin@...aro.org>, Tudor Ambarus <tudor.ambarus@...aro.org>, Will
 McVicker <willmcvicker@...gle.com>,  kernel-team@...roid.com,
 linux-kernel@...r.kernel.org,  devicetree@...r.kernel.org
Subject: Re: [PATCH 2/2] regulator: max20339: add Maxim MAX20339 regulator
 driver

Hi Mark,

Thanks for the review!

On Tue, 2024-09-17 at 11:19 +0200, Mark Brown wrote:
> On Mon, Sep 16, 2024 at 05:48:53PM +0100, André Draszik wrote:
> 
> > +config REGULATOR_MAX20339
> > +       tristate "Maxim MAX20339 overvoltage protector with load switches"
> > +       depends on GPIOLIB || COMPILE_TEST
> > +       depends on I2C
> > +       select GPIO_REGMAP if GPIOLIB
> 
> I don't see any dependency on gpiolib here, the GPIO functionality
> appears unrelated to the regulator functionality (this could reasonably
> be a MFD, though it's probably not worth it given how trivial the GPIO
> functionality is).

Yes, it's very trivial and I opted to go the simpler path without MFD.

The alternative is just
         depends on GPIO_REGMAP || COMPILE_TEST
which doesn't appear used at all in the tree, so I opted for the above
instead.

I'll change it the dependency line

> 
> > +++ b/drivers/regulator/max20339-regulator.c
> > @@ -0,0 +1,912 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Copyright (C) 2024 Linaro Ltd.
> 
> Nothing inherited from the original Pixel 6 kernel?

No, not for this one.

> > + *
> > + * Maxim MAX20339 load switch with over voltage protection
> 
> Please make the entire comment a C++ one so things look more
> intentional.
> 
> > +static const struct regmap_config max20339_regmap_config = {
> > +	.reg_bits = 8,
> > +	.val_bits = 8,
> > +	.max_register = MAX20339_LAST_REGISTER,
> > +	.wr_table = &max20339_write_table,
> > +	.rd_table = &max20339_rd_table,
> > +	.volatile_table = &max20339_volatile_table,
> > +	.precious_table = &max20339_precious_table,
> > +};
> 
> You've specified volatile registers here but not configured a cache.

Yes, cache didn't seem worthwhile, but I wanted to document the volatile
registers nonetheless.

I'll enable the cache.

> > +	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);
> > +	}
> 
> This is an error on the input, not an error from this regulator, so the
> notification isn't appropriate here.

The input is usually a USB plug / cable. Is there a better option to report
this? I guess I could register a power supply.

> > +static int max20339_insw_is_enabled(struct regulator_dev *rdev)
> > +{
> > +	unsigned int val;
> > +	int ret;
> > +	struct device *dev = rdev_get_dev(rdev);
> > +
> > +	ret = regmap_read(rdev_get_regmap(rdev), MAX20339_STATUS1, &val);
> > +	if (ret) {
> > +		dev_err(dev, "error reading STATUS1: %d\n", ret);
> > +		return ret;
> > +	}
> > +
> > +	dev_dbg(dev, "%s: %s: %c\n", __func__, rdev->desc->name,
> > +		"ny"[FIELD_GET(MAX20339_INSWCLOSED, val)]);
> 
> In addition to the log spam issues I've no idea how anyone is supposed
> to interpret this log :/

I'll remove it, it doesn't add much value.

> > +
> > +	return FIELD_GET(MAX20339_INSWCLOSED, val) == 1;
> > +}
> 
> This does not appear to be an enable control, it's reading back a status
> register rather than turning on or off a regulator.

This is the regulator_ops::is_enabled() callback, shouldn't it return the
status in effect? It's required to return effective status for one of the
code paths in _regulator_do_enable(), when .poll_enabled_time is != 0.

> It's not clear to
> me what the status actually is (possibly saying if there's a voltage
> present?)

To enable, one writes to MAX20339_IN_CTR. While one can read back that
register, it doesn't reflect the actual status (e.g. it takes time to
take effect), so it has this MAX20339_STATUS1 register to inform us if
the output is actually enabled (switch closed or open).

On top of that, yes, the switch will also open if the input disappears
(cable unplug), this also is reflected in MAX20339_STATUS1 only.

So this regulator_ops::is_enabled() callback returns whether or not
it's open or closed - it returns the status that is in effect.

>  but it should be reported with a get_status() operation.

I missed ::get_status(), I'll implement it.

> > +static int max20339_set_voltage_sel(struct regulator_dev *rdev,
> > +				    unsigned int sel)
> > +{
> > +	return max20339_set_ovlo_helper(rdev,
> > +					FIELD_PREP(MAX20339_OVLOSEL_INOVLOSEL,
> > +						   sel));
> > +}
> 
> This device does not appear to be a voltage regualtor, it is a
> protection device.  A set_voltage() operation is therfore inappropriate
> for it, any voltage configuration would need to be done on the parent
> regulator.

This is handling one of the switches, and the input usually is
a USB plug / cable.

Based on the use-case (peripheral / OTG / wireless charging), the
overvoltage voltage needs to be modified at runtime for full
protection.

The set-voltage APIs seemed like a good fit for that, given the
regulator APIs allow setting those thresholds already (during init).

I'll see if I could maybe add a power supply as the parent and leave out
all the voltage and current related settings here altogether and make it
just control the switches, like some other regulator drivers do.

> 
> > +static const struct regulator_ops max20339_insw_ops = {
> > +	.enable = regulator_enable_regmap,
> > +	.disable = regulator_disable_regmap,
> > +	.is_enabled = max20339_insw_is_enabled,
> 
> The is_enabled() operation should match the enable() and disable(), it
> should reflect what the device is being told to do.

That wouldn't match _regulator_do_enable(), which requires .is_enabled()
to return the status in effect rather than the requested status, when
.poll_enabled_time is != 0.


> > +static int max20339_lsw_is_enabled(struct regulator_dev *rdev)
> > +{
> > +	struct max20339_regulator *data = rdev_get_drvdata(rdev);
> > +	unsigned int val;
> > +	int ret;
> > +	struct device *dev = rdev_get_dev(rdev);
> > +
> > +	ret = regmap_read(rdev_get_regmap(rdev), data->status_reg, &val);
> > +	if (ret) {
> > +		dev_err(dev, "error reading STATUS%d: %d\n",
> > +			data->status_reg, ret);
> > +		return ret;
> > +	}
> 
> Same issues here.

See above.

> 
> > +	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;
> 
> These statuses should be flagged ot the core.

OK

> 
> > +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;
> 
> Please just write a normal series of assignments, it's much clearer.

OK

> `
> > +		dev_info(&client->dev, "registered MAX20339 regulator %s\n",
> > +			 max20339_regulators[i].desc.name);
> 
> This is just noise, remove it.

OK

Thanks,
Andre'

Powered by blists - more mailing lists