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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <2e93e810adbc252cd630d89bb753ec81@codeaurora.org>
Date:   Sun, 08 Jan 2017 00:11:14 +0530
From:   vivek.gautam@...eaurora.org
To:     Bjorn Andersson <bjorn.andersson@...aro.org>
Cc:     robh+dt@...nel.org, kishon@...com, sboyd@...eaurora.org,
        linux-kernel@...r.kernel.org, devicetree@...r.kernel.org,
        mark.rutland@....com, srinivas.kandagatla@...aro.org,
        linux-arm-msm@...r.kernel.org, linux-arm-msm-owner@...r.kernel.org
Subject: Re: [PATCH v3 4/4] phy: qcom-qmp: new qmp phy driver for
 qcom-chipsets

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@...r.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html