linux-kernel - Re: [PATCH V3 1/7] tty: serial: lpuart: introduce lpuart_soc

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 13 Jun 2017 10:28:48 +0800
From:   Dong Aisheng <dongas86@...il.com>
To:     Andy Shevchenko <andy.shevchenko@...il.com>
Cc:     Dong Aisheng <aisheng.dong@....com>,
        "linux-serial@...r.kernel.org" <linux-serial@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        linux-arm Mailing List <linux-arm-kernel@...ts.infradead.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Jiri Slaby <jslaby@...e.com>, Andy Duan <fugang.duan@....com>,
        Stefan Agner <stefan@...er.ch>,
        Mingkai Hu <Mingkai.Hu@....com>, "Y.B. Lu" <yangbo.lu@....com>,
        Nikita Yushchenko <nikita.yoush@...entembedded.com>
Subject: Re: [PATCH V3 1/7] tty: serial: lpuart: introduce lpuart_soc_data to
 represent SoC property

On Mon, Jun 12, 2017 at 08:49:36PM +0300, Andy Shevchenko wrote:
> On Mon, Jun 12, 2017 at 6:37 PM, Dong Aisheng <aisheng.dong@....com> wrote:
> > This is used to dynamically check the SoC specific lpuart properies.
> > Currently only the iotype is added, it functions the same as before.
> > With this, new chips with different iotype will be more easily added.
> 
> 
> > +struct lpuart_soc_data {
> > +       char    iotype;
> > +};
> > +
> > +static const struct lpuart_soc_data vf_data = {
> > +       .iotype = UPIO_MEM,
> > +};
> > +
> > +static const struct lpuart_soc_data ls_data = {
> > +       .iotype = UPIO_MEM32BE,
> 
> > +
> 
> Redundant.

My mistake to introduce one more extra blank line...

> 
> > +};
> 
> And now most interesting part...
> 
> > -               if (sport->lpuart32)
> > +               if (sport->port.iotype & UPIO_MEM32BE)
> >                         lpuart32_write(sport->port.x_char, sport->port.membase + UARTDATA);
> >                 else
> >                         writeb(sport->port.x_char, sport->port.membase + UARTDR);
> 
> > -               if (sport->lpuart32)
> > +               if (sport->port.iotype & UPIO_MEM32BE)
> >                         lpuart32_stop_tx(&sport->port);
> >                 else
> >                         lpuart_stop_tx(&sport->port);
> 
> > -       if (sport->lpuart32)
> > +       if (sport->port.iotype & UPIO_MEM32BE)
> >                 lpuart32_transmit_buffer(sport);
> >         else
> >                 lpuart_transmit_buffer(sport);
> 
> > -               if (sport->lpuart32)
> > +               if (sport->port.iotype & UPIO_MEM32BE)
> >                         lpuart32_console_get_options(sport, &baud, &parity, &bits);
> >                 else
> >                         lpuart_console_get_options(sport, &baud, &parity, &bits);
> 
> > -       if (sport->lpuart32)
> > +       if (sport->port.iotype & UPIO_MEM32BE)
> >                 lpuart32_setup_watermark(sport);
> >         else
> >                 lpuart_setup_watermark(sport);
> 
> > -       if (sport->lpuart32)
> > +       sport->port.iotype = sdata->iotype;
> > +       if (sport->port.iotype & UPIO_MEM32BE)
> >                 sport->port.ops = &lpuart32_pops;
> >         else
> >                 sport->port.ops = &lpuart_pops;
> 
> > -       if (sport->lpuart32)
> > +       if (sport->port.iotype & UPIO_MEM32BE)
> >                 lpuart_reg.cons = LPUART32_CONSOLE;
> >         else
> >                 lpuart_reg.cons = LPUART_CONSOLE;
> 
> ...all above since you introduced nice struct, can get rid of conditionals.
> Instead it might be a members of the struct above.
> 
> (I dunno if it's good to have in this patch, but at list a follow up
> could be nice to have)
> 

Yes, to clean up all conditionals, much more things need to be done,
so a separate follow up patch may be better.

This patch only address iotype which is just the same as before.

> > -       if (sport->lpuart32) {
> > +       if (sport->port.iotype & UPIO_MEM32BE) {
> >                 /* disable Rx/Tx and interrupts */
> >                 temp = lpuart32_read(sport->port.membase + UARTCTRL);
> >                 temp &= ~(UARTCTRL_TE | UARTCTRL_TIE | UARTCTRL_TCIE);
> 
> > -       if (sport->lpuart32) {
> > +       if (sport->port.iotype & UPIO_MEM32BE) {
> >                 lpuart32_setup_watermark(sport);
> >                 temp = lpuart32_read(sport->port.membase + UARTCTRL);
> >                 temp |= (UARTCTRL_RIE | UARTCTRL_TIE | UARTCTRL_RE |
> 
> Above are questionable, might be not need to convert them.
> 
> So, in any case above is a sighting which you could address (separately).

Yes, seems not a easy convert which can be investigated later.

Thanks for the review.

Regards
Dong Aisheng

> 
> -- 
> With Best Regards,
> Andy Shevchenko