Re: [PATCH v7 3/7] drivers/i2c: Add port structure to FSI algorithm

[Benjamin Herrenschmidt <benh@kernel.crashing.org>]

On Thu, 2018-05-31 at 00:27 +0300, Andy Shevchenko wrote:
> On Wed, May 30, 2018 at 6:47 PM, Eddie James <eajames@linux.vnet.ibm.com> wrote:
> > On 05/29/2018 06:19 PM, Andy Shevchenko wrote:
> > > On Wed, May 30, 2018 at 1:24 AM, Eddie James <eajames@linux.vnet.ibm.com>
> > > wrote:
> > > >   static int fsi_i2c_probe(struct device *dev)
> > > >   {
> > > 
> > > Isn't below somehow repeats of_i2c_register_devices() ?
> > > Why not to use it?
> > 
> > 
> > Because I need to assign all these port structure fields. Also looks like
> > of_i2c_register_devices creates new devices; I just want an adapter for each
> > port.
> 
> Hmm... Wolfram, what is your opinion on this design?

Andy, I don't understand your issue.

of_i2c_register_devices() is about discovering the i2c devices below a
given bus. This is not what is happening here.

This is a driver for a master that supports multiple busses, so it the
above loop creates all the busses.

> > > > +                       devm_kfree(dev, port);
> > > 
> > > This hurts my eyes. Why?!
> > What would you suggest instead?
> 
> You even didn't wait for answer, why to ask then?

Please stop being so rude.

> Moreover, you didn't answer to my question. Why are you doing that
> call implicitly?

"implicitly" ? What's implicit here ? This is just pretty standard
cleanup after failure, you are being very cryptic here.

Please state precisely what it is you dislike with that code instead of
expecting us to guess and being nasty about it. Eddie was a genuine
question, he doesn't see what you think is "hurtful to the eyes" in the
code you quoted.

> > > > +       if (!list_empty(&i2c->ports)) {
> > > 
> > > My gosh, this is done already in list_for_each*()
> > No, list_for_each_entry does NOT check if the list is empty or if the first
> > entry is NULL.
> 
> Please, read the macro source code again.

Cheers,
Ben.