diff mbox

[v2,1/4] ARM: pwm: sun4i: unification of register operations for support sun6i.

Message ID 1486489846-662-2-git-send-email-lis8215@gmail.com
State Superseded
Headers show

Commit Message

Siarhei Volkau Feb. 7, 2017, 5:50 p.m. UTC 
From: Siarhei Volkau <lis8215@gmail.com>

This patch not introduce new features, just prepare code for
adding sun6i PWM driver in next commits.

A31 SoC have a different map of PWM registers than others ASoCs,
but register bits purposes are very similar.

This patch introduce set of register access routines, which
are common for existing in driver ASoCs:
 - ctl_rdy   - checks the ready bit of specified PWM channel,
 - ctl_read  - reads value from control register of specified PWM channel,
 - ctl_write - writes significant bits to control register of specified PWM channel,
 - prd_read  - reads value from period register of specified PWM channel,
 - prd_write - writes value to period register of specified PWM channel.
Driver code redesigned to use the new routines.

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

Comments

Maxime Ripard Feb. 8, 2017, 7:52 a.m. UTC | #1 
Hi,

On Tue, Feb 07, 2017 at 08:50:43PM +0300, lis8215@gmail.com wrote:
> From: Siarhei Volkau <lis8215@gmail.com>
> 
> This patch not introduce new features, just prepare code for
> adding sun6i PWM driver in next commits.
> 
> A31 SoC have a different map of PWM registers than others ASoCs,
> but register bits purposes are very similar.
> 
> This patch introduce set of register access routines, which
> are common for existing in driver ASoCs:
>  - ctl_rdy   - checks the ready bit of specified PWM channel,
>  - ctl_read  - reads value from control register of specified PWM channel,
>  - ctl_write - writes significant bits to control register of specified PWM channel,
>  - prd_read  - reads value from period register of specified PWM channel,
>  - prd_write - writes value to period register of specified PWM channel.
> Driver code redesigned to use the new routines.

Why don't you use regmap for that?

Thanks,
Maxime
Maxime Ripard Feb. 10, 2017, 8:38 a.m. UTC | #2 
Hi,

On Wed, Feb 08, 2017 at 07:41:55PM +0300, Волков Сергей wrote:
> Hello all,
> 
> >> This patch not introduce new features, just prepare code for
> >> adding sun6i PWM driver in next commits.
> >>
> >> A31 SoC have a different map of PWM registers than others ASoCs,
> >> but register bits purposes are very similar.
> >>
> >> This patch introduce set of register access routines, which
> >> are common for existing in driver ASoCs:
> >>  - ctl_rdy   - checks the ready bit of specified PWM channel,
> >>  - ctl_read  - reads value from control register of specified PWM
> channel,
> >>  - ctl_write - writes significant bits to control register of specified
> PWM channel,
> >>  - prd_read  - reads value from period register of specified PWM channel,
> >>  - prd_write - writes value to period register of specified PWM channel.
> >> Driver code redesigned to use the new routines.
> >
> > Why don't you use regmap for that?
> 
> First of all, i'm newbie and its my first patchset. I just remake
> what i see in the driver. I even don't suspect about use regmap
> here.

Don't worry, a review is here to point where things could be better,
which doesn't mean that you did wrong :)

> Second thing - i can't test these driver on all existing sunxi SoCs,
> i have only A31 and A20 based boards where only pwm0 is
> accessible. Huge redesign cause bigger chance to make a mistake.

Don't worry about that either. We have a good number of platforms
using the PWMs already. If there's something broken at some point,
we'll catch it as well.

> If you think regmap solution is a must, then i will do that. This
> will be a good experience for me.

Yes, this really should cover what you're trying to do here,
especially reg_field.

You can have a look at drivers/reset/sti/reset-syscfg.c if you want to
have a look at a rather simple driver using it.

Maxime
diff mbox

Patch

diff --git a/drivers/pwm/pwm-sun4i.c b/drivers/pwm/pwm-sun4i.c
index b0803f6..04ad3b4 100644
--- a/drivers/pwm/pwm-sun4i.c
+++ b/drivers/pwm/pwm-sun4i.c
@@ -34,6 +34,7 @@ 
 #define PWM_MODE		BIT(7)
 #define PWM_PULSE		BIT(8)
 #define PWM_BYPASS		BIT(9)
+#define PWM_CHCTL_MASK		GENMASK(9, 0)
 
 #define PWM_RDY_BASE		28
 #define PWM_RDY_OFFSET		1
@@ -46,6 +47,8 @@ 
 
 #define BIT_CH(bit, chan)	((bit) << ((chan) * PWMCH_OFFSET))
 
+struct sun4i_pwm_chip;
+
 static const u32 prescaler_table[] = {
 	120,
 	180,
@@ -65,10 +68,19 @@  static const u32 prescaler_table[] = {
 	0, /* Actually 1 but tested separately */
 };
 
+struct sunxi_reg_ops {
+	int (*ctl_rdy)(struct sun4i_pwm_chip *chip, int npwm);
+	u32 (*ctl_read)(struct sun4i_pwm_chip *chip, int npwm);
+	void (*ctl_write)(struct sun4i_pwm_chip *chip, int npwm, u32 val);
+	u32 (*prd_read)(struct sun4i_pwm_chip *chip, int npwm);
+	void (*prd_write)(struct sun4i_pwm_chip *chip, int npwm, u32 val);
+};
+
 struct sun4i_pwm_data {
 	bool has_prescaler_bypass;
 	bool has_rdy;
 	unsigned int npwm;
+	const struct sunxi_reg_ops *ops;
 };
 
 struct sun4i_pwm_chip {
@@ -96,10 +108,42 @@  static inline void sun4i_pwm_writel(struct sun4i_pwm_chip *chip,
 	writel(val, chip->base + offset);
 }
 
+static int sun4i_reg_ctl_rdy(struct sun4i_pwm_chip *chip, int npwm)
+{
+	return PWM_RDY(npwm) & sun4i_pwm_readl(chip, PWM_CTRL_REG);
+}
+
+static u32 sun4i_reg_ctl_read(struct sun4i_pwm_chip *chip, int npwm)
+{
+	u32 val = sun4i_pwm_readl(chip, PWM_CTRL_REG);
+
+	return val >> (PWMCH_OFFSET * (npwm));
+}
+
+static void sun4i_reg_ctl_write(struct sun4i_pwm_chip *chip, int npwm, u32 val)
+{
+	u32 rd = sun4i_pwm_readl(chip, PWM_CTRL_REG);
+
+	rd &= ~(PWM_CHCTL_MASK << (PWMCH_OFFSET * npwm));
+	val &= (PWM_CHCTL_MASK << (PWMCH_OFFSET * npwm));
+	sun4i_pwm_writel(chip, rd | val, PWM_CTRL_REG);
+}
+
+static u32 sun4i_reg_prd_read(struct sun4i_pwm_chip *chip, int npwm)
+{
+	return sun4i_pwm_readl(chip, PWM_CH_PRD(npwm));
+}
+
+static void sun4i_reg_prd_write(struct sun4i_pwm_chip *chip, int npwm, u32 val)
+{
+	sun4i_pwm_writel(chip, val, PWM_CH_PRD(npwm));
+}
+
 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);
+	const struct sunxi_reg_ops *reg_ops = sun4i_pwm->data->ops;
 	u32 prd, dty, val, clk_gate;
 	u64 clk_rate, div = 0;
 	unsigned int prescaler = 0;
@@ -152,32 +196,31 @@  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);
 
