linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [resend PATCH 1/3] pwm: mediatek: drop flag 'has_clks'
@ 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
                   ` (3 more replies)
  0 siblings, 4 replies; 12+ messages in thread
From: Ryder Lee @ 2018-11-13  2:08 UTC (permalink / raw)
  To: Thierry Reding
  Cc: Rob Herring, linux-pwm, Weijie Gao, Roy Luo, devicetree,
	linux-kernel, linux-arm-kernel, linux-mediatek, Ryder Lee,
	John Crispin

The flag 'has_clks' and related checks are superfluous as the CCF
subsystem does this for you.

Cc: John Crispin <john@phrozen.org>
Signed-off-by: Ryder Lee <ryder.lee@mediatek.com>
---
 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;
 		}
 	}
 
@@ -281,25 +275,21 @@ static int mtk_pwm_remove(struct platform_device *pdev)
 static const struct mtk_pwm_platform_data mt2712_pwm_data = {
 	.num_pwms = 8,
 	.pwm45_fixup = false,
-	.has_clks = true,
 };
 
 static const struct mtk_pwm_platform_data mt7622_pwm_data = {
 	.num_pwms = 6,
 	.pwm45_fixup = false,
-	.has_clks = true,
 };
 
 static const struct mtk_pwm_platform_data mt7623_pwm_data = {
 	.num_pwms = 5,
 	.pwm45_fixup = true,
-	.has_clks = true,
 };
 
 static const struct mtk_pwm_platform_data mt7628_pwm_data = {
 	.num_pwms = 4,
 	.pwm45_fixup = true,
-	.has_clks = false,
 };
 
 static const struct of_device_id mtk_pwm_of_match[] = {
-- 
1.9.1


^ permalink raw reply	[flat|nested] 12+ messages in thread

* [resend PATCH 2/3] pwm: mediatek: add pwm support for MT7629 SoC
  2018-11-13  2:08 [resend PATCH 1/3] pwm: mediatek: drop flag 'has_clks' Ryder Lee
@ 2018-11-13  2:08 ` 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
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 12+ messages in thread
From: Ryder Lee @ 2018-11-13  2:08 UTC (permalink / raw)
  To: Thierry Reding
  Cc: Rob Herring, linux-pwm, Weijie Gao, Roy Luo, devicetree,
	linux-kernel, linux-arm-kernel, linux-mediatek, Ryder Lee

This adds pwm controller support for MT7629 SoC.

Signed-off-by: Ryder Lee <ryder.lee@mediatek.com>
---
 drivers/pwm/pwm-mediatek.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/pwm/pwm-mediatek.c b/drivers/pwm/pwm-mediatek.c
index 9400c41..4ed95e5 100644
--- a/drivers/pwm/pwm-mediatek.c
+++ b/drivers/pwm/pwm-mediatek.c
@@ -292,11 +292,16 @@ static int mtk_pwm_remove(struct platform_device *pdev)
 	.pwm45_fixup = true,
 };
 
+static const struct mtk_pwm_platform_data mt7629_pwm_data = {
+	.num_pwms = 1,
+};
+
 static const struct of_device_id mtk_pwm_of_match[] = {
 	{ .compatible = "mediatek,mt2712-pwm", .data = &mt2712_pwm_data },
 	{ .compatible = "mediatek,mt7622-pwm", .data = &mt7622_pwm_data },
 	{ .compatible = "mediatek,mt7623-pwm", .data = &mt7623_pwm_data },
 	{ .compatible = "mediatek,mt7628-pwm", .data = &mt7628_pwm_data },
+	{ .compatible = "mediatek,mt7629-pwm", .data = &mt7629_pwm_data },
 	{ },
 };
 MODULE_DEVICE_TABLE(of, mtk_pwm_of_match);
-- 
1.9.1


^ permalink raw reply	[flat|nested] 12+ messages in thread

