Re: [PATCH 2/2] iio: light: vcnl4000: add shared IRQ support

[Jonathan Cameron]

On Tue, 11 Aug 2026 12:49:37 +0300
Andy Shevchenko <andriy.shevchenko@xxxxxxxxx> wrote:

> On Tue, Aug 11, 2026 at 05:07:25PM +1000, Tsz Shan Chan wrote:
> > Use the IRQ trigger type set by firmware instead, and fall back to
> > IRQF_TRIGGER_FALLING if no trigger type is specified to maintain current
> > behaviour.
> > 
> > Support IRQF_TRIGGER_FALLING and IRQF_TRIGGER_LOW, which match the open
> > drain active low interrupt output. Reject unsupported trigger types.  
> 
> Can you elaborate with the reference to datasheet if the HW support this
> type of IRQ? In such a case, how does HW know which type to trigger?
> 
> > Request the interrupt with IRQF_SHARED, and return IRQ_NONE in the irq
> > handler when there is no interrupt pending.  
> 
> ...
A couple of follow ups to add a few more things to what Any called out.

> 
> >  	ret = i2c_smbus_read_word_data(data->client, data->chip_spec->int_reg);
> > -	if (ret < 0)
> > -		return IRQ_HANDLED;
> > +	if (ret <= 0)  
> 
> I haven't seen mention of this change in the commit message. Is it related
> somehow to the trigger type? How?

I'd definitely prefer to see the error case separately handled from the
no known interrupts.  That no interrupt check should probably also
only be the ones we have support for, so something like:

	if (ret < 0)
		return IRQ_NONE;

	if (!(ret & (VCNL4040_PS_IF_CLOSE | VCNL4040_PS_IF_AWAY |
		     VCNL4040_ALS_FALLING | VCNL4040_ALS_RISING)))
		return IRQ_NONE;

or something along those lines.
	

> 
> > +		return IRQ_NONE;  
> 
> ...
> 
> >  	ret = i2c_smbus_read_byte_data(data->client, VCNL4010_ISR);
> > -	if (ret < 0)
> > -		goto end;
> > +	if (ret <= 0)
> > +		return IRQ_NONE;  
> 
> Ditto.

snap :)

> 
> >  	isr = ret;  
> 
> ...
> 
> >  	if (client->irq && data->chip_spec->irq_thread) {
> > +		u32 irq_type = irq_get_trigger_type(client->irq);
> > +
> > +		switch (irq_type) {
> > +		case IRQF_TRIGGER_FALLING:  
> 
> Hmm... Do you have a case with edge sharing interrupts IRL? I think it's
> a brain damage setup if it exists.

Would indeed be unusual to put it lightly!

> 
> > +		case IRQF_TRIGGER_LOW:
> > +			break;
> > +		case IRQF_TRIGGER_NONE:
> > +			irq_type = IRQF_TRIGGER_FALLING;  
> 
> Ditto.
> 
> > +			break;
> > +		default:
> > +			return dev_err_probe(dev, -EINVAL,
> > +					"unsupported irq trigger type %x\n",
> > +					irq_type);  
> 
> Broken indentation.
> 
> > +		}
> >  		ret = devm_request_threaded_irq(dev, client->irq, NULL,
> >  						data->chip_spec->irq_thread,
> > -						IRQF_TRIGGER_FALLING |
> > -						IRQF_ONESHOT,  
> 
> > +						IRQF_ONESHOT | IRQF_SHARED |  
> 
> Also assign these above in a separate line, so this will be just irq_flags (and
> name it irq_flags as IRQF_ stands for).
> 
> > +						irq_type,
> >  						"vcnl4000_irq",
> >  						indio_dev);  
>