Re: [PATCH 06/24] dtoc: Support adding a string list to a device tree

[Simon Glass]

Hi Alper,

On Thu, 3 Mar 2022 at 14:14, Alper Nebi Yasak  wrote:
>
> On 24/02/2022 01:58, Simon Glass wrote:
> > On Tue, 15 Feb 2022 at 04:53, Alper Nebi Yasak  
> > wrote:
> >> On 08/02/2022 21:49, Simon Glass wrote:
> >>> diff --git a/tools/dtoc/fdt.py b/tools/dtoc/fdt.py
> >>> index 32a7aa9829..e7197f8f12 100644
> >>> --- a/tools/dtoc/fdt.py
> >>> +++ b/tools/dtoc/fdt.py
> >>> @@ -501,6 +501,24 @@ class Node:
> >>>  val = bytes(val, 'utf-8')
> >>>  return self.AddData(prop_name, val + b'\0')
> >>>
> >>> +def AddStringList(self, prop_name, val):
> >>> +"""Add a new string-list property to a node
> >>> +
> >>> +The device tree is marked dirty so that the value will be 
> >>> written to
> >>> +the blob on the next sync.
> >>> +
> >>> +Args:
> >>> +prop_name: Name of property to add
> >>> +val (list of str): List of strings to add
> >>> +
> >>> +Returns:
> >>> +Prop added
> >>> +"""
> >>> +out = b''
> >>> +for string in val:
> >>> +out += bytes(string, 'utf-8') + b'\0'
> >>> +return self.AddData(prop_name, out)
> >>
> >> If val is an empty list this would try to set a zero-byte data, but then
> >> the Prop class considers that a boolean True.
> >
> > Yes, but I believe that is correct, isn't it? I did propose a change
> > to DT bool props but it has not got anywhere.
>
> I just wasn't sure about the semantics and wanted to highlight what
> might be an edge case. Looking again, I think this is binary-wise
> correct, but GetString and GetStringList would return True and [True]
> for these boolean props where I'd want None and [].

I'll add a test for that in v3.

Regards,
Simon

Re: [PATCH 06/24] dtoc: Support adding a string list to a device tree

[Alper Nebi Yasak]

On 24/02/2022 01:58, Simon Glass wrote:
> On Tue, 15 Feb 2022 at 04:53, Alper Nebi Yasak  
> wrote:
>> On 08/02/2022 21:49, Simon Glass wrote:
>>> diff --git a/tools/dtoc/fdt.py b/tools/dtoc/fdt.py
>>> index 32a7aa9829..e7197f8f12 100644
>>> --- a/tools/dtoc/fdt.py
>>> +++ b/tools/dtoc/fdt.py
>>> @@ -501,6 +501,24 @@ class Node:
>>>  val = bytes(val, 'utf-8')
>>>  return self.AddData(prop_name, val + b'\0')
>>>
>>> +def AddStringList(self, prop_name, val):
>>> +"""Add a new string-list property to a node
>>> +
>>> +The device tree is marked dirty so that the value will be written 
>>> to
>>> +the blob on the next sync.
>>> +
>>> +Args:
>>> +prop_name: Name of property to add
>>> +val (list of str): List of strings to add
>>> +
>>> +Returns:
>>> +Prop added
>>> +"""
>>> +out = b''
>>> +for string in val:
>>> +out += bytes(string, 'utf-8') + b'\0'
>>> +return self.AddData(prop_name, out)
>>
>> If val is an empty list this would try to set a zero-byte data, but then
>> the Prop class considers that a boolean True.
> 
> Yes, but I believe that is correct, isn't it? I did propose a change
> to DT bool props but it has not got anywhere.

I just wasn't sure about the semantics and wanted to highlight what
might be an edge case. Looking again, I think this is binary-wise
correct, but GetString and GetStringList would return True and [True]
for these boolean props where I'd want None and [].

Re: [PATCH 06/24] dtoc: Support adding a string list to a device tree

[Simon Glass]

Hi Alper,

