lkml.org 
[lkml]   [2022]   [Jul]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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));

\
 
 \ /
  Last update: 2022-07-12 09:31    [W:0.082 / U:3.404 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site