Hello Pavel,
On Fri, May 25, 2018 at 11:12:58AM +0200, Pavel Machek wrote:
> > 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.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
