Re: [PATCH v2 1/1] leds: syscon: Add tristate option

[Bevan Weiss]

On Thu, 19 Mar 2026 10:13:27 +0000
Lee Jones <lee@xxxxxxxxxx> wrote:

> > Additional changes:
> > - (int)(struct platform_device *pdev) => void return  
> 
> Please rephrase into a human parseable sentence.

Thanks, will do.

> > +	led_classdev_unregister(&sled->cdev);  
> 
> The driver uses devm_led_classdev_register() in probe, which
> automatically handles unregistering the LED class device when the
> driver is detached. Calling it here manually will result in a
> double-unregister.

My oversight, will fix.

> > +	/* Turn it off */  
> 
> Turn what off?
> 
> > +	regmap_update_bits(sled->map, sled->offset, sled->mask,
> > 0);  
> 
> If 0 was defined, as it should be, then you can drop the comment.

This is carry over from the original source, when it was originally
modularised.  I'll update the comment to explicitly mention that it's
the LED which is turned off on module remove.

> > +MODULE_LICENSE("GPL");  
> 
> The MODULE_LICENSE() string should match the SPDX identifier at the
> top of the file.
> 

Sorry, I'm not quite understanding this in the context of other files.
Most modules within the led subsystem seem to have 
'GPL-2.0-or-later' as the SPDX identifier, and MODULE_LICENSE("GPL"),
as I currently have here.
Is there a different subtlety I'm missing?
e.g. leds-adp5520.c, leds-as3668.c, leds-blinkm.c
Perhaps MODULE_LICENSE("GPL v2+")? although I don't see similar
anywhere in the kernel.
I'd expect MODULE_LICENSE("GPL v2") to be for 'GPL-2.0-only' licenses.


Thanks and regards,
Bevan Weiss