Re: [PATCH v2 2/2] pwm: Add support for Xilinx AXI Timer

[Sean Anderson <sean.anderson@seco.com>]

On 5/5/21 2:37 AM, Michal Simek wrote:
 >
 >
 > On 5/4/21 8:49 PM, Sean Anderson wrote:
 >> This adds PWM support for Xilinx LogiCORE IP AXI soft timers commonly
 >> found on Xilinx FPGAs. There is another driver for this device located
 >> at arch/microblaze/kernel/timer.c, but it is only used for timekeeping.
 >> This driver was written with reference to Xilinx DS764 for v1.03.a [1].
 >>
 >> [1] https://www.xilinx.com/support/documentation/ip_documentation/axi_timer/v1_03_a/axi_timer_ds764.pdf
 >>
 >> Signed-off-by: Sean Anderson <sean.anderson@seco.com>
 >> ---
 >> I tried adding a XILINX_PWM_ prefix to all the defines, but IMO it
 >> really hurt readability. That prefix almost doubles the size the
 >> defines, and is particularly excessive in something like
 >> XILINX_PWM_TCSR_RUN_MASK.
 >>
 >> Changes in v2:
 >> - Don't compile this module by default for arm64
 >> - Add dependencies on COMMON_CLK and HAS_IOMEM
 >> - Add comment explaining why we depend on !MICROBLAZE
 >> - Add comment describing device
 >> - Rename TCSR_(SET|CLEAR) to TCSR_RUN_(SET|CLEAR)
 >> - Use NSEC_TO_SEC instead of defining our own
 >> - Use TCSR_RUN_MASK to check if the PWM is enabled, as suggested by Uwe
 >> - Cast dividends to u64 to avoid overflow
 >> - Check for over- and underflow when calculating TLR
 >> - Set xilinx_pwm_ops.owner
 >> - Don't set pwmchip.base to -1
 >> - Check range of xlnx,count-width
 >> - Ensure the clock is always running when the pwm is registered
 >> - Remove debugfs file :l
 >> - Report errors with dev_error_probe
 >>
 >>   drivers/pwm/Kconfig      |  13 ++
 >>   drivers/pwm/Makefile     |   1 +
 >>   drivers/pwm/pwm-xilinx.c | 301 +++++++++++++++++++++++++++++++++++++++
 >>   3 files changed, 315 insertions(+)
 >>   create mode 100644 drivers/pwm/pwm-xilinx.c
 >
 > Without looking below another driver which target the same IP is just
 > wrong that's why NACK from me.

Can you elaborate on this position a bit more? I don't think a rework of
the microblaze driver should hold back this one. They cannot be enabled
at the same time. I think it is OK to leave the work of making them
coexist for a future series (written by someone with microblaze hardware
to test on).

--Sean