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

From: Daniel Gomez <daniel@qtec.com>
To: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: Jarkko Nikula <jarkko.nikula@linux.intel.com>,
	Mika Westerberg <mika.westerberg@linux.intel.com>,
	linux-i2c@vger.kernel.org,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] i2c: designware: Add base addr info
Date: Fri, 26 Mar 2021 11:35:08 +0100	[thread overview]
Message-ID: <CAH1Ww+Qs13GBC02PCgW60No2Z+vNsV14yRe7S4rtnnMLqH7BYQ@mail.gmail.com> (raw)
In-Reply-To: <YFyvh3sqyVcg8Iqj@smile.fi.intel.com>

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