Re: [PATCH] ASoC: twl6040: Support for DT

Re: [PATCH] ASoC: twl6040: Support for DT

[Cousson, Benoit]

+ DT list

Hi Mark,

On 5/9/2012 3:35 PM, Mark Brown wrote:
> On Wed, May 09, 2012 at 03:01:23PM +0300, Peter Ujfalusi wrote:
>> On 05/08/2012 04:41 PM, Mark Brown wrote:
>>> On Tue, May 08, 2012 at 03:42:41PM +0300, Peter Ujfalusi wrote:
>
>>>> Also without a DT entry I will not have a way to use phandle to
>>>> connect the codec in the machine driver.
>
>>> Of course you do!  There's going to be a device tree entry for the
>>> chip, things can point at this perfectly happily.
>
>> That's true but what can I do with that in the ASoC machine driver?
>> The chip driver is _not_ the codec driver.
>
> This really doesn't seem like it should be at all hard to resolve...
>
>> I can craft the pdata for the MFD child devices out from the DT
>> information but then the child devices will have no dev.of_node since
>> they were not probed via OF.  So I would have not phandle for the
>> codec to be used to define the dai-link connection within the sound
>> dts section.
>
>> Am I missing something here?
>
> Clearly.  This is all very circular.  Obviously if you're intent on
> using a phandle specific to the MFD child then you need to have that in
> the device tree but this is because you're making the child devices
> externally visible...  Clearly if we're not going to use the MFD
> subdevices in the DT then the links ought to reference the chip.

I'm not sure to understand you concern here.

Describing sub nodes, especially for big SoC is pretty useful.
It is as useful as doing that for board that are sharing similar components.
It will allow to define several Audio / PMIC variants without having to 
rewrite a driver potentially.

DT is about HW description. You can go to whatever level you'd like as 
soon as it is relevant for the drivers.

In this is case it is relevant since the sub-devices will have driver to 
be bound to the device nodes.

Both Vibra and Codec IPs can be located elsewhere, so by exposing that 
inside the DT, you will increase the level of HW details and thus make 
the re-use of these sub-IPs easier.

The twl6040 is just a container. These IPs can be inside a PCI chip, or 
inside OMAP or whatever, but they do need a node to represent them. In 
this case the container is irrelevant, only the children matter.

Moreover, the fact the Linux implementation uses MFD to represent that 
is irrelevant for the DT description. We should be able to use whatever 
SW representation for this type of HW.

DT was useful to get rid of static board definition. DT can be useful as 
well to get rid of the static cell definition inside a MFD.
This is exactly the same problem at different HW level.

Regards,
Benoit

Re: [PATCH] ASoC: twl6040: Support for DT

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 2571 bytes --]

On Fri, May 11, 2012 at 01:02:26PM +0200, Cousson, Benoit wrote:
> On 5/9/2012 3:35 PM, Mark Brown wrote:

> >Clearly.  This is all very circular.  Obviously if you're intent on
> >using a phandle specific to the MFD child then you need to have that in
> >the device tree but this is because you're making the child devices
> >externally visible...  Clearly if we're not going to use the MFD
> >subdevices in the DT then the links ought to reference the chip.

> I'm not sure to understand you concern here.

> Describing sub nodes, especially for big SoC is pretty useful.
> It is as useful as doing that for board that are sharing similar components.

The concern here is that the device tree you're writing here is clearly
just a direct translation of the particular stuff Linux happens to use
internally into device tree; this is similar to the thing with using
hwmod in the device tree representation and omitting basic stuff like
the register ranges.

> It will allow to define several Audio / PMIC variants without having
> to rewrite a driver potentially.

This binding doesn't do anything to move towards that goal given that
the only information it includes about the contents of the chip is the 
name.  Writing the name out in separate CODEC and vibra nodes really
isn't going to accomplish much to promote reuse that can't trivially be
achieved by parsing the name in the MFD driver.

If the binding were doing things like describing the internals of the
device in a way that meant the driver didn't need to know that this was
a twl6040 in particular this sort of thinking is useful but the binding
we have here just isn't doing that at all.

> Both Vibra and Codec IPs can be located elsewhere, so by exposing
> that inside the DT, you will increase the level of HW details and
> thus make the re-use of these sub-IPs easier.

Especially for the CODEC it's not really an IP in itself, it's an
assembly of large numbers of other IPs - digital audio interfaces,
analogue amps and whatnot.  Like I say if the device tree described
this assembly it'd be different but it's really not doing that.

> Moreover, the fact the Linux implementation uses MFD to represent
> that is irrelevant for the DT description. We should be able to use
> whatever SW representation for this type of HW.

This is precisely my point, what we're being presented with here is a
device tree description of the particular way that Linux wants to
understand this stuff rather than something that lets us learn about the
chip internals.

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

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH] ASoC: twl6040: Support for DT

