Re: [PATCH v15 2/7] power: add power sequence library

[Peter Chen]

On Mon, Jun 19, 2017 at 11:48:05AM +0200, Ulf Hansson wrote:
> [...]
> 
> >> >
> >> > 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 */ }
> +};

Sorry, I should update this "generic" compatible at 1st binding-doc
patch.

> 
> [...]
> 
> >> >
> >> > 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(0x, 0x), .alloc_instance = 
> > pwrseq_generic_alloc_instance },
> 
> What does the  PWRSEQ_DEV() macro do?

In fact, this should be compatible string, I exampled it as USB vid,pid
wrongly.

> > 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.
> 

I will have a updated version for your suggestion, thanks.

-- 

Best Regards,
Peter Chen

Re: [PATCH v15 2/7] power: add power sequence library

[Peter Chen]

On Mon, Jun 19, 2017 at 11:48:05AM +0200, Ulf Hansson wrote:
> [...]
> 
> >> >
> >> > 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 */ }
> +};

Sorry, I should update this "generic" compatible at 1st binding-doc
patch.

> 
> [...]
> 
> >> >
> >> > 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(0x, 0x), .alloc_instance = 
> > pwrseq_generic_alloc_instance },
> 
> What does the  PWRSEQ_DEV() macro do?

In fact, this should be compatible string, I exampled it as USB vid,pid
wrongly.

> > 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.
> 

I will have a updated version for your suggestion, thanks.

-- 

Best Regards,
Peter Chen

Re: [PATCH v15 2/7] power: add power sequence library

[Ulf Hansson]

[...]

>> >
>> > 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(0x, 0x), .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

Re: [PATCH v15 2/7] power: add power sequence library

[Ulf Hansson]

[...]

>> >
>> > 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(0x, 0x), .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

Re: [PATCH v15 2/7] power: add power sequence library

[Peter Chen]

On Mon, Jun 19, 2017 at 10:09:58AM +0200, Ulf Hansson wrote:
> On 15 June 2017 at 12:06, Peter Chen  wrote:
> > On Thu, Jun 15, 2017 at 11:35:20AM +0200, Ulf Hansson wrote:
> >> On 15 June 2017 at 11:11, Peter Chen  wrote:
> >> > On Thu, Jun 15, 2017 at 10:11:45AM +0200, Ulf Hansson wrote:
> >> >> > Yes, you are right. This is the limitation for this power sequence
> >> >> > library, the registration for the 1st power sequence instance must
> >> >> > be finished before device driver uses it. I am appreciated that
> >> >> > you can supply some suggestions for it.
> >> >>
> >> >> In general this kind of problems is solved by first parsing the DTB,
> >> >> which means you will find out whether there is a resource (a pwrseq)
> >> >> required for the device. Then you try to fetch that resource, and if
> >> >> that fails, it means the resource is not yet available, and hence you
> >> >> want to retry later and should return -EPROBE_DEFER.
> >> >>
> >> >> In this case, of_pwrseq_on() needs to be converted to start looking
> >> >> for a pwrseq compatible in it's child node - I guess. Then if that is
> >> >> found, you try to fetch the instance of the corresponding library.
> >> >> Failing to fetch the library instance should then cause a return
> >> >> -EPROBE_DEFER.
> >> >
> >> > The most difficulty for this is we can't know whether the requested
> >> > pwrseq instance will be registered or not, the kernel configuration
> >> > for this pwrseq library may not be chosen at all.
> >>
> >> In such case it is still correct to return -EPROBE_DEFER, because the
> >> driver that tries to probe its device will fail unless it can run the
> >> needed pwrseq. Right?
> >>
> >
> > 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.
> 
> >
> >> >
> >> >>
> >> >> >
> >> >> >> Moreover, I have found yet another severe problem but reviewing the 
> >> >> >> code:
> >> >> >> In the struct pwrseq, you have a "bool used", which you are setting 
> >> >> >> to
> >> >> >> "true" once the pwrseq has been hooked up with the device, when a
> >> >> >> driver calls of_pwrseq_on(). Setting that variable to true, will also
> >> >> >> prevent another driver from using the same instance of the pwrseq for
> >> >> >> its device. So, to cope with multiple users, you register a new
> >> >> >> instance of the same pwrseq library that got hooked up, once the
> >> >> >> ->get() callback is about to complete.
> >> >> >>
> >> >> >> The problem the occurs, when there is another driver calling
> >> >> >> of_pwrseq_on() in between, meaning that the new instance has not yet
> >> >> >> been registered. This will simply fail, won't it?
> >> >> >
> >> >> > Yes, you are right, thanks for pointing that, I will add mutex_lock 
> >> >> > for
> >> >> > of_pwrseq_on.
> >> >>
> >> >> Another option is to entirely skip to two step approach.
> >> >>
> >> >> In other words, make the library to cope with multiple users via the
> >> >> same registered library instance.
> >> >>
> >> >
> >> > No, the pwrseq instance stores dtb information (clock, gpio, etc), it
> >> > needs to be per device.
> >>
> >> I think you misunderstood my suggestion here. Of course you need to
> >> allocate one pwrseq data per device.
> >>
> >> However, my point is that you shouldn't need more than one instance of
> >> the library functions to be registered in the list of available pwrseq
> >> libraries.
> >>
> >
> > 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 

Re: [PATCH v15 2/7] power: add power sequence library

[Peter Chen]

On Mon, Jun 19, 2017 at 10:09:58AM +0200, Ulf Hansson wrote:
> On 15 June 2017 at 12:06, Peter Chen  wrote:
> > On Thu, Jun 15, 2017 at 11:35:20AM +0200, Ulf Hansson wrote:
> >> On 15 June 2017 at 11:11, Peter Chen  wrote:
> >> > On Thu, Jun 15, 2017 at 10:11:45AM +0200, Ulf Hansson wrote:
> >> >> > Yes, you are right. This is the limitation for this power sequence
> >> >> > library, the registration for the 1st power sequence instance must
> >> >> > be finished before device driver uses it. I am appreciated that
> >> >> > you can supply some suggestions for it.
> >> >>
> >> >> In general this kind of problems is solved by first parsing the DTB,
> >> >> which means you will find out whether there is a resource (a pwrseq)
> >> >> required for the device. Then you try to fetch that resource, and if
> >> >> that fails, it means the resource is not yet available, and hence you
> >> >> want to retry later and should return -EPROBE_DEFER.
> >> >>
> >> >> In this case, of_pwrseq_on() needs to be converted to start looking
> >> >> for a pwrseq compatible in it's child node - I guess. Then if that is
> >> >> found, you try to fetch the instance of the corresponding library.
> >> >> Failing to fetch the library instance should then cause a return
> >> >> -EPROBE_DEFER.
> >> >
> >> > The most difficulty for this is we can't know whether the requested
> >> > pwrseq instance will be registered or not, the kernel configuration
> >> > for this pwrseq library may not be chosen at all.
> >>
> >> In such case it is still correct to return -EPROBE_DEFER, because the
> >> driver that tries to probe its device will fail unless it can run the
> >> needed pwrseq. Right?
> >>
> >
> > 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.
> 
> >
> >> >
> >> >>
> >> >> >
> >> >> >> Moreover, I have found yet another severe problem but reviewing the 
> >> >> >> code:
> >> >> >> In the struct pwrseq, you have a "bool used", which you are setting 
> >> >> >> to
> >> >> >> "true" once the pwrseq has been hooked up with the device, when a
> >> >> >> driver calls of_pwrseq_on(). Setting that variable to true, will also
> >> >> >> prevent another driver from using the same instance of the pwrseq for
> >> >> >> its device. So, to cope with multiple users, you register a new
> >> >> >> instance of the same pwrseq library that got hooked up, once the
> >> >> >> ->get() callback is about to complete.
> >> >> >>
> >> >> >> The problem the occurs, when there is another driver calling
> >> >> >> of_pwrseq_on() in between, meaning that the new instance has not yet
> >> >> >> been registered. This will simply fail, won't it?
> >> >> >
> >> >> > Yes, you are right, thanks for pointing that, I will add mutex_lock 
> >> >> > for
> >> >> > of_pwrseq_on.
> >> >>
> >> >> Another option is to entirely skip to two step approach.
> >> >>
> >> >> In other words, make the library to cope with multiple users via the
> >> >> same registered library instance.
> >> >>
> >> >
> >> > No, the pwrseq instance stores dtb information (clock, gpio, etc), it
> >> > needs to be per device.
> >>
> >> I think you misunderstood my suggestion here. Of course you need to
> >> allocate one pwrseq data per device.
> >>
> >> However, my point is that you shouldn't need more than one instance of
> >> the library functions to be registered in the list of available pwrseq
> >> libraries.
> >>
> >
> > 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 

