diff mbox series

[RFC,3/4] dtoc: add support for generate stuct udevice_id

Message ID 20200619211140.5081-4-walter.lozano@collabora.com
State New
Headers show
Series drivers: footprint reduction proposal | expand

Commit Message

Walter Lozano June 19, 2020, 9:11 p.m. UTC
Based on several reports there is an increasing concern in the impact
of adding additional features to drivers based on compatible strings.
A good example of this situation is found in [1].

In order to reduce this impact and as an initial step for further
reduction, propose a new way to declare compatible strings, which allows
to only include the useful ones.

The idea is to define compatible strings in a way to be easily parsed by
dtoc, which will be responsible to build struct udevice_id [] based on
the compatible strings present in the dtb.

Additional features can be easily added, such as define constants
depending on the presence of compatible strings, which allows to enable
code blocks only in such cases without the need of adding additional
configuration options.

[1] http://patchwork.ozlabs.org/project/uboot/patch/20200525202429.2146-1-agust at denx.de/

Signed-off-by: Walter Lozano <walter.lozano at collabora.com>
---
 tools/dtoc/dtb_platdata.py | 32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

Comments

Simon Glass July 26, 2020, 2:53 p.m. UTC | #1
Hi Walter,

On Tue, 7 Jul 2020 at 08:08, Walter Lozano <walter.lozano@collabora.com> wrote:
>

> Hi Simon

>

> On 6/7/20 16:21, Simon Glass wrote:

> > Hi Walter,

> >

> > On Fri, 19 Jun 2020 at 15:12, Walter Lozano <walter.lozano@collabora.com> wrote:

> >> Based on several reports there is an increasing concern in the impact

> >> of adding additional features to drivers based on compatible strings.

> >> A good example of this situation is found in [1].

> >>

> >> In order to reduce this impact and as an initial step for further

> >> reduction, propose a new way to declare compatible strings, which allows

> >> to only include the useful ones.

> > What are the useful ones?

>

> The useful ones would be those that are used by the selected DTB by the

> current configuration. The idea of this patch is to declare all the

> possible compatible strings in a way that dtoc can generate code for

> only those which are going to be used, and in this way avoid lots of

> #ifdef like the ones shows in

>

> http://patchwork.ozlabs.org/project/uboot/patch/20200525202429.2146-1-agust@denx.de/

>

>

> >> The idea is to define compatible strings in a way to be easily parsed by

> >> dtoc, which will be responsible to build struct udevice_id [] based on

> >> the compatible strings present in the dtb.

> >>

> >> Additional features can be easily added, such as define constants

> >> depending on the presence of compatible strings, which allows to enable

> >> code blocks only in such cases without the need of adding additional

> >> configuration options.

> >>

> >> [1] http://patchwork.ozlabs.org/project/uboot/patch/20200525202429.2146-1-agust@denx.de/

> >>

> >> Signed-off-by: Walter Lozano <walter.lozano@collabora.com>

> >> ---

> >>   tools/dtoc/dtb_platdata.py | 32 ++++++++++++++++++++++++++++++++

> >>   1 file changed, 32 insertions(+)

> > I think dtoc should be able to parse the compatible strings as they

> > are today - e.g. see the tiny-dm stuff.

>

>

> Yes, I agree. My idea is that dtoc parses compatible strings as they are

> today but also in this new way. The reason for this is to allow dtoc to

> generate the code to include the useful compatible strings. Of course,

> this only makes sense if the idea of generating the compatible string

> associated  code is accepted.

>

> What do you think?


I think this is useful and better than using #ifdef in the source code
for this sort of thing. We need a way to specify the driver_data value
as well, right?

Re naming, perhaps DT_COMPAT() might be better than COMPATIBLE()? Or
even a name that indicates that it is optional, like DT_OPT_COMPAT() ?

Regards,
Simon
Walter Lozano July 27, 2020, 2:16 a.m. UTC | #2
Hi Simon,

On 26/7/20 11:53, Simon Glass wrote:
> Hi Walter,

>

> On Tue, 7 Jul 2020 at 08:08, Walter Lozano <walter.lozano@collabora.com> wrote:

>> Hi Simon

>>

>> On 6/7/20 16:21, Simon Glass wrote:

>>> Hi Walter,

>>>

>>> On Fri, 19 Jun 2020 at 15:12, Walter Lozano <walter.lozano@collabora.com> wrote:

>>>> Based on several reports there is an increasing concern in the impact

>>>> of adding additional features to drivers based on compatible strings.

>>>> A good example of this situation is found in [1].

>>>>

>>>> In order to reduce this impact and as an initial step for further

>>>> reduction, propose a new way to declare compatible strings, which allows

>>>> to only include the useful ones.

>>> What are the useful ones?

>> The useful ones would be those that are used by the selected DTB by the

>> current configuration. The idea of this patch is to declare all the

>> possible compatible strings in a way that dtoc can generate code for

>> only those which are going to be used, and in this way avoid lots of

>> #ifdef like the ones shows in

>>

>> http://patchwork.ozlabs.org/project/uboot/patch/20200525202429.2146-1-agust@denx.de/

>>

>>

>>>> The idea is to define compatible strings in a way to be easily parsed by

>>>> dtoc, which will be responsible to build struct udevice_id [] based on

>>>> the compatible strings present in the dtb.

>>>>

>>>> Additional features can be easily added, such as define constants

>>>> depending on the presence of compatible strings, which allows to enable

>>>> code blocks only in such cases without the need of adding additional

>>>> configuration options.

>>>>

>>>> [1] http://patchwork.ozlabs.org/project/uboot/patch/20200525202429.2146-1-agust@denx.de/

>>>>

>>>> Signed-off-by: Walter Lozano <walter.lozano@collabora.com>

>>>> ---

>>>>    tools/dtoc/dtb_platdata.py | 32 ++++++++++++++++++++++++++++++++

>>>>    1 file changed, 32 insertions(+)

>>> I think dtoc should be able to parse the compatible strings as they

>>> are today - e.g. see the tiny-dm stuff.

>>

>> Yes, I agree. My idea is that dtoc parses compatible strings as they are

>> today but also in this new way. The reason for this is to allow dtoc to

>> generate the code to include the useful compatible strings. Of course,

>> this only makes sense if the idea of generating the compatible string

>> associated  code is accepted.

>>

>> What do you think?

> I think this is useful and better than using #ifdef in the source code

> for this sort of thing. We need a way to specify the driver_data value

> as well, right?


Yes, I agree, it is better than #ifdef and c/ould give us some extra 
functionality.

What doe you mean by driver_data value? Are you referring to the data 
field? like

static struct esdhc_soc_data usdhc_imx7d_data = {
         .flags = ESDHC_FLAG_USDHC | ESDHC_FLAG_STD_TUNING
                         | ESDHC_FLAG_HAVE_CAP1 | ESDHC_FLAG_HS200
                         | ESDHC_FLAG_HS400,
};

