DT include files (was: [GIT PULL 2/2] ARM: imx: device tree changes for 3.14)

DT include files (was: [GIT PULL 2/2] ARM: imx: device tree changes for 3.14)

[Olof Johansson]

Hi,

On Tue, Dec 31, 2013 at 01:44:29PM +0800, Shawn Guo wrote:
> This pull-request has the following two dependencies:
> 
>  - The first pull-request, i.e. [GIT PULL 1/2] ARM: imx: soc changes for 3.14
> 
>  - The pinctrl 'devel' branch below.
> 
>      git://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-pinctrl.git devel
> 
>    Linus Walleij promised that the branch will be stable at least from
>    the point I pulled into my tree, that is commit 31d610f (pinctrl:
>    imx1-core populate subdevices).
> 
> Please pull, thanks.
> 
> Shawn
> 
>  .../devicetree/bindings/vendor-prefixes.txt        |    3 +
>  arch/arm/boot/dts/imx25-pinfunc.h                  |  494 +++++++++++
>  arch/arm/boot/dts/imx25-pingrp.h                   |   81 ++
>  arch/arm/boot/dts/imx27-pinfunc.h                  |  526 +++++++++++
>  arch/arm/boot/dts/imx27-pingrp.h                   |  151 ++++
>  arch/arm/boot/dts/imx35-pingrp.h                   |  104 +++
>  arch/arm/boot/dts/imx50-pinfunc.h                  |  923 ++++++++++++++++++++
>  arch/arm/boot/dts/imx50-pingrp.h                   |  146 ++++
>  arch/arm/boot/dts/imx51-pingrp.h                   |  249 ++++++
>  arch/arm/boot/dts/imx53-pingrp.h                   |  352 ++++++++
>  arch/arm/boot/dts/imx6dl-pinfunc.h                 |    2 +
>  arch/arm/boot/dts/imx6q-pinfunc.h                  |    2 +
>  arch/arm/boot/dts/imx6qdl-pingrp.h                 |  532 +++++++++++
>  arch/arm/boot/dts/imx6sl-pingrp.h                  |  148 ++++
>  arch/arm/boot/dts/vf610-pingrp.h                   |  127 +++

Hm, these don't quite use include files the way include files were
originally meant to be used -- initially the idea was to use them to
define mostly simple constants instead of full properties like this.

I'm not against the idea of using it this way, but I also want to make sure the
DT maintainers are OK with it. So I've cc:d them on this reply.

I'm also not crazy about the insanely long identifiers used here, but I guess
they correlate with some user manual tables?


-Olof
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: DT include files (was: [GIT PULL 2/2] ARM: imx: device tree changes for 3.14)

[Shawn Guo]

Hi Olof,

On Thu, Jan 02, 2014 at 12:21:08PM -0800, Olof Johansson wrote:
> >  .../devicetree/bindings/vendor-prefixes.txt        |    3 +
> >  arch/arm/boot/dts/imx25-pinfunc.h                  |  494 +++++++++++
> >  arch/arm/boot/dts/imx25-pingrp.h                   |   81 ++
> >  arch/arm/boot/dts/imx27-pinfunc.h                  |  526 +++++++++++
> >  arch/arm/boot/dts/imx27-pingrp.h                   |  151 ++++
> >  arch/arm/boot/dts/imx35-pingrp.h                   |  104 +++
> >  arch/arm/boot/dts/imx50-pinfunc.h                  |  923 ++++++++++++++++++++
> >  arch/arm/boot/dts/imx50-pingrp.h                   |  146 ++++
> >  arch/arm/boot/dts/imx51-pingrp.h                   |  249 ++++++
> >  arch/arm/boot/dts/imx53-pingrp.h                   |  352 ++++++++
> >  arch/arm/boot/dts/imx6dl-pinfunc.h                 |    2 +
> >  arch/arm/boot/dts/imx6q-pinfunc.h                  |    2 +
> >  arch/arm/boot/dts/imx6qdl-pingrp.h                 |  532 +++++++++++
> >  arch/arm/boot/dts/imx6sl-pingrp.h                  |  148 ++++
> >  arch/arm/boot/dts/vf610-pingrp.h                   |  127 +++
> 
> Hm, these don't quite use include files the way include files were
> originally meant to be used -- initially the idea was to use them to
> define mostly simple constants instead of full properties like this.

The DT macro support was introduced to improve the readability of device
tree sources by replacing those magic numbers with readable macros.  I
think the usage in imx pinctrl binding perfectly fits the purpose.  You
can get details of the binding in
Documentation/devicetree/bindings/pinctrl/fsl,imx-pinctrl.txt.

> 
> I'm not against the idea of using it this way, but I also want to make sure the
> DT maintainers are OK with it. So I've cc:d them on this reply.

This is not a new thing. It was firstly adopted for imx6q in v3.10
release with commit e164153 (pinctrl: imx: move hard-coding data into
device tree), which had been posted to devicetree list for sure.  We're
just moving more i.MX SoCs to it.

> 
> I'm also not crazy about the insanely long identifiers used here, but I guess
> they correlate with some user manual tables?

Yes, the identifiers follows the pad and function names from reference
manual.

Shawn

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: DT include files (was: [GIT PULL 2/2] ARM: imx: device tree changes for 3.14)

[Olof Johansson]

