Re: [PATCH] pinctrl: msm: Add support for MSM TLMM pinmux

Re: [PATCH] pinctrl: msm: Add support for MSM TLMM pinmux

[Linus Walleij]

On Wed, Jul 24, 2013 at 11:41 PM, Hanumant Singh
<hanumant@codeaurora.org> wrote:

> Add a new device tree enabled pinctrl driver for
> Qualcomm MSM SoC's. This driver provides an extensible
> framework to interface all MSM's that use a TLMM pinmux,
> with the pinctrl subsytem.
>
> This driver is split into two parts: the pinctrl interface
> and the TLMM version specific implementation. The pinctrl
> interface parses the device tree and registers with the pinctrl
> subsytem. The TLMM version specific implementation supports
> pin configuration/register programming for the different
> pin types present on a given TLMM pinmux version.
>
> Add support only for TLMM version 3 pinmux right now,
> as well as, only two of the different pin types present on the
> TLMM v3 pinmux.
> Pintype 1: General purpose pins.
> Pintype 2: SDC pins.

I guess this is the v4 patch set?

Please include a small changelog so I can keep track of
things...


> Change-Id: I065d874cd2c6fd002d5b3cb4b355445bb6912bf4

This thing is not interesting to the kernel community.

> diff --git a/Documentation/devicetree/bindings/pinctrl/msm-pinctrl.txt b/Documentation/devicetree/bindings/pinctrl/msm-pinctrl.txt
> new file mode 100644
> index 0000000..0f17a94
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pinctrl/msm-pinctrl.txt

This needs to be broken out and sent for separate review on
the devicetree@vger.kernel.org mailing list.

> +Required Properties
> +  - qcom,pin-type-*: identifies the pin type. Pin types supported are
> +                       qcom,pin-type-gp (General purpose)
> +                       qcom,pin-type-sdc (SDC)

I am wondering if it it would not be simpler to split this
driver in two, one for the GP pins and one for the SDC pins.
Having this tag on each and every pin seems *very*
awkward.

If you have two drivers and two device tree nodes, one for
GP pins and one for SDC pins, just separated by different
compatible strings and different memory regs, things will
look simpler and be easier to maintain I think.

Unless there is some strong cross-dependency between
GP and SDC please explore this approach.

> +- Pin groups as child nodes: The pin mux (selecting pin function
> +  mode) and pin config (pull up/down, driver strength, direction) settings are
> +  represented as child nodes of the pin-controller node. There is no limit on
> +  the count of these child nodes.
> +
> +  Required Properties
> +    -qcom,pins: phandle specifying pin type and a pin number.
> +    -qcom,num-grp-pins: number of pins in the group.
> +    -label: name to to identify the pin group.
> +
> +  Optional Properties
> +    -qcom,pin-func: function setting for the pin group.

After some scratching my head I realize that you are trying
to reimplement parts of pinctrl-single.c.

I.e. you try to put all the definitions of groups and functions
into the devicetree instead of having this in the driver
as tables.

If this is what you want to do you should use pinctrl-single.c.
It might be possible to use pinctrl-single.c only for the
GP pins.

But if there is something complex about the hardware that
make pinctrl-single.c not fit the bill I advice you to encode
the lists of groups and functions into the driver instead.

Also, split this in a per-soc manner (compare the other
drivers) so you can support plugging one file per SoC
for the different MSM chips using this pin controller.

Yours,
Linus Walleij

Re: [PATCH] pinctrl: msm: Add support for MSM TLMM pinmux

[Stephen Warren]

On 07/29/2013 10:37 AM, Linus Walleij wrote:
> On Wed, Jul 24, 2013 at 11:41 PM, Hanumant Singh
> <hanumant@codeaurora.org> wrote:
> 
>> Add a new device tree enabled pinctrl driver for
>> Qualcomm MSM SoC's. This driver provides an extensible
>> framework to interface all MSM's that use a TLMM pinmux,
>> with the pinctrl subsytem.
>>
>> This driver is split into two parts: the pinctrl interface
>> and the TLMM version specific implementation. The pinctrl
>> interface parses the device tree and registers with the pinctrl
>> subsytem. The TLMM version specific implementation supports
>> pin configuration/register programming for the different
>> pin types present on a given TLMM pinmux version.
>>
>> Add support only for TLMM version 3 pinmux right now,
>> as well as, only two of the different pin types present on the
>> TLMM v3 pinmux.
>> Pintype 1: General purpose pins.
>> Pintype 2: SDC pins.
> 
> I guess this is the v4 patch set?
> 
> Please include a small changelog so I can keep track of
> things...
> 
> 
>> Change-Id: I065d874cd2c6fd002d5b3cb4b355445bb6912bf4
> 
> This thing is not interesting to the kernel community.
> 
>> diff --git a/Documentation/devicetree/bindings/pinctrl/msm-pinctrl.txt b/Documentation/devicetree/bindings/pinctrl/msm-pinctrl.txt
>> new file mode 100644
>> index 0000000..0f17a94
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/pinctrl/msm-pinctrl.txt
> 
> This needs to be broken out and sent for separate review on
> the devicetree@vger.kernel.org mailing list.

