From patchwork Mon Jan 15 08:41:50 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Claudiu Beznea X-Patchwork-Id: 10163283 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id 1F96160325 for ; Mon, 15 Jan 2018 08:42:51 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 0582B21BED for ; Mon, 15 Jan 2018 08:42:51 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id EE156288D8; Mon, 15 Jan 2018 08:42:50 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-4.2 required=2.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,RCVD_IN_DNSWL_MED autolearn=unavailable version=3.3.1 Received: from bombadil.infradead.org (bombadil.infradead.org [65.50.211.133]) (using TLSv1.2 with cipher AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.wl.linuxfoundation.org (Postfix) with ESMTPS id 6C1F021BED for ; Mon, 15 Jan 2018 08:42:50 +0000 (UTC) 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:Date: Message-ID:From:References:To:Subject:Reply-To:Content-ID:Content-Description :Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=Umd3GaSQYU/cJVAuF24MwDD8l1mP7avQ1JY47GUueyo=; b=IPCsd9za0N+8su npA0nJdVOheZx8u8OQompaoq27OGswp6P7GgO3Pe2S9UZkdGjjt+BxoiT5a4txIc9VAzARMABwrvy mF26haXyKrBIaLt+Nb9SYj/8bdsaqVFcXA/jYOrWApClq7rc1s14JFI6Wu/0S5d7DburBzrT2LDcd X3FoYYGsvNfOZWE9FabQK+oE9lr/VJ5OdIH5zUhuYtTe3+D5qYSYusnaTcT8BlvTocrKQaZ9JXZ6y hFXXUXyil2IICQ8lpe00Rc7Jy/y9C0vb0d1DupTfqX2NpN4VDeofBksYE+pjaAZfS/PAH5T000lqL kdBub5QhHAvgXr0Hkn2g==; Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.89 #1 (Red Hat Linux)) id 1eb0Lx-0001cc-65; Mon, 15 Jan 2018 08:42:49 +0000 Received: from esa5.microchip.iphmx.com ([216.71.150.166]) by bombadil.infradead.org with esmtps (Exim 4.89 #1 (Red Hat Linux)) id 1eb0LI-00019Y-Pz; Mon, 15 Jan 2018 08:42:11 +0000 X-IronPort-AV: E=Sophos;i="5.46,362,1511852400"; d="scan'208";a="8262467" Received: from smtpout.microchip.com (HELO email.microchip.com) ([198.175.253.82]) by esa5.microchip.iphmx.com with ESMTP/TLS/DHE-RSA-AES256-SHA; 15 Jan 2018 01:41:56 -0700 Received: from [10.145.6.81] (10.10.76.4) by chn-sv-exch05.mchp-main.com (10.10.76.106) with Microsoft SMTP Server id 14.3.352.0; Mon, 15 Jan 2018 01:41:55 -0700 Subject: Re: [PATCH v2 01/16] drivers: pwm: core: use a single of xlate function To: Brian Norris References: <1515766983-15151-1-git-send-email-claudiu.beznea@microchip.com> <1515766983-15151-2-git-send-email-claudiu.beznea@microchip.com> <20180112183512.GB102880@google.com> From: Claudiu Beznea Message-ID: <6441df76-fb4a-ce00-1019-f7ff9143b75e@microchip.com> Date: Mon, 15 Jan 2018 10:41:50 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.5.0 MIME-Version: 1.0 In-Reply-To: <20180112183512.GB102880@google.com> Content-Language: en-US X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20180115_004208_878993_493B97C9 X-CRM114-Status: GOOD ( 27.03 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: mark.rutland@arm.com, linux-pwm@vger.kernel.org, linux-rpi-kernel@lists.infradead.org, Mike Dunn , Alexander Shiyan , corbet@lwn.net, linux-kernel@vger.kernel.org, linux@armlinux.org.uk, robh+dt@kernel.org, linux-rockchip@lists.infradead.org, devicetree@vger.kernel.org, thierry.reding@gmail.com, alexandre.belloni@free-electrons.com, haojian.zhuang@gmail.com, linux-amlogic@lists.infradead.org, robert.jarzmik@free.fr, daniel@zonque.org, linux-arm-kernel@lists.infradead.org Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+patchwork-linux-arm=patchwork.kernel.org@lists.infradead.org X-Virus-Scanned: ClamAV using ClamSMTP Hi Boris, Thanks for your review. See below my answers. On 12.01.2018 20:35, Brian Norris wrote: > Hi, > > On Fri, Jan 12, 2018 at 04:22:48PM +0200, Claudiu Beznea wrote: >> Remove of_pwm_simple_xlate() and of_pwm_xlate_with_flags() functions >> and add of_pwm_xlate() which is used in all cases no mather if the OF >> bindings are with PWM flags or not. This should not affect the old >> behavior since the xlate will be based on #pwm-cells property of the >> PWM controller. Based on #pwm-cells property the xlate will consider >> the flags or not. This will permit the addition of other inputs to OF >> xlate by just adding proper code at the end of of_pwm_xlate() and a new >> input to enum pwm_args_xlate_options. With this changes there will be >> no need to fill of_xlate and of_pwm_n_cells of struct pwm_chip from >> the drivers probe methods. References in drives to references to of_xlate >> and of_pwm_n_cells were removed. Drivers which used private of_xlate >> functions switched to the generic of_pwm_xlate() function which fits >> for it but with little changes in device trees (these drivers translated >> differently the "pwms" bindings; the "pwms" bindings now are generic to >> all drivers and all drivers should provide them in the format described >> in pwm documentation). >> >> Cc: Thierry Reding >> Cc: Mike Dunn >> Cc: Brian Norris >> Cc: Alexander Shiyan >> Signed-off-by: Claudiu Beznea >> --- >> >> This patch (and the next 7) could be applied independetly by this series, if >> any, but I choosed to have it here since it makes easy the PWM modes parsing. >> If you feel it could be independently of this series I could send a new version. >> >> Also, Thierry, Mike, Brian, Shiyan, please take an extra look over pwm-pxa.c, >> pwm-cros-ec.c and pwm-clps711x.c since these were moved to use the generic >> pwms (minimum 2 pwm-cells). >> >> drivers/pwm/core.c | 56 +++++++++++------------------------------- >> drivers/pwm/pwm-atmel-hlcdc.c | 2 -- >> drivers/pwm/pwm-atmel-tcb.c | 2 -- >> drivers/pwm/pwm-atmel.c | 6 ----- >> drivers/pwm/pwm-bcm-iproc.c | 2 -- >> drivers/pwm/pwm-bcm-kona.c | 2 -- >> drivers/pwm/pwm-bcm2835.c | 2 -- >> drivers/pwm/pwm-berlin.c | 2 -- >> drivers/pwm/pwm-clps711x.c | 11 --------- >> drivers/pwm/pwm-cros-ec.c | 20 --------------- > > For pwm-cros-ec.c: > > Nacked-by: Brian Norris > > This is a fiat change of the documented binding, which breaks the RK3399 > Kevin board. That's not how we do device tree. > > You can extend the binding if you want, so you can represent the period > in the device tree if you'd like (though the value won't mean anything; > it can't be changed by the kernel), but don't break existing device > trees. That wasn't the idea, I wasn't intended to break something. The idea was to have a generic device tree parsing function since all the drivers, except pwm-pxa.c, pwm-cros-ec.c and pwm-clps711x.c, uses the same function to parse DT bindings. And I think, these 3 drivers could use this way of parsing, which is not something new, is what all the current PWM drivers uses (except pwm-pxa.c, pwm-cros-ec.c and pwm-clps711x.c). It is true I have no RK3399 board to run any tests. pwm-cross-ec.c it is true that it's period cannot be changed. It is fixed, as I saw in the driver, at EC_PWM_MAX_DUTY=0xffff. The driver itself won't apply any PWM state if the period is different from EC_PWM_MAX_DUTY. For this driver, the PWM bindings were changed (I did a grep by "google,cros-ec-pwm" and located only: arch/arm64/boot/dts/rockchip/rk3399-gru-kevin.dts arch/arm64/boot/dts/rockchip/rk3399-gru.dtsi files) and changed the bindings in this series, as follows, patch 7 from this series: The code that was removed requests a PWM, the one that was set in the bindings, and then set pwm->args.period: -static struct pwm_device * -cros_ec_pwm_xlate(struct pwm_chip *pc, const struct of_phandle_args *args) -{ - struct pwm_device *pwm; - - if (args->args[0] >= pc->npwm) - return ERR_PTR(-EINVAL); - - pwm = pwm_request_from_chip(pc, args->args[0], NULL); - if (IS_ERR(pwm)) - return pwm; - - /* The EC won't let us change the period */ - pwm->args.period = EC_PWM_MAX_DUTY; - - return pwm; -} The old flow is as follows: of_pwm_get() -> cros_ec_pwm_xlate() { request chip and set constant period } The new flow uses of_pwm_xlate(): of_pwm_get() -> of_pwm_xlate() -> { parse PWM args: channel number, period, flags + request PWM chip + set pwm->args; } This path is only used at DT parsing. In case of PWM channel requested by PWM backlight driver it looks good to me with the changes in rk3399-gru-kevin.dts (please correct me if I'm wrong). Since this driver accepts only EC_PWM_MAX_DUTY period maybe the documentation should be updated regarding this value? Please, let me know what you think! Thanks, Claudiu > >> drivers/pwm/pwm-fsl-ftm.c | 2 -- >> drivers/pwm/pwm-hibvt.c | 2 -- >> drivers/pwm/pwm-imx.c | 8 ------ >> drivers/pwm/pwm-lpc18xx-sct.c | 2 -- >> drivers/pwm/pwm-meson.c | 2 -- >> drivers/pwm/pwm-omap-dmtimer.c | 2 -- >> drivers/pwm/pwm-pxa.c | 19 -------------- >> drivers/pwm/pwm-renesas-tpu.c | 2 -- >> drivers/pwm/pwm-rockchip.c | 5 ---- >> drivers/pwm/pwm-samsung.c | 3 --- >> drivers/pwm/pwm-sun4i.c | 2 -- >> drivers/pwm/pwm-tiecap.c | 2 -- >> drivers/pwm/pwm-tiehrpwm.c | 2 -- >> drivers/pwm/pwm-vt8500.c | 2 -- >> drivers/pwm/pwm-zx.c | 2 -- >> include/linux/pwm.h | 23 ++++++++++------- >> 26 files changed, 29 insertions(+), 156 deletions(-) >> > ... > >> diff --git a/drivers/pwm/pwm-cros-ec.c b/drivers/pwm/pwm-cros-ec.c >> index 9c13694eaa24..692298693768 100644 >> --- a/drivers/pwm/pwm-cros-ec.c >> +++ b/drivers/pwm/pwm-cros-ec.c >> @@ -133,24 +133,6 @@ static void cros_ec_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm, >> state->duty_cycle = ret; >> } >> >> -static struct pwm_device * >> -cros_ec_pwm_xlate(struct pwm_chip *pc, const struct of_phandle_args *args) >> -{ >> - struct pwm_device *pwm; >> - >> - if (args->args[0] >= pc->npwm) >> - return ERR_PTR(-EINVAL); >> - >> - pwm = pwm_request_from_chip(pc, args->args[0], NULL); >> - if (IS_ERR(pwm)) >> - return pwm; >> - >> - /* The EC won't let us change the period */ >> - pwm->args.period = EC_PWM_MAX_DUTY; >> - >> - return pwm; >> -} >> - >> static const struct pwm_ops cros_ec_pwm_ops = { >> .get_state = cros_ec_pwm_get_state, >> .apply = cros_ec_pwm_apply, >> @@ -207,8 +189,6 @@ static int cros_ec_pwm_probe(struct platform_device *pdev) >> /* PWM chip */ >> chip->dev = dev; >> chip->ops = &cros_ec_pwm_ops; >> - chip->of_xlate = cros_ec_pwm_xlate; >> - chip->of_pwm_n_cells = 1; >> chip->base = -1; >> ret = cros_ec_num_pwms(ec); >> if (ret < 0) { > > ... > > Brian > diff --git a/arch/arm64/boot/dts/rockchip/rk3399-gru-kevin.dts b/arch/arm64/boot/dts/rockchip/rk3399-gru-kevin.dts index 0384e3121f18..0c790ec387eb 100644 --- a/arch/arm64/boot/dts/rockchip/rk3399-gru-kevin.dts +++ b/arch/arm64/boot/dts/rockchip/rk3399-gru-kevin.dts @@ -77,7 +77,7 @@ backlight: backlight { compatible = "pwm-backlight"; - pwms = <&cros_ec_pwm 1>; + pwms = <&cros_ec_pwm 1 65535>; brightness-levels = <0 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 diff --git a/arch/arm64/boot/dts/rockchip/rk3399-gru.dtsi b/arch/arm64/boot/dts/rockchip/rk3399-gru.dtsi index 5772c52fbfd3..aa377f9ae6ad 100644 --- a/arch/arm64/boot/dts/rockchip/rk3399-gru.dtsi +++ b/arch/arm64/boot/dts/rockchip/rk3399-gru.dtsi @@ -853,7 +853,7 @@ ap_i2c_audio: &i2c8 { cros_ec_pwm: ec-pwm { compatible = "google,cros-ec-pwm"; - #pwm-cells = <1>; + #pwm-cells = <2>; }; }; };