Message-ID: <20200121125607.GA899558@ulmo>
Date:   Tue, 21 Jan 2020 13:56:07 +0100
From:   Thierry Reding <thierry.reding@...il.com>
To:     Uwe Kleine-König <u.kleine-koenig@...gutronix.de>
Cc:     Miquel Raynal <miquel.raynal@...tlin.com>,
        Linus Walleij <linus.walleij@...aro.org>,
        Bartosz Golaszewski <bgolaszewski@...libre.com>,
        linux-gpio@...r.kernel.org, linux-pwm@...r.kernel.org,
        Andy Shevchenko <andy.shevchenko@...il.com>,
        Thomas Petazzoni <thomas.petazzoni@...tlin.com>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v5] gpio: pca953x: Add Maxim MAX7313 PWM support

On Mon, Jan 20, 2020 at 03:44:57PM +0100, Uwe Kleine-König wrote:
> Hello Thierry,
> 
> On Mon, Jan 20, 2020 at 03:19:44PM +0100, Thierry Reding wrote:
> > On Mon, Jan 20, 2020 at 01:41:37PM +0100, Miquel Raynal wrote:
> > > Hi Thierry,
> > > 
> > > Thanks for reviewing,
> > > 
> > > > > +static bool max7313_pwm_reg_is_accessible(struct device *dev, unsigned int reg)
> > > > > +{
> > > > > +	struct pca953x_chip *chip = dev_get_drvdata(dev);
> > > > > +	unsigned int bank_sz = chip->driver_data & PCA_GPIO_MASK;
> > > > > +
> > > > > +	if (reg >= MAX7313_MASTER && reg < (MAX7313_INTENSITY + bank_sz))
> > > > > +		return true;
> > > > > +
> > > > > +	return false;
> > > > > +}
> > > > > +
> > > > >  static bool pca953x_readable_register(struct device *dev, unsigned int reg)
> > > > >  {
> > > > >  	struct pca953x_chip *chip = dev_get_drvdata(dev);
> > > > >  	u32 bank;
> > > > >  
> > > > > +	if ((chip->driver_data & MAX_PWM) &&
> > > > > +	    max7313_pwm_reg_is_accessible(dev, reg))
> > > > > +		return true;  
> > > > 
> > > > This doesn't look correct. The MAX_PWM flag doesn't signify that all
> > > > GPIOs are used in PWM mode, right? So the above check would return true
> > > > even if you're trying to access GPIO registers on a chip that has PWM
> > > > support.
> > > 
> > > Not exactly: this part returns true only if we are using a chip with
> > > PWM and we are accessing PWM registers.
> > > 
> > > Otherwise, for instance if we are accessing GPIO registers, this will
> > > not return anything.
> > > 
> > > > 
> > > > I think you still want to proceed with the checks below if reg doesn't
> > > > match any of the PWM related registers.
> > > 
> > > This is precisely what we do here. See the
> > > max7313_pwm_reg_is_accessible helper above: only the PWM registers are
> > > checked, I suppose this is the part you missed.
> > 
> > No idea what I missed, but on a second look, yes, you're absolutely
> > right.
> > 
> > > > So it'd be something more along
> > > > these lines:
> > > > 
> > > > 	if ((chip->driver_data & MAX_PWM) &&
> > > > 	    !max7313_pwm_reg_is_accessible(dev, reg))
> > > > 		return false;
> > > > 
> > > > > +
> > > > >  	if (PCA_CHIP_TYPE(chip->driver_data) == PCA953X_TYPE) {
> > > > >  		bank = PCA953x_BANK_INPUT | PCA953x_BANK_OUTPUT |
> > > > >  		       PCA953x_BANK_POLARITY | PCA953x_BANK_CONFIG;
> > > > > @@ -267,6 +318,10 @@ static bool pca953x_writeable_register(struct device *dev, unsigned int reg)
> > > > >  	struct pca953x_chip *chip = dev_get_drvdata(dev);
> > > > >  	u32 bank;
> > > > >  
> > > > > +	if ((chip->driver_data & MAX_PWM) &&
> > > > > +	    max7313_pwm_reg_is_accessible(dev, reg))
> > > > > +		return true;  
> > > > 
> > > > Same here.
> > > > 
> > > > > +
> > > > >  	if (PCA_CHIP_TYPE(chip->driver_data) == PCA953X_TYPE) {
> > > > >  		bank = PCA953x_BANK_OUTPUT | PCA953x_BANK_POLARITY |
> > > > >  			PCA953x_BANK_CONFIG;
> > > > > @@ -855,6 +910,335 @@ static int device_pca957x_init(struct pca953x_chip *chip, u32 invert)
> > > > >  	return ret;
> > > > >  }
> > > > >  
> > > 
> > > [...]
> > > 
> > > > > +static void max7313_pwm_free(struct pwm_chip *chip,
> > > > > +			     struct pwm_device *pwm)
> > > > > +{
> > > > > +	struct max7313_pwm_data *data = pwm_get_chip_data(pwm);
> > > > > +
> > > > > +	gpiochip_free_own_desc(data->desc);
> > > > > +	kfree(data);
> > > > > +}
> > > > > +
> > > > > +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 ||  
> > > > 
> > > > I think you should actually make this a != so that you refuse any
> > > > attempt to change the period, since you can't do it anyway.
> > > 
> > > Actually we discussed this with Uwe, see the below snippet:
> > > 
> > > ---8<---
> > > > > > +	if (state->period != PWM_PERIOD_NS ||
> > > > > > +	    state->polarity != PWM_POLARITY_NORMAL)
> > > > > > +		return -EINVAL;    
> > > > > 
> > > > > The check for period is too strong. Anything bigger than PWM_PERIOD_NS
> > > > > is acceptable, too. (The policy I'd like to see is: Provide the biggest
> > > > > period possible not bigger than the requested policy.)  
> > > > 
> > > > I don't understand, what is this parameter supposed to mean? the period
> > > > cannot be changed, it is ruled by an internal oscillator. In this case
> > > > any period bigger than the actual period cannot be physically achieved.
> > > > If we derive ratios with a bigger period than possible, why not
> > > > allowing it for lower periods too?  
> > > 
> > > Yes, I understood that the period is fixed for your PWM. However
> > > consider a consumer who would prefer a different period. If you decline
> > > all requests unless state->period == PWM_PERIOD_NS the consumer has no
> > > guide to determine that unless all periods are tested. If however asking
> > > for period = 2s results in getting 31.25 ms this allows the consumer to
> > > assume that no period setting between 2s and 31.25 ms is possible. And
> > > so the usual policy to implement is as stated in my previous mail.
> > > --->8---
> > 
> > I think I understand what Uwe is getting at, but I don't think we should
> > lie to consumers. It's true that in some cases the drivers will silently
> > use a slightly different period if they can't match the one requested,
> > but I don't think that's a good thing. Most of the time in those drivers
> > the computed period that the controller can support is "close enough".
> > 
> > But in this case the controller doesn't support anything other than the
> > one period, so I don't think accepting anything other than that is good
> > for any consumer.
> > 
> > Also, typically this doesn't really matter because this will have been
> > defined in device tree and if the device tree has the wrong period, then
> > this should already be caught before the buggy DTS is upstreamed.
> > 
> > So, I agree that the current situation is not ideal and perhaps we
> > should enforce stronger requirements for accuracy. I suspect that a good
> > solution would be for the drivers to report back the state that would've
> > been applied without actually applying it (kind of like the semantics of
> > clk_round_rate() from the common clock framework). That would give users
> > a good way of checking whether the supported parameters are within the
> > desired range before applying them. For consumers that don't care all
> > that much about precision, they can feel free to ignore any differences
> > between what they asked and what they got, and most of the time that
> > will be fine.
> 
> Yeah, it's something like clk_round_rate that I want in the end. And to
> make it actually workable the IMHO only sane approach is to allow
> rounding in one direction without limit. And as pwm_apply_state() should
> be consistent with pwm_round_state() the former must round without
> limit, too.

Agreed on the point that both pwm_round_state() and pwm_apply_state()
should do the same rounding. In fact, in most cases I'd expect drivers
to implement the bulk of ->apply() and ->round() in the same function
that basically constructs the new state that will be applied to the
hardware in ->apply() but will be returned from ->round().

I'm not so sure about rounding without limit, though. I think it makes
sense to allow rounding to happen if you can match things closely enough
for it not to matter in most cases. Strictly speaking we're already
breaking use-cases that require a fixed period because there's currently
no way for consumers to determine what the exact state is that is going
to get applied. Consumers could read back the state, but we already know
that that doesn't yield the correct result for some drivers.

Also, in practice, for the large majority of use-cases the exact period
doesn't matter as long as the actual numbers are close enough to the
requested values and the duty cycle/period ratio is about the same as
what was requested.

Similarily, the period will typically come from DT or board files, and
will usually be a value that was hand-picked and would typically match
some value that the hardware supports. And if it isn't a supported
value, it's usually close enough because it was tested to work by who
ever submitted the DT or board file patch.

That said, I think allowing a driver to clamp any given period to the
one and only period that it supports isn't right. There's no way that
the controller can get close to the requested value unless it matches
the fixed period. If we silently ignore that restriction, we're just
going to make it very simple to write DT content that's completely
bogus, yet will still work because we effectively ignore it.

> And if you want to require that a consumer of a PWM that only supports a
> single period setting passes that period, how do you want to handle the
> situation if this period happens to be 2000/3 ns. Is it ok to pass
> .period = 666? Is it ok to pass 667?

That seems like a bit of an artificial example. I suspect that a
consumer that supports 666 ns as period will run just fine when run with
a period of 667 ns. So it's ultimately up to the consumer driver to make
use of the pwm_round_state() (or whatever the name will end up to be)
and apply whatever error margins it can deal with before rejecting any
given state.

Also, these kinds of disconnects are very unlikely to happen in practice
because board designs usually get tested for this kind of compatibility
before they are produced. If some PWM consumer requires one specific
period, then the corresponding PWM producer would have been picked to be
compatible with that.

> > In many cases it doesn't matter because the period is defined in DT and
> > is hand-picked to be among the ones supported by the controller, or the
> > small differences between the period in DT and the closest one supported
> > by the controller is not significant and things will just work.
> 
> In my eyes to get a uniform behaviour of the PWM framework independant
> of the backend used, it must not be the driver who decides if a request
> is "close enough". We need a defined policy. And then it is obvious to
> me that this policy must be implemented in a way that it is in fact the
> consumer who has to decide which settings are ok and which are not. And
> then rounding without limit is the easiest to work with.

That still means that we'll be ignoring mismatches between fixed-period
producers and variable-period consumers. Allowing producers to overwrite
whatever is passed in (without potentially being able to get anywhere
near the requested values) is making it too easy to get things wrong,
don't you think?

> > However, ignoring period settings because the controller supports only a
> > fixed period seems a bit of an extreme.
> 
> So the setting I want is:
> 
> 	if (request.period < HW_PERIOD)
> 		fail();
> 		
> and with the reasoning above, that's the only sensible thing (apart from
> the revered policy of rounding up and so failing for requested periods
> that are bigger than the implementable period).

But that's just as arbitrary as anything else. request.period ==
HW_PERIOD - 1 might be an entirely fine setting in many cases.

Thierry

Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)

Powered by blists - more mailing lists