[PATCH v3 0/3] Remove platform data and introduce DT bindings

[PATCH v3 0/3] Remove platform data and introduce DT bindings

[Shreeya Patel]

This patchset introduces the use of device tree bindings
for setting up the irq_type and removes the usage of platform data.
Also, code related to interrupt is moved to the new function
adt7316_setup_irq to unclutter the code in adt7316_probe().
A dev_err() message is added to give more details about the error.

Shreeya Patel (3):
  Staging: iio: adt7316: Use device tree data to assign irq_type
  Staging: iio: adt7316: Move interrupt related code
  Staging: iio: adt7316: Add a dev_err() message

 drivers/staging/iio/addac/adt7316.c | 52 +++++++++++++++++++++--------
 1 file changed, 38 insertions(+), 14 deletions(-)

-- 
2.17.1

[PATCH v3 1/3] Staging: iio: adt7316: Use device tree data to assign irq_type

[Shreeya Patel]

ADT7316 driver no more uses platform data and hence use device tree
data instead of platform data for assigning irq_type field.
Switch case figures out the type of irq and if it's the default case
then assign the default value to the irq_type i.e.
irq_type = IRQF_TRIGGER_LOW
All this is implemented in a new function called adt7316_setup_irq.

Signed-off-by: Shreeya Patel <shreeya.patel23498@gmail.com>
---
 drivers/staging/iio/addac/adt7316.c | 29 +++++++++++++++++++++++++----
 1 file changed, 25 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/iio/addac/adt7316.c b/drivers/staging/iio/addac/adt7316.c
index 9c72538baf9e..86b2c3d53588 100644
--- a/drivers/staging/iio/addac/adt7316.c
+++ b/drivers/staging/iio/addac/adt7316.c
@@ -1807,6 +1807,29 @@ static irqreturn_t adt7316_event_handler(int irq, void *private)
 	return IRQ_HANDLED;
 }
 
+static int adt7316_setup_irq(struct device *dev, int irq)
+{
+	int irq_type;
+
+	irq_type = irqd_get_trigger_type(irq_get_irq_data(irq));
+
+	switch (irq_type) {
+	case IRQF_TRIGGER_HIGH:
+	case IRQF_TRIGGER_RISING:
+		break;
+	case IRQF_TRIGGER_LOW:
+	case IRQF_TRIGGER_FALLING:
+		break;
+	default:
+		dev_info(dev, "mode %d unsupported, using IRQF_TRIGGER_LOW\n",
+			 irq_type);
+		irq_type = IRQF_TRIGGER_LOW;
+		break;
+	}
+
+	return irq_type;
+}
+
 /*
  * Show mask of enabled interrupts in Hex.
  */
