[PATCH v4 0/9] Add the Allwinner A31/A31s PWM driver.

[PATCH v4 0/9] Add the Allwinner A31/A31s PWM driver.

[lis8215-Re5JQEeQqe8AvxtiuMwx3w]

From: Siarhei Volkau <lis8215-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

This is the 4-th version of the sun6i PWM patchset.
Difference between v3 and v4:
 - patchset split on many small patches for easier bisect.
 - avoid unsafe macros usage.
 - some minor cleanups.

First two patches moves register access operations
from custom iomem read-modify-write operations to regmap API.

Patches from 3 to 6 makes all different things between
sun4i and sun6i PWM variants customizable.

Patch 7 introduce sun6i PWM driver itself.

Siarhei Volkau (9):
  pwm: sunxi: Use regmap API for register access.
  pwm: sunxi: Use regmap fields for bit operations.
  pwm: sunxi: Selectable prescaler table.
  pwm: sunxi: Customizable control and period register position.
  pwm: sunxi: Customizable regmap fields and enable bit mask.
  pwm: sunxi: Increase max number of pwm channels.
  pwm: sunxi: Add support the Allwinner A31 PWM.
  pwm: sunxi: Code cleanup.
  ARM: dts: sun6i: Add the PWM block to the A31/A31s.

 .../devicetree/bindings/pwm/pwm-sun4i.txt          |   3 +-
 arch/arm/boot/dts/sun6i-a31.dtsi                   |   8 +
 drivers/pwm/Kconfig                                |   2 +-
 drivers/pwm/pwm-sun4i.c                            | 366 +++++++++++++++++----
 4 files changed, 311 insertions(+), 68 deletions(-)

-- 
2.4.11

[PATCH v4 1/9] pwm: sunxi: Use regmap API for register access.

[lis8215]

From: Siarhei Volkau <lis8215@gmail.com>

The patch replaces iomem register access routines to regmap
equivalents.

Signed-off-by: Siarhei Volkau <lis8215@gmail.com>
---
 drivers/pwm/Kconfig     |   2 +-
 drivers/pwm/pwm-sun4i.c | 143 ++++++++++++++++++++++++++++++++++++------------
 2 files changed, 110 insertions(+), 35 deletions(-)

diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index 2d0cfaa..6b4dc1a 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -416,7 +416,7 @@ config PWM_STMPE
 config PWM_SUN4I
 	tristate "Allwinner PWM support"
 	depends on ARCH_SUNXI || COMPILE_TEST
-	depends on HAS_IOMEM && COMMON_CLK
+	depends on REGMAP_MMIO && COMMON_CLK
 	help
 	  Generic PWM framework driver for Allwinner SoCs.
 
diff --git a/drivers/pwm/pwm-sun4i.c b/drivers/pwm/pwm-sun4i.c
index b0803f6..5565f03 100644
--- a/drivers/pwm/pwm-sun4i.c
+++ b/drivers/pwm/pwm-sun4i.c
@@ -9,7 +9,7 @@
 #include <linux/bitops.h>
 #include <linux/clk.h>
 #include <linux/err.h>
-#include <linux/io.h>
+#include <linux/regmap.h>
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/of_device.h>
@@ -74,7 +74,7 @@ struct sun4i_pwm_data {
 struct sun4i_pwm_chip {
 	struct pwm_chip chip;
 	struct clk *clk;
-	void __iomem *base;
+	struct regmap *regmap;
 	spinlock_t ctrl_lock;
 	const struct sun4i_pwm_data *data;
 };
@@ -84,18 +84,6 @@ static inline struct sun4i_pwm_chip *to_sun4i_pwm_chip(struct pwm_chip *chip)
 	return container_of(chip, struct sun4i_pwm_chip, chip);
 }
 
-static inline u32 sun4i_pwm_readl(struct sun4i_pwm_chip *chip,
-				  unsigned long offset)
-{
-	return readl(chip->base + offset);
-}
-
-static inline void sun4i_pwm_writel(struct sun4i_pwm_chip *chip,
-				    u32 val, unsigned long offset)
-{
-	writel(val, chip->base + offset);
-}
-
 static int sun4i_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 			    int duty_ns, int period_ns)
 {
@@ -152,7 +140,11 @@ static int sun4i_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 	}
 
 	spin_lock(&sun4i_pwm->ctrl_lock);
-	val = sun4i_pwm_readl(sun4i_pwm, PWM_CTRL_REG);
+	err = regmap_read(sun4i_pwm->regmap, PWM_CTRL_REG, &val);
+	if (err) {
+		dev_err(chip->dev, "failed to read from CTL register\n");
+		goto err_cleanup;
+	}
 
 	if (sun4i_pwm->data->has_rdy && (val & PWM_RDY(pwm->hwpwm))) {
 		spin_unlock(&sun4i_pwm->ctrl_lock);
@@ -163,27 +155,53 @@ static int sun4i_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 	clk_gate = val & BIT_CH(PWM_CLK_GATING, pwm->hwpwm);
 	if (clk_gate) {
 		val &= ~BIT_CH(PWM_CLK_GATING, pwm->hwpwm);
-		sun4i_pwm_writel(sun4i_pwm, val, PWM_CTRL_REG);
+		err = regmap_write(sun4i_pwm->regmap, PWM_CTRL_REG, val);
+		if (err) {
+			dev_err(chip->dev, "failed to write to CTL register\n");
+			goto err_cleanup;
+		}
 	}
 
-	val = sun4i_pwm_readl(sun4i_pwm, PWM_CTRL_REG);
+	err = regmap_read(sun4i_pwm->regmap, PWM_CTRL_REG, &val);
+	if (err) {
+		dev_err(chip->dev, "failed to read from CTL register\n");
+		goto err_cleanup;
+	}
 	val &= ~BIT_CH(PWM_PRESCAL_MASK, pwm->hwpwm);
 	val |= BIT_CH(prescaler, pwm->hwpwm);
-	sun4i_pwm_writel(sun4i_pwm, val, PWM_CTRL_REG);
+	err = regmap_write(sun4i_pwm->regmap, PWM_CTRL_REG, val);
+	if (err) {
+		dev_err(chip->dev, "failed to write to CTL register\n");
+		goto err_cleanup;
+	}
 
 	val = (dty & PWM_DTY_MASK) | PWM_PRD(prd);
-	sun4i_pwm_writel(sun4i_pwm, val, PWM_CH_PRD(pwm->hwpwm));
+	err = regmap_write(sun4i_pwm->regmap, PWM_CH_PRD(pwm->hwpwm), val);
+	if (err) {
+		dev_err(chip->dev, "failed to write to PRD register\n");
+		goto err_cleanup;
+	}
 
 	if (clk_gate) {
-		val = sun4i_pwm_readl(sun4i_pwm, PWM_CTRL_REG);
+		err = regmap_read(sun4i_pwm->regmap, PWM_CTRL_REG, &val);
+		if (err) {
+			dev_err(chip->dev,
+				"failed to read from CTL register\n");
+			goto err_cleanup;
+		}
 		val |= clk_gate;
-		sun4i_pwm_writel(sun4i_pwm, val, PWM_CTRL_REG);
+		err = regmap_write(sun4i_pwm->regmap, PWM_CTRL_REG, val);
+		if (err) {
+			dev_err(chip->dev, "failed to write to CTL register\n");
+			goto err_cleanup;
+		}
 	}
 
+err_cleanup:
 	spin_unlock(&sun4i_pwm->ctrl_lock);
 	clk_disable_unprepare(sun4i_pwm->clk);
 
-	return 0;
+	return err;
 }
 
 static int sun4i_pwm_set_polarity(struct pwm_chip *chip, struct pwm_device *pwm,
@@ -200,19 +218,29 @@ static int sun4i_pwm_set_polarity(struct pwm_chip *chip, struct pwm_device *pwm,
 	}
 
 	spin_lock(&sun4i_pwm->ctrl_lock);
-	val = sun4i_pwm_readl(sun4i_pwm, PWM_CTRL_REG);
+	ret = regmap_read(sun4i_pwm->regmap, PWM_CTRL_REG, &val);
+	if (ret) {
+		dev_err(chip->dev,
+			"failed to read from CTL register\n");
+		goto err_cleanup;
+	}
 
 	if (polarity != PWM_POLARITY_NORMAL)
 		val &= ~BIT_CH(PWM_ACT_STATE, pwm->hwpwm);
 	else
 		val |= BIT_CH(PWM_ACT_STATE, pwm->hwpwm);
 
-	sun4i_pwm_writel(sun4i_pwm, val, PWM_CTRL_REG);
+	ret = regmap_write(sun4i_pwm->regmap, PWM_CTRL_REG, val);
+	if (ret) {
+		dev_err(chip->dev, "failed to write to CTL register\n");
+		goto err_cleanup;
+	}
 
+err_cleanup:
 	spin_unlock(&sun4i_pwm->ctrl_lock);
 	clk_disable_unprepare(sun4i_pwm->clk);
 
-	return 0;
+	return ret;
 }
 
 static int sun4i_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
@@ -228,25 +256,53 @@ static int sun4i_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
 	}
 
 	spin_lock(&sun4i_pwm->ctrl_lock);
-	val = sun4i_pwm_readl(sun4i_pwm, PWM_CTRL_REG);
+	ret = regmap_read(sun4i_pwm->regmap, PWM_CTRL_REG, &val);
+	if (ret) {
+		dev_err(chip->dev,
+			"failed to read from CTL register\n");
+		goto err_cleanup;
+	}
 	val |= BIT_CH(PWM_EN, pwm->hwpwm);
 	val |= BIT_CH(PWM_CLK_GATING, pwm->hwpwm);
-	sun4i_pwm_writel(sun4i_pwm, val, PWM_CTRL_REG);
+	ret = regmap_write(sun4i_pwm->regmap, PWM_CTRL_REG, val);
+	if (ret) {
+		dev_err(chip->dev, "failed to write to CTL register\n");
+		goto err_cleanup;
+	}
+
 	spin_unlock(&sun4i_pwm->ctrl_lock);
+	return ret;
 