On Thu, Jan 2, 2014 at 6:32 PM, Shawn Guo <shawn.guo-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> Hi Olof,
>
> On Thu, Jan 02, 2014 at 12:21:08PM -0800, Olof Johansson wrote:
>> >  .../devicetree/bindings/vendor-prefixes.txt        |    3 +
>> >  arch/arm/boot/dts/imx25-pinfunc.h                  |  494 +++++++++++
>> >  arch/arm/boot/dts/imx25-pingrp.h                   |   81 ++
>> >  arch/arm/boot/dts/imx27-pinfunc.h                  |  526 +++++++++++
>> >  arch/arm/boot/dts/imx27-pingrp.h                   |  151 ++++
>> >  arch/arm/boot/dts/imx35-pingrp.h                   |  104 +++
>> >  arch/arm/boot/dts/imx50-pinfunc.h                  |  923 ++++++++++++++++++++
>> >  arch/arm/boot/dts/imx50-pingrp.h                   |  146 ++++
>> >  arch/arm/boot/dts/imx51-pingrp.h                   |  249 ++++++
>> >  arch/arm/boot/dts/imx53-pingrp.h                   |  352 ++++++++
>> >  arch/arm/boot/dts/imx6dl-pinfunc.h                 |    2 +
>> >  arch/arm/boot/dts/imx6q-pinfunc.h                  |    2 +
>> >  arch/arm/boot/dts/imx6qdl-pingrp.h                 |  532 +++++++++++
>> >  arch/arm/boot/dts/imx6sl-pingrp.h                  |  148 ++++
>> >  arch/arm/boot/dts/vf610-pingrp.h                   |  127 +++
>>
>> Hm, these don't quite use include files the way include files were
>> originally meant to be used -- initially the idea was to use them to
>> define mostly simple constants instead of full properties like this.
>
> The DT macro support was introduced to improve the readability of device
> tree sources by replacing those magic numbers with readable macros.  I
> think the usage in imx pinctrl binding perfectly fits the purpose.  You
> can get details of the binding in
> Documentation/devicetree/bindings/pinctrl/fsl,imx-pinctrl.txt.

To be honest I didn't follow that discussion closely. If DT
maintainers are OK with that style, then I'm OK. :)

>> I'm not against the idea of using it this way, but I also want to make sure the
>> DT maintainers are OK with it. So I've cc:d them on this reply.
>
> This is not a new thing. It was firstly adopted for imx6q in v3.10
> release with commit e164153 (pinctrl: imx: move hard-coding data into
> device tree), which had been posted to devicetree list for sure.  We're
> just moving more i.MX SoCs to it.

Ok, then it's probably just the location of the header files that
should be adjusted. Other subsystems have placed them under
include/dt-bindings/<subsystem>, so that's likely a better place for
these as well, don't you think?

>> I'm also not crazy about the insanely long identifiers used here, but I guess
>> they correlate with some user manual tables?
>
> Yes, the identifiers follows the pad and function names from reference
> manual.

Ok, fair enough.


-Olof
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: DT include files (was: [GIT PULL 2/2] ARM: imx: device tree changes for 3.14)

[Shawn Guo]

On Thu, Jan 02, 2014 at 06:41:30PM -0800, Olof Johansson wrote:
> Ok, then it's probably just the location of the header files that
> should be adjusted. Other subsystems have placed them under
> include/dt-bindings/<subsystem>, so that's likely a better place for
> these as well, don't you think?

I had a little discussion with DT people when the headers were firstly
created.  These pinctrl headers are a little different from the headers
in include/dt-bindings/<subsystem>.  The latter are used by both kernel
and device tree sources, while the pinctrl headers are used by device
tree sources only, so I chose to put them just in the same folder as
dts files.  And DT people are fine with my take.

Shawn

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: DT include files (was: [GIT PULL 2/2] ARM: imx: device tree changes for 3.14)

[Olof Johansson]

On Thu, Jan 2, 2014 at 7:04 PM, Shawn Guo <shawn.guo-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> On Thu, Jan 02, 2014 at 06:41:30PM -0800, Olof Johansson wrote:
>> Ok, then it's probably just the location of the header files that
>> should be adjusted. Other subsystems have placed them under
>> include/dt-bindings/<subsystem>, so that's likely a better place for
>> these as well, don't you think?
>
> I had a little discussion with DT people when the headers were firstly
> created.  These pinctrl headers are a little different from the headers
> in include/dt-bindings/<subsystem>.  The latter are used by both kernel
> and device tree sources, while the pinctrl headers are used by device
> tree sources only, so I chose to put them just in the same folder as
> dts files.  And DT people are fine with my take.

Since you don't provide references to this I had to go searching for
it. All I find is some discussion from 8 months ago, and quite a bit
of that seems to have been about changing bindings, and some about the
preprocessor behavior. Also, the patches seem to have been too big to
make it out on the lists.

I'd like a fresh look from DT people on this just to make sure no
opinions have changed -- lots of things have changed in the last 8
months w.r.t. DT.

I've pinged them on IRC, let's see if they wake up. Adding explicit cc too.


-Olof
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: DT include files (was: [GIT PULL 2/2] ARM: imx: device tree changes for 3.14)

[Shawn Guo]

On Fri, Jan 03, 2014 at 11:29:35AM -0800, Olof Johansson wrote:
> On Thu, Jan 2, 2014 at 7:04 PM, Shawn Guo <shawn.guo@linaro.org> wrote:
> > On Thu, Jan 02, 2014 at 06:41:30PM -0800, Olof Johansson wrote:
> >> Ok, then it's probably just the location of the header files that
> >> should be adjusted. Other subsystems have placed them under
> >> include/dt-bindings/<subsystem>, so that's likely a better place for
> >> these as well, don't you think?
> >
> > I had a little discussion with DT people when the headers were firstly
> > created.  These pinctrl headers are a little different from the headers
> > in include/dt-bindings/<subsystem>.  The latter are used by both kernel
> > and device tree sources, while the pinctrl headers are used by device
> > tree sources only, so I chose to put them just in the same folder as
> > dts files.  And DT people are fine with my take.
> 
> Since you don't provide references to this I had to go searching for
> it. All I find is some discussion from 8 months ago, and quite a bit
> of that seems to have been about changing bindings, and some about the
> preprocessor behavior. Also, the patches seem to have been too big to
> make it out on the lists.
> 
> I'd like a fresh look from DT people on this just to make sure no
> opinions have changed -- lots of things have changed in the last 8
> months w.r.t. DT.

Indeed, it's been quite a long time.  Let me restate my point.  The
include/dt-bindings is introduced as a folder to hold headers that are
referenced by both kernel and DTS.  That's why we create the folder in
the kernel include folder and have arch/arm/boot/dts/include/dt-bindings
being a symbol link to it.  All the headers in there need to be
duplicated between kernel and DTS tree, when we move DTS files into
a separated repository.  Putting DTS local headers into the folder is
absolutely unnecessary, and will only confuse people and bother
ourselves when moving DTS files out of kernel tree.

Shawn

> 
> I've pinged them on IRC, let's see if they wake up. Adding explicit cc too.
> 
> 
> -Olof

Re: DT include files (was: [GIT PULL 2/2] ARM: imx: device tree changes for 3.14)

