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=-3.8 required=3.0 tests=BAYES_00, 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 6505CC2B9F4 for ; Mon, 28 Jun 2021 16:24:15 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 46A656144E for ; Mon, 28 Jun 2021 16:24:15 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S233674AbhF1Q0j (ORCPT ); Mon, 28 Jun 2021 12:26:39 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:33586 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S233318AbhF1Q0i (ORCPT ); Mon, 28 Jun 2021 12:26:38 -0400 Received: from metis.ext.pengutronix.de (metis.ext.pengutronix.de [IPv6:2001:67c:670:201:290:27ff:fe1d:cc33]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id BE0D4C061574 for ; Mon, 28 Jun 2021 09:24:12 -0700 (PDT) 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 1lxu3N-0006TZ-5k; Mon, 28 Jun 2021 18:24:09 +0200 Received: from ukl by ptx.hi.pengutronix.de with local (Exim 4.92) (envelope-from ) id 1lxu3L-00088p-V1; Mon, 28 Jun 2021 18:24:07 +0200 Date: Mon, 28 Jun 2021 18:24:07 +0200 From: Uwe =?utf-8?Q?Kleine-K=C3=B6nig?= To: Sean Anderson Cc: linux-pwm@vger.kernel.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, Emil Lenngren , michal.simek@xilinx.com, Alvaro Gamez , Thierry Reding , kernel@pengutronix.de, linux-arm-kernel@lists.infradead.org Subject: Re: [PATCH v4 3/3] pwm: Add support for Xilinx AXI Timer Message-ID: <20210628162407.dxxt6hqfzeokdtxa@pengutronix.de> References: <20210528214522.617435-1-sean.anderson@seco.com> <20210528214522.617435-3-sean.anderson@seco.com> <20210625061958.yeaxjltuq7q2t7i7@pengutronix.de> <20210625165642.5iuorl5guuq5c7gc@pengutronix.de> <20210627181919.iunagls4j67ignhh@pengutronix.de> <59e93f67-0552-04bb-116e-73ddf878761e@seco.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="mxlp6tyzyeaflos4" Content-Disposition: inline In-Reply-To: <59e93f67-0552-04bb-116e-73ddf878761e@seco.com> 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-kernel@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org --mxlp6tyzyeaflos4 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable Hello Sean, On Mon, Jun 28, 2021 at 11:50:33AM -0400, Sean Anderson wrote: > On 6/27/21 2:19 PM, Uwe Kleine-K=F6nig wrote: > > On Fri, Jun 25, 2021 at 01:46:26PM -0400, Sean Anderson wrote: > > > So for the moment, why not give an error? This will be legal code both > > > now and after round_state is implemented. > >=20 > > The problem is where to draw the line. To stay with your example: If a > > request for period =3D 150 ns comes in, and let X be the biggest period= <=3D > > 150 ns that the hardware can configure. For which values of X should an > > error be returned and for which values the setting should be > > implemented. > >=20 > > In my eyes the only sensible thing to implement here is to tell the > > consumer about X and let it decide if it's good enough. If you have a > > better idea let me hear about it. >=20 > Sure. And I think it's ok to tell the consumer that X is the best we can > do. But if they go along and request an unconfigurable state anyway, we > should tell them as much. I have the impression you didn't understand where I see the problem. If you request 150 ns and the controller can only do 149 ns (or 149.6667 ns) should we refuse? If yes: This is very unusable, e.g. the led-pwm driver expects that it can configure the duty_cycle in 1/256 steps of the period, and then maybe only steps 27 and 213 of the 256 possible steps work. (This example doesn't really match because the led-pwm driver varies duty_cycle and not period, but the principle becomes clear I assume.) If no: Should we accept 151 ns? Isn't that ridiculous? > IMO, this is the best way to prevent surprising results in the API. I think it's not possible in practise to refuse "near" misses and every definition of "near" is in some case ridiculous. Also if you consider the pwm_round_state() case you don't want to refuse any request to tell as much as possible about your controller's capabilities. And then it's straight forward to let apply behave in the same way to keep complexity low. > The real issue here is that it is impossible to determine the correct > way to round the PWM a priori, and in particular, without considering > both duty_cycle and period. If a consumer requests very small > period/duty cycle which we cannot produce, how should it be rounded? Yeah, because there is no obviously right one, I picked one that is as wrong as the other possibilities but is easy to work with. > Should we just set TLR0=3D1 and TLR1=3D0 to give them 66% duty cycle with > the least period? Or should we try and increase the period to better > approximate the % duty cycle? And both of these decisions must be made > knowing both parameters. We cannot (for example) just always round up, > since we may produce a configuration with TLR0 =3D=3D TLR1, which would > produce 0% duty cycle instead of whatever was requested. Rounding rate > will introduce significant complexity into the driver. Most of the time > if a consumer requests an invalid rate, it is due to misconfiguration > which is best solved by fixing the configuration. In the first step pick the biggest period not bigger than the requested and then pick the biggest duty cycle that is not bigger than the requested and that can be set with the just picked period. That is the behaviour that all new drivers should do. This is somewhat arbitrary but after quite some thought the most sensible in my eyes. > > > Perhaps I should add > > >=20 > > > if (tlr0 <=3D tlr1) > > > return -EINVAL; > > >=20 > > > here to prevent accidentally getting 0% duty cycle. > >=20 > > You can assume that duty_cycle <=3D period when .apply is called. >=20 > Ok, I will only check for =3D=3D then. You just have to pay attention to the case that you had to decrement =2Eperiod to the next possible value. Then .duty_cycle might be bigger than the corrected period. Best regards Uwe --=20 Pengutronix e.K. | Uwe Kleine-K=F6nig | Industrial Linux Solutions | https://www.pengutronix.de/ | --mxlp6tyzyeaflos4 Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQEzBAABCgAdFiEEfnIqFpAYrP8+dKQLwfwUeK3K7AkFAmDZ96QACgkQwfwUeK3K 7AlJXgf7BKBFOIdY64LBcokhYBcDKmKMqIXrY1FhK8oF+FxlLdyQ89SrYSXpI29t DagR8JwKJZn6eigEOVD8b1q+0oB4Rrou9g6Q3CkfF5OMLBcxXCXqne1nAOY1lpD2 AXzJjkfCHPNIcH6Cr3ESFhaGGdDzpb4KyxmYOjbrdxlbxlAr8HjgK8e2hbjAW31V m6jzcwWcE/Rru6TklnULd0pkGt4+hEbFTKuwsxoUVMQr10dT7ZN+5zSD9G4nOejc H6Zw8jdqSLsQ+DdI05+iADMUd7oVUnfYkMkCnJq8l7EYtrU35p+HDJNvJ0Djy19/ JYMrzGjxkiegkRdQubUWtuMHlDQwOQ== =srzj -----END PGP SIGNATURE----- --mxlp6tyzyeaflos4-- 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=-4.1 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,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 E8920C2B9F4 for ; Mon, 28 Jun 2021 16:26:34 +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 AD5796144E for ; Mon, 28 Jun 2021 16:26:34 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org AD5796144E 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=bombadil.20210309; h=Sender:Content-Type: List-Subscribe:List-Help:List-Post:List-Archive:List-Unsubscribe:List-Id: In-Reply-To:MIME-Version:References:Message-ID:Subject:Cc: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=4dhbltp43TPsk4h/NnsmzgCAcpu32idGKoe7wVbH0UI=; b=Ycf6T3My0wXEb+BvUaSdHq+hx3 DWhP4PjrRR2FhYl3798SN9fq3UDm7TplaNW5yFwoogOyOG/kCWxW6zpXX61hCaNsFPRjT4BXmJvm9 w9KhvHUaDDwPhWoCM5Zy0JqWyN4Blubdh5qXWXifGQI/GJAM9I5MiD2DPV0whNwXWOqfD0SZl+gRK 6VrUc69eHMf8rfhPQI/LORawvxrASg6q2jz4V0t+tJ2rrQIZoqcgpeN8RlrHt5W15kVjOajjLtD2K A0ygkRGkQTfw7n2Jz/j5gj6hlyALJGKjh3I2SVzWSy81f/8tHD9AQma9rfLhT22t74eGAO7ZEylwJ M0deuWhw==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1lxu3Y-008aIU-6s; Mon, 28 Jun 2021 16:24:20 +0000 Received: from metis.ext.pengutronix.de ([2001:67c:670:201:290:27ff:fe1d:cc33]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1lxu3T-008aH3-Qf for linux-arm-kernel@lists.infradead.org; Mon, 28 Jun 2021 16:24:17 +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 1lxu3N-0006TZ-5k; Mon, 28 Jun 2021 18:24:09 +0200 Received: from ukl by ptx.hi.pengutronix.de with local (Exim 4.92) (envelope-from ) id 1lxu3L-00088p-V1; Mon, 28 Jun 2021 18:24:07 +0200 Date: Mon, 28 Jun 2021 18:24:07 +0200 From: Uwe =?utf-8?Q?Kleine-K=C3=B6nig?= To: Sean Anderson Cc: linux-pwm@vger.kernel.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, Emil Lenngren , michal.simek@xilinx.com, Alvaro Gamez , Thierry Reding , kernel@pengutronix.de, linux-arm-kernel@lists.infradead.org Subject: Re: [PATCH v4 3/3] pwm: Add support for Xilinx AXI Timer Message-ID: <20210628162407.dxxt6hqfzeokdtxa@pengutronix.de> References: <20210528214522.617435-1-sean.anderson@seco.com> <20210528214522.617435-3-sean.anderson@seco.com> <20210625061958.yeaxjltuq7q2t7i7@pengutronix.de> <20210625165642.5iuorl5guuq5c7gc@pengutronix.de> <20210627181919.iunagls4j67ignhh@pengutronix.de> <59e93f67-0552-04bb-116e-73ddf878761e@seco.com> MIME-Version: 1.0 In-Reply-To: <59e93f67-0552-04bb-116e-73ddf878761e@seco.com> 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-20210628_092415_896412_6E78A5A9 X-CRM114-Status: GOOD ( 35.74 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: multipart/mixed; boundary="===============4375813988387476662==" Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org --===============4375813988387476662== Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="mxlp6tyzyeaflos4" Content-Disposition: inline --mxlp6tyzyeaflos4 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable Hello Sean, On Mon, Jun 28, 2021 at 11:50:33AM -0400, Sean Anderson wrote: > On 6/27/21 2:19 PM, Uwe Kleine-K=F6nig wrote: > > On Fri, Jun 25, 2021 at 01:46:26PM -0400, Sean Anderson wrote: > > > So for the moment, why not give an error? This will be legal code both > > > now and after round_state is implemented. > >=20 > > The problem is where to draw the line. To stay with your example: If a > > request for period =3D 150 ns comes in, and let X be the biggest period= <=3D > > 150 ns that the hardware can configure. For which values of X should an > > error be returned and for which values the setting should be > > implemented. > >=20 > > In my eyes the only sensible thing to implement here is to tell the > > consumer about X and let it decide if it's good enough. If you have a > > better idea let me hear about it. >=20 > Sure. And I think it's ok to tell the consumer that X is the best we can > do. But if they go along and request an unconfigurable state anyway, we > should tell them as much. I have the impression you didn't understand where I see the problem. If you request 150 ns and the controller can only do 149 ns (or 149.6667 ns) should we refuse? If yes: This is very unusable, e.g. the led-pwm driver expects that it can configure the duty_cycle in 1/256 steps of the period, and then maybe only steps 27 and 213 of the 256 possible steps work. (This example doesn't really match because the led-pwm driver varies duty_cycle and not period, but the principle becomes clear I assume.) If no: Should we accept 151 ns? Isn't that ridiculous? > IMO, this is the best way to prevent surprising results in the API. I think it's not possible in practise to refuse "near" misses and every definition of "near" is in some case ridiculous. Also if you consider the pwm_round_state() case you don't want to refuse any request to tell as much as possible about your controller's capabilities. And then it's straight forward to let apply behave in the same way to keep complexity low. > The real issue here is that it is impossible to determine the correct > way to round the PWM a priori, and in particular, without considering > both duty_cycle and period. If a consumer requests very small > period/duty cycle which we cannot produce, how should it be rounded? Yeah, because there is no obviously right one, I picked one that is as wrong as the other possibilities but is easy to work with. > Should we just set TLR0=3D1 and TLR1=3D0 to give them 66% duty cycle with > the least period? Or should we try and increase the period to better > approximate the % duty cycle? And both of these decisions must be made > knowing both parameters. We cannot (for example) just always round up, > since we may produce a configuration with TLR0 =3D=3D TLR1, which would > produce 0% duty cycle instead of whatever was requested. Rounding rate > will introduce significant complexity into the driver. Most of the time > if a consumer requests an invalid rate, it is due to misconfiguration > which is best solved by fixing the configuration. In the first step pick the biggest period not bigger than the requested and then pick the biggest duty cycle that is not bigger than the requested and that can be set with the just picked period. That is the behaviour that all new drivers should do. This is somewhat arbitrary but after quite some thought the most sensible in my eyes. > > > Perhaps I should add > > >=20 > > > if (tlr0 <=3D tlr1) > > > return -EINVAL; > > >=20 > > > here to prevent accidentally getting 0% duty cycle. > >=20 > > You can assume that duty_cycle <=3D period when .apply is called. >=20 > Ok, I will only check for =3D=3D then. You just have to pay attention to the case that you had to decrement =2Eperiod to the next possible value. Then .duty_cycle might be bigger than the corrected period. Best regards Uwe --=20 Pengutronix e.K. | Uwe Kleine-K=F6nig | Industrial Linux Solutions | https://www.pengutronix.de/ | --mxlp6tyzyeaflos4 Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQEzBAABCgAdFiEEfnIqFpAYrP8+dKQLwfwUeK3K7AkFAmDZ96QACgkQwfwUeK3K 7AlJXgf7BKBFOIdY64LBcokhYBcDKmKMqIXrY1FhK8oF+FxlLdyQ89SrYSXpI29t DagR8JwKJZn6eigEOVD8b1q+0oB4Rrou9g6Q3CkfF5OMLBcxXCXqne1nAOY1lpD2 AXzJjkfCHPNIcH6Cr3ESFhaGGdDzpb4KyxmYOjbrdxlbxlAr8HjgK8e2hbjAW31V m6jzcwWcE/Rru6TklnULd0pkGt4+hEbFTKuwsxoUVMQr10dT7ZN+5zSD9G4nOejc H6Zw8jdqSLsQ+DdI05+iADMUd7oVUnfYkMkCnJq8l7EYtrU35p+HDJNvJ0Djy19/ JYMrzGjxkiegkRdQubUWtuMHlDQwOQ== =srzj -----END PGP SIGNATURE----- --mxlp6tyzyeaflos4-- --===============4375813988387476662== 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 --===============4375813988387476662==--