On Tue, 2015-05-26 at 15:28 +0100, Alan Cox wrote: > > > + > > > +#define UNIPHIER_UART_CHAR_FCR 3 /* Character / FIFO Control Register */ > > > +#define UNIPHIER_UART_LCR_MCR 4 /* Line/Modem Control Register */ > > > +#define UNIPHIER_UART_LCR_SHIFT 8 > > > > Indentation problem, needs to be fixed. > > If you are going to review a patch set at least look at the previous > reviews - the indentation was already discussed and is done that way to > show (as many drivers do) which are fields for which registers This is not exactly the field, the way how to get the field. In some cases it is even better to define something like _LCR(x) ((x) << 8) > > > > +static unsigned int uniphier_serial_in(struct uart_port *p, int offset) > > > +{ > > > + int valshift = 0; > > > > Perhaps unsigned int? > > Why ? even if it mattered gcc is already realising that the value can > only be 0 or 8 and will be generating whatever works best for that. It's not about how gcc does, it's about what assumptions can be made from the reading of the source code. I think if we do a counter of shift value it would be nice to set an unsigned type explicitly. -- Andy Shevchenko <andriy.shevchenko@xxxxxxxxx> Intel Finland Oy --------------------------------------------------------------------- Intel Finland Oy Registered Address: PL 281, 00181 Helsinki Business Identity Code: 0357606 - 4 Domiciled in Helsinki This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). Any review or distribution by others is strictly prohibited. If you are not the intended recipient, please contact the sender and delete all copies. ��.n��������+%������w��{.n�����{��ǫ����{ay�ʇڙ���f���h������_�(�階�ݢj"��������G����?���&��