On Fri, Oct 21, 2016 at 02:56:15PM +0200, Jacek Anaszewski wrote:
> Hi Hui,
>
> Thanks for the patch. I have few comments in the code below.
>
> On 10/21/2016 08:33 AM, Hui Chun Ong wrote:
> > Add the driver to support User LEDs on PXI Embedded Controller.
> >
> > Signed-off-by: Hui Chun Ong <hui.chun.ong@xxxxxx>
> > Signed-off-by: Brad Mouring <brad.mouring@xxxxxx>
[..]
>
> > +
> > +struct ni78bx_led {
> > + u8 bit;
> > + u8 mask;
> > + struct led_classdev cdev;
> > +};
> > +
> > +static inline struct ni78bx_led *to_ni78bx_led(struct led_classdev *cdev)
> > +{
> > + return container_of(cdev, struct ni78bx_led, cdev);
> > +}
> > +
> > +static void ni78bx_brightness_set(struct led_classdev *cdev,
> > + enum led_brightness brightness)
> > +{
> > + struct ni78bx_led *nled = to_ni78bx_led(cdev);
> > + u8 value;
> > +
> > + mutex_lock(&led_lock);
>
> You can use spin_lock_irqsave() instead, since we are not going to sleep
> while accessing memory mapped registers.
Could you explain why spin_lock_irqsave() is appropriate here? This
device doesn't have an interrupt to be synchronized with. I could
understand spin_lock()/spin_unlock(), maybe, but why disable interrupts?
Is what you're saying that the set/get brightness calls can be re-entered
by LED consumers in hardirq context?
The systems this driver is deployed to is much more sensitive to RT
execution latency than anything else, which is why I take pause to
unnecessary interrupt mask twiddling/preemption disabling :).
Julia
--
To unsubscribe from this list: send the line "unsubscribe linux-leds" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
