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

[Manivannan Sadhasivam]

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

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