* [resend PATCH 3/3] dt-bindings: pwm: update bindings for MT7629 SoC
  2018-11-13  2:08 [resend PATCH 1/3] pwm: mediatek: drop flag 'has_clks' Ryder Lee
  2018-11-13  2:08 ` [resend PATCH 2/3] pwm: mediatek: add pwm support for MT7629 SoC Ryder Lee
@ 2018-11-13  2:08 ` Ryder Lee
  2018-11-13  9:55   ` Uwe Kleine-König
  2018-11-13  9:52 ` [resend PATCH 1/3] pwm: mediatek: drop flag 'has_clks' Uwe Kleine-König
  2018-11-14 12:47 ` Thierry Reding
  3 siblings, 1 reply; 12+ messages in thread
From: Ryder Lee @ 2018-11-13  2:08 UTC (permalink / raw)
  To: Thierry Reding
  Cc: Rob Herring, linux-pwm, Weijie Gao, Roy Luo, devicetree,
	linux-kernel, linux-arm-kernel, linux-mediatek, Ryder Lee

This updates bindings for MT7629 pwm controller.

Signed-off-by: Ryder Lee <ryder.lee@mediatek.com>
---
 Documentation/devicetree/bindings/pwm/pwm-mediatek.txt | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/pwm/pwm-mediatek.txt b/Documentation/devicetree/bindings/pwm/pwm-mediatek.txt
index 991728c..4a2885b 100644
--- a/Documentation/devicetree/bindings/pwm/pwm-mediatek.txt
+++ b/Documentation/devicetree/bindings/pwm/pwm-mediatek.txt
@@ -6,6 +6,7 @@ Required properties:
    - "mediatek,mt7622-pwm": found on mt7622 SoC.
    - "mediatek,mt7623-pwm": found on mt7623 SoC.
    - "mediatek,mt7628-pwm": found on mt7628 SoC.
+   - "mediatek,mt7629-pwm": found on mt7629 SoC.
  - reg: physical base address and length of the controller's registers.
  - #pwm-cells: must be 2. See pwm.txt in this directory for a description of
    the cell format.
-- 
1.9.1


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [resend PATCH 1/3] pwm: mediatek: drop flag 'has_clks'
  2018-11-13  2:08 [resend PATCH 1/3] pwm: mediatek: drop flag 'has_clks' Ryder Lee
  2018-11-13  2:08 ` [resend PATCH 2/3] pwm: mediatek: add pwm support for MT7629 SoC Ryder Lee
  2018-11-13  2:08 ` [resend PATCH 3/3] dt-bindings: pwm: update bindings " Ryder Lee
@ 2018-11-13  9:52 ` Uwe Kleine-König
  2018-11-13 18:00   ` Stephen Boyd
  2018-11-14 12:47 ` Thierry Reding
  3 siblings, 1 reply; 12+ messages in thread
From: Uwe Kleine-König @ 2018-11-13  9:52 UTC (permalink / raw)
  To: Ryder Lee
  Cc: Thierry Reding, Rob Herring, linux-pwm, Weijie Gao, Roy Luo,
	devicetree, linux-kernel, linux-arm-kernel, linux-mediatek,
	John Crispin, kernel, linux-clk, Michael Turquette, Stephen Boyd

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/  |

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [resend PATCH 3/3] dt-bindings: pwm: update bindings for MT7629 SoC
  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
  0 siblings, 0 replies; 12+ messages in thread
From: Uwe Kleine-König @ 2018-11-13  9:55 UTC (permalink / raw)
  To: Ryder Lee
  Cc: Thierry Reding, Rob Herring, linux-pwm, Weijie Gao, Roy Luo,
	devicetree, linux-kernel, linux-arm-kernel, linux-mediatek

On Tue, Nov 13, 2018 at 10:08:24AM +0800, Ryder Lee wrote:
> This updates bindings for MT7629 pwm controller.
> 
> Signed-off-by: Ryder Lee <ryder.lee@mediatek.com>
> ---
>  Documentation/devicetree/bindings/pwm/pwm-mediatek.txt | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/Documentation/devicetree/bindings/pwm/pwm-mediatek.txt b/Documentation/devicetree/bindings/pwm/pwm-mediatek.txt
> index 991728c..4a2885b 100644
> --- a/Documentation/devicetree/bindings/pwm/pwm-mediatek.txt
> +++ b/Documentation/devicetree/bindings/pwm/pwm-mediatek.txt
> @@ -6,6 +6,7 @@ Required properties:
>     - "mediatek,mt7622-pwm": found on mt7622 SoC.
>     - "mediatek,mt7623-pwm": found on mt7623 SoC.
>     - "mediatek,mt7628-pwm": found on mt7628 SoC.
> +   - "mediatek,mt7629-pwm": found on mt7629 SoC.
>   - reg: physical base address and length of the controller's registers.
>   - #pwm-cells: must be 2. See pwm.txt in this directory for a description of
>     the cell format.

Does the mt7629 need clocks? I'd suggest to move the clocks and
clock-names description to a section "Optional properties" to match what
is implemented in patch 1 of this series.

Best regards
Uwe

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [resend PATCH 1/3] pwm: mediatek: drop flag 'has_clks'
  2018-11-13  9:52 ` [resend PATCH 1/3] pwm: mediatek: drop flag 'has_clks' Uwe Kleine-König
@ 2018-11-13 18:00   ` Stephen Boyd
  0 siblings, 0 replies; 12+ messages in thread