-	return 0;
+err_cleanup:
+	spin_unlock(&sun4i_pwm->ctrl_lock);
+	if (ret)
+		clk_disable_unprepare(sun4i_pwm->clk);
+
+	return ret;
 }
 
 static void sun4i_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
 {
 	struct sun4i_pwm_chip *sun4i_pwm = to_sun4i_pwm_chip(chip);
 	u32 val;
+	int err;
 
 	spin_lock(&sun4i_pwm->ctrl_lock);
-	val = sun4i_pwm_readl(sun4i_pwm, PWM_CTRL_REG);
+	err = regmap_read(sun4i_pwm->regmap, PWM_CTRL_REG, &val);
+	if (err) {
+		dev_err(chip->dev,
+			"failed to read from CTL register\n");
+		goto err_cleanup;
+	}
 	val &= ~BIT_CH(PWM_EN, pwm->hwpwm);
 	val &= ~BIT_CH(PWM_CLK_GATING, pwm->hwpwm);
-	sun4i_pwm_writel(sun4i_pwm, val, PWM_CTRL_REG);
+	err = regmap_write(sun4i_pwm->regmap, PWM_CTRL_REG, val);
+	if (err) {
+		dev_err(chip->dev, "failed to write to CTL register\n");
+		goto err_cleanup;
+	}
+
+err_cleanup:
 	spin_unlock(&sun4i_pwm->ctrl_lock);
 
 	clk_disable_unprepare(sun4i_pwm->clk);
@@ -312,10 +368,17 @@ static const struct of_device_id sun4i_pwm_dt_ids[] = {
 };
 MODULE_DEVICE_TABLE(of, sun4i_pwm_dt_ids);
 
+static const struct regmap_config sunxi_pwm_regmap_config = {
+	.reg_bits = 32,
+	.val_bits = 32,
+	.reg_stride = 4,
+};
+
 static int sun4i_pwm_probe(struct platform_device *pdev)
 {
 	struct sun4i_pwm_chip *pwm;
 	struct resource *res;
+	void __iomem *base;
 	u32 val;
 	int i, ret;
 	const struct of_device_id *match;
@@ -327,9 +390,14 @@ static int sun4i_pwm_probe(struct platform_device *pdev)
 		return -ENOMEM;
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	pwm->base = devm_ioremap_resource(&pdev->dev, res);
-	if (IS_ERR(pwm->base))
-		return PTR_ERR(pwm->base);
+	base = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(base))
+		return PTR_ERR(base);
+
+	pwm->regmap = devm_regmap_init_mmio(&pdev->dev, base,
+					    &sunxi_pwm_regmap_config);
+	if (IS_ERR(pwm->regmap))
+		return PTR_ERR(pwm->regmap);
 
 	pwm->clk = devm_clk_get(&pdev->dev, NULL);
 	if (IS_ERR(pwm->clk))
@@ -360,7 +428,11 @@ static int sun4i_pwm_probe(struct platform_device *pdev)
 		goto clk_error;
 	}
 
-	val = sun4i_pwm_readl(pwm, PWM_CTRL_REG);
+	ret = regmap_read(pwm->regmap, PWM_CTRL_REG, &val);
+	if (ret) {
+		dev_err(&pdev->dev, "failed to read from CTL register\n");
+		goto read_error;
+	}
 	for (i = 0; i < pwm->chip.npwm; i++)
 		if (!(val & BIT_CH(PWM_ACT_STATE, i)))
 			pwm_set_polarity(&pwm->chip.pwms[i],
@@ -369,6 +441,9 @@ static int sun4i_pwm_probe(struct platform_device *pdev)
 
 	return 0;
 
+read_error:
+	clk_disable_unprepare(pwm->clk);
+
 clk_error:
 	pwmchip_remove(&pwm->chip);
 	return ret;
-- 
2.4.11

Re: [PATCH v4 1/9] pwm: sunxi: Use regmap API for register access.

[Maxime Ripard]

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

Hi Siarhei,

On Fri, Feb 24, 2017 at 08:41:08AM +0300, lis8215-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org wrote:
> From: Siarhei Volkau <lis8215-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> 
> The patch replaces iomem register access routines to regmap
> equivalents.
> 
> Signed-off-by: Siarhei Volkau <lis8215-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>  drivers/pwm/Kconfig     |   2 +-
>  drivers/pwm/pwm-sun4i.c | 143 ++++++++++++++++++++++++++++++++++++------------
>  2 files changed, 110 insertions(+), 35 deletions(-)
> 
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index 2d0cfaa..6b4dc1a 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -416,7 +416,7 @@ config PWM_STMPE
>  config PWM_SUN4I
>  	tristate "Allwinner PWM support"
>  	depends on ARCH_SUNXI || COMPILE_TEST
> -	depends on HAS_IOMEM && COMMON_CLK
> +	depends on REGMAP_MMIO && COMMON_CLK
>  	help
>  	  Generic PWM framework driver for Allwinner SoCs.
>  
> diff --git a/drivers/pwm/pwm-sun4i.c b/drivers/pwm/pwm-sun4i.c
> index b0803f6..5565f03 100644
> --- a/drivers/pwm/pwm-sun4i.c
> +++ b/drivers/pwm/pwm-sun4i.c
> @@ -9,7 +9,7 @@
>  #include <linux/bitops.h>
>  #include <linux/clk.h>
>  #include <linux/err.h>
> -#include <linux/io.h>
> +#include <linux/regmap.h>
>  #include <linux/module.h>
>  #include <linux/of.h>
>  #include <linux/of_device.h>
> @@ -74,7 +74,7 @@ struct sun4i_pwm_data {
>  struct sun4i_pwm_chip {
>  	struct pwm_chip chip;
>  	struct clk *clk;
> -	void __iomem *base;
> +	struct regmap *regmap;
>  	spinlock_t ctrl_lock;
>  	const struct sun4i_pwm_data *data;
>  };
> @@ -84,18 +84,6 @@ static inline struct sun4i_pwm_chip *to_sun4i_pwm_chip(struct pwm_chip *chip)
>  	return container_of(chip, struct sun4i_pwm_chip, chip);
>  }
>  
> -static inline u32 sun4i_pwm_readl(struct sun4i_pwm_chip *chip,
> -				  unsigned long offset)
> -{
> -	return readl(chip->base + offset);
> -}
> -
> -static inline void sun4i_pwm_writel(struct sun4i_pwm_chip *chip,
> -				    u32 val, unsigned long offset)
> -{
> -	writel(val, chip->base + offset);
> -}
> -
>  static int sun4i_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
>  			    int duty_ns, int period_ns)
>  {
> @@ -152,7 +140,11 @@ static int sun4i_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
>  	}
>  
>  	spin_lock(&sun4i_pwm->ctrl_lock);
> -	val = sun4i_pwm_readl(sun4i_pwm, PWM_CTRL_REG);
> +	err = regmap_read(sun4i_pwm->regmap, PWM_CTRL_REG, &val);
> +	if (err) {
> +		dev_err(chip->dev, "failed to read from CTL register\n");
> +		goto err_cleanup;
> +	}

I'm not sure you need those error checks. If there's an error when you
write to an MMIO bus, you have much more important issues than your
return code there.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Re: [PATCH v4 1/9] pwm: sunxi: Use regmap API for register access.

[Siarhei Volkau]

Hi, Maxime

2017-02-27 12:17 GMT+03:00 Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>:
> Hi Siarhei,
>
> On Fri, Feb 24, 2017 at 08:41:08AM +0300, lis8215-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org wrote:
>> From: Siarhei Volkau <lis8215-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>>
>> The patch replaces iomem register access routines to regmap
>> equivalents.
>>
>> Signed-off-by: Siarhei Volkau <lis8215-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> ---
>>  drivers/pwm/Kconfig     |   2 +-
>>  drivers/pwm/pwm-sun4i.c | 143 ++++++++++++++++++++++++++++++++++++------------
>>  2 files changed, 110 insertions(+), 35 deletions(-)
>>
>> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
>> index 2d0cfaa..6b4dc1a 100644
>> --- a/drivers/pwm/Kconfig
>> +++ b/drivers/pwm/Kconfig
>> @@ -416,7 +416,7 @@ config PWM_STMPE
>>  config PWM_SUN4I
>>       tristate "Allwinner PWM support"
>>       depends on ARCH_SUNXI || COMPILE_TEST
>> -     depends on HAS_IOMEM && COMMON_CLK
>> +     depends on REGMAP_MMIO && COMMON_CLK
>>       help
>>         Generic PWM framework driver for Allwinner SoCs.
>>
>> diff --git a/drivers/pwm/pwm-sun4i.c b/drivers/pwm/pwm-sun4i.c
>> index b0803f6..5565f03 100644
>> --- a/drivers/pwm/pwm-sun4i.c
>> +++ b/drivers/pwm/pwm-sun4i.c
>> @@ -9,7 +9,7 @@
>>  #include <linux/bitops.h>
>>  #include <linux/clk.h>
>>  #include <linux/err.h>
>> -#include <linux/io.h>
>> +#include <linux/regmap.h>
>>  #include <linux/module.h>
>>  #include <linux/of.h>
>>  #include <linux/of_device.h>
>> @@ -74,7 +74,7 @@ struct sun4i_pwm_data {
>>  struct sun4i_pwm_chip {
>>       struct pwm_chip chip;
>>       struct clk *clk;
>> -     void __iomem *base;
>> +     struct regmap *regmap;
>>       spinlock_t ctrl_lock;
>>       const struct sun4i_pwm_data *data;
>>  };
>> @@ -84,18 +84,6 @@ static inline struct sun4i_pwm_chip *to_sun4i_pwm_chip(struct pwm_chip *chip)
>>       return container_of(chip, struct sun4i_pwm_chip, chip);
>>  }
>>
>> -static inline u32 sun4i_pwm_readl(struct sun4i_pwm_chip *chip,
>> -                               unsigned long offset)
>> -{
>> -     return readl(chip->base + offset);
>> -}
>> -
>> -static inline void sun4i_pwm_writel(struct sun4i_pwm_chip *chip,
>> -                                 u32 val, unsigned long offset)
>> -{
>> -     writel(val, chip->base + offset);
>> -}
>> -
>>  static int sun4i_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
>>                           int duty_ns, int period_ns)
>>  {
>> @@ -152,7 +140,11 @@ static int sun4i_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
>>       }
>>
>>       spin_lock(&sun4i_pwm->ctrl_lock);
>> -     val = sun4i_pwm_readl(sun4i_pwm, PWM_CTRL_REG);
>> +     err = regmap_read(sun4i_pwm->regmap, PWM_CTRL_REG, &val);
>> +     if (err) {
>> +             dev_err(chip->dev, "failed to read from CTL register\n");
>> +             goto err_cleanup;
>> +     }
>
> I'm not sure you need those error checks. If there's an error when you
> write to an MMIO bus, you have much more important issues than your
> return code there.
>
> Maxime
>
> --
> Maxime Ripard, Free Electrons
> Embedded Linux and Kernel engineering
> http://free-electrons.com

This probably overkill for MMIO, but it helps when wrong parameters passed
to regmap functions - mostly during debugging stage.

Siarhei

