Hello,
here come some simplifications for the cros-ec PWM driver and a followup
change in the core. The latter was the motivator for me, as I intend to
work on some changes how PWM wave forms are represented and this reduces
the number of code locations I have to care for.
I claim in the commit log that the changes work just fine, however I
only build-test them, so a runtime test from someone with a cros-ec
device would be very welcome.
Best regards
Uwe
Uwe Kleine-König (3):
pwm: cros-ec: Don't care about consumers in .get_state()
pwm: cros-ec: Simplify device tree xlation
pwm: Make pwm_request_from_chip() private to the core
drivers/pwm/core.c | 8 ++---
drivers/pwm/pwm-cros-ec.c | 63 ++++++++-------------------------------
include/linux/pwm.h | 12 --------
3 files changed, 15 insertions(+), 68 deletions(-)
base-commit: 1613e604df0cd359cf2a7fbd9be7a0bcfacfabd0
--
2.43.0
The get_state() callback is never called (in a visible way) after there
is a consumer for a pwm device. The core handles loosing the information
about duty_cycle just fine.
Simplify the driver accordingly.
Signed-off-by: Uwe Kleine-König <[email protected]>
---
drivers/pwm/pwm-cros-ec.c | 33 +--------------------------------
1 file changed, 1 insertion(+), 32 deletions(-)
diff --git a/drivers/pwm/pwm-cros-ec.c b/drivers/pwm/pwm-cros-ec.c
index 606ccfdaf4cc..ba4ee0b507b7 100644
--- a/drivers/pwm/pwm-cros-ec.c
+++ b/drivers/pwm/pwm-cros-ec.c
@@ -25,15 +25,6 @@
struct cros_ec_pwm_device {
struct cros_ec_device *ec;
bool use_pwm_type;
- struct cros_ec_pwm *channel;
-};
-
-/**
- * struct cros_ec_pwm - per-PWM driver data
- * @duty_cycle: cached duty cycle
- */
-struct cros_ec_pwm {
- u16 duty_cycle;
};
static inline struct cros_ec_pwm_device *pwm_to_cros_ec_pwm(struct pwm_chip *chip)
@@ -135,7 +126,6 @@ static int cros_ec_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
const struct pwm_state *state)
{
struct cros_ec_pwm_device *ec_pwm = pwm_to_cros_ec_pwm(chip);
- struct cros_ec_pwm *channel = &ec_pwm->channel[pwm->hwpwm];
u16 duty_cycle;
int ret;
@@ -156,8 +146,6 @@ static int cros_ec_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
if (ret < 0)
return ret;
- channel->duty_cycle = state->duty_cycle;
-
return 0;
}
@@ -165,7 +153,6 @@ static int cros_ec_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
struct pwm_state *state)
{
struct cros_ec_pwm_device *ec_pwm = pwm_to_cros_ec_pwm(chip);
- struct cros_ec_pwm *channel = &ec_pwm->channel[pwm->hwpwm];
int ret;
ret = cros_ec_pwm_get_duty(ec_pwm->ec, ec_pwm->use_pwm_type, pwm->hwpwm);
@@ -175,23 +162,10 @@ static int cros_ec_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
}
state->enabled = (ret > 0);
+ state->duty_cycle = ret;
state->period = EC_PWM_MAX_DUTY;
state->polarity = PWM_POLARITY_NORMAL;
- /*
- * Note that "disabled" and "duty cycle == 0" are treated the same. If
- * the cached duty cycle is not zero, used the cached duty cycle. This
- * ensures that the configured duty cycle is kept across a disable and
- * enable operation and avoids potentially confusing consumers.
- *
- * For the case of the initial hardware readout, channel->duty_cycle
- * will be 0 and the actual duty cycle read from the EC is used.
- */
- if (ret == 0 && channel->duty_cycle > 0)
- state->duty_cycle = channel->duty_cycle;
- else
- state->duty_cycle = ret;
-
return 0;
}
@@ -291,11 +265,6 @@ static int cros_ec_pwm_probe(struct platform_device *pdev)
chip->ops = &cros_ec_pwm_ops;
chip->of_xlate = cros_ec_pwm_xlate;
- ec_pwm->channel = devm_kcalloc(dev, chip->npwm, sizeof(*ec_pwm->channel),
- GFP_KERNEL);
- if (!ec_pwm->channel)
- return -ENOMEM;
-
dev_dbg(dev, "Probed %u PWMs\n", chip->npwm);
ret = devm_pwmchip_add(dev, chip);
--
2.43.0
The last user of this function outside of core.c is gone, so it can be
made static.
Signed-off-by: Uwe Kleine-König <[email protected]>
---
drivers/pwm/core.c | 8 +++-----
include/linux/pwm.h | 12 ------------
2 files changed, 3 insertions(+), 17 deletions(-)
diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index 18574857641e..76969d5052af 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c
@@ -394,9 +394,9 @@ static int pwm_device_request(struct pwm_device *pwm, const char *label)
* chip. A negative error code is returned if the index is not valid for the
* specified PWM chip or if the PWM device cannot be requested.
*/
-struct pwm_device *pwm_request_from_chip(struct pwm_chip *chip,
- unsigned int index,
- const char *label)
+static struct pwm_device *pwm_request_from_chip(struct pwm_chip *chip,
+ unsigned int index,
+ const char *label)
{
struct pwm_device *pwm;
int err;
@@ -414,8 +414,6 @@ struct pwm_device *pwm_request_from_chip(struct pwm_chip *chip,
mutex_unlock(&pwm_lock);
return pwm;
}
-EXPORT_SYMBOL_GPL(pwm_request_from_chip);
-
struct pwm_device *
of_pwm_xlate_with_flags(struct pwm_chip *chip, const struct of_phandle_args *args)
diff --git a/include/linux/pwm.h b/include/linux/pwm.h
index 60b92c2c75ef..3ac1a04acc0e 100644
--- a/include/linux/pwm.h
+++ b/include/linux/pwm.h
@@ -407,10 +407,6 @@ void pwmchip_remove(struct pwm_chip *chip);
int __devm_pwmchip_add(struct device *dev, struct pwm_chip *chip, struct module *owner);
#define devm_pwmchip_add(dev, chip) __devm_pwmchip_add(dev, chip, THIS_MODULE)
-struct pwm_device *pwm_request_from_chip(struct pwm_chip *chip,
- unsigned int index,
- const char *label);
-
struct pwm_device *of_pwm_xlate_with_flags(struct pwm_chip *chip,
const struct of_phandle_args *args);
struct pwm_device *of_pwm_single_xlate(struct pwm_chip *chip,
@@ -505,14 +501,6 @@ static inline int devm_pwmchip_add(struct device *dev, struct pwm_chip *chip)
return -EINVAL;
}
-static inline struct pwm_device *pwm_request_from_chip(struct pwm_chip *chip,
- unsigned int index,
- const char *label)
-{
- might_sleep();
- return ERR_PTR(-ENODEV);
-}
-
static inline struct pwm_device *pwm_get(struct device *dev,
const char *consumer)
{
--
2.43.0
The cros-ec device tree binding only uses #pwm-cells = <1>, and so there
is no period provided in the device tree. Up to now this was handled by
hardcoding the period to the only supported value in the custom xlate
callback. Apart from that, the default xlate callback (i.e.
of_pwm_xlate_with_flags()) handles this just fine (and better, e.g. by
checking args->args_count >= 1 before accessing args->args[0]).
To simplify make use of of_pwm_xlate_with_flags(), drop the custom
callback and provide the default period in .probe() already.
Apart from simplifying the driver this also drops the last non-core user
of pwm_request_from_chip() and so makes further simplifications
possible.
Signed-off-by: Uwe Kleine-König <[email protected]>
---
drivers/pwm/pwm-cros-ec.c | 32 ++++++++++++--------------------
1 file changed, 12 insertions(+), 20 deletions(-)
diff --git a/drivers/pwm/pwm-cros-ec.c b/drivers/pwm/pwm-cros-ec.c
index ba4ee0b507b7..fcc33a2cb878 100644
--- a/drivers/pwm/pwm-cros-ec.c
+++ b/drivers/pwm/pwm-cros-ec.c
@@ -169,24 +169,6 @@ static int cros_ec_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
return 0;
}
-static struct pwm_device *
-cros_ec_pwm_xlate(struct pwm_chip *chip, const struct of_phandle_args *args)
-{
- struct pwm_device *pwm;
-
- if (args->args[0] >= chip->npwm)
- return ERR_PTR(-EINVAL);
-
- pwm = pwm_request_from_chip(chip, args->args[0], NULL);
- if (IS_ERR(pwm))
- return pwm;
-
- /* The EC won't let us change the period */
- pwm->args.period = EC_PWM_MAX_DUTY;
-
- return pwm;
-}
-
static const struct pwm_ops cros_ec_pwm_ops = {
.get_state = cros_ec_pwm_get_state,
.apply = cros_ec_pwm_apply,
@@ -237,7 +219,7 @@ static int cros_ec_pwm_probe(struct platform_device *pdev)
struct cros_ec_pwm_device *ec_pwm;
struct pwm_chip *chip;
bool use_pwm_type = false;
- unsigned int npwm;
+ unsigned int i, npwm;
int ret;
if (!ec)
@@ -263,7 +245,17 @@ static int cros_ec_pwm_probe(struct platform_device *pdev)
/* PWM chip */
chip->ops = &cros_ec_pwm_ops;
- chip->of_xlate = cros_ec_pwm_xlate;
+
+ /*
+ * The device tree binding for this device is special as it only uses a
+ * single cell (for the hwid) and so doesn't provide a default period.
+ * This isn't a big problem though as the hardware only supports a
+ * single period length, it's just a bit ugly to make this fit into the
+ * pwm core abstractions. So initialize the period here, as
+ * of_pwm_xlate_with_flags() won't do that for us.
+ */
+ for (i = 0; i < npwm; ++i)
+ chip->pwms[i].args.period = EC_PWM_MAX_DUTY;
dev_dbg(dev, "Probed %u PWMs\n", chip->npwm);
--
2.43.0
On Fri, Jun 07, 2024 at 10:44:15AM +0200, Uwe Kleine-K?nig wrote:
> The get_state() callback is never called (in a visible way) after there
> is a consumer for a pwm device. The core handles loosing the information
> about duty_cycle just fine.
>
> Simplify the driver accordingly.
>
> Signed-off-by: Uwe Kleine-K?nig <[email protected]>
> ---
> drivers/pwm/pwm-cros-ec.c | 33 +--------------------------------
> 1 file changed, 1 insertion(+), 32 deletions(-)
>
> diff --git a/drivers/pwm/pwm-cros-ec.c b/drivers/pwm/pwm-cros-ec.c
> index 606ccfdaf4cc..ba4ee0b507b7 100644
> --- a/drivers/pwm/pwm-cros-ec.c
> +++ b/drivers/pwm/pwm-cros-ec.c
> @@ -25,15 +25,6 @@
> struct cros_ec_pwm_device {
> struct cros_ec_device *ec;
> bool use_pwm_type;
> - struct cros_ec_pwm *channel;
> -};
I forgot to drop the kernel doc comment. Unless I get some feedback that
makes me send a v2, I'll squash the following into this patch when
applying:
diff --git a/drivers/pwm/pwm-cros-ec.c b/drivers/pwm/pwm-cros-ec.c
index fcc33a2cb878..189301dc395e 100644
--- a/drivers/pwm/pwm-cros-ec.c
+++ b/drivers/pwm/pwm-cros-ec.c
@@ -20,7 +20,6 @@
*
* @ec: Pointer to EC device
* @use_pwm_type: Use PWM types instead of generic channels
- * @channel: array with per-channel data
*/
struct cros_ec_pwm_device {
struct cros_ec_device *ec;
Best regards
Uwe
Hi Uwe,
kernel test robot noticed the following build warnings:
[auto build test WARNING on 1613e604df0cd359cf2a7fbd9be7a0bcfacfabd0]
url: https://github.com/intel-lab-lkp/linux/commits/Uwe-Kleine-K-nig/pwm-cros-ec-Don-t-care-about-consumers-in-get_state/20240607-164747
base: 1613e604df0cd359cf2a7fbd9be7a0bcfacfabd0
patch link: https://lore.kernel.org/r/20240607084416.897777-6-u.kleine-koenig%40baylibre.com
patch subject: [PATCH 1/3] pwm: cros-ec: Don't care about consumers in .get_state()
config: arm64-defconfig (https://download.01.org/0day-ci/archive/20240608/[email protected]/config)
compiler: aarch64-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240608/[email protected]/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 <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
All warnings (new ones prefixed by >>):
>> drivers/pwm/pwm-cros-ec.c:28: warning: Excess struct member 'channel' description in 'cros_ec_pwm_device'
vim +28 drivers/pwm/pwm-cros-ec.c
3d593b6e80ad2c Fabio Baltieri 2022-04-28 17
1f0d3bb02785f6 Brian Norris 2016-07-15 18 /**
1f0d3bb02785f6 Brian Norris 2016-07-15 19 * struct cros_ec_pwm_device - Driver data for EC PWM
1f0d3bb02785f6 Brian Norris 2016-07-15 20 *
1f0d3bb02785f6 Brian Norris 2016-07-15 21 * @ec: Pointer to EC device
3d593b6e80ad2c Fabio Baltieri 2022-04-28 22 * @use_pwm_type: Use PWM types instead of generic channels
82adc1b2688b02 Uwe Kleine-K?nig 2023-07-05 23 * @channel: array with per-channel data
1f0d3bb02785f6 Brian Norris 2016-07-15 24 */
1f0d3bb02785f6 Brian Norris 2016-07-15 25 struct cros_ec_pwm_device {
1f0d3bb02785f6 Brian Norris 2016-07-15 26 struct cros_ec_device *ec;
3d593b6e80ad2c Fabio Baltieri 2022-04-28 27 bool use_pwm_type;
1db37f9561b2b3 Thierry Reding 2019-10-17 @28 };
1db37f9561b2b3 Thierry Reding 2019-10-17 29
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
On Fri, Jun 07, 2024 at 10:44:15AM +0200, Uwe Kleine-K?nig wrote:
> The get_state() callback is never called (in a visible way) after there
> is a consumer for a pwm device. The core handles loosing the information
> about duty_cycle just fine.
ChromeOS EC has no separated "enabled" state, it sees `duty == 0` as
"disabled"[1]. 1db37f9561b2 ("pwm: cros-ec: Cache duty cycle value")
caches the value in kernel side so that it can retrieve the original duty
value even if (struct pwm_state *)->enabled is false.
To make sure I understand, did you mean the original duty value could be less
important because:
- We are less caring as it is in a debug context at [2]?
- At [3], the PWM device is still initializing.
[1]: https://crrev.com/0e16954460a08133b2557150e0897014ea2b9672/common/pwm.c#66
[2]: https://elixir.bootlin.com/linux/v6.10-rc3/source/drivers/pwm/core.c#L52
[3]: https://elixir.bootlin.com/linux/v6.10-rc3/source/drivers/pwm/core.c#L371
Hello Tzung,
On Tue, Jun 11, 2024 at 08:50:44AM +0000, Tzung-Bi Shih wrote:
> On Fri, Jun 07, 2024 at 10:44:15AM +0200, Uwe Kleine-K?nig wrote:
> > The get_state() callback is never called (in a visible way) after there
> > is a consumer for a pwm device. The core handles loosing the information
> > about duty_cycle just fine.
>
> ChromeOS EC has no separated "enabled" state, it sees `duty == 0` as
> "disabled"[1]. 1db37f9561b2 ("pwm: cros-ec: Cache duty cycle value")
> caches the value in kernel side so that it can retrieve the original duty
> value even if (struct pwm_state *)->enabled is false.
There is no need to cache, so the following would work:
diff --git a/drivers/pwm/pwm-cros-ec.c b/drivers/pwm/pwm-cros-ec.c
index 606ccfdaf4cc..2b72468767f4 100644
--- a/drivers/pwm/pwm-cros-ec.c
+++ b/drivers/pwm/pwm-cros-ec.c
@@ -25,15 +25,6 @@
struct cros_ec_pwm_device {
struct cros_ec_device *ec;
bool use_pwm_type;
- struct cros_ec_pwm *channel;
-};
-
-/**
- * struct cros_ec_pwm - per-PWM driver data
- * @duty_cycle: cached duty cycle
- */
-struct cros_ec_pwm {
- u16 duty_cycle;
};
static inline struct cros_ec_pwm_device *pwm_to_cros_ec_pwm(struct pwm_chip *chip)
@@ -135,37 +126,33 @@ static int cros_ec_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
const struct pwm_state *state)
{
struct cros_ec_pwm_device *ec_pwm = pwm_to_cros_ec_pwm(chip);
- struct cros_ec_pwm *channel = &ec_pwm->channel[pwm->hwpwm];
u16 duty_cycle;
- int ret;
- /* The EC won't let us change the period */
- if (state->period != EC_PWM_MAX_DUTY)
- return -EINVAL;
+ if (state->enabled) {
- if (state->polarity != PWM_POLARITY_NORMAL)
- return -EINVAL;
+ /* The EC only supports period = EC_PWM_MAX_DUTY */
+ if (state->period < EC_PWM_MAX_DUTY ||
+ state->polarity != PWM_POLARITY_NORMAL)
+ return -EINVAL;
- /*
- * EC doesn't separate the concept of duty cycle and enabled, but
- * kernel does. Translate.
- */
- duty_cycle = state->enabled ? state->duty_cycle : 0;
+ duty_cycle = min(state->duty_cycle, (u64)EC_PWM_MAX_DUTY);
- ret = cros_ec_pwm_set_duty(ec_pwm, pwm->hwpwm, duty_cycle);
- if (ret < 0)
- return ret;
+ } else {
+ /*
+ * The hardware has no possibility to disable and so save power.
+ * Many consumers expect the PWM to at least stop to oscilate, so just
+ * configure for duty_cycle = 0.
+ */
+ duty_cycle = 0;
+ }
- channel->duty_cycle = state->duty_cycle;
-
- return 0;
+ return cros_ec_pwm_set_duty(ec_pwm, pwm->hwpwm, duty_cycle);
}
static int cros_ec_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
struct pwm_state *state)
{
struct cros_ec_pwm_device *ec_pwm = pwm_to_cros_ec_pwm(chip);
- struct cros_ec_pwm *channel = &ec_pwm->channel[pwm->hwpwm];
int ret;
ret = cros_ec_pwm_get_duty(ec_pwm->ec, ec_pwm->use_pwm_type, pwm->hwpwm);
@@ -175,23 +162,10 @@ static int cros_ec_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
}
state->enabled = (ret > 0);
+ state->duty_cycle = ret;
state->period = EC_PWM_MAX_DUTY;
state->polarity = PWM_POLARITY_NORMAL;
- /*
- * Note that "disabled" and "duty cycle == 0" are treated the same. If
- * the cached duty cycle is not zero, used the cached duty cycle. This
- * ensures that the configured duty cycle is kept across a disable and
- * enable operation and avoids potentially confusing consumers.
- *
- * For the case of the initial hardware readout, channel->duty_cycle
- * will be 0 and the actual duty cycle read from the EC is used.
- */
- if (ret == 0 && channel->duty_cycle > 0)
- state->duty_cycle = channel->duty_cycle;
- else
- state->duty_cycle = ret;
-
return 0;
}
@@ -291,11 +265,6 @@ static int cros_ec_pwm_probe(struct platform_device *pdev)
chip->ops = &cros_ec_pwm_ops;
chip->of_xlate = cros_ec_pwm_xlate;
- ec_pwm->channel = devm_kcalloc(dev, chip->npwm, sizeof(*ec_pwm->channel),
- GFP_KERNEL);
- if (!ec_pwm->channel)
- return -ENOMEM;
-
dev_dbg(dev, "Probed %u PWMs\n", chip->npwm);
ret = devm_pwmchip_add(dev, chip);
> To make sure I understand, did you mean the original duty value could be less
> important because:
> - We are less caring as it is in a debug context at [2]?
> - At [3], the PWM device is still initializing.
It doesn't really matter that this is about debug or initialisation. The
key here is that the core can handle the PWM using duty_cycle 0 (or
anything else) when it was requested to be disabled.
Best regards
Uwe
> [1]: https://crrev.com/0e16954460a08133b2557150e0897014ea2b9672/common/pwm.c#66
> [2]: https://elixir.bootlin.com/linux/v6.10-rc3/source/drivers/pwm/core.c#L52
> [3]: https://elixir.bootlin.com/linux/v6.10-rc3/source/drivers/pwm/core.c#L371
On Tue, Jun 11, 2024 at 12:39:44PM +0200, Uwe Kleine-K?nig wrote:
> On Tue, Jun 11, 2024 at 08:50:44AM +0000, Tzung-Bi Shih wrote:
> > On Fri, Jun 07, 2024 at 10:44:15AM +0200, Uwe Kleine-K?nig wrote:
> > > The get_state() callback is never called (in a visible way) after there
> > > is a consumer for a pwm device. The core handles loosing the information
> > > about duty_cycle just fine.
> >
> > ChromeOS EC has no separated "enabled" state, it sees `duty == 0` as
> > "disabled"[1]. 1db37f9561b2 ("pwm: cros-ec: Cache duty cycle value")
> > caches the value in kernel side so that it can retrieve the original duty
> > value even if (struct pwm_state *)->enabled is false.
>
> There is no need to cache, so the following would work:
Ack.
> > To make sure I understand, did you mean the original duty value could be less
> > important because:
> > - We are less caring as it is in a debug context at [2]?
> > - At [3], the PWM device is still initializing.
>
> It doesn't really matter that this is about debug or initialisation. The
> key here is that the core can handle the PWM using duty_cycle 0 (or
> anything else) when it was requested to be disabled.
>
>
> > [1]: https://crrev.com/0e16954460a08133b2557150e0897014ea2b9672/common/pwm.c#66
> > [2]: https://elixir.bootlin.com/linux/v6.10-rc3/source/drivers/pwm/core.c#L52
> > [3]: https://elixir.bootlin.com/linux/v6.10-rc3/source/drivers/pwm/core.c#L371
I was trying to understand the description in the commit message:
: The get_state() callback is never called (in a visible way) after there
: is a consumer for a pwm device.
I guess I understood; the core reads the duty value via get_state() when:
- Initializing the device for the intial value.
- Debugging for checking if apply() really takes effect.
What 1db37f9561b2 worried about is already addressed by the core[4].
[4]: https://elixir.bootlin.com/linux/v6.10-rc3/source/drivers/pwm/core.c#L495
Reviewed-by: Tzung-Bi Shih <[email protected]>
On Fri, Jun 07, 2024 at 10:44:16AM +0200, Uwe Kleine-K?nig wrote:
> The cros-ec device tree binding only uses #pwm-cells = <1>, and so there
> is no period provided in the device tree. Up to now this was handled by
> hardcoding the period to the only supported value in the custom xlate
> callback. Apart from that, the default xlate callback (i.e.
> of_pwm_xlate_with_flags()) handles this just fine (and better, e.g. by
> checking args->args_count >= 1 before accessing args->args[0]).
>
> To simplify make use of of_pwm_xlate_with_flags(), drop the custom
> callback and provide the default period in .probe() already.
>
> Apart from simplifying the driver this also drops the last non-core user
> of pwm_request_from_chip() and so makes further simplifications
> possible.
>
> Signed-off-by: Uwe Kleine-K?nig <[email protected]>
Reviewed-by: Tzung-Bi Shih <[email protected]>
On Fri, Jun 07, 2024 at 10:44:17AM +0200, Uwe Kleine-K?nig wrote:
> The last user of this function outside of core.c is gone, so it can be
> made static.
>
> Signed-off-by: Uwe Kleine-K?nig <[email protected]>
Reviewed-by: Tzung-Bi Shih <[email protected]>
Hello,
On Wed, Jun 12, 2024 at 06:27:14AM +0000, Tzung-Bi Shih wrote:
> On Tue, Jun 11, 2024 at 12:39:44PM +0200, Uwe Kleine-K?nig wrote:
> > On Tue, Jun 11, 2024 at 08:50:44AM +0000, Tzung-Bi Shih wrote:
> > > On Fri, Jun 07, 2024 at 10:44:15AM +0200, Uwe Kleine-K?nig wrote:
> > > > The get_state() callback is never called (in a visible way) after there
> > > > is a consumer for a pwm device. The core handles loosing the information
> > > > about duty_cycle just fine.
> > >
> > > ChromeOS EC has no separated "enabled" state, it sees `duty == 0` as
> > > "disabled"[1]. 1db37f9561b2 ("pwm: cros-ec: Cache duty cycle value")
> > > caches the value in kernel side so that it can retrieve the original duty
> > > value even if (struct pwm_state *)->enabled is false.
> >
> > There is no need to cache, so the following would work:
>
> Ack.
>
> > > To make sure I understand, did you mean the original duty value could be less
> > > important because:
> > > - We are less caring as it is in a debug context at [2]?
> > > - At [3], the PWM device is still initializing.
> >
> > It doesn't really matter that this is about debug or initialisation. The
> > key here is that the core can handle the PWM using duty_cycle 0 (or
> > anything else) when it was requested to be disabled.
> >
> >
> > > [1]: https://crrev.com/0e16954460a08133b2557150e0897014ea2b9672/common/pwm.c#66
> > > [2]: https://elixir.bootlin.com/linux/v6.10-rc3/source/drivers/pwm/core.c#L52
> > > [3]: https://elixir.bootlin.com/linux/v6.10-rc3/source/drivers/pwm/core.c#L371
>
> I was trying to understand the description in the commit message:
> : The get_state() callback is never called (in a visible way) after there
> : is a consumer for a pwm device.
>
> I guess I understood; the core reads the duty value via get_state() when:
> - Initializing the device for the intial value.
Yes, that a consumer cannot do any assumptions about a PWM just aquired,
so there is no danger for confusion.
> - Debugging for checking if apply() really takes effect.
Right, and the read pwm_state is only used in the core. The core won't
be confused about disabled vs. duty=0. Consumers never see the result of
the .get_state callback.
> What 1db37f9561b2 worried about is already addressed by the core[4].
> [4]: https://elixir.bootlin.com/linux/v6.10-rc3/source/drivers/pwm/core.c#L495
What 1db37f9561b2 worried about is bogus, because pwm_get_state()
doesn't call the .get_state() callback:
$ git show 1db37f9561b2:include/linux/pwm.h | awk '/void pwm_get_state/,/^$/'
static inline void pwm_get_state(const struct pwm_device *pwm,
struct pwm_state *state)
{
*state = pwm->state;
}
> Reviewed-by: Tzung-Bi Shih <[email protected]>
I'll apply the patch under discussion and create a proper patch for the
ad-hoc change from my previous mail.
Thanks
Uwe