Re: [PATCH] leds: Add user LED driver for NIC78bx device

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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



[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux