LP3943 has 16 output pins which can be used as GPIO expander and PWM generator.
* Why do we need GPIO and PWM drivers instead of LED driver?
To support LED control and general usage, GPIO and PWM drivers are necessary.
According to the datasheet(1), it's just a LED driver which has 16 channels.
But here is another application, a GPIO expander.(2)
(1) http://www.ti.com/lit/ds/snvs256b/snvs256b.pdf
(2) http://www.ti.com/lit/an/snva287a/snva287a.pdf
Internal two PWM controllers are used for LED dimming effect.
And each output pin can be used as a GPIO as well.
LED functionality can work with GPIOs or PWMs.
LEDs can be controlled with legacy leds-gpio(static brightness) or
leds-pwm drivers(dynamic brightness control).
Additionally, it can be used for generic GPIO and PWM controller.
For example, a GPIO is HW enable pin of a device.
PWM is input pin of a backlight device.
Therefore, LP3943 GPIO and PWM drivers support all application cases.
LED control General usage for a device
___________ ____________________________
LP3943 MFD ---- GPIO expander leds-gpio eg) HW enable pin
|
--- PWM generator leds-pwm eg) PWM input
* Regmap I2C interface for R/W LP3943 registers
* Atomic operations for output pin assignment
The driver should check whether requested pin is available or not.
If the pin is already used, pin request returns as a failure.
A driver data, 'pin_used' is checked when gpio_request() and
pwm_request() are called. If the pin is available, then pin_used is set.
And it is cleared when gpio_free() and pwm_free().
* Device tree support
Compatible strings for GPIO and PWM driver.
LP3943 platform data is PWM related, so parsing the device tree is
implemented in the PWM driver.
Cc: Lee Jones <[email protected]>
Cc: Linus Walleij <[email protected]>
Cc: Mark Brown <[email protected]>
Cc: Samuel Ortiz <[email protected]>
Cc: Thierry Reding <[email protected]>
Signed-off-by: Milo Kim <[email protected]>
---
drivers/mfd/Kconfig | 11 +++
drivers/mfd/Makefile | 1 +
drivers/mfd/lp3943.c | 165 ++++++++++++++++++++++++++++++++++++++++++++
include/linux/mfd/lp3943.h | 115 ++++++++++++++++++++++++++++++
4 files changed, 292 insertions(+)
create mode 100644 drivers/mfd/lp3943.c
create mode 100644 include/linux/mfd/lp3943.h
diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index ff553ba..2d47d43 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -506,6 +506,17 @@ config PMIC_ADP5520
individual components like LCD backlight, LEDs, GPIOs and Kepad
under the corresponding menus.
+config MFD_LP3943
+ tristate "TI/National Semiconductor LP3943 MFD Driver"
+ depends on I2C
+ select MFD_CORE
+ select REGMAP_I2C
+ help
+ Support for the TI/National Semiconductor LP3943.
+ This driver consists of GPIO and PWM drivers.
+ With these functionality, it can be used for LED string control or
+ general usage such like a GPIO controller and a PWM controller.
+
config MFD_LP8788
bool "Texas Instruments LP8788 Power Management Unit Driver"
depends on I2C=y
diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
index 8b977f8..8f129a4 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -96,6 +96,7 @@ obj-$(CONFIG_PMIC_DA9052) += da9052-core.o
obj-$(CONFIG_MFD_DA9052_SPI) += da9052-spi.o
obj-$(CONFIG_MFD_DA9052_I2C) += da9052-i2c.o
+obj-$(CONFIG_MFD_LP3943) += lp3943.o
obj-$(CONFIG_MFD_LP8788) += lp8788.o lp8788-irq.o
da9055-objs := da9055-core.o da9055-i2c.o
diff --git a/drivers/mfd/lp3943.c b/drivers/mfd/lp3943.c
new file mode 100644
index 0000000..4658965
--- /dev/null
+++ b/drivers/mfd/lp3943.c
@@ -0,0 +1,165 @@
+/*
+ * TI/National Semiconductor LP3943 MFD Core Driver
+ *
+ * Copyright 2013 Texas Instruments
+ *
+ * Author: Milo Kim <[email protected]>
+ *
+ * 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.
+ *
+ */
+
+#include <linux/err.h>
+#include <linux/gpio.h>
+#include <linux/i2c.h>
+#include <linux/mfd/core.h>
+#include <linux/mfd/lp3943.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/slab.h>
+
+#define LP3943_MAX_REGISTERS 0x09
+
+/* Register configuration for pin MUX */
+static const struct lp3943_reg_cfg lp3943_mux_cfg[] = {
+ /* address, mask, shift */
+ { LP3943_REG_MUX0, 0x03, 0 },
+ { LP3943_REG_MUX0, 0x0C, 2 },
+ { LP3943_REG_MUX0, 0x30, 4 },
+ { LP3943_REG_MUX0, 0xC0, 6 },
+ { LP3943_REG_MUX1, 0x03, 0 },
+ { LP3943_REG_MUX1, 0x0C, 2 },
+ { LP3943_REG_MUX1, 0x30, 4 },
+ { LP3943_REG_MUX1, 0xC0, 6 },
+ { LP3943_REG_MUX2, 0x03, 0 },
+ { LP3943_REG_MUX2, 0x0C, 2 },
+ { LP3943_REG_MUX2, 0x30, 4 },
+ { LP3943_REG_MUX2, 0xC0, 6 },
+ { LP3943_REG_MUX3, 0x03, 0 },
+ { LP3943_REG_MUX3, 0x0C, 2 },
+ { LP3943_REG_MUX3, 0x30, 4 },
+ { LP3943_REG_MUX3, 0xC0, 6 },
+};
+
+static struct mfd_cell lp3943_devs[] = {
+ {
+ .name = "lp3943-pwm",
+ .of_compatible = "ti,lp3943-pwm",
+ },
+ {
+ .name = "lp3943-gpio",
+ .of_compatible = "ti,lp3943-gpio",
+ },
+};
+
+int lp3943_read_byte(struct lp3943 *lp3943, u8 reg, u8 *read)
+{
+ int ret;
+ unsigned int val;
+
+ ret = regmap_read(lp3943->regmap, reg, &val);
+ if (ret < 0)
+ return ret;
+
+ *read = (u8)val;
+ return 0;
+}
+EXPORT_SYMBOL_GPL(lp3943_read_byte);
+
+int lp3943_write_byte(struct lp3943 *lp3943, u8 reg, u8 data)
+{
+ return regmap_write(lp3943->regmap, reg, data);
+}
+EXPORT_SYMBOL_GPL(lp3943_write_byte);
+
+int lp3943_update_bits(struct lp3943 *lp3943, u8 reg, u8 mask, u8 data)
+{
+ return regmap_update_bits(lp3943->regmap, reg, mask, data);
+}
+EXPORT_SYMBOL_GPL(lp3943_update_bits);
+
+static const struct regmap_config lp3943_regmap_config = {
+ .reg_bits = 8,
+ .val_bits = 8,
+ .max_register = LP3943_MAX_REGISTERS,
+};
+
+static int lp3943_probe(struct i2c_client *cl, const struct i2c_device_id *id)
+{
+ struct lp3943 *lp3943;
+ int ret;
+
+ lp3943 = devm_kzalloc(&cl->dev, sizeof(*lp3943), GFP_KERNEL);
+ if (!lp3943)
+ return -ENOMEM;
+
+ lp3943->regmap = devm_regmap_init_i2c(cl, &lp3943_regmap_config);
+ if (IS_ERR(lp3943->regmap)) {
+ ret = PTR_ERR(lp3943->regmap);
+ dev_err(&cl->dev, "Regmap init error: %d\n", ret);
+ return ret;
+ }
+
+ lp3943->pdata = cl->dev.platform_data;
+ lp3943->dev = &cl->dev;
+ lp3943->mux_cfg = lp3943_mux_cfg;
+ i2c_set_clientdata(cl, lp3943);
+
+ ret = mfd_add_devices(lp3943->dev, -1, lp3943_devs,
+ ARRAY_SIZE(lp3943_devs), NULL, 0, NULL);
+ if (ret) {
+ dev_err(lp3943->dev, "failed to add MFD devices: %d\n", ret);
+ return ret;
+ }
+
+ return 0;
+}
+
+static int lp3943_remove(struct i2c_client *cl)
+{
+ struct lp3943 *lp3943 = i2c_get_clientdata(cl);
+
+ mfd_remove_devices(lp3943->dev);
+ return 0;
+}
+
+static const struct i2c_device_id lp3943_ids[] = {
+ { "lp3943", 0 },
+ { }
+};
+MODULE_DEVICE_TABLE(i2c, lp3943_ids);
+
+static const struct of_device_id lp3943_dt_ids[] = {
+ { .compatible = "ti,lp3943", },
+ { }
+};
+MODULE_DEVICE_TABLE(of, lp3943_dt_ids);
+
+static struct i2c_driver lp3943_driver = {
+ .driver = {
+ .name = "lp3943",
+ .owner = THIS_MODULE,
+ .of_match_table = of_match_ptr(lp3943_dt_ids),
+ },
+ .probe = lp3943_probe,
+ .remove = lp3943_remove,
+ .id_table = lp3943_ids,
+};
+
+static int __init lp3943_init(void)
+{
+ return i2c_add_driver(&lp3943_driver);
+}
+subsys_initcall(lp3943_init);
+
+static void __exit lp3943_exit(void)
+{
+ i2c_del_driver(&lp3943_driver);
+}
+module_exit(lp3943_exit);
+
+MODULE_DESCRIPTION("LP3943 MFD Core Driver");
+MODULE_AUTHOR("Milo Kim");
+MODULE_LICENSE("GPL");
diff --git a/include/linux/mfd/lp3943.h b/include/linux/mfd/lp3943.h
new file mode 100644
index 0000000..fae320d
--- /dev/null
+++ b/include/linux/mfd/lp3943.h
@@ -0,0 +1,115 @@
+/*
+ * TI/National Semiconductor LP3943 Device
+ *
+ * Copyright 2013 Texas Instruments
+ *
+ * Author: Milo Kim <[email protected]>
+ *
+ * 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.
+ *
+ */
+
+#ifndef __MFD_LP3943_H__
+#define __MFD_LP3943_H__
+
+#include <linux/gpio.h>
+#include <linux/pwm.h>
+#include <linux/regmap.h>
+
+/* Registers */
+#define LP3943_REG_GPIO_A 0x00
+#define LP3943_REG_GPIO_B 0x01
+#define LP3943_REG_PRESCALE0 0x02
+#define LP3943_REG_PWM0 0x03
+#define LP3943_REG_PRESCALE1 0x04
+#define LP3943_REG_PWM1 0x05
+#define LP3943_REG_MUX0 0x06
+#define LP3943_REG_MUX1 0x07
+#define LP3943_REG_MUX2 0x08
+#define LP3943_REG_MUX3 0x09
+
+/* Bit description for LP3943_REG_MUX0 ~ 3 */
+#define LP3943_GPIO_IN 0x00
+#define LP3943_GPIO_OUT_HIGH 0x00
+#define LP3943_GPIO_OUT_LOW 0x01
+#define LP3943_DIM_PWM0 0x02
+#define LP3943_DIM_PWM1 0x03
+
+enum lp3943_pwm_output {
+ LP3943_PWM_INVALID,
+ LP3943_PWM_OUT0,
+ LP3943_PWM_OUT1,
+ LP3943_PWM_OUT2,
+ LP3943_PWM_OUT3,
+ LP3943_PWM_OUT4,
+ LP3943_PWM_OUT5,
+ LP3943_PWM_OUT6,
+ LP3943_PWM_OUT7,
+ LP3943_PWM_OUT8,
+ LP3943_PWM_OUT9,
+ LP3943_PWM_OUT10,
+ LP3943_PWM_OUT11,
+ LP3943_PWM_OUT12,
+ LP3943_PWM_OUT13,
+ LP3943_PWM_OUT14,
+ LP3943_PWM_OUT15,
+};
+
+/*
+ * struct lp3943_pwm_map
+ * @output: Output pins which are mapped to each PWM controller
+ * @num_outputs: Number of outputs
+ */
+struct lp3943_pwm_map {
+ enum lp3943_pwm_output *output;
+ int num_outputs;
+};
+
+/*
+ * struct lp3943_platform_data
+ * @pwm0: Output channel definitions for PWM 0 controller
+ * @pwm1: Output channel definitions for PWM 1 controller
+ */
+struct lp3943_platform_data {
+ struct lp3943_pwm_map *pwm0;
+ struct lp3943_pwm_map *pwm1;
+};
+
+/*
+ * struct lp3943_reg_cfg
+ * @reg: Register address
+ * @mask: Register bit mask to be updated
+ * @shift: Register bit shift
+ */
+struct lp3943_reg_cfg {
+ u8 reg;
+ u8 mask;
+ u8 shift;
+};
+
+/*
+ * struct lp3943
+ * @dev: Parent device pointer
+ * @regmap: Used for I2C communication on accessing registers
+ * @pdata: LP3943 platform specific data
+ * @mux_cfg: Register configuration for pin MUX
+ * @pin_used: Bit mask for output pin used.
+ * This bitmask is used for pin assignment management.
+ * 1 = pin used, 0 = available.
+ * Only LSB 16 bits are used, but it is unsigned long type
+ * for atomic bitwise operations.
+ */
+struct lp3943 {
+ struct device *dev;
+ struct regmap *regmap;
+ struct lp3943_platform_data *pdata;
+ const struct lp3943_reg_cfg *mux_cfg;
+ unsigned long pin_used;
+};
+
+int lp3943_read_byte(struct lp3943 *lp3943, u8 reg, u8 *read);
+int lp3943_write_byte(struct lp3943 *lp3943, u8 reg, u8 data);
+int lp3943_update_bits(struct lp3943 *lp3943, u8 reg, u8 mask, u8 data);
+#endif
--
1.7.9.5
Best Regards,
Milo
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?
> Cc: Lee Jones <[email protected]>
> Cc: Linus Walleij <[email protected]>
> Cc: Mark Brown <[email protected]>
> Cc: Samuel Ortiz <[email protected]>
> Cc: Thierry Reding <[email protected]>
> Signed-off-by: Milo Kim <[email protected]>
> ---
> drivers/mfd/Kconfig | 11 +++
> drivers/mfd/Makefile | 1 +
> drivers/mfd/lp3943.c | 165 ++++++++++++++++++++++++++++++++++++++++++++
> include/linux/mfd/lp3943.h | 115 ++++++++++++++++++++++++++++++
> 4 files changed, 292 insertions(+)
> create mode 100644 drivers/mfd/lp3943.c
> create mode 100644 include/linux/mfd/lp3943.h
<snip> * looks good up to me up to here *
Although, I think the 0 = 1, 1 = 2 ... stuff is really confusing. Is
there nothing we can do about that?
> +static int lp3943_probe(struct i2c_client *cl, const struct i2c_device_id *id)
> +{
> + struct lp3943 *lp3943;
> + int ret;
> +
> + lp3943 = devm_kzalloc(&cl->dev, sizeof(*lp3943), GFP_KERNEL);
> + if (!lp3943)
> + return -ENOMEM;
> +
> + lp3943->regmap = devm_regmap_init_i2c(cl, &lp3943_regmap_config);
> + if (IS_ERR(lp3943->regmap)) {
> + ret = PTR_ERR(lp3943->regmap);
Bit of a nit.
Why don't you: return PTR_ERR(lp3943->regmap);
> + dev_err(&cl->dev, "Regmap init error: %d\n", ret);
> + return ret;
> + }
> +
> + lp3943->pdata = cl->dev.platform_data;
Can you change this to use the helper?
dev_get_platdata()
> + lp3943->dev = &cl->dev;
> + lp3943->mux_cfg = lp3943_mux_cfg;
> + i2c_set_clientdata(cl, lp3943);
> +
> + ret = mfd_add_devices(lp3943->dev, -1, lp3943_devs,
> + ARRAY_SIZE(lp3943_devs), NULL, 0, NULL);
> + if (ret) {
> + dev_err(lp3943->dev, "failed to add MFD devices: %d\n", ret);
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +static int lp3943_remove(struct i2c_client *cl)
> +{
> + struct lp3943 *lp3943 = i2c_get_clientdata(cl);
> +
> + mfd_remove_devices(lp3943->dev);
> + return 0;
> +}
> +
> +static const struct i2c_device_id lp3943_ids[] = {
> + { "lp3943", 0 },
> + { }
> +};
> +MODULE_DEVICE_TABLE(i2c, lp3943_ids);
Are we expecting this driver to support more devices?
> +static const struct of_device_id lp3943_dt_ids[] = {
> + { .compatible = "ti,lp3943", },
> + { }
> +};
> +MODULE_DEVICE_TABLE(of, lp3943_dt_ids);
> +
> +static struct i2c_driver lp3943_driver = {
> + .driver = {
> + .name = "lp3943",
> + .owner = THIS_MODULE,
> + .of_match_table = of_match_ptr(lp3943_dt_ids),
> + },
> + .probe = lp3943_probe,
> + .remove = lp3943_remove,
> + .id_table = lp3943_ids,
> +};
> +
> +static int __init lp3943_init(void)
> +{
> + return i2c_add_driver(&lp3943_driver);
> +}
> +subsys_initcall(lp3943_init);
> +
> +static void __exit lp3943_exit(void)
> +{
> + i2c_del_driver(&lp3943_driver);
> +}
> +module_exit(lp3943_exit);
I think you want to replace:
lp3943_init()
lp3943_exit
With:
module_i2c_driver()
> +MODULE_DESCRIPTION("LP3943 MFD Core Driver");
> +MODULE_AUTHOR("Milo Kim");
> +MODULE_LICENSE("GPL");
--
Lee Jones
Linaro ST-Ericsson Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
Thanks for the review, please see my comments.
> <snip> * looks good up to me up to here *
>
> Although, I think the 0 = 1, 1 = 2 ... stuff is really confusing. Is
> there nothing we can do about that?
OK, enum value of lp3943_pwm_output can be changed as below
because LP3943_PWM_INVALID is not used anymore.
enum lp3943_pwm_output {
LP3943_PWM_OUT0,
LP3943_PWM_OUT1,
...
LP3943_PWM_OUT15,
};
Then, output index will match each enum integer value.
Does it make sense?
> > +static const struct i2c_device_id lp3943_ids[] = {
> > + { "lp3943", 0 },
> > + { }
> > +};
> > +MODULE_DEVICE_TABLE(i2c, lp3943_ids);
>
> Are we expecting this driver to support more devices?
At this moment, this is the only one device supported.
> > +static int __init lp3943_init(void)
> > +{
> > + return i2c_add_driver(&lp3943_driver);
> > +}
> > +subsys_initcall(lp3943_init);
> > +
> > +static void __exit lp3943_exit(void)
> > +{
> > + i2c_del_driver(&lp3943_driver);
> > +}
> > +module_exit(lp3943_exit);
>
> I think you want to replace:
> lp3943_init()
> lp3943_exit
>
> With:
> module_i2c_driver()
This is related with initcall sequence.
Some problem may happen if any GPIO or PWM consumer tries to request before
LP3943 MFDs are added.
For example, a GPIO is requested in _probe() of some device.
Let's assume the GPIO number is in range of what LP3943 GPIO driver provided.
Then, gpio_request() will be failed because the GPIO is invalid at this moment.
If the device request again later, it will be OK, but we can't expect this
situation for every driver.
Some drivers request a GPIO only once in _probe(), other devices may request
a GPIO in some cases.
So, I think lp3943_init() should be defined as subsys_initcall() instead of
module_init().
Best regards,
Milo -
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?
On Wed, 31 Jul 2013, Kim, Milo wrote:
> Thanks for the review, please see my comments.
>
> > <snip> * looks good up to me up to here *
> >
> > Although, I think the 0 = 1, 1 = 2 ... stuff is really confusing. Is
> > there nothing we can do about that?
>
> OK, enum value of lp3943_pwm_output can be changed as below
> because LP3943_PWM_INVALID is not used anymore.
>
> enum lp3943_pwm_output {
> LP3943_PWM_OUT0,
> LP3943_PWM_OUT1,
> ...
> LP3943_PWM_OUT15,
> };
>
> Then, output index will match each enum integer value.
> Does it make sense?
Not really. IIRC the documentation said that LED0 (which I believe
you're calling OUT0 here) is located at pin one. So your enum above is
now incorrect isn't it? As *_OUT0 will be 0 and not 1? Or am I missing
something?
> > > +static int __init lp3943_init(void)
> > > +{
> > > + return i2c_add_driver(&lp3943_driver);
> > > +}
> > > +subsys_initcall(lp3943_init);
> > > +
> > > +static void __exit lp3943_exit(void)
> > > +{
> > > + i2c_del_driver(&lp3943_driver);
> > > +}
> > > +module_exit(lp3943_exit);
> >
> > I think you want to replace:
> > lp3943_init()
> > lp3943_exit
> >
> > With:
> > module_i2c_driver()
>
> This is related with initcall sequence.
> Some problem may happen if any GPIO or PWM consumer tries to request before
> LP3943 MFDs are added.
> For example, a GPIO is requested in _probe() of some device.
> Let's assume the GPIO number is in range of what LP3943 GPIO driver provided.
> Then, gpio_request() will be failed because the GPIO is invalid at this moment.
> If the device request again later, it will be OK, but we can't expect this
> situation for every driver.
> Some drivers request a GPIO only once in _probe(), other devices may request
> a GPIO in some cases.
> So, I think lp3943_init() should be defined as subsys_initcall() instead of
> module_init().
No I don't think so. Instead, you should use -EPROBE_DEFER in lieu of
messing around with initialisation orders.
--
Lee Jones
Linaro ST-Ericsson Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
> > > Although, I think the 0 = 1, 1 = 2 ... stuff is really confusing. Is
> > > there nothing we can do about that?
> >
> > OK, enum value of lp3943_pwm_output can be changed as below because
> > LP3943_PWM_INVALID is not used anymore.
> >
> > enum lp3943_pwm_output {
> > LP3943_PWM_OUT0,
> > LP3943_PWM_OUT1,
> > ...
> > LP3943_PWM_OUT15,
> > };
> >
> > Then, output index will match each enum integer value.
> > Does it make sense?
>
> Not really. IIRC the documentation said that LED0 (which I believe you're
> calling OUT0 here) is located at pin one. So your enum above is now incorrect
> isn't it? As *_OUT0 will be 0 and not 1? Or am I missing something?
If we consider this naming as the pin control description, it maybe confusing.
However, this enum type means configurable platform data which output channel(s)
are connected to LP3943 PWM controller.
I've changed this name from _PWM_LEDx to _PWM_OUTx in the second patch because
PWM is used for not only LED function but also other operations.
Zero base index notation is derived from the datasheet.
If I remove LP3943_PWM_INVALID, then each enum type matches with
register index(or offset) exactly. (But I need to fix LP3943 PWM driver)
In the meantime, I've reviewed the pin control subsystem,
I think it is not the best way to implement LP3943 driver.
The GPIO controller is OK, but I can't make flexible pin assignment for the PWM
operation.
For example, multiple output pins can be controlled by one PWM generator.
These pin assignment are configurable - not fixed type.
And pinmux are only two cases - GPIO and PWM.
I think current driver structure is better because LP3943 uses very limited
pinctrl functionalities.
Any suggestion for this?
> > > > +static int __init lp3943_init(void) {
> > > > + return i2c_add_driver(&lp3943_driver); }
> > > > +subsys_initcall(lp3943_init);
> > > > +
> > > > +static void __exit lp3943_exit(void) {
> > > > + i2c_del_driver(&lp3943_driver);
> > > > +}
> > > > +module_exit(lp3943_exit);
> > >
> > > I think you want to replace:
> > > lp3943_init()
> > > lp3943_exit
> > >
> > > With:
> > > module_i2c_driver()
> >
> > This is related with initcall sequence.
> > Some problem may happen if any GPIO or PWM consumer tries to request
> > before
> > LP3943 MFDs are added.
> > For example, a GPIO is requested in _probe() of some device.
> > Let's assume the GPIO number is in range of what LP3943 GPIO driver
> provided.
> > Then, gpio_request() will be failed because the GPIO is invalid at this
> moment.
> > If the device request again later, it will be OK, but we can't expect
> > this situation for every driver.
> > Some drivers request a GPIO only once in _probe(), other devices may
> > request a GPIO in some cases.
> > So, I think lp3943_init() should be defined as subsys_initcall()
> > instead of module_init().
>
> No I don't think so. Instead, you should use -EPROBE_DEFER in lieu of messing
> around with initialisation orders.
OK, got it. I'll replace them with module_i2c_driver(). Thanks!
Best Regards,
Milo
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?
> > > > Although, I think the 0 = 1, 1 = 2 ... stuff is really confusing. Is
> > > > there nothing we can do about that?
> > >
> > > OK, enum value of lp3943_pwm_output can be changed as below because
> > > LP3943_PWM_INVALID is not used anymore.
> > >
> > > enum lp3943_pwm_output {
> > > LP3943_PWM_OUT0,
> > > LP3943_PWM_OUT1,
> > > ...
> > > LP3943_PWM_OUT15,
> > > };
> > >
> > > Then, output index will match each enum integer value.
> > > Does it make sense?
> >
> > Not really. IIRC the documentation said that LED0 (which I believe you're
> > calling OUT0 here) is located at pin one. So your enum above is now incorrect
> > isn't it? As *_OUT0 will be 0 and not 1? Or am I missing something?
>
> If we consider this naming as the pin control description, it maybe confusing.
> However, this enum type means configurable platform data which output channel(s)
> are connected to LP3943 PWM controller.
>
> I've changed this name from _PWM_LEDx to _PWM_OUTx in the second patch because
> PWM is used for not only LED function but also other operations.
> Zero base index notation is derived from the datasheet.
> If I remove LP3943_PWM_INVALID, then each enum type matches with
> register index(or offset) exactly. (But I need to fix LP3943 PWM driver)
>
> In the meantime, I've reviewed the pin control subsystem,
> I think it is not the best way to implement LP3943 driver.
> The GPIO controller is OK, but I can't make flexible pin assignment for the PWM
> operation.
> For example, multiple output pins can be controlled by one PWM generator.
> These pin assignment are configurable - not fixed type.
> And pinmux are only two cases - GPIO and PWM.
> I think current driver structure is better because LP3943 uses very limited
> pinctrl functionalities.
> Any suggestion for this?
This is Linus Walleij's (CC'ed) domain.
> > > > > +static int __init lp3943_init(void) {
> > > > > + return i2c_add_driver(&lp3943_driver); }
> > > > > +subsys_initcall(lp3943_init);
> > > > > +
> > > > > +static void __exit lp3943_exit(void) {
> > > > > + i2c_del_driver(&lp3943_driver);
> > > > > +}
> > > > > +module_exit(lp3943_exit);
> > > >
> > > > I think you want to replace:
> > > > lp3943_init()
> > > > lp3943_exit
> > > >
> > > > With:
> > > > module_i2c_driver()
> > >
> > > This is related with initcall sequence.
> > > Some problem may happen if any GPIO or PWM consumer tries to request
> > > before
> > > LP3943 MFDs are added.
> > > For example, a GPIO is requested in _probe() of some device.
> > > Let's assume the GPIO number is in range of what LP3943 GPIO driver
> > provided.
> > > Then, gpio_request() will be failed because the GPIO is invalid at this
> > moment.
> > > If the device request again later, it will be OK, but we can't expect
> > > this situation for every driver.
> > > Some drivers request a GPIO only once in _probe(), other devices may
> > > request a GPIO in some cases.
> > > So, I think lp3943_init() should be defined as subsys_initcall()
> > > instead of module_init().
> >
> > No I don't think so. Instead, you should use -EPROBE_DEFER in lieu of messing
> > around with initialisation orders.
>
> OK, got it. I'll replace them with module_i2c_driver(). Thanks!
>
> Best Regards,
> Milo
>
--
Lee Jones
Linaro ST-Ericsson Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
On Mon, Aug 5, 2013 at 9:09 AM, Kim, Milo <[email protected]> wrote:
> In the meantime, I've reviewed the pin control subsystem,
> I think it is not the best way to implement LP3943 driver.
> The GPIO controller is OK, but I can't make flexible pin assignment for the PWM
> operation.
> For example, multiple output pins can be controlled by one PWM generator.
> These pin assignment are configurable - not fixed type.
So it's more like a router than a PWM goes out on a certain pin,
you can route the same PWM output to e.g. all pins if you want
to, and each pin will have its own driver stage?
> And pinmux are only two cases - GPIO and PWM.
> I think current driver structure is better because LP3943 uses very limited
> pinctrl functionalities.
> Any suggestion for this?
Well if you'r going to do things like remuxing this dynamically
you'd use the pinctrl subsystem. But if you only want to pass
a certain static configuration from e.g. platform data or the device
tree, it can be done like this.
Yours,
Linus Walleij