Re: [PATCH v15 2/7] drm/ingenic: Add dw-hdmi driver specialization for jz4780

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





Le lun., févr. 14 2022 at 12:02:53 +0100, H. Nikolaus Schaller <hns@xxxxxxxxxxxxx> a écrit :
Hi Paul,

 Am 14.02.2022 um 11:24 schrieb Paul Cercueil <paul@xxxxxxxxxxxxxxx>:

 Hi,

Le sam., févr. 12 2022 at 16:50:50 +0100, H. Nikolaus Schaller <hns@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx/

It made sense for your v8, not so much for your v15...

So we now almost go back to RFC v1 almost 2 years ago:

https://patchwork.kernel.org/project/linux-mips/patch/2c131e1fb19e19f958a612f7186bc83f4afb0b0a.1582744379.git.hns@xxxxxxxxxxxxx/

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.

Yeah, I guess it would look strange. Fine then.

Then I guess just merge your current [6/7] with this one (and make sure it comes after your current [5/7]) and it looks mergeable to me.

Cheers,
-Paul

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"...








[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux