Hi Uwe
On 20/09/2022 08:01, Uwe Kleine-König wrote:
By swapping the definition of skl_int3472_discrete_remove() and
skl_int3472_discrete_probe() the forward declaration of the former can
be dropped. This is a good thing as it removes code duplication.
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@xxxxxxxxxxxxxx>
Ah thanks - not sure why I ever did it that way now that you point it out :)
Reviewed-by: Daniel Scally <djrscally@xxxxxxxxx>
---
Hello,
I didn't check in detail, but in my experience calling the remove
function in the error path of the probe function is prone to cleanup
errors. I didn't spot anything after a quick glance, but let me point
out this is unstable. E.g. in an error path of
skl_int3472_register_clock() the function is left with
int3472->clock.clk pointing to an unregistered clk and int3472->clock.cl
== NULL. Someone modifying the return function must be well aware of
that.
Best regards
Uwe
I take your point - I have to revisit this driver shortly anyway, so
I'll try to remember to revise that away. Thanks
drivers/platform/x86/intel/int3472/discrete.c | 34 +++++++++----------
1 file changed, 16 insertions(+), 18 deletions(-)
diff --git a/drivers/platform/x86/intel/int3472/discrete.c b/drivers/platform/x86/intel/int3472/discrete.c
index ed4c9d760757..974a132db651 100644
--- a/drivers/platform/x86/intel/int3472/discrete.c
+++ b/drivers/platform/x86/intel/int3472/discrete.c
@@ -331,7 +331,22 @@ static int skl_int3472_parse_crs(struct int3472_discrete_device *int3472)
return 0;
}
-static int skl_int3472_discrete_remove(struct platform_device *pdev);
+static int skl_int3472_discrete_remove(struct platform_device *pdev)
+{
+ struct int3472_discrete_device *int3472 = platform_get_drvdata(pdev);
+
+ gpiod_remove_lookup_table(&int3472->gpios);
+
+ if (int3472->clock.cl)
+ skl_int3472_unregister_clock(int3472);
+
+ gpiod_put(int3472->clock.ena_gpio);
+ gpiod_put(int3472->clock.led_gpio);
+
+ skl_int3472_unregister_regulator(int3472);
+
+ return 0;
+}
static int skl_int3472_discrete_probe(struct platform_device *pdev)
{
@@ -383,23 +398,6 @@ static int skl_int3472_discrete_probe(struct platform_device *pdev)
return 0;
}
-static int skl_int3472_discrete_remove(struct platform_device *pdev)
-{
- struct int3472_discrete_device *int3472 = platform_get_drvdata(pdev);
-
- gpiod_remove_lookup_table(&int3472->gpios);
-
- if (int3472->clock.cl)
- skl_int3472_unregister_clock(int3472);
-
- gpiod_put(int3472->clock.ena_gpio);
- gpiod_put(int3472->clock.led_gpio);
-
- skl_int3472_unregister_regulator(int3472);
-
- return 0;
-}
-
static const struct acpi_device_id int3472_device_id[] = {
{ "INT3472", 0 },
{ }
base-commit: 568035b01cfb107af8d2e4bd2fb9aea22cf5b868
