Re: [PATCH] pwm: cros-ec: Let cros_ec_pwm_get_state() return the last applied state

From: Enric Balletbo i Serra
Date: Mon Oct 21 2019 - 05:42:13 EST

Next message: Naoya Horiguchi: "[PATCH] mm,hwpoison: introduce MF_MSG_UNSPLIT_THP"
Previous message: David Hildenbrand: "Re: [RFC PATCH v2 15/16] mm/hwpoison-inject: Rip off duplicated checks"
In reply to: Thierry Reding: "Re: [PATCH] pwm: cros-ec: Let cros_ec_pwm_get_state() return the last applied state"
Next in thread: Uwe Kleine-König: "Re: [PATCH] pwm: cros-ec: Let cros_ec_pwm_get_state() return the last applied state"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Hi Thierry,
On 17/10/19 13:35, Thierry Reding wrote:
> On Wed, Oct 09, 2019 at 03:47:43PM +0200, Enric Balletbo i Serra wrote:

[snip]

>
> --- >8 ---
> From 15245e5f0dc02af021451b098df93901c9f49373 Mon Sep 17 00:00:00 2001
> From: Thierry Reding <thierry.reding@xxxxxxxxx>
> Date: Thu, 17 Oct 2019 13:21:15 +0200
> Subject: [PATCH] pwm: cros-ec: Cache duty cycle value
>
> The ChromeOS embedded controller doesn't differentiate between disabled
> and duty cycle being 0. In order not to potentially confuse consumers,
> cache the duty cycle and return the cached value instead of the real
> value when the PWM is disabled.
>
> Signed-off-by: Thierry Reding <thierry.reding@xxxxxxxxx>
> ---
> drivers/pwm/pwm-cros-ec.c | 58 ++++++++++++++++++++++++++++++++++++---
> 1 file changed, 54 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/pwm/pwm-cros-ec.c b/drivers/pwm/pwm-cros-ec.c
> index 89497448d217..09c08dee099e 100644
> --- a/drivers/pwm/pwm-cros-ec.c
> +++ b/drivers/pwm/pwm-cros-ec.c
> @@ -25,11 +25,39 @@ struct cros_ec_pwm_device {
> struct pwm_chip chip;
> };
>
> +/**
> + * 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 *c)
> {
> return container_of(c, struct cros_ec_pwm_device, chip);
> }
>
> +static int cros_ec_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
> +{
> + struct cros_ec_pwm *channel;
> +
> + channel = kzalloc(sizeof(*channel), GFP_KERNEL);
> + if (!channel)
> + return -ENOMEM;
> +
> + pwm_set_chip_data(pwm, channel);
> +
> + return 0;
> +}
> +
> +static void cros_ec_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
> +{
> + struct cros_ec_pwm *channel = pwm_get_chip_data(pwm);
> +
> + kfree(channel);
> +}
> +
> static int cros_ec_pwm_set_duty(struct cros_ec_device *ec, u8 index, u16 duty)
> {
> struct {
> @@ -96,7 +124,9 @@ 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);
> - int duty_cycle;
> + struct cros_ec_pwm *channel = pwm_get_chip_data(pwm);
> + u16 duty_cycle;
> + int ret;
>
> /* The EC won't let us change the period */
> if (state->period != EC_PWM_MAX_DUTY)
> @@ -108,13 +138,20 @@ static int cros_ec_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> */
> duty_cycle = state->enabled ? state->duty_cycle : 0;
>
> - return cros_ec_pwm_set_duty(ec_pwm->ec, pwm->hwpwm, duty_cycle);
> + ret = cros_ec_pwm_set_duty(ec_pwm->ec, pwm->hwpwm, duty_cycle);
> + if (ret < 0)
> + return ret;
> +
> + channel->duty_cycle = state->duty_cycle;
> +
> + return 0;
> }
>
> static void 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 = pwm_get_chip_data(pwm);
> int ret;
>
> ret = cros_ec_pwm_get_duty(ec_pwm->ec, pwm->hwpwm);
> @@ -126,8 +163,19 @@ static void cros_ec_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
> state->enabled = (ret > 0);
> state->period = EC_PWM_MAX_DUTY;
>
> - /* Note that "disabled" and "duty cycle == 0" are treated the same */
> - state->duty_cycle = ret;
> + /*
> + * 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;
> }
>
> static struct pwm_device *
> @@ -149,6 +197,8 @@ cros_ec_pwm_xlate(struct pwm_chip *pc, const struct of_phandle_args *args)
> }
>
> static const struct pwm_ops cros_ec_pwm_ops = {
> + .request = cros_ec_pwm_request,
> + .free = cros_ec_pwm_free,
> .get_state = cros_ec_pwm_get_state,
> .apply = cros_ec_pwm_apply,
> .owner = THIS_MODULE,
>

I just tried your approach but I got a NULL pointer dereference while probe. I
am just back from a week off but I'll be able to dig into it between today and
tomorrow, just wanted to let you know that the patch doesn't works as is for me.

[ 10.128455] Unable to handle kernel NULL pointer dereference at virtual
address 0000000000000000
[ 10.141895] Mem abort info:

[ 10.145090] ESR = 0x96000004

[ 10.148537] EC = 0x25: DABT (current EL), IL = 32 bits

[ 10.154560] SET = 0, FnV = 0

[ 10.157986] EA = 0, S1PTW = 0

[ 10.161548] Data abort info:

[ 10.164804] ISV = 0, ISS = 0x00000004

[ 10.169111] CM = 0, WnR = 0

[ 10.172436] user pgtable: 4k pages, 48-bit VAs, pgdp=00000000ed44b000

[ 10.179660] [0000000000000000] pgd=0000000000000000

[ 10.179669] Internal error: Oops: 96000004 [#1] PREEMPT SMP

[ 10.179673] Modules linked in: atmel_mxt_ts(+) rockchip_saradc pwm_cros_ec(+)
rockchip_thermal pcie_rockchip_host snd_soc_rl6231 ip_tables x_
tables ipv6 nf_defrag_ipv6

[ 10.179694] CPU: 1 PID: 255 Comm: systemd-udevd Not tainted 5.4.0-rc4+ #230

[ 10.179696] Hardware name: Google Kevin (DT)

[ 10.179700] pstate: 60000005 (nZCv daif -PAN -UAO)

[ 10.179714] pc : cros_ec_pwm_get_state+0xcc/0xf8 [pwm_cros_ec]

[ 10.179721] lr : cros_ec_pwm_get_state+0x80/0xf8 [pwm_cros_ec]

[ 10.247829] sp : ffff800012433810

[ 10.251531] x29: ffff800012433810 x28: 0000000200000026

[ 10.257469] x27: ffff800012433942 x26: ffff0000ef075010

[ 10.263405] x25: ffff800011ca8508 x24: ffff800011e68e30

[ 10.269341] x23: 0000000000000000 x22: ffff0000ee273190
[ 10.275278] x21: ffff0000ee2e3240 x20: ffff0000ee2e3270
[ 10.281214] x19: ffff800011bc98c8 x18: 0000000000000003
[ 10.287150] x17: 0000000000000007 x16: 000000000000000e
[ 10.293088] x15: 0000000000000001 x14: 0000000000000019
[ 10.299024] x13: 0000000000000033 x12: 0000000000000018
[ 10.304962] x11: 071c71c71c71c71c x10: 00000000000009d0
[ 10.310379] atmel_mxt_ts 5-004a: Family: 164 Variant: 17 Firmware V2.0.AA
Objects: 31
[ 10.310901] x9 : ffff800012433490 x8 : ffff0000edb81830
[ 10.310905] x7 : 000000000000011b x6 : 0000000000000001
[ 10.310908] x5 : 0000000000000000 x4 : 0000000000000000
[ 10.310911] x3 : ffff0000edb80e00 x2 : 0000000000000002
[ 10.310914] x1 : 0000000000000000 x0 : 0000000000000000
[ 10.310918] Call trace:
[ 10.310931] cros_ec_pwm_get_state+0xcc/0xf8 [pwm_cros_ec]
[ 10.310944] pwmchip_add_with_polarity+0x134/0x290
[ 10.363576] pwmchip_add+0x24/0x30
[ 10.367383] cros_ec_pwm_probe+0x13c/0x1cc [pwm_cros_ec]
[ 10.373325] platform_drv_probe+0x58/0xa8
[ 10.377809] really_probe+0xe0/0x318
[ 10.381804] driver_probe_device+0x5c/0xf0
[ 10.386381] device_driver_attach+0x74/0x80

Thanks,
Enric

Next message: Naoya Horiguchi: "[PATCH] mm,hwpoison: introduce MF_MSG_UNSPLIT_THP"
Previous message: David Hildenbrand: "Re: [RFC PATCH v2 15/16] mm/hwpoison-inject: Rip off duplicated checks"
In reply to: Thierry Reding: "Re: [PATCH] pwm: cros-ec: Let cros_ec_pwm_get_state() return the last applied state"
Next in thread: Uwe Kleine-König: "Re: [PATCH] pwm: cros-ec: Let cros_ec_pwm_get_state() return the last applied state"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]