Re: [PATCH v15 2/7] power: add power sequence library

[Ulf Hansson]

On 15 June 2017 at 12:06, Peter Chen  wrote:
> On Thu, Jun 15, 2017 at 11:35:20AM +0200, Ulf Hansson wrote:
>> On 15 June 2017 at 11:11, Peter Chen  wrote:
>> > On Thu, Jun 15, 2017 at 10:11:45AM +0200, Ulf Hansson wrote:
>> >> > Yes, you are right. This is the limitation for this power sequence
>> >> > library, the registration for the 1st power sequence instance must
>> >> > be finished before device driver uses it. I am appreciated that
>> >> > you can supply some suggestions for it.
>> >>
>> >> In general this kind of problems is solved by first parsing the DTB,
>> >> which means you will find out whether there is a resource (a pwrseq)
>> >> required for the device. Then you try to fetch that resource, and if
>> >> that fails, it means the resource is not yet available, and hence you
>> >> want to retry later and should return -EPROBE_DEFER.
>> >>
>> >> In this case, of_pwrseq_on() needs to be converted to start looking
>> >> for a pwrseq compatible in it's child node - I guess. Then if that is
>> >> found, you try to fetch the instance of the corresponding library.
>> >> Failing to fetch the library instance should then cause a return
>> >> -EPROBE_DEFER.
>> >
>> > The most difficulty for this is we can't know whether the requested
>> > pwrseq instance will be registered or not, the kernel configuration
>> > for this pwrseq library may not be chosen at all.
>>
>> In such case it is still correct to return -EPROBE_DEFER, because the
>> driver that tries to probe its device will fail unless it can run the
>> needed pwrseq. Right?
>>
>
> 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?

Why can't the match be done before trying to fetch a library instance
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.

>
>> >
>> >>
>> >> >
>> >> >> Moreover, I have found yet another severe problem but reviewing the 
>> >> >> code:
>> >> >> In the struct pwrseq, you have a "bool used", which you are setting to
>> >> >> "true" once the pwrseq has been hooked up with the device, when a
>> >> >> driver calls of_pwrseq_on(). Setting that variable to true, will also
>> >> >> prevent another driver from using the same instance of the pwrseq for
>> >> >> its device. So, to cope with multiple users, you register a new
>> >> >> instance of the same pwrseq library that got hooked up, once the
>> >> >> ->get() callback is about to complete.
>> >> >>
>> >> >> The problem the occurs, when there is another driver calling
>> >> >> of_pwrseq_on() in between, meaning that the new instance has not yet
>> >> >> been registered. This will simply fail, won't it?
>> >> >
>> >> > Yes, you are right, thanks for pointing that, I will add mutex_lock for
>> >> > of_pwrseq_on.
>> >>
>> >> Another option is to entirely skip to two step approach.
>> >>
>> >> In other words, make the library to cope with multiple users via the
>> >> same registered library instance.
>> >>
>> >
>> > No, the pwrseq instance stores dtb information (clock, gpio, etc), it
>> > needs to be per device.
>>
>> I think you misunderstood my suggestion here. Of course you need to
>> allocate one pwrseq data per device.
>>
>> However, my point is that you shouldn't need more than one instance of
>> the library functions to be registered in the list of available pwrseq
>> libraries.
>>
>
> 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.

Kind regards
Uffe

Re: [PATCH v15 2/7] power: add power sequence library

[Ulf Hansson]

On 15 June 2017 at 12:06, Peter Chen  wrote:
> On Thu, Jun 15, 2017 at 11:35:20AM +0200, Ulf Hansson wrote:
>> On 15 June 2017 at 11:11, Peter Chen  wrote:
>> > On Thu, Jun 15, 2017 at 10:11:45AM +0200, Ulf Hansson wrote:
>> >> > Yes, you are right. This is the limitation for this power sequence
>> >> > library, the registration for the 1st power sequence instance must
>> >> > be finished before device driver uses it. I am appreciated that
>> >> > you can supply some suggestions for it.
>> >>
>> >> In general this kind of problems is solved by first parsing the DTB,
>> >> which means you will find out whether there is a resource (a pwrseq)
>> >> required for the device. Then you try to fetch that resource, and if
>> >> that fails, it means the resource is not yet available, and hence you
>> >> want to retry later and should return -EPROBE_DEFER.
>> >>
>> >> In this case, of_pwrseq_on() needs to be converted to start looking
>> >> for a pwrseq compatible in it's child node - I guess. Then if that is
>> >> found, you try to fetch the instance of the corresponding library.
>> >> Failing to fetch the library instance should then cause a return
>> >> -EPROBE_DEFER.
>> >
>> > The most difficulty for this is we can't know whether the requested
>> > pwrseq instance will be registered or not, the kernel configuration
>> > for this pwrseq library may not be chosen at all.
>>
>> In such case it is still correct to return -EPROBE_DEFER, because the
>> driver that tries to probe its device will fail unless it can run the
>> needed pwrseq. Right?
>>
>
> 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?

Why can't the match be done before trying to fetch a library instance
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.

>
>> >
>> >>
>> >> >
>> >> >> Moreover, I have found yet another severe problem but reviewing the 
>> >> >> code:
>> >> >> In the struct pwrseq, you have a "bool used", which you are setting to
>> >> >> "true" once the pwrseq has been hooked up with the device, when a
>> >> >> driver calls of_pwrseq_on(). Setting that variable to true, will also
>> >> >> prevent another driver from using the same instance of the pwrseq for
>> >> >> its device. So, to cope with multiple users, you register a new
>> >> >> instance of the same pwrseq library that got hooked up, once the
>> >> >> ->get() callback is about to complete.
>> >> >>
>> >> >> The problem the occurs, when there is another driver calling
>> >> >> of_pwrseq_on() in between, meaning that the new instance has not yet
>> >> >> been registered. This will simply fail, won't it?
>> >> >
>> >> > Yes, you are right, thanks for pointing that, I will add mutex_lock for
>> >> > of_pwrseq_on.
>> >>
>> >> Another option is to entirely skip to two step approach.
>> >>
>> >> In other words, make the library to cope with multiple users via the
>> >> same registered library instance.
>> >>
>> >
>> > No, the pwrseq instance stores dtb information (clock, gpio, etc), it
>> > needs to be per device.
>>
>> I think you misunderstood my suggestion here. Of course you need to
>> allocate one pwrseq data per device.
>>
>> However, my point is that you shouldn't need more than one instance of
>> the library functions to be registered in the list of available pwrseq
>> libraries.
>>
>
> 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.

Kind regards
Uffe

Re: [PATCH v15 2/7] power: add power sequence library

[Peter Chen]

