Re: [PATCH V6 2/5] pwm: Add i.MX TPM PWM driver support

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hello,

On Tue, Mar 19, 2019 at 11:49:32AM +0000, Anson Huang wrote:
> > On Tue, Mar 19, 2019 at 09:08:26AM +0000, Anson Huang wrote:
> > > > On Tue, Mar 19, 2019 at 06:50:12AM +0000, Anson Huang wrote:
> > > > > +	}
> > > > > +
> > > > > +	/* if no valid prescale found, use MAX instead */
> > > > > +	if ((!FIELD_FIT(PWM_IMX_TPM_SC_PS, prescale)))
> > > > > +		prescale = PWM_IMX_TPM_SC_PS >> __bf_shf(PWM_IMX_TPM_SC_PS);
> > > >
> > > > 		prescale = FIELD_PREP(PWM_IMX_TPM_SC_PS, PWM_IMX_TPM_SC_PS)
> > > >
> > > > What is the consequence if the calculated prescale isn't valid? I
> > > > assume this yields a greatly different period? If yes, this should result in
> > > > an error.
> > >
> > > Yes, that is what I intend to do, if a request period is too large and
> > > the prescale Can NOT meet the requirement, I force it to the largest
> > > value that hardware can Provide, I am NOT sure if it is the correct
> > > solution, if no, I can make it return error If round period returns an invalid
> > > result.
> > 
> > I'd say for sure it is not correct to configure for a period of 3 s if
> > 20 s are requested. Where to draw the line exactly I don't know either.
> > 
> > In my opinion we need a general guideline like:
> > 
> > 	If the requested period + duty_cycle combo isn't supported use
> > 	the biggest available period that is not bigger than requested,
> > 	scale duty cycle accordingly and pick the biggest available duty
> > 	cycle that is not bigger than the scaled value.
> > 	If the requested period is shorter than possible, return
> > 	-ERANGE.
> > 
> > I think the above is sensible, but that's something that IMHO must first be
> > declared to be "official" as it doesn't make sense if a single driver implements
> > this and the other still do their own stuff.
> 
> As for now, do you agree if I make TPM driver ONLY support those period/duty
> Setting that HW can support, for other request, just return -ERANGE to make it
> Simple until there is a general guideline available?

I would be surprised if that would result in something that is actually
usable. But feel free to try.

> > Additionally I would consider it beneficial to have something like:
> > 
> > 	int pwm_round_state(pwm, &state)
> > 
> > that modifies state according to the above rules without affecting the
> > hardware. With the respective driver callback the framework could then
> > implement stuff like: "If the resulting configuration deviates too much from
> > the requested state, return an error to the consumer." with having the
> > definition of "too much" in a single place. It would also allow a consumer
> > who cares much about the resulting waveform to determine the effects
> > without testing.
> > 
> > The framework could then add additional checks like:
> > 
> > 	int pwm_apply_state(pwm, state)
> > 	{
> > 		struct pwm_state rounded_state;
> > 
> > 		if IS_ENABLED(CONFIG_PWM_EXTRA_CHECKS):
> > 			rounded_state = pwm->ops->round_state(state);
> > 
> > 	if !was_rounded_according_to_the_rules(round_state):
> > 				warn(...)
> > 
> > 		pwm->ops->apply(pwm, state)
> > 
> > 		if IS_ENABLED(CONFIG_PWM_EXTRA_CHECKS):
> > 			actual_state = pwm->ops->get_state(state);
> > 			if actual_state != round_state:
> > 				warn(...)
> > 
> 
> As for now, do you agree if I add the "round state" function to make sure every time
> the newly requested state applied, the HW outputs the expected result? I can merge
> the "config" and "enable" function into 1 function and make sure of that.

Yeah, that sounds right. Splitting into enable and config is a thing
from the past and makes it hard to provide the needed atomicity.

> > > > > +	/*
> > > > > +	 * polarity settings will enabled/disable output status
> > > > > +	 * immediately, so here ONLY save the config and write
> > > > > +	 * it into register when channel is enabled/disabled.
> > > > > +	 */
> > > > > +	chan->config = val;
> > > >
> > > > This function's behaviour is strange. It configures the hardware
> > > > with the right the duty_cycle but not the polarity. I cannot imagine that
> > > > this not buggy.
> > >
> > > I think that is hardware limitation here, I used the polarity setting
> > > to enable/disable the channel, if we set the polarity here directly,
> > > the output status may NOT reflect the real state, if eventually the
> > > status is disabled, setting the polarity directly into register will
> > > make the output active, that is NOT expected, right? And that is why I put
> > > comments here.
> > 
> > The frameworks expectation is that .apply results in the hardware switches
> > to the new configuration directly after completing a previously configured
> > period. So if you change the period, get preempted and then change the
> > polarity three periods later that is a bug. If it's impossible to fix this in
> > software please do as good as possible (whatever that means) and add this
> > problem to the list of limitations at the top of the driver.
> 
> I think after merging the "config" and "enable" function into ONE function, the
> output result should can be expected and no "polarity setting late" issue, talking
> about the preempt between the period setting and polarity settings, should we
> use the spin_lock_irqsave() instead of mutex() for .apply?

No, I think mutexes are fine.
 
Best regards
Uwe

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



[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux