Re: [PATCH 10/10] net: New ax88796 platform driver for Amiga X-Surf 100 Zorro board (m68k)

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Geert,

thanks for your suggestions!

On Wed, Apr 18, 2018 at 1:53 AM, Geert Uytterhoeven
<geert@xxxxxxxxxxxxxx> wrote:
Hi Michael,

Thanks for your patch!

On Tue, Apr 17, 2018 at 12:04 AM, Michael Schmitz <schmitzmic@xxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxxxxxxxxxxx>

Missing "From: Michael Karcher ..."?

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


Signed-off-by: Michael Schmitz <schmitzmic@xxxxxxxxx>

--- 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@xxxxxxxxxxxxxx

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
--
To unsubscribe from this list: send the line "unsubscribe linux-m68k" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Video for Linux]     [Yosemite News]     [Linux S/390]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux