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