Re: [PATCH v4 4/5] soc: qcom: socinfo: Expose custom attributes

[Vaishali Thakkar]

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.

>