Re: [PATCH v3 2/2] pwm: add T-HEAD PWM driver

[Uwe Kleine-König]

Hello,

On Thu, Oct 05, 2023 at 09:05:19PM +0800, Jisheng Zhang wrote:
> diff --git a/drivers/pwm/pwm-thead.c b/drivers/pwm/pwm-thead.c
> new file mode 100644
> index 000000000000..3b9ffddfe33d
> --- /dev/null
> +++ b/drivers/pwm/pwm-thead.c
> @@ -0,0 +1,270 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * T-HEAD PWM driver
> + *
> + * Copyright (C) 2021 Alibaba Group Holding Limited.
> + * Copyright (C) 2023 Jisheng Zhang <jszhang@xxxxxxxxxx>
> + *
> + * Limitations:
> + * - The THEAD_PWM_CTRL_START bit is only effective when 0 -> 1, which is used
> + *   to start the channel, 1 -> 0 doesn't change anything. so 0 % duty cycle
> + *   is used to "disable" the channel.
> + * - The THEAD_PWM_CTRL_START bit is automatically cleared once PWM channel is
> + *   started.
> + * - The THEAD_PWM_CFG_UPDATE atomically updates and only updates period and duty.
> + * - To update period and duty, THEAD_PWM_CFG_UPDATE needs to go through 0 -> 1
> + *   step, I.E if THEAD_PWM_CFG_UPDATE is already 1, it's necessary to clear it
> + *   to 0 beforehand.
> + * - Polarity can only be changed if never started before.

Questions I'd like to have answered here additionally are:

 - How does the hardware behave when it's disabled? Typical canditates
   are:
    - drives the inactive level
    - freezes where it just happens to be when disable gets effective
    - disable drivers (HIGH-Z)
    - drives low

 - Does the hardware complete the current running period when apply is
   called with enabled=false?

 - Does the hardware complete the current running period when apply is
   called with enabled=true?

> +static const struct dev_pm_ops thead_pwm_pm_ops = {
> +	SET_RUNTIME_PM_OPS(thead_pwm_runtime_suspend, thead_pwm_runtime_resume, NULL)
> +};

Please see
https://lore.kernel.org/linux-pwm/20231023174616.2282067-13-u.kleine-koenig@xxxxxxxxxxxxxx/t/
how this can be improved.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

Attachment: signature.asc
Description: PGP signature