Received-SPF: softfail (google.com: domain of transitioning linux-kernel-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
Content-Type: text/plain;
        charset=utf-8
Mime-Version: 1.0 (Mac OS X Mail 12.4 \(3445.104.21\))
Subject: Re: [PATCH v15 2/7] drm/ingenic: Add dw-hdmi driver specialization
 for jz4780
From:   "H. Nikolaus Schaller" <hns@goldelico.com>
In-Reply-To: <58IA7R.PZ9FQXN7FVAK@crapouillou.net>
Date:   Mon, 14 Feb 2022 12:02:53 +0100
Cc:     Andrzej Hajda <andrzej.hajda@intel.com>,
        Neil Armstrong <narmstrong@baylibre.com>,
        Robert Foss <robert.foss@linaro.org>,
        Paul Boddie <paul@boddie.org.uk>,
        Laurent Pinchart <Laurent.pinchart@ideasonboard.com>,
        Jernej Skrabec <jernej.skrabec@gmail.com>,
        David Airlie <airlied@linux.ie>,
        Daniel Vetter <daniel@ffwll.ch>,
        Maxime Ripard <maxime@cerno.tech>,
        Harry Wentland <harry.wentland@amd.com>,
        Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>,
        Jonas Karlman <jonas@kwiboo.se>,
        linux-kernel <linux-kernel@vger.kernel.org>,
        dri-devel <dri-devel@lists.freedesktop.org>,
        linux-mips <linux-mips@vger.kernel.org>,
        Discussions about the Letux Kernel 
        <letux-kernel@openphoenux.org>,
        Ezequiel Garcia <ezequiel@collabora.com>
Content-Transfer-Encoding: quoted-printable
Message-Id: <AD978522-8321-476D-955C-3CDD747EFCAC@goldelico.com>
References: <cover.1644681054.git.hns@goldelico.com>
 <e6e1f3f44e6979a998ec9c372e329b6facaded15.1644681054.git.hns@goldelico.com>
 <58IA7R.PZ9FQXN7FVAK@crapouillou.net>
To:     Paul Cercueil <paul@crapouillou.net>
Precedence: bulk

Hi Paul,

> Am 14.02.2022 um 11:24 schrieb Paul Cercueil <paul@crapouillou.net>:
>=20
> Hi,
>=20
> Le sam., f=C3=A9vr. 12 2022 at 16:50:50 +0100, H. Nikolaus Schaller =
<hns@goldelico.com> a =C3=A9crit :

>> +static void ingenic_dw_hdmi_cleanup(void *data)
>> +{
>> +	struct dw_hdmi *hdmi =3D (struct dw_hdmi *)data;
>> +
>> +	dw_hdmi_remove(hdmi);
>> +}
>> +
>> +static int ingenic_dw_hdmi_probe(struct platform_device *pdev)
>> +{
>> +	struct dw_hdmi *hdmi;
>> +
>> +	hdmi =3D 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);
>=20
> 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/2c131e1fb19e19f958a6=
12f7186bc83f4afb0b0a.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.

>=20
> 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.

>=20
>> +}
>> +
>> +static struct platform_driver ingenic_dw_hdmi_driver =3D {
>> +	.probe  =3D ingenic_dw_hdmi_probe,
>> +	.driver =3D {
>> +		.name =3D "dw-hdmi-ingenic",
>> +		.of_match_table =3D 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");
>=20
> 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