Re: [PATCH 4/5] leds: add ChromeOS EC driver

[Thomas Weißschuh]

On 2024-05-28 07:15:07+0000, Tzung-Bi Shih wrote:
> On Tue, May 28, 2024 at 07:25:07AM +0200, Thomas Weißschuh wrote:
> > On 2024-05-28 05:09:29+0000, Tzung-Bi Shih wrote:
> > > On Mon, May 20, 2024 at 12:00:32PM +0200, Thomas Weißschuh wrote:
> > > > +static int __init cros_ec_led_init(void)
> > > > +{
> > > > +	int ret;
> > > > +
> > > > +	ret = led_trigger_register(&cros_ec_led_trigger);
> > > > +	if (ret)
> > > > +		return ret;
> > > > +
> > > > +	ret = platform_driver_register(&cros_ec_led_driver);
> > > > +	if (ret)
> > > > +		led_trigger_unregister(&cros_ec_led_trigger);
> > > > +
> > > > +	return ret;
> > > > +};
> > > > +module_init(cros_ec_led_init);
> > > > +
> > > > +static void __exit cros_ec_led_exit(void)
> > > > +{
> > > > +	platform_driver_unregister(&cros_ec_led_driver);
> > > > +	led_trigger_unregister(&cros_ec_led_trigger);
> > > > +};
> > > > +module_exit(cros_ec_led_exit);
> > > 
> > > I wonder it could use module_led_trigger() and module_platform_driver().
> > 
> > This won't compile as the macros generate various duplicate symbols.
> > 
> > Also the order is important, so I think the explicit logic is clearer.
> 
> I'm not sure if it is feasible by separating the trigger part to
> drivers/leds/trigger/ and specify it in `default_trigger`.

I don't think so.

The trigger is a private one and can only ever used with those LEDs.
(through cros_ec_led_trigger_type)

If we want to split it out we would need to export at least
cros_ec_led_trigger_type, cros_ec_led_cdev_to_priv,
cros_ec_led_cmd_arg_data, cros_ec_led_priv and more from leds-cros_ec to
the trigger.