----- Original Message -----
> From: "Andy Shevchenko" <andriy.shevchenko@xxxxxxxxxxxxxxx>
> To: "Aaron Sierra" <asierra@xxxxxxxxxxx>
> Sent: Tuesday, July 24, 2018 4:23:17 AM
> On Mon, 2018-07-23 at 10:03 -0500, Aaron Sierra wrote:
>
>> > > /* Clear all PCI interrupts by reading INT0. No effect on
>> > > IIR
>> > > */
>> > > ioread8(priv->virt + UART_EXAR_INT0);
>> >
>> > As I mentioned in previous message, this and below it better to be
>> > readb().
>>
>> Hi Andy,
>>
>> Why better?
>
> Two points:
> - consistency with the existing code
> - no need to check for / use IO when you 100% sure there will be MMIO
> access only
>
> (Note, I put consistency first)
Andy,
I will concede that your consistency argument has merit. However, how
do you propose to change the ioread8() call that's already present? I
would typically shy away from submitting possible "churn" except for
your insistence:
[ start patch ]
commit d477821f0d5b66545a0ec805d0ed69a7a654d581
Author: Aaron Sierra <asierra@xxxxxxxxxxx>
Date: Tue Jul 24 10:02:12 2018 -0500
serial: 8250_exar: Convert ioread8() to readb()
Stick to the traditional MMIO-only accessors used elsewhere in the
driver for visual consistency.
Requested-by: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
Signed-off-by: Aaron Sierra <asierra@xxxxxxxxxxx>
diff --git a/drivers/tty/serial/8250/8250_exar.c b/drivers/tty/serial/8250/8250_exar.c
index 60b2b48..20387f8 100644
--- a/drivers/tty/serial/8250/8250_exar.c
+++ b/drivers/tty/serial/8250/8250_exar.c
@@ -522,7 +522,7 @@ static irqreturn_t exar_misc_handler(int irq, void *data)
struct exar8250 *priv = data;
/* Clear all PCI interrupts by reading INT0. No effect on IIR */
- ioread8(priv->virt + UART_EXAR_INT0);
+ readb(priv->virt + UART_EXAR_INT0);
return IRQ_HANDLED;
[ end patch ]
>> I've been under the impression that ioreadX() are the
>> preferred accessors
>
> Why do we need it? Why it's preferred here?
It's not *needed*, but it is already there and is not harmful.
>> for the general case due to type checking and
>> general utility. They are typically implemented around readX() anyway.
>> The virtual address is mapped from an MMIO BAR, so all accesses will
>> be
>> MMIO in either case.
>>
>> Here's my (possibly dated) primary motivator:
>>
>> https://yarchive.net/comp/linux/io_space_accesses.html
>>
>> Has something changed that I'm not aware of?
>
> From the same link:
>
> --- 8< --- 8< ---
>
> - if your device only supports MMIO, you might as well just use the old
> interfaces.
>
> --- 8< --- 8< ---
>
> See what I wrote above.
Sure, but that's not really a mandate to avoid using them when dealing
with MMIO-only devices. This quote from the very next bullet covers my
stance:
It's also, perhaps, a bit syntactically easier to work with, so some
people might prefer that for that reason.
I prefer the ioread/iowrite accessors for two reasons related to syntax:
* clearer naming of ioread[8,16,32,64] over read[b,w,l,q]
* more "natural" argument ordering of iowriteX over writeY
Regardless, I am willing to acquiesce to your demand. Please review the
patch that I included inline above.
-Aaron S.
>> > > + /* Clear INT0 for Expansion Interface slave ports, too */
>> > > + if (priv->board->num_ports > 8)
>> > > + ioread8(priv->virt + (8 * 0x400) +
>> > > UART_EXAR_INT0);
>> >
>> > 0x2000 is explicitly mentioned in the data sheet, so, I doubt we
>> > need to
>> > describe it as 8 * 0x400.
>>
>> I was conflicted on that, thinking showing the logic behind that value
>> might have some benefit, but I agree that 0x2000 would be more readily
>> identifiable within the manual. I'll follow your recommendation in the
>> version that I submit.
>
> Thanks!
>
> --
> Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
> Intel Finland Oy
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
