Hi!
> > > On Thu, May 24, 2018 at 01:54:26PM +0200, Pavel Machek wrote:
> > > > > Given that activating a trigger can fail, let the callback return an
> > > > > indication. This prevents to have a trigger active according to the
> > > > > "trigger" sysfs attribure but not functional.
> > > > >
> > > > > This is done using a new function callback .new_activate to not break
> > > > > existing drivers and to allow to convert one after the other to the new
> > > > > mechanism. Once all users are converted, .activate can be dropped and
> > > > > .new_activate renamed.
> > > >
> > > > No, this is not reasonable.
> > > >
> > > > Instead of big rename, replace return value in ->activate with int. It
> > > > will currently return 0 for all drivers.
> > >
> > > No, currently the .activate callbacks "return" void, which e.g. on ARM
> > > means that the r0 register (which holds the return value when the return
> > > type is int) contains an unspecified value which most of the time is not
> > > zero.
> >
> > Yes, you need to change the .activate callbacks to return int (and add
> > return 0 there).
>
> That's from my POV more complicated because then I have a single patch
> that touches not only drivers/leds but also triggers in other
> directories. And if I break any driver, there is no simple commit to
> revert. And out-of-tree-triggers are broken, too.
>
> > Yes, it will touch all triggers, but so would your proposal of "Once
> > all users are converted, .activate can be dropped and .new_activate
> > renamed.". Better do it now.
>
> This can also be done processing one driver after another, too, e.g. by
> doing:
>
> union {
> int (*activate)(...),
> int (*new_activate)(...),
> };
>
> in struct led_trigger and then when all drivers are converted, drop the
> union.
>
> The obvious downsides are:
>
> - ugly new_activate
> - in the sum more changes because there is an intermediate state
> - process takes maybe longer
>
> but IMHO the upsides outweight these:
>
> - one patch per driver that can be individually reviewed, applied and
> (if needed) reverted. So "in the sum more changes" is also an
> advantage.
> - patches to drivers/usb can go via the usb tree, so there is no need
> to synchronize different trees and maintainers. (So "process takes
> longer" is also good because people have more time.)
> - a given release can support both, converted and unconverted triggers
> which also helps out-of-tree drivers and slow maintainers.
> - I can convert trigger A and benefit from the conversion (e.g. by
> forgetting about A) before the changes to trigger B are reviewed and
> acked by the respective maintainer.
>
> So I really like my approach better. If however you ask me to do it in
> one single step and I only have to provide the patch to you and you then
> care about the synchronization and getting all the needed acks, I won't
> refuse my cooperation.
I believe downsides are not worth the upsides here. (And I worry that
we'll end up with both .activate and .new_activate forever).
I think conversion is trivial enough that it can be done in one
step. You just send the series, and give maintainers of different
trees "enough" time to acknowledge or object. And then we merge it.
Best regards,
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Attachment:
signature.asc
Description: Digital signature
