The parse config function now performs action on the device either
reading or writing and a reset. If the regulator is managed it needs
to be turned on. So turn on the regulator if available if the parsing
fails then turn off the regulator.
Fixes: a5235f3c7c23 ("can: tcan45x: Make wake-up GPIO an optional GPIO")
Signed-off-by: Dan Murphy <[email protected]>
---
v2 - Added error handling and moved regulator_get to probe
drivers/net/can/m_can/tcan4x5x.c | 17 ++++++++++-------
1 file changed, 10 insertions(+), 7 deletions(-)
diff --git a/drivers/net/can/m_can/tcan4x5x.c b/drivers/net/can/m_can/tcan4x5x.c
index 4e1789ea2bc3..ddf7db498241 100644
--- a/drivers/net/can/m_can/tcan4x5x.c
+++ b/drivers/net/can/m_can/tcan4x5x.c
@@ -374,11 +374,6 @@ static int tcan4x5x_parse_config(struct m_can_classdev *cdev)
if (IS_ERR(tcan4x5x->device_state_gpio))
tcan4x5x->device_state_gpio = NULL;
- tcan4x5x->power = devm_regulator_get_optional(cdev->dev,
- "vsup");
- if (PTR_ERR(tcan4x5x->power) == -EPROBE_DEFER)
- return -EPROBE_DEFER;
-
return 0;
}
@@ -412,6 +407,12 @@ static int tcan4x5x_can_probe(struct spi_device *spi)
if (!priv)
return -ENOMEM;
+ priv->power = devm_regulator_get_optional(&spi->dev, "vsup");
+ if (PTR_ERR(priv->power) == -EPROBE_DEFER)
+ return -EPROBE_DEFER;
+ else
+ priv->power = NULL;
+
mcan_class->device_data = priv;
m_can_class_get_clocks(mcan_class);
@@ -451,11 +452,13 @@ static int tcan4x5x_can_probe(struct spi_device *spi)
priv->regmap = devm_regmap_init(&spi->dev, &tcan4x5x_bus,
&spi->dev, &tcan4x5x_regmap);
- ret = tcan4x5x_parse_config(mcan_class);
+ ret = tcan4x5x_power_enable(priv->power, 1);
if (ret)
goto out_clk;
- tcan4x5x_power_enable(priv->power, 1);
+ ret = tcan4x5x_parse_config(mcan_class);
+ if (ret)
+ goto out_power;
ret = m_can_class_register(mcan_class);
if (ret)
--
2.23.0
On 12/10/19 5:32 PM, Dan Murphy wrote:
> The parse config function now performs action on the device either
> reading or writing and a reset. If the regulator is managed it needs
> to be turned on. So turn on the regulator if available if the parsing
> fails then turn off the regulator.
>
> Fixes: a5235f3c7c23 ("can: tcan45x: Make wake-up GPIO an optional GPIO")
> Signed-off-by: Dan Murphy <[email protected]>
Applied to linux-can.
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Embedded Linux | https://www.pengutronix.de |
Vertretung West/Dortmund | Phone: +49-231-2826-924 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
On 12/10/19 5:32 PM, Dan Murphy wrote:
> The parse config function now performs action on the device either
> reading or writing and a reset. If the regulator is managed it needs
> to be turned on. So turn on the regulator if available if the parsing
> fails then turn off the regulator.
>
> Fixes: a5235f3c7c23 ("can: tcan45x: Make wake-up GPIO an optional GPIO")
> Signed-off-by: Dan Murphy <[email protected]>
> ---
>
> v2 - Added error handling and moved regulator_get to probe
>
> drivers/net/can/m_can/tcan4x5x.c | 17 ++++++++++-------
> 1 file changed, 10 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/net/can/m_can/tcan4x5x.c b/drivers/net/can/m_can/tcan4x5x.c
> index 4e1789ea2bc3..ddf7db498241 100644
> --- a/drivers/net/can/m_can/tcan4x5x.c
> +++ b/drivers/net/can/m_can/tcan4x5x.c
> @@ -374,11 +374,6 @@ static int tcan4x5x_parse_config(struct m_can_classdev *cdev)
> if (IS_ERR(tcan4x5x->device_state_gpio))
> tcan4x5x->device_state_gpio = NULL;
>
> - tcan4x5x->power = devm_regulator_get_optional(cdev->dev,
> - "vsup");
> - if (PTR_ERR(tcan4x5x->power) == -EPROBE_DEFER)
> - return -EPROBE_DEFER;
> -
> return 0;
> }
>
> @@ -412,6 +407,12 @@ static int tcan4x5x_can_probe(struct spi_device *spi)
> if (!priv)
> return -ENOMEM;
>
> + priv->power = devm_regulator_get_optional(&spi->dev, "vsup");
> + if (PTR_ERR(priv->power) == -EPROBE_DEFER)
> + return -EPROBE_DEFER;
> + else
> + priv->power = NULL;
> +
BTW: you are leaking the netdev allocated with m_can_class_allocate_dev().
In order to fix this:
- introduce a m_can_class_free_dev() function that calls the
free_candev().
- fix error path in tcan4x5x_can_probe() and m_can_plat_probe() by
adding the missing m_can_class_free_dev()
- remove the free_candev() from the error path in m_can_class_register()
it makes no sense that this function free a ressource it has not
allocated
- remove the free_candev() from m_can_class_unregister()
it makes no sense that this function free a ressource it has not
allocated
- add needed m_can_class_free_dev() to tcan4x5x_can_remove() and
m_can_plat_remove()
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Embedded Linux | https://www.pengutronix.de |
Vertretung West/Dortmund | Phone: +49-231-2826-924 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
On 12/10/19 5:32 PM, Dan Murphy wrote:
> The parse config function now performs action on the device either
> reading or writing and a reset. If the regulator is managed it needs
> to be turned on. So turn on the regulator if available if the parsing
> fails then turn off the regulator.
Another BTW:
Consider converting the switching of the vsup to runtime_pm.
Yet another one:
Why do you disable the clocks in the error path of tcan4x5x_can_probe(),
but never enable them?
> out_clk:
> if (!IS_ERR(mcan_class->cclk)) {
> clk_disable_unprepare(mcan_class->cclk);
> clk_disable_unprepare(mcan_class->hclk);
> }
- please move the clock handling from the m_can.c to the individual
driver
- please move the clock handling to runtime_pm in the individual driver
- remove the obsolete m_can_class_get_clocks()
- make runtime_pm mandatory
regards,
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Embedded Linux | https://www.pengutronix.de |
Vertretung West/Dortmund | Phone: +49-231-2826-924 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
On 12/10/19 5:32 PM, Dan Murphy wrote:
> The parse config function now performs action on the device either
> reading or writing and a reset.
BTW n+1:
Why is the function called parse_config? I don't see any parsing going on.
Please add it directly to the tcan4x5x_can_probe() function.
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Embedded Linux | https://www.pengutronix.de |
Vertretung West/Dortmund | Phone: +49-231-2826-924 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
Marc
On 1/2/20 6:38 AM, Marc Kleine-Budde wrote:
> On 12/10/19 5:32 PM, Dan Murphy wrote:
>> The parse config function now performs action on the device either
>> reading or writing and a reset. If the regulator is managed it needs
>> to be turned on. So turn on the regulator if available if the parsing
>> fails then turn off the regulator.
> Another BTW:
> Consider converting the switching of the vsup to runtime_pm.
>
> Yet another one:
> Why do you disable the clocks in the error path of tcan4x5x_can_probe(),
> but never enable them?
>
>> out_clk:
>> if (!IS_ERR(mcan_class->cclk)) {
>> clk_disable_unprepare(mcan_class->cclk);
>> clk_disable_unprepare(mcan_class->hclk);
>> }
> - please move the clock handling from the m_can.c to the individual
> driver
> - please move the clock handling to runtime_pm in the individual driver
> - remove the obsolete m_can_class_get_clocks()
> - make runtime_pm mandatory
Ack to the above I have made these changes locally. Will submit next week.
Dan
> regards,
> Marc
>
Marc
On 1/2/20 6:38 AM, Marc Kleine-Budde wrote:
> On 12/10/19 5:32 PM, Dan Murphy wrote:
>> The parse config function now performs action on the device either
>> reading or writing and a reset. If the regulator is managed it needs
>> to be turned on. So turn on the regulator if available if the parsing
>> fails then turn off the regulator.
> Another BTW:
> Consider converting the switching of the vsup to runtime_pm.
>
> Yet another one:
> Why do you disable the clocks in the error path of tcan4x5x_can_probe(),
> but never enable them?
>
>> out_clk:
>> if (!IS_ERR(mcan_class->cclk)) {
>> clk_disable_unprepare(mcan_class->cclk);
>> clk_disable_unprepare(mcan_class->hclk);
>> }
> - please move the clock handling from the m_can.c to the individual
> driver
> - please move the clock handling to runtime_pm in the individual driver
> - remove the obsolete m_can_class_get_clocks()
> - make runtime_pm mandatory
>
> regards,
> Marc
>
I have separate the clock calls into pm runtime calls and moved the
clock init into the respective children of the framework.
Did you want me to submit 1 patch with all the changes or would you like
3 separate patches? First 2 patches will abstract the clocks away into
the children and the 3rd patch would be to remove the clocks API from
the framework
Dan