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> <CAHp75VcgknapFkZx5YhNhUgaHKGHBXdeTZ+pBcP_G9wnwfFSnw@mail.gmail.com>
In-Reply-To: <CAHp75VcgknapFkZx5YhNhUgaHKGHBXdeTZ+pBcP_G9wnwfFSnw@mail.gmail.com>
From:   "Lad, Prabhakar" <prabhakar.csengg@gmail.com>
Date:   Sat, 25 Dec 2021 17:25:12 +0000
Message-ID: <CA+V-a8smDqOfC8xxscuhDvicUk=Xf=29-KQS5CGKr3qgb8-1LA@mail.gmail.com>
Subject: Re: [PATCH v3 07/10] ata: pata_platform: Merge pata_of_platform into pata_platform
To:     Andy Shevchenko <andy.shevchenko@gmail.com>
Cc:     Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>,
        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>,
        "open list:LIBATA SUBSYSTEM (Serial and Parallel ATA drivers)" 
        <linux-ide@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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