[Cousson, Benoit]

On 5/11/2012 3:08 PM, Mark Brown wrote:
> On Fri, May 11, 2012 at 01:02:26PM +0200, Cousson, Benoit wrote:
>> On 5/9/2012 3:35 PM, Mark Brown wrote:
>
>>> Clearly.  This is all very circular.  Obviously if you're intent on
>>> using a phandle specific to the MFD child then you need to have that in
>>> the device tree but this is because you're making the child devices
>>> externally visible...  Clearly if we're not going to use the MFD
>>> subdevices in the DT then the links ought to reference the chip.
>
>> I'm not sure to understand you concern here.
>
>> Describing sub nodes, especially for big SoC is pretty useful.
>> It is as useful as doing that for board that are sharing similar components.
>
> The concern here is that the device tree you're writing here is clearly
> just a direct translation of the particular stuff Linux happens to use
> internally into device tree; this is similar to the thing with using
> hwmod in the device tree representation and omitting basic stuff like
> the register ranges.
>
>> It will allow to define several Audio / PMIC variants without having
>> to rewrite a driver potentially.
>
> This binding doesn't do anything to move towards that goal given that
> the only information it includes about the contents of the chip is the
> name.

But it does not have to expose everything. And what will be your 
definition of everything in that case?

> Writing the name out in separate CODEC and vibra nodes really
> isn't going to accomplish much to promote reuse that can't trivially be
> achieved by parsing the name in the MFD driver.

Maybe, but... so what? This is not because you can do it with MFD that 
you cannot make it more flexible with DT.
Preferring to hard code that in MFD is similar to keeping all the static 
C board files we are all trying to remove. It is possible, sure, but 
there are now better way to describe HW modules than hard-coding that in 
a C file.

Moreover, there is not an unique way to describe the HW. So for sure we 
will cheat a little bit and make some assumption about what the SW will 
really use. But otherwise you will end up putting the full RTL inside 
the DT node.
It is very similar to the discussion we had for the clock tree. For sure 
we can describe the full clock tree in DT, but that's huge and useless. 
So we are taking some assumption and expose only the ones that have to 
be exposed because dependent of the board or needed by the devices.

> If the binding were doing things like describing the internals of the
> device in a way that meant the driver didn't need to know that this was
> a twl6040 in particular this sort of thinking is useful but the binding
> we have here just isn't doing that at all.
>
>> Both Vibra and Codec IPs can be located elsewhere, so by exposing
>> that inside the DT, you will increase the level of HW details and
>> thus make the re-use of these sub-IPs easier.
>
> Especially for the CODEC it's not really an IP in itself, it's an
> assembly of large numbers of other IPs - digital audio interfaces,
> analogue amps and whatnot.  Like I say if the device tree described
> this assembly it'd be different but it's really not doing that.
>
>> Moreover, the fact the Linux implementation uses MFD to represent
>> that is irrelevant for the DT description. We should be able to use
>> whatever SW representation for this type of HW.
>
> This is precisely my point, what we're being presented with here is a
> device tree description of the particular way that Linux wants to
> understand this stuff rather than something that lets us learn about the
> chip internals.

As I said before; there is not a single way to describe that kind of HW. 
But still this description is perfectly valid for the HW point of view. 
It just does not represent all the details inside the CODEC. But the way 
DT is done, nothing can prevent someone else to add further details 
below that node assuming there will be some SW to use that at some point.

It is up to the driver owner to assess the level of information he'd 
like to expose to DT based on the number of chip variants that already 
exist.

So far, MFD was the only way to describe some HW hierarchy in a SoC that 
only have platform_device.
Now, DT can do that as well without hard coding some sub device 
information inside a driver, thus allowing you to change that sub-device 
without changing the driver.

Both formats are valid.
It is just a matter of flexibility / personal choice / mood of the day.

The important point is that this is not a black or white kind of 
decision, we can be in the grey area and use a little bit of both approach.

Thanks,
Benoit

Re: [PATCH] ASoC: twl6040: Support for DT

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 4611 bytes --]

On Fri, May 11, 2012 at 05:44:19PM +0200, Cousson, Benoit wrote:
> On 5/11/2012 3:08 PM, Mark Brown wrote:

> >This binding doesn't do anything to move towards that goal given that
> >the only information it includes about the contents of the chip is the
> >name.

> But it does not have to expose everything. And what will be your
> definition of everything in that case?

A useful binding that abstracted things sufficiently to allow multiple
devices would be one which contained some information about the contents
of the chip.

> >Writing the name out in separate CODEC and vibra nodes really
> >isn't going to accomplish much to promote reuse that can't trivially be
> >achieved by parsing the name in the MFD driver.

