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

From: Vaishali Thakkar <vaishali.thakkar@linaro.org>
To: Stephen Boyd <swboyd@chromium.org>
Cc: Andy Gross <andy.gross@linaro.org>,
	David Brown <david.brown@linaro.org>,
	Greg KH <gregkh@linuxfoundation.org>,
	linux-arm-msm@vger.kernel.org, linux-kernel@vger.kernel.org,
	rafael@kernel.org, Bjorn Andersson <bjorn.andersson@linaro.org>,
	Vinod Koul <vkoul@kernel.org>
Subject: Re: [PATCH v4 4/5] soc: qcom: socinfo: Expose custom attributes
Date: Sun, 24 Mar 2019 23:12:36 +0530	[thread overview]
Message-ID: <CANNG1HUCvL3=mtitvDZ3fOtt3u5oK7kr=9Nftov71aOsUn3nXA@mail.gmail.com> (raw)
In-Reply-To: <155329931528.20095.6505466693897029315@swboyd.mtv.corp.google.com>

On Sat, 23 Mar 2019 at 05:31, Stephen Boyd <swboyd@chromium.org> wrote:
>
> Quoting Vaishali Thakkar (2019-03-20 22:51:20)
> > On Thu, 14 Mar 2019 at 21:28, Stephen Boyd <swboyd@chromium.org> wrote:
> > >
> > > Quoting Vaishali Thakkar (2019-03-14 04:25:16)
> > > > On Fri, 1 Mar 2019 at 03:02, Stephen Boyd <swboyd@chromium.org> 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.

>