Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
Date:   Fri, 11 Mar 2022 09:26:59 -0500
From:   Trevor Woerner <twoerner@gmail.com>
To:     Jiri Slaby <jirislaby@kernel.org>
Cc:     linux-kernel@vger.kernel.org,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        linux-serial@vger.kernel.org
Subject: Re: [PATCH 2/3] serial: 8250_fintek.c: Report chipID
Message-ID: <20220311142659.GA38527@localhost>
References: <20220311070203.18159-2-twoerner@gmail.com>
 <0fb3795e-0ab1-494b-aefd-ccfa78420723@kernel.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <0fb3795e-0ab1-494b-aefd-ccfa78420723@kernel.org>
User-Agent: Mutt/1.10.1 (2018-07-13)
Precedence: bulk

Hi Jiri,

Thanks for your review.

On Fri 2022-03-11 @ 10:21:46 AM, Jiri Slaby wrote:
> On 11. 03. 22, 8:02, Trevor Woerner wrote:
> > Provide some feedback to confirm this driver is enabled, and specify which
> > chip was detected.
> 
> No, we don't do that. In fact, the output is mostly useless as it doesn't
> even tell the user what device this is about.

With this patch, when looking at the kernel's bootup messages one would see:

	[    1.809223] Serial: 8250/16550 driver, 5 ports, IRQ sharing enabled
	[    1.814420] Fintek F81865
	[    1.815783] 00:04: ttyS1 at I/O 0x2f8 (irq = 3, base_baud = 115200) is a 16550A
	[    1.822078] Fintek F81865
	[    1.823453] 00:05: ttyS2 at I/O 0x3e8 (irq = 10, base_baud = 115200) is a 16550A
	[    1.829813] Fintek F81865
	[    1.831194] 00:06: ttyS3 at I/O 0x2e8 (irq = 10, base_baud = 115200) is a 16550A
	[    1.837556] Fintek F81865
	[    1.838943] 00:07: ttyS4 at I/O 0x2c0 (irq = 10, base_baud = 115200) is a 16550A
	[    1.845350] Fintek F81865
	[    1.846703] 00:0c: ttyS0 at I/O 0x3f8 (irq = 4, base_baud = 115200) is a 16550A

…so the grouping would hopefully be a clue, and anyone looking up what a
fintek f81865 is would see that it's a SuperIO chip with 6 UARTs.

Ideally that information would all be on one line for each port. The first
line is what I've added, which specifies the actual device, and the second
line comes from serial_core.c's uart_report_port(). If there were a way to
pass the fintek information to serial_core, or a way to turn off serial_core's
uart_report_port() so I could report it in the fintek driver that would be
better, but neither of those exist.

If I want to know if a kernel has support for a device on my board I would
normally grep the dmesg output and look for a product id, a chip id, or a
manufacturer's name. I thought it was strange that with this driver enabled,
there's no hint that the kernel knows anything about it.

> > Signed-off-by: Trevor Woerner <twoerner@gmail.com>
> > ---
> >   drivers/tty/serial/8250/8250_fintek.c | 11 +++++++++++
> >   1 file changed, 11 insertions(+)
> > 
> > diff --git a/drivers/tty/serial/8250/8250_fintek.c b/drivers/tty/serial/8250/8250_fintek.c
> > index d9f0e546b1a1..03ad2354d808 100644
> > --- a/drivers/tty/serial/8250/8250_fintek.c
> > +++ b/drivers/tty/serial/8250/8250_fintek.c
> > @@ -155,11 +155,22 @@ static int fintek_8250_check_id(struct fintek_8250 *pdata)
> >   	switch (chip) {
> >   	case CHIP_ID_F81865:
> > +		pr_info("Fintek F81865\n");
> > +		break;
> >   	case CHIP_ID_F81866:
> > +		pr_info("Fintek F81866\n");
> > +		break;
> >   	case CHIP_ID_F81966:
> > +		pr_info("Fintek F81966\n");
> > +		break;
> >   	case CHIP_ID_F81216AD:
> > +		pr_info("Fintek F81216AD\n");
> > +		break;
> >   	case CHIP_ID_F81216H:
> > +		pr_info("Fintek F81216H\n");
> > +		break;
> >   	case CHIP_ID_F81216:
> > +		pr_info("Fintek F81216\n");
> >   		break;
> >   	default:
> >   		return -ENODEV;
> 
> thanks,
> -- 
> js