| Message ID | 20241110115054.2555-1-ansuelsmth@gmail.com |
|---|---|
| Headers | show |
| Series | led: update LED boot/activity to new property implementation | expand |
On Sun, Nov 10, 2024 at 12:50:19PM +0100, Christian Marangi wrote: > This series is split in 2 part. > > While adapting the LED boot and activity code to the new property > accepted by Rob in dt-schema repository, a big BUG was discovered. > > The reason wasn't clear at start and took me some days to figure it > out. > > This was triggered by adding a new phandle in the test.dts to > introduce test for the new OPs. > > This single addition caused the sandbox CI test to fail in the > dm_test_ofnode_phandle_ot test. > > This doesn't make sense as reverting the change made the CI test > to correctly finish. Also moving the uboot node down > after the first phandle (in test.dts the gpio one) also made > the CI test to correctly finish. > > A little bit of searching and debugging made me realize the > parse phandle OPs didn't support other.dts at all and they > were still referencing phandle index from test.dts. > (more info in the related commit) > > In short the test was broken all along and was working by > pure luck. The first 4 patch address and fix the problem for good. > > The other 4 patch expand and address the property change for > LED boot/activity. > > Posting in a single series as changes are trivial and just > to speedup review process. (and also because the second > part depends on the first) > > All CI tested with azure pipeline. > > Changes v2: > - Fix handling of flat tree for phandle > - Fix test and other.dts changes > > Christian Marangi (8): > dm: core: implement oftree variant of parse_phandle OPs > test: dm: fix broken dm_test_ofnode_phandle_ot and get_by_phandle_ot > dm: core: implement ofnode/tree_parse_phandle() helper > test: dm: Expand dm_test_ofnode_phandle(_ot) with new > ofnode/tree_parse_phandle > dm: core: implement phandle ofnode_options helper > test: dm: Add test for ofnode options phandle helper > led: update LED boot/activity to new property implementation > test: dm: Update test for LED activity and boot > > arch/sandbox/dts/other.dts | 31 ++++++++- > arch/sandbox/dts/test.dts | 16 +++-- > drivers/core/of_access.c | 61 ++++++++++++----- > drivers/core/ofnode.c | 124 ++++++++++++++++++++++++++++++++- > drivers/led/led-uclass.c | 30 +++++--- > include/dm/of_access.h | 86 +++++++++++++++++++++++ > include/dm/ofnode.h | 107 +++++++++++++++++++++++++++++ > test/dm/led.c | 18 +++-- > test/dm/ofnode.c | 136 ++++++++++++++++++++++++++++++++----- > 9 files changed, 551 insertions(+), 58 deletions(-) My main issue with the series is a lack of documentation updates, as the biggest challenge thus far has been that for example Peter couldn't figure out how to make use of this on PinePhone. We generate documentation today based on include/led.h for this API, yes? Thanks.
On Wed, Nov 13, 2024 at 12:00:59PM -0600, Tom Rini wrote: > On Sun, Nov 10, 2024 at 12:50:19PM +0100, Christian Marangi wrote: > > > This series is split in 2 part. > > > > While adapting the LED boot and activity code to the new property > > accepted by Rob in dt-schema repository, a big BUG was discovered. > > > > The reason wasn't clear at start and took me some days to figure it > > out. > > > > This was triggered by adding a new phandle in the test.dts to > > introduce test for the new OPs. > > > > This single addition caused the sandbox CI test to fail in the > > dm_test_ofnode_phandle_ot test. > > > > This doesn't make sense as reverting the change made the CI test > > to correctly finish. Also moving the uboot node down > > after the first phandle (in test.dts the gpio one) also made > > the CI test to correctly finish. > > > > A little bit of searching and debugging made me realize the > > parse phandle OPs didn't support other.dts at all and they > > were still referencing phandle index from test.dts. > > (more info in the related commit) > > > > In short the test was broken all along and was working by > > pure luck. The first 4 patch address and fix the problem for good. > > > > The other 4 patch expand and address the property change for > > LED boot/activity. > > > > Posting in a single series as changes are trivial and just > > to speedup review process. (and also because the second > > part depends on the first) > > > > All CI tested with azure pipeline. > > > > Changes v2: > > - Fix handling of flat tree for phandle > > - Fix test and other.dts changes > > > > Christian Marangi (8): > > dm: core: implement oftree variant of parse_phandle OPs > > test: dm: fix broken dm_test_ofnode_phandle_ot and get_by_phandle_ot > > dm: core: implement ofnode/tree_parse_phandle() helper > > test: dm: Expand dm_test_ofnode_phandle(_ot) with new > > ofnode/tree_parse_phandle > > dm: core: implement phandle ofnode_options helper > > test: dm: Add test for ofnode options phandle helper > > led: update LED boot/activity to new property implementation > > test: dm: Update test for LED activity and boot > > > > arch/sandbox/dts/other.dts | 31 ++++++++- > > arch/sandbox/dts/test.dts | 16 +++-- > > drivers/core/of_access.c | 61 ++++++++++++----- > > drivers/core/ofnode.c | 124 ++++++++++++++++++++++++++++++++- > > drivers/led/led-uclass.c | 30 +++++--- > > include/dm/of_access.h | 86 +++++++++++++++++++++++ > > include/dm/ofnode.h | 107 +++++++++++++++++++++++++++++ > > test/dm/led.c | 18 +++-- > > test/dm/ofnode.c | 136 ++++++++++++++++++++++++++++++++----- > > 9 files changed, 551 insertions(+), 58 deletions(-) > > My main issue with the series is a lack of documentation updates, as the > biggest challenge thus far has been that for example Peter couldn't > figure out how to make use of this on PinePhone. We generate > documentation today based on include/led.h for this API, yes? Thanks. > Hi Tom, actually quite the oppisite, led.h describe how these works and was instructed to first have the options property merged in dt-schema. [1] Here we have all the option documented under a yaml. I notice there is a series from Simon that is also pushing a .yaml here locally in U-Boot and I was waiting for that to be merged to also include the additional entry there. Also led.h description and API have info on where to look about the handling of /options/u-boot/ I feel the main problem with documentation is currently the fact that we are migrating to a more robust schema and people are used to using Doc directory? Anyway happy to get any hint on how to improve this. [1] https://github.com/devicetree-org/dt-schema
On Wed, Nov 13, 2024 at 09:24:59PM +0100, Christian Marangi wrote: > On Wed, Nov 13, 2024 at 12:00:59PM -0600, Tom Rini wrote: > > On Sun, Nov 10, 2024 at 12:50:19PM +0100, Christian Marangi wrote: > > > > > This series is split in 2 part. > > > > > > While adapting the LED boot and activity code to the new property > > > accepted by Rob in dt-schema repository, a big BUG was discovered. > > > > > > The reason wasn't clear at start and took me some days to figure it > > > out. > > > > > > This was triggered by adding a new phandle in the test.dts to > > > introduce test for the new OPs. > > > > > > This single addition caused the sandbox CI test to fail in the > > > dm_test_ofnode_phandle_ot test. > > > > > > This doesn't make sense as reverting the change made the CI test > > > to correctly finish. Also moving the uboot node down > > > after the first phandle (in test.dts the gpio one) also made > > > the CI test to correctly finish. > > > > > > A little bit of searching and debugging made me realize the > > > parse phandle OPs didn't support other.dts at all and they > > > were still referencing phandle index from test.dts. > > > (more info in the related commit) > > > > > > In short the test was broken all along and was working by > > > pure luck. The first 4 patch address and fix the problem for good. > > > > > > The other 4 patch expand and address the property change for > > > LED boot/activity. > > > > > > Posting in a single series as changes are trivial and just > > > to speedup review process. (and also because the second > > > part depends on the first) > > > > > > All CI tested with azure pipeline. > > > > > > Changes v2: > > > - Fix handling of flat tree for phandle > > > - Fix test and other.dts changes > > > > > > Christian Marangi (8): > > > dm: core: implement oftree variant of parse_phandle OPs > > > test: dm: fix broken dm_test_ofnode_phandle_ot and get_by_phandle_ot > > > dm: core: implement ofnode/tree_parse_phandle() helper > > > test: dm: Expand dm_test_ofnode_phandle(_ot) with new > > > ofnode/tree_parse_phandle > > > dm: core: implement phandle ofnode_options helper > > > test: dm: Add test for ofnode options phandle helper > > > led: update LED boot/activity to new property implementation > > > test: dm: Update test for LED activity and boot > > > > > > arch/sandbox/dts/other.dts | 31 ++++++++- > > > arch/sandbox/dts/test.dts | 16 +++-- > > > drivers/core/of_access.c | 61 ++++++++++++----- > > > drivers/core/ofnode.c | 124 ++++++++++++++++++++++++++++++++- > > > drivers/led/led-uclass.c | 30 +++++--- > > > include/dm/of_access.h | 86 +++++++++++++++++++++++ > > > include/dm/ofnode.h | 107 +++++++++++++++++++++++++++++ > > > test/dm/led.c | 18 +++-- > > > test/dm/ofnode.c | 136 ++++++++++++++++++++++++++++++++----- > > > 9 files changed, 551 insertions(+), 58 deletions(-) > > > > My main issue with the series is a lack of documentation updates, as the > > biggest challenge thus far has been that for example Peter couldn't > > figure out how to make use of this on PinePhone. We generate > > documentation today based on include/led.h for this API, yes? Thanks. > > > > Hi Tom, > > actually quite the oppisite, led.h describe how these works and was > instructed to first have the options property merged in dt-schema. [1] > > Here we have all the option documented under a yaml. > > I notice there is a series from Simon that is also pushing a .yaml > here locally in U-Boot and I was waiting for that to be merged to also > include the additional entry there. > > Also led.h description and API have info on where to look about the > handling of /options/u-boot/ > > I feel the main problem with documentation is currently the fact that we > are migrating to a more robust schema and people are used to using Doc > directory? Anyway happy to get any hint on how to improve this. > > [1] https://github.com/devicetree-org/dt-schema So Peter, does this bring you what you were asking for in terms of details, or can you explain what's still missing from your point of view? Thanks!
On Fri, Nov 22, 2024 at 08:41:13PM -0600, Tom Rini wrote: > On Wed, Nov 13, 2024 at 09:24:59PM +0100, Christian Marangi wrote: > > On Wed, Nov 13, 2024 at 12:00:59PM -0600, Tom Rini wrote: > > > On Sun, Nov 10, 2024 at 12:50:19PM +0100, Christian Marangi wrote: > > > > > > > This series is split in 2 part. > > > > > > > > While adapting the LED boot and activity code to the new property > > > > accepted by Rob in dt-schema repository, a big BUG was discovered. > > > > > > > > The reason wasn't clear at start and took me some days to figure it > > > > out. > > > > > > > > This was triggered by adding a new phandle in the test.dts to > > > > introduce test for the new OPs. > > > > > > > > This single addition caused the sandbox CI test to fail in the > > > > dm_test_ofnode_phandle_ot test. > > > > > > > > This doesn't make sense as reverting the change made the CI test > > > > to correctly finish. Also moving the uboot node down > > > > after the first phandle (in test.dts the gpio one) also made > > > > the CI test to correctly finish. > > > > > > > > A little bit of searching and debugging made me realize the > > > > parse phandle OPs didn't support other.dts at all and they > > > > were still referencing phandle index from test.dts. > > > > (more info in the related commit) > > > > > > > > In short the test was broken all along and was working by > > > > pure luck. The first 4 patch address and fix the problem for good. > > > > > > > > The other 4 patch expand and address the property change for > > > > LED boot/activity. > > > > > > > > Posting in a single series as changes are trivial and just > > > > to speedup review process. (and also because the second > > > > part depends on the first) > > > > > > > > All CI tested with azure pipeline. > > > > > > > > Changes v2: > > > > - Fix handling of flat tree for phandle > > > > - Fix test and other.dts changes > > > > > > > > Christian Marangi (8): > > > > dm: core: implement oftree variant of parse_phandle OPs > > > > test: dm: fix broken dm_test_ofnode_phandle_ot and get_by_phandle_ot > > > > dm: core: implement ofnode/tree_parse_phandle() helper > > > > test: dm: Expand dm_test_ofnode_phandle(_ot) with new > > > > ofnode/tree_parse_phandle > > > > dm: core: implement phandle ofnode_options helper > > > > test: dm: Add test for ofnode options phandle helper > > > > led: update LED boot/activity to new property implementation > > > > test: dm: Update test for LED activity and boot > > > > > > > > arch/sandbox/dts/other.dts | 31 ++++++++- > > > > arch/sandbox/dts/test.dts | 16 +++-- > > > > drivers/core/of_access.c | 61 ++++++++++++----- > > > > drivers/core/ofnode.c | 124 ++++++++++++++++++++++++++++++++- > > > > drivers/led/led-uclass.c | 30 +++++--- > > > > include/dm/of_access.h | 86 +++++++++++++++++++++++ > > > > include/dm/ofnode.h | 107 +++++++++++++++++++++++++++++ > > > > test/dm/led.c | 18 +++-- > > > > test/dm/ofnode.c | 136 ++++++++++++++++++++++++++++++++----- > > > > 9 files changed, 551 insertions(+), 58 deletions(-) > > > > > > My main issue with the series is a lack of documentation updates, as the > > > biggest challenge thus far has been that for example Peter couldn't > > > figure out how to make use of this on PinePhone. We generate > > > documentation today based on include/led.h for this API, yes? Thanks. > > > > > > > Hi Tom, > > > > actually quite the oppisite, led.h describe how these works and was > > instructed to first have the options property merged in dt-schema. [1] > > > > Here we have all the option documented under a yaml. > > > > I notice there is a series from Simon that is also pushing a .yaml > > here locally in U-Boot and I was waiting for that to be merged to also > > include the additional entry there. > > > > Also led.h description and API have info on where to look about the > > handling of /options/u-boot/ > > > > I feel the main problem with documentation is currently the fact that we > > are migrating to a more robust schema and people are used to using Doc > > directory? Anyway happy to get any hint on how to improve this. > > > > [1] https://github.com/devicetree-org/dt-schema > > So Peter, does this bring you what you were asking for in terms of > details, or can you explain what's still missing from your point of > view? Thanks! > Hi Tom, any idea how to improve this and get it trought?
On Fri, Dec 06, 2024 at 06:30:00PM +0100, Christian Marangi wrote: > On Fri, Nov 22, 2024 at 08:41:13PM -0600, Tom Rini wrote: > > On Wed, Nov 13, 2024 at 09:24:59PM +0100, Christian Marangi wrote: > > > On Wed, Nov 13, 2024 at 12:00:59PM -0600, Tom Rini wrote: > > > > On Sun, Nov 10, 2024 at 12:50:19PM +0100, Christian Marangi wrote: > > > > > > > > > This series is split in 2 part. > > > > > > > > > > While adapting the LED boot and activity code to the new property > > > > > accepted by Rob in dt-schema repository, a big BUG was discovered. > > > > > > > > > > The reason wasn't clear at start and took me some days to figure it > > > > > out. > > > > > > > > > > This was triggered by adding a new phandle in the test.dts to > > > > > introduce test for the new OPs. > > > > > > > > > > This single addition caused the sandbox CI test to fail in the > > > > > dm_test_ofnode_phandle_ot test. > > > > > > > > > > This doesn't make sense as reverting the change made the CI test > > > > > to correctly finish. Also moving the uboot node down > > > > > after the first phandle (in test.dts the gpio one) also made > > > > > the CI test to correctly finish. > > > > > > > > > > A little bit of searching and debugging made me realize the > > > > > parse phandle OPs didn't support other.dts at all and they > > > > > were still referencing phandle index from test.dts. > > > > > (more info in the related commit) > > > > > > > > > > In short the test was broken all along and was working by > > > > > pure luck. The first 4 patch address and fix the problem for good. > > > > > > > > > > The other 4 patch expand and address the property change for > > > > > LED boot/activity. > > > > > > > > > > Posting in a single series as changes are trivial and just > > > > > to speedup review process. (and also because the second > > > > > part depends on the first) > > > > > > > > > > All CI tested with azure pipeline. > > > > > > > > > > Changes v2: > > > > > - Fix handling of flat tree for phandle > > > > > - Fix test and other.dts changes > > > > > > > > > > Christian Marangi (8): > > > > > dm: core: implement oftree variant of parse_phandle OPs > > > > > test: dm: fix broken dm_test_ofnode_phandle_ot and get_by_phandle_ot > > > > > dm: core: implement ofnode/tree_parse_phandle() helper > > > > > test: dm: Expand dm_test_ofnode_phandle(_ot) with new > > > > > ofnode/tree_parse_phandle > > > > > dm: core: implement phandle ofnode_options helper > > > > > test: dm: Add test for ofnode options phandle helper > > > > > led: update LED boot/activity to new property implementation > > > > > test: dm: Update test for LED activity and boot > > > > > > > > > > arch/sandbox/dts/other.dts | 31 ++++++++- > > > > > arch/sandbox/dts/test.dts | 16 +++-- > > > > > drivers/core/of_access.c | 61 ++++++++++++----- > > > > > drivers/core/ofnode.c | 124 ++++++++++++++++++++++++++++++++- > > > > > drivers/led/led-uclass.c | 30 +++++--- > > > > > include/dm/of_access.h | 86 +++++++++++++++++++++++ > > > > > include/dm/ofnode.h | 107 +++++++++++++++++++++++++++++ > > > > > test/dm/led.c | 18 +++-- > > > > > test/dm/ofnode.c | 136 ++++++++++++++++++++++++++++++++----- > > > > > 9 files changed, 551 insertions(+), 58 deletions(-) > > > > > > > > My main issue with the series is a lack of documentation updates, as the > > > > biggest challenge thus far has been that for example Peter couldn't > > > > figure out how to make use of this on PinePhone. We generate > > > > documentation today based on include/led.h for this API, yes? Thanks. > > > > > > > > > > Hi Tom, > > > > > > actually quite the oppisite, led.h describe how these works and was > > > instructed to first have the options property merged in dt-schema. [1] > > > > > > Here we have all the option documented under a yaml. > > > > > > I notice there is a series from Simon that is also pushing a .yaml > > > here locally in U-Boot and I was waiting for that to be merged to also > > > include the additional entry there. > > > > > > Also led.h description and API have info on where to look about the > > > handling of /options/u-boot/ > > > > > > I feel the main problem with documentation is currently the fact that we > > > are migrating to a more robust schema and people are used to using Doc > > > directory? Anyway happy to get any hint on how to improve this. > > > > > > [1] https://github.com/devicetree-org/dt-schema > > > > So Peter, does this bring you what you were asking for in terms of > > details, or can you explain what's still missing from your point of > > view? Thanks! > > > > Hi Tom, > > any idea how to improve this and get it trought? I'll pick this up for -next soon, thanks.
On Sun, 10 Nov 2024 12:50:19 +0100, Christian Marangi wrote: > This series is split in 2 part. > > While adapting the LED boot and activity code to the new property > accepted by Rob in dt-schema repository, a big BUG was discovered. > > The reason wasn't clear at start and took me some days to figure it > out. > > [...] Applied to u-boot/next, thanks!
This series is split in 2 part. While adapting the LED boot and activity code to the new property accepted by Rob in dt-schema repository, a big BUG was discovered. The reason wasn't clear at start and took me some days to figure it out. This was triggered by adding a new phandle in the test.dts to introduce test for the new OPs. This single addition caused the sandbox CI test to fail in the dm_test_ofnode_phandle_ot test. This doesn't make sense as reverting the change made the CI test to correctly finish. Also moving the uboot node down after the first phandle (in test.dts the gpio one) also made the CI test to correctly finish. A little bit of searching and debugging made me realize the parse phandle OPs didn't support other.dts at all and they were still referencing phandle index from test.dts. (more info in the related commit) In short the test was broken all along and was working by pure luck. The first 4 patch address and fix the problem for good. The other 4 patch expand and address the property change for LED boot/activity. Posting in a single series as changes are trivial and just to speedup review process. (and also because the second part depends on the first) All CI tested with azure pipeline. Changes v2: - Fix handling of flat tree for phandle - Fix test and other.dts changes Christian Marangi (8): dm: core: implement oftree variant of parse_phandle OPs test: dm: fix broken dm_test_ofnode_phandle_ot and get_by_phandle_ot dm: core: implement ofnode/tree_parse_phandle() helper test: dm: Expand dm_test_ofnode_phandle(_ot) with new ofnode/tree_parse_phandle dm: core: implement phandle ofnode_options helper test: dm: Add test for ofnode options phandle helper led: update LED boot/activity to new property implementation test: dm: Update test for LED activity and boot arch/sandbox/dts/other.dts | 31 ++++++++- arch/sandbox/dts/test.dts | 16 +++-- drivers/core/of_access.c | 61 ++++++++++++----- drivers/core/ofnode.c | 124 ++++++++++++++++++++++++++++++++- drivers/led/led-uclass.c | 30 +++++--- include/dm/of_access.h | 86 +++++++++++++++++++++++ include/dm/ofnode.h | 107 +++++++++++++++++++++++++++++ test/dm/led.c | 18 +++-- test/dm/ofnode.c | 136 ++++++++++++++++++++++++++++++++----- 9 files changed, 551 insertions(+), 58 deletions(-)