On 16/06/14 14:12, Opensource [Adam Thomson] wrote:
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.
I'll be interested to see how this interacts with the rest of the driver.
Or does it? Often these dual i2c address devices are best handled as two
entirely separate drivers sitting on the same bus. No idea if that
is true here as don't have a datasheet!
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.
Kernel code generally has to keep to the kernel coding style. If you leave the
single line comments as are, then chances are the mfd maintainers will get a
patch soon after 'fixing' them which is always irritating.
Having said that, mfd is their area not mine, so up to Lee / Samuel.
The same is true of the code structure comments. Now for the IIO driver
I get to be picky ;)
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.
I'm happy to let this go if it is something that would not be done, but we
certainly should not be preventing it if someone wants to build hardware
with more than one of these. If they submit a patch later allowing it because
they have hardware that does this, then there is no way anyone is going to
block it.
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.
Fair enough. I couldn't find a datasheet and the product brief only seemed to mention
i2c.
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.
--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
