RE: [PATCH v10 3/5] iio: adc: Add Xilinx AMS driver

Hi Andy,

Thanks for the review.

-----Original Message-----
From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Sent: Thursday 18 November 2021 3:43 PM
To: Anand Ashok Dumbre <redacted>
Cc: linux-kernel@vger.kernel.org; jic23@kernel.org; lars@metafoo.de; linux-
iio@vger.kernel.org; git [off-list ref]; Michal Simek
[off-list ref]; gregkh@linuxfoundation.org; rafael@kernel.org;
linux-acpi@vger.kernel.org; heikki.krogerus@linux.intel.com; Manish Narani
[off-list ref]
Subject: Re: [PATCH v10 3/5] iio: adc: Add Xilinx AMS driver

On Thu, Nov 18, 2021 at 02:14:41PM +0000, Anand Ashok Dumbre wrote:

quoted

quoted

From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Sent: Wednesday 17 November 2021 8:03 PM On Wed, Nov 17, 2021 at
04:10:26PM +0000, Anand Ashok Dumbre wrote:

...

quoted

quoted

quoted

+			ret = ams_read_vcc_reg(ams, chan->address, val);
+			if (ret) {
+				mutex_unlock(&ams->lock);
+				return -EINVAL;

Shadowed error code.

I don’t understand.

You get an error code in ret variable, but you always return -EINVAL, why?

Got it. Will fix it.

quoted

quoted

quoted

+			}

...

quoted

quoted

default: ?

This is limited by hw bits.
For default I will use the default scan_index value.
Is that ok?

I think so, you know hardware better, you decide what to put there.

Will do

...

quoted

quoted

quoted

+		if (ret || reg > (AMS_PL_MAX_EXT_CHANNEL + 30))

Too many parentheses.

Is it a good practice to not have parantheses around

(AMS_PL_MAX_EXT_CHANNEL + 30) ?

It's minor thing, up to you.

quoted

quoted

quoted

+			continue;

...

quoted

quoted

quoted

+	dev_chan_size = array_size((size_t)num_channels, sizeof(struct

iio_chan_spec));

quoted

+	if (dev_chan_size == SIZE_MAX)
+		return -EINVAL;

Why is it needed now?

I didn’t see kcalloc did it internally. I will remove it.

quoted

quoted

quoted

+	dev_channels = devm_kcalloc(&pdev->dev, (size_t)num_channels,

Why casting?

Removed.

quoted

quoted

quoted

+				    sizeof(struct iio_chan_spec), GFP_KERNEL);

sizeof(*dev_channels)

quoted

+	if (!dev_channels) {
+		ret = -ENOMEM;
+		goto free_mem;
+	}

quoted

+	memcpy(dev_channels, ams_channels,
+	       sizeof(*ams_channels) * num_channels);

Hmm... according to the code the num_channels can be less than or
equal to num_chan. Hence, what you should use is the

devm_krealloc_array().

quoted

quoted

static inline void *devm_krealloc_aray(...) {
	...see how krealloc_array() is defined...
}

No need to copy memory again.

Will take a look.

I did see that there was previously a devm_krealloc_array implementation sent for the other Xilinx xadc driver.

https://patchwork.kernel.org/project/linux-iio/patch/20201102142228.14949-2-brgl@bgdev.pl/

From the conversations based here, should the use of devm_krealloc just be fine?
If not, I will send a patch with devm_krealloc for now, and then I can send a patch later for devm_krealloc array and
Its usage for both Xilinx-ams and xadc driver as well.

--
With Best Regards,
Andy Shevchenko

Thanks,
Anand