Hi Quentin,
On Wed, 28 May 2025 at 17:15, Quentin Schulz <quentin.sch...@cherry.de> wrote:
>
> Hi Simon,
>
> On 5/28/25 4:32 PM, Simon Glass wrote:
> > Hi Quentin,
> >
> > On Wed, 28 May 2025 at 14:06, Quentin Schulz <foss+ub...@0leil.net> wrote:
> >>
> >> From: Quentin Schulz <quentin.sch...@cherry.de>
> >>
> >> dev_read_u32_default is for getting a u32 from a Device Tree property
> >> and allows to take a default value if that property is missing.
> >>
> >> Considering it calls ofnode_read_u32_default which takes a u32 and
> >> returns a u32, it should do the same instead of using an int, especially
> >> considering that int size is typically architecture-specific, as opposed
> >> to u32.
> >>
> >> This incidentally matches all other dev_read_*_default functions (except
> >> dev_read_s32_default which will be tackled in the next commit).
> >>
> >> Fixes: 47a0fd3bad38 ("dm: core: Implement live tree 'read' functions")
> >> Signed-off-by: Quentin Schulz <quentin.sch...@cherry.de>
> >> ---
> >> drivers/core/read.c | 4 ++--
> >> include/dm/read.h | 6 +++---
> >> 2 files changed, 5 insertions(+), 5 deletions(-)
> >
> > Re your comment above 'int' seems to mean 32-bit on the toolchains we
> > use. For example, in global_data we use int when we want a 32-bit
> > value and long when we want a machine-word-sized value.
> >
>
> Fair enough, the only thing I can say - without real world experience -
> is that int isn't guaranteed to be 4 B by the standard? c.f.
> https://en.wikipedia.org/wiki/C_data_types#Main_types
That's right. I suppose we could include the compiler's stdint.h
somehow and use int32_t but in practice Linux expects things to work
as I described and other embedded projects follow that.
>
> > In general I try to use natural types (int, long, ulong) where
> > possible, to avoid the compiler masking values before calling a
> > function, which can increase code size. In this case I could make the
> > argument either way.
> >
>
> I'm happy to reword this commit log to be less confusing or alarmist. I
> can remove the "especially", would that be enough, do you have something
> else in mind?
No, the patch is fine, I was just adding my comments.
Regards,
Simon
