Re: [PATCH v4 4/5] regulator: qcom: Add labibb driver

[Sumit Semwal]

Hello Mark,

On Tue, 2 Jun 2020 at 17:55, Mark Brown <broonie@xxxxxxxxxx> wrote:
>
> On Tue, Jun 02, 2020 at 05:40:45PM +0530, Sumit Semwal wrote:
> > On Tue, 2 Jun 2020 at 17:02, Mark Brown <broonie@xxxxxxxxxx> wrote:
> > > On Tue, Jun 02, 2020 at 03:39:23PM +0530, Sumit Semwal wrote:
>
> > > This should be a get_status() callback...
>
> > From my (limited) understanding of downstream code, it seemed like for
> > this set of regulators, the 'enabled' check is done via the
> > 'REG_LABIBB_STATUS1 reg; for some reason, not via the same enable_reg
> > / enable_mask ones.  That's why I used it as is_enabled() callback.
> > I will try and check with the QC folks to clarify this point about
> > their hardware.
>
> The way this is functioning at the minute the downstream code is just
> buggy.

Apologies for the delay in responding - I pinged the QC folks, and was
waiting for their reply but haven't got any response so far.

I tried your suggestion to use the ENABLE_CTL register for checking if
the regulator is actually enabled. In my limited testing on the Poco,
it seems like the STATUS1 register updates faster than the ENABLE_CTL
register, so on the device, I see noticeable lag when I use ENABLE_CTL
for is_enabled() check. [This is especially true for the IBB, which
takes longer to become usable than the LAB regulator.]

I understand from a pure regulators' correctness point of view,
ENABLE_CTL should be the one checked there, so I can change the patch
as you suggested, but there seems to be some performance penalty
there.

>
> > > ...is_enabled() should just be regulator_is_enabled_regmap() and these
> > > functions should just be removed entirely, you can use the regmap
> > > operations directly as the ops without the wrapper.
>
> > The 2 wrappers are a precursor to the next patch, where we keep track
> > of regulator's enable status to check during SC handling.
>
> Add the functions when they're useful, not before.  TBH if the register
> is write only you're probably better off adding a register cache.

Agreed, I will remove the wrappers from here, using the regmap
functions, and add the wrappers with the SC handling patch.
>
> > > > +     match = of_match_device(qcom_labibb_match, &pdev->dev);
> > > > +     if (!match)
> > > > +             return -ENODEV;
> > > > +
> > > > +     for (reg_data = match->data; reg_data->name; reg_data++) {
> > > > +             child = of_get_child_by_name(pdev->dev.of_node, reg_data->name);
> > > > +
> > > > +             if (WARN_ON(child == NULL))
> > > > +                     return -EINVAL;
> > >
> > > This feels like the DT bindings are confused - why do we need to search
> > > like this?
>
> > The WARN_ON? This was suggested by Bjorn to catch the case where the
> > DT binding for a PMIC instantiates only one of the regulators.
>
> No, this whole loop - why this whole match and get child stuff?
This loop mechanism is what I saw in the other qcom regulators
upstream, so thought it was an acceptable way.
For the two children nodes, do you recommend another mechanism to get
and validate both nodes?

Best,
Sumit.