If that is the case, I was thinking in defining a constant when specific 
compatible strings are enabled by dtoc, based in the above case

#ifdef FSL_ESDHC_IMX_V2
static struct esdhc_soc_data usdhc_imx7d_data = {
         .flags = ESDHC_FLAG_USDHC | ESDHC_FLAG_STD_TUNING
                         | ESDHC_FLAG_HAVE_CAP1 | ESDHC_FLAG_HS200
                         | ESDHC_FLAG_HS400,
};
#endif

COMPATIBLE(FSL_ESDHC, "fsl,imx7d-usdhc", &usdhc_imx7d_data, FSL_ESDHC_IMX_V2)

So when dtoc parses COMPATIBLE and determines that compatible 
"fsl,imx7d-usdhc" should be added it also defines FSL_ESDHC_IMX_V2.

This is alsoAs I comment you in the tread about tiny-dm I think that we 
can save some space following your suggestions, and for instance implement


> Re naming, perhaps DT_COMPAT() might be better than COMPATIBLE()? Or

> even a name that indicates that it is optional, like DT_OPT_COMPAT() ?

>

I totally agree, naming is very important, and DT_COMPAT() is much better.

What I don't fully understand is what are the cases for DT_OPT_COMPAT(), 
could you please clarify?

Regards,

Walter
Simon Glass July 29, 2020, 2:42 a.m. UTC | #3
Hi Walter,

On Sun, 26 Jul 2020 at 20:16, Walter Lozano <walter.lozano@collabora.com> wrote:
>

> Hi Simon,

>

> On 26/7/20 11:53, Simon Glass wrote:

> > Hi Walter,

> >

> > On Tue, 7 Jul 2020 at 08:08, Walter Lozano <walter.lozano@collabora.com> wrote:

> >> Hi Simon

> >>

> >> On 6/7/20 16:21, Simon Glass wrote:

> >>> Hi Walter,

> >>>

> >>> On Fri, 19 Jun 2020 at 15:12, Walter Lozano <walter.lozano@collabora.com> wrote:

> >>>> Based on several reports there is an increasing concern in the impact

> >>>> of adding additional features to drivers based on compatible strings.

> >>>> A good example of this situation is found in [1].

> >>>>

> >>>> In order to reduce this impact and as an initial step for further

> >>>> reduction, propose a new way to declare compatible strings, which allows

> >>>> to only include the useful ones.

> >>> What are the useful ones?

> >> The useful ones would be those that are used by the selected DTB by the

> >> current configuration. The idea of this patch is to declare all the

> >> possible compatible strings in a way that dtoc can generate code for

> >> only those which are going to be used, and in this way avoid lots of

> >> #ifdef like the ones shows in

> >>

> >> http://patchwork.ozlabs.org/project/uboot/patch/20200525202429.2146-1-agust@denx.de/

> >>

> >>

> >>>> The idea is to define compatible strings in a way to be easily parsed by

> >>>> dtoc, which will be responsible to build struct udevice_id [] based on

> >>>> the compatible strings present in the dtb.

> >>>>

> >>>> Additional features can be easily added, such as define constants

> >>>> depending on the presence of compatible strings, which allows to enable

> >>>> code blocks only in such cases without the need of adding additional

> >>>> configuration options.

> >>>>

> >>>> [1] http://patchwork.ozlabs.org/project/uboot/patch/20200525202429.2146-1-agust@denx.de/

> >>>>

> >>>> Signed-off-by: Walter Lozano <walter.lozano@collabora.com>

> >>>> ---

> >>>>    tools/dtoc/dtb_platdata.py | 32 ++++++++++++++++++++++++++++++++

> >>>>    1 file changed, 32 insertions(+)

> >>> I think dtoc should be able to parse the compatible strings as they

> >>> are today - e.g. see the tiny-dm stuff.

> >>

> >> Yes, I agree. My idea is that dtoc parses compatible strings as they are

> >> today but also in this new way. The reason for this is to allow dtoc to

> >> generate the code to include the useful compatible strings. Of course,

> >> this only makes sense if the idea of generating the compatible string

> >> associated  code is accepted.

> >>

> >> What do you think?

> > I think this is useful and better than using #ifdef in the source code

> > for this sort of thing. We need a way to specify the driver_data value

> > as well, right?

>

> Yes, I agree, it is better than #ifdef and c/ould give us some extra

> functionality.

>

> What doe you mean by driver_data value? Are you referring to the data

> field? like

>

> static struct esdhc_soc_data usdhc_imx7d_data = {

>          .flags = ESDHC_FLAG_USDHC | ESDHC_FLAG_STD_TUNING

>                          | ESDHC_FLAG_HAVE_CAP1 | ESDHC_FLAG_HS200

>                          | ESDHC_FLAG_HS400,

> };

>


Actually I was talking about the .data member in struct udevice_id.

> If that is the case, I was thinking in defining a constant when specific

> compatible strings are enabled by dtoc, based in the above case

>

> #ifdef FSL_ESDHC_IMX_V2

> static struct esdhc_soc_data usdhc_imx7d_data = {

>          .flags = ESDHC_FLAG_USDHC | ESDHC_FLAG_STD_TUNING

>                          | ESDHC_FLAG_HAVE_CAP1 | ESDHC_FLAG_HS200

>                          | ESDHC_FLAG_HS400,

> };

> #endif

>

> COMPATIBLE(FSL_ESDHC, "fsl,imx7d-usdhc", &usdhc_imx7d_data, FSL_ESDHC_IMX_V2)

>

> So when dtoc parses COMPATIBLE and determines that compatible

> "fsl,imx7d-usdhc" should be added it also defines FSL_ESDHC_IMX_V2.


I think we can put that data in the dt-platdata.c file perhaps.

>

> This is alsoAs I comment you in the tread about tiny-dm I think that we

> can save some space following your suggestions, and for instance implement

>

>

> > Re naming, perhaps DT_COMPAT() might be better than COMPATIBLE()? Or

> > even a name that indicates that it is optional, like DT_OPT_COMPAT() ?

> >

> I totally agree, naming is very important, and DT_COMPAT() is much better.

>

> What I don't fully understand is what are the cases for DT_OPT_COMPAT(),

> could you please clarify?


It's just an alternative name, with OPT meaning optional. But I think
we can leave out the OPT.

Regards,
Simon
Walter Lozano July 29, 2020, 4 p.m. UTC | #4
Hi Simon,

On 28/7/20 23:42, Simon Glass wrote:
> Hi Walter,

>

> On Sun, 26 Jul 2020 at 20:16, Walter Lozano <walter.lozano@collabora.com> wrote:

>> Hi Simon,

>>

>> On 26/7/20 11:53, Simon Glass wrote:

