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=-2.2 required=3.0 tests=BAYES_00,DKIM_ADSP_CUSTOM_MED, DKIM_SIGNED,DKIM_VALID,FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS, URIBL_BLOCKED,USER_AGENT_SANE_1 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 87503C433FE for ; Mon, 7 Dec 2020 15:31:10 +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 303D42371F for ; Mon, 7 Dec 2020 15:31:10 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 303D42371F Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=gmail.com 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=EN5LfFTiV5v8tC4H2AJ7FDRltr38G9EXUuJmgqALnJA=; b=KPwKjL2EdOEpLfR4pTPey8wh2 0sqsaSjS6sSq+1VeolgKCG0WZbVMFcWOp9qBxUTe0KEB9gQAJRWUhh0ZkOET1oy7XiM9DDZY0LXDc C84AwXwF4xYjiW1jRJJpMkVX3QUEeYKmTBoDlZwuadWzQx9hdXVhIHXIutSHYZRzLE4EaygCJie2o htOC8m76Y1KIZrjAx9OZ5saxBX3HuTka8btDkpQ4Fuqmb+Y6CKdZJPRM0ZbIBrElBwWPj5HKg71t9 9+bzLMmLl6BaStFV893wHR0z95sa3r6vX4iJ33Rc7i6IbGRw8z0R5Q5zpSE5jSw/VT3PWp/i3H58e m3lpRhohg==; Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1kmISQ-0008Hf-1P; Mon, 07 Dec 2020 15:29:46 +0000 Received: from mail-ej1-x644.google.com ([2a00:1450:4864:20::644]) by merlin.infradead.org with esmtps (Exim 4.92.3 #3 (Red Hat Linux)) id 1kmISM-0008G5-Dy; Mon, 07 Dec 2020 15:29:44 +0000 Received: by mail-ej1-x644.google.com with SMTP id m19so20002387ejj.11; Mon, 07 Dec 2020 07:29:40 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=czieh0SMLeCnWTqxAlKEr+JxJKJgLdYp49r0pLF9I84=; b=exZdHbMuTVxUx0l4mqHiicOz1w8B0EnlED5S4nkg0zZCOdEchmV1whh1v85k93rpfL RGVHuXlfvosq/pvg7D7gAnJyLiqcUsxnrnOoptdrPQQKuca5WqNmiwmjwaSelz9hKqpY kFbvX+RLIMUNcDKgXoOR/vJAUp1526HO03y8GZkHjND0yiDQTwDha0xWdDh7Nezl2w+7 55KELOFdiy9rBAxzoLJYX1/6sQT/4EDNF31ZU8VRAP9JEJbJ7gbWesiHdb8qe2xUwfK3 YzGo4f4Akb4Ks+CzgHjXCGwu9y98M2XSIiCiKHxCP8StrwniFXq7Xltzu5m3n3K15bQz u6tA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=czieh0SMLeCnWTqxAlKEr+JxJKJgLdYp49r0pLF9I84=; b=tNl17XIg8Oh3b/k1BX4nHji191rQm0uyYDXa6PMntplbpelwKC+O2grxmICnODCs/0 Zt2EqKk+iOheGWpNeFVIkC+5l5sSWqq5GzYNHnlkyfx5nI06z7O2RXps0noQw9mDCZ/E VArV/UpswZqMixDrEleONcFk2n6lc+87eV3UkN/O5NVQ1I5U4bDyVqLfhFjH/IMc4TEX Vc9PrSYVJ9XxImgU4vcJt1raoC8MX6lgoiQU8ihrgsdT7jNbXRL+xU7511JZ1k0Ap14t jUeXtVDk+4YxJHARQCAqKOPeywV/+1h2sTPaKv81zZBfFh9d2W0D5uwOY5FGZsOqkLBg tWIw== X-Gm-Message-State: AOAM531Pnr0Z2LyQ0fnnDGivwm9Nbw4PKUEeoeOvW6WIWCmVpXNphK9E 2H5Gg+0qRfCsucN8tLGvr4A= X-Google-Smtp-Source: ABdhPJxy15rnADfl5o/5RYP4BifXScV3Maf+Q6Mcc//8MHh20OVX/3ZszuOCO0i+gvlfAoqNPukW7w== X-Received: by 2002:a17:906:3b56:: with SMTP id h22mr18815714ejf.491.1607354979213; Mon, 07 Dec 2020 07:29:39 -0800 (PST) Received: from localhost ([62.96.65.119]) by smtp.gmail.com with ESMTPSA id u17sm12676796eje.11.2020.12.07.07.29.37 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 07 Dec 2020 07:29:37 -0800 (PST) Date: Mon, 7 Dec 2020 16:29:36 +0100 From: Thierry Reding To: Uwe =?utf-8?Q?Kleine-K=C3=B6nig?= Subject: Re: [PATCH v2] pwm: bcm2835: Support apply function for atomic configuration Message-ID: References: <20201204084417.GA2154@gofer.mess.org> <20201204111326.qjux6k2472dmukot@pengutronix.de> <20201204113846.GA6547@gofer.mess.org> <20201204232834.xzsafkzfmfpw7pqz@pengutronix.de> <20201205173444.GA1265@gofer.mess.org> <20201205192510.o76pjs3yc524nwvm@pengutronix.de> <20201206141941.GA24807@gofer.mess.org> <20201207081628.tm3yg7az5k5sbivu@pengutronix.de> <20201207094320.GA10460@gofer.mess.org> <20201207135209.htp7plyotjxp37q2@pengutronix.de> MIME-Version: 1.0 In-Reply-To: <20201207135209.htp7plyotjxp37q2@pengutronix.de> User-Agent: Mutt/2.0.2 (d9268908) (2020-11-20) X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20201207_102942_519022_FA2075B7 X-CRM114-Status: GOOD ( 55.09 ) 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-arm-kernel@lists.infradead.org, linux-pwm@vger.kernel.org, f.fainelli@gmail.com, sbranden@broadcom.com, Sean Young , rjui@broadcom.com, linux-kernel@vger.kernel.org, Lino Sanfilippo , bcm-kernel-feedback-list@broadcom.com, linux-rpi-kernel@lists.infradead.org, lee.jones@linaro.org, nsaenzjulienne@suse.de Content-Type: multipart/mixed; boundary="===============8375233062605454347==" Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org --===============8375233062605454347== Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="sadRVDrrxYS6pVuJ" Content-Disposition: inline --sadRVDrrxYS6pVuJ Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Mon, Dec 07, 2020 at 02:52:09PM +0100, Uwe Kleine-K=C3=B6nig wrote: > Hello Sean, >=20 > On Mon, Dec 07, 2020 at 09:43:20AM +0000, Sean Young wrote: > > Thank you for taking the time to explain your thinking. >=20 > I'm happy you have an open ear for it. With this I really enjoy spending > the time to find the right arguments and examples. >=20 > > On Mon, Dec 07, 2020 at 09:16:28AM +0100, Uwe Kleine-K=C3=B6nig wrote: > > > On Sun, Dec 06, 2020 at 02:19:41PM +0000, Sean Young wrote: > > > > On Sat, Dec 05, 2020 at 08:25:10PM +0100, Uwe Kleine-K=C3=B6nig wro= te: > > > > > On Sat, Dec 05, 2020 at 05:34:44PM +0000, Sean Young wrote: > > > > > > What real life uses-cases are there for round down? If you want= to round > > > > > > down, is there any need for round up? > > > > >=20 > > > > > The scenario I have in mind is for driving a motor. I have to adm= it > > > > > however that usually the period doesn't matter much and it's the > > > > > duty_cycle that defines the motor's speed. So for this case the > > > > > conservative behaviour is round-down to not make the motor run fa= ster > > > > > than expected. > > > >=20 > > > > I am reading here that for driving motors, only the duty cycle matt= ers, > > > > not the period. > > >=20 > > > There is an upper limit (usually around 1 ms) for the period, but if = you > > > choose 0.1 ms or 0.001 ms doesn't matter much AFAICT. > > >=20 > > > @Thierry: Do you have further use cases in mind? >=20 > I asked in the hardware department of the company I work for and they > had another usecase: Motors where for example a 1 ms pulse means "move > forwards" and 2 ms means "move backwards". They had the same idea as I > had: You want to know beforehand the result of a given > pwm_apply_state(). I've occasionally considered the idea of adding a pwm_check_state() API that would allow you to pass in a struct pwm_state and get a result as to whether it can be applied or not. It's never really made much sense because pwm_apply_state() can already return failure if it can't apply the state. However, if we need some way for consumers to be more clever about state changes, then something like pwm_check_state() might be more useful if, in addition to just checking the validity/applicability of the state we can also return the state that would be applied after all the hardware- specific rounding. That way the consumer can use it to perform some checks on the resulting state and submit it if satisfied with the outcome. Alternatively, if it determines that the state is not viable, it can retry with different values. I'm not sure how useful that really is because it makes the usage really difficult on the consumer side. Perhaps there's no need for this anymore if the consumer is able to specify the rounding, so perhaps we should concentrate on that API first. > > > > > For other usecases (fan, backlight, LED) exactness typically does= n't > > > > > matter that much. > > > >=20 > > > > So, the use-cases you have are driving motor, fan, backlight, and l= ed. > > > > And in all these cases the exact Hz does not matter. > > > >=20 > > > > The only uses case where the exact Hz does matter is pwm-ir-tx.=20 > > > >=20 > > > > So, I gather there are no use-cases for round-down. Yes, should rou= nd-down > > > > be needed, then this is more difficult to implement if the driver a= lways > > > > does a round-closest. But, since there is no reason to have round-d= own, > > > > this is all academic. > > > >=20 > > > > Your policy of forcing new pwm drivers to use round-down is breaking > > > > pwm-ir-tx. > > >=20 > > > So you're indeed suggesting that the "right" rounding strategy for > > > lowlevel drivers should be: > > >=20 > > > - Use the period length closest to the requested period (in doubt ro= und > > > down?) > > > - With the chosen period length use the biggest duty_cycle not bigger > > > than the requested duty_cycle. > > >=20 > > > While this seems technically fine I think for maintenance this is a > > > nightmare. > > >=20 > > > My preference would be to stick to the rounding strategy we used so f= ar > > > (i.e.: > > >=20 > > > - Use the biggest period length not bigger than the requested period > > > - With the chosen period length use the biggest duty_cycle not bigger > > > than the requested duty_cycle. > > >=20 > > > ) and for pwm-ir-tx add support to the PWM API to still make it possi= ble > > > (and easy) to select the best setting. > > >=20 > > > The reasons why I think that this rounding-down strategy is the best > > > are (in order of importance): > > >=20 > > > - It is easier to implement correctly [1] > >=20 > > Yes, you are right. You have given a great example where a simple > > DIV_ROUND_CLOSEST() does not give the result you want. > >=20 > > > - Same rounding method for period and duty cycle > > > - most drivers already do this (I think) > > >=20 > > > The (IMHO nice) result would then mean: > > >=20 > > > - All consumers can get the setting they want; and > >=20 > > Once there is a nice pwm api for selecting round-nearest, then yes. > >=20 > > For the uses cases you've given, fan, backlight, and led a round-nearest > > is the rounding they would want, I would expect. >=20 > maybe, yes. Maybe it is also not important enough to spend the extra > cycles getting round nearest and so sticking to round-down is good > enough. Yeah, I think in most cases currently the consumer just doesn't care enough and things happen to just work. Maybe they're not perfect, but good enough. One of the reasons I was reluctant to introduce a "default" rounding behaviour is precisely because it's not clear cut, so in some cases the default may not be what we really want, such as in the pwm-ir-tx case here. > > > - Code in lowlevel drivers is simple and the complexity is in common > > > code and so a single place. > > >=20 > > > And it would also allow the pwm-ir-tx driver to notice if the PWM to = be > > > used can for example only support frequencies under 400 kHz. > >=20 > > I doubt pwm-ir-tx cares about this, however it is a nice-to-have. It wo= uld > > also be nice if the rounding could be used with atomic configuration > > as well. >=20 > I cannot follow, you created 11fc4edc483bea8bf0efa0cc726886d2342f6fa6 > because 476.2 Mhz was too bad. So you seem to be interested in > deviations and part of the problem is that you don't get feedback about > how your request is fulfilled. >=20 > > Please let me know when/if this new API exists for pwm so that pwm-ir-tx > > can select the right rounding. >=20 > Given that the bcm2835 driver is quite trivial I would be happy to > create a series that "fixes" the driver to round down and provide a > prototype for pwm_round_nearest for you to test on pwm-ir-tx. A willing > tester and a real use-case were the single two things that stopped me > investing time here. I'd like to avoid adding a new function for this functionality and instead add a rounding type field to the PWM state. Also, in doing so we should be able to keep the status quo for everyone by making the default rounding behaviour "don't care", which is what basically everyone right now uses. In specific cases like pwm-ir-tx we can adjust the rounding to become "nearest". That said, the rounding behaviour is not something that the API can guarantee, because if we start rejecting "nearest" requests, we might end up breaking a bunch of setups that want "nearest" but where the controller doesn't support it. At the same time I don't want to make it a prerequisite that all drivers implement all possible rounding behaviours because it puts a very high burden on the driver writer that may not need (or have a way of testing) anything other than "nearest", or "round down", or whatever. So I think from an API perspective the rounding behaviour would always have to be a sort of "hint" to the driver to specify what the consumer wants to use, but it should never fail to apply a state purely on this rounding behaviour, so returning some state that's the best the driver can do is better than failing if it doesn't know some mode. This also ensures that existing drivers will be able to continue to work the same way they always have, and the new mechanism is merely something to improve the use-cases where we need more precise control. Thierry --sadRVDrrxYS6pVuJ Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAABCAAdFiEEiOrDCAFJzPfAjcif3SOs138+s6EFAl/OSl0ACgkQ3SOs138+ s6Fp6g/9EGoRDw/oiLD0Rf2wTPLILTQuDgAqBJCnzfQ/lVteQy1PHo5Lvo/IE7c8 xBY2I7e3N3V39NmprqIBQdyXD9hEkVGMtWCcyO+gh7uvZyjKN4R7E0h1xzqV4JQv 35XZSVZiBSxjhyc2uMNrjtx3kbZqiNcRNyGjPCF/CrDNfHsxJqzt75n3MLTXz8py p+bPW+YvDzS/miKYlqxjeOVtxa7lid60kwfV4k89nBD+yPV7FqqAMkIawmqL31q3 tPxzFN8mcuqbJOb+k3uZlP8O16HDFxrUz0hSGxJ0Uzgyb0Z95rOquLwL0fmoRBEH 3a6Qi6jyEiwtJfMMIX77zLkrCFIRfrsFL7oqKvYjMTYHmUfJePBxtcDjFkskE7PU F1kt8ArcvV1taE2MrUxKfydNNdnC3eGfhdGgXfDYoVSpXTCD3Iwwyxn+4Bwso9W6 ni2duZ9PUdAZE0TzpQ2ngc3Vs5bvy828wjEpF+KaZ4KwcN+JwGuTpW1TympBZV57 7wI3L1y3jTpARP2BiRmduM168H1WbPVG8IhbtdjnSmfeHGY0vBGeId7HvASM1YA2 BzwYl3mLUEekRcZaMfhEPLg1JRaF2xYbTVHAamPmSM44G0dKHMYuvtWLQ6SF2MSI MOwIeomKCgob37BViH8IX1ZqZNTVjfJYxvXr14aYXqIXIFmeT6Q= =+2zM -----END PGP SIGNATURE----- --sadRVDrrxYS6pVuJ-- --===============8375233062605454347== 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 --===============8375233062605454347==--