linux-kernel - Re: [PATCH v3 07/10] ata: pata_platform: Merge pata_of_platform into pata

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Sat, 25 Dec 2021 19:37:58 +0200
From:   Andy Shevchenko <andy.shevchenko@...il.com>
To:     "Lad, Prabhakar" <prabhakar.csengg@...il.com>
Cc:     Lad Prabhakar <prabhakar.mahadev-lad.rj@...renesas.com>,
        Damien Le Moal <damien.lemoal@...nsource.wdc.com>,
        Sergey Shtylyov <s.shtylyov@....ru>,
        Rob Herring <robh+dt@...nel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        "open list:LIBATA SUBSYSTEM (Serial and Parallel ATA drivers)" 
        <linux-ide@...r.kernel.org>
Subject: Re: [PATCH v3 07/10] ata: pata_platform: Merge pata_of_platform into pata_platform

On Sat, Dec 25, 2021 at 7:25 PM Lad, Prabhakar
<prabhakar.csengg@...il.com> wrote:
> On Sat, Dec 25, 2021 at 5:16 PM Andy Shevchenko
> <andy.shevchenko@...il.com> wrote:
> > On Sat, Dec 25, 2021 at 3:56 AM Lad Prabhakar
> > <prabhakar.mahadev-lad.rj@...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