>>> Hi Walter,

>>>

>>> On Tue, 7 Jul 2020 at 08:08, Walter Lozano <walter.lozano@collabora.com> wrote:

>>>> Hi Simon

>>>>

>>>> On 6/7/20 16:21, Simon Glass wrote:

>>>>> Hi Walter,

>>>>>

>>>>> On Fri, 19 Jun 2020 at 15:12, Walter Lozano <walter.lozano@collabora.com> wrote:

>>>>>> Based on several reports there is an increasing concern in the impact

>>>>>> of adding additional features to drivers based on compatible strings.

>>>>>> A good example of this situation is found in [1].

>>>>>>

>>>>>> In order to reduce this impact and as an initial step for further

>>>>>> reduction, propose a new way to declare compatible strings, which allows

>>>>>> to only include the useful ones.

>>>>> What are the useful ones?

>>>> The useful ones would be those that are used by the selected DTB by the

>>>> current configuration. The idea of this patch is to declare all the

>>>> possible compatible strings in a way that dtoc can generate code for

>>>> only those which are going to be used, and in this way avoid lots of

>>>> #ifdef like the ones shows in

>>>>

>>>> http://patchwork.ozlabs.org/project/uboot/patch/20200525202429.2146-1-agust@denx.de/

>>>>

>>>>

>>>>>> The idea is to define compatible strings in a way to be easily parsed by

>>>>>> dtoc, which will be responsible to build struct udevice_id [] based on

>>>>>> the compatible strings present in the dtb.

>>>>>>

>>>>>> Additional features can be easily added, such as define constants

>>>>>> depending on the presence of compatible strings, which allows to enable

>>>>>> code blocks only in such cases without the need of adding additional

>>>>>> configuration options.

>>>>>>

>>>>>> [1] http://patchwork.ozlabs.org/project/uboot/patch/20200525202429.2146-1-agust@denx.de/

>>>>>>

>>>>>> Signed-off-by: Walter Lozano <walter.lozano@collabora.com>

>>>>>> ---

>>>>>>     tools/dtoc/dtb_platdata.py | 32 ++++++++++++++++++++++++++++++++

>>>>>>     1 file changed, 32 insertions(+)

>>>>> I think dtoc should be able to parse the compatible strings as they

>>>>> are today - e.g. see the tiny-dm stuff.

>>>> Yes, I agree. My idea is that dtoc parses compatible strings as they are

>>>> today but also in this new way. The reason for this is to allow dtoc to

>>>> generate the code to include the useful compatible strings. Of course,

>>>> this only makes sense if the idea of generating the compatible string

>>>> associated  code is accepted.

>>>>

>>>> What do you think?

>>> I think this is useful and better than using #ifdef in the source code

>>> for this sort of thing. We need a way to specify the driver_data value

>>> as well, right?

>> Yes, I agree, it is better than #ifdef and c/ould give us some extra

>> functionality.

>>

>> What doe you mean by driver_data value? Are you referring to the data

>> field? like

>>

>> static struct esdhc_soc_data usdhc_imx7d_data = {

>>           .flags = ESDHC_FLAG_USDHC | ESDHC_FLAG_STD_TUNING

>>                           | ESDHC_FLAG_HAVE_CAP1 | ESDHC_FLAG_HS200

>>                           | ESDHC_FLAG_HS400,

>> };

>>

> Actually I was talking about the .data member in struct udevice_id.

So my example is correct, as usdhc_imx7d_data is the value for .data in 
one case as shown bellow.
>> If that is the case, I was thinking in defining a constant when specific

>> compatible strings are enabled by dtoc, based in the above case

>>

>> #ifdef FSL_ESDHC_IMX_V2

>> static struct esdhc_soc_data usdhc_imx7d_data = {

>>           .flags = ESDHC_FLAG_USDHC | ESDHC_FLAG_STD_TUNING

>>                           | ESDHC_FLAG_HAVE_CAP1 | ESDHC_FLAG_HS200

>>                           | ESDHC_FLAG_HS400,

>> };

>> #endif

>>

>> COMPATIBLE(FSL_ESDHC, "fsl,imx7d-usdhc", &usdhc_imx7d_data, FSL_ESDHC_IMX_V2)

>>

>> So when dtoc parses COMPATIBLE and determines that compatible

>> "fsl,imx7d-usdhc" should be added it also defines FSL_ESDHC_IMX_V2.

> I think we can put that data in the dt-platdata.c file perhaps.


I thought the same at the beginning, but then I changed my mind, because

1- in order to work dt-platdata.c will need to include several different 
.h, in this example, only for fsl_esdhc_imx to work, we will need to 
include fsl_esdhc_imx.h where all the flags are defined.

2- it case we use #define to avoid having to include several different 
.h probably the errors will be more difficult to catch/debug

What do you think?

>

>> This is alsoAs I comment you in the tread about tiny-dm I think that we

>> can save some space following your suggestions, and for instance implement

>>

>>

>>> Re naming, perhaps DT_COMPAT() might be better than COMPATIBLE()? Or

>>> even a name that indicates that it is optional, like DT_OPT_COMPAT() ?

>>>

>> I totally agree, naming is very important, and DT_COMPAT() is much better.

>>

>> What I don't fully understand is what are the cases for DT_OPT_COMPAT(),

>> could you please clarify?

> It's just an alternative name, with OPT meaning optional. But I think

> we can leave out the OPT.


Thanks for clarifying.

Regards,

Walter
Simon Glass Aug. 7, 2020, 4:23 p.m. UTC | #5
Hi Walter,

On Wed, 29 Jul 2020 at 10:00, Walter Lozano <walter.lozano@collabora.com> wrote:
>

> Hi Simon,

>

> On 28/7/20 23:42, Simon Glass wrote:

> > Hi Walter,

> >

> > On Sun, 26 Jul 2020 at 20:16, Walter Lozano <walter.lozano@collabora.com> wrote:

> >> Hi Simon,

> >>

> >> On 26/7/20 11:53, Simon Glass wrote:

> >>> Hi Walter,

> >>>

> >>> On Tue, 7 Jul 2020 at 08:08, Walter Lozano <walter.lozano@collabora.com> wrote:

> >>>> Hi Simon

> >>>>

> >>>> On 6/7/20 16:21, Simon Glass wrote:

> >>>>> Hi Walter,

> >>>>>

> >>>>> On Fri, 19 Jun 2020 at 15:12, Walter Lozano <walter.lozano@collabora.com> wrote:

> >>>>>> Based on several reports there is an increasing concern in the impact

> >>>>>> of adding additional features to drivers based on compatible strings.

> >>>>>> A good example of this situation is found in [1].

> >>>>>>

> >>>>>> In order to reduce this impact and as an initial step for further

> >>>>>> reduction, propose a new way to declare compatible strings, which allows

