Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]


Groups > linux.kernel > #1262358 > unrolled thread

Re: [PATCH v4 4/5] regulator: tps65912: Add regulator driver for the TPS65912 PMIC

Started by"Andrew F. Davis" <afd@ti.com>
First post2015-11-04 16:40 +0100
Last post2015-11-10 20:50 +0100
Articles 13 — 2 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.


Contents

  Re: [PATCH v4 4/5] regulator: tps65912: Add regulator driver for the  TPS65912 PMIC "Andrew F. Davis" <afd@ti.com> - 2015-11-04 16:40 +0100
    Re: [PATCH v4 4/5] regulator: tps65912: Add regulator driver for the  TPS65912 PMIC Mark Brown <broonie@kernel.org> - 2015-11-05 11:20 +0100
      Re: [PATCH v4 4/5] regulator: tps65912: Add regulator driver for the  TPS65912 PMIC "Andrew F. Davis" <afd@ti.com> - 2015-11-05 19:10 +0100
        Re: [PATCH v4 4/5] regulator: tps65912: Add regulator driver for the  TPS65912 PMIC Mark Brown <broonie@kernel.org> - 2015-11-06 11:50 +0100
          Re: [PATCH v4 4/5] regulator: tps65912: Add regulator driver for the  TPS65912 PMIC "Andrew F. Davis" <afd@ti.com> - 2015-11-06 19:20 +0100
            Re: [PATCH v4 4/5] regulator: tps65912: Add regulator driver for the  TPS65912 PMIC Mark Brown <broonie@kernel.org> - 2015-11-06 22:20 +0100
              Re: [PATCH v4 4/5] regulator: tps65912: Add regulator driver for the  TPS65912 PMIC "Andrew F. Davis" <afd@ti.com> - 2015-11-09 18:50 +0100
                Re: [PATCH v4 4/5] regulator: tps65912: Add regulator driver for the  TPS65912 PMIC Mark Brown <broonie@kernel.org> - 2015-11-10 11:00 +0100
                  Re: [PATCH v4 4/5] regulator: tps65912: Add regulator driver for the  TPS65912 PMIC "Andrew F. Davis" <afd@ti.com> - 2015-11-10 17:50 +0100
                    Re: [PATCH v4 4/5] regulator: tps65912: Add regulator driver for the  TPS65912 PMIC Mark Brown <broonie@kernel.org> - 2015-11-10 18:10 +0100
                      Re: [PATCH v4 4/5] regulator: tps65912: Add regulator driver for the  TPS65912 PMIC "Andrew F. Davis" <afd@ti.com> - 2015-11-10 19:00 +0100
                        Re: [PATCH v4 4/5] regulator: tps65912: Add regulator driver for the  TPS65912 PMIC Mark Brown <broonie@kernel.org> - 2015-11-10 19:50 +0100
                          Re: [PATCH v4 4/5] regulator: tps65912: Add regulator driver for the  TPS65912 PMIC "Andrew F. Davis" <afd@ti.com> - 2015-11-10 20:50 +0100

#1262358 — Re: [PATCH v4 4/5] regulator: tps65912: Add regulator driver for the TPS65912 PMIC

From"Andrew F. Davis" <afd@ti.com>
Date2015-11-04 16:40 +0100
SubjectRe: [PATCH v4 4/5] regulator: tps65912: Add regulator driver for the TPS65912 PMIC
Message-ID<qr8kh-2go-13@gated-at.bofh.it>
On 10/22/2015 11:47 AM, Mark Brown wrote:
> On Thu, Oct 01, 2015 at 03:37:53PM -0500, Andrew F. Davis wrote:
>
>> +static const struct of_device_id tps65912_regulator_of_match_table[] = {
>> +	{ .compatible = "ti,tps65912-regulator", },
>> +	{ /* sentinel */ },
>> +};
>> +MODULE_DEVICE_TABLE(of, tps65912_regulator_of_match_table);
>
> Does this IP block exist outside of the tps65912?  If not why is it
> directly represented in DT?  It seems like this is describing how Linux
> loads drivers not how the hardware is constructed but DT should describe
> the hardware.
>

