re: CONFIG_IS_ENABLED vs IS_ENABLED

re: CONFIG_IS_ENABLED vs IS_ENABLED

[Troy Kisky]

Thanks Simon

I switched emails to get rid of the legalese.
Below are scripts to commit the changes sorted by
CONFIG_x variable. The only one I know causes a problem
is CONFIG_OF_LIVE because of

drivers/core/Makefile
obj-$(CONFIG_$(SPL_)OF_LIVE)

So, that config needs to keep using CONFIG_IS_ENABLED even
though SPL_OF_LIVE isn't in any Kconfig file.

Maybe something like
config SPL_OF_LIVE
       bool

can be added to a Kconfig to prevent the bad change.

git grep CONFIG_IS_ENABLED|sed -n -e
"s/\(CONFIG_IS_ENABLED([0-9a-zA-Z_]*)\)/\n\1\n/gp"| \
sed -n -r "s/CONFIG_IS_ENABLED\(([0-9a-zA-Z_]+)\)/\1/p" |sort -u|xargs -I
{} \
sh -c "git grep -E 'config [ST]PL_{}' | grep -q -E -w '[ST]PL_{}' || { git
grep -l 'CONFIG_IS_ENABLED({})' | \
xargs -IFile sh -c \"sed -i
\\\"s/CONFIG_IS_ENABLED({})/IS_ENABLED(CONFIG_{})/g\\\" File\" ; \
git commit -a -m\"CONFIG_{}: change CONFIG_IS_ENABLED to IS_ENABLED\"; }"

git grep -w IS_ENABLED|sed -n -e
"s/\(IS_ENABLED(CONFIG_[0-9a-zA-Z_]*)\)/\n\1\n/gp"| \
sed -n -r "s/IS_ENABLED\(CONFIG_([0-9a-zA-Z_]+)\)/\1/p" |sort -u|xargs -I
{} \
sh -c "git grep -E 'config [ST]PL_{}' | grep -q -E -w '[ST]PL_{}' &&  { git
grep -l 'IS_ENABLED(CONFIG_{})' | \
xargs -IFile sh -c \"sed -i
\\\"s/IS_ENABLED(CONFIG_{})/CONFIG_IS_ENABLED({})/g\\\" File\" ; \
git commit -a -m\"CONFIG_{}: change IS_ENABLED to CONFIG_IS_ENABLED\"; }"


I compile tested a few boards, but a thorough compile test would be good. I
hope the above helps someone get a few of the changes in mainline.

BR
Troy

Re: CONFIG_IS_ENABLED vs IS_ENABLED

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 1920 bytes --]

On Thu, Jan 26, 2023 at 09:26:18AM -0800, Troy Kisky wrote:
> Thanks Simon
> 
> I switched emails to get rid of the legalese.
> Below are scripts to commit the changes sorted by
> CONFIG_x variable. The only one I know causes a problem
> is CONFIG_OF_LIVE because of
> 
> drivers/core/Makefile
> obj-$(CONFIG_$(SPL_)OF_LIVE)
> 
> So, that config needs to keep using CONFIG_IS_ENABLED even
> though SPL_OF_LIVE isn't in any Kconfig file.
> 
> Maybe something like
> config SPL_OF_LIVE
>        bool
> 
> can be added to a Kconfig to prevent the bad change.
> 
> git grep CONFIG_IS_ENABLED|sed -n -e
> "s/\(CONFIG_IS_ENABLED([0-9a-zA-Z_]*)\)/\n\1\n/gp"| \
> sed -n -r "s/CONFIG_IS_ENABLED\(([0-9a-zA-Z_]+)\)/\1/p" |sort -u|xargs -I
> {} \
> sh -c "git grep -E 'config [ST]PL_{}' | grep -q -E -w '[ST]PL_{}' || { git
> grep -l 'CONFIG_IS_ENABLED({})' | \
> xargs -IFile sh -c \"sed -i
> \\\"s/CONFIG_IS_ENABLED({})/IS_ENABLED(CONFIG_{})/g\\\" File\" ; \
> git commit -a -m\"CONFIG_{}: change CONFIG_IS_ENABLED to IS_ENABLED\"; }"
> 
> git grep -w IS_ENABLED|sed -n -e
> "s/\(IS_ENABLED(CONFIG_[0-9a-zA-Z_]*)\)/\n\1\n/gp"| \
> sed -n -r "s/IS_ENABLED\(CONFIG_([0-9a-zA-Z_]+)\)/\1/p" |sort -u|xargs -I
> {} \
> sh -c "git grep -E 'config [ST]PL_{}' | grep -q -E -w '[ST]PL_{}' &&  { git
> grep -l 'IS_ENABLED(CONFIG_{})' | \
> xargs -IFile sh -c \"sed -i
> \\\"s/IS_ENABLED(CONFIG_{})/CONFIG_IS_ENABLED({})/g\\\" File\" ; \
> git commit -a -m\"CONFIG_{}: change IS_ENABLED to CONFIG_IS_ENABLED\"; }"
> 
> 
> I compile tested a few boards, but a thorough compile test would be good. I
> hope the above helps someone get a few of the changes in mainline.

Submitting a PR against https://github.com/u-boot/u-boot/ will trigger
an Azure CI run, and so a global build. And adding a test to CI to fail
on new introductions of this would be how to prevent further issues.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: CONFIG_IS_ENABLED vs IS_ENABLED

[Troy Kisky]

Thanks Tom,

I cleaned up the PR based on the CI results. Here's my current changes.

Author: Troy Kisky <troy.kisky@boundarydevices.com>
Date:   Fri Jan 27 11:03:11 2023 -0800

    dm: device-internal: use EVENT instead of DM_EVENT, because
event_notify is built for EVENT

    Signed-off-by: Troy Kisky <troy.kisky@boundarydevices.com>

