Hi All,
This patch series aims to merge pata_of_platform into pata_platform
driver.
Note:
* The patches have been build tested only.
* Other thing I'm concerned about is the Copyright message which
existed in pata_of_platform.c should that be pasted in pata_platform.c?
Cheers,
Prabhakar
Lad Prabhakar (2):
ata: pata_platform: make use of platform_get_mem_or_io()
ata: pata_platform: Merge pata_of_platform into pata_platform
drivers/ata/Kconfig | 3 +-
drivers/ata/Makefile | 1 -
drivers/ata/pata_of_platform.c | 90 --------------
drivers/ata/pata_platform.c | 209 ++++++++++++++++++++++++---------
include/linux/ata_platform.h | 9 --
5 files changed, 155 insertions(+), 157 deletions(-)
delete mode 100644 drivers/ata/pata_of_platform.c
--
2.17.1
Make use of platform_get_mem_or_io() to simplify the code.
Signed-off-by: Lad Prabhakar <[email protected]>
---
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
Merge the OF pata_of_platform driver into pata_platform.
Signed-off-by: Lad Prabhakar <[email protected]>
---
drivers/ata/Kconfig | 3 +-
drivers/ata/Makefile | 1 -
drivers/ata/pata_of_platform.c | 90 ---------------
drivers/ata/pata_platform.c | 199 +++++++++++++++++++++++++--------
include/linux/ata_platform.h | 9 --
5 files changed, 153 insertions(+), 149 deletions(-)
delete mode 100644 drivers/ata/pata_of_platform.c
diff --git a/drivers/ata/Kconfig b/drivers/ata/Kconfig
index a7da8ea7b3ed..0fab5cae45d5 100644
--- a/drivers/ata/Kconfig
+++ b/drivers/ata/Kconfig
@@ -1122,7 +1122,8 @@ config PATA_PLATFORM
config PATA_OF_PLATFORM
tristate "OpenFirmware platform device PATA support"
- depends on PATA_PLATFORM && OF
+ depends on OF
+ select PATA_PLATFORM
help
This option enables support for generic directly connected ATA
devices commonly found on embedded systems with OpenFirmware
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 35aa158fc976..000000000000
--- a/drivers/ata/pata_of_platform.c
+++ /dev/null
@@ -1,90 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0-only
-/*
- * OF-platform PATA driver
- *
- * Copyright (c) 2007 MontaVista Software, Inc.
- * Anton Vorontsov <[email protected]>
- */
-
-#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>
-
-#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)
-{
- int ret;
- 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;
-
- ret = of_address_to_resource(dn, 0, &io_res);
- if (ret) {
- 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) {
- dev_err(&ofdev->dev, "can't get CTL address from "
- "device tree\n");
- return -EINVAL;
- }
-
- irq_res = platform_get_resource(ofdev, IORESOURCE_IRQ, 0);
-
- of_property_read_u32(dn, "reg-shift", ®_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_res,
- 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 <[email protected]>");
-MODULE_LICENSE("GPL");
diff --git a/drivers/ata/pata_platform.c b/drivers/ata/pata_platform.c
index cb3134bf88eb..b8d8d51bc562 100644
--- a/drivers/ata/pata_platform.c
+++ b/drivers/ata/pata_platform.c
@@ -11,21 +11,42 @@
* 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/of_address.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"
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
+ * @mask: PIO mask
+ * @sht: scsi_host_template to use when registering
+ * @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 mask;
+ struct scsi_host_template *sht;
+ bool use16bit;
+};
/*
* Provide our own set_mode() as we don't want to change anything that has
@@ -66,15 +87,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 +109,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 +120,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_res->start > 0) {
- 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 +143,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->mask;
ap->flags |= ATA_FLAG_SLAVE_POSS;
/*
@@ -151,15 +163,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,23 +180,83 @@ 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, priv->sht);
}
-EXPORT_SYMBOL_GPL(__pata_platform_probe);
-static int pata_platform_probe(struct platform_device *pdev)
+static int pata_of_platform_get_pdata(struct platform_device *ofdev,
+ struct pata_platform_priv *priv)
{
- struct resource *io_res;
+ struct device_node *dn = ofdev->dev.of_node;
struct resource *ctl_res;
struct resource *irq_res;
+ struct resource *io_res;
+ int pio_mode = 0;
+ int irq;
+ int ret;
+
+ ctl_res = devm_kzalloc(&ofdev->dev, sizeof(*ctl_res), GFP_KERNEL);
+ io_res = devm_kzalloc(&ofdev->dev, sizeof(*io_res), GFP_KERNEL);
+ irq_res = devm_kzalloc(&ofdev->dev, sizeof(*irq_res), GFP_KERNEL);
+ if (!ctl_res || !io_res || !irq_res)
+ return -ENOMEM;
+
+ ret = of_address_to_resource(dn, 0, io_res);
+ if (ret) {
+ dev_err(&ofdev->dev, "can't get IO address from device tree\n");
+ return -EINVAL;
+ }
+ priv->io_res = io_res;
+
+ ret = of_address_to_resource(dn, 1, ctl_res);
+ if (ret) {
+ dev_err(&ofdev->dev, "can't get CTL address from device tree\n");
+ return -EINVAL;
+ }
+ priv->ctl_res = ctl_res;
+
+ irq = platform_get_irq_optional(ofdev, 0);
+ if (irq <= 0 && irq != -ENXIO)
+ return irq ? irq : -ENXIO;
+
+ if (irq > 0) {
+ irq_res->start = irq;
+ irq_res->end = irq;
+ irq_res->flags = IORESOURCE_IRQ | irq_get_trigger_type(irq);
+ priv->irq_res = irq_res;
+ } else {
+ devm_kfree(&ofdev->dev, io_res);
+ }
+
+ of_property_read_u32(dn, "reg-shift", &priv->ioport_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");
+ }
+
+ priv->use16bit = of_property_read_bool(dn, "ata-generic,use16bit");
+
+ priv->mask = 1 << pio_mode;
+ priv->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);
/*
@@ -198,32 +270,63 @@ 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))
+ priv->io_res = platform_get_mem_or_io(pdev, 0);
+ if (unlikely(!priv->io_res))
return -EINVAL;
/*
* Then the CTL base
*/
- ctl_res = platform_get_mem_or_io(pdev, 1);
- if (unlikely(!ctl_res))
+ priv->ctl_res = platform_get_mem_or_io(pdev, 1);
+ if (unlikely(!priv->ctl_res))
return -EINVAL;
/*
* And the IRQ
*/
- irq_res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
+ priv->irq_res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
+
+ priv->ioport_shift = pp_info ? pp_info->ioport_shift : 0;
+ priv->mask = pio_mask;
+ priv->use16bit = false;
- return __pata_platform_probe(&pdev->dev, io_res, ctl_res, irq_res,
- pp_info ? pp_info->ioport_shift : 0,
- pio_mask, &pata_platform_sht, 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;
+
+ priv->sht = &pata_platform_sht;
+
+ 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
On Fri, Dec 17, 2021 at 8:17 AM Lad Prabhakar
<[email protected]> wrote:
>
> Merge the OF pata_of_platform driver into pata_platform.
>
> Signed-off-by: Lad Prabhakar <[email protected]>
> ---
> drivers/ata/Kconfig | 3 +-
> drivers/ata/Makefile | 1 -
> drivers/ata/pata_of_platform.c | 90 ---------------
> drivers/ata/pata_platform.c | 199 +++++++++++++++++++++++++--------
> include/linux/ata_platform.h | 9 --
> 5 files changed, 153 insertions(+), 149 deletions(-)
> delete mode 100644 drivers/ata/pata_of_platform.c
>
> diff --git a/drivers/ata/Kconfig b/drivers/ata/Kconfig
> index a7da8ea7b3ed..0fab5cae45d5 100644
> --- a/drivers/ata/Kconfig
> +++ b/drivers/ata/Kconfig
> @@ -1122,7 +1122,8 @@ config PATA_PLATFORM
>
> config PATA_OF_PLATFORM
> tristate "OpenFirmware platform device PATA support"
> - depends on PATA_PLATFORM && OF
> + depends on OF
> + select PATA_PLATFORM
Can't we just kill the kconfig option?
> help
> This option enables support for generic directly connected ATA
> devices commonly found on embedded systems with OpenFirmware
> diff --git a/drivers/ata/pata_platform.c b/drivers/ata/pata_platform.c
> index cb3134bf88eb..b8d8d51bc562 100644
> --- a/drivers/ata/pata_platform.c
> +++ b/drivers/ata/pata_platform.c
> @@ -11,21 +11,42 @@
> * 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/of_address.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"
>
> 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
> + * @mask: PIO mask
> + * @sht: scsi_host_template to use when registering
> + * @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 mask;
> + struct scsi_host_template *sht;
> + bool use16bit;
> +};
>
> /*
> * Provide our own set_mode() as we don't want to change anything that has
> @@ -66,15 +87,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 +109,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 +120,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_res->start > 0) {
> - 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 +143,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->mask;
> ap->flags |= ATA_FLAG_SLAVE_POSS;
>
> /*
> @@ -151,15 +163,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,23 +180,83 @@ 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, priv->sht);
> }
> -EXPORT_SYMBOL_GPL(__pata_platform_probe);
>
> -static int pata_platform_probe(struct platform_device *pdev)
> +static int pata_of_platform_get_pdata(struct platform_device *ofdev,
> + struct pata_platform_priv *priv)
> {
> - struct resource *io_res;
> + struct device_node *dn = ofdev->dev.of_node;
> struct resource *ctl_res;
> struct resource *irq_res;
> + struct resource *io_res;
> + int pio_mode = 0;
> + int irq;
> + int ret;
> +
> + ctl_res = devm_kzalloc(&ofdev->dev, sizeof(*ctl_res), GFP_KERNEL);
> + io_res = devm_kzalloc(&ofdev->dev, sizeof(*io_res), GFP_KERNEL);
> + irq_res = devm_kzalloc(&ofdev->dev, sizeof(*irq_res), GFP_KERNEL);
> + if (!ctl_res || !io_res || !irq_res)
> + return -ENOMEM;
> +
> + ret = of_address_to_resource(dn, 0, io_res);
> + if (ret) {
> + dev_err(&ofdev->dev, "can't get IO address from device tree\n");
> + return -EINVAL;
> + }
> + priv->io_res = io_res;
> +
> + ret = of_address_to_resource(dn, 1, ctl_res);
> + if (ret) {
> + dev_err(&ofdev->dev, "can't get CTL address from device tree\n");
> + return -EINVAL;
> + }
> + priv->ctl_res = ctl_res;
> +
> + irq = platform_get_irq_optional(ofdev, 0);
> + if (irq <= 0 && irq != -ENXIO)
> + return irq ? irq : -ENXIO;
> +
> + if (irq > 0) {
> + irq_res->start = irq;
> + irq_res->end = irq;
> + irq_res->flags = IORESOURCE_IRQ | irq_get_trigger_type(irq);
Though of_irq_to_resource() does the same thing, I think the irq code
handles the flags automatically for DT.
> + priv->irq_res = irq_res;
> + } else {
> + devm_kfree(&ofdev->dev, io_res);
Is this supposed to be irq_res? Otherwise, there shouldn't be any need
to free a devres allocation.
> + }
> +
> + of_property_read_u32(dn, "reg-shift", &priv->ioport_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");
> + }
> +
> + priv->use16bit = of_property_read_bool(dn, "ata-generic,use16bit");
> +
> + priv->mask = 1 << pio_mode;
> + priv->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);
>
> /*
> @@ -198,32 +270,63 @@ 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))
> + priv->io_res = platform_get_mem_or_io(pdev, 0);
> + if (unlikely(!priv->io_res))
> return -EINVAL;
>
> /*
> * Then the CTL base
> */
> - ctl_res = platform_get_mem_or_io(pdev, 1);
> - if (unlikely(!ctl_res))
> + priv->ctl_res = platform_get_mem_or_io(pdev, 1);
> + if (unlikely(!priv->ctl_res))
> return -EINVAL;
>
> /*
> * And the IRQ
> */
> - irq_res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
> + priv->irq_res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
> +
> + priv->ioport_shift = pp_info ? pp_info->ioport_shift : 0;
> + priv->mask = pio_mask;
> + priv->use16bit = false;
>
> - return __pata_platform_probe(&pdev->dev, io_res, ctl_res, irq_res,
> - pp_info ? pp_info->ioport_shift : 0,
> - pio_mask, &pata_platform_sht, 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;
> +
> + priv->sht = &pata_platform_sht;
> +
> + 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
>
Hi Rob,
On Fri, Dec 17, 2021 at 2:50 PM Rob Herring <[email protected]> wrote:
>
> On Fri, Dec 17, 2021 at 8:17 AM Lad Prabhakar
> <[email protected]> wrote:
> >
> > Merge the OF pata_of_platform driver into pata_platform.
> >
> > Signed-off-by: Lad Prabhakar <[email protected]>
> > ---
> > drivers/ata/Kconfig | 3 +-
> > drivers/ata/Makefile | 1 -
> > drivers/ata/pata_of_platform.c | 90 ---------------
> > drivers/ata/pata_platform.c | 199 +++++++++++++++++++++++++--------
> > include/linux/ata_platform.h | 9 --
> > 5 files changed, 153 insertions(+), 149 deletions(-)
> > delete mode 100644 drivers/ata/pata_of_platform.c
> >
> > diff --git a/drivers/ata/Kconfig b/drivers/ata/Kconfig
> > index a7da8ea7b3ed..0fab5cae45d5 100644
> > --- a/drivers/ata/Kconfig
> > +++ b/drivers/ata/Kconfig
> > @@ -1122,7 +1122,8 @@ config PATA_PLATFORM
> >
> > config PATA_OF_PLATFORM
> > tristate "OpenFirmware platform device PATA support"
> > - depends on PATA_PLATFORM && OF
> > + depends on OF
> > + select PATA_PLATFORM
>
> Can't we just kill the kconfig option?
>
No, as there are a couple defconfigs which have this option enabled.
So the plan is once this patch is merged for the send in the patches
for defconfig files for next release and a release later we can safely
drop this. So that we don't break anything.
> > help
> > This option enables support for generic directly connected ATA
> > devices commonly found on embedded systems with OpenFirmware
>
>
> > diff --git a/drivers/ata/pata_platform.c b/drivers/ata/pata_platform.c
> > index cb3134bf88eb..b8d8d51bc562 100644
> > --- a/drivers/ata/pata_platform.c
> > +++ b/drivers/ata/pata_platform.c
> > @@ -11,21 +11,42 @@
> > * 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/of_address.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"
> >
> > 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
> > + * @mask: PIO mask
> > + * @sht: scsi_host_template to use when registering
> > + * @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 mask;
> > + struct scsi_host_template *sht;
> > + bool use16bit;
> > +};
> >
> > /*
> > * Provide our own set_mode() as we don't want to change anything that has
> > @@ -66,15 +87,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 +109,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 +120,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_res->start > 0) {
> > - 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 +143,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->mask;
> > ap->flags |= ATA_FLAG_SLAVE_POSS;
> >
> > /*
> > @@ -151,15 +163,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,23 +180,83 @@ 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, priv->sht);
> > }
> > -EXPORT_SYMBOL_GPL(__pata_platform_probe);
> >
> > -static int pata_platform_probe(struct platform_device *pdev)
> > +static int pata_of_platform_get_pdata(struct platform_device *ofdev,
> > + struct pata_platform_priv *priv)
> > {
> > - struct resource *io_res;
> > + struct device_node *dn = ofdev->dev.of_node;
> > struct resource *ctl_res;
> > struct resource *irq_res;
> > + struct resource *io_res;
> > + int pio_mode = 0;
> > + int irq;
> > + int ret;
> > +
> > + ctl_res = devm_kzalloc(&ofdev->dev, sizeof(*ctl_res), GFP_KERNEL);
> > + io_res = devm_kzalloc(&ofdev->dev, sizeof(*io_res), GFP_KERNEL);
> > + irq_res = devm_kzalloc(&ofdev->dev, sizeof(*irq_res), GFP_KERNEL);
> > + if (!ctl_res || !io_res || !irq_res)
> > + return -ENOMEM;
> > +
> > + ret = of_address_to_resource(dn, 0, io_res);
> > + if (ret) {
> > + dev_err(&ofdev->dev, "can't get IO address from device tree\n");
> > + return -EINVAL;
> > + }
> > + priv->io_res = io_res;
> > +
> > + ret = of_address_to_resource(dn, 1, ctl_res);
> > + if (ret) {
> > + dev_err(&ofdev->dev, "can't get CTL address from device tree\n");
> > + return -EINVAL;
> > + }
> > + priv->ctl_res = ctl_res;
> > +
> > + irq = platform_get_irq_optional(ofdev, 0);
> > + if (irq <= 0 && irq != -ENXIO)
> > + return irq ? irq : -ENXIO;
> > +
> > + if (irq > 0) {
> > + irq_res->start = irq;
> > + irq_res->end = irq;
> > + irq_res->flags = IORESOURCE_IRQ | irq_get_trigger_type(irq);
>
> Though of_irq_to_resource() does the same thing, I think the irq code
> handles the flags automatically for DT.
>
sorry you mean to drop the flags?
> > + priv->irq_res = irq_res;
> > + } else {
> > + devm_kfree(&ofdev->dev, io_res);
>
> Is this supposed to be irq_res? Otherwise, there shouldn't be any need
> to free a devres allocation.
>
Oops, yes that should have been irq_res.
Cheers,
Prabhakar
> > + }
> > +
> > + of_property_read_u32(dn, "reg-shift", &priv->ioport_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");
> > + }
> > +
> > + priv->use16bit = of_property_read_bool(dn, "ata-generic,use16bit");
> > +
> > + priv->mask = 1 << pio_mode;
> > + priv->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);
> >
> > /*
> > @@ -198,32 +270,63 @@ 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))
> > + priv->io_res = platform_get_mem_or_io(pdev, 0);
> > + if (unlikely(!priv->io_res))
> > return -EINVAL;
> >
> > /*
> > * Then the CTL base
> > */
> > - ctl_res = platform_get_mem_or_io(pdev, 1);
> > - if (unlikely(!ctl_res))
> > + priv->ctl_res = platform_get_mem_or_io(pdev, 1);
> > + if (unlikely(!priv->ctl_res))
> > return -EINVAL;
> >
> > /*
> > * And the IRQ
> > */
> > - irq_res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
> > + priv->irq_res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
> > +
> > + priv->ioport_shift = pp_info ? pp_info->ioport_shift : 0;
> > + priv->mask = pio_mask;
> > + priv->use16bit = false;
> >
> > - return __pata_platform_probe(&pdev->dev, io_res, ctl_res, irq_res,
> > - pp_info ? pp_info->ioport_shift : 0,
> > - pio_mask, &pata_platform_sht, 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;
> > +
> > + priv->sht = &pata_platform_sht;
> > +
> > + 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
> >
On Fri, Dec 17, 2021 at 10:42 AM Lad, Prabhakar
<[email protected]> wrote:
>
> Hi Rob,
>
> On Fri, Dec 17, 2021 at 2:50 PM Rob Herring <[email protected]> wrote:
> >
> > On Fri, Dec 17, 2021 at 8:17 AM Lad Prabhakar
> > <[email protected]> wrote:
> > >
> > > Merge the OF pata_of_platform driver into pata_platform.
> > >
> > > Signed-off-by: Lad Prabhakar <[email protected]>
> > > ---
> > > drivers/ata/Kconfig | 3 +-
> > > drivers/ata/Makefile | 1 -
> > > drivers/ata/pata_of_platform.c | 90 ---------------
> > > drivers/ata/pata_platform.c | 199 +++++++++++++++++++++++++--------
> > > include/linux/ata_platform.h | 9 --
> > > 5 files changed, 153 insertions(+), 149 deletions(-)
> > > delete mode 100644 drivers/ata/pata_of_platform.c
> > >
> > > diff --git a/drivers/ata/Kconfig b/drivers/ata/Kconfig
> > > index a7da8ea7b3ed..0fab5cae45d5 100644
> > > --- a/drivers/ata/Kconfig
> > > +++ b/drivers/ata/Kconfig
> > > @@ -1122,7 +1122,8 @@ config PATA_PLATFORM
> > >
> > > config PATA_OF_PLATFORM
> > > tristate "OpenFirmware platform device PATA support"
> > > - depends on PATA_PLATFORM && OF
> > > + depends on OF
> > > + select PATA_PLATFORM
> >
> > Can't we just kill the kconfig option?
> >
> No, as there are a couple defconfigs which have this option enabled.
> So the plan is once this patch is merged for the send in the patches
> for defconfig files for next release and a release later we can safely
> drop this. So that we don't break anything.
Wouldn't they all be fine since they already have
CONFIG_PATA_PLATFORM=y as well?
Either way, that's fine.
>
> > > help
> > > This option enables support for generic directly connected ATA
> > > devices commonly found on embedded systems with OpenFirmware
> >
> >
> > > diff --git a/drivers/ata/pata_platform.c b/drivers/ata/pata_platform.c
> > > index cb3134bf88eb..b8d8d51bc562 100644
> > > --- a/drivers/ata/pata_platform.c
> > > +++ b/drivers/ata/pata_platform.c
> > > @@ -11,21 +11,42 @@
> > > * 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/of_address.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"
> > >
> > > 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
> > > + * @mask: PIO mask
> > > + * @sht: scsi_host_template to use when registering
> > > + * @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 mask;
> > > + struct scsi_host_template *sht;
> > > + bool use16bit;
> > > +};
> > >
> > > /*
> > > * Provide our own set_mode() as we don't want to change anything that has
> > > @@ -66,15 +87,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 +109,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 +120,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_res->start > 0) {
> > > - 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 +143,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->mask;
> > > ap->flags |= ATA_FLAG_SLAVE_POSS;
> > >
> > > /*
> > > @@ -151,15 +163,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,23 +180,83 @@ 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, priv->sht);
> > > }
> > > -EXPORT_SYMBOL_GPL(__pata_platform_probe);
> > >
> > > -static int pata_platform_probe(struct platform_device *pdev)
> > > +static int pata_of_platform_get_pdata(struct platform_device *ofdev,
> > > + struct pata_platform_priv *priv)
> > > {
> > > - struct resource *io_res;
> > > + struct device_node *dn = ofdev->dev.of_node;
> > > struct resource *ctl_res;
> > > struct resource *irq_res;
> > > + struct resource *io_res;
> > > + int pio_mode = 0;
> > > + int irq;
> > > + int ret;
> > > +
> > > + ctl_res = devm_kzalloc(&ofdev->dev, sizeof(*ctl_res), GFP_KERNEL);
> > > + io_res = devm_kzalloc(&ofdev->dev, sizeof(*io_res), GFP_KERNEL);
> > > + irq_res = devm_kzalloc(&ofdev->dev, sizeof(*irq_res), GFP_KERNEL);
> > > + if (!ctl_res || !io_res || !irq_res)
> > > + return -ENOMEM;
> > > +
> > > + ret = of_address_to_resource(dn, 0, io_res);
> > > + if (ret) {
> > > + dev_err(&ofdev->dev, "can't get IO address from device tree\n");
> > > + return -EINVAL;
> > > + }
> > > + priv->io_res = io_res;
> > > +
> > > + ret = of_address_to_resource(dn, 1, ctl_res);
> > > + if (ret) {
> > > + dev_err(&ofdev->dev, "can't get CTL address from device tree\n");
> > > + return -EINVAL;
> > > + }
> > > + priv->ctl_res = ctl_res;
> > > +
> > > + irq = platform_get_irq_optional(ofdev, 0);
> > > + if (irq <= 0 && irq != -ENXIO)
> > > + return irq ? irq : -ENXIO;
> > > +
> > > + if (irq > 0) {
> > > + irq_res->start = irq;
> > > + irq_res->end = irq;
> > > + irq_res->flags = IORESOURCE_IRQ | irq_get_trigger_type(irq);
> >
> > Though of_irq_to_resource() does the same thing, I think the irq code
> > handles the flags automatically for DT.
> >
> sorry you mean to drop the flags?
Right.
Rob
On Fri, Dec 17, 2021 at 5:04 PM Rob Herring <[email protected]> wrote:
>
> On Fri, Dec 17, 2021 at 10:42 AM Lad, Prabhakar
> <[email protected]> wrote:
> >
> > Hi Rob,
> >
> > On Fri, Dec 17, 2021 at 2:50 PM Rob Herring <[email protected]> wrote:
> > >
> > > On Fri, Dec 17, 2021 at 8:17 AM Lad Prabhakar
> > > <[email protected]> wrote:
> > > >
> > > > Merge the OF pata_of_platform driver into pata_platform.
> > > >
> > > > Signed-off-by: Lad Prabhakar <[email protected]>
> > > > ---
> > > > drivers/ata/Kconfig | 3 +-
> > > > drivers/ata/Makefile | 1 -
> > > > drivers/ata/pata_of_platform.c | 90 ---------------
> > > > drivers/ata/pata_platform.c | 199 +++++++++++++++++++++++++--------
> > > > include/linux/ata_platform.h | 9 --
> > > > 5 files changed, 153 insertions(+), 149 deletions(-)
> > > > delete mode 100644 drivers/ata/pata_of_platform.c
> > > >
> > > > diff --git a/drivers/ata/Kconfig b/drivers/ata/Kconfig
> > > > index a7da8ea7b3ed..0fab5cae45d5 100644
> > > > --- a/drivers/ata/Kconfig
> > > > +++ b/drivers/ata/Kconfig
> > > > @@ -1122,7 +1122,8 @@ config PATA_PLATFORM
> > > >
> > > > config PATA_OF_PLATFORM
> > > > tristate "OpenFirmware platform device PATA support"
> > > > - depends on PATA_PLATFORM && OF
> > > > + depends on OF
> > > > + select PATA_PLATFORM
> > >
> > > Can't we just kill the kconfig option?
> > >
> > No, as there are a couple defconfigs which have this option enabled.
> > So the plan is once this patch is merged for the send in the patches
> > for defconfig files for next release and a release later we can safely
> > drop this. So that we don't break anything.
>
> Wouldn't they all be fine since they already have
> CONFIG_PATA_PLATFORM=y as well?
>
I didn't realize all those defconfigs already have
CONFIG_PATA_PLATFORM enabled. So in that case PATA_OF_PLATFORM can be
wiped out.
> Either way, that's fine.
>
> >
> > > > help
> > > > This option enables support for generic directly connected ATA
> > > > devices commonly found on embedded systems with OpenFirmware
> > >
> > >
> > > > diff --git a/drivers/ata/pata_platform.c b/drivers/ata/pata_platform.c
> > > > index cb3134bf88eb..b8d8d51bc562 100644
> > > > --- a/drivers/ata/pata_platform.c
> > > > +++ b/drivers/ata/pata_platform.c
> > > > @@ -11,21 +11,42 @@
> > > > * 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/of_address.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"
> > > >
> > > > 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
> > > > + * @mask: PIO mask
> > > > + * @sht: scsi_host_template to use when registering
> > > > + * @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 mask;
> > > > + struct scsi_host_template *sht;
> > > > + bool use16bit;
> > > > +};
> > > >
> > > > /*
> > > > * Provide our own set_mode() as we don't want to change anything that has
> > > > @@ -66,15 +87,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 +109,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 +120,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_res->start > 0) {
> > > > - 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 +143,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->mask;
> > > > ap->flags |= ATA_FLAG_SLAVE_POSS;
> > > >
> > > > /*
> > > > @@ -151,15 +163,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,23 +180,83 @@ 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, priv->sht);
> > > > }
> > > > -EXPORT_SYMBOL_GPL(__pata_platform_probe);
> > > >
> > > > -static int pata_platform_probe(struct platform_device *pdev)
> > > > +static int pata_of_platform_get_pdata(struct platform_device *ofdev,
> > > > + struct pata_platform_priv *priv)
> > > > {
> > > > - struct resource *io_res;
> > > > + struct device_node *dn = ofdev->dev.of_node;
> > > > struct resource *ctl_res;
> > > > struct resource *irq_res;
> > > > + struct resource *io_res;
> > > > + int pio_mode = 0;
> > > > + int irq;
> > > > + int ret;
> > > > +
> > > > + ctl_res = devm_kzalloc(&ofdev->dev, sizeof(*ctl_res), GFP_KERNEL);
> > > > + io_res = devm_kzalloc(&ofdev->dev, sizeof(*io_res), GFP_KERNEL);
> > > > + irq_res = devm_kzalloc(&ofdev->dev, sizeof(*irq_res), GFP_KERNEL);
> > > > + if (!ctl_res || !io_res || !irq_res)
> > > > + return -ENOMEM;
> > > > +
> > > > + ret = of_address_to_resource(dn, 0, io_res);
> > > > + if (ret) {
> > > > + dev_err(&ofdev->dev, "can't get IO address from device tree\n");
> > > > + return -EINVAL;
> > > > + }
> > > > + priv->io_res = io_res;
> > > > +
> > > > + ret = of_address_to_resource(dn, 1, ctl_res);
> > > > + if (ret) {
> > > > + dev_err(&ofdev->dev, "can't get CTL address from device tree\n");
> > > > + return -EINVAL;
> > > > + }
> > > > + priv->ctl_res = ctl_res;
> > > > +
> > > > + irq = platform_get_irq_optional(ofdev, 0);
> > > > + if (irq <= 0 && irq != -ENXIO)
> > > > + return irq ? irq : -ENXIO;
> > > > +
> > > > + if (irq > 0) {
> > > > + irq_res->start = irq;
> > > > + irq_res->end = irq;
> > > > + irq_res->flags = IORESOURCE_IRQ | irq_get_trigger_type(irq);
> > >
> > > Though of_irq_to_resource() does the same thing, I think the irq code
> > > handles the flags automatically for DT.
> > >
> > sorry you mean to drop the flags?
>
> Right.
>
Ok, I will drop it.
Cheers,
Prabhakar
Hello!
On 12/17/21 5:17 PM, Lad Prabhakar wrote:
> Merge the OF pata_of_platform driver into pata_platform.
>
> Signed-off-by: Lad Prabhakar <[email protected]>
[...]
> diff --git a/drivers/ata/Kconfig b/drivers/ata/Kconfig
> index a7da8ea7b3ed..0fab5cae45d5 100644
> --- a/drivers/ata/Kconfig
> +++ b/drivers/ata/Kconfig
> @@ -1122,7 +1122,8 @@ config PATA_PLATFORM
>
> config PATA_OF_PLATFORM
> tristate "OpenFirmware platform device PATA support"
> - depends on PATA_PLATFORM && OF
> + depends on OF
> + select PATA_PLATFORM
> help
> This option enables support for generic directly connected ATA
> devices commonly found on embedded systems with OpenFirmware
Hm, why in the world you're keeping this Konfig entry? You doint even use it
anywhere... :-/
[...]
> diff --git a/drivers/ata/pata_platform.c b/drivers/ata/pata_platform.c
> index cb3134bf88eb..b8d8d51bc562 100644
> --- a/drivers/ata/pata_platform.c
> +++ b/drivers/ata/pata_platform.c
> @@ -11,21 +11,42 @@
> * 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/of_address.h>
> #include <linux/platform_device.h>
> -#include <linux/ata_platform.h>
> +#include <scsi/scsi_host.h>
I'd make the sorting of the #include's a separate patch...
[...]
> +/**
> + * 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
> + * @mask: PIO mask
> + * @sht: scsi_host_template to use when registering
> + * @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 mask;
Why not pio_mask?
> + struct scsi_host_template *sht;
> + bool use16bit;
> +};
>
> /*
> * Provide our own set_mode() as we don't want to change anything that has
[...]
> @@ -168,23 +180,83 @@ int __pata_platform_probe(struct device *dev, struct resource *io_res,
[...]
>
> -static int pata_platform_probe(struct platform_device *pdev)
> +static int pata_of_platform_get_pdata(struct platform_device *ofdev,
> + struct pata_platform_priv *priv)
> {
> - struct resource *io_res;
> + struct device_node *dn = ofdev->dev.of_node;
> struct resource *ctl_res;
> struct resource *irq_res;
> + struct resource *io_res;
Should be declared before ctl_res...
> + int pio_mode = 0;
> + int irq;
> + int ret;
> +
> + ctl_res = devm_kzalloc(&ofdev->dev, sizeof(*ctl_res), GFP_KERNEL);
> + io_res = devm_kzalloc(&ofdev->dev, sizeof(*io_res), GFP_KERNEL);
> + irq_res = devm_kzalloc(&ofdev->dev, sizeof(*irq_res), GFP_KERNEL);
> + if (!ctl_res || !io_res || !irq_res)
> + return -ENOMEM;
Can't we get away from these allocated resources? Or at least irq_res?
[...]
> + priv->use16bit = of_property_read_bool(dn, "ata-generic,use16bit");
> +
> + priv->mask = 1 << pio_mode;
> + priv->mask |= (1 << pio_mode) - 1;
You can make use of GENMASK(pio_mode, 0), in a separate pre-patch (or post-patch?).
[...]
> @@ -198,32 +270,63 @@ static int pata_platform_probe(struct platform_device *pdev)
[...]
> +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);
> +
No need for empty line here...
> + if (ret)
> + return ret;
> +
> + priv->sht = &pata_platform_sht;
Aren't those structures identical between the formerly separate drivers?
[...]
MBR, Sergey
Hi Sergey,
Thank you for the review.
On Fri, Dec 17, 2021 at 9:38 PM Sergey Shtylyov <[email protected]> wrote:
>
> Hello!
>
> On 12/17/21 5:17 PM, Lad Prabhakar wrote:
>
> > Merge the OF pata_of_platform driver into pata_platform.
> >
> > Signed-off-by: Lad Prabhakar <[email protected]>
> [...]
>
> > diff --git a/drivers/ata/Kconfig b/drivers/ata/Kconfig
> > index a7da8ea7b3ed..0fab5cae45d5 100644
> > --- a/drivers/ata/Kconfig
> > +++ b/drivers/ata/Kconfig
> > @@ -1122,7 +1122,8 @@ config PATA_PLATFORM
> >
> > config PATA_OF_PLATFORM
> > tristate "OpenFirmware platform device PATA support"
> > - depends on PATA_PLATFORM && OF
> > + depends on OF
> > + select PATA_PLATFORM
> > help
> > This option enables support for generic directly connected ATA
> > devices commonly found on embedded systems with OpenFirmware
>
> Hm, why in the world you're keeping this Konfig entry? You doint even use it
> anywhere... :-/
>
There are defconfig users of it, but luckily as Rob pointed out they
even have PATA_PLATFORM enabled so will be dropping it.
> [...]
> > diff --git a/drivers/ata/pata_platform.c b/drivers/ata/pata_platform.c
> > index cb3134bf88eb..b8d8d51bc562 100644
> > --- a/drivers/ata/pata_platform.c
> > +++ b/drivers/ata/pata_platform.c
> > @@ -11,21 +11,42 @@
> > * 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/of_address.h>
> > #include <linux/platform_device.h>
> > -#include <linux/ata_platform.h>
> > +#include <scsi/scsi_host.h>
>
> I'd make the sorting of the #include's a separate patch...
>
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
> > + * @ioport_shift: I/O port shift
> > + * @mask: PIO mask
> > + * @sht: scsi_host_template to use when registering
> > + * @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 mask;
>
> Why not pio_mask?
>
OK
> > + struct scsi_host_template *sht;
> > + bool use16bit;
> > +};
> >
> > /*
> > * Provide our own set_mode() as we don't want to change anything that has
> [...]
> > @@ -168,23 +180,83 @@ int __pata_platform_probe(struct device *dev, struct resource *io_res,
> [...]
> >
> > -static int pata_platform_probe(struct platform_device *pdev)
> > +static int pata_of_platform_get_pdata(struct platform_device *ofdev,
> > + struct pata_platform_priv *priv)
> > {
> > - struct resource *io_res;
> > + struct device_node *dn = ofdev->dev.of_node;
> > struct resource *ctl_res;
> > struct resource *irq_res;
> > + struct resource *io_res;
>
> Should be declared before ctl_res...
>
Any reason why?
>
> > + int pio_mode = 0;
> > + int irq;
> > + int ret;
> > +
> > + ctl_res = devm_kzalloc(&ofdev->dev, sizeof(*ctl_res), GFP_KERNEL);
> > + io_res = devm_kzalloc(&ofdev->dev, sizeof(*io_res), GFP_KERNEL);
> > + irq_res = devm_kzalloc(&ofdev->dev, sizeof(*irq_res), GFP_KERNEL);
> > + if (!ctl_res || !io_res || !irq_res)
> > + return -ENOMEM;
>
> Can't we get away from these allocated resources? Or at least irq_res?
>
Do you have any suggestions?
> [...]
> > + priv->use16bit = of_property_read_bool(dn, "ata-generic,use16bit");
> > +
> > + priv->mask = 1 << pio_mode;
> > + priv->mask |= (1 << pio_mode) - 1;
>
> You can make use of GENMASK(pio_mode, 0), in a separate pre-patch (or post-patch?).
>
OK
> [...]
> > @@ -198,32 +270,63 @@ static int pata_platform_probe(struct platform_device *pdev)
> [...]
> > +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);
> > +
>
> No need for empty line here...
>
OK
> > + if (ret)
> > + return ret;
> > +
> > + priv->sht = &pata_platform_sht;
>
> Aren't those structures identical between the formerly separate drivers?
>
Yes so are you suggesting to drop sht from priv and use it directly?
Cheers,
Prabhakar
> [...]
>
> MBR, Sergey
On 18.12.2021 13:51, Lad, Prabhakar wrote:
[...]
>>> Merge the OF pata_of_platform driver into pata_platform.
>>>
>>> Signed-off-by: Lad Prabhakar <[email protected]>
[...]
>>> diff --git a/drivers/ata/pata_platform.c b/drivers/ata/pata_platform.c
>>> index cb3134bf88eb..b8d8d51bc562 100644
>>> --- a/drivers/ata/pata_platform.c
>>> +++ b/drivers/ata/pata_platform.c
[...]
>>> @@ -168,23 +180,83 @@ int __pata_platform_probe(struct device *dev, struct resource *io_res,
>> [...]
>>>
>>> -static int pata_platform_probe(struct platform_device *pdev)
>>> +static int pata_of_platform_get_pdata(struct platform_device *ofdev,
>>> + struct pata_platform_priv *priv)
>>> {
>>> - struct resource *io_res;
>>> + struct device_node *dn = ofdev->dev.of_node;
>>> struct resource *ctl_res;
>>> struct resource *irq_res;
>>> + struct resource *io_res;
>>
>> Should be declared before ctl_res...
>>
> Any reason why?
Well, it's a natural order, following from the driver logic, no?
>>> + int pio_mode = 0;
>>> + int irq;
>>> + int ret;
>>> +
>>> + ctl_res = devm_kzalloc(&ofdev->dev, sizeof(*ctl_res), GFP_KERNEL);
>>> + io_res = devm_kzalloc(&ofdev->dev, sizeof(*io_res), GFP_KERNEL);
>>> + irq_res = devm_kzalloc(&ofdev->dev, sizeof(*irq_res), GFP_KERNEL);
>>> + if (!ctl_res || !io_res || !irq_res)
>>> + return -ENOMEM;
>>
>> Can't we get away from these allocated resources? Or at least irq_res?
>>
> Do you have any suggestions?
Let me look deeper...
[...]
>>> @@ -198,32 +270,63 @@ static int pata_platform_probe(struct platform_device *pdev)
[...]
>>> + if (ret)
>>> + return ret;
>>> +
>>> + priv->sht = &pata_platform_sht;
>>
>> Aren't those structures identical between the formerly separate drivers?
>>
> Yes so are you suggesting to drop sht from priv and use it directly?
Yep.
> Cheers,
> Prabhakar
MBR, Sergey