[PATCH 0/4] iio: adc: xilinx: XADC driver enhancements and bug fixes

[PATCH 0/4] iio: adc: xilinx: XADC driver enhancements and bug fixes

[Manish Narani]

This patch series resolves code style problems as reported by code analysis
tools.

Manish Narani (4):
  iio: adc: xilinx: Rename 'channels' variable name to
    'iio_xadc_channels'
  iio: adc: xilinx: Remove dead code from xadc_zynq_setup
  iio: adc: xilinx: Check for return values in clk related functions
  iio: adc: xilinx: Use devm_ functions while requesting irq

 drivers/iio/adc/xilinx-xadc-core.c | 60 ++++++++++++++++++++++++--------------
 1 file changed, 38 insertions(+), 22 deletions(-)

-- 
2.1.1

[PATCH 1/4] iio: adc: xilinx: Rename 'channels' variable name to 'iio_xadc_channels'

[Manish Narani]

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

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;
 }
-- 
2.1.1

Re: [PATCH 1/4] iio: adc: xilinx: Rename 'channels' variable name to 'iio_xadc_channels'

[Lars-Peter Clausen]

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

RE: [PATCH 1/4] iio: adc: xilinx: Rename 'channels' variable name to 'iio_xadc_channels'

[Manish Narani]

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

Re: [PATCH 1/4] iio: adc: xilinx: Rename 'channels' variable name to 'iio_xadc_channels'

[Michal Simek]

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

Re: [PATCH 1/4] iio: adc: xilinx: Rename 'channels' variable name to 'iio_xadc_channels'

[Jonathan Cameron]

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

Re: [PATCH 1/4] iio: adc: xilinx: Rename 'channels' variable name to 'iio_xadc_channels'

[Joe Perches]

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/

Re: [PATCH 1/4] iio: adc: xilinx: Rename 'channels' variable name to 'iio_xadc_channels'

[Jonathan Cameron]

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

[PATCH 2/4] iio: adc: xilinx: Remove dead code from xadc_zynq_setup

[Manish Narani]

This patch removes dead code from xadc_zynq_setup. The condition
"if (tck_rate > XADC_ZYNQ_TCK_RATE_MAX)" cannot be true at any point of
time. There is also an incompatible parameter used in the code.
This patch fixes the same reported by coverity.

Signed-off-by: Manish Narani <manish.narani@xilinx.com>
---
 drivers/iio/adc/xilinx-xadc-core.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/iio/adc/xilinx-xadc-core.c b/drivers/iio/adc/xilinx-xadc-core.c
