Re: [PATCH leds v3 06/11] leds: turris-omnia: Notify sysfs on MCU global LEDs brightness change

[Lee Jones <lee@kernel.org>]

On Fri, 13 Sep 2024, Marek Behún wrote:

> Recall that on Turris Omnia, the LED controller has a global brightness
> property, which allows the user to make the front LED panel dimmer.
> 
> There is also a button on the front panel, which by default is
> configured so that pressing it changes the global brightness to a lower
> value (unless it is at 0%, in which case pressing the button changes the
> global brightness to 100%).
> 
> Newer versions of the MCU firmware support informing the SOC that the
> brightness was changed by button press event via an interrupt.
> 
> Now that we have the turris-omnia-mcu driver, which adds support for MCU
> interrupts, add the ability to inform the userspace (via a sysfs
> notification) that the global brightness was changed.
> 
> Signed-off-by: Marek Behún <kabel@kernel.org>
> ---
>  drivers/leds/leds-turris-omnia.c | 37 ++++++++++++++++++++++++++++++++
>  1 file changed, 37 insertions(+)
> 
> diff --git a/drivers/leds/leds-turris-omnia.c b/drivers/leds/leds-turris-omnia.c
> index 82cf58fbe946..a87cdb58e476 100644
> --- a/drivers/leds/leds-turris-omnia.c
> +++ b/drivers/leds/leds-turris-omnia.c
> @@ -33,6 +33,7 @@ struct omnia_leds {
>  	struct i2c_client *client;
>  	struct mutex lock;
>  	bool has_gamma_correction;
> +	struct kernfs_node *brightness_kn;

Variable nomenclature should be self documenting.

What is kn?  Please improve this.

>  	struct omnia_led leds[];
>  };
>  
> @@ -357,6 +358,21 @@ static struct attribute *omnia_led_controller_attrs[] = {
>  };
>  ATTRIBUTE_GROUPS(omnia_led_controller);
>  
> +static irqreturn_t omnia_brightness_changed_handler(int irq, void *dev_id)

Why dev_id?  This appears to be driver data.  ddata sounds more applicable.

> +{
> +	struct omnia_leds *leds = dev_id;
> +
> +	if (unlikely(!leds->brightness_kn)) {
> +		leds->brightness_kn = sysfs_get_dirent(leds->client->dev.kobj.sd, "brightness");

NACK.  This will sleep in IRQ context.

> +		if (!leds->brightness_kn)
> +			return IRQ_NONE;
> +	}
> +
> +	sysfs_notify_dirent(leds->brightness_kn);
> +
> +	return IRQ_HANDLED;
> +}
> +
>  static int omnia_mcu_get_features(const struct i2c_client *client)
>  {
>  	struct i2c_client mcu_client = *client;
> @@ -420,6 +436,14 @@ static int omnia_leds_probe(struct i2c_client *client)
>  			 "Consider upgrading MCU firmware with the omnia-mcutool utility.\n");
>  	}
>  
> +	if (ret & OMNIA_FEAT_BRIGHTNESS_INT) {
> +		ret = devm_request_any_context_irq(dev, client->irq,
> +						   omnia_brightness_changed_handler, IRQF_ONESHOT,
> +						   "leds-turris-omnia", leds);
> +		if (ret <= 0)
> +			return dev_err_probe(dev, ret ?: -ENXIO, "Cannot request brightness IRQ\n");
> +	}
> +
>  	mutex_init(&leds->lock);
>  
>  	ret = devm_led_trigger_register(dev, &omnia_hw_trigger);
> @@ -442,6 +466,19 @@ static int omnia_leds_probe(struct i2c_client *client)
>  
>  static void omnia_leds_remove(struct i2c_client *client)
>  {
> +	struct omnia_leds *leds = i2c_get_clientdata(client);
> +
> +	/*
> +	 * We need to free the brightness IRQ here, before putting away the brightness sysfs node.
> +	 * Otherwise devres would free the interrupt only after the sysfs node is removed, and if
> +	 * an interrupt occurred between those two events, it would use a removed sysfs node.
> +	 */
> +	devm_free_irq(&client->dev, client->irq, leds);
> +
> +	/* Now put away the sysfs node we got the first time the interrupt handler was called */
> +	if (leds->brightness_kn)
> +		sysfs_put(leds->brightness_kn);
> +
>  	/* put all LEDs into default (HW triggered) mode */
>  	omnia_cmd_write_u8(client, OMNIA_CMD_LED_MODE, OMNIA_CMD_LED_MODE_LED(OMNIA_BOARD_LEDS));
>  
> -- 
> 2.44.2
> 

-- 
Lee Jones [李琼斯]