Re: [PATCH] leds-fsg: Change order of initialization anddeinitialization

[Sven Wegener]

On Wed, 30 Jul 2008, Andrew Morton wrote:

> On Sun, 27 Jul 2008 20:30:59 +0200 (CEST) Sven Wegener <sven.wegener@xxxxxxxxxxx> wrote:
> 
> > --- a/drivers/leds/leds-fsg.c
> > +++ b/drivers/leds/leds-fsg.c
> > @@ -161,6 +161,16 @@ static int fsg_led_probe(struct platform_device *pdev)
> >  {
> >  	int ret;
> >  
> > +	/* Map the LED chip select address space */
> > +	latch_address = (unsigned short *) ioremap(IXP4XX_EXP_BUS_BASE(2), 512);
> > +	if (!latch_address) {
> > +		ret = -ENOMEM;
> > +		goto failremap;
> > +	}
> > +
> > +	latch_value = 0xffff;
> > +	*latch_address = latch_value;
> 
> Mutter.  Shouldn't this driver be using readw/writew?  It's not even
> using a volatile pointer, so there's a decent risk that the compiler
> will optimise away important things..

I don't own a FSG-3, so I can't test my changes. Currently I don't have a 
arm cross toolchain, so I even can't compile-test anything. But you mean 
something along the lines below on top of the patch? Don't know if we need 
to writew an initial value or if the hardware sets it up properly. Using 
__raw functions to avoid the cpu_to_le16 conversion as the conversion 
isn't there in the current code.

diff --git a/drivers/leds/leds-fsg.c b/drivers/leds/leds-fsg.c
index a26fe36..42fb09d 100644
--- a/drivers/leds/leds-fsg.c
+++ b/drivers/leds/leds-fsg.c
@@ -22,80 +22,55 @@
 #include <asm/arch/hardware.h>
 #include <asm/io.h>
 
-static short __iomem *latch_address;
-static unsigned short latch_value;
+static unsigned short __iomem *latch_address;
 
 
+static void fsg_led_set(enum led_brightness value, int bit)
+{
+	unsigned short latch_value = __raw_readw(latch_address);
+
+	if (value)
+		latch_value &= ~(1 << bit);
+	else
+		latch_value |=  (1 << bit);
+
+	__raw_writew(latch_value, latch_address);
+}
+
 static void fsg_led_wlan_set(struct led_classdev *led_cdev,
 			     enum led_brightness value)
 {
-	if (value) {
-		latch_value &= ~(1 << FSG_LED_WLAN_BIT);
-		*latch_address = latch_value;
-	} else {
-		latch_value |=  (1 << FSG_LED_WLAN_BIT);
-		*latch_address = latch_value;
-	}
+	fsg_led_set(value, FSG_LED_WLAN_BIT);
 }
 
 static void fsg_led_wan_set(struct led_classdev *led_cdev,
 			    enum led_brightness value)
 {
-	if (value) {
-		latch_value &= ~(1 << FSG_LED_WAN_BIT);
-		*latch_address = latch_value;
-	} else {
-		latch_value |=  (1 << FSG_LED_WAN_BIT);
-		*latch_address = latch_value;
-	}
+	fsg_led_set(value, FSG_LED_WAN_BIT);
 }
 
 static void fsg_led_sata_set(struct led_classdev *led_cdev,
 			     enum led_brightness value)
 {
-	if (value) {
-		latch_value &= ~(1 << FSG_LED_SATA_BIT);
-		*latch_address = latch_value;
-	} else {
-		latch_value |=  (1 << FSG_LED_SATA_BIT);
-		*latch_address = latch_value;
-	}
+	fsg_led_set(value, FSG_LED_SATA_BIT);
 }
 
 static void fsg_led_usb_set(struct led_classdev *led_cdev,
 			    enum led_brightness value)
 {
-	if (value) {
-		latch_value &= ~(1 << FSG_LED_USB_BIT);
-		*latch_address = latch_value;
-	} else {
-		latch_value |=  (1 << FSG_LED_USB_BIT);
-		*latch_address = latch_value;
-	}
+	fsg_led_set(value, FSG_LED_USB_BIT);
 }
 
 static void fsg_led_sync_set(struct led_classdev *led_cdev,
 			     enum led_brightness value)
 {
-	if (value) {
-		latch_value &= ~(1 << FSG_LED_SYNC_BIT);
-		*latch_address = latch_value;
-	} else {
-		latch_value |=  (1 << FSG_LED_SYNC_BIT);
-		*latch_address = latch_value;
-	}
+	fsg_led_set(value, FSG_LED_SYNC_BIT);
 }
 
 static void fsg_led_ring_set(struct led_classdev *led_cdev,
 			     enum led_brightness value)
 {
-	if (value) {
-		latch_value &= ~(1 << FSG_LED_RING_BIT);
-		*latch_address = latch_value;
-	} else {
-		latch_value |=  (1 << FSG_LED_RING_BIT);
-		*latch_address = latch_value;
-	}
+	fsg_led_set(value, FSG_LED_RING_BIT);
 }
 
 
@@ -168,9 +143,6 @@ static int fsg_led_probe(struct platform_device *pdev)
 		goto failremap;
 	}
 
-	latch_value = 0xffff;
-	*latch_address = latch_value;
-
 	ret = led_classdev_register(&pdev->dev, &fsg_wlan_led);
 	if (ret < 0)
 		goto failwlan;
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/