Re: [PATCH 5/5] pwm: adp5585: Add Analog Devices ADP5585 support

[Uwe Kleine-König]

Hello Laurent,

On Wed, May 22, 2024 at 01:13:35PM +0300, Laurent Pinchart wrote:
> On Tue, May 21, 2024 at 03:05:53PM +0200, Uwe Kleine-König wrote:
> > On Tue, May 21, 2024 at 01:09:22PM +0300, Laurent Pinchart wrote:
> > > On Tue, May 21, 2024 at 10:51:26AM +0200, Uwe Kleine-König wrote:
> > > > On Mon, May 20, 2024 at 10:59:41PM +0300, Laurent Pinchart wrote:
> > > > > +	if (state->polarity == PWM_POLARITY_INVERSED)
> > > > > +		swap(on, off);
> > > > 
> > > > Uhh, no. Either you can do inverted polarity or you cannot. Don't claim
> > > > you can.
> > > 
> > > OK, but what's the rationale ? This is also an area where I couldn't
> > > find documentation.
> > 
> > I don't have a good rationale here. IMHO this inverted polarity stuff is
> > only a convenience for consumers because the start of the period isn't
> > visible from the output wave form (apart from (maybe) the moment where
> > you change the configuration) and so
> > 
> > 	.period = 5000, duty_cycle = 1000, polarity = PWM_POLARITY_NORMAL
> > 
> > isn't distinguishable from
> > 
> > 	.period = 5000, duty_cycle = 4000, polarity = PWM_POLARITY_INVERSED
> > 
> > . But it's a historic assumption of the pwm core that there is a
> > relevant difference between the two polarities and I want at least a
> > consistent behaviour among the lowlevel drivers. BTW, this convenience
> > is the reason I'm not yet clear how I want to implemement a duty_offset.
> 
> Consistency is certainly good. Inverting the duty cycle to implement
> inverted polarity would belong in the PWM core if we wanted to implement
> it in software I suppose. I'll drop it from the driver.

This isn't as easy as it sounds however. From the POV of the PWM core
the capabilities of the currently used hardware are unclear. So if a
request with (say) normal polarity and a certain duty_cycle + period
fails, it's unknown if it would be beneficial to try with inverted
polarity and if that is OK for the requesting consumer. So there is
information missing in both directions.

Best regards
Uwe

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

Attachment: signature.asc
Description: PGP signature