[v1,1/6] i2c: acpi: Export i2c_acpi_find_client_by_adev() for users

Message ID 20210526124322.48915-1-andriy.shevchenko@linux.intel.com
State New
Headers show
Series
  • [v1,1/6] i2c: acpi: Export i2c_acpi_find_client_by_adev() for users
Related show

Commit Message

Andy Shevchenko May 26, 2021, 12:43 p.m.
There is at least one user that will gain from the
i2c_acpi_find_client_by_adev() being exported.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/i2c/i2c-core-acpi.c | 3 ++-
 include/linux/i2c.h         | 6 ++++++
 2 files changed, 8 insertions(+), 1 deletion(-)

Comments

Wolfram Sang May 27, 2021, 8:26 p.m. | #1
On Wed, May 26, 2021 at 03:43:17PM +0300, Andy Shevchenko wrote:
> There is at least one user that will gain from the
> i2c_acpi_find_client_by_adev() being exported.

No objections per se. But as the user is in staging, I want to ask if
the use there is really a solution we would also accept outside of
staging? Or is it a hack?
Andy Shevchenko May 28, 2021, 9:23 a.m. | #2
On Thu, May 27, 2021 at 10:26:33PM +0200, Wolfram Sang wrote:
> On Wed, May 26, 2021 at 03:43:17PM +0300, Andy Shevchenko wrote:

> > There is at least one user that will gain from the

> > i2c_acpi_find_client_by_adev() being exported.

> 

> No objections per se. But as the user is in staging, I want to ask if

> the use there is really a solution we would also accept outside of

> staging? Or is it a hack?


The similar OF API is exported for users, although amount of users and their
locations are different. The AtomISP driver is not in the best shape, I agree,
but for now any possible steps to make it better would be good steps in my
opinion. Later we may see if we can do this piece of code differently (IIRC
current way is probably the best taking into account legacy platforms support).

-- 
With Best Regards,
Andy Shevchenko
Andy Shevchenko May 28, 2021, 9:25 a.m. | #3
On Fri, May 28, 2021 at 12:23:31PM +0300, Andy Shevchenko wrote:
> On Thu, May 27, 2021 at 10:26:33PM +0200, Wolfram Sang wrote:

> > On Wed, May 26, 2021 at 03:43:17PM +0300, Andy Shevchenko wrote:

> > > There is at least one user that will gain from the

> > > i2c_acpi_find_client_by_adev() being exported.

> > 

> > No objections per se. But as the user is in staging, I want to ask if

> > the use there is really a solution we would also accept outside of

> > staging? Or is it a hack?

> 

> The similar OF API is exported for users, although amount of users and their

> locations are different. The AtomISP driver is not in the best shape, I agree,

> but for now any possible steps to make it better would be good steps in my

> opinion. Later we may see if we can do this piece of code differently (IIRC

> current way is probably the best taking into account legacy platforms support).


Btw, we may move all current exports from I2C ACPI to its own namespace, then
we won't really care if it'e exported or not, only explicit consumers will use
it.

-- 
With Best Regards,
Andy Shevchenko
Greg Kroah-Hartman May 28, 2021, 9:25 a.m. | #4
On Thu, May 27, 2021 at 10:26:33PM +0200, Wolfram Sang wrote:
> On Wed, May 26, 2021 at 03:43:17PM +0300, Andy Shevchenko wrote:
> > There is at least one user that will gain from the
> > i2c_acpi_find_client_by_adev() being exported.
> 
> No objections per se. But as the user is in staging, I want to ask if
> the use there is really a solution we would also accept outside of
> staging? Or is it a hack?

staging drivers should be self-contained, do not accept code in the core
kernel that only is used by staging drivers.

So I would not recommend this be accepted at this point in time.

Andy, work to get the driver out of staging first before doing stuff
like this.

thanks,

greg k-h
Andy Shevchenko May 28, 2021, 9:34 a.m. | #5
On Fri, May 28, 2021 at 11:25:51AM +0200, Greg Kroah-Hartman wrote:
> On Thu, May 27, 2021 at 10:26:33PM +0200, Wolfram Sang wrote:

