On Tue, 2016-03-01 at 00:26 +0300, Sergei Ianovich wrote:
> The patch adds support for 3 additional LP-8x4x built-in serial
> ports.
>
> The device can also host up to 8 extension cards with 4 serial ports
> on each card for a total of 35 ports. However, I don't have
> the hardware to test extension cards, so they are not supported, yet.
>
> Signed-off-by: Sergei Ianovich <[email protected]>
> Reviewed-by: Heikki Krogerus <[email protected]>
Few more comments, mostly about style.
> +static void lp8841_serial_set_termios(struct uart_port *port,
> + struct ktermios *termios, struct ktermios *old)
> +{
> +#ifdef BOTHER
> + unsigned int cbaud;
> +#endif
> + unsigned int baud;
> + unsigned int len;
Since you are writing this to the register at the end maybe
- unsigned int -> u8 (write*b* — exactly one byte)
- len -> value (it's not only about data length)
> + unsigned int old_csize = old ? old->c_cflag & CSIZE : CS8;
> + struct lp8841_serial_data *data = port->private_data;
> +
> + /* We only support CS7 and CS8 */
> + while ((termios->c_cflag & CSIZE) != CS7 &&
> + (termios->c_cflag & CSIZE) != CS8) {
> + termios->c_cflag &= ~CSIZE;
> + termios->c_cflag |= old_csize;
> + old_csize = CS8;
> + }
> +
> + serial8250_do_set_termios(port, termios, old);
> +
> + if ((termios->c_cflag & CSIZE) == CS7)
> + len = 9;
> + else
> + len = 10;
> +
> + if (termios->c_cflag & CSTOPB)
> + len++;
> + if (termios->c_cflag & PARENB)
> + len++;
> + if (!(termios->c_cflag & PARODD))
> + len++;
> +#ifdef CMSPAR
> + if (termios->c_cflag & CMSPAR)
> + len++;
> +#endif
I don't know if someone likes it or not (up to you), but for me looks
better to have ternary operators here:
value += (termios->c_cflag & CSTOPB) ? 1 : 0;
value += (termios->c_cflag & PARENB) ? 1 : 0;
value += (termios->c_cflag & PARODD) ? 0 : 1;
#ifdef CMSPAR
value += (termios->c_cflag & CMSPAR) ? 1 : 0;
#endif
> + len -= 9;
This one could be part of previous evaluation:
value = (termios->c_cflag & CSIZE) == CS7 ? 0 : 1;
> + len &= 3;
> + len <<= 3;
Perhaps to define magic number (e.g. LP8841_DATA_LEN_SHIFT).
> +
> + baud = tty_termios_baud_rate(termios);
> +
> +#ifdef BOTHER
> + /* We only support fixed rates */
> + cbaud = termios->c_cflag & CBAUD;
> +
> + if (cbaud == BOTHER) {
> + termios->c_cflag &= ~BOTHER;
> +
> + /* Don't rewrite B0 */
> + if (baud) {
> + tty_termios_encode_baud_rate(termios, baud,
> baud);
> + baud = tty_termios_baud_rate(termios);
> +
> + /* Set sane default speed if we get 0 */
> + if (!baud) {
> + baud = 9600;
> + tty_termios_encode_baud_rate(termios
> ,
> + baud, baud);
I think you can call this unconditionally together with case > 115200.
> + }
> + }
> + }
> +#endif
> +
> + /* We only support up to 115200 */
> + if (baud > 115200) {
> + baud = 115200;
> + tty_termios_encode_baud_rate(termios, baud, baud);
> + }
Btw, can we use uart_get_baud_rate() here?
> + writeb(len, data->ios_mem);
> +
> +}
> +static int lp8841_serial_probe(struct platform_device *pdev)
> +{
> + struct uart_8250_port uart = {};
{0}
> + struct lp8841_serial_data *data;
> + struct resource *mmres, *mires;
> + int ret;
> +
> + mmres = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + mires = platform_get_resource(pdev, IORESOURCE_MEM, 1);
Perhaps move it down to be closer to devm_ioremap_resource() call.
> + if (!mmres)
> + return -ENODEV;
> +
> + data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
> + if (!data)
> + return -ENOMEM;
> +
+ mires = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> + data->ios_mem = devm_ioremap_resource(&pdev->dev, mires);
> + if (IS_ERR(data->ios_mem))
> + return PTR_ERR(data->ios_mem);
--
Andy Shevchenko <[email protected]>
Intel Finland Oy
On Tue, 2016-03-01 at 13:06 +0200, Andy Shevchenko wrote:
> On Tue, 2016-03-01 at 00:26 +0300, Sergei Ianovich wrote:
> > The patch adds support for 3 additional LP-8x4x built-in serial
> > ports.
> >
> > The device can also host up to 8 extension cards with 4 serial
> > ports
> > on each card for a total of 35 ports. However, I don't have
> > the hardware to test extension cards, so they are not supported,
> > yet.
> >
> > Signed-off-by: Sergei Ianovich <[email protected]>
> > Reviewed-by: Heikki Krogerus <[email protected]>
>
> Few more comments, mostly about style.
>
> > +static void lp8841_serial_set_termios(struct uart_port *port,
> > + struct ktermios *termios, struct ktermios *old)
> > +{
> > +#ifdef BOTHER
> > + unsigned int cbaud;
> > +#endif
> > + unsigned int baud;
> > + unsigned int len;
>
> Since you are writing this to the register at the end maybe
> - unsigned int -> u8 (write*b* — exactly one byte)
> - len -> value (it's not only about data length)
>
> > + unsigned int old_csize = old ? old->c_cflag & CSIZE : CS8;
> > + struct lp8841_serial_data *data = port->private_data;
> > +
> > + /* We only support CS7 and CS8 */
> > + while ((termios->c_cflag & CSIZE) != CS7 &&
> > + (termios->c_cflag & CSIZE) != CS8) {
> > + termios->c_cflag &= ~CSIZE;
> > + termios->c_cflag |= old_csize;
> > + old_csize = CS8;
> > + }
> > +
> > + serial8250_do_set_termios(port, termios, old);
> > +
> > + if ((termios->c_cflag & CSIZE) == CS7)
> > + len = 9;
> > + else
> > + len = 10;
> > +
> > + if (termios->c_cflag & CSTOPB)
> > + len++;
>
> > + if (termios->c_cflag & PARENB)
> > + len++;
> > + if (!(termios->c_cflag & PARODD))
> > + len++;
> > +#ifdef CMSPAR
> > + if (termios->c_cflag & CMSPAR)
> > + len++;
> > +#endif
>
> I don't know if someone likes it or not (up to you), but for me looks
> better to have ternary operators here:
>
> value += (termios->c_cflag & CSTOPB) ? 1 : 0;
> value += (termios->c_cflag & PARENB) ? 1 : 0;
> value += (termios->c_cflag & PARODD) ? 0 : 1;
>
> #ifdef CMSPAR
> value += (termios->c_cflag & CMSPAR) ? 1 : 0;
> #endif
>
> > + len -= 9;
>
> This one could be part of previous evaluation:
> value = (termios->c_cflag & CSIZE) == CS7 ? 0 : 1;
Great point.
> > + len &= 3;
> > + len <<= 3;
>
> Perhaps to define magic number (e.g. LP8841_DATA_LEN_SHIFT).
OK
> > +
> > + baud = tty_termios_baud_rate(termios);
> > +
> > +#ifdef BOTHER
> > + /* We only support fixed rates */
> > + cbaud = termios->c_cflag & CBAUD;
> > +
> > + if (cbaud == BOTHER) {
> > + termios->c_cflag &= ~BOTHER;
> > +
> > + /* Don't rewrite B0 */
> > + if (baud) {
> > + tty_termios_encode_baud_rate(termios,
> > baud,
> > baud);
> > + baud = tty_termios_baud_rate(termios);
> > +
> > + /* Set sane default speed if we get 0 */
> > + if (!baud) {
> > + baud = 9600;
>
> > + tty_termios_encode_baud_rate(termi
> > os
> > ,
> > + baud, baud);
>
> I think you can call this unconditionally together with case >
> 115200.
The calls are orthogonal. This one deals with the case when BOTHER is
defined and set, and we have non-zero rate with BOTHER, but we have
zero rate after BOTHER is cleared. So we set 9600 as a sane default
speed.
> > + }
> > + }
> > + }
> > +#endif
> > +
> > + /* We only support up to 115200 */
> > + if (baud > 115200) {
> > + baud = 115200;
> > + tty_termios_encode_baud_rate(termios, baud, baud);
> > + }
This one deals with the case when the rate is over 115200. If the
previous case has been triggered, this one won't be.
> Btw, can we use uart_get_baud_rate() here?
uart_get_baud_rate() has already been called
in serial8250_do_set_termios(). uart_get_baud_rate()
calls tty_termios_encode_baud_rate(). uart_get_baud_rate() won't help
us if BOTHER is set. Once BOTHER is cleared, we don't need any special
processing of uart_get_baud_rate().
> > +static int lp8841_serial_probe(struct platform_device *pdev)
> > +{
> > + struct uart_8250_port uart = {};
>
> {0}
---
drivers/tty/serial/8250/8250_lp8841.c: In function 'lp8841_serial_probe':
drivers/tty/serial/8250/8250_lp8841.c:124:32: warning: excess elements in struct initializer
struct uart_8250_port uart = {0};
^
drivers/tty/serial/8250/8250_lp8841.c:124:32: note: (near initialization for 'uart.port.lock.<anonymous>.rlock.raw_lock')
---
Zero triggers a warning. I'll use memset().
> > + struct lp8841_serial_data *data;
> > + struct resource *mmres, *mires;
> > + int ret;
> > +
> > + mmres = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>
> > + mires = platform_get_resource(pdev, IORESOURCE_MEM, 1);
>
> Perhaps move it down to be closer to devm_ioremap_resource() call.
OK
Thanks for lightning fast reviews. I'll resubmit v8 if there is no
objections to the points above.
On Tue, 2016-03-01 at 19:25 +0300, Sergei Ianovich wrote:
> On Tue, 2016-03-01 at 13:06 +0200, Andy Shevchenko wrote:
> > On Tue, 2016-03-01 at 00:26 +0300, Sergei Ianovich wrote:
> > > + len &= 3;
Mask as well to be defined.
> > > + len <<= 3;
> >
> > Perhaps to define magic number (e.g. LP8841_DATA_LEN_SHIFT).
>
> OK
> + baud = tty_termios_baud_rate(termios);
> > > +
> > > +#ifdef BOTHER
> > > + /* We only support fixed rates */
So, but if you support only fixed rates, why do you care about BOTHER
at all?
> > >
> > I think you can call this unconditionally together with case >
> > 115200.
>
> The calls are orthogonal. This one deals with the case when BOTHER is
> defined and set, and we have non-zero rate with BOTHER, but we have
> zero rate after BOTHER is cleared. So we set 9600 as a sane default
> speed.
> +
> > > + /* We only support up to 115200 */
> > > + if (baud > 115200) {
> > > + baud = 115200;
> > > + tty_termios_encode_baud_rate(termios, baud,
> > > baud);
> > > + }
>
> This one deals with the case when the rate is over 115200. If the
> previous case has been triggered, this one won't be.
Yeah, but I meant to unconditionally call it just once here every time.
tty_termios_encode_baud_rate(termios, baud, baud);
> +static int lp8841_serial_probe(struct platform_device *pdev)
> > > +{
> > > + struct uart_8250_port uart = {};
> >
> > {0}
>
> ---
> drivers/tty/serial/8250/8250_lp8841.c: In function
> 'lp8841_serial_probe':
> drivers/tty/serial/8250/8250_lp8841.c:124:32: warning: excess
> elements in struct initializer
> struct uart_8250_port uart = {0};
> ^
> drivers/tty/serial/8250/8250_lp8841.c:124:32: note: (near
> initialization for 'uart.port.lock.<anonymous>.rlock.raw_lock')
Do you have any warning verbosity enabled? I see a lot of stuff like
this in the code
$ git grep -n 'struct .* = {0};' | wc -l
338
$ git grep -n 'struct .* = { \?0 \?};' | wc -l
550
( '… = { 0 };' included)
> ---
>
> Zero triggers a warning. I'll use memset().
Either will work.
> Thanks for lightning fast reviews. I'll resubmit v8 if there is no
> objections to the points above.
See above.
--
Andy Shevchenko <[email protected]>
Intel Finland Oy
On Tue, 2016-03-01 at 18:46 +0200, Andy Shevchenko wrote:
> On Tue, 2016-03-01 at 19:25 +0300, Sergei Ianovich wrote:
> > On Tue, 2016-03-01 at 13:06 +0200, Andy Shevchenko wrote:
> > > On Tue, 2016-03-01 at 00:26 +0300, Sergei Ianovich wrote:
>
> > > > + len &= 3;
>
> Mask as well to be defined.
Sure.
> So, but if you support only fixed rates, why do you care about BOTHER
> at all?
If BOTHER is defined, tty_termios_baud_rate()
and tty_termios_encode_baud_rate() allow non-standard baud rates. I
should clear it from c_cflag to indicate I don't support it.
> > > >
> > > I think you can call this unconditionally together with case >
> > > 115200.
> >
> > The calls are orthogonal. This one deals with the case when BOTHER
> > is
> > defined and set, and we have non-zero rate with BOTHER, but we have
> > zero rate after BOTHER is cleared. So we set 9600 as a sane default
> > speed.
> >
> > This one deals with the case when the rate is over 115200. If the
> > previous case has been triggered, this one won't be.
>
> Yeah, but I meant to unconditionally call it just once here every
> time.
I see. It saves a few lines.
> > ---
> > drivers/tty/serial/8250/8250_lp8841.c: In function
> > 'lp8841_serial_probe':
> > drivers/tty/serial/8250/8250_lp8841.c:124:32: warning: excess
> > elements in struct initializer
> > struct uart_8250_port uart = {0};
> > ^
> > drivers/tty/serial/8250/8250_lp8841.c:124:32: note: (near
> > initialization for 'uart.port.lock.<anonymous>.rlock.raw_lock')
>
> Do you have any warning verbosity enabled? I see a lot of stuff like
> this in the code
Plain `make`.
The warning seems to be the result of initializing a spinlock with
zero. Spinlocks are intentionally obfuscated, but I didn't investigate
further.
> $ git grep -n 'struct .* = {0};' | wc -l
> 338
>
> $ git grep -n 'struct .* = { \?0 \?};' | wc -l
> 550
>
> ( '… = { 0 };' included)
The first structure member is most likely not a spinlock in those
cases.
> > ---
> >
> > Zero triggers a warning. I'll use memset().
>
> Either will work.
OK
The only remaining open point is BOTHER handling.
On Tue, 2016-03-01 at 20:14 +0300, Sergei Ianovich wrote:
> On Tue, 2016-03-01 at 18:46 +0200, Andy Shevchenko wrote:
> > On Tue, 2016-03-01 at 19:25 +0300, Sergei Ianovich wrote:
> > > On Tue, 2016-03-01 at 13:06 +0200, Andy Shevchenko wrote:
> > > > On Tue, 2016-03-01 at 00:26 +0300, Sergei Ianovich wrote:
> > So, but if you support only fixed rates, why do you care about
> > BOTHER
> > at all?
>
> If BOTHER is defined, tty_termios_baud_rate()
> and tty_termios_encode_baud_rate() allow non-standard baud rates. I
> should clear it from c_cflag to indicate I don't support it.
>
> > > > >
> > > > I think you can call this unconditionally together with case >
> > > > 115200.
> > >
> > > The calls are orthogonal. This one deals with the case when
> > > BOTHER
> > > is
> > > defined and set, and we have non-zero rate with BOTHER, but we
> > > have
> > > zero rate after BOTHER is cleared. So we set 9600 as a sane
> > > default
> > > speed.
Maybe you just set a baud rate nearest to the one from the table in
case of BOTHER?
In that case perhaps you have to supply +-1 to the range. That's why I
asked about uart_get_baud_rate().
Maybe this flow will work for you
if (BOTHER)
clear BOTHER
call uart_get_baud_rate()
?
> The warning seems to be the result of initializing a spinlock with
> zero. Spinlocks are intentionally obfuscated, but I didn't
> investigate
> further.
>
> > $ git grep -n 'struct .* = {0};' | wc -l
> > 338
> >
> > $ git grep -n 'struct .* = { \?0 \?};' | wc -l
> > 550
> >
> > ( '… = { 0 };' included)
>
> The first structure member is most likely not a spinlock in those
> cases.
Hmm... Interesting. On one hand the poison is reasonable, on the other
we often do a memset() or {0} on structures, i.o.w. assign 0 as initial
value until spinlock_init().
Arnd, what do you think about this (and similar) case(s)?
--
Andy Shevchenko <[email protected]>
Intel Finland Oy
> Maybe you just set a baud rate nearest to the one from the table in
> case of BOTHER?
This is broken. BOTHER can be set with a perfectly valid baud rate that
could equally be represented by B9600 say.
If you are stuck with limited ranges then
switch(baud) {
case 9600:
case 4800:
etc
and don't worry about BOTHER, it's entirely transparent to you. The core
kernel code will provide you with a baud rate number, the re-encoder will
always do the right thing.
A driver should never care about BOTHER or any of the baud bits in the
termios structure directly.
Alan
On Tue, 2016-03-01 at 19:48 +0200, Andy Shevchenko wrote:
> On Tue, 2016-03-01 at 20:14 +0300, Sergei Ianovich wrote:
> > On Tue, 2016-03-01 at 18:46 +0200, Andy Shevchenko wrote:
> > > On Tue, 2016-03-01 at 19:25 +0300, Sergei Ianovich wrote:
> > > > On Tue, 2016-03-01 at 13:06 +0200, Andy Shevchenko wrote:
> > > > > On Tue, 2016-03-01 at 00:26 +0300, Sergei Ianovich wrote:
>
> > > So, but if you support only fixed rates, why do you care about
> > > BOTHER
> > > at all?
> >
> > If BOTHER is defined, tty_termios_baud_rate()
> > and tty_termios_encode_baud_rate() allow non-standard baud rates. I
> > should clear it from c_cflag to indicate I don't support it.
> >
> > > > > >
> > > > > I think you can call this unconditionally together with case
> > > > > >
> > > > > 115200.
> > > >
> > > > The calls are orthogonal. This one deals with the case when
> > > > BOTHER
> > > > is
> > > > defined and set, and we have non-zero rate with BOTHER, but we
> > > > have
> > > > zero rate after BOTHER is cleared. So we set 9600 as a sane
> > > > default
> > > > speed.
>
> Maybe you just set a baud rate nearest to the one from the table in
> case of BOTHER?
>
> In that case perhaps you have to supply +-1 to the range. That's why
> I
> asked about uart_get_baud_rate().
>
> Maybe this flow will work for you
>
> if (BOTHER)
> clear BOTHER
> call uart_get_baud_rate()
>
> ?
It works well for standard rates, let it be so. If there ever is a
problem, we can fix it.
> > Maybe this flow will work for you
> >
> > if (BOTHER)
> > clear BOTHER
> > call uart_get_baud_rate()
> >
> > ?
>
> It works well for standard rates, let it be so. If there ever is a
> problem, we can fix it.
I'm NAKking the v7 PATCH because we spent ages getting all the drivers to
use tty_termios_get_baud_rate() cleanly.
Get rid of everything in the ifdef BOTHER
Remove the if baud > 115200 stuff
For the default: entry in the case add
tty_termios_encode_baud_rate(termios, 2400, 2400);
and all will be good. Anything not a standard rate will get 2400 baud and
reported back to the user properly as that rate.
You could do matches for "within 10%" but really I don't think it matters
and other drivers don't bother either when they have such fixed clocks.
Alan