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=-1.0 required=3.0 tests=DKIM_SIGNED,DKIM_VALID, FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM,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 108BFC433E0 for ; Fri, 3 Jul 2020 16:24:08 +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 CEC0D20870 for ; Fri, 3 Jul 2020 16:24:07 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="MVqaVRhh"; dkim=fail reason="signature verification failed" (1024-bit key) header.d=gmx.net header.i=@gmx.net header.b="Nyzx6n9U" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org CEC0D20870 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=gmx.net 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=Wj5oXfB05pfZ0wUACojn+EvyVLC9w3ZYEs41wKELqBA=; b=MVqaVRhhvrRkURRyaL6fL5AQD s+ZetOcelX+rxISQmY5HACJmrT6cHFphyWB9fyjsUHxm5rfFKLLV5hg92C7tcBa069Fofe6xMap1m jm1KyO1OZgbEvRZiNS6mKM8BD/TnZ0uHmEI3rAcSIS0Gt5NekYbUgewlVv9WcktFTyjwyFvR7bR7W YgMl/tsxsoGDqdjs9NxSknRRgC8TFACBQgJB6M4VElE/3hV1zoYTmX4pOTX/teBcUBM4LFfzG6RN8 xu/e5KRVFpdFp4obP6FgD94a6+XVTuNJVAyOz3KNVisOCgQQwIWl1+q9LHjFCNShTS75IlwUlUuu8 OJDLdrtIA==; Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1jrOSl-0005yy-Bn; Fri, 03 Jul 2020 16:22:55 +0000 Received: from mout.gmx.net ([212.227.15.18]) by merlin.infradead.org with esmtps (Exim 4.92.3 #3 (Red Hat Linux)) id 1jrOSi-0005xa-Io for linux-arm-kernel@lists.infradead.org; Fri, 03 Jul 2020 16:22:53 +0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=gmx.net; s=badeba3b8450; t=1593793367; bh=eqadiF6CtQU+rbRtG0zl0hM0GNO8oDiiIhsQ1OBoKpQ=; h=X-UI-Sender-Class:Date:From:To:Cc:Subject:References:In-Reply-To; b=Nyzx6n9UjZ08bdLAhU/aAnT9LXfs2MjRuMNiyCcF42kDpBE/KIGPvqnpTmeCfN3mM rU4zxHZ+hU3fhrNHHVP/VboHfSxqSl7wkKTji2sOuXG+WRYQ+BX2JI9E8U/cPhcCJA W8VyaoAVnxnhgJbFQOsg6cA0HI6d7mwyrwhpW/+Y= X-UI-Sender-Class: 01bb95c1-4bf8-414a-932a-4f6e2808ef9c Received: from longitude ([5.146.195.26]) by mail.gmx.com (mrgmx004 [212.227.17.190]) with ESMTPSA (Nemesis) id 1MxDkw-1iu2PZ0Vku-00xZbm; Fri, 03 Jul 2020 18:16:01 +0200 Date: Fri, 3 Jul 2020 18:15:56 +0200 From: Jonathan =?utf-8?Q?Neusch=C3=A4fer?= To: Uwe =?utf-8?Q?Kleine-K=C3=B6nig?= Subject: Re: [RFC PATCH 06/10] pwm: ntxec: Add driver for PWM function in Netronix EC Message-ID: <20200703161556.GA2578@latitude> References: <20200620224222.1312520-1-j.neuschaefer@gmx.net> <20200620224222.1312520-5-j.neuschaefer@gmx.net> <20200622081802.pv4xmb7vn4te5r5t@taurus.defre.kleine-koenig.org> MIME-Version: 1.0 In-Reply-To: <20200622081802.pv4xmb7vn4te5r5t@taurus.defre.kleine-koenig.org> X-Provags-ID: V03:K1:gwpnTl/wbXQ6CtVRQZ2/poQHgxJmcBWGUDwOXRaaOlwSttTLMCE Hm977PBvuIYzYaHdYwKjEy0d7C1/fYmpcMzoRMhnZzzpq3ozFsXjcq/lkKFNIWGX1VLRWNU MQH0fIrjpIVOw5zDbqcABp5xNW0umxvHoyLJPDYo4sNEc01PMbQLar53iVHLRVWOTi2a9iI SP/Nb+2yWtTsb7zorvDbQ== X-UI-Out-Filterresults: notjunk:1;V03:K0:JsI8eDfYsFI=:dKcjg6/IaxJ5kEt3DWRbN9 rCN24m2V5BXhnMin89JlnN12qmjgzB+jx5OtZWWQ2XfbzrOkJry84GBmR0YTkAFI9mNNuNKG0 cyZYic6eep8ojdJondPHxNTto2WiBHbX+PF7Q33IX5yLmP2GH4V6a0O5hKc8TLFwC2wKYi4Xd H8ubw0sRX+qY7Cvm17/wcgSz0xG9tBjzsVVbNeq6KM0YbNzmeWx03Ow6EtMfOT6cQrhOrltNp ilKlHnbLAik9gg3cLv03vl6dEC21LSJsNaFuSZLrTze73Z6LR/oytRKUKqw+vHBG+kmxYvYb1 YSzz44w2WS50EBGWz5ddA9ZTl0pqRIoSfiObGaWUVkhXaQp7Yiw+5HrslGQetrf6vz5LfX57L UJbElq6DYdQK+tLuQJu2Vk8jHrxE+lI63GSB9hKKD4NrE8sPaygbd3GCpmGKO4iIXiwkXI/bM 5u746STgXF56EsBM3ZFJLz4KAyI6/LN3hvph5G9zmTy9cmr/ms0pf9RL+Xj6yt+WR7PAEo4V2 Sieo7d1zUkeZ4CknIdPuwZJ0Ws2drSWPWjDjEVFZlyOdjiy/d7xdaZ1oysjgxrTAbcGXC2SeE 6QVL6uB/nC3pRwIZ9GLdaOrd7CuVOz3Xitp8QztTg3P4wm49rf7qad/WpN9wlPLSk4ZJ8QfI8 JPFmVF22kZ2M70BrK0pkSsgcIpm8V8BNRXMbubeIX5Cy7H9j0r0zvQLKyGqwpvDxxLdqSvX9D BAVkuJVWYtXbXmnC8tM2TDH5KOqppz4rPaOHsplOOp+Q2IC/3C2DQVTerQJT6ihjQA0fldX8f 7HnlIrgcgN7yIhjwVV4HwkJyIKJ84hoXFGr4RgDf+/paMX81GtVVCxpkbPcwUfW0z2aT54SbB Jf5t5PKUl2zcTSVGKTboHeTqQE1Z7YwyXRNrv33cN/AigWkGal6uajn7FQiTLgiPPjjDKQcR5 CuOTSinDPSJNZbIc4BNeCm1cGxIVHWW10Ss5W5A0SrvkkoEbgkoHZbNNJLqYIZSww7odE9a0u 7Rh/1ZvIbz73WadpuZl5An5ItyDRETmWQBa8V3tUnCOlra+2lBU3LxwTiSABpYAS0GW5nlxzB zNwZvo4bPRdgL499sdrRFGNRn6x5iwOMCFHQKV2OF3kYLhmHFdWL8OZe4uZte8pP6GgHJ3+Mo 6aDC2R7trxIw3ILrTeV0wqVob3ZfHw9STLEtNhPlSJ2vGtiMfp0MqOQit+m4nSQ/RKA2S0R2V ajdjuSF9alEw8GqCqmf0Av1WqWjC/Z2kLn0X+7w== X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20200703_122252_821160_9FE6349E X-CRM114-Status: GOOD ( 30.32 ) 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: Alexandre Belloni , Heiko Stuebner , devicetree@vger.kernel.org, Linus Walleij , Thierry Reding , Sam Ravnborg , linux-rtc@vger.kernel.org, Mauro Carvalho Chehab , Lee Jones , Andreas Kemnade , NXP Linux Team , linux-pwm@vger.kernel.org, Stephan Gerhold , allen , Sascha Hauer , Jonathan =?utf-8?Q?Neusch=C3=A4fer?= , Lubomir Rintel , Rob Herring , Fabio Estevam , linux-arm-kernel@lists.infradead.org, Alessandro Zummo , linux-kernel@vger.kernel.org, Mark Brown , Pengutronix Kernel Team , Heiko Stuebner , Josua Mayer , Shawn Guo , "David S. Miller" Content-Type: multipart/mixed; boundary="===============4263553803943667544==" Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org --===============4263553803943667544== Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="Qxx1br4bt0+wmkIi" Content-Disposition: inline --Qxx1br4bt0+wmkIi Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Mon, Jun 22, 2020 at 10:18:02AM +0200, Uwe Kleine-K=C3=B6nig wrote: > Hello, >=20 > On Sun, Jun 21, 2020 at 12:42:17AM +0200, Jonathan Neusch=C3=A4fer wrote: > > The Netronix EC provides a PWM output, which is used for the backlight >=20 > s/,// >=20 > > on ebook readers. This patches adds a driver for the PWM output. >=20 > on *some* ebook readers Ok, I'll fix these. >=20 > > +#define NTXEC_UNK_A 0xa1 > > +#define NTXEC_UNK_B 0xa2 > > +#define NTXEC_ENABLE 0xa3 > > +#define NTXEC_PERIOD_LOW 0xa4 > > +#define NTXEC_PERIOD_HIGH 0xa5 > > +#define NTXEC_DUTY_LOW 0xa6 > > +#define NTXEC_DUTY_HIGH 0xa7 > > + > > +/* > > + * The time base used in the EC is 8MHz, or 125ns. Period and duty cyc= le are > > + * measured in this unit. > > + */ > > +static int ntxec_pwm_config(struct pwm_chip *chip, struct pwm_device *= pwm_dev, > > + int duty_ns, int period_ns) > > +{ > > + struct ntxec_pwm *pwm =3D pwmchip_to_pwm(chip); > > + uint64_t duty =3D duty_ns; > > + uint64_t period =3D period_ns; >=20 > As you cannot use values bigger than 8191999 anyhow, I wonder why you > use a 64 bit type here. No particular reason; I possibly got confused by the division API. I'll use uint32_t instead. > > + int res =3D 0; > > + > > + do_div(period, 125); >=20 > Please use a define instead of plain 125. Will do. > > + if (period > 0xffff) { > > + dev_warn(pwm->dev, > > + "Period is not representable in 16 bits: %llu\n", period); > > + return -ERANGE; > > + } > > + > > + do_div(duty, 125); > > + if (duty > 0xffff) { > > + dev_warn(pwm->dev, "Duty cycle is not representable in 16 bits: %llu= \n", > > + duty); > > + return -ERANGE; > > + } >=20 > This check isn't necessary as the pwm core ensures that duty <=3D period. Ok, I'll remove it. >=20 > > + res |=3D ntxec_write8(pwm->ec, NTXEC_PERIOD_HIGH, period >> 8); > > + res |=3D ntxec_write8(pwm->ec, NTXEC_PERIOD_LOW, period); > > + res |=3D ntxec_write8(pwm->ec, NTXEC_DUTY_HIGH, duty >> 8); > > + res |=3D ntxec_write8(pwm->ec, NTXEC_DUTY_LOW, duty); >=20 > Does this complete the currently running period? Can it happen that a > new period starts between the first and the last write and so a mixed > period can be seen at the output? Good question. I haven't measured it, and also don't have the code running on the EC. >=20 > > + > > + return (res < 0) ? -EIO : 0; > > +} > > + > > +static int ntxec_pwm_enable(struct pwm_chip *chip, > > + struct pwm_device *pwm_dev) > > +{ > > + struct ntxec_pwm *pwm =3D pwmchip_to_pwm(chip); > > + > > + return ntxec_write8(pwm->ec, NTXEC_ENABLE, 1); > > +} > > + > > +static void ntxec_pwm_disable(struct pwm_chip *chip, > > + struct pwm_device *pwm_dev) > > +{ > > + struct ntxec_pwm *pwm =3D pwmchip_to_pwm(chip); > > + > > + ntxec_write8(pwm->ec, NTXEC_ENABLE, 0); > > +} > > + > > +static struct pwm_ops ntxec_pwm_ops =3D { > > + .config =3D ntxec_pwm_config, > > + .enable =3D ntxec_pwm_enable, > > + .disable =3D ntxec_pwm_disable, > > + .owner =3D THIS_MODULE, >=20 > Please don't align the =3D, just a single space before them is fine. Ok > More important: Please implement .apply() (and .get_state()) instead of > the old API. Also please enable PWM_DEBUG which might save us a review > iteration. Will do! >=20 > > +}; > > + > > +static int ntxec_pwm_probe(struct platform_device *pdev) > > +{ > > + struct ntxec *ec =3D dev_get_drvdata(pdev->dev.parent); > > + struct ntxec_pwm *pwm; > > + struct pwm_chip *chip; > > + int res; > > + > > + pwm =3D devm_kzalloc(&pdev->dev, sizeof(*pwm), GFP_KERNEL); > > + if (!pwm) > > + return -ENOMEM; > > + > > + pwm->ec =3D ec; > > + pwm->dev =3D &pdev->dev; > > + > > + chip =3D &pwm->chip; > > + chip->dev =3D &pdev->dev; > > + chip->ops =3D &ntxec_pwm_ops; > > + chip->base =3D -1; > > + chip->npwm =3D 1; > > + > > + res =3D pwmchip_add(chip); > > + if (res < 0) > > + return res; > > + > > + platform_set_drvdata(pdev, pwm); > > + > > + res |=3D ntxec_write8(pwm->ec, NTXEC_ENABLE, 0); > > + res |=3D ntxec_write8(pwm->ec, NTXEC_UNK_A, 0xff); > > + res |=3D ntxec_write8(pwm->ec, NTXEC_UNK_B, 0xff); > > + > > + return (res < 0) ? -EIO : 0; >=20 > This is broken for several reasons: >=20 > - You're not supposed to modify the output in .probe > - if ntxec_write8 results in an error you keep the pwm registered. > - From the moment on pwmchip_add returns the callbacks can be called. > The calls to ntxec_write8 probably interfere here. Ok, I'll rework the probe function to avoid these issues. Thanks for the review, Jonathan Neusch=C3=A4fer --Qxx1br4bt0+wmkIi Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAABCgAdFiEEvHAHGBBjQPVy+qvDCDBEmo7zX9sFAl7/WbUACgkQCDBEmo7z X9sA1BAAoaUXyqhLeTCR3RU7shyvr9x8b/kQ4QQahjrf7CK5jyE4aDDebspprYnq 13AftAJFOoVGhnFOqT8H8jhNO9qCRZ7udKCPABMdc4kzs5NMWQ2zK+s9890zcIZC 6UClwhRCkG+/wEY55LEW0lVCkQJtrkIXQ8OHCc9Cgb83hTQXULu/7Lxq/kOW5rkH 7mffJP46Ul5mtNYdd/EfKxHzxViPHswPKNEkQ9tI26Lc4IcQ3tg4amS480uez8Mi EJZQaQAPYUPmxF5msepumrB5lAm9UL+Hn1EFnHhFdoWv+9TuaHTOwJM6uksOGPjr RBcup36uhof9G80yNlFm59IgR52KWeAMWNBixBYHlkT6uObYMsY4RZOVb61lgf+v WhVi8EJS7xIGTrT6DyYZRqhaN4SaPdQ5YOMsr+W0QqbeGv9W2Z6MtY3cZxKnyREI EcrZ/qlbdcwhwpwknW/TyPA5qXO5M9WLnaJ4AR8bwDpX04CmtwDx0ej088ID2mwC tWoy/bz0Ubsip1h8F5ClTGK8C/vg1PVD6qKTai1iDhVmucwL6KrU2lnvEmzY/RJX 6HYkfboXXEGAiK3TBw5rJ/x5MvhKPBUrlaI9Zfk/A/m27FlgUutWbCVcWYiOdeKo K/nBRBQW9L8ArmDjUOpv2jXTs6wa+eMX7sxaO7uhyxZu5amuemA= =1TeA -----END PGP SIGNATURE----- --Qxx1br4bt0+wmkIi-- --===============4263553803943667544== 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 --===============4263553803943667544==--