Received-SPF: pass (google.com: domain of linux-kernel+bounces-191255-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Date: Mon, 27 May 2024 19:34:48 +0300
From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
To: Markus Schneider-Pargmann <msp@baylibre.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Jiri Slaby <jirislaby@kernel.org>, Rob Herring <robh@kernel.org>,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
	Conor Dooley <conor+dt@kernel.org>,
	Tony Lindgren <tony@atomide.com>,
	John Ogness <john.ogness@linutronix.de>,
	Uwe =?iso-8859-1?Q?Kleine-K=F6nig?= <u.kleine-koenig@pengutronix.de>,
	Ronald Wahl <ronald.wahl@raritan.com>,
	Thomas Richard <thomas.richard@bootlin.com>,
	Thomas Gleixner <tglx@linutronix.de>, Arnd Bergmann <arnd@arndb.de>,
	Vignesh Raghavendra <vigneshr@ti.com>,
	Vibhore Vardhan <vibhore@ti.com>,
	Kevin Hilman <khilman@baylibre.com>, Dhruva Gole <d-gole@ti.com>,
	linux-kernel@vger.kernel.org, linux-serial@vger.kernel.org,
	devicetree@vger.kernel.org
Subject: Re: [PATCH 4/5] serial: 8250: omap: Support wakeup pinctrl state
Message-ID: <ZlS2KBTvhizdmow-@smile.fi.intel.com>
References: <20240523075819.1285554-1-msp@baylibre.com>
 <20240523075819.1285554-5-msp@baylibre.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240523075819.1285554-5-msp@baylibre.com>
Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo

On Thu, May 23, 2024 at 09:58:18AM +0200, Markus Schneider-Pargmann wrote:
> UART can be used as a wakeup source for am62 from a powered-off SoC
> state. To enable wakeup from UART am62 requires a wakeup flag being set
> in the pinctrl.
> 
> If the device is marked as wakeup enabled, select the 'wakeup' pinctrl
> state on sys_off.

..

>  #include <linux/pm_qos.h>
>  #include <linux/pm_wakeirq.h>
>  #include <linux/dma-mapping.h>

> +#include <linux/reboot.h>

See below.

>  #include <linux/sys_soc.h>
>  #include <linux/pm_domain.h>

> +#include <linux/pinctrl/consumer.h>

Can we make some order here and put this before above pm_*.h or even earlier
according to the alphabet?

..

> +	priv->pinctrl = devm_pinctrl_get(&pdev->dev);
> +	if (!IS_ERR_OR_NULL(priv->pinctrl))

Shouldn't we report an error to the user? I assume that NULL is for optional
pin control, otherwise it's an error state which at bare minimum has to be
reported.

> +		priv->pinctrl_wakeup = pinctrl_lookup_state(priv->pinctrl, "wakeup");

> +	devm_register_sys_off_handler(&pdev->dev,

No error check?

> +				      SYS_OFF_MODE_POWER_OFF_PREPARE,
> +				      SYS_OFF_PRIO_DEFAULT,
> +				      omap8250_sysoff_handler, NULL);
> +

-- 
With Best Regards,
Andy Shevchenko