Re: [PATCH 3/5] regulator: max597x: Add support for max597x regulator

[Matti Vaittinen <mazziesaccount@gmail.com>]

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