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

Groups > linux.kernel > #1254230 > unrolled thread

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

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 Mark Brown <broonie@kernel.org> - 2015-10-23 01:40 +0200
    Re: [PATCH v4 4/5] regulator: tps65912: Add regulator driver for the  TPS65912 PMIC "Andrew F. Davis" <afd@ti.com> - 2015-10-23 14:50 +0200
      Re: [PATCH v4 4/5] regulator: tps65912: Add regulator driver for the  TPS65912 PMIC Mark Brown <broonie@kernel.org> - 2015-10-24 01:20 +0200
        Re: [PATCH v4 4/5] regulator: tps65912: Add regulator driver for the  TPS65912 PMIC "Andrew F. Davis" <afd@ti.com> - 2015-10-24 02:20 +0200
          Re: [PATCH v4 4/5] regulator: tps65912: Add regulator driver for the  TPS65912 PMIC Mark Brown <broonie@kernel.org> - 2015-10-25 00:20 +0200
            Re: [PATCH v4 4/5] regulator: tps65912: Add regulator driver for the  TPS65912 PMIC "Andrew F. Davis" <afd@ti.com> - 2015-10-25 21:50 +0100
              Re: [PATCH v4 4/5] regulator: tps65912: Add regulator driver for the  TPS65912 PMIC Mark Brown <broonie@kernel.org> - 2015-10-26 01:50 +0100
                Re: [PATCH v4 4/5] regulator: tps65912: Add regulator driver for the  TPS65912 PMIC "Andrew F. Davis" <afd@ti.com> - 2015-10-26 16:50 +0100
                  Re: [PATCH v4 4/5] regulator: tps65912: Add regulator driver for the  TPS65912 PMIC Mark Brown <broonie@kernel.org> - 2015-10-27 01:20 +0100
                    Re: [PATCH v4 4/5] regulator: tps65912: Add regulator driver for the  TPS65912 PMIC "Andrew F. Davis" <afd@ti.com> - 2015-10-27 15:30 +0100

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

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

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.

[toc] | [next] | [standalone]

#1254567

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?

Not that I know of yet.

> If not why is it directly represented in DT?

My logic here is that when spins of this device are released they will
add/modify/remove regulators or GPIO pins or other on chip IP, with this
we should be able to simply describe the hardware change by loading
a compatible handler module ("ti,tps65912[x]-regulator" or something),
without having to change out the core or add a bunch of checks and flags.

I know just because other drivers do it doesn't mean it's a good idea,
but this is not new for MFDs and it is done in other regulators as well
(mt6397, tps659038, qcom,spmi, etc..).

> It seems like this is describing how Linux
> loads drivers not how the hardware is constructed but DT should describe
> the hardware.
>

While I agree to a point, if we follow this to its logical conclusion we
would end up with one compatible binding per SoC and be basically back to
board files. We need some granularity, just finding out where is the issue,
I would say that as these devices belong to different subsystems and are
almost completely independent there should be no problem with having their
own compatible matched hardware sub-node.
--
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]

#1255047

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

On Fri, Oct 23, 2015 at 07:46:39AM -0500, Andrew F. Davis wrote:

> I know just because other drivers do it doesn't mean it's a good idea,
> but this is not new for MFDs and it is done in other regulators as well
> (mt6397, tps659038, qcom,spmi, etc..).

