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@xxxxxxx> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
