From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751823AbdIVEQw (ORCPT ); Fri, 22 Sep 2017 00:16:52 -0400 Received: from mailout4.samsung.com ([203.254.224.34]:56672 "EHLO mailout4.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751329AbdIVEQu (ORCPT ); Fri, 22 Sep 2017 00:16:50 -0400 X-AuditID: b6c32a45-f79466d000002ac6-5d-59c48eafc591 Date: Fri, 22 Sep 2017 13:17:02 +0900 From: Andi Shyti To: Dmitry Torokhov Cc: Rob Herring , linux-input@vger.kernel.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, Andi Shyti Subject: Re: [PATCH] Input: add support for the Samsung S6SY761 touchscreen Message-id: <20170922041702.GE2957@gangnam> MIME-version: 1.0 Content-type: text/plain; charset="us-ascii" Content-disposition: inline In-reply-to: <20170921205607.GA15858@dtor-ws> User-Agent: Mutt/1.9.0 (2017-09-02) X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFvrCKsWRmVeSWpSXmKPExsWy7bCmqe76viORBrcXaVgs/vGcyWL+kXOs FocXvWC0uPnpG6vF5V1z2Cxa9x5hd2DzuL7kE7PHzll32T02repk8/i8SS6AJSrVJiM1MSW1 SCE1Lzk/JTMv3VbJOzjeOd7UzMBQ19DSwlxJIS8xN9VWycUnQNctMwdouZJCWWJOKVAoILG4 WEnfzqYov7QkVSEjv7jEVina0NBIz9DAXM/IyEjPxDjWysgUqCQhNWP1o6tMBTfkKjat2MXW wDhZvIuRk0NCwETiwe/7TBC2mMSFe+vZuhi5OIQEdjBKHDx4nxXC+c4oce3pb0aYjqfHNjFC JDYwSly//4AdwnnJKHHhZD87SBWLgKpE99IjYB1sApoSTbd/sIHYIgL6Ettn/wLrZhaYxyix 7PYWsAZhAW+JR1/Pg9m8AtoSzXd6GCFsQYkfk++xgNjMAjoSZ4+tY4SwpSUe/Z0BVs8poCtx a91NZhBbVEBZ4uHfvSwgCyQENrBJzP5/jgXibheJrwe+QNnCEq+OQyyWABr0bNVGRoiGZkaJ DdsuM0E4LYwSv19eZYOoMpY41dXIBLGaT6Lj8F+gbg6gOK9ER5sQhOkhsWCpDkS1o8Tv5XvA dgkJbAIa80F0AqPcLCT/zELyzywk/yxgZF7FKJZaUJybnlpsVGCoV5yYW1yal66XnJ+7iRGc 4rRcdzDOOOdziFGAg1GJh9fg4OFIIdbEsuLK3EOMEhzMSiK8+1uORArxpiRWVqUW5ccXleak Fh9iNAVGykRmKdHkfGD6zSuJNzSxNDAxMzMyN7MApjFx3vpt1yKEBNITS1KzU1MLUotg+pg4 OKUaGAVN1wu9X3EoQcxFPF+EV3+j4E0Fu2Ku+be/PNilnvM4LpHP/h5/61bDBv2Fd09uTg/s 97ia9fOVkVx0Xn38gUkqHy4t+TZlv46nCotLwVp5W2N91xW/Xs9ruXfs/P/5vutOGyy0bJ+U dketWETgdMtsDYWVn8MXWB2d+JfxzzXp+cKfmyRKlymxFGckGmoxFxUnAgBUQl30hwMAAA== X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFjrDLMWRmVeSWpSXmKPExsVy+t9jQd31fUciDa5vFLdY/OM5k8X8I+dY LQ4vesFocfPTN1aLy7vmsFm07j3C7sDmcX3JJ2aPnbPusntsWtXJ5vF5k1wASxSXTUpqTmZZ apG+XQJXxupHV5kKbshVbFqxi62BcbJ4FyMnh4SAicTTY5sYuxi5OIQE1jFKbGw8wAThvGSU 6FnWxw5SxSKgKtG99AgjiM0moCnRdPsHG4gtIqAvsX32L7BuZoF5jBKXXi0BaxAW8JZ49PU8 mM0roC3RfKcHasUWRokLS05CJQQlfky+xwJiMwtoSazfeZwJwpaWePR3BlgNp4CuxK11N5lB bFEBZYmHf/eyTGDkn4WkfRaS9llI2hcwMq9ilEwtKM5Nzy02KjDKSy3XK07MLS7NS9dLzs/d xAgM422Htfp3MD5eEn+IUYCDUYmH1+Dg4Ugh1sSy4srcQ4wSHMxKIrz7W45ECvGmJFZWpRbl xxeV5qQWH2KU5mBREufN7JsRKSSQnliSmp2aWpBaBJNl4uCUamCsmm4sJcxZnb6x8Vl7/FmX wO9aBhI7O1t9rZfM9/yvwfX9x7xi5qc5Kko5LqJiJUfKivbeO23y5e3+J8rns54xa63MmPLa ZmGWfx9nzjQn30N5zeHsvxKuH5xa3szhFP5G+pRUxJ1F9lNUv5433H1TJm2iyjvfqIYNIUK8 y/n+u8clp/fnflZiKc5INNRiLipOBACvYnATXwIAAA== X-CMS-MailID: 20170922041647epcas2p27afd735f71fd4d8bd211398434c95756 X-Msg-Generator: CA X-Sender-IP: 182.195.42.143 X-Local-Sender: =?UTF-8?B?7JWI65SUG1RpemVuIFBsYXRmb3JtIExhYihTL1fshLzthLAp?= =?UTF-8?B?G+yCvOyEseyghOyekBtTZW5pb3IgRW5naW5lZXI=?= X-Global-Sender: =?UTF-8?B?QW5kaSBTaHl0aRtUaXplbiBQbGF0Zm9ybSBMYWIuG1NhbXN1?= =?UTF-8?B?bmcgRWxlY3Ryb25pY3MbU2VuaW9yIEVuZ2luZWVy?= X-Sender-Code: =?UTF-8?B?QzEwG1RFTEUbQzEwVjgxMTE=?= CMS-TYPE: 102P DLP-Filter: Pass X-CFilter-Loop: Reflected X-CMS-RootMailID: 20170921132940epcas2p35b501f1ccc79d55c0427bb1ed36e10c6 X-RootMTR: 20170921132940epcas2p35b501f1ccc79d55c0427bb1ed36e10c6 References: <20170921132950.17452-1-andi.shyti@samsung.com> <20170921205607.GA15858@dtor-ws> Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Dmitry, thanks for your review! [...] > > +static void s6sy761_report_coordinates(struct s6sy761_data *sdata, u8 *event) > > +{ > > + u8 tid = ((event[0] & S6SY761_MASK_TID) >> 2) - 1; > > Should we make sure that event[0] & S6SY761_MASK_TID is not 0? I check event[0] already in s6sy761_handle_events (called by the irq handler), if we get here event[0] is for sure positive... [...] > > +static void s6sy761_handle_events(struct s6sy761_data *sdata, u8 left_event) > > +{ > > + int i; > > + > > + for (i = 0; i < left_event; i++) { > > + u8 *event = &sdata->data[i * S6SY761_EVENT_SIZE]; > > + u8 event_id = event[0] & S6SY761_MASK_EID; > > + > > + if (!event[0]) > > + return; ^^^^^^^^ ... exactly here. '!event[0]' means also to me that there is nothing left, therefore I can discard whatever is next (given that there is something left). > > + switch (event_id) { > > + > > + case S6SY761_EVENT_ID_COORDINATE: > > + s6sy761_handle_coordinates(sdata, event); > > + break; > > + > > + case S6SY761_EVENT_ID_STATUS: > > + break; > > + > > + default: > > + break; > > + } > > + } > > +} [...] > > +static ssize_t s6sy761_sysfs_low_power_store(struct device *dev, > > + struct device_attribute *attr, > > + const char *buf, size_t len) > > +{ > > + struct s6sy761_data *sdata = dev_get_drvdata(dev); > > + unsigned long value; > > + s32 ret; > > + u8 new_status; > > + > > + if (kstrtoul(buf, 0, &value)) > > + return -EINVAL; > > + > > + /* > > + * The device does not respond to read/write in low power, > > + * it will enable only in case of external events (e.g. touch). > > + * The i2c read will fail as expected if no external events occur > > + */ > > I am not quite sure how to parse this. Are you saying that the device in > low power mode will wake up when touched? Then your runtime PM > implementation seems incomplete. I was startled as well when I saw this working. It cannot be in the PM runtime because the device would freeze (unless is touched). I don't know if it's a bug in the firmware or this is how it meant to be. > In any case, I'd rather we did not expose this state as a custom > attribute. I can remove it completely, indeed I don't see much use of it. [...] > > + sdata->devid = buffer[1] << 8 | buffer[2]; > > get_unaligned_be16()? Thanks! [...] > > + /* check if both max_x and max_y have a value */ > > + if (unlikely(!sdata->prop.max_x || !sdata->prop.max_y)) > > This is not in hot path, we do not need unlikely() here. OK, Thanks! > > + return -EINVAL; > > + > > + /* if no tx channels defined, at least keep one */ > > + sdata->tx_channel = !buffer[8] ? 1 : buffer[8]; > > sdata->tx_channel = max(buffer[8], 1); Thanks! [...] > > +static int s6sy761_probe(struct i2c_client *client, > > + const struct i2c_device_id *id) > > +{ [...] > > + err = devm_request_threaded_irq(&client->dev, client->irq, NULL, > > + s6sy761_irq_handler, > > + IRQF_TRIGGER_LOW | IRQF_ONESHOT, > > + "s6sy761_irq", sdata); > > + if (err) > > + return err; > > + > > + disable_irq(client->irq); > > Can you request IRQ after allocating and setting up the input device? > Then you do not need to check for its presence in the interrupt handler. The reason I do it here is because the x and y are embedded in the device itself. This means that I first need to enable the device, read x and y and then register the input device. At power up I might expect an interrupt coming, thus I need to check if 'input' is not 'NULL'. [...] > > + err = sysfs_create_group(&sdata->client->dev.kobj, > > + &s6sy761_attribute_group); > > We have devm_device_add_groups() now. Thanks! I will patch also the other driver I sent, then (the stmfts). [...] > Thanks. Thank you! Andi