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.
Attachment:
signature.asc
Description: PGP signature
