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 <pthom...@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
