Re: [PATCH 1/2] ata: pata_artop: read the ATP865 mode register through pci_iomap

[Daniel Palmer]

Hi Damien,

On Thu, 22 Oct 2026 at 16:01, Damien Le Moal <dlemoal@xxxxxxxxxx> wrote:
> > +     case 2: {       /* 6280 or 6280 + fast */
> > +             void __iomem *bmdma = pci_iomap(pdev, 4, 1);
> > +             u8 cfg;
> > +
> > +             if (!bmdma)
> > +                     return -ENOMEM;
> > +             cfg = ioread8(bmdma);
> > +             pci_iounmap(pdev, bmdma);
> > +             if (cfg & 0x10)
> >                       ppi[0] = &info_628x_fast;
> >               else
> >                       ppi[0] = &info_628x;
> >               break;
> >       }
> > +     }
>
> This is really ugly... Please move the "case 2" variable declarations to the top
> of the function so that you do not have to do something like this.

That's fair. I'll fix that up.

> Of note also is that this switch does not have a "default" case, which is really
> bad practice, even though in this case, we should never hit unknown values.
> But I would still prefer seeing something like:
>
>         default:
>                 pr_warn("Invalid driver data\n");
>                 pci_disable_device(pdev);
>                 return -ENODEV;
>         }

Can we fix that later or should I roll it into this patch? Being an
old driver I think it probably has a bunch of preexisting style issues
that need to be fixed but that seems like a separate issue from making
it functional again.

Cheers,

Daniel