From mboxrd@z Thu Jan 1 00:00:00 1970 From: Stephen Boyd Subject: Re: [PATCH v4 4/5] soc: qcom: socinfo: Expose custom attributes Date: Thu, 14 Mar 2019 08:58:48 -0700 Message-ID: <155257912871.20095.1653828341120157988@swboyd.mtv.corp.google.com> References: <20190225065044.11023-1-vaishali.thakkar@linaro.org> <20190225065044.11023-5-vaishali.thakkar@linaro.org> <155138953788.16805.6820097041346672619@swboyd.mtv.corp.google.com> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable Return-path: In-Reply-To: Sender: linux-kernel-owner@vger.kernel.org To: Vaishali Thakkar Cc: Andy Gross , David Brown , Greg KH , linux-arm-msm@vger.kernel.org, linux-kernel@vger.kernel.org, rafael@kernel.org, Bjorn Andersson , Vinod Koul List-Id: linux-arm-msm@vger.kernel.org Quoting Vaishali Thakkar (2019-03-14 04:25:16) > On Fri, 1 Mar 2019 at 03:02, Stephen Boyd wrote: > > > > Why can't we use the debugfs_create_u32 function? It would make things > > clearer if there was either a debugfs_create_le32() function or if the > > socinfo structure stored in smem was unmarshalled from little endian > > to the cpu native endian format during probe time and then all the rest > > of the code can treat it as a native endian u32 values. >=20 > Thanks for the review. I've worked on the next version with all the > changes you suggested in the patchset but I'm kind of not sure > what would be the best solution in this case. Alright, thanks. >=20 > I'm bit skeptical about introducing debugfs_create_le32 as we don't > really have any endian specific functions in the debugfs core at the > moment. And if I do that, should I also introduce debugfs_create_be32 > [and to an extent debugfs_create_le{16,64}]? More importantly, would > it be useful to introduce them in core? I suppose it's ambiguous if the endianness should be swapped to be CPU native, or if it should just export them as little endian or big endian to userspace. I wouldn't introduce any APIs that aren't used, because it's just dead code. If the code is documented clearly and indicates what it does then it should be fine. This patch has pretty much already written the code, so it's just a matter of moving it into debugfs core now and getting gregkh to approve. >=20 > 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.