[PATCH 0/3] omap: dmtimer: Fix and cleanup moved driver

[PATCH 0/3] omap: dmtimer: Fix and cleanup moved driver

[Ladislav Michl]

This series is build on top od Keerthy's 'omap: dmtimer: Move driver
out of plat-omap' v7.

Ladislav Michl (3):
  clocksource: timer-dm: Check prescaler value
  pwm: pwm-omap-dmtimer: Fix frequency when using prescaler
  clocksource: timer-dm: Make unexported functions static

 drivers/clocksource/timer-dm.c |  222 ++++++++++++++++++++---------------------
 drivers/pwm/pwm-omap-dmtimer.c |   92 +++++++++-------
 include/clocksource/dmtimer.h  |   24 ----
 3 files changed, 164 insertions(+), 174 deletions(-)

[PATCH 1/3] clocksource: timer-dm: Check prescaler value

[Ladislav Michl]

Invalid value silently disables use of the prescaler.
Use -1 explicitely for that purpose and error out on
invalid value.

Signed-off-by: Ladislav Michl <ladis@linux-mips.org>
---
 drivers/clocksource/timer-dm.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/clocksource/timer-dm.c b/drivers/clocksource/timer-dm.c
index 60db1734ea3b..324ec93d3dd2 100644
--- a/drivers/clocksource/timer-dm.c
+++ b/drivers/clocksource/timer-dm.c
@@ -663,13 +663,13 @@ int omap_dm_timer_set_prescaler(struct omap_dm_timer *timer, int prescaler)
 {
 	u32 l;
 
-	if (unlikely(!timer))
+	if (unlikely(!timer) || prescaler < -1 || prescaler > 7)
 		return -EINVAL;
 
 	omap_dm_timer_enable(timer);
 	l = omap_dm_timer_read_reg(timer, OMAP_TIMER_CTRL_REG);
 	l &= ~(OMAP_TIMER_CTRL_PRE | (0x07 << 2));
-	if (prescaler >= 0x00 && prescaler <= 0x07) {
+	if (prescaler >= 0) {
 		l |= OMAP_TIMER_CTRL_PRE;
 		l |= prescaler << 2;
 	}

[PATCH 2/3] pwm: pwm-omap-dmtimer: Fix frequency when using prescaler

[Ladislav Michl]

Prescaler setting is currently not taken into account.
Fix that by introducing freq member variable and initialize
it at device probe time. This also avoids frequency
recomputing at each pwm configure time.

Signed-off-by: Ladislav Michl <ladis@linux-mips.org>
---
 drivers/pwm/pwm-omap-dmtimer.c |   92 +++++++++++++++++++++++----------------
 1 file changed, 53 insertions(+), 39 deletions(-)

diff --git a/drivers/pwm/pwm-omap-dmtimer.c b/drivers/pwm/pwm-omap-dmtimer.c
index cc485d9946f3..81c79e41a167 100644
--- a/drivers/pwm/pwm-omap-dmtimer.c
+++ b/drivers/pwm/pwm-omap-dmtimer.c
@@ -40,6 +40,7 @@ struct pwm_omap_dmtimer_chip {
 	pwm_omap_dmtimer *dm_timer;
 	struct omap_dm_timer_ops *pdata;
 	struct platform_device *dm_timer_pdev;
+	unsigned long freq;
 };
 
 static inline struct pwm_omap_dmtimer_chip *
@@ -48,9 +49,10 @@ to_pwm_omap_dmtimer_chip(struct pwm_chip *chip)
 	return container_of(chip, struct pwm_omap_dmtimer_chip, chip);
 }
 
-static u32 pwm_omap_dmtimer_get_clock_cycles(unsigned long clk_rate, int ns)
+static inline u32
+pwm_omap_dmtimer_get_clock_cycles(struct pwm_omap_dmtimer_chip *omap, int ns)
 {
-	return DIV_ROUND_CLOSEST_ULL((u64)clk_rate * ns, NSEC_PER_SEC);
+	return DIV_ROUND_CLOSEST_ULL((u64)omap->freq * ns, NSEC_PER_SEC);
 }
 
 static void pwm_omap_dmtimer_start(struct pwm_omap_dmtimer_chip *omap)
@@ -99,8 +101,6 @@ static int pwm_omap_dmtimer_config(struct pwm_chip *chip,
 	struct pwm_omap_dmtimer_chip *omap = to_pwm_omap_dmtimer_chip(chip);
 	u32 period_cycles, duty_cycles;
 	u32 load_value, match_value;
-	struct clk *fclk;
-	unsigned long clk_rate;
 	bool timer_active;
 
 	dev_dbg(chip->dev, "requested duty cycle: %d ns, period: %d ns\n",
@@ -114,19 +114,6 @@ static int pwm_omap_dmtimer_config(struct pwm_chip *chip,
 		return 0;
 	}
 
-	fclk = omap->pdata->get_fclk(omap->dm_timer);
-	if (!fclk) {
-		dev_err(chip->dev, "invalid pmtimer fclk\n");
-		goto err_einval;
-	}
-
-	clk_rate = clk_get_rate(fclk);
-	if (!clk_rate) {
-		dev_err(chip->dev, "invalid pmtimer fclk rate\n");
-		goto err_einval;
-	}
-
-	dev_dbg(chip->dev, "clk rate: %luHz\n", clk_rate);
 
 	/*
 	 * Calculate the appropriate load and match values based on the
@@ -144,35 +131,35 @@ static int pwm_omap_dmtimer_config(struct pwm_chip *chip,
 	 *   OMAP4430/60/70 TRM sections 22.2.4.10 and 22.2.4.11
 	 *   AM335x Sitara TRM sections 20.1.3.5 and 20.1.3.6
 	 */
-	period_cycles = pwm_omap_dmtimer_get_clock_cycles(clk_rate, period_ns);
-	duty_cycles = pwm_omap_dmtimer_get_clock_cycles(clk_rate, duty_ns);
+	period_cycles = pwm_omap_dmtimer_get_clock_cycles(omap, period_ns);
+	duty_cycles = pwm_omap_dmtimer_get_clock_cycles(omap, duty_ns);
 
 	if (period_cycles < 2) {
 		dev_info(chip->dev,
 			 "period %d ns too short for clock rate %lu Hz\n",
-			 period_ns, clk_rate);
+			 period_ns, omap->freq);
 		goto err_einval;
 	}
 
 	if (duty_cycles < 1) {
 		dev_dbg(chip->dev,
 			"duty cycle %d ns is too short for clock rate %lu Hz\n",
-			duty_ns, clk_rate);
+			duty_ns, omap->freq);
 		dev_dbg(chip->dev, "using minimum of 1 clock cycle\n");
 		duty_cycles = 1;
 	} else if (duty_cycles >= period_cycles) {
 		dev_dbg(chip->dev,
 			"duty cycle %d ns is too long for period %d ns at clock rate %lu Hz\n",
-			duty_ns, period_ns, clk_rate);
+			duty_ns, period_ns, omap->freq);
 		dev_dbg(chip->dev, "using maximum of 1 clock cycle less than period\n");
 		duty_cycles = period_cycles - 1;
 	}
 
 	dev_dbg(chip->dev, "effective duty cycle: %lld ns, period: %lld ns\n",
 		DIV_ROUND_CLOSEST_ULL((u64)NSEC_PER_SEC * duty_cycles,
-				      clk_rate),
+				      omap->freq),
 		DIV_ROUND_CLOSEST_ULL((u64)NSEC_PER_SEC * period_cycles,
-				      clk_rate));
+				      omap->freq));
 
 	load_value = (DM_TIMER_MAX - period_cycles) + 1;
 	match_value = load_value + duty_cycles - 1;
@@ -248,8 +235,9 @@ static int pwm_omap_dmtimer_probe(struct platform_device *pdev)
 	struct dmtimer_platform_data *timer_pdata;
 	struct omap_dm_timer_ops *pdata;
 	pwm_omap_dmtimer *dm_timer;
+	struct clk *fclk;
 	u32 v;
-	int status, ret;
+	int ret;
 
 	timer = of_parse_phandle(np, "ti,timers", 0);
 	if (!timer)
@@ -302,9 +290,8 @@ static int pwm_omap_dmtimer_probe(struct platform_device *pdev)
 
 	omap = devm_kzalloc(&pdev->dev, sizeof(*omap), GFP_KERNEL);
 	if (!omap) {
-		pdata->free(dm_timer);
 		ret = -ENOMEM;
-		goto put;
+		goto free;
 	}
 
 	omap->pdata = pdata;
@@ -315,15 +302,42 @@ static int pwm_omap_dmtimer_probe(struct platform_device *pdev)
 	 * Ensure that the timer is stopped before we allow PWM core to call
 	 * pwm_enable.
 	 */
