On Fri, 17 Apr 2020 18:21:02 +0200
Parshuram Thombare <pthom...@cadence.com> wrote:
> +
> +/* This function is expected to be called with normaluse_lock */
> +int i3c_master_acquire_bus(struct i3c_master_controller *master)
> +{
> + int ret = 0;
> +
> + if (!master->this || master->this != master->bus.cur_master) {
Let's limit the number of indentations by doing
if (master->this == master->bus.cur_master)
return 0;
> + if (master->mr_state == I3C_MR_IDLE) {
> + master->mr_state = I3C_MR_WAIT_DA;
> + init_completion(&master->mr_comp);
> + queue_work(master->wq, &master->sec_mst_work);
> + /*
> + * Bus acquire procedure may need write lock
> + * so release read lock before yielding
> + * to bus acquire state machine
> + */
> + i3c_bus_normaluse_unlock(&master->bus);
> + wait_for_completion(&master->mr_comp);
> + i3c_bus_normaluse_lock(&master->bus);
Is that really enough? I remember we had something a bit more complex
to handle the case where bus is acquired to send a message to a device,
and another master on the bus re-acquires it before we have a chance to
send this message message. i3c_master_acquire_bus_ownership() was
dealing with that in Przemek series. It seems you've rewritten a lot of
these things. Would you mind explaining why, and how that works?
> + if (master->mr_state != I3C_MR_DONE)
> + ret = -EAGAIN;
> + master->mr_state = I3C_MR_IDLE;
> + } else {
> + /*
> + * MR request is already in process for
> + * this master
> + */
> + ret = -EAGAIN;
> + }
> + }
> +
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(i3c_master_acquire_bus);
