| 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 | expand |
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 >
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 >> > >
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 > > > > >
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, }, };
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(+)