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 bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 78D79C43458 for ; Wed, 1 Jul 2026 21:11:31 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender:List-Subscribe:List-Help :List-Post:List-Archive:List-Unsubscribe:List-Id:Content-Transfer-Encoding: Content-Type:MIME-Version:References:In-Reply-To:Message-ID:Subject:Cc:To: From:Date:Reply-To:Content-ID:Content-Description:Resent-Date:Resent-From: Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=CtwYQrYYiW6hpQCNYyX45j7zmtq9O7BK7U1u+USXMvo=; b=qFcHjGrBoygamO1Niqc3D8ts+t tmlB7VMJlslGPFmr+0y2sO/RSFmW9yUAoybyTtuEJsXmsuws5ok+FfIwUuP/CqfDDmtVYvlLFdHrH Od7OJG6+5SackTuxUT++eM/m3lHMWvcGk8AaFIsrExQNtJRiaqr/S7Fr3f/bKPr/OUjPDaezLQ/ah gNScJMN+SsSSfwvyhqwq7FVhElV/J1oH82+ZHOXEYvJpD7iuKkKtmRkCeZEq2epbCw+kwnucq0q1K BmiRqaw3VxC/FN2BBNi70DXRzok4wZThYjL0bUuucweUllsVmtWfvb0O18myG1+kvtDevt/RBg8it SxBFM0BA==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.99.1 #2 (Red Hat Linux)) id 1wf2DK-000000031Ia-2Vqn; Wed, 01 Jul 2026 21:11:22 +0000 Received: from sea.source.kernel.org ([2600:3c0a:e001:78e:0:1991:8:25]) by bombadil.infradead.org with esmtps (Exim 4.99.1 #2 (Red Hat Linux)) id 1wf2DJ-000000031IG-0n4f for linux-arm-kernel@lists.infradead.org; Wed, 01 Jul 2026 21:11:21 +0000 Received: from smtp.kernel.org (quasi.space.kernel.org [100.103.45.18]) by sea.source.kernel.org (Postfix) with ESMTP id C6C79416CF; Wed, 1 Jul 2026 21:11:19 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 7A8E61F00A3D; Wed, 1 Jul 2026 21:11:18 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=kernel.org; s=k20260515; t=1782940279; bh=CtwYQrYYiW6hpQCNYyX45j7zmtq9O7BK7U1u+USXMvo=; h=Date:From:To:Cc:Subject:In-Reply-To:References; b=GVr16EHM6F29TG6rx8ee3/BJXvM3iFNK8XlRXFDlrCBPAyR1JOSQJcYN78AwU3h7V Vp4DNtiJZqa4Dnp+T+JcDUoH5155CntFIqOFW7HgirKVue2GtWXELsmPZe9texo2OW ioRXmmdl9g9Mr0oKl7GjzxTizdi2FKjvr9yabqhfuDcJI58SQjlbF23ApzUw9edJM6 ZkypS1p4hlG1qjv7pnTCAoQA6ACiJvyQqkJSzRjxdTkdk8KJyHPxEkuM0yMURh2+in 6mtbHjJVf+I82s9pBJInuFVALmj/lN74Q5Lf5PEGLOf9BM3ePX/mDxqzuA5YjhTUzF YTRm4s/0ADLDQ== Date: Wed, 1 Jul 2026 22:11:15 +0100 From: Jonathan Cameron To: Chi-Wen Weng Cc: robh@kernel.org, krzk+dt@kernel.org, conor+dt@kernel.org, dlechner@baylibre.com, nuno.sa@analog.com, andy@kernel.org, linux-arm-kernel@lists.infradead.org, linux-iio@vger.kernel.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, cwweng@nuvoton.com Subject: Re: [PATCH 2/2] iio: adc: Add Nuvoton MA35D1 EADC driver Message-ID: <20260701221115.544e33fd@jic23-huawei> In-Reply-To: <20260625110638.38438-3-cwweng.linux@gmail.com> References: <20260625110638.38438-1-cwweng.linux@gmail.com> <20260625110638.38438-3-cwweng.linux@gmail.com> X-Mailer: Claws Mail 4.4.0 (GTK 3.24.52; x86_64-pc-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Thu, 25 Jun 2026 19:06:38 +0800 Chi-Wen Weng wrote: > From: Chi-Wen Weng > > Add an IIO driver for the Nuvoton MA35D1 Enhanced ADC controller. > > The driver supports direct raw reads and triggered buffered capture. The > controller end-of-conversion interrupt is exposed as the device trigger > and is used to push samples into the IIO buffer. > > Channels are described by firmware child nodes and can be configured as > single-ended or differential inputs. Since the differential enable bit is > global, mixed single-ended and differential buffered scans are rejected. > > DMA support is intentionally not included in this initial upstream driver; > conversions are handled through the interrupt-driven path. > > Signed-off-by: Chi-Wen Weng A few minor additional comments in line. I've tried to avoid duplication but there may be some there! Jonathan > diff --git a/drivers/iio/adc/ma35d1_eadc.c b/drivers/iio/adc/ma35d1_eadc.c > new file mode 100644 > index 000000000000..0c075126e139 > --- /dev/null > +++ b/drivers/iio/adc/ma35d1_eadc.c ... > +} > + > +static int ma35d1_adc_read_raw(struct iio_dev *indio_dev, > + const struct iio_chan_spec *chan, > + int *val, int *val2, long mask) > +{ > + struct ma35d1_adc *adc = iio_priv(indio_dev); > + int ret; > + > + switch (mask) { > + case IIO_CHAN_INFO_RAW: > + if (!iio_device_claim_direct(indio_dev)) > + return -EBUSY; > + > + mutex_lock(&adc->lock); Look at the ACQUIRE() macros for claim direct stuff (in iio.h) and then use guard() for this. Be careful to add {} to define scope to being this case block. May not save much code but it will be easeir to read than this currently is. > + ret = ma35d1_adc_read_conversion(indio_dev, chan, val); > + mutex_unlock(&adc->lock); > + > + iio_device_release_direct(indio_dev); > + if (ret) > + return ret; > + > + return IIO_VAL_INT; > + default: > + return -EINVAL; > + } > +} > +static int ma35d1_adc_parse_channels(struct iio_dev *indio_dev, > + struct device *dev) > +{ > + struct ma35d1_adc *adc = iio_priv(indio_dev); > + DECLARE_BITMAP(used_channels, MA35D1_EADC_MAX_CHANNELS); > + struct fwnode_handle *child; > + struct iio_chan_spec *channels; > + int num_channels; > + int scan_index = 0; Move assignment down to just above the loop. Makes it easier for reviewers to associate the initial value with what is going on. > + int ret; > + > + bitmap_zero(used_channels, MA35D1_EADC_MAX_CHANNELS); > + > + num_channels = device_get_child_node_count(dev); > + if (!num_channels) > + return dev_err_probe(dev, -ENODATA, > + "no ADC channels configured\n"); > + > + if (num_channels > MA35D1_EADC_MAX_CHANNELS) > + return dev_err_probe(dev, -EINVAL, "too many ADC channels\n"); > + > + channels = devm_kcalloc(dev, num_channels + 1, sizeof(*channels), > + GFP_KERNEL); > + if (!channels) > + return -ENOMEM; > + > + device_for_each_child_node(dev, child) { ... > + > +static int ma35d1_adc_probe(struct platform_device *pdev) > +{ > + struct device *dev = &pdev->dev; > + struct iio_dev *indio_dev; > + struct ma35d1_adc *adc; > + int irq; > + int ret; > + > + indio_dev = devm_iio_device_alloc(dev, sizeof(*adc)); > + if (!indio_dev) > + return -ENOMEM; > + adc = iio_priv(indio_dev); > + adc->dev = dev; > + mutex_init(&adc->lock); For new code ret = devm_mutex_init(&adc->lock); if (ret) return ret; The advantage this brings to debug is small but it's also very simple so we are no doing it for all new code in IIO. > + init_completion(&adc->completion); > + > + adc->regs = devm_platform_ioremap_resource(pdev, 0); > + if (IS_ERR(adc->regs)) > + return dev_err_probe(dev, PTR_ERR(adc->regs), > + "failed to map registers\n"); > + > + adc->clk = devm_clk_get_enabled(dev, NULL); > + if (IS_ERR(adc->clk)) > + return dev_err_probe(dev, PTR_ERR(adc->clk), > + "failed to get and enable ADC clock\n"); > + > + indio_dev->name = "ma35d1-eadc"; > + indio_dev->modes = INDIO_DIRECT_MODE | INDIO_BUFFER_TRIGGERED; Second part of this is set by the triggered_buffer call later. So don't set it here. > + indio_dev->info = &ma35d1_adc_info; > + > + ret = ma35d1_adc_parse_channels(indio_dev, dev); > + if (ret) > + return ret; > + > + ma35d1_adc_hw_init(adc); > + > + ret = devm_add_action_or_reset(dev, ma35d1_adc_hw_disable, adc); > + if (ret) > + return ret; > + > + ret = ma35d1_adc_setup_trigger(indio_dev, dev); > + if (ret) > + return ret; > + > + irq = platform_get_irq(pdev, 0); > + if (irq < 0) > + return irq; > + > + ret = devm_request_irq(dev, irq, ma35d1_adc_isr, 0, dev_name(dev), > + indio_dev); > + if (ret) > + return dev_err_probe(dev, ret, "failed to request IRQ %d\n", irq); > + > + ret = devm_iio_triggered_buffer_setup(dev, indio_dev, > + iio_pollfunc_store_time, > + ma35d1_adc_trigger_handler, > + &ma35d1_adc_buffer_ops); > + if (ret) > + return dev_err_probe(dev, ret, > + "failed to setup triggered buffer\n"); > + > + platform_set_drvdata(pdev, indio_dev); > + > + ret = devm_iio_device_register(dev, indio_dev); > + if (ret) > + return dev_err_probe(dev, ret, "failed to register IIO device\n"); > + > + return 0; > +} > + > +static int ma35d1_adc_suspend(struct device *dev) > +{ > + struct iio_dev *indio_dev = dev_get_drvdata(dev); > + struct ma35d1_adc *adc = iio_priv(indio_dev); > + > + if (iio_buffer_enabled(indio_dev)) > + return -EBUSY; Failing suspend because a buffer is enabled is unlikely to be popular. Can you not save necessary state and restore so buffered capture continues on resume? > + > + ma35d1_adc_hw_disable(adc); > + clk_disable_unprepare(adc->clk); > + > + return 0; > +} > + > +static int ma35d1_adc_resume(struct device *dev) > +{ > + struct iio_dev *indio_dev = dev_get_drvdata(dev); > + struct ma35d1_adc *adc = iio_priv(indio_dev); > + int ret; > + > + ret = clk_prepare_enable(adc->clk); > + if (ret) > + return ret; > + > + ma35d1_adc_hw_init(adc); > + > + return 0; > +}