Re: [RESEND v5 2/2] pwm: core: Convert period and duty cycle to u64

[Uwe Kleine-König]

On Thu, Feb 13, 2020 at 01:06:49PM -0800, Guru Das Srinagesh wrote:
> On Thu, Feb 13, 2020 at 09:28:04PM +0100, Uwe Kleine-König wrote:
> > On Thu, Feb 13, 2020 at 11:39:26AM -0800, Guru Das Srinagesh wrote:
> > > On Thu, Feb 13, 2020 at 11:18:02AM +0100, Uwe Kleine-König wrote:
> > > > Hello,
> > > > 
> > > > On Wed, Feb 12, 2020 at 10:54:08AM -0800, Guru Das Srinagesh wrote:
> > > > > @@ -305,8 +305,8 @@ struct pwm_chip {
> > > > >   * @duty_cycle: duty cycle of the PWM signal (in nanoseconds)
> > > > >   */
> > > > >  struct pwm_capture {
> > > > > -	unsigned int period;
> > > > > -	unsigned int duty_cycle;
> > > > > +	u64 period;
> > > > > +	u64 duty_cycle;
> > > > >  };
> > > > 
> > > > Is this last hunk a separate change?
> > > > 
> > > > Otherwise looks fine.
> > > 
> > > No, this is very much a part of the change and not a separate one.
> > 
> > Not sure we understand each other. I wondered if conversion of the
> > pwm_capture stuff should be done separately. (OTOH I wonder if this is
> > used at all and already considered deleting it.)
> 
> I see. Could you please elaborate on your concerns? I think this hunk's
> being in this patch makes sense as duty and period should be converted
> to u64 throughout the file in one go.

I guess that capturing isn't much used if at all. So keeping it as
limited as it is today doesn't seem like a bad idea to me. (OK, you
could also accidentally break it such that we can say in a few years
time: This is broken since v5.6, obviously nobody cares and we remove it
for good :-))

> Also, it looks like pwm_capture is being used by pwm-sti.c and
> pwm-stm32.c currently.

Yeah, these two drivers provide the needed callback. That doesn't
necessarily mean there are active users. Also I'm convinced that there
are implementation problems. For example the commit that added capture
support for stm32 has in its commit log:

	Capture requires exclusive access (e.g. no pwm output running at
	the same time, to protect common prescaler).

but the capture callback doesn't even check if the PWMs are in use (but
I only looked quickly, so I might have missed something).

Apart from that I think that the capturing stuff doesn't really fit into
the PWM framework which is (apart from the capture callback and API
function) about PWM *outputs* and most hardware's I know about either
don't support capturing or it is located in a different IP than the PWM
output. (And it is not even possible today to register a driver that can
only capture but not drive a PWM output.)

Best regards
Uwe

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