On Thu, Aug 04, 2011 at 11:38:53AM +0400, Antony Pavlov wrote:
> On 4 August 2011 11:19, Sascha Hauer <s.hauer@xxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxx
http://lists.infradead.org/mailman/listinfo/barebox
