| Message ID | 20221208104006.316606-2-tomi.valkeinen@ideasonboard.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | i2c-atr and FPDLink | expand |
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.
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
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; >
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,
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?
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?
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 --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;