On Thu, Jun 15, 2017 at 06:06:04PM +0800, Peter Chen wrote:
> On Thu, Jun 15, 2017 at 11:35:20AM +0200, Ulf Hansson wrote:
> > On 15 June 2017 at 11:11, Peter Chen  wrote:
> > > On Thu, Jun 15, 2017 at 10:11:45AM +0200, Ulf Hansson wrote:
> > >> > Yes, you are right. This is the limitation for this power sequence
> > >> > library, the registration for the 1st power sequence instance must
> > >> > be finished before device driver uses it. I am appreciated that
> > >> > you can supply some suggestions for it.
> > >>
> > >> In general this kind of problems is solved by first parsing the DTB,
> > >> which means you will find out whether there is a resource (a pwrseq)
> > >> required for the device. Then you try to fetch that resource, and if
> > >> that fails, it means the resource is not yet available, and hence you
> > >> want to retry later and should return -EPROBE_DEFER.
> > >>
> > >> In this case, of_pwrseq_on() needs to be converted to start looking
> > >> for a pwrseq compatible in it's child node - I guess. Then if that is
> > >> found, you try to fetch the instance of the corresponding library.
> > >> Failing to fetch the library instance should then cause a return
> > >> -EPROBE_DEFER.
> > >
> > > The most difficulty for this is we can't know whether the requested
> > > pwrseq instance will be registered or not, the kernel configuration
> > > for this pwrseq library may not be chosen at all.
> > 
> > In such case it is still correct to return -EPROBE_DEFER, because the
> > driver that tries to probe its device will fail unless it can run the
> > needed pwrseq. Right?
> > 
> 
> 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.
> 

Ulf, since it is the use case limitation, it can't work like device
driver. Do you have more comments for it, thanks.

Peter


> > >
> > >>
> > >> >
> > >> >> Moreover, I have found yet another severe problem but reviewing the 
> > >> >> code:
> > >> >> In the struct pwrseq, you have a "bool used", which you are setting to
> > >> >> "true" once the pwrseq has been hooked up with the device, when a
> > >> >> driver calls of_pwrseq_on(). Setting that variable to true, will also
> > >> >> prevent another driver from using the same instance of the pwrseq for
> > >> >> its device. So, to cope with multiple users, you register a new
> > >> >> instance of the same pwrseq library that got hooked up, once the
> > >> >> ->get() callback is about to complete.
> > >> >>
> > >> >> The problem the occurs, when there is another driver calling
> > >> >> of_pwrseq_on() in between, meaning that the new instance has not yet
> > >> >> been registered. This will simply fail, won't it?
> > >> >
> > >> > Yes, you are right, thanks for pointing that, I will add mutex_lock for
> > >> > of_pwrseq_on.
> > >>
> > >> Another option is to entirely skip to two step approach.
> > >>
> > >> In other words, make the library to cope with multiple users via the
> > >> same registered library instance.
> > >>
> > >
> > > No, the pwrseq instance stores dtb information (clock, gpio, etc), it
> > > needs to be per device.
> > 
> > I think you misunderstood my suggestion here. Of course you need to
> > allocate one pwrseq data per device.
> > 
> > However, my point is that you shouldn't need more than one instance of
> > the library functions to be registered in the list of available pwrseq
> > libraries.
> > 
> 
> 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.
> 
> -- 
> 
> Best Regards,
> Peter Chen
> 
> ___
> linux-arm-kernel mailing list
> linux-arm-ker...@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

-- 

Best Regards,
Peter Chen

Re: [PATCH v15 2/7] power: add power sequence library

[Peter Chen]

On Thu, Jun 15, 2017 at 06:06:04PM +0800, Peter Chen wrote:
> On Thu, Jun 15, 2017 at 11:35:20AM +0200, Ulf Hansson wrote:
> > On 15 June 2017 at 11:11, Peter Chen  wrote:
> > > On Thu, Jun 15, 2017 at 10:11:45AM +0200, Ulf Hansson wrote:
> > >> > Yes, you are right. This is the limitation for this power sequence
> > >> > library, the registration for the 1st power sequence instance must
> > >> > be finished before device driver uses it. I am appreciated that
> > >> > you can supply some suggestions for it.
> > >>
> > >> In general this kind of problems is solved by first parsing the DTB,
> > >> which means you will find out whether there is a resource (a pwrseq)
> > >> required for the device. Then you try to fetch that resource, and if
> > >> that fails, it means the resource is not yet available, and hence you
> > >> want to retry later and should return -EPROBE_DEFER.
> > >>
> > >> In this case, of_pwrseq_on() needs to be converted to start looking
> > >> for a pwrseq compatible in it's child node - I guess. Then if that is
> > >> found, you try to fetch the instance of the corresponding library.
> > >> Failing to fetch the library instance should then cause a return
> > >> -EPROBE_DEFER.
> > >
> > > The most difficulty for this is we can't know whether the requested
> > > pwrseq instance will be registered or not, the kernel configuration
> > > for this pwrseq library may not be chosen at all.
> > 
> > In such case it is still correct to return -EPROBE_DEFER, because the
> > driver that tries to probe its device will fail unless it can run the
> > needed pwrseq. Right?
> > 
> 
> 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.
> 

Ulf, since it is the use case limitation, it can't work like device
driver. Do you have more comments for it, thanks.

Peter


> > >
> > >>
> > >> >
> > >> >> Moreover, I have found yet another severe problem but reviewing the 
> > >> >> code:
> > >> >> In the struct pwrseq, you have a "bool used", which you are setting to
> > >> >> "true" once the pwrseq has been hooked up with the device, when a
> > >> >> driver calls of_pwrseq_on(). Setting that variable to true, will also
> > >> >> prevent another driver from using the same instance of the pwrseq for
> > >> >> its device. So, to cope with multiple users, you register a new
> > >> >> instance of the same pwrseq library that got hooked up, once the
> > >> >> ->get() callback is about to complete.
> > >> >>
> > >> >> The problem the occurs, when there is another driver calling
> > >> >> of_pwrseq_on() in between, meaning that the new instance has not yet
> > >> >> been registered. This will simply fail, won't it?
> > >> >
> > >> > Yes, you are right, thanks for pointing that, I will add mutex_lock for
> > >> > of_pwrseq_on.
> > >>
> > >> Another option is to entirely skip to two step approach.
> > >>
> > >> In other words, make the library to cope with multiple users via the
> > >> same registered library instance.
> > >>
> > >
> > > No, the pwrseq instance stores dtb information (clock, gpio, etc), it
> > > needs to be per device.
> > 
> > I think you misunderstood my suggestion here. Of course you need to
> > allocate one pwrseq data per device.
> > 
> > However, my point is that you shouldn't need more than one instance of
> > the library functions to be registered in the list of available pwrseq
> > libraries.
> > 
> 
> 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.
> 
> -- 
> 
> Best Regards,
> Peter Chen
> 
> ___
> linux-arm-kernel mailing list
> linux-arm-ker...@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

-- 

Best Regards,
Peter Chen

Re: [PATCH v15 2/7] power: add power sequence library

[Peter Chen]

On Thu, Jun 15, 2017 at 11:35:20AM +0200, Ulf Hansson wrote:
> On 15 June 2017 at 11:11, Peter Chen  wrote:
> > On Thu, Jun 15, 2017 at 10:11:45AM +0200, Ulf Hansson wrote:
> >> > Yes, you are right. This is the limitation for this power sequence
> >> > library, the registration for the 1st power sequence instance must
> >> > be finished before device driver uses it. I am appreciated that
> >> > you can supply some suggestions for it.
> >>
> >> In general this kind of problems is solved by first parsing the DTB,
> >> which means you will find out whether there is a resource (a pwrseq)
> >> required for the device. Then you try to fetch that resource, and if
> >> that fails, it means the resource is not yet available, and hence you
> >> want to retry later and should return -EPROBE_DEFER.
> >>
> >> In this case, of_pwrseq_on() needs to be converted to start looking
> >> for a pwrseq compatible in it's child node - I guess. Then if that is
> >> found, you try to fetch the instance of the corresponding library.
> >> Failing to fetch the library instance should then cause a return
> >> -EPROBE_DEFER.
> >
> > The most difficulty for this is we can't know whether the requested
> > pwrseq instance will be registered or not, the kernel configuration
> > for this pwrseq library may not be chosen at all.
> 
> In such case it is still correct to return -EPROBE_DEFER, because the
> driver that tries to probe its device will fail unless it can run the
> needed pwrseq. Right?
> 

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.

