Currently the i2c subsystem rely on the controller device tree to
initialize the pinctrl recovery information, part of the drivers does
not set this field (rinfo->pinctrl), for example i2c designware driver.
The pins information is saved part of the device structure before probe
and it's done on pinctrl_bind_pins().
Make the i2c init recovery to get the device pins if it's not
initialized by the driver from the device pins.
Added new API to get the device pinctrl.
Signed-off-by: Hanna Hawa <[email protected]>
Change Log v2->v3:
- Add API to get the device pinctrl
- Make the i2c init recovery to get the device pins
Change Log v1->v2:
- set the rinfo->pinctrl to dev->pins->p instead calling
devm_pinctrl_get()
---
drivers/i2c/i2c-core-base.c | 7 ++++++-
include/linux/pinctrl/devinfo.h | 11 +++++++++++
2 files changed, 17 insertions(+), 1 deletion(-)
diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index 7539b0740351..17eecdcf1cb2 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -34,6 +34,7 @@
#include <linux/of.h>
#include <linux/of_irq.h>
#include <linux/pinctrl/consumer.h>
+#include <linux/pinctrl/devinfo.h>
#include <linux/pm_domain.h>
#include <linux/pm_runtime.h>
#include <linux/pm_wakeirq.h>
@@ -282,7 +283,11 @@ static void i2c_gpio_init_pinctrl_recovery(struct i2c_adapter *adap)
{
struct i2c_bus_recovery_info *bri = adap->bus_recovery_info;
struct device *dev = &adap->dev;
- struct pinctrl *p = bri->pinctrl;
+ struct pinctrl *p;
+
+ if (!bri->pinctrl)
+ bri->pinctrl = dev_pinctrl(dev->parent);
+ p = bri->pinctrl;
/*
* we can't change states without pinctrl, so remove the states if
diff --git a/include/linux/pinctrl/devinfo.h b/include/linux/pinctrl/devinfo.h
index a48ff69acddd..5c00ee115528 100644
--- a/include/linux/pinctrl/devinfo.h
+++ b/include/linux/pinctrl/devinfo.h
@@ -17,6 +17,7 @@
#ifdef CONFIG_PINCTRL
/* The device core acts as a consumer toward pinctrl */
+#include <linux/device.h>
#include <linux/pinctrl/consumer.h>
/**
@@ -40,6 +41,11 @@ struct dev_pin_info {
extern int pinctrl_bind_pins(struct device *dev);
extern int pinctrl_init_done(struct device *dev);
+static inline struct pinctrl *dev_pinctrl(struct device *dev)
+{
+ return dev->pins && dev->pins->p ? dev->pins->p : NULL;
+}
+
#else
struct device;
@@ -56,5 +62,10 @@ static inline int pinctrl_init_done(struct device *dev)
return 0;
}
+static inline struct pinctrl *get_device_pinctrl(struct device *dev)
+{
+ return NULL;
+}
+
#endif /* CONFIG_PINCTRL */
#endif /* PINCTRL_DEVINFO_H */
--
2.38.1
Hi Hanna,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on wsa/i2c/for-next]
[also build test ERROR on linusw-pinctrl/devel linusw-pinctrl/for-next linus/master v6.1 next-20221220]
[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#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Hanna-Hawa/i2c-Set-pinctrl-recovery-info-to-device-pinctrl/20221220-034335
base: https://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git i2c/for-next
patch link: https://lore.kernel.org/r/20221219193228.35078-1-hhhawa%40amazon.com
patch subject: [PATCH v3 1/1] i2c: Set pinctrl recovery info to device pinctrl
config: arc-vdk_hs38_smp_defconfig
compiler: arc-elf-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/a0e0c7c95aa46232edade3d28e0df8d133eb976b
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Hanna-Hawa/i2c-Set-pinctrl-recovery-info-to-device-pinctrl/20221220-034335
git checkout a0e0c7c95aa46232edade3d28e0df8d133eb976b
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=arc olddefconfig
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=arc SHELL=/bin/bash drivers/i2c/
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <[email protected]>
All error/warnings (new ones prefixed by >>):
drivers/i2c/i2c-core-base.c: In function 'i2c_gpio_init_pinctrl_recovery':
>> drivers/i2c/i2c-core-base.c:289:32: error: implicit declaration of function 'dev_pinctrl'; did you mean 'devm_pinctrl_put'? [-Werror=implicit-function-declaration]
289 | bri->pinctrl = dev_pinctrl(dev->parent);
| ^~~~~~~~~~~
| devm_pinctrl_put
>> drivers/i2c/i2c-core-base.c:289:30: warning: assignment to 'struct pinctrl *' from 'int' makes pointer from integer without a cast [-Wint-conversion]
289 | bri->pinctrl = dev_pinctrl(dev->parent);
| ^
cc1: some warnings being treated as errors
vim +289 drivers/i2c/i2c-core-base.c
281
282 static void i2c_gpio_init_pinctrl_recovery(struct i2c_adapter *adap)
283 {
284 struct i2c_bus_recovery_info *bri = adap->bus_recovery_info;
285 struct device *dev = &adap->dev;
286 struct pinctrl *p;
287
288 if (!bri->pinctrl)
> 289 bri->pinctrl = dev_pinctrl(dev->parent);
290 p = bri->pinctrl;
291
292 /*
293 * we can't change states without pinctrl, so remove the states if
294 * populated
295 */
296 if (!p) {
297 bri->pins_default = NULL;
298 bri->pins_gpio = NULL;
299 return;
300 }
301
302 if (!bri->pins_default) {
303 bri->pins_default = pinctrl_lookup_state(p,
304 PINCTRL_STATE_DEFAULT);
305 if (IS_ERR(bri->pins_default)) {
306 dev_dbg(dev, PINCTRL_STATE_DEFAULT " state not found for GPIO recovery\n");
307 bri->pins_default = NULL;
308 }
309 }
310 if (!bri->pins_gpio) {
311 bri->pins_gpio = pinctrl_lookup_state(p, "gpio");
312 if (IS_ERR(bri->pins_gpio))
313 bri->pins_gpio = pinctrl_lookup_state(p, "recovery");
314
315 if (IS_ERR(bri->pins_gpio)) {
316 dev_dbg(dev, "no gpio or recovery state found for GPIO recovery\n");
317 bri->pins_gpio = NULL;
318 }
319 }
320
321 /* for pinctrl state changes, we need all the information */
322 if (bri->pins_default && bri->pins_gpio) {
323 dev_info(dev, "using pinctrl states for GPIO recovery");
324 } else {
325 bri->pinctrl = NULL;
326 bri->pins_default = NULL;
327 bri->pins_gpio = NULL;
328 }
329 }
330
--
0-DAY CI Kernel Test Service
https://01.org/lkp
Hi Hanna,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on wsa/i2c/for-next]
[also build test WARNING on linusw-pinctrl/devel linusw-pinctrl/for-next linus/master v6.1 next-20221220]
[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#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Hanna-Hawa/i2c-Set-pinctrl-recovery-info-to-device-pinctrl/20221220-034335
base: https://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git i2c/for-next
patch link: https://lore.kernel.org/r/20221219193228.35078-1-hhhawa%40amazon.com
patch subject: [PATCH v3 1/1] i2c: Set pinctrl recovery info to device pinctrl
config: x86_64-randconfig-a002-20221219
compiler: gcc-11 (Debian 11.3.0-8) 11.3.0
reproduce (this is a W=1 build):
# https://github.com/intel-lab-lkp/linux/commit/a0e0c7c95aa46232edade3d28e0df8d133eb976b
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Hanna-Hawa/i2c-Set-pinctrl-recovery-info-to-device-pinctrl/20221220-034335
git checkout a0e0c7c95aa46232edade3d28e0df8d133eb976b
# save the config file
mkdir build_dir && cp config build_dir/.config
make W=1 O=build_dir ARCH=x86_64 olddefconfig
make W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash drivers/i2c/
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <[email protected]>
All warnings (new ones prefixed by >>):
drivers/i2c/i2c-core-base.c: In function 'i2c_gpio_init_pinctrl_recovery':
drivers/i2c/i2c-core-base.c:289:32: error: implicit declaration of function 'dev_pinctrl'; did you mean 'devm_pinctrl_put'? [-Werror=implicit-function-declaration]
289 | bri->pinctrl = dev_pinctrl(dev->parent);
| ^~~~~~~~~~~
| devm_pinctrl_put
>> drivers/i2c/i2c-core-base.c:289:30: warning: assignment to 'struct pinctrl *' from 'int' makes pointer from integer without a cast [-Wint-conversion]
289 | bri->pinctrl = dev_pinctrl(dev->parent);
| ^
cc1: some warnings being treated as errors
vim +289 drivers/i2c/i2c-core-base.c
281
282 static void i2c_gpio_init_pinctrl_recovery(struct i2c_adapter *adap)
283 {
284 struct i2c_bus_recovery_info *bri = adap->bus_recovery_info;
285 struct device *dev = &adap->dev;
286 struct pinctrl *p;
287
288 if (!bri->pinctrl)
> 289 bri->pinctrl = dev_pinctrl(dev->parent);
290 p = bri->pinctrl;
291
292 /*
293 * we can't change states without pinctrl, so remove the states if
294 * populated
295 */
296 if (!p) {
297 bri->pins_default = NULL;
298 bri->pins_gpio = NULL;
299 return;
300 }
301
302 if (!bri->pins_default) {
303 bri->pins_default = pinctrl_lookup_state(p,
304 PINCTRL_STATE_DEFAULT);
305 if (IS_ERR(bri->pins_default)) {
306 dev_dbg(dev, PINCTRL_STATE_DEFAULT " state not found for GPIO recovery\n");
307 bri->pins_default = NULL;
308 }
309 }
310 if (!bri->pins_gpio) {
311 bri->pins_gpio = pinctrl_lookup_state(p, "gpio");
312 if (IS_ERR(bri->pins_gpio))
313 bri->pins_gpio = pinctrl_lookup_state(p, "recovery");
314
315 if (IS_ERR(bri->pins_gpio)) {
316 dev_dbg(dev, "no gpio or recovery state found for GPIO recovery\n");
317 bri->pins_gpio = NULL;
318 }
319 }
320
321 /* for pinctrl state changes, we need all the information */
322 if (bri->pins_default && bri->pins_gpio) {
323 dev_info(dev, "using pinctrl states for GPIO recovery");
324 } else {
325 bri->pinctrl = NULL;
326 bri->pins_default = NULL;
327 bri->pins_gpio = NULL;
328 }
329 }
330
--
0-DAY CI Kernel Test Service
https://01.org/lkp
On 12/19/2022 9:32 PM, Hanna Hawa wrote:
> +static inline struct pinctrl *get_device_pinctrl(struct device *dev)
> +{
> + return NULL;
> +}
Will fix - my mistake didn't replace the function name.
On Mon, Dec 19, 2022 at 07:32:28PM +0000, Hanna Hawa wrote:
> Currently the i2c subsystem rely on the controller device tree to
> initialize the pinctrl recovery information, part of the drivers does
> not set this field (rinfo->pinctrl), for example i2c designware driver.
>
> The pins information is saved part of the device structure before probe
> and it's done on pinctrl_bind_pins().
>
> Make the i2c init recovery to get the device pins if it's not
> initialized by the driver from the device pins.
>
> Added new API to get the device pinctrl.
>
> Signed-off-by: Hanna Hawa <[email protected]>
The same comment about changelog. Place it in the correct position.
> Change Log v2->v3:
> - Add API to get the device pinctrl
> - Make the i2c init recovery to get the device pins
>
> Change Log v1->v2:
> - set the rinfo->pinctrl to dev->pins->p instead calling
> devm_pinctrl_get()
> ---
> include/linux/pinctrl/devinfo.h | 11 +++++++++++
This should be a separate patch.
...
> +static inline struct pinctrl *dev_pinctrl(struct device *dev)
> +{
> + return dev->pins && dev->pins->p ? dev->pins->p : NULL;
GCC supports Elvis, you can use it to simplify the above.
> +}
--
With Best Regards,
Andy Shevchenko
On Mon, Dec 19, 2022 at 07:32:28PM +0000, Hanna Hawa wrote:
> Currently the i2c subsystem rely on the controller device tree to
> initialize the pinctrl recovery information, part of the drivers does
> not set this field (rinfo->pinctrl), for example i2c designware driver.
>
> The pins information is saved part of the device structure before probe
> and it's done on pinctrl_bind_pins().
>
> Make the i2c init recovery to get the device pins if it's not
> initialized by the driver from the device pins.
>
> Added new API to get the device pinctrl.
...
> - struct pinctrl *p = bri->pinctrl;
> + struct pinctrl *p;
> +
> + if (!bri->pinctrl)
> + bri->pinctrl = dev_pinctrl(dev->parent);
> + p = bri->pinctrl;
As I said, you may use Elvis here as well.
bri->pinctrl = bri->pinctrl ?: dev_pinctrl(...);
p = bri->pinctrl;
--
With Best Regards,
Andy Shevchenko
Hi Hanna,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on wsa/i2c/for-next]
[also build test WARNING on linusw-pinctrl/for-next linus/master v6.1 next-20221220]
[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#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Hanna-Hawa/i2c-Set-pinctrl-recovery-info-to-device-pinctrl/20221220-034335
base: https://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git i2c/for-next
patch link: https://lore.kernel.org/r/20221219193228.35078-1-hhhawa%40amazon.com
patch subject: [PATCH v3 1/1] i2c: Set pinctrl recovery info to device pinctrl
config: x86_64-randconfig-a013-20221219
compiler: clang version 14.0.6 (https://github.com/llvm/llvm-project f28c006a5895fc0e329fe15fead81e37457cb1d1)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/a0e0c7c95aa46232edade3d28e0df8d133eb976b
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Hanna-Hawa/i2c-Set-pinctrl-recovery-info-to-device-pinctrl/20221220-034335
git checkout a0e0c7c95aa46232edade3d28e0df8d133eb976b
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=x86_64 olddefconfig
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash drivers/i2c/
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <[email protected]>
All warnings (new ones prefixed by >>):
drivers/i2c/i2c-core-base.c:289:18: error: implicit declaration of function 'dev_pinctrl' is invalid in C99 [-Werror,-Wimplicit-function-declaration]
bri->pinctrl = dev_pinctrl(dev->parent);
^
>> drivers/i2c/i2c-core-base.c:289:16: warning: incompatible integer to pointer conversion assigning to 'struct pinctrl *' from 'int' [-Wint-conversion]
bri->pinctrl = dev_pinctrl(dev->parent);
^ ~~~~~~~~~~~~~~~~~~~~~~~~
1 warning and 1 error generated.
vim +289 drivers/i2c/i2c-core-base.c
281
282 static void i2c_gpio_init_pinctrl_recovery(struct i2c_adapter *adap)
283 {
284 struct i2c_bus_recovery_info *bri = adap->bus_recovery_info;
285 struct device *dev = &adap->dev;
286 struct pinctrl *p;
287
288 if (!bri->pinctrl)
> 289 bri->pinctrl = dev_pinctrl(dev->parent);
290 p = bri->pinctrl;
291
292 /*
293 * we can't change states without pinctrl, so remove the states if
294 * populated
295 */
296 if (!p) {
297 bri->pins_default = NULL;
298 bri->pins_gpio = NULL;
299 return;
300 }
301
302 if (!bri->pins_default) {
303 bri->pins_default = pinctrl_lookup_state(p,
304 PINCTRL_STATE_DEFAULT);
305 if (IS_ERR(bri->pins_default)) {
306 dev_dbg(dev, PINCTRL_STATE_DEFAULT " state not found for GPIO recovery\n");
307 bri->pins_default = NULL;
308 }
309 }
310 if (!bri->pins_gpio) {
311 bri->pins_gpio = pinctrl_lookup_state(p, "gpio");
312 if (IS_ERR(bri->pins_gpio))
313 bri->pins_gpio = pinctrl_lookup_state(p, "recovery");
314
315 if (IS_ERR(bri->pins_gpio)) {
316 dev_dbg(dev, "no gpio or recovery state found for GPIO recovery\n");
317 bri->pins_gpio = NULL;
318 }
319 }
320
321 /* for pinctrl state changes, we need all the information */
322 if (bri->pins_default && bri->pins_gpio) {
323 dev_info(dev, "using pinctrl states for GPIO recovery");
324 } else {
325 bri->pinctrl = NULL;
326 bri->pins_default = NULL;
327 bri->pins_gpio = NULL;
328 }
329 }
330
--
0-DAY CI Kernel Test Service
https://01.org/lkp
On 12/20/2022 12:57 PM, Andy Shevchenko wrote:
> As I said, you may use Elvis here as well.
>
> bri->pinctrl = bri->pinctrl ?: dev_pinctrl(...);
> p = bri->pinctrl;
Will handle in the next patchset. thanks.
Thanks,
Hanna
On 12/20/2022 12:53 PM, Andy Shevchenko wrote:
>> Signed-off-by: Hanna Hawa <[email protected]>
> > The same comment about changelog. Place it in the correct position.
Will be fixed.
>
>> Change Log v2->v3:
>> - Add API to get the device pinctrl
>> - Make the i2c init recovery to get the device pins
>>
>> Change Log v1->v2:
>> - set the rinfo->pinctrl to dev->pins->p instead calling
>> devm_pinctrl_get()
>> ---
>
>> include/linux/pinctrl/devinfo.h | 11 +++++++++++
>
> This should be a separate patch.
Sure, will move and create cover letter with the change log.
>
> ...
>
>> +static inline struct pinctrl *dev_pinctrl(struct device *dev)
>> +{
>> + return dev->pins && dev->pins->p ? dev->pins->p : NULL;
>
> GCC supports Elvis, you can use it to simplify the above.
How you suggest to simplify this?
I can use 'return dev->pins ? dev->pins->p ?: dev->pins->p : NULL;'
>
>> +}
>
> --
> With Best Regards,
> Andy Shevchenko
>
>
On 12/20/2022 9:18 PM, Andy Shevchenko wrote:
>> How you suggest to simplify this?
> Using Elvis operator, which is ?:.
Are you refer to use 'return dev->pins && dev->pins->p ?: NULL;' ?
Can't use Elvis operator in this way, because it will return the result
of 'dev->pins && dev->pins->p' and not the value of 'dev->pins->p'
>
>> I can use 'return dev->pins ? dev->pins->p ?: dev->pins->p : NULL;'
> Have you even try to compile this?
Yup, the code compiled, but i think the first suggestion is more readable.
>
On Tue, Dec 20, 2022 at 07:07:51PM +0200, Hawa, Hanna wrote:
> On 12/20/2022 12:53 PM, Andy Shevchenko wrote:
...
> > > +static inline struct pinctrl *dev_pinctrl(struct device *dev)
> > > +{
> > > + return dev->pins && dev->pins->p ? dev->pins->p : NULL;
> >
> > GCC supports Elvis, you can use it to simplify the above.
>
> How you suggest to simplify this?
Using Elvis operator, which is ?:.
> I can use 'return dev->pins ? dev->pins->p ?: dev->pins->p : NULL;'
Have you even try to compile this?
> > > +}
--
With Best Regards,
Andy Shevchenko
On Tue, Dec 20, 2022 at 09:32:11PM +0200, Hawa, Hanna wrote:
> On 12/20/2022 9:18 PM, Andy Shevchenko wrote:
> > > How you suggest to simplify this?
> > Using Elvis operator, which is ?:.
>
> Are you refer to use 'return dev->pins && dev->pins->p ?: NULL;' ?
> Can't use Elvis operator in this way, because it will return the result of
> 'dev->pins && dev->pins->p' and not the value of 'dev->pins->p'
I see now. Then we need to check pins separately, something like
if (!dev->pins)
return NULL;
return dev->pins->p;
Sorry that I haven't noticed that before.
> > > I can use 'return dev->pins ? dev->pins->p ?: dev->pins->p : NULL;'
> > Have you even try to compile this?
> Yup, the code compiled, but i think the first suggestion is more readable.
--
With Best Regards,
Andy Shevchenko