It certainly should be reviewed there, although I don't think the
binding .txt file needs to be a separate patch. At present, it's quite
typical to include the binding doc with the first driver that implements
it, although that may change in the future; we'll see.

Re: [PATCH] pinctrl: msm: Add support for MSM TLMM pinmux

[hanumant]

On 7/29/2013 9:37 AM, Linus Walleij wrote:
> On Wed, Jul 24, 2013 at 11:41 PM, Hanumant Singh
> <hanumant@codeaurora.org> wrote:
>
>> Add a new device tree enabled pinctrl driver for
>> Qualcomm MSM SoC's. This driver provides an extensible
>> framework to interface all MSM's that use a TLMM pinmux,
>> with the pinctrl subsytem.
>>
>> This driver is split into two parts: the pinctrl interface
>> and the TLMM version specific implementation. The pinctrl
>> interface parses the device tree and registers with the pinctrl
>> subsytem. The TLMM version specific implementation supports
>> pin configuration/register programming for the different
>> pin types present on a given TLMM pinmux version.
>>
>> Add support only for TLMM version 3 pinmux right now,
>> as well as, only two of the different pin types present on the
>> TLMM v3 pinmux.
>> Pintype 1: General purpose pins.
>> Pintype 2: SDC pins.
>
> I guess this is the v4 patch set?
>
> Please include a small changelog so I can keep track of
> things...
>
>
>> Change-Id: I065d874cd2c6fd002d5b3cb4b355445bb6912bf4
>
> This thing is not interesting to the kernel community.
>
>> diff --git a/Documentation/devicetree/bindings/pinctrl/msm-pinctrl.txt b/Documentation/devicetree/bindings/pinctrl/msm-pinctrl.txt
>> new file mode 100644
>> index 0000000..0f17a94
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/pinctrl/msm-pinctrl.txt
>
> This needs to be broken out and sent for separate review on
> the devicetree@vger.kernel.org mailing list.
>
>> +Required Properties
>> +  - qcom,pin-type-*: identifies the pin type. Pin types supported are
>> +                       qcom,pin-type-gp (General purpose)
>> +                       qcom,pin-type-sdc (SDC)
>
> I am wondering if it it would not be simpler to split this
> driver in two, one for the GP pins and one for the SDC pins.
> Having this tag on each and every pin seems *very*
> awkward.
>
> If you have two drivers and two device tree nodes, one for
> GP pins and one for SDC pins, just separated by different
> compatible strings and different memory regs, things will
> look simpler and be easier to maintain I think.
>
> Unless there is some strong cross-dependency between
> GP and SDC please explore this approach.

Actually we have had this discussion.
There are more pin types on a TLMM then just these 2.
The registers are interleaved.
They are no separated at a clean page boundary.
Also there is mode register that governs the function mode of the 
"unifunction"/single function pins like SDC. This register is again 
interspersed in the address map.
Having it the way it is right now, will help me add clean support for
other pintypes on a given TLMM version.

>
>> +- Pin groups as child nodes: The pin mux (selecting pin function
>> +  mode) and pin config (pull up/down, driver strength, direction) settings are
>> +  represented as child nodes of the pin-controller node. There is no limit on
>> +  the count of these child nodes.
>> +
>> +  Required Properties
>> +    -qcom,pins: phandle specifying pin type and a pin number.
>> +    -qcom,num-grp-pins: number of pins in the group.
>> +    -label: name to to identify the pin group.
>> +
>> +  Optional Properties
>> +    -qcom,pin-func: function setting for the pin group.
>
> After some scratching my head I realize that you are trying
> to reimplement parts of pinctrl-single.c.
>
> I.e. you try to put all the definitions of groups and functions
> into the devicetree instead of having this in the driver
> as tables.
>
> If this is what you want to do you should use pinctrl-single.c.
> It might be possible to use pinctrl-single.c only for the
> GP pins.
I would prefer not to split the driver into one driver for its own 
pintype for above reasons.
Also there isn't just one register per pin for gp pins, there are more 
registers.
For example to configure direction, I have to touch additional registers
then just the config register.
Also for future TLMM version, we are very likely to move away from 
single config register.
>
> But if there is something complex about the hardware that
> make pinctrl-single.c not fit the bill I advice you to encode
> the lists of groups and functions into the driver instead.
>
> Also, split this in a per-soc manner (compare the other
> drivers) so you can support plugging one file per SoC
> for the different MSM chips using this pin controller.
>
We actually have the same TLMM pinmux used by several socs of a family.
The number of pins on each soc may vary.
Also a given soc gets used in a number of boards.
The device tree for a given soc is split into the different boards that 
its in ie the boards inherit a common soc.dtsi but have separate dts.
The boards for the same soc may use different pin groups for 
accomplishing a function, since we have multiple i2c, spi uart etc
peripheral instances on a soc. A different instance of each of the above 
peripherals, can be used in different boards, utilizing different
or subset of same pin groups.
Thus I would need to have multiple C files for one soc, based on the 
boards that it goes into.

