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

[Tsz Shan Chan <tsz.chan.dev@gmail.com>]

On Wed, Aug 12, 2026 at 04:50:02AM +0100, Jonathan Cameron wrote:
> On Tue, 11 Aug 2026 12:49:37 +0300
> Andy Shevchenko <andriy.shevchenko@intel.com> 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?

In the vcnl4010/vcnl4020 datasheet:
- Page 5 (Application circuit Notes): The interrupt pin is an open drain
  output.

In the vcnl4040/vcnl4200 datasheet:
- Page 7 (Fig. 11 - Hardware Pin Connection Diagram) shows INT pin
  connected to V_Pull_up with an 8.2k resistor
- Page 13 (Interruption Section): the level of INT pin (pin 6/8) is pulled
  low once an interrupt event has been triggered.

This confirms that the interrupt line is an open drain active low
output, so both IRQF_TRIGGER_LOW and IRQF_TRIGGER_FALLING (on a
dedicated unshared INT line) are both valid parent trigger type.
The sensor doesn't know about the parent trigger type and simply
pulls the INT line down when an event happens.

> > 
> > > 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 :)

The (ret <= 0) checks for two cases:
1. ret < 0: I2C read error. Cannot confirm this device caused the
   interrupt, so returning IRQ_NONE is safer for a shared line
2. ret = 0: No interrupt flag was set. This means no interrupt pending
   so this device didn't cause the interrupt, so return IRQ_NONE.

Combining both into (ret <= 0) was confusing. I will split these checks,
use bitmake and update the commit message in v2.


> > >  	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.

I agree that edge triggers should not be shared. My goal was to support
level triggers for shared interrupt line without silently changing the
trigger type for other setups that do not share irq line.

In my understanding, IRQF_SHARED does not force sharing, only enables
the capabilities. IRQF_TRIGGER_FALLING was kept for existing setups
using dedicated irq line. IRQF_TRIGGER_LOW was added so shared lines can
work reliably without missing interrupts.

Or would it be better to completely drop the trigger type check? Simply
pass IRQF_SHARED | IRQF_ONESHOT and let the kernel use whatever trigger
type the firmware configures.


> > 
> > > +			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);  
> > 

Will fix them in v2.