Re: [PATCH v4 1/9] pwm: sunxi: Use regmap API for register access.

[Maxime Ripard]

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

On Mon, Feb 27, 2017 at 02:22:02PM +0300, Siarhei Volkau wrote:
> Hi, Maxime
> 
> 2017-02-27 12:17 GMT+03:00 Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>:
> > Hi Siarhei,
> >
> > On Fri, Feb 24, 2017 at 08:41:08AM +0300, lis8215-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org wrote:
> >> From: Siarhei Volkau <lis8215-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> >>
> >> The patch replaces iomem register access routines to regmap
> >> equivalents.
> >>
> >> Signed-off-by: Siarhei Volkau <lis8215-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> >> ---
> >>  drivers/pwm/Kconfig     |   2 +-
> >>  drivers/pwm/pwm-sun4i.c | 143 ++++++++++++++++++++++++++++++++++++------------
> >>  2 files changed, 110 insertions(+), 35 deletions(-)
> >>
> >> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> >> index 2d0cfaa..6b4dc1a 100644
> >> --- a/drivers/pwm/Kconfig
> >> +++ b/drivers/pwm/Kconfig
> >> @@ -416,7 +416,7 @@ config PWM_STMPE
> >>  config PWM_SUN4I
> >>       tristate "Allwinner PWM support"
> >>       depends on ARCH_SUNXI || COMPILE_TEST
> >> -     depends on HAS_IOMEM && COMMON_CLK
> >> +     depends on REGMAP_MMIO && COMMON_CLK
> >>       help
> >>         Generic PWM framework driver for Allwinner SoCs.
> >>
> >> diff --git a/drivers/pwm/pwm-sun4i.c b/drivers/pwm/pwm-sun4i.c
> >> index b0803f6..5565f03 100644
> >> --- a/drivers/pwm/pwm-sun4i.c
> >> +++ b/drivers/pwm/pwm-sun4i.c
> >> @@ -9,7 +9,7 @@
> >>  #include <linux/bitops.h>
> >>  #include <linux/clk.h>
> >>  #include <linux/err.h>
> >> -#include <linux/io.h>
> >> +#include <linux/regmap.h>
> >>  #include <linux/module.h>
> >>  #include <linux/of.h>
> >>  #include <linux/of_device.h>
> >> @@ -74,7 +74,7 @@ struct sun4i_pwm_data {
> >>  struct sun4i_pwm_chip {
> >>       struct pwm_chip chip;
> >>       struct clk *clk;
> >> -     void __iomem *base;
> >> +     struct regmap *regmap;
> >>       spinlock_t ctrl_lock;
> >>       const struct sun4i_pwm_data *data;
> >>  };
> >> @@ -84,18 +84,6 @@ static inline struct sun4i_pwm_chip *to_sun4i_pwm_chip(struct pwm_chip *chip)
> >>       return container_of(chip, struct sun4i_pwm_chip, chip);
> >>  }
> >>
> >> -static inline u32 sun4i_pwm_readl(struct sun4i_pwm_chip *chip,
> >> -                               unsigned long offset)
> >> -{
> >> -     return readl(chip->base + offset);
> >> -}
> >> -
> >> -static inline void sun4i_pwm_writel(struct sun4i_pwm_chip *chip,
> >> -                                 u32 val, unsigned long offset)
> >> -{
> >> -     writel(val, chip->base + offset);
> >> -}
> >> -
> >>  static int sun4i_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
> >>                           int duty_ns, int period_ns)
> >>  {
> >> @@ -152,7 +140,11 @@ static int sun4i_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
> >>       }
> >>
> >>       spin_lock(&sun4i_pwm->ctrl_lock);
> >> -     val = sun4i_pwm_readl(sun4i_pwm, PWM_CTRL_REG);
> >> +     err = regmap_read(sun4i_pwm->regmap, PWM_CTRL_REG, &val);
> >> +     if (err) {
> >> +             dev_err(chip->dev, "failed to read from CTL register\n");
> >> +             goto err_cleanup;
> >> +     }
> >
> > I'm not sure you need those error checks. If there's an error when you
> > write to an MMIO bus, you have much more important issues than your
> > return code there.
> 
> This probably overkill for MMIO, but it helps when wrong parameters
> passed to regmap functions - mostly during debugging stage.

There's no way you can get it wrong with regmap_read / write here. And
what's useful during debugging may not be fit for real world use
case. We never had error checking before, it was working just fine,
this is just as true here.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[PATCH v4 3/9] pwm: sunxi: Selectable prescaler table.

[lis8215]

From: Siarhei Volkau <lis8215@gmail.com>

A31 SoC have a different set of prescalers than sun4i
compatible ASoCs, but its position and count in the control
register are the same.

This patch make the table of prescalers customizable.

Signed-off-by: Siarhei Volkau <lis8215@gmail.com>
---
 drivers/pwm/pwm-sun4i.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/pwm/pwm-sun4i.c b/drivers/pwm/pwm-sun4i.c
index 7af6bd8..418a625 100644
--- a/drivers/pwm/pwm-sun4i.c
+++ b/drivers/pwm/pwm-sun4i.c
@@ -60,7 +60,7 @@ enum {
 	NUM_FIELDS,
 };
 
