On Fri, 26 Mar 2021 at 13:28, Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx> 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 > >
