Hi,
On Wed, Dec 07, 2022 at 11:22:24AM +0000, Russell King (Oracle) wrote:
> Add fwnode APIs for finding and getting I2C adapters, which will be
> used by the SFP code. These are passed the fwnode corresponding to
> the adapter, and return the I2C adapter. It is the responsibility of
> the caller to find the appropriate fwnode.
>
> We keep the DT and ACPI interfaces, but where appropriate, recode them
> to use the fwnode interfaces internally.
>
> Signed-off-by: Russell King (Oracle) <rmk+kernel@xxxxxxxxxxxxxxx>
Looks good, just few minor comments below. :)
> ---
> drivers/i2c/i2c-core-acpi.c | 13 +------
> drivers/i2c/i2c-core-base.c | 72 +++++++++++++++++++++++++++++++++++++
> drivers/i2c/i2c-core-of.c | 51 ++------------------------
> include/linux/i2c.h | 9 +++++
> 4 files changed, 85 insertions(+), 60 deletions(-)
>
> diff --git a/drivers/i2c/i2c-core-acpi.c b/drivers/i2c/i2c-core-acpi.c
> index 4dd777cc0c89..d6037a328669 100644
> --- a/drivers/i2c/i2c-core-acpi.c
> +++ b/drivers/i2c/i2c-core-acpi.c
> @@ -442,18 +442,7 @@ EXPORT_SYMBOL_GPL(i2c_acpi_find_adapter_by_handle);
>
> static struct i2c_client *i2c_acpi_find_client_by_adev(struct acpi_device *adev)
> {
> - struct device *dev;
> - struct i2c_client *client;
> -
> - dev = bus_find_device_by_acpi_dev(&i2c_bus_type, adev);
> - if (!dev)
> - return NULL;
> -
> - client = i2c_verify_client(dev);
> - if (!client)
> - put_device(dev);
> -
> - return client;
> + return i2c_find_device_by_fwnode(acpi_fwnode_handle(adev));
> }
>
> static int i2c_acpi_notify(struct notifier_block *nb, unsigned long value,
> diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
> index 9aa7b9d9a485..254ec043ce90 100644
> --- a/drivers/i2c/i2c-core-base.c
> +++ b/drivers/i2c/i2c-core-base.c
> @@ -1011,6 +1011,27 @@ void i2c_unregister_device(struct i2c_client *client)
> }
> EXPORT_SYMBOL_GPL(i2c_unregister_device);
>
> +/* must call put_device() when done with returned i2c_client device */
I think proper kernel-doc would be better here and all the exported
functions.
> +struct i2c_client *i2c_find_device_by_fwnode(struct fwnode_handle *fwnode)
> +{
> + struct i2c_client *client;
> + struct device *dev;
> +
> + if (!fwnode)
> + return NULL;
> +
> + dev = bus_find_device_by_fwnode(&i2c_bus_type, fwnode);
> + if (!dev)
> + return NULL;
> +
> + client = i2c_verify_client(dev);
> + if (!client)
> + put_device(dev);
> +
> + return client;
> +}
> +EXPORT_SYMBOL(i2c_find_device_by_fwnode);
> +
Drop this empty line.
>
> static const struct i2c_device_id dummy_id[] = {
> { "dummy", 0 },
> @@ -1761,6 +1782,57 @@ int devm_i2c_add_adapter(struct device *dev, struct i2c_adapter *adapter)
> }
> EXPORT_SYMBOL_GPL(devm_i2c_add_adapter);
>
> +static int i2c_dev_or_parent_fwnode_match(struct device *dev, const void *data)
> +{
> + if (dev_fwnode(dev) == data)
> + return 1;
> +
> + if (dev->parent && dev_fwnode(dev->parent) == data)
> + return 1;
> +
> + return 0;
> +}
> +
> +/* must call put_device() when done with returned i2c_adapter device */
> +struct i2c_adapter *i2c_find_adapter_by_fwnode(struct fwnode_handle *fwnode)
> +{
> + struct i2c_adapter *adapter;
> + struct device *dev;
> +
> + if (!fwnode)
> + return NULL;
> +
> + dev = bus_find_device(&i2c_bus_type, NULL, fwnode,
> + i2c_dev_or_parent_fwnode_match);
> + if (!dev)
> + return NULL;
> +
> + adapter = i2c_verify_adapter(dev);
> + if (!adapter)
> + put_device(dev);
> +
> + return adapter;
> +}
> +EXPORT_SYMBOL(i2c_find_adapter_by_fwnode);
> +
> +/* must call i2c_put_adapter() when done with returned i2c_adapter device */
> +struct i2c_adapter *i2c_get_adapter_by_fwnode(struct fwnode_handle *fwnode)
> +{
> + struct i2c_adapter *adapter;
> +
> + adapter = i2c_find_adapter_by_fwnode(fwnode);
> + if (!adapter)
> + return NULL;
> +
> + if (!try_module_get(adapter->owner)) {
> + put_device(&adapter->dev);
> + adapter = NULL;
> + }
> +
> + return adapter;
> +}
> +EXPORT_SYMBOL(i2c_get_adapter_by_fwnode);
> +
> static void i2c_parse_timing(struct device *dev, char *prop_name, u32 *cur_val_p,
> u32 def_val, bool use_def)
> {
> diff --git a/drivers/i2c/i2c-core-of.c b/drivers/i2c/i2c-core-of.c
> index 3ed74aa4b44b..c3e565e4bddf 100644
> --- a/drivers/i2c/i2c-core-of.c
> +++ b/drivers/i2c/i2c-core-of.c
> @@ -113,69 +113,24 @@ void of_i2c_register_devices(struct i2c_adapter *adap)
> of_node_put(bus);
> }
>
> -static int of_dev_or_parent_node_match(struct device *dev, const void *data)
> -{
> - if (dev->of_node == data)
> - return 1;
> -
> - if (dev->parent)
> - return dev->parent->of_node == data;
> -
> - return 0;
> -}
> -
> /* must call put_device() when done with returned i2c_client device */
> struct i2c_client *of_find_i2c_device_by_node(struct device_node *node)
> {
> - struct device *dev;
> - struct i2c_client *client;
> -
> - dev = bus_find_device_by_of_node(&i2c_bus_type, node);
> - if (!dev)
> - return NULL;
> -
> - client = i2c_verify_client(dev);
> - if (!client)
> - put_device(dev);
> -
> - return client;
> + return i2c_find_device_by_fwnode(of_fwnode_handle(node));
> }
> EXPORT_SYMBOL(of_find_i2c_device_by_node);
>
> /* must call put_device() when done with returned i2c_adapter device */
> struct i2c_adapter *of_find_i2c_adapter_by_node(struct device_node *node)
> {
> - struct device *dev;
> - struct i2c_adapter *adapter;
> -
> - dev = bus_find_device(&i2c_bus_type, NULL, node,
> - of_dev_or_parent_node_match);
> - if (!dev)
> - return NULL;
> -
> - adapter = i2c_verify_adapter(dev);
> - if (!adapter)
> - put_device(dev);
> -
> - return adapter;
> + return i2c_find_adapter_by_fwnode(of_fwnode_handle(node));
> }
> EXPORT_SYMBOL(of_find_i2c_adapter_by_node);
>
> /* must call i2c_put_adapter() when done with returned i2c_adapter device */
> struct i2c_adapter *of_get_i2c_adapter_by_node(struct device_node *node)
> {
> - struct i2c_adapter *adapter;
> -
> - adapter = of_find_i2c_adapter_by_node(node);
> - if (!adapter)
> - return NULL;
> -
> - if (!try_module_get(adapter->owner)) {
> - put_device(&adapter->dev);
> - adapter = NULL;
> - }
> -
> - return adapter;
> + return i2c_get_adapter_by_fwnode(of_fwnode_handle(node));
> }
> EXPORT_SYMBOL(of_get_i2c_adapter_by_node);
>
> diff --git a/include/linux/i2c.h b/include/linux/i2c.h
> index d84e0e99f084..bcee9faaf2e6 100644
> --- a/include/linux/i2c.h
> +++ b/include/linux/i2c.h
> @@ -965,6 +965,15 @@ int i2c_handle_smbus_host_notify(struct i2c_adapter *adap, unsigned short addr);
>
> #endif /* I2C */
>
> +/* must call put_device() when done with returned i2c_client device */
> +struct i2c_client *i2c_find_device_by_fwnode(struct fwnode_handle *fwnode);
With the kernel-docs in place you probably can drop these comments.
> +
> +/* must call put_device() when done with returned i2c_adapter device */
> +struct i2c_adapter *i2c_find_adapter_by_fwnode(struct fwnode_handle *fwnode);
> +
> +/* must call i2c_put_adapter() when done with returned i2c_adapter device */
> +struct i2c_adapter *i2c_get_adapter_by_fwnode(struct fwnode_handle *fwnode);
> +
> #if IS_ENABLED(CONFIG_OF)
> /* must call put_device() when done with returned i2c_client device */
> struct i2c_client *of_find_i2c_device_by_node(struct device_node *node);
> --
> 2.30.2
