The current implementation of designware recovery mechanism fit for
specific device (Intel / Altera Cyclone V SOC) which have two separated
"wired" GPIOs to the i2c bus via the SOC FPGA for the i2c recovery.
Make pinctrl recovery information to points to the device pinctrl by
setting the rinfo->pinctrl to dev->pins->p.
Change Log v1->v2:
- set the rinfo->pinctrl to dev->pins->p instead calling
devm_pinctrl_get().
Signed-off-by: Hanna Hawa <[email protected]>
---
drivers/i2c/busses/i2c-designware-master.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/drivers/i2c/busses/i2c-designware-master.c b/drivers/i2c/busses/i2c-designware-master.c
index dc3c5a15a95b..16a4cd68567c 100644
--- a/drivers/i2c/busses/i2c-designware-master.c
+++ b/drivers/i2c/busses/i2c-designware-master.c
@@ -17,6 +17,7 @@
#include <linux/interrupt.h>
#include <linux/io.h>
#include <linux/module.h>
+#include <linux/pinctrl/devinfo.h>
#include <linux/pm_runtime.h>
#include <linux/regmap.h>
#include <linux/reset.h>
@@ -832,6 +833,9 @@ static int i2c_dw_init_recovery_info(struct dw_i2c_dev *dev)
struct i2c_adapter *adap = &dev->adapter;
struct gpio_desc *gpio;
+ if (dev->dev->pins && dev->dev->pins->p)
+ rinfo->pinctrl = dev->dev->pins->p;
+
gpio = devm_gpiod_get_optional(dev->dev, "scl", GPIOD_OUT_HIGH);
if (IS_ERR_OR_NULL(gpio))
return PTR_ERR_OR_ZERO(gpio);
--
2.38.1
On Wed, Dec 14, 2022 at 02:27:25PM +0000, Hanna Hawa wrote:
> The current implementation of designware recovery mechanism fit for
> specific device (Intel / Altera Cyclone V SOC) which have two separated
> "wired" GPIOs to the i2c bus via the SOC FPGA for the i2c recovery.
>
> Make pinctrl recovery information to points to the device pinctrl by
> setting the rinfo->pinctrl to dev->pins->p.
> Change Log v1->v2:
> - set the rinfo->pinctrl to dev->pins->p instead calling
> devm_pinctrl_get().
Wrong place for a changelog...
> Signed-off-by: Hanna Hawa <[email protected]>
> ---
...should be located here.
...
> + if (dev->dev->pins && dev->dev->pins->p)
> + rinfo->pinctrl = dev->dev->pins->p;
Hmm... I don't see how this field is being used.
Can you elaborate?
--
With Best Regards,
Andy Shevchenko
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 linus/master v6.1 next-20221214]
[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-designware-set-pinctrl-recovery-information-from-device-pinctrl/20221214-222910
base: https://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git i2c/for-next
patch link: https://lore.kernel.org/r/20221214142725.23881-1-hhhawa%40amazon.com
patch subject: [PATCH v2 1/1] i2c: designware: set pinctrl recovery information from device pinctrl
config: sparc-buildonly-randconfig-r005-20221214
compiler: sparc-linux-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/6e428843e5a6779565aae5f37fe0093ad526f139
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Hanna-Hawa/i2c-designware-set-pinctrl-recovery-information-from-device-pinctrl/20221214-222910
git checkout 6e428843e5a6779565aae5f37fe0093ad526f139
# 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=sparc SHELL=/bin/bash
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <[email protected]>
All errors (new ones prefixed by >>):
drivers/i2c/busses/i2c-designware-master.c: In function 'i2c_dw_init_recovery_info':
>> drivers/i2c/busses/i2c-designware-master.c:829:21: error: 'struct device' has no member named 'pins'
829 | if (dev->dev->pins && dev->dev->pins->p)
| ^~
drivers/i2c/busses/i2c-designware-master.c:829:39: error: 'struct device' has no member named 'pins'
829 | if (dev->dev->pins && dev->dev->pins->p)
| ^~
drivers/i2c/busses/i2c-designware-master.c:830:42: error: 'struct device' has no member named 'pins'
830 | rinfo->pinctrl = dev->dev->pins->p;
| ^~
vim +829 drivers/i2c/busses/i2c-designware-master.c
822
823 static int i2c_dw_init_recovery_info(struct dw_i2c_dev *dev)
824 {
825 struct i2c_bus_recovery_info *rinfo = &dev->rinfo;
826 struct i2c_adapter *adap = &dev->adapter;
827 struct gpio_desc *gpio;
828
> 829 if (dev->dev->pins && dev->dev->pins->p)
830 rinfo->pinctrl = dev->dev->pins->p;
831
832 gpio = devm_gpiod_get_optional(dev->dev, "scl", GPIOD_OUT_HIGH);
833 if (IS_ERR_OR_NULL(gpio))
834 return PTR_ERR_OR_ZERO(gpio);
835
836 rinfo->scl_gpiod = gpio;
837
838 gpio = devm_gpiod_get_optional(dev->dev, "sda", GPIOD_IN);
839 if (IS_ERR(gpio))
840 return PTR_ERR(gpio);
841 rinfo->sda_gpiod = gpio;
842
843 rinfo->recover_bus = i2c_generic_scl_recovery;
844 rinfo->prepare_recovery = i2c_dw_prepare_recovery;
845 rinfo->unprepare_recovery = i2c_dw_unprepare_recovery;
846 adap->bus_recovery_info = rinfo;
847
848 dev_info(dev->dev, "running with gpio recovery mode! scl%s",
849 rinfo->sda_gpiod ? ",sda" : "");
850
851 return 0;
852 }
853
--
0-DAY CI Kernel Test Service
https://01.org/lkp
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 linus/master v6.1 next-20221214]
[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-designware-set-pinctrl-recovery-information-from-device-pinctrl/20221214-222910
base: https://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git i2c/for-next
patch link: https://lore.kernel.org/r/20221214142725.23881-1-hhhawa%40amazon.com
patch subject: [PATCH v2 1/1] i2c: designware: set pinctrl recovery information from device pinctrl
config: x86_64-randconfig-a014
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/6e428843e5a6779565aae5f37fe0093ad526f139
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Hanna-Hawa/i2c-designware-set-pinctrl-recovery-information-from-device-pinctrl/20221214-222910
git checkout 6e428843e5a6779565aae5f37fe0093ad526f139
# 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 SHELL=/bin/bash
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <[email protected]>
All errors (new ones prefixed by >>):
>> drivers/i2c/busses/i2c-designware-master.c:829:16: error: no member named 'pins' in 'struct device'
if (dev->dev->pins && dev->dev->pins->p)
~~~~~~~~ ^
drivers/i2c/busses/i2c-designware-master.c:829:34: error: no member named 'pins' in 'struct device'
if (dev->dev->pins && dev->dev->pins->p)
~~~~~~~~ ^
drivers/i2c/busses/i2c-designware-master.c:830:30: error: no member named 'pins' in 'struct device'
rinfo->pinctrl = dev->dev->pins->p;
~~~~~~~~ ^
3 errors generated.
vim +829 drivers/i2c/busses/i2c-designware-master.c
822
823 static int i2c_dw_init_recovery_info(struct dw_i2c_dev *dev)
824 {
825 struct i2c_bus_recovery_info *rinfo = &dev->rinfo;
826 struct i2c_adapter *adap = &dev->adapter;
827 struct gpio_desc *gpio;
828
> 829 if (dev->dev->pins && dev->dev->pins->p)
830 rinfo->pinctrl = dev->dev->pins->p;
831
832 gpio = devm_gpiod_get_optional(dev->dev, "scl", GPIOD_OUT_HIGH);
833 if (IS_ERR_OR_NULL(gpio))
834 return PTR_ERR_OR_ZERO(gpio);
835
836 rinfo->scl_gpiod = gpio;
837
838 gpio = devm_gpiod_get_optional(dev->dev, "sda", GPIOD_IN);
839 if (IS_ERR(gpio))
840 return PTR_ERR(gpio);
841 rinfo->sda_gpiod = gpio;
842
843 rinfo->recover_bus = i2c_generic_scl_recovery;
844 rinfo->prepare_recovery = i2c_dw_prepare_recovery;
845 rinfo->unprepare_recovery = i2c_dw_unprepare_recovery;
846 adap->bus_recovery_info = rinfo;
847
848 dev_info(dev->dev, "running with gpio recovery mode! scl%s",
849 rinfo->sda_gpiod ? ",sda" : "");
850
851 return 0;
852 }
853
--
0-DAY CI Kernel Test Service
https://01.org/lkp
On 12/14/2022 6:09 PM, Andy Shevchenko wrote:
> ...should be located here.
>
> ...
Ack - will move them.
>
>> + if (dev->dev->pins && dev->dev->pins->p)
>> + rinfo->pinctrl = dev->dev->pins->p;
> Hmm... I don't see how this field is being used.
> Can you elaborate?
This field is used in i2c_generic_scl_recovery(), if it's not NULL then
the flow will set the state to GPIO before running the recovery mechanism.
if (bri->pinctrl)
pinctrl_select_state(bri->pinctrl, bri->pins_gpio);
.
.
I saw that that the change failed in complication for SPARC
architecture, as the pins field is wraparound with CONFIG_PINCTRL in
device struct. I though on two options to solve the compilation error,
first by adding wraparound of CONFIG_PINCTRL when accessing the pins
field. And the second option is to add get function in pinctrl/devinfo.h
file, which return the pins field, or NULL in case the PINCTRL is not
defined. Which option you think we can go with?
On Thu, Dec 15, 2022 at 10:15:35AM +0200, Hawa, Hanna wrote:
> On 12/14/2022 6:09 PM, Andy Shevchenko wrote:
...
> > > + if (dev->dev->pins && dev->dev->pins->p)
> > > + rinfo->pinctrl = dev->dev->pins->p;
> > Hmm... I don't see how this field is being used.
> > Can you elaborate?
>
> This field is used in i2c_generic_scl_recovery(), if it's not NULL then the
> flow will set the state to GPIO before running the recovery mechanism.
> if (bri->pinctrl)
> pinctrl_select_state(bri->pinctrl, bri->pins_gpio);
OK, but why that function doesn't use the dev->pins->p if it's defined?
(As a fallback when rinfo->pinctrl is NULL.)
Wolfram?
Hanna, it seems you missed I?C maintainer to Cc...
...
> I saw that that the change failed in complication for SPARC architecture, as
> the pins field is wraparound with CONFIG_PINCTRL in device struct. I though
> on two options to solve the compilation error, first by adding wraparound of
> CONFIG_PINCTRL when accessing the pins field. And the second option is to
> add get function in pinctrl/devinfo.h file, which return the pins field, or
> NULL in case the PINCTRL is not defined. Which option you think we can go
> with?
Getter with a stub sounds better to me, so you won't access some device core
fields.
Linus, what do you think about all these (including previous paragraph)?
--
With Best Regards,
Andy Shevchenko
On 12/15/2022 4:06 PM, Linus Walleij wrote:
>> Getter with a stub sounds better to me, so you won't access some device core
>> fields.
>>
>> Linus, what do you think about all these (including previous paragraph)?
> A getter may be a good solution, it depends, it can also be pushed
> somewhere local in the designware i2c driver can it not?
> I am thinking that the rest of the code that is using that field is
> certainly not going to work without pinctrl either.
the compilation failure occurs on platform which not define the
CONFIG_PINCTRL , most of the pinctrl APIs are wrapped with PINCTRL
config, for example pinctrl_select_state() or devm_pinctrl_get().
In addition if we add the getter in pinctrl/devinfo.h other drivers may
access the pins field without re-call devm_pinctrl_get().
Thanks,
Hanna
On Thu, Dec 15, 2022 at 11:28 AM Andy Shevchenko
<[email protected]> wrote:
> On Thu, Dec 15, 2022 at 10:15:35AM +0200, Hawa, Hanna wrote:
> > On 12/14/2022 6:09 PM, Andy Shevchenko wrote:
>
> ...
>
> > > > + if (dev->dev->pins && dev->dev->pins->p)
> > > > + rinfo->pinctrl = dev->dev->pins->p;
> > > Hmm... I don't see how this field is being used.
> > > Can you elaborate?
> >
> > This field is used in i2c_generic_scl_recovery(), if it's not NULL then the
> > flow will set the state to GPIO before running the recovery mechanism.
> > if (bri->pinctrl)
> > pinctrl_select_state(bri->pinctrl, bri->pins_gpio);
>
> OK, but why that function doesn't use the dev->pins->p if it's defined?
> (As a fallback when rinfo->pinctrl is NULL.)
I don't understand the context of these things so can't say much
about it.
> > I saw that that the change failed in complication for SPARC architecture, as
> > the pins field is wraparound with CONFIG_PINCTRL in device struct. I though
> > on two options to solve the compilation error, first by adding wraparound of
> > CONFIG_PINCTRL when accessing the pins field. And the second option is to
> > add get function in pinctrl/devinfo.h file, which return the pins field, or
> > NULL in case the PINCTRL is not defined. Which option you think we can go
> > with?
>
> Getter with a stub sounds better to me, so you won't access some device core
> fields.
>
> Linus, what do you think about all these (including previous paragraph)?
A getter may be a good solution, it depends, it can also be pushed
somewhere local in the designware i2c driver can it not?
I am thinking that the rest of the code that is using that field is
certainly not going to work without pinctrl either.
Yours,
Linus Walleij
On Thu, Dec 15, 2022 at 03:06:13PM +0100, Linus Walleij wrote:
> On Thu, Dec 15, 2022 at 11:28 AM Andy Shevchenko
> <[email protected]> wrote:
> > On Thu, Dec 15, 2022 at 10:15:35AM +0200, Hawa, Hanna wrote:
> > > On 12/14/2022 6:09 PM, Andy Shevchenko wrote:
...
> > > > > + if (dev->dev->pins && dev->dev->pins->p)
> > > > > + rinfo->pinctrl = dev->dev->pins->p;
> > > > Hmm... I don't see how this field is being used.
> > > > Can you elaborate?
> > >
> > > This field is used in i2c_generic_scl_recovery(), if it's not NULL then the
> > > flow will set the state to GPIO before running the recovery mechanism.
> > > if (bri->pinctrl)
> > > pinctrl_select_state(bri->pinctrl, bri->pins_gpio);
> >
> > OK, but why that function doesn't use the dev->pins->p if it's defined?
> > (As a fallback when rinfo->pinctrl is NULL.)
>
> I don't understand the context of these things so can't say much
> about it.
Main question here is that, is it possible that GPIOs that back up I?C bus are
on the different pin control device that the bus pins themselves?
And while writing above I think it may be the case if we use additional
GPIO pins over the I?C bus for purpose of recovery. In such case the pin control
device can be different.
At the same time, the fallback option might still work, in case the pinctrl not
overridden and I?C bus has backed GPIO function (on SCL/SDA).
...
> > > I saw that that the change failed in complication for SPARC architecture, as
> > > the pins field is wraparound with CONFIG_PINCTRL in device struct. I though
> > > on two options to solve the compilation error, first by adding wraparound of
> > > CONFIG_PINCTRL when accessing the pins field. And the second option is to
> > > add get function in pinctrl/devinfo.h file, which return the pins field, or
> > > NULL in case the PINCTRL is not defined. Which option you think we can go
> > > with?
> >
> > Getter with a stub sounds better to me, so you won't access some device core
> > fields.
> >
> > Linus, what do you think about all these (including previous paragraph)?
>
> A getter may be a good solution, it depends, it can also be pushed
> somewhere local in the designware i2c driver can it not?
Yeah, but my point in the above paragraph that it uses the generic recovery
mechanism which may (or may not?) utilise the same pin control as I?C bus
sitting on).
> I am thinking that the rest of the code that is using that field is
> certainly not going to work without pinctrl either.
--
With Best Regards,
Andy Shevchenko
On 12/15/2022 12:27 PM, Andy Shevchenko wrote:
> OK, but why that function doesn't use the dev->pins->p if it's defined?
> (As a fallback when rinfo->pinctrl is NULL. >
The solution will look like the below diff (get_device_pinctrl() is new
function that i've added that return the dev->pins->p)
diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index 7539b0740351..469344d4ee43 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 = get_device_pinctrl(dev->parent);
+ p = bri->pinctrl;
> Wolfram?
>
> Hanna, it seems you missed I²C maintainer to Cc...
I based my CC/TO on get_maintainer.pl script. Will make sure that
Wolfram on CC next time.
On Fri, Dec 16, 2022 at 03:50:19PM +0200, Hawa, Hanna wrote:
> On 12/15/2022 12:27 PM, Andy Shevchenko wrote:
> > OK, but why that function doesn't use the dev->pins->p if it's defined?
> > (As a fallback when rinfo->pinctrl is NULL. >
>
> The solution will look like the below diff (get_device_pinctrl() is new
> function that i've added that return the dev->pins->p)
Naming is not aligned with a namespace.
I would rather name it dev_pinctrl() and locate it in pinctrl/devinfo.h.
...
> - struct pinctrl *p = bri->pinctrl;
> + struct pinctrl *p;
> +
> + if (!bri->pinctrl)
> + bri->pinctrl = get_device_pinctrl(dev->parent);
> + p = bri->pinctrl;
Can be simplified with help of Elvis:
p = bri->pinctrl ?: dev_pinctrl(dev->parent);
> > Wolfram?
> >
> > Hanna, it seems you missed I?C maintainer to Cc...
>
> I based my CC/TO on get_maintainer.pl script. Will make sure that Wolfram on
> CC next time.
All the same about Linus W., who is pin control subsystem maintainer, and be
sure the respective mailing lists are also included.
--
With Best Regards,
Andy Shevchenko
On 12/16/2022 4:46 PM, Andy Shevchenko wrote:
> Can be simplified with help of Elvis:
>
> p = bri->pinctrl ?: dev_pinctrl(dev->parent);
Can't use this, as need to set the bri->pinctrl to dev_pinctrl() in case
it's not set by the driver.
>> I based my CC/TO on get_maintainer.pl script. Will make sure that Wolfram on
>> CC next time.
> All the same about Linus W., who is pin control subsystem maintainer, and be
> sure the respective mailing lists are also included.
Sure, thanks
Thanks,
Hanna
On Mon, Dec 19, 2022 at 09:35:37PM +0200, Hawa, Hanna wrote:
> On 12/16/2022 4:46 PM, Andy Shevchenko wrote:
> > Can be simplified with help of Elvis:
> >
> > p = bri->pinctrl ?: dev_pinctrl(dev->parent);
>
> Can't use this, as need to set the bri->pinctrl to dev_pinctrl() in case
> it's not set by the driver.
I guess you can. I will answer to the v3.
--
With Best Regards,
Andy Shevchenko