Hi Jacek,
On Sun, 2016-10-23 at 16:12 +0200, Jacek Anaszewski wrote:
> Hi Julia,
>
> On 10/21/2016 11:33 PM, Julia Cartwright wrote:
> >
> > 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?
> brightness_set op can be called from atomic context e.g. in case of
> timer trigger. The code that runs in the interrupt context cannot
> sleep
> due to reasons explained in [0], whereas mutex can sleep on
> contention.
>
> spin_lock() is appropriate only in a hard irq handler.
>
> >
> > 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 :).
> [0] http://www.makelinux.net/ldd3/chp-5-sect-5
>
I'm update the driver to use spin_lock_irqsave. Thanks for the
feedback.��.n��������+%������w��{.n�����{��W����ܨ}���Ơz�j:+v�����w����ޙ��&�)ߡ�a����z�ޗ���ݢj��w�f
