netdev - Re: [PATCH v2 2/2] NFC: nxp-nci: Add pad supply voltage pvdd-supply

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <e2bb439c-9b72-991b-00f6-0b5e7602efd9@linaro.org>
Date: Fri, 9 Jun 2023 17:46:31 +0200
From: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
To: Raymond Hackley <raymondhackley@...tonmail.com>,
 linux-kernel@...r.kernel.org
Cc: "David S . Miller" <davem@...emloft.net>,
 Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>,
 Paolo Abeni <pabeni@...hat.com>, Rob Herring <robh+dt@...nel.org>,
 Liam Girdwood <lgirdwood@...il.com>, Mark Brown <broonie@...nel.org>,
 Michael Walle <michael@...le.cc>,
 Uwe Kleine-König <u.kleine-koenig@...gutronix.de>,
 Jeremy Kerr <jk@...econstruct.com.au>, netdev@...r.kernel.org,
 devicetree@...r.kernel.org
Subject: Re: [PATCH v2 2/2] NFC: nxp-nci: Add pad supply voltage pvdd-supply

On 09/06/2023 17:42, Raymond Hackley wrote:
> PN547/553, QN310/330 chips on some devices require a pad supply voltage
> (PVDD). Otherwise, the NFC won't power up.
> 
> Implement support for pad supply voltage pvdd-supply that is enabled by
> the nxp-nci driver so that the regulator gets enabled when needed.
> 
> Signed-off-by: Raymond Hackley <raymondhackley@...tonmail.com>
> ---
>  drivers/nfc/nxp-nci/i2c.c | 42 +++++++++++++++++++++++++++++++++++++++
>  1 file changed, 42 insertions(+)
> 
> diff --git a/drivers/nfc/nxp-nci/i2c.c b/drivers/nfc/nxp-nci/i2c.c
> index d4c299be7949..1b8877757cee 100644
> --- a/drivers/nfc/nxp-nci/i2c.c
> +++ b/drivers/nfc/nxp-nci/i2c.c
> @@ -35,6 +35,7 @@ struct nxp_nci_i2c_phy {
>  
>  	struct gpio_desc *gpiod_en;
>  	struct gpio_desc *gpiod_fw;
> +	struct regulator *pvdd;
>  
>  	int hard_fault; /*
>  			 * < 0 if hardware error occurred (e.g. i2c err)
> @@ -263,6 +264,22 @@ static const struct acpi_gpio_mapping acpi_nxp_nci_gpios[] = {
>  	{ }
>  };
>  
> +static void nxp_nci_i2c_poweroff(void *data)
> +{
> +	struct nxp_nci_i2c_phy *phy = data;
> +	struct device *dev = &phy->i2c_dev->dev;
> +	struct regulator *pvdd = phy->pvdd;
> +	int r;
> +
> +	if (!IS_ERR(pvdd) && regulator_is_enabled(pvdd)) {

Why do you need these checks? This should be called in correct context,
so when regulator is valid and enabled. If you have such checks it
suggests that code is buggy and this is being called in wrong contexts.

> +		r = regulator_disable(pvdd);
> +		if (r < 0)
> +			dev_warn(dev,
> +				 "Failed to disable regulator pvdd: %d\n",
> +				 r);

Weird wrapping. Why r is wrapped?

> +	}
> +}
> +
>  static int nxp_nci_i2c_probe(struct i2c_client *client)
>  {
>  	struct device *dev = &client->dev;
> @@ -298,6 +315,29 @@ static int nxp_nci_i2c_probe(struct i2c_client *client)
>  		return PTR_ERR(phy->gpiod_fw);
>  	}
>  
> +	phy->pvdd = devm_regulator_get_optional(dev, "pvdd");
> +	if (IS_ERR(phy->pvdd)) {
> +		r = PTR_ERR(phy->pvdd);
> +		if (r != -ENODEV)
> +			return dev_err_probe(dev, r,
> +					     "Failed to get regulator pvdd\n");
> +	} else {
> +		r = regulator_enable(phy->pvdd);
> +		if (r < 0) {
> +			nfc_err(dev,
> +				"Failed to enable regulator pvdd: %d\n",
> +				r);

Weird wrapping. Why r is wrapped?

> +			return r;
> +		}
> +	}
> +
> +	r = devm_add_action_or_reset(dev, nxp_nci_i2c_poweroff, phy);
> +	if (r < 0) {
> +		nfc_err(dev, "Failed to install poweroff handler: %d\n",
> +			r);

Weird wrapping. Why r is wrapped?

Just move it to the success path of enabling regulator.

> +		return r;
> +	}
> +
>  	r = nxp_nci_probe(phy, &client->dev, &i2c_phy_ops,
>  			  NXP_NCI_I2C_MAX_PAYLOAD, &phy->ndev);
>  	if (r < 0)
> @@ -319,6 +359,8 @@ static void nxp_nci_i2c_remove(struct i2c_client *client)
>  
>  	nxp_nci_remove(phy->ndev);
>  	free_irq(client->irq, phy);
> +
> +	nxp_nci_i2c_poweroff(phy);

Why? This code is buggy...

Best regards,
Krzysztof