On Thu, 2014-11-13 at 14:10 +0100, Wolfram Sang wrote:
> > > 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" ?
> > > > + 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.
> > > 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.
> > > > + 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 ..... :-)
> > > > + 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
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
