| Message ID | AM0PR09MB2675EDFD44EC82B6BCBB430F95082@AM0PR09MB2675.eurprd09.prod.outlook.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | NFC: trf7970a: disable all regulators on removal | expand |
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
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 >
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 --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); }
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(-)