Re: [PATCH v2 3/3] pwm: tegra: Implement .get_state()

[Thierry Reding <thierry.reding@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 2280 bytes --]

On Wed, Sep 30, 2026 at 11:54:03AM +0200, Uwe Kleine-König wrote:
> Hello Thierry,
> 
> On Tue, Sep 22, 2026 at 12:07:26PM +0200, Thierry Reding wrote:
> > On Mon, Sep 21, 2026 at 04:26:03PM +0200, Uwe Kleine-König wrote:
> > > On Mon, Sep 21, 2026 at 12:18:16PM +0200, Thierry Reding wrote:
> > > > It feels like this has too many assumptions built-in. That's mostly a
> > > > predefined issue, but I think if we want to get accurate hardware read-
> > > > out, we need to address this.
> > > > 
> > > > According to the register documentation, the PWM depth is 16 bits wide
> > > > (on generations where it can be programmed). The value defaults to 255
> > > > (which is n - 1 encoded, hence TEGRA_PWM_DEPTH), but it can technically
> > > > be reprogrammed to any 16-bit value, as far as I can tell.
> > > > 
> > > > So I think for this to be correct we'd need to read out the actual value
> > > > before overwriting with TEGRA_PWM_CSR_0 contents above. At that point I
> > > > think we'd need to either adjust the mask to be (2 * depth) - 1, or
> > > > maybe better yet, avoid masking it out arbitrarily based on the depth
> > > > and instead cap it at depth so we never exceed the 1:1 ratio for duty
> > > > cycle vs. period.
> > > 
> > > As long as .apply() also hardcodes TEGRA_PWM_DEPTH, it's IMO fine that
> > > .get_state() does so, too.
> > 
> > Okay, fair enough.
> 
> Is that an Ack then?

I've been thinking about this some more and I don't know if it really
makes sense to keep hard-coding TEGRA_PWM_DEPTH. If only .apply() uses
it, then it's mostly fine, I suppose, because we don't care what the
current (or initial) state is/was. So we either don't use the device or
we overwrite it with a custom set of values.

Once we add .get_state() into the mix, now we kind of have to care about
the initial state, because we might end up using those values. If we did
not care, what would be the point, right? Which means that if we read
out wrong values, we, well, get wrong values. Which then may mean that
we overwrite values that we shouldn't, etc.

I suppose this would be okay if we reject any depth values other than
the default as errors. But then we also significantly reduce the
usefulness of this patch.

Thierry

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