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 E0D0CC77B7A for ; Fri, 2 Jun 2023 14:49:20 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S236220AbjFBOtT (ORCPT ); Fri, 2 Jun 2023 10:49:19 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:47574 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S236228AbjFBOtS (ORCPT ); Fri, 2 Jun 2023 10:49:18 -0400 Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 4D8CEE49; Fri, 2 Jun 2023 07:49:15 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1685717355; x=1717253355; h=date:from:to:cc:subject:message-id:references: mime-version:in-reply-to; bh=YPUlxeS32cApP+ZDIUHR9khrq40A8i+TVQo/LyymAAg=; b=iSIFuCLsrzjIhWQBq0iHm1gHLZ5Fe81EIAgx/XJKBBeb5h4s9L/vuxNi sxbmnLW1Vnpkz3AhKgcybk4fgrK2BzmQ+6gqei9fF0FkGprGa9olHloGP xbX4agugBUo/X5HDCdot1VlKxug48zEhqKIluLCyE89uT7YoWFAMR3dwT YiFJ9I1SCjzWW4pXvbbWu+vcrodW4esE9qEgmGm+AmQvW+g9TyP3NlnQ/ 4FFQHCk5TwMl0rQbmioL314poFzQMNdXl10QfJc4pI33sR3WqYza4DiSN rNi+V4AieD9jdYVpu8Wv6UVZyzPUrz4tEzMO9KgE1ijDl4a/VnxI/bOWP Q==; X-IronPort-AV: E=McAfee;i="6600,9927,10729"; a="353378802" X-IronPort-AV: E=Sophos;i="6.00,213,1681196400"; d="scan'208";a="353378802" Received: from fmsmga008.fm.intel.com ([10.253.24.58]) by fmsmga102.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 02 Jun 2023 07:49:14 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=McAfee;i="6600,9927,10729"; a="772905903" X-IronPort-AV: E=Sophos;i="6.00,213,1681196400"; d="scan'208";a="772905903" Received: from smile.fi.intel.com ([10.237.72.54]) by fmsmga008.fm.intel.com with ESMTP; 02 Jun 2023 07:49:07 -0700 Received: from andy by smile.fi.intel.com with local (Exim 4.96) (envelope-from ) id 1q565Q-000fQj-0X; Fri, 02 Jun 2023 17:49:04 +0300 Date: Fri, 2 Jun 2023 17:49:03 +0300 From: Andy Shevchenko To: Maksim Kiselev Cc: linux-iio@vger.kernel.org, Jonathan Cameron , Lars-Peter Clausen , Rob Herring , Krzysztof Kozlowski , Conor Dooley , Chen-Yu Tsai , Jernej Skrabec , Samuel Holland , Paul Walmsley , Palmer Dabbelt , Albert Ou , Philipp Zabel , Cristian Ciocaltea , Cosmin Tanislav , Miquel Raynal , ChiaEn Wu , Arnd Bergmann , Ramona Bolboaca , Caleb Connolly , ChiYuan Huang , Ibrahim Tilki , William Breathitt Gray , Leonard =?iso-8859-1?Q?G=F6hrs?= , Haibo Chen , Hugo Villeneuve , Mike Looijmans , devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-sunxi@lists.linux.dev, linux-kernel@vger.kernel.org, linux-riscv@lists.infradead.org Subject: Re: [PATCH v2 1/3] iio: adc: Add Allwinner D1/T113s/R329/T507 SoCs GPADC Message-ID: References: <20230601223104.1243871-1-bigunclemax@gmail.com> <20230601223104.1243871-2-bigunclemax@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20230601223104.1243871-2-bigunclemax@gmail.com> Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo Precedence: bulk List-ID: X-Mailing-List: devicetree@vger.kernel.org On Fri, Jun 02, 2023 at 01:30:39AM +0300, Maksim Kiselev wrote: > From: Maxim Kiselev > > The General Purpose ADC (GPADC) can convert the external signal into > a certain proportion of digital value, to realize the measurement of > analog signal, which can be applied to power detection and key detection. > > Theoretically, this ADC can support up to 16 channels. All SoCs below > contain this GPADC IP. The only difference between them is the number > of available channels: > > T113 - 1 channel > D1 - 2 channels > R329 - 4 channels > T507 - 4 channels ... > +struct sun20i_gpadc_iio { > + struct regmap *regmap; > + struct completion completion; > + struct mutex lock; The locks should be explained (what are they for? what do they protect?). > + int lastch; > +}; ... > +static const struct regmap_config sun20i_gpadc_regmap_config = { > + .reg_bits = 32, > + .val_bits = 32, > + .reg_stride = 4, > + .fast_io = true, I forgot if I asked about regmap lock do you need it? > +}; ... > + if (!wait_for_completion_timeout(&info->completion, > + msecs_to_jiffies(100))) { Dunno if it's better to have this parameter to be defined with self-explanatory name. > + ret = -ETIMEDOUT; > + goto err; > + } ... > +err: err_unlock: > + mutex_unlock(&info->lock); > + > + return ret; ... > + switch (mask) { > + case IIO_CHAN_INFO_RAW: > + ret = sun20i_gpadc_adc_read(info, chan, val); > + return ret; return sun...; > + case IIO_CHAN_INFO_SCALE: > + /* value in mv = 1800mV / 4096 raw */ > + *val = 1800; > + *val2 = 12; > + return IIO_VAL_FRACTIONAL_LOG2; > + default: > + return -EINVAL; > + } ... > + if (num_channels > SUN20I_GPADC_MAX_CHANNELS) { > + dev_err(dev, "num of channel children out of range"); > + return -EINVAL; > + } Is it really critical error? ... > + channels = devm_kcalloc(dev, num_channels, > + sizeof(*channels), GFP_KERNEL); At least one more parameter can be located on the previous line. > + if (!channels) > + return -ENOMEM; ... > +err_child_out: err_put_child: The goto labels should be self-explanatory of what to expect when goto. > + fwnode_handle_put(node); > + > + return ret; ... > + ret = devm_request_irq(dev, irq, sun20i_gpadc_irq_handler, > + 0, dev_name(dev), info); > + if (ret < 0) Here... > + return dev_err_probe(dev, ret, > + "failed requesting irq %d\n", irq); ... > + ret = devm_iio_device_register(dev, indio_dev); > + if (ret < 0) ...and here, do the positive returned values even possible? > + return dev_err_probe(dev, ret, > + "could not register the device\n"); ... > + { .compatible = "allwinner,sun20i-d1-gpadc", }, Inner comma is not needed. -- With Best Regards, Andy Shevchenko