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

[Sean Anderson]

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@xxxxxxxx>
>> ---
>> 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