Something I just noticed, when I remove this table, module loading stops
working, even with 'MODULE_ALIAS("platform:tps65912-regulator");'. It
looks like when DT is enabled platform_uevent (drivers/base/platform.c:787)
only sends out the OF MODALIAS event then returns, not sending out the
platform event, is this desired behavior? If so then I will need this
table even though I still create the device and match it on platform
name as you suggested.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]


#1263068

FromMark Brown <broonie@kernel.org>
Date2015-11-05 11:20 +0100
Message-ID<qrpOa-5g9-13@gated-at.bofh.it>
In reply to#1262358

[Multipart message — attachments visible in raw view] — view raw

On Wed, Nov 04, 2015 at 09:35:26AM -0600, Andrew F. Davis wrote:

> Something I just noticed, when I remove this table, module loading stops
> working, even with 'MODULE_ALIAS("platform:tps65912-regulator");'. It
> looks like when DT is enabled platform_uevent (drivers/base/platform.c:787)
> only sends out the OF MODALIAS event then returns, not sending out the
> platform event, is this desired behavior? If so then I will need this
> table even though I still create the device and match it on platform
> name as you suggested.

That sounds like a bug to me, it'll have broken a bunch of existing
devices.

[toc] | [prev] | [next] | [standalone]


#1263461

From"Andrew F. Davis" <afd@ti.com>
Date2015-11-05 19:10 +0100
Message-ID<qrx8Z-1FQ-5@gated-at.bofh.it>
In reply to#1263068
On 11/05/2015 04:14 AM, Mark Brown wrote:
> On Wed, Nov 04, 2015 at 09:35:26AM -0600, Andrew F. Davis wrote:
>
>> Something I just noticed, when I remove this table, module loading stops
>> working, even with 'MODULE_ALIAS("platform:tps65912-regulator");'. It
>> looks like when DT is enabled platform_uevent (drivers/base/platform.c:787)
>> only sends out the OF MODALIAS event then returns, not sending out the
>> platform event, is this desired behavior? If so then I will need this
>> table even though I still create the device and match it on platform
>> name as you suggested.
>
> That sounds like a bug to me, it'll have broken a bunch of existing
> devices.
>

Most OF drivers have the OF MODALIAS.

'platform_uevent' can only emit one MODALIAS string per device (only
the last emitted one seems to count), so for any device with
'dev->of_node' set it will be the OF MODALIAS string. So I need
that table (to generate the OF MODALIAS) or this sub-device module
will not be loaded.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]


#1263913

FromMark Brown <broonie@kernel.org>
Date2015-11-06 11:50 +0100
Message-ID<qrMKK-3mu-11@gated-at.bofh.it>
In reply to#1263461

[Multipart message — attachments visible in raw view] — view raw

On Thu, Nov 05, 2015 at 12:04:00PM -0600, Andrew F. Davis wrote:
> On 11/05/2015 04:14 AM, Mark Brown wrote:

> >That sounds like a bug to me, it'll have broken a bunch of existing
> >devices.

> Most OF drivers have the OF MODALIAS.

That's nice but not relevant to non-OF devices.

> 'platform_uevent' can only emit one MODALIAS string per device (only
> the last emitted one seems to count), so for any device with
> 'dev->of_node' set it will be the OF MODALIAS string. So I need
> that table (to generate the OF MODALIAS) or this sub-device module
> will not be loaded.

No, you need to fix the bug that is causing dev->of_node to be populated
for the MFD function device.  Probably the issue is that you have put
this pointless compatible string in your DT.

Please stop this.  I don't understand why you are pushing so hard to put
the Linux device model representation of the device into DT but it's
getting very repetitive.

[toc] | [prev] | [next] | [standalone]


#1264208

