[v2,2/2] NFC: nxp-nci: Add pad supply voltage pvdd-supply

  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@protonmail.com>
---
 drivers/nfc/nxp-nci/i2c.c | 42 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 42 insertions(+)
  

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@protonmail.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
  

Hi Krzysztof,

On Friday, June 9th, 2023 at 3:46 PM, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:


> 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@protonmail.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.
>

First condition !IS_ERR(pvdd) is to check if pvdd exists.
Some devices, msm8916-samsung-serranove for example, doesn't need pvdd or
have it bound in the device tree:

https://github.com/torvalds/linux/commit/ab0f0987e035f908d670fed7d86efa6fac66c0bb

Without !IS_ERR(pvdd), checking it with regulator_is_enabled(pvdd):

[   50.161882] 8<--- cut here ---
[   50.161906] Unable to handle kernel paging request at virtual address fffffff9 when read
[   50.161916] [fffffff9] *pgd=affff841, *pte=00000000, *ppte=00000000
[   50.161938] Internal error: Oops: 27 [#1] PREEMPT SMP ARM

Or disabling it directly with regulator_disable(pvdd):

[   69.439827] 8<--- cut here ---
[   69.439841] Unable to handle kernel NULL pointer dereference at virtual address 00000045 when read
[   69.439852] [00000045] *pgd=00000000
[   69.439864] Internal error: Oops: 5 [#1] PREEMPT SMP ARM

Second condition regulator_is_enabled(pvdd) is to make sure that pvdd is
disabled with balance.

Similar checks can be found here:

https://elixir.bootlin.com/linux/v6.4-rc5/source/drivers/staging/greybus/arche-apb-ctrl.c#L208

> > + 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.
>

Yes. This will be fixed in v3.

> > + 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...
>

I don't have a good reason to keep it and will drop it in v3.

Regards,
Raymond

>
>
> Best regards,
> Krzysztof
  

On 09/06/2023 19:40, Raymond Hackley wrote:
> Hi Krzysztof,
> 
> On Friday, June 9th, 2023 at 3:46 PM, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:
> 
> 
>> 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@protonmail.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.
>>
> 
> First condition !IS_ERR(pvdd) is to check if pvdd exists.
> Some devices, msm8916-samsung-serranove for example, doesn't need pvdd or
> have it bound in the device tree:

If regulator is missing you should get a dummy.

But anyway the code will not be executed if you don't get proper regulator.

> 
> https://github.com/torvalds/linux/commit/ab0f0987e035f908d670fed7d86efa6fac66c0bb
> 
> Without !IS_ERR(pvdd), checking it with regulator_is_enabled(pvdd):
> 
> [   50.161882] 8<--- cut here ---
> [   50.161906] Unable to handle kernel paging request at virtual address fffffff9 when read
> [   50.161916] [fffffff9] *pgd=affff841, *pte=00000000, *ppte=00000000
> [   50.161938] Internal error: Oops: 27 [#1] PREEMPT SMP ARM
> 
> Or disabling it directly with regulator_disable(pvdd):
> 
> [   69.439827] 8<--- cut here ---
> [   69.439841] Unable to handle kernel NULL pointer dereference at virtual address 00000045 when read
> [   69.439852] [00000045] *pgd=00000000
> [   69.439864] Internal error: Oops: 5 [#1] PREEMPT SMP ARM
> 
> Second condition regulator_is_enabled(pvdd) is to make sure that pvdd is
> disabled with balance.
> 

So you have buggy code and to hide the bug you add checks? No, make the
code correct so the check is not needed.


> Similar checks can be found here:
> 
> https://elixir.bootlin.com/linux/v6.4-rc5/source/drivers/staging/greybus/arche-apb-ctrl.c#L208

staging driver is not an example...

> 


Best regards,
Krzysztof
  

On Fri, Jun 09, 2023 at 09:29:51PM +0200, Krzysztof Kozlowski wrote:
> On 09/06/2023 19:40, Raymond Hackley wrote:
> > On Friday, June 9th, 2023 at 3:46 PM, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:

> > Second condition regulator_is_enabled(pvdd) is to make sure that pvdd is
> > disabled with balance.

> So you have buggy code and to hide the bug you add checks? No, make the
> code correct so the check is not needed.

Specifically your driver should only ever call regulator_disable() to
balance out regulator_enable() calls it made itself and it should know
how many of those it has done.  regulator_is_enabled() should only ever
be used during probe if for some reason it is important to figure out if
the device is already powered for startup, this should be very unusual.
If something else enabled the regualtor then whatever did that needs to
undo those enables, not another driver.
  

Hi Krzysztof,

On Friday, June 9th, 2023 at 7:29 PM, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:

> > > 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.
> > 
> > First condition !IS_ERR(pvdd) is to check if pvdd exists.
> > Some devices, msm8916-samsung-serranove for example, doesn't need pvdd or
> > have it bound in the device tree:
> 
> 
> If regulator is missing you should get a dummy.
> 
> But anyway the code will not be executed if you don't get proper regulator.
> 

The current patch set is using devm_regulator_get_optional() instead of
devm_regulator_get(), which doesn't get a dummy regulator.

> > https://github.com/torvalds/linux/commit/ab0f0987e035f908d670fed7d86efa6fac66c0bb
> > 
> > Without !IS_ERR(pvdd), checking it with regulator_is_enabled(pvdd):
> > 
> > [ 50.161882] 8<--- cut here ---
> > [ 50.161906] Unable to handle kernel paging request at virtual address fffffff9 when read
> > [ 50.161916] [fffffff9] *pgd=affff841, *pte=00000000, *ppte=00000000
> > [ 50.161938] Internal error: Oops: 27 [#1] PREEMPT SMP ARM
> > 
> > Or disabling it directly with regulator_disable(pvdd):
> > 
> > [ 69.439827] 8<--- cut here ---
> > [ 69.439841] Unable to handle kernel NULL pointer dereference at virtual address 00000045 when read
> > [ 69.439852] [00000045] *pgd=00000000
> > [ 69.439864] Internal error: Oops: 5 [#1] PREEMPT SMP ARM
> > 
> > Second condition regulator_is_enabled(pvdd) is to make sure that pvdd is
> > disabled with balance.
> 
> 
> So you have buggy code and to hide the bug you add checks? No, make the
> code correct so the check is not needed.
> 

Do you mean that I should use devm_regulator_get() instead in order to get
a dummy regulator, so that it can disable pvdd without unnecessary checks?
Actually there is v4 with those buggy codes and checks dropped.
Please do let me know if I am understanding and doing it correctly. I would
send it after proper period of cooldown.

Regards,
Raymond
  

Hi Mark,

On Friday, June 9th, 2023 at 7:34 PM, Mark Brown <broonie@kernel.org> wrote:
 
> Specifically your driver should only ever call regulator_disable() to
> balance out regulator_enable() calls it made itself and it should know
> how many of those it has done. regulator_is_enabled() should only ever
> be used during probe if for some reason it is important to figure out if
> the device is already powered for startup, this should be very unusual.
> If something else enabled the regualtor then whatever did that needs to
> undo those enables, not another driver.

Thnak you for explanation. I should drop regulator_is_enabled() here since
it's misused.

Regards,
Raymond