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

[Miquel Raynal <miquel.raynal@xxxxxxxxxxx>]

Hi Uwe,

Uwe Kleine-König <u.kleine-koenig@xxxxxxxxxxxxxx> wrote on Mon, 16 Dec
2019 09:54:24 +0100:

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

Great!

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

I fully understand and comply with this.

The above logic was not the first one that would have came off my mind
but it is 100% true that it keeps the computation easy (= less bugs, =
quicker) and has probably been much more pondered than mine.

> 
> > > . 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.)

I feel stupid now - let's put it on monday mood. Of course it's more
accurate this way.

> (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.)

Do you plan to change duty_cycle type?

Right now the multiplication cannot be over 500 000 which is totally
okay for a s32 but not for a s16 for instance. 

> > > > +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.

No problem, thanks anyway for all the careful reviews!

Thanks,
Miquèl