Re: [PATCH v5 2/7] pwm: pca9685: Support hardware readout

From: Clemens Gruber <clemens.gruber@pqgruber.com>
To: "Thierry Reding" <thierry.reding@gmail.com>,
	"Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>,
	"Sven Van Asbroeck" <thesven73@gmail.com>
Cc: Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	linux-pwm@vger.kernel.org
Subject: Re: [PATCH v5 2/7] pwm: pca9685: Support hardware readout
Date: Sun, 3 Jan 2021 18:04:10 +0100	[thread overview]
Message-ID: <X/H5CqcRHelg5M4p@workstation.tuxnet> (raw)
In-Reply-To: <CAGngYiUYOL6EF3VTGwcwTuN4EmE26ML3ye7689FTEpowjEcU2w@mail.gmail.com>

Hi everyone,

happy new year, hope you are all well!

On Thu, Dec 17, 2020 at 12:52:42PM -0500, Sven Van Asbroeck wrote:
> On Thu, Dec 17, 2020 at 12:43 PM Clemens Gruber
> <clemens.gruber@pqgruber.com> wrote:
> > >
> > > Conclusion: .get_state() will always return "pwm disabled", so why do we
> > > bother reading out the h/w?
> >
> > If there are no plans for the PWM core to call .get_state more often in
> > the future, we could just read out the period and return 0 duty and
> > disabled.
> 
> I'm not sure why we should even read out the period?
> When a channel is disabled, the period is not externally visible,
> therefore it's meaningless ?
> 
> As far as I can tell, we can use this for .get_state():
> memset(&pwm->state, 0, sizeof(pwm_state));
> 
> >
> > Thierry, Uwe, what's your take on this?

I will continue working on this series in the upcoming weeks.
Feedback on the .get_state issue would be greatly appreciated.

To summarize:
Is it OK for a driver to expect the chip->ops->get_state function to be
only called from the place in pwm core it is currently called from?
(Namely in pwm_device_request after chip->ops->request)

If yes, we could always return a 0 duty cycle and disabled state,
because this is the state we left it in after .probe (and .free).

However, if in the future, the pwm core adds additional calls to
chip->ops->get_state in other places, this could lead to problems.

--

Another point is the period: Sven suggested we do not read out the
period at all, as the PWM is disabled anyway (see above).
Is this acceptable?

And, if we never return anything but 0 in .get_state, should it be
implemented at all?

> >
> > > Of course, if we choose to leave the pwm enabled after .free(), then
> > > .get_state() can even be left out! Do we want that? Genuine question, I do
> > > not know the answer.
> >
> > I do not think we should leave it enabled after free. It is less
> > complicated if we know that unrequested channels are not in use.
> >
> 
> Good point, I agree with you.

Thanks and best regards,
Clemens