> >>>>>> to only include the useful ones.

> >>>>> What are the useful ones?

> >>>> The useful ones would be those that are used by the selected DTB by the

> >>>> current configuration. The idea of this patch is to declare all the

> >>>> possible compatible strings in a way that dtoc can generate code for

> >>>> only those which are going to be used, and in this way avoid lots of

> >>>> #ifdef like the ones shows in

> >>>>

> >>>> http://patchwork.ozlabs.org/project/uboot/patch/20200525202429.2146-1-agust@denx.de/

> >>>>

> >>>>

> >>>>>> The idea is to define compatible strings in a way to be easily parsed by

> >>>>>> dtoc, which will be responsible to build struct udevice_id [] based on

> >>>>>> the compatible strings present in the dtb.

> >>>>>>

> >>>>>> Additional features can be easily added, such as define constants

> >>>>>> depending on the presence of compatible strings, which allows to enable

> >>>>>> code blocks only in such cases without the need of adding additional

> >>>>>> configuration options.

> >>>>>>

> >>>>>> [1] http://patchwork.ozlabs.org/project/uboot/patch/20200525202429.2146-1-agust@denx.de/

> >>>>>>

> >>>>>> Signed-off-by: Walter Lozano <walter.lozano@collabora.com>

> >>>>>> ---

> >>>>>>     tools/dtoc/dtb_platdata.py | 32 ++++++++++++++++++++++++++++++++

> >>>>>>     1 file changed, 32 insertions(+)

> >>>>> I think dtoc should be able to parse the compatible strings as they

> >>>>> are today - e.g. see the tiny-dm stuff.

> >>>> Yes, I agree. My idea is that dtoc parses compatible strings as they are

> >>>> today but also in this new way. The reason for this is to allow dtoc to

> >>>> generate the code to include the useful compatible strings. Of course,

> >>>> this only makes sense if the idea of generating the compatible string

> >>>> associated  code is accepted.

> >>>>

> >>>> What do you think?

> >>> I think this is useful and better than using #ifdef in the source code

> >>> for this sort of thing. We need a way to specify the driver_data value

> >>> as well, right?

> >> Yes, I agree, it is better than #ifdef and c/ould give us some extra

> >> functionality.

> >>

> >> What doe you mean by driver_data value? Are you referring to the data

> >> field? like

> >>

> >> static struct esdhc_soc_data usdhc_imx7d_data = {

> >>           .flags = ESDHC_FLAG_USDHC | ESDHC_FLAG_STD_TUNING

> >>                           | ESDHC_FLAG_HAVE_CAP1 | ESDHC_FLAG_HS200

> >>                           | ESDHC_FLAG_HS400,

> >> };

> >>

> > Actually I was talking about the .data member in struct udevice_id.

> So my example is correct, as usdhc_imx7d_data is the value for .data in

> one case as shown bellow.

> >> If that is the case, I was thinking in defining a constant when specific

> >> compatible strings are enabled by dtoc, based in the above case

> >>

> >> #ifdef FSL_ESDHC_IMX_V2

> >> static struct esdhc_soc_data usdhc_imx7d_data = {

> >>           .flags = ESDHC_FLAG_USDHC | ESDHC_FLAG_STD_TUNING

> >>                           | ESDHC_FLAG_HAVE_CAP1 | ESDHC_FLAG_HS200

> >>                           | ESDHC_FLAG_HS400,

> >> };

> >> #endif

> >>

> >> COMPATIBLE(FSL_ESDHC, "fsl,imx7d-usdhc", &usdhc_imx7d_data, FSL_ESDHC_IMX_V2)

> >>

> >> So when dtoc parses COMPATIBLE and determines that compatible

> >> "fsl,imx7d-usdhc" should be added it also defines FSL_ESDHC_IMX_V2.

> > I think we can put that data in the dt-platdata.c file perhaps.

>

> I thought the same at the beginning, but then I changed my mind, because

>

> 1- in order to work dt-platdata.c will need to include several different

> .h, in this example, only for fsl_esdhc_imx to work, we will need to

> include fsl_esdhc_imx.h where all the flags are defined.


Yes I hit that problem with the tiny-dm experiment and ended up adding
a macro to specify the header.

Do you need FSL_ESDHC_IMX_V2? Is it just to avoid a warning about
usdhc_imx7d_data not being used? If so, we could use _maybe_unused

>

> 2- it case we use #define to avoid having to include several different

> .h probably the errors will be more difficult to catch/debug


Yes we would have to include the real header, not just copy bits out of it.
>

> What do you think?


I'm not sure overall. On the one hand I don't really like hiding C
code inside macros. On the other, it avoids the horrible manual
#ifdefs. So on balance I think your idea is the best approach. We can
always refine it later and it is easier to iterate on this sort of
thing if it is actually being used by some boards.


>

> >

> >> This is alsoAs I comment you in the tread about tiny-dm I think that we

> >> can save some space following your suggestions, and for instance implement

> >>

> >>

> >>> Re naming, perhaps DT_COMPAT() might be better than COMPATIBLE()? Or

> >>> even a name that indicates that it is optional, like DT_OPT_COMPAT() ?

> >>>

> >> I totally agree, naming is very important, and DT_COMPAT() is much better.

> >>

> >> What I don't fully understand is what are the cases for DT_OPT_COMPAT(),

> >> could you please clarify?

> > It's just an alternative name, with OPT meaning optional. But I think

> > we can leave out the OPT.

>

> Thanks for clarifying.


Regards,
SImon
Walter Lozano Aug. 7, 2020, 5:23 p.m. UTC | #6
Hi Simon

On 7/8/20 13:23, Simon Glass wrote:
> Hi Walter,

>

> On Wed, 29 Jul 2020 at 10:00, Walter Lozano <walter.lozano@collabora.com> wrote:

>> Hi Simon,

>>

>> On 28/7/20 23:42, Simon Glass wrote:

>>> Hi Walter,

>>>

>>> On Sun, 26 Jul 2020 at 20:16, Walter Lozano <walter.lozano@collabora.com> wrote:

>>>> Hi Simon,

>>>>

>>>> On 26/7/20 11:53, Simon Glass wrote:

>>>>> Hi Walter,

>>>>>

>>>>> On Tue, 7 Jul 2020 at 08:08, Walter Lozano <walter.lozano@collabora.com> wrote:

>>>>>> Hi Simon

>>>>>>

>>>>>> On 6/7/20 16:21, Simon Glass wrote:

>>>>>>> Hi Walter,

>>>>>>>

>>>>>>> On Fri, 19 Jun 2020 at 15:12, Walter Lozano <walter.lozano@collabora.com> wrote:

>>>>>>>> Based on several reports there is an increasing concern in the impact

>>>>>>>> of adding additional features to drivers based on compatible strings.

>>>>>>>> A good example of this situation is found in [1].