Thanks
Hanumant


-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
-- 

Re: [PATCH] pinctrl: msm: Add support for MSM TLMM pinmux

[Stephen Warren]

On 07/30/2013 03:10 PM, hanumant wrote:
...
> We actually have the same TLMM pinmux used by several socs of a family.
> The number of pins on each soc may vary.
> Also a given soc gets used in a number of boards.
> The device tree for a given soc is split into the different boards that
> its in ie the boards inherit a common soc.dtsi but have separate dts.
> The boards for the same soc may use different pin groups for
> accomplishing a function, since we have multiple i2c, spi uart etc
> peripheral instances on a soc. A different instance of each of the above
> peripherals, can be used in different boards, utilizing different
> or subset of same pin groups.
> Thus I would need to have multiple C files for one soc, based on the
> boards that it goes into.

The pinctrl driver should be exposing the raw capabilities of the HW.
All the board-specific configuration should be expressed in DT. So, the
driver shouldn't have to know anything about different boards at
compile-time.

Re: [PATCH] pinctrl: msm: Add support for MSM TLMM pinmux

[Hanumant Singh]

On 7/30/2013 2:22 PM, Stephen Warren wrote:
> On 07/30/2013 03:10 PM, hanumant wrote:
> ...
>> We actually have the same TLMM pinmux used by several socs of a family.
>> The number of pins on each soc may vary.
>> Also a given soc gets used in a number of boards.
>> The device tree for a given soc is split into the different boards that
>> its in ie the boards inherit a common soc.dtsi but have separate dts.
>> The boards for the same soc may use different pin groups for
>> accomplishing a function, since we have multiple i2c, spi uart etc
>> peripheral instances on a soc. A different instance of each of the above
>> peripherals, can be used in different boards, utilizing different
>> or subset of same pin groups.
>> Thus I would need to have multiple C files for one soc, based on the
>> boards that it goes into.
>
> The pinctrl driver should be exposing the raw capabilities of the HW.
> All the board-specific configuration should be expressed in DT. So, the
> driver shouldn't have to know anything about different boards at
> compile-time.
>
I agree, so I wanted to keep the pin grouping information in DT, we 
already have a board based differentiation of dts files in DT, for the 
same soc.

Thanks
Hanumant

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
-- 

Re: [PATCH] pinctrl: msm: Add support for MSM TLMM pinmux

[Stephen Warren]

On 07/30/2013 06:01 PM, Hanumant Singh wrote:
> On 7/30/2013 2:22 PM, Stephen Warren wrote:
>> On 07/30/2013 03:10 PM, hanumant wrote:
>> ...
>>> We actually have the same TLMM pinmux used by several socs of a family.
>>> The number of pins on each soc may vary.
>>> Also a given soc gets used in a number of boards.
>>> The device tree for a given soc is split into the different boards that
>>> its in ie the boards inherit a common soc.dtsi but have separate dts.
>>> The boards for the same soc may use different pin groups for
>>> accomplishing a function, since we have multiple i2c, spi uart etc
>>> peripheral instances on a soc. A different instance of each of the above
>>> peripherals, can be used in different boards, utilizing different
>>> or subset of same pin groups.
>>> Thus I would need to have multiple C files for one soc, based on the
>>> boards that it goes into.
>>
>> The pinctrl driver should be exposing the raw capabilities of the HW.
>> All the board-specific configuration should be expressed in DT. So, the
>> driver shouldn't have to know anything about different boards at
>> compile-time.
>>
> I agree, so I wanted to keep the pin grouping information in DT, we
> already have a board based differentiation of dts files in DT, for the
> same soc.

That's the opposite of what I was saying. Pin groups are a feature of
the SoC design, not the board.

Re: [PATCH] pinctrl: msm: Add support for MSM TLMM pinmux

[Hanumant Singh]

On 7/30/2013 5:08 PM, Stephen Warren wrote:
> On 07/30/2013 06:01 PM, Hanumant Singh wrote:
>> On 7/30/2013 2:22 PM, Stephen Warren wrote:
>>> On 07/30/2013 03:10 PM, hanumant wrote:
>>> ...
>>>> We actually have the same TLMM pinmux used by several socs of a family.
>>>> The number of pins on each soc may vary.
>>>> Also a given soc gets used in a number of boards.
>>>> The device tree for a given soc is split into the different boards that
>>>> its in ie the boards inherit a common soc.dtsi but have separate dts.
>>>> The boards for the same soc may use different pin groups for
>>>> accomplishing a function, since we have multiple i2c, spi uart etc
>>>> peripheral instances on a soc. A different instance of each of the above
>>>> peripherals, can be used in different boards, utilizing different
>>>> or subset of same pin groups.
>>>> Thus I would need to have multiple C files for one soc, based on the
>>>> boards that it goes into.
>>>
>>> The pinctrl driver should be exposing the raw capabilities of the HW.
>>> All the board-specific configuration should be expressed in DT. So, the
>>> driver shouldn't have to know anything about different boards at
>>> compile-time.
>>>
>> I agree, so I wanted to keep the pin grouping information in DT, we
>> already have a board based differentiation of dts files in DT, for the
>> same soc.
>
> That's the opposite of what I was saying. Pin groups are a feature of
> the SoC design, not the board.
>
Sorry I guess I wasn't clear.
Right now I have a soc-pinctrl.dtsi containing pin groupings.
This will be "inherited" by soc-boardtype.dts.
The pinctrl client device nodes in soc-boardtype.dts will point to pin 
groupings in soc-pinctrl.dtsi that are valid for that particular boardtype.
Is this a valid design?

