Message-ID: <CAOmrzkJVPn+zpaJTGhCpc=sSSZhBtbO_2BTGaVDpmNVm+VzAEg@mail.gmail.com>
Date:   Wed, 18 Apr 2018 10:35:43 +1200
From:   Michael Schmitz <schmitzmic@...il.com>
To:     Geert Uytterhoeven <geert@...ux-m68k.org>
Cc:     netdev <netdev@...r.kernel.org>,
        "Linux/m68k" <linux-m68k@...r.kernel.org>,
        Michael Karcher <Michael.Karcher@...berlin.de>,
        Michael Karcher <kernel@...rcher.dialup.fu-berlin.de>
Subject: Re: [PATCH 10/10] net: New ax88796 platform driver for Amiga X-Surf
 100 Zorro board (m68k)

Hi Geert,

thanks for your suggestions!

On Wed, Apr 18, 2018 at 1:53 AM, Geert Uytterhoeven
<geert@...ux-m68k.org> wrote:
> Hi Michael,
>
> Thanks for your patch!
>
> On Tue, Apr 17, 2018 at 12:04 AM, Michael Schmitz <schmitzmic@...il.com> wrote:
>> Add platform device driver to populate the ax88796 platform data from
>> information provided by the XSurf100 zorro device driver.
>> This driver will have to be loaded before loading the ax88796 module,
>> or compiled as built-in.
>
> Is that really true? The platform device should be probed when both the
> device and driver have been registered, but order shouldn't matter.

Loading the xsurf100 module will pull in the ax88796 module, so order
does not matter. I'll drop that.

>
>> Signed-off-by: Michael Karcher <kernel@...rcher.dialup.fu-berlin.de>
>
> Missing "From: Michael Karcher ..."?

Fixed the authorship now - probably got mangled when squashing in my
local edits.

>
>> Signed-off-by: Michael Schmitz <schmitzmic@...il.com>
>
>> --- a/drivers/net/ethernet/8390/Kconfig
>> +++ b/drivers/net/ethernet/8390/Kconfig
>> @@ -30,7 +30,7 @@ config PCMCIA_AXNET
>>
>>  config AX88796
>>         tristate "ASIX AX88796 NE2000 clone support"
>> -       depends on (ARM || MIPS || SUPERH)
>> +       depends on (ARM || MIPS || SUPERH || AMIGA)
>
> s/AMIGA/ZORRO/, for consistency with the below.

Will do.

>
>>         select CRC32
>>         select PHYLIB
>>         select MDIO_BITBANG
>> @@ -45,6 +45,18 @@ config AX88796_93CX6
>>         ---help---
>>           Select this if your platform comes with an external 93CX6 eeprom.
>>
>> +config XSURF100
>> +       tristate "Amiga XSurf 100 AX88796/NE2000 clone support"
>> +       depends on ZORRO
>> +       depends on AX88796
>
> It's a bit unfortunate the user has to enable _two_ config options to enable
> this driver.
>
> I see two solutions for that:
>
> 1) Hide the XSURF100 symbol, so it gets enabled automatically if AX88796 is
>    enabled on a Zorro bus system:
>
>     config XSURF100
>             tristate
>             depends on ZORRO
>             default AX88796
>
> 2) Hide the AX88796 symbol, and let it be selected by XSURF100:
>
>     config AX88796
>             tristate "ASIX AX88796 NE2000 clone support" if !ZORRO
>             depends on ARM || MIPS || SUPERH || ZORRO
>             ...
>
>     config XSURF100
>             tristate "Amiga XSurf 100 AX88796/NE2000 clone support"
>             depends on ZORRO
>             select AX88796

I'll use the latter -

