Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.32 as permitted sender) client-ip=23.128.96.32;
Feedback-ID: i36794607:Fastmail
User-Agent: Cyrus-JMAP/3.9.0-alpha0-745-g95dd7bea33-fm-20230905.001-g95dd7bea
Mime-Version: 1.0
Message-Id: <83c58cdc-b96a-40c3-bed1-767e9e832893@app.fastmail.com>
In-Reply-To: <79be8582-2395-42c7-92ae-29d9e4c4c33b@mev.co.uk>
References: <20230913112032.90618-1-abbotti@mev.co.uk>
 <20230913112032.90618-10-abbotti@mev.co.uk>
 <ca073163-d947-42f4-b45a-3ea75ab2f7fc@app.fastmail.com>
 <79be8582-2395-42c7-92ae-29d9e4c4c33b@mev.co.uk>
Date:   Wed, 13 Sep 2023 20:01:40 +0200
From:   "Arnd Bergmann" <arnd@kernel.org>
To:     "Ian Abbott" <abbotti@mev.co.uk>, linux-kernel@vger.kernel.org
Cc:     "Niklas Schnelle" <schnelle@linux.ibm.com>
Subject: Re: [PATCH 09/13] comedi: ni_mio_common: Conditionally use I/O port or MMIO
Content-Type: text/plain
Precedence: bulk

On Wed, Sep 13, 2023, at 17:45, Ian Abbott wrote:
> On 13/09/2023 13:17, Arnd Bergmann wrote:
>> On Wed, Sep 13, 2023, at 13:20, Ian Abbott wrote:
>>>
>>> The changes make it possible to build the ni_pcimio module even if the
>>> port I/O functions have not been declared.  (The ni_atmio and ni_mio_cs
>>> modules do still require the port I/O functions to be declared.)
>> 
>> I think this all works, but there is probably a simpler way to
>> achieve the same:
>>   
>>> +#ifdef PCIDMA
>>> +
>>>   static void ni_writel(struct comedi_device *dev, unsigned int data, int reg)
>>>   {
>>> -	if (dev->mmio)
>>> -		writel(data, dev->mmio + reg);
>>> -	else
>>> -		outl(data, dev->iobase + reg);
>>> +	writel(data, dev->mmio + reg);
>>>   }
>>>
>>>
>>> +#else /* PCIDMA */
>>> +
>>> +static void ni_writel(struct comedi_device *dev, unsigned int data, int reg)
>>> +{
>>> +	outl(data, dev->iobase + reg);
>>> +}
>> 
>> We already have an abstraction for this using iowrite32(),
>> which turns into either writel() or outl() depending on the
>> argument, so you could just use pci_iomap() or ioport_map()
>> to turn port numbers into tokens suitable for the common
>> helper.
>
> It would affect three modules, and there are similar changes that could 
> be made to other modules, so I think I'll put that suggestion on hold 
> for now.  I'd also like to get rid of the '#include "ni_mio_common.c"' 
> nonsense at some point, but that's a big job!

Sure, that's fine, I just thought I'd mention it in case you
were looking for a nicer implementation. I wouldn't have done
the current series to start with since there is not a huge amount
of value in running these drivers on obscure architectures
without HAS_IOPORT, but your changes are otherwise completely
sensible.

  Arnd