[PATCH v3 00/10] ata: pata_platform: Refurbish the driver

[PATCH v3 00/10] ata: pata_platform: Refurbish the driver

[Lad Prabhakar]

Hi All,

This patch series aims to merge pata_of_platform into pata_platform
driver.

Cheers,
Prabhakar

Changes for v3:
* Split up the patches furthermore.

Changes for v2:
* Dropped check for IRQ0
* Dropped setting the irqflags as suggested by Rob
* Fixed freeing up irq_res when not present in DT
* Dropped PATA_OF_PLATFORM entry
* Split up sorting of headers in separate patch
* Dropped sht from struct pata_platform_priv
* Used GENMASK() to calculate mask

Lad Prabhakar (10):
  ata: pata_platform: Make use of platform_get_mem_or_io()
  ata: pata_platform: Drop use of unlikely() in pata_platform_probe
  ata: pata_of_platform: Use platform_get_irq_optional() to get the
    interrupt
  ata: pata_platform: Use platform_get_irq_optional() to get the
    interrupt
  ata: pata_platform: Drop check for invalid IRQ number
  ata: pata_of_platform: Make use of platform_get_mem_or_io()
  ata: pata_platform: Merge pata_of_platform into pata_platform
  ata: pata_platform: Drop validating num_resources count
  ata: pata_platform: Sort the #includes alphabetically
  ata: pata_platform: Make use of GENMASK() macro

 drivers/ata/Kconfig            |  10 --
 drivers/ata/Makefile           |   1 -
 drivers/ata/pata_of_platform.c |  90 --------------
 drivers/ata/pata_platform.c    | 208 ++++++++++++++++++++++-----------
 include/linux/ata_platform.h   |   9 --
 5 files changed, 142 insertions(+), 176 deletions(-)
 delete mode 100644 drivers/ata/pata_of_platform.c

-- 
2.17.1

[PATCH v3 01/10] ata: pata_platform: Make use of platform_get_mem_or_io()

[Lad Prabhakar]

Make use of platform_get_mem_or_io() to simplify the code.

Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
Reviewed-by: Sergey Shtylyov <s.shtylyov@omp.ru>
---
 drivers/ata/pata_platform.c | 18 ++++++------------
 1 file changed, 6 insertions(+), 12 deletions(-)

diff --git a/drivers/ata/pata_platform.c b/drivers/ata/pata_platform.c
index 028329428b75..cb3134bf88eb 100644
--- a/drivers/ata/pata_platform.c
+++ b/drivers/ata/pata_platform.c
@@ -198,22 +198,16 @@ static int pata_platform_probe(struct platform_device *pdev)
 	/*
 	 * Get the I/O base first
 	 */
-	io_res = platform_get_resource(pdev, IORESOURCE_IO, 0);
-	if (io_res == NULL) {
-		io_res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-		if (unlikely(io_res == NULL))
-			return -EINVAL;
-	}
+	io_res = platform_get_mem_or_io(pdev, 0);
+	if (unlikely(!io_res))
+		return -EINVAL;
 
 	/*
 	 * Then the CTL base
 	 */
-	ctl_res = platform_get_resource(pdev, IORESOURCE_IO, 1);
-	if (ctl_res == NULL) {
-		ctl_res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
-		if (unlikely(ctl_res == NULL))
-			return -EINVAL;
-	}
+	ctl_res = platform_get_mem_or_io(pdev, 1);
+	if (unlikely(!ctl_res))
+		return -EINVAL;
 
 	/*
 	 * And the IRQ
-- 
2.17.1

[PATCH v3 02/10] ata: pata_platform: Drop use of unlikely() in pata_platform_probe

[Lad Prabhakar]

pata_platform_probe() isn't a hotpath, which makes it's questionable to
use unlikely(). Therefore let's simply drop it.

Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
v2-->v3
* New patch
---
 drivers/ata/pata_platform.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/ata/pata_platform.c b/drivers/ata/pata_platform.c
index cb3134bf88eb..29902001e223 100644
--- a/drivers/ata/pata_platform.c
+++ b/drivers/ata/pata_platform.c
@@ -199,14 +199,14 @@ static int pata_platform_probe(struct platform_device *pdev)
 	 * Get the I/O base first
 	 */
 	io_res = platform_get_mem_or_io(pdev, 0);
-	if (unlikely(!io_res))
+	if (!io_res)
 		return -EINVAL;
 
 	/*
 	 * Then the CTL base
 	 */
 	ctl_res = platform_get_mem_or_io(pdev, 1);
-	if (unlikely(!ctl_res))
+	if (!ctl_res)
 		return -EINVAL;
 
 	/*
-- 
2.17.1

[PATCH v3 03/10] ata: pata_of_platform: Use platform_get_irq_optional() to get the interrupt

[Lad Prabhakar]

platform_get_resource(pdev, IORESOURCE_IRQ, ..) relies on static
allocation of IRQ resources in DT core code, this causes an issue
when using hierarchical interrupt domains using "interrupts" property
in the node as this bypasses the hierarchical setup and messes up the
irq chaining.

In preparation for removal of static setup of IRQ resource from DT core
code use platform_get_irq_optional().

Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
v2-->v3
* New patch
---
 drivers/ata/pata_of_platform.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/drivers/ata/pata_of_platform.c b/drivers/ata/pata_of_platform.c
index 35aa158fc976..2e2ec7d77726 100644
--- a/drivers/ata/pata_of_platform.c
+++ b/drivers/ata/pata_of_platform.c
@@ -25,11 +25,12 @@ static int pata_of_platform_probe(struct platform_device *ofdev)
 	struct device_node *dn = ofdev->dev.of_node;
 	struct resource io_res;
 	struct resource ctl_res;
-	struct resource *irq_res;
+	struct resource irq_res;
 	unsigned int reg_shift = 0;
 	int pio_mode = 0;
 	int pio_mask;
 	bool use16bit;
+	int irq;
 
 	ret = of_address_to_resource(dn, 0, &io_res);
 	if (ret) {
@@ -45,7 +46,14 @@ static int pata_of_platform_probe(struct platform_device *ofdev)
 		return -EINVAL;
 	}
 
-	irq_res = platform_get_resource(ofdev, IORESOURCE_IRQ, 0);
+	irq = platform_get_irq_optional(ofdev, 0);
+	if (irq < 0 && irq != -ENXIO)
+		return irq;
+
+	if (irq > 0) {
+		memset(&irq_res, 0x0, sizeof(struct resource));
+		irq_res.start = irq;
+	}
 
 	of_property_read_u32(dn, "reg-shift", &reg_shift);
 
@@ -63,7 +71,7 @@ static int pata_of_platform_probe(struct platform_device *ofdev)
 	pio_mask = 1 << pio_mode;
 	pio_mask |= (1 << pio_mode) - 1;
 
-	return __pata_platform_probe(&ofdev->dev, &io_res, &ctl_res, irq_res,
+	return __pata_platform_probe(&ofdev->dev, &io_res, &ctl_res, irq > 0 ? &irq_res : NULL,
 				     reg_shift, pio_mask, &pata_platform_sht,
 				     use16bit);
 }
-- 
2.17.1

[PATCH v3 04/10] ata: pata_platform: Use platform_get_irq_optional() to get the interrupt

[Lad Prabhakar]

To be consistent with pata_of_platform driver use
platform_get_irq_optional() instead of
platform_get_resource(pdev, IORESOURCE_IRQ, 0).

Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
v2-->v3
* New patch
---
 drivers/ata/pata_platform.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/drivers/ata/pata_platform.c b/drivers/ata/pata_platform.c
index 29902001e223..2e439b923762 100644
--- a/drivers/ata/pata_platform.c
+++ b/drivers/ata/pata_platform.c
@@ -184,8 +184,9 @@ static int pata_platform_probe(struct platform_device *pdev)
 {
 	struct resource *io_res;
 	struct resource *ctl_res;
-	struct resource *irq_res;
+	struct resource irq_res;
 	struct pata_platform_info *pp_info = dev_get_platdata(&pdev->dev);
+	int irq;
 
 	/*
 	 * Simple resource validation ..
@@ -212,9 +213,15 @@ static int pata_platform_probe(struct platform_device *pdev)
 	/*
 	 * And the IRQ
 	 */
-	irq_res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
+	irq = platform_get_irq_optional(pdev, 0);
+	if (irq < 0 && irq != -ENXIO)
+		return irq;
+	if (irq > 0) {
+		memset(&irq_res, 0x0, sizeof(struct resource));
+		irq_res.start = irq;
+	}
 
-	return __pata_platform_probe(&pdev->dev, io_res, ctl_res, irq_res,
+	return __pata_platform_probe(&pdev->dev, io_res, ctl_res, irq > 0 ? &irq_res : NULL,
 				     pp_info ? pp_info->ioport_shift : 0,
 				     pio_mask, &pata_platform_sht, false);
 }
