[PATCH v2 5/5] Staging: iio: adt7316: Use device tree data to assign irq_type

[PATCH v2 5/5] 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

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

diff --git a/drivers/staging/iio/addac/adt7316.c b/drivers/staging/iio/addac/adt7316.c
index 9c72538baf9e..c647875a64f5 100644
--- a/drivers/staging/iio/addac/adt7316.c
+++ b/drivers/staging/iio/addac/adt7316.c
@@ -2101,8 +2101,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 +2145,22 @@ 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 =
+			irqd_get_trigger_type(irq_get_irq_data(chip->bus.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;
+		}
 
 		ret = devm_request_threaded_irq(dev, chip->bus.irq,
 						NULL,
-- 
2.17.1

Re: [PATCH v2 5/5] Staging: iio: adt7316: Use device tree data to assign irq_type

[Ardelean, Alexandru]

On Tue, 2018-11-20 at 22:30 +0530, Shreeya Patel wrote:
> 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
> 

1 comment inline

> Signed-off-by: Shreeya Patel <shreeya.patel23498@gmail.com>
> ---
>  drivers/staging/iio/addac/adt7316.c | 21 +++++++++++++++++----
>  1 file changed, 17 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/staging/iio/addac/adt7316.c
> b/drivers/staging/iio/addac/adt7316.c
> index 9c72538baf9e..c647875a64f5 100644
> --- a/drivers/staging/iio/addac/adt7316.c
> +++ b/drivers/staging/iio/addac/adt7316.c
> @@ -2101,8 +2101,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 +2145,22 @@ 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 =
> +			irqd_get_trigger_type(irq_get_irq_data(chip-
> >bus.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;
> +		}

It would be an idea to move this part [together with
devm_request_threaded_irq()] into a "adt7316_setup_irq()" function. To un-
clutter the code in the adt7316_probe() function.

>  
>  		ret = devm_request_threaded_irq(dev, chip->bus.irq,
>  						NULL,

Re: [PATCH v2 5/5] Staging: iio: adt7316: Use device tree data to assign irq_type

[Ardelean, Alexandru]

T24gVHVlLCAyMDE4LTExLTIwIGF0IDIyOjMwICswNTMwLCBTaHJlZXlhIFBhdGVsIHdyb3RlOg0K
PiBBRFQ3MzE2IGRyaXZlciBubyBtb3JlIHVzZXMgcGxhdGZvcm0gZGF0YSBhbmQgaGVuY2UgdXNl
IGRldmljZSB0cmVlDQo+IGRhdGEgaW5zdGVhZCBvZiBwbGF0Zm9ybSBkYXRhIGZvciBhc3NpZ25p
bmcgaXJxX3R5cGUgZmllbGQuDQo+IFN3aXRjaCBjYXNlIGZpZ3VyZXMgb3V0IHRoZSB0eXBlIG9m
IGlycSBhbmQgaWYgaXQncyB0aGUgZGVmYXVsdCBjYXNlDQo+IHRoZW4gYXNzaWduIHRoZSBkZWZh
dWx0IHZhbHVlIHRvIHRoZSBpcnFfdHlwZSBpLmUuDQo+IGlycV90eXBlID0gSVJRRl9UUklHR0VS
X0xPVw0KPiANCg0KMSBjb21tZW50IGlubGluZQ0KDQo+IFNpZ25lZC1vZmYtYnk6IFNocmVleWEg
UGF0ZWwgPHNocmVleWEucGF0ZWwyMzQ5OEBnbWFpbC5jb20+DQo+IC0tLQ0KPiAgZHJpdmVycy9z
dGFnaW5nL2lpby9hZGRhYy9hZHQ3MzE2LmMgfCAyMSArKysrKysrKysrKysrKysrKy0tLS0NCj4g
IDEgZmlsZSBjaGFuZ2VkLCAxNyBpbnNlcnRpb25zKCspLCA0IGRlbGV0aW9ucygtKQ0KPiANCj4g
ZGlmZiAtLWdpdCBhL2RyaXZlcnMvc3RhZ2luZy9paW8vYWRkYWMvYWR0NzMxNi5jDQo+IGIvZHJp
dmVycy9zdGFnaW5nL2lpby9hZGRhYy9hZHQ3MzE2LmMNCj4gaW5kZXggOWM3MjUzOGJhZjllLi5j
NjQ3ODc1YTY0ZjUgMTAwNjQ0DQo+IC0tLSBhL2RyaXZlcnMvc3RhZ2luZy9paW8vYWRkYWMvYWR0
NzMxNi5jDQo+ICsrKyBiL2RyaXZlcnMvc3RhZ2luZy9paW8vYWRkYWMvYWR0NzMxNi5jDQo+IEBA
IC0yMTAxLDggKzIxMDEsNyBAQCBpbnQgYWR0NzMxNl9wcm9iZShzdHJ1Y3QgZGV2aWNlICpkZXYs
IHN0cnVjdA0KPiBhZHQ3MzE2X2J1cyAqYnVzLA0KPiAgew0KPiAgCXN0cnVjdCBhZHQ3MzE2X2No
aXBfaW5mbyAqY2hpcDsNCj4gIAlzdHJ1Y3QgaWlvX2RldiAqaW5kaW9fZGV2Ow0KPiAtCXVuc2ln
bmVkIHNob3J0ICphZHQ3MzE2X3BsYXRmb3JtX2RhdGEgPSBkZXYtPnBsYXRmb3JtX2RhdGE7DQo+
IC0JaW50IGlycV90eXBlID0gSVJRRl9UUklHR0VSX0xPVzsNCj4gKwlpbnQgaXJxX3R5cGU7DQo+
ICAJaW50IHJldCA9IDA7DQo+ICANCj4gIAlpbmRpb19kZXYgPSBkZXZtX2lpb19kZXZpY2VfYWxs
b2MoZGV2LCBzaXplb2YoKmNoaXApKTsNCj4gQEAgLTIxNDYsOCArMjE0NSwyMiBAQCBpbnQgYWR0
NzMxNl9wcm9iZShzdHJ1Y3QgZGV2aWNlICpkZXYsIHN0cnVjdA0KPiBhZHQ3MzE2X2J1cyAqYnVz
LA0KPiAgCWluZGlvX2Rldi0+bW9kZXMgPSBJTkRJT19ESVJFQ1RfTU9ERTsNCj4gIA0KPiAgCWlm
IChjaGlwLT5idXMuaXJxID4gMCkgew0KPiAtCQlpZiAoYWR0NzMxNl9wbGF0Zm9ybV9kYXRhWzBd
KQ0KPiAtCQkJaXJxX3R5cGUgPSBhZHQ3MzE2X3BsYXRmb3JtX2RhdGFbMF07DQo+ICsJCWlycV90
eXBlID0NCj4gKwkJCWlycWRfZ2V0X3RyaWdnZXJfdHlwZShpcnFfZ2V0X2lycV9kYXRhKGNoaXAt
DQo+ID5idXMuaXJxKSk7DQo+ICsNCj4gKwkJc3dpdGNoIChpcnFfdHlwZSkgew0KPiArCQljYXNl
IElSUUZfVFJJR0dFUl9ISUdIOg0KPiArCQljYXNlIElSUUZfVFJJR0dFUl9SSVNJTkc6DQo+ICsJ
CQlicmVhazsNCj4gKwkJY2FzZSBJUlFGX1RSSUdHRVJfTE9XOg0KPiArCQljYXNlIElSUUZfVFJJ
R0dFUl9GQUxMSU5HOg0KPiArCQkJYnJlYWs7DQo+ICsJCWRlZmF1bHQ6DQo+ICsJCQlkZXZfaW5m
byhkZXYsICJtb2RlICVkIHVuc3VwcG9ydGVkLCB1c2luZw0KPiBJUlFGX1RSSUdHRVJfTE9XXG4i
LA0KPiArCQkJCSBpcnFfdHlwZSk7DQo+ICsJCQlpcnFfdHlwZSA9IElSUUZfVFJJR0dFUl9MT1c7
DQo+ICsJCQlicmVhazsNCj4gKwkJfQ0KDQpJdCB3b3VsZCBiZSBhbiBpZGVhIHRvIG1vdmUgdGhp
cyBwYXJ0IFt0b2dldGhlciB3aXRoDQpkZXZtX3JlcXVlc3RfdGhyZWFkZWRfaXJxKCldIGludG8g
YSAiYWR0NzMxNl9zZXR1cF9pcnEoKSIgZnVuY3Rpb24uIFRvIHVuLQ0KY2x1dHRlciB0aGUgY29k
ZSBpbiB0aGUgYWR0NzMxNl9wcm9iZSgpIGZ1bmN0aW9uLg0KDQo+ICANCj4gIAkJcmV0ID0gZGV2
bV9yZXF1ZXN0X3RocmVhZGVkX2lycShkZXYsIGNoaXAtPmJ1cy5pcnEsDQo+ICAJCQkJCQlOVUxM
LA0K

Re: [PATCH v2 5/5] Staging: iio: adt7316: Use device tree data to assign irq_type

[Shreeya Patel]

On Wed, 2018-11-21 at 08:21 +0000, Ardelean, Alexandru wrote:
> On Tue, 2018-11-20 at 22:30 +0530, Shreeya Patel wrote:
> > 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
> > 
> 
> 1 comment inline
> 
> > Signed-off-by: Shreeya Patel <shreeya.patel23498@gmail.com>
> > ---
> >  drivers/staging/iio/addac/adt7316.c | 21 +++++++++++++++++----
> >  1 file changed, 17 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/staging/iio/addac/adt7316.c
> > b/drivers/staging/iio/addac/adt7316.c
> > index 9c72538baf9e..c647875a64f5 100644
> > --- a/drivers/staging/iio/addac/adt7316.c
> > +++ b/drivers/staging/iio/addac/adt7316.c
> > @@ -2101,8 +2101,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 +2145,22 @@ 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 =
> > +			irqd_get_trigger_type(irq_get_irq_data(chi
> > p-
> > > bus.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;
> > +		}
> 
> It would be an idea to move this part [together with
> devm_request_threaded_irq()] into a "adt7316_setup_irq()" function.
> To un-
> clutter the code in the adt7316_probe() function.
> 

Yes, seems like a good idea!
Even other drivers are doing the same as you told me to do...thanks :)

I'll do the change after Jonathan picks up the other patches and will
wait for some other reviews to come up if there are any.

Thanks

> >  
> >  		ret = devm_request_threaded_irq(dev, chip-
> > >bus.irq,
> >  						NULL,

Re: [PATCH v2 5/5] Staging: iio: adt7316: Use device tree data to assign irq_type

[Jonathan Cameron]

On Wed, 21 Nov 2018 15:02:52 +0530
Shreeya Patel <shreeya.patel23498@gmail.com> wrote:

> On Wed, 2018-11-21 at 08:21 +0000, Ardelean, Alexandru wrote:
> > On Tue, 2018-11-20 at 22:30 +0530, Shreeya Patel wrote:  
> > > 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
> > >   
> > 
> > 1 comment inline
> >   
> > > Signed-off-by: Shreeya Patel <shreeya.patel23498@gmail.com>
> > > ---
> > >  drivers/staging/iio/addac/adt7316.c | 21 +++++++++++++++++----
> > >  1 file changed, 17 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/drivers/staging/iio/addac/adt7316.c
> > > b/drivers/staging/iio/addac/adt7316.c
> > > index 9c72538baf9e..c647875a64f5 100644
> > > --- a/drivers/staging/iio/addac/adt7316.c
> > > +++ b/drivers/staging/iio/addac/adt7316.c
> > > @@ -2101,8 +2101,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 +2145,22 @@ 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 =
> > > +			irqd_get_trigger_type(irq_get_irq_data(chi
> > > p-  
> > > > bus.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;
> > > +		}  
> > 
> > It would be an idea to move this part [together with
> > devm_request_threaded_irq()] into a "adt7316_setup_irq()" function.
> > To un-
> > clutter the code in the adt7316_probe() function.
> >   
> 
> Yes, seems like a good idea!
> Even other drivers are doing the same as you told me to do...thanks :)
> 
> I'll do the change after Jonathan picks up the other patches and will
> wait for some other reviews to come up if there are any.
Agreed. This suggested change is good, so I'll leave this patch for now
on the basis it probably makes sense to do it as a short series focused
on that one element.

Thanks,

Jonathan

> 
> Thanks
> 
> > >  
> > >  		ret = devm_request_threaded_irq(dev, chip-  
> > > >bus.irq,  
> > >  						NULL,