Re: [PATCH v3 4/4] iio: accel: adxl367: add support for INT2 interrupt pin

[Jonathan Cameron]

...

> > +static int adxl367_set_int_map_reg(struct adxl367_state *st, int irq)
> > +{
> > +	int ret;
> > +
> > +	ret = fwnode_irq_get_byname(dev_fwnode(st->dev), "INT1");
> > +	if (ret == -EPROBE_DEFER)
> > +		return ret;

There is only one return value that means the value wasn't there, -ENODATA

For other values we'd ideally return the error.  Bit ugly but something like

	if (ret < 0) {
		if (ret != -ENODATA)
			return ret;
	} else {
		st->int_map_reg = ADXL367_REG_INT1_MAP;
		return ret;
	}



> > +	if (ret > 0) {
> > +		st->int_map_reg = ADXL367_REG_INT1_MAP;
> > +		return ret;
> > +	}
> > +
> > +	ret = fwnode_irq_get_byname(dev_fwnode(st->dev), "INT2");
> > +	if (ret == -EPROBE_DEFER)
> > +		return ret;
> > +	if (ret > 0) {
> > +		st->int_map_reg = ADXL367_REG_INT2_MAP;
> > +		return ret;
> > +	}
> > +
> > +	/* No interrupt-names: default to INT1 for backwards compatibility. */
> > +	st->int_map_reg = ADXL367_REG_INT1_MAP;
> > +	return irq;  
> 
> I guess this works, but it seems like trial and error rather than being
> purposeful. 
> 
> Could probably do something like this too:
> 
> 	if (device_property_present("interrupt-names")) {
> 		ret = device_property_match_string(st->dev, "interrupt-names",
> 						   "INT2");
> 		if (ret < 0)
> 			return ret;
If we get -ENODATA this exits - but that just means we didn't find the property
(the docs aren't great for these functions - could do with improving if anyone
has time!)

> 
> 		/* If INT2 is at index 0, use it, otherwise use INT1. */

As you suggest below, an explicit INT1 match would be better.  Also why do we care about
the index being 0. We just need to provide irq to map_reg for an irq.

> 		st->int_map_reg = ret ? ADXL367_REG_INT2_MAP : ADXL367_REG_INT1_MAP;
> 	} else {
> 		/* No interrupt-names: default to INT1 for backwards compatibility. */
> 		st->int_map_reg = ADXL367_REG_INT1_MAP;
> 	}
> 
> 
> 	ret = fwnode_irq_get_byname(dev_fwnode(st->dev),
> 				    st->int_map_reg == ADXL367_REG_INT1_MAP ?
> 				    "INT1" : "INT2");
> 
> 
> (Could use device_property_match_property_string() instead if we want to
> return error on invalid values instead of assuming INT1.)

Whilst I see your point about trying and failing being a bit ugly that is
how we normally handle optional irqs and I think the code flow ends up
simpler than trying to be more clever. 

I wouldn't mind a helper that lets us find any random irq from a set of
provided names providing us the index and the irq number				
	const char *irq_names[] = { "INT1", "INT2 };
	int index = 0;
	
	ret = fwnode_irq_get_byname_from_array(dev_fwnode(st->dev),
					       irq_names, ARRAY_SIZE(irq_names),
					       &index);
	if (ret < 0) {
		if (ret == -ENODATA) {
			/* Default to INT1 for backwards compat */
			st->int_map_reg = ADXL367_REG_INT1_MAP;
			return irq;
		}
		return ret;
	}

	st->int_map_reg = index ? ADXL367_REG_INT2_MAP : ADXL367_REG_INT1_MAP;

	return ret;				


I haven't checked but such a helper should I think be pretty useful across
a range of IIO drivers and probably beyond.

Obviously that pushes the style of code choice into the helper though so
the above discussion on what is the better choice continues..

Jonathan.
> 
> > +}
> > +
> >  int adxl367_probe(struct device *dev, const struct adxl367_ops *ops,
> >  		  void *context, struct regmap *regmap, int irq)
> >  {
> > @@ -1479,6 +1508,11 @@ int adxl367_probe(struct device *dev, const struct adxl367_ops *ops,
> >  	if (ret)
> >  		return ret;
> >  
> > +	ret = adxl367_set_int_map_reg(st, irq);
> > +	if (ret < 0)
> > +		return dev_err_probe(st->dev, ret, "Failed to get interrupt\n");
> > +	irq = ret;
> > +
> >  	ret = devm_request_threaded_irq(st->dev, irq, NULL,
> >  					adxl367_irq_handler, IRQF_ONESHOT,
> >  					indio_dev->name, indio_dev);  
>