Re: [PATCH] i2c: designware: Add base addr info

[Daniel Gomez <daniel@qtec.com>]

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
>
>