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

William Qiu
2023-10-27T10:24:08Z

On 2023/10/24 19:45, Uwe Kleine-König wrote:
> 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@starfivetech.com>
>> >> Signed-off-by: Hal Feng <hal.feng@starfivetech.com>
>> >> Signed-off-by: William Qiu <william.qiu@starfivetech.com>
>> >> ---
>> >> 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@starfivetech.com>
>> >> +M: Hal Feng <hal.feng@starfivetech.com>
>> >> +S: Supported
>> >> +F: Documentation/devicetree/bindings/pwm/opencores,pwm-ocores.yaml
>> >> +F: drivers/pwm/pwm-ocores.c
>> >> +
>> >> OPENRISC ARCHITECTURE
>> >> M: Jonas Bonn <jonas@southpole.se>
>> >> M: Stefan Kristiansson <stefan.kristiansson@saunalahti.fi>
>> >> 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)".
>
I see, I'll think about it.
Maybe depend on STARFIVE'S SoCs first?

Best regards,
William