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
--
Best regards,
Jacek Anaszewski
--
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
