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

[Sascha Hauer <s.hauer@pengutronix.de>]

On Fri, Aug 05, 2011 at 12:58:32AM +0400, Antony Pavlov wrote:
> 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.

And as another rule, locally used struct (and also defines) go to the C
file.

> 
> > +
> > +static uint32_t ns16550_readb(void __iomem *base, uint8_t off)
> > +{
> > +       return readb(base + off);
> 
>  warning: pointer of type ‘void *’ used in arithmetic

Honestly, I can find nothing wrong in void * arithmetic. The
alternatives like casting to/from unsigned long are much worse.

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

The compiler can decide this for itself.

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

I don't care much. If anything, I would say that positive logic is
easier to read.

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

:)

Sascha


-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

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