On Mon, Jun 12, 2017 at 08:49:36PM +0300, Andy Shevchenko wrote:

On Mon, Jun 12, 2017 at 6:37 PM, Dong Aisheng [off-list ref] wrote:

quoted

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.

quoted

+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,

quoted

+

Redundant.

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

quoted

+};

And now most interesting part...

quoted

-               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);

quoted

-               if (sport->lpuart32)
+               if (sport->port.iotype & UPIO_MEM32BE)
                        lpuart32_stop_tx(&sport->port);
                else
                        lpuart_stop_tx(&sport->port);

quoted

-       if (sport->lpuart32)
+       if (sport->port.iotype & UPIO_MEM32BE)
                lpuart32_transmit_buffer(sport);
        else
                lpuart_transmit_buffer(sport);

quoted

-               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);

quoted

-       if (sport->lpuart32)
+       if (sport->port.iotype & UPIO_MEM32BE)
                lpuart32_setup_watermark(sport);
        else
                lpuart_setup_watermark(sport);

quoted

-       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;

quoted

-       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.

quoted

-       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);

quoted

-       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