Thanks
Hanumant
that are valid for that


-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
-- 

Re: [PATCH] pinctrl: msm: Add support for MSM TLMM pinmux

[Stephen Warren]

On 07/30/2013 06:13 PM, Hanumant Singh wrote:
> On 7/30/2013 5:08 PM, Stephen Warren wrote:
>> On 07/30/2013 06:01 PM, Hanumant Singh wrote:
>>> On 7/30/2013 2:22 PM, Stephen Warren wrote:
>>>> On 07/30/2013 03:10 PM, hanumant wrote:
>>>> ...
>>>>> We actually have the same TLMM pinmux used by several socs of a
>>>>> family.
>>>>> The number of pins on each soc may vary.
>>>>> Also a given soc gets used in a number of boards.
>>>>> The device tree for a given soc is split into the different boards
>>>>> that
>>>>> its in ie the boards inherit a common soc.dtsi but have separate dts.
>>>>> The boards for the same soc may use different pin groups for
>>>>> accomplishing a function, since we have multiple i2c, spi uart etc
>>>>> peripheral instances on a soc. A different instance of each of the
>>>>> above
>>>>> peripherals, can be used in different boards, utilizing different
>>>>> or subset of same pin groups.
>>>>> Thus I would need to have multiple C files for one soc, based on the
>>>>> boards that it goes into.
>>>>
>>>> The pinctrl driver should be exposing the raw capabilities of the HW.
>>>> All the board-specific configuration should be expressed in DT. So, the
>>>> driver shouldn't have to know anything about different boards at
>>>> compile-time.
>>>>
>>> I agree, so I wanted to keep the pin grouping information in DT, we
>>> already have a board based differentiation of dts files in DT, for the
>>> same soc.
>>
>> That's the opposite of what I was saying. Pin groups are a feature of
>> the SoC design, not the board.
>>
> Sorry I guess I wasn't clear.
> Right now I have a soc-pinctrl.dtsi containing pin groupings.
> This will be "inherited" by soc-boardtype.dts.
> The pinctrl client device nodes in soc-boardtype.dts will point to pin
> groupings in soc-pinctrl.dtsi that are valid for that particular boardtype.
> Is this a valid design?

OK, so you have two types of child node inside the pinctrl DT node; some
define the pin groups the SoC has (in soc.dtsi) and some define pinctrl
states that reference the pin group nodes and are referenced by the
client nodes.

That's probably fine. However, I'd still question putting the pin group
nodes in DT at all; I'm not convinced it's better than just putting
those into the driver itself. You end up with the same data tables after
parsing the DT anyway.

Re: [PATCH] pinctrl: msm: Add support for MSM TLMM pinmux

[Hanumant Singh]

On 7/30/2013 8:59 PM, Stephen Warren wrote:
> On 07/30/2013 06:13 PM, Hanumant Singh wrote:
>> On 7/30/2013 5:08 PM, Stephen Warren wrote:
>>> On 07/30/2013 06:01 PM, Hanumant Singh wrote:
>>>> On 7/30/2013 2:22 PM, Stephen Warren wrote:
>>>>> On 07/30/2013 03:10 PM, hanumant wrote:
>>>>> ...
>>>>>> We actually have the same TLMM pinmux used by several socs of a
>>>>>> family.
>>>>>> The number of pins on each soc may vary.
>>>>>> Also a given soc gets used in a number of boards.
>>>>>> The device tree for a given soc is split into the different boards
>>>>>> that
>>>>>> its in ie the boards inherit a common soc.dtsi but have separate dts.
>>>>>> The boards for the same soc may use different pin groups for
>>>>>> accomplishing a function, since we have multiple i2c, spi uart etc
>>>>>> peripheral instances on a soc. A different instance of each of the
>>>>>> above
>>>>>> peripherals, can be used in different boards, utilizing different
>>>>>> or subset of same pin groups.
>>>>>> Thus I would need to have multiple C files for one soc, based on the
>>>>>> boards that it goes into.
>>>>>
>>>>> The pinctrl driver should be exposing the raw capabilities of the HW.
>>>>> All the board-specific configuration should be expressed in DT. So, the
>>>>> driver shouldn't have to know anything about different boards at
>>>>> compile-time.
>>>>>
>>>> I agree, so I wanted to keep the pin grouping information in DT, we
>>>> already have a board based differentiation of dts files in DT, for the
>>>> same soc.
>>>
>>> That's the opposite of what I was saying. Pin groups are a feature of
>>> the SoC design, not the board.
>>>
>> Sorry I guess I wasn't clear.
>> Right now I have a soc-pinctrl.dtsi containing pin groupings.
>> This will be "inherited" by soc-boardtype.dts.
>> The pinctrl client device nodes in soc-boardtype.dts will point to pin
>> groupings in soc-pinctrl.dtsi that are valid for that particular boardtype.
>> Is this a valid design?
>
> OK, so you have two types of child node inside the pinctrl DT node; some
> define the pin groups the SoC has (in soc.dtsi) and some define pinctrl
> states that reference the pin group nodes and are referenced by the
> client nodes.
>
> That's probably fine. However, I'd still question putting the pin group
> nodes in DT at all; I'm not convinced it's better than just putting
> those into the driver itself. You end up with the same data tables after
> parsing the DT anyway.
>

