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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 14 Feb 2022 11:30:54 +0000
From:   Paul Cercueil <paul@...pouillou.net>
To:     "H. Nikolaus Schaller" <hns@...delico.com>
Cc:     Andrzej Hajda <andrzej.hajda@...el.com>,
        Neil Armstrong <narmstrong@...libre.com>,
        Robert Foss <robert.foss@...aro.org>,
        Paul Boddie <paul@...die.org.uk>,
        Laurent Pinchart <Laurent.pinchart@...asonboard.com>,
        Jernej Skrabec <jernej.skrabec@...il.com>,
        David Airlie <airlied@...ux.ie>,
        Daniel Vetter <daniel@...ll.ch>,
        Maxime Ripard <maxime@...no.tech>,
        Harry Wentland <harry.wentland@....com>,
        Kieran Bingham <kieran.bingham+renesas@...asonboard.com>,
        Jonas Karlman <jonas@...boo.se>,
        linux-kernel <linux-kernel@...r.kernel.org>,
        dri-devel <dri-devel@...ts.freedesktop.org>,
        linux-mips <linux-mips@...r.kernel.org>,
        Discussions about the Letux Kernel 
        <letux-kernel@...nphoenux.org>,
        Ezequiel Garcia <ezequiel@...labora.com>
Subject: Re: [PATCH v15 2/7] drm/ingenic: Add dw-hdmi driver specialization
 for jz4780



Le lun., févr. 14 2022 at 12:02:53 +0100, H. Nikolaus Schaller 
<hns@...delico.com> a écrit :
> Hi Paul,
> 
>>  Am 14.02.2022 um 11:24 schrieb Paul Cercueil <paul@...pouillou.net>:
>> 
>>  Hi,
>> 
>>  Le sam., févr. 12 2022 at 16:50:50 +0100, H. Nikolaus Schaller 
>> <hns@...delico.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/

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@goldelico.com/
> 
> 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"...