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 Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 2FE6FC25B0C for ; Sat, 6 Aug 2022 16:18:49 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S230281AbiHFQSs (ORCPT ); Sat, 6 Aug 2022 12:18:48 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:44416 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S230128AbiHFQSr (ORCPT ); Sat, 6 Aug 2022 12:18:47 -0400 Received: from dfw.source.kernel.org (dfw.source.kernel.org [IPv6:2604:1380:4641:c500::1]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 2BECB659C; Sat, 6 Aug 2022 09:18:46 -0700 (PDT) Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dfw.source.kernel.org (Postfix) with ESMTPS id BB94260E04; Sat, 6 Aug 2022 16:18:45 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 83AFEC433D7; Sat, 6 Aug 2022 16:18:43 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1659802725; bh=GqybX519p1IsieOaAtqe8J7TdeClhx1gw0hFu/BXJ8E=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=DZ6P/+DOGMuth/bQCjdVqp3kzAy1ejRgTndNJU7jdzNLJS/c8BN1kmMAOfHvZnhwL tCf6cXa3KuIRg6rJb8NHbw/CErP5M1yCNXoX/cuuezVCUTEV1bJ/G0bG2RdLXp+4z+ 4std5m0OGFACaRw5BVRxuzSblpb5iilYUPrLKJtRG+V1wO6gMg4gEzmkxu07J/kpHk TAcF7fF1Ud84kJv5hVc6iUUNZL6YO4SmFVE65AIYLf2tflcRBsC5/83LqeFCcXuuPB 4F2WHCcanBo+ET1v1NWCvWBlq56D7qJ+rhdZYJzhj/hlOiFaSlRbrtN8iLlNH1aG9A PTgZrlxntw1zQ== Date: Sat, 6 Aug 2022 17:29:05 +0100 From: Jonathan Cameron To: Andy Shevchenko Cc: Matt Ranostay , linux-input , linux-iio , Rishi Gupta , Jonathan Cameron Subject: Re: [PATCH] HID: mcp2221: add ADC/DAC support via iio subsystem Message-ID: <20220806172905.3755bdc0@jic23-huawei> In-Reply-To: References: <20220729154723.99947-1-matt.ranostay@konsulko.com> X-Mailer: Claws Mail 4.1.0 (GTK 3.24.34; x86_64-pc-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Precedence: bulk List-ID: X-Mailing-List: linux-input@vger.kernel.org On Mon, 1 Aug 2022 11:08:39 +0200 Andy Shevchenko wrote: > On Mon, Aug 1, 2022 at 6:19 AM Matt Ranostay wrote: > > On Mon, Aug 1, 2022 at 3:11 AM Andy Shevchenko > > wrote: > > > On Fri, Jul 29, 2022 at 5:49 PM Matt Ranostay > > > wrote: > > First of all, please, remove unneeded context when replying! > (And I believe that non-commented stuff will be addressed as suggested) > > ... > > > > > - depends on GPIOLIB > > > > + select GPIOLIB > > > > > > I'm not sure why. > > > > Changed to select from 'depends on' to avoid this circular dependency > > Was it before your patch? If so, it should be addressed separately as a fix. > > > SYNC include/config/auto.conf.cmd > > drivers/gpio/Kconfig:14:error: recursive dependency detected! > > > drivers/gpio/Kconfig:14: symbol GPIOLIB is selected by I2C_MUX_LTC4306 > > Isn't it the real problem here? > > > drivers/i2c/muxes/Kconfig:47: symbol I2C_MUX_LTC4306 depends on I2C_MUX > > drivers/i2c/Kconfig:62: symbol I2C_MUX is selected by MPU3050_I2C > > drivers/iio/gyro/Kconfig:127: symbol MPU3050_I2C depends on IIO > > drivers/iio/Kconfig:6: symbol IIO is implied by HID_MCP2221 > > drivers/hid/Kconfig:1298: symbol HID_MCP2221 depends on GPIOLIB > > ... > > > > > + if (mcp->indio_dev) > > > > + memcpy(&mcp->adc_values, &data[50], 6); > > > > > > sizeof() > > > > sizeof(mcp->adc_values) would work here if needed. > > You need to write code to be more robust, using hardcoded magics when > it's easy to derive is not good practice. > > ... > > > > > + memset(mcp->txbuf, 0, 12); > > > > > > sizeof() ? > > > > txbuf isn't 12 bytes long but 64 since that is the full max size a HID > > transaction could > > have. So sizeof() won't work in these cases.. > > I see, what about a specific definition with a self-explanatory name? > > ... > > > > > + ret = mcp_send_data_req_status(mcp, mcp->txbuf, 12); > > > > > > Ditto, > > > > See above. > > See above. > > ... > > > > > + if (mcp->indio_dev) > > > > > > Do you need this check? > > > > Yes basically if no ADC or DAC channel is enabled then no iio_device > > get allocated or registered. > > > > > + iio_device_unregister(mcp->indio_dev); > > So, we have an inconvenience in the iio_device_unregister(), i.e. it > doesn't perform the NULL-check by itself. I recommend fixing it there > and dropping this check in the caller. This is standard practice in > the Linux kernel for resource deallocator APIs. ah. Now the other patch makes more sense. Make sure to pull this driver an that one together in a series if we are taking that forwards. I agree for allocator APIs but not so much for registration APIs. I checked a few similar ones. input_unregister_device() doesn't hwmon_device_unregister() doesn't. Got bored at that point :) Would be relatively easy to take this driver fully devm_ I think then you can just use devm_iio_device_register() if you have a device to register. > > ... > > > > Overall what I really do not like is that ugly ifdeffery. Can we avoid > > > adding it? > > > > Could make CONFIG_IIO required for building but not sure we really > > want to add as an additional dependency. > > Which is way the imply is set for CONFIG_IIO > > The code looks ugly with this kind of ifdeffery. But okay, I leave it > up to maintainers, just my 2cents. > Agreed. HID maintainer call to make I think. One alternative is the standard approach of spin another file with wrappers around the IIO registration calls, then stub that out in the header if CONFIG_IIO not built appropriately. Jonathan