Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Content-Type: text/plain; charset="utf-8"
MIME-Version: 1.0
Content-Transfer-Encoding: quoted-printable
In-Reply-To: <CANNG1HV0Ai9oeUr55uk+wAf456vDn1qiYmeXYc3=rkpqhdoM4g@mail.gmail.com>
References: <20190225065044.11023-1-vaishali.thakkar@linaro.org> <20190225065044.11023-5-vaishali.thakkar@linaro.org> <155138953788.16805.6820097041346672619@swboyd.mtv.corp.google.com> <CANNG1HWDc-PraBwG=8TBcfJSFFe+X+k6oVhOqyU=i5fAjpuoLw@mail.gmail.com> <155257912871.20095.1653828341120157988@swboyd.mtv.corp.google.com> <CANNG1HV0Ai9oeUr55uk+wAf456vDn1qiYmeXYc3=rkpqhdoM4g@mail.gmail.com>
From:   Stephen Boyd <swboyd@chromium.org>
Subject: Re: [PATCH v4 4/5] soc: qcom: socinfo: Expose custom attributes
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>
To:     Vaishali Thakkar <vaishali.thakkar@linaro.org>
Message-ID: <155329931528.20095.6505466693897029315@swboyd.mtv.corp.google.com>
User-Agent: alot/0.8
Date:   Fri, 22 Mar 2019 17:01:55 -0700
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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.
> >
>=20
> 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
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?

>=20
> 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.
>=20
>  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.