From"Andrew F. Davis" <afd@ti.com>
Date2015-11-06 19:20 +0100
Message-ID<qrTMd-80j-7@gated-at.bofh.it>
In reply to#1263913
On 11/06/2015 04:43 AM, Mark Brown wrote:
> On Thu, Nov 05, 2015 at 12:04:00PM -0600, Andrew F. Davis wrote:
>> On 11/05/2015 04:14 AM, Mark Brown wrote:
>
>>> That sounds like a bug to me, it'll have broken a bunch of existing
>>> devices.
>
>> Most OF drivers have the OF MODALIAS.
>
> That's nice but not relevant to non-OF devices.
>
>> 'platform_uevent' can only emit one MODALIAS string per device (only
>> the last emitted one seems to count), so for any device with
>> 'dev->of_node' set it will be the OF MODALIAS string. So I need
>> that table (to generate the OF MODALIAS) or this sub-device module
>> will not be loaded.
>
> No, you need to fix the bug that is causing dev->of_node to be populated
> for the MFD function device.  Probably the issue is that you have put
> this pointless compatible string in your DT.
>

If it is pointless what is the reason we have .of_compatible in mfd_cell?
How else do you want us to populate the sub-device dev->of_node? Looking
at other DT regulators a lot *do( just use an OF table, others use their
parent's dev to get of_node, why all the push back on having an OF match
table? Probe gets called with the pdev filled with its of_node to begin with.

> Please stop this.  I don't understand why you are pushing so hard to put
> the Linux device model representation of the device into DT but it's
> getting very repetitive.
>

I'm not pushing anything, this is how other sub-nodes of MFD devices are
represented, I'm not sure what you think I'm doing that is so wrong here.
No one else seems to have an issue with the DT for this device, I see no
reason the regulator node has to be different than the other sub-device
nodes.

It looks rather out of place to have regulators be singled out like this,
for instance look at the mfd_cells for drivers/mfd/rt5033.c
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]


#1264566

FromMark Brown <broonie@kernel.org>
Date2015-11-06 22:20 +0100
Message-ID<qrWAq-1tw-25@gated-at.bofh.it>
In reply to#1264208

[Multipart message — attachments visible in raw view] — view raw

On Fri, Nov 06, 2015 at 12:10:45PM -0600, Andrew F. Davis wrote:
> On 11/06/2015 04:43 AM, Mark Brown wrote:

> >No, you need to fix the bug that is causing dev->of_node to be populated
> >for the MFD function device.  Probably the issue is that you have put
> >this pointless compatible string in your DT.

> If it is pointless what is the reason we have .of_compatible in mfd_cell?

There are cases where it's useful where we're abstracting something and
gaining some meaningful reuse.  This really does not appear to be one of
those cases, there are no parameters in the DT and the compatible string
is the full device name.

> How else do you want us to populate the sub-device dev->of_node? Looking

You do not need to populate it.  There is no value in populating it and
as previously discussed putting the Linux driver model into DT can be
actively harmful if we change our idea of how we should model things.

> >Please stop this.  I don't understand why you are pushing so hard to put
> >the Linux device model representation of the device into DT but it's
> >getting very repetitive.

> I'm not pushing anything, this is how other sub-nodes of MFD devices are

Every time we go through this we finish the discussion and then you come
back with yet another excuse for trying to push the current Linux device
model into the DT or another version of the patch with the same problem.

> represented, I'm not sure what you think I'm doing that is so wrong here.

I am providing the same review feedback repeatedly, this is not good.

> No one else seems to have an issue with the DT for this device, I see no
> reason the regulator node has to be different than the other sub-device
> nodes.

I would prefer it if other areas where there's no reuse gained by
breaking things down and where we're just encoding the Linux driver
model into DT weren't done like that either.  Perhaps other people care
less here, perhaps they haven't been bitten by the problems that can
arise.  If I read such patches I'd probably comment on the issue but
I've got enough to do already without trying to review every single DT
binding.

> It looks rather out of place to have regulators be singled out like this,
> for instance look at the mfd_cells for drivers/mfd/rt5033.c