From: Stephen Boyd @ 2018-11-13 18:00 UTC (permalink / raw)
  To: Ryder Lee, Uwe Kleine-König
  Cc: Thierry Reding, Rob Herring, linux-pwm, Weijie Gao, Roy Luo,
	devicetree, linux-kernel, linux-arm-kernel, linux-mediatek,
	John Crispin, kernel, linux-clk, Michael Turquette

Quoting Uwe Kleine-König (2018-11-13 01:52:10)
> 
> 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).
> 

Such a patch is already on the list and not getting much review.

http://lkml.kernel.org/r/1535724443-21150-1-git-send-email-phil.edworthy@renesas.com


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [resend PATCH 1/3] pwm: mediatek: drop flag 'has_clks'
  2018-11-13  2:08 [resend PATCH 1/3] pwm: mediatek: drop flag 'has_clks' Ryder Lee
                   ` (2 preceding siblings ...)
  2018-11-13  9:52 ` [resend PATCH 1/3] pwm: mediatek: drop flag 'has_clks' Uwe Kleine-König
@ 2018-11-14 12:47 ` Thierry Reding
  2018-11-14 13:27   ` John Crispin
  2018-11-16  6:47   ` Uwe Kleine-König
  3 siblings, 2 replies; 12+ messages in thread
From: Thierry Reding @ 2018-11-14 12:47 UTC (permalink / raw)
  To: Ryder Lee
  Cc: Rob Herring, linux-pwm, Weijie Gao, Roy Luo, devicetree,
	linux-kernel, linux-arm-kernel, linux-mediatek, John Crispin

[-- Attachment #1: Type: text/plain, Size: 1312 bytes --]

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.

Both of these mechanisms aren't equivalent. While CCF can deal with
optional clocks, what the has_clks flag actually means is that the
device doesn't need a clock (or doesn't have a clock input) on the
devices where it is cleared.

So I'd actually be in favor of keeping the has_clks property because it
serves as an additional sanity check. For example if you run this driver
on an SoC that "has clocks" but if you don't list them in DT, then after
this patch the driver will happily continue without clocks, even though
it may break completely without those clocks. I've seen SoCs respond to
disabled clocks for a hardware block in different ways, in many cases an
access to any of the registers will completely hang the CPU. In other
cases it may just crash in some other way or give you some sort of
machine exception. None of those are good, and make the tiny bit of
additional code required to support the has_clks flag very attractive.

But that's just my opinion. If you prefer to throw away that safety
barrier, be my guest. But if you do, please move this functionality into
the clock framework first and then make the driver use it.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [resend PATCH 1/3] pwm: mediatek: drop flag 'has_clks'
  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
  1 sibling, 1 reply; 12+ messages in thread
From: John Crispin @ 2018-11-14 13:27 UTC (permalink / raw)
  To: Thierry Reding, Ryder Lee
  Cc: Rob Herring, linux-pwm, Weijie Gao, Roy Luo, devicetree,
	linux-kernel, linux-arm-kernel, linux-mediatek


On 14/11/2018 13:47, Thierry Reding wrote:
> 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.
> Both of these mechanisms aren't equivalent. While CCF can deal with
> optional clocks, what the has_clks flag actually means is that the
> device doesn't need a clock (or doesn't have a clock input) on the
> devices where it is cleared.
>
> So I'd actually be in favor of keeping the has_clks property because it
> serves as an additional sanity check. For example if you run this driver
> on an SoC that "has clocks" but if you don't list them in DT, then after
> this patch the driver will happily continue without clocks, even though
> it may break completely without those clocks. I've seen SoCs respond to
> disabled clocks for a hardware block in different ways, in many cases an
> access to any of the registers will completely hang the CPU. In other
> cases it may just crash in some other way or give you some sort of
> machine exception. None of those are good, and make the tiny bit of
> additional code required to support the has_clks flag very attractive.
>
> But that's just my opinion. If you prefer to throw away that safety
> barrier, be my guest. But if you do, please move this functionality into
> the clock framework first and then make the driver use it.
>
> Thierry

Hi,

sorry for my late response. I added the flag for the legacy MIPS 
silicon. These SoCs only have a single clock register with a few on/off 
bits. there is no complex clocktree or scaling. Hence COMMON_CLK is not 
supported by those SoCs. I fully agree with Thierry, that the flag makes 
this explicit and the intent was indeed to make sure that on silicon 
where clocks are required, that they really are listed in OF. This is 
indeed an extra sanity check and hiding it in an implicit check inside 
CCF does not feel right.

     John


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [resend PATCH 1/3] pwm: mediatek: drop flag 'has_clks'
  2018-11-14 13:27   ` John Crispin
