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> <CA+V-a8smDqOfC8xxscuhDvicUk=Xf=29-KQS5CGKr3qgb8-1LA@mail.gmail.com>
In-Reply-To: <CA+V-a8smDqOfC8xxscuhDvicUk=Xf=29-KQS5CGKr3qgb8-1LA@mail.gmail.com>
From:   Andy Shevchenko <andy.shevchenko@gmail.com>
Date:   Sat, 25 Dec 2021 19:37:58 +0200
Message-ID: <CAHp75Veb5H0mpfq8R5OeOTPvqqMinkEhnzmJWJ=W5Lk_2U+h=A@mail.gmail.com>
Subject: Re: [PATCH v3 07/10] ata: pata_platform: Merge pata_of_platform into pata_platform
To:     "Lad, Prabhakar" <prabhakar.csengg@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

On Sat, Dec 25, 2021 at 7:25 PM Lad, Prabhakar
<prabhakar.csengg@gmail.com> wrote:
> 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:

> > For the further improvements...

As above, it means that I understand that you simply integrate an old
code, so consider additional changes on top of it.

...

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

Perhaps %pR?

...

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

Yes, a lot of LOCs if being used in all suitable cases.

> > > +               }

...

> > > +       if (!dev_of_node(&pdev->dev))

Just noticed, why not use positive conditional?

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

Yeah, but my question is why do you need separate functions?
Also, can the driver be converted to use device property API and
eventually get rid of legacy platform data?

-- 
With Best Regards,
Andy Shevchenko