diff mbox series

NFC: trf7970a: disable all regulators on removal

Message ID AM0PR09MB2675EDFD44EC82B6BCBB430F95082@AM0PR09MB2675.eurprd09.prod.outlook.com (mailing list archive)
State Not Applicable
Delegated to: Johannes Berg
Headers show
Series NFC: trf7970a: disable all regulators on removal | expand

Commit Message

Paul Geurts April 16, 2024, 8:28 p.m. UTC
During module probe, regulator 'vin' and 'vdd-io' are used and enabled,
but the vdd-io regulator overwrites the 'vin' regulator pointer. During
remove, only the vdd-io is disabled, as the vin regulator pointer is not
available anymore. When regulator_put() is called during resource
cleanup a kernel warning is given, as the regulator is still enabled.

Store the two regulators in separate pointers and disable both the
regulators on module remove.

Fixes: 49d22c70aaf0 ("NFC: trf7970a: Add device tree option of 1.8 Volt IO voltage")

Signed-off-by: Paul Geurts <paul_geurts@live.nl>
---
 drivers/nfc/trf7970a.c | 42 +++++++++++++++++++++++-------------------
 1 file changed, 23 insertions(+), 19 deletions(-)

Comments

Krzysztof Kozlowski April 17, 2024, 1:15 p.m. UTC | #1
On 16/04/2024 22:28, Paul Geurts wrote:
> During module probe, regulator 'vin' and 'vdd-io' are used and enabled,
> but the vdd-io regulator overwrites the 'vin' regulator pointer. During
> remove, only the vdd-io is disabled, as the vin regulator pointer is not
> available anymore. When regulator_put() is called during resource
> cleanup a kernel warning is given, as the regulator is still enabled.
> 
> Store the two regulators in separate pointers and disable both the
> regulators on module remove.
> 
> Fixes: 49d22c70aaf0 ("NFC: trf7970a: Add device tree option of 1.8 Volt IO voltage")
> 
> Signed-off-by: Paul Geurts <paul_geurts@live.nl>

No blank lines between tags. Please look at existing commits (git log).