-static const u32 prescaler_table[] = {
+static const u32 sun4i_prescaler_table[] = {
 	120,
 	180,
 	240,
@@ -83,6 +83,7 @@ struct sun4i_pwm_data {
 	bool has_prescaler_bypass;
 	bool has_rdy;
 	unsigned int npwm;
+	const u32 *prescaler_table;
 };
 
 struct sun4i_pwm_chip {
@@ -104,6 +105,7 @@ static int sun4i_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 {
 	struct sun4i_pwm_chip *sun4i_pwm = to_sun4i_pwm_chip(chip);
 	struct regmap_field **chan_fields = sun4i_pwm->fields[pwm->hwpwm];
+	const u32 *prescaler_table = sun4i_pwm->data->prescaler_table;
 	u32 prd, dty, val, clk_gate;
 	u64 clk_rate, div = 0;
 	unsigned int prescaler = 0;
@@ -310,30 +312,35 @@ static const struct sun4i_pwm_data sun4i_pwm_data_a10 = {
 	.has_prescaler_bypass = false,
 	.has_rdy = false,
 	.npwm = 2,
+	.prescaler_table = sun4i_prescaler_table,
 };
 
 static const struct sun4i_pwm_data sun4i_pwm_data_a10s = {
 	.has_prescaler_bypass = true,
 	.has_rdy = true,
 	.npwm = 2,
+	.prescaler_table = sun4i_prescaler_table,
 };
 
 static const struct sun4i_pwm_data sun4i_pwm_data_a13 = {
 	.has_prescaler_bypass = true,
 	.has_rdy = true,
 	.npwm = 1,
+	.prescaler_table = sun4i_prescaler_table,
 };
 
 static const struct sun4i_pwm_data sun4i_pwm_data_a20 = {
 	.has_prescaler_bypass = true,
 	.has_rdy = true,
 	.npwm = 2,
+	.prescaler_table = sun4i_prescaler_table,
 };
 
 static const struct sun4i_pwm_data sun4i_pwm_data_h3 = {
 	.has_prescaler_bypass = true,
 	.has_rdy = true,
 	.npwm = 1,
+	.prescaler_table = sun4i_prescaler_table,
 };
 
 static const struct of_device_id sun4i_pwm_dt_ids[] = {
-- 
2.4.11

Re: [PATCH v4 3/9] pwm: sunxi: Selectable prescaler table.

[Maxime Ripard]

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

On Fri, Feb 24, 2017 at 08:41:10AM +0300, lis8215-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org wrote:
> From: Siarhei Volkau <lis8215-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> 
> A31 SoC have a different set of prescalers than sun4i
> compatible ASoCs, but its position and count in the control
> register are the same.
> 
> This patch make the table of prescalers customizable.
> 
> Signed-off-by: Siarhei Volkau <lis8215-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

Acked-by: Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>

Thanks,
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[PATCH v4 2/9] pwm: sunxi: Use regmap fields for bit operations.

[lis8215-Re5JQEeQqe8AvxtiuMwx3w]

From: Siarhei Volkau <lis8215-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

This patch replaces a bunch of custom read-modify-write operations
by regmap fields.

Signed-off-by: Siarhei Volkau <lis8215-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 drivers/pwm/pwm-sun4i.c | 197 ++++++++++++++++++++++++++----------------------
 1 file changed, 108 insertions(+), 89 deletions(-)

diff --git a/drivers/pwm/pwm-sun4i.c b/drivers/pwm/pwm-sun4i.c
index 5565f03..7af6bd8 100644
--- a/drivers/pwm/pwm-sun4i.c
+++ b/drivers/pwm/pwm-sun4i.c
@@ -46,6 +46,20 @@
 
 #define BIT_CH(bit, chan)	((bit) << ((chan) * PWMCH_OFFSET))
 
+#define SUN4I_MAX_PWM_CHANNELS	2
+
+/* regmap fields */
+enum {
+	/* Used bit fields in control register */
+	FIELD_PRESCALER = 0,
+	FIELD_POLARITY,
+	FIELD_CLK_GATING,
+	FIELD_READY,
+
+	/* Keep last */
+	NUM_FIELDS,
+};
+
 static const u32 prescaler_table[] = {
 	120,
 	180,
@@ -75,6 +89,7 @@ struct sun4i_pwm_chip {
 	struct pwm_chip chip;
 	struct clk *clk;
 	struct regmap *regmap;
+	struct regmap_field *fields[SUN4I_MAX_PWM_CHANNELS][NUM_FIELDS];
 	spinlock_t ctrl_lock;
 	const struct sun4i_pwm_data *data;
 };
@@ -88,6 +103,7 @@ static int sun4i_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 			    int duty_ns, int period_ns)
 {
 	struct sun4i_pwm_chip *sun4i_pwm = to_sun4i_pwm_chip(chip);
+	struct regmap_field **chan_fields = sun4i_pwm->fields[pwm->hwpwm];
 	u32 prd, dty, val, clk_gate;
 	u64 clk_rate, div = 0;
 	unsigned int prescaler = 0;
@@ -140,38 +156,36 @@ static int sun4i_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 	}
 
 	spin_lock(&sun4i_pwm->ctrl_lock);
-	err = regmap_read(sun4i_pwm->regmap, PWM_CTRL_REG, &val);
-	if (err) {
-		dev_err(chip->dev, "failed to read from CTL register\n");
-		goto err_cleanup;
-	}
 
-	if (sun4i_pwm->data->has_rdy && (val & PWM_RDY(pwm->hwpwm))) {
-		spin_unlock(&sun4i_pwm->ctrl_lock);
-		clk_disable_unprepare(sun4i_pwm->clk);
-		return -EBUSY;
-	}
-
-	clk_gate = val & BIT_CH(PWM_CLK_GATING, pwm->hwpwm);
-	if (clk_gate) {
-		val &= ~BIT_CH(PWM_CLK_GATING, pwm->hwpwm);
-		err = regmap_write(sun4i_pwm->regmap, PWM_CTRL_REG, val);
+	if (sun4i_pwm->data->has_rdy) {
+		err = regmap_field_read(chan_fields[FIELD_READY], &val);
 		if (err) {
-			dev_err(chip->dev, "failed to write to CTL register\n");
+			dev_err(chip->dev, "failed to read ready bit\n");
+			goto err_cleanup;
+		}
+		if (val) {
+			err = -EBUSY;
 			goto err_cleanup;
 		}
 	}
 
-	err = regmap_read(sun4i_pwm->regmap, PWM_CTRL_REG, &val);
+	err = regmap_field_read(chan_fields[FIELD_CLK_GATING], &clk_gate);
 	if (err) {
-		dev_err(chip->dev, "failed to read from CTL register\n");
+		dev_err(chip->dev, "failed to read clock_gating bit\n");
 		goto err_cleanup;
 	}
-	val &= ~BIT_CH(PWM_PRESCAL_MASK, pwm->hwpwm);
-	val |= BIT_CH(prescaler, pwm->hwpwm);
-	err = regmap_write(sun4i_pwm->regmap, PWM_CTRL_REG, val);
+	if (clk_gate) {
+		err = regmap_field_write(chan_fields[FIELD_CLK_GATING], 0);
+		if (err) {
+			dev_err(chip->dev,
+				"failed to write to clock_gating bit\n");
+			goto err_cleanup;
+		}
+	}
+
+	err = regmap_field_write(chan_fields[FIELD_PRESCALER], prescaler);
 	if (err) {
-		dev_err(chip->dev, "failed to write to CTL register\n");
+		dev_err(chip->dev, "failed to write to prescaler field\n");
 		goto err_cleanup;
 	}
 
@@ -183,16 +197,10 @@ static int sun4i_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 	}
 
 	if (clk_gate) {
-		err = regmap_read(sun4i_pwm->regmap, PWM_CTRL_REG, &val);
+		err = regmap_field_write(chan_fields[FIELD_CLK_GATING], 1);
 		if (err) {
 			dev_err(chip->dev,
-				"failed to read from CTL register\n");
-			goto err_cleanup;
-		}
-		val |= clk_gate;
-		err = regmap_write(sun4i_pwm->regmap, PWM_CTRL_REG, val);
-		if (err) {
-			dev_err(chip->dev, "failed to write to CTL register\n");
+				"failed to write to clock_gating bit\n");
 			goto err_cleanup;
 		}
 	}
@@ -208,7 +216,7 @@ static int sun4i_pwm_set_polarity(struct pwm_chip *chip, struct pwm_device *pwm,
 				  enum pwm_polarity polarity)
 {
 	struct sun4i_pwm_chip *sun4i_pwm = to_sun4i_pwm_chip(chip);
-	u32 val;
+	struct regmap_field **chan_fields = sun4i_pwm->fields[pwm->hwpwm];
 	int ret;
 
 	ret = clk_prepare_enable(sun4i_pwm->clk);
@@ -218,27 +226,14 @@ static int sun4i_pwm_set_polarity(struct pwm_chip *chip, struct pwm_device *pwm,
 	}
 
 	spin_lock(&sun4i_pwm->ctrl_lock);
-	ret = regmap_read(sun4i_pwm->regmap, PWM_CTRL_REG, &val);
-	if (ret) {
-		dev_err(chip->dev,
-			"failed to read from CTL register\n");
-		goto err_cleanup;
-	}
-
-	if (polarity != PWM_POLARITY_NORMAL)
-		val &= ~BIT_CH(PWM_ACT_STATE, pwm->hwpwm);
-	else
-		val |= BIT_CH(PWM_ACT_STATE, pwm->hwpwm);
-
-	ret = regmap_write(sun4i_pwm->regmap, PWM_CTRL_REG, val);
-	if (ret) {
-		dev_err(chip->dev, "failed to write to CTL register\n");
-		goto err_cleanup;
-	}
+	ret = regmap_field_write(chan_fields[FIELD_POLARITY],
+				 polarity == PWM_POLARITY_NORMAL);
+	if (ret)
+		dev_err(chip->dev, "failed to write to polarity bit\n");
 
-err_cleanup:
 	spin_unlock(&sun4i_pwm->ctrl_lock);
-	clk_disable_unprepare(sun4i_pwm->clk);
+	if (ret)
+		clk_disable_unprepare(sun4i_pwm->clk);
 
 	return ret;
 }
@@ -246,7 +241,7 @@ static int sun4i_pwm_set_polarity(struct pwm_chip *chip, struct pwm_device *pwm,
 static int sun4i_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
 {
 	struct sun4i_pwm_chip *sun4i_pwm = to_sun4i_pwm_chip(chip);
-	u32 val;
+	u32 mask;
 	int ret;
 
 	ret = clk_prepare_enable(sun4i_pwm->clk);
@@ -255,54 +250,33 @@ static int sun4i_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
 		return ret;
 	}
 
+	mask = BIT_CH(PWM_EN, pwm->hwpwm);
+	mask |= BIT_CH(PWM_CLK_GATING, pwm->hwpwm);
+
 	spin_lock(&sun4i_pwm->ctrl_lock);
-	ret = regmap_read(sun4i_pwm->regmap, PWM_CTRL_REG, &val);
-	if (ret) {
-		dev_err(chip->dev,
-			"failed to read from CTL register\n");
-		goto err_cleanup;
-	}
-	val |= BIT_CH(PWM_EN, pwm->hwpwm);
-	val |= BIT_CH(PWM_CLK_GATING, pwm->hwpwm);
-	ret = regmap_write(sun4i_pwm->regmap, PWM_CTRL_REG, val);
-	if (ret) {
+	ret = regmap_update_bits(sun4i_pwm->regmap, PWM_CTRL_REG, mask, mask);
+	if (ret)
 		dev_err(chip->dev, "failed to write to CTL register\n");
-		goto err_cleanup;
-	}
-
 	spin_unlock(&sun4i_pwm->ctrl_lock);
-	return ret;
 
-err_cleanup:
-	spin_unlock(&sun4i_pwm->ctrl_lock);
 	if (ret)
 		clk_disable_unprepare(sun4i_pwm->clk);
-
 	return ret;
 }
 
 static void sun4i_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
 {
 	struct sun4i_pwm_chip *sun4i_pwm = to_sun4i_pwm_chip(chip);
-	u32 val;
+	u32 mask;
 	int err;
 
+	mask = BIT_CH(PWM_EN, pwm->hwpwm);
+	mask |= BIT_CH(PWM_CLK_GATING, pwm->hwpwm);
+
 	spin_lock(&sun4i_pwm->ctrl_lock);
-	err = regmap_read(sun4i_pwm->regmap, PWM_CTRL_REG, &val);
-	if (err) {
-		dev_err(chip->dev,
-			"failed to read from CTL register\n");
-		goto err_cleanup;
-	}
-	val &= ~BIT_CH(PWM_EN, pwm->hwpwm);
-	val &= ~BIT_CH(PWM_CLK_GATING, pwm->hwpwm);
-	err = regmap_write(sun4i_pwm->regmap, PWM_CTRL_REG, val);
-	if (err) {
+	err = regmap_update_bits(sun4i_pwm->regmap, PWM_CTRL_REG, mask, 0);
+	if (err)
 		dev_err(chip->dev, "failed to write to CTL register\n");
-		goto err_cleanup;
-	}
-
-err_cleanup:
 	spin_unlock(&sun4i_pwm->ctrl_lock);
 
 	clk_disable_unprepare(sun4i_pwm->clk);
@@ -316,6 +290,22 @@ static const struct pwm_ops sun4i_pwm_ops = {
 	.owner = THIS_MODULE,
 };
 
+static const struct reg_field
+sun4i_pwm_regfields[SUN4I_MAX_PWM_CHANNELS][NUM_FIELDS] = {
+	{
+		[FIELD_PRESCALER]  = REG_FIELD(PWM_CTRL_REG,  0,  3),
+		[FIELD_POLARITY]   = REG_FIELD(PWM_CTRL_REG,  5,  5),
+		[FIELD_CLK_GATING] = REG_FIELD(PWM_CTRL_REG,  6,  6),
+		[FIELD_READY]      = REG_FIELD(PWM_CTRL_REG, 28, 28),
+	},
+	{
+		[FIELD_PRESCALER]  = REG_FIELD(PWM_CTRL_REG, 15, 18),
+		[FIELD_POLARITY]   = REG_FIELD(PWM_CTRL_REG, 20, 20),
+		[FIELD_CLK_GATING] = REG_FIELD(PWM_CTRL_REG, 21, 21),
+		[FIELD_READY]      = REG_FIELD(PWM_CTRL_REG, 29, 29),
+	},
+};
+
 static const struct sun4i_pwm_data sun4i_pwm_data_a10 = {
 	.has_prescaler_bypass = false,
 	.has_rdy = false,
@@ -374,6 +364,28 @@ static const struct regmap_config sunxi_pwm_regmap_config = {
 	.reg_stride = 4,
 };
 
+static int sun4i_alloc_reg_fields(struct device *dev,
+				  struct sun4i_pwm_chip *pwm, int chan)
+{
+	int i, err;
+
+	if (chan >= SUN4I_MAX_PWM_CHANNELS)
+		return -EINVAL;
+	for (i = 0; i < NUM_FIELDS; i++) {
+		pwm->fields[chan][i] =
+			devm_regmap_field_alloc(dev, pwm->regmap,
+						sun4i_pwm_regfields[chan][i]);
+		if (IS_ERR(pwm->fields[chan][i])) {
+			dev_err(dev, "regmap field allocation failed\n");
+			err = PTR_ERR(pwm->fields[chan][i]);
+			pwm->fields[chan][i] = NULL;
+			return err;
+		}
+	}
+
+	return 0;
+}
+
 static int sun4i_pwm_probe(struct platform_device *pdev)
 {
 	struct sun4i_pwm_chip *pwm;
@@ -428,15 +440,22 @@ static int sun4i_pwm_probe(struct platform_device *pdev)
 		goto clk_error;
 	}
 
-	ret = regmap_read(pwm->regmap, PWM_CTRL_REG, &val);
-	if (ret) {
-		dev_err(&pdev->dev, "failed to read from CTL register\n");
-		goto read_error;
+	for (i = 0; i < pwm->chip.npwm; i++) {
+		ret = sun4i_alloc_reg_fields(&pdev->dev, pwm, i);
+		if (ret)
+			goto read_error;
 	}
-	for (i = 0; i < pwm->chip.npwm; i++)
-		if (!(val & BIT_CH(PWM_ACT_STATE, i)))
+
+	for (i = 0; i < pwm->chip.npwm; i++) {
+		ret = regmap_field_read(pwm->fields[i][FIELD_POLARITY], &val);
+		if (ret) {
+			dev_err(&pdev->dev, "failed to read polarity bit\n");
+			goto read_error;
+		}
+		if (val)
 			pwm_set_polarity(&pwm->chip.pwms[i],
 					 PWM_POLARITY_INVERSED);
+	}
 	clk_disable_unprepare(pwm->clk);
 
 	return 0;
-- 
2.4.11

Re: [PATCH v4 2/9] pwm: sunxi: Use regmap fields for bit operations.

[Maxime Ripard]

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

Hi,

On Fri, Feb 24, 2017 at 08:41:09AM +0300, lis8215-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org wrote:
> +static const struct reg_field
> +sun4i_pwm_regfields[SUN4I_MAX_PWM_CHANNELS][NUM_FIELDS] = {
> +	{
> +		[FIELD_PRESCALER]  = REG_FIELD(PWM_CTRL_REG,  0,  3),
> +		[FIELD_POLARITY]   = REG_FIELD(PWM_CTRL_REG,  5,  5),
> +		[FIELD_CLK_GATING] = REG_FIELD(PWM_CTRL_REG,  6,  6),
> +		[FIELD_READY]      = REG_FIELD(PWM_CTRL_REG, 28, 28),
> +	},
> +	{
> +		[FIELD_PRESCALER]  = REG_FIELD(PWM_CTRL_REG, 15, 18),
> +		[FIELD_POLARITY]   = REG_FIELD(PWM_CTRL_REG, 20, 20),
> +		[FIELD_CLK_GATING] = REG_FIELD(PWM_CTRL_REG, 21, 21),
> +		[FIELD_READY]      = REG_FIELD(PWM_CTRL_REG, 29, 29),
> +	},
> +};
> +

I'm not sure you need something that complicated.

What about something like:

struct sun4i_chan_fields {
       struct reg_field	prescaler;
       struct reg_field	polarity;
       struct reg_field	clk_gating;
       struct reg_field	ready;
};

And in sun4i_pwm_data add an array of this struct.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Re: [PATCH v4 2/9] pwm: sunxi: Use regmap fields for bit operations.

[Siarhei Volkau]

Hi,

2017-02-27 12:28 GMT+03:00 Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>:
>
> Hi,
>
> On Fri, Feb 24, 2017 at 08:41:09AM +0300, lis8215-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org wrote:
> > +static const struct reg_field
> > +sun4i_pwm_regfields[SUN4I_MAX_PWM_CHANNELS][NUM_FIELDS] = {
> > +     {
> > +             [FIELD_PRESCALER]  = REG_FIELD(PWM_CTRL_REG,  0,  3),
> > +             [FIELD_POLARITY]   = REG_FIELD(PWM_CTRL_REG,  5,  5),
> > +             [FIELD_CLK_GATING] = REG_FIELD(PWM_CTRL_REG,  6,  6),
> > +             [FIELD_READY]      = REG_FIELD(PWM_CTRL_REG, 28, 28),
> > +     },
> > +     {
> > +             [FIELD_PRESCALER]  = REG_FIELD(PWM_CTRL_REG, 15, 18),
> > +             [FIELD_POLARITY]   = REG_FIELD(PWM_CTRL_REG, 20, 20),
> > +             [FIELD_CLK_GATING] = REG_FIELD(PWM_CTRL_REG, 21, 21),
> > +             [FIELD_READY]      = REG_FIELD(PWM_CTRL_REG, 29, 29),
> > +     },
> > +};
> > +
>
> I'm not sure you need something that complicated.
>
> What about something like:
>
> struct sun4i_chan_fields {
>        struct reg_field prescaler;
>        struct reg_field polarity;
>        struct reg_field clk_gating;
>        struct reg_field ready;
> };
>
> And in sun4i_pwm_data add an array of this struct.
>
> Maxime
>
> --
> Maxime Ripard, Free Electrons
> Embedded Linux and Kernel engineering
> http://free-electrons.com

Code with struct looks cleaner, but in this case need to be
written separate code for each reg_field entry allocation,
please look at the sunxi_alloc_reg_fields() function.

Current solution allows adding new fields slightly easier.

Siarhei

Re: [PATCH v4 2/9] pwm: sunxi: Use regmap fields for bit operations.

[Maxime Ripard]

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

On Mon, Feb 27, 2017 at 02:41:10PM +0300, Siarhei Volkau wrote:
> Hi,
> 
> 2017-02-27 12:28 GMT+03:00 Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>:
> >
> > Hi,
> >
> > On Fri, Feb 24, 2017 at 08:41:09AM +0300, lis8215-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org wrote:
> > > +static const struct reg_field
> > > +sun4i_pwm_regfields[SUN4I_MAX_PWM_CHANNELS][NUM_FIELDS] = {
> > > +     {
> > > +             [FIELD_PRESCALER]  = REG_FIELD(PWM_CTRL_REG,  0,  3),
> > > +             [FIELD_POLARITY]   = REG_FIELD(PWM_CTRL_REG,  5,  5),
> > > +             [FIELD_CLK_GATING] = REG_FIELD(PWM_CTRL_REG,  6,  6),
> > > +             [FIELD_READY]      = REG_FIELD(PWM_CTRL_REG, 28, 28),
> > > +     },
> > > +     {
> > > +             [FIELD_PRESCALER]  = REG_FIELD(PWM_CTRL_REG, 15, 18),
> > > +             [FIELD_POLARITY]   = REG_FIELD(PWM_CTRL_REG, 20, 20),
> > > +             [FIELD_CLK_GATING] = REG_FIELD(PWM_CTRL_REG, 21, 21),
> > > +             [FIELD_READY]      = REG_FIELD(PWM_CTRL_REG, 29, 29),
> > > +     },
> > > +};
> > > +
> >
> > I'm not sure you need something that complicated.
> >
> > What about something like:
> >
> > struct sun4i_chan_fields {
> >        struct reg_field prescaler;
> >        struct reg_field polarity;
> >        struct reg_field clk_gating;
> >        struct reg_field ready;
> > };
> >
> > And in sun4i_pwm_data add an array of this struct.
> >
> > Maxime
> >
> > --
> > Maxime Ripard, Free Electrons
> > Embedded Linux and Kernel engineering
> > http://free-electrons.com
> 
> Code with struct looks cleaner, but in this case need to be
> written separate code for each reg_field entry allocation,
> please look at the sunxi_alloc_reg_fields() function.
> 
> Current solution allows adding new fields slightly easier.

Yet, you also end up with a similar pattern in your later patches,
with the pwmch_data structure. But since using that structure for that
was not as easy as it was supposed to be, you just dropped reg_field
entirely for those.

Really, consistency is key, and having a structure like this, even if
slightly less easy to initialise (but that's not rocket science
either) provides that consistency that makes it easier to review,
understand and maintain.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[PATCH v4 4/9] pwm: sunxi: Customizable control and period register position.

[lis8215-Re5JQEeQqe8AvxtiuMwx3w]

From: Siarhei Volkau <lis8215-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

sun6i has same registers as sun4i compatible chips, but its position
in register map are different.

This patch make register's position selectable for support sun6i in
next patches.

Signed-off-by: Siarhei Volkau <lis8215-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 drivers/pwm/pwm-sun4i.c | 57 ++++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 54 insertions(+), 3 deletions(-)

diff --git a/drivers/pwm/pwm-sun4i.c b/drivers/pwm/pwm-sun4i.c
index 418a625..9ddc812 100644
--- a/drivers/pwm/pwm-sun4i.c
+++ b/drivers/pwm/pwm-sun4i.c
@@ -79,11 +79,17 @@ static const u32 sun4i_prescaler_table[] = {
 	0, /* Actually 1 but tested separately */
 };
 
+struct sunxi_pwmch_data {
+	unsigned int ctl_reg;
+	unsigned int prd_reg;
+};
+
 struct sun4i_pwm_data {
 	bool has_prescaler_bypass;
 	bool has_rdy;
 	unsigned int npwm;
 	const u32 *prescaler_table;
+	const struct sunxi_pwmch_data *chan_data[SUN4I_MAX_PWM_CHANNELS];
 };
 
 struct sun4i_pwm_chip {
@@ -100,6 +106,18 @@ static inline struct sun4i_pwm_chip *to_sun4i_pwm_chip(struct pwm_chip *chip)
 	return container_of(chip, struct sun4i_pwm_chip, chip);
 }
 
+static inline unsigned int sunxi_pwm_ctl_reg(struct sun4i_pwm_chip *chip,
+					     int chan)
+{
+	return chip->data->chan_data[chan]->ctl_reg;
+}
+
+static inline unsigned int sunxi_pwm_prd_reg(struct sun4i_pwm_chip *chip,
+					     int chan)
+{
+	return chip->data->chan_data[chan]->prd_reg;
+}
+
 static int sun4i_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 			    int duty_ns, int period_ns)
 {
@@ -192,7 +210,8 @@ static int sun4i_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 	}
 
 	val = (dty & PWM_DTY_MASK) | PWM_PRD(prd);
-	err = regmap_write(sun4i_pwm->regmap, PWM_CH_PRD(pwm->hwpwm), val);
+	err = regmap_write(sun4i_pwm->regmap,
+			   sunxi_pwm_prd_reg(sun4i_pwm, pwm->hwpwm), val);
 	if (err) {
 		dev_err(chip->dev, "failed to write to PRD register\n");
 		goto err_cleanup;
@@ -256,7 +275,9 @@ static int sun4i_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
 	mask |= BIT_CH(PWM_CLK_GATING, pwm->hwpwm);
 
 	spin_lock(&sun4i_pwm->ctrl_lock);
-	ret = regmap_update_bits(sun4i_pwm->regmap, PWM_CTRL_REG, mask, mask);
+	ret = regmap_update_bits(sun4i_pwm->regmap,
+				 sunxi_pwm_ctl_reg(sun4i_pwm, pwm->hwpwm),
+				 mask, mask);
 	if (ret)
 		dev_err(chip->dev, "failed to write to CTL register\n");
 	spin_unlock(&sun4i_pwm->ctrl_lock);
@@ -276,7 +297,9 @@ static void sun4i_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
 	mask |= BIT_CH(PWM_CLK_GATING, pwm->hwpwm);
 
 	spin_lock(&sun4i_pwm->ctrl_lock);
-	err = regmap_update_bits(sun4i_pwm->regmap, PWM_CTRL_REG, mask, 0);
+	err = regmap_update_bits(sun4i_pwm->regmap,
+				 sunxi_pwm_ctl_reg(sun4i_pwm, pwm->hwpwm),
+				 mask, 0);
 	if (err)
 		dev_err(chip->dev, "failed to write to CTL register\n");
 	spin_unlock(&sun4i_pwm->ctrl_lock);
@@ -308,11 +331,25 @@ sun4i_pwm_regfields[SUN4I_MAX_PWM_CHANNELS][NUM_FIELDS] = {
 	},
 };
 
+static const struct sunxi_pwmch_data sun4i_pwm_chan0_data = {
+	.ctl_reg = PWM_CTRL_REG,
+	.prd_reg = PWM_CH_PRD(0),
+};
+
+static const struct sunxi_pwmch_data sun4i_pwm_chan1_data = {
+	.ctl_reg = PWM_CTRL_REG,
+	.prd_reg = PWM_CH_PRD(1),
+};
+
 static const struct sun4i_pwm_data sun4i_pwm_data_a10 = {
 	.has_prescaler_bypass = false,
 	.has_rdy = false,
 	.npwm = 2,
 	.prescaler_table = sun4i_prescaler_table,
+	.chan_data = {
+		&sun4i_pwm_chan0_data,
+		&sun4i_pwm_chan1_data,
+	}
 };
 
 static const struct sun4i_pwm_data sun4i_pwm_data_a10s = {
@@ -320,6 +357,10 @@ static const struct sun4i_pwm_data sun4i_pwm_data_a10s = {
 	.has_rdy = true,
 	.npwm = 2,
 	.prescaler_table = sun4i_prescaler_table,
+	.chan_data = {
+		&sun4i_pwm_chan0_data,
+		&sun4i_pwm_chan1_data,
+	}
 };
 
 static const struct sun4i_pwm_data sun4i_pwm_data_a13 = {
@@ -327,6 +368,9 @@ static const struct sun4i_pwm_data sun4i_pwm_data_a13 = {
 	.has_rdy = true,
 	.npwm = 1,
 	.prescaler_table = sun4i_prescaler_table,
+	.chan_data = {
+		&sun4i_pwm_chan0_data,
+	}
 };
 
 static const struct sun4i_pwm_data sun4i_pwm_data_a20 = {
@@ -334,6 +378,10 @@ static const struct sun4i_pwm_data sun4i_pwm_data_a20 = {
 	.has_rdy = true,
 	.npwm = 2,
 	.prescaler_table = sun4i_prescaler_table,
+	.chan_data = {
+		&sun4i_pwm_chan0_data,
+		&sun4i_pwm_chan1_data,
+	}
 };
 
 static const struct sun4i_pwm_data sun4i_pwm_data_h3 = {
@@ -341,6 +389,9 @@ static const struct sun4i_pwm_data sun4i_pwm_data_h3 = {
 	.has_rdy = true,
 	.npwm = 1,
 	.prescaler_table = sun4i_prescaler_table,
+	.chan_data = {
+		&sun4i_pwm_chan0_data,
+	}
 };
 
 static const struct of_device_id sun4i_pwm_dt_ids[] = {
-- 
2.4.11

Re: [PATCH v4 4/9] pwm: sunxi: Customizable control and period register position.

[Maxime Ripard]

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

On Fri, Feb 24, 2017 at 08:41:11AM +0300, lis8215-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org wrote:
> From: Siarhei Volkau <lis8215-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> 
> sun6i has same registers as sun4i compatible chips, but its position
> in register map are different.
> 
> This patch make register's position selectable for support sun6i in
> next patches.
> 
> Signed-off-by: Siarhei Volkau <lis8215-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>  drivers/pwm/pwm-sun4i.c | 57 ++++++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 54 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-sun4i.c b/drivers/pwm/pwm-sun4i.c
> index 418a625..9ddc812 100644
> --- a/drivers/pwm/pwm-sun4i.c
> +++ b/drivers/pwm/pwm-sun4i.c
> @@ -79,11 +79,17 @@ static const u32 sun4i_prescaler_table[] = {
>  	0, /* Actually 1 but tested separately */
>  };
>  
> +struct sunxi_pwmch_data {
> +	unsigned int ctl_reg;
> +	unsigned int prd_reg;
> +};
> +

Why don't you use regmap_fields for that too?

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Re: [PATCH v4 4/9] pwm: sunxi: Customizable control and period register position.

[Siarhei Volkau]

Hi,

2017-02-27 12:30 GMT+03:00 Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>:
> On Fri, Feb 24, 2017 at 08:41:11AM +0300, lis8215-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org wrote:
>> From: Siarhei Volkau <lis8215-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>>
>> sun6i has same registers as sun4i compatible chips, but its position
>> in register map are different.
>>
>> This patch make register's position selectable for support sun6i in
>> next patches.
>>
>> Signed-off-by: Siarhei Volkau <lis8215-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> ---
>>  drivers/pwm/pwm-sun4i.c | 57 ++++++++++++++++++++++++++++++++++++++++++++++---
>>  1 file changed, 54 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/pwm/pwm-sun4i.c b/drivers/pwm/pwm-sun4i.c
>> index 418a625..9ddc812 100644
>> --- a/drivers/pwm/pwm-sun4i.c
>> +++ b/drivers/pwm/pwm-sun4i.c
>> @@ -79,11 +79,17 @@ static const u32 sun4i_prescaler_table[] = {
>>       0, /* Actually 1 but tested separately */
>>  };
>>
>> +struct sunxi_pwmch_data {
>> +     unsigned int ctl_reg;
>> +     unsigned int prd_reg;
>> +};
>> +
>
> Why don't you use regmap_fields for that too?
>
> Maxime
>
> --
> Maxime Ripard, Free Electrons
> Embedded Linux and Kernel engineering
> http://free-electrons.com

Period register contains 2 fields (duty cycle and active period),
A20 user manual says:
(C) Note: the active cycles should be no larger than the period cycles.

I just try to avoid situation when active cycles can have bigger value
than duty cycle. Regmap fields not allows update 2
or more fields simultaneously, or maybe i'm not found this.

Same situation with enable/disable bitmask in control register.
Updating these bits (clock gating and enable) in different moments
in time can produce some artefacts on pwm output signal.

Siarhei

Re: [PATCH v4 4/9] pwm: sunxi: Customizable control and period register position.

[Maxime Ripard]

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

On Mon, Feb 27, 2017 at 03:35:04PM +0300, Siarhei Volkau wrote:
> Hi,
> 
> 2017-02-27 12:30 GMT+03:00 Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>:
> > On Fri, Feb 24, 2017 at 08:41:11AM +0300, lis8215-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org wrote:
> >> From: Siarhei Volkau <lis8215-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> >>
> >> sun6i has same registers as sun4i compatible chips, but its position
> >> in register map are different.
> >>
> >> This patch make register's position selectable for support sun6i in
> >> next patches.
> >>
> >> Signed-off-by: Siarhei Volkau <lis8215-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> >> ---
> >>  drivers/pwm/pwm-sun4i.c | 57 ++++++++++++++++++++++++++++++++++++++++++++++---
> >>  1 file changed, 54 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/drivers/pwm/pwm-sun4i.c b/drivers/pwm/pwm-sun4i.c
> >> index 418a625..9ddc812 100644
> >> --- a/drivers/pwm/pwm-sun4i.c
> >> +++ b/drivers/pwm/pwm-sun4i.c
> >> @@ -79,11 +79,17 @@ static const u32 sun4i_prescaler_table[] = {
> >>       0, /* Actually 1 but tested separately */
> >>  };
> >>
> >> +struct sunxi_pwmch_data {
> >> +     unsigned int ctl_reg;
> >> +     unsigned int prd_reg;
> >> +};
> >> +
> >
> > Why don't you use regmap_fields for that too?
> >
> > Maxime
> >
> > --
> > Maxime Ripard, Free Electrons
> > Embedded Linux and Kernel engineering
> > http://free-electrons.com
> 
> Period register contains 2 fields (duty cycle and active period),
> A20 user manual says:
> (C) Note: the active cycles should be no larger than the period cycles.
> 
> I just try to avoid situation when active cycles can have bigger value
> than duty cycle. Regmap fields not allows update 2
> or more fields simultaneously, or maybe i'm not found this.
> 
> Same situation with enable/disable bitmask in control register.
> Updating these bits (clock gating and enable) in different moments
> in time can produce some artefacts on pwm output signal.

That really should be dealt with in the driver itself though, and not
in your accessor. It's up to the driver to use the accessor in a way
that makes sense for that particular device.

All these constraints should be documented, but you don't need to
change the way you access those registers to do that (unless you
really really really have no other way to do so).

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[PATCH v4 5/9] pwm: sunxi: Customizable regmap fields and enable bit mask.

[lis8215-Re5JQEeQqe8AvxtiuMwx3w]

From: Siarhei Volkau <lis8215-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

sun6i has similar control registers bit map in comparison
to sun4i channel 0, but each channel has its own control
register.

This patch make:
 - regmap fields declarations selectable,
 - enable/disable bitmask selectable.
These things needed for support sun6i in next patches.

Signed-off-by: Siarhei Volkau <lis8215-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 drivers/pwm/pwm-sun4i.c | 43 +++++++++++++++++++++----------------------
 1 file changed, 21 insertions(+), 22 deletions(-)

diff --git a/drivers/pwm/pwm-sun4i.c b/drivers/pwm/pwm-sun4i.c
index 9ddc812..9463148 100644
--- a/drivers/pwm/pwm-sun4i.c
+++ b/drivers/pwm/pwm-sun4i.c
@@ -82,6 +82,8 @@ static const u32 sun4i_prescaler_table[] = {
 struct sunxi_pwmch_data {
 	unsigned int ctl_reg;
 	unsigned int prd_reg;
+	u32 enable_bits;
+	struct reg_field fields[NUM_FIELDS];
 };
 
 struct sun4i_pwm_data {
@@ -271,8 +273,7 @@ static int sun4i_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
 		return ret;
 	}
 
-	mask = BIT_CH(PWM_EN, pwm->hwpwm);
-	mask |= BIT_CH(PWM_CLK_GATING, pwm->hwpwm);
+	mask = sun4i_pwm->data->chan_data[pwm->hwpwm]->enable_bits;
 
 	spin_lock(&sun4i_pwm->ctrl_lock);
 	ret = regmap_update_bits(sun4i_pwm->regmap,
@@ -293,8 +294,7 @@ static void sun4i_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
 	u32 mask;
 	int err;
 
-	mask = BIT_CH(PWM_EN, pwm->hwpwm);
-	mask |= BIT_CH(PWM_CLK_GATING, pwm->hwpwm);
+	mask = sun4i_pwm->data->chan_data[pwm->hwpwm]->enable_bits;
 
 	spin_lock(&sun4i_pwm->ctrl_lock);
 	err = regmap_update_bits(sun4i_pwm->regmap,
@@ -315,30 +315,28 @@ static const struct pwm_ops sun4i_pwm_ops = {
 	.owner = THIS_MODULE,
 };
 
-static const struct reg_field
-sun4i_pwm_regfields[SUN4I_MAX_PWM_CHANNELS][NUM_FIELDS] = {
-	{
+static const struct sunxi_pwmch_data sun4i_pwm_chan0_data = {
+	.ctl_reg = PWM_CTRL_REG,
+	.prd_reg = PWM_CH_PRD(0),
+	.enable_bits = BIT_CH(PWM_EN | PWM_CLK_GATING, 0),
+	.fields = {
 		[FIELD_PRESCALER]  = REG_FIELD(PWM_CTRL_REG,  0,  3),
 		[FIELD_POLARITY]   = REG_FIELD(PWM_CTRL_REG,  5,  5),
 		[FIELD_CLK_GATING] = REG_FIELD(PWM_CTRL_REG,  6,  6),
 		[FIELD_READY]      = REG_FIELD(PWM_CTRL_REG, 28, 28),
 	},
-	{
-		[FIELD_PRESCALER]  = REG_FIELD(PWM_CTRL_REG, 15, 18),
-		[FIELD_POLARITY]   = REG_FIELD(PWM_CTRL_REG, 20, 20),
-		[FIELD_CLK_GATING] = REG_FIELD(PWM_CTRL_REG, 21, 21),
-		[FIELD_READY]      = REG_FIELD(PWM_CTRL_REG, 29, 29),
-	},
-};
-
-static const struct sunxi_pwmch_data sun4i_pwm_chan0_data = {
-	.ctl_reg = PWM_CTRL_REG,
-	.prd_reg = PWM_CH_PRD(0),
 };
 
 static const struct sunxi_pwmch_data sun4i_pwm_chan1_data = {
 	.ctl_reg = PWM_CTRL_REG,
 	.prd_reg = PWM_CH_PRD(1),
+	.enable_bits = BIT_CH(PWM_EN | PWM_CLK_GATING, 1),
+	.fields = {
+		[FIELD_PRESCALER]  = REG_FIELD(PWM_CTRL_REG, 15, 18),
+		[FIELD_POLARITY]   = REG_FIELD(PWM_CTRL_REG, 20, 20),
+		[FIELD_CLK_GATING] = REG_FIELD(PWM_CTRL_REG, 21, 21),
+		[FIELD_READY]      = REG_FIELD(PWM_CTRL_REG, 29, 29),
+	},
 };
 
 static const struct sun4i_pwm_data sun4i_pwm_data_a10 = {
@@ -422,17 +420,18 @@ static const struct regmap_config sunxi_pwm_regmap_config = {
 	.reg_stride = 4,
 };
 
-static int sun4i_alloc_reg_fields(struct device *dev,
+static int sunxi_alloc_reg_fields(struct device *dev,
 				  struct sun4i_pwm_chip *pwm, int chan)
 {
 	int i, err;
+	const struct sunxi_pwmch_data *data = pwm->data->chan_data[chan];
 
-	if (chan >= SUN4I_MAX_PWM_CHANNELS)
+	if (!data || chan >= SUN4I_MAX_PWM_CHANNELS)
 		return -EINVAL;
 	for (i = 0; i < NUM_FIELDS; i++) {
 		pwm->fields[chan][i] =
 			devm_regmap_field_alloc(dev, pwm->regmap,
-						sun4i_pwm_regfields[chan][i]);
+						data->fields[i]);
 		if (IS_ERR(pwm->fields[chan][i])) {
 			dev_err(dev, "regmap field allocation failed\n");
 			err = PTR_ERR(pwm->fields[chan][i]);
@@ -499,7 +498,7 @@ static int sun4i_pwm_probe(struct platform_device *pdev)
 	}
 
 	for (i = 0; i < pwm->chip.npwm; i++) {
-		ret = sun4i_alloc_reg_fields(&pdev->dev, pwm, i);
+		ret = sunxi_alloc_reg_fields(&pdev->dev, pwm, i);
 		if (ret)
 			goto read_error;
 	}
-- 
2.4.11

[PATCH v4 6/9] pwm: sunxi: Increase max number of pwm channels.

[lis8215-Re5JQEeQqe8AvxtiuMwx3w]

From: Siarhei Volkau <lis8215-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

sun6i have 4 pwm channels onboard. This patch increase
maximal possible count of channels.

Signed-off-by: Siarhei Volkau <lis8215-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 drivers/pwm/pwm-sun4i.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/pwm/pwm-sun4i.c b/drivers/pwm/pwm-sun4i.c
index 9463148..a179a53 100644
--- a/drivers/pwm/pwm-sun4i.c
+++ b/drivers/pwm/pwm-sun4i.c
@@ -46,7 +46,7 @@
 
 #define BIT_CH(bit, chan)	((bit) << ((chan) * PWMCH_OFFSET))
 
-#define SUN4I_MAX_PWM_CHANNELS	2
+#define SUNXI_MAX_PWM_CHANNELS	4
 
 /* regmap fields */
 enum {
@@ -91,14 +91,14 @@ struct sun4i_pwm_data {
 	bool has_rdy;
 	unsigned int npwm;
 	const u32 *prescaler_table;
-	const struct sunxi_pwmch_data *chan_data[SUN4I_MAX_PWM_CHANNELS];
+	const struct sunxi_pwmch_data *chan_data[SUNXI_MAX_PWM_CHANNELS];
 };
 
 struct sun4i_pwm_chip {
 	struct pwm_chip chip;
 	struct clk *clk;
 	struct regmap *regmap;
-	struct regmap_field *fields[SUN4I_MAX_PWM_CHANNELS][NUM_FIELDS];
+	struct regmap_field *fields[SUNXI_MAX_PWM_CHANNELS][NUM_FIELDS];
 	spinlock_t ctrl_lock;
 	const struct sun4i_pwm_data *data;
 };
@@ -426,7 +426,7 @@ static int sunxi_alloc_reg_fields(struct device *dev,
 	int i, err;
 	const struct sunxi_pwmch_data *data = pwm->data->chan_data[chan];
 
-	if (!data || chan >= SUN4I_MAX_PWM_CHANNELS)
+	if (!data || chan >= SUNXI_MAX_PWM_CHANNELS)
 		return -EINVAL;
 	for (i = 0; i < NUM_FIELDS; i++) {
 		pwm->fields[chan][i] =
-- 
2.4.11

[PATCH v4 7/9] pwm: sunxi: Add support the Allwinner A31 PWM.

[lis8215-Re5JQEeQqe8AvxtiuMwx3w]

From: Siarhei Volkau <lis8215-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

This patch introduce the sun6i PWM driver itself:
 - sun6i channels register and field map,
 - sun6i prescaler table,
 - DT bindings for A31 SoC,
 - documentation update.

Signed-off-by: Siarhei Volkau <lis8215-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 .../devicetree/bindings/pwm/pwm-sun4i.txt          |  3 +-
 drivers/pwm/pwm-sun4i.c                            | 95 +++++++++++++++++++++-
 2 files changed, 95 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/pwm/pwm-sun4i.txt b/Documentation/devicetree/bindings/pwm/pwm-sun4i.txt
index f1cbeef..109b997 100644
--- a/Documentation/devicetree/bindings/pwm/pwm-sun4i.txt
+++ b/Documentation/devicetree/bindings/pwm/pwm-sun4i.txt
@@ -1,10 +1,11 @@
-Allwinner sun4i and sun7i SoC PWM controller
+Allwinner sunxi SoC PWM controller
 
 Required properties:
   - compatible: should be one of:
     - "allwinner,sun4i-a10-pwm"
     - "allwinner,sun5i-a10s-pwm"
     - "allwinner,sun5i-a13-pwm"
+    - "allwinner,sun6i-a31-pwm"
     - "allwinner,sun7i-a20-pwm"
     - "allwinner,sun8i-h3-pwm"
   - reg: physical base address and length of the controller's registers
diff --git a/drivers/pwm/pwm-sun4i.c b/drivers/pwm/pwm-sun4i.c
index a179a53..e474303 100644
--- a/drivers/pwm/pwm-sun4i.c
+++ b/drivers/pwm/pwm-sun4i.c
@@ -1,7 +1,8 @@
 /*
- * Driver for Allwinner sun4i Pulse Width Modulation Controller
+ * Driver for Allwinner sunxi Pulse Width Modulation Controller
  *
  * Copyright (C) 2014 Alexandre Belloni <alexandre.belloni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
+ * Copyright (C) 2017 Siarhei Volkau <lis8215-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
  *
  * Licensed under GPLv2.
  */
@@ -48,6 +49,12 @@
 
 #define SUNXI_MAX_PWM_CHANNELS	4
 
+#define SUN6I_PWMCH_OFFS	0x10
+#define SUN6I_CH_CTL_OFFS	0x0
+#define SUN6I_CH_PRD_OFFS	0x4
+#define SUN6I_PWM_CTL_REG(ch)	(SUN6I_PWMCH_OFFS * (ch) + SUN6I_CH_CTL_OFFS)
+#define SUN6I_PWM_PRD_REG(ch)	(SUN6I_PWMCH_OFFS * (ch) + SUN6I_CH_PRD_OFFS)
+
 /* regmap fields */
 enum {
 	/* Used bit fields in control register */
@@ -79,6 +86,25 @@ static const u32 sun4i_prescaler_table[] = {
 	0, /* Actually 1 but tested separately */
 };
 
+static const u32 sun6i_prescaler_table[] = {
+	1,
+	2,
+	4,
+	8,
+	16,
+	32,
+	64,
+	0,
+	0,
+	0,
+	0,
+	0,
+	0,
+	0,
+	0,
+	0,
+};
+
 struct sunxi_pwmch_data {
 	unsigned int ctl_reg;
 	unsigned int prd_reg;
@@ -339,6 +365,54 @@ static const struct sunxi_pwmch_data sun4i_pwm_chan1_data = {
 	},
 };
 
+static const struct sunxi_pwmch_data sun6i_pwm_chan0_data = {
+	.ctl_reg = SUN6I_PWM_CTL_REG(0),
+	.prd_reg = SUN6I_PWM_PRD_REG(0),
+	.enable_bits = PWM_EN | PWM_CLK_GATING,
+	.fields = {
+		[FIELD_PRESCALER]  = REG_FIELD(SUN6I_PWM_CTL_REG(0),  0,  3),
+		[FIELD_POLARITY]   = REG_FIELD(SUN6I_PWM_CTL_REG(0),  5,  5),
+		[FIELD_CLK_GATING] = REG_FIELD(SUN6I_PWM_CTL_REG(0),  6,  6),
+		[FIELD_READY]      = REG_FIELD(SUN6I_PWM_CTL_REG(0), 28, 28),
+	},
+};
+
+static const struct sunxi_pwmch_data sun6i_pwm_chan1_data = {
+	.ctl_reg = SUN6I_PWM_CTL_REG(1),
+	.prd_reg = SUN6I_PWM_PRD_REG(1),
+	.enable_bits = PWM_EN | PWM_CLK_GATING,
+	.fields = {
+		[FIELD_PRESCALER]  = REG_FIELD(SUN6I_PWM_CTL_REG(1),  0,  3),
+		[FIELD_POLARITY]   = REG_FIELD(SUN6I_PWM_CTL_REG(1),  5,  5),
+		[FIELD_CLK_GATING] = REG_FIELD(SUN6I_PWM_CTL_REG(1),  6,  6),
+		[FIELD_READY]      = REG_FIELD(SUN6I_PWM_CTL_REG(1), 28, 28),
+	},
+};
+
+static const struct sunxi_pwmch_data sun6i_pwm_chan2_data = {
+	.ctl_reg = SUN6I_PWM_CTL_REG(2),
+	.prd_reg = SUN6I_PWM_PRD_REG(2),
+	.enable_bits = PWM_EN | PWM_CLK_GATING,
+	.fields = {
+		[FIELD_PRESCALER]  = REG_FIELD(SUN6I_PWM_CTL_REG(2),  0,  3),
+		[FIELD_POLARITY]   = REG_FIELD(SUN6I_PWM_CTL_REG(2),  5,  5),
+		[FIELD_CLK_GATING] = REG_FIELD(SUN6I_PWM_CTL_REG(2),  6,  6),
+		[FIELD_READY]      = REG_FIELD(SUN6I_PWM_CTL_REG(2), 28, 28),
+	},
+};
+
+static const struct sunxi_pwmch_data sun6i_pwm_chan3_data = {
+	.ctl_reg = SUN6I_PWM_CTL_REG(3),
+	.prd_reg = SUN6I_PWM_PRD_REG(3),
+	.enable_bits = PWM_EN | PWM_CLK_GATING,
+	.fields = {
+		[FIELD_PRESCALER]  = REG_FIELD(SUN6I_PWM_CTL_REG(3),  0,  3),
+		[FIELD_POLARITY]   = REG_FIELD(SUN6I_PWM_CTL_REG(3),  5,  5),
+		[FIELD_CLK_GATING] = REG_FIELD(SUN6I_PWM_CTL_REG(3),  6,  6),
+		[FIELD_READY]      = REG_FIELD(SUN6I_PWM_CTL_REG(3), 28, 28),
+	},
+};
+
 static const struct sun4i_pwm_data sun4i_pwm_data_a10 = {
 	.has_prescaler_bypass = false,
 	.has_rdy = false,
@@ -371,6 +445,19 @@ static const struct sun4i_pwm_data sun4i_pwm_data_a13 = {
 	}
 };
 
+static const struct sun4i_pwm_data sun6i_pwm_data_a31 = {
+	.has_prescaler_bypass = false,
+	.has_rdy = true,
+	.npwm = 4,
+	.prescaler_table = sun6i_prescaler_table,
+	.chan_data = {
+		&sun6i_pwm_chan0_data,
+		&sun6i_pwm_chan1_data,
+		&sun6i_pwm_chan2_data,
+		&sun6i_pwm_chan3_data,
+	}
+};
+
 static const struct sun4i_pwm_data sun4i_pwm_data_a20 = {
 	.has_prescaler_bypass = true,
 	.has_rdy = true,
@@ -403,6 +490,9 @@ static const struct of_device_id sun4i_pwm_dt_ids[] = {
 		.compatible = "allwinner,sun5i-a13-pwm",
 		.data = &sun4i_pwm_data_a13,
 	}, {
+		.compatible = "allwinner,sun6i-a31-pwm",
+		.data = &sun6i_pwm_data_a31,
+	}, {
 		.compatible = "allwinner,sun7i-a20-pwm",
 		.data = &sun4i_pwm_data_a20,
 	}, {
@@ -544,5 +634,6 @@ module_platform_driver(sun4i_pwm_driver);
 
 MODULE_ALIAS("platform:sun4i-pwm");
 MODULE_AUTHOR("Alexandre Belloni <alexandre.belloni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>");
-MODULE_DESCRIPTION("Allwinner sun4i PWM driver");
+MODULE_AUTHOR("Siarhei Volkau <lis8215-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>");
+MODULE_DESCRIPTION("Allwinner sunxi PWM driver");
 MODULE_LICENSE("GPL v2");
-- 
2.4.11

[PATCH v4 9/9] ARM: dts: sun6i: Add the PWM block to the A31/A31s.

[lis8215-Re5JQEeQqe8AvxtiuMwx3w]

From: Siarhei Volkau <lis8215-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

Signed-off-by: Siarhei Volkau <lis8215-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 arch/arm/boot/dts/sun6i-a31.dtsi | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/arch/arm/boot/dts/sun6i-a31.dtsi b/arch/arm/boot/dts/sun6i-a31.dtsi
index ee1eb6d..fcba129 100644
--- a/arch/arm/boot/dts/sun6i-a31.dtsi
+++ b/arch/arm/boot/dts/sun6i-a31.dtsi
@@ -627,6 +627,14 @@
 			status = "disabled";
 		};
 
+		pwm: pwm@01c21400 {
+			compatible = "allwinner,sun6i-a31-pwm";
+			reg = <0x01c21400 0x400>;
+			clocks = <&osc24M>;
+			#pwm-cells = <3>;
+			status = "disabled";
+		};
+
 		lradc: lradc@01c22800 {
 			compatible = "allwinner,sun4i-a10-lradc-keys";
 			reg = <0x01c22800 0x100>;
-- 
2.4.11

[PATCH v4 8/9] pwm: sunxi: Code cleanup.

[lis8215]

From: Siarhei Volkau <lis8215@gmail.com>

This patch removes macros, which are not use anymore and
fixes two extra -Wsign-compare warnings.

Signed-off-by: Siarhei Volkau <lis8215@gmail.com>
---
 drivers/pwm/pwm-sun4i.c | 12 ++----------
 1 file changed, 2 insertions(+), 10 deletions(-)

diff --git a/drivers/pwm/pwm-sun4i.c b/drivers/pwm/pwm-sun4i.c
index e474303..37fc746 100644
--- a/drivers/pwm/pwm-sun4i.c
+++ b/drivers/pwm/pwm-sun4i.c
@@ -28,17 +28,8 @@
 
 #define PWMCH_OFFSET		15
 #define PWM_PRESCAL_MASK	GENMASK(3, 0)
-#define PWM_PRESCAL_OFF		0
 #define PWM_EN			BIT(4)
-#define PWM_ACT_STATE		BIT(5)
 #define PWM_CLK_GATING		BIT(6)
-#define PWM_MODE		BIT(7)
-#define PWM_PULSE		BIT(8)
-#define PWM_BYPASS		BIT(9)
-
-#define PWM_RDY_BASE		28
-#define PWM_RDY_OFFSET		1
-#define PWM_RDY(ch)		BIT(PWM_RDY_BASE + PWM_RDY_OFFSET * (ch))
 
 #define PWM_PRD(prd)		(((prd) - 1) << 16)
 #define PWM_PRD_MASK		GENMASK(15, 0)
@@ -539,7 +530,8 @@ static int sun4i_pwm_probe(struct platform_device *pdev)
 	struct resource *res;
 	void __iomem *base;
 	u32 val;
-	int i, ret;
+	int ret;
+	unsigned int i;
 	const struct of_device_id *match;
 
 	match = of_match_device(sun4i_pwm_dt_ids, &pdev->dev);
-- 
2.4.11

Re: [PATCH v4 8/9] pwm: sunxi: Code cleanup.

[Maxime Ripard]

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

On Fri, Feb 24, 2017 at 08:41:15AM +0300, lis8215-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org wrote:
> From: Siarhei Volkau <lis8215-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> 
> This patch removes macros, which are not use anymore and
> fixes two extra -Wsign-compare warnings.
> 
> Signed-off-by: Siarhei Volkau <lis8215-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

You've been adding this code in your previous patches, why is that
even added there?

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Re: [PATCH v4 8/9] pwm: sunxi: Code cleanup.

[Siarhei Volkau]

Hi,

2017-02-27 12:32 GMT+03:00 Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>:
> On Fri, Feb 24, 2017 at 08:41:15AM +0300, lis8215-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org wrote:
>> From: Siarhei Volkau <lis8215-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>>
>> This patch removes macros, which are not use anymore and
>> fixes two extra -Wsign-compare warnings.
>>
>> Signed-off-by: Siarhei Volkau <lis8215-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>
> You've been adding this code in your previous patches, why is that
> even added there?
>
> Maxime
>
> --
> Maxime Ripard, Free Electrons
> Embedded Linux and Kernel engineering
> http://free-electrons.com

Are you mean there are no needs in separate patch for that?

Siarhei

Re: [PATCH v4 8/9] pwm: sunxi: Code cleanup.

[Maxime Ripard]

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

On Mon, Feb 27, 2017 at 04:21:49PM +0300, Siarhei Volkau wrote:
> Hi,
> 
> 2017-02-27 12:32 GMT+03:00 Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>:
> > On Fri, Feb 24, 2017 at 08:41:15AM +0300, lis8215-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org wrote:
> >> From: Siarhei Volkau <lis8215-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> >>
> >> This patch removes macros, which are not use anymore and
> >> fixes two extra -Wsign-compare warnings.
> >>
> >> Signed-off-by: Siarhei Volkau <lis8215-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> >
> > You've been adding this code in your previous patches, why is that
> > even added there?
> >
> > Maxime
> >
> > --
> > Maxime Ripard, Free Electrons
> > Embedded Linux and Kernel engineering
> > http://free-electrons.com
> 
> Are you mean there are no needs in separate patch for that?

Yes

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com