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 E5989C00144 for ; Mon, 1 Aug 2022 04:20:00 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S239148AbiHAET7 (ORCPT ); Mon, 1 Aug 2022 00:19:59 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:50104 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S239136AbiHAET6 (ORCPT ); Mon, 1 Aug 2022 00:19:58 -0400 Received: from mail-io1-xd2c.google.com (mail-io1-xd2c.google.com [IPv6:2607:f8b0:4864:20::d2c]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id E315F13DD8 for ; Sun, 31 Jul 2022 21:19:49 -0700 (PDT) Received: by mail-io1-xd2c.google.com with SMTP id x64so7609732iof.1 for ; Sun, 31 Jul 2022 21:19:49 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=konsulko.com; s=google; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc; bh=7y69nF8yylApvNF8tnbnFLvYv1tkhmPPUVUPa2891qc=; b=DdXTOmV+TuIZy/mE4W6jNZd6uiwDY7MDmCSWRkBNW3cgfAqAI38xyqbn6VVnzEEAhb 8G4MUMe9Hy5VxVs9CTBDnrEEMkIReFp4kcAdZ6D9CR9PFIdm+C6XGZE1hd1vdvN00oOL 2RcgT4WsE2Y7qBZ8/6ootJRrgq5EGDvO0G2QI= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc; bh=7y69nF8yylApvNF8tnbnFLvYv1tkhmPPUVUPa2891qc=; b=ACbCfdPWr7YQGs/xzQsZ8Kp9zhniGslZYaD5P6UQu5z1ks+7bOC/aOvnpbZ3gbAmF9 XfaAX8rYvCZUuDExEG7Ibw2RTE5H3r0mvL+lErbiks8PAbsRPWzlImn15LHWyS/Dw7dS 4NKf/MYVAjUdQon1j7dBm+UrpRpFq89f46c06Zc6i+CdJjxHAs8XXPrJJHXT1mUu5RFg FBIfO8w0KlIRTNIlcoVAqHHiV5tSOHR14yLSYUOoPy/eltknPFu4Jzq/hoyMvCOfW2pP lnlg0R24HQRPcsZNweGUtpzvLTGZSs0W2fyGYJ2pDMoO2aLUFSq+JOKTyP8NqzVDEaHN ya4Q== X-Gm-Message-State: AJIora8TIFRPZbOG6KcqKNtpPM34gH/96JwH7Xy0HEZi73SoHZvAZkBn Ged6zNNb8c4OIqHkiqRk1l/U9xFBJA58cu9boADIRg== X-Google-Smtp-Source: AGRyM1tBcfkws8S5Mv3ZWo4TZS8WuMZ7q8sUQvrr9rAzPOy8paIkj6HqvTFTxnv3QpKTmLVLyVlVJB1j6A/PSngNKz0= X-Received: by 2002:a05:6638:dd1:b0:341:55c2:38b6 with SMTP id m17-20020a0566380dd100b0034155c238b6mr5539717jaj.245.1659327589288; Sun, 31 Jul 2022 21:19:49 -0700 (PDT) MIME-Version: 1.0 References: <20220729154723.99947-1-matt.ranostay@konsulko.com> In-Reply-To: From: Matt Ranostay Date: Mon, 1 Aug 2022 12:19:37 +0800 Message-ID: Subject: Re: [PATCH] HID: mcp2221: add ADC/DAC support via iio subsystem To: Andy Shevchenko Cc: linux-input , linux-iio , Rishi Gupta , Jonathan Cameron Content-Type: text/plain; charset="UTF-8" Precedence: bulk List-ID: X-Mailing-List: linux-input@vger.kernel.org On Mon, Aug 1, 2022 at 3:11 AM Andy Shevchenko wrote: > > On Fri, Jul 29, 2022 at 5:49 PM Matt Ranostay > wrote: > > > > Add support for 3x 10-bit ADC and 1x DAC channels registered via > > the iio subsystem. > > > > To prevent breakage and unexpected dependencies this support only is > > only built if CONFIG_IIO is enabled, and is only weakly referenced by > > 'imply IIO' within the respective Kconfig. > > > > Additionally the iio device only gets registered if at least one channel > > is enabled in the power-on configuration read from SRAM. > > I tried to leave the comments not clashed with Jonathan's ones below. > > ... > > > Cc: Rishi Gupta > > Cc: Jonathan Cameron > > Use --cc in the parameters to `git format-patch` or move them after > the cutter '---' line below, so they won't pollute the Git commit > message. Noted for the future. > > ... > > > - depends on GPIOLIB > > + select GPIOLIB > > I'm not sure why. Changed to select from 'depends on' to avoid this circular dependency 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 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 For a resolution refer to Documentation/kbuild/kconfig-language.rst > > ... > > > #include > > #include > > #include > > + blank line. > > > +#include > > +#include > > + blank line. > > > #include "hid-ids.h" > > > +#if IS_REACHABLE(CONFIG_IIO) > > + struct iio_chan_spec iio_channels[3]; > > + struct iio_dev *indio_dev; > > + u16 adc_values[3]; > > + u8 dac_value; > > +#endif > > +}; > > ... > > > +#if IS_REACHABLE(CONFIG_IIO) > > + if (mcp->indio_dev) > > + memcpy(&mcp->adc_values, &data[50], 6); > > sizeof() sizeof(mcp->adc_values) would work here if needed. > > > +#endif > > ... > > > + // Confirm value is within 10-bit range > > + if (*val > GENMASK(9, 0)) > > if (*val >= BIT(10)) > > will make comment useless > > > + return -EINVAL; > > + } > > ... > > > + if (val < 0 || val > GENMASK(4, 0)) > > In a similar way, val >= BIT(5). > > > + return -EINVAL; > > ... > > > + 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.. > > ... > > > + ret = mcp_send_data_req_status(mcp, mcp->txbuf, 12); > > Ditto, See above. > > > + hid_hw_power(mcp->hdev, PM_HINT_NORMAL); > > Even in an error case? hid_hw_power was set to PM_HINT_FULLON before the check, and in error case it should reset back to normal hint. > > > + if (ret) { > > + mutex_unlock(&mcp->lock); > > + return -EINVAL; > > + } > > ... > > > +#if IS_REACHABLE(CONFIG_IIO) > > + 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); > > +#endif > > ... > > 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 - Matt > > -- > With Best Regards, > Andy Shevchenko