Re: [PATCH v6 2/4] pwm: opencores: Add PWM driver support

[Uwe Kleine-König]

Hello William,

On Tue, Oct 24, 2023 at 05:16:49PM +0800, William Qiu wrote:
> On 2023/10/20 19:25, Uwe Kleine-König wrote:
> > Hello,
> > 
> > On Fri, Oct 20, 2023 at 06:37:39PM +0800, William Qiu wrote:
> >> Add Pulse Width Modulation driver support for OpenCores.
> >> 
> >> Co-developed-by: Hal Feng <hal.feng@xxxxxxxxxxxxxxxx>
> >> Signed-off-by: Hal Feng <hal.feng@xxxxxxxxxxxxxxxx>
> >> Signed-off-by: William Qiu <william.qiu@xxxxxxxxxxxxxxxx>
> >> ---
> >>  MAINTAINERS              |   7 ++
> >>  drivers/pwm/Kconfig      |  11 ++
> >>  drivers/pwm/Makefile     |   1 +
> >>  drivers/pwm/pwm-ocores.c | 211 +++++++++++++++++++++++++++++++++++++++
> >>  4 files changed, 230 insertions(+)
> >>  create mode 100644 drivers/pwm/pwm-ocores.c
> >> 
> >> diff --git a/MAINTAINERS b/MAINTAINERS
> >> index 6c4cce45a09d..321af8fa7aad 100644
> >> --- a/MAINTAINERS
> >> +++ b/MAINTAINERS
> >> @@ -16003,6 +16003,13 @@ F:	Documentation/i2c/busses/i2c-ocores.rst
> >>  F:	drivers/i2c/busses/i2c-ocores.c
> >>  F:	include/linux/platform_data/i2c-ocores.h
> >> 
> >> +OPENCORES PWM DRIVER
> >> +M:	William Qiu <william.qiu@xxxxxxxxxxxxxxxx>
> >> +M:	Hal Feng <hal.feng@xxxxxxxxxxxxxxxx>
> >> +S:	Supported
> >> +F:	Documentation/devicetree/bindings/pwm/opencores,pwm-ocores.yaml
> >> +F:	drivers/pwm/pwm-ocores.c
> >> +
> >>  OPENRISC ARCHITECTURE
> >>  M:	Jonas Bonn <jonas@xxxxxxxxxxxx>
> >>  M:	Stefan Kristiansson <stefan.kristiansson@xxxxxxxxxxxxx>
> >> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> >> index 8ebcddf91f7b..cbfbf227d957 100644
> >> --- a/drivers/pwm/Kconfig
> >> +++ b/drivers/pwm/Kconfig
> >> @@ -434,6 +434,17 @@ config PWM_NTXEC
> >>  	  controller found in certain e-book readers designed by the original
> >>  	  design manufacturer Netronix.
> >> 
> >> +config PWM_OCORES
> >> +	tristate "Opencores PWM support"
> >> +	depends on HAS_IOMEM && OF
> >> +	depends on COMMON_CLK && RESET_CONTROLLER
> > 
> > Would it make sense to add something like:
> > 
> > 	depends on ARCH_SOMETHING || COMPILE_TEST
> > 
> > here?
> > 
> But there is no mention of architectural limitations in the OpenCores's
> specification.

I already guessed that. Still it probably makes no sense to enable that
option on most machines. The PWM device found in i.MX SoCs can
theoretically also be implemented on AT91 or S390x. In practice it
isn't, so there is a dependency on ARCH_MXC || COMPILE_TEST.

Consider the role of someone who does a kernel bump for a certain
machine (on one end of the spectrum) or a distribution kernel (on the
other end).

If you take a 6.5 x86_64 allmodconfig + COMPILE_TEST=n and upgrade to
v6.6-rc7 and do an oldconfig, you get 90 questions[1].

Just looking quickly through this list, among them are:

	DRM support for Loongson Graphics (DRM_LOONGSON) [N/m/?] (NEW) 
	Xilinx AXI DMAS Engine (XILINX_DMA) [N/m/y/?] (NEW)
	Clock driver for Renesas VersaClock 3 devices (COMMON_CLK_VC3) [N/m/y/?] (NEW)
	Realtek RT1017 SDCA Codec - SDW (SND_SOC_RT1017_SDCA_SDW) [N/m/?] (NEW)

I didn't check in detail and maybe one or the other is valid on x86_64,
but I'd be surprised if you find two that are sensible to enable on
x86_64 to support a real machine.

While I think Kconfig cannot be held responsible to only allow
generating "real world sensible" configurations, we should work a bit
harder to rule out the obvious violators and make it easy for people
configuring the kernel where sensible.

In my book it's better to have a too strong dependency at first for a
new driver (but allow it with COMPILE_TEST). Someone who as a device
needing that driver will find it out and speak up. However if you allow
to enable the driver everywhere, many people will disable the driver
(maybe using yes '' | make oldconfig), some will spend time to research
about this option to find which machines actually have such a device and
if the machine(s) they care about are in this set. This is a waste of
time and opportunities. (And note, this isn't only about people spending
time to decide if they enable or disable PWM_OCORES, this is also about
people who use yes '' because there are too many questions and so they
might miss the handful of useful ones.)

Best regards
Uwe

[1] measured using

	yes '' | make oldconfig

and counting the occurrences of "(NEW)".

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

Attachment: signature.asc
Description: PGP signature