lkml.org 
[lkml]   [2019]   [Dec]   [16]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v4] gpio: pca953x: Add Maxim MAX7313 PWM support
Hi Uwe,

Uwe Kleine-König <u.kleine-koenig@pengutronix.de> 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@pengutronix.de> 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

\
 
 \ /
  Last update: 2019-12-16 10:14    [W:0.214 / U:0.060 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site