[PATCH 1/2] dm: core: Add functions to read 8/16-bit integers

[Stefan Herbrechtsmeier]

Hi,

Am 14.07.2022 um 16:51 schrieb Simon Glass:
> Hi Stefan,
> 
> On Thu, 14 Jul 2022 at 04:37, Stefan Herbrechtsmeier
> <stefan.herbrechtsmeier-oss at weidmueller.com> wrote:
>>
>> Hi Simon,
>>
>> Am 14.07.2022 um 12:22 schrieb Simon Glass:
>>> Hi Stefan,
>>>
>>> On Wed, 13 Jul 2022 at 10:08, Stefan Herbrechtsmeier
>>> <stefan.herbrechtsmeier-oss at weidmueller.com> wrote:
>>>>
>>>> Hi Simon,
>>>>
>>>> Am 13.07.2022 um 17:28 schrieb Simon Glass:
>>>>> Hi Stefan,
>>>>>
>>>>> On Tue, 12 Jul 2022 at 12:31, Stefan Herbrechtsmeier
>>>>> <stefan.herbrechtsmeier-oss at weidmueller.com> wrote:
>>>>>>
>>>>>> Hi Simon,
>>>>>>
>>>>>> Am 12.07.2022 um 12:58 schrieb Simon Glass:
>>>>>>> Hi Stefan,
>>>>>>>
>>>>>>> On Tue, 14 Jun 2022 at 07:22, Stefan Herbrechtsmeier
>>>>>>> <stefan.herbrechtsmeier-oss at weidmueller.com> wrote:
>>>>>>>>
>>>>>>>> From: Stefan Herbrechtsmeier <stefan.herbrechtsmeier at weidmueller.com>
>>>>>>>>
>>>>>>>> Add functions to read 8/16-bit integers like the existing functions for
>>>>>>>> 32/64-bit to simplify read of 8/16-bit integers from device tree
>>>>>>>> properties.
>>>>>>>>
>>>>>>>> Signed-off-by: Stefan Herbrechtsmeier <stefan.herbrechtsmeier at weidmueller.com>
>>>>>>>> ---
>>>>>>>>
>>>>>>>>      arch/sandbox/dts/test.dts |  2 ++
>>>>>>>>      drivers/core/of_access.c  | 38 +++++++++++++++++++++++
>>>>>>>>      drivers/core/ofnode.c     | 62 +++++++++++++++++++++++++++++++++++++
>>>>>>>>      drivers/core/read.c       | 21 +++++++++++++
>>>>>>>>      include/dm/of_access.h    | 32 +++++++++++++++++++
>>>>>>>>      include/dm/ofnode.h       | 40 ++++++++++++++++++++++++
>>>>>>>>      include/dm/read.h         | 65 +++++++++++++++++++++++++++++++++++++++
>>>>>>>>      test/dm/test-fdt.c        | 19 ++++++++++++
>>>>>>>>      8 files changed, 279 insertions(+)
>>>>>>>
>>>>>>> This looks good but is very expensive in terms of code size. Can you
>>>>>>> update your u8 and u16 functions to reuse the existing u32 function
>>>>>>> and just convert the value?
>>>>>>
>>>>>> The u32 function requires a 32 bit value inside the device tree because
>>>>>> it checks the size and maybe swap the bytes.
>>>>>>
>>>>>> The u8 and u16 function requires only a 8 and 16 bit value inside the
>>>>>> device tree.
>>>>>
>>>>> Yes that's true. What is the use case for these functions?
>>>>
>>>> The usb251xb driver requires this functions [1]. The usb251xb device
>>>> tree binding [2] defines the ids as 16 bit values and the Linux driver
>>>> use 8 bit for an unspecified property. Without this changes the driver
>>>> doesn't satisfy the specification and is incompatible to the Linux driver.
>>>
>>> I wonder if that binding is a bit ambiguous. From what I have seen we
>>> normally use a single cell for int values, partly so that fdtdump
>>> works and partly because the format doesn't allow using any less space
>>> anyway.
>>>
>>> IMO that binding should use a whole cell for the byte and u16 values.
>>
>> How should we go on? The specification is 5 years old. I can ignore the
>> specification  and remove the "/bits/ 16" from my device tree source.
> 
> I believe either would work, since reading a u32 from the device tree
> and masking it would achieve the same result. The problem with the
> current u32 function is that it requires 4 bytes.
> 
> IMO the binding is odd, but I don't think you can just change it.
> 
>>
>>> Then we can use the u32() function and do a mask.
>>
>> The driver cast away the unseeded bits.
> 
> OK.
> 
> Anyway as Heinrich says below, most boards won't use these functions.
> I hope that other drivers don't start using u8 and u16 values when a
> u32 will do.
> 
> Reviewed-by: Simon Glass <sjg at chromium.org>
> 
>>
>>>>
>>>> [1] https://lists.denx.de/pipermail/u-boot/2022-June/486424.html
>>>> [2]
>>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/usb/usb251xb.txt
>>       Stefan
>>
> 
> Regards,
> Simon

Does this series need a rework to be applied?

Regards
   Stefan