[Shawn Guo]

On Sat, Jan 04, 2014 at 09:10:58AM +0800, Shawn Guo wrote:
> On Fri, Jan 03, 2014 at 11:29:35AM -0800, Olof Johansson wrote:
> > On Thu, Jan 2, 2014 at 7:04 PM, Shawn Guo <shawn.guo-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> > > On Thu, Jan 02, 2014 at 06:41:30PM -0800, Olof Johansson wrote:
> > >> Ok, then it's probably just the location of the header files that
> > >> should be adjusted. Other subsystems have placed them under
> > >> include/dt-bindings/<subsystem>, so that's likely a better place for
> > >> these as well, don't you think?
> > >
> > > I had a little discussion with DT people when the headers were firstly
> > > created.  These pinctrl headers are a little different from the headers
> > > in include/dt-bindings/<subsystem>.  The latter are used by both kernel
> > > and device tree sources, while the pinctrl headers are used by device
> > > tree sources only, so I chose to put them just in the same folder as
> > > dts files.  And DT people are fine with my take.
> > 
> > Since you don't provide references to this I had to go searching for
> > it. All I find is some discussion from 8 months ago, and quite a bit
> > of that seems to have been about changing bindings, and some about the
> > preprocessor behavior. Also, the patches seem to have been too big to
> > make it out on the lists.
> > 
> > I'd like a fresh look from DT people on this just to make sure no
> > opinions have changed -- lots of things have changed in the last 8
> > months w.r.t. DT.
> 
> Indeed, it's been quite a long time.  Let me restate my point.  The
> include/dt-bindings is introduced as a folder to hold headers that are
> referenced by both kernel and DTS.  That's why we create the folder in
> the kernel include folder and have arch/arm/boot/dts/include/dt-bindings
> being a symbol link to it.  All the headers in there need to be
> duplicated between kernel and DTS tree, when we move DTS files into
> a separated repository.  Putting DTS local headers into the folder is
> absolutely unnecessary, and will only confuse people and bother
> ourselves when moving DTS files out of kernel tree.

Just a gentle ping to ensure we do not get the pull request lost.  Or do
you have any further comment?

Shawn

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: DT include files (was: [GIT PULL 2/2] ARM: imx: device tree changes for 3.14)

[Olof Johansson]

On Thu, Jan 9, 2014 at 6:41 PM, Shawn Guo <shawn.guo-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> On Sat, Jan 04, 2014 at 09:10:58AM +0800, Shawn Guo wrote:
>> On Fri, Jan 03, 2014 at 11:29:35AM -0800, Olof Johansson wrote:
>> > On Thu, Jan 2, 2014 at 7:04 PM, Shawn Guo <shawn.guo-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
>> > > On Thu, Jan 02, 2014 at 06:41:30PM -0800, Olof Johansson wrote:
>> > >> Ok, then it's probably just the location of the header files that
>> > >> should be adjusted. Other subsystems have placed them under
>> > >> include/dt-bindings/<subsystem>, so that's likely a better place for
>> > >> these as well, don't you think?
>> > >
>> > > I had a little discussion with DT people when the headers were firstly
>> > > created.  These pinctrl headers are a little different from the headers
>> > > in include/dt-bindings/<subsystem>.  The latter are used by both kernel
>> > > and device tree sources, while the pinctrl headers are used by device
>> > > tree sources only, so I chose to put them just in the same folder as
>> > > dts files.  And DT people are fine with my take.
>> >
>> > Since you don't provide references to this I had to go searching for
>> > it. All I find is some discussion from 8 months ago, and quite a bit
>> > of that seems to have been about changing bindings, and some about the
>> > preprocessor behavior. Also, the patches seem to have been too big to
>> > make it out on the lists.
>> >
>> > I'd like a fresh look from DT people on this just to make sure no
>> > opinions have changed -- lots of things have changed in the last 8
>> > months w.r.t. DT.
>>
>> Indeed, it's been quite a long time.  Let me restate my point.  The
>> include/dt-bindings is introduced as a folder to hold headers that are
>> referenced by both kernel and DTS.  That's why we create the folder in
>> the kernel include folder and have arch/arm/boot/dts/include/dt-bindings
>> being a symbol link to it.  All the headers in there need to be
>> duplicated between kernel and DTS tree, when we move DTS files into
>> a separated repository.  Putting DTS local headers into the folder is
>> absolutely unnecessary, and will only confuse people and bother
>> ourselves when moving DTS files out of kernel tree.
>
> Just a gentle ping to ensure we do not get the pull request lost.  Or do
> you have any further comment?

Still waiting on DT maintainers to chime in.

-Olof
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: DT include files

[Tomasz Figa]

Hi,

On 10.01.2014 03:41, Olof Johansson wrote:
> On Thu, Jan 9, 2014 at 6:41 PM, Shawn Guo <shawn.guo-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
>> On Sat, Jan 04, 2014 at 09:10:58AM +0800, Shawn Guo wrote:
>>> On Fri, Jan 03, 2014 at 11:29:35AM -0800, Olof Johansson wrote:
>>>> On Thu, Jan 2, 2014 at 7:04 PM, Shawn Guo <shawn.guo-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
>>>>> On Thu, Jan 02, 2014 at 06:41:30PM -0800, Olof Johansson wrote:
>>>>>> Ok, then it's probably just the location of the header files that
>>>>>> should be adjusted. Other subsystems have placed them under
>>>>>> include/dt-bindings/<subsystem>, so that's likely a better place for
>>>>>> these as well, don't you think?
>>>>>
>>>>> I had a little discussion with DT people when the headers were firstly
>>>>> created.  These pinctrl headers are a little different from the headers
>>>>> in include/dt-bindings/<subsystem>.  The latter are used by both kernel
>>>>> and device tree sources, while the pinctrl headers are used by device
>>>>> tree sources only, so I chose to put them just in the same folder as
>>>>> dts files.  And DT people are fine with my take.
>>>>
>>>> Since you don't provide references to this I had to go searching for
>>>> it. All I find is some discussion from 8 months ago, and quite a bit
>>>> of that seems to have been about changing bindings, and some about the
>>>> preprocessor behavior. Also, the patches seem to have been too big to
>>>> make it out on the lists.
>>>>
>>>> I'd like a fresh look from DT people on this just to make sure no
>>>> opinions have changed -- lots of things have changed in the last 8
>>>> months w.r.t. DT.
>>>
>>> Indeed, it's been quite a long time.  Let me restate my point.  The
>>> include/dt-bindings is introduced as a folder to hold headers that are
>>> referenced by both kernel and DTS.  That's why we create the folder in
>>> the kernel include folder and have arch/arm/boot/dts/include/dt-bindings
>>> being a symbol link to it.  All the headers in there need to be
>>> duplicated between kernel and DTS tree, when we move DTS files into
>>> a separated repository.  Putting DTS local headers into the folder is
>>> absolutely unnecessary, and will only confuse people and bother
>>> ourselves when moving DTS files out of kernel tree.
>>
>> Just a gentle ping to ensure we do not get the pull request lost.  Or do
>> you have any further comment?
>
> Still waiting on DT maintainers to chime in.

