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

[Wolfram Sang <wsa@the-dreams.de>]

[-- Attachment #1: Type: text/plain, Size: 2491 bytes --]


> > Please sort the includes.
> 
> Ugh ? Since when do we do that ? :-)

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

> > > +	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 :)

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

> > > +	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 ;)

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

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.

Regards,

   Wolfram

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]