On Thu, 14 May 2020 18:30:09 +0200
Parshuram Thombare wrote:
> This patch fix following issues.
> 1. Controller slots blocked for devices with static_addr
>but no init_dyn_addr may limit the number of I3C devices
>on the bus which gets dynamic address in DAA. So
>instead of attaching all the devices with static_addr,
>now we only attach the devices which successfully
>complete SETDASA. Remaining devices are handled in DAA.
> 2. Since we alreay handled devices with init_dyn_addr, removed
>it's handling from i3c_master_add_i3c_dev_locked().
>Now only case handled is devices already with dyn_addr
>participated in DAA, and again got new dyn_addr with an
>extra slot in the master controller.
I don't get that one.
> 3. Removed unnecessary i3c_master_reattach_i3c_dev() from
>i3c_master_add_i3c_dev_locked(), right away after finding
>if duplicate device exists in the I3C list.
>In case of different new and old dyn_addr
>i3c_master_reattach_i3c_dev() will fail which is wrong,
>and in case of same dyn_addr it doesn't add anything new.
I think we should fix re-attach instead, which is what we discussed
with Przemek if I remember correctly.
>
Can you please split the patch accordingly (one fix per commit)?
> Fixes: 3a379bbcea0a ("i3c: Add core I3C infrastructure")
> Signed-off-by: Parshuram Thombare
> ---
> drivers/i3c/master.c | 111 ++-
> 1 file changed, 46 insertions(+), 65 deletions(-)
>
> diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
> index 5f4bd52121fe..f1d929b58549 100644
> --- a/drivers/i3c/master.c
> +++ b/drivers/i3c/master.c
> @@ -1375,6 +1375,11 @@ static int i3c_master_reattach_i3c_dev(struct
> i3c_dev_desc *dev,
> i3c_bus_set_addr_slot_status(>bus,
>dev->info.dyn_addr,
>I3C_ADDR_SLOT_I3C_DEV);
> +
> + if (old_dyn_addr)
> + i3c_bus_set_addr_slot_status(>bus,
> + old_dyn_addr,
> + I3C_ADDR_SLOT_FREE);
> }
>
> if (master->ops->reattach_i3c_dev) {
> @@ -1426,33 +1431,52 @@ static void i3c_master_detach_i2c_dev(struct
> i2c_dev_desc *dev)
> master->ops->detach_i2c_dev(dev);
> }
>
> -static void i3c_master_pre_assign_dyn_addr(struct i3c_dev_desc *dev)
> +static void i3c_master_pre_assign_dyn_addr(struct i3c_master_controller
> *master,
> +struct i3c_dev_boardinfo *boardinfo)
> {
> - struct i3c_master_controller *master = i3c_dev_get_master(dev);
> + struct i3c_device_info info = {
> + .static_addr = boardinfo->static_addr,
> + };
> + struct i3c_dev_desc *i3cdev;
> int ret;
>
> - if (!dev->boardinfo || !dev->boardinfo->init_dyn_addr ||
> - !dev->boardinfo->static_addr)
> + /*
> + * We anyway don't attach devices which are not addressable
> + * (no static_addr and dyn_addr) and devices with static_addr
> + * but no init_dyn_addr will participate in DAA.
> + */
> + if (!boardinfo->static_addr || !boardinfo->init_dyn_addr)
> + return;
> +
> + i3cdev = i3c_master_alloc_i3c_dev(master, );
> + if (IS_ERR(i3cdev))
> return;
>
> - ret = i3c_master_setdasa_locked(master, dev->info.static_addr,
> - dev->boardinfo->init_dyn_addr);
> + i3cdev->boardinfo = boardinfo;
> +
> + ret = i3c_master_attach_i3c_dev(master, i3cdev);
> if (ret)
> return;
>
> - dev->info.dyn_addr = dev->boardinfo->init_dyn_addr;
> - ret = i3c_master_reattach_i3c_dev(dev, 0);
> + ret = i3c_master_setdasa_locked(master, i3cdev->info.static_addr,
> + i3cdev->boardinfo->init_dyn_addr);
> if (ret)
> - goto err_rstdaa;
> + goto err_setdasa;
>
> - ret = i3c_master_retrieve_dev_info(dev);
> + i3cdev->info.dyn_addr = i3cdev->boardinfo->init_dyn_addr;
> + ret = i3c_master_reattach_i3c_dev(i3cdev, 0);
> if (ret)
> goto err_rstdaa;
>
> - return;
> + ret = i3c_master_retrieve_dev_info(i3cdev);
> + if (ret)
> + goto err_rstdaa;
>
> err_rstdaa:
> - i3c_master_rstdaa_locked(master, dev->boardinfo->init_dyn_addr);
> + i3c_master_rstdaa_locked(master, i3cdev->boardinfo->init_dyn_addr);
> +err_setdasa:
> + i3c_master_detach_i3c_dev(i3cdev);
> + i3c_master_free_i3c_dev(i3cdev);
> }
>
> static void
> @@ -1619,8 +1643,8 @@ static void i3c_master_detach_free_devs(struct
> i3c_master_controller *master)
> * This function is following all initialisation steps described in the I3C
> * specification:
> *
> - * 1. Attach I2C and statically defined I3C devs to the master so that the
> - *master can fill its
