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

[Maciek Borzecki <maciek.borzecki@gmail.com>]

On 10/01 09:47, Josh Cartwright wrote:
> 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@gmail.com>
> > ---
> [..]
> > +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?
>
Thanks for the comments.

The first proof of concept used `sturct device` as parameter in all API
calls, but then there's a problem of naming of a trigger in a sane
way. The trigger name followed the same approach as __dev_printk, and
the naming was done in this fashion:

   sprintf(..., "dev-%s-%s", dev_driver_string(dev), dev_name(dev));

Then for instance on wandboard, /dev/ttymxc0 and /dev/ttymxc2 would
appear as `dev-serial-2020000` and `dev-serial-21ec000`. In my opinion
this was unnecessarily complicated. Also, if I'm not mistaken, using
this approach the partitions on MMC card or SATA drive would end up with
the same trigger name, as it is a single device. However the the
major:minor numbers assigned to respective partitions are different, and
you'd still be able to say trigger the LEDS on writes to a particular
partition.

Multiple dev nodes will already have different minor numbers, so
their dev_t is different anyway.

As for devices that do not have a dev_t assigned to them one can still
pass a custom tag in ledtrig_dev_add(). It's just a number so as long as
there's no collision in numbering things should be fine.

Hopefull this clears up the things a little.

> > +{
> > +	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?

I'm ok with EXPORT_SYMBOL_GPL() if that's a policy for new code. Though,
I've looked at other triggers that are called from kernel code, and it
seems that ledtrig-camera is the only one using _GPL.

--
Maciek Borzecki