| Message ID | d6c7de200c9447dbafa74ee76739da943a0b73b2.1409145187.git.Adam.Thomson.Opensource@diasemi.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
On 08/28/2014 04:18 PM, Adam Thomson wrote: (...) > +static int da9150_probe(struct i2c_client *client, > + const struct i2c_device_id *id) > +{ > + struct da9150 *da9150; > + int ret; > + > + da9150 = devm_kzalloc(&client->dev, sizeof(struct da9150), GFP_KERNEL); > + if (da9150 == NULL) > + return -ENOMEM; da9150 = devm_kzalloc(&client->dev, sizeof(*da9150), GFP_KERNEL); if (!da9150) return -ENOMEM; > + da9150->dev = &client->dev; > + da9150->irq = client->irq; > + i2c_set_clientdata(client, da9150); > + dev_set_drvdata(da9150->dev, da9150); > + > + da9150->regmap = devm_regmap_init_i2c(client, &da9150_regmap_config); > + if (IS_ERR(da9150->regmap)) { > + ret = PTR_ERR(da9150->regmap); > + dev_err(da9150->dev, "Failed to allocate register map: %d\n", > + ret); > + return ret; > + } > + > + return da9150_device_init(da9150); > +} > + > +static int da9150_remove(struct i2c_client *client) > +{ > + struct da9150 *da9150 = i2c_get_clientdata(client); > + > + da9150_device_exit(da9150); > + > + return 0; > +} > + > +static void da9150_shutdown(struct i2c_client *client) > +{ > + struct da9150 *da9150 = i2c_get_clientdata(client); > + > + da9150_device_shutdown(da9150); > +} > + > +static const struct i2c_device_id da9150_i2c_id[] = { > + { "da9150", 0 }, > + { } > +}; > +MODULE_DEVICE_TABLE(i2c, da9150_i2c_id); > + > +static const struct of_device_id da9150_of_match[] = { > + { .compatible = "dlg,da9150", }, > + { } > +}; > + missed MODULE_DEVICE_TABLE(of, ...) ? > +static struct i2c_driver da9150_driver = { > + .driver = { > + .name = "da9150", > + .owner = THIS_MODULE, No need to update this field... > + .of_match_table = of_match_ptr(da9150_of_match), > + }, > + .probe = da9150_probe, > + .remove = da9150_remove, > + .shutdown = da9150_shutdown, > + .id_table = da9150_i2c_id, > +}; > + > +module_i2c_driver(da9150_driver); > + > +MODULE_DESCRIPTION("I2C Driver for DA9150"); > +MODULE_AUTHOR("Adam Thomson <Adam.Thomson.Opensource@diasemi.com"); > +MODULE_LICENSE("GPL"); >
On Thu, 28 Aug 2014, Adam Thomson wrote: > DA9150 is a combined Charger and Fuel-Gauge IC, with additional > GPIO and GPADC functionality. > > Signed-off-by: Adam Thomson <Adam.Thomson.Opensource@diasemi.com> > --- > drivers/mfd/Kconfig | 12 + > drivers/mfd/Makefile | 2 + > drivers/mfd/da9150-core.c | 332 ++++++++++ > drivers/mfd/da9150-i2c.c | 176 ++++++ Do you also have another, say SPI version? > include/linux/mfd/da9150/core.h | 80 +++ > include/linux/mfd/da9150/pdata.h | 21 + > include/linux/mfd/da9150/registers.h | 1153 ++++++++++++++++++++++++++++++++++ > 7 files changed, 1776 insertions(+) > create mode 100644 drivers/mfd/da9150-core.c > create mode 100644 drivers/mfd/da9150-i2c.c > create mode 100644 include/linux/mfd/da9150/core.h > create mode 100644 include/linux/mfd/da9150/pdata.h > create mode 100644 include/linux/mfd/da9150/registers.h > > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig > index de5abf2..76adb2c 100644 > --- a/drivers/mfd/Kconfig > +++ b/drivers/mfd/Kconfig > @@ -183,6 +183,18 @@ config MFD_DA9063 > Additional drivers must be enabled in order to use the functionality > of the device. > > +config MFD_DA9150 > + bool "Dialog Semiconductor DA9150 Charger Fuel-Gauge chip" Why can't this be built as a module? > + depends on I2C=y > + select MFD_CORE > + select REGMAP_I2C > + select REGMAP_IRQ > + help > + This adds support for the DA9150 integrated charger and fuel-gauge > + chip. This driver provides common support for accessing the device. > + Additional drivers must be enabled in order to use the specific > + features of the device. > + > config MFD_MC13XXX > tristate > depends on (SPI_MASTER || I2C) > diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile > index f001487..098dfa1 100644 > --- a/drivers/mfd/Makefile > +++ b/drivers/mfd/Makefile > @@ -114,6 +114,8 @@ obj-$(CONFIG_MFD_DA9055) += da9055.o > da9063-objs := da9063-core.o da9063-irq.o da9063-i2c.o > obj-$(CONFIG_MFD_DA9063) += da9063.o > > +obj-$(CONFIG_MFD_DA9150) += da9150-core.o da9150-i2c.o > + Do the other drivers smell? Please butt up against them. I'm not entirely sure why there are so many '\n's in the Makefile! > obj-$(CONFIG_MFD_MAX14577) += max14577.o > obj-$(CONFIG_MFD_MAX77686) += max77686.o > obj-$(CONFIG_MFD_MAX77693) += max77693.o > diff --git a/drivers/mfd/da9150-core.c b/drivers/mfd/da9150-core.c > new file mode 100644 > index 0000000..029a30b > --- /dev/null > +++ b/drivers/mfd/da9150-core.c > @@ -0,0 +1,332 @@ > +/* > + * DA9150 Core MFD Driver > + * > + * Copyright (c) 2014 Dialog Semiconductor > + * > + * Author: Adam Thomson <Adam.Thomson.Opensource@diasemi.com> > + * > + * This program is free software; you can redistribute it and/or modify it > + * under the terms of the GNU General Public License as published by the > + * Free Software Foundation; either version 2 of the License, or (at your > + * option) any later version. > + */ > + > +#include <linux/kernel.h> > +#include <linux/module.h> > +#include <linux/platform_device.h> > +#include <linux/slab.h> > +#include <linux/irq.h> > +#include <linux/interrupt.h> > +#include <linux/mfd/core.h> > + No real need for this '\n'. > +#include <linux/mfd/da9150/core.h> > +#include <linux/mfd/da9150/registers.h> > +#include <linux/mfd/da9150/pdata.h> > + > +u8 da9150_reg_read(struct da9150 *da9150, u16 reg) > +{ > + int val, ret; > + > + ret = regmap_read(da9150->regmap, reg, &val); > + if (ret < 0) What if ret > 0? Is that a good thing? :) Just 'if (ret)'. > + dev_err(da9150->dev, "Failed to read from reg 0x%x: %d\n", > + reg, ret); > + > + return (u8) val; > +} > +EXPORT_SYMBOL_GPL(da9150_reg_read); Not sure I like this abstraction stuff. I could understand if there were locking involved, but there isn't. You don't appear to check for errors in the subordinate drivers either, rather you just plough on ahead. Not sure that's a good idea either. Anyone have a second opinion? > +int da9150_reg_write(struct da9150 *da9150, u16 reg, u8 val) > +{ > + int ret; > + > + ret = regmap_write(da9150->regmap, reg, val); > + if (ret < 0) > + dev_err(da9150->dev, "Failed to write to reg 0x%x: %d\n", > + reg, ret); > + > + return ret; > +} > +EXPORT_SYMBOL_GPL(da9150_reg_write); Blah. > +int da9150_set_bits(struct da9150 *da9150, u16 reg, u8 mask, u8 val) > +{ > + int ret; > + > + ret = regmap_update_bits(da9150->regmap, reg, mask, val); > + if (ret < 0) > + dev_err(da9150->dev, "Failed to set bits in reg 0x%x: %d\n", > + reg, ret); > + > + return ret; > +} > +EXPORT_SYMBOL_GPL(da9150_set_bits); Blah. > +int da9150_bulk_read(struct da9150 *da9150, u16 reg, int count, u8 *buf) > +{ > + int ret; > + > + ret = regmap_bulk_read(da9150->regmap, reg, buf, count); > + if (ret < 0) > + dev_err(da9150->dev, "Failed to bulk read from reg 0x%x: %d\n", > + reg, ret); > + > + return ret; > +} > +EXPORT_SYMBOL_GPL(da9150_bulk_read); Blah. > +int da9150_bulk_write(struct da9150 *da9150, u16 reg, int count, const u8 *buf) > +{ > + int ret; > + > + ret = regmap_raw_write(da9150->regmap, reg, buf, count); > + if (ret < 0) > + dev_err(da9150->dev, "Failed to bulk write to reg 0x%x %d\n", > + reg, ret); > + > + return ret; > +} > +EXPORT_SYMBOL_GPL(da9150_bulk_write); Blah. > +static struct regmap_irq da9150_irqs[] = { > + [DA9150_IRQ_VBUS] = { > + .reg_offset = 0, > + .mask = DA9150_E_VBUS_MASK, > + }, > + [DA9150_IRQ_CHG] = { > + .reg_offset = 0, > + .mask = DA9150_E_CHG_MASK, > + }, > + [DA9150_IRQ_TCLASS] = { > + .reg_offset = 0, > + .mask = DA9150_E_TCLASS_MASK, > + }, > + [DA9150_IRQ_TJUNC] = { > + .reg_offset = 0, > + .mask = DA9150_E_TJUNC_MASK, > + }, > + [DA9150_IRQ_VFAULT] = { > + .reg_offset = 0, > + .mask = DA9150_E_VFAULT_MASK, > + }, > + [DA9150_IRQ_CONF] = { > + .reg_offset = 1, > + .mask = DA9150_E_CONF_MASK, > + }, > + [DA9150_IRQ_DAT] = { > + .reg_offset = 1, > + .mask = DA9150_E_DAT_MASK, > + }, > + [DA9150_IRQ_DTYPE] = { > + .reg_offset = 1, > + .mask = DA9150_E_DTYPE_MASK, > + }, > + [DA9150_IRQ_ID] = { > + .reg_offset = 1, > + .mask = DA9150_E_ID_MASK, > + }, > + [DA9150_IRQ_ADP] = { > + .reg_offset = 1, > + .mask = DA9150_E_ADP_MASK, > + }, > + [DA9150_IRQ_SESS_END] = { > + .reg_offset = 1, > + .mask = DA9150_E_SESS_END_MASK, > + }, > + [DA9150_IRQ_SESS_VLD] = { > + .reg_offset = 1, > + .mask = DA9150_E_SESS_VLD_MASK, > + }, > + [DA9150_IRQ_FG] = { > + .reg_offset = 2, > + .mask = DA9150_E_FG_MASK, > + }, > + [DA9150_IRQ_GP] = { > + .reg_offset = 2, > + .mask = DA9150_E_GP_MASK, > + }, > + [DA9150_IRQ_TBAT] = { > + .reg_offset = 2, > + .mask = DA9150_E_TBAT_MASK, > + }, > + [DA9150_IRQ_GPIOA] = { > + .reg_offset = 2, > + .mask = DA9150_E_GPIOA_MASK, > + }, > + [DA9150_IRQ_GPIOB] = { > + .reg_offset = 2, > + .mask = DA9150_E_GPIOB_MASK, > + }, > + [DA9150_IRQ_GPIOC] = { > + .reg_offset = 2, > + .mask = DA9150_E_GPIOC_MASK, > + }, > + [DA9150_IRQ_GPIOD] = { > + .reg_offset = 2, > + .mask = DA9150_E_GPIOD_MASK, > + }, > + [DA9150_IRQ_GPADC] = { > + .reg_offset = 2, > + .mask = DA9150_E_GPADC_MASK, > + }, > + [DA9150_IRQ_WKUP] = { > + .reg_offset = 3, > + .mask = DA9150_E_WKUP_MASK, > + }, > +}; > + > +static struct regmap_irq_chip da9150_regmap_irq_chip = { > + .name = "da9150_irq", > + .status_base = DA9150_EVENT_E, > + .mask_base = DA9150_IRQ_MASK_E, > + .ack_base = DA9150_EVENT_E, > + .num_regs = DA9150_NUM_IRQ_REGS, > + .irqs = da9150_irqs, > + .num_irqs = ARRAY_SIZE(da9150_irqs), > +}; > + > +/* Helper functions for sub-devices to request/free IRQs */ > +int da9150_register_irq(struct platform_device *pdev, void *dev_id, > + irq_handler_t handler, const char *name) > +{ > + int irq, ret; > + > + irq = platform_get_irq_byname(pdev, name); > + if (irq < 0) > + return irq; > + > + ret = devm_request_threaded_irq(&pdev->dev, irq, NULL, handler, > + IRQF_ONESHOT, name, dev_id); > + if (ret) > + dev_err(&pdev->dev, "Failed to request IRQ %d: %d\n", irq, ret); > + > + return ret; > +} > +EXPORT_SYMBOL_GPL(da9150_register_irq); Why do they need help? What problem does adding these layers solve? > +void da9150_release_irq(struct platform_device *pdev, void *dev_id, > + const char *name) > +{ > + int irq; > + > + irq = platform_get_irq_byname(pdev, name); > + if (irq < 0) > + return; > + > + devm_free_irq(&pdev->dev, irq, dev_id); > +} > +EXPORT_SYMBOL_GPL(da9150_release_irq); Do you ever release the IRQ and not unbind the driver? Are there ordering issues at play here? If not, there's no need to conduct a manual free. > +static struct resource da9150_gpadc_resources[] = { > + { > + .name = "GPADC", > + .start = DA9150_IRQ_GPADC, > + .end = DA9150_IRQ_GPADC, > + .flags = IORESOURCE_IRQ, > + }, > +}; > + > +static struct resource da9150_charger_resources[] = { > + { > + .name = "CHG_STATUS", > + .start = DA9150_IRQ_CHG, > + .end = DA9150_IRQ_CHG, > + .flags = IORESOURCE_IRQ, > + }, > + { > + .name = "CHG_TJUNC", > + .start = DA9150_IRQ_TJUNC, > + .end = DA9150_IRQ_TJUNC, > + .flags = IORESOURCE_IRQ, > + }, > + { > + .name = "CHG_VFAULT", > + .start = DA9150_IRQ_VFAULT, > + .end = DA9150_IRQ_VFAULT, > + .flags = IORESOURCE_IRQ, > + }, > + { > + .name = "CHG_VBUS", > + .start = DA9150_IRQ_VBUS, > + .end = DA9150_IRQ_VBUS, > + .flags = IORESOURCE_IRQ, > + }, > +}; > + > +static struct mfd_cell da9150_devs[] = { > + { > + .name = "da9150-gpadc", > + .of_compatible = "dlg,da9150-gpadc", > + .resources = da9150_gpadc_resources, > + .num_resources = ARRAY_SIZE(da9150_gpadc_resources), > + }, > + { > + .name = "da9150-charger", > + .of_compatible = "dlg,da9150-charger", > + .resources = da9150_charger_resources, > + .num_resources = ARRAY_SIZE(da9150_charger_resources), > + }, > +}; > + > +int da9150_device_init(struct da9150 *da9150) > +{ > + struct da9150_pdata *pdata = da9150->dev->platform_data; dev_get_platdata() > + int ret; > + > + /* Handle platform data */ This comment doesn't add anything - the code is clear enough. > + if (pdata) > + da9150->irq_base = pdata->irq_base; > + else > + da9150->irq_base = -1; pdata ? pdata->irq_base : -1; > + /* Init IRQs */ No need for these, please only add comments where the code is complex or convoluted. > + ret = regmap_add_irq_chip(da9150->regmap, da9150->irq, > + IRQF_TRIGGER_LOW | IRQF_ONESHOT, > + da9150->irq_base, &da9150_regmap_irq_chip, > + &da9150->regmap_irq_data); > + if (ret < 0) 'if (ret)' where positive replies aren't possible or are errors. > + goto err_irq; Just return here and remove 'err_irq' label. > + da9150->irq_base = regmap_irq_chip_get_base(da9150->regmap_irq_data); > + > + /* Make the IRQ line a wake source */ > + enable_irq_wake(da9150->irq); > + > + /* Add MFD Devices */ > + ret = mfd_add_devices(da9150->dev, -1, da9150_devs, > + ARRAY_SIZE(da9150_devs), NULL, > + da9150->irq_base, NULL); > + if (ret < 0) { > + dev_err(da9150->dev, "Failed to add child devices: %d\n", ret); > + goto err_mfd; > + } > + > + return 0; > + > +err_mfd: > + regmap_del_irq_chip(da9150->irq, da9150->regmap_irq_data); > +err_irq: > + return ret; > +} > + > +void da9150_device_exit(struct da9150 *da9150) > +{ > + regmap_del_irq_chip(da9150->irq, da9150->regmap_irq_data); > + mfd_remove_devices(da9150->dev); > +} > + > +void da9150_device_shutdown(struct da9150 *da9150) > +{ > + /* Make sure we have a wakup source for the device */ > + da9150_set_bits(da9150, DA9150_CONFIG_D, > + DA9150_WKUP_PM_EN_MASK, > + DA9150_WKUP_PM_EN_MASK); > + > + /* Set device to DISABLED mode */ > + da9150_set_bits(da9150, DA9150_CONTROL_C, > + DA9150_DISABLE_MASK, DA9150_DISABLE_MASK); > +} > + > +MODULE_DESCRIPTION("MFD Core Driver for DA9150"); > +MODULE_AUTHOR("Adam Thomson <Adam.Thomson.Opensource@diasemi.com"); > +MODULE_LICENSE("GPL"); > diff --git a/drivers/mfd/da9150-i2c.c b/drivers/mfd/da9150-i2c.c > new file mode 100644 > index 0000000..a02525c > --- /dev/null > +++ b/drivers/mfd/da9150-i2c.c > @@ -0,0 +1,176 @@ > +/* > + * DA9150 I2C Driver > + * > + * Copyright (c) 2014 Dialog Semiconductor > + * > + * Author: Adam Thomson <Adam.Thomson.Opensource@diasemi.com> > + * > + * This program is free software; you can redistribute it and/or modify it > + * under the terms of the GNU General Public License as published by the > + * Free Software Foundation; either version 2 of the License, or (at your > + * option) any later version. > + */ > + > +#include <linux/kernel.h> > +#include <linux/module.h> > +#include <linux/platform_device.h> > +#include <linux/i2c.h> > +#include <linux/regmap.h> > +#include <linux/slab.h> > + Remove this line. > +#include <linux/mfd/da9150/core.h> > +#include <linux/mfd/da9150/registers.h> > + > +static bool da9150_volatile_reg(struct device *dev, unsigned int reg) > +{ > + switch (reg) { > + case DA9150_PAGE_CON: > + case DA9150_STATUS_A: > + case DA9150_STATUS_B: > + case DA9150_STATUS_C: > + case DA9150_STATUS_D: > + case DA9150_STATUS_E: > + case DA9150_STATUS_F: > + case DA9150_STATUS_G: > + case DA9150_STATUS_H: > + case DA9150_STATUS_I: > + case DA9150_STATUS_J: > + case DA9150_STATUS_K: > + case DA9150_STATUS_L: > + case DA9150_STATUS_N: > + case DA9150_FAULT_LOG_A: > + case DA9150_FAULT_LOG_B: > + case DA9150_EVENT_E: > + case DA9150_EVENT_F: > + case DA9150_EVENT_G: > + case DA9150_EVENT_H: > + case DA9150_CONTROL_B: > + case DA9150_CONTROL_C: > + case DA9150_GPADC_MAN: > + case DA9150_GPADC_RES_A: > + case DA9150_GPADC_RES_B: > + case DA9150_ADETVB_CFG_C: > + case DA9150_ADETD_STAT: > + case DA9150_ADET_CMPSTAT: > + case DA9150_ADET_CTRL_A: > + case DA9150_PPR_TCTR_B: > + case DA9150_COREBTLD_STAT_A: > + case DA9150_CORE_DATA_A: > + case DA9150_CORE_DATA_B: > + case DA9150_CORE_DATA_C: > + case DA9150_CORE_DATA_D: > + case DA9150_CORE2WIRE_STAT_A: > + case DA9150_FW_CTRL_C: > + case DA9150_FG_CTRL_B: > + case DA9150_FW_CTRL_B: > + case DA9150_GPADC_CMAN: > + case DA9150_GPADC_CRES_A: > + case DA9150_GPADC_CRES_B: > + case DA9150_CC_ICHG_RES_A: > + case DA9150_CC_ICHG_RES_B: > + case DA9150_CC_IAVG_RES_A: > + case DA9150_CC_IAVG_RES_B: > + case DA9150_TAUX_CTRL_A: > + case DA9150_TAUX_VALUE_H: > + case DA9150_TAUX_VALUE_L: > + case DA9150_TBAT_RES_A: > + case DA9150_TBAT_RES_B: > + return true; > + default: > + return false; > + } > +} > + > +static const struct regmap_range_cfg da9150_range_cfg[] = { > + { > + .range_min = DA9150_PAGE_CON, > + .range_max = DA9150_TBAT_RES_B, > + .selector_reg = DA9150_PAGE_CON, > + .selector_mask = DA9150_I2C_PAGE_MASK, > + .selector_shift = DA9150_I2C_PAGE_SHIFT, > + .window_start = 0, > + .window_len = 256, > + }, > +}; > + > +static struct regmap_config da9150_regmap_config = { > + .reg_bits = 8, > + .val_bits = 8, > + .ranges = da9150_range_cfg, > + .num_ranges = ARRAY_SIZE(da9150_range_cfg), > + .max_register = DA9150_TBAT_RES_B, > + > + .cache_type = REGCACHE_RBTREE, > + > + .volatile_reg = da9150_volatile_reg, > +}; > + > +static int da9150_probe(struct i2c_client *client, > + const struct i2c_device_id *id) > +{ > + struct da9150 *da9150; > + int ret; > + > + da9150 = devm_kzalloc(&client->dev, sizeof(struct da9150), GFP_KERNEL); sizeof(*da9150) > + if (da9150 == NULL) if (!da9150) > + return -ENOMEM; '\n' > + da9150->dev = &client->dev; > + da9150->irq = client->irq; > + i2c_set_clientdata(client, da9150); > + dev_set_drvdata(da9150->dev, da9150); Why do you need this in both locations? > + da9150->regmap = devm_regmap_init_i2c(client, &da9150_regmap_config); > + if (IS_ERR(da9150->regmap)) { > + ret = PTR_ERR(da9150->regmap); > + dev_err(da9150->dev, "Failed to allocate register map: %d\n", > + ret); > + return ret; > + } > + > + return da9150_device_init(da9150); > +} > + > +static int da9150_remove(struct i2c_client *client) > +{ > + struct da9150 *da9150 = i2c_get_clientdata(client); > + > + da9150_device_exit(da9150); > + > + return 0; > +} > + > +static void da9150_shutdown(struct i2c_client *client) > +{ > + struct da9150 *da9150 = i2c_get_clientdata(client); > + > + da9150_device_shutdown(da9150); > +} > + > +static const struct i2c_device_id da9150_i2c_id[] = { > + { "da9150", 0 }, I don't see the .id parameter being used, just leave it blank. > + { } > +}; > +MODULE_DEVICE_TABLE(i2c, da9150_i2c_id); > + > +static const struct of_device_id da9150_of_match[] = { > + { .compatible = "dlg,da9150", }, > + { } > +}; MODULE_DEVICE_TABLE(of, ...) > +static struct i2c_driver da9150_driver = { > + .driver = { > + .name = "da9150", > + .owner = THIS_MODULE, You can remove this line, it's taken care of for you elsewhere. > + .of_match_table = of_match_ptr(da9150_of_match), > + }, > + .probe = da9150_probe, > + .remove = da9150_remove, > + .shutdown = da9150_shutdown, > + .id_table = da9150_i2c_id, > +}; > + > +module_i2c_driver(da9150_driver); > + > +MODULE_DESCRIPTION("I2C Driver for DA9150"); > +MODULE_AUTHOR("Adam Thomson <Adam.Thomson.Opensource@diasemi.com"); > +MODULE_LICENSE("GPL"); > diff --git a/include/linux/mfd/da9150/core.h b/include/linux/mfd/da9150/core.h > new file mode 100644 > index 0000000..d23c500 > --- /dev/null > +++ b/include/linux/mfd/da9150/core.h > @@ -0,0 +1,80 @@ > +/* > + * DA9150 MFD Driver - Core Data > + * > + * Copyright (c) 2014 Dialog Semiconductor > + * > + * Author: Adam Thomson <Adam.Thomson.Opensource@diasemi.com> > + * > + * This program is free software; you can redistribute it and/or modify it > + * under the terms of the GNU General Public License as published by the > + * Free Software Foundation; either version 2 of the License, or (at your > + * option) any later version. > + */ > + > +#ifndef __DA9150_CORE_H > +#define __DA9150_CORE_H > + > +#include <linux/device.h> > +#include <linux/i2c.h> What's this used for? > +#include <linux/interrupt.h> > +#include <linux/regmap.h> > +#include <linux/mutex.h> > + > +/* I2C address paging */ > +#define DA9150_REG_PAGE_SHIFT 8 > +#define DA9150_REG_PAGE_MASK 0xFF > + > +/* IRQs */ > +#define DA9150_NUM_IRQ_REGS 4 > +#define DA9150_IRQ_VBUS 0 > +#define DA9150_IRQ_CHG 1 > +#define DA9150_IRQ_TCLASS 2 > +#define DA9150_IRQ_TJUNC 3 > +#define DA9150_IRQ_VFAULT 4 > +#define DA9150_IRQ_CONF 5 > +#define DA9150_IRQ_DAT 6 > +#define DA9150_IRQ_DTYPE 7 > +#define DA9150_IRQ_ID 8 > +#define DA9150_IRQ_ADP 9 > +#define DA9150_IRQ_SESS_END 10 > +#define DA9150_IRQ_SESS_VLD 11 > +#define DA9150_IRQ_FG 12 > +#define DA9150_IRQ_GP 13 > +#define DA9150_IRQ_TBAT 14 > +#define DA9150_IRQ_GPIOA 15 > +#define DA9150_IRQ_GPIOB 16 > +#define DA9150_IRQ_GPIOC 17 > +#define DA9150_IRQ_GPIOD 18 > +#define DA9150_IRQ_GPADC 19 > +#define DA9150_IRQ_WKUP 20 > + > +struct da9150 { > + struct device *dev; > + > + struct regmap *regmap; > + Why the '\n's? > + struct regmap_irq_chip_data *regmap_irq_data; > + int irq; > + int irq_base; > +}; > + > +/* Device I/O */ > +u8 da9150_reg_read(struct da9150 *da9150, u16 reg); > +int da9150_reg_write(struct da9150 *da9150, u16 reg, u8 val); > +int da9150_set_bits(struct da9150 *da9150, u16 reg, u8 mask, u8 val); > + > +int da9150_bulk_read(struct da9150 *da9150, u16 reg, int count, u8 *buf); > +int da9150_bulk_write(struct da9150 *da9150, u16 reg, int count, const u8 *buf); > + > +/* IRQ helper functions */ > +int da9150_register_irq(struct platform_device *pdev, void *dev_id, > + irq_handler_t handler, const char *name); > +void da9150_release_irq(struct platform_device *pdev, void *dev_id, > + const char *name); I'm not sure why any of these 7 functions are required. > +/* Init/Exit */ > +int da9150_device_init(struct da9150 *da9150); > +void da9150_device_exit(struct da9150 *da9150); > +void da9150_device_shutdown(struct da9150 *da9150); > + > +#endif /* __DA9150_CORE_H */ > diff --git a/include/linux/mfd/da9150/pdata.h b/include/linux/mfd/da9150/pdata.h > new file mode 100644 > index 0000000..e2b37f1 > --- /dev/null > +++ b/include/linux/mfd/da9150/pdata.h > @@ -0,0 +1,21 @@ > +/* > + * DA9150 MFD Driver - Platform Data > + * > + * Copyright (c) 2014 Dialog Semiconductor > + * > + * Author: Adam Thomson <Adam.Thomson.Opensource@diasemi.com> > + * > + * This program is free software; you can redistribute it and/or modify it > + * under the terms of the GNU General Public License as published by the > + * Free Software Foundation; either version 2 of the License, or (at your > + * option) any later version. > + */ > + > +#ifndef __DA9150_PDATA_H > +#define __DA9150_PDATA_H > + > +struct da9150_pdata { > + int irq_base; > +}; Just put this in core.h and do away witht this header file. > +#endif /* __DA9150_PDATA_H */ > diff --git a/include/linux/mfd/da9150/registers.h b/include/linux/mfd/da9150/registers.h > new file mode 100644 > index 0000000..ef4826d > --- /dev/null > +++ b/include/linux/mfd/da9150/registers.h > @@ -0,0 +1,1153 @@ > +/* > + * DA9150 MFD Driver - Registers > + * > + * Copyright (c) 2014 Dialog Semiconductor > + * > + * Author: Adam Thomson <Adam.Thomson.Opensource@diasemi.com> > + * > + * This program is free software; you can redistribute it and/or modify it > + * under the terms of the GNU General Public License as published by the > + * Free Software Foundation; either version 2 of the License, or (at your > + * option) any later version. > + */ > + > +#ifndef __DA9150_REGISTERS_H > +#define __DA9150_REGISTERS_H > + > +/* Registers */ [...] > +/* DA9150_CONTROL_A = 0x0E5 */ > +#define DA9150_VDD33_SL_SHIFT 0 > +#define DA9150_VDD33_SL_MASK (0x01 << 0) > +#define DA9150_VDD33_LPM_SHIFT 1 > +#define DA9150_VDD33_LPM_MASK (0x03 << 1) > +#define DA9150_VDD33_EN_SHIFT 3 > +#define DA9150_VDD33_EN_MASK (0x01 << 3) > +#define DA9150_GPI_LPM_SHIFT 6 > +#define DA9150_GPI_LPM_MASK (0x01 << 6) > +#define DA9150_PM_IF_LPM_SHIFT 7 > +#define DA9150_PM_IF_LPM_MASK (0x01 << 7) > + > +/* DA9150_CONTROL_B = 0x0E6 */ > +#define DA9150_LPM_SHIFT 0 > +#define DA9150_LPM_MASK (0x01 << 0) > +#define DA9150_RESET_SHIFT 1 > +#define DA9150_RESET_MASK (0x01 << 1) > +#define DA9150_RESET_USRCONF_EN_SHIFT 2 > +#define DA9150_RESET_USRCONF_EN_MASK (0x01 << 2) > + > +/* DA9150_CONTROL_C = 0x0E7 */ > +#define DA9150_DISABLE_SHIFT 0 > +#define DA9150_DISABLE_MASK (0x01 << 0) Use BIT() for all of these (1 << X) macros. [...]
On August 28, 2014 12:47, Varka Bhadram wrote: > On 08/28/2014 04:18 PM, Adam Thomson wrote: > > (...) > > > +static int da9150_probe(struct i2c_client *client, > > + const struct i2c_device_id *id) > > +{ > > + struct da9150 *da9150; > > + int ret; > > + > > + da9150 = devm_kzalloc(&client->dev, sizeof(struct da9150), GFP_KERNEL); > > + if (da9150 == NULL) > > + return -ENOMEM; > > da9150 = devm_kzalloc(&client->dev, sizeof(*da9150), GFP_KERNEL); > if (!da9150) > return -ENOMEM; Ok, no real difference, but can change it. > > > + da9150->dev = &client->dev; > > + da9150->irq = client->irq; > > + i2c_set_clientdata(client, da9150); > > + dev_set_drvdata(da9150->dev, da9150); > > + > > + da9150->regmap = devm_regmap_init_i2c(client, &da9150_regmap_config); > > + if (IS_ERR(da9150->regmap)) { > > + ret = PTR_ERR(da9150->regmap); > > + dev_err(da9150->dev, "Failed to allocate register map: %d\n", > > + ret); > > + return ret; > > + } > > + > > + return da9150_device_init(da9150); > > +} > > + > > +static int da9150_remove(struct i2c_client *client) > > +{ > > + struct da9150 *da9150 = i2c_get_clientdata(client); > > + > > + da9150_device_exit(da9150); > > + > > + return 0; > > +} > > + > > +static void da9150_shutdown(struct i2c_client *client) > > +{ > > + struct da9150 *da9150 = i2c_get_clientdata(client); > > + > > + da9150_device_shutdown(da9150); > > +} > > + > > +static const struct i2c_device_id da9150_i2c_id[] = { > > + { "da9150", 0 }, > > + { } > > +}; > > +MODULE_DEVICE_TABLE(i2c, da9150_i2c_id); > > + > > +static const struct of_device_id da9150_of_match[] = { > > + { .compatible = "dlg,da9150", }, > > + { } > > +}; > > + > > missed MODULE_DEVICE_TABLE(of, ...) ? Ok, will add that in. > > > +static struct i2c_driver da9150_driver = { > > + .driver = { > > + .name = "da9150", > > + .owner = THIS_MODULE, > > No need to update this field... Ok, will remove .owner setting. > > > + .of_match_table = of_match_ptr(da9150_of_match), > > + }, > > + .probe = da9150_probe, > > + .remove = da9150_remove, > > + .shutdown = da9150_shutdown, > > + .id_table = da9150_i2c_id, > > +}; > > + > > +module_i2c_driver(da9150_driver); > > + > > +MODULE_DESCRIPTION("I2C Driver for DA9150"); > > +MODULE_AUTHOR("Adam Thomson > <Adam.Thomson.Opensource@diasemi.com"); > > +MODULE_LICENSE("GPL"); > > > -- > Regards, > Varka Bhadram.
On August 28, 2014 17:36, Lee Jones wrote: Thanks for the feedback. As a general comment a couple of the items you've identified relate to future updates (additional functionality being added). I already have code in place for this but have stripped out a couple of the drivers just to reduce the churn and size of patch submission. These will be added once these patches have been accepted. Where this is the case, I have added notes in-line against the relevant comments you made. > On Thu, 28 Aug 2014, Adam Thomson wrote: > > > DA9150 is a combined Charger and Fuel-Gauge IC, with additional > > GPIO and GPADC functionality. > > > > Signed-off-by: Adam Thomson <Adam.Thomson.Opensource@diasemi.com> > > --- > > drivers/mfd/Kconfig | 12 + > > drivers/mfd/Makefile | 2 + > > drivers/mfd/da9150-core.c | 332 ++++++++++ > > drivers/mfd/da9150-i2c.c | 176 ++++++ > > Do you also have another, say SPI version? No, not yet, but this is something that we may add later as the device does support SPI. > > > include/linux/mfd/da9150/core.h | 80 +++ > > include/linux/mfd/da9150/pdata.h | 21 + > > include/linux/mfd/da9150/registers.h | 1153 > ++++++++++++++++++++++++++++++++++ > > 7 files changed, 1776 insertions(+) > > create mode 100644 drivers/mfd/da9150-core.c > > create mode 100644 drivers/mfd/da9150-i2c.c > > create mode 100644 include/linux/mfd/da9150/core.h > > create mode 100644 include/linux/mfd/da9150/pdata.h > > create mode 100644 include/linux/mfd/da9150/registers.h > > > > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig > > index de5abf2..76adb2c 100644 > > --- a/drivers/mfd/Kconfig > > +++ b/drivers/mfd/Kconfig > > @@ -183,6 +183,18 @@ config MFD_DA9063 > > Additional drivers must be enabled in order to use the functionality > > of the device. > > > > +config MFD_DA9150 > > + bool "Dialog Semiconductor DA9150 Charger Fuel-Gauge chip" > > Why can't this be built as a module? No reason. Will change it. > > > + depends on I2C=y > > + select MFD_CORE > > + select REGMAP_I2C > > + select REGMAP_IRQ > > + help > > + This adds support for the DA9150 integrated charger and fuel-gauge > > + chip. This driver provides common support for accessing the device. > > + Additional drivers must be enabled in order to use the specific > > + features of the device. > > + > > config MFD_MC13XXX > > tristate > > depends on (SPI_MASTER || I2C) > > diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile > > index f001487..098dfa1 100644 > > --- a/drivers/mfd/Makefile > > +++ b/drivers/mfd/Makefile > > @@ -114,6 +114,8 @@ obj-$(CONFIG_MFD_DA9055) += da9055.o > > da9063-objs := da9063-core.o da9063-irq.o da9063-i2c.o > > obj-$(CONFIG_MFD_DA9063) += da9063.o > > > > +obj-$(CONFIG_MFD_DA9150) += da9150-core.o da9150-i2c.o > > + > > Do the other drivers smell? Please butt up against them. > > I'm not entirely sure why there are so many '\n's in the Makefile! Okey dokey. Will change. > > > obj-$(CONFIG_MFD_MAX14577) += max14577.o > > obj-$(CONFIG_MFD_MAX77686) += max77686.o > > obj-$(CONFIG_MFD_MAX77693) += max77693.o > > diff --git a/drivers/mfd/da9150-core.c b/drivers/mfd/da9150-core.c > > new file mode 100644 > > index 0000000..029a30b > > --- /dev/null > > +++ b/drivers/mfd/da9150-core.c > > @@ -0,0 +1,332 @@ > > +/* > > + * DA9150 Core MFD Driver > > + * > > + * Copyright (c) 2014 Dialog Semiconductor > > + * > > + * Author: Adam Thomson <Adam.Thomson.Opensource@diasemi.com> > > + * > > + * This program is free software; you can redistribute it and/or modify it > > + * under the terms of the GNU General Public License as published by the > > + * Free Software Foundation; either version 2 of the License, or (at your > > + * option) any later version. > > + */ > > + > > +#include <linux/kernel.h> > > +#include <linux/module.h> > > +#include <linux/platform_device.h> > > +#include <linux/slab.h> > > +#include <linux/irq.h> > > +#include <linux/interrupt.h> > > +#include <linux/mfd/core.h> > > + > > No real need for this '\n'. I can change this, but my reason was to separate common kernel includes from device specific ones, for readability. > > > +#include <linux/mfd/da9150/core.h> > > +#include <linux/mfd/da9150/registers.h> > > +#include <linux/mfd/da9150/pdata.h> > > + > > +u8 da9150_reg_read(struct da9150 *da9150, u16 reg) > > +{ > > + int val, ret; > > + > > + ret = regmap_read(da9150->regmap, reg, &val); > > + if (ret < 0) > > What if ret > 0? Is that a good thing? :) > > Just 'if (ret)'. Fine, will change. > > > + dev_err(da9150->dev, "Failed to read from reg 0x%x: %d\n", > > + reg, ret); > > + > > + return (u8) val; > > +} > > +EXPORT_SYMBOL_GPL(da9150_reg_read); > > Not sure I like this abstraction stuff. I could understand if there > were locking involved, but there isn't. You don't appear to check for > errors in the subordinate drivers either, rather you just plough on > ahead. Not sure that's a good idea either. > > Anyone have a second opinion? The reason for these is because future patches to add additional functionality will introduce I2C access functions which do not use regmap and access the device via a separate I2C address for this purpose. I will need to provide access functions for that, and so having a common style of I2C access makes sense for this driver. Means any access just needs to provide the MFD private data, and the relevant functions take care of the rest. I think this is cleaner in this instance. With regards to errors, if we're seeing problems on I2C I don't believe dealing with the errors elsewhere is going to help much. I wanted to provide logging though in case this is seen for some reason. However I could just make these functions void return as you're right in your comments that no checking is done elsewhere. > > > +int da9150_reg_write(struct da9150 *da9150, u16 reg, u8 val) > > +{ > > + int ret; > > + > > + ret = regmap_write(da9150->regmap, reg, val); > > + if (ret < 0) > > + dev_err(da9150->dev, "Failed to write to reg 0x%x: %d\n", > > + reg, ret); > > + > > + return ret; > > +} > > +EXPORT_SYMBOL_GPL(da9150_reg_write); > > Blah. > > > +int da9150_set_bits(struct da9150 *da9150, u16 reg, u8 mask, u8 val) > > +{ > > + int ret; > > + > > + ret = regmap_update_bits(da9150->regmap, reg, mask, val); > > + if (ret < 0) > > + dev_err(da9150->dev, "Failed to set bits in reg 0x%x: %d\n", > > + reg, ret); > > + > > + return ret; > > +} > > +EXPORT_SYMBOL_GPL(da9150_set_bits); > > Blah. > > > +int da9150_bulk_read(struct da9150 *da9150, u16 reg, int count, u8 *buf) > > +{ > > + int ret; > > + > > + ret = regmap_bulk_read(da9150->regmap, reg, buf, count); > > + if (ret < 0) > > + dev_err(da9150->dev, "Failed to bulk read from reg 0x%x: %d\n", > > + reg, ret); > > + > > + return ret; > > +} > > +EXPORT_SYMBOL_GPL(da9150_bulk_read); > > Blah. > > > +int da9150_bulk_write(struct da9150 *da9150, u16 reg, int count, const u8 *buf) > > +{ > > + int ret; > > + > > + ret = regmap_raw_write(da9150->regmap, reg, buf, count); > > + if (ret < 0) > > + dev_err(da9150->dev, "Failed to bulk write to reg 0x%x %d\n", > > + reg, ret); > > + > > + return ret; > > +} > > +EXPORT_SYMBOL_GPL(da9150_bulk_write); > > Blah. > > > +static struct regmap_irq da9150_irqs[] = { > > + [DA9150_IRQ_VBUS] = { > > + .reg_offset = 0, > > + .mask = DA9150_E_VBUS_MASK, > > + }, > > + [DA9150_IRQ_CHG] = { > > + .reg_offset = 0, > > + .mask = DA9150_E_CHG_MASK, > > + }, > > + [DA9150_IRQ_TCLASS] = { > > + .reg_offset = 0, > > + .mask = DA9150_E_TCLASS_MASK, > > + }, > > + [DA9150_IRQ_TJUNC] = { > > + .reg_offset = 0, > > + .mask = DA9150_E_TJUNC_MASK, > > + }, > > + [DA9150_IRQ_VFAULT] = { > > + .reg_offset = 0, > > + .mask = DA9150_E_VFAULT_MASK, > > + }, > > + [DA9150_IRQ_CONF] = { > > + .reg_offset = 1, > > + .mask = DA9150_E_CONF_MASK, > > + }, > > + [DA9150_IRQ_DAT] = { > > + .reg_offset = 1, > > + .mask = DA9150_E_DAT_MASK, > > + }, > > + [DA9150_IRQ_DTYPE] = { > > + .reg_offset = 1, > > + .mask = DA9150_E_DTYPE_MASK, > > + }, > > + [DA9150_IRQ_ID] = { > > + .reg_offset = 1, > > + .mask = DA9150_E_ID_MASK, > > + }, > > + [DA9150_IRQ_ADP] = { > > + .reg_offset = 1, > > + .mask = DA9150_E_ADP_MASK, > > + }, > > + [DA9150_IRQ_SESS_END] = { > > + .reg_offset = 1, > > + .mask = DA9150_E_SESS_END_MASK, > > + }, > > + [DA9150_IRQ_SESS_VLD] = { > > + .reg_offset = 1, > > + .mask = DA9150_E_SESS_VLD_MASK, > > + }, > > + [DA9150_IRQ_FG] = { > > + .reg_offset = 2, > > + .mask = DA9150_E_FG_MASK, > > + }, > > + [DA9150_IRQ_GP] = { > > + .reg_offset = 2, > > + .mask = DA9150_E_GP_MASK, > > + }, > > + [DA9150_IRQ_TBAT] = { > > + .reg_offset = 2, > > + .mask = DA9150_E_TBAT_MASK, > > + }, > > + [DA9150_IRQ_GPIOA] = { > > + .reg_offset = 2, > > + .mask = DA9150_E_GPIOA_MASK, > > + }, > > + [DA9150_IRQ_GPIOB] = { > > + .reg_offset = 2, > > + .mask = DA9150_E_GPIOB_MASK, > > + }, > > + [DA9150_IRQ_GPIOC] = { > > + .reg_offset = 2, > > + .mask = DA9150_E_GPIOC_MASK, > > + }, > > + [DA9150_IRQ_GPIOD] = { > > + .reg_offset = 2, > > + .mask = DA9150_E_GPIOD_MASK, > > + }, > > + [DA9150_IRQ_GPADC] = { > > + .reg_offset = 2, > > + .mask = DA9150_E_GPADC_MASK, > > + }, > > + [DA9150_IRQ_WKUP] = { > > + .reg_offset = 3, > > + .mask = DA9150_E_WKUP_MASK, > > + }, > > +}; > > + > > +static struct regmap_irq_chip da9150_regmap_irq_chip = { > > + .name = "da9150_irq", > > + .status_base = DA9150_EVENT_E, > > + .mask_base = DA9150_IRQ_MASK_E, > > + .ack_base = DA9150_EVENT_E, > > + .num_regs = DA9150_NUM_IRQ_REGS, > > + .irqs = da9150_irqs, > > + .num_irqs = ARRAY_SIZE(da9150_irqs), > > +}; > > + > > +/* Helper functions for sub-devices to request/free IRQs */ > > +int da9150_register_irq(struct platform_device *pdev, void *dev_id, > > + irq_handler_t handler, const char *name) > > +{ > > + int irq, ret; > > + > > + irq = platform_get_irq_byname(pdev, name); > > + if (irq < 0) > > + return irq; > > + > > + ret = devm_request_threaded_irq(&pdev->dev, irq, NULL, handler, > > + IRQF_ONESHOT, name, dev_id); > > + if (ret) > > + dev_err(&pdev->dev, "Failed to request IRQ %d: %d\n", irq, ret); > > + > > + return ret; > > +} > > +EXPORT_SYMBOL_GPL(da9150_register_irq); > > Why do they need help? What problem does adding these layers solve? Means I don't have to keep adding print error lines everywhere else if this function takes care of it. Thought that would be cleaner. > > > +void da9150_release_irq(struct platform_device *pdev, void *dev_id, > > + const char *name) > > +{ > > + int irq; > > + > > + irq = platform_get_irq_byname(pdev, name); > > + if (irq < 0) > > + return; > > + > > + devm_free_irq(&pdev->dev, irq, dev_id); > > +} > > +EXPORT_SYMBOL_GPL(da9150_release_irq); > > Do you ever release the IRQ and not unbind the driver? > > Are there ordering issues at play here? > > If not, there's no need to conduct a manual free. In the charger driver, in the remove function there is a need I believe to free the IRQs before other items are cleared up (e.g. power_supply classes), so this is why I have added this in here. > > > +static struct resource da9150_gpadc_resources[] = { > > + { > > + .name = "GPADC", > > + .start = DA9150_IRQ_GPADC, > > + .end = DA9150_IRQ_GPADC, > > + .flags = IORESOURCE_IRQ, > > + }, > > +}; > > + > > +static struct resource da9150_charger_resources[] = { > > + { > > + .name = "CHG_STATUS", > > + .start = DA9150_IRQ_CHG, > > + .end = DA9150_IRQ_CHG, > > + .flags = IORESOURCE_IRQ, > > + }, > > + { > > + .name = "CHG_TJUNC", > > + .start = DA9150_IRQ_TJUNC, > > + .end = DA9150_IRQ_TJUNC, > > + .flags = IORESOURCE_IRQ, > > + }, > > + { > > + .name = "CHG_VFAULT", > > + .start = DA9150_IRQ_VFAULT, > > + .end = DA9150_IRQ_VFAULT, > > + .flags = IORESOURCE_IRQ, > > + }, > > + { > > + .name = "CHG_VBUS", > > + .start = DA9150_IRQ_VBUS, > > + .end = DA9150_IRQ_VBUS, > > + .flags = IORESOURCE_IRQ, > > + }, > > +}; > > + > > +static struct mfd_cell da9150_devs[] = { > > + { > > + .name = "da9150-gpadc", > > + .of_compatible = "dlg,da9150-gpadc", > > + .resources = da9150_gpadc_resources, > > + .num_resources = ARRAY_SIZE(da9150_gpadc_resources), > > + }, > > + { > > + .name = "da9150-charger", > > + .of_compatible = "dlg,da9150-charger", > > + .resources = da9150_charger_resources, > > + .num_resources = ARRAY_SIZE(da9150_charger_resources), > > + }, > > +}; > > + > > +int da9150_device_init(struct da9150 *da9150) > > +{ > > + struct da9150_pdata *pdata = da9150->dev->platform_data; > > dev_get_platdata() Right ho. Will update. > > > + int ret; > > + > > + /* Handle platform data */ > > This comment doesn't add anything - the code is clear enough. Ok will scrap that. > > > + if (pdata) > > + da9150->irq_base = pdata->irq_base; > > + else > > + da9150->irq_base = -1; > > pdata ? pdata->irq_base : -1; This is left this way as later updates to add additional functionality will require addtional work to be done with the platform data. Seemed pointless changing it here just to change it back later. > > > + /* Init IRQs */ > > No need for these, please only add comments where the code is complex > or convoluted. Ok, will remove. > > > + ret = regmap_add_irq_chip(da9150->regmap, da9150->irq, > > + IRQF_TRIGGER_LOW | IRQF_ONESHOT, > > + da9150->irq_base, &da9150_regmap_irq_chip, > > + &da9150->regmap_irq_data); > > + if (ret < 0) > > 'if (ret)' where positive replies aren't possible or are errors. Ok, fine. > > > + goto err_irq; > > Just return here and remove 'err_irq' label. Yep, will tidy up. > > > + da9150->irq_base = regmap_irq_chip_get_base(da9150->regmap_irq_data); > > + > > + /* Make the IRQ line a wake source */ > > + enable_irq_wake(da9150->irq); > > + > > + /* Add MFD Devices */ > > + ret = mfd_add_devices(da9150->dev, -1, da9150_devs, > > + ARRAY_SIZE(da9150_devs), NULL, > > + da9150->irq_base, NULL); > > + if (ret < 0) { > > + dev_err(da9150->dev, "Failed to add child devices: %d\n", ret); > > + goto err_mfd; > > + } > > + > > + return 0; > > + > > +err_mfd: > > + regmap_del_irq_chip(da9150->irq, da9150->regmap_irq_data); > > +err_irq: > > + return ret; > > +} > > + > > +void da9150_device_exit(struct da9150 *da9150) > > +{ > > + regmap_del_irq_chip(da9150->irq, da9150->regmap_irq_data); > > + mfd_remove_devices(da9150->dev); > > +} > > + > > +void da9150_device_shutdown(struct da9150 *da9150) > > +{ > > + /* Make sure we have a wakup source for the device */ > > + da9150_set_bits(da9150, DA9150_CONFIG_D, > > + DA9150_WKUP_PM_EN_MASK, > > + DA9150_WKUP_PM_EN_MASK); > > + > > + /* Set device to DISABLED mode */ > > + da9150_set_bits(da9150, DA9150_CONTROL_C, > > + DA9150_DISABLE_MASK, DA9150_DISABLE_MASK); > > +} > > + > > +MODULE_DESCRIPTION("MFD Core Driver for DA9150"); > > +MODULE_AUTHOR("Adam Thomson > <Adam.Thomson.Opensource@diasemi.com"); > > +MODULE_LICENSE("GPL"); > > diff --git a/drivers/mfd/da9150-i2c.c b/drivers/mfd/da9150-i2c.c > > new file mode 100644 > > index 0000000..a02525c > > --- /dev/null > > +++ b/drivers/mfd/da9150-i2c.c > > @@ -0,0 +1,176 @@ > > +/* > > + * DA9150 I2C Driver > > + * > > + * Copyright (c) 2014 Dialog Semiconductor > > + * > > + * Author: Adam Thomson <Adam.Thomson.Opensource@diasemi.com> > > + * > > + * This program is free software; you can redistribute it and/or modify it > > + * under the terms of the GNU General Public License as published by the > > + * Free Software Foundation; either version 2 of the License, or (at your > > + * option) any later version. > > + */ > > + > > +#include <linux/kernel.h> > > +#include <linux/module.h> > > +#include <linux/platform_device.h> > > +#include <linux/i2c.h> > > +#include <linux/regmap.h> > > +#include <linux/slab.h> > > + > > Remove this line. Ok. > > > +#include <linux/mfd/da9150/core.h> > > +#include <linux/mfd/da9150/registers.h> > > + > > +static bool da9150_volatile_reg(struct device *dev, unsigned int reg) > > +{ > > + switch (reg) { > > + case DA9150_PAGE_CON: > > + case DA9150_STATUS_A: > > + case DA9150_STATUS_B: > > + case DA9150_STATUS_C: > > + case DA9150_STATUS_D: > > + case DA9150_STATUS_E: > > + case DA9150_STATUS_F: > > + case DA9150_STATUS_G: > > + case DA9150_STATUS_H: > > + case DA9150_STATUS_I: > > + case DA9150_STATUS_J: > > + case DA9150_STATUS_K: > > + case DA9150_STATUS_L: > > + case DA9150_STATUS_N: > > + case DA9150_FAULT_LOG_A: > > + case DA9150_FAULT_LOG_B: > > + case DA9150_EVENT_E: > > + case DA9150_EVENT_F: > > + case DA9150_EVENT_G: > > + case DA9150_EVENT_H: > > + case DA9150_CONTROL_B: > > + case DA9150_CONTROL_C: > > + case DA9150_GPADC_MAN: > > + case DA9150_GPADC_RES_A: > > + case DA9150_GPADC_RES_B: > > + case DA9150_ADETVB_CFG_C: > > + case DA9150_ADETD_STAT: > > + case DA9150_ADET_CMPSTAT: > > + case DA9150_ADET_CTRL_A: > > + case DA9150_PPR_TCTR_B: > > + case DA9150_COREBTLD_STAT_A: > > + case DA9150_CORE_DATA_A: > > + case DA9150_CORE_DATA_B: > > + case DA9150_CORE_DATA_C: > > + case DA9150_CORE_DATA_D: > > + case DA9150_CORE2WIRE_STAT_A: > > + case DA9150_FW_CTRL_C: > > + case DA9150_FG_CTRL_B: > > + case DA9150_FW_CTRL_B: > > + case DA9150_GPADC_CMAN: > > + case DA9150_GPADC_CRES_A: > > + case DA9150_GPADC_CRES_B: > > + case DA9150_CC_ICHG_RES_A: > > + case DA9150_CC_ICHG_RES_B: > > + case DA9150_CC_IAVG_RES_A: > > + case DA9150_CC_IAVG_RES_B: > > + case DA9150_TAUX_CTRL_A: > > + case DA9150_TAUX_VALUE_H: > > + case DA9150_TAUX_VALUE_L: > > + case DA9150_TBAT_RES_A: > > + case DA9150_TBAT_RES_B: > > + return true; > > + default: > > + return false; > > + } > > +} > > + > > +static const struct regmap_range_cfg da9150_range_cfg[] = { > > + { > > + .range_min = DA9150_PAGE_CON, > > + .range_max = DA9150_TBAT_RES_B, > > + .selector_reg = DA9150_PAGE_CON, > > + .selector_mask = DA9150_I2C_PAGE_MASK, > > + .selector_shift = DA9150_I2C_PAGE_SHIFT, > > + .window_start = 0, > > + .window_len = 256, > > + }, > > +}; > > + > > +static struct regmap_config da9150_regmap_config = { > > + .reg_bits = 8, > > + .val_bits = 8, > > + .ranges = da9150_range_cfg, > > + .num_ranges = ARRAY_SIZE(da9150_range_cfg), > > + .max_register = DA9150_TBAT_RES_B, > > + > > + .cache_type = REGCACHE_RBTREE, > > + > > + .volatile_reg = da9150_volatile_reg, > > +}; > > + > > +static int da9150_probe(struct i2c_client *client, > > + const struct i2c_device_id *id) > > +{ > > + struct da9150 *da9150; > > + int ret; > > + > > + da9150 = devm_kzalloc(&client->dev, sizeof(struct da9150), GFP_KERNEL); > > sizeof(*da9150) Same difference, but ok. > > > + if (da9150 == NULL) > > if (!da9150) Right, fine. > > > + return -ENOMEM; > > '\n' Right, fine. > > > + da9150->dev = &client->dev; > > + da9150->irq = client->irq; > > + i2c_set_clientdata(client, da9150); > > + dev_set_drvdata(da9150->dev, da9150); > > Why do you need this in both locations? > Don't. Will remove accordingly. > > + da9150->regmap = devm_regmap_init_i2c(client, &da9150_regmap_config); > > + if (IS_ERR(da9150->regmap)) { > > + ret = PTR_ERR(da9150->regmap); > > + dev_err(da9150->dev, "Failed to allocate register map: %d\n", > > + ret); > > + return ret; > > + } > > + > > + return da9150_device_init(da9150); > > +} > > + > > +static int da9150_remove(struct i2c_client *client) > > +{ > > + struct da9150 *da9150 = i2c_get_clientdata(client); > > + > > + da9150_device_exit(da9150); > > + > > + return 0; > > +} > > + > > +static void da9150_shutdown(struct i2c_client *client) > > +{ > > + struct da9150 *da9150 = i2c_get_clientdata(client); > > + > > + da9150_device_shutdown(da9150); > > +} > > + > > +static const struct i2c_device_id da9150_i2c_id[] = { > > + { "da9150", 0 }, > > I don't see the .id parameter being used, just leave it blank. Ok, no problem. > > > + { } > > +}; > > +MODULE_DEVICE_TABLE(i2c, da9150_i2c_id); > > + > > +static const struct of_device_id da9150_of_match[] = { > > + { .compatible = "dlg,da9150", }, > > + { } > > +}; > > MODULE_DEVICE_TABLE(of, ...) > > > +static struct i2c_driver da9150_driver = { > > + .driver = { > > + .name = "da9150", > > + .owner = THIS_MODULE, > > You can remove this line, it's taken care of for you elsewhere. Yes, will remove. > > > + .of_match_table = of_match_ptr(da9150_of_match), > > + }, > > + .probe = da9150_probe, > > + .remove = da9150_remove, > > + .shutdown = da9150_shutdown, > > + .id_table = da9150_i2c_id, > > +}; > > + > > +module_i2c_driver(da9150_driver); > > + > > +MODULE_DESCRIPTION("I2C Driver for DA9150"); > > +MODULE_AUTHOR("Adam Thomson > <Adam.Thomson.Opensource@diasemi.com"); > > +MODULE_LICENSE("GPL"); > > diff --git a/include/linux/mfd/da9150/core.h b/include/linux/mfd/da9150/core.h > > new file mode 100644 > > index 0000000..d23c500 > > --- /dev/null > > +++ b/include/linux/mfd/da9150/core.h > > @@ -0,0 +1,80 @@ > > +/* > > + * DA9150 MFD Driver - Core Data > > + * > > + * Copyright (c) 2014 Dialog Semiconductor > > + * > > + * Author: Adam Thomson <Adam.Thomson.Opensource@diasemi.com> > > + * > > + * This program is free software; you can redistribute it and/or modify it > > + * under the terms of the GNU General Public License as published by the > > + * Free Software Foundation; either version 2 of the License, or (at your > > + * option) any later version. > > + */ > > + > > +#ifndef __DA9150_CORE_H > > +#define __DA9150_CORE_H > > + > > +#include <linux/device.h> > > +#include <linux/i2c.h> > > What's this used for? > > > +#include <linux/interrupt.h> > > +#include <linux/regmap.h> > > +#include <linux/mutex.h> > > + > > +/* I2C address paging */ > > +#define DA9150_REG_PAGE_SHIFT 8 > > +#define DA9150_REG_PAGE_MASK 0xFF > > + > > +/* IRQs */ > > +#define DA9150_NUM_IRQ_REGS 4 > > +#define DA9150_IRQ_VBUS 0 > > +#define DA9150_IRQ_CHG 1 > > +#define DA9150_IRQ_TCLASS 2 > > +#define DA9150_IRQ_TJUNC 3 > > +#define DA9150_IRQ_VFAULT 4 > > +#define DA9150_IRQ_CONF 5 > > +#define DA9150_IRQ_DAT 6 > > +#define DA9150_IRQ_DTYPE 7 > > +#define DA9150_IRQ_ID 8 > > +#define DA9150_IRQ_ADP 9 > > +#define DA9150_IRQ_SESS_END 10 > > +#define DA9150_IRQ_SESS_VLD 11 > > +#define DA9150_IRQ_FG 12 > > +#define DA9150_IRQ_GP 13 > > +#define DA9150_IRQ_TBAT 14 > > +#define DA9150_IRQ_GPIOA 15 > > +#define DA9150_IRQ_GPIOB 16 > > +#define DA9150_IRQ_GPIOC 17 > > +#define DA9150_IRQ_GPIOD 18 > > +#define DA9150_IRQ_GPADC 19 > > +#define DA9150_IRQ_WKUP 20 > > + > > +struct da9150 { > > + struct device *dev; > > + > > + struct regmap *regmap; > > + > > Why the '\n's? Personal preference, but will remove. > > > + struct regmap_irq_chip_data *regmap_irq_data; > > + int irq; > > + int irq_base; > > +}; > > + > > +/* Device I/O */ > > +u8 da9150_reg_read(struct da9150 *da9150, u16 reg); > > +int da9150_reg_write(struct da9150 *da9150, u16 reg, u8 val); > > +int da9150_set_bits(struct da9150 *da9150, u16 reg, u8 mask, u8 val); > > + > > +int da9150_bulk_read(struct da9150 *da9150, u16 reg, int count, u8 *buf); > > +int da9150_bulk_write(struct da9150 *da9150, u16 reg, int count, const u8 *buf); > > + > > +/* IRQ helper functions */ > > +int da9150_register_irq(struct platform_device *pdev, void *dev_id, > > + irq_handler_t handler, const char *name); > > +void da9150_release_irq(struct platform_device *pdev, void *dev_id, > > + const char *name); > > I'm not sure why any of these 7 functions are required. Clarified my intentions in previous comments. > > > +/* Init/Exit */ > > +int da9150_device_init(struct da9150 *da9150); > > +void da9150_device_exit(struct da9150 *da9150); > > +void da9150_device_shutdown(struct da9150 *da9150); > > + > > +#endif /* __DA9150_CORE_H */ > > diff --git a/include/linux/mfd/da9150/pdata.h b/include/linux/mfd/da9150/pdata.h > > new file mode 100644 > > index 0000000..e2b37f1 > > --- /dev/null > > +++ b/include/linux/mfd/da9150/pdata.h > > @@ -0,0 +1,21 @@ > > +/* > > + * DA9150 MFD Driver - Platform Data > > + * > > + * Copyright (c) 2014 Dialog Semiconductor > > + * > > + * Author: Adam Thomson <Adam.Thomson.Opensource@diasemi.com> > > + * > > + * This program is free software; you can redistribute it and/or modify it > > + * under the terms of the GNU General Public License as published by the > > + * Free Software Foundation; either version 2 of the License, or (at your > > + * option) any later version. > > + */ > > + > > +#ifndef __DA9150_PDATA_H > > +#define __DA9150_PDATA_H > > + > > +struct da9150_pdata { > > + int irq_base; > > +}; > > Just put this in core.h and do away witht this header file. The reason for this is that I will add more platform data items later with subsequent submissions for additional features. It felt cleaner to separate out these structures than throw it in the core.h header. However if it's going to be a problem I'll fold this into core.h > > > +#endif /* __DA9150_PDATA_H */ > > diff --git a/include/linux/mfd/da9150/registers.h > b/include/linux/mfd/da9150/registers.h > > new file mode 100644 > > index 0000000..ef4826d > > --- /dev/null > > +++ b/include/linux/mfd/da9150/registers.h > > @@ -0,0 +1,1153 @@ > > +/* > > + * DA9150 MFD Driver - Registers > > + * > > + * Copyright (c) 2014 Dialog Semiconductor > > + * > > + * Author: Adam Thomson <Adam.Thomson.Opensource@diasemi.com> > > + * > > + * This program is free software; you can redistribute it and/or modify it > > + * under the terms of the GNU General Public License as published by the > > + * Free Software Foundation; either version 2 of the License, or (at your > > + * option) any later version. > > + */ > > + > > +#ifndef __DA9150_REGISTERS_H > > +#define __DA9150_REGISTERS_H > > + > > +/* Registers */ > > [...] > > > +/* DA9150_CONTROL_A = 0x0E5 */ > > +#define DA9150_VDD33_SL_SHIFT 0 > > +#define DA9150_VDD33_SL_MASK (0x01 << 0) > > +#define DA9150_VDD33_LPM_SHIFT 1 > > +#define DA9150_VDD33_LPM_MASK (0x03 << 1) > > +#define DA9150_VDD33_EN_SHIFT 3 > > +#define DA9150_VDD33_EN_MASK (0x01 << 3) > > +#define DA9150_GPI_LPM_SHIFT 6 > > +#define DA9150_GPI_LPM_MASK (0x01 << 6) > > +#define DA9150_PM_IF_LPM_SHIFT 7 > > +#define DA9150_PM_IF_LPM_MASK (0x01 << 7) > > + > > +/* DA9150_CONTROL_B = 0x0E6 */ > > +#define DA9150_LPM_SHIFT 0 > > +#define DA9150_LPM_MASK (0x01 << 0) > > +#define DA9150_RESET_SHIFT 1 > > +#define DA9150_RESET_MASK (0x01 << 1) > > +#define DA9150_RESET_USRCONF_EN_SHIFT 2 > > +#define DA9150_RESET_USRCONF_EN_MASK (0x01 << 2) > > + > > +/* DA9150_CONTROL_C = 0x0E7 */ > > +#define DA9150_DISABLE_SHIFT 0 > > +#define DA9150_DISABLE_MASK (0x01 << 0) > > Use BIT() for all of these (1 << X) macros. OK, will do. > > [...] > > -- > Lee Jones > Linaro STMicroelectronics Landing Team Lead > Linaro.org ? Open source software for ARM SoCs > Follow Linaro: Facebook | Twitter | Blog
On Tue, 09 Sep 2014, Opensource [Adam Thomson] wrote: > On August 28, 2014 17:36, Lee Jones wrote: > > Thanks for the feedback. As a general comment a couple of the items you've > identified relate to future updates (additional functionality being added). > I already have code in place for this but have stripped out a couple of the > drivers just to reduce the churn and size of patch submission. These will be > added once these patches have been accepted. > > Where this is the case, I have added notes in-line against the relevant > comments you made. > > > On Thu, 28 Aug 2014, Adam Thomson wrote: > > > > > DA9150 is a combined Charger and Fuel-Gauge IC, with additional > > > GPIO and GPADC functionality. > > > > > > Signed-off-by: Adam Thomson <Adam.Thomson.Opensource@diasemi.com> > > > --- > > > drivers/mfd/Kconfig | 12 + > > > drivers/mfd/Makefile | 2 + > > > drivers/mfd/da9150-core.c | 332 ++++++++++ > > > drivers/mfd/da9150-i2c.c | 176 ++++++ > > > > Do you also have another, say SPI version? > > No, not yet, but this is something that we may add later as the device does > support SPI. I'm not sure we want to split up the files like this for an 'if we decide to produce an SPI variant in the future'. If you do, then you can split the files up. Until then, stick everything in -core. > > > include/linux/mfd/da9150/core.h | 80 +++ > > > include/linux/mfd/da9150/pdata.h | 21 + > > > include/linux/mfd/da9150/registers.h | 1153 > > ++++++++++++++++++++++++++++++++++ > > > 7 files changed, 1776 insertions(+) > > > create mode 100644 drivers/mfd/da9150-core.c > > > create mode 100644 drivers/mfd/da9150-i2c.c > > > create mode 100644 include/linux/mfd/da9150/core.h > > > create mode 100644 include/linux/mfd/da9150/pdata.h > > > create mode 100644 include/linux/mfd/da9150/registers.h > > > > > > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig > > > index de5abf2..76adb2c 100644 > > > --- a/drivers/mfd/Kconfig > > > +++ b/drivers/mfd/Kconfig > > > @@ -183,6 +183,18 @@ config MFD_DA9063 > > > Additional drivers must be enabled in order to use the functionality > > > of the device. > > > > > > +config MFD_DA9150 > > > + bool "Dialog Semiconductor DA9150 Charger Fuel-Gauge chip" > > > > Why can't this be built as a module? > > No reason. Will change it. > > > > > > + depends on I2C=y > > > + select MFD_CORE > > > + select REGMAP_I2C > > > + select REGMAP_IRQ > > > + help > > > + This adds support for the DA9150 integrated charger and fuel-gauge > > > + chip. This driver provides common support for accessing the device. > > > + Additional drivers must be enabled in order to use the specific > > > + features of the device. > > > + > > > config MFD_MC13XXX > > > tristate > > > depends on (SPI_MASTER || I2C) > > > diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile > > > index f001487..098dfa1 100644 > > > --- a/drivers/mfd/Makefile > > > +++ b/drivers/mfd/Makefile > > > @@ -114,6 +114,8 @@ obj-$(CONFIG_MFD_DA9055) += da9055.o > > > da9063-objs := da9063-core.o da9063-irq.o da9063-i2c.o > > > obj-$(CONFIG_MFD_DA9063) += da9063.o > > > > > > +obj-$(CONFIG_MFD_DA9150) += da9150-core.o da9150-i2c.o > > > + > > > > Do the other drivers smell? Please butt up against them. > > > > I'm not entirely sure why there are so many '\n's in the Makefile! > > Okey dokey. Will change. > > > > > > obj-$(CONFIG_MFD_MAX14577) += max14577.o > > > obj-$(CONFIG_MFD_MAX77686) += max77686.o > > > obj-$(CONFIG_MFD_MAX77693) += max77693.o > > > diff --git a/drivers/mfd/da9150-core.c b/drivers/mfd/da9150-core.c > > > new file mode 100644 > > > index 0000000..029a30b > > > --- /dev/null > > > +++ b/drivers/mfd/da9150-core.c > > > @@ -0,0 +1,332 @@ > > > +/* > > > + * DA9150 Core MFD Driver > > > + * > > > + * Copyright (c) 2014 Dialog Semiconductor > > > + * > > > + * Author: Adam Thomson <Adam.Thomson.Opensource@diasemi.com> > > > + * > > > + * This program is free software; you can redistribute it and/or modify it > > > + * under the terms of the GNU General Public License as published by the > > > + * Free Software Foundation; either version 2 of the License, or (at your > > > + * option) any later version. > > > + */ > > > + > > > +#include <linux/kernel.h> > > > +#include <linux/module.h> > > > +#include <linux/platform_device.h> > > > +#include <linux/slab.h> > > > +#include <linux/irq.h> > > > +#include <linux/interrupt.h> > > > +#include <linux/mfd/core.h> > > > + > > > > No real need for this '\n'. > > I can change this, but my reason was to separate common kernel includes from > device specific ones, for readability. It isn't any less readable with the '\n' removed. > > > +#include <linux/mfd/da9150/core.h> > > > +#include <linux/mfd/da9150/registers.h> > > > +#include <linux/mfd/da9150/pdata.h> > > > + > > > +u8 da9150_reg_read(struct da9150 *da9150, u16 reg) > > > +{ > > > + int val, ret; > > > + > > > + ret = regmap_read(da9150->regmap, reg, &val); > > > + if (ret < 0) > > > > What if ret > 0? Is that a good thing? :) > > > > Just 'if (ret)'. > > Fine, will change. > > > > > > + dev_err(da9150->dev, "Failed to read from reg 0x%x: %d\n", > > > + reg, ret); > > > + > > > + return (u8) val; > > > +} > > > +EXPORT_SYMBOL_GPL(da9150_reg_read); > > > > Not sure I like this abstraction stuff. I could understand if there > > were locking involved, but there isn't. You don't appear to check for > > errors in the subordinate drivers either, rather you just plough on > > ahead. Not sure that's a good idea either. > > > > Anyone have a second opinion? > > The reason for these is because future patches to add additional functionality > will introduce I2C access functions which do not use regmap and access the > device via a separate I2C address for this purpose. I will need to provide > access functions for that, and so having a common style of I2C access makes > sense for this driver. Means any access just needs to provide the MFD private > data, and the relevant functions take care of the rest. I think this is cleaner > in this instance. So long as these appear soon. Otherwise it's just cruft. [...] > > > +/* Helper functions for sub-devices to request/free IRQs */ > > > +int da9150_register_irq(struct platform_device *pdev, void *dev_id, > > > + irq_handler_t handler, const char *name) > > > +{ > > > + int irq, ret; > > > + > > > + irq = platform_get_irq_byname(pdev, name); > > > + if (irq < 0) > > > + return irq; > > > + > > > + ret = devm_request_threaded_irq(&pdev->dev, irq, NULL, handler, > > > + IRQF_ONESHOT, name, dev_id); > > > + if (ret) > > > + dev_err(&pdev->dev, "Failed to request IRQ %d: %d\n", irq, ret); > > > + > > > + return ret; > > > +} > > > +EXPORT_SYMBOL_GPL(da9150_register_irq); > > > > Why do they need help? What problem does adding these layers solve? > > Means I don't have to keep adding print error lines everywhere else if this > function takes care of it. Thought that would be cleaner. You only need to request each IRQ once. It's just more abstraction for the sake of it. I would prefer if you removed them. > > > +void da9150_release_irq(struct platform_device *pdev, void *dev_id, > > > + const char *name) > > > +{ > > > + int irq; > > > + > > > + irq = platform_get_irq_byname(pdev, name); > > > + if (irq < 0) > > > + return; > > > + > > > + devm_free_irq(&pdev->dev, irq, dev_id); > > > +} > > > +EXPORT_SYMBOL_GPL(da9150_release_irq); > > > > Do you ever release the IRQ and not unbind the driver? > > > > Are there ordering issues at play here? > > > > If not, there's no need to conduct a manual free. > > In the charger driver, in the remove function there is a need I believe to > free the IRQs before other items are cleared up (e.g. power_supply classes), > so this is why I have added this in here. Can you handle this separately in the Charger driver then please? [...] > > > + if (pdata) > > > + da9150->irq_base = pdata->irq_base; > > > + else > > > + da9150->irq_base = -1; > > > > pdata ? pdata->irq_base : -1; > > This is left this way as later updates to add additional functionality will > require addtional work to be done with the platform data. Seemed pointless > changing it here just to change it back later. You're not changing anything, as this is the introduction of the code. I can't tell you how many times I've heard "I will change it later", or "doing it this way will support subsequent submissions", then received no more patches. It's okay to do it nicely now and expand it back out in the new patches. [...] > > > + da9150 = devm_kzalloc(&client->dev, sizeof(struct da9150), GFP_KERNEL); > > > > sizeof(*da9150) > > Same difference, but ok. It's more succinct and almost always done this way because of it. [...] > > > +struct da9150_pdata { > > > + int irq_base; > > > +}; > > > > Just put this in core.h and do away witht this header file. > > The reason for this is that I will add more platform data items later with > subsequent submissions for additional features. It felt cleaner to separate out > these structures than throw it in the core.h header. However if it's going to > be a problem I'll fold this into core.h More "I will"s. :) Please do what's right for 'this submission'. If things change later you can act accordingly.
On Tue, 09 Sep 2014, Opensource [Adam Thomson] wrote: > On August 28, 2014 12:47, Varka Bhadram wrote: > > > On 08/28/2014 04:18 PM, Adam Thomson wrote: > > > > (...) > > > > > +static int da9150_probe(struct i2c_client *client, > > > + const struct i2c_device_id *id) > > > +{ > > > + struct da9150 *da9150; > > > + int ret; > > > + > > > + da9150 = devm_kzalloc(&client->dev, sizeof(struct da9150), GFP_KERNEL); > > > + if (da9150 == NULL) > > > + return -ENOMEM; > > > > da9150 = devm_kzalloc(&client->dev, sizeof(*da9150), GFP_KERNEL); > > if (!da9150) > > return -ENOMEM; > > Ok, no real difference, but can change it. Not functionally, but we like to do things as succinctly as possible.
T24gU2VwdGVtYmVyIDEwLCAyMDE0IDEwOjUwLCBMZWUgSm9uZXMgd3JvdGU6DQoNCj4gT24gVHVl LCAwOSBTZXAgMjAxNCwgT3BlbnNvdXJjZSBbQWRhbSBUaG9tc29uXSB3cm90ZToNCj4gDQo+ID4g T24gQXVndXN0IDI4LCAyMDE0IDE3OjM2LCBMZWUgSm9uZXMgd3JvdGU6DQo+ID4NCj4gPiBUaGFu a3MgZm9yIHRoZSBmZWVkYmFjay4gQXMgYSBnZW5lcmFsIGNvbW1lbnQgYSBjb3VwbGUgb2YgdGhl IGl0ZW1zIHlvdSd2ZQ0KPiA+IGlkZW50aWZpZWQgcmVsYXRlIHRvIGZ1dHVyZSB1cGRhdGVzIChh ZGRpdGlvbmFsIGZ1bmN0aW9uYWxpdHkgYmVpbmcgYWRkZWQpLg0KPiA+IEkgYWxyZWFkeSBoYXZl IGNvZGUgaW4gcGxhY2UgZm9yIHRoaXMgYnV0IGhhdmUgc3RyaXBwZWQgb3V0IGEgY291cGxlIG9m IHRoZQ0KPiA+IGRyaXZlcnMganVzdCB0byByZWR1Y2UgdGhlIGNodXJuIGFuZCBzaXplIG9mIHBh dGNoIHN1Ym1pc3Npb24uIFRoZXNlIHdpbGwgYmUNCj4gPiBhZGRlZCBvbmNlIHRoZXNlIHBhdGNo ZXMgaGF2ZSBiZWVuIGFjY2VwdGVkLg0KPiA+DQo+ID4gV2hlcmUgdGhpcyBpcyB0aGUgY2FzZSwg SSBoYXZlIGFkZGVkIG5vdGVzIGluLWxpbmUgYWdhaW5zdCB0aGUgcmVsZXZhbnQNCj4gPiBjb21t ZW50cyB5b3UgbWFkZS4NCj4gPg0KPiA+ID4gT24gVGh1LCAyOCBBdWcgMjAxNCwgQWRhbSBUaG9t c29uIHdyb3RlOg0KPiA+ID4NCj4gPiA+ID4gREE5MTUwIGlzIGEgY29tYmluZWQgQ2hhcmdlciBh bmQgRnVlbC1HYXVnZSBJQywgd2l0aCBhZGRpdGlvbmFsDQo+ID4gPiA+IEdQSU8gYW5kIEdQQURD IGZ1bmN0aW9uYWxpdHkuDQo+ID4gPiA+DQo+ID4gPiA+IFNpZ25lZC1vZmYtYnk6IEFkYW0gVGhv bXNvbiA8QWRhbS5UaG9tc29uLk9wZW5zb3VyY2VAZGlhc2VtaS5jb20+DQo+ID4gPiA+IC0tLQ0K PiA+ID4gPiAgZHJpdmVycy9tZmQvS2NvbmZpZyAgICAgICAgICAgICAgICAgIHwgICAxMiArDQo+ ID4gPiA+ICBkcml2ZXJzL21mZC9NYWtlZmlsZSAgICAgICAgICAgICAgICAgfCAgICAyICsNCj4g PiA+ID4gIGRyaXZlcnMvbWZkL2RhOTE1MC1jb3JlLmMgICAgICAgICAgICB8ICAzMzIgKysrKysr KysrKw0KPiA+ID4gPiAgZHJpdmVycy9tZmQvZGE5MTUwLWkyYy5jICAgICAgICAgICAgIHwgIDE3 NiArKysrKysNCj4gPiA+DQo+ID4gPiBEbyB5b3UgYWxzbyBoYXZlIGFub3RoZXIsIHNheSBTUEkg dmVyc2lvbj8NCj4gPg0KPiA+IE5vLCBub3QgeWV0LCBidXQgdGhpcyBpcyBzb21ldGhpbmcgdGhh dCB3ZSBtYXkgYWRkIGxhdGVyIGFzIHRoZSBkZXZpY2UgZG9lcw0KPiA+IHN1cHBvcnQgU1BJLg0K PiANCj4gSSdtIG5vdCBzdXJlIHdlIHdhbnQgdG8gc3BsaXQgdXAgdGhlIGZpbGVzIGxpa2UgdGhp cyBmb3IgYW4gJ2lmIHdlDQo+IGRlY2lkZSB0byBwcm9kdWNlIGFuIFNQSSB2YXJpYW50IGluIHRo ZSBmdXR1cmUnLiAgSWYgeW91IGRvLCB0aGVuIHlvdQ0KPiBjYW4gc3BsaXQgdGhlIGZpbGVzIHVw LiAgVW50aWwgdGhlbiwgc3RpY2sgZXZlcnl0aGluZyBpbiAtY29yZS4NCg0KUmlnaHQuIENhbid0 IHNheSBJIGFncmVlIGhlcmUsIGJ1dCB3aWxsIHJlZmFjdG9yLg0KDQo+IA0KPiANCj4gPiA+ID4g IGluY2x1ZGUvbGludXgvbWZkL2RhOTE1MC9jb3JlLmggICAgICB8ICAgODAgKysrDQo+ID4gPiA+ ICBpbmNsdWRlL2xpbnV4L21mZC9kYTkxNTAvcGRhdGEuaCAgICAgfCAgIDIxICsNCj4gPiA+ID4g IGluY2x1ZGUvbGludXgvbWZkL2RhOTE1MC9yZWdpc3RlcnMuaCB8IDExNTMNCj4gPiA+ICsrKysr KysrKysrKysrKysrKysrKysrKysrKysrKysrKysNCj4gPiA+ID4gIDcgZmlsZXMgY2hhbmdlZCwg MTc3NiBpbnNlcnRpb25zKCspDQo+ID4gPiA+ICBjcmVhdGUgbW9kZSAxMDA2NDQgZHJpdmVycy9t ZmQvZGE5MTUwLWNvcmUuYw0KPiA+ID4gPiAgY3JlYXRlIG1vZGUgMTAwNjQ0IGRyaXZlcnMvbWZk L2RhOTE1MC1pMmMuYw0KPiA+ID4gPiAgY3JlYXRlIG1vZGUgMTAwNjQ0IGluY2x1ZGUvbGludXgv bWZkL2RhOTE1MC9jb3JlLmgNCj4gPiA+ID4gIGNyZWF0ZSBtb2RlIDEwMDY0NCBpbmNsdWRlL2xp bnV4L21mZC9kYTkxNTAvcGRhdGEuaA0KPiA+ID4gPiAgY3JlYXRlIG1vZGUgMTAwNjQ0IGluY2x1 ZGUvbGludXgvbWZkL2RhOTE1MC9yZWdpc3RlcnMuaA0KPiA+ID4gPg0KPiA+ID4gPiBkaWZmIC0t Z2l0IGEvZHJpdmVycy9tZmQvS2NvbmZpZyBiL2RyaXZlcnMvbWZkL0tjb25maWcNCj4gPiA+ID4g aW5kZXggZGU1YWJmMi4uNzZhZGIyYyAxMDA2NDQNCj4gPiA+ID4gLS0tIGEvZHJpdmVycy9tZmQv S2NvbmZpZw0KPiA+ID4gPiArKysgYi9kcml2ZXJzL21mZC9LY29uZmlnDQo+ID4gPiA+IEBAIC0x ODMsNiArMTgzLDE4IEBAIGNvbmZpZyBNRkRfREE5MDYzDQo+ID4gPiA+ICAJICBBZGRpdGlvbmFs IGRyaXZlcnMgbXVzdCBiZSBlbmFibGVkIGluIG9yZGVyIHRvIHVzZSB0aGUgZnVuY3Rpb25hbGl0 eQ0KPiA+ID4gPiAgCSAgb2YgdGhlIGRldmljZS4NCj4gPiA+ID4NCj4gPiA+ID4gK2NvbmZpZyBN RkRfREE5MTUwDQo+ID4gPiA+ICsJYm9vbCAiRGlhbG9nIFNlbWljb25kdWN0b3IgREE5MTUwIENo YXJnZXIgRnVlbC1HYXVnZSBjaGlwIg0KPiA+ID4NCj4gPiA+IFdoeSBjYW4ndCB0aGlzIGJlIGJ1 aWx0IGFzIGEgbW9kdWxlPw0KPiA+DQo+ID4gTm8gcmVhc29uLiBXaWxsIGNoYW5nZSBpdC4NCj4g Pg0KPiA+ID4NCj4gPiA+ID4gKwlkZXBlbmRzIG9uIEkyQz15DQo+ID4gPiA+ICsJc2VsZWN0IE1G RF9DT1JFDQo+ID4gPiA+ICsJc2VsZWN0IFJFR01BUF9JMkMNCj4gPiA+ID4gKwlzZWxlY3QgUkVH TUFQX0lSUQ0KPiA+ID4gPiArCWhlbHANCj4gPiA+ID4gKwkgIFRoaXMgYWRkcyBzdXBwb3J0IGZv ciB0aGUgREE5MTUwIGludGVncmF0ZWQgY2hhcmdlciBhbmQgZnVlbC1nYXVnZQ0KPiA+ID4gPiAr CSAgY2hpcC4gVGhpcyBkcml2ZXIgcHJvdmlkZXMgY29tbW9uIHN1cHBvcnQgZm9yIGFjY2Vzc2lu ZyB0aGUgZGV2aWNlLg0KPiA+ID4gPiArCSAgQWRkaXRpb25hbCBkcml2ZXJzIG11c3QgYmUgZW5h YmxlZCBpbiBvcmRlciB0byB1c2UgdGhlIHNwZWNpZmljDQo+ID4gPiA+ICsJICBmZWF0dXJlcyBv ZiB0aGUgZGV2aWNlLg0KPiA+ID4gPiArDQo+ID4gPiA+ICBjb25maWcgTUZEX01DMTNYWFgNCj4g PiA+ID4gIAl0cmlzdGF0ZQ0KPiA+ID4gPiAgCWRlcGVuZHMgb24gKFNQSV9NQVNURVIgfHwgSTJD KQ0KPiA+ID4gPiBkaWZmIC0tZ2l0IGEvZHJpdmVycy9tZmQvTWFrZWZpbGUgYi9kcml2ZXJzL21m ZC9NYWtlZmlsZQ0KPiA+ID4gPiBpbmRleCBmMDAxNDg3Li4wOThkZmExIDEwMDY0NA0KPiA+ID4g PiAtLS0gYS9kcml2ZXJzL21mZC9NYWtlZmlsZQ0KPiA+ID4gPiArKysgYi9kcml2ZXJzL21mZC9N YWtlZmlsZQ0KPiA+ID4gPiBAQCAtMTE0LDYgKzExNCw4IEBAIG9iai0kKENPTkZJR19NRkRfREE5 MDU1KQkrPSBkYTkwNTUubw0KPiA+ID4gPiAgZGE5MDYzLW9ianMJCQk6PSBkYTkwNjMtY29yZS5v IGRhOTA2My1pcnEubyBkYTkwNjMtDQo+IGkyYy5vDQo+ID4gPiA+ICBvYmotJChDT05GSUdfTUZE X0RBOTA2MykJKz0gZGE5MDYzLm8NCj4gPiA+ID4NCj4gPiA+ID4gK29iai0kKENPTkZJR19NRkRf REE5MTUwKQkrPSBkYTkxNTAtY29yZS5vIGRhOTE1MC1pMmMubw0KPiA+ID4gPiArDQo+ID4gPg0K PiA+ID4gRG8gdGhlIG90aGVyIGRyaXZlcnMgc21lbGw/ICBQbGVhc2UgYnV0dCB1cCBhZ2FpbnN0 IHRoZW0uDQo+ID4gPg0KPiA+ID4gSSdtIG5vdCBlbnRpcmVseSBzdXJlIHdoeSB0aGVyZSBhcmUg c28gbWFueSAnXG4ncyBpbiB0aGUgTWFrZWZpbGUhDQo+ID4NCj4gPiBPa2V5IGRva2V5LiBXaWxs IGNoYW5nZS4NCj4gPg0KPiA+ID4NCj4gPiA+ID4gIG9iai0kKENPTkZJR19NRkRfTUFYMTQ1Nzcp CSs9IG1heDE0NTc3Lm8NCj4gPiA+ID4gIG9iai0kKENPTkZJR19NRkRfTUFYNzc2ODYpCSs9IG1h eDc3Njg2Lm8NCj4gPiA+ID4gIG9iai0kKENPTkZJR19NRkRfTUFYNzc2OTMpCSs9IG1heDc3Njkz Lm8NCj4gPiA+ID4gZGlmZiAtLWdpdCBhL2RyaXZlcnMvbWZkL2RhOTE1MC1jb3JlLmMgYi9kcml2 ZXJzL21mZC9kYTkxNTAtY29yZS5jDQo+ID4gPiA+IG5ldyBmaWxlIG1vZGUgMTAwNjQ0DQo+ID4g PiA+IGluZGV4IDAwMDAwMDAuLjAyOWEzMGINCj4gPiA+ID4gLS0tIC9kZXYvbnVsbA0KPiA+ID4g PiArKysgYi9kcml2ZXJzL21mZC9kYTkxNTAtY29yZS5jDQo+ID4gPiA+IEBAIC0wLDAgKzEsMzMy IEBADQo+ID4gPiA+ICsvKg0KPiA+ID4gPiArICogREE5MTUwIENvcmUgTUZEIERyaXZlcg0KPiA+ ID4gPiArICoNCj4gPiA+ID4gKyAqIENvcHlyaWdodCAoYykgMjAxNCBEaWFsb2cgU2VtaWNvbmR1 Y3Rvcg0KPiA+ID4gPiArICoNCj4gPiA+ID4gKyAqIEF1dGhvcjogQWRhbSBUaG9tc29uIDxBZGFt LlRob21zb24uT3BlbnNvdXJjZUBkaWFzZW1pLmNvbT4NCj4gPiA+ID4gKyAqDQo+ID4gPiA+ICsg KiBUaGlzIHByb2dyYW0gaXMgZnJlZSBzb2Z0d2FyZTsgeW91IGNhbiByZWRpc3RyaWJ1dGUgIGl0 IGFuZC9vciBtb2RpZnkgaXQNCj4gPiA+ID4gKyAqIHVuZGVyICB0aGUgdGVybXMgb2YgIHRoZSBH TlUgR2VuZXJhbCAgUHVibGljIExpY2Vuc2UgYXMgcHVibGlzaGVkIGJ5IHRoZQ0KPiA+ID4gPiAr ICogRnJlZSBTb2Z0d2FyZSBGb3VuZGF0aW9uOyAgZWl0aGVyIHZlcnNpb24gMiBvZiB0aGUgIExp Y2Vuc2UsIG9yIChhdCB5b3VyDQo+ID4gPiA+ICsgKiBvcHRpb24pIGFueSBsYXRlciB2ZXJzaW9u Lg0KPiA+ID4gPiArICovDQo+ID4gPiA+ICsNCj4gPiA+ID4gKyNpbmNsdWRlIDxsaW51eC9rZXJu ZWwuaD4NCj4gPiA+ID4gKyNpbmNsdWRlIDxsaW51eC9tb2R1bGUuaD4NCj4gPiA+ID4gKyNpbmNs dWRlIDxsaW51eC9wbGF0Zm9ybV9kZXZpY2UuaD4NCj4gPiA+ID4gKyNpbmNsdWRlIDxsaW51eC9z bGFiLmg+DQo+ID4gPiA+ICsjaW5jbHVkZSA8bGludXgvaXJxLmg+DQo+ID4gPiA+ICsjaW5jbHVk ZSA8bGludXgvaW50ZXJydXB0Lmg+DQo+ID4gPiA+ICsjaW5jbHVkZSA8bGludXgvbWZkL2NvcmUu aD4NCj4gPiA+ID4gKw0KPiA+ID4NCj4gPiA+IE5vIHJlYWwgbmVlZCBmb3IgdGhpcyAnXG4nLg0K PiA+DQo+ID4gSSBjYW4gY2hhbmdlIHRoaXMsIGJ1dCBteSByZWFzb24gd2FzIHRvIHNlcGFyYXRl IGNvbW1vbiBrZXJuZWwgaW5jbHVkZXMgZnJvbQ0KPiA+IGRldmljZSBzcGVjaWZpYyBvbmVzLCBm b3IgcmVhZGFiaWxpdHkuDQo+IA0KPiBJdCBpc24ndCBhbnkgbGVzcyByZWFkYWJsZSB3aXRoIHRo ZSAnXG4nIHJlbW92ZWQuDQoNCkkgcHJlZmVyIHdpdGgsIGJ1dCBwZXJzb25hbCBwcmVmZXJlbmNl IEkgZ3Vlc3MuIEFueXdheSwgd2lsbCB1cGRhdGUuDQoNCj4gDQo+ID4gPiA+ICsjaW5jbHVkZSA8 bGludXgvbWZkL2RhOTE1MC9jb3JlLmg+DQo+ID4gPiA+ICsjaW5jbHVkZSA8bGludXgvbWZkL2Rh OTE1MC9yZWdpc3RlcnMuaD4NCj4gPiA+ID4gKyNpbmNsdWRlIDxsaW51eC9tZmQvZGE5MTUwL3Bk YXRhLmg+DQo+ID4gPiA+ICsNCj4gPiA+ID4gK3U4IGRhOTE1MF9yZWdfcmVhZChzdHJ1Y3QgZGE5 MTUwICpkYTkxNTAsIHUxNiByZWcpDQo+ID4gPiA+ICt7DQo+ID4gPiA+ICsJaW50IHZhbCwgcmV0 Ow0KPiA+ID4gPiArDQo+ID4gPiA+ICsJcmV0ID0gcmVnbWFwX3JlYWQoZGE5MTUwLT5yZWdtYXAs IHJlZywgJnZhbCk7DQo+ID4gPiA+ICsJaWYgKHJldCA8IDApDQo+ID4gPg0KPiA+ID4gV2hhdCBp ZiByZXQgPiAwPyAgSXMgdGhhdCBhIGdvb2QgdGhpbmc/IDopDQo+ID4gPg0KPiA+ID4gSnVzdCAn aWYgKHJldCknLg0KPiA+DQo+ID4gRmluZSwgd2lsbCBjaGFuZ2UuDQo+ID4NCj4gPiA+DQo+ID4g PiA+ICsJCWRldl9lcnIoZGE5MTUwLT5kZXYsICJGYWlsZWQgdG8gcmVhZCBmcm9tIHJlZyAweCV4 OiAlZFxuIiwNCj4gPiA+ID4gKwkJCXJlZywgcmV0KTsNCj4gPiA+ID4gKw0KPiA+ID4gPiArCXJl dHVybiAodTgpIHZhbDsNCj4gPiA+ID4gK30NCj4gPiA+ID4gK0VYUE9SVF9TWU1CT0xfR1BMKGRh OTE1MF9yZWdfcmVhZCk7DQo+ID4gPg0KPiA+ID4gTm90IHN1cmUgSSBsaWtlIHRoaXMgYWJzdHJh Y3Rpb24gc3R1ZmYuICBJIGNvdWxkIHVuZGVyc3RhbmQgaWYgdGhlcmUNCj4gPiA+IHdlcmUgbG9j a2luZyBpbnZvbHZlZCwgYnV0IHRoZXJlIGlzbid0LiAgWW91IGRvbid0IGFwcGVhciB0byBjaGVj ayBmb3INCj4gPiA+IGVycm9ycyBpbiB0aGUgc3Vib3JkaW5hdGUgZHJpdmVycyBlaXRoZXIsIHJh dGhlciB5b3UganVzdCBwbG91Z2ggb24NCj4gPiA+IGFoZWFkLiAgTm90IHN1cmUgdGhhdCdzIGEg Z29vZCBpZGVhIGVpdGhlci4NCj4gPiA+DQo+ID4gPiBBbnlvbmUgaGF2ZSBhIHNlY29uZCBvcGlu aW9uPw0KPiA+DQo+ID4gVGhlIHJlYXNvbiBmb3IgdGhlc2UgaXMgYmVjYXVzZSBmdXR1cmUgcGF0 Y2hlcyB0byBhZGQgYWRkaXRpb25hbCBmdW5jdGlvbmFsaXR5DQo+ID4gd2lsbCBpbnRyb2R1Y2Ug STJDIGFjY2VzcyBmdW5jdGlvbnMgd2hpY2ggZG8gbm90IHVzZSByZWdtYXAgYW5kIGFjY2VzcyB0 aGUNCj4gPiBkZXZpY2UgdmlhIGEgc2VwYXJhdGUgSTJDIGFkZHJlc3MgZm9yIHRoaXMgcHVycG9z ZS4gSSB3aWxsIG5lZWQgdG8gcHJvdmlkZQ0KPiA+IGFjY2VzcyBmdW5jdGlvbnMgZm9yIHRoYXQs IGFuZCBzbyBoYXZpbmcgYSBjb21tb24gc3R5bGUgb2YgSTJDIGFjY2VzcyBtYWtlcw0KPiA+IHNl bnNlIGZvciB0aGlzIGRyaXZlci4gTWVhbnMgYW55IGFjY2VzcyBqdXN0IG5lZWRzIHRvIHByb3Zp ZGUgdGhlIE1GRCBwcml2YXRlDQo+ID4gZGF0YSwgYW5kIHRoZSByZWxldmFudCBmdW5jdGlvbnMg dGFrZSBjYXJlIG9mIHRoZSByZXN0LiBJIHRoaW5rIHRoaXMgaXMgY2xlYW5lcg0KPiA+IGluIHRo aXMgaW5zdGFuY2UuDQo+IA0KPiBTbyBsb25nIGFzIHRoZXNlIGFwcGVhciBzb29uLiAgT3RoZXJ3 aXNlIGl0J3MganVzdCBjcnVmdC4NCg0KSGF2ZSBvdGhlciBwYXRjaGVzIGFscmVhZHkgaW4gcGxh Y2UgYW5kIHJlYWR5IHRvIGdvLiBXaGVuIHRoaXMgcGF0Y2ggc2V0IGlzDQphY2NlcHRlZCwgSSB3 aWxsIGJlZ2luIHN1Ym1pc3Npb24gb2YgdGhlIHJlbWFpbmluZyBkcml2ZXJzLg0KDQo+IA0KPiBb Li4uXQ0KPiANCj4gPiA+ID4gKy8qIEhlbHBlciBmdW5jdGlvbnMgZm9yIHN1Yi1kZXZpY2VzIHRv IHJlcXVlc3QvZnJlZSBJUlFzICovDQo+ID4gPiA+ICtpbnQgZGE5MTUwX3JlZ2lzdGVyX2lycShz dHJ1Y3QgcGxhdGZvcm1fZGV2aWNlICpwZGV2LCB2b2lkICpkZXZfaWQsDQo+ID4gPiA+ICsJCQlp cnFfaGFuZGxlcl90IGhhbmRsZXIsIGNvbnN0IGNoYXIgKm5hbWUpDQo+ID4gPiA+ICt7DQo+ID4g PiA+ICsJaW50IGlycSwgcmV0Ow0KPiA+ID4gPiArDQo+ID4gPiA+ICsJaXJxID0gcGxhdGZvcm1f Z2V0X2lycV9ieW5hbWUocGRldiwgbmFtZSk7DQo+ID4gPiA+ICsJaWYgKGlycSA8IDApDQo+ID4g PiA+ICsJCXJldHVybiBpcnE7DQo+ID4gPiA+ICsNCj4gPiA+ID4gKwlyZXQgPSBkZXZtX3JlcXVl c3RfdGhyZWFkZWRfaXJxKCZwZGV2LT5kZXYsIGlycSwgTlVMTCwgaGFuZGxlciwNCj4gPiA+ID4g KwkJCQkJSVJRRl9PTkVTSE9ULCBuYW1lLCBkZXZfaWQpOw0KPiA+ID4gPiArCWlmIChyZXQpDQo+ ID4gPiA+ICsJCWRldl9lcnIoJnBkZXYtPmRldiwgIkZhaWxlZCB0byByZXF1ZXN0IElSUSAlZDog JWRcbiIsIGlycSwgcmV0KTsNCj4gPiA+ID4gKw0KPiA+ID4gPiArCXJldHVybiByZXQ7DQo+ID4g PiA+ICt9DQo+ID4gPiA+ICtFWFBPUlRfU1lNQk9MX0dQTChkYTkxNTBfcmVnaXN0ZXJfaXJxKTsN Cj4gPiA+DQo+ID4gPiBXaHkgZG8gdGhleSBuZWVkIGhlbHA/ICBXaGF0IHByb2JsZW0gZG9lcyBh ZGRpbmcgdGhlc2UgbGF5ZXJzIHNvbHZlPw0KPiA+DQo+ID4gTWVhbnMgSSBkb24ndCBoYXZlIHRv IGtlZXAgYWRkaW5nIHByaW50IGVycm9yIGxpbmVzIGV2ZXJ5d2hlcmUgZWxzZSBpZiB0aGlzDQo+ ID4gZnVuY3Rpb24gdGFrZXMgY2FyZSBvZiBpdC4gVGhvdWdodCB0aGF0IHdvdWxkIGJlIGNsZWFu ZXIuDQo+IA0KPiBZb3Ugb25seSBuZWVkIHRvIHJlcXVlc3QgZWFjaCBJUlEgb25jZS4gIEl0J3Mg anVzdCBtb3JlIGFic3RyYWN0aW9uDQo+IGZvciB0aGUgc2FrZSBvZiBpdC4gIEkgd291bGQgcHJl ZmVyIGlmIHlvdSByZW1vdmVkIHRoZW0uDQoNClllcywgYnV0IGluIHRoZSBjaGFyZ2VyIGRyaXZl ciBmb3IgZXhhbXBsZSwgdGhlcmUgYXJlIDQgSVJRcyB0byByZXF1ZXN0LiBJZg0KSSBkb24ndCB1 c2UgdGhpcyBhcHByb2FjaCB0aGUgSVJRIHJlcXVlc3RpbmcgYmVjb21lcyBibG9hdGVkLCBoZW5j ZSB3aHkgSSB3ZW50DQpmb3IgYSBjb21tb24gZnVuY3Rpb24gbGlrZSB0aGlzLiBUaG91Z2h0IGdl bmVyYWxseSB0aGUgaW50ZW50aW9uIHdhcyB0byBjdXQNCmRvd24gb24gcmVwZWF0ZWQgY29kZT8N Cg0KPiANCj4gPiA+ID4gK3ZvaWQgZGE5MTUwX3JlbGVhc2VfaXJxKHN0cnVjdCBwbGF0Zm9ybV9k ZXZpY2UgKnBkZXYsIHZvaWQgKmRldl9pZCwNCj4gPiA+ID4gKwkJICAgICAgIGNvbnN0IGNoYXIg Km5hbWUpDQo+ID4gPiA+ICt7DQo+ID4gPiA+ICsJaW50IGlycTsNCj4gPiA+ID4gKw0KPiA+ID4g PiArCWlycSA9IHBsYXRmb3JtX2dldF9pcnFfYnluYW1lKHBkZXYsIG5hbWUpOw0KPiA+ID4gPiAr CWlmIChpcnEgPCAwKQ0KPiA+ID4gPiArCQlyZXR1cm47DQo+ID4gPiA+ICsNCj4gPiA+ID4gKwlk ZXZtX2ZyZWVfaXJxKCZwZGV2LT5kZXYsIGlycSwgZGV2X2lkKTsNCj4gPiA+ID4gK30NCj4gPiA+ ID4gK0VYUE9SVF9TWU1CT0xfR1BMKGRhOTE1MF9yZWxlYXNlX2lycSk7DQo+ID4gPg0KPiA+ID4g RG8geW91IGV2ZXIgcmVsZWFzZSB0aGUgSVJRIGFuZCBub3QgdW5iaW5kIHRoZSBkcml2ZXI/DQo+ ID4gPg0KPiA+ID4gQXJlIHRoZXJlIG9yZGVyaW5nIGlzc3VlcyBhdCBwbGF5IGhlcmU/DQo+ID4g Pg0KPiA+ID4gSWYgbm90LCB0aGVyZSdzIG5vIG5lZWQgdG8gY29uZHVjdCBhIG1hbnVhbCBmcmVl Lg0KPiA+DQo+ID4gSW4gdGhlIGNoYXJnZXIgZHJpdmVyLCBpbiB0aGUgcmVtb3ZlIGZ1bmN0aW9u IHRoZXJlIGlzIGEgbmVlZCBJIGJlbGlldmUgdG8NCj4gPiBmcmVlIHRoZSBJUlFzIGJlZm9yZSBv dGhlciBpdGVtcyBhcmUgY2xlYXJlZCB1cCAoZS5nLiBwb3dlcl9zdXBwbHkgY2xhc3NlcyksDQo+ ID4gc28gdGhpcyBpcyB3aHkgSSBoYXZlIGFkZGVkIHRoaXMgaW4gaGVyZS4NCj4gDQo+IENhbiB5 b3UgaGFuZGxlIHRoaXMgc2VwYXJhdGVseSBpbiB0aGUgQ2hhcmdlciBkcml2ZXIgdGhlbiBwbGVh c2U/DQo+IA0KPiBbLi4uXQ0KDQpJZiBJIGhhdmUgdG8gcmVtb3ZlIHRoZSBJUlEgcmVnaXN0ZXIg ZnVuY3Rpb24sIHRoZW4geWVzLCBvdGhlcndpc2UgaXQgbWFrZXMgbW9yZQ0Kc2Vuc2UgdG8gaGF2 ZSB0aGUgcGFpciBvZiBmdW5jdGlvbnMgaW4gdGhlIE1GRCBjb3JlIEkgd291bGQgc2F5Lg0KDQo+ IA0KPiA+ID4gPiArCWlmIChwZGF0YSkNCj4gPiA+ID4gKwkJZGE5MTUwLT5pcnFfYmFzZSA9IHBk YXRhLT5pcnFfYmFzZTsNCj4gPiA+ID4gKwllbHNlDQo+ID4gPiA+ICsJCWRhOTE1MC0+aXJxX2Jh c2UgPSAtMTsNCj4gPiA+DQo+ID4gPiBwZGF0YSA/IHBkYXRhLT5pcnFfYmFzZSA6IC0xOw0KPiA+ DQo+ID4gVGhpcyBpcyBsZWZ0IHRoaXMgd2F5IGFzIGxhdGVyIHVwZGF0ZXMgdG8gYWRkIGFkZGl0 aW9uYWwgZnVuY3Rpb25hbGl0eSB3aWxsDQo+ID4gcmVxdWlyZSBhZGR0aW9uYWwgd29yayB0byBi ZSBkb25lIHdpdGggdGhlIHBsYXRmb3JtIGRhdGEuIFNlZW1lZCBwb2ludGxlc3MNCj4gPiBjaGFu Z2luZyBpdCBoZXJlIGp1c3QgdG8gY2hhbmdlIGl0IGJhY2sgbGF0ZXIuDQo+IA0KPiBZb3UncmUg bm90IGNoYW5naW5nIGFueXRoaW5nLCBhcyB0aGlzIGlzIHRoZSBpbnRyb2R1Y3Rpb24gb2YgdGhl IGNvZGUuDQo+IEkgY2FuJ3QgdGVsbCB5b3UgaG93IG1hbnkgdGltZXMgSSd2ZSBoZWFyZCAiSSB3 aWxsIGNoYW5nZSBpdCBsYXRlciIsDQo+IG9yICJkb2luZyBpdCB0aGlzIHdheSB3aWxsIHN1cHBv cnQgc3Vic2VxdWVudCBzdWJtaXNzaW9ucyIsIHRoZW4NCj4gcmVjZWl2ZWQgbm8gbW9yZSBwYXRj aGVzLiAgSXQncyBva2F5IHRvIGRvIGl0IG5pY2VseSBub3cgYW5kIGV4cGFuZA0KPiBpdCBiYWNr IG91dCBpbiB0aGUgbmV3IHBhdGNoZXMuDQo+IA0KPiBbLi4uXQ0KDQpJdCBhcHBlYXJzIHRoYXQg d2F5IHRvIHlvdSBidXQgSSBoYXZlIHRvIG1vZGlmeSBteSBjb2RlIGZvciBzdW1iaXNzaW9uIGFz IHRoZQ0KbG9jYWwgY29kZSBJIGhhdmUgY292ZXJzIGFsbCBmdW5jdGlvbmFsaXR5LiBBbSBoYXZp bmcgdG8gcmVmYWN0b3IgYWdhaW4gYW5kDQphZ2FpbiBqdXN0IHRvIHN1aXQgdGhpcyBpbml0aWFs IHN1Ym1pc3Npb24sIGFuZCB0aGVuIEkgaGF2ZSB0byByZXZlcnQgaXQgYmFjaw0KYWdhaW4gd2hl biBzdWJtaXR0aW5nIHRoZSBsYXN0IGNvdXBsZSBvZiBkcml2ZXJzLiBUaW1lIGNvbnN1bWluZywg YW5kIHF1aXRlDQpmcnVzdHJhdGluZyB3aGVuIHRoZSBpbnRlbnRpb24gd2FzIHRvIG1ha2UgdGhl IHdob2xlIHByb2Nlc3MgZWFzaWVyLiBBbnl3YXksDQp3aWxsIHVwZGF0ZSBmb3Igbm93IGFuZCBy ZXZlcnQgaW4gc3Vic2VxdWVudCBwYXRjaGVzLg0KDQo+IA0KPiA+ID4gPiArCWRhOTE1MCA9IGRl dm1fa3phbGxvYygmY2xpZW50LT5kZXYsIHNpemVvZihzdHJ1Y3QgZGE5MTUwKSwgR0ZQX0tFUk5F TCk7DQo+ID4gPg0KPiA+ID4gc2l6ZW9mKCpkYTkxNTApDQo+ID4NCj4gPiBTYW1lIGRpZmZlcmVu Y2UsIGJ1dCBvay4NCj4gDQo+IEl0J3MgbW9yZSBzdWNjaW5jdCBhbmQgYWxtb3N0IGFsd2F5cyBk b25lIHRoaXMgd2F5IGJlY2F1c2Ugb2YgaXQuDQo+IA0KPiBbLi4uXQ0KPiANCj4gPiA+ID4gK3N0 cnVjdCBkYTkxNTBfcGRhdGEgew0KPiA+ID4gPiArCWludCBpcnFfYmFzZTsNCj4gPiA+ID4gK307 DQo+ID4gPg0KPiA+ID4gSnVzdCBwdXQgdGhpcyBpbiBjb3JlLmggYW5kIGRvIGF3YXkgd2l0aHQg dGhpcyBoZWFkZXIgZmlsZS4NCj4gPg0KPiA+IFRoZSByZWFzb24gZm9yIHRoaXMgaXMgdGhhdCBJ IHdpbGwgYWRkIG1vcmUgcGxhdGZvcm0gZGF0YSBpdGVtcyBsYXRlciB3aXRoDQo+ID4gc3Vic2Vx dWVudCBzdWJtaXNzaW9ucyBmb3IgYWRkaXRpb25hbCBmZWF0dXJlcy4gSXQgZmVsdCBjbGVhbmVy IHRvIHNlcGFyYXRlIG91dA0KPiA+IHRoZXNlIHN0cnVjdHVyZXMgdGhhbiB0aHJvdyBpdCBpbiB0 aGUgY29yZS5oIGhlYWRlci4gSG93ZXZlciBpZiBpdCdzIGdvaW5nIHRvDQo+ID4gYmUgYSBwcm9i bGVtIEknbGwgZm9sZCB0aGlzIGludG8gY29yZS5oDQo+IA0KPiBNb3JlICJJIHdpbGwicy4gOikN Cj4gDQo+IFBsZWFzZSBkbyB3aGF0J3MgcmlnaHQgZm9yICd0aGlzIHN1Ym1pc3Npb24nLiAgSWYg dGhpbmdzIGNoYW5nZSBsYXRlcg0KPiB5b3UgY2FuIGFjdCBhY2NvcmRpbmdseS4NCg0KSXQncyBu b3QgYSBjYXNlIG9mICdpZiB0aGluZ3MgY2hhbmdlJy4gVGhleSB3aWxsLiBBbnl3YXksIHdpbGwg cmVmYWN0b3Igbm93IGFuZA0KcmV2ZXJ0IGxhdGVyLg0KDQo+IA0KPiAtLQ0KPiBMZWUgSm9uZXMN Cj4gTGluYXJvIFNUTWljcm9lbGVjdHJvbmljcyBMYW5kaW5nIFRlYW0gTGVhZA0KPiBMaW5hcm8u b3JnIOKUgiBPcGVuIHNvdXJjZSBzb2Z0d2FyZSBmb3IgQVJNIFNvQ3MNCj4gRm9sbG93IExpbmFy bzogRmFjZWJvb2sgfCBUd2l0dGVyIHwgQmxvZw0K -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 10 Sep 2014, Opensource [Adam Thomson] wrote: > On September 10, 2014 10:50, Lee Jones wrote: > > On Tue, 09 Sep 2014, Opensource [Adam Thomson] wrote: > > > > > On August 28, 2014 17:36, Lee Jones wrote: > > > > > > Thanks for the feedback. As a general comment a couple of the items you've > > > identified relate to future updates (additional functionality being added). > > > I already have code in place for this but have stripped out a couple of the > > > drivers just to reduce the churn and size of patch submission. These will be > > > added once these patches have been accepted. > > > > > > Where this is the case, I have added notes in-line against the relevant > > > comments you made. > > > > > > > On Thu, 28 Aug 2014, Adam Thomson wrote: > > > > > > > > > DA9150 is a combined Charger and Fuel-Gauge IC, with additional > > > > > GPIO and GPADC functionality. > > > > > > > > > > Signed-off-by: Adam Thomson <Adam.Thomson.Opensource@diasemi.com> > > > > > --- > > > > > drivers/mfd/Kconfig | 12 + > > > > > drivers/mfd/Makefile | 2 + > > > > > drivers/mfd/da9150-core.c | 332 ++++++++++ > > > > > drivers/mfd/da9150-i2c.c | 176 ++++++ [...] > > > > > +/* Helper functions for sub-devices to request/free IRQs */ > > > > > +int da9150_register_irq(struct platform_device *pdev, void *dev_id, > > > > > + irq_handler_t handler, const char *name) > > > > > +{ > > > > > + int irq, ret; > > > > > + > > > > > + irq = platform_get_irq_byname(pdev, name); > > > > > + if (irq < 0) > > > > > + return irq; > > > > > + > > > > > + ret = devm_request_threaded_irq(&pdev->dev, irq, NULL, handler, > > > > > + IRQF_ONESHOT, name, dev_id); > > > > > + if (ret) > > > > > + dev_err(&pdev->dev, "Failed to request IRQ %d: %d\n", irq, ret); > > > > > + > > > > > + return ret; > > > > > +} > > > > > +EXPORT_SYMBOL_GPL(da9150_register_irq); > > > > > > > > Why do they need help? What problem does adding these layers solve? > > > > > > Means I don't have to keep adding print error lines everywhere else if this > > > function takes care of it. Thought that would be cleaner. > > > > You only need to request each IRQ once. It's just more abstraction > > for the sake of it. I would prefer if you removed them. > > Yes, but in the charger driver for example, there are 4 IRQs to request. If > I don't use this approach the IRQ requesting becomes bloated, hence why I went > for a common function like this. Thought generally the intention was to cut > down on repeated code? If you're worried about bloat in .probe() it's okay to define a new function within the charger driver; however, creating a call-back into the MFD driver like this I think it over-kill for 4 requests. > > > > > +void da9150_release_irq(struct platform_device *pdev, void *dev_id, > > > > > + const char *name) > > > > > +{ > > > > > + int irq; > > > > > + > > > > > + irq = platform_get_irq_byname(pdev, name); > > > > > + if (irq < 0) > > > > > + return; > > > > > + > > > > > + devm_free_irq(&pdev->dev, irq, dev_id); > > > > > +} > > > > > +EXPORT_SYMBOL_GPL(da9150_release_irq); > > > > > > > > Do you ever release the IRQ and not unbind the driver? > > > > > > > > Are there ordering issues at play here? > > > > > > > > If not, there's no need to conduct a manual free. > > > > > > In the charger driver, in the remove function there is a need I believe to > > > free the IRQs before other items are cleared up (e.g. power_supply classes), > > > so this is why I have added this in here. > > > > Can you handle this separately in the Charger driver then please? > > > > [...] > > If I have to remove the IRQ register function, then yes, otherwise it makes more > sense to have the pair of functions in the MFD core I would say. I would prefer you to remove the call-back please. > > > > > + if (pdata) > > > > > + da9150->irq_base = pdata->irq_base; > > > > > + else > > > > > + da9150->irq_base = -1; > > > > > > > > pdata ? pdata->irq_base : -1; > > > > > > This is left this way as later updates to add additional functionality will > > > require addtional work to be done with the platform data. Seemed pointless > > > changing it here just to change it back later. > > > > You're not changing anything, as this is the introduction of the code. > > I can't tell you how many times I've heard "I will change it later", > > or "doing it this way will support subsequent submissions", then > > received no more patches. It's okay to do it nicely now and expand > > it back out in the new patches. > > > > [...] > > It appears that way to you but I have to modify my code for sumbission as the > local code I have covers all functionality. Am having to refactor again and > again just to suit this initial submission, and then I have to revert it back > again when submitting the last couple of drivers. Time consuming, and quite > frustrating when the intention was to make the whole process easier. Anyway, > will update for now and revert in subsequent patches. I sincerely hope the refactorings won't add too much effort, but it's difficult to have one rule for the masses and different ones for others.
On September 15, 2014 23:39, Lee Jones wrote: > On Wed, 10 Sep 2014, Opensource [Adam Thomson] wrote: > > On September 10, 2014 10:50, Lee Jones wrote: > > > On Tue, 09 Sep 2014, Opensource [Adam Thomson] wrote: > > > > > > > On August 28, 2014 17:36, Lee Jones wrote: > > > > > > > > Thanks for the feedback. As a general comment a couple of the items you've > > > > identified relate to future updates (additional functionality being added). > > > > I already have code in place for this but have stripped out a couple of the > > > > drivers just to reduce the churn and size of patch submission. These will be > > > > added once these patches have been accepted. > > > > > > > > Where this is the case, I have added notes in-line against the relevant > > > > comments you made. > > > > > > > > > On Thu, 28 Aug 2014, Adam Thomson wrote: > > > > > > > > > > > DA9150 is a combined Charger and Fuel-Gauge IC, with additional > > > > > > GPIO and GPADC functionality. > > > > > > > > > > > > Signed-off-by: Adam Thomson > <Adam.Thomson.Opensource@diasemi.com> > > > > > > --- > > > > > > drivers/mfd/Kconfig | 12 + > > > > > > drivers/mfd/Makefile | 2 + > > > > > > drivers/mfd/da9150-core.c | 332 ++++++++++ > > > > > > drivers/mfd/da9150-i2c.c | 176 ++++++ > > [...] > > > > > > > +/* Helper functions for sub-devices to request/free IRQs */ > > > > > > +int da9150_register_irq(struct platform_device *pdev, void *dev_id, > > > > > > + irq_handler_t handler, const char *name) > > > > > > +{ > > > > > > + int irq, ret; > > > > > > + > > > > > > + irq = platform_get_irq_byname(pdev, name); > > > > > > + if (irq < 0) > > > > > > + return irq; > > > > > > + > > > > > > + ret = devm_request_threaded_irq(&pdev->dev, irq, NULL, handler, > > > > > > + IRQF_ONESHOT, name, dev_id); > > > > > > + if (ret) > > > > > > + dev_err(&pdev->dev, "Failed to request IRQ %d: %d\n", irq, > ret); > > > > > > + > > > > > > + return ret; > > > > > > +} > > > > > > +EXPORT_SYMBOL_GPL(da9150_register_irq); > > > > > > > > > > Why do they need help? What problem does adding these layers solve? > > > > > > > > Means I don't have to keep adding print error lines everywhere else if this > > > > function takes care of it. Thought that would be cleaner. > > > > > > You only need to request each IRQ once. It's just more abstraction > > > for the sake of it. I would prefer if you removed them. > > > > Yes, but in the charger driver for example, there are 4 IRQs to request. If > > I don't use this approach the IRQ requesting becomes bloated, hence why I went > > for a common function like this. Thought generally the intention was to cut > > down on repeated code? > > If you're worried about bloat in .probe() it's okay to define a new > function within the charger driver; however, creating a call-back into > the MFD driver like this I think it over-kill for 4 requests. I could do something just in the charger, but why not have something which can be used for all sub-devices? There is also an IRQ used in the IIO ADC driver and there will be another in the later fuel-gauge driver too. Doesn't make sense to me not to do in the MFD code when that will provide a simple common call for all sub-devices. What is your concern with adding something like this, just so I'm clear? > > > > > > > +void da9150_release_irq(struct platform_device *pdev, void *dev_id, > > > > > > + const char *name) > > > > > > +{ > > > > > > + int irq; > > > > > > + > > > > > > + irq = platform_get_irq_byname(pdev, name); > > > > > > + if (irq < 0) > > > > > > + return; > > > > > > + > > > > > > + devm_free_irq(&pdev->dev, irq, dev_id); > > > > > > +} > > > > > > +EXPORT_SYMBOL_GPL(da9150_release_irq); > > > > > > > > > > Do you ever release the IRQ and not unbind the driver? > > > > > > > > > > Are there ordering issues at play here? > > > > > > > > > > If not, there's no need to conduct a manual free. > > > > > > > > In the charger driver, in the remove function there is a need I believe to > > > > free the IRQs before other items are cleared up (e.g. power_supply classes), > > > > so this is why I have added this in here. > > > > > > Can you handle this separately in the Charger driver then please? > > > > > > [...] > > > > If I have to remove the IRQ register function, then yes, otherwise it makes more > > sense to have the pair of functions in the MFD core I would say. > > I would prefer you to remove the call-back please. Right. > > > > > > > + if (pdata) > > > > > > + da9150->irq_base = pdata->irq_base; > > > > > > + else > > > > > > + da9150->irq_base = -1; > > > > > > > > > > pdata ? pdata->irq_base : -1; > > > > > > > > This is left this way as later updates to add additional functionality will > > > > require addtional work to be done with the platform data. Seemed pointless > > > > changing it here just to change it back later. > > > > > > You're not changing anything, as this is the introduction of the code. > > > I can't tell you how many times I've heard "I will change it later", > > > or "doing it this way will support subsequent submissions", then > > > received no more patches. It's okay to do it nicely now and expand > > > it back out in the new patches. > > > > > > [...] > > > > It appears that way to you but I have to modify my code for sumbission as the > > local code I have covers all functionality. Am having to refactor again and > > again just to suit this initial submission, and then I have to revert it back > > again when submitting the last couple of drivers. Time consuming, and quite > > frustrating when the intention was to make the whole process easier. Anyway, > > will update for now and revert in subsequent patches. > > I sincerely hope the refactorings won't add too much effort, but it's > difficult to have one rule for the masses and different ones for > others. I do understand that, and that's fair enough. Is just frustrating when you're trying to do a proper job. Anyway, am sure I'll live. :) > > -- > Lee Jones > Linaro STMicroelectronics Landing Team Lead > Linaro.org ? Open source software for ARM SoCs > Follow Linaro: Facebook | Twitter | Blog
On Tue, 16 Sep 2014, Opensource [Adam Thomson] wrote: > On September 15, 2014 23:39, Lee Jones wrote: > > > On Wed, 10 Sep 2014, Opensource [Adam Thomson] wrote: > > > On September 10, 2014 10:50, Lee Jones wrote: > > > > On Tue, 09 Sep 2014, Opensource [Adam Thomson] wrote: > > > > > > > > > On August 28, 2014 17:36, Lee Jones wrote: > > > > > > > > > > Thanks for the feedback. As a general comment a couple of the items you've > > > > > identified relate to future updates (additional functionality being added). > > > > > I already have code in place for this but have stripped out a couple of the > > > > > drivers just to reduce the churn and size of patch submission. These will be > > > > > added once these patches have been accepted. > > > > > > > > > > Where this is the case, I have added notes in-line against the relevant > > > > > comments you made. > > > > > > > > > > > On Thu, 28 Aug 2014, Adam Thomson wrote: > > > > > > > > > > > > > DA9150 is a combined Charger and Fuel-Gauge IC, with additional > > > > > > > GPIO and GPADC functionality. > > > > > > > > > > > > > > Signed-off-by: Adam Thomson > > <Adam.Thomson.Opensource@diasemi.com> > > > > > > > --- > > > > > > > drivers/mfd/Kconfig | 12 + > > > > > > > drivers/mfd/Makefile | 2 + > > > > > > > drivers/mfd/da9150-core.c | 332 ++++++++++ > > > > > > > drivers/mfd/da9150-i2c.c | 176 ++++++ > > > > [...] > > > > > > > > > +/* Helper functions for sub-devices to request/free IRQs */ > > > > > > > +int da9150_register_irq(struct platform_device *pdev, void *dev_id, > > > > > > > + irq_handler_t handler, const char *name) > > > > > > > +{ > > > > > > > + int irq, ret; > > > > > > > + > > > > > > > + irq = platform_get_irq_byname(pdev, name); > > > > > > > + if (irq < 0) > > > > > > > + return irq; > > > > > > > + > > > > > > > + ret = devm_request_threaded_irq(&pdev->dev, irq, NULL, handler, > > > > > > > + IRQF_ONESHOT, name, dev_id); > > > > > > > + if (ret) > > > > > > > + dev_err(&pdev->dev, "Failed to request IRQ %d: %d\n", irq, > > ret); > > > > > > > + > > > > > > > + return ret; > > > > > > > +} > > > > > > > +EXPORT_SYMBOL_GPL(da9150_register_irq); > > > > > > > > > > > > Why do they need help? What problem does adding these layers solve? > > > > > > > > > > Means I don't have to keep adding print error lines everywhere else if this > > > > > function takes care of it. Thought that would be cleaner. > > > > > > > > You only need to request each IRQ once. It's just more abstraction > > > > for the sake of it. I would prefer if you removed them. > > > > > > Yes, but in the charger driver for example, there are 4 IRQs to request. If > > > I don't use this approach the IRQ requesting becomes bloated, hence why I went > > > for a common function like this. Thought generally the intention was to cut > > > down on repeated code? > > > > If you're worried about bloat in .probe() it's okay to define a new > > function within the charger driver; however, creating a call-back into > > the MFD driver like this I think it over-kill for 4 requests. > > I could do something just in the charger, but why not have something which can > be used for all sub-devices? There is also an IRQ used in the IIO ADC driver and > there will be another in the later fuel-gauge driver too. Doesn't make sense to > me not to do in the MFD code when that will provide a simple common call for all > sub-devices. What is your concern with adding something like this, just so I'm > clear? I guess my last response wasn't as descriptive as it could have been. I don't think that any helper function is required at all. No need to abstract/obfuscate how the IRQ is obtained and registered. What I meant by 'do it in the charger driver' was, copy and paste all of the platform_get_irq_byname() and devm_request_threaded_irq() calls from .probe() into a separate function, but only if you are worried about a bloated .probe(). Personally I'd just leave them in there. Bottom line is; I don't feel there is a necessity for any helper function here. I think it adds unnecessary complexity for the sake of saving a few lines of code. If you still think there is a requirement for it, perhaps a more system-wide devm_request_threaded_irq_byname() is in order instead? > > > > > > > +void da9150_release_irq(struct platform_device *pdev, void *dev_id, > > > > > > > + const char *name) > > > > > > > +{ > > > > > > > + int irq; > > > > > > > + > > > > > > > + irq = platform_get_irq_byname(pdev, name); > > > > > > > + if (irq < 0) > > > > > > > + return; > > > > > > > + > > > > > > > + devm_free_irq(&pdev->dev, irq, dev_id); > > > > > > > +} > > > > > > > +EXPORT_SYMBOL_GPL(da9150_release_irq); > > > > > > > > > > > > Do you ever release the IRQ and not unbind the driver? > > > > > > > > > > > > Are there ordering issues at play here? > > > > > > > > > > > > If not, there's no need to conduct a manual free. > > > > > > > > > > In the charger driver, in the remove function there is a need I believe to > > > > > free the IRQs before other items are cleared up (e.g. power_supply classes), > > > > > so this is why I have added this in here. > > > > > > > > Can you handle this separately in the Charger driver then please? > > > > > > > > [...] > > > > > > If I have to remove the IRQ register function, then yes, otherwise it makes more > > > sense to have the pair of functions in the MFD core I would say. > > > > I would prefer you to remove the call-back please. > > Right. > > > > > > > > > > + if (pdata) > > > > > > > + da9150->irq_base = pdata->irq_base; > > > > > > > + else > > > > > > > + da9150->irq_base = -1; > > > > > > > > > > > > pdata ? pdata->irq_base : -1; > > > > > > > > > > This is left this way as later updates to add additional functionality will > > > > > require addtional work to be done with the platform data. Seemed pointless > > > > > changing it here just to change it back later. > > > > > > > > You're not changing anything, as this is the introduction of the code. > > > > I can't tell you how many times I've heard "I will change it later", > > > > or "doing it this way will support subsequent submissions", then > > > > received no more patches. It's okay to do it nicely now and expand > > > > it back out in the new patches. > > > > > > > > [...] > > > > > > It appears that way to you but I have to modify my code for sumbission as the > > > local code I have covers all functionality. Am having to refactor again and > > > again just to suit this initial submission, and then I have to revert it back > > > again when submitting the last couple of drivers. Time consuming, and quite > > > frustrating when the intention was to make the whole process easier. Anyway, > > > will update for now and revert in subsequent patches. > > > > I sincerely hope the refactorings won't add too much effort, but it's > > difficult to have one rule for the masses and different ones for > > others. > > I do understand that, and that's fair enough. Is just frustrating when you're > trying to do a proper job. Anyway, am sure I'll live. :) I know how you feel, as I've been on the receiving end of such rules more than once, but you could have probably re-factored twice in the time it's taken us to have this conversation. :)
diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig index de5abf2..76adb2c 100644 --- a/drivers/mfd/Kconfig +++ b/drivers/mfd/Kconfig @@ -183,6 +183,18 @@ config MFD_DA9063 Additional drivers must be enabled in order to use the functionality of the device. +config MFD_DA9150 + bool "Dialog Semiconductor DA9150 Charger Fuel-Gauge chip" + depends on I2C=y + select MFD_CORE + select REGMAP_I2C + select REGMAP_IRQ + help + This adds support for the DA9150 integrated charger and fuel-gauge + chip. This driver provides common support for accessing the device. + Additional drivers must be enabled in order to use the specific + features of the device. + config MFD_MC13XXX tristate depends on (SPI_MASTER || I2C) diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile index f001487..098dfa1 100644 --- a/drivers/mfd/Makefile +++ b/drivers/mfd/Makefile @@ -114,6 +114,8 @@ obj-$(CONFIG_MFD_DA9055) += da9055.o da9063-objs := da9063-core.o da9063-irq.o da9063-i2c.o obj-$(CONFIG_MFD_DA9063) += da9063.o +obj-$(CONFIG_MFD_DA9150) += da9150-core.o da9150-i2c.o + obj-$(CONFIG_MFD_MAX14577) += max14577.o obj-$(CONFIG_MFD_MAX77686) += max77686.o obj-$(CONFIG_MFD_MAX77693) += max77693.o diff --git a/drivers/mfd/da9150-core.c b/drivers/mfd/da9150-core.c new file mode 100644 index 0000000..029a30b --- /dev/null +++ b/drivers/mfd/da9150-core.c @@ -0,0 +1,332 @@ +/* + * DA9150 Core MFD Driver + * + * Copyright (c) 2014 Dialog Semiconductor + * + * Author: Adam Thomson <Adam.Thomson.Opensource@diasemi.com> + * + * This program is free software; you can redistribute it and/or modify it + * under the terms of the GNU General Public License as published by the + * Free Software Foundation; either version 2 of the License, or (at your + * option) any later version. + */ + +#include <linux/kernel.h> +#include <linux/module.h> +#include <linux/platform_device.h> +#include <linux/slab.h> +#include <linux/irq.h> +#include <linux/interrupt.h> +#include <linux/mfd/core.h> + +#include <linux/mfd/da9150/core.h> +#include <linux/mfd/da9150/registers.h> +#include <linux/mfd/da9150/pdata.h> + +u8 da9150_reg_read(struct da9150 *da9150, u16 reg) +{ + int val, ret; + + ret = regmap_read(da9150->regmap, reg, &val); + if (ret < 0) + dev_err(da9150->dev, "Failed to read from reg 0x%x: %d\n", + reg, ret); + + return (u8) val; +} +EXPORT_SYMBOL_GPL(da9150_reg_read); + +int da9150_reg_write(struct da9150 *da9150, u16 reg, u8 val) +{ + int ret; + + ret = regmap_write(da9150->regmap, reg, val); + if (ret < 0) + dev_err(da9150->dev, "Failed to write to reg 0x%x: %d\n", + reg, ret); + + return ret; +} +EXPORT_SYMBOL_GPL(da9150_reg_write); + +int da9150_set_bits(struct da9150 *da9150, u16 reg, u8 mask, u8 val) +{ + int ret; + + ret = regmap_update_bits(da9150->regmap, reg, mask, val); + if (ret < 0) + dev_err(da9150->dev, "Failed to set bits in reg 0x%x: %d\n", + reg, ret); + + return ret; +} +EXPORT_SYMBOL_GPL(da9150_set_bits); + +int da9150_bulk_read(struct da9150 *da9150, u16 reg, int count, u8 *buf) +{ + int ret; + + ret = regmap_bulk_read(da9150->regmap, reg, buf, count); + if (ret < 0) + dev_err(da9150->dev, "Failed to bulk read from reg 0x%x: %d\n", + reg, ret); + + return ret; +} +EXPORT_SYMBOL_GPL(da9150_bulk_read); + +int da9150_bulk_write(struct da9150 *da9150, u16 reg, int count, const u8 *buf) +{ + int ret; + + ret = regmap_raw_write(da9150->regmap, reg, buf, count); + if (ret < 0) + dev_err(da9150->dev, "Failed to bulk write to reg 0x%x %d\n", + reg, ret); + + return ret; +} +EXPORT_SYMBOL_GPL(da9150_bulk_write); + +static struct regmap_irq da9150_irqs[] = { + [DA9150_IRQ_VBUS] = { + .reg_offset = 0, + .mask = DA9150_E_VBUS_MASK, + }, + [DA9150_IRQ_CHG] = { + .reg_offset = 0, + .mask = DA9150_E_CHG_MASK, + }, + [DA9150_IRQ_TCLASS] = { + .reg_offset = 0, + .mask = DA9150_E_TCLASS_MASK, + }, + [DA9150_IRQ_TJUNC] = { + .reg_offset = 0, + .mask = DA9150_E_TJUNC_MASK, + }, + [DA9150_IRQ_VFAULT] = { + .reg_offset = 0, + .mask = DA9150_E_VFAULT_MASK, + }, + [DA9150_IRQ_CONF] = { + .reg_offset = 1, + .mask = DA9150_E_CONF_MASK, + }, + [DA9150_IRQ_DAT] = { + .reg_offset = 1, + .mask = DA9150_E_DAT_MASK, + }, + [DA9150_IRQ_DTYPE] = { + .reg_offset = 1, + .mask = DA9150_E_DTYPE_MASK, + }, + [DA9150_IRQ_ID] = { + .reg_offset = 1, + .mask = DA9150_E_ID_MASK, + }, + [DA9150_IRQ_ADP] = { + .reg_offset = 1, + .mask = DA9150_E_ADP_MASK, + }, + [DA9150_IRQ_SESS_END] = { + .reg_offset = 1, + .mask = DA9150_E_SESS_END_MASK, + }, + [DA9150_IRQ_SESS_VLD] = { + .reg_offset = 1, + .mask = DA9150_E_SESS_VLD_MASK, + }, + [DA9150_IRQ_FG] = { + .reg_offset = 2, + .mask = DA9150_E_FG_MASK, + }, + [DA9150_IRQ_GP] = { + .reg_offset = 2, + .mask = DA9150_E_GP_MASK, + }, + [DA9150_IRQ_TBAT] = { + .reg_offset = 2, + .mask = DA9150_E_TBAT_MASK, + }, + [DA9150_IRQ_GPIOA] = { + .reg_offset = 2, + .mask = DA9150_E_GPIOA_MASK, + }, + [DA9150_IRQ_GPIOB] = { + .reg_offset = 2, + .mask = DA9150_E_GPIOB_MASK, + }, + [DA9150_IRQ_GPIOC] = { + .reg_offset = 2, + .mask = DA9150_E_GPIOC_MASK, + }, + [DA9150_IRQ_GPIOD] = { + .reg_offset = 2, + .mask = DA9150_E_GPIOD_MASK, + }, + [DA9150_IRQ_GPADC] = { + .reg_offset = 2, + .mask = DA9150_E_GPADC_MASK, + }, + [DA9150_IRQ_WKUP] = { + .reg_offset = 3, + .mask = DA9150_E_WKUP_MASK, + }, +}; + +static struct regmap_irq_chip da9150_regmap_irq_chip = { + .name = "da9150_irq", + .status_base = DA9150_EVENT_E, + .mask_base = DA9150_IRQ_MASK_E, + .ack_base = DA9150_EVENT_E, + .num_regs = DA9150_NUM_IRQ_REGS, + .irqs = da9150_irqs, + .num_irqs = ARRAY_SIZE(da9150_irqs), +}; + +/* Helper functions for sub-devices to request/free IRQs */ +int da9150_register_irq(struct platform_device *pdev, void *dev_id, + irq_handler_t handler, const char *name) +{ + int irq, ret; + + irq = platform_get_irq_byname(pdev, name); + if (irq < 0) + return irq; + + ret = devm_request_threaded_irq(&pdev->dev, irq, NULL, handler, + IRQF_ONESHOT, name, dev_id); + if (ret) + dev_err(&pdev->dev, "Failed to request IRQ %d: %d\n", irq, ret); + + return ret; +} +EXPORT_SYMBOL_GPL(da9150_register_irq); + +void da9150_release_irq(struct platform_device *pdev, void *dev_id, + const char *name) +{ + int irq; + + irq = platform_get_irq_byname(pdev, name); + if (irq < 0) + return; + + devm_free_irq(&pdev->dev, irq, dev_id); +} +EXPORT_SYMBOL_GPL(da9150_release_irq); + +static struct resource da9150_gpadc_resources[] = { + { + .name = "GPADC", + .start = DA9150_IRQ_GPADC, + .end = DA9150_IRQ_GPADC, + .flags = IORESOURCE_IRQ, + }, +}; + +static struct resource da9150_charger_resources[] = { + { + .name = "CHG_STATUS", + .start = DA9150_IRQ_CHG, + .end = DA9150_IRQ_CHG, + .flags = IORESOURCE_IRQ, + }, + { + .name = "CHG_TJUNC", + .start = DA9150_IRQ_TJUNC, + .end = DA9150_IRQ_TJUNC, + .flags = IORESOURCE_IRQ, + }, + { + .name = "CHG_VFAULT", + .start = DA9150_IRQ_VFAULT, + .end = DA9150_IRQ_VFAULT, + .flags = IORESOURCE_IRQ, + }, + { + .name = "CHG_VBUS", + .start = DA9150_IRQ_VBUS, + .end = DA9150_IRQ_VBUS, + .flags = IORESOURCE_IRQ, + }, +}; + +static struct mfd_cell da9150_devs[] = { + { + .name = "da9150-gpadc", + .of_compatible = "dlg,da9150-gpadc", + .resources = da9150_gpadc_resources, + .num_resources = ARRAY_SIZE(da9150_gpadc_resources), + }, + { + .name = "da9150-charger", + .of_compatible = "dlg,da9150-charger", + .resources = da9150_charger_resources, + .num_resources = ARRAY_SIZE(da9150_charger_resources), + }, +}; + +int da9150_device_init(struct da9150 *da9150) +{ + struct da9150_pdata *pdata = da9150->dev->platform_data; + int ret; + + /* Handle platform data */ + if (pdata) + da9150->irq_base = pdata->irq_base; + else + da9150->irq_base = -1; + + /* Init IRQs */ + ret = regmap_add_irq_chip(da9150->regmap, da9150->irq, + IRQF_TRIGGER_LOW | IRQF_ONESHOT, + da9150->irq_base, &da9150_regmap_irq_chip, + &da9150->regmap_irq_data); + if (ret < 0) + goto err_irq; + + da9150->irq_base = regmap_irq_chip_get_base(da9150->regmap_irq_data); + + /* Make the IRQ line a wake source */ + enable_irq_wake(da9150->irq); + + /* Add MFD Devices */ + ret = mfd_add_devices(da9150->dev, -1, da9150_devs, + ARRAY_SIZE(da9150_devs), NULL, + da9150->irq_base, NULL); + if (ret < 0) { + dev_err(da9150->dev, "Failed to add child devices: %d\n", ret); + goto err_mfd; + } + + return 0; + +err_mfd: + regmap_del_irq_chip(da9150->irq, da9150->regmap_irq_data); +err_irq: + return ret; +} + +void da9150_device_exit(struct da9150 *da9150) +{ + regmap_del_irq_chip(da9150->irq, da9150->regmap_irq_data); + mfd_remove_devices(da9150->dev); +} + +void da9150_device_shutdown(struct da9150 *da9150) +{ + /* Make sure we have a wakup source for the device */ + da9150_set_bits(da9150, DA9150_CONFIG_D, + DA9150_WKUP_PM_EN_MASK, + DA9150_WKUP_PM_EN_MASK); + + /* Set device to DISABLED mode */ + da9150_set_bits(da9150, DA9150_CONTROL_C, + DA9150_DISABLE_MASK, DA9150_DISABLE_MASK); +} + +MODULE_DESCRIPTION("MFD Core Driver for DA9150"); +MODULE_AUTHOR("Adam Thomson <Adam.Thomson.Opensource@diasemi.com"); +MODULE_LICENSE("GPL"); diff --git a/drivers/mfd/da9150-i2c.c b/drivers/mfd/da9150-i2c.c new file mode 100644 index 0000000..a02525c --- /dev/null +++ b/drivers/mfd/da9150-i2c.c @@ -0,0 +1,176 @@ +/* + * DA9150 I2C Driver + * + * Copyright (c) 2014 Dialog Semiconductor + * + * Author: Adam Thomson <Adam.Thomson.Opensource@diasemi.com> + * + * This program is free software; you can redistribute it and/or modify it + * under the terms of the GNU General Public License as published by the + * Free Software Foundation; either version 2 of the License, or (at your + * option) any later version. + */ + +#include <linux/kernel.h> +#include <linux/module.h> +#include <linux/platform_device.h> +#include <linux/i2c.h> +#include <linux/regmap.h> +#include <linux/slab.h> + +#include <linux/mfd/da9150/core.h> +#include <linux/mfd/da9150/registers.h> + +static bool da9150_volatile_reg(struct device *dev, unsigned int reg) +{ + switch (reg) { + case DA9150_PAGE_CON: + case DA9150_STATUS_A: + case DA9150_STATUS_B: + case DA9150_STATUS_C: + case DA9150_STATUS_D: + case DA9150_STATUS_E: + case DA9150_STATUS_F: + case DA9150_STATUS_G: + case DA9150_STATUS_H: + case DA9150_STATUS_I: + case DA9150_STATUS_J: + case DA9150_STATUS_K: + case DA9150_STATUS_L: + case DA9150_STATUS_N: + case DA9150_FAULT_LOG_A: + case DA9150_FAULT_LOG_B: + case DA9150_EVENT_E: + case DA9150_EVENT_F: + case DA9150_EVENT_G: + case DA9150_EVENT_H: + case DA9150_CONTROL_B: + case DA9150_CONTROL_C: + case DA9150_GPADC_MAN: + case DA9150_GPADC_RES_A: + case DA9150_GPADC_RES_B: + case DA9150_ADETVB_CFG_C: + case DA9150_ADETD_STAT: + case DA9150_ADET_CMPSTAT: + case DA9150_ADET_CTRL_A: + case DA9150_PPR_TCTR_B: + case DA9150_COREBTLD_STAT_A: + case DA9150_CORE_DATA_A: + case DA9150_CORE_DATA_B: + case DA9150_CORE_DATA_C: + case DA9150_CORE_DATA_D: + case DA9150_CORE2WIRE_STAT_A: + case DA9150_FW_CTRL_C: + case DA9150_FG_CTRL_B: + case DA9150_FW_CTRL_B: + case DA9150_GPADC_CMAN: + case DA9150_GPADC_CRES_A: + case DA9150_GPADC_CRES_B: + case DA9150_CC_ICHG_RES_A: + case DA9150_CC_ICHG_RES_B: + case DA9150_CC_IAVG_RES_A: + case DA9150_CC_IAVG_RES_B: + case DA9150_TAUX_CTRL_A: + case DA9150_TAUX_VALUE_H: + case DA9150_TAUX_VALUE_L: + case DA9150_TBAT_RES_A: + case DA9150_TBAT_RES_B: + return true; + default: + return false; + } +} + +static const struct regmap_range_cfg da9150_range_cfg[] = { + { + .range_min = DA9150_PAGE_CON, + .range_max = DA9150_TBAT_RES_B, + .selector_reg = DA9150_PAGE_CON, + .selector_mask = DA9150_I2C_PAGE_MASK, + .selector_shift = DA9150_I2C_PAGE_SHIFT, + .window_start = 0, + .window_len = 256, + }, +}; + +static struct regmap_config da9150_regmap_config = { + .reg_bits = 8, + .val_bits = 8, + .ranges = da9150_range_cfg, + .num_ranges = ARRAY_SIZE(da9150_range_cfg), + .max_register = DA9150_TBAT_RES_B, + + .cache_type = REGCACHE_RBTREE, + + .volatile_reg = da9150_volatile_reg, +}; + +static int da9150_probe(struct i2c_client *client, + const struct i2c_device_id *id) +{ + struct da9150 *da9150; + int ret; + + da9150 = devm_kzalloc(&client->dev, sizeof(struct da9150), GFP_KERNEL); + if (da9150 == NULL) + return -ENOMEM; + da9150->dev = &client->dev; + da9150->irq = client->irq; + i2c_set_clientdata(client, da9150); + dev_set_drvdata(da9150->dev, da9150); + + da9150->regmap = devm_regmap_init_i2c(client, &da9150_regmap_config); + if (IS_ERR(da9150->regmap)) { + ret = PTR_ERR(da9150->regmap); + dev_err(da9150->dev, "Failed to allocate register map: %d\n", + ret); + return ret; + } + + return da9150_device_init(da9150); +} + +static int da9150_remove(struct i2c_client *client) +{ + struct da9150 *da9150 = i2c_get_clientdata(client); + + da9150_device_exit(da9150); + + return 0; +} + +static void da9150_shutdown(struct i2c_client *client) +{ + struct da9150 *da9150 = i2c_get_clientdata(client); + + da9150_device_shutdown(da9150); +} + +static const struct i2c_device_id da9150_i2c_id[] = { + { "da9150", 0 }, + { } +}; +MODULE_DEVICE_TABLE(i2c, da9150_i2c_id); + +static const struct of_device_id da9150_of_match[] = { + { .compatible = "dlg,da9150", }, + { } +}; + +static struct i2c_driver da9150_driver = { + .driver = { + .name = "da9150", + .owner = THIS_MODULE, + .of_match_table = of_match_ptr(da9150_of_match), + }, + .probe = da9150_probe, + .remove = da9150_remove, + .shutdown = da9150_shutdown, + .id_table = da9150_i2c_id, +}; + +module_i2c_driver(da9150_driver); + +MODULE_DESCRIPTION("I2C Driver for DA9150"); +MODULE_AUTHOR("Adam Thomson <Adam.Thomson.Opensource@diasemi.com"); +MODULE_LICENSE("GPL"); diff --git a/include/linux/mfd/da9150/core.h b/include/linux/mfd/da9150/core.h new file mode 100644 index 0000000..d23c500 --- /dev/null +++ b/include/linux/mfd/da9150/core.h @@ -0,0 +1,80 @@ +/* + * DA9150 MFD Driver - Core Data + * + * Copyright (c) 2014 Dialog Semiconductor + * + * Author: Adam Thomson <Adam.Thomson.Opensource@diasemi.com> + * + * This program is free software; you can redistribute it and/or modify it + * under the terms of the GNU General Public License as published by the + * Free Software Foundation; either version 2 of the License, or (at your + * option) any later version. + */ + +#ifndef __DA9150_CORE_H +#define __DA9150_CORE_H + +#include <linux/device.h> +#include <linux/i2c.h> +#include <linux/interrupt.h> +#include <linux/regmap.h> +#include <linux/mutex.h> + +/* I2C address paging */ +#define DA9150_REG_PAGE_SHIFT 8 +#define DA9150_REG_PAGE_MASK 0xFF + +/* IRQs */ +#define DA9150_NUM_IRQ_REGS 4 +#define DA9150_IRQ_VBUS 0 +#define DA9150_IRQ_CHG 1 +#define DA9150_IRQ_TCLASS 2 +#define DA9150_IRQ_TJUNC 3 +#define DA9150_IRQ_VFAULT 4 +#define DA9150_IRQ_CONF 5 +#define DA9150_IRQ_DAT 6 +#define DA9150_IRQ_DTYPE 7 +#define DA9150_IRQ_ID 8 +#define DA9150_IRQ_ADP 9 +#define DA9150_IRQ_SESS_END 10 +#define DA9150_IRQ_SESS_VLD 11 +#define DA9150_IRQ_FG 12 +#define DA9150_IRQ_GP 13 +#define DA9150_IRQ_TBAT 14 +#define DA9150_IRQ_GPIOA 15 +#define DA9150_IRQ_GPIOB 16 +#define DA9150_IRQ_GPIOC 17 +#define DA9150_IRQ_GPIOD 18 +#define DA9150_IRQ_GPADC 19 +#define DA9150_IRQ_WKUP 20 + +struct da9150 { + struct device *dev; + + struct regmap *regmap; + + struct regmap_irq_chip_data *regmap_irq_data; + int irq; + int irq_base; +}; + +/* Device I/O */ +u8 da9150_reg_read(struct da9150 *da9150, u16 reg); +int da9150_reg_write(struct da9150 *da9150, u16 reg, u8 val); +int da9150_set_bits(struct da9150 *da9150, u16 reg, u8 mask, u8 val); + +int da9150_bulk_read(struct da9150 *da9150, u16 reg, int count, u8 *buf); +int da9150_bulk_write(struct da9150 *da9150, u16 reg, int count, const u8 *buf); + +/* IRQ helper functions */ +int da9150_register_irq(struct platform_device *pdev, void *dev_id, + irq_handler_t handler, const char *name); +void da9150_release_irq(struct platform_device *pdev, void *dev_id, + const char *name); + +/* Init/Exit */ +int da9150_device_init(struct da9150 *da9150); +void da9150_device_exit(struct da9150 *da9150); +void da9150_device_shutdown(struct da9150 *da9150); + +#endif /* __DA9150_CORE_H */ diff --git a/include/linux/mfd/da9150/pdata.h b/include/linux/mfd/da9150/pdata.h new file mode 100644 index 0000000..e2b37f1 --- /dev/null +++ b/include/linux/mfd/da9150/pdata.h @@ -0,0 +1,21 @@ +/* + * DA9150 MFD Driver - Platform Data + * + * Copyright (c) 2014 Dialog Semiconductor + * + * Author: Adam Thomson <Adam.Thomson.Opensource@diasemi.com> + * + * This program is free software; you can redistribute it and/or modify it + * under the terms of the GNU General Public License as published by the + * Free Software Foundation; either version 2 of the License, or (at your + * option) any later version. + */ + +#ifndef __DA9150_PDATA_H +#define __DA9150_PDATA_H + +struct da9150_pdata { + int irq_base; +}; + +#endif /* __DA9150_PDATA_H */ diff --git a/include/linux/mfd/da9150/registers.h b/include/linux/mfd/da9150/registers.h new file mode 100644 index 0000000..ef4826d --- /dev/null +++ b/include/linux/mfd/da9150/registers.h @@ -0,0 +1,1153 @@ +/* + * DA9150 MFD Driver - Registers + * + * Copyright (c) 2014 Dialog Semiconductor + * + * Author: Adam Thomson <Adam.Thomson.Opensource@diasemi.com> + * + * This program is free software; you can redistribute it and/or modify it + * under the terms of the GNU General Public License as published by the + * Free Software Foundation; either version 2 of the License, or (at your + * option) any later version. + */ + +#ifndef __DA9150_REGISTERS_H +#define __DA9150_REGISTERS_H + +/* Registers */ +#define DA9150_PAGE_CON 0x000 +#define DA9150_STATUS_A 0x068 +#define DA9150_STATUS_B 0x069 +#define DA9150_STATUS_C 0x06A +#define DA9150_STATUS_D 0x06B +#define DA9150_STATUS_E 0x06C +#define DA9150_STATUS_F 0x06D +#define DA9150_STATUS_G 0x06E +#define DA9150_STATUS_H 0x06F +#define DA9150_STATUS_I 0x070 +#define DA9150_STATUS_J 0x071 +#define DA9150_STATUS_K 0x072 +#define DA9150_STATUS_L 0x073 +#define DA9150_STATUS_N 0x074 +#define DA9150_FAULT_LOG_A 0x076 +#define DA9150_FAULT_LOG_B 0x077 +#define DA9150_EVENT_E 0x078 +#define DA9150_EVENT_F 0x079 +#define DA9150_EVENT_G 0x07A +#define DA9150_EVENT_H 0x07B +#define DA9150_IRQ_MASK_E 0x07C +#define DA9150_IRQ_MASK_F 0x07D +#define DA9150_IRQ_MASK_G 0x07E +#define DA9150_IRQ_MASK_H 0x07F +#define DA9150_PAGE_CON_1 0x080 +#define DA9150_CONFIG_A 0x0E0 +#define DA9150_CONFIG_B 0x0E1 +#define DA9150_CONFIG_C 0x0E2 +#define DA9150_CONFIG_D 0x0E3 +#define DA9150_CONFIG_E 0x0E4 +#define DA9150_CONTROL_A 0x0E5 +#define DA9150_CONTROL_B 0x0E6 +#define DA9150_CONTROL_C 0x0E7 +#define DA9150_GPIO_A_B 0x0E8 +#define DA9150_GPIO_C_D 0x0E9 +#define DA9150_GPIO_MODE_CONT 0x0EA +#define DA9150_GPIO_CTRL_B 0x0EB +#define DA9150_GPIO_CTRL_A 0x0EC +#define DA9150_GPIO_CTRL_C 0x0ED +#define DA9150_GPIO_CFG_A 0x0EE +#define DA9150_GPIO_CFG_B 0x0EF +#define DA9150_GPIO_CFG_C 0x0F0 +#define DA9150_GPADC_MAN 0x0F2 +#define DA9150_GPADC_RES_A 0x0F4 +#define DA9150_GPADC_RES_B 0x0F5 +#define DA9150_PAGE_CON_2 0x100 +#define DA9150_OTP_CONT_SHARED 0x101 +#define DA9150_INTERFACE_SHARED 0x105 +#define DA9150_CONFIG_A_SHARED 0x106 +#define DA9150_CONFIG_D_SHARED 0x109 +#define DA9150_ADETVB_CFG_C 0x150 +#define DA9150_ADETD_STAT 0x151 +#define DA9150_ADET_CMPSTAT 0x152 +#define DA9150_ADET_CTRL_A 0x153 +#define DA9150_ADETVB_CFG_B 0x154 +#define DA9150_ADETVB_CFG_A 0x155 +#define DA9150_ADETAC_CFG_A 0x156 +#define DA9150_ADDETAC_CFG_B 0x157 +#define DA9150_ADETAC_CFG_C 0x158 +#define DA9150_ADETAC_CFG_D 0x159 +#define DA9150_ADETVB_CFG_D 0x15A +#define DA9150_ADETID_CFG_A 0x15B +#define DA9150_ADET_RID_PT_CHG_H 0x15C +#define DA9150_ADET_RID_PT_CHG_L 0x15D +#define DA9150_PPR_TCTR_B 0x160 +#define DA9150_PPR_BKCTRL_A 0x163 +#define DA9150_PPR_BKCFG_A 0x164 +#define DA9150_PPR_BKCFG_B 0x165 +#define DA9150_PPR_CHGCTRL_A 0x166 +#define DA9150_PPR_CHGCTRL_B 0x167 +#define DA9150_PPR_CHGCTRL_C 0x168 +#define DA9150_PPR_TCTR_A 0x169 +#define DA9150_PPR_CHGCTRL_D 0x16A +#define DA9150_PPR_CHGCTRL_E 0x16B +#define DA9150_PPR_CHGCTRL_F 0x16C +#define DA9150_PPR_CHGCTRL_G 0x16D +#define DA9150_PPR_CHGCTRL_H 0x16E +#define DA9150_PPR_CHGCTRL_I 0x16F +#define DA9150_PPR_CHGCTRL_J 0x170 +#define DA9150_PPR_CHGCTRL_K 0x171 +#define DA9150_PPR_CHGCTRL_L 0x172 +#define DA9150_PPR_CHGCTRL_M 0x173 +#define DA9150_PPR_THYST_A 0x174 +#define DA9150_PPR_THYST_B 0x175 +#define DA9150_PPR_THYST_C 0x176 +#define DA9150_PPR_THYST_D 0x177 +#define DA9150_PPR_THYST_E 0x178 +#define DA9150_PPR_THYST_F 0x179 +#define DA9150_PPR_THYST_G 0x17A +#define DA9150_PAGE_CON_3 0x180 +#define DA9150_PAGE_CON_4 0x200 +#define DA9150_PAGE_CON_5 0x280 +#define DA9150_PAGE_CON_6 0x300 +#define DA9150_COREBTLD_STAT_A 0x302 +#define DA9150_COREBTLD_CTRL_A 0x303 +#define DA9150_CORE_CONFIG_A 0x304 +#define DA9150_CORE_CONFIG_C 0x305 +#define DA9150_CORE_CONFIG_B 0x306 +#define DA9150_CORE_CFG_DATA_A 0x307 +#define DA9150_CORE_CFG_DATA_B 0x308 +#define DA9150_CORE_CMD_A 0x309 +#define DA9150_CORE_DATA_A 0x30A +#define DA9150_CORE_DATA_B 0x30B +#define DA9150_CORE_DATA_C 0x30C +#define DA9150_CORE_DATA_D 0x30D +#define DA9150_CORE2WIRE_STAT_A 0x310 +#define DA9150_CORE2WIRE_CTRL_A 0x311 +#define DA9150_FW_CTRL_A 0x312 +#define DA9150_FW_CTRL_C 0x313 +#define DA9150_FW_CTRL_D 0x314 +#define DA9150_FG_CTRL_A 0x315 +#define DA9150_FG_CTRL_B 0x316 +#define DA9150_FW_CTRL_E 0x317 +#define DA9150_FW_CTRL_B 0x318 +#define DA9150_GPADC_CMAN 0x320 +#define DA9150_GPADC_CRES_A 0x322 +#define DA9150_GPADC_CRES_B 0x323 +#define DA9150_CC_CFG_A 0x328 +#define DA9150_CC_CFG_B 0x329 +#define DA9150_CC_ICHG_RES_A 0x32A +#define DA9150_CC_ICHG_RES_B 0x32B +#define DA9150_CC_IAVG_RES_A 0x32C +#define DA9150_CC_IAVG_RES_B 0x32D +#define DA9150_TAUX_CTRL_A 0x330 +#define DA9150_TAUX_RELOAD_H 0x332 +#define DA9150_TAUX_RELOAD_L 0x333 +#define DA9150_TAUX_VALUE_H 0x334 +#define DA9150_TAUX_VALUE_L 0x335 +#define DA9150_AUX_DATA_0 0x338 +#define DA9150_AUX_DATA_1 0x339 +#define DA9150_AUX_DATA_2 0x33A +#define DA9150_AUX_DATA_3 0x33B +#define DA9150_BIF_CTRL 0x340 +#define DA9150_TBAT_CTRL_A 0x342 +#define DA9150_TBAT_CTRL_B 0x343 +#define DA9150_TBAT_RES_A 0x344 +#define DA9150_TBAT_RES_B 0x345 + +/* DA9150_PAGE_CON = 0x000 */ +#define DA9150_PAGE_SHIFT 0 +#define DA9150_PAGE_MASK (0x3f << 0) +#define DA9150_I2C_PAGE_SHIFT 1 +#define DA9150_I2C_PAGE_MASK (0x1f << 1) +#define DA9150_WRITE_MODE_SHIFT 6 +#define DA9150_WRITE_MODE_MASK (0x01 << 6) +#define DA9150_REVERT_SHIFT 7 +#define DA9150_REVERT_MASK (0x01 << 7) + +/* DA9150_STATUS_A = 0x068 */ +#define DA9150_WKUP_STAT_SHIFT 2 +#define DA9150_WKUP_STAT_MASK (0x0f << 2) +#define DA9150_SLEEP_STAT_SHIFT 6 +#define DA9150_SLEEP_STAT_MASK (0x03 << 6) + +/* DA9150_STATUS_B = 0x069 */ +#define DA9150_VFAULT_STAT_SHIFT 0 +#define DA9150_VFAULT_STAT_MASK (0x01 << 0) +#define DA9150_TFAULT_STAT_SHIFT 1 +#define DA9150_TFAULT_STAT_MASK (0x01 << 1) + +/* DA9150_STATUS_C = 0x06A */ +#define DA9150_VDD33_STAT_SHIFT 0 +#define DA9150_VDD33_STAT_MASK (0x01 << 0) +#define DA9150_VDD33_SLEEP_SHIFT 1 +#define DA9150_VDD33_SLEEP_MASK (0x01 << 1) +#define DA9150_LFOSC_STAT_SHIFT 7 +#define DA9150_LFOSC_STAT_MASK (0x01 << 7) + +/* DA9150_STATUS_D = 0x06B */ +#define DA9150_GPIOA_STAT_SHIFT 0 +#define DA9150_GPIOA_STAT_MASK (0x01 << 0) +#define DA9150_GPIOB_STAT_SHIFT 1 +#define DA9150_GPIOB_STAT_MASK (0x01 << 1) +#define DA9150_GPIOC_STAT_SHIFT 2 +#define DA9150_GPIOC_STAT_MASK (0x01 << 2) +#define DA9150_GPIOD_STAT_SHIFT 3 +#define DA9150_GPIOD_STAT_MASK (0x01 << 3) + +/* DA9150_STATUS_E = 0x06C */ +#define DA9150_DTYPE_SHIFT 0 +#define DA9150_DTYPE_MASK (0x1f << 0) +#define DA9150_DTYPE_DT_NIL (0x00 << 0) +#define DA9150_DTYPE_DT_USB_OTG (0x01 << 0) +#define DA9150_DTYPE_DT_USB_STD (0x02 << 0) +#define DA9150_DTYPE_DT_USB_CHG (0x03 << 0) +#define DA9150_DTYPE_DT_ACA_CHG (0x04 << 0) +#define DA9150_DTYPE_DT_ACA_OTG (0x05 << 0) +#define DA9150_DTYPE_DT_ACA_DOC (0x06 << 0) +#define DA9150_DTYPE_DT_DED_CHG (0x07 << 0) +#define DA9150_DTYPE_DT_CR5_CHG (0x08 << 0) +#define DA9150_DTYPE_DT_CR4_CHG (0x0c << 0) +#define DA9150_DTYPE_DT_PT_CHG (0x11 << 0) +#define DA9150_DTYPE_DT_NN_ACC (0x16 << 0) +#define DA9150_DTYPE_DT_NN_CHG (0x17 << 0) + +/* DA9150_STATUS_F = 0x06D */ +#define DA9150_SESS_VLD_SHIFT 0 +#define DA9150_SESS_VLD_MASK (0x01 << 0) +#define DA9150_ID_ERR_SHIFT 1 +#define DA9150_ID_ERR_MASK (0x01 << 1) +#define DA9150_PT_CHG_SHIFT 2 +#define DA9150_PT_CHG_MASK (0x01 << 2) + +/* DA9150_STATUS_G = 0x06E */ +#define DA9150_RID_SHIFT 0 +#define DA9150_RID_MASK (0xff << 0) + +/* DA9150_STATUS_H = 0x06F */ +#define DA9150_VBUS_STAT_SHIFT 0 +#define DA9150_VBUS_STAT_MASK (0x07 << 0) +#define DA9150_VBUS_STAT_OFF (0x00 << 0) +#define DA9150_VBUS_STAT_WAIT (0x01 << 0) +#define DA9150_VBUS_STAT_CHG (0x02 << 0) +#define DA9150_VBUS_TRED_SHIFT 3 +#define DA9150_VBUS_TRED_MASK (0x01 << 3) +#define DA9150_VBUS_DROP_STAT_SHIFT 4 +#define DA9150_VBUS_DROP_STAT_MASK (0x0f << 4) + +/* DA9150_STATUS_I = 0x070 */ +#define DA9150_VBUS_ISET_STAT_SHIFT 0 +#define DA9150_VBUS_ISET_STAT_MASK (0x1f << 0) +#define DA9150_VBUS_OT_SHIFT 7 +#define DA9150_VBUS_OT_MASK (0x01 << 7) + +/* DA9150_STATUS_J = 0x071 */ +#define DA9150_CHG_STAT_SHIFT 0 +#define DA9150_CHG_STAT_MASK (0x0f << 0) +#define DA9150_CHG_STAT_OFF (0x00 << 0) +#define DA9150_CHG_STAT_SUSP (0x01 << 0) +#define DA9150_CHG_STAT_ACT (0x02 << 0) +#define DA9150_CHG_STAT_PRE (0x03 << 0) +#define DA9150_CHG_STAT_CC (0x04 << 0) +#define DA9150_CHG_STAT_CV (0x05 << 0) +#define DA9150_CHG_STAT_FULL (0x06 << 0) +#define DA9150_CHG_STAT_TEMP (0x07 << 0) +#define DA9150_CHG_STAT_TIME (0x08 << 0) +#define DA9150_CHG_STAT_BAT (0x09 << 0) +#define DA9150_CHG_TEMP_SHIFT 4 +#define DA9150_CHG_TEMP_MASK (0x07 << 4) +#define DA9150_CHG_TEMP_UNDER (0x06 << 4) +#define DA9150_CHG_TEMP_OVER (0x07 << 4) +#define DA9150_CHG_IEND_STAT_SHIFT 7 +#define DA9150_CHG_IEND_STAT_MASK (0x01 << 7) + +/* DA9150_STATUS_K = 0x072 */ +#define DA9150_CHG_IAV_H_SHIFT 0 +#define DA9150_CHG_IAV_H_MASK (0xff << 0) + +/* DA9150_STATUS_L = 0x073 */ +#define DA9150_CHG_IAV_L_SHIFT 5 +#define DA9150_CHG_IAV_L_MASK (0x07 << 5) + +/* DA9150_STATUS_N = 0x074 */ +#define DA9150_CHG_TIME_SHIFT 1 +#define DA9150_CHG_TIME_MASK (0x01 << 1) +#define DA9150_CHG_TRED_SHIFT 2 +#define DA9150_CHG_TRED_MASK (0x01 << 2) +#define DA9150_CHG_TJUNC_CLASS_SHIFT 3 +#define DA9150_CHG_TJUNC_CLASS_MASK (0x07 << 3) +#define DA9150_CHG_TJUNC_CLASS_6 (0x06 << 3) +#define DA9150_EBS_STAT_SHIFT 6 +#define DA9150_EBS_STAT_MASK (0x01 << 6) +#define DA9150_CHG_BAT_REMOVED_SHIFT 7 +#define DA9150_CHG_BAT_REMOVED_MASK (0x01 << 7) + +/* DA9150_FAULT_LOG_A = 0x076 */ +#define DA9150_TEMP_FAULT_SHIFT 0 +#define DA9150_TEMP_FAULT_MASK (0x01 << 0) +#define DA9150_VSYS_FAULT_SHIFT 1 +#define DA9150_VSYS_FAULT_MASK (0x01 << 1) +#define DA9150_START_FAULT_SHIFT 2 +#define DA9150_START_FAULT_MASK (0x01 << 2) +#define DA9150_EXT_FAULT_SHIFT 3 +#define DA9150_EXT_FAULT_MASK (0x01 << 3) +#define DA9150_POR_FAULT_SHIFT 4 +#define DA9150_POR_FAULT_MASK (0x01 << 4) + +/* DA9150_FAULT_LOG_B = 0x077 */ +#define DA9150_VBUS_FAULT_SHIFT 0 +#define DA9150_VBUS_FAULT_MASK (0x01 << 0) +#define DA9150_OTG_FAULT_SHIFT 1 +#define DA9150_OTG_FAULT_MASK (0x01 << 1) + +/* DA9150_EVENT_E = 0x078 */ +#define DA9150_E_VBUS_SHIFT 0 +#define DA9150_E_VBUS_MASK (0x01 << 0) +#define DA9150_E_CHG_SHIFT 1 +#define DA9150_E_CHG_MASK (0x01 << 1) +#define DA9150_E_TCLASS_SHIFT 2 +#define DA9150_E_TCLASS_MASK (0x01 << 2) +#define DA9150_E_TJUNC_SHIFT 3 +#define DA9150_E_TJUNC_MASK (0x01 << 3) +#define DA9150_E_VFAULT_SHIFT 4 +#define DA9150_E_VFAULT_MASK (0x01 << 4) +#define DA9150_EVENTS_H_SHIFT 5 +#define DA9150_EVENTS_H_MASK (0x01 << 5) +#define DA9150_EVENTS_G_SHIFT 6 +#define DA9150_EVENTS_G_MASK (0x01 << 6) +#define DA9150_EVENTS_F_SHIFT 7 +#define DA9150_EVENTS_F_MASK (0x01 << 7) + +/* DA9150_EVENT_F = 0x079 */ +#define DA9150_E_CONF_SHIFT 0 +#define DA9150_E_CONF_MASK (0x01 << 0) +#define DA9150_E_DAT_SHIFT 1 +#define DA9150_E_DAT_MASK (0x01 << 1) +#define DA9150_E_DTYPE_SHIFT 3 +#define DA9150_E_DTYPE_MASK (0x01 << 3) +#define DA9150_E_ID_SHIFT 4 +#define DA9150_E_ID_MASK (0x01 << 4) +#define DA9150_E_ADP_SHIFT 5 +#define DA9150_E_ADP_MASK (0x01 << 5) +#define DA9150_E_SESS_END_SHIFT 6 +#define DA9150_E_SESS_END_MASK (0x01 << 6) +#define DA9150_E_SESS_VLD_SHIFT 7 +#define DA9150_E_SESS_VLD_MASK (0x01 << 7) + +/* DA9150_EVENT_G = 0x07A */ +#define DA9150_E_FG_SHIFT 0 +#define DA9150_E_FG_MASK (0x01 << 0) +#define DA9150_E_GP_SHIFT 1 +#define DA9150_E_GP_MASK (0x01 << 1) +#define DA9150_E_TBAT_SHIFT 2 +#define DA9150_E_TBAT_MASK (0x01 << 2) +#define DA9150_E_GPIOA_SHIFT 3 +#define DA9150_E_GPIOA_MASK (0x01 << 3) +#define DA9150_E_GPIOB_SHIFT 4 +#define DA9150_E_GPIOB_MASK (0x01 << 4) +#define DA9150_E_GPIOC_SHIFT 5 +#define DA9150_E_GPIOC_MASK (0x01 << 5) +#define DA9150_E_GPIOD_SHIFT 6 +#define DA9150_E_GPIOD_MASK (0x01 << 6) +#define DA9150_E_GPADC_SHIFT 7 +#define DA9150_E_GPADC_MASK (0x01 << 7) + +/* DA9150_EVENT_H = 0x07B */ +#define DA9150_E_WKUP_SHIFT 0 +#define DA9150_E_WKUP_MASK (0x01 << 0) + +/* DA9150_IRQ_MASK_E = 0x07C */ +#define DA9150_M_VBUS_SHIFT 0 +#define DA9150_M_VBUS_MASK (0x01 << 0) +#define DA9150_M_CHG_SHIFT 1 +#define DA9150_M_CHG_MASK (0x01 << 1) +#define DA9150_M_TJUNC_SHIFT 3 +#define DA9150_M_TJUNC_MASK (0x01 << 3) +#define DA9150_M_VFAULT_SHIFT 4 +#define DA9150_M_VFAULT_MASK (0x01 << 4) + +/* DA9150_IRQ_MASK_F = 0x07D */ +#define DA9150_M_CONF_SHIFT 0 +#define DA9150_M_CONF_MASK (0x01 << 0) +#define DA9150_M_DAT_SHIFT 1 +#define DA9150_M_DAT_MASK (0x01 << 1) +#define DA9150_M_DTYPE_SHIFT 3 +#define DA9150_M_DTYPE_MASK (0x01 << 3) +#define DA9150_M_ID_SHIFT 4 +#define DA9150_M_ID_MASK (0x01 << 4) +#define DA9150_M_ADP_SHIFT 5 +#define DA9150_M_ADP_MASK (0x01 << 5) +#define DA9150_M_SESS_END_SHIFT 6 +#define DA9150_M_SESS_END_MASK (0x01 << 6) +#define DA9150_M_SESS_VLD_SHIFT 7 +#define DA9150_M_SESS_VLD_MASK (0x01 << 7) + +/* DA9150_IRQ_MASK_G = 0x07E */ +#define DA9150_M_FG_SHIFT 0 +#define DA9150_M_FG_MASK (0x01 << 0) +#define DA9150_M_GP_SHIFT 1 +#define DA9150_M_GP_MASK (0x01 << 1) +#define DA9150_M_TBAT_SHIFT 2 +#define DA9150_M_TBAT_MASK (0x01 << 2) +#define DA9150_M_GPIOA_SHIFT 3 +#define DA9150_M_GPIOA_MASK (0x01 << 3) +#define DA9150_M_GPIOB_SHIFT 4 +#define DA9150_M_GPIOB_MASK (0x01 << 4) +#define DA9150_M_GPIOC_SHIFT 5 +#define DA9150_M_GPIOC_MASK (0x01 << 5) +#define DA9150_M_GPIOD_SHIFT 6 +#define DA9150_M_GPIOD_MASK (0x01 << 6) +#define DA9150_M_GPADC_SHIFT 7 +#define DA9150_M_GPADC_MASK (0x01 << 7) + +/* DA9150_IRQ_MASK_H = 0x07F */ +#define DA9150_M_WKUP_SHIFT 0 +#define DA9150_M_WKUP_MASK (0x01 << 0) + +/* DA9150_PAGE_CON_1 = 0x080 */ +#define DA9150_PAGE_SHIFT 0 +#define DA9150_PAGE_MASK (0x3f << 0) +#define DA9150_WRITE_MODE_SHIFT 6 +#define DA9150_WRITE_MODE_MASK (0x01 << 6) +#define DA9150_REVERT_SHIFT 7 +#define DA9150_REVERT_MASK (0x01 << 7) + +/* DA9150_CONFIG_A = 0x0E0 */ +#define DA9150_RESET_DUR_SHIFT 0 +#define DA9150_RESET_DUR_MASK (0x03 << 0) +#define DA9150_RESET_EXT_SHIFT 2 +#define DA9150_RESET_EXT_MASK (0x03 << 2) +#define DA9150_START_MAX_SHIFT 4 +#define DA9150_START_MAX_MASK (0x03 << 4) +#define DA9150_PS_WAIT_EN_SHIFT 6 +#define DA9150_PS_WAIT_EN_MASK (0x01 << 6) +#define DA9150_PS_DISABLE_DIRECT_SHIFT 7 +#define DA9150_PS_DISABLE_DIRECT_MASK (0x01 << 7) + +/* DA9150_CONFIG_B = 0x0E1 */ +#define DA9150_VFAULT_ADJ_SHIFT 0 +#define DA9150_VFAULT_ADJ_MASK (0x0f << 0) +#define DA9150_VFAULT_HYST_SHIFT 4 +#define DA9150_VFAULT_HYST_MASK (0x07 << 4) +#define DA9150_VFAULT_EN_SHIFT 7 +#define DA9150_VFAULT_EN_MASK (0x01 << 7) + +/* DA9150_CONFIG_C = 0x0E2 */ +#define DA9150_VSYS_MIN_SHIFT 3 +#define DA9150_VSYS_MIN_MASK (0x1f << 3) + +/* DA9150_CONFIG_D = 0x0E3 */ +#define DA9150_LFOSC_EXT_SHIFT 0 +#define DA9150_LFOSC_EXT_MASK (0x01 << 0) +#define DA9150_VDD33_DWN_SHIFT 1 +#define DA9150_VDD33_DWN_MASK (0x01 << 1) +#define DA9150_WKUP_PM_EN_SHIFT 2 +#define DA9150_WKUP_PM_EN_MASK (0x01 << 2) +#define DA9150_WKUP_CE_SEL_SHIFT 3 +#define DA9150_WKUP_CE_SEL_MASK (0x03 << 3) +#define DA9150_WKUP_CLK32K_EN_SHIFT 5 +#define DA9150_WKUP_CLK32K_EN_MASK (0x01 << 5) +#define DA9150_DISABLE_DEL_SHIFT 7 +#define DA9150_DISABLE_DEL_MASK (0x01 << 7) + +/* DA9150_CONFIG_E = 0x0E4 */ +#define DA9150_PM_SPKSUP_DIS_SHIFT 0 +#define DA9150_PM_SPKSUP_DIS_MASK (0x01 << 0) +#define DA9150_PM_MERGE_SHIFT 1 +#define DA9150_PM_MERGE_MASK (0x01 << 1) +#define DA9150_PM_SR_OFF_SHIFT 2 +#define DA9150_PM_SR_OFF_MASK (0x01 << 2) +#define DA9150_PM_TIMEOUT_EN_SHIFT 3 +#define DA9150_PM_TIMEOUT_EN_MASK (0x01 << 3) +#define DA9150_PM_DLY_SEL_SHIFT 4 +#define DA9150_PM_DLY_SEL_MASK (0x07 << 4) +#define DA9150_PM_OUT_DLY_SEL_SHIFT 7 +#define DA9150_PM_OUT_DLY_SEL_MASK (0x01 << 7) + +/* DA9150_CONTROL_A = 0x0E5 */ +#define DA9150_VDD33_SL_SHIFT 0 +#define DA9150_VDD33_SL_MASK (0x01 << 0) +#define DA9150_VDD33_LPM_SHIFT 1 +#define DA9150_VDD33_LPM_MASK (0x03 << 1) +#define DA9150_VDD33_EN_SHIFT 3 +#define DA9150_VDD33_EN_MASK (0x01 << 3) +#define DA9150_GPI_LPM_SHIFT 6 +#define DA9150_GPI_LPM_MASK (0x01 << 6) +#define DA9150_PM_IF_LPM_SHIFT 7 +#define DA9150_PM_IF_LPM_MASK (0x01 << 7) + +/* DA9150_CONTROL_B = 0x0E6 */ +#define DA9150_LPM_SHIFT 0 +#define DA9150_LPM_MASK (0x01 << 0) +#define DA9150_RESET_SHIFT 1 +#define DA9150_RESET_MASK (0x01 << 1) +#define DA9150_RESET_USRCONF_EN_SHIFT 2 +#define DA9150_RESET_USRCONF_EN_MASK (0x01 << 2) + +/* DA9150_CONTROL_C = 0x0E7 */ +#define DA9150_DISABLE_SHIFT 0 +#define DA9150_DISABLE_MASK (0x01 << 0) + +/* DA9150_GPIO_A_B = 0x0E8 */ +#define DA9150_GPIOA_PIN_SHIFT 0 +#define DA9150_GPIOA_PIN_MASK (0x07 << 0) +#define DA9150_GPIOA_PIN_GPI (0x00 << 0) +#define DA9150_GPIOA_PIN_GPO_OD (0x01 << 0) +#define DA9150_GPIOA_TYPE_SHIFT 3 +#define DA9150_GPIOA_TYPE_MASK (0x01 << 3) +#define DA9150_GPIOB_PIN_SHIFT 4 +#define DA9150_GPIOB_PIN_MASK (0x07 << 4) +#define DA9150_GPIOB_PIN_GPI (0x00 << 4) +#define DA9150_GPIOB_PIN_GPO_OD (0x01 << 4) +#define DA9150_GPIOB_TYPE_SHIFT 7 +#define DA9150_GPIOB_TYPE_MASK (0x01 << 7) + +/* DA9150_GPIO_C_D = 0x0E9 */ +#define DA9150_GPIOC_PIN_SHIFT 0 +#define DA9150_GPIOC_PIN_MASK (0x07 << 0) +#define DA9150_GPIOC_PIN_GPI (0x00 << 0) +#define DA9150_GPIOC_PIN_GPO_OD (0x01 << 0) +#define DA9150_GPIOC_TYPE_SHIFT 3 +#define DA9150_GPIOC_TYPE_MASK (0x01 << 3) +#define DA9150_GPIOD_PIN_SHIFT 4 +#define DA9150_GPIOD_PIN_MASK (0x07 << 4) +#define DA9150_GPIOD_PIN_GPI (0x00 << 4) +#define DA9150_GPIOD_PIN_GPO_OD (0x01 << 4) +#define DA9150_GPIOD_TYPE_SHIFT 7 +#define DA9150_GPIOD_TYPE_MASK (0x01 << 7) + +/* DA9150_GPIO_MODE_CONT = 0x0EA */ +#define DA9150_GPIOA_MODE_SHIFT 0 +#define DA9150_GPIOA_MODE_MASK (0x01 << 0) +#define DA9150_GPIOB_MODE_SHIFT 1 +#define DA9150_GPIOB_MODE_MASK (0x01 << 1) +#define DA9150_GPIOC_MODE_SHIFT 2 +#define DA9150_GPIOC_MODE_MASK (0x01 << 2) +#define DA9150_GPIOD_MODE_SHIFT 3 +#define DA9150_GPIOD_MODE_MASK (0x01 << 3) +#define DA9150_GPIOA_CONT_SHIFT 4 +#define DA9150_GPIOA_CONT_MASK (0x01 << 4) +#define DA9150_GPIOB_CONT_SHIFT 5 +#define DA9150_GPIOB_CONT_MASK (0x01 << 5) +#define DA9150_GPIOC_CONT_SHIFT 6 +#define DA9150_GPIOC_CONT_MASK (0x01 << 6) +#define DA9150_GPIOD_CONT_SHIFT 7 +#define DA9150_GPIOD_CONT_MASK (0x01 << 7) + +/* DA9150_GPIO_CTRL_B = 0x0EB */ +#define DA9150_WAKE_PIN_SHIFT 0 +#define DA9150_WAKE_PIN_MASK (0x03 << 0) +#define DA9150_WAKE_MODE_SHIFT 2 +#define DA9150_WAKE_MODE_MASK (0x01 << 2) +#define DA9150_WAKE_CONT_SHIFT 3 +#define DA9150_WAKE_CONT_MASK (0x01 << 3) +#define DA9150_WAKE_DLY_SHIFT 4 +#define DA9150_WAKE_DLY_MASK (0x01 << 4) + +/* DA9150_GPIO_CTRL_A = 0x0EC */ +#define DA9150_GPIOA_ANAEN_SHIFT 0 +#define DA9150_GPIOA_ANAEN_MASK (0x01 << 0) +#define DA9150_GPIOB_ANAEN_SHIFT 1 +#define DA9150_GPIOB_ANAEN_MASK (0x01 << 1) +#define DA9150_GPIOC_ANAEN_SHIFT 2 +#define DA9150_GPIOC_ANAEN_MASK (0x01 << 2) +#define DA9150_GPIOD_ANAEN_SHIFT 3 +#define DA9150_GPIOD_ANAEN_MASK (0x01 << 3) +#define DA9150_GPIO_ANAEN 0x01 +#define DA9150_GPIO_ANAEN_MASK 0x0F +#define DA9150_CHGLED_PIN_SHIFT 5 +#define DA9150_CHGLED_PIN_MASK (0x07 << 5) + +/* DA9150_GPIO_CTRL_C = 0x0ED */ +#define DA9150_CHGBL_DUR_SHIFT 0 +#define DA9150_CHGBL_DUR_MASK (0x03 << 0) +#define DA9150_CHGBL_DBL_SHIFT 2 +#define DA9150_CHGBL_DBL_MASK (0x01 << 2) +#define DA9150_CHGBL_FRQ_SHIFT 3 +#define DA9150_CHGBL_FRQ_MASK (0x03 << 3) +#define DA9150_CHGBL_FLKR_SHIFT 5 +#define DA9150_CHGBL_FLKR_MASK (0x01 << 5) + +/* DA9150_GPIO_CFG_A = 0x0EE */ +#define DA9150_CE_LPM_DEB_SHIFT 0 +#define DA9150_CE_LPM_DEB_MASK (0x07 << 0) + +/* DA9150_GPIO_CFG_B = 0x0EF */ +#define DA9150_GPIOA_PUPD_SHIFT 0 +#define DA9150_GPIOA_PUPD_MASK (0x01 << 0) +#define DA9150_GPIOB_PUPD_SHIFT 1 +#define DA9150_GPIOB_PUPD_MASK (0x01 << 1) +#define DA9150_GPIOC_PUPD_SHIFT 2 +#define DA9150_GPIOC_PUPD_MASK (0x01 << 2) +#define DA9150_GPIOD_PUPD_SHIFT 3 +#define DA9150_GPIOD_PUPD_MASK (0x01 << 3) +#define DA9150_GPIO_PUPD_MASK (0xF << 0) +#define DA9150_GPI_DEB_SHIFT 4 +#define DA9150_GPI_DEB_MASK (0x07 << 4) +#define DA9150_LPM_EN_SHIFT 7 +#define DA9150_LPM_EN_MASK (0x01 << 7) + +/* DA9150_GPIO_CFG_C = 0x0F0 */ +#define DA9150_GPI_V_SHIFT 0 +#define DA9150_GPI_V_MASK (0x01 << 0) +#define DA9150_VDDIO_INT_SHIFT 1 +#define DA9150_VDDIO_INT_MASK (0x01 << 1) +#define DA9150_FAULT_PIN_SHIFT 3 +#define DA9150_FAULT_PIN_MASK (0x07 << 3) +#define DA9150_FAULT_TYPE_SHIFT 6 +#define DA9150_FAULT_TYPE_MASK (0x01 << 6) +#define DA9150_NIRQ_PUPD_SHIFT 7 +#define DA9150_NIRQ_PUPD_MASK (0x01 << 7) + +/* DA9150_GPADC_MAN = 0x0F2 */ +#define DA9150_GPADC_EN_SHIFT 0 +#define DA9150_GPADC_EN_MASK (0x01 << 0) +#define DA9150_GPADC_MUX_SHIFT 1 +#define DA9150_GPADC_MUX_MASK (0x1f << 1) + +/* DA9150_GPADC_RES_A = 0x0F4 */ +#define DA9150_GPADC_RES_H_SHIFT 0 +#define DA9150_GPADC_RES_H_MASK (0xff << 0) + +/* DA9150_GPADC_RES_B = 0x0F5 */ +#define DA9150_GPADC_RUN_SHIFT 0 +#define DA9150_GPADC_RUN_MASK (0x01 << 0) +#define DA9150_GPADC_RES_L_SHIFT 6 +#define DA9150_GPADC_RES_L_MASK (0x03 << 6) +#define DA9150_GPADC_RES_L_BITS 2 + +/* DA9150_PAGE_CON_2 = 0x100 */ +#define DA9150_PAGE_SHIFT 0 +#define DA9150_PAGE_MASK (0x3f << 0) +#define DA9150_WRITE_MODE_SHIFT 6 +#define DA9150_WRITE_MODE_MASK (0x01 << 6) +#define DA9150_REVERT_SHIFT 7 +#define DA9150_REVERT_MASK (0x01 << 7) + +/* DA9150_OTP_CONT_SHARED = 0x101 */ +#define DA9150_PC_DONE_SHIFT 3 +#define DA9150_PC_DONE_MASK (0x01 << 3) + +/* DA9150_INTERFACE_SHARED = 0x105 */ +#define DA9150_IF_BASE_ADDR_SHIFT 4 +#define DA9150_IF_BASE_ADDR_MASK (0x0f << 4) + +/* DA9150_CONFIG_A_SHARED = 0x106 */ +#define DA9150_NIRQ_VDD_SHIFT 1 +#define DA9150_NIRQ_VDD_MASK (0x01 << 1) +#define DA9150_NIRQ_PIN_SHIFT 2 +#define DA9150_NIRQ_PIN_MASK (0x01 << 2) +#define DA9150_NIRQ_TYPE_SHIFT 3 +#define DA9150_NIRQ_TYPE_MASK (0x01 << 3) +#define DA9150_PM_IF_V_SHIFT 4 +#define DA9150_PM_IF_V_MASK (0x01 << 4) +#define DA9150_PM_IF_FMP_SHIFT 5 +#define DA9150_PM_IF_FMP_MASK (0x01 << 5) +#define DA9150_PM_IF_HSM_SHIFT 6 +#define DA9150_PM_IF_HSM_MASK (0x01 << 6) + +/* DA9150_CONFIG_D_SHARED = 0x109 */ +#define DA9150_NIRQ_MODE_SHIFT 1 +#define DA9150_NIRQ_MODE_MASK (0x01 << 1) + +/* DA9150_ADETVB_CFG_C = 0x150 */ +#define DA9150_TADP_RISE_SHIFT 0 +#define DA9150_TADP_RISE_MASK (0xff << 0) + +/* DA9150_ADETD_STAT = 0x151 */ +#define DA9150_DCD_STAT_SHIFT 0 +#define DA9150_DCD_STAT_MASK (0x01 << 0) +#define DA9150_PCD_STAT_SHIFT 1 +#define DA9150_PCD_STAT_MASK (0x03 << 1) +#define DA9150_SCD_STAT_SHIFT 3 +#define DA9150_SCD_STAT_MASK (0x03 << 3) +#define DA9150_DP_STAT_SHIFT 5 +#define DA9150_DP_STAT_MASK (0x01 << 5) +#define DA9150_DM_STAT_SHIFT 6 +#define DA9150_DM_STAT_MASK (0x01 << 6) + +/* DA9150_ADET_CMPSTAT = 0x152 */ +#define DA9150_DP_COMP_SHIFT 1 +#define DA9150_DP_COMP_MASK (0x01 << 1) +#define DA9150_DM_COMP_SHIFT 2 +#define DA9150_DM_COMP_MASK (0x01 << 2) +#define DA9150_ADP_SNS_COMP_SHIFT 3 +#define DA9150_ADP_SNS_COMP_MASK (0x01 << 3) +#define DA9150_ADP_PRB_COMP_SHIFT 4 +#define DA9150_ADP_PRB_COMP_MASK (0x01 << 4) +#define DA9150_ID_COMP_SHIFT 5 +#define DA9150_ID_COMP_MASK (0x01 << 5) + +/* DA9150_ADET_CTRL_A = 0x153 */ +#define DA9150_AID_DAT_SHIFT 0 +#define DA9150_AID_DAT_MASK (0x01 << 0) +#define DA9150_AID_ID_SHIFT 1 +#define DA9150_AID_ID_MASK (0x01 << 1) +#define DA9150_AID_TRIG_SHIFT 2 +#define DA9150_AID_TRIG_MASK (0x01 << 2) + +/* DA9150_ADETVB_CFG_B = 0x154 */ +#define DA9150_VB_MODE_SHIFT 0 +#define DA9150_VB_MODE_MASK (0x03 << 0) +#define DA9150_VB_MODE_VB_SESS (0x01 << 0) + +#define DA9150_TADP_PRB_SHIFT 2 +#define DA9150_TADP_PRB_MASK (0x01 << 2) +#define DA9150_DAT_RPD_EXT_SHIFT 5 +#define DA9150_DAT_RPD_EXT_MASK (0x01 << 5) +#define DA9150_CONF_RPD_SHIFT 6 +#define DA9150_CONF_RPD_MASK (0x01 << 6) +#define DA9150_CONF_SRP_SHIFT 7 +#define DA9150_CONF_SRP_MASK (0x01 << 7) + +/* DA9150_ADETVB_CFG_A = 0x155 */ +#define DA9150_AID_MODE_SHIFT 0 +#define DA9150_AID_MODE_MASK (0x03 << 0) +#define DA9150_AID_EXT_POL_SHIFT 2 +#define DA9150_AID_EXT_POL_MASK (0x01 << 2) + +/* DA9150_ADETAC_CFG_A = 0x156 */ +#define DA9150_ISET_CDP_SHIFT 0 +#define DA9150_ISET_CDP_MASK (0x1f << 0) +#define DA9150_CONF_DBP_SHIFT 5 +#define DA9150_CONF_DBP_MASK (0x01 << 5) + +/* DA9150_ADDETAC_CFG_B = 0x157 */ +#define DA9150_ISET_DCHG_SHIFT 0 +#define DA9150_ISET_DCHG_MASK (0x1f << 0) +#define DA9150_CONF_GPIOA_SHIFT 5 +#define DA9150_CONF_GPIOA_MASK (0x01 << 5) +#define DA9150_CONF_GPIOB_SHIFT 6 +#define DA9150_CONF_GPIOB_MASK (0x01 << 6) +#define DA9150_AID_VB_SHIFT 7 +#define DA9150_AID_VB_MASK (0x01 << 7) + +/* DA9150_ADETAC_CFG_C = 0x158 */ +#define DA9150_ISET_DEF_SHIFT 0 +#define DA9150_ISET_DEF_MASK (0x1f << 0) +#define DA9150_CONF_MODE_SHIFT 5 +#define DA9150_CONF_MODE_MASK (0x03 << 5) +#define DA9150_AID_CR_DIS_SHIFT 7 +#define DA9150_AID_CR_DIS_MASK (0x01 << 7) + +/* DA9150_ADETAC_CFG_D = 0x159 */ +#define DA9150_ISET_UNIT_SHIFT 0 +#define DA9150_ISET_UNIT_MASK (0x1f << 0) +#define DA9150_AID_UNCLAMP_SHIFT 5 +#define DA9150_AID_UNCLAMP_MASK (0x01 << 5) + +/* DA9150_ADETVB_CFG_D = 0x15A */ +#define DA9150_ID_MODE_SHIFT 0 +#define DA9150_ID_MODE_MASK (0x03 << 0) +#define DA9150_DAT_MODE_SHIFT 2 +#define DA9150_DAT_MODE_MASK (0x0f << 2) +#define DA9150_DAT_SWP_SHIFT 6 +#define DA9150_DAT_SWP_MASK (0x01 << 6) +#define DA9150_DAT_CLAMP_EXT_SHIFT 7 +#define DA9150_DAT_CLAMP_EXT_MASK (0x01 << 7) + +/* DA9150_ADETID_CFG_A = 0x15B */ +#define DA9150_TID_POLL_SHIFT 0 +#define DA9150_TID_POLL_MASK (0x07 << 0) +#define DA9150_RID_CONV_SHIFT 3 +#define DA9150_RID_CONV_MASK (0x01 << 3) + +/* DA9150_ADET_RID_PT_CHG_H = 0x15C */ +#define DA9150_RID_PT_CHG_H_SHIFT 0 +#define DA9150_RID_PT_CHG_H_MASK (0xff << 0) + +/* DA9150_ADET_RID_PT_CHG_L = 0x15D */ +#define DA9150_RID_PT_CHG_L_SHIFT 6 +#define DA9150_RID_PT_CHG_L_MASK (0x03 << 6) + +/* DA9150_PPR_TCTR_B = 0x160 */ +#define DA9150_CHG_TCTR_VAL_SHIFT 0 +#define DA9150_CHG_TCTR_VAL_MASK (0xff << 0) + +/* DA9150_PPR_BKCTRL_A = 0x163 */ +#define DA9150_VBUS_MODE_SHIFT 0 +#define DA9150_VBUS_MODE_MASK (0x03 << 0) +#define DA9150_VBUS_MODE_CHG (0x01 << 0) +#define DA9150_VBUS_MODE_OTG (0x02 << 0) +#define DA9150_VBUS_LPM_SHIFT 2 +#define DA9150_VBUS_LPM_MASK (0x03 << 2) +#define DA9150_VBUS_SUSP_SHIFT 4 +#define DA9150_VBUS_SUSP_MASK (0x01 << 4) +#define DA9150_VBUS_PWM_SHIFT 5 +#define DA9150_VBUS_PWM_MASK (0x01 << 5) +#define DA9150_VBUS_ISO_SHIFT 6 +#define DA9150_VBUS_ISO_MASK (0x01 << 6) +#define DA9150_VBUS_LDO_SHIFT 7 +#define DA9150_VBUS_LDO_MASK (0x01 << 7) + +/* DA9150_PPR_BKCFG_A = 0x164 */ +#define DA9150_VBUS_ISET_SHIFT 0 +#define DA9150_VBUS_ISET_MASK (0x1f << 0) +#define DA9150_VBUS_IMAX_SHIFT 5 +#define DA9150_VBUS_IMAX_MASK (0x01 << 5) +#define DA9150_VBUS_IOTG_SHIFT 6 +#define DA9150_VBUS_IOTG_MASK (0x03 << 6) + +/* DA9150_PPR_BKCFG_B = 0x165 */ +#define DA9150_VBUS_DROP_SHIFT 0 +#define DA9150_VBUS_DROP_MASK (0x0f << 0) +#define DA9150_VBUS_FAULT_DIS_SHIFT 6 +#define DA9150_VBUS_FAULT_DIS_MASK (0x01 << 6) +#define DA9150_OTG_FAULT_DIS_SHIFT 7 +#define DA9150_OTG_FAULT_DIS_MASK (0x01 << 7) + +/* DA9150_PPR_CHGCTRL_A = 0x166 */ +#define DA9150_CHG_EN_SHIFT 0 +#define DA9150_CHG_EN_MASK (0x01 << 0) + +/* DA9150_PPR_CHGCTRL_B = 0x167 */ +#define DA9150_CHG_VBAT_SHIFT 0 +#define DA9150_CHG_VBAT_MASK (0x1f << 0) +#define DA9150_CHG_VDROP_SHIFT 6 +#define DA9150_CHG_VDROP_MASK (0x03 << 6) + +/* DA9150_PPR_CHGCTRL_C = 0x168 */ +#define DA9150_CHG_VFAULT_SHIFT 0 +#define DA9150_CHG_VFAULT_MASK (0x0f << 0) +#define DA9150_CHG_IPRE_SHIFT 4 +#define DA9150_CHG_IPRE_MASK (0x03 << 4) + +/* DA9150_PPR_TCTR_A = 0x169 */ +#define DA9150_CHG_TCTR_SHIFT 0 +#define DA9150_CHG_TCTR_MASK (0x07 << 0) +#define DA9150_CHG_TCTR_MODE_SHIFT 4 +#define DA9150_CHG_TCTR_MODE_MASK (0x01 << 4) + +/* DA9150_PPR_CHGCTRL_D = 0x16A */ +#define DA9150_CHG_IBAT_SHIFT 0 +#define DA9150_CHG_IBAT_MASK (0xff << 0) + +/* DA9150_PPR_CHGCTRL_E = 0x16B */ +#define DA9150_CHG_IEND_SHIFT 0 +#define DA9150_CHG_IEND_MASK (0xff << 0) + +/* DA9150_PPR_CHGCTRL_F = 0x16C */ +#define DA9150_CHG_VCOLD_SHIFT 0 +#define DA9150_CHG_VCOLD_MASK (0x1f << 0) +#define DA9150_TBAT_TQA_EN_SHIFT 6 +#define DA9150_TBAT_TQA_EN_MASK (0x01 << 6) +#define DA9150_TBAT_TDP_EN_SHIFT 7 +#define DA9150_TBAT_TDP_EN_MASK (0x01 << 7) + +/* DA9150_PPR_CHGCTRL_G = 0x16D */ +#define DA9150_CHG_VWARM_SHIFT 0 +#define DA9150_CHG_VWARM_MASK (0x1f << 0) + +/* DA9150_PPR_CHGCTRL_H = 0x16E */ +#define DA9150_CHG_VHOT_SHIFT 0 +#define DA9150_CHG_VHOT_MASK (0x1f << 0) + +/* DA9150_PPR_CHGCTRL_I = 0x16F */ +#define DA9150_CHG_ICOLD_SHIFT 0 +#define DA9150_CHG_ICOLD_MASK (0xff << 0) + +/* DA9150_PPR_CHGCTRL_J = 0x170 */ +#define DA9150_CHG_IWARM_SHIFT 0 +#define DA9150_CHG_IWARM_MASK (0xff << 0) + +/* DA9150_PPR_CHGCTRL_K = 0x171 */ +#define DA9150_CHG_IHOT_SHIFT 0 +#define DA9150_CHG_IHOT_MASK (0xff << 0) + +/* DA9150_PPR_CHGCTRL_L = 0x172 */ +#define DA9150_CHG_IBAT_TRED_SHIFT 0 +#define DA9150_CHG_IBAT_TRED_MASK (0xff << 0) + +/* DA9150_PPR_CHGCTRL_M = 0x173 */ +#define DA9150_CHG_VFLOAT_SHIFT 0 +#define DA9150_CHG_VFLOAT_MASK (0x0f << 0) +#define DA9150_CHG_LPM_SHIFT 5 +#define DA9150_CHG_LPM_MASK (0x01 << 5) +#define DA9150_CHG_NBLO_SHIFT 6 +#define DA9150_CHG_NBLO_MASK (0x01 << 6) +#define DA9150_EBS_EN_SHIFT 7 +#define DA9150_EBS_EN_MASK (0x01 << 7) + +/* DA9150_PPR_THYST_A = 0x174 */ +#define DA9150_TBAT_T1_SHIFT 0 +#define DA9150_TBAT_T1_MASK (0xff << 0) + +/* DA9150_PPR_THYST_B = 0x175 */ +#define DA9150_TBAT_T2_SHIFT 0 +#define DA9150_TBAT_T2_MASK (0xff << 0) + +/* DA9150_PPR_THYST_C = 0x176 */ +#define DA9150_TBAT_T3_SHIFT 0 +#define DA9150_TBAT_T3_MASK (0xff << 0) + +/* DA9150_PPR_THYST_D = 0x177 */ +#define DA9150_TBAT_T4_SHIFT 0 +#define DA9150_TBAT_T4_MASK (0xff << 0) + +/* DA9150_PPR_THYST_E = 0x178 */ +#define DA9150_TBAT_T5_SHIFT 0 +#define DA9150_TBAT_T5_MASK (0xff << 0) + +/* DA9150_PPR_THYST_F = 0x179 */ +#define DA9150_TBAT_H1_SHIFT 0 +#define DA9150_TBAT_H1_MASK (0xff << 0) + +/* DA9150_PPR_THYST_G = 0x17A */ +#define DA9150_TBAT_H5_SHIFT 0 +#define DA9150_TBAT_H5_MASK (0xff << 0) + +/* DA9150_PAGE_CON_3 = 0x180 */ +#define DA9150_PAGE_SHIFT 0 +#define DA9150_PAGE_MASK (0x3f << 0) +#define DA9150_WRITE_MODE_SHIFT 6 +#define DA9150_WRITE_MODE_MASK (0x01 << 6) +#define DA9150_REVERT_SHIFT 7 +#define DA9150_REVERT_MASK (0x01 << 7) + +/* DA9150_PAGE_CON_4 = 0x200 */ +#define DA9150_PAGE_SHIFT 0 +#define DA9150_PAGE_MASK (0x3f << 0) +#define DA9150_WRITE_MODE_SHIFT 6 +#define DA9150_WRITE_MODE_MASK (0x01 << 6) +#define DA9150_REVERT_SHIFT 7 +#define DA9150_REVERT_MASK (0x01 << 7) + +/* DA9150_PAGE_CON_5 = 0x280 */ +#define DA9150_PAGE_SHIFT 0 +#define DA9150_PAGE_MASK (0x3f << 0) +#define DA9150_WRITE_MODE_SHIFT 6 +#define DA9150_WRITE_MODE_MASK (0x01 << 6) +#define DA9150_REVERT_SHIFT 7 +#define DA9150_REVERT_MASK (0x01 << 7) + +/* DA9150_PAGE_CON_6 = 0x300 */ +#define DA9150_PAGE_SHIFT 0 +#define DA9150_PAGE_MASK (0x3f << 0) +#define DA9150_WRITE_MODE_SHIFT 6 +#define DA9150_WRITE_MODE_MASK (0x01 << 6) +#define DA9150_REVERT_SHIFT 7 +#define DA9150_REVERT_MASK (0x01 << 7) + +/* DA9150_COREBTLD_STAT_A = 0x302 */ +#define DA9150_BOOTLD_STAT_SHIFT 0 +#define DA9150_BOOTLD_STAT_MASK (0x03 << 0) +#define DA9150_CORE_LOCKUP_SHIFT 2 +#define DA9150_CORE_LOCKUP_MASK (0x01 << 2) + +/* DA9150_COREBTLD_CTRL_A = 0x303 */ +#define DA9150_CORE_RESET_SHIFT 0 +#define DA9150_CORE_RESET_MASK (0x01 << 0) +#define DA9150_CORE_STOP_SHIFT 1 +#define DA9150_CORE_STOP_MASK (0x01 << 1) + +/* DA9150_CORE_CONFIG_A = 0x304 */ +#define DA9150_CORE_MEMMUX_SHIFT 0 +#define DA9150_CORE_MEMMUX_MASK (0x03 << 0) +#define DA9150_WDT_AUTO_START_SHIFT 2 +#define DA9150_WDT_AUTO_START_MASK (0x01 << 2) +#define DA9150_WDT_AUTO_LOCK_SHIFT 3 +#define DA9150_WDT_AUTO_LOCK_MASK (0x01 << 3) +#define DA9150_WDT_HLT_NO_CLK_SHIFT 4 +#define DA9150_WDT_HLT_NO_CLK_MASK (0x01 << 4) + +/* DA9150_CORE_CONFIG_C = 0x305 */ +#define DA9150_CORE_SW_SIZE_SHIFT 0 +#define DA9150_CORE_SW_SIZE_MASK (0xff << 0) + +/* DA9150_CORE_CONFIG_B = 0x306 */ +#define DA9150_BOOTLD_EN_SHIFT 0 +#define DA9150_BOOTLD_EN_MASK (0x01 << 0) +#define DA9150_CORE_EN_SHIFT 2 +#define DA9150_CORE_EN_MASK (0x01 << 2) +#define DA9150_CORE_SW_SRC_SHIFT 3 +#define DA9150_CORE_SW_SRC_MASK (0x07 << 3) +#define DA9150_DEEP_SLEEP_EN_SHIFT 7 +#define DA9150_DEEP_SLEEP_EN_MASK (0x01 << 7) + +/* DA9150_CORE_CFG_DATA_A = 0x307 */ +#define DA9150_CORE_CFG_DT_A_SHIFT 0 +#define DA9150_CORE_CFG_DT_A_MASK (0xff << 0) + +/* DA9150_CORE_CFG_DATA_B = 0x308 */ +#define DA9150_CORE_CFG_DT_B_SHIFT 0 +#define DA9150_CORE_CFG_DT_B_MASK (0xff << 0) + +/* DA9150_CORE_CMD_A = 0x309 */ +#define DA9150_CORE_CMD_SHIFT 0 +#define DA9150_CORE_CMD_MASK (0xff << 0) + +/* DA9150_CORE_DATA_A = 0x30A */ +#define DA9150_CORE_DATA_0_SHIFT 0 +#define DA9150_CORE_DATA_0_MASK (0xff << 0) + +/* DA9150_CORE_DATA_B = 0x30B */ +#define DA9150_CORE_DATA_1_SHIFT 0 +#define DA9150_CORE_DATA_1_MASK (0xff << 0) + +/* DA9150_CORE_DATA_C = 0x30C */ +#define DA9150_CORE_DATA_2_SHIFT 0 +#define DA9150_CORE_DATA_2_MASK (0xff << 0) + +/* DA9150_CORE_DATA_D = 0x30D */ +#define DA9150_CORE_DATA_3_SHIFT 0 +#define DA9150_CORE_DATA_3_MASK (0xff << 0) + +/* DA9150_CORE2WIRE_STAT_A = 0x310 */ +#define DA9150_FW_FWDL_ERR_SHIFT 7 +#define DA9150_FW_FWDL_ERR_MASK (0x01 << 7) + +/* DA9150_CORE2WIRE_CTRL_A = 0x311 */ +#define DA9150_FW_FWDL_EN_SHIFT 0 +#define DA9150_FW_FWDL_EN_MASK (0x01 << 0) +#define DA9150_FG_QIF_EN_SHIFT 1 +#define DA9150_FG_QIF_EN_MASK (0x01 << 1) +#define DA9150_CORE_BASE_ADDR_SHIFT 4 +#define DA9150_CORE_BASE_ADDR_MASK (0x0f << 4) + +/* DA9150_FW_CTRL_A = 0x312 */ +#define DA9150_FW_SEAL_SHIFT 0 +#define DA9150_FW_SEAL_MASK (0xff << 0) + +/* DA9150_FW_CTRL_C = 0x313 */ +#define DA9150_FW_FWDL_CRC_SHIFT 0 +#define DA9150_FW_FWDL_CRC_MASK (0xff << 0) + +/* DA9150_FW_CTRL_D = 0x314 */ +#define DA9150_FW_FWDL_BASE_SHIFT 0 +#define DA9150_FW_FWDL_BASE_MASK (0x0f << 0) + +/* DA9150_FG_CTRL_A = 0x315 */ +#define DA9150_FG_QIF_CODE_SHIFT 0 +#define DA9150_FG_QIF_CODE_MASK (0xff << 0) + +/* DA9150_FG_CTRL_B = 0x316 */ +#define DA9150_FG_QIF_VALUE_SHIFT 0 +#define DA9150_FG_QIF_VALUE_MASK (0xff << 0) + +/* DA9150_FW_CTRL_E = 0x317 */ +#define DA9150_FW_FWDL_SEG_SHIFT 0 +#define DA9150_FW_FWDL_SEG_MASK (0xff << 0) + +/* DA9150_FW_CTRL_B = 0x318 */ +#define DA9150_FW_FWDL_VALUE_SHIFT 0 +#define DA9150_FW_FWDL_VALUE_MASK (0xff << 0) + +/* DA9150_GPADC_CMAN = 0x320 */ +#define DA9150_GPADC_CEN_SHIFT 0 +#define DA9150_GPADC_CEN_MASK (0x01 << 0) +#define DA9150_GPADC_CMUX_SHIFT 1 +#define DA9150_GPADC_CMUX_MASK (0x1f << 1) + +/* DA9150_GPADC_CRES_A = 0x322 */ +#define DA9150_GPADC_CRES_H_SHIFT 0 +#define DA9150_GPADC_CRES_H_MASK (0xff << 0) + +/* DA9150_GPADC_CRES_B = 0x323 */ +#define DA9150_GPADC_CRUN_SHIFT 0 +#define DA9150_GPADC_CRUN_MASK (0x01 << 0) +#define DA9150_GPADC_CRES_L_SHIFT 6 +#define DA9150_GPADC_CRES_L_MASK (0x03 << 6) + +/* DA9150_CC_CFG_A = 0x328 */ +#define DA9150_CC_EN_SHIFT 0 +#define DA9150_CC_EN_MASK (0x01 << 0) +#define DA9150_CC_TIMEBASE_SHIFT 1 +#define DA9150_CC_TIMEBASE_MASK (0x03 << 1) +#define DA9150_CC_CFG_SHIFT 5 +#define DA9150_CC_CFG_MASK (0x03 << 5) +#define DA9150_CC_ENDLESS_MODE_SHIFT 7 +#define DA9150_CC_ENDLESS_MODE_MASK (0x01 << 7) + +/* DA9150_CC_CFG_B = 0x329 */ +#define DA9150_CC_OPT_SHIFT 0 +#define DA9150_CC_OPT_MASK (0x03 << 0) +#define DA9150_CC_PREAMP_SHIFT 2 +#define DA9150_CC_PREAMP_MASK (0x03 << 2) + +/* DA9150_CC_ICHG_RES_A = 0x32A */ +#define DA9150_CC_ICHG_RES_H_SHIFT 0 +#define DA9150_CC_ICHG_RES_H_MASK (0xff << 0) + +/* DA9150_CC_ICHG_RES_B = 0x32B */ +#define DA9150_CC_ICHG_RES_L_SHIFT 3 +#define DA9150_CC_ICHG_RES_L_MASK (0x1f << 3) + +/* DA9150_CC_IAVG_RES_A = 0x32C */ +#define DA9150_CC_IAVG_RES_H_SHIFT 0 +#define DA9150_CC_IAVG_RES_H_MASK (0xff << 0) + +/* DA9150_CC_IAVG_RES_B = 0x32D */ +#define DA9150_CC_IAVG_RES_L_SHIFT 0 +#define DA9150_CC_IAVG_RES_L_MASK (0xff << 0) + +/* DA9150_TAUX_CTRL_A = 0x330 */ +#define DA9150_TAUX_EN_SHIFT 0 +#define DA9150_TAUX_EN_MASK (0x01 << 0) +#define DA9150_TAUX_MOD_SHIFT 1 +#define DA9150_TAUX_MOD_MASK (0x01 << 1) +#define DA9150_TAUX_UPDATE_SHIFT 2 +#define DA9150_TAUX_UPDATE_MASK (0x01 << 2) + +/* DA9150_TAUX_RELOAD_H = 0x332 */ +#define DA9150_TAUX_RLD_H_SHIFT 0 +#define DA9150_TAUX_RLD_H_MASK (0xff << 0) + +/* DA9150_TAUX_RELOAD_L = 0x333 */ +#define DA9150_TAUX_RLD_L_SHIFT 3 +#define DA9150_TAUX_RLD_L_MASK (0x1f << 3) + +/* DA9150_TAUX_VALUE_H = 0x334 */ +#define DA9150_TAUX_VAL_H_SHIFT 0 +#define DA9150_TAUX_VAL_H_MASK (0xff << 0) + +/* DA9150_TAUX_VALUE_L = 0x335 */ +#define DA9150_TAUX_VAL_L_SHIFT 3 +#define DA9150_TAUX_VAL_L_MASK (0x1f << 3) + +/* DA9150_AUX_DATA_0 = 0x338 */ +#define DA9150_AUX_DAT_0_SHIFT 0 +#define DA9150_AUX_DAT_0_MASK (0xff << 0) + +/* DA9150_AUX_DATA_1 = 0x339 */ +#define DA9150_AUX_DAT_1_SHIFT 0 +#define DA9150_AUX_DAT_1_MASK (0xff << 0) + +/* DA9150_AUX_DATA_2 = 0x33A */ +#define DA9150_AUX_DAT_2_SHIFT 0 +#define DA9150_AUX_DAT_2_MASK (0xff << 0) + +/* DA9150_AUX_DATA_3 = 0x33B */ +#define DA9150_AUX_DAT_3_SHIFT 0 +#define DA9150_AUX_DAT_3_MASK (0xff << 0) + +/* DA9150_BIF_CTRL = 0x340 */ +#define DA9150_BIF_ISRC_EN_SHIFT 0 +#define DA9150_BIF_ISRC_EN_MASK (0x01 << 0) + +/* DA9150_TBAT_CTRL_A = 0x342 */ +#define DA9150_TBAT_EN_SHIFT 0 +#define DA9150_TBAT_EN_MASK (0x01 << 0) +#define DA9150_TBAT_SW1_SHIFT 1 +#define DA9150_TBAT_SW1_MASK (0x01 << 1) +#define DA9150_TBAT_SW2_SHIFT 2 +#define DA9150_TBAT_SW2_MASK (0x01 << 2) + +/* DA9150_TBAT_CTRL_B = 0x343 */ +#define DA9150_TBAT_SW_FRC_SHIFT 0 +#define DA9150_TBAT_SW_FRC_MASK (0x01 << 0) +#define DA9150_TBAT_STAT_SW1_SHIFT 1 +#define DA9150_TBAT_STAT_SW1_MASK (0x01 << 1) +#define DA9150_TBAT_STAT_SW2_SHIFT 2 +#define DA9150_TBAT_STAT_SW2_MASK (0x01 << 2) +#define DA9150_TBAT_HIGH_CURR_SHIFT 3 +#define DA9150_TBAT_HIGH_CURR_MASK (0x01 << 3) + +/* DA9150_TBAT_RES_A = 0x344 */ +#define DA9150_TBAT_RES_H_SHIFT 0 +#define DA9150_TBAT_RES_H_MASK (0xff << 0) + +/* DA9150_TBAT_RES_B = 0x345 */ +#define DA9150_TBAT_RES_DIS_SHIFT 0 +#define DA9150_TBAT_RES_DIS_MASK (0x01 << 0) +#define DA9150_TBAT_RES_L_SHIFT 6 +#define DA9150_TBAT_RES_L_MASK (0x03 << 6) + +#endif /* __DA9150_REGISTERS_H */
DA9150 is a combined Charger and Fuel-Gauge IC, with additional GPIO and GPADC functionality. Signed-off-by: Adam Thomson <Adam.Thomson.Opensource@diasemi.com> --- drivers/mfd/Kconfig | 12 + drivers/mfd/Makefile | 2 + drivers/mfd/da9150-core.c | 332 ++++++++++ drivers/mfd/da9150-i2c.c | 176 ++++++ include/linux/mfd/da9150/core.h | 80 +++ include/linux/mfd/da9150/pdata.h | 21 + include/linux/mfd/da9150/registers.h | 1153 ++++++++++++++++++++++++++++++++++ 7 files changed, 1776 insertions(+) create mode 100644 drivers/mfd/da9150-core.c create mode 100644 drivers/mfd/da9150-i2c.c create mode 100644 include/linux/mfd/da9150/core.h create mode 100644 include/linux/mfd/da9150/pdata.h create mode 100644 include/linux/mfd/da9150/registers.h -- 1.9.3 -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html