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

Submitted by lis8215@gmail.com on Feb. 24, 2017, 5:41 a.m.

Details

Message ID 1487914876-8594-5-git-send-email-lis8215@gmail.com
State New
Headers show

Commit Message

lis8215@gmail.com Feb. 24, 2017, 5:41 a.m.
From: Siarhei Volkau <lis8215@gmail.com>

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@gmail.com>
---
 drivers/pwm/pwm-sun4i.c | 57 ++++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 54 insertions(+), 3 deletions(-)

Comments

Maxime Ripard Feb. 27, 2017, 9:30 a.m.
On Fri, Feb 24, 2017 at 08:41:11AM +0300, lis8215@gmail.com wrote:
> From: Siarhei Volkau <lis8215@gmail.com>
> 
> 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@gmail.com>
> ---
>  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
lis8215@gmail.com Feb. 27, 2017, 12:35 p.m.
Hi,

2017-02-27 12:30 GMT+03:00 Maxime Ripard <maxime.ripard@free-electrons.com>:
> On Fri, Feb 24, 2017 at 08:41:11AM +0300, lis8215@gmail.com wrote:
>> From: Siarhei Volkau <lis8215@gmail.com>
>>
>> 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@gmail.com>
>> ---
>>  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
--
To unsubscribe from this list: send the line "unsubscribe linux-pwm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Maxime Ripard Feb. 28, 2017, 3:48 p.m.
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@free-electrons.com>:
> > On Fri, Feb 24, 2017 at 08:41:11AM +0300, lis8215@gmail.com wrote:
> >> From: Siarhei Volkau <lis8215@gmail.com>
> >>
> >> 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@gmail.com>
> >> ---
> >>  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

Patch hide | download patch | download mbox

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[] = {