| 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
| ||
|
Message-ID: <a5d6d63b-ae37-4cd6-94f7-50647c8b0a21@collabora.com>
Date: Mon, 10 Feb 2025 18:11:45 +0100
From: AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>
To: CK Hu (胡俊光) <ck.hu@...iatek.com>,
"chunkuang.hu@...nel.org" <chunkuang.hu@...nel.org>
Cc: "robh@...nel.org" <robh@...nel.org>,
"jie.qiu@...iatek.com" <jie.qiu@...iatek.com>,
"tzimmermann@...e.de" <tzimmermann@...e.de>,
"simona@...ll.ch" <simona@...ll.ch>, "mripard@...nel.org"
<mripard@...nel.org>, Jitao Shi (石记涛)
<jitao.shi@...iatek.com>,
"linux-mediatek@...ts.infradead.org" <linux-mediatek@...ts.infradead.org>,
"dri-devel@...ts.freedesktop.org" <dri-devel@...ts.freedesktop.org>,
"maarten.lankhorst@...ux.intel.com" <maarten.lankhorst@...ux.intel.com>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
"kernel@...labora.com" <kernel@...labora.com>,
"dmitry.baryshkov@...aro.org" <dmitry.baryshkov@...aro.org>,
"krzk+dt@...nel.org" <krzk+dt@...nel.org>,
Lewis Liao (廖柏鈞) <Lewis.Liao@...iatek.com>,
"p.zabel@...gutronix.de" <p.zabel@...gutronix.de>,
"conor+dt@...nel.org" <conor+dt@...nel.org>,
TommyYL Chen (陳彥良) <TommyYL.Chen@...iatek.com>,
Ives Chenjh (陳俊弘) <Ives.Chenjh@...iatek.com>,
"airlied@...il.com" <airlied@...il.com>,
"linux-arm-kernel@...ts.infradead.org"
<linux-arm-kernel@...ts.infradead.org>,
"matthias.bgg@...il.com" <matthias.bgg@...il.com>,
Jason-JH Lin (林睿祥) <Jason-JH.Lin@...iatek.com>,
"junzhi.zhao@...iatek.com" <junzhi.zhao@...iatek.com>
Subject: Re: [PATCH v5 29/34] drm/mediatek: mtk_hdmi: Split driver and add
common probe function
Il 10/02/25 03:18, CK Hu (胡俊光) ha scritto:
> Hi, Angelo:
>
> On Mon, 2025-01-13 at 15:52 +0100, AngeloGioacchino Del Regno wrote:
>> External email : Please do not click links or open attachments until you have verified the sender or the content.
>>
>>
>> In preparation for adding a new driver for the HDMI TX v2 IP,
>> split out the functions that will be common between the already
>> present mtk_hdmi (v1) driver and the new one.
>>
>> Since the probe flow for both drivers is 90% similar, add a common
>> probe function that will be called from each driver's .probe()
>> callback, avoiding lots of code duplication.
>>
>> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>
>> ---
>> drivers/gpu/drm/mediatek/Kconfig | 11 +-
>> drivers/gpu/drm/mediatek/Makefile | 1 +
>> drivers/gpu/drm/mediatek/mtk_hdmi.c | 553 +--------------------
>> drivers/gpu/drm/mediatek/mtk_hdmi_common.c | 426 ++++++++++++++++
>> drivers/gpu/drm/mediatek/mtk_hdmi_common.h | 188 +++++++
>> 5 files changed, 648 insertions(+), 531 deletions(-)
>> create mode 100644 drivers/gpu/drm/mediatek/mtk_hdmi_common.c
>> create mode 100644 drivers/gpu/drm/mediatek/mtk_hdmi_common.h
>>
>> diff --git a/drivers/gpu/drm/mediatek/Kconfig b/drivers/gpu/drm/mediatek/Kconfig
>> index e47debd60619..994b48b82d44 100644
>> --- a/drivers/gpu/drm/mediatek/Kconfig
>> +++ b/drivers/gpu/drm/mediatek/Kconfig
>> @@ -30,9 +30,18 @@ config DRM_MEDIATEK_DP
>> help
>> DRM/KMS Display Port driver for MediaTek SoCs.
>>
>> +config DRM_MEDIATEK_HDMI_COMMON
>> + tristate
>> + depends on DRM_MEDIATEK
>> + select DRM_DISPLAY_HDMI_HELPER
>> + select DRM_DISPLAY_HELPER
>> + select SND_SOC_HDMI_CODEC if SND_SOC
>> + help
>> + MediaTek SoC HDMI common library
>> +
>> config DRM_MEDIATEK_HDMI
>> tristate "DRM HDMI Support for Mediatek SoCs"
>> depends on DRM_MEDIATEK
>> - select SND_SOC_HDMI_CODEC if SND_SOC
>> + select DRM_MEDIATEK_HDMI_COMMON
>> help
>> DRM/KMS HDMI driver for Mediatek SoCs
>> diff --git a/drivers/gpu/drm/mediatek/Makefile b/drivers/gpu/drm/mediatek/Makefile
>> index 43afd0a26d14..78cf2d4fc85f 100644
>> --- a/drivers/gpu/drm/mediatek/Makefile
>> +++ b/drivers/gpu/drm/mediatek/Makefile
>> @@ -21,6 +21,7 @@ mediatek-drm-y := mtk_crtc.o \
>>
>> obj-$(CONFIG_DRM_MEDIATEK) += mediatek-drm.o
>>
>> +obj-$(CONFIG_DRM_MEDIATEK_HDMI_COMMON) += mtk_hdmi_common.o
>
> It's not necessary to create CONFIG_DRM_MEDIATEK_HDMI_COMMON to build mtk_hdmi_common.o.
>
> obj-$(CONFIG_DRM_MEDIATEK_HDMI) += mtk_hdmi_common.o
> obj-$(CONFIG_DRM_MEDIATEK_HDMI_V2) += mtk_hdmi_common.o
>
What you proposed would work - but it's not feasible: this is duplication, and
duplication is bad. That's why CONFIG_DRM_MEDIATEK_HDMI_COMMON was created.
I will not change that, sorry.
>> obj-$(CONFIG_DRM_MEDIATEK_HDMI) += mtk_cec.o
>> obj-$(CONFIG_DRM_MEDIATEK_HDMI) += mtk_hdmi.o
>> obj-$(CONFIG_DRM_MEDIATEK_HDMI) += mtk_hdmi_ddc.o
>> diff --git a/drivers/gpu/drm/mediatek/mtk_hdmi.c b/drivers/gpu/drm/mediatek/mtk_hdmi.c
>> index 138c6283c038..7940c9b598c1 100644
>> --- a/drivers/gpu/drm/mediatek/mtk_hdmi.c
>> +++ b/drivers/gpu/drm/mediatek/mtk_hdmi.c
..snip..
>> -
>> static int mtk_hdmi_probe(struct platform_device *pdev)
>> {
>> struct mtk_hdmi *hdmi;
>> - struct device *dev = &pdev->dev;
>> - const int num_clocks = MTK_HDMI_CLK_COUNT;
>> - int ret;
>> -
>> - hdmi = devm_kzalloc(dev, sizeof(*hdmi), GFP_KERNEL);
>> - if (!hdmi)
>> - return -ENOMEM;
>>
>> - hdmi->dev = dev;
>> - hdmi->conf = of_device_get_match_data(dev);
>> + hdmi = mtk_hdmi_common_probe(pdev);
>> + if (IS_ERR(hdmi))
>> + return PTR_ERR(hdmi);
>>
>> - hdmi->clk = devm_kcalloc(dev, num_clocks, sizeof(*hdmi->clk), GFP_KERNEL);
>> - if (!hdmi->clk)
>> - return -ENOMEM;
>> -
>> - ret = mtk_hdmi_dt_parse_pdata(hdmi, pdev, mtk_hdmi_clk_names, num_clocks);
>> - if (ret)
>> - return ret;
>> -
>> - hdmi->phy = devm_phy_get(dev, "hdmi");
>> - if (IS_ERR(hdmi->phy))
>> - return dev_err_probe(dev, PTR_ERR(hdmi->phy),
>> - "Failed to get HDMI PHY\n");
>> -
>> - mutex_init(&hdmi->update_plugged_status_lock);
>> - platform_set_drvdata(pdev, hdmi);
>> -
>> - ret = mtk_hdmi_register_audio_driver(dev);
>> - if (ret)
>> - return dev_err_probe(dev, ret,
>> - "Failed to register audio driver\n");
>> -
>> - hdmi->bridge.funcs = &mtk_hdmi_bridge_funcs;
>> - hdmi->bridge.of_node = pdev->dev.of_node;
>> - hdmi->bridge.ops = DRM_BRIDGE_OP_DETECT | DRM_BRIDGE_OP_EDID
>> - | DRM_BRIDGE_OP_HPD;
>> - hdmi->bridge.type = DRM_MODE_CONNECTOR_HDMIA;
>> - hdmi->bridge.vendor = "MediaTek";
>> - hdmi->bridge.product = "On-Chip HDMI";
>> -
>> - ret = devm_drm_bridge_add(dev, &hdmi->bridge);
>> - if (ret)
>> - return dev_err_probe(dev, ret, "Failed to add bridge\n");
>> + if (!hdmi->cec_dev)
>> + return dev_err_probe(hdmi->dev, -ENODEV, "CEC is required by HDMIv1\n");
>>
>> - ret = mtk_hdmi_clk_enable_audio(hdmi);
>> - if (ret)
>> - return dev_err_probe(dev, ret,
>> - "Failed to enable audio clocks\n");
>
> Originally, print error message for mtk_hdmi_clk_enable_audio() here.
> In this patch, it does not print error message here.
> If this modification is necessary, separate this to another patch.
>
I'll separate that to a different patch. Ok.
>> -
>> - return 0;
>> + return mtk_hdmi_clk_enable_audio(hdmi);
>> }
>>
>> static void mtk_hdmi_remove(struct platform_device *pdev)
..snip..
>> diff --git a/drivers/gpu/drm/mediatek/mtk_hdmi_common.c b/drivers/gpu/drm/mediatek/mtk_hdmi_common.c
>> new file mode 100644
>> index 000000000000..7dc7ba46f44d
>> --- /dev/null
>> +++ b/drivers/gpu/drm/mediatek/mtk_hdmi_common.c
..snip..
>> +
>> +static int mtk_hdmi_dt_parse_pdata(struct mtk_hdmi *hdmi, struct platform_device *pdev,
>> + const char * const *clk_names, size_t num_clocks)
>> +{
>> + struct device *dev = &pdev->dev;
>> + struct device_node *np = dev->of_node;
>> + struct device_node *remote, *i2c_np;
>> + int ret;
>> +
>> + ret = mtk_hdmi_get_all_clk(hdmi, np, clk_names, num_clocks);
>> + if (ret)
>> + return dev_err_probe(dev, ret, "Failed to get clocks\n");
>> +
>> + hdmi->regs = device_node_to_regmap(dev->of_node);
>> + if (IS_ERR(hdmi->regs))
>> + return PTR_ERR(hdmi->regs);
>> +
>> + remote = of_graph_get_remote_node(np, 1, 0);
>> + if (!remote)
>> + return -EINVAL;
>> +
>> + if (!of_device_is_compatible(remote, "hdmi-connector")) {
>> + hdmi->next_bridge = of_drm_find_bridge(remote);
>> + if (!hdmi->next_bridge) {
>> + dev_err(dev, "Waiting for external bridge\n");
>> + of_node_put(remote);
>> + return -EPROBE_DEFER;
>> + }
>> + }
>> +
>> + i2c_np = of_parse_phandle(remote, "ddc-i2c-bus", 0);
>> + of_node_put(remote);
>> + if (!i2c_np)
>> + return dev_err_probe(dev, -EINVAL, "No ddc-i2c-bus in connector\n");
>> +
>> + hdmi->ddc_adpt = of_find_i2c_adapter_by_node(i2c_np);
>> + of_node_put(i2c_np);
>> + if (!hdmi->ddc_adpt)
>> + return dev_err_probe(dev, -EINVAL, "Failed to get ddc i2c adapter by node\n");
>> +
>> + ret = mtk_hdmi_get_cec_dev(hdmi, dev, np);
>> + if (ret == -ENOTSUPP)
>> + dev_info(dev, "CEC support unavailable: node not found\n");
>
> CEC is used only in v1 driver. So separate CEC related modification to another patch.
>
Ok.
>> + else if (ret)
>> + return ret;
>> +
>> + return 0;
>> +}
>> +
..snip..
>> diff --git a/drivers/gpu/drm/mediatek/mtk_hdmi_common.h b/drivers/gpu/drm/mediatek/mtk_hdmi_common.h
>> new file mode 100644
>> index 000000000000..d3de8afff40f
>> --- /dev/null
>> +++ b/drivers/gpu/drm/mediatek/mtk_hdmi_common.h
>> @@ -0,0 +1,188 @@
..snip..
>> +struct mtk_hdmi_ver_conf {
>> + const struct drm_bridge_funcs *bridge_funcs;
>> + const struct hdmi_codec_ops *codec_ops;
>> + const char * const *mtk_hdmi_clock_names;
>> + int num_clocks;
>> +};
>
> This patch create something new for new hdmi driver.
> And this patch also separate common function to common file.
> I would like to separate these to two patches.
> One for creating something new for new hdmi driver.
> Another one for separating common function to common file.
>
That makes sense, will do.
Regards,
Angelo
> Regards,
> CK
>
>> +
>> +struct mtk_hdmi_conf {
>> + const struct mtk_hdmi_ver_conf *ver_conf;
>> + bool tz_disabled;
>> + bool cea_modes_only;
>> + unsigned long max_mode_clock;
>> +};
>> +
>> +struct mtk_hdmi {
>> + struct drm_bridge bridge;
>> + struct drm_bridge *next_bridge;
>> + struct drm_connector *curr_conn;/* current connector (only valid when 'enabled') */
>> + struct device *dev;
>> + const struct mtk_hdmi_conf *conf;
>> + struct phy *phy;
>> + struct device *cec_dev;
>> + struct i2c_adapter *ddc_adpt;
>> + struct clk **clk;
>> + struct drm_display_mode mode;
>> + bool dvi_mode;
>> + struct regmap *sys_regmap;
>> + unsigned int sys_offset;
>> + struct regmap *regs;
>> + struct platform_device *audio_pdev;
>> + struct hdmi_audio_param aud_param;
>> + bool audio_enable;
>> + bool powered;
>> + bool enabled;
>> + hdmi_codec_plugged_cb plugged_cb;
>> + struct device *codec_dev;
>> + struct mutex update_plugged_status_lock;
>> +};
>> +
>> +static inline struct mtk_hdmi *hdmi_ctx_from_bridge(struct drm_bridge *b)
>> +{
>> + return container_of(b, struct mtk_hdmi, bridge);
>> +}
>> +
>> +
>> +int mtk_hdmi_audio_get_eld(struct device *dev, void *data, uint8_t *buf, size_t len);
>> +void mtk_hdmi_audio_set_plugged_cb(struct mtk_hdmi *hdmi, hdmi_codec_plugged_cb fn,
>> + struct device *codec_dev);
>> +int mtk_hdmi_audio_params(struct mtk_hdmi *hdmi, struct hdmi_codec_daifmt *daifmt,
>> + struct hdmi_codec_params *params);
>> +void mtk_hdmi_get_ncts(unsigned int sample_rate, unsigned int clock,
>> + unsigned int *n, unsigned int *cts);
>> +bool mtk_hdmi_bridge_mode_fixup(struct drm_bridge *bridge,
>> + const struct drm_display_mode *mode,
>> + struct drm_display_mode *adjusted_mode);
>> +void mtk_hdmi_bridge_mode_set(struct drm_bridge *bridge,
>> + const struct drm_display_mode *mode,
>> + const struct drm_display_mode *adjusted_mode);
>> +struct mtk_hdmi *mtk_hdmi_common_probe(struct platform_device *pdev);
>> +#endif /* _MTK_HDMI_COMMON_H */
>> --
>> 2.47.0
>>
>
Powered by blists - more mailing lists