> ---
>  drivers/nfc/trf7970a.c | 42 +++++++++++++++++++++++-------------------
>  1 file changed, 23 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/nfc/trf7970a.c b/drivers/nfc/trf7970a.c
> index 7eb17f46a815..9e1a34e23af2 100644
> --- a/drivers/nfc/trf7970a.c
> +++ b/drivers/nfc/trf7970a.c
> @@ -424,7 +424,8 @@ struct trf7970a {
>  	enum trf7970a_state		state;
>  	struct device			*dev;
>  	struct spi_device		*spi;
> -	struct regulator		*regulator;
> +	struct regulator		*vin_regulator;
> +	struct regulator		*vddio_regulator;
>  	struct nfc_digital_dev		*ddev;
>  	u32				quirks;
>  	bool				is_initiator;
> @@ -1883,7 +1884,7 @@ static int trf7970a_power_up(struct trf7970a *trf)
>  	if (trf->state != TRF7970A_ST_PWR_OFF)
>  		return 0;
>  
> -	ret = regulator_enable(trf->regulator);
> +	ret = regulator_enable(trf->vin_regulator);

That does not look like equivalent code. Previously this was vddio, right?


Best regards,
Krzysztof
Paul Geurts April 18, 2024, 8:12 a.m. UTC | #2
On 17-04-2024 15:15, Krzysztof Kozlowski wrote:
> On 16/04/2024 22:28, Paul Geurts wrote:
>> During module probe, regulator 'vin' and 'vdd-io' are used and enabled,
>> but the vdd-io regulator overwrites the 'vin' regulator pointer. During
>> remove, only the vdd-io is disabled, as the vin regulator pointer is not
>> available anymore. When regulator_put() is called during resource
>> cleanup a kernel warning is given, as the regulator is still enabled.
>>
>> Store the two regulators in separate pointers and disable both the
>> regulators on module remove.
>>
>> Fixes: 49d22c70aaf0 ("NFC: trf7970a: Add device tree option of 1.8 Volt IO voltage")
>>
>> Signed-off-by: Paul Geurts <paul_geurts@live.nl>
> No blank lines between tags. Please look at existing commits (git log).
Will fix this, thanks
>
>> ---
>>  drivers/nfc/trf7970a.c | 42 +++++++++++++++++++++++-------------------
>>  1 file changed, 23 insertions(+), 19 deletions(-)
>>
>> diff --git a/drivers/nfc/trf7970a.c b/drivers/nfc/trf7970a.c
>> index 7eb17f46a815..9e1a34e23af2 100644
>> --- a/drivers/nfc/trf7970a.c
>> +++ b/drivers/nfc/trf7970a.c
>> @@ -424,7 +424,8 @@ struct trf7970a {
>>  	enum trf7970a_state		state;
>>  	struct device			*dev;
>>  	struct spi_device		*spi;
>> -	struct regulator		*regulator;
>> +	struct regulator		*vin_regulator;
>> +	struct regulator		*vddio_regulator;
>>  	struct nfc_digital_dev		*ddev;
>>  	u32				quirks;
>>  	bool				is_initiator;
>> @@ -1883,7 +1884,7 @@ static int trf7970a_power_up(struct trf7970a *trf)
>>  	if (trf->state != TRF7970A_ST_PWR_OFF)
>>  		return 0;
>>  
>> -	ret = regulator_enable(trf->regulator);
>> +	ret = regulator_enable(trf->vin_regulator);
> That does not look like equivalent code. Previously this was vddio, right?
This is part of the original issue created by 49d22c70aaf0. This should be the VIN regulator, but the pointer override made it VDD-IO.
>
>
> Best regards,
> Krzysztof
>
Krzysztof Kozlowski April 18, 2024, 5:07 p.m. UTC | #3
On 18/04/2024 10:12, Paul Geurts wrote:
> On 17-04-2024 15:15, Krzysztof Kozlowski wrote:
>> On 16/04/2024 22:28, Paul Geurts wrote:
>>> During module probe, regulator 'vin' and 'vdd-io' are used and enabled,
>>> but the vdd-io regulator overwrites the 'vin' regulator pointer. During
>>> remove, only the vdd-io is disabled, as the vin regulator pointer is not
>>> available anymore. When regulator_put() is called during resource
>>> cleanup a kernel warning is given, as the regulator is still enabled.
>>>
>>> Store the two regulators in separate pointers and disable both the
>>> regulators on module remove.
>>>
>>> Fixes: 49d22c70aaf0 ("NFC: trf7970a: Add device tree option of 1.8 Volt IO voltage")
>>>
>>> Signed-off-by: Paul Geurts <paul_geurts@live.nl>
>> No blank lines between tags. Please look at existing commits (git log).
> Will fix this, thanks
>>
>>> ---
>>>  drivers/nfc/trf7970a.c | 42 +++++++++++++++++++++++-------------------
>>>  1 file changed, 23 insertions(+), 19 deletions(-)
>>>
>>> diff --git a/drivers/nfc/trf7970a.c b/drivers/nfc/trf7970a.c
>>> index 7eb17f46a815..9e1a34e23af2 100644
>>> --- a/drivers/nfc/trf7970a.c
>>> +++ b/drivers/nfc/trf7970a.c
>>> @@ -424,7 +424,8 @@ struct trf7970a {
>>>  	enum trf7970a_state		state;
>>>  	struct device			*dev;
>>>  	struct spi_device		*spi;
>>> -	struct regulator		*regulator;
>>> +	struct regulator		*vin_regulator;
>>> +	struct regulator		*vddio_regulator;
>>>  	struct nfc_digital_dev		*ddev;
>>>  	u32				quirks;
>>>  	bool				is_initiator;
>>> @@ -1883,7 +1884,7 @@ static int trf7970a_power_up(struct trf7970a *trf)
>>>  	if (trf->state != TRF7970A_ST_PWR_OFF)
>>>  		return 0;
>>>  
>>> -	ret = regulator_enable(trf->regulator);
>>> +	ret = regulator_enable(trf->vin_regulator);
>> That does not look like equivalent code. Previously this was vddio, right?
> This is part of the original issue created by 49d22c70aaf0. This should be the VIN regulator, but the pointer override made it VDD-IO.

True, good point.


Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org>

Best regards,
Krzysztof
diff mbox series

Patch

diff --git a/drivers/nfc/trf7970a.c b/drivers/nfc/trf7970a.c
index 7eb17f46a815..9e1a34e23af2 100644
--- a/drivers/nfc/trf7970a.c
+++ b/drivers/nfc/trf7970a.c
@@ -424,7 +424,8 @@  struct trf7970a {
 	enum trf7970a_state		state;
 	struct device			*dev;
 	struct spi_device		*spi;
-	struct regulator		*regulator;
+	struct regulator		*vin_regulator;
+	struct regulator		*vddio_regulator;
 	struct nfc_digital_dev		*ddev;
 	u32				quirks;
 	bool				is_initiator;
@@ -1883,7 +1884,7 @@  static int trf7970a_power_up(struct trf7970a *trf)
 	if (trf->state != TRF7970A_ST_PWR_OFF)
 		return 0;
 
-	ret = regulator_enable(trf->regulator);
+	ret = regulator_enable(trf->vin_regulator);
 	if (ret) {
 		dev_err(trf->dev, "%s - Can't enable VIN: %d\n", __func__, ret);
 		return ret;
@@ -1926,7 +1927,7 @@  static int trf7970a_power_down(struct trf7970a *trf)
 	if (trf->en2_gpiod && !(trf->quirks & TRF7970A_QUIRK_EN2_MUST_STAY_LOW))
 		gpiod_set_value_cansleep(trf->en2_gpiod, 0);
 
-	ret = regulator_disable(trf->regulator);
+	ret = regulator_disable(trf->vin_regulator);
 	if (ret)
 		dev_err(trf->dev, "%s - Can't disable VIN: %d\n", __func__,
 			ret);
@@ -2065,37 +2066,37 @@  static int trf7970a_probe(struct spi_device *spi)
 	mutex_init(&trf->lock);
 	INIT_DELAYED_WORK(&trf->timeout_work, trf7970a_timeout_work_handler);
 
-	trf->regulator = devm_regulator_get(&spi->dev, "vin");
-	if (IS_ERR(trf->regulator)) {
-		ret = PTR_ERR(trf->regulator);
+	trf->vin_regulator = devm_regulator_get(&spi->dev, "vin");
+	if (IS_ERR(trf->vin_regulator)) {
+		ret = PTR_ERR(trf->vin_regulator);
 		dev_err(trf->dev, "Can't get VIN regulator: %d\n", ret);
 		goto err_destroy_lock;
 	}
 
-	ret = regulator_enable(trf->regulator);
+	ret = regulator_enable(trf->vin_regulator);
 	if (ret) {
 		dev_err(trf->dev, "Can't enable VIN: %d\n", ret);
 		goto err_destroy_lock;
 	}
 
-	uvolts = regulator_get_voltage(trf->regulator);
+	uvolts = regulator_get_voltage(trf->vin_regulator);
 	if (uvolts > 4000000)
 		trf->chip_status_ctrl = TRF7970A_CHIP_STATUS_VRS5_3;
 
-	trf->regulator = devm_regulator_get(&spi->dev, "vdd-io");
-	if (IS_ERR(trf->regulator)) {
-		ret = PTR_ERR(trf->regulator);
+	trf->vddio_regulator = devm_regulator_get(&spi->dev, "vdd-io");
+	if (IS_ERR(trf->vddio_regulator)) {
+		ret = PTR_ERR(trf->vddio_regulator);
 		dev_err(trf->dev, "Can't get VDD_IO regulator: %d\n", ret);
-		goto err_destroy_lock;
+		goto err_disable_vin_regulator;
 	}
 
-	ret = regulator_enable(trf->regulator);
+	ret = regulator_enable(trf->vddio_regulator);
 	if (ret) {
 		dev_err(trf->dev, "Can't enable VDD_IO: %d\n", ret);
-		goto err_destroy_lock;
+		goto err_disable_vin_regulator;
 	}
 
-	if (regulator_get_voltage(trf->regulator) == 1800000) {
+	if (regulator_get_voltage(trf->vddio_regulator) == 1800000) {
 		trf->io_ctrl = TRF7970A_REG_IO_CTRL_IO_LOW;
 		dev_dbg(trf->dev, "trf7970a config vdd_io to 1.8V\n");
 	}
@@ -2108,7 +2109,7 @@  static int trf7970a_probe(struct spi_device *spi)
 	if (!trf->ddev) {
 		dev_err(trf->dev, "Can't allocate NFC digital device\n");
 		ret = -ENOMEM;
-		goto err_disable_regulator;
+		goto err_disable_vddio_regulator;
 	}
 
 	nfc_digital_set_parent_dev(trf->ddev, trf->dev);
@@ -2137,8 +2138,10 @@  static int trf7970a_probe(struct spi_device *spi)
 	trf7970a_shutdown(trf);
 err_free_ddev:
 	nfc_digital_free_device(trf->ddev);
-err_disable_regulator:
-	regulator_disable(trf->regulator);
+err_disable_vddio_regulator:
+	regulator_disable(trf->vddio_regulator);
+err_disable_vin_regulator:
+	regulator_disable(trf->vin_regulator);
 err_destroy_lock:
 	mutex_destroy(&trf->lock);
 	return ret;
@@ -2157,7 +2160,8 @@  static void trf7970a_remove(struct spi_device *spi)
 	nfc_digital_unregister_device(trf->ddev);
 	nfc_digital_free_device(trf->ddev);
 
-	regulator_disable(trf->regulator);
+	regulator_disable(trf->vddio_regulator);
+	regulator_disable(trf->vin_regulator);
 
 	mutex_destroy(&trf->lock);
 }