From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-pg1-f179.google.com (mail-pg1-f179.google.com [209.85.215.179]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id ED0623148D8 for ; Thu, 13 Aug 2026 02:38:06 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.215.179 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1786588688; cv=none; b=Rd6LNYpZGBCuP7caSkuID9dB6RmDs8x4EI0lY0mau68N3zEuLbSej3JC85T6lqYzbF4+hfFo/BLpgvWcoV/oLGREjRm0VvMpAqpNacY7QeicamCze2MbavaWWqRh/0FtjQYdvikGpFDUpA0V1snfXsRYcqKVqiR2B8ZP/Sa0fto= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1786588688; c=relaxed/simple; bh=zs7pdiSpbSplYyqgCA5Lq8Pmo57kXuFbPXJQooB5OiY=; h=Date:From:To:Cc:Subject:Message-ID:MIME-Version:Content-Type: Content-Disposition:In-Reply-To; b=pGqdRVmHW5OfatwdB5Y5QROiJoBgyLdsvueqgRxlP2sigqMEm4x8VI52rPnu2JfImHJivtx6MG1BrXD1vcj0SwwLTJBYD0VRgjn3kZmVvISdmzjMkZQ01i1QbG8VzNOOWd+dW9ZuIB7zlXN9Q6qtrjHe8PgDA6MIlq4mO9FzOYM= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=gmail.com; spf=pass smtp.mailfrom=gmail.com; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b=T7YVzOzd; arc=none smtp.client-ip=209.85.215.179 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=gmail.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=gmail.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b="T7YVzOzd" Received: by mail-pg1-f179.google.com with SMTP id 41be03b00d2f7-c9d1fff21edso1269252a12.1 for ; Wed, 12 Aug 2026 19:38:06 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20251104; t=1786588686; x=1787193486; darn=vger.kernel.org; h=in-reply-to:content-disposition:content-type:mime-version :message-id:subject:cc:to:from:date:from:to:cc:subject:date :message-id:reply-to:content-type; bh=k6ZNlQuUr6EHPk3SgGGoGxYcXJS5G15cJiEQQRspIzs=; b=T7YVzOzd3vx675luWTO8VWk0jaGvXC+f28NSl+H75XzQMU6ytuhJhWq11XqwrlSgMU uL/IWKy01czjK1CZcQIjuJ4WP4N7mjZpS5ntCePfjf4HYAP3RcrLfG2b81aXiw7r/HnM IeJzg/cQ/Vzj0yWeyFZyI5lifiCXRlSqahojZOzPeyHxGoFckLWXjuiCuURaZGWbKiat qecT+o51evzPA8NVfz3ujz45uyiq6PRF/3KWu2Uol4DNVIdaeG4NLTEzEXiaiJ2MSSkV ADH3qrQ/sd597TOt6HQayAW9+kJbdlvS50Mf/1UULJFUQ3Azo8DMu+vzA1OwI4/jm/KK rayQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20251104; t=1786588686; x=1787193486; h=in-reply-to:content-disposition:content-type:mime-version :message-id:subject:cc:to:from:date:x-gm-gg:x-gm-message-state:from :to:cc:subject:date:message-id:reply-to:content-type; bh=k6ZNlQuUr6EHPk3SgGGoGxYcXJS5G15cJiEQQRspIzs=; b=aIlM4hOj0+/Di1sjQhM1I10LdefRBmTlP1VgM26HT1raV4EMj3cD0sAn30UyHmo+SZ 1f8VCmW6xdjMcY4G9En9VS8QvtsDOGRPOzZV6CrgsQQDcG6J8enXyNRq9mvoVTw8kHou zl2ZrEtcdeQ1f3hU8EtfGQ8I2OoAvaCdZgTjEFRaxuF7a4NjSZuPTmfafgkPuuOLk7IB Y70HBA8PhEZnwejcYiNHuHndiV1yydQWXGjvWDgGG7P7gtanHA0L6QEoD7hSHwL083bh dInRZorH7s+oD9SWmCgXGLvSf+yULUr+qNwICSLp4TK7VMoCVBGM/zV/ArB27TwzZMCY ERWw== X-Forwarded-Encrypted: i=1; AHgh+Rpe4HTtWOXOGUMijx19lU1AEXY/SnYkN1umwoWaAQE1O+DjA/EENSvKUGgmBRX4pxDBFGF9ZpO0LXfzO7k=@vger.kernel.org X-Gm-Message-State: AOJu0Yw9dZ5Qb2Xa4LduTKxI7/5NCeY7jrsXgrpM13tDBAIflkdL9Fvw H0x1Roi3b1DWRzYX7cHX8eMF7qUCwkc19gP83LmO4G5M+GRtUam5SM6/ X-Gm-Gg: AR+sD13SefLD05qRGLBQXj0U//Qga99OEkwjOcA5d47WRIpUXhhJ6lW/ZZlWCefQaQI xdyGAfqPbezfltA1Su6g5d9NHeWX5Y5vcHZkGFshhVAGjSawTXcmF9lCaZ2NgmeGuBzBo/10jEY so1DxBaglOUwhIdLwIlOK/YMT/0TGU1HmKd+Ioi+hHagWJlGrrru0iGxKYvtj7TkLOei6TUksU3 O9gOz8m9yZ23144KG7ESFzRjP33YduDy3IQMsjS11G3j+nkDHm1H9cdMTMnMFc/gyAXJ6LWor+T ToQnSrHH6al+W4ouFGnyi+onLwHdrd8D7aA/o6qy63VVLSvwhhKYaw4Bxb9Oat3vHhoUPjRTa1J Y5KfCcglMOpUlS7spqIQ2lEJ1dSgLV05shuT5uCQg/+s2jTBY2qHJUDNtQSfs+CeUYsl0dfopoD hRA9r0K/3FP3X+7HDFKlKuScI/KLX/3A4WMa7pvF8vVrLKPV8m9v07LOFGyBK6VQ== X-Received: by 2002:a05:6a20:728c:b0:3c1:fbf:1e2e with SMTP id adf61e73a8af0-3cc550e3a2fmr3383789637.10.1786588686042; Wed, 12 Aug 2026 19:38:06 -0700 (PDT) Received: from localhost ([115.70.157.242]) by smtp.gmail.com with ESMTPSA id 5a478bee46e88-31ebcb69fbbsm1971018eec.11.2026.08.12.19.38.04 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 12 Aug 2026 19:38:05 -0700 (PDT) Date: Thu, 13 Aug 2026 12:37:57 +1000 From: Tsz Shan Chan To: Andy Shevchenko , Jonathan Cameron Cc: David Lechner , Nuno =?utf-8?B?U8Oh?= , Andy Shevchenko , linux-iio@vger.kernel.org, linux-kernel@vger.kernel.org, Tsz Shan Chan Subject: Re: [PATCH 2/2] iio: light: vcnl4000: add shared IRQ support Message-ID: Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20260812044941.6f28cfb7@jic23-huawei> On Wed, Aug 12, 2026 at 04:50:02AM +0100, Jonathan Cameron wrote: > On Tue, 11 Aug 2026 12:49:37 +0300 > Andy Shevchenko 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.