I'm not officially a DT maintainer, but let me share my thoughts on this.

So what options we have for this:

  1) include/dt-bindings - this directory is designed to contain headers
     that define the ABI between firmware and kernel code, in other
     words - DT bindings.

  2) arch/*/boot/dts - we already have files that can be included by
     other files in this directory, i.e. *.dtsi. Some are used to
     implement hierarchies of devices (e.g. s3c64xx.dtsi), but some are
     purely used as headers (s3c64xx-pinctrl.dtsi).

  3) include/dts? - I'm not sure if this have any benefits, but I'm
     listing it since it's one of the ideas that came to my mind.

Now 2) seems to be already used and doesn't seem to generate any 
problems, so I'm all for it. Still, there is one more issue. For files 
to be included merely by DTS files and so limited by DTS+CPP syntax, I 
don't think it's too good idea to call them *.h. Let's stay with *.dtsi, 
since that's the file extension supposed to be used for files that can 
be included from device tree sources.

Best regards,
Tomasz
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: DT include files

[Rob Herring]

On Fri, Jan 10, 2014 at 7:28 AM, Tomasz Figa <t.figa-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org> wrote:
> Hi,
>
> On 10.01.2014 03:41, Olof Johansson wrote:
>>
>> On Thu, Jan 9, 2014 at 6:41 PM, Shawn Guo <shawn.guo-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
>>>
>>> On Sat, Jan 04, 2014 at 09:10:58AM +0800, Shawn Guo wrote:
>>>>
>>>> On Fri, Jan 03, 2014 at 11:29:35AM -0800, Olof Johansson wrote:
>>>>>
>>>>> On Thu, Jan 2, 2014 at 7:04 PM, Shawn Guo <shawn.guo-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
>>>>>>
>>>>>> On Thu, Jan 02, 2014 at 06:41:30PM -0800, Olof Johansson wrote:
>>>>>>>
>>>>>>> Ok, then it's probably just the location of the header files that
>>>>>>> should be adjusted. Other subsystems have placed them under
>>>>>>> include/dt-bindings/<subsystem>, so that's likely a better place for
>>>>>>> these as well, don't you think?
>>>>>>
>>>>>>
>>>>>> I had a little discussion with DT people when the headers were firstly
>>>>>> created.  These pinctrl headers are a little different from the
>>>>>> headers
>>>>>> in include/dt-bindings/<subsystem>.  The latter are used by both
>>>>>> kernel
>>>>>> and device tree sources, while the pinctrl headers are used by device
>>>>>> tree sources only, so I chose to put them just in the same folder as
>>>>>> dts files.  And DT people are fine with my take.
>>>>>
>>>>>
>>>>> Since you don't provide references to this I had to go searching for
>>>>> it. All I find is some discussion from 8 months ago, and quite a bit
>>>>> of that seems to have been about changing bindings, and some about the
>>>>> preprocessor behavior. Also, the patches seem to have been too big to
>>>>> make it out on the lists.
>>>>>
>>>>> I'd like a fresh look from DT people on this just to make sure no
>>>>> opinions have changed -- lots of things have changed in the last 8
>>>>> months w.r.t. DT.
>>>>
>>>>
>>>> Indeed, it's been quite a long time.  Let me restate my point.  The
>>>> include/dt-bindings is introduced as a folder to hold headers that are
>>>> referenced by both kernel and DTS.  That's why we create the folder in
>>>> the kernel include folder and have arch/arm/boot/dts/include/dt-bindings
>>>> being a symbol link to it.  All the headers in there need to be
>>>> duplicated between kernel and DTS tree, when we move DTS files into
>>>> a separated repository.  Putting DTS local headers into the folder is
>>>> absolutely unnecessary, and will only confuse people and bother
>>>> ourselves when moving DTS files out of kernel tree.
>>>
>>>
>>> Just a gentle ping to ensure we do not get the pull request lost.  Or do
>>> you have any further comment?
>>
>>
>> Still waiting on DT maintainers to chime in.

It helps (slightly) to be copied directly.

> I'm not officially a DT maintainer, but let me share my thoughts on this.

We can fix that... :)

> So what options we have for this:
>
>  1) include/dt-bindings - this directory is designed to contain headers
>     that define the ABI between firmware and kernel code, in other
>     words - DT bindings.
>
>  2) arch/*/boot/dts - we already have files that can be included by
>     other files in this directory, i.e. *.dtsi. Some are used to
>     implement hierarchies of devices (e.g. s3c64xx.dtsi), but some are
>     purely used as headers (s3c64xx-pinctrl.dtsi).
>
>  3) include/dts? - I'm not sure if this have any benefits, but I'm
>     listing it since it's one of the ideas that came to my mind.
>
> Now 2) seems to be already used and doesn't seem to generate any problems,
> so I'm all for it. Still, there is one more issue. For files to be included
> merely by DTS files and so limited by DTS+CPP syntax, I don't think it's too
> good idea to call them *.h. Let's stay with *.dtsi, since that's the file
> extension supposed to be used for files that can be included from device
> tree sources.

I think having 1 and 2 is the right way. Minimizing the number of
shared headers between dts and the kernel is a good thing.

As for *.h vs. *.dtsi naming, if the include is only pre-processor
defines, then I think using .h is the right way. Otherwise, if there
is any dts syntax, then .dtsi is the right name. It looks to me like
this style has been followed in both the imx and s3c64xx cases.

