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.
Ideally drop these for now and bring them in as a precursor patch in the series
that introduces them being used.
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.
They really don't make it significantly easier to follow and after a few
cycles of the driver being patched with new stuff etc, they tend to become
actively misleading.
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.
I'll leave it up to the other maintainers to say they don't mind. But for IIO
please keep strictly to the style (including all the unwritten bits ;)
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.
You are a brave man to tell your customers what to do. If some crazy person
does use multiple of these chips on a device you get to deal with the inevitable
question of why doesn't it work ;)
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.
Ah, I didn't find that from the details I could find via google. In that
case fair enough.
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.
--
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
