| Message ID | 20211207162457.18450-1-andriy.shevchenko@linux.intel.com |
|---|---|
| State | Deferred |
| Headers | show |
| Series | [v1,1/1] i2c: core: Do not dereference fwnode in struct device | expand |
> client->dev.parent = &client->adapter->dev; > client->dev.bus = &i2c_bus_type; > client->dev.type = &i2c_client_type; > - client->dev.of_node = of_node_get(info->of_node); > - client->dev.fwnode = info->fwnode; > > device_enable_async_suspend(&client->dev); > i2c_dev_set_name(adap, client, info); > > + device_set_node(&client->dev, info->fwnode); > + client->dev.of_node = of_node_get(info->of_node); > + I am basically OK with this change. I'd just move the code block a little to have the same behaviour as before. Something like this (hand-edited preview version): > client->dev.bus = &i2c_bus_type; > client->dev.type = &i2c_client_type; > client->dev.of_node = of_node_get(info->of_node); > - client->dev.fwnode = info->fwnode; > > + device_set_node(&client->dev, info->fwnode); > device_enable_async_suspend(&client->dev); > i2c_dev_set_name(adap, client, info); Are you okay with that?
On Thu, Dec 09, 2021 at 09:44:49AM +0100, Wolfram Sang wrote: > > > client->dev.parent = &client->adapter->dev; > > client->dev.bus = &i2c_bus_type; > > client->dev.type = &i2c_client_type; > > - client->dev.of_node = of_node_get(info->of_node); > > - client->dev.fwnode = info->fwnode; > > > > device_enable_async_suspend(&client->dev); > > i2c_dev_set_name(adap, client, info); > > > > + device_set_node(&client->dev, info->fwnode); > > + client->dev.of_node = of_node_get(info->of_node); > > + > > I am basically OK with this change. I'd just move the code block a > little to have the same behaviour as before. Something like this > (hand-edited preview version): And this is exactly what have been done. > > client->dev.bus = &i2c_bus_type; > > client->dev.type = &i2c_client_type; > > client->dev.of_node = of_node_get(info->of_node); > > - client->dev.fwnode = info->fwnode; > > > > + device_set_node(&client->dev, info->fwnode); > > device_enable_async_suspend(&client->dev); > > i2c_dev_set_name(adap, client, info); > > Are you okay with that? It will be broken in this case. There is subtle detail about device_set_node(), it sets both fwnode and of_node at once. More thinking about this, I admit that the best strategy now is to drop this change for now.
diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c index 80f36709e36a..e685a46fd07a 100644 --- a/drivers/i2c/i2c-core-base.c +++ b/drivers/i2c/i2c-core-base.c @@ -1049,12 +1049,13 @@ i2c_new_client_device(struct i2c_adapter *adap, struct i2c_board_info const *inf client->dev.parent = &client->adapter->dev; client->dev.bus = &i2c_bus_type; client->dev.type = &i2c_client_type; - client->dev.of_node = of_node_get(info->of_node); - client->dev.fwnode = info->fwnode; device_enable_async_suspend(&client->dev); i2c_dev_set_name(adap, client, info); + device_set_node(&client->dev, info->fwnode); + client->dev.of_node = of_node_get(info->of_node); + if (info->swnode) { status = device_add_software_node(&client->dev, info->swnode); if (status) {
In order to make the underneath API easier to change in the future, prevent users from dereferencing fwnode from struct device. Instead, use the specific device_set_node() API for that. Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> --- drivers/i2c/i2c-core-base.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)