On Tue, 2016-09-06 at 12:04 +0200, Thierry Reding wrote:

On Tue, Sep 06, 2016 at 11:14:45AM +0200, Neil Armstrong wrote:

quoted

On 09/06/2016 11:07 AM, Thierry Reding wrote:

quoted

On Tue, Sep 06, 2016 at 10:36:49AM +0200, Neil Armstrong wrote:

quoted

Hi Thierry,

[...]

quoted

quoted

The second bug is in probe(), I understand the point to
allocate
dynamically the channels and attach them to each pwm chip, but
when
calling meson_pwm_init_channels() we get an OOPS because
meson->chip.pwms[i] are allocated in pwmchip_add(). Moving
meson_pwm_init_channels() would fix this, but in case of a clk
PROBE_DEFER, we would need to remove back the pwmchip, which is
a
quite a bad design decision....

Ah yes... that one again. I remember running into that a while
ago with
some other driver. To be honest, I think that's a short-coming of
the
PWM subsystem and the fix would be for PWM chip registration to
be split
into two parts: pwm_chip_init() and pwm_chip_add(). That way, a
chip
would be initialized using pwm_chip_init() where the pwms array
would be
allocated, and pwm_chip_add() would register the chip with the
system.

Currently a few drivers might be vulnerable to a race condition
between
registration and implementation (i.e. PWM channels aren't fully
set up
when they are exposed to users and sysfs).

quoted

The smartest fix I found was to allocate channels in probe,
init them
them attach them after pwmchip_add():

[...]

quoted

That's the race I was talking about above. I suppose it's not too
big an
issue since other drivers seem to manage, so I'm going to merge
your
fixed driver.

ok thanks !

I've made a few tiny changes (reg -> offset, temporary variable to
track
&channels[i], ...) and pushed it all out. Hopefully that now fixes
any
of the remaining issues.

quoted

quoted

Unless you feel like taking a stab at the
pwm_chip_init()/pwm_chip_add()
split, in which case your driver would be the first to be race-
free. =)

Having he driver upstream is a priority, but having it completely
race-free would be great! I'll be happy to collaborate to a race-
free
pwmchip probe somehow !

Fair enough. I'll do some prototyping and keep you in the loop if I
come
up with something that I think will do.

Thierry

Hi Thierry,

I have tested the latest version on the P200 (S905), channels E and F.
It works as expected.

Regards

Tested-by: Jerome Brunet <jbrunet@baylibre.com>