Re: [PATCH v6 3/7] leds: add driver for LEDs from qnap-mcu devices

["Heiko Stübner" <heiko@sntech.de>]

Am Donnerstag, 29. August 2024, 18:27:05 CEST schrieben Sie:
> On Sun, 25 Aug 2024, Heiko Stuebner wrote:
> 
> > This adds a driver that connects to the qnap-mcu mfd driver and provides
> > access to the LEDs on it.
> > 
> > Signed-off-by: Heiko Stuebner <heiko@sntech.de>

[...]

> > +{
> > +	struct qnap_mcu_err_led *err_led = cdev_to_qnap_mcu_err_led(led_cdev);
> > +	u8 cmd[] = { 0x40, 0x52, 0x30 + err_led->num, 0x30 };
> 
> Really not fan of these magic values being used raw like this.

Me neither, I tried my luck with QNAP support to get some sort of
documentation on what these magic characters mean, and it did
even get up to some "product team" but ultimately they decided
against providing any help.

But ok, if it makes you feel more at ease, I'll switch to at least
replaying ascii-values where possible :-) .


> > +static int qnap_mcu_err_led_blink_set(struct led_classdev *led_cdev,
> > +				      unsigned long *delay_on,
> > +				      unsigned long *delay_off)
> > +{
> > +	struct qnap_mcu_err_led *err_led = cdev_to_qnap_mcu_err_led(led_cdev);
> > +	u8 cmd[] = { 0x40, 0x52, 0x30 + err_led->num, 0x30 };
> > +
> > +	/* LED is off, nothing to do */
> > +	if (err_led->mode == QNAP_MCU_ERR_LED_OFF)
> > +		return 0;
> > +
> > +	if (*delay_on < 500) {
> 
> Setting delay_on based on the current value of delay_on sounds sketchy.

As far as I understood the API, the parameter should indicated the wanted
blink time, while the function then should set in those variables the delay
the driver actually set.

So if the delay_on is < 500, select the fast blink mode, which is this
100/100 variant and for everything >= 500 the slow blink mode.

I.e. you set the trigger to "timer" and this creates the delay_on and
delay_off sysfs files, where you put you wish into and can read the
actually set delays out of.


> > +		*delay_on = 100;
> > +		*delay_off = 100;
> > +		err_led->mode = QNAP_MCU_ERR_LED_BLINK_FAST;
> > +	} else {
> > +		*delay_on = 500;
> > +		*delay_off = 500;
> > +		err_led->mode = QNAP_MCU_ERR_LED_BLINK_SLOW;
> > +	}
> 
> How do you change from a fast to a slow blinking LED and back again?

echo timer > /sys/class/leds/hdd4:red:status/trigger
## creates delay_on + delay_off sysfs files

echo 150 > /sys/class/leds/hdd4:red:status/delay_on
cat /sys/class/leds/hdd4:red:status/delay_on --> 100

echo 250 > /sys/class/leds/hdd4:red:status/delay_on
cat /sys/class/leds/hdd4:red:status/delay_on --> 100

## switch to slow blink

echo 500 > /sys/class/leds/hdd4:red:status/delay_on
cat /sys/class/leds/hdd4:red:status/delay_on --> 500

echo 600 > /sys/class/leds/hdd4:red:status/delay_on
cat /sys/class/leds/hdd4:red:status/delay_on --> 500

## switch to fast blink again

echo 150 > /sys/class/leds/hdd4:red:status/delay_on
cat /sys/class/leds/hdd4:red:status/delay_on --> 100

echo heartbeat > /sys/class/leds/hdd4:red:status/trigger
## removes the delay_on + delay_off files again


Heiko