Re: [PATCH v2] clk: Add PWM clock driver

[Philipp Zabel <p.zabel@pengutronix.de>]

Hi Janusz,

Am Montag, den 08.12.2014, 21:03 +0100 schrieb Janusz Użycki:
> Hi,
> 
> I've fixed my pwm driver and I can enable 12MHz 50% output using sysfs. 
> Then I rebased the pwm-clock to 3.14.
> I have connected mcp2515 and it works with fixed clock. When I switch 
> the chip's clock to pwm clock in dt
> I get "mcp251x: probe of spi1.2 failed with error -2". I've also added 
> clock-frequency property
> but it didn't help.
> When I set the mcp2515 clock to fixed clock again but the clock is not 
> applied to mcp2515 I get
> "mcp251x spi1.2: MCP251x didn't enter in conf mode after reset".
> So it looks this is indeed probe error caused likely by dt.

Did pwm-clock fail to probe already? Could you check with the patch
below?

> The fixed and pwm clock in DT:
>          clocks {
>                  #address-cells = <1>;
>                  #size-cells = <1>;
>                  ranges;
>                  mcp251x_xtal_clk: mcp2515_xtal {
>                          compatible = "fixed-clock";
>                          #clock-cells = <0>;
>                          clock-frequency = <12000000>;
>                  };
> 
>                  mcp251x_pwm_clk: mcp2515_pwm {
>                          compatible = "pwm-clock";
>                          #clock-cells = <0>;
>                          clock-frequency = <12000000>;
>                          clock-output-names = "can_clk";
>                          pwms = <&pwm 3 83>;     /* 12MHz = 1 / ~83ns */
>                  };
>          };
> 
> Also the mentioned frequency recalculation problem appears here.
> In the case above recalc value is about 12.048MHz instead of 12.0MHz.
> While PWM block is clocked 24MHz and the pwm generates exactly 12MHz
> the pwm-clock driver returns drifted value. Using clock-frequency like
> fixed-clock does could simply solve the binding problem.

Yes, for this case it is very unfortunate that there's only nanosecond
resolution for the duty cycle. Adding a clock-frequency property to
indicate the real frequency would solve this problem.

------8<------
diff --git a/drivers/clk/clk-pwm.c b/drivers/clk/clk-pwm.c
index 8f747b3..9c13856 100644
--- a/drivers/clk/clk-pwm.c
+++ b/drivers/clk/clk-pwm.c
@@ -63,12 +63,16 @@ int clk_pwm_probe(struct platform_device *pdev)
 		return -ENOMEM;
 
 	pwm = devm_pwm_get(&pdev->dev, NULL);
-	if (IS_ERR(pwm))
+	if (IS_ERR(pwm)) {
+		dev_err(&pdev->dev, "failed to get pwm: %ld\n", PTR_ERR(pwm));
 		return PTR_ERR(pwm);
+	}
 
 	ret = pwm_config(pwm, (pwm->period + 1) >> 1, pwm->period);
-	if (ret < 0)
+	if (ret < 0) {
+		dev_err(&pdev->dev, "failed to configure pwm: %d\n", ret);
 		return ret;
+	}
 
 	init.name = "pwm-clock";
 	init.ops = &clk_pwm_ops;
@@ -78,11 +82,18 @@ int clk_pwm_probe(struct platform_device *pdev)
 	clk_pwm->pwm = pwm;
 	clk_pwm->hw.init = &init;
 	clk = devm_clk_register(&pdev->dev, &clk_pwm->hw);
-	if (IS_ERR(clk))
+	if (IS_ERR(clk)) {
+		dev_err(&pdev->dev, "failed to register clock: %ld\n",
+			PTR_ERR(clk));
 		return PTR_ERR(clk);
+	}
 
-	return of_clk_add_provider(pdev->dev.of_node,
-				   of_clk_src_simple_get, clk);
+	ret = of_clk_add_provider(pdev->dev.of_node,
+				  of_clk_src_simple_get, clk);
+	if (ret)
+		dev_err(&pdev->dev, "failed to add clock provider: %d\n", ret);
+
+	return ret;
 }
 
 int clk_pwm_remove(struct platform_device *pdev)