linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v3 0/2] switch to atomic PWM
@ 2017-03-22 13:29 Claudiu Beznea
  2017-03-22 13:29 ` [PATCH v3 1/2] drivers: pwm: pwm-atmel: " Claudiu Beznea
                   ` (2 more replies)
  0 siblings, 3 replies; 9+ messages in thread
From: Claudiu Beznea @ 2017-03-22 13:29 UTC (permalink / raw)
  To: thierry.reding, robh+dt, pawel.moll, mark.rutland,
	ijc+devicetree, galak, boris.brezillon, alexandre.belloni
  Cc: linux-pwm, devicetree, linux-kernel, linux-arm-kernel,
	nicolas.ferre, cristian.birsan, andrei.pistirica, tudor.ambarus,
	eugen.hristev, Claudiu Beznea

Changes in v3:
- since v2 introduced per-IP register layout there is no need
to keep update_cdty and set_cprd_cdty members in atmel_pwm_data
data structure used in v2; doing in this way the atmel_pwm_data
data structure will remain with only one member, regs. To avoid
another level of indirection, the atmel_pwm_data has been removed
and only atmel_pwm_registers data structure has been keept. In
this way, "data" member of atmel_pwm_chip data structure was
replaced by "regs" member; due to these changes prototype of
atmel_pwm_get_driver_data() function was also changed;
also, driver private data variables were renamed in the following
format "atmel_pwm_regs_v*"
- there is no need for the following checks at the begining
of atmel_pwm_apply() which were present in v2:

	if (cstate.enabled &&
	    (cstate.polarity != state->polarity ||
	     cstate.period != state->period))
		pwm_reset = true;

