[PATCH 2/2] pwm: correct pwm->state.enabled handling to allow fops control

[PATCH 2/2] pwm: correct pwm->state.enabled handling to allow fops control

[Rafael V. Volkmer]

Ensure pwm->state.enabled is consistently updated during enable and
disable operations in ehrpwm_pwm_apply() to resolve this issue.

Previously, when attempting to interact with the ti PWM driver through
fops, the pwm->state.enabled field was not updated correctly after
applying enable or disable. This led to a state mismatch where the
driver's state detection logic prevented disabling the PWM through
fops once it had been activated.

Signed-off-by: Rafael V. Volkmer <rafael.v.volkmer@gmail.com>
---
 drivers/pwm/pwm-tiehrpwm.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/pwm/pwm-tiehrpwm.c b/drivers/pwm/pwm-tiehrpwm.c
index 0125e73b98df..9f939d535440 100644
--- a/drivers/pwm/pwm-tiehrpwm.c
+++ b/drivers/pwm/pwm-tiehrpwm.c
@@ -420,6 +420,7 @@ static int ehrpwm_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 	if (!state->enabled) {
 		if (enabled)
 			ehrpwm_pwm_disable(chip, pwm);
+			pwm->state.enabled = false;
 		return 0;
 	}
 
@@ -429,6 +430,7 @@ static int ehrpwm_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 
 	if (!enabled)
 		err = ehrpwm_pwm_enable(chip, pwm);
+		pwm->state.enabled = true;
 
 	return err;
 }
-- 
2.25.1

Re: [PATCH 2/2] pwm: correct pwm->state.enabled handling to allow fops control

[Uwe Kleine-König]

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

Hello,

On Tue, Nov 26, 2024 at 06:24:14PM -0300, Rafael V. Volkmer wrote:
> Ensure pwm->state.enabled is consistently updated during enable and
> disable operations in ehrpwm_pwm_apply() to resolve this issue.
> 
> Previously, when attempting to interact with the ti PWM driver through
> fops, the pwm->state.enabled field was not updated correctly after
> applying enable or disable. This led to a state mismatch where the
> driver's state detection logic prevented disabling the PWM through
> fops once it had been activated.
> 
> Signed-off-by: Rafael V. Volkmer <rafael.v.volkmer@gmail.com>
> ---
>  drivers/pwm/pwm-tiehrpwm.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/pwm/pwm-tiehrpwm.c b/drivers/pwm/pwm-tiehrpwm.c
> index 0125e73b98df..9f939d535440 100644
> --- a/drivers/pwm/pwm-tiehrpwm.c
> +++ b/drivers/pwm/pwm-tiehrpwm.c
> @@ -420,6 +420,7 @@ static int ehrpwm_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>  	if (!state->enabled) {
>  		if (enabled)
>  			ehrpwm_pwm_disable(chip, pwm);
> +			pwm->state.enabled = false;

This is a layer violation (pwm->state is under control of core.c only)
and uses irritating indention.

Please rethink!

Best regards
Uwe

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

[PATCH v2] pwm: improve state handling in ehrpwm driver

[Rafael V. Volkmer]

Introduce ehrpwm_is_enabled() to verify the module's enable/disable state
by checking the AQCSFRC and AQCTLA registers, instead of relying solely
on pwm->state.enabled. This ensures a more accurate representation of
the ePWM state in the kernel.

Previously, when performing fops operations directly in kernel space
after retrieving the platform device (pdev)—bypassing the sysfs interface—
pwm->state.enabled could incorrectly signal transitions between active
and inactive states, leading to inconsistent behavior.

Signed-off-by: Rafael V. Volkmer <rafael.v.volkmer@gmail.com>
---
Hello Uwe,

Thank you very much for the feedback.

I understand your concern about the kernel layer structure, so I took 
a new approach that also fixes my problem, but this time without directly 
manipulating `pwm->state`, but rather through a double check of the registers
within `apply`.

I hope you can see the implementation and tell me, again, what you think.

Best regards

Changes in v2:
- Implemented `ehrpwm_is_enabled()` to check hardware registers instead of relying on `pwm->state.enabled`.
- Removed direct manipulation of `pwm->state` in `ehrpwm_pwm_apply()`.
- Addressed your feedback regarding kernel layer structure.

 drivers/pwm/pwm-tiehrpwm.c | 34 +++++++++++++++++++++++++++-------
 1 file changed, 27 insertions(+), 7 deletions(-)

diff --git a/drivers/pwm/pwm-tiehrpwm.c b/drivers/pwm/pwm-tiehrpwm.c
index 9f939d535440..fdcda0ffc9db 100644
--- a/drivers/pwm/pwm-tiehrpwm.c
+++ b/drivers/pwm/pwm-tiehrpwm.c
@@ -387,6 +387,25 @@ static void ehrpwm_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
 	pm_runtime_put_sync(pwmchip_parent(chip));
 }
 
