[PATCH v1 1/5] pwm: mediatek: add a property "mediatek,num-pwms"

[PATCH v1 1/5] pwm: mediatek: add a property "mediatek,num-pwms"

[Ryder Lee]

This adds a property "mediatek,num-pwms" to avoid having an endless
list of compatibles with no differences for the same driver.

Thus, the driver should have backwards compatibility to older DTs.

Signed-off-by: Ryder Lee <ryder.lee@mediatek.com>
---
Changes since v1: add some checks for backwards compatibility.
---
 drivers/pwm/pwm-mediatek.c | 27 ++++++++++++++++++---------
 1 file changed, 18 insertions(+), 9 deletions(-)

diff --git a/drivers/pwm/pwm-mediatek.c b/drivers/pwm/pwm-mediatek.c
index eb6674c..81b7e5e 100644
--- a/drivers/pwm/pwm-mediatek.c
+++ b/drivers/pwm/pwm-mediatek.c
@@ -55,7 +55,7 @@ enum {
 };
 
 struct mtk_pwm_platform_data {
-	unsigned int num_pwms;
+	unsigned int num_pwms;	/* it should not be used in the future SoCs */
 	bool pwm45_fixup;
 	bool has_clks;
 };
@@ -226,27 +226,36 @@ static void mtk_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
 
 static int mtk_pwm_probe(struct platform_device *pdev)
 {
-	const struct mtk_pwm_platform_data *data;
+	struct device_node *np = pdev->dev.of_node;
 	struct mtk_pwm_chip *pc;
 	struct resource *res;
-	unsigned int i;
+	unsigned int i, num_pwms;
 	int ret;
 
 	pc = devm_kzalloc(&pdev->dev, sizeof(*pc), GFP_KERNEL);
 	if (!pc)
 		return -ENOMEM;
 
-	data = of_device_get_match_data(&pdev->dev);
-	if (data == NULL)
-		return -EINVAL;
-	pc->soc = data;
+	pc->soc = of_device_get_match_data(&pdev->dev);
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	pc->regs = devm_ioremap_resource(&pdev->dev, res);
 	if (IS_ERR(pc->regs))
 		return PTR_ERR(pc->regs);
 
-	for (i = 0; i < data->num_pwms + 2 && pc->soc->has_clks; i++) {
+	/* Check if we have set 'num-pwms' in DTs. */
+	ret = of_property_read_u32(np, "mediatek,num-pwms", &num_pwms);
+	if (ret < 0) {
+		/* If no, fallback to use SoC data for backwards compatibility. */
+		if (pc->soc->num_pwms) {
+			num_pwms = pc->soc->num_pwms;
+		} else {
+			dev_err(&pdev->dev, "failed to get pwm number: %d\n", ret);
+			return ret;
+		}
+	}
+
+	for (i = 0; i < num_pwms + 2 && pc->soc->has_clks; 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",
@@ -260,7 +269,7 @@ static int mtk_pwm_probe(struct platform_device *pdev)
 	pc->chip.dev = &pdev->dev;
 	pc->chip.ops = &mtk_pwm_ops;
 	pc->chip.base = -1;
-	pc->chip.npwm = data->num_pwms;
+	pc->chip.npwm = num_pwms;
 
 	ret = pwmchip_add(&pc->chip);
 	if (ret < 0) {
-- 
1.9.1

[PATCH v1 2/5] dt-bindings: pwm: add a property "mediatek,num-pwms"

[Ryder Lee]

This adds a property "mediatek,num-pwms" in example so that we could
set the number of PWM channels via device tree.

Signed-off-by: Ryder Lee <ryder.lee@mediatek.com>
Reviewed-by: Matthias Brugger <matthias.bgg@gmail.com>
---
Changes since v1: add a Reviewed-by tag.
---
 Documentation/devicetree/bindings/pwm/pwm-mediatek.txt | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/pwm/pwm-mediatek.txt b/Documentation/devicetree/bindings/pwm/pwm-mediatek.txt
index 991728c..f9e2d1f 100644
--- a/Documentation/devicetree/bindings/pwm/pwm-mediatek.txt
+++ b/Documentation/devicetree/bindings/pwm/pwm-mediatek.txt
@@ -20,6 +20,7 @@ Required properties:
  - pinctrl-names: Must contain a "default" entry.
  - pinctrl-0: One property must exist for each entry in pinctrl-names.
    See pinctrl/pinctrl-bindings.txt for details of the property values.
+ - mediatek,num-pwms: the number of PWM channels.
 
 Example:
 	pwm0: pwm@11006000 {
@@ -37,4 +38,5 @@ Example:
 			      "pwm3", "pwm4", "pwm5";
 		pinctrl-names = "default";
 		pinctrl-0 = <&pwm0_pins>;
+		mediatek,num-pwms = <5>;
 	};
-- 
1.9.1

[PATCH v1 3/5] arm64: dts: mt7622: add a property "mediatek,num-pwms" for PWM

[Ryder Lee]

This add a property "mediatek,num-pwms" for PWM controller.

Signed-off-by: Ryder Lee <ryder.lee@mediatek.com>
---
 arch/arm64/boot/dts/mediatek/mt7622.dtsi | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm64/boot/dts/mediatek/mt7622.dtsi b/arch/arm64/boot/dts/mediatek/mt7622.dtsi
index 8fc4aa7..ab016cf 100644
--- a/arch/arm64/boot/dts/mediatek/mt7622.dtsi
+++ b/arch/arm64/boot/dts/mediatek/mt7622.dtsi
@@ -436,6 +436,7 @@
 			 <&pericfg CLK_PERI_PWM6_PD>;
 		clock-names = "top", "main", "pwm1", "pwm2", "pwm3", "pwm4",
 			      "pwm5", "pwm6";
+		mediatek,num-pwms = <6>;
 		status = "disabled";
 	};
 
-- 
1.9.1

[PATCH v1 4/5] arm: dts: mt7623: add a property "mediatek,num-pwms" for PWM

[Ryder Lee]

This add a property "mediatek,num-pwms" for PWM controller.

Signed-off-by: Ryder Lee <ryder.lee@mediatek.com>
---
 arch/arm/boot/dts/mt7623.dtsi | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/boot/dts/mt7623.dtsi b/arch/arm/boot/dts/mt7623.dtsi
index 0253691..9d59c28 100644
--- a/arch/arm/boot/dts/mt7623.dtsi
+++ b/arch/arm/boot/dts/mt7623.dtsi
@@ -443,6 +443,7 @@
 			 <&pericfg CLK_PERI_PWM5>;
 		clock-names = "top", "main", "pwm1", "pwm2",
 			      "pwm3", "pwm4", "pwm5";
+		mediatek,num-pwms = <5>;
 		status = "disabled";
 	};
 
-- 
1.9.1

[PATCH v1 5/5] dt-bindings: pwm: update bindings for MT7629 SoC

[Ryder Lee]

This updates bindings for MT7629 pwm controller.

Signed-off-by: Ryder Lee <ryder.lee@mediatek.com>
Reviewed-by: Matthias Brugger <matthias.bgg@gmail.com>
---
Changes since v1: add a Reviewed-by tag.
---
 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 f9e2d1f..f7e7784 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", "mediatek,mt7622-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

Re: [PATCH v1 1/5] pwm: mediatek: add a property "mediatek,num-pwms"

[Uwe Kleine-König]

Hello,

On Fri, Jan 18, 2019 at 11:24:41AM +0800, Ryder Lee wrote:
> This adds a property "mediatek,num-pwms" to avoid having an endless
> list of compatibles with no differences for the same driver.
> 
> Thus, the driver should have backwards compatibility to older DTs.

I still think Thierry should bless "num-pwms" without vendor prefix.

> Signed-off-by: Ryder Lee <ryder.lee@mediatek.com>
> ---
> Changes since v1: add some checks for backwards compatibility.
> ---
>  drivers/pwm/pwm-mediatek.c | 27 ++++++++++++++++++---------
>  1 file changed, 18 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-mediatek.c b/drivers/pwm/pwm-mediatek.c
> index eb6674c..81b7e5e 100644
> --- a/drivers/pwm/pwm-mediatek.c
> +++ b/drivers/pwm/pwm-mediatek.c
> @@ -55,7 +55,7 @@ enum {
>  };
>  
>  struct mtk_pwm_platform_data {

Unrelated to this patch: This name is bad. This struct is not used as
platform_data and so should better be named mtk_pwm_of_data. While at
criticizing existing stuff: I'd prefer pwm_mediatek as common prefix to
match the filename.

> -	unsigned int num_pwms;
> +	unsigned int num_pwms;	/* it should not be used in the future SoCs */

I'd drop this comment in favour of a runtime warning.

>  	bool pwm45_fixup;
>  	bool has_clks;
>  };
> @@ -226,27 +226,36 @@ static void mtk_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
>  
>  static int mtk_pwm_probe(struct platform_device *pdev)
>  {
> -	const struct mtk_pwm_platform_data *data;
> +	struct device_node *np = pdev->dev.of_node;
>  	struct mtk_pwm_chip *pc;
>  	struct resource *res;
> -	unsigned int i;
> +	unsigned int i, num_pwms;
>  	int ret;
>  
>  	pc = devm_kzalloc(&pdev->dev, sizeof(*pc), GFP_KERNEL);
>  	if (!pc)
>  		return -ENOMEM;
>  
> -	data = of_device_get_match_data(&pdev->dev);
> -	if (data == NULL)
> -		return -EINVAL;
> -	pc->soc = data;
> +	pc->soc = of_device_get_match_data(&pdev->dev);

This might return NULL which ...

>  
>  	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>  	pc->regs = devm_ioremap_resource(&pdev->dev, res);
>  	if (IS_ERR(pc->regs))
>  		return PTR_ERR(pc->regs);
>  
> -	for (i = 0; i < data->num_pwms + 2 && pc->soc->has_clks; i++) {
> +	/* Check if we have set 'num-pwms' in DTs. */
> +	ret = of_property_read_u32(np, "mediatek,num-pwms", &num_pwms);
> +	if (ret < 0) {
> +		/* If no, fallback to use SoC data for backwards compatibility. */
> +		if (pc->soc->num_pwms) {

... here then results in a NULL pointer dereference. I think you want

		if (pc->soc)

here.

> +			num_pwms = pc->soc->num_pwms;
> +		} else {
> +			dev_err(&pdev->dev, "failed to get pwm number: %d\n", ret);
> +			return ret;
> +		}
> +	}
> +
> +	for (i = 0; i < num_pwms + 2 && pc->soc->has_clks; 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",
> @@ -260,7 +269,7 @@ static int mtk_pwm_probe(struct platform_device *pdev)
>  	pc->chip.dev = &pdev->dev;
>  	pc->chip.ops = &mtk_pwm_ops;
>  	pc->chip.base = -1;
> -	pc->chip.npwm = data->num_pwms;
> +	pc->chip.npwm = num_pwms;
>  
>  	ret = pwmchip_add(&pc->chip);
>  	if (ret < 0) {

Best regards
Uwe

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

Re: [PATCH v1 3/5] arm64: dts: mt7622: add a property "mediatek, num-pwms" for PWM

[Uwe Kleine-König]

Hello,

there is a whitespace between the comma and "num-pwms" in the Subject
that should not be there. (But I hope this is resolved by dropping the
vendor prefix from the property.)

Best regards
Uwe

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

Re: [PATCH v1 1/5] pwm: mediatek: add a property "mediatek,num-pwms"

[Uwe Kleine-König]

Hello,

just realized another issue while looking up a driver detail ...

On Fri, Jan 18, 2019 at 11:24:41AM +0800, Ryder Lee wrote:
> This adds a property "mediatek,num-pwms" to avoid having an endless
> list of compatibles with no differences for the same driver.
> 
> Thus, the driver should have backwards compatibility to older DTs.
> 
> Signed-off-by: Ryder Lee <ryder.lee@mediatek.com>
> ---
> Changes since v1: add some checks for backwards compatibility.
> ---
>  drivers/pwm/pwm-mediatek.c | 27 ++++++++++++++++++---------
>  1 file changed, 18 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-mediatek.c b/drivers/pwm/pwm-mediatek.c
> index eb6674c..81b7e5e 100644
> --- a/drivers/pwm/pwm-mediatek.c
> +++ b/drivers/pwm/pwm-mediatek.c
> @@ -55,7 +55,7 @@ enum {
>  };
>  
>  struct mtk_pwm_platform_data {
> -	unsigned int num_pwms;
> +	unsigned int num_pwms;	/* it should not be used in the future SoCs */
>  	bool pwm45_fixup;
>  	bool has_clks;
>  };
> @@ -226,27 +226,36 @@ static void mtk_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
>  
>  static int mtk_pwm_probe(struct platform_device *pdev)
>  {
> -	const struct mtk_pwm_platform_data *data;
> +	struct device_node *np = pdev->dev.of_node;
>  	struct mtk_pwm_chip *pc;
>  	struct resource *res;
> -	unsigned int i;
> +	unsigned int i, num_pwms;
>  	int ret;
>  
>  	pc = devm_kzalloc(&pdev->dev, sizeof(*pc), GFP_KERNEL);
>  	if (!pc)
>  		return -ENOMEM;
>  
> -	data = of_device_get_match_data(&pdev->dev);
> -	if (data == NULL)
> -		return -EINVAL;
> -	pc->soc = data;
> +	pc->soc = of_device_get_match_data(&pdev->dev);
>  
>  	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>  	pc->regs = devm_ioremap_resource(&pdev->dev, res);
>  	if (IS_ERR(pc->regs))
>  		return PTR_ERR(pc->regs);
>  
> -	for (i = 0; i < data->num_pwms + 2 && pc->soc->has_clks; i++) {
> +	/* Check if we have set 'num-pwms' in DTs. */
> +	ret = of_property_read_u32(np, "mediatek,num-pwms", &num_pwms);
> +	if (ret < 0) {
> +		/* If no, fallback to use SoC data for backwards compatibility. */
> +		if (pc->soc->num_pwms) {
> +			num_pwms = pc->soc->num_pwms;
> +		} else {
> +			dev_err(&pdev->dev, "failed to get pwm number: %d\n", ret);
> +			return ret;
> +		}
> +	}
> +
> +	for (i = 0; i < num_pwms + 2 && pc->soc->has_clks; i++) {
>  		pc->clks[i] = devm_clk_get(&pdev->dev, mtk_pwm_clk_name[i]);

If a dt contains

	mediatek,num-pwms = <17>;

you're accessing pc->clks[18] which is an out-of-bounds access, so
better check the limit or allocate the clks array dynamically.

Best regards
Uwe

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

Re: [PATCH v1 1/5] pwm: mediatek: add a property "mediatek,num-pwms"

[Matthias Brugger]

On 18/01/2019 04:24, Ryder Lee wrote:
> This adds a property "mediatek,num-pwms" to avoid having an endless
> list of compatibles with no differences for the same driver.
> 
> Thus, the driver should have backwards compatibility to older DTs.
> 
> Signed-off-by: Ryder Lee <ryder.lee@mediatek.com>
> ---
> Changes since v1: add some checks for backwards compatibility.
> ---
>  drivers/pwm/pwm-mediatek.c | 27 ++++++++++++++++++---------
>  1 file changed, 18 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-mediatek.c b/drivers/pwm/pwm-mediatek.c
> index eb6674c..81b7e5e 100644
> --- a/drivers/pwm/pwm-mediatek.c
> +++ b/drivers/pwm/pwm-mediatek.c
> @@ -55,7 +55,7 @@ enum {
>  };
>  
>  struct mtk_pwm_platform_data {
> -	unsigned int num_pwms;
> +	unsigned int num_pwms;	/* it should not be used in the future SoCs */
>  	bool pwm45_fixup;
>  	bool has_clks;
>  };
> @@ -226,27 +226,36 @@ static void mtk_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
>  
>  static int mtk_pwm_probe(struct platform_device *pdev)
>  {
> -	const struct mtk_pwm_platform_data *data;
> +	struct device_node *np = pdev->dev.of_node;
>  	struct mtk_pwm_chip *pc;
>  	struct resource *res;
> -	unsigned int i;
> +	unsigned int i, num_pwms;
>  	int ret;
>  
>  	pc = devm_kzalloc(&pdev->dev, sizeof(*pc), GFP_KERNEL);
>  	if (!pc)
>  		return -ENOMEM;
>  
> -	data = of_device_get_match_data(&pdev->dev);
> -	if (data == NULL)
> -		return -EINVAL;
> -	pc->soc = data;
> +	pc->soc = of_device_get_match_data(&pdev->dev);
>  
>  	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>  	pc->regs = devm_ioremap_resource(&pdev->dev, res);
>  	if (IS_ERR(pc->regs))
>  		return PTR_ERR(pc->regs);
>  
> -	for (i = 0; i < data->num_pwms + 2 && pc->soc->has_clks; i++) {
> +	/* Check if we have set 'num-pwms' in DTs. */
> +	ret = of_property_read_u32(np, "mediatek,num-pwms", &num_pwms);
> +	if (ret < 0) {
> +		/* If no, fallback to use SoC data for backwards compatibility. */
> +		if (pc->soc->num_pwms) {
> +			num_pwms = pc->soc->num_pwms;

Maybe that's bike shedding, but I think it would be better to carve out the
num_pwms from the mtk_pwm_platform_data and check against the compatible here.
With a expressive comment it will help other driver developers to not start
adding num_pwms in the platform data in their first attempt.

Regards,
Matthias

> +		} else {
> +			dev_err(&pdev->dev, "failed to get pwm number: %d\n", ret);
> +			return ret;
> +		}
> +	}
> +
> +	for (i = 0; i < num_pwms + 2 && pc->soc->has_clks; 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",
> @@ -260,7 +269,7 @@ static int mtk_pwm_probe(struct platform_device *pdev)
>  	pc->chip.dev = &pdev->dev;
>  	pc->chip.ops = &mtk_pwm_ops;
>  	pc->chip.base = -1;
> -	pc->chip.npwm = data->num_pwms;
> +	pc->chip.npwm = num_pwms;
>  
>  	ret = pwmchip_add(&pc->chip);
>  	if (ret < 0) {
> 

Re: [PATCH v1 2/5] dt-bindings: pwm: add a property "mediatek,num-pwms"

[Matthias Brugger]

On 18/01/2019 04:24, Ryder Lee wrote:
> This adds a property "mediatek,num-pwms" in example so that we could
> set the number of PWM channels via device tree.
> 
> Signed-off-by: Ryder Lee <ryder.lee@mediatek.com>
> Reviewed-by: Matthias Brugger <matthias.bgg@gmail.com>
> ---
> Changes since v1: add a Reviewed-by tag.
> ---
>  Documentation/devicetree/bindings/pwm/pwm-mediatek.txt | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/pwm/pwm-mediatek.txt b/Documentation/devicetree/bindings/pwm/pwm-mediatek.txt
> index 991728c..f9e2d1f 100644
> --- a/Documentation/devicetree/bindings/pwm/pwm-mediatek.txt
> +++ b/Documentation/devicetree/bindings/pwm/pwm-mediatek.txt
> @@ -20,6 +20,7 @@ Required properties:
>   - pinctrl-names: Must contain a "default" entry.
>   - pinctrl-0: One property must exist for each entry in pinctrl-names.
>     See pinctrl/pinctrl-bindings.txt for details of the property values.
> + - mediatek,num-pwms: the number of PWM channels.
>  
>  Example:
>  	pwm0: pwm@11006000 {
> @@ -37,4 +38,5 @@ Example:
>  			      "pwm3", "pwm4", "pwm5";
>  		pinctrl-names = "default";
>  		pinctrl-0 = <&pwm0_pins>;
> +		mediatek,num-pwms = <5>;
>  	};
> 

Wasn't there a comment in the last version to use num-pwms instead of
mediatek,num-pwms?

Uwe I think you requested that.

Regards,
Matthias

Re: [PATCH v1 1/5] pwm: mediatek: add a property "mediatek,num-pwms"

[Ryder Lee]

On Fri, 2019-01-18 at 08:59 +0100, Uwe Kleine-König wrote:
> Hello,
> 
> On Fri, Jan 18, 2019 at 11:24:41AM +0800, Ryder Lee wrote:
> > This adds a property "mediatek,num-pwms" to avoid having an endless
> > list of compatibles with no differences for the same driver.
> > 
> > Thus, the driver should have backwards compatibility to older DTs.
> 
> I still think Thierry should bless "num-pwms" without vendor prefix.

Okay.

> > Signed-off-by: Ryder Lee <ryder.lee@mediatek.com>
> > ---
> > Changes since v1: add some checks for backwards compatibility.
> > ---
> >  drivers/pwm/pwm-mediatek.c | 27 ++++++++++++++++++---------
> >  1 file changed, 18 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/pwm/pwm-mediatek.c b/drivers/pwm/pwm-mediatek.c
> > index eb6674c..81b7e5e 100644
> > --- a/drivers/pwm/pwm-mediatek.c
> > +++ b/drivers/pwm/pwm-mediatek.c
> > @@ -55,7 +55,7 @@ enum {
> >  };
> >  
> >  struct mtk_pwm_platform_data {
> 
> Unrelated to this patch: This name is bad. This struct is not used as
> platform_data and so should better be named mtk_pwm_of_data. While at
> criticizing existing stuff: I'd prefer pwm_mediatek as common prefix to
> match the filename.

I think we can take care about that in another patch.

> > -	unsigned int num_pwms;
> > +	unsigned int num_pwms;	/* it should not be used in the future SoCs */
> 
> I'd drop this comment in favour of a runtime warning.

Sorry, I can't get you here.

> >  	bool pwm45_fixup;
> >  	bool has_clks;
> >  };
> > @@ -226,27 +226,36 @@ static void mtk_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
> >  
> >  static int mtk_pwm_probe(struct platform_device *pdev)
> >  {
> > -	const struct mtk_pwm_platform_data *data;
> > +	struct device_node *np = pdev->dev.of_node;
> >  	struct mtk_pwm_chip *pc;
> >  	struct resource *res;
> > -	unsigned int i;
> > +	unsigned int i, num_pwms;
> >  	int ret;
> >  
> >  	pc = devm_kzalloc(&pdev->dev, sizeof(*pc), GFP_KERNEL);
> >  	if (!pc)
> >  		return -ENOMEM;
> >  
> > -	data = of_device_get_match_data(&pdev->dev);
> > -	if (data == NULL)
> > -		return -EINVAL;
> > -	pc->soc = data;
> > +	pc->soc = of_device_get_match_data(&pdev->dev);
> 
> This might return NULL which ...

The only way to call probe() is to match an entry in
mtk_pwm_of_match[], so match cannot be NULL.

> >  
> >  	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> >  	pc->regs = devm_ioremap_resource(&pdev->dev, res);
> >  	if (IS_ERR(pc->regs))
> >  		return PTR_ERR(pc->regs);
> >  
> > -	for (i = 0; i < data->num_pwms + 2 && pc->soc->has_clks; i++) {
> > +	/* Check if we have set 'num-pwms' in DTs. */
> > +	ret = of_property_read_u32(np, "mediatek,num-pwms", &num_pwms);
> > +	if (ret < 0) {
> > +		/* If no, fallback to use SoC data for backwards compatibility. */
> > +		if (pc->soc->num_pwms) {
> 
> ... here then results in a NULL pointer dereference. I think you want

So we have no chance to get a NULL pointer, right?

> 		if (pc->soc)
> 
> here.
> 
> > +			num_pwms = pc->soc->num_pwms;
> > +		} else {
> > +			dev_err(&pdev->dev, "failed to get pwm number: %d\n", ret);
> > +			return ret;
> > +		}
> > +	}
> > +
> > +	for (i = 0; i < num_pwms + 2 && pc->soc->has_clks; 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",
> > @@ -260,7 +269,7 @@ static int mtk_pwm_probe(struct platform_device *pdev)
> >  	pc->chip.dev = &pdev->dev;
> >  	pc->chip.ops = &mtk_pwm_ops;
> >  	pc->chip.base = -1;
> > -	pc->chip.npwm = data->num_pwms;
> > +	pc->chip.npwm = num_pwms;
> >  
> >  	ret = pwmchip_add(&pc->chip);
> >  	if (ret < 0) {
> 
> Best regards
> Uwe
> 

Re: [PATCH v1 1/5] pwm: mediatek: add a property "mediatek,num-pwms"

[Ryder Lee]

On Fri, 2019-01-18 at 09:05 +0100, Uwe Kleine-König wrote:
> Hello,
> 
> just realized another issue while looking up a driver detail ...
> 
> On Fri, Jan 18, 2019 at 11:24:41AM +0800, Ryder Lee wrote:
> > This adds a property "mediatek,num-pwms" to avoid having an endless
> > list of compatibles with no differences for the same driver.
> > 
> > Thus, the driver should have backwards compatibility to older DTs.
> > 
> > Signed-off-by: Ryder Lee <ryder.lee@mediatek.com>
> > ---
> > Changes since v1: add some checks for backwards compatibility.
> > ---
> >  drivers/pwm/pwm-mediatek.c | 27 ++++++++++++++++++---------
> >  1 file changed, 18 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/pwm/pwm-mediatek.c b/drivers/pwm/pwm-mediatek.c
> > index eb6674c..81b7e5e 100644
> > --- a/drivers/pwm/pwm-mediatek.c
> > +++ b/drivers/pwm/pwm-mediatek.c
> > @@ -55,7 +55,7 @@ enum {
> >  };
> >  
> >  struct mtk_pwm_platform_data {
> > -	unsigned int num_pwms;
> > +	unsigned int num_pwms;	/* it should not be used in the future SoCs */
> >  	bool pwm45_fixup;
> >  	bool has_clks;
> >  };
> > @@ -226,27 +226,36 @@ static void mtk_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
> >  
> >  static int mtk_pwm_probe(struct platform_device *pdev)
> >  {
> > -	const struct mtk_pwm_platform_data *data;
> > +	struct device_node *np = pdev->dev.of_node;
> >  	struct mtk_pwm_chip *pc;
> >  	struct resource *res;
> > -	unsigned int i;
> > +	unsigned int i, num_pwms;
> >  	int ret;
> >  
> >  	pc = devm_kzalloc(&pdev->dev, sizeof(*pc), GFP_KERNEL);
> >  	if (!pc)
> >  		return -ENOMEM;
> >  
> > -	data = of_device_get_match_data(&pdev->dev);
> > -	if (data == NULL)
> > -		return -EINVAL;
> > -	pc->soc = data;
> > +	pc->soc = of_device_get_match_data(&pdev->dev);
> >  
> >  	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> >  	pc->regs = devm_ioremap_resource(&pdev->dev, res);
> >  	if (IS_ERR(pc->regs))
> >  		return PTR_ERR(pc->regs);
> >  
> > -	for (i = 0; i < data->num_pwms + 2 && pc->soc->has_clks; i++) {
> > +	/* Check if we have set 'num-pwms' in DTs. */
> > +	ret = of_property_read_u32(np, "mediatek,num-pwms", &num_pwms);
> > +	if (ret < 0) {
> > +		/* If no, fallback to use SoC data for backwards compatibility. */
> > +		if (pc->soc->num_pwms) {
> > +			num_pwms = pc->soc->num_pwms;
> > +		} else {
> > +			dev_err(&pdev->dev, "failed to get pwm number: %d\n", ret);
> > +			return ret;
> > +		}
> > +	}
> > +
> > +	for (i = 0; i < num_pwms + 2 && pc->soc->has_clks; i++) {
> >  		pc->clks[i] = devm_clk_get(&pdev->dev, mtk_pwm_clk_name[i]);
> 
> If a dt contains
> 
> 	mediatek,num-pwms = <17>;
> 
> you're accessing pc->clks[18] which is an out-of-bounds access, so
> better check the limit or allocate the clks array dynamically.
> 

Thanks for the reminder. I will fix it in v2.

Ryder

Re: [PATCH v1 1/5] pwm: mediatek: add a property "mediatek,num-pwms"

[Uwe Kleine-König]

Hello Ryder,

On Fri, Jan 18, 2019 at 05:42:54PM +0800, Ryder Lee wrote:
> On Fri, 2019-01-18 at 08:59 +0100, Uwe Kleine-König wrote:
> > Hello,
> > 
> > On Fri, Jan 18, 2019 at 11:24:41AM +0800, Ryder Lee wrote:
> > > This adds a property "mediatek,num-pwms" to avoid having an endless
> > > list of compatibles with no differences for the same driver.
> > > 
> > > Thus, the driver should have backwards compatibility to older DTs.
> > 
> > I still think Thierry should bless "num-pwms" without vendor prefix.
> 
> Okay.
> 
> > > Signed-off-by: Ryder Lee <ryder.lee@mediatek.com>
> > > ---
> > > Changes since v1: add some checks for backwards compatibility.
> > > ---
> > >  drivers/pwm/pwm-mediatek.c | 27 ++++++++++++++++++---------
> > >  1 file changed, 18 insertions(+), 9 deletions(-)
> > > 
> > > diff --git a/drivers/pwm/pwm-mediatek.c b/drivers/pwm/pwm-mediatek.c
> > > index eb6674c..81b7e5e 100644
> > > --- a/drivers/pwm/pwm-mediatek.c
> > > +++ b/drivers/pwm/pwm-mediatek.c
> > > @@ -55,7 +55,7 @@ enum {
> > >  };
> > >  
> > >  struct mtk_pwm_platform_data {
> > 
> > Unrelated to this patch: This name is bad. This struct is not used as
> > platform_data and so should better be named mtk_pwm_of_data. While at
> > criticizing existing stuff: I'd prefer pwm_mediatek as common prefix to
> > match the filename.
> 
> I think we can take care about that in another patch.

That's what I wanted to say, right. Do you follow up?

> > > -	unsigned int num_pwms;
> > > +	unsigned int num_pwms;	/* it should not be used in the future SoCs */
> > 
> > I'd drop this comment in favour of a runtime warning.
> 
> Sorry, I can't get you here.

I'd do a

	dev_warn(dev, "dt didn't specify number of PWMs, falling back to %d\n", pc->soc->num_pwms);

to make people aware that updating the dt would be nice.

> 
> > >  	bool pwm45_fixup;
> > >  	bool has_clks;
> > >  };
> > > @@ -226,27 +226,36 @@ static void mtk_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
> > >  
> > >  static int mtk_pwm_probe(struct platform_device *pdev)
> > >  {
> > > -	const struct mtk_pwm_platform_data *data;
> > > +	struct device_node *np = pdev->dev.of_node;
> > >  	struct mtk_pwm_chip *pc;
> > >  	struct resource *res;
> > > -	unsigned int i;
> > > +	unsigned int i, num_pwms;
> > >  	int ret;
> > >  
> > >  	pc = devm_kzalloc(&pdev->dev, sizeof(*pc), GFP_KERNEL);
> > >  	if (!pc)
> > >  		return -ENOMEM;
> > >  
> > > -	data = of_device_get_match_data(&pdev->dev);
> > > -	if (data == NULL)
> > > -		return -EINVAL;
> > > -	pc->soc = data;
> > > +	pc->soc = of_device_get_match_data(&pdev->dev);
> > 
> > This might return NULL which ...
> 
> The only way to call probe() is to match an entry in
> mtk_pwm_of_match[], so match cannot be NULL.

(<pedantic>Theoretically the driver can be probed by device name, but
that is not what I meant here.</pedantic>).

You're right, as long as all entries in mtk_pwm_of_match have a non-NULL
.data entry, you're fine. I somehow thought there might be some without
one. I wouldn't oppose to check for that anyhow as a defensive measure.

> > > [...]
> > > +	/* Check if we have set 'num-pwms' in DTs. */
> > > +	ret = of_property_read_u32(np, "mediatek,num-pwms", &num_pwms);
> > > +	if (ret < 0) {
> > > +		/* If no, fallback to use SoC data for backwards compatibility. */
> > > +		if (pc->soc->num_pwms) {
> > 
> > ... here then results in a NULL pointer dereference. I think you want
> 
> So we have no chance to get a NULL pointer, right?

Ack.

Best regards
Uwe

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

Re: [PATCH v1 1/5] pwm: mediatek: add a property "mediatek,num-pwms"

[Ryder Lee]

On Fri, 2019-01-18 at 10:53 +0100, Uwe Kleine-König wrote:
> Hello Ryder,
> 
> On Fri, Jan 18, 2019 at 05:42:54PM +0800, Ryder Lee wrote:
> > On Fri, 2019-01-18 at 08:59 +0100, Uwe Kleine-König wrote:
> > > Hello,
> > > 
> > > On Fri, Jan 18, 2019 at 11:24:41AM +0800, Ryder Lee wrote:
> > > > This adds a property "mediatek,num-pwms" to avoid having an endless
> > > > list of compatibles with no differences for the same driver.
> > > > 
> > > > Thus, the driver should have backwards compatibility to older DTs.
> > > 
> > > I still think Thierry should bless "num-pwms" without vendor prefix.
> > 
> > Okay.
> > 
> > > > Signed-off-by: Ryder Lee <ryder.lee@mediatek.com>
> > > > ---
> > > > Changes since v1: add some checks for backwards compatibility.
> > > > ---
> > > >  drivers/pwm/pwm-mediatek.c | 27 ++++++++++++++++++---------
> > > >  1 file changed, 18 insertions(+), 9 deletions(-)
> > > > 
> > > > diff --git a/drivers/pwm/pwm-mediatek.c b/drivers/pwm/pwm-mediatek.c
> > > > index eb6674c..81b7e5e 100644
> > > > --- a/drivers/pwm/pwm-mediatek.c
> > > > +++ b/drivers/pwm/pwm-mediatek.c
> > > > @@ -55,7 +55,7 @@ enum {
> > > >  };
> > > >  
> > > >  struct mtk_pwm_platform_data {
> > > 
> > > Unrelated to this patch: This name is bad. This struct is not used as
> > > platform_data and so should better be named mtk_pwm_of_data. While at
> > > criticizing existing stuff: I'd prefer pwm_mediatek as common prefix to
> > > match the filename.
> > 
> > I think we can take care about that in another patch.
> 
> That's what I wanted to say, right. Do you follow up?

Yes, I will do that.

> > > > -	unsigned int num_pwms;
> > > > +	unsigned int num_pwms;	/* it should not be used in the future SoCs */
> > > 
> > > I'd drop this comment in favour of a runtime warning.
> > 
> > Sorry, I can't get you here.
> 
> I'd do a
> 
> 	dev_warn(dev, "dt didn't specify number of PWMs, falling back to %d\n", pc->soc->num_pwms);
> 
> to make people aware that updating the dt would be nice.

Okay!

Thanks
Ryder

Re: [PATCH v1 1/5] pwm: mediatek: add a property "mediatek,num-pwms"

[Ryder Lee]

On Fri, 2019-01-18 at 09:43 +0100, Matthias Brugger wrote:
> 
> On 18/01/2019 04:24, Ryder Lee wrote:
> > This adds a property "mediatek,num-pwms" to avoid having an endless
> > list of compatibles with no differences for the same driver.
> > 
> > Thus, the driver should have backwards compatibility to older DTs.
> > 
> > Signed-off-by: Ryder Lee <ryder.lee@mediatek.com>
> > ---
> > Changes since v1: add some checks for backwards compatibility.
> > ---
> >  drivers/pwm/pwm-mediatek.c | 27 ++++++++++++++++++---------
> >  1 file changed, 18 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/pwm/pwm-mediatek.c b/drivers/pwm/pwm-mediatek.c
> > index eb6674c..81b7e5e 100644
> > --- a/drivers/pwm/pwm-mediatek.c
> > +++ b/drivers/pwm/pwm-mediatek.c
> > @@ -55,7 +55,7 @@ enum {
> >  };
> >  
> >  struct mtk_pwm_platform_data {
> > -	unsigned int num_pwms;
> > +	unsigned int num_pwms;	/* it should not be used in the future SoCs */
> >  	bool pwm45_fixup;
> >  	bool has_clks;
> >  };
> > @@ -226,27 +226,36 @@ static void mtk_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
> >  
> >  static int mtk_pwm_probe(struct platform_device *pdev)
> >  {
> > -	const struct mtk_pwm_platform_data *data;
> > +	struct device_node *np = pdev->dev.of_node;
> >  	struct mtk_pwm_chip *pc;
> >  	struct resource *res;
> > -	unsigned int i;
> > +	unsigned int i, num_pwms;
> >  	int ret;
> >  
> >  	pc = devm_kzalloc(&pdev->dev, sizeof(*pc), GFP_KERNEL);
> >  	if (!pc)
> >  		return -ENOMEM;
> >  
> > -	data = of_device_get_match_data(&pdev->dev);
> > -	if (data == NULL)
> > -		return -EINVAL;
> > -	pc->soc = data;
> > +	pc->soc = of_device_get_match_data(&pdev->dev);
> >  
> >  	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> >  	pc->regs = devm_ioremap_resource(&pdev->dev, res);
> >  	if (IS_ERR(pc->regs))
> >  		return PTR_ERR(pc->regs);
> >  
> > -	for (i = 0; i < data->num_pwms + 2 && pc->soc->has_clks; i++) {
> > +	/* Check if we have set 'num-pwms' in DTs. */
> > +	ret = of_property_read_u32(np, "mediatek,num-pwms", &num_pwms);
> > +	if (ret < 0) {
> > +		/* If no, fallback to use SoC data for backwards compatibility. */
> > +		if (pc->soc->num_pwms) {
> > +			num_pwms = pc->soc->num_pwms;
> 
> Maybe that's bike shedding, but I think it would be better to carve out the
> num_pwms from the mtk_pwm_platform_data and check against the compatible here.

I'm not sure how to properly curve it out? I think we still need this
variable to save the specific value for some legacy SoCs (with older
DTs).

> With a expressive comment it will help other driver developers to not start
> adding num_pwms in the platform data in their first attempt.

Definitely.

Ryder
> 
> > +		} else {
> > +			dev_err(&pdev->dev, "failed to get pwm number: %d\n", ret);
> > +			return ret;
> > +		}
> > +	}
> > +
> > +	for (i = 0; i < num_pwms + 2 && pc->soc->has_clks; 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",
> > @@ -260,7 +269,7 @@ static int mtk_pwm_probe(struct platform_device *pdev)
> >  	pc->chip.dev = &pdev->dev;
> >  	pc->chip.ops = &mtk_pwm_ops;
> >  	pc->chip.base = -1;
> > -	pc->chip.npwm = data->num_pwms;
> > +	pc->chip.npwm = num_pwms;
> >  
> >  	ret = pwmchip_add(&pc->chip);
> >  	if (ret < 0) {
> > 

Re: [PATCH v1 2/5] dt-bindings: pwm: add a property "mediatek,num-pwms"

[Uwe Kleine-König]

On Fri, Jan 18, 2019 at 09:44:49AM +0100, Matthias Brugger wrote:
> 
> 
> On 18/01/2019 04:24, Ryder Lee wrote:
> > This adds a property "mediatek,num-pwms" in example so that we could
> > set the number of PWM channels via device tree.
> > 
> > Signed-off-by: Ryder Lee <ryder.lee@mediatek.com>
> > Reviewed-by: Matthias Brugger <matthias.bgg@gmail.com>
> > ---
> > Changes since v1: add a Reviewed-by tag.
> > ---
> >  Documentation/devicetree/bindings/pwm/pwm-mediatek.txt | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/pwm/pwm-mediatek.txt b/Documentation/devicetree/bindings/pwm/pwm-mediatek.txt
> > index 991728c..f9e2d1f 100644
> > --- a/Documentation/devicetree/bindings/pwm/pwm-mediatek.txt
> > +++ b/Documentation/devicetree/bindings/pwm/pwm-mediatek.txt
> > @@ -20,6 +20,7 @@ Required properties:
> >   - pinctrl-names: Must contain a "default" entry.
> >   - pinctrl-0: One property must exist for each entry in pinctrl-names.
> >     See pinctrl/pinctrl-bindings.txt for details of the property values.
> > + - mediatek,num-pwms: the number of PWM channels.
> >  
> >  Example:
> >  	pwm0: pwm@11006000 {
> > @@ -37,4 +38,5 @@ Example:
> >  			      "pwm3", "pwm4", "pwm5";
> >  		pinctrl-names = "default";
> >  		pinctrl-0 = <&pwm0_pins>;
> > +		mediatek,num-pwms = <5>;
> >  	};
> > 
> 
> Wasn't there a comment in the last version to use num-pwms instead of
> mediatek,num-pwms?
> 
> Uwe I think you requested that.

I think it is sensible, but it needs Thierry's blessing. Unfortunately
he seems busy with other stuff and getting feedback from him needs
patience.

Best regards
Uwe

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

Re: [PATCH v1 1/5] pwm: mediatek: add a property "mediatek,num-pwms"

[Uwe Kleine-König]

On Sat, Jan 19, 2019 at 10:54:47AM +0800, Ryder Lee wrote:
> On Fri, 2019-01-18 at 09:43 +0100, Matthias Brugger wrote:
> > 
> > On 18/01/2019 04:24, Ryder Lee wrote:
> > > This adds a property "mediatek,num-pwms" to avoid having an endless
> > > list of compatibles with no differences for the same driver.
> > > 
> > > Thus, the driver should have backwards compatibility to older DTs.
> > > 
> > > Signed-off-by: Ryder Lee <ryder.lee@mediatek.com>
> > > ---
> > > Changes since v1: add some checks for backwards compatibility.
> > > ---
> > >  drivers/pwm/pwm-mediatek.c | 27 ++++++++++++++++++---------
> > >  1 file changed, 18 insertions(+), 9 deletions(-)
> > > 
> > > diff --git a/drivers/pwm/pwm-mediatek.c b/drivers/pwm/pwm-mediatek.c
> > > index eb6674c..81b7e5e 100644
> > > --- a/drivers/pwm/pwm-mediatek.c
> > > +++ b/drivers/pwm/pwm-mediatek.c
> > > @@ -55,7 +55,7 @@ enum {
> > >  };
> > >  
> > >  struct mtk_pwm_platform_data {
> > > -	unsigned int num_pwms;
> > > +	unsigned int num_pwms;	/* it should not be used in the future SoCs */
> > >  	bool pwm45_fixup;
> > >  	bool has_clks;
> > >  };
> > > @@ -226,27 +226,36 @@ static void mtk_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
> > >  
> > >  static int mtk_pwm_probe(struct platform_device *pdev)
> > >  {
> > > -	const struct mtk_pwm_platform_data *data;
> > > +	struct device_node *np = pdev->dev.of_node;
> > >  	struct mtk_pwm_chip *pc;
> > >  	struct resource *res;
> > > -	unsigned int i;
> > > +	unsigned int i, num_pwms;
> > >  	int ret;
> > >  
> > >  	pc = devm_kzalloc(&pdev->dev, sizeof(*pc), GFP_KERNEL);
> > >  	if (!pc)
> > >  		return -ENOMEM;
> > >  
> > > -	data = of_device_get_match_data(&pdev->dev);
> > > -	if (data == NULL)
> > > -		return -EINVAL;
> > > -	pc->soc = data;
> > > +	pc->soc = of_device_get_match_data(&pdev->dev);
> > >  
> > >  	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > >  	pc->regs = devm_ioremap_resource(&pdev->dev, res);
> > >  	if (IS_ERR(pc->regs))
> > >  		return PTR_ERR(pc->regs);
> > >  
> > > -	for (i = 0; i < data->num_pwms + 2 && pc->soc->has_clks; i++) {
> > > +	/* Check if we have set 'num-pwms' in DTs. */
> > > +	ret = of_property_read_u32(np, "mediatek,num-pwms", &num_pwms);
> > > +	if (ret < 0) {
> > > +		/* If no, fallback to use SoC data for backwards compatibility. */
> > > +		if (pc->soc->num_pwms) {
> > > +			num_pwms = pc->soc->num_pwms;
> > 
> > Maybe that's bike shedding, but I think it would be better to carve out the
> > num_pwms from the mtk_pwm_platform_data and check against the compatible here.
> 
> I'm not sure how to properly curve it out? I think we still need this
> variable to save the specific value for some legacy SoCs (with older
> DTs).

I guess he means  something like:

	if (is_compatible_to_variant_A(dev))
		num_pwms = 12;
	else if (is_compatible_to_variant_B(dev))
		num_pwms = 2;

. In my eyes the bike shed should be light red and I prefer to collect
the fallback num_pwms in the compatible_data as is to keep the code
simpler. Maybe rename the member from num_pwms to fallback_num_pwms to
make it more obvious that it doesn't represent the actually used value.

> > With a expressive comment it will help other driver developers to not start
> > adding num_pwms in the platform data in their first attempt.
> 
> Definitely.

My suggestion was to add a dev_warn, which IMHO is still better than a
comment.

Best regards
Uwe

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

Re: [PATCH v1 1/5] pwm: mediatek: add a property "mediatek,num-pwms"

[Ryder Lee]

+John

HI John,

On Mon, 2019-01-21 at 16:49 +0800, Uwe Kleine-König wrote:
> On Sat, Jan 19, 2019 at 10:54:47AM +0800, Ryder Lee wrote:
> > On Fri, 2019-01-18 at 09:43 +0100, Matthias Brugger wrote:
> > >
> > > On 18/01/2019 04:24, Ryder Lee wrote:
> > > > This adds a property "mediatek,num-pwms" to avoid having an endless
> > > > list of compatibles with no differences for the same driver.
> > > >
> > > > Thus, the driver should have backwards compatibility to older DTs.
> > > >
> > > > Signed-off-by: Ryder Lee <ryder.lee@mediatek.com>
> > > > ---
> > > > Changes since v1: add some checks for backwards compatibility.
> > > > ---
> > > >  drivers/pwm/pwm-mediatek.c | 27 ++++++++++++++++++---------
> > > >  1 file changed, 18 insertions(+), 9 deletions(-)
> > > >
> > > > diff --git a/drivers/pwm/pwm-mediatek.c b/drivers/pwm/pwm-mediatek.c
> > > > index eb6674c..81b7e5e 100644
> > > > --- a/drivers/pwm/pwm-mediatek.c
> > > > +++ b/drivers/pwm/pwm-mediatek.c
> > > > @@ -55,7 +55,7 @@ enum {
> > > >  };
> > > >
> > > >  struct mtk_pwm_platform_data {
> > > > - unsigned int num_pwms;
> > > > + unsigned int num_pwms;  /* it should not be used in the future SoCs */
> > > >   bool pwm45_fixup;
> > > >   bool has_clks;
> > > >  };
> > > > @@ -226,27 +226,36 @@ static void mtk_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
> > > >
> > > >  static int mtk_pwm_probe(struct platform_device *pdev)
> > > >  {
> > > > - const struct mtk_pwm_platform_data *data;
> > > > + struct device_node *np = pdev->dev.of_node;
> > > >   struct mtk_pwm_chip *pc;
> > > >   struct resource *res;
> > > > - unsigned int i;
> > > > + unsigned int i, num_pwms;
> > > >   int ret;
> > > >
> > > >   pc = devm_kzalloc(&pdev->dev, sizeof(*pc), GFP_KERNEL);
> > > >   if (!pc)
> > > >           return -ENOMEM;
> > > >
> > > > - data = of_device_get_match_data(&pdev->dev);
> > > > - if (data == NULL)
> > > > -         return -EINVAL;
> > > > - pc->soc = data;
> > > > + pc->soc = of_device_get_match_data(&pdev->dev);
> > > >
> > > >   res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > > >   pc->regs = devm_ioremap_resource(&pdev->dev, res);
> > > >   if (IS_ERR(pc->regs))
> > > >           return PTR_ERR(pc->regs);
> > > >
> > > > - for (i = 0; i < data->num_pwms + 2 && pc->soc->has_clks; i++) {
> > > > + /* Check if we have set 'num-pwms' in DTs. */
> > > > + ret = of_property_read_u32(np, "mediatek,num-pwms", &num_pwms);
> > > > + if (ret < 0) {
> > > > +         /* If no, fallback to use SoC data for backwards compatibility. */
> > > > +         if (pc->soc->num_pwms) {
> > > > +                 num_pwms = pc->soc->num_pwms;
> > >
> > > Maybe that's bike shedding, but I think it would be better to carve out the
> > > num_pwms from the mtk_pwm_platform_data and check against the compatible here.
> >
> > I'm not sure how to properly curve it out? I think we still need this
> > variable to save the specific value for some legacy SoCs (with older
> > DTs).
> 
> I guess he means  something like:
> 
>         if (is_compatible_to_variant_A(dev))
>                 num_pwms = 12;
>         else if (is_compatible_to_variant_B(dev))
>                 num_pwms = 2;
> 
> . In my eyes the bike shed should be light red and I prefer to collect
> the fallback num_pwms in the compatible_data as is to keep the code
> simpler. Maybe rename the member from num_pwms to fallback_num_pwms to
> make it more obvious that it doesn't represent the actually used value.
> 
> > > With a expressive comment it will help other driver developers to not start
> > > adding num_pwms in the platform data in their first attempt.
> >
> > Definitely.
> 
> My suggestion was to add a dev_warn, which IMHO is still better than a
> comment.
> 
> Best regards
> Uwe
> 

Unrelated to this patch: I'm ready to send v2 to allocate the clks array
dynamically, but I guess MT7628 couldn't work in original code.


In mtk_pwm_config():

	clk = pc->clks[MTK_CLK_PWM1 + pwm->hwpwm]; 
	....
	resolution = (u64)NSEC_PER_SEC * 1000;
	do_div(resolution, clk_get_rate(clk));
	....

I think clk should be NULL and resolution is always 0 here. 


Ryder

Re: [PATCH v1 1/5] pwm: mediatek: add a property "mediatek,num-pwms"

[Ryder Lee]

+John

HI John,

On Mon, 2019-01-21 at 16:49 +0800, Uwe Kleine-König wrote:
> On Sat, Jan 19, 2019 at 10:54:47AM +0800, Ryder Lee wrote:
> > On Fri, 2019-01-18 at 09:43 +0100, Matthias Brugger wrote:
> > >
> > > On 18/01/2019 04:24, Ryder Lee wrote:
> > > > This adds a property "mediatek,num-pwms" to avoid having an endless
> > > > list of compatibles with no differences for the same driver.
> > > >
> > > > Thus, the driver should have backwards compatibility to older DTs.
> > > >
> > > > Signed-off-by: Ryder Lee <ryder.lee@mediatek.com>
> > > > ---
> > > > Changes since v1: add some checks for backwards compatibility.
> > > > ---
> > > >  drivers/pwm/pwm-mediatek.c | 27 ++++++++++++++++++---------
> > > >  1 file changed, 18 insertions(+), 9 deletions(-)
> > > >
> > > > diff --git a/drivers/pwm/pwm-mediatek.c b/drivers/pwm/pwm-mediatek.c
> > > > index eb6674c..81b7e5e 100644
> > > > --- a/drivers/pwm/pwm-mediatek.c
> > > > +++ b/drivers/pwm/pwm-mediatek.c
> > > > @@ -55,7 +55,7 @@ enum {
> > > >  };
> > > >
> > > >  struct mtk_pwm_platform_data {
> > > > - unsigned int num_pwms;
> > > > + unsigned int num_pwms;  /* it should not be used in the future SoCs */
> > > >   bool pwm45_fixup;
> > > >   bool has_clks;
> > > >  };
> > > > @@ -226,27 +226,36 @@ static void mtk_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
> > > >
> > > >  static int mtk_pwm_probe(struct platform_device *pdev)
> > > >  {
> > > > - const struct mtk_pwm_platform_data *data;
> > > > + struct device_node *np = pdev->dev.of_node;
> > > >   struct mtk_pwm_chip *pc;
> > > >   struct resource *res;
> > > > - unsigned int i;
> > > > + unsigned int i, num_pwms;
> > > >   int ret;
> > > >
> > > >   pc = devm_kzalloc(&pdev->dev, sizeof(*pc), GFP_KERNEL);
> > > >   if (!pc)
> > > >           return -ENOMEM;
> > > >
> > > > - data = of_device_get_match_data(&pdev->dev);
> > > > - if (data == NULL)
> > > > -         return -EINVAL;
> > > > - pc->soc = data;
> > > > + pc->soc = of_device_get_match_data(&pdev->dev);
> > > >
> > > >   res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > > >   pc->regs = devm_ioremap_resource(&pdev->dev, res);
> > > >   if (IS_ERR(pc->regs))
> > > >           return PTR_ERR(pc->regs);
> > > >
> > > > - for (i = 0; i < data->num_pwms + 2 && pc->soc->has_clks; i++) {
> > > > + /* Check if we have set 'num-pwms' in DTs. */
> > > > + ret = of_property_read_u32(np, "mediatek,num-pwms", &num_pwms);
> > > > + if (ret < 0) {
> > > > +         /* If no, fallback to use SoC data for backwards compatibility. */
> > > > +         if (pc->soc->num_pwms) {
> > > > +                 num_pwms = pc->soc->num_pwms;
> > >
> > > Maybe that's bike shedding, but I think it would be better to carve out the
> > > num_pwms from the mtk_pwm_platform_data and check against the compatible here.
> >
> > I'm not sure how to properly curve it out? I think we still need this
> > variable to save the specific value for some legacy SoCs (with older
> > DTs).
> 
> I guess he means  something like:
> 
>         if (is_compatible_to_variant_A(dev))
>                 num_pwms = 12;
>         else if (is_compatible_to_variant_B(dev))
>                 num_pwms = 2;
> 
> . In my eyes the bike shed should be light red and I prefer to collect
> the fallback num_pwms in the compatible_data as is to keep the code
> simpler. Maybe rename the member from num_pwms to fallback_num_pwms to
> make it more obvious that it doesn't represent the actually used value.
> 
> > > With a expressive comment it will help other driver developers to not start
> > > adding num_pwms in the platform data in their first attempt.
> >
> > Definitely.
> 
> My suggestion was to add a dev_warn, which IMHO is still better than a
> comment.
> 
> Best regards
> Uwe
> 

Unrelated to this patch: I'm ready to send v2 to allocate the clks array
dynamically, but I guess MT7628 couldn't work in original code.


In mtk_pwm_config():

        clk = pc->clks[MTK_CLK_PWM1 + pwm->hwpwm]; 
        ....
        resolution = (u64)NSEC_PER_SEC * 1000;
        do_div(resolution, clk_get_rate(clk));
        ....

I think clk should be NULL and resolution is always 0 here. 


Ryder

Re: [PATCH v1 2/5] dt-bindings: pwm: add a property "mediatek,num-pwms"

[Rob Herring]

On Fri, Jan 18, 2019 at 09:44:49AM +0100, Matthias Brugger wrote:
> 
> 
> On 18/01/2019 04:24, Ryder Lee wrote:
> > This adds a property "mediatek,num-pwms" in example so that we could
> > set the number of PWM channels via device tree.
> > 
> > Signed-off-by: Ryder Lee <ryder.lee@mediatek.com>
> > Reviewed-by: Matthias Brugger <matthias.bgg@gmail.com>
> > ---
> > Changes since v1: add a Reviewed-by tag.
> > ---
> >  Documentation/devicetree/bindings/pwm/pwm-mediatek.txt | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/pwm/pwm-mediatek.txt b/Documentation/devicetree/bindings/pwm/pwm-mediatek.txt
> > index 991728c..f9e2d1f 100644
> > --- a/Documentation/devicetree/bindings/pwm/pwm-mediatek.txt
> > +++ b/Documentation/devicetree/bindings/pwm/pwm-mediatek.txt
> > @@ -20,6 +20,7 @@ Required properties:
> >   - pinctrl-names: Must contain a "default" entry.
> >   - pinctrl-0: One property must exist for each entry in pinctrl-names.
> >     See pinctrl/pinctrl-bindings.txt for details of the property values.
> > + - mediatek,num-pwms: the number of PWM channels.
> >  
> >  Example:
> >  	pwm0: pwm@11006000 {
> > @@ -37,4 +38,5 @@ Example:
> >  			      "pwm3", "pwm4", "pwm5";
> >  		pinctrl-names = "default";
> >  		pinctrl-0 = <&pwm0_pins>;
> > +		mediatek,num-pwms = <5>;
> >  	};
> > 
> 
> Wasn't there a comment in the last version to use num-pwms instead of
> mediatek,num-pwms?
> 
> Uwe I think you requested that.

Perhaps, but why is this needed? Typically, this would be implied by the 
compatible or the driver doesn't care and we assume 'pwms' properties 
are not out of range.

Rob

Re: [PATCH v1 5/5] dt-bindings: pwm: update bindings for MT7629 SoC

[Rob Herring]

On Fri, 18 Jan 2019 11:24:45 +0800, Ryder Lee wrote:
> This updates bindings for MT7629 pwm controller.
> 
> Signed-off-by: Ryder Lee <ryder.lee@mediatek.com>
> Reviewed-by: Matthias Brugger <matthias.bgg@gmail.com>
> ---
> Changes since v1: add a Reviewed-by tag.
> ---
>  Documentation/devicetree/bindings/pwm/pwm-mediatek.txt | 1 +
>  1 file changed, 1 insertion(+)
> 

Reviewed-by: Rob Herring <robh@kernel.org>

Re: [PATCH v1 2/5] dt-bindings: pwm: add a property "mediatek,num-pwms"

[Uwe Kleine-König]

On Mon, Feb 18, 2019 at 02:36:58PM -0600, Rob Herring wrote:
> On Fri, Jan 18, 2019 at 09:44:49AM +0100, Matthias Brugger wrote:
> > 
> > 
> > On 18/01/2019 04:24, Ryder Lee wrote:
> > > This adds a property "mediatek,num-pwms" in example so that we could
> > > set the number of PWM channels via device tree.
> > > 
> > > Signed-off-by: Ryder Lee <ryder.lee@mediatek.com>
> > > Reviewed-by: Matthias Brugger <matthias.bgg@gmail.com>
> > > ---
> > > Changes since v1: add a Reviewed-by tag.
> > > ---
> > >  Documentation/devicetree/bindings/pwm/pwm-mediatek.txt | 2 ++
> > >  1 file changed, 2 insertions(+)
> > > 
> > > diff --git a/Documentation/devicetree/bindings/pwm/pwm-mediatek.txt b/Documentation/devicetree/bindings/pwm/pwm-mediatek.txt
> > > index 991728c..f9e2d1f 100644
> > > --- a/Documentation/devicetree/bindings/pwm/pwm-mediatek.txt
> > > +++ b/Documentation/devicetree/bindings/pwm/pwm-mediatek.txt
> > > @@ -20,6 +20,7 @@ Required properties:
> > >   - pinctrl-names: Must contain a "default" entry.
> > >   - pinctrl-0: One property must exist for each entry in pinctrl-names.
> > >     See pinctrl/pinctrl-bindings.txt for details of the property values.
> > > + - mediatek,num-pwms: the number of PWM channels.
> > >  
> > >  Example:
> > >  	pwm0: pwm@11006000 {
> > > @@ -37,4 +38,5 @@ Example:
> > >  			      "pwm3", "pwm4", "pwm5";
> > >  		pinctrl-names = "default";
> > >  		pinctrl-0 = <&pwm0_pins>;
> > > +		mediatek,num-pwms = <5>;
> > >  	};
> > > 
> > 
> > Wasn't there a comment in the last version to use num-pwms instead of
> > mediatek,num-pwms?
> > 
> > Uwe I think you requested that.
> 
> Perhaps, but why is this needed? Typically, this would be implied by the 
> compatible or the driver doesn't care and we assume 'pwms' properties 
> are not out of range.

I think it is sensible to standardize such a property because there are
a few drivers that support different variants of PWMs that only differ
in the number of supported pwms. See for example pwm-mediatek and
pwm-sun4i. I wrote a longer motivation in a mail that is available at
https://www.spinics.net/lists/linux-pwm/msg08859.html.

The short version is that there are at least four drivers that have to
solve the same task to determine the number of available pwms from the
compatible. While this isn't a hard task it seems sensible to me to
describe this generic property of a piece of hardware that provides PWMs
in the device tree instead of implementing a table in the driver.

Best regards
Uwe

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