linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: "Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>
To: Ryder Lee <ryder.lee@mediatek.com>
Cc: Thierry Reding <thierry.reding@gmail.com>,
	Rob Herring <robh+dt@kernel.org>,
	linux-pwm@vger.kernel.org, Weijie Gao <weijie.gao@mediatek.com>,
	Roy Luo <cheng-hao.luo@mediatek.com>,
	devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org,
	linux-mediatek@lists.infradead.org,
	John Crispin <john@phrozen.org>,
	kernel@pengutronix.de, linux-clk@vger.kernel.org,
	Michael Turquette <mturquette@baylibre.com>,
	Stephen Boyd <sboyd@kernel.org>
Subject: Re: [resend PATCH 1/3] pwm: mediatek: drop flag 'has_clks'
Date: Tue, 13 Nov 2018 10:52:10 +0100	[thread overview]
Message-ID: <20181113095210.mh3iy5tcsu6w6tem@pengutronix.de> (raw)
In-Reply-To: <4c9044427b1aab373acd6ac76f0c905e2be79784.1542074855.git.ryder.lee@mediatek.com>

Hello,

On Tue, Nov 13, 2018 at 10:08:22AM +0800, Ryder Lee wrote:
> The flag 'has_clks' and related checks are superfluous as the CCF
> subsystem does this for you.

I'd write instead:

	Handle optional clocks by using NULL as clk instead of a
	separate bool field in the device's platform data.

There is a semantic difference this patch introduces (i.e. if on mt2712
there are no provided clocks, the driver now successfully binds while
before it failed with -ENOENT. And for mt7628 it's the other way round). 
IMHO this should be noted in the commit log, too. Otherwise it sounds as
if this patch was just an optimisation without side effects.

> ---
>  drivers/pwm/pwm-mediatek.c | 20 +++++---------------
>  1 file changed, 5 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-mediatek.c b/drivers/pwm/pwm-mediatek.c
> index eb6674c..9400c41 100644
> --- a/drivers/pwm/pwm-mediatek.c
> +++ b/drivers/pwm/pwm-mediatek.c
> @@ -57,7 +57,6 @@ enum {
>  struct mtk_pwm_platform_data {
>  	unsigned int num_pwms;
>  	bool pwm45_fixup;
> -	bool has_clks;
>  };
>  
>  /**
> @@ -87,9 +86,6 @@ static int mtk_pwm_clk_enable(struct pwm_chip *chip, struct pwm_device *pwm)
>  	struct mtk_pwm_chip *pc = to_mtk_pwm_chip(chip);
>  	int ret;
>  
> -	if (!pc->soc->has_clks)
> -		return 0;
> -
>  	ret = clk_prepare_enable(pc->clks[MTK_CLK_TOP]);
>  	if (ret < 0)
>  		return ret;
> @@ -116,9 +112,6 @@ static void mtk_pwm_clk_disable(struct pwm_chip *chip, struct pwm_device *pwm)
>  {
>  	struct mtk_pwm_chip *pc = to_mtk_pwm_chip(chip);
>  
> -	if (!pc->soc->has_clks)
> -		return;
> -
>  	clk_disable_unprepare(pc->clks[MTK_CLK_PWM1 + pwm->hwpwm]);
>  	clk_disable_unprepare(pc->clks[MTK_CLK_MAIN]);
>  	clk_disable_unprepare(pc->clks[MTK_CLK_TOP]);
> @@ -246,12 +239,13 @@ static int mtk_pwm_probe(struct platform_device *pdev)
>  	if (IS_ERR(pc->regs))
>  		return PTR_ERR(pc->regs);
>  
> -	for (i = 0; i < data->num_pwms + 2 && pc->soc->has_clks; i++) {
> +	for (i = 0; i < data->num_pwms + 2; i++) {
>  		pc->clks[i] = devm_clk_get(&pdev->dev, mtk_pwm_clk_name[i]);
>  		if (IS_ERR(pc->clks[i])) {
> -			dev_err(&pdev->dev, "clock: %s fail: %ld\n",
> -				mtk_pwm_clk_name[i], PTR_ERR(pc->clks[i]));
> -			return PTR_ERR(pc->clks[i]);
> +			if (PTR_ERR(pc->clks[i]) == -EPROBE_DEFER)
> +				return -EPROBE_DEFER;
> +
> +			pc->clks[i] = NULL;

I'd prefer the following instead:

	pc->clks[i] = devm_clk_get(&pdev->dev, mtk_pwm_clk_name[i]);
	if (IS_ERR(pc->clks[i])) {
		if (PTR_ERR(pc->clks[i]) == -ENODEV) {
			pc->clks[i] = NULL;
		} else {
			if (PTR_ERR(pc->clks[i]) == -EPROBE_DEFER)
				dev_err(...);
			return PTR_ERR(pc->clks[i]);
		}

This way you only handle "There is no such clock" and are not ignoring
say an IO error.

I wonder if it would make sense to introduce functions like:

	struct clk *clk_get_optional(struct device *dev, const char *id)

that return NULL instead of ERR_PTR(-ENODEV).

Then the above would simplify to:

	pc->clks[i] = devm_clk_get_optional(&pdev->dev, mtk_pwm_clk_name[i]);
	if (IS_ERR(pc->clks[i]) {
		if (PTR_ERR(pc->clks[i]) == -EPROBE_DEFER)
			dev_err(...);
		return PTR_ERR(pc->clks[i]);
	}

(added the clk people to Cc for this question).

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

  parent reply	other threads:[~2018-11-13  9:52 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-11-13  2:08 Ryder Lee
2018-11-13  2:08 ` [resend PATCH 2/3] pwm: mediatek: add pwm support for MT7629 SoC Ryder Lee
2018-11-15 19:20   ` Sean Wang
2018-11-13  2:08 ` [resend PATCH 3/3] dt-bindings: pwm: update bindings " Ryder Lee
2018-11-13  9:55   ` Uwe Kleine-König
2018-11-13  9:52 ` Uwe Kleine-König [this message]
2018-11-13 18:00   ` [resend PATCH 1/3] pwm: mediatek: drop flag 'has_clks' Stephen Boyd
2018-11-14 12:47 ` Thierry Reding
2018-11-14 13:27   ` John Crispin
2018-11-15  2:16     ` Ryder Lee
2018-11-16  6:47   ` Uwe Kleine-König
2018-11-16 10:22     ` Thierry Reding

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20181113095210.mh3iy5tcsu6w6tem@pengutronix.de \
    --to=u.kleine-koenig@pengutronix.de \
    --cc=cheng-hao.luo@mediatek.com \
    --cc=devicetree@vger.kernel.org \
    --cc=john@phrozen.org \
    --cc=kernel@pengutronix.de \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-clk@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mediatek@lists.infradead.org \
    --cc=linux-pwm@vger.kernel.org \
    --cc=mturquette@baylibre.com \
    --cc=robh+dt@kernel.org \
    --cc=ryder.lee@mediatek.com \
    --cc=sboyd@kernel.org \
    --cc=thierry.reding@gmail.com \
    --cc=weijie.gao@mediatek.com \
    --subject='Re: [resend PATCH 1/3] pwm: mediatek: drop flag '\''has_clks'\''' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).