-	if (pm_runtime_active(&omap->dm_timer_pdev->dev))
-		omap->pdata->stop(omap->dm_timer);
-
-	if (!of_property_read_u32(pdev->dev.of_node, "ti,prescaler", &v))
-		omap->pdata->set_prescaler(omap->dm_timer, v);
+	if (pm_runtime_active(&timer_pdev->dev))
+		pdata->stop(dm_timer);
 
 	/* setup dmtimer clock source */
-	if (!of_property_read_u32(pdev->dev.of_node, "ti,clock-source", &v))
-		omap->pdata->set_source(omap->dm_timer, v);
+	if (!of_property_read_u32(pdev->dev.of_node, "ti,clock-source", &v)) {
+		ret = pdata->set_source(dm_timer, v);
+		if (ret) {
+			dev_err(&pdev->dev, "invalid clock-source\n");
+			goto free;
+		}
+	}
+
+	fclk = pdata->get_fclk(dm_timer);
+	if (!fclk) {
+		dev_err(&pdev->dev, "invalid fclk\n");
+		ret = -EINVAL;
+		goto free;
+	}
+
+	omap->freq = clk_get_rate(fclk);
+	if (!omap->freq) {
+		dev_err(&pdev->dev, "invalid fclk rate\n");
+		ret = -EINVAL;
+		goto free;
+	}
+
+	if (!of_property_read_u32(pdev->dev.of_node, "ti,prescaler", &v)) {
+		ret = pdata->set_prescaler(dm_timer, v);
+		if (ret) {
+			dev_err(&pdev->dev, "invalid prescaler\n");
+			goto free;
+		}
+		omap->freq >>= v + 1;
+	}
+
+	dev_dbg(&pdev->dev, "clk rate: %luHz\n", omap->freq);
 
 	omap->chip.dev = &pdev->dev;
 	omap->chip.ops = &pwm_omap_dmtimer_ops;
@@ -334,18 +348,18 @@ static int pwm_omap_dmtimer_probe(struct platform_device *pdev)
 
 	mutex_init(&omap->mutex);
 
-	status = pwmchip_add(&omap->chip);
-	if (status < 0) {
+	ret = pwmchip_add(&omap->chip);
+	if (ret < 0) {
 		dev_err(&pdev->dev, "failed to register PWM\n");
-		omap->pdata->free(omap->dm_timer);
-		ret = status;
-		goto put;
+		goto free;
 	}
 
 	platform_set_drvdata(pdev, omap);
 
 	return 0;
 
+free:
+	pdata->free(dm_timer);
 put:
 	of_node_put(timer);
 

[PATCH 3/3] clocksource: timer-dm: Make unexported functions static

[Ladislav Michl]

As dmtimer no longer exports functions, make those previously
exported static.

Signed-off-by: Ladislav Michl <ladis@linux-mips.org>
---
 Note: only those functions assigned to timer ops are made static
       as some others will be needed later for event capture.

 drivers/clocksource/timer-dm.c |  218 ++++++++++++++++++++---------------------
 include/clocksource/dmtimer.h  |   24 ----
 2 files changed, 109 insertions(+), 133 deletions(-)

diff --git a/drivers/clocksource/timer-dm.c b/drivers/clocksource/timer-dm.c
index 324ec93d3dd2..8de9e543d129 100644
--- a/drivers/clocksource/timer-dm.c
+++ b/drivers/clocksource/timer-dm.c
@@ -163,6 +163,92 @@ static int omap_dm_timer_of_set_source(struct omap_dm_timer *timer)
 	return ret;
 }
 
+static int omap_dm_timer_set_source(struct omap_dm_timer *timer, int source)
+{
+	int ret;
+	char *parent_name = NULL;
+	struct clk *parent;
+	struct dmtimer_platform_data *pdata;
+
+	if (unlikely(!timer))
+		return -EINVAL;
+
+	pdata = timer->pdev->dev.platform_data;
+
+	if (source < 0 || source >= 3)
+		return -EINVAL;
+
+	/*
+	 * FIXME: Used for OMAP1 devices only because they do not currently
+	 * use the clock framework to set the parent clock. To be removed
+	 * once OMAP1 migrated to using clock framework for dmtimers
+	 */
+	if (pdata && pdata->set_timer_src)
+		return pdata->set_timer_src(timer->pdev, source);
+
+	if (IS_ERR(timer->fclk))
+		return -EINVAL;
+
+#if defined(CONFIG_COMMON_CLK)
+	/* Check if the clock has configurable parents */
+	if (clk_hw_get_num_parents(__clk_get_hw(timer->fclk)) < 2)
+		return 0;
+#endif
+
+	switch (source) {
+	case OMAP_TIMER_SRC_SYS_CLK:
+		parent_name = "timer_sys_ck";
+		break;
+
+	case OMAP_TIMER_SRC_32_KHZ:
+		parent_name = "timer_32k_ck";
+		break;
+
+	case OMAP_TIMER_SRC_EXT_CLK:
+		parent_name = "timer_ext_ck";
+		break;
+	}
+
+	parent = clk_get(&timer->pdev->dev, parent_name);
+	if (IS_ERR(parent)) {
+		pr_err("%s: %s not found\n", __func__, parent_name);
+		return -EINVAL;
+	}
+
+	ret = clk_set_parent(timer->fclk, parent);
+	if (ret < 0)
+		pr_err("%s: failed to set %s as parent\n", __func__,
+			parent_name);
+
+	clk_put(parent);
+
+	return ret;
+}
+
+static void omap_dm_timer_enable(struct omap_dm_timer *timer)
+{
+	int c;
+
+	pm_runtime_get_sync(&timer->pdev->dev);
+
+	if (!(timer->capability & OMAP_TIMER_ALWON)) {
+		if (timer->get_context_loss_count) {
+			c = timer->get_context_loss_count(&timer->pdev->dev);
+			if (c != timer->ctx_loss_count) {
+				omap_timer_restore_context(timer);
+				timer->ctx_loss_count = c;
+			}
+		} else {
+			omap_timer_restore_context(timer);
+		}
+	}
+}
+
+static void omap_dm_timer_disable(struct omap_dm_timer *timer)
+{
+	pm_runtime_put_sync(&timer->pdev->dev);
+}
+
 static int omap_dm_timer_prepare(struct omap_dm_timer *timer)
 {
 	int rc;
@@ -298,16 +384,16 @@ static struct omap_dm_timer *_omap_dm_timer_request(int req_type, void *data)
 	return timer;
 }
 
-struct omap_dm_timer *omap_dm_timer_request(void)
+static struct omap_dm_timer *omap_dm_timer_request(void)
 {
 	return _omap_dm_timer_request(REQUEST_ANY, NULL);
 }
 
