diff mbox series

[v5,1/8] i2c: core: let adapters be notified of client attach/detach

Message ID 20221208104006.316606-2-tomi.valkeinen@ideasonboard.com
State New
Headers
Series i2c-atr and FPDLink |

Commit Message

Tomi Valkeinen Dec. 8, 2022, 10:39 a.m. UTC 
  From: Luca Ceresoli <luca@lucaceresoli.net>

An adapter might need to know when a new device is about to be
added. This will soon bee needed to implement an "I2C address
translator" (ATR for short), a device that propagates I2C transactions
with a different slave address (an "alias" address). An ATR driver
needs to know when a slave is being added to find a suitable alias and
program the device translation map.

Add an attach/detach callback pair to allow adapter drivers to be
notified of clients being added and removed.

Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net>
Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
---
 drivers/i2c/i2c-core-base.c | 18 +++++++++++++++++-
 include/linux/i2c.h         | 16 ++++++++++++++++
 2 files changed, 33 insertions(+), 1 deletion(-)

Comments

Andy Shevchenko Dec. 8, 2022, 12:30 p.m. UTC | #1 
On Thu, Dec 08, 2022 at 12:39:59PM +0200, Tomi Valkeinen wrote:
> From: Luca Ceresoli <luca@lucaceresoli.net>
> 
> An adapter might need to know when a new device is about to be
> added. This will soon bee needed to implement an "I2C address
> translator" (ATR for short), a device that propagates I2C transactions
> with a different slave address (an "alias" address). An ATR driver
> needs to know when a slave is being added to find a suitable alias and
> program the device translation map.
> 
> Add an attach/detach callback pair to allow adapter drivers to be
> notified of clients being added and removed.

...

> +	if (adap->attach_ops &&
> +	    adap->attach_ops->attach_client &&
> +	    adap->attach_ops->attach_client(adap, info, client) != 0)
> +		goto out_remove_swnode;

With a temporary variable it becomes better

	... *ops = adap->attach_ops;

	if (ops && ops->attach_client && ops->attach_client(adap, info, client))


Also notice drop of unneeded ' != 0' part.


>  	status = device_register(&client->dev);
>  	if (status)
> -		goto out_remove_swnode;
> +		goto out_detach_client;
>  
>  	dev_dbg(&adap->dev, "client [%s] registered with bus id %s\n",
>  		client->name, dev_name(&client->dev));
>  
>  	return client;
>  
> +out_detach_client:
> +	if (adap->attach_ops && adap->attach_ops->detach_client)
> +		adap->attach_ops->detach_client(adap, client);

In the similar way.

...

> +	if (adap->attach_ops &&
> +	    adap->attach_ops->detach_client)
> +		adap->attach_ops->detach_client(adap, client);

In the similar way.
Tomi Valkeinen Dec. 8, 2022, 4:10 p.m. UTC | #2 
On 08/12/2022 14:30, Andy Shevchenko wrote:
> On Thu, Dec 08, 2022 at 12:39:59PM +0200, Tomi Valkeinen wrote:
>> From: Luca Ceresoli <luca@lucaceresoli.net>
>>
>> An adapter might need to know when a new device is about to be
>> added. This will soon bee needed to implement an "I2C address
>> translator" (ATR for short), a device that propagates I2C transactions
>> with a different slave address (an "alias" address). An ATR driver
>> needs to know when a slave is being added to find a suitable alias and
>> program the device translation map.
>>
>> Add an attach/detach callback pair to allow adapter drivers to be
>> notified of clients being added and removed.
> 
> ...
> 
>> +	if (adap->attach_ops &&
>> +	    adap->attach_ops->attach_client &&
>> +	    adap->attach_ops->attach_client(adap, info, client) != 0)
>> +		goto out_remove_swnode;
> 
> With a temporary variable it becomes better
>

Ok.

  Tomi
Laurent Pinchart Dec. 11, 2022, 4:55 p.m. UTC | #3 
Hi Tomi and Luca,

Thank you for the patch.

On Thu, Dec 08, 2022 at 12:39:59PM +0200, Tomi Valkeinen wrote:
> From: Luca Ceresoli <luca@lucaceresoli.net>
> 
> An adapter might need to know when a new device is about to be
> added. This will soon bee needed to implement an "I2C address
> translator" (ATR for short), a device that propagates I2C transactions
> with a different slave address (an "alias" address). An ATR driver
> needs to know when a slave is being added to find a suitable alias and
> program the device translation map.
> 
> Add an attach/detach callback pair to allow adapter drivers to be
> notified of clients being added and removed.

