Re: [PATCH v7 16/24] i2c: allow adapter drivers to override the adapter locking

[Wolfram Sang]

On Wed, Apr 20, 2016 at 05:17:56PM +0200, Peter Rosin wrote:
> Add i2c_lock_bus() and i2c_unlock_bus(), which call the new lock_bus and
> unlock_bus ops in the adapter. These funcs/ops take an additional flags
> argument that indicates for what purpose the adapter is locked.
> 
> There are two flags, I2C_LOCK_ADAPTER and I2C_LOCK_SEGMENT, but they are
> both implemented the same. For now. Locking the adapter means that the
> whole bus is locked, locking the segment means that only the current bus
> segment is locked (i.e. i2c traffic on the parent side of mux is still
> allowed even if the child side of the mux is locked.
> 
> Also support a trylock_bus op (but no function to call it, as it is not
> expected to be needed outside of the i2c core).
> 
> Implement i2c_lock_adapter/i2c_unlock_adapter in terms of the new locking
> scheme (i.e. lock with the I2C_LOCK_ADAPTER flag).
> 
> Annotate some of the locking with explicit I2C_LOCK_SEGMENT flags.

Can you explain a little why it is SEGMENT and not ADAPTER here? That
probably makes it easier to get into this patch.

And to double check my understanding: I was surprised to not see any
i2c_lock_adapter() or I2C_LOCK_ADAPTER in action. This is because muxes
call I2C_LOCK_SEGMENT on their parent which in case of the parent being
the root adapter is essentially the same as I2C_LOCK_ADAPTER. Correct?

> 
> Signed-off-by: Peter Rosin <peda@xxxxxxxxxx>
> ---
>  drivers/i2c/i2c-core.c | 46 ++++++++++++++++++++++++++++------------------
>  include/linux/i2c.h    | 28 ++++++++++++++++++++++++++--
>  2 files changed, 54 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
> index 0f2f8484e8ec..21f46d011c33 100644
> --- a/drivers/i2c/i2c-core.c
> +++ b/drivers/i2c/i2c-core.c
> @@ -960,10 +960,12 @@ static int i2c_check_addr_busy(struct i2c_adapter *adapter, int addr)
>  }
>  
>  /**
> - * i2c_lock_adapter - Get exclusive access to an I2C bus segment
> + * i2c_adapter_lock_bus - Get exclusive access to an I2C bus segment
>   * @adapter: Target I2C bus segment
> + * @flags: I2C_LOCK_ADAPTER locks the root i2c adapter, I2C_LOCK_SEGMENT
> + *	locks only this branch in the adapter tree
>   */

I think this kerneldoc should be moved to i2c_lock_adapter and/or
i2c_lock_bus() which are now in i2c.h. This is what users will use, not
this static, adapter-specific implementation. I think it is enough to
have a comment here explaining what is special in handling adapters.

Thanks,

   Wolfram