| Message ID | 20191014124803.13661-1-miquel.raynal@bootlin.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | gpio: pca953x: Add Maxim MAX7313 PWM support | expand |
On Mon, Oct 14, 2019 at 02:48:03PM +0200, 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> > --- > drivers/gpio/gpio-pca953x.c | 249 +++++++++++++++++++++++++++++++++++- > 1 file changed, 247 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpio/gpio-pca953x.c b/drivers/gpio/gpio-pca953x.c > index de5d1383f28d..16b5a991b32e 100644 > --- a/drivers/gpio/gpio-pca953x.c > +++ b/drivers/gpio/gpio-pca953x.c > @@ -18,12 +18,15 @@ > #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 +66,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 +103,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 +128,16 @@ MODULE_DEVICE_TABLE(acpi, pca953x_acpi_ids); > > #define NBANK(chip) DIV_ROUND_UP(chip->gpio_chip.ngpio, BANK_SZ) > > +#define PWM_CHANNELS 16 Does it really make sense to hard-code this? During PWM chip registration you assume that the number of PWM channels is the same as the number of GPIO lines, so the hard-coded values here are only going to work as long as the chip has 16 GPIO lines. Why not just parameterize all the code based on the number of GPIO lines? > +#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_BANK_SZ (PWM_CHANNELS / PWM_PER_REG) > + > +#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 +159,12 @@ static const struct pca953x_reg_config pca957x_regs = { > .invert = PCA957X_INVRT, > }; > > +struct max7313_pwm { > + struct pwm_chip chip; > + struct gpio_desc *gpiods[PWM_CHANNELS]; Have you considered using per pwm_device data to store the GPIO descriptors? That avoids having to index some global variable with the PWM index and allows you to directly get at the per-PWM channel data. > + unsigned int used; > +}; > + > struct pca953x_chip { > unsigned gpio_start; > struct mutex i2c_lock; > @@ -161,6 +187,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) > @@ -243,6 +271,13 @@ static bool pca953x_readable_register(struct device *dev, unsigned int reg) > struct pca953x_chip *chip = dev_get_drvdata(dev); > u32 bank; > > + if (PCA_CHIP_TYPE(chip->driver_data) == PCA953X_TYPE && > + chip->driver_data & MAX_PWM) { > + if (reg >= MAX7313_MASTER && > + reg < (MAX7313_INTENSITY + PWM_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; > @@ -266,6 +301,13 @@ static bool pca953x_writeable_register(struct device *dev, unsigned int reg) > struct pca953x_chip *chip = dev_get_drvdata(dev); > u32 bank; > > + if (PCA_CHIP_TYPE(chip->driver_data) == PCA953X_TYPE && > + chip->driver_data & MAX_PWM) { > + if (reg >= MAX7313_MASTER && > + reg < (MAX7313_INTENSITY + PWM_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 +928,206 @@ 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 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, off; off is really not a good abbreviation because it has a different meaning. That means you have to go and actually look at the definition to find out what value it contains. I think it's clearer to name this "offset" or "shift". > + u8 val, mask; > + int ret; > + > + reg = MAX7313_INTENSITY + (idx / PWM_PER_REG); > + off = (idx % PWM_PER_REG) ? PWM_BITS_PER_REG : 0; > + > + mask = PWM_INTENSITY_MASK << off; > + val = intensity << off; > + > + mutex_lock(&chip->i2c_lock); > + ret = regmap_write_bits(chip->regmap, reg, mask, val); > + mutex_unlock(&chip->i2c_lock); > + > + return ret; > +} > + > +static int max7313_pwm_disable(struct pca953x_chip *chip, unsigned int idx) > +{ > + return pca953x_gpio_direction_input(&chip->gpio_chip, idx); > +} Does this really do what is expected? In my experience setting a GPIO as input will typically make it high-impedance, which usually means it'll be seen as "high". That's not what a disabled PWM is supposed to be. Also, you've already requested the GPIO, why not continue using the GPIO as a regular consumer? Seems like that would make all of this a little more idiomatic. > + > +static int max7313_pwm_enable_static_low(struct pca953x_chip *chip, > + unsigned int idx) > +{ > + /* > + * 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 operation will produce a small glitch. > + */ > + > + /* Invert the the PWM phase */ > + pca953x_gpio_direction_output(&chip->gpio_chip, idx, 0); > + > + /* Maximize the low time */ > + return max7313_pwm_set_intensity(chip, idx, PWM_DC_STATES); > +} > + > +static int max7313_pwm_enable(struct pca953x_chip *chip, unsigned int idx, > + unsigned int duty_cycle) > +{ > + /* PWM phase must not be inverted to work in the [1/16;16/16] range */ > + pca953x_gpio_direction_output(&chip->gpio_chip, idx, 1); > + > + /* Set the PWM intensity to the desired duty-cycle */ > + return max7313_pwm_set_intensity(chip, idx, 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); > + int idx = pwm_device->hwpwm; unsigned int to mirror the type of hwpwm. > + struct gpio_desc *desc; > + int ret; > + > + if (pwm->gpiods[idx]) { > + ret = -EBUSY; > + } else { > + desc = gpio_to_desc(chip->gpio_chip.base + idx); > + if (!desc) > + return -ENODEV; > + > + ret = gpiod_request(desc, "max7313-pwm"); > + if (ret) > + return ret; > + > + pwm->gpiods[idx] = desc; Can't you do this using gpiochip_request_own_desc()? Seems rather odd to go through the global number space to get back at a "local" GPIO descriptor. > + } > + > + ret = max7313_pwm_disable(chip, idx); > + if (ret) > + return ret; ->request() shouldn't change the state of the PWM. > + > + /* > + * Set master intensity to the maximum level to let individual outputs > + * the greatest flexibility range. Also enables the internal oscillator. > + */ > + if (!pwm->used) { > + 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) > + return ret; > + } > + > + pwm->used++; > + > + return 0; > +} > + > +static void max7313_pwm_free(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); > + int idx = pwm_device->hwpwm; unsigned int, please. > + > + max7313_pwm_disable(chip, idx); > + pwm->used--; > + > + /* Disable the internal oscillator if no channel is in use */ > + if (!pwm->used) { > + 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); > + } > + > + gpiod_free(pwm->gpiods[idx]); > + pwm->gpiods[idx] = NULL; > +} > + > +static int max7313_pwm_apply(struct pwm_chip *pwm_chip, > + struct pwm_device *pwm_device, > + const struct pwm_state *state) > +{ > + struct max7313_pwm *pwm = to_max7313_pwm(pwm_chip); > + struct pca953x_chip *chip = to_pca953x(pwm); > + int idx = pwm_device->hwpwm; unsigned int, please. > + unsigned int duty_cycle; > + > + if (!state->enabled) > + return max7313_pwm_disable(chip, idx); > + > + if (!state->period || !state->duty_cycle) > + return max7313_pwm_enable_static_low(chip, idx); > + > + /* Convert the duty-cycle to be in the [1;16] range */ > + duty_cycle = DIV_ROUND_UP(state->duty_cycle * PWM_DC_STATES, > + state->period); > + > + return max7313_pwm_enable(chip, idx, duty_cycle); > +} > + > +static const struct pwm_ops max7313_pwm_ops = { > + .request = max7313_pwm_request, > + .free = max7313_pwm_free, > + .apply = max7313_pwm_apply, > + .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 */ > + 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; > + > + ret = pwmchip_add(pwm_chip); > + > + return ret; Just "return pwmchip_add(...);" will do. Thierry > +} > + > static const struct of_device_id pca953x_dt_ids[]; > > static int pca953x_probe(struct i2c_client *client, > @@ -1018,6 +1260,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 +1407,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 >
On Mon, Oct 14, 2019 at 4:09 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. Can we rather not contaminate driver with this? Just register a separate PWM driver and export its functionality to GPIO, or other way around (in the latter case we actually have PCA8685 which provides a GPIO fgunctionality).
Hi Andy, Thanks for the feedback. Andy Shevchenko <andy.shevchenko@gmail.com> wrote on Mon, 14 Oct 2019 20:59:01 +0300: > On Mon, Oct 14, 2019 at 4:09 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. > > Can we rather not contaminate driver with this? > > Just register a separate PWM driver and export its functionality to > GPIO, or other way around (in the latter case we actually have PCA8685 > which provides a GPIO fgunctionality). > I understand your concern but I am not sure to understand which solution you have in mind. In the former case, could you explain a bit more what you are thinking about? Would linking the PWM support with the GPIO driver (code would be located in another .c file) work for you? Or maybe you would prefer an MFD on top of the GPIO driver? As for the later case, I am not willing to re-implement GPIO support in an alternate driver for an already supported chip, it is too much work for the time I can spend on it. Thanks, Miquèl
On Tue, Oct 15, 2019 at 5:30 PM Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > Hi Andy, > > Thanks for the feedback. > > Andy Shevchenko <andy.shevchenko@gmail.com> wrote on Mon, 14 Oct 2019 > 20:59:01 +0300: > > > On Mon, Oct 14, 2019 at 4:09 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. > > > > Can we rather not contaminate driver with this? > > > > Just register a separate PWM driver and export its functionality to > > GPIO, or other way around (in the latter case we actually have PCA8685 > > which provides a GPIO fgunctionality). > > > > I understand your concern but I am not sure to understand which > solution you have in mind. In the former case, could you explain a bit > more what you are thinking about? Would linking the PWM support with > the GPIO driver (code would be located in another .c file) work for > you? Or maybe you would prefer an MFD on top of the GPIO driver? > > As for the later case, I am not willing to re-implement GPIO support in > an alternate driver for an already supported chip, it is too much work > for the time I can spend on it. drivers/pwm/pwm-max7313.c: probe(platform_device) { struct regmap = pdata; ... } --- 8< --- 8< --- drivers/gpio/gpio-pca953x.c: probe() { struct regmap rm; ... if (dev_has_pwm) pca953x_register_pwm_driver(rm); ... } In the above regmap may be replaced with some (shared) container. Or other way around. PWM registers GPIO (which actually I prefer since we have PCA9685 case where PWM provides GPIO functionality, though via different means)
Hi Linus, Andy Shevchenko <andy.shevchenko@gmail.com> wrote on Tue, 15 Oct 2019 17:55:33 +0300: > On Tue, Oct 15, 2019 at 5:30 PM Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > > > Hi Andy, > > > > Thanks for the feedback. > > > > Andy Shevchenko <andy.shevchenko@gmail.com> wrote on Mon, 14 Oct 2019 > > 20:59:01 +0300: > > > > > On Mon, Oct 14, 2019 at 4:09 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. > > > > > > Can we rather not contaminate driver with this? > > > > > > Just register a separate PWM driver and export its functionality to > > > GPIO, or other way around (in the latter case we actually have PCA8685 > > > which provides a GPIO fgunctionality). > > > > > > > I understand your concern but I am not sure to understand which > > solution you have in mind. In the former case, could you explain a bit > > more what you are thinking about? Would linking the PWM support with > > the GPIO driver (code would be located in another .c file) work for > > you? Or maybe you would prefer an MFD on top of the GPIO driver? > > > > As for the later case, I am not willing to re-implement GPIO support in > > an alternate driver for an already supported chip, it is too much work > > for the time I can spend on it. > > > drivers/pwm/pwm-max7313.c: > > probe(platform_device) > { > struct regmap = pdata; > ... > } > > --- 8< --- 8< --- > drivers/gpio/gpio-pca953x.c: > > probe() > { > struct regmap rm; > ... > if (dev_has_pwm) > pca953x_register_pwm_driver(rm); > ... > } > > In the above regmap may be replaced with some (shared) container. > > Or other way around. PWM registers GPIO (which actually I prefer since > we have PCA9685 case where PWM provides GPIO functionality, though via > different means) > Can I have your input on this proposal? On one hand I agree that the GPIO driver is already quite big due to its genericity and the amount of controllers it supports, on the other hand: 1/ Registering a PWM driver from the GPIO core seems strange. Maybe registering a platform device could do the trick but I am not convinced it is worth the trouble. 2/ Putting the PWM logic in the drivers/pwm/ directory is not very convenient as the object file will have to be embedded within the GPIO one; this line in drivers/gpio/Makefile would be horrible: ... += gpio-pca953x.o ../pwm/pwm-max7313.o (not even sure it works) 3/ In any cases, the regmap's ->readable_reg(), ->writable_reg() callbacks shall be tweaked to turn the PWM registers accessible, so we would still have PWM related code in the PCA953x GPIO driver. In the end, I wonder if keeping everything in one file is not better? Eventually I can create a separate file and fill it with just the PWM helpers/hooks. Please advise on the better solution for you, I'll wait your feedback before addressing Thierry Reding's other review and resubmit. Thanks, Miquèl
pon., 4 lis 2019 o 16:11 Miquel Raynal <miquel.raynal@bootlin.com> napisał(a): > > Hi Linus, > > Andy Shevchenko <andy.shevchenko@gmail.com> wrote on Tue, 15 Oct 2019 > 17:55:33 +0300: > > > On Tue, Oct 15, 2019 at 5:30 PM Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > > > > > Hi Andy, > > > > > > Thanks for the feedback. > > > > > > Andy Shevchenko <andy.shevchenko@gmail.com> wrote on Mon, 14 Oct 2019 > > > 20:59:01 +0300: > > > > > > > On Mon, Oct 14, 2019 at 4:09 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. > > > > > > > > Can we rather not contaminate driver with this? > > > > > > > > Just register a separate PWM driver and export its functionality to > > > > GPIO, or other way around (in the latter case we actually have PCA8685 > > > > which provides a GPIO fgunctionality). > > > > > > > > > > I understand your concern but I am not sure to understand which > > > solution you have in mind. In the former case, could you explain a bit > > > more what you are thinking about? Would linking the PWM support with > > > the GPIO driver (code would be located in another .c file) work for > > > you? Or maybe you would prefer an MFD on top of the GPIO driver? > > > > > > As for the later case, I am not willing to re-implement GPIO support in > > > an alternate driver for an already supported chip, it is too much work > > > for the time I can spend on it. > > > > > > drivers/pwm/pwm-max7313.c: > > > > probe(platform_device) > > { > > struct regmap = pdata; > > ... > > } > > > > --- 8< --- 8< --- > > drivers/gpio/gpio-pca953x.c: > > > > probe() > > { > > struct regmap rm; > > ... > > if (dev_has_pwm) > > pca953x_register_pwm_driver(rm); > > ... > > } > > > > In the above regmap may be replaced with some (shared) container. > > > > Or other way around. PWM registers GPIO (which actually I prefer since > > we have PCA9685 case where PWM provides GPIO functionality, though via > > different means) > > > > Can I have your input on this proposal? > > On one hand I agree that the GPIO driver is already quite big due to > its genericity and the amount of controllers it supports, on the other > hand: > 1/ Registering a PWM driver from the GPIO core seems strange. Maybe > registering a platform device could do the trick but I am not convinced > it is worth the trouble. > 2/ Putting the PWM logic in the drivers/pwm/ directory is not very > convenient as the object file will have to be embedded within the GPIO > one; this line in drivers/gpio/Makefile would be horrible: > ... += gpio-pca953x.o ../pwm/pwm-max7313.o (not even sure it works) > 3/ In any cases, the regmap's ->readable_reg(), ->writable_reg() > callbacks shall be tweaked to turn the PWM registers accessible, so we > would still have PWM related code in the PCA953x GPIO driver. > > In the end, I wonder if keeping everything in one file is not better? > Eventually I can create a separate file and fill it with just the PWM > helpers/hooks. Please advise on the better solution for you, I'll wait > your feedback before addressing Thierry Reding's other review and > resubmit. > I'm not sure if this has been discussed, but is it possible to create an MFD driver for this chip and conditionally plug in the GPIO part from pca953x? I don't like the idea of having PWM functionality in a GPIO driver either. Bart > > Thanks, > Miquèl
Hello, On Mon, Nov 04, 2019 at 04:32:23PM +0100, Bartosz Golaszewski wrote: > pon., 4 lis 2019 o 16:11 Miquel Raynal <miquel.raynal@bootlin.com> napisał(a): > > Andy Shevchenko <andy.shevchenko@gmail.com> wrote on Tue, 15 Oct 2019 > > 17:55:33 +0300: > > > > > Or other way around. PWM registers GPIO (which actually I prefer since > > > we have PCA9685 case where PWM provides GPIO functionality, though via > > > different means) > > > > > > > Can I have your input on this proposal? > > > > On one hand I agree that the GPIO driver is already quite big due to > > its genericity and the amount of controllers it supports, on the other > > hand: > > 1/ Registering a PWM driver from the GPIO core seems strange. Maybe > > registering a platform device could do the trick but I am not convinced > > it is worth the trouble. > > 2/ Putting the PWM logic in the drivers/pwm/ directory is not very > > convenient as the object file will have to be embedded within the GPIO > > one; this line in drivers/gpio/Makefile would be horrible: > > ... += gpio-pca953x.o ../pwm/pwm-max7313.o (not even sure it works) > > 3/ In any cases, the regmap's ->readable_reg(), ->writable_reg() > > callbacks shall be tweaked to turn the PWM registers accessible, so we > > would still have PWM related code in the PCA953x GPIO driver. > > > > In the end, I wonder if keeping everything in one file is not better? > > Eventually I can create a separate file and fill it with just the PWM > > helpers/hooks. Please advise on the better solution for you, I'll wait > > your feedback before addressing Thierry Reding's other review and > > resubmit. > > > > I'm not sure if this has been discussed, but is it possible to create > an MFD driver for this chip and conditionally plug in the GPIO part > from pca953x? I don't like the idea of having PWM functionality in a > GPIO driver either. I didn't check the manual or driver in depth, but I guess it doesn't match the MFD abstraction well. (That is, the PWM and GPIO parts live in different address areas of the chip and can be used independently of each other.) While it's not nice to have a driver that provides two different devices (here: gpio controller and pwm controller) similar things are not unseen. And for example the splitting of watchdog (drivers/watchdog/stmp3xxx_rtc_wdt.c) and rtc (drivers/rtc/rtc-stmp3xxx.c) of the device in the mx28 is more trouble than worth. So I'd vote for putting it in a single file that lives where the bigger/more complex part fits to. So assuming that's the GPIO part (as the driver supports several variants and not all of them have a PWM function if I'm not mistaken) having it in drivers/gpio is fine for me. Best regards Uwe
On Mon, Nov 4, 2019 at 10:33 PM Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote: > On Mon, Nov 04, 2019 at 04:32:23PM +0100, Bartosz Golaszewski wrote: > > pon., 4 lis 2019 o 16:11 Miquel Raynal <miquel.raynal@bootlin.com> napisał(a): > > > Andy Shevchenko <andy.shevchenko@gmail.com> wrote on Tue, 15 Oct 2019 > > > 17:55:33 +0300: > > > > > > > Or other way around. PWM registers GPIO (which actually I prefer since > > > > we have PCA9685 case where PWM provides GPIO functionality, though via > > > > different means) > While it's not nice to have a driver that provides two different devices > (here: gpio controller and pwm controller) similar things are not > unseen. And for example the splitting of watchdog > (drivers/watchdog/stmp3xxx_rtc_wdt.c) and rtc > (drivers/rtc/rtc-stmp3xxx.c) of the device in the mx28 is more trouble > than worth. > > So I'd vote for putting it in a single file that lives where the > bigger/more complex part fits to. So assuming that's the GPIO part (as > the driver supports several variants and not all of them have a PWM > function if I'm not mistaken) having it in drivers/gpio is fine for me. For me it sounds more likely that PWM is a *pin function* of a pin controller and actually this GPIO driver should be a pin controller with corresponding function(s).
Hi Uwe, Andy Shevchenko <andy.shevchenko@gmail.com> wrote on Tue, 5 Nov 2019 09:06:37 +0200: > On Mon, Nov 4, 2019 at 10:33 PM Uwe Kleine-König > <u.kleine-koenig@pengutronix.de> wrote: > > On Mon, Nov 04, 2019 at 04:32:23PM +0100, Bartosz Golaszewski wrote: > > > pon., 4 lis 2019 o 16:11 Miquel Raynal <miquel.raynal@bootlin.com> napisał(a): > > > > Andy Shevchenko <andy.shevchenko@gmail.com> wrote on Tue, 15 Oct 2019 > > > > 17:55:33 +0300: > > > > > > > > > Or other way around. PWM registers GPIO (which actually I prefer since > > > > > we have PCA9685 case where PWM provides GPIO functionality, though via > > > > > different means) > > > While it's not nice to have a driver that provides two different devices > > (here: gpio controller and pwm controller) similar things are not > > unseen. And for example the splitting of watchdog > > (drivers/watchdog/stmp3xxx_rtc_wdt.c) and rtc > > (drivers/rtc/rtc-stmp3xxx.c) of the device in the mx28 is more trouble > > than worth. > > > > So I'd vote for putting it in a single file that lives where the > > bigger/more complex part fits to. So assuming that's the GPIO part (as > > the driver supports several variants and not all of them have a PWM > > function if I'm not mistaken) having it in drivers/gpio is fine for me. > > For me it sounds more likely that PWM is a *pin function* of a pin > controller and actually this GPIO driver should be a pin controller > with corresponding function(s). > Ok, thanks for the input, I will address Thierry's comments and re-submit as a single file (same shape as in v1). Kind regards, Miquèl
On Tue, Nov 5, 2019 at 11:02 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote: > Andy Shevchenko <andy.shevchenko@gmail.com> wrote on Tue, 5 Nov 2019 > 09:06:37 +0200: > > On Mon, Nov 4, 2019 at 10:33 PM Uwe Kleine-König > > <u.kleine-koenig@pengutronix.de> wrote: > > For me it sounds more likely that PWM is a *pin function* of a pin > > controller and actually this GPIO driver should be a pin controller > > with corresponding function(s). > > > > Ok, thanks for the input, I will address Thierry's comments and > re-submit as a single file (same shape as in v1). Perhaps I have to be more clear. What I meant is that pin control should support PWM pin function and PCA953x be converted to a pin control + GPIO + PWM. Above definitely is not what you did and it's not for immediate submission. I really would like to hear if it makes sense.
Hi Andy, Andy Shevchenko <andy.shevchenko@gmail.com> wrote on Tue, 5 Nov 2019 11:08:39 +0200: > On Tue, Nov 5, 2019 at 11:02 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > Andy Shevchenko <andy.shevchenko@gmail.com> wrote on Tue, 5 Nov 2019 > > 09:06:37 +0200: > > > On Mon, Nov 4, 2019 at 10:33 PM Uwe Kleine-König > > > <u.kleine-koenig@pengutronix.de> wrote: > > > > For me it sounds more likely that PWM is a *pin function* of a pin > > > controller and actually this GPIO driver should be a pin controller > > > with corresponding function(s). > > > > > > > Ok, thanks for the input, I will address Thierry's comments and > > re-submit as a single file (same shape as in v1). > > Perhaps I have to be more clear. What I meant is that pin control > should support PWM pin function and PCA953x be converted to a pin > control + GPIO + PWM. > Above definitely is not what you did and it's not for immediate submission. Sorry for the misunderstanding, I was answering Uwe. > I really would like to hear if it makes sense. While this would probably be the nicest and cleanest solution, I don't think it is doable in a reasonable amount of time compared to the benefits. Thanks, Miquèl
diff --git a/drivers/gpio/gpio-pca953x.c b/drivers/gpio/gpio-pca953x.c index de5d1383f28d..16b5a991b32e 100644 --- a/drivers/gpio/gpio-pca953x.c +++ b/drivers/gpio/gpio-pca953x.c @@ -18,12 +18,15 @@ #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 +66,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 +103,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 +128,16 @@ MODULE_DEVICE_TABLE(acpi, pca953x_acpi_ids); #define NBANK(chip) DIV_ROUND_UP(chip->gpio_chip.ngpio, BANK_SZ) +#define PWM_CHANNELS 16 +#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_BANK_SZ (PWM_CHANNELS / PWM_PER_REG) + +#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 +159,12 @@ static const struct pca953x_reg_config pca957x_regs = { .invert = PCA957X_INVRT, }; +struct max7313_pwm { + struct pwm_chip chip; + struct gpio_desc *gpiods[PWM_CHANNELS]; + unsigned int used; +}; + struct pca953x_chip { unsigned gpio_start; struct mutex i2c_lock; @@ -161,6 +187,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) @@ -243,6 +271,13 @@ static bool pca953x_readable_register(struct device *dev, unsigned int reg) struct pca953x_chip *chip = dev_get_drvdata(dev); u32 bank; + if (PCA_CHIP_TYPE(chip->driver_data) == PCA953X_TYPE && + chip->driver_data & MAX_PWM) { + if (reg >= MAX7313_MASTER && + reg < (MAX7313_INTENSITY + PWM_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; @@ -266,6 +301,13 @@ static bool pca953x_writeable_register(struct device *dev, unsigned int reg) struct pca953x_chip *chip = dev_get_drvdata(dev); u32 bank; + if (PCA_CHIP_TYPE(chip->driver_data) == PCA953X_TYPE && + chip->driver_data & MAX_PWM) { + if (reg >= MAX7313_MASTER && + reg < (MAX7313_INTENSITY + PWM_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 +928,206 @@ 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 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, off; + u8 val, mask; + int ret; + + reg = MAX7313_INTENSITY + (idx / PWM_PER_REG); + off = (idx % PWM_PER_REG) ? PWM_BITS_PER_REG : 0; + + mask = PWM_INTENSITY_MASK << off; + val = intensity << off; + + mutex_lock(&chip->i2c_lock); + ret = regmap_write_bits(chip->regmap, reg, mask, val); + mutex_unlock(&chip->i2c_lock); + + return ret; +} + +static int max7313_pwm_disable(struct pca953x_chip *chip, unsigned int idx) +{ + return pca953x_gpio_direction_input(&chip->gpio_chip, idx); +} + +static int max7313_pwm_enable_static_low(struct pca953x_chip *chip, + unsigned int idx) +{ + /* + * 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 operation will produce a small glitch. + */ + + /* Invert the the PWM phase */ + pca953x_gpio_direction_output(&chip->gpio_chip, idx, 0); + + /* Maximize the low time */ + return max7313_pwm_set_intensity(chip, idx, PWM_DC_STATES); +} + +static int max7313_pwm_enable(struct pca953x_chip *chip, unsigned int idx, + unsigned int duty_cycle) +{ + /* PWM phase must not be inverted to work in the [1/16;16/16] range */ + pca953x_gpio_direction_output(&chip->gpio_chip, idx, 1); + + /* Set the PWM intensity to the desired duty-cycle */ + return max7313_pwm_set_intensity(chip, idx, 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); + int idx = pwm_device->hwpwm; + struct gpio_desc *desc; + int ret; + + if (pwm->gpiods[idx]) { + ret = -EBUSY; + } else { + desc = gpio_to_desc(chip->gpio_chip.base + idx); + if (!desc) + return -ENODEV; + + ret = gpiod_request(desc, "max7313-pwm"); + if (ret) + return ret; + + pwm->gpiods[idx] = desc; + } + + ret = max7313_pwm_disable(chip, idx); + if (ret) + return ret; + + /* + * Set master intensity to the maximum level to let individual outputs + * the greatest flexibility range. Also enables the internal oscillator. + */ + if (!pwm->used) { + 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) + return ret; + } + + pwm->used++; + + return 0; +} + +static void max7313_pwm_free(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); + int idx = pwm_device->hwpwm; + + max7313_pwm_disable(chip, idx); + pwm->used--; + + /* Disable the internal oscillator if no channel is in use */ + if (!pwm->used) { + 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); + } + + gpiod_free(pwm->gpiods[idx]); + pwm->gpiods[idx] = NULL; +} + +static int max7313_pwm_apply(struct pwm_chip *pwm_chip, + struct pwm_device *pwm_device, + const struct pwm_state *state) +{ + struct max7313_pwm *pwm = to_max7313_pwm(pwm_chip); + struct pca953x_chip *chip = to_pca953x(pwm); + int idx = pwm_device->hwpwm; + unsigned int duty_cycle; + + if (!state->enabled) + return max7313_pwm_disable(chip, idx); + + if (!state->period || !state->duty_cycle) + return max7313_pwm_enable_static_low(chip, idx); + + /* Convert the duty-cycle to be in the [1;16] range */ + duty_cycle = DIV_ROUND_UP(state->duty_cycle * PWM_DC_STATES, + state->period); + + return max7313_pwm_enable(chip, idx, duty_cycle); +} + +static const struct pwm_ops max7313_pwm_ops = { + .request = max7313_pwm_request, + .free = max7313_pwm_free, + .apply = max7313_pwm_apply, + .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 */ + 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; + + ret = pwmchip_add(pwm_chip); + + return ret; +} + static const struct of_device_id pca953x_dt_ids[]; static int pca953x_probe(struct i2c_client *client, @@ -1018,6 +1260,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 +1407,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> --- drivers/gpio/gpio-pca953x.c | 249 +++++++++++++++++++++++++++++++++++- 1 file changed, 247 insertions(+), 2 deletions(-)