-	if (sun4i_pwm->data->has_rdy && (val & PWM_RDY(pwm->hwpwm))) {
+	if (sun4i_pwm->data->has_rdy &&
+	    reg_ops->ctl_rdy(sun4i_pwm, 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);
+	val = reg_ops->ctl_read(sun4i_pwm, pwm->hwpwm);
+	clk_gate = val & PWM_CLK_GATING;
 	if (clk_gate) {
-		val &= ~BIT_CH(PWM_CLK_GATING, pwm->hwpwm);
-		sun4i_pwm_writel(sun4i_pwm, val, PWM_CTRL_REG);
+		val &= ~PWM_CLK_GATING;
+		reg_ops->ctl_write(sun4i_pwm, pwm->hwpwm, val);
 	}
 
-	val = sun4i_pwm_readl(sun4i_pwm, PWM_CTRL_REG);
 	val &= ~BIT_CH(PWM_PRESCAL_MASK, pwm->hwpwm);
 	val |= BIT_CH(prescaler, pwm->hwpwm);
-	sun4i_pwm_writel(sun4i_pwm, val, PWM_CTRL_REG);
+	reg_ops->ctl_write(sun4i_pwm, pwm->hwpwm, val);
 
-	val = (dty & PWM_DTY_MASK) | PWM_PRD(prd);
-	sun4i_pwm_writel(sun4i_pwm, val, PWM_CH_PRD(pwm->hwpwm));
+	reg_ops->prd_write(sun4i_pwm, pwm->hwpwm,
+			   (dty & PWM_DTY_MASK) | PWM_PRD(prd));
 
 	if (clk_gate) {
-		val = sun4i_pwm_readl(sun4i_pwm, PWM_CTRL_REG);
 		val |= clk_gate;
-		sun4i_pwm_writel(sun4i_pwm, val, PWM_CTRL_REG);
+		reg_ops->ctl_write(sun4i_pwm, pwm->hwpwm, val);
 	}
 
 	spin_unlock(&sun4i_pwm->ctrl_lock);
@@ -190,6 +233,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);
+	const struct sunxi_reg_ops *reg_ops = sun4i_pwm->data->ops;
 	u32 val;
 	int ret;
 
@@ -200,14 +244,14 @@  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);
+	val = reg_ops->ctl_read(sun4i_pwm, pwm->hwpwm);
 
 	if (polarity != PWM_POLARITY_NORMAL)
-		val &= ~BIT_CH(PWM_ACT_STATE, pwm->hwpwm);
+		val &= ~PWM_ACT_STATE;
 	else
-		val |= BIT_CH(PWM_ACT_STATE, pwm->hwpwm);
+		val |= PWM_ACT_STATE;
 
-	sun4i_pwm_writel(sun4i_pwm, val, PWM_CTRL_REG);
+	reg_ops->ctl_write(sun4i_pwm, pwm->hwpwm, val);
 
 	spin_unlock(&sun4i_pwm->ctrl_lock);
 	clk_disable_unprepare(sun4i_pwm->clk);
@@ -218,6 +262,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);
+	const struct sunxi_reg_ops *reg_ops = sun4i_pwm->data->ops;
 	u32 val;
 	int ret;
 
@@ -228,10 +273,9 @@  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);
-	val |= BIT_CH(PWM_EN, pwm->hwpwm);
-	val |= BIT_CH(PWM_CLK_GATING, pwm->hwpwm);
-	sun4i_pwm_writel(sun4i_pwm, val, PWM_CTRL_REG);
+	val = reg_ops->ctl_rdy(sun4i_pwm, pwm->hwpwm);
+	val |= PWM_EN | PWM_CLK_GATING;
+	reg_ops->ctl_write(sun4i_pwm, pwm->hwpwm, val);
 	spin_unlock(&sun4i_pwm->ctrl_lock);
 
 	return 0;
