Re: [PATCH v4 7/8] clocksource: Add a new timer-ingenic driver

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





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.

 Signed-off-by: Paul Cercueil <paul@xxxxxxxxxxxxxxx>
 ---
  drivers/clocksource/Kconfig         |   8 ++
  drivers/clocksource/Makefile        |   1 +
  drivers/clocksource/timer-ingenic.c | 278
 ++++++++++++++++++++++++++++++++++++
  3 files changed, 287 insertions(+)
  create mode 100644 drivers/clocksource/timer-ingenic.c

  v2: Use SPDX identifier for the license
  v3: - Move documentation to its own patch
      - Search the devicetree for PWM clients, and use all the TCU
        channels that won't be used for PWM
  v4: - Add documentation about why we search for PWM clients
      - Verify that the PWM clients are for the TCU PWM driver

diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
 index d2e5382821a4..481422145fb4 100644
 --- a/drivers/clocksource/Kconfig
 +++ b/drivers/clocksource/Kconfig
 @@ -592,4 +592,12 @@ config CLKSRC_ST_LPC
        Enable this option to use the Low Power controller timer
        as clocksource.

 +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...

 +    select CLKSRC_OF
 +    default y

 No default, Kconfig platform selects the timer.

 Alright.
[ ... ]

--
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog




[Index of Archives]     [Linux MIPS Home]     [LKML Archive]     [Linux ARM Kernel]     [Linux ARM]     [Linux]     [Git]     [Yosemite News]     [Linux SCSI]     [Linux Hams]

  Powered by Linux