Any feedback for the rest of the patch?

Thanks
Hanumant

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
-- 

Re: [PATCH] pinctrl: msm: Add support for MSM TLMM pinmux

[Stephen Warren]

On 07/31/2013 01:46 PM, Hanumant Singh wrote:
> On 7/30/2013 8:59 PM, Stephen Warren wrote:
>> On 07/30/2013 06:13 PM, Hanumant Singh wrote:
>>> On 7/30/2013 5:08 PM, Stephen Warren wrote:
>>>> On 07/30/2013 06:01 PM, Hanumant Singh wrote:
>>>>> On 7/30/2013 2:22 PM, Stephen Warren wrote:
>>>>>> On 07/30/2013 03:10 PM, hanumant wrote:
>>>>>> ...
>>>>>>> We actually have the same TLMM pinmux used by several socs of a
>>>>>>> family.
>>>>>>> The number of pins on each soc may vary.
>>>>>>> Also a given soc gets used in a number of boards.
>>>>>>> The device tree for a given soc is split into the different boards
>>>>>>> that
>>>>>>> its in ie the boards inherit a common soc.dtsi but have separate
>>>>>>> dts.
>>>>>>> The boards for the same soc may use different pin groups for
>>>>>>> accomplishing a function, since we have multiple i2c, spi uart etc
>>>>>>> peripheral instances on a soc. A different instance of each of the
>>>>>>> above
>>>>>>> peripherals, can be used in different boards, utilizing different
>>>>>>> or subset of same pin groups.
>>>>>>> Thus I would need to have multiple C files for one soc, based on the
>>>>>>> boards that it goes into.
>>>>>>
>>>>>> The pinctrl driver should be exposing the raw capabilities of the HW.
>>>>>> All the board-specific configuration should be expressed in DT.
>>>>>> So, the
>>>>>> driver shouldn't have to know anything about different boards at
>>>>>> compile-time.
>>>>>>
>>>>> I agree, so I wanted to keep the pin grouping information in DT, we
>>>>> already have a board based differentiation of dts files in DT, for the
>>>>> same soc.
>>>>
>>>> That's the opposite of what I was saying. Pin groups are a feature of
>>>> the SoC design, not the board.
>>>>
>>> Sorry I guess I wasn't clear.
>>> Right now I have a soc-pinctrl.dtsi containing pin groupings.
>>> This will be "inherited" by soc-boardtype.dts.
>>> The pinctrl client device nodes in soc-boardtype.dts will point to pin
>>> groupings in soc-pinctrl.dtsi that are valid for that particular
>>> boardtype.
>>> Is this a valid design?
>>
>> OK, so you have two types of child node inside the pinctrl DT node; some
>> define the pin groups the SoC has (in soc.dtsi) and some define pinctrl
>> states that reference the pin group nodes and are referenced by the
>> client nodes.
>>
>> That's probably fine. However, I'd still question putting the pin group
>> nodes in DT at all; I'm not convinced it's better than just putting
>> those into the driver itself. You end up with the same data tables after
>> parsing the DT anyway.
>>
> 
> Any feedback for the rest of the patch?

I'm certainly waiting for this aspect of the patch to be resolved; I
think it will impact the rest of the patch so much that it's not worth
reviewing until we decide on where to represent the pin groups (some DT
parsing could would be removed if we put the pin group definitions into
the driver, hence wouldn't need to be reviewed, and likewise there's be
some new tables to review).

Re: [PATCH] pinctrl: msm: Add support for MSM TLMM pinmux

[Hanumant Singh]

