From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Shevchenko, Andriy" Subject: Re: [PATCH v6] serial: 8250_uniphier: add UniPhier serial driver Date: Tue, 26 May 2015 15:08:27 +0000 Message-ID: <1432652906.8736.44.camel@intel.com> References: <1432525477-14051-1-git-send-email-yamada.masahiro@socionext.com> <1432545245.8736.5.camel@intel.com> <1432650518.13900.72.camel@linux.intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: In-Reply-To: <1432650518.13900.72.camel@linux.intel.com> Content-Language: en-US Content-ID: Sender: linux-kernel-owner@vger.kernel.org To: "alan@linux.intel.com" Cc: "manabian@gmail.com" , "linux-kernel@vger.kernel.org" , "blogic@openwrt.org" , "bigeasy@linutronix.de" , "alan@lxorguk.ukuu.org.uk" , "ricardo.ribalda@gmail.com" , "yamada.masahiro@socionext.com" , "linux-serial@vger.kernel.org" , "gregkh@linuxfoundation.org" , "jslaby@suse.cz" , "matthias.bgg@gmail.com" , "peter@hurleysoftware.com" , "linux-arm-kernel@lists.infradead.org" List-Id: linux-serial@vger.kernel.org T24gVHVlLCAyMDE1LTA1LTI2IGF0IDE1OjI4ICswMTAwLCBBbGFuIENveCB3cm90ZToNCj4gPiA+ ICsNCj4gPiA+ICsjZGVmaW5lIFVOSVBISUVSX1VBUlRfQ0hBUl9GQ1IJMwkvKiBDaGFyYWN0ZXIg LyBGSUZPIENvbnRyb2wgUmVnaXN0ZXIgKi8NCj4gPiA+ICsjZGVmaW5lIFVOSVBISUVSX1VBUlRf TENSX01DUgk0CS8qIExpbmUvTW9kZW0gQ29udHJvbCBSZWdpc3RlciAqLw0KPiA+ID4gKyNkZWZp bmUgICBVTklQSElFUl9VQVJUX0xDUl9TSElGVAk4DQo+ID4gDQo+ID4gSW5kZW50YXRpb24gcHJv YmxlbSwgbmVlZHMgdG8gYmUgZml4ZWQuDQo+IA0KPiBJZiB5b3UgYXJlIGdvaW5nIHRvIHJldmll dyBhIHBhdGNoIHNldCBhdCBsZWFzdCBsb29rIGF0IHRoZSBwcmV2aW91cw0KPiByZXZpZXdzIC0g dGhlIGluZGVudGF0aW9uIHdhcyBhbHJlYWR5IGRpc2N1c3NlZCBhbmQgaXMgZG9uZSB0aGF0IHdh eSB0bw0KPiBzaG93IChhcyBtYW55IGRyaXZlcnMgZG8pIHdoaWNoIGFyZSBmaWVsZHMgZm9yIHdo aWNoIHJlZ2lzdGVycw0KDQpUaGlzIGlzIG5vdCBleGFjdGx5IHRoZSBmaWVsZCwgdGhlIHdheSBo b3cgdG8gZ2V0IHRoZSBmaWVsZC4NCkluIHNvbWUgY2FzZXMgaXQgaXMgZXZlbiBiZXR0ZXIgdG8g ZGVmaW5lIHNvbWV0aGluZyBsaWtlDQpfTENSKHgpICAoKHgpIDw8IDgpDQoNCg0KPiANCj4gPiA+ ICtzdGF0aWMgdW5zaWduZWQgaW50IHVuaXBoaWVyX3NlcmlhbF9pbihzdHJ1Y3QgdWFydF9wb3J0 ICpwLCBpbnQgb2Zmc2V0KQ0KPiA+ID4gK3sNCj4gPiA+ICsJaW50IHZhbHNoaWZ0ID0gMDsNCj4g PiANCj4gPiBQZXJoYXBzIHVuc2lnbmVkIGludD8NCj4gDQo+IFdoeSA/IGV2ZW4gaWYgaXQgbWF0 dGVyZWQgZ2NjIGlzIGFscmVhZHkgcmVhbGlzaW5nIHRoYXQgdGhlIHZhbHVlIGNhbg0KPiBvbmx5 IGJlIDAgb3IgOCBhbmQgd2lsbCBiZSBnZW5lcmF0aW5nIHdoYXRldmVyIHdvcmtzIGJlc3QgZm9y IHRoYXQuDQoNCkl0J3Mgbm90IGFib3V0IGhvdyBnY2MgZG9lcywgaXQncyBhYm91dCB3aGF0IGFz c3VtcHRpb25zIGNhbiBiZSBtYWRlDQpmcm9tIHRoZSByZWFkaW5nIG9mIHRoZSBzb3VyY2UgY29k ZS4gSSB0aGluayBpZiB3ZSBkbyBhIGNvdW50ZXIgb2Ygc2hpZnQNCnZhbHVlIGl0IHdvdWxkIGJl IG5pY2UgdG8gc2V0IGFuIHVuc2lnbmVkIHR5cGUgZXhwbGljaXRseS4NCg0KLS0gDQpBbmR5IFNo ZXZjaGVua28gPGFuZHJpeS5zaGV2Y2hlbmtvQGludGVsLmNvbT4NCkludGVsIEZpbmxhbmQgT3kN Ci0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0t LS0tLS0tLS0tLS0tLQpJbnRlbCBGaW5sYW5kIE95ClJlZ2lzdGVyZWQgQWRkcmVzczogUEwgMjgx LCAwMDE4MSBIZWxzaW5raSAKQnVzaW5lc3MgSWRlbnRpdHkgQ29kZTogMDM1NzYwNiAtIDQgCkRv bWljaWxlZCBpbiBIZWxzaW5raSAKClRoaXMgZS1tYWlsIGFuZCBhbnkgYXR0YWNobWVudHMgbWF5 IGNvbnRhaW4gY29uZmlkZW50aWFsIG1hdGVyaWFsIGZvcgp0aGUgc29sZSB1c2Ugb2YgdGhlIGlu dGVuZGVkIHJlY2lwaWVudChzKS4gQW55IHJldmlldyBvciBkaXN0cmlidXRpb24KYnkgb3RoZXJz IGlzIHN0cmljdGx5IHByb2hpYml0ZWQuIElmIHlvdSBhcmUgbm90IHRoZSBpbnRlbmRlZApyZWNp cGllbnQsIHBsZWFzZSBjb250YWN0IHRoZSBzZW5kZXIgYW5kIGRlbGV0ZSBhbGwgY29waWVzLgo= From mboxrd@z Thu Jan 1 00:00:00 1970 From: andriy.shevchenko@intel.com (Shevchenko, Andriy) Date: Tue, 26 May 2015 15:08:27 +0000 Subject: [PATCH v6] serial: 8250_uniphier: add UniPhier serial driver In-Reply-To: <1432650518.13900.72.camel@linux.intel.com> References: <1432525477-14051-1-git-send-email-yamada.masahiro@socionext.com> <1432545245.8736.5.camel@intel.com> <1432650518.13900.72.camel@linux.intel.com> Message-ID: <1432652906.8736.44.camel@intel.com> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org 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 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. From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751608AbbEZPIf (ORCPT ); Tue, 26 May 2015 11:08:35 -0400 Received: from mga14.intel.com ([192.55.52.115]:19694 "EHLO mga14.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750726AbbEZPIb (ORCPT ); Tue, 26 May 2015 11:08:31 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.13,498,1427785200"; d="scan'208";a="731863179" From: "Shevchenko, Andriy" To: "alan@linux.intel.com" CC: "manabian@gmail.com" , "linux-kernel@vger.kernel.org" , "blogic@openwrt.org" , "bigeasy@linutronix.de" , "alan@lxorguk.ukuu.org.uk" , "ricardo.ribalda@gmail.com" , "yamada.masahiro@socionext.com" , "linux-serial@vger.kernel.org" , "gregkh@linuxfoundation.org" , "jslaby@suse.cz" , "matthias.bgg@gmail.com" , "peter@hurleysoftware.com" , "linux-arm-kernel@lists.infradead.org" Subject: Re: [PATCH v6] serial: 8250_uniphier: add UniPhier serial driver Thread-Topic: [PATCH v6] serial: 8250_uniphier: add UniPhier serial driver Thread-Index: AQHQl8BBSViTbfCFTEeK18jo2NRp8Z2OS10A Date: Tue, 26 May 2015 15:08:27 +0000 Message-ID: <1432652906.8736.44.camel@intel.com> References: <1432525477-14051-1-git-send-email-yamada.masahiro@socionext.com> <1432545245.8736.5.camel@intel.com> <1432650518.13900.72.camel@linux.intel.com> In-Reply-To: <1432650518.13900.72.camel@linux.intel.com> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.237.72.90] Content-Type: text/plain; charset="utf-8" Content-ID: MIME-Version: 1.0 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Transfer-Encoding: 8bit X-MIME-Autoconverted: from base64 to 8bit by nfs id t4QF93vR012498 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 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+{G{ayʇڙ,jfhz_(階ݢj"mG?&~iOzv^m ?I