Re: [PATCH] ns16550: switch platform_data to drivers private data

[Antony Pavlov <antonynpavlov@gmail.com>]

On 4 August 2011 23:37, Jean-Christophe PLAGNIOL-VILLARD
<plagnioj@jcrosoft.com> wrote:

> --- a/drivers/serial/serial_ns16550.c
> +++ b/drivers/serial/serial_ns16550.c
> @@ -48,6 +48,55 @@
>
>  /*********** Private Functions **********************************/
>
> +/*
> + * We wrap our port structure around the generic console_device.
> + */
> +struct ns16550_uart_port {
> +       void __iomem *base;
> +       uint32_t shift;
> +       uint32_t clock;
> +       uint32_t (*read)(void __iomem *base, uint8_t off);
> +       void (*write)(uint32_t val, void __iomem *base, uint8_t off);
> +
> +       struct console_device uart;

          nice trick :)))

          But why the name is 'uart', not 'cdev'?
> +};

As a rule, structure declarations go to header files.

> +
> +static uint32_t ns16550_readb(void __iomem *base, uint8_t off)
> +{
> +       return readb(base + off);

 warning: pointer of type ‘void *’ used in arithmetic


> -static unsigned int ns16550_calc_divisor(struct console_device *cdev,
> +static unsigned int ns16550_calc_divisor(struct ns16550_uart_port *uart,
>                                         unsigned int baudrate)
>  {
> -       struct NS16550_plat *plat = (struct NS16550_plat *)
> -           cdev->dev->platform_data;
> -       unsigned int clk = plat->clock;
> -
> -       return (clk / MODE_X_DIV / baudrate);
> -
> +       return (uart->clock / MODE_X_DIV / baudrate);
>  }

inline?


> +       if (!plat->reg_read) {
> +               switch (width) {
> +               case IORESOURCE_MEM_8BIT:
> +                       uart->read = ns16550_readb;
> +                       break;
> +               case IORESOURCE_MEM_16BIT:
> +                       uart->read = ns16550_readw;
> +                       break;
> +               case IORESOURCE_MEM_32BIT:
> +                       uart->read = ns16550_readl;
> +                       break;
> +               }
> +       } else {
> +               uart->read = plat->reg_read;
> +       }

In this if the 'else' block is very short. 'if' block is much longer.
Swapping of them will improve readability.

> +       if (!plat->reg_write) {
> +               switch (width) {
> +               case IORESOURCE_MEM_8BIT:
> +                       uart->write = ns16550_writeb;
> +                       break;
> +               case IORESOURCE_MEM_16BIT:
> +                       uart->write = ns16550_writew;
> +                       break;
> +               case IORESOURCE_MEM_32BIT:
> +                       uart->write = ns16550_writel;
> +                       break;
> +               }
> +       } else {
> +               uart->write = plat->reg_write;
> +       }

why do it twice?

-- 
Best regards,
  Antony Pavlov

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox