Hi Sergey,

Thank you for the review.

On Fri, Dec 17, 2021 at 9:38 PM Sergey Shtylyov [off-list ref] wrote:

Hello!

On 12/17/21 5:17 PM, Lad Prabhakar wrote:

quoted

Merge the OF pata_of_platform driver into pata_platform.

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

[...]

quoted

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.

[...]

quoted

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.

[...]

quoted

+/**
+ * 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

quoted

+     struct scsi_host_template *sht;
+     bool use16bit;
+};

 /*
  * Provide our own set_mode() as we don't want to change anything that has

[...]

quoted

@@ -168,23 +180,83 @@ int __pata_platform_probe(struct device *dev, struct resource *io_res,

[...]

quoted

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

quoted

+     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?

[...]

quoted

+     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

[...]

quoted

@@ -198,32 +270,63 @@ static int pata_platform_probe(struct platform_device *pdev)

[...]

quoted

+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

quoted

+     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