> >
> >>
> >> >
> >> >> Moreover, I have found yet another severe problem but reviewing the 
> >> >> code:
> >> >> In the struct pwrseq, you have a "bool used", which you are setting to
> >> >> "true" once the pwrseq has been hooked up with the device, when a
> >> >> driver calls of_pwrseq_on(). Setting that variable to true, will also
> >> >> prevent another driver from using the same instance of the pwrseq for
> >> >> its device. So, to cope with multiple users, you register a new
> >> >> instance of the same pwrseq library that got hooked up, once the
> >> >> ->get() callback is about to complete.
> >> >>
> >> >> The problem the occurs, when there is another driver calling
> >> >> of_pwrseq_on() in between, meaning that the new instance has not yet
> >> >> been registered. This will simply fail, won't it?
> >> >
> >> > Yes, you are right, thanks for pointing that, I will add mutex_lock for
> >> > of_pwrseq_on.
> >>
> >> Another option is to entirely skip to two step approach.
> >>
> >> In other words, make the library to cope with multiple users via the
> >> same registered library instance.
> >>
> >
> > No, the pwrseq instance stores dtb information (clock, gpio, etc), it
> > needs to be per device.
> 
> I think you misunderstood my suggestion here. Of course you need to
> allocate one pwrseq data per device.
> 
> However, my point is that you shouldn't need more than one instance of
> the library functions to be registered in the list of available pwrseq
> libraries.
> 

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.

-- 

Best Regards,
Peter Chen

Re: [PATCH v15 2/7] power: add power sequence library

[Peter Chen]

On Thu, Jun 15, 2017 at 11:35:20AM +0200, Ulf Hansson wrote:
> On 15 June 2017 at 11:11, Peter Chen  wrote:
> > On Thu, Jun 15, 2017 at 10:11:45AM +0200, Ulf Hansson wrote:
> >> > Yes, you are right. This is the limitation for this power sequence
> >> > library, the registration for the 1st power sequence instance must
> >> > be finished before device driver uses it. I am appreciated that
> >> > you can supply some suggestions for it.
> >>
> >> In general this kind of problems is solved by first parsing the DTB,
> >> which means you will find out whether there is a resource (a pwrseq)
> >> required for the device. Then you try to fetch that resource, and if
> >> that fails, it means the resource is not yet available, and hence you
> >> want to retry later and should return -EPROBE_DEFER.
> >>
> >> In this case, of_pwrseq_on() needs to be converted to start looking
> >> for a pwrseq compatible in it's child node - I guess. Then if that is
> >> found, you try to fetch the instance of the corresponding library.
> >> Failing to fetch the library instance should then cause a return
> >> -EPROBE_DEFER.
> >
> > The most difficulty for this is we can't know whether the requested
> > pwrseq instance will be registered or not, the kernel configuration
> > for this pwrseq library may not be chosen at all.
> 
> In such case it is still correct to return -EPROBE_DEFER, because the
> driver that tries to probe its device will fail unless it can run the
> needed pwrseq. Right?
> 

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.

> >
> >>
> >> >
> >> >> Moreover, I have found yet another severe problem but reviewing the 
> >> >> code:
> >> >> In the struct pwrseq, you have a "bool used", which you are setting to
> >> >> "true" once the pwrseq has been hooked up with the device, when a
> >> >> driver calls of_pwrseq_on(). Setting that variable to true, will also
> >> >> prevent another driver from using the same instance of the pwrseq for
> >> >> its device. So, to cope with multiple users, you register a new
> >> >> instance of the same pwrseq library that got hooked up, once the
> >> >> ->get() callback is about to complete.
> >> >>
> >> >> The problem the occurs, when there is another driver calling
> >> >> of_pwrseq_on() in between, meaning that the new instance has not yet
> >> >> been registered. This will simply fail, won't it?
> >> >
> >> > Yes, you are right, thanks for pointing that, I will add mutex_lock for
> >> > of_pwrseq_on.
> >>
> >> Another option is to entirely skip to two step approach.
> >>
> >> In other words, make the library to cope with multiple users via the
> >> same registered library instance.
> >>
> >
> > No, the pwrseq instance stores dtb information (clock, gpio, etc), it
> > needs to be per device.
> 
> I think you misunderstood my suggestion here. Of course you need to
> allocate one pwrseq data per device.
> 
> However, my point is that you shouldn't need more than one instance of
> the library functions to be registered in the list of available pwrseq
> libraries.
> 

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.

-- 

Best Regards,
Peter Chen

Re: [PATCH v15 2/7] power: add power sequence library

[Ulf Hansson]

On 15 June 2017 at 11:11, Peter Chen  wrote:
> On Thu, Jun 15, 2017 at 10:11:45AM +0200, Ulf Hansson wrote:
>> > Yes, you are right. This is the limitation for this power sequence
>> > library, the registration for the 1st power sequence instance must
>> > be finished before device driver uses it. I am appreciated that
>> > you can supply some suggestions for it.
>>
>> In general this kind of problems is solved by first parsing the DTB,
>> which means you will find out whether there is a resource (a pwrseq)
>> required for the device. Then you try to fetch that resource, and if
>> that fails, it means the resource is not yet available, and hence you
>> want to retry later and should return -EPROBE_DEFER.
>>
>> In this case, of_pwrseq_on() needs to be converted to start looking
>> for a pwrseq compatible in it's child node - I guess. Then if that is
>> found, you try to fetch the instance of the corresponding library.
>> Failing to fetch the library instance should then cause a return
>> -EPROBE_DEFER.
>
> The most difficulty for this is we can't know whether the requested
> pwrseq instance will be registered or not, the kernel configuration
> for this pwrseq library may not be chosen at all.

In such case it is still correct to return -EPROBE_DEFER, because the
driver that tries to probe its device will fail unless it can run the
needed pwrseq. Right?

>
>>
>> >
>> >> Moreover, I have found yet another severe problem but reviewing the code:
>> >> In the struct pwrseq, you have a "bool used", which you are setting to
>> >> "true" once the pwrseq has been hooked up with the device, when a
>> >> driver calls of_pwrseq_on(). Setting that variable to true, will also
>> >> prevent another driver from using the same instance of the pwrseq for
>> >> its device. So, to cope with multiple users, you register a new
>> >> instance of the same pwrseq library that got hooked up, once the
>> >> ->get() callback is about to complete.
>> >>
>> >> The problem the occurs, when there is another driver calling
>> >> of_pwrseq_on() in between, meaning that the new instance has not yet
>> >> been registered. This will simply fail, won't it?
>> >
>> > Yes, you are right, thanks for pointing that, I will add mutex_lock for
>> > of_pwrseq_on.
>>
>> Another option is to entirely skip to two step approach.
>>
>> In other words, make the library to cope with multiple users via the
>> same registered library instance.
>>
>
> No, the pwrseq instance stores dtb information (clock, gpio, etc), it
> needs to be per device.

I think you misunderstood my suggestion here. Of course you need to
allocate one pwrseq data per device.

However, my point is that you shouldn't need more than one instance of
the library functions to be registered in the list of available pwrseq
libraries.

Kind regards
Uffe

Re: [PATCH v15 2/7] power: add power sequence library

[Ulf Hansson]

On 15 June 2017 at 11:11, Peter Chen  wrote:
> On Thu, Jun 15, 2017 at 10:11:45AM +0200, Ulf Hansson wrote:
>> > Yes, you are right. This is the limitation for this power sequence
>> > library, the registration for the 1st power sequence instance must
>> > be finished before device driver uses it. I am appreciated that
>> > you can supply some suggestions for it.
>>
>> In general this kind of problems is solved by first parsing the DTB,
>> which means you will find out whether there is a resource (a pwrseq)
>> required for the device. Then you try to fetch that resource, and if
>> that fails, it means the resource is not yet available, and hence you
>> want to retry later and should return -EPROBE_DEFER.
>>
>> In this case, of_pwrseq_on() needs to be converted to start looking
>> for a pwrseq compatible in it's child node - I guess. Then if that is
>> found, you try to fetch the instance of the corresponding library.
>> Failing to fetch the library instance should then cause a return
>> -EPROBE_DEFER.
>
> The most difficulty for this is we can't know whether the requested
> pwrseq instance will be registered or not, the kernel configuration
> for this pwrseq library may not be chosen at all.

