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 2E2B2C433EF for ; Fri, 3 Jun 2022 16:21:37 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id: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=wExREfQEWPM0p3E/f2r64rlSwE2ye67OFhh985lzRes=; b=aASvvz9QYcYtHu v75KcNuP2jtC1K/4GKR3BCj7UivPLhS5AIzyo0W3ZHg86hdMgYLmrSdMJKwZ2UuvQfTtED30ZL4nl i+oULCc3Nyb3zs0yAzWJFvoQnEr2iwlZsBiF+O0ztS7Tju6JL9aaeQAF7jzOflnz1rIjT+y5hSM3b bn4+3RXu8zXu9UsFGPKvpb3L90XQBuwAOyjYaLUYWw2a4bmamR5xmIIhDvkYw9QoC6Qi8rgHZDeff O6nSaj8rqLkK5nxflIYYjSMzD5sXOcEnjjG4EMUbvCADvdd88Le+bMbRawLc/C5navk1pSSnef4C5 QNFUngOM5fU8yaNwj3Nw==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1nxA2F-0089FV-TV; Fri, 03 Jun 2022 16:20:28 +0000 Received: from ams.source.kernel.org ([2604:1380:4601:e00::1]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1nxA2B-0089Eg-2S; Fri, 03 Jun 2022 16:20:25 +0000 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 ams.source.kernel.org (Postfix) with ESMTPS id B5B9EB82371; Fri, 3 Jun 2022 16:20:21 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id E9DE9C385B8; Fri, 3 Jun 2022 16:20:17 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1654273220; bh=Wl6jNcEEZrNKgBUdf6JAvynPXfoygzNFLAPCwn93l58=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=iNVcwujOuPJbVl+BIdBVWyszCXQ7Fb4XQjIcBWE3RG7YCSPVSclztPRGYGcTN8oHV gRchtkFwLJfZD1Rd0M6Yf1O0G2NXYhE4dR1yeVnVxfTx71iEmPcgxdTqbLBpUotvye 8gy6bjTGA8E/aW+unfLyGtFgLkqTIpMlIp6hsAo54pxPT1wtIN+LRv/oLilD15N3Lw rDYPG7DIv6+99RCcgYXmlf0T3Z3tAZi0351Y24fIzwR8hSeZ+60teryt91JLrt2s1+ Ed03pX2CxAfQPEyhEwNuh52MI+ydmRQ+QvJEALE1LZbooPCczEpxvKRUFhhUG1EVKN QVpOtXvMtEF0g== Date: Fri, 3 Jun 2022 17:29:20 +0100 From: Jonathan Cameron To: Andy Shevchenko Cc: linux-iio@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-amlogic@lists.infradead.org, linux-kernel@vger.kernel.org, Lars-Peter Clausen , Neil Armstrong , Kevin Hilman , Jerome Brunet , Martin Blumenstingl Subject: Re: [PATCH v3 1/6] iio: adc: meson_saradc: Don't attach managed resource to IIO device object Message-ID: <20220603172920.3239bbd6@jic23-huawei> In-Reply-To: <20220603172307.5d2f3c52@jic23-huawei> References: <20220603100004.70336-1-andriy.shevchenko@linux.intel.com> <20220603170612.561edfbf@jic23-huawei> <20220603172307.5d2f3c52@jic23-huawei> X-Mailer: Claws Mail 4.1.0 (GTK 3.24.34; x86_64-pc-linux-gnu) MIME-Version: 1.0 X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20220603_092023_459209_AB08AD14 X-CRM114-Status: GOOD ( 40.33 ) 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: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Fri, 3 Jun 2022 17:23:07 +0100 Jonathan Cameron wrote: > On Fri, 3 Jun 2022 17:06:12 +0100 > Jonathan Cameron wrote: > > > On Fri, 3 Jun 2022 12:59:59 +0300 > > Andy Shevchenko wrote: > > > > > It feels wrong and actually inconsistent to attach managed resources > > > to the IIO device object, which is child of the physical device object. > > > The rest of the ->probe() calls do that against physical device. > > > > > > Resolve this by reassigning managed resources to the physical device object. > > > > > > Fixes: 3adbf3427330 ("iio: adc: add a driver for the SAR ADC found in Amlogic Meson SoCs") > > > Suggested-by: Lars-Peter Clausen > > > Signed-off-by: Andy Shevchenko > > Hi Andy, > > > > This has come up a few times in the past (and we elected to not clean it up > > at the time, though it wasn't a decision to never do so!) > > > > It would definitely be wrong if we had another driver binding against > > the resulting created device (funnily enough I reported a bug on a driver > > doing just that earlier this week), but in this case it's harmless because the > > the tear down will occur with a put_device() ultimately calling device_release() > > and devres_release_all() > > > > https://elixir.bootlin.com/linux/latest/source/drivers/base/core.c#L2211 > > > > Has a comment that covers this case (more or less). > > " > > * Some platform devices are driven without driver attached > > * and managed resources may have been acquired. Make sure > > * all resources are released. > > " > > > > Now, I definitely agree with your statement that it's a bit inconsistent to > > do this, just not the fixes tag. > > > > One other suggestion below. Andy, put a cover letter on these larger series - if nothing else it gives somewhere convenient for people to give tags for the whole series, or maintainer to say what they are doing with it. Anyhow, I'm fine with the series, but will leave it on list for a while longer, particularly to get patch 6 some eyes + testing. Currently I plan to drop the fixes tag from this first patch, but I'm prepared to be convinced it's a bug fix rather than a consistency cleanup. Jonathan > > > > > > > --- > > > v3: new fix-patch > > > drivers/iio/adc/meson_saradc.c | 12 +++++------- > > > 1 file changed, 5 insertions(+), 7 deletions(-) > > > > > > diff --git a/drivers/iio/adc/meson_saradc.c b/drivers/iio/adc/meson_saradc.c > > > index 62cc6fb0ef85..4fe6b997cd03 100644 > > > --- a/drivers/iio/adc/meson_saradc.c > > > +++ b/drivers/iio/adc/meson_saradc.c > > > @@ -650,11 +650,11 @@ static int meson_sar_adc_clk_init(struct iio_dev *indio_dev, > > > void __iomem *base) > > > { > > > struct meson_sar_adc_priv *priv = iio_priv(indio_dev); > > > + struct device *dev = indio_dev->dev.parent; > > > > I'd slightly prefer the device was passed in explicitly to this function rather > > than using the parent assignment which feels a little fragile. > > Meh, ignore this. I see from one of the later patches, the driver is already > making the assumption this is set in other calls, so we aren't making anything > worse with this change. > > Jonathan > > > > > > > > struct clk_init_data init; > > > const char *clk_parents[1]; > > > > > > - init.name = devm_kasprintf(&indio_dev->dev, GFP_KERNEL, "%s#adc_div", > > > - dev_name(indio_dev->dev.parent)); > > > + init.name = devm_kasprintf(dev, GFP_KERNEL, "%s#adc_div", dev_name(dev)); > > > if (!init.name) > > > return -ENOMEM; > > > > > > @@ -670,13 +670,11 @@ static int meson_sar_adc_clk_init(struct iio_dev *indio_dev, > > > priv->clk_div.hw.init = &init; > > > priv->clk_div.flags = 0; > > > > > > - priv->adc_div_clk = devm_clk_register(&indio_dev->dev, > > > - &priv->clk_div.hw); > > > + priv->adc_div_clk = devm_clk_register(dev, &priv->clk_div.hw); > > > if (WARN_ON(IS_ERR(priv->adc_div_clk))) > > > return PTR_ERR(priv->adc_div_clk); > > > > > > - init.name = devm_kasprintf(&indio_dev->dev, GFP_KERNEL, "%s#adc_en", > > > - dev_name(indio_dev->dev.parent)); > > > + init.name = devm_kasprintf(dev, GFP_KERNEL, "%s#adc_en", dev_name(dev)); > > > if (!init.name) > > > return -ENOMEM; > > > > > > @@ -690,7 +688,7 @@ static int meson_sar_adc_clk_init(struct iio_dev *indio_dev, > > > priv->clk_gate.bit_idx = __ffs(MESON_SAR_ADC_REG3_CLK_EN); > > > priv->clk_gate.hw.init = &init; > > > > > > - priv->adc_clk = devm_clk_register(&indio_dev->dev, &priv->clk_gate.hw); > > > + priv->adc_clk = devm_clk_register(dev, &priv->clk_gate.hw); > > > if (WARN_ON(IS_ERR(priv->adc_clk))) > > > return PTR_ERR(priv->adc_clk); > > > > > > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel