[v9,3/7] i2c: fsi: Add port structures

Message ID 1528138850-18259-4-git-send-email-eajames@linux.vnet.ibm.com
State Superseded
Headers show
Series
  • i2c: Add FSI-attached I2C master algorithm
Related show

Commit Message

Eddie James June 4, 2018, 7 p.m.
Add and initialize I2C adapters for each port on the FSI-attached I2C
master. Ports for each master are defined in the devicetree.

Signed-off-by: Eddie James <eajames@linux.vnet.ibm.com>
---
 drivers/i2c/busses/i2c-fsi.c | 90 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 90 insertions(+)

Comments

Andy Shevchenko June 4, 2018, 7:17 p.m. | #1
On Mon, Jun 4, 2018 at 10:00 PM, Eddie James <eajames@linux.vnet.ibm.com> wrote:
> Add and initialize I2C adapters for each port on the FSI-attached I2C
> master. Ports for each master are defined in the devicetree.
>
> Signed-off-by: Eddie James <eajames@linux.vnet.ibm.com>
> ---
>  drivers/i2c/busses/i2c-fsi.c | 90 ++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 90 insertions(+)
>
> diff --git a/drivers/i2c/busses/i2c-fsi.c b/drivers/i2c/busses/i2c-fsi.c
> index e1b183c..12130c3 100644
> --- a/drivers/i2c/busses/i2c-fsi.c
> +++ b/drivers/i2c/busses/i2c-fsi.c
> @@ -17,7 +17,10 @@
>  #include <linux/fsi.h>
>  #include <linux/i2c.h>
>  #include <linux/kernel.h>
> +#include <linux/list.h>
>  #include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/slab.h>
>
>  #define FSI_ENGID_I2C          0x7
>
> @@ -123,6 +126,14 @@
>  struct fsi_i2c_master {
>         struct fsi_device       *fsi;
>         u8                      fifo_size;
> +       struct list_head        ports;
> +};
> +
> +struct fsi_i2c_port {
> +       struct list_head        list;
> +       struct i2c_adapter      adapter;
> +       struct fsi_i2c_master   *master;
> +       u16                     port;
>  };
>
>  static int fsi_i2c_read_reg(struct fsi_device *fsi, unsigned int reg,
> @@ -176,9 +187,38 @@ static int fsi_i2c_dev_init(struct fsi_i2c_master *i2c)
>         return fsi_i2c_write_reg(i2c->fsi, I2C_FSI_WATER_MARK, &watermark);
>  }
>
> +static int fsi_i2c_set_port(struct fsi_i2c_port *port)
> +{
> +       int rc;
> +       struct fsi_device *fsi = port->master->fsi;
> +       u32 mode, dummy = 0;
> +
> +       rc = fsi_i2c_read_reg(fsi, I2C_FSI_MODE, &mode);
> +       if (rc)
> +               return rc;
> +
> +       if (FIELD_GET(I2C_MODE_PORT, mode) == port->port)
> +               return 0;
> +

> +       mode = (mode & ~I2C_MODE_PORT) | FIELD_PREP(I2C_MODE_PORT, port->port);

Did you consider to split this to two lines / assignments?

> +       rc = fsi_i2c_write_reg(fsi, I2C_FSI_MODE, &mode);
> +       if (rc)
> +               return rc;
> +
> +       /* reset engine when port is changed */
> +       return fsi_i2c_write_reg(fsi, I2C_FSI_RESET_ERR, &dummy);
> +}
> +
>  static int fsi_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
>                         int num)
>  {
> +       int rc;
> +       struct fsi_i2c_port *port = adap->algo_data;
> +
> +       rc = fsi_i2c_set_port(port);
> +       if (rc)
> +               return rc;
> +
>         return -EOPNOTSUPP;
>  }
>
> @@ -196,23 +236,72 @@ static u32 fsi_i2c_functionality(struct i2c_adapter *adap)
>  static int fsi_i2c_probe(struct device *dev)
>  {
>         struct fsi_i2c_master *i2c;
> +       struct fsi_i2c_port *port;
> +       struct device_node *np;
>         int rc;
> +       u32 port_no;
>
>         i2c = devm_kzalloc(dev, sizeof(*i2c), GFP_KERNEL);
>         if (!i2c)
>                 return -ENOMEM;
>
>         i2c->fsi = to_fsi_dev(dev);
> +       INIT_LIST_HEAD(&i2c->ports);
>
>         rc = fsi_i2c_dev_init(i2c);
>         if (rc)
>                 return rc;
>
> +       /* 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 = kzalloc(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);
> +                       kfree(port);
> +                       continue;
> +               }
> +
> +               list_add(&port->list, &i2c->ports);
> +       }
> +
>         dev_set_drvdata(dev, i2c);
>
>         return 0;
>  }
>
> +static int fsi_i2c_remove(struct device *dev)
> +{
> +       struct fsi_i2c_master *i2c = dev_get_drvdata(dev);
> +       struct fsi_i2c_port *port;
> +
> +       list_for_each_entry(port, &i2c->ports, list) {
> +               i2c_del_adapter(&port->adapter);
> +               kfree(port);
> +       }

Just to be sure, it will be called if and only if all adapters are not
busy. Correct?

> +
> +       return 0;
> +}
> +
>  static const struct fsi_device_id fsi_i2c_ids[] = {
>         { FSI_ENGID_I2C, FSI_VERSION_ANY },
>         { 0 }
> @@ -224,6 +313,7 @@ static int fsi_i2c_probe(struct device *dev)
>                 .name = "i2c-fsi",
>                 .bus = &fsi_bus_type,
>                 .probe = fsi_i2c_probe,
> +               .remove = fsi_i2c_remove,
>         },
>  };
>
> --
> 1.8.3.1
>
Eddie James June 4, 2018, 7:40 p.m. | #2
On 06/04/2018 02:17 PM, Andy Shevchenko wrote:
> On Mon, Jun 4, 2018 at 10:00 PM, Eddie James <eajames@linux.vnet.ibm.com> wrote:
>> Add and initialize I2C adapters for each port on the FSI-attached I2C
>> master. Ports for each master are defined in the devicetree.
>>
>> Signed-off-by: Eddie James <eajames@linux.vnet.ibm.com>
>> ---
>>   drivers/i2c/busses/i2c-fsi.c | 90 ++++++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 90 insertions(+)
>>
>> diff --git a/drivers/i2c/busses/i2c-fsi.c b/drivers/i2c/busses/i2c-fsi.c
>> index e1b183c..12130c3 100644
>> --- a/drivers/i2c/busses/i2c-fsi.c
>> +++ b/drivers/i2c/busses/i2c-fsi.c
>> @@ -17,7 +17,10 @@
>>   #include <linux/fsi.h>
>>   #include <linux/i2c.h>
>>   #include <linux/kernel.h>
>> +#include <linux/list.h>
>>   #include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/slab.h>
>>
>>   #define FSI_ENGID_I2C          0x7
>>
>> @@ -123,6 +126,14 @@
>>   struct fsi_i2c_master {
>>          struct fsi_device       *fsi;
>>          u8                      fifo_size;
>> +       struct list_head        ports;
>> +};
>> +
>> +struct fsi_i2c_port {
>> +       struct list_head        list;
>> +       struct i2c_adapter      adapter;
>> +       struct fsi_i2c_master   *master;
>> +       u16                     port;
>>   };
>>
>>   static int fsi_i2c_read_reg(struct fsi_device *fsi, unsigned int reg,
>> @@ -176,9 +187,38 @@ static int fsi_i2c_dev_init(struct fsi_i2c_master *i2c)
>>          return fsi_i2c_write_reg(i2c->fsi, I2C_FSI_WATER_MARK, &watermark);
>>   }
>>
>> +static int fsi_i2c_set_port(struct fsi_i2c_port *port)
>> +{
>> +       int rc;
>> +       struct fsi_device *fsi = port->master->fsi;
>> +       u32 mode, dummy = 0;
>> +
>> +       rc = fsi_i2c_read_reg(fsi, I2C_FSI_MODE, &mode);
>> +       if (rc)
>> +               return rc;
>> +
>> +       if (FIELD_GET(I2C_MODE_PORT, mode) == port->port)
>> +               return 0;
>> +
>> +       mode = (mode & ~I2C_MODE_PORT) | FIELD_PREP(I2C_MODE_PORT, port->port);
> Did you consider to split this to two lines / assignments?

It fit on one line (< 80 chars) so I left it as-is...

>
>> +       rc = fsi_i2c_write_reg(fsi, I2C_FSI_MODE, &mode);
>> +       if (rc)
>> +               return rc;
>> +
>> +       /* reset engine when port is changed */
>> +       return fsi_i2c_write_reg(fsi, I2C_FSI_RESET_ERR, &dummy);
>> +}
>> +
>>   static int fsi_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
>>                          int num)
>>   {
>> +       int rc;
>> +       struct fsi_i2c_port *port = adap->algo_data;
>> +
>> +       rc = fsi_i2c_set_port(port);
>> +       if (rc)
>> +               return rc;
>> +
>>          return -EOPNOTSUPP;
>>   }
>>
>> @@ -196,23 +236,72 @@ static u32 fsi_i2c_functionality(struct i2c_adapter *adap)
>>   static int fsi_i2c_probe(struct device *dev)
>>   {
>>          struct fsi_i2c_master *i2c;
>> +       struct fsi_i2c_port *port;
>> +       struct device_node *np;
>>          int rc;
>> +       u32 port_no;
>>
>>          i2c = devm_kzalloc(dev, sizeof(*i2c), GFP_KERNEL);
>>          if (!i2c)
>>                  return -ENOMEM;
>>
>>          i2c->fsi = to_fsi_dev(dev);
>> +       INIT_LIST_HEAD(&i2c->ports);
>>
>>          rc = fsi_i2c_dev_init(i2c);
>>          if (rc)
>>                  return rc;
>>
>> +       /* 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 = kzalloc(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);
>> +                       kfree(port);
>> +                       continue;
>> +               }
>> +
>> +               list_add(&port->list, &i2c->ports);
>> +       }
>> +
>>          dev_set_drvdata(dev, i2c);
>>
>>          return 0;
>>   }
>>
>> +static int fsi_i2c_remove(struct device *dev)
>> +{
>> +       struct fsi_i2c_master *i2c = dev_get_drvdata(dev);
>> +       struct fsi_i2c_port *port;
>> +
>> +       list_for_each_entry(port, &i2c->ports, list) {
>> +               i2c_del_adapter(&port->adapter);
>> +               kfree(port);
>> +       }
> Just to be sure, it will be called if and only if all adapters are not
> busy. Correct?

If I understand the code and comments correctly, i2c_del_adapter will 
block until all references to the adapter device are gone. So, should be 
safe to free it now.

Thanks,
Eddie

>
>> +
>> +       return 0;
>> +}
>> +
>>   static const struct fsi_device_id fsi_i2c_ids[] = {
>>          { FSI_ENGID_I2C, FSI_VERSION_ANY },
>>          { 0 }
>> @@ -224,6 +313,7 @@ static int fsi_i2c_probe(struct device *dev)
>>                  .name = "i2c-fsi",
>>                  .bus = &fsi_bus_type,
>>                  .probe = fsi_i2c_probe,
>> +               .remove = fsi_i2c_remove,
>>          },
>>   };
>>
>> --
>> 1.8.3.1
>>
>
>
Benjamin Herrenschmidt June 4, 2018, 11:33 p.m. | #3
On Mon, 2018-06-04 at 22:17 +0300, Andy Shevchenko wrote:
> 
> > +static int fsi_i2c_remove(struct device *dev)
> > +{
> > +       struct fsi_i2c_master *i2c = dev_get_drvdata(dev);
> > +       struct fsi_i2c_port *port;
> > +
> > +       list_for_each_entry(port, &i2c->ports, list) {
> > +               i2c_del_adapter(&port->adapter);
> > +               kfree(port);
> > +       }
> 
> Just to be sure, it will be called if and only if all adapters are not
> busy. Correct?

Actually i2c_del_adapter() will do the right thing. It even waits until
the embedded struct device has been fully released.

As indicated by the comment in there, it should all be turned into
something a bit better, but this is what the i2c layer gives us today.

> > +
> > +       return 0;
> > +}
> > +
> >  static const struct fsi_device_id fsi_i2c_ids[] = {
> >         { FSI_ENGID_I2C, FSI_VERSION_ANY },
> >         { 0 }
> > @@ -224,6 +313,7 @@ static int fsi_i2c_probe(struct device *dev)
> >                 .name = "i2c-fsi",
> >                 .bus = &fsi_bus_type,
> >                 .probe = fsi_i2c_probe,
> > +               .remove = fsi_i2c_remove,
> >         },
> >  };
> > 
> > --
> > 1.8.3.1
> > 
> 
> 
>

Patch

diff --git a/drivers/i2c/busses/i2c-fsi.c b/drivers/i2c/busses/i2c-fsi.c
index e1b183c..12130c3 100644
--- a/drivers/i2c/busses/i2c-fsi.c
+++ b/drivers/i2c/busses/i2c-fsi.c
@@ -17,7 +17,10 @@ 
 #include <linux/fsi.h>
 #include <linux/i2c.h>
 #include <linux/kernel.h>
+#include <linux/list.h>
 #include <linux/module.h>
+#include <linux/of.h>
+#include <linux/slab.h>
 
 #define FSI_ENGID_I2C		0x7
 
@@ -123,6 +126,14 @@ 
 struct fsi_i2c_master {
 	struct fsi_device	*fsi;
 	u8			fifo_size;
+	struct list_head	ports;
+};
+
+struct fsi_i2c_port {
+	struct list_head	list;
+	struct i2c_adapter	adapter;
+	struct fsi_i2c_master	*master;
+	u16			port;
 };
 
 static int fsi_i2c_read_reg(struct fsi_device *fsi, unsigned int reg,
@@ -176,9 +187,38 @@  static int fsi_i2c_dev_init(struct fsi_i2c_master *i2c)
 	return fsi_i2c_write_reg(i2c->fsi, I2C_FSI_WATER_MARK, &watermark);
 }
 
+static int fsi_i2c_set_port(struct fsi_i2c_port *port)
+{
+	int rc;
+	struct fsi_device *fsi = port->master->fsi;
+	u32 mode, dummy = 0;
+
+	rc = fsi_i2c_read_reg(fsi, I2C_FSI_MODE, &mode);
+	if (rc)
+		return rc;
+
+	if (FIELD_GET(I2C_MODE_PORT, mode) == port->port)
+		return 0;
+
+	mode = (mode & ~I2C_MODE_PORT) | FIELD_PREP(I2C_MODE_PORT, port->port);
+	rc = fsi_i2c_write_reg(fsi, I2C_FSI_MODE, &mode);
+	if (rc)
+		return rc;
+
+	/* reset engine when port is changed */
+	return fsi_i2c_write_reg(fsi, I2C_FSI_RESET_ERR, &dummy);
+}
+
 static int fsi_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
 			int num)
 {
+	int rc;
+	struct fsi_i2c_port *port = adap->algo_data;
+
+	rc = fsi_i2c_set_port(port);
+	if (rc)
+		return rc;
+
 	return -EOPNOTSUPP;
 }
 
