Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20220705122244.472894-2-Naresh.Solanki@9elements.com>
 <20220705122244.472894-4-Naresh.Solanki@9elements.com> <c03fd83f-97b3-1837-b7fd-5f3f28c4b557@gmail.com>
In-Reply-To: <c03fd83f-97b3-1837-b7fd-5f3f28c4b557@gmail.com>
From:   Naresh Solanki <naresh.solanki@9elements.com>
Date:   Tue, 19 Jul 2022 21:29:15 +0530
Message-ID: <CABqG17gL=7TJHNowpM8OJD4Pmezp5iVmAKx4csmakrTpp5bUkA@mail.gmail.com>
Subject: Re: [PATCH 3/5] regulator: max597x: Add support for max597x regulator
To:     Matti Vaittinen <mazziesaccount@gmail.com>
Cc:     linux-kernel@vger.kernel.org, Liam Girdwood <lgirdwood@gmail.com>,
        Mark Brown <broonie@kernel.org>,
        Patrick Rudolph <patrick.rudolph@9elements.com>,
        Marcello Sylvester Bauer <sylv@sylv.io>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

> 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 :)
>
Userspace applications would receive sysfs notify for these events in realtime
(there is another patch that enables sysfs notify on regulator events)
This will enable taking necessary action if the regulator is not in good state.

> > +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?
>
Yes in the current scenario, if non-zero ocp is provided then
protection should always be enabled.
But since there is an enable switch in function call, I felt like it
should be addressed in this way.

> > +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...
>
I'm not sure what best way would be to address this but if Under/Over
Voltage & Over current scenario
occurs then the interrupt line remains low until things are normal.
The fault register bit can be clear only if output is normal or power cycled.


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