> > On Wed, May 26, 2021 at 03:43:17PM +0300, Andy Shevchenko wrote:

> > > There is at least one user that will gain from the

> > > i2c_acpi_find_client_by_adev() being exported.

> > 

> > No objections per se. But as the user is in staging, I want to ask if

> > the use there is really a solution we would also accept outside of

> > staging? Or is it a hack?

> 

> staging drivers should be self-contained, do not accept code in the core

> kernel that only is used by staging drivers.

> 

> So I would not recommend this be accepted at this point in time.


Fair enough.

> Andy, work to get the driver out of staging first before doing stuff

> like this.


Okay, I'll drop first one and patches related to it in the v2.
It should bring us closer to the mentioned point.

Thanks for clarification!

-- 
With Best Regards,
Andy Shevchenko
Wolfram Sang May 28, 2021, 10:01 a.m. | #6
> > staging drivers should be self-contained, do not accept code in the core

> > kernel that only is used by staging drivers.

> > 

> > So I would not recommend this be accepted at this point in time.

> 

> Fair enough.


You could add a comment, though, so we won't forget to remove the open
coding then.
Mauro Carvalho Chehab July 22, 2021, 8:57 a.m. | #7
Hi Andy,

Em Wed, 26 May 2021 15:43:18 +0300
Andy Shevchenko <andriy.shevchenko@linux.intel.com> escreveu:

> gmin_i2c_dev_exists() is using open-coded variant of
> i2c_acpi_find_client_by_adev(). Replace it with a corresponding call.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

At least on the top of v5.14-rc1, this patch causes a compilation
issue:

	drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c: In function ‘gmin_i2c_dev_exists’:
	drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c:386:19: error: implicit declaration of function ‘i2c_acpi_find_client_by_adev’; did you mean ‘i2c_acpi_find_adapter_by_handle’? [-Werror=implicit-function-declaration]
	  386 |         *client = i2c_acpi_find_client_by_adev(adev);
	      |                   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
	      |                   i2c_acpi_find_adapter_by_handle
	drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c:386:17: warning: assignment to ‘struct i2c_client *’ from ‘int’ makes pointer from integer without a cast [-Wint-conversion]
	  386 |         *client = i2c_acpi_find_client_by_adev(adev);
	      |                 ^

The reason is because such function is static:

	$ git grep i2c_acpi_find_client_by_adev
	drivers/i2c/i2c-core-acpi.c:static struct i2c_client *i2c_acpi_find_client_by_adev(struct acpi_device *adev)

IMO, a patch like that should be applied at the same tree as a patch
dropping "static" from drivers/i2c/i2c-core-acpi.c. If you want to do
so, feel free to add:

Reviewed-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>

