re: CONFIG_IS_ENABLED vs IS_ENABLED
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
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.
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
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, becauseevent_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 ?
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, becauseevent_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
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, becauseevent_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.
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, becauseevent_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
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, becauseevent_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
participants (3)
-
Simon Glass -
Tom Rini -
Troy Kisky