Re: [PATCH v2 8/9] i2c: atr: add static flag

[Romain Gantois <romain.gantois@bootlin.com>]

[-- Attachment #1: Type: text/plain, Size: 5479 bytes --]

On mardi 25 février 2025 12:39:36 heure normale d’Europe centrale Cosmin 
Tanislav wrote:
> Some I2C ATRs do not support dynamic remapping, only static mapping
> of direct children.
> 
> Add a new flag that prevents old mappings to be replaced or new mappings
> to be created in the alias finding code paths.
> 
> Signed-off-by: Cosmin Tanislav <demonsingur@gmail.com>
> ---
>  drivers/i2c/i2c-atr.c   | 16 ++++++++++++----
>  include/linux/i2c-atr.h | 20 +++++++++++++++++---
>  2 files changed, 29 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/i2c/i2c-atr.c b/drivers/i2c/i2c-atr.c
> index d8748d71ae15..f7b853f55630 100644
> --- a/drivers/i2c/i2c-atr.c
> +++ b/drivers/i2c/i2c-atr.c
> @@ -106,6 +106,7 @@ struct i2c_atr_chan {
>   * @lock:      Lock for the I2C bus segment (see &struct
> i2c_lock_operations) * @lock_key:  Lock key for @lock
>   * @max_adapters: Maximum number of adapters this I2C ATR can have
> + * @flags:     Flags for ATR
>   * @alias_pool: Optional common pool of available client aliases
>   * @i2c_nb:    Notifier for remote client add & del events
>   * @adapter:   Array of adapters
> @@ -122,6 +123,7 @@ struct i2c_atr {
>  	struct mutex lock;
>  	struct lock_class_key lock_key;
>  	int max_adapters;
> +	u32 flags;
> 
>  	struct i2c_atr_alias_pool *alias_pool;
> 
> @@ -339,12 +341,16 @@ i2c_atr_create_mapping_by_addr(struct i2c_atr_chan
> *chan, u16 addr) static struct i2c_atr_alias_pair *
>  i2c_atr_get_mapping_by_addr(struct i2c_atr_chan *chan, u16 addr)
>  {
> +	struct i2c_atr *atr = chan->atr;
>  	struct i2c_atr_alias_pair *c2a;
> 
>  	c2a = i2c_atr_find_mapping_by_addr(chan, addr);
>  	if (c2a)
>  		return c2a;
> 
> +	if (atr->flags & I2C_ATR_STATIC)
> +		return NULL;
> +
>  	c2a = i2c_atr_create_mapping_by_addr(chan, addr);
>  	if (c2a)
>  		return c2a;
> @@ -543,7 +549,7 @@ static int i2c_atr_attach_addr(struct i2c_adapter
> *adapter, mutex_lock(&chan->alias_pairs_lock);
> 
>  	c2a = i2c_atr_create_mapping_by_addr(chan, addr);
> -	if (!c2a)
> +	if (!c2a && !(atr->flags & I2C_ATR_STATIC))
>  		c2a = i2c_atr_replace_mapping_by_addr(chan, addr);
> 
>  	if (!c2a) {
> @@ -702,8 +708,9 @@ static int i2c_atr_parse_alias_pool(struct i2c_atr *atr)
> return ret;
>  }
> 
> -struct i2c_atr *i2c_atr_new(struct i2c_adapter *parent, struct device *dev,
> -			    const struct i2c_atr_ops *ops, int max_adapters)
> +struct i2c_atr *i2c_atr_new_flags(struct i2c_adapter *parent, struct device
> *dev, +				  const struct i2c_atr_ops *ops, int 
max_adapters,
> +				  u32 flags)
>  {
>  	struct i2c_atr *atr;
>  	int ret;
> @@ -725,6 +732,7 @@ struct i2c_atr *i2c_atr_new(struct i2c_adapter *parent,
> struct device *dev, atr->dev = dev;
>  	atr->ops = ops;
>  	atr->max_adapters = max_adapters;
> +	atr->flags = flags;
> 
>  	if (parent->algo->master_xfer)
>  		atr->algo.master_xfer = i2c_atr_master_xfer;
> @@ -752,7 +760,7 @@ struct i2c_atr *i2c_atr_new(struct i2c_adapter *parent,
> struct device *dev,
> 
>  	return ERR_PTR(ret);
>  }
> -EXPORT_SYMBOL_NS_GPL(i2c_atr_new, "I2C_ATR");
> +EXPORT_SYMBOL_NS_GPL(i2c_atr_new_flags, "I2C_ATR");
> 
>  void i2c_atr_delete(struct i2c_atr *atr)
>  {
> diff --git a/include/linux/i2c-atr.h b/include/linux/i2c-atr.h
> index 1c3a5bcd939f..2f79d0d9140f 100644
> --- a/include/linux/i2c-atr.h
> +++ b/include/linux/i2c-atr.h
> @@ -18,6 +18,15 @@ struct device;
>  struct fwnode_handle;
>  struct i2c_atr;
> 
> +/**
> + * enum i2c_atr_flags - Flags for an I2C ATR driver
> + *
> + * @I2C_ATR_STATIC: ATR does not support dynamic mapping, use static
> mapping + */
> +enum i2c_atr_flags {
> +	I2C_ATR_STATIC = BIT(0),

I'd personally prefer the name I2C_ATR_F_STATIC, which would make it explicit 
that this is a flag.

> +};
> +
>  /**
>   * struct i2c_atr_ops - Callbacks from ATR to the device driver.
>   * @attach_addr: Notify the driver of a new device connected on a child
> @@ -60,11 +69,12 @@ struct i2c_atr_adap_desc {
>  };
> 
>  /**
> - * i2c_atr_new() - Allocate and initialize an I2C ATR helper.
> + * i2c_atr_new_flags() - Allocate and initialize an I2C ATR helper.
>   * @parent:       The parent (upstream) adapter
>   * @dev:          The device acting as an ATR
>   * @ops:          Driver-specific callbacks
>   * @max_adapters: Maximum number of child adapters
> + * @flags:        Flags for ATR
>   *
>   * The new ATR helper is connected to the parent adapter but has no child
>   * adapters. Call i2c_atr_add_adapter() to add some.
> @@ -73,8 +83,12 @@ struct i2c_atr_adap_desc {
>   *
>   * Return: pointer to the new ATR helper object, or ERR_PTR
>   */
> -struct i2c_atr *i2c_atr_new(struct i2c_adapter *parent, struct device *dev,
> -			    const struct i2c_atr_ops *ops, int max_adapters);
> +struct i2c_atr *i2c_atr_new_flags(struct i2c_adapter *parent, struct device
> *dev, +				  const struct i2c_atr_ops *ops, int 
max_adapters,
> +				  u32 flags);
> +
> +#define i2c_atr_new(parent, dev, ops, max_adapters) \
> +	i2c_atr_new_flags(parent, dev, ops, max_adapters, 0)

There aren't that many users of i2c_atr_new() yet, so I think it would be 
preferable to just add the "0" parameter to existing callers (including 
FPC202 which this series depends on) rather than adding a layer of 
indirection.

Thanks,

-- 
Romain Gantois, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 833 bytes --]