it is enough to add:

	if (state->enabled) {
		if (cstate.enabled &&
		    cstate.polarity == state->polarity &&
		    cstate.period == state->period) {

inside "if (state->enabled)" block and also to check cstate.enabled
instead of checking pwm_reset variable introduced in v2:

		if (cstate.enabled) {
			atmel_pwm_disable(chip, pwm, false);
		} else {
			ret = clk_enable(atmel_pwm->clk);
			if (ret) {
				dev_err(chip->dev, "failed to enable clock\n");
				return ret;
			}
		}

Changes in v2:
- update only duty factor without disabling the PWM channel
- if PWM channel is enabled, period, as signal polarity, is
updated by disabling + enabling the PWM channel
- atmel_pwm_config_prepare() function has been removed and
added instead two functions, one to compute the CPRD+Prescaler
(atmel_pwm_calculate_cprd_and_pres()), one to compute CRDY
(atmel_pwm_calculate_cdty())
- atmel_pwm_config_set() body was directly moved to atmel_pwm_apply()
- add 3 new members to atmel_pwm_data: update_cdty, set_cprd_cdty and
regs:
	- update_cdty is called to configure duty factor without
	disabling PWM channel, when necessary
	- set_cprd_cdty is called to configure both period and
	duty factor parameters
	- regs keeps the period and duty registers and was added to
	have common functions for update_cdty and set_cprd_cdty
	members of atmel_pwm_data for all boards;
- add a new parameter to atmel_pwm_disable(); this will be used in
updating period + signal polarity by disabling + enabling the
PWM channel. In this case, there is no need to disable PWM clock
since new configuration will be imediately applied.
- adapted the other reviewer comments excepts the one regarding
"cdty = cprd - cycles;" from atmel_pwm_calculate_cdty() since
in atmel_pwm_apply(), selecting polarity in the other way arround
than is currently done in this commit will need the changing of DPOLI
bit from Channel Mode Register, in order to keep the initial
output level of PWM channel after disable operation; this works
for sama5d2 but not for sam9rl which hasn't document the DPOLI
bit in datasheet; sama5d3 also hasn't document the DPOLI bit in
datasheet; one option was to have different aproach for different
boards but the code becomes messy.

Claudiu Beznea (2):
  drivers: pwm: pwm-atmel: switch to atomic PWM
  drivers: pwm: pwm-atmel: enable PWM on sama5d2

 .../devicetree/bindings/pwm/atmel-pwm.txt          |   1 +
 drivers/pwm/pwm-atmel.c                            | 276 ++++++++++-----------
 2 files changed, 133 insertions(+), 144 deletions(-)

-- 
2.7.4

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH v3 1/2] drivers: pwm: pwm-atmel: switch to atomic PWM
  2017-03-22 13:29 [PATCH v3 0/2] switch to atomic PWM Claudiu Beznea
@ 2017-03-22 13:29 ` Claudiu Beznea
  2017-03-27 12:57   ` Boris Brezillon
  2017-03-27 13:02   ` Boris Brezillon
  2017-03-22 13:29 ` [PATCH v3 2/2] drivers: pwm: pwm-atmel: enable PWM on sama5d2 Claudiu Beznea
  2017-04-06 14:33 ` [PATCH v3 0/2] switch to atomic PWM Thierry Reding
  2 siblings, 2 replies; 9+ messages in thread
From: Claudiu Beznea @ 2017-03-22 13:29 UTC (permalink / raw)
  To: thierry.reding, robh+dt, pawel.moll, mark.rutland,
	ijc+devicetree, galak, boris.brezillon, alexandre.belloni
  Cc: linux-pwm, devicetree, linux-kernel, linux-arm-kernel,
	nicolas.ferre, cristian.birsan, andrei.pistirica, tudor.ambarus,
	eugen.hristev, Claudiu Beznea

The currently Atmel PWM controllers supported by this driver
could change period or duty factor without channel disable,
for regular channels (sama5d3 support this by using period
or duty factor update registers, sam9rl support this by
writing channel update register and select the corresponding
update: period or duty factor). The chip doesn't support run
time changings of signal polarity. To take advantage of
atomic PWM framework and let controller works without glitches,
in this patch only the duty factor could be changed without
disabling PWM channel. For period and signal polarity the
atomic PWM is simulated by disabling + enabling the right PWM channel.

Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>

---
 drivers/pwm/pwm-atmel.c | 273 +++++++++++++++++++++++-------------------------
 1 file changed, 129 insertions(+), 144 deletions(-)

diff --git a/drivers/pwm/pwm-atmel.c b/drivers/pwm/pwm-atmel.c
index 67a7023..f147154 100644
--- a/drivers/pwm/pwm-atmel.c
+++ b/drivers/pwm/pwm-atmel.c
@@ -58,17 +58,22 @@
 #define PWM_MAX_PRD		0xFFFF
 #define PRD_MAX_PRES		10
 
+struct atmel_pwm_registers {
+	u8 period;
+	u8 period_upd;
+	u8 duty;
+	u8 duty_upd;
+};
+
 struct atmel_pwm_chip {
 	struct pwm_chip chip;
 	struct clk *clk;
 	void __iomem *base;
+	const struct atmel_pwm_registers *regs;
 
 	unsigned int updated_pwms;
 	/* ISR is cleared when read, ensure only one thread does that */
 	struct mutex isr_lock;
-
-	void (*config)(struct pwm_chip *chip, struct pwm_device *pwm,
-		       unsigned long dty, unsigned long prd);
 };
 
 static inline struct atmel_pwm_chip *to_atmel_pwm_chip(struct pwm_chip *chip)
@@ -105,153 +110,71 @@ static inline void atmel_pwm_ch_writel(struct atmel_pwm_chip *chip,
 	writel_relaxed(val, chip->base + base + offset);
 }
 
-static int atmel_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
-			    int duty_ns, int period_ns)
+static int atmel_pwm_calculate_cprd_and_pres(struct pwm_chip *chip,
+					     const struct pwm_state *state,
+					     unsigned long *cprd, u32 *pres)
 {
 	struct atmel_pwm_chip *atmel_pwm = to_atmel_pwm_chip(chip);
-	unsigned long prd, dty;
-	unsigned long long div;
-	unsigned int pres = 0;
-	u32 val;
-	int ret;
-
-	if (pwm_is_enabled(pwm) && (period_ns != pwm_get_period(pwm))) {
-		dev_err(chip->dev, "cannot change PWM period while enabled\n");
-		return -EBUSY;
-	}
+	unsigned long long cycles = state->period;
 
 	/* Calculate the period cycles and prescale value */
-	div = (unsigned long long)clk_get_rate(atmel_pwm->clk) * period_ns;
-	do_div(div, NSEC_PER_SEC);
+	cycles *= clk_get_rate(atmel_pwm->clk);
+	do_div(cycles, NSEC_PER_SEC);
 
-	while (div > PWM_MAX_PRD) {
-		div >>= 1;
-		pres++;
-	}
+	for (*pres = 0; cycles > PWM_MAX_PRD; cycles >>= 1)
+		(*pres)++;
 
-	if (pres > PRD_MAX_PRES) {
+	if (*pres > PRD_MAX_PRES) {
 		dev_err(chip->dev, "pres exceeds the maximum value\n");
 		return -EINVAL;
 	}
 
-	/* Calculate the duty cycles */
-	prd = div;
-	div *= duty_ns;
-	do_div(div, period_ns);
-	dty = prd - div;
-
-	ret = clk_enable(atmel_pwm->clk);
-	if (ret) {
-		dev_err(chip->dev, "failed to enable PWM clock\n");
-		return ret;
-	}
-
-	/* It is necessary to preserve CPOL, inside CMR */
-	val = atmel_pwm_ch_readl(atmel_pwm, pwm->hwpwm, PWM_CMR);
-	val = (val & ~PWM_CMR_CPRE_MSK) | (pres & PWM_CMR_CPRE_MSK);
-	atmel_pwm_ch_writel(atmel_pwm, pwm->hwpwm, PWM_CMR, val);
-	atmel_pwm->config(chip, pwm, dty, prd);
-	mutex_lock(&atmel_pwm->isr_lock);
-	atmel_pwm->updated_pwms |= atmel_pwm_readl(atmel_pwm, PWM_ISR);
-	atmel_pwm->updated_pwms &= ~(1 << pwm->hwpwm);
-	mutex_unlock(&atmel_pwm->isr_lock);
+	*cprd = cycles;
 
-	clk_disable(atmel_pwm->clk);
-	return ret;
+	return 0;
 }
 
-static void atmel_pwm_config_v1(struct pwm_chip *chip, struct pwm_device *pwm,
-				unsigned long dty, unsigned long prd)
+static void atmel_pwm_calculate_cdty(const struct pwm_state *state,
+				     unsigned long cprd, unsigned long *cdty)
 {
-	struct atmel_pwm_chip *atmel_pwm = to_atmel_pwm_chip(chip);
-	unsigned int val;
+	unsigned long long cycles = state->duty_cycle;
 
-
-	atmel_pwm_ch_writel(atmel_pwm, pwm->hwpwm, PWMV1_CUPD, dty);
-
-	val = atmel_pwm_ch_readl(atmel_pwm, pwm->hwpwm, PWM_CMR);
-	val &= ~PWM_CMR_UPD_CDTY;
-	atmel_pwm_ch_writel(atmel_pwm, pwm->hwpwm, PWM_CMR, val);
-
-	/*
-	 * If the PWM channel is enabled, only update CDTY by using the update
-	 * register, it needs to set bit 10 of CMR to 0
-	 */
-	if (pwm_is_enabled(pwm))
-		return;
-	/*
-	 * If the PWM channel is disabled, write value to duty and period
-	 * registers directly.
-	 */
-	atmel_pwm_ch_writel(atmel_pwm, pwm->hwpwm, PWMV1_CDTY, dty);
-	atmel_pwm_ch_writel(atmel_pwm, pwm->hwpwm, PWMV1_CPRD, prd);
+	cycles *= cprd;
+	do_div(cycles, state->period);
+	*cdty = cprd - cycles;
 }
 
-static void atmel_pwm_config_v2(struct pwm_chip *chip, struct pwm_device *pwm,
-				unsigned long dty, unsigned long prd)
-{
-	struct atmel_pwm_chip *atmel_pwm = to_atmel_pwm_chip(chip);
-
-	if (pwm_is_enabled(pwm)) {
-		/*
-		 * If the PWM channel is enabled, using the duty update register
-		 * to update the value.
-		 */
-		atmel_pwm_ch_writel(atmel_pwm, pwm->hwpwm, PWMV2_CDTYUPD, dty);
-	} else {
-		/*
-		 * If the PWM channel is disabled, write value to duty and
-		 * period registers directly.
-		 */
-		atmel_pwm_ch_writel(atmel_pwm, pwm->hwpwm, PWMV2_CDTY, dty);
-		atmel_pwm_ch_writel(atmel_pwm, pwm->hwpwm, PWMV2_CPRD, prd);
-	}
-}
-
-static int atmel_pwm_set_polarity(struct pwm_chip *chip, struct pwm_device *pwm,
-				  enum pwm_polarity polarity)
+static void atmel_pwm_update_cdty(struct pwm_chip *chip, struct pwm_device *pwm,
+				  unsigned long cdty)
 {
 	struct atmel_pwm_chip *atmel_pwm = to_atmel_pwm_chip(chip);
 	u32 val;
-	int ret;
-
-	val = atmel_pwm_ch_readl(atmel_pwm, pwm->hwpwm, PWM_CMR);
 
-	if (polarity == PWM_POLARITY_NORMAL)
-		val &= ~PWM_CMR_CPOL;
-	else
-		val |= PWM_CMR_CPOL;
-
-	ret = clk_enable(atmel_pwm->clk);
-	if (ret) {
-		dev_err(chip->dev, "failed to enable PWM clock\n");
-		return ret;
+	if (atmel_pwm->regs->duty_upd ==
+	    atmel_pwm->regs->period_upd) {
+		val = atmel_pwm_ch_readl(atmel_pwm, pwm->hwpwm, PWM_CMR);
+		val &= ~PWM_CMR_UPD_CDTY;
+		atmel_pwm_ch_writel(atmel_pwm, pwm->hwpwm, PWM_CMR, val);
 	}
 
-	atmel_pwm_ch_writel(atmel_pwm, pwm->hwpwm, PWM_CMR, val);
-
-	clk_disable(atmel_pwm->clk);
-
-	return 0;
+	atmel_pwm_ch_writel(atmel_pwm, pwm->hwpwm,
+			    atmel_pwm->regs->duty_upd, cdty);
 }
 
-static int atmel_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
+static void atmel_pwm_set_cprd_cdty(struct pwm_chip *chip,
+				    struct pwm_device *pwm,
+				    unsigned long cprd, unsigned long cdty)
 {
 	struct atmel_pwm_chip *atmel_pwm = to_atmel_pwm_chip(chip);
-	int ret;
-
-	ret = clk_enable(atmel_pwm->clk);
-	if (ret) {
-		dev_err(chip->dev, "failed to enable PWM clock\n");
-		return ret;
-	}
 
-	atmel_pwm_writel(atmel_pwm, PWM_ENA, 1 << pwm->hwpwm);
-
-	return 0;
+	atmel_pwm_ch_writel(atmel_pwm, pwm->hwpwm,
+			    atmel_pwm->regs->duty, cdty);
+	atmel_pwm_ch_writel(atmel_pwm, pwm->hwpwm,
+			    atmel_pwm->regs->period, cprd);
 }
 
-static void atmel_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
+static void atmel_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm,
+			      bool disable_clk)
 {
 	struct atmel_pwm_chip *atmel_pwm = to_atmel_pwm_chip(chip);
 	unsigned long timeout = jiffies + 2 * HZ;
@@ -282,37 +205,99 @@ static void atmel_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
 	       time_before(jiffies, timeout))
 		usleep_range(10, 100);
 
-	clk_disable(atmel_pwm->clk);
+	if (disable_clk)
+		clk_disable(atmel_pwm->clk);
+}
+
+static int atmel_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
+			   struct pwm_state *state)
+{
+	struct atmel_pwm_chip *atmel_pwm = to_atmel_pwm_chip(chip);
+	struct pwm_state cstate;
+	unsigned long cprd, cdty;
+	u32 pres, val;
+	int ret;
+
+	pwm_get_state(pwm, &cstate);
+
+	if (state->enabled) {
+		if (cstate.enabled &&
+		    cstate.polarity == state->polarity &&
+		    cstate.period == state->period) {
+			cprd = atmel_pwm_ch_readl(atmel_pwm, pwm->hwpwm,
+						  atmel_pwm->regs->period);
+			atmel_pwm_calculate_cdty(state, cprd, &cdty);
+			atmel_pwm_update_cdty(chip, pwm, cdty);
+			return 0;
+		}
+
+		ret = atmel_pwm_calculate_cprd_and_pres(chip, state, &cprd,
+							&pres);
+		if (ret) {
+			dev_err(chip->dev,
+				"failed to calculate cprd and prescaler\n");
+			return ret;
+		}
+
+		atmel_pwm_calculate_cdty(state, cprd, &cdty);
+
+		if (cstate.enabled) {
+			atmel_pwm_disable(chip, pwm, false);
+		} else {
+			ret = clk_enable(atmel_pwm->clk);
+			if (ret) {
+				dev_err(chip->dev, "failed to enable clock\n");
+				return ret;
+			}
+		}
+
+		/* It is necessary to preserve CPOL, inside CMR */
+		val = atmel_pwm_ch_readl(atmel_pwm, pwm->hwpwm, PWM_CMR);
+		val = (val & ~PWM_CMR_CPRE_MSK) | (pres & PWM_CMR_CPRE_MSK);
+		if (state->polarity == PWM_POLARITY_NORMAL)
+			val &= ~PWM_CMR_CPOL;
+		else
+			val |= PWM_CMR_CPOL;
+		atmel_pwm_ch_writel(atmel_pwm, pwm->hwpwm, PWM_CMR, val);
+		atmel_pwm_set_cprd_cdty(chip, pwm, cprd, cdty);
+		mutex_lock(&atmel_pwm->isr_lock);
+		atmel_pwm->updated_pwms |= atmel_pwm_readl(atmel_pwm, PWM_ISR);
+		atmel_pwm->updated_pwms &= ~(1 << pwm->hwpwm);
+		mutex_unlock(&atmel_pwm->isr_lock);
+		atmel_pwm_writel(atmel_pwm, PWM_ENA, 1 << pwm->hwpwm);
+	} else if (cstate.enabled) {
+		atmel_pwm_disable(chip, pwm, true);
+	}
+
+	return 0;
 }
 
 static const struct pwm_ops atmel_pwm_ops = {
-	.config = atmel_pwm_config,
-	.set_polarity = atmel_pwm_set_polarity,
-	.enable = atmel_pwm_enable,
-	.disable = atmel_pwm_disable,
+	.apply = atmel_pwm_apply,
 	.owner = THIS_MODULE,
 };
 
-struct atmel_pwm_data {
-	void (*config)(struct pwm_chip *chip, struct pwm_device *pwm,
-		       unsigned long dty, unsigned long prd);
-};
-
-static const struct atmel_pwm_data atmel_pwm_data_v1 = {
-	.config = atmel_pwm_config_v1,
+static const struct atmel_pwm_registers atmel_pwm_regs_v1 = {
+	.period		= PWMV1_CPRD,
+	.period_upd	= PWMV1_CUPD,
+	.duty		= PWMV1_CDTY,
+	.duty_upd	= PWMV1_CUPD,
 };
 
-static const struct atmel_pwm_data atmel_pwm_data_v2 = {
-	.config = atmel_pwm_config_v2,
+static const struct atmel_pwm_registers atmel_pwm_regs_v2 = {
+	.period		= PWMV2_CPRD,
+	.period_upd	= PWMV2_CPRDUPD,
+	.duty		= PWMV2_CDTY,
+	.duty_upd	= PWMV2_CDTYUPD,
 };
 
 static const struct platform_device_id atmel_pwm_devtypes[] = {
 	{
 		.name = "at91sam9rl-pwm",
-		.driver_data = (kernel_ulong_t)&atmel_pwm_data_v1,
+		.driver_data = (kernel_ulong_t)&atmel_pwm_regs_v1,
 	}, {
 		.name = "sama5d3-pwm",
-		.driver_data = (kernel_ulong_t)&atmel_pwm_data_v2,
+		.driver_data = (kernel_ulong_t)&atmel_pwm_regs_v2,
 	}, {
 		/* sentinel */
 	},
@@ -322,17 +307,17 @@ MODULE_DEVICE_TABLE(platform, atmel_pwm_devtypes);
 static const struct of_device_id atmel_pwm_dt_ids[] = {
 	{
 		.compatible = "atmel,at91sam9rl-pwm",
-		.data = &atmel_pwm_data_v1,
+		.data = &atmel_pwm_regs_v1,
 	}, {
 		.compatible = "atmel,sama5d3-pwm",
-		.data = &atmel_pwm_data_v2,
+		.data = &atmel_pwm_regs_v2,
 	}, {
 		/* sentinel */
 	},
 };
 MODULE_DEVICE_TABLE(of, atmel_pwm_dt_ids);
 
-static inline const struct atmel_pwm_data *
+static inline const struct atmel_pwm_registers *
 atmel_pwm_get_driver_data(struct platform_device *pdev)
 {
 	const struct platform_device_id *id;
@@ -342,18 +327,18 @@ atmel_pwm_get_driver_data(struct platform_device *pdev)
 
 	id = platform_get_device_id(pdev);
 
-	return (struct atmel_pwm_data *)id->driver_data;
+	return (struct atmel_pwm_registers *)id->driver_data;
 }
 
 static int atmel_pwm_probe(struct platform_device *pdev)
 {
-	const struct atmel_pwm_data *data;
+	const struct atmel_pwm_registers *regs;
 	struct atmel_pwm_chip *atmel_pwm;
 	struct resource *res;
 	int ret;
 
-	data = atmel_pwm_get_driver_data(pdev);
-	if (!data)
+	regs = atmel_pwm_get_driver_data(pdev);
+	if (!regs)
 		return -ENODEV;
 
 	atmel_pwm = devm_kzalloc(&pdev->dev, sizeof(*atmel_pwm), GFP_KERNEL);
@@ -385,7 +370,7 @@ static int atmel_pwm_probe(struct platform_device *pdev)
 
 	atmel_pwm->chip.base = -1;
 	atmel_pwm->chip.npwm = 4;
-	atmel_pwm->config = data->config;
+	atmel_pwm->regs = regs;
 	atmel_pwm->updated_pwms = 0;
 	mutex_init(&atmel_pwm->isr_lock);
 
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH v3 2/2] drivers: pwm: pwm-atmel: enable PWM on sama5d2
  2017-03-22 13:29 [PATCH v3 0/2] switch to atomic PWM Claudiu Beznea
  2017-03-22 13:29 ` [PATCH v3 1/2] drivers: pwm: pwm-atmel: " Claudiu Beznea
@ 2017-03-22 13:29 ` Claudiu Beznea
  2017-03-27 13:03   ` Boris Brezillon
  2017-04-06 14:33 ` [PATCH v3 0/2] switch to atomic PWM Thierry Reding
  2 siblings, 1 reply; 9+ messages in thread
From: Claudiu Beznea @ 2017-03-22 13:29 UTC (permalink / raw)
  To: thierry.reding, robh+dt, pawel.moll, mark.rutland,
	ijc+devicetree, galak, boris.brezillon, alexandre.belloni
  Cc: linux-pwm, devicetree, linux-kernel, linux-arm-kernel,
	nicolas.ferre, cristian.birsan, andrei.pistirica, tudor.ambarus,
	eugen.hristev, Claudiu Beznea

sama5d2 can use the same atmel_pwm_data as sama5d3.

Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>
Acked-by: Rob Herring <robh@kernel.org>

---
Changes since v2:
- the variable name used to initialize the .data member of
the of_device_id was renamed to atmel_pwm_regs_v2 to be
consistent with the approach from the first patch from
this series

 Documentation/devicetree/bindings/pwm/atmel-pwm.txt | 1 +
 drivers/pwm/pwm-atmel.c                             | 3 +++
 2 files changed, 4 insertions(+)

diff --git a/Documentation/devicetree/bindings/pwm/atmel-pwm.txt b/Documentation/devicetree/bindings/pwm/atmel-pwm.txt
index 02331b9..c8c831d 100644
--- a/Documentation/devicetree/bindings/pwm/atmel-pwm.txt
+++ b/Documentation/devicetree/bindings/pwm/atmel-pwm.txt
@@ -4,6 +4,7 @@ Required properties:
   - compatible: should be one of:
     - "atmel,at91sam9rl-pwm"
     - "atmel,sama5d3-pwm"
+    - "atmel,sama5d2-pwm"
   - reg: physical base address and length of the controller's registers
   - #pwm-cells: Should be 3. See pwm.txt in this directory for a
     description of the cells format.
diff --git a/drivers/pwm/pwm-atmel.c b/drivers/pwm/pwm-atmel.c
index f147154..530d7dc 100644
--- a/drivers/pwm/pwm-atmel.c
+++ b/drivers/pwm/pwm-atmel.c
@@ -312,6 +312,9 @@ static const struct of_device_id atmel_pwm_dt_ids[] = {
 		.compatible = "atmel,sama5d3-pwm",
 		.data = &atmel_pwm_regs_v2,
 	}, {
+		.compatible = "atmel,sama5d2-pwm",
+		.data = &atmel_pwm_regs_v2,
+	}, {
 		/* sentinel */
 	},
 };
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH v3 1/2] drivers: pwm: pwm-atmel: switch to atomic PWM
  2017-03-22 13:29 ` [PATCH v3 1/2] drivers: pwm: pwm-atmel: " Claudiu Beznea
@ 2017-03-27 12:57   ` Boris Brezillon
  2017-03-27 13:02   ` Boris Brezillon
  1 sibling, 0 replies; 9+ messages in thread
From: Boris Brezillon @ 2017-03-27 12:57 UTC (permalink / raw)
  To: Claudiu Beznea
  Cc: thierry.reding, robh+dt, pawel.moll, mark.rutland,
	ijc+devicetree, galak, alexandre.belloni, linux-pwm, devicetree,
	linux-kernel, linux-arm-kernel, nicolas.ferre, cristian.birsan,
	andrei.pistirica, tudor.ambarus, eugen.hristev

On Wed, 22 Mar 2017 15:29:34 +0200
Claudiu Beznea <claudiu.beznea@microchip.com> wrote:

> The currently Atmel PWM controllers supported by this driver
> could change period or duty factor without channel disable,
> for regular channels (sama5d3 support this by using period
> or duty factor update registers, sam9rl support this by
> writing channel update register and select the corresponding
> update: period or duty factor). The chip doesn't support run
> time changings of signal polarity. To take advantage of
> atomic PWM framework and let controller works without glitches,
> in this patch only the duty factor could be changed without
> disabling PWM channel. For period and signal polarity the
> atomic PWM is simulated by disabling + enabling the right PWM channel.
> 
> Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>

Reviewed-by: Boris Brezillon <boris.brezillon@free-electrons.com>

> 
> ---
>  drivers/pwm/pwm-atmel.c | 273 +++++++++++++++++++++++-------------------------
>  1 file changed, 129 insertions(+), 144 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-atmel.c b/drivers/pwm/pwm-atmel.c
> index 67a7023..f147154 100644
> --- a/drivers/pwm/pwm-atmel.c
> +++ b/drivers/pwm/pwm-atmel.c
> @@ -58,17 +58,22 @@
>  #define PWM_MAX_PRD		0xFFFF
>  #define PRD_MAX_PRES		10
>  
> +struct atmel_pwm_registers {
> +	u8 period;
> +	u8 period_upd;
> +	u8 duty;
> +	u8 duty_upd;
> +};
> +
>  struct atmel_pwm_chip {
>  	struct pwm_chip chip;
>  	struct clk *clk;
>  	void __iomem *base;
> +	const struct atmel_pwm_registers *regs;
>  
>  	unsigned int updated_pwms;
>  	/* ISR is cleared when read, ensure only one thread does that */
>  	struct mutex isr_lock;
> -
> -	void (*config)(struct pwm_chip *chip, struct pwm_device *pwm,
> -		       unsigned long dty, unsigned long prd);
>  };
>  
>  static inline struct atmel_pwm_chip *to_atmel_pwm_chip(struct pwm_chip *chip)
> @@ -105,153 +110,71 @@ static inline void atmel_pwm_ch_writel(struct atmel_pwm_chip *chip,
>  	writel_relaxed(val, chip->base + base + offset);
>  }
>  
> -static int atmel_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
> -			    int duty_ns, int period_ns)
> +static int atmel_pwm_calculate_cprd_and_pres(struct pwm_chip *chip,
> +					     const struct pwm_state *state,
> +					     unsigned long *cprd, u32 *pres)
>  {
>  	struct atmel_pwm_chip *atmel_pwm = to_atmel_pwm_chip(chip);
> -	unsigned long prd, dty;
> -	unsigned long long div;
> -	unsigned int pres = 0;
> -	u32 val;
> -	int ret;
> -
> -	if (pwm_is_enabled(pwm) && (period_ns != pwm_get_period(pwm))) {
> -		dev_err(chip->dev, "cannot change PWM period while enabled\n");
> -		return -EBUSY;
> -	}
> +	unsigned long long cycles = state->period;
>  
>  	/* Calculate the period cycles and prescale value */
> -	div = (unsigned long long)clk_get_rate(atmel_pwm->clk) * period_ns;
> -	do_div(div, NSEC_PER_SEC);
> +	cycles *= clk_get_rate(atmel_pwm->clk);
> +	do_div(cycles, NSEC_PER_SEC);
>  
> -	while (div > PWM_MAX_PRD) {
> -		div >>= 1;
> -		pres++;
> -	}
> +	for (*pres = 0; cycles > PWM_MAX_PRD; cycles >>= 1)
> +		(*pres)++;
>  
> -	if (pres > PRD_MAX_PRES) {
> +	if (*pres > PRD_MAX_PRES) {
>  		dev_err(chip->dev, "pres exceeds the maximum value\n");
>  		return -EINVAL;
>  	}
>  
> -	/* Calculate the duty cycles */
> -	prd = div;
> -	div *= duty_ns;
> -	do_div(div, period_ns);
> -	dty = prd - div;
> -
> -	ret = clk_enable(atmel_pwm->clk);
> -	if (ret) {
> -		dev_err(chip->dev, "failed to enable PWM clock\n");
> -		return ret;
> -	}
> -
> -	/* It is necessary to preserve CPOL, inside CMR */
> -	val = atmel_pwm_ch_readl(atmel_pwm, pwm->hwpwm, PWM_CMR);
> -	val = (val & ~PWM_CMR_CPRE_MSK) | (pres & PWM_CMR_CPRE_MSK);
> -	atmel_pwm_ch_writel(atmel_pwm, pwm->hwpwm, PWM_CMR, val);
> -	atmel_pwm->config(chip, pwm, dty, prd);
> -	mutex_lock(&atmel_pwm->isr_lock);
> -	atmel_pwm->updated_pwms |= atmel_pwm_readl(atmel_pwm, PWM_ISR);
> -	atmel_pwm->updated_pwms &= ~(1 << pwm->hwpwm);
> -	mutex_unlock(&atmel_pwm->isr_lock);
> +	*cprd = cycles;
>  
> -	clk_disable(atmel_pwm->clk);
> -	return ret;
> +	return 0;
>  }
>  
> -static void atmel_pwm_config_v1(struct pwm_chip *chip, struct pwm_device *pwm,
> -				unsigned long dty, unsigned long prd)
> +static void atmel_pwm_calculate_cdty(const struct pwm_state *state,
> +				     unsigned long cprd, unsigned long *cdty)
>  {
> -	struct atmel_pwm_chip *atmel_pwm = to_atmel_pwm_chip(chip);
> -	unsigned int val;
> +	unsigned long long cycles = state->duty_cycle;
>  
> -
> -	atmel_pwm_ch_writel(atmel_pwm, pwm->hwpwm, PWMV1_CUPD, dty);
> -
> -	val = atmel_pwm_ch_readl(atmel_pwm, pwm->hwpwm, PWM_CMR);
> -	val &= ~PWM_CMR_UPD_CDTY;
> -	atmel_pwm_ch_writel(atmel_pwm, pwm->hwpwm, PWM_CMR, val);
> -
> -	/*
> -	 * If the PWM channel is enabled, only update CDTY by using the update
> -	 * register, it needs to set bit 10 of CMR to 0
> -	 */
> -	if (pwm_is_enabled(pwm))
> -		return;
> -	/*
> -	 * If the PWM channel is disabled, write value to duty and period
> -	 * registers directly.
> -	 */
> -	atmel_pwm_ch_writel(atmel_pwm, pwm->hwpwm, PWMV1_CDTY, dty);
> -	atmel_pwm_ch_writel(atmel_pwm, pwm->hwpwm, PWMV1_CPRD, prd);
> +	cycles *= cprd;
> +	do_div(cycles, state->period);
> +	*cdty = cprd - cycles;
>  }
>  
> -static void atmel_pwm_config_v2(struct pwm_chip *chip, struct pwm_device *pwm,
> -				unsigned long dty, unsigned long prd)
> -{
> -	struct atmel_pwm_chip *atmel_pwm = to_atmel_pwm_chip(chip);
> -
> -	if (pwm_is_enabled(pwm)) {
> -		/*
> -		 * If the PWM channel is enabled, using the duty update register
> -		 * to update the value.
> -		 */
> -		atmel_pwm_ch_writel(atmel_pwm, pwm->hwpwm, PWMV2_CDTYUPD, dty);
> -	} else {
> -		/*
> -		 * If the PWM channel is disabled, write value to duty and
> -		 * period registers directly.
> -		 */
> -		atmel_pwm_ch_writel(atmel_pwm, pwm->hwpwm, PWMV2_CDTY, dty);
> -		atmel_pwm_ch_writel(atmel_pwm, pwm->hwpwm, PWMV2_CPRD, prd);
> -	}
> -}
> -
> -static int atmel_pwm_set_polarity(struct pwm_chip *chip, struct pwm_device *pwm,
> -				  enum pwm_polarity polarity)
> +static void atmel_pwm_update_cdty(struct pwm_chip *chip, struct pwm_device *pwm,
> +				  unsigned long cdty)
>  {
>  	struct atmel_pwm_chip *atmel_pwm = to_atmel_pwm_chip(chip);
>  	u32 val;
> -	int ret;
> -
> -	val = atmel_pwm_ch_readl(atmel_pwm, pwm->hwpwm, PWM_CMR);
>  
> -	if (polarity == PWM_POLARITY_NORMAL)
> -		val &= ~PWM_CMR_CPOL;
> -	else
> -		val |= PWM_CMR_CPOL;
> -
> -	ret = clk_enable(atmel_pwm->clk);
> -	if (ret) {
> -		dev_err(chip->dev, "failed to enable PWM clock\n");
> -		return ret;
> +	if (atmel_pwm->regs->duty_upd ==
> +	    atmel_pwm->regs->period_upd) {
> +		val = atmel_pwm_ch_readl(atmel_pwm, pwm->hwpwm, PWM_CMR);
> +		val &= ~PWM_CMR_UPD_CDTY;
> +		atmel_pwm_ch_writel(atmel_pwm, pwm->hwpwm, PWM_CMR, val);
>  	}
>  
> -	atmel_pwm_ch_writel(atmel_pwm, pwm->hwpwm, PWM_CMR, val);
> -
> -	clk_disable(atmel_pwm->clk);
> -
> -	return 0;
> +	atmel_pwm_ch_writel(atmel_pwm, pwm->hwpwm,
> +			    atmel_pwm->regs->duty_upd, cdty);
>  }
>  
> -static int atmel_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
> +static void atmel_pwm_set_cprd_cdty(struct pwm_chip *chip,
> +				    struct pwm_device *pwm,
> +				    unsigned long cprd, unsigned long cdty)
>  {
>  	struct atmel_pwm_chip *atmel_pwm = to_atmel_pwm_chip(chip);
> -	int ret;
> -
> -	ret = clk_enable(atmel_pwm->clk);
> -	if (ret) {
> -		dev_err(chip->dev, "failed to enable PWM clock\n");
> -		return ret;
> -	}
>  
> -	atmel_pwm_writel(atmel_pwm, PWM_ENA, 1 << pwm->hwpwm);
> -
> -	return 0;
> +	atmel_pwm_ch_writel(atmel_pwm, pwm->hwpwm,
> +			    atmel_pwm->regs->duty, cdty);
> +	atmel_pwm_ch_writel(atmel_pwm, pwm->hwpwm,
> +			    atmel_pwm->regs->period, cprd);
>  }
>  
> -static void atmel_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
> +static void atmel_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm,
> +			      bool disable_clk)
>  {
>  	struct atmel_pwm_chip *atmel_pwm = to_atmel_pwm_chip(chip);
>  	unsigned long timeout = jiffies + 2 * HZ;
> @@ -282,37 +205,99 @@ static void atmel_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
>  	       time_before(jiffies, timeout))
>  		usleep_range(10, 100);
>  
> -	clk_disable(atmel_pwm->clk);
> +	if (disable_clk)
> +		clk_disable(atmel_pwm->clk);
> +}
> +
> +static int atmel_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> +			   struct pwm_state *state)
> +{
> +	struct atmel_pwm_chip *atmel_pwm = to_atmel_pwm_chip(chip);
> +	struct pwm_state cstate;
> +	unsigned long cprd, cdty;
> +	u32 pres, val;
> +	int ret;
> +
> +	pwm_get_state(pwm, &cstate);
> +
> +	if (state->enabled) {
> +		if (cstate.enabled &&
> +		    cstate.polarity == state->polarity &&
> +		    cstate.period == state->period) {
> +			cprd = atmel_pwm_ch_readl(atmel_pwm, pwm->hwpwm,
> +						  atmel_pwm->regs->period);
> +			atmel_pwm_calculate_cdty(state, cprd, &cdty);
> +			atmel_pwm_update_cdty(chip, pwm, cdty);
> +			return 0;
> +		}
> +
> +		ret = atmel_pwm_calculate_cprd_and_pres(chip, state, &cprd,
> +							&pres);
> +		if (ret) {
> +			dev_err(chip->dev,
> +				"failed to calculate cprd and prescaler\n");
> +			return ret;
> +		}
> +
> +		atmel_pwm_calculate_cdty(state, cprd, &cdty);
> +
> +		if (cstate.enabled) {
> +			atmel_pwm_disable(chip, pwm, false);
> +		} else {
> +			ret = clk_enable(atmel_pwm->clk);
> +			if (ret) {
> +				dev_err(chip->dev, "failed to enable clock\n");
> +				return ret;
> +			}
> +		}
> +
> +		/* It is necessary to preserve CPOL, inside CMR */
> +		val = atmel_pwm_ch_readl(atmel_pwm, pwm->hwpwm, PWM_CMR);
> +		val = (val & ~PWM_CMR_CPRE_MSK) | (pres & PWM_CMR_CPRE_MSK);
> +		if (state->polarity == PWM_POLARITY_NORMAL)
> +			val &= ~PWM_CMR_CPOL;
> +		else
> +			val |= PWM_CMR_CPOL;
> +		atmel_pwm_ch_writel(atmel_pwm, pwm->hwpwm, PWM_CMR, val);
> +		atmel_pwm_set_cprd_cdty(chip, pwm, cprd, cdty);
> +		mutex_lock(&atmel_pwm->isr_lock);
> +		atmel_pwm->updated_pwms |= atmel_pwm_readl(atmel_pwm, PWM_ISR);
> +		atmel_pwm->updated_pwms &= ~(1 << pwm->hwpwm);
> +		mutex_unlock(&atmel_pwm->isr_lock);
> +		atmel_pwm_writel(atmel_pwm, PWM_ENA, 1 << pwm->hwpwm);
> +	} else if (cstate.enabled) {
> +		atmel_pwm_disable(chip, pwm, true);
> +	}
> +
> +	return 0;
>  }
>  
>  static const struct pwm_ops atmel_pwm_ops = {
> -	.config = atmel_pwm_config,
> -	.set_polarity = atmel_pwm_set_polarity,
> -	.enable = atmel_pwm_enable,
> -	.disable = atmel_pwm_disable,
> +	.apply = atmel_pwm_apply,
>  	.owner = THIS_MODULE,
>  };
>  
> -struct atmel_pwm_data {
> -	void (*config)(struct pwm_chip *chip, struct pwm_device *pwm,
> -		       unsigned long dty, unsigned long prd);
> -};
> -
> -static const struct atmel_pwm_data atmel_pwm_data_v1 = {
> -	.config = atmel_pwm_config_v1,
> +static const struct atmel_pwm_registers atmel_pwm_regs_v1 = {
> +	.period		= PWMV1_CPRD,
> +	.period_upd	= PWMV1_CUPD,
> +	.duty		= PWMV1_CDTY,
> +	.duty_upd	= PWMV1_CUPD,
>  };
>  
> -static const struct atmel_pwm_data atmel_pwm_data_v2 = {
> -	.config = atmel_pwm_config_v2,
> +static const struct atmel_pwm_registers atmel_pwm_regs_v2 = {
> +	.period		= PWMV2_CPRD,
> +	.period_upd	= PWMV2_CPRDUPD,
> +	.duty		= PWMV2_CDTY,
> +	.duty_upd	= PWMV2_CDTYUPD,
>  };
>  
>  static const struct platform_device_id atmel_pwm_devtypes[] = {
>  	{
>  		.name = "at91sam9rl-pwm",
> -		.driver_data = (kernel_ulong_t)&atmel_pwm_data_v1,
> +		.driver_data = (kernel_ulong_t)&atmel_pwm_regs_v1,
>  	}, {
>  		.name = "sama5d3-pwm",
> -		.driver_data = (kernel_ulong_t)&atmel_pwm_data_v2,
> +		.driver_data = (kernel_ulong_t)&atmel_pwm_regs_v2,
>  	}, {
>  		/* sentinel */
>  	},
> @@ -322,17 +307,17 @@ MODULE_DEVICE_TABLE(platform, atmel_pwm_devtypes);
>  static const struct of_device_id atmel_pwm_dt_ids[] = {
>  	{
>  		.compatible = "atmel,at91sam9rl-pwm",
> -		.data = &atmel_pwm_data_v1,
> +		.data = &atmel_pwm_regs_v1,
>  	}, {
>  		.compatible = "atmel,sama5d3-pwm",
> -		.data = &atmel_pwm_data_v2,
> +		.data = &atmel_pwm_regs_v2,
>  	}, {
>  		/* sentinel */
>  	},
>  };
>  MODULE_DEVICE_TABLE(of, atmel_pwm_dt_ids);
>  
> -static inline const struct atmel_pwm_data *
> +static inline const struct atmel_pwm_registers *
>  atmel_pwm_get_driver_data(struct platform_device *pdev)
>  {
>  	const struct platform_device_id *id;
> @@ -342,18 +327,18 @@ atmel_pwm_get_driver_data(struct platform_device *pdev)
>  
>  	id = platform_get_device_id(pdev);
>  
> -	return (struct atmel_pwm_data *)id->driver_data;
> +	return (struct atmel_pwm_registers *)id->driver_data;
>  }
>  
>  static int atmel_pwm_probe(struct platform_device *pdev)
>  {
> -	const struct atmel_pwm_data *data;
> +	const struct atmel_pwm_registers *regs;
>  	struct atmel_pwm_chip *atmel_pwm;
>  	struct resource *res;
>  	int ret;
>  
> -	data = atmel_pwm_get_driver_data(pdev);
> -	if (!data)
> +	regs = atmel_pwm_get_driver_data(pdev);
> +	if (!regs)
>  		return -ENODEV;
>  
>  	atmel_pwm = devm_kzalloc(&pdev->dev, sizeof(*atmel_pwm), GFP_KERNEL);
> @@ -385,7 +370,7 @@ static int atmel_pwm_probe(struct platform_device *pdev)
>  
>  	atmel_pwm->chip.base = -1;
>  	atmel_pwm->chip.npwm = 4;
> -	atmel_pwm->config = data->config;
> +	atmel_pwm->regs = regs;
>  	atmel_pwm->updated_pwms = 0;
>  	mutex_init(&atmel_pwm->isr_lock);
>  

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v3 1/2] drivers: pwm: pwm-atmel: switch to atomic PWM
  2017-03-22 13:29 ` [PATCH v3 1/2] drivers: pwm: pwm-atmel: " Claudiu Beznea
  2017-03-27 12:57   ` Boris Brezillon
@ 2017-03-27 13:02   ` Boris Brezillon
  2017-03-27 13:15     ` Alexandre Belloni
  1 sibling, 1 reply; 9+ messages in thread
From: Boris Brezillon @ 2017-03-27 13:02 UTC (permalink / raw)
  To: Claudiu Beznea
  Cc: thierry.reding, robh+dt, pawel.moll, mark.rutland,
	ijc+devicetree, galak, alexandre.belloni, linux-pwm, devicetree,
	linux-kernel, linux-arm-kernel, nicolas.ferre, cristian.birsan,
	andrei.pistirica, tudor.ambarus, eugen.hristev

Hi Claudiu,

On Wed, 22 Mar 2017 15:29:34 +0200
Claudiu Beznea <claudiu.beznea@microchip.com> wrote:

>  static const struct platform_device_id atmel_pwm_devtypes[] = {
>  	{
>  		.name = "at91sam9rl-pwm",
> -		.driver_data = (kernel_ulong_t)&atmel_pwm_data_v1,
> +		.driver_data = (kernel_ulong_t)&atmel_pwm_regs_v1,
>  	}, {
>  		.name = "sama5d3-pwm",
> -		.driver_data = (kernel_ulong_t)&atmel_pwm_data_v2,
> +		.driver_data = (kernel_ulong_t)&atmel_pwm_regs_v2,
>  	}, {
>  		/* sentinel */
>  	},

Unrelated to this series, but can you prepare a patch to get rid of
this platform id table (AT91 platforms have completely switched to DT
for quite some time now).

You can also get rid of the "if (pdev->dev.of_node)" condition in
atmel_pwm_probe() since it's guaranteed to be true, otherwise the
->probe() method wouldn't be called.

Thanks,

Boris

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v3 2/2] drivers: pwm: pwm-atmel: enable PWM on sama5d2
  2017-03-22 13:29 ` [PATCH v3 2/2] drivers: pwm: pwm-atmel: enable PWM on sama5d2 Claudiu Beznea
@ 2017-03-27 13:03   ` Boris Brezillon
  0 siblings, 0 replies; 9+ messages in thread
From: Boris Brezillon @ 2017-03-27 13:03 UTC (permalink / raw)
  To: Claudiu Beznea
  Cc: thierry.reding, robh+dt, pawel.moll, mark.rutland,
	ijc+devicetree, galak, alexandre.belloni, linux-pwm, devicetree,
	linux-kernel, linux-arm-kernel, nicolas.ferre, cristian.birsan,
	andrei.pistirica, tudor.ambarus, eugen.hristev

On Wed, 22 Mar 2017 15:29:35 +0200
Claudiu Beznea <claudiu.beznea@microchip.com> wrote:

> sama5d2 can use the same atmel_pwm_data as sama5d3.
> 
> Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>
> Acked-by: Rob Herring <robh@kernel.org>

Reviewed-by: Boris Brezillon <boris.brezillon@free-electrons.com>

> 
> ---
> Changes since v2:
> - the variable name used to initialize the .data member of
> the of_device_id was renamed to atmel_pwm_regs_v2 to be
> consistent with the approach from the first patch from
> this series
> 
>  Documentation/devicetree/bindings/pwm/atmel-pwm.txt | 1 +
>  drivers/pwm/pwm-atmel.c                             | 3 +++
>  2 files changed, 4 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/pwm/atmel-pwm.txt b/Documentation/devicetree/bindings/pwm/atmel-pwm.txt
> index 02331b9..c8c831d 100644
> --- a/Documentation/devicetree/bindings/pwm/atmel-pwm.txt
> +++ b/Documentation/devicetree/bindings/pwm/atmel-pwm.txt
> @@ -4,6 +4,7 @@ Required properties:
>    - compatible: should be one of:
>      - "atmel,at91sam9rl-pwm"
>      - "atmel,sama5d3-pwm"
> +    - "atmel,sama5d2-pwm"
>    - reg: physical base address and length of the controller's registers
>    - #pwm-cells: Should be 3. See pwm.txt in this directory for a
>      description of the cells format.
> diff --git a/drivers/pwm/pwm-atmel.c b/drivers/pwm/pwm-atmel.c
> index f147154..530d7dc 100644
> --- a/drivers/pwm/pwm-atmel.c
> +++ b/drivers/pwm/pwm-atmel.c
> @@ -312,6 +312,9 @@ static const struct of_device_id atmel_pwm_dt_ids[] = {
>  		.compatible = "atmel,sama5d3-pwm",
>  		.data = &atmel_pwm_regs_v2,
>  	}, {
> +		.compatible = "atmel,sama5d2-pwm",
> +		.data = &atmel_pwm_regs_v2,
> +	}, {
>  		/* sentinel */
>  	},
>  };

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v3 1/2] drivers: pwm: pwm-atmel: switch to atomic PWM
  2017-03-27 13:02   ` Boris Brezillon
@ 2017-03-27 13:15     ` Alexandre Belloni
  2017-03-27 14:05       ` m18063
  0 siblings, 1 reply; 9+ messages in thread
From: Alexandre Belloni @ 2017-03-27 13:15 UTC (permalink / raw)
  To: Boris Brezillon
  Cc: Claudiu Beznea, thierry.reding, robh+dt, pawel.moll,
	mark.rutland, ijc+devicetree, galak, linux-pwm, devicetree,
	linux-kernel, linux-arm-kernel, nicolas.ferre, cristian.birsan,
	andrei.pistirica, tudor.ambarus, eugen.hristev

On 27/03/2017 at 15:02:37 +0200, Boris Brezillon wrote:
> Hi Claudiu,
> 
> On Wed, 22 Mar 2017 15:29:34 +0200
> Claudiu Beznea <claudiu.beznea@microchip.com> wrote:
> 
> >  static const struct platform_device_id atmel_pwm_devtypes[] = {
> >  	{
> >  		.name = "at91sam9rl-pwm",
> > -		.driver_data = (kernel_ulong_t)&atmel_pwm_data_v1,
> > +		.driver_data = (kernel_ulong_t)&atmel_pwm_regs_v1,
> >  	}, {
> >  		.name = "sama5d3-pwm",
> > -		.driver_data = (kernel_ulong_t)&atmel_pwm_data_v2,
> > +		.driver_data = (kernel_ulong_t)&atmel_pwm_regs_v2,
> >  	}, {
> >  		/* sentinel */
> >  	},
> 
> Unrelated to this series, but can you prepare a patch to get rid of
> this platform id table (AT91 platforms have completely switched to DT
> for quite some time now).
> 

Please, don't until AVR32 is gone.

> You can also get rid of the "if (pdev->dev.of_node)" condition in
> atmel_pwm_probe() since it's guaranteed to be true, otherwise the
> ->probe() method wouldn't be called.
> 
> Thanks,
> 
> Boris

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v3 1/2] drivers: pwm: pwm-atmel: switch to atomic PWM
  2017-03-27 13:15     ` Alexandre Belloni
@ 2017-03-27 14:05       ` m18063
  0 siblings, 0 replies; 9+ messages in thread
From: m18063 @ 2017-03-27 14:05 UTC (permalink / raw)
  To: Alexandre Belloni, Boris Brezillon
  Cc: thierry.reding, robh+dt, pawel.moll, mark.rutland,
	ijc+devicetree, galak, linux-pwm, devicetree, linux-kernel,
	linux-arm-kernel, nicolas.ferre, cristian.birsan,
	andrei.pistirica, tudor.ambarus, eugen.hristev


On 27.03.2017 16:15, Alexandre Belloni wrote:
> On 27/03/2017 at 15:02:37 +0200, Boris Brezillon wrote:
>> Hi Claudiu,
>>
>> On Wed, 22 Mar 2017 15:29:34 +0200
>> Claudiu Beznea <claudiu.beznea@microchip.com> wrote:
>>
>>>  static const struct platform_device_id atmel_pwm_devtypes[] = {
>>>  	{
>>>  		.name = "at91sam9rl-pwm",
>>> -		.driver_data = (kernel_ulong_t)&atmel_pwm_data_v1,
>>> +		.driver_data = (kernel_ulong_t)&atmel_pwm_regs_v1,
>>>  	}, {
>>>  		.name = "sama5d3-pwm",
>>> -		.driver_data = (kernel_ulong_t)&atmel_pwm_data_v2,
>>> +		.driver_data = (kernel_ulong_t)&atmel_pwm_regs_v2,
>>>  	}, {
>>>  		/* sentinel */
>>>  	},
>> Unrelated to this series, but can you prepare a patch to get rid of
>> this platform id table (AT91 platforms have completely switched to DT
>> for quite some time now).
>>
> Please, don't until AVR32 is gone.

Sure, I will wait for it.

>
>> You can also get rid of the "if (pdev->dev.of_node)" condition in
>> atmel_pwm_probe() since it's guaranteed to be true, otherwise the
>> ->probe() method wouldn't be called.
>>
>> Thanks,
>>
>> Boris

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v3 0/2] switch to atomic PWM
  2017-03-22 13:29 [PATCH v3 0/2] switch to atomic PWM Claudiu Beznea
  2017-03-22 13:29 ` [PATCH v3 1/2] drivers: pwm: pwm-atmel: " Claudiu Beznea
  2017-03-22 13:29 ` [PATCH v3 2/2] drivers: pwm: pwm-atmel: enable PWM on sama5d2 Claudiu Beznea
@ 2017-04-06 14:33 ` Thierry Reding
  2 siblings, 0 replies; 9+ messages in thread
From: Thierry Reding @ 2017-04-06 14:33 UTC (permalink / raw)
  To: Claudiu Beznea
  Cc: robh+dt, pawel.moll, mark.rutland, ijc+devicetree, galak,
	boris.brezillon, alexandre.belloni, linux-pwm, devicetree,
	linux-kernel, linux-arm-kernel, nicolas.ferre, cristian.birsan,
	andrei.pistirica, tudor.ambarus, eugen.hristev

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

On Wed, Mar 22, 2017 at 03:29:33PM +0200, Claudiu Beznea wrote:
> Changes in v3:
> - since v2 introduced per-IP register layout there is no need
> to keep update_cdty and set_cprd_cdty members in atmel_pwm_data
> data structure used in v2; doing in this way the atmel_pwm_data
> data structure will remain with only one member, regs. To avoid
> another level of indirection, the atmel_pwm_data has been removed
> and only atmel_pwm_registers data structure has been keept. In
> this way, "data" member of atmel_pwm_chip data structure was
> replaced by "regs" member; due to these changes prototype of
> atmel_pwm_get_driver_data() function was also changed;
> also, driver private data variables were renamed in the following
> format "atmel_pwm_regs_v*"
> - there is no need for the following checks at the begining
> of atmel_pwm_apply() which were present in v2:
> 
> 	if (cstate.enabled &&
> 	    (cstate.polarity != state->polarity ||
> 	     cstate.period != state->period))
> 		pwm_reset = true;
> 
> it is enough to add:
> 
> 	if (state->enabled) {
> 		if (cstate.enabled &&
> 		    cstate.polarity == state->polarity &&
> 		    cstate.period == state->period) {
> 
> inside "if (state->enabled)" block and also to check cstate.enabled
> instead of checking pwm_reset variable introduced in v2:
> 
> 		if (cstate.enabled) {
> 			atmel_pwm_disable(chip, pwm, false);
> 		} else {
> 			ret = clk_enable(atmel_pwm->clk);
> 			if (ret) {
> 				dev_err(chip->dev, "failed to enable clock\n");
> 				return ret;
> 			}
> 		}
> 
> Changes in v2:
> - update only duty factor without disabling the PWM channel
> - if PWM channel is enabled, period, as signal polarity, is
> updated by disabling + enabling the PWM channel
> - atmel_pwm_config_prepare() function has been removed and
> added instead two functions, one to compute the CPRD+Prescaler
> (atmel_pwm_calculate_cprd_and_pres()), one to compute CRDY
> (atmel_pwm_calculate_cdty())
> - atmel_pwm_config_set() body was directly moved to atmel_pwm_apply()
> - add 3 new members to atmel_pwm_data: update_cdty, set_cprd_cdty and
> regs:
> 	- update_cdty is called to configure duty factor without
> 	disabling PWM channel, when necessary
> 	- set_cprd_cdty is called to configure both period and
> 	duty factor parameters
> 	- regs keeps the period and duty registers and was added to
> 	have common functions for update_cdty and set_cprd_cdty
> 	members of atmel_pwm_data for all boards;
> - add a new parameter to atmel_pwm_disable(); this will be used in
> updating period + signal polarity by disabling + enabling the
> PWM channel. In this case, there is no need to disable PWM clock
> since new configuration will be imediately applied.
> - adapted the other reviewer comments excepts the one regarding
> "cdty = cprd - cycles;" from atmel_pwm_calculate_cdty() since
> in atmel_pwm_apply(), selecting polarity in the other way arround
> than is currently done in this commit will need the changing of DPOLI
> bit from Channel Mode Register, in order to keep the initial
> output level of PWM channel after disable operation; this works
> for sama5d2 but not for sam9rl which hasn't document the DPOLI
> bit in datasheet; sama5d3 also hasn't document the DPOLI bit in
> datasheet; one option was to have different aproach for different
> boards but the code becomes messy.
> 
> Claudiu Beznea (2):
>   drivers: pwm: pwm-atmel: switch to atomic PWM
>   drivers: pwm: pwm-atmel: enable PWM on sama5d2
> 
>  .../devicetree/bindings/pwm/atmel-pwm.txt          |   1 +
>  drivers/pwm/pwm-atmel.c                            | 276 ++++++++++-----------
>  2 files changed, 133 insertions(+), 144 deletions(-)

Applied, thanks.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2017-04-06 14:33 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-03-22 13:29 [PATCH v3 0/2] switch to atomic PWM Claudiu Beznea
2017-03-22 13:29 ` [PATCH v3 1/2] drivers: pwm: pwm-atmel: " Claudiu Beznea
2017-03-27 12:57   ` Boris Brezillon
2017-03-27 13:02   ` Boris Brezillon
2017-03-27 13:15     ` Alexandre Belloni
2017-03-27 14:05       ` m18063
2017-03-22 13:29 ` [PATCH v3 2/2] drivers: pwm: pwm-atmel: enable PWM on sama5d2 Claudiu Beznea
2017-03-27 13:03   ` Boris Brezillon
2017-04-06 14:33 ` [PATCH v3 0/2] switch to atomic PWM Thierry Reding

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).