Re: [PATCH 2/2] ata: pata_platform: Merge pata_of_platform into pata_platform
From: Sergey Shtylyov <hidden>
Date: 2021-12-17 21:38:31
Also in:
lkml
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 <prabhakar.mahadev-lad.rj@bp.renesas.com>
[...]
quoted hunk ↗ jump to hunk
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... :-/ [...]
quoted hunk ↗ jump to hunk
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
[...]
quoted hunk ↗ jump to hunk
@@ -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?). [...]
quoted hunk ↗ jump to hunk
@@ -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