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=-0.7 required=3.0 tests=FROM_EXCESS_BASE64, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS, URIBL_BLOCKED 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 9EC12C433FF for ; Mon, 29 Jul 2019 22:04:55 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 6E80E20C01 for ; Mon, 29 Jul 2019 22:04:55 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1729331AbfG2WEy convert rfc822-to-8bit (ORCPT ); Mon, 29 Jul 2019 18:04:54 -0400 Received: from mailoutvs39.siol.net ([185.57.226.230]:34773 "EHLO mail.siol.net" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1728032AbfG2WEy (ORCPT ); Mon, 29 Jul 2019 18:04:54 -0400 Received: from localhost (localhost [127.0.0.1]) by mail.siol.net (Zimbra) with ESMTP id 490F8522CFE; Tue, 30 Jul 2019 00:04:49 +0200 (CEST) X-Virus-Scanned: amavisd-new at psrvmta12.zcs-production.pri Received: from mail.siol.net ([127.0.0.1]) by localhost (psrvmta12.zcs-production.pri [127.0.0.1]) (amavisd-new, port 10032) with ESMTP id WKzvpPpu-wT9; Tue, 30 Jul 2019 00:04:48 +0200 (CEST) Received: from mail.siol.net (localhost [127.0.0.1]) by mail.siol.net (Zimbra) with ESMTPS id 923D1522CD2; Tue, 30 Jul 2019 00:04:48 +0200 (CEST) Received: from jernej-laptop.localnet (cpe-194-152-11-237.cable.triera.net [194.152.11.237]) (Authenticated sender: jernej.skrabec@siol.net) by mail.siol.net (Zimbra) with ESMTPA id DCAB5522CFE; Tue, 30 Jul 2019 00:04:47 +0200 (CEST) From: Jernej =?utf-8?B?xaBrcmFiZWM=?= To: Uwe =?ISO-8859-1?Q?Kleine=2DK=F6nig?= Cc: linux-sunxi@googlegroups.com, Chen-Yu Tsai , Mark Rutland , linux-pwm@vger.kernel.org, devicetree , linux-kernel , Maxime Ripard , Rob Herring , Thierry Reding , kernel@pengutronix.de, linux-arm-kernel Subject: Re: [linux-sunxi] Re: [PATCH 4/6] pwm: sun4i: Add support for H6 PWM Date: Tue, 30 Jul 2019 00:04:47 +0200 Message-ID: <2452836.v7ux4bnEjb@jernej-laptop> In-Reply-To: <20190729185108.tpilwoooxvi2z72e@pengutronix.de> References: <20190726184045.14669-1-jernej.skrabec@siol.net> <173825848.1FZsmuHfpq@jernej-laptop> <20190729185108.tpilwoooxvi2z72e@pengutronix.de> MIME-Version: 1.0 Content-Transfer-Encoding: 8BIT Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Dne ponedeljek, 29. julij 2019 ob 20:51:08 CEST je Uwe Kleine-König napisal(a): > On Mon, Jul 29, 2019 at 08:46:25PM +0200, Jernej Škrabec wrote: > > Dne ponedeljek, 29. julij 2019 ob 20:40:41 CEST je Uwe Kleine-König > > > > napisal(a): > > > On Mon, Jul 29, 2019 at 06:40:15PM +0200, Jernej Škrabec wrote: > > > > Dne ponedeljek, 29. julij 2019 ob 18:24:28 CEST je Uwe Kleine-König > > > > > > > > napisal(a): > > > > > Hello, > > > > > > > > > > On Tue, Jul 30, 2019 at 12:09:40AM +0800, Chen-Yu Tsai wrote: > > > > > > On Tue, Jul 30, 2019 at 12:07 AM Uwe Kleine-König > > > > > > > > > > > > wrote: > > > > > > > On Mon, Jul 29, 2019 at 05:55:52PM +0200, Jernej Škrabec wrote: > > > > > > > > Dne ponedeljek, 29. julij 2019 ob 08:40:30 CEST je Uwe > > > > > > > > Kleine-König > > > > > > > > > > > > > > > > napisal(a): > > > > > > > > > On Fri, Jul 26, 2019 at 08:40:43PM +0200, Jernej Skrabec wrote: > > > > > > > > > > --- a/drivers/pwm/pwm-sun4i.c > > > > > > > > > > +++ b/drivers/pwm/pwm-sun4i.c > > > > > > > > > > @@ -331,6 +331,13 @@ static const struct sun4i_pwm_data > > > > > > > > > > sun4i_pwm_single_bypass = {> > > > > > > > > > > > > > > > > > > > > .npwm = 1, > > > > > > > > > > > > > > > > > > > > }; > > > > > > > > > > > > > > > > > > > > +static const struct sun4i_pwm_data > > > > > > > > > > sun50i_pwm_dual_bypass_clk_rst > > > > > > > > > > = { > > > > > > > > > > + .has_bus_clock = true, > > > > > > > > > > + .has_prescaler_bypass = true, > > > > > > > > > > + .has_reset = true, > > > > > > > > > > + .npwm = 2, > > > > > > > > > > +}; > > > > > > > > > > + > > > > > > > > > > > > > > > > > > > > static const struct of_device_id sun4i_pwm_dt_ids[] = { > > > > > > > > > > > > > > > > > > > > { > > > > > > > > > > > > > > > > > > > > .compatible = "allwinner,sun4i-a10-pwm", > > > > > > > > > > > > > > > > > > > > @@ -347,6 +354,9 @@ static const struct of_device_id > > > > > > > > > > sun4i_pwm_dt_ids[] = > > > > > > > > > > { > > > > > > > > > > > > > > > > > > > > }, { > > > > > > > > > > > > > > > > > > > > .compatible = "allwinner,sun8i-h3-pwm", > > > > > > > > > > .data = &sun4i_pwm_single_bypass, > > > > > > > > > > > > > > > > > > > > + }, { > > > > > > > > > > + .compatible = "allwinner,sun50i-h6-pwm", > > > > > > > > > > + .data = &sun50i_pwm_dual_bypass_clk_rst, > > > > > > > > > > > > > > > > > > If you follow my suggestion for the two previous patches, > > > > > > > > > you > > > > > > > > > can > > > > > > > > > just > > > > > > > > > > > > > > > > > > use: > > > > > > > > > compatible = "allwinner,sun50i-h6-pwm", > > > > > > > > > "allwinner,sun5i-a10s-pwm"; > > > > > > > > > > > > > > > > > > and drop this patch. > > > > > > > > > > > > > > > > Maxime found out that it's not compatible with A10s due to > > > > > > > > difference > > > > > > > > in bypass bit, but yes, I know what you mean. > > > > > > > > > > > > > > > > Since H6 requires reset line and bus clock to be specified, > > > > > > > > it's > > > > > > > > not > > > > > > > > compatible from DT binding side. New yaml based binding must > > > > > > > > somehow > > > > > > > > know that in order to be able to validate DT node, so it needs > > > > > > > > standalone compatible. However, depending on conclusions of > > > > > > > > other > > > > > > > > discussions, this new compatible can be associated with > > > > > > > > already > > > > > > > > available quirks structure or have it's own.> > > > > > > > > > > > > > > > I cannot follow. You should be able to specify in the binding > > > > > > > that > > > > > > > the > > > > > > > reset line and bus clock is optional. Then > > > > > > > allwinner,sun50i-h6-pwm > > > > > > > without a reset line and bus clock also verifies, but this > > > > > > > doesn't > > > > > > > really hurt (and who knows, maybe the next allwinner chip needs > > > > > > > exactly > > > > > > > this). > > > > > > > > > > > > It is not optional. It will not work if either the clocks or reset > > > > > > controls > > > > > > are missing. How would these be optional anyway? Either it's > > > > > > connected > > > > > > and > > > > > > thus required, or it's not and therefore should be omitted from > > > > > > the > > > > > > description. > > > > > > > > > > [Just arguing about the clock here, the argumentation is analogous > > > > > for > > > > > the reset control.] > > > > > > > > > > From the driver's perspective it's optional: There are devices with > > > > > and > > > > > without a bus clock. This doesn't mean that you can just ignore this > > > > > clock if it's specified. It's optional in the sense "If dt doesn't > > > > > specify it, then assume this is a device that doesn't have it and so > > > > > you > > > > > don't need to handle it." but not in the sense "it doesn't matter if > > > > > you handle it or not.". > > > > > > > > > > Other than that I'm on your side. So for example I think it's not > > > > > optimal that gpiod_get_optional returns NULL if GPIOLIB=n or that > > > > > devm_reset_control_get_optional returns NULL if RESET_CONTROLLER=n > > > > > because this hides exactly the kind of problem you point out here. > > > > > > > > I think there's misunderstanding. I only argued that we can't use > > > > > > > > compatible = "allwinner,sun50i-h6-pwm", > > > > > > > > "allwinner,sun5i-a10s-pwm"; > > > > > > > > as you suggested and only > > > > > > > > compatible = "allwinner,sun50i-h6-pwm"; > > > > > > > > will work. Not because of driver itself (it can still use _optional() > > > > variants), but because of DT binding, which should be able to validate > > > > H6 > > > > PWM node - reset and bus clock references are required in this case. > > > > > > I think I understood. In my eyes there is no need to let validation of > > > the DT bindings catch a missing "optional" property that is needed on > > > H6. > > > > > > You have to draw the line somewhere which information the driver has > > > hard-coded and what is only provided by the device tree and just assumed > > > to be correct by the driver. You argue the driver should know that > > > > No, in this thread I argue that DT validation tool, executed by > > > > make ARCH=arm64 dtbs_check > > > > should catch that. This is not a driver, but DT binding described in YAML. > > The argumentation is the same. dtbs_check doesn't notice if the base > address of your "allwinner,sun50i-h6-pwm" device is wrong. So why should > it catch a missing reset controller phandle? Of course checking actual values of node properties doesn't make sense in dtbs_check, otherwise we would have million DT bindings. If you have 10 copies of the same IP core, of course you would use same compatible, but actual register ranges, interrupts, etc. would be different in DT nodes. At this point I would make same argument as were made before, but there is no point going in circles. I'm interested what have DT maintainers to say. Best regards, Jernej