On 7/31/2013 2:06 PM, Stephen Warren wrote:
> On 07/31/2013 01:46 PM, Hanumant Singh wrote:
>> On 7/30/2013 8:59 PM, Stephen Warren wrote:
>>> On 07/30/2013 06:13 PM, Hanumant Singh wrote:
>>>> On 7/30/2013 5:08 PM, Stephen Warren wrote:
>>>>> On 07/30/2013 06:01 PM, Hanumant Singh wrote:
>>>>>> On 7/30/2013 2:22 PM, Stephen Warren wrote:
>>>>>>> On 07/30/2013 03:10 PM, hanumant wrote:
>>>>>>> ...
>>>>>>>> We actually have the same TLMM pinmux used by several socs of a
>>>>>>>> family.
>>>>>>>> The number of pins on each soc may vary.
>>>>>>>> Also a given soc gets used in a number of boards.
>>>>>>>> The device tree for a given soc is split into the different boards
>>>>>>>> that
>>>>>>>> its in ie the boards inherit a common soc.dtsi but have separate
>>>>>>>> dts.
>>>>>>>> The boards for the same soc may use different pin groups for
>>>>>>>> accomplishing a function, since we have multiple i2c, spi uart etc
>>>>>>>> peripheral instances on a soc. A different instance of each of the
>>>>>>>> above
>>>>>>>> peripherals, can be used in different boards, utilizing different
>>>>>>>> or subset of same pin groups.
>>>>>>>> Thus I would need to have multiple C files for one soc, based on the
>>>>>>>> boards that it goes into.
>>>>>>>
>>>>>>> The pinctrl driver should be exposing the raw capabilities of the HW.
>>>>>>> All the board-specific configuration should be expressed in DT.
>>>>>>> So, the
>>>>>>> driver shouldn't have to know anything about different boards at
>>>>>>> compile-time.
>>>>>>>
>>>>>> I agree, so I wanted to keep the pin grouping information in DT, we
>>>>>> already have a board based differentiation of dts files in DT, for the
>>>>>> same soc.
>>>>>
>>>>> That's the opposite of what I was saying. Pin groups are a feature of
>>>>> the SoC design, not the board.
>>>>>
>>>> Sorry I guess I wasn't clear.
>>>> Right now I have a soc-pinctrl.dtsi containing pin groupings.
>>>> This will be "inherited" by soc-boardtype.dts.
>>>> The pinctrl client device nodes in soc-boardtype.dts will point to pin
>>>> groupings in soc-pinctrl.dtsi that are valid for that particular
>>>> boardtype.
>>>> Is this a valid design?
>>>
>>> OK, so you have two types of child node inside the pinctrl DT node; some
>>> define the pin groups the SoC has (in soc.dtsi) and some define pinctrl
>>> states that reference the pin group nodes and are referenced by the
>>> client nodes.
>>>
>>> That's probably fine. However, I'd still question putting the pin group
>>> nodes in DT at all; I'm not convinced it's better than just putting
>>> those into the driver itself. You end up with the same data tables after
>>> parsing the DT anyway.
>>>
>>
>> Any feedback for the rest of the patch?
>
> I'm certainly waiting for this aspect of the patch to be resolved; I
> think it will impact the rest of the patch so much that it's not worth
> reviewing until we decide on where to represent the pin groups (some DT
> parsing could would be removed if we put the pin group definitions into
> the driver, hence wouldn't need to be reviewed, and likewise there's be
> some new tables to review).
>

I am trying to look at examples of what you are suggesting.
I was looking at the exynos implementation, and just from a brief glance 
it seems like there too the pin grouping is being specified in the 
device tree, using what looks like labels of the pins.
The labels are matched to group structures in soc specific files?

By having the pin groupings in DT I am able to reuse the driver without 
any SOC based code bloat.
As I mentioned earlier, we have entire families of SOCs using the same
TLMM hardware.
Its not a guarantee that for a given TLMM version,
the pin groupings on that hardware are the same for every SOC that its 
in. Its infact most likely that I wont be able to use the pin groupings
from one SOC to the next even if they both use the same TLMM.
It will very quickly lead to a bloat of
pinctrl-<msm_soc>.c (containing the pin groupings replicated for each soc)
which use TLMM version specific register programming implementation
pinctrl-tlmm-<version>.c
and the DT parsing and interface to framework (which remains unchanged).
pinctrl-msm.c.

Thanks
Hanumant

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
-- 

Re: [PATCH] pinctrl: msm: Add support for MSM TLMM pinmux

[Hanumant Singh]