> Maybe, but... so what? This is not because you can do it with MFD
> that you cannot make it more flexible with DT.

The issue is the tastefulness.  Just looking at the binding it's
immediately clear that the only thing that the extra levels of
indirection are adding is the removal of the mfd_add_cells() call from
the MFD driver which isn't particularly helping anything and is
basically Linux specific.

> Preferring to hard code that in MFD is similar to keeping all the
> static C board files we are all trying to remove. It is possible,
> sure, but there are now better way to describe HW modules than
> hard-coding that in a C file.

Sure, but this binding doesn't do anything like that.  If it said things
like "there's a PDM speaker driver with register map X at address Y"
then you'd have a block you could easily pick up and reuse when defining
another device but it doesn't do that at all.

> Moreover, there is not an unique way to describe the HW. So for sure
> we will cheat a little bit and make some assumption about what the
> SW will really use. But otherwise you will end up putting the full
> RTL inside the DT node.

This isn't a "cheat a little bit" type thing - it's really not talking
about blocks that are likely to reappear elsewhere in exactly the same
form on a device that's different enough that you'd be able to add some
useful reuse via the device tree.

With CODECs the IP blocks you see are generally things like audio
interfaces, mixers, PLLs, amplifiers and so on.  New devices that are
meaningfully different to software will generally be some new
combination of that sort of stuff rather than a straight clone of the
chip top level which is what this binding is talking about.

> It is very similar to the discussion we had for the clock tree. For
> sure we can describe the full clock tree in DT, but that's huge and
> useless. So we are taking some assumption and expose only the ones
> that have to be exposed because dependent of the board or needed by
> the devices.

The difference I'm seeing here is that the split in the device tree here
is at such that it's the equivalent of having a single device tree node
for the entire clock tree of a given OMAP model - it's just completely
redundant, it's not adding any additional information beyond the top
level information about which chip this is.

> It is up to the driver owner to assess the level of information he'd
> like to expose to DT based on the number of chip variants that
> already exist.

This code handles exactly one chip and looking at it it's *extremely*
hard to see it as a useful abstraction for multiple chips.  There's also
the fact that with things like this where the chip has to be hooked up
to other pieces of the system it makes much more of a difference than it
might otherwise since it's much more externally visible.  In this
particular case for example we might want to split the clocking out
differently as the generic clock API comes along, and there's extcon
too.  All of which should be entirely Linux internal things.

> Both formats are valid.
> It is just a matter of flexibility / personal choice / mood of the day.

> The important point is that this is not a black or white kind of
> decision, we can be in the grey area and use a little bit of both
> approach.

If I could see how this would allow reuse I'd probably agree with you
but I really can't - it's immediately obvious looking at what's there
that all that's happening is that a bit of the Linux internals that's
not particularly great as a generic abstraction dropped straight into
the device tree.

Plus there's the fact that from both a device tree and a code point of
view it's utterly trivial to not land ourselves with this in the device
tree, it's just registering two devices and using a different of_node
for reading properties in code and removing a layer of indentation in
the DT.

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

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH] ASoC: twl6040: Support for DT

[Peter Ujfalusi]

On 05/11/2012 11:34 PM, Mark Brown wrote:
> The issue is the tastefulness.  Just looking at the binding it's
> immediately clear that the only thing that the extra levels of
> indirection are adding is the removal of the mfd_add_cells() call from
> the MFD driver which isn't particularly helping anything and is
> basically Linux specific.

It is not really a coincident that we have the drivers structured in a
way they are at the moment (with or without MFD).
We have several functionality provided by this chip (twl6040). In one
chip it provides audio (twl6040-codec), vibra (twl6040-vibra) and we are
going to have few more additions as well (GPO, clock driver to provide
the McPDM functional clock).

We might see more integrated solution in a future which would combine
the current pmic and the audio in a single chip. If this happens we can
just use the dts sections describing the functionalities of the twl6040
appended to this - theoretical - chip.
The child (or drivers for the functionality) only needs small update for
compatible_of, and new chip access wrapper.
In this case the core chip driver (twl6040 MFD) will no longer
exist/make sense since the functionality provided by the twl6040 chip
has been merged together with the PMIC.

You might be right that the resulting dts section for the chip is just
represents the current MD stack, but if I want to prepare for the future
this is the way I think is the best for us.

-- 
Péter

Re: [PATCH] ASoC: twl6040: Support for DT

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 1589 bytes --]

On Mon, May 14, 2012 at 02:38:20PM +0300, Peter Ujfalusi wrote:

> The child (or drivers for the functionality) only needs small update for
> compatible_of, and new chip access wrapper.

