On Fri, Aug 05, 2011 at 12:58:32AM +0400, Antony Pavlov wrote:
> On 4 August 2011 23:37, Jean-Christophe PLAGNIOL-VILLARD
> <plagnioj@xxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxx
http://lists.infradead.org/mailman/listinfo/barebox
