From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mgamail.intel.com (mgamail.intel.com [134.134.136.24]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 2B50A46521 for ; Wed, 27 Dec 2023 17:35:06 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=intel.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=intel.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=intel.com header.i=@intel.com header.b="ChGeEBvx" DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1703698507; x=1735234507; h=date:from:to:cc:subject:message-id:references: mime-version:in-reply-to; bh=xgw6Rx1xyiu9/c4fqxAyWEIp8q2MXXE0eNzx2fpq55Q=; b=ChGeEBvxgrXzqyiRzZLFCTmhek0sRxg4jmILlzEWaJAxBBj9gXpeI8yi apgHwfXuFSS38Ds89j1HldS3BMAlCr8J5IRT6bP+4c6XJVvCRT1skNSFi Pt2EFvA+Ps/k7Ajs5bvd8t8zWEVj4ukRUZCvR6M/y/RDBtqPiWfJ30xBm BMbmFeLBisYWgrYhulG06/kdQs9gSl14UBsY+xEHJftRV695mW/y8ki3m C8VP7vbvFAIiNV7Gqklz1/y9ias5wJZREa6jXGAFW8lbFR6mspW5ZQYIu dhh3QXxSOsfEwQ+vJ7fh0F2AhO7UceRrwdDYcIUeSJZz+0IVLfPRbPjL2 A==; X-IronPort-AV: E=McAfee;i="6600,9927,10936"; a="399256025" X-IronPort-AV: E=Sophos;i="6.04,309,1695711600"; d="scan'208";a="399256025" Received: from fmsmga001.fm.intel.com ([10.253.24.23]) by orsmga102.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 27 Dec 2023 09:35:06 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=McAfee;i="6600,9927,10936"; a="921884517" X-IronPort-AV: E=Sophos;i="6.04,309,1695711600"; d="scan'208";a="921884517" Received: from smile.fi.intel.com ([10.237.72.54]) by fmsmga001.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 27 Dec 2023 09:35:02 -0800 Received: from andy by smile.fi.intel.com with local (Exim 4.97) (envelope-from ) id 1rIXo3-00000009Tbk-0hcN; Wed, 27 Dec 2023 19:34:59 +0200 Date: Wed, 27 Dec 2023 19:34:58 +0200 From: Andy Shevchenko To: Mark Hasemeyer Cc: LKML , AngeloGioacchino Del Regno , Krzysztof Kozlowski , Rob Herring , Konrad Dybcio , Sudeep Holla , Raul Rangel , Tzung-Bi Shih , Benson Leung , Bhanu Prakash Maiya , Chen-Yu Tsai , Guenter Roeck , Lee Jones , Prashant Malani , Rob Barnes , Stephen Boyd , chrome-platform@lists.linux.dev Subject: Re: [PATCH v3 24/24] platform/chrome: cros_ec: Use PM subsystem to manage wakeirq Message-ID: References: <20231226192149.1830592-1-markhas@chromium.org> <20231226122113.v3.24.Ieee574a0e94fbaae01fd6883ffe2ceeb98d7df28@changeid> Precedence: bulk X-Mailing-List: chrome-platform@lists.linux.dev List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20231226122113.v3.24.Ieee574a0e94fbaae01fd6883ffe2ceeb98d7df28@changeid> Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo On Tue, Dec 26, 2023 at 12:21:28PM -0700, Mark Hasemeyer wrote: > The cros ec driver is manually managing the wake IRQ by calling > enable_irq_wake()/disable_irq_wake() during suspend/resume. > > Modify the driver to use the power management subsystem to manage the > wakeirq. > > Rather than assuming that the IRQ is wake capable, use the underlying > firmware/device tree to determine whether or not to enable it as a wake > source. Some Chromebooks rely solely on the ec_sync pin to wake the AP > but do not specify the interrupt as wake capable in the ACPI _CRS. For > LPC/ACPI based systems a DMI quirk is introduced listing boards whose > firmware should not be trusted to provide correct wake capable values. > For device tree base systems, it is not an issue as the relevant device > tree entries have been updated and DTS is built from source for each > ChromeOS update. ... > acpi_status status; > struct cros_ec_device *ec_dev; > + struct resource irqres; struct resource irqres = {}; ? > u8 buf[2] = {}; > int irq, ret; ... > - irq = platform_get_irq_optional(pdev, 0); > - if (irq > 0) > + irq = platform_get_irq_resource_optional(pdev, 0, &irqres); > + if (irq > 0) { > ec_dev->irq = irq; > - else if (irq != -ENXIO) { > + if (should_force_irq_wake_capable()) > + ec_dev->irq_wake = true; > + else > + ec_dev->irq_wake = irqres.flags & IORESOURCE_IRQ_WAKECAPABLE; > + } else if (irq != -ENXIO) { > dev_err(dev, "couldn't retrieve IRQ number (%d)\n", irq); > return irq; > } Still I do not like ambiguity behind irq > 0 vs. irqres.start. For this, and if needed others, return plain error. Seems I gave the tag for the previous patch, consider that tag conditional (it seems I missed this). ... > u16 proto_version; > void *priv; > int irq; > + bool irq_wake; > u8 *din; > u8 *dout; > int din_size; > int dout_size; > - bool wake_enabled; > bool suspended; > int (*cmd_xfer)(struct cros_ec_device *ec, > struct cros_ec_command *msg); Have you run pahole on this (before and after)? -- With Best Regards, Andy Shevchenko