Received: by 2002:a05:6a10:22f:0:0:0:0 with SMTP id 15csp1451489pxk; Thu, 10 Sep 2020 16:07:50 -0700 (PDT) X-Google-Smtp-Source: ABdhPJzCqO8YuoDMx+GLb9nGZ8osj0uuhi8VQxG+wR04RGc2zxLBoWvTHfgDE0KnKBt00DxKUIHw X-Received: by 2002:a05:6402:2c3:: with SMTP id b3mr11927064edx.213.1599779270268; Thu, 10 Sep 2020 16:07:50 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1599779270; cv=none; d=google.com; s=arc-20160816; b=xP8DeQ3SFs83S16HIyercLaOKUDnTgtMuYC19q5k7xAUTlbcCVldYb9GeV/Jqu4nmZ ekt0NRzZQ+PfbSAXisR2N4GIBz/FeO6jdd86u2rZ8/v3XKqFwBGMmxbeE3nvIpFObTL6 WkvFiNeBKTZBBsLPVNFVluLtq/5E79zXhtPI0SM9yhr0Xr+jZTyNDIecLJ43kA6suKbp rjJnjllOcPqopwJTgwShpASwEiKv0JdZk4qkWXDjS6QW5WdBnHnWaKb4uqRxs8nqdjCg ZuhI49sSTDQgaKpqooA9COkkgotyigK5WzRv3iRNydVhluL0SUheWzE6R3bRFmgv3tRN tcSw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:sender:mime-version:references:message-id :in-reply-to:subject:cc:to:from:date; bh=dZkLesexf/hvwKhaTpnNBIQbzZxpEtEF9KpHtgaVUjc=; b=DR0yA93J96FYRdb9MUsUeLzAyLGkPzldvGN6BO/7XhDkZABgESHIMDngyhbUt38l16 GSXqEzZ5rki/ZVSw+9il7+EjOtKeHhxY6a89QbxITDUOHrYeYDYFp+qCU4RWSJbkmull 9kxHG8VBM8jkGm3wmEwK+QmIusBncNo/RAvu1tOcPEMOqnq+UEwEHpV6XAxaUDX4wHCh OqWMyMxKQPb/nE1biEfEPkd3GYCz1DecSfXLBJ4AYl9HtZr4BouflrGg97rwUEzzuSsN v/Yf43A9DnZnnYebnTX1Arj6/0JdHb/Ps434qPuLn2ErxFkJ5pA3zIg6uCdzxvp5aIMW lF0Q== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Return-Path: Received: from vger.kernel.org (vger.kernel.org. [23.128.96.18]) by mx.google.com with ESMTP id 7si71479edx.162.2020.09.10.16.07.26; Thu, 10 Sep 2020 16:07:50 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18; Authentication-Results: mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1725768AbgIJXFZ (ORCPT + 99 others); Thu, 10 Sep 2020 19:05:25 -0400 Received: from kvm5.telegraphics.com.au ([98.124.60.144]:43682 "EHLO kvm5.telegraphics.com.au" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1725294AbgIJXFX (ORCPT ); Thu, 10 Sep 2020 19:05:23 -0400 Received: from localhost (localhost.localdomain [127.0.0.1]) by kvm5.telegraphics.com.au (Postfix) with ESMTP id 2132F2A937; Thu, 10 Sep 2020 19:05:17 -0400 (EDT) Date: Fri, 11 Sep 2020 09:05:21 +1000 (AEST) From: Finn Thain To: Geert Uytterhoeven cc: "David S. Miller" , Bartlomiej Zolnierkiewicz , Joshua Thompson , linux-m68k , Linux Kernel Mailing List , linux-ide@vger.kernel.org Subject: Re: [PATCH] ide/macide: Convert Mac IDE driver to platform driver In-Reply-To: Message-ID: References: <00ee44fe6ecdce1c783c3cc3b1b9a62b498dcdb2.1597736545.git.fthain@telegraphics.com.au> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, 10 Sep 2020, Geert Uytterhoeven wrote: > On Thu, Sep 10, 2020 at 2:23 AM Finn Thain wrote: > > I prefer a declarative or data-driven style, even if it takes a few > > more lines of code. But there is a compromise: > > > > static const struct resource mac_ide_quadra_rsrc[] __initconst = { > > DEFINE_RES_MEM(0x50F1A000, 0x104), > > DEFINE_RES_IRQ(IRQ_NUBUS_F), > > } > > > > static const struct resource mac_ide_pb_rsrc[] __initconst = { > > DEFINE_RES_MEM(0x50F1A000, 0x104), > > DEFINE_RES_IRQ(IRQ_NUBUS_C), > > } > > > > The reason I didn't use these macros was to avoid making the reader go and > > look up their definitions. Anyway, would that style be preferred here? > > I think the DEFINE_RES_*() are sufficiently common (well, in pre-DT > platforms ;-) > OK, I'll use the macros in v2. > > I could do the same with the mac_ide_baboon_rsrc[] initializer: > > > > static const struct resource mac_pata_baboon_rsrc[] __initconst = { > > DEFINE_RES_MEM(0x50F1A000, 0x38), > > DEFINE_RES_MEM(0x50F1A038, 0x04), > > DEFINE_RES_IRQ(IRQ_BABOON_1), > > }; > > > > ... but that would lose the IORESOURCE_IRQ_SHAREABLE flag. I'm not sure > > whether that matters (it's a vestige of macide.c). > > You can still use DEFINE_RES_NAMED() to pass the flags. > Would you consider that to be a good compromise? > Sure. I was happy with v1. I'm not that worried about brevity. Anyway, the question remains, is the flag actually needed. I can't see a need for it so I'll omit the flag in v2 and ask Stan to test again. > > > > +static const struct resource mac_pata_baboon_rsrc[] __initconst = { > > > > + { > > > > + .flags = IORESOURCE_MEM, > > > > + .start = 0x50F1A000, > > > > + .end = 0x50F1A037, > > > > + }, { > > > > + .flags = IORESOURCE_MEM, > > > > + .start = 0x50F1A038, > > > > + .end = 0x50F1A03B, > > > > + }, { > > > > + .flags = IORESOURCE_IRQ | IORESOURCE_IRQ_SHAREABLE, > > > > + .start = IRQ_BABOON_1, > > > > + .end = IRQ_BABOON_1, > > > > + }, > > > > +}; > > > > + > > > > +static const struct pata_platform_info mac_pata_baboon_data __initconst = { > > > > + .ioport_shift = 2, > > > > +}; > > > > > > Just wondering: how is this implemented in drivers/ide/macide.c, which > > > doesn't use the platform info? > > > > That factor of 4 is embedded in the address caclulation: > > > > for (i = 0; i < 8; i++) > > hw->io_ports_array[i] = base + i * 4; > > IC. But in the new code, the platform info is passed for Baboon only, > while the old code used it for all variants. > Correct. Is that a problem for some reason? > > > > --- a/drivers/ide/macide.c > > > > +++ b/drivers/ide/macide.c > > > > @@ -18,10 +18,11 @@ > > > > #include > > > > #include > > > > #include > > > > +#include > > > > > > > > #include > > > > -#include > > > > -#include > > > > + > > > > +#define DRV_NAME "mac_ide" > > > > > > > > #define IDE_BASE 0x50F1A000 /* Base address of IDE controller */ > > > > > > Do you still need this definition? > > > Yes, because it's still used to access IDE_IFR. > > > Ideally, that should be converted to use the base from the resource, > > > too. > > > > > > > Yes, that was my thought too. I can make the change if you like, but I > > can't test it until I set up the appropriate hardware (MAC_IDE_QUADRA or > > MAC_IDE_PB). I do own that hardware but it is located in Melbourne and it > > is now illegal to visit Melbourne without official papers. Besides, once I > > can test on that hardware I can replace the entire driver anyway, and > > this kind of refactoring would become moot. > > OK. > > > > > @@ -109,42 +110,65 @@ static const char *mac_ide_name[] = > > > > * Probe for a Macintosh IDE interface > > > > */ > > > > > > > > -static int __init macide_init(void) > > > > +static int mac_ide_probe(struct platform_device *pdev) > > > > { > > > > - unsigned long base; > > > > - int irq; > > > > + struct resource *mem, *irq; > > > > struct ide_hw hw, *hws[] = { &hw }; > > > > struct ide_port_info d = macide_port_info; > > > > + struct ide_host *host; > > > > + int rc; > > > > > > > > if (!MACH_IS_MAC) > > > > return -ENODEV; > > > > > > > > - switch (macintosh_config->ide_type) { > > > > - case MAC_IDE_QUADRA: > > > > - base = IDE_BASE; > > > > - irq = IRQ_NUBUS_F; > > > > - break; > > > > - case MAC_IDE_PB: > > > > - base = IDE_BASE; > > > > - irq = IRQ_NUBUS_C; > > > > - break; > > > > - case MAC_IDE_BABOON: > > > > - base = BABOON_BASE; > > > > - d.port_ops = NULL; > > > > > > How does the driver know not to use the special port_ops after > > > this change? > > > > > > > The driver always uses the special port_ops after this change because it > > no longer handles the MAC_IDE_BABOON case. That case is handled by either > > non-MAC_IDE_BABOON case? > > > drivers/ata/pata_platform.c or drivers/ide/ide_platform.c, depending on > > .config. > > Ideally, we do need to differentiate, right? > Sorry, I'm lost. The commit log explains that the macide driver is only intended to support two of the three variants, because the generic drivers are already able to support the third variant (Baboon). Stan confirmed this on his PB 190. But, IIUC, you seem to want macide to continue to support all three variants (?) The existing code to implement that has an old bug that reassigns d.port_ops when it is const. IMO, the const is correct because macide should concern itself with mac hardware quirks and should not try to mimic a generic driver by setting d.port_ops = NULL. Does that make sense? > Gr{oetje,eeting}s, > > Geert > >