Re: [PATCH RESEND] ARM: tegra: set regulator full constraints

Re: [PATCH RESEND] ARM: tegra: set regulator full constraints

[Wei Ni]

On 11/06/2013 04:51 PM, Mark Brown wrote:
> * PGP Signed by an unknown key
> 
> On Tue, Nov 05, 2013 at 05:05:15PM +0800, Wei Ni wrote:
>> On 11/05/2013 12:18 AM, Mark Brown wrote:
> 
>>> We can do it as part of parsing the DT or deciding to parse the DT.
> 
>> I checked the kernel codes, it seems the different arch uses different
>> way to parse DT, and I think these codes are related with DT, it's not
>> better to add such regulator full constraints codes.
> 
> There's a lot of shared code there, and it's most likely that any new
> platforms will be using the flattened device tree code.  Anything other
> than doing this when we start device tree is going to leave a window
> where we will be using device tree but not handling that in the
> regulator API.

I still can't find a good place to set full_constraints, could you
please show me some reference codes where we can set it?

BTW, add Cc to devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org

Thanks.
Wei.

> 
> * Unknown Key
> * 0x7EA229BD
> 

Re: [PATCH RESEND] ARM: tegra: set regulator full constraints

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 285 bytes --]

On Wed, Nov 06, 2013 at 06:39:03PM +0800, Wei Ni wrote:

> I still can't find a good place to set full_constraints, could you
> please show me some reference codes where we can set it?

The places where we unflatten the device tree for example, or start
instantiating devices from DT.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH RESEND] ARM: tegra: set regulator full constraints

[Wei Ni]

On 11/06/2013 07:01 PM, Mark Brown wrote:
> * PGP Signed by an unknown key
> 
> On Wed, Nov 06, 2013 at 06:39:03PM +0800, Wei Ni wrote:
> 
>> I still can't find a good place to set full_constraints, could you
>> please show me some reference codes where we can set it?
> 
> The places where we unflatten the device tree for example, or start
> instantiating devices from DT.

Hi, Mark
Sorry, I really don't familiar with the DT, could you please take this
by youself?

Thanks.
Wei.

> 
> * Unknown Key
> * 0x7EA229BD
> 

Re: [PATCH RESEND] ARM: tegra: set regulator full constraints

[Stephen Warren]

On 11/11/2013 10:52 PM, Wei Ni wrote:
> On 11/06/2013 07:01 PM, Mark Brown wrote:
>> * PGP Signed by an unknown key
>>
>> On Wed, Nov 06, 2013 at 06:39:03PM +0800, Wei Ni wrote:
>>
>>> I still can't find a good place to set full_constraints, could you
>>> please show me some reference codes where we can set it?
>>
>> The places where we unflatten the device tree for example, or start
>> instantiating devices from DT.
> 
> Hi, Mark
> Sorry, I really don't familiar with the DT, could you please take this
> by youself?

I think if you want a feature implemented or a patch created, it's up to
you to do so.

Isn't this as simple as moving the following code:

> 	/*
> 	 * Since DT doesn't provide an idiomatic mechanism for
> 	 * enabling full constraints and since it's much more natural
> 	 * with DT to provide them just assume that a DT enabled
> 	 * system has full constraints.
> 	 */
> 	if (of_have_populated_dt())
> 		has_full_constraints = true;

... out of regulator_init_complete() and into regulator_init()?

The only issue you may have to watch out for is: When is
regulator_init() called (i.e. when does core_initcall happen) relative
to when driver probe()s can be called? If it's earlier, then
core_initcall is early enough I suspect.

Re: [PATCH RESEND] ARM: tegra: set regulator full constraints

[Wei Ni]