@@ -2101,8 +2124,7 @@ int adt7316_probe(struct device *dev, struct adt7316_bus *bus,
 {
 	struct adt7316_chip_info *chip;
 	struct iio_dev *indio_dev;
-	unsigned short *adt7316_platform_data = dev->platform_data;
-	int irq_type = IRQF_TRIGGER_LOW;
+	int irq_type;
 	int ret = 0;
 
 	indio_dev = devm_iio_device_alloc(dev, sizeof(*chip));
@@ -2146,8 +2168,7 @@ int adt7316_probe(struct device *dev, struct adt7316_bus *bus,
 	indio_dev->modes = INDIO_DIRECT_MODE;
 
 	if (chip->bus.irq > 0) {
-		if (adt7316_platform_data[0])
-			irq_type = adt7316_platform_data[0];
+		irq_type = adt7316_setup_irq(dev, chip->bus.irq);
 
 		ret = devm_request_threaded_irq(dev, chip->bus.irq,
 						NULL,
-- 
2.17.1

[PATCH v3 2/3] Staging: iio: adt7316: Move interrupt related code

[Shreeya Patel]

There is a function adt7316_irq_setup() where irq_type is being
set. It would be good to move devm_request_threaded_irq() function
and assignment of chip->config1 in adt7316_irq_setup() to unclutter
the code in probe function.

Signed-off-by: Shreeya Patel <shreeya.patel23498@gmail.com>
---
 drivers/staging/iio/addac/adt7316.c | 34 ++++++++++++++---------------
 1 file changed, 17 insertions(+), 17 deletions(-)

diff --git a/drivers/staging/iio/addac/adt7316.c b/drivers/staging/iio/addac/adt7316.c
index 86b2c3d53588..97dd48153293 100644
--- a/drivers/staging/iio/addac/adt7316.c
+++ b/drivers/staging/iio/addac/adt7316.c
@@ -1807,11 +1807,12 @@ static irqreturn_t adt7316_event_handler(int irq, void *private)
 	return IRQ_HANDLED;
 }
 
-static int adt7316_setup_irq(struct device *dev, int irq)
+static int adt7316_setup_irq(struct iio_dev *indio_dev)
 {
-	int irq_type;
+	struct adt7316_chip_info *chip = iio_priv(indio_dev);
+	int irq_type, ret;
 
-	irq_type = irqd_get_trigger_type(irq_get_irq_data(irq));
+	irq_type = irqd_get_trigger_type(irq_get_irq_data(chip->bus.irq));
 
 	switch (irq_type) {
 	case IRQF_TRIGGER_HIGH:
@@ -1821,13 +1822,23 @@ static int adt7316_setup_irq(struct device *dev, int irq)
 	case IRQF_TRIGGER_FALLING:
 		break;
 	default:
-		dev_info(dev, "mode %d unsupported, using IRQF_TRIGGER_LOW\n",
+		dev_info(&indio_dev->dev, "mode %d unsupported, using IRQF_TRIGGER_LOW\n",
 			 irq_type);
 		irq_type = IRQF_TRIGGER_LOW;
 		break;
 	}
 
-	return irq_type;
+	ret = devm_request_threaded_irq(&indio_dev->dev, chip->bus.irq,
+					NULL, adt7316_event_handler,
+					irq_type | IRQF_ONESHOT,
+					indio_dev->name, indio_dev);
+	if (ret)
+		return ret;
+
+	if (irq_type & IRQF_TRIGGER_HIGH)
+		chip->config1 |= ADT7316_INT_POLARITY;
+
+	return ret;
 }
 
 /*
@@ -2124,7 +2135,6 @@ int adt7316_probe(struct device *dev, struct adt7316_bus *bus,
 {
 	struct adt7316_chip_info *chip;
 	struct iio_dev *indio_dev;
-	int irq_type;
 	int ret = 0;
 
 	indio_dev = devm_iio_device_alloc(dev, sizeof(*chip));
@@ -2168,19 +2178,9 @@ int adt7316_probe(struct device *dev, struct adt7316_bus *bus,
 	indio_dev->modes = INDIO_DIRECT_MODE;
 
 	if (chip->bus.irq > 0) {
-		irq_type = adt7316_setup_irq(dev, chip->bus.irq);
-
-		ret = devm_request_threaded_irq(dev, chip->bus.irq,
-						NULL,
-						adt7316_event_handler,
-						irq_type | IRQF_ONESHOT,
-						indio_dev->name,
-						indio_dev);
+		ret = adt7316_setup_irq(indio_dev);
 		if (ret)
 			return ret;
-
-		if (irq_type & IRQF_TRIGGER_HIGH)
-			chip->config1 |= ADT7316_INT_POLARITY;
 	}
 
 	ret = chip->bus.write(chip->bus.client, ADT7316_CONFIG1, chip->config1);
-- 
2.17.1

[PATCH v3 3/3] Staging: iio: adt7316: Add a dev_err() message

[Shreeya Patel]

Add a dev_err() message "failed to request irq" for describing
what went wrong when an error contition is statisfied.

Signed-off-by: Shreeya Patel <shreeya.patel23498@gmail.com>
---
 drivers/staging/iio/addac/adt7316.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/iio/addac/adt7316.c b/drivers/staging/iio/addac/adt7316.c
index 97dd48153293..e3eb8ad06403 100644
--- a/drivers/staging/iio/addac/adt7316.c
+++ b/drivers/staging/iio/addac/adt7316.c
@@ -1832,8 +1832,11 @@ static int adt7316_setup_irq(struct iio_dev *indio_dev)
 					NULL, adt7316_event_handler,
 					irq_type | IRQF_ONESHOT,
 					indio_dev->name, indio_dev);
-	if (ret)
+	if (ret) {
+		dev_err(&indio_dev->dev, "failed to request irq %d\n",
+			chip->bus.irq);
 		return ret;
+	}
 
 	if (irq_type & IRQF_TRIGGER_HIGH)
 		chip->config1 |= ADT7316_INT_POLARITY;
-- 
2.17.1

Re: [PATCH v3 2/3] Staging: iio: adt7316: Move interrupt related code

[Jonathan Cameron]

On Sat,  8 Dec 2018 20:46:37 +0530
Shreeya Patel <shreeya.patel23498@gmail.com> wrote:

> There is a function adt7316_irq_setup() where irq_type is being
> set. It would be good to move devm_request_threaded_irq() function
> and assignment of chip->config1 in adt7316_irq_setup() to unclutter
> the code in probe function.
> 
> Signed-off-by: Shreeya Patel <shreeya.patel23498@gmail.com>
As commented below, this didn't end up as tidy as it might have been.
It would I think have been simpler before patch 1 or just merged with it.

Anyhow, I might combine the two whilst applying. However before I do
that I'd like to leave this on list for a few days to let Alex
or others have time for another look before I apply it.

All heading in the right direction!

Thanks,

Jonathan

> ---
>  drivers/staging/iio/addac/adt7316.c | 34 ++++++++++++++---------------
>  1 file changed, 17 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/staging/iio/addac/adt7316.c b/drivers/staging/iio/addac/adt7316.c
> index 86b2c3d53588..97dd48153293 100644
> --- a/drivers/staging/iio/addac/adt7316.c
> +++ b/drivers/staging/iio/addac/adt7316.c
> @@ -1807,11 +1807,12 @@ static irqreturn_t adt7316_event_handler(int irq, void *private)
>  	return IRQ_HANDLED;
>  }
>  
> -static int adt7316_setup_irq(struct device *dev, int irq)
> +static int adt7316_setup_irq(struct iio_dev *indio_dev)

Hmm. This has ended up a lot more complex than ideal due
to the effective two layers of rework.  

I would either have done patches 1 and 2 as one patch or
reordered them so the rework preceded the change
to DT.  It's not that important but it would have lead
to code that was easier to review.


>  {
> -	int irq_type;
> +	struct adt7316_chip_info *chip = iio_priv(indio_dev);
> +	int irq_type, ret;
>  
> -	irq_type = irqd_get_trigger_type(irq_get_irq_data(irq));
> +	irq_type = irqd_get_trigger_type(irq_get_irq_data(chip->bus.irq));
>  
>  	switch (irq_type) {
>  	case IRQF_TRIGGER_HIGH:
> @@ -1821,13 +1822,23 @@ static int adt7316_setup_irq(struct device *dev, int irq)
>  	case IRQF_TRIGGER_FALLING:
>  		break;
>  	default:
> -		dev_info(dev, "mode %d unsupported, using IRQF_TRIGGER_LOW\n",
> +		dev_info(&indio_dev->dev, "mode %d unsupported, using IRQF_TRIGGER_LOW\n",
>  			 irq_type);
>  		irq_type = IRQF_TRIGGER_LOW;
>  		break;
>  	}
>  
> -	return irq_type;
> +	ret = devm_request_threaded_irq(&indio_dev->dev, chip->bus.irq,
> +					NULL, adt7316_event_handler,
> +					irq_type | IRQF_ONESHOT,
> +					indio_dev->name, indio_dev);
> +	if (ret)
> +		return ret;
> +
> +	if (irq_type & IRQF_TRIGGER_HIGH)
> +		chip->config1 |= ADT7316_INT_POLARITY;
> +
> +	return ret;
>  }
>  
>  /*
> @@ -2124,7 +2135,6 @@ int adt7316_probe(struct device *dev, struct adt7316_bus *bus,
>  {
>  	struct adt7316_chip_info *chip;
>  	struct iio_dev *indio_dev;
> -	int irq_type;
>  	int ret = 0;
>  
>  	indio_dev = devm_iio_device_alloc(dev, sizeof(*chip));
> @@ -2168,19 +2178,9 @@ int adt7316_probe(struct device *dev, struct adt7316_bus *bus,
>  	indio_dev->modes = INDIO_DIRECT_MODE;
>  
>  	if (chip->bus.irq > 0) {
> -		irq_type = adt7316_setup_irq(dev, chip->bus.irq);
> -
> -		ret = devm_request_threaded_irq(dev, chip->bus.irq,
> -						NULL,
> -						adt7316_event_handler,
> -						irq_type | IRQF_ONESHOT,
> -						indio_dev->name,
> -						indio_dev);
> +		ret = adt7316_setup_irq(indio_dev);
>  		if (ret)
>  			return ret;
> -
> -		if (irq_type & IRQF_TRIGGER_HIGH)
> -			chip->config1 |= ADT7316_INT_POLARITY;
>  	}
>  
>  	ret = chip->bus.write(chip->bus.client, ADT7316_CONFIG1, chip->config1);

Re: [PATCH v3 2/3] Staging: iio: adt7316: Move interrupt related code

[Shreeya Patel]

On Sat, 2018-12-08 at 16:12 +0000, Jonathan Cameron wrote:
> On Sat,  8 Dec 2018 20:46:37 +0530
> Shreeya Patel <shreeya.patel23498@gmail.com> wrote:
> 
> > There is a function adt7316_irq_setup() where irq_type is being
> > set. It would be good to move devm_request_threaded_irq() function
> > and assignment of chip->config1 in adt7316_irq_setup() to unclutter
> > the code in probe function.
> > 
> > Signed-off-by: Shreeya Patel <shreeya.patel23498@gmail.com>
> 
> As commented below, this didn't end up as tidy as it might have been.
> It would I think have been simpler before patch 1 or just merged with
> it.
> 
As I was introducing a new function named "adt7316_setup_irq" so I
thought patch 1 should come first because we are setting up the
irq_type there.
But yes, this made the code complex to review.

I didn't merge both patches because both the patches were having
different changes. If I would have done that then there was a
possibility where someone would have said to split the patches.

> Anyhow, I might combine the two whilst applying. However before I do
> that I'd like to leave this on list for a few days to let Alex
> or others have time for another look before I apply it.
> 

It's ok, I'll merge both patches and send as a v4 to you. I'll send it
after 3-4 days so we can get other reviews by that time if there are
any to come.

My vacation has started so I'll work faster now.

> All heading in the right direction!
> 
> Thanks,
> 
> Jonathan
> 
> > ---
> >  drivers/staging/iio/addac/adt7316.c | 34 ++++++++++++++-----------
> > ----
> >  1 file changed, 17 insertions(+), 17 deletions(-)
> > 
> > diff --git a/drivers/staging/iio/addac/adt7316.c
> > b/drivers/staging/iio/addac/adt7316.c
> > index 86b2c3d53588..97dd48153293 100644
> > --- a/drivers/staging/iio/addac/adt7316.c
> > +++ b/drivers/staging/iio/addac/adt7316.c
> > @@ -1807,11 +1807,12 @@ static irqreturn_t
> > adt7316_event_handler(int irq, void *private)
> >  	return IRQ_HANDLED;
> >  }
> >  
> > -static int adt7316_setup_irq(struct device *dev, int irq)
> > +static int adt7316_setup_irq(struct iio_dev *indio_dev)
> 
> Hmm. This has ended up a lot more complex than ideal due
> to the effective two layers of rework.  
> 
> I would either have done patches 1 and 2 as one patch or
> reordered them so the rework preceded the change
> to DT.  It's not that important but it would have lead
> to code that was easier to review.
> 
> 
> >  {
> > -	int irq_type;
> > +	struct adt7316_chip_info *chip = iio_priv(indio_dev);
> > +	int irq_type, ret;
> >  
> > -	irq_type = irqd_get_trigger_type(irq_get_irq_data(irq));
> > +	irq_type = irqd_get_trigger_type(irq_get_irq_data(chip-
> > >bus.irq));
> >  
> >  	switch (irq_type) {
> >  	case IRQF_TRIGGER_HIGH:
> > @@ -1821,13 +1822,23 @@ static int adt7316_setup_irq(struct device
> > *dev, int irq)
> >  	case IRQF_TRIGGER_FALLING:
> >  		break;
> >  	default:
> > -		dev_info(dev, "mode %d unsupported, using
> > IRQF_TRIGGER_LOW\n",
> > +		dev_info(&indio_dev->dev, "mode %d unsupported,
> > using IRQF_TRIGGER_LOW\n",
> >  			 irq_type);
> >  		irq_type = IRQF_TRIGGER_LOW;
> >  		break;
> >  	}
> >  
> > -	return irq_type;
> > +	ret = devm_request_threaded_irq(&indio_dev->dev, chip-
> > >bus.irq,
> > +					NULL,
> > adt7316_event_handler,
> > +					irq_type | IRQF_ONESHOT,
> > +					indio_dev->name,
> > indio_dev);
> > +	if (ret)
> > +		return ret;
> > +
> > +	if (irq_type & IRQF_TRIGGER_HIGH)
> > +		chip->config1 |= ADT7316_INT_POLARITY;
> > +
> > +	return ret;
> >  }
> >  
> >  /*
> > @@ -2124,7 +2135,6 @@ int adt7316_probe(struct device *dev, struct
> > adt7316_bus *bus,
> >  {
> >  	struct adt7316_chip_info *chip;
> >  	struct iio_dev *indio_dev;
> > -	int irq_type;
> >  	int ret = 0;
> >  
> >  	indio_dev = devm_iio_device_alloc(dev, sizeof(*chip));
> > @@ -2168,19 +2178,9 @@ int adt7316_probe(struct device *dev, struct
> > adt7316_bus *bus,
> >  	indio_dev->modes = INDIO_DIRECT_MODE;
> >  
> >  	if (chip->bus.irq > 0) {
> > -		irq_type = adt7316_setup_irq(dev, chip->bus.irq);
> > -
> > -		ret = devm_request_threaded_irq(dev, chip-
> > >bus.irq,
> > -						NULL,
> > -						adt7316_event_hand
> > ler,
> > -						irq_type |
> > IRQF_ONESHOT,
> > -						indio_dev->name,
> > -						indio_dev);
> > +		ret = adt7316_setup_irq(indio_dev);
> >  		if (ret)
> >  			return ret;
> > -
> > -		if (irq_type & IRQF_TRIGGER_HIGH)
> > -			chip->config1 |= ADT7316_INT_POLARITY;
> >  	}
> >  
> >  	ret = chip->bus.write(chip->bus.client, ADT7316_CONFIG1,
> > chip->config1);
> 
> 

Re: [PATCH v3 2/3] Staging: iio: adt7316: Move interrupt related code

[Dan Carpenter]

On Sat, Dec 08, 2018 at 08:46:37PM +0530, Shreeya Patel wrote:
> -	return irq_type;
> +	ret = devm_request_threaded_irq(&indio_dev->dev, chip->bus.irq,
> +					NULL, adt7316_event_handler,
> +					irq_type | IRQF_ONESHOT,
> +					indio_dev->name, indio_dev);
> +	if (ret)
> +		return ret;
> +
> +	if (irq_type & IRQF_TRIGGER_HIGH)
> +		chip->config1 |= ADT7316_INT_POLARITY;
> +
> +	return ret;

return 0; here.

>  }
>  
>  /*

regards,
dan carpenter