Re: [PATCH 2/2] drivers/ata: convert pata_falcon to arch platform device

[Michael Schmitz <schmitzmic@gmail.com>]

Hi Geert,

thanks for your feedback!

Am 04.09.2019 um 00:44 schrieb Geert Uytterhoeven:
> Hi Michael,
>
> On Tue, Jul 2, 2019 at 12:02 AM Michael Schmitz <schmitzmic@gmail.com> wrote:
>> The Atari platform device setup now provides a platform device
>> for the Falcon IDE interface. Use this in place of the simple platform
>> device set up in the old pata_falcon probe code.
>>
>> Signed-off-by: Michael Schmitz <schmitzmic@gmail.com>
>
> Thanks for your patch!
>
>> --- a/drivers/ata/pata_falcon.c
>> +++ b/drivers/ata/pata_falcon.c
>> @@ -120,23 +120,21 @@ static int pata_falcon_set_mode(struct ata_link *link,
>>         .set_mode       = pata_falcon_set_mode,
>>  };
>>
>> -static int pata_falcon_init_one(void)
>> +static int __init pata_falcon_init_one(struct platform_device *pdev)
>>  {
>> +       struct resource *res;
>>         struct ata_host *host;
>>         struct ata_port *ap;
>> -       struct platform_device *pdev;
>>         void __iomem *base;
>>
>> -       if (!MACH_IS_ATARI || !ATARIHW_PRESENT(IDE))
>> -               return -ENODEV;
>> -
>> -       pr_info(DRV_NAME ": Atari Falcon PATA controller\n");
>> +       dev_info(&pdev->dev, ": Atari Falcon PATA controller\n");
>>
>> -       pdev = platform_device_register_simple(DRV_NAME, 0, NULL, 0);
>> -       if (IS_ERR(pdev))
>> -               return PTR_ERR(pdev);
>> +       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +       if (!res)
>> +               return -ENODEV;
>>
>> -       if (!devm_request_mem_region(&pdev->dev, ATA_HD_BASE, 0x40, DRV_NAME)) {
>
> ATA_HD_BASE is now unused, and can be removed.

Right - well spotted!
>
>> +       if (!devm_request_mem_region(&pdev->dev, res->start,
>> +                                    resource_size(res), DRV_NAME)) {
>>                 pr_err(DRV_NAME ": resources busy\n");
>
> dev_err(&pdev->dev, "resources busy\n");

I stole that from pata_gayle, Bartlomiej may want to fix it there as well?

>>                 return -EBUSY;
>>         }
>
>> @@ -174,9 +172,26 @@ static int pata_falcon_init_one(void)
>>         return ata_host_activate(host, 0, NULL, 0, &pata_falcon_sht);
>>  }
>>
>> -module_init(pata_falcon_init_one);
>> +static int __exit pata_falcon_remove_one(struct platform_device *pdev)
>> +{
>> +       struct ata_host *host = platform_get_drvdata(pdev);
>> +
>> +       ata_host_detach(host);
>> +
>> +       return 0;
>> +}
>> +
>> +static struct platform_driver pata_falcon_driver = {
>> +       .remove = __exit_p(pata_falcon_remove_one),
>> +       .driver   = {
>> +               .name   = "atari-falcon-ide",
>> +       },
>> +};
>> +
>> +module_platform_driver_probe(pata_falcon_driver, pata_falcon_init_one);
>
> This doesn't seem to work in the builtin case (e.g. atari_defconfig with
> ide replaced by ata): no hard drives are detected.

Due to a dumb naming mismatch between driver and platform code 
(shouldn't have rushed this right before going on leave). This would 
have made the driver fail in the modular case as well.

I'll fix that along with adding some error checking in the Atari 
platform init code.

Regarding the potential bisection issue with this series - that ought to 
be fixed as well by renaming the platform resource to match what the new 
driver expects. I'd rather leave the two patches separate so the Atari 
platform code one can go through your tree.

Cheers,

	Michael

>
> Gr{oetje,eeting}s,
>
>                         Geert
>