index 27b45df..248cffa 100644
--- a/drivers/iio/adc/xilinx-xadc-core.c
+++ b/drivers/iio/adc/xilinx-xadc-core.c
@@ -341,8 +341,6 @@ static int xadc_zynq_setup(struct platform_device *pdev,
 
 	pcap_rate = clk_get_rate(xadc->clk);
 
-	if (tck_rate > XADC_ZYNQ_TCK_RATE_MAX)
-		tck_rate = XADC_ZYNQ_TCK_RATE_MAX;
 	if (tck_rate > pcap_rate / 2) {
 		div = 2;
 	} else {
@@ -1045,7 +1043,7 @@ static int xadc_parse_dt(struct iio_dev *indio_dev, struct device_node *np,
 	unsigned int num_channels;
 	const char *external_mux;
 	u32 ext_mux_chan;
-	int reg;
+	u32 reg;
 	int ret;
 
 	*conf = 0;
-- 
2.1.1

Re: [PATCH 2/4] iio: adc: xilinx: Remove dead code from xadc_zynq_setup

[Jonathan Cameron]

On Wed, 18 Jul 2018 16:42:09 +0530
Manish Narani <manish.narani@xilinx.com> wrote:

> This patch removes dead code from xadc_zynq_setup. The condition
> "if (tck_rate > XADC_ZYNQ_TCK_RATE_MAX)" cannot be true at any point of
> time. There is also an incompatible parameter used in the code.
> This patch fixes the same reported by coverity.
> 
> Signed-off-by: Manish Narani <manish.narani@xilinx.com>

Applied to the togreg branch of iio.git and pushed out as testing
for the autobuilders to try and break it.

Thanks,

Jonathan

> ---
>  drivers/iio/adc/xilinx-xadc-core.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/drivers/iio/adc/xilinx-xadc-core.c b/drivers/iio/adc/xilinx-xadc-core.c
> index 27b45df..248cffa 100644
> --- a/drivers/iio/adc/xilinx-xadc-core.c
> +++ b/drivers/iio/adc/xilinx-xadc-core.c
> @@ -341,8 +341,6 @@ static int xadc_zynq_setup(struct platform_device *pdev,
>  
>  	pcap_rate = clk_get_rate(xadc->clk);
>  
> -	if (tck_rate > XADC_ZYNQ_TCK_RATE_MAX)
> -		tck_rate = XADC_ZYNQ_TCK_RATE_MAX;
>  	if (tck_rate > pcap_rate / 2) {
>  		div = 2;
>  	} else {
> @@ -1045,7 +1043,7 @@ static int xadc_parse_dt(struct iio_dev *indio_dev, struct device_node *np,
>  	unsigned int num_channels;
>  	const char *external_mux;
>  	u32 ext_mux_chan;
> -	int reg;
> +	u32 reg;
>  	int ret;
>  
>  	*conf = 0;

[PATCH 3/4] iio: adc: xilinx: Check for return values in clk related functions

[Manish Narani]

This patch adds check for return values from clock related functions.
This was reported by static code analysis tool.

Signed-off-by: Manish Narani <manish.narani@xilinx.com>
---
 drivers/iio/adc/xilinx-xadc-core.c | 24 ++++++++++++++++++++++--
 1 file changed, 22 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/adc/xilinx-xadc-core.c b/drivers/iio/adc/xilinx-xadc-core.c
index 248cffa..47eb364 100644
--- a/drivers/iio/adc/xilinx-xadc-core.c
+++ b/drivers/iio/adc/xilinx-xadc-core.c
@@ -322,6 +322,7 @@ static irqreturn_t xadc_zynq_interrupt_handler(int irq, void *devid)
 
 #define XADC_ZYNQ_TCK_RATE_MAX 50000000
 #define XADC_ZYNQ_IGAP_DEFAULT 20
+#define XADC_ZYNQ_PCAP_RATE_MAX 200000000
 
 static int xadc_zynq_setup(struct platform_device *pdev,
 	struct iio_dev *indio_dev, int irq)
@@ -332,6 +333,7 @@ static int xadc_zynq_setup(struct platform_device *pdev,
 	unsigned int div;
 	unsigned int igap;
 	unsigned int tck_rate;
+	int ret;
 
 	/* TODO: Figure out how to make igap and tck_rate configurable */
 	igap = XADC_ZYNQ_IGAP_DEFAULT;
@@ -341,6 +343,13 @@ static int xadc_zynq_setup(struct platform_device *pdev,
 
 	pcap_rate = clk_get_rate(xadc->clk);
 
+	if (pcap_rate > XADC_ZYNQ_PCAP_RATE_MAX) {
+		ret = clk_set_rate(xadc->clk,
+				   (unsigned long)XADC_ZYNQ_PCAP_RATE_MAX);
+		if (ret)
+			return ret;
+	}
+
 	if (tck_rate > pcap_rate / 2) {
 		div = 2;
 	} else {
@@ -366,6 +375,12 @@ static int xadc_zynq_setup(struct platform_device *pdev,
 			XADC_ZYNQ_CFG_REDGE | XADC_ZYNQ_CFG_WEDGE |
 			tck_div | XADC_ZYNQ_CFG_IGAP(igap));
 
+	if (pcap_rate > XADC_ZYNQ_PCAP_RATE_MAX) {
+		ret = clk_set_rate(xadc->clk, pcap_rate);
+		if (ret)
+			return ret;
+	}
+
 	return 0;
 }
 
@@ -887,6 +902,9 @@ static int xadc_write_raw(struct iio_dev *indio_dev,
 	unsigned long clk_rate = xadc_get_dclk_rate(xadc);
 	unsigned int div;
 
+	if (!clk_rate)
+		return -EINVAL;
+
 	if (info != IIO_CHAN_INFO_SAMP_FREQ)
 		return -EINVAL;
 
@@ -1239,8 +1257,10 @@ static int xadc_probe(struct platform_device *pdev)
 		goto err_free_irq;
 
 	/* Disable all alarms */
-	xadc_update_adc_reg(xadc, XADC_REG_CONF1, XADC_CONF1_ALARM_MASK,
-		XADC_CONF1_ALARM_MASK);
+	ret = xadc_update_adc_reg(xadc, XADC_REG_CONF1, XADC_CONF1_ALARM_MASK,
+				  XADC_CONF1_ALARM_MASK);
+	if (ret)
+		goto err_free_irq;
 
 	/* Set thresholds to min/max */
 	for (i = 0; i < 16; i++) {
-- 
2.1.1

Re: [PATCH 3/4] iio: adc: xilinx: Check for return values in clk related functions

[Lars-Peter Clausen]

On 07/18/2018 01:12 PM, Manish Narani wrote:
> This patch adds check for return values from clock related functions.
> This was reported by static code analysis tool.

This patch seems to do something else.

> 
> Signed-off-by: Manish Narani <manish.narani@xilinx.com>
> ---
>  drivers/iio/adc/xilinx-xadc-core.c | 24 ++++++++++++++++++++++--
>  1 file changed, 22 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iio/adc/xilinx-xadc-core.c b/drivers/iio/adc/xilinx-xadc-core.c
> index 248cffa..47eb364 100644
> --- a/drivers/iio/adc/xilinx-xadc-core.c
> +++ b/drivers/iio/adc/xilinx-xadc-core.c
> @@ -322,6 +322,7 @@ static irqreturn_t xadc_zynq_interrupt_handler(int irq, void *devid)
>  
>  #define XADC_ZYNQ_TCK_RATE_MAX 50000000
>  #define XADC_ZYNQ_IGAP_DEFAULT 20
> +#define XADC_ZYNQ_PCAP_RATE_MAX 200000000
>  
>  static int xadc_zynq_setup(struct platform_device *pdev,
>  	struct iio_dev *indio_dev, int irq)
> @@ -332,6 +333,7 @@ static int xadc_zynq_setup(struct platform_device *pdev,
>  	unsigned int div;
>  	unsigned int igap;
>  	unsigned int tck_rate;
> +	int ret;
>  
>  	/* TODO: Figure out how to make igap and tck_rate configurable */
>  	igap = XADC_ZYNQ_IGAP_DEFAULT;
> @@ -341,6 +343,13 @@ static int xadc_zynq_setup(struct platform_device *pdev,
>  
>  	pcap_rate = clk_get_rate(xadc->clk);
>  
> +	if (pcap_rate > XADC_ZYNQ_PCAP_RATE_MAX) {
> +		ret = clk_set_rate(xadc->clk,
> +				   (unsigned long)XADC_ZYNQ_PCAP_RATE_MAX);
> +		if (ret)
> +			return ret;
> +	}
> +
>  	if (tck_rate > pcap_rate / 2) {
>  		div = 2;
>  	} else {
> @@ -366,6 +375,12 @@ static int xadc_zynq_setup(struct platform_device *pdev,
>  			XADC_ZYNQ_CFG_REDGE | XADC_ZYNQ_CFG_WEDGE |
>  			tck_div | XADC_ZYNQ_CFG_IGAP(igap));
>  
> +	if (pcap_rate > XADC_ZYNQ_PCAP_RATE_MAX) {
> +		ret = clk_set_rate(xadc->clk, pcap_rate);
> +		if (ret)
> +			return ret;
> +	}
> +
>  	return 0;
>  }
>  
> @@ -887,6 +902,9 @@ static int xadc_write_raw(struct iio_dev *indio_dev,
>  	unsigned long clk_rate = xadc_get_dclk_rate(xadc);
>  	unsigned int div;
>  
> +	if (!clk_rate)
> +		return -EINVAL;
> +
>  	if (info != IIO_CHAN_INFO_SAMP_FREQ)
>  		return -EINVAL;
>  
> @@ -1239,8 +1257,10 @@ static int xadc_probe(struct platform_device *pdev)
>  		goto err_free_irq;
>  
>  	/* Disable all alarms */
> -	xadc_update_adc_reg(xadc, XADC_REG_CONF1, XADC_CONF1_ALARM_MASK,
> -		XADC_CONF1_ALARM_MASK);
> +	ret = xadc_update_adc_reg(xadc, XADC_REG_CONF1, XADC_CONF1_ALARM_MASK,
> +				  XADC_CONF1_ALARM_MASK);
> +	if (ret)
> +		goto err_free_irq;
>  
>  	/* Set thresholds to min/max */
>  	for (i = 0; i < 16; i++) {
> 

Re: [PATCH 3/4] iio: adc: xilinx: Check for return values in clk related functions

[Jonathan Cameron]

On Thu, 19 Jul 2018 18:40:26 +0200
Lars-Peter Clausen <lars@metafoo.de> wrote:

> On 07/18/2018 01:12 PM, Manish Narani wrote:
> > This patch adds check for return values from clock related functions.
> > This was reported by static code analysis tool.  
> 
> This patch seems to do something else.
Indeed, definitely doing two things only one of which is mentioned.
Please break it up and resend.

Jonathan

> 
> > 
> > Signed-off-by: Manish Narani <manish.narani@xilinx.com>
> > ---
> >  drivers/iio/adc/xilinx-xadc-core.c | 24 ++++++++++++++++++++++--
> >  1 file changed, 22 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/iio/adc/xilinx-xadc-core.c b/drivers/iio/adc/xilinx-xadc-core.c
> > index 248cffa..47eb364 100644
> > --- a/drivers/iio/adc/xilinx-xadc-core.c
> > +++ b/drivers/iio/adc/xilinx-xadc-core.c
> > @@ -322,6 +322,7 @@ static irqreturn_t xadc_zynq_interrupt_handler(int irq, void *devid)
> >  
> >  #define XADC_ZYNQ_TCK_RATE_MAX 50000000
> >  #define XADC_ZYNQ_IGAP_DEFAULT 20
> > +#define XADC_ZYNQ_PCAP_RATE_MAX 200000000
> >  
> >  static int xadc_zynq_setup(struct platform_device *pdev,
> >  	struct iio_dev *indio_dev, int irq)
> > @@ -332,6 +333,7 @@ static int xadc_zynq_setup(struct platform_device *pdev,
> >  	unsigned int div;
> >  	unsigned int igap;
> >  	unsigned int tck_rate;
> > +	int ret;
> >  
> >  	/* TODO: Figure out how to make igap and tck_rate configurable */
> >  	igap = XADC_ZYNQ_IGAP_DEFAULT;
> > @@ -341,6 +343,13 @@ static int xadc_zynq_setup(struct platform_device *pdev,
> >  
> >  	pcap_rate = clk_get_rate(xadc->clk);
> >  
> > +	if (pcap_rate > XADC_ZYNQ_PCAP_RATE_MAX) {
> > +		ret = clk_set_rate(xadc->clk,
> > +				   (unsigned long)XADC_ZYNQ_PCAP_RATE_MAX);
> > +		if (ret)
> > +			return ret;
> > +	}
> > +
> >  	if (tck_rate > pcap_rate / 2) {
> >  		div = 2;
> >  	} else {
> > @@ -366,6 +375,12 @@ static int xadc_zynq_setup(struct platform_device *pdev,
> >  			XADC_ZYNQ_CFG_REDGE | XADC_ZYNQ_CFG_WEDGE |
> >  			tck_div | XADC_ZYNQ_CFG_IGAP(igap));
> >  
> > +	if (pcap_rate > XADC_ZYNQ_PCAP_RATE_MAX) {
> > +		ret = clk_set_rate(xadc->clk, pcap_rate);
> > +		if (ret)
> > +			return ret;
> > +	}
> > +
> >  	return 0;
> >  }
> >  
> > @@ -887,6 +902,9 @@ static int xadc_write_raw(struct iio_dev *indio_dev,
> >  	unsigned long clk_rate = xadc_get_dclk_rate(xadc);
> >  	unsigned int div;
> >  
> > +	if (!clk_rate)
> > +		return -EINVAL;
> > +
> >  	if (info != IIO_CHAN_INFO_SAMP_FREQ)
> >  		return -EINVAL;
> >  
> > @@ -1239,8 +1257,10 @@ static int xadc_probe(struct platform_device *pdev)
> >  		goto err_free_irq;
> >  
> >  	/* Disable all alarms */
> > -	xadc_update_adc_reg(xadc, XADC_REG_CONF1, XADC_CONF1_ALARM_MASK,
> > -		XADC_CONF1_ALARM_MASK);
> > +	ret = xadc_update_adc_reg(xadc, XADC_REG_CONF1, XADC_CONF1_ALARM_MASK,
> > +				  XADC_CONF1_ALARM_MASK);
> > +	if (ret)
> > +		goto err_free_irq;
> >  
> >  	/* Set thresholds to min/max */
> >  	for (i = 0; i < 16; i++) {
> >   
> 

RE: [PATCH 3/4] iio: adc: xilinx: Check for return values in clk related functions

[Manish Narani]

Hi Jonathan,

Thanks for your comments.

> -----Original Message-----
> From: Jonathan Cameron [mailto:jic23@kernel.org]
> Sent: Saturday, July 21, 2018 9:54 PM
> To: Lars-Peter Clausen <lars@metafoo.de>
> Cc: Manish Narani <MNARANI@xilinx.com>; 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; Anirudha Sarangi <anirudh@xilinx.com>; Srinivas
> Goud <sgoud@xilinx.com>
> Subject: Re: [PATCH 3/4] iio: adc: xilinx: Check for return values in clk related
> functions
> 
> On Thu, 19 Jul 2018 18:40:26 +0200
> Lars-Peter Clausen <lars@metafoo.de> wrote:
> 
> > On 07/18/2018 01:12 PM, Manish Narani wrote:
> > > This patch adds check for return values from clock related functions.
> > > This was reported by static code analysis tool.
> >
> > This patch seems to do something else.
> Indeed, definitely doing two things only one of which is mentioned.
> Please break it up and resend.
I will break it up and resend.

> 
> Jonathan
> 
> >
> > >
> > > Signed-off-by: Manish Narani <manish.narani@xilinx.com>
> > > ---
> > >  drivers/iio/adc/xilinx-xadc-core.c | 24 ++++++++++++++++++++++--
> > >  1 file changed, 22 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/iio/adc/xilinx-xadc-core.c
> > > b/drivers/iio/adc/xilinx-xadc-core.c
> > > index 248cffa..47eb364 100644
> > > --- a/drivers/iio/adc/xilinx-xadc-core.c
> > > +++ b/drivers/iio/adc/xilinx-xadc-core.c
> > > @@ -322,6 +322,7 @@ static irqreturn_t
> > > xadc_zynq_interrupt_handler(int irq, void *devid)
> > >
> > >  #define XADC_ZYNQ_TCK_RATE_MAX 50000000  #define
> > > XADC_ZYNQ_IGAP_DEFAULT 20
> > > +#define XADC_ZYNQ_PCAP_RATE_MAX 200000000
> > >
> > >  static int xadc_zynq_setup(struct platform_device *pdev,
> > >  	struct iio_dev *indio_dev, int irq) @@ -332,6 +333,7 @@ static int
> > > xadc_zynq_setup(struct platform_device *pdev,
> > >  	unsigned int div;
> > >  	unsigned int igap;
> > >  	unsigned int tck_rate;
> > > +	int ret;
> > >
> > >  	/* TODO: Figure out how to make igap and tck_rate configurable */
> > >  	igap = XADC_ZYNQ_IGAP_DEFAULT;
> > > @@ -341,6 +343,13 @@ static int xadc_zynq_setup(struct
> > > platform_device *pdev,
> > >
> > >  	pcap_rate = clk_get_rate(xadc->clk);
> > >
> > > +	if (pcap_rate > XADC_ZYNQ_PCAP_RATE_MAX) {
> > > +		ret = clk_set_rate(xadc->clk,
> > > +				   (unsigned
> long)XADC_ZYNQ_PCAP_RATE_MAX);
> > > +		if (ret)
> > > +			return ret;
> > > +	}
> > > +
> > >  	if (tck_rate > pcap_rate / 2) {
> > >  		div = 2;
> > >  	} else {
> > > @@ -366,6 +375,12 @@ static int xadc_zynq_setup(struct platform_device
> *pdev,
> > >  			XADC_ZYNQ_CFG_REDGE | XADC_ZYNQ_CFG_WEDGE
> |
> > >  			tck_div | XADC_ZYNQ_CFG_IGAP(igap));
> > >
> > > +	if (pcap_rate > XADC_ZYNQ_PCAP_RATE_MAX) {
> > > +		ret = clk_set_rate(xadc->clk, pcap_rate);
> > > +		if (ret)
> > > +			return ret;
> > > +	}
> > > +
> > >  	return 0;
> > >  }
> > >
> > > @@ -887,6 +902,9 @@ static int xadc_write_raw(struct iio_dev *indio_dev,
> > >  	unsigned long clk_rate = xadc_get_dclk_rate(xadc);
> > >  	unsigned int div;
> > >
> > > +	if (!clk_rate)
> > > +		return -EINVAL;
> > > +
> > >  	if (info != IIO_CHAN_INFO_SAMP_FREQ)
> > >  		return -EINVAL;
> > >
> > > @@ -1239,8 +1257,10 @@ static int xadc_probe(struct platform_device
> *pdev)
> > >  		goto err_free_irq;
> > >
> > >  	/* Disable all alarms */
> > > -	xadc_update_adc_reg(xadc, XADC_REG_CONF1,
> XADC_CONF1_ALARM_MASK,
> > > -		XADC_CONF1_ALARM_MASK);
> > > +	ret = xadc_update_adc_reg(xadc, XADC_REG_CONF1,
> XADC_CONF1_ALARM_MASK,
> > > +				  XADC_CONF1_ALARM_MASK);
> > > +	if (ret)
> > > +		goto err_free_irq;
> > >
> > >  	/* Set thresholds to min/max */
> > >  	for (i = 0; i < 16; i++) {
> > >
> >

Thanks,
Manish Narani

[PATCH 4/4] iio: adc: xilinx: Use devm_ functions while requesting irq

[Manish Narani]

This patch adds support for using devm_ functions for requesting irq.
This will let the core free irq itself whenever the error condition
happens in probe or when xadc_remove is called.

Signed-off-by: Manish Narani <manish.narani@xilinx.com>
---
 drivers/iio/adc/xilinx-xadc-core.c | 18 +++++++-----------
 1 file changed, 7 insertions(+), 11 deletions(-)

diff --git a/drivers/iio/adc/xilinx-xadc-core.c b/drivers/iio/adc/xilinx-xadc-core.c
index 47eb364..7cadcc77 100644
--- a/drivers/iio/adc/xilinx-xadc-core.c
+++ b/drivers/iio/adc/xilinx-xadc-core.c
@@ -1229,8 +1229,8 @@ static int xadc_probe(struct platform_device *pdev)
 	if (ret)
 		goto err_clk_disable_unprepare;
 
-	ret = request_irq(irq, xadc->ops->interrupt_handler, 0,
-			dev_name(&pdev->dev), indio_dev);
+	ret = devm_request_irq(&pdev->dev, irq, xadc->ops->interrupt_handler, 0,
+			       dev_name(&pdev->dev), indio_dev);
 	if (ret)
 		goto err_clk_disable_unprepare;
 
@@ -1240,7 +1240,7 @@ static int xadc_probe(struct platform_device *pdev)
 
 	ret = xadc_write_adc_reg(xadc, XADC_REG_CONF0, conf0);
 	if (ret)
-		goto err_free_irq;
+		goto err_clk_disable_unprepare;
 
 	bipolar_mask = 0;
 	for (i = 0; i < indio_dev->num_channels; i++) {
@@ -1250,17 +1250,17 @@ static int xadc_probe(struct platform_device *pdev)
 
 	ret = xadc_write_adc_reg(xadc, XADC_REG_INPUT_MODE(0), bipolar_mask);
 	if (ret)
-		goto err_free_irq;
+		goto err_clk_disable_unprepare;
 	ret = xadc_write_adc_reg(xadc, XADC_REG_INPUT_MODE(1),
 		bipolar_mask >> 16);
 	if (ret)
-		goto err_free_irq;
+		goto err_clk_disable_unprepare;
 
 	/* Disable all alarms */
 	ret = xadc_update_adc_reg(xadc, XADC_REG_CONF1, XADC_CONF1_ALARM_MASK,
 				  XADC_CONF1_ALARM_MASK);
 	if (ret)
-		goto err_free_irq;
+		goto err_clk_disable_unprepare;
 
 	/* Set thresholds to min/max */
 	for (i = 0; i < 16; i++) {
@@ -1281,14 +1281,12 @@ static int xadc_probe(struct platform_device *pdev)
 
 	ret = iio_device_register(indio_dev);
 	if (ret)
-		goto err_free_irq;
+		goto err_clk_disable_unprepare;
 
 	platform_set_drvdata(pdev, indio_dev);
 
 	return 0;
 
-err_free_irq:
-	free_irq(irq, indio_dev);
 err_clk_disable_unprepare:
 	clk_disable_unprepare(xadc->clk);
 err_free_samplerate_trigger:
@@ -1310,7 +1308,6 @@ static int xadc_remove(struct platform_device *pdev)
 {
 	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
 	struct xadc *xadc = iio_priv(indio_dev);
-	int irq = platform_get_irq(pdev, 0);
 
 	iio_device_unregister(indio_dev);
 	if (xadc->ops->flags & XADC_FLAGS_BUFFERED) {
@@ -1318,7 +1315,6 @@ static int xadc_remove(struct platform_device *pdev)
 		iio_trigger_free(xadc->convst_trigger);
 		iio_triggered_buffer_cleanup(indio_dev);
 	}
-	free_irq(irq, indio_dev);
 	clk_disable_unprepare(xadc->clk);
 	cancel_delayed_work(&xadc->zynq_unmask_work);
 	kfree(xadc->data);
-- 
2.1.1

Re: [PATCH 4/4] iio: adc: xilinx: Use devm_ functions while requesting irq

[Lars-Peter Clausen]

> @@ -1310,7 +1308,6 @@ static int xadc_remove(struct platform_device *pdev)
>  {
>  	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
>  	struct xadc *xadc = iio_priv(indio_dev);
> -	int irq = platform_get_irq(pdev, 0);
>  
>  	iio_device_unregister(indio_dev);
>  	if (xadc->ops->flags & XADC_FLAGS_BUFFERED) {
> @@ -1318,7 +1315,6 @@ static int xadc_remove(struct platform_device *pdev)
>  		iio_trigger_free(xadc->convst_trigger);
>  		iio_triggered_buffer_cleanup(indio_dev);
>  	}
> -	free_irq(irq, indio_dev);

This opens up a race condition. The IRQ needs to be freed before any of
these other things below the free_irq() are executed.

>  	clk_disable_unprepare(xadc->clk);
>  	cancel_delayed_work(&xadc->zynq_unmask_work);
>  	kfree(xadc->data);
> 

RE: [PATCH 4/4] iio: adc: xilinx: Use devm_ functions while requesting irq

[Manish Narani]

Hi Lars,

> -----Original Message-----
> From: Lars-Peter Clausen [mailto:lars@metafoo.de]
> Sent: Thursday, July 19, 2018 10:08 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>
> Subject: Re: [PATCH 4/4] iio: adc: xilinx: Use devm_ functions while requesting
> irq
> 
> > @@ -1310,7 +1308,6 @@ static int xadc_remove(struct platform_device
> > *pdev)  {
> >  	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
> >  	struct xadc *xadc = iio_priv(indio_dev);
> > -	int irq = platform_get_irq(pdev, 0);
> >
> >  	iio_device_unregister(indio_dev);
> >  	if (xadc->ops->flags & XADC_FLAGS_BUFFERED) { @@ -1318,7 +1315,6
> @@
> > static int xadc_remove(struct platform_device *pdev)
> >  		iio_trigger_free(xadc->convst_trigger);
> >  		iio_triggered_buffer_cleanup(indio_dev);
> >  	}
> > -	free_irq(irq, indio_dev);
> 
> This opens up a race condition. The IRQ needs to be freed before any of these
> other things below the free_irq() are executed.
Will look into it and send with taking care of the same.

> 
> >  	clk_disable_unprepare(xadc->clk);
> >  	cancel_delayed_work(&xadc->zynq_unmask_work);
> >  	kfree(xadc->data);
> >

Thanks,
Manish