Re: [PATCH v2 0/8] led: update LED boot/activity to new property implementation

[Tom Rini <trini@konsulko.com>]

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

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.

-- 
Tom

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