Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756141AbcCCDln (ORCPT ); Wed, 2 Mar 2016 22:41:43 -0500 Received: from 60-251-194-8.HINET-IP.hinet.net ([60.251.194.8]:64735 "EHLO mse1.corp.rad-ic.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1752425AbcCCDll (ORCPT ); Wed, 2 Mar 2016 22:41:41 -0500 X-Greylist: delayed 1487 seconds by postgrey-1.27 at vger.kernel.org; Wed, 02 Mar 2016 22:41:40 EST From: =?utf-8?B?SmVmZnJleSBMaW4gKOael+e+qeeroCk=?= To: Joe Perches , "jeffrey.lin" , "dmitry.torokhov@gmail.com" , "rydberg@euromail.se" , "grant.likely@linaro.org" , "robh+dt@kernel.org" , "jeesw@melfas.com" , "bleung@chromium.org" , "scott.liu@emc.com.tw" CC: =?utf-8?B?Um9nZXIgWWFuZyAo5qWK6Y6u55GLKQ==?= , =?utf-8?B?S1AgTGkgKOadjuaYhuWAjSk=?= , =?utf-8?B?QWxiZXJ0IFNoaWVoICjorJ3mrKPnkYsp?= , "linux-kernel@vger.kernel.org" , "linux-input@vger.kernel.org" , "devicetree@vger.kernel.org" Subject: RE: [PATCH] driver: input :touchscreen : add Raydium I2C touch driver Thread-Topic: [PATCH] driver: input :touchscreen : add Raydium I2C touch driver Thread-Index: AQHRdPabvI8FsmV5ekahpTUijpE3b59HC1IQ Date: Thu, 3 Mar 2016 03:14:42 +0000 Message-ID: <9BE4B49389AB2F41A64A32DFC844F1DF73EF8627@RADHCMS5.corp.rad-ic.com> References: <1456972150-32303-1-git-send-email-jeffrey.lin@rad-ic.com> <1456973050.4044.81.camel@perches.com> In-Reply-To: <1456973050.4044.81.camel@perches.com> Accept-Language: zh-TW, en-US Content-Language: zh-TW X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [192.168.16.126] Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 X-MAIL: mse1.corp.rad-ic.com u233Eig1081111 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 mail.home.local id u233flgN019389 Content-Length: 3311 Lines: 119 Hi Joe: Thank you for your response. I'll follow your comments in next version. Best Regards ---------------------------------------------------------------------- Jeffrey Lin,林義章 瑞鼎科技 Raydium Semiconductor Corporation Tel:(03)666-1818 Ext.4163 Fax:(03)666-1919 -----Original Message----- From: Joe Perches [mailto:joe@perches.com] Sent: Thursday, March 03, 2016 10:44 AM To: jeffrey.lin; dmitry.torokhov@gmail.com; rydberg@euromail.se; grant.likely@linaro.org; robh+dt@kernel.org; jeesw@melfas.com; bleung@chromium.org; scott.liu@emc.com.tw Cc: Jeffrey Lin (林義章); Roger Yang (楊鎮瑋); KP Li (李昆倍); Albert Shieh (謝欣瑋); linux-kernel@vger.kernel.org; linux-input@vger.kernel.org; devicetree@vger.kernel.org Subject: Re: [PATCH] driver: input :touchscreen : add Raydium I2C touch driver On Thu, 2016-03-03 at 10:29 +0800, jeffrey.lin wrote: > This patch is porting Raydium I2C touch driver. Developer can enable raydium touch driver by modifying define "CONFIG_TOUCHSCREEN_RM_TS". trivial comments: > diff --git a/drivers/input/touchscreen/raydium_i2c_ts.c b/drivers/input/touchscreen/raydium_i2c_ts.c [] > +static int raydium_i2c_send(struct i2c_client *client, > + u8 addr, u8 *data, size_t len) > +{ > + u8 buf[MAX_PACKET_SIZE + 1]; > + int i, tries = 0; > + > + if (len > MAX_PACKET_SIZE) > + return -EINVAL; > + > + buf[0] = addr & 0xff; Seems pointless to use & 0xff when both buf and addr are u8 > + for (i = 0; i < len; i++) > + buf[i + 1] = *data++; memcpy.  Maybe: buf[0] = addr; memcpy(&buf[1], data, len); > +static int raydium_i2c_send_message(struct i2c_client *client, > + size_t len, void *data) > +{ > + int error; > + u8 buf[HEADER_SIZE], ii; > + > + for (ii = 0; ii < HEADER_SIZE; ii++) > + buf[ii] = ((u8 *)data)[3 - ii]; > + /*set data bank*/ > + error = raydium_i2c_send(client, CMD_BANK_SWITCH, > + (u8 *)buf, HEADER_SIZE); > + > + /*send messange*/ typo, message > + if (!error) > + error = raydium_i2c_send(client, buf[ADDR_INDEX], buf, len); > + > + return error; > +} > + > +static int raydium_i2c_sw_reset(struct i2c_client *client) > +{ > + const u8 soft_rst_cmd[] = { 0x01, 0x04, 0x00, 0x00, 0x40}; static > + int error; > + > + error = raydium_i2c_send_message(client, > + 1, (void *)soft_rst_cmd); could be better on a single line > +static int raydium_i2c_fastboot(struct i2c_client *client) > +{ > + const u8 boot_cmd[] = { 0x20, 0x06, 0x00, 0x50 }; > + u8 buf[HEADER_SIZE]; > + int error; > + > + error = raydium_i2c_read_message(client, > + get_unaligned_be32(boot_cmd), > + sizeof(boot_cmd), > + buf); > + > + if (error) { > + dev_err(&client->dev, "boot failed: %d\n", error); > + return error; > + } else if (buf[0] == RM_BOOT_BLDR) { unnecessary else > + dev_dbg(&client->dev, "boot in fastboot mode\n"); > + return -EINVAL; > + } > + > + dev_dbg(&client->dev, "boot success -- 0x%x\n", client->addr); > + return 0; > +} > + > +static int raydium_i2c_initialize(struct raydium_data *ts) > +{ > + struct i2c_client *client = ts->client; > + int error, retry_cnt; > + const u8 recov_packet[] = { 0x04, 0x81 }; static and etc... generally, it'd be nice to align to open parenthesis and to more maximally fill lines to 80 chars instead of using relatively short line lengths and multiple line statements.