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

[Damien Le Moal <dlemoal@kernel.org>]

On 2026/10/22 11:17, Daniel Palmer wrote:
> Hi Damien,
> 
> On Thu, 22 Oct 2026 at 16:01, Damien Le Moal <dlemoal@kernel.org> 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.

Later is fine.

> 
> Cheers,
> 
> Daniel


-- 
Damien Le Moal
Western Digital Research