In such case it is still correct to return -EPROBE_DEFER, because the
driver that tries to probe its device will fail unless it can run the
needed pwrseq. Right?

>
>>
>> >
>> >> Moreover, I have found yet another severe problem but reviewing the code:
>> >> In the struct pwrseq, you have a "bool used", which you are setting to
>> >> "true" once the pwrseq has been hooked up with the device, when a
>> >> driver calls of_pwrseq_on(). Setting that variable to true, will also
>> >> prevent another driver from using the same instance of the pwrseq for
>> >> its device. So, to cope with multiple users, you register a new
>> >> instance of the same pwrseq library that got hooked up, once the
>> >> ->get() callback is about to complete.
>> >>
>> >> The problem the occurs, when there is another driver calling
>> >> of_pwrseq_on() in between, meaning that the new instance has not yet
>> >> been registered. This will simply fail, won't it?
>> >
>> > Yes, you are right, thanks for pointing that, I will add mutex_lock for
>> > of_pwrseq_on.
>>
>> Another option is to entirely skip to two step approach.
>>
>> In other words, make the library to cope with multiple users via the
>> same registered library instance.
>>
>
> No, the pwrseq instance stores dtb information (clock, gpio, etc), it
> needs to be per device.

I think you misunderstood my suggestion here. Of course you need to
allocate one pwrseq data per device.

However, my point is that you shouldn't need more than one instance of
the library functions to be registered in the list of available pwrseq
libraries.

Kind regards
Uffe

Re: [PATCH v15 2/7] power: add power sequence library

[Peter Chen]

On Thu, Jun 15, 2017 at 10:11:45AM +0200, Ulf Hansson wrote:
> > Yes, you are right. This is the limitation for this power sequence
> > library, the registration for the 1st power sequence instance must
> > be finished before device driver uses it. I am appreciated that
> > you can supply some suggestions for it.
> 
> In general this kind of problems is solved by first parsing the DTB,
> which means you will find out whether there is a resource (a pwrseq)
> required for the device. Then you try to fetch that resource, and if
> that fails, it means the resource is not yet available, and hence you
> want to retry later and should return -EPROBE_DEFER.
> 
> In this case, of_pwrseq_on() needs to be converted to start looking
> for a pwrseq compatible in it's child node - I guess. Then if that is
> found, you try to fetch the instance of the corresponding library.
> Failing to fetch the library instance should then cause a return
> -EPROBE_DEFER.

The most difficulty for this is we can't know whether the requested
pwrseq instance will be registered or not, the kernel configuration
for this pwrseq library may not be chosen at all.

> 
> >
> >> Moreover, I have found yet another severe problem but reviewing the code:
> >> In the struct pwrseq, you have a "bool used", which you are setting to
> >> "true" once the pwrseq has been hooked up with the device, when a
> >> driver calls of_pwrseq_on(). Setting that variable to true, will also
> >> prevent another driver from using the same instance of the pwrseq for
> >> its device. So, to cope with multiple users, you register a new
> >> instance of the same pwrseq library that got hooked up, once the
> >> ->get() callback is about to complete.
> >>
> >> The problem the occurs, when there is another driver calling
> >> of_pwrseq_on() in between, meaning that the new instance has not yet
> >> been registered. This will simply fail, won't it?
> >
> > Yes, you are right, thanks for pointing that, I will add mutex_lock for
> > of_pwrseq_on.
> 
> Another option is to entirely skip to two step approach.
> 
> In other words, make the library to cope with multiple users via the
> same registered library instance.
> 

No, the pwrseq instance stores dtb information (clock, gpio, etc), it
needs to be per device.

-- 

Best Regards,
Peter Chen

Re: [PATCH v15 2/7] power: add power sequence library

[Peter Chen]

On Thu, Jun 15, 2017 at 10:11:45AM +0200, Ulf Hansson wrote:
> > Yes, you are right. This is the limitation for this power sequence
> > library, the registration for the 1st power sequence instance must
> > be finished before device driver uses it. I am appreciated that
> > you can supply some suggestions for it.
> 
> In general this kind of problems is solved by first parsing the DTB,
> which means you will find out whether there is a resource (a pwrseq)
> required for the device. Then you try to fetch that resource, and if
> that fails, it means the resource is not yet available, and hence you
> want to retry later and should return -EPROBE_DEFER.
> 
> In this case, of_pwrseq_on() needs to be converted to start looking
> for a pwrseq compatible in it's child node - I guess. Then if that is
> found, you try to fetch the instance of the corresponding library.
> Failing to fetch the library instance should then cause a return
> -EPROBE_DEFER.

The most difficulty for this is we can't know whether the requested
pwrseq instance will be registered or not, the kernel configuration
for this pwrseq library may not be chosen at all.

> 
> >
> >> Moreover, I have found yet another severe problem but reviewing the code:
> >> In the struct pwrseq, you have a "bool used", which you are setting to
> >> "true" once the pwrseq has been hooked up with the device, when a
> >> driver calls of_pwrseq_on(). Setting that variable to true, will also
> >> prevent another driver from using the same instance of the pwrseq for
> >> its device. So, to cope with multiple users, you register a new
> >> instance of the same pwrseq library that got hooked up, once the
> >> ->get() callback is about to complete.
> >>
> >> The problem the occurs, when there is another driver calling
> >> of_pwrseq_on() in between, meaning that the new instance has not yet
> >> been registered. This will simply fail, won't it?
> >
> > Yes, you are right, thanks for pointing that, I will add mutex_lock for
> > of_pwrseq_on.
> 
> Another option is to entirely skip to two step approach.
> 
> In other words, make the library to cope with multiple users via the
> same registered library instance.
> 

No, the pwrseq instance stores dtb information (clock, gpio, etc), it
needs to be per device.

-- 

Best Regards,
Peter Chen

Re: [PATCH v15 2/7] power: add power sequence library

[Ulf Hansson]