>> --- /dev/null
>> +++ b/drivers/net/ethernet/8390/xsurf100.c
>> @@ -0,0 +1,411 @@
>> +#include <linux/module.h>
>> +#include <linux/netdevice.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/zorro.h>
>> +#include <net/ax88796.h>
>> +#include <asm/amigaints.h>
>> +
>> +#define ZORRO_PROD_INDIVIDUAL_COMPUTERS_X_SURF100 \
>> +               ZORRO_ID(INDIVIDUAL_COMPUTERS, 0x64, 0)
>
> Another long define to get rid of? ;-)
>
>> +/* Hard reset the card. This used to pause for the same period that a
>> + * 8390 reset command required, but that shouldn't be necessary.
>> + */
>> +static void ax_reset_8390(struct net_device *dev)
>> +{
>> +       struct ei_device *ei_local = netdev_priv(dev);
>> +       unsigned long reset_start_time = jiffies;
>> +       void __iomem *addr = (void __iomem *)dev->base_addr;
>> +
>> +       netif_dbg(ei_local, hw, dev, "resetting the 8390 t=%ld...\n", jiffies);
>> +
>> +       ei_outb(ei_inb(addr + NE_RESET), addr + NE_RESET);
>> +
>> +       ei_local->txing = 0;
>> +       ei_local->dmaing = 0;
>> +
>> +       /* This check _should_not_ be necessary, omit eventually. */
>> +       while ((ei_inb(addr + EN0_ISR) & ENISR_RESET) == 0) {
>> +               if (time_after(jiffies, reset_start_time + 2 * HZ / 100)) {
>> +                       netdev_warn(dev, "%s: did not complete.\n", __func__);
>> +                       break;
>> +               }
>
> cpu_relax()?
>
> How long does this usually take? If > 1 ms, you can use e.g. msleep(1)
> instead of cpu_relax().

No idea how long this will take - the reset function is lifted
straight out of ax88796.c with no modifications whatsoever.

Come to think of it - it's exported as ei_local->reset_8390 there, so
there is no good reason for even duplicating the code that I can see.
I'lll drop it.

>
>> +       }
>> +
>> +       ei_outb(ENISR_RESET, addr + EN0_ISR);   /* Ack intr. */
>> +}
>
>> +       if (ei_local->dmaing) {
>> +               netdev_err(dev,
>> +                          "DMAing conflict in %s "
>> +                          "[DMAstat:%d][irqlock:%d].\n",
>
> Please don't split error messages, as that makes it more difficult to
> grep for them.

Again, found like that in ax88796.c. Will fix here (and eventually in
ax88796.c).

>> +                          __func__,
>> +                          ei_local->dmaing, ei_local->irqlock);
>> +               return;
>
>> +static int xsurf100_probe(struct zorro_dev *zdev,
>> +                         const struct zorro_device_id *ent)
>> +{
>
>> +       /* error handling for ioremap regs */
>> +       if (!ax88796_data.base_regs) {
>> +               dev_err(&zdev->dev, "Cannot ioremap area %p (registers)\n",
>> +                       (void *)zdev->resource.start);
>
> Please use %pR to format struct resource.
> Documentation/core-api/printk-formats.rst

The driver uses ioremap to map two subsections of the mem resource for
two different purposes - control register access, and ring buffer
access. The output of %pR may be misleading here (wrong size), and
even more so below.

>
>> +       /* error handling for ioremap data */
>> +       if (!ax88796_data.data_area) {
>> +               dev_err(&zdev->dev, "Cannot ioremap area %p (32-bit access)\n",
>> +                       (void *)zdev->resource.start + XS100_8390_DATA32_BASE);
>
> %pR

I've added the offset into the mem resource here to clarify what we've
tried to map.

>
>> +static void xsurf100_remove(struct zorro_dev *zdev)
>> +{
>> +       struct platform_device *pdev;
>> +       struct xsurf100_ax_plat_data *xs100;
>> +
>> +       pdev = zorro_get_drvdata(zdev);
>> +       xs100 = dev_get_platdata(&pdev->dev);
>
> struct platform_device *pdev = pdev = zorro_get_drvdata(zdev);
> struct xsurf100_ax_plat_data *xs100 = dev_get_platdata(&pdev->dev);

Of course.

Cheers,

  Michael


>
> Gr{oetje,eeting}s,
>
>                         Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@...ux-m68k.org
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds

Powered by blists - more mailing lists