Message-ID: <CAK8P3a1TkKtG3E3U0xxpJrf=qph0dkn27Apd99EaHMAPOfPtpw@mail.gmail.com>
Date:   Thu, 21 Mar 2019 09:53:14 +0100
From:   Arnd Bergmann <arnd@...db.de>
To:     "Enrico Weigelt, metux IT consult" <lkml@...ux.net>
Cc:     Morris Ku <saumah@...il.com>, gregkh <gregkh@...uxfoundation.org>,
        morris_ku@...ix.com,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] Respond:add support for SUNIX Multi-I/O board

On Wed, Mar 20, 2019 at 7:24 PM Enrico Weigelt, metux IT consult
<lkml@...ux.net> wrote:
> On 15.03.19 11:06, Morris Ku wrote:
> > +Oh, is there really a hard restriction on the max number of boards ?
> > +
> > +driver support maximum 4 boards can be installed incombination
> > +(up to 32 serial port and 2 parallel port)
>
> Could somebody install more than 4 boards in a machine ?
>
> Is there any problem w/ having this data allocated per board,
> associating it to the corresponding struct device instance ?
>
> You do know that - when using probe'able bus'es (eg. PCI) - the kernel
> automatically creates device instances (per card) for you ? You just
> have to register a driver w/ a table of the supported device IDs, and
> the kernel will instantiate a device for you, finally calling the
> probe() function to do the actual initialization.
>
> For PCI, an easily understandable example could be pata_cmd640.c.
> The other bus'es work in a similar way, even platform devices (those
> which usually aren't on a probe'able bus) when using oftree or acpi.

I think serial port drivers typically have a limit on the number of
ports, so we likely need the same here. However the number of
boards should not be limited in the mfd driver that just exports
the serial ports to the serial driver.

Similarly, there should be no limit for the parallel port driver and
the gpio driver.

> > +> +static int snx_ser_port_total_cnt;
> > +> +static int snx_par_port_total_cnt;
> > +> +
> > +> +int snx_board_count;
> > +> +
> > +> +static struct snx_ser_driver sunix_ser_reg = {
> > +> +     .dev_name = "ttySNX",
> > +> +     .major = 0,
> > +> +     .minor = 0,
> > +> +     .nr = (SNX_SER_TOTAL_MAX + 1),
> > +> +};
> > +
> > +can't this be const ?
> > +
> > i prefer keep it in current format.
>
> Why ? do you ever have to write into this struct ?

I think this will just go away after the rewrite into a proper
layered driver, so it doesn't matter.

> > +<snip>
> > +
> > +> +     if ((sb->par_port > 0) && (sb->par_isr != NULL)) {
> > +> +             pp = &sunix_par_table[sb->par_port_index];
> > +> +
> > +> +             if (!pp)
> > +> +                     status = 1;
> > +> +
> > +> +             status = sb->par_isr(sb, pp);
> > +> +     }
> > +> +
> > +> +     if (status != 0)
> > +> +             return handled;
> > +> +
> > +> +     handled = IRQ_HANDLED;
> > +> +     return handled;
> > +> +}
> > +
> > +btw: is there only one irq per board or one per port ?
> > +
> > only one irq per board.
>
> That is very unfortunate. if there was one per port, you could register
> one handler per port and let the kernel directly route to the right port
> instance. (see above: let it directly pass the pointer to the port data)
>
> Are there other devices on the board that share the same irq ?

The best way I can think of to do this would be to have a nested
irqchip in the mfd driver that handles the per-board interrupt and
provides a unique irq number to the individual port drivers.
This makes a very simple irq handler in the port driver, and keeps
the multiplexing where it belongs.

> > +if it already is a global, why not statically initialized ?
> > +Why not doing this in snx_pci_probe() on per-board basis, and let device
> > +registration be done by pci subsystem ?
> > +
> > i prefer keep it in current format.
>
> See above: when probing on per board-basis, the pci subsystem already
> handles most of the stuff for you, and the driver becomes smaller and
> easier to understand.

Agreed, this is only correct way to do it.

       Arnd

Powered by blists - more mailing lists