Re: [PATCH v3 3/4] iio: mpl3115: add support for DRDY interrupt

[Jonathan Cameron <jic23@kernel.org>]

> > > +static int mpl3115_trigger_probe(struct mpl3115_data *data,
> > > +				 struct iio_dev *indio_dev)
> > > +{
> > > +	struct fwnode_handle *fwnode = dev_fwnode(&data->client->dev);
> > > +	int ret, irq, irq_type, irq_cfg_flags = 0;
> > > +
> > > +	irq = fwnode_irq_get_byname(fwnode, "INT1");
> > > +	if (irq < 0) {
> > > +		irq = fwnode_irq_get_byname(fwnode, "INT2");
> > > +		if (irq < 0)
> > > +			return 0;
> > > +
> > > +		irq_cfg_flags |= MPL3115_INT2;
> > > +	}
> > > +
> > > +	irq_type = irq_get_trigger_type(irq);
> > > +	if (irq_type != IRQF_TRIGGER_RISING && irq_type != IRQF_TRIGGER_FALLING)
> > > +		return -EINVAL;
> > > +
> > > +	irq_cfg_flags |= irq_type;  
> > Commented on this before, but mixing flags that are local to this driver
> > with those that are global provides not guarantees against future changes
> > of the global ones to overlap with your local values.
> > 
> > So just track these as two separate values rather than combining them.
> >  
> 
> So you mean 2 separate variables, one for INT1/INT2 and one for the
> trigger RISING/FALLING, am I right?

Yes.

> This was the approach in v1, but the code for writing the regs CTRL3 and
> CTRL5 should be improved, I was thinking something like:
> 
> if (irq_pin == MPL3115_IRQ_INT1) {
>     write_byte_data(REG5, INT_CFG_DRDY);
>     if (irq_type == IRQF_TRIGGER_RISING)
>         write_byte_data(REG3, IPOL1);
> } else if (irq_type == IRQF_TRIGGER_RISING) {
>     write_byte_data(REG3, IPOL2);
> }
> 
> This is perhaps a bit less readable than the switch(int_cfg_flags) with 4
> cases... but IMO it's still quite ok and it's less verbose since we do not
> duplicate the write_byte_data(REG5, INT_CFG_DRDY).

That looks ok to me.

...

				 indio_dev->name,
> > > +						 iio_device_id(indio_dev));
> > > +	if (!data->drdy_trig)
> > > +		return -ENOMEM;
> > > +
> > > +	data->drdy_trig->ops = &mpl3115_trigger_ops;
> > > +	iio_trigger_set_drvdata(data->drdy_trig, indio_dev);
> > > +	ret = devm_iio_trigger_register(&data->client->dev, data->drdy_trig);  
> > 
> > Whilst unlikely the race matters. It is this call that creates the infrastructure
> > that might allow the interrupt generation to be triggered via userspace controls.
> > So the handler should probably be in place firsts.  I.e. do the devm_request_threaded_irq
> > before this.
> >  
> Will fix in v4

Side process related note: If you agree with something, just crop it out!  That means
we get to focus in quickly on the bits where there is more discussion to be done.

Your change log in v4 is where I'll see you made these changes.

When there is nothing to continue the discussion around in a thread, don't reply at all.
Thanks etc can come alongside the change log.

Thanks,

Jonathan

p.s. I have periodic sessions of mailing people about the process stuff once the
overall list traffic is larger than it should be for stuff like this. You just happened
to be an 'unlucky' recipient today!