RE: [PATCH v2 1/4] mfd: da9150: Add support for Fuel-Gauge

[Opensource [Adam Thomson]]

On July 3, 2015 16:16, Lee Jones wrote:

> 
> Changelog v1 => v2?

Is described in the cover letter, but can move that information to the individual
patches in the future, if that helps.

> >
> > +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?

Hadn't spotted those before. Will use those instead.

> > -	if (!da9150)
> > -		return -ENOMEM;
> > +	if (!da9150) {
> > +		ret = -ENOMEM;
> > +		goto fail;
> 
> Don't do this.  'fail' doesn't do anything, just return here.

Ok. I was opting for choosing one consistent method of handling failure, rather
than a combination of gotos and immediate returns, but will change.

> 
> > +	}
> >
> >  	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.

Ok

> 
> > +	}
> > +
> > +	/* 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.

Ok

> 
> >  	}
> >
> > -	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?

Right now there is no SPI support in the driver so no. Given the comment further
down, I'll remove these function pointers for now, and directly call the I2C
related functions.

> 
> > +	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.
> 

It is unlikely, but you are right. Will fix this.

> > 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.

Yep.

> 
> >  /* 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.

Fine.

> 
> > +struct da9150_fg_pdata;
> 
> Why can't you just put the struct definition in here?
> 

Was a design choice as I was providing a header for FG to expose the temp
reading callback function and figured it made sense to keep other FG related
items in there as well. Can fold them all into the core.h though if that's the
preferred method.

> >  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.

Will remove use of this for now, as without SPI support, they don't add
anything.

> 
> >  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.

Not 2 IRQ bases. 'irq' is the chip IRQ. Same as is done in a number of other
MFD drivers. Technically though they don't need storing within the private data,
and was done as a convenience. I can add a follow up patch to remove this, if
it's wanted though.

> 
> >  };
> >
> >  /* 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.

This is an interface which is specific to the device (Query Interface), and is
used to access data from the fuel-gauge. Is documented in the device
datasheet, but can add comments here just to clarify. 

> 
> >  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?

Will fold this into the main header, if you don't want to separate out the FG
related data and temp callback related defines.
N‹§²æ¸›yú²X¬¶ÇvØ–)Þ{.nlj·¥Š{±‘êX§¶›¡Ü}©ž²ÆzÚj:+v‰¨¾«‘êZ+€Êzf£¢·hšˆ§~†­†Ûÿû®w¥¢¸?™¨è&¢)ßf”ùy§m…á«a¶Úÿ0¶ìå