Re: [PATCH] of_mmc_spi: add card detect irq support

[Esben Haabendal <esbenhaabendal@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 2232 bytes --]

Hi

Comments below, and updated patch attached.

On Mon, Aug 30, 2010 at 3:29 PM, Anton Vorontsov <cbouatmailru@gmail.com> wrote:
>>> +static int of_mmc_spi_init(struct device *dev,
>> +                          irqreturn_t (*irqhandler)(int, void *), void *mmc)
>> +{
>> +       struct of_mmc_spi *oms = to_of_mmc_spi(dev);
>
> Please add an empty line here.

Ok.

>> +       return request_threaded_irq(
>> +               oms->detect_irq, NULL, irqhandler, 0, dev_name(dev), mmc);
>
> I'd write it this way:
>
> return request_threaded_irq(oms->detect_irq, NULL, irqhandler,
>                            0, dev_name(dev), mmc);
>
> But that's a matter of taste.

Fine with me.

>> +}
>> +
>> +static void of_mmc_spi_exit(struct device *dev, void *mmc)
>> +{
>> +       struct of_mmc_spi *oms = to_of_mmc_spi(dev);
>
> Empty line.

Ok.

>> +       free_irq(oms->detect_irq, mmc);
>> +}
>> +
>>  struct mmc_spi_platform_data *mmc_spi_get_pdata(struct spi_device *spi)
>>  {
>>         struct device *dev = &spi->dev;
>> @@ -121,8 +136,14 @@ struct mmc_spi_platform_data
>> *mmc_spi_get_pdata(struct spi_device *spi)
>>         if (gpio_is_valid(oms->gpios[WP_GPIO]))
>>                 oms->pdata.get_ro = of_mmc_spi_get_ro;
>>
>> -       /* We don't support interrupts yet, let's poll. */
>> -       oms->pdata.caps |= MMC_CAP_NEEDS_POLL;
>> +       oms->detect_irq = irq_of_parse_and_map(np, 0);
>> +       if (oms->detect_irq != NO_IRQ) {
>
> I'd write "if (oms->detect_irq)", which is a bit more natural
> (and still correct, 0 is the only invalid VIRQ number).

Most other architectures has NO_IRQ defined to -1, so I will stick
with the NO_IRQ comparsion.
Hopefully, arm users will soon enjoy this driver/wrapper soon also.

>> +               oms->pdata.init = of_mmc_spi_init;
>> +               oms->pdata.exit = of_mmc_spi_exit;
>> +       }
>> +       else {
>
> } else {

Done.

> Plus, please add an appropriate interrupts = <> bindings into
> Documentation/powerpc/dts-bindings/mmc-spi-slot.txt.

Done.

> And on the next resend, be sure to add Andrew Morton
> <akpm@linux-foundation.org>, David Brownell
> <dbrownell@users.sourceforge.net>, and linux-mmc@vger.kernel.org
> the Cc list.

Should be there now.

/Esben

[-- Attachment #2: 0001-of_mmc_spi-add-card-detect-irq-support.patch --]
[-- Type: text/x-patch, Size: 2869 bytes --]

of_mmc_spi: add card detect irq support

Signed-off-by: Esben Haabendal <eha@doredevelopment.dk>
---
 .../powerpc/dts-bindings/mmc-spi-slot.txt          |    9 ++++++-
 drivers/mmc/host/of_mmc_spi.c                      |   26 ++++++++++++++++++-
 2 files changed, 32 insertions(+), 3 deletions(-)

diff --git a/Documentation/powerpc/dts-bindings/mmc-spi-slot.txt b/Documentation/powerpc/dts-bindings/mmc-spi-slot.txt
index c39ac28..89a0084 100644
--- a/Documentation/powerpc/dts-bindings/mmc-spi-slot.txt
+++ b/Documentation/powerpc/dts-bindings/mmc-spi-slot.txt
@@ -7,8 +7,13 @@ Required properties:
 - voltage-ranges : two cells are required, first cell specifies minimum
   slot voltage (mV), second cell specifies maximum slot voltage (mV).
   Several ranges could be specified.
-- gpios : (optional) may specify GPIOs in this order: Card-Detect GPIO,
+
+Optional properties:
+- gpios : may specify GPIOs in this order: Card-Detect GPIO,
   Write-Protect GPIO.
+- interrupts : the interrupt of a card detect interrupt.
+- interrupt-parent : the phandle for the interrupt controller that
+  services interrupts for this device.
 
 Example:
 
@@ -20,4 +25,6 @@ Example:
 			 &qe_pio_d 15 0>;
 		voltage-ranges = <3300 3300>;
 		spi-max-frequency = <50000000>;
+		interrupts = <42>;
+		interrupt-parent = <&PIC>;
 	};
diff --git a/drivers/mmc/host/of_mmc_spi.c b/drivers/mmc/host/of_mmc_spi.c
index 1247e5d..5530def 100644
--- a/drivers/mmc/host/of_mmc_spi.c
+++ b/drivers/mmc/host/of_mmc_spi.c
@@ -34,6 +34,7 @@ enum {
 struct of_mmc_spi {
 	int gpios[NUM_GPIOS];
 	bool alow_gpios[NUM_GPIOS];
+	int detect_irq;
 	struct mmc_spi_platform_data pdata;
 };
 
@@ -61,6 +62,22 @@ static int of_mmc_spi_get_ro(struct device *dev)
 	return of_mmc_spi_read_gpio(dev, WP_GPIO);
 }
 
+static int of_mmc_spi_init(struct device *dev,
+			   irqreturn_t (*irqhandler)(int, void *), void *mmc)
+{
+	struct of_mmc_spi *oms = to_of_mmc_spi(dev);
+
+	return request_threaded_irq(oms->detect_irq, NULL, irqhandler, 0,
+				    dev_name(dev), mmc);
+}
+
+static void of_mmc_spi_exit(struct device *dev, void *mmc)
+{
+	struct of_mmc_spi *oms = to_of_mmc_spi(dev);
+
+	free_irq(oms->detect_irq, mmc);
+}
+
 struct mmc_spi_platform_data *mmc_spi_get_pdata(struct spi_device *spi)
 {
 	struct device *dev = &spi->dev;
@@ -121,8 +138,13 @@ struct mmc_spi_platform_data *mmc_spi_get_pdata(struct spi_device *spi)
 	if (gpio_is_valid(oms->gpios[WP_GPIO]))
 		oms->pdata.get_ro = of_mmc_spi_get_ro;
 
-	/* We don't support interrupts yet, let's poll. */
-	oms->pdata.caps |= MMC_CAP_NEEDS_POLL;
+	oms->detect_irq = irq_of_parse_and_map(np, 0);
+	if (oms->detect_irq != NO_IRQ) {
+		oms->pdata.init = of_mmc_spi_init;
+		oms->pdata.exit = of_mmc_spi_exit;
+	} else {
+		oms->pdata.caps |= MMC_CAP_NEEDS_POLL;
+	}
 
 	dev->platform_data = &oms->pdata;
 	return dev->platform_data;
-- 
1.7.1.1