Re: [PATCH] i2c: Driver to expose PowerNV platform i2c busses

On Thu, 2014-11-13 at 14:10 +0100, Wolfram Sang wrote:

quoted

quoted

Please sort the includes.

Ugh ? Since when do we do that ? :-)

Since I realised it is more readable and reduces likeliness of
duplicated includes.

Ok, I assume alphabetical rather than Ingo's aesthetic "tree" ?

quoted

quoted

quoted

+	rc = opal_i2c_request(token, bus_id, req);
+	if (rc != OPAL_ASYNC_COMPLETION) {
+		rc = -EIO;
+		goto exit;
+	}
+
+	rc = opal_async_wait_response(token, &msg);
+	if (rc) {
+		rc = -EIO;
+		goto exit;

Is it really -EIO? Maybe -ETIMEDOUT?

No, there is no timeout, if that fails something went quite wrong, it
could almost be a BUG_ON (basically we passed a wrong token or a NULL
msg).

OK. I'd think it at least makes sense to use error codes which
distinguish I2C bus errors from OPAL interface errors. Always using -EIO
seems very generic :)

Ok, any suggestion ? We don't have a -EINTERNAL :) In any case, I'm not
too worried, the above basically cannot happen.

quoted

quoted

I don't think you should offer I2C_FUNC_I2C with those limitations. Is
there a case you really needs this?

Yes there is, and it's pretty common :-) I actually added this to
Neelesh original driver, it's the "smbus" style but with 2 bytes offset.
Typically what we need for driver such as at24. They use normal raw i2c
writes for writes but need the 2-bytes write + read combo without stop
for reads.

Understood. So, basically something like I2C_SMBUS_WORD_I2C_BLOCK_DATA
is missing where the 'command' argument is not u8 but u16? Brainstorming
here, not relevant for this driver now.

Yes, or a generic "N bytes command". My FW driver supports up to 4 bytes
in fact at the moment.

quoted

quoted

quoted

+	adapter = kzalloc(sizeof(struct i2c_adapter), GFP_KERNEL);

devm_kzalloc?

Ah, I never got the knack of using the new devm stuff, Neelesh, can you
take care of this ?

New? :D This would be a dead simple exercise to learn about it ;)

Oh I *know* about it, it's just a habit I didn't catch ... yet :-) I'm
probably getting old and set in my ways ..... :-)

quoted

quoted

quoted

+	adapter->dev.parent = &pdev->dev;
+	adapter->dev.of_node = of_node_get(pdev->dev.of_node);
+	pname = of_get_property(pdev->dev.of_node, "port-name", NULL);

I have never seen this binding before, it looks fishy. Where is it documented?

We made it up, like pretty every SoC vendor out there. What's fishy
about it ? It's a very good way to get fixed i2c port names on the
system, the firmware defines them.

But the SoC vendors prefix it with their company name and add
documentation for the binding.

Why do we need to prefix arbitrary props for a very specific device ?
When adding things to an existing more/less generic device it makes some
sense but here I don't see much point. I can whip up a "binding"
document for this adapter and make "port-name" be part of it if you
want :) In fact a better name for the property might be "bus-id"...

Furthermore, this is just wrong, too. The adapter name is the name of
the IP core or chip or whatever which does the I2C bus. It is not the
functional name of the bus. It should be plain "Opal I2C" or similar.

But that really makes no sense. On one hand we have a way to get
something decent and useful out of i2c-detect -l, and on the other hand,
we get a list of N (N = 3*number of P8 chips at least) identical names
and have to go through hoops figuring out which is which ...

I don't see why we can't use the name for that... the name scheme I use
does convey both bits of information btw, I use something like:

	p8_xxxxxxxx_eypz (ex: p8_00000000_e0p0)

Where p8 means power 8 (centaur's, our memory buffers, will have
something else there), xxxxxxxx is the chip ID (identify a processor or
centaur chip uniquely in the system), y the engine number on the chip
and z the port number on the engine.

Cheers,
Ben.

Regards,

   Wolfram