Messages in this thread | | | Date | Tue, 12 Jul 2022 10:30:41 +0300 | From | Matti Vaittinen <> | Subject | Re: [PATCH 3/5] regulator: max597x: Add support for max597x regulator |
| |
On 7/5/22 15:22, Naresh Solanki wrote: > From: Patrick Rudolph <patrick.rudolph@9elements.com> > > max597x is hot swap controller. > This regulator driver controls the same & also configures fault > protection features supported by the chip. > > Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com> > Signed-off-by: Marcello Sylvester Bauer <sylv@sylv.io> > Signed-off-by: Naresh Solanki <Naresh.Solanki@9elements.com>
I like the way the IRQ helpers have been used here. It'd be cool to hear how the rest of the system you're dealing with utilize the WARN level events :)
> +static int max597x_set_ocp(struct regulator_dev *rdev, int lim_uA, > + int severity, bool enable) > +{ > + int ret, val, reg; > + unsigned int vthst, vthfst; > + > + struct max597x_regulator *data = rdev_get_drvdata(rdev); > + int rdev_id = rdev_get_id(rdev); > + /* > + * MAX5970 doesn't has enable control for ocp. > + * If limit is specified but enable is not set then hold the value in > + * variable & later use it when ocp needs to be enabled. > + */
Is this a possible scenario? I think that if a non zero limit is given in a "regulator-oc-protection-microamp"-property, then the protection should always be enabled. Am I overlooking something?
> + if (lim_uA != 0 && lim_uA != data->lim_uA) > + data->lim_uA = lim_uA; > + > + if (severity != REGULATOR_SEVERITY_PROT) > + return -EINVAL; > + > + if (enable) { > + > + /* Calc Vtrip threshold in uV. */ > + vthst = > + div_u64(mul_u32_u32(data->shunt_micro_ohms, data->lim_uA), > + 1000000); > + > + /* > + * As recommended in datasheed, add 20% margin to avoid > + * spurious event & passive component tolerance. > + */ > + vthst = div_u64(mul_u32_u32(vthst, 120), 100); > + > + /* Calc fast Vtrip threshold in uV */ > + vthfst = vthst * (MAX5970_FAST2SLOW_RATIO / 100); > + > + if (vthfst > data->irng) { > + dev_err(&rdev->dev, "Current limit out of range\n"); > + return -EINVAL; > + } > + /* Fast trip threshold to be programmed */ > + val = div_u64(mul_u32_u32(0xFF, vthfst), data->irng); > + } else > + /* > + * Since there is no option to disable ocp, set limit to max > + * value > + */ > + val = 0xFF; > + > + reg = MAX5970_REG_DAC_FAST(rdev_id); > + ret = regmap_write(rdev->regmap, reg, val); > + > + return ret; > +} > +
> +static int max597x_irq_handler(int irq, struct regulator_irq_data *rid, > + unsigned long *dev_mask) > +{ > + struct regulator_err_state *stat; > + struct max597x_regulator *d = (struct max597x_regulator *)rid->data; > + int val, ret, i; > + > + ret = max597x_regmap_read_clear(d->regmap, MAX5970_REG_FAULT0, &val); > + if (ret) > + return REGULATOR_FAILED_RETRY;
This "read_clear" smells like a race-by-design to me...
> + > + *dev_mask = 0; > + for (i = 0; i < d->num_switches; i++) { > + stat = &rid->states[i]; > + stat->notifs = 0; > + stat->errors = 0; > + } > + > + for (i = 0; i < d->num_switches; i++) { > + stat = &rid->states[i]; > + > + if (val & UV_STATUS_CRIT(i)) { > + *dev_mask |= 1 << i; > + stat->notifs |= REGULATOR_EVENT_UNDER_VOLTAGE; > + stat->errors |= REGULATOR_ERROR_UNDER_VOLTAGE; > + } else if (val & UV_STATUS_WARN(i)) { > + *dev_mask |= 1 << i; > + stat->notifs |= REGULATOR_EVENT_UNDER_VOLTAGE_WARN; > + stat->errors |= REGULATOR_ERROR_UNDER_VOLTAGE_WARN; > + } > + } > + > + ret = max597x_regmap_read_clear(d->regmap, MAX5970_REG_FAULT1, &val); > + if (ret) > + return REGULATOR_FAILED_RETRY;
... and same here...
> + > + for (i = 0; i < d->num_switches; i++) { > + stat = &rid->states[i]; > + > + if (val & OV_STATUS_CRIT(i)) { > + *dev_mask |= 1 << i; > + stat->notifs |= REGULATOR_EVENT_REGULATION_OUT; > + stat->errors |= REGULATOR_ERROR_REGULATION_OUT; > + } else if (val & OV_STATUS_WARN(i)) { > + *dev_mask |= 1 << i; > + stat->notifs |= REGULATOR_EVENT_OVER_VOLTAGE_WARN; > + stat->errors |= REGULATOR_ERROR_OVER_VOLTAGE_WARN; > + } > + } > + > + ret = max597x_regmap_read_clear(d->regmap, MAX5970_REG_FAULT2, &val); > + if (ret) > + return REGULATOR_FAILED_RETRY; > +
... and here. I wonder if the reason for "clearing" would be worth commenting?
> + for (i = 0; i < d->num_switches; i++) { > + stat = &rid->states[i]; > + > + if (val & OC_STATUS_WARN(i)) { > + *dev_mask |= 1 << i; > + stat->notifs |= REGULATOR_EVENT_OVER_CURRENT_WARN; > + stat->errors |= REGULATOR_ERROR_OVER_CURRENT_WARN; > + } > + } > + > + ret = regmap_read(d->regmap, MAX5970_REG_STATUS0, &val); > + if (ret) > + return REGULATOR_FAILED_RETRY; > + > + for (i = 0; i < d->num_switches; i++) { > + stat = &rid->states[i]; > + > + if ((val & MAX5970_CB_IFAULTF(i)) > + || (val & MAX5970_CB_IFAULTS(i))) { > + *dev_mask |= 1 << i; > + stat->notifs |= > + REGULATOR_EVENT_OVER_CURRENT | > + REGULATOR_EVENT_DISABLE; > + stat->errors |= > + REGULATOR_ERROR_OVER_CURRENT | REGULATOR_ERROR_FAIL; > + > + /* Clear the sub-IRQ status */ > + regulator_disable_regmap(stat->rdev); > + } > + } > + return 0; > +} > +
-- Matti Vaittinen Linux kernel developer at ROHM Semiconductors Oulu Finland
~~ When things go utterly wrong vim users can always type :help! ~~
Discuss - Estimate - Plan - Report and finally accomplish this: void do_work(int time) __attribute__ ((const));
| |