On a side note, I'm not really a fan of this pattern:

#define FOO1 1 2 3

#define BAR FOO1 FOO2 FOO3

While it definitely simplifies the dts files, it would never be used
in C and obfuscates what the actual property size is. Reading a dts
file, I would naturally assume the define was a single value while in
fact it could expand to a very large property size.

Rob
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: DT include files

[Gerhard Sittig]

On Fri, Jan 10, 2014 at 09:30 -0600, Rob Herring wrote:
> 
> As for *.h vs. *.dtsi naming, if the include is only pre-processor
> defines, then I think using .h is the right way. Otherwise, if there
> is any dts syntax, then .dtsi is the right name. It looks to me like
> this style has been followed in both the imx and s3c64xx cases.
> 
> On a side note, I'm not really a fan of this pattern:
> 
> #define FOO1 1 2 3
> 
> #define BAR FOO1 FOO2 FOO3
> 
> While it definitely simplifies the dts files, it would never be used
> in C and obfuscates what the actual property size is. Reading a dts
> file, I would naturally assume the define was a single value while in
> fact it could expand to a very large property size.

For more complex yet tedious repetitive cases like GPIO banks and
pins I've seen #define directives which introduce "functions"
(macros with parameters).  They appear to be rather useful, can
reflect very well the essence of the information, don't
necessarily pretend to be single values, but still may hide how
many cells they expand to.  For example:

  arch/arm/boot/dts/tegra30-cardhu.dtsi:
    interrupts = <TEGRA_GPIO(L, 0) IRQ_TYPE_LEVEL_HIGH>;


virtually yours
Gerhard Sittig
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr. 5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office-ynQEQJNshbs@public.gmane.org
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: DT include files

[Stephen Warren]

On 01/10/2014 10:03 AM, Gerhard Sittig wrote:
> On Fri, Jan 10, 2014 at 09:30 -0600, Rob Herring wrote:
>>
>> As for *.h vs. *.dtsi naming, if the include is only pre-processor
>> defines, then I think using .h is the right way. Otherwise, if there
>> is any dts syntax, then .dtsi is the right name. It looks to me like
>> this style has been followed in both the imx and s3c64xx cases.
>>
>> On a side note, I'm not really a fan of this pattern:
>>
>> #define FOO1 1 2 3
>>
>> #define BAR FOO1 FOO2 FOO3
>>
>> While it definitely simplifies the dts files, it would never be used
>> in C and obfuscates what the actual property size is. Reading a dts
>> file, I would naturally assume the define was a single value while in
>> fact it could expand to a very large property size.
> 
> For more complex yet tedious repetitive cases like GPIO banks and
> pins I've seen #define directives which introduce "functions"
> (macros with parameters).  They appear to be rather useful, can
> reflect very well the essence of the information, don't
> necessarily pretend to be single values, but still may hide how
> many cells they expand to.  For example:
> 
>   arch/arm/boot/dts/tegra30-cardhu.dtsi:
>     interrupts = <TEGRA_GPIO(L, 0) IRQ_TYPE_LEVEL_HIGH>;

I'm not sure I entirely understand your point, but for the record, both
TEGRA_GPIO() and IRQ_TYPE_LEVEL_HIGH expand to a single cell, as I would
expect (almost?) any DT macro to do.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: DT include files

[Gerhard Sittig]

On Mon, Jan 13, 2014 at 09:48 -0700, Stephen Warren wrote:
> 
> On 01/10/2014 10:03 AM, Gerhard Sittig wrote:
> > On Fri, Jan 10, 2014 at 09:30 -0600, Rob Herring wrote:
> >>
> >> As for *.h vs. *.dtsi naming, if the include is only pre-processor
> >> defines, then I think using .h is the right way. Otherwise, if there
> >> is any dts syntax, then .dtsi is the right name. It looks to me like
> >> this style has been followed in both the imx and s3c64xx cases.
> >>
> >> On a side note, I'm not really a fan of this pattern:
> >>
> >> #define FOO1 1 2 3
> >>
> >> #define BAR FOO1 FOO2 FOO3
> >>
> >> While it definitely simplifies the dts files, it would never be used
> >> in C and obfuscates what the actual property size is. Reading a dts
> >> file, I would naturally assume the define was a single value while in
> >> fact it could expand to a very large property size.
> > 
> > For more complex yet tedious repetitive cases like GPIO banks and
> > pins I've seen #define directives which introduce "functions"
> > (macros with parameters).  They appear to be rather useful, can
> > reflect very well the essence of the information, don't
> > necessarily pretend to be single values, but still may hide how
> > many cells they expand to.  For example:
> > 
> >   arch/arm/boot/dts/tegra30-cardhu.dtsi:
> >     interrupts = <TEGRA_GPIO(L, 0) IRQ_TYPE_LEVEL_HIGH>;
> 
> I'm not sure I entirely understand your point, but for the record, both
> TEGRA_GPIO() and IRQ_TYPE_LEVEL_HIGH expand to a single cell, as I would
> expect (almost?) any DT macro to do.

It turns out I slightly misunderstood Rob's concern.  The issue
is not that the use of the C language preprocessor is discouraged
or frouned upon, but instead that obfuscating invariant data or
spilling random parts of the information across the place in
unneeded ways is unwanted.  While there is agreement that the use
of symbolic identifiers can help readability and maintainability
in certain situations.

Regarding your specific reply:  I haven't noticed before that all
of the macros I cited do expand to a single cell.  Before that I
saw the potential to generate more cells when required for a
single value (like "wide" addresses, or multi cell pins or
channels or whatever).  Can't tell how much of a concern there is
against such an approach, don't have strong feelings about it.

Actually I wasn't picking at those specific macros above, quite
the contrary.  Those were the first I came across searching for
demonstration of useful preprocessor use. :)  I just did not
bother to cite several hundred other useful phrases as well.


virtually yours
Gerhard Sittig
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr. 5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office-ynQEQJNshbs@public.gmane.org
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: DT include files

[Olof Johansson]

