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

[Josh Cartwright]

On Fri, Oct 02, 2015 at 09:45:37AM +0200, Maciek Borzecki wrote:
> On 10/01 09:47, Josh Cartwright wrote:
> > 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?
> >
> 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.

Hmm, maybe we're bikeshedding at this point, but LEDs with those names
seem much more straightfoward to me than a "dev-<maj>:<min>" name, for
devices which have done dynamic dev_t allocation.

> 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.

This would only be true if you used _just_ the struct device.  I was
imagining that you'd specify a (struct device, unsigned index) pair.
Better, you could do a (struct device, const char *) pair.

Also, from a lifetime management perspective, it starts to feel like
something that might integrate better as a managed resource (devm_*).

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

Okay, backing up I don't really see what this API really buys the
consumer.  The dev_t -> struct led_trigger mapping just seems like a
total waste.  Why not just make your ledtrig_dev_add() function return
the struct led_trigger * that the consumer keeps track of?

Maybe seeing an example consumer would provide some clarification.

> 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.

Ensuring no collision will be difficult, especially given that it's most
common that the dynamic allocator is used.  In order to guarantee no
collisions, a user who doesn't expose any device nodes would need to do
their own dev_t allocation...to use this interface.  And that seems
silly to me.

  Josh

Attachment: signature.asc
Description: PGP signature