| Message ID | 1606717066-3785-1-git-send-email-pthombar@cadence.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | I3C mastership handover support | expand |
On Mon, 30 Nov 2020, Parshuram Thombare wrote: > Removed last argument 'secondary' and restructured i3c_master_register > to move code that can be common to i3c_secondary_master_register > to separate function i3c_master_init. > > Signed-off-by: Parshuram Thombare <pthombar@cadence.com> [...] > +static int i3c_master_init(struct i3c_master_controller *master, > + struct device *parent, > + const struct i3c_master_controller_ops *ops, > + bool secondary) > { > unsigned long i2c_scl_rate = I3C_BUS_I2C_FM_PLUS_SCL_RATE; > struct i3c_bus *i3cbus = i3c_master_get_bus(master); > @@ -2535,10 +2514,49 @@ int i3c_master_register(struct i3c_master_controller *master, > goto err_put_dev; > } > > - ret = i3c_master_bus_init(master); > + ret = i3c_primary_master_bus_init(master); > if (ret) > goto err_destroy_wq; > > + return 0; > + > +err_destroy_wq: > + destroy_workqueue(master->wq); > + > +err_put_dev: > + put_device(&master->dev); > + > + return ret; > +} [...] > +int i3c_primary_master_register(struct i3c_master_controller *master, > + struct device *parent, > + const struct i3c_master_controller_ops *ops) > +{ > + int ret; > + > + ret = i3c_master_init(master, parent, ops, false); > + if (ret) > + return ret; > + > ret = device_add(&master->dev); > if (ret) > goto err_cleanup_bus; > @@ -2568,15 +2586,13 @@ int i3c_master_register(struct i3c_master_controller *master, > err_cleanup_bus: > i3c_master_bus_cleanup(master); > > -err_destroy_wq: > destroy_workqueue(master->wq); > > -err_put_dev: > put_device(&master->dev); > > return ret; > } This looks a bit confusing. Here you're rolling back detailss in i3c_primary_master_register() that were factored out in i3c_master_init(). If i3c_master_init() is successful, then you shouldn't be undoing its things openly in i3c_primary_master_register(). Instead, there should be another function that does the reverse of i3c_master_init() here. Nicolas
>This looks a bit confusing. Here you're rolling back detailss in >i3c_primary_master_register() that were factored out in >i3c_master_init(). If i3c_master_init() is successful, then you >shouldn't be undoing its things openly in i3c_primary_master_register(). >Instead, there should be another function that does the reverse of >i3c_master_init() here. Every function do its cleanup in case of failures. And if any failure occur after successful i3c_master_init(), we have function i3c_master_bus_cleanup() which does the major cleanup. Regards, Parshuram Thombare
On 08/12/2020 05:47:51+0000, Parshuram Raju Thombare wrote: > >This looks a bit confusing. Here you're rolling back detailss in > >i3c_primary_master_register() that were factored out in > >i3c_master_init(). If i3c_master_init() is successful, then you > >shouldn't be undoing its things openly in i3c_primary_master_register(). > >Instead, there should be another function that does the reverse of > >i3c_master_init() here. > > Every function do its cleanup in case of failures. > And if any failure occur after successful i3c_master_init(), we have > function i3c_master_bus_cleanup() which does the major cleanup. > The point from Nicolas here was that the workqueue is allocated in i3c_master_init so you should have a function to destroy it instead of having to do that separately in i3c_primary_master_register. The same is true for the put_device. Or you have to ensure i3c_masterdev_release is called when i3c_primary_master_register fails.
Hi Alexandre, Thank you for review comments. >On 08/12/2020 05:47:51+0000, Parshuram Raju Thombare wrote: >> >This looks a bit confusing. Here you're rolling back detailss in >> >i3c_primary_master_register() that were factored out in >> >i3c_master_init(). If i3c_master_init() is successful, then you >> >shouldn't be undoing its things openly in i3c_primary_master_register(). >> >Instead, there should be another function that does the reverse of >> >i3c_master_init() here. >> >> Every function do its cleanup in case of failures. >> And if any failure occur after successful i3c_master_init(), we have >> function i3c_master_bus_cleanup() which does the major cleanup. >> > >The point from Nicolas here was that the workqueue is allocated in >i3c_master_init so you should have a function to destroy it instead of >having to do that separately in i3c_primary_master_register. The same is >true for the put_device. Or you have to ensure i3c_masterdev_release >is called when i3c_primary_master_register fails. Ok, IIUC, it is just that we need separate clean up function. i3c_master_bus_cleanup is used here to call driver cleanup function and detach + release devices from bus, and I am not sure i3c_masterdev_release can be used for that. Better alternative is i3c_master_unregister(). However, it doesn't handle the case where device_add fails, as this function unconditionally calls device_unregister which call both device_del and put_device.
diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c index b61bf53..56e8fe4 100644 --- a/drivers/i3c/master.c +++ b/drivers/i3c/master.c @@ -1637,7 +1637,7 @@ static void i3c_master_detach_free_devs(struct i3c_master_controller *master) } /** - * i3c_master_bus_init() - initialize an I3C bus + * i3c_primary_master_bus_init() - initialize an I3C bus * @master: main master initializing the bus * * This function is following all initialisation steps described in the I3C @@ -1668,7 +1668,7 @@ static void i3c_master_detach_free_devs(struct i3c_master_controller *master) * * Return: a 0 in case of success, an negative error code otherwise. */ -static int i3c_master_bus_init(struct i3c_master_controller *master) +static int i3c_primary_master_bus_init(struct i3c_master_controller *master) { enum i3c_addr_slot_status status; struct i2c_dev_boardinfo *i2cboardinfo; @@ -2441,31 +2441,10 @@ static int i3c_master_check_ops(const struct i3c_master_controller_ops *ops) return 0; } -/** - * i3c_master_register() - register an I3C master - * @master: master used to send frames on the bus - * @parent: the parent device (the one that provides this I3C master - * controller) - * @ops: the master controller operations - * @secondary: true if you are registering a secondary master. Will return - * -ENOTSUPP if set to true since secondary masters are not yet - * supported - * - * This function takes care of everything for you: - * - * - creates and initializes the I3C bus - * - populates the bus with static I2C devs if @parent->of_node is not - * NULL - * - registers all I3C devices added by the controller during bus - * initialization - * - registers the I2C adapter and all I2C devices - * - * Return: 0 in case of success, a negative error code otherwise. - */ -int i3c_master_register(struct i3c_master_controller *master, - struct device *parent, - const struct i3c_master_controller_ops *ops, - bool secondary) +static int i3c_master_init(struct i3c_master_controller *master, + struct device *parent, + const struct i3c_master_controller_ops *ops, + bool secondary) { unsigned long i2c_scl_rate = I3C_BUS_I2C_FM_PLUS_SCL_RATE; struct i3c_bus *i3cbus = i3c_master_get_bus(master); @@ -2535,10 +2514,49 @@ int i3c_master_register(struct i3c_master_controller *master, goto err_put_dev; } - ret = i3c_master_bus_init(master); + ret = i3c_primary_master_bus_init(master); if (ret) goto err_destroy_wq; + return 0; + +err_destroy_wq: + destroy_workqueue(master->wq); + +err_put_dev: + put_device(&master->dev); + + return ret; +} + +/** + * i3c_primary_master_register() - register an I3C master + * @master: master used to send frames on the bus + * @parent: the parent device (the one that provides this I3C master + * controller) + * @ops: the master controller operations + * + * This function takes care of everything for you: + * + * - creates and initializes the I3C bus + * - populates the bus with static I2C devs if @parent->of_node is not + * NULL + * - registers all I3C devices added by the controller during bus + * initialization + * - registers the I2C adapter and all I2C devices + * + * Return: 0 in case of success, a negative error code otherwise. + */ +int i3c_primary_master_register(struct i3c_master_controller *master, + struct device *parent, + const struct i3c_master_controller_ops *ops) +{ + int ret; + + ret = i3c_master_init(master, parent, ops, false); + if (ret) + return ret; + ret = device_add(&master->dev); if (ret) goto err_cleanup_bus; @@ -2568,15 +2586,13 @@ int i3c_master_register(struct i3c_master_controller *master, err_cleanup_bus: i3c_master_bus_cleanup(master); -err_destroy_wq: destroy_workqueue(master->wq); -err_put_dev: put_device(&master->dev); return ret; } -EXPORT_SYMBOL_GPL(i3c_master_register); +EXPORT_SYMBOL_GPL(i3c_primary_master_register); /** * i3c_master_unregister() - unregister an I3C master diff --git a/drivers/i3c/master/dw-i3c-master.c b/drivers/i3c/master/dw-i3c-master.c index 8513bd3..4f4d41f 100644 --- a/drivers/i3c/master/dw-i3c-master.c +++ b/drivers/i3c/master/dw-i3c-master.c @@ -1166,8 +1166,8 @@ static int dw_i3c_probe(struct platform_device *pdev) master->maxdevs = ret >> 16; master->free_pos = GENMASK(master->maxdevs - 1, 0); - ret = i3c_master_register(&master->base, &pdev->dev, - &dw_mipi_i3c_ops, false); + ret = i3c_primary_master_register(&master->base, &pdev->dev, + &dw_mipi_i3c_ops); if (ret) goto err_assert_rst; diff --git a/drivers/i3c/master/i3c-master-cdns.c b/drivers/i3c/master/i3c-master-cdns.c index 3f22269..f1d6d68 100644 --- a/drivers/i3c/master/i3c-master-cdns.c +++ b/drivers/i3c/master/i3c-master-cdns.c @@ -1644,8 +1644,8 @@ static int cdns_i3c_master_probe(struct platform_device *pdev) writel(MST_INT_IBIR_THR, master->regs + MST_IER); writel(DEVS_CTRL_DEV_CLR_ALL, master->regs + DEVS_CTRL); - ret = i3c_master_register(&master->base, &pdev->dev, - &cdns_i3c_master_ops, false); + ret = i3c_primary_master_register(&master->base, &pdev->dev, + &cdns_i3c_master_ops); if (ret) goto err_disable_sysclk; diff --git a/drivers/i3c/master/mipi-i3c-hci/core.c b/drivers/i3c/master/mipi-i3c-hci/core.c index 113c4c9..b288245 100644 --- a/drivers/i3c/master/mipi-i3c-hci/core.c +++ b/drivers/i3c/master/mipi-i3c-hci/core.c @@ -757,8 +757,8 @@ static int i3c_hci_probe(struct platform_device *pdev) if (ret) return ret; - ret = i3c_master_register(&hci->master, &pdev->dev, - &i3c_hci_ops, false); + ret = i3c_primary_master_register(&hci->master, &pdev->dev, + &i3c_hci_ops); if (ret) return ret; diff --git a/include/linux/i3c/master.h b/include/linux/i3c/master.h index 9cb39d9..e543f68 100644 --- a/include/linux/i3c/master.h +++ b/include/linux/i3c/master.h @@ -538,10 +538,9 @@ int i3c_master_add_i3c_dev_locked(struct i3c_master_controller *master, int i3c_master_set_info(struct i3c_master_controller *master, const struct i3c_device_info *info); -int i3c_master_register(struct i3c_master_controller *master, - struct device *parent, - const struct i3c_master_controller_ops *ops, - bool secondary); +int i3c_primary_master_register(struct i3c_master_controller *master, + struct device *parent, + const struct i3c_master_controller_ops *ops); int i3c_master_unregister(struct i3c_master_controller *master); /**
Removed last argument 'secondary' and restructured i3c_master_register to move code that can be common to i3c_secondary_master_register to separate function i3c_master_init. Signed-off-by: Parshuram Thombare <pthombar@cadence.com> --- drivers/i3c/master.c | 78 +++++++++++++++++++------------- drivers/i3c/master/dw-i3c-master.c | 4 +- drivers/i3c/master/i3c-master-cdns.c | 4 +- drivers/i3c/master/mipi-i3c-hci/core.c | 4 +- include/linux/i3c/master.h | 7 +-- 5 files changed, 56 insertions(+), 41 deletions(-)