On 30/08/2022 10:38, Johan Hovold wrote:
On Thu, Aug 25, 2022 at 01:50:40PM +0300, Dmitry Baryshkov wrote:
SM8250 configuration tables are split into two parts: the common one and
the PHY-specific tables. Make this split more formal. Rather than having
a blind renamed copy of all QMP table fields, add separate struct
qmp_phy_cfg_tables and add two instances of this structure to the struct
qmp_phy_cfg. Later on this will be used to support different PHY modes
(RC vs EP).
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>
---
drivers/phy/qualcomm/phy-qcom-qmp-pcie.c | 141 +++++++++++++----------
1 file changed, 83 insertions(+), 58 deletions(-)
diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
index c84846020272..60cbd2eae346 100644
--- a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
+++ b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
@@ -1346,34 +1346,33 @@ static const struct qmp_phy_init_tbl sm8450_qmp_gen4x2_pcie_pcs_misc_tbl[] = {
struct qmp_phy;
-/* struct qmp_phy_cfg - per-PHY initialization config */
-struct qmp_phy_cfg {
- /* phy-type - PCIE/UFS/USB */
- unsigned int type;
- /* number of lanes provided by phy */
- int nlanes;
-
- /* Init sequence for PHY blocks - serdes, tx, rx, pcs */
+struct qmp_phy_cfg_tables {
const struct qmp_phy_init_tbl *serdes_tbl;
int serdes_tbl_num;
- const struct qmp_phy_init_tbl *serdes_tbl_sec;
- int serdes_tbl_num_sec;
const struct qmp_phy_init_tbl *tx_tbl;
int tx_tbl_num;
- const struct qmp_phy_init_tbl *tx_tbl_sec;
- int tx_tbl_num_sec;
const struct qmp_phy_init_tbl *rx_tbl;
int rx_tbl_num;
- const struct qmp_phy_init_tbl *rx_tbl_sec;
- int rx_tbl_num_sec;
const struct qmp_phy_init_tbl *pcs_tbl;
int pcs_tbl_num;
- const struct qmp_phy_init_tbl *pcs_tbl_sec;
- int pcs_tbl_num_sec;
const struct qmp_phy_init_tbl *pcs_misc_tbl;
int pcs_misc_tbl_num;
- const struct qmp_phy_init_tbl *pcs_misc_tbl_sec;
- int pcs_misc_tbl_num_sec;
+};
+
+/* struct qmp_phy_cfg - per-PHY initialization config */
+struct qmp_phy_cfg {
+ /* phy-type - PCIE/UFS/USB */
+ unsigned int type;
+ /* number of lanes provided by phy */
+ int nlanes;
+
+ /* Init sequence for PHY blocks - serdes, tx, rx, pcs */
+ struct qmp_phy_cfg_tables primary;
+ /*
+ * Init sequence for PHY blocks, providing additional register
+ * programming. Unless required it can be left omitted.
+ */
+ struct qmp_phy_cfg_tables secondary;
I haven't really had time to look at this series yet, but it seems the
way these structures are named and organised could be improved.
First, "primary" and "secondary" says nothing about what these
structures are and the names are also unnecessarily long.
I started with 'pri'/'sec', but was asked to improve them. Any sensible
suggestion is welcomed here. 'main'/'aux'?
Regarding 'saying nothing'. It's true, initially it just followed the
existing split for the sm8250. Then I added the `secondary_ep` table.
Second, once you add a containing structure, the "_tbl" suffixes could
be removed (e.g. in "pcs_misc_tbl").
Doing something like below should make the code cleaner:
struct qmp_phy_cfg_tables {
const struct qmp_phy_init_tbl *serdes;
const struct qmp_phy_init_tbl *tx;
...
};
struct qmp_phy_cfg {
struct qmp_phy_cfg_tables tbls1;
struct qmp_phy_cfg_tables tbls2;
...
};
as the tables can be referred to as
cfg.tbls2.serdes
instead of
cfg.secondary.serdes_tbl;
Nice suggestion, I'll implement it for v3 if Vinod doesn't object.
Johan
--
With best wishes
Dmitry