On 11/13/2013 02:20 AM, Stephen Warren wrote:
> On 11/11/2013 10:52 PM, Wei Ni wrote:
>> On 11/06/2013 07:01 PM, Mark Brown wrote:
>>> * PGP Signed by an unknown key
>>>
>>> On Wed, Nov 06, 2013 at 06:39:03PM +0800, Wei Ni wrote:
>>>
>>>> I still can't find a good place to set full_constraints, could you
>>>> please show me some reference codes where we can set it?
>>>
>>> The places where we unflatten the device tree for example, or start
>>> instantiating devices from DT.
>>
>> Hi, Mark
>> Sorry, I really don't familiar with the DT, could you please take this
>> by youself?
> 
> I think if you want a feature implemented or a patch created, it's up to
> you to do so.
> 
> Isn't this as simple as moving the following code:

Yes, I had sent patch:
[PATCH v2] regulator: core: set full constraints in regulator_init
http://www.mail-archive.com/linux-tegra-u79uwXL29TY76Z2rM5mHXA@public.gmane.org/msg13788.html .
It's same as you said, but Mark think it should be set in DT.
If we set it in DT, it will affect all DT system, and I still didn't
find a good place to set it.

So I think it's better to let Mark to handle it.

Thanks.
Wei.

> 
>> 	/*
>> 	 * Since DT doesn't provide an idiomatic mechanism for
>> 	 * enabling full constraints and since it's much more natural
>> 	 * with DT to provide them just assume that a DT enabled
>> 	 * system has full constraints.
>> 	 */
>> 	if (of_have_populated_dt())
>> 		has_full_constraints = true;
> 
> ... out of regulator_init_complete() and into regulator_init()?
> 
> The only issue you may have to watch out for is: When is
> regulator_init() called (i.e. when does core_initcall happen) relative
> to when driver probe()s can be called? If it's earlier, then
> core_initcall is early enough I suspect.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH RESEND] ARM: tegra: set regulator full constraints

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 521 bytes --]

On Tue, Nov 12, 2013 at 11:20:44AM -0700, Stephen Warren wrote:

> The only issue you may have to watch out for is: When is
> regulator_init() called (i.e. when does core_initcall happen) relative
> to when driver probe()s can be called? If it's earlier, then
> core_initcall is early enough I suspect.

What I said was to set this up when we hand the DT over to the DT code
to be parsed so that we don't need to worry about any gaps like that,
it seems like a more direct solution than worrying about initcall
ordering.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH RESEND] ARM: tegra: set regulator full constraints

[Stephen Warren]

On 11/13/2013 05:23 AM, Mark Brown wrote:
> On Tue, Nov 12, 2013 at 11:20:44AM -0700, Stephen Warren wrote:
> 
>> The only issue you may have to watch out for is: When is 
>> regulator_init() called (i.e. when does core_initcall happen)
>> relative to when driver probe()s can be called? If it's earlier,
>> then core_initcall is early enough I suspect.
> 
> What I said was to set this up when we hand the DT over to the DT
> code to be parsed so that we don't need to worry about any gaps
> like that, it seems like a more direct solution than worrying about
> initcall ordering.

