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.
--
With Best Regards,
Andy Shevchenko
Hi Andy,
On Thu, 25 Mar 2021 at 16:43, Andy Shevchenko
<[email protected]> 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
> <[email protected]> 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.
--
With Best Regards,
Andy Shevchenko
On Fri, 26 Mar 2021 at 13:28, Andy Shevchenko
<[email protected]> wrote:
>
> On Fri, Mar 26, 2021 at 11:35:08AM +0100, Daniel Gomez wrote:
> > On Thu, 25 Mar 2021 at 16:43, Andy Shevchenko
> > <[email protected]> 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 <[email protected]> wrote:
> On Fri, 26 Mar 2021 at 13:28, Andy Shevchenko
> <[email protected]> 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!
--
With Best Regards,
Andy Shevchenko