* [PATCH v2 0/2] NFC: nxp-nci: Add pad supply voltage pvdd-supply
@ 2023-06-09 15:41 Raymond Hackley
2023-06-09 15:42 ` [PATCH v2 1/2] dt-bindings: nfc: nxp-nci: document pvdd-supply Raymond Hackley
2023-06-09 15:42 ` [PATCH v2 2/2] NFC: nxp-nci: Add pad supply voltage pvdd-supply Raymond Hackley
0 siblings, 2 replies; 9+ messages in thread
From: Raymond Hackley @ 2023-06-09 15:41 UTC (permalink / raw)
To: linux-kernel
Cc: Krzysztof Kozlowski, David S . Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, Rob Herring, Liam Girdwood,
Mark Brown, Michael Walle, Uwe Kleine-König, Jeremy Kerr,
Raymond Hackley, netdev, devicetree
v2: Use dev_err_probe() and drop unnecessary nxp_nci_i2c_poweroff()
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.
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v2 1/2] dt-bindings: nfc: nxp-nci: document pvdd-supply
2023-06-09 15:41 [PATCH v2 0/2] NFC: nxp-nci: Add pad supply voltage pvdd-supply Raymond Hackley
@ 2023-06-09 15:42 ` Raymond Hackley
2023-06-09 15:42 ` [PATCH v2 2/2] NFC: nxp-nci: Add pad supply voltage pvdd-supply Raymond Hackley
1 sibling, 0 replies; 9+ messages in thread
From: Raymond Hackley @ 2023-06-09 15:42 UTC (permalink / raw)
To: linux-kernel
Cc: Krzysztof Kozlowski, David S . Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, Rob Herring, Liam Girdwood,
Mark Brown, Michael Walle, Uwe Kleine-König, Jeremy Kerr,
Raymond Hackley, netdev, devicetree
PN547/553, QN310/330 chips on some devices require a pad supply voltage
(PVDD). Otherwise, the NFC won't power up.
Document 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>
Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
---
Documentation/devicetree/bindings/net/nfc/nxp,nci.yaml | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/Documentation/devicetree/bindings/net/nfc/nxp,nci.yaml b/Documentation/devicetree/bindings/net/nfc/nxp,nci.yaml
index 6924aff0b2c5..d5a4f935c2ce 100644
--- a/Documentation/devicetree/bindings/net/nfc/nxp,nci.yaml
+++ b/Documentation/devicetree/bindings/net/nfc/nxp,nci.yaml
@@ -25,6 +25,11 @@ properties:
firmware-gpios:
description: Output GPIO pin used to enter firmware download mode
+ pvdd-supply:
+ description: |
+ Optional regulator that provides pad supply voltage for some
+ controllers
+
interrupts:
maxItems: 1
--
2.30.2
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH v2 2/2] NFC: nxp-nci: Add pad supply voltage pvdd-supply
2023-06-09 15:41 [PATCH v2 0/2] NFC: nxp-nci: Add pad supply voltage pvdd-supply Raymond Hackley
2023-06-09 15:42 ` [PATCH v2 1/2] dt-bindings: nfc: nxp-nci: document pvdd-supply Raymond Hackley
@ 2023-06-09 15:42 ` Raymond Hackley
2023-06-09 15:46 ` Krzysztof Kozlowski
1 sibling, 1 reply; 9+ messages in thread
From: Raymond Hackley @ 2023-06-09 15:42 UTC (permalink / raw)
To: linux-kernel
Cc: Krzysztof Kozlowski, David S . Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, Rob Herring, Liam Girdwood,
Mark Brown, Michael Walle, Uwe Kleine-König, Jeremy Kerr,
Raymond Hackley, netdev, devicetree
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)) {
+ r = regulator_disable(pvdd);
+ if (r < 0)
+ dev_warn(dev,
+ "Failed to disable regulator pvdd: %d\n",
+ r);
+ }
+}
+
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);
+ 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);
+ 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);
}
static const struct i2c_device_id nxp_nci_i2c_id_table[] = {
--
2.30.2
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v2 2/2] NFC: nxp-nci: Add pad supply voltage pvdd-supply
2023-06-09 15:42 ` [PATCH v2 2/2] NFC: nxp-nci: Add pad supply voltage pvdd-supply Raymond Hackley
@ 2023-06-09 15:46 ` Krzysztof Kozlowski
2023-06-09 17:40 ` Raymond Hackley
0 siblings, 1 reply; 9+ messages in thread
From: Krzysztof Kozlowski @ 2023-06-09 15:46 UTC (permalink / raw)
To: Raymond Hackley, linux-kernel
Cc: David S . Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Rob Herring, Liam Girdwood, Mark Brown, Michael Walle,
Uwe Kleine-König, Jeremy Kerr, netdev, devicetree
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 2/2] NFC: nxp-nci: Add pad supply voltage pvdd-supply
2023-06-09 15:46 ` Krzysztof Kozlowski
@ 2023-06-09 17:40 ` Raymond Hackley
2023-06-09 19:29 ` Krzysztof Kozlowski
0 siblings, 1 reply; 9+ messages in thread
From: Raymond Hackley @ 2023-06-09 17:40 UTC (permalink / raw)
To: krzysztof.kozlowski
Cc: broonie, davem, devicetree, edumazet, jk, kuba, lgirdwood,
linux-kernel, michael, netdev, pabeni, raymondhackley, robh+dt,
u.kleine-koenig
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 2/2] NFC: nxp-nci: Add pad supply voltage pvdd-supply
2023-06-09 17:40 ` Raymond Hackley
@ 2023-06-09 19:29 ` Krzysztof Kozlowski
2023-06-09 19:34 ` Mark Brown
2023-06-09 20:44 ` Raymond Hackley
0 siblings, 2 replies; 9+ messages in thread
From: Krzysztof Kozlowski @ 2023-06-09 19:29 UTC (permalink / raw)
To: Raymond Hackley
Cc: broonie, davem, devicetree, edumazet, jk, kuba, lgirdwood,
linux-kernel, michael, netdev, pabeni, robh+dt, u.kleine-koenig
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 2/2] NFC: nxp-nci: Add pad supply voltage pvdd-supply
2023-06-09 19:29 ` Krzysztof Kozlowski
@ 2023-06-09 19:34 ` Mark Brown
2023-06-09 20:52 ` Raymond Hackley
2023-06-09 20:44 ` Raymond Hackley
1 sibling, 1 reply; 9+ messages in thread
From: Mark Brown @ 2023-06-09 19:34 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: Raymond Hackley, davem, devicetree, edumazet, jk, kuba, lgirdwood,
linux-kernel, michael, netdev, pabeni, robh+dt, u.kleine-koenig
[-- Attachment #1: Type: text/plain, Size: 911 bytes --]
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.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 2/2] NFC: nxp-nci: Add pad supply voltage pvdd-supply
2023-06-09 19:29 ` Krzysztof Kozlowski
2023-06-09 19:34 ` Mark Brown
@ 2023-06-09 20:44 ` Raymond Hackley
1 sibling, 0 replies; 9+ messages in thread
From: Raymond Hackley @ 2023-06-09 20:44 UTC (permalink / raw)
To: krzysztof.kozlowski
Cc: broonie, davem, devicetree, edumazet, jk, kuba, lgirdwood,
linux-kernel, michael, netdev, pabeni, raymondhackley, robh+dt,
u.kleine-koenig
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 2/2] NFC: nxp-nci: Add pad supply voltage pvdd-supply
2023-06-09 19:34 ` Mark Brown
@ 2023-06-09 20:52 ` Raymond Hackley
0 siblings, 0 replies; 9+ messages in thread
From: Raymond Hackley @ 2023-06-09 20:52 UTC (permalink / raw)
To: broonie
Cc: davem, devicetree, edumazet, jk, krzysztof.kozlowski, kuba,
lgirdwood, linux-kernel, michael, netdev, pabeni, raymondhackley,
robh+dt, u.kleine-koenig
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
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2023-06-09 20:52 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-06-09 15:41 [PATCH v2 0/2] NFC: nxp-nci: Add pad supply voltage pvdd-supply Raymond Hackley
2023-06-09 15:42 ` [PATCH v2 1/2] dt-bindings: nfc: nxp-nci: document pvdd-supply Raymond Hackley
2023-06-09 15:42 ` [PATCH v2 2/2] NFC: nxp-nci: Add pad supply voltage pvdd-supply Raymond Hackley
2023-06-09 15:46 ` Krzysztof Kozlowski
2023-06-09 17:40 ` Raymond Hackley
2023-06-09 19:29 ` Krzysztof Kozlowski
2023-06-09 19:34 ` Mark Brown
2023-06-09 20:52 ` Raymond Hackley
2023-06-09 20:44 ` Raymond Hackley
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).