Message ID | 20221108142226.63161-7-andriy.shevchenko@linux.intel.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a5d:6687:0:0:0:0:0 with SMTP id l7csp2741074wru; Tue, 8 Nov 2022 06:28:28 -0800 (PST) X-Google-Smtp-Source: AA0mqf7jknN6QtXx+KYrIh7331PSzae3pDVEVXdRCPQ/J9FTRnD7np1gMI13pr0InkhHi1ZP33s4 X-Received: by 2002:a05:6a00:a86:b0:56f:3529:e310 with SMTP id b6-20020a056a000a8600b0056f3529e310mr7140248pfl.79.1667917708484; Tue, 08 Nov 2022 06:28:28 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1667917708; cv=none; d=google.com; s=arc-20160816; b=PDX11n6s0kG5/SOEqja5Pv7/+cTR7u8Bbb0EWBRlo98hSAxm+GhsPcevUhSvFAujcN aFr8DhBDnRRXhitqIdABOcQg7/hmqZ0/v2+YB9aT1SYeQ1nK4LzmCxxwfdgpOOBX80cw Av9pZn16/NJXg5DVWuSa6PRfnojqktVLx2Kzs5LIFcGm7TvwLre9Yrn8LXYkTzFNNNVz ZeMy6rBA6RwJI+4tQS8Zl3H9a0cW5sc225p0je2uZe8ikcfTR1lw5os7YRePFO25TIRU 5rT5WJIINr+odVEUDNIMqttJC2I8BVas1AaoQJZ26CTuj2T/z+jsUkF9zPnqQf+Ou0BM 3Bsg== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :references:in-reply-to:message-id:date:subject:cc:to:from :dkim-signature; bh=x27RTc5/n84/ShKHSf1BizBMPXkcUYH5HqIJniat2gg=; b=xB85YudWacgl8X4CEsGT0fVk86SOIpoiLVgtM8/7Hdr1DaaNLWq/1079v6YaHRrV5B YenZXTuITJE0sBc4QwPaZDwh5JtM5GYFmsGbzuRrUd1Uh29ENLfqWji562hptVtz6uDs ztHYGxyfWc/iIKUMPHqp0ZJa1BJ0YCvpEZlQ3IpElqendSklqoCxpe6lNT6ABUiZFQGA LlmX62Ti6fjt8zRP8DFyzJuhmzVkxRD2WhIvWyQPyjh3Z5lBLfHzsAtNbE/KZIHPo0Rk 5mKD+F/d0sNgI7MERIL09u1OSA26Almsm+rFYykhQArVJygSQ4b0y8py4Rbuu4IwlFDU vYUw== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@intel.com header.s=Intel header.b=G1+TTT56; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=intel.com Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id d18-20020a170902ced200b0017d2bb81273si16215193plg.600.2022.11.08.06.28.12; Tue, 08 Nov 2022 06:28:28 -0800 (PST) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; dkim=pass header.i=@intel.com header.s=Intel header.b=G1+TTT56; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=intel.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S234333AbiKHOWt (ORCPT <rfc822;tony84727@gmail.com> + 99 others); Tue, 8 Nov 2022 09:22:49 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:58142 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S234674AbiKHOWK (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Tue, 8 Nov 2022 09:22:10 -0500 Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 323C254B1D; Tue, 8 Nov 2022 06:22:10 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1667917330; x=1699453330; h=from:to:cc:subject:date:message-id:in-reply-to: references:mime-version:content-transfer-encoding; bh=vyYkmlrH3FTwJFyY/NU3XVwDT0lsgxH2Rn0Y02epYKs=; b=G1+TTT565RKy1lMiBsTNmEHn5iMOuM8unZyEpMe+MWemIzobWOPfbfC8 Sf+QGiIRCuaIvGJmctei9VJl8rRomJCnx25omWEVtZ+HWykVCsqtb0dm7 eMSH658/teUTWEwbdD2q8QMKHFAH7p4LsjevAGxPN7qI/0ZNPWxnl0dI0 OC8+Pe6UxF6Wd26EcUDhOb2ZE7D5I4/jC+97KpvSzKjtZ0kdvfiAp/hye 03gtvqqoeQPMVhL1/VbyHojmli/bBXKf+N1PDa9w03hgiztobDtSUrgBO 2BaqU96GIKZyr4nzdzMwGdB5PFUL0LEomCDapyxOtqcr+yYrvtODfRGjF A==; X-IronPort-AV: E=McAfee;i="6500,9779,10524"; a="298219291" X-IronPort-AV: E=Sophos;i="5.96,148,1665471600"; d="scan'208";a="298219291" Received: from orsmga004.jf.intel.com ([10.7.209.38]) by orsmga101.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 08 Nov 2022 06:22:10 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=McAfee;i="6500,9779,10524"; a="761506504" X-IronPort-AV: E=Sophos;i="5.96,148,1665471600"; d="scan'208";a="761506504" Received: from black.fi.intel.com ([10.237.72.28]) by orsmga004.jf.intel.com with ESMTP; 08 Nov 2022 06:22:07 -0800 Received: by black.fi.intel.com (Postfix, from userid 1003) id 9945657A; Tue, 8 Nov 2022 16:22:27 +0200 (EET) From: Andy Shevchenko <andriy.shevchenko@linux.intel.com> To: Andy Shevchenko <andriy.shevchenko@linux.intel.com>, Mika Westerberg <mika.westerberg@linux.intel.com>, Hans de Goede <hdegoede@redhat.com>, =?utf-8?q?Uwe_Kleine-K=C3=B6nig?= <u.kleine-koenig@pengutronix.de>, Thierry Reding <thierry.reding@gmail.com>, linux-kernel@vger.kernel.org, linux-gpio@vger.kernel.org, linux-pwm@vger.kernel.org Cc: Andy Shevchenko <andy@kernel.org>, Linus Walleij <linus.walleij@linaro.org> Subject: [PATCH v2 6/6] pinctrl: intel: Enumerate PWM device when community has a capabilitty Date: Tue, 8 Nov 2022 16:22:26 +0200 Message-Id: <20221108142226.63161-7-andriy.shevchenko@linux.intel.com> X-Mailer: git-send-email 2.35.1 In-Reply-To: <20221108142226.63161-1-andriy.shevchenko@linux.intel.com> References: <20221108142226.63161-1-andriy.shevchenko@linux.intel.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-4.3 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_EF,RCVD_IN_DNSWL_MED, RCVD_IN_MSPIKE_H3,RCVD_IN_MSPIKE_WL,SPF_HELO_NONE,SPF_NONE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1748938479013512233?= X-GMAIL-MSGID: =?utf-8?q?1748938479013512233?= |
Series |
pinctrl: intel: Enable PWM optional feature
|
|
Commit Message
Andy Shevchenko
Nov. 8, 2022, 2:22 p.m. UTC
Some of the Communities may have PWM capability. In such cases, enumerate PWM device via respective driver. User is still responsible for setting correct pin muxing for the line that needs to output the signal. Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com> --- drivers/pinctrl/intel/pinctrl-intel.c | 29 +++++++++++++++++++++++++++ 1 file changed, 29 insertions(+)
Comments
On Tue, Nov 8, 2022 at 3:22 PM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > Some of the Communities may have PWM capability. In such cases, > enumerate PWM device via respective driver. User is still responsible > for setting correct pin muxing for the line that needs to output the > signal. > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com> So: > +#include <linux/platform_data/x86/pwm-lpss.h> (...) > +static int intel_pinctrl_probe_pwm(struct intel_pinctrl *pctrl, > + struct intel_community *community) > +{ > + static const struct pwm_lpss_boardinfo info = { > + .clk_rate = 19200000, > + .npwm = 1, > + .base_unit_bits = 22, > + .bypass = true, > + }; > + struct pwm_lpss_chip *pwm; > + > + if (!(community->features & PINCTRL_FEATURE_PWM)) > + return 0; > + > + pwm = pwm_lpss_probe(pctrl->dev, community->regs + PWMC, &info); > + if (IS_ERR(pwm) && PTR_ERR(pwm) != -ENODEV) > + return PTR_ERR(pwm); This is alike a boardfile embedded into the pin control driver. It's a bit backwards since we usually go the other direction, i.e. probe a PWM driver or whatever and then that driver request its resources that are assigned from e.g. DT or ACPI, and in this case that would mean it request its pin control handle and the pins get set up. I guess I can be convinced that this hack is the lesser evil :D What is it in the platform that makes this kind of hacks necessary? Inconsistent description in ACPI or is the PWM device simply missing from the DSDT (or whatever is the right form in ACPI) in already shipped devices that need it? Or is it simply impossible to describe the PWM device in ACPI? Yours, Linus Walleij
On Wed, Nov 09, 2022 at 10:08:51AM +0100, Linus Walleij wrote: > On Tue, Nov 8, 2022 at 3:22 PM Andy Shevchenko > <andriy.shevchenko@linux.intel.com> wrote: ... > > + static const struct pwm_lpss_boardinfo info = { > > + .clk_rate = 19200000, > > + .npwm = 1, > > + .base_unit_bits = 22, > > + .bypass = true, > > + }; > > + struct pwm_lpss_chip *pwm; > > + > > + if (!(community->features & PINCTRL_FEATURE_PWM)) > > + return 0; > > + > > + pwm = pwm_lpss_probe(pctrl->dev, community->regs + PWMC, &info); > > + if (IS_ERR(pwm) && PTR_ERR(pwm) != -ENODEV) > > + return PTR_ERR(pwm); > > This is alike a boardfile embedded into the pin control driver. Correct. > It's a bit backwards since we usually go the other direction, i.e. probe > a PWM driver or whatever and then that driver request its resources > that are assigned from e.g. DT or ACPI, and in this case that would > mean it request its pin control handle and the pins get set up. > > I guess I can be convinced that this hack is the lesser evil :D > > What is it in the platform that makes this kind of hacks necessary? The PWM capability is discoverable by the looking for it in the pin control IP MMIO, it's not a separate device, but a sibling (child?) of the pin control, that's not a separate entity. Moreover, not every pin control _community_ has that capability (capabilities are on the Community level and depends on ACPI representation of the communities themself - single device or device per community - the PWM may or may not be easily attached. What you are proposing is to invent at least two additional properties or so for the pin control device description and then to support old platforms, create a board file somewhere else, which will go through all pin control devices, checks the capabilities, then embeds the properties via properties (Either embedded into DSDT, if done in BIOS, or swnodes). Do I get you right? If so, in my opinion it's way more ugly and overkill that the current approach. > Inconsistent description in ACPI or is the PWM device simply > missing from the DSDT (or whatever is the right form in ACPI) > in already shipped devices that need it? Right. > Or is it simply impossible to describe the PWM device in ACPI? It's a dynamic feature and existing firmwares are carved in stone. It might be possible to move the above mentioned uglification to the BIOS. But the horizon of that is 5+ years in case I am able to convince program managers for it (TBH, I don't believe it's feasible, since "Windows works!" mantra, they are not engineers). That said, I agree that this looks not nice, but that's all what Mika and me can come up with to make all this as little ugly and intrusive as possible.
On Wed, Nov 9, 2022 at 10:56 AM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > On Wed, Nov 09, 2022 at 10:08:51AM +0100, Linus Walleij wrote: > > I guess I can be convinced that this hack is the lesser evil :D > > > > What is it in the platform that makes this kind of hacks necessary? > > The PWM capability is discoverable by the looking for it in the pin > control IP MMIO, it's not a separate device, but a sibling (child?) > of the pin control, that's not a separate entity. OK I get it. > Moreover, not every pin control _community_ has that capability (capabilities > are on the Community level and depends on ACPI representation of the > communities themself - single device or device per community - the PWM may or > may not be easily attached. OK I think I understand it a bit, if ACPI thinks about the PWM as "some feature of the community" then that is how it is, we have this bad fit between device tree and Linux internals at times as well, then spawning a device from another one is the way to go, we need to consider the option that it is Linux that is weird at times, not the HW description. > What you are proposing is to invent at least two additional properties or so > for the pin control device description and then to support old platforms, > create a board file somewhere else, which will go through all pin control > devices, checks the capabilities, then embeds the properties via properties > (Either embedded into DSDT, if done in BIOS, or swnodes). > > Do I get you right? > > If so, in my opinion it's way more ugly and overkill that the current > approach. No I just wanted to understand things better. This small hack in the pin controller is way better than a bigger and widespread hack somewhere else. > That said, I agree that this looks not nice, but that's all what > Mika and me can come up with to make all this as little ugly and > intrusive as possible. I can live with it, rough consensus and running code. Acked-by: Linus Walleij <linus.walleij@linaro.org> Yours, Linus Walleij
On Wed, Nov 09, 2022 at 11:08:04AM +0100, Linus Walleij wrote: > On Wed, Nov 9, 2022 at 10:56 AM Andy Shevchenko > <andriy.shevchenko@linux.intel.com> wrote: > > On Wed, Nov 09, 2022 at 10:08:51AM +0100, Linus Walleij wrote: ... > > > I guess I can be convinced that this hack is the lesser evil :D > > > > > > What is it in the platform that makes this kind of hacks necessary? > > > > The PWM capability is discoverable by the looking for it in the pin > > control IP MMIO, it's not a separate device, but a sibling (child?) > > of the pin control, that's not a separate entity. > > OK I get it. > > > Moreover, not every pin control _community_ has that capability (capabilities > > are on the Community level and depends on ACPI representation of the > > communities themself - single device or device per community - the PWM may or > > may not be easily attached. > > OK I think I understand it a bit, if ACPI thinks about the PWM > as "some feature of the community" then that is how it is, we have > this bad fit between device tree and Linux internals at times as well, > then spawning a device from another one is the way to go, we need > to consider the option that it is Linux that is weird at times, not the > HW description. The problem here is not the impossibility to do the things. The problem is that things are done and validated on a Windows system. After that it close to impossible to update the firmware or perform any architectural changes. OTOH, announcing the separate device out of the existing MMIO space doesn't sound right from the software point of view that should follow the hardware representation. Ideally, this should be an adaptive MFD-like device, but it makes things even more complicated than has been discussed already. (Note, that some of the pin control drivers are enumerated as platform devices, and that code should also be taken into account) ... > > That said, I agree that this looks not nice, but that's all what > > Mika and me can come up with to make all this as little ugly and > > intrusive as possible. > > I can live with it, rough consensus and running code. > Acked-by: Linus Walleij <linus.walleij@linaro.org> Thank you!
On Tue, Nov 08, 2022 at 04:22:26PM +0200, Andy Shevchenko wrote: > Some of the Communities may have PWM capability. In such cases, > enumerate PWM device via respective driver. User is still responsible > for setting correct pin muxing for the line that needs to output the > signal. > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com> > --- > drivers/pinctrl/intel/pinctrl-intel.c | 29 +++++++++++++++++++++++++++ > 1 file changed, 29 insertions(+) > > diff --git a/drivers/pinctrl/intel/pinctrl-intel.c b/drivers/pinctrl/intel/pinctrl-intel.c > index 6e630e87fed6..6b685ff7041f 100644 > --- a/drivers/pinctrl/intel/pinctrl-intel.c > +++ b/drivers/pinctrl/intel/pinctrl-intel.c > @@ -24,6 +24,8 @@ > #include <linux/pinctrl/pinctrl.h> > #include <linux/pinctrl/pinmux.h> > > +#include <linux/platform_data/x86/pwm-lpss.h> > + > #include "../core.h" > #include "pinctrl-intel.h" > > @@ -49,6 +51,8 @@ > #define PADOWN_MASK(p) (GENMASK(3, 0) << PADOWN_SHIFT(p)) > #define PADOWN_GPP(p) ((p) / 8) > > +#define PWMC 0x204 > + > /* Offset from pad_regs */ > #define PADCFG0 0x000 > #define PADCFG0_RXEVCFG_SHIFT 25 > @@ -1502,6 +1506,27 @@ static int intel_pinctrl_pm_init(struct intel_pinctrl *pctrl) > return 0; > } > > +static int intel_pinctrl_probe_pwm(struct intel_pinctrl *pctrl, > + struct intel_community *community) > +{ > + static const struct pwm_lpss_boardinfo info = { > + .clk_rate = 19200000, > + .npwm = 1, > + .base_unit_bits = 22, > + .bypass = true, > + }; > + struct pwm_lpss_chip *pwm; > + > + if (!(community->features & PINCTRL_FEATURE_PWM)) > + return 0; > + > + pwm = pwm_lpss_probe(pctrl->dev, community->regs + PWMC, &info); > + if (IS_ERR(pwm) && PTR_ERR(pwm) != -ENODEV) > + return PTR_ERR(pwm); Linus and Andy already agreed that this patch is ugly. I wonder if this here would be a bit less ugly if you do: if (IS_REACHABLE(...)) { pwm = pwm_lpss_probe(...); ... } and drop the check PTR_ERR(pwm) != -ENODEV (which might have a different semantic than "the pwm driver isn't available"). Best regards Uwe
On Thu, Nov 10, 2022 at 9:45 AM Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote: > On Tue, Nov 08, 2022 at 04:22:26PM +0200, Andy Shevchenko wrote: > > Some of the Communities may have PWM capability. In such cases, > > enumerate PWM device via respective driver. User is still responsible > > for setting correct pin muxing for the line that needs to output the > > signal. ... > > + pwm = pwm_lpss_probe(pctrl->dev, community->regs + PWMC, &info); > > + if (IS_ERR(pwm) && PTR_ERR(pwm) != -ENODEV) > > + return PTR_ERR(pwm); > > Linus and Andy already agreed that this patch is ugly. I wonder if this > here would be a bit less ugly if you do: > > if (IS_REACHABLE(...)) { > pwm = pwm_lpss_probe(...); > ... > > > } > > and drop the check PTR_ERR(pwm) != -ENODEV (which might have a different > semantic than "the pwm driver isn't available"). I will think about it (in such case the comment against the previous patch makes more sense to me). Thank you for the review!
diff --git a/drivers/pinctrl/intel/pinctrl-intel.c b/drivers/pinctrl/intel/pinctrl-intel.c index 6e630e87fed6..6b685ff7041f 100644 --- a/drivers/pinctrl/intel/pinctrl-intel.c +++ b/drivers/pinctrl/intel/pinctrl-intel.c @@ -24,6 +24,8 @@ #include <linux/pinctrl/pinctrl.h> #include <linux/pinctrl/pinmux.h> +#include <linux/platform_data/x86/pwm-lpss.h> + #include "../core.h" #include "pinctrl-intel.h" @@ -49,6 +51,8 @@ #define PADOWN_MASK(p) (GENMASK(3, 0) << PADOWN_SHIFT(p)) #define PADOWN_GPP(p) ((p) / 8) +#define PWMC 0x204 + /* Offset from pad_regs */ #define PADCFG0 0x000 #define PADCFG0_RXEVCFG_SHIFT 25 @@ -1502,6 +1506,27 @@ static int intel_pinctrl_pm_init(struct intel_pinctrl *pctrl) return 0; } +static int intel_pinctrl_probe_pwm(struct intel_pinctrl *pctrl, + struct intel_community *community) +{ + static const struct pwm_lpss_boardinfo info = { + .clk_rate = 19200000, + .npwm = 1, + .base_unit_bits = 22, + .bypass = true, + }; + struct pwm_lpss_chip *pwm; + + if (!(community->features & PINCTRL_FEATURE_PWM)) + return 0; + + pwm = pwm_lpss_probe(pctrl->dev, community->regs + PWMC, &info); + if (IS_ERR(pwm) && PTR_ERR(pwm) != -ENODEV) + return PTR_ERR(pwm); + + return 0; +} + static int intel_pinctrl_probe(struct platform_device *pdev, const struct intel_pinctrl_soc_data *soc_data) { @@ -1588,6 +1613,10 @@ static int intel_pinctrl_probe(struct platform_device *pdev, ret = intel_pinctrl_add_padgroups_by_size(pctrl, community); if (ret) return ret; + + ret = intel_pinctrl_probe_pwm(pctrl, community); + if (ret) + return ret; } irq = platform_get_irq(pdev, 0);