lkml.org 
[lkml]   [2020]   [Mar]   [17]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v1] clocksource: Avoid creating dead devices
From
Date
On 17/03/2020 19:08, Saravana Kannan wrote:
> On Tue, Mar 17, 2020 at 4:36 AM Paul Cercueil <paul@crapouillou.net> wrote:

[ ... ]

>>>> >> Why not just set the OF_POPULATED if the probe succeeds?
>>>> >>
>>>> >> Like:
>>>> >>
>>>> >> diff --git a/drivers/clocksource/timer-probe.c
>>>> >> b/drivers/clocksource/timer-probe.c
>>>> >> index ee9574da53c0..f290639ff824 100644
>>>> >> --- a/drivers/clocksource/timer-probe.c
>>>> >> +++ b/drivers/clocksource/timer-probe.c
>>>> >> @@ -35,6 +35,7 @@ void __init timer_probe(void)
>>>> >> continue;
>>>> >> }
>>>> >>
>>>> >> + of_node_set_flag(np, OF_POPULATED);
>>>> >> timers++;
>>>> >> }
>>>> >>
>>>> >> instead of setting the flag and clearing it in case of failure?
>>>> >
>>>> > Looking at IRQ framework which first did it the way you suggested
>>>> and
>>>> > then changed it to the way I did it, it looks like it allows for
>>>> > drivers that need to split the initialization between early init
>>>> (not
>>>> > just error out, but init partly) and later driver probe. See [1].
>>>> >
>>>> > Also, most of the other frameworks that set OF_POPULATED, set it
>>>> > before calling the initialization function for the device. Maybe
>>>> it's
>>>> > to make sure the device node data "looks the same" whether a
>>>> device is
>>>> > initialized during early init or during normal device probe
>>>> (since the
>>>> > OF_POPULATED is set before the probe is called) -- i.e. have
>>>> > OF_POPULATED set before the device initialization code is
>>>> actually
>>>> > run?
>>>> >
>>>> > Honestly I don't have a strong opinion either way, but I lean
>>>> towards
>>>> > following what IRQ does.
>>>>
>>>> Thanks for the pointer. Indeed it is to catch situation where the
>>>> driver
>>>> is clearing the flag like:
>>>>
>>>>
>>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/clocksource/ingenic-timer.c#n245
>>>>
>>>> But I'm not able to figure out why it is cleared here :/
>>>
>>> I think I know what's going on. He wants to implement PM support for
>>> this timer. But PM support is tied to devices. So, clearing out the
>>> flag allows creating the device which then hooks into PM ops.
>>
>> That's correct - the OF_POPULATED flag is cleared so that the driver
>> will probe as a platform_device. When I did write the driver this was
>> required or the platform_device would not probe.
>
> Interesting. I went and looked at the kernel when your patch merged.
> As far as I can tell, you shouldn't have needed to clear OF_POPULATED
> because the timer framework never set OF_POPULATED even back then.
>
> If this driver was based in drivers/irqchip/irq-ingenic-tcu.c and you
> were initially just trying to get it to create a device, then you'd
> have needed to clear OF_POPULATED because IRQ chip framework does set
> the flag.
>
> In any case, it's good that you cleared it -- it'll continue to work
> with my patch.
>
> Daniel,
>
> Looks like this answers all the concerns you had. I also checked every
> driver in drivers/clocksource that had the word "probe" in it to make
> sure it won't need any updates to ingenic-timer.c. Can we merge this?

Applied [1], thanks

-- Daniel

[1]
https://git.linaro.org/people/daniel.lezcano/linux.git/commit/?h=timers/drivers/next&id=4f41fe386a94639cd9a1831298d4f85db5662f1e


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

\
 
 \ /
  Last update: 2020-03-17 19:19    [W:0.067 / U:0.008 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site