Re: [PATCH v3 4/4] phy: qcom-qmp: new qmp phy driver for qcom-chipsets

From: vivek . gautam
Date: Sat Jan 07 2017 - 13:41:23 EST


On 2017-01-07 02:47, Bjorn Andersson wrote:
On Fri 06 Jan 01:47 PST 2017, Vivek Gautam wrote:

> > +static int qcom_qmp_phy_com_init(struct qcom_qmp_phy *qphy)
> > +{
> > + const struct qmp_phy_cfg *cfg = qphy->cfg;
> > + void __iomem *serdes = qphy->serdes;
> > + int ret;
> > +
> > + mutex_lock(&qphy->phy_mutex);
> > + if (qphy->init_count++) {
> > + mutex_unlock(&qphy->phy_mutex);
> > + return 0;
> > + }
> As far as I can see phy_init() and phy_exit() already keep reference
> count on the initialization and you only call this function from
> phy_ops->init, so you should be able to drop this.
This is an intermediary function that does the common block initialization.
PHYs like PCIe have a separate common block (apart from SerDes)
for all phy channels. We shouldn't program this common block
multiple times for each channel. That's why this init_count.


You're right!

Unfortunately it took me several minutes to wrap my head around the phy
vs multi-lane and I have a really hard time keeping "qcom_qmp_phy" and
"qmp_phy_desc" apart throughout the driver.

If I understand correctly the qcom_qmp_phy is the context representing a
"QMP block", while this is a PHY block it's not actually the phy in
Linux eyes. The qcom_phy_desc represents a "QMP lane", which in Linux
eyes is the phys, but as we think of QMP as the PHY this confused me.

That's correct. The qcom_qmp_phy structure represents the QMP phy block
as a whole and not the individual phy lane instances. These phy lanes
are represented by qcom_phy_desc, and are the actual PHYs in Linux eyes.


How about naming them "struct qmp" and "struct qmp_lane" (or possibly
qmp_phy) instead? That way we remove the confusion of QMP PHY vs Linux
PHY and we make the lane part explicit.

Sure, this sounds good to me - "struct qmp" and "struct qmp_phy" (will
call the respective variables as qblk for qmp block (struct qmp) and
qphy for struct qmp_phy).
Thanks for pointing out. Will change them.

Regards
Vivek


Regards,
Bjorn

--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html