Re: [PATCH v6 2/3] i3c: master: Extend address status bit to 4 and add I3C_ADDR_SLOT_EXT_DESIRED

[Miquel Raynal <miquel.raynal@bootlin.com>]

Hi Frank,

> + * However, some I3C targets may request specific addresses (called as "init_dyn_addr"), which is
> + * typically specified by the DT-'s assigned-address property. Lower addresses having higher IBI
> + * priority. If it is available, i3c_bus_get_free_addr() preferably return a free address that is
> + * not in the list of desired addresses (called as "init_dyn_addr"). This allows the device with
> + * the "init_dyn_addr" to switch to its "init_dyn_addr" when it hot-joins the I3C bus. Otherwise,
> + * if the "init_dyn_addr" is already in use by another I3C device, the target device will not be
> + * able to switch to its desired address.
> + *
> + * If the previous step fails, fallback returning one of the remaining unassigned address,
> + * regardless of its state in the desired list.
> + */
>  static int i3c_bus_get_free_addr(struct i3c_bus *bus, u8 start_addr)
>  {
>  	enum i3c_addr_slot_status status;
>  	u8 addr;
>  
> +	for (addr = start_addr; addr < I3C_MAX_ADDR; addr++) {
> +		status = i3c_bus_get_addr_slot_status_mask(bus, addr,
> +							   I3C_ADDR_SLOT_EXT_STATUS_MASK);
> +		if (status == I3C_ADDR_SLOT_FREE)
> +			return addr;
> +	}
> +
>  	for (addr = start_addr; addr < I3C_MAX_ADDR; addr++) {
>  		status = i3c_bus_get_addr_slot_status(bus, addr);

Maybe here we could also use the same status_mask() with the other mask,
just to clarify the difference.

>  		if (status == I3C_ADDR_SLOT_FREE)
> @@ -1918,9 +1959,10 @@ static int i3c_master_bus_init(struct i3c_master_controller *master)
>  			goto err_rstdaa;
>  		}
>  
> -		i3c_bus_set_addr_slot_status(&master->bus,
> -					     i3cboardinfo->init_dyn_addr,
> -					     I3C_ADDR_SLOT_I3C_DEV);
> +		i3c_bus_set_addr_slot_status_mask(&master->bus,
> +						 i3cboardinfo->init_dyn_addr,
> +						 I3C_ADDR_SLOT_I3C_DEV | I3C_ADDR_SLOT_EXT_DESIRED,
> +						 I3C_ADDR_SLOT_EXT_STATUS_MASK);

However I'm not sure I understand the use of the
set_addr_slot_status_mask() function. Can't we just use the normal
function and just extend the mask in the fist place?

>  		/*
>  		 * Only try to create/attach devices that have a static
> diff --git a/include/linux/i3c/master.h b/include/linux/i3c/master.h
> index 2100547b2d8d2..6e5328c6c6afd 100644
> --- a/include/linux/i3c/master.h
> +++ b/include/linux/i3c/master.h
> @@ -298,7 +298,8 @@ enum i3c_open_drain_speed {
>   * @I3C_ADDR_SLOT_I2C_DEV: address is assigned to an I2C device
>   * @I3C_ADDR_SLOT_I3C_DEV: address is assigned to an I3C device
>   * @I3C_ADDR_SLOT_STATUS_MASK: address slot mask
> - *
> + * @I3C_ADDR_SLOT_EXT_DESIRED: the bitmask represents addresses that are preferred by some devices,
> + *			       such as the "assigned-address" property in a device tree source.
>   * On an I3C bus, addresses are assigned dynamically, and we need to know which
>   * addresses are free to use and which ones are already assigned.
>   *
> @@ -311,9 +312,11 @@ enum i3c_addr_slot_status {
>  	I3C_ADDR_SLOT_I2C_DEV,
>  	I3C_ADDR_SLOT_I3C_DEV,
>  	I3C_ADDR_SLOT_STATUS_MASK = 3,
> +	I3C_ADDR_SLOT_EXT_STATUS_MASK = 7,
> +	I3C_ADDR_SLOT_EXT_DESIRED = BIT(2),
>  };
>  
> -#define I3C_ADDR_SLOT_STATUS_BITS 2
> +#define I3C_ADDR_SLOT_STATUS_BITS 4
>  
>  /**
>   * struct i3c_bus - I3C bus object
> 

Otherwise lgtm.

Thanks,
Miquèl