| Message ID | 20210325151248.1066643-1-daniel@qtec.com |
|---|---|
| State | Rejected |
| Headers | show |
| Series | i2c: designware: Add base addr info | expand |
On Thu, Mar 25, 2021 at 04:12:48PM +0100, Daniel Gomez wrote: > Add i2c hw base address in the adapter name and when the device is > probed. Why? We have /proc/iomem for that. Sorry, I don't see value of this change. Some comments below just to let you know about style:ish issues. ... > snprintf(adap->name, sizeof(adap->name), > - "Synopsys DesignWare I2C adapter"); > + "Synopsys DesignWare I2C adapter at 0x%llx", dev->base_addr); It actually should be resource_size_t and corresponding specifier, i.e. %pa to print it. Moreover, we have %pR (and %pr) specifiers for struct resource. ... > + dev_info(&pdev->dev, "%s\n", adap->name); Unneeded noise.
Hi Daniel,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on wsa/i2c/for-next]
[also build test WARNING on v5.12-rc4 next-20210325]
[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/Daniel-Gomez/i2c-designware-Add-base-addr-info/20210325-232218
base: https://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git i2c/for-next
config: i386-randconfig-r016-20210325 (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/5233e225fc69cfeef637d28c49f12967cbc36430
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Daniel-Gomez/i2c-designware-Add-base-addr-info/20210325-232218
git checkout 5233e225fc69cfeef637d28c49f12967cbc36430
# save the attached .config to linux build tree
make W=1 ARCH=i386
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
drivers/i2c/busses/i2c-designware-master.c: In function 'i2c_dw_probe_master':
>> drivers/i2c/busses/i2c-designware-master.c:770:45: warning: format '%llx' expects argument of type 'long long unsigned int', but argument 4 has type 'phys_addr_t' {aka 'unsigned int'} [-Wformat=]
770 | "Synopsys DesignWare I2C adapter at 0x%llx", dev->base_addr);
| ~~~^ ~~~~~~~~~~~~~~
| | |
| | phys_addr_t {aka unsigned int}
| long long unsigned int
| %x
vim +770 drivers/i2c/busses/i2c-designware-master.c
740
741 int i2c_dw_probe_master(struct dw_i2c_dev *dev)
742 {
743 struct i2c_adapter *adap = &dev->adapter;
744 unsigned long irq_flags;
745 int ret;
746
747 init_completion(&dev->cmd_complete);
748
749 dev->init = i2c_dw_init_master;
750 dev->disable = i2c_dw_disable;
751 dev->disable_int = i2c_dw_disable_int;
752
753 ret = i2c_dw_init_regmap(dev);
754 if (ret)
755 return ret;
756
757 ret = i2c_dw_set_timings_master(dev);
758 if (ret)
759 return ret;
760
761 ret = i2c_dw_set_fifo_size(dev);
762 if (ret)
763 return ret;
764
765 ret = dev->init(dev);
766 if (ret)
767 return ret;
768
769 snprintf(adap->name, sizeof(adap->name),
> 770 "Synopsys DesignWare I2C adapter at 0x%llx", dev->base_addr);
771 adap->retries = 3;
772 adap->algo = &i2c_dw_algo;
773 adap->quirks = &i2c_dw_quirks;
774 adap->dev.parent = dev->dev;
775 i2c_set_adapdata(adap, dev);
776
777 if (dev->flags & ACCESS_NO_IRQ_SUSPEND) {
778 irq_flags = IRQF_NO_SUSPEND;
779 } else {
780 irq_flags = IRQF_SHARED | IRQF_COND_SUSPEND;
781 }
782
783 i2c_dw_disable_int(dev);
784 ret = devm_request_irq(dev->dev, dev->irq, i2c_dw_isr, irq_flags,
785 dev_name(dev->dev), dev);
786 if (ret) {
787 dev_err(dev->dev, "failure requesting irq %i: %d\n",
788 dev->irq, ret);
789 return ret;
790 }
791
792 ret = i2c_dw_init_recovery_info(dev);
793 if (ret)
794 return ret;
795
796 /*
797 * Increment PM usage count during adapter registration in order to
798 * avoid possible spurious runtime suspend when adapter device is
799 * registered to the device core and immediate resume in case bus has
800 * registered I2C slaves that do I2C transfers in their probe.
801 */
802 pm_runtime_get_noresume(dev->dev);
803 ret = i2c_add_numbered_adapter(adap);
804 if (ret)
805 dev_err(dev->dev, "failure adding adapter: %d\n", ret);
806 pm_runtime_put_noidle(dev->dev);
807
808 return ret;
809 }
810 EXPORT_SYMBOL_GPL(i2c_dw_probe_master);
811
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
Hi Andy, On Thu, 25 Mar 2021 at 16:43, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > On Thu, Mar 25, 2021 at 04:12:48PM +0100, Daniel Gomez wrote: > > Add i2c hw base address in the adapter name and when the device is > > probed. > > Why? > We have /proc/iomem for that. The initial reason was because I wasn't aware of /proc/iomem therefore I didn't have a way to match the physical address to the i2c adapter. So, thanks for pointing that out as now I'm able to match the physical address listed in iomem with the sysfs i2c bus. > > Sorry, I don't see value of this change. > Some comments below just to let you know about style:ish issues. Thanks for the comments. Although there are no reasons to apply this patch I have some doubts perhaps they will help me next time. > > ... > > > snprintf(adap->name, sizeof(adap->name), > > - "Synopsys DesignWare I2C adapter"); > > + "Synopsys DesignWare I2C adapter at 0x%llx", dev->base_addr); > > It actually should be resource_size_t and corresponding specifier, i.e. %pa to > print it. Moreover, we have %pR (and %pr) specifiers for struct resource. I understand this but I had some doubts when I declared the variable. I took this as reference: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/i2c/busses/i2c-tegra.c?h=v5.12-rc4#n268 Should it be then defined as resource_size_t instead? Out of the i2c subsystem, I also found several examples. For example this: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/iio/adc/at91-sama5d2_adc.c?h=v5.12-rc4#n364 But I understand this could be out of the scope. Some others, even assign the the start to the dma_addr_t which could vary depending on CONFIG_ARCH_DMA_ADDR_T_64BIT but it seems equivalent to the phys_addr_t definition. > > ... > > > + dev_info(&pdev->dev, "%s\n", adap->name); > > Unneeded noise. Also this might be out of the scope again but I added because in tty they were printing that information: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/tty/serial/serial_core.c?h=v5.12-rc4#n2336 Anyway, thanks again Andy for the review. Really appreciate it! :) > > -- > With Best Regards, > Andy Shevchenko > >
On Fri, Mar 26, 2021 at 11:35:08AM +0100, Daniel Gomez wrote: > On Thu, 25 Mar 2021 at 16:43, Andy Shevchenko > <andriy.shevchenko@linux.intel.com> wrote: > > On Thu, Mar 25, 2021 at 04:12:48PM +0100, Daniel Gomez wrote: ... > > > Add i2c hw base address in the adapter name and when the device is > > > probed. > > > > Why? > > We have /proc/iomem for that. > The initial reason was because I wasn't aware of /proc/iomem therefore > I didn't have a way to match the physical address to the i2c adapter. > So, thanks for pointing that out as now I'm able to match the physical > address listed in iomem with the sysfs i2c bus. You are welcome! ... > > > snprintf(adap->name, sizeof(adap->name), > > > - "Synopsys DesignWare I2C adapter"); > > > + "Synopsys DesignWare I2C adapter at 0x%llx", dev->base_addr); > > > > It actually should be resource_size_t and corresponding specifier, i.e. %pa to > > print it. Moreover, we have %pR (and %pr) specifiers for struct resource. > I understand this but I had some doubts when I declared the variable. > I took this as reference: > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/i2c/busses/i2c-tegra.c?h=v5.12-rc4#n268 > Should it be then defined as resource_size_t instead? It's a good question. On one hand we know that resource_size_t is a simple redefinition of phys_addr_t, but it might be changed in the future. OTOH, struct resource has types of resource_size_t. In any case it's a type that is platform dependent (like long, size_t). Hence, the special specifier is needed. > Out of the i2c subsystem, I also found several examples. For example this: > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/iio/adc/at91-sama5d2_adc.c?h=v5.12-rc4#n364 > But I understand this could be out of the scope. Not all examples in the kernel are good examples (many of them is a cargo-cult and / or outdated). Take them with grain of salt. But common rule is to check the log of the interesting subsystem (`git log -- drivers/<subsystem>/`) in order to find the most recent drivers or modules added. There you very likely will find more or less modern standards and APIs you might reuse in your code. > Some others, even assign the the start to the dma_addr_t which could > vary depending on CONFIG_ARCH_DMA_ADDR_T_64BIT > but it seems equivalent to the phys_addr_t definition. There is a document that describes all possible extensions we have for %p. You might be curious to read more there. ... > > > + dev_info(&pdev->dev, "%s\n", adap->name); > > > > Unneeded noise. > Also this might be out of the scope again but I added because in tty > they were printing that information: > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/tty/serial/serial_core.c?h=v5.12-rc4#n2336 TTY is different. TTY often related to a console and it's very important to know some information as soon as possible (don't forget also hardware debuggers, e.g. Lauterbach, which able to show kernel message ring buffer). As you may know console is the first common target during new platform bring-up.
On Fri, 26 Mar 2021 at 13:28, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > On Fri, Mar 26, 2021 at 11:35:08AM +0100, Daniel Gomez wrote: > > On Thu, 25 Mar 2021 at 16:43, Andy Shevchenko > > <andriy.shevchenko@linux.intel.com> wrote: > > > On Thu, Mar 25, 2021 at 04:12:48PM +0100, Daniel Gomez wrote: > > ... > > > > > Add i2c hw base address in the adapter name and when the device is > > > > probed. > > > > > > Why? > > > We have /proc/iomem for that. > > The initial reason was because I wasn't aware of /proc/iomem therefore > > I didn't have a way to match the physical address to the i2c adapter. > > So, thanks for pointing that out as now I'm able to match the physical > > address listed in iomem with the sysfs i2c bus. > > You are welcome! > > ... > > > > > snprintf(adap->name, sizeof(adap->name), > > > > - "Synopsys DesignWare I2C adapter"); > > > > + "Synopsys DesignWare I2C adapter at 0x%llx", dev->base_addr); > > > > > > It actually should be resource_size_t and corresponding specifier, i.e. %pa to > > > print it. Moreover, we have %pR (and %pr) specifiers for struct resource. > > I understand this but I had some doubts when I declared the variable. > > I took this as reference: > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/i2c/busses/i2c-tegra.c?h=v5.12-rc4#n268 > > Should it be then defined as resource_size_t instead? > > It's a good question. On one hand we know that resource_size_t is a simple > redefinition of phys_addr_t, but it might be changed in the future. OTOH, > struct resource has types of resource_size_t. In any case it's a type that is > platform dependent (like long, size_t). Hence, the special specifier is needed. This 'issue' occurs in other subsystems like iio but I can see the patches are quite old in comparison with the i2c-tegra one. Also, the same happens when they print the variable (wrong specifier). > > > Out of the i2c subsystem, I also found several examples. For example this: > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/iio/adc/at91-sama5d2_adc.c?h=v5.12-rc4#n364 > > But I understand this could be out of the scope. > > Not all examples in the kernel are good examples (many of them is a cargo-cult > and / or outdated). Take them with grain of salt. > Yes, you are right. > But common rule is to check the log of the interesting subsystem (`git log -- > drivers/<subsystem>/`) in order to find the most recent drivers or modules > added. There you very likely will find more or less modern standards and APIs > you might reuse in your code. > > > Some others, even assign the the start to the dma_addr_t which could > > vary depending on CONFIG_ARCH_DMA_ADDR_T_64BIT > > but it seems equivalent to the phys_addr_t definition. > > There is a document that describes all possible extensions we have for %p. You > might be curious to read more there. https://www.kernel.org/doc/html/latest/core-api/printk-formats.html?highlight=physical%20addresses%20types#how-to-get-printk-format-specifiers-right > > ... > > > > > + dev_info(&pdev->dev, "%s\n", adap->name); > > > > > > Unneeded noise. > > Also this might be out of the scope again but I added because in tty > > they were printing that information: > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/tty/serial/serial_core.c?h=v5.12-rc4#n2336 > > TTY is different. TTY often related to a console and it's very important to > know some information as soon as possible (don't forget also hardware > debuggers, e.g. Lauterbach, which able to show kernel message ring buffer). > As you may know console is the first common target during new platform > bring-up. That's right, completely different topic, I used Lauterbach in the past and the kernel initcalls with the tty physical address and I understand your point. > > -- > With Best Regards, > Andy Shevchenko Thank you Andy for all the comments and hints! :) Regards, Daniel Gomez > >
On Sat, Mar 27, 2021 at 8:18 PM Daniel Gomez <daniel@qtec.com> wrote: > On Fri, 26 Mar 2021 at 13:28, Andy Shevchenko > <andriy.shevchenko@linux.intel.com> wrote: > > On Fri, Mar 26, 2021 at 11:35:08AM +0100, Daniel Gomez wrote: ... > > It's a good question. On one hand we know that resource_size_t is a simple > > redefinition of phys_addr_t, but it might be changed in the future. OTOH, > > struct resource has types of resource_size_t. In any case it's a type that is > > platform dependent (like long, size_t). Hence, the special specifier is needed. > > This 'issue' occurs in other subsystems like iio but I can see the > patches are quite old in comparison with the i2c-tegra one. > Also, the same happens when they print the variable (wrong specifier). Now since you have better understanding you may clean those up!
Hi Daniel,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on wsa/i2c/for-next]
[also build test WARNING on v5.12-rc5 next-20210329]
[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/Daniel-Gomez/i2c-designware-Add-base-addr-info/20210325-232218
base: https://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git i2c/for-next
config: powerpc-randconfig-r012-20210329 (attached as .config)
compiler: clang version 13.0.0 (https://github.com/llvm/llvm-project 2a28d1d3b7bf2062288b46af34e33ccc543a99fa)
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
# install powerpc cross compiling tool for clang build
# apt-get install binutils-powerpc-linux-gnu
# https://github.com/0day-ci/linux/commit/5233e225fc69cfeef637d28c49f12967cbc36430
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Daniel-Gomez/i2c-designware-Add-base-addr-info/20210325-232218
git checkout 5233e225fc69cfeef637d28c49f12967cbc36430
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=powerpc
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
>> drivers/i2c/busses/i2c-designware-master.c:770:49: warning: format specifies type 'unsigned long long' but the argument has type 'phys_addr_t' (aka 'unsigned int') [-Wformat]
"Synopsys DesignWare I2C adapter at 0x%llx", dev->base_addr);
~~~~ ^~~~~~~~~~~~~~
%x
1 warning generated.
vim +770 drivers/i2c/busses/i2c-designware-master.c
740
741 int i2c_dw_probe_master(struct dw_i2c_dev *dev)
742 {
743 struct i2c_adapter *adap = &dev->adapter;
744 unsigned long irq_flags;
745 int ret;
746
747 init_completion(&dev->cmd_complete);
748
749 dev->init = i2c_dw_init_master;
750 dev->disable = i2c_dw_disable;
751 dev->disable_int = i2c_dw_disable_int;
752
753 ret = i2c_dw_init_regmap(dev);
754 if (ret)
755 return ret;
756
757 ret = i2c_dw_set_timings_master(dev);
758 if (ret)
759 return ret;
760
761 ret = i2c_dw_set_fifo_size(dev);
762 if (ret)
763 return ret;
764
765 ret = dev->init(dev);
766 if (ret)
767 return ret;
768
769 snprintf(adap->name, sizeof(adap->name),
> 770 "Synopsys DesignWare I2C adapter at 0x%llx", dev->base_addr);
771 adap->retries = 3;
772 adap->algo = &i2c_dw_algo;
773 adap->quirks = &i2c_dw_quirks;
774 adap->dev.parent = dev->dev;
775 i2c_set_adapdata(adap, dev);
776
777 if (dev->flags & ACCESS_NO_IRQ_SUSPEND) {
778 irq_flags = IRQF_NO_SUSPEND;
779 } else {
780 irq_flags = IRQF_SHARED | IRQF_COND_SUSPEND;
781 }
782
783 i2c_dw_disable_int(dev);
784 ret = devm_request_irq(dev->dev, dev->irq, i2c_dw_isr, irq_flags,
785 dev_name(dev->dev), dev);
786 if (ret) {
787 dev_err(dev->dev, "failure requesting irq %i: %d\n",
788 dev->irq, ret);
789 return ret;
790 }
791
792 ret = i2c_dw_init_recovery_info(dev);
793 if (ret)
794 return ret;
795
796 /*
797 * Increment PM usage count during adapter registration in order to
798 * avoid possible spurious runtime suspend when adapter device is
799 * registered to the device core and immediate resume in case bus has
800 * registered I2C slaves that do I2C transfers in their probe.
801 */
802 pm_runtime_get_noresume(dev->dev);
803 ret = i2c_add_numbered_adapter(adap);
804 if (ret)
805 dev_err(dev->dev, "failure adding adapter: %d\n", ret);
806 pm_runtime_put_noidle(dev->dev);
807
808 return ret;
809 }
810 EXPORT_SYMBOL_GPL(i2c_dw_probe_master);
811
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
diff --git a/drivers/i2c/busses/i2c-designware-core.h b/drivers/i2c/busses/i2c-designware-core.h index 5392b82f68a4..8c56a7ec8693 100644 --- a/drivers/i2c/busses/i2c-designware-core.h +++ b/drivers/i2c/busses/i2c-designware-core.h @@ -241,6 +241,7 @@ struct dw_i2c_dev { struct regmap *sysmap; void __iomem *base; void __iomem *ext; + phys_addr_t base_addr; struct completion cmd_complete; struct clk *clk; struct clk *pclk; diff --git a/drivers/i2c/busses/i2c-designware-master.c b/drivers/i2c/busses/i2c-designware-master.c index dd27b9dbe931..5e78b0aec2d3 100644 --- a/drivers/i2c/busses/i2c-designware-master.c +++ b/drivers/i2c/busses/i2c-designware-master.c @@ -767,7 +767,7 @@ int i2c_dw_probe_master(struct dw_i2c_dev *dev) return ret; snprintf(adap->name, sizeof(adap->name), - "Synopsys DesignWare I2C adapter"); + "Synopsys DesignWare I2C adapter at 0x%llx", dev->base_addr); adap->retries = 3; adap->algo = &i2c_dw_algo; adap->quirks = &i2c_dw_quirks; diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c index 0dfeb2d11603..c8ffcc85bc51 100644 --- a/drivers/i2c/busses/i2c-designware-platdrv.c +++ b/drivers/i2c/busses/i2c-designware-platdrv.c @@ -178,6 +178,7 @@ static void dw_i2c_plat_pm_cleanup(struct dw_i2c_dev *dev) static int dw_i2c_plat_request_regs(struct dw_i2c_dev *dev) { struct platform_device *pdev = to_platform_device(dev->dev); + struct resource *res; int ret; switch (dev->flags & MODEL_MASK) { @@ -185,7 +186,8 @@ static int dw_i2c_plat_request_regs(struct dw_i2c_dev *dev) ret = bt1_i2c_request_regs(dev); break; default: - dev->base = devm_platform_ioremap_resource(pdev, 0); + dev->base = devm_platform_get_and_ioremap_resource(pdev, 0, &res); + dev->base_addr = res->start; ret = PTR_ERR_OR_ZERO(dev->base); break; } @@ -313,6 +315,8 @@ static int dw_i2c_plat_probe(struct platform_device *pdev) if (ret) goto exit_probe; + dev_info(&pdev->dev, "%s\n", adap->name); + return ret; exit_probe:
Add i2c hw base address in the adapter name and when the device is probed. Output: root@qt5222:~# dmesg | grep -i syno [ 0.347045] i2c_designware AMDI0010:00: Synopsys DesignWare I2C adapter at 0xfedc4000 [ 0.348843] i2c_designware AMDI0010:01: Synopsys DesignWare I2C adapter at 0xfedc5000 root@qt5222:~# i2cdetect -l | grep -i desig i2c-1 i2c Synopsys DesignWare I2C adapter at 0xfedc5000 I2C adapter i2c-0 i2c Synopsys DesignWare I2C adapter at 0xfedc4000 I2C adapter Signed-off-by: Daniel Gomez <daniel@qtec.com> --- Hi, We think it might be interesting to add the physical address if you have multiple adapters like the above example so we don't write to the wrong device. Could it be possible to add this patch? Thanks drivers/i2c/busses/i2c-designware-core.h | 1 + drivers/i2c/busses/i2c-designware-master.c | 2 +- drivers/i2c/busses/i2c-designware-platdrv.c | 6 +++++- 3 files changed, 7 insertions(+), 2 deletions(-) -- 2.30.2