On Fri, Jan 10, 2014 at 7:30 AM, Rob Herring <robherring2-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> On Fri, Jan 10, 2014 at 7:28 AM, Tomasz Figa <t.figa-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org> wrote:
>> Hi,
>>
>> On 10.01.2014 03:41, Olof Johansson wrote:
>>>
>>> On Thu, Jan 9, 2014 at 6:41 PM, Shawn Guo <shawn.guo-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
>>>>
>>>> On Sat, Jan 04, 2014 at 09:10:58AM +0800, Shawn Guo wrote:
>>>>>
>>>>> On Fri, Jan 03, 2014 at 11:29:35AM -0800, Olof Johansson wrote:
>>>>>>
>>>>>> On Thu, Jan 2, 2014 at 7:04 PM, Shawn Guo <shawn.guo-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
>>>>>>>
>>>>>>> On Thu, Jan 02, 2014 at 06:41:30PM -0800, Olof Johansson wrote:
>>>>>>>>
>>>>>>>> Ok, then it's probably just the location of the header files that
>>>>>>>> should be adjusted. Other subsystems have placed them under
>>>>>>>> include/dt-bindings/<subsystem>, so that's likely a better place for
>>>>>>>> these as well, don't you think?
>>>>>>>
>>>>>>>
>>>>>>> I had a little discussion with DT people when the headers were firstly
>>>>>>> created.  These pinctrl headers are a little different from the
>>>>>>> headers
>>>>>>> in include/dt-bindings/<subsystem>.  The latter are used by both
>>>>>>> kernel
>>>>>>> and device tree sources, while the pinctrl headers are used by device
>>>>>>> tree sources only, so I chose to put them just in the same folder as
>>>>>>> dts files.  And DT people are fine with my take.
>>>>>>
>>>>>>
>>>>>> Since you don't provide references to this I had to go searching for
>>>>>> it. All I find is some discussion from 8 months ago, and quite a bit
>>>>>> of that seems to have been about changing bindings, and some about the
>>>>>> preprocessor behavior. Also, the patches seem to have been too big to
>>>>>> make it out on the lists.
>>>>>>
>>>>>> I'd like a fresh look from DT people on this just to make sure no
>>>>>> opinions have changed -- lots of things have changed in the last 8
>>>>>> months w.r.t. DT.
>>>>>
>>>>>
>>>>> Indeed, it's been quite a long time.  Let me restate my point.  The
>>>>> include/dt-bindings is introduced as a folder to hold headers that are
>>>>> referenced by both kernel and DTS.  That's why we create the folder in
>>>>> the kernel include folder and have arch/arm/boot/dts/include/dt-bindings
>>>>> being a symbol link to it.  All the headers in there need to be
>>>>> duplicated between kernel and DTS tree, when we move DTS files into
>>>>> a separated repository.  Putting DTS local headers into the folder is
>>>>> absolutely unnecessary, and will only confuse people and bother
>>>>> ourselves when moving DTS files out of kernel tree.
>>>>
>>>>
>>>> Just a gentle ping to ensure we do not get the pull request lost.  Or do
>>>> you have any further comment?
>>>
>>>
>>> Still waiting on DT maintainers to chime in.
>
> It helps (slightly) to be copied directly.

I should have copied you on some of the thread, but this was also
right around when your calxeda address was going away so I might have
missed you.

