| Message ID | 20191129191023.2209-1-miquel.raynal@bootlin.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v4] gpio: pca953x: Add Maxim MAX7313 PWM support | expand |
On Fri, Nov 29, 2019 at 8:10 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> > --- > > Changes in v4: I'm fine to merge this v4 if I get Thierry's ACK on it. Yours, Linus Walleij
On Fri, Nov 29, 2019 at 08:10:23PM +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 v4: > * Fix wrong comment about register value. > * Rewrite ->set_state() to make it more readable, include the fact > that the phase may blink and to limit the number of blink changes > when possible ("lazy switching" as discussed with Uwe). > * Prevent using managed memory when not relevant. > * Add a definition to the master intensity shift. > * Rename all struct pwm_device to pwm and all struct pwm_chip as > chip. Then, struct pca953x_chip are called pca_chip instead of chip > and struct max7313_pwm are called max_pwm intead of pwm. > * Enhance the comment about glitch-free hardware. > * Add a plain error check at the ->pwm_probe() return location. > * Rename duty_cycle to intensity when relevant. > * Do not initialize the PWM in ->request(). Also do not change the > state in ->free(). > * New way to count enabled/disabled PWM (with a bitmap). Disable the > oscillator only when 0 PWM are in use, enable it when there is > one. Also always set the pin to output state otherwise the default > might be input. > * Force state->enable to be true and drop all the boilerplate around > enable and .duty_cycle. > > 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 | 375 +++++++++++++++++++++++++++++++++++- > 1 file changed, 373 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpio/gpio-pca953x.c b/drivers/gpio/gpio-pca953x.c > index de5d1383f28d..347988bc630f 100644 > --- a/drivers/gpio/gpio-pca953x.c > +++ b/drivers/gpio/gpio-pca953x.c > @@ -10,20 +10,25 @@ > > #include <linux/acpi.h> > #include <linux/bits.h> > +#include <linux/bitmap.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 +68,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 +105,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 +130,16 @@ MODULE_DEVICE_TABLE(acpi, pca953x_acpi_ids); > > #define NBANK(chip) DIV_ROUND_UP(chip->gpio_chip.ngpio, BANK_SZ) > > +#define PWM_MAX_COUNT 16 > +#define PWM_PER_REG 2 > +#define PWM_BITS_PER_REG (8 / PWM_PER_REG) > +#define PWM_MASTER_INTENSITY_SHIFT 4 > +#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 +161,20 @@ static const struct pca953x_reg_config pca957x_regs = { > .invert = PCA957X_INVRT, > }; > > +struct max7313_pwm_data { > + struct gpio_desc *desc; > +}; > + > +struct max7313_pwm { > + struct pwm_chip chip; > + /* > + * Protect races when counting active PWMs for enabling or disabling > + * the internal oscillator. > + */ > + struct mutex count_lock; > + DECLARE_BITMAP(active_pwm, PWM_MAX_COUNT); > +}; > + > struct pca953x_chip { > unsigned gpio_start; > struct mutex i2c_lock; > @@ -161,6 +197,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 +279,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 +310,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 +940,315 @@ static int device_pca957x_init(struct pca953x_chip *chip, u32 invert) > return ret; > } > > +/* > + * Max7313 PWM specific methods > + * > + * Limitations: > + * - Does not support a disabled state > + * - Period fixed to 31.25ms > + * - Only supports normal polarity > + * - Some glitches cannot be prevented > + */ > + > +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 *pca_chip, > + unsigned int idx) > +{ > + unsigned int reg, shift, val, output; > + u8 intensity; > + bool phase; > + > + /* Retrieve the intensity */ > + reg = MAX7313_INTENSITY + (idx / PWM_PER_REG); > + shift = (idx % PWM_PER_REG) ? PWM_BITS_PER_REG : 0; > + > + mutex_lock(&pca_chip->i2c_lock); > + regmap_read(pca_chip->regmap, reg, &val); > + mutex_unlock(&pca_chip->i2c_lock); > + > + if (shift) > + val >>= shift; > + > + val &= PWM_INTENSITY_MASK; > + > + /* Retrieve the phase */ > + reg = pca953x_recalc_addr(pca_chip, pca_chip->regs->output, idx, 0, 0); > + > + mutex_lock(&pca_chip->i2c_lock); > + regmap_read(pca_chip->regmap, reg, &output); > + mutex_unlock(&pca_chip->i2c_lock); > + > + phase = output & BIT(idx % BANK_SZ); > + > + /* > + * Register values in the [0;15] range mean a value in the [1/16;16/16] > + * range if the phase is set, a [15/16;0/16] range otherwise. > + */ > + if (phase) > + intensity = val + 1; > + else > + intensity = PWM_INTENSITY_MASK - val; > + > + return intensity; > +} > + > +static int max7313_pwm_set_intensity(struct pca953x_chip *pca_chip, > + unsigned int idx, u8 intensity) > +{ > + 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 & PWM_INTENSITY_MASK) << shift; > + > + mutex_lock(&pca_chip->i2c_lock); > + ret = regmap_write_bits(pca_chip->regmap, reg, mask, val); > + mutex_unlock(&pca_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 switch the blink phase only when mandatory > + * because there is no way to atomically flip the register *and* change the PWM > + * value at the same time so, in this case, it will produce a small glitch. > + */ > +static int max7313_pwm_set_state(struct pca953x_chip *pca_chip, > + struct pwm_device *pwm, > + unsigned int intensity) > +{ > + struct max7313_pwm_data *data = pwm_get_chip_data(pwm); > + struct gpio_desc *desc = data->desc; > + unsigned int idx = pwm->hwpwm, reg, output; > + bool phase; > + int ret; > + > + /* Retrieve the phase */ > + reg = pca953x_recalc_addr(pca_chip, pca_chip->regs->output, idx, 0, 0); > + > + mutex_lock(&pca_chip->i2c_lock); > + regmap_read(pca_chip->regmap, reg, &output); > + mutex_unlock(&pca_chip->i2c_lock); > + > + phase = output & BIT(idx % BANK_SZ); > + > + /* Need to blink the phase */ > + if ((phase && !intensity) || (!phase && intensity == PWM_DC_STATES)) { > + phase = !phase; > + ret = gpiod_direction_output(desc, phase); > + if (ret) > + return ret; > + } else { > + /* Ensure the pin is in output state (default might be input) */ > + ret = gpiod_direction_output(desc, phase); > + if (ret) > + return ret; > + } > + > + if (phase) > + intensity = intensity - 1; > + else > + intensity = PWM_INTENSITY_MASK - intensity; > + > + return max7313_pwm_set_intensity(pca_chip, idx, intensity); > +} > + > +static int max7313_pwm_set_master_intensity(struct pca953x_chip *pca_chip, > + u8 intensity) > +{ > + int ret; > + > + intensity &= PWM_INTENSITY_MASK; > + > + mutex_lock(&pca_chip->i2c_lock); > + ret = regmap_write_bits(pca_chip->regmap, MAX7313_MASTER, > + PWM_INTENSITY_MASK << PWM_MASTER_INTENSITY_SHIFT, > + intensity << PWM_MASTER_INTENSITY_SHIFT); > + mutex_unlock(&pca_chip->i2c_lock); > + > + return ret; > +} > + > +static int max7313_pwm_request(struct pwm_chip *chip, > + struct pwm_device *pwm) > +{ > + struct max7313_pwm *max_pwm = to_max7313_pwm(chip); > + struct pca953x_chip *pca_chip = to_pca953x(max_pwm); > + struct max7313_pwm_data *data; > + struct gpio_desc *desc; > + > + desc = gpiochip_request_own_desc(&pca_chip->gpio_chip, pwm->hwpwm, > + "max7313-pwm", GPIO_ACTIVE_HIGH, 0); > + if (IS_ERR(desc)) { > + dev_err(&pca_chip->client->dev, > + "pin already in use (probably as GPIO): %ld\n", > + PTR_ERR(desc)); > + return PTR_ERR(desc); > + } > + > + data = kzalloc(sizeof(*data), GFP_KERNEL); > + if (!data) { > + gpiochip_free_own_desc(desc); > + return -ENOMEM; > + } > + > + data->desc = desc; > + pwm_set_chip_data(pwm, data); > + > + return 0; > +} > + > +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 || > + state->polarity != PWM_POLARITY_NORMAL) > + return -EINVAL; > + > + /* Convert the duty-cycle to be in the [0;16] range */ > + intensity = DIV_ROUND_DOWN_ULL(state->duty_cycle, > + state->period / PWM_DC_STATES); this looks wrong. The period must not have an influence on the selection of the duty_cycle (other than duty_cycle < period). So given that the period is fixed at 31.25 ms, the result of pwm_apply_state(pwm, { .enabled = 1, .period = 2s, .duty_cycle = 16 ms }) must be the same as for pwm_apply_state(pwm, { .enabled = 1, .period = 3s, .duty_cycle = 16 ms }) . Also dividing by a division looses precision. > + /* > + * If this is the first PWM to enable, set the master intensity to the > + * maximum level to let individual outputs the greatest flexibility > + * range. It also enables the internal oscillator. > + * > + * When shutting down the last active PWM, the oscillator is disabled. > + * > + * Lazy logic is applied to simplify the code: always enable the > + * oscillator when there is 1 active pwm, always disable it when there > + * is none. > + */ > + mutex_lock(&max_pwm->count_lock); > + if (intensity) > + set_bit(pwm->hwpwm, max_pwm->active_pwm); > + else > + clear_bit(pwm->hwpwm, max_pwm->active_pwm); > + > + active = bitmap_weight(max_pwm->active_pwm, PWM_MAX_COUNT); > + if (!active) > + ret = max7313_pwm_set_master_intensity(pca_chip, 0); > + else if (active == 1) > + ret = max7313_pwm_set_master_intensity(pca_chip, > + PWM_INTENSITY_MASK); > + mutex_unlock(&max_pwm->count_lock); > + > + if (ret) > + return ret; > + > + /* > + * The hardware is supposedly glitch-free when changing the intensity, > + * unless we need to flip the blink phase to reach an extremity or the > + * other of the spectre (0/16 from phase 1, 16/16 from phase 0). I think you mean spectrum here, not spectre. :-) > + */ > + return max7313_pwm_set_state(pca_chip, pwm, intensity); > +} > + > +static void max7313_pwm_get_state(struct pwm_chip *chip, > + struct pwm_device *pwm, > + struct pwm_state *state) > +{ > + struct max7313_pwm *max_pwm = to_max7313_pwm(chip); > + struct pca953x_chip *pca_chip = to_pca953x(max_pwm); > + u8 intensity; > + > + state->enabled = true; > + state->period = PWM_PERIOD_NS; > + state->polarity = PWM_POLARITY_NORMAL; > + > + intensity = max7313_pwm_get_intensity(pca_chip, pwm->hwpwm); > + state->duty_cycle = intensity * PWM_OFFSET_NS; I think you need to take into account the blink phase bit. Best regards Uwe
Hi Uwe, Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote on Thu, 12 Dec 2019 22:14:34 +0100: > On Fri, Nov 29, 2019 at 08:10:23PM +0100, Miquel Raynal wrote: > > 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 v4: > > * Fix wrong comment about register value. > > * Rewrite ->set_state() to make it more readable, include the fact > > that the phase may blink and to limit the number of blink changes > > when possible ("lazy switching" as discussed with Uwe). > > * Prevent using managed memory when not relevant. > > * Add a definition to the master intensity shift. > > * Rename all struct pwm_device to pwm and all struct pwm_chip as > > chip. Then, struct pca953x_chip are called pca_chip instead of chip > > and struct max7313_pwm are called max_pwm intead of pwm. > > * Enhance the comment about glitch-free hardware. > > * Add a plain error check at the ->pwm_probe() return location. > > * Rename duty_cycle to intensity when relevant. > > * Do not initialize the PWM in ->request(). Also do not change the > > state in ->free(). > > * New way to count enabled/disabled PWM (with a bitmap). Disable the > > oscillator only when 0 PWM are in use, enable it when there is > > one. Also always set the pin to output state otherwise the default > > might be input. > > * Force state->enable to be true and drop all the boilerplate around > > enable and .duty_cycle. > > > > 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 | 375 +++++++++++++++++++++++++++++++++++- > > 1 file changed, 373 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/gpio/gpio-pca953x.c b/drivers/gpio/gpio-pca953x.c > > index de5d1383f28d..347988bc630f 100644 > > --- a/drivers/gpio/gpio-pca953x.c > > +++ b/drivers/gpio/gpio-pca953x.c > > @@ -10,20 +10,25 @@ > > > > #include <linux/acpi.h> > > #include <linux/bits.h> > > +#include <linux/bitmap.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 +68,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 +105,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 +130,16 @@ MODULE_DEVICE_TABLE(acpi, pca953x_acpi_ids); > > > > #define NBANK(chip) DIV_ROUND_UP(chip->gpio_chip.ngpio, BANK_SZ) > > > > +#define PWM_MAX_COUNT 16 > > +#define PWM_PER_REG 2 > > +#define PWM_BITS_PER_REG (8 / PWM_PER_REG) > > +#define PWM_MASTER_INTENSITY_SHIFT 4 > > +#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 +161,20 @@ static const struct pca953x_reg_config pca957x_regs = { > > .invert = PCA957X_INVRT, > > }; > > > > +struct max7313_pwm_data { > > + struct gpio_desc *desc; > > +}; > > + > > +struct max7313_pwm { > > + struct pwm_chip chip; > > + /* > > + * Protect races when counting active PWMs for enabling or disabling > > + * the internal oscillator. > > + */ > > + struct mutex count_lock; > > + DECLARE_BITMAP(active_pwm, PWM_MAX_COUNT); > > +}; > > + > > struct pca953x_chip { > > unsigned gpio_start; > > struct mutex i2c_lock; > > @@ -161,6 +197,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 +279,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 +310,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 +940,315 @@ static int device_pca957x_init(struct pca953x_chip *chip, u32 invert) > > return ret; > > } > > > > +/* > > + * Max7313 PWM specific methods > > + * > > + * Limitations: > > + * - Does not support a disabled state > > + * - Period fixed to 31.25ms > > + * - Only supports normal polarity > > + * - Some glitches cannot be prevented > > + */ > > + > > +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 *pca_chip, > > + unsigned int idx) > > +{ > > + unsigned int reg, shift, val, output; > > + u8 intensity; > > + bool phase; > > + > > + /* Retrieve the intensity */ > > + reg = MAX7313_INTENSITY + (idx / PWM_PER_REG); > > + shift = (idx % PWM_PER_REG) ? PWM_BITS_PER_REG : 0; > > + > > + mutex_lock(&pca_chip->i2c_lock); > > + regmap_read(pca_chip->regmap, reg, &val); > > + mutex_unlock(&pca_chip->i2c_lock); > > + > > + if (shift) > > + val >>= shift; > > + > > + val &= PWM_INTENSITY_MASK; > > + > > + /* Retrieve the phase */ > > + reg = pca953x_recalc_addr(pca_chip, pca_chip->regs->output, idx, 0, 0); > > + > > + mutex_lock(&pca_chip->i2c_lock); > > + regmap_read(pca_chip->regmap, reg, &output); > > + mutex_unlock(&pca_chip->i2c_lock); > > + > > + phase = output & BIT(idx % BANK_SZ); > > + > > + /* > > + * Register values in the [0;15] range mean a value in the [1/16;16/16] > > + * range if the phase is set, a [15/16;0/16] range otherwise. > > + */ > > + if (phase) > > + intensity = val + 1; > > + else > > + intensity = PWM_INTENSITY_MASK - val; > > + > > + return intensity; > > +} > > + > > +static int max7313_pwm_set_intensity(struct pca953x_chip *pca_chip, > > + unsigned int idx, u8 intensity) > > +{ > > + 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 & PWM_INTENSITY_MASK) << shift; > > + > > + mutex_lock(&pca_chip->i2c_lock); > > + ret = regmap_write_bits(pca_chip->regmap, reg, mask, val); > > + mutex_unlock(&pca_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 switch the blink phase only when mandatory > > + * because there is no way to atomically flip the register *and* change the PWM > > + * value at the same time so, in this case, it will produce a small glitch. > > + */ > > +static int max7313_pwm_set_state(struct pca953x_chip *pca_chip, > > + struct pwm_device *pwm, > > + unsigned int intensity) > > +{ > > + struct max7313_pwm_data *data = pwm_get_chip_data(pwm); > > + struct gpio_desc *desc = data->desc; > > + unsigned int idx = pwm->hwpwm, reg, output; > > + bool phase; > > + int ret; > > + > > + /* Retrieve the phase */ > > + reg = pca953x_recalc_addr(pca_chip, pca_chip->regs->output, idx, 0, 0); > > + > > + mutex_lock(&pca_chip->i2c_lock); > > + regmap_read(pca_chip->regmap, reg, &output); > > + mutex_unlock(&pca_chip->i2c_lock); > > + > > + phase = output & BIT(idx % BANK_SZ); > > + > > + /* Need to blink the phase */ > > + if ((phase && !intensity) || (!phase && intensity == PWM_DC_STATES)) { > > + phase = !phase; > > + ret = gpiod_direction_output(desc, phase); > > + if (ret) > > + return ret; > > + } else { > > + /* Ensure the pin is in output state (default might be input) */ > > + ret = gpiod_direction_output(desc, phase); > > + if (ret) > > + return ret; > > + } > > + > > + if (phase) > > + intensity = intensity - 1; > > + else > > + intensity = PWM_INTENSITY_MASK - intensity; > > + > > + return max7313_pwm_set_intensity(pca_chip, idx, intensity); > > +} > > + > > +static int max7313_pwm_set_master_intensity(struct pca953x_chip *pca_chip, > > + u8 intensity) > > +{ > > + int ret; > > + > > + intensity &= PWM_INTENSITY_MASK; > > + > > + mutex_lock(&pca_chip->i2c_lock); > > + ret = regmap_write_bits(pca_chip->regmap, MAX7313_MASTER, > > + PWM_INTENSITY_MASK << PWM_MASTER_INTENSITY_SHIFT, > > + intensity << PWM_MASTER_INTENSITY_SHIFT); > > + mutex_unlock(&pca_chip->i2c_lock); > > + > > + return ret; > > +} > > + > > +static int max7313_pwm_request(struct pwm_chip *chip, > > + struct pwm_device *pwm) > > +{ > > + struct max7313_pwm *max_pwm = to_max7313_pwm(chip); > > + struct pca953x_chip *pca_chip = to_pca953x(max_pwm); > > + struct max7313_pwm_data *data; > > + struct gpio_desc *desc; > > + > > + desc = gpiochip_request_own_desc(&pca_chip->gpio_chip, pwm->hwpwm, > > + "max7313-pwm", GPIO_ACTIVE_HIGH, 0); > > + if (IS_ERR(desc)) { > > + dev_err(&pca_chip->client->dev, > > + "pin already in use (probably as GPIO): %ld\n", > > + PTR_ERR(desc)); > > + return PTR_ERR(desc); > > + } > > + > > + data = kzalloc(sizeof(*data), GFP_KERNEL); > > + if (!data) { > > + gpiochip_free_own_desc(desc); > > + return -ENOMEM; > > + } > > + > > + data->desc = desc; > > + pwm_set_chip_data(pwm, data); > > + > > + return 0; > > +} > > + > > +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 || > > + state->polarity != PWM_POLARITY_NORMAL) > > + return -EINVAL; > > + > > + /* Convert the duty-cycle to be in the [0;16] range */ > > + intensity = DIV_ROUND_DOWN_ULL(state->duty_cycle, > > + state->period / PWM_DC_STATES); > > this looks wrong. The period must not have an influence on the selection > of the duty_cycle (other than duty_cycle < period). So given that the > period is fixed at 31.25 ms, the result of > > pwm_apply_state(pwm, { .enabled = 1, .period = 2s, .duty_cycle = 16 ms }) > > must be the same as for > > pwm_apply_state(pwm, { .enabled = 1, .period = 3s, .duty_cycle = 16 ms }) This was not my understanding of our previous discussion and, again, I don't really understand this choice but if the framework works like that we shall probably continue with this logic. However, all this should probably be explained directly in the kernel doc of each core helper, that would help a lot. > . Also dividing by a division looses precision. I agree but this is a problem with fixed point calculations. Maybe I could first multiply the numerator by a factor of 100 or 1000 to minimize the error. Do you have a better idea? > > > + /* > > + * If this is the first PWM to enable, set the master intensity to the > > + * maximum level to let individual outputs the greatest flexibility > > + * range. It also enables the internal oscillator. > > + * > > + * When shutting down the last active PWM, the oscillator is disabled. > > + * > > + * Lazy logic is applied to simplify the code: always enable the > > + * oscillator when there is 1 active pwm, always disable it when there > > + * is none. > > + */ > > + mutex_lock(&max_pwm->count_lock); > > + if (intensity) > > + set_bit(pwm->hwpwm, max_pwm->active_pwm); > > + else > > + clear_bit(pwm->hwpwm, max_pwm->active_pwm); > > + > > + active = bitmap_weight(max_pwm->active_pwm, PWM_MAX_COUNT); > > + if (!active) > > + ret = max7313_pwm_set_master_intensity(pca_chip, 0); > > + else if (active == 1) > > + ret = max7313_pwm_set_master_intensity(pca_chip, > > + PWM_INTENSITY_MASK); > > + mutex_unlock(&max_pwm->count_lock); > > + > > + if (ret) > > + return ret; > > + > > + /* > > + * The hardware is supposedly glitch-free when changing the intensity, > > + * unless we need to flip the blink phase to reach an extremity or the > > + * other of the spectre (0/16 from phase 1, 16/16 from phase 0). > > I think you mean spectrum here, not spectre. :-) Oops :) > > > + */ > > + return max7313_pwm_set_state(pca_chip, pwm, intensity); > > +} > > + > > +static void max7313_pwm_get_state(struct pwm_chip *chip, > > + struct pwm_device *pwm, > > + struct pwm_state *state) > > +{ > > + struct max7313_pwm *max_pwm = to_max7313_pwm(chip); > > + struct pca953x_chip *pca_chip = to_pca953x(max_pwm); > > + u8 intensity; > > + > > + state->enabled = true; > > + state->period = PWM_PERIOD_NS; > > + state->polarity = PWM_POLARITY_NORMAL; > > + > > + intensity = max7313_pwm_get_intensity(pca_chip, pwm->hwpwm); > > + state->duty_cycle = intensity * PWM_OFFSET_NS; > > I think you need to take into account the blink phase bit. It is already done by .pwm_get_intensity itself, no? Thanks, Miquèl
Hi Miquel, On Mon, Dec 16, 2019 at 09:39:55AM +0100, Miquel Raynal wrote: > Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote on Thu, 12 Dec > 2019 22:14:34 +0100: > > > On Fri, Nov 29, 2019 at 08:10:23PM +0100, Miquel Raynal wrote: > > > +static int max7313_pwm_apply(struct pwm_chip *chip, > > > + struct pwm_device *pwm, > > > + const struct pwm_state *state) > > > +{ > > > + struct max7313_pwm *max_pwm = to_max7313_pwm(chip); > > > + struct pca953x_chip *pca_chip = to_pca953x(max_pwm); > > > + unsigned int intensity, active; > > > + int ret = 0; > > > + > > > + if (!state->enabled || > > > + state->period < PWM_PERIOD_NS || > > > + state->polarity != PWM_POLARITY_NORMAL) > > > + return -EINVAL; > > > + > > > + /* Convert the duty-cycle to be in the [0;16] range */ > > > + intensity = DIV_ROUND_DOWN_ULL(state->duty_cycle, > > > + state->period / PWM_DC_STATES); > > > > this looks wrong. The period must not have an influence on the selection > > of the duty_cycle (other than duty_cycle < period). So given that the > > period is fixed at 31.25 ms, the result of > > > > pwm_apply_state(pwm, { .enabled = 1, .period = 2s, .duty_cycle = 16 ms }) > > > > must be the same as for > > > > pwm_apply_state(pwm, { .enabled = 1, .period = 3s, .duty_cycle = 16 ms }) > > This was not my understanding of our previous discussion and, again, I > don't really understand this choice but if the framework works like > that we shall probably continue with this logic. However, all this > should probably be explained directly in the kernel doc of each core > helper, that would help a lot. I agree. There is a pending doc patch and once Thierry applies it I plan to add these details. The idea is to make the policy simple (both computational and to understand). With each policy there are strange corner cases, so for sure you can come up with examples that yield results that are way off from the request. The idea is that drivers all implement the same policy and then create helper functions to match the different consumer needs. > > . Also dividing by a division looses precision. > > I agree but this is a problem with fixed point calculations. Maybe I > could first multiply the numerator by a factor of 100 or 1000 to > minimize the error. Do you have a better idea? intensity = DIV_ROUND_DOWN_ULL((unsigned long long)state->duty_cycle * PWM_DC_STATES, state->period); should be both more exact and cheaper to calculate. (But this is somewhat moot given that state->period shouldn't be there.) (And in general you have to care for overflowing, but I think that's not a problem in practise here as struct pwm_state::duty_cycle is an int (still), and PWM_DC_STATES is small.) > > > +static void max7313_pwm_get_state(struct pwm_chip *chip, > > > + struct pwm_device *pwm, > > > + struct pwm_state *state) > > > +{ > > > + struct max7313_pwm *max_pwm = to_max7313_pwm(chip); > > > + struct pca953x_chip *pca_chip = to_pca953x(max_pwm); > > > + u8 intensity; > > > + > > > + state->enabled = true; > > > + state->period = PWM_PERIOD_NS; > > > + state->polarity = PWM_POLARITY_NORMAL; > > > + > > > + intensity = max7313_pwm_get_intensity(pca_chip, pwm->hwpwm); > > > + state->duty_cycle = intensity * PWM_OFFSET_NS; > > > > I think you need to take into account the blink phase bit. > > It is already done by .pwm_get_intensity itself, no? Ah, possible, I admin I didn't look deep enough to catch it there. Best regards Uwe
Hi Uwe, Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote on Mon, 16 Dec 2019 09:54:24 +0100: > Hi Miquel, > > On Mon, Dec 16, 2019 at 09:39:55AM +0100, Miquel Raynal wrote: > > Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote on Thu, 12 Dec > > 2019 22:14:34 +0100: > > > > > On Fri, Nov 29, 2019 at 08:10:23PM +0100, Miquel Raynal wrote: > > > > +static int max7313_pwm_apply(struct pwm_chip *chip, > > > > + struct pwm_device *pwm, > > > > + const struct pwm_state *state) > > > > +{ > > > > + struct max7313_pwm *max_pwm = to_max7313_pwm(chip); > > > > + struct pca953x_chip *pca_chip = to_pca953x(max_pwm); > > > > + unsigned int intensity, active; > > > > + int ret = 0; > > > > + > > > > + if (!state->enabled || > > > > + state->period < PWM_PERIOD_NS || > > > > + state->polarity != PWM_POLARITY_NORMAL) > > > > + return -EINVAL; > > > > + > > > > + /* Convert the duty-cycle to be in the [0;16] range */ > > > > + intensity = DIV_ROUND_DOWN_ULL(state->duty_cycle, > > > > + state->period / PWM_DC_STATES); > > > > > > this looks wrong. The period must not have an influence on the selection > > > of the duty_cycle (other than duty_cycle < period). So given that the > > > period is fixed at 31.25 ms, the result of > > > > > > pwm_apply_state(pwm, { .enabled = 1, .period = 2s, .duty_cycle = 16 ms }) > > > > > > must be the same as for > > > > > > pwm_apply_state(pwm, { .enabled = 1, .period = 3s, .duty_cycle = 16 ms }) > > > > This was not my understanding of our previous discussion and, again, I > > don't really understand this choice but if the framework works like > > that we shall probably continue with this logic. However, all this > > should probably be explained directly in the kernel doc of each core > > helper, that would help a lot. > > I agree. There is a pending doc patch and once Thierry applies it I plan > to add these details. Great! > > The idea is to make the policy simple (both computational and to > understand). With each policy there are strange corner cases, so for > sure you can come up with examples that yield results that are way off > from the request. The idea is that drivers all implement the same policy > and then create helper functions to match the different consumer needs. I fully understand and comply with this. The above logic was not the first one that would have came off my mind but it is 100% true that it keeps the computation easy (= less bugs, = quicker) and has probably been much more pondered than mine. > > > > . Also dividing by a division looses precision. > > > > I agree but this is a problem with fixed point calculations. Maybe I > > could first multiply the numerator by a factor of 100 or 1000 to > > minimize the error. Do you have a better idea? > > intensity = DIV_ROUND_DOWN_ULL((unsigned long long)state->duty_cycle * PWM_DC_STATES, state->period); > > should be both more exact and cheaper to calculate. (But this is > somewhat moot given that state->period shouldn't be there.) I feel stupid now - let's put it on monday mood. Of course it's more accurate this way. > (And in general you have to care for overflowing, but I think that's not > a problem in practise here as struct pwm_state::duty_cycle is an int > (still), and PWM_DC_STATES is small.) Do you plan to change duty_cycle type? Right now the multiplication cannot be over 500 000 which is totally okay for a s32 but not for a s16 for instance. > > > > +static void max7313_pwm_get_state(struct pwm_chip *chip, > > > > + struct pwm_device *pwm, > > > > + struct pwm_state *state) > > > > +{ > > > > + struct max7313_pwm *max_pwm = to_max7313_pwm(chip); > > > > + struct pca953x_chip *pca_chip = to_pca953x(max_pwm); > > > > + u8 intensity; > > > > + > > > > + state->enabled = true; > > > > + state->period = PWM_PERIOD_NS; > > > > + state->polarity = PWM_POLARITY_NORMAL; > > > > + > > > > + intensity = max7313_pwm_get_intensity(pca_chip, pwm->hwpwm); > > > > + state->duty_cycle = intensity * PWM_OFFSET_NS; > > > > > > I think you need to take into account the blink phase bit. > > > > It is already done by .pwm_get_intensity itself, no? > > Ah, possible, I admin I didn't look deep enough to catch it there. No problem, thanks anyway for all the careful reviews! Thanks, Miquèl
Hi Miquèl On Mon, Dec 16, 2019 at 10:14:16AM +0100, Miquel Raynal wrote: > Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote on Mon, 16 Dec > 2019 09:54:24 +0100: > > intensity = DIV_ROUND_DOWN_ULL((unsigned long long)state->duty_cycle * PWM_DC_STATES, state->period); > > > > should be both more exact and cheaper to calculate. (But this is > > somewhat moot given that state->period shouldn't be there.) > > (And in general you have to care for overflowing, but I think that's not > > a problem in practise here as struct pwm_state::duty_cycle is an int > > (still), and PWM_DC_STATES is small.) > > Do you plan to change duty_cycle type? I don't, but https://patchwork.ozlabs.org/patch/1195481/ Best regards Uwe
I would like to be Cc'ed on the matters. On Fri, Nov 29, 2019 at 9:13 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. Thanks for an update! Still I think it's wrong approach. What should be done is: - adding a pin configuration type of PWM (when, for example, argument defines duty cycle and period) - conversion to pin control of this driver - enabling pin configuration PWM for it. For now it looks like a custom way of doing it. If GPIO maintainers are okay with it, I'll not object, just want to have than something like TODO updated for the matter. Taking above into consideration, I also provide my comments to the patch ... > #include <linux/bits.h> > +#include <linux/bitmap.h> It seems you need to take gpio/fixes branch as a base. ... > #define PCA_INT BIT(8) > #define PCA_PCAL BIT(9) > +#define MAX_PWM BIT(10) Use same prefix. ... > +#define PWM_MAX_COUNT 16 > +#define PWM_PER_REG 2 > +#define PWM_BITS_PER_REG (8 / PWM_PER_REG) Can we simple put 4 here? ... > +#define PWM_INTENSITY_MASK GENMASK(PWM_BITS_PER_REG - 1, 0) Please use plain numbers for the GENMASK() arguments. ... > +struct max7313_pwm_data { > + struct gpio_desc *desc; > +}; Are you plan to extend this? Can we directly use struct gpio_desc pointer? ... > + if (PCA_CHIP_TYPE(chip->driver_data) == PCA953X_TYPE && > + chip->driver_data & MAX_PWM) { Can't we simple check only for a flag for now? > + if (reg >= MAX7313_MASTER && > + reg < (MAX7313_INTENSITY + bank_sz)) > + return true; > + } ... > + 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; > + } This is a duplicate from above. Need a helper? ... > +/* > + * Max7313 PWM specific methods > + * > + * Limitations: > + * - Does not support a disabled state > + * - Period fixed to 31.25ms > + * - Only supports normal polarity > + * - Some glitches cannot be prevented > + */ Can we have below in a separate file and attach it to the gpio-pca953x code iff CONFIG_PWM != n? ... > + mutex_lock(&pca_chip->i2c_lock); > + regmap_read(pca_chip->regmap, reg, &val); No error check? > + mutex_unlock(&pca_chip->i2c_lock); ... > + if (shift) Redundant. > + val >>= shift; ... > + mutex_lock(&pca_chip->i2c_lock); > + regmap_read(pca_chip->regmap, reg, &output); > + mutex_unlock(&pca_chip->i2c_lock); No error check? ... > + mutex_lock(&pca_chip->i2c_lock); > + regmap_read(pca_chip->regmap, reg, &output); > + mutex_unlock(&pca_chip->i2c_lock); No error check? ... > +static int max7313_pwm_request(struct pwm_chip *chip, > + struct pwm_device *pwm) > +{ > + struct max7313_pwm *max_pwm = to_max7313_pwm(chip); > + struct pca953x_chip *pca_chip = to_pca953x(max_pwm); > + struct max7313_pwm_data *data; > + struct gpio_desc *desc; > + > + desc = gpiochip_request_own_desc(&pca_chip->gpio_chip, pwm->hwpwm, > + "max7313-pwm", GPIO_ACTIVE_HIGH, 0); > + if (IS_ERR(desc)) { > + dev_err(&pca_chip->client->dev, Can't we get to struct device easily? If it's possible maybe we could move next line to this one? > + "pin already in use (probably as GPIO): %ld\n", > + PTR_ERR(desc)); > + return PTR_ERR(desc); > + } > + return 0; > +} ... > + if (intensity) > + set_bit(pwm->hwpwm, max_pwm->active_pwm); > + else > + clear_bit(pwm->hwpwm, max_pwm->active_pwm); assign_bit() By the way, do you really need it to be atomic? Perhaps __asign_bit()? ... > + active = bitmap_weight(max_pwm->active_pwm, PWM_MAX_COUNT); > + if (!active) In this case more readable will be active == 0 since you compare this to the exact value. > + ret = max7313_pwm_set_master_intensity(pca_chip, 0); > + else if (active == 1) > + ret = max7313_pwm_set_master_intensity(pca_chip, > + PWM_INTENSITY_MASK); ... > + if (IS_ENABLED(CONFIG_PWM)) { I'm not sure it eliminates all PWM related callbacks. > + ret = max7313_pwm_probe(&client->dev, chip); > + if (ret) { > + dev_err(&client->dev, "pwm probe failed, %d\n", ret); > + return ret; > + } > + } -- With Best Regards, Andy Shevchenko
Hi Andy, > > #define PCA_INT BIT(8) > > #define PCA_PCAL BIT(9) > > > +#define MAX_PWM BIT(10) > > Use same prefix. I am not sure it is relevant here, I think showing the specificity of the MAXIM PWM is okay. > > ... > > > +#define PWM_MAX_COUNT 16 > > +#define PWM_PER_REG 2 > > > +#define PWM_BITS_PER_REG (8 / PWM_PER_REG) > > Can we simple put 4 here? > Fine > ... > > > +#define PWM_INTENSITY_MASK GENMASK(PWM_BITS_PER_REG - 1, 0) > > Please use plain numbers for the GENMASK() arguments. Ok > > ... > > > +struct max7313_pwm_data { > > + struct gpio_desc *desc; > > +}; > > Are you plan to extend this? Can we directly use struct gpio_desc pointer? I'm not a fan of this method at all, I think it is better practice to keep a container in this case, which can be easily extended when needed. > > ... > > > + if (PCA_CHIP_TYPE(chip->driver_data) == PCA953X_TYPE && > > + chip->driver_data & MAX_PWM) { > > Can't we simple check only for a flag for now? I don't get it. You just want the driver_data & MAX_PWM check? > > > + if (reg >= MAX7313_MASTER && > > + reg < (MAX7313_INTENSITY + bank_sz)) > > + return true; > > + } > > ... > > > + 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; > > + } > > This is a duplicate from above. Need a helper? Perhaps! > > ... > > > +/* > > + * Max7313 PWM specific methods > > + * > > + * Limitations: > > + * - Does not support a disabled state > > + * - Period fixed to 31.25ms > > + * - Only supports normal polarity > > + * - Some glitches cannot be prevented > > + */ > > Can we have below in a separate file and attach it to the gpio-pca953x > code iff CONFIG_PWM != n? I'll check, why not. > > ... > > > + mutex_lock(&pca_chip->i2c_lock); > > > + regmap_read(pca_chip->regmap, reg, &val); > > No error check? > > > + mutex_unlock(&pca_chip->i2c_lock); > > ... > > > + if (shift) > > Redundant. Ok > > > + val >>= shift; > > ... > > > + mutex_lock(&pca_chip->i2c_lock); > > + regmap_read(pca_chip->regmap, reg, &output); > > + mutex_unlock(&pca_chip->i2c_lock); > > No error check? > > ... > > > + mutex_lock(&pca_chip->i2c_lock); > > + regmap_read(pca_chip->regmap, reg, &output); > > + mutex_unlock(&pca_chip->i2c_lock); > > No error check? > > ... > > > +static int max7313_pwm_request(struct pwm_chip *chip, > > + struct pwm_device *pwm) > > +{ > > + struct max7313_pwm *max_pwm = to_max7313_pwm(chip); > > + struct pca953x_chip *pca_chip = to_pca953x(max_pwm); > > + struct max7313_pwm_data *data; > > + struct gpio_desc *desc; > > + > > + desc = gpiochip_request_own_desc(&pca_chip->gpio_chip, pwm->hwpwm, > > + "max7313-pwm", GPIO_ACTIVE_HIGH, 0); > > + if (IS_ERR(desc)) { > > > + dev_err(&pca_chip->client->dev, > > Can't we get to struct device easily? > If it's possible maybe we could move next line to this one? I'll try. > > > + "pin already in use (probably as GPIO): %ld\n", > > + PTR_ERR(desc)); > > + return PTR_ERR(desc); > > + } > > > + return 0; > > +} > > ... > > > + if (intensity) > > + set_bit(pwm->hwpwm, max_pwm->active_pwm); > > + else > > + clear_bit(pwm->hwpwm, max_pwm->active_pwm); > > assign_bit() Nice! > > By the way, do you really need it to be atomic? Perhaps __asign_bit()? Maybe not, indeed. > > ... > > > + active = bitmap_weight(max_pwm->active_pwm, PWM_MAX_COUNT); > > > + if (!active) > > In this case more readable will be active == 0 since you compare this > to the exact value. > "if (!active)" is read "if not active" which is IMHO very descriptive! I'll correct most of your comments and send a v5. Thanks, Miquèl
Hi Andy, > > +/* > > + * Max7313 PWM specific methods > > + * > > + * Limitations: > > + * - Does not support a disabled state > > + * - Period fixed to 31.25ms > > + * - Only supports normal polarity > > + * - Some glitches cannot be prevented > > + */ > > Can we have below in a separate file and attach it to the gpio-pca953x > code iff CONFIG_PWM != n? I tried to do it but there are too many functions from the PCA driver that would be called in the PWM annex. This leads to hiding generic parts of the driver which have nothing to do with the PWM in a header file, which I don't think is wise. I prefer not to do it. Thanks for your understanding, Miquèl
On Mon, Dec 16, 2019 at 10:51 PM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Fri, Nov 29, 2019 at 9:13 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. > > Thanks for an update! > > Still I think it's wrong approach. What should be done is: > - adding a pin configuration type of PWM (when, for example, argument > defines duty cycle and period) > - conversion to pin control of this driver > - enabling pin configuration PWM for it. > > For now it looks like a custom way of doing it. > If GPIO maintainers are okay with it, I'll not object, just want to > have than something like TODO updated for the matter. Yeah well that is a possible way, it pretty much lies with the PWM maintainer, I have one guiding stanza "rough consensus and running code". Making big upfront code conversions just to get a small piece of hardware going is just too much from me as subsystem maintainer, a dual sub-system driver is perfectly fine in my opinion. That said contributors are encouraged to extend scope and be ambitious and set precedents for others to follow by going the extra mile. (That sounds like corporate management!) But that must be voluntary work outside the scope of just hardware enablement. Yours, Linus Walleij
diff --git a/drivers/gpio/gpio-pca953x.c b/drivers/gpio/gpio-pca953x.c index de5d1383f28d..347988bc630f 100644 --- a/drivers/gpio/gpio-pca953x.c +++ b/drivers/gpio/gpio-pca953x.c @@ -10,20 +10,25 @@ #include <linux/acpi.h> #include <linux/bits.h> +#include <linux/bitmap.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 +68,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 +105,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 +130,16 @@ MODULE_DEVICE_TABLE(acpi, pca953x_acpi_ids); #define NBANK(chip) DIV_ROUND_UP(chip->gpio_chip.ngpio, BANK_SZ) +#define PWM_MAX_COUNT 16 +#define PWM_PER_REG 2 +#define PWM_BITS_PER_REG (8 / PWM_PER_REG) +#define PWM_MASTER_INTENSITY_SHIFT 4 +#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 +161,20 @@ static const struct pca953x_reg_config pca957x_regs = { .invert = PCA957X_INVRT, }; +struct max7313_pwm_data { + struct gpio_desc *desc; +}; + +struct max7313_pwm { + struct pwm_chip chip; + /* + * Protect races when counting active PWMs for enabling or disabling + * the internal oscillator. + */ + struct mutex count_lock; + DECLARE_BITMAP(active_pwm, PWM_MAX_COUNT); +}; + struct pca953x_chip { unsigned gpio_start; struct mutex i2c_lock; @@ -161,6 +197,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 +279,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 +310,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 +940,315 @@ static int device_pca957x_init(struct pca953x_chip *chip, u32 invert) return ret; } +/* + * Max7313 PWM specific methods + * + * Limitations: + * - Does not support a disabled state + * - Period fixed to 31.25ms + * - Only supports normal polarity + * - Some glitches cannot be prevented + */ + +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 *pca_chip, + unsigned int idx) +{ + unsigned int reg, shift, val, output; + u8 intensity; + bool phase; + + /* Retrieve the intensity */ + reg = MAX7313_INTENSITY + (idx / PWM_PER_REG); + shift = (idx % PWM_PER_REG) ? PWM_BITS_PER_REG : 0; + + mutex_lock(&pca_chip->i2c_lock); + regmap_read(pca_chip->regmap, reg, &val); + mutex_unlock(&pca_chip->i2c_lock); + + if (shift) + val >>= shift; + + val &= PWM_INTENSITY_MASK; + + /* Retrieve the phase */ + reg = pca953x_recalc_addr(pca_chip, pca_chip->regs->output, idx, 0, 0); + + mutex_lock(&pca_chip->i2c_lock); + regmap_read(pca_chip->regmap, reg, &output); + mutex_unlock(&pca_chip->i2c_lock); + + phase = output & BIT(idx % BANK_SZ); + + /* + * Register values in the [0;15] range mean a value in the [1/16;16/16] + * range if the phase is set, a [15/16;0/16] range otherwise. + */ + if (phase) + intensity = val + 1; + else + intensity = PWM_INTENSITY_MASK - val; + + return intensity; +} + +static int max7313_pwm_set_intensity(struct pca953x_chip *pca_chip, + unsigned int idx, u8 intensity) +{ + 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 & PWM_INTENSITY_MASK) << shift; + + mutex_lock(&pca_chip->i2c_lock); + ret = regmap_write_bits(pca_chip->regmap, reg, mask, val); + mutex_unlock(&pca_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 switch the blink phase only when mandatory + * because there is no way to atomically flip the register *and* change the PWM + * value at the same time so, in this case, it will produce a small glitch. + */ +static int max7313_pwm_set_state(struct pca953x_chip *pca_chip, + struct pwm_device *pwm, + unsigned int intensity) +{ + struct max7313_pwm_data *data = pwm_get_chip_data(pwm); + struct gpio_desc *desc = data->desc; + unsigned int idx = pwm->hwpwm, reg, output; + bool phase; + int ret; + + /* Retrieve the phase */ + reg = pca953x_recalc_addr(pca_chip, pca_chip->regs->output, idx, 0, 0); + + mutex_lock(&pca_chip->i2c_lock); + regmap_read(pca_chip->regmap, reg, &output); + mutex_unlock(&pca_chip->i2c_lock); + + phase = output & BIT(idx % BANK_SZ); + + /* Need to blink the phase */ + if ((phase && !intensity) || (!phase && intensity == PWM_DC_STATES)) { + phase = !phase; + ret = gpiod_direction_output(desc, phase); + if (ret) + return ret; + } else { + /* Ensure the pin is in output state (default might be input) */ + ret = gpiod_direction_output(desc, phase); + if (ret) + return ret; + } + + if (phase) + intensity = intensity - 1; + else + intensity = PWM_INTENSITY_MASK - intensity; + + return max7313_pwm_set_intensity(pca_chip, idx, intensity); +} + +static int max7313_pwm_set_master_intensity(struct pca953x_chip *pca_chip, + u8 intensity) +{ + int ret; + + intensity &= PWM_INTENSITY_MASK; + + mutex_lock(&pca_chip->i2c_lock); + ret = regmap_write_bits(pca_chip->regmap, MAX7313_MASTER, + PWM_INTENSITY_MASK << PWM_MASTER_INTENSITY_SHIFT, + intensity << PWM_MASTER_INTENSITY_SHIFT); + mutex_unlock(&pca_chip->i2c_lock); + + return ret; +} + +static int max7313_pwm_request(struct pwm_chip *chip, + struct pwm_device *pwm) +{ + struct max7313_pwm *max_pwm = to_max7313_pwm(chip); + struct pca953x_chip *pca_chip = to_pca953x(max_pwm); + struct max7313_pwm_data *data; + struct gpio_desc *desc; + + desc = gpiochip_request_own_desc(&pca_chip->gpio_chip, pwm->hwpwm, + "max7313-pwm", GPIO_ACTIVE_HIGH, 0); + if (IS_ERR(desc)) { + dev_err(&pca_chip->client->dev, + "pin already in use (probably as GPIO): %ld\n", + PTR_ERR(desc)); + return PTR_ERR(desc); + } + + data = kzalloc(sizeof(*data), GFP_KERNEL); + if (!data) { + gpiochip_free_own_desc(desc); + return -ENOMEM; + } + + data->desc = desc; + pwm_set_chip_data(pwm, data); + + return 0; +} + +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 || + state->polarity != PWM_POLARITY_NORMAL) + return -EINVAL; + + /* Convert the duty-cycle to be in the [0;16] range */ + intensity = DIV_ROUND_DOWN_ULL(state->duty_cycle, + state->period / PWM_DC_STATES); + + /* + * If this is the first PWM to enable, set the master intensity to the + * maximum level to let individual outputs the greatest flexibility + * range. It also enables the internal oscillator. + * + * When shutting down the last active PWM, the oscillator is disabled. + * + * Lazy logic is applied to simplify the code: always enable the + * oscillator when there is 1 active pwm, always disable it when there + * is none. + */ + mutex_lock(&max_pwm->count_lock); + if (intensity) + set_bit(pwm->hwpwm, max_pwm->active_pwm); + else + clear_bit(pwm->hwpwm, max_pwm->active_pwm); + + active = bitmap_weight(max_pwm->active_pwm, PWM_MAX_COUNT); + if (!active) + ret = max7313_pwm_set_master_intensity(pca_chip, 0); + else if (active == 1) + ret = max7313_pwm_set_master_intensity(pca_chip, + PWM_INTENSITY_MASK); + mutex_unlock(&max_pwm->count_lock); + + if (ret) + return ret; + + /* + * The hardware is supposedly glitch-free when changing the intensity, + * unless we need to flip the blink phase to reach an extremity or the + * other of the spectre (0/16 from phase 1, 16/16 from phase 0). + */ + return max7313_pwm_set_state(pca_chip, pwm, intensity); +} + +static void max7313_pwm_get_state(struct pwm_chip *chip, + struct pwm_device *pwm, + struct pwm_state *state) +{ + struct max7313_pwm *max_pwm = to_max7313_pwm(chip); + struct pca953x_chip *pca_chip = to_pca953x(max_pwm); + u8 intensity; + + state->enabled = true; + state->period = PWM_PERIOD_NS; + state->polarity = PWM_POLARITY_NORMAL; + + intensity = max7313_pwm_get_intensity(pca_chip, pwm->hwpwm); + state->duty_cycle = intensity * PWM_OFFSET_NS; +}; + +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 *pca_chip) +{ + struct max7313_pwm *max_pwm = &pca_chip->pwm; + struct pwm_chip *chip = &max_pwm->chip; + int ret, i; + + if (!(pca_chip->driver_data & MAX_PWM)) + return 0; + + chip->dev = dev; + chip->ops = &max7313_pwm_ops; + chip->npwm = pca_chip->gpio_chip.ngpio; + chip->base = -1; + + /* Disable global control (does not affect GPIO functionality) */ + mutex_lock(&pca_chip->i2c_lock); + ret = regmap_write_bits(pca_chip->regmap, MAX7313_CONFIGURATION, + MAX7313_GLOB_INTENSITY, 0); + mutex_unlock(&pca_chip->i2c_lock); + if (ret) + return ret; + + mutex_init(&max_pwm->count_lock); + bitmap_zero(max_pwm->active_pwm, PWM_MAX_COUNT); + + /* Count currently active PWM */ + mutex_lock(&max_pwm->count_lock); + for (i = 0; i < chip->npwm; i++) { + if (max7313_pwm_get_intensity(pca_chip, i)) + set_bit(i, max_pwm->active_pwm); + } + + if (bitmap_weight(max_pwm->active_pwm, PWM_MAX_COUNT)) + ret = max7313_pwm_set_master_intensity(pca_chip, + PWM_INTENSITY_MASK); + + mutex_unlock(&max_pwm->count_lock); + + if (ret) + return ret; + + return pwmchip_add(chip); +} + static const struct of_device_id pca953x_dt_ids[]; static int pca953x_probe(struct i2c_client *client, @@ -1018,6 +1381,14 @@ static int pca953x_probe(struct i2c_client *client, dev_warn(&client->dev, "setup failed, %d\n", ret); } + if (IS_ENABLED(CONFIG_PWM)) { + ret = max7313_pwm_probe(&client->dev, chip); + if (ret) { + dev_err(&client->dev, "pwm probe failed, %d\n", ret); + return ret; + } + } + return 0; err_exit: @@ -1162,7 +1533,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), },
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 v4: * Fix wrong comment about register value. * Rewrite ->set_state() to make it more readable, include the fact that the phase may blink and to limit the number of blink changes when possible ("lazy switching" as discussed with Uwe). * Prevent using managed memory when not relevant. * Add a definition to the master intensity shift. * Rename all struct pwm_device to pwm and all struct pwm_chip as chip. Then, struct pca953x_chip are called pca_chip instead of chip and struct max7313_pwm are called max_pwm intead of pwm. * Enhance the comment about glitch-free hardware. * Add a plain error check at the ->pwm_probe() return location. * Rename duty_cycle to intensity when relevant. * Do not initialize the PWM in ->request(). Also do not change the state in ->free(). * New way to count enabled/disabled PWM (with a bitmap). Disable the oscillator only when 0 PWM are in use, enable it when there is one. Also always set the pin to output state otherwise the default might be input. * Force state->enable to be true and drop all the boilerplate around enable and .duty_cycle. 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 | 375 +++++++++++++++++++++++++++++++++++- 1 file changed, 373 insertions(+), 2 deletions(-)