The fact that other people have merged imperfect code into the kernel is
not a good reason to merge even more of it when we have better tools.
Looking at that binding I'm seeing no reason why any of the subfunctions
should have compatible strings (and if we're going down the route you're
trying to go down we really ought to have something in the binding for
at least an interrupt controller in there as well...).

Seriously, please stop this - having to go through the same things
repeatedly is not helpful.

[toc] | [prev] | [next] | [standalone]


#1265904

From"Andrew F. Davis" <afd@ti.com>
Date2015-11-09 18:50 +0100
Message-ID<qsYJQ-15E-29@gated-at.bofh.it>
In reply to#1264566
On 11/06/2015 03:16 PM, Mark Brown wrote:
> On Fri, Nov 06, 2015 at 12:10:45PM -0600, Andrew F. Davis wrote:
>> On 11/06/2015 04:43 AM, Mark Brown wrote:
>
>>> No, you need to fix the bug that is causing dev->of_node to be populated
>>> for the MFD function device.  Probably the issue is that you have put
>>> this pointless compatible string in your DT.
>
>> If it is pointless what is the reason we have .of_compatible in mfd_cell?
>
> There are cases where it's useful where we're abstracting something and
> gaining some meaningful reuse.  This really does not appear to be one of
> those cases, there are no parameters in the DT and the compatible string
> is the full device name.
>

As before I see no reason to make that call now and limit ourselves.

>> How else do you want us to populate the sub-device dev->of_node? Looking
>
> You do not need to populate it.  There is no value in populating it and
> as previously discussed putting the Linux driver model into DT can be
> actively harmful if we change our idea of how we should model things.
>

The dev passed to regulator_register needs to have of_node populated for
your OF init_data helper to work. Devices with OF tables can just pass
their own dev. Others have to use their parents' nodes, this is a
workaround, OF devices should be probed with their of_node pre-populated.

>>> Please stop this.  I don't understand why you are pushing so hard to put
>>> the Linux device model representation of the device into DT but it's
>>> getting very repetitive.
>
>> I'm not pushing anything, this is how other sub-nodes of MFD devices are
>
> Every time we go through this we finish the discussion and then you come
> back with yet another excuse for trying to push the current Linux device
> model into the DT or another version of the patch with the same problem.
>

I keep finding different problems, do you expect me to ignore them?

>> represented, I'm not sure what you think I'm doing that is so wrong here.
>
> I am providing the same review feedback repeatedly, this is not good.
>
>> No one else seems to have an issue with the DT for this device, I see no
>> reason the regulator node has to be different than the other sub-device
>> nodes.
>
> I would prefer it if other areas where there's no reuse gained by
> breaking things down and where we're just encoding the Linux driver
> model into DT weren't done like that either.  Perhaps other people care
> less here, perhaps they haven't been bitten by the problems that can
> arise.  If I read such patches I'd probably comment on the issue but
> I've got enough to do already without trying to review every single DT
> binding.
>
>> It looks rather out of place to have regulators be singled out like this,
>> for instance look at the mfd_cells for drivers/mfd/rt5033.c
>
> The fact that other people have merged imperfect code into the kernel is
> not a good reason to merge even more of it when we have better tools.
> Looking at that binding I'm seeing no reason why any of the subfunctions
> should have compatible strings (and if we're going down the route you're
> trying to go down we really ought to have something in the binding for
> at least an interrupt controller in there as well...).
>

These are not "subfunctions" they are full drivers, they only need
register accessors passed in, they do not call the core and the core
does not call them.

If your problem is with the DT binding for this or other MFDs, then
nack *them* and explain to everyone why what they are doing is wrong
and why regulators should be special cases. Blocking the regulator
drivers to force a change in DT is not going to fix this issue.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]


#1266369

FromMark Brown <broonie@kernel.org>
Date2015-11-10 11:00 +0100
Message-ID<qtdSy-3na-15@gated-at.bofh.it>
In reply to#1265904