This may be a stupid question, but couldn't you instead use the
BUS_NOTIFY_ADD_DEVICE and BUS_NOTIFY_DEL_DEVICE bus notifiers ?

> Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net>
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> ---
>  drivers/i2c/i2c-core-base.c | 18 +++++++++++++++++-
>  include/linux/i2c.h         | 16 ++++++++++++++++
>  2 files changed, 33 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
> index b4edf10e8fd0..c8bc71b1db82 100644
> --- a/drivers/i2c/i2c-core-base.c
> +++ b/drivers/i2c/i2c-core-base.c
> @@ -966,15 +966,23 @@ i2c_new_client_device(struct i2c_adapter *adap, struct i2c_board_info const *inf
>  		}
>  	}
>  
> +	if (adap->attach_ops &&
> +	    adap->attach_ops->attach_client &&
> +	    adap->attach_ops->attach_client(adap, info, client) != 0)
> +		goto out_remove_swnode;
> +
>  	status = device_register(&client->dev);
>  	if (status)
> -		goto out_remove_swnode;
> +		goto out_detach_client;
>  
>  	dev_dbg(&adap->dev, "client [%s] registered with bus id %s\n",
>  		client->name, dev_name(&client->dev));
>  
>  	return client;
>  
> +out_detach_client:
> +	if (adap->attach_ops && adap->attach_ops->detach_client)
> +		adap->attach_ops->detach_client(adap, client);
>  out_remove_swnode:
>  	device_remove_software_node(&client->dev);
>  out_err_put_of_node:
> @@ -996,9 +1004,17 @@ EXPORT_SYMBOL_GPL(i2c_new_client_device);
>   */
>  void i2c_unregister_device(struct i2c_client *client)
>  {
> +	struct i2c_adapter *adap;
> +
>  	if (IS_ERR_OR_NULL(client))
>  		return;
>  
> +	adap = client->adapter;
> +
> +	if (adap->attach_ops &&
> +	    adap->attach_ops->detach_client)
> +		adap->attach_ops->detach_client(adap, client);
> +
>  	if (client->dev.of_node) {
>  		of_node_clear_flag(client->dev.of_node, OF_POPULATED);
>  		of_node_put(client->dev.of_node);
> diff --git a/include/linux/i2c.h b/include/linux/i2c.h
> index f7c49bbdb8a1..9a385b6de388 100644
> --- a/include/linux/i2c.h
> +++ b/include/linux/i2c.h
> @@ -584,6 +584,21 @@ struct i2c_lock_operations {
>  	void (*unlock_bus)(struct i2c_adapter *adapter, unsigned int flags);
>  };
>  
> +/**
> + * struct i2c_attach_operations - callbacks to notify client attach/detach
> + * @attach_client: Notify of new client being attached
> + * @detach_client: Notify of new client being detached
> + *
> + * Both ops are optional.
> + */
> +struct i2c_attach_operations {
> +	int  (*attach_client)(struct i2c_adapter *adapter,
> +			      const struct i2c_board_info *info,
> +			      const struct i2c_client *client);
> +	void (*detach_client)(struct i2c_adapter *adapter,
> +			      const struct i2c_client *client);
> +};
> +
>  /**
>   * struct i2c_timings - I2C timing information
>   * @bus_freq_hz: the bus frequency in Hz
> @@ -726,6 +741,7 @@ struct i2c_adapter {
>  
>  	/* data fields that are valid for all devices	*/
>  	const struct i2c_lock_operations *lock_ops;
> +	const struct i2c_attach_operations *attach_ops;
>  	struct rt_mutex bus_lock;
>  	struct rt_mutex mux_lock;
>
Luca Ceresoli Dec. 19, 2022, 8:51 a.m. UTC | #4 
Hello Laurent,

thanks for the feedback and apologies for the delayed reply.

On Sun, 11 Dec 2022 18:55:39 +0200
Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote:

> Hi Tomi and Luca,
> 
> Thank you for the patch.
> 
> On Thu, Dec 08, 2022 at 12:39:59PM +0200, Tomi Valkeinen wrote:
> > From: Luca Ceresoli <luca@lucaceresoli.net>
> > 
> > An adapter might need to know when a new device is about to be
> > added. This will soon bee needed to implement an "I2C address
> > translator" (ATR for short), a device that propagates I2C transactions
> > with a different slave address (an "alias" address). An ATR driver
> > needs to know when a slave is being added to find a suitable alias and
> > program the device translation map.
> > 
> > Add an attach/detach callback pair to allow adapter drivers to be
> > notified of clients being added and removed.  
> 
> This may be a stupid question, but couldn't you instead use the
> BUS_NOTIFY_ADD_DEVICE and BUS_NOTIFY_DEL_DEVICE bus notifiers ?

I'm not sure they would be the correct tool for this task. Bus
notifiers inform about new events on the 'struct bus_type, i.e. any
event on the global i2c bus type. In the i2c world this means being
notified about new _adapters_, which is exactly what
drivers/i2c/i2c-dev.c does.

Here, however, we need to be informed about new _clients_ being added
under a specific adapter. I'm not sure whether the bus notifiers can
inform about new clients in addition of new adapters, but they at least
seem unable to provide per-adapter notification.

Does that seem correct?

Best regards,
Andy Shevchenko Dec. 19, 2022, 9:48 a.m. UTC | #5 
On Mon, Dec 19, 2022 at 09:51:43AM +0100, Luca Ceresoli wrote:
> On Sun, 11 Dec 2022 18:55:39 +0200
> Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote:
> > On Thu, Dec 08, 2022 at 12:39:59PM +0200, Tomi Valkeinen wrote:

...

> > This may be a stupid question, but couldn't you instead use the
> > BUS_NOTIFY_ADD_DEVICE and BUS_NOTIFY_DEL_DEVICE bus notifiers ?
> 
> I'm not sure they would be the correct tool for this task. Bus
> notifiers inform about new events on the 'struct bus_type, i.e. any
> event on the global i2c bus type. In the i2c world this means being
> notified about new _adapters_, which is exactly what
> drivers/i2c/i2c-dev.c does.
> 
> Here, however, we need to be informed about new _clients_ being added
> under a specific adapter.

This is for example exactly what ACPI integration in I2C framework does. But...

> I'm not sure whether the bus notifiers can
> inform about new clients in addition of new adapters, but they at least
> seem unable to provide per-adapter notification.

...personally I don't like notifiers, they looks like overkill for this task.

> Does that seem correct?
Laurent Pinchart Dec. 26, 2022, 4:54 p.m. UTC | #6 
On Mon, Dec 19, 2022 at 11:48:51AM +0200, Andy Shevchenko wrote:
> On Mon, Dec 19, 2022 at 09:51:43AM +0100, Luca Ceresoli wrote:
> > On Sun, 11 Dec 2022 18:55:39 +0200 Laurent Pinchart wrote:
> > > On Thu, Dec 08, 2022 at 12:39:59PM +0200, Tomi Valkeinen wrote:
> 
> ...
> 
> > > This may be a stupid question, but couldn't you instead use the
> > > BUS_NOTIFY_ADD_DEVICE and BUS_NOTIFY_DEL_DEVICE bus notifiers ?
> > 
> > I'm not sure they would be the correct tool for this task. Bus
> > notifiers inform about new events on the 'struct bus_type, i.e. any
> > event on the global i2c bus type. In the i2c world this means being
> > notified about new _adapters_, which is exactly what
> > drivers/i2c/i2c-dev.c does.
> > 
> > Here, however, we need to be informed about new _clients_ being added
> > under a specific adapter.
> 
> This is for example exactly what ACPI integration in I2C framework does. But...
> 
> > I'm not sure whether the bus notifiers can
> > inform about new clients in addition of new adapters, but they at least
> > seem unable to provide per-adapter notification.
> 
> ...personally I don't like notifiers, they looks like overkill for this task.

But isn't this patch essentially implementing a custom notification
system ? If we need notifiers, why would it be better to add an ad-hoc
API for I2C instead of using bus notifiers ?

> > Does that seem correct?
Andy Shevchenko Dec. 27, 2022, 8:07 p.m. UTC | #7 
On Mon, Dec 26, 2022 at 06:54:37PM +0200, Laurent Pinchart wrote:
> On Mon, Dec 19, 2022 at 11:48:51AM +0200, Andy Shevchenko wrote:
> > On Mon, Dec 19, 2022 at 09:51:43AM +0100, Luca Ceresoli wrote:
> > > On Sun, 11 Dec 2022 18:55:39 +0200 Laurent Pinchart wrote:
> > > > On Thu, Dec 08, 2022 at 12:39:59PM +0200, Tomi Valkeinen wrote:

...

> > > > This may be a stupid question, but couldn't you instead use the
> > > > BUS_NOTIFY_ADD_DEVICE and BUS_NOTIFY_DEL_DEVICE bus notifiers ?
> > > 
> > > I'm not sure they would be the correct tool for this task. Bus
> > > notifiers inform about new events on the 'struct bus_type, i.e. any
> > > event on the global i2c bus type. In the i2c world this means being
> > > notified about new _adapters_, which is exactly what
> > > drivers/i2c/i2c-dev.c does.
> > > 
> > > Here, however, we need to be informed about new _clients_ being added
> > > under a specific adapter.
> > 
> > This is for example exactly what ACPI integration in I2C framework does. But...
> > 
> > > I'm not sure whether the bus notifiers can
> > > inform about new clients in addition of new adapters, but they at least
> > > seem unable to provide per-adapter notification.
> > 
> > ...personally I don't like notifiers, they looks like overkill for this task.
> 
> But isn't this patch essentially implementing a custom notification
> system ? If we need notifiers, why would it be better to add an ad-hoc
> API for I2C instead of using bus notifiers ?

Notifiers (as implemented in the Linux kernel) have some drawbacks IIUC.
For example, it's not easy to pass the data over it. Another example is
the context (you wouldn't know when the notifier be called and if it can
hold some locks or not).

That said, each case should be discussed individually despite the generic
approach being present (i.o.w. do not consider notifiers as a silver
bullet in _any_ notification scheme).

> > > Does that seem correct?
diff mbox series

Patch

diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index b4edf10e8fd0..c8bc71b1db82 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -966,15 +966,23 @@  i2c_new_client_device(struct i2c_adapter *adap, struct i2c_board_info const *inf
 		}
 	}
 
+	if (adap->attach_ops &&
+	    adap->attach_ops->attach_client &&
+	    adap->attach_ops->attach_client(adap, info, client) != 0)
+		goto out_remove_swnode;
+
 	status = device_register(&client->dev);
 	if (status)
-		goto out_remove_swnode;
+		goto out_detach_client;
 
 	dev_dbg(&adap->dev, "client [%s] registered with bus id %s\n",
 		client->name, dev_name(&client->dev));
 
 	return client;
 
+out_detach_client:
+	if (adap->attach_ops && adap->attach_ops->detach_client)
+		adap->attach_ops->detach_client(adap, client);
 out_remove_swnode:
 	device_remove_software_node(&client->dev);
 out_err_put_of_node:
@@ -996,9 +1004,17 @@  EXPORT_SYMBOL_GPL(i2c_new_client_device);
  */
 void i2c_unregister_device(struct i2c_client *client)
 {
+	struct i2c_adapter *adap;
+
 	if (IS_ERR_OR_NULL(client))
 		return;
 
+	adap = client->adapter;
+
+	if (adap->attach_ops &&
+	    adap->attach_ops->detach_client)
+		adap->attach_ops->detach_client(adap, client);
+
 	if (client->dev.of_node) {
 		of_node_clear_flag(client->dev.of_node, OF_POPULATED);
 		of_node_put(client->dev.of_node);
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index f7c49bbdb8a1..9a385b6de388 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -584,6 +584,21 @@  struct i2c_lock_operations {
 	void (*unlock_bus)(struct i2c_adapter *adapter, unsigned int flags);
 };
 
+/**
+ * struct i2c_attach_operations - callbacks to notify client attach/detach
+ * @attach_client: Notify of new client being attached
+ * @detach_client: Notify of new client being detached
+ *
+ * Both ops are optional.
+ */
+struct i2c_attach_operations {
+	int  (*attach_client)(struct i2c_adapter *adapter,
+			      const struct i2c_board_info *info,
+			      const struct i2c_client *client);
+	void (*detach_client)(struct i2c_adapter *adapter,
+			      const struct i2c_client *client);
+};
+
 /**
  * struct i2c_timings - I2C timing information
  * @bus_freq_hz: the bus frequency in Hz
@@ -726,6 +741,7 @@  struct i2c_adapter {
 
 	/* data fields that are valid for all devices	*/
 	const struct i2c_lock_operations *lock_ops;
+	const struct i2c_attach_operations *attach_ops;
 	struct rt_mutex bus_lock;
 	struct rt_mutex mux_lock;