Re: [PATCH 1/2] serial_ns16550: move switch from ns16550_{read, write}() to ns16550_probe()

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

On Thu, Aug 04, 2011 at 11:38:53AM +0400, Antony Pavlov wrote:
> On 4 August 2011 11:19, Sascha Hauer <s.hauer@pengutronix.de> wrote:
> 
> >>
> >> +#define NS16550_READ_WRITE_UART_FUNC(pfx, cmdr, cmdw, mask) \
> >> +static unsigned int ns16550_generic_##pfx##_read(unsigned long base, \
> >> +             unsigned char reg_idx)          \
> >> +{                                            \
> >> +     return cmdr((char *)base + reg_idx);    \
> >> +}                                            \
> >> +                                             \
> >> +static void ns16550_generic_##pfx##_write(unsigned int val, unsigned long base, \
> >> +             unsigned char reg_idx)                  \
> >> +{                                                    \
> >> +     cmdw((mask val), (char *)base + reg_idx);       \
> >> +}
> >> +
> >> +NS16550_READ_WRITE_UART_FUNC(8bit, readb, writeb, 0xff &)
> >> +NS16550_READ_WRITE_UART_FUNC(16bit, readw, writew, 0xffff &)
> >> +NS16550_READ_WRITE_UART_FUNC(32bit, readl, writel, )
> >
> > Please don't do such preprocessor tricks if not necessary. we can afford
> > the 8 additional lines for making this readable.
> 
> approx. > 16 additional lines

Whatever.

> 
> >> +     if (plat->reg_read == NULL) {
> >> +             int width = dev->resource[0].flags & IORESOURCE_MEM_TYPE_MASK;
> >> +
> >> +             switch (width) {
> ...
> >> +     }
> >> +
> >> +     if (plat->reg_write == NULL) {
> >> +             int width = dev->resource[0].flags & IORESOURCE_MEM_TYPE_MASK;
> >> +
> >> +             switch (width) {
> ...
> >> +             }
> >> +     }
> >
> > Passing one of the register access function without the other shouldn't
> > be a valid usecase. Jean is right, there should be only one if().
> 
> Agree.
> 
> > Generally platform_data shouldn't be used after device probe and for
> > sure it shouldn't be modified. This may be the point where this driver
> > needs a private data struct.
> 
> I like this point of view.
> We must add private data struct and copy plat->width, plat->reg_read etc there.

You won't need plat->width, only the correct the register accessors
depending on plat->width.

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