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=-14.3 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER, INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=ham 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 6619CC433E0 for ; Fri, 12 Feb 2021 16:43:15 +0000 (UTC) Received: from merlin.infradead.org (merlin.infradead.org [205.233.59.134]) (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 1F48A64E7D for ; Fri, 12 Feb 2021 16:43:15 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 1F48A64E7D 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+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=merlin.20170209; h=Sender: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-Transfer-Encoding:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=Y63aHllgTYR+Mk524hQTg+9RjvL/Ft1EJUuONq/80aw=; b=uuhR1CgbPdTRYSaMeurhBMKM0 3qYojq+WJs9ilq5Cj8u6wO/UOmBxQ6rAGQkax/tf3gYIgQ5c2txTXKUn3CQ0x5OkjEEOr93O64I2z lFQ6ABfCpqbspnyDHcnYF+hd7++2ggkhUzu2XjJHmjn7WWHE+usi/kbeaK1/3P6GTGez5fhKp8gSc XhBkdlFFEndivVyIwIAkRTnNCOA+mMwzWakBGAG7f5iLArJIuT7/qQyInxM18V3767DT8ozK7A5wd KQOLo+ktv4FID6DpXsu54TaAZjmo/tYQpvuixky/If9ET9+sMv7Wx59ztxrqQPiSMBLD3iJmWrixX k9sa2YWow==; Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1lAbWA-00049O-Hg; Fri, 12 Feb 2021 16:42:07 +0000 Received: from metis.ext.pengutronix.de ([2001:67c:670:201:290:27ff:fe1d:cc33]) by merlin.infradead.org with esmtps (Exim 4.92.3 #3 (Red Hat Linux)) id 1lAbW5-00046g-1J for linux-arm-kernel@lists.infradead.org; Fri, 12 Feb 2021 16:42:04 +0000 Received: from ptx.hi.pengutronix.de ([2001:67c:670:100:1d::c0]) by metis.ext.pengutronix.de with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1lAbVt-0003Jl-6x; Fri, 12 Feb 2021 17:41:49 +0100 Received: from ukl by ptx.hi.pengutronix.de with local (Exim 4.92) (envelope-from ) id 1lAbVo-0006yD-Oe; Fri, 12 Feb 2021 17:41:44 +0100 Date: Fri, 12 Feb 2021 17:41:44 +0100 From: Uwe =?utf-8?Q?Kleine-K=C3=B6nig?= To: Nobuhiro Iwamatsu Subject: Re: [PATCH v2 2/2] pwm: visconti: Add Toshiba Visconti SoC PWM support Message-ID: <20210212164144.wcvy7jkxmrysqxux@pengutronix.de> References: <20210212131910.557581-1-nobuhiro1.iwamatsu@toshiba.co.jp> <20210212131910.557581-3-nobuhiro1.iwamatsu@toshiba.co.jp> MIME-Version: 1.0 In-Reply-To: <20210212131910.557581-3-nobuhiro1.iwamatsu@toshiba.co.jp> X-SA-Exim-Connect-IP: 2001:67c:670:100:1d::c0 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-20210212_114201_291702_3E128243 X-CRM114-Status: GOOD ( 35.35 ) 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: linux-pwm@vger.kernel.org, punit1.agrawal@toshiba.co.jp, devicetree@vger.kernel.org, yuji2.ishikawa@toshiba.co.jp, linux-kernel@vger.kernel.org, Rob Herring , Thierry Reding , kernel@pengutronix.de, Lee Jones , linux-arm-kernel@lists.infradead.org Content-Type: multipart/mixed; boundary="===============3568978834812423668==" Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org --===============3568978834812423668== Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="2ssz6scb22myda7o" Content-Disposition: inline --2ssz6scb22myda7o Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable Hello Nobuhiro, On Fri, Feb 12, 2021 at 10:19:10PM +0900, Nobuhiro Iwamatsu wrote: > Add driver for the PWM controller on Toshiba Visconti ARM SoC. >=20 > Signed-off-by: Nobuhiro Iwamatsu > --- > drivers/pwm/Kconfig | 9 ++ > drivers/pwm/Makefile | 1 + > drivers/pwm/pwm-visconti.c | 173 +++++++++++++++++++++++++++++++++++++ > 3 files changed, 183 insertions(+) > create mode 100644 drivers/pwm/pwm-visconti.c >=20 > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig > index 9a4f66ae8070..8ae68d6203fb 100644 > --- a/drivers/pwm/Kconfig > +++ b/drivers/pwm/Kconfig > @@ -601,6 +601,15 @@ config PWM_TWL_LED > To compile this driver as a module, choose M here: the module > will be called pwm-twl-led. > =20 > +config PWM_VISCONTI > + tristate "Toshiba Visconti PWM support" > + depends on ARCH_VISCONTI || COMPILE_TEST > + help > + PWM Subsystem driver support for Toshiba Visconti SoCs. > + > + To compile this driver as a module, choose M here: the module > + will be called pwm-visconti. > + > config PWM_VT8500 > tristate "vt8500 PWM support" > depends on ARCH_VT8500 || COMPILE_TEST > diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile > index 6374d3b1d6f3..d43b1e17e8e1 100644 > --- a/drivers/pwm/Makefile > +++ b/drivers/pwm/Makefile > @@ -56,4 +56,5 @@ obj-$(CONFIG_PWM_TIECAP) +=3D pwm-tiecap.o > obj-$(CONFIG_PWM_TIEHRPWM) +=3D pwm-tiehrpwm.o > obj-$(CONFIG_PWM_TWL) +=3D pwm-twl.o > obj-$(CONFIG_PWM_TWL_LED) +=3D pwm-twl-led.o > +obj-$(CONFIG_PWM_VISCONTI) +=3D pwm-visconti.o > obj-$(CONFIG_PWM_VT8500) +=3D pwm-vt8500.o > diff --git a/drivers/pwm/pwm-visconti.c b/drivers/pwm/pwm-visconti.c > new file mode 100644 > index 000000000000..2aa140f1ec04 > --- /dev/null > +++ b/drivers/pwm/pwm-visconti.c > @@ -0,0 +1,173 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* > + * Toshiba Visconti pulse-width-modulation controller driver > + * > + * Copyright (c) 2020 TOSHIBA CORPORATION > + * Copyright (c) 2020 Toshiba Electronic Devices & Storage Corporation > + * > + * Authors: Nobuhiro Iwamatsu > + * > + */ > + > +#include > +#include > +#include > +#include > +#include > +#include > + > + > +#define PIPGM_PCSR(ch) (0x400 + 4 * (ch)) > +#define PIPGM_PDUT(ch) (0x420 + 4 * (ch)) > +#define PIPGM_PWMC(ch) (0x440 + 4 * (ch)) > + > +#define PIPGM_PWMC_PWMACT BIT(5) > +#define PIPGM_PWMC_CLK_MASK GENMASK(1, 0) > +#define PIPGM_PWMC_POLARITY_MASK GENMASK(5, 5) > +#define PIPGM_PDUT_MAX 0xFFFF > + > +struct visconti_pwm_chip { > + struct pwm_chip chip; > + void __iomem *base; > +}; > + > +#define to_visconti_chip(chip) \ > + container_of(chip, struct visconti_pwm_chip, chip) > + > +static int visconti_pwm_apply(struct pwm_chip *chip, struct pwm_device *= pwm, > + const struct pwm_state *state) Please align the continuation line to the opening parenthesis. > +{ > + struct visconti_pwm_chip *priv =3D to_visconti_chip(chip); > + u32 period, duty, pwmc0; > + > + dev_dbg(chip->dev, "%s: ch =3D %d en =3D %d p =3D 0x%llx d =3D 0x%llx\n= ", __func__, > + pwm->hwpwm, state->enabled, state->period, state->duty_cycle); > + > + /* > + * pwmc is a 2-bit divider for the input clock running at 1 MHz. > + * When the settings of the PWM are modified, the new values are shadow= ed in hardware until > + * the period register (PCSR) is written and the currently running peri= od is completed. This > + * way the hardware switches atomically from the old setting to the new. > + * Also, disabling the hardware completes the currently running period = and keeps the output > + * at low level at all times. Did you just copy my optimal description or is your hardware really that nice? Do you know scripts/checkpatch.pl? I bet it will tell you to limit your lines to approx. 80 chars where sensible. > + */ > + if (!state->enabled) { > + writel(0, priv->base + PIPGM_PCSR(pwm->hwpwm)); > + return 0; > + } > + > + period =3D state->period / NSEC_PER_USEC; This becomes wrong if state->period > 1000 * 0xffffffff because you discard non-zero bits when reducing the size to u32. > + duty =3D state->duty_cycle / NSEC_PER_USEC; > + if (period < 0x10000) > + pwmc0 =3D 0; > + else if (period < 0x20000) > + pwmc0 =3D 1; > + else if (period < 0x40000) > + pwmc0 =3D 2; > + else if (period < 0x80000) > + pwmc0 =3D 3; > + else > + return -EINVAL; This is equivalent to: pwmc0 =3D ilog2(period >> 16); if (pwmc0 > 3) return -EINVAL; > + if (duty > PIPGM_PDUT_MAX) > + return -EINVAL; I would expect that this check should only happen after duty is shifted below?! I think this cannot happen if you rely on the core to only give you states with duty_cycle <=3D period. > + period >>=3D pwmc0; > + duty >>=3D pwmc0; > + > + if (state->polarity =3D=3D PWM_POLARITY_INVERSED) > + pwmc0 |=3D PIPGM_PWMC_PWMACT; > + > + writel(pwmc0, priv->base + PIPGM_PWMC(pwm->hwpwm)); > + writel(duty, priv->base + PIPGM_PDUT(pwm->hwpwm)); > + writel(period, priv->base + PIPGM_PCSR(pwm->hwpwm)); Please implement the following policy: Pick the biggest possible period not bigger than the requested period. With that pick the biggest possible duty cycle not bigger than the requested duty cycle. That means (assuming I understood your hardware correctly): u32 period, duty_cycle; /* * The biggest period the hardware can provide is * (0xffff << 3) * 1000 ns * This value fits easily in an u32, so simplify the maths by * capping the values to 32 bit integers. */ if (state->period > (0xffff << 3) * 1000) period =3D (0xffff << 3) * 1000; else period =3D state->period; if (state->duty_cycle > period) duty_cycle =3D period; else duty_cycle =3D state->duty_cycle; /* * The input clock runs fixed at 1 MHz, so we have only * microsecond resolution and so can divide by * NSEC_PER_SEC / CLKFREQ =3D 1000 without loosing precision. */ period /=3D 1000; duty_cycle /=3D 1000; if (!period) /* period too small */ return -ERANGE; /* * PWMC controls a divider that divides the input clk by a * power of two between 1 and 8. As a smaller divider yields * higher precision, pick the smallest possible one. */ pwmc0 =3D ilog2(period >> 16); BUG_ON(pwmc0 > 3); period >>=3D pwmc0; duty_cycle >>=3D pwmc0; =09 if (state->polarity =3D=3D PWM_POLARITY_INVERSED) pwmc0 |=3D PIPGM_PWMC_PWMACT; =09 writel(pwmc0, priv->base + PIPGM_PWMC(pwm->hwpwm)); writel(duty, priv->base + PIPGM_PDUT(pwm->hwpwm)); writel(period, priv->base + PIPGM_PCSR(pwm->hwpwm)); > + return 0; > +} > + > +static void visconti_pwm_get_state(struct pwm_chip *chip, struct pwm_dev= ice *pwm, > + struct pwm_state *state) > +{ > +[...] > +} Looks good. > [...] >=20 > +static struct platform_driver visconti_pwm_driver =3D { > + .driver =3D { > + .name =3D "pwm-visconti", > + .of_match_table =3D visconti_pwm_of_match, > + }, > + .probe =3D visconti_pwm_probe, > + .remove =3D visconti_pwm_remove, > +}; > +module_platform_driver(visconti_pwm_driver); > + > +MODULE_LICENSE("GPL v2"); > +MODULE_AUTHOR("Nobuhiro Iwamatsu "); > +MODULE_ALIAS("platform:visconti-pwm"); This must match the .name field of the platform driver, so it must be MODULE_ALIAS("platform:pwm-visconti"); Best regards Uwe --=20 Pengutronix e.K. | Uwe Kleine-K=F6nig | Industrial Linux Solutions | https://www.pengutronix.de/ | --2ssz6scb22myda7o Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQEzBAABCgAdFiEEfnIqFpAYrP8+dKQLwfwUeK3K7AkFAmAmr8QACgkQwfwUeK3K 7AkXrQf+OSZRSopiaKm+LYuAkFlcVUCiRoyePIf28ohZt07zPF8u7HCGb/tYHXKl 3pDlplkZoXolQzi3RsTENbH8SMB0ceHzsW2/8G8Dnpz9t7FIaWOLYsjuj3xxHKhs EVW1US87mi/4ZT7CwyMiDFSou2zVCe8hI9VHEWYRb6+jfIIIdbWkpOFlwACGOqpl 3F/ACImmr6uQbzAjWunk1xkV3wB2nv7qPe6KvMxs0AzZ664cGxTG6VaWYCheJAl6 fI/iXHpZQgUgIEQTXOMDaK/xmow/VC6X9EdkHBj1g/ejWMfIX/hrGajxlIVZ9eYj Xmdy3VgdkYYKEDox/WxUyHM6JfUZag== =f243 -----END PGP SIGNATURE----- --2ssz6scb22myda7o-- --===============3568978834812423668== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel --===============3568978834812423668==--