Hi Paul,
Am 14.02.2022 um 11:24 schrieb Paul Cercueil paul@crapouillou.net:
Hi,
Le sam., févr. 12 2022 at 16:50:50 +0100, H. Nikolaus Schaller hns@goldelico.com a écrit :
+static void ingenic_dw_hdmi_cleanup(void *data) +{
- struct dw_hdmi *hdmi = (struct dw_hdmi *)data;
- dw_hdmi_remove(hdmi);
+}
+static int ingenic_dw_hdmi_probe(struct platform_device *pdev) +{
- struct dw_hdmi *hdmi;
- hdmi = dw_hdmi_probe(pdev, &ingenic_dw_hdmi_plat_data);
- if (IS_ERR(hdmi))
return PTR_ERR(hdmi);- return devm_add_action_or_reset(&pdev->dev, ingenic_dw_hdmi_cleanup, hdmi);
I think I said it already, but in this driver you could use a .remove callback, there's not much point in using devm cleanups in such a simple setup.
Well it was your suggestion after v8:
https://lore.kernel.org/all/DIA33R.QE29K7RKLI2C1@crapouillou.net/
So we now almost go back to RFC v1 almost 2 years ago:
https://patchwork.kernel.org/project/linux-mips/patch/2c131e1fb19e19f958a612...
Of course there was a good reason to better handle the regulator AND the dw_hdmi_remove() by a single mechanism.
Now the regulator has gone and been replaced by the hdmi connector and we can go back.
In your probe you could just: return PTR_ERR_OR_ZERO(hdmi);
No, this does not work since we need to platform_set_drvdata(). to be able to access the private struct in the remove callback. And checking errors after platform_set_drvdata() can be done but looks strange to me.
It is up to you what you prefer.
+}
+static struct platform_driver ingenic_dw_hdmi_driver = {
- .probe = ingenic_dw_hdmi_probe,
- .driver = {
.name = "dw-hdmi-ingenic",.of_match_table = ingenic_dw_hdmi_dt_ids,- },
+}; +module_platform_driver(ingenic_dw_hdmi_driver);
+MODULE_DESCRIPTION("JZ4780 Specific DW-HDMI Driver Extension"); +MODULE_LICENSE("GPL v2"); +MODULE_ALIAS("platform:dwhdmi-ingenic");
Should probably be "platform:dw-hdmi-ingenic"?
Yes, indeed. Thanks for spotting!
Was also good in v1. Probably someone deleted the hyphen unnoticed during editing of "jz4780" to "ingenic"...
BR and thanks, Nikolaus