diff --git a/include/dm/device-internal.h b/include/dm/device-internal.h
index f31c4702086..2e725aa9416 100644
--- a/include/dm/device-internal.h
+++ b/include/dm/device-internal.h
@@ -431,7 +431,7 @@ static inline void devres_release_all(struct udevice
*dev)

 static inline int device_notify(const struct udevice *dev, enum event_t
type)
 {
-#if CONFIG_IS_ENABLED(DM_EVENT)
+#if CONFIG_IS_ENABLED(EVENT)
        return event_notify(type, &dev, sizeof(dev));
 #else
        return 0;
______________________________

Here's the CI test scripts to prevent more occurrences.

{ { git grep 'obj-$(CONFIG_$(SPL_'|grep Makefile|sed -e "s/SPL_TPL_/SPL_/"|
\
sed -n -r 's/obj\-\$\(CONFIG_\$\(SPL_\)([0-9a-zA-Z_]+)\)/\n\{\1\}\n/gp'| \
sed -n -r 's/\{([0-9a-zA-Z_]+)\}/\1/p'; } ;\
{ git grep -E 'config [ST]PL_'|grep Kconfig| \
sed -n -r "s/config [ST]PL_([0-9a-zA-Z_]+)/\n\{\1\}\n/p" |
sed -n -r 's/\{([0-9a-zA-Z_]+)\}/\1/p'; } ; \
git grep -E 'CONFIG_IS_ENABLED\(CMD_'|sed -n -e
"s/\(CONFIG_IS_ENABLED(CMD_[0-9a-zA-Z_]*)\)/\n\1\n/gp"| \
sed -n -r "s/CONFIG_IS_ENABLED\((CMD_[0-9a-zA-Z_]+)\)/\1/p"; \
echo -e "\
BZIP2\n\
CONFIG_CLK\n\
EFI_DEVICE_PATH_TO_TEXT\n\
EFI_LOADER\n\
ERRNO_STR\n\
FOO\n\
GENERATE_SMBIOS_TABLE\n\
";\
} | sort -u >splvar.tmp


git grep CONFIG_IS_ENABLED|sed -n -e
"s/\(CONFIG_IS_ENABLED([0-9a-zA-Z_]*)\)/\n\1\n/gp"| \
sed -n -r "s/CONFIG_IS_ENABLED\(([0-9a-zA-Z_]+)\)/\1/p" |sort -u| comm -23
- splvar.tmp

git grep -w IS_ENABLED|sed -n -e
"s/\(IS_ENABLED(CONFIG_[0-9a-zA-Z_]*)\)/\n\1\n/gp"| \
sed -n -r "s/IS_ENABLED\(CONFIG_([0-9a-zA-Z_]+)\)/\1/p" |sort -u|grep -v
FOO| join - splvar.tmp
______________________


And the commit scripts to change current complaints.

git grep CONFIG_IS_ENABLED|sed -n -e
"s/\(CONFIG_IS_ENABLED([0-9a-zA-Z_]*)\)/\n\1\n/gp"| \
sed -n -r "s/CONFIG_IS_ENABLED\(([0-9a-zA-Z_]+)\)/\1/p" |sort -u| comm -23
- splvar.tmp|xargs -I {} \
sh -c "git grep -l 'CONFIG_IS_ENABLED({})' | \
xargs -IFile sh -c \"sed -i -e
\\\"s/CONFIG_IS_ENABLED({})/IS_ENABLED(CONFIG_{})/g\\\" File\" ; \
git commit -a -m\"CONFIG_{}: change CONFIG_IS_ENABLED to IS_ENABLED\";"

git grep -w IS_ENABLED|sed -n -e
"s/\(IS_ENABLED(CONFIG_[0-9a-zA-Z_]*)\)/\n\1\n/gp"| \
sed -n -r "s/IS_ENABLED\(CONFIG_([0-9a-zA-Z_]+)\)/\1/p" |sort -u|grep -v
FOO| \
join - splvar.tmp |xargs -I {} \
sh -c "git grep -l 'IS_ENABLED(CONFIG_{})' | \
xargs -IFile sh -c \"sed -i -e
\\\"s/\([^_]\)IS_ENABLED(CONFIG_{})/\1CONFIG_IS_ENABLED({})/g\\\" File\" ; \
git commit -a -m\"CONFIG_{}: change IS_ENABLED to CONFIG_IS_ENABLED\";"
__________

BR
Troy





On Thu, Jan 26, 2023 at 9:34 AM Tom Rini <trini@konsulko.com> wrote:

> On Thu, Jan 26, 2023 at 09:26:18AM -0800, Troy Kisky wrote:
> > Thanks Simon
> >
> > I switched emails to get rid of the legalese.
> > Below are scripts to commit the changes sorted by
> > CONFIG_x variable. The only one I know causes a problem
> > is CONFIG_OF_LIVE because of
> >
> > drivers/core/Makefile
> > obj-$(CONFIG_$(SPL_)OF_LIVE)
> >
> > So, that config needs to keep using CONFIG_IS_ENABLED even
> > though SPL_OF_LIVE isn't in any Kconfig file.
> >
> > Maybe something like
> > config SPL_OF_LIVE
> >        bool
> >
> > can be added to a Kconfig to prevent the bad change.
> >
> > git grep CONFIG_IS_ENABLED|sed -n -e
> > "s/\(CONFIG_IS_ENABLED([0-9a-zA-Z_]*)\)/\n\1\n/gp"| \
> > sed -n -r "s/CONFIG_IS_ENABLED\(([0-9a-zA-Z_]+)\)/\1/p" |sort -u|xargs -I
> > {} \
> > sh -c "git grep -E 'config [ST]PL_{}' | grep -q -E -w '[ST]PL_{}' || {
> git
> > grep -l 'CONFIG_IS_ENABLED({})' | \
> > xargs -IFile sh -c \"sed -i
> > \\\"s/CONFIG_IS_ENABLED({})/IS_ENABLED(CONFIG_{})/g\\\" File\" ; \
> > git commit -a -m\"CONFIG_{}: change CONFIG_IS_ENABLED to IS_ENABLED\"; }"
> >
> > git grep -w IS_ENABLED|sed -n -e
> > "s/\(IS_ENABLED(CONFIG_[0-9a-zA-Z_]*)\)/\n\1\n/gp"| \
> > sed -n -r "s/IS_ENABLED\(CONFIG_([0-9a-zA-Z_]+)\)/\1/p" |sort -u|xargs -I
> > {} \
> > sh -c "git grep -E 'config [ST]PL_{}' | grep -q -E -w '[ST]PL_{}' &&  {
> git
> > grep -l 'IS_ENABLED(CONFIG_{})' | \
> > xargs -IFile sh -c \"sed -i
> > \\\"s/IS_ENABLED(CONFIG_{})/CONFIG_IS_ENABLED({})/g\\\" File\" ; \
> > git commit -a -m\"CONFIG_{}: change IS_ENABLED to CONFIG_IS_ENABLED\"; }"
> >
> >
> > I compile tested a few boards, but a thorough compile test would be
> good. I
> > hope the above helps someone get a few of the changes in mainline.
>
> Submitting a PR against https://github.com/u-boot/u-boot/ will trigger
> an Azure CI run, and so a global build. And adding a test to CI to fail
> on new introductions of this would be how to prevent further issues.
>
> --
> Tom
>

Re: CONFIG_IS_ENABLED vs IS_ENABLED

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 1305 bytes --]

