Re: [PATCH v3 8/9] clocksource: Add a new timer-ingenic driver

[Paul Cercueil <paul@crapouillou.net>]

Hi Rob,

Le jeu. 11 janv. 2018 à 15:53, Rob Herring <robh+dt@kernel.org> a 
écrit :
> On Wed, Jan 10, 2018 at 4:48 PM, Paul Cercueil <paul@crapouillou.net> 
> 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.
>> 
>>  Signed-off-by: Paul Cercueil <paul@crapouillou.net>
>>  ---
>>   drivers/clocksource/Kconfig         |   8 ++
>>   drivers/clocksource/Makefile        |   1 +
>>   drivers/clocksource/timer-ingenic.c | 258 
>> ++++++++++++++++++++++++++++++++++++
>>   3 files changed, 267 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
> 
> [...]
> 
>>  +static int __init ingenic_tcu_init(struct device_node *np)
>>  +{
>>  +       unsigned long available_channels = GENMASK(NUM_CHANNELS - 
>> 1, 0);
>>  +       struct device_node *node;
>>  +       struct ingenic_tcu *tcu;
>>  +       unsigned int i, channel;
>>  +       int err;
>>  +       u32 val;
>>  +
>>  +       for_each_node_with_property(node, "pwms") {
>>  +               err = of_property_read_u32_index(node, "pwms", 1, 
>> &val);
> 
> This is the right idea, but a bit fragile. Perhaps its good enough for
> your platform, but it would fail if you have another PWM provider like
> the gpio-pwm binding or the cell size is not 1 (BTW, I thought the PWM
> binding defined 3 cells typically).

Index 1 is always the channel number of the PWM, it works fine with 
3-cells
PWM bindings, that's what I tested it with.

Ok, would it be enough to check that "the PWM clients we detect are 
connected
to the PWM driver whose parent is also our parent" (since both drivers 
are in
the same syscon/simple-mfd node)?

Thanks,
-Paul