On 6/16/21 8:12 AM, Michal Simek wrote: > 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. > The approach I took in v4 is that probe functions and driver callbacks live in drivers/timer and drivers/pwm, and common functions live in drivers/mfd (although I may move them to drivers/timer since Lee Jones doesn't like them there). I would greatly appreciate if you could review v4. It has been on the list for three weeks now with no comments from either you or Uwe. --Sean
