From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-3.8 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 567D5C433E2 for ; Sun, 6 Sep 2020 12:09:50 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 111C8208B3 for ; Sun, 6 Sep 2020 12:09:50 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726342AbgIFMJs (ORCPT ); Sun, 6 Sep 2020 08:09:48 -0400 Received: from relay8-d.mail.gandi.net ([217.70.183.201]:50543 "EHLO relay8-d.mail.gandi.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1725788AbgIFMJf (ORCPT ); Sun, 6 Sep 2020 08:09:35 -0400 Received: from webmail.gandi.net (webmail15.sd4.0x35.net [10.200.201.15]) (Authenticated sender: contact@artur-rojek.eu) by relay8-d.mail.gandi.net (Postfix) with ESMTPA id EFD411BF203; Sun, 6 Sep 2020 12:09:28 +0000 (UTC) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII; format=flowed Content-Transfer-Encoding: 7bit Date: Sun, 06 Sep 2020 14:09:28 +0200 From: Artur Rojek To: Andy Shevchenko Cc: Dmitry Torokhov , Rob Herring , Mark Rutland , Jonathan Cameron , Paul Cercueil , Heiko Stuebner , Ezequiel Garcia , linux-input , devicetree , Linux Kernel Mailing List Subject: Re: [PATCH v9 2/2] input: joystick: Add ADC attached joystick driver. In-Reply-To: References: <20200905163403.64390-1-contact@artur-rojek.eu> <20200905163403.64390-2-contact@artur-rojek.eu> Message-ID: <2f2047e7ada6fcb70489ea6e5917e20a@artur-rojek.eu> X-Sender: contact@artur-rojek.eu User-Agent: Roundcube Webmail/1.3.15 Sender: devicetree-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: devicetree@vger.kernel.org Hi Andy, thanks for the review, replies inline. On 2020-09-06 11:22, Andy Shevchenko wrote: > On Sat, Sep 5, 2020 at 7:34 PM Artur Rojek > wrote: >> >> Add a driver for joystick devices connected to ADC controllers >> supporting the Industrial I/O subsystem. > > ... > >> +static int adc_joystick_handle(const void *data, void *private) >> +{ >> + struct adc_joystick *joy = private; >> + enum iio_endian endianness; >> + int bytes, msb, val, idx, i; >> + bool sign; >> + >> + bytes = joy->chans[0].channel->scan_type.storagebits >> 3; >> + >> + for (i = 0; i < joy->num_chans; ++i) { >> + idx = joy->chans[i].channel->scan_index; >> + endianness = >> joy->chans[i].channel->scan_type.endianness; >> + msb = joy->chans[i].channel->scan_type.realbits - 1; > >> + sign = (tolower(joy->chans[i].channel->scan_type.sign) >> == 's'); > > Redundant parentheses. > >> + switch (bytes) { >> + case 1: >> + val = ((const u8 *)data)[idx]; >> + break; >> + case 2: > >> + if (endianness == IIO_BE) >> + val = be16_to_cpu(((const __be16 >> *)data)[idx]); >> + else if (endianness == IIO_LE) >> + val = le16_to_cpu(((const __le16 >> *)data)[idx]); >> + else /* IIO_CPU */ >> + val = ((const u16 *)data)[idx]; >> + break; > > Hmm... I don't like explicit castings to restricted types. On top of > that is it guaranteed that pointer to data will be aligned? The buffer comes from the IIO core, it is aligned to the sample size. > As a solution for the first two I would recommend to use > get_unaligned_be16() / get_unaligned_le16(). > The last one is an interesting case and if data can be unaligned needs > to be fixed. > >> + default: >> + return -EINVAL; >> + } >> + >> + val >>= joy->chans[i].channel->scan_type.shift; >> + if (sign) >> + val = sign_extend32(val, msb); >> + else > >> + val &= GENMASK(msb, 0); > > It includes msb. Is it expected? Yes, that's expected as `msb = joy->chans[i].channel->scan_type.realbits - 1`. > >> + input_report_abs(joy->input, joy->axes[i].code, val); >> + } >> + >> + input_sync(joy->input); >> + >> + return 0; >> +} > > ... > >> +static int adc_joystick_open(struct input_dev *dev) > >> +static void adc_joystick_close(struct input_dev *dev) > > Just wondering if this is protected against object lifetime cases. Can you clarify that in more details? > > ... > >> +err: > > err_fwnode_put: ? > >> + fwnode_handle_put(child); >> + return ret; > > ... > >> + /* Count how many channels we got. NULL terminated. */ >> + for (i = 0; joy->chans[i].indio_dev; ++i) { >> + bits = joy->chans[i].channel->scan_type.storagebits; >> + if (!bits || (bits > 16)) { >> + dev_err(dev, "Unsupported channel storage >> size\n"); > >> + return -EINVAL; > > -ERANGE? > >> + } >> + if (bits != >> joy->chans[0].channel->scan_type.storagebits) { >> + dev_err(dev, "Channels must have equal storage >> size\n"); >> + return -EINVAL; >> + } >> + }