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

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

[Esben Haabendal]

[-- 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

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

[Anton Vorontsov]

> of_mmc_spi: add card detect irq support
> 
> Signed-off-by: Esben Haabendal <eha@doredevelopment.dk>

Acked-by: Anton Vorontsov <cbouatmailru@gmail.com>

Thanks!

> ---
>  .../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


-- 
Anton Vorontsov
email: cbouatmailru@gmail.com
irc://irc.freenode.net/bd2

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

[David Brownell]

Since I don't do OpenFirmware, let's hear from
Grant on this one.

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

[Grant Likely]

On Mon, Aug 30, 2010 at 10:38 AM, David Brownell <david-b@pacbell.net> wrote:
> Since I don't do OpenFirmware, let's hear from
> Grant on this one.

Looks good to me.

Acked-by: Grant Likely <grant.likely@secretlab.ca>

>
>
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/linuxppc-dev
>



-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

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

[Anton Vorontsov]

On Mon, Aug 30, 2010 at 11:49:08AM -0600, Grant Likely wrote:
> On Mon, Aug 30, 2010 at 10:38 AM, David Brownell <david-b@pacbell.net> wrote:
> > Since I don't do OpenFirmware, let's hear from
> > Grant on this one.
> 
> Looks good to me.
> 
> Acked-by: Grant Likely <grant.likely@secretlab.ca>

I wonder what happened with this patch?

Thanks,

-- 
Anton Vorontsov
Email: cbouatmailru@gmail.com

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

[Grant Likely]

On Tue, Dec 28, 2010 at 07:05:26PM +0300, Anton Vorontsov wrote:
> On Mon, Aug 30, 2010 at 11:49:08AM -0600, Grant Likely wrote:
> > On Mon, Aug 30, 2010 at 10:38 AM, David Brownell <david-b@pacbell.net> wrote:
> > > Since I don't do OpenFirmware, let's hear from
> > > Grant on this one.
> > 
> > Looks good to me.
> > 
> > Acked-by: Grant Likely <grant.likely@secretlab.ca>
> 
> I wonder what happened with this patch?

Merged, thanks.  :-)

g.

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

[Grant Likely]

On Mon, Aug 30, 2010 at 10:04 AM, Esben Haabendal
<esbenhaabendal@gmail.com> wrote:
> On Mon, Aug 30, 2010 at 3:29 PM, Anton Vorontsov <cbouatmailru@gmail.com> wrote:
>>> @@ -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.

Not true.  NO_IRQ is only defined as -1 on ARM, microblaze, mn10300
and parisc, and I've got a patch pending to remove microblaze from
that list.  ARM just happens to be a really big user.

$ git grep NO_IRQ arch/*/include  (I've trimmed the irrelevant matches)
arch/arm/include/asm/irq.h:#ifndef NO_IRQ
arch/arm/include/asm/irq.h:#define NO_IRQ       ((unsigned int)(-1))
arch/microblaze/include/asm/irq.h:#define NO_IRQ (-1)
arch/mn10300/include/asm/irq.h:#define NO_IRQ           INT_MAX
arch/parisc/include/asm/irq.h:#define NO_IRQ            (-1)
arch/powerpc/include/asm/irq.h:#define NO_IRQ                   (0)

> Hopefully, arm users will soon enjoy this driver/wrapper soon also.

My hope is that even on ARM when the device tree is used I'll be able
eliminate IRQs mapped to 0 (but I need to do a lot more research on
how best to do that though).  Are you actually using this on ARM?

I'm okay with you keeping the NO_IRQ test for the short term though.

g.

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

[Esben Haabendal]

On Mon, Aug 30, 2010 at 7:46 PM, Grant Likely <grant.likely@secretlab.ca> wrote:
> On Mon, Aug 30, 2010 at 10:04 AM, Esben Haabendal
>> Hopefully, arm users will soon enjoy this driver/wrapper soon also.
>
> My hope is that even on ARM when the device tree is used I'll be able
> eliminate IRQs mapped to 0 (but I need to do a lot more research on
> how best to do that though).  Are you actually using this on ARM?

No.

> I'm okay with you keeping the NO_IRQ test for the short term though.

Ok.  I personally don't see a big problem with dropping it either, as
I don't actually
use it on ARM. But until at least ARM has NO_IRQ==0, I just thought it would be
the right thing to try to be compatible.

/Esben