[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Date: Wed, 10 Oct 2018 21:46:33 +0800
From: cang@...eaurora.org
To: Doug Anderson <dianders@...omium.org>
Cc: subhashj@...eaurora.org, Asutosh Das <asutoshd@...eaurora.org>,
Vivek Gautam <vivek.gautam@...eaurora.org>,
Rajendra Nayak <rnayak@...eaurora.org>,
Evan Green <evgreen@...omium.org>,
Vinayak Holikatti <vinholikatti@...il.com>,
jejb@...ux.vnet.ibm.com,
"Martin K. Petersen" <martin.petersen@...cle.com>,
linux-scsi@...r.kernel.org, venkatg@...eaurora.org,
LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 1/1] scsi: ufs: make UFS Tx lane1 clock optional
Hi Doug,
Really thank you for your review.
On 2018-10-10 05:56, Doug Anderson wrote:
> Hi,
>
> On Sun, Oct 7, 2018 at 9:34 PM Can Guo <cang@...eaurora.org> wrote:
>>
>> From: Venkat Gopalakrishnan <venkatg@...eaurora.org>
>>
>> The UFS Tx lane1 clock could be muxed, hence keep it optional by
>> ignoring
>> it if it is not provided in device tree.
>
> Thanks for the updated commit message. This makes much more sense now!
> :-)
>
>
Yeah, sorry for making you guys confused. My bad previously.
>> @@ -113,10 +110,10 @@ static void ufs_qcom_disable_lane_clks(struct
>> ufs_qcom_host *host)
>> if (!host->is_lane_clks_enabled)
>> return;
>>
>> - if (host->hba->lanes_per_direction > 1)
>> + if (host->tx_l1_sync_clk)
>> clk_disable_unprepare(host->tx_l1_sync_clk);
>
> I don't believe you need the "if". A NULL clock is by definition OK
> to enable / disable which is designed to make optional clocks easier
> to deal with.
>
>
You are right, I checked the implemention, clock enable/disable func
would
bail out if clk is NULL just as your comments.
>> clk_disable_unprepare(host->tx_l0_sync_clk);
>> - if (host->hba->lanes_per_direction > 1)
>> + if (host->rx_l1_sync_clk)
>> clk_disable_unprepare(host->rx_l1_sync_clk);
>
> optional: Technically this part of the patch wasn't actually needed,
> right? "rx_l1_sync_clk" is not optional so that means that
> "rx_l1_sync_clk" should be non-NULL exactly when lanes_per_direction >
> 1.
>
> ...but actually I'm fine with keeping this part of your patch for
> symmetry. Especially since you can leverage the
> clk_disable_unprepare(NULL) to simplify your code. Please mention in
> your commit message that this is a cleanup and just for symmetry.
> Probably also remove the "if" tests that are guarding
> ufs_qcom_host_clk_enable().
>
>
Sure, got it.
>> clk_disable_unprepare(host->rx_l0_sync_clk);
>>
>> @@ -141,24 +138,21 @@ static int ufs_qcom_enable_lane_clks(struct
>> ufs_qcom_host *host)
>> if (err)
>> goto disable_rx_l0;
>>
>> - if (host->hba->lanes_per_direction > 1) {
>> + if (host->rx_l1_sync_clk) {
>
> Similar: the above change isn't required but I'm OK if you want to
> make this change for symmetry / to cleanup clock handling. Please
> mention in your commit message.
>
>
Sure, shall do so.
>> + /* The tx lane1 clk could be muxed, hence keep this optional
>> */
>> + if (host->tx_l1_sync_clk)
>> + ufs_qcom_host_clk_enable(dev, "tx_lane1_sync_clk",
>> + host->tx_l1_sync_clk);
>
> If "host->tx_l1_sync_clk" is provided then you should still check the
> return value of ufs_qcom_host_clk_enable(), right? ...so please leave
> the error path here.
>
>
Yeah... you are right.
>> @@ -174,23 +168,33 @@ static int ufs_qcom_init_lane_clks(struct
>> ufs_qcom_host *host)
>>
>> err = ufs_qcom_host_clk_get(dev,
>> "rx_lane0_sync_clk", &host->rx_l0_sync_clk);
>> - if (err)
>> + if (err) {
>> + dev_err(dev, "%s: failed to get rx_lane0_sync_clk, err
>> %d",
>> + __func__, err);
>
> nit: including "__func__" in dev_xxx() calls is discouraged. The
> "dev_xxx" calls already print the device name and the string within a
> given device driver should be unique enough so __func__ just adds crap
> to the logs. If you really feel that __func__ adds something for you,
> try posting up a patch to make all "dev_err" functions include
> __func__. ...but I think you'd probably be rejected.
>
> suggestion: you'd save a few lines of code and simplify your probe if
> you just passed an "optional" bool to the ufs_qcom_host_clk_get() that
> controlled the printout.
>
>
Good idea!
>> - err = ufs_qcom_host_clk_get(dev, "tx_lane1_sync_clk",
>> - &host->tx_l1_sync_clk);
>> + /* The tx lane1 clk could be muxed, hence keep this
>> optional */
>> + ufs_qcom_host_clk_get(dev, "tx_lane1_sync_clk",
>> + &host->tx_l1_sync_clk);
>
> To be technically correct you should actually check the error value
> returned by ufs_qcom_host_clk_get(). Specifically figure out what the
> error value is when "tx_lane1_sync_clk" isn't specified and check for
> that.
>
> ...one reason this matters is -EPROBE_DEFER. In theory devm_clk_get()
> could return -EPROBE_DEFER. In such a case you'd want to exit the
> probe, not continue on. It's also just good coding practice to handle
> just the error you want though so that if the function returned some
> other failing case you'd propagate it down instead of eating it.
>
> If you passed "optional" to ufs_qcom_host_clk_get() as suggested above
> you could put this error-code checking in ufs_qcom_host_clk_get() and
> return 0 from that function if an optional clock was found to not
> exist.
>
> -Doug
I think I understand it. Thanks a lot for the thorough review and
suggestions.
-Can Guo
Powered by blists - more mailing lists