Messages in this thread Patch in this message | | | Date | Wed, 17 Jul 2013 12:20:33 +0100 | From | Lee Jones <> | Subject | Re: [PATCH 1/3] mfd: add LP3943 MFD driver |
| |
This driver is heavily regmap based, so I think it would make sense for Mark Brown to glance over it briefly.
> LP3943 has 16 output LED channels which can be used as GPIO expander and > PWM generators. > This patch supports the MFD structure of those features. > > * Regmap I2C interface for R/W LP3943 registers > > * Device tree bindings updated > PWM generator output ports can be defined in the platform data. > Those are configurable with the DT structure as well. > > Signed-off-by: Milo Kim <milo.kim@ti.com> > --- > Documentation/devicetree/bindings/mfd/lp3943.txt | 23 +++ > drivers/mfd/Kconfig | 8 + > drivers/mfd/Makefile | 1 + > drivers/mfd/lp3943.c | 206 ++++++++++++++++++++++ > include/linux/mfd/lp3943.h | 98 ++++++++++ > 5 files changed, 336 insertions(+) > create mode 100644 Documentation/devicetree/bindings/mfd/lp3943.txt > create mode 100644 drivers/mfd/lp3943.c > create mode 100644 include/linux/mfd/lp3943.h > > diff --git a/Documentation/devicetree/bindings/mfd/lp3943.txt b/Documentation/devicetree/bindings/mfd/lp3943.txt > new file mode 100644 > index 0000000..4eb251d > --- /dev/null > +++ b/Documentation/devicetree/bindings/mfd/lp3943.txt > @@ -0,0 +1,23 @@ > +Bindings for TI/National Semiconductor LP3943 Driver > + > +Required properties: > + - compatible: "ti,lp3943" > + - reg: 7bit I2C slave address. 0x60 ~ 0x67 > + > +Optional properties: > + - ti,pwm0, ti,pwm1: Output channel definition for PWM port 0 and 1 > + 0 = invalid, 1 = LED0, 2 = LED 1, ... 16 = LED15 No space here ^ ^ comma here
This is actually pretty confusing.
Any way you can put the invalid entry at the end so, 0 == LED0?
> +Datasheet > + http://www.ti.com/lit/ds/snvs256b/snvs256b.pdf
Ah, I see.... So the above are pins, rather than arbitrary channel Nos.
Would it make sense to use pinctrl instead then?
> +Application note: How to use LP3943 as a GPIO expander and PWM generator > + http://www.ti.com/lit/an/snva287a/snva287a.pdf > + > +Example: > + > + lp3943@60 { > + compatible = "ti,lp3943"; > + reg = <0x60>; > + pwm1 = /bits/ 8 <2>; /* use LED1 output as PWM #1 */ > + }; > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig > index ff553ba..cf9b943 100644 > --- a/drivers/mfd/Kconfig > +++ b/drivers/mfd/Kconfig > @@ -506,6 +506,14 @@ 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 > + TI LP3943 supports a GPIO expander and two PWM generators. > + > 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..6d31066 > --- /dev/null > +++ b/drivers/mfd/lp3943.c > @@ -0,0 +1,206 @@ > +/* > + * TI/National Semiconductor LP3943 MFD Core Driver > + * > + * Copyright 2013 Texas Instruments > + * > + * Author: Milo(Woogyom) Kim <milo.kim@ti.com> > + * > + * 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/of_gpio.h> > +#include <linux/slab.h> > + > +#define LP3943_MAX_REGISTERS 0x09 > + > +/* Register configuration for pin MUX */ > +static const struct lp3943_reg_cfg lp3943_mux_cfg[16] = { > + /* 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", > + }, > + { > + .name = "lp3943-gpio", > + }, > +}; > + > +int lp3943_read_byte(struct lp3943 *l, u8 reg, u8 *read) Not sure I like 'l' as a variable name. The function is small enough to get away with it in this context, but it would probably be better if it were renamed to something more meaningful.
> +{ > + int ret; > + unsigned int val; > + > + ret = regmap_read(l->regmap, reg, &val); > + if (ret < 0) { > + dev_err(l->dev, "failed to read 0x%.2x\n", reg); > + return ret; > + } > + > + *read = (u8)val; > + return 0; > +} > +EXPORT_SYMBOL_GPL(lp3943_read_byte); > + > +int lp3943_write_byte(struct lp3943 *l, u8 reg, u8 data) > +{ > + return regmap_write(l->regmap, reg, data); > +} > +EXPORT_SYMBOL_GPL(lp3943_write_byte); > + > +int lp3943_update_bits(struct lp3943 *l, u8 reg, u8 mask, u8 data) > +{ > + return regmap_update_bits(l->regmap, reg, mask, data); > +} > +EXPORT_SYMBOL_GPL(lp3943_update_bits); > + > +#ifdef CONFIG_OF > +static int lp3943_parse_dt(struct device *dev, struct device_node *node) > +{ > + struct lp3943_platform_data *pdata; > + u8 port = 0; > + > + if (!node) { > + dev_err(dev, "no platform data\n"); > + return -EINVAL; > + } > + > + pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL); > + if (!pdata) > + return -ENOMEM; > + > + of_property_read_u8(node, "ti,pwm0", &port); > + pdata->pwm0 = (enum lp3943_pwm_output)port; > + port = 0; > + of_property_read_u8(node, "ti,pwm1", &port); > + pdata->pwm1 = (enum lp3943_pwm_output)port; > + > + dev->platform_data = pdata; > + return 0; > +} > +#else > +static int lp3943_parse_dt(struct device *dev, struct device_node *node) > +{ > + return -EINVAL; > +} > +#endif > + > +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_platform_data *pdata; > + struct lp3943 *l; > + int err; > + > + if (!cl->dev.platform_data) { > + err = lp3943_parse_dt(&cl->dev, cl->dev.of_node); > + if (err) > + return err; > + } > + > + pdata = cl->dev.platform_data; > + > + l = devm_kzalloc(&cl->dev, sizeof(struct lp3943), GFP_KERNEL); > + if (!l) > + return -ENOMEM; > + > + l->regmap = devm_regmap_init_i2c(cl, &lp3943_regmap_config); > + if (IS_ERR(l->regmap)) { > + err = PTR_ERR(l->regmap); > + dev_err(&cl->dev, "regmap init i2c err: %d\n", err); > + return err; > + } > + > + l->pdata = pdata; > + l->dev = &cl->dev; > + l->mux_cfg = lp3943_mux_cfg; > + i2c_set_clientdata(cl, l); > + > + err = mfd_add_devices(l->dev, -1, lp3943_devs, ARRAY_SIZE(lp3943_devs), > + NULL, 0, NULL); > + if (err) { > + dev_err(l->dev, "add device err: %d\n", err); > + return err; > + } > + > + return 0; > +} > + > +static int lp3943_remove(struct i2c_client *cl) > +{ > + struct lp3943 *l = i2c_get_clientdata(cl); > + > + mfd_remove_devices(l->dev); > + return 0; > +} > + > +static const struct i2c_device_id lp3943_ids[] = { > + {"lp3943", 0},
Lack of consistency ...
> + { } > +}; > +MODULE_DEVICE_TABLE(i2c, lp3943_ids); > + > +static const struct of_device_id lp3943_dt_ids[] = { > + { .compatible = "ti,lp3943", },
... with this one. Personally, I prefer this one.
> + { } > +}; > +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("TI 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..208fc73 > --- /dev/null > +++ b/include/linux/mfd/lp3943.h > @@ -0,0 +1,98 @@ > +/* > + * TI/National Semiconductor LP3943 Device > + * > + * Copyright 2013 Texas Instruments > + * > + * Author: Milo(Woogyom) Kim <milo.kim@ti.com> > + * > + * 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_LED0, > + LP3943_PWM_LED1, > + LP3943_PWM_LED2, > + LP3943_PWM_LED3, > + LP3943_PWM_LED4, > + LP3943_PWM_LED5, > + LP3943_PWM_LED6, > + LP3943_PWM_LED7, > + LP3943_PWM_LED8, > + LP3943_PWM_LED9, > + LP3943_PWM_LED10, > + LP3943_PWM_LED11, > + LP3943_PWM_LED12, > + LP3943_PWM_LED13, > + LP3943_PWM_LED14, > + LP3943_PWM_LED15, > +}; > + > +/** > + * struct lp3943_platform_data > + * @pwm0: LED output channel definition for PWM 0 port > + * @pwm1: LED output channel definition for PWM 1 port > + */ > +struct lp3943_platform_data { > + enum lp3943_pwm_output pwm0; > + enum lp3943_pwm_output 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 communcation on accessing registers > + * @pdata: LP3943 platform specific data > + */ > +struct lp3943 { > + struct device *dev; > + struct regmap *regmap; > + struct lp3943_platform_data *pdata; > + const struct lp3943_reg_cfg *mux_cfg; > +}; > + > +int lp3943_read_byte(struct lp3943 *lm, u8 reg, u8 *read); > +int lp3943_write_byte(struct lp3943 *lm, u8 reg, u8 data); > +int lp3943_update_bits(struct lp3943 *lm, u8 reg, u8 mask, u8 data); > +#endif
-- Lee Jones Linaro ST-Ericsson Landing Team Lead Linaro.org │ Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |