Hi Uwe, On 5/25/21 8:11 AM, Uwe Kleine-König wrote: > Hello Sean, hello Michal, > > On Mon, May 24, 2021 at 09:00:51AM +0200, Michal Simek wrote: >> On 5/20/21 10:13 PM, Sean Anderson wrote: >>> On 5/19/21 3:24 AM, Michal Simek wrote: >>>> On 5/18/21 12:15 AM, Sean Anderson wrote: >>>>> This could be deprecated, but cannot be removed since existing device >>>>> trees (e.g. qemu) have neither clocks nor clock-frequency properties. >>>> >>>> Rob: Do we have any obligation to keep properties for other projects? > > If a binding is in the wild and used to be documented, it has to stay. > >>>>>> 4. Make driver as module >>>>>> 5. Do whatever changes you want before adding pwm support >>>>>> 6. Extend DT binding doc for PWM support >>>>>> 7. Add PWM support >>>>> >>>>> Frankly, I am inclined to just leave the microblaze timer as-is. The PWM >>>>> driver is completely independent. I have already put too much effort into >>>>> this driver, and I don't have the energy to continue working on the >>>>> microblaze timer. >>>> >>>> I understand. I am actually using axi timer as pwm driver in one of my >>>> project but never had time to upstream it because of couple of steps above. >>>> We need to do it right based on steps listed above. If this is too much >>>> work it will have to wait. I will NACK all attempts to add separate >>>> driver for IP which we already support in the tree. >>> >>> 1. Many timers have separate clocksource and PWM drivers. E.g. samsung, >>> renesas TPU, etc. It is completely reasonable to keep separate >>> drivers for these purposes. There is no Linux requirement that each >>> device have only one driver, especially if it has multiple functions >>> or ways to be configured. >> >> It doesn't mean that it was done properly and correctly. Code >> duplication is bad all the time. > > IMHO it's not so much about code duplication. Yes, code duplication is > bad and should be prevented if possible. But it's more important to not > introduce surprises. So I think it should be obvious from reading the > device tree source which timer is used to provide the PWM. I don't care > much if this is from an extra property (like xilinx,provide-pwm), > overriding the compatible or some other explicit mechanism. IIUC in this > suggested patch the selection is implicit and so this isn't so nice. > >>> 2. If you want to do work on a driver, I'm all for it. However, if you >>> have not yet submitted that work to the list, you should not gate >>> other work behind it. Saying that X feature must be gated behind Y >>> *even if X works completely independently of Y* is just stifling >>> development. >> >> I gave you guidance how I think this should be done. I am not gating you >> from this work. Your patch is not working on Microblaze arch which is >> what I maintain. And I don't want to go the route that we will have two >> drivers for the same IP without integration. We were there in past and >> it is just pain. >> I am expecting that PWM guys will guide how this should be done >> properly. I haven't heard any guidance on this yet. >> Thierry/Uwe: Any comment? > > Not sure I can and want to provide guidance here. This is not Perl, but > still TIMTOWTDI. If it was me who cared here, I'd look into the > auxiliary bus (Documentation/driver-api/auxiliary_bus.rst) to check if > it can help to solve this problem. I recently got patches for cadence TTC driver (drivers/clocksource/timer-cadence-ttc.c) for PWM support too. It is the second and very similar case. This driver is used on Zynq as clock source and can be also use as PWM. I can't believe that there are no other examples how to deal with these timers which are used for PWM generation. Thanks, Michal