At the very least anything that's using interrupts through the device
core will also need to know how those have been wired out into the chip
top level, and of course if anything happened to move about in the
register map or there are any changes in available functionality (which
would be unsurprising, perhaps an additional line output or something
for example) those will also need to be dealt with.

Besides, if that's all the function drivers need to look at they can
just look at their parent node instead of looking at the child node,
problem sorted.

> You might be right that the resulting dts section for the chip is just
> represents the current MD stack, but if I want to prepare for the future
> this is the way I think is the best for us.

Again, if I could see how this supported non-trivial reuse I'd be right
with you but I'm just not seeing how this would allow reuse on another
chip without rework of all existing device trees or what it hides about
the chip top level.  The code changes to drop the child nodes from the
device tree should be trivial.  This in itself seems to me like a clear
sign that the abstraction in the device tree isn't actually providing
useful generalisation of the chip.

I'm just not seeing anything in the device tree that abstracts the chip
top level from the function drivers; if there were then it'd be very
much easier to see a general use for this.

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

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH] ASoC: twl6040: Support for DT

[Peter Ujfalusi]

Hi Mark,

On 05/14/2012 03:11 PM, Mark Brown wrote:
> I'm just not seeing anything in the device tree that abstracts the chip
> top level from the function drivers; if there were then it'd be very
> much easier to see a general use for this.

While we are discussing on how to handle the twl6040 MFD deive via DT
and to whether load the child drivers via DT or to load them via
mfd_add_devices() I came up with this dts entry for the later case
(mfd_add_devices mode):

&i2c1 {
	twl6040: twl@4b {
		compatible = "ti,twl6040";
		reg = <0x4b>;

		interrupts = <0 119 4>;
		interrupt-parent = <&gic>;
		twl6040,audpwron-gpio = <&gpio4 31 0>;

		vio-supply = <&v1v8>;
		v2v1-supply = <&v2v1>;
		enable-active-high;

		/* regulators for vibra motor */
		vddvibl-supply = <&vbat>;
		vddvibr-supply = <&vbat>;

		vibra {
			/* Vibra driver, motor resistance parameters */
			ti,vibldrv-res = <8>;
			ti,vibrdrv-res = <3>;
			ti,viblmotor-res = <10>;
			ti,vibrmotor-res = <10>;
		};
	};
};

With this mode:
No need to tell that twl6040 is acts as interrupt controller since the
interrupts it is handling is only for internal functions dispatched via
a single irq line toward the CPU.

The audio (ASoC codec) child is always enabled since it is the main
function of the twl6040.

The tricky part is the vibra: it needs two regulators (to provide power
to the motors). Since the regulators can only be connected to the device
described in dt, this is what I need to do:
to get the vibra parameters (in the twl6040-vibra driver):

struct device_node *node = of_find_node_by_name(
				pdev->dev.parent->of_node, "vibra");

...
of_property_read_u32(node, "ti,vibldrv-res", &info->vibldrv_res);
...

To get the regulators needed by the vibra driver when booted with DT:

info->supplies[0].supply = "vddvibl";
info->supplies[1].supply = "vddvibr";
if (!pdata)
	ret = regulator_bulk_get(pdev->dev.parent,
		ARRAY_SIZE(info->supplies), info->supplies);
else
	ret = regulator_bulk_get(info->dev,
		ARRAY_SIZE(info->supplies), info->supplies);


I need to use the pdev->dev.parent to be able to get the needed
regulators. Is this acceptable?

-- 
Péter

Re: [PATCH] ASoC: twl6040: Support for DT

[Tony Lindgren]

* Mark Brown <broonie-yzvPICuk2AATkU/dhu1WVueM+bqZidxxQQ4Iyu8u01E@public.gmane.org> [120511 06:12]:
> On Fri, May 11, 2012 at 01:02:26PM +0200, Cousson, Benoit wrote:
> 
> > Describing sub nodes, especially for big SoC is pretty useful.
> > It is as useful as doing that for board that are sharing similar components.
> 
> The concern here is that the device tree you're writing here is clearly
> just a direct translation of the particular stuff Linux happens to use
> internally into device tree; this is similar to the thing with using
> hwmod in the device tree representation and omitting basic stuff like
> the register ranges.

I agree we should not omit the register ranges from DT. That is
hardware specific information that other operating systems possibly
need, and they don't have hwmod.

Regards,

Tony

Re: [PATCH] ASoC: twl6040: Support for DT

[Peter Ujfalusi]

On 05/14/2012 06:34 PM, Tony Lindgren wrote:
> I agree we should not omit the register ranges from DT. That is
> hardware specific information that other operating systems possibly
> need, and they don't have hwmod.

I was considering to do that for the omap-mcpdm, omap-dmic. I think the
reason I did not done it is that none of the other dt sections for OMAP
devices have this information (most probably for a reason).

-- 
Péter