drivers/pwm/pwm-tiehrpwm.c | 151 ++++++++++++++++++++++++++++++++++++- 1 file changed, 150 insertions(+), 1 deletion(-)
Fixes potential desynchronization of state.enabled in the .probe()
method by suggesting proper handling of hardware state initialization.
Adds considerations for implementing .get_hw_state() to check the
current state of the module by checking physical registers.
Signed-off-by: Rafael V. Volkmer <rafael.v.volkmer@gmail.com>
---
drivers/pwm/pwm-tiehrpwm.c | 151 ++++++++++++++++++++++++++++++++++++-
1 file changed, 150 insertions(+), 1 deletion(-)
diff --git a/drivers/pwm/pwm-tiehrpwm.c b/drivers/pwm/pwm-tiehrpwm.c
index 0125e73b98df..5de213bc3ef5 100644
--- a/drivers/pwm/pwm-tiehrpwm.c
+++ b/drivers/pwm/pwm-tiehrpwm.c
@@ -91,6 +91,20 @@
#define AQCSFRC_CSFA_FRCHIGH BIT(1)
#define AQCSFRC_CSFA_DISSWFRC (BIT(1) | BIT(0))
+#define AQCTLA_CAU_MASK (BIT(5) | BIT(4))
+#define AQCTLA_CAU_SHIFT 4
+#define AQCTLA_CAD_MASK (BIT(9) | BIT(8))
+#define AQCTLA_CAD_SHIFT 8
+
+/* The ePWM hardware encodes compare actions with two bits each:
+ * 00 = Do nothing
+ * 01 = Clear
+ * 10 = Set
+ * 11 = Toggle
+ */
+#define AQ_CLEAR 1
+#define AQ_SET 2
+
#define NUM_PWM_CHANNEL 2 /* EHRPWM channels */
struct ehrpwm_context {
@@ -353,6 +367,118 @@ static int ehrpwm_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
return 0;
}
+static bool ehrpwm_is_enabled(struct pwm_chip *chip)
+{
+ struct ehrpwm_pwm_chip *pc = to_ehrpwm_pwm_chip(chip);
+ bool ret;
+ u16 aqcsfrc_reg;
+ u8 csfa_bits;
+ u16 aqctla_reg;
+
+ if(chip == NULL) {
+ return -EINVAL;
+ }
+
+ aqcsfrc_reg = readw(pc->mmio_base + AQCSFRC);
+ csfa_bits = (u8)(aqcsfrc_reg & AQCSFRC_CSFA_MASK);
+
+ aqctla_reg = readw(pc->mmio_base + AQCTLA);
+
+ ret = (csfa_bits != 0u) ? false :
+ (aqctla_reg == 0u) ? false : true;
+
+ return ret;
+}
+
+static u64 ehrpwm_read_period(struct pwm_chip *chip)
+{
+ struct ehrpwm_pwm_chip *pc = to_ehrpwm_pwm_chip(chip);
+ u64 ret;
+ unsigned long tbclk_rate;
+ u16 tbprd_reg;
+ u64 period_cycles;
+ u64 period_ns;
+
+ if(chip == NULL) {
+ return -EINVAL;
+ }
+
+ tbprd_reg = readw(pc->mmio_base + TBPRD);
+ tbclk_rate = clk_get_rate(pc->tbclk);
+ period_cycles = tbprd_reg + 1u;
+
+ /* period_ns = (period_cycles * 1e9) / tblck_rate */
+ period_ns = DIV_ROUND_UP_ULL(period_cycles * NSEC_PER_SEC, tbclk_rate);
+
+ ret = period_ns;
+ return ret;
+}
+
+static u64 ehrpwm_read_duty_cycle(struct pwm_chip *chip)
+{
+ struct ehrpwm_pwm_chip *pc = to_ehrpwm_pwm_chip(chip);
+ u64 ret;
+ u16 cmpa_reg;
+ u64 duty_cycles;
+ u64 duty_ns;
+ unsigned long tbclk_rate;
+
+ if(chip == NULL) {
+ return -EINVAL;
+ }
+
+ cmpa_reg = readw(pc->mmio_base + CMPA);
+ tbclk_rate = clk_get_rate(pc->tbclk);
+ duty_cycles = cmpa_reg;
+ duty_ns = DIV_ROUND_UP_ULL(duty_cycles * NSEC_PER_SEC, tbclk_rate);
+ ret = duty_ns;
+
+ return ret;
+}
+
+static enum pwm_polarity ehrpwm_read_polarity(struct pwm_chip *chip)
+{
+ struct ehrpwm_pwm_chip *pc = to_ehrpwm_pwm_chip(chip);
+ enum pwm_polarity ret;
+ u16 aqctla_reg;
+ u8 cau_action;
+ u8 cad_action;
+
+ if(chip == NULL) {
+ return -EINVAL;
+ }
+
+ aqctla_reg = readw(pc->mmio_base + AQCTLA);
+ cau_action = (aqctla_reg & AQCTLA_CAU_MASK) >> AQCTLA_CAU_SHIFT;
+ cad_action = (aqctla_reg & AQCTLA_CAD_MASK) >> AQCTLA_CAD_SHIFT;
+
+ if (cau_action == AQ_SET && cad_action == AQ_CLEAR) {
+ ret = PWM_POLARITY_NORMAL;
+ }
+ else if (cau_action == AQ_CLEAR && cad_action == AQ_SET) {
+ ret = PWM_POLARITY_INVERSED;
+ }
+
+ return ret;
+}
+
+static int ehrpwm_get_hw_state(struct pwm_chip *chip, struct pwm_device *pwm,
+ struct pwm_state *state)
+{
+ int ret;
+
+ if(chip == NULL || pwm == NULL || state == NULL){
+ return -EINVAL;
+ }
+
+ state->enabled = ehrpwm_is_enabled(chip);
+ state->period = ehrpwm_read_period(chip);
+ state->duty_cycle = ehrpwm_read_duty_cycle(chip);
+ state->polarity = ehrpwm_read_polarity(chip);
+
+ return ret;
+}
+
static void ehrpwm_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
{
struct ehrpwm_pwm_chip *pc = to_ehrpwm_pwm_chip(chip);
@@ -449,8 +575,10 @@ static int ehrpwm_pwm_probe(struct platform_device *pdev)
{
struct device_node *np = pdev->dev.of_node;
struct ehrpwm_pwm_chip *pc;
+ struct pwm_state state;
struct pwm_chip *chip;
struct clk *clk;
+ bool tbclk_enabled;
int ret;
chip = devm_pwmchip_alloc(&pdev->dev, NUM_PWM_CHANNEL, sizeof(*pc));
@@ -501,10 +629,31 @@ static int ehrpwm_pwm_probe(struct platform_device *pdev)
platform_set_drvdata(pdev, chip);
pm_runtime_enable(&pdev->dev);
+ ehrpwm_get_hw_state(chip, &chip->pwms[0], &state);
+ if(state.enabled == true) {
+ ret = clk_prepare_enable(pc->tbclk);
+ if (ret) {
+ dev_err(&pdev->dev, "clk_prepare_enable() failed: %d\n", ret);
+ goto err_pwmchip_remove;
+ }
+
+ tbclk_enabled = true;
+ ret = pm_runtime_get_sync(&pdev->dev);
+ if(ret < 0) {
+ dev_err(&pdev->dev, "pm_runtime_get_sync() failed: %d\n", ret);
+ clk_disable_unprepare(pc->tbclk);
+ goto err_pwmchip_remove;
+ }
+ }
+
return 0;
+err_pwmchip_remove:
+ pwmchip_remove(chip);
+
err_clk_unprepare:
- clk_unprepare(pc->tbclk);
+ if(tbclk_enabled)
+ clk_unprepare(pc->tbclk);
return ret;
}
--
2.25.1Hello Rafael,
first of all: There are several issues regarding code style, checkpatch
reports
total: 20 errors, 13 warnings, 180 lines checked
Please fix them all for v2 (or mention unfixed ones in the cover text,
ideally with a reasoning). (In case you don't know:
scripts/checkpatch.pl -g @
; for bonus points add --strict.)
On Tue, Feb 04, 2025 at 03:55:40PM -0300, Rafael V. Volkmer wrote:
> Fixes potential desynchronization of state.enabled in the .probe()
> method by suggesting proper handling of hardware state initialization.
> Adds considerations for implementing .get_hw_state() to check the
> current state of the module by checking physical registers.
I don't understand that. What is .get_hw_state()?
Returning here after reading through the complete patch, I suggest
something like:
If the hardware is already active during probe, it's not
asserted that the clock is enabled. So check in
ehrpwm_pwm_probe() if the enable bit is set and call
clk_enable() and pm_runtime_get_sync() if yes.
> Signed-off-by: Rafael V. Volkmer <rafael.v.volkmer@gmail.com>
> ---
> drivers/pwm/pwm-tiehrpwm.c | 151 ++++++++++++++++++++++++++++++++++++-
> 1 file changed, 150 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/pwm/pwm-tiehrpwm.c b/drivers/pwm/pwm-tiehrpwm.c
> index 0125e73b98df..5de213bc3ef5 100644
> --- a/drivers/pwm/pwm-tiehrpwm.c
> +++ b/drivers/pwm/pwm-tiehrpwm.c
> @@ -91,6 +91,20 @@
> #define AQCSFRC_CSFA_FRCHIGH BIT(1)
> #define AQCSFRC_CSFA_DISSWFRC (BIT(1) | BIT(0))
>
> +#define AQCTLA_CAU_MASK (BIT(5) | BIT(4))
> +#define AQCTLA_CAU_SHIFT 4
> +#define AQCTLA_CAD_MASK (BIT(9) | BIT(8))
> +#define AQCTLA_CAD_SHIFT 8
Please use
#define AQCTLA_CAD GENMASK(9, 8)
and for the usage then instead of
cad_action = (aqctla_reg & AQCTLA_CAD_MASK) >> AQCTLA_CAD_SHIFT;
better:
cad_action = FIELD_GET(AQCTLA_CAD, aqctla_reg);
> +
> +/* The ePWM hardware encodes compare actions with two bits each:
> + * 00 = Do nothing
> + * 01 = Clear
> + * 10 = Set
> + * 11 = Toggle
> + */
> +#define AQ_CLEAR 1
> +#define AQ_SET 2
> +
> #define NUM_PWM_CHANNEL 2 /* EHRPWM channels */
>
> struct ehrpwm_context {
> @@ -353,6 +367,118 @@ static int ehrpwm_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
> return 0;
> }
>
> +static bool ehrpwm_is_enabled(struct pwm_chip *chip)
> +{
> + struct ehrpwm_pwm_chip *pc = to_ehrpwm_pwm_chip(chip);
> + bool ret;
> + u16 aqcsfrc_reg;
> + u8 csfa_bits;
> + u16 aqctla_reg;
> +
> + if(chip == NULL) {
> + return -EINVAL;
> + }
return -EINVAL is bogus for a function returning bool. The solution is
easy: chip is never NULL here, so please drop.
> +
> + aqcsfrc_reg = readw(pc->mmio_base + AQCSFRC);
> + csfa_bits = (u8)(aqcsfrc_reg & AQCSFRC_CSFA_MASK);
Single space before = please.
> + aqctla_reg = readw(pc->mmio_base + AQCTLA);
> +
> + ret = (csfa_bits != 0u) ? false :
> + (aqctla_reg == 0u) ? false : true;
I didn't understand what these values mean, but this is unreadable. I
prefer:
if (csfa_bits)
return false;
if (aqctla_reg)
return true;
return false;
Maybe also add code comments about the semantic to the respective
blocks.
> +
> + return ret;
> +}
> +
> +static u64 ehrpwm_read_period(struct pwm_chip *chip)
> +{
> + struct ehrpwm_pwm_chip *pc = to_ehrpwm_pwm_chip(chip);
> + u64 ret;
> + unsigned long tbclk_rate;
> + u16 tbprd_reg;
> + u64 period_cycles;
> + u64 period_ns;
> +
> + if(chip == NULL) {
> + return -EINVAL;
> + }
> +
> + tbprd_reg = readw(pc->mmio_base + TBPRD);
> + tbclk_rate = clk_get_rate(pc->tbclk);
> + period_cycles = tbprd_reg + 1u;
> +
> + /* period_ns = (period_cycles * 1e9) / tblck_rate */
> + period_ns = DIV_ROUND_UP_ULL(period_cycles * NSEC_PER_SEC, tbclk_rate);
> +
> + ret = period_ns;
> + return ret;
return period_ns;
Given that ehrpwm_read_period() and ehrpwm_read_duty_cycle() are called
directly after another, I suggest to rework the code that clk_get_rate()
is only called once. (Maybe even put it into ehrpwm_pwm_chip.)
> +}
> +
> +static u64 ehrpwm_read_duty_cycle(struct pwm_chip *chip)
> +{
> + struct ehrpwm_pwm_chip *pc = to_ehrpwm_pwm_chip(chip);
> + u64 ret;
> + u16 cmpa_reg;
> + u64 duty_cycles;
> + u64 duty_ns;
> + unsigned long tbclk_rate;
> +
> + if(chip == NULL) {
> + return -EINVAL;
> + }
> +
> + cmpa_reg = readw(pc->mmio_base + CMPA);
> + tbclk_rate = clk_get_rate(pc->tbclk);
> + duty_cycles = cmpa_reg;
> + duty_ns = DIV_ROUND_UP_ULL(duty_cycles * NSEC_PER_SEC, tbclk_rate);
> + ret = duty_ns;
> +
> + return ret;
> +}
> +
> +static enum pwm_polarity ehrpwm_read_polarity(struct pwm_chip *chip)
> +{
> + struct ehrpwm_pwm_chip *pc = to_ehrpwm_pwm_chip(chip);
> + enum pwm_polarity ret;
> + u16 aqctla_reg;
> + u8 cau_action;
> + u8 cad_action;
> +
> + if(chip == NULL) {
> + return -EINVAL;
> + }
> +
> + aqctla_reg = readw(pc->mmio_base + AQCTLA);
> + cau_action = (aqctla_reg & AQCTLA_CAU_MASK) >> AQCTLA_CAU_SHIFT;
> + cad_action = (aqctla_reg & AQCTLA_CAD_MASK) >> AQCTLA_CAD_SHIFT;
> +
> + if (cau_action == AQ_SET && cad_action == AQ_CLEAR) {
> + ret = PWM_POLARITY_NORMAL;
> + }
> + else if (cau_action == AQ_CLEAR && cad_action == AQ_SET) {
> + ret = PWM_POLARITY_INVERSED;
> + }
I would expect the compiler to issue a warning here about ret being
uninitialized in some situations.
Not sure checkpatch points that out: } and "else" go on the same line.
> +
> + return ret;
> +}
> +
> +static int ehrpwm_get_hw_state(struct pwm_chip *chip, struct pwm_device *pwm,
> + struct pwm_state *state)
The canonical name for this function is ehrpwm_pwm_get_state. And I
would have expected an assignment to .get_state in ehrpwm_pwm_ops.
> +{
> + int ret;
> +
> + if(chip == NULL || pwm == NULL || state == NULL){
> + return -EINVAL;
> + }
> +
> + state->enabled = ehrpwm_is_enabled(chip);
> + state->period = ehrpwm_read_period(chip);
> + state->duty_cycle = ehrpwm_read_duty_cycle(chip);
> + state->polarity = ehrpwm_read_polarity(chip);
> +
> + return ret;
> +}
> +
> static void ehrpwm_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
> {
> struct ehrpwm_pwm_chip *pc = to_ehrpwm_pwm_chip(chip);
> @@ -449,8 +575,10 @@ static int ehrpwm_pwm_probe(struct platform_device *pdev)
> {
> struct device_node *np = pdev->dev.of_node;
> struct ehrpwm_pwm_chip *pc;
> + struct pwm_state state;
> struct pwm_chip *chip;
> struct clk *clk;
> + bool tbclk_enabled;
> int ret;
>
> chip = devm_pwmchip_alloc(&pdev->dev, NUM_PWM_CHANNEL, sizeof(*pc));
> @@ -501,10 +629,31 @@ static int ehrpwm_pwm_probe(struct platform_device *pdev)
> platform_set_drvdata(pdev, chip);
> pm_runtime_enable(&pdev->dev);
>
> + ehrpwm_get_hw_state(chip, &chip->pwms[0], &state);
> + if(state.enabled == true) {
Given that you only use state.enabled here, maybe shorten that to
if (ehrpwm_is_enabled(...))
> + ret = clk_prepare_enable(pc->tbclk);
While it's a good idea to enable the clock if the PWM is already running
at probe time, this must happen before pwmchip_add().
> + if (ret) {
> + dev_err(&pdev->dev, "clk_prepare_enable() failed: %d\n", ret);
Please use dev_err_probe() here.
> + goto err_pwmchip_remove;
> + }
> +
> + tbclk_enabled = true;
> + ret = pm_runtime_get_sync(&pdev->dev);
> + if(ret < 0) {
> + dev_err(&pdev->dev, "pm_runtime_get_sync() failed: %d\n", ret);
> + clk_disable_unprepare(pc->tbclk);
> + goto err_pwmchip_remove;
> + }
> + }
> +
> return 0;
>
> +err_pwmchip_remove:
> + pwmchip_remove(chip);
> +
> err_clk_unprepare:
> - clk_unprepare(pc->tbclk);
> + if(tbclk_enabled)
> + clk_unprepare(pc->tbclk);
>
> return ret;
> }
Best regards
Uwe
Hi Uwe. Thanks for the feedback! It was very constructive for improving the code! I fixed the points you mentioned and hope you can review v2 of my patch! Changes in v2: - Adjusted the indentation reported by checkpatch.pl; - Added the bitfield header to replace the register bit defines with GEN_MASK and FIELD_GET; - Concentrated the use of clk_get_rate() only once, inside ehrpwm_get_state(); - Added the ehrpwm_get_state() function as part of the driver's fops, directly linked to .get_state; - Now, the activation of clk_prepare_enable() when the pwm is already running is done before pwmchip_add(), and runtime_get_sync() after; - Added comments for each new function; - Added comments in specific areas of register manipulation, in order to make understanding how they work more intuitive; - Removed unnecessary null pointer checks in functions and also improper returns; - I improved the commit message, as suggested. Rafael V. Volkmer (1): pwm: tiehrpwm: ensures that state.enabled is synchronized in .probe() drivers/pwm/pwm-tiehrpwm.c | 202 ++++++++++++++++++++++++++++++++++++- 1 file changed, 201 insertions(+), 1 deletion(-) -- 2.25.1
If the hardware is already active during probe, it's not asserted
that the clock is enabled. To address this, added ehrpwm_get_state()
to verify the enable bit and update the pwm_state struct with the
correct hardware parameters.
In ehrpwm_pwm_probe(), the function checks if the enable bit is set
and calls clk_enable() and pm_runtime_get_sync() if needed.
Additionally, ehrpwm_get_state() was integrated as .get_state in
the ehrpwm_pwm_ops struct, incorporating it into the driver's pwm_ops
for consistent state management.
Signed-off-by: Rafael V. Volkmer <rafael.v.volkmer@gmail.com>
---
drivers/pwm/pwm-tiehrpwm.c | 202 ++++++++++++++++++++++++++++++++++++-
1 file changed, 201 insertions(+), 1 deletion(-)
diff --git a/drivers/pwm/pwm-tiehrpwm.c b/drivers/pwm/pwm-tiehrpwm.c
index 0125e73b98df..cbddbbd48a75 100644
--- a/drivers/pwm/pwm-tiehrpwm.c
+++ b/drivers/pwm/pwm-tiehrpwm.c
@@ -13,6 +13,7 @@
#include <linux/clk.h>
#include <linux/pm_runtime.h>
#include <linux/of.h>
+#include <linux/bitfield.h>
/* EHRPWM registers and bits definitions */
@@ -91,6 +92,19 @@
#define AQCSFRC_CSFA_FRCHIGH BIT(1)
#define AQCSFRC_CSFA_DISSWFRC (BIT(1) | BIT(0))
+#define AQCTLA_CAU GENMASK(5, 4)
+#define AQCTLA_CAD GENMASK(9, 8)
+
+/**
+ * The ePWM hardware encodes compare actions with two bits each:
+ * 00 = Do nothing
+ * 01 = Clear
+ * 10 = Set
+ * 11 = Toggle
+ */
+#define AQ_CLEAR 1
+#define AQ_SET 2
+
#define NUM_PWM_CHANNEL 2 /* EHRPWM channels */
struct ehrpwm_context {
@@ -353,6 +367,165 @@ static int ehrpwm_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
return 0;
}
+/**
+ * ehrpwm_is_enabled - Checks if the eHRPWM channel is enabled
+ * @chip: pointer to the PWM chip structure
+ *
+ * @return: true if the channel is enabled, false otherwise
+ */
+static bool ehrpwm_is_enabled(struct pwm_chip *chip)
+{
+ bool ret = false;
+
+ struct ehrpwm_pwm_chip *pc = to_ehrpwm_pwm_chip(chip);
+
+ u16 aqcsfrc_reg = 0u;
+ u8 csfa_bits = 0u;
+ u16 aqctla_reg = 0u;
+
+ aqcsfrc_reg = readw(pc->mmio_base + AQCSFRC);
+ csfa_bits = (u8)(aqcsfrc_reg & AQCSFRC_CSFA_MASK);
+ aqctla_reg = readw(pc->mmio_base + AQCTLA);
+ /*
+ * If the CSFA value is non-zero,
+ * it means channel A is being forced via software
+ * (override), so we consider the PWM "disabled".
+ */
+ if (csfa_bits)
+ ret = false;
+
+ /*
+ * If the control register (AQCTLA) is configured
+ * (non-zero), it means channel A has qualified actions
+ * and is therefore enabled to generate PWM.
+ */
+ if (aqctla_reg)
+ ret = true;
+
+ return ret;
+}
+
+/**
+ * ehrpwm_read_period - Reads the period of the eHRPWM channel (in ns)
+ * @chip: pointer to the PWM chip structure
+ * @tbclk_rate: time-base clock rate in Hz
+ *
+ * @return: period in nanoseconds
+ */
+static u64 ehrpwm_read_period(struct pwm_chip *chip, unsigned long tbclk_rate)
+{
+ struct ehrpwm_pwm_chip *pc = to_ehrpwm_pwm_chip(chip);
+
+ u16 tbprd_reg = 0u;
+ u64 period_cycles = 0u;
+ u64 period_ns = 0u;
+
+ tbprd_reg = readw(pc->mmio_base + TBPRD);
+ period_cycles = tbprd_reg + 1u;
+
+ /*
+ * period_ns = (period_cycles * 1e9) / tbclk_rate
+ * Using DIV_ROUND_UP_ULL to avoid floating-point operations.
+ */
+ period_ns = DIV_ROUND_UP_ULL(period_cycles * NSEC_PER_SEC, tbclk_rate);
+
+ return period_ns;
+}
+
+/**
+ * ehrpwm_read_duty_cycle - Reads the duty cycle of the eHRPWM channel (in ns)
+ * @chip: pointer to the PWM chip structure
+ * @tbclk_rate: time-base clock rate in Hz
+ *
+ * @return: duty cycle in nanoseconds
+ */
+static u64 ehrpwm_read_duty_cycle(struct pwm_chip *chip, unsigned long tbclk_rate)
+{
+ struct ehrpwm_pwm_chip *pc = to_ehrpwm_pwm_chip(chip);
+
+ u16 cmpa_reg = 0u;
+ u64 duty_cycles = 0u;
+ u64 duty_ns = 0u;
+
+ cmpa_reg = readw(pc->mmio_base + CMPA);
+
+ duty_cycles = cmpa_reg;
+
+ /*
+ * duty_ns = (duty_cycles * 1e9) / tbclk_rate
+ * Using DIV_ROUND_UP_ULL to avoid floating-point operations.
+ */
+ duty_ns = DIV_ROUND_UP_ULL(duty_cycles * NSEC_PER_SEC, tbclk_rate);
+
+ return duty_ns;
+}
+
+/**
+ * ehrpwm_read_polarity - Reads the polarity of the eHRPWM channel
+ * @chip: pointer to the PWM chip structure
+ *
+ * @return: the polarity of the PWM (PWM_POLARITY_NORMAL or PWM_POLARITY_INVERSED)
+ */
+static enum pwm_polarity ehrpwm_read_polarity(struct pwm_chip *chip)
+{
+ enum pwm_polarity ret = PWM_POLARITY_NORMAL;
+
+ struct ehrpwm_pwm_chip *pc = to_ehrpwm_pwm_chip(chip);
+
+ u16 aqctla_reg = 0u;
+ u8 cau_action = 0u;
+ u8 cad_action = 0u;
+
+ aqctla_reg = readw(pc->mmio_base + AQCTLA);
+ cau_action = FIELD_GET(AQCTLA_CAU, aqctla_reg);
+ cad_action = FIELD_GET(AQCTLA_CAD, aqctla_reg);
+
+ /*
+ * Evaluate the actions to determine the PWM polarity:
+ * - If an up-count event sets the output (AQ_SET) and a down-count
+ * event clears it (AQ_CLEAR), then polarity is NORMAL.
+ * - If an up-count event clears the output (AQ_CLEAR) and a down-count
+ * event sets it (AQ_SET), then polarity is INVERSED.
+ */
+ if (cau_action == AQ_SET && cad_action == AQ_CLEAR)
+ ret = PWM_POLARITY_NORMAL;
+
+ else if (cau_action == AQ_CLEAR && cad_action == AQ_SET)
+ ret = PWM_POLARITY_INVERSED;
+
+ return ret;
+}
+
+/**
+ * ehrpwm_get_state - Retrieves the current state of the eHRPWM channel
+ * @chip: pointer to the PWM chip structure
+ * @pwm: pointer to the PWM device structure
+ * @state: pointer to the pwm_state structure to be filled
+ *
+ * @return: 0 on success or a negative error code on failure
+ */
+static int ehrpwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
+ struct pwm_state *state)
+{
+ int ret = 0u;
+ struct ehrpwm_pwm_chip *pc = to_ehrpwm_pwm_chip(chip);
+ unsigned long tbclk_rate = 0u;
+
+ if (chip == NULL || pwm == NULL || state == NULL)
+ return -EINVAL;
+
+ tbclk_rate = clk_get_rate(pc->tbclk);
+ if (tbclk_rate <= 0)
+ return -EINVAL;
+
+ state->enabled = ehrpwm_is_enabled(chip);
+ state->period = ehrpwm_read_period(chip, tbclk_rate);
+ state->duty_cycle = ehrpwm_read_duty_cycle(chip, tbclk_rate);
+ state->polarity = ehrpwm_read_polarity(chip);
+
+ return ret;
+}
+
static void ehrpwm_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
{
struct ehrpwm_pwm_chip *pc = to_ehrpwm_pwm_chip(chip);
@@ -436,6 +609,7 @@ static int ehrpwm_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
static const struct pwm_ops ehrpwm_pwm_ops = {
.free = ehrpwm_pwm_free,
.apply = ehrpwm_pwm_apply,
+ .get_state = ehrpwm_get_state,
};
static const struct of_device_id ehrpwm_of_match[] = {
@@ -449,8 +623,10 @@ static int ehrpwm_pwm_probe(struct platform_device *pdev)
{
struct device_node *np = pdev->dev.of_node;
struct ehrpwm_pwm_chip *pc;
+ struct pwm_state state;
struct pwm_chip *chip;
struct clk *clk;
+ bool tbclk_enabled;
int ret;
chip = devm_pwmchip_alloc(&pdev->dev, NUM_PWM_CHANNEL, sizeof(*pc));
@@ -492,6 +668,18 @@ static int ehrpwm_pwm_probe(struct platform_device *pdev)
return ret;
}
+ ehrpwm_get_state(chip, &chip->pwms[0], &state);
+
+ if (state.enabled == true) {
+ ret = clk_prepare_enable(pc->tbclk);
+ if (ret) {
+ dev_err_probe(&pdev->dev, ret, "clk_prepare_enable() failed");
+ goto err_pwmchip_remove;
+ }
+
+ tbclk_enabled = true;
+ }
+
ret = pwmchip_add(chip);
if (ret < 0) {
dev_err(&pdev->dev, "pwmchip_add() failed: %d\n", ret);
@@ -501,10 +689,22 @@ static int ehrpwm_pwm_probe(struct platform_device *pdev)
platform_set_drvdata(pdev, chip);
pm_runtime_enable(&pdev->dev);
+ if (state.enabled == true) {
+ ret = pm_runtime_get_sync(&pdev->dev);
+ if (ret < 0) {
+ dev_err_probe(&pdev->dev, ret, "pm_runtime_get_sync() failed");
+ clk_disable_unprepare(pc->tbclk);
+ goto err_pwmchip_remove;
+ }
+ }
+
return 0;
+err_pwmchip_remove:
+ pwmchip_remove(chip);
err_clk_unprepare:
- clk_unprepare(pc->tbclk);
+ if (tbclk_enabled)
+ clk_unprepare(pc->tbclk);
return ret;
}
--
2.25.1Hi Rafael,
kernel test robot noticed the following build warnings:
[auto build test WARNING on linus/master]
[also build test WARNING on v6.15-rc2 next-20250417]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Rafael-V-Volkmer/pwm-tiehrpwm-ensures-that-state-enabled-is-synchronized-in-probe/20250420-075200
base: linus/master
patch link: https://lore.kernel.org/r/20250206031852.64853-1-rafael.v.volkmer%40gmail.com
patch subject: [PATCH v2 1/1] pwm: tiehrpwm: ensures that state.enabled is synchronized in .probe()
config: um-randconfig-002-20250420 (https://download.01.org/0day-ci/archive/20250420/202504201347.OiWigSUq-lkp@intel.com/config)
compiler: clang version 21.0.0git (https://github.com/llvm/llvm-project f819f46284f2a79790038e1f6649172789734ae8)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250420/202504201347.OiWigSUq-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202504201347.OiWigSUq-lkp@intel.com/
All warnings (new ones prefixed by >>):
In file included from drivers/pwm/pwm-tiehrpwm.c:11:
In file included from include/linux/io.h:12:
In file included from arch/um/include/asm/io.h:24:
include/asm-generic/io.h:549:31: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
549 | val = __raw_readb(PCI_IOBASE + addr);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:567:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
567 | val = __le16_to_cpu((__le16 __force)__raw_readw(PCI_IOBASE + addr));
| ~~~~~~~~~~ ^
include/uapi/linux/byteorder/little_endian.h:37:51: note: expanded from macro '__le16_to_cpu'
37 | #define __le16_to_cpu(x) ((__force __u16)(__le16)(x))
| ^
In file included from drivers/pwm/pwm-tiehrpwm.c:11:
In file included from include/linux/io.h:12:
In file included from arch/um/include/asm/io.h:24:
include/asm-generic/io.h:585:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
585 | val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr));
| ~~~~~~~~~~ ^
include/uapi/linux/byteorder/little_endian.h:35:51: note: expanded from macro '__le32_to_cpu'
35 | #define __le32_to_cpu(x) ((__force __u32)(__le32)(x))
| ^
In file included from drivers/pwm/pwm-tiehrpwm.c:11:
In file included from include/linux/io.h:12:
In file included from arch/um/include/asm/io.h:24:
include/asm-generic/io.h:601:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
601 | __raw_writeb(value, PCI_IOBASE + addr);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:616:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
616 | __raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:631:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
631 | __raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE + addr);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:724:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
724 | readsb(PCI_IOBASE + addr, buffer, count);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:737:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
737 | readsw(PCI_IOBASE + addr, buffer, count);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:750:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
750 | readsl(PCI_IOBASE + addr, buffer, count);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:764:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
764 | writesb(PCI_IOBASE + addr, buffer, count);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:778:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
778 | writesw(PCI_IOBASE + addr, buffer, count);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:792:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
792 | writesl(PCI_IOBASE + addr, buffer, count);
| ~~~~~~~~~~ ^
>> drivers/pwm/pwm-tiehrpwm.c:675:7: warning: variable 'tbclk_enabled' is used uninitialized whenever 'if' condition is true [-Wsometimes-uninitialized]
675 | if (ret) {
| ^~~
drivers/pwm/pwm-tiehrpwm.c:706:6: note: uninitialized use occurs here
706 | if (tbclk_enabled)
| ^~~~~~~~~~~~~
drivers/pwm/pwm-tiehrpwm.c:675:3: note: remove the 'if' if its condition is always false
675 | if (ret) {
| ^~~~~~~~~~
676 | dev_err_probe(&pdev->dev, ret, "clk_prepare_enable() failed");
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
677 | goto err_pwmchip_remove;
| ~~~~~~~~~~~~~~~~~~~~~~~~
678 | }
| ~
drivers/pwm/pwm-tiehrpwm.c:629:20: note: initialize the variable 'tbclk_enabled' to silence this warning
629 | bool tbclk_enabled;
| ^
| = 0
13 warnings generated.
vim +675 drivers/pwm/pwm-tiehrpwm.c
621
622 static int ehrpwm_pwm_probe(struct platform_device *pdev)
623 {
624 struct device_node *np = pdev->dev.of_node;
625 struct ehrpwm_pwm_chip *pc;
626 struct pwm_state state;
627 struct pwm_chip *chip;
628 struct clk *clk;
629 bool tbclk_enabled;
630 int ret;
631
632 chip = devm_pwmchip_alloc(&pdev->dev, NUM_PWM_CHANNEL, sizeof(*pc));
633 if (IS_ERR(chip))
634 return PTR_ERR(chip);
635 pc = to_ehrpwm_pwm_chip(chip);
636
637 clk = devm_clk_get(&pdev->dev, "fck");
638 if (IS_ERR(clk)) {
639 if (of_device_is_compatible(np, "ti,am33xx-ecap")) {
640 dev_warn(&pdev->dev, "Binding is obsolete.\n");
641 clk = devm_clk_get(pdev->dev.parent, "fck");
642 }
643 }
644
645 if (IS_ERR(clk))
646 return dev_err_probe(&pdev->dev, PTR_ERR(clk), "Failed to get fck\n");
647
648 pc->clk_rate = clk_get_rate(clk);
649 if (!pc->clk_rate) {
650 dev_err(&pdev->dev, "failed to get clock rate\n");
651 return -EINVAL;
652 }
653
654 chip->ops = &ehrpwm_pwm_ops;
655
656 pc->mmio_base = devm_platform_ioremap_resource(pdev, 0);
657 if (IS_ERR(pc->mmio_base))
658 return PTR_ERR(pc->mmio_base);
659
660 /* Acquire tbclk for Time Base EHRPWM submodule */
661 pc->tbclk = devm_clk_get(&pdev->dev, "tbclk");
662 if (IS_ERR(pc->tbclk))
663 return dev_err_probe(&pdev->dev, PTR_ERR(pc->tbclk), "Failed to get tbclk\n");
664
665 ret = clk_prepare(pc->tbclk);
666 if (ret < 0) {
667 dev_err(&pdev->dev, "clk_prepare() failed: %d\n", ret);
668 return ret;
669 }
670
671 ehrpwm_get_state(chip, &chip->pwms[0], &state);
672
673 if (state.enabled == true) {
674 ret = clk_prepare_enable(pc->tbclk);
> 675 if (ret) {
676 dev_err_probe(&pdev->dev, ret, "clk_prepare_enable() failed");
677 goto err_pwmchip_remove;
678 }
679
680 tbclk_enabled = true;
681 }
682
683 ret = pwmchip_add(chip);
684 if (ret < 0) {
685 dev_err(&pdev->dev, "pwmchip_add() failed: %d\n", ret);
686 goto err_clk_unprepare;
687 }
688
689 platform_set_drvdata(pdev, chip);
690 pm_runtime_enable(&pdev->dev);
691
692 if (state.enabled == true) {
693 ret = pm_runtime_get_sync(&pdev->dev);
694 if (ret < 0) {
695 dev_err_probe(&pdev->dev, ret, "pm_runtime_get_sync() failed");
696 clk_disable_unprepare(pc->tbclk);
697 goto err_pwmchip_remove;
698 }
699 }
700
701 return 0;
702
703 err_pwmchip_remove:
704 pwmchip_remove(chip);
705 err_clk_unprepare:
706 if (tbclk_enabled)
707 clk_unprepare(pc->tbclk);
708
709 return ret;
710 }
711
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
Hi Rafael,
kernel test robot noticed the following build warnings:
[auto build test WARNING on linus/master]
[also build test WARNING on v6.15-rc2 next-20250417]
[cannot apply to thierry-reding-pwm/for-next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Rafael-V-Volkmer/pwm-tiehrpwm-ensures-that-state-enabled-is-synchronized-in-probe/20250420-075200
base: linus/master
patch link: https://lore.kernel.org/r/20250206031852.64853-1-rafael.v.volkmer%40gmail.com
patch subject: [PATCH v2 1/1] pwm: tiehrpwm: ensures that state.enabled is synchronized in .probe()
config: arc-randconfig-001-20250420 (https://download.01.org/0day-ci/archive/20250420/202504201050.o3m04RP6-lkp@intel.com/config)
compiler: arc-linux-gcc (GCC) 12.4.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250420/202504201050.o3m04RP6-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202504201050.o3m04RP6-lkp@intel.com/
All warnings (new ones prefixed by >>):
>> drivers/pwm/pwm-tiehrpwm.c:105: warning: expecting prototype for The ePWM hardware encodes compare actions with two bits each(). Prototype was for AQ_CLEAR() instead
vim +105 drivers/pwm/pwm-tiehrpwm.c
97
98 /**
99 * The ePWM hardware encodes compare actions with two bits each:
100 * 00 = Do nothing
101 * 01 = Clear
102 * 10 = Set
103 * 11 = Toggle
104 */
> 105 #define AQ_CLEAR 1
106 #define AQ_SET 2
107
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
Hello Rafael, On Thu, Feb 06, 2025 at 12:18:52AM -0300, Rafael V. Volkmer wrote: > If the hardware is already active during probe, it's not asserted > that the clock is enabled. To address this, added ehrpwm_get_state() > to verify the enable bit and update the pwm_state struct with the > correct hardware parameters. > > In ehrpwm_pwm_probe(), the function checks if the enable bit is set > and calls clk_enable() and pm_runtime_get_sync() if needed. > > Additionally, ehrpwm_get_state() was integrated as .get_state in > the ehrpwm_pwm_ops struct, incorporating it into the driver's pwm_ops > for consistent state management. No in depth review here. One thing I just noticed when skimming your patch: There are still whitespace style issues (that checkpatch might not notice), please only use only a single space after =. Also please split the patch in two: One to introduce .get_state() and the other to fix clock enable accounting. I would also consider inlining ehrpwm_read_period(), ehrpwm_read_duty_cycle() and ehrpwm_read_polarity() into ehrpwm_get_state() to not have to jump around to understand the register usage. For a more detailed review I have to ask for your patience. There are still some more driver reviews in my list, but I come to your driver eventually. Best regards Uwe
Simplify bit manipulation by replacing manual BIT(x) definitions with
GENMASK() and FIELD_PREP() from <linux/bitfield.h>. This improves
readability, consistency, and aligns with modern kernel practices
while preserving existing functionality.
Additionally, update set_prescale_div() to use FIELD_PREP() for
TBCTL_CLKDIV_MASK and TBCTL_HSPCLKDIV_MASK instead of manually
shifting bits. This makes the code more maintainable and avoids
potential errors in bit field assignments.
Signed-off-by: Rafael V. Volkmer <rafael.v.volkmer@gmail.com>
---
drivers/pwm/pwm-tiehrpwm.c | 222 ++++++++++++++++++++++++-------------
1 file changed, 142 insertions(+), 80 deletions(-)
diff --git a/drivers/pwm/pwm-tiehrpwm.c b/drivers/pwm/pwm-tiehrpwm.c
index 0125e73b98df..50516f46ab04 100644
--- a/drivers/pwm/pwm-tiehrpwm.c
+++ b/drivers/pwm/pwm-tiehrpwm.c
@@ -13,85 +13,147 @@
#include <linux/clk.h>
#include <linux/pm_runtime.h>
#include <linux/of.h>
+#include <linux/bitfield.h>
-/* EHRPWM registers and bits definitions */
-
-/* Time base module registers */
-#define TBCTL 0x00
-#define TBPRD 0x0A
-
-#define TBCTL_PRDLD_MASK BIT(3)
-#define TBCTL_PRDLD_SHDW 0
-#define TBCTL_PRDLD_IMDT BIT(3)
-#define TBCTL_CLKDIV_MASK (BIT(12) | BIT(11) | BIT(10) | BIT(9) | \
- BIT(8) | BIT(7))
-#define TBCTL_CTRMODE_MASK (BIT(1) | BIT(0))
-#define TBCTL_CTRMODE_UP 0
-#define TBCTL_CTRMODE_DOWN BIT(0)
-#define TBCTL_CTRMODE_UPDOWN BIT(1)
-#define TBCTL_CTRMODE_FREEZE (BIT(1) | BIT(0))
-
-#define TBCTL_HSPCLKDIV_SHIFT 7
-#define TBCTL_CLKDIV_SHIFT 10
-
-#define CLKDIV_MAX 7
-#define HSPCLKDIV_MAX 7
-#define PERIOD_MAX 0xFFFF
-
-/* compare module registers */
-#define CMPA 0x12
-#define CMPB 0x14
-
-/* Action qualifier module registers */
-#define AQCTLA 0x16
-#define AQCTLB 0x18
-#define AQSFRC 0x1A
-#define AQCSFRC 0x1C
-
-#define AQCTL_CBU_MASK (BIT(9) | BIT(8))
-#define AQCTL_CBU_FRCLOW BIT(8)
-#define AQCTL_CBU_FRCHIGH BIT(9)
-#define AQCTL_CBU_FRCTOGGLE (BIT(9) | BIT(8))
-#define AQCTL_CAU_MASK (BIT(5) | BIT(4))
-#define AQCTL_CAU_FRCLOW BIT(4)
-#define AQCTL_CAU_FRCHIGH BIT(5)
-#define AQCTL_CAU_FRCTOGGLE (BIT(5) | BIT(4))
-#define AQCTL_PRD_MASK (BIT(3) | BIT(2))
-#define AQCTL_PRD_FRCLOW BIT(2)
-#define AQCTL_PRD_FRCHIGH BIT(3)
-#define AQCTL_PRD_FRCTOGGLE (BIT(3) | BIT(2))
-#define AQCTL_ZRO_MASK (BIT(1) | BIT(0))
-#define AQCTL_ZRO_FRCLOW BIT(0)
-#define AQCTL_ZRO_FRCHIGH BIT(1)
-#define AQCTL_ZRO_FRCTOGGLE (BIT(1) | BIT(0))
-
-#define AQCTL_CHANA_POLNORMAL (AQCTL_CAU_FRCLOW | AQCTL_PRD_FRCHIGH | \
- AQCTL_ZRO_FRCHIGH)
-#define AQCTL_CHANA_POLINVERSED (AQCTL_CAU_FRCHIGH | AQCTL_PRD_FRCLOW | \
- AQCTL_ZRO_FRCLOW)
-#define AQCTL_CHANB_POLNORMAL (AQCTL_CBU_FRCLOW | AQCTL_PRD_FRCHIGH | \
- AQCTL_ZRO_FRCHIGH)
-#define AQCTL_CHANB_POLINVERSED (AQCTL_CBU_FRCHIGH | AQCTL_PRD_FRCLOW | \
- AQCTL_ZRO_FRCLOW)
-
-#define AQSFRC_RLDCSF_MASK (BIT(7) | BIT(6))
-#define AQSFRC_RLDCSF_ZRO 0
-#define AQSFRC_RLDCSF_PRD BIT(6)
-#define AQSFRC_RLDCSF_ZROPRD BIT(7)
-#define AQSFRC_RLDCSF_IMDT (BIT(7) | BIT(6))
-
-#define AQCSFRC_CSFB_MASK (BIT(3) | BIT(2))
-#define AQCSFRC_CSFB_FRCDIS 0
-#define AQCSFRC_CSFB_FRCLOW BIT(2)
-#define AQCSFRC_CSFB_FRCHIGH BIT(3)
-#define AQCSFRC_CSFB_DISSWFRC (BIT(3) | BIT(2))
-#define AQCSFRC_CSFA_MASK (BIT(1) | BIT(0))
-#define AQCSFRC_CSFA_FRCDIS 0
-#define AQCSFRC_CSFA_FRCLOW BIT(0)
-#define AQCSFRC_CSFA_FRCHIGH BIT(1)
-#define AQCSFRC_CSFA_DISSWFRC (BIT(1) | BIT(0))
-
-#define NUM_PWM_CHANNEL 2 /* EHRPWM channels */
+/*
+ * --------------------------------------
+ * Time base module registers
+ * --------------------------------------
+ */
+#define TBCTL 0x00
+#define TBPRD 0x0A
+
+/* TBCTL: CTRMODE field (bits [1:0]) */
+#define TBCTL_CTRMODE_MASK GENMASK(1, 0)
+
+/* The possible values for that field */
+#define TBCTL_CTRMODE_UP FIELD_PREP(TBCTL_CTRMODE_MASK, 0U)
+#define TBCTL_CTRMODE_DOWN FIELD_PREP(TBCTL_CTRMODE_MASK, 1U)
+#define TBCTL_CTRMODE_UPDOWN FIELD_PREP(TBCTL_CTRMODE_MASK, 2U)
+#define TBCTL_CTRMODE_FREEZE FIELD_PREP(TBCTL_CTRMODE_MASK, 3U)
+
+/* TBCTL: PRDLD bit (bit [3]) */
+#define TBCTL_PRDLD_MASK GENMASK(3, 3)
+
+/* Possible values for PRDLD */
+#define TBCTL_PRDLD_SHDW FIELD_PREP(TBCTL_PRDLD_MASK, 0U) /* 0 => shadow */
+#define TBCTL_PRDLD_IMDT FIELD_PREP(TBCTL_PRDLD_MASK, 1U) /* 1 => immediate */
+
+/* TBCTL: Combined bits [12:7] actually split into two subfields:
+ * - TBCTL_HSPCLKDIV => bits [9:7]
+ * - TBCTL_CLKDIV => bits [12:10]
+ */
+#define TBCTL_HSPCLKDIV_MASK GENMASK(9, 7)
+#define TBCTL_CLKDIV_MASK GENMASK(12, 10)
+
+/* Max values if needed: */
+#define CLKDIV_MAX 7U
+#define HSPCLKDIV_MAX 7U
+#define PERIOD_MAX 0xFFFF
+
+/*
+ * --------------------------------------
+ * Compare module registers
+ * --------------------------------------
+ */
+#define CMPA 0x12
+#define CMPB 0x14
+
+/*
+ * --------------------------------------
+ * Action qualifier (AQ) module registers
+ * --------------------------------------
+ */
+#define AQCTLA 0x16
+#define AQCTLB 0x18
+#define AQSFRC 0x1A
+#define AQCSFRC 0x1C
+
+/*
+ * AQCTLA and AQCTLB share similar 2-bit fields for
+ * various events (ZRO, PRD, CAU, CBU, etc.).
+ *
+ * For instance, bits [1:0] => ZRO,
+ * bits [3:2] => PRD,
+ * bits [5:4] => CAU,
+ * bits [9:8] => CBU, etc.
+ */
+#define AQCTL_ZRO_MASK GENMASK(1, 0)
+#define AQCTL_PRD_MASK GENMASK(3, 2)
+#define AQCTL_CAU_MASK GENMASK(5, 4)
+#define AQCTL_CBU_MASK GENMASK(9, 8)
+
+/*
+ * FORCE LOW => 0b01
+ * FORCE HIGH => 0b01
+ * FORCE TOGGLE => 0b01
+ */
+#define AQCTL_FRCLOW 1U
+#define AQCTL_FRCHIGH 2U
+#define AQCTL_FRCTOGGLE 3U
+
+/* These values in CAU: */
+#define AQCTL_CAU_FRCLOW FIELD_PREP(AQCTL_CAU_MASK, AQCTL_FRCLOW)
+#define AQCTL_CAU_FRCHIGH FIELD_PREP(AQCTL_CAU_MASK, AQCTL_FRCHIGH)
+#define AQCTL_CAU_FRCTOGGLE FIELD_PREP(AQCTL_CAU_MASK, AQCTL_FRCTOGGLE)
+
+/* These values in PRD: */
+#define AQCTL_PRD_FRCLOW FIELD_PREP(AQCTL_PRD_MASK, AQCTL_FRCLOW)
+#define AQCTL_PRD_FRCHIGH FIELD_PREP(AQCTL_PRD_MASK, AQCTL_FRCHIGH)
+#define AQCTL_PRD_FRCTOGGLE FIELD_PREP(AQCTL_PRD_MASK, AQCTL_FRCTOGGLE)
+
+/* These values in ZRO: */
+#define AQCTL_ZRO_FRCLOW FIELD_PREP(AQCTL_ZRO_MASK, AQCTL_FRCLOW)
+#define AQCTL_ZRO_FRCHIGH FIELD_PREP(AQCTL_ZRO_MASK, AQCTL_FRCHIGH)
+#define AQCTL_ZRO_FRCTOGGLE FIELD_PREP(AQCTL_ZRO_MASK, AQCTL_FRCTOGGLE)
+
+/* These values in CBU: */
+#define AQCTL_CBU_FRCLOW FIELD_PREP(AQCTL_CBU_MASK, AQCTL_FRCLOW)
+#define AQCTL_CBU_FRCHIGH FIELD_PREP(AQCTL_CBU_MASK, AQCTL_FRCHIGH)
+#define AQCTL_CBU_FRCTOGGLE FIELD_PREP(AQCTL_CBU_MASK, AQCTL_FRCTOGGLE)
+
+/* Predefined combinations for channel polarity */
+#define AQCTL_CHANA_POLNORMAL \
+ (AQCTL_CAU_FRCLOW | AQCTL_PRD_FRCHIGH | AQCTL_ZRO_FRCHIGH)
+#define AQCTL_CHANA_POLINVERSED \
+ (AQCTL_CAU_FRCHIGH | AQCTL_PRD_FRCLOW | AQCTL_ZRO_FRCLOW)
+#define AQCTL_CHANB_POLNORMAL \
+ (AQCTL_CBU_FRCLOW | AQCTL_PRD_FRCHIGH | AQCTL_ZRO_FRCHIGH)
+#define AQCTL_CHANB_POLINVERSED \
+ (AQCTL_CBU_FRCHIGH | AQCTL_PRD_FRCLOW | AQCTL_ZRO_FRCLOW)
+
+/* AQSFRC: RLDCSF => bits [7:6] */
+#define AQSFRC_RLDCSF_MASK GENMASK(7, 6)
+
+#define AQSFRC_RLDCSF_ZRO FIELD_PREP(AQSFRC_RLDCSF_MASK, 0U)
+#define AQSFRC_RLDCSF_PRD FIELD_PREP(AQSFRC_RLDCSF_MASK, 1U)
+#define AQSFRC_RLDCSF_ZROPRD FIELD_PREP(AQSFRC_RLDCSF_MASK, 2U)
+#define AQSFRC_RLDCSF_IMDT FIELD_PREP(AQSFRC_RLDCSF_MASK, 3U)
+
+/* AQCSFRC: CSFB => bits [3:2], CSFA => bits [1:0] */
+#define AQCSFRC_CSFB_MASK GENMASK(3, 2)
+#define AQCSFRC_CSFA_MASK GENMASK(1, 0)
+
+/* The possible 2-bit values for CSFB or CSFA fields */
+#define AQCSFRC_FRCDIS 0U
+#define AQCSFRC_FRCLOW 1U
+#define AQCSFRC_FRCHIGH 2U
+#define AQCSFRC_DISSWFRC 3U
+
+/* These values in CSFB: */
+#define AQCSFRC_CSFB_FRCDIS FIELD_PREP(AQCSFRC_CSFB_MASK, AQCSFRC_FRCDIS)
+#define AQCSFRC_CSFB_FRCLOW FIELD_PREP(AQCSFRC_CSFB_MASK, AQCSFRC_FRCLOW)
+#define AQCSFRC_CSFB_FRCHIGH FIELD_PREP(AQCSFRC_CSFB_MASK, AQCSFRC_FRCHIGH)
+#define AQCSFRC_CSFB_DISSWFRC FIELD_PREP(AQCSFRC_CSFB_MASK, AQCSFRC_DISSWFRC)
+
+/* These values in CSFA: */
+#define AQCSFRC_CSFA_FRCDIS FIELD_PREP(AQCSFRC_CSFA_MASK, AQCSFRC_FRCDIS)
+#define AQCSFRC_CSFA_FRCLOW FIELD_PREP(AQCSFRC_CSFA_MASK, AQCSFRC_FRCLOW)
+#define AQCSFRC_CSFA_FRCHIGH FIELD_PREP(AQCSFRC_CSFA_MASK, AQCSFRC_FRCHIGH)
+#define AQCSFRC_CSFA_DISSWFRC FIELD_PREP(AQCSFRC_CSFA_MASK, AQCSFRC_DISSWFRC)
+
+/* EHRPWM channels */
+#define NUM_PWM_CHANNEL 2U
struct ehrpwm_context {
u16 tbctl;
@@ -167,8 +229,8 @@ static int set_prescale_div(unsigned long rqst_prescaler, u16 *prescale_div,
*prescale_div = (1 << clkdiv) *
(hspclkdiv ? (hspclkdiv * 2) : 1);
if (*prescale_div > rqst_prescaler) {
- *tb_clk_div = (clkdiv << TBCTL_CLKDIV_SHIFT) |
- (hspclkdiv << TBCTL_HSPCLKDIV_SHIFT);
+ *tb_clk_div = FIELD_PREP(TBCTL_CLKDIV_MASK, clkdiv) |
+ FIELD_PREP(TBCTL_HSPCLKDIV_MASK, hspclkdiv);
return 0;
}
}
--
2.25.1The ehrpwm driver was missing a get_state function, which is required
to properly retrieve the current state of the PWM channel. This commit
adds the ehrpwm_get_state() function, allowing users to query the
enabled state, period, duty cycle, and polarity of the PWM output.
The function reads the relevant registers (AQCSFRC, AQCTLx, TBPRD, CMPA)
to determine:
- Whether the PWM is enabled or disabled
- The configured period and duty cycle
- The polarity based on action-qualifier configurations
Additionally, this commit updates the pwm_ops structure to include
.get_state, ensuring proper integration with the PWM subsystem.
Signed-off-by: Rafael V. Volkmer <rafael.v.volkmer@gmail.com>
---
drivers/pwm/pwm-tiehrpwm.c | 122 +++++++++++++++++++++++++++++++++++++
1 file changed, 122 insertions(+)
diff --git a/drivers/pwm/pwm-tiehrpwm.c b/drivers/pwm/pwm-tiehrpwm.c
index 50516f46ab04..52527136c507 100644
--- a/drivers/pwm/pwm-tiehrpwm.c
+++ b/drivers/pwm/pwm-tiehrpwm.c
@@ -81,7 +81,9 @@
#define AQCTL_ZRO_MASK GENMASK(1, 0)
#define AQCTL_PRD_MASK GENMASK(3, 2)
#define AQCTL_CAU_MASK GENMASK(5, 4)
+#define AQCTL_CAD_MASK GENMASK(7, 6)
#define AQCTL_CBU_MASK GENMASK(9, 8)
+#define AQCTL_CBD_MASK GENMASK(11, 10)
/*
* FORCE LOW => 0b01
@@ -495,9 +497,129 @@ static int ehrpwm_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
return err;
}
+/**
+ * ehrpwm_get_state - Retrieves the current state of the eHRPWM channel
+ * @chip: pointer to the PWM chip structure
+ * @pwm: pointer to the PWM device structure
+ * @state: pointer to the pwm_state structure to be filled
+ *
+ * @return: 0 on success or a negative error code on failure
+ */
+static int ehrpwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
+ struct pwm_state *state)
+{
+ int ret = 0u;
+
+ struct ehrpwm_pwm_chip *pc = NULL;
+
+ unsigned long long tbclk_rate = 0u;
+
+ /* Registers */
+ u16 aqcsfrc_reg = 0u;
+ u16 aqctl_reg = 0u;
+ u16 tbprd_reg = 0u;
+ u16 cmpa_reg = 0u;
+
+ /* Bits */
+ u8 csf_bits = 0u;
+
+ /* Values */
+ u64 period_cycles = 0u;
+ u64 duty_cycles = 0u;
+
+ /* Actions */
+ u8 up_action = 0u;
+ u8 down_action = 0u;
+
+ if (chip == NULL || pwm == NULL || state == NULL)
+ return -EINVAL;
+
+ pc = to_ehrpwm_pwm_chip(chip);
+ if (pc == NULL || pwm == NULL || state == NULL)
+ return -EINVAL;
+
+ tbclk_rate = clk_get_rate(pc->tbclk);
+ if (tbclk_rate <= 0)
+ return -EINVAL;
+
+ /*< Get if EHRPWM is enable by checking registers values >*/
+
+ /*
+ * The 'hwpwm' field identifies which hardware output channel (e.g.,
+ * 0 for channel A and 1 for channel B) of the eHRPWM module is in use.
+ */
+ if (pwm->hwpwm == 0) {
+ aqcsfrc_reg = readw(pc->mmio_base + AQCSFRC);
+ csf_bits = FIELD_GET(AQCSFRC_CSFA_MASK, aqcsfrc_reg);
+ aqctl_reg = readw(pc->mmio_base + AQCTLA);
+ } else {
+ aqcsfrc_reg = readw(pc->mmio_base + AQCSFRC);
+ csf_bits = FIELD_GET(AQCSFRC_CSFB_MASK, aqcsfrc_reg);
+ aqctl_reg = readw(pc->mmio_base + AQCTLB);
+ }
+
+ if (csf_bits)
+ state->enabled = false;
+
+ else if (aqctl_reg)
+ state->enabled = true;
+
+ /*< Get EHRPWM Period by checking registers values >*/
+ tbprd_reg = readw(pc->mmio_base + TBPRD);
+ period_cycles = (u64)(tbprd_reg + 1u);
+
+ /*
+ * period (in ns) = (period_cycles * 1e9) / tbclk_rate
+ * Using DIV_ROUND_UP_ULL to avoid floating-point operations.
+ */
+ state->period = DIV_ROUND_UP_ULL(period_cycles * NSEC_PER_SEC, tbclk_rate);
+
+ /*< Get EHRPWM Duty Cycle by checking registers values >*/
+ cmpa_reg = readw(pc->mmio_base + CMPA);
+ duty_cycles = cmpa_reg;
+
+ /*
+ * duty_cycle (in ns) = (duty_cycles * 1e9) / tbclk_rate
+ * Using DIV_ROUND_UP_ULL to avoid floating-point operations.
+ */
+ state->duty_cycle = DIV_ROUND_UP_ULL(duty_cycles * NSEC_PER_SEC, tbclk_rate);
+
+ /*< Get EHRPWM polarity by checking registers values >*/
+
+ /*
+ * The 'hwpwm' field identifies which hardware output channel (e.g.,
+ * 0 for channel A and 1 for channel B) of the eHRPWM module is in use.
+ */
+ if (pwm->hwpwm == 0) {
+ aqctl_reg = readw(pc->mmio_base + AQCTLA);
+ up_action = FIELD_GET(AQCTL_CAU_MASK, aqctl_reg);
+ down_action = FIELD_GET(AQCTL_CAD_MASK, aqctl_reg);
+ } else {
+ aqctl_reg = readw(pc->mmio_base + AQCTLB);
+ up_action = FIELD_GET(AQCTL_CBU_MASK, aqctl_reg);
+ down_action = FIELD_GET(AQCTL_CBD_MASK, aqctl_reg);
+ }
+
+ /*
+ * Evaluate the actions to determine the PWM polarity:
+ * - If an up-count event sets the output (AQCTL_FRCHIGH) and a down-count
+ * event clears it (AQ_CLEAR), then polarity is NORMAL.
+ * - If an up-count event clears the output (AQ_CLEAR) and a down-count
+ * event sets it (AQCTL_FRCLOW), then polarity is INVERSED.
+ */
+ if (up_action == AQCTL_FRCHIGH && down_action == AQCTL_FRCLOW)
+ state->polarity = PWM_POLARITY_NORMAL;
+
+ else if (up_action == AQCTL_FRCLOW && down_action == AQCTL_FRCHIGH)
+ state->polarity = PWM_POLARITY_INVERSED;
+
+ return ret;
+}
+
static const struct pwm_ops ehrpwm_pwm_ops = {
.free = ehrpwm_pwm_free,
.apply = ehrpwm_pwm_apply,
+ .get_state = ehrpwm_get_state,
};
static const struct of_device_id ehrpwm_of_match[] = {
--
2.25.1Hello Rafael,
On Fri, Feb 07, 2025 at 06:32:34PM -0300, Rafael V. Volkmer wrote:
> The ehrpwm driver was missing a get_state function, which is required
> to properly retrieve the current state of the PWM channel. This commit
> adds the ehrpwm_get_state() function, allowing users to query the
> enabled state, period, duty cycle, and polarity of the PWM output.
s/This commit adds/Add/
> The function reads the relevant registers (AQCSFRC, AQCTLx, TBPRD, CMPA)
> to determine:
> - Whether the PWM is enabled or disabled
> - The configured period and duty cycle
> - The polarity based on action-qualifier configurations
>
> Additionally, this commit updates the pwm_ops structure to include
> .get_state, ensuring proper integration with the PWM subsystem.
The last two paragraphs are too verbose. I'd drop them.
> Signed-off-by: Rafael V. Volkmer <rafael.v.volkmer@gmail.com>
> ---
> drivers/pwm/pwm-tiehrpwm.c | 122 +++++++++++++++++++++++++++++++++++++
> 1 file changed, 122 insertions(+)
>
> diff --git a/drivers/pwm/pwm-tiehrpwm.c b/drivers/pwm/pwm-tiehrpwm.c
> index 50516f46ab04..52527136c507 100644
> --- a/drivers/pwm/pwm-tiehrpwm.c
> +++ b/drivers/pwm/pwm-tiehrpwm.c
> @@ -81,7 +81,9 @@
> #define AQCTL_ZRO_MASK GENMASK(1, 0)
> #define AQCTL_PRD_MASK GENMASK(3, 2)
> #define AQCTL_CAU_MASK GENMASK(5, 4)
> +#define AQCTL_CAD_MASK GENMASK(7, 6)
> #define AQCTL_CBU_MASK GENMASK(9, 8)
> +#define AQCTL_CBD_MASK GENMASK(11, 10)
>
> /*
> * FORCE LOW => 0b01
> @@ -495,9 +497,129 @@ static int ehrpwm_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> return err;
> }
>
> +/**
> + * ehrpwm_get_state - Retrieves the current state of the eHRPWM channel
> + * @chip: pointer to the PWM chip structure
> + * @pwm: pointer to the PWM device structure
> + * @state: pointer to the pwm_state structure to be filled
> + *
> + * @return: 0 on success or a negative error code on failure
> + */
> +static int ehrpwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
> + struct pwm_state *state)
> +{
> + int ret = 0u;
> +
> + struct ehrpwm_pwm_chip *pc = NULL;
> +
> + unsigned long long tbclk_rate = 0u;
> +
> + /* Registers */
> + u16 aqcsfrc_reg = 0u;
> + u16 aqctl_reg = 0u;
> + u16 tbprd_reg = 0u;
> + u16 cmpa_reg = 0u;
No need to initialize these. Also please drop the u suffix (which is
also inconsistant as you used U in patch #1). And then they can group
them in single code lines (i.e.
u16 aqcsfrc_reg, aqctl_reg, tbprd_reg, cmpa_reg;
).
> + /* Bits */
> + u8 csf_bits = 0u;
> +
> + /* Values */
> + u64 period_cycles = 0u;
> + u64 duty_cycles = 0u;
> +
> + /* Actions */
> + u8 up_action = 0u;
> + u8 down_action = 0u;
> +
> + if (chip == NULL || pwm == NULL || state == NULL)
> + return -EINVAL;
Drop this check, the core makes sure these are all valid.
> + pc = to_ehrpwm_pwm_chip(chip);
> + if (pc == NULL || pwm == NULL || state == NULL)
> + return -EINVAL;
Some of these you already checked above (so drop these, too). In my eyes
you can assume that to_ehrpwm_pwm_chip() doesn't return NULL.
> + tbclk_rate = clk_get_rate(pc->tbclk);
> + if (tbclk_rate <= 0)
> + return -EINVAL;
tbclk_rate is long long while the return value of clk_get_rate is long
only. Also given that tbclk_rate is unsigned <= doesn't make much sense.
Also note that clk_get_rate() is only supposed to be called for an
enabled clock.
> + /*< Get if EHRPWM is enable by checking registers values >*/
s/enable/enabled/
> + /*
> + * The 'hwpwm' field identifies which hardware output channel (e.g.,
> + * 0 for channel A and 1 for channel B) of the eHRPWM module is in use.
> + */
> + if (pwm->hwpwm == 0) {
> + aqcsfrc_reg = readw(pc->mmio_base + AQCSFRC);
> + csf_bits = FIELD_GET(AQCSFRC_CSFA_MASK, aqcsfrc_reg);
> + aqctl_reg = readw(pc->mmio_base + AQCTLA);
> + } else {
> + aqcsfrc_reg = readw(pc->mmio_base + AQCSFRC);
> + csf_bits = FIELD_GET(AQCSFRC_CSFB_MASK, aqcsfrc_reg);
> + aqctl_reg = readw(pc->mmio_base + AQCTLB);
> + }
> +
> + if (csf_bits)
> + state->enabled = false;
> +
> + else if (aqctl_reg)
> + state->enabled = true;
else
state->enable is uninitialized :-\
> + /*< Get EHRPWM Period by checking registers values >*/
This is kind of obvious. What is that /*< style? Looks like a bird :-)
> + tbprd_reg = readw(pc->mmio_base + TBPRD);
> + period_cycles = (u64)(tbprd_reg + 1u);
Maybe you need to cast first and then add 1?
> + /*
> + * period (in ns) = (period_cycles * 1e9) / tbclk_rate
> + * Using DIV_ROUND_UP_ULL to avoid floating-point operations.
> + */
> + state->period = DIV_ROUND_UP_ULL(period_cycles * NSEC_PER_SEC, tbclk_rate);
> +
> + /*< Get EHRPWM Duty Cycle by checking registers values >*/
> + cmpa_reg = readw(pc->mmio_base + CMPA);
> + duty_cycles = cmpa_reg;
> +
> + /*
> + * duty_cycle (in ns) = (duty_cycles * 1e9) / tbclk_rate
> + * Using DIV_ROUND_UP_ULL to avoid floating-point operations.
> + */
> + state->duty_cycle = DIV_ROUND_UP_ULL(duty_cycles * NSEC_PER_SEC, tbclk_rate);
s/ / /
> +
> + /*< Get EHRPWM polarity by checking registers values >*/
> +
> + /*
> + * The 'hwpwm' field identifies which hardware output channel (e.g.,
> + * 0 for channel A and 1 for channel B) of the eHRPWM module is in use.
> + */
> + if (pwm->hwpwm == 0) {
> + aqctl_reg = readw(pc->mmio_base + AQCTLA);
> + up_action = FIELD_GET(AQCTL_CAU_MASK, aqctl_reg);
> + down_action = FIELD_GET(AQCTL_CAD_MASK, aqctl_reg);
> + } else {
> + aqctl_reg = readw(pc->mmio_base + AQCTLB);
> + up_action = FIELD_GET(AQCTL_CBU_MASK, aqctl_reg);
> + down_action = FIELD_GET(AQCTL_CBD_MASK, aqctl_reg);
> + }
> +
> + /*
> + * Evaluate the actions to determine the PWM polarity:
> + * - If an up-count event sets the output (AQCTL_FRCHIGH) and a down-count
> + * event clears it (AQ_CLEAR), then polarity is NORMAL.
> + * - If an up-count event clears the output (AQ_CLEAR) and a down-count
> + * event sets it (AQCTL_FRCLOW), then polarity is INVERSED.
> + */
> + if (up_action == AQCTL_FRCHIGH && down_action == AQCTL_FRCLOW)
> + state->polarity = PWM_POLARITY_NORMAL;
> +
> + else if (up_action == AQCTL_FRCLOW && down_action == AQCTL_FRCHIGH)
> + state->polarity = PWM_POLARITY_INVERSED;
else ??? (maybe return an error?)
> + return ret;
> +}
> +
Best regards
Uwe
During probe, if the hardware is already active, it is not guaranteed
that the clock is enabled. To address this, ehrpwm_pwm_probe() now
checks whether the PWM is enabled and ensures that the necessary
resources are initialized.
Changes:
- Call ehrpwm_get_state() during probe to check if the PWM is active.
- If the PWM is enabled, call clk_prepare_enable() to ensure the clock
is active.
- If the clock is successfully enabled, call pm_runtime_get_sync() to
manage power state.
- Handle failure cases by properly disabling and unpreparing the clock.
This ensures that the driver correctly handles cases where the hardware
is already in use at the time of initialization, preventing potential
failures due to uninitialized resources.
Signed-off-by: Rafael V. Volkmer <rafael.v.volkmer@gmail.com>
---
drivers/pwm/pwm-tiehrpwm.c | 28 +++++++++++++++++++++++++++-
1 file changed, 27 insertions(+), 1 deletion(-)
diff --git a/drivers/pwm/pwm-tiehrpwm.c b/drivers/pwm/pwm-tiehrpwm.c
index 52527136c507..30beaf7d1721 100644
--- a/drivers/pwm/pwm-tiehrpwm.c
+++ b/drivers/pwm/pwm-tiehrpwm.c
@@ -633,8 +633,10 @@ static int ehrpwm_pwm_probe(struct platform_device *pdev)
{
struct device_node *np = pdev->dev.of_node;
struct ehrpwm_pwm_chip *pc;
+ struct pwm_state state;
struct pwm_chip *chip;
struct clk *clk;
+ bool tbclk_enabled;
int ret;
chip = devm_pwmchip_alloc(&pdev->dev, NUM_PWM_CHANNEL, sizeof(*pc));
@@ -676,6 +678,18 @@ static int ehrpwm_pwm_probe(struct platform_device *pdev)
return ret;
}
+ ehrpwm_get_state(chip, &chip->pwms[0], &state);
+
+ if (state.enabled == true) {
+ ret = clk_prepare_enable(pc->tbclk);
+ if (ret) {
+ dev_err_probe(&pdev->dev, ret, "clk_prepare_enable() failed");
+ goto err_pwmchip_remove;
+ }
+
+ tbclk_enabled = true;
+ }
+
ret = pwmchip_add(chip);
if (ret < 0) {
dev_err(&pdev->dev, "pwmchip_add() failed: %d\n", ret);
@@ -685,10 +699,22 @@ static int ehrpwm_pwm_probe(struct platform_device *pdev)
platform_set_drvdata(pdev, chip);
pm_runtime_enable(&pdev->dev);
+ if (state.enabled == true) {
+ ret = pm_runtime_get_sync(&pdev->dev);
+ if (ret < 0) {
+ dev_err_probe(&pdev->dev, ret, "pm_runtime_get_sync() failed");
+ clk_disable_unprepare(pc->tbclk);
+ goto err_pwmchip_remove;
+ }
+ }
+
return 0;
+err_pwmchip_remove:
+ pwmchip_remove(chip);
err_clk_unprepare:
- clk_unprepare(pc->tbclk);
+ if (tbclk_enabled)
+ clk_unprepare(pc->tbclk);
return ret;
}
--
2.25.1Hello Rafael,
On Fri, Feb 07, 2025 at 06:34:24PM -0300, Rafael V. Volkmer wrote:
> During probe, if the hardware is already active, it is not guaranteed
> that the clock is enabled. To address this, ehrpwm_pwm_probe() now
> checks whether the PWM is enabled and ensures that the necessary
> resources are initialized.
>
> Changes:
> - Call ehrpwm_get_state() during probe to check if the PWM is active.
> - If the PWM is enabled, call clk_prepare_enable() to ensure the clock
> is active.
> - If the clock is successfully enabled, call pm_runtime_get_sync() to
> manage power state.
> - Handle failure cases by properly disabling and unpreparing the clock.
This is too detailed, just drop the changes list.
> This ensures that the driver correctly handles cases where the hardware
> is already in use at the time of initialization, preventing potential
> failures due to uninitialized resources.
>
> Signed-off-by: Rafael V. Volkmer <rafael.v.volkmer@gmail.com>
> ---
> drivers/pwm/pwm-tiehrpwm.c | 28 +++++++++++++++++++++++++++-
> 1 file changed, 27 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/pwm/pwm-tiehrpwm.c b/drivers/pwm/pwm-tiehrpwm.c
> index 52527136c507..30beaf7d1721 100644
> --- a/drivers/pwm/pwm-tiehrpwm.c
> +++ b/drivers/pwm/pwm-tiehrpwm.c
> @@ -633,8 +633,10 @@ static int ehrpwm_pwm_probe(struct platform_device *pdev)
> {
> struct device_node *np = pdev->dev.of_node;
> struct ehrpwm_pwm_chip *pc;
> + struct pwm_state state;
> struct pwm_chip *chip;
> struct clk *clk;
> + bool tbclk_enabled;
> int ret;
>
> chip = devm_pwmchip_alloc(&pdev->dev, NUM_PWM_CHANNEL, sizeof(*pc));
> @@ -676,6 +678,18 @@ static int ehrpwm_pwm_probe(struct platform_device *pdev)
> return ret;
> }
>
> + ehrpwm_get_state(chip, &chip->pwms[0], &state);
ehrpwm_get_state() does some things that are not needed here given that
you only evaluate state.enabled. I suggest to just read the one (or
two?) registers you need to determine if the PWM is on.
> + if (state.enabled == true) {
> + ret = clk_prepare_enable(pc->tbclk);
pc->tbclk is already prepared, so clk_enable() should be enough. After
all this should match what ehrpwm_pwm_enable() does.
> + if (ret) {
> + dev_err_probe(&pdev->dev, ret, "clk_prepare_enable() failed");
> + goto err_pwmchip_remove;
> + }
> +
> + tbclk_enabled = true;
> + }
> +
> ret = pwmchip_add(chip);
> if (ret < 0) {
> dev_err(&pdev->dev, "pwmchip_add() failed: %d\n", ret);
> @@ -685,10 +699,22 @@ static int ehrpwm_pwm_probe(struct platform_device *pdev)
> platform_set_drvdata(pdev, chip);
> pm_runtime_enable(&pdev->dev);
>
> + if (state.enabled == true) {
> + ret = pm_runtime_get_sync(&pdev->dev);
> + if (ret < 0) {
> + dev_err_probe(&pdev->dev, ret, "pm_runtime_get_sync() failed");
> + clk_disable_unprepare(pc->tbclk);
> + goto err_pwmchip_remove;
> + }
It feels a bit strange to do this here. I think technically it's fine
here, but doing pm_runtime_get_sync() before pwmchip_add() would make
that a bit clearer.
> + }
> +
> return 0;
>
> +err_pwmchip_remove:
> + pwmchip_remove(chip);
> err_clk_unprepare:
> - clk_unprepare(pc->tbclk);
> + if (tbclk_enabled)
> + clk_unprepare(pc->tbclk);
I think this is wrong an keeping the unconditional clk_unprepare() is
right. Might be easier to convert the driver to use
devm_clk_get_enabled().
Best regards
Uwe
Simplify bit manipulation by replacing manual BIT(x) definitions with
GENMASK() and FIELD_PREP() from <linux/bitfield.h>. This improves
readability, consistency, and aligns with modern kernel practices
while preserving existing functionality.
Additionally, update set_prescale_div() to use FIELD_PREP() for
TBCTL_CLKDIV_MASK and TBCTL_HSPCLKDIV_MASK instead of manually
shifting bits. This makes the code more maintainable and avoids
potential errors in bit field assignments.
Signed-off-by: Rafael V. Volkmer <rafael.v.volkmer@gmail.com>
---
drivers/pwm/pwm-tiehrpwm.c | 191 ++++++++++++++++++++++---------------
1 file changed, 114 insertions(+), 77 deletions(-)
diff --git a/drivers/pwm/pwm-tiehrpwm.c b/drivers/pwm/pwm-tiehrpwm.c
index 0125e73b98df..1ead1aa91a1a 100644
--- a/drivers/pwm/pwm-tiehrpwm.c
+++ b/drivers/pwm/pwm-tiehrpwm.c
@@ -13,85 +13,122 @@
#include <linux/clk.h>
#include <linux/pm_runtime.h>
#include <linux/of.h>
+#include <linux/bitfield.h>
/* EHRPWM registers and bits definitions */
/* Time base module registers */
-#define TBCTL 0x00
-#define TBPRD 0x0A
-
-#define TBCTL_PRDLD_MASK BIT(3)
-#define TBCTL_PRDLD_SHDW 0
-#define TBCTL_PRDLD_IMDT BIT(3)
-#define TBCTL_CLKDIV_MASK (BIT(12) | BIT(11) | BIT(10) | BIT(9) | \
- BIT(8) | BIT(7))
-#define TBCTL_CTRMODE_MASK (BIT(1) | BIT(0))
-#define TBCTL_CTRMODE_UP 0
-#define TBCTL_CTRMODE_DOWN BIT(0)
-#define TBCTL_CTRMODE_UPDOWN BIT(1)
-#define TBCTL_CTRMODE_FREEZE (BIT(1) | BIT(0))
-
-#define TBCTL_HSPCLKDIV_SHIFT 7
-#define TBCTL_CLKDIV_SHIFT 10
-
-#define CLKDIV_MAX 7
-#define HSPCLKDIV_MAX 7
-#define PERIOD_MAX 0xFFFF
-
-/* compare module registers */
-#define CMPA 0x12
-#define CMPB 0x14
-
-/* Action qualifier module registers */
-#define AQCTLA 0x16
-#define AQCTLB 0x18
-#define AQSFRC 0x1A
-#define AQCSFRC 0x1C
-
-#define AQCTL_CBU_MASK (BIT(9) | BIT(8))
-#define AQCTL_CBU_FRCLOW BIT(8)
-#define AQCTL_CBU_FRCHIGH BIT(9)
-#define AQCTL_CBU_FRCTOGGLE (BIT(9) | BIT(8))
-#define AQCTL_CAU_MASK (BIT(5) | BIT(4))
-#define AQCTL_CAU_FRCLOW BIT(4)
-#define AQCTL_CAU_FRCHIGH BIT(5)
-#define AQCTL_CAU_FRCTOGGLE (BIT(5) | BIT(4))
-#define AQCTL_PRD_MASK (BIT(3) | BIT(2))
-#define AQCTL_PRD_FRCLOW BIT(2)
-#define AQCTL_PRD_FRCHIGH BIT(3)
-#define AQCTL_PRD_FRCTOGGLE (BIT(3) | BIT(2))
-#define AQCTL_ZRO_MASK (BIT(1) | BIT(0))
-#define AQCTL_ZRO_FRCLOW BIT(0)
-#define AQCTL_ZRO_FRCHIGH BIT(1)
-#define AQCTL_ZRO_FRCTOGGLE (BIT(1) | BIT(0))
-
-#define AQCTL_CHANA_POLNORMAL (AQCTL_CAU_FRCLOW | AQCTL_PRD_FRCHIGH | \
- AQCTL_ZRO_FRCHIGH)
-#define AQCTL_CHANA_POLINVERSED (AQCTL_CAU_FRCHIGH | AQCTL_PRD_FRCLOW | \
- AQCTL_ZRO_FRCLOW)
-#define AQCTL_CHANB_POLNORMAL (AQCTL_CBU_FRCLOW | AQCTL_PRD_FRCHIGH | \
- AQCTL_ZRO_FRCHIGH)
-#define AQCTL_CHANB_POLINVERSED (AQCTL_CBU_FRCHIGH | AQCTL_PRD_FRCLOW | \
- AQCTL_ZRO_FRCLOW)
-
-#define AQSFRC_RLDCSF_MASK (BIT(7) | BIT(6))
-#define AQSFRC_RLDCSF_ZRO 0
-#define AQSFRC_RLDCSF_PRD BIT(6)
-#define AQSFRC_RLDCSF_ZROPRD BIT(7)
-#define AQSFRC_RLDCSF_IMDT (BIT(7) | BIT(6))
-
-#define AQCSFRC_CSFB_MASK (BIT(3) | BIT(2))
-#define AQCSFRC_CSFB_FRCDIS 0
-#define AQCSFRC_CSFB_FRCLOW BIT(2)
-#define AQCSFRC_CSFB_FRCHIGH BIT(3)
-#define AQCSFRC_CSFB_DISSWFRC (BIT(3) | BIT(2))
-#define AQCSFRC_CSFA_MASK (BIT(1) | BIT(0))
-#define AQCSFRC_CSFA_FRCDIS 0
-#define AQCSFRC_CSFA_FRCLOW BIT(0)
-#define AQCSFRC_CSFA_FRCHIGH BIT(1)
-#define AQCSFRC_CSFA_DISSWFRC (BIT(1) | BIT(0))
-
-#define NUM_PWM_CHANNEL 2 /* EHRPWM channels */
+#define TBCTL 0x00
+#define TBPRD 0x0A
+
+/* TBCTL: CTRMODE field (bits [1:0]) */
+#define TBCTL_CTRMODE_MASK GENMASK(1, 0)
+#define TBCTL_CTRMODE_UP FIELD_PREP(TBCTL_CTRMODE_MASK, 0)
+#define TBCTL_CTRMODE_DOWN FIELD_PREP(TBCTL_CTRMODE_MASK, 1)
+#define TBCTL_CTRMODE_UPDOWN FIELD_PREP(TBCTL_CTRMODE_MASK, 2)
+#define TBCTL_CTRMODE_FREEZE FIELD_PREP(TBCTL_CTRMODE_MASK, 3)
+
+/* TBCTL: PRDLD bit (bit [3]) */
+#define TBCTL_PRDLD_MASK GENMASK(3, 3)
+#define TBCTL_PRDLD_SHDW FIELD_PREP(TBCTL_PRDLD_MASK, 0) /* shadow load */
+#define TBCTL_PRDLD_IMDT FIELD_PREP(TBCTL_PRDLD_MASK, 1) /* immediate */
+
+/* TBCTL: high‑speed prescale (bits [9:7]) */
+#define TBCTL_HSPCLKDIV_MASK GENMASK(9, 7)
+/* TBCTL: clock prescale (bits [12:10]) */
+#define TBCTL_CLKDIV_MASK GENMASK(12, 10)
+
+#define CLKDIV_MAX 7
+#define HSPCLKDIV_MAX 7
+#define PERIOD_MAX 0xFFFF
+
+/*
+ * ----------------------------------------------------------------
+ * Compare module registers
+ * ----------------------------------------------------------------
+ */
+#define CMPA 0x12
+#define CMPB 0x14
+
+/*
+ * ----------------------------------------------------------------
+ * Action Qualifier (AQ) module registers
+ * ----------------------------------------------------------------
+ */
+#define AQCTLA 0x16
+#define AQCTLB 0x18
+#define AQSFRC 0x1A
+#define AQCSFRC 0x1
+
+/* AQCTL: event-action fields */
+/* ZRO = bits [1:0] */
+/* PRD = bits [3:2] */
+/* CAU = bits [5:4] */
+/* CBU = bits [9:8] */
+#define AQCTL_ZRO_MASK GENMASK(1, 0)
+#define AQCTL_PRD_MASK GENMASK(3, 2)
+#define AQCTL_CAU_MASK GENMASK(5, 4)
+#define AQCTL_CBU_MASK GENMASK(9, 8)
+
+/* common action codes (2‑bit) */
+#define AQCTL_FRCLOW 1
+#define AQCTL_FRCHIGH 2
+#define AQCTL_FRCTOGGLE 3
+
+/* ZRO actions */
+#define AQCTL_ZRO_FRCLOW FIELD_PREP(AQCTL_ZRO_MASK, AQCTL_FRCLOW)
+#define AQCTL_ZRO_FRCHIGH FIELD_PREP(AQCTL_ZRO_MASK, AQCTL_FRCHIGH)
+#define AQCTL_ZRO_FRCTOGGLE FIELD_PREP(AQCTL_ZRO_MASK, AQCTL_FRCTOGGLE)
+
+/* PRD actions */
+#define AQCTL_PRD_FRCLOW FIELD_PREP(AQCTL_PRD_MASK, AQCTL_FRCLOW)
+#define AQCTL_PRD_FRCHIGH FIELD_PREP(AQCTL_PRD_MASK, AQCTL_FRCHIGH)
+#define AQCTL_PRD_FRCTOGGLE FIELD_PREP(AQCTL_PRD_MASK, AQCTL_FRCTOGGLE)
+
+/* CAU actions */
+#define AQCTL_CAU_FRCLOW FIELD_PREP(AQCTL_CAU_MASK, AQCTL_FRCLOW)
+#define AQCTL_CAU_FRCHIGH FIELD_PREP(AQCTL_CAU_MASK, AQCTL_FRCHIGH)
+#define AQCTL_CAU_FRCTOGGLE FIELD_PREP(AQCTL_CAU_MASK, AQCTL_FRCTOGGLE)
+
+/* CBU actions */
+#define AQCTL_CBU_FRCLOW FIELD_PREP(AQCTL_CBU_MASK, AQCTL_FRCLOW)
+#define AQCTL_CBU_FRCHIGH FIELD_PREP(AQCTL_CBU_MASK, AQCTL_FRCHIGH)
+#define AQCTL_CBU_FRCTOGGLE FIELD_PREP(AQCTL_CBU_MASK, AQCTL_FRCTOGGLE)
+
+/* predefined channel‑polarity combos */
+#define AQCTL_CHANA_POLNORMAL \
+ (AQCTL_CAU_FRCLOW | AQCTL_PRD_FRCHIGH | AQCTL_ZRO_FRCHIGH)
+#define AQCTL_CHANA_POLINVERSED \
+ (AQCTL_CAU_FRCHIGH | AQCTL_PRD_FRCLOW | AQCTL_ZRO_FRCLOW)
+
+#define AQCTL_CHANB_POLNORMAL \
+ (AQCTL_CBU_FRCLOW | AQCTL_PRD_FRCHIGH | AQCTL_ZRO_FRCHIGH)
+#define AQCTL_CHANB_POLINVERSED \
+ (AQCTL_CBU_FRCHIGH | AQCTL_PRD_FRCLOW | AQCTL_ZRO_FRCLOW)
+
+/* AQSFRC: RLDCSF (bits [7:6]) */
+#define AQSFRC_RLDCSF_MASK GENMASK(7, 6)
+#define AQSFRC_RLDCSF_ZRO FIELD_PREP(AQSFRC_RLDCSF_MASK, 0)
+#define AQSFRC_RLDCSF_PRD FIELD_PREP(AQSFRC_RLDCSF_MASK, 1)
+#define AQSFRC_RLDCSF_ZROPRD FIELD_PREP(AQSFRC_RLDCSF_MASK, 2)
+#define AQSFRC_RLDCSF_IMDT FIELD_PREP(AQSFRC_RLDCSF_MASK, 3)
+
+/* AQCSFRC: CSFB (bits [3:2]), CSFA (bits [1:0]) */
+#define AQCSFRC_CSFB_MASK GENMASK(3, 2)
+#define AQCSFRC_CSFA_MASK GENMASK(1, 0)
+
+#define AQCSFRC_CSFB_FRCDIS FIELD_PREP(AQCSFRC_CSFB_MASK, 0)
+#define AQCSFRC_CSFB_FRCLOW FIELD_PREP(AQCSFRC_CSFB_MASK, 1)
+#define AQCSFRC_CSFB_FRCHIGH FIELD_PREP(AQCSFRC_CSFB_MASK, 2)
+#define AQCSFRC_CSFB_DISSWFRC FIELD_PREP(AQCSFRC_CSFB_MASK, 3)
+
+#define AQCSFRC_CSFA_FRCDIS FIELD_PREP(AQCSFRC_CSFA_MASK, 0)
+#define AQCSFRC_CSFA_FRCLOW FIELD_PREP(AQCSFRC_CSFA_MASK, 1)
+#define AQCSFRC_CSFA_FRCHIGH FIELD_PREP(AQCSFRC_CSFA_MASK, 2)
+#define AQCSFRC_CSFA_DISSWFRC FIELD_PREP(AQCSFRC_CSFA_MASK, 3)
+
+/* Number of EHRPWM channels */
+#define NUM_PWM_CHANNEL 2U
struct ehrpwm_context {
u16 tbctl;
@@ -167,8 +204,8 @@ static int set_prescale_div(unsigned long rqst_prescaler, u16 *prescale_div,
*prescale_div = (1 << clkdiv) *
(hspclkdiv ? (hspclkdiv * 2) : 1);
if (*prescale_div > rqst_prescaler) {
- *tb_clk_div = (clkdiv << TBCTL_CLKDIV_SHIFT) |
- (hspclkdiv << TBCTL_HSPCLKDIV_SHIFT);
+ *tb_clk_div = FIELD_PREP(TBCTL_CLKDIV_MASK, clkdiv) |
+ FIELD_PREP(TBCTL_HSPCLKDIV_MASK, hspclkdiv);
return 0;
}
}
--
2.25.1
On Sat, Apr 19, 2025 at 04:48:35PM -0300, Rafael V. Volkmer wrote:
> Simplify bit manipulation by replacing manual BIT(x) definitions with
> GENMASK() and FIELD_PREP() from <linux/bitfield.h>. This improves
> readability, consistency, and aligns with modern kernel practices
> while preserving existing functionality.
>
> Additionally, update set_prescale_div() to use FIELD_PREP() for
> TBCTL_CLKDIV_MASK and TBCTL_HSPCLKDIV_MASK instead of manually
> shifting bits. This makes the code more maintainable and avoids
> potential errors in bit field assignments.
>
> Signed-off-by: Rafael V. Volkmer <rafael.v.volkmer@gmail.com>
> ---
> drivers/pwm/pwm-tiehrpwm.c | 191 ++++++++++++++++++++++---------------
> 1 file changed, 114 insertions(+), 77 deletions(-)
I reorder the patch a bit with the intend to not change semantics but to
make my points a bit clearer.
> diff --git a/drivers/pwm/pwm-tiehrpwm.c b/drivers/pwm/pwm-tiehrpwm.c
> index 0125e73b98df..1ead1aa91a1a 100644
> --- a/drivers/pwm/pwm-tiehrpwm.c
> +++ b/drivers/pwm/pwm-tiehrpwm.c
> @@ -13,85 +13,122 @@
> #include <linux/clk.h>
> #include <linux/pm_runtime.h>
> #include <linux/of.h>
> +#include <linux/bitfield.h>
>
> /* EHRPWM registers and bits definitions */
>
> /* Time base module registers */
> -#define TBCTL 0x00
> -#define TBPRD 0x0A
> +#define TBCTL 0x00
> +#define TBPRD 0x0A
It's unclear to me why you changed the indention here. While you might
consider that inconsistent as I hate indenting struct initializers, for
#defines I think it greatly improves readability. I'd also keep the
whitespace changes to a minimum to make the patch better readable.
> -#define TBCTL_PRDLD_MASK BIT(3)
> -#define TBCTL_PRDLD_SHDW 0
> -#define TBCTL_PRDLD_IMDT BIT(3)
> -#define TBCTL_CLKDIV_MASK (BIT(12) | BIT(11) | BIT(10) | BIT(9) | \
> - BIT(8) | BIT(7))
> -#define TBCTL_CTRMODE_MASK (BIT(1) | BIT(0))
> -#define TBCTL_CTRMODE_UP 0
> -#define TBCTL_CTRMODE_DOWN BIT(0)
> -#define TBCTL_CTRMODE_UPDOWN BIT(1)
> -#define TBCTL_CTRMODE_FREEZE (BIT(1) | BIT(0))
Huh, you reordered these?
> -#define TBCTL_HSPCLKDIV_SHIFT 7
> -#define TBCTL_CLKDIV_SHIFT 10
> -
> -#define CLKDIV_MAX 7
> -#define HSPCLKDIV_MAX 7
> -#define PERIOD_MAX 0xFFFF
> -
> -/* compare module registers */
> -#define CMPA 0x12
> -#define CMPB 0x14
> -
> -/* Action qualifier module registers */
> -#define AQCTLA 0x16
> -#define AQCTLB 0x18
> -#define AQSFRC 0x1A
> -#define AQCSFRC 0x1C
> -
> -#define AQCTL_CBU_MASK (BIT(9) | BIT(8))
> -#define AQCTL_CBU_FRCLOW BIT(8)
> -#define AQCTL_CBU_FRCHIGH BIT(9)
> -#define AQCTL_CBU_FRCTOGGLE (BIT(9) | BIT(8))
> -#define AQCTL_CAU_MASK (BIT(5) | BIT(4))
> -#define AQCTL_CAU_FRCLOW BIT(4)
> -#define AQCTL_CAU_FRCHIGH BIT(5)
> -#define AQCTL_CAU_FRCTOGGLE (BIT(5) | BIT(4))
> -#define AQCTL_PRD_MASK (BIT(3) | BIT(2))
> -#define AQCTL_PRD_FRCLOW BIT(2)
> -#define AQCTL_PRD_FRCHIGH BIT(3)
> -#define AQCTL_PRD_FRCTOGGLE (BIT(3) | BIT(2))
> -#define AQCTL_ZRO_MASK (BIT(1) | BIT(0))
> -#define AQCTL_ZRO_FRCLOW BIT(0)
> -#define AQCTL_ZRO_FRCHIGH BIT(1)
> -#define AQCTL_ZRO_FRCTOGGLE (BIT(1) | BIT(0))
> -
> -#define AQCTL_CHANA_POLNORMAL (AQCTL_CAU_FRCLOW | AQCTL_PRD_FRCHIGH | \
> - AQCTL_ZRO_FRCHIGH)
> -#define AQCTL_CHANA_POLINVERSED (AQCTL_CAU_FRCHIGH | AQCTL_PRD_FRCLOW | \
> - AQCTL_ZRO_FRCLOW)
> -#define AQCTL_CHANB_POLNORMAL (AQCTL_CBU_FRCLOW | AQCTL_PRD_FRCHIGH | \
> - AQCTL_ZRO_FRCHIGH)
> -#define AQCTL_CHANB_POLINVERSED (AQCTL_CBU_FRCHIGH | AQCTL_PRD_FRCLOW | \
> - AQCTL_ZRO_FRCLOW)
> -
> -#define AQSFRC_RLDCSF_MASK (BIT(7) | BIT(6))
> -#define AQSFRC_RLDCSF_ZRO 0
> -#define AQSFRC_RLDCSF_PRD BIT(6)
> -#define AQSFRC_RLDCSF_ZROPRD BIT(7)
> -#define AQSFRC_RLDCSF_IMDT (BIT(7) | BIT(6))
> -
> -#define AQCSFRC_CSFB_MASK (BIT(3) | BIT(2))
> -#define AQCSFRC_CSFB_FRCDIS 0
> -#define AQCSFRC_CSFB_FRCLOW BIT(2)
> -#define AQCSFRC_CSFB_FRCHIGH BIT(3)
> -#define AQCSFRC_CSFB_DISSWFRC (BIT(3) | BIT(2))
> -#define AQCSFRC_CSFA_MASK (BIT(1) | BIT(0))
> -#define AQCSFRC_CSFA_FRCDIS 0
> -#define AQCSFRC_CSFA_FRCLOW BIT(0)
> -#define AQCSFRC_CSFA_FRCHIGH BIT(1)
> -#define AQCSFRC_CSFA_DISSWFRC (BIT(1) | BIT(0))
> -
> -#define NUM_PWM_CHANNEL 2 /* EHRPWM channels */
> +/* TBCTL: CTRMODE field (bits [1:0]) */
> +#define TBCTL_CTRMODE_MASK GENMASK(1, 0)
> +#define TBCTL_CTRMODE_UP FIELD_PREP(TBCTL_CTRMODE_MASK, 0)
> +#define TBCTL_CTRMODE_DOWN FIELD_PREP(TBCTL_CTRMODE_MASK, 1)
> +#define TBCTL_CTRMODE_UPDOWN FIELD_PREP(TBCTL_CTRMODE_MASK, 2)
> +#define TBCTL_CTRMODE_FREEZE FIELD_PREP(TBCTL_CTRMODE_MASK, 3)
> +
> +/* TBCTL: PRDLD bit (bit [3]) */
> +#define TBCTL_PRDLD_MASK GENMASK(3, 3)
> +#define TBCTL_PRDLD_SHDW FIELD_PREP(TBCTL_PRDLD_MASK, 0) /* shadow load */
> +#define TBCTL_PRDLD_IMDT FIELD_PREP(TBCTL_PRDLD_MASK, 1) /* immediate */
> +
> +/* TBCTL: high‑speed prescale (bits [9:7]) */
> +#define TBCTL_HSPCLKDIV_MASK GENMASK(9, 7)
> +/* TBCTL: clock prescale (bits [12:10]) */
> +#define TBCTL_CLKDIV_MASK GENMASK(12, 10)
This is different than before. Please only change the way the constants
are defined and not their values.
> +
> +#define CLKDIV_MAX 7
> +#define HSPCLKDIV_MAX 7
> +#define PERIOD_MAX 0xFFFF
> +
> +/*
> + * ----------------------------------------------------------------
> + * Compare module registers
> + * ----------------------------------------------------------------
> + */
> +#define CMPA 0x12
> +#define CMPB 0x14
> +
> +/*
> + * ----------------------------------------------------------------
> + * Action Qualifier (AQ) module registers
> + * ----------------------------------------------------------------
> + */
> +#define AQCTLA 0x16
> +#define AQCTLB 0x18
> +#define AQSFRC 0x1A
> +#define AQCSFRC 0x1
> +
> +/* AQCTL: event-action fields */
> +/* ZRO = bits [1:0] */
> +/* PRD = bits [3:2] */
> +/* CAU = bits [5:4] */
> +/* CBU = bits [9:8] */
> +#define AQCTL_ZRO_MASK GENMASK(1, 0)
> +#define AQCTL_PRD_MASK GENMASK(3, 2)
> +#define AQCTL_CAU_MASK GENMASK(5, 4)
> +#define AQCTL_CBU_MASK GENMASK(9, 8)
> +
> +/* common action codes (2‑bit) */
> +#define AQCTL_FRCLOW 1
> +#define AQCTL_FRCHIGH 2
> +#define AQCTL_FRCTOGGLE 3
> +
> +/* ZRO actions */
> +#define AQCTL_ZRO_FRCLOW FIELD_PREP(AQCTL_ZRO_MASK, AQCTL_FRCLOW)
> +#define AQCTL_ZRO_FRCHIGH FIELD_PREP(AQCTL_ZRO_MASK, AQCTL_FRCHIGH)
> +#define AQCTL_ZRO_FRCTOGGLE FIELD_PREP(AQCTL_ZRO_MASK, AQCTL_FRCTOGGLE)
> +
> +/* PRD actions */
> +#define AQCTL_PRD_FRCLOW FIELD_PREP(AQCTL_PRD_MASK, AQCTL_FRCLOW)
> +#define AQCTL_PRD_FRCHIGH FIELD_PREP(AQCTL_PRD_MASK, AQCTL_FRCHIGH)
> +#define AQCTL_PRD_FRCTOGGLE FIELD_PREP(AQCTL_PRD_MASK, AQCTL_FRCTOGGLE)
> +
> +/* CAU actions */
> +#define AQCTL_CAU_FRCLOW FIELD_PREP(AQCTL_CAU_MASK, AQCTL_FRCLOW)
> +#define AQCTL_CAU_FRCHIGH FIELD_PREP(AQCTL_CAU_MASK, AQCTL_FRCHIGH)
> +#define AQCTL_CAU_FRCTOGGLE FIELD_PREP(AQCTL_CAU_MASK, AQCTL_FRCTOGGLE)
> +
> +/* CBU actions */
> +#define AQCTL_CBU_FRCLOW FIELD_PREP(AQCTL_CBU_MASK, AQCTL_FRCLOW)
> +#define AQCTL_CBU_FRCHIGH FIELD_PREP(AQCTL_CBU_MASK, AQCTL_FRCHIGH)
> +#define AQCTL_CBU_FRCTOGGLE FIELD_PREP(AQCTL_CBU_MASK, AQCTL_FRCTOGGLE)
> +
> +/* predefined channel‑polarity combos */
> +#define AQCTL_CHANA_POLNORMAL \
> + (AQCTL_CAU_FRCLOW | AQCTL_PRD_FRCHIGH | AQCTL_ZRO_FRCHIGH)
> +#define AQCTL_CHANA_POLINVERSED \
> + (AQCTL_CAU_FRCHIGH | AQCTL_PRD_FRCLOW | AQCTL_ZRO_FRCLOW)
> +
> +#define AQCTL_CHANB_POLNORMAL \
> + (AQCTL_CBU_FRCLOW | AQCTL_PRD_FRCHIGH | AQCTL_ZRO_FRCHIGH)
> +#define AQCTL_CHANB_POLINVERSED \
> + (AQCTL_CBU_FRCHIGH | AQCTL_PRD_FRCLOW | AQCTL_ZRO_FRCLOW)
> +
> +/* AQSFRC: RLDCSF (bits [7:6]) */
> +#define AQSFRC_RLDCSF_MASK GENMASK(7, 6)
> +#define AQSFRC_RLDCSF_ZRO FIELD_PREP(AQSFRC_RLDCSF_MASK, 0)
> +#define AQSFRC_RLDCSF_PRD FIELD_PREP(AQSFRC_RLDCSF_MASK, 1)
> +#define AQSFRC_RLDCSF_ZROPRD FIELD_PREP(AQSFRC_RLDCSF_MASK, 2)
> +#define AQSFRC_RLDCSF_IMDT FIELD_PREP(AQSFRC_RLDCSF_MASK, 3)
> +
> +/* AQCSFRC: CSFB (bits [3:2]), CSFA (bits [1:0]) */
> +#define AQCSFRC_CSFB_MASK GENMASK(3, 2)
> +#define AQCSFRC_CSFA_MASK GENMASK(1, 0)
> +
> +#define AQCSFRC_CSFB_FRCDIS FIELD_PREP(AQCSFRC_CSFB_MASK, 0)
> +#define AQCSFRC_CSFB_FRCLOW FIELD_PREP(AQCSFRC_CSFB_MASK, 1)
> +#define AQCSFRC_CSFB_FRCHIGH FIELD_PREP(AQCSFRC_CSFB_MASK, 2)
> +#define AQCSFRC_CSFB_DISSWFRC FIELD_PREP(AQCSFRC_CSFB_MASK, 3)
> +
> +#define AQCSFRC_CSFA_FRCDIS FIELD_PREP(AQCSFRC_CSFA_MASK, 0)
> +#define AQCSFRC_CSFA_FRCLOW FIELD_PREP(AQCSFRC_CSFA_MASK, 1)
> +#define AQCSFRC_CSFA_FRCHIGH FIELD_PREP(AQCSFRC_CSFA_MASK, 2)
> +#define AQCSFRC_CSFA_DISSWFRC FIELD_PREP(AQCSFRC_CSFA_MASK, 3)
> +
> +/* Number of EHRPWM channels */
> +#define NUM_PWM_CHANNEL 2U
That U is quite useless, a plain 2 does well.
> struct ehrpwm_context {
> u16 tbctl;
> @@ -167,8 +204,8 @@ static int set_prescale_div(unsigned long rqst_prescaler, u16 *prescale_div,
> *prescale_div = (1 << clkdiv) *
> (hspclkdiv ? (hspclkdiv * 2) : 1);
> if (*prescale_div > rqst_prescaler) {
> - *tb_clk_div = (clkdiv << TBCTL_CLKDIV_SHIFT) |
> - (hspclkdiv << TBCTL_HSPCLKDIV_SHIFT);
> + *tb_clk_div = FIELD_PREP(TBCTL_CLKDIV_MASK, clkdiv) |
> + FIELD_PREP(TBCTL_HSPCLKDIV_MASK, hspclkdiv);
I'd like to have this change in a separate patch. Is it obvious that the
new variant has the same semantic as the old? Or does this introduce
changes for some inputs?
Best regards
Uwe
The ehrpwm driver was missing a get_state function, which is required
to properly retrieve the current state of the PWM channel. Add the
ehrpwm_get_state() function, allowing users to query the enabled state,
period, duty cycle, and polarity of the PWM output.
Signed-off-by: Rafael V. Volkmer <rafael.v.volkmer@gmail.com>
---
drivers/pwm/pwm-tiehrpwm.c | 97 ++++++++++++++++++++++++++++++++++++++
1 file changed, 97 insertions(+)
diff --git a/drivers/pwm/pwm-tiehrpwm.c b/drivers/pwm/pwm-tiehrpwm.c
index 1ead1aa91a1a..cde331a73696 100644
--- a/drivers/pwm/pwm-tiehrpwm.c
+++ b/drivers/pwm/pwm-tiehrpwm.c
@@ -68,7 +68,9 @@
#define AQCTL_ZRO_MASK GENMASK(1, 0)
#define AQCTL_PRD_MASK GENMASK(3, 2)
#define AQCTL_CAU_MASK GENMASK(5, 4)
+#define AQCTL_CAD_MASK GENMASK(7, 6)
#define AQCTL_CBU_MASK GENMASK(9, 8)
+#define AQCTL_CBD_MASK GENMASK(11, 10)
/* common action codes (2‑bit) */
#define AQCTL_FRCLOW 1
@@ -470,9 +472,104 @@ static int ehrpwm_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
return err;
}
+static int ehrpwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
+ struct pwm_state *state)
+{
+ int ret = 0;
+
+ struct ehrpwm_pwm_chip *pc = NULL;
+
+ /* Registers */
+ u16 aqcsfrc_reg, aqctl_reg, tbprd_reg, cmpa_reg;
+
+ /* Bits */
+ u8 csf_bits;
+
+ /* Values */
+ u64 period_cycles, duty_cycles;
+
+ /* Actions */
+ u8 up_action, down_action;
+
+ pc = to_ehrpwm_pwm_chip(chip);
+
+ /*
+ * The 'hwpwm' field identifies which hardware output channel (e.g.,
+ * 0 for channel A and 1 for channel B) of the eHRPWM module is in use.
+ */
+ if (pwm->hwpwm == 0) {
+ aqcsfrc_reg = readw(pc->mmio_base + AQCSFRC);
+ csf_bits = FIELD_GET(AQCSFRC_CSFA_MASK, aqcsfrc_reg);
+ aqctl_reg = readw(pc->mmio_base + AQCTLA);
+ } else {
+ aqcsfrc_reg = readw(pc->mmio_base + AQCSFRC);
+ csf_bits = FIELD_GET(AQCSFRC_CSFB_MASK, aqcsfrc_reg);
+ aqctl_reg = readw(pc->mmio_base + AQCTLB);
+ }
+
+ if (csf_bits)
+ state->enabled = false;
+ else if (aqctl_reg)
+ state->enabled = true;
+ else
+ state->enabled = false;
+
+ tbprd_reg = readw(pc->mmio_base + TBPRD);
+ period_cycles = (u64)tbprd_reg + 1u;
+
+ /*
+ * period (in ns) = (period_cycles * 1e9) / clk_rate
+ * Using DIV_ROUND_UP_ULL to avoid floating-point operations.
+ */
+ state->period = DIV_ROUND_UP_ULL(period_cycles * NSEC_PER_SEC, pc->clk_rate);
+
+ cmpa_reg = readw(pc->mmio_base + CMPA);
+ duty_cycles = cmpa_reg;
+
+ /*
+ * duty_cycle (in ns) = (duty_cycles * 1e9) / clk_rate
+ * Using DIV_ROUND_UP_ULL to avoid floating-point operations.
+ */
+ state->duty_cycle = DIV_ROUND_UP_ULL(duty_cycles * NSEC_PER_SEC, pc->clk_rate);
+
+ /*
+ * The 'hwpwm' field identifies which hardware output channel (e.g.,
+ * 0 for channel A and 1 for channel B) of the eHRPWM module is in use.
+ */
+ if (pwm->hwpwm == 0) {
+ aqctl_reg = readw(pc->mmio_base + AQCTLA);
+ up_action = FIELD_GET(AQCTL_CAU_MASK, aqctl_reg);
+ down_action = FIELD_GET(AQCTL_CAD_MASK, aqctl_reg);
+ } else {
+ aqctl_reg = readw(pc->mmio_base + AQCTLB);
+ up_action = FIELD_GET(AQCTL_CBU_MASK, aqctl_reg);
+ down_action = FIELD_GET(AQCTL_CBD_MASK, aqctl_reg);
+ }
+
+ /*
+ * Evaluate the actions to determine the PWM polarity:
+ * - If an up-count event sets the output (AQCTL_FRCHIGH) and a down-count
+ * event clears it (AQ_CLEAR), then polarity is NORMAL.
+ * - If an up-count event clears the output (AQ_CLEAR) and a down-count
+ * event sets it (AQCTL_FRCLOW), then polarity is INVERSED.
+ */
+ if (up_action == AQCTL_FRCHIGH && down_action == AQCTL_FRCLOW) {
+ state->polarity = PWM_POLARITY_NORMAL;
+ } else if (up_action == AQCTL_FRCLOW && down_action == AQCTL_FRCHIGH) {
+ state->polarity = PWM_POLARITY_INVERSED;
+ } else {
+ state->polarity = PWM_POLARITY_NORMAL;
+ dev_dbg(&chip->dev, "ehrpwm: unknown polarity bits (0x%x/0x%x), defaulting to NORMAL\n",
+ up_action, down_action);
+ }
+
+ return ret;
+}
+
static const struct pwm_ops ehrpwm_pwm_ops = {
.free = ehrpwm_pwm_free,
.apply = ehrpwm_pwm_apply,
+ .get_state = ehrpwm_get_state,
};
static const struct of_device_id ehrpwm_of_match[] = {
--
2.25.1
Hello Rafael,
On Sat, Apr 19, 2025 at 04:55:55PM -0300, Rafael V. Volkmer wrote:
> The ehrpwm driver was missing a get_state function, which is required
> to properly retrieve the current state of the PWM channel. Add the
> ehrpwm_get_state() function, allowing users to query the enabled state,
> period, duty cycle, and polarity of the PWM output.
>
> Signed-off-by: Rafael V. Volkmer <rafael.v.volkmer@gmail.com>
> ---
> drivers/pwm/pwm-tiehrpwm.c | 97 ++++++++++++++++++++++++++++++++++++++
> 1 file changed, 97 insertions(+)
>
> diff --git a/drivers/pwm/pwm-tiehrpwm.c b/drivers/pwm/pwm-tiehrpwm.c
> index 1ead1aa91a1a..cde331a73696 100644
> --- a/drivers/pwm/pwm-tiehrpwm.c
> +++ b/drivers/pwm/pwm-tiehrpwm.c
> @@ -68,7 +68,9 @@
> #define AQCTL_ZRO_MASK GENMASK(1, 0)
> #define AQCTL_PRD_MASK GENMASK(3, 2)
> #define AQCTL_CAU_MASK GENMASK(5, 4)
> +#define AQCTL_CAD_MASK GENMASK(7, 6)
> #define AQCTL_CBU_MASK GENMASK(9, 8)
> +#define AQCTL_CBD_MASK GENMASK(11, 10)
>
> /* common action codes (2‑bit) */
> #define AQCTL_FRCLOW 1
> @@ -470,9 +472,104 @@ static int ehrpwm_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> return err;
> }
>
> +static int ehrpwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
> + struct pwm_state *state)
> +{
> + int ret = 0;
> +
> + struct ehrpwm_pwm_chip *pc = NULL;
> +
> + /* Registers */
> + u16 aqcsfrc_reg, aqctl_reg, tbprd_reg, cmpa_reg;
> +
> + /* Bits */
> + u8 csf_bits;
> +
> + /* Values */
> + u64 period_cycles, duty_cycles;
> +
> + /* Actions */
> + u8 up_action, down_action;
> +
> + pc = to_ehrpwm_pwm_chip(chip);
> +
> + /*
> + * The 'hwpwm' field identifies which hardware output channel (e.g.,
> + * 0 for channel A and 1 for channel B) of the eHRPWM module is in use.
> + */
> + if (pwm->hwpwm == 0) {
> + aqcsfrc_reg = readw(pc->mmio_base + AQCSFRC);
> + csf_bits = FIELD_GET(AQCSFRC_CSFA_MASK, aqcsfrc_reg);
> + aqctl_reg = readw(pc->mmio_base + AQCTLA);
> + } else {
> + aqcsfrc_reg = readw(pc->mmio_base + AQCSFRC);
> + csf_bits = FIELD_GET(AQCSFRC_CSFB_MASK, aqcsfrc_reg);
> + aqctl_reg = readw(pc->mmio_base + AQCTLB);
> + }
> +
> + if (csf_bits)
> + state->enabled = false;
> + else if (aqctl_reg)
> + state->enabled = true;
> + else
> + state->enabled = false;
if (csf_bits || !aqctl_reg) {
state->enabled = false;
return 0;
}
?
> +
> + tbprd_reg = readw(pc->mmio_base + TBPRD);
> + period_cycles = (u64)tbprd_reg + 1u;
> +
> + /*
> + * period (in ns) = (period_cycles * 1e9) / clk_rate
> + * Using DIV_ROUND_UP_ULL to avoid floating-point operations.
> + */
> + state->period = DIV_ROUND_UP_ULL(period_cycles * NSEC_PER_SEC, pc->clk_rate);
> +
> + cmpa_reg = readw(pc->mmio_base + CMPA);
> + duty_cycles = cmpa_reg;
duty_cycles = readw(pc->mmio_base + CMPA);
and drop the otherwise unused cmpa_reg.
I would expect that you need to read CMPB for hwpwm == 1?
I see the naming is consistent with ehrpwm_pwm_config, but I'd prefer
period_ticks and duty_ticks over period_cycles and duty_cycles. Then
it's clear that the unit is clock ticks and not ns.
> + /*
> + * duty_cycle (in ns) = (duty_cycles * 1e9) / clk_rate
> + * Using DIV_ROUND_UP_ULL to avoid floating-point operations.
> + */
> + state->duty_cycle = DIV_ROUND_UP_ULL(duty_cycles * NSEC_PER_SEC, pc->clk_rate);
> +
> + /*
> + * The 'hwpwm' field identifies which hardware output channel (e.g.,
> + * 0 for channel A and 1 for channel B) of the eHRPWM module is in use.
> + */
> + if (pwm->hwpwm == 0) {
> + aqctl_reg = readw(pc->mmio_base + AQCTLA);
> + up_action = FIELD_GET(AQCTL_CAU_MASK, aqctl_reg);
> + down_action = FIELD_GET(AQCTL_CAD_MASK, aqctl_reg);
> + } else {
> + aqctl_reg = readw(pc->mmio_base + AQCTLB);
> + up_action = FIELD_GET(AQCTL_CBU_MASK, aqctl_reg);
> + down_action = FIELD_GET(AQCTL_CBD_MASK, aqctl_reg);
> + }
When you send a new revision, please start a new thread.
Best regards
Uwe
During probe, if the hardware is already active, it is not guaranteed
that the clock is enabled. To address this, ehrpwm_pwm_probe() now
checks whether the PWM is enabled and ensures that the necessary
resources are initialized.
Signed-off-by: Rafael V. Volkmer <rafael.v.volkmer@gmail.com>
---
drivers/pwm/pwm-tiehrpwm.c | 46 ++++++++++++++++++++++++++++++++++++++
1 file changed, 46 insertions(+)
diff --git a/drivers/pwm/pwm-tiehrpwm.c b/drivers/pwm/pwm-tiehrpwm.c
index cde331a73696..23530d53e177 100644
--- a/drivers/pwm/pwm-tiehrpwm.c
+++ b/drivers/pwm/pwm-tiehrpwm.c
@@ -583,15 +583,50 @@ static int ehrpwm_pwm_probe(struct platform_device *pdev)
{
struct device_node *np = pdev->dev.of_node;
struct ehrpwm_pwm_chip *pc;
+ struct pwm_device *pwm;
+ struct pwm_state state;
struct pwm_chip *chip;
struct clk *clk;
+ bool tbclk_enabled;
int ret;
+ u16 aqcsfrc_reg, aqctl_reg;
+
+ u8 csf_bits;
+
chip = devm_pwmchip_alloc(&pdev->dev, NUM_PWM_CHANNEL, sizeof(*pc));
if (IS_ERR(chip))
return PTR_ERR(chip);
pc = to_ehrpwm_pwm_chip(chip);
+ pwm = &chip->pwms[0];
+
+ if (pwm->hwpwm == 0) {
+ aqcsfrc_reg = readw(pc->mmio_base + AQCSFRC);
+ csf_bits = FIELD_GET(AQCSFRC_CSFA_MASK, aqcsfrc_reg);
+ aqctl_reg = readw(pc->mmio_base + AQCTLA);
+ } else {
+ aqcsfrc_reg = readw(pc->mmio_base + AQCSFRC);
+ csf_bits = FIELD_GET(AQCSFRC_CSFB_MASK, aqcsfrc_reg);
+ aqctl_reg = readw(pc->mmio_base + AQCTLB);
+ }
+
+ if (csf_bits)
+ state.enabled = false;
+ else if (aqctl_reg)
+ state.enabled = true;
+ else
+ state.enabled = false;
+
+ if (state.enabled) {
+ ret = clk_enable(pc->tbclk);
+ if (ret) {
+ dev_err_probe(&pdev->dev, ret, "clk_prepare_enable() failed");
+ goto err_pwmchip_remove;
+ }
+ tbclk_enabled = true;
+ }
+
clk = devm_clk_get(&pdev->dev, "fck");
if (IS_ERR(clk)) {
if (of_device_is_compatible(np, "ti,am33xx-ecap")) {
@@ -626,6 +661,15 @@ static int ehrpwm_pwm_probe(struct platform_device *pdev)
return ret;
}
+ if (state.enabled) {
+ ret = pm_runtime_get_sync(&pdev->dev);
+ if (ret < 0) {
+ dev_err_probe(&pdev->dev, ret, "pm_runtime_get_sync() failed");
+ clk_disable_unprepare(pc->tbclk);
+ goto err_pwmchip_remove;
+ }
+ }
+
ret = pwmchip_add(chip);
if (ret < 0) {
dev_err(&pdev->dev, "pwmchip_add() failed: %d\n", ret);
@@ -637,6 +681,8 @@ static int ehrpwm_pwm_probe(struct platform_device *pdev)
return 0;
+err_pwmchip_remove:
+ pwmchip_remove(chip);
err_clk_unprepare:
clk_unprepare(pc->tbclk);
--
2.25.1Hello Rafael,
On Sat, Apr 19, 2025 at 04:58:30PM -0300, Rafael V. Volkmer wrote:
> During probe, if the hardware is already active, it is not guaranteed
> that the clock is enabled. To address this, ehrpwm_pwm_probe() now
> checks whether the PWM is enabled and ensures that the necessary
> resources are initialized.
>
> Signed-off-by: Rafael V. Volkmer <rafael.v.volkmer@gmail.com>
> ---
> drivers/pwm/pwm-tiehrpwm.c | 46 ++++++++++++++++++++++++++++++++++++++
> 1 file changed, 46 insertions(+)
>
> diff --git a/drivers/pwm/pwm-tiehrpwm.c b/drivers/pwm/pwm-tiehrpwm.c
> index cde331a73696..23530d53e177 100644
> --- a/drivers/pwm/pwm-tiehrpwm.c
> +++ b/drivers/pwm/pwm-tiehrpwm.c
> @@ -583,15 +583,50 @@ static int ehrpwm_pwm_probe(struct platform_device *pdev)
> {
> struct device_node *np = pdev->dev.of_node;
> struct ehrpwm_pwm_chip *pc;
> + struct pwm_device *pwm;
> + struct pwm_state state;
> struct pwm_chip *chip;
> struct clk *clk;
> + bool tbclk_enabled;
> int ret;
>
> + u16 aqcsfrc_reg, aqctl_reg;
> +
> + u8 csf_bits;
> +
> chip = devm_pwmchip_alloc(&pdev->dev, NUM_PWM_CHANNEL, sizeof(*pc));
> if (IS_ERR(chip))
> return PTR_ERR(chip);
> pc = to_ehrpwm_pwm_chip(chip);
>
> + pwm = &chip->pwms[0];
> +
> + if (pwm->hwpwm == 0) {
This is always true. And actually a lowlevel driver shouldn't need
chip->pwms at all.
> + aqcsfrc_reg = readw(pc->mmio_base + AQCSFRC);
> + csf_bits = FIELD_GET(AQCSFRC_CSFA_MASK, aqcsfrc_reg);
> + aqctl_reg = readw(pc->mmio_base + AQCTLA);
> + } else {
> + aqcsfrc_reg = readw(pc->mmio_base + AQCSFRC);
> + csf_bits = FIELD_GET(AQCSFRC_CSFB_MASK, aqcsfrc_reg);
> + aqctl_reg = readw(pc->mmio_base + AQCTLB);
> + }
> +
> + if (csf_bits)
> + state.enabled = false;
> + else if (aqctl_reg)
> + state.enabled = true;
> + else
> + state.enabled = false;
> +
> + if (state.enabled) {
> + ret = clk_enable(pc->tbclk);
> + if (ret) {
> + dev_err_probe(&pdev->dev, ret, "clk_prepare_enable() failed");
> + goto err_pwmchip_remove;
> + }
> + tbclk_enabled = true;
> + }
You don't check the 2nd PWM, this is needed however. You even need to
call pm_runtime_get_sync() twice if both PWMs are on.
> clk = devm_clk_get(&pdev->dev, "fck");
> if (IS_ERR(clk)) {
> if (of_device_is_compatible(np, "ti,am33xx-ecap")) {
> @@ -626,6 +661,15 @@ static int ehrpwm_pwm_probe(struct platform_device *pdev)
> return ret;
> }
>
> + if (state.enabled) {
> + ret = pm_runtime_get_sync(&pdev->dev);
> + if (ret < 0) {
> + dev_err_probe(&pdev->dev, ret, "pm_runtime_get_sync() failed");
> + clk_disable_unprepare(pc->tbclk);
> + goto err_pwmchip_remove;
> + }
indention inconsistency.
> + }
> +
> ret = pwmchip_add(chip);
> if (ret < 0) {
> dev_err(&pdev->dev, "pwmchip_add() failed: %d\n", ret);
> @@ -637,6 +681,8 @@ static int ehrpwm_pwm_probe(struct platform_device *pdev)
>
> return 0;
>
> +err_pwmchip_remove:
> + pwmchip_remove(chip);
That's wrong. When the `goto err_pwmchip_remove` is taken, pwmchip_add
wasn't called yet.
> err_clk_unprepare:
> clk_unprepare(pc->tbclk);
>
Best regards
Uwe
The comparison in pwm-tiehrpwm.c triggered “UNNECESSARY_PARENTHESES” and
“PARENTHESIS_ALIGNMENT” when wrapping the second clause in extra parens.
This removes superfluous parentheses, aligns continued lines under the ‘if’,
and ensures operators are properly spaced.
Signed-off-by: Rafael V. Volkmer <rafael.v.volkmer@gmail.com>
---
drivers/pwm/pwm-tiehrpwm.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/drivers/pwm/pwm-tiehrpwm.c b/drivers/pwm/pwm-tiehrpwm.c
index 23530d53e177..73c3dd57a50b 100644
--- a/drivers/pwm/pwm-tiehrpwm.c
+++ b/drivers/pwm/pwm-tiehrpwm.c
@@ -285,8 +285,7 @@ static int ehrpwm_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
* same period register for multiple channels.
*/
for (i = 0; i < NUM_PWM_CHANNEL; i++) {
- if (pc->period_cycles[i] &&
- (pc->period_cycles[i] != period_cycles)) {
+ if (pc->period_cycles[i] && pc->period_cycles[i] != period_cycles) {
/*
* Allow channel to reconfigure period if no other
* channels being configured.
@@ -304,7 +303,7 @@ static int ehrpwm_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
pc->period_cycles[pwm->hwpwm] = period_cycles;
/* Configure clock prescaler to support Low frequency PWM wave */
- if (set_prescale_div(period_cycles/PERIOD_MAX, &ps_divval,
+ if (set_prescale_div(period_cycles / PERIOD_MAX, &ps_divval,
&tb_divval)) {
dev_err(pwmchip_parent(chip), "Unsupported values\n");
return -EINVAL;
--
2.25.1
On Sat, Apr 19, 2025 at 05:01:00PM -0300, Rafael V. Volkmer wrote: > The comparison in pwm-tiehrpwm.c triggered “UNNECESSARY_PARENTHESES” and > “PARENTHESIS_ALIGNMENT” when wrapping the second clause in extra parens. What is the source of "UNNECESSARY_PARENTHESES" and "PARENTHESIS_ALIGNMENT"? > This removes superfluous parentheses, aligns continued lines under the ‘if’, The "aligns continued lines under the ‘if’" part is wrong? > and ensures operators are properly spaced. > > Signed-off-by: Rafael V. Volkmer <rafael.v.volkmer@gmail.com> Best regards Uwe
© 2016 - 2025 Red Hat, Inc.