I guess this means putting the call to
regulator_has_full_constraints() somewhere inside
early_init_dt_scan(), unflatten_device_tree(), or just perhaps inside
of_platform_populate() (all in drivers/of/*.c).

It seems slightly odd to tightly link the drivers/of and
drivers/regulator code like that, but I guess with the appropriate
ifdefs it'll work out OK.

Re: [PATCH RESEND] ARM: tegra: set regulator full constraints

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 449 bytes --]

On Wed, Nov 13, 2013 at 10:29:07AM -0700, Stephen Warren wrote:

> It seems slightly odd to tightly link the drivers/of and
> drivers/regulator code like that, but I guess with the appropriate
> ifdefs it'll work out OK.

This isn't really regulator specific - it's something that applies in
general to things implementing deferred probing - so we ought to have a
generic "we know if devices can appear later or not" flag that
subsystems can check.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH RESEND] ARM: tegra: set regulator full constraints

[Stephen Warren]

On 11/13/2013 12:07 PM, Mark Brown wrote:
> On Wed, Nov 13, 2013 at 10:29:07AM -0700, Stephen Warren wrote:
> 
>> It seems slightly odd to tightly link the drivers/of and 
>> drivers/regulator code like that, but I guess with the
>> appropriate ifdefs it'll work out OK.
> 
> This isn't really regulator specific - it's something that applies
> in general to things implementing deferred probing - so we ought to
> have a generic "we know if devices can appear later or not" flag
> that subsystems can check.

I guess I misunderstand then, since given that modules exist, wouldn't
that flag always be true?

IIUC, the issue being discussed here isn't about deferred probe at
all. You always must defer probe if an object is specified as existing
yet the provider isn't available yet. The issue here is when a
regulator isn't specified as existing, yet something asks for that
regulator, should the regulator subsystem automatically provide a
dummy regulator instead, rather than erroring out.

Re: [PATCH RESEND] ARM: tegra: set regulator full constraints

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1345 bytes --]

On Wed, Nov 13, 2013 at 01:13:06PM -0700, Stephen Warren wrote:
> On 11/13/2013 12:07 PM, Mark Brown wrote:

> > This isn't really regulator specific - it's something that applies
> > in general to things implementing deferred probing - so we ought to
> > have a generic "we know if devices can appear later or not" flag
> > that subsystems can check.

> I guess I misunderstand then, since given that modules exist, wouldn't
> that flag always be true?

No, with DT you can say that if there is no DT binding configuring a
given thing (clock, regulator, GPIO or whatever) then no amount of
module loading will ever cause it to appear - this is what the flag in
question controls.

> IIUC, the issue being discussed here isn't about deferred probe at
> all. You always must defer probe if an object is specified as existing
> yet the provider isn't available yet. The issue here is when a
> regulator isn't specified as existing, yet something asks for that
> regulator, should the regulator subsystem automatically provide a
> dummy regulator instead, rather than erroring out.

No, it's a deferred probing thing - one of the effects here is that
we're choosing between deferring waiting for the resource we know is
bound to appear and substituting a dummy for something we know exists
physically and we know can never be provided by software.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH RESEND] ARM: tegra: set regulator full constraints

[Stephen Warren]

On 11/13/2013 01:49 PM, Mark Brown wrote:
> On Wed, Nov 13, 2013 at 01:13:06PM -0700, Stephen Warren wrote:
>> On 11/13/2013 12:07 PM, Mark Brown wrote:
> 
>>> This isn't really regulator specific - it's something that
>>> applies in general to things implementing deferred probing - so
>>> we ought to have a generic "we know if devices can appear later
>>> or not" flag that subsystems can check.
> 
>> I guess I misunderstand then, since given that modules exist,
>> wouldn't that flag always be true?
> 
> No, with DT you can say that if there is no DT binding configuring
> a given thing (clock, regulator, GPIO or whatever) then no amount
> of module loading will ever cause it to appear - this is what the
> flag in question controls.

But we do have a binding for regulators, so wouldn't that flag always
be true?

Perhaps you can suggest a name for the flag, and a specific set of
conditions when it will have specific values. That might help me
understand what you mean.

Are you confusing having a binding (schema definition) with having DT
content ("foo-supply" property)?

Re: [PATCH RESEND] ARM: tegra: set regulator full constraints

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1279 bytes --]

On Wed, Nov 13, 2013 at 02:59:01PM -0700, Stephen Warren wrote:
> On 11/13/2013 01:49 PM, Mark Brown wrote:

> > No, with DT you can say that if there is no DT binding configuring
> > a given thing (clock, regulator, GPIO or whatever) then no amount
> > of module loading will ever cause it to appear - this is what the
> > flag in question controls.

> But we do have a binding for regulators, so wouldn't that flag always
> be true?

In theory.  In practice people often add bindings for devices without
including the regulators and then someone comes along and adds the
regulators later, perhaps not even using a system with DT, and renders
all existing DTs buggy.  This is generally miserable for everyone so
it's better if we're liberal in what we accept.

> Perhaps you can suggest a name for the flag, and a specific set of
> conditions when it will have specific values. That might help me
> understand what you mean.

Well, of_have_populated_dt() is essentially doing the same thing
(probably, I don't know if it's set at quite the right time) - it's
saying we have a DT.  We could even change the users to check that as
well if it's doing the right thing.

> Are you confusing having a binding (schema definition) with having DT
> content ("foo-supply" property)?

No.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH RESEND] ARM: tegra: set regulator full constraints

[Stephen Warren]

On 11/13/2013 03:29 PM, Mark Brown wrote:
> On Wed, Nov 13, 2013 at 02:59:01PM -0700, Stephen Warren wrote:
>> On 11/13/2013 01:49 PM, Mark Brown wrote:
> 
>>> No, with DT you can say that if there is no DT binding configuring
>>> a given thing (clock, regulator, GPIO or whatever) then no amount
>>> of module loading will ever cause it to appear - this is what the
>>> flag in question controls.
> 
>> But we do have a binding for regulators, so wouldn't that flag always
>> be true?
> 
> In theory.  In practice people often add bindings for devices without
> including the regulators and then someone comes along and adds the
> regulators later, perhaps not even using a system with DT, and renders
> all existing DTs buggy.  This is generally miserable for everyone so
> it's better if we're liberal in what we accept.

But that's a per-binding issue. Earlier, you wrote:

>> The only issue you may have to watch out for is: When is
>> regulator_init() called (i.e. when does core_initcall happen) relative
>> to when driver probe()s can be called? If it's earlier, then
>> core_initcall is early enough I suspect.
> 
> What I said was to set this up when we hand the DT over to the DT code
> to be parsed so that we don't need to worry about any gaps like that,
> it seems like a more direct solution than worrying about initcall
> ordering.

That sounds like a system-global flag, not a per-device/binding flag.
Has the conversation shifted topics? And indeed, I've been talking about
the system-global has_full_constraints flag all along here.

>> Perhaps you can suggest a name for the flag, and a specific set of
>> conditions when it will have specific values. That might help me
>> understand what you mean.
> 
> Well, of_have_populated_dt() is essentially doing the same thing
> (probably, I don't know if it's set at quite the right time) - it's
> saying we have a DT.  We could even change the users to check that as
> well if it's doing the right thing.

But the regulator code already calls of_have_populated_dt() in order to
determine whether to set has_full_constraints = true; The only issue is
that it does it too late. If of_have_populated_dt() is the flag that the
code should key off, what's wrong with the suggestion I made yesterday
to simply move the existing code (that uses of_have_populated_dt()) to
an earlier location?

Re: [PATCH RESEND] ARM: tegra: set regulator full constraints

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1576 bytes --]

On Wed, Nov 13, 2013 at 04:41:23PM -0700, Stephen Warren wrote:
> On 11/13/2013 03:29 PM, Mark Brown wrote:

> > In theory.  In practice people often add bindings for devices without
> > including the regulators and then someone comes along and adds the
> > regulators later, perhaps not even using a system with DT, and renders
> > all existing DTs buggy.  This is generally miserable for everyone so
> > it's better if we're liberal in what we accept.

> But that's a per-binding issue. Earlier, you wrote:

No, it's an issue that affects running systems every time a binding gets
updated.  The binding doesn't care.

> That sounds like a system-global flag, not a per-device/binding flag.
> Has the conversation shifted topics? And indeed, I've been talking about
> the system-global has_full_constraints flag all along here.

Yes, this is a behaviour which is implemented by the system in order to
help keep device trees valid.

> But the regulator code already calls of_have_populated_dt() in order to
> determine whether to set has_full_constraints = true; The only issue is
> that it does it too late. If of_have_populated_dt() is the flag that the
> code should key off, what's wrong with the suggestion I made yesterday
> to simply move the existing code (that uses of_have_populated_dt()) to
> an earlier location?

This is a third suggestion which is to check have_populated_dt() at use
time not at some random point during initialisation which may or may not
be the right one.  And someone needs to check that this is actually the
check we want to use earlier on.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]