@ 2018-11-15  2:16     ` Ryder Lee
  0 siblings, 0 replies; 12+ messages in thread
From: Ryder Lee @ 2018-11-15  2:16 UTC (permalink / raw)
  To: John Crispin
  Cc: Thierry Reding, Rob Herring, linux-pwm, Weijie Gao, Roy Luo,
	devicetree, linux-kernel, linux-arm-kernel, linux-mediatek

On Wed, 2018-11-14 at 14:27 +0100, John Crispin wrote:
> On 14/11/2018 13:47, Thierry Reding wrote:
> > 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.
> > Both of these mechanisms aren't equivalent. While CCF can deal with
> > optional clocks, what the has_clks flag actually means is that the
> > device doesn't need a clock (or doesn't have a clock input) on the
> > devices where it is cleared.
> >
> > So I'd actually be in favor of keeping the has_clks property because it
> > serves as an additional sanity check. For example if you run this driver
> > on an SoC that "has clocks" but if you don't list them in DT, then after
> > this patch the driver will happily continue without clocks, even though
> > it may break completely without those clocks. I've seen SoCs respond to
> > disabled clocks for a hardware block in different ways, in many cases an
> > access to any of the registers will completely hang the CPU. In other
> > cases it may just crash in some other way or give you some sort of
> > machine exception. None of those are good, and make the tiny bit of
> > additional code required to support the has_clks flag very attractive.
> >
> > But that's just my opinion. If you prefer to throw away that safety
> > barrier, be my guest. But if you do, please move this functionality into
> > the clock framework first and then make the driver use it.
> >
> > Thierry
> 
> Hi,
> 
> sorry for my late response. I added the flag for the legacy MIPS 
> silicon. These SoCs only have a single clock register with a few on/off 
> bits. there is no complex clocktree or scaling. Hence COMMON_CLK is not 
> supported by those SoCs. I fully agree with Thierry, that the flag makes 
> this explicit and the intent was indeed to make sure that on silicon 
> where clocks are required, that they really are listed in OF. This is 
> indeed an extra sanity check and hiding it in an implicit check inside 
> CCF does not feel right.
> 
>      John
> 

Thanks for the detailed information:)  I will drop this patch in v1.

Ryder


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [resend PATCH 2/3] pwm: mediatek: add pwm support for MT7629 SoC
  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
  0 siblings, 0 replies; 12+ messages in thread
From: Sean Wang @ 2018-11-15 19:20 UTC (permalink / raw)
  To: ryder.lee
  Cc: thierry.reding, linux-pwm, devicetree, weijie.gao, linux-kernel,
	cheng-hao.luo, robh+dt, linux-mediatek, linux-arm-kernel

On Mon, Nov 12, 2018 at 6:09 PM Ryder Lee <ryder.lee@mediatek.com> wrote:
>
> This adds pwm controller support for MT7629 SoC.
>
> Signed-off-by: Ryder Lee <ryder.lee@mediatek.com>
> ---
>  drivers/pwm/pwm-mediatek.c | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/drivers/pwm/pwm-mediatek.c b/drivers/pwm/pwm-mediatek.c
> index 9400c41..4ed95e5 100644
> --- a/drivers/pwm/pwm-mediatek.c
> +++ b/drivers/pwm/pwm-mediatek.c
> @@ -292,11 +292,16 @@ static int mtk_pwm_remove(struct platform_device *pdev)
>         .pwm45_fixup = true,
>  };
>
> +static const struct mtk_pwm_platform_data mt7629_pwm_data = {
> +       .num_pwms = 1,
> +};

How about adding a property for the num_pwms for the PWM controller?
It at least can help stop adding a new entry for every SoC with only
differences on PWM number.

> +
>  static const struct of_device_id mtk_pwm_of_match[] = {
>         { .compatible = "mediatek,mt2712-pwm", .data = &mt2712_pwm_data },
>         { .compatible = "mediatek,mt7622-pwm", .data = &mt7622_pwm_data },
>         { .compatible = "mediatek,mt7623-pwm", .data = &mt7623_pwm_data },
>         { .compatible = "mediatek,mt7628-pwm", .data = &mt7628_pwm_data },
> +       { .compatible = "mediatek,mt7629-pwm", .data = &mt7629_pwm_data },
>         { },
>  };
>  MODULE_DEVICE_TABLE(of, mtk_pwm_of_match);
> --
> 1.9.1
>
>
> _______________________________________________
> Linux-mediatek mailing list
> Linux-mediatek@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-mediatek

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [resend PATCH 1/3] pwm: mediatek: drop flag 'has_clks'
  2018-11-14 12:47 ` Thierry Reding
  2018-11-14 13:27   ` John Crispin
@ 2018-11-16  6:47   ` Uwe Kleine-König
  2018-11-16 10:22     ` Thierry Reding
  1 sibling, 1 reply; 12+ messages in thread
From: Uwe Kleine-König @ 2018-11-16  6:47 UTC (permalink / raw)
  To: Thierry Reding
  Cc: Ryder Lee, Rob Herring, linux-pwm, Weijie Gao, Roy Luo,
	devicetree, linux-kernel, linux-arm-kernel, linux-mediatek,
	John Crispin

On Wed, Nov 14, 2018 at 01:47:52PM +0100, Thierry Reding wrote:
> 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.
> 
> Both of these mechanisms aren't equivalent. While CCF can deal with
> optional clocks, what the has_clks flag actually means is that the
> device doesn't need a clock (or doesn't have a clock input) on the
> devices where it is cleared.
> 
> So I'd actually be in favor of keeping the has_clks property because it
> serves as an additional sanity check. For example if you run this driver
> on an SoC that "has clocks" but if you don't list them in DT, then after
> this patch the driver will happily continue without clocks, even though
> it may break completely without those clocks. I've seen SoCs respond to
> disabled clocks for a hardware block in different ways, in many cases an
> access to any of the registers will completely hang the CPU. In other
> cases it may just crash in some other way or give you some sort of
> machine exception. None of those are good, and make the tiny bit of
> additional code required to support the has_clks flag very attractive.
> 
> But that's just my opinion. If you prefer to throw away that safety
> barrier, be my guest. But if you do, please move this functionality into
> the clock framework first and then make the driver use it.

The usual policy is: If the things specified in the dt are
wrong or incomplete, it's ok to fail however you like. So from a
correctness POV I think the change is fine.

I don't know about the mips details that John pointed out in a followup
to this mail though.

Best regards
Uwe

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [resend PATCH 1/3] pwm: mediatek: drop flag 'has_clks'
  2018-11-16  6:47   ` Uwe Kleine-König
