Re: [PATCH v2 1/2] i2c: i2c-amd-mp2: Remove NIH logging functions

From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
To: mail@richard-neumann.de
Cc: syniurge@gmail.com, nehal-bakulchandra.shah@amd.com,
	shyam-sundar.s-k@amd.com, linux-i2c@vger.kernel.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 1/2] i2c: i2c-amd-mp2: Remove NIH logging functions
Date: Mon, 1 Feb 2021 12:37:01 +0200	[thread overview]
Message-ID: <YBfZzUHx4UpRwhM9@smile.fi.intel.com> (raw)
In-Reply-To: <20210129192553.55906-2-mail@richard-neumann.de>

On Fri, Jan 29, 2021 at 08:25:52PM +0100, mail@richard-neumann.de wrote:
> From: Richard Neumann <mail@richard-neumann.de>

Thanks for an update, my comments below.

> Use pci_{info,warn,err,dbg} functions of the kernel's PCI API.
> Remove unnecessary ndev_pdev, ndev_name and ndev_dev macros.

"ndev_pdev(), ndev_name() and ndev_dev()"

> Remove useless __func__ from logging.

"While at it, remove..."

...

> -		dev_err(ndev_dev(privdata),
> +		pci_err(pdev,
>  			"length %d in event doesn't match buffer length %d!\n",

Can be one line, but it's up to you (checkpatch won't scream on this).
Same comment for other similar cases.

>  			len, i2c_common->msg->len);

...

>  	struct pci_dev *pci_dev = to_pci_dev(dev);
>  	struct amd_mp2_dev *privdata = pci_get_drvdata(pci_dev);
> +	struct pci_dev *pdev = privdata->pci_dev;

I guess it's a bit overkill. You already have pci_dev above. Isn't it enough?

...

> +		pci_err(pdev, "pci_save_state failed = %d\n", ret);

And here pci_dev will be still okay and fit 80 limit.

-- 
With Best Regards,
Andy Shevchenko