On Sat, Jan 28, 2023 at 09:25:54AM -0800, Troy Kisky wrote:
> Thanks Tom,
> 
> I cleaned up the PR based on the CI results. Here's my current changes.
> 
> Author: Troy Kisky <troy.kisky@boundarydevices.com>
> Date:   Fri Jan 27 11:03:11 2023 -0800
> 
>     dm: device-internal: use EVENT instead of DM_EVENT, because
> event_notify is built for EVENT
> 
>     Signed-off-by: Troy Kisky <troy.kisky@boundarydevices.com>
> 
> diff --git a/include/dm/device-internal.h b/include/dm/device-internal.h
> index f31c4702086..2e725aa9416 100644
> --- a/include/dm/device-internal.h
> +++ b/include/dm/device-internal.h
> @@ -431,7 +431,7 @@ static inline void devres_release_all(struct udevice
> *dev)
> 
>  static inline int device_notify(const struct udevice *dev, enum event_t
> type)
>  {
> -#if CONFIG_IS_ENABLED(DM_EVENT)
> +#if CONFIG_IS_ENABLED(EVENT)
>         return event_notify(type, &dev, sizeof(dev));
>  #else
>         return 0;

Given 448e2b6327d0 ("event: Correct dependencies on the EVENT
framework") I'm a little worried about this change here and want to be
extra sure it doesn't break something inadvertently.  Aside from that,
are you able to post your series?  And incorporate your checking script
in to .gitlab-ci.yml / .azure-pipline.yml ?

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: CONFIG_IS_ENABLED vs IS_ENABLED

[Troy Kisky]

Hi Tom


On Mon, Jan 30, 2023 at 9:18 AM Tom Rini <trini@konsulko.com> wrote:

> On Sat, Jan 28, 2023 at 09:25:54AM -0800, Troy Kisky wrote:
> > Thanks Tom,
> >
> > I cleaned up the PR based on the CI results. Here's my current changes.
> >
> > Author: Troy Kisky <troy.kisky@boundarydevices.com>
> > Date:   Fri Jan 27 11:03:11 2023 -0800
> >
> >     dm: device-internal: use EVENT instead of DM_EVENT, because
> > event_notify is built for EVENT
> >
> >     Signed-off-by: Troy Kisky <troy.kisky@boundarydevices.com>
> >
> > diff --git a/include/dm/device-internal.h b/include/dm/device-internal.h
> > index f31c4702086..2e725aa9416 100644
> > --- a/include/dm/device-internal.h
> > +++ b/include/dm/device-internal.h
> > @@ -431,7 +431,7 @@ static inline void devres_release_all(struct udevice
> > *dev)
> >
> >  static inline int device_notify(const struct udevice *dev, enum event_t
> > type)
> >  {
> > -#if CONFIG_IS_ENABLED(DM_EVENT)
> > +#if CONFIG_IS_ENABLED(EVENT)
> >         return event_notify(type, &dev, sizeof(dev));
> >  #else
> >         return 0;
>
> Given 448e2b6327d0 ("event: Correct dependencies on the EVENT
> framework") I'm a little worried about this change here and want to be
> extra sure it doesn't break something inadvertently.


event_notify is in common/event, and the Makefile has
obj-$(CONFIG_$(SPL_TPL_)EVENT) += event.o

So, the other option is to change the Makefile line to
  obj-$(CONFIG_$(SPL_TPL_)DM_EVENT) += event.o

I don't know which is best.



>   Aside from that,
> are you able to post your series?



I was hoping each of the maintainers could run the script and see if the
patches for their area make sense.
I don't need my sign-off on any of the patches.

  And incorporate your checking script
> in to .gitlab-ci.yml / .azure-pipline.yml ?
>

I'll post a patch for that as an RFC


>
> --
> Tom
>

Re: CONFIG_IS_ENABLED vs IS_ENABLED

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 2327 bytes --]

On Mon, Jan 30, 2023 at 10:51:03AM -0800, Troy Kisky wrote:
> Hi Tom
> 
> 
> On Mon, Jan 30, 2023 at 9:18 AM Tom Rini <trini@konsulko.com> wrote:
> 
> > On Sat, Jan 28, 2023 at 09:25:54AM -0800, Troy Kisky wrote:
> > > Thanks Tom,
> > >
> > > I cleaned up the PR based on the CI results. Here's my current changes.
> > >
> > > Author: Troy Kisky <troy.kisky@boundarydevices.com>
> > > Date:   Fri Jan 27 11:03:11 2023 -0800
> > >
> > >     dm: device-internal: use EVENT instead of DM_EVENT, because
> > > event_notify is built for EVENT
> > >
> > >     Signed-off-by: Troy Kisky <troy.kisky@boundarydevices.com>
> > >
> > > diff --git a/include/dm/device-internal.h b/include/dm/device-internal.h
> > > index f31c4702086..2e725aa9416 100644
> > > --- a/include/dm/device-internal.h
> > > +++ b/include/dm/device-internal.h
> > > @@ -431,7 +431,7 @@ static inline void devres_release_all(struct udevice
> > > *dev)
> > >
> > >  static inline int device_notify(const struct udevice *dev, enum event_t
> > > type)
> > >  {
> > > -#if CONFIG_IS_ENABLED(DM_EVENT)
> > > +#if CONFIG_IS_ENABLED(EVENT)
> > >         return event_notify(type, &dev, sizeof(dev));
> > >  #else
> > >         return 0;
> >
> > Given 448e2b6327d0 ("event: Correct dependencies on the EVENT
> > framework") I'm a little worried about this change here and want to be
> > extra sure it doesn't break something inadvertently.
> 
> 
> event_notify is in common/event, and the Makefile has
> obj-$(CONFIG_$(SPL_TPL_)EVENT) += event.o
> 
> So, the other option is to change the Makefile line to
>   obj-$(CONFIG_$(SPL_TPL_)DM_EVENT) += event.o
> 
> I don't know which is best.

Right, event_notify is part of the general event framework. The function
above, device_notify, is part of DM_EVENT. This should probably be
IS_ENABLED and not CONFIG_IS_ENABLED.

> >   Aside from that,
> > are you able to post your series?
> 
> 
> 
> I was hoping each of the maintainers could run the script and see if the
> patches for their area make sense.
> I don't need my sign-off on any of the patches.

Ah.  I'm not sure how likely that is to happen.

>   And incorporate your checking script
> > in to .gitlab-ci.yml / .azure-pipline.yml ?
> >
> 
> I'll post a patch for that as an RFC

OK.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: CONFIG_IS_ENABLED vs IS_ENABLED

[Troy Kisky]

On Mon, Jan 30, 2023 at 11:44 AM Tom Rini <trini@konsulko.com> wrote:

> On Mon, Jan 30, 2023 at 10:51:03AM -0800, Troy Kisky wrote:
> > Hi Tom
> >
> >
> > On Mon, Jan 30, 2023 at 9:18 AM Tom Rini <trini@konsulko.com> wrote:
> >
> > > On Sat, Jan 28, 2023 at 09:25:54AM -0800, Troy Kisky wrote:
> > > > Thanks Tom,
> > > >
> > > > I cleaned up the PR based on the CI results. Here's my current
> changes.
> > > >
> > > > Author: Troy Kisky <troy.kisky@boundarydevices.com>
> > > > Date:   Fri Jan 27 11:03:11 2023 -0800
> > > >
> > > >     dm: device-internal: use EVENT instead of DM_EVENT, because
> > > > event_notify is built for EVENT
> > > >
> > > >     Signed-off-by: Troy Kisky <troy.kisky@boundarydevices.com>
> > > >
> > > > diff --git a/include/dm/device-internal.h
> b/include/dm/device-internal.h
> > > > index f31c4702086..2e725aa9416 100644
> > > > --- a/include/dm/device-internal.h
> > > > +++ b/include/dm/device-internal.h
> > > > @@ -431,7 +431,7 @@ static inline void devres_release_all(struct
> udevice
> > > > *dev)
> > > >
> > > >  static inline int device_notify(const struct udevice *dev, enum
> event_t
> > > > type)
> > > >  {
> > > > -#if CONFIG_IS_ENABLED(DM_EVENT)
> > > > +#if CONFIG_IS_ENABLED(EVENT)
> > > >         return event_notify(type, &dev, sizeof(dev));
> > > >  #else
> > > >         return 0;
> > >
> > > Given 448e2b6327d0 ("event: Correct dependencies on the EVENT
> > > framework") I'm a little worried about this change here and want to be
> > > extra sure it doesn't break something inadvertently.
> >
> >
> > event_notify is in common/event, and the Makefile has
> > obj-$(CONFIG_$(SPL_TPL_)EVENT) += event.o
> >
> > So, the other option is to change the Makefile line to
> >   obj-$(CONFIG_$(SPL_TPL_)DM_EVENT) += event.o
> >
> > I don't know which is best.
>
> Right, event_notify is part of the general event framework. The function
> above, device_notify, is part of DM_EVENT. This should probably be
> IS_ENABLED and not CONFIG_IS_ENABLED.
>
>
I think DM_EVENT should be globally replaced by  EVENT. It doesn't seem to
control much.
Alternatively, I could add DM_EVENT to my SPL config list.
changing to IS_ENABLED  is what my script did before this patch, and caused
CI to fail.


Simon ?


> >   Aside from that,
> > > are you able to post your series?
> >
> >
> >
> > I was hoping each of the maintainers could run the script and see if the
> > patches for their area make sense.
> > I don't need my sign-off on any of the patches.
>
> Ah.  I'm not sure how likely that is to happen.
>
> >   And incorporate your checking script
> > > in to .gitlab-ci.yml / .azure-pipline.yml ?
> > >
> >
> > I'll post a patch for that as an RFC
>
> OK.
>
> --
> Tom
>

Re: CONFIG_IS_ENABLED vs IS_ENABLED

[Simon Glass]

Hi Troy,

On Mon, 30 Jan 2023 at 15:17, Troy Kisky <troykiskyboundary@gmail.com> wrote:
>
>
>
> On Mon, Jan 30, 2023 at 11:44 AM Tom Rini <trini@konsulko.com> wrote:
>>
>> On Mon, Jan 30, 2023 at 10:51:03AM -0800, Troy Kisky wrote:
>> > Hi Tom
>> >
>> >
>> > On Mon, Jan 30, 2023 at 9:18 AM Tom Rini <trini@konsulko.com> wrote:
>> >
>> > > On Sat, Jan 28, 2023 at 09:25:54AM -0800, Troy Kisky wrote:
>> > > > Thanks Tom,
>> > > >
>> > > > I cleaned up the PR based on the CI results. Here's my current changes.
>> > > >
>> > > > Author: Troy Kisky <troy.kisky@boundarydevices.com>
>> > > > Date:   Fri Jan 27 11:03:11 2023 -0800
>> > > >
>> > > >     dm: device-internal: use EVENT instead of DM_EVENT, because
>> > > > event_notify is built for EVENT
>> > > >
>> > > >     Signed-off-by: Troy Kisky <troy.kisky@boundarydevices.com>
>> > > >
>> > > > diff --git a/include/dm/device-internal.h b/include/dm/device-internal.h
>> > > > index f31c4702086..2e725aa9416 100644
>> > > > --- a/include/dm/device-internal.h
>> > > > +++ b/include/dm/device-internal.h
>> > > > @@ -431,7 +431,7 @@ static inline void devres_release_all(struct udevice
>> > > > *dev)
>> > > >
>> > > >  static inline int device_notify(const struct udevice *dev, enum event_t
>> > > > type)
>> > > >  {
>> > > > -#if CONFIG_IS_ENABLED(DM_EVENT)
>> > > > +#if CONFIG_IS_ENABLED(EVENT)
>> > > >         return event_notify(type, &dev, sizeof(dev));
>> > > >  #else
>> > > >         return 0;
>> > >
>> > > Given 448e2b6327d0 ("event: Correct dependencies on the EVENT
>> > > framework") I'm a little worried about this change here and want to be
>> > > extra sure it doesn't break something inadvertently.
>> >
>> >
>> > event_notify is in common/event, and the Makefile has
>> > obj-$(CONFIG_$(SPL_TPL_)EVENT) += event.o
>> >
>> > So, the other option is to change the Makefile line to
>> >   obj-$(CONFIG_$(SPL_TPL_)DM_EVENT) += event.o
>> >
>> > I don't know which is best.
>>
>> Right, event_notify is part of the general event framework. The function
>> above, device_notify, is part of DM_EVENT. This should probably be
>> IS_ENABLED and not CONFIG_IS_ENABLED.
>>
>
> I think DM_EVENT should be globally replaced by  EVENT. It doesn't seem to control much.
> Alternatively, I could add DM_EVENT to my SPL config list.
> changing to IS_ENABLED  is what my script did before this patch, and caused CI to fail.
>
>
> Simon ?

DM_EVENT controls whether driver model sends out events via
device_notify(). I don't see that this feature is widely enabled yet,
so I don't think we should require it. However perhaps the usage will
increase with time.

>
>
>> > >   Aside from that,
>> > > are you able to post your series?
>> >
>> >
>> >
>> > I was hoping each of the maintainers could run the script and see if the
>> > patches for their area make sense.
>> > I don't need my sign-off on any of the patches.
>>
>> Ah.  I'm not sure how likely that is to happen.
>>
>> >   And incorporate your checking script
>> > > in to .gitlab-ci.yml / .azure-pipline.yml ?
>> > >
>> >
>> > I'll post a patch for that as an RFC
>>
>> OK.

Regards,
Simon

Re: CONFIG_IS_ENABLED vs IS_ENABLED

[Simon Glass]

Hi Tom,

On Thu, 26 Jan 2023 at 14:33, Tom Rini <trini@konsulko.com> wrote:
>
> On Thu, Jan 26, 2023 at 02:29:53PM -0700, Simon Glass wrote:
> > Hi Tom,
> >
> > On Thu, 26 Jan 2023 at 11:16, Tom Rini <trini@konsulko.com> wrote:
> > >
> > > On Thu, Jan 26, 2023 at 11:04:21AM -0700, Simon Glass wrote:
> > > > Hi Tom,
> > > >
> > > > On Thu, 26 Jan 2023 at 10:21, Tom Rini <trini@konsulko.com> wrote:
> > > > >
> > > > > On Tue, Jan 24, 2023 at 06:36:00PM -0700, Simon Glass wrote:
> > > > > > Hi Troy,
> > > > > >
> > > > > > On Tue, 24 Jan 2023 at 16:31, Troy Kisky <Troy.Kisky@lairdconnect.com> wrote:
> > > > > > >
> > > > > > >
> > > > > > >
> > > > > > > ________________________________
> > > > > > > From: Troy Kisky
> > > > > > > Sent: Tuesday, January 24, 2023 2:52 PM
> > > > > > > To: u-boot@lists.denx.de <u-boot@lists.denx.de>; sbabic@denx.de <sbabic@denx.de>; trini@konsulko.com <trini@konsulko.com>; festevam@gmail.com <festevam@gmail.com>
> > > > > > > Cc: sjg@chromium.org <sjg@chromium.org>; marex@denx.de <marex@denx.de>
> > > > > > > Subject: CONFIG_IS_ENABLED vs IS_ENABLED
> > > > > > >
> > > > > > > Hi Guys
> > > > > > >
> > > > > > > In a recent debugging session, I stumbled across this line
> > > > > > > drivers/mmc/mmc.c:      if (CONFIG_IS_ENABLED(MMC_QUIRKS) && mmc->quirks & quirk)
> > > > > > >
> > > > > > > which prevents retries in SPL code, and was causing booting from an SD card to fail.
> > > > > > > So I wrote a little script to print uses of
> > > > > > > CONFIG_IS_ENABLED(x) which might need to be
> > > > > > > IS_ENABLED(CONFIG_x) like the above one.
> > > > > > >
> > > > > > > Here it is if you want to try it out.
> > > > > > >
> > > > > > > git grep CONFIG_IS_ENABLED|sed -n -e "s/\(CONFIG_IS_ENABLED([0-9a-zA-Z_]*)\)/\n\1\n/gp"| \
> > > > > > > sed -n -r "s/CONFIG_IS_ENABLED\(([0-9a-zA-Z_]+)\)/\1/p" |sort -u|xargs -I {} \
> > > > > > > sh -c "git grep -E 'config [ST]PL_{}' | grep -q -E -w '[ST]PL_{}' || git grep 'CONFIG_IS_ENABLED({})'"
> > > > > > >
> > > > > > > It prints CONFIG_IS_ENABLED(x) uses where there is no SPL_x or TPL_x.
> > > > > > >
> > > > > > > BR
> > > > > > > Troy
> > > > > > >
> > > > > > > _______
> > > > > > > And here is the opposite check
> > > > > > >
> > > > > > > git grep -w IS_ENABLED|sed -n -e "s/\(IS_ENABLED(CONFIG_[0-9a-zA-Z_]*)\)/\n\1\n/gp"| \
> > > > > > > sed -n -r "s/IS_ENABLED\(CONFIG_([0-9a-zA-Z_]+)\)/\1/p" |sort -u|xargs -I {} \
> > > > > > > sh -c "git grep -E 'config [ST]PL_{}' | grep -q -E -w '[ST]PL_{}' && git grep 'IS_ENABLED(CONFIG_{})'"
> > > > > > >
> > > > > > >
> > > > > > > It prints uses of IS_ENABLED(CONFIG_x) where CONFIG_SPL_x exists.
> > > > > >
> > > > > > Thank you for that. We definitely have quite a few of these.
> > > > > >
> > > > > > By a great coincidence I updated moveconfig.py to do something a
> > > > > > little like that:.
> > > > > >
> > > > > > https://patchwork.ozlabs.org/project/uboot/patch/20230123220031.3540724-2-sjg@chromium.org/
> > > > >
> > > > > I think this also shows that we might really want to just drop the
> > > > > checkpatch.pl note about IS_ENABLED / CONFIG_IS_ENABLED, it's getting
> > > > > used in a lot of wrong places where it's not helpful. It's not the root
> > > > > cause here (where a compile time check that allows for the rest of the
> > > > > code to be statically checked still is OK), but it's part of the
> > > > > problem.
> > > >
> > > > Firstly, we want to drop the use of #ifdef so what should we say instead?
> > >
> > > I'm not sure that dropping #ifdef in and of itself is a good goal.
> > > #if IS_ENABLED(CONFIG_FOO)
> > > does not read better
> > > #ifdef CONFIG_FOO
> >
> > So far in my prototype I have implemented
> >
> > #if CONFIG(FOO)
> >
> > and
> >
> > if (CONFIG(FOO))
> >
> > which replaces direct use of CONFIG_FOO and also
> > CONFIG_IS_ENABLED(FOO). We could ban use of CONFIG_FOO easily enough.
>
> I'm not convinced #if CONFIG(FOO) is better, but OK.

OK.

>
> > > And we have a lot of cases of the former that I'm not sure can
> > > logically or helpfully be replaced with
> > >         if (IS_ENABLED(CONFIG_FOO))
> > > either for functional / legibility reasons (ie it doesn't read better
> > > and just getting static analysis isn't a great reasons, more indent
> > > makes the code harder to follow) or isn't possible because things like:
> > > #ifdef CONFIG_FOO
> > >         int i = CONFIG_BAZ;
> > >         ...
> > > #endif
> > >
> > > Can't be replaced with if (IS_ENABLED(CONFIG_FOO)) { ... } when
> > > CONFIG_BAZ is only ever asked when CONFIG_FOO is enabled. And we can't
> > > define CONFIG_BAZ to 0 if not set (that leads back to introducing
> > > CONFIG things being defined).
> >
> > We have IF_ENABLED_INT() so can do things like that
>
> And could come up wit IF_ENABLED_STRING() I suppose. But I'm still not
> sure it'll read better / more clearly.

No it isn't that great, but we might get used to it. It is rare but
does resolve one of the two main remaining needs for #ifdef

>
> > > > Secondly, I think we should fix all this by splitting the config,
> > > > along the lines of my old series [1]. I hit similar problems to Troy
> > > > and have modified moveconfig.py to detect these (hence my recent
> > > > series [2]). I will see if I can get some sort of series out by
> > > > Monday. I had something pretty close(TM) but it failed on a few qemu
> > > > tests [3]
> > >
> > > I don't disagree with seeing what things look like with a "split" config
> > > again, but I think that fundamentally Yamada-san was right way back, and
> > > we really need to move towards separate config files for each stage and
> > > then combine them when applicable.
> >
> > Well let's see what you think once I get this next version of the series out.
>
> OK.
>
> > > That's also not easy, but I'm also
> > > not sure how to deal with the cases where we intentionally have
> > > CONFIG_FOO=n CONFIG_SPL_FOO=y without doing what we're doing today, at
> > > some level.
> >
> > Well I believe these are bugs, but don't know how many. Will probably
> > have some idea by early next week.
>
> There's certainly some bugs, but there's also some intentionally done
> ones, such as around SKIP_LOWLEVEL_INIT

Ah OK. Well if there is SPL_ symbol as well, as in this case, then
perhaps it is clear what the intention is?

Regards,
Simon

Re: CONFIG_IS_ENABLED vs IS_ENABLED

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 5890 bytes --]

On Thu, Jan 26, 2023 at 02:29:53PM -0700, Simon Glass wrote:
> Hi Tom,
> 
> On Thu, 26 Jan 2023 at 11:16, Tom Rini <trini@konsulko.com> wrote:
> >
> > On Thu, Jan 26, 2023 at 11:04:21AM -0700, Simon Glass wrote:
> > > Hi Tom,
> > >
> > > On Thu, 26 Jan 2023 at 10:21, Tom Rini <trini@konsulko.com> wrote:
> > > >
> > > > On Tue, Jan 24, 2023 at 06:36:00PM -0700, Simon Glass wrote:
> > > > > Hi Troy,
> > > > >
> > > > > On Tue, 24 Jan 2023 at 16:31, Troy Kisky <Troy.Kisky@lairdconnect.com> wrote:
> > > > > >
> > > > > >
> > > > > >
> > > > > > ________________________________
> > > > > > From: Troy Kisky
> > > > > > Sent: Tuesday, January 24, 2023 2:52 PM
> > > > > > To: u-boot@lists.denx.de <u-boot@lists.denx.de>; sbabic@denx.de <sbabic@denx.de>; trini@konsulko.com <trini@konsulko.com>; festevam@gmail.com <festevam@gmail.com>
> > > > > > Cc: sjg@chromium.org <sjg@chromium.org>; marex@denx.de <marex@denx.de>
> > > > > > Subject: CONFIG_IS_ENABLED vs IS_ENABLED
> > > > > >
> > > > > > Hi Guys
> > > > > >
> > > > > > In a recent debugging session, I stumbled across this line
> > > > > > drivers/mmc/mmc.c:      if (CONFIG_IS_ENABLED(MMC_QUIRKS) && mmc->quirks & quirk)
> > > > > >
> > > > > > which prevents retries in SPL code, and was causing booting from an SD card to fail.
> > > > > > So I wrote a little script to print uses of
> > > > > > CONFIG_IS_ENABLED(x) which might need to be
> > > > > > IS_ENABLED(CONFIG_x) like the above one.
> > > > > >
> > > > > > Here it is if you want to try it out.
> > > > > >
> > > > > > git grep CONFIG_IS_ENABLED|sed -n -e "s/\(CONFIG_IS_ENABLED([0-9a-zA-Z_]*)\)/\n\1\n/gp"| \
> > > > > > sed -n -r "s/CONFIG_IS_ENABLED\(([0-9a-zA-Z_]+)\)/\1/p" |sort -u|xargs -I {} \
> > > > > > sh -c "git grep -E 'config [ST]PL_{}' | grep -q -E -w '[ST]PL_{}' || git grep 'CONFIG_IS_ENABLED({})'"
> > > > > >
> > > > > > It prints CONFIG_IS_ENABLED(x) uses where there is no SPL_x or TPL_x.
> > > > > >
> > > > > > BR
> > > > > > Troy
> > > > > >
> > > > > > _______
> > > > > > And here is the opposite check
> > > > > >
> > > > > > git grep -w IS_ENABLED|sed -n -e "s/\(IS_ENABLED(CONFIG_[0-9a-zA-Z_]*)\)/\n\1\n/gp"| \
> > > > > > sed -n -r "s/IS_ENABLED\(CONFIG_([0-9a-zA-Z_]+)\)/\1/p" |sort -u|xargs -I {} \
> > > > > > sh -c "git grep -E 'config [ST]PL_{}' | grep -q -E -w '[ST]PL_{}' && git grep 'IS_ENABLED(CONFIG_{})'"
> > > > > >
> > > > > >
> > > > > > It prints uses of IS_ENABLED(CONFIG_x) where CONFIG_SPL_x exists.
> > > > >
> > > > > Thank you for that. We definitely have quite a few of these.
> > > > >
> > > > > By a great coincidence I updated moveconfig.py to do something a
> > > > > little like that:.
> > > > >
> > > > > https://patchwork.ozlabs.org/project/uboot/patch/20230123220031.3540724-2-sjg@chromium.org/
> > > >
> > > > I think this also shows that we might really want to just drop the
> > > > checkpatch.pl note about IS_ENABLED / CONFIG_IS_ENABLED, it's getting
> > > > used in a lot of wrong places where it's not helpful. It's not the root
> > > > cause here (where a compile time check that allows for the rest of the
> > > > code to be statically checked still is OK), but it's part of the
> > > > problem.
> > >
> > > Firstly, we want to drop the use of #ifdef so what should we say instead?
> >
> > I'm not sure that dropping #ifdef in and of itself is a good goal.
> > #if IS_ENABLED(CONFIG_FOO)
> > does not read better
> > #ifdef CONFIG_FOO
> 
> So far in my prototype I have implemented
> 
> #if CONFIG(FOO)
> 
> and
> 
> if (CONFIG(FOO))
> 
> which replaces direct use of CONFIG_FOO and also
> CONFIG_IS_ENABLED(FOO). We could ban use of CONFIG_FOO easily enough.

I'm not convinced #if CONFIG(FOO) is better, but OK.

> > And we have a lot of cases of the former that I'm not sure can
> > logically or helpfully be replaced with
> >         if (IS_ENABLED(CONFIG_FOO))
> > either for functional / legibility reasons (ie it doesn't read better
> > and just getting static analysis isn't a great reasons, more indent
> > makes the code harder to follow) or isn't possible because things like:
> > #ifdef CONFIG_FOO
> >         int i = CONFIG_BAZ;
> >         ...
> > #endif
> >
> > Can't be replaced with if (IS_ENABLED(CONFIG_FOO)) { ... } when
> > CONFIG_BAZ is only ever asked when CONFIG_FOO is enabled. And we can't
> > define CONFIG_BAZ to 0 if not set (that leads back to introducing
> > CONFIG things being defined).
> 
> We have IF_ENABLED_INT() so can do things like that

And could come up wit IF_ENABLED_STRING() I suppose. But I'm still not
sure it'll read better / more clearly.

> > > Secondly, I think we should fix all this by splitting the config,
> > > along the lines of my old series [1]. I hit similar problems to Troy
> > > and have modified moveconfig.py to detect these (hence my recent
> > > series [2]). I will see if I can get some sort of series out by
> > > Monday. I had something pretty close(TM) but it failed on a few qemu
> > > tests [3]
> >
> > I don't disagree with seeing what things look like with a "split" config
> > again, but I think that fundamentally Yamada-san was right way back, and
> > we really need to move towards separate config files for each stage and
> > then combine them when applicable.
> 
> Well let's see what you think once I get this next version of the series out.

OK.

> > That's also not easy, but I'm also
> > not sure how to deal with the cases where we intentionally have
> > CONFIG_FOO=n CONFIG_SPL_FOO=y without doing what we're doing today, at
> > some level.
> 
> Well I believe these are bugs, but don't know how many. Will probably
> have some idea by early next week.

There's certainly some bugs, but there's also some intentionally done
ones, such as around SKIP_LOWLEVEL_INIT

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: CONFIG_IS_ENABLED vs IS_ENABLED

[Simon Glass]

Hi Tom,

On Thu, 26 Jan 2023 at 11:16, Tom Rini <trini@konsulko.com> wrote:
>
> On Thu, Jan 26, 2023 at 11:04:21AM -0700, Simon Glass wrote:
> > Hi Tom,
> >
> > On Thu, 26 Jan 2023 at 10:21, Tom Rini <trini@konsulko.com> wrote:
> > >
> > > On Tue, Jan 24, 2023 at 06:36:00PM -0700, Simon Glass wrote:
> > > > Hi Troy,
> > > >
> > > > On Tue, 24 Jan 2023 at 16:31, Troy Kisky <Troy.Kisky@lairdconnect.com> wrote:
> > > > >
> > > > >
> > > > >
> > > > > ________________________________
> > > > > From: Troy Kisky
> > > > > Sent: Tuesday, January 24, 2023 2:52 PM
> > > > > To: u-boot@lists.denx.de <u-boot@lists.denx.de>; sbabic@denx.de <sbabic@denx.de>; trini@konsulko.com <trini@konsulko.com>; festevam@gmail.com <festevam@gmail.com>
> > > > > Cc: sjg@chromium.org <sjg@chromium.org>; marex@denx.de <marex@denx.de>
> > > > > Subject: CONFIG_IS_ENABLED vs IS_ENABLED
> > > > >
> > > > > Hi Guys
> > > > >
> > > > > In a recent debugging session, I stumbled across this line
> > > > > drivers/mmc/mmc.c:      if (CONFIG_IS_ENABLED(MMC_QUIRKS) && mmc->quirks & quirk)
> > > > >
> > > > > which prevents retries in SPL code, and was causing booting from an SD card to fail.
> > > > > So I wrote a little script to print uses of
> > > > > CONFIG_IS_ENABLED(x) which might need to be
> > > > > IS_ENABLED(CONFIG_x) like the above one.
> > > > >
> > > > > Here it is if you want to try it out.
> > > > >
> > > > > git grep CONFIG_IS_ENABLED|sed -n -e "s/\(CONFIG_IS_ENABLED([0-9a-zA-Z_]*)\)/\n\1\n/gp"| \
> > > > > sed -n -r "s/CONFIG_IS_ENABLED\(([0-9a-zA-Z_]+)\)/\1/p" |sort -u|xargs -I {} \
> > > > > sh -c "git grep -E 'config [ST]PL_{}' | grep -q -E -w '[ST]PL_{}' || git grep 'CONFIG_IS_ENABLED({})'"
> > > > >
> > > > > It prints CONFIG_IS_ENABLED(x) uses where there is no SPL_x or TPL_x.
> > > > >
> > > > > BR
> > > > > Troy
> > > > >
> > > > > _______
> > > > > And here is the opposite check
> > > > >
> > > > > git grep -w IS_ENABLED|sed -n -e "s/\(IS_ENABLED(CONFIG_[0-9a-zA-Z_]*)\)/\n\1\n/gp"| \
> > > > > sed -n -r "s/IS_ENABLED\(CONFIG_([0-9a-zA-Z_]+)\)/\1/p" |sort -u|xargs -I {} \
> > > > > sh -c "git grep -E 'config [ST]PL_{}' | grep -q -E -w '[ST]PL_{}' && git grep 'IS_ENABLED(CONFIG_{})'"
> > > > >
> > > > >
> > > > > It prints uses of IS_ENABLED(CONFIG_x) where CONFIG_SPL_x exists.
> > > >
> > > > Thank you for that. We definitely have quite a few of these.
> > > >
> > > > By a great coincidence I updated moveconfig.py to do something a
> > > > little like that:.
> > > >
> > > > https://patchwork.ozlabs.org/project/uboot/patch/20230123220031.3540724-2-sjg@chromium.org/
> > >
> > > I think this also shows that we might really want to just drop the
> > > checkpatch.pl note about IS_ENABLED / CONFIG_IS_ENABLED, it's getting
> > > used in a lot of wrong places where it's not helpful. It's not the root
> > > cause here (where a compile time check that allows for the rest of the
> > > code to be statically checked still is OK), but it's part of the
> > > problem.
> >
> > Firstly, we want to drop the use of #ifdef so what should we say instead?
>
> I'm not sure that dropping #ifdef in and of itself is a good goal.
> #if IS_ENABLED(CONFIG_FOO)
> does not read better
> #ifdef CONFIG_FOO

So far in my prototype I have implemented

#if CONFIG(FOO)

and

if (CONFIG(FOO))

which replaces direct use of CONFIG_FOO and also
CONFIG_IS_ENABLED(FOO). We could ban use of CONFIG_FOO easily enough.

>
> And we have a lot of cases of the former that I'm not sure can
> logically or helpfully be replaced with
>         if (IS_ENABLED(CONFIG_FOO))
> either for functional / legibility reasons (ie it doesn't read better
> and just getting static analysis isn't a great reasons, more indent
> makes the code harder to follow) or isn't possible because things like:
> #ifdef CONFIG_FOO
>         int i = CONFIG_BAZ;
>         ...
> #endif
>
> Can't be replaced with if (IS_ENABLED(CONFIG_FOO)) { ... } when
> CONFIG_BAZ is only ever asked when CONFIG_FOO is enabled. And we can't
> define CONFIG_BAZ to 0 if not set (that leads back to introducing
> CONFIG things being defined).

We have IF_ENABLED_INT() so can do things like that

>
> > Secondly, I think we should fix all this by splitting the config,
> > along the lines of my old series [1]. I hit similar problems to Troy
> > and have modified moveconfig.py to detect these (hence my recent
> > series [2]). I will see if I can get some sort of series out by
> > Monday. I had something pretty close(TM) but it failed on a few qemu
> > tests [3]
>
> I don't disagree with seeing what things look like with a "split" config
> again, but I think that fundamentally Yamada-san was right way back, and
> we really need to move towards separate config files for each stage and
> then combine them when applicable.

Well let's see what you think once I get this next version of the series out.

> That's also not easy, but I'm also
> not sure how to deal with the cases where we intentionally have
> CONFIG_FOO=n CONFIG_SPL_FOO=y without doing what we're doing today, at
> some level.

Well I believe these are bugs, but don't know how many. Will probably
have some idea by early next week.

>
> > Troy, are you able to create patches for the problems you find, and
> > perhaps adding your checks to a Makefile rule or script so we can run
> > it in CI?
>
> What we're doing today does need to be analyzed and some good number of
> cases corrected I am sure.

Yes.

Regards,
Simon

Re: CONFIG_IS_ENABLED vs IS_ENABLED

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 4795 bytes --]

On Thu, Jan 26, 2023 at 11:04:21AM -0700, Simon Glass wrote:
> Hi Tom,
> 
> On Thu, 26 Jan 2023 at 10:21, Tom Rini <trini@konsulko.com> wrote:
> >
> > On Tue, Jan 24, 2023 at 06:36:00PM -0700, Simon Glass wrote:
> > > Hi Troy,
> > >
> > > On Tue, 24 Jan 2023 at 16:31, Troy Kisky <Troy.Kisky@lairdconnect.com> wrote:
> > > >
> > > >
> > > >
> > > > ________________________________
> > > > From: Troy Kisky
> > > > Sent: Tuesday, January 24, 2023 2:52 PM
> > > > To: u-boot@lists.denx.de <u-boot@lists.denx.de>; sbabic@denx.de <sbabic@denx.de>; trini@konsulko.com <trini@konsulko.com>; festevam@gmail.com <festevam@gmail.com>
> > > > Cc: sjg@chromium.org <sjg@chromium.org>; marex@denx.de <marex@denx.de>
> > > > Subject: CONFIG_IS_ENABLED vs IS_ENABLED
> > > >
> > > > Hi Guys
> > > >
> > > > In a recent debugging session, I stumbled across this line
> > > > drivers/mmc/mmc.c:      if (CONFIG_IS_ENABLED(MMC_QUIRKS) && mmc->quirks & quirk)
> > > >
> > > > which prevents retries in SPL code, and was causing booting from an SD card to fail.
> > > > So I wrote a little script to print uses of
> > > > CONFIG_IS_ENABLED(x) which might need to be
> > > > IS_ENABLED(CONFIG_x) like the above one.
> > > >
> > > > Here it is if you want to try it out.
> > > >
> > > > git grep CONFIG_IS_ENABLED|sed -n -e "s/\(CONFIG_IS_ENABLED([0-9a-zA-Z_]*)\)/\n\1\n/gp"| \
> > > > sed -n -r "s/CONFIG_IS_ENABLED\(([0-9a-zA-Z_]+)\)/\1/p" |sort -u|xargs -I {} \
> > > > sh -c "git grep -E 'config [ST]PL_{}' | grep -q -E -w '[ST]PL_{}' || git grep 'CONFIG_IS_ENABLED({})'"
> > > >
> > > > It prints CONFIG_IS_ENABLED(x) uses where there is no SPL_x or TPL_x.
> > > >
> > > > BR
> > > > Troy
> > > >
> > > > _______
> > > > And here is the opposite check
> > > >
> > > > git grep -w IS_ENABLED|sed -n -e "s/\(IS_ENABLED(CONFIG_[0-9a-zA-Z_]*)\)/\n\1\n/gp"| \
> > > > sed -n -r "s/IS_ENABLED\(CONFIG_([0-9a-zA-Z_]+)\)/\1/p" |sort -u|xargs -I {} \
> > > > sh -c "git grep -E 'config [ST]PL_{}' | grep -q -E -w '[ST]PL_{}' && git grep 'IS_ENABLED(CONFIG_{})'"
> > > >
> > > >
> > > > It prints uses of IS_ENABLED(CONFIG_x) where CONFIG_SPL_x exists.
> > >
> > > Thank you for that. We definitely have quite a few of these.
> > >
> > > By a great coincidence I updated moveconfig.py to do something a
> > > little like that:.
> > >
> > > https://patchwork.ozlabs.org/project/uboot/patch/20230123220031.3540724-2-sjg@chromium.org/
> >
> > I think this also shows that we might really want to just drop the
> > checkpatch.pl note about IS_ENABLED / CONFIG_IS_ENABLED, it's getting
> > used in a lot of wrong places where it's not helpful. It's not the root
> > cause here (where a compile time check that allows for the rest of the
> > code to be statically checked still is OK), but it's part of the
> > problem.
> 
> Firstly, we want to drop the use of #ifdef so what should we say instead?

I'm not sure that dropping #ifdef in and of itself is a good goal.
#if IS_ENABLED(CONFIG_FOO)
does not read better
#ifdef CONFIG_FOO

And we have a lot of cases of the former that I'm not sure can
logically or helpfully be replaced with
	if (IS_ENABLED(CONFIG_FOO))
either for functional / legibility reasons (ie it doesn't read better
and just getting static analysis isn't a great reasons, more indent
makes the code harder to follow) or isn't possible because things like:
#ifdef CONFIG_FOO
	int i = CONFIG_BAZ;
	...
#endif

Can't be replaced with if (IS_ENABLED(CONFIG_FOO)) { ... } when
CONFIG_BAZ is only ever asked when CONFIG_FOO is enabled. And we can't
define CONFIG_BAZ to 0 if not set (that leads back to introducing
CONFIG things being defined).

> Secondly, I think we should fix all this by splitting the config,
> along the lines of my old series [1]. I hit similar problems to Troy
> and have modified moveconfig.py to detect these (hence my recent
> series [2]). I will see if I can get some sort of series out by
> Monday. I had something pretty close(TM) but it failed on a few qemu
> tests [3]

I don't disagree with seeing what things look like with a "split" config
again, but I think that fundamentally Yamada-san was right way back, and
we really need to move towards separate config files for each stage and
then combine them when applicable. That's also not easy, but I'm also
not sure how to deal with the cases where we intentionally have
CONFIG_FOO=n CONFIG_SPL_FOO=y without doing what we're doing today, at
some level.

> Troy, are you able to create patches for the problems you find, and
> perhaps adding your checks to a Makefile rule or script so we can run
> it in CI?

What we're doing today does need to be analyzed and some good number of
cases corrected I am sure.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: CONFIG_IS_ENABLED vs IS_ENABLED

[Simon Glass]

Hi Tom,

On Thu, 26 Jan 2023 at 10:21, Tom Rini <trini@konsulko.com> wrote:
>
> On Tue, Jan 24, 2023 at 06:36:00PM -0700, Simon Glass wrote:
> > Hi Troy,
> >
> > On Tue, 24 Jan 2023 at 16:31, Troy Kisky <Troy.Kisky@lairdconnect.com> wrote:
> > >
> > >
> > >
> > > ________________________________
> > > From: Troy Kisky
> > > Sent: Tuesday, January 24, 2023 2:52 PM
> > > To: u-boot@lists.denx.de <u-boot@lists.denx.de>; sbabic@denx.de <sbabic@denx.de>; trini@konsulko.com <trini@konsulko.com>; festevam@gmail.com <festevam@gmail.com>
> > > Cc: sjg@chromium.org <sjg@chromium.org>; marex@denx.de <marex@denx.de>
> > > Subject: CONFIG_IS_ENABLED vs IS_ENABLED
> > >
> > > Hi Guys
> > >
> > > In a recent debugging session, I stumbled across this line
> > > drivers/mmc/mmc.c:      if (CONFIG_IS_ENABLED(MMC_QUIRKS) && mmc->quirks & quirk)
> > >
> > > which prevents retries in SPL code, and was causing booting from an SD card to fail.
> > > So I wrote a little script to print uses of
> > > CONFIG_IS_ENABLED(x) which might need to be
> > > IS_ENABLED(CONFIG_x) like the above one.
> > >
> > > Here it is if you want to try it out.
> > >
> > > git grep CONFIG_IS_ENABLED|sed -n -e "s/\(CONFIG_IS_ENABLED([0-9a-zA-Z_]*)\)/\n\1\n/gp"| \
> > > sed -n -r "s/CONFIG_IS_ENABLED\(([0-9a-zA-Z_]+)\)/\1/p" |sort -u|xargs -I {} \
> > > sh -c "git grep -E 'config [ST]PL_{}' | grep -q -E -w '[ST]PL_{}' || git grep 'CONFIG_IS_ENABLED({})'"
> > >
> > > It prints CONFIG_IS_ENABLED(x) uses where there is no SPL_x or TPL_x.
> > >
> > > BR
> > > Troy
> > >
> > > _______
> > > And here is the opposite check
> > >
> > > git grep -w IS_ENABLED|sed -n -e "s/\(IS_ENABLED(CONFIG_[0-9a-zA-Z_]*)\)/\n\1\n/gp"| \
> > > sed -n -r "s/IS_ENABLED\(CONFIG_([0-9a-zA-Z_]+)\)/\1/p" |sort -u|xargs -I {} \
> > > sh -c "git grep -E 'config [ST]PL_{}' | grep -q -E -w '[ST]PL_{}' && git grep 'IS_ENABLED(CONFIG_{})'"
> > >
> > >
> > > It prints uses of IS_ENABLED(CONFIG_x) where CONFIG_SPL_x exists.
> >
> > Thank you for that. We definitely have quite a few of these.
> >
> > By a great coincidence I updated moveconfig.py to do something a
> > little like that:.
> >
> > https://patchwork.ozlabs.org/project/uboot/patch/20230123220031.3540724-2-sjg@chromium.org/
>
> I think this also shows that we might really want to just drop the
> checkpatch.pl note about IS_ENABLED / CONFIG_IS_ENABLED, it's getting
> used in a lot of wrong places where it's not helpful. It's not the root
> cause here (where a compile time check that allows for the rest of the
> code to be statically checked still is OK), but it's part of the
> problem.

Firstly, we want to drop the use of #ifdef so what should we say instead?

Secondly, I think we should fix all this by splitting the config,
along the lines of my old series [1]. I hit similar problems to Troy
and have modified moveconfig.py to detect these (hence my recent
series [2]). I will see if I can get some sort of series out by
Monday. I had something pretty close(TM) but it failed on a few qemu
tests [3]

Troy, are you able to create patches for the problems you find, and
perhaps adding your checks to a Makefile rule or script so we can run
it in CI?

Regards,
Simon

[1] https://patchwork.ozlabs.org/project/uboot/list/?series=262853&state=*
[2] https://patchwork.ozlabs.org/project/uboot/list/?series=338142
[3] https://source.denx.de/u-boot/custodians/u-boot-dm/-/pipelines/14830

Re: CONFIG_IS_ENABLED vs IS_ENABLED

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 2485 bytes --]

On Tue, Jan 24, 2023 at 06:36:00PM -0700, Simon Glass wrote:
> Hi Troy,
> 
> On Tue, 24 Jan 2023 at 16:31, Troy Kisky <Troy.Kisky@lairdconnect.com> wrote:
> >
> >
> >
> > ________________________________
> > From: Troy Kisky
> > Sent: Tuesday, January 24, 2023 2:52 PM
> > To: u-boot@lists.denx.de <u-boot@lists.denx.de>; sbabic@denx.de <sbabic@denx.de>; trini@konsulko.com <trini@konsulko.com>; festevam@gmail.com <festevam@gmail.com>
> > Cc: sjg@chromium.org <sjg@chromium.org>; marex@denx.de <marex@denx.de>
> > Subject: CONFIG_IS_ENABLED vs IS_ENABLED
> >
> > Hi Guys
> >
> > In a recent debugging session, I stumbled across this line
> > drivers/mmc/mmc.c:      if (CONFIG_IS_ENABLED(MMC_QUIRKS) && mmc->quirks & quirk)
> >
> > which prevents retries in SPL code, and was causing booting from an SD card to fail.
> > So I wrote a little script to print uses of
> > CONFIG_IS_ENABLED(x) which might need to be
> > IS_ENABLED(CONFIG_x) like the above one.
> >
> > Here it is if you want to try it out.
> >
> > git grep CONFIG_IS_ENABLED|sed -n -e "s/\(CONFIG_IS_ENABLED([0-9a-zA-Z_]*)\)/\n\1\n/gp"| \
> > sed -n -r "s/CONFIG_IS_ENABLED\(([0-9a-zA-Z_]+)\)/\1/p" |sort -u|xargs -I {} \
> > sh -c "git grep -E 'config [ST]PL_{}' | grep -q -E -w '[ST]PL_{}' || git grep 'CONFIG_IS_ENABLED({})'"
> >
> > It prints CONFIG_IS_ENABLED(x) uses where there is no SPL_x or TPL_x.
> >
> > BR
> > Troy
> >
> > _______
> > And here is the opposite check
> >
> > git grep -w IS_ENABLED|sed -n -e "s/\(IS_ENABLED(CONFIG_[0-9a-zA-Z_]*)\)/\n\1\n/gp"| \
> > sed -n -r "s/IS_ENABLED\(CONFIG_([0-9a-zA-Z_]+)\)/\1/p" |sort -u|xargs -I {} \
> > sh -c "git grep -E 'config [ST]PL_{}' | grep -q -E -w '[ST]PL_{}' && git grep 'IS_ENABLED(CONFIG_{})'"
> >
> >
> > It prints uses of IS_ENABLED(CONFIG_x) where CONFIG_SPL_x exists.
> 
> Thank you for that. We definitely have quite a few of these.
> 
> By a great coincidence I updated moveconfig.py to do something a
> little like that:.
> 
> https://patchwork.ozlabs.org/project/uboot/patch/20230123220031.3540724-2-sjg@chromium.org/

I think this also shows that we might really want to just drop the
checkpatch.pl note about IS_ENABLED / CONFIG_IS_ENABLED, it's getting
used in a lot of wrong places where it's not helpful. It's not the root
cause here (where a compile time check that allows for the rest of the
code to be statically checked still is OK), but it's part of the
problem.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: CONFIG_IS_ENABLED vs IS_ENABLED

[Simon Glass]

Hi Troy,

On Tue, 24 Jan 2023 at 16:31, Troy Kisky <Troy.Kisky@lairdconnect.com> wrote:
>
>
>
> ________________________________
> From: Troy Kisky
> Sent: Tuesday, January 24, 2023 2:52 PM
> To: u-boot@lists.denx.de <u-boot@lists.denx.de>; sbabic@denx.de <sbabic@denx.de>; trini@konsulko.com <trini@konsulko.com>; festevam@gmail.com <festevam@gmail.com>
> Cc: sjg@chromium.org <sjg@chromium.org>; marex@denx.de <marex@denx.de>
> Subject: CONFIG_IS_ENABLED vs IS_ENABLED
>
> Hi Guys
>
> In a recent debugging session, I stumbled across this line
> drivers/mmc/mmc.c:      if (CONFIG_IS_ENABLED(MMC_QUIRKS) && mmc->quirks & quirk)
>
> which prevents retries in SPL code, and was causing booting from an SD card to fail.
> So I wrote a little script to print uses of
> CONFIG_IS_ENABLED(x) which might need to be
> IS_ENABLED(CONFIG_x) like the above one.
>
> Here it is if you want to try it out.
>
> git grep CONFIG_IS_ENABLED|sed -n -e "s/\(CONFIG_IS_ENABLED([0-9a-zA-Z_]*)\)/\n\1\n/gp"| \
> sed -n -r "s/CONFIG_IS_ENABLED\(([0-9a-zA-Z_]+)\)/\1/p" |sort -u|xargs -I {} \
> sh -c "git grep -E 'config [ST]PL_{}' | grep -q -E -w '[ST]PL_{}' || git grep 'CONFIG_IS_ENABLED({})'"
>
> It prints CONFIG_IS_ENABLED(x) uses where there is no SPL_x or TPL_x.
>
> BR
> Troy
>
> _______
> And here is the opposite check
>
> git grep -w IS_ENABLED|sed -n -e "s/\(IS_ENABLED(CONFIG_[0-9a-zA-Z_]*)\)/\n\1\n/gp"| \
> sed -n -r "s/IS_ENABLED\(CONFIG_([0-9a-zA-Z_]+)\)/\1/p" |sort -u|xargs -I {} \
> sh -c "git grep -E 'config [ST]PL_{}' | grep -q -E -w '[ST]PL_{}' && git grep 'IS_ENABLED(CONFIG_{})'"
>
>
> It prints uses of IS_ENABLED(CONFIG_x) where CONFIG_SPL_x exists.

Thank you for that. We definitely have quite a few of these.

By a great coincidence I updated moveconfig.py to do something a
little like that:.

https://patchwork.ozlabs.org/project/uboot/patch/20230123220031.3540724-2-sjg@chromium.org/

Regards,
Simon


>
>
> THIS MESSAGE, ANY ATTACHMENT(S), AND THE INFORMATION CONTAINED HEREIN MAY BE PROPRIETARY TO LAIRD CONNECTIVITY, LLC., BOUNDARY DEVICES, LLC. AND/OR ANOTHER PARTY, AND MAY FURTHER BE INTENDED TO BE KEPT CONFIDENTIAL. IF YOU ARE NOT THE INTENDED RECIPIENT, PLEASE DELETE THE EMAIL AND ANY ATTACHMENTS, AND IMMEDIATELY NOTIFY THE SENDER BY RETURN EMAIL. THIS MESSAGE AND ITS CONTENTS ARE THE PROPERTY OF LAIRD CONNECTIVITY, LLC. AND BOUNDARY DEVICES, LLC. AND MAY NOT BE REPRODUCED OR USED WITHOUT THE EXPRESS WRITTEN CONSENT OF LAIRD CONNECTIVITY, LLC. AND BOUNDARY DEVICES, LLC.

Are you able to drop that from mailing list posts, please?

Re: CONFIG_IS_ENABLED vs IS_ENABLED

[Troy Kisky]

________________________________
From: Troy Kisky
Sent: Tuesday, January 24, 2023 2:52 PM
To: u-boot@lists.denx.de <u-boot@lists.denx.de>; sbabic@denx.de <sbabic@denx.de>; trini@konsulko.com <trini@konsulko.com>; festevam@gmail.com <festevam@gmail.com>
Cc: sjg@chromium.org <sjg@chromium.org>; marex@denx.de <marex@denx.de>
Subject: CONFIG_IS_ENABLED vs IS_ENABLED

Hi Guys

In a recent debugging session, I stumbled across this line
drivers/mmc/mmc.c:      if (CONFIG_IS_ENABLED(MMC_QUIRKS) && mmc->quirks & quirk)

which prevents retries in SPL code, and was causing booting from an SD card to fail.
So I wrote a little script to print uses of
CONFIG_IS_ENABLED(x) which might need to be
IS_ENABLED(CONFIG_x) like the above one.

Here it is if you want to try it out.

git grep CONFIG_IS_ENABLED|sed -n -e "s/\(CONFIG_IS_ENABLED([0-9a-zA-Z_]*)\)/\n\1\n/gp"| \
sed -n -r "s/CONFIG_IS_ENABLED\(([0-9a-zA-Z_]+)\)/\1/p" |sort -u|xargs -I {} \
sh -c "git grep -E 'config [ST]PL_{}' | grep -q -E -w '[ST]PL_{}' || git grep 'CONFIG_IS_ENABLED({})'"

It prints CONFIG_IS_ENABLED(x) uses where there is no SPL_x or TPL_x.

BR
Troy

_______
And here is the opposite check

git grep -w IS_ENABLED|sed -n -e "s/\(IS_ENABLED(CONFIG_[0-9a-zA-Z_]*)\)/\n\1\n/gp"| \
sed -n -r "s/IS_ENABLED\(CONFIG_([0-9a-zA-Z_]+)\)/\1/p" |sort -u|xargs -I {} \
sh -c "git grep -E 'config [ST]PL_{}' | grep -q -E -w '[ST]PL_{}' && git grep 'IS_ENABLED(CONFIG_{})'"


It prints uses of IS_ENABLED(CONFIG_x) where CONFIG_SPL_x exists.


BR
Troy



THIS MESSAGE, ANY ATTACHMENT(S), AND THE INFORMATION CONTAINED HEREIN MAY BE PROPRIETARY TO LAIRD CONNECTIVITY, LLC., BOUNDARY DEVICES, LLC. AND/OR ANOTHER PARTY, AND MAY FURTHER BE INTENDED TO BE KEPT CONFIDENTIAL. IF YOU ARE NOT THE INTENDED RECIPIENT, PLEASE DELETE THE EMAIL AND ANY ATTACHMENTS, AND IMMEDIATELY NOTIFY THE SENDER BY RETURN EMAIL. THIS MESSAGE AND ITS CONTENTS ARE THE PROPERTY OF LAIRD CONNECTIVITY, LLC. AND BOUNDARY DEVICES, LLC. AND MAY NOT BE REPRODUCED OR USED WITHOUT THE EXPRESS WRITTEN CONSENT OF LAIRD CONNECTIVITY, LLC. AND BOUNDARY DEVICES, LLC.

CONFIG_IS_ENABLED vs IS_ENABLED

[Troy Kisky]

Hi Guys

In a recent debugging session, I stumbled across this line
drivers/mmc/mmc.c:      if (CONFIG_IS_ENABLED(MMC_QUIRKS) && mmc->quirks & quirk)

which prevents retries in SPL code, and was causing booting from an SD card to fail.
So I wrote a little script to print uses of
CONFIG_IS_ENABLED(x) which might need to be
IS_ENABLED(CONFIG_x) like the above one.

Here it is if you want to try it out.

git grep CONFIG_IS_ENABLED|sed -n -e "s/\(CONFIG_IS_ENABLED([0-9a-zA-Z_]*)\)/\n\1\n/gp"| \
sed -n -r "s/CONFIG_IS_ENABLED\(([0-9a-zA-Z_]+)\)/\1/p" |sort -u|xargs -I {} \
sh -c "git grep -E 'config [ST]PL_{}' | grep -q -E -w '[ST]PL_{}' || git grep 'CONFIG_IS_ENABLED({})'"

It prints CONFIG_IS_ENABLED(x) uses where there is no SPL_x or TPL_x.

BR
Troy


THIS MESSAGE, ANY ATTACHMENT(S), AND THE INFORMATION CONTAINED HEREIN MAY BE PROPRIETARY TO LAIRD CONNECTIVITY, LLC., BOUNDARY DEVICES, LLC. AND/OR ANOTHER PARTY, AND MAY FURTHER BE INTENDED TO BE KEPT CONFIDENTIAL. IF YOU ARE NOT THE INTENDED RECIPIENT, PLEASE DELETE THE EMAIL AND ANY ATTACHMENTS, AND IMMEDIATELY NOTIFY THE SENDER BY RETURN EMAIL. THIS MESSAGE AND ITS CONTENTS ARE THE PROPERTY OF LAIRD CONNECTIVITY, LLC. AND BOUNDARY DEVICES, LLC. AND MAY NOT BE REPRODUCED OR USED WITHOUT THE EXPRESS WRITTEN CONSENT OF LAIRD CONNECTIVITY, LLC. AND BOUNDARY DEVICES, LLC.