MIME-Version: 1.0
References: <20211224131300.18198-1-prabhakar.mahadev-lad.rj@bp.renesas.com> <20211224131300.18198-8-prabhakar.mahadev-lad.rj@bp.renesas.com>
In-Reply-To: <20211224131300.18198-8-prabhakar.mahadev-lad.rj@bp.renesas.com>
From:   Andy Shevchenko <andy.shevchenko@gmail.com>
Date:   Sat, 25 Dec 2021 19:16:13 +0200
Message-ID: <CAHp75VcgknapFkZx5YhNhUgaHKGHBXdeTZ+pBcP_G9wnwfFSnw@mail.gmail.com>
Subject: Re: [PATCH v3 07/10] ata: pata_platform: Merge pata_of_platform into pata_platform
To:     Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
Cc:     Damien Le Moal <damien.lemoal@opensource.wdc.com>,
        Sergey Shtylyov <s.shtylyov@omp.ru>,
        Rob Herring <robh+dt@kernel.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Prabhakar <prabhakar.csengg@gmail.com>, linux-ide@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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