On Tue, 15 Feb 2022 at 04:53, Alper Nebi Yasak  wrote:
>
> On 08/02/2022 21:49, Simon Glass wrote:
> > Add a new function to add a string list.
> >
> > Signed-off-by: Simon Glass 
> > ---
> >
> >  tools/dtoc/fdt.py  | 18 ++
> >  tools/dtoc/test_fdt.py |  8 
> >  2 files changed, 26 insertions(+)
> >
> > diff --git a/tools/dtoc/fdt.py b/tools/dtoc/fdt.py
> > index 32a7aa9829..e7197f8f12 100644
> > --- a/tools/dtoc/fdt.py
> > +++ b/tools/dtoc/fdt.py
> > @@ -501,6 +501,24 @@ class Node:
> >  val = bytes(val, 'utf-8')
> >  return self.AddData(prop_name, val + b'\0')
> >
> > +def AddStringList(self, prop_name, val):
> > +"""Add a new string-list property to a node
> > +
> > +The device tree is marked dirty so that the value will be written 
> > to
> > +the blob on the next sync.
> > +
> > +Args:
> > +prop_name: Name of property to add
> > +val (list of str): List of strings to add
> > +
> > +Returns:
> > +Prop added
> > +"""
> > +out = b''
> > +for string in val:
> > +out += bytes(string, 'utf-8') + b'\0'
> > +return self.AddData(prop_name, out)
>
> If val is an empty list this would try to set a zero-byte data, but then
> the Prop class considers that a boolean True.

Yes, but I believe that is correct, isn't it? I did propose a change
to DT bool props but it has not got anywhere.

>
> Perhaps use "s" as the variable name as there's a "string" module
> (though not imported here).
>
> Also, b'\0'.join() works just like in strings if you prefer that to a loop.

OK, will update both of those.

Regards,
Simon

Re: [PATCH 06/24] dtoc: Support adding a string list to a device tree

[Simon Glass]

On 08/02/2022 21:49, Simon Glass wrote:
> Add a new function to add a string list.
>
> Signed-off-by: Simon Glass 
> ---
>
>  tools/dtoc/fdt.py  | 18 ++
>  tools/dtoc/test_fdt.py |  8 
>  2 files changed, 26 insertions(+)
>
Applied to u-boot-dm, thanks!

Re: [PATCH 06/24] dtoc: Support adding a string list to a device tree

[Alper Nebi Yasak]

On 08/02/2022 21:49, Simon Glass wrote:
> Add a new function to add a string list.
> 
> Signed-off-by: Simon Glass 
> ---
> 
>  tools/dtoc/fdt.py  | 18 ++
>  tools/dtoc/test_fdt.py |  8 
>  2 files changed, 26 insertions(+)
> 
> diff --git a/tools/dtoc/fdt.py b/tools/dtoc/fdt.py
> index 32a7aa9829..e7197f8f12 100644
> --- a/tools/dtoc/fdt.py
> +++ b/tools/dtoc/fdt.py
> @@ -501,6 +501,24 @@ class Node:
>  val = bytes(val, 'utf-8')
>  return self.AddData(prop_name, val + b'\0')
>  
> +def AddStringList(self, prop_name, val):
> +"""Add a new string-list property to a node
> +
> +The device tree is marked dirty so that the value will be written to
> +the blob on the next sync.
> +
> +Args:
> +prop_name: Name of property to add
> +val (list of str): List of strings to add
> +
> +Returns:
> +Prop added
> +"""
> +out = b''
> +for string in val:
> +out += bytes(string, 'utf-8') + b'\0'
> +return self.AddData(prop_name, out)

If val is an empty list this would try to set a zero-byte data, but then
the Prop class considers that a boolean True.

Perhaps use "s" as the variable name as there's a "string" module
(though not imported here).

Also, b'\0'.join() works just like in strings if you prefer that to a loop.

> +
>  def AddInt(self, prop_name, val):
>  """Add a new integer property to a node
>  
> diff --git a/tools/dtoc/test_fdt.py b/tools/dtoc/test_fdt.py
> index 55b70e9876..1a7e73ffce 100755
> --- a/tools/dtoc/test_fdt.py
> +++ b/tools/dtoc/test_fdt.py
> @@ -531,6 +531,14 @@ class TestProp(unittest.TestCase):
>  self.node.AddData('data', tools.GetBytes(65, 2))
>  self.dtb.Sync(auto_resize=True)
>  
> +def test_string_list(self):
> +"""Test adding string-list property to a node"""
> +val = ['123', '456']
> +self.node.AddStringList('stringlist', val)
> +self.dtb.Sync(auto_resize=True)
> +data = self.fdt.getprop(self.node.Offset(), 'stringlist')
> +self.assertEqual(b'123\x00456\0', data)
> +
>  def testFromData(self):
>  dtb2 = fdt.Fdt.FromData(self.dtb.GetContents())
>  self.assertEqual(dtb2.GetContents(), self.dtb.GetContents())