[Multipart message — attachments visible in raw view] — view raw

On Mon, Nov 09, 2015 at 11:41:20AM -0600, Andrew F. Davis wrote:
> On 11/06/2015 03:16 PM, Mark Brown wrote:

> >There are cases where it's useful where we're abstracting something and
> >gaining some meaningful reuse.  This really does not appear to be one of
> >those cases, there are no parameters in the DT and the compatible string
> >is the full device name.

> As before I see no reason to make that call now and limit ourselves.

To repeat *yet* *again* the point is that putting the current Linux
driver model into the DT is limiting our future selves.

> >You do not need to populate it.  There is no value in populating it and
> >as previously discussed putting the Linux driver model into DT can be
> >actively harmful if we change our idea of how we should model things.

> The dev passed to regulator_register needs to have of_node populated for
> your OF init_data helper to work. Devices with OF tables can just pass
> their own dev. Others have to use their parents' nodes, this is a
> workaround, OF devices should be probed with their of_node pre-populated.

This is not a workaroud, the only reason you think it is a workaround is
the desire to directly represent the Linux device model in the DT.

> >>>Please stop this.  I don't understand why you are pushing so hard to put
> >>>the Linux device model representation of the device into DT but it's
> >>>getting very repetitive.

> >>I'm not pushing anything, this is how other sub-nodes of MFD devices are

> >Every time we go through this we finish the discussion and then you come
> >back with yet another excuse for trying to push the current Linux device
> >model into the DT or another version of the patch with the same problem.

> I keep finding different problems, do you expect me to ignore them?

You are making minor restatements of the same thing over and over again
which ignore the main feedback.

> >The fact that other people have merged imperfect code into the kernel is
> >not a good reason to merge even more of it when we have better tools.
> >Looking at that binding I'm seeing no reason why any of the subfunctions
> >should have compatible strings (and if we're going down the route you're
> >trying to go down we really ought to have something in the binding for
> >at least an interrupt controller in there as well...).

> These are not "subfunctions" they are full drivers, they only need
> register accessors passed in, they do not call the core and the core
> does not call them.

To repeat *yet* *again* they are groupings of functionality which happen
to represent the way Linux models devices right now.  There's no
generality in there, it's just a dump of the current Linux model of the
functions into the DT.

> If your problem is with the DT binding for this or other MFDs, then
> nack *them* and explain to everyone why what they are doing is wrong
> and why regulators should be special cases. Blocking the regulator
> drivers to force a change in DT is not going to fix this issue.

Of course this is a negative review of the binding!  What on earth did
you think my feedback meant?  The driver and the binding go together.

[toc] | [prev] | [next] | [standalone]


#1266635

