Re: [PATCH v4 08/16] phy: qcom-qmp-pcie: use shorter tables identifier

[Johan Hovold <johan@kernel.org>]

On Sat, Nov 05, 2022 at 05:37:27PM +0530, Vinod Koul wrote:
> On 28-10-22, 15:35, Johan Hovold wrote:
> > In preparation for adding a new helper function that initialises all the
> > registers in one place, use a shorter name for the configuration-tables
> > pointer so that it becomes easier to see which table is being processed.
> > 
> > Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> > ---
> >  drivers/phy/qualcomm/phy-qcom-qmp-pcie.c | 26 ++++++++++++------------
> >  1 file changed, 13 insertions(+), 13 deletions(-)
> > 
> > diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
> > index dd7e72424fc0..791ed7ef0eab 100644
> > --- a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
> > +++ b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
> > @@ -1820,17 +1820,17 @@ static void qmp_pcie_configure(void __iomem *base,
> >  	qmp_pcie_configure_lane(base, tbl, num, 0xff);
> >  }
> >  
> > -static void qmp_pcie_serdes_init(struct qmp_pcie *qmp, const struct qmp_phy_cfg_tables *tables)
> > +static void qmp_pcie_serdes_init(struct qmp_pcie *qmp, const struct qmp_phy_cfg_tables *tbls)
> >  {
> >  	void __iomem *serdes = qmp->serdes;
> >  
> > -	if (!tables)
> > +	if (!tbls)
> 
> I am not sure saving 2 chars really helps either with readability or
> helps with shorter name, but for now am not going to nit pick this...

As I tried to explain earlier, the reason for doing using a shorter
identifier is to improve readability of the initialisation sequence in
the new helper function added by the next patch.

It's more obvious if you compare the end result, but you can also see it
some degree for the tx/tx tables below.

> 
> >  		return;
> >  
> > -	qmp_pcie_configure(serdes, tables->serdes, tables->serdes_num);
> > +	qmp_pcie_configure(serdes, tbls->serdes, tbls->serdes_num);
> >  }
> >  
> > -static void qmp_pcie_lanes_init(struct qmp_pcie *qmp, const struct qmp_phy_cfg_tables *tables)
> > +static void qmp_pcie_lanes_init(struct qmp_pcie *qmp, const struct qmp_phy_cfg_tables *tbls)
> >  {
> >  	const struct qmp_phy_cfg *cfg = qmp->cfg;
> >  	void __iomem *tx = qmp->tx;
> > @@ -1838,28 +1838,28 @@ static void qmp_pcie_lanes_init(struct qmp_pcie *qmp, const struct qmp_phy_cfg_t
> >  	void __iomem *tx2 = qmp->tx2;
> >  	void __iomem *rx2 = qmp->rx2;
> >  
> > -	if (!tables)
> > +	if (!tbls)
> >  		return;
> >  
> > -	qmp_pcie_configure_lane(tx, tables->tx, tables->tx_num, 1);
> > -	qmp_pcie_configure_lane(rx, tables->rx, tables->rx_num, 1);
> > +	qmp_pcie_configure_lane(tx, tbls->tx, tbls->tx_num, 1);
> > +	qmp_pcie_configure_lane(rx, tbls->rx, tbls->rx_num, 1);
> >  
> >  	if (cfg->lanes >= 2) {
> > -		qmp_pcie_configure_lane(tx2, tables->tx, tables->tx_num, 2);
> > -		qmp_pcie_configure_lane(rx2, tables->rx, tables->rx_num, 2);
> > +		qmp_pcie_configure_lane(tx2, tbls->tx, tbls->tx_num, 2);
> > +		qmp_pcie_configure_lane(rx2, tbls->rx, tbls->rx_num, 2);

Here 'tbls' is no longer significantly longer than 'tx' or 'tx_num'
which I find improves readability.

After this series, there will also be a function call that uses
arguments from the configuration struct and this one is consistently
referred to as 'cfg' rather than 'configuration' as the latter would
likewise hurt readability.

Also note that the driver already used 'tbl' consistently (e.g. see
qmp_pcie_configure() in the context at the top of the diff) until the
new tables struct was recently added.

Johan

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