>>>>>>>>

>>>>>>>> In order to reduce this impact and as an initial step for further

>>>>>>>> reduction, propose a new way to declare compatible strings, which allows

>>>>>>>> to only include the useful ones.

>>>>>>> What are the useful ones?

>>>>>> The useful ones would be those that are used by the selected DTB by the

>>>>>> current configuration. The idea of this patch is to declare all the

>>>>>> possible compatible strings in a way that dtoc can generate code for

>>>>>> only those which are going to be used, and in this way avoid lots of

>>>>>> #ifdef like the ones shows in

>>>>>>

>>>>>> http://patchwork.ozlabs.org/project/uboot/patch/20200525202429.2146-1-agust@denx.de/

>>>>>>

>>>>>>

>>>>>>>> The idea is to define compatible strings in a way to be easily parsed by

>>>>>>>> dtoc, which will be responsible to build struct udevice_id [] based on

>>>>>>>> the compatible strings present in the dtb.

>>>>>>>>

>>>>>>>> Additional features can be easily added, such as define constants

>>>>>>>> depending on the presence of compatible strings, which allows to enable

>>>>>>>> code blocks only in such cases without the need of adding additional

>>>>>>>> configuration options.

>>>>>>>>

>>>>>>>> [1] http://patchwork.ozlabs.org/project/uboot/patch/20200525202429.2146-1-agust@denx.de/

>>>>>>>>

>>>>>>>> Signed-off-by: Walter Lozano <walter.lozano@collabora.com>

>>>>>>>> ---

>>>>>>>>      tools/dtoc/dtb_platdata.py | 32 ++++++++++++++++++++++++++++++++

>>>>>>>>      1 file changed, 32 insertions(+)

>>>>>>> I think dtoc should be able to parse the compatible strings as they

>>>>>>> are today - e.g. see the tiny-dm stuff.

>>>>>> Yes, I agree. My idea is that dtoc parses compatible strings as they are

>>>>>> today but also in this new way. The reason for this is to allow dtoc to

>>>>>> generate the code to include the useful compatible strings. Of course,

>>>>>> this only makes sense if the idea of generating the compatible string

>>>>>> associated  code is accepted.

>>>>>>

>>>>>> What do you think?

>>>>> I think this is useful and better than using #ifdef in the source code

>>>>> for this sort of thing. We need a way to specify the driver_data value

>>>>> as well, right?

>>>> Yes, I agree, it is better than #ifdef and c/ould give us some extra

>>>> functionality.

>>>>

>>>> What doe you mean by driver_data value? Are you referring to the data

>>>> field? like

>>>>

>>>> static struct esdhc_soc_data usdhc_imx7d_data = {

>>>>            .flags = ESDHC_FLAG_USDHC | ESDHC_FLAG_STD_TUNING

>>>>                            | ESDHC_FLAG_HAVE_CAP1 | ESDHC_FLAG_HS200

>>>>                            | ESDHC_FLAG_HS400,

>>>> };

>>>>

>>> Actually I was talking about the .data member in struct udevice_id.

>> So my example is correct, as usdhc_imx7d_data is the value for .data in

>> one case as shown bellow.

>>>> If that is the case, I was thinking in defining a constant when specific

>>>> compatible strings are enabled by dtoc, based in the above case

>>>>

>>>> #ifdef FSL_ESDHC_IMX_V2

>>>> static struct esdhc_soc_data usdhc_imx7d_data = {

>>>>            .flags = ESDHC_FLAG_USDHC | ESDHC_FLAG_STD_TUNING

>>>>                            | ESDHC_FLAG_HAVE_CAP1 | ESDHC_FLAG_HS200

>>>>                            | ESDHC_FLAG_HS400,

>>>> };

>>>> #endif

>>>>

>>>> COMPATIBLE(FSL_ESDHC, "fsl,imx7d-usdhc", &usdhc_imx7d_data, FSL_ESDHC_IMX_V2)

>>>>

>>>> So when dtoc parses COMPATIBLE and determines that compatible

>>>> "fsl,imx7d-usdhc" should be added it also defines FSL_ESDHC_IMX_V2.

>>> I think we can put that data in the dt-platdata.c file perhaps.

>> I thought the same at the beginning, but then I changed my mind, because

>>

>> 1- in order to work dt-platdata.c will need to include several different

>> .h, in this example, only for fsl_esdhc_imx to work, we will need to

>> include fsl_esdhc_imx.h where all the flags are defined.

> Yes I hit that problem with the tiny-dm experiment and ended up adding

> a macro to specify the header.


I haven't seen that. I will check it. Thanks.


> Do you need FSL_ESDHC_IMX_V2? Is it just to avoid a warning about

> usdhc_imx7d_data not being used? If so, we could use _maybe_unused


Well, it is not that I really need it, but I try to give the possibility 
to add some #ifdef or similar based on compatible strings, the 
usdhc_imx7d_data was just an example. A more interesting example could 
be some code that makes sense only on specific "compatible string" cases 
and using #ifdef or if would save some bytes in other cases.

>> 2- it case we use #define to avoid having to include several different

>> .h probably the errors will be more difficult to catch/debug

> Yes we would have to include the real header, not just copy bits out of it.


Yes, for that reason I feel it could lead to more issues than benefits. 
However, it is only a personal opinion, I'm not completely sure.


>> What do you think?

> I'm not sure overall. On the one hand I don't really like hiding C

> code inside macros. On the other, it avoids the horrible manual

> #ifdefs. So on balance I think your idea is the best approach. We can

> always refine it later and it is easier to iterate on this sort of

> thing if it is actually being used by some boards.



It is exactly what I feel, we need to find the best balance here, and it 
always easy to improve it if this is used by some boards.

>>>> This is alsoAs I comment you in the tread about tiny-dm I think that we

>>>> can save some space following your suggestions, and for instance implement

>>>>

>>>>

>>>>> Re naming, perhaps DT_COMPAT() might be better than COMPATIBLE()? Or

>>>>> even a name that indicates that it is optional, like DT_OPT_COMPAT() ?

>>>>>

>>>> I totally agree, naming is very important, and DT_COMPAT() is much better.

>>>>

>>>> What I don't fully understand is what are the cases for DT_OPT_COMPAT(),

>>>> could you please clarify?

>>> It's just an alternative name, with OPT meaning optional. But I think

>>> we can leave out the OPT.

>> Thanks for clarifying.


Thanks for your review and comments.

BTW, as this work is based in some of the improvements you developed for 
tiny-dm, I was wondering what are your plans regarding it.

Regards,

Walter
Simon Glass Sept. 7, 2020, 1:44 a.m. UTC | #7
Hi Walter,

On Fri, 7 Aug 2020 at 11:23, Walter Lozano <walter.lozano@collabora.com> wrote:
>

> Hi Simon

>

