Re: [PATCH 3/3] ARM: dts: qcom: basic HP TouchPad support

[Linus Walleij]

On Wed, Jan 26, 2022 at 5:13 AM Bjorn Andersson
<bjorn.andersson@xxxxxxxxxx> wrote:

> @Linus, please take a look at the regulator question below.

OK!

> >  arch/arm/boot/dts/qcom-apq8060-tenderloin.dts | 549 ++----------------

Nice to see some use of all the work put into the APQ8060 support!

FYI: if you want graphics, this work may need to get finished:
https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-integrator.git/log/?h=apq8060-dragonboard-graphics
(mainly the MMCC)

> >               pinctrl@800000 {
> > -                     /* eMMMC pins, all 8 data lines connected */
>
> It would be nice if you could throw a separate patch on the list that
> fixes this spelling mistake in the original as well.

Yes pls.

> >                               l21 {
> > -                                     // 1.1 V according to schematic
> >                                       regulator-min-microvolt = <1200000>;
> >                                       regulator-max-microvolt = <1200000>;
> >                                       bias-pull-down;
> > -                                     regulator-always-on;
> > +                                     /*
> > +                                      * RPM driver can't handle always-on regulators that are
> > +                                      * supplied by regulators initialized after them.
> > +                                      */
>
> That looks like an oversight that should be corrected, perhaps it needs
> similar attention that was given to the smd-rpm driver recently?

Indeed

> But this makes me wonder, how can this work on the other board? Linus?

I suppose these don't supply anything vital?

I know I have seen some regulators switch off and on but I
may have been confused.


> >                               s0 {
> > -                                     // regulator-min-microvolt = <500000>;
> > +                                     // regulator-min-microvolt = <800000>;
> >                                       // regulator-max-microvolt = <1325000>;
>
> This looks like the full range the regulator could do, do you see a
> reason for documenting that here? Unless there's a good reason I think
> you should leave the commented min/max out.

Yours,
Linus Walleij