Patchwork [v2] pata-generic/of: Make probing via device tree non-powerpc-specific

login
register
mail settings
Submitter Dave Martin
Date Sept. 16, 2011, 4:22 p.m.
Message ID <1316190120-17010-1-git-send-email-dave.martin@linaro.org>
Download mbox | patch
Permalink /patch/114980/
State Not Applicable
Delegated to: David Miller
Headers show

Comments

Dave Martin - Sept. 16, 2011, 4:22 p.m.
This patch enables device-tree-based probing of the pata-generic
platform driver across all architectures:

  * make the pata_of_generic module depend on OF instead of PPC_OF;
  * supply some missing inclues;
  * replace endianness-sensitive raw access to device tree data
    with of_property_read_u32() calls.

Signed-off-by: Dave Martin <dave.martin@linaro.org>
---
v2: correct sense of the check of_property_read_u32(dn, "pio-mode",
&pio_mode).  Somehow I posted an old version of this patch, depite
having already fixed this...

Tested on ARM Versatile Express, with my soon-to-be-posted device
tree support patches.

I'm not in a position to build/test this for powerpc easily --
if anyone is able to do that, it would be appreciated.

Grant, does this require similar cleanup to the isp1760 USB hcd driver?

 drivers/ata/Kconfig            |    2 +-
 drivers/ata/pata_of_platform.c |   16 +++++++---------
 2 files changed, 8 insertions(+), 10 deletions(-)
Rob Herring - Sept. 16, 2011, 9:34 p.m.
Dave,

On 09/16/2011 11:22 AM, Dave Martin wrote:
> This patch enables device-tree-based probing of the pata-generic
> platform driver across all architectures:
> 
>   * make the pata_of_generic module depend on OF instead of PPC_OF;
>   * supply some missing inclues;
>   * replace endianness-sensitive raw access to device tree data
>     with of_property_read_u32() calls.
> 
> Signed-off-by: Dave Martin <dave.martin@linaro.org>
> ---
> v2: correct sense of the check of_property_read_u32(dn, "pio-mode",
> &pio_mode).  Somehow I posted an old version of this patch, depite
> having already fixed this...
> 
> Tested on ARM Versatile Express, with my soon-to-be-posted device
> tree support patches.
> 
> I'm not in a position to build/test this for powerpc easily --
> if anyone is able to do that, it would be appreciated.
> 
Building just requires getting Codesourcery PPC toolchain...

You also have to be aware that you are enabling this for all OF-enabled
arches which could break with an allyesconfig. So really you need sparc,
x86, mips, and microblaze, but a subset is probably sufficient.

> Grant, does this require similar cleanup to the isp1760 USB hcd driver?
> 
Yes. It really should be merged with pata_platform.c. If you're willing
to combine them, I'll do ppc and sparc builds as I already have those on
my system.

Rob

>  drivers/ata/Kconfig            |    2 +-
>  drivers/ata/pata_of_platform.c |   16 +++++++---------
>  2 files changed, 8 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/ata/Kconfig b/drivers/ata/Kconfig
> index 5987e0b..c6ef9d0 100644
> --- a/drivers/ata/Kconfig
> +++ b/drivers/ata/Kconfig
> @@ -820,7 +820,7 @@ config PATA_PLATFORM
>  
>  config PATA_OF_PLATFORM
>  	tristate "OpenFirmware platform device PATA support"
> -	depends on PATA_PLATFORM && PPC_OF
> +	depends on PATA_PLATFORM && OF
>  	help
>  	  This option enables support for generic directly connected ATA
>  	  devices commonly found on embedded systems with OpenFirmware
> diff --git a/drivers/ata/pata_of_platform.c b/drivers/ata/pata_of_platform.c
> index f305400..e6e9aa9 100644
> --- a/drivers/ata/pata_of_platform.c
> +++ b/drivers/ata/pata_of_platform.c
> @@ -11,6 +11,9 @@
>  
>  #include <linux/kernel.h>
>  #include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/of_irq.h>
>  #include <linux/of_platform.h>
>  #include <linux/ata_platform.h>
>  
> @@ -21,10 +24,9 @@ static int __devinit pata_of_platform_probe(struct platform_device *ofdev)
>  	struct resource io_res;
>  	struct resource ctl_res;
>  	struct resource irq_res;
> -	unsigned int reg_shift = 0;
> -	int pio_mode = 0;
> +	u32 reg_shift = 0;
> +	u32 pio_mode = 0;
>  	int pio_mask;
> -	const u32 *prop;
>  
>  	ret = of_address_to_resource(dn, 0, &io_res);
>  	if (ret) {
> @@ -55,13 +57,9 @@ static int __devinit pata_of_platform_probe(struct platform_device *ofdev)
>  	else
>  		irq_res.flags = 0;
>  
> -	prop = of_get_property(dn, "reg-shift", NULL);
> -	if (prop)
> -		reg_shift = *prop;
> +	of_property_read_u32(dn, "reg-shift", &reg_shift);
>  
> -	prop = of_get_property(dn, "pio-mode", NULL);
> -	if (prop) {
> -		pio_mode = *prop;
> +	if (!of_property_read_u32(dn, "pio-mode", &pio_mode)) {
>  		if (pio_mode > 6) {
>  			dev_err(&ofdev->dev, "invalid pio-mode\n");
>  			return -EINVAL;

--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Grant Likely - Sept. 17, 2011, 3:37 p.m.
On Fri, Sep 16, 2011 at 05:22:00PM +0100, Dave Martin wrote:
> This patch enables device-tree-based probing of the pata-generic
> platform driver across all architectures:
> 
>   * make the pata_of_generic module depend on OF instead of PPC_OF;
>   * supply some missing inclues;
>   * replace endianness-sensitive raw access to device tree data
>     with of_property_read_u32() calls.
> 
> Signed-off-by: Dave Martin <dave.martin@linaro.org>
> ---
> v2: correct sense of the check of_property_read_u32(dn, "pio-mode",
> &pio_mode).  Somehow I posted an old version of this patch, depite
> having already fixed this...
> 
> Tested on ARM Versatile Express, with my soon-to-be-posted device
> tree support patches.
> 
> I'm not in a position to build/test this for powerpc easily --
> if anyone is able to do that, it would be appreciated.

What driver is normally used for versatile express pata?  This driver
is kind of legacy in that it was created when there was a split
between platform_device and of_platform_devices.  But that split was a
bad idea and the same driver should be used regardless of whether or
not DT is enabled.  pata_of_platform.c really should be removed.

So, instead of removing the PPC_OF restriction from
pata_of_platform.c, you should look at adding DT support to the
existing binding.  Bonus points if you can roll pata_of_platform.c
support into pata_platform.c (assuming that is the correct driver).

g.

--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Arnd Bergmann - Sept. 17, 2011, 6:40 p.m.
On Saturday 17 September 2011 09:37:32 Grant Likely wrote:
> What driver is normally used for versatile express pata?  This driver
> is kind of legacy in that it was created when there was a split
> between platform_device and of_platform_devices.  But that split was a
> bad idea and the same driver should be used regardless of whether or
> not DT is enabled.  pata_of_platform.c really should be removed.

It normally uses the plain pata_platform.c driver.

Note that the pata_of_platform driver is already just a shim on
top of the regular pata_platform driver. They could easily be combined,
but the current state is also ok, since there is very little code
duplication.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Grant Likely - Sept. 17, 2011, 9:30 p.m.
On Sat, Sep 17, 2011 at 12:40 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Saturday 17 September 2011 09:37:32 Grant Likely wrote:
>> What driver is normally used for versatile express pata?  This driver
>> is kind of legacy in that it was created when there was a split
>> between platform_device and of_platform_devices.  But that split was a
>> bad idea and the same driver should be used regardless of whether or
>> not DT is enabled.  pata_of_platform.c really should be removed.
>
> It normally uses the plain pata_platform.c driver.
>
> Note that the pata_of_platform driver is already just a shim on
> top of the regular pata_platform driver. They could easily be combined,
> but the current state is also ok, since there is very little code
> duplication.

A bunch of the code is actually redundant since the resource table is
no populated for DT devices.  I also see some directly references to
reg-shift and pio-mode property values without using be32_to_cpu(), so
that will also need to be fixed.  The of_irq_to_resource() and
of_address_to_resource() calls are now redundant since
platform_get_*() works for DT sourced platform device (with one quirk
for the electra-ide device).  Given that the conversion is straight
forward, I'd rather see pata_of_platform.c dropped and rolled into
pata_platform.c.  I've hacked together a patch to do so, but I've only
compile tested it.  Dave, if I send it to you, can you take care of
testing it?

Thanks,
g.
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Dave Martin - Sept. 19, 2011, 10:10 a.m.
On Fri, Sep 16, 2011 at 10:34 PM, Rob Herring <robherring2@gmail.com> wrote:
> Dave,
>
> On 09/16/2011 11:22 AM, Dave Martin wrote:
>> This patch enables device-tree-based probing of the pata-generic
>> platform driver across all architectures:
>>
>>   * make the pata_of_generic module depend on OF instead of PPC_OF;
>>   * supply some missing inclues;
>>   * replace endianness-sensitive raw access to device tree data
>>     with of_property_read_u32() calls.
>>
>> Signed-off-by: Dave Martin <dave.martin@linaro.org>
>> ---
>> v2: correct sense of the check of_property_read_u32(dn, "pio-mode",
>> &pio_mode).  Somehow I posted an old version of this patch, depite
>> having already fixed this...
>>
>> Tested on ARM Versatile Express, with my soon-to-be-posted device
>> tree support patches.
>>
>> I'm not in a position to build/test this for powerpc easily --
>> if anyone is able to do that, it would be appreciated.
>>
> Building just requires getting Codesourcery PPC toolchain...

True, but although I can try to build it, I don't have a PPC platform
to do the actual testing.

> You also have to be aware that you are enabling this for all OF-enabled
> arches which could break with an allyesconfig. So really you need sparc,
> x86, mips, and microblaze, but a subset is probably sufficient.

Fair point.

Since this has now been superseded by Grant's patch anyway, I'll leave
it for others to judge.

Cheers
---Dave
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jeff Garzik - Sept. 20, 2011, 7:30 p.m.
On 09/19/2011 06:10 AM, Dave Martin wrote:
> Since this has now been superseded by Grant's patch anyway, I'll leave
> it for others to judge.


Grant's patch is certainly preferable, all other things being equal...

	Jeff



--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/ata/Kconfig b/drivers/ata/Kconfig
index 5987e0b..c6ef9d0 100644
--- a/drivers/ata/Kconfig
+++ b/drivers/ata/Kconfig
@@ -820,7 +820,7 @@  config PATA_PLATFORM
 
 config PATA_OF_PLATFORM
 	tristate "OpenFirmware platform device PATA support"
-	depends on PATA_PLATFORM && PPC_OF
+	depends on PATA_PLATFORM && OF
 	help
 	  This option enables support for generic directly connected ATA
 	  devices commonly found on embedded systems with OpenFirmware
diff --git a/drivers/ata/pata_of_platform.c b/drivers/ata/pata_of_platform.c
index f305400..e6e9aa9 100644
--- a/drivers/ata/pata_of_platform.c
+++ b/drivers/ata/pata_of_platform.c
@@ -11,6 +11,9 @@ 
 
 #include <linux/kernel.h>
 #include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/of_irq.h>
 #include <linux/of_platform.h>
 #include <linux/ata_platform.h>
 
@@ -21,10 +24,9 @@  static int __devinit pata_of_platform_probe(struct platform_device *ofdev)
 	struct resource io_res;
 	struct resource ctl_res;
 	struct resource irq_res;
-	unsigned int reg_shift = 0;
-	int pio_mode = 0;
+	u32 reg_shift = 0;
+	u32 pio_mode = 0;
 	int pio_mask;
-	const u32 *prop;
 
 	ret = of_address_to_resource(dn, 0, &io_res);
 	if (ret) {
@@ -55,13 +57,9 @@  static int __devinit pata_of_platform_probe(struct platform_device *ofdev)
 	else
 		irq_res.flags = 0;
 
-	prop = of_get_property(dn, "reg-shift", NULL);
-	if (prop)
-		reg_shift = *prop;
+	of_property_read_u32(dn, "reg-shift", &reg_shift);
 
-	prop = of_get_property(dn, "pio-mode", NULL);
-	if (prop) {
-		pio_mode = *prop;
+	if (!of_property_read_u32(dn, "pio-mode", &pio_mode)) {
 		if (pio_mode > 6) {
 			dev_err(&ofdev->dev, "invalid pio-mode\n");
 			return -EINVAL;