Add pwm driver for the NXP pca9685 16 channel pwm-led controller.
The driver is really barebones at this stage. E.g. the OE' pin and
therefore the according registers are not supported.
The driver was tested on a HW where this pin is tied to GND.
Signed-off-by: Steffen Trumtrar <[email protected]>
---
Changes since v1:
- fix typo in documentation example
.../devicetree/bindings/pwm/nxp,pca9685-pwmled.txt | 28 +++
drivers/pwm/Kconfig | 9 +
drivers/pwm/Makefile | 1 +
drivers/pwm/pwm-pca9685-led.c | 245 +++++++++++++++++++++
4 files changed, 283 insertions(+)
create mode 100644 Documentation/devicetree/bindings/pwm/nxp,pca9685-pwmled.txt
create mode 100644 drivers/pwm/pwm-pca9685-led.c
diff --git a/Documentation/devicetree/bindings/pwm/nxp,pca9685-pwmled.txt b/Documentation/devicetree/bindings/pwm/nxp,pca9685-pwmled.txt
new file mode 100644
index 0000000..6646980
--- /dev/null
+++ b/Documentation/devicetree/bindings/pwm/nxp,pca9685-pwmled.txt
@@ -0,0 +1,28 @@
+NXP PCA9685 16-channel 12-bit PWM LED controller
+================================================
+
+Required properties:
+ - compatible: "nxp,pca9685-pwmled"
+ - #pwm-cells: should be 2. The first cell specifies the per-chip index
+ of the PWM to use and the second cell is the period in nanoseconds.
+ The index 16 is the ALLCALL channel, that sets all pwm channels at the same
+ time.
+
+Optional properties:
+ - invrt: <0> output logic state not inverted
+ <1> output logic state inverted
+ - outdrv: <0> configure outputs with open-drain structure
+ <1> configure outputs with totem pole structure
+
+Example:
+
+For LEDs that are directly connected to the pca, the following setting is
+applicable:
+
+pca: pca@41 {
+ compatible = "nxp,pca9685-pwmled";
+ #pwm-cells = <2>;
+ reg = <0x41>;
+ invrt = <1>;
+ outdrv = <0>;
+};
diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index 115b644..8696f61 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -97,6 +97,15 @@ config PWM_MXS
To compile this driver as a module, choose M here: the module
will be called pwm-mxs.
+config PWM_PCA9685_LED
+ tristate "NXP PCA9685 PWM support for LED drivers"
+ depends on OF
+ help
+ Generic PWM framework driver for NXP PCA9685 LED controller.
+
+ To compile this driver as a module, choose M here: the module
+ will be called pwm-pca9685-led.
+
config PWM_PUV3
tristate "PKUnity NetBook-0916 PWM support"
depends on ARCH_PUV3
diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
index 94ba21e..1924f03 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -6,6 +6,7 @@ obj-$(CONFIG_PWM_IMX) += pwm-imx.o
obj-$(CONFIG_PWM_JZ4740) += pwm-jz4740.o
obj-$(CONFIG_PWM_LPC32XX) += pwm-lpc32xx.o
obj-$(CONFIG_PWM_MXS) += pwm-mxs.o
+obj-$(CONFIG_PWM_PCA9685_LED) += pwm-pca9685-led.o
obj-$(CONFIG_PWM_PUV3) += pwm-puv3.o
obj-$(CONFIG_PWM_PXA) += pwm-pxa.o
obj-$(CONFIG_PWM_SAMSUNG) += pwm-samsung.o
diff --git a/drivers/pwm/pwm-pca9685-led.c b/drivers/pwm/pwm-pca9685-led.c
new file mode 100644
index 0000000..c5e78b0
--- /dev/null
+++ b/drivers/pwm/pwm-pca9685-led.c
@@ -0,0 +1,245 @@
+/*
+ * Driver for PCA9685 16-channel 12-bit PWM LED controller
+ *
+ * Copyright (C) 2013 Steffen Trumtrar <[email protected]>
+ *
+ * based on the pwm-twl-led.c driver
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 as published by
+ * the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program. If not, see <http://www.gnu.org/licenses/>.
+ */
+#include <linux/i2c.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/pwm.h>
+#include <linux/regmap.h>
+#include <linux/slab.h>
+
+#define PCA9685_MODE1 0x00
+#define PCA9685_MODE2 0x01
+#define PCA9685_SUBADDR1 0x02
+#define PCA9685_SUBADDR2 0x03
+#define PCA9685_SUBADDR3 0x04
+#define PCA9685_ALLCALLADDR 0x05
+#define PCA9685_LEDX_ON_L 0x06
+#define PCA9685_LEDX_ON_H 0x07
+#define PCA9685_LEDX_OFF_L 0x08
+#define PCA9685_LEDX_OFF_H 0x09
+
+#define PCA9685_ALL_LED_ON_L 0xFA
+#define PCA9685_ALL_LED_ON_H 0xFB
+#define PCA9685_ALL_LED_OFF_L 0xFC
+#define PCA9685_ALL_LED_OFF_H 0xFD
+#define PCA9685_PRESCALE 0xFE
+
+#define PCA9685_NUMREGS 0xFF
+#define PCA9685_MAXCHAN 0x10
+
+#define LED_FULL (1 << 4)
+#define MODE1_SLEEP (1 << 4)
+#define MODE2_INVRT (1 << 4)
+#define MODE2_OUTDRV (1 << 2)
+
+#define LED_N_ON_H(N) (PCA9685_LEDX_ON_H + (4 * (N)))
+#define LED_N_ON_L(N) (PCA9685_LEDX_ON_L + (4 * (N)))
+#define LED_N_OFF_H(N) (PCA9685_LEDX_OFF_H + (4 * (N)))
+#define LED_N_OFF_L(N) (PCA9685_LEDX_OFF_L + (4 * (N)))
+
+struct pca9685 {
+ struct pwm_chip chip;
+ struct regmap *regmap;
+ struct device *dev;
+};
+
+static inline struct pca9685 *to_pca(struct pwm_chip *chip)
+{
+ return container_of(chip, struct pca9685, chip);
+}
+
+static int pca9685_pwmled_config(struct pwm_chip *chip, struct pwm_device *pwm,
+ int duty_ns, int period_ns)
+{
+ struct pca9685 *pca = to_pca(chip);
+ unsigned long long duty;
+ unsigned int reg;
+
+ if (duty_ns < 1)
+ return 0;
+
+ if (duty_ns == period_ns) {
+ reg = (pwm->hwpwm == PCA9685_MAXCHAN) ? PCA9685_ALL_LED_ON_H
+ : LED_N_ON_H(pwm->hwpwm);
+ regmap_write(pca->regmap, reg, LED_FULL);
+ return 0;
+ }
+
+ duty = 4096 * (unsigned long long)duty_ns;
+ duty = DIV_ROUND_UP_ULL(duty, period_ns);
+
+ reg = (pwm->hwpwm == PCA9685_MAXCHAN) ? PCA9685_ALL_LED_OFF_L
+ : LED_N_OFF_L(pwm->hwpwm);
+ regmap_write(pca->regmap, reg, 0xff & (int)duty);
+ reg = (pwm->hwpwm == PCA9685_MAXCHAN) ? PCA9685_ALL_LED_OFF_H
+ : LED_N_OFF_H(pwm->hwpwm);
+ regmap_write(pca->regmap, reg, 0xf & ((int)duty >> 8));
+
+ return 0;
+}
+
+static int pca9685_pwmled_enable(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+ struct pca9685 *pca = to_pca(chip);
+ unsigned int reg;
+
+ /* the PWM subsystem does not support a pre-delay */
+ reg = (pwm->hwpwm == PCA9685_MAXCHAN) ? PCA9685_ALL_LED_ON_L
+ : LED_N_ON_L(pwm->hwpwm);
+ regmap_write(pca->regmap, reg, 0);
+ reg = (pwm->hwpwm == PCA9685_MAXCHAN) ? PCA9685_ALL_LED_ON_H
+ : LED_N_ON_H(pwm->hwpwm);
+ regmap_write(pca->regmap, reg, 0);
+
+ reg = (pwm->hwpwm == PCA9685_MAXCHAN) ? PCA9685_ALL_LED_OFF_H
+ : LED_N_OFF_H(pwm->hwpwm);
+ regmap_update_bits(pca->regmap, reg, LED_FULL, 0x0);
+
+ return 0;
+}
+
+static void pca9685_pwmled_disable(struct pwm_chip *chip,
+ struct pwm_device *pwm)
+{
+ struct pca9685 *pca = to_pca(chip);
+ unsigned int reg;
+
+ reg = (pwm->hwpwm == PCA9685_MAXCHAN) ? PCA9685_ALL_LED_OFF_H
+ : LED_N_OFF_H(pwm->hwpwm);
+ regmap_write(pca->regmap, reg, LED_FULL);
+ reg = (pwm->hwpwm == PCA9685_MAXCHAN) ? PCA9685_ALL_LED_OFF_L
+ : LED_N_OFF_L(pwm->hwpwm);
+ regmap_write(pca->regmap, reg, 0x0);
+}
+
+static int pca9685_pwmled_request(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+ struct pca9685 *pca = to_pca(chip);
+
+ return regmap_update_bits(pca->regmap, PCA9685_MODE1, MODE1_SLEEP, 0x0);
+}
+
+static const struct pwm_ops pca9685_pwmled_ops = {
+ .enable = pca9685_pwmled_enable,
+ .disable = pca9685_pwmled_disable,
+ .config = pca9685_pwmled_config,
+ .request = pca9685_pwmled_request,
+};
+
+static struct regmap_config pca9685_regmap_i2c_config = {
+ .reg_bits = 8,
+ .val_bits = 8,
+ .max_register = PCA9685_NUMREGS,
+ .cache_type = REGCACHE_NONE,
+};
+
+static int pca9685_pwmled_probe(struct i2c_client *client,
+ const struct i2c_device_id *id)
+{
+ struct device_node *np = client->dev.of_node;
+ struct pca9685 *pca;
+ int ret;
+ int val;
+ int mode2;
+
+ pca = devm_kzalloc(&client->dev, sizeof(*pca), GFP_KERNEL);
+ if (!pca)
+ return -ENOMEM;
+
+ pca->regmap = devm_regmap_init_i2c(client, &pca9685_regmap_i2c_config);
+ if (IS_ERR(pca->regmap)) {
+ ret = PTR_ERR(pca->regmap);
+ dev_err(&client->dev, "Failed to initialize register map: %d\n",
+ ret);
+ return ret;
+ }
+
+ i2c_set_clientdata(client, pca);
+ pca->dev = &client->dev;
+
+ regmap_read(pca->regmap, PCA9685_MODE2, &mode2);
+ if (!of_property_read_u32(np, "invrt", &val)) {
+ if (val)
+ mode2 |= MODE2_INVRT;
+ else
+ mode2 &= ~MODE2_INVRT;
+ }
+ if (!of_property_read_u32(np, "outdrv", &val)) {
+ if (val)
+ mode2 |= MODE2_OUTDRV;
+ else
+ mode2 &= ~MODE2_OUTDRV;
+ }
+ regmap_write(pca->regmap, PCA9685_MODE2, mode2);
+
+ /* clear all "full off" bits */
+ regmap_write(pca->regmap, PCA9685_ALL_LED_OFF_L, 0);
+ regmap_write(pca->regmap, PCA9685_ALL_LED_OFF_H, 0);
+
+ pca->chip.ops = &pca9685_pwmled_ops;
+ /* add an extra channel for ALL_LED */
+ pca->chip.npwm = PCA9685_MAXCHAN + 1;
+
+ pca->chip.dev = &client->dev;
+ pca->chip.base = -1;
+ pca->chip.can_sleep = true;
+
+ ret = pwmchip_add(&pca->chip);
+ if (ret < 0)
+ return ret;
+
+ return 0;
+}
+
+static int pca9685_pwmled_remove(struct i2c_client *client)
+{
+ struct pca9685 *pca = i2c_get_clientdata(client);
+
+ return pwmchip_remove(&pca->chip);
+}
+
+static const struct i2c_device_id pca9685_id[] = {
+ {"pca9685", 0},
+ {},
+};
+MODULE_DEVICE_TABLE(i2c, pca9685_id);
+
+static const struct of_device_id pca9685_dt_ids[] = {
+ { .compatible = "nxp,pca9685-pwmled", },
+ { /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, pca9685_dt_ids);
+
+static struct i2c_driver pca9685_i2c_driver = {
+ .driver = {
+ .name = "pca9685",
+ .owner = THIS_MODULE,
+ .of_match_table = pca9685_dt_ids,
+ },
+ .probe = pca9685_pwmled_probe,
+ .remove = pca9685_pwmled_remove,
+ .id_table = pca9685_id,
+};
+
+module_i2c_driver(pca9685_i2c_driver);
+
+MODULE_AUTHOR("Steffen Trumtrar <[email protected]>");
+MODULE_DESCRIPTION("PWM driver for PCA9685 LED outputs");
+MODULE_LICENSE("GPL");
--
1.8.2.rc2
On Mon, May 27, 2013 at 11:28:27AM +0200, Steffen Trumtrar wrote:
> Add pwm driver for the NXP pca9685 16 channel pwm-led controller.
Please stick to the all-caps spelling of PWM in prose. Also why is this
called pwm-led? Can't the generated PWM signal be used for other
purposes?
> The driver is really barebones at this stage. E.g. the OE' pin and
> therefore the according registers are not supported.
s/according/corresponding/?
> The driver was tested on a HW where this pin is tied to GND.
>
> Signed-off-by: Steffen Trumtrar <[email protected]>
> ---
> Changes since v1:
> - fix typo in documentation example
>
> .../devicetree/bindings/pwm/nxp,pca9685-pwmled.txt | 28 +++
> drivers/pwm/Kconfig | 9 +
> drivers/pwm/Makefile | 1 +
> drivers/pwm/pwm-pca9685-led.c | 245 +++++++++++++++++++++
> 4 files changed, 283 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/pwm/nxp,pca9685-pwmled.txt
> create mode 100644 drivers/pwm/pwm-pca9685-led.c
>
> diff --git a/Documentation/devicetree/bindings/pwm/nxp,pca9685-pwmled.txt b/Documentation/devicetree/bindings/pwm/nxp,pca9685-pwmled.txt
> new file mode 100644
> index 0000000..6646980
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pwm/nxp,pca9685-pwmled.txt
> @@ -0,0 +1,28 @@
> +NXP PCA9685 16-channel 12-bit PWM LED controller
> +================================================
> +
> +Required properties:
> + - compatible: "nxp,pca9685-pwmled"
> + - #pwm-cells: should be 2. The first cell specifies the per-chip index
> + of the PWM to use and the second cell is the period in nanoseconds.
> + The index 16 is the ALLCALL channel, that sets all pwm channels at the same
"PWM channels" please.
> +Optional properties:
> + - invrt: <0> output logic state not inverted
> + <1> output logic state inverted
Why not make this "invert"?
> + - outdrv: <0> configure outputs with open-drain structure
> + <1> configure outputs with totem pole structure
And this could be a boolean property called "totem-pole"? I think that's
much more intuitive to use.
> +Example:
> +
> +For LEDs that are directly connected to the pca, the following setting is
"to the PCA"?
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index 115b644..8696f61 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -97,6 +97,15 @@ config PWM_MXS
> To compile this driver as a module, choose M here: the module
> will be called pwm-mxs.
>
> +config PWM_PCA9685_LED
> + tristate "NXP PCA9685 PWM support for LED drivers"
> + depends on OF
Isn't this missing a "depends on REGMAP_I2C"? And again, why is this LED
specific? Even if only LEDs are driven by the PWM signals, the part does
not have other functionality than driving PWM signals, right? So adding
an -led suffix isn't meaningful.
> diff --git a/drivers/pwm/pwm-pca9685-led.c b/drivers/pwm/pwm-pca9685-led.c
[...]
> + * this program. If not, see <http://www.gnu.org/licenses/>.
> + */
> +#include <linux/i2c.h>
I know I'm being picky here, but can you leave a blank line between the
end of the comment and the first #include, please?
> +#define PCA9685_MODE1 0x00
> +#define PCA9685_MODE2 0x01
> +#define PCA9685_SUBADDR1 0x02
> +#define PCA9685_SUBADDR2 0x03
> +#define PCA9685_SUBADDR3 0x04
> +#define PCA9685_ALLCALLADDR 0x05
> +#define PCA9685_LEDX_ON_L 0x06
> +#define PCA9685_LEDX_ON_H 0x07
> +#define PCA9685_LEDX_OFF_L 0x08
> +#define PCA9685_LEDX_OFF_H 0x09
> +
> +#define PCA9685_ALL_LED_ON_L 0xFA
> +#define PCA9685_ALL_LED_ON_H 0xFB
> +#define PCA9685_ALL_LED_OFF_L 0xFC
> +#define PCA9685_ALL_LED_OFF_H 0xFD
> +#define PCA9685_PRESCALE 0xFE
> +
> +#define PCA9685_NUMREGS 0xFF
> +#define PCA9685_MAXCHAN 0x10
> +
> +#define LED_FULL (1 << 4)
> +#define MODE1_SLEEP (1 << 4)
> +#define MODE2_INVRT (1 << 4)
> +#define MODE2_OUTDRV (1 << 2)
> +
> +#define LED_N_ON_H(N) (PCA9685_LEDX_ON_H + (4 * (N)))
> +#define LED_N_ON_L(N) (PCA9685_LEDX_ON_L + (4 * (N)))
> +#define LED_N_OFF_H(N) (PCA9685_LEDX_OFF_H + (4 * (N)))
> +#define LED_N_OFF_L(N) (PCA9685_LEDX_OFF_L + (4 * (N)))
> +
> +struct pca9685 {
> + struct pwm_chip chip;
> + struct regmap *regmap;
> + struct device *dev;
> +};
> +
> +static inline struct pca9685 *to_pca(struct pwm_chip *chip)
> +{
> + return container_of(chip, struct pca9685, chip);
> +}
> +
> +static int pca9685_pwmled_config(struct pwm_chip *chip, struct pwm_device *pwm,
> + int duty_ns, int period_ns)
The alignment doesn't look right here.
> +{
> + struct pca9685 *pca = to_pca(chip);
> + unsigned long long duty;
> + unsigned int reg;
> +
> + if (duty_ns < 1)
> + return 0;
Doesn't duty_ns < 1 mean the signal is off? Why are you ignoring this
case?
> +
> + if (duty_ns == period_ns) {
> + reg = (pwm->hwpwm == PCA9685_MAXCHAN) ? PCA9685_ALL_LED_ON_H
> + : LED_N_ON_H(pwm->hwpwm);
There's a few occurrences of this style in the driver. I don't like it
much because it makes things hard to read. How about writing it this
way?
if (pwm->hwpwm < PCA9685_MAXCHAN)
reg = LED_N_ON_H(pwm->hwpwm);
else
reg = PCA9685_ALL_LED_ON_H;
And similar for the other cases.
> + duty = 4096 * (unsigned long long)duty_ns;
> + duty = DIV_ROUND_UP_ULL(duty, period_ns);
> +
> + reg = (pwm->hwpwm == PCA9685_MAXCHAN) ? PCA9685_ALL_LED_OFF_L
> + : LED_N_OFF_L(pwm->hwpwm);
Like here.
> + regmap_write(pca->regmap, reg, 0xff & (int)duty);
"(int)duty & 0xff", please.
> + regmap_write(pca->regmap, reg, 0xf & ((int)duty >> 8));
Same here.
> +static int pca9685_pwmled_enable(struct pwm_chip *chip, struct pwm_device *pwm)
> +{
> + struct pca9685 *pca = to_pca(chip);
> + unsigned int reg;
> +
> + /* the PWM subsystem does not support a pre-delay */
What do you mean by that? How is it relevant to this code. Maybe there's
a case to be made to add functionality that you miss.
> +static int pca9685_pwmled_request(struct pwm_chip *chip, struct pwm_device *pwm)
> +{
> + struct pca9685 *pca = to_pca(chip);
> +
> + return regmap_update_bits(pca->regmap, PCA9685_MODE1, MODE1_SLEEP, 0x0);
> +}
Don't you need to set the SLEEP bit in a pca9685_pwmled_free() function?
Since the SLEEP bit is for all PWM channels, you probably need some
reference counting to make sure you only set it when all channels have
been released.
> +static int pca9685_pwmled_probe(struct i2c_client *client,
> + const struct i2c_device_id *id)
> +{
> + struct device_node *np = client->dev.of_node;
> + struct pca9685 *pca;
> + int ret;
> + int val;
> + int mode2;
> +
> + pca = devm_kzalloc(&client->dev, sizeof(*pca), GFP_KERNEL);
> + if (!pca)
> + return -ENOMEM;
> +
> + pca->regmap = devm_regmap_init_i2c(client, &pca9685_regmap_i2c_config);
> + if (IS_ERR(pca->regmap)) {
> + ret = PTR_ERR(pca->regmap);
> + dev_err(&client->dev, "Failed to initialize register map: %d\n",
> + ret);
> + return ret;
> + }
> +
> + i2c_set_clientdata(client, pca);
> + pca->dev = &client->dev;
> +
> + regmap_read(pca->regmap, PCA9685_MODE2, &mode2);
> + if (!of_property_read_u32(np, "invrt", &val)) {
These two lines could use a blank line as a separator.
> + if (val)
> + mode2 |= MODE2_INVRT;
> + else
> + mode2 &= ~MODE2_INVRT;
> + }
> + if (!of_property_read_u32(np, "outdrv", &val)) {
These too.
> + if (val)
> + mode2 |= MODE2_OUTDRV;
> + else
> + mode2 &= ~MODE2_OUTDRV;
> + }
> + regmap_write(pca->regmap, PCA9685_MODE2, mode2);
And these.
> + ret = pwmchip_add(&pca->chip);
> + if (ret < 0)
> + return ret;
> +
> + return 0;
"return pwmchip_add(&pca->chip);"?
> +static const struct i2c_device_id pca9685_id[] = {
> + {"pca9685", 0},
Spaces between '{' and '"' as well as '0' and '}', please.
> + {},
"{ }", please. Or "{ /* sentinel */ }" as in the "of" table.
> +static struct i2c_driver pca9685_i2c_driver = {
> + .driver = {
> + .name = "pca9685",
> + .owner = THIS_MODULE,
> + .of_match_table = pca9685_dt_ids,
> + },
This uses weird identing. Please fix.
Thierry
Hi!
On Mon, May 27, 2013 at 09:13:34PM +0200, Thierry Reding wrote:
> On Mon, May 27, 2013 at 11:28:27AM +0200, Steffen Trumtrar wrote:
> > Add pwm driver for the NXP pca9685 16 channel pwm-led controller.
>
> Please stick to the all-caps spelling of PWM in prose. Also why is this
> called pwm-led? Can't the generated PWM signal be used for other
> purposes?
>
Hm, sure. They can. The unit is marketed as a PWM LED controller, but you can
use it for whatever PWM needs you have. I will change that.
> > The driver is really barebones at this stage. E.g. the OE' pin and
> > therefore the according registers are not supported.
>
> s/according/corresponding/?
Yes. Better.
>
> > The driver was tested on a HW where this pin is tied to GND.
> >
> > Signed-off-by: Steffen Trumtrar <[email protected]>
> > ---
> > Changes since v1:
> > - fix typo in documentation example
> >
> > .../devicetree/bindings/pwm/nxp,pca9685-pwmled.txt | 28 +++
> > drivers/pwm/Kconfig | 9 +
> > drivers/pwm/Makefile | 1 +
> > drivers/pwm/pwm-pca9685-led.c | 245 +++++++++++++++++++++
> > 4 files changed, 283 insertions(+)
> > create mode 100644 Documentation/devicetree/bindings/pwm/nxp,pca9685-pwmled.txt
> > create mode 100644 drivers/pwm/pwm-pca9685-led.c
> >
> > diff --git a/Documentation/devicetree/bindings/pwm/nxp,pca9685-pwmled.txt b/Documentation/devicetree/bindings/pwm/nxp,pca9685-pwmled.txt
> > new file mode 100644
> > index 0000000..6646980
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/pwm/nxp,pca9685-pwmled.txt
> > @@ -0,0 +1,28 @@
> > +NXP PCA9685 16-channel 12-bit PWM LED controller
> > +================================================
> > +
> > +Required properties:
> > + - compatible: "nxp,pca9685-pwmled"
> > + - #pwm-cells: should be 2. The first cell specifies the per-chip index
> > + of the PWM to use and the second cell is the period in nanoseconds.
> > + The index 16 is the ALLCALL channel, that sets all pwm channels at the same
>
> "PWM channels" please.
>
Okay.
> > +Optional properties:
> > + - invrt: <0> output logic state not inverted
> > + <1> output logic state inverted
>
> Why not make this "invert"?
>
I used the naming of NXPs datasheet, but I can change that.
> > + - outdrv: <0> configure outputs with open-drain structure
> > + <1> configure outputs with totem pole structure
>
> And this could be a boolean property called "totem-pole"? I think that's
> much more intuitive to use.
>
This is also a register field from NXP. The datasheet references the invrt and
outdrv bit in different locations to describe some use case and the corresponding
setting of this two bits. That is why I thought it might be easier to use.
Get use case from datasheet -> put corresponding value in DT -> done.
On the other hand, the DT might be easier to understand with a boolean property.
I would than go for "open-drain" though, as totem-pole is the reset default.
> > +Example:
> > +
> > +For LEDs that are directly connected to the pca, the following setting is
>
> "to the PCA"?
>
Okay.
> > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> > index 115b644..8696f61 100644
> > --- a/drivers/pwm/Kconfig
> > +++ b/drivers/pwm/Kconfig
> > @@ -97,6 +97,15 @@ config PWM_MXS
> > To compile this driver as a module, choose M here: the module
> > will be called pwm-mxs.
> >
> > +config PWM_PCA9685_LED
> > + tristate "NXP PCA9685 PWM support for LED drivers"
> > + depends on OF
>
> Isn't this missing a "depends on REGMAP_I2C"? And again, why is this LED
> specific? Even if only LEDs are driven by the PWM signals, the part does
> not have other functionality than driving PWM signals, right? So adding
> an -led suffix isn't meaningful.
>
Yes, you are right. I will change that.
> > diff --git a/drivers/pwm/pwm-pca9685-led.c b/drivers/pwm/pwm-pca9685-led.c
> [...]
> > + * this program. If not, see <http://www.gnu.org/licenses/>.
> > + */
> > +#include <linux/i2c.h>
>
> I know I'm being picky here, but can you leave a blank line between the
> end of the comment and the first #include, please?
>
Okay.
> > +#define PCA9685_MODE1 0x00
> > +#define PCA9685_MODE2 0x01
> > +#define PCA9685_SUBADDR1 0x02
> > +#define PCA9685_SUBADDR2 0x03
> > +#define PCA9685_SUBADDR3 0x04
> > +#define PCA9685_ALLCALLADDR 0x05
> > +#define PCA9685_LEDX_ON_L 0x06
> > +#define PCA9685_LEDX_ON_H 0x07
> > +#define PCA9685_LEDX_OFF_L 0x08
> > +#define PCA9685_LEDX_OFF_H 0x09
> > +
> > +#define PCA9685_ALL_LED_ON_L 0xFA
> > +#define PCA9685_ALL_LED_ON_H 0xFB
> > +#define PCA9685_ALL_LED_OFF_L 0xFC
> > +#define PCA9685_ALL_LED_OFF_H 0xFD
> > +#define PCA9685_PRESCALE 0xFE
> > +
> > +#define PCA9685_NUMREGS 0xFF
> > +#define PCA9685_MAXCHAN 0x10
> > +
> > +#define LED_FULL (1 << 4)
> > +#define MODE1_SLEEP (1 << 4)
> > +#define MODE2_INVRT (1 << 4)
> > +#define MODE2_OUTDRV (1 << 2)
> > +
> > +#define LED_N_ON_H(N) (PCA9685_LEDX_ON_H + (4 * (N)))
> > +#define LED_N_ON_L(N) (PCA9685_LEDX_ON_L + (4 * (N)))
> > +#define LED_N_OFF_H(N) (PCA9685_LEDX_OFF_H + (4 * (N)))
> > +#define LED_N_OFF_L(N) (PCA9685_LEDX_OFF_L + (4 * (N)))
> > +
> > +struct pca9685 {
> > + struct pwm_chip chip;
> > + struct regmap *regmap;
> > + struct device *dev;
> > +};
> > +
> > +static inline struct pca9685 *to_pca(struct pwm_chip *chip)
> > +{
> > + return container_of(chip, struct pca9685, chip);
> > +}
> > +
> > +static int pca9685_pwmled_config(struct pwm_chip *chip, struct pwm_device *pwm,
> > + int duty_ns, int period_ns)
>
> The alignment doesn't look right here.
>
> > +{
> > + struct pca9685 *pca = to_pca(chip);
> > + unsigned long long duty;
> > + unsigned int reg;
> > +
> > + if (duty_ns < 1)
> > + return 0;
>
> Doesn't duty_ns < 1 mean the signal is off? Why are you ignoring this
> case?
>
I just tested with led-pwm which calls config and then disable.
So, I don't need to do any setup in this case. I don't know, if that is
PWM framework or driver specific. Maybe I do have to do something in this
case? I will have a look.
> > +
> > + if (duty_ns == period_ns) {
> > + reg = (pwm->hwpwm == PCA9685_MAXCHAN) ? PCA9685_ALL_LED_ON_H
> > + : LED_N_ON_H(pwm->hwpwm);
>
> There's a few occurrences of this style in the driver. I don't like it
> much because it makes things hard to read. How about writing it this
> way?
>
> if (pwm->hwpwm < PCA9685_MAXCHAN)
> reg = LED_N_ON_H(pwm->hwpwm);
> else
> reg = PCA9685_ALL_LED_ON_H;
>
> And similar for the other cases.
>
If it is preferred, I can change that. No problem.
> > + duty = 4096 * (unsigned long long)duty_ns;
> > + duty = DIV_ROUND_UP_ULL(duty, period_ns);
> > +
> > + reg = (pwm->hwpwm == PCA9685_MAXCHAN) ? PCA9685_ALL_LED_OFF_L
> > + : LED_N_OFF_L(pwm->hwpwm);
>
> Like here.
>
> > + regmap_write(pca->regmap, reg, 0xff & (int)duty);
>
> "(int)duty & 0xff", please.
>
> > + regmap_write(pca->regmap, reg, 0xf & ((int)duty >> 8));
>
> Same here.
>
I will change all three.
> > +static int pca9685_pwmled_enable(struct pwm_chip *chip, struct pwm_device *pwm)
> > +{
> > + struct pca9685 *pca = to_pca(chip);
> > + unsigned int reg;
> > +
> > + /* the PWM subsystem does not support a pre-delay */
>
> What do you mean by that? How is it relevant to this code. Maybe there's
> a case to be made to add functionality that you miss.
>
Well, you are right. The PCA supports setting two delays: start of duty cycle and end of
duty cycle. The PWM subsystem AFAIK only supports setting the length of the duty cycle.
Which results in me setting the start of duty cycle to 0.
If other PWM chips also support this and there actually is a use case for this,
than that might be a functionality missing from the PWM subsystem.
> > +static int pca9685_pwmled_request(struct pwm_chip *chip, struct pwm_device *pwm)
> > +{
> > + struct pca9685 *pca = to_pca(chip);
> > +
> > + return regmap_update_bits(pca->regmap, PCA9685_MODE1, MODE1_SLEEP, 0x0);
> > +}
>
> Don't you need to set the SLEEP bit in a pca9685_pwmled_free() function?
> Since the SLEEP bit is for all PWM channels, you probably need some
> reference counting to make sure you only set it when all channels have
> been released.
>
Hm, yes. I might have to add that.
> > +static int pca9685_pwmled_probe(struct i2c_client *client,
> > + const struct i2c_device_id *id)
> > +{
> > + struct device_node *np = client->dev.of_node;
> > + struct pca9685 *pca;
> > + int ret;
> > + int val;
> > + int mode2;
> > +
> > + pca = devm_kzalloc(&client->dev, sizeof(*pca), GFP_KERNEL);
> > + if (!pca)
> > + return -ENOMEM;
> > +
> > + pca->regmap = devm_regmap_init_i2c(client, &pca9685_regmap_i2c_config);
> > + if (IS_ERR(pca->regmap)) {
> > + ret = PTR_ERR(pca->regmap);
> > + dev_err(&client->dev, "Failed to initialize register map: %d\n",
> > + ret);
> > + return ret;
> > + }
> > +
> > + i2c_set_clientdata(client, pca);
> > + pca->dev = &client->dev;
> > +
> > + regmap_read(pca->regmap, PCA9685_MODE2, &mode2);
> > + if (!of_property_read_u32(np, "invrt", &val)) {
>
> These two lines could use a blank line as a separator.
>
> > + if (val)
> > + mode2 |= MODE2_INVRT;
> > + else
> > + mode2 &= ~MODE2_INVRT;
> > + }
> > + if (!of_property_read_u32(np, "outdrv", &val)) {
>
> These too.
>
> > + if (val)
> > + mode2 |= MODE2_OUTDRV;
> > + else
> > + mode2 &= ~MODE2_OUTDRV;
> > + }
> > + regmap_write(pca->regmap, PCA9685_MODE2, mode2);
>
> And these.
>
> > + ret = pwmchip_add(&pca->chip);
> > + if (ret < 0)
> > + return ret;
> > +
> > + return 0;
>
> "return pwmchip_add(&pca->chip);"?
>
> > +static const struct i2c_device_id pca9685_id[] = {
> > + {"pca9685", 0},
>
> Spaces between '{' and '"' as well as '0' and '}', please.
>
> > + {},
>
> "{ }", please. Or "{ /* sentinel */ }" as in the "of" table.
>
> > +static struct i2c_driver pca9685_i2c_driver = {
> > + .driver = {
> > + .name = "pca9685",
> > + .owner = THIS_MODULE,
> > + .of_match_table = pca9685_dt_ids,
> > + },
>
> This uses weird identing. Please fix.
>
All of the above: I will fix those.
Thanks,
Steffen
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
On Tue, May 28, 2013 at 09:38:01AM +0200, Steffen Trumtrar wrote:
> On Mon, May 27, 2013 at 09:13:34PM +0200, Thierry Reding wrote:
> > On Mon, May 27, 2013 at 11:28:27AM +0200, Steffen Trumtrar wrote:
[...]
> > > +Optional properties:
> > > + - invrt: <0> output logic state not inverted
> > > + <1> output logic state inverted
> >
> > Why not make this "invert"?
> >
>
> I used the naming of NXPs datasheet, but I can change that.
I didn't say it explicitly, but "invert" should also be a boolean
property.
> > > + - outdrv: <0> configure outputs with open-drain structure
> > > + <1> configure outputs with totem pole structure
> >
> > And this could be a boolean property called "totem-pole"? I think that's
> > much more intuitive to use.
> >
>
> This is also a register field from NXP. The datasheet references the invrt and
> outdrv bit in different locations to describe some use case and the corresponding
> setting of this two bits. That is why I thought it might be easier to use.
> Get use case from datasheet -> put corresponding value in DT -> done.
> On the other hand, the DT might be easier to understand with a boolean property.
That was exactly my reasoning. I think it's good and meaningful to
mirror the naming of the datasheet in the driver source code, but the DT
description can be much more intuitive by using common language instead
of some abbreviation taken from the datasheet.
> I would than go for "open-drain" though, as totem-pole is the reset default.
Sounds good.
> > > + if (duty_ns < 1)
> > > + return 0;
> >
> > Doesn't duty_ns < 1 mean the signal is off? Why are you ignoring this
> > case?
> >
>
> I just tested with led-pwm which calls config and then disable.
> So, I don't need to do any setup in this case. I don't know, if that is
> PWM framework or driver specific. Maybe I do have to do something in this
> case? I will have a look.
Generally it can't be assumed that every user will call pwm_disable()
after configuring the duty-cycle to be 0. Both of the in-kernel users do
that, but it's not a strict requirement by the API. Calling
pwm_disable() typically results in the PWM signal being switched off.
pwm_config() with a zero duty-cycle isn't necessarily the same thing,
even if there is often no way to distinguish between both cases on a
physical level.
> > > + /* the PWM subsystem does not support a pre-delay */
> >
> > What do you mean by that? How is it relevant to this code. Maybe there's
> > a case to be made to add functionality that you miss.
> >
>
> Well, you are right. The PCA supports setting two delays: start of duty cycle and end of
> duty cycle. The PWM subsystem AFAIK only supports setting the length of the duty cycle.
> Which results in me setting the start of duty cycle to 0.
> If other PWM chips also support this and there actually is a use case for this,
> than that might be a functionality missing from the PWM subsystem.
So the PCA doesn't define the length of the duty-cycle, but rather an
offset where it starts as well as an offset where it ends? That's very
different from other chips we currently support. I don't think any of
them can do this.
On a related note, there's a bit of a gray area in the PWM subsystem
when it comes to what the PWM signal is supposed to look like. This
first came up when a chip was added that could invert the pin output.
None of our use-cases currently care about the exact form of the signal
and are only interested in the power delivered by the signal (LED,
backlight).
As you say, there's currently no use-case that requires this
functionality, so little sense it adding support now.
Thierry