| Message ID | 1531912331-26431-2-git-send-email-manish.narani@xilinx.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 07/18/2018 01:12 PM, Manish Narani wrote: > This patch fix the following checkpatch warning in xadc driver. > - Reusing the krealloc arg is almost always a bug. > > Renamed the 'channels' variable as 'iio_xadc_channels' to fix the above > warning. > This is a bug in checkpatch and should be fixed in checkpatch. The code is not actually re-using the parameter. channels and xadc_channels are independent variables, just checkpatch somehow does not realize this. > Signed-off-by: Manish Narani <manish.narani@xilinx.com> > --- > drivers/iio/adc/xilinx-xadc-core.c | 16 +++++++++------- > 1 file changed, 9 insertions(+), 7 deletions(-) > > diff --git a/drivers/iio/adc/xilinx-xadc-core.c b/drivers/iio/adc/xilinx-xadc-core.c > index d4f21d1..27b45df 100644 > --- a/drivers/iio/adc/xilinx-xadc-core.c > +++ b/drivers/iio/adc/xilinx-xadc-core.c > @@ -1040,7 +1040,7 @@ static int xadc_parse_dt(struct iio_dev *indio_dev, struct device_node *np, > unsigned int *conf) > { > struct xadc *xadc = iio_priv(indio_dev); > - struct iio_chan_spec *channels, *chan; > + struct iio_chan_spec *iio_xadc_channels, *chan; > struct device_node *chan_node, *child; > unsigned int num_channels; > const char *external_mux; > @@ -1083,12 +1083,13 @@ static int xadc_parse_dt(struct iio_dev *indio_dev, struct device_node *np, > *conf |= XADC_CONF0_MUX | XADC_CONF0_CHAN(ext_mux_chan); > } > > - channels = kmemdup(xadc_channels, sizeof(xadc_channels), GFP_KERNEL); > - if (!channels) > + iio_xadc_channels = kmemdup(xadc_channels, sizeof(xadc_channels), > + GFP_KERNEL); > + if (!iio_xadc_channels) > return -ENOMEM; > > num_channels = 9; > - chan = &channels[9]; > + chan = &iio_xadc_channels[9]; > > chan_node = of_get_child_by_name(np, "xlnx,channels"); > if (chan_node) { > @@ -1119,11 +1120,12 @@ static int xadc_parse_dt(struct iio_dev *indio_dev, struct device_node *np, > of_node_put(chan_node); > > indio_dev->num_channels = num_channels; > - indio_dev->channels = krealloc(channels, sizeof(*channels) * > - num_channels, GFP_KERNEL); > + indio_dev->channels = krealloc(iio_xadc_channels, > + sizeof(*iio_xadc_channels) * > + num_channels, GFP_KERNEL); > /* If we can't resize the channels array, just use the original */ > if (!indio_dev->channels) > - indio_dev->channels = channels; > + indio_dev->channels = iio_xadc_channels; > > return 0; > } >
Hi Lars, > -----Original Message----- > From: Lars-Peter Clausen [mailto:lars@metafoo.de] > Sent: Wednesday, July 18, 2018 4:49 PM > To: Manish Narani <MNARANI@xilinx.com>; jic23@kernel.org; > knaack.h@gmx.de; pmeerw@pmeerw.net; Michal Simek > <michals@xilinx.com>; linux-iio@vger.kernel.org; linux-arm- > kernel@lists.infradead.org; linux-kernel@vger.kernel.org > Cc: Anirudha Sarangi <anirudh@xilinx.com>; Srinivas Goud > <sgoud@xilinx.com>; Joe Perches <joe@perches.com> > Subject: Re: [PATCH 1/4] iio: adc: xilinx: Rename 'channels' variable name to > 'iio_xadc_channels' > > On 07/18/2018 01:12 PM, Manish Narani wrote: > > This patch fix the following checkpatch warning in xadc driver. > > - Reusing the krealloc arg is almost always a bug. > > > > Renamed the 'channels' variable as 'iio_xadc_channels' to fix the > > above warning. > > > > This is a bug in checkpatch and should be fixed in checkpatch. The code is not > actually re-using the parameter. channels and xadc_channels are independent > variables, just checkpatch somehow does not realize this. I agree with you on this. Initially I presumed the checkpatch to be giving genuine warning but now I am sure that this is certainly the checkpatch script issue. In that case should we keep this code change? Please suggest. Thanks, Manish > > > Signed-off-by: Manish Narani <manish.narani@xilinx.com> > > --- > > drivers/iio/adc/xilinx-xadc-core.c | 16 +++++++++------- > > 1 file changed, 9 insertions(+), 7 deletions(-) > > > > diff --git a/drivers/iio/adc/xilinx-xadc-core.c > > b/drivers/iio/adc/xilinx-xadc-core.c > > index d4f21d1..27b45df 100644 > > --- a/drivers/iio/adc/xilinx-xadc-core.c > > +++ b/drivers/iio/adc/xilinx-xadc-core.c > > @@ -1040,7 +1040,7 @@ static int xadc_parse_dt(struct iio_dev *indio_dev, > struct device_node *np, > > unsigned int *conf) > > { > > struct xadc *xadc = iio_priv(indio_dev); > > - struct iio_chan_spec *channels, *chan; > > + struct iio_chan_spec *iio_xadc_channels, *chan; > > struct device_node *chan_node, *child; > > unsigned int num_channels; > > const char *external_mux; > > @@ -1083,12 +1083,13 @@ static int xadc_parse_dt(struct iio_dev > *indio_dev, struct device_node *np, > > *conf |= XADC_CONF0_MUX | > XADC_CONF0_CHAN(ext_mux_chan); > > } > > > > - channels = kmemdup(xadc_channels, sizeof(xadc_channels), > GFP_KERNEL); > > - if (!channels) > > + iio_xadc_channels = kmemdup(xadc_channels, sizeof(xadc_channels), > > + GFP_KERNEL); > > + if (!iio_xadc_channels) > > return -ENOMEM; > > > > num_channels = 9; > > - chan = &channels[9]; > > + chan = &iio_xadc_channels[9]; > > > > chan_node = of_get_child_by_name(np, "xlnx,channels"); > > if (chan_node) { > > @@ -1119,11 +1120,12 @@ static int xadc_parse_dt(struct iio_dev > *indio_dev, struct device_node *np, > > of_node_put(chan_node); > > > > indio_dev->num_channels = num_channels; > > - indio_dev->channels = krealloc(channels, sizeof(*channels) * > > - num_channels, GFP_KERNEL); > > + indio_dev->channels = krealloc(iio_xadc_channels, > > + sizeof(*iio_xadc_channels) * > > + num_channels, GFP_KERNEL); > > /* If we can't resize the channels array, just use the original */ > > if (!indio_dev->channels) > > - indio_dev->channels = channels; > > + indio_dev->channels = iio_xadc_channels; > > > > return 0; > > } > >
On 18.7.2018 13:18, Lars-Peter Clausen wrote: > On 07/18/2018 01:12 PM, Manish Narani wrote: >> This patch fix the following checkpatch warning in xadc driver. >> - Reusing the krealloc arg is almost always a bug. >> >> Renamed the 'channels' variable as 'iio_xadc_channels' to fix the above >> warning. >> > > This is a bug in checkpatch and should be fixed in checkpatch. The code is > not actually re-using the parameter. channels and xadc_channels are > independent variables, just checkpatch somehow does not realize this. I think it should be fine to have this patch in tree. Change in commit message to reflect this should be enough. But that's just view. Thanks, Michal
On Thu, 19 Jul 2018 10:22:07 +0200 Michal Simek <michal.simek@xilinx.com> wrote: > On 18.7.2018 13:18, Lars-Peter Clausen wrote: > > On 07/18/2018 01:12 PM, Manish Narani wrote: > >> This patch fix the following checkpatch warning in xadc driver. > >> - Reusing the krealloc arg is almost always a bug. > >> > >> Renamed the 'channels' variable as 'iio_xadc_channels' to fix the above > >> warning. > >> > > > > This is a bug in checkpatch and should be fixed in checkpatch. The code is > > not actually re-using the parameter. channels and xadc_channels are > > independent variables, just checkpatch somehow does not realize this. > > I think it should be fine to have this patch in tree. Change in commit > message to reflect this should be enough. But that's just view. Let's wait and see if Joe has time to take a look at this. Might be better to fix checkpatch if it's not too hard! Jonathan > > Thanks, > Michal > -- > To unsubscribe from this list: send the line "unsubscribe linux-iio" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, 2018-07-21 at 17:18 +0100, Jonathan Cameron wrote: > On Thu, 19 Jul 2018 10:22:07 +0200 > Michal Simek <michal.simek@xilinx.com> wrote: > > > On 18.7.2018 13:18, Lars-Peter Clausen wrote: > > > On 07/18/2018 01:12 PM, Manish Narani wrote: > > > > This patch fix the following checkpatch warning in xadc driver. > > > > - Reusing the krealloc arg is almost always a bug. > > > > > > > > Renamed the 'channels' variable as 'iio_xadc_channels' to fix the above > > > > warning. > > > > > > > > > > This is a bug in checkpatch and should be fixed in checkpatch. The code is > > > not actually re-using the parameter. channels and xadc_channels are > > > independent variables, just checkpatch somehow does not realize this. > > > > I think it should be fine to have this patch in tree. Change in commit > > message to reflect this should be enough. But that's just view. > > Let's wait and see if Joe has time to take a look at this. Might be better > to fix checkpatch if it's not too hard! I submitted a proposed patch. I believe it works. https://patchwork.kernel.org/patch/10533011/
On Sat, 21 Jul 2018 09:22:05 -0700 Joe Perches <joe@perches.com> wrote: > On Sat, 2018-07-21 at 17:18 +0100, Jonathan Cameron wrote: > > On Thu, 19 Jul 2018 10:22:07 +0200 > > Michal Simek <michal.simek@xilinx.com> wrote: > > > > > On 18.7.2018 13:18, Lars-Peter Clausen wrote: > > > > On 07/18/2018 01:12 PM, Manish Narani wrote: > > > > > This patch fix the following checkpatch warning in xadc driver. > > > > > - Reusing the krealloc arg is almost always a bug. > > > > > > > > > > Renamed the 'channels' variable as 'iio_xadc_channels' to fix the above > > > > > warning. > > > > > > > > > > > > > This is a bug in checkpatch and should be fixed in checkpatch. The code is > > > > not actually re-using the parameter. channels and xadc_channels are > > > > independent variables, just checkpatch somehow does not realize this. > > > > > > I think it should be fine to have this patch in tree. Change in commit > > > message to reflect this should be enough. But that's just view. > > > > Let's wait and see if Joe has time to take a look at this. Might be better > > to fix checkpatch if it's not too hard! > > I submitted a proposed patch. > I believe it works. > > https://patchwork.kernel.org/patch/10533011/ > Thanks! Should have known you would already have it covered :) My perl foo goes no where near figuring out how that magic works! Jonathan
diff --git a/drivers/iio/adc/xilinx-xadc-core.c b/drivers/iio/adc/xilinx-xadc-core.c index d4f21d1..27b45df 100644 --- a/drivers/iio/adc/xilinx-xadc-core.c +++ b/drivers/iio/adc/xilinx-xadc-core.c @@ -1040,7 +1040,7 @@ static int xadc_parse_dt(struct iio_dev *indio_dev, struct device_node *np, unsigned int *conf) { struct xadc *xadc = iio_priv(indio_dev); - struct iio_chan_spec *channels, *chan; + struct iio_chan_spec *iio_xadc_channels, *chan; struct device_node *chan_node, *child; unsigned int num_channels; const char *external_mux; @@ -1083,12 +1083,13 @@ static int xadc_parse_dt(struct iio_dev *indio_dev, struct device_node *np, *conf |= XADC_CONF0_MUX | XADC_CONF0_CHAN(ext_mux_chan); } - channels = kmemdup(xadc_channels, sizeof(xadc_channels), GFP_KERNEL); - if (!channels) + iio_xadc_channels = kmemdup(xadc_channels, sizeof(xadc_channels), + GFP_KERNEL); + if (!iio_xadc_channels) return -ENOMEM; num_channels = 9; - chan = &channels[9]; + chan = &iio_xadc_channels[9]; chan_node = of_get_child_by_name(np, "xlnx,channels"); if (chan_node) { @@ -1119,11 +1120,12 @@ static int xadc_parse_dt(struct iio_dev *indio_dev, struct device_node *np, of_node_put(chan_node); indio_dev->num_channels = num_channels; - indio_dev->channels = krealloc(channels, sizeof(*channels) * - num_channels, GFP_KERNEL); + indio_dev->channels = krealloc(iio_xadc_channels, + sizeof(*iio_xadc_channels) * + num_channels, GFP_KERNEL); /* If we can't resize the channels array, just use the original */ if (!indio_dev->channels) - indio_dev->channels = channels; + indio_dev->channels = iio_xadc_channels; return 0; }
This patch fix the following checkpatch warning in xadc driver. - Reusing the krealloc arg is almost always a bug. Renamed the 'channels' variable as 'iio_xadc_channels' to fix the above warning. Signed-off-by: Manish Narani <manish.narani@xilinx.com> --- drivers/iio/adc/xilinx-xadc-core.c | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-)