> On 7/8/20 13:23, Simon Glass wrote:

> > Hi Walter,

> >

> > On Wed, 29 Jul 2020 at 10:00, Walter Lozano <walter.lozano@collabora.com> wrote:

> >> Hi Simon,

> >>

> >> On 28/7/20 23:42, Simon Glass wrote:

> >>> Hi Walter,

> >>>

> >>> On Sun, 26 Jul 2020 at 20:16, Walter Lozano <walter.lozano@collabora.com> wrote:

> >>>> Hi Simon,

> >>>>

> >>>> On 26/7/20 11:53, Simon Glass wrote:

> >>>>> Hi Walter,

> >>>>>

> >>>>> On Tue, 7 Jul 2020 at 08:08, Walter Lozano <walter.lozano@collabora.com> wrote:

> >>>>>> Hi Simon

> >>>>>>

> >>>>>> On 6/7/20 16:21, Simon Glass wrote:

> >>>>>>> Hi Walter,

> >>>>>>>

> >>>>>>> On Fri, 19 Jun 2020 at 15:12, Walter Lozano <walter.lozano@collabora.com> wrote:

> >>>>>>>> Based on several reports there is an increasing concern in the impact

> >>>>>>>> of adding additional features to drivers based on compatible strings.

> >>>>>>>> A good example of this situation is found in [1].

> >>>>>>>>

> >>>>>>>> In order to reduce this impact and as an initial step for further

> >>>>>>>> reduction, propose a new way to declare compatible strings, which allows

> >>>>>>>> to only include the useful ones.

> >>>>>>> What are the useful ones?

> >>>>>> The useful ones would be those that are used by the selected DTB by the

> >>>>>> current configuration. The idea of this patch is to declare all the

> >>>>>> possible compatible strings in a way that dtoc can generate code for

> >>>>>> only those which are going to be used, and in this way avoid lots of

> >>>>>> #ifdef like the ones shows in

> >>>>>>

> >>>>>> http://patchwork.ozlabs.org/project/uboot/patch/20200525202429.2146-1-agust@denx.de/

> >>>>>>

> >>>>>>

> >>>>>>>> The idea is to define compatible strings in a way to be easily parsed by

> >>>>>>>> dtoc, which will be responsible to build struct udevice_id [] based on

> >>>>>>>> the compatible strings present in the dtb.

> >>>>>>>>

> >>>>>>>> Additional features can be easily added, such as define constants

> >>>>>>>> depending on the presence of compatible strings, which allows to enable

> >>>>>>>> code blocks only in such cases without the need of adding additional

> >>>>>>>> configuration options.

> >>>>>>>>

> >>>>>>>> [1] http://patchwork.ozlabs.org/project/uboot/patch/20200525202429.2146-1-agust@denx.de/

> >>>>>>>>

> >>>>>>>> Signed-off-by: Walter Lozano <walter.lozano@collabora.com>

> >>>>>>>> ---

> >>>>>>>>      tools/dtoc/dtb_platdata.py | 32 ++++++++++++++++++++++++++++++++

> >>>>>>>>      1 file changed, 32 insertions(+)

> >>>>>>> I think dtoc should be able to parse the compatible strings as they

> >>>>>>> are today - e.g. see the tiny-dm stuff.

> >>>>>> Yes, I agree. My idea is that dtoc parses compatible strings as they are

> >>>>>> today but also in this new way. The reason for this is to allow dtoc to

> >>>>>> generate the code to include the useful compatible strings. Of course,

> >>>>>> this only makes sense if the idea of generating the compatible string

> >>>>>> associated  code is accepted.

> >>>>>>

> >>>>>> What do you think?

> >>>>> I think this is useful and better than using #ifdef in the source code

> >>>>> for this sort of thing. We need a way to specify the driver_data value

> >>>>> as well, right?

> >>>> Yes, I agree, it is better than #ifdef and c/ould give us some extra

> >>>> functionality.

> >>>>

> >>>> What doe you mean by driver_data value? Are you referring to the data

> >>>> field? like

> >>>>

> >>>> static struct esdhc_soc_data usdhc_imx7d_data = {

> >>>>            .flags = ESDHC_FLAG_USDHC | ESDHC_FLAG_STD_TUNING

> >>>>                            | ESDHC_FLAG_HAVE_CAP1 | ESDHC_FLAG_HS200

> >>>>                            | ESDHC_FLAG_HS400,

> >>>> };

> >>>>

> >>> Actually I was talking about the .data member in struct udevice_id.

> >> So my example is correct, as usdhc_imx7d_data is the value for .data in

> >> one case as shown bellow.

> >>>> If that is the case, I was thinking in defining a constant when specific

> >>>> compatible strings are enabled by dtoc, based in the above case

> >>>>

> >>>> #ifdef FSL_ESDHC_IMX_V2

> >>>> static struct esdhc_soc_data usdhc_imx7d_data = {

> >>>>            .flags = ESDHC_FLAG_USDHC | ESDHC_FLAG_STD_TUNING

> >>>>                            | ESDHC_FLAG_HAVE_CAP1 | ESDHC_FLAG_HS200

> >>>>                            | ESDHC_FLAG_HS400,

> >>>> };

> >>>> #endif

> >>>>

> >>>> COMPATIBLE(FSL_ESDHC, "fsl,imx7d-usdhc", &usdhc_imx7d_data, FSL_ESDHC_IMX_V2)

> >>>>

> >>>> So when dtoc parses COMPATIBLE and determines that compatible

> >>>> "fsl,imx7d-usdhc" should be added it also defines FSL_ESDHC_IMX_V2.

> >>> I think we can put that data in the dt-platdata.c file perhaps.

> >> I thought the same at the beginning, but then I changed my mind, because

> >>

> >> 1- in order to work dt-platdata.c will need to include several different

> >> .h, in this example, only for fsl_esdhc_imx to work, we will need to

> >> include fsl_esdhc_imx.h where all the flags are defined.

> > Yes I hit that problem with the tiny-dm experiment and ended up adding

> > a macro to specify the header.

>

> I haven't seen that. I will check it. Thanks.

>

>

> > Do you need FSL_ESDHC_IMX_V2? Is it just to avoid a warning about

> > usdhc_imx7d_data not being used? If so, we could use _maybe_unused

>

> Well, it is not that I really need it, but I try to give the possibility

> to add some #ifdef or similar based on compatible strings, the

> usdhc_imx7d_data was just an example. A more interesting example could

> be some code that makes sense only on specific "compatible string" cases

> and using #ifdef or if would save some bytes in other cases.

>

> >> 2- it case we use #define to avoid having to include several different

> >> .h probably the errors will be more difficult to catch/debug

> > Yes we would have to include the real header, not just copy bits out of it.

>

> Yes, for that reason I feel it could lead to more issues than benefits.