On 7/31/2013 5:17 PM, Hanumant Singh wrote:
> On 7/31/2013 2:06 PM, Stephen Warren wrote:
>> On 07/31/2013 01:46 PM, Hanumant Singh wrote:
>>> On 7/30/2013 8:59 PM, Stephen Warren wrote:
>>>> On 07/30/2013 06:13 PM, Hanumant Singh wrote:
>>>>> On 7/30/2013 5:08 PM, Stephen Warren wrote:
>>>>>> On 07/30/2013 06:01 PM, Hanumant Singh wrote:
>>>>>>> On 7/30/2013 2:22 PM, Stephen Warren wrote:
>>>>>>>> On 07/30/2013 03:10 PM, hanumant wrote:
>>>>>>>> ...
>>>>>>>>> We actually have the same TLMM pinmux used by several socs of a
>>>>>>>>> family.
>>>>>>>>> The number of pins on each soc may vary.
>>>>>>>>> Also a given soc gets used in a number of boards.
>>>>>>>>> The device tree for a given soc is split into the different boards
>>>>>>>>> that
>>>>>>>>> its in ie the boards inherit a common soc.dtsi but have separate
>>>>>>>>> dts.
>>>>>>>>> The boards for the same soc may use different pin groups for
>>>>>>>>> accomplishing a function, since we have multiple i2c, spi uart etc
>>>>>>>>> peripheral instances on a soc. A different instance of each of the
>>>>>>>>> above
>>>>>>>>> peripherals, can be used in different boards, utilizing different
>>>>>>>>> or subset of same pin groups.
>>>>>>>>> Thus I would need to have multiple C files for one soc, based
>>>>>>>>> on the
>>>>>>>>> boards that it goes into.
>>>>>>>>
>>>>>>>> The pinctrl driver should be exposing the raw capabilities of
>>>>>>>> the HW.
>>>>>>>> All the board-specific configuration should be expressed in DT.
>>>>>>>> So, the
>>>>>>>> driver shouldn't have to know anything about different boards at
>>>>>>>> compile-time.
>>>>>>>>
>>>>>>> I agree, so I wanted to keep the pin grouping information in DT, we
>>>>>>> already have a board based differentiation of dts files in DT,
>>>>>>> for the
>>>>>>> same soc.
>>>>>>
>>>>>> That's the opposite of what I was saying. Pin groups are a feature of
>>>>>> the SoC design, not the board.
>>>>>>
>>>>> Sorry I guess I wasn't clear.
>>>>> Right now I have a soc-pinctrl.dtsi containing pin groupings.
>>>>> This will be "inherited" by soc-boardtype.dts.
>>>>> The pinctrl client device nodes in soc-boardtype.dts will point to pin
>>>>> groupings in soc-pinctrl.dtsi that are valid for that particular
>>>>> boardtype.
>>>>> Is this a valid design?
>>>>
>>>> OK, so you have two types of child node inside the pinctrl DT node;
>>>> some
>>>> define the pin groups the SoC has (in soc.dtsi) and some define pinctrl
>>>> states that reference the pin group nodes and are referenced by the
>>>> client nodes.
>>>>
>>>> That's probably fine. However, I'd still question putting the pin group
>>>> nodes in DT at all; I'm not convinced it's better than just putting
>>>> those into the driver itself. You end up with the same data tables
>>>> after
>>>> parsing the DT anyway.
>>>>
>>>
>>> Any feedback for the rest of the patch?
>>
>> I'm certainly waiting for this aspect of the patch to be resolved; I
>> think it will impact the rest of the patch so much that it's not worth
>> reviewing until we decide on where to represent the pin groups (some DT
>> parsing could would be removed if we put the pin group definitions into
>> the driver, hence wouldn't need to be reviewed, and likewise there's be
>> some new tables to review).
>>
>
> I am trying to look at examples of what you are suggesting.
> I was looking at the exynos implementation, and just from a brief glance
> it seems like there too the pin grouping is being specified in the
> device tree, using what looks like labels of the pins.
> The labels are matched to group structures in soc specific files?
>
> By having the pin groupings in DT I am able to reuse the driver without
> any SOC based code bloat.
> As I mentioned earlier, we have entire families of SOCs using the same
> TLMM hardware.
> Its not a guarantee that for a given TLMM version,
> the pin groupings on that hardware are the same for every SOC that its
> in. Its infact most likely that I wont be able to use the pin groupings
> from one SOC to the next even if they both use the same TLMM.
> It will very quickly lead to a bloat of
> pinctrl-<msm_soc>.c (containing the pin groupings replicated for each soc)
> which use TLMM version specific register programming implementation
> pinctrl-tlmm-<version>.c
> and the DT parsing and interface to framework (which remains unchanged).
> pinctrl-msm.c.
>
> Thanks
> Hanumant
>

Any comments on this?

Thanks
Hanumant


-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
-- 

Re: [PATCH] pinctrl: msm: Add support for MSM TLMM pinmux

[Stephen Warren]