>
>> I'm not officially a DT maintainer, but let me share my thoughts on this.
>
> We can fix that... :)
>
>> So what options we have for this:
>>
>>  1) include/dt-bindings - this directory is designed to contain headers
>>     that define the ABI between firmware and kernel code, in other
>>     words - DT bindings.
>>
>>  2) arch/*/boot/dts - we already have files that can be included by
>>     other files in this directory, i.e. *.dtsi. Some are used to
>>     implement hierarchies of devices (e.g. s3c64xx.dtsi), but some are
>>     purely used as headers (s3c64xx-pinctrl.dtsi).
>>
>>  3) include/dts? - I'm not sure if this have any benefits, but I'm
>>     listing it since it's one of the ideas that came to my mind.
>>
>> Now 2) seems to be already used and doesn't seem to generate any problems,
>> so I'm all for it. Still, there is one more issue. For files to be included
>> merely by DTS files and so limited by DTS+CPP syntax, I don't think it's too
>> good idea to call them *.h. Let's stay with *.dtsi, since that's the file
>> extension supposed to be used for files that can be included from device
>> tree sources.
>
> I think having 1 and 2 is the right way. Minimizing the number of
> shared headers between dts and the kernel is a good thing.
>
> As for *.h vs. *.dtsi naming, if the include is only pre-processor
> defines, then I think using .h is the right way. Otherwise, if there
> is any dts syntax, then .dtsi is the right name. It looks to me like
> this style has been followed in both the imx and s3c64xx cases.

I'm mostly reacting to the use of .h for something that isn't C
preprocessing. Since dtc/dts has their own namespace for everything
else, it seems appropriate to have something unique. But this is
pretty far up bikeshed alley.

> On a side note, I'm not really a fan of this pattern:
>
> #define FOO1 1 2 3
>
> #define BAR FOO1 FOO2 FOO3
>
> While it definitely simplifies the dts files, it would never be used
> in C and obfuscates what the actual property size is. Reading a dts
> file, I would naturally assume the define was a single value while in
> fact it could expand to a very large property size.

Hmm. Shawn?


-Olof
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: DT include files

[Shawn Guo]

On Fri, Jan 10, 2014 at 10:37:01AM -0800, Olof Johansson wrote:
> > On a side note, I'm not really a fan of this pattern:
> >
> > #define FOO1 1 2 3
> >
> > #define BAR FOO1 FOO2 FOO3
> >
> > While it definitely simplifies the dts files, it would never be used
> > in C and obfuscates what the actual property size is. Reading a dts
> > file, I would naturally assume the define was a single value while in
> > fact it could expand to a very large property size.
> 
> Hmm. Shawn?

What I read from Rob is that he does not like it but he does not hate it
so much either.  And that's probably why I did not get either ACK or
NACK from him when the usage was firstly introduced :)

While I agree such macro usage should be avoided or limited, it really
helps IMX pinctrl case to improve the readability and make the handwriting
of these pinctrl entries less error prone.  Isn't it the whole point of
introducing macro support for DTC?

Shawn

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: DT include files

[Arnd Bergmann]

On Saturday 11 January 2014, Shawn Guo wrote:
> While I agree such macro usage should be avoided or limited, it really
> helps IMX pinctrl case to improve the readability and make the handwriting
> of these pinctrl entries less error prone.  Isn't it the whole point of
> introducing macro support for DTC?

The macros are useful for all cases where the binding defines an
arbitrary number space, e.g. for the interrupt flags or when a
clock or pin controller provides a set of pins/clocks that don't
already have a number assigned to them. In this case, the header
file can be shared between the dts and kernel source files to ensure
that they are talking about the same things.

However, I don't like to see uses of such macros where the definition
is done to match a hardware constant (bit number, register index, etc),
like in the recently posted eDMA driver case. Here the macro /reduces/
readability of the dts source IMHO, because you cannot easily check
the dts file against the data sheet without cross-referencing the
header file as well.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: DT include files

[Shawn Guo]

On Sat, Jan 11, 2014 at 02:15:28PM +0100, Arnd Bergmann wrote:
> On Saturday 11 January 2014, Shawn Guo wrote:
> > While I agree such macro usage should be avoided or limited, it really
> > helps IMX pinctrl case to improve the readability and make the handwriting
> > of these pinctrl entries less error prone.  Isn't it the whole point of
> > introducing macro support for DTC?
> 
> The macros are useful for all cases where the binding defines an
> arbitrary number space, e.g. for the interrupt flags or when a
> clock or pin controller provides a set of pins/clocks that don't
> already have a number assigned to them. In this case, the header
> file can be shared between the dts and kernel source files to ensure
> that they are talking about the same things.
> 
> However, I don't like to see uses of such macros where the definition
> is done to match a hardware constant (bit number, register index, etc),
> like in the recently posted eDMA driver case. Here the macro /reduces/
> readability of the dts source IMHO, because you cannot easily check
> the dts file against the data sheet without cross-referencing the
> header file as well.

I agree with you that we shouldn't use macros for IP block resources
like irq number and dma channel.  Once we code these SoC specific
data/number in <soc>.dtsi, all that board level dts files need to do
is to include <soc>.dtsi, nothing else.  However, pinctrl data is a
different case.  Which pin is in use, which mux option is selected for
the pin, how the pin should be configured, all these are board specific.
So these pinctrl configuration should be defined by individual board
level dts file rather than <soc>.dtsi.

Let's say the mux option SPDIF_OUT of pin GPIO_17 is used on a few board
designs, hummingboard, nitrogen6x and wandboard.  If we do not use
macro, every author of these board dts files need to look into reference
manual to find out the following numbers

 - The mux register offset of pin GPIO_17
 - The select input register offset of pin GPIO_17
 - The config register offset of pin GPIO_17
 - The value of SPDIF_OUT mux option for GPIO_17
 - The select input value of SPDIF_OUT mux option for GPIO_17

, and then code these numbers in their <board>.dts by hand.  It's
boring and error prone.  As a comparison, we generate these numbers
from reference manual database using a tool and define a macro
MX6QDL_PAD_GPIO_17__SPDIF_OUT for these numbers.  The authors only need
to find out this macro from imx6q-pinfunc.dtsi and fill it into his dts.
Isn't the macro use here helping to ease everyone's life and make the
coding less error prone, since the macro is generated from database?

Shawn

Re: DT include files

[Arnd Bergmann]

On Sunday 12 January 2014, Shawn Guo wrote:
> Let's say the mux option SPDIF_OUT of pin GPIO_17 is used on a few board
> designs, hummingboard, nitrogen6x and wandboard.  If we do not use
> macro, every author of these board dts files need to look into reference
> manual to find out the following numbers
> 
>  - The mux register offset of pin GPIO_17
>  - The select input register offset of pin GPIO_17
>  - The config register offset of pin GPIO_17
>  - The value of SPDIF_OUT mux option for GPIO_17
>  - The select input value of SPDIF_OUT mux option for GPIO_17
> 
> , and then code these numbers in their <board>.dts by hand.  It's
> boring and error prone.  As a comparison, we generate these numbers
> from reference manual database using a tool and define a macro
> MX6QDL_PAD_GPIO_17__SPDIF_OUT for these numbers.  The authors only need
> to find out this macro from imx6q-pinfunc.dtsi and fill it into his dts.
> Isn't the macro use here helping to ease everyone's life and make the
> coding less error prone, since the macro is generated from database?

I was under the impression that the generic pinctrl binding was designed
in a way to let you assign labels to each possible (reasonable)
configuration so you didn't have get to this level of detail at the
driver. I'm also surprised that you have to know multiple constants
(mux register, input register, config register offsets) to select a
particular pin. I would have expected that you could have one constant
from which the driver is able to compute the other ones.

If you actually need the complexity that you describe in the board files,
the macros don't seem worse than the alternative, I would just be happier
if the pinctrl driver binding could do without them.

	Arnd

Re: DT include files

[Linus Walleij]

On Sun, Jan 12, 2014 at 9:21 PM, Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org> wrote:
> On Sunday 12 January 2014, Shawn Guo wrote:

>> , and then code these numbers in their <board>.dts by hand.  It's
>> boring and error prone.  As a comparison, we generate these numbers
>> from reference manual database using a tool and define a macro
>> MX6QDL_PAD_GPIO_17__SPDIF_OUT for these numbers.  The authors only need
>> to find out this macro from imx6q-pinfunc.dtsi and fill it into his dts.
>> Isn't the macro use here helping to ease everyone's life and make the
>> coding less error prone, since the macro is generated from database?
>
> I was under the impression that the generic pinctrl binding was designed
> in a way to let you assign labels to each possible (reasonable)
> configuration so you didn't have get to this level of detail at the
> driver.

There is a binding for handles and states tied to a device.

There is no generic pinctrl binding for the definition of groups
and functions and how they map, i.e. muxing.

The discussion around this ended up with everyone disagreeing
and eventually everybody started writing their own bindings and
have them merged.

There is a generic pin config binding, not everyone uses it,
as they defined their bindings before I got that in place. It
is mostly a requirement for newer drivers.

This Freescale driver does not use the generic binding but
instead it's homebrew thing. The reason being that there was
no generic binding around when it was defined and written.

It is possible to migrate it to generic pinconf, which would
be nice.

> I'm also surprised that you have to know multiple constants
> (mux register, input register, config register offsets) to select a
> particular pin. I would have expected that you could have one constant
> from which the driver is able to compute the other ones.

I might have merged this but I was younger then :-/

Now I don't like the looks of this. I think the DT typically shall not
store register offsets and bitfield information. I think doing
so is taking away the responsibility of the driver knowing the
hardware and moving too close to Open Firmware.

pinctrl-single does store such things, the reason being that
the OMAP people did not even have a datasheet for these regs,
all they get is datafiles from the ASIC engineers, so there is
nothing sensible to encode in the driver in C. But I'm reluctant
also about this driver.

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: DT include files

[Shawn Guo]

On Mon, Jan 13, 2014 at 12:16:10AM +0100, Linus Walleij wrote:
> > I'm also surprised that you have to know multiple constants
> > (mux register, input register, config register offsets) to select a
> > particular pin. I would have expected that you could have one constant
> > from which the driver is able to compute the other ones.
> 
> I might have merged this but I was younger then :-/
> 
> Now I don't like the looks of this. I think the DT typically shall not
> store register offsets and bitfield information. I think doing
> so is taking away the responsibility of the driver knowing the
> hardware and moving too close to Open Firmware.

We only have data in device tree, and drivers, at least imx pinctrl
driver, still need to know hardware for how to apply these data into
hardware.

As I just replied to Arnd, all the motivation of having these data in
device tree is to release kernel from being bloated by those huge mount
of data due to the complexity and flexibility of IMX pin controller.

Shawn

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: DT include files

[Shawn Guo]

On Sun, Jan 12, 2014 at 09:21:19PM +0100, Arnd Bergmann wrote:
> I was under the impression that the generic pinctrl binding was designed
> in a way to let you assign labels to each possible (reasonable)
> configuration so you didn't have get to this level of detail at the
> driver.

The generic part of pinctrl binding only covers the procedure how a
client device get its pinctrl state configuration from a pin controller
node.  That's what we defined in bindings/pinctrl/pinctrl-bindings.txt
and implemented in pinctrl core.  But the details of how a pinctrl state
configuration should be interpreted for a particular pin controller is
defined by individual pin controller binding like fsl,imx-pinctrl.txt,
and implemented in the pin controller driver like
drivers/pinctrl/pinctrl-imx.c.

> I'm also surprised that you have to know multiple constants
> (mux register, input register, config register offsets) to select a
> particular pin. I would have expected that you could have one constant
> from which the driver is able to compute the other ones.

That's what we do before.  We define a constant in the binding and have
the driver to maintain these data and look up the data using the
constant.  See commit below for imx6q example.

  d8fe357 pinctrl: pinctrl-imx: add imx6q pinctrl driver

The biggest problem with that approach is we have huge data to maintain
in the driver because of the complexity and flexibility of IMX pin
controller design.  And these data can not be init data.  Check that big
array of struct imx_pin_reg in commit above for what I'm talking about.
So when we build a v7 kernel image for IMX, we will have such big array
for each of these SoCs, imx50, imx51, imx53, imx6sl, imx6dl, imx6q, and
more to come.

That's why we went through the pain of breaking DT compatibility to move
all these data into device tree one year ago with the commit below.

  e164153 pinctrl: imx: move hard-coding data into device tree

Now kernel gets released from the floating and we do not even need to
touch kernel to add these data when new SoC support is added.

Shawn

Re: DT include files

[Heiko Stübner]

Hi Shawn,

did this topic get any final resolution - especially in terms of the pingrp.h 
header?

As I'm currently preparing two board files (imx50 and imx6sl) this discussion 
made me unsure if using the pin-group definitions is still the preferred style.


Thanks
Heiko

On Monday 13 January 2014 10:19:14 Shawn Guo wrote:
> On Sun, Jan 12, 2014 at 09:21:19PM +0100, Arnd Bergmann wrote:
> > I was under the impression that the generic pinctrl binding was designed
> > in a way to let you assign labels to each possible (reasonable)
> > configuration so you didn't have get to this level of detail at the
> > driver.
> 
> The generic part of pinctrl binding only covers the procedure how a
> client device get its pinctrl state configuration from a pin controller
> node.  That's what we defined in bindings/pinctrl/pinctrl-bindings.txt
> and implemented in pinctrl core.  But the details of how a pinctrl state
> configuration should be interpreted for a particular pin controller is
> defined by individual pin controller binding like fsl,imx-pinctrl.txt,
> and implemented in the pin controller driver like
> drivers/pinctrl/pinctrl-imx.c.
> 
> > I'm also surprised that you have to know multiple constants
> > (mux register, input register, config register offsets) to select a
> > particular pin. I would have expected that you could have one constant
> > from which the driver is able to compute the other ones.
> 
> That's what we do before.  We define a constant in the binding and have
> the driver to maintain these data and look up the data using the
> constant.  See commit below for imx6q example.
> 
>   d8fe357 pinctrl: pinctrl-imx: add imx6q pinctrl driver
> 
> The biggest problem with that approach is we have huge data to maintain
> in the driver because of the complexity and flexibility of IMX pin
> controller design.  And these data can not be init data.  Check that big
> array of struct imx_pin_reg in commit above for what I'm talking about.
> So when we build a v7 kernel image for IMX, we will have such big array
> for each of these SoCs, imx50, imx51, imx53, imx6sl, imx6dl, imx6q, and
> more to come.
> 
> That's why we went through the pain of breaking DT compatibility to move
> all these data into device tree one year ago with the commit below.
> 
>   e164153 pinctrl: imx: move hard-coding data into device tree
> 
> Now kernel gets released from the floating and we do not even need to
> touch kernel to add these data when new SoC support is added.
> 
> Shawn

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: DT include files

[Shawn Guo]

On Fri, Jan 24, 2014 at 09:02:09AM +0100, Heiko Stübner wrote:
> Hi Shawn,
> 
> did this topic get any final resolution - especially in terms of the pingrp.h 
> header?
> 
> As I'm currently preparing two board files (imx50 and imx6sl) this discussion 
> made me unsure if using the pin-group definitions is still the preferred style.

Yes, we need to get a conclusion ASAP.  The whole imx-dt-3.14 pull
request has been held for a few weeks because of that, and I'm asking
for direction there [1].

Shawn

[1] http://thread.gmane.org/gmane.linux.drivers.devicetree/58685/focus=296761

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html