Re: [PATCH RFC 1/2] i2c: add fwnode APIs

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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



[Index of Archives]     [Linux IBM ACPI]     [Linux Power Management]     [Linux Kernel]     [Linux Laptop]     [Kernel Newbies]     [Share Photos]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Video 4 Linux]     [Device Mapper]     [Linux Resources]
  Powered by Linux