@@ -196,23 +236,72 @@  static u32 fsi_i2c_functionality(struct i2c_adapter *adap)
 static int fsi_i2c_probe(struct device *dev)
 {
 	struct fsi_i2c_master *i2c;
+	struct fsi_i2c_port *port;
+	struct device_node *np;
 	int rc;
+	u32 port_no;
 
 	i2c = devm_kzalloc(dev, sizeof(*i2c), GFP_KERNEL);
 	if (!i2c)
 		return -ENOMEM;
 
 	i2c->fsi = to_fsi_dev(dev);
+	INIT_LIST_HEAD(&i2c->ports);
 
 	rc = fsi_i2c_dev_init(i2c);
 	if (rc)
 		return rc;
 
+	/* 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 = kzalloc(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);
+			kfree(port);
+			continue;
+		}
+
+		list_add(&port->list, &i2c->ports);
+	}
+
 	dev_set_drvdata(dev, i2c);
 
 	return 0;
 }
 
+static int fsi_i2c_remove(struct device *dev)
+{
+	struct fsi_i2c_master *i2c = dev_get_drvdata(dev);
+	struct fsi_i2c_port *port;
+
+	list_for_each_entry(port, &i2c->ports, list) {
+		i2c_del_adapter(&port->adapter);
+		kfree(port);
+	}
+
+	return 0;
+}
+
 static const struct fsi_device_id fsi_i2c_ids[] = {
 	{ FSI_ENGID_I2C, FSI_VERSION_ANY },
 	{ 0 }
@@ -224,6 +313,7 @@  static int fsi_i2c_probe(struct device *dev)
 		.name = "i2c-fsi",
 		.bus = &fsi_bus_type,
 		.probe = fsi_i2c_probe,
+		.remove = fsi_i2c_remove,
 	},
 };