On 08/06/2013 05:45 PM, Hanumant Singh wrote:
> On 7/31/2013 5:17 PM, Hanumant Singh wrote:
>> On 7/31/2013 2:06 PM, Stephen Warren wrote:
>>> On 07/31/2013 01:46 PM, Hanumant Singh wrote:
>>>> On 7/30/2013 8:59 PM, Stephen Warren wrote:
>>>>> On 07/30/2013 06:13 PM, Hanumant Singh wrote:
>>>>>> On 7/30/2013 5:08 PM, Stephen Warren wrote:
>>>>>>> On 07/30/2013 06:01 PM, Hanumant Singh wrote:
>>>>>>>> On 7/30/2013 2:22 PM, Stephen Warren wrote:
>>>>>>>>> On 07/30/2013 03:10 PM, hanumant wrote:
>>>>>>>>> ...
>>>>>>>>>> We actually have the same TLMM pinmux used by several socs of a
>>>>>>>>>> family.
>>>>>>>>>> The number of pins on each soc may vary.
>>>>>>>>>> Also a given soc gets used in a number of boards.
>>>>>>>>>> The device tree for a given soc is split into the different
>>>>>>>>>> boards
>>>>>>>>>> that
>>>>>>>>>> its in ie the boards inherit a common soc.dtsi but have separate
>>>>>>>>>> dts.
>>>>>>>>>> The boards for the same soc may use different pin groups for
>>>>>>>>>> accomplishing a function, since we have multiple i2c, spi uart
>>>>>>>>>> etc
>>>>>>>>>> peripheral instances on a soc. A different instance of each of
>>>>>>>>>> the
>>>>>>>>>> above
>>>>>>>>>> peripherals, can be used in different boards, utilizing different
>>>>>>>>>> or subset of same pin groups.
>>>>>>>>>> Thus I would need to have multiple C files for one soc, based
>>>>>>>>>> on the
>>>>>>>>>> boards that it goes into.
>>>>>>>>>
>>>>>>>>> The pinctrl driver should be exposing the raw capabilities of
>>>>>>>>> the HW.
>>>>>>>>> All the board-specific configuration should be expressed in DT.
>>>>>>>>> So, the
>>>>>>>>> driver shouldn't have to know anything about different boards at
>>>>>>>>> compile-time.
>>>>>>>>>
>>>>>>>> I agree, so I wanted to keep the pin grouping information in DT, we
>>>>>>>> already have a board based differentiation of dts files in DT,
>>>>>>>> for the
>>>>>>>> same soc.
>>>>>>>
>>>>>>> That's the opposite of what I was saying. Pin groups are a
>>>>>>> feature of
>>>>>>> the SoC design, not the board.
>>>>>>>
>>>>>> Sorry I guess I wasn't clear.
>>>>>> Right now I have a soc-pinctrl.dtsi containing pin groupings.
>>>>>> This will be "inherited" by soc-boardtype.dts.
>>>>>> The pinctrl client device nodes in soc-boardtype.dts will point to
>>>>>> pin
>>>>>> groupings in soc-pinctrl.dtsi that are valid for that particular
>>>>>> boardtype.
>>>>>> Is this a valid design?
>>>>>
>>>>> OK, so you have two types of child node inside the pinctrl DT node;
>>>>> some
>>>>> define the pin groups the SoC has (in soc.dtsi) and some define
>>>>> pinctrl
>>>>> states that reference the pin group nodes and are referenced by the
>>>>> client nodes.
>>>>>
>>>>> That's probably fine. However, I'd still question putting the pin
>>>>> group
>>>>> nodes in DT at all; I'm not convinced it's better than just putting
>>>>> those into the driver itself. You end up with the same data tables
>>>>> after
>>>>> parsing the DT anyway.
>>>>>
>>>>
>>>> Any feedback for the rest of the patch?
>>>
>>> I'm certainly waiting for this aspect of the patch to be resolved; I
>>> think it will impact the rest of the patch so much that it's not worth
>>> reviewing until we decide on where to represent the pin groups (some DT
>>> parsing could would be removed if we put the pin group definitions into
>>> the driver, hence wouldn't need to be reviewed, and likewise there's be
>>> some new tables to review).
>>>
>>
>> I am trying to look at examples of what you are suggesting.
>> I was looking at the exynos implementation, and just from a brief glance
>> it seems like there too the pin grouping is being specified in the
>> device tree, using what looks like labels of the pins.
>> The labels are matched to group structures in soc specific files?
>>
>> By having the pin groupings in DT I am able to reuse the driver without
>> any SOC based code bloat.
>> As I mentioned earlier, we have entire families of SOCs using the same
>> TLMM hardware.
>> Its not a guarantee that for a given TLMM version,
>> the pin groupings on that hardware are the same for every SOC that its
>> in. Its infact most likely that I wont be able to use the pin groupings
>> from one SOC to the next even if they both use the same TLMM.
>> It will very quickly lead to a bloat of
>> pinctrl-<msm_soc>.c (containing the pin groupings replicated for each
>> soc)
>> which use TLMM version specific register programming implementation
>> pinctrl-tlmm-<version>.c
>> and the DT parsing and interface to framework (which remains unchanged).
>> pinctrl-msm.c.
>>
>> Thanks
>> Hanumant
>>
> 
> Any comments on this?

No. As I said, I personally want to see all the pingroups defined in the
pinctrl driver. But, if someone else acks/... the patches without it, I
probably won't nack it.

Re: [PATCH] pinctrl: msm: Add support for MSM TLMM pinmux

[Linus Walleij]

On Wed, Aug 7, 2013 at 6:00 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 08/06/2013 05:45 PM, Hanumant Singh wrote:

>> Any comments on this?
>
> No. As I said, I personally want to see all the pingroups defined in the
> pinctrl driver. But, if someone else acks/... the patches without it, I
> probably won't nack it.

I agree with Stephen that I want to see the groups defined in a
(per-SoC) in-kernel driver, not as DT data.

Yours,
Linus Walleij