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

From: Manivannan Sadhasivam <mani@kernel.org>
To: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Cc: Can Guo <quic_cang@quicinc.com>,
	quic_nguyenb@quicinc.com, quic_nitirawa@quicinc.com,
	martin.petersen@oracle.com, linux-scsi@vger.kernel.org,
	Andy Gross <agross@kernel.org>,
	Bjorn Andersson <andersson@kernel.org>,
	Konrad Dybcio <konrad.dybcio@linaro.org>,
	Vinod Koul <vkoul@kernel.org>,
	Kishon Vijay Abraham I <kishon@kernel.org>,
	Philipp Zabel <p.zabel@pengutronix.de>,
	open list <linux-kernel@vger.kernel.org>,
	"open list:ARM/QUALCOMM SUPPORT" <linux-arm-msm@vger.kernel.org>,
	"open list:GENERIC PHY FRAMEWORK" <linux-phy@lists.infradead.org>
Subject: Re: [PATCH 4/6] phy: qualcomm: phy-qcom-qmp-ufs: Move data structs and setting tables to header
Date: Wed, 20 Sep 2023 12:19:23 +0200	[thread overview]
Message-ID: <20230920101923.GG4732@thinkpad> (raw)
In-Reply-To: <CAA8EJpoqRBxS7HJrFdRAvv677hKJw+d_K+fN_4ABDrj+68r28w@mail.gmail.com>

On Wed, Sep 20, 2023 at 01:30:21AM +0300, Dmitry Baryshkov wrote:
> On Tue, 19 Sept 2023 at 15:15, Manivannan Sadhasivam <mani@kernel.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@quicinc.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@quicinc.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

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

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy