Re: [PATCH v4] gpio: pca953x: Add Maxim MAX7313 PWM support

[Uwe Kleine-König <u.kleine-koenig@xxxxxxxxxxxxxx>]

Hi Miquel,

On Mon, Dec 16, 2019 at 09:39:55AM +0100, Miquel Raynal wrote:
> Uwe Kleine-König <u.kleine-koenig@xxxxxxxxxxxxxx> wrote on Thu, 12 Dec
> 2019 22:14:34 +0100:
> 
> > On Fri, Nov 29, 2019 at 08:10:23PM +0100, Miquel Raynal wrote:
> > > +static int max7313_pwm_apply(struct pwm_chip *chip,
> > > +			     struct pwm_device *pwm,
> > > +			     const struct pwm_state *state)
> > > +{
> > > +	struct max7313_pwm *max_pwm = to_max7313_pwm(chip);
> > > +	struct pca953x_chip *pca_chip = to_pca953x(max_pwm);
> > > +	unsigned int intensity, active;
> > > +	int ret = 0;
> > > +
> > > +	if (!state->enabled ||
> > > +	    state->period < PWM_PERIOD_NS ||
> > > +	    state->polarity != PWM_POLARITY_NORMAL)
> > > +		return -EINVAL;
> > > +
> > > +	/* Convert the duty-cycle to be in the [0;16] range */
> > > +	intensity = DIV_ROUND_DOWN_ULL(state->duty_cycle,
> > > +				       state->period / PWM_DC_STATES);  
> > 
> > this looks wrong. The period must not have an influence on the selection
> > of the duty_cycle (other than duty_cycle < period). So given that the
> > period is fixed at 31.25 ms, the result of
> > 
> > 	pwm_apply_state(pwm, { .enabled = 1, .period = 2s, .duty_cycle = 16 ms })
> > 
> > must be the same as for
> > 
> > 	pwm_apply_state(pwm, { .enabled = 1, .period = 3s, .duty_cycle = 16 ms })
> 
> This was not my understanding of our previous discussion and, again, I
> don't really understand this choice but if the framework works like
> that we shall probably continue with this logic. However, all this
> should probably be explained directly in the kernel doc of each core
> helper, that would help a lot.

I agree. There is a pending doc patch and once Thierry applies it I plan
to add these details.

The idea is to make the policy simple (both computational and to
understand). With each policy there are strange corner cases, so for
sure you can come up with examples that yield results that are way off
from the request. The idea is that drivers all implement the same policy
and then create helper functions to match the different consumer needs.

> > . Also dividing by a division looses precision.
> 
> I agree but this is a problem with fixed point calculations. Maybe I
> could first multiply the numerator by a factor of 100 or 1000 to
> minimize the error. Do you have a better idea?

intensity = DIV_ROUND_DOWN_ULL((unsigned long long)state->duty_cycle * PWM_DC_STATES, state->period);

should be both more exact and cheaper to calculate. (But this is
somewhat moot given that state->period shouldn't be there.)
(And in general you have to care for overflowing, but I think that's not
a problem in practise here as struct pwm_state::duty_cycle is an int
(still), and PWM_DC_STATES is small.)
 
> > > +static void max7313_pwm_get_state(struct pwm_chip *chip,
> > > +				  struct pwm_device *pwm,
> > > +				  struct pwm_state *state)
> > > +{
> > > +	struct max7313_pwm *max_pwm = to_max7313_pwm(chip);
> > > +	struct pca953x_chip *pca_chip = to_pca953x(max_pwm);
> > > +	u8 intensity;
> > > +
> > > +	state->enabled = true;
> > > +	state->period = PWM_PERIOD_NS;
> > > +	state->polarity = PWM_POLARITY_NORMAL;
> > > +
> > > +	intensity = max7313_pwm_get_intensity(pca_chip, pwm->hwpwm);
> > > +	state->duty_cycle = intensity * PWM_OFFSET_NS;  
> > 
> > I think you need to take into account the blink phase bit.
> 
> It is already done by .pwm_get_intensity itself, no?

Ah, possible, I admin I didn't look deep enough to catch it there.

Best regards
Uwe

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