-- 
2.17.1

[PATCH v3 05/10] ata: pata_platform: Drop checking IRQ number

[Lad Prabhakar]

The callers of __pata_platform_probe() will pass the IRQ resource only for
valid IRQ's, for invalid IRQ's the IRQ resource will always be NULL. So
drop checking the IRQ number.

Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
v2-->v3
* New patch
---
 drivers/ata/pata_platform.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/ata/pata_platform.c b/drivers/ata/pata_platform.c
index 2e439b923762..f500f631f72b 100644
--- a/drivers/ata/pata_platform.c
+++ b/drivers/ata/pata_platform.c
@@ -114,7 +114,7 @@ int __pata_platform_probe(struct device *dev, struct resource *io_res,
 	/*
 	 * And the IRQ
 	 */
-	if (irq_res && irq_res->start > 0) {
+	if (irq_res) {
 		irq = irq_res->start;
 		irq_flags = (irq_res->flags & IRQF_TRIGGER_MASK) | IRQF_SHARED;
 	}
-- 
2.17.1

[PATCH v3 06/10] ata: pata_of_platform: Make use of platform_get_mem_or_io()

[Lad Prabhakar]

To be consistent with pata_platform driver use
platform_get_mem_or_io() instead of of_address_to_resource().

Suggested-by: Sergey Shtylyov <s.shtylyov@omp.ru>
Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
v2-->v3
* New patch
---
 drivers/ata/pata_of_platform.c | 16 +++++++---------
 1 file changed, 7 insertions(+), 9 deletions(-)

diff --git a/drivers/ata/pata_of_platform.c b/drivers/ata/pata_of_platform.c
index 2e2ec7d77726..b9c9b7311112 100644
--- a/drivers/ata/pata_of_platform.c
+++ b/drivers/ata/pata_of_platform.c
@@ -8,7 +8,6 @@
 
 #include <linux/kernel.h>
 #include <linux/module.h>
-#include <linux/of_address.h>
 #include <linux/platform_device.h>
 #include <linux/ata_platform.h>
 #include <linux/libata.h>
@@ -21,10 +20,9 @@ static struct scsi_host_template pata_platform_sht = {
 
 static int pata_of_platform_probe(struct platform_device *ofdev)
 {
-	int ret;
 	struct device_node *dn = ofdev->dev.of_node;
-	struct resource io_res;
-	struct resource ctl_res;
+	struct resource *io_res;
+	struct resource *ctl_res;
 	struct resource irq_res;
 	unsigned int reg_shift = 0;
 	int pio_mode = 0;
@@ -32,15 +30,15 @@ static int pata_of_platform_probe(struct platform_device *ofdev)
 	bool use16bit;
 	int irq;
 
-	ret = of_address_to_resource(dn, 0, &io_res);
-	if (ret) {
+	io_res = platform_get_mem_or_io(ofdev, 0);
+	if (!io_res) {
 		dev_err(&ofdev->dev, "can't get IO address from "
 			"device tree\n");
 		return -EINVAL;
 	}
 
-	ret = of_address_to_resource(dn, 1, &ctl_res);
-	if (ret) {
+	ctl_res = platform_get_mem_or_io(ofdev, 1);
+	if (!ctl_res) {
 		dev_err(&ofdev->dev, "can't get CTL address from "
 			"device tree\n");
 		return -EINVAL;
@@ -71,7 +69,7 @@ static int pata_of_platform_probe(struct platform_device *ofdev)
 	pio_mask = 1 << pio_mode;
 	pio_mask |= (1 << pio_mode) - 1;
 
-	return __pata_platform_probe(&ofdev->dev, &io_res, &ctl_res, irq > 0 ? &irq_res : NULL,
+	return __pata_platform_probe(&ofdev->dev, io_res, ctl_res, irq > 0 ? &irq_res : NULL,
 				     reg_shift, pio_mask, &pata_platform_sht,
 				     use16bit);
 }
-- 
2.17.1

[PATCH v3 07/10] ata: pata_platform: Merge pata_of_platform into pata_platform

[Lad Prabhakar]

Merge the OF pata_of_platform driver into pata_platform.

Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
v2-->v3
* Introduced new function pata_platform_get_resources()

v1-->v2
* Dropped check for IRQ0
* Dropped setting the irqflags as suggested by Rob
* Set just the irq start
* Fixed freeing up irq_res when not present in DT
* Dropped PATA_OF_PLATFORM entry
* Split up sorting of headers in separate patch
* Dropped sht from struct pata_platform_priv
---
 drivers/ata/Kconfig            |  10 --
 drivers/ata/Makefile           |   1 -
 drivers/ata/pata_of_platform.c |  96 ----------------
 drivers/ata/pata_platform.c    | 194 +++++++++++++++++++++++----------
 include/linux/ata_platform.h   |   9 --
 5 files changed, 139 insertions(+), 171 deletions(-)
 delete mode 100644 drivers/ata/pata_of_platform.c

diff --git a/drivers/ata/Kconfig b/drivers/ata/Kconfig
index a7da8ea7b3ed..0132a6a49247 100644
--- a/drivers/ata/Kconfig
+++ b/drivers/ata/Kconfig
@@ -1120,16 +1120,6 @@ config PATA_PLATFORM
 
 	  If unsure, say N.
 
-config PATA_OF_PLATFORM
-	tristate "OpenFirmware platform device PATA support"
-	depends on PATA_PLATFORM && OF
-	help
-	  This option enables support for generic directly connected ATA
-	  devices commonly found on embedded systems with OpenFirmware
-	  bindings.
-
-	  If unsure, say N.
-
 config PATA_QDI
 	tristate "QDI VLB PATA support"
 	depends on ISA
diff --git a/drivers/ata/Makefile b/drivers/ata/Makefile
index b8aebfb14e82..0323b2be1b2f 100644
--- a/drivers/ata/Makefile
+++ b/drivers/ata/Makefile
@@ -107,7 +107,6 @@ obj-$(CONFIG_PATA_OPTI)		+= pata_opti.o
 obj-$(CONFIG_PATA_PCMCIA)	+= pata_pcmcia.o
 obj-$(CONFIG_PATA_PALMLD)	+= pata_palmld.o
 obj-$(CONFIG_PATA_PLATFORM)	+= pata_platform.o
-obj-$(CONFIG_PATA_OF_PLATFORM)	+= pata_of_platform.o
 obj-$(CONFIG_PATA_RB532)	+= pata_rb532_cf.o
 obj-$(CONFIG_PATA_RZ1000)	+= pata_rz1000.o
 obj-$(CONFIG_PATA_SAMSUNG_CF)	+= pata_samsung_cf.o
diff --git a/drivers/ata/pata_of_platform.c b/drivers/ata/pata_of_platform.c
deleted file mode 100644
index b9c9b7311112..000000000000
--- a/drivers/ata/pata_of_platform.c
+++ /dev/null
@@ -1,96 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0-only
-/*
- * OF-platform PATA driver
- *
- * Copyright (c) 2007  MontaVista Software, Inc.
- *                     Anton Vorontsov <avorontsov@ru.mvista.com>
- */
-
-#include <linux/kernel.h>
-#include <linux/module.h>
-#include <linux/platform_device.h>
-#include <linux/ata_platform.h>
-#include <linux/libata.h>
-
-#define DRV_NAME "pata_of_platform"
-
-static struct scsi_host_template pata_platform_sht = {
-	ATA_PIO_SHT(DRV_NAME),
-};
-
-static int pata_of_platform_probe(struct platform_device *ofdev)
-{
-	struct device_node *dn = ofdev->dev.of_node;
-	struct resource *io_res;
-	struct resource *ctl_res;
-	struct resource irq_res;
-	unsigned int reg_shift = 0;
-	int pio_mode = 0;
-	int pio_mask;
-	bool use16bit;
-	int irq;
-
-	io_res = platform_get_mem_or_io(ofdev, 0);
-	if (!io_res) {
-		dev_err(&ofdev->dev, "can't get IO address from "
-			"device tree\n");
-		return -EINVAL;
-	}
-
-	ctl_res = platform_get_mem_or_io(ofdev, 1);
-	if (!ctl_res) {
-		dev_err(&ofdev->dev, "can't get CTL address from "
-			"device tree\n");
-		return -EINVAL;
-	}
-
-	irq = platform_get_irq_optional(ofdev, 0);
-	if (irq < 0 && irq != -ENXIO)
-		return irq;
-
-	if (irq > 0) {
-		memset(&irq_res, 0x0, sizeof(struct resource));
-		irq_res.start = irq;
-	}
-
-	of_property_read_u32(dn, "reg-shift", &reg_shift);
-
-	if (!of_property_read_u32(dn, "pio-mode", &pio_mode)) {
-		if (pio_mode > 6) {
-			dev_err(&ofdev->dev, "invalid pio-mode\n");
-			return -EINVAL;
-		}
-	} else {
-		dev_info(&ofdev->dev, "pio-mode unspecified, assuming PIO0\n");
-	}
-
-	use16bit = of_property_read_bool(dn, "ata-generic,use16bit");
-
-	pio_mask = 1 << pio_mode;
-	pio_mask |= (1 << pio_mode) - 1;
-
-	return __pata_platform_probe(&ofdev->dev, io_res, ctl_res, irq > 0 ? &irq_res : NULL,
-				     reg_shift, pio_mask, &pata_platform_sht,
-				     use16bit);
-}
-
-static const struct of_device_id pata_of_platform_match[] = {
-	{ .compatible = "ata-generic", },
-	{ },
-};
-MODULE_DEVICE_TABLE(of, pata_of_platform_match);
-
-static struct platform_driver pata_of_platform_driver = {
-	.driver = {
-		.name = DRV_NAME,
-		.of_match_table = pata_of_platform_match,
-	},
-	.probe		= pata_of_platform_probe,
-	.remove		= ata_platform_remove_one,
-};
-
-module_platform_driver(pata_of_platform_driver);
-
-MODULE_DESCRIPTION("OF-platform PATA driver");
-MODULE_AUTHOR("Anton Vorontsov <avorontsov@ru.mvista.com>");
-MODULE_LICENSE("GPL");
diff --git a/drivers/ata/pata_platform.c b/drivers/ata/pata_platform.c
index f500f631f72b..4273f1a9abd2 100644
--- a/drivers/ata/pata_platform.c
+++ b/drivers/ata/pata_platform.c
@@ -25,7 +25,25 @@
 
 static int pio_mask = 1;
 module_param(pio_mask, int, 0);
-MODULE_PARM_DESC(pio_mask, "PIO modes supported, mode 0 only by default");
+MODULE_PARM_DESC(pio_mask, "PIO modes supported, mode 0 only by default (param valid only for non DT users)");
+
+/**
+ * struct pata_platform_priv - Private info
+ * @io_res: Resource representing I/O base
+ * @ctl_res: Resource representing CTL base
+ * @irq_res: Resource representing IRQ and its flags
+ * @ioport_shift: I/O port shift
+ * @pio_mask: PIO mask
+ * @use16bit: Flag to indicate 16-bit IO instead of 32-bit
+ */
+struct pata_platform_priv {
+	struct resource *io_res;
+	struct resource *ctl_res;
+	struct resource *irq_res;
+	unsigned int ioport_shift;
+	int pio_mask;
+	bool use16bit;
+};
 
 /*
  * Provide our own set_mode() as we don't want to change anything that has
@@ -66,15 +84,9 @@ static void pata_platform_setup_port(struct ata_ioports *ioaddr,
 }
 
 /**
- *	__pata_platform_probe		-	attach a platform interface
+ *	pata_platform_host_activate - attach a platform interface
  *	@dev: device
- *	@io_res: Resource representing I/O base
- *	@ctl_res: Resource representing CTL base
- *	@irq_res: Resource representing IRQ and its flags
- *	@ioport_shift: I/O port shift
- *	@__pio_mask: PIO mask
- *	@sht: scsi_host_template to use when registering
- *	@use16bit: Flag to indicate 16-bit IO instead of 32-bit
+ *	@priv: Pointer to struct pata_platform_priv
  *
  *	Register a platform bus IDE interface. Such interfaces are PIO and we
  *	assume do not support IRQ sharing.
@@ -94,10 +106,7 @@ static void pata_platform_setup_port(struct ata_ioports *ioaddr,
  *
  *	If no IRQ resource is present, PIO polling mode is used instead.
  */
-int __pata_platform_probe(struct device *dev, struct resource *io_res,
-			  struct resource *ctl_res, struct resource *irq_res,
-			  unsigned int ioport_shift, int __pio_mask,
-			  struct scsi_host_template *sht, bool use16bit)
+static int pata_platform_host_activate(struct device *dev, struct pata_platform_priv *priv)
 {
 	struct ata_host *host;
 	struct ata_port *ap;
@@ -108,15 +117,15 @@ int __pata_platform_probe(struct device *dev, struct resource *io_res,
 	/*
 	 * Check for MMIO
 	 */
-	mmio = (( io_res->flags == IORESOURCE_MEM) &&
-		(ctl_res->flags == IORESOURCE_MEM));
+	mmio = ((priv->io_res->flags == IORESOURCE_MEM) &&
+		(priv->ctl_res->flags == IORESOURCE_MEM));
 
 	/*
 	 * And the IRQ
 	 */
-	if (irq_res) {
-		irq = irq_res->start;
-		irq_flags = (irq_res->flags & IRQF_TRIGGER_MASK) | IRQF_SHARED;
+	if (priv->irq_res && priv->irq_res->start > 0) {
+		irq = priv->irq_res->start;
+		irq_flags = (priv->irq_res->flags & IRQF_TRIGGER_MASK) | IRQF_SHARED;
 	}
 
 	/*
@@ -131,12 +140,12 @@ int __pata_platform_probe(struct device *dev, struct resource *io_res,
 	ap->ops->inherits = &ata_sff_port_ops;
 	ap->ops->cable_detect = ata_cable_unknown;
 	ap->ops->set_mode = pata_platform_set_mode;
-	if (use16bit)
+	if (priv->use16bit)
 		ap->ops->sff_data_xfer = ata_sff_data_xfer;
 	else
 		ap->ops->sff_data_xfer = ata_sff_data_xfer32;
 
-	ap->pio_mask = __pio_mask;
+	ap->pio_mask = priv->pio_mask;
 	ap->flags |= ATA_FLAG_SLAVE_POSS;
 
 	/*
@@ -151,15 +160,15 @@ int __pata_platform_probe(struct device *dev, struct resource *io_res,
 	 * Handle the MMIO case
 	 */
 	if (mmio) {
-		ap->ioaddr.cmd_addr = devm_ioremap(dev, io_res->start,
-				resource_size(io_res));
-		ap->ioaddr.ctl_addr = devm_ioremap(dev, ctl_res->start,
-				resource_size(ctl_res));
+		ap->ioaddr.cmd_addr = devm_ioremap(dev, priv->io_res->start,
+						   resource_size(priv->io_res));
+		ap->ioaddr.ctl_addr = devm_ioremap(dev, priv->ctl_res->start,
+						   resource_size(priv->ctl_res));
 	} else {
-		ap->ioaddr.cmd_addr = devm_ioport_map(dev, io_res->start,
-				resource_size(io_res));
-		ap->ioaddr.ctl_addr = devm_ioport_map(dev, ctl_res->start,
-				resource_size(ctl_res));
+		ap->ioaddr.cmd_addr = devm_ioport_map(dev, priv->io_res->start,
+						      resource_size(priv->io_res));
+		ap->ioaddr.ctl_addr = devm_ioport_map(dev, priv->ctl_res->start,
+						      resource_size(priv->ctl_res));
 	}
 	if (!ap->ioaddr.cmd_addr || !ap->ioaddr.ctl_addr) {
 		dev_err(dev, "failed to map IO/CTL base\n");
@@ -168,46 +177,34 @@ int __pata_platform_probe(struct device *dev, struct resource *io_res,
 
 	ap->ioaddr.altstatus_addr = ap->ioaddr.ctl_addr;
 
-	pata_platform_setup_port(&ap->ioaddr, ioport_shift);
+	pata_platform_setup_port(&ap->ioaddr, priv->ioport_shift);
 
 	ata_port_desc(ap, "%s cmd 0x%llx ctl 0x%llx", mmio ? "mmio" : "ioport",
-		      (unsigned long long)io_res->start,
-		      (unsigned long long)ctl_res->start);
+		      (unsigned long long)priv->io_res->start,
+		      (unsigned long long)priv->ctl_res->start);
 
 	/* activate */
 	return ata_host_activate(host, irq, irq ? ata_sff_interrupt : NULL,
-				 irq_flags, sht);
+				 irq_flags, &pata_platform_sht);
 }
-EXPORT_SYMBOL_GPL(__pata_platform_probe);
 
-static int pata_platform_probe(struct platform_device *pdev)
+static int pata_platform_get_resources(struct platform_device *pdev,
+				       struct pata_platform_priv *priv)
 {
-	struct resource *io_res;
-	struct resource *ctl_res;
-	struct resource irq_res;
-	struct pata_platform_info *pp_info = dev_get_platdata(&pdev->dev);
 	int irq;
 
-	/*
-	 * Simple resource validation ..
-	 */
-	if ((pdev->num_resources != 3) && (pdev->num_resources != 2)) {
-		dev_err(&pdev->dev, "invalid number of resources\n");
-		return -EINVAL;
-	}
-
 	/*
 	 * Get the I/O base first
 	 */
-	io_res = platform_get_mem_or_io(pdev, 0);
-	if (!io_res)
+	priv->io_res = platform_get_mem_or_io(pdev, 0);
+	if (!priv->io_res)
 		return -EINVAL;
 
 	/*
 	 * Then the CTL base
 	 */
-	ctl_res = platform_get_mem_or_io(pdev, 1);
-	if (!ctl_res)
+	priv->ctl_res = platform_get_mem_or_io(pdev, 1);
+	if (!priv->ctl_res)
 		return -EINVAL;
 
 	/*
@@ -216,21 +213,108 @@ static int pata_platform_probe(struct platform_device *pdev)
 	irq = platform_get_irq_optional(pdev, 0);
 	if (irq < 0 && irq != -ENXIO)
 		return irq;
+
 	if (irq > 0) {
-		memset(&irq_res, 0x0, sizeof(struct resource));
-		irq_res.start = irq;
+		struct resource *irq_res;
+
+		irq_res = devm_kzalloc(&pdev->dev, sizeof(*irq_res), GFP_KERNEL);
+		if (!irq_res)
+			return -ENOMEM;
+
+		irq_res->start = irq;
+		priv->irq_res = irq_res;
 	}
 
-	return __pata_platform_probe(&pdev->dev, io_res, ctl_res, irq > 0 ? &irq_res : NULL,
-				     pp_info ? pp_info->ioport_shift : 0,
-				     pio_mask, &pata_platform_sht, false);
+	return 0;
 }
 
+static int pata_of_platform_get_pdata(struct platform_device *ofdev,
+				      struct pata_platform_priv *priv)
+{
+	struct device_node *dn = ofdev->dev.of_node;
+	int pio_mode = 0;
+	int ret;
+
+	ret = pata_platform_get_resources(ofdev, priv);
+	if (ret)
+		return ret;
+
+	of_property_read_u32(dn, "reg-shift", &priv->ioport_shift);
+
+	ret = of_property_read_u32(dn, "pio-mode", &pio_mode);
+	if (!ret) {
+		if (pio_mode > 6) {
+			dev_err(&ofdev->dev, "invalid pio-mode\n");
+			return -EINVAL;
+		}
+	} else {
+		dev_info(&ofdev->dev, "pio-mode unspecified, assuming PIO0\n");
+	}
+
+	priv->use16bit = of_property_read_bool(dn, "ata-generic,use16bit");
+
+	priv->pio_mask = 1 << pio_mode;
+	priv->pio_mask |= (1 << pio_mode) - 1;
+
+	return 0;
+}
+
+static int pata_platform_get_pdata(struct platform_device *pdev,
+				   struct pata_platform_priv *priv)
+{
+	struct pata_platform_info *pp_info = dev_get_platdata(&pdev->dev);
+	int ret;
+
+	/*
+	 * Simple resource validation ..
+	 */
+	if ((pdev->num_resources != 3) && (pdev->num_resources != 2)) {
+		dev_err(&pdev->dev, "invalid number of resources\n");
+		return -EINVAL;
+	}
+
+	ret = pata_platform_get_resources(pdev, priv);
+	if (ret)
+		return ret;
+
+	priv->ioport_shift = pp_info ? pp_info->ioport_shift : 0;
+	priv->pio_mask = pio_mask;
+	priv->use16bit = false;
+
+	return 0;
+}
+
+static int pata_platform_probe(struct platform_device *pdev)
+{
+	struct pata_platform_priv *priv;
+	int ret;
+
+	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	if (!dev_of_node(&pdev->dev))
+		ret = pata_platform_get_pdata(pdev, priv);
+	else
+		ret = pata_of_platform_get_pdata(pdev, priv);
+	if (ret)
+		return ret;
+
+	return pata_platform_host_activate(&pdev->dev, priv);
+}
+
+static const struct of_device_id pata_of_platform_match[] = {
+	{ .compatible = "ata-generic", },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, pata_of_platform_match);
+
 static struct platform_driver pata_platform_driver = {
 	.probe		= pata_platform_probe,
 	.remove		= ata_platform_remove_one,
 	.driver = {
 		.name		= DRV_NAME,
+		.of_match_table = pata_of_platform_match,
 	},
 };
 
diff --git a/include/linux/ata_platform.h b/include/linux/ata_platform.h
index 9cafec92282d..91b8529e6712 100644
--- a/include/linux/ata_platform.h
+++ b/include/linux/ata_platform.h
@@ -13,15 +13,6 @@ struct pata_platform_info {
 
 struct scsi_host_template;
 
-extern int __pata_platform_probe(struct device *dev,
-				 struct resource *io_res,
-				 struct resource *ctl_res,
-				 struct resource *irq_res,
-				 unsigned int ioport_shift,
-				 int __pio_mask,
-				 struct scsi_host_template *sht,
-				 bool use16bit);
-
 /*
  * Marvell SATA private data
  */
-- 
2.17.1

[PATCH v3 08/10] ata: pata_platform: Drop validating num_resources count

[Lad Prabhakar]

Drop validating num_resources count as pata_platform_get_resources()
already does this check for us.

Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
v2-->v3
* New patch
---
 drivers/ata/pata_platform.c | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/drivers/ata/pata_platform.c b/drivers/ata/pata_platform.c
index 4273f1a9abd2..88a9bdc81e68 100644
--- a/drivers/ata/pata_platform.c
+++ b/drivers/ata/pata_platform.c
@@ -265,14 +265,6 @@ static int pata_platform_get_pdata(struct platform_device *pdev,
 	struct pata_platform_info *pp_info = dev_get_platdata(&pdev->dev);
 	int ret;
 
-	/*
-	 * Simple resource validation ..
-	 */
-	if ((pdev->num_resources != 3) && (pdev->num_resources != 2)) {
-		dev_err(&pdev->dev, "invalid number of resources\n");
-		return -EINVAL;
-	}
-
 	ret = pata_platform_get_resources(pdev, priv);
 	if (ret)
 		return ret;
-- 
2.17.1

[PATCH v3 09/10] ata: pata_platform: Sort the #includes alphabetically

[Lad Prabhakar]

Sort the #includes alphabetically.

Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
v2-->v3
* Dropped of_address.h
* Dropped RB tag

v1-->v2
* New patch
---
 drivers/ata/pata_platform.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/ata/pata_platform.c b/drivers/ata/pata_platform.c
index 88a9bdc81e68..b37c1028fd54 100644
--- a/drivers/ata/pata_platform.c
+++ b/drivers/ata/pata_platform.c
@@ -11,14 +11,14 @@
  * License.  See the file "COPYING" in the main directory of this archive
  * for more details.
  */
-#include <linux/kernel.h>
-#include <linux/module.h>
-#include <linux/blkdev.h>
-#include <scsi/scsi_host.h>
 #include <linux/ata.h>
+#include <linux/ata_platform.h>
+#include <linux/blkdev.h>
+#include <linux/kernel.h>
 #include <linux/libata.h>
+#include <linux/module.h>
 #include <linux/platform_device.h>
-#include <linux/ata_platform.h>
+#include <scsi/scsi_host.h>
 
 #define DRV_NAME "pata_platform"
 #define DRV_VERSION "1.2"
-- 
2.17.1

[PATCH v3 10/10] ata: pata_platform: Make use of GENMASK() macro

[Lad Prabhakar]

Make use of GENMASK() macro instead of open coding.

Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
Reviewed-by: Sergey Shtylyov <s.shtylyov@omp.ru>
---
v2-->v3
* Included RB tag

v1-->v2
* New patch
---
 drivers/ata/pata_platform.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/ata/pata_platform.c b/drivers/ata/pata_platform.c
index b37c1028fd54..b68bce361c74 100644
--- a/drivers/ata/pata_platform.c
+++ b/drivers/ata/pata_platform.c
@@ -253,8 +253,7 @@ static int pata_of_platform_get_pdata(struct platform_device *ofdev,
 
 	priv->use16bit = of_property_read_bool(dn, "ata-generic,use16bit");
 
-	priv->pio_mask = 1 << pio_mode;
-	priv->pio_mask |= (1 << pio_mode) - 1;
+	priv->pio_mask = GENMASK(pio_mode, 0);
 
 	return 0;
 }
-- 
2.17.1

Re: [PATCH v3 02/10] ata: pata_platform: Drop use of unlikely() in pata_platform_probe

[Sergey Shtylyov]

On 12/24/21 4:12 PM, Lad Prabhakar wrote:

> pata_platform_probe() isn't a hotpath, which makes it's questionable to
> use unlikely(). Therefore let's simply drop it.
> 
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> ---
> v2-->v3
> * New patch
> ---
>  drivers/ata/pata_platform.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/ata/pata_platform.c b/drivers/ata/pata_platform.c
> index cb3134bf88eb..29902001e223 100644
> --- a/drivers/ata/pata_platform.c
> +++ b/drivers/ata/pata_platform.c
> @@ -199,14 +199,14 @@ static int pata_platform_probe(struct platform_device *pdev)
>  	 * Get the I/O base first
>  	 */
>  	io_res = platform_get_mem_or_io(pdev, 0);
> -	if (unlikely(!io_res))
> +	if (!io_res)
>  		return -EINVAL;
>  
>  	/*
>  	 * Then the CTL base
>  	 */
>  	ctl_res = platform_get_mem_or_io(pdev, 1);
> -	if (unlikely(!ctl_res))
> +	if (!ctl_res)
>  		return -EINVAL;

   I think you should combine this with patch #1.

[...]

MBR, Sergey

Re: [PATCH v3 01/10] ata: pata_platform: Make use of platform_get_mem_or_io()

[Sergey Shtylyov]

On 12/24/21 4:12 PM, Lad Prabhakar wrote:

> Make use of platform_get_mem_or_io() to simplify the code.
> 
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> Reviewed-by: Sergey Shtylyov <s.shtylyov@omp.ru>
> ---
>  drivers/ata/pata_platform.c | 18 ++++++------------
>  1 file changed, 6 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/ata/pata_platform.c b/drivers/ata/pata_platform.c
> index 028329428b75..cb3134bf88eb 100644
> --- a/drivers/ata/pata_platform.c
> +++ b/drivers/ata/pata_platform.c
> @@ -198,22 +198,16 @@ static int pata_platform_probe(struct platform_device *pdev)
>  	/*
>  	 * Get the I/O base first
>  	 */
> -	io_res = platform_get_resource(pdev, IORESOURCE_IO, 0);
> -	if (io_res == NULL) {
> -		io_res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> -		if (unlikely(io_res == NULL))
> -			return -EINVAL;
> -	}
> +	io_res = platform_get_mem_or_io(pdev, 0);
> +	if (unlikely(!io_res))

   You don't have to keep unlikely() here -- the first *if* doesn't have it anyway,
only the 2nd one does...

> +		return -EINVAL;
>  
>  	/*
>  	 * Then the CTL base
>  	 */
> -	ctl_res = platform_get_resource(pdev, IORESOURCE_IO, 1);
> -	if (ctl_res == NULL) {
> -		ctl_res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> -		if (unlikely(ctl_res == NULL))
> -			return -EINVAL;
> -	}
> +	ctl_res = platform_get_mem_or_io(pdev, 1);
> +	if (unlikely(!ctl_res))

   Ditto.

> +		return -EINVAL;

MBR, Sergey

Re: [PATCH v3 01/10] ata: pata_platform: Make use of platform_get_mem_or_io()

[Lad, Prabhakar]

Hi Sergey,

Thank you for the review.

On Fri, Dec 24, 2021 at 5:56 PM Sergey Shtylyov <s.shtylyov@omp.ru> wrote:
>
> On 12/24/21 4:12 PM, Lad Prabhakar wrote:
>
> > Make use of platform_get_mem_or_io() to simplify the code.
> >
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > Reviewed-by: Sergey Shtylyov <s.shtylyov@omp.ru>
> > ---
> >  drivers/ata/pata_platform.c | 18 ++++++------------
> >  1 file changed, 6 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/ata/pata_platform.c b/drivers/ata/pata_platform.c
> > index 028329428b75..cb3134bf88eb 100644
> > --- a/drivers/ata/pata_platform.c
> > +++ b/drivers/ata/pata_platform.c
> > @@ -198,22 +198,16 @@ static int pata_platform_probe(struct platform_device *pdev)
> >       /*
> >        * Get the I/O base first
> >        */
> > -     io_res = platform_get_resource(pdev, IORESOURCE_IO, 0);
> > -     if (io_res == NULL) {
> > -             io_res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > -             if (unlikely(io_res == NULL))
> > -                     return -EINVAL;
> > -     }
> > +     io_res = platform_get_mem_or_io(pdev, 0);
> > +     if (unlikely(!io_res))
>
>    You don't have to keep unlikely() here -- the first *if* doesn't have it anyway,
> only the 2nd one does...
>
Agreed the first if doesn't have it, ie unlikely() will only be hit
when resource 0 isn't mem/IO, So with my patch introduced if mem/io is
NULL  unlikely() will be called. So there is "NO" behavioral change.

> > +             return -EINVAL;
> >
> >       /*
> >        * Then the CTL base
> >        */
> > -     ctl_res = platform_get_resource(pdev, IORESOURCE_IO, 1);
> > -     if (ctl_res == NULL) {
> > -             ctl_res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> > -             if (unlikely(ctl_res == NULL))
> > -                     return -EINVAL;
> > -     }
> > +     ctl_res = platform_get_mem_or_io(pdev, 1);
> > +     if (unlikely(!ctl_res))
>
>    Ditto.
>
Ditto.

Cheers,
Prabhakar

Re: [PATCH v3 02/10] ata: pata_platform: Drop use of unlikely() in pata_platform_probe

[Lad, Prabhakar]

Hi Sergey,

Thank you for the review.

On Fri, Dec 24, 2021 at 5:54 PM Sergey Shtylyov <s.shtylyov@omp.ru> wrote:
>
> On 12/24/21 4:12 PM, Lad Prabhakar wrote:
>
> > pata_platform_probe() isn't a hotpath, which makes it's questionable to
> > use unlikely(). Therefore let's simply drop it.
> >
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > ---
> > v2-->v3
> > * New patch
> > ---
> >  drivers/ata/pata_platform.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/ata/pata_platform.c b/drivers/ata/pata_platform.c
> > index cb3134bf88eb..29902001e223 100644
> > --- a/drivers/ata/pata_platform.c
> > +++ b/drivers/ata/pata_platform.c
> > @@ -199,14 +199,14 @@ static int pata_platform_probe(struct platform_device *pdev)
> >        * Get the I/O base first
> >        */
> >       io_res = platform_get_mem_or_io(pdev, 0);
> > -     if (unlikely(!io_res))
> > +     if (!io_res)
> >               return -EINVAL;
> >
> >       /*
> >        * Then the CTL base
> >        */
> >       ctl_res = platform_get_mem_or_io(pdev, 1);
> > -     if (unlikely(!ctl_res))
> > +     if (!ctl_res)
> >               return -EINVAL;
>
>    I think you should combine this with patch #1.
>
I'd like to keep the changes separate from patch #1, as it's unrelated.

Cheers,
Prabhakar

Re: [PATCH v3 03/10] ata: pata_of_platform: Use platform_get_irq_optional() to get the interrupt

[Andy Shevchenko]

On Sat, Dec 25, 2021 at 3:55 AM Lad Prabhakar
<prabhakar.mahadev-lad.rj@bp.renesas.com> wrote:
>
> platform_get_resource(pdev, IORESOURCE_IRQ, ..) relies on static
> allocation of IRQ resources in DT core code, this causes an issue
> when using hierarchical interrupt domains using "interrupts" property
> in the node as this bypasses the hierarchical setup and messes up the
> irq chaining.
>
> In preparation for removal of static setup of IRQ resource from DT core
> code use platform_get_irq_optional().

...

> +       irq = platform_get_irq_optional(ofdev, 0);
> +       if (irq < 0 && irq != -ENXIO)
> +               return irq;
> +
> +       if (irq > 0) {

> +               memset(&irq_res, 0x0, sizeof(struct resource));

Why do you need that variable at all?

0x0 -> 0
sizeof(irq_res)

> +               irq_res.start = irq;
> +       }

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 03/10] ata: pata_of_platform: Use platform_get_irq_optional() to get the interrupt

[Lad, Prabhakar]

Hi Andy,

Thank you for the review.

On Sat, Dec 25, 2021 at 5:02 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Sat, Dec 25, 2021 at 3:55 AM Lad Prabhakar
> <prabhakar.mahadev-lad.rj@bp.renesas.com> wrote:
> >
> > platform_get_resource(pdev, IORESOURCE_IRQ, ..) relies on static
> > allocation of IRQ resources in DT core code, this causes an issue
> > when using hierarchical interrupt domains using "interrupts" property
> > in the node as this bypasses the hierarchical setup and messes up the
> > irq chaining.
> >
> > In preparation for removal of static setup of IRQ resource from DT core
> > code use platform_get_irq_optional().
>
> ...
>
> > +       irq = platform_get_irq_optional(ofdev, 0);
> > +       if (irq < 0 && irq != -ENXIO)
> > +               return irq;
> > +
> > +       if (irq > 0) {
>
> > +               memset(&irq_res, 0x0, sizeof(struct resource));
>
> Why do you need that variable at all?
>
Are you referring to the irq_res variable? That's because
__pata_platform_probe() requires it.

> 0x0 -> 0
> sizeof(irq_res)
>
OK, I will update it.

Cheers,
Prabhakar

Re: [PATCH v3 07/10] ata: pata_platform: Merge pata_of_platform into pata_platform

[Andy Shevchenko]

On Sat, Dec 25, 2021 at 3:56 AM Lad Prabhakar
<prabhakar.mahadev-lad.rj@bp.renesas.com> wrote:
>
> Merge the OF pata_of_platform driver into pata_platform.

For the further improvements...

...

> +MODULE_PARM_DESC(pio_mask, "PIO modes supported, mode 0 only by default (param valid only for non DT users)");

non-DT

...

> +/**
> + * struct pata_platform_priv - Private info
> + * @io_res: Resource representing I/O base
> + * @ctl_res: Resource representing CTL base

> + * @irq_res: Resource representing IRQ and its flags

Why do we need to keep entire resource for just one value?

> + * @ioport_shift: I/O port shift
> + * @pio_mask: PIO mask
> + * @use16bit: Flag to indicate 16-bit IO instead of 32-bit
> + */

...

>         ata_port_desc(ap, "%s cmd 0x%llx ctl 0x%llx", mmio ? "mmio" : "ioport",
> -                     (unsigned long long)io_res->start,
> -                     (unsigned long long)ctl_res->start);
> +                     (unsigned long long)priv->io_res->start,
> +                     (unsigned long long)priv->ctl_res->start);

Using castings here is not fully correct. Instead just use %pR/%pR or
at least %pa.

...

>         irq = platform_get_irq_optional(pdev, 0);
>         if (irq < 0 && irq != -ENXIO)
>                 return irq;
> +

Stray change?

>         if (irq > 0) {
> -               memset(&irq_res, 0x0, sizeof(struct resource));
> -               irq_res.start = irq;
> +               struct resource *irq_res;
> +
> +               irq_res = devm_kzalloc(&pdev->dev, sizeof(*irq_res), GFP_KERNEL);
> +               if (!irq_res)
> +                       return -ENOMEM;
> +
> +               irq_res->start = irq;
> +               priv->irq_res = irq_res;
>         }

...

> +       ret = of_property_read_u32(dn, "pio-mode", &pio_mode);
> +       if (!ret) {
> +               if (pio_mode > 6) {

> +                       dev_err(&ofdev->dev, "invalid pio-mode\n");
> +                       return -EINVAL;

return dev_err_probe(...); ?

> +               }
> +       } else {
> +               dev_info(&ofdev->dev, "pio-mode unspecified, assuming PIO0\n");
> +       }

...

> +       priv->pio_mask = 1 << pio_mode;
> +       priv->pio_mask |= (1 << pio_mode) - 1;

So, pio_mode defines the MSB in the mask, why not to use

 ->pio_mask = GENMASK(pio_mode, 0);

?

...

> +       if ((pdev->num_resources != 3) && (pdev->num_resources != 2)) {
> +               dev_err(&pdev->dev, "invalid number of resources\n");
> +               return -EINVAL;

return dev_err_probe(); ?

> +       }

...

> +       if (!dev_of_node(&pdev->dev))
> +               ret = pata_platform_get_pdata(pdev, priv);
> +       else
> +               ret = pata_of_platform_get_pdata(pdev, priv);

What the difference between them? Can't you unify them and leave only
DT related part separately?

...

> +static const struct of_device_id pata_of_platform_match[] = {
> +       { .compatible = "ata-generic", },

> +       { },

No comma for terminator line.

> +};

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 07/10] ata: pata_platform: Merge pata_of_platform into pata_platform

[Lad, Prabhakar]

Hi Andy,

Thank you for the review.

On Sat, Dec 25, 2021 at 5:16 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Sat, Dec 25, 2021 at 3:56 AM Lad Prabhakar
> <prabhakar.mahadev-lad.rj@bp.renesas.com> wrote:
> >
> > Merge the OF pata_of_platform driver into pata_platform.
>
> For the further improvements...
>
> ...
>
> > +MODULE_PARM_DESC(pio_mask, "PIO modes supported, mode 0 only by default (param valid only for non DT users)");
>
> non-DT
>
OK.

> ...
>
> > +/**
> > + * struct pata_platform_priv - Private info
> > + * @io_res: Resource representing I/O base
> > + * @ctl_res: Resource representing CTL base
>
> > + * @irq_res: Resource representing IRQ and its flags
>
> Why do we need to keep entire resource for just one value?
>
Agreed can be dropped.

> > + * @ioport_shift: I/O port shift
> > + * @pio_mask: PIO mask
> > + * @use16bit: Flag to indicate 16-bit IO instead of 32-bit
> > + */
>
> ...
>
> >         ata_port_desc(ap, "%s cmd 0x%llx ctl 0x%llx", mmio ? "mmio" : "ioport",
> > -                     (unsigned long long)io_res->start,
> > -                     (unsigned long long)ctl_res->start);
> > +                     (unsigned long long)priv->io_res->start,
> > +                     (unsigned long long)priv->ctl_res->start);
>
> Using castings here is not fully correct. Instead just use %pR/%pR or
> at least %pa.
>
Ok will use %pa.

> ...
>
> >         irq = platform_get_irq_optional(pdev, 0);
> >         if (irq < 0 && irq != -ENXIO)
> >                 return irq;
> > +
>
> Stray change?
>
My bad.

> >         if (irq > 0) {
> > -               memset(&irq_res, 0x0, sizeof(struct resource));
> > -               irq_res.start = irq;
> > +               struct resource *irq_res;
> > +
> > +               irq_res = devm_kzalloc(&pdev->dev, sizeof(*irq_res), GFP_KERNEL);
> > +               if (!irq_res)
> > +                       return -ENOMEM;
> > +
> > +               irq_res->start = irq;
> > +               priv->irq_res = irq_res;
> >         }
>
> ...
>
> > +       ret = of_property_read_u32(dn, "pio-mode", &pio_mode);
> > +       if (!ret) {
> > +               if (pio_mode > 6) {
>
> > +                       dev_err(&ofdev->dev, "invalid pio-mode\n");
> > +                       return -EINVAL;
>
> return dev_err_probe(...); ?
>
Is it just to reduce the lines?

> > +               }
> > +       } else {
> > +               dev_info(&ofdev->dev, "pio-mode unspecified, assuming PIO0\n");
> > +       }
>
> ...
>
> > +       priv->pio_mask = 1 << pio_mode;
> > +       priv->pio_mask |= (1 << pio_mode) - 1;
>
> So, pio_mode defines the MSB in the mask, why not to use
>
>  ->pio_mask = GENMASK(pio_mode, 0);
>
> ?
>
patch 10/10 doesn this.
> ...
>
> > +       if ((pdev->num_resources != 3) && (pdev->num_resources != 2)) {
> > +               dev_err(&pdev->dev, "invalid number of resources\n");
> > +               return -EINVAL;
>
> return dev_err_probe(); ?
>
This is the old code, later patch drops this chunk anyway.

> > +       }
>
> ...
>
> > +       if (!dev_of_node(&pdev->dev))
> > +               ret = pata_platform_get_pdata(pdev, priv);
> > +       else
> > +               ret = pata_of_platform_get_pdata(pdev, priv);
>
> What the difference between them? Can't you unify them and leave only
> DT related part separately?
>
pata_of_platform_get_pdata() basically reads OF data, and there is a
function which is already shared by both the functions.

> ...
>
> > +static const struct of_device_id pata_of_platform_match[] = {
> > +       { .compatible = "ata-generic", },
>
> > +       { },
>
> No comma for terminator line.
>
OK, I will drop it.

Cheers,
Prabhakar

Re: [PATCH v3 07/10] ata: pata_platform: Merge pata_of_platform into pata_platform

[Andy Shevchenko]

On Sat, Dec 25, 2021 at 7:25 PM Lad, Prabhakar
<prabhakar.csengg@gmail.com> wrote:
> On Sat, Dec 25, 2021 at 5:16 PM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> > On Sat, Dec 25, 2021 at 3:56 AM Lad Prabhakar
> > <prabhakar.mahadev-lad.rj@bp.renesas.com> wrote:

> > For the further improvements...

As above, it means that I understand that you simply integrate an old
code, so consider additional changes on top of it.

...

> > >         ata_port_desc(ap, "%s cmd 0x%llx ctl 0x%llx", mmio ? "mmio" : "ioport",
> > > -                     (unsigned long long)io_res->start,
> > > -                     (unsigned long long)ctl_res->start);
> > > +                     (unsigned long long)priv->io_res->start,
> > > +                     (unsigned long long)priv->ctl_res->start);
> >
> > Using castings here is not fully correct. Instead just use %pR/%pr or
> > at least %pa.
> >
> Ok will use %pa.

Perhaps %pR?

...

> > > +               if (pio_mode > 6) {
> >
> > > +                       dev_err(&ofdev->dev, "invalid pio-mode\n");
> > > +                       return -EINVAL;
> >
> > return dev_err_probe(...); ?
> >
> Is it just to reduce the lines?

Yes, a lot of LOCs if being used in all suitable cases.

> > > +               }

...

> > > +       if (!dev_of_node(&pdev->dev))

Just noticed, why not use positive conditional?

> > > +               ret = pata_platform_get_pdata(pdev, priv);
> > > +       else
> > > +               ret = pata_of_platform_get_pdata(pdev, priv);
> >
> > What the difference between them? Can't you unify them and leave only
> > DT related part separately?
> >
> pata_of_platform_get_pdata() basically reads OF data, and there is a
> function which is already shared by both the functions.

Yeah, but my question is why do you need separate functions?
Also, can the driver be converted to use device property API and
eventually get rid of legacy platform data?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 02/10] ata: pata_platform: Drop use of unlikely() in pata_platform_probe

[Damien Le Moal]

On 12/25/21 03:02, Lad, Prabhakar wrote:
> Hi Sergey,
> 
> Thank you for the review.
> 
> On Fri, Dec 24, 2021 at 5:54 PM Sergey Shtylyov <s.shtylyov@omp.ru> wrote:
>>
>> On 12/24/21 4:12 PM, Lad Prabhakar wrote:
>>
>>> pata_platform_probe() isn't a hotpath, which makes it's questionable to
>>> use unlikely(). Therefore let's simply drop it.
>>>
>>> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
>>> ---
>>> v2-->v3
>>> * New patch
>>> ---
>>>  drivers/ata/pata_platform.c | 4 ++--
>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/ata/pata_platform.c b/drivers/ata/pata_platform.c
>>> index cb3134bf88eb..29902001e223 100644
>>> --- a/drivers/ata/pata_platform.c
>>> +++ b/drivers/ata/pata_platform.c
>>> @@ -199,14 +199,14 @@ static int pata_platform_probe(struct platform_device *pdev)
>>>        * Get the I/O base first
>>>        */
>>>       io_res = platform_get_mem_or_io(pdev, 0);
>>> -     if (unlikely(!io_res))
>>> +     if (!io_res)
>>>               return -EINVAL;
>>>
>>>       /*
>>>        * Then the CTL base
>>>        */
>>>       ctl_res = platform_get_mem_or_io(pdev, 1);
>>> -     if (unlikely(!ctl_res))
>>> +     if (!ctl_res)
>>>               return -EINVAL;
>>
>>    I think you should combine this with patch #1.
>>
> I'd like to keep the changes separate from patch #1, as it's unrelated.

But your patch 1 adds the unlikely... So simply do not add it in patch
one and this patch is not necessary anymore.

> 
> Cheers,
> Prabhakar


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v3 02/10] ata: pata_platform: Drop use of unlikely() in pata_platform_probe

[Lad, Prabhakar]

Hi Damien,

On Sun, Dec 26, 2021 at 11:56 AM Damien Le Moal
<damien.lemoal@opensource.wdc.com> wrote:
>
> On 12/25/21 03:02, Lad, Prabhakar wrote:
> > Hi Sergey,
> >
> > Thank you for the review.
> >
> > On Fri, Dec 24, 2021 at 5:54 PM Sergey Shtylyov <s.shtylyov@omp.ru> wrote:
> >>
> >> On 12/24/21 4:12 PM, Lad Prabhakar wrote:
> >>
> >>> pata_platform_probe() isn't a hotpath, which makes it's questionable to
> >>> use unlikely(). Therefore let's simply drop it.
> >>>
> >>> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> >>> ---
> >>> v2-->v3
> >>> * New patch
> >>> ---
> >>>  drivers/ata/pata_platform.c | 4 ++--
> >>>  1 file changed, 2 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/drivers/ata/pata_platform.c b/drivers/ata/pata_platform.c
> >>> index cb3134bf88eb..29902001e223 100644
> >>> --- a/drivers/ata/pata_platform.c
> >>> +++ b/drivers/ata/pata_platform.c
> >>> @@ -199,14 +199,14 @@ static int pata_platform_probe(struct platform_device *pdev)
> >>>        * Get the I/O base first
> >>>        */
> >>>       io_res = platform_get_mem_or_io(pdev, 0);
> >>> -     if (unlikely(!io_res))
> >>> +     if (!io_res)
> >>>               return -EINVAL;
> >>>
> >>>       /*
> >>>        * Then the CTL base
> >>>        */
> >>>       ctl_res = platform_get_mem_or_io(pdev, 1);
> >>> -     if (unlikely(!ctl_res))
> >>> +     if (!ctl_res)
> >>>               return -EINVAL;
> >>
> >>    I think you should combine this with patch #1.
> >>
> > I'd like to keep the changes separate from patch #1, as it's unrelated.
>
> But your patch 1 adds the unlikely... So simply do not add it in patch
> one and this patch is not necessary anymore.
>
patch #1 just replaces two platform_get_resource() with one
platform_get_mem_or_io() call, the unlikely() is just indented towards
the left. But anyway I can merge this into #1.

Are you OK with the rest of the patches?

Cheers,
Prabhakar

Re: [PATCH v3 03/10] ata: pata_of_platform: Use platform_get_irq_optional() to get the interrupt

[Sergey Shtylyov]

Hello!

On 12/24/21 4:12 PM, Lad Prabhakar wrote:

> platform_get_resource(pdev, IORESOURCE_IRQ, ..) relies on static
> allocation of IRQ resources in DT core code, this causes an issue
> when using hierarchical interrupt domains using "interrupts" property
> in the node as this bypasses the hierarchical setup and messes up the
> irq chaining.
> 
> In preparation for removal of static setup of IRQ resource from DT core
> code use platform_get_irq_optional().
> 
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> ---
> v2-->v3
> * New patch
> ---
>  drivers/ata/pata_of_platform.c | 14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/ata/pata_of_platform.c b/drivers/ata/pata_of_platform.c
> index 35aa158fc976..2e2ec7d77726 100644
> --- a/drivers/ata/pata_of_platform.c
> +++ b/drivers/ata/pata_of_platform.c
> @@ -25,11 +25,12 @@ static int pata_of_platform_probe(struct platform_device *ofdev)
>  	struct device_node *dn = ofdev->dev.of_node;
>  	struct resource io_res;
>  	struct resource ctl_res;
> -	struct resource *irq_res;
> +	struct resource irq_res;
>  	unsigned int reg_shift = 0;
>  	int pio_mode = 0;
>  	int pio_mask;
>  	bool use16bit;
> +	int irq;
>  
>  	ret = of_address_to_resource(dn, 0, &io_res);
>  	if (ret) {
> @@ -45,7 +46,14 @@ static int pata_of_platform_probe(struct platform_device *ofdev)
>  		return -EINVAL;
>  	}
>  
> -	irq_res = platform_get_resource(ofdev, IORESOURCE_IRQ, 0);
> +	irq = platform_get_irq_optional(ofdev, 0);
> +	if (irq < 0 && irq != -ENXIO)
> +		return irq;
> +
> +	if (irq > 0) {
> +		memset(&irq_res, 0x0, sizeof(struct resource));
> +		irq_res.start = irq;

   Is that really enough? Doesn't __pata_platform_probe() use irq_res->flags?

[...]

MBR, Sergey

Re: [PATCH v3 04/10] ata: pata_platform: Use platform_get_irq_optional() to get the interrupt

[Sergey Shtylyov]

On 12/24/21 4:12 PM, Lad Prabhakar wrote:

> To be consistent with pata_of_platform driver use
> platform_get_irq_optional() instead of
> platform_get_resource(pdev, IORESOURCE_IRQ, 0).

   But why can't we be consistent with the unpatched pata_of_platfrom(), and then
convert to platform_get_irq_optional() after merging both drivers?
   I'd like to avoid patching the driver to be gone if possible...

> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
[...]

MBR, Sergey

Re: [PATCH v3 05/10] ata: pata_platform: Drop checking IRQ number

[Sergey Shtylyov]

On 12/24/21 4:12 PM, Lad Prabhakar wrote:

> The callers of __pata_platform_probe() will pass the IRQ resource only for
> valid IRQ's, for invalid IRQ's the IRQ resource will always be NULL. So
> drop checking the IRQ number.
> 
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

Reviewed-by: Sergey Shtylyov <s.shtylyov@omp.ru>

[...]

MBR, Sergey

Re: [PATCH v3 06/10] ata: pata_of_platform: Make use of platform_get_mem_or_io()

[Sergey Shtylyov]

On 12/24/21 4:12 PM, Lad Prabhakar wrote:

> To be consistent with pata_platform driver use
> platform_get_mem_or_io() instead of of_address_to_resource().
> 
> Suggested-by: Sergey Shtylyov <s.shtylyov@omp.ru>
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

Reviewed-by: Sergey Shtylyov <s.shtylyov@omp.ru>

[...]

MBR, Sergey

Re: [PATCH v3 07/10] ata: pata_platform: Merge pata_of_platform into pata_platform

[Sergey Shtylyov]

On 12/24/21 4:12 PM, Lad Prabhakar wrote:

> Merge the OF pata_of_platform driver into pata_platform.
> 
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

[...]
> diff --git a/drivers/ata/pata_platform.c b/drivers/ata/pata_platform.c
> index f500f631f72b..4273f1a9abd2 100644
> --- a/drivers/ata/pata_platform.c
> +++ b/drivers/ata/pata_platform.c
[...]
>  	/*
>  	 * And the IRQ
>  	 */
> -	if (irq_res) {
> -		irq = irq_res->start;
> -		irq_flags = (irq_res->flags & IRQF_TRIGGER_MASK) | IRQF_SHARED;
> +	if (priv->irq_res && priv->irq_res->start > 0) {

    I thought you've just dropped the > 0 check?

[...]
> @@ -168,46 +177,34 @@ int __pata_platform_probe(struct device *dev, struct resource *io_res,
>  
>  	ap->ioaddr.altstatus_addr = ap->ioaddr.ctl_addr;
>  
> -	pata_platform_setup_port(&ap->ioaddr, ioport_shift);
> +	pata_platform_setup_port(&ap->ioaddr, priv->ioport_shift);
>  
>  	ata_port_desc(ap, "%s cmd 0x%llx ctl 0x%llx", mmio ? "mmio" : "ioport",
> -		      (unsigned long long)io_res->start,
> -		      (unsigned long long)ctl_res->start);
> +		      (unsigned long long)priv->io_res->start,
> +		      (unsigned long long)priv->ctl_res->start);

   As Andy has noted, these could be converted to %pR (but only after this patch)...

[...]
> @@ -216,21 +213,108 @@ static int pata_platform_probe(struct platform_device *pdev)
>  	irq = platform_get_irq_optional(pdev, 0);
>  	if (irq < 0 && irq != -ENXIO)
>  		return irq;
> +
>  	if (irq > 0) {
> -		memset(&irq_res, 0x0, sizeof(struct resource));
> -		irq_res.start = irq;
> +		struct resource *irq_res;
> +
> +		irq_res = devm_kzalloc(&pdev->dev, sizeof(*irq_res), GFP_KERNEL);
> +		if (!irq_res)
> +			return -ENOMEM;
> +
> +		irq_res->start = irq;
> +		priv->irq_res = irq_res;

   Is that enough? Flags not needed?

[...]
> +static int pata_of_platform_get_pdata(struct platform_device *ofdev,
> +				      struct pata_platform_priv *priv)

   Can I suggest another name, like pata_platform_parse_dt()?

[...]
> +static int pata_platform_get_pdata(struct platform_device *pdev,
> +				   struct pata_platform_priv *priv)

   Can I suggest another name, like pata_platform_init_pdata()?

> +{
> +	struct pata_platform_info *pp_info = dev_get_platdata(&pdev->dev);
> +	int ret;
> +
> +	/*
> +	 * Simple resource validation ..
> +	 */
> +	if ((pdev->num_resources != 3) && (pdev->num_resources != 2)) {
> +		dev_err(&pdev->dev, "invalid number of resources\n");
> +		return -EINVAL;
> +	}

   Hm, do we really need this check? If we drop it beforehand, we could call
pata_platform_get_resources() only once (and expand it inline?).

> +
> +	ret = pata_platform_get_resources(pdev, priv);
> +	if (ret)
> +		return ret;
> +
> +	priv->ioport_shift = pp_info ? pp_info->ioport_shift : 0;
> +	priv->pio_mask = pio_mask;
> +	priv->use16bit = false;
> +
> +	return 0;
> +}
> +
[...]

MBR, Sergey

Re: [PATCH v3 08/10] ata: pata_platform: Drop validating num_resources count

[Sergey Shtylyov]

Hm, this ended up in my spam folder... :-(

On 12/24/21 4:12 PM, Lad Prabhakar wrote:

> Drop validating num_resources count as pata_platform_get_resources()
> already does this check for us.
> 
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
[...]

   Good patch but should have been placed before patch #7...

Reviewed-by: Sergey Shtylyov <s.shtylyov@omp.ru>

MBR, Sergey

Re: [PATCH v3 09/10] ata: pata_platform: Sort the #includes alphabetically

[Sergey Shtylyov]

On 12/24/21 4:12 PM, Lad Prabhakar wrote:

> Sort the #includes alphabetically.
> 
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
[...]

Reviewed-by: Sergey Shtylyov <s.shtylyov@omp.ru>

MBR, Sergey

Re: [PATCH v3 07/10] ata: pata_platform: Merge pata_of_platform into pata_platform

[Sergey Shtylyov]

Hello!

On 12/25/21 8:16 PM, Andy Shevchenko wrote:

[...]
> ...
> 
>> +       if ((pdev->num_resources != 3) && (pdev->num_resources != 2)) {
>> +               dev_err(&pdev->dev, "invalid number of resources\n");
>> +               return -EINVAL;
> 
> return dev_err_probe(); ?
> 
>> +       }
> 
> ...
> 
>> +       if (!dev_of_node(&pdev->dev))
>> +               ret = pata_platform_get_pdata(pdev, priv);
>> +       else
>> +               ret = pata_of_platform_get_pdata(pdev, priv);
> 
> What the difference between them?

   One parses DT props into the private structure, the other inits this structure without DT...

> Can't you unify them and leave only
> DT related part separately?

   He can't -- grep *defconfig for PATA_PLATFORM=, please.

[...]

MBR, Sergey

Re: [PATCH v3 07/10] ata: pata_platform: Merge pata_of_platform into pata_platform

[Sergey Shtylyov]

On 12/27/21 11:54 PM, Sergey Shtylyov wrote:

[...]
>> ...
>>
>>> +       if ((pdev->num_resources != 3) && (pdev->num_resources != 2)) {
>>> +               dev_err(&pdev->dev, "invalid number of resources\n");
>>> +               return -EINVAL;
>>
>> return dev_err_probe(); ?
>>
>>> +       }
>>
>> ...
>>
>>> +       if (!dev_of_node(&pdev->dev))
>>> +               ret = pata_platform_get_pdata(pdev, priv);
>>> +       else
>>> +               ret = pata_of_platform_get_pdata(pdev, priv);
>>
>> What the difference between them?
> 
>    One parses DT props into the private structure, the other inits this structure without DT...
> 
>> Can't you unify them and leave only
>> DT related part separately?
> 
>    He can't -- grep *defconfig for PATA_PLATFORM=, please.

   I take it back -- I think I misunderstood. :-)

> [...]

MBR, Sergey

Re: [PATCH v3 04/10] ata: pata_platform: Use platform_get_irq_optional() to get the interrupt

[Sergey Shtylyov]

On 27.12.2021 22:58, Sergey Shtylyov wrote:

>> To be consistent with pata_of_platform driver use
>> platform_get_irq_optional() instead of
>> platform_get_resource(pdev, IORESOURCE_IRQ, 0).
> 
>     But why can't we be consistent with the unpatched pata_of_platfrom(), and then

    Sorry, pata_of_platform.

> convert to platform_get_irq_optional() after merging both drivers?
>     I'd like to avoid patching the driver to be gone if possible...
> 
>> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> [...]

MBR, Sergey

Re: [PATCH v3 04/10] ata: pata_platform: Use platform_get_irq_optional() to get the interrupt

[Lad, Prabhakar]

Hi Sergey,

Thank you for the review.

On Mon, Dec 27, 2021 at 7:58 PM Sergey Shtylyov <s.shtylyov@omp.ru> wrote:
>
> On 12/24/21 4:12 PM, Lad Prabhakar wrote:
>
> > To be consistent with pata_of_platform driver use
> > platform_get_irq_optional() instead of
> > platform_get_resource(pdev, IORESOURCE_IRQ, 0).
>
>    But why can't we be consistent with the unpatched pata_of_platfrom(), and then
> convert to platform_get_irq_optional() after merging both drivers?
>    I'd like to avoid patching the driver to be gone if possible...
>
Basically to have members of struct pata_platform_priv{} in one shot,
instead of changing them again and again. btw you are OK with patching
for 06/10.

Cheers,
Prabhakar