-struct omap_dm_timer *omap_dm_timer_request_specific(int id)
+static struct omap_dm_timer *omap_dm_timer_request_specific(int id)
 {
 	/* Requesting timer by ID is not supported when device tree is used */
 	if (of_have_populated_dt()) {
-		pr_warn("%s: Please use omap_dm_timer_request_by_cap/node()\n",
+		pr_warn("%s: Please use omap_dm_timer_request_by_node()\n",
 			__func__);
 		return NULL;
 	}
@@ -336,7 +422,7 @@ struct omap_dm_timer *omap_dm_timer_request_by_cap(u32 cap)
  * Request a timer based upon a device node pointer. Returns pointer to
  * timer handle on success and a NULL pointer on failure.
  */
-struct omap_dm_timer *omap_dm_timer_request_by_node(struct device_node *np)
+static struct omap_dm_timer *omap_dm_timer_request_by_node(struct device_node *np)
 {
 	if (!np)
 		return NULL;
@@ -344,7 +430,7 @@ struct omap_dm_timer *omap_dm_timer_request_by_node(struct device_node *np)
 	return _omap_dm_timer_request(REQUEST_BY_NODE, np);
 }
 
-int omap_dm_timer_free(struct omap_dm_timer *timer)
+static int omap_dm_timer_free(struct omap_dm_timer *timer)
 {
 	if (unlikely(!timer))
 		return -EINVAL;
@@ -356,30 +442,6 @@ int omap_dm_timer_free(struct omap_dm_timer *timer)
 	return 0;
 }
 
-void omap_dm_timer_enable(struct omap_dm_timer *timer)
-{
-	int c;
-
-	pm_runtime_get_sync(&timer->pdev->dev);
-
-	if (!(timer->capability & OMAP_TIMER_ALWON)) {
-		if (timer->get_context_loss_count) {
-			c = timer->get_context_loss_count(&timer->pdev->dev);
-			if (c != timer->ctx_loss_count) {
-				omap_timer_restore_context(timer);
-				timer->ctx_loss_count = c;
-			}
-		} else {
-			omap_timer_restore_context(timer);
-		}
-	}
-}
-
-void omap_dm_timer_disable(struct omap_dm_timer *timer)
-{
-	pm_runtime_put_sync(&timer->pdev->dev);
-}
-
 int omap_dm_timer_get_irq(struct omap_dm_timer *timer)
 {
 	if (timer)
@@ -424,7 +486,7 @@ __u32 omap_dm_timer_modify_idlect_mask(__u32 inputmask)
 
 #else
 
-struct clk *omap_dm_timer_get_fclk(struct omap_dm_timer *timer)
+static struct clk *omap_dm_timer_get_fclk(struct omap_dm_timer *timer)
 {
 	if (timer && !IS_ERR(timer->fclk))
 		return timer->fclk;
@@ -451,7 +513,7 @@ int omap_dm_timer_trigger(struct omap_dm_timer *timer)
 	return 0;
 }
 
-int omap_dm_timer_start(struct omap_dm_timer *timer)
+static int omap_dm_timer_start(struct omap_dm_timer *timer)
 {
 	u32 l;
 
@@ -471,7 +533,7 @@ int omap_dm_timer_start(struct omap_dm_timer *timer)
 	return 0;
 }
 
-int omap_dm_timer_stop(struct omap_dm_timer *timer)
+static int omap_dm_timer_stop(struct omap_dm_timer *timer)
 {
 	unsigned long rate = 0;
 
@@ -494,70 +556,8 @@ int omap_dm_timer_stop(struct omap_dm_timer *timer)
 	return 0;
 }
 
-int omap_dm_timer_set_source(struct omap_dm_timer *timer, int source)
-{
-	int ret;
-	char *parent_name = NULL;
-	struct clk *parent;
-	struct dmtimer_platform_data *pdata;
-
-	if (unlikely(!timer))
-		return -EINVAL;
-
-	pdata = timer->pdev->dev.platform_data;
-
-	if (source < 0 || source >= 3)
-		return -EINVAL;
-
-	/*
-	 * FIXME: Used for OMAP1 devices only because they do not currently
-	 * use the clock framework to set the parent clock. To be removed
-	 * once OMAP1 migrated to using clock framework for dmtimers
-	 */
-	if (pdata && pdata->set_timer_src)
-		return pdata->set_timer_src(timer->pdev, source);
-
-	if (IS_ERR(timer->fclk))
-		return -EINVAL;
-
-#if defined(CONFIG_COMMON_CLK)
-	/* Check if the clock has configurable parents */
-	if (clk_hw_get_num_parents(__clk_get_hw(timer->fclk)) < 2)
-		return 0;
-#endif
-
-	switch (source) {
-	case OMAP_TIMER_SRC_SYS_CLK:
-		parent_name = "timer_sys_ck";
-		break;
-
-	case OMAP_TIMER_SRC_32_KHZ:
-		parent_name = "timer_32k_ck";
-		break;
-
-	case OMAP_TIMER_SRC_EXT_CLK:
-		parent_name = "timer_ext_ck";
-		break;
-	}
-
-	parent = clk_get(&timer->pdev->dev, parent_name);
-	if (IS_ERR(parent)) {
-		pr_err("%s: %s not found\n", __func__, parent_name);
-		return -EINVAL;
-	}
-
-	ret = clk_set_parent(timer->fclk, parent);
-	if (ret < 0)
-		pr_err("%s: failed to set %s as parent\n", __func__,
-			parent_name);
-
-	clk_put(parent);
-
-	return ret;
-}
-
-int omap_dm_timer_set_load(struct omap_dm_timer *timer, int autoreload,
-			    unsigned int load)
+static int omap_dm_timer_set_load(struct omap_dm_timer *timer, int autoreload,
+				  unsigned int load)
 {
 	u32 l;
 
@@ -609,9 +609,8 @@ int omap_dm_timer_set_load_start(struct omap_dm_timer *timer, int autoreload,
 	timer->context.tcrr = load;
 	return 0;
 }
-
-int omap_dm_timer_set_match(struct omap_dm_timer *timer, int enable,
-			     unsigned int match)
+static int omap_dm_timer_set_match(struct omap_dm_timer *timer, int enable,
+				   unsigned int match)
 {
 	u32 l;
 
@@ -634,8 +633,8 @@ int omap_dm_timer_set_match(struct omap_dm_timer *timer, int enable,
 	return 0;
 }
 
-int omap_dm_timer_set_pwm(struct omap_dm_timer *timer, int def_on,
-			   int toggle, int trigger)
+static int omap_dm_timer_set_pwm(struct omap_dm_timer *timer, int def_on,
+				 int toggle, int trigger)
 {
 	u32 l;
 
@@ -659,7 +658,8 @@ int omap_dm_timer_set_pwm(struct omap_dm_timer *timer, int def_on,
 	return 0;
 }
 
-int omap_dm_timer_set_prescaler(struct omap_dm_timer *timer, int prescaler)
+static int omap_dm_timer_set_prescaler(struct omap_dm_timer *timer,
+					int prescaler)
 {
 	u32 l;
 
@@ -681,8 +681,8 @@ int omap_dm_timer_set_prescaler(struct omap_dm_timer *timer, int prescaler)
 	return 0;
 }
 
-int omap_dm_timer_set_int_enable(struct omap_dm_timer *timer,
-				  unsigned int value)
+static int omap_dm_timer_set_int_enable(struct omap_dm_timer *timer,
+					unsigned int value)
 {
 	if (unlikely(!timer))
 		return -EINVAL;
@@ -704,7 +704,7 @@ int omap_dm_timer_set_int_enable(struct omap_dm_timer *timer,
  *
  * Disables the specified timer interrupts for a timer.
  */
-int omap_dm_timer_set_int_disable(struct omap_dm_timer *timer, u32 mask)
+static int omap_dm_timer_set_int_disable(struct omap_dm_timer *timer, u32 mask)
 {
 	u32 l = mask;
 
@@ -727,7 +727,7 @@ int omap_dm_timer_set_int_disable(struct omap_dm_timer *timer, u32 mask)
 	return 0;
 }
 
-unsigned int omap_dm_timer_read_status(struct omap_dm_timer *timer)
+static unsigned int omap_dm_timer_read_status(struct omap_dm_timer *timer)
 {
 	unsigned int l;
 
@@ -741,7 +741,7 @@ unsigned int omap_dm_timer_read_status(struct omap_dm_timer *timer)
 	return l;
 }
 
-int omap_dm_timer_write_status(struct omap_dm_timer *timer, unsigned int value)
+static int omap_dm_timer_write_status(struct omap_dm_timer *timer, unsigned int value)
 {
 	if (unlikely(!timer || pm_runtime_suspended(&timer->pdev->dev)))
 		return -EINVAL;
@@ -751,7 +751,7 @@ int omap_dm_timer_write_status(struct omap_dm_timer *timer, unsigned int value)
 	return 0;
 }
 
-unsigned int omap_dm_timer_read_counter(struct omap_dm_timer *timer)
+static unsigned int omap_dm_timer_read_counter(struct omap_dm_timer *timer)
 {
 	if (unlikely(!timer || pm_runtime_suspended(&timer->pdev->dev))) {
 		pr_err("%s: timer not iavailable or enabled.\n", __func__);
@@ -761,7 +761,7 @@ unsigned int omap_dm_timer_read_counter(struct omap_dm_timer *timer)
 	return __omap_dm_timer_read_counter(timer, timer->posted);
 }
 
-int omap_dm_timer_write_counter(struct omap_dm_timer *timer, unsigned int value)
+static int omap_dm_timer_write_counter(struct omap_dm_timer *timer, unsigned int value)
 {
 	if (unlikely(!timer || pm_runtime_suspended(&timer->pdev->dev))) {
 		pr_err("%s: timer not available or enabled.\n", __func__);
diff --git a/include/clocksource/dmtimer.h b/include/clocksource/dmtimer.h
index 862ad62dab9d..ce596172a93a 100644
--- a/include/clocksource/dmtimer.h
+++ b/include/clocksource/dmtimer.h
@@ -125,37 +125,13 @@ struct omap_dm_timer {
 };
 
 int omap_dm_timer_reserve_systimer(int id);
-struct omap_dm_timer *omap_dm_timer_request(void);
-struct omap_dm_timer *omap_dm_timer_request_specific(int timer_id);
 struct omap_dm_timer *omap_dm_timer_request_by_cap(u32 cap);
-struct omap_dm_timer *omap_dm_timer_request_by_node(struct device_node *np);
-int omap_dm_timer_free(struct omap_dm_timer *timer);
-void omap_dm_timer_enable(struct omap_dm_timer *timer);
-void omap_dm_timer_disable(struct omap_dm_timer *timer);
 
 int omap_dm_timer_get_irq(struct omap_dm_timer *timer);
 
 u32 omap_dm_timer_modify_idlect_mask(u32 inputmask);
-struct clk *omap_dm_timer_get_fclk(struct omap_dm_timer *timer);
 
 int omap_dm_timer_trigger(struct omap_dm_timer *timer);
-int omap_dm_timer_start(struct omap_dm_timer *timer);
-int omap_dm_timer_stop(struct omap_dm_timer *timer);
-
-int omap_dm_timer_set_source(struct omap_dm_timer *timer, int source);
-int omap_dm_timer_set_load(struct omap_dm_timer *timer, int autoreload, unsigned int value);
-int omap_dm_timer_set_load_start(struct omap_dm_timer *timer, int autoreload, unsigned int value);
-int omap_dm_timer_set_match(struct omap_dm_timer *timer, int enable, unsigned int match);
-int omap_dm_timer_set_pwm(struct omap_dm_timer *timer, int def_on, int toggle, int trigger);
-int omap_dm_timer_set_prescaler(struct omap_dm_timer *timer, int prescaler);
-
-int omap_dm_timer_set_int_enable(struct omap_dm_timer *timer, unsigned int value);
-int omap_dm_timer_set_int_disable(struct omap_dm_timer *timer, u32 mask);
-
-unsigned int omap_dm_timer_read_status(struct omap_dm_timer *timer);
-int omap_dm_timer_write_status(struct omap_dm_timer *timer, unsigned int value);
-unsigned int omap_dm_timer_read_counter(struct omap_dm_timer *timer);
-int omap_dm_timer_write_counter(struct omap_dm_timer *timer, unsigned int value);
 
 int omap_dm_timers_active(void);
 

Re: [PATCH 1/3] clocksource: timer-dm: Check prescaler value

[Claudiu Beznea]

On 17.01.2018 23:47, Ladislav Michl wrote:
> Invalid value silently disables use of the prescaler.
> Use -1 explicitely for that purpose and error out on
> invalid value.
> 
> Signed-off-by: Ladislav Michl <ladis@linux-mips.org>
> ---
>  drivers/clocksource/timer-dm.c |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/clocksource/timer-dm.c b/drivers/clocksource/timer-dm.c
> index 60db1734ea3b..324ec93d3dd2 100644
> --- a/drivers/clocksource/timer-dm.c
> +++ b/drivers/clocksource/timer-dm.c
> @@ -663,13 +663,13 @@ int omap_dm_timer_set_prescaler(struct omap_dm_timer *timer, int prescaler)
>  {
>  	u32 l;
>  
> -	if (unlikely(!timer))
> +	if (unlikely(!timer) || prescaler < -1 || prescaler > 7)
You are checking the prescaller here to be in [0, 7] interval.
>  		return -EINVAL;
>  
>  	omap_dm_timer_enable(timer);
>  	l = omap_dm_timer_read_reg(timer, OMAP_TIMER_CTRL_REG);
>  	l &= ~(OMAP_TIMER_CTRL_PRE | (0x07 << 2));
> -	if (prescaler >= 0x00 && prescaler <= 0x07) {
> +	if (prescaler >= 0) {
Is this check still necessary?
>  		l |= OMAP_TIMER_CTRL_PRE;
>  		l |= prescaler << 2;
>  	}
> 

Re: [PATCH 2/3] pwm: pwm-omap-dmtimer: Fix frequency when using prescaler

[Claudiu Beznea]

On 17.01.2018 23:47, Ladislav Michl wrote:
> Prescaler setting is currently not taken into account.
> Fix that by introducing freq member variable and initialize
> it at device probe time. This also avoids frequency
> recomputing at each pwm configure time.
> 
> Signed-off-by: Ladislav Michl <ladis@linux-mips.org>
> ---
>  drivers/pwm/pwm-omap-dmtimer.c |   92 +++++++++++++++++++++++----------------
>  1 file changed, 53 insertions(+), 39 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-omap-dmtimer.c b/drivers/pwm/pwm-omap-dmtimer.c
> index cc485d9946f3..81c79e41a167 100644
> --- a/drivers/pwm/pwm-omap-dmtimer.c
> +++ b/drivers/pwm/pwm-omap-dmtimer.c
> @@ -40,6 +40,7 @@ struct pwm_omap_dmtimer_chip {
>  	pwm_omap_dmtimer *dm_timer;
>  	struct omap_dm_timer_ops *pdata;
>  	struct platform_device *dm_timer_pdev;
> +	unsigned long freq;
>  };
>  
>  static inline struct pwm_omap_dmtimer_chip *
> @@ -48,9 +49,10 @@ to_pwm_omap_dmtimer_chip(struct pwm_chip *chip)
>  	return container_of(chip, struct pwm_omap_dmtimer_chip, chip);
>  }
>  
> -static u32 pwm_omap_dmtimer_get_clock_cycles(unsigned long clk_rate, int ns)
> +static inline u32
> +pwm_omap_dmtimer_get_clock_cycles(struct pwm_omap_dmtimer_chip *omap, int ns)
>  {
> -	return DIV_ROUND_CLOSEST_ULL((u64)clk_rate * ns, NSEC_PER_SEC);
> +	return DIV_ROUND_CLOSEST_ULL((u64)omap->freq * ns, NSEC_PER_SEC);
>  }
>  
>  static void pwm_omap_dmtimer_start(struct pwm_omap_dmtimer_chip *omap)
> @@ -99,8 +101,6 @@ static int pwm_omap_dmtimer_config(struct pwm_chip *chip,
>  	struct pwm_omap_dmtimer_chip *omap = to_pwm_omap_dmtimer_chip(chip);
>  	u32 period_cycles, duty_cycles;
>  	u32 load_value, match_value;
> -	struct clk *fclk;
> -	unsigned long clk_rate;
>  	bool timer_active;
>  
>  	dev_dbg(chip->dev, "requested duty cycle: %d ns, period: %d ns\n",
> @@ -114,19 +114,6 @@ static int pwm_omap_dmtimer_config(struct pwm_chip *chip,
>  		return 0;
>  	}
>  
> -	fclk = omap->pdata->get_fclk(omap->dm_timer);
> -	if (!fclk) {
> -		dev_err(chip->dev, "invalid pmtimer fclk\n");
> -		goto err_einval;
> -	}
> -
> -	clk_rate = clk_get_rate(fclk);
> -	if (!clk_rate) {
> -		dev_err(chip->dev, "invalid pmtimer fclk rate\n");
> -		goto err_einval;
> -	}
> -
> -	dev_dbg(chip->dev, "clk rate: %luHz\n", clk_rate);
>  
>  	/*
>  	 * Calculate the appropriate load and match values based on the
> @@ -144,35 +131,35 @@ static int pwm_omap_dmtimer_config(struct pwm_chip *chip,
>  	 *   OMAP4430/60/70 TRM sections 22.2.4.10 and 22.2.4.11
>  	 *   AM335x Sitara TRM sections 20.1.3.5 and 20.1.3.6
>  	 */
> -	period_cycles = pwm_omap_dmtimer_get_clock_cycles(clk_rate, period_ns);
> -	duty_cycles = pwm_omap_dmtimer_get_clock_cycles(clk_rate, duty_ns);
> +	period_cycles = pwm_omap_dmtimer_get_clock_cycles(omap, period_ns);
> +	duty_cycles = pwm_omap_dmtimer_get_clock_cycles(omap, duty_ns);
>  
>  	if (period_cycles < 2) {
>  		dev_info(chip->dev,
>  			 "period %d ns too short for clock rate %lu Hz\n",
> -			 period_ns, clk_rate);
> +			 period_ns, omap->freq);
>  		goto err_einval;
>  	}
>  
>  	if (duty_cycles < 1) {
>  		dev_dbg(chip->dev,
>  			"duty cycle %d ns is too short for clock rate %lu Hz\n",
> -			duty_ns, clk_rate);
> +			duty_ns, omap->freq);
>  		dev_dbg(chip->dev, "using minimum of 1 clock cycle\n");
>  		duty_cycles = 1;
>  	} else if (duty_cycles >= period_cycles) {
>  		dev_dbg(chip->dev,
>  			"duty cycle %d ns is too long for period %d ns at clock rate %lu Hz\n",
> -			duty_ns, period_ns, clk_rate);
> +			duty_ns, period_ns, omap->freq);
>  		dev_dbg(chip->dev, "using maximum of 1 clock cycle less than period\n");
>  		duty_cycles = period_cycles - 1;
>  	}
>  
>  	dev_dbg(chip->dev, "effective duty cycle: %lld ns, period: %lld ns\n",
>  		DIV_ROUND_CLOSEST_ULL((u64)NSEC_PER_SEC * duty_cycles,
> -				      clk_rate),
> +				      omap->freq),
>  		DIV_ROUND_CLOSEST_ULL((u64)NSEC_PER_SEC * period_cycles,
> -				      clk_rate));
> +				      omap->freq));
>  
>  	load_value = (DM_TIMER_MAX - period_cycles) + 1;
>  	match_value = load_value + duty_cycles - 1;
> @@ -248,8 +235,9 @@ static int pwm_omap_dmtimer_probe(struct platform_device *pdev)
>  	struct dmtimer_platform_data *timer_pdata;
>  	struct omap_dm_timer_ops *pdata;
>  	pwm_omap_dmtimer *dm_timer;
> +	struct clk *fclk;
>  	u32 v;
> -	int status, ret;
> +	int ret;
>  
>  	timer = of_parse_phandle(np, "ti,timers", 0);
>  	if (!timer)
> @@ -302,9 +290,8 @@ static int pwm_omap_dmtimer_probe(struct platform_device *pdev)
>  
>  	omap = devm_kzalloc(&pdev->dev, sizeof(*omap), GFP_KERNEL);
>  	if (!omap) {
> -		pdata->free(dm_timer);
>  		ret = -ENOMEM;
> -		goto put;
> +		goto free;
>  	}
>  
>  	omap->pdata = pdata;
> @@ -315,15 +302,42 @@ static int pwm_omap_dmtimer_probe(struct platform_device *pdev)
>  	 * Ensure that the timer is stopped before we allow PWM core to call
>  	 * pwm_enable.
>  	 */
> -	if (pm_runtime_active(&omap->dm_timer_pdev->dev))
> -		omap->pdata->stop(omap->dm_timer);
> -
> -	if (!of_property_read_u32(pdev->dev.of_node, "ti,prescaler", &v))
> -		omap->pdata->set_prescaler(omap->dm_timer, v);
> +	if (pm_runtime_active(&timer_pdev->dev))
> +		pdata->stop(dm_timer);
>  
>  	/* setup dmtimer clock source */
> -	if (!of_property_read_u32(pdev->dev.of_node, "ti,clock-source", &v))
> -		omap->pdata->set_source(omap->dm_timer, v);
> +	if (!of_property_read_u32(pdev->dev.of_node, "ti,clock-source", &v)) {
> +		ret = pdata->set_source(dm_timer, v);
> +		if (ret) {
> +			dev_err(&pdev->dev, "invalid clock-source\n");
> +			goto free;
> +		}
> +	}
> +
> +	fclk = pdata->get_fclk(dm_timer);
> +	if (!fclk) {
> +		dev_err(&pdev->dev, "invalid fclk\n");
> +		ret = -EINVAL;
> +		goto free;
> +	}
> +
> +	omap->freq = clk_get_rate(fclk);
> +	if (!omap->freq) {
> +		dev_err(&pdev->dev, "invalid fclk rate\n");
> +		ret = -EINVAL;
> +		goto free;
> +	}
> +
> +	if (!of_property_read_u32(pdev->dev.of_node, "ti,prescaler", &v)) {
> +		ret = pdata->set_prescaler(dm_timer, v);
> +		if (ret) {
> +			dev_err(&pdev->dev, "invalid prescaler\n");
> +			goto free;
> +		}
> +		omap->freq >>= v + 1;
> +	}
> +
> +	dev_dbg(&pdev->dev, "clk rate: %luHz\n", omap->freq);
>  
>  	omap->chip.dev = &pdev->dev;
>  	omap->chip.ops = &pwm_omap_dmtimer_ops;
> @@ -334,18 +348,18 @@ static int pwm_omap_dmtimer_probe(struct platform_device *pdev)
>  
>  	mutex_init(&omap->mutex);
>  
> -	status = pwmchip_add(&omap->chip);
> -	if (status < 0) {
> +	ret = pwmchip_add(&omap->chip);
> +	if (ret < 0) {
>  		dev_err(&pdev->dev, "failed to register PWM\n");
> -		omap->pdata->free(omap->dm_timer);
> -		ret = status;
> -		goto put;
> +		goto free;
>  	}
>  
>  	platform_set_drvdata(pdev, omap);
>From documentation: "of_parse_phandle(): Returns the device_node pointer with refcount
incremented. Use of_node_put() on it when done."
In case of success the of_node_put() should also be called as I see.

>  
>  	return 0;
>  
> +free:
> +	pdata->free(dm_timer);
>  put:
>  	of_node_put(timer);
>  
> 

Re: [PATCH 3/3] clocksource: timer-dm: Make unexported functions static

[Claudiu Beznea]

On 17.01.2018 23:48, Ladislav Michl wrote:
> As dmtimer no longer exports functions, make those previously
> exported static.
> 
> Signed-off-by: Ladislav Michl <ladis@linux-mips.org>
> ---
>  Note: only those functions assigned to timer ops are made static
>        as some others will be needed later for event capture.
> 
>  drivers/clocksource/timer-dm.c |  218 ++++++++++++++++++++---------------------
>  include/clocksource/dmtimer.h  |   24 ----
>  2 files changed, 109 insertions(+), 133 deletions(-)
> 
> diff --git a/drivers/clocksource/timer-dm.c b/drivers/clocksource/timer-dm.c
> index 324ec93d3dd2..8de9e543d129 100644
> --- a/drivers/clocksource/timer-dm.c
> +++ b/drivers/clocksource/timer-dm.c
> @@ -163,6 +163,92 @@ static int omap_dm_timer_of_set_source(struct omap_dm_timer *timer)
>  	return ret;
>  }
>  
> +static int omap_dm_timer_set_source(struct omap_dm_timer *timer, int source)
> +{
> +	int ret;
> +	char *parent_name = NULL;
Could be declared as const: const char *parent_name = NULL;

> +	struct clk *parent;
> +	struct dmtimer_platform_data *pdata;
> +
> +	if (unlikely(!timer))
> +		return -EINVAL;
> +
> +	pdata = timer->pdev->dev.platform_data;
> +
> +	if (source < 0 || source >= 3)
> +		return -EINVAL;
> +
> +	/*
> +	 * FIXME: Used for OMAP1 devices only because they do not currently
> +	 * use the clock framework to set the parent clock. To be removed
> +	 * once OMAP1 migrated to using clock framework for dmtimers
> +	 */
> +	if (pdata && pdata->set_timer_src)
> +		return pdata->set_timer_src(timer->pdev, source);
> +
> +	if (IS_ERR(timer->fclk))
> +		return -EINVAL;
Souldn't this check be done at the beginning of the function?

> +
> +#if defined(CONFIG_COMMON_CLK)
> +	/* Check if the clock has configurable parents */
> +	if (clk_hw_get_num_parents(__clk_get_hw(timer->fclk)) < 2)
> +		return 0;
> +#endif
> +
> +	switch (source) {
> +	case OMAP_TIMER_SRC_SYS_CLK:
> +		parent_name = "timer_sys_ck";
> +		break;
> +
> +	case OMAP_TIMER_SRC_32_KHZ:
> +		parent_name = "timer_32k_ck";
> +		break;
> +
> +	case OMAP_TIMER_SRC_EXT_CLK:
> +		parent_name = "timer_ext_ck";
> +		break;
> +	}
> +
> +	parent = clk_get(&timer->pdev->dev, parent_name);
> +	if (IS_ERR(parent)) {
> +		pr_err("%s: %s not found\n", __func__, parent_name);
> +		return -EINVAL;
> +	}
> +
> +	ret = clk_set_parent(timer->fclk, parent);
> +	if (ret < 0)
> +		pr_err("%s: failed to set %s as parent\n", __func__,
> +			parent_name);
> +
> +	clk_put(parent);
> +
> +	return ret;
> +}
> +
> +static void omap_dm_timer_enable(struct omap_dm_timer *timer)
> +{
> +	int c;
> +
> +	pm_runtime_get_sync(&timer->pdev->dev);
> +
> +	if (!(timer->capability & OMAP_TIMER_ALWON)) {
> +		if (timer->get_context_loss_count) {
> +			c = timer->get_context_loss_count(&timer->pdev->dev);
> +			if (c != timer->ctx_loss_count) {
> +				omap_timer_restore_context(timer);
> +				timer->ctx_loss_count = c;
> +			}
> +		} else {
> +			omap_timer_restore_context(timer);
> +		}
> +	}
> +}
> +
> +static void omap_dm_timer_disable(struct omap_dm_timer *timer)
> +{
> +	pm_runtime_put_sync(&timer->pdev->dev);
> +}
> +
>  static int omap_dm_timer_prepare(struct omap_dm_timer *timer)
>  {
>  	int rc;
> @@ -298,16 +384,16 @@ static struct omap_dm_timer *_omap_dm_timer_request(int req_type, void *data)
>  	return timer;
>  }
>  
> -struct omap_dm_timer *omap_dm_timer_request(void)
> +static struct omap_dm_timer *omap_dm_timer_request(void)
>  {
>  	return _omap_dm_timer_request(REQUEST_ANY, NULL);
>  }
>  
> -struct omap_dm_timer *omap_dm_timer_request_specific(int id)
> +static struct omap_dm_timer *omap_dm_timer_request_specific(int id)
>  {
>  	/* Requesting timer by ID is not supported when device tree is used */
>  	if (of_have_populated_dt()) {
> -		pr_warn("%s: Please use omap_dm_timer_request_by_cap/node()\n",
> +		pr_warn("%s: Please use omap_dm_timer_request_by_node()\n",
>  			__func__);
>  		return NULL;
>  	}
> @@ -336,7 +422,7 @@ struct omap_dm_timer *omap_dm_timer_request_by_cap(u32 cap)
>   * Request a timer based upon a device node pointer. Returns pointer to
>   * timer handle on success and a NULL pointer on failure.
>   */
> -struct omap_dm_timer *omap_dm_timer_request_by_node(struct device_node *np)
> +static struct omap_dm_timer *omap_dm_timer_request_by_node(struct device_node *np)
>  {
>  	if (!np)
>  		return NULL;
> @@ -344,7 +430,7 @@ struct omap_dm_timer *omap_dm_timer_request_by_node(struct device_node *np)
>  	return _omap_dm_timer_request(REQUEST_BY_NODE, np);
>  }
>  
> -int omap_dm_timer_free(struct omap_dm_timer *timer)
> +static int omap_dm_timer_free(struct omap_dm_timer *timer)
>  {
>  	if (unlikely(!timer))
>  		return -EINVAL;
> @@ -356,30 +442,6 @@ int omap_dm_timer_free(struct omap_dm_timer *timer)
>  	return 0;
>  }
>  
> -void omap_dm_timer_enable(struct omap_dm_timer *timer)
> -{
> -	int c;
> -
> -	pm_runtime_get_sync(&timer->pdev->dev);
> -
> -	if (!(timer->capability & OMAP_TIMER_ALWON)) {
> -		if (timer->get_context_loss_count) {
> -			c = timer->get_context_loss_count(&timer->pdev->dev);
> -			if (c != timer->ctx_loss_count) {
> -				omap_timer_restore_context(timer);
> -				timer->ctx_loss_count = c;
> -			}
> -		} else {
> -			omap_timer_restore_context(timer);
> -		}
> -	}
> -}
> -
> -void omap_dm_timer_disable(struct omap_dm_timer *timer)
> -{
> -	pm_runtime_put_sync(&timer->pdev->dev);
> -}
> -
>  int omap_dm_timer_get_irq(struct omap_dm_timer *timer)
>  {
>  	if (timer)
> @@ -424,7 +486,7 @@ __u32 omap_dm_timer_modify_idlect_mask(__u32 inputmask)
>  
>  #else
>  
> -struct clk *omap_dm_timer_get_fclk(struct omap_dm_timer *timer)
> +static struct clk *omap_dm_timer_get_fclk(struct omap_dm_timer *timer)
>  {
>  	if (timer && !IS_ERR(timer->fclk))
>  		return timer->fclk;
> @@ -451,7 +513,7 @@ int omap_dm_timer_trigger(struct omap_dm_timer *timer)
>  	return 0;
>  }
>  
> -int omap_dm_timer_start(struct omap_dm_timer *timer)
> +static int omap_dm_timer_start(struct omap_dm_timer *timer)
>  {
>  	u32 l;
>  
> @@ -471,7 +533,7 @@ int omap_dm_timer_start(struct omap_dm_timer *timer)
>  	return 0;
>  }
>  
> -int omap_dm_timer_stop(struct omap_dm_timer *timer)
> +static int omap_dm_timer_stop(struct omap_dm_timer *timer)
>  {
>  	unsigned long rate = 0;
>  
> @@ -494,70 +556,8 @@ int omap_dm_timer_stop(struct omap_dm_timer *timer)
>  	return 0;
>  }
>  
> -int omap_dm_timer_set_source(struct omap_dm_timer *timer, int source)
> -{
> -	int ret;
> -	char *parent_name = NULL;
> -	struct clk *parent;
> -	struct dmtimer_platform_data *pdata;
> -
> -	if (unlikely(!timer))
> -		return -EINVAL;
> -
> -	pdata = timer->pdev->dev.platform_data;
> -
> -	if (source < 0 || source >= 3)
> -		return -EINVAL;
> -
> -	/*
> -	 * FIXME: Used for OMAP1 devices only because they do not currently
> -	 * use the clock framework to set the parent clock. To be removed
> -	 * once OMAP1 migrated to using clock framework for dmtimers
> -	 */
> -	if (pdata && pdata->set_timer_src)
> -		return pdata->set_timer_src(timer->pdev, source);
> -
> -	if (IS_ERR(timer->fclk))
> -		return -EINVAL;
> -
> -#if defined(CONFIG_COMMON_CLK)
> -	/* Check if the clock has configurable parents */
> -	if (clk_hw_get_num_parents(__clk_get_hw(timer->fclk)) < 2)
> -		return 0;
> -#endif
> -
> -	switch (source) {
> -	case OMAP_TIMER_SRC_SYS_CLK:
> -		parent_name = "timer_sys_ck";
> -		break;
> -
> -	case OMAP_TIMER_SRC_32_KHZ:
> -		parent_name = "timer_32k_ck";
> -		break;
> -
> -	case OMAP_TIMER_SRC_EXT_CLK:
> -		parent_name = "timer_ext_ck";
> -		break;
> -	}
> -
> -	parent = clk_get(&timer->pdev->dev, parent_name);
> -	if (IS_ERR(parent)) {
> -		pr_err("%s: %s not found\n", __func__, parent_name);
> -		return -EINVAL;
> -	}
> -
> -	ret = clk_set_parent(timer->fclk, parent);
> -	if (ret < 0)
> -		pr_err("%s: failed to set %s as parent\n", __func__,
> -			parent_name);
> -
> -	clk_put(parent);
> -
> -	return ret;
> -}
> -
> -int omap_dm_timer_set_load(struct omap_dm_timer *timer, int autoreload,
> -			    unsigned int load)
> +static int omap_dm_timer_set_load(struct omap_dm_timer *timer, int autoreload,
> +				  unsigned int load)
>  {
>  	u32 l;
>  
> @@ -609,9 +609,8 @@ int omap_dm_timer_set_load_start(struct omap_dm_timer *timer, int autoreload,
>  	timer->context.tcrr = load;
>  	return 0;
>  }
> -
> -int omap_dm_timer_set_match(struct omap_dm_timer *timer, int enable,
> -			     unsigned int match)
> +static int omap_dm_timer_set_match(struct omap_dm_timer *timer, int enable,
> +				   unsigned int match)
>  {
>  	u32 l;
>  
> @@ -634,8 +633,8 @@ int omap_dm_timer_set_match(struct omap_dm_timer *timer, int enable,
>  	return 0;
>  }
>  
> -int omap_dm_timer_set_pwm(struct omap_dm_timer *timer, int def_on,
> -			   int toggle, int trigger)
> +static int omap_dm_timer_set_pwm(struct omap_dm_timer *timer, int def_on,
> +				 int toggle, int trigger)
>  {
>  	u32 l;
>  
> @@ -659,7 +658,8 @@ int omap_dm_timer_set_pwm(struct omap_dm_timer *timer, int def_on,
>  	return 0;
>  }
>  
> -int omap_dm_timer_set_prescaler(struct omap_dm_timer *timer, int prescaler)
> +static int omap_dm_timer_set_prescaler(struct omap_dm_timer *timer,
> +					int prescaler)
>  {
>  	u32 l;
>  
> @@ -681,8 +681,8 @@ int omap_dm_timer_set_prescaler(struct omap_dm_timer *timer, int prescaler)
>  	return 0;
>  }
>  
> -int omap_dm_timer_set_int_enable(struct omap_dm_timer *timer,
> -				  unsigned int value)
> +static int omap_dm_timer_set_int_enable(struct omap_dm_timer *timer,
> +					unsigned int value)
>  {
>  	if (unlikely(!timer))
>  		return -EINVAL;
> @@ -704,7 +704,7 @@ int omap_dm_timer_set_int_enable(struct omap_dm_timer *timer,
>   *
>   * Disables the specified timer interrupts for a timer.
>   */
> -int omap_dm_timer_set_int_disable(struct omap_dm_timer *timer, u32 mask)
> +static int omap_dm_timer_set_int_disable(struct omap_dm_timer *timer, u32 mask)
>  {
>  	u32 l = mask;
>  
> @@ -727,7 +727,7 @@ int omap_dm_timer_set_int_disable(struct omap_dm_timer *timer, u32 mask)
>  	return 0;
>  }
>  
> -unsigned int omap_dm_timer_read_status(struct omap_dm_timer *timer)
> +static unsigned int omap_dm_timer_read_status(struct omap_dm_timer *timer)
>  {
>  	unsigned int l;
>  
> @@ -741,7 +741,7 @@ unsigned int omap_dm_timer_read_status(struct omap_dm_timer *timer)
>  	return l;
>  }
>  
> -int omap_dm_timer_write_status(struct omap_dm_timer *timer, unsigned int value)
> +static int omap_dm_timer_write_status(struct omap_dm_timer *timer, unsigned int value)
>  {
>  	if (unlikely(!timer || pm_runtime_suspended(&timer->pdev->dev)))
>  		return -EINVAL;
> @@ -751,7 +751,7 @@ int omap_dm_timer_write_status(struct omap_dm_timer *timer, unsigned int value)
>  	return 0;
>  }
>  
> -unsigned int omap_dm_timer_read_counter(struct omap_dm_timer *timer)
> +static unsigned int omap_dm_timer_read_counter(struct omap_dm_timer *timer)
>  {
>  	if (unlikely(!timer || pm_runtime_suspended(&timer->pdev->dev))) {
>  		pr_err("%s: timer not iavailable or enabled.\n", __func__);
> @@ -761,7 +761,7 @@ unsigned int omap_dm_timer_read_counter(struct omap_dm_timer *timer)
>  	return __omap_dm_timer_read_counter(timer, timer->posted);
>  }
>  
> -int omap_dm_timer_write_counter(struct omap_dm_timer *timer, unsigned int value)
> +static int omap_dm_timer_write_counter(struct omap_dm_timer *timer, unsigned int value)
>  {
>  	if (unlikely(!timer || pm_runtime_suspended(&timer->pdev->dev))) {
>  		pr_err("%s: timer not available or enabled.\n", __func__);
> diff --git a/include/clocksource/dmtimer.h b/include/clocksource/dmtimer.h
> index 862ad62dab9d..ce596172a93a 100644
> --- a/include/clocksource/dmtimer.h
> +++ b/include/clocksource/dmtimer.h
> @@ -125,37 +125,13 @@ struct omap_dm_timer {
>  };
>  
>  int omap_dm_timer_reserve_systimer(int id);
> -struct omap_dm_timer *omap_dm_timer_request(void);
> -struct omap_dm_timer *omap_dm_timer_request_specific(int timer_id);
>  struct omap_dm_timer *omap_dm_timer_request_by_cap(u32 cap);
> -struct omap_dm_timer *omap_dm_timer_request_by_node(struct device_node *np);
> -int omap_dm_timer_free(struct omap_dm_timer *timer);
> -void omap_dm_timer_enable(struct omap_dm_timer *timer);
> -void omap_dm_timer_disable(struct omap_dm_timer *timer);
>  
>  int omap_dm_timer_get_irq(struct omap_dm_timer *timer);
>  
>  u32 omap_dm_timer_modify_idlect_mask(u32 inputmask);
> -struct clk *omap_dm_timer_get_fclk(struct omap_dm_timer *timer);
>  
>  int omap_dm_timer_trigger(struct omap_dm_timer *timer);
> -int omap_dm_timer_start(struct omap_dm_timer *timer);
> -int omap_dm_timer_stop(struct omap_dm_timer *timer);
> -
> -int omap_dm_timer_set_source(struct omap_dm_timer *timer, int source);
> -int omap_dm_timer_set_load(struct omap_dm_timer *timer, int autoreload, unsigned int value);
> -int omap_dm_timer_set_load_start(struct omap_dm_timer *timer, int autoreload, unsigned int value);
> -int omap_dm_timer_set_match(struct omap_dm_timer *timer, int enable, unsigned int match);
> -int omap_dm_timer_set_pwm(struct omap_dm_timer *timer, int def_on, int toggle, int trigger);
> -int omap_dm_timer_set_prescaler(struct omap_dm_timer *timer, int prescaler);
> -
> -int omap_dm_timer_set_int_enable(struct omap_dm_timer *timer, unsigned int value);
> -int omap_dm_timer_set_int_disable(struct omap_dm_timer *timer, u32 mask);
> -
> -unsigned int omap_dm_timer_read_status(struct omap_dm_timer *timer);
> -int omap_dm_timer_write_status(struct omap_dm_timer *timer, unsigned int value);
> -unsigned int omap_dm_timer_read_counter(struct omap_dm_timer *timer);
> -int omap_dm_timer_write_counter(struct omap_dm_timer *timer, unsigned int value);
>  
>  int omap_dm_timers_active(void);
>  
> 

Re: [PATCH 1/3] clocksource: timer-dm: Check prescaler value

[Ladislav Michl]

On Mon, Jan 22, 2018 at 11:00:15AM +0200, Claudiu Beznea wrote:
> 
> 
> On 17.01.2018 23:47, Ladislav Michl wrote:
> > Invalid value silently disables use of the prescaler.
> > Use -1 explicitely for that purpose and error out on
> > invalid value.
> > 
> > Signed-off-by: Ladislav Michl <ladis@linux-mips.org>
> > ---
> >  drivers/clocksource/timer-dm.c |    4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/clocksource/timer-dm.c b/drivers/clocksource/timer-dm.c
> > index 60db1734ea3b..324ec93d3dd2 100644
> > --- a/drivers/clocksource/timer-dm.c
> > +++ b/drivers/clocksource/timer-dm.c
> > @@ -663,13 +663,13 @@ int omap_dm_timer_set_prescaler(struct omap_dm_timer *timer, int prescaler)
> >  {
> >  	u32 l;
> >  
> > -	if (unlikely(!timer))
> > +	if (unlikely(!timer) || prescaler < -1 || prescaler > 7)
> You are checking the prescaller here to be in [0, 7] interval.

You mean [-1, 7] I suppose.

> >  		return -EINVAL;
> >  
> >  	omap_dm_timer_enable(timer);
> >  	l = omap_dm_timer_read_reg(timer, OMAP_TIMER_CTRL_REG);
> >  	l &= ~(OMAP_TIMER_CTRL_PRE | (0x07 << 2));
> > -	if (prescaler >= 0x00 && prescaler <= 0x07) {
> > +	if (prescaler >= 0) {
> Is this check still necessary?

Yes, as we need also some way to disable prescaler, see commit message.

> >  		l |= OMAP_TIMER_CTRL_PRE;
> >  		l |= prescaler << 2;
> >  	}

Best regards,
	ladis

Re: [PATCH 1/3] clocksource: timer-dm: Check prescaler value

[Claudiu Beznea]

On 22.01.2018 11:59, Ladislav Michl wrote:
> On Mon, Jan 22, 2018 at 11:00:15AM +0200, Claudiu Beznea wrote:
>>
>>
>> On 17.01.2018 23:47, Ladislav Michl wrote:
>>> Invalid value silently disables use of the prescaler.
>>> Use -1 explicitely for that purpose and error out on
>>> invalid value.
>>>
>>> Signed-off-by: Ladislav Michl <ladis@linux-mips.org>
>>> ---
>>>  drivers/clocksource/timer-dm.c |    4 ++--
>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/clocksource/timer-dm.c b/drivers/clocksource/timer-dm.c
>>> index 60db1734ea3b..324ec93d3dd2 100644
>>> --- a/drivers/clocksource/timer-dm.c
>>> +++ b/drivers/clocksource/timer-dm.c
>>> @@ -663,13 +663,13 @@ int omap_dm_timer_set_prescaler(struct omap_dm_timer *timer, int prescaler)
>>>  {
>>>  	u32 l;
>>>  
>>> -	if (unlikely(!timer))
>>> +	if (unlikely(!timer) || prescaler < -1 || prescaler > 7)
>> You are checking the prescaller here to be in [0, 7] interval.
> 
> You mean [-1, 7] I suppose.
Right, [-1, 7], my bad. Ok, So you use -1 to disable the prescaler.

> 
>>>  		return -EINVAL;
>>>  
>>>  	omap_dm_timer_enable(timer);
>>>  	l = omap_dm_timer_read_reg(timer, OMAP_TIMER_CTRL_REG);
>>>  	l &= ~(OMAP_TIMER_CTRL_PRE | (0x07 << 2));
>>> -	if (prescaler >= 0x00 && prescaler <= 0x07) {
>>> +	if (prescaler >= 0) {
>> Is this check still necessary?
> 
> Yes, as we need also some way to disable prescaler, see commit message.
> 
>>>  		l |= OMAP_TIMER_CTRL_PRE;
>>>  		l |= prescaler << 2;
>>>  	}
> 
> Best regards,
> 	ladis
> 

Re: [PATCH 3/3] clocksource: timer-dm: Make unexported functions static

[Ladislav Michl]

On Mon, Jan 22, 2018 at 11:26:44AM +0200, Claudiu Beznea wrote:
> On 17.01.2018 23:48, Ladislav Michl wrote:
> > As dmtimer no longer exports functions, make those previously
> > exported static.
> > 
> > Signed-off-by: Ladislav Michl <ladis@linux-mips.org>
> > ---
> >  Note: only those functions assigned to timer ops are made static
> >        as some others will be needed later for event capture.
> > 
> >  drivers/clocksource/timer-dm.c |  218 ++++++++++++++++++++---------------------
> >  include/clocksource/dmtimer.h  |   24 ----
> >  2 files changed, 109 insertions(+), 133 deletions(-)
> > 
> > diff --git a/drivers/clocksource/timer-dm.c b/drivers/clocksource/timer-dm.c
> > index 324ec93d3dd2..8de9e543d129 100644
> > --- a/drivers/clocksource/timer-dm.c
> > +++ b/drivers/clocksource/timer-dm.c
> > @@ -163,6 +163,92 @@ static int omap_dm_timer_of_set_source(struct omap_dm_timer *timer)
> >  	return ret;
> >  }
> >  
> > +static int omap_dm_timer_set_source(struct omap_dm_timer *timer, int source)
> > +{
> > +	int ret;
> > +	char *parent_name = NULL;
> Could be declared as const: const char *parent_name = NULL;
> 
> > +	struct clk *parent;
> > +	struct dmtimer_platform_data *pdata;
> > +
> > +	if (unlikely(!timer))
> > +		return -EINVAL;
> > +
> > +	pdata = timer->pdev->dev.platform_data;
> > +
> > +	if (source < 0 || source >= 3)
> > +		return -EINVAL;
> > +
> > +	/*
> > +	 * FIXME: Used for OMAP1 devices only because they do not currently
> > +	 * use the clock framework to set the parent clock. To be removed
> > +	 * once OMAP1 migrated to using clock framework for dmtimers
> > +	 */
> > +	if (pdata && pdata->set_timer_src)
> > +		return pdata->set_timer_src(timer->pdev, source);
> > +
> > +	if (IS_ERR(timer->fclk))
> > +		return -EINVAL;
> Souldn't this check be done at the beginning of the function?

Well, this patch only moves existing functions around, just to make it
compile after declarations are removed from header file.

Fixes should be sent separately as this is not the only problem with
clocksource code :)

Anyway, thank you for review, this one could be addressed by a following
patch:

diff --git a/drivers/clocksource/timer-dm.c b/drivers/clocksource/timer-dm.c
index dbf2b1f6a941..02b3436db70e 100644
--- a/drivers/clocksource/timer-dm.c
+++ b/drivers/clocksource/timer-dm.c
@@ -166,17 +166,30 @@ static int omap_dm_timer_of_set_source(struct omap_dm_timer *timer)
 static int omap_dm_timer_set_source(struct omap_dm_timer *timer, int source)
 {
 	int ret;
-	char *parent_name = NULL;
+	const char *parent_name;
 	struct clk *parent;
 	struct dmtimer_platform_data *pdata;
 
-	if (unlikely(!timer))
+	if (unlikely(!timer) || IS_ERR(timer->fclk))
 		return -EINVAL;
 
-	pdata = timer->pdev->dev.platform_data;
+	switch (source) {
+	case OMAP_TIMER_SRC_SYS_CLK:
+		parent_name = "timer_sys_ck";
+		break;
 
-	if (source < 0 || source >= 3)
+	case OMAP_TIMER_SRC_32_KHZ:
+		parent_name = "timer_32k_ck";
+		break;
+
+	case OMAP_TIMER_SRC_EXT_CLK:
+		parent_name = "timer_ext_ck";
+		break;
+	default:
 		return -EINVAL;
+	}
+
+	pdata = timer->pdev->dev.platform_data;
 
 	/*
 	 * FIXME: Used for OMAP1 devices only because they do not currently
@@ -186,29 +199,12 @@ static int omap_dm_timer_set_source(struct omap_dm_timer *timer, int source)
 	if (pdata && pdata->set_timer_src)
 		return pdata->set_timer_src(timer->pdev, source);
 
-	if (IS_ERR(timer->fclk))
-		return -EINVAL;
-
 #if defined(CONFIG_COMMON_CLK)
 	/* Check if the clock has configurable parents */
 	if (clk_hw_get_num_parents(__clk_get_hw(timer->fclk)) < 2)
 		return 0;
 #endif
 
-	switch (source) {
-	case OMAP_TIMER_SRC_SYS_CLK:
-		parent_name = "timer_sys_ck";
-		break;
-
-	case OMAP_TIMER_SRC_32_KHZ:
-		parent_name = "timer_32k_ck";
-		break;
-
-	case OMAP_TIMER_SRC_EXT_CLK:
-		parent_name = "timer_ext_ck";
-		break;
-	}
-
 	parent = clk_get(&timer->pdev->dev, parent_name);
 	if (IS_ERR(parent)) {
 		pr_err("%s: %s not found\n", __func__, parent_name);

Re: [PATCH 2/3] pwm: pwm-omap-dmtimer: Fix frequency when using prescaler

[Ladislav Michl]

Dear Claudiu,

On Mon, Jan 22, 2018 at 11:17:08AM +0200, Claudiu Beznea wrote:
> On 17.01.2018 23:47, Ladislav Michl wrote:
> > @@ -334,18 +348,18 @@ static int pwm_omap_dmtimer_probe(struct platform_device *pdev)
> >  
> >  	mutex_init(&omap->mutex);
> >  
> > -	status = pwmchip_add(&omap->chip);
> > -	if (status < 0) {
> > +	ret = pwmchip_add(&omap->chip);
> > +	if (ret < 0) {
> >  		dev_err(&pdev->dev, "failed to register PWM\n");
> > -		omap->pdata->free(omap->dm_timer);
> > -		ret = status;
> > -		goto put;
> > +		goto free;
> >  	}
> >  
> >  	platform_set_drvdata(pdev, omap);
> >From documentation: "of_parse_phandle(): Returns the device_node pointer with refcount
> incremented. Use of_node_put() on it when done."
> In case of success the of_node_put() should also be called as I see.

Based on you previous suggestions found here:
https://patchwork.kernel.org/patch/10140209/
I'd say this fix belongs to patch which introduces of_node_put() in the error
path. I'll then rebase this patches on top of the fix.

Thank you,
	ladis

Re: [PATCH 2/3] pwm: pwm-omap-dmtimer: Fix frequency when using prescaler

[Keerthy]

On Monday 22 January 2018 04:23 PM, Ladislav Michl wrote:
> Dear Claudiu,
> 
> On Mon, Jan 22, 2018 at 11:17:08AM +0200, Claudiu Beznea wrote:
>> On 17.01.2018 23:47, Ladislav Michl wrote:
>>> @@ -334,18 +348,18 @@ static int pwm_omap_dmtimer_probe(struct platform_device *pdev)
>>>  
>>>  	mutex_init(&omap->mutex);
>>>  
>>> -	status = pwmchip_add(&omap->chip);
>>> -	if (status < 0) {
>>> +	ret = pwmchip_add(&omap->chip);
>>> +	if (ret < 0) {
>>>  		dev_err(&pdev->dev, "failed to register PWM\n");
>>> -		omap->pdata->free(omap->dm_timer);
>>> -		ret = status;
>>> -		goto put;
>>> +		goto free;
>>>  	}
>>>  
>>>  	platform_set_drvdata(pdev, omap);
>> >From documentation: "of_parse_phandle(): Returns the device_node pointer with refcount
>> incremented. Use of_node_put() on it when done."
>> In case of success the of_node_put() should also be called as I see.
> 
> Based on you previous suggestions found here:
> https://patchwork.kernel.org/patch/10140209/
> I'd say this fix belongs to patch which introduces of_node_put() in the error
> path. I'll then rebase this patches on top of the fix.

Agreed. I missed the success path as i assumed only error paths needed
te of_node_put(). I will post v8 of this patch alone as other patches
can be left untouched. Hope that is okay.

Regards,
Keerthy
> 
> Thank you,
> 	ladis
> 

Re: [PATCH 2/3] pwm: pwm-omap-dmtimer: Fix frequency when using prescaler

[Keerthy]

On Wednesday 24 January 2018 10:56 AM, Keerthy wrote:
> 
> 
> On Monday 22 January 2018 04:23 PM, Ladislav Michl wrote:
>> Dear Claudiu,
>>
>> On Mon, Jan 22, 2018 at 11:17:08AM +0200, Claudiu Beznea wrote:
>>> On 17.01.2018 23:47, Ladislav Michl wrote:
>>>> @@ -334,18 +348,18 @@ static int pwm_omap_dmtimer_probe(struct platform_device *pdev)
>>>>  
>>>>  	mutex_init(&omap->mutex);
>>>>  
>>>> -	status = pwmchip_add(&omap->chip);
>>>> -	if (status < 0) {
>>>> +	ret = pwmchip_add(&omap->chip);
>>>> +	if (ret < 0) {
>>>>  		dev_err(&pdev->dev, "failed to register PWM\n");
>>>> -		omap->pdata->free(omap->dm_timer);
>>>> -		ret = status;
>>>> -		goto put;
>>>> +		goto free;
>>>>  	}
>>>>  
>>>>  	platform_set_drvdata(pdev, omap);
>>> >From documentation: "of_parse_phandle(): Returns the device_node pointer with refcount
>>> incremented. Use of_node_put() on it when done."
>>> In case of success the of_node_put() should also be called as I see.
>>
>> Based on you previous suggestions found here:
>> https://patchwork.kernel.org/patch/10140209/
>> I'd say this fix belongs to patch which introduces of_node_put() in the error
>> path. I'll then rebase this patches on top of the fix.
> 
> Agreed. I missed the success path as i assumed only error paths needed
> te of_node_put(). I will post v8 of this patch alone as other patches
> can be left untouched. Hope that is okay.

Ladis,

I posted v8 of the above mentioned patch alone.

https://marc.info/?l=linux-omap&m=151677273619960&w=2

Thanks,
Keerthy
> 
> Regards,
> Keerthy
>>
>> Thank you,
>> 	ladis
>>