Messages in this thread | | | From | Anand Ashok Dumbre <> | Subject | RE: [PATCH v10 3/5] iio: adc: Add Xilinx AMS driver | Date | Sun, 21 Nov 2021 12:29:59 +0000 |
| |
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 <ANANDASH@xilinx.com> > Cc: linux-kernel@vger.kernel.org; jic23@kernel.org; lars@metafoo.de; linux- > iio@vger.kernel.org; git <git@xilinx.com>; Michal Simek > <michals@xilinx.com>; gregkh@linuxfoundation.org; rafael@kernel.org; > linux-acpi@vger.kernel.org; heikki.krogerus@linux.intel.com; Manish Narani > <MNARANI@xilinx.com> > 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: > > > 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: > > ... > > > > > + 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.
> > > > > + } > > ... > > > > 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
> > ... > > > > > + 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. > > > > > + continue; > > ... > > > > > + dev_chan_size = array_size((size_t)num_channels, sizeof(struct > > > iio_chan_spec)); > > > > + 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.
> > > > > > > + dev_channels = devm_kcalloc(&pdev->dev, (size_t)num_channels, > > > > > > Why casting?
Removed.
> > > > > > > + sizeof(struct iio_chan_spec), GFP_KERNEL); > > > > > > sizeof(*dev_channels) > > > > > > > + if (!dev_channels) { > > > > + ret = -ENOMEM; > > > > + goto free_mem; > > > > + } > > > > > > > + 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(). > > > > > > 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
| |