From"Andrew F. Davis" <afd@ti.com>
Date2015-11-10 17:50 +0100
Message-ID<qtkhj-7vy-11@gated-at.bofh.it>
In reply to#1266369
On 11/10/2015 03:57 AM, Mark Brown wrote:
> On Mon, Nov 09, 2015 at 11:41:20AM -0600, Andrew F. Davis wrote:
>> On 11/06/2015 03:16 PM, Mark Brown wrote:
>
>>> There are cases where it's useful where we're abstracting something and
>>> gaining some meaningful reuse.  This really does not appear to be one of
>>> those cases, there are no parameters in the DT and the compatible string
>>> is the full device name.
>
>> As before I see no reason to make that call now and limit ourselves.
>
> To repeat *yet* *again* the point is that putting the current Linux
> driver model into the DT is limiting our future selves.
>
>>> You do not need to populate it.  There is no value in populating it and
>>> as previously discussed putting the Linux driver model into DT can be
>>> actively harmful if we change our idea of how we should model things.
>
>> The dev passed to regulator_register needs to have of_node populated for
>> your OF init_data helper to work. Devices with OF tables can just pass
>> their own dev. Others have to use their parents' nodes, this is a
>> workaround, OF devices should be probed with their of_node pre-populated.
>
> This is not a workaroud, the only reason you think it is a workaround is
> the desire to directly represent the Linux device model in the DT.
>
>>>>> Please stop this.  I don't understand why you are pushing so hard to put
>>>>> the Linux device model representation of the device into DT but it's
>>>>> getting very repetitive.
>
>>>> I'm not pushing anything, this is how other sub-nodes of MFD devices are
>
>>> Every time we go through this we finish the discussion and then you come
>>> back with yet another excuse for trying to push the current Linux device
>>> model into the DT or another version of the patch with the same problem.
>
>> I keep finding different problems, do you expect me to ignore them?
>
> You are making minor restatements of the same thing over and over again
> which ignore the main feedback.
>
>>> The fact that other people have merged imperfect code into the kernel is
>>> not a good reason to merge even more of it when we have better tools.
>>> Looking at that binding I'm seeing no reason why any of the subfunctions
>>> should have compatible strings (and if we're going down the route you're
>>> trying to go down we really ought to have something in the binding for
>>> at least an interrupt controller in there as well...).
>
>> These are not "subfunctions" they are full drivers, they only need
>> register accessors passed in, they do not call the core and the core
>> does not call them.
>
> To repeat *yet* *again* they are groupings of functionality which happen
> to represent the way Linux models devices right now.  There's no
> generality in there, it's just a dump of the current Linux model of the
> functions into the DT.
>

I've made different points every time, you are repeating yourself
because you only have one counter, you don't like what you perceive as
putting the "Linux device model representation of the device into DT".
I understand this, I simply don't agree that is what is going on, or
that this way will cause us any problems in the future.

>> If your problem is with the DT binding for this or other MFDs, then
>> nack *them* and explain to everyone why what they are doing is wrong
>> and why regulators should be special cases. Blocking the regulator
>> drivers to force a change in DT is not going to fix this issue.
>
> Of course this is a negative review of the binding!  What on earth did
> you think my feedback meant?  The driver and the binding go together.
>

The bindings should be driver/platform/OS agnostic, changing the bindings
because the Linux regulator subsystem maintainer doesn't like them
in regulator drivers is then not correct.

If the binding is accepted then the regulator driver will just have
to deal with it, so as I said, why not nack the bindings patch, and
explain your objection where DT maintainers might see it.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]


#1266650

FromMark Brown <broonie@kernel.org>
Date2015-11-10 18:10 +0100
Message-ID<qtkAG-7SL-21@gated-at.bofh.it>
In reply to#1266635

[Multipart message — attachments visible in raw view] — view raw

On Tue, Nov 10, 2015 at 10:47:33AM -0600, Andrew F. Davis wrote:
> On 11/10/2015 03:57 AM, Mark Brown wrote:

> >Of course this is a negative review of the binding!  What on earth did
> >you think my feedback meant?  The driver and the binding go together.

> The bindings should be driver/platform/OS agnostic, changing the bindings
> because the Linux regulator subsystem maintainer doesn't like them
> in regulator drivers is then not correct.

> If the binding is accepted then the regulator driver will just have
> to deal with it, so as I said, why not nack the bindings patch, and
> explain your objection where DT maintainers might see it.

If I'm not going to merge the driver because of issues in the DT code it
is vanishingly unlikely that I'm going to merge the regulator bindings
either.  I would have thought it should be clear that my review comments
cover both the manifestation of the bindings in the driver and the
bindings themselves. 

[toc] | [prev] | [next] | [standalone]


#1266677

