Messages in this thread | | | Date | Fri, 6 Jan 2017 13:17:22 -0800 | From | Bjorn Andersson <> | Subject | Re: [PATCH v3 4/4] phy: qcom-qmp: new qmp phy driver for qcom-chipsets |
| |
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.
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.
Regards, Bjorn
| |