> However, it is only a personal opinion, I'm not completely sure.

>

>

> >> What do you think?

> > I'm not sure overall. On the one hand I don't really like hiding C

> > code inside macros. On the other, it avoids the horrible manual

> > #ifdefs. So on balance I think your idea is the best approach. We can

> > always refine it later and it is easier to iterate on this sort of

> > thing if it is actually being used by some boards.

>

>

> It is exactly what I feel, we need to find the best balance here, and it

> always easy to improve it if this is used by some boards.

>

> >>>> This is alsoAs I comment you in the tread about tiny-dm I think that we

> >>>> can save some space following your suggestions, and for instance implement

> >>>>

> >>>>

> >>>>> Re naming, perhaps DT_COMPAT() might be better than COMPATIBLE()? Or

> >>>>> even a name that indicates that it is optional, like DT_OPT_COMPAT() ?

> >>>>>

> >>>> I totally agree, naming is very important, and DT_COMPAT() is much better.

> >>>>

> >>>> What I don't fully understand is what are the cases for DT_OPT_COMPAT(),

> >>>> could you please clarify?

> >>> It's just an alternative name, with OPT meaning optional. But I think

> >>> we can leave out the OPT.

> >> Thanks for clarifying.

>

> Thanks for your review and comments.

>

> BTW, as this work is based in some of the improvements you developed for

> tiny-dm, I was wondering what are your plans regarding it.


I haven't had a lot of time recently. Don't let my side hold you up,
though. I am hoping to look at the platdata parent stuff soon.

Regards,
SImon
Walter Lozano Sept. 7, 2020, 7:10 p.m. UTC | #8
Hi Simon,

On 6/9/20 22:44, Simon Glass wrote:
> Hi Walter,

>

> On Fri, 7 Aug 2020 at 11:23, Walter Lozano <walter.lozano@collabora.com> wrote:

>> Hi Simon

>>

>> On 7/8/20 13:23, Simon Glass wrote:

>>> Hi Walter,

>>>

>>> On Wed, 29 Jul 2020 at 10:00, Walter Lozano <walter.lozano@collabora.com> wrote:

>>>> Hi Simon,

>>>>

>>>> On 28/7/20 23:42, Simon Glass wrote:

>>>>> Hi Walter,

>>>>>

>>>>> On Sun, 26 Jul 2020 at 20:16, Walter Lozano <walter.lozano@collabora.com> wrote:

>>>>>> Hi Simon,

>>>>>>

>>>>>> On 26/7/20 11:53, Simon Glass wrote:

>>>>>>> Hi Walter,

>>>>>>>

>>>>>>> On Tue, 7 Jul 2020 at 08:08, Walter Lozano <walter.lozano@collabora.com> wrote:

>>>>>>>> Hi Simon

>>>>>>>>

>>>>>>>> On 6/7/20 16:21, Simon Glass wrote:

>>>>>>>>> Hi Walter,

>>>>>>>>>

>>>>>>>>> On Fri, 19 Jun 2020 at 15:12, Walter Lozano <walter.lozano@collabora.com> wrote:

>>>>>>>>>> Based on several reports there is an increasing concern in the impact

>>>>>>>>>> of adding additional features to drivers based on compatible strings.

>>>>>>>>>> A good example of this situation is found in [1].

>>>>>>>>>>

>>>>>>>>>> In order to reduce this impact and as an initial step for further

>>>>>>>>>> reduction, propose a new way to declare compatible strings, which allows

>>>>>>>>>> to only include the useful ones.

>>>>>>>>> What are the useful ones?

>>>>>>>> The useful ones would be those that are used by the selected DTB by the

>>>>>>>> current configuration. The idea of this patch is to declare all the

>>>>>>>> possible compatible strings in a way that dtoc can generate code for

>>>>>>>> only those which are going to be used, and in this way avoid lots of

>>>>>>>> #ifdef like the ones shows in

>>>>>>>>

>>>>>>>> http://patchwork.ozlabs.org/project/uboot/patch/20200525202429.2146-1-agust@denx.de/

>>>>>>>>

>>>>>>>>

>>>>>>>>>> The idea is to define compatible strings in a way to be easily parsed by

>>>>>>>>>> dtoc, which will be responsible to build struct udevice_id [] based on

>>>>>>>>>> the compatible strings present in the dtb.

>>>>>>>>>>

>>>>>>>>>> Additional features can be easily added, such as define constants

>>>>>>>>>> depending on the presence of compatible strings, which allows to enable

>>>>>>>>>> code blocks only in such cases without the need of adding additional

>>>>>>>>>> configuration options.

>>>>>>>>>>

>>>>>>>>>> [1] http://patchwork.ozlabs.org/project/uboot/patch/20200525202429.2146-1-agust@denx.de/

>>>>>>>>>>

>>>>>>>>>> Signed-off-by: Walter Lozano <walter.lozano@collabora.com>

>>>>>>>>>> ---

>>>>>>>>>>       tools/dtoc/dtb_platdata.py | 32 ++++++++++++++++++++++++++++++++

>>>>>>>>>>       1 file changed, 32 insertions(+)

>>>>>>>>> I think dtoc should be able to parse the compatible strings as they

>>>>>>>>> are today - e.g. see the tiny-dm stuff.

>>>>>>>> Yes, I agree. My idea is that dtoc parses compatible strings as they are

>>>>>>>> today but also in this new way. The reason for this is to allow dtoc to

>>>>>>>> generate the code to include the useful compatible strings. Of course,

>>>>>>>> this only makes sense if the idea of generating the compatible string

>>>>>>>> associated  code is accepted.

>>>>>>>>

>>>>>>>> What do you think?

>>>>>>> I think this is useful and better than using #ifdef in the source code

>>>>>>> for this sort of thing. We need a way to specify the driver_data value

>>>>>>> as well, right?

>>>>>> Yes, I agree, it is better than #ifdef and c/ould give us some extra

>>>>>> functionality.

>>>>>>

>>>>>> What doe you mean by driver_data value? Are you referring to the data

>>>>>> field? like

>>>>>>

>>>>>> static struct esdhc_soc_data usdhc_imx7d_data = {

>>>>>>             .flags = ESDHC_FLAG_USDHC | ESDHC_FLAG_STD_TUNING

>>>>>>                             | ESDHC_FLAG_HAVE_CAP1 | ESDHC_FLAG_HS200

>>>>>>                             | ESDHC_FLAG_HS400,

>>>>>> };

>>>>>>

>>>>> Actually I was talking about the .data member in struct udevice_id.

>>>> So my example is correct, as usdhc_imx7d_data is the value for .data in

>>>> one case as shown bellow.

>>>>>> If that is the case, I was thinking in defining a constant when specific

>>>>>> compatible strings are enabled by dtoc, based in the above case

>>>>>>

>>>>>> #ifdef FSL_ESDHC_IMX_V2

