Re: [PATCH 7/9] drm/meson: dw-hdmi: use matched data

From: Jerome Brunet <jbrunet@baylibre.com>
To: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
Cc: Neil Armstrong <neil.armstrong@linaro.org>,
	 Maarten Lankhorst <maarten.lankhorst@linux.intel.com>,
	 Maxime Ripard <mripard@kernel.org>,
	Thomas Zimmermann <tzimmermann@suse.de>,
	 David Airlie <airlied@gmail.com>,
	 Daniel Vetter <daniel@ffwll.ch>,
	 Kevin Hilman <khilman@baylibre.com>,
	 dri-devel@lists.freedesktop.org,
	linux-amlogic@lists.infradead.org,  linux-kernel@vger.kernel.org
Subject: Re: [PATCH 7/9] drm/meson: dw-hdmi: use matched data
Date: Wed, 07 Aug 2024 11:12:52 +0200	[thread overview]
Message-ID: <1j5xsczoff.fsf@starbuckisacylon.baylibre.com> (raw)
In-Reply-To: <CAFBinCAaZumGU6dOq0RrHRTQV=MejTJ=RW0P_6tQFOG9vybY6g@mail.gmail.com> (Martin Blumenstingl's message of "Tue, 6 Aug 2024 23:03:25 +0200")

On Tue 06 Aug 2024 at 23:03, Martin Blumenstingl <martin.blumenstingl@googlemail.com> wrote:

> Hi Jerome,
>
> On Tue, Jul 30, 2024 at 2:50 PM Jerome Brunet <jbrunet@baylibre.com> wrote:
> [...]
>> +       }, {
>> +               .limit = 297000,
>> +               .regs = gxbb_3g_regs,
>> +               .reg_num = ARRAY_SIZE(gxbb_3g_regs)
> Just as a side-note: this looked odd when reading for the first time
> as I thought that it's a typo (and it should be gxbb_2g97_regs - but
> that name is not used).

I know it looks odd but there is a (perhaps silly) reason for it.

The names are derived from PHY modes used in the Amlogic vendor tree.
Those are magic pokes and we don't really know anything about it. The
vendor tree often update and mainline does not always follow. I know
that we are not 100% aligned right now. No one knows what branch is the
reference to follow anyway.

Using the same names is way to leave breadcrumbs that may help people
linking this to vendor code later on (if necessary)

I think the modes are named after the (rounded) bandwidth they provide,
not necessarily the limit we are using to pick it ... except for def,
which might just mean 'default' I guess.

https://github.com/khadas/linux/blob/khadas-vims-5.4.y/drivers/amlogic/media/vout/hdmitx/hdmi_tx_20/hw/hw_g12a.c#L589

I focused on keeping mainline as it was for the value poked, retaining
as much information as possible to find our way back.

Your proposed naming convention is fine by me as well.

>
> [...]
>> +static const struct meson_dw_hdmi_speed gxl_speeds[] = {
>> +       {
>> +               .limit = 371250,
>> +               .regs = gxl_3g7_regs,
>> +               .reg_num = ARRAY_SIZE(gxl_3g7_regs)
>> +       }, {
>> +               .limit = 297000,
>> +               .regs = gxl_3g_regs,
>> +               .reg_num = ARRAY_SIZE(gxl_3g_regs)
>> +       }, {
>> +               .limit = 148500,
>> +               .regs = gxl_def_regs,
>> +               .reg_num = ARRAY_SIZE(gxl_def_regs)
> this is not consistent with what we have above or below so it either
> needs to be updated or a comment.
> I think this should be called gxl_1g48_regs
>
>> +       }, {
>> +               .regs = gxl_270m_regs,
>> +               .reg_num = ARRAY_SIZE(gxl_270m_regs)
> and this should be called gxl_def_regs

def in not the last one, it is another name used by AML

It so happens that on mainline with we have only put the SD/270M for
gxl. In the AML tree, it does exist for G12 too. Maybe it will be needed someday.

>
>
>
> Best regards,
> Martin

-- 
Jerome

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic