[v3] gpio: pca953x: Add Maxim MAX7313 PWM support
diff mbox series

Message ID 20191122113230.16486-1-miquel.raynal@bootlin.com
State New
Headers show
Series
  • [v3] gpio: pca953x: Add Maxim MAX7313 PWM support
Related show

Commit Message

Miquel Raynal Nov. 22, 2019, 11:32 a.m. UTC
The MAX7313 chip is fully compatible with the PCA9535 on its basic
functions but can also manage the intensity on each of its ports with
PWM. Each output is independent and may be tuned with 16 values (4
bits per output). The period is always 32kHz, only the duty-cycle may
be changed. One can use any output as GPIO or PWM.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---

Changes in v3:
* Added two error messages in ->request().
* Protected the PWM count agains races with an additional mutex.
* Dropped an useless check on the period value in ->apply().
* Forced the .period to be constant.
* Checked state->polarity when needed.
* Used DIV_ROUND_DOWN_ULL for computing the duty_cycle.
* Implemented ->get_state().
* Added a comment to explain that the GPIO functionality is not harmed
  by the global intensity setting.

Changes in v2:
* Removed the hardcoding of PWM_CHANNELS, changed the code to use the
  number of GPIO lines which is programatically known.
* Used per pwm_device chip data to store the GPIO descriptors instead
  of having a static array of GPIO descriptors in the private PWM
  structure. It also enhanced the readability.
* Rename an offset variable: s/off/shift/.
* The default PWM state is now static low instead of input.
* Used the GPIO as regular consumer thanks to the stored GPIO
  descriptors to "make it more idiomatic" (requested by Thierry).
* Used gpiochip_request_own_desc() instead of
  gpio_to_desc()/gpiod_request(). This prevented the build issue and
  an additional dependency that would have requested a DEPENDS ON line
  in Kconfig.
* Enhanced the return line of max7313_pwm_probe().


 drivers/gpio/gpio-pca953x.c | 332 +++++++++++++++++++++++++++++++++++-
 1 file changed, 330 insertions(+), 2 deletions(-)

Comments

Linus Walleij Nov. 22, 2019, 1:23 p.m. UTC | #1
On Fri, Nov 22, 2019 at 12:32 PM Miquel Raynal
<miquel.raynal@bootlin.com> wrote:

> The MAX7313 chip is fully compatible with the PCA9535 on its basic
> functions but can also manage the intensity on each of its ports with
> PWM. Each output is independent and may be tuned with 16 values (4
> bits per output). The period is always 32kHz, only the duty-cycle may
> be changed. One can use any output as GPIO or PWM.
>
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>

I'm happy with this patch, but I would need Thierry's consent
to merge it so waiting for his ACK.

Yours,
Linus Walleij
Thierry Reding Nov. 25, 2019, 7:14 a.m. UTC | #2
On Fri, Nov 22, 2019 at 12:32:30PM +0100, Miquel Raynal wrote:
> The MAX7313 chip is fully compatible with the PCA9535 on its basic
> functions but can also manage the intensity on each of its ports with
> PWM. Each output is independent and may be tuned with 16 values (4
> bits per output). The period is always 32kHz, only the duty-cycle may
> be changed. One can use any output as GPIO or PWM.
> 
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---
> 
> Changes in v3:
> * Added two error messages in ->request().
> * Protected the PWM count agains races with an additional mutex.
> * Dropped an useless check on the period value in ->apply().
> * Forced the .period to be constant.
> * Checked state->polarity when needed.
> * Used DIV_ROUND_DOWN_ULL for computing the duty_cycle.
> * Implemented ->get_state().
> * Added a comment to explain that the GPIO functionality is not harmed
>   by the global intensity setting.
> 
> Changes in v2:
> * Removed the hardcoding of PWM_CHANNELS, changed the code to use the
>   number of GPIO lines which is programatically known.
> * Used per pwm_device chip data to store the GPIO descriptors instead
>   of having a static array of GPIO descriptors in the private PWM
>   structure. It also enhanced the readability.
> * Rename an offset variable: s/off/shift/.
> * The default PWM state is now static low instead of input.
> * Used the GPIO as regular consumer thanks to the stored GPIO
>   descriptors to "make it more idiomatic" (requested by Thierry).
> * Used gpiochip_request_own_desc() instead of
>   gpio_to_desc()/gpiod_request(). This prevented the build issue and
>   an additional dependency that would have requested a DEPENDS ON line
>   in Kconfig.
> * Enhanced the return line of max7313_pwm_probe().
> 
> 
>  drivers/gpio/gpio-pca953x.c | 332 +++++++++++++++++++++++++++++++++++-
>  1 file changed, 330 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpio/gpio-pca953x.c b/drivers/gpio/gpio-pca953x.c
> index de5d1383f28d..baf639bec18d 100644
> --- a/drivers/gpio/gpio-pca953x.c
> +++ b/drivers/gpio/gpio-pca953x.c
> @@ -12,18 +12,22 @@
>  #include <linux/bits.h>
>  #include <linux/gpio/driver.h>
>  #include <linux/gpio/consumer.h>
> +#include <linux/gpio/machine.h>
>  #include <linux/i2c.h>
>  #include <linux/init.h>
>  #include <linux/interrupt.h>
>  #include <linux/module.h>
>  #include <linux/of_platform.h>
>  #include <linux/platform_data/pca953x.h>
> +#include <linux/pwm.h>
>  #include <linux/regmap.h>
>  #include <linux/regulator/consumer.h>
>  #include <linux/slab.h>
>  
>  #include <asm/unaligned.h>
>  
> +#include "gpiolib.h"
> +
>  #define PCA953X_INPUT		0x00
>  #define PCA953X_OUTPUT		0x01
>  #define PCA953X_INVERT		0x02
> @@ -63,11 +67,18 @@
>  
>  #define PCA_INT			BIT(8)
>  #define PCA_PCAL		BIT(9)
> +#define MAX_PWM			BIT(10)
>  #define PCA_LATCH_INT		(PCA_PCAL | PCA_INT)
>  #define PCA953X_TYPE		BIT(12)
>  #define PCA957X_TYPE		BIT(13)
>  #define PCA_TYPE_MASK		GENMASK(15, 12)
>  
> +#define MAX7313_MASTER		0x0E
> +#define MAX7313_CONFIGURATION	0x0F
> +#define MAX7313_INTENSITY	0x10
> +
> +#define MAX7313_GLOB_INTENSITY	BIT(2)
> +
>  #define PCA_CHIP_TYPE(x)	((x) & PCA_TYPE_MASK)
>  
>  static const struct i2c_device_id pca953x_id[] = {
> @@ -93,7 +104,7 @@ static const struct i2c_device_id pca953x_id[] = {
>  
>  	{ "max7310", 8  | PCA953X_TYPE, },
>  	{ "max7312", 16 | PCA953X_TYPE | PCA_INT, },
> -	{ "max7313", 16 | PCA953X_TYPE | PCA_INT, },
> +	{ "max7313", 16 | PCA953X_TYPE | PCA_INT | MAX_PWM, },
>  	{ "max7315", 8  | PCA953X_TYPE | PCA_INT, },
>  	{ "max7318", 16 | PCA953X_TYPE | PCA_INT, },
>  	{ "pca6107", 8  | PCA953X_TYPE | PCA_INT, },
> @@ -118,6 +129,14 @@ MODULE_DEVICE_TABLE(acpi, pca953x_acpi_ids);
>  
>  #define NBANK(chip) DIV_ROUND_UP(chip->gpio_chip.ngpio, BANK_SZ)
>  
> +#define PWM_PER_REG 2
> +#define PWM_BITS_PER_REG (8 / PWM_PER_REG)
> +#define PWM_INTENSITY_MASK GENMASK(PWM_BITS_PER_REG - 1, 0)
> +
> +#define PWM_PERIOD_NS 31250
> +#define PWM_DC_STATES 16
> +#define PWM_OFFSET_NS (PWM_PERIOD_NS / PWM_DC_STATES)
> +
>  struct pca953x_reg_config {
>  	int direction;
>  	int output;
> @@ -139,6 +158,22 @@ static const struct pca953x_reg_config pca957x_regs = {
>  	.invert = PCA957X_INVRT,
>  };
>  
> +struct max7313_pwm_data {
> +	struct gpio_desc *desc;
> +	bool enabled;
> +	unsigned int duty_cycle;
> +};
> +
> +struct max7313_pwm {
> +	struct pwm_chip chip;
> +	/*
> +	 * Protect races when counting active PWMs for enabling or disabling
> +	 * the internal oscillator.
> +	 */
> +	struct mutex count_lock;
> +	unsigned int count;
> +};
> +
>  struct pca953x_chip {
>  	unsigned gpio_start;
>  	struct mutex i2c_lock;
> @@ -161,6 +196,8 @@ struct pca953x_chip {
>  	struct regulator *regulator;
>  
>  	const struct pca953x_reg_config *regs;
> +
> +	struct max7313_pwm pwm;
>  };
>  
>  static int pca953x_bank_shift(struct pca953x_chip *chip)
> @@ -241,8 +278,16 @@ static bool pca953x_check_register(struct pca953x_chip *chip, unsigned int reg,
>  static bool pca953x_readable_register(struct device *dev, unsigned int reg)
>  {
>  	struct pca953x_chip *chip = dev_get_drvdata(dev);
> +	unsigned int bank_sz = chip->driver_data & PCA_GPIO_MASK;
>  	u32 bank;
>  
> +	if (PCA_CHIP_TYPE(chip->driver_data) == PCA953X_TYPE &&
> +	    chip->driver_data & MAX_PWM) {
> +		if (reg >= MAX7313_MASTER &&
> +		    reg < (MAX7313_INTENSITY + bank_sz))
> +			return true;
> +	}
> +
>  	if (PCA_CHIP_TYPE(chip->driver_data) == PCA953X_TYPE) {
>  		bank = PCA953x_BANK_INPUT | PCA953x_BANK_OUTPUT |
>  		       PCA953x_BANK_POLARITY | PCA953x_BANK_CONFIG;
> @@ -264,8 +309,16 @@ static bool pca953x_readable_register(struct device *dev, unsigned int reg)
>  static bool pca953x_writeable_register(struct device *dev, unsigned int reg)
>  {
>  	struct pca953x_chip *chip = dev_get_drvdata(dev);
> +	unsigned int bank_sz = chip->driver_data & PCA_GPIO_MASK;
>  	u32 bank;
>  
> +	if (PCA_CHIP_TYPE(chip->driver_data) == PCA953X_TYPE &&
> +	    chip->driver_data & MAX_PWM) {
> +		if (reg >= MAX7313_MASTER &&
> +		    reg < (MAX7313_INTENSITY + bank_sz))
> +			return true;
> +	}
> +
>  	if (PCA_CHIP_TYPE(chip->driver_data) == PCA953X_TYPE) {
>  		bank = PCA953x_BANK_OUTPUT | PCA953x_BANK_POLARITY |
>  			PCA953x_BANK_CONFIG;
> @@ -886,6 +939,278 @@ static int device_pca957x_init(struct pca953x_chip *chip, u32 invert)
>  	return ret;
>  }
>  
> +/* PWM specific methods */
> +
> +static struct max7313_pwm *to_max7313_pwm(struct pwm_chip *chip)
> +{
> +	return container_of(chip, struct max7313_pwm, chip);
> +}
> +
> +static struct pca953x_chip *to_pca953x(struct max7313_pwm *chip)
> +{
> +	return container_of(chip, struct pca953x_chip, pwm);
> +}
> +
> +static u8 max7313_pwm_get_intensity(struct pca953x_chip *chip,
> +				    unsigned int idx)
> +{
> +	unsigned int reg, shift, val;
> +	u8 duty_cycle;
> +
> +	reg = MAX7313_INTENSITY + (idx / PWM_PER_REG);
> +	shift = (idx % PWM_PER_REG) ? PWM_BITS_PER_REG : 0;
> +
> +	mutex_lock(&chip->i2c_lock);
> +	regmap_read(chip->regmap, reg, &val);
> +	mutex_unlock(&chip->i2c_lock);
> +
> +	if (shift)
> +		val >>= shift;
> +
> +	val &= PWM_INTENSITY_MASK;
> +
> +	/*
> +	 * Register values in the [0;15] range mean a value in the [1;16] range.
> +	 * A register value of 16 means the logic has been inverted to produce a
> +	 * static low output.
> +	 */
> +	if (val == PWM_INTENSITY_MASK)
> +		duty_cycle = 0;
> +	else
> +		duty_cycle = val + 1;

That comment doesn't seem right. PWM_INTENSITY_MASK is 0xf as far as I
can tell, which means that your comment above is off-by-one with regards
to the upper limit of the range.

Also, a register value of 16 doesn't make sense if your field is 4 bits
wide.

> +
> +	return duty_cycle;
> +}
> +
> +static int max7313_pwm_set_intensity(struct pca953x_chip *chip,
> +				     unsigned int idx, u8 duty_cycle)
> +{
> +	/* Duty-cycle is in the range [1;16] while registers expect [0;15] */
> +	u8 intensity = (duty_cycle - 1) & PWM_INTENSITY_MASK;
> +	unsigned int reg, shift;
> +	u8 val, mask;
> +	int ret;
> +
> +	reg = MAX7313_INTENSITY + (idx / PWM_PER_REG);
> +	shift = (idx % PWM_PER_REG) ? PWM_BITS_PER_REG : 0;
> +
> +	mask = PWM_INTENSITY_MASK << shift;
> +	val = intensity << shift;
> +
> +	mutex_lock(&chip->i2c_lock);
> +	ret = regmap_write_bits(chip->regmap, reg, mask, val);
> +	mutex_unlock(&chip->i2c_lock);
> +
> +	return ret;
> +}
> +
> +/*
> + * For a given PWM channel, when the blink phase 0 bit is set, the intensity
> + * range is only [1/16;16/16]. With this range, a static low output is
> + * physically not possible. When the blink phase 0 bit is cleared, the intensity
> + * range is [15/16;0/16] which then allows a static low output but not a static
> + * high output.
> + *
> + * In this driver we choose to set the blink phase 0 bit by default, hence we
> + * can slide from a low output to a fully high output without glitch. However,
> + * the only way to get a static low output is by clearing the blink phase 0 bit,
> + * and by changing the intensity value to its maximum (as, at this moment,
> + * intensity is reversed). There is no way to atomically flip the register *and*
> + * change the PWM value at the same time so this will produce a small glitch.
> + */
> +static int max7313_pwm_set_state(struct pca953x_chip *chip,
> +				 struct pwm_device *pwm_device,
> +				 unsigned int duty_cycle)
> +{
> +	struct max7313_pwm_data *data = pwm_get_chip_data(pwm_device);
> +	struct gpio_desc *desc = data->desc;
> +	int ret;
> +
> +	/* A null duty_cycle will invert the phase */
> +	ret = gpiod_direction_output(desc, duty_cycle);

It might be worth making this a little more explicit. While gpiolib does
reduce the value to [0, 1], and hence this should work correctly, I find
this a little confusing to read.

> +	if (ret)
> +		return ret;
> +
> +	/* Maximize the low time in case of static low state */
> +	if (!duty_cycle)
> +		duty_cycle = PWM_DC_STATES;
> +
> +	return max7313_pwm_set_intensity(chip, pwm_device->hwpwm, duty_cycle);
> +}
> +
> +static int max7313_pwm_request(struct pwm_chip *pwm_chip,
> +			       struct pwm_device *pwm_device)
> +{
> +	struct max7313_pwm *pwm = to_max7313_pwm(pwm_chip);
> +	struct pca953x_chip *chip = to_pca953x(pwm);
> +	struct max7313_pwm_data *data;
> +	struct gpio_desc *desc;
> +	int ret;
> +
> +	desc = gpiochip_request_own_desc(&chip->gpio_chip, pwm_device->hwpwm,
> +					 "max7313-pwm", GPIO_ACTIVE_HIGH,
> +					 GPIOD_OUT_LOW);
> +	if (IS_ERR(desc)) {
> +		dev_err(&chip->client->dev,
> +			"pin already in use (probably as GPIO)\n");
> +		return PTR_ERR(desc);
> +	}
> +
> +	data = devm_kzalloc(&chip->client->dev, sizeof(*data), GFP_KERNEL);

There should be no need to use managed memory in this case since the
core will take care of calling ->free() on the PWM at the right time. I
can't think of a case where the memory wouldn't be freed, unless perhaps
if there's a crash somewhere and the subsystem is thrown off course, in
which case memory leaks should be the least of your worries.

> +	if (!data)
> +		return -ENOMEM;
> +
> +	data->desc = desc;
> +	pwm_set_chip_data(pwm_device, data);
> +
> +	ret = max7313_pwm_set_state(chip, pwm_device, 0);
> +	if (ret) {
> +		dev_err(&chip->client->dev, "cannot set PWM default state\n");
> +		goto free_gpiod;
> +	}
> +
> +	/*
> +	 * Set master intensity to the maximum level to let individual outputs
> +	 * the greatest flexibility range. Also enables the internal oscillator.
> +	 */
> +	mutex_lock(&pwm->count_lock);
> +	if (!pwm->count) {
> +		mutex_lock(&chip->i2c_lock);
> +		ret = regmap_write_bits(chip->regmap, MAX7313_MASTER,
> +					PWM_INTENSITY_MASK << PWM_BITS_PER_REG,
> +					PWM_INTENSITY_MASK << PWM_BITS_PER_REG);

This is a bit of a nitpick, but it seems to me like PWM_BITS_PER_REG
isn't really appropriate here since this is a register completely
different from those that control the PWM functionality of the PWMs.

Using the same definition implies some sort of correlation here, even
though it seems like this is really just a coincidence.

> +		mutex_unlock(&chip->i2c_lock);
> +	}
> +
> +	if (!ret)
> +		pwm->count++;
> +
> +	mutex_unlock(&pwm->count_lock);
> +
> +	if (ret)
> +		goto free_gpiod;
> +
> +	return 0;
> +
> +free_gpiod:
> +	gpiochip_free_own_desc(data->desc);
> +
> +	return ret;
> +}
> +
> +static void max7313_pwm_free(struct pwm_chip *pwm_chip,
> +			     struct pwm_device *pwm_device)
> +{
> +	struct max7313_pwm_data *data = pwm_get_chip_data(pwm_device);
> +	struct max7313_pwm *pwm = to_max7313_pwm(pwm_chip);
> +	struct pca953x_chip *chip = to_pca953x(pwm);
> +
> +	max7313_pwm_set_state(chip, pwm_device, 0);
> +
> +	mutex_lock(&pwm->count_lock);
> +
> +	pwm->count--;
> +
> +	/* Disable the internal oscillator if no channel is in use */
> +	if (!pwm->count) {
> +		mutex_lock(&chip->i2c_lock);
> +		regmap_write_bits(chip->regmap, MAX7313_MASTER,
> +				  PWM_INTENSITY_MASK << PWM_BITS_PER_REG, 0);
> +		mutex_unlock(&chip->i2c_lock);
> +	}
> +
> +	mutex_unlock(&pwm->count_lock);
> +
> +	gpiochip_free_own_desc(data->desc);
> +}
> +
> +static int max7313_pwm_apply(struct pwm_chip *pwm_chip,
> +			     struct pwm_device *pwm_device,

We'd typically just call this "pwm".

> +			     const struct pwm_state *state)
> +{
> +	struct max7313_pwm_data *data = pwm_get_chip_data(pwm_device);
> +	struct max7313_pwm *pwm = to_max7313_pwm(pwm_chip);
> +	struct pca953x_chip *chip = to_pca953x(pwm);
> +	unsigned int duty_cycle;
> +
> +	if (state->period != PWM_PERIOD_NS ||
> +	    state->polarity != PWM_POLARITY_NORMAL)
> +		return -EINVAL;
> +
> +	data->enabled = state->enabled;
> +	data->duty_cycle = state->duty_cycle;
> +
> +	if (!state->enabled || !state->duty_cycle)
> +		duty_cycle = 0;
> +	else
> +		/* Convert the duty-cycle to be in the [1;16] range */
> +		duty_cycle = DIV_ROUND_DOWN_ULL(state->duty_cycle,
> +						PWM_OFFSET_NS);

If duty-cycle is somewhere in the range [1, PWM_OFFSET_NS - 1], then you
still end up with duty-cycle = 0 here, right?

> +
> +	/* The hardware is supposedly glitch-free */

Not sure I understand this comment. Didn't you say above that it's in
fact not glitch-free in the case where you need to change the blink bit?

> +	return max7313_pwm_set_state(chip, pwm_device, duty_cycle);
> +}
> +
> +static void max7313_pwm_get_state(struct pwm_chip *pwm_chip,
> +				  struct pwm_device *pwm_device,
> +				  struct pwm_state *state)
> +{
> +	struct max7313_pwm_data *data = pwm_get_chip_data(pwm_device);
> +	struct max7313_pwm *pwm = to_max7313_pwm(pwm_chip);
> +	struct pca953x_chip *chip = to_pca953x(pwm);
> +	u8 duty_cycle;
> +
> +	state->period = PWM_PERIOD_NS;
> +	state->polarity = PWM_POLARITY_NORMAL;
> +
> +	if (!data)
> +		return;
> +
> +	state->enabled = data->enabled;
> +	if (!state->enabled) {
> +		state->duty_cycle = data->duty_cycle;
> +	} else {
> +		duty_cycle = max7313_pwm_get_intensity(chip, pwm_device->hwpwm);
> +		state->duty_cycle = duty_cycle * PWM_OFFSET_NS;
> +	}
> +};
> +
> +static const struct pwm_ops max7313_pwm_ops = {
> +	.request = max7313_pwm_request,
> +	.free = max7313_pwm_free,
> +	.apply = max7313_pwm_apply,
> +	.get_state = max7313_pwm_get_state,
> +	.owner = THIS_MODULE,
> +};
> +
> +static int max7313_pwm_probe(struct device *dev,
> +			     struct pca953x_chip *chip)
> +{
> +	struct max7313_pwm *pwm = &chip->pwm;
> +	struct pwm_chip *pwm_chip = &pwm->chip;
> +	int ret;
> +
> +	if (!(chip->driver_data & MAX_PWM))
> +		return 0;
> +
> +	pwm_chip->dev = dev;
> +	pwm_chip->ops = &max7313_pwm_ops;
> +	pwm_chip->npwm = chip->gpio_chip.ngpio;
> +	pwm_chip->base = -1;
> +
> +	/* Disable global control (does not affect GPIO functionality) */
> +	mutex_lock(&chip->i2c_lock);
> +	ret = regmap_write_bits(chip->regmap, MAX7313_CONFIGURATION,
> +				MAX7313_GLOB_INTENSITY, 0);
> +	mutex_unlock(&chip->i2c_lock);
> +	if (ret)
> +		return ret;
> +
> +	mutex_init(&pwm->count_lock);
> +
> +	return pwmchip_add(pwm_chip);
> +}
> +
>  static const struct of_device_id pca953x_dt_ids[];
>  
>  static int pca953x_probe(struct i2c_client *client,
> @@ -1018,6 +1343,9 @@ static int pca953x_probe(struct i2c_client *client,
>  			dev_warn(&client->dev, "setup failed, %d\n", ret);
>  	}
>  
> +	if (IS_ENABLED(CONFIG_PWM))
> +		return max7313_pwm_probe(&client->dev, chip);

Might be more cautious to use a regular error check here, rather than a
plain return statement. As it is, if somebody's not careful, they'll go
and add some code between this and the "return 0;" below and end up
scratching their head why the code isn't getting executed on chips that
support PWM.

I guess that would be on them, but why not be proactive about preventing
that from happening in the first place? You never know if the person you
might be helping out ends up being yourself...

Thierry

> +
>  	return 0;
>  
>  err_exit:
> @@ -1162,7 +1490,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), },
>  
> -- 
> 2.20.1
>
Uwe Kleine-König Nov. 25, 2019, 8:38 p.m. UTC | #3
Hello,

On Fri, Nov 22, 2019 at 12:32:30PM +0100, Miquel Raynal wrote:
> The MAX7313 chip is fully compatible with the PCA9535 on its basic
> functions but can also manage the intensity on each of its ports with
> PWM. Each output is independent and may be tuned with 16 values (4
> bits per output). The period is always 32kHz, only the duty-cycle may
> be changed. One can use any output as GPIO or PWM.
> 
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---
> 
> Changes in v3:
> * Added two error messages in ->request().
> * Protected the PWM count agains races with an additional mutex.
> * Dropped an useless check on the period value in ->apply().
> * Forced the .period to be constant.
> * Checked state->polarity when needed.
> * Used DIV_ROUND_DOWN_ULL for computing the duty_cycle.
> * Implemented ->get_state().
> * Added a comment to explain that the GPIO functionality is not harmed
>   by the global intensity setting.
> 
> Changes in v2:
> * Removed the hardcoding of PWM_CHANNELS, changed the code to use the
>   number of GPIO lines which is programatically known.
> * Used per pwm_device chip data to store the GPIO descriptors instead
>   of having a static array of GPIO descriptors in the private PWM
>   structure. It also enhanced the readability.
> * Rename an offset variable: s/off/shift/.
> * The default PWM state is now static low instead of input.
> * Used the GPIO as regular consumer thanks to the stored GPIO
>   descriptors to "make it more idiomatic" (requested by Thierry).
> * Used gpiochip_request_own_desc() instead of
>   gpio_to_desc()/gpiod_request(). This prevented the build issue and
>   an additional dependency that would have requested a DEPENDS ON line
>   in Kconfig.
> * Enhanced the return line of max7313_pwm_probe().
> 
> 
>  drivers/gpio/gpio-pca953x.c | 332 +++++++++++++++++++++++++++++++++++-
>  1 file changed, 330 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpio/gpio-pca953x.c b/drivers/gpio/gpio-pca953x.c
> index de5d1383f28d..baf639bec18d 100644
> --- a/drivers/gpio/gpio-pca953x.c
> +++ b/drivers/gpio/gpio-pca953x.c
> @@ -12,18 +12,22 @@
>  #include <linux/bits.h>
>  #include <linux/gpio/driver.h>
>  #include <linux/gpio/consumer.h>
> +#include <linux/gpio/machine.h>
>  #include <linux/i2c.h>
>  #include <linux/init.h>
>  #include <linux/interrupt.h>
>  #include <linux/module.h>
>  #include <linux/of_platform.h>
>  #include <linux/platform_data/pca953x.h>
> +#include <linux/pwm.h>
>  #include <linux/regmap.h>
>  #include <linux/regulator/consumer.h>
>  #include <linux/slab.h>
>  
>  #include <asm/unaligned.h>
>  
> +#include "gpiolib.h"
> +
>  #define PCA953X_INPUT		0x00
>  #define PCA953X_OUTPUT		0x01
>  #define PCA953X_INVERT		0x02
> @@ -63,11 +67,18 @@
>  
>  #define PCA_INT			BIT(8)
>  #define PCA_PCAL		BIT(9)
> +#define MAX_PWM			BIT(10)
>  #define PCA_LATCH_INT		(PCA_PCAL | PCA_INT)
>  #define PCA953X_TYPE		BIT(12)
>  #define PCA957X_TYPE		BIT(13)
>  #define PCA_TYPE_MASK		GENMASK(15, 12)
>  
> +#define MAX7313_MASTER		0x0E
> +#define MAX7313_CONFIGURATION	0x0F
> +#define MAX7313_INTENSITY	0x10
> +
> +#define MAX7313_GLOB_INTENSITY	BIT(2)
> +
>  #define PCA_CHIP_TYPE(x)	((x) & PCA_TYPE_MASK)
>  
>  static const struct i2c_device_id pca953x_id[] = {
> @@ -93,7 +104,7 @@ static const struct i2c_device_id pca953x_id[] = {
>  
>  	{ "max7310", 8  | PCA953X_TYPE, },
>  	{ "max7312", 16 | PCA953X_TYPE | PCA_INT, },
> -	{ "max7313", 16 | PCA953X_TYPE | PCA_INT, },
> +	{ "max7313", 16 | PCA953X_TYPE | PCA_INT | MAX_PWM, },
>  	{ "max7315", 8  | PCA953X_TYPE | PCA_INT, },
>  	{ "max7318", 16 | PCA953X_TYPE | PCA_INT, },
>  	{ "pca6107", 8  | PCA953X_TYPE | PCA_INT, },
> @@ -118,6 +129,14 @@ MODULE_DEVICE_TABLE(acpi, pca953x_acpi_ids);
>  
>  #define NBANK(chip) DIV_ROUND_UP(chip->gpio_chip.ngpio, BANK_SZ)
>  
> +#define PWM_PER_REG 2
> +#define PWM_BITS_PER_REG (8 / PWM_PER_REG)
> +#define PWM_INTENSITY_MASK GENMASK(PWM_BITS_PER_REG - 1, 0)
> +
> +#define PWM_PERIOD_NS 31250
> +#define PWM_DC_STATES 16
> +#define PWM_OFFSET_NS (PWM_PERIOD_NS / PWM_DC_STATES)
> +
>  struct pca953x_reg_config {
>  	int direction;
>  	int output;
> @@ -139,6 +158,22 @@ static const struct pca953x_reg_config pca957x_regs = {
>  	.invert = PCA957X_INVRT,
>  };
>  
> +struct max7313_pwm_data {
> +	struct gpio_desc *desc;
> +	bool enabled;
> +	unsigned int duty_cycle;
> +};
> +
> +struct max7313_pwm {
> +	struct pwm_chip chip;
> +	/*
> +	 * Protect races when counting active PWMs for enabling or disabling
> +	 * the internal oscillator.
> +	 */
> +	struct mutex count_lock;
> +	unsigned int count;
> +};
> +
>  struct pca953x_chip {
>  	unsigned gpio_start;
>  	struct mutex i2c_lock;
> @@ -161,6 +196,8 @@ struct pca953x_chip {
>  	struct regulator *regulator;
>  
>  	const struct pca953x_reg_config *regs;
> +
> +	struct max7313_pwm pwm;
>  };
>  
>  static int pca953x_bank_shift(struct pca953x_chip *chip)
> @@ -241,8 +278,16 @@ static bool pca953x_check_register(struct pca953x_chip *chip, unsigned int reg,
>  static bool pca953x_readable_register(struct device *dev, unsigned int reg)
>  {
>  	struct pca953x_chip *chip = dev_get_drvdata(dev);
> +	unsigned int bank_sz = chip->driver_data & PCA_GPIO_MASK;
>  	u32 bank;
>  
> +	if (PCA_CHIP_TYPE(chip->driver_data) == PCA953X_TYPE &&
> +	    chip->driver_data & MAX_PWM) {
> +		if (reg >= MAX7313_MASTER &&
> +		    reg < (MAX7313_INTENSITY + bank_sz))
> +			return true;
> +	}
> +
>  	if (PCA_CHIP_TYPE(chip->driver_data) == PCA953X_TYPE) {
>  		bank = PCA953x_BANK_INPUT | PCA953x_BANK_OUTPUT |
>  		       PCA953x_BANK_POLARITY | PCA953x_BANK_CONFIG;
> @@ -264,8 +309,16 @@ static bool pca953x_readable_register(struct device *dev, unsigned int reg)
>  static bool pca953x_writeable_register(struct device *dev, unsigned int reg)
>  {
>  	struct pca953x_chip *chip = dev_get_drvdata(dev);
> +	unsigned int bank_sz = chip->driver_data & PCA_GPIO_MASK;
>  	u32 bank;
>  
> +	if (PCA_CHIP_TYPE(chip->driver_data) == PCA953X_TYPE &&
> +	    chip->driver_data & MAX_PWM) {
> +		if (reg >= MAX7313_MASTER &&
> +		    reg < (MAX7313_INTENSITY + bank_sz))
> +			return true;
> +	}
> +
>  	if (PCA_CHIP_TYPE(chip->driver_data) == PCA953X_TYPE) {
>  		bank = PCA953x_BANK_OUTPUT | PCA953x_BANK_POLARITY |
>  			PCA953x_BANK_CONFIG;
> @@ -886,6 +939,278 @@ static int device_pca957x_init(struct pca953x_chip *chip, u32 invert)
>  	return ret;
>  }
>  
> +/* PWM specific methods */
> +
> +static struct max7313_pwm *to_max7313_pwm(struct pwm_chip *chip)
> +{
> +	return container_of(chip, struct max7313_pwm, chip);
> +}
> +
> +static struct pca953x_chip *to_pca953x(struct max7313_pwm *chip)
> +{
> +	return container_of(chip, struct pca953x_chip, pwm);
> +}
> +
> +static u8 max7313_pwm_get_intensity(struct pca953x_chip *chip,
> +				    unsigned int idx)
> +{
> +	unsigned int reg, shift, val;
> +	u8 duty_cycle;
> +
> +	reg = MAX7313_INTENSITY + (idx / PWM_PER_REG);
> +	shift = (idx % PWM_PER_REG) ? PWM_BITS_PER_REG : 0;
> +
> +	mutex_lock(&chip->i2c_lock);
> +	regmap_read(chip->regmap, reg, &val);
> +	mutex_unlock(&chip->i2c_lock);
> +
> +	if (shift)
> +		val >>= shift;
> +
> +	val &= PWM_INTENSITY_MASK;
> +
> +	/*
> +	 * Register values in the [0;15] range mean a value in the [1;16] range.
> +	 * A register value of 16 means the logic has been inverted to produce a
> +	 * static low output.
> +	 */
> +	if (val == PWM_INTENSITY_MASK)
> +		duty_cycle = 0;
> +	else
> +		duty_cycle = val + 1;
> +
> +	return duty_cycle;
> +}
> +
> +static int max7313_pwm_set_intensity(struct pca953x_chip *chip,
> +				     unsigned int idx, u8 duty_cycle)
> +{
> +	/* Duty-cycle is in the range [1;16] while registers expect [0;15] */
> +	u8 intensity = (duty_cycle - 1) & PWM_INTENSITY_MASK;
> +	unsigned int reg, shift;
> +	u8 val, mask;
> +	int ret;
> +
> +	reg = MAX7313_INTENSITY + (idx / PWM_PER_REG);
> +	shift = (idx % PWM_PER_REG) ? PWM_BITS_PER_REG : 0;
> +
> +	mask = PWM_INTENSITY_MASK << shift;
> +	val = intensity << shift;
> +
> +	mutex_lock(&chip->i2c_lock);
> +	ret = regmap_write_bits(chip->regmap, reg, mask, val);
> +	mutex_unlock(&chip->i2c_lock);
> +
> +	return ret;
> +}
> +
> +/*
> + * For a given PWM channel, when the blink phase 0 bit is set, the intensity
> + * range is only [1/16;16/16]. With this range, a static low output is
> + * physically not possible. When the blink phase 0 bit is cleared, the intensity
> + * range is [15/16;0/16] which then allows a static low output but not a static
> + * high output.
> + *
> + * In this driver we choose to set the blink phase 0 bit by default, hence we
> + * can slide from a low output to a fully high output without glitch. However,
> + * the only way to get a static low output is by clearing the blink phase 0 bit,
> + * and by changing the intensity value to its maximum (as, at this moment,
> + * intensity is reversed). There is no way to atomically flip the register *and*
> + * change the PWM value at the same time so this will produce a small glitch.
> + */
> +static int max7313_pwm_set_state(struct pca953x_chip *chip,
> +				 struct pwm_device *pwm_device,
> +				 unsigned int duty_cycle)

I'd like to see a better name than "duty_cycle" here. In the comment
above you call it "intensity" which is more clear IMHO.

> +{
> +	struct max7313_pwm_data *data = pwm_get_chip_data(pwm_device);
> +	struct gpio_desc *desc = data->desc;
> +	int ret;
> +
> +	/* A null duty_cycle will invert the phase */
> +	ret = gpiod_direction_output(desc, duty_cycle);
> +	if (ret)
> +		return ret;
> +
> +	/* Maximize the low time in case of static low state */
> +	if (!duty_cycle)
> +		duty_cycle = PWM_DC_STATES;
> +
> +	return max7313_pwm_set_intensity(chip, pwm_device->hwpwm, duty_cycle);

Thierry already commented that the call to gpiod_direction_output is a
bit strange. What about:

	if (intensity) {
		gpiod_direction_output(desc, 1);
		max7313_pwm_set_intensity(chip, pwm_device->hwpwm, intensity - 1)
	} else {
		gpiod_direction_output(desc, 0);
		max7313_pwm_set_intensity(chip, pwm_device->hwpwm, 15);
	}

Also when switching from 0% to 50% you could prevent a glitch by
sticking to an unset blink phase 0 bit.

> +}
> +
> +static int max7313_pwm_request(struct pwm_chip *pwm_chip,
> +			       struct pwm_device *pwm_device)
> +{
> +	struct max7313_pwm *pwm = to_max7313_pwm(pwm_chip);
> +	struct pca953x_chip *chip = to_pca953x(pwm);
> +	struct max7313_pwm_data *data;
> +	struct gpio_desc *desc;
> +	int ret;
> +
> +	desc = gpiochip_request_own_desc(&chip->gpio_chip, pwm_device->hwpwm,
> +					 "max7313-pwm", GPIO_ACTIVE_HIGH,
> +					 GPIOD_OUT_LOW);
> +	if (IS_ERR(desc)) {
> +		dev_err(&chip->client->dev,
> +			"pin already in use (probably as GPIO)\n");
> +		return PTR_ERR(desc);
> +	}
> +
> +	data = devm_kzalloc(&chip->client->dev, sizeof(*data), GFP_KERNEL);
> +	if (!data)
> +		return -ENOMEM;
> +
> +	data->desc = desc;
> +	pwm_set_chip_data(pwm_device, data);
> +
> +	ret = max7313_pwm_set_state(chip, pwm_device, 0);
> +	if (ret) {
> +		dev_err(&chip->client->dev, "cannot set PWM default state\n");

Would be good to include the error code here. I suggest to use

	dev_err(&chip->client->dev, "cannot set PWM default state: %pe\n",
		ERR_PTR(ret));

Ditto for the error message above when gpiochip_request_own_desc fails.

If I'm not mistaken this already initializes the PWM, however .request
isn't supposed to do that. The output state should be unmodified to
allow to take over an already initialized PWM from the boot loader.

> +		goto free_gpiod;
> +	}
> +
> +	/*
> +	 * Set master intensity to the maximum level to let individual outputs
> +	 * the greatest flexibility range. Also enables the internal oscillator.
> +	 */
> +	mutex_lock(&pwm->count_lock);
> +	if (!pwm->count) {
> +		mutex_lock(&chip->i2c_lock);
> +		ret = regmap_write_bits(chip->regmap, MAX7313_MASTER,
> +					PWM_INTENSITY_MASK << PWM_BITS_PER_REG,
> +					PWM_INTENSITY_MASK << PWM_BITS_PER_REG);
> +		mutex_unlock(&chip->i2c_lock);
> +	}
> +
> +	if (!ret)
> +		pwm->count++;
> +
> +	mutex_unlock(&pwm->count_lock);
> +
> +	if (ret)
> +		goto free_gpiod;
> +
> +	return 0;
> +
> +free_gpiod:
> +	gpiochip_free_own_desc(data->desc);
> +
> +	return ret;
> +}
> +
> +static void max7313_pwm_free(struct pwm_chip *pwm_chip,
> +			     struct pwm_device *pwm_device)
> +{
> +	struct max7313_pwm_data *data = pwm_get_chip_data(pwm_device);
> +	struct max7313_pwm *pwm = to_max7313_pwm(pwm_chip);
> +	struct pca953x_chip *chip = to_pca953x(pwm);
> +
> +	max7313_pwm_set_state(chip, pwm_device, 0);

As .request above also .free is not supposed to modify the output.

> +	mutex_lock(&pwm->count_lock);
> +
> +	pwm->count--;
> +
> +	/* Disable the internal oscillator if no channel is in use */
> +	if (!pwm->count) {
> +		mutex_lock(&chip->i2c_lock);
> +		regmap_write_bits(chip->regmap, MAX7313_MASTER,
> +				  PWM_INTENSITY_MASK << PWM_BITS_PER_REG, 0);
> +		mutex_unlock(&chip->i2c_lock);
> +	}

What happens to the output pin when the oscillator gets disabled?

> +	mutex_unlock(&pwm->count_lock);
> +
> +	gpiochip_free_own_desc(data->desc);
> +}
> +
> +static int max7313_pwm_apply(struct pwm_chip *pwm_chip,
> +			     struct pwm_device *pwm_device,
> +			     const struct pwm_state *state)
> +{
> +	struct max7313_pwm_data *data = pwm_get_chip_data(pwm_device);
> +	struct max7313_pwm *pwm = to_max7313_pwm(pwm_chip);
> +	struct pca953x_chip *chip = to_pca953x(pwm);
> +	unsigned int duty_cycle;
> +
> +	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.)

> +	data->enabled = state->enabled;
> +	data->duty_cycle = state->duty_cycle;

Storing these is only to let .get_state yield the last set values,
right?

> +	if (!state->enabled || !state->duty_cycle)
> +		duty_cycle = 0;
> +	else
> +		/* Convert the duty-cycle to be in the [1;16] range */
> +		duty_cycle = DIV_ROUND_DOWN_ULL(state->duty_cycle,
> +						PWM_OFFSET_NS);
> +
> +	/* The hardware is supposedly glitch-free */
> +	return max7313_pwm_set_state(chip, pwm_device, duty_cycle);
> +}
> +
> +static void max7313_pwm_get_state(struct pwm_chip *pwm_chip,
> +				  struct pwm_device *pwm_device,
> +				  struct pwm_state *state)
> +{
> +	struct max7313_pwm_data *data = pwm_get_chip_data(pwm_device);
> +	struct max7313_pwm *pwm = to_max7313_pwm(pwm_chip);
> +	struct pca953x_chip *chip = to_pca953x(pwm);
> +	u8 duty_cycle;
> +
> +	state->period = PWM_PERIOD_NS;
> +	state->polarity = PWM_POLARITY_NORMAL;
> +
> +	if (!data)
> +		return;
> +
> +	state->enabled = data->enabled;

data->enabled is not initialized from hardware in .probe, so currently
.get doesn't provide anything useful for the initial call. :-|

> +	if (!state->enabled) {
> +		state->duty_cycle = data->duty_cycle;
> +	} else {
> +		duty_cycle = max7313_pwm_get_intensity(chip, pwm_device->hwpwm);
> +		state->duty_cycle = duty_cycle * PWM_OFFSET_NS;

if the hardware is configured with blink phase 0 bit unset the returned
value is wrong, isn't it?

> +	}
> +};

Best regards
Uwe
Miquel Raynal Nov. 26, 2019, 8:51 a.m. UTC | #4
Hi Uwe,


> > +/*
> > + * For a given PWM channel, when the blink phase 0 bit is set, the intensity
> > + * range is only [1/16;16/16]. With this range, a static low output is
> > + * physically not possible. When the blink phase 0 bit is cleared, the intensity
> > + * range is [15/16;0/16] which then allows a static low output but not a static
> > + * high output.
> > + *
> > + * In this driver we choose to set the blink phase 0 bit by default, hence we
> > + * can slide from a low output to a fully high output without glitch. However,
> > + * the only way to get a static low output is by clearing the blink phase 0 bit,
> > + * and by changing the intensity value to its maximum (as, at this moment,
> > + * intensity is reversed). There is no way to atomically flip the register *and*
> > + * change the PWM value at the same time so this will produce a small glitch.
> > + */
> > +static int max7313_pwm_set_state(struct pca953x_chip *chip,
> > +				 struct pwm_device *pwm_device,
> > +				 unsigned int duty_cycle)  
> 
> I'd like to see a better name than "duty_cycle" here. In the comment
> above you call it "intensity" which is more clear IMHO.
> 
> > +{
> > +	struct max7313_pwm_data *data = pwm_get_chip_data(pwm_device);
> > +	struct gpio_desc *desc = data->desc;
> > +	int ret;
> > +
> > +	/* A null duty_cycle will invert the phase */
> > +	ret = gpiod_direction_output(desc, duty_cycle);
> > +	if (ret)
> > +		return ret;
> > +
> > +	/* Maximize the low time in case of static low state */
> > +	if (!duty_cycle)
> > +		duty_cycle = PWM_DC_STATES;
> > +
> > +	return max7313_pwm_set_intensity(chip, pwm_device->hwpwm, duty_cycle);  
> 
> Thierry already commented that the call to gpiod_direction_output is a
> bit strange. What about:
> 
> 	if (intensity) {
> 		gpiod_direction_output(desc, 1);
> 		max7313_pwm_set_intensity(chip, pwm_device->hwpwm, intensity - 1)
> 	} else {
> 		gpiod_direction_output(desc, 0);
> 		max7313_pwm_set_intensity(chip, pwm_device->hwpwm, 15);
> 	}

I'm fine with that.

> 
> Also when switching from 0% to 50% you could prevent a glitch by
> sticking to an unset blink phase 0 bit.

You mean, when turning off the PWM, I should first change the intensity
to 50%, then blink the phase, then change the intensity to 100%?
(reversed logic when intensity > 0).

If the intensity was already low, this will produce a glitch in any
case, right? Same if the user changes the intensity from 0 to 15%, that
will bright at 50%.

Do you think it is worth adding such logic?

> 
> > +}
> > +
> > +static int max7313_pwm_request(struct pwm_chip *pwm_chip,
> > +			       struct pwm_device *pwm_device)
> > +{
> > +	struct max7313_pwm *pwm = to_max7313_pwm(pwm_chip);
> > +	struct pca953x_chip *chip = to_pca953x(pwm);
> > +	struct max7313_pwm_data *data;
> > +	struct gpio_desc *desc;
> > +	int ret;
> > +
> > +	desc = gpiochip_request_own_desc(&chip->gpio_chip, pwm_device->hwpwm,
> > +					 "max7313-pwm", GPIO_ACTIVE_HIGH,
> > +					 GPIOD_OUT_LOW);
> > +	if (IS_ERR(desc)) {
> > +		dev_err(&chip->client->dev,
> > +			"pin already in use (probably as GPIO)\n");
> > +		return PTR_ERR(desc);
> > +	}
> > +
> > +	data = devm_kzalloc(&chip->client->dev, sizeof(*data), GFP_KERNEL);
> > +	if (!data)
> > +		return -ENOMEM;
> > +
> > +	data->desc = desc;
> > +	pwm_set_chip_data(pwm_device, data);
> > +
> > +	ret = max7313_pwm_set_state(chip, pwm_device, 0);
> > +	if (ret) {
> > +		dev_err(&chip->client->dev, "cannot set PWM default state\n");  
> 
> Would be good to include the error code here. I suggest to use
> 
> 	dev_err(&chip->client->dev, "cannot set PWM default state: %pe\n",
> 		ERR_PTR(ret));
> 
> Ditto for the error message above when gpiochip_request_own_desc fails.
> 
> If I'm not mistaken this already initializes the PWM, however .request
> isn't supposed to do that. The output state should be unmodified to
> allow to take over an already initialized PWM from the boot loader.

Ok, I'll get rid of that call.

> 
> > +		goto free_gpiod;
> > +	}
> > +
> > +	/*
> > +	 * Set master intensity to the maximum level to let individual outputs
> > +	 * the greatest flexibility range. Also enables the internal oscillator.
> > +	 */
> > +	mutex_lock(&pwm->count_lock);
> > +	if (!pwm->count) {
> > +		mutex_lock(&chip->i2c_lock);
> > +		ret = regmap_write_bits(chip->regmap, MAX7313_MASTER,
> > +					PWM_INTENSITY_MASK << PWM_BITS_PER_REG,
> > +					PWM_INTENSITY_MASK << PWM_BITS_PER_REG);
> > +		mutex_unlock(&chip->i2c_lock);
> > +	}
> > +
> > +	if (!ret)
> > +		pwm->count++;
> > +
> > +	mutex_unlock(&pwm->count_lock);
> > +
> > +	if (ret)
> > +		goto free_gpiod;
> > +
> > +	return 0;
> > +
> > +free_gpiod:
> > +	gpiochip_free_own_desc(data->desc);
> > +
> > +	return ret;
> > +}
> > +
> > +static void max7313_pwm_free(struct pwm_chip *pwm_chip,
> > +			     struct pwm_device *pwm_device)
> > +{
> > +	struct max7313_pwm_data *data = pwm_get_chip_data(pwm_device);
> > +	struct max7313_pwm *pwm = to_max7313_pwm(pwm_chip);
> > +	struct pca953x_chip *chip = to_pca953x(pwm);
> > +
> > +	max7313_pwm_set_state(chip, pwm_device, 0);  
> 
> As .request above also .free is not supposed to modify the output.
> 
> > +	mutex_lock(&pwm->count_lock);
> > +
> > +	pwm->count--;
> > +
> > +	/* Disable the internal oscillator if no channel is in use */
> > +	if (!pwm->count) {
> > +		mutex_lock(&chip->i2c_lock);
> > +		regmap_write_bits(chip->regmap, MAX7313_MASTER,
> > +				  PWM_INTENSITY_MASK << PWM_BITS_PER_REG, 0);
> > +		mutex_unlock(&chip->i2c_lock);
> > +	}  
> 
> What happens to the output pin when the oscillator gets disabled?

If .free does not modify the output, then disabling the oscillator
will force a static state if that was not already the case. I suppose
that's not what you want. Why one would free a pin if it stills need to
blink?

If I am not allowed to disable the oscillator it means energy
consumption will stay high for no reason as long as one PWM has been
enabled, ever.

> 
> > +	mutex_unlock(&pwm->count_lock);
> > +
> > +	gpiochip_free_own_desc(data->desc);
> > +}
> > +
> > +static int max7313_pwm_apply(struct pwm_chip *pwm_chip,
> > +			     struct pwm_device *pwm_device,
> > +			     const struct pwm_state *state)
> > +{
> > +	struct max7313_pwm_data *data = pwm_get_chip_data(pwm_device);
> > +	struct max7313_pwm *pwm = to_max7313_pwm(pwm_chip);
> > +	struct pca953x_chip *chip = to_pca953x(pwm);
> > +	unsigned int duty_cycle;
> > +
> > +	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?

> 
> > +	data->enabled = state->enabled;
> > +	data->duty_cycle = state->duty_cycle;  
> 
> Storing these is only to let .get_state yield the last set values,
> right?

I can't guess the duty_cycle/enabled state just by reading the
hardware. For instance, I cannot represent a "40% duty with PWM
disabled". Reading the hardware will not be able to know if the PWM
is enabled or not and the duty_cycle will be read as 0.

> 
> > +	if (!state->enabled || !state->duty_cycle)
> > +		duty_cycle = 0;
> > +	else
> > +		/* Convert the duty-cycle to be in the [1;16] range */
> > +		duty_cycle = DIV_ROUND_DOWN_ULL(state->duty_cycle,
> > +						PWM_OFFSET_NS);
> > +
> > +	/* The hardware is supposedly glitch-free */
> > +	return max7313_pwm_set_state(chip, pwm_device, duty_cycle);
> > +}
> > +
> > +static void max7313_pwm_get_state(struct pwm_chip *pwm_chip,
> > +				  struct pwm_device *pwm_device,
> > +				  struct pwm_state *state)
> > +{
> > +	struct max7313_pwm_data *data = pwm_get_chip_data(pwm_device);
> > +	struct max7313_pwm *pwm = to_max7313_pwm(pwm_chip);
> > +	struct pca953x_chip *chip = to_pca953x(pwm);
> > +	u8 duty_cycle;
> > +
> > +	state->period = PWM_PERIOD_NS;
> > +	state->polarity = PWM_POLARITY_NORMAL;
> > +
> > +	if (!data)
> > +		return;
> > +
> > +	state->enabled = data->enabled;  
> 
> data->enabled is not initialized from hardware in .probe, so currently
> .get doesn't provide anything useful for the initial call. :-|

'enabled' only has a software meaning here, so it will return false at
the first call as the structure is initially zeroed. Then it will
provide the last state of the boolean. This IP has no "enable/disable"
feature so I don't know how to handle this in a better way.

> > +	if (!state->enabled) {
> > +		state->duty_cycle = data->duty_cycle;
> > +	} else {
> > +		duty_cycle = max7313_pwm_get_intensity(chip, pwm_device->hwpwm);
> > +		state->duty_cycle = duty_cycle * PWM_OFFSET_NS;  
> 
> if the hardware is configured with blink phase 0 bit unset the returned
> value is wrong, isn't it?

True, I'll correct.

> 
> > +	}
> > +};  
> 
> Best regards
> Uwe
> 

Thanks,
Miquèl
Uwe Kleine-König Nov. 26, 2019, 10:49 a.m. UTC | #5
Hallo Miquel,

On Tue, Nov 26, 2019 at 09:51:56AM +0100, Miquel Raynal wrote:
> > Also when switching from 0% to 50% you could prevent a glitch by
> > sticking to an unset blink phase 0 bit.
> 
> You mean, when turning off the PWM, I should first change the intensity
> to 50%, then blink the phase, then change the intensity to 100%?
> (reversed logic when intensity > 0).

No, that's not what I meant. Your hardware seems to have two different
"modes". One where you can set the intensity between 0 and 15/16, and
another where the intensity is between 1/16 and 16/16, right? Switching
between these two results in a glitch and you use the first mode only
for intensity 0 and the second for the rest. If now you have to go from
0 to 8/16 your driver does a mode switch while this isn't necessary.

I also wonder if a glitch can at least be made less likely, even when
going from 0% to 100%. You claimed that changing intensity was glitch
free (i.e. the currently running period is completed before the changed
setting has an effect). Does this hold also for changing the blink
phase?

> > > +	/* Disable the internal oscillator if no channel is in use */
> > > +	if (!pwm->count) {
> > > +		mutex_lock(&chip->i2c_lock);
> > > +		regmap_write_bits(chip->regmap, MAX7313_MASTER,
> > > +				  PWM_INTENSITY_MASK << PWM_BITS_PER_REG, 0);
> > > +		mutex_unlock(&chip->i2c_lock);
> > > +	}  
> > 
> > What happens to the output pin when the oscillator gets disabled?
> 
> If .free does not modify the output, then disabling the oscillator
> will force a static state if that was not already the case. I suppose
> that's not what you want. Why one would free a pin if it stills need to
> blink?

It's the consumer who is supposed to stop the PWM before freeing it.
Then better do the oscillator stop when pwm_apply_state(pwm, {...
.enabled = false }) is called (and the other PWMs are off, too).

> If I am not allowed to disable the oscillator it means energy
> consumption will stay high for no reason as long as one PWM has been
> enabled, ever.
> 
> > 
> > > +	mutex_unlock(&pwm->count_lock);
> > > +
> > > +	gpiochip_free_own_desc(data->desc);
> > > +}
> > > +
> > > +static int max7313_pwm_apply(struct pwm_chip *pwm_chip,
> > > +			     struct pwm_device *pwm_device,
> > > +			     const struct pwm_state *state)
> > > +{
> > > +	struct max7313_pwm_data *data = pwm_get_chip_data(pwm_device);
> > > +	struct max7313_pwm *pwm = to_max7313_pwm(pwm_chip);
> > > +	struct pca953x_chip *chip = to_pca953x(pwm);
> > > +	unsigned int duty_cycle;
> > > +
> > > +	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.

(Of course for we'd need something like pwm_round_state() to effectively
find a good request without actually modifying the hardware state. That
is on my todo list.)

> > > +	data->enabled = state->enabled;
> > > +	data->duty_cycle = state->duty_cycle;  
> > 
> > Storing these is only to let .get_state yield the last set values,
> > right?
> 
> I can't guess the duty_cycle/enabled state just by reading the
> hardware. For instance, I cannot represent a "40% duty with PWM
> disabled". Reading the hardware will not be able to know if the PWM
> is enabled or not and the duty_cycle will be read as 0.

I interpret that as a "yes". IMHO it's a misconcept that a PWM driver
has to remember the duty cycle (and period) with enabled=false even
though this has no influence on the actual output in that state. I'd
like to get rid of .enabled in struct pwm_state completely, but Thierry
doesn't agree.

> > > +	if (!state->enabled || !state->duty_cycle)
> > > +		duty_cycle = 0;
> > > +	else
> > > +		/* Convert the duty-cycle to be in the [1;16] range */
> > > +		duty_cycle = DIV_ROUND_DOWN_ULL(state->duty_cycle,
> > > +						PWM_OFFSET_NS);
> > > +
> > > +	/* The hardware is supposedly glitch-free */
> > > +	return max7313_pwm_set_state(chip, pwm_device, duty_cycle);
> > > +}
> > > +
> > > +static void max7313_pwm_get_state(struct pwm_chip *pwm_chip,
> > > +				  struct pwm_device *pwm_device,
> > > +				  struct pwm_state *state)
> > > +{
> > > +	struct max7313_pwm_data *data = pwm_get_chip_data(pwm_device);
> > > +	struct max7313_pwm *pwm = to_max7313_pwm(pwm_chip);
> > > +	struct pca953x_chip *chip = to_pca953x(pwm);
> > > +	u8 duty_cycle;
> > > +
> > > +	state->period = PWM_PERIOD_NS;
> > > +	state->polarity = PWM_POLARITY_NORMAL;
> > > +
> > > +	if (!data)
> > > +		return;
> > > +
> > > +	state->enabled = data->enabled;  
> > 
> > data->enabled is not initialized from hardware in .probe, so currently
> > .get doesn't provide anything useful for the initial call. :-|
> 
> 'enabled' only has a software meaning here, so it will return false at
> the first call as the structure is initially zeroed. Then it will
> provide the last state of the boolean. This IP has no "enable/disable"
> feature so I don't know how to handle this in a better way.

When we'd get rid of struct pwm_state::enabled this would be clearer,
right? As this driver will be another instance that will hopefully help
me convincing Thierry in the end, can you please add a comment somewhere
at the top like this?:

/*
 * Limitations:
 * - Period fixed to 31.25 ms
 * - Only supports normal polarity
 * - Doesn't support a disabled state
 * - Some glitches cannot be prevented
 */

(The idea is that a command like

	for f in drivers/pwm/pwm-*.c ; do echo $f; sed -rn '/^ \* Limitation/,/^ \*\/?$/p' $f ; done

gives some nice overview about common limitations.)

But also today you should check the hardware state and if duty_cycle is
greater than 0 don't report the PWM as disabled.

Best regards
Uwe
Miquel Raynal Nov. 26, 2019, 11:15 a.m. UTC | #6
Hi Uwe,

Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote on Tue, 26 Nov
2019 11:49:20 +0100:

> Hallo Miquel,
> 
> On Tue, Nov 26, 2019 at 09:51:56AM +0100, Miquel Raynal wrote:
> > > Also when switching from 0% to 50% you could prevent a glitch by
> > > sticking to an unset blink phase 0 bit.  
> > 
> > You mean, when turning off the PWM, I should first change the intensity
> > to 50%, then blink the phase, then change the intensity to 100%?
> > (reversed logic when intensity > 0).  
> 
> No, that's not what I meant. Your hardware seems to have two different
> "modes". One where you can set the intensity between 0 and 15/16, and
> another where the intensity is between 1/16 and 16/16, right? Switching

Right!

> between these two results in a glitch and you use the first mode only
> for intensity 0 and the second for the rest.

Indeed.

> If now you have to go from
> 0 to 8/16 your driver does a mode switch while this isn't necessary.

This is right but it complicates a bit the logic as intensity changes
become stateful. If this is what you want, I can do it.

> 
> I also wonder if a glitch can at least be made less likely, even when
> going from 0% to 100%. You claimed that changing intensity was glitch
> free (i.e. the currently running period is completed before the changed
> setting has an effect). Does this hold also for changing the blink
> phase?

I honestly don't know, the datasheet does not tell anything about it.
If I implement the above logic, glitches will be made less likely.

I could also add more logic to switch the blink state at the 50%
whenever crossing this value but with the above logic added I think it
becomes unreadable and error prone, the risks are not balanced with the
benefits. Of course anyone can enhance the driver in the future.

> 
> > > > +	/* Disable the internal oscillator if no channel is in use */
> > > > +	if (!pwm->count) {
> > > > +		mutex_lock(&chip->i2c_lock);
> > > > +		regmap_write_bits(chip->regmap, MAX7313_MASTER,
> > > > +				  PWM_INTENSITY_MASK << PWM_BITS_PER_REG, 0);
> > > > +		mutex_unlock(&chip->i2c_lock);
> > > > +	}    
> > > 
> > > What happens to the output pin when the oscillator gets disabled?  
> > 
> > If .free does not modify the output, then disabling the oscillator
> > will force a static state if that was not already the case. I suppose
> > that's not what you want. Why one would free a pin if it stills need to
> > blink?  
> 
> It's the consumer who is supposed to stop the PWM before freeing it.
> Then better do the oscillator stop when pwm_apply_state(pwm, {...
> .enabled = false }) is called (and the other PWMs are off, too).

Ok, I can disable the oscillator in pwm_apply_state() if all
duty_cycles are at 0.

> 
> > If I am not allowed to disable the oscillator it means energy
> > consumption will stay high for no reason as long as one PWM has been
> > enabled, ever.
> >   
> > >   
> > > > +	mutex_unlock(&pwm->count_lock);
> > > > +
> > > > +	gpiochip_free_own_desc(data->desc);
> > > > +}
> > > > +
> > > > +static int max7313_pwm_apply(struct pwm_chip *pwm_chip,
> > > > +			     struct pwm_device *pwm_device,
> > > > +			     const struct pwm_state *state)
> > > > +{
> > > > +	struct max7313_pwm_data *data = pwm_get_chip_data(pwm_device);
> > > > +	struct max7313_pwm *pwm = to_max7313_pwm(pwm_chip);
> > > > +	struct pca953x_chip *chip = to_pca953x(pwm);
> > > > +	unsigned int duty_cycle;
> > > > +
> > > > +	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.

I am not sure to follow you, here are two possible understandings:

1/ state->period > PWM_PERIOD_NS should not be refused, but in the end
get_state should always return PWM_PERIOD_NS.

2/ I should always do the ratio between state->period and
state->duty_cycle as long as state->period >= PWM_PERIOD_NS (In this
case I still don't understand why I should refuse state->period <
PWM_PERIOD_NS).

> (Of course for we'd need something like pwm_round_state() to effectively
> find a good request without actually modifying the hardware state. That
> is on my todo list.)
> 
> > > > +	data->enabled = state->enabled;
> > > > +	data->duty_cycle = state->duty_cycle;    
> > > 
> > > Storing these is only to let .get_state yield the last set values,
> > > right?  
> > 
> > I can't guess the duty_cycle/enabled state just by reading the
> > hardware. For instance, I cannot represent a "40% duty with PWM
> > disabled". Reading the hardware will not be able to know if the PWM
> > is enabled or not and the duty_cycle will be read as 0.  
> 
> I interpret that as a "yes". IMHO it's a misconcept that a PWM driver
> has to remember the duty cycle (and period) with enabled=false even
> though this has no influence on the actual output in that state. I'd
> like to get rid of .enabled in struct pwm_state completely, but Thierry
> doesn't agree.

I have no choice. Actually I don't understand why the core do not
provide the 'last' duty cycle when enabled == false. It provides 0. So
if I don't use the above trick here is what happens:

echo 1 > enabled
echo 50 > duty_cycle
echo 0 > enabled
echo 1 > enabled
* duty_cycle is 0 while I expect it to be 50 *

> > > > +	if (!state->enabled || !state->duty_cycle)
> > > > +		duty_cycle = 0;
> > > > +	else
> > > > +		/* Convert the duty-cycle to be in the [1;16] range */
> > > > +		duty_cycle = DIV_ROUND_DOWN_ULL(state->duty_cycle,
> > > > +						PWM_OFFSET_NS);
> > > > +
> > > > +	/* The hardware is supposedly glitch-free */
> > > > +	return max7313_pwm_set_state(chip, pwm_device, duty_cycle);
> > > > +}
> > > > +
> > > > +static void max7313_pwm_get_state(struct pwm_chip *pwm_chip,
> > > > +				  struct pwm_device *pwm_device,
> > > > +				  struct pwm_state *state)
> > > > +{
> > > > +	struct max7313_pwm_data *data = pwm_get_chip_data(pwm_device);
> > > > +	struct max7313_pwm *pwm = to_max7313_pwm(pwm_chip);
> > > > +	struct pca953x_chip *chip = to_pca953x(pwm);
> > > > +	u8 duty_cycle;
> > > > +
> > > > +	state->period = PWM_PERIOD_NS;
> > > > +	state->polarity = PWM_POLARITY_NORMAL;
> > > > +
> > > > +	if (!data)
> > > > +		return;
> > > > +
> > > > +	state->enabled = data->enabled;    
> > > 
> > > data->enabled is not initialized from hardware in .probe, so currently
> > > .get doesn't provide anything useful for the initial call. :-|  
> > 
> > 'enabled' only has a software meaning here, so it will return false at
> > the first call as the structure is initially zeroed. Then it will
> > provide the last state of the boolean. This IP has no "enable/disable"
> > feature so I don't know how to handle this in a better way.  
> 
> When we'd get rid of struct pwm_state::enabled this would be clearer,
> right? As this driver will be another instance that will hopefully help
> me convincing Thierry in the end, can you please add a comment somewhere
> at the top like this?:
> 
> /*
>  * Limitations:
>  * - Period fixed to 31.25 ms
>  * - Only supports normal polarity
>  * - Doesn't support a disabled state
>  * - Some glitches cannot be prevented
>  */
> 
> (The idea is that a command like
> 
> 	for f in drivers/pwm/pwm-*.c ; do echo $f; sed -rn '/^ \* Limitation/,/^ \*\/?$/p' $f ; done

Sure!

> 
> gives some nice overview about common limitations.)
> 
> But also today you should check the hardware state and if duty_cycle is
> greater than 0 don't report the PWM as disabled.

Ok.

> 
> Best regards
> Uwe
> 

Thanks,
Miquèl
Uwe Kleine-König Nov. 26, 2019, 1:12 p.m. UTC | #7
Hello Miquèl,

On Tue, Nov 26, 2019 at 12:15:14PM +0100, Miquel Raynal wrote:
> Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote on Tue, 26 Nov
> 2019 11:49:20 +0100:
> 
> > Hallo Miquel,
> > 
> > On Tue, Nov 26, 2019 at 09:51:56AM +0100, Miquel Raynal wrote:
> > > > Also when switching from 0% to 50% you could prevent a glitch by
> > > > sticking to an unset blink phase 0 bit.  
> > > 
> > > You mean, when turning off the PWM, I should first change the intensity
> > > to 50%, then blink the phase, then change the intensity to 100%?
> > > (reversed logic when intensity > 0).  
> > 
> > No, that's not what I meant. Your hardware seems to have two different
> > "modes". One where you can set the intensity between 0 and 15/16, and
> > another where the intensity is between 1/16 and 16/16, right? Switching
> 
> Right!
> 
> > between these two results in a glitch and you use the first mode only
> > for intensity 0 and the second for the rest.
> 
> Indeed.
> 
> > If now you have to go from
> > 0 to 8/16 your driver does a mode switch while this isn't necessary.
> 
> This is right but it complicates a bit the logic as intensity changes
> become stateful. If this is what you want, I can do it.
> 
> > 
> > I also wonder if a glitch can at least be made less likely, even when
> > going from 0% to 100%. You claimed that changing intensity was glitch
> > free (i.e. the currently running period is completed before the changed
> > setting has an effect). Does this hold also for changing the blink
> > phase?
> 
> I honestly don't know, the datasheet does not tell anything about it.

So you don't have the equipment to test that?

> If I implement the above logic, glitches will be made less likely.

Sounds good.

> I could also add more logic to switch the blink state at the 50%
> whenever crossing this value but with the above logic added I think it
> becomes unreadable and error prone, the risks are not balanced with the
> benefits. Of course anyone can enhance the driver in the future.

I would just implement "lazy" switching. So switch if (and only if)
necessary.
 
> > > > > +	mutex_unlock(&pwm->count_lock);
> > > > > +
> > > > > +	gpiochip_free_own_desc(data->desc);
> > > > > +}
> > > > > +
> > > > > +static int max7313_pwm_apply(struct pwm_chip *pwm_chip,
> > > > > +			     struct pwm_device *pwm_device,
> > > > > +			     const struct pwm_state *state)
> > > > > +{
> > > > > +	struct max7313_pwm_data *data = pwm_get_chip_data(pwm_device);
> > > > > +	struct max7313_pwm *pwm = to_max7313_pwm(pwm_chip);
> > > > > +	struct pca953x_chip *chip = to_pca953x(pwm);
> > > > > +	unsigned int duty_cycle;
> > > > > +
> > > > > +	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.
> 
> I am not sure to follow you, here are two possible understandings:
> 
> 1/ state->period > PWM_PERIOD_NS should not be refused, but in the end
> get_state should always return PWM_PERIOD_NS.
> 
> 2/ I should always do the ratio between state->period and
> state->duty_cycle as long as state->period >= PWM_PERIOD_NS (In this
> case I still don't understand why I should refuse state->period <
> PWM_PERIOD_NS).

The first is the one I want. There are some strange corner cases, but
the policy is easy and also for other policies there are corner cases.
So in general you should do (when .enabled = true):

 - use the requested polarity
 - choose the biggest period not bigger than the requested one
 - with the above period choose the biggest duty_cycle not bigger than
   the requested one.

> > > > > +	data->enabled = state->enabled;
> > > > > +	data->duty_cycle = state->duty_cycle;    
> > > > 
> > > > Storing these is only to let .get_state yield the last set values,
> > > > right?  
> > > 
> > > I can't guess the duty_cycle/enabled state just by reading the
> > > hardware. For instance, I cannot represent a "40% duty with PWM
> > > disabled". Reading the hardware will not be able to know if the PWM
> > > is enabled or not and the duty_cycle will be read as 0.  
> > 
> > I interpret that as a "yes". IMHO it's a misconcept that a PWM driver
> > has to remember the duty cycle (and period) with enabled=false even
> > though this has no influence on the actual output in that state. I'd
> > like to get rid of .enabled in struct pwm_state completely, but Thierry
> > doesn't agree.
> 
> I have no choice. Actually I don't understand why the core do not
> provide the 'last' duty cycle when enabled == false. It provides 0. So
> if I don't use the above trick here is what happens:
> 
> echo 1 > enabled
> echo 50 > duty_cycle
> echo 0 > enabled
> echo 1 > enabled
> * duty_cycle is 0 while I expect it to be 50 *

IMHO this should be fixed in the framework. As I assume this is a bigger
discussion I suggest to keep the code as is and when at some point in
time we have the framework fixed, we simplify all drivers that can
benefit.

Best regards
Uwe

Patch
diff mbox series

diff --git a/drivers/gpio/gpio-pca953x.c b/drivers/gpio/gpio-pca953x.c
index de5d1383f28d..baf639bec18d 100644
--- a/drivers/gpio/gpio-pca953x.c
+++ b/drivers/gpio/gpio-pca953x.c
@@ -12,18 +12,22 @@ 
 #include <linux/bits.h>
 #include <linux/gpio/driver.h>
 #include <linux/gpio/consumer.h>
+#include <linux/gpio/machine.h>
 #include <linux/i2c.h>
 #include <linux/init.h>
 #include <linux/interrupt.h>
 #include <linux/module.h>
 #include <linux/of_platform.h>
 #include <linux/platform_data/pca953x.h>
+#include <linux/pwm.h>
 #include <linux/regmap.h>
 #include <linux/regulator/consumer.h>
 #include <linux/slab.h>
 
 #include <asm/unaligned.h>
 
+#include "gpiolib.h"
+
 #define PCA953X_INPUT		0x00
 #define PCA953X_OUTPUT		0x01
 #define PCA953X_INVERT		0x02
@@ -63,11 +67,18 @@ 
 
 #define PCA_INT			BIT(8)
 #define PCA_PCAL		BIT(9)
+#define MAX_PWM			BIT(10)
 #define PCA_LATCH_INT		(PCA_PCAL | PCA_INT)
 #define PCA953X_TYPE		BIT(12)
 #define PCA957X_TYPE		BIT(13)
 #define PCA_TYPE_MASK		GENMASK(15, 12)
 
+#define MAX7313_MASTER		0x0E
+#define MAX7313_CONFIGURATION	0x0F
+#define MAX7313_INTENSITY	0x10
+
+#define MAX7313_GLOB_INTENSITY	BIT(2)
+
 #define PCA_CHIP_TYPE(x)	((x) & PCA_TYPE_MASK)
 
 static const struct i2c_device_id pca953x_id[] = {
@@ -93,7 +104,7 @@  static const struct i2c_device_id pca953x_id[] = {
 
 	{ "max7310", 8  | PCA953X_TYPE, },
 	{ "max7312", 16 | PCA953X_TYPE | PCA_INT, },
-	{ "max7313", 16 | PCA953X_TYPE | PCA_INT, },
+	{ "max7313", 16 | PCA953X_TYPE | PCA_INT | MAX_PWM, },
 	{ "max7315", 8  | PCA953X_TYPE | PCA_INT, },
 	{ "max7318", 16 | PCA953X_TYPE | PCA_INT, },
 	{ "pca6107", 8  | PCA953X_TYPE | PCA_INT, },
@@ -118,6 +129,14 @@  MODULE_DEVICE_TABLE(acpi, pca953x_acpi_ids);
 
 #define NBANK(chip) DIV_ROUND_UP(chip->gpio_chip.ngpio, BANK_SZ)
 
+#define PWM_PER_REG 2
+#define PWM_BITS_PER_REG (8 / PWM_PER_REG)
+#define PWM_INTENSITY_MASK GENMASK(PWM_BITS_PER_REG - 1, 0)
+
+#define PWM_PERIOD_NS 31250
+#define PWM_DC_STATES 16
+#define PWM_OFFSET_NS (PWM_PERIOD_NS / PWM_DC_STATES)
+
 struct pca953x_reg_config {
 	int direction;
 	int output;
@@ -139,6 +158,22 @@  static const struct pca953x_reg_config pca957x_regs = {
 	.invert = PCA957X_INVRT,
 };
 
+struct max7313_pwm_data {
+	struct gpio_desc *desc;
+	bool enabled;
+	unsigned int duty_cycle;
+};
+
+struct max7313_pwm {
+	struct pwm_chip chip;
+	/*
+	 * Protect races when counting active PWMs for enabling or disabling
+	 * the internal oscillator.
+	 */
+	struct mutex count_lock;
+	unsigned int count;
+};
+
 struct pca953x_chip {
 	unsigned gpio_start;
 	struct mutex i2c_lock;
@@ -161,6 +196,8 @@  struct pca953x_chip {
 	struct regulator *regulator;
 
 	const struct pca953x_reg_config *regs;
+
+	struct max7313_pwm pwm;
 };
 
 static int pca953x_bank_shift(struct pca953x_chip *chip)
@@ -241,8 +278,16 @@  static bool pca953x_check_register(struct pca953x_chip *chip, unsigned int reg,
 static bool pca953x_readable_register(struct device *dev, unsigned int reg)
 {
 	struct pca953x_chip *chip = dev_get_drvdata(dev);
+	unsigned int bank_sz = chip->driver_data & PCA_GPIO_MASK;
 	u32 bank;
 
+	if (PCA_CHIP_TYPE(chip->driver_data) == PCA953X_TYPE &&
+	    chip->driver_data & MAX_PWM) {
+		if (reg >= MAX7313_MASTER &&
+		    reg < (MAX7313_INTENSITY + bank_sz))
+			return true;
+	}
+
 	if (PCA_CHIP_TYPE(chip->driver_data) == PCA953X_TYPE) {
 		bank = PCA953x_BANK_INPUT | PCA953x_BANK_OUTPUT |
 		       PCA953x_BANK_POLARITY | PCA953x_BANK_CONFIG;
@@ -264,8 +309,16 @@  static bool pca953x_readable_register(struct device *dev, unsigned int reg)
 static bool pca953x_writeable_register(struct device *dev, unsigned int reg)
 {
 	struct pca953x_chip *chip = dev_get_drvdata(dev);
+	unsigned int bank_sz = chip->driver_data & PCA_GPIO_MASK;
 	u32 bank;
 
+	if (PCA_CHIP_TYPE(chip->driver_data) == PCA953X_TYPE &&
+	    chip->driver_data & MAX_PWM) {
+		if (reg >= MAX7313_MASTER &&
+		    reg < (MAX7313_INTENSITY + bank_sz))
+			return true;
+	}
+
 	if (PCA_CHIP_TYPE(chip->driver_data) == PCA953X_TYPE) {
 		bank = PCA953x_BANK_OUTPUT | PCA953x_BANK_POLARITY |
 			PCA953x_BANK_CONFIG;
@@ -886,6 +939,278 @@  static int device_pca957x_init(struct pca953x_chip *chip, u32 invert)
 	return ret;
 }
 
+/* PWM specific methods */
+
+static struct max7313_pwm *to_max7313_pwm(struct pwm_chip *chip)
+{
+	return container_of(chip, struct max7313_pwm, chip);
+}
+
+static struct pca953x_chip *to_pca953x(struct max7313_pwm *chip)
+{
+	return container_of(chip, struct pca953x_chip, pwm);
+}
+
+static u8 max7313_pwm_get_intensity(struct pca953x_chip *chip,
+				    unsigned int idx)
+{
+	unsigned int reg, shift, val;
+	u8 duty_cycle;
+
+	reg = MAX7313_INTENSITY + (idx / PWM_PER_REG);
+	shift = (idx % PWM_PER_REG) ? PWM_BITS_PER_REG : 0;
+
+	mutex_lock(&chip->i2c_lock);
+	regmap_read(chip->regmap, reg, &val);
+	mutex_unlock(&chip->i2c_lock);
+
+	if (shift)
+		val >>= shift;
+
+	val &= PWM_INTENSITY_MASK;
+
+	/*
+	 * Register values in the [0;15] range mean a value in the [1;16] range.
+	 * A register value of 16 means the logic has been inverted to produce a
+	 * static low output.
+	 */
+	if (val == PWM_INTENSITY_MASK)
+		duty_cycle = 0;
+	else
+		duty_cycle = val + 1;
+
+	return duty_cycle;
+}
+
+static int max7313_pwm_set_intensity(struct pca953x_chip *chip,
+				     unsigned int idx, u8 duty_cycle)
+{
+	/* Duty-cycle is in the range [1;16] while registers expect [0;15] */
+	u8 intensity = (duty_cycle - 1) & PWM_INTENSITY_MASK;
+	unsigned int reg, shift;
+	u8 val, mask;
+	int ret;
+
+	reg = MAX7313_INTENSITY + (idx / PWM_PER_REG);
+	shift = (idx % PWM_PER_REG) ? PWM_BITS_PER_REG : 0;
+
+	mask = PWM_INTENSITY_MASK << shift;
+	val = intensity << shift;
+
+	mutex_lock(&chip->i2c_lock);
+	ret = regmap_write_bits(chip->regmap, reg, mask, val);
+	mutex_unlock(&chip->i2c_lock);
+
+	return ret;
+}
+
+/*
+ * For a given PWM channel, when the blink phase 0 bit is set, the intensity
+ * range is only [1/16;16/16]. With this range, a static low output is
+ * physically not possible. When the blink phase 0 bit is cleared, the intensity
+ * range is [15/16;0/16] which then allows a static low output but not a static
+ * high output.
+ *
+ * In this driver we choose to set the blink phase 0 bit by default, hence we
+ * can slide from a low output to a fully high output without glitch. However,
+ * the only way to get a static low output is by clearing the blink phase 0 bit,
+ * and by changing the intensity value to its maximum (as, at this moment,
+ * intensity is reversed). There is no way to atomically flip the register *and*
+ * change the PWM value at the same time so this will produce a small glitch.
+ */
+static int max7313_pwm_set_state(struct pca953x_chip *chip,
+				 struct pwm_device *pwm_device,
+				 unsigned int duty_cycle)
+{
+	struct max7313_pwm_data *data = pwm_get_chip_data(pwm_device);
+	struct gpio_desc *desc = data->desc;
+	int ret;
+
+	/* A null duty_cycle will invert the phase */
+	ret = gpiod_direction_output(desc, duty_cycle);
+	if (ret)
+		return ret;
+
+	/* Maximize the low time in case of static low state */
+	if (!duty_cycle)
+		duty_cycle = PWM_DC_STATES;
+
+	return max7313_pwm_set_intensity(chip, pwm_device->hwpwm, duty_cycle);
+}
+
+static int max7313_pwm_request(struct pwm_chip *pwm_chip,
+			       struct pwm_device *pwm_device)
+{
+	struct max7313_pwm *pwm = to_max7313_pwm(pwm_chip);
+	struct pca953x_chip *chip = to_pca953x(pwm);
+	struct max7313_pwm_data *data;
+	struct gpio_desc *desc;
+	int ret;
+
+	desc = gpiochip_request_own_desc(&chip->gpio_chip, pwm_device->hwpwm,
+					 "max7313-pwm", GPIO_ACTIVE_HIGH,
+					 GPIOD_OUT_LOW);
+	if (IS_ERR(desc)) {
+		dev_err(&chip->client->dev,
+			"pin already in use (probably as GPIO)\n");
+		return PTR_ERR(desc);
+	}
+
+	data = devm_kzalloc(&chip->client->dev, sizeof(*data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	data->desc = desc;
+	pwm_set_chip_data(pwm_device, data);
+
+	ret = max7313_pwm_set_state(chip, pwm_device, 0);
+	if (ret) {
+		dev_err(&chip->client->dev, "cannot set PWM default state\n");
+		goto free_gpiod;
+	}
+
+	/*
+	 * Set master intensity to the maximum level to let individual outputs
+	 * the greatest flexibility range. Also enables the internal oscillator.
+	 */
+	mutex_lock(&pwm->count_lock);
+	if (!pwm->count) {
+		mutex_lock(&chip->i2c_lock);
+		ret = regmap_write_bits(chip->regmap, MAX7313_MASTER,
+					PWM_INTENSITY_MASK << PWM_BITS_PER_REG,
+					PWM_INTENSITY_MASK << PWM_BITS_PER_REG);
+		mutex_unlock(&chip->i2c_lock);
+	}
+
+	if (!ret)
+		pwm->count++;
+
+	mutex_unlock(&pwm->count_lock);
+
+	if (ret)
+		goto free_gpiod;
+
+	return 0;
+
+free_gpiod:
+	gpiochip_free_own_desc(data->desc);
+
+	return ret;
+}
+
+static void max7313_pwm_free(struct pwm_chip *pwm_chip,
+			     struct pwm_device *pwm_device)
+{
+	struct max7313_pwm_data *data = pwm_get_chip_data(pwm_device);
+	struct max7313_pwm *pwm = to_max7313_pwm(pwm_chip);
+	struct pca953x_chip *chip = to_pca953x(pwm);
+
+	max7313_pwm_set_state(chip, pwm_device, 0);
+
+	mutex_lock(&pwm->count_lock);
+
+	pwm->count--;
+
+	/* Disable the internal oscillator if no channel is in use */
+	if (!pwm->count) {
+		mutex_lock(&chip->i2c_lock);
+		regmap_write_bits(chip->regmap, MAX7313_MASTER,
+				  PWM_INTENSITY_MASK << PWM_BITS_PER_REG, 0);
+		mutex_unlock(&chip->i2c_lock);
+	}
+
+	mutex_unlock(&pwm->count_lock);
+
+	gpiochip_free_own_desc(data->desc);
+}
+
+static int max7313_pwm_apply(struct pwm_chip *pwm_chip,
+			     struct pwm_device *pwm_device,
+			     const struct pwm_state *state)
+{
+	struct max7313_pwm_data *data = pwm_get_chip_data(pwm_device);
+	struct max7313_pwm *pwm = to_max7313_pwm(pwm_chip);
+	struct pca953x_chip *chip = to_pca953x(pwm);
+	unsigned int duty_cycle;
+
+	if (state->period != PWM_PERIOD_NS ||
+	    state->polarity != PWM_POLARITY_NORMAL)
+		return -EINVAL;
+
+	data->enabled = state->enabled;
+	data->duty_cycle = state->duty_cycle;
+
+	if (!state->enabled || !state->duty_cycle)
+		duty_cycle = 0;
+	else
+		/* Convert the duty-cycle to be in the [1;16] range */
+		duty_cycle = DIV_ROUND_DOWN_ULL(state->duty_cycle,
+						PWM_OFFSET_NS);
+
+	/* The hardware is supposedly glitch-free */
+	return max7313_pwm_set_state(chip, pwm_device, duty_cycle);
+}
+
+static void max7313_pwm_get_state(struct pwm_chip *pwm_chip,
+				  struct pwm_device *pwm_device,
+				  struct pwm_state *state)
+{
+	struct max7313_pwm_data *data = pwm_get_chip_data(pwm_device);
+	struct max7313_pwm *pwm = to_max7313_pwm(pwm_chip);
+	struct pca953x_chip *chip = to_pca953x(pwm);
+	u8 duty_cycle;
+
+	state->period = PWM_PERIOD_NS;
+	state->polarity = PWM_POLARITY_NORMAL;
+
+	if (!data)
+		return;
+
+	state->enabled = data->enabled;
+	if (!state->enabled) {
+		state->duty_cycle = data->duty_cycle;
+	} else {
+		duty_cycle = max7313_pwm_get_intensity(chip, pwm_device->hwpwm);
+		state->duty_cycle = duty_cycle * PWM_OFFSET_NS;
+	}
+};
+
+static const struct pwm_ops max7313_pwm_ops = {
+	.request = max7313_pwm_request,
+	.free = max7313_pwm_free,
+	.apply = max7313_pwm_apply,
+	.get_state = max7313_pwm_get_state,
+	.owner = THIS_MODULE,
+};
+
+static int max7313_pwm_probe(struct device *dev,
+			     struct pca953x_chip *chip)
+{
+	struct max7313_pwm *pwm = &chip->pwm;
+	struct pwm_chip *pwm_chip = &pwm->chip;
+	int ret;
+
+	if (!(chip->driver_data & MAX_PWM))
+		return 0;
+
+	pwm_chip->dev = dev;
+	pwm_chip->ops = &max7313_pwm_ops;
+	pwm_chip->npwm = chip->gpio_chip.ngpio;
+	pwm_chip->base = -1;
+
+	/* Disable global control (does not affect GPIO functionality) */
+	mutex_lock(&chip->i2c_lock);
+	ret = regmap_write_bits(chip->regmap, MAX7313_CONFIGURATION,
+				MAX7313_GLOB_INTENSITY, 0);
+	mutex_unlock(&chip->i2c_lock);
+	if (ret)
+		return ret;
+
+	mutex_init(&pwm->count_lock);
+
+	return pwmchip_add(pwm_chip);
+}
+
 static const struct of_device_id pca953x_dt_ids[];
 
 static int pca953x_probe(struct i2c_client *client,
@@ -1018,6 +1343,9 @@  static int pca953x_probe(struct i2c_client *client,
 			dev_warn(&client->dev, "setup failed, %d\n", ret);
 	}
 
+	if (IS_ENABLED(CONFIG_PWM))
+		return max7313_pwm_probe(&client->dev, chip);
+
 	return 0;
 
 err_exit:
@@ -1162,7 +1490,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), },