mt6397 doesn't do this, it doesn't have a compatible string at all (it's
doing what I'm recommending that you do).  The SPMI devices are
standalone devices, their parent device is actually functioning as a bus
controller here (it's really a microcontroller inside the SoC).  The
Palmas is part of how we realised this was a problem.

> >It seems like this is describing how Linux
> >loads drivers not how the hardware is constructed but DT should describe
> >the hardware.

> While I agree to a point, if we follow this to its logical conclusion we
> would end up with one compatible binding per SoC and be basically back to
> board files. We need some granularity, just finding out where is the issue,

The fact that the SoC DT is not distinct from the board DT is actually
one of the problems with the way we're using DT at the minute, it means
that DTBs are much less stable than they should be since we can enhance
support for SoCs but DTBs need regenerating to take advantage of it.  It
would be much better if the boards just referenced the SoC they use and
pulled in a separate definition of the SoC (DT overlays will make it
much more tractable to implement that if someone has time...).

> I would say that as these devices belong to different subsystems and are
> almost completely independent there should be no problem with having their
> own compatible matched hardware sub-node.

All it's adding is more typing for users.

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

#1255070

On 10/23/2015 06:18 PM, Mark Brown wrote:
> On Fri, Oct 23, 2015 at 07:46:39AM -0500, Andrew F. Davis wrote:
>
>> I know just because other drivers do it doesn't mean it's a good idea,
>> but this is not new for MFDs and it is done in other regulators as well
>> (mt6397, tps659038, qcom,spmi, etc..).
>
> mt6397 doesn't do this, it doesn't have a compatible string at all (it's
> doing what I'm recommending that you do).  The SPMI devices are
> standalone devices, their parent device is actually functioning as a bus
> controller here (it's really a microcontroller inside the SoC).  The
> Palmas is part of how we realised this was a problem.
>

mt6397: Documentation/devicetree/bindings/mfd/mt6397.txt
Doing exactly what I'm doing,

pmic {
	compatible = "mediatek,mt6397";

	codec: mt6397codec {
		compatible = "mediatek,mt6397-codec";
	};

	regulators {
		compatible = "mediatek,mt6397-regulator";

		buck_vpca15 {
			....

The Palmas is a great example of why this is a good idea, there are
so many spins on this common base, and look how we can re-use sub-nodes:

tps659038: tps659038@58 {
	compatible = "ti,tps659038";
	reg = <0x58>;
	...

	tps659038_pmic {
		compatible = "ti,tps659038-pmic";
		...
	};

	tps659038_rtc: tps659038_rtc {
		compatible = "ti,palmas-rtc";
		...
	};

	tps659038_pwr_button: tps659038_pwr_button {
		compatible = "ti,palmas-pwrbutton";
		...
	};

	tps659038_gpio: tps659038_gpio {
		compatible = "ti,palmas-gpio";
		...
	};
};

(from am57xx-beagle-x15.dts)

looks like only the "ti,tps659038-pmic" node needed re-made without
re-making the whole driver.

>>> It seems like this is describing how Linux
>>> loads drivers not how the hardware is constructed but DT should describe
>>> the hardware.
>
>> While I agree to a point, if we follow this to its logical conclusion we
>> would end up with one compatible binding per SoC and be basically back to
>> board files. We need some granularity, just finding out where is the issue,
>
> The fact that the SoC DT is not distinct from the board DT is actually
> one of the problems with the way we're using DT at the minute, it means
> that DTBs are much less stable than they should be since we can enhance
> support for SoCs but DTBs need regenerating to take advantage of it.  It
> would be much better if the boards just referenced the SoC they use and
> pulled in a separate definition of the SoC (DT overlays will make it
> much more tractable to implement that if someone has time...).
>

I figured this can already be done by keeping the SoC stuff in dtsi files?
Anyway DT seems to have a lot of use issues with how it is being used, but
I'm probably not a person with enough free time for fixing that.. :|

>> I would say that as these devices belong to different subsystems and are
>> almost completely independent there should be no problem with having their
>> own compatible matched hardware sub-node.
>
> All it's adding is more typing for users.
>

Well I have to match the sub-devices on something, it's ether the node name
or the compatible string, so they might have to get used to typing :)

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

#1255256

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

On Fri, Oct 23, 2015 at 07:11:56PM -0500, Andrew F. Davis wrote:
> On 10/23/2015 06:18 PM, Mark Brown wrote:

> >mt6397 doesn't do this, it doesn't have a compatible string at all (it's
> >doing what I'm recommending that you do).  The SPMI devices are
> >standalone devices, their parent device is actually functioning as a bus
> >controller here (it's really a microcontroller inside the SoC).  The
> >Palmas is part of how we realised this was a problem.

> mt6397: Documentation/devicetree/bindings/mfd/mt6397.txt
> Doing exactly what I'm doing,

Tbe binding document is buggy and doesn't reflect the code, there's no
compatible string in the driver.

> The Palmas is a great example of why this is a good idea, there are
> so many spins on this common base, and look how we can re-use sub-nodes:

There's no real reuse here, we have to have a table in both the MFD and
regulator listing every variant.  Remember that the only reason the user
is having to type most of those subnodes at all is that we pushed things
into the DT, if someone forgot to include one of the nodes in their
board DT then they won't be able to use the relevant feature even if
it's there.

> >The fact that the SoC DT is not distinct from the board DT is actually
> >one of the problems with the way we're using DT at the minute, it means
> >that DTBs are much less stable than they should be since we can enhance
> >support for SoCs but DTBs need regenerating to take advantage of it.  It
> >would be much better if the boards just referenced the SoC they use and
> >pulled in a separate definition of the SoC (DT overlays will make it
> >much more tractable to implement that if someone has time...).

> I figured this can already be done by keeping the SoC stuff in dtsi files?

That doesn't help with the above issue, include files get processed at
the time the binary is generated.

> Well I have to match the sub-devices on something, it's ether the node name
> or the compatible string, so they might have to get used to typing :)

No, that's not the case - remember, users don't have to write a new
driver every time they instantiate a device on a board.  They're going
to have to list the in-use regulators one way or another but if we have
the extra compatible for regulators they have to bind both the core
device (which is going to be required anyway due to the control bus) and
the subnode saying that it has regulators (which we knew anyway as soon
as we knew we had the core device).

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

#1255556

On 10/24/2015 05:14 PM, Mark Brown wrote:
> On Fri, Oct 23, 2015 at 07:11:56PM -0500, Andrew F. Davis wrote:
>> On 10/23/2015 06:18 PM, Mark Brown wrote:
>
>>> mt6397 doesn't do this, it doesn't have a compatible string at all (it's
>>> doing what I'm recommending that you do).  The SPMI devices are
>>> standalone devices, their parent device is actually functioning as a bus
>>> controller here (it's really a microcontroller inside the SoC).  The
>>> Palmas is part of how we realised this was a problem.
>
>> mt6397: Documentation/devicetree/bindings/mfd/mt6397.txt
>> Doing exactly what I'm doing,
>
> Tbe binding document is buggy and doesn't reflect the code, there's no
> compatible string in the driver.
>

Sure there is:

drivers/mfd/mt6397-core.c:48:
.of_compatible = "mediatek,mt6397-regulator",

Then mfd_add_devices uses this to find the regulator node and fill
in .of_node, then in the regulator driver:

drivers/regulator/mt6397-regulator.c:48:
.of_match = of_match_ptr(match),

which uses your helper to match the nodes in the filled in .of_node.

>> The Palmas is a great example of why this is a good idea, there are
>> so many spins on this common base, and look how we can re-use sub-nodes:
>
> There's no real reuse here, we have to have a table in both the MFD and
> regulator listing every variant.

That's not true, the only variant that needs its own table is the tps65917,
and that's for the IRQ and regulator differences. The RTC, pwrbutton, and
GPIO nodes are completely variant agnostic and their drivers are reused
by just adding their subnode to a new PMIC device node.

> Remember that the only reason the user
> is having to type most of those subnodes at all is that we pushed things
> into the DT, if someone forgot to include one of the nodes in their
> board DT then they won't be able to use the relevant feature even if
> it's there.
>

This is what DT is for, we want to push this kind of thing into DT, the
driver should not have to know the devices hardware configuration anymore
that in needs to.

>>> The fact that the SoC DT is not distinct from the board DT is actually
>>> one of the problems with the way we're using DT at the minute, it means
>>> that DTBs are much less stable than they should be since we can enhance
>>> support for SoCs but DTBs need regenerating to take advantage of it.  It
>>> would be much better if the boards just referenced the SoC they use and
>>> pulled in a separate definition of the SoC (DT overlays will make it
>>> much more tractable to implement that if someone has time...).
>
>> I figured this can already be done by keeping the SoC stuff in dtsi files?
>
> That doesn't help with the above issue, include files get processed at
> the time the binary is generated.
>

Yeah so the board DT and the SoC DT are already mostly separate, it would
then just be a matter enforcing where nodes are defined.

>> Well I have to match the sub-devices on something, it's ether the node name
>> or the compatible string, so they might have to get used to typing :)
>
> No, that's not the case - remember, users don't have to write a new
> driver every time they instantiate a device on a board.  They're going
> to have to list the in-use regulators one way or another but if we have
> the extra compatible for regulators they have to bind both the core
> device (which is going to be required anyway due to the control bus) and
> the subnode saying that it has regulators (which we knew anyway as soon
> as we knew we had the core device).
>

We don't know what sub-devices the core device has, PMICs are more like
SoCs on a bus than a regular device, the sub-parts change with every spin and
we can represent this in DT like we do with SoCs. Else we would have to have
a new core binding for every spin. We know what devices are on a particular
SoC too, but we still list them and match them in DT so some SoC driver
doesn't have to.

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

#1255599

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

On Sun, Oct 25, 2015 at 03:45:43PM -0500, Andrew F. Davis wrote:
> On 10/24/2015 05:14 PM, Mark Brown wrote:

> >Tbe binding document is buggy and doesn't reflect the code, there's no
> >compatible string in the driver.

> Sure there is:

> drivers/mfd/mt6397-core.c:48:
> .of_compatible = "mediatek,mt6397-regulator",

This is in the MFD, this is not used in actual systems.

> Then mfd_add_devices uses this to find the regulator node and fill
> in .of_node, then in the regulator driver:

> drivers/regulator/mt6397-regulator.c:48:
> .of_match = of_match_ptr(match),

> which uses your helper to match the nodes in the filled in .of_node.

This is in a regulator definition, it is using the regulator framework
support for parsing DT which must be used by modern drivers.  It is not
part of how the Linux driver model device is instantiated, that is done
using the struct platform_driver which is what we are talking about
here.

Please stop this, it is getting very tiresome.  

> >No, that's not the case - remember, users don't have to write a new
> >driver every time they instantiate a device on a board.  They're going
> >to have to list the in-use regulators one way or another but if we have
> >the extra compatible for regulators they have to bind both the core
> >device (which is going to be required anyway due to the control bus) and
> >the subnode saying that it has regulators (which we knew anyway as soon
> >as we knew we had the core device).

> We don't know what sub-devices the core device has, PMICs are more like
> SoCs on a bus than a regular device, the sub-parts change with every spin and
> we can represent this in DT like we do with SoCs. Else we would have to have
> a new core binding for every spin. We know what devices are on a particular
> SoC too, but we still list them and match them in DT so some SoC driver
> doesn't have to.

PMICs are very much smaller than SoCs, and again if you're not able to
usefully represent individual IPs in the DT (as is *clearly* the case
here where you are trying to make one node for the entire collection of
regulators) we're not getting any value.

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

#1256119

On 10/25/2015 07:43 PM, Mark Brown wrote:
> On Sun, Oct 25, 2015 at 03:45:43PM -0500, Andrew F. Davis wrote:
>> On 10/24/2015 05:14 PM, Mark Brown wrote:
>
>>> Tbe binding document is buggy and doesn't reflect the code, there's no
>>> compatible string in the driver.
>
>> Sure there is:
>
>> drivers/mfd/mt6397-core.c:48:
>> .of_compatible = "mediatek,mt6397-regulator",
>
> This is in the MFD, this is not used in actual systems.
>

Not sure what you mean by "actual systems", it looks like these
use it?:

arch/arm/boot/dts/mt8135-evbp1.dts
arch/arm64/boot/dts/mediatek/mt8173-evb.dts

>> Then mfd_add_devices uses this to find the regulator node and fill
>> in .of_node, then in the regulator driver:
>
>> drivers/regulator/mt6397-regulator.c:48:
>> .of_match = of_match_ptr(match),
>
>> which uses your helper to match the nodes in the filled in .of_node.
>
> This is in a regulator definition, it is using the regulator framework
> support for parsing DT which must be used by modern drivers.  It is not
> part of how the Linux driver model device is instantiated, that is done
> using the struct platform_driver which is what we are talking about
> here.
>

I understand this, I thought we are talking about compatible strings in
the regulator sub-node, not how the core instantiates the sub-driver.

> Please stop this, it is getting very tiresome.
>

Sorry about that, I'm really not trying to prolong this, but I don't know
what you want. New multifunction devices have compatible strings in their
DT sub-nodes, the framework even helps support this (.of_compatible in
struct mfd_cell), I'm not doing anything new here.
--
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]

#1256355

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

On Mon, Oct 26, 2015 at 10:47:41AM -0500, Andrew F. Davis wrote:
> On 10/25/2015 07:43 PM, Mark Brown wrote:

> >>.of_compatible = "mediatek,mt6397-regulator",

> >This is in the MFD, this is not used in actual systems.

> Not sure what you mean by "actual systems", it looks like these
> use it?:

> arch/arm/boot/dts/mt8135-evbp1.dts
> arch/arm64/boot/dts/mediatek/mt8173-evb.dts

To repeat: the regulator driver does not register a compatible string
for the device for device model probing.

> I understand this, I thought we are talking about compatible strings in
> the regulator sub-node, not how the core instantiates the sub-driver.

No, this exactly about how the device model devices are instantiated.

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

#1256780

On 10/26/2015 07:16 PM, Mark Brown wrote:
> On Mon, Oct 26, 2015 at 10:47:41AM -0500, Andrew F. Davis wrote:
>> On 10/25/2015 07:43 PM, Mark Brown wrote:
>
>>>> .of_compatible = "mediatek,mt6397-regulator",
>
>>> This is in the MFD, this is not used in actual systems.
>
>> Not sure what you mean by "actual systems", it looks like these
>> use it?:
>
>> arch/arm/boot/dts/mt8135-evbp1.dts
>> arch/arm64/boot/dts/mediatek/mt8173-evb.dts
>
> To repeat: the regulator driver does not register a compatible string
> for the device for device model probing.
>
>> I understand this, I thought we are talking about compatible strings in
>> the regulator sub-node, not how the core instantiates the sub-driver.
>
> No, this exactly about how the device model devices are instantiated.
>

Ahh, I see, this makes more sense then, I'll re-spin this matching on
platform name.

Apologies about all this noise. :)
--
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