i2c: acpi: Export i2c_acpi_find_client_by_adev() for users
Subsystems (8)
PATCH 1/6 i2c: acpi: Export i2c_acpi_find_client_by_adev() for users
There is at least one user that will gain from the i2c_acpi_find_client_by_adev() being exported.
--- drivers/i2c/i2c-core-acpi.c | 3 ++- include/linux/i2c.h | 6 ++++++ 2 files changed, 8 insertions(+), 1 deletion(-)
drivers/i2c/i2c-core-acpi.c +2 -1
@@ -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)
include/linux/i2c.h +6 -0
@@ -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 */
Signature · 1 line
2.30.2
PATCH 2/6 staging: atomisp: Replace open-coded i2c_acpi_find_client_by_adev()
gmin_i2c_dev_exists() is using open-coded variant of i2c_acpi_find_client_by_adev(). Replace it with a corresponding call.
--- .../staging/media/atomisp/pci/atomisp_gmin_platform.c | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-)
drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c +2 -7
@@ -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);
Signature · 1 line
2.30.2
Re: [PATCH v1 2/6] staging: atomisp: Replace open-coded i2c_acpi_find_client_by_adev()
Hi Andy,
Em Wed, 26 May 2021 15:43:18 +0300
Andy Shevchenko <[email protected]> escreveu:
Quoted 4 lines
gmin_i2c_dev_exists()is using open-coded variant ofi2c_acpi_find_client_by_adev(). Replace it with a corresponding call.Signed-off-by Andy Shevchenko <[email protected]>
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:
Quoted 20 lines
--- .../staging/media/atomisp/pci/atomisp_gmin_platform.c | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-)drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c +0
-3@@ -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);
drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c static struct i2c_client *gmin_i2c_dev_exists(struct device *dev, char *name, · line 391
Quoted 10 lines
drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c +2
-4-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
Re: [PATCH v1 2/6] staging: atomisp: Replace open-coded i2c_acpi_find_client_by_adev()
Quoted 22 lines · Mauro Carvalho Chehab
Em Wed, 26 May 2021 15:43:18 +0300
Andy Shevchenko <[email protected]> escreveu:Quoted 4 lines
gmin_i2c_dev_exists()is using open-coded variant ofi2c_acpi_find_client_by_adev(). Replace it with a corresponding call.Signed-off-by Andy Shevchenko <[email protected]>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:
Quoted 8 lines
$ 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 <[email protected]>
Thanks!
There is a v2 of this where the patch is dropped from.
Signature · 2 lines
With Best Regards, Andy Shevchenko
PATCH 3/6 staging: atomisp: Remove unused declarations
In reply to Andy ShevchenkoThere is a few static declarations that are not used anywhere, remove them.
--- .../atomisp/pci/isp2400_input_system_local.h | 17 ----------------- 1 file changed, 17 deletions(-)
drivers/staging/media/atomisp/pci/isp2400_input_system_local.h +0 -17
@@ -362,23 +362,6 @@ static const hrt_address MIPI_PORT_OFFSET[N_MIPI_PORT_ID] = {0x00000200UL }; -static const mipi_lane_cfg_t MIPI_PORT_MAXLANES [N_MIPI_PORT_ID ] = { -MIPI_4LANE_CFG , -MIPI_1LANE_CFG , -MIPI_2LANE_CFG -}; - -static const bool MIPI_PORT_ACTIVE [N_RX_MODE ][N_MIPI_PORT_ID ] = { - {true, true, false}, - {true, true, false}, - {true, true, false}, - {true, true, false}, - {true, true, true}, - {true, true, true}, - {true, true, true}, - {true, true, true} -}; -static const mipi_lane_cfg_t MIPI_PORT_LANES [N_RX_MODE ][N_MIPI_PORT_ID ] = { {MIPI_4LANE_CFG ,MIPI_1LANE_CFG ,MIPI_0LANE_CFG }, {MIPI_3LANE_CFG ,MIPI_1LANE_CFG ,MIPI_0LANE_CFG },
Signature · 1 line
2.30.2
PATCH 4/6 staging: atomisp: Annotate a couple of definitions with __maybe_unused
In reply to Andy ShevchenkoThere are definitions in the header that are not used by all modules inside the driver. Annotate them with __maybe_unused to avoid compiler warnings.
--- .../staging/media/atomisp/pci/isp2400_input_system_local.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/staging/media/atomisp/pci/isp2400_input_system_local.h +2 -2
@@ -356,7 +356,7 @@ struct rx_cfg_s { };/* NOTE: The base has already an offset of 0x0100 */ -static const hrt_address MIPI_PORT_OFFSET [N_MIPI_PORT_ID ] = { +static const hrt_address __maybe_unused MIPI_PORT_OFFSET [N_MIPI_PORT_ID ] = {0x00000000UL ,0x00000100UL ,0x00000200UL
@@ -373,7 +373,7 @@ static const mipi_lane_cfg_t MIPI_PORT_LANES[N_RX_MODE][N_MIPI_PORT_ID] = { {MIPI_1LANE_CFG, MIPI_1LANE_CFG, MIPI_1LANE_CFG} }; -static const hrt_address SUB_SYSTEM_OFFSET [N_SUB_SYSTEM_ID ] = { +static const hrt_address __maybe_unused SUB_SYSTEM_OFFSET [N_SUB_SYSTEM_ID ] = {0x00001000UL ,0x00002000UL ,0x00003000UL ,
Signature · 1 line
2.30.2
PATCH 5/6 staging: atomisp: Move MIPI_PORT_LANES to the only user
In reply to Andy ShevchenkoMove MIPI_PORT_LANES to the only user of it, i.e. pci/runtime/isys/src/rx.c.
--- .../media/atomisp/pci/isp2400_input_system_local.h | 11 ----------- .../staging/media/atomisp/pci/runtime/isys/src/rx.c | 11 +++++++++++ 2 files changed, 11 insertions(+), 11 deletions(-)
drivers/staging/media/atomisp/pci/isp2400_input_system_local.h +0 -11
@@ -362,17 +362,6 @@ static const hrt_address __maybe_unused MIPI_PORT_OFFSET[N_MIPI_PORT_ID] = {0x00000200UL }; -static const mipi_lane_cfg_t MIPI_PORT_LANES [N_RX_MODE ][N_MIPI_PORT_ID ] = { - {MIPI_4LANE_CFG ,MIPI_1LANE_CFG ,MIPI_0LANE_CFG }, - {MIPI_3LANE_CFG ,MIPI_1LANE_CFG ,MIPI_0LANE_CFG }, - {MIPI_2LANE_CFG ,MIPI_1LANE_CFG ,MIPI_0LANE_CFG }, - {MIPI_1LANE_CFG ,MIPI_1LANE_CFG ,MIPI_0LANE_CFG }, - {MIPI_2LANE_CFG ,MIPI_1LANE_CFG ,MIPI_2LANE_CFG }, - {MIPI_3LANE_CFG ,MIPI_1LANE_CFG ,MIPI_1LANE_CFG }, - {MIPI_2LANE_CFG ,MIPI_1LANE_CFG ,MIPI_1LANE_CFG }, - {MIPI_1LANE_CFG ,MIPI_1LANE_CFG ,MIPI_1LANE_CFG } -}; -static const hrt_address __maybe_unused SUB_SYSTEM_OFFSET [N_SUB_SYSTEM_ID ] = {0x00001000UL ,0x00002000UL ,
drivers/staging/media/atomisp/pci/runtime/isys/src/rx.c +11 -0
@@ -20,6 +20,17 @@#include "ia_css_irq.h" #include "sh_css_internal.h" +static const mipi_lane_cfg_t MIPI_PORT_LANES [N_RX_MODE ][N_MIPI_PORT_ID ] = { + {MIPI_4LANE_CFG ,MIPI_1LANE_CFG ,MIPI_0LANE_CFG }, + {MIPI_3LANE_CFG ,MIPI_1LANE_CFG ,MIPI_0LANE_CFG }, + {MIPI_2LANE_CFG ,MIPI_1LANE_CFG ,MIPI_0LANE_CFG }, + {MIPI_1LANE_CFG ,MIPI_1LANE_CFG ,MIPI_0LANE_CFG }, + {MIPI_2LANE_CFG ,MIPI_1LANE_CFG ,MIPI_2LANE_CFG }, + {MIPI_3LANE_CFG ,MIPI_1LANE_CFG ,MIPI_1LANE_CFG }, + {MIPI_2LANE_CFG ,MIPI_1LANE_CFG ,MIPI_1LANE_CFG }, + {MIPI_1LANE_CFG ,MIPI_1LANE_CFG ,MIPI_1LANE_CFG } +}; +#if !defined(ISP2401 )void ia_css_isys_rx_enable_all_interrupts (enum mipi_port_id port) {
Signature · 1 line
2.30.2
kernel test robot
build botRe: [PATCH v1 5/6] staging: atomisp: Move MIPI_PORT_LANES to the only user
I love your patch! Yet something to improve:
Hi Andy,
I love your patch! Yet something to improve:
[auto build test ERROR on linuxtv-media/master]
[also build test ERROR on wsa/i2c/for-next v5.13-rc3 next-20210526] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Andy-Shevchenko/i2c-acpi-Export-i2c_acpi_find_client_by_adev-for-users/20210526-204453 base: git://linuxtv.org/media_tree.git master config: i386-allyesconfig (attached as .config) compiler: gcc-9 (Debian 9.3.0-22) 9.3.0 reproduce (this is a W=1 build): # https://github.com/0day-ci/linux/commit/8f95e744d93a29bc02a3e8487a9fbb49e68d38dc git remote add linux-review https://github.com/0day-ci/linux git fetch --no-tags linux-review Andy-Shevchenko/i2c-acpi-Export-i2c_acpi_find_client_by_adev-for-users/20210526-204453 git checkout 8f95e744d93a29bc02a3e8487a9fbb49e68d38dc # save the attached .config to linux build tree make W=1 ARCH=i386
If you fix the issue, kindly add following tag as appropriate
All errors (new ones prefixed by >>):
>> drivers/staging/media/atomisp/pci/runtime/isys/src/rx.c:23:46: error: 'N_RX_MODE' undeclared here (not in a function); did you mean 'N_RX_ID'?
23 | static const mipi_lane_cfg_t MIPI_PORT_LANES[N_RX_MODE][N_MIPI_PORT_ID] = {
| ^~~~~~~~~
| N_RX_ID
drivers/staging/media/atomisp/pci/runtime/isys/src/rx.c:23:30: warning: 'MIPI_PORT_LANES' defined but not used [-Wunused-variable]
23 | static const mipi_lane_cfg_t MIPI_PORT_LANES[N_RX_MODE][N_MIPI_PORT_ID] = {
| ^~~~~~~~~~~~~~~vim +23 drivers/staging/media/atomisp/pci/runtime/isys/src/rx.c
22
> 23 static const mipi_lane_cfg_t MIPI_PORT_LANES[N_RX_MODE][N_MIPI_PORT_ID] = {
24 {MIPI_4LANE_CFG, MIPI_1LANE_CFG, MIPI_0LANE_CFG},
25 {MIPI_3LANE_CFG, MIPI_1LANE_CFG, MIPI_0LANE_CFG},
26 {MIPI_2LANE_CFG, MIPI_1LANE_CFG, MIPI_0LANE_CFG},
27 {MIPI_1LANE_CFG, MIPI_1LANE_CFG, MIPI_0LANE_CFG},
28 {MIPI_2LANE_CFG, MIPI_1LANE_CFG, MIPI_2LANE_CFG},
29 {MIPI_3LANE_CFG, MIPI_1LANE_CFG, MIPI_1LANE_CFG},
30 {MIPI_2LANE_CFG, MIPI_1LANE_CFG, MIPI_1LANE_CFG},
31 {MIPI_1LANE_CFG, MIPI_1LANE_CFG, MIPI_1LANE_CFG}
32 };
33 ---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org
PATCH 6/6 staging: atomisp: Remove unused port_enabled variable
In reply to Andy ShevchenkoRemove unused port_enabled variable in ia_css_isys_rx_configure().
--- drivers/staging/media/atomisp/pci/runtime/isys/src/rx.c | 3 --- 1 file changed, 3 deletions(-)
drivers/staging/media/atomisp/pci/runtime/isys/src/rx.c +0 -3
@@ -490,7 +490,6 @@ unsigned int ia_css_csi2_calculate_input_system_alignment(void ia_css_isys_rx_configure (const rx_cfg_t *config,const enum ia_css_input_mode input_mode) { -bool port_enabled[N_MIPI_PORT_ID ];bool any_port_enabled = false;enum mipi_port_id port;
@@ -527,8 +526,6 @@ void ia_css_isys_rx_configure(const rx_cfg_t *config, _HRT_CSS_RECEIVER_2400_RX_COUNT_REG_IDX, config->rxcount); - port_enabled[port] = true; -if (input_mode !=IA_CSS_INPUT_MODE_BUFFERED_SENSOR ) {/* MW: A bit of a hack, straight wiring of the capture * units,assuming they are linearly enumerated. */
Signature · 1 line
2.30.2
Quoted 2 lines · Andy Shevchenko
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?
Quoted 7 lines · Wolfram Sang
Quoted 2 lines · Andy Shevchenko
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).
Signature · 2 lines
With Best Regards, Andy Shevchenko
Quoted 9 lines · Andy Shevchenko
Quoted 7 lines · Wolfram Sang
Quoted 2 lines · Andy Shevchenko
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?
Quoted 5 lines
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.
Signature · 2 lines
With Best Regards, Andy Shevchenko
Greg Kroah-Hartman
maintainerQuoted 7 lines · Wolfram Sang
Quoted 2 lines · Andy Shevchenko
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
Quoted 5 lines · Greg Kroah-Hartman
Quoted 4 lines · Wolfram Sang
Quoted 2 lines · Andy Shevchenko
There is at least one user that will gain from the
i2c_acpi_find_client_by_adev()being exported.
Quoted 8 lines
Quoted 3 lines
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.
Quoted 2 lines
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!
Signature · 2 lines
With Best Regards, Andy Shevchenko
Quoted 7 lines
Quoted 5 lines
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.