On Fri, 26 Jun 2015, Adam Thomson wrote: > Signed-off-by: Adam Thomson <Adam.Thomson.Opensource@xxxxxxxxxxx> > --- Changelog v1 => v2? > drivers/mfd/da9150-core.c | 162 ++++++++++++++++++++++++++++++++++++++-- > include/linux/mfd/da9150/core.h | 19 +++++ > include/linux/mfd/da9150/fg.h | 29 +++++++ > 3 files changed, 202 insertions(+), 8 deletions(-) > create mode 100644 include/linux/mfd/da9150/fg.h > > diff --git a/drivers/mfd/da9150-core.c b/drivers/mfd/da9150-core.c > index 5549817..8feb75d 100644 > --- a/drivers/mfd/da9150-core.c > +++ b/drivers/mfd/da9150-core.c > @@ -21,8 +21,80 @@ > #include <linux/interrupt.h> > #include <linux/mfd/core.h> > #include <linux/mfd/da9150/core.h> > +#include <linux/mfd/da9150/fg.h> > #include <linux/mfd/da9150/registers.h> > > +/* Raw device access, used for QIF */ > +static int da9150_i2c_read_device(struct i2c_client *client, u8 addr, int count, > + u8 *buf) > +{ > + struct i2c_msg xfer; > + int ret; > + > + /* > + * Read is split into two transfers as device expects STOP/START rather > + * than repeated start to carry out this kind of access. > + */ > + > + /* Write address */ > + xfer.addr = client->addr; > + xfer.flags = 0; > + xfer.len = 1; > + xfer.buf = &addr; > + > + ret = i2c_transfer(client->adapter, &xfer, 1); > + if (ret != 1) { > + if (ret < 0) > + return ret; > + else > + return -EIO; > + } > + > + /* Read data */ > + xfer.addr = client->addr; > + xfer.flags = I2C_M_RD; > + xfer.len = count; > + xfer.buf = buf; > + > + ret = i2c_transfer(client->adapter, &xfer, 1); > + if (ret == 1) > + return 0; > + else if (ret < 0) > + return ret; > + else > + return -EIO; > +} > + > +static int da9150_i2c_write_device(struct i2c_client *client, u8 addr, > + int count, const u8 *buf) > +{ > + struct i2c_msg xfer; > + u8 *reg_data; > + int ret; > + > + reg_data = kzalloc(1 + count, GFP_KERNEL); > + if (!reg_data) > + return -ENOMEM; > + > + reg_data[0] = addr; > + memcpy(®_data[1], buf, count); > + > + /* Write address & data */ > + xfer.addr = client->addr; > + xfer.flags = 0; > + xfer.len = 1 + count; > + xfer.buf = reg_data; > + > + ret = i2c_transfer(client->adapter, &xfer, 1); > + kfree(reg_data); > + if (ret == 1) > + return 0; > + else if (ret < 0) > + return ret; > + else > + return -EIO; > +} > + > static bool da9150_volatile_reg(struct device *dev, unsigned int reg) > { > switch (reg) { > @@ -107,6 +179,28 @@ static const struct regmap_config da9150_regmap_config = { > .volatile_reg = da9150_volatile_reg, > }; > > +void da9150_read_qif(struct da9150 *da9150, u8 addr, int count, u8 *buf) > +{ > + int ret; > + > + ret = da9150->read_dev(da9150->core_qif, addr, count, buf); > + if (ret < 0) > + dev_err(da9150->dev, "Failed to read from QIF 0x%x: %d\n", > + addr, ret); > +} > +EXPORT_SYMBOL_GPL(da9150_read_qif); > + > +void da9150_write_qif(struct da9150 *da9150, u8 addr, int count, const u8 *buf) > +{ > + int ret; > + > + ret = da9150->write_dev(da9150->core_qif, addr, count, buf); > + if (ret < 0) > + dev_err(da9150->dev, "Failed to write to QIF 0x%x: %d\n", > + addr, ret); > +} > +EXPORT_SYMBOL_GPL(da9150_write_qif); > + > u8 da9150_reg_read(struct da9150 *da9150, u16 reg) > { > int val, ret; > @@ -297,6 +391,15 @@ static struct resource da9150_charger_resources[] = { > }, > }; > > +static struct resource da9150_fg_resources[] = { > + { > + .name = "FG", > + .start = DA9150_IRQ_FG, > + .end = DA9150_IRQ_FG, > + .flags = IORESOURCE_IRQ, Can you use the DEFINE_RES_* helpers? > + }, > +}; > + > static struct mfd_cell da9150_devs[] = { > { > .name = "da9150-gpadc", > @@ -310,6 +413,12 @@ static struct mfd_cell da9150_devs[] = { > .resources = da9150_charger_resources, > .num_resources = ARRAY_SIZE(da9150_charger_resources), > }, > + { > + .name = "da9150-fuelgauge", > + .of_compatible = "dlg,da9150-fg", > + .resources = da9150_fg_resources, > + .num_resources = ARRAY_SIZE(da9150_fg_resources), > + }, > }; > > static int da9150_probe(struct i2c_client *client, > @@ -317,11 +426,14 @@ static int da9150_probe(struct i2c_client *client, > { > struct da9150 *da9150; > struct da9150_pdata *pdata = dev_get_platdata(&client->dev); > + int qif_addr; > int ret; > > da9150 = devm_kzalloc(&client->dev, sizeof(*da9150), GFP_KERNEL); > - if (!da9150) > - return -ENOMEM; > + if (!da9150) { > + ret = -ENOMEM; > + goto fail; Don't do this. 'fail' doesn't do anything, just return here. > + } > > da9150->dev = &client->dev; > da9150->irq = client->irq; > @@ -332,19 +444,46 @@ static int da9150_probe(struct i2c_client *client, > ret = PTR_ERR(da9150->regmap); > dev_err(da9150->dev, "Failed to allocate register map: %d\n", > ret); > - return ret; > + goto fail; It was better before. > + } > + > + /* Setup secondary I2C interface for QIF access */ > + qif_addr = da9150_reg_read(da9150, DA9150_CORE2WIRE_CTRL_A); > + qif_addr = (qif_addr & DA9150_CORE_BASE_ADDR_MASK) >> 1; > + qif_addr |= DA9150_QIF_I2C_ADDR_LSB; > + da9150->core_qif = i2c_new_dummy(client->adapter, qif_addr); > + if (!da9150->core_qif) { > + dev_err(da9150->dev, "Failed to attach QIF client\n"); > + ret = -ENODEV; > + goto fail; Same. > } > > - da9150->irq_base = pdata ? pdata->irq_base : -1; > + i2c_set_clientdata(da9150->core_qif, da9150); > + da9150->read_dev = (read_dev_t) da9150_i2c_read_device; > + da9150->write_dev = (write_dev_t) da9150_i2c_write_device; Is it possible for read_dev to be set to anything other than da9150_i2c_read_device? > + if (pdata) { > + da9150->irq_base = pdata->irq_base; > + > + da9150_devs[2].platform_data = pdata->fg_pdata; > + da9150_devs[2].pdata_size = sizeof(struct da9150_fg_pdata); This is fragile. If another device gets inserted above the FG, this will break. > + } else { > + da9150->irq_base = -1; > + } > > 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) > - return ret; > + if (ret) { > + dev_err(da9150->dev, "Failed to add regmap irq chip: %d\n", > + ret); > + goto regmap_irq_fail; > + } > + > > da9150->irq_base = regmap_irq_chip_get_base(da9150->regmap_irq_data); > + > enable_irq_wake(da9150->irq); > > ret = mfd_add_devices(da9150->dev, -1, da9150_devs, > @@ -352,11 +491,17 @@ static int da9150_probe(struct i2c_client *client, > da9150->irq_base, NULL); > if (ret) { > dev_err(da9150->dev, "Failed to add child devices: %d\n", ret); > - regmap_del_irq_chip(da9150->irq, da9150->regmap_irq_data); > - return ret; > + goto mfd_fail; > } > > return 0; > + > +mfd_fail: > + regmap_del_irq_chip(da9150->irq, da9150->regmap_irq_data); > +regmap_irq_fail: > + i2c_unregister_device(da9150->core_qif); > +fail: > + return ret; > } > > static int da9150_remove(struct i2c_client *client) > @@ -365,6 +510,7 @@ static int da9150_remove(struct i2c_client *client) > > regmap_del_irq_chip(da9150->irq, da9150->regmap_irq_data); > mfd_remove_devices(da9150->dev); > + i2c_unregister_device(da9150->core_qif); > > return 0; > } > diff --git a/include/linux/mfd/da9150/core.h b/include/linux/mfd/da9150/core.h > index 76e6689..98ecfea 100644 > --- a/include/linux/mfd/da9150/core.h > +++ b/include/linux/mfd/da9150/core.h > @@ -15,9 +15,11 @@ > #define __DA9150_CORE_H > > #include <linux/device.h> > +#include <linux/i2c.h> > #include <linux/interrupt.h> > #include <linux/regmap.h> > > + Please remove this. > /* I2C address paging */ > #define DA9150_REG_PAGE_SHIFT 8 > #define DA9150_REG_PAGE_MASK 0xFF > @@ -46,23 +48,40 @@ > #define DA9150_IRQ_GPADC 19 > #define DA9150_IRQ_WKUP 20 > > +/* Platform Data */ No need for this comment. The pdata in the name give it away. > +struct da9150_fg_pdata; Why can't you just put the struct definition in here? > struct da9150_pdata { > int irq_base; > + struct da9150_fg_pdata *fg_pdata; > }; > > +/* I/O function typedefs for raw access */ > +typedef int (*read_dev_t)(void *client, u8 addr, int count, u8 *buf); > +typedef int (*write_dev_t)(void *client, u8 addr, int count, const u8 *buf); No need to typedef, just pop them directly into the struct. Although I'm not convinced that need them at all really. > struct da9150 { > struct device *dev; > struct regmap *regmap; > + struct i2c_client *core_qif; > + > + read_dev_t read_dev; > + write_dev_t write_dev; > + > struct regmap_irq_chip_data *regmap_irq_data; > int irq; > int irq_base; Why are there 2 irq_bases? That could get pretty confusing. Why is it being stored? Same with irq. > }; > > /* Device I/O */ > +void da9150_read_qif(struct da9150 *da9150, u8 addr, int count, u8 *buf); > +void da9150_write_qif(struct da9150 *da9150, u8 addr, int count, const u8 *buf); What is qif? The naming convention isn't very transparent. > u8 da9150_reg_read(struct da9150 *da9150, u16 reg); > void da9150_reg_write(struct da9150 *da9150, u16 reg, u8 val); > void da9150_set_bits(struct da9150 *da9150, u16 reg, u8 mask, u8 val); > > void da9150_bulk_read(struct da9150 *da9150, u16 reg, int count, u8 *buf); > void da9150_bulk_write(struct da9150 *da9150, u16 reg, int count, const u8 *buf); > + > #endif /* __DA9150_CORE_H */ > diff --git a/include/linux/mfd/da9150/fg.h b/include/linux/mfd/da9150/fg.h > new file mode 100644 > index 0000000..0ff52ab > --- /dev/null > +++ b/include/linux/mfd/da9150/fg.h > @@ -0,0 +1,29 @@ > +/* > + * DA9150 MFD Driver - Fuel Gauge Data Does it really require it's very own header file? > + * Copyright (c) 2015 Dialog Semiconductor > + * > + * Author: Adam Thomson <Adam.Thomson.Opensource@xxxxxxxxxxx> > + * > + * 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_FG_H > +#define __DA9150_FG_H > + > +#include <linux/power_supply.h> > + > +/* I2C sub-device address */ > +#define DA9150_QIF_I2C_ADDR_LSB 0x5 > + > +/* Platform data */ > +struct da9150_fg_pdata { > + u32 update_interval; /* msecs */ > + u8 warn_soc_lvl; /* % value */ > + u8 crit_soc_lvl; /* % value */ > +}; > + > +#endif /* __DA9150_FG_H */ -- Lee Jones Linaro STMicroelectronics Landing Team Lead Linaro.org │ Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html