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 X-Spam-Level: X-Spam-Status: No, score=-2.2 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED,USER_AGENT_SANE_1 autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 4026DC433FF for ; Mon, 29 Jul 2019 18:21:03 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 1C82D20C01 for ; Mon, 29 Jul 2019 18:21:03 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S2387665AbfG2SVB (ORCPT ); Mon, 29 Jul 2019 14:21:01 -0400 Received: from metis.ext.pengutronix.de ([85.220.165.71]:38451 "EHLO metis.ext.pengutronix.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1727136AbfG2SVB (ORCPT ); Mon, 29 Jul 2019 14:21:01 -0400 Received: from pty.hi.pengutronix.de ([2001:67c:670:100:1d::c5]) by metis.ext.pengutronix.de with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1hsAGS-0003Im-Gu; Mon, 29 Jul 2019 20:20:52 +0200 Received: from ukl by pty.hi.pengutronix.de with local (Exim 4.89) (envelope-from ) id 1hsAGM-0001Qa-Ko; Mon, 29 Jul 2019 20:20:46 +0200 Date: Mon, 29 Jul 2019 20:20:46 +0200 From: Uwe =?iso-8859-1?Q?Kleine-K=F6nig?= To: Maxime Ripard Cc: Chen-Yu Tsai , Jernej Skrabec , Thierry Reding , Rob Herring , Mark Rutland , linux-pwm@vger.kernel.org, devicetree , linux-arm-kernel , linux-kernel , linux-sunxi , Philipp Zabel Subject: Re: [PATCH 2/6] pwm: sun4i: Add a quirk for reset line Message-ID: <20190729182046.6bwrterbxoceulrx@pengutronix.de> References: <20190726184045.14669-1-jernej.skrabec@siol.net> <20190726184045.14669-3-jernej.skrabec@siol.net> <20190729063630.rn325whatfnc3m7n@pengutronix.de> <20190729071218.bukw7vxilqy523k3@pengutronix.de> <20190729163715.vtv7lkrywewomxga@flea.home> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20190729163715.vtv7lkrywewomxga@flea.home> User-Agent: NeoMutt/20170113 (1.7.2) X-SA-Exim-Connect-IP: 2001:67c:670:100:1d::c5 X-SA-Exim-Mail-From: ukl@pengutronix.de X-SA-Exim-Scanned: No (on metis.ext.pengutronix.de); SAEximRunCond expanded to false X-PTX-Original-Recipient: linux-kernel@vger.kernel.org Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, Jul 29, 2019 at 06:37:15PM +0200, Maxime Ripard wrote: > On Mon, Jul 29, 2019 at 09:12:18AM +0200, Uwe Kleine-König wrote: > > Hello, > > > > On Mon, Jul 29, 2019 at 02:43:23PM +0800, Chen-Yu Tsai wrote: > > > On Mon, Jul 29, 2019 at 2:36 PM Uwe Kleine-König > > > wrote: > > > > On Fri, Jul 26, 2019 at 08:40:41PM +0200, Jernej Skrabec wrote: > > > > > @@ -371,6 +374,14 @@ static int sun4i_pwm_probe(struct platform_device *pdev) > > > > > if (IS_ERR(pwm->clk)) > > > > > return PTR_ERR(pwm->clk); > > > > > > > > > > + if (pwm->data->has_reset) { > > > > > + pwm->rst = devm_reset_control_get(&pdev->dev, NULL); > > > > > + if (IS_ERR(pwm->rst)) > > > > > + return PTR_ERR(pwm->rst); > > > > > + > > > > > + reset_control_deassert(pwm->rst); > > > > > + } > > > > > + > > > > > > > > I wonder why there is a need to track if a given chip needs a reset > > > > line. I'd just use devm_reset_control_get_optional() and drop the > > > > .has_reset member in struct sun4i_pwm_data. > > > > > > Because it's not optional for this platform, i.e. it won't work if > > > the reset control (or clk, in the next patch) is somehow missing from > > > the device tree. > > > > If the device tree is wrong it is considered ok that the driver doesn't > > behave correctly. So this is not a problem you need (or should) care > > about. > > To some extent that's true, but if we can make the life easier for > everyone by reporting an error and bailing out instead of silently > ignoring that, continuing to probe and just ending up with a > completely broken system for no apparent reason, then why not just do > that? > > I mean, all it takes is three lines of code. Actually it's more because you need to track for each variant if it needs the clock (or reset stuff) or not. It's a weighing between "simpler driver" and "catch unlikely errors". (If the SoC's .dtsi includes the needed stuff, an error here is really unlikely, isn't it?) So to some degree it's subjective which one is better. I tend to prefer "simpler driver". Also when catching this type of error you have to do it right twice (in the .dtsi and the driver) while with my approach there is only a single place that defines what is right, which is a good design according to what I learned during my studies. > It's no different than just calling clk_get, and testing the return > code to see if it's there or not. I wouldn't call that check when you > depend on a clock "validating the DT". It's just making sure that all > the resources needed for you to operate properly are there, which is a > pretty common thing to do. This is different. As a driver author you are allowed (or even supposed) to assume that the device tree (or ACPI or platform data ...) is right and completely defines the stuff for the driven hardware to work correctly. You must not assume that clk_get() succeeds unconditionally. Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | http://www.pengutronix.de/ | From mboxrd@z Thu Jan 1 00:00:00 1970 From: Uwe =?iso-8859-1?Q?Kleine-K=F6nig?= Subject: Re: [PATCH 2/6] pwm: sun4i: Add a quirk for reset line Date: Mon, 29 Jul 2019 20:20:46 +0200 Message-ID: <20190729182046.6bwrterbxoceulrx@pengutronix.de> References: <20190726184045.14669-1-jernej.skrabec@siol.net> <20190726184045.14669-3-jernej.skrabec@siol.net> <20190729063630.rn325whatfnc3m7n@pengutronix.de> <20190729071218.bukw7vxilqy523k3@pengutronix.de> <20190729163715.vtv7lkrywewomxga@flea.home> Reply-To: u.kleine-koenig-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org Mime-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Return-path: Sender: linux-sunxi-/JYPxA39Uh5TLH3MbocFFw@public.gmane.org Content-Disposition: inline In-Reply-To: <20190729163715.vtv7lkrywewomxga-YififvaboMKzQB+pC5nmwQ@public.gmane.org> List-Post: , List-Help: , List-Archive: , List-Unsubscribe: , To: Maxime Ripard Cc: Chen-Yu Tsai , Jernej Skrabec , Thierry Reding , Rob Herring , Mark Rutland , linux-pwm-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, devicetree , linux-arm-kernel , linux-kernel , linux-sunxi , Philipp Zabel List-Id: devicetree@vger.kernel.org On Mon, Jul 29, 2019 at 06:37:15PM +0200, Maxime Ripard wrote: > On Mon, Jul 29, 2019 at 09:12:18AM +0200, Uwe Kleine-K=C3=B6nig wrote: > > Hello, > > > > On Mon, Jul 29, 2019 at 02:43:23PM +0800, Chen-Yu Tsai wrote: > > > On Mon, Jul 29, 2019 at 2:36 PM Uwe Kleine-K=C3=B6nig > > > wrote: > > > > On Fri, Jul 26, 2019 at 08:40:41PM +0200, Jernej Skrabec wrote: > > > > > @@ -371,6 +374,14 @@ static int sun4i_pwm_probe(struct platform_d= evice *pdev) > > > > > if (IS_ERR(pwm->clk)) > > > > > return PTR_ERR(pwm->clk); > > > > > > > > > > + if (pwm->data->has_reset) { > > > > > + pwm->rst =3D devm_reset_control_get(&pdev->dev, NUL= L); > > > > > + if (IS_ERR(pwm->rst)) > > > > > + return PTR_ERR(pwm->rst); > > > > > + > > > > > + reset_control_deassert(pwm->rst); > > > > > + } > > > > > + > > > > > > > > I wonder why there is a need to track if a given chip needs a reset > > > > line. I'd just use devm_reset_control_get_optional() and drop the > > > > .has_reset member in struct sun4i_pwm_data. > > > > > > Because it's not optional for this platform, i.e. it won't work if > > > the reset control (or clk, in the next patch) is somehow missing from > > > the device tree. > > > > If the device tree is wrong it is considered ok that the driver doesn't > > behave correctly. So this is not a problem you need (or should) care > > about. >=20 > To some extent that's true, but if we can make the life easier for > everyone by reporting an error and bailing out instead of silently > ignoring that, continuing to probe and just ending up with a > completely broken system for no apparent reason, then why not just do > that? >=20 > I mean, all it takes is three lines of code. Actually it's more because you need to track for each variant if it needs the clock (or reset stuff) or not. It's a weighing between "simpler driver" and "catch unlikely errors". (If the SoC's .dtsi includes the needed stuff, an error here is really unlikely, isn't it?) So to some degree it's subjective which one is better. I tend to prefer "simpler driver". Also when catching this type of error you have to do it right twice (in the .dtsi and the driver) while with my approach there is only a single place that defines what is right, which is a good design according to what I learned during my studies. > It's no different than just calling clk_get, and testing the return > code to see if it's there or not. I wouldn't call that check when you > depend on a clock "validating the DT". It's just making sure that all > the resources needed for you to operate properly are there, which is a > pretty common thing to do. This is different. As a driver author you are allowed (or even supposed) to assume that the device tree (or ACPI or platform data ...) is right and completely defines the stuff for the driven hardware to work correctly. You must not assume that clk_get() succeeds unconditionally. Best regards Uwe --=20 Pengutronix e.K. | Uwe Kleine-K=C3=B6nig = | Industrial Linux Solutions | http://www.pengutronix.de/ | --=20 You received this message because you are subscribed to the Google Groups "= linux-sunxi" group. To unsubscribe from this group and stop receiving emails from it, send an e= mail to linux-sunxi+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org To view this discussion on the web, visit https://groups.google.com/d/msgid= /linux-sunxi/20190729182046.6bwrterbxoceulrx%40pengutronix.de. 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 X-Spam-Level: X-Spam-Status: No, score=-2.3 required=3.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED, DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_HELO_NONE, SPF_PASS,URIBL_BLOCKED,USER_AGENT_SANE_1 autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 6DAADC433FF for ; Mon, 29 Jul 2019 18:21:35 +0000 (UTC) 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 mail.kernel.org (Postfix) with ESMTPS id 40C21206DD for ; Mon, 29 Jul 2019 18:21:35 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="dTtLPtkX" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 40C21206DD Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=pengutronix.de Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+infradead-linux-arm-kernel=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20170209; h=Sender: Content-Transfer-Encoding:Content-Type:Cc:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References: Message-ID:Subject: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=t44tQGcnE2gG8t3bha9xnDyrl3qsE9Fc45HpR+XfaBk=; b=dTtLPtkX543vtJ gYYFHu7YIQi5Rz1UHOjaZxVbfc49zqerEkytPIduevw699cIdxjxKhzEJoPQYmPeXrG2+monM9bkP mKrl00x3X1hmMsOtt0Mk/8hIm+c23wFj9xuKz1rWyzBkFt4SLtgOjYtL3W5GSmk8Tl3udP8A6/91o 5kNTVF6m8Nvq5OShzzlvTUU1AZQEKJTGWKHGWW3XByE0ifKa2pfZw4wwTgc0WYODdfQ2Hw+PdUlMk HFHi8ewSi2Aez4J2BBlMRAJGFNJg8Qh4eIUNTS9wi5jnJ7/vnESLgxmgCacKTMoBtwGbt8c7grdb6 Zo2z2nSmME9tsGzy2Suw==; Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.92 #3 (Red Hat Linux)) id 1hsAH8-0005cV-JW; Mon, 29 Jul 2019 18:21:34 +0000 Received: from metis.ext.pengutronix.de ([2001:67c:670:201:290:27ff:fe1d:cc33]) by bombadil.infradead.org with esmtps (Exim 4.92 #3 (Red Hat Linux)) id 1hsAGv-0004Nl-HI for linux-arm-kernel@lists.infradead.org; Mon, 29 Jul 2019 18:21:32 +0000 Received: from pty.hi.pengutronix.de ([2001:67c:670:100:1d::c5]) by metis.ext.pengutronix.de with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1hsAGS-0003Im-Gu; Mon, 29 Jul 2019 20:20:52 +0200 Received: from ukl by pty.hi.pengutronix.de with local (Exim 4.89) (envelope-from ) id 1hsAGM-0001Qa-Ko; Mon, 29 Jul 2019 20:20:46 +0200 Date: Mon, 29 Jul 2019 20:20:46 +0200 From: Uwe =?iso-8859-1?Q?Kleine-K=F6nig?= To: Maxime Ripard Subject: Re: [PATCH 2/6] pwm: sun4i: Add a quirk for reset line Message-ID: <20190729182046.6bwrterbxoceulrx@pengutronix.de> References: <20190726184045.14669-1-jernej.skrabec@siol.net> <20190726184045.14669-3-jernej.skrabec@siol.net> <20190729063630.rn325whatfnc3m7n@pengutronix.de> <20190729071218.bukw7vxilqy523k3@pengutronix.de> <20190729163715.vtv7lkrywewomxga@flea.home> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20190729163715.vtv7lkrywewomxga@flea.home> User-Agent: NeoMutt/20170113 (1.7.2) X-SA-Exim-Connect-IP: 2001:67c:670:100:1d::c5 X-SA-Exim-Mail-From: ukl@pengutronix.de X-SA-Exim-Scanned: No (on metis.ext.pengutronix.de); SAEximRunCond expanded to false X-PTX-Original-Recipient: linux-arm-kernel@lists.infradead.org X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20190729_112126_931903_08A40199 X-CRM114-Status: GOOD ( 24.55 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Mark Rutland , linux-pwm@vger.kernel.org, Jernej Skrabec , devicetree , linux-sunxi , linux-kernel , Rob Herring , Chen-Yu Tsai , Thierry Reding , Philipp Zabel , linux-arm-kernel Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+infradead-linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Mon, Jul 29, 2019 at 06:37:15PM +0200, Maxime Ripard wrote: > On Mon, Jul 29, 2019 at 09:12:18AM +0200, Uwe Kleine-K=F6nig wrote: > > Hello, > > > > On Mon, Jul 29, 2019 at 02:43:23PM +0800, Chen-Yu Tsai wrote: > > > On Mon, Jul 29, 2019 at 2:36 PM Uwe Kleine-K=F6nig > > > wrote: > > > > On Fri, Jul 26, 2019 at 08:40:41PM +0200, Jernej Skrabec wrote: > > > > > @@ -371,6 +374,14 @@ static int sun4i_pwm_probe(struct platform_d= evice *pdev) > > > > > if (IS_ERR(pwm->clk)) > > > > > return PTR_ERR(pwm->clk); > > > > > > > > > > + if (pwm->data->has_reset) { > > > > > + pwm->rst =3D devm_reset_control_get(&pdev->dev, NUL= L); > > > > > + if (IS_ERR(pwm->rst)) > > > > > + return PTR_ERR(pwm->rst); > > > > > + > > > > > + reset_control_deassert(pwm->rst); > > > > > + } > > > > > + > > > > > > > > I wonder why there is a need to track if a given chip needs a reset > > > > line. I'd just use devm_reset_control_get_optional() and drop the > > > > .has_reset member in struct sun4i_pwm_data. > > > > > > Because it's not optional for this platform, i.e. it won't work if > > > the reset control (or clk, in the next patch) is somehow missing from > > > the device tree. > > > > If the device tree is wrong it is considered ok that the driver doesn't > > behave correctly. So this is not a problem you need (or should) care > > about. > = > To some extent that's true, but if we can make the life easier for > everyone by reporting an error and bailing out instead of silently > ignoring that, continuing to probe and just ending up with a > completely broken system for no apparent reason, then why not just do > that? > = > I mean, all it takes is three lines of code. Actually it's more because you need to track for each variant if it needs the clock (or reset stuff) or not. It's a weighing between "simpler driver" and "catch unlikely errors". (If the SoC's .dtsi includes the needed stuff, an error here is really unlikely, isn't it?) So to some degree it's subjective which one is better. I tend to prefer "simpler driver". Also when catching this type of error you have to do it right twice (in the .dtsi and the driver) while with my approach there is only a single place that defines what is right, which is a good design according to what I learned during my studies. > It's no different than just calling clk_get, and testing the return > code to see if it's there or not. I wouldn't call that check when you > depend on a clock "validating the DT". It's just making sure that all > the resources needed for you to operate properly are there, which is a > pretty common thing to do. This is different. As a driver author you are allowed (or even supposed) to assume that the device tree (or ACPI or platform data ...) is right and completely defines the stuff for the driven hardware to work correctly. You must not assume that clk_get() succeeds unconditionally. Best regards Uwe -- = Pengutronix e.K. | Uwe Kleine-K=F6nig | Industrial Linux Solutions | http://www.pengutronix.de/ | _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel