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

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
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <ZulJuCu-QcMYrphP@finisterre.sirena.org.uk>
Date: Tue, 17 Sep 2024 11:19:52 +0200
From: Mark Brown <broonie@...nel.org>
To: André Draszik <andre.draszik@...aro.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

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

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

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

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

> +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 :/

> +
> +	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.  It's not clear to
me what the status actually is (possibly saying if there's a voltage
present?) but it should be reported with a get_status() operation.

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

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

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

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

> +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.
`
> +		dev_info(&client->dev, "registered MAX20339 regulator %s\n",
> +			 max20339_regulators[i].desc.name);

This is just noise, remove it.

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)