@@ -240,18 +284,26 @@  static int sun4i_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
 static void sun4i_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
 {
 	struct sun4i_pwm_chip *sun4i_pwm = to_sun4i_pwm_chip(chip);
+	const struct sunxi_reg_ops *reg_ops = sun4i_pwm->data->ops;
 	u32 val;
 
 	spin_lock(&sun4i_pwm->ctrl_lock);
-	val = sun4i_pwm_readl(sun4i_pwm, PWM_CTRL_REG);
-	val &= ~BIT_CH(PWM_EN, pwm->hwpwm);
-	val &= ~BIT_CH(PWM_CLK_GATING, pwm->hwpwm);
-	sun4i_pwm_writel(sun4i_pwm, val, PWM_CTRL_REG);
+	val = reg_ops->ctl_rdy(sun4i_pwm, pwm->hwpwm);
+	val &= ~(PWM_EN | PWM_CLK_GATING);
+	reg_ops->ctl_write(sun4i_pwm, pwm->hwpwm, val);
 	spin_unlock(&sun4i_pwm->ctrl_lock);
 
 	clk_disable_unprepare(sun4i_pwm->clk);
 }
 
+static const struct sunxi_reg_ops sun4i_reg_ops = {
+	.ctl_rdy   = sun4i_reg_ctl_rdy,
+	.ctl_read  = sun4i_reg_ctl_read,
+	.ctl_write = sun4i_reg_ctl_write,
+	.prd_read  = sun4i_reg_prd_read,
+	.prd_write = sun4i_reg_prd_write,
+};
+
 static const struct pwm_ops sun4i_pwm_ops = {
 	.config = sun4i_pwm_config,
 	.set_polarity = sun4i_pwm_set_polarity,
@@ -264,30 +316,35 @@  static const struct sun4i_pwm_data sun4i_pwm_data_a10 = {
 	.has_prescaler_bypass = false,
 	.has_rdy = false,
 	.npwm = 2,
+	.ops = &sun4i_reg_ops,
 };
 
 static const struct sun4i_pwm_data sun4i_pwm_data_a10s = {
 	.has_prescaler_bypass = true,
 	.has_rdy = true,
 	.npwm = 2,
+	.ops = &sun4i_reg_ops,
 };
 
 static const struct sun4i_pwm_data sun4i_pwm_data_a13 = {
 	.has_prescaler_bypass = true,
 	.has_rdy = true,
 	.npwm = 1,
+	.ops = &sun4i_reg_ops,
 };
 
 static const struct sun4i_pwm_data sun4i_pwm_data_a20 = {
 	.has_prescaler_bypass = true,
 	.has_rdy = true,
 	.npwm = 2,
+	.ops = &sun4i_reg_ops,
 };
 
 static const struct sun4i_pwm_data sun4i_pwm_data_h3 = {
 	.has_prescaler_bypass = true,
 	.has_rdy = true,
 	.npwm = 1,
+	.ops = &sun4i_reg_ops,
 };
 
 static const struct of_device_id sun4i_pwm_dt_ids[] = {
@@ -319,6 +376,7 @@  static int sun4i_pwm_probe(struct platform_device *pdev)
 	u32 val;
 	int i, ret;
 	const struct of_device_id *match;
+	const struct sunxi_reg_ops *reg_ops;
 
 	match = of_match_device(sun4i_pwm_dt_ids, &pdev->dev);
 
@@ -360,11 +418,14 @@  static int sun4i_pwm_probe(struct platform_device *pdev)
 		goto clk_error;
 	}
 
-	val = sun4i_pwm_readl(pwm, PWM_CTRL_REG);
-	for (i = 0; i < pwm->chip.npwm; i++)
-		if (!(val & BIT_CH(PWM_ACT_STATE, i)))
+	reg_ops = pwm->data->ops;
+
+	for (i = 0; i < pwm->chip.npwm; i++) {
+		val = reg_ops->ctl_read(pwm, i);
+		if (!(val & PWM_ACT_STATE))
 			pwm_set_polarity(&pwm->chip.pwms[i],
 					 PWM_POLARITY_INVERSED);
+	}
 	clk_disable_unprepare(pwm->clk);
 
 	return 0;