>>>>>> static struct esdhc_soc_data usdhc_imx7d_data = {

>>>>>>             .flags = ESDHC_FLAG_USDHC | ESDHC_FLAG_STD_TUNING

>>>>>>                             | ESDHC_FLAG_HAVE_CAP1 | ESDHC_FLAG_HS200

>>>>>>                             | ESDHC_FLAG_HS400,

>>>>>> };

>>>>>> #endif

>>>>>>

>>>>>> COMPATIBLE(FSL_ESDHC, "fsl,imx7d-usdhc", &usdhc_imx7d_data, FSL_ESDHC_IMX_V2)

>>>>>>

>>>>>> So when dtoc parses COMPATIBLE and determines that compatible

>>>>>> "fsl,imx7d-usdhc" should be added it also defines FSL_ESDHC_IMX_V2.

>>>>> I think we can put that data in the dt-platdata.c file perhaps.

>>>> I thought the same at the beginning, but then I changed my mind, because

>>>>

>>>> 1- in order to work dt-platdata.c will need to include several different

>>>> .h, in this example, only for fsl_esdhc_imx to work, we will need to

>>>> include fsl_esdhc_imx.h where all the flags are defined.

>>> Yes I hit that problem with the tiny-dm experiment and ended up adding

>>> a macro to specify the header.

>> I haven't seen that. I will check it. Thanks.

>>

>>

>>> Do you need FSL_ESDHC_IMX_V2? Is it just to avoid a warning about

>>> usdhc_imx7d_data not being used? If so, we could use _maybe_unused

>> Well, it is not that I really need it, but I try to give the possibility

>> to add some #ifdef or similar based on compatible strings, the

>> usdhc_imx7d_data was just an example. A more interesting example could

>> be some code that makes sense only on specific "compatible string" cases

>> and using #ifdef or if would save some bytes in other cases.

>>

>>>> 2- it case we use #define to avoid having to include several different

>>>> .h probably the errors will be more difficult to catch/debug

>>> Yes we would have to include the real header, not just copy bits out of it.

>> Yes, for that reason I feel it could lead to more issues than benefits.

>> However, it is only a personal opinion, I'm not completely sure.

>>

>>

>>>> What do you think?

>>> I'm not sure overall. On the one hand I don't really like hiding C

>>> code inside macros. On the other, it avoids the horrible manual

>>> #ifdefs. So on balance I think your idea is the best approach. We can

>>> always refine it later and it is easier to iterate on this sort of

>>> thing if it is actually being used by some boards.

>>

>> It is exactly what I feel, we need to find the best balance here, and it

>> always easy to improve it if this is used by some boards.

>>

>>>>>> This is alsoAs I comment you in the tread about tiny-dm I think that we

>>>>>> can save some space following your suggestions, and for instance implement

>>>>>>

>>>>>>

>>>>>>> Re naming, perhaps DT_COMPAT() might be better than COMPATIBLE()? Or

>>>>>>> even a name that indicates that it is optional, like DT_OPT_COMPAT() ?

>>>>>>>

>>>>>> I totally agree, naming is very important, and DT_COMPAT() is much better.

>>>>>>

>>>>>> What I don't fully understand is what are the cases for DT_OPT_COMPAT(),

>>>>>> could you please clarify?

>>>>> It's just an alternative name, with OPT meaning optional. But I think

>>>>> we can leave out the OPT.

>>>> Thanks for clarifying.

>> Thanks for your review and comments.

>>

>> BTW, as this work is based in some of the improvements you developed for

>> tiny-dm, I was wondering what are your plans regarding it.

> I haven't had a lot of time recently. Don't let my side hold you up,

> though. I am hoping to look at the platdata parent stuff soon.



Thanks for the warning. I have planed to send a patch for this but I 
have also been a bit busy. However, I hope to have some time this week 
to prepare and send it.

Regards,

Walter
diff mbox series

Patch

diff --git a/tools/dtoc/dtb_platdata.py b/tools/dtoc/dtb_platdata.py
index d3fb4dcbf2..e199caf8c9 100644
--- a/tools/dtoc/dtb_platdata.py
+++ b/tools/dtoc/dtb_platdata.py
@@ -170,6 +170,9 @@  class DtbPlatdata(object):
             key: alias name
             value: node path
         _tiny_uclasses: List of uclass names that are marked as 'tiny'
+        _compatible_strings: Dict of compatible strings read from drivres
+            key: driver name
+            value: list of struct udevice_id
     """
     def __init__(self, dtb_fname, config_fname, include_disabled):
         self._fdt = None
@@ -188,6 +191,7 @@  class DtbPlatdata(object):
         self._tiny_uclasses = []
         self._of_match = {}
         self._compat_to_driver = {}
+        self._compatible_strings = {}
 
     def get_normalized_compat_name(self, node):
         compat_c, aliases_c = get_compat_name(node)
@@ -395,6 +399,15 @@  class DtbPlatdata(object):
             except:
                 pass
 
+        # find entries declared with the form
+        # COMPATIBLE(driver_name, compatible_string)
+        compatible_strings = re.findall('COMPATIBLE\(\s*(\w+)\s*,\s*(\S+)\s*\)', b)
+
+        for co in compatible_strings:
+            if not self._compatible_strings.get(co[0]):
+                self._compatible_strings[co[0]] = []
+            self._compatible_strings[co[0]].append(co)
+
     def scan_drivers(self):
         """Scan the driver folders to build a list of driver names and possible
         aliases
@@ -805,6 +818,23 @@  class DtbPlatdata(object):
         self.out(''.join(self.get_buf()))
         self.close_output()
 
+    def generate_compatible(self):
+        """Generates the struct udevice_id[] to be used in drivers
+
+        This writes C code to implement struct udevice_id[] based on
+        COMPATIBLE(driver_name, compatible) entries found in drivers.
+
+        Additionally this function can filter entries in order to avoid
+        adding those that are not present in DT.
+        """
+        self.out('#define COMPATIBLE(__driver_name, __compatible, ...)\n\n')
+        for vals in self._compatible_strings.values():
+            st = ''
+            for comp in vals:
+                st += '{.compatible = %s},\\\n' % (comp[1])
+            st += '{ /* sentinel */ },\\\n'
+            self.out('#define COMPATIBLE_LIST_%s { \\\n%s}\n' % (comp[0], st))
+
     def shrink(self):
         """Generate a shrunk version of DTB bases on valid drivers
 
@@ -895,6 +925,8 @@  def run_steps(args, dtb_file, config_file, include_disabled, output):
             plat.shrink()
         elif cmd == 'test_del_node':
             plat.test_del_node()
+        elif cmd == 'compatible':
+            plat.generate_compatible()
         else:
             raise ValueError("Unknown command '%s': (use: struct, platdata)" %
                              cmd)