Message-ID: <20200120141944.GD206171@ulmo>
Date:   Mon, 20 Jan 2020 15:19:44 +0100
From:   Thierry Reding <thierry.reding@...il.com>
To:     Miquel Raynal <miquel.raynal@...tlin.com>
Cc:     Linus Walleij <linus.walleij@...aro.org>,
        Bartosz Golaszewski <bgolaszewski@...libre.com>,
        Uwe Kleine-Konig <u.kleine-koenig@...gutronix.de>,
        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 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.

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.

However, ignoring period settings because the controller supports only a
fixed period seems a bit of an extreme.

Thierry

> > > +	    state->polarity != PWM_POLARITY_NORMAL)
> > > +		return -EINVAL;
> > > +
> > > +	/* Convert the duty-cycle to be in the [0;16] range */
> > > +	intensity = max7313_pwm_duty_to_intensity(state->duty_cycle);
> > > +
> 
> [...]
> 
> > > @@ -1130,7 +1522,7 @@ static const struct of_device_id pca953x_dt_ids[] = {
> > >  
> > >  	{ .compatible = "maxim,max7310", .data = OF_953X( 8, 0), },
> > >  	{ .compatible = "maxim,max7312", .data = OF_953X(16, PCA_INT), },
> > > -	{ .compatible = "maxim,max7313", .data = OF_953X(16, PCA_INT), },
> > > +	{ .compatible = "maxim,max7313", .data = OF_953X(16, PCA_INT | MAX_PWM), },
> > >  	{ .compatible = "maxim,max7315", .data = OF_953X( 8, PCA_INT), },
> > >  	{ .compatible = "maxim,max7318", .data = OF_953X(16, PCA_INT), },  
> > 
> > Aren't you missing a call to pwmchip_remove() somewhere? Otherwise once
> > you unload the driver, the PWM chip will become dangling and any attempt
> > to access its PWMs will crash.
> 
> That's true, I'll correct.
> 
> 
> Thanks,
> Miquèl

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

Powered by blists - more mailing lists