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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





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;

?

+               /* 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?

Sorry, misunderstood your comment here. You are correct, it can only be zero.

Thanks,
Eddie


+}
  static int fsi_i2c_probe(struct device *dev)
  {
Isn't below somehow repeats of_i2c_register_devices() ?
Why not to use it?

+       /* 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?!

+                       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*()

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



--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux