On Mon, 11 Sept 2023 at 09:01, Can Guo <[email protected]> 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 <[email protected]>
> ---
> 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.
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
On Thu, Sep 14, 2023 at 03:28:59PM +0300, Dmitry Baryshkov wrote:
> On Mon, 11 Sept 2023 at 09:01, Can Guo <[email protected]> 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 <[email protected]>
> > ---
> > 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.
- 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
--
மணிவண்ணன் சதாசிவம்
On Tue, 19 Sept 2023 at 15:15, Manivannan Sadhasivam <[email protected]> wrote:
>
> On Thu, Sep 14, 2023 at 03:28:59PM +0300, Dmitry Baryshkov wrote:
> > On Mon, 11 Sept 2023 at 09:01, Can Guo <[email protected]> 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 <[email protected]>
> > > ---
> > > 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.
>
> - 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
On Wed, Sep 20, 2023 at 01:30:21AM +0300, Dmitry Baryshkov wrote:
> On Tue, 19 Sept 2023 at 15:15, Manivannan Sadhasivam <[email protected]> wrote:
> >
> > On Thu, Sep 14, 2023 at 03:28:59PM +0300, Dmitry Baryshkov wrote:
> > > On Mon, 11 Sept 2023 at 09:01, Can Guo <[email protected]> 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 <[email protected]>
> > > > ---
> > > > 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
--
மணிவண்ணன் சதாசிவம்
On Wed, Sep 20, 2023 at 12:19:23PM +0200, Manivannan Sadhasivam wrote:
[..]
> Btw, why do we have "phy-qcom" prefix inside drivers/phy/qualcomm/?
>
That would be a historical artifact, but it does provide nice
namespacing for the generated .ko files - and iirc mkinitcpio doesn't
automatically pick these up, so changing it would cause issues for our
users.
Regards,
Bjorn
On Tue, Sep 19, 2023 at 02:15:24PM +0200, Manivannan Sadhasivam wrote:
> On Thu, Sep 14, 2023 at 03:28:59PM +0300, Dmitry Baryshkov wrote:
> > On Mon, 11 Sept 2023 at 09:01, Can Guo <[email protected]> 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 <[email protected]>
> > > ---
> > > 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.
>
To me, the problem with the current layout is that we have:
* structures
* data
* structures
* implementation
So to find the second structures you need to jump somewhere in the
middle of the file. If we shift those up, it's easy to jump between the
three portions of the file.
Regards,
Bjorn
On Thu, Sep 21, 2023 at 07:02:20AM -0700, Bjorn Andersson wrote:
> On Wed, Sep 20, 2023 at 12:19:23PM +0200, Manivannan Sadhasivam wrote:
> [..]
> > Btw, why do we have "phy-qcom" prefix inside drivers/phy/qualcomm/?
> >
>
> That would be a historical artifact, but it does provide nice
> namespacing for the generated .ko files - and iirc mkinitcpio doesn't
> automatically pick these up, so changing it would cause issues for our
> users.
>
Well, my concern is only with the driver name and not with the module name.
And yes, module name should have the proper prefix (unlike "msm" for the
DRM module).
- Mani
> Regards,
> Bjorn
--
மணிவண்ணன் சதாசிவம்