Message-ID: <CAPDyKFo0d2FD1U-MgWTExz_cKC8BjB6QQFu1StTyPN1TGBG9eA@mail.gmail.com>
Date:   Mon, 19 Jun 2017 11:48:05 +0200
From:   Ulf Hansson <ulf.hansson@...aro.org>
To:     Peter Chen <hzpeterchen@...il.com>
Cc:     Peter Chen <peter.chen@....com>,
        Mark Rutland <mark.rutland@....com>,
        Heiko Stuebner <heiko@...ech.de>,
        Stephen Boyd <stephen.boyd@...aro.org>, frank.li@....com,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Gary Bisson <gary.bisson@...ndarydevices.com>,
        Fabio Estevam <festevam@...il.com>,
        Joshua Clayton <stillcompiling@...il.com>,
        Arnd Bergmann <arnd@...db.de>,
        Dmitry Eremin-Solenikov <dbaryshkov@...il.com>,
        Vaibhav Hiremath <vaibhav.hiremath@...aro.org>,
        Krzysztof Kozlowski <krzk@...nel.org>, mka@...omium.org,
        Alan Stern <stern@...land.harvard.edu>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "Maciej S. Szmigiero" <mail@...iej.szmigiero.name>,
        Pawel Moll <pawel.moll@....com>,
        "linux-pm@...r.kernel.org" <linux-pm@...r.kernel.org>,
        Sascha Hauer <s.hauer@...gutronix.de>,
        troy.kisky@...ndarydevices.com, Rob Herring <robh+dt@...nel.org>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>, hverkuil@...all.nl,
        oscar@...andei.net,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Linux USB List <linux-usb@...r.kernel.org>,
        "Rafael J. Wysocki" <rjw@...ysocki.net>,
        Sebastian Reichel <sre@...nel.org>,
        Mark Brown <broonie@...nel.org>,
        Philipp Zabel <p.zabel@...gutronix.de>,
        Shawn Guo <shawnguo@...nel.org>, jun.li@....com
Subject: Re: [PATCH v15 2/7] power: add power sequence library

[...]

>> >
>> > Unlike the MMC design, there is no dts entry to indicate whether this
>> > device needs pwrseq or not at this design, it will only carry out power
>> > on sequence after matching. So, return -EPROBE_DEFER may not work since
>> > this device may never need pwrseq.
>>
>> Then, how will you really be able to fetch the correct pwrseq library
>> instance for the device node?
>>
>> Suppose their is a *list* of pwrseq library instances available. In
>> pwrseq_find_available_instance() you call of_match_node(table, np).
>> The "table" there corresponds to the compatible for the pwrseq library
>> and the np is the device node provided by the caller of
>> of_pwrseq_on().
>>
>> Why is this match done?
>
> The compatible in table is from the source code, and the compatible in
> np is from the dts. This is the current match way, I comment your
> suggestion below.
>
>>
>> Why can't the match be done before trying to fetch a library instance
>
> How? If there is no pwrseq instance, how can we do match?
>
>> and then in a second step, really try to fetch the instance? If only
>> the second step fails, returning -EPROBE_DEFER can be done, no?
>>
>> BTW, I didn't compatible for the generic pwrseq library being
>> documented in this series.

Seems like you need to update the DT documentation for the below
compatible, which is used for the generic pwrseq library. Perhaps this
is what puzzles me a bit on *why* the match is done.

+static const struct of_device_id generic_id_table[] = {
+       { .compatible = "generic",},
+       { /* sentinel */ }
+};

[...]

>> >
>> > This additional instance is used to store compatible information for
>> > this pwrseq library, it is used for the next matching between device
>> > and pwrseq library, it just likes we need the first pwrseq instance
>> > registered at boot stage.
>>
>> Why can't the compatible information be a static table, known by the
>> pwrseq core library?
>>
>> Then when of_pwrseq_on() is called, that static table is parsed and
>> matched, then a corresponding pwrseq library instance tries to be
>> fetched.
>>
>
> So, you suggest allocating and registering pwrseq instance on the
> demand? Eg, we maintain a power sequence static table, including
> compatible and allocate function.

Yes, something like that.

>
> static const struct pwrseq_match_table pwrseq_match_table_list[] = {
>         { PWRSEQ_DEV(0x0204, 0x6025), .alloc_instance = pwrseq_AA_alloc_instance },
>         { PWRSEQ_DEV(0x0204, 0x6026), .alloc_instance = pwrseq_BB_alloc_instance },
>         { PWRSEQ_DEV(0xffff, 0xffff), .alloc_instance = pwrseq_generic_alloc_instance },

What does the  PWRSEQ_DEV() macro do?

> };
>
> And pwrseq_AA{BB}_alloc_instance are defined at each pwrseq library, and
> are exported.

With "exported", I guess you mean shared via a common pwrseq header?

>
> Since the pwrseq_match_table_list is static, we can always do match, and
> will not return -EPROBE_DEFER anymore, one problem for this is we need
> always compile all pwrseq libraries. Any good suggestions?

You never returned -EPROBE_DEFER in the first case. That's why I complained. :-)

So, in case the OF match doesn't succeed, there are no reason to
propagate an error, but instead just bail out and returning 0 to the
caller.

If the OF match succeeds, it means the device requires a pwrseq
library to be used. Then, pwrseq_XX_alloc_instance() will be called,
on demand and which tries to fetch the resources (clocks, gpios etc).
If any of those attempts fetching a resource fails, its corresponding
error code should be propagated to the caller - including
-EPROBE_DEFER.

Regarding the "always compile all pwrseq libraries"; no we don't need
to do that. Instead we only need a to have a stub function for
pwrseq_XX_alloc_instance, in case its corresponding Kconfig option is
unset. That stub, should of course return an error code.

Kind regards
Uffe

Powered by blists - more mailing lists