From"Andrew F. Davis" <afd@ti.com>
Date2015-11-10 19:00 +0100
Message-ID<qtln5-8aF-21@gated-at.bofh.it>
In reply to#1266650
On 11/10/2015 11:04 AM, Mark Brown wrote:
> On Tue, Nov 10, 2015 at 10:47:33AM -0600, Andrew F. Davis wrote:
>> On 11/10/2015 03:57 AM, Mark Brown wrote:
>
>>> Of course this is a negative review of the binding!  What on earth did
>>> you think my feedback meant?  The driver and the binding go together.
>
>> The bindings should be driver/platform/OS agnostic, changing the bindings
>> because the Linux regulator subsystem maintainer doesn't like them
>> in regulator drivers is then not correct.
>
>> If the binding is accepted then the regulator driver will just have
>> to deal with it, so as I said, why not nack the bindings patch, and
>> explain your objection where DT maintainers might see it.
>
> If I'm not going to merge the driver because of issues in the DT code it
> is vanishingly unlikely that I'm going to merge the regulator bindings
> either.  I would have thought it should be clear that my review comments
> cover both the manifestation of the bindings in the driver and the
> bindings themselves.
>

Kind of an interesting situation, if I didn't have the regulator as a separate
node like you want, then I wouldn't really need a separate regulator binding Doc,
for you to merge, it could all be merged as a single MFD binding.

Anyway, All I'm trying to do here is keep things clean in the DT. We only have
one consistent option:

Match all sub parts by compatible:

tps65912: tps65912@2d {
	compatible = "ti,tps65912";
	reg = <0x58>;
	interrupts ...

	regulator {
		compatible = "ti,tps65912-regulator";
		dcdc1 {
			regulator-name = "vdd_core";
			regulator-min-microvolt = <912000>;
			regulator-max-microvolt = <1144000>;
		};
		...
	};

	pwrbutton {
		compatible = "ti,palmas-pwrbutton";
		interrupt-parent = <&tps65912>;
		interrupts = <1 IRQ_TYPE_EDGE_FALLING>;
		wakeup-source;
		ti,palmas-long-press-seconds = <12>;
	};

	gpio {
		compatible = "ti,palmas-gpio";
		gpio-controller;
		#gpio-cells = <2>;
	};
	...
};

Or we end up with some hybrid approach, matching some on node name, others
on compatible when needed. Yes, the above matches Linux device model (still
not sure why that is such a problem?), but it also matches modular functionality
in the device.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]


#1266720

FromMark Brown <broonie@kernel.org>
Date2015-11-10 19:50 +0100
Message-ID<qtm9s-gB-13@gated-at.bofh.it>
In reply to#1266677

[Multipart message — attachments visible in raw view] — view raw

On Tue, Nov 10, 2015 at 11:52:12AM -0600, Andrew F. Davis wrote:

> Anyway, All I'm trying to do here is keep things clean in the DT. We only have
> one consistent option:

No, not really.

> Match all sub parts by compatible:

> Or we end up with some hybrid approach, matching some on node name, others
> on compatible when needed. Yes, the above matches Linux device model (still
> not sure why that is such a problem?), but it also matches modular functionality
> in the device.

There's also the third option where we don't have any compatible strings
in the subnodes at all.

[toc] | [prev] | [next] | [standalone]


#1266748

From"Andrew F. Davis" <afd@ti.com>
Date2015-11-10 20:50 +0100
Message-ID<qtn5w-Uo-29@gated-at.bofh.it>
In reply to#1266720
On 11/10/2015 12:44 PM, Mark Brown wrote:
> On Tue, Nov 10, 2015 at 11:52:12AM -0600, Andrew F. Davis wrote:
>
>> Anyway, All I'm trying to do here is keep things clean in the DT. We only have
>> one consistent option:
>
> No, not really.
>
>> Match all sub parts by compatible:
>
>> Or we end up with some hybrid approach, matching some on node name, others
>> on compatible when needed. Yes, the above matches Linux device model (still
>> not sure why that is such a problem?), but it also matches modular functionality
>> in the device.
>
> There's also the third option where we don't have any compatible strings
> in the subnodes at all.
>

Ok, two, but would you really want to go that way? Matching by node name costs
us all of the flexibility of DT sub-device selection. Still don't see an upside
as we would now be locked to node names instead of compatible strings to declare
component type compatibility (what they are for).
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]


Back to top | Article view | linux.kernel


csiph-web