Hi Walter, On Sun, 26 Jul 2020 at 20:16, Walter Lozano <walter.loz...@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.loz...@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.loz...@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-ag...@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-ag...@denx.de/ > >>>> > >>>> Signed-off-by: Walter Lozano <walter.loz...@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