On 15 June 2017 at 08:58, Peter Chen  wrote:
> On Wed, Jun 14, 2017 at 10:53:29AM +0200, Ulf Hansson wrote:
>> On 14 June 2017 at 03:53, Peter Chen  wrote:
>> > On Tue, Jun 13, 2017 at 12:24:42PM +0200, Ulf Hansson wrote:
>> >> [...]
>> >>
>> >> > +
>> >> > +/**
>> >> > + * of_pwrseq_on - Carry out power sequence on for device node
>> >> > + *
>> >> > + * @np: the device node would like to power on
>> >> > + *
>> >> > + * Carry out a single device power on.  If multiple devices
>> >> > + * need to be handled, use of_pwrseq_on_list() instead.
>> >> > + *
>> >> > + * Return a pointer to the power sequence instance on success,
>> >> > + * or an error code otherwise.
>> >> > + */
>> >> > +struct pwrseq *of_pwrseq_on(struct device_node *np)
>> >> > +{
>> >> > +   struct pwrseq *pwrseq;
>> >> > +   int ret;
>> >> > +
>> >> > +   pwrseq = pwrseq_find_available_instance(np);
>> >> > +   if (!pwrseq)
>> >> > +   return ERR_PTR(-ENOENT);
>> >>
>> >> In case the pwrseq instance hasn't been registered yet, then there is
>> >> no way to deal with -EPROBE_DEFER properly here.
>> >>
>> >> I haven't been following the discussions in-depth during all
>> >> iterations, so perhaps you have already discussed why doing it like
>> >> this.
>> >
>> > Yes, it has been discussed. In order to compare with compatible string
>> > at dts, we need to have one registered pwrseq instance for each
>> > pwrseq library, this pre-registered one is allocated using
>> > postcore_initcall, and the new (eg, second) instance is registered
>> > after pwrseq_get has succeeded.
>>
>> I understand you need one compatible per pwrseq library, but how does
>> that have anything to do with -EPROBE_DEFER?
>>
>> My point is that, if a driver calls of_pwrseq_on() (which calls
>> pwrseq_find_available_instance()), but the corresponding pwrseq
>> library and instance has not yet been registered for that device. Then
>> how will you handle -EPROBE_DEFER? I guess you simply can't, which is
>> why *all* pwrseq libraries needs to be registered in early boot phase,
>> like at postcore_initcall(). Right?
>>
>> If that is the case, I really don't like it.
>>
>
> Yes, you are right. This is the limitation for this power sequence
> library, the registration for the 1st power sequence instance must
> be finished before device driver uses it. I am appreciated that
> you can supply some suggestions for it.

In general this kind of problems is solved by first parsing the DTB,
which means you will find out whether there is a resource (a pwrseq)
required for the device. Then you try to fetch that resource, and if
that fails, it means the resource is not yet available, and hence you
want to retry later and should return -EPROBE_DEFER.

In this case, of_pwrseq_on() needs to be converted to start looking
for a pwrseq compatible in it's child node - I guess. Then if that is
found, you try to fetch the instance of the corresponding library.
Failing to fetch the library instance should then cause a return
-EPROBE_DEFER.

>
>> Moreover, I have found yet another severe problem but reviewing the code:
>> In the struct pwrseq, you have a "bool used", which you are setting to
>> "true" once the pwrseq has been hooked up with the device, when a
>> driver calls of_pwrseq_on(). Setting that variable to true, will also
>> prevent another driver from using the same instance of the pwrseq for
>> its device. So, to cope with multiple users, you register a new
>> instance of the same pwrseq library that got hooked up, once the
>> ->get() callback is about to complete.
>>
>> The problem the occurs, when there is another driver calling
>> of_pwrseq_on() in between, meaning that the new instance has not yet
>> been registered. This will simply fail, won't it?
>
> Yes, you are right, thanks for pointing that, I will add mutex_lock for
> of_pwrseq_on.

Another option is to entirely skip to two step approach.

In other words, make the library to cope with multiple users via the
same registered library instance.

[...]

Kind regards
Uffe

Re: [PATCH v15 2/7] power: add power sequence library

[Ulf Hansson]

On 15 June 2017 at 08:58, Peter Chen  wrote:
> On Wed, Jun 14, 2017 at 10:53:29AM +0200, Ulf Hansson wrote:
>> On 14 June 2017 at 03:53, Peter Chen  wrote:
>> > On Tue, Jun 13, 2017 at 12:24:42PM +0200, Ulf Hansson wrote:
>> >> [...]
>> >>
>> >> > +
>> >> > +/**
>> >> > + * of_pwrseq_on - Carry out power sequence on for device node
>> >> > + *
>> >> > + * @np: the device node would like to power on
>> >> > + *
>> >> > + * Carry out a single device power on.  If multiple devices
>> >> > + * need to be handled, use of_pwrseq_on_list() instead.
>> >> > + *
>> >> > + * Return a pointer to the power sequence instance on success,
>> >> > + * or an error code otherwise.
>> >> > + */
>> >> > +struct pwrseq *of_pwrseq_on(struct device_node *np)
>> >> > +{
>> >> > +   struct pwrseq *pwrseq;
>> >> > +   int ret;
>> >> > +
>> >> > +   pwrseq = pwrseq_find_available_instance(np);
>> >> > +   if (!pwrseq)
>> >> > +   return ERR_PTR(-ENOENT);
>> >>
>> >> In case the pwrseq instance hasn't been registered yet, then there is
>> >> no way to deal with -EPROBE_DEFER properly here.
>> >>
>> >> I haven't been following the discussions in-depth during all
>> >> iterations, so perhaps you have already discussed why doing it like
>> >> this.
>> >
>> > Yes, it has been discussed. In order to compare with compatible string
>> > at dts, we need to have one registered pwrseq instance for each
>> > pwrseq library, this pre-registered one is allocated using
>> > postcore_initcall, and the new (eg, second) instance is registered
>> > after pwrseq_get has succeeded.
>>
>> I understand you need one compatible per pwrseq library, but how does
>> that have anything to do with -EPROBE_DEFER?
>>
>> My point is that, if a driver calls of_pwrseq_on() (which calls
>> pwrseq_find_available_instance()), but the corresponding pwrseq
>> library and instance has not yet been registered for that device. Then
>> how will you handle -EPROBE_DEFER? I guess you simply can't, which is
>> why *all* pwrseq libraries needs to be registered in early boot phase,
>> like at postcore_initcall(). Right?
>>
>> If that is the case, I really don't like it.
>>
>
> Yes, you are right. This is the limitation for this power sequence
> library, the registration for the 1st power sequence instance must
> be finished before device driver uses it. I am appreciated that
> you can supply some suggestions for it.

In general this kind of problems is solved by first parsing the DTB,
which means you will find out whether there is a resource (a pwrseq)
required for the device. Then you try to fetch that resource, and if
that fails, it means the resource is not yet available, and hence you
want to retry later and should return -EPROBE_DEFER.

In this case, of_pwrseq_on() needs to be converted to start looking
for a pwrseq compatible in it's child node - I guess. Then if that is
found, you try to fetch the instance of the corresponding library.
Failing to fetch the library instance should then cause a return
-EPROBE_DEFER.

>
>> Moreover, I have found yet another severe problem but reviewing the code:
>> In the struct pwrseq, you have a "bool used", which you are setting to
>> "true" once the pwrseq has been hooked up with the device, when a
>> driver calls of_pwrseq_on(). Setting that variable to true, will also
>> prevent another driver from using the same instance of the pwrseq for
>> its device. So, to cope with multiple users, you register a new
>> instance of the same pwrseq library that got hooked up, once the
>> ->get() callback is about to complete.
>>
>> The problem the occurs, when there is another driver calling
>> of_pwrseq_on() in between, meaning that the new instance has not yet
>> been registered. This will simply fail, won't it?
>
> Yes, you are right, thanks for pointing that, I will add mutex_lock for
> of_pwrseq_on.

Another option is to entirely skip to two step approach.

In other words, make the library to cope with multiple users via the
same registered library instance.

[...]

Kind regards
Uffe

Re: [PATCH v15 2/7] power: add power sequence library

[Peter Chen]

On Wed, Jun 14, 2017 at 10:53:29AM +0200, Ulf Hansson wrote:
> On 14 June 2017 at 03:53, Peter Chen  wrote:
> > On Tue, Jun 13, 2017 at 12:24:42PM +0200, Ulf Hansson wrote:
> >> [...]
> >>
> >> > +
> >> > +/**
> >> > + * of_pwrseq_on - Carry out power sequence on for device node
> >> > + *
> >> > + * @np: the device node would like to power on
> >> > + *
> >> > + * Carry out a single device power on.  If multiple devices
> >> > + * need to be handled, use of_pwrseq_on_list() instead.
> >> > + *
> >> > + * Return a pointer to the power sequence instance on success,
> >> > + * or an error code otherwise.
> >> > + */
> >> > +struct pwrseq *of_pwrseq_on(struct device_node *np)
> >> > +{
> >> > +   struct pwrseq *pwrseq;
> >> > +   int ret;
> >> > +
> >> > +   pwrseq = pwrseq_find_available_instance(np);
> >> > +   if (!pwrseq)
> >> > +   return ERR_PTR(-ENOENT);
> >>
> >> In case the pwrseq instance hasn't been registered yet, then there is
> >> no way to deal with -EPROBE_DEFER properly here.
> >>
> >> I haven't been following the discussions in-depth during all
> >> iterations, so perhaps you have already discussed why doing it like
> >> this.
> >
> > Yes, it has been discussed. In order to compare with compatible string
> > at dts, we need to have one registered pwrseq instance for each
> > pwrseq library, this pre-registered one is allocated using
> > postcore_initcall, and the new (eg, second) instance is registered
> > after pwrseq_get has succeeded.
> 
> I understand you need one compatible per pwrseq library, but how does
> that have anything to do with -EPROBE_DEFER?
> 
> My point is that, if a driver calls of_pwrseq_on() (which calls
> pwrseq_find_available_instance()), but the corresponding pwrseq
> library and instance has not yet been registered for that device. Then
> how will you handle -EPROBE_DEFER? I guess you simply can't, which is
> why *all* pwrseq libraries needs to be registered in early boot phase,
> like at postcore_initcall(). Right?
> 
> If that is the case, I really don't like it.
> 

Yes, you are right. This is the limitation for this power sequence
library, the registration for the 1st power sequence instance must
be finished before device driver uses it. I am appreciated that
you can supply some suggestions for it.

> Moreover, I have found yet another severe problem but reviewing the code:
> In the struct pwrseq, you have a "bool used", which you are setting to
> "true" once the pwrseq has been hooked up with the device, when a
> driver calls of_pwrseq_on(). Setting that variable to true, will also
> prevent another driver from using the same instance of the pwrseq for
> its device. So, to cope with multiple users, you register a new
> instance of the same pwrseq library that got hooked up, once the
> ->get() callback is about to complete.
> 
> The problem the occurs, when there is another driver calling
> of_pwrseq_on() in between, meaning that the new instance has not yet
> been registered. This will simply fail, won't it?

Yes, you are right, thanks for pointing that, I will add mutex_lock for
of_pwrseq_on.

> 
> Sorry for jumping in late, however to me it seems like there is still
> some pieces missing to make this work.
> 
> [...]
> 
> Kind regards
> Uffe

-- 

Best Regards,
Peter Chen

Re: [PATCH v15 2/7] power: add power sequence library

[Peter Chen]

On Wed, Jun 14, 2017 at 10:53:29AM +0200, Ulf Hansson wrote:
> On 14 June 2017 at 03:53, Peter Chen  wrote:
> > On Tue, Jun 13, 2017 at 12:24:42PM +0200, Ulf Hansson wrote:
> >> [...]
> >>
> >> > +
> >> > +/**
> >> > + * of_pwrseq_on - Carry out power sequence on for device node
> >> > + *
> >> > + * @np: the device node would like to power on
> >> > + *
> >> > + * Carry out a single device power on.  If multiple devices
> >> > + * need to be handled, use of_pwrseq_on_list() instead.
> >> > + *
> >> > + * Return a pointer to the power sequence instance on success,
> >> > + * or an error code otherwise.
> >> > + */
> >> > +struct pwrseq *of_pwrseq_on(struct device_node *np)
> >> > +{
> >> > +   struct pwrseq *pwrseq;
> >> > +   int ret;
> >> > +
> >> > +   pwrseq = pwrseq_find_available_instance(np);
> >> > +   if (!pwrseq)
> >> > +   return ERR_PTR(-ENOENT);
> >>
> >> In case the pwrseq instance hasn't been registered yet, then there is
> >> no way to deal with -EPROBE_DEFER properly here.
> >>
> >> I haven't been following the discussions in-depth during all
> >> iterations, so perhaps you have already discussed why doing it like
> >> this.
> >
> > Yes, it has been discussed. In order to compare with compatible string
> > at dts, we need to have one registered pwrseq instance for each
> > pwrseq library, this pre-registered one is allocated using
> > postcore_initcall, and the new (eg, second) instance is registered
> > after pwrseq_get has succeeded.
> 
> I understand you need one compatible per pwrseq library, but how does
> that have anything to do with -EPROBE_DEFER?
> 
> My point is that, if a driver calls of_pwrseq_on() (which calls
> pwrseq_find_available_instance()), but the corresponding pwrseq
> library and instance has not yet been registered for that device. Then
> how will you handle -EPROBE_DEFER? I guess you simply can't, which is
> why *all* pwrseq libraries needs to be registered in early boot phase,
> like at postcore_initcall(). Right?
> 
> If that is the case, I really don't like it.
> 

Yes, you are right. This is the limitation for this power sequence
library, the registration for the 1st power sequence instance must
be finished before device driver uses it. I am appreciated that
you can supply some suggestions for it.

> Moreover, I have found yet another severe problem but reviewing the code:
> In the struct pwrseq, you have a "bool used", which you are setting to
> "true" once the pwrseq has been hooked up with the device, when a
> driver calls of_pwrseq_on(). Setting that variable to true, will also
> prevent another driver from using the same instance of the pwrseq for
> its device. So, to cope with multiple users, you register a new
> instance of the same pwrseq library that got hooked up, once the
> ->get() callback is about to complete.
> 
> The problem the occurs, when there is another driver calling
> of_pwrseq_on() in between, meaning that the new instance has not yet
> been registered. This will simply fail, won't it?

Yes, you are right, thanks for pointing that, I will add mutex_lock for
of_pwrseq_on.

> 
> Sorry for jumping in late, however to me it seems like there is still
> some pieces missing to make this work.
> 
> [...]
> 
> Kind regards
> Uffe

-- 

Best Regards,
Peter Chen

Re: [PATCH v15 2/7] power: add power sequence library

[Ulf Hansson]

On 14 June 2017 at 03:53, Peter Chen  wrote:
> On Tue, Jun 13, 2017 at 12:24:42PM +0200, Ulf Hansson wrote:
>> [...]
>>
>> > +
>> > +/**
>> > + * of_pwrseq_on - Carry out power sequence on for device node
>> > + *
>> > + * @np: the device node would like to power on
>> > + *
>> > + * Carry out a single device power on.  If multiple devices
>> > + * need to be handled, use of_pwrseq_on_list() instead.
>> > + *
>> > + * Return a pointer to the power sequence instance on success,
>> > + * or an error code otherwise.
>> > + */
>> > +struct pwrseq *of_pwrseq_on(struct device_node *np)
>> > +{
>> > +   struct pwrseq *pwrseq;
>> > +   int ret;
>> > +
>> > +   pwrseq = pwrseq_find_available_instance(np);
>> > +   if (!pwrseq)
>> > +   return ERR_PTR(-ENOENT);
>>
>> In case the pwrseq instance hasn't been registered yet, then there is
>> no way to deal with -EPROBE_DEFER properly here.
>>
>> I haven't been following the discussions in-depth during all
>> iterations, so perhaps you have already discussed why doing it like
>> this.
>
> Yes, it has been discussed. In order to compare with compatible string
> at dts, we need to have one registered pwrseq instance for each
> pwrseq library, this pre-registered one is allocated using
> postcore_initcall, and the new (eg, second) instance is registered
> after pwrseq_get has succeeded.

I understand you need one compatible per pwrseq library, but how does
that have anything to do with -EPROBE_DEFER?

My point is that, if a driver calls of_pwrseq_on() (which calls
pwrseq_find_available_instance()), but the corresponding pwrseq
library and instance has not yet been registered for that device. Then
how will you handle -EPROBE_DEFER? I guess you simply can't, which is
why *all* pwrseq libraries needs to be registered in early boot phase,
like at postcore_initcall(). Right?

If that is the case, I really don't like it.

Moreover, I have found yet another severe problem but reviewing the code:
In the struct pwrseq, you have a "bool used", which you are setting to
"true" once the pwrseq has been hooked up with the device, when a
driver calls of_pwrseq_on(). Setting that variable to true, will also
prevent another driver from using the same instance of the pwrseq for
its device. So, to cope with multiple users, you register a new
instance of the same pwrseq library that got hooked up, once the
->get() callback is about to complete.

The problem the occurs, when there is another driver calling
of_pwrseq_on() in between, meaning that the new instance has not yet
been registered. This will simply fail, won't it?

Sorry for jumping in late, however to me it seems like there is still
some pieces missing to make this work.

[...]

Kind regards
Uffe

Re: [PATCH v15 2/7] power: add power sequence library

[Ulf Hansson]

On 14 June 2017 at 03:53, Peter Chen  wrote:
> On Tue, Jun 13, 2017 at 12:24:42PM +0200, Ulf Hansson wrote:
>> [...]
>>
>> > +
>> > +/**
>> > + * of_pwrseq_on - Carry out power sequence on for device node
>> > + *
>> > + * @np: the device node would like to power on
>> > + *
>> > + * Carry out a single device power on.  If multiple devices
>> > + * need to be handled, use of_pwrseq_on_list() instead.
>> > + *
>> > + * Return a pointer to the power sequence instance on success,
>> > + * or an error code otherwise.
>> > + */
>> > +struct pwrseq *of_pwrseq_on(struct device_node *np)
>> > +{
>> > +   struct pwrseq *pwrseq;
>> > +   int ret;
>> > +
>> > +   pwrseq = pwrseq_find_available_instance(np);
>> > +   if (!pwrseq)
>> > +   return ERR_PTR(-ENOENT);
>>
>> In case the pwrseq instance hasn't been registered yet, then there is
>> no way to deal with -EPROBE_DEFER properly here.
>>
>> I haven't been following the discussions in-depth during all
>> iterations, so perhaps you have already discussed why doing it like
>> this.
>
> Yes, it has been discussed. In order to compare with compatible string
> at dts, we need to have one registered pwrseq instance for each
> pwrseq library, this pre-registered one is allocated using
> postcore_initcall, and the new (eg, second) instance is registered
> after pwrseq_get has succeeded.

I understand you need one compatible per pwrseq library, but how does
that have anything to do with -EPROBE_DEFER?

My point is that, if a driver calls of_pwrseq_on() (which calls
pwrseq_find_available_instance()), but the corresponding pwrseq
library and instance has not yet been registered for that device. Then
how will you handle -EPROBE_DEFER? I guess you simply can't, which is
why *all* pwrseq libraries needs to be registered in early boot phase,
like at postcore_initcall(). Right?

If that is the case, I really don't like it.

Moreover, I have found yet another severe problem but reviewing the code:
In the struct pwrseq, you have a "bool used", which you are setting to
"true" once the pwrseq has been hooked up with the device, when a
driver calls of_pwrseq_on(). Setting that variable to true, will also
prevent another driver from using the same instance of the pwrseq for
its device. So, to cope with multiple users, you register a new
instance of the same pwrseq library that got hooked up, once the
->get() callback is about to complete.

The problem the occurs, when there is another driver calling
of_pwrseq_on() in between, meaning that the new instance has not yet
been registered. This will simply fail, won't it?

Sorry for jumping in late, however to me it seems like there is still
some pieces missing to make this work.

[...]

Kind regards
Uffe

Re: [PATCH v15 2/7] power: add power sequence library

[Peter Chen]

On Tue, Jun 13, 2017 at 12:24:42PM +0200, Ulf Hansson wrote:
> [...]
> 
> > +
> > +/**
> > + * of_pwrseq_on - Carry out power sequence on for device node
> > + *
> > + * @np: the device node would like to power on
> > + *
> > + * Carry out a single device power on.  If multiple devices
> > + * need to be handled, use of_pwrseq_on_list() instead.
> > + *
> > + * Return a pointer to the power sequence instance on success,
> > + * or an error code otherwise.
> > + */
> > +struct pwrseq *of_pwrseq_on(struct device_node *np)
> > +{
> > +   struct pwrseq *pwrseq;
> > +   int ret;
> > +
> > +   pwrseq = pwrseq_find_available_instance(np);
> > +   if (!pwrseq)
> > +   return ERR_PTR(-ENOENT);
> 
> In case the pwrseq instance hasn't been registered yet, then there is
> no way to deal with -EPROBE_DEFER properly here.
> 
> I haven't been following the discussions in-depth during all
> iterations, so perhaps you have already discussed why doing it like
> this.

Yes, it has been discussed. In order to compare with compatible string
at dts, we need to have one registered pwrseq instance for each
pwrseq library, this pre-registered one is allocated using
postcore_initcall, and the new (eg, second) instance is registered
after pwrseq_get has succeeded.

Peter

> 
> Anyway, that means all pwrseq instances needs to be registered an
> early boot level, to be safe. To me, that seems like poor design
> choice.
> 
> Otherwise I think this looks okay to me.
> 

-- 

Best Regards,
Peter Chen

Re: [PATCH v15 2/7] power: add power sequence library

[Peter Chen]

On Tue, Jun 13, 2017 at 12:24:42PM +0200, Ulf Hansson wrote:
> [...]
> 
> > +
> > +/**
> > + * of_pwrseq_on - Carry out power sequence on for device node
> > + *
> > + * @np: the device node would like to power on
> > + *
> > + * Carry out a single device power on.  If multiple devices
> > + * need to be handled, use of_pwrseq_on_list() instead.
> > + *
> > + * Return a pointer to the power sequence instance on success,
> > + * or an error code otherwise.
> > + */
> > +struct pwrseq *of_pwrseq_on(struct device_node *np)
> > +{
> > +   struct pwrseq *pwrseq;
> > +   int ret;
> > +
> > +   pwrseq = pwrseq_find_available_instance(np);
> > +   if (!pwrseq)
> > +   return ERR_PTR(-ENOENT);
> 
> In case the pwrseq instance hasn't been registered yet, then there is
> no way to deal with -EPROBE_DEFER properly here.
> 
> I haven't been following the discussions in-depth during all
> iterations, so perhaps you have already discussed why doing it like
> this.

Yes, it has been discussed. In order to compare with compatible string
at dts, we need to have one registered pwrseq instance for each
pwrseq library, this pre-registered one is allocated using
postcore_initcall, and the new (eg, second) instance is registered
after pwrseq_get has succeeded.

Peter

> 
> Anyway, that means all pwrseq instances needs to be registered an
> early boot level, to be safe. To me, that seems like poor design
> choice.
> 
> Otherwise I think this looks okay to me.
> 

-- 

Best Regards,
Peter Chen

Re: [PATCH v15 2/7] power: add power sequence library

[Ulf Hansson]

[...]

> +
> +/**
> + * of_pwrseq_on - Carry out power sequence on for device node
> + *
> + * @np: the device node would like to power on
> + *
> + * Carry out a single device power on.  If multiple devices
> + * need to be handled, use of_pwrseq_on_list() instead.
> + *
> + * Return a pointer to the power sequence instance on success,
> + * or an error code otherwise.
> + */
> +struct pwrseq *of_pwrseq_on(struct device_node *np)
> +{
> +   struct pwrseq *pwrseq;
> +   int ret;
> +
> +   pwrseq = pwrseq_find_available_instance(np);
> +   if (!pwrseq)
> +   return ERR_PTR(-ENOENT);

In case the pwrseq instance hasn't been registered yet, then there is
no way to deal with -EPROBE_DEFER properly here.

I haven't been following the discussions in-depth during all
iterations, so perhaps you have already discussed why doing it like
this.

Anyway, that means all pwrseq instances needs to be registered an
early boot level, to be safe. To me, that seems like poor design
choice.

[...]

Otherwise I think this looks okay to me.

Kind regards
Uffe

Re: [PATCH v15 2/7] power: add power sequence library

[Ulf Hansson]

[...]

> +
> +/**
> + * of_pwrseq_on - Carry out power sequence on for device node
> + *
> + * @np: the device node would like to power on
> + *
> + * Carry out a single device power on.  If multiple devices
> + * need to be handled, use of_pwrseq_on_list() instead.
> + *
> + * Return a pointer to the power sequence instance on success,
> + * or an error code otherwise.
> + */
> +struct pwrseq *of_pwrseq_on(struct device_node *np)
> +{
> +   struct pwrseq *pwrseq;
> +   int ret;
> +
> +   pwrseq = pwrseq_find_available_instance(np);
> +   if (!pwrseq)
> +   return ERR_PTR(-ENOENT);

In case the pwrseq instance hasn't been registered yet, then there is
no way to deal with -EPROBE_DEFER properly here.

I haven't been following the discussions in-depth during all
iterations, so perhaps you have already discussed why doing it like
this.

Anyway, that means all pwrseq instances needs to be registered an
early boot level, to be safe. To me, that seems like poor design
choice.

[...]

Otherwise I think this looks okay to me.

Kind regards
Uffe