@ 2018-11-16 10:22     ` Thierry Reding
  0 siblings, 0 replies; 12+ messages in thread
From: Thierry Reding @ 2018-11-16 10:22 UTC (permalink / raw)
  To: Uwe Kleine-König
  Cc: Ryder Lee, Rob Herring, linux-pwm, Weijie Gao, Roy Luo,
	devicetree, linux-kernel, linux-arm-kernel, linux-mediatek,
	John Crispin

[-- Attachment #1: Type: text/plain, Size: 2089 bytes --]

On Fri, Nov 16, 2018 at 07:47:48AM +0100, Uwe Kleine-König wrote:
> On Wed, Nov 14, 2018 at 01:47:52PM +0100, Thierry Reding wrote:
> > 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.
> > 
> > Both of these mechanisms aren't equivalent. While CCF can deal with
> > optional clocks, what the has_clks flag actually means is that the
> > device doesn't need a clock (or doesn't have a clock input) on the
> > devices where it is cleared.
> > 
> > So I'd actually be in favor of keeping the has_clks property because it
> > serves as an additional sanity check. For example if you run this driver
> > on an SoC that "has clocks" but if you don't list them in DT, then after
> > this patch the driver will happily continue without clocks, even though
> > it may break completely without those clocks. I've seen SoCs respond to
> > disabled clocks for a hardware block in different ways, in many cases an
> > access to any of the registers will completely hang the CPU. In other
> > cases it may just crash in some other way or give you some sort of
> > machine exception. None of those are good, and make the tiny bit of
> > additional code required to support the has_clks flag very attractive.
> > 
> > But that's just my opinion. If you prefer to throw away that safety
> > barrier, be my guest. But if you do, please move this functionality into
> > the clock framework first and then make the driver use it.
> 
> The usual policy is: If the things specified in the dt are
> wrong or incomplete, it's ok to fail however you like. So from a
> correctness POV I think the change is fine.

Erm... that's pretty much what I said. It doesn't necessarily mean that
it's the right thing to do, though. If we know that clocks are required
and we don't find them in DT, it's better to complain and let the user
know exactly what is wrong rather than just let it crash and have them
track down the bug without additional information.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2018-11-16 10:22 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-11-13  2:08 [resend PATCH 1/3] pwm: mediatek: drop flag 'has_clks' 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 ` [resend PATCH 1/3] pwm: mediatek: drop flag 'has_clks' Uwe Kleine-König
2018-11-13 18:00   ` 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

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).