> ---
>  .../staging/media/atomisp/pci/atomisp_gmin_platform.c    | 9 ++-------
>  1 file changed, 2 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c b/drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c
> index 135994d44802..a1064d1a3d6b 100644
> --- a/drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c
> +++ b/drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c
> @@ -378,19 +378,14 @@ static struct i2c_client *gmin_i2c_dev_exists(struct device *dev, char *name,
>  					      struct i2c_client **client)
>  {
>  	struct acpi_device *adev;
> -	struct device *d;
>  
>  	adev = acpi_dev_get_first_match_dev(name, NULL, -1);
>  	if (!adev)
>  		return NULL;
>  
> -	d = bus_find_device_by_acpi_dev(&i2c_bus_type, adev);
> -	acpi_dev_put(adev);
> -	if (!d)
> -		return NULL;
> +	*client = i2c_acpi_find_client_by_adev(adev);
>  
> -	*client = i2c_verify_client(d);
> -	put_device(d);
> +	acpi_dev_put(adev);
>  
>  	dev_dbg(dev, "found '%s' at address 0x%02x, adapter %d\n",
>  		(*client)->name, (*client)->addr, (*client)->adapter->nr);



Thanks,
Mauro
Andy Shevchenko July 22, 2021, 9:02 a.m. | #8
On Thu, Jul 22, 2021 at 10:57:44AM +0200, Mauro Carvalho Chehab wrote:
> Em Wed, 26 May 2021 15:43:18 +0300

> Andy Shevchenko <andriy.shevchenko@linux.intel.com> escreveu:

> 

> > gmin_i2c_dev_exists() is using open-coded variant of

> > i2c_acpi_find_client_by_adev(). Replace it with a corresponding call.

> > 

> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

> 

> At least on the top of v5.14-rc1, this patch causes a compilation

> issue:

> 

> 	drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c: In function ‘gmin_i2c_dev_exists’:

> 	drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c:386:19: error: implicit declaration of function ‘i2c_acpi_find_client_by_adev’; did you mean ‘i2c_acpi_find_adapter_by_handle’? [-Werror=implicit-function-declaration]

> 	  386 |         *client = i2c_acpi_find_client_by_adev(adev);

> 	      |                   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~

> 	      |                   i2c_acpi_find_adapter_by_handle

> 	drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c:386:17: warning: assignment to ‘struct i2c_client *’ from ‘int’ makes pointer from integer without a cast [-Wint-conversion]

> 	  386 |         *client = i2c_acpi_find_client_by_adev(adev);

> 	      |                 ^

> 

> The reason is because such function is static:

> 

> 	$ git grep i2c_acpi_find_client_by_adev

> 	drivers/i2c/i2c-core-acpi.c:static struct i2c_client *i2c_acpi_find_client_by_adev(struct acpi_device *adev)

> 

> IMO, a patch like that should be applied at the same tree as a patch

> dropping "static" from drivers/i2c/i2c-core-acpi.c. If you want to do

> so, feel free to add:

> 

> Reviewed-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>


Thanks!

There is a v2 of this where the patch is dropped from.


-- 
With Best Regards,
Andy Shevchenko

Patch

diff --git a/drivers/i2c/i2c-core-acpi.c b/drivers/i2c/i2c-core-acpi.c
index 8ceaa88dd78f..5be37a5efcb4 100644
--- a/drivers/i2c/i2c-core-acpi.c
+++ b/drivers/i2c/i2c-core-acpi.c
@@ -387,7 +387,7 @@  struct i2c_adapter *i2c_acpi_find_adapter_by_handle(acpi_handle handle)
 }
 EXPORT_SYMBOL_GPL(i2c_acpi_find_adapter_by_handle);
 
-static struct i2c_client *i2c_acpi_find_client_by_adev(struct acpi_device *adev)
+struct i2c_client *i2c_acpi_find_client_by_adev(struct acpi_device *adev)
 {
 	struct device *dev;
 	struct i2c_client *client;
@@ -402,6 +402,7 @@  static struct i2c_client *i2c_acpi_find_client_by_adev(struct acpi_device *adev)
 
 	return client;
 }
+EXPORT_SYMBOL_GPL(i2c_acpi_find_client_by_adev);
 
 static int i2c_acpi_notify(struct notifier_block *nb, unsigned long value,
 			   void *arg)
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index e8f2ac8c9c3d..335dc4f5abbb 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -995,6 +995,7 @@  static inline int of_i2c_get_board_info(struct device *dev,
 
 #endif /* CONFIG_OF */
 
+struct acpi_device;
 struct acpi_resource;
 struct acpi_resource_i2c_serialbus;
 
@@ -1005,6 +1006,7 @@  u32 i2c_acpi_find_bus_speed(struct device *dev);
 struct i2c_client *i2c_acpi_new_device(struct device *dev, int index,
 				       struct i2c_board_info *info);
 struct i2c_adapter *i2c_acpi_find_adapter_by_handle(acpi_handle handle);
+struct i2c_client *i2c_acpi_find_client_by_adev(struct acpi_device *adev);
 #else
 static inline bool i2c_acpi_get_i2c_resource(struct acpi_resource *ares,
 					     struct acpi_resource_i2c_serialbus **i2c)
@@ -1024,6 +1026,10 @@  static inline struct i2c_adapter *i2c_acpi_find_adapter_by_handle(acpi_handle ha
 {
 	return NULL;
 }
+static inline struct i2c_client *i2c_acpi_find_client_by_adev(struct acpi_device *adev)
+{
+	return NULL;
+}
 #endif /* CONFIG_ACPI */
 
 #endif /* _LINUX_I2C_H */