+static bool ehrpwm_is_enabled(struct pwm_chip *chip)
+{
+	struct ehrpwm_pwm_chip *pc = to_ehrpwm_pwm_chip(chip);
+	bool ret;
+	u16 aqcsfrc_reg;
+	u16 aqctla_reg;
+	u8 csfa_bits;
+	
+	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 void ehrpwm_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
 {
 	struct ehrpwm_pwm_chip *pc = to_ehrpwm_pwm_chip(chip);
@@ -404,7 +423,9 @@ static int ehrpwm_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 			    const struct pwm_state *state)
 {
 	int err;
-	bool enabled = pwm->state.enabled;
+	bool enabled;
+
+	enabled = (ehrpwm_is_enabled(chip) | pwm->state.enabled);
 
 	if (state->polarity != pwm->state.polarity) {
 		if (enabled) {
@@ -417,10 +438,8 @@ static int ehrpwm_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 			return err;
 	}
 
-	if (!state->enabled) {
-		if (enabled)
-			ehrpwm_pwm_disable(chip, pwm);
+	if ((state->enabled != enabled) && (state->enabled == false)) {
+		ehrpwm_pwm_disable(chip, pwm);
 		return 0;
 	}
 
@@ -428,9 +447,10 @@ static int ehrpwm_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 	if (err)
 		return err;
 
-	if (!enabled)
+	if ((state->enabled != enabled) && (state->enabled == true)) {
 		err = ehrpwm_pwm_enable(chip, pwm);
+		return err;
+	}
 
 	return err;
 }
-- 
2.25.1

Re: [PATCH v2] pwm: improve state handling in ehrpwm driver

[Uwe Kleine-König]

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

Hello,

Subject should use "pwm: tiehrpwm:" as prefix, then drop "in ehrpwm
driver".

On Wed, Nov 27, 2024 at 07:26:49PM -0300, Rafael V. Volkmer wrote:
> Introduce ehrpwm_is_enabled() to verify the module's enable/disable state
> by checking the AQCSFRC and AQCTLA registers, instead of relying solely
> on pwm->state.enabled. This ensures a more accurate representation of
> the ePWM state in the kernel.
> 
> Previously, when performing fops operations directly in kernel space
> after retrieving the platform device (pdev)—bypassing the sysfs interface—
> pwm->state.enabled could incorrectly signal transitions between active
> and inactive states, leading to inconsistent behavior.

I don't see a problem in this driver but I wonder what you do trigger
the problem you're describing. Are you saying some other driver calls
ehrpwm_pwm_apply() directly bypassing the pwm core? If so: Don't.

Otherwise I'd claim the only place where state.enabled can get out of
sync is during .probe(). The driver might need something like:

	if (hw_is_enabled) {
		clk_enable(...)
		pm_runtime_get(...)
	}

there and an implementation of .get_state(). BTW, .free() looks wrong; a
driver isn't supposed to make up for a consumer failing to disable the
hardware before it releases the pwm handle (but that's orthogonal to
your problem I think).

Am I missing something?

> [...]
>  static void ehrpwm_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
>  {
>  	struct ehrpwm_pwm_chip *pc = to_ehrpwm_pwm_chip(chip);
> @@ -404,7 +423,9 @@ static int ehrpwm_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>  			    const struct pwm_state *state)
>  {
>  	int err;
> -	bool enabled = pwm->state.enabled;
> +	bool enabled;
> +
> +	enabled = (ehrpwm_is_enabled(chip) | pwm->state.enabled);

|| is the right operator for bools. No parenthesis are needed.

ehrpwm_is_enabled() should never return something different than
pwm->state.enabled. This is the thing you need to address. Don't paper
over this.

Best regards
Uwe

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

[PATCH] pwm: tiehrpwm: ensures that state.enabled is synchronized during .probe()

[Rafael V. Volkmer]

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>
---

Hi, Uwe!

Thanks again for the feedback!

I have taken your findings into consideration again and am working on 
getting my application up and running. Regarding the points you mentioned 
earlier about the driver, I made this small patch, using some hardware 
validation functions I had in my possession, to check for occasionality.

Best regards,
Rafael V. Volkmer

 drivers/pwm/pwm-tiehrpwm.c | 162 ++++++++++++++++++++++++++++++++++++-
 1 file changed, 161 insertions(+), 1 deletion(-)

diff --git a/drivers/pwm/pwm-tiehrpwm.c b/drivers/pwm/pwm-tiehrpwm.c
index 0125e73b98df..693704406f25 100644
--- a/drivers/pwm/pwm-tiehrpwm.c
+++ b/drivers/pwm/pwm-tiehrpwm.c
@@ -91,6 +91,15 @@
 #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  4U
+
+#define AQCTLA_CAD_MASK   (BIT(7) | BIT(6)) 
+#define AQCTLA_CAD_SHIFT  6U
+
+#define AQ_SET    0x1
+#define AQ_CLEAR  0x2
+
 #define NUM_PWM_CHANNEL		2	/* EHRPWM channels */
 
 struct ehrpwm_context {
@@ -400,6 +409,134 @@ static void ehrpwm_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
 	pc->period_cycles[pwm->hwpwm] = 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 = (duty_cycles * 1e9) / tblck_rate */
+	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;
+
+	ret = (cau_action == AQ_SET && cad_action == AQ_CLEAR) ? PWM_POLARITY_NORMAL :
+		  (cau_action == AQ_CLEAR && cad_action == AQ_SET) ? PWM_POLARITY_INVERSED : PWM_POLARITY_NORMAL;
+
+	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 int ehrpwm_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 			    const struct pwm_state *state)
 {
@@ -450,6 +587,7 @@ static int ehrpwm_pwm_probe(struct platform_device *pdev)
 	struct device_node *np = pdev->dev.of_node;
 	struct ehrpwm_pwm_chip *pc;
 	struct pwm_chip *chip;
+	bool tbclk_enabled;
 	struct clk *clk;
 	int ret;
 
@@ -501,10 +639,32 @@ static int ehrpwm_pwm_probe(struct platform_device *pdev)
 	platform_set_drvdata(pdev, chip);
 	pm_runtime_enable(&pdev->dev);
 
+	ehrpwm_get_hw_state(&pc->chip, &pc->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(&pc->chip);
 err_clk_unprepare:
-	clk_unprepare(pc->tbclk);
+	if(tbclk_enabled)
+		clk_unprepare(pc->tbclk);
 
 	return ret;
 }
-- 
2.25.1

Re: [PATCH] pwm: tiehrpwm: ensures that state.enabled is synchronized during .probe()

[kernel test robot]

Hi Rafael,

kernel test robot noticed the following build errors:

[auto build test ERROR on linus/master]
[also build test ERROR on v6.12 next-20241128]
[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-during-probe/20241129-114649
base:   linus/master
patch link:    https://lore.kernel.org/r/20241129034334.27203-1-rafael.v.volkmer%40gmail.com
patch subject: [PATCH] pwm: tiehrpwm: ensures that state.enabled is synchronized during .probe()
config: alpha-randconfig-r053-20241129 (https://download.01.org/0day-ci/archive/20241129/202411291940.6T4OMy3k-lkp@intel.com/config)
compiler: alpha-linux-gcc (GCC) 14.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241129/202411291940.6T4OMy3k-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/202411291940.6T4OMy3k-lkp@intel.com/

All errors (new ones prefixed by >>):

   drivers/pwm/pwm-tiehrpwm.c: In function 'ehrpwm_pwm_probe':
>> drivers/pwm/pwm-tiehrpwm.c:642:32: error: 'struct ehrpwm_pwm_chip' has no member named 'chip'
     642 |         ehrpwm_get_hw_state(&pc->chip, &pc->chip.pwms[0], &state);
         |                                ^~
   drivers/pwm/pwm-tiehrpwm.c:642:43: error: 'struct ehrpwm_pwm_chip' has no member named 'chip'
     642 |         ehrpwm_get_hw_state(&pc->chip, &pc->chip.pwms[0], &state);
         |                                           ^~
>> drivers/pwm/pwm-tiehrpwm.c:642:60: error: 'state' undeclared (first use in this function); did you mean 'statx'?
     642 |         ehrpwm_get_hw_state(&pc->chip, &pc->chip.pwms[0], &state);
         |                                                            ^~~~~
         |                                                            statx
   drivers/pwm/pwm-tiehrpwm.c:642:60: note: each undeclared identifier is reported only once for each function it appears in
   drivers/pwm/pwm-tiehrpwm.c:664:27: error: 'struct ehrpwm_pwm_chip' has no member named 'chip'
     664 |         pwmchip_remove(&pc->chip);
         |                           ^~


vim +642 drivers/pwm/pwm-tiehrpwm.c

   584	
   585	static int ehrpwm_pwm_probe(struct platform_device *pdev)
   586	{
   587		struct device_node *np = pdev->dev.of_node;
   588		struct ehrpwm_pwm_chip *pc;
   589		struct pwm_chip *chip;
   590		bool tbclk_enabled;
   591		struct clk *clk;
   592		int ret;
   593	
   594		chip = devm_pwmchip_alloc(&pdev->dev, NUM_PWM_CHANNEL, sizeof(*pc));
   595		if (IS_ERR(chip))
   596			return PTR_ERR(chip);
   597		pc = to_ehrpwm_pwm_chip(chip);
   598	
   599		clk = devm_clk_get(&pdev->dev, "fck");
   600		if (IS_ERR(clk)) {
   601			if (of_device_is_compatible(np, "ti,am33xx-ecap")) {
   602				dev_warn(&pdev->dev, "Binding is obsolete.\n");
   603				clk = devm_clk_get(pdev->dev.parent, "fck");
   604			}
   605		}
   606	
   607		if (IS_ERR(clk))
   608			return dev_err_probe(&pdev->dev, PTR_ERR(clk), "Failed to get fck\n");
   609	
   610		pc->clk_rate = clk_get_rate(clk);
   611		if (!pc->clk_rate) {
   612			dev_err(&pdev->dev, "failed to get clock rate\n");
   613			return -EINVAL;
   614		}
   615	
   616		chip->ops = &ehrpwm_pwm_ops;
   617	
   618		pc->mmio_base = devm_platform_ioremap_resource(pdev, 0);
   619		if (IS_ERR(pc->mmio_base))
   620			return PTR_ERR(pc->mmio_base);
   621	
   622		/* Acquire tbclk for Time Base EHRPWM submodule */
   623		pc->tbclk = devm_clk_get(&pdev->dev, "tbclk");
   624		if (IS_ERR(pc->tbclk))
   625			return dev_err_probe(&pdev->dev, PTR_ERR(pc->tbclk), "Failed to get tbclk\n");
   626	
   627		ret = clk_prepare(pc->tbclk);
   628		if (ret < 0) {
   629			dev_err(&pdev->dev, "clk_prepare() failed: %d\n", ret);
   630			return ret;
   631		}
   632	
   633		ret = pwmchip_add(chip);
   634		if (ret < 0) {
   635			dev_err(&pdev->dev, "pwmchip_add() failed: %d\n", ret);
   636			goto err_clk_unprepare;
   637		}
   638	
   639		platform_set_drvdata(pdev, chip);
   640		pm_runtime_enable(&pdev->dev);
   641	
 > 642		ehrpwm_get_hw_state(&pc->chip, &pc->chip.pwms[0], &state);
   643	
   644		if(state.enabled == true) {
   645			ret = clk_prepare_enable(pc->tbclk);
   646			if (ret) {	
   647				dev_err(&pdev->dev, "clk_prepare_enable() failed: %d\n", ret);
   648				goto err_pwmchip_remove;
   649			}
   650			
   651			tbclk_enabled = true;
   652	
   653			ret = pm_runtime_get_sync(&pdev->dev);
   654			if(ret < 0) {
   655				dev_err(&pdev->dev, "pm_runtime_get_sync() failed: %d\n", ret);
   656				clk_disable_unprepare(pc->tbclk);
   657				goto err_pwmchip_remove;
   658			}
   659		}
   660	
   661		return 0;
   662	
   663	err_pwmchip_remove:
   664		pwmchip_remove(&pc->chip);
   665	err_clk_unprepare:
   666		if(tbclk_enabled)
   667			clk_unprepare(pc->tbclk);
   668	
   669		return ret;
   670	}
   671	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH] pwm: tiehrpwm: ensures that state.enabled is synchronized during .probe()

[kernel test robot]

Hi Rafael,

kernel test robot noticed the following build errors:

[auto build test ERROR on linus/master]
[also build test ERROR on v6.12 next-20241128]
[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-during-probe/20241129-114649
base:   linus/master
patch link:    https://lore.kernel.org/r/20241129034334.27203-1-rafael.v.volkmer%40gmail.com
patch subject: [PATCH] pwm: tiehrpwm: ensures that state.enabled is synchronized during .probe()
config: x86_64-buildonly-randconfig-002-20241129 (https://download.01.org/0day-ci/archive/20241130/202411300250.wkH4Isc0-lkp@intel.com/config)
compiler: clang version 19.1.3 (https://github.com/llvm/llvm-project ab51eccf88f5321e7c60591c5546b254b6afab99)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241130/202411300250.wkH4Isc0-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/202411300250.wkH4Isc0-lkp@intel.com/

All error/warnings (new ones prefixed by >>):

>> drivers/pwm/pwm-tiehrpwm.c:537:9: warning: variable 'ret' is uninitialized when used here [-Wuninitialized]
     537 |         return ret;
         |                ^~~
   drivers/pwm/pwm-tiehrpwm.c:525:9: note: initialize the variable 'ret' to silence this warning
     525 |         int ret;
         |                ^
         |                 = 0
>> drivers/pwm/pwm-tiehrpwm.c:642:27: error: no member named 'chip' in 'struct ehrpwm_pwm_chip'
     642 |         ehrpwm_get_hw_state(&pc->chip, &pc->chip.pwms[0], &state);
         |                              ~~  ^
   drivers/pwm/pwm-tiehrpwm.c:642:38: error: no member named 'chip' in 'struct ehrpwm_pwm_chip'
     642 |         ehrpwm_get_hw_state(&pc->chip, &pc->chip.pwms[0], &state);
         |                                         ~~  ^
>> drivers/pwm/pwm-tiehrpwm.c:642:53: error: use of undeclared identifier 'state'; did you mean 'stac'?
     642 |         ehrpwm_get_hw_state(&pc->chip, &pc->chip.pwms[0], &state);
         |                                                            ^~~~~
         |                                                            stac
   arch/x86/include/asm/smap.h:36:29: note: 'stac' declared here
      36 | static __always_inline void stac(void)
         |                             ^
   drivers/pwm/pwm-tiehrpwm.c:644:5: error: use of undeclared identifier 'state'
     644 |         if(state.enabled == true) {
         |            ^
   drivers/pwm/pwm-tiehrpwm.c:664:22: error: no member named 'chip' in 'struct ehrpwm_pwm_chip'
     664 |         pwmchip_remove(&pc->chip);
         |                         ~~  ^
   1 warning and 5 errors generated.


vim +642 drivers/pwm/pwm-tiehrpwm.c

   521	
   522	static int ehrpwm_get_hw_state(struct pwm_chip *chip, struct pwm_device *pwm, 
   523									struct pwm_state *state)
   524	{
   525		int ret;
   526	
   527		if(chip == NULL || pwm == NULL || state == NULL){
   528			return -EINVAL;
   529		}
   530	
   531		state->enabled = ehrpwm_is_enabled(chip);
   532	
   533		state->period = ehrpwm_read_period(chip);
   534	    state->duty_cycle = ehrpwm_read_duty_cycle(chip);
   535	    state->polarity = ehrpwm_read_polarity(chip);
   536	
 > 537		return ret;
   538	}
   539	
   540	static int ehrpwm_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
   541				    const struct pwm_state *state)
   542	{
   543		int err;
   544		bool enabled = pwm->state.enabled;
   545	
   546		if (state->polarity != pwm->state.polarity) {
   547			if (enabled) {
   548				ehrpwm_pwm_disable(chip, pwm);
   549				enabled = false;
   550			}
   551	
   552			err = ehrpwm_pwm_set_polarity(chip, pwm, state->polarity);
   553			if (err)
   554				return err;
   555		}
   556	
   557		if (!state->enabled) {
   558			if (enabled)
   559				ehrpwm_pwm_disable(chip, pwm);
   560			return 0;
   561		}
   562	
   563		err = ehrpwm_pwm_config(chip, pwm, state->duty_cycle, state->period);
   564		if (err)
   565			return err;
   566	
   567		if (!enabled)
   568			err = ehrpwm_pwm_enable(chip, pwm);
   569	
   570		return err;
   571	}
   572	
   573	static const struct pwm_ops ehrpwm_pwm_ops = {
   574		.free = ehrpwm_pwm_free,
   575		.apply = ehrpwm_pwm_apply,
   576	};
   577	
   578	static const struct of_device_id ehrpwm_of_match[] = {
   579		{ .compatible = "ti,am3352-ehrpwm" },
   580		{ .compatible = "ti,am33xx-ehrpwm" },
   581		{},
   582	};
   583	MODULE_DEVICE_TABLE(of, ehrpwm_of_match);
   584	
   585	static int ehrpwm_pwm_probe(struct platform_device *pdev)
   586	{
   587		struct device_node *np = pdev->dev.of_node;
   588		struct ehrpwm_pwm_chip *pc;
   589		struct pwm_chip *chip;
   590		bool tbclk_enabled;
   591		struct clk *clk;
   592		int ret;
   593	
   594		chip = devm_pwmchip_alloc(&pdev->dev, NUM_PWM_CHANNEL, sizeof(*pc));
   595		if (IS_ERR(chip))
   596			return PTR_ERR(chip);
   597		pc = to_ehrpwm_pwm_chip(chip);
   598	
   599		clk = devm_clk_get(&pdev->dev, "fck");
   600		if (IS_ERR(clk)) {
   601			if (of_device_is_compatible(np, "ti,am33xx-ecap")) {
   602				dev_warn(&pdev->dev, "Binding is obsolete.\n");
   603				clk = devm_clk_get(pdev->dev.parent, "fck");
   604			}
   605		}
   606	
   607		if (IS_ERR(clk))
   608			return dev_err_probe(&pdev->dev, PTR_ERR(clk), "Failed to get fck\n");
   609	
   610		pc->clk_rate = clk_get_rate(clk);
   611		if (!pc->clk_rate) {
   612			dev_err(&pdev->dev, "failed to get clock rate\n");
   613			return -EINVAL;
   614		}
   615	
   616		chip->ops = &ehrpwm_pwm_ops;
   617	
   618		pc->mmio_base = devm_platform_ioremap_resource(pdev, 0);
   619		if (IS_ERR(pc->mmio_base))
   620			return PTR_ERR(pc->mmio_base);
   621	
   622		/* Acquire tbclk for Time Base EHRPWM submodule */
   623		pc->tbclk = devm_clk_get(&pdev->dev, "tbclk");
   624		if (IS_ERR(pc->tbclk))
   625			return dev_err_probe(&pdev->dev, PTR_ERR(pc->tbclk), "Failed to get tbclk\n");
   626	
   627		ret = clk_prepare(pc->tbclk);
   628		if (ret < 0) {
   629			dev_err(&pdev->dev, "clk_prepare() failed: %d\n", ret);
   630			return ret;
   631		}
   632	
   633		ret = pwmchip_add(chip);
   634		if (ret < 0) {
   635			dev_err(&pdev->dev, "pwmchip_add() failed: %d\n", ret);
   636			goto err_clk_unprepare;
   637		}
   638	
   639		platform_set_drvdata(pdev, chip);
   640		pm_runtime_enable(&pdev->dev);
   641	
 > 642		ehrpwm_get_hw_state(&pc->chip, &pc->chip.pwms[0], &state);
   643	
   644		if(state.enabled == true) {
   645			ret = clk_prepare_enable(pc->tbclk);
   646			if (ret) {	
   647				dev_err(&pdev->dev, "clk_prepare_enable() failed: %d\n", ret);
   648				goto err_pwmchip_remove;
   649			}
   650			
   651			tbclk_enabled = true;
   652	
   653			ret = pm_runtime_get_sync(&pdev->dev);
   654			if(ret < 0) {
   655				dev_err(&pdev->dev, "pm_runtime_get_sync() failed: %d\n", ret);
   656				clk_disable_unprepare(pc->tbclk);
   657				goto err_pwmchip_remove;
   658			}
   659		}
   660	
   661		return 0;
   662	
   663	err_pwmchip_remove:
   664		pwmchip_remove(&pc->chip);
   665	err_clk_unprepare:
   666		if(tbclk_enabled)
   667			clk_unprepare(pc->tbclk);
   668	
   669		return ret;
   670	}
   671	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki