Re: [PATCH v3 04/15] pwm: lpss: Add range limit check for the base_unit register value - Uwe Kleine-König

From: "Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>
To: Hans de Goede <hdegoede@redhat.com>
Cc: linux-pwm@vger.kernel.org,
	intel-gfx <intel-gfx@lists.freedesktop.org>,
	"Rafael J . Wysocki" <rjw@rjwysocki.net>,
	linux-acpi@vger.kernel.org,
	Thierry Reding <thierry.reding@gmail.com>,
	dri-devel@lists.freedesktop.org,
	Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
	Mika Westerberg <mika.westerberg@linux.intel.com>,
	Len Brown <lenb@kernel.org>
Subject: Re: [PATCH v3 04/15] pwm: lpss: Add range limit check for the base_unit register value
Date: Tue, 7 Jul 2020 09:34:24 +0200	[thread overview]
Message-ID: <20200707073424.w6vd6e4bhl56kosd@pengutronix.de> (raw)
In-Reply-To: <e07a7dd6-c361-32b8-c4e7-91e022f9c21d@redhat.com>

[-- Attachment #1.1: Type: text/plain, Size: 1613 bytes --]

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.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel