Skip to contentSkip to conversation

mfd: Add ASIC3 LED support

PATCH Superseded Paul Parsons 3 participants 8 messages Apr 30, 2011 – May 3, 2011

Subsystems (2)
Multifunction devices (MFD) LED

v1 of 4 Superseded Latest is v4 →

lore
Share & tools
lore

Paul Parsons

author
#0 lore raw

PATCH 1/2 mfd: Add ASIC3 LED support

Add LED support for the HTC ASIC3. Underlying support is provided by the mfd/asic3 and leds/leds-asic3 drivers. An example configuration is provided by the pxa/hx4700 platform.

Signed-off-by Paul Parsons <[email protected]>

---

drivers/mfd/asic3.c +67 -9
diff -uprN clean-2.6.39-rc5/drivers/mfd/asic3.c linux-2.6.39-rc5/drivers/mfd/asic3.c
@@ -88,19 +88,19 @@ struct asic3 {
 
 static int asic3_gpio_get(struct gpio_chip *chip, unsigned offset);
 
-static inline void asic3_write_register(struct asic3 *asic,
-				 unsigned int reg, u32 value)
+void asic3_write_register(struct asic3 *asic, unsigned int reg, u32 value)
 {
 	iowrite16(value, asic->mapping +
 		  (reg >> asic->bus_shift));
 }
+EXPORT_SYMBOL_GPL(asic3_write_register);
 
-static inline u32 asic3_read_register(struct asic3 *asic,
-			       unsigned int reg)
+u32 asic3_read_register(struct asic3 *asic, unsigned int reg)
 {
 	return ioread16(asic->mapping +
 			(reg >> asic->bus_shift));
 }
+EXPORT_SYMBOL_GPL(asic3_read_register);
 
 static void asic3_set_register(struct asic3 *asic, u32 reg, u32 bits, bool set)
 {

@@ -584,7 +584,7 @@ static int asic3_gpio_remove(struct plat
 	return gpiochip_remove(&asic->gpio);
 }
 
-static int asic3_clk_enable(struct asic3 *asic, struct asic3_clk *clk)
+static void asic3_clk_enable(struct asic3 *asic, struct asic3_clk *clk)
 {
 	unsigned long flags;
 	u32 cdex;

@@ -596,8 +596,6 @@ static int asic3_clk_enable(struct asic3
 		asic3_write_register(asic, ASIC3_OFFSET(CLOCK, CDEX), cdex);
 	}
 	spin_unlock_irqrestore(&asic->lock, flags);
-
-	return 0;
 }
 
 static void asic3_clk_disable(struct asic3 *asic, struct asic3_clk *clk)

@@ -782,7 +780,55 @@ static struct mfd_cell asic3_cell_mmc = 
 	.resources     = asic3_mmc_resources,
 };
 
+static const int clock_ledn[ASIC3_NUM_LEDS] = {
+	[0] = ASIC3_CLOCK_LED0,
+	[1] = ASIC3_CLOCK_LED1,
+	[2] = ASIC3_CLOCK_LED2,
+};
+
+static int asic3_leds_enable(struct platform_device *pdev)
+{
+	const struct mfd_cell *cell = mfd_get_cell(pdev);
+	struct asic3 *asic = dev_get_drvdata(pdev->dev.parent);
+
+	asic3_clk_enable(asic, &asic->clocks[clock_ledn[cell->id]]);
+
+	return 0;
+}
+
+static int asic3_leds_disable(struct platform_device *pdev)
+{
+	const struct mfd_cell *cell = mfd_get_cell(pdev);
+	struct asic3 *asic = dev_get_drvdata(pdev->dev.parent);
+
+	asic3_clk_disable(asic, &asic->clocks[clock_ledn[cell->id]]);
+
+	return 0;
+}
+
+static struct mfd_cell asic3_cell_leds[ASIC3_NUM_LEDS] = {
+	[0] = {
+		.name          = "leds-asic3",
+		.id            = 0,
+		.enable        = asic3_leds_enable,
+		.disable       = asic3_leds_disable,
+	},
+	[1] = {
+		.name          = "leds-asic3",
+		.id            = 1,
+		.enable        = asic3_leds_enable,
+		.disable       = asic3_leds_disable,
+	},
+	[2] = {
+		.name          = "leds-asic3",
+		.id            = 2,
+		.enable        = asic3_leds_enable,
+		.disable       = asic3_leds_disable,
+	},
+};
+
 static int __init asic3_mfd_probe(struct platform_device *pdev,
+				  struct asic3_platform_data *pdata,
 				  struct resource *mem)
 {
 	struct asic3 *asic = platform_get_drvdata(pdev);

@@ -820,9 +866,20 @@ static int __init asic3_mfd_probe(struct
 	if (ret < 0)
 		goto out;
 
-	if (mem_sdio && (irq >= 0))
+	if (mem_sdio && (irq >= 0)) {
 		ret = mfd_add_devices(&pdev->dev, pdev->id,
 			&asic3_cell_mmc, 1, mem_sdio, irq);
+		if (ret < 0)
+			goto out;
+	}
+
+	if (pdata->leds) {
+		asic3_cell_leds[0].mfd_data = &pdata->leds[0];
+		asic3_cell_leds[1].mfd_data = &pdata->leds[1];
+		asic3_cell_leds[2].mfd_data = &pdata->leds[2];
+		ret = mfd_add_devices(&pdev->dev, 0,
+			asic3_cell_leds, ASIC3_NUM_LEDS, NULL, 0);
+	}
 
  out:
 	return ret;

@@ -883,6 +940,7 @@ static int __init asic3_probe(struct pla
 		goto out_unmap;
 	}
 
+	asic->gpio.label = "asic3";
 	asic->gpio.base = pdata->gpio_base;
 	asic->gpio.ngpio = ASIC3_NUM_GPIOS;
 	asic->gpio.get = asic3_gpio_get;

@@ -903,7 +961,7 @@ static int __init asic3_probe(struct pla
 	 */
 	memcpy(asic->clocks, asic3_clk_init, sizeof(asic3_clk_init));
 
-	asic3_mfd_probe(pdev, mem);
+	asic3_mfd_probe(pdev, pdata, mem);
 
 	dev_info(asic->dev, "ASIC3 Core driver\n");
include/linux/mfd/asic3.h +19 -3
diff -uprN clean-2.6.39-rc5/include/linux/mfd/asic3.h linux-2.6.39-rc5/include/linux/mfd/asic3.h
@@ -16,6 +16,13 @@
 
 #include <linux/types.h>
 
+struct led_classdev;
+struct asic3_led {
+	const char	*name;
+	const char	*default_trigger;
+	struct led_classdev *cdev;
+};
+
 struct asic3_platform_data {
 	u16 *gpio_config;
 	unsigned int gpio_config_num;

@@ -23,6 +30,8 @@ struct asic3_platform_data {
 	unsigned int irq_base;
 
 	unsigned int gpio_base;
+
+	struct asic3_led *leds;
 };
 
 #define ASIC3_NUM_GPIO_BANKS	4

@@ -111,9 +120,9 @@ struct asic3_platform_data {
 #define ASIC3_GPIOA11_PWM0		ASIC3_CONFIG_GPIO(11, 1, 1, 0)
 #define ASIC3_GPIOA12_PWM1		ASIC3_CONFIG_GPIO(12, 1, 1, 0)
 #define ASIC3_GPIOA15_CONTROL_CX	ASIC3_CONFIG_GPIO(15, 1, 1, 0)
-#define ASIC3_GPIOC0_LED0		ASIC3_CONFIG_GPIO(32, 1, 1, 0)
-#define ASIC3_GPIOC1_LED1		ASIC3_CONFIG_GPIO(33, 1, 1, 0)
-#define ASIC3_GPIOC2_LED2		ASIC3_CONFIG_GPIO(34, 1, 1, 0)
+#define ASIC3_GPIOC0_LED0		ASIC3_CONFIG_GPIO(32, 1, 0, 0)
+#define ASIC3_GPIOC1_LED1		ASIC3_CONFIG_GPIO(33, 1, 0, 0)
+#define ASIC3_GPIOC2_LED2		ASIC3_CONFIG_GPIO(34, 1, 0, 0)
 #define ASIC3_GPIOC3_SPI_RXD		ASIC3_CONFIG_GPIO(35, 1, 0, 0)
 #define ASIC3_GPIOC4_CF_nCD		ASIC3_CONFIG_GPIO(36, 1, 0, 0)
 #define ASIC3_GPIOC4_SPI_TXD		ASIC3_CONFIG_GPIO(36, 1, 1, 0)

@@ -152,6 +161,7 @@ struct asic3_platform_data {
 #define PWM_TIMEBASE_VALUE(x)    ((x)&0xf)   /* Low 4 bits sets time base */
 #define PWM_TIMEBASE_ENABLE     (1 << 4)   /* Enable clock */
 
+#define ASIC3_NUM_LEDS                  3
 #define ASIC3_LED_0_Base                0x0700
 #define ASIC3_LED_1_Base                0x0800
 #define ASIC3_LED_2_Base 		      0x0900

@@ -293,4 +303,10 @@ struct asic3_platform_data {
 #define ASIC3_MAP_SIZE_32BIT	0x2000
 #define ASIC3_MAP_SIZE_16BIT	0x1000
 
+/* Functions needed by leds-asic3 */
+
+struct asic3;
+extern void asic3_write_register(struct asic3 *asic, unsigned int reg, u32 val);
+extern u32 asic3_read_register(struct asic3 *asic, unsigned int reg);
+
 #endif /* __ASIC3_H__ */
#1 lore raw

Hi Paul,

Quoted 2 lines · Paul Parsons

Add LED support for the HTC ASIC3. Underlying support is provided by the mfd/asic3 and leds/leds-asic3 drivers.

Where is the LED driver ?

drivers/mfd/asic3.c static int asic3_clk_enable(struct asic3 · line 599

Quoted 12 lines
drivers/mfd/asic3.c +1 -3
-static int asic3_clk_enable(struct asic3 *asic, struct asic3_clk *clk)
+static void asic3_clk_enable(struct asic3 *asic, struct asic3_clk *clk)
 {
 	unsigned long flags;
 	u32 cdex;

@@ -596,8 +596,6 @@ static int asic3_clk_enable(struct asic3
 		asic3_write_register(asic, ASIC3_OFFSET(CLOCK, CDEX), cdex);
 	}
 	spin_unlock_irqrestore(&asic->lock, flags);
-
-	return 0;
 }

Although a valid change, this is unrelated to this patch.

drivers/mfd/asic3.c static int __init asic3_probe(struct pla · line 943

Quoted 5 lines
drivers/mfd/asic3.c +1 -0
@@ -883,6 +940,7 @@ static int __init asic3_probe(struct pla
 		goto out_unmap;
 	}
 
+	asic->gpio.label = "asic3";

Ditto.

Cheers,
Samuel.

Signature · 2 lines
Intel Open Source Technology Centre
http://oss.intel.com/
author
#2 lore raw

Hi Samuel,

Quoted 1 line · Samuel Ortiz

Where is the LED driver ?

It was posted separately - [PATCH 2/2] leds: Add ASIC3 LED support - since drivers/mfd and drivers/leds are maintained separately. Sorry, I should have cross copied them.

Quoted 5 lines
Quoted 1 line

-static int asic3_clk_enable(struct asic3 *asic,

struct asic3_clk *clk)

Quoted 1 line

+static void asic3_clk_enable(struct asic3 *asic,

struct asic3_clk *clk)
Although a valid change, this is unrelated to this patch.

Not completely unrelated; I call asic3_clk_enable() and found it simpler to remove the unnecessary return value rather than check it.

Quoted 2 lines
Quoted 1 line

+    asic->gpio.label = "asic3";

Ditto.

Agreed; that was left in by mistake.

Should I resubmit?

Regards,
Paul

#3 lore raw

Hi Paul,

Quoted 6 lines · Paul Parsons

Hi Samuel,

Quoted 1 line · Samuel Ortiz

Where is the LED driver ?

It was posted separately - [PATCH 2/2] leds: Add ASIC3 LED support - since drivers/mfd and drivers/leds are maintained separately. Sorry, I should have cross copied them.

I believe the LED driver is dependent on the asic3.h changes this patches carry. So both patches should go in at the same time, probably through my tree.

Quoted 7 lines
Quoted 5 lines
Quoted 1 line

-static int asic3_clk_enable(struct asic3 *asic,

struct asic3_clk *clk)

Quoted 1 line

+static void asic3_clk_enable(struct asic3 *asic,

struct asic3_clk *clk)
Although a valid change, this is unrelated to this patch.

Not completely unrelated; I call asic3_clk_enable() and found it simpler to remove the unnecessary return value rather than check it.

I understood that. And I am fine with the change, but I would like it to be a separate patch.

Quoted 5 lines
Quoted 2 lines
Quoted 1 line

+    asic->gpio.label = "asic3";

Ditto.

Agreed; that was left in by mistake.

Should I resubmit?

Yes, please. Also, I'm deprecating the mfd_data usage in my mfd-2.6 tree, so you should fix your code according to that.

Thanks in advance.

Cheers,
Samuel.

Signature · 2 lines
Intel Open Source Technology Centre
http://oss.intel.com/
#4 lore raw
In reply to Paul Parsons

Am Samstag, den 30.04.2011, 17:59 +0000 schrieb Paul Parsons:

Quoted 9 lines

Add LED support for the HTC ASIC3. Underlying support is provided by the mfd/asic3 and leds/leds-asic3 drivers. An example configuration is provided by the pxa/hx4700 platform.

Signed-off-by Paul Parsons <[email protected]>

---
diff -uprN clean-2.6.39-rc5/drivers/mfd/asic3.c linux-2.6.39-rc5/drivers/mfd/asic3.c

drivers/mfd/asic3.c +0 -0
@@ -88,19 +88,19 @@ struct asic3 {

drivers/mfd/asic3.c struct asic3 { · line 96

Quoted 10 lines
drivers/mfd/asic3.c +2 -2
 static int asic3_gpio_get(struct gpio_chip *chip, unsigned offset);
 
-static inline void asic3_write_register(struct asic3 *asic,
-				 unsigned int reg, u32 value)
+void asic3_write_register(struct asic3 *asic, unsigned int reg, u32 value)
 {
 	iowrite16(value, asic->mapping +
 		  (reg >> asic->bus_shift));
 }
+EXPORT_SYMBOL_GPL(asic3_write_register);

I'd like to avoid this export. Could you inform the cell driver of mapping and bus shift via io resources as it is done in ds1wm?

drivers/mfd/asic3.c struct asic3 { · line 103

Quoted 8 lines
drivers/mfd/asic3.c +2 -2
-static inline u32 asic3_read_register(struct asic3 *asic,
-			       unsigned int reg)
+u32 asic3_read_register(struct asic3 *asic, unsigned int reg)
 {
 	return ioread16(asic->mapping +
 			(reg >> asic->bus_shift));
 }
+EXPORT_SYMBOL_GPL(asic3_read_register);

Same for this one.

Quoted 12 lines
drivers/mfd/asic3.c +1 -1
 static void asic3_set_register(struct asic3 *asic, u32 reg, u32 bits, bool set)
 {

@@ -584,7 +584,7 @@ static int asic3_gpio_remove(struct plat
 	return gpiochip_remove(&asic->gpio);
 }
 
-static int asic3_clk_enable(struct asic3 *asic, struct asic3_clk *clk)
+static void asic3_clk_enable(struct asic3 *asic, struct asic3_clk *clk)
 {
 	unsigned long flags;
 	u32 cdex;

drivers/mfd/asic3.c static int asic3_clk_enable(struct asic3 · line 599

Quoted 6 lines
drivers/mfd/asic3.c +0 -2
@@ -596,8 +596,6 @@ static int asic3_clk_enable(struct asic3
 		asic3_write_register(asic, ASIC3_OFFSET(CLOCK, CDEX), cdex);
 	}
 	spin_unlock_irqrestore(&asic->lock, flags);
-
-	return 0;
 }

Ok for now, I guess. But it's unrelated to led support. And once we have a common struct clk and can use clk_enable in the led driver, there's going to be error checking anyway...

Quoted 63 lines
drivers/mfd/asic3.c +49 -1
 static void asic3_clk_disable(struct asic3 *asic, struct asic3_clk *clk)

@@ -782,7 +780,55 @@ static struct mfd_cell asic3_cell_mmc = 
 	.resources     = asic3_mmc_resources,
 };
 
+static const int clock_ledn[ASIC3_NUM_LEDS] = {
+	[0] = ASIC3_CLOCK_LED0,
+	[1] = ASIC3_CLOCK_LED1,
+	[2] = ASIC3_CLOCK_LED2,
+};
+
+static int asic3_leds_enable(struct platform_device *pdev)
+{
+	const struct mfd_cell *cell = mfd_get_cell(pdev);
+	struct asic3 *asic = dev_get_drvdata(pdev->dev.parent);
+
+	asic3_clk_enable(asic, &asic->clocks[clock_ledn[cell->id]]);
+
+	return 0;
+}
+
+static int asic3_leds_disable(struct platform_device *pdev)
+{
+	const struct mfd_cell *cell = mfd_get_cell(pdev);
+	struct asic3 *asic = dev_get_drvdata(pdev->dev.parent);
+
+	asic3_clk_disable(asic, &asic->clocks[clock_ledn[cell->id]]);
+
+	return 0;
+}
+
+static struct mfd_cell asic3_cell_leds[ASIC3_NUM_LEDS] = {
+	[0] = {
+		.name          = "leds-asic3",
+		.id            = 0,
+		.enable        = asic3_leds_enable,
+		.disable       = asic3_leds_disable,
+	},
+	[1] = {
+		.name          = "leds-asic3",
+		.id            = 1,
+		.enable        = asic3_leds_enable,
+		.disable       = asic3_leds_disable,
+	},
+	[2] = {
+		.name          = "leds-asic3",
+		.id            = 2,
+		.enable        = asic3_leds_enable,
+		.disable       = asic3_leds_disable,
+	},
+};
+
 static int __init asic3_mfd_probe(struct platform_device *pdev,
+				  struct asic3_platform_data *pdata,
 				  struct resource *mem)
 {
 	struct asic3 *asic = platform_get_drvdata(pdev);

@@ -820,9 +866,20 @@ static int __init asic3_mfd_probe(struct
 	if (ret < 0)
 		goto out;
 
-	if (mem_sdio && (irq >= 0))
+	if (mem_sdio && (irq >= 0)) {

drivers/mfd/asic3.c static int __init asic3_mfd_probe(struct · line 879

Quoted 10 lines
drivers/mfd/asic3.c +8 -0
 		ret = mfd_add_devices(&pdev->dev, pdev->id,
 			&asic3_cell_mmc, 1, mem_sdio, irq);
+		if (ret < 0)
+			goto out;
+	}
+
+	if (pdata->leds) {
+		asic3_cell_leds[0].mfd_data = &pdata->leds[0];
+		asic3_cell_leds[1].mfd_data = &pdata->leds[1];
+		asic3_cell_leds[2].mfd_data = &pdata->leds[2];

As Samuel pointed out, better use .platform_data and .platform_size here right away.

drivers/mfd/asic3.c static int __init asic3_probe(struct pla · line 943

Quoted 11 lines
drivers/mfd/asic3.c +4 -0
+		ret = mfd_add_devices(&pdev->dev, 0,
+			asic3_cell_leds, ASIC3_NUM_LEDS, NULL, 0);
+	}
 
  out:
 	return ret;

@@ -883,6 +940,7 @@ static int __init asic3_probe(struct pla
 		goto out_unmap;
 	}
 
+	asic->gpio.label = "asic3";

Separate patch?

Quoted 39 lines
drivers/mfd/asic3.c +1 -1
 	asic->gpio.base = pdata->gpio_base;
 	asic->gpio.ngpio = ASIC3_NUM_GPIOS;
 	asic->gpio.get = asic3_gpio_get;

@@ -903,7 +961,7 @@ static int __init asic3_probe(struct pla
 	 */
 	memcpy(asic->clocks, asic3_clk_init, sizeof(asic3_clk_init));
 
-	asic3_mfd_probe(pdev, mem);
+	asic3_mfd_probe(pdev, pdata, mem);
 
 	dev_info(asic->dev, "ASIC3 Core driver\n");

diff -uprN clean-2.6.39-rc5/include/linux/mfd/asic3.h linux-2.6.39-rc5/include/linux/mfd/asic3.h

include/linux/mfd/asic3.h +9 -0
@@ -16,6 +16,13 @@
 
 #include <linux/types.h>
 
+struct led_classdev;
+struct asic3_led {
+	const char	*name;
+	const char	*default_trigger;
+	struct led_classdev *cdev;
+};
+
 struct asic3_platform_data {
 	u16 *gpio_config;
 	unsigned int gpio_config_num;

@@ -23,6 +30,8 @@ struct asic3_platform_data {
 	unsigned int irq_base;
 
 	unsigned int gpio_base;
+
+	struct asic3_led *leds;
 };
 
 #define ASIC3_NUM_GPIO_BANKS	4

include/linux/mfd/asic3.h struct asic3_platform_data { · line 125

Quoted 9 lines
include/linux/mfd/asic3.h +3 -3
@@ -111,9 +120,9 @@ struct asic3_platform_data {
 #define ASIC3_GPIOA11_PWM0		ASIC3_CONFIG_GPIO(11, 1, 1, 0)
 #define ASIC3_GPIOA12_PWM1		ASIC3_CONFIG_GPIO(12, 1, 1, 0)
 #define ASIC3_GPIOA15_CONTROL_CX	ASIC3_CONFIG_GPIO(15, 1, 1, 0)
-#define ASIC3_GPIOC0_LED0		ASIC3_CONFIG_GPIO(32, 1, 1, 0)
-#define ASIC3_GPIOC1_LED1		ASIC3_CONFIG_GPIO(33, 1, 1, 0)
-#define ASIC3_GPIOC2_LED2		ASIC3_CONFIG_GPIO(34, 1, 1, 0)
+#define ASIC3_GPIOC0_LED0		ASIC3_CONFIG_GPIO(32, 1, 0, 0)
+#define ASIC3_GPIOC1_LED1		ASIC3_CONFIG_GPIO(33, 1, 0, 0)
+#define ASIC3_GPIOC2_LED2		ASIC3_CONFIG_GPIO(34, 1, 0, 0)

As I said, not convinced about that one. I'll follow up with a patch to test on top of yours.

Quoted 12 lines
include/linux/mfd/asic3.h +1 -0
 #define ASIC3_GPIOC3_SPI_RXD		ASIC3_CONFIG_GPIO(35, 1, 0, 0)
 #define ASIC3_GPIOC4_CF_nCD		ASIC3_CONFIG_GPIO(36, 1, 0, 0)
 #define ASIC3_GPIOC4_SPI_TXD		ASIC3_CONFIG_GPIO(36, 1, 1, 0)

@@ -152,6 +161,7 @@ struct asic3_platform_data {
 #define PWM_TIMEBASE_VALUE(x)    ((x)&0xf)   /* Low 4 bits sets time base */
 #define PWM_TIMEBASE_ENABLE     (1 << 4)   /* Enable clock */
 
+#define ASIC3_NUM_LEDS                  3
 #define ASIC3_LED_0_Base                0x0700
 #define ASIC3_LED_1_Base                0x0800
 #define ASIC3_LED_2_Base 		      0x0900

include/linux/mfd/asic3.h struct asic3_platform_data { · line 312

Quoted 10 lines
include/linux/mfd/asic3.h +6 -0
@@ -293,4 +303,10 @@ struct asic3_platform_data {
 #define ASIC3_MAP_SIZE_32BIT	0x2000
 #define ASIC3_MAP_SIZE_16BIT	0x1000
 
+/* Functions needed by leds-asic3 */
+
+struct asic3;
+extern void asic3_write_register(struct asic3 *asic, unsigned int reg, u32 val);
+extern u32 asic3_read_register(struct asic3 *asic, unsigned int reg);
+
 #endif /* __ASIC3_H__ */

regards
Philipp

#5 lore raw

Hi Paul,

This patch reverts your ASIC3_GPIOC?_LED? changes to keep the gpios as outputs and informs the led driver via struct asic3_led of the gpio to toggle in the brightness_set and blink_set callbacks.

Does this work for you?

regards
Philipp

---
 drivers/leds/leds-asic3.c |   19 +++++++++++++++++++
 drivers/mfd/asic3.c       |    3 +++
 include/linux/mfd/asic3.h |    8 +++++---
 3 files changed, 27 insertions(+), 3 deletions(-)
drivers/leds/leds-asic3.c +19 -0
index abda364..62b35dc 100644
@@ -9,6 +9,7 @@
 #include <linux/kernel.h>
 #include <linux/init.h>
 #include <linux/platform_device.h>
+#include <linux/gpio.h>
 #include <linux/leds.h>
 #include <linux/slab.h>
 

@@ -41,10 +42,14 @@ static void brightness_set(struct led_classdev *cdev,
 {
 	struct platform_device *pdev = to_platform_device(cdev->dev->parent);
 	const struct mfd_cell *cell = mfd_get_cell(pdev);
+	struct asic3_led *led = mfd_get_data(pdev);
 	struct asic3 *asic = dev_get_drvdata(pdev->dev.parent);
 	u32 timebase;
 	unsigned int base;
 
+	if (value != LED_OFF)
+		gpio_set_value(led->gpio, 1);
+
 	timebase = (value == LED_OFF) ? 0 : (LED_EN|0x4);
 
 	base = led_n_base[cell->id];

@@ -52,6 +57,9 @@ static void brightness_set(struct led_classdev *cdev,
 	asic3_write_register(asic, (base + ASIC3_LED_DutyTime), 32);
 	asic3_write_register(asic, (base + ASIC3_LED_AutoStopCount), 0);
 	asic3_write_register(asic, (base + ASIC3_LED_TimeBase), timebase);
+
+	if (value == LED_OFF)
+		gpio_set_value(led->gpio, 0);
 }
 
 static int blink_set(struct led_classdev *cdev,

@@ -60,6 +68,7 @@ static int blink_set(struct led_classdev *cdev,
 {
 	struct platform_device *pdev = to_platform_device(cdev->dev->parent);
 	const struct mfd_cell *cell = mfd_get_cell(pdev);
+	struct asic3_led *led = mfd_get_data(pdev);
 	struct asic3 *asic = dev_get_drvdata(pdev->dev.parent);
 	u32 on;
 	u32 off;

@@ -79,6 +88,8 @@ static int blink_set(struct led_classdev *cdev,
 			return -EINVAL;
 	}
 
+	gpio_set_value(led->gpio, 1);
+
 	base = led_n_base[cell->id];
 	asic3_write_register(asic, (base + ASIC3_LED_PeriodTime), (on + off));
 	asic3_write_register(asic, (base + ASIC3_LED_DutyTime), on);

@@ -96,6 +107,10 @@ static int __devinit asic3_led_probe(struct platform_device *pdev)
 	struct asic3_led *led = mfd_get_data(pdev);
 	int ret;
 
+	ret = gpio_request(led->gpio, "leds-asic3");
+	if (ret < 0)
+		goto ret_gpio;
+
 	ret = mfd_cell_enable(pdev);
 	if (ret < 0)
 		goto ret0;

@@ -122,6 +137,8 @@ ret2:
 ret1:
 	(void) mfd_cell_disable(pdev);
 ret0:
+	gpio_free(led->gpio);
+ret_gpio:
 	return ret;
 }
 

@@ -135,6 +152,8 @@ static int __devexit asic3_led_remove(struct platform_device *pdev)
 
 	(void) mfd_cell_disable(pdev);
 
+	gpio_free(led->gpio);
+
 	return 0;
 }
drivers/mfd/asic3.c +3 -0
index 77c10b7..5e6758d 100644
@@ -889,6 +889,9 @@ static int __init asic3_mfd_probe(struct platform_device *pdev,
 	}
 
 	if (pdata->leds) {
+		pdata->leds[0].gpio = asic->gpio.base + 32; /* ASIC3_GPIOC0_LED0 */
+		pdata->leds[1].gpio = asic->gpio.base + 33; /* ASIC3_GPIOC1_LED1 */
+		pdata->leds[2].gpio = asic->gpio.base + 34; /* ASIC3_GPIOC2_LED2 */
 		asic3_cell_leds[0].mfd_data = &pdata->leds[0];
 		asic3_cell_leds[1].mfd_data = &pdata->leds[1];
 		asic3_cell_leds[2].mfd_data = &pdata->leds[2];
include/linux/mfd/asic3.h +5 -3
index d0dd3eb..f1944ef 100644
@@ -21,6 +21,8 @@ struct asic3_led {
 	const char	*name;
 	const char	*default_trigger;
 	struct led_classdev *cdev;
+
+	int		gpio; /* set by the asic3 mfd driver itself */
 };
 
 struct asic3_platform_data {

@@ -120,9 +122,9 @@ struct asic3_platform_data {
 #define ASIC3_GPIOA11_PWM0		ASIC3_CONFIG_GPIO(11, 1, 1, 0)
 #define ASIC3_GPIOA12_PWM1		ASIC3_CONFIG_GPIO(12, 1, 1, 0)
 #define ASIC3_GPIOA15_CONTROL_CX	ASIC3_CONFIG_GPIO(15, 1, 1, 0)
-#define ASIC3_GPIOC0_LED0		ASIC3_CONFIG_GPIO(32, 1, 0, 0)
-#define ASIC3_GPIOC1_LED1		ASIC3_CONFIG_GPIO(33, 1, 0, 0)
-#define ASIC3_GPIOC2_LED2		ASIC3_CONFIG_GPIO(34, 1, 0, 0)
+#define ASIC3_GPIOC0_LED0		ASIC3_CONFIG_GPIO(32, 1, 1, 0)
+#define ASIC3_GPIOC1_LED1		ASIC3_CONFIG_GPIO(33, 1, 1, 0)
+#define ASIC3_GPIOC2_LED2		ASIC3_CONFIG_GPIO(34, 1, 1, 0)
 #define ASIC3_GPIOC3_SPI_RXD		ASIC3_CONFIG_GPIO(35, 1, 0, 0)
 #define ASIC3_GPIOC4_CF_nCD		ASIC3_CONFIG_GPIO(36, 1, 0, 0)
 #define ASIC3_GPIOC4_SPI_TXD		ASIC3_CONFIG_GPIO(36, 1, 1, 0)
Signature · 1 line
1.7.4.4
author
#6 lore raw

Hi Philipp,

Quoted 7 lines · Philipp Zabel

This patch reverts your ASIC3_GPIOC?_LED? changes to keep the gpios as
outputs and informs the led driver via struct asic3_led of the gpio to
toggle in the brightness_set and blink_set callbacks.

Does this work for you?

Well, it works in the sense that the LEDs still operate as before. In other words, calling gpio_set_value() has made no difference to the driver's behaviour.

So I'm confused as to what the patch is intended to demonstrate. Surely we want to use the LED GPIOs instead of the TimeBase enable bit, not in addition to it.

As a quick experiment, I modified the driver again to clear the TimeBase enable bit:

leds/leds-asic3.c renamed from leds-asic3.c +2 -2
@@ -50,7 +50,7 @@ static void brightness_set(struct led_cl
 	if (value != LED_OFF)
 		gpio_set_value(led->gpio, 1);
 
-	timebase = (value == LED_OFF) ? 0 : (LED_EN|0x4);
+	timebase = (value == LED_OFF) ? 0 : (0|0x4);
 
 	base = led_n_base[cell->id];
 	asic3_write_register(asic, (base + ASIC3_LED_PeriodTime), 32);

@@ -94,7 +94,7 @@ static int blink_set(struct led_classdev
 	asic3_write_register(asic, (base + ASIC3_LED_PeriodTime), (on + off));
 	asic3_write_register(asic, (base + ASIC3_LED_DutyTime), on);
 	asic3_write_register(asic, (base + ASIC3_LED_AutoStopCount), 0);
-	asic3_write_register(asic, (base + ASIC3_LED_TimeBase), (LED_EN|0x4));
+	asic3_write_register(asic, (base + ASIC3_LED_TimeBase), (0|0x4));
 
 	*delay_on = CLK_TO_MS(on);
 	*delay_off = CLK_TO_MS(off);

This was to check whether the LED GPIOs performed the same function as the TimeBase enable bit, which would be a useful result. But they didn't; the LEDs stopped working.

So I must conclude that there is still no evidence that the LED GPIOs work as outputs. They do work as inputs however, per my previous observations.

Maybe there exists an ASIC3 control register which can change the direction of the LED GPIOs from inputs to outputs?

Regards,
Paul

author
#7 lore raw
In reply to Philipp Zabel

Hi Philipp,

Quoted 5 lines · Philipp Zabel

I'd like to avoid this export. Could you inform the cell driver of
mapping and bus shift via io resources as it is done in ds1wm?
Same for this one.

The LED registers are already mapped by the mfd driver, and the functions to access them are already provided. My feeling is that replicating all of that seems a high price to pay to avoid exporting a couple of functions.

Quoted 5 lines

Ok for now, I guess. But it's unrelated to led support. And once we have
a common struct clk and can use clk_enable in the led driver, there's
going to be error checking anyway...

Yes, that's a separate patch if at all.

Quoted 3 lines

As Samuel pointed out, better use .platform_data and .platform_size here
right away.

Will do.

Quoted 1 line

Separate patch?

Separate patch.

Quoted 3 lines

As I said, not convinced about that one. I'll follow up with a patch to
test on top of yours.

OK.

Regards,
Paul