RE: [PATCH 1/8] mfd: Add support for DA9150 combined charger & fuel-gauge device

[Opensource [Adam Thomson]]

On Sun, Jun 15, 2014 at 20:49, Jonathan Cameron wrote: 

> Hi Adam,
> 
> Some general comments inline.
> 
> It's been a while since I've looked at any particularly similar parts,
> but it seems to me that a lot of indirection gets added here that
> if anything makes the codes slightly harder to follow...
> 
> Feel free to disagree with me though!

Will do :)

> To my mind all these wrappers add nothing significant so you might as well
> just call da9150->read_dev etc directly.
> 
> Also, what are the read_qif and write_qif for?  They don't seem to be used
> anywhere.

read_qif and write_qif are for the Fuel-Gauge functionality of the chip. The
associated driver will be submitted after acceptance of initial driver code,
and will make use of these functions.

The wrappers automatically choose the correct client to use (QIF uses a
different slave address to the main chip one). Means the child drivers only need
to pass through the da9150 struct and the rest is dealt with underneath.

> The only real reason I can see for these wrappers is because you want
> to hide the struct da9150 contents from the children of the mfd. As you
> aren't doing that, you might as well drop these in favour of direct
> calls to regmap_read and friends.

As I have a need to pass through the main da9150 struct point for the
aforementioned wrappers, it seemed cleaner and more consistent to have wrappers
for these as well, which did the job of regmap access. Means all HW access
uses the same kind of approach, and all sub-devices just need a point to the
main da9150 struct to be able to use the functions.

> I'll continue my tirade against obvious comments. Wrong format and
> adds nothing to what is here as init and exit functions are clearly
> doing what their name suggests (it's one of my pet hates ;)

I agree the comment doesn't add much in terms of description but for me it
breaks up the code to make it easier to follow. However if I get an overwhelming
hatred for this I can change it. Also, I know the rule regarding single/multiple
line comments but here again I feel it helps separate the code and makes it
easier to read.

> As a general good practice point, I'd rather that the driver supported
> more than one instance of the chip.. Hence you'd take a copy of da9150_devs
> to use here.  I guess it is relatively unlikely with one of these, but
> you never know ;)

Have followed the general methods for MFD here, and a number of drivers take the
same approach. Also, I think it would be undesirable to have multiple charger
chips of the same type in one platform. I agree generally it's best to support
multiple instances, but here I don't think we should.

> Why does this need it's own file?  Does the DA9150 support any other
> interfaces?

Yes, the DA9150 also has a SPI interface. At present the plan is to just add I2C
support for now, but in the future we may add SPI support, so have written the
code with this in mind.

> Why the indirection?  The da9150 only supports i2c as far as I can see.

As per my last comment. 

> I'd roll this into one line and not bother with the local variable...

Fair enough but I think this keeps the code cleaner, and to me it makes sense
for the actual logic to be in core file as that's interface agnostic.

> Drop comments on things that are self-evident.  Also these are one
> line comments so should be using the single line comment syntax.

As per my previous comment I think it just helps to break up the code and makes
it more readable. Will change it though if the general consensus is to remove
it.