On Sat, 23 Mar 2019 at 05:31, Stephen Boyd <swboyd@xxxxxxxxxxxx> wrote: > > Quoting Vaishali Thakkar (2019-03-20 22:51:20) > > On Thu, 14 Mar 2019 at 21:28, Stephen Boyd <swboyd@xxxxxxxxxxxx> wrote: > > > > > > Quoting Vaishali Thakkar (2019-03-14 04:25:16) > > > > On Fri, 1 Mar 2019 at 03:02, Stephen Boyd <swboyd@xxxxxxxxxxxx> wrote: > > > > > > > > > > > In the case of converting it to cpu native during probe, I'll need to > > > > declare an extra struct with u32 being the parsed version for it to be > > > > correct. Wouldn't it add extra overhead? > > > > > > Yes it would be some small extra overhead that could be allocated on the > > > kernel's heap. What's the maximum size? A hundred bytes or so? > > > > > > I don't see much of a problem with this approach. It simplifies the > > > patch series because nothing new is introduced in debugfs core and the > > > endian conversion is done once in one place instead of being scattered > > > throughout the code. Sounds like a good improvement to me. > > > > > > > Yes, it's true that this approach is better than introducing new endian > > functions in debugfs core but we should also keep in mind that this is > > applicable only for 4 use cases. For other usecases, we want to print > > string and hex values. So, I would either need new debugfs core > > functions for the same. I tried introducing debugfs_create_str for string > > values but we're ending up with introducing bunch of other helpers in > > the core as simple_attr_read expects integer values. Similarly, for hex > > values , I can't use debugfs_create_u32 as defined attributes in the > > core has unsigned int as a specifier, will need to introduce some extra > > helpers over there again. > > I imagine there are other uses of printing a string and hex value in > debugfs. There's debugfs_create_x32() and debugfs_create_x64() for the > hex value printing part (if you want that format). There's also Ok. > debugfs_create_devm_seqfile() which may work to print a string from some > struct member. I'm not sure why you're using simple_attr_read(). Where > does that become important? DEFINE_DEBUGFS_ATTRIBUTE has simple_attr helpers which expects int value. So, in the case of a string it requires to implement similar macro and separate helpers for the same. > > > > Also, in case of keeping all other cases as it is, it'll look quite > > asymmetric to use debugfs u32 function in init and using local macros > > for other cases. I can have DEBUGFS_UINT_ADD like wrapper > > macro for debugfs_create_u32 but again not sure if doing > > all of this looks better than what we have at the moment as just having > > 3 local macros covering our all cases without having lot of duplicated > > code. > > > > Let me know if about your opinion on the same. Thanks. > > My opinion is still that it would be best to push things that aren't SoC > specific into the debugfs core and try to use as much from the core as > possible. There doesn't seem to be anything very SoC specific here so > I'm lost why this isn't doable. Yes, that's true. We don't have much of SoC specific code here. >
