Message-ID: <20190319130633.2kiwwxqhyttizf3h@pengutronix.de>
Date:   Tue, 19 Mar 2019 14:06:33 +0100
From:   Uwe Kleine-König 
        <u.kleine-koenig@...gutronix.de>
To:     Anson Huang <anson.huang@....com>
Cc:     "mark.rutland@....com" <mark.rutland@....com>,
        "linux-pwm@...r.kernel.org" <linux-pwm@...r.kernel.org>,
        Robin Gong <yibin.gong@....com>,
        "schnitzeltony@...il.com" <schnitzeltony@...il.com>,
        "otavio@...ystems.com.br" <otavio@...ystems.com.br>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "festevam@...il.com" <festevam@...il.com>,
        "s.hauer@...gutronix.de" <s.hauer@...gutronix.de>,
        "jan.tuerk@...rion.com" <jan.tuerk@...rion.com>,
        "linux@...linux.org.uk" <linux@...linux.org.uk>,
        "robh+dt@...nel.org" <robh+dt@...nel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "thierry.reding@...il.com" <thierry.reding@...il.com>,
        "stefan@...er.ch" <stefan@...er.ch>,
        "kernel@...gutronix.de" <kernel@...gutronix.de>,
        Leonard Crestez <leonard.crestez@....com>,
        "shawnguo@...nel.org" <shawnguo@...nel.org>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        dl-linux-imx <linux-imx@....com>
Subject: Re: [PATCH V6 2/5] pwm: Add i.MX TPM PWM driver support

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/  |

Powered by blists - more mailing lists