Re: [PATCH V3 2/5] pwm: Add i.MX TPM PWM driver support

From: Uwe Kleine-König
Date: Thu Mar 14 2019 - 05:22:23 EST


On Thu, Mar 14, 2019 at 06:27:20AM +0000, Anson Huang wrote:
> i.MX7ULP has TPM(Low Power Timer/Pulse Width Modulation Module)
> inside, add TPM PWM driver support.
>
> Signed-off-by: Anson Huang <Anson.Huang@xxxxxxx>
> ---
> Changes since V2:
> - Add "IMX_" as prefix to macro define as TPM is already used;
> - Use macro define for channel registers address instead of calculation in everywhere.
> - Add limitations statement in copyright.
> ---
> drivers/pwm/Kconfig | 10 ++
> drivers/pwm/Makefile | 1 +
> drivers/pwm/pwm-imx-tpm.c | 343 ++++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 354 insertions(+)
> create mode 100644 drivers/pwm/pwm-imx-tpm.c
>
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index a8f47df..c1cbb43 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -201,6 +201,16 @@ config PWM_IMX
> To compile this driver as a module, choose M here: the module
> will be called pwm-imx.
>
> +config PWM_IMX_TPM
> + tristate "i.MX TPM PWM support"
> + depends on ARCH_MXC
> + help
> + Generic PWM framework driver for i.MX7ULP TPM module, TPM's full
> + name is Low Power Timer/Pulse Width Modulation Module.
> +
> + To compile this driver as a module, choose M here: the module
> + will be called pwm-imx-tpm.
> +
> config PWM_JZ4740
> tristate "Ingenic JZ47xx PWM support"
> depends on MACH_INGENIC
> diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> index 9c676a0..64e036c 100644
> --- a/drivers/pwm/Makefile
> +++ b/drivers/pwm/Makefile
> @@ -18,6 +18,7 @@ obj-$(CONFIG_PWM_FSL_FTM) += pwm-fsl-ftm.o
> obj-$(CONFIG_PWM_HIBVT) += pwm-hibvt.o
> obj-$(CONFIG_PWM_IMG) += pwm-img.o
> obj-$(CONFIG_PWM_IMX) += pwm-imx.o
> +obj-$(CONFIG_PWM_IMX_TPM) += pwm-imx-tpm.o
> obj-$(CONFIG_PWM_JZ4740) += pwm-jz4740.o
> obj-$(CONFIG_PWM_LP3943) += pwm-lp3943.o
> obj-$(CONFIG_PWM_LPC18XX_SCT) += pwm-lpc18xx-sct.o
> diff --git a/drivers/pwm/pwm-imx-tpm.c b/drivers/pwm/pwm-imx-tpm.c
> new file mode 100644
> index 0000000..cdb29f6
> --- /dev/null
> +++ b/drivers/pwm/pwm-imx-tpm.c
> @@ -0,0 +1,343 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright 2018-2019 NXP.
> + *
> + * Limitations:
> + * - The TPM counter and period counter are shared between
> + * multiple channels, so all channels should use same period
> + * settings.
> + * - When a channel is disabled, its polarity settings will be
> + * saved and its output will be disabled by clearing polarity
> + * setting, when channel is enabled, polarity settings will be
> + * restored and output will be enabled again.
> + * - ONLY when all channels are disabled, then TPM counter will
> + * be disabled.

I wouldn't list the last two items in this section. These are
implementation details that a PWM consumer doesn't need to bother with.
These are fine to be noted in a code comment at the appropriate place.

Other than that please look at my feedback for the previous series that
I just sent out. I think most things still apply to this version.

Best regards
Uwe

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