Le 3 avr. 2018 6:59 AM, Daniel Lezcano <daniel.lezcano@xxxxxxxxxx> a écrit : > > On 31/03/2018 19:46, Paul Cercueil wrote: > > Le 2018-03-31 10:10, Daniel Lezcano a écrit : > >> On 29/03/2018 16:52, Paul Cercueil wrote: > >>> > >>> > >>> Le mer. 28 mars 2018 à 18:25, Daniel Lezcano <daniel.lezcano@xxxxxxxxxx> > >>> a écrit : > >>>> On 28/03/2018 17:15, Paul Cercueil wrote: > >>>>> Le 2018-03-24 07:26, Daniel Lezcano a écrit : > >>>>>> On 18/03/2018 00:29, Paul Cercueil wrote: > >>>>>>> This driver will use the TCU (Timer Counter Unit) present on the > >>>>>>> Ingenic > >>>>>>> JZ47xx SoCs to provide the kernel with a clocksource and timers. > >>>>>> > >>>>>> Please provide a more detailed description about the timer. > >>>>> > >>>>> There's a doc file for that :) > >>>> > >>>> Usually, when there is a new driver I ask for a description in the > >>>> changelog for reference. > >>>> > >>>>>> Where is the clocksource ? > >>>>> > >>>>> Right, there is no clocksource, just timers. > >>>>> > >>>>>> I don't see the point of using channel idx and pwm checking here. > >>>>>> > >>>>>> There is one clockevent, why create multiple channels ? Can't you > >>>>>> stick > >>>>>> to the usual init routine for a timer. > >>>>> > >>>>> So the idea is that we use all the TCU channels that won't be used > >>>>> for PWM > >>>>> as timers. Hence the PWM checking. Why is this bad? > >>>> > >>>> It is not bad but arguable. By checking the channels used by the pwm in > >>>> the code, you introduce an adherence between two subsystems even if it > >>>> is just related to the DT parsing part. > >>>> > >>>> As it is not needed to have more than one timer in the time framework > >>>> (at least with the same characteristics), the pwm channels check is > >>>> pointless. We can assume the author of the DT file is smart enough to > >>>> prevent conflicts and define a pwm and a timer properly instead of > >>>> adding more code complexity. > >>>> > >>>> In addition, simplifying the code will allow you to use the timer-of > >>>> code and reduce very significantly the init function. > >>> > >>> That's what I had in my V1 and V2, my DT node for the timer-ingenic > >>> driver > >>> had a "timers" property (e.g. "timers = <4 5>;") to select the channels > >>> that > >>> should be used as timers. Then Rob told me I shouldn't do that, and > >>> instead > >>> detect the channels that will be used for PWM. > >>> > >> > >> [ ... ] > >> > >> How do you specify the channels used for PWM ? > > > > To detect the channels that will be used as PWM I parse the whole > > devicetree > > searching for "pwms" properties; check that the PWM handle is for our > > TCU PWM > > driver; then read the PWM number from there. > > > > Of course it's hackish, and it only works for devicetree. I preferred the > > method with the "timers" property. > > Do you have a DT portion describing that? Eg somewhere in the kernel's > git tree ? > > From what I understood, we can specify the channel for a pwm but not for > a timer, there is certainly something I'm missing. No, it was something custom. There is no standard way to specify a channel to use for a timer. > >>>>>>> > >>>>>>> +config INGENIC_TIMER > >>>>>>> + bool "Clocksource/timer using the TCU in Ingenic JZ SoCs" > >>>>>>> + depends on MACH_INGENIC || COMPILE_TEST > >>>>>> > >>>>>> bool "Clocksource/timer using the TCU in Ingenic JZ SoCs" if > >>>>>> COMPILE_TEST > >>>>>> > >>>>>> Remove the depends MACH_INGENIC. > >>>>> > >>>>> This driver is not useful on anything else than Ingenic SoCs, why > >>>>> should I > >>>>> remove MACH_INGENIC then? > >>>> > >>>> For COMPILE_TEST on x86. > >>> > >>> Well that's a logical OR right here, so it will work... > >> > >> Right, I missed the second part of the condition. For consistency > >> reason, we don't add a dependency on the platform. The platform will > >> select it. Look the other timer options and you will see there is no > >> MACH deps. I'm trying consolidating all these options to have same > >> format and hopefully factor them out. > > > > I'm all for factorisation, but what I dislike with not depending on > > MACH_INGENIC, is that the driver now appears in the menuconfig for > > every arch, even if it only applies to one MIPS SoC. > > Can you do the following change? > > bool "Clocksource/timer using the TCU in Ingenic JZ SoCs" if COMPILE_TEST > > so it will appear only when the COMPILE_TEST option is set whatever the > platform which is the purpose of this option to increase compile test > coverage. Ok, I get it now. It won't appear in the menuconfig unless COMPILE_TEST is selected, but I can still select it from the platform. Thanks! -Paul
