Re: [PATCH v2 51/53] leds: Add driver for the SGI Fuel and Tezro front-panel LEDs

[Imre Kaloz <kaloz@kernel.org>]

On Tue, 20 Oct 2026, Lee Jones wrote:

> > +struct ip35_leds;
>
> Can you find another way to avoid: leds->led->leds->led->leds ...

I can, and it comes out nicer too. The LED core registers the classdev
with &pdev->dev as its parent, so the callbacks can get at the shared
state through dev_get_drvdata(cdev->dev->parent). I'll set it with
platform_set_drvdata() before the registration loop, drop the forward
declaration and the back-pointer, and have the update helper take both
the shared state and the LED:

	static int ip35_led_update(struct ip35_leds *leds,
				   struct ip35_led *led, unsigned int state)

> > +	struct mutex lock;	/* protects ->state of both LEDs */
>
> Not sure that comment is required.

Agreed, gone.

> > +	mutex_lock(&leds->lock);
>
> guard(mutex)(&leds->lock) ?

Will do. The explicit mutex_unlock() goes with it.

> > +	for (i = 0; i < ARRAY_SIZE(leds->led); i++) {
>
> for (int i = 0; ...

Will do, here and in probe.

> > +	*delay_on = 1000;
> > +	*delay_off = 1000;
>
> If the hardware only supports a fixed 2s period (1000ms on / 1000ms
> off), shouldn't we only accept it when '*delay_on == 0 && *delay_off
> == 0' (or when both are already 1000) and return '-EINVAL' otherwise
> so the LED core falls back to software blinking for other requested
> rates?

You're right, and that is the better behaviour.

> > +static struct platform_driver ip35_leds_driver = {
> > +	.probe		= ip35_leds_probe,
> > +	.driver		= {
> > +		.name	= "ip35-leds",
> > +	},
> > +};
>
> This padding is creeping me out.
>
> Why would you line-up things that aren't at the same level?

Old habits, and no good reason beyond that :) The tab alignment on
.probe, .driver and .name goes; v3 will use a single space before each '='.


Thanks a lot for the thorough review,

Imre