Re: [PATCH v2 1/3] leds: add device activity LED triggers

[Josh Cartwright]

Hello Maciek-

Some architectural questions below:

On Thu, Oct 01, 2015 at 04:04:31PM +0200, Maciek Borzecki wrote:
> The patch adds LED triggers for indicating an activity on a selected
> device. The drivers that intend to use triggers need to register
> respective devices using ledtrig_dev_add(). Triggers are generated by
> explicitly calling ledtrig_dev_activity().
> 
> Signed-off-by: Maciek Borzecki <maciek.borzecki@xxxxxxxxx>
> ---
[..]
> +struct ledtrig_dev_data {
> +	char name[MAX_NAME_LEN];
> +	dev_t dev;
> +	struct led_trigger *trig;
> +	struct list_head node;
> +};
> +
> +/**
> + * ledtrig_dev_activity - signal activity on device
> + * @dev: device
> + *
> + * Fires a trigger assigned to @dev device.
> + */
> +void ledtrig_dev_activity(dev_t dev)

It seems a bit strange to me to associate a device LED trigger with
dev_t.  Some devices don't expose a dev node, some devices expose
multiple dev nodes...

Is there a reason why you are not tying to the device model?

> +{
> +	struct ledtrig_dev_data *dev_trig;
> +
> +	if (!down_read_trylock(&devs_list_lock))
> +		return;
> +
> +	list_for_each_entry(dev_trig, &devs_list, node) {
> +		if (dev_trig->dev == dev) {
> +			led_trigger_blink_oneshot(dev_trig->trig,
> +						  &blink_delay,
> +						  &blink_delay,
> +						  0);
> +			break;
> +		}
> +	}
> +	up_read(&devs_list_lock);
> +}
> +EXPORT_SYMBOL(ledtrig_dev_activity);

Not _GPL?

  Josh

Attachment: signature.asc
Description: PGP signature