Re: [PATCH v8 11/11] pwm: Add Raspberry Pi Firmware based PWM bus - Uwe Kleine-König

From: "Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>
To: Nicolas Saenz Julienne <nsaenzjulienne@suse.de>
Cc: f.fainelli@gmail.com, linux-kernel@vger.kernel.org,
	linux-pwm@vger.kernel.org, bcm-kernel-feedback-list@broadcom.com,
	linux-arm-kernel@lists.infradead.org, devicetree@vger.kernel.org,
	wahrenst@gmx.net, linux-input@vger.kernel.org,
	dmitry.torokhov@gmail.com, gregkh@linuxfoundation.org,
	devel@driverdev.osuosl.org, p.zabel@pengutronix.de,
	linux-gpio@vger.kernel.org, linus.walleij@linaro.org,
	linux-clk@vger.kernel.org, sboyd@kernel.org,
	linux-rpi-kernel@lists.infradead.org, bgolaszewski@baylibre.com,
	andy.shevchenko@gmail.com
Subject: Re: [PATCH v8 11/11] pwm: Add Raspberry Pi Firmware based PWM bus
Date: Fri, 12 Mar 2021 21:12:17 +0100	[thread overview]
Message-ID: <20210312201217.n2sav23swy7ii4uo@pengutronix.de> (raw)
In-Reply-To: <20210312122454.24480-12-nsaenzjulienne@suse.de>

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

Hello Nicolas,

On Fri, Mar 12, 2021 at 01:24:54PM +0100, Nicolas Saenz Julienne wrote:
> Adds support to control the PWM bus available in official Raspberry Pi
> PoE HAT. Only RPi's co-processor has access to it, so commands have to
> be sent through RPi's firmware mailbox interface.
> 
> Signed-off-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de>
> 
> ---
> 
> Changes since v7:
>  - Remove unwarranted RPI_PWM_DEF_DUTY_REG usage
> 
>  Changes since v6:
> - Use %pe
> - Round divisions properly
> - Use dev_err_probe()
> - Pass check_patch
> 
> Changes since v3:
>  - Rename compatible string to be more explicit WRT to bus's limitations
> 
> Changes since v2:
>  - Use devm_rpi_firmware_get()
>  - Rename driver
>  - Small cleanups
> 
> Changes since v1:
>  - Use default pwm bindings and get rid of xlate() function
>  - Correct spelling errors
>  - Correct apply() function
>  - Round values
>  - Fix divisions in arm32 mode
>  - Small cleanups
> 
>  drivers/pwm/Kconfig               |   9 ++
>  drivers/pwm/Makefile              |   1 +
>  drivers/pwm/pwm-raspberrypi-poe.c | 206 ++++++++++++++++++++++++++++++
>  3 files changed, 216 insertions(+)
>  create mode 100644 drivers/pwm/pwm-raspberrypi-poe.c
> 
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index a7a7a9f26aef..d3371ac7b871 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -431,6 +431,15 @@ config PWM_PXA
>  	  To compile this driver as a module, choose M here: the module
>  	  will be called pwm-pxa.
>  
> +config PWM_RASPBERRYPI_POE
> +	tristate "Raspberry Pi Firwmware PoE Hat PWM support"
> +	# Make sure not 'y' when RASPBERRYPI_FIRMWARE is 'm'. This can only
> +	# happen when COMPILE_TEST=y, hence the added !RASPBERRYPI_FIRMWARE.
> +	depends on RASPBERRYPI_FIRMWARE || (COMPILE_TEST && !RASPBERRYPI_FIRMWARE)
> +	help
> +	  Enable Raspberry Pi firmware controller PWM bus used to control the
> +	  official RPI PoE hat
> +
>  config PWM_RCAR
>  	tristate "Renesas R-Car PWM support"
>  	depends on ARCH_RENESAS || COMPILE_TEST
> diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> index 4e35a55fa7b6..d3879619bd76 100644
> --- a/drivers/pwm/Makefile
> +++ b/drivers/pwm/Makefile
> @@ -39,6 +39,7 @@ obj-$(CONFIG_PWM_NTXEC)		+= pwm-ntxec.o
>  obj-$(CONFIG_PWM_OMAP_DMTIMER)	+= pwm-omap-dmtimer.o
>  obj-$(CONFIG_PWM_PCA9685)	+= pwm-pca9685.o
>  obj-$(CONFIG_PWM_PXA)		+= pwm-pxa.o
> +obj-$(CONFIG_PWM_RASPBERRYPI_POE)	+= pwm-raspberrypi-poe.o
>  obj-$(CONFIG_PWM_RCAR)		+= pwm-rcar.o
>  obj-$(CONFIG_PWM_RENESAS_TPU)	+= pwm-renesas-tpu.o
>  obj-$(CONFIG_PWM_ROCKCHIP)	+= pwm-rockchip.o
> diff --git a/drivers/pwm/pwm-raspberrypi-poe.c b/drivers/pwm/pwm-raspberrypi-poe.c
> new file mode 100644
> index 000000000000..71ade5e55069
> --- /dev/null
> +++ b/drivers/pwm/pwm-raspberrypi-poe.c
> @@ -0,0 +1,206 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright 2020 Nicolas Saenz Julienne <nsaenzjulienne@suse.de>

2021?

> + * For more information on Raspberry Pi's PoE hat see:
> + * https://www.raspberrypi.org/products/poe-hat/

Out of personal interest: Is this hat also able to power a RPi CM4?

> + * Limitations:
> + *  - No disable bit, so a disabled PWM is simulated by duty_cycle 0
> + *  - Only normal polarity
> + *  - Fixed 12.5 kHz period
> + *
> + * The current period is completed when HW is reconfigured.
> + */

Other than that as mentioned in the previous round: This looks good,

Reviewed-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

What is your thought about how to get this series merged? At least
input, staging, armsoc, clk, reset anf firmware are touched. Do you
prepare a branch for merging in the relevant trees (once you have all
the necessary Acks)?

Best regards
Uwe

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

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