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

[Eddie James]

On 05/29/2018 06:19 PM, Andy Shevchenko wrote:
> On Wed, May 30, 2018 at 1:24 AM, Eddie James <eajames@xxxxxxxxxxxxxxxxxx> wrote:
> > From: "Edward A. James" <eajames@xxxxxxxxxx>
> >
> > Add and initialize I2C adapters for each port on the FSI-attached I2C
> > master. Ports for each master are defined in the devicetree.
> > +#include <linux/of.h>
>
> > +static int fsi_i2c_set_port(struct fsi_i2c_port *port)
> > +{
> > +       int rc;
> > +       struct fsi_device *fsi = port->master->fsi;
> > +       u32 mode, dummy = 0;
> > +       u16 old_port;
> > +
> > +       rc = fsi_i2c_read_reg(fsi, I2C_FSI_MODE, &mode);
> > +       if (rc)
> > +               return rc;
> > +
> > +       old_port = GETFIELD(I2C_MODE_PORT, mode);
> > +
> > +       if (old_port != port->port) {
> Why not simple
>
> if (port->port == GETFIELD())
>    return 0;
>
> ?

Sure.

> > +               /* reset engine when port is changed */
> > +               rc = fsi_i2c_write_reg(fsi, I2C_FSI_RESET_ERR, &dummy);
> > +               if (rc)
> > +                       return rc;
> > +       }
> > +       return rc;
> It's hardly would be non-zero, right?

No, fsi_i2c_read_reg and fsi_i2c_write_reg both return 0 on success and 
non-zero on error. That is the desired behavior of this function also.

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

> > +       /* Add adapter for each i2c port of the master. */
> > +       for_each_available_child_of_node(dev->of_node, np) {
> > +               rc = of_property_read_u32(np, "reg", &port_no);
> > +               if (rc || port_no > USHRT_MAX)
> > +                       continue;
> > +
> > +               port = devm_kzalloc(dev, sizeof(*port), GFP_KERNEL);
> > +               if (!port)
> > +                       break;
> > +
> > +               port->master = i2c;
> > +               port->port = port_no;
> > +
> > +               port->adapter.owner = THIS_MODULE;
> > +               port->adapter.dev.of_node = np;
> > +               port->adapter.dev.parent = dev;
> > +               port->adapter.algo = &fsi_i2c_algorithm;
> > +               port->adapter.algo_data = port;
> > +
> > +               snprintf(port->adapter.name, sizeof(port->adapter.name),
> > +                        "i2c_bus-%u", port_no);
> > +
> > +               rc = i2c_add_adapter(&port->adapter);
> > +               if (rc < 0) {
> > +                       dev_err(dev, "Failed to register adapter: %d\n", rc);
> > +                       devm_kfree(dev, port);
> This hurts my eyes. Why?!

What would you suggest instead?

> > +                       continue;
> > +               }
> > +
> > +               list_add(&port->list, &i2c->ports);
> > +       }
> > +
> >          dev_set_drvdata(dev, i2c);
> >
> >          return 0;
> >   }
> > +       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.

Thanks,
Eddie

> > +               list_for_each_entry(port, &i2c->ports, list) {
> > +                       i2c_del_adapter(&port->adapter);
> > +               }
> > +       }