From mboxrd@z Thu Jan 1 00:00:00 1970 From: Hans de Goede Subject: Re: [PATCH v3 04/15] pwm: lpss: Add range limit check for the base_unit register value Date: Tue, 7 Jul 2020 19:31:29 +0200 Message-ID: <1496178b-ce39-9285-ff75-cd39bc0e9aa7@redhat.com> References: <20200620121758.14836-1-hdegoede@redhat.com> <20200620121758.14836-5-hdegoede@redhat.com> <20200622073554.wf3smq3tvnr6t2xy@taurus.defre.kleine-koenig.org> <20200707073424.w6vd6e4bhl56kosd@pengutronix.de> Mime-Version: 1.0 Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 8bit Return-path: In-Reply-To: <20200707073424.w6vd6e4bhl56kosd@pengutronix.de> Content-Language: en-US Sender: linux-acpi-owner@vger.kernel.org To: =?UTF-8?Q?Uwe_Kleine-K=c3=b6nig?= Cc: Thierry Reding , Jani Nikula , Joonas Lahtinen , =?UTF-8?B?VmlsbGUgU3lyasOkbMOk?= , "Rafael J . Wysocki" , Len Brown , linux-pwm@vger.kernel.org, intel-gfx , dri-devel@lists.freedesktop.org, Andy Shevchenko , Mika Westerberg , linux-acpi@vger.kernel.org List-Id: linux-pwm@vger.kernel.org Hi, On 7/7/20 9:34 AM, Uwe Kleine-König wrote: > On Mon, Jul 06, 2020 at 10:53:08PM +0200, Hans de Goede wrote: >> Hi, >> >> Thank you for your review and sorry for the slow reply. > > No problem for me, I didn't hold my breath :-) > >>>> diff --git a/drivers/pwm/pwm-lpss.c b/drivers/pwm/pwm-lpss.c >>>> index 43b1fc634af1..80d0f9c64f9d 100644 >>>> --- a/drivers/pwm/pwm-lpss.c >>>> +++ b/drivers/pwm/pwm-lpss.c >>>> @@ -97,6 +97,9 @@ static void pwm_lpss_prepare(struct pwm_lpss_chip *lpwm, struct pwm_device *pwm, >>>> freq *= base_unit_range; >>>> base_unit = DIV_ROUND_CLOSEST_ULL(freq, c); >>> >>> DIV_ROUND_CLOSEST_ULL is most probably wrong, too. But I didn't spend >>> the time to actually confirm that. >> >> Yes I saw your comment elsewhere that the PWM API defines rounding >> in a certain direction, but fixing that falls outside of this patch. > > Yeah, sure. > >> [...] >> I hope this helps to explain what is going on a bit. > > I will try to make sense of that and reply to the patch directly when I > succeeded. > >> ### >> >> As for the behavior on base_unit==0 in the get_state method, >> as mentioned above I wrote that when I did not fully understood >> how the controller works. >> >> We really should never encounter this. >> >> But if we do then I think closest to the truth would be: >> >> state->period = UINT_MAX; >> state->duty_cycle = 0; > > I'd say state->period = 1 & state->duty_cycle = 0 is a better > representation. But that would suggest the output is configured for an infinitely high output frequency, but the frequency is actually 0, the reason why get_state needs to treat a base_unit val of 0 special at all is to avoid a division by 0, and in math dividing by 0 gives infinite, isn't UINT_MAX a better way to represent infinity ? Regards, Hans