linux-kernel - Re: [PATCH 4/6] phy: qualcomm: phy-qcom-qmp-ufs: Move data structs and setting tables to header

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 20 Sep 2023 12:19:23 +0200
From:   Manivannan Sadhasivam <mani@...nel.org>
To:     Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
Cc:     Can Guo <quic_cang@...cinc.com>, quic_nguyenb@...cinc.com,
        quic_nitirawa@...cinc.com, martin.petersen@...cle.com,
        linux-scsi@...r.kernel.org, Andy Gross <agross@...nel.org>,
        Bjorn Andersson <andersson@...nel.org>,
        Konrad Dybcio <konrad.dybcio@...aro.org>,
        Vinod Koul <vkoul@...nel.org>,
        Kishon Vijay Abraham I <kishon@...nel.org>,
        Philipp Zabel <p.zabel@...gutronix.de>,
        open list <linux-kernel@...r.kernel.org>,
        "open list:ARM/QUALCOMM SUPPORT" <linux-arm-msm@...r.kernel.org>,
        "open list:GENERIC PHY FRAMEWORK" <linux-phy@...ts.infradead.org>
Subject: Re: [PATCH 4/6] phy: qualcomm: phy-qcom-qmp-ufs: Move data structs
 and setting tables to header

On Wed, Sep 20, 2023 at 01:30:21AM +0300, Dmitry Baryshkov wrote:
> On Tue, 19 Sept 2023 at 15:15, Manivannan Sadhasivam <mani@...nel.org> wrote:
> >
> > On Thu, Sep 14, 2023 at 03:28:59PM +0300, Dmitry Baryshkov wrote:
> > > On Mon, 11 Sept 2023 at 09:01, Can Guo <quic_cang@...cinc.com> wrote:
> > > >
> > > > To make the code more readable, move the data structs and PHY settting
> > > > tables to a header file, namely the phy-qcom-qmp-ufs.h.
> > > >
> > > > Signed-off-by: Can Guo <quic_cang@...cinc.com>
> > > > ---
> > > >  drivers/phy/qualcomm/phy-qcom-qmp-ufs.c | 802 +------------------------------
> > > >  drivers/phy/qualcomm/phy-qcom-qmp-ufs.h | 805 ++++++++++++++++++++++++++++++++
> > > >  2 files changed, 806 insertions(+), 801 deletions(-)
> > > >  create mode 100644 drivers/phy/qualcomm/phy-qcom-qmp-ufs.h
> > >
> > > Is there any reason to do so? Other than just moving stuff around, it
> > > doesn't give us anything. This header will not be shared with any
> > > other driver. Just moving data tables to the header (ugh, static data
> > > in the header) doesn't make code more readable.
> > >
> >
> > I think the motive here is to move the static tables to one file and have the
> > rest of the code in another. Because, the static tables itself occupy 1.2k LoC
> > now and it is going to grow. So let's keep them in a single file to avoid mixing
> > it with rest of the driver code.
> 
> My 2c is that this is mostly useless. The headers are for sharing, not
> for moving the data out of the .c files. Not to mention that the
> driver code comes after the tables.
> I'd really suggest starting such a move with separating common parts
> of all the QMP drivers.
> 

Makes sense.

Can, please propose a separate series if you want to pursue the effort.
Also, I'd say that instead of moving the tables to a header (which defeats the
purpose of the header), the tables can be moved to a separate .c file. Like,

phy-qcom-qmp-ufs-tables.c
phy-qcom-qmp-ufs.c

Btw, why do we have "phy-qcom" prefix inside drivers/phy/qualcomm/?

- Mani

> >
> > - Mani
> >
> > > If you really would like to clean up the QMP drivers, please consider
> > > splitting _common_ parts. But at this point I highly doubt that it is
> > > possible in a useful way.
> 
> 
> -- 
> With best wishes
> Dmitry

-- 
மணிவண்ணன் சதாசிவம்