From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andi Shyti Subject: Re: [PATCH] Input: add support for the Samsung S6SY761 touchscreen Date: Mon, 25 Sep 2017 11:47:25 +0900 Message-ID: <20170925024725.GH2957@gangnam> References: <20170921132950.17452-1-andi.shyti@samsung.com> <20170921205607.GA15858@dtor-ws> <20170922041702.GE2957@gangnam> <20170922171410.GD24037@dtor-ws> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Return-path: Content-disposition: inline In-reply-to: <20170922171410.GD24037@dtor-ws> Sender: devicetree-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org To: Dmitry Torokhov Cc: Rob Herring , linux-input-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Andi Shyti List-Id: linux-input@vger.kernel.org Hi Dmitry, > > > > +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). > > What happens if you get event[0] == S6SY761_EVENT_ID_COORDINATE? I.e. > the value is non-zero, but tid component is 0? Oh, I see what you mean. It shouldn't happen, in anycase I can put it under an 'unlikely' statement. > > > > + 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'. > > But you do not need interrupts to read x and y, right? So you can power > device, create input device, set it up as needed, and then request irq, > or am I missing something? OK, all right. I'll do that. I will move the irq request after the input registration. Thanks again for your review, Andi -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753818AbdIYCrO (ORCPT ); Sun, 24 Sep 2017 22:47:14 -0400 Received: from mailout3.samsung.com ([203.254.224.33]:18875 "EHLO mailout3.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752884AbdIYCrK (ORCPT ); Sun, 24 Sep 2017 22:47:10 -0400 X-AuditID: b6c32a45-d3bff70000001088-5b-59c86e2b9806 Date: Mon, 25 Sep 2017 11:47:25 +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: <20170925024725.GH2957@gangnam> MIME-version: 1.0 Content-type: text/plain; charset="us-ascii" Content-disposition: inline In-reply-to: <20170922171410.GD24037@dtor-ws> User-Agent: Mutt/1.9.0 (2017-09-02) X-Brightmail-Tracker: H4sIAAAAAAAAA01SW0wTURD1stvtQlhdCugVAzZLTASh0AplITxU0FT0A2OiolHcwE1B+0q3 RfEHRETolw0YsYACiiQ1KhBSwXcoL018JCj4DtYoAsYPCoIiiW0XE/7O3DlnzszcITHJuCiM LNKZkFHHaRgiAHc4o5JiN+mGcuN7f29kr86P+7FX+p6LWGfLd8C+nf4lYofvNhDs2Qd94i2E avTaNKbqsX0Uqzrt1YTK3RmRgx9EqYWIK0BGKdLl6wuKdOo0ZtfevMy8RGW8PFaezCYxUh2n RWlM1u6c2B1FGo85Iy3mNGbPUw7H80xceqpRbzYhaaGeN6Uxh+RyhUwenyRTKBSyhM2HUxSJ HspRVDjgmhAbfgadbBx7gZcBB2UBJAnpBOh+xVtAACmhuwF0trtxIZgDsKNiDrMAfx/pdusj QkjcA/BjjVMsBBMAtj524N5SOL0BzjSnewUEHQXL388TXhxCx8E79X+Al4/RlwG8/r5L7E0E 07uga/aFD1P0Jjj80oILOAjO13zyYYyOgc8GbgEBr4OuxTof35+OhW+a3D6DUDoSfl584Gsb 0l0EdJ4bIIS2s+CI9QMu4GA4OSgYQ0+hb/YOIAjOANjuGPYTggoAFyZeL6k3w6eW036C9UpY 5VwUCxujYFWlRIAq2NQaI7C3woW2+0u7m/VM+aWeOA/CbcsGsi0byLZsoCaA2cFqZOC1asQr DHIZz2l5s04ty9drO4HvyKK3d4O657t7AU0CJpBa8XcwVyLiivkSbS+AJMaEUJNFQ7kSqoAr OYWM+jyjWYP4XpDo+RQrFhaar/ecrM6UJ09Ijk9QKhVJStZzVmuoUsfIAQmt5kzoOEIGZPyv 8yP9w8qAqHtPYLbV2HbkB1sLKvtvBt0I+VppUUfYyf02a3Z75hDh/n44Stm8/k3XWGLMzLvJ rIdTgSfKx98WT0912y+trX72ZMzq6mlxl2bsnEu9aCbDpQ8z6qiZ4BTqndY/LcbsFO1DQ8cM q8zz7n5HQ+5oRWR0U+22Cx0RrgAtDGxkcL6Qk0djRp77Bzszjqp6AwAA X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFjrNLMWRmVeSWpSXmKPExsVy+t9jAV3tvBORBv/2yFks/vGcyWL+kXOs FocXvWC0uPnpG6vF5V1z2Cxa9x5hd2DzuL7kE7PHzll32T02repk8/i8SS6AJYrLJiU1J7Ms tUjfLoEr49ijl+wF7wQr5j44z9LAuI23i5GTQ0LARGL90v1sXYxcHEICOxklnh1qhnJeMkp8 6G5l6WLk4GARUJX4stAOpIFNQFOi6fYPNhBbREBfYvvsX4wg9cwC8xglLr1awg6SEBbwlnj0 9TyYzSugLXH5QhcLxNDvjBIL511hgkgISvyYfI8FxGYW0JJYv/M4E4QtLfHo7wywZk4BXYkb Cz6DbRMVUJZ4+HcvywRG/llI2mchaZ+FpH0BI/MqRsnUguLc9NxiowKjvNRyveLE3OLSvHS9 5PzcTYzAIN52WKt/B+PjJfGHGAU4GJV4eCP+HY8UYk0sK67MPcQowcGsJML7KvNEpBBvSmJl VWpRfnxRaU5q8SFGaQ4WJXFe/vxjkUIC6YklqdmpqQWpRTBZJg5OqQbGZZueL4qunvJs5rJX x5d2S605m3HTcn4lX7JJm5T6/Q2Gvku7q6/cehme4HF7c5V2/a8jj9o+igW9YZc4Fhmy129b 2AqF70cjFrcwX3hzkDX6u++J2om6bC3Tl7L5hvFeu+f4tz14kWbBd78nh6fV9O7W33pkwy91 IQsN8z+M+6bc+Nyr9sN6qxJLcUaioRZzUXEiALdbgOleAgAA X-CMS-MailID: 20170925024707epcas2p167512949ea62502fe00ca222b8143791 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> <20170922041702.GE2957@gangnam> <20170922171410.GD24037@dtor-ws> Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Dmitry, > > > > +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). > > What happens if you get event[0] == S6SY761_EVENT_ID_COORDINATE? I.e. > the value is non-zero, but tid component is 0? Oh, I see what you mean. It shouldn't happen, in anycase I can put it under an 'unlikely' statement. > > > > + 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'. > > But you do not need interrupts to read x and y, right? So you can power > device, create input device, set it up as needed, and then request irq, > or am I missing something? OK, all right. I'll do that. I will move the irq request after the input registration. Thanks again for your review, Andi