Re: [v14,06/28] drm/tests: Add output bpc tests
Hi, Maxime
I love you patch, yet it generates warnning calltrace. Despite it's
just a warning but it can overwhelm when we run kunit tests. Hence,
I suggest switch to the drm_atomic_connector_get_property() function.
Logs are pasted as below for easier to ready.
[ cut here ]
WARNING: CPU: 3 PID: 1264 at drivers/gpu/drm/drm_mode_object.c:354
drm_object_property_get_value+0x2c/0x34
Modules linked in: drm_connector_test drm_display_helper
drm_kunit_helpers kunit rfkill ip_set nf_tables nfnetlink vfat fat uas
usb_storage kvm efi_pstore pstore spi_loongson_pci spi_loongson_core
fuse efivarfs [last unloaded: drm_connector_test]
CPU: 3 PID: 1264 Comm: kunit_try_catch Tainted: G N
6.9.0+ #443
Hardware name: Loongson
Loongson-3A6000-HV-7A2000-XA61200/Loongson-3A6000-HV-7A2000-XA61200,
BIOS Loongson-UDK2018-V4.0.05636-stable202311 12/
pc 93469fec ra 8225afdc tp 90011fc54000 sp
90011fc57d80
a0 90010aa84658 a1 900104432a00 a2 90011fc57d98 a3
90011fc57d98
a4 900104432a4c a5 93f14e98 a6 0008 a7
fffe
t0 0010 t1 90010aa84000 t2 t3
c0c0c0c0
t4 c0c0c0c0 t5 0220 t6 0001 t7
00107203
t8 00107303 u0 0008 s9 9001000ebe60 s0
90010aa84000
s1 9001470679c8 s2 900104432a00 s3 82284000 s4
90010aa84658
s5 90010aa84618 s6 1000 s7 0001 s8
ra: 8225afdc drm_test_connector_hdmi_init_bpc_8+0xcc/0x2d0
[drm_connector_test]
ERA: 93469fec drm_object_property_get_value+0x2c/0x34
CRMD: 00b0 (PLV0 -IE -DA +PG DACF=CC DACM=CC -WE)
PRMD: 0004 (PPLV0 +PIE -PWE)
EUEN: (-FPE -SXE -ASXE -BTE)
ECFG: 00071c1c (LIE=2-4,10-12 VS=7)
ESTAT: 000c [BRK] (IS= ECode=12 EsubCode=0)
PRID: 0014d000 (Loongson-64bit, Loongson-3A6000-HV)
CPU: 3 PID: 1264 Comm: kunit_try_catch Tainted: G N
6.9.0+ #443
Hardware name: Loongson
Loongson-3A6000-HV-7A2000-XA61200/Loongson-3A6000-HV-7A2000-XA61200,
BIOS Loongson-UDK2018-V4.0.05636-stable202311 12/
Stack : 94065000 92ac339c
90011fc54000
90011fc579f0 90011fc579f8
90011fc57b38
90011fc57b30 90011fc57b30 90011fc57940
0001
0001 90011fc579f8 18e7bf3ffb6e59df
900100328a00
0001 0003 0434
4c206e6f73676e6f
6f4c203a656d616e 000d0ad3 0704c000
9001000ebe60
93ee6ab0
94065000
90010aa84618 1000
0001
92ac33b4
7dd80078
00b0 0004
00071c1c
...
Call Trace:
[<92ac33b4>] show_stack+0x5c/0x180
[<93b1ed2c>] dump_stack_lvl+0x70/0xa0
[<93b01fd8>] __warn+0x84/0xc8
[<93ad282c>] report_bug+0x19c/0x204
[<93b1fe00>] do_bp+0x264/0x2b4
[<>] 0x0
[<93469fec>] drm_object_property_get_value+0x2c/0x34
[] drm_test_connector_hdmi_init_bpc_8+0xcc/0x2d0
[drm_connector_test]
[] kunit_try_run_case+0x7c/0x18c [kunit]
[] kunit_generic_run_threadfn_adapter+0x1c/0x28 [kunit]
[<92b06238>] kthread+0x124/0x130
[<92ac1248>] ret_from_kernel_thread+0xc/0xa4
---[ end trace ]---
[ cut here ]
On 5/21/24 18:13, Maxime Ripard wrote:
Now that we're tracking the output bpc count in the connector state,
let's add a few tests to make sure it works as expected.
Reviewed-by: Dave Stevenson
Signed-off-by: Maxime Ripard
Reviewed-by: Dmitry Baryshkov
---
drivers/gpu/drm/Kconfig| 1 +
drivers/gpu/drm/tests/Makefile | 1 +
drivers/gpu/drm/tests/drm_connector_test.c | 140 +++
drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c | 438 +
drivers/gpu/drm/tests/drm_kunit_edid.h | 106 +
5 files changed, 686 insertions(+)
[...]
+
+/*
+ * Test that the registration of a connector with a maximum bpc count of
+ * 8 succeeds, registers the max bpc property, but doesn't register the
+ * HDR output metadata one.
+ */
+static void drm_test_connector_hdmi_init_bpc_8(struct kunit *test)
+{
+ struct drm_connector_init_priv *priv = test->priv;
+ struct drm_connector *connector = >connector;
+ struct drm_property *prop;
+ uint64_t val;
+ int ret;
+
+ ret = drmm_connector_hdmi_init(>drm, connector,
+ _funcs,
+ DRM_MODE_CONNECTOR_HDMIA,
+ >ddc,
+
Re: [v5,3/3] drm/mediatek: Implement OF graphs support for display paths
Hi,
On 5/21/24 15:57, AngeloGioacchino Del Regno wrote:
+static int mtk_drm_of_ddp_path_build(struct device *dev, struct device_node
*node,
+struct mtk_mmsys_driver_data *data)
+{
+ struct device_node *ep_node;
+ struct of_endpoint of_ep;
+ bool output_present[MAX_CRTC] = { false };
+ int ret;
+
+ for_each_endpoint_of_node(node, ep_node) {
+ ret = of_graph_parse_endpoint(ep_node, _ep);
+ of_node_put(ep_node);
There is going to *double* decline the reference counter, as the
__of_get_next_child() will decrease the reference counter for us
on the next iteration.
+ if (ret) {
+ dev_err_probe(dev, ret, "Cannot parse endpoint\n");
+ break;
+ }
Move the 'of_node_put(ep_node)' into brace '{}' here, if we really cares
about the reference count.
+
+ if (of_ep.id >= MAX_CRTC) {
ditto ?
+ ret = dev_err_probe(dev, -EINVAL,
+ "Invalid endpoint%u number\n",
of_ep.port);
+ break;
+ }
+
+ output_present[of_ep.id] = true;
+ }
+
+ if (ret)
+ return ret;
+
+ if (output_present[CRTC_MAIN]) {
+ ret = mtk_drm_of_ddp_path_build_one(dev, CRTC_MAIN,
+ >main_path,
>main_len);
+ if (ret)
+ return ret;
+ }
+
+ if (output_present[CRTC_EXT]) {
+ ret = mtk_drm_of_ddp_path_build_one(dev, CRTC_EXT,
+ >ext_path,
>ext_len);
+ if (ret)
+ return ret;
+ }
+
+ if (output_present[CRTC_THIRD]) {
+ ret = mtk_drm_of_ddp_path_build_one(dev, CRTC_THIRD,
+ >third_path,
>third_len);
+ if (ret)
+ return ret;
+ }
+
+ return 0;
+}
+
--
Best regards
Sui Jingfeng
Re: [v5,3/3] drm/mediatek: Implement OF graphs support for display paths
Hi,
On 5/21/24 15:57, AngeloGioacchino Del Regno wrote:
+static int mtk_drm_of_get_ddp_comp_type(struct device_node *node, enum
mtk_ddp_comp_type *ctype)
+{
+ const struct of_device_id *of_id = of_match_node(mtk_ddp_comp_dt_ids,
node);
+
+ if (!of_id)
+ return -EINVAL;
+
+ *ctype = (enum mtk_ddp_comp_type)((uintptr_t)of_id->data);
+
+ return 0;
+}
+
+static int mtk_drm_of_get_ddp_ep_cid(struct device_node *node,
+int output_port, enum mtk_crtc_path
crtc_path,
+struct device_node **next, unsigned int
*cid)
+{
+ struct device_node *ep_dev_node, *ep_out;
+ enum mtk_ddp_comp_type comp_type;
+ int ret;
+
+ ep_out = of_graph_get_endpoint_by_regs(node, output_port, crtc_path);
+ if (!ep_out)
+ return -ENOENT;
+
+ ep_dev_node = of_graph_get_remote_port_parent(ep_out);
below here, 'ep_out' will not be used anymore.
of_node_put(ep_out);
Maybe we should call it under a error handling tag?
But this is trivial problem.
+ if (!ep_dev_node)
+ return -EINVAL;
+
+ /*
+* Pass the next node pointer regardless of failures in the later code
+* so that if this function is called in a loop it will walk through all
+* of the subsequent endpoints anyway.
+*/
+ *next = ep_dev_node;
+
+ if (!of_device_is_available(ep_dev_node))
+ return -ENODEV;
+
+ ret = mtk_drm_of_get_ddp_comp_type(ep_dev_node, _type);
+ if (ret) {
+ if (mtk_ovl_adaptor_is_comp_present(ep_dev_node)) {
+ *cid = (unsigned int)DDP_COMPONENT_DRM_OVL_ADAPTOR;
+ return 0;
+ }
+ return ret;
+ }
+
+ ret = mtk_ddp_comp_get_id(ep_dev_node, comp_type);
+ if (ret < 0)
+ return ret;
+
+ /* All ok! Pass the Component ID to the caller. */
+ *cid = (unsigned int)ret;
+
+ return 0;
+}
+
--
Best regards
Sui Jingfeng
Re: [v5,3/3] drm/mediatek: Implement OF graphs support for display paths
Hi, On 5/22/24 19:48, Sui Jingfeng wrote: if the not bridge is not ready 'not' -> 'next'
Re: [v5,3/3] drm/mediatek: Implement OF graphs support for display paths
Hi,
Looks good to me in overall!
On 5/21/24 15:57, AngeloGioacchino Del Regno wrote:
It is impossible to add each and every possible DDP path combination
for each and every possible combination of SoC and board: right now,
this driver hardcodes configuration for 10 SoCs and this is going to
grow larger and larger, and with new hacks like the introduction of
mtk_drm_route which is anyway not enough for all final routes as the
DSI cannot be connected to MERGE if it's not a dual-DSI, or enabling
DSC preventively doesn't work if the display doesn't support it, or
others.
Since practically all display IPs in MediaTek SoCs support being
interconnected with different instances of other, or the same, IPs
or with different IPs and in different combinations, the final DDP
pipeline is effectively a board specific configuration.
Implement OF graphs support to the mediatek-drm drivers, allowing to
stop hardcoding the paths, and preventing this driver to get a huge
amount of arrays for each board and SoC combination, also paving the
way to share the same mtk_mmsys_driver_data between multiple SoCs,
making it more straightforward to add support for new chips.
Reviewed-by: Alexandre Mergnat
Tested-by: Alexandre Mergnat
Signed-off-by: AngeloGioacchino Del Regno
---
drivers/gpu/drm/mediatek/mtk_disp_drv.h | 1 +
.../gpu/drm/mediatek/mtk_disp_ovl_adaptor.c | 40 ++-
drivers/gpu/drm/mediatek/mtk_dpi.c| 16 +-
drivers/gpu/drm/mediatek/mtk_drm_drv.c| 282 --
drivers/gpu/drm/mediatek/mtk_drm_drv.h| 2 +-
drivers/gpu/drm/mediatek/mtk_dsi.c| 10 +-
6 files changed, 313 insertions(+), 38 deletions(-)
diff --git a/drivers/gpu/drm/mediatek/mtk_disp_drv.h
b/drivers/gpu/drm/mediatek/mtk_disp_drv.h
index 082ac18fe04a..94843974851f 100644
--- a/drivers/gpu/drm/mediatek/mtk_disp_drv.h
+++ b/drivers/gpu/drm/mediatek/mtk_disp_drv.h
@@ -108,6 +108,7 @@ size_t mtk_ovl_get_num_formats(struct device *dev);
void mtk_ovl_adaptor_add_comp(struct device *dev, struct mtk_mutex *mutex);
void mtk_ovl_adaptor_remove_comp(struct device *dev, struct mtk_mutex *mutex);
+bool mtk_ovl_adaptor_is_comp_present(struct device_node *node);
void mtk_ovl_adaptor_connect(struct device *dev, struct device *mmsys_dev,
unsigned int next);
void mtk_ovl_adaptor_disconnect(struct device *dev, struct device *mmsys_dev,
diff --git a/drivers/gpu/drm/mediatek/mtk_disp_ovl_adaptor.c
b/drivers/gpu/drm/mediatek/mtk_disp_ovl_adaptor.c
index 02dd7dcdfedb..400519d1ca1f 100644
--- a/drivers/gpu/drm/mediatek/mtk_disp_ovl_adaptor.c
+++ b/drivers/gpu/drm/mediatek/mtk_disp_ovl_adaptor.c
@@ -491,6 +491,38 @@ static int compare_of(struct device *dev, void *data)
return dev->of_node == data;
}
+static int ovl_adaptor_of_get_ddp_comp_type(struct device_node *node,
+ enum mtk_ovl_adaptor_comp_type
*ctype)
+{
+ const struct of_device_id *of_id =
of_match_node(mtk_ovl_adaptor_comp_dt_ids, node);
+
+ if (!of_id)
+ return -EINVAL;
+
+ *ctype = (enum mtk_ovl_adaptor_comp_type)((uintptr_t)of_id->data);
+
+ return 0;
+}
+
+bool mtk_ovl_adaptor_is_comp_present(struct device_node *node)
+{
+ enum mtk_ovl_adaptor_comp_type type;
+ int ret;
+
+ ret = ovl_adaptor_of_get_ddp_comp_type(node, );
+ if (ret)
+ return false;
+
+ if (type >= OVL_ADAPTOR_TYPE_NUM)
+ return false;
+
+ /*
+* ETHDR and Padding are used exclusively in OVL Adaptor: if this
+* component is not one of those, it's likely not an OVL Adaptor path.
+*/
+ return type == OVL_ADAPTOR_TYPE_ETHDR || type ==
OVL_ADAPTOR_TYPE_PADDING;
+}
+
static int ovl_adaptor_comp_init(struct device *dev, struct component_match
**match)
{
struct mtk_disp_ovl_adaptor *priv = dev_get_drvdata(dev);
@@ -500,12 +532,11 @@ static int ovl_adaptor_comp_init(struct device *dev,
struct component_match **ma
parent = dev->parent->parent->of_node->parent;
for_each_child_of_node(parent, node) {
- const struct of_device_id *of_id;
enum mtk_ovl_adaptor_comp_type type;
- int id;
+ int id, ret;
- of_id = of_match_node(mtk_ovl_adaptor_comp_dt_ids, node);
- if (!of_id)
+ ret = ovl_adaptor_of_get_ddp_comp_type(node, );
+ if (ret)
continue;
if (!of_device_is_available(node)) {
@@ -514,7 +545,6 @@ static int ovl_adaptor_comp_init(struct device *dev, struct
component_match **ma
continue;
}
- type = (enum mtk_ovl_adaptor_comp_type)(uintptr_t)of_id->data;
id = ovl_adaptor_comp_get_id(dev, node, type);
if (id < 0) {
dev_warn(dev, "Skipping unknown component %pOF\n",
diff --git
Re: [PATCH] drm/bridge: adv7511: Exit interrupt handling when necessary
Hi,
On 5/20/24 19:13, Dmitry Baryshkov wrote:
On Mon, 20 May 2024 at 14:11, Sui Jingfeng wrote:
Hi,
On 5/20/24 06:11, Dmitry Baryshkov wrote:
On Thu, May 16, 2024 at 06:10:06PM +0800, Liu Ying wrote:
Commit f3d9683346d6 ("drm/bridge: adv7511: Allow IRQ to share GPIO pins")
fails to consider the case where adv7511->i2c_main->irq is zero, i.e.,
no interrupt requested at all.
Without interrupt, adv7511_wait_for_edid() could return -EIO sometimes,
because it polls adv7511->edid_read flag by calling adv7511_irq_process()
a few times, but adv7511_irq_process() happens to refuse to handle
interrupt by returning -ENODATA. Hence, EDID retrieval fails randomly.
Fix the issue by checking adv7511->i2c_main->irq before exiting interrupt
handling from adv7511_irq_process().
Fixes: f3d9683346d6 ("drm/bridge: adv7511: Allow IRQ to share GPIO pins")
Signed-off-by: Liu Ying
---
drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
index 6089b0bb9321..2074fa3c1b7b 100644
--- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
+++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
@@ -479,7 +479,8 @@ static int adv7511_irq_process(struct adv7511 *adv7511,
bool process_hpd)
return ret;
/* If there is no IRQ to handle, exit indicating no IRQ data */
-if (!(irq0 & (ADV7511_INT0_HPD | ADV7511_INT0_EDID_READY)) &&
+if (adv7511->i2c_main->irq &&
+!(irq0 & (ADV7511_INT0_HPD | ADV7511_INT0_EDID_READY)) &&
!(irq1 & ADV7511_INT1_DDC_ERROR))
return -ENODATA;
I think it might be better to handle -ENODATA in adv7511_wait_for_edid()
instead. WDYT?
I think this is may deserve another patch.
My point is that the IRQ handler is fine to remove -ENODATA here,
[...]
there is no pending IRQ that can be handled.
But there may has other things need to do in the adv7511_irq_process()
function.
So instead of continuing
the execution when we know that IRQ bits are not set,
Even when IRQ bits are not set, it just means that there is no HPD
and no EDID ready-to-read signal. HDMI CEC interrupts still need
to process.
it's better to
ignore -ENODATA in the calling code and go on with msleep().
So, It's confusing to ignore the -ENODATA here.
--
Best regards
Sui
Re: [PATCH] drm/bridge: adv7511: Exit interrupt handling when necessary
Hi,
On 5/20/24 06:11, Dmitry Baryshkov wrote:
On Thu, May 16, 2024 at 06:10:06PM +0800, Liu Ying wrote:
Commit f3d9683346d6 ("drm/bridge: adv7511: Allow IRQ to share GPIO pins")
fails to consider the case where adv7511->i2c_main->irq is zero, i.e.,
no interrupt requested at all.
Without interrupt, adv7511_wait_for_edid() could return -EIO sometimes,
because it polls adv7511->edid_read flag by calling adv7511_irq_process()
a few times, but adv7511_irq_process() happens to refuse to handle
interrupt by returning -ENODATA. Hence, EDID retrieval fails randomly.
Fix the issue by checking adv7511->i2c_main->irq before exiting interrupt
handling from adv7511_irq_process().
Fixes: f3d9683346d6 ("drm/bridge: adv7511: Allow IRQ to share GPIO pins")
Signed-off-by: Liu Ying
---
drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
index 6089b0bb9321..2074fa3c1b7b 100644
--- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
+++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
@@ -479,7 +479,8 @@ static int adv7511_irq_process(struct adv7511 *adv7511,
bool process_hpd)
return ret;
/* If there is no IRQ to handle, exit indicating no IRQ data */
- if (!(irq0 & (ADV7511_INT0_HPD | ADV7511_INT0_EDID_READY)) &&
+ if (adv7511->i2c_main->irq &&
+ !(irq0 & (ADV7511_INT0_HPD | ADV7511_INT0_EDID_READY)) &&
!(irq1 & ADV7511_INT1_DDC_ERROR))
return -ENODATA;
I think it might be better to handle -ENODATA in adv7511_wait_for_edid()
instead. WDYT?
I think this is may deserve another patch.
--
Best regards
Sui
[etnaviv-next v14 8/8] drm/etnaviv: Add support for vivante GPU cores attached via PCIe device
Previouly, the component framework is being used to bind multiple platform
GPU devices to a virtual master. The virtual master is manually created by
the driver, and is also a platform device. This is fine and works well for
various SoCs, yet there some hardware venders integrate Vivante GPU cores
into PCIe card and the driver lacks the support for PCIe devices.
Create virtual platform devices as a representation for each GPU IP core,
the manually created platform devices are functional as subcomponent, and
all of them are child of the PCIe master device. The master is real for
PCIe devices, as the PCIe device has already been created by the time the
etnaviv.ko is loaded. Hence, bind all of the virtual child to the real
master, this design reflects the hardware layout perfectly and is
extensible.
Signed-off-by: Sui Jingfeng
---
drivers/gpu/drm/etnaviv/Kconfig | 9 ++
drivers/gpu/drm/etnaviv/Makefile | 2 +
drivers/gpu/drm/etnaviv/etnaviv_drv.c | 12 +-
drivers/gpu/drm/etnaviv/etnaviv_drv.h | 2 +
drivers/gpu/drm/etnaviv/etnaviv_gpu.c | 75 --
drivers/gpu/drm/etnaviv/etnaviv_gpu.h | 4 +
drivers/gpu/drm/etnaviv/etnaviv_pci_drv.c | 161 ++
drivers/gpu/drm/etnaviv/etnaviv_pci_drv.h | 44 ++
8 files changed, 293 insertions(+), 16 deletions(-)
create mode 100644 drivers/gpu/drm/etnaviv/etnaviv_pci_drv.c
create mode 100644 drivers/gpu/drm/etnaviv/etnaviv_pci_drv.h
diff --git a/drivers/gpu/drm/etnaviv/Kconfig b/drivers/gpu/drm/etnaviv/Kconfig
index faa7fc68b009..7cb44f72d512 100644
--- a/drivers/gpu/drm/etnaviv/Kconfig
+++ b/drivers/gpu/drm/etnaviv/Kconfig
@@ -15,6 +15,15 @@ config DRM_ETNAVIV
help
DRM driver for Vivante GPUs.
+config DRM_ETNAVIV_PCI_DRIVER
+ bool "enable ETNAVIV PCI driver support"
+ depends on DRM_ETNAVIV
+ depends on PCI
+ default n
+ help
+ Compile in support for Vivante GPUs attached via PCIe card.
+ Say Y if you have such hardwares.
+
config DRM_ETNAVIV_THERMAL
bool "enable ETNAVIV thermal throttling"
depends on DRM_ETNAVIV
diff --git a/drivers/gpu/drm/etnaviv/Makefile b/drivers/gpu/drm/etnaviv/Makefile
index 46e5ffad69a6..6829e1ebf2db 100644
--- a/drivers/gpu/drm/etnaviv/Makefile
+++ b/drivers/gpu/drm/etnaviv/Makefile
@@ -16,4 +16,6 @@ etnaviv-y := \
etnaviv_perfmon.o \
etnaviv_sched.o
+etnaviv-$(CONFIG_DRM_ETNAVIV_PCI_DRIVER) += etnaviv_pci_drv.o
+
obj-$(CONFIG_DRM_ETNAVIV) += etnaviv.o
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_drv.c
b/drivers/gpu/drm/etnaviv/etnaviv_drv.c
index dc3556aad134..90ee60b00c24 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_drv.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_drv.c
@@ -24,6 +24,7 @@
#include "etnaviv_gpu.h"
#include "etnaviv_gem.h"
#include "etnaviv_mmu.h"
+#include "etnaviv_pci_drv.h"
#include "etnaviv_perfmon.h"
/*
@@ -568,6 +569,10 @@ static int etnaviv_bind(struct device *dev)
if (ret < 0)
goto out_free_priv;
+ ret = etnaviv_register_irq_handler(dev, priv);
+ if (ret)
+ goto out_unbind;
+
load_gpu(drm);
ret = drm_dev_register(drm, 0);
@@ -596,7 +601,7 @@ static void etnaviv_unbind(struct device *dev)
etnaviv_private_fini(priv);
}
-static const struct component_master_ops etnaviv_master_ops = {
+const struct component_master_ops etnaviv_master_ops = {
.bind = etnaviv_bind,
.unbind = etnaviv_unbind,
};
@@ -740,6 +745,10 @@ static int __init etnaviv_init(void)
if (ret != 0)
goto unregister_gpu_driver;
+ ret = etnaviv_register_pci_driver();
+ if (ret)
+ goto unregister_platform_driver;
+
/*
* If the DT contains at least one available GPU device, instantiate
* the DRM platform device.
@@ -769,6 +778,7 @@ module_init(etnaviv_init);
static void __exit etnaviv_exit(void)
{
etnaviv_destroy_platform_device(_drm);
+ etnaviv_unregister_pci_driver();
platform_driver_unregister(_platform_driver);
platform_driver_unregister(_gpu_driver);
}
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_drv.h
b/drivers/gpu/drm/etnaviv/etnaviv_drv.h
index 4612843ff9f6..6db26d384cbe 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_drv.h
+++ b/drivers/gpu/drm/etnaviv/etnaviv_drv.h
@@ -27,6 +27,8 @@ struct etnaviv_gem_object;
struct etnaviv_gem_submit;
struct etnaviv_iommu_global;
+extern const struct component_master_ops etnaviv_master_ops;
+
#define ETNAVIV_SOFTPIN_START_ADDRESS SZ_4M /* must be >= SUBALLOC_SIZE */
struct etnaviv_file_private {
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
index 3a14e187388a..2b5955693fbb 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
@@ -10,6 +10,7 @@
#include
#in
[etnaviv-next v14 7/8] drm/etnaviv: Allow creating subdevices and pass platform specific data
Because some hardware are too complex to be managed by a monolithic driver,
a split of the functionality into child devices can helps to achieve better
modularity.
We will use this function to create subdevice as a repensentation of a
single hardware ip block, so that the same modular approach that works
for ARM-SoC can also works for PCIe cards.
Signed-off-by: Sui Jingfeng
---
drivers/gpu/drm/etnaviv/etnaviv_drv.c | 33 +++
drivers/gpu/drm/etnaviv/etnaviv_drv.h | 9
2 files changed, 37 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_drv.c
b/drivers/gpu/drm/etnaviv/etnaviv_drv.c
index 863faac2ea19..dc3556aad134 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_drv.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_drv.c
@@ -670,16 +670,36 @@ static struct platform_driver etnaviv_platform_driver = {
},
};
-static int etnaviv_create_platform_device(const char *name,
- struct platform_device **ppdev)
+int etnaviv_create_platform_device(struct device *parent,
+ const char *name, int id,
+ struct resource *pres,
+ void *data,
+ struct platform_device **ppdev)
{
struct platform_device *pdev;
int ret;
- pdev = platform_device_alloc(name, PLATFORM_DEVID_NONE);
+ pdev = platform_device_alloc(name, id);
if (!pdev)
return -ENOMEM;
+ pdev->dev.parent = parent;
+
+ if (pres) {
+ ret = platform_device_add_resources(pdev, pres, 1);
+ if (ret) {
+ platform_device_put(pdev);
+ return ret;
+ }
+ }
+
+ if (data) {
+ void *pdata = kmalloc(sizeof(void *), GFP_KERNEL);
+
+ *(void **)pdata = data;
+ pdev->dev.platform_data = pdata;
+ }
+
ret = platform_device_add(pdev);
if (ret) {
platform_device_put(pdev);
@@ -691,7 +711,7 @@ static int etnaviv_create_platform_device(const char *name,
return 0;
}
-static void etnaviv_destroy_platform_device(struct platform_device **ppdev)
+void etnaviv_destroy_platform_device(struct platform_device **ppdev)
{
struct platform_device *pdev = *ppdev;
@@ -728,7 +748,10 @@ static int __init etnaviv_init(void)
if (np) {
of_node_put(np);
- ret = etnaviv_create_platform_device("etnaviv", _drm);
+ ret = etnaviv_create_platform_device(NULL, "etnaviv",
+PLATFORM_DEVID_NONE,
+NULL, NULL,
+_drm);
if (ret)
goto unregister_platform_driver;
}
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_drv.h
b/drivers/gpu/drm/etnaviv/etnaviv_drv.h
index 4b59fdb457b7..4612843ff9f6 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_drv.h
+++ b/drivers/gpu/drm/etnaviv/etnaviv_drv.h
@@ -9,6 +9,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -98,6 +99,14 @@ bool etnaviv_cmd_validate_one(struct etnaviv_gpu *gpu,
u32 *stream, unsigned int size,
struct drm_etnaviv_gem_submit_reloc *relocs, unsigned int reloc_size);
+int etnaviv_create_platform_device(struct device *parent,
+ const char *name, int id,
+ struct resource *pres,
+ void *data,
+ struct platform_device **ppdev);
+
+void etnaviv_destroy_platform_device(struct platform_device **ppdev);
+
#ifdef CONFIG_DEBUG_FS
void etnaviv_gem_describe_objects(struct etnaviv_drm_private *priv,
struct seq_file *m);
--
2.34.1
[etnaviv-next v14 6/8] drm/etnaviv: Replace the '>dev' with 'dev'
In the etnaviv_pdev_probe(), etnaviv_gpu_platform_probe() function, the
value of '>dev' has been cached to the 'dev' local auto variable.
But part of callers use 'dev' as argument, while the rest use '>dev'.
To keep it consistent, use 'dev' uniformly.
Signed-off-by: Sui Jingfeng
---
drivers/gpu/drm/etnaviv/etnaviv_drv.c | 10 +-
drivers/gpu/drm/etnaviv/etnaviv_gpu.c | 16
2 files changed, 13 insertions(+), 13 deletions(-)
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_drv.c
b/drivers/gpu/drm/etnaviv/etnaviv_drv.c
index 986fd68b489a..863faac2ea19 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_drv.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_drv.c
@@ -614,7 +614,7 @@ static int etnaviv_pdev_probe(struct platform_device *pdev)
if (!of_device_is_available(core_node))
continue;
- drm_of_component_match_add(>dev, ,
+ drm_of_component_match_add(dev, ,
component_compare_of,
core_node);
}
} else {
@@ -637,9 +637,9 @@ static int etnaviv_pdev_probe(struct platform_device *pdev)
* bit to make sure we are allocating the command buffers and
* TLBs in the lower 4 GiB address space.
*/
- if (dma_set_mask(>dev, DMA_BIT_MASK(40)) ||
- dma_set_coherent_mask(>dev, DMA_BIT_MASK(32))) {
- dev_dbg(>dev, "No suitable DMA available\n");
+ if (dma_set_mask(dev, DMA_BIT_MASK(40)) ||
+ dma_set_coherent_mask(dev, DMA_BIT_MASK(32))) {
+ dev_dbg(dev, "No suitable DMA available\n");
return -ENODEV;
}
@@ -650,7 +650,7 @@ static int etnaviv_pdev_probe(struct platform_device *pdev)
*/
first_node = etnaviv_of_first_available_node();
if (first_node) {
- of_dma_configure(>dev, first_node, true);
+ of_dma_configure(dev, first_node, true);
of_node_put(first_node);
}
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
index aa15682f94db..3a14e187388a 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
@@ -1891,7 +1891,7 @@ static int etnaviv_gpu_platform_probe(struct
platform_device *pdev)
if (!gpu)
return -ENOMEM;
- gpu->dev = >dev;
+ gpu->dev = dev;
mutex_init(>lock);
mutex_init(>sched_lock);
@@ -1905,8 +1905,8 @@ static int etnaviv_gpu_platform_probe(struct
platform_device *pdev)
if (gpu->irq < 0)
return gpu->irq;
- err = devm_request_irq(>dev, gpu->irq, irq_handler, 0,
- dev_name(gpu->dev), gpu);
+ err = devm_request_irq(dev, gpu->irq, irq_handler, 0,
+ dev_name(dev), gpu);
if (err) {
dev_err(dev, "failed to request IRQ%u: %d\n", gpu->irq, err);
return err;
@@ -1925,13 +1925,13 @@ static int etnaviv_gpu_platform_probe(struct
platform_device *pdev)
* autosuspend delay is rather arbitary: no measurements have
* yet been performed to determine an appropriate value.
*/
- pm_runtime_use_autosuspend(gpu->dev);
- pm_runtime_set_autosuspend_delay(gpu->dev, 200);
- pm_runtime_enable(gpu->dev);
+ pm_runtime_use_autosuspend(dev);
+ pm_runtime_set_autosuspend_delay(dev, 200);
+ pm_runtime_enable(dev);
- err = component_add(>dev, _ops);
+ err = component_add(dev, _ops);
if (err < 0) {
- dev_err(>dev, "failed to register component: %d\n", err);
+ dev_err(dev, "failed to register component: %d\n", err);
return err;
}
--
2.34.1
[etnaviv-next v14 5/8] drm/etnaviv: Add support for cached coherent caching mode
Many modern CPUs and/or platforms choose to define their peripheral devices
as cached coherent by default, to be specific, the PCH is capable of
snooping CPU's cache. When hit the peripheral devices will access data
directly from CPU's cache. This means that device drivers do not need to
maintain the coherency issue between a processor and peripheral I/O for
the cached buffers. Hence, it dosen't need us to sync manually on the
software side, which is useful to avoid some overheads, especially for
userspace, but userspace is not known yet.
Probe the hardware maintained cached coherent support of the host platform
with the dev_is_dma_coherent() function, and store the result in struct
etnaviv_drm_private. As this is a platform implementation-defined hardware
feature and again is meant to be shared by all GPU cores. And expose it
via etnaviv parameter mechanism to let userspace know.
Please note that write-combine mapping out of scope of the discussion
and therefore is not being addressed.
Signed-off-by: Sui Jingfeng
---
drivers/gpu/drm/etnaviv/etnaviv_drv.c | 3 +++
drivers/gpu/drm/etnaviv/etnaviv_drv.h | 9 +
drivers/gpu/drm/etnaviv/etnaviv_gpu.c | 4
include/uapi/drm/etnaviv_drm.h| 1 +
4 files changed, 17 insertions(+)
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_drv.c
b/drivers/gpu/drm/etnaviv/etnaviv_drv.c
index e3eb31ba9a2b..986fd68b489a 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_drv.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_drv.c
@@ -5,6 +5,7 @@
#include
#include
+#include
#include
#include
#include
@@ -57,6 +58,8 @@ static int etnaviv_private_init(struct device *dev,
return -ENOMEM;
}
+ priv->cached_coherent = dev_is_dma_coherent(dev);
+
return 0;
}
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_drv.h
b/drivers/gpu/drm/etnaviv/etnaviv_drv.h
index 1f9b50b5a6aa..4b59fdb457b7 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_drv.h
+++ b/drivers/gpu/drm/etnaviv/etnaviv_drv.h
@@ -46,6 +46,15 @@ struct etnaviv_drm_private {
struct xarray active_contexts;
u32 next_context_id;
+ /*
+* If true, the cached mapping is consistent for all CPU cores and
+* peripheral bus masters in the system. It means that both of the
+* CPU and GPU will see the same data if the buffer being accessed
+* is cached. And the coherency is guaranteed by the host platform
+* specific hardwares.
+*/
+ bool cached_coherent;
+
/* list of GEM objects: */
struct mutex gem_lock;
struct list_head gem_list;
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
index 02d7efdc82c0..aa15682f94db 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
@@ -164,6 +164,10 @@ int etnaviv_gpu_get_param(struct etnaviv_gpu *gpu, u32
param, u64 *value)
*value = gpu->identity.eco_id;
break;
+ case ETNAVIV_PARAM_CACHED_COHERENT:
+ *value = priv->cached_coherent;
+ break;
+
default:
DBG("%s: invalid param: %u", dev_name(gpu->dev), param);
return -EINVAL;
diff --git a/include/uapi/drm/etnaviv_drm.h b/include/uapi/drm/etnaviv_drm.h
index af024d90453d..61eaa8cd0f5e 100644
--- a/include/uapi/drm/etnaviv_drm.h
+++ b/include/uapi/drm/etnaviv_drm.h
@@ -77,6 +77,7 @@ struct drm_etnaviv_timespec {
#define ETNAVIV_PARAM_GPU_PRODUCT_ID0x1c
#define ETNAVIV_PARAM_GPU_CUSTOMER_ID 0x1d
#define ETNAVIV_PARAM_GPU_ECO_ID0x1e
+#define ETNAVIV_PARAM_CACHED_COHERENT 0x1f
#define ETNA_MAX_PIPES 4
--
2.34.1
[etnaviv-next v14 4/8] drm/etnaviv: Fix wrong cache property being used for vmap()
In the etnaviv_gem_vmap_impl() function, the driver vmap whatever buffers
with Write-Combine page property. This is unreasonable, as some platforms
are cached coherent. And cached buffers should be mapped with cached page
property.
Fixes: a0a5ab3e99b8 ("drm/etnaviv: call correct function when trying to vmap a
DMABUF")
Signed-off-by: Sui Jingfeng
---
drivers/gpu/drm/etnaviv/etnaviv_gem.c | 16 ++--
1 file changed, 14 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem.c
b/drivers/gpu/drm/etnaviv/etnaviv_gem.c
index aa95a5e98374..eed98bb9e446 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_gem.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_gem.c
@@ -342,6 +342,7 @@ void *etnaviv_gem_vmap(struct drm_gem_object *obj)
static void *etnaviv_gem_vmap_impl(struct etnaviv_gem_object *obj)
{
struct page **pages;
+ pgprot_t prot;
lockdep_assert_held(>lock);
@@ -349,8 +350,19 @@ static void *etnaviv_gem_vmap_impl(struct
etnaviv_gem_object *obj)
if (IS_ERR(pages))
return NULL;
- return vmap(pages, obj->base.size >> PAGE_SHIFT,
- VM_MAP, pgprot_writecombine(PAGE_KERNEL));
+ switch (obj->flags) {
+ case ETNA_BO_CACHED:
+ prot = PAGE_KERNEL;
+ break;
+ case ETNA_BO_UNCACHED:
+ prot = pgprot_noncached(PAGE_KERNEL);
+ break;
+ case ETNA_BO_WC:
+ default:
+ prot = pgprot_writecombine(PAGE_KERNEL);
+ }
+
+ return vmap(pages, obj->base.size >> PAGE_SHIFT, VM_MAP, prot);
}
static inline enum dma_data_direction etnaviv_op_to_dma_dir(u32 op)
--
2.34.1
[etnaviv-next v14 3/8] drm/etnaviv: Embed struct drm_device into struct etnaviv_drm_private
Both the instance of struct drm_device and the instance of struct
etnaviv_drm_private are intended to be shared by all GPU cores, both have
only one instance created across drm/etnaviv driver. After embedded in,
the whole structure can be allocated with devm_drm_dev_alloc(). And the
DRM device created is automatically put on driver detach, so we don't need
to call drm_dev_put() explicitly on driver leave. It's also eliminate the
need to use the .dev_private member, which is deprecated according to the
drm document. We can also use container_of() to retrieve pointer for the
containing structure.
Signed-off-by: Sui Jingfeng
---
drivers/gpu/drm/etnaviv/etnaviv_drv.c| 65
drivers/gpu/drm/etnaviv/etnaviv_drv.h| 7 +++
drivers/gpu/drm/etnaviv/etnaviv_gem.c| 6 +-
drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c | 2 +-
drivers/gpu/drm/etnaviv/etnaviv_gpu.c| 6 +-
drivers/gpu/drm/etnaviv/etnaviv_mmu.c| 4 +-
6 files changed, 40 insertions(+), 50 deletions(-)
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_drv.c
b/drivers/gpu/drm/etnaviv/etnaviv_drv.c
index 22c78bc944c4..e3eb31ba9a2b 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_drv.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_drv.c
@@ -41,14 +41,9 @@ static struct device_node
*etnaviv_of_first_available_node(void)
return NULL;
}
-static struct etnaviv_drm_private *etnaviv_alloc_private(struct device *dev)
+static int etnaviv_private_init(struct device *dev,
+ struct etnaviv_drm_private *priv)
{
- struct etnaviv_drm_private *priv;
-
- priv = kzalloc(sizeof(*priv), GFP_KERNEL);
- if (!priv)
- return ERR_PTR(-ENOMEM);
-
xa_init_flags(>active_contexts, XA_FLAGS_ALLOC);
mutex_init(>gem_lock);
@@ -58,15 +53,14 @@ static struct etnaviv_drm_private
*etnaviv_alloc_private(struct device *dev)
priv->cmdbuf_suballoc = etnaviv_cmdbuf_suballoc_new(dev);
if (IS_ERR(priv->cmdbuf_suballoc)) {
- kfree(priv);
dev_err(dev, "Failed to create cmdbuf suballocator\n");
- return ERR_PTR(-ENOMEM);
+ return -ENOMEM;
}
- return priv;
+ return 0;
}
-static void etnaviv_free_private(struct etnaviv_drm_private *priv)
+static void etnaviv_private_fini(struct etnaviv_drm_private *priv)
{
if (!priv)
return;
@@ -76,13 +70,11 @@ static void etnaviv_free_private(struct etnaviv_drm_private
*priv)
etnaviv_cmdbuf_suballoc_destroy(priv->cmdbuf_suballoc);
xa_destroy(>active_contexts);
-
- kfree(priv);
}
static void load_gpu(struct drm_device *dev)
{
- struct etnaviv_drm_private *priv = dev->dev_private;
+ struct etnaviv_drm_private *priv = to_etnaviv_priv(dev);
unsigned int i;
for (i = 0; i < ETNA_MAX_PIPES; i++) {
@@ -100,7 +92,7 @@ static void load_gpu(struct drm_device *dev)
static int etnaviv_open(struct drm_device *dev, struct drm_file *file)
{
- struct etnaviv_drm_private *priv = dev->dev_private;
+ struct etnaviv_drm_private *priv = to_etnaviv_priv(dev);
struct etnaviv_file_private *ctx;
int ret, i;
@@ -143,7 +135,7 @@ static int etnaviv_open(struct drm_device *dev, struct
drm_file *file)
static void etnaviv_postclose(struct drm_device *dev, struct drm_file *file)
{
- struct etnaviv_drm_private *priv = dev->dev_private;
+ struct etnaviv_drm_private *priv = to_etnaviv_priv(dev);
struct etnaviv_file_private *ctx = file->driver_priv;
unsigned int i;
@@ -168,7 +160,7 @@ static void etnaviv_postclose(struct drm_device *dev,
struct drm_file *file)
#ifdef CONFIG_DEBUG_FS
static int etnaviv_gem_show(struct drm_device *dev, struct seq_file *m)
{
- struct etnaviv_drm_private *priv = dev->dev_private;
+ struct etnaviv_drm_private *priv = to_etnaviv_priv(dev);
etnaviv_gem_describe_objects(priv, m);
@@ -262,7 +254,7 @@ static int show_each_gpu(struct seq_file *m, void *arg)
{
struct drm_info_node *node = (struct drm_info_node *) m->private;
struct drm_device *dev = node->minor->dev;
- struct etnaviv_drm_private *priv = dev->dev_private;
+ struct etnaviv_drm_private *priv = to_etnaviv_priv(dev);
struct etnaviv_gpu *gpu;
int (*show)(struct etnaviv_gpu *gpu, struct seq_file *m) =
node->info_ent->data;
@@ -305,7 +297,7 @@ static void etnaviv_debugfs_init(struct drm_minor *minor)
static int etnaviv_ioctl_get_param(struct drm_device *dev, void *data,
struct drm_file *file)
{
- struct etnaviv_drm_private *priv = dev->dev_private;
+ struct etnaviv_drm_private *priv = to_etnaviv_priv(dev);
struct drm_etnaviv_param *args = data;
struct etnaviv_gpu *gpu;
@@ -398,7 +390,7 @@ static int etnaviv_ioctl_wait_fence(s
[etnaviv-next v14 2/8] drm/etnaviv: Add constructor and destructor for the etnaviv_drm_private structure
Because there are a lot of data members in the struct etnaviv_drm_private,
which are intended to be shared by all GPU cores. It can be lengthy and
daunting on error handling, the 'gem_lock' of struct etnaviv_drm_private
just be forgeten to destroy on driver leave.
Switch to use the dedicated helpers introduced, etnaviv_bind() and
etnaviv_unbind() gets simplified. Another potential benefit is that
we could put the struct drm_device into struct etnaviv_drm_private
in the future, which made them share the same life time.
Signed-off-by: Sui Jingfeng
---
drivers/gpu/drm/etnaviv/etnaviv_drv.c | 72 +--
1 file changed, 46 insertions(+), 26 deletions(-)
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_drv.c
b/drivers/gpu/drm/etnaviv/etnaviv_drv.c
index 6500f3999c5f..22c78bc944c4 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_drv.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_drv.c
@@ -41,6 +41,45 @@ static struct device_node
*etnaviv_of_first_available_node(void)
return NULL;
}
+static struct etnaviv_drm_private *etnaviv_alloc_private(struct device *dev)
+{
+ struct etnaviv_drm_private *priv;
+
+ priv = kzalloc(sizeof(*priv), GFP_KERNEL);
+ if (!priv)
+ return ERR_PTR(-ENOMEM);
+
+ xa_init_flags(>active_contexts, XA_FLAGS_ALLOC);
+
+ mutex_init(>gem_lock);
+ INIT_LIST_HEAD(>gem_list);
+ priv->num_gpus = 0;
+ priv->shm_gfp_mask = GFP_HIGHUSER | __GFP_RETRY_MAYFAIL | __GFP_NOWARN;
+
+ priv->cmdbuf_suballoc = etnaviv_cmdbuf_suballoc_new(dev);
+ if (IS_ERR(priv->cmdbuf_suballoc)) {
+ kfree(priv);
+ dev_err(dev, "Failed to create cmdbuf suballocator\n");
+ return ERR_PTR(-ENOMEM);
+ }
+
+ return priv;
+}
+
+static void etnaviv_free_private(struct etnaviv_drm_private *priv)
+{
+ if (!priv)
+ return;
+
+ mutex_destroy(>gem_lock);
+
+ etnaviv_cmdbuf_suballoc_destroy(priv->cmdbuf_suballoc);
+
+ xa_destroy(>active_contexts);
+
+ kfree(priv);
+}
+
static void load_gpu(struct drm_device *dev)
{
struct etnaviv_drm_private *priv = dev->dev_private;
@@ -521,35 +560,21 @@ static int etnaviv_bind(struct device *dev)
if (IS_ERR(drm))
return PTR_ERR(drm);
- priv = kzalloc(sizeof(*priv), GFP_KERNEL);
- if (!priv) {
- dev_err(dev, "failed to allocate private data\n");
- ret = -ENOMEM;
+ priv = etnaviv_alloc_private(dev);
+ if (IS_ERR(priv)) {
+ ret = PTR_ERR(priv);
goto out_put;
}
+
drm->dev_private = priv;
dma_set_max_seg_size(dev, SZ_2G);
- xa_init_flags(>active_contexts, XA_FLAGS_ALLOC);
-
- mutex_init(>gem_lock);
- INIT_LIST_HEAD(>gem_list);
- priv->num_gpus = 0;
- priv->shm_gfp_mask = GFP_HIGHUSER | __GFP_RETRY_MAYFAIL | __GFP_NOWARN;
-
- priv->cmdbuf_suballoc = etnaviv_cmdbuf_suballoc_new(drm->dev);
- if (IS_ERR(priv->cmdbuf_suballoc)) {
- dev_err(drm->dev, "Failed to create cmdbuf suballocator\n");
- ret = PTR_ERR(priv->cmdbuf_suballoc);
- goto out_free_priv;
- }
-
dev_set_drvdata(dev, drm);
ret = component_bind_all(dev, drm);
if (ret < 0)
- goto out_destroy_suballoc;
+ goto out_free_priv;
load_gpu(drm);
@@ -561,10 +586,8 @@ static int etnaviv_bind(struct device *dev)
out_unbind:
component_unbind_all(dev, drm);
-out_destroy_suballoc:
- etnaviv_cmdbuf_suballoc_destroy(priv->cmdbuf_suballoc);
out_free_priv:
- kfree(priv);
+ etnaviv_free_private(priv);
out_put:
drm_dev_put(drm);
@@ -580,12 +603,9 @@ static void etnaviv_unbind(struct device *dev)
component_unbind_all(dev, drm);
- etnaviv_cmdbuf_suballoc_destroy(priv->cmdbuf_suballoc);
-
- xa_destroy(>active_contexts);
+ etnaviv_free_private(priv);
drm->dev_private = NULL;
- kfree(priv);
drm_dev_put(drm);
}
--
2.34.1
[etnaviv-next v14 1/8] drm/etnaviv: Add a dedicated helper function to get various clocks
Because the current implementation is DT-based, this only works when the
host platform has the DT support. The problem is that some host platforms
does not provide DT-based clocks drivers, as a result, the driver rage
quit.
PLL hardwares are typically provided by the host platform, which is part
of the entire clock tree. The PLL hardware provide clock pulse to the GPU
core, but it's not belong to the GPU corei itself. PLL registers can be
manipulated directly by the device driver. Hence, it may need dedicated
clock driver.
Add a the etnaviv_gpu_clk_get() function to group similar code blocks,
which make it easier to call this function on the platform where it works.
Signed-off-by: Sui Jingfeng
---
drivers/gpu/drm/etnaviv/etnaviv_gpu.c | 53 ---
1 file changed, 32 insertions(+), 21 deletions(-)
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
index d84d73c197fc..e0c36f564fa6 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
@@ -1605,6 +1605,35 @@ static irqreturn_t irq_handler(int irq, void *data)
return ret;
}
+static int etnaviv_gpu_clk_get(struct etnaviv_gpu *gpu)
+{
+ struct device *dev = gpu->dev;
+
+ gpu->clk_reg = devm_clk_get_optional(dev, "reg");
+ DBG("clk_reg: %p", gpu->clk_reg);
+ if (IS_ERR(gpu->clk_reg))
+ return PTR_ERR(gpu->clk_reg);
+
+ gpu->clk_bus = devm_clk_get_optional(dev, "bus");
+ DBG("clk_bus: %p", gpu->clk_bus);
+ if (IS_ERR(gpu->clk_bus))
+ return PTR_ERR(gpu->clk_bus);
+
+ gpu->clk_core = devm_clk_get(dev, "core");
+ DBG("clk_core: %p", gpu->clk_core);
+ if (IS_ERR(gpu->clk_core))
+ return PTR_ERR(gpu->clk_core);
+ gpu->base_rate_core = clk_get_rate(gpu->clk_core);
+
+ gpu->clk_shader = devm_clk_get_optional(dev, "shader");
+ DBG("clk_shader: %p", gpu->clk_shader);
+ if (IS_ERR(gpu->clk_shader))
+ return PTR_ERR(gpu->clk_shader);
+ gpu->base_rate_shader = clk_get_rate(gpu->clk_shader);
+
+ return 0;
+}
+
static int etnaviv_gpu_clk_enable(struct etnaviv_gpu *gpu)
{
int ret;
@@ -1880,27 +1909,9 @@ static int etnaviv_gpu_platform_probe(struct
platform_device *pdev)
}
/* Get Clocks: */
- gpu->clk_reg = devm_clk_get_optional(>dev, "reg");
- DBG("clk_reg: %p", gpu->clk_reg);
- if (IS_ERR(gpu->clk_reg))
- return PTR_ERR(gpu->clk_reg);
-
- gpu->clk_bus = devm_clk_get_optional(>dev, "bus");
- DBG("clk_bus: %p", gpu->clk_bus);
- if (IS_ERR(gpu->clk_bus))
- return PTR_ERR(gpu->clk_bus);
-
- gpu->clk_core = devm_clk_get(>dev, "core");
- DBG("clk_core: %p", gpu->clk_core);
- if (IS_ERR(gpu->clk_core))
- return PTR_ERR(gpu->clk_core);
- gpu->base_rate_core = clk_get_rate(gpu->clk_core);
-
- gpu->clk_shader = devm_clk_get_optional(>dev, "shader");
- DBG("clk_shader: %p", gpu->clk_shader);
- if (IS_ERR(gpu->clk_shader))
- return PTR_ERR(gpu->clk_shader);
- gpu->base_rate_shader = clk_get_rate(gpu->clk_shader);
+ err = etnaviv_gpu_clk_get(gpu);
+ if (err)
+ return err;
/* TODO: figure out max mapped size */
dev_set_drvdata(dev, gpu);
--
2.34.1
[etnaviv-next v14 0/8] drm/etnaviv: Add driver wrapper for vivante GPUs attached on PCI(e) device
drm/etnaviv use the component framework to bind multiple GPU cores to a virtual master, the virtual master is manually create during driver load time. This works well for various SoCs, yet there are some PCIe card has the vivante GPU cores integrated. The driver lacks the support for PCIe devices currently. Adds PCIe driver wrapper on the top of what drm/etnaviv already has, the component framework is still being used to bind subdevices, even though there is only one GPU core. But the process is going to be reversed, we create virtual platform device for each of the vivante GPU IP core shipped by the PCIe master. The PCIe master is real, bind all the virtual child to the master with component framework. v6: * Fix build issue on system without CONFIG_PCI enabled v7: * Add a separate patch for the platform driver rearrangement (Bjorn) * Switch to runtime check if the GPU is dma coherent or not (Lucas) * Add ETNAVIV_PARAM_GPU_COHERENT to allow userspace to query (Lucas) * Remove etnaviv_gpu.no_clk member (Lucas) * Fix Various typos and coding style fixed (Bjorn) v8: * Fix typos and remove unnecessary header included (Bjorn). * Add a dedicated function to create the virtual master platform device. v9: * Use PCI_VDEVICE() macro (Bjorn) * Add trivial stubs for the PCI driver (Bjorn) * Remove a redundant dev_err() usage (Bjorn) * Clean up etnaviv_pdev_probe() with etnaviv_of_first_available_node() v10: * Add one more cleanup patch * Resolve the conflict with a patch from Rob * Make the dummy PCI stub inlined * Print only if the platform is dma-coherrent V11: * Drop unnecessary changes (Lucas) * Tweak according to other reviews of v10. V12: * Create a virtual platform device for the subcomponent GPU cores * Bind all subordinate GPU cores to the real PCI master via component. V13: * Drop the non-component code path, always use the component framework to bind subcomponent GPU core. Even though there is only one core. * Defer the irq handler register. * Rebase and improve the commit message V14: * Rebase onto etnaviv-next and improve commit message. Tested with JD9230P GPU and LingJiu GP102 GPU. Sui Jingfeng (8): drm/etnaviv: Add a dedicated helper function to get various clocks drm/etnaviv: Add constructor and destructor for the etnaviv_drm_private structure drm/etnaviv: Embed struct drm_device into struct etnaviv_drm_private drm/etnaviv: Fix wrong cache property being used for vmap() drm/etnaviv: Add support for cached coherent caching mode drm/etnaviv: Replace the '>dev' with 'dev' drm/etnaviv: Allow creating subdevices and pass platform specific data drm/etnaviv: Add support for vivante GPU cores attached via PCIe device drivers/gpu/drm/etnaviv/Kconfig | 8 + drivers/gpu/drm/etnaviv/Makefile | 2 + drivers/gpu/drm/etnaviv/etnaviv_drv.c| 159 ++-- drivers/gpu/drm/etnaviv/etnaviv_drv.h| 27 +++ drivers/gpu/drm/etnaviv/etnaviv_gem.c| 22 ++- drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c | 2 +- drivers/gpu/drm/etnaviv/etnaviv_gpu.c| 144 +- drivers/gpu/drm/etnaviv/etnaviv_gpu.h| 4 + drivers/gpu/drm/etnaviv/etnaviv_mmu.c| 4 +- drivers/gpu/drm/etnaviv/etnaviv_pci_drv.c| 187 +++ drivers/gpu/drm/etnaviv/etnaviv_pci_drv.h| 18 ++ include/uapi/drm/etnaviv_drm.h | 1 + 12 files changed, 468 insertions(+), 110 deletions(-) create mode 100644 drivers/gpu/drm/etnaviv/etnaviv_pci_drv.c create mode 100644 drivers/gpu/drm/etnaviv/etnaviv_pci_drv.h base-commit: 52272bfff15ee70c7bd5be9368f175948fb8ecfd -- 2.34.1
Re: [RFT,v2,26/48] drm/panel: simple: Stop tracking prepared/enabled
Hi,
On 2024/5/4 05:33, Douglas Anderson wrote:
As talked about in commit d2aacaf07395 ("drm/panel: Check for already
prepared/enabled in drm_panel"), we want to remove needless code from
panel drivers that was storing and double-checking the
prepared/enabled state. Even if someone was relying on the
double-check before, that double-check is now in the core and not
needed in individual drivers.
I think you are right, as I see drm_panel already has embed
the 'prepared' and 'enabled' as members, remove them from
the derived structure could probably save memory footprint.
Reducing boilerplate is also a side benefit.
Signed-off-by: Douglas Anderson
Acked-by: Sui Jingfeng
--
Best regards,
Sui
Re: [PATCH 3/3] drm/loongson: Refactor lsdc device initialize and the output port
Hi, On 5/16/24 14:26, Markus Elfring wrote: … fullfill the implement under the new framework. fulfil the implementation? Please improve your change descriptions another bit. OK, despite has a few typos. but the quality of the patch itself can be guaranteed. The first version is mainly for preview purpose. I'll fix the problem you mentioned at the next version, thanks for reviewing. I have tested this series before posting on ls3a6000+ls7a2000 PC: $ dmesg | grep loongson: [4.125926] loongson :00:06.1: Found LS7A2000 bridge chipset, revision: 2 [4.133035] loongson :00:06.1: [drm] dc: 400MHz, gmc: 1200MHz, gpu: 480MHz [4.140215] loongson :00:06.1: [drm] Dedicated vram start: 0xe003000, size: 256MiB [4.148538] loongson :00:06.1: [drm] VRAM: 16384 pages ready [4.154506] loongson :00:06.1: [drm] GTT: 32768 pages ready [4.160410] loongson :00:06.1: [drm] lsdc-i2c0(sda pin mask=1, scl pin mask=2) created [4.168638] loongson :00:06.1: [drm] lsdc-i2c1(sda pin mask=4, scl pin mask=8) created [4.176938] loongson :00:06.1: [drm] registered irq: 54 [4.189404] loongson :00:06.1: [drm] Output port-0 bound, type: HDMI-or-VGA-0 [4.196839] loongson :00:06.1: bound lsdc-output-port.0 (ops lsdc_output_port_component_ops) [4.211629] loongson :00:06.1: [drm] Output port-1 bound, type: HDMI-1 [4.218459] loongson :00:06.1: bound lsdc-output-port.1 (ops lsdc_output_port_component_ops) [4.227203] loongson :00:06.1: [drm] Total 2 outputs [4.272617] [drm] Initialized loongson 1.0.0 for :00:06.1 on minor 0 [4.341588] loongson :00:06.1: [drm] fb0: loongsondrmfb frame buffer device [4.348867] loongson :00:06.1: loongson add component: 0 Regards, Markus -- Best regards Sui
Re: drm: have config DRM_WERROR depend on !WERROR
Hi,
On 5/16/24 16:33, Jani Nikula wrote:
If WERROR is already enabled, there's no point in enabling DRM_WERROR or
asking users about it.
Reported-by: Linus Torvalds
Closes:
https://lore.kernel.org/r/CAHk-=whxT8D_0j=bjtrvj-O=veojn6gw8gk4j2v+biduntz...@mail.gmail.com
Fixes: f89632a9e5fa ("drm: Add CONFIG_DRM_WERROR")
Signed-off-by: Jani Nikula
Wow, you successfully get Linus's attention, haha.
---
drivers/gpu/drm/Kconfig | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
index 026444eeb5c6..d0aa277fc3bf 100644
--- a/drivers/gpu/drm/Kconfig
+++ b/drivers/gpu/drm/Kconfig
@@ -450,6 +450,7 @@ config DRM_PRIVACY_SCREEN
config DRM_WERROR
bool "Compile the drm subsystem with warnings as errors"
depends on DRM && EXPERT
+ depends on !WERROR
default n
help
A kernel build should not cause any compiler warnings, and this
--
Best regards
Sui
Re: [PATCH] drm/bridge: adv7511: Exit interrupt handling when necessary
Hi,
On 5/16/24 18:10, Liu Ying wrote:
Commit f3d9683346d6 ("drm/bridge: adv7511: Allow IRQ to share GPIO pins")
fails to consider the case where adv7511->i2c_main->irq is zero, i.e.,
no interrupt requested at all.
Without interrupt, adv7511_wait_for_edid() could return -EIO sometimes,
because it polls adv7511->edid_read flag by calling adv7511_irq_process()
a few times, but adv7511_irq_process() happens to refuse to handle
interrupt by returning -ENODATA. Hence, EDID retrieval fails randomly.
Fix the issue by checking adv7511->i2c_main->irq before exiting interrupt
handling from adv7511_irq_process().
Fixes: f3d9683346d6 ("drm/bridge: adv7511: Allow IRQ to share GPIO pins")
Signed-off-by: Liu Ying
Acked-by: Sui Jingfeng
---
drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
index 6089b0bb9321..2074fa3c1b7b 100644
--- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
+++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
@@ -479,7 +479,8 @@ static int adv7511_irq_process(struct adv7511 *adv7511,
bool process_hpd)
return ret;
/* If there is no IRQ to handle, exit indicating no IRQ data */
- if (!(irq0 & (ADV7511_INT0_HPD | ADV7511_INT0_EDID_READY)) &&
+ if (adv7511->i2c_main->irq &&
+ !(irq0 & (ADV7511_INT0_HPD | ADV7511_INT0_EDID_READY)) &&
!(irq1 & ADV7511_INT1_DDC_ERROR))
return -ENODATA;
--
Best regards
Sui
Re: [PATCH 0/2] drm/bridge: Add 'struct device *' field to the drm_bridge structure
On 5/16/24 18:40, Sui Jingfeng wrote: use 'to_i2c_client(bridge->dev)' to retrieve the pointer to_i2c_client(bridge->kdev). Besides, this also means that we don't need to add the fwnode pointer into struct drm_bridge as member. Relief the conflicts with other reviewers if the work of switching to fwnode is still needed. As for majorities cases (1 to 1), of_node and fwnode can be retrieved with 'struct device *' easily. The aux-bridge.c and aux-hdp-bridge.c can also be converted too easily. of_node, fwnode, swnode and device properties are all belong to the backing device structure itself. It can be more natural to use device_proterty_read_xxx() APIs after init time, Which in turn avoid the need to acquire and duplicate all properties another time in the driver private structure. We could do the programming around the 'struct device *.', remove a batch of boilerplate.
Re: [PATCH] drm/bridge: adv7511: Exit interrupt handling when necessary
Hi,
On 5/16/24 18:10, Liu Ying wrote:
Commit f3d9683346d6 ("drm/bridge: adv7511: Allow IRQ to share GPIO pins")
fails to consider the case where adv7511->i2c_main->irq is zero, i.e.,
no interrupt requested at all.
Without interrupt, adv7511_wait_for_edid() could return -EIO sometimes,
because it polls adv7511->edid_read flag by calling adv7511_irq_process()
a few times, but adv7511_irq_process() happens to refuse to handle
interrupt by returning -ENODATA. Hence, EDID retrieval fails randomly.
Fix the issue by checking adv7511->i2c_main->irq before exiting interrupt
handling from adv7511_irq_process().
Fixes: f3d9683346d6 ("drm/bridge: adv7511: Allow IRQ to share GPIO pins")
Signed-off-by: Liu Ying
---
drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
index 6089b0bb9321..2074fa3c1b7b 100644
--- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
+++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
@@ -479,7 +479,8 @@ static int adv7511_irq_process(struct adv7511 *adv7511,
bool process_hpd)
return ret;
/* If there is no IRQ to handle, exit indicating no IRQ data */
- if (!(irq0 & (ADV7511_INT0_HPD | ADV7511_INT0_EDID_READY)) &&
+ if (adv7511->i2c_main->irq &&
Maybe you could use 'if (process_hpd)' here.
+ !(irq0 & (ADV7511_INT0_HPD | ADV7511_INT0_EDID_READY)) &&
!(irq1 & ADV7511_INT1_DDC_ERROR))
return -ENODATA;
--
Best regards
Sui
Re: [PATCH 0/2] drm/bridge: Add 'struct device *' field to the drm_bridge structure
Hi, On 5/16/24 16:25, Maxime Ripard wrote: On Wed, May 15, 2024 at 11:19:58PM +0800, Sui Jingfeng wrote: Hi, On 5/15/24 22:58, Maxime Ripard wrote: On Wed, May 15, 2024 at 10:53:00PM +0800, Sui Jingfeng wrote: On 5/15/24 22:30, Maxime Ripard wrote: On Wed, May 15, 2024 at 12:53:33AM +0800, Sui Jingfeng wrote: On 2024/5/15 00:22, Maxime Ripard wrote: Hi, On Tue, May 14, 2024 at 11:40:43PM +0800, Sui Jingfeng wrote: Because a lot of implementations has already added it into their drived class, promote it into drm_bridge core may benifits a lot. drm bridge is a driver, it should know the underlying hardware entity. Is there some actual benefits, or is it theoretical at this point? I think, DRM bridge drivers could remove the 'struct device *dev' member from their derived structure. Rely on the drm bridge core when they need the 'struct device *' pointer. Sure, but why do we need to do so? The other thread you had with Jani points out that it turns out that things are more complicated than "every bridge driver has a struct device anyway", it creates inconsistency in the API (bridges would have a struct device, but not other entities), and it looks like there's no use for it anyway. None of these things are deal-breaker by themselves, but if there's only downsides and no upside, it's not clear to me why we should do it at all. It can reduce boilerplate. You're still using a conditional here. It's for safety reason, prevent NULL pointer dereference. drm bridge can be seen as either a software entity or a device driver. It's fine to pass NULL if specific KMS drivers intend to see drm bridge as a pure software entity, and for internal use only. Both use cases are valid. Sorry, I don't follow you. We can't NULL dereference a pointer that doesn't exist. Please state why we should merge this series: what does it fix or improve, aside from the potential gain of making bridges declare one less pointer in their private structure. We could reduce more. Bridge driver instances also don't have to embed 'struct i2c_client *'. We could use 'to_i2c_client(bridge->dev)' to retrieve the pointer, where needed. Maxime -- Best regards Sui
Re: [PATCH 3/3] drm/loongson: Refactor lsdc device initialize and the output port
Hi, On 5/16/24 14:26, Markus Elfring wrote: … fullfill the implement under the new framework. fulfil the implementation? Please improve your change descriptions another bit. I'll accept you suggestions, with pleasure. Thanks. Regards, Markus -- Best regards Sui
Re: [PATCH 1/3] drm/loongson: Add helpers for creating subdevice
Hi, On 5/16/24 04:30, Markus Elfring wrote: In some display subsystems, the functionality of a PCI(e) device may too … of the functionality into child devices can helps to achieve better modularity, eaiser for understand and maintain. Add the loongson_create_platform_device() function to pove the way … Please avoid typos in such a change description. I was too hurry, sorry, my bad. Will be fixed at the next version. Regards, Markus -- Best regards Sui
Re: [07/11] drm/loongson/7a2000: convert to struct drm_edid
Hi, Jani I love your patch, thanks. On 2024/5/14 20:55, Jani Nikula wrote: Prefer the struct drm_edid based functions for reading the EDID and updating the connector. Signed-off-by: Jani Nikula --- Reviewed-by: Sui Jingfeng -- Best regards, Sui
Re: [06/11] drm/loongson/7a1000: convert to struct drm_edid
Hi, Jani I love your patch, thanks. On 2024/5/14 20:55, Jani Nikula wrote: Prefer the struct drm_edid based functions for reading the EDID and updating the connector. Signed-off-by: Jani Nikula --- Reviewed-by: Sui Jingfeng -- Best regards, Sui
Re: drm/bridge: adv7511: Attach next bridge without creating connector
Hi,
On 5/14/24 23:12, Laurent Pinchart wrote:
Hello,
On Tue, May 14, 2024 at 12:26:15AM +0800, Sui Jingfeng wrote:
On 5/13/24 16:02, Liu Ying wrote:
The connector is created by either this ADV7511 bridge driver or
any DRM device driver/previous bridge driver, so this ADV7511
bridge driver should not let the next bridge driver create connector.
If the next bridge is a HDMI connector, the next bridge driver
would fail to attach bridge from display_connector_attach() without
the DRM_BRIDGE_ATTACH_NO_CONNECTOR flag.
In theory we could have another HDMI-to-something bridge connected to
the ADV7511 output, and that bridge could create a connector. However,
before commit 14b3cdbd0e5b the adv7511 driver didn't try to attach to
the next bridge, so it's clear that no platform support in mainline had
such a setup. It should be safe to set DRM_BRIDGE_ATTACH_NO_CONNECTOR
unconditionally here.
But what if there is a drm bridge prior to adv7511 but after the KMS
engine? Even though we are still safe if it doesn't create connector
by obeying modern rule.
Reviewed-by: Laurent Pinchart
Add that flag to drm_bridge_attach() function call in
adv7511_bridge_attach() to fix the issue.
This fixes the issue where the HDMI connector bridge fails to attach
to the previous ADV7535 bridge on i.MX8MP EVK platform:
[2.216442] [drm:drm_bridge_attach] *ERROR* failed to attach bridge
/hdmi-connector to encoder None-37: -22
[2.220675] mmc1: SDHCI controller on 30b5.mmc [30b5.mmc] using ADMA
[2.226262] [drm:drm_bridge_attach] *ERROR* failed to attach bridge
/soc@0/bus@3080/i2c@30a3/hdmi@3d to encoder None-37: -22
[2.245204] [drm:drm_bridge_attach] *ERROR* failed to attach bridge
/soc@0/bus@32c0/dsi@32e6 to encoder None-37: -22
[2.256445] imx-lcdif 32e8.display-controller: error -EINVAL: Failed to
attach bridge for endpoint0
[2.265850] imx-lcdif 32e8.display-controller: error -EINVAL: Cannot
connect bridge
[2.274009] imx-lcdif 32e8.display-controller: probe with driver
imx-lcdif failed with error -22
Fixes: 14b3cdbd0e5b ("drm/bridge: adv7511: make it honour next bridge in DT")
Signed-off-by: Liu Ying
---
drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
index dd21b81bd28f..66ccb61e2a66 100644
--- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
+++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
@@ -953,7 +953,8 @@ static int adv7511_bridge_attach(struct drm_bridge *bridge,
int ret = 0;
if (adv->next_bridge) {
- ret = drm_bridge_attach(bridge->encoder, adv->next_bridge,
bridge, flags);
+ ret = drm_bridge_attach(bridge->encoder, adv->next_bridge,
bridge,
+ flags | DRM_BRIDGE_ATTACH_NO_CONNECTOR);
As a side note, I think, maybe you could do better in the future.
If we know that the KMS display driver side has the HDMI connector
already created for us, we should pass DRM_BRIDGE_ATTACH_NO_CONNECTOR
from the root KMS driver side. Which is to forbidden all potential
drm bridge drivers to create a connector in the middle.
That's the recommended way for new drivers. Using the
drm_bridge_connector helper handles all this for you.
The KMS display driver side could parse the DT to know if there is
a hdmi connector, or merely just hdmi connector device node, or
something else.
No, that would violate the basic principle of not peeking into the DT of
devices you know nothing about. The display engine driver can't walk the
pipeline in DT and expect to understand all the DT nodes on the path,
and what their properties mean.
The (next) bridge at the remote port is not necessary a display bridge.
Or it is a bridge from the perspective of hardware viewpoint, but under
controlled by a more complex foreign driver which generic drm bridge
driver has no authority to attach.
What KMS drivers should do is to use the drm_bridge_connector helper.
Calling drm_bridge_connector_init() will create a connector for a chain
of bridges. The KMS driver should then attach to the first bridge with
DRM_BRIDGE_ATTACH_NO_CONNECTOR, unconditionally.
OK, thanks for teaching us the modern way to use drm_bridge_connector
helper, also thanks Ying for providing the patch.
However, other maintainer and/or reviewer's opinion are of cause
more valuable. I send a A-b because I thought the bug is urgency
and it's probably more important to solve this bug first. And
maybe you can Cc: if you like.
if (ret)
return ret;
}
--
Best regards
Sui
Re: [PATCH 0/2] drm/bridge: Add 'struct device *' field to the drm_bridge structure
Hi, On 5/15/24 22:58, Maxime Ripard wrote: On Wed, May 15, 2024 at 10:53:00PM +0800, Sui Jingfeng wrote: On 5/15/24 22:30, Maxime Ripard wrote: On Wed, May 15, 2024 at 12:53:33AM +0800, Sui Jingfeng wrote: On 2024/5/15 00:22, Maxime Ripard wrote: Hi, On Tue, May 14, 2024 at 11:40:43PM +0800, Sui Jingfeng wrote: Because a lot of implementations has already added it into their drived class, promote it into drm_bridge core may benifits a lot. drm bridge is a driver, it should know the underlying hardware entity. Is there some actual benefits, or is it theoretical at this point? I think, DRM bridge drivers could remove the 'struct device *dev' member from their derived structure. Rely on the drm bridge core when they need the 'struct device *' pointer. Sure, but why do we need to do so? The other thread you had with Jani points out that it turns out that things are more complicated than "every bridge driver has a struct device anyway", it creates inconsistency in the API (bridges would have a struct device, but not other entities), and it looks like there's no use for it anyway. None of these things are deal-breaker by themselves, but if there's only downsides and no upside, it's not clear to me why we should do it at all. It can reduce boilerplate. You're still using a conditional here. It's for safety reason, prevent NULL pointer dereference. drm bridge can be seen as either a software entity or a device driver. It's fine to pass NULL if specific KMS drivers intend to see drm bridge as a pure software entity, and for internal use only. Both use cases are valid. Maxime -- Best regards Sui
Re: [PATCH 0/2] drm/bridge: Add 'struct device *' field to the drm_bridge structure
Hi, On 5/15/24 22:30, Maxime Ripard wrote: On Wed, May 15, 2024 at 12:53:33AM +0800, Sui Jingfeng wrote: Hi, On 2024/5/15 00:22, Maxime Ripard wrote: Hi, On Tue, May 14, 2024 at 11:40:43PM +0800, Sui Jingfeng wrote: Because a lot of implementations has already added it into their drived class, promote it into drm_bridge core may benifits a lot. drm bridge is a driver, it should know the underlying hardware entity. Is there some actual benefits, or is it theoretical at this point? I think, DRM bridge drivers could remove the 'struct device *dev' member from their derived structure. Rely on the drm bridge core when they need the 'struct device *' pointer. Sure, but why do we need to do so? The other thread you had with Jani points out that it turns out that things are more complicated than "every bridge driver has a struct device anyway", it creates inconsistency in the API (bridges would have a struct device, but not other entities), and it looks like there's no use for it anyway. None of these things are deal-breaker by themselves, but if there's only downsides and no upside, it's not clear to me why we should do it at all. It can reduce boilerplate. Maxime -- Best regards Sui
Re: [PATCH 1/2] drm/bridge: Support finding bridge with struct device
Hi,
On 5/15/24 18:28, Jani Nikula wrote:
On Wed, 15 May 2024, Sui Jingfeng wrote:
Hi,
On 5/15/24 17:39, Jani Nikula wrote:
On Tue, 14 May 2024, Sui Jingfeng wrote:
diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
index 584d109330ab..1928d9d0dd3c 100644
--- a/drivers/gpu/drm/drm_bridge.c
+++ b/drivers/gpu/drm/drm_bridge.c
@@ -213,6 +213,23 @@ void drm_bridge_add(struct drm_bridge *bridge)
}
EXPORT_SYMBOL(drm_bridge_add);
+/**
+ * drm_bridge_add_with_dev - add the given bridge to the global bridge list
+ *
+ * @bridge: bridge control structure
+ * @dev: pointer to the kernel device that this bridge is backed.
+ */
+void drm_bridge_add_with_dev(struct drm_bridge *bridge, struct device *dev)
+{
+ if (dev) {
+ bridge->kdev = dev;
+ bridge->of_node = dev->of_node;
+ }
+
+ drm_bridge_add(bridge);
+}
+EXPORT_SYMBOL_GPL(drm_bridge_add_with_dev);
I don't actually have an opinion on whether the dev parameter is useful
or not.
But please don't add a drm_bridge_add_with_dev() and then convert more
than half the drm_bridge_add() users to that. Please just add a struct
device *dev parameter to drm_bridge_add(), and pass NULL if it's not
relevant.
To be honest, previously, I'm just do it exactly same as the way you
told me here. But I'm exhausted and finally give up.
Because this is again need me to modify *all* callers of
drm_bridge_add(), not only those bridges in drm/bridge/, but also
bridge instances in various KMS drivers.
However, their some exceptions just don't fit!
For example, the imx/imx8qxp-pixel-combiner.c just don't fit our
simple model. Our helper function assume that one device backing
one drm_bridge instance (1 to 1). Yet, that driver backing two or
more bridges with one platform device (1 to 2, 1 to 3, ..., ).
Hence, the imx/imx8qxp-pixel-combiner.c just can't use
drm_bridge_add_with_dev().
The aux_hpd_bridge.c is also bad, it store the of_node of struct device
at the .platform_data member of the struct device.
Like I said, "pass NULL if it's not relevant."
OK, good idea.
"_add_with_dev" is a terrible function name.
What if you need to add another parameter later? Add _add_with_foo and
_add_with_dev_and_foo variants?
Yes, you are right, I'll back give another try then.
BR,
Jani.
--
Best regards
Sui
Re: [PATCH 1/2] drm/bridge: Support finding bridge with struct device
Hi,
On 5/15/24 17:39, Jani Nikula wrote:
On Tue, 14 May 2024, Sui Jingfeng wrote:
The pointer of 'struct device' can also be used as a key to search drm
bridge instance from the global bridge list, traditionally, fwnode and
'OF' based APIs requires the system has decent fwnode/OF Graph support.
While the drm_find_bridge_by_dev() function introduced in this series
don't has such a restriction. It only require you has a pointer to the
backing device. Hence, it may suitable for some small and/or limited
display subsystems.
Also add the drm_bridge_add_with_dev() as a helper, which automatically
set the .of_node field of drm_bridge instances if you call it. But it
suitable for simple bridge drivers which one device backing one drm_bridge
instance.
Signed-off-by: Sui Jingfeng
---
drivers/gpu/drm/drm_bridge.c | 39
include/drm/drm_bridge.h | 5 +
2 files changed, 44 insertions(+)
diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
index 584d109330ab..1928d9d0dd3c 100644
--- a/drivers/gpu/drm/drm_bridge.c
+++ b/drivers/gpu/drm/drm_bridge.c
@@ -213,6 +213,23 @@ void drm_bridge_add(struct drm_bridge *bridge)
}
EXPORT_SYMBOL(drm_bridge_add);
+/**
+ * drm_bridge_add_with_dev - add the given bridge to the global bridge list
+ *
+ * @bridge: bridge control structure
+ * @dev: pointer to the kernel device that this bridge is backed.
+ */
+void drm_bridge_add_with_dev(struct drm_bridge *bridge, struct device *dev)
+{
+ if (dev) {
+ bridge->kdev = dev;
+ bridge->of_node = dev->of_node;
+ }
+
+ drm_bridge_add(bridge);
+}
+EXPORT_SYMBOL_GPL(drm_bridge_add_with_dev);
I don't actually have an opinion on whether the dev parameter is useful
or not.
But please don't add a drm_bridge_add_with_dev() and then convert more
than half the drm_bridge_add() users to that. Please just add a struct
device *dev parameter to drm_bridge_add(), and pass NULL if it's not
relevant.
To be honest, previously, I'm just do it exactly same as the way you
told me here. But I'm exhausted and finally give up.
Because this is again need me to modify *all* callers of
drm_bridge_add(), not only those bridges in drm/bridge/, but also
bridge instances in various KMS drivers.
However, their some exceptions just don't fit!
For example, the imx/imx8qxp-pixel-combiner.c just don't fit our
simple model. Our helper function assume that one device backing
one drm_bridge instance (1 to 1). Yet, that driver backing two or
more bridges with one platform device (1 to 2, 1 to 3, ..., ).
Hence, the imx/imx8qxp-pixel-combiner.c just can't use
drm_bridge_add_with_dev().
The aux_hpd_bridge.c is also bad, it store the of_node of struct device
at the .platform_data member of the struct device.
BR,
Jani.
+
static void drm_bridge_remove_void(void *bridge)
{
drm_bridge_remove(bridge);
@@ -1334,6 +1351,27 @@ void drm_bridge_hpd_notify(struct drm_bridge *bridge,
}
EXPORT_SYMBOL_GPL(drm_bridge_hpd_notify);
+struct drm_bridge *drm_find_bridge_by_dev(struct device *kdev)
+{
+ struct drm_bridge *bridge;
+
+ if (!kdev)
+ return NULL;
+
+ mutex_lock(_lock);
+
+ list_for_each_entry(bridge, _list, list) {
+ if (bridge->kdev == kdev) {
+ mutex_unlock(_lock);
+ return bridge;
+ }
+ }
+
+ mutex_unlock(_lock);
+ return NULL;
+}
+EXPORT_SYMBOL_GPL(drm_find_bridge_by_dev);
+
#ifdef CONFIG_OF
/**
* of_drm_find_bridge - find the bridge corresponding to the device node in
@@ -1361,6 +1399,7 @@ struct drm_bridge *of_drm_find_bridge(struct device_node
*np)
return NULL;
}
EXPORT_SYMBOL(of_drm_find_bridge);
+
#endif
MODULE_AUTHOR("Ajay Kumar ");
diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
index 4baca0d9107b..70d8393bbd9c 100644
--- a/include/drm/drm_bridge.h
+++ b/include/drm/drm_bridge.h
@@ -715,6 +715,8 @@ struct drm_bridge {
struct drm_private_obj base;
/** @dev: DRM device this bridge belongs to */
struct drm_device *dev;
+ /** @kdev: pointer to the kernel device backing this bridge */
+ struct device *kdev;
/** @encoder: encoder to which this bridge is connected */
struct drm_encoder *encoder;
/** @chain_node: used to form a bridge chain */
@@ -782,12 +784,15 @@ drm_priv_to_bridge(struct drm_private_obj *priv)
}
void drm_bridge_add(struct drm_bridge *bridge);
+void drm_bridge_add_with_dev(struct drm_bridge *bridge, struct device *dev);
int devm_drm_bridge_add(struct device *dev, struct drm_bridge *bridge);
void drm_bridge_remove(struct drm_bridge *bridge);
int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge,
struct drm_bridge *previous,
enum drm_bridge_attach_flags flags);
Re: [PATCH 0/2] drm/bridge: Add 'struct device *' field to the drm_bridge structure
Hi, On 2024/5/15 00:22, Maxime Ripard wrote: Hi, On Tue, May 14, 2024 at 11:40:43PM +0800, Sui Jingfeng wrote: Because a lot of implementations has already added it into their drived class, promote it into drm_bridge core may benifits a lot. drm bridge is a driver, it should know the underlying hardware entity. Is there some actual benefits, or is it theoretical at this point? I think, DRM bridge drivers could remove the 'struct device *dev' member from their derived structure. Rely on the drm bridge core when they need the 'struct device *' pointer. Maxime -- Best regards, Sui
[PATCH 1/2] drm/bridge: Support finding bridge with struct device
The pointer of 'struct device' can also be used as a key to search drm
bridge instance from the global bridge list, traditionally, fwnode and
'OF' based APIs requires the system has decent fwnode/OF Graph support.
While the drm_find_bridge_by_dev() function introduced in this series
don't has such a restriction. It only require you has a pointer to the
backing device. Hence, it may suitable for some small and/or limited
display subsystems.
Also add the drm_bridge_add_with_dev() as a helper, which automatically
set the .of_node field of drm_bridge instances if you call it. But it
suitable for simple bridge drivers which one device backing one drm_bridge
instance.
Signed-off-by: Sui Jingfeng
---
drivers/gpu/drm/drm_bridge.c | 39
include/drm/drm_bridge.h | 5 +
2 files changed, 44 insertions(+)
diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
index 584d109330ab..1928d9d0dd3c 100644
--- a/drivers/gpu/drm/drm_bridge.c
+++ b/drivers/gpu/drm/drm_bridge.c
@@ -213,6 +213,23 @@ void drm_bridge_add(struct drm_bridge *bridge)
}
EXPORT_SYMBOL(drm_bridge_add);
+/**
+ * drm_bridge_add_with_dev - add the given bridge to the global bridge list
+ *
+ * @bridge: bridge control structure
+ * @dev: pointer to the kernel device that this bridge is backed.
+ */
+void drm_bridge_add_with_dev(struct drm_bridge *bridge, struct device *dev)
+{
+ if (dev) {
+ bridge->kdev = dev;
+ bridge->of_node = dev->of_node;
+ }
+
+ drm_bridge_add(bridge);
+}
+EXPORT_SYMBOL_GPL(drm_bridge_add_with_dev);
+
static void drm_bridge_remove_void(void *bridge)
{
drm_bridge_remove(bridge);
@@ -1334,6 +1351,27 @@ void drm_bridge_hpd_notify(struct drm_bridge *bridge,
}
EXPORT_SYMBOL_GPL(drm_bridge_hpd_notify);
+struct drm_bridge *drm_find_bridge_by_dev(struct device *kdev)
+{
+ struct drm_bridge *bridge;
+
+ if (!kdev)
+ return NULL;
+
+ mutex_lock(_lock);
+
+ list_for_each_entry(bridge, _list, list) {
+ if (bridge->kdev == kdev) {
+ mutex_unlock(_lock);
+ return bridge;
+ }
+ }
+
+ mutex_unlock(_lock);
+ return NULL;
+}
+EXPORT_SYMBOL_GPL(drm_find_bridge_by_dev);
+
#ifdef CONFIG_OF
/**
* of_drm_find_bridge - find the bridge corresponding to the device node in
@@ -1361,6 +1399,7 @@ struct drm_bridge *of_drm_find_bridge(struct device_node
*np)
return NULL;
}
EXPORT_SYMBOL(of_drm_find_bridge);
+
#endif
MODULE_AUTHOR("Ajay Kumar ");
diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
index 4baca0d9107b..70d8393bbd9c 100644
--- a/include/drm/drm_bridge.h
+++ b/include/drm/drm_bridge.h
@@ -715,6 +715,8 @@ struct drm_bridge {
struct drm_private_obj base;
/** @dev: DRM device this bridge belongs to */
struct drm_device *dev;
+ /** @kdev: pointer to the kernel device backing this bridge */
+ struct device *kdev;
/** @encoder: encoder to which this bridge is connected */
struct drm_encoder *encoder;
/** @chain_node: used to form a bridge chain */
@@ -782,12 +784,15 @@ drm_priv_to_bridge(struct drm_private_obj *priv)
}
void drm_bridge_add(struct drm_bridge *bridge);
+void drm_bridge_add_with_dev(struct drm_bridge *bridge, struct device *dev);
int devm_drm_bridge_add(struct device *dev, struct drm_bridge *bridge);
void drm_bridge_remove(struct drm_bridge *bridge);
int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge,
struct drm_bridge *previous,
enum drm_bridge_attach_flags flags);
+struct drm_bridge *drm_find_bridge_by_dev(struct device *kdev);
+
#ifdef CONFIG_OF
struct drm_bridge *of_drm_find_bridge(struct device_node *np);
#else
--
2.43.0
[PATCH 2/2] drm/bridge: Switch to use drm_bridge_add_with_dev()
Normally, the drm_bridge::of_node member won't be used by drm bridge driver
instances, as display driver instances will use the device node fetched
from its backing device directly. The drm_bridge::of_node is mainly for
finding the drm bridge instance associated. Hence, display bridge drivers
don't have to set it manually for the canonical cases.
Let's reduce the boilerplates by using drm_bridge_add_with_dev().
Signed-off-by: Sui Jingfeng
---
drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 3 +--
drivers/gpu/drm/bridge/analogix/analogix-anx6345.c | 4 +---
drivers/gpu/drm/bridge/analogix/analogix-anx78xx.c | 4 +---
drivers/gpu/drm/bridge/analogix/anx7625.c| 3 +--
drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c | 3 +--
drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c | 3 +--
drivers/gpu/drm/bridge/chipone-icn6211.c | 5 ++---
drivers/gpu/drm/bridge/chrontel-ch7033.c | 3 +--
drivers/gpu/drm/bridge/cros-ec-anx7688.c | 4 +---
drivers/gpu/drm/bridge/display-connector.c | 3 +--
drivers/gpu/drm/bridge/fsl-ldb.c | 3 +--
drivers/gpu/drm/bridge/imx/imx8mp-hdmi-pvi.c | 3 +--
drivers/gpu/drm/bridge/imx/imx8qxp-pixel-link.c | 3 +--
drivers/gpu/drm/bridge/imx/imx8qxp-pxl2dpi.c | 3 +--
drivers/gpu/drm/bridge/ite-it6505.c | 3 +--
drivers/gpu/drm/bridge/ite-it66121.c | 3 +--
drivers/gpu/drm/bridge/lontium-lt8912b.c | 3 +--
drivers/gpu/drm/bridge/lontium-lt9211.c | 3 +--
drivers/gpu/drm/bridge/lontium-lt9611.c | 3 +--
drivers/gpu/drm/bridge/lontium-lt9611uxc.c | 3 +--
drivers/gpu/drm/bridge/lvds-codec.c | 3 +--
drivers/gpu/drm/bridge/megachips-stdp-ge-b850v3-fw.c | 3 +--
drivers/gpu/drm/bridge/microchip-lvds.c | 3 +--
drivers/gpu/drm/bridge/nwl-dsi.c | 3 +--
drivers/gpu/drm/bridge/nxp-ptn3460.c | 3 +--
drivers/gpu/drm/bridge/panel.c | 3 +--
drivers/gpu/drm/bridge/parade-ps8622.c | 3 +--
drivers/gpu/drm/bridge/parade-ps8640.c | 1 -
drivers/gpu/drm/bridge/samsung-dsim.c| 3 +--
drivers/gpu/drm/bridge/sii902x.c | 3 +--
drivers/gpu/drm/bridge/sii9234.c | 3 +--
drivers/gpu/drm/bridge/sil-sii8620.c | 3 +--
drivers/gpu/drm/bridge/simple-bridge.c | 3 +--
drivers/gpu/drm/bridge/synopsys/dw-hdmi.c| 3 +--
drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c| 3 +--
drivers/gpu/drm/bridge/tc358762.c| 3 +--
drivers/gpu/drm/bridge/tc358764.c| 3 +--
drivers/gpu/drm/bridge/tc358767.c| 3 +--
drivers/gpu/drm/bridge/tc358768.c| 3 +--
drivers/gpu/drm/bridge/tc358775.c| 3 +--
drivers/gpu/drm/bridge/thc63lvd1024.c| 3 +--
drivers/gpu/drm/bridge/ti-dlpc3433.c | 3 +--
drivers/gpu/drm/bridge/ti-sn65dsi83.c| 3 +--
drivers/gpu/drm/bridge/ti-sn65dsi86.c| 3 +--
drivers/gpu/drm/bridge/ti-tfp410.c | 3 +--
drivers/gpu/drm/bridge/ti-tpd12s015.c| 3 +--
drivers/gpu/drm/i2c/tda998x_drv.c| 5 +
47 files changed, 47 insertions(+), 99 deletions(-)
diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
index 6089b0bb9321..8e4a95584ad8 100644
--- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
+++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
@@ -1318,10 +1318,9 @@ static int adv7511_probe(struct i2c_client *i2c)
if (adv7511->i2c_main->irq)
adv7511->bridge.ops |= DRM_BRIDGE_OP_HPD;
- adv7511->bridge.of_node = dev->of_node;
adv7511->bridge.type = DRM_MODE_CONNECTOR_HDMIA;
- drm_bridge_add(>bridge);
+ drm_bridge_add_with_dev(>bridge, dev);
adv7511_audio_init(dev, adv7511);
diff --git a/drivers/gpu/drm/bridge/analogix/analogix-anx6345.c
b/drivers/gpu/drm/bridge/analogix/analogix-anx6345.c
index b754947e3e00..f4f3298404d2 100644
--- a/drivers/gpu/drm/bridge/analogix/analogix-anx6345.c
+++ b/drivers/gpu/drm/bridge/analogix/analogix-anx6345.c
@@ -697,8 +697,6 @@ static int anx6345_i2c_probe(struct i2c_client *client)
mutex_init(>lock);
- anx6345->bridge.of_node = client->dev.of_node;
-
anx6345->client = client;
i2c_set_clientdata(client, anx6345);
@@ -766,7 +764,7 @@ static int anx6345_i2c_probe(struct i2c_client *client)
anx6345_poweron(anx6345);
if (anx6345_get_chip_id(anx6345)) {
a
[PATCH 0/2] drm/bridge: Add 'struct device *' field to the drm_bridge structure
Because a lot of implementations has already added it into their drived class, promote it into drm_bridge core may benifits a lot. drm bridge is a driver, it should know the underlying hardware entity. Sui Jingfeng (2): drm/bridge: Support finding bridge with struct device drm/bridge: Switch to use drm_bridge_add_with_dev() drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 3 +- .../drm/bridge/analogix/analogix-anx6345.c| 4 +- .../drm/bridge/analogix/analogix-anx78xx.c| 4 +- drivers/gpu/drm/bridge/analogix/anx7625.c | 3 +- .../gpu/drm/bridge/cadence/cdns-dsi-core.c| 3 +- .../drm/bridge/cadence/cdns-mhdp8546-core.c | 3 +- drivers/gpu/drm/bridge/chipone-icn6211.c | 5 +-- drivers/gpu/drm/bridge/chrontel-ch7033.c | 3 +- drivers/gpu/drm/bridge/cros-ec-anx7688.c | 4 +- drivers/gpu/drm/bridge/display-connector.c| 3 +- drivers/gpu/drm/bridge/fsl-ldb.c | 3 +- drivers/gpu/drm/bridge/imx/imx8mp-hdmi-pvi.c | 3 +- .../gpu/drm/bridge/imx/imx8qxp-pixel-link.c | 3 +- drivers/gpu/drm/bridge/imx/imx8qxp-pxl2dpi.c | 3 +- drivers/gpu/drm/bridge/ite-it6505.c | 3 +- drivers/gpu/drm/bridge/ite-it66121.c | 3 +- drivers/gpu/drm/bridge/lontium-lt8912b.c | 3 +- drivers/gpu/drm/bridge/lontium-lt9211.c | 3 +- drivers/gpu/drm/bridge/lontium-lt9611.c | 3 +- drivers/gpu/drm/bridge/lontium-lt9611uxc.c| 3 +- drivers/gpu/drm/bridge/lvds-codec.c | 3 +- .../bridge/megachips-stdp-ge-b850v3-fw.c | 3 +- drivers/gpu/drm/bridge/microchip-lvds.c | 3 +- drivers/gpu/drm/bridge/nwl-dsi.c | 3 +- drivers/gpu/drm/bridge/nxp-ptn3460.c | 3 +- drivers/gpu/drm/bridge/panel.c| 3 +- drivers/gpu/drm/bridge/parade-ps8622.c| 3 +- drivers/gpu/drm/bridge/parade-ps8640.c| 1 - drivers/gpu/drm/bridge/samsung-dsim.c | 3 +- drivers/gpu/drm/bridge/sii902x.c | 3 +- drivers/gpu/drm/bridge/sii9234.c | 3 +- drivers/gpu/drm/bridge/sil-sii8620.c | 3 +- drivers/gpu/drm/bridge/simple-bridge.c| 3 +- drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 3 +- drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c | 3 +- drivers/gpu/drm/bridge/tc358762.c | 3 +- drivers/gpu/drm/bridge/tc358764.c | 3 +- drivers/gpu/drm/bridge/tc358767.c | 3 +- drivers/gpu/drm/bridge/tc358768.c | 3 +- drivers/gpu/drm/bridge/tc358775.c | 3 +- drivers/gpu/drm/bridge/thc63lvd1024.c | 3 +- drivers/gpu/drm/bridge/ti-dlpc3433.c | 3 +- drivers/gpu/drm/bridge/ti-sn65dsi83.c | 3 +- drivers/gpu/drm/bridge/ti-sn65dsi86.c | 3 +- drivers/gpu/drm/bridge/ti-tfp410.c| 3 +- drivers/gpu/drm/bridge/ti-tpd12s015.c | 3 +- drivers/gpu/drm/drm_bridge.c | 39 +++ drivers/gpu/drm/i2c/tda998x_drv.c | 5 +-- include/drm/drm_bridge.h | 5 +++ 49 files changed, 91 insertions(+), 99 deletions(-) -- 2.43.0
Re: drm/bridge: adv7511: Attach next bridge without creating connector
Hi,
On 5/13/24 16:02, Liu Ying wrote:
The connector is created by either this ADV7511 bridge driver or
any DRM device driver/previous bridge driver, so this ADV7511
bridge driver should not let the next bridge driver create connector.
If the next bridge is a HDMI connector, the next bridge driver
would fail to attach bridge from display_connector_attach() without
the DRM_BRIDGE_ATTACH_NO_CONNECTOR flag.
Add that flag to drm_bridge_attach() function call in
adv7511_bridge_attach() to fix the issue.
This fixes the issue where the HDMI connector bridge fails to attach
to the previous ADV7535 bridge on i.MX8MP EVK platform:
[2.216442] [drm:drm_bridge_attach] *ERROR* failed to attach bridge
/hdmi-connector to encoder None-37: -22
[2.220675] mmc1: SDHCI controller on 30b5.mmc [30b5.mmc] using ADMA
[2.226262] [drm:drm_bridge_attach] *ERROR* failed to attach bridge
/soc@0/bus@3080/i2c@30a3/hdmi@3d to encoder None-37: -22
[2.245204] [drm:drm_bridge_attach] *ERROR* failed to attach bridge
/soc@0/bus@32c0/dsi@32e6 to encoder None-37: -22
[2.256445] imx-lcdif 32e8.display-controller: error -EINVAL: Failed to
attach bridge for endpoint0
[2.265850] imx-lcdif 32e8.display-controller: error -EINVAL: Cannot
connect bridge
[2.274009] imx-lcdif 32e8.display-controller: probe with driver
imx-lcdif failed with error -22
Fixes: 14b3cdbd0e5b ("drm/bridge: adv7511: make it honour next bridge in DT")
Signed-off-by: Liu Ying
---
drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
index dd21b81bd28f..66ccb61e2a66 100644
--- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
+++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
@@ -953,7 +953,8 @@ static int adv7511_bridge_attach(struct drm_bridge *bridge,
int ret = 0;
if (adv->next_bridge) {
- ret = drm_bridge_attach(bridge->encoder, adv->next_bridge,
bridge, flags);
+ ret = drm_bridge_attach(bridge->encoder, adv->next_bridge,
bridge,
+ flags | DRM_BRIDGE_ATTACH_NO_CONNECTOR);
As a side note, I think, maybe you could do better in the future.
If we know that the KMS display driver side has the HDMI connector
already created for us, we should pass DRM_BRIDGE_ATTACH_NO_CONNECTOR
from the root KMS driver side. Which is to forbidden all potential
drm bridge drivers to create a connector in the middle.
The KMS display driver side could parse the DT to know if there is
a hdmi connector, or merely just hdmi connector device node, or
something else.
However, other maintainer and/or reviewer's opinion are of cause
more valuable. I send a A-b because I thought the bug is urgency
and it's probably more important to solve this bug first. And
maybe you can Cc: if you like.
Thanks.
if (ret)
return ret;
}
--
Best regards
Sui Jingfeng
Re: drm/bridge: adv7511: Attach next bridge without creating connector
Hi,
On 5/13/24 16:02, Liu Ying wrote:
The connector is created by either this ADV7511 bridge driver or
any DRM device driver/previous bridge driver, so this ADV7511
bridge driver should not let the next bridge driver create connector.
If the next bridge is a HDMI connector, the next bridge driver
would fail to attach bridge from display_connector_attach() without
the DRM_BRIDGE_ATTACH_NO_CONNECTOR flag.
Add that flag to drm_bridge_attach() function call in
adv7511_bridge_attach() to fix the issue.
This fixes the issue where the HDMI connector bridge fails to attach
to the previous ADV7535 bridge on i.MX8MP EVK platform:
[2.216442] [drm:drm_bridge_attach] *ERROR* failed to attach bridge
/hdmi-connector to encoder None-37: -22
[2.220675] mmc1: SDHCI controller on 30b5.mmc [30b5.mmc] using ADMA
[2.226262] [drm:drm_bridge_attach] *ERROR* failed to attach bridge
/soc@0/bus@3080/i2c@30a3/hdmi@3d to encoder None-37: -22
[2.245204] [drm:drm_bridge_attach] *ERROR* failed to attach bridge
/soc@0/bus@32c0/dsi@32e6 to encoder None-37: -22
[2.256445] imx-lcdif 32e8.display-controller: error -EINVAL: Failed to
attach bridge for endpoint0
[2.265850] imx-lcdif 32e8.display-controller: error -EINVAL: Cannot
connect bridge
[2.274009] imx-lcdif 32e8.display-controller: probe with driver
imx-lcdif failed with error -22
Indeed, looks safer finally.
Fixes: 14b3cdbd0e5b ("drm/bridge: adv7511: make it honour next bridge in DT")
Signed-off-by: Liu Ying
Acked-by: Sui Jingfeng
---
drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
index dd21b81bd28f..66ccb61e2a66 100644
--- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
+++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
@@ -953,7 +953,8 @@ static int adv7511_bridge_attach(struct drm_bridge *bridge,
int ret = 0;
if (adv->next_bridge) {
- ret = drm_bridge_attach(bridge->encoder, adv->next_bridge,
bridge, flags);
+ ret = drm_bridge_attach(bridge->encoder, adv->next_bridge,
bridge,
+ flags | DRM_BRIDGE_ATTACH_NO_CONNECTOR);
if (ret)
return ret;
}
--
Best regards
Sui Jingfeng
[PATCH v2 12/12] drm/bridge: analogix: Remove redundant checks on existence of bridge->encoder
The checks on the existence of bridge->encoder in the implementation of
drm_bridge_funcs::attach() is not necessary, as it has already been checked
in the drm_bridge_attach() function call by previous bridge or KMS driver.
The drm_bridge_attach() will quit with a negative error code returned if
it fails for some reasons, hence, it is guaranteed that the .encoder member
of the drm_bridge instance is not NULL when various bridge attach functions
are called.
Remove the redundant checking codes "if (!bridge->encoder) { ... }".
Reviewed-by: Laurent Pinchart
Signed-off-by: Sui Jingfeng
---
drivers/gpu/drm/bridge/analogix/analogix-anx6345.c | 5 -
drivers/gpu/drm/bridge/analogix/analogix-anx78xx.c | 5 -
drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 5 -
drivers/gpu/drm/bridge/analogix/anx7625.c | 10 --
4 files changed, 25 deletions(-)
diff --git a/drivers/gpu/drm/bridge/analogix/analogix-anx6345.c
b/drivers/gpu/drm/bridge/analogix/analogix-anx6345.c
index c9e35731e6a1..cfe43d2ca3be 100644
--- a/drivers/gpu/drm/bridge/analogix/analogix-anx6345.c
+++ b/drivers/gpu/drm/bridge/analogix/analogix-anx6345.c
@@ -528,11 +528,6 @@ static int anx6345_bridge_attach(struct drm_bridge *bridge,
return -EINVAL;
}
- if (!bridge->encoder) {
- DRM_ERROR("Parent encoder object not found");
- return -ENODEV;
- }
-
/* Register aux channel */
anx6345->aux.name = "DP-AUX";
anx6345->aux.dev = >client->dev;
diff --git a/drivers/gpu/drm/bridge/analogix/analogix-anx78xx.c
b/drivers/gpu/drm/bridge/analogix/analogix-anx78xx.c
index 5748a8581af4..58875dde496f 100644
--- a/drivers/gpu/drm/bridge/analogix/analogix-anx78xx.c
+++ b/drivers/gpu/drm/bridge/analogix/analogix-anx78xx.c
@@ -897,11 +897,6 @@ static int anx78xx_bridge_attach(struct drm_bridge *bridge,
return -EINVAL;
}
- if (!bridge->encoder) {
- DRM_ERROR("Parent encoder object not found");
- return -ENODEV;
- }
-
/* Register aux channel */
anx78xx->aux.name = "DP-AUX";
anx78xx->aux.dev = >client->dev;
diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
index df9370e0ff23..7b841232321f 100644
--- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
+++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
@@ -1228,11 +1228,6 @@ static int analogix_dp_bridge_attach(struct drm_bridge
*bridge,
return -EINVAL;
}
- if (!bridge->encoder) {
- DRM_ERROR("Parent encoder object not found");
- return -ENODEV;
- }
-
if (!dp->plat_data->skip_connector) {
connector = >connector;
connector->polled = DRM_CONNECTOR_POLL_HPD;
diff --git a/drivers/gpu/drm/bridge/analogix/anx7625.c
b/drivers/gpu/drm/bridge/analogix/anx7625.c
index 59e9ad349969..3d09efa4199c 100644
--- a/drivers/gpu/drm/bridge/analogix/anx7625.c
+++ b/drivers/gpu/drm/bridge/analogix/anx7625.c
@@ -2193,11 +2193,6 @@ static int anx7625_bridge_attach(struct drm_bridge
*bridge,
if (!(flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR))
return -EINVAL;
- if (!bridge->encoder) {
- DRM_DEV_ERROR(dev, "Parent encoder object not found");
- return -ENODEV;
- }
-
ctx->aux.drm_dev = bridge->dev;
err = drm_dp_aux_register(>aux);
if (err) {
@@ -2435,11 +2430,6 @@ static void anx7625_bridge_atomic_enable(struct
drm_bridge *bridge,
dev_dbg(dev, "drm atomic enable\n");
- if (!bridge->encoder) {
- dev_err(dev, "Parent encoder object not found");
- return;
- }
-
connector = drm_atomic_get_new_connector_for_encoder(state->base.state,
bridge->encoder);
if (!connector)
--
2.43.0
[PATCH v2 11/12] drm/bridge: imx: Remove redundant checks on existence of bridge->encoder
The checks on the existence of bridge->encoder in the implementation of
drm_bridge_funcs::attach() is not necessary, as it has already been checked
in the drm_bridge_attach() function call by previous bridge or KMS driver.
The drm_bridge_attach() will quit with a negative error code returned if
it fails for some reasons, hence, it is guaranteed that the .encoder member
of the drm_bridge instance is not NULL when various i.MX specific bridge
attach functions are called.
Remove the redundant checking codes "if (!bridge->encoder) { ... }".
Reviewed-by: Laurent Pinchart
Signed-off-by: Sui Jingfeng
---
drivers/gpu/drm/bridge/imx/imx-ldb-helper.c | 5 -
drivers/gpu/drm/bridge/imx/imx8qxp-pixel-combiner.c | 5 -
drivers/gpu/drm/bridge/imx/imx8qxp-pixel-link.c | 5 -
drivers/gpu/drm/bridge/imx/imx8qxp-pxl2dpi.c| 5 -
4 files changed, 20 deletions(-)
diff --git a/drivers/gpu/drm/bridge/imx/imx-ldb-helper.c
b/drivers/gpu/drm/bridge/imx/imx-ldb-helper.c
index 6967325cd8ee..9b5bebbe357d 100644
--- a/drivers/gpu/drm/bridge/imx/imx-ldb-helper.c
+++ b/drivers/gpu/drm/bridge/imx/imx-ldb-helper.c
@@ -116,11 +116,6 @@ int ldb_bridge_attach_helper(struct drm_bridge *bridge,
return -EINVAL;
}
- if (!bridge->encoder) {
- DRM_DEV_ERROR(ldb->dev, "missing encoder\n");
- return -ENODEV;
- }
-
return drm_bridge_attach(bridge->encoder,
ldb_ch->next_bridge, bridge,
DRM_BRIDGE_ATTACH_NO_CONNECTOR);
diff --git a/drivers/gpu/drm/bridge/imx/imx8qxp-pixel-combiner.c
b/drivers/gpu/drm/bridge/imx/imx8qxp-pixel-combiner.c
index d0868a6ac6c9..e6dbbdc87ce2 100644
--- a/drivers/gpu/drm/bridge/imx/imx8qxp-pixel-combiner.c
+++ b/drivers/gpu/drm/bridge/imx/imx8qxp-pixel-combiner.c
@@ -119,11 +119,6 @@ static int imx8qxp_pc_bridge_attach(struct drm_bridge
*bridge,
return -EINVAL;
}
- if (!bridge->encoder) {
- DRM_DEV_ERROR(pc->dev, "missing encoder\n");
- return -ENODEV;
- }
-
return drm_bridge_attach(bridge->encoder,
ch->next_bridge, bridge,
DRM_BRIDGE_ATTACH_NO_CONNECTOR);
diff --git a/drivers/gpu/drm/bridge/imx/imx8qxp-pixel-link.c
b/drivers/gpu/drm/bridge/imx/imx8qxp-pixel-link.c
index ed8b7a4e0e11..1d11cc1df43c 100644
--- a/drivers/gpu/drm/bridge/imx/imx8qxp-pixel-link.c
+++ b/drivers/gpu/drm/bridge/imx/imx8qxp-pixel-link.c
@@ -138,11 +138,6 @@ static int imx8qxp_pixel_link_bridge_attach(struct
drm_bridge *bridge,
return -EINVAL;
}
- if (!bridge->encoder) {
- DRM_DEV_ERROR(pl->dev, "missing encoder\n");
- return -ENODEV;
- }
-
return drm_bridge_attach(bridge->encoder,
pl->next_bridge, bridge,
DRM_BRIDGE_ATTACH_NO_CONNECTOR);
diff --git a/drivers/gpu/drm/bridge/imx/imx8qxp-pxl2dpi.c
b/drivers/gpu/drm/bridge/imx/imx8qxp-pxl2dpi.c
index 4a886cb808ca..fb7cf4369bb8 100644
--- a/drivers/gpu/drm/bridge/imx/imx8qxp-pxl2dpi.c
+++ b/drivers/gpu/drm/bridge/imx/imx8qxp-pxl2dpi.c
@@ -58,11 +58,6 @@ static int imx8qxp_pxl2dpi_bridge_attach(struct drm_bridge
*bridge,
return -EINVAL;
}
- if (!bridge->encoder) {
- DRM_DEV_ERROR(p2d->dev, "missing encoder\n");
- return -ENODEV;
- }
-
return drm_bridge_attach(bridge->encoder,
p2d->next_bridge, bridge,
DRM_BRIDGE_ATTACH_NO_CONNECTOR);
--
2.43.0
[PATCH v2 10/12] drm/bridge: lt9611uxc: Remove a redundant check on existence of bridge->encoder
In the lt9611uxc_connector_init() function, the check on the existence
of bridge->encoder is not necessary, as it has already been checked in
the drm_bridge_attach() function. And the check on the drm bridge core
happens before check in the implementation. Hence, it is guaranteed that
the .encoder member of the struct drm_bridge is not NULL when
lt9611uxc_connector_init() function get called.
Remove the redundant checking codes "if (!bridge->encoder) { ... }".
Reviewed-by: Laurent Pinchart
Signed-off-by: Sui Jingfeng
---
drivers/gpu/drm/bridge/lontium-lt9611uxc.c | 5 -
1 file changed, 5 deletions(-)
diff --git a/drivers/gpu/drm/bridge/lontium-lt9611uxc.c
b/drivers/gpu/drm/bridge/lontium-lt9611uxc.c
index ab702471f3ab..f864c033ba81 100644
--- a/drivers/gpu/drm/bridge/lontium-lt9611uxc.c
+++ b/drivers/gpu/drm/bridge/lontium-lt9611uxc.c
@@ -337,11 +337,6 @@ static int lt9611uxc_connector_init(struct drm_bridge
*bridge, struct lt9611uxc
{
int ret;
- if (!bridge->encoder) {
- DRM_ERROR("Parent encoder object not found");
- return -ENODEV;
- }
-
lt9611uxc->connector.polled = DRM_CONNECTOR_POLL_HPD;
drm_connector_helper_add(>connector,
--
2.43.0
[PATCH v2 09/12] drm/bridge: synopsys: dw-mipi-dsi: Remove a redundant check on existence of bridge->encoder
In the dw_mipi_dsi_bridge_attach() function, the check on the existence
of bridge->encoder is not necessary, as it has already been checked in
the drm_bridge_attach() function invocked by previous bridge or KMS driver.
The previous drm_bridge_attach() will quit with a negative error code
returned if it fails for some reasons, hence, it is guaranteed that the
.encoder member of the struct drm_bridge is not NULL when
dw_mipi_dsi_bridge_attach() function gets called.
Remove the redundant checking codes "if (!bridge->encoder) { ... }".
Reviewed-by: Laurent Pinchart
Signed-off-by: Sui Jingfeng
---
drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c | 5 -
1 file changed, 5 deletions(-)
diff --git a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
index 824fb3c65742..c4e9d96933dc 100644
--- a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
+++ b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
@@ -1071,11 +1071,6 @@ static int dw_mipi_dsi_bridge_attach(struct drm_bridge
*bridge,
{
struct dw_mipi_dsi *dsi = bridge_to_dsi(bridge);
- if (!bridge->encoder) {
- DRM_ERROR("Parent encoder object not found\n");
- return -ENODEV;
- }
-
/* Set the encoder type as caller does not know it */
bridge->encoder->encoder_type = DRM_MODE_ENCODER_DSI;
--
2.43.0
[PATCH v2 08/12] drm/bridge: megachips-stdpxxxx-ge-b850v3-fw: Remove a redundant check on existence of bridge->encoder
In the ge_b850v3_lvds_create_connector function, the check on the existence
of bridge->encoder is not necessary, as it has already been checked in the
drm_bridge_attach() function called by upstream bridge or driver. Hence, it
is guaranteed that the .encoder member of the drm_bridge instance is not
NULL when cdns_mhdp_connector_init() function get called.
Remove the redundant checking codes "if (!bridge->encoder) { ... }".
Reviewed-by: Laurent Pinchart
Signed-off-by: Sui Jingfeng
---
drivers/gpu/drm/bridge/megachips-stdp-ge-b850v3-fw.c | 5 -
1 file changed, 5 deletions(-)
diff --git a/drivers/gpu/drm/bridge/megachips-stdp-ge-b850v3-fw.c
b/drivers/gpu/drm/bridge/megachips-stdp-ge-b850v3-fw.c
index 4480523244e4..37f1acf5c0f8 100644
--- a/drivers/gpu/drm/bridge/megachips-stdp-ge-b850v3-fw.c
+++ b/drivers/gpu/drm/bridge/megachips-stdp-ge-b850v3-fw.c
@@ -165,11 +165,6 @@ static int ge_b850v3_lvds_create_connector(struct
drm_bridge *bridge)
struct drm_connector *connector = _b850v3_lvds_ptr->connector;
int ret;
- if (!bridge->encoder) {
- DRM_ERROR("Parent encoder object not found");
- return -ENODEV;
- }
-
connector->polled = DRM_CONNECTOR_POLL_HPD;
drm_connector_helper_add(connector,
--
2.43.0
[PATCH v2 07/12] drm/bridge: cdns-mhdp8546: Remove a redundant check on existence of bridge->encoder
In the cdns_mhdp_connector_init() function, the check on the existence
of bridge->encoder is not necessary, as it has already been checked in
the drm_bridge_attach() function. As the cdns_mhdp_connector_init() is
only called by cdns_mhdp_attach(), it is guaranteed that the .encoder
member of the struct drm_bridge is not NULL when cdns_mhdp_attach() gets
called.
Remove the redundant checking codes "if (!bridge->encoder) { ... }".
Reviewed-by: Laurent Pinchart
Signed-off-by: Sui Jingfeng
---
drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c | 5 -
1 file changed, 5 deletions(-)
diff --git a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
index 8a91ef0ae065..dee640ab1d3a 100644
--- a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
+++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
@@ -1697,11 +1697,6 @@ static int cdns_mhdp_connector_init(struct
cdns_mhdp_device *mhdp)
struct drm_bridge *bridge = >bridge;
int ret;
- if (!bridge->encoder) {
- dev_err(mhdp->dev, "Parent encoder object not found");
- return -ENODEV;
- }
-
conn->polled = DRM_CONNECTOR_POLL_HPD;
ret = drm_connector_init(bridge->dev, conn, _mhdp_conn_funcs,
--
2.43.0
[PATCH v2 06/12] drm/bridge: adv7511: Remove a redundant check on existence of bridge->encoder
In the adv7511_connector_init() function, the check on the existence of
bridge->encoder is not necessary. As it has already been checked in the
drm_bridge_attach() which happens prior to the adv7511_bridge_attach()
get called. Also note that the adv7511_connector_init() is only called by
adv7511_bridge_attach(). Hence, it is guaranteed that the .encoder member
of the drm_bridge instance is not NULL when adv7511_connector_init() get
called.
Remove the redundant checking codes "if (!bridge->encoder) { ... }".
Reviewed-by: Laurent Pinchart
Signed-off-by: Sui Jingfeng
---
drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 5 -
1 file changed, 5 deletions(-)
diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
index dd21b81bd28f..6089b0bb9321 100644
--- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
+++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
@@ -877,11 +877,6 @@ static int adv7511_connector_init(struct adv7511 *adv)
struct drm_bridge *bridge = >bridge;
int ret;
- if (!bridge->encoder) {
- DRM_ERROR("Parent encoder object not found");
- return -ENODEV;
- }
-
if (adv->i2c_main->irq)
adv->connector.polled = DRM_CONNECTOR_POLL_HPD;
else
--
2.43.0
[PATCH v2 05/12] drm/bridge: it6505: Remove a redundant check on existence of bridge->encoder
In it6505_bridge_attach(), the check on the existence of 'bridge->encoder'
is not necessary, as it has already been checked in the drm_bridge_attach()
which happens prior to it6505_bridge_attach() get called. Note that the
it6505_bridge_attach() will only be called by .attach() of the previous
bridge or KMS driver. The previous drm_bridge_attach() will quit with a
negative error code returned if it fails for some reasons. Hence, it is
guaranteed that the .encoder member of the drm_bridge instance is not NULL
when it6505_bridge_attach() function get called.
Remove the redundant checking codes "if (!bridge->encoder) { ... }".
Reviewed-by: Laurent Pinchart
Signed-off-by: Sui Jingfeng
---
drivers/gpu/drm/bridge/ite-it6505.c | 5 -
1 file changed, 5 deletions(-)
diff --git a/drivers/gpu/drm/bridge/ite-it6505.c
b/drivers/gpu/drm/bridge/ite-it6505.c
index 3f68c82888c2..469157341f3a 100644
--- a/drivers/gpu/drm/bridge/ite-it6505.c
+++ b/drivers/gpu/drm/bridge/ite-it6505.c
@@ -2882,11 +2882,6 @@ static int it6505_bridge_attach(struct drm_bridge
*bridge,
return -EINVAL;
}
- if (!bridge->encoder) {
- dev_err(dev, "Parent encoder object not found");
- return -ENODEV;
- }
-
/* Register aux channel */
it6505->aux.drm_dev = bridge->dev;
--
2.43.0
[PATCH v2 04/12] drm/bridge: panel: Remove a redundant check on existence of bridge->encoder
Because the existence of 'bridge->encoder' has already been checked before
the panel_bridge_attach() function get called, and the drm_bridge_attach()
will quit with a negative error code returned if it fails for some reasons.
Hence, it is guaranteed that the .encoder member of the drm_bridge instance
is not NULL when panel_bridge_attach() function get called.
Remove the redundant checking codes "if (!bridge->encoder) { ... }".
Reviewed-by: Laurent Pinchart
Signed-off-by: Sui Jingfeng
---
drivers/gpu/drm/bridge/panel.c | 5 -
1 file changed, 5 deletions(-)
diff --git a/drivers/gpu/drm/bridge/panel.c b/drivers/gpu/drm/bridge/panel.c
index 32506524d9a2..56c40b516a8f 100644
--- a/drivers/gpu/drm/bridge/panel.c
+++ b/drivers/gpu/drm/bridge/panel.c
@@ -67,11 +67,6 @@ static int panel_bridge_attach(struct drm_bridge *bridge,
if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR)
return 0;
- if (!bridge->encoder) {
- DRM_ERROR("Missing encoder\n");
- return -ENODEV;
- }
-
drm_connector_helper_add(connector,
_bridge_connector_helper_funcs);
--
2.43.0
[PATCH v2 03/12] drm/bridge: nxp-ptn3460: Remove a redundant check on existence of bridge->encoder
Because the existence of 'bridge->encoder' has already been checked before
the ptn3460_bridge_attach() function get called, and drm_bridge_attach()
will quit with a negative error code returned if it fails for some reasons.
Hence, it is guaranteed that the .encoder member of the drm_bridge instance
is not NULL when the ptn3460_bridge_attach() function get called.
Remove the redundant checking codes "if (!bridge->encoder) { ... }".
Reviewed-by: Laurent Pinchart
Signed-off-by: Sui Jingfeng
---
drivers/gpu/drm/bridge/nxp-ptn3460.c | 5 -
1 file changed, 5 deletions(-)
diff --git a/drivers/gpu/drm/bridge/nxp-ptn3460.c
b/drivers/gpu/drm/bridge/nxp-ptn3460.c
index ed93fd4c3265..e77aab965fcf 100644
--- a/drivers/gpu/drm/bridge/nxp-ptn3460.c
+++ b/drivers/gpu/drm/bridge/nxp-ptn3460.c
@@ -229,11 +229,6 @@ static int ptn3460_bridge_attach(struct drm_bridge *bridge,
if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR)
return 0;
- if (!bridge->encoder) {
- DRM_ERROR("Parent encoder object not found");
- return -ENODEV;
- }
-
ptn_bridge->connector.polled = DRM_CONNECTOR_POLL_HPD;
ret = drm_connector_init(bridge->dev, _bridge->connector,
_connector_funcs, DRM_MODE_CONNECTOR_LVDS);
--
2.43.0
[PATCH v2 01/12] drm/bridge: simple-bridge: Remove a redundant check on existence of bridge->encoder
Because the existence of 'bridge->encoder' has already been checked before
the simple_bridge_attach() function get called, and drm_bridge_attach()
will quit with a negative error code returned if it fails for some reasons.
Hence, it is guaranteed that the .encoder member of the drm_bridge instance
is not NULL when the simple_bridge_attach() get called.
Remove the redundant checking codes "if (!bridge->encoder) { ... }".
Reviewed-by: Laurent Pinchart
Signed-off-by: Sui Jingfeng
---
drivers/gpu/drm/bridge/simple-bridge.c | 5 -
1 file changed, 5 deletions(-)
diff --git a/drivers/gpu/drm/bridge/simple-bridge.c
b/drivers/gpu/drm/bridge/simple-bridge.c
index 5813a2c4fc5e..2ca89f313cd1 100644
--- a/drivers/gpu/drm/bridge/simple-bridge.c
+++ b/drivers/gpu/drm/bridge/simple-bridge.c
@@ -116,11 +116,6 @@ static int simple_bridge_attach(struct drm_bridge *bridge,
if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR)
return 0;
- if (!bridge->encoder) {
- DRM_ERROR("Missing encoder\n");
- return -ENODEV;
- }
-
drm_connector_helper_add(>connector,
_bridge_con_helper_funcs);
ret = drm_connector_init_with_ddc(bridge->dev, >connector,
--
2.43.0
[PATCH v2 02/12] drm/bridge: tfp410: Remove a redundant check on existence of bridge->encoder
Because the existence of bridge->encoder has already been checked before
the simple_bridge_attach() function get called, And drm_bridge_attach()
will quit with a negative error code returned if it fails for some reasons.
Hence, it is guaranteed that the .encoder member of the drm_bridge instance
is not NULL when the tfp410_attach() function get called.
Remove the redundant checking codes "if (!bridge->encoder) { ... }".
Reviewed-by: Laurent Pinchart
Signed-off-by: Sui Jingfeng
---
drivers/gpu/drm/bridge/ti-tfp410.c | 5 -
1 file changed, 5 deletions(-)
diff --git a/drivers/gpu/drm/bridge/ti-tfp410.c
b/drivers/gpu/drm/bridge/ti-tfp410.c
index c7bef5c23927..b1b1e4d5a24a 100644
--- a/drivers/gpu/drm/bridge/ti-tfp410.c
+++ b/drivers/gpu/drm/bridge/ti-tfp410.c
@@ -133,11 +133,6 @@ static int tfp410_attach(struct drm_bridge *bridge,
if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR)
return 0;
- if (!bridge->encoder) {
- dev_err(dvi->dev, "Missing encoder\n");
- return -ENODEV;
- }
-
if (dvi->next_bridge->ops & DRM_BRIDGE_OP_DETECT)
dvi->connector.polled = DRM_CONNECTOR_POLL_HPD;
else
--
2.43.0
[PATCH v2 00/12] Remove redundant checks on existence of 'bridge->encoder'
The checks on the existence of bridge->encoder in the implementation of drm_bridge_funcs::attach() is not necessary, as it has already been checked in the drm_bridge_attach() function call by previous bridge or KMS driver. The drm_bridge_attach() will quit with a negative error code returned if it fails for some reasons, hence, it is guaranteed that the .encoder member of the drm_bridge instance is not NULL when various bridge attach functions are called. V1 -> V2: * Gather all similar patches to form a series (Laurent) * Fix various spell error (Laurent) * Correct commit message for bridges of i.MX (Ying) Sui Jingfeng (12): drm/bridge: simple-bridge: Remove a redundant check on existence of bridge->encoder drm/bridge: tfp410: Remove a redundant check on existence of bridge->encoder drm/bridge: nxp-ptn3460: Remove a redundant check on existence of bridge->encoder drm/bridge: panel: Remove a redundant check on existence of bridge->encoder drm/bridge: it6505: Remove a redundant check on existence of bridge->encoder drm/bridge: adv7511: Remove a redundant check on existence of bridge->encoder drm/bridge: cdns-mhdp8546: Remove a redundant check on existence of bridge->encoder drm/bridge: megachips-stdp-ge-b850v3-fw: Remove a redundant check on existence of bridge->encoder drm/bridge: synopsys: dw-mipi-dsi: Remove a redundant check on existence of bridge->encoder drm/bridge: lt9611uxc: Remove a redundant check on existence of bridge->encoder drm/bridge: imx: Remove redundant checks on existence of bridge->encoder drm/bridge: analogix: Remove redundant checks on existence of bridge->encoder drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 5 - drivers/gpu/drm/bridge/analogix/analogix-anx6345.c | 5 - drivers/gpu/drm/bridge/analogix/analogix-anx78xx.c | 5 - drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 5 - drivers/gpu/drm/bridge/analogix/anx7625.c | 10 -- drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c| 5 - drivers/gpu/drm/bridge/imx/imx-ldb-helper.c| 5 - drivers/gpu/drm/bridge/imx/imx8qxp-pixel-combiner.c| 5 - drivers/gpu/drm/bridge/imx/imx8qxp-pixel-link.c| 5 - drivers/gpu/drm/bridge/imx/imx8qxp-pxl2dpi.c | 5 - drivers/gpu/drm/bridge/ite-it6505.c| 5 - drivers/gpu/drm/bridge/lontium-lt9611uxc.c | 5 - .../gpu/drm/bridge/megachips-stdp-ge-b850v3-fw.c | 5 - drivers/gpu/drm/bridge/nxp-ptn3460.c | 5 - drivers/gpu/drm/bridge/panel.c | 5 - drivers/gpu/drm/bridge/simple-bridge.c | 5 - drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c | 5 - drivers/gpu/drm/bridge/ti-tfp410.c | 5 - 18 files changed, 95 deletions(-) -- 2.43.0
Re: [PATCH] drm/bridge: imx: Remove redundant checks on existence of bridge->encoder
Hi, On 5/13/24 13:56, Liu Ying wrote: On 5/11/24 23:08, Sui Jingfeng wrote: The check on the existence of bridge->encoder on the implementation layer of drm bridge driver is not necessary, as it has already been done in the drm_bridge_attach() function. It is guaranteed that the .encoder member of the drm_bridge instance is not NULL when various imx_xxx_bridge_attach() function gets called. Nit: ldb_bridge_attach_helper() doesn't follow the fashion of imx_xxx_bridge_attach(), not even the other bridge attach functions touched by this patch do. Right, my bad. But personally, I think rename it as ldb_bridge_attach() should enough to express the meaning. :-) Maybe, reword as "when various i.MX specific bridge attach functions are called." OK, fine. I will correct the commit message at the next version, so happy being reviewed. Thanks. Regards, Liu Ying
Re: [PATCH] drm/bridge: Remove a small useless code snippet
Hi,
On 2024/5/13 05:09, Laurent Pinchart wrote:
Hi Sui,
Thank you for the patch.
On Sat, May 11, 2024 at 08:42:38PM +0800, Sui Jingfeng wrote:
Because the check on the non-existence (encoder == NULL) has already been
done in the implementation of drm_bridge_attach() function, and
drm_bridge_attach() is called earlier. The driver won't get to check point
even if drm_bridge_attach() fails for some reasons, as it will clear the
bridge->encoder to NULL and return a negective error code.
s/negective/negative/
Sorry, my bad.
Therefore, there is no need to check another again. Remove the redundant
codes at the later.
Signed-off-by: Sui Jingfeng
Reviewed-by: Laurent Pinchart
If you end up sending a second version of this patch, please include all
similar patches you have sent at the same time in a patch series,
instead of sending them separately.
I will send a second version to correct spell error and collect and fold them
into a series.
By the way, thanks a lot for you time review my patch and thanks a lot for the
generous.
So happy be reviewed and its my pleasure to have opportunities to talk with
you. Thanks a lot!
---
drivers/gpu/drm/bridge/simple-bridge.c | 5 -
1 file changed, 5 deletions(-)
diff --git a/drivers/gpu/drm/bridge/simple-bridge.c
b/drivers/gpu/drm/bridge/simple-bridge.c
index 28376d0ecd09..3caa918ac2e0 100644
--- a/drivers/gpu/drm/bridge/simple-bridge.c
+++ b/drivers/gpu/drm/bridge/simple-bridge.c
@@ -116,11 +116,6 @@ static int simple_bridge_attach(struct drm_bridge *bridge,
if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR)
return 0;
- if (!bridge->encoder) {
- DRM_ERROR("Missing encoder\n");
- return -ENODEV;
- }
-
drm_connector_helper_add(>connector,
_bridge_con_helper_funcs);
ret = drm_connector_init_with_ddc(bridge->dev, >connector,
--
Best regards,
Sui
[PATCH 3/3] drm/loongson: Refactor lsdc device initialize and the output port
Move drm related device initialization into loongson_drm_master_bind(),
As we need to wait all other submodules ready before we could register
the drm device to userspace. Move output related things into subdriver,
fullfill the implement under the new framework.
Signed-off-by: Sui Jingfeng
---
drivers/gpu/drm/loongson/lsdc_drv.c | 187 +-
drivers/gpu/drm/loongson/lsdc_drv.h | 22 +--
drivers/gpu/drm/loongson/lsdc_output.c| 41
drivers/gpu/drm/loongson/lsdc_output.h| 16 +-
drivers/gpu/drm/loongson/lsdc_output_7a1000.c | 3 +-
drivers/gpu/drm/loongson/lsdc_output_7a2000.c | 15 +-
6 files changed, 152 insertions(+), 132 deletions(-)
diff --git a/drivers/gpu/drm/loongson/lsdc_drv.c
b/drivers/gpu/drm/loongson/lsdc_drv.c
index 45c30e3d178f..796da5c3c2ee 100644
--- a/drivers/gpu/drm/loongson/lsdc_drv.c
+++ b/drivers/gpu/drm/loongson/lsdc_drv.c
@@ -69,31 +69,6 @@ static int lsdc_modeset_init(struct lsdc_device *ldev,
unsigned int i;
int ret;
- for (i = 0; i < num_crtc; i++) {
- dispipe = >dispipe[i];
-
- /* We need an index before crtc is initialized */
- dispipe->index = i;
-
- ret = funcs->create_i2c(ddev, dispipe, i);
- if (ret)
- return ret;
- }
-
- for (i = 0; i < num_crtc; i++) {
- struct i2c_adapter *ddc = NULL;
-
- dispipe = >dispipe[i];
- if (dispipe->li2c)
- ddc = >li2c->adapter;
-
- ret = funcs->output_init(ddev, dispipe, ddc, i);
- if (ret)
- return ret;
-
- ldev->num_output++;
- }
-
for (i = 0; i < num_crtc; i++) {
dispipe = >dispipe[i];
@@ -189,30 +164,17 @@ static int lsdc_get_dedicated_vram(struct lsdc_device
*ldev,
return (size > SZ_1M) ? 0 : -ENODEV;
}
-static struct lsdc_device *
-lsdc_create_device(struct pci_dev *pdev,
- const struct lsdc_desc *descp,
- const struct drm_driver *driver)
+static int lsdc_device_init(struct lsdc_device *ldev,
+ const struct lsdc_desc *descp,
+ const struct drm_driver *driver)
{
- struct lsdc_device *ldev;
- struct drm_device *ddev;
+ struct drm_device *ddev = >base;
int ret;
- ldev = devm_drm_dev_alloc(>dev, driver, struct lsdc_device, base);
- if (IS_ERR(ldev))
- return ldev;
-
- ldev->dc = pdev;
- ldev->descp = descp;
-
- ddev = >base;
-
- loongson_gfxpll_create(ddev, >gfxpll);
-
- ret = lsdc_get_dedicated_vram(ldev, pdev, descp);
+ ret = lsdc_get_dedicated_vram(ldev, ldev->dc, ldev->descp);
if (ret) {
drm_err(ddev, "Init VRAM failed: %d\n", ret);
- return ERR_PTR(ret);
+ return ret;
}
ret = drm_aperture_remove_conflicting_framebuffers(ldev->vram_base,
@@ -220,36 +182,25 @@ lsdc_create_device(struct pci_dev *pdev,
driver);
if (ret) {
drm_err(ddev, "Remove firmware framebuffers failed: %d\n", ret);
- return ERR_PTR(ret);
+ return ret;
}
ret = lsdc_ttm_init(ldev);
if (ret) {
drm_err(ddev, "Memory manager init failed: %d\n", ret);
- return ERR_PTR(ret);
+ return ret;
}
lsdc_gem_init(ddev);
/* Bar 0 of the DC device contains the MMIO register's base address */
- ldev->reg_base = pcim_iomap(pdev, 0, 0);
+ ldev->reg_base = pcim_iomap(ldev->dc, 0, 0);
if (!ldev->reg_base)
- return ERR_PTR(-ENODEV);
+ return -ENODEV;
spin_lock_init(>reglock);
- ret = lsdc_mode_config_init(ddev, descp);
- if (ret)
- return ERR_PTR(ret);
-
- ret = lsdc_modeset_init(ldev, descp->num_of_crtc, descp->funcs,
- loongson_vblank);
- if (ret)
- return ERR_PTR(ret);
-
- drm_mode_config_reset(ddev);
-
- return ldev;
+ return 0;
}
/* For multiple GPU driver instance co-exixt in the system */
@@ -261,20 +212,64 @@ static unsigned int lsdc_vga_set_decode(struct pci_dev
*pdev, bool state)
static int loongson_drm_master_bind(struct device *dev)
{
+ struct lsdc_device *ldev = dev_get_drvdata(dev);
+ const struct lsdc_desc *descp = ldev->descp;
+ struct drm_device *ddev = >base;
int ret;
- ret = component_bind_all(dev, NULL);
+ if (loongson_vblank) {
+ struct pci_dev *pdev = to_pci_dev(dev);
+
+ ret = drm_vblank_init(ddev, descp->n
[PATCH 2/3] drm/loongson: Introduce component framework support
Introduce the component framework to bind childs and siblings, for better
modularity and paper over the deferral probe problems if it need to attach
exterinal module someday. Hardware units come with PCI(e) are actually all
ready to drive, but there has some board specific modules will return
-EPROBE_DEFER. We need all other submodules ready to attach before we can
register the drm device to userspace.
The idea is to devide the exterinal module dependent part and exterinal
module independent part clearly, for example, the display controller and
the builtin GPIO-I2C just belong to exterinal module independent part.
While the output is belong to exterinal module dependent part.
Also for better reflecting the hardware, we intend to abstract the output
ports as child devices. The output ports may consists of encoder phy and
level shift, while the GPU and VPU are standalone siblings. As those units
are relative separate hardware units from display controller itself.
By design, the display controller PCI(e) is selected as the component
master, gpio-i2c go with master. The manually created virtual child device
are functional as agents for the master, it could return the -EPROBE_DEFER
back to the component core. This allows the master don't have to tear down
everything, the majority setups work can be preserved. The potential cyclic
dependency problem can be solved with such framework.
Signed-off-by: Sui Jingfeng
---
drivers/gpu/drm/loongson/Makefile | 1 +
drivers/gpu/drm/loongson/loongson_module.c | 17 ++-
drivers/gpu/drm/loongson/loongson_module.h | 1 +
drivers/gpu/drm/loongson/lsdc_drv.c| 59 +
drivers/gpu/drm/loongson/lsdc_drv.h| 6 +-
drivers/gpu/drm/loongson/lsdc_output.c | 142 +
drivers/gpu/drm/loongson/lsdc_output.h | 22 +++-
7 files changed, 241 insertions(+), 7 deletions(-)
create mode 100644 drivers/gpu/drm/loongson/lsdc_output.c
diff --git a/drivers/gpu/drm/loongson/Makefile
b/drivers/gpu/drm/loongson/Makefile
index 91e72bd900c1..e15cb9bff378 100644
--- a/drivers/gpu/drm/loongson/Makefile
+++ b/drivers/gpu/drm/loongson/Makefile
@@ -9,6 +9,7 @@ loongson-y := \
lsdc_gfxpll.o \
lsdc_i2c.o \
lsdc_irq.o \
+ lsdc_output.o \
lsdc_output_7a1000.o \
lsdc_output_7a2000.o \
lsdc_plane.o \
diff --git a/drivers/gpu/drm/loongson/loongson_module.c
b/drivers/gpu/drm/loongson/loongson_module.c
index d2a51bd395f6..037fa7ffe9c9 100644
--- a/drivers/gpu/drm/loongson/loongson_module.c
+++ b/drivers/gpu/drm/loongson/loongson_module.c
@@ -4,6 +4,7 @@
*/
#include
+#include
#include
@@ -19,15 +20,29 @@ module_param_named(vblank, loongson_vblank, int, 0400);
static int __init loongson_module_init(void)
{
+ int ret;
+
if (!loongson_modeset || video_firmware_drivers_only())
return -ENODEV;
- return pci_register_driver(_pci_driver);
+ ret = platform_driver_register(_output_port_platform_driver);
+ if (ret)
+ return ret;
+
+ ret = pci_register_driver(_pci_driver);
+ if (ret) {
+ platform_driver_unregister(_output_port_platform_driver);
+ return ret;
+ }
+
+ return 0;
}
module_init(loongson_module_init);
static void __exit loongson_module_exit(void)
{
pci_unregister_driver(_pci_driver);
+
+ platform_driver_unregister(_output_port_platform_driver);
}
module_exit(loongson_module_exit);
diff --git a/drivers/gpu/drm/loongson/loongson_module.h
b/drivers/gpu/drm/loongson/loongson_module.h
index 931c17521bf0..8dc71b98f5cc 100644
--- a/drivers/gpu/drm/loongson/loongson_module.h
+++ b/drivers/gpu/drm/loongson/loongson_module.h
@@ -8,5 +8,6 @@
extern int loongson_vblank;
extern struct pci_driver lsdc_pci_driver;
+extern struct platform_driver lsdc_output_port_platform_driver;
#endif
diff --git a/drivers/gpu/drm/loongson/lsdc_drv.c
b/drivers/gpu/drm/loongson/lsdc_drv.c
index adc7344d2f80..45c30e3d178f 100644
--- a/drivers/gpu/drm/loongson/lsdc_drv.c
+++ b/drivers/gpu/drm/loongson/lsdc_drv.c
@@ -3,7 +3,9 @@
* Copyright (C) 2023 Loongson Technology Corporation Limited
*/
+#include
#include
+#include
#include
#include
@@ -257,12 +259,39 @@ static unsigned int lsdc_vga_set_decode(struct pci_dev
*pdev, bool state)
return VGA_RSRC_NORMAL_IO | VGA_RSRC_NORMAL_MEM;
}
+static int loongson_drm_master_bind(struct device *dev)
+{
+ int ret;
+
+ ret = component_bind_all(dev, NULL);
+ if (ret) {
+ dev_err(dev, "master bind all failed: %d\n", ret);
+ return ret;
+ }
+
+ return 0;
+}
+
+static void loongson_drm_master_unbind(struct device *dev)
+{
+ component_unbind_all(dev, NULL);
+
+ return;
+}
+
+const struct component_master_ops loongson_drm_master_ops = {
+ .bind = loongson_drm_master_bind,
+ .unbind = loongson_drm_master_unbind,
+};
+
[PATCH 1/3] drm/loongson: Add helpers for creating subdevice
In some display subsystems, the functionality of a PCI(e) device may too
complex for a single driver to be managed by a monolithic driver. A split
of the functionality into child devices can helps to achieve better
modularity, eaiser for understand and maintain.
Add the loongson_create_platform_device() function to pove the way for the
mentioned goals. Pure software method, no hardware operations involved.
Signed-off-by: Sui Jingfeng
---
drivers/gpu/drm/loongson/loongson_device.c | 42 ++
drivers/gpu/drm/loongson/lsdc_drv.h| 6
2 files changed, 48 insertions(+)
diff --git a/drivers/gpu/drm/loongson/loongson_device.c
b/drivers/gpu/drm/loongson/loongson_device.c
index 9986c8a2a255..b268549d643e 100644
--- a/drivers/gpu/drm/loongson/loongson_device.c
+++ b/drivers/gpu/drm/loongson/loongson_device.c
@@ -4,6 +4,7 @@
*/
#include
+#include
#include "lsdc_drv.h"
@@ -100,3 +101,44 @@ lsdc_device_probe(struct pci_dev *pdev, enum
loongson_chip_id chip_id)
{
return __chip_id_desc_table[chip_id];
}
+
+int loongson_create_platform_device(struct device *parent,
+ const char *name, int id,
+ struct resource *pres,
+ void *data,
+ struct platform_device **ppdev)
+{
+ struct platform_device *pdev;
+ int ret;
+
+ pdev = platform_device_alloc(name, id);
+ if (!pdev)
+ return -ENOMEM;
+
+ pdev->dev.parent = parent;
+
+ if (pres) {
+ ret = platform_device_add_resources(pdev, pres, 1);
+ if (ret) {
+ platform_device_put(pdev);
+ return ret;
+ }
+ }
+
+ if (data) {
+ void *pdata = kmalloc(sizeof(void *), GFP_KERNEL);
+
+ *(void **)pdata = data;
+ pdev->dev.platform_data = pdata;
+ }
+
+ ret = platform_device_add(pdev);
+ if (ret) {
+ platform_device_put(pdev);
+ return ret;
+ }
+
+ *ppdev = pdev;
+
+ return 0;
+}
diff --git a/drivers/gpu/drm/loongson/lsdc_drv.h
b/drivers/gpu/drm/loongson/lsdc_drv.h
index fbf2d760ef27..a2c6b496a69f 100644
--- a/drivers/gpu/drm/loongson/lsdc_drv.h
+++ b/drivers/gpu/drm/loongson/lsdc_drv.h
@@ -47,6 +47,12 @@ enum loongson_chip_id {
const struct lsdc_desc *
lsdc_device_probe(struct pci_dev *pdev, enum loongson_chip_id chip);
+int loongson_create_platform_device(struct device *parent,
+ const char *name, int id,
+ struct resource *pres,
+ void *data,
+ struct platform_device **ppdev);
+
struct lsdc_kms_funcs;
/* DC specific */
--
2.34.1
[PATCH 0/3] drm/loongson: Introduce component framework support
Introduce the component framework to bind childs and siblings, for better modularity and paper over the deferral probe problems if it need to attach exterinal module someday. Hardware units come with PCI(e) are actually all ready to drive, but there has some board specific modules will return -EPROBE_DEFER. We need all other submodules ready to attach before we can register the drm device to userspace. The idea is to devide the exterinal module dependent part and exterinal module independent part clearly, for example, the display controller and the builtin GPIO-I2C just belong to exterinal module independent part. While the output is belong to exterinal module dependent part. Also for better reflecting the hardware, we intend to abstract the output ports as child devices. The output ports may consists of encoder phy and level shift, while the GPU and VPU are standalone siblings. As those units are relative separate hardware units from display controller itself. By design, the display controller PCI(e) is selected as the component master, gpio-i2c go with master. The manually created virtual child device are functional as agents for the master, it could return the -EPROBE_DEFER back to the component core. This allows the master don't have to tear down everything, the majority setups work can be preserved. The potential cyclic dependency problem can be solved with such framework. Sui Jingfeng (3): drm/loongson: Add helpers for creating subdevice drm/loongson: Introduce component framework support drm/loongson: Refactor lsdc device initialize and the output port drivers/gpu/drm/loongson/Makefile | 1 + drivers/gpu/drm/loongson/loongson_device.c| 42 drivers/gpu/drm/loongson/loongson_module.c| 17 +- drivers/gpu/drm/loongson/loongson_module.h| 1 + drivers/gpu/drm/loongson/lsdc_drv.c | 208 +++--- drivers/gpu/drm/loongson/lsdc_drv.h | 34 +-- drivers/gpu/drm/loongson/lsdc_output.c| 183 +++ drivers/gpu/drm/loongson/lsdc_output.h| 38 +++- drivers/gpu/drm/loongson/lsdc_output_7a1000.c | 3 +- drivers/gpu/drm/loongson/lsdc_output_7a2000.c | 15 +- 10 files changed, 422 insertions(+), 120 deletions(-) create mode 100644 drivers/gpu/drm/loongson/lsdc_output.c -- 2.34.1
[PATCH] drm/bridge: analogix: Remove redundant checks on existence of bridge->encoder
The check on the existence of bridge->encoder on the implementation layer
of drm bridge driver is not necessary, as it has already been done in the
drm_bridge_attach() function. It is guaranteed that the .encoder member
of the drm_bridge instance is not NULL when various an_bridge_attach()
function gets called. And .atomic_enable() of struct drm_bridge_funcs
shouldn't be able to called before the various is acctached.
Remove the redundant checking codes "if (!bridge->encoder) { ... }".
Signed-off-by: Sui Jingfeng
---
drivers/gpu/drm/bridge/analogix/analogix-anx6345.c | 5 -
drivers/gpu/drm/bridge/analogix/analogix-anx78xx.c | 5 -
drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 5 -
drivers/gpu/drm/bridge/analogix/anx7625.c | 10 --
4 files changed, 25 deletions(-)
diff --git a/drivers/gpu/drm/bridge/analogix/analogix-anx6345.c
b/drivers/gpu/drm/bridge/analogix/analogix-anx6345.c
index c9e35731e6a1..cfe43d2ca3be 100644
--- a/drivers/gpu/drm/bridge/analogix/analogix-anx6345.c
+++ b/drivers/gpu/drm/bridge/analogix/analogix-anx6345.c
@@ -528,11 +528,6 @@ static int anx6345_bridge_attach(struct drm_bridge *bridge,
return -EINVAL;
}
- if (!bridge->encoder) {
- DRM_ERROR("Parent encoder object not found");
- return -ENODEV;
- }
-
/* Register aux channel */
anx6345->aux.name = "DP-AUX";
anx6345->aux.dev = >client->dev;
diff --git a/drivers/gpu/drm/bridge/analogix/analogix-anx78xx.c
b/drivers/gpu/drm/bridge/analogix/analogix-anx78xx.c
index 5748a8581af4..58875dde496f 100644
--- a/drivers/gpu/drm/bridge/analogix/analogix-anx78xx.c
+++ b/drivers/gpu/drm/bridge/analogix/analogix-anx78xx.c
@@ -897,11 +897,6 @@ static int anx78xx_bridge_attach(struct drm_bridge *bridge,
return -EINVAL;
}
- if (!bridge->encoder) {
- DRM_ERROR("Parent encoder object not found");
- return -ENODEV;
- }
-
/* Register aux channel */
anx78xx->aux.name = "DP-AUX";
anx78xx->aux.dev = >client->dev;
diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
index df9370e0ff23..7b841232321f 100644
--- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
+++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
@@ -1228,11 +1228,6 @@ static int analogix_dp_bridge_attach(struct drm_bridge
*bridge,
return -EINVAL;
}
- if (!bridge->encoder) {
- DRM_ERROR("Parent encoder object not found");
- return -ENODEV;
- }
-
if (!dp->plat_data->skip_connector) {
connector = >connector;
connector->polled = DRM_CONNECTOR_POLL_HPD;
diff --git a/drivers/gpu/drm/bridge/analogix/anx7625.c
b/drivers/gpu/drm/bridge/analogix/anx7625.c
index 59e9ad349969..3d09efa4199c 100644
--- a/drivers/gpu/drm/bridge/analogix/anx7625.c
+++ b/drivers/gpu/drm/bridge/analogix/anx7625.c
@@ -2193,11 +2193,6 @@ static int anx7625_bridge_attach(struct drm_bridge
*bridge,
if (!(flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR))
return -EINVAL;
- if (!bridge->encoder) {
- DRM_DEV_ERROR(dev, "Parent encoder object not found");
- return -ENODEV;
- }
-
ctx->aux.drm_dev = bridge->dev;
err = drm_dp_aux_register(>aux);
if (err) {
@@ -2435,11 +2430,6 @@ static void anx7625_bridge_atomic_enable(struct
drm_bridge *bridge,
dev_dbg(dev, "drm atomic enable\n");
- if (!bridge->encoder) {
- dev_err(dev, "Parent encoder object not found");
- return;
- }
-
connector = drm_atomic_get_new_connector_for_encoder(state->base.state,
bridge->encoder);
if (!connector)
--
2.43.0
[PATCH] drm/bridge: imx: Remove redundant checks on existence of bridge->encoder
The check on the existence of bridge->encoder on the implementation layer
of drm bridge driver is not necessary, as it has already been done in the
drm_bridge_attach() function. It is guaranteed that the .encoder member
of the drm_bridge instance is not NULL when various imx_xxx_bridge_attach()
function gets called.
Remove the redundant checking codes "if (!bridge->encoder) { ... }".
Signed-off-by: Sui Jingfeng
---
drivers/gpu/drm/bridge/imx/imx-ldb-helper.c | 5 -
drivers/gpu/drm/bridge/imx/imx8qxp-pixel-combiner.c | 5 -
drivers/gpu/drm/bridge/imx/imx8qxp-pixel-link.c | 5 -
drivers/gpu/drm/bridge/imx/imx8qxp-pxl2dpi.c| 5 -
4 files changed, 20 deletions(-)
diff --git a/drivers/gpu/drm/bridge/imx/imx-ldb-helper.c
b/drivers/gpu/drm/bridge/imx/imx-ldb-helper.c
index 6967325cd8ee..9b5bebbe357d 100644
--- a/drivers/gpu/drm/bridge/imx/imx-ldb-helper.c
+++ b/drivers/gpu/drm/bridge/imx/imx-ldb-helper.c
@@ -116,11 +116,6 @@ int ldb_bridge_attach_helper(struct drm_bridge *bridge,
return -EINVAL;
}
- if (!bridge->encoder) {
- DRM_DEV_ERROR(ldb->dev, "missing encoder\n");
- return -ENODEV;
- }
-
return drm_bridge_attach(bridge->encoder,
ldb_ch->next_bridge, bridge,
DRM_BRIDGE_ATTACH_NO_CONNECTOR);
diff --git a/drivers/gpu/drm/bridge/imx/imx8qxp-pixel-combiner.c
b/drivers/gpu/drm/bridge/imx/imx8qxp-pixel-combiner.c
index d0868a6ac6c9..e6dbbdc87ce2 100644
--- a/drivers/gpu/drm/bridge/imx/imx8qxp-pixel-combiner.c
+++ b/drivers/gpu/drm/bridge/imx/imx8qxp-pixel-combiner.c
@@ -119,11 +119,6 @@ static int imx8qxp_pc_bridge_attach(struct drm_bridge
*bridge,
return -EINVAL;
}
- if (!bridge->encoder) {
- DRM_DEV_ERROR(pc->dev, "missing encoder\n");
- return -ENODEV;
- }
-
return drm_bridge_attach(bridge->encoder,
ch->next_bridge, bridge,
DRM_BRIDGE_ATTACH_NO_CONNECTOR);
diff --git a/drivers/gpu/drm/bridge/imx/imx8qxp-pixel-link.c
b/drivers/gpu/drm/bridge/imx/imx8qxp-pixel-link.c
index ed8b7a4e0e11..1d11cc1df43c 100644
--- a/drivers/gpu/drm/bridge/imx/imx8qxp-pixel-link.c
+++ b/drivers/gpu/drm/bridge/imx/imx8qxp-pixel-link.c
@@ -138,11 +138,6 @@ static int imx8qxp_pixel_link_bridge_attach(struct
drm_bridge *bridge,
return -EINVAL;
}
- if (!bridge->encoder) {
- DRM_DEV_ERROR(pl->dev, "missing encoder\n");
- return -ENODEV;
- }
-
return drm_bridge_attach(bridge->encoder,
pl->next_bridge, bridge,
DRM_BRIDGE_ATTACH_NO_CONNECTOR);
diff --git a/drivers/gpu/drm/bridge/imx/imx8qxp-pxl2dpi.c
b/drivers/gpu/drm/bridge/imx/imx8qxp-pxl2dpi.c
index 4a886cb808ca..fb7cf4369bb8 100644
--- a/drivers/gpu/drm/bridge/imx/imx8qxp-pxl2dpi.c
+++ b/drivers/gpu/drm/bridge/imx/imx8qxp-pxl2dpi.c
@@ -58,11 +58,6 @@ static int imx8qxp_pxl2dpi_bridge_attach(struct drm_bridge
*bridge,
return -EINVAL;
}
- if (!bridge->encoder) {
- DRM_DEV_ERROR(p2d->dev, "missing encoder\n");
- return -ENODEV;
- }
-
return drm_bridge_attach(bridge->encoder,
p2d->next_bridge, bridge,
DRM_BRIDGE_ATTACH_NO_CONNECTOR);
--
2.43.0
[PATCH] drm/bridge: lt9611uxc: Remove a redundant check on existence of bridge->encoder
In the lt9611uxc_connector_init() function, the check on the existence
of bridge->encoder is not necessary, as it has already been done in the
drm_bridge_attach() function. And the check on the drm bridge core
happens before check in the implementation. Hence, it is guaranteed that
the .encoder member of the struct drm_bridge is not NULL when
lt9611uxc_connector_init() function gets called.
Remove the redundant checking codes "if (!bridge->encoder) { ... }".
Signed-off-by: Sui Jingfeng
---
drivers/gpu/drm/bridge/lontium-lt9611uxc.c | 5 -
1 file changed, 5 deletions(-)
diff --git a/drivers/gpu/drm/bridge/lontium-lt9611uxc.c
b/drivers/gpu/drm/bridge/lontium-lt9611uxc.c
index f4f593ad8f79..f1fccfe6c534 100644
--- a/drivers/gpu/drm/bridge/lontium-lt9611uxc.c
+++ b/drivers/gpu/drm/bridge/lontium-lt9611uxc.c
@@ -339,11 +339,6 @@ static int lt9611uxc_connector_init(struct drm_bridge
*bridge, struct lt9611uxc
{
int ret;
- if (!bridge->encoder) {
- DRM_ERROR("Parent encoder object not found");
- return -ENODEV;
- }
-
lt9611uxc->connector.polled = DRM_CONNECTOR_POLL_HPD;
drm_connector_helper_add(>connector,
--
2.43.0
[PATCH] drm/bridge: synopsys: dw-mipi-dsi: Remove a redundant check on existence of bridge->encoder
In the dw_mipi_dsi_bridge_attach() function, the check on the existence
of bridge->encoder is not necessary, as it has already been done in the
drm_bridge_attach() function. And the check on the drm bridge core
happens before check in the implementation. Hence, it is guaranteed that
the .encoder member of the struct drm_bridge is not NULL when
dw_mipi_dsi_bridge_attach() function gets called.
Remove the redundant checking codes "if (!bridge->encoder) { ... }".
Signed-off-by: Sui Jingfeng
---
drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c | 5 -
1 file changed, 5 deletions(-)
diff --git a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
index 824fb3c65742..c4e9d96933dc 100644
--- a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
+++ b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
@@ -1071,11 +1071,6 @@ static int dw_mipi_dsi_bridge_attach(struct drm_bridge
*bridge,
{
struct dw_mipi_dsi *dsi = bridge_to_dsi(bridge);
- if (!bridge->encoder) {
- DRM_ERROR("Parent encoder object not found\n");
- return -ENODEV;
- }
-
/* Set the encoder type as caller does not know it */
bridge->encoder->encoder_type = DRM_MODE_ENCODER_DSI;
--
2.43.0
[PATCH] drm/bridge: megachips-stdpxxxx-ge-b850v3-fw: Remove a redundant check on existence of bridge->encoder
In the ge_b850v3_lvds_create_connector function, the check on the existence
of bridge->encoder is not necessary, as it has already been done in the
drm_bridge_attach() function. And the check on the drm bridge core
happens before check in the implementation. Hence, it is guaranteed that
the .encoder member of the struct drm_bridge is not NULL when
ge_b850v3_lvds_attach() function gets called.
Remove the redundant checking codes "if (!bridge->encoder) { ... }".
Signed-off-by: Sui Jingfeng
---
drivers/gpu/drm/bridge/megachips-stdp-ge-b850v3-fw.c | 5 -
1 file changed, 5 deletions(-)
diff --git a/drivers/gpu/drm/bridge/megachips-stdp-ge-b850v3-fw.c
b/drivers/gpu/drm/bridge/megachips-stdp-ge-b850v3-fw.c
index 4480523244e4..37f1acf5c0f8 100644
--- a/drivers/gpu/drm/bridge/megachips-stdp-ge-b850v3-fw.c
+++ b/drivers/gpu/drm/bridge/megachips-stdp-ge-b850v3-fw.c
@@ -165,11 +165,6 @@ static int ge_b850v3_lvds_create_connector(struct
drm_bridge *bridge)
struct drm_connector *connector = _b850v3_lvds_ptr->connector;
int ret;
- if (!bridge->encoder) {
- DRM_ERROR("Parent encoder object not found");
- return -ENODEV;
- }
-
connector->polled = DRM_CONNECTOR_POLL_HPD;
drm_connector_helper_add(connector,
--
2.43.0
[PATCH] drm/bridge: cdns-mhdp8546: Remove a redundant check on existence of bridge->encoder
In the cdns_mhdp_connector_init() function, the check on the existence
of bridge->encoder is not necessary, as it has already been done in the
drm_bridge_attach() function. And the check on the drm bridge core
happens before check in the implementation. Hence, it is guaranteed that
the .encoder member of the struct drm_bridge is not NULL when
adv7511_bridge_attach() function gets called.
Remove the redundant checking codes "if (!bridge->encoder) { ... }".
Signed-off-by: Sui Jingfeng
---
drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c | 5 -
1 file changed, 5 deletions(-)
diff --git a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
index e226acc5c15e..16b58a7dcc54 100644
--- a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
+++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
@@ -1697,11 +1697,6 @@ static int cdns_mhdp_connector_init(struct
cdns_mhdp_device *mhdp)
struct drm_bridge *bridge = >bridge;
int ret;
- if (!bridge->encoder) {
- dev_err(mhdp->dev, "Parent encoder object not found");
- return -ENODEV;
- }
-
conn->polled = DRM_CONNECTOR_POLL_HPD;
ret = drm_connector_init(bridge->dev, conn, _mhdp_conn_funcs,
--
2.43.0
[PATCH] drm/bridge: adv7511: Remove a redundant check on existence of bridge->encoder
In the adv7511_connector_init() function, the check on the existence
of bridge->encoder is not necessary, as it has already been done in the
drm_bridge_attach() function. And the check on the drm bridge core
happens before check in the implementation. Hence, it is guaranteed that
the .encoder member of the struct drm_bridge is not NULL when
adv7511_bridge_attach() function is called.
Remove the redundant checking codes "if (!bridge->encoder) { ... }".
Signed-off-by: Sui Jingfeng
---
drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 5 -
1 file changed, 5 deletions(-)
diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
index b5518ff97165..1a0e614e0fd3 100644
--- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
+++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
@@ -871,11 +871,6 @@ static int adv7511_connector_init(struct adv7511 *adv)
struct drm_bridge *bridge = >bridge;
int ret;
- if (!bridge->encoder) {
- DRM_ERROR("Parent encoder object not found");
- return -ENODEV;
- }
-
if (adv->i2c_main->irq)
adv->connector.polled = DRM_CONNECTOR_POLL_HPD;
else
--
2.43.0
[PATCH] drm/bridge: it6505: Remove a redundant check on existence of bridge->encoder
In it6505_bridge_attach(), the check on the existence of bridge->encoder
has already been done in the implementation of drm_bridge_attach(). And
it is done before the bridge->funcs->attach function hook is called. Hence,
it is guaranteed that the .encoder member of the struct drm_bridge is not
NULL when the panel_bridge_attach() is called.
There is no need to check the existence of bridge->encoder another time,
remove the redundant checking codes "if (!bridge->encoder) { ... }".
Signed-off-by: Sui Jingfeng
---
drivers/gpu/drm/bridge/ite-it6505.c | 5 -
1 file changed, 5 deletions(-)
diff --git a/drivers/gpu/drm/bridge/ite-it6505.c
b/drivers/gpu/drm/bridge/ite-it6505.c
index 27334173e911..494030a75dba 100644
--- a/drivers/gpu/drm/bridge/ite-it6505.c
+++ b/drivers/gpu/drm/bridge/ite-it6505.c
@@ -2881,11 +2881,6 @@ static int it6505_bridge_attach(struct drm_bridge
*bridge,
return -EINVAL;
}
- if (!bridge->encoder) {
- dev_err(dev, "Parent encoder object not found");
- return -ENODEV;
- }
-
/* Register aux channel */
it6505->aux.drm_dev = bridge->dev;
--
2.43.0
[PATCH] drm/bridge: panel: Remove a redundant check on existence of bridge->encoder
In panel_bridge_attach(), the check on the existence of bridge->encoder
has already been done in the implementation of drm_bridge_attach(). And
it is done before the bridge->funcs->attach hook is called. Hence, it is
guaranteed that the .encoder member of the struct drm_bridge is not NULL
when the panel_bridge_attach() is called.
There is no need to check the existence of bridge->encoder another time
at the implementation layer, therefore remove the redundant checking codes
"if (!bridge->encoder) { ... }".
Signed-off-by: Sui Jingfeng
---
drivers/gpu/drm/bridge/panel.c | 5 -
1 file changed, 5 deletions(-)
diff --git a/drivers/gpu/drm/bridge/panel.c b/drivers/gpu/drm/bridge/panel.c
index 7f41525f7a6e..762402dca6dd 100644
--- a/drivers/gpu/drm/bridge/panel.c
+++ b/drivers/gpu/drm/bridge/panel.c
@@ -65,11 +65,6 @@ static int panel_bridge_attach(struct drm_bridge *bridge,
if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR)
return 0;
- if (!bridge->encoder) {
- DRM_ERROR("Missing encoder\n");
- return -ENODEV;
- }
-
drm_connector_helper_add(connector,
_bridge_connector_helper_funcs);
--
2.43.0
[PATCH] drm/bridge: nxp-ptn3460: Remove a small useless code snippet
In ptn3460_bridge_attach(), the check on the existence of bridge->encoder
has already been done in the implementation of drm_bridge_attach(). The
driver won't go further if bridge->encoder is NULL and the driver will quit
even if drm_bridge_attach() fails for some reasons. Thereforei, there is
no need to check another time at the later, remove the redundant checking
codes "if (!bridge->encoder) { ... }".
Signed-off-by: Sui Jingfeng
---
drivers/gpu/drm/bridge/nxp-ptn3460.c | 5 -
1 file changed, 5 deletions(-)
diff --git a/drivers/gpu/drm/bridge/nxp-ptn3460.c
b/drivers/gpu/drm/bridge/nxp-ptn3460.c
index ed93fd4c3265..e77aab965fcf 100644
--- a/drivers/gpu/drm/bridge/nxp-ptn3460.c
+++ b/drivers/gpu/drm/bridge/nxp-ptn3460.c
@@ -229,11 +229,6 @@ static int ptn3460_bridge_attach(struct drm_bridge *bridge,
if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR)
return 0;
- if (!bridge->encoder) {
- DRM_ERROR("Parent encoder object not found");
- return -ENODEV;
- }
-
ptn_bridge->connector.polled = DRM_CONNECTOR_POLL_HPD;
ret = drm_connector_init(bridge->dev, _bridge->connector,
_connector_funcs, DRM_MODE_CONNECTOR_LVDS);
--
2.43.0
[PATCH] drm/bridge: tfp410: Remove a small useless code snippet
In the tfp410_attach(), the check on the existence of bridge->encoder has
already been done in the implementation of drm_bridge_attach() function.
The driver won't go further if bridge->encoder is NULL and the driver will
quit even if drm_bridge_attach() fails for some reasons.
Therefore there is no need to check another time at the later, remove the
redundant checking codes "if (!bridge->encoder) { ... }".
Signed-off-by: Sui Jingfeng
---
drivers/gpu/drm/bridge/ti-tfp410.c | 5 -
1 file changed, 5 deletions(-)
diff --git a/drivers/gpu/drm/bridge/ti-tfp410.c
b/drivers/gpu/drm/bridge/ti-tfp410.c
index c7bef5c23927..b1b1e4d5a24a 100644
--- a/drivers/gpu/drm/bridge/ti-tfp410.c
+++ b/drivers/gpu/drm/bridge/ti-tfp410.c
@@ -133,11 +133,6 @@ static int tfp410_attach(struct drm_bridge *bridge,
if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR)
return 0;
- if (!bridge->encoder) {
- dev_err(dvi->dev, "Missing encoder\n");
- return -ENODEV;
- }
-
if (dvi->next_bridge->ops & DRM_BRIDGE_OP_DETECT)
dvi->connector.polled = DRM_CONNECTOR_POLL_HPD;
else
--
2.43.0
[PATCH] drm/bridge: Remove a small useless code snippet
Because the check on the non-existence (encoder == NULL) has already been
done in the implementation of drm_bridge_attach() function, and
drm_bridge_attach() is called earlier. The driver won't get to check point
even if drm_bridge_attach() fails for some reasons, as it will clear the
bridge->encoder to NULL and return a negective error code.
Therefore, there is no need to check another again. Remove the redundant
codes at the later.
Signed-off-by: Sui Jingfeng
---
drivers/gpu/drm/bridge/simple-bridge.c | 5 -
1 file changed, 5 deletions(-)
diff --git a/drivers/gpu/drm/bridge/simple-bridge.c
b/drivers/gpu/drm/bridge/simple-bridge.c
index 28376d0ecd09..3caa918ac2e0 100644
--- a/drivers/gpu/drm/bridge/simple-bridge.c
+++ b/drivers/gpu/drm/bridge/simple-bridge.c
@@ -116,11 +116,6 @@ static int simple_bridge_attach(struct drm_bridge *bridge,
if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR)
return 0;
- if (!bridge->encoder) {
- DRM_ERROR("Missing encoder\n");
- return -ENODEV;
- }
-
drm_connector_helper_add(>connector,
_bridge_con_helper_funcs);
ret = drm_connector_init_with_ddc(bridge->dev, >connector,
--
2.43.0
[PATCH] drm/drm-bridge.c: Drop conditionals around of_node pointers
Having conditional around the of_node pointer of the drm_bridge structure
is not necessary, since drm_bridge structure always has the of_node as its
member.
Let's drop the conditional to get a better looks, please also note that
this is following the already accepted commitments. see commit d8dfccde2709
("drm/bridge: Drop conditionals around of_node pointers") for reference.
Signed-off-by: Sui Jingfeng
---
drivers/gpu/drm/drm_bridge.c | 5 -
1 file changed, 5 deletions(-)
diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
index 30d66bee0ec6..a6dbe1751e88 100644
--- a/drivers/gpu/drm/drm_bridge.c
+++ b/drivers/gpu/drm/drm_bridge.c
@@ -352,13 +352,8 @@ int drm_bridge_attach(struct drm_encoder *encoder, struct
drm_bridge *bridge,
bridge->encoder = NULL;
list_del(>chain_node);
-#ifdef CONFIG_OF
DRM_ERROR("failed to attach bridge %pOF to encoder %s: %d\n",
bridge->of_node, encoder->name, ret);
-#else
- DRM_ERROR("failed to attach bridge to encoder %s: %d\n",
- encoder->name, ret);
-#endif
return ret;
}
--
2.34.1
[PATCH v5 09/10] drm/bridge: ch7033: Switch to use fwnode based APIs
Use the freshly created helper to replace the use of DT-dependent APIs,
also print error log if the fwnode graph is not complete which is benefit
to debug.
Signed-off-by: Sui Jingfeng
---
drivers/gpu/drm/bridge/chrontel-ch7033.c | 14 ++
1 file changed, 10 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/bridge/chrontel-ch7033.c
b/drivers/gpu/drm/bridge/chrontel-ch7033.c
index c83486cf6b15..d856ad0987cc 100644
--- a/drivers/gpu/drm/bridge/chrontel-ch7033.c
+++ b/drivers/gpu/drm/bridge/chrontel-ch7033.c
@@ -531,6 +531,7 @@ static const struct regmap_config ch7033_regmap_config = {
static int ch7033_probe(struct i2c_client *client)
{
struct device *dev = >dev;
+ struct fwnode_handle *fwnode = dev_fwnode(dev);
struct ch7033_priv *priv;
unsigned int val;
int ret;
@@ -541,10 +542,15 @@ static int ch7033_probe(struct i2c_client *client)
dev_set_drvdata(dev, priv);
- ret = drm_of_find_panel_or_bridge(dev->of_node, 1, -1, NULL,
- >next_bridge);
- if (ret)
+ priv->next_bridge = drm_bridge_find_next_bridge_by_fwnode(fwnode, 1);
+ if (IS_ERR(priv->next_bridge)) {
+ ret = PTR_ERR(priv->next_bridge);
+ dev_err(dev, "Error in founding the next bridge: %d\n", ret);
return ret;
+ } else if (!priv->next_bridge) {
+ dev_dbg(dev, "Next bridge not found, deferring probe\n");
+ return -EPROBE_DEFER;
+ }
priv->regmap = devm_regmap_init_i2c(client, _regmap_config);
if (IS_ERR(priv->regmap)) {
@@ -575,7 +581,7 @@ static int ch7033_probe(struct i2c_client *client)
INIT_LIST_HEAD(>bridge.list);
priv->bridge.funcs = _bridge_funcs;
- priv->bridge.of_node = dev->of_node;
+ drm_bridge_set_node(>bridge, fwnode);
drm_bridge_add(>bridge);
dev_info(dev, "Chrontel CH7033 Video Encoder\n");
--
2.34.1
[PATCH v5 10/10] drm/bridge: anx7688: Switch to use drm_bridge_set_node() helper
Switch to use the freshly created drm_bridge_set_node() helper, because
fwnode APIs can be used to abstract DT dependent APIs away. As the fwnode
APIs has wider coverage than DT counterpart, driver don't have to bear
the burden of "OF dependent", let the core take the resposibility. No
functional changes.
Signed-off-by: Sui Jingfeng
---
drivers/gpu/drm/bridge/cros-ec-anx7688.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/bridge/cros-ec-anx7688.c
b/drivers/gpu/drm/bridge/cros-ec-anx7688.c
index c8abd9920fee..30b27b808e02 100644
--- a/drivers/gpu/drm/bridge/cros-ec-anx7688.c
+++ b/drivers/gpu/drm/bridge/cros-ec-anx7688.c
@@ -98,6 +98,7 @@ static const struct drm_bridge_funcs
cros_ec_anx7688_bridge_funcs = {
static int cros_ec_anx7688_bridge_probe(struct i2c_client *client)
{
struct device *dev = >dev;
+ struct fwnode_handle *fwnode = dev_fwnode(dev);
struct cros_ec_anx7688 *anx7688;
u16 vendor, device, fw_version;
u8 buffer[4];
@@ -143,7 +144,7 @@ static int cros_ec_anx7688_bridge_probe(struct i2c_client
*client)
fw_version = (u16)buffer[0] << 8 | buffer[1];
dev_info(dev, "ANX7688 firmware version 0x%04x\n", fw_version);
- anx7688->bridge.of_node = dev->of_node;
+ drm_bridge_set_node(>bridge, fwnode);
/* FW version >= 0.85 supports bandwidth/lane count registers */
if (fw_version >= ANX7688_MINIMUM_FW_VERSION)
--
2.34.1
[PATCH v5 08/10] drm/bridge: sii9234: Use fwnode APIs to abstract DT dependent API away
Switch to use the freshly created drm_bridge_set_node() helper, no
functional changes. The reason behind of this introduction is that
the name 'of_node' itself has a smell of DT dependent, and it is a
internal memeber, when there has helper function, we should use the
revelant helper and avoid directly referencing and/or dereferencing
it.
Signed-off-by: Sui Jingfeng
---
drivers/gpu/drm/bridge/sii9234.c | 8 +---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/bridge/sii9234.c b/drivers/gpu/drm/bridge/sii9234.c
index d8373d918324..19b09634a134 100644
--- a/drivers/gpu/drm/bridge/sii9234.c
+++ b/drivers/gpu/drm/bridge/sii9234.c
@@ -817,10 +817,11 @@ static int sii9234_init_resources(struct sii9234 *ctx,
struct i2c_client *client)
{
struct i2c_adapter *adapter = client->adapter;
+ struct fwnode_handle *fwnode = dev_fwnode(ctx->dev);
int ret;
- if (!ctx->dev->of_node) {
- dev_err(ctx->dev, "not DT device\n");
+ if (!fwnode) {
+ dev_err(ctx->dev, "firmware data is missing\n");
return -ENODEV;
}
@@ -886,6 +887,7 @@ static int sii9234_probe(struct i2c_client *client)
struct i2c_adapter *adapter = client->adapter;
struct sii9234 *ctx;
struct device *dev = >dev;
+ struct fwnode_handle *fwnode = dev_fwnode(dev);
int ret;
ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL);
@@ -922,7 +924,7 @@ static int sii9234_probe(struct i2c_client *client)
i2c_set_clientdata(client, ctx);
ctx->bridge.funcs = _bridge_funcs;
- ctx->bridge.of_node = dev->of_node;
+ drm_bridge_set_node(>bridge, fwnode);
drm_bridge_add(>bridge);
sii9234_cable_in(ctx);
--
2.34.1
[PATCH v5 07/10] drm/bridge: tfp410: Use fwnode APIs to acquire device properties
Make this driver less DT-dependent by calling the newly created helpers,
also switch to use fwnode APIs to acquire additional device properties.
No functional changes for DT-based systems.
Signed-off-by: Sui Jingfeng
---
drivers/gpu/drm/bridge/ti-tfp410.c | 41 +++---
1 file changed, 21 insertions(+), 20 deletions(-)
diff --git a/drivers/gpu/drm/bridge/ti-tfp410.c
b/drivers/gpu/drm/bridge/ti-tfp410.c
index c7bef5c23927..58dc7492844f 100644
--- a/drivers/gpu/drm/bridge/ti-tfp410.c
+++ b/drivers/gpu/drm/bridge/ti-tfp410.c
@@ -266,8 +266,9 @@ static const struct drm_bridge_timings
tfp410_default_timings = {
static int tfp410_parse_timings(struct tfp410 *dvi, bool i2c)
{
+ struct fwnode_handle *fwnode = dev_fwnode(dvi->dev);
struct drm_bridge_timings *timings = >timings;
- struct device_node *ep;
+ struct fwnode_handle *ep;
u32 pclk_sample = 0;
u32 bus_width = 24;
u32 deskew = 0;
@@ -288,14 +289,14 @@ static int tfp410_parse_timings(struct tfp410 *dvi, bool
i2c)
* and EDGE pins. They are specified in DT through endpoint properties
* and vendor-specific properties.
*/
- ep = of_graph_get_endpoint_by_regs(dvi->dev->of_node, 0, 0);
+ ep = fwnode_graph_get_endpoint_by_id(fwnode, 0, 0, 0);
if (!ep)
return -EINVAL;
/* Get the sampling edge from the endpoint. */
- of_property_read_u32(ep, "pclk-sample", _sample);
- of_property_read_u32(ep, "bus-width", _width);
- of_node_put(ep);
+ fwnode_property_read_u32(ep, "pclk-sample", _sample);
+ fwnode_property_read_u32(ep, "bus-width", _width);
+ fwnode_handle_put(ep);
timings->input_bus_flags = DRM_BUS_FLAG_DE_HIGH;
@@ -324,7 +325,7 @@ static int tfp410_parse_timings(struct tfp410 *dvi, bool
i2c)
}
/* Get the setup and hold time from vendor-specific properties. */
- of_property_read_u32(dvi->dev->of_node, "ti,deskew", );
+ fwnode_property_read_u32(fwnode, "ti,deskew", );
if (deskew > 7)
return -EINVAL;
@@ -336,12 +337,12 @@ static int tfp410_parse_timings(struct tfp410 *dvi, bool
i2c)
static int tfp410_init(struct device *dev, bool i2c)
{
- struct device_node *node;
+ struct fwnode_handle *fwnode = dev_fwnode(dev);
struct tfp410 *dvi;
int ret;
- if (!dev->of_node) {
- dev_err(dev, "device-tree data is missing\n");
+ if (!fwnode) {
+ dev_err(dev, "firmware data is missing\n");
return -ENXIO;
}
@@ -352,8 +353,8 @@ static int tfp410_init(struct device *dev, bool i2c)
dvi->dev = dev;
dev_set_drvdata(dev, dvi);
+ drm_bridge_set_node(>bridge, fwnode);
dvi->bridge.funcs = _bridge_funcs;
- dvi->bridge.of_node = dev->of_node;
dvi->bridge.timings = >timings;
dvi->bridge.type = DRM_MODE_CONNECTOR_DVID;
@@ -362,15 +363,15 @@ static int tfp410_init(struct device *dev, bool i2c)
return ret;
/* Get the next bridge, connected to port@1. */
- node = of_graph_get_remote_node(dev->of_node, 1, -1);
- if (!node)
- return -ENODEV;
-
- dvi->next_bridge = of_drm_find_bridge(node);
- of_node_put(node);
-
- if (!dvi->next_bridge)
+ dvi->next_bridge = drm_bridge_find_next_bridge_by_fwnode(fwnode, 1);
+ if (IS_ERR(dvi->next_bridge)) {
+ ret = PTR_ERR(dvi->next_bridge);
+ dev_err(dev, "Error in founding the next bridge: %d\n", ret);
+ return ret;
+ } else if (!dvi->next_bridge) {
+ dev_dbg(dev, "Next bridge not found, deferring probe\n");
return -EPROBE_DEFER;
+ }
/* Get the powerdown GPIO. */
dvi->powerdown = devm_gpiod_get_optional(dev, "powerdown",
@@ -422,10 +423,10 @@ static struct platform_driver tfp410_platform_driver = {
/* There is currently no i2c functionality. */
static int tfp410_i2c_probe(struct i2c_client *client)
{
+ struct fwnode_handle *fwnode = dev_fwnode(>dev);
int reg;
- if (!client->dev.of_node ||
- of_property_read_u32(client->dev.of_node, "reg", )) {
+ if (!fwnode || fwnode_property_read_u32(fwnode, "reg", )) {
dev_err(>dev,
"Can't get i2c reg property from device-tree\n");
return -ENXIO;
--
2.34.1
[PATCH v5 06/10] drm-bridge: it66121: Use fwnode APIs to acquire device properties
Make this driver less DT-dependent by calling the newly created helpers,
also switch to use fwnode APIs to acquire additional device properties.
A side benifit is that boilerplates get reduced, no functional changes
for DT-based systems.
Signed-off-by: Sui Jingfeng
---
drivers/gpu/drm/bridge/ite-it66121.c | 57 +---
1 file changed, 35 insertions(+), 22 deletions(-)
diff --git a/drivers/gpu/drm/bridge/ite-it66121.c
b/drivers/gpu/drm/bridge/ite-it66121.c
index 925e42f46cd8..688dc1830654 100644
--- a/drivers/gpu/drm/bridge/ite-it66121.c
+++ b/drivers/gpu/drm/bridge/ite-it66121.c
@@ -15,7 +15,6 @@
#include
#include
#include
-#include
#include
#include
#include
@@ -1480,7 +1479,7 @@ static int it66121_audio_codec_init(struct it66121_ctx
*ctx, struct device *dev)
dev_dbg(dev, "%s\n", __func__);
- if (!of_property_read_bool(dev->of_node, "#sound-dai-cells")) {
+ if (!fwnode_property_present(dev_fwnode(dev), "#sound-dai-cells")) {
dev_info(dev, "No \"#sound-dai-cells\", no audio\n");
return 0;
}
@@ -1503,13 +1502,36 @@ static const char * const it66121_supplies[] = {
"vcn33", "vcn18", "vrf12"
};
+static int it66121_read_bus_width(struct fwnode_handle *fwnode, u32 *bus_width)
+{
+ struct fwnode_handle *endpoint;
+ u32 val;
+ int ret;
+
+ endpoint = fwnode_graph_get_endpoint_by_id(fwnode, 0, 0, 0);
+ if (!endpoint)
+ return -EINVAL;
+
+ ret = fwnode_property_read_u32(endpoint, "bus-width", );
+ fwnode_handle_put(endpoint);
+ if (ret)
+ return ret;
+
+ if (val != 12 && val != 24)
+ return -EINVAL;
+
+ *bus_width = val;
+
+ return 0;
+}
+
static int it66121_probe(struct i2c_client *client)
{
u32 revision_id, vendor_ids[2] = { 0 }, device_ids[2] = { 0 };
- struct device_node *ep;
int ret;
struct it66121_ctx *ctx;
struct device *dev = >dev;
+ struct fwnode_handle *fwnode = dev_fwnode(dev);
if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
dev_err(dev, "I2C check functionality failed.\n");
@@ -1520,29 +1542,20 @@ static int it66121_probe(struct i2c_client *client)
if (!ctx)
return -ENOMEM;
- ep = of_graph_get_endpoint_by_regs(dev->of_node, 0, 0);
- if (!ep)
- return -EINVAL;
-
ctx->dev = dev;
ctx->client = client;
ctx->info = i2c_get_match_data(client);
- of_property_read_u32(ep, "bus-width", >bus_width);
- of_node_put(ep);
-
- if (ctx->bus_width != 12 && ctx->bus_width != 24)
- return -EINVAL;
-
- ep = of_graph_get_remote_node(dev->of_node, 1, -1);
- if (!ep) {
- dev_err(ctx->dev, "The endpoint is unconnected\n");
- return -EINVAL;
- }
+ ret = it66121_read_bus_width(fwnode, >bus_width);
+ if (ret)
+ return ret;
- ctx->next_bridge = of_drm_find_bridge(ep);
- of_node_put(ep);
- if (!ctx->next_bridge) {
+ ctx->next_bridge = drm_bridge_find_next_bridge_by_fwnode(fwnode, 1);
+ if (IS_ERR(ctx->next_bridge)) {
+ ret = PTR_ERR(ctx->next_bridge);
+ dev_err(dev, "Error in founding the next bridge: %d\n", ret);
+ return ret;
+ } else if (!ctx->next_bridge) {
dev_dbg(ctx->dev, "Next bridge not found, deferring probe\n");
return -EPROBE_DEFER;
}
@@ -1577,8 +1590,8 @@ static int it66121_probe(struct i2c_client *client)
return -ENODEV;
}
+ drm_bridge_set_node(>bridge, fwnode);
ctx->bridge.funcs = _bridge_funcs;
- ctx->bridge.of_node = dev->of_node;
ctx->bridge.type = DRM_MODE_CONNECTOR_HDMIA;
ctx->bridge.ops = DRM_BRIDGE_OP_DETECT | DRM_BRIDGE_OP_EDID;
if (client->irq > 0) {
--
2.34.1
[PATCH v5 05/10] drm/bridge: sii902x: Switch to use fwnode APIs to acquire device properties
Make this driver less DT-dependent by calling the freshly created helpers,
also switch to use fwnode APIs to acquire additional device properties.
One side benifit is that boilerplates get reduced, no functional changes
for DT-based systems.
Signed-off-by: Sui Jingfeng
---
drivers/gpu/drm/bridge/sii902x.c | 45
1 file changed, 16 insertions(+), 29 deletions(-)
diff --git a/drivers/gpu/drm/bridge/sii902x.c b/drivers/gpu/drm/bridge/sii902x.c
index 8f84e98249c7..bc906b71c793 100644
--- a/drivers/gpu/drm/bridge/sii902x.c
+++ b/drivers/gpu/drm/bridge/sii902x.c
@@ -827,20 +827,17 @@ static int sii902x_audio_codec_init(struct sii902x
*sii902x,
.spdif = 0,
.max_i2s_channels = 0,
};
+ struct fwnode_handle *fwnode = dev_fwnode(dev);
u8 lanes[4];
int num_lanes, i;
- if (!of_property_read_bool(dev->of_node, "#sound-dai-cells")) {
+ if (!fwnode_property_present(fwnode, "#sound-dai-cells")) {
dev_dbg(dev, "%s: No \"#sound-dai-cells\", no audio\n",
__func__);
return 0;
}
- num_lanes = of_property_read_variable_u8_array(dev->of_node,
- "sil,i2s-data-lanes",
- lanes, 1,
- ARRAY_SIZE(lanes));
-
+ num_lanes = fwnode_property_count_u8(fwnode, "sil,i2s-data-lanes");
if (num_lanes == -EINVAL) {
dev_dbg(dev,
"%s: No \"sil,i2s-data-lanes\", use default <0>\n",
@@ -852,7 +849,11 @@ static int sii902x_audio_codec_init(struct sii902x
*sii902x,
"%s: Error gettin \"sil,i2s-data-lanes\": %d\n",
__func__, num_lanes);
return num_lanes;
+ } else {
+ fwnode_property_read_u8_array(fwnode, "sil,i2s-data-lanes",
+ lanes, num_lanes);
}
+
codec_data.max_i2s_channels = 2 * num_lanes;
for (i = 0; i < num_lanes; i++)
@@ -1096,8 +1097,8 @@ static int sii902x_init(struct sii902x *sii902x)
if (ret)
goto err_unreg_audio;
+ drm_bridge_set_node(>bridge, dev_fwnode(dev));
sii902x->bridge.funcs = _bridge_funcs;
- sii902x->bridge.of_node = dev->of_node;
sii902x->bridge.timings = _sii902x_timings;
sii902x->bridge.ops = DRM_BRIDGE_OP_DETECT | DRM_BRIDGE_OP_EDID;
@@ -1118,7 +1119,6 @@ static int sii902x_init(struct sii902x *sii902x)
static int sii902x_probe(struct i2c_client *client)
{
struct device *dev = >dev;
- struct device_node *endpoint;
struct sii902x *sii902x;
static const char * const supplies[] = {"iovcc", "cvcc12"};
int ret;
@@ -1147,27 +1147,14 @@ static int sii902x_probe(struct i2c_client *client)
return PTR_ERR(sii902x->reset_gpio);
}
- endpoint = of_graph_get_endpoint_by_regs(dev->of_node, 1, -1);
- if (endpoint) {
- struct device_node *remote =
of_graph_get_remote_port_parent(endpoint);
-
- of_node_put(endpoint);
- if (!remote) {
- dev_err(dev, "Endpoint in port@1 unconnected\n");
- return -ENODEV;
- }
-
- if (!of_device_is_available(remote)) {
- dev_err(dev, "port@1 remote device is disabled\n");
- of_node_put(remote);
- return -ENODEV;
- }
-
- sii902x->next_bridge = of_drm_find_bridge(remote);
- of_node_put(remote);
- if (!sii902x->next_bridge)
- return dev_err_probe(dev, -EPROBE_DEFER,
-"Failed to find remote bridge\n");
+ sii902x->next_bridge =
drm_bridge_find_next_bridge_by_fwnode(dev_fwnode(dev), 1);
+ if (!sii902x->next_bridge) {
+ return dev_err_probe(dev, -EPROBE_DEFER,
+"Failed to find the next bridge\n");
+ } else if (IS_ERR(sii902x->next_bridge)) {
+ ret = PTR_ERR(sii902x->next_bridge);
+ dev_err(dev, "Error on find the next bridge: %d\n", ret);
+ return ret;
}
mutex_init(>mutex);
--
2.34.1
[PATCH v5 04/10] drm/bridge: display-connector: Use fwnode APIs to acquire device properties
Switch to use the fwnode APIs, which is a fundamental step to make this
driver OF-independent possible. No functional changes for DT-based systems.
Signed-off-by: Sui Jingfeng
---
drivers/gpu/drm/bridge/display-connector.c | 25 +++---
1 file changed, 12 insertions(+), 13 deletions(-)
diff --git a/drivers/gpu/drm/bridge/display-connector.c
b/drivers/gpu/drm/bridge/display-connector.c
index ab8e00baf3f1..ac680c6e9bd6 100644
--- a/drivers/gpu/drm/bridge/display-connector.c
+++ b/drivers/gpu/drm/bridge/display-connector.c
@@ -9,7 +9,6 @@
#include
#include
#include
-#include
#include
#include
@@ -204,6 +203,7 @@ static int display_connector_get_supply(struct
platform_device *pdev,
static int display_connector_probe(struct platform_device *pdev)
{
+ struct fwnode_handle *fwnode = dev_fwnode(>dev);
struct display_connector *conn;
unsigned int type;
const char *label = NULL;
@@ -215,15 +215,15 @@ static int display_connector_probe(struct platform_device
*pdev)
platform_set_drvdata(pdev, conn);
- type = (uintptr_t)of_device_get_match_data(>dev);
+ type = (uintptr_t)device_get_match_data(>dev);
/* Get the exact connector type. */
switch (type) {
case DRM_MODE_CONNECTOR_DVII: {
bool analog, digital;
- analog = of_property_read_bool(pdev->dev.of_node, "analog");
- digital = of_property_read_bool(pdev->dev.of_node, "digital");
+ analog = fwnode_property_present(fwnode, "analog");
+ digital = fwnode_property_present(fwnode, "digital");
if (analog && !digital) {
conn->bridge.type = DRM_MODE_CONNECTOR_DVIA;
} else if (!analog && digital) {
@@ -240,8 +240,7 @@ static int display_connector_probe(struct platform_device
*pdev)
case DRM_MODE_CONNECTOR_HDMIA: {
const char *hdmi_type;
- ret = of_property_read_string(pdev->dev.of_node, "type",
- _type);
+ ret = fwnode_property_read_string(fwnode, "type", _type);
if (ret < 0) {
dev_err(>dev, "HDMI connector with no type\n");
return -EINVAL;
@@ -271,7 +270,7 @@ static int display_connector_probe(struct platform_device
*pdev)
conn->bridge.interlace_allowed = true;
/* Get the optional connector label. */
- of_property_read_string(pdev->dev.of_node, "label", );
+ fwnode_property_read_string(fwnode, "label", );
/*
* Get the HPD GPIO for DVI, HDMI and DP connectors. If the GPIO can
provide
@@ -309,12 +308,12 @@ static int display_connector_probe(struct platform_device
*pdev)
if (type == DRM_MODE_CONNECTOR_DVII ||
type == DRM_MODE_CONNECTOR_HDMIA ||
type == DRM_MODE_CONNECTOR_VGA) {
- struct device_node *phandle;
+ struct fwnode_handle *phandle;
- phandle = of_parse_phandle(pdev->dev.of_node, "ddc-i2c-bus", 0);
- if (phandle) {
- conn->bridge.ddc = of_get_i2c_adapter_by_node(phandle);
- of_node_put(phandle);
+ phandle = fwnode_find_reference(fwnode, "ddc-i2c-bus", 0);
+ if (!IS_ERR(phandle)) {
+ conn->bridge.ddc = i2c_get_adapter_by_fwnode(phandle);
+ fwnode_handle_put(phandle);
if (!conn->bridge.ddc)
return -EPROBE_DEFER;
} else {
@@ -358,7 +357,7 @@ static int display_connector_probe(struct platform_device
*pdev)
}
conn->bridge.funcs = _connector_bridge_funcs;
- conn->bridge.of_node = pdev->dev.of_node;
+ drm_bridge_set_node(>bridge, fwnode);
if (conn->bridge.ddc)
conn->bridge.ops |= DRM_BRIDGE_OP_EDID
--
2.34.1
[PATCH v5 03/10] drm/bridge: simple-bridge: Use fwnode APIs to acquire device properties
Make this driver less DT-dependent by calling the newly created helpers,
also switch to use fwnode APIs to acquire additional device properties.
A side benifit is that boilerplates get reduced, no functional changes
for DT-based systems.
Signed-off-by: Sui Jingfeng
---
drivers/gpu/drm/bridge/simple-bridge.c | 23 ++-
1 file changed, 10 insertions(+), 13 deletions(-)
diff --git a/drivers/gpu/drm/bridge/simple-bridge.c
b/drivers/gpu/drm/bridge/simple-bridge.c
index 5813a2c4fc5e..865ddd38f371 100644
--- a/drivers/gpu/drm/bridge/simple-bridge.c
+++ b/drivers/gpu/drm/bridge/simple-bridge.c
@@ -8,8 +8,6 @@
#include
#include
-#include
-#include
#include
#include
@@ -169,33 +167,32 @@ static const struct drm_bridge_funcs
simple_bridge_bridge_funcs = {
static int simple_bridge_probe(struct platform_device *pdev)
{
+ struct fwnode_handle *fwnode = dev_fwnode(>dev);
struct simple_bridge *sbridge;
- struct device_node *remote;
+ int ret;
sbridge = devm_kzalloc(>dev, sizeof(*sbridge), GFP_KERNEL);
if (!sbridge)
return -ENOMEM;
platform_set_drvdata(pdev, sbridge);
- sbridge->info = of_device_get_match_data(>dev);
+ sbridge->info = device_get_match_data(>dev);
/* Get the next bridge in the pipeline. */
- remote = of_graph_get_remote_node(pdev->dev.of_node, 1, -1);
- if (!remote)
- return -EINVAL;
-
- sbridge->next_bridge = of_drm_find_bridge(remote);
- of_node_put(remote);
-
+ sbridge->next_bridge = drm_bridge_find_next_bridge_by_fwnode(fwnode, 1);
if (!sbridge->next_bridge) {
dev_dbg(>dev, "Next bridge not found, deferring probe\n");
return -EPROBE_DEFER;
+ } else if (IS_ERR(sbridge->next_bridge)) {
+ ret = PTR_ERR(sbridge->next_bridge);
+ dev_err(>dev, "Error on finding the next bridge: %d\n",
ret);
+ return ret;
}
/* Get the regulator and GPIO resources. */
sbridge->vdd = devm_regulator_get_optional(>dev, "vdd");
if (IS_ERR(sbridge->vdd)) {
- int ret = PTR_ERR(sbridge->vdd);
+ ret = PTR_ERR(sbridge->vdd);
if (ret == -EPROBE_DEFER)
return -EPROBE_DEFER;
sbridge->vdd = NULL;
@@ -209,8 +206,8 @@ static int simple_bridge_probe(struct platform_device *pdev)
"Unable to retrieve enable GPIO\n");
/* Register the bridge. */
+ drm_bridge_set_node(>bridge, fwnode);
sbridge->bridge.funcs = _bridge_bridge_funcs;
- sbridge->bridge.of_node = pdev->dev.of_node;
sbridge->bridge.timings = sbridge->info->timings;
drm_bridge_add(>bridge);
--
2.34.1
[PATCH v5 01/10] drm/bridge: Allow using fwnode APIs to get the next bridge
The various display bridge drivers rely on 'OF' infrastructures to
works very well, yet there are some platforms and/or devices lack of
'OF' support. Such as virtual display drivers, USB display apapters
and ACPI based systems etc.
Add fwnode based helpers to fill the niche, this allows part of display
bridge drivers to work across systems. As the fwnode APIs has wider
coverage than DT counterpart, and fwnode graphs are compatible with
OF graphs. So the newly created helpers can be used on all systems
in theory, assumed that there has valid OF/fwnode graphs established.
Note, the involved drm bridge instance should also has the fwnode
assigned, so that the user of it could find it via the fwnode handle.
Signed-off-by: Sui Jingfeng
---
drivers/gpu/drm/drm_bridge.c | 74
include/drm/drm_bridge.h | 8
2 files changed, 82 insertions(+)
diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
index 28abe9aa99ca..fc1a314140e7 100644
--- a/drivers/gpu/drm/drm_bridge.c
+++ b/drivers/gpu/drm/drm_bridge.c
@@ -1368,6 +1368,80 @@ struct drm_bridge *of_drm_find_bridge(struct device_node
*np)
EXPORT_SYMBOL(of_drm_find_bridge);
#endif
+/**
+ * drm_bridge_find_by_fwnode - Find the bridge corresponding to the fwnode
+ *
+ * @fwnode: fwnode for which to find the matching drm_bridge
+ *
+ * This function looks up a drm_bridge in the global bridge list, based on
+ * its associated fwnode. Drivers who want to use this function should has
+ * fwnode handle assigned to the fwnode member of the struct drm_bridge
+ * instance.
+ *
+ * Returns:
+ * * A reference to the requested drm_bridge object on success, or
+ * * %NULL otherwise (object does not exist or the driver of requested
+ *bridge not probed yet).
+ */
+struct drm_bridge *drm_bridge_find_by_fwnode(struct fwnode_handle *fwnode)
+{
+ struct drm_bridge *ret = NULL;
+ struct drm_bridge *bridge;
+
+ if (!fwnode)
+ return NULL;
+
+ mutex_lock(_lock);
+
+ list_for_each_entry(bridge, _list, list) {
+ if (bridge->fwnode == fwnode) {
+ ret = bridge;
+ break;
+ }
+ }
+
+ mutex_unlock(_lock);
+
+ return ret;
+}
+EXPORT_SYMBOL_GPL(drm_bridge_find_by_fwnode);
+
+/**
+ * drm_bridge_find_next_bridge_by_fwnode - Find the next bridge by fwnode
+ * @fwnode: fwnode pointer to the current device.
+ * @port: identifier of the port node of the next bridge is connected.
+ *
+ * This function find the next bridge at the current node, it assumed that
+ * there has valid fwnode graph established.
+ *
+ * Returns:
+ * * A reference to the requested drm_bridge object on success, or
+ * * -%EINVAL or -%ENODEV if the fwnode graph or OF graph is not complete, or
+ * * %NULL if object does not exist or the next bridge is not probed yet.
+ */
+struct drm_bridge *
+drm_bridge_find_next_bridge_by_fwnode(struct fwnode_handle *fwnode, u32 port)
+{
+ struct drm_bridge *bridge;
+ struct fwnode_handle *ep;
+ struct fwnode_handle *remote;
+
+ ep = fwnode_graph_get_endpoint_by_id(fwnode, port, 0, 0);
+ if (!ep)
+ return ERR_PTR(-EINVAL);
+
+ remote = fwnode_graph_get_remote_port_parent(ep);
+ fwnode_handle_put(ep);
+ if (!remote)
+ return ERR_PTR(-ENODEV);
+
+ bridge = drm_bridge_find_by_fwnode(remote);
+ fwnode_handle_put(remote);
+
+ return bridge;
+}
+EXPORT_SYMBOL_GPL(drm_bridge_find_next_bridge_by_fwnode);
+
MODULE_AUTHOR("Ajay Kumar ");
MODULE_DESCRIPTION("DRM bridge infrastructure");
MODULE_LICENSE("GPL and additional rights");
diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
index 4baca0d9107b..a1bb19425761 100644
--- a/include/drm/drm_bridge.h
+++ b/include/drm/drm_bridge.h
@@ -721,6 +721,8 @@ struct drm_bridge {
struct list_head chain_node;
/** @of_node: device node pointer to the bridge */
struct device_node *of_node;
+ /** @fwnode: fwnode pointer to the bridge */
+ struct fwnode_handle *fwnode;
/** @list: to keep track of all added bridges */
struct list_head list;
/**
@@ -797,6 +799,12 @@ static inline struct drm_bridge *of_drm_find_bridge(struct
device_node *np)
}
#endif
+struct drm_bridge *
+drm_bridge_find_by_fwnode(struct fwnode_handle *fwnode);
+
+struct drm_bridge *
+drm_bridge_find_next_bridge_by_fwnode(struct fwnode_handle *fwnode, u32 port);
+
/**
* drm_bridge_get_next_bridge() - Get the next bridge in the chain
* @bridge: bridge object
--
2.34.1
[PATCH v5 02/10] drm/bridge: Add a helper to setup both the of_node and fwnode of drm bridge
The newly added helper is drm_bridge_set_node(), the reason behind of this
introduction is that the name 'of_node' itself has a smell of DT dependent,
when we are going to make drivers truly DT independent, we have to has a
way to hide it from its user referencing and/or dereferencing.
Please note that the introduction of drm_bridge_set_node() is actually
trying to follow the convention used by driver core, see device_set_node()
for reference.
While at it, include the of.h header as the drm_bridge struct always has
the of_node as its member. Therefore drop the forward declaration.
Signed-off-by: Sui Jingfeng
---
include/drm/drm_bridge.h | 10 --
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
index a1bb19425761..b18e7c2f62c9 100644
--- a/include/drm/drm_bridge.h
+++ b/include/drm/drm_bridge.h
@@ -26,14 +26,13 @@
#include
#include
#include
+#include
#include
#include
#include
#include
-struct device_node;
-
struct drm_bridge;
struct drm_bridge_timings;
struct drm_connector;
@@ -790,6 +789,13 @@ int drm_bridge_attach(struct drm_encoder *encoder, struct
drm_bridge *bridge,
struct drm_bridge *previous,
enum drm_bridge_attach_flags flags);
+static inline void
+drm_bridge_set_node(struct drm_bridge *bridge, struct fwnode_handle *fwnode)
+{
+ bridge->fwnode = fwnode;
+ bridge->of_node = to_of_node(fwnode);
+}
+
#ifdef CONFIG_OF
struct drm_bridge *of_drm_find_bridge(struct device_node *np);
#else
--
2.34.1
[PATCH v5 00/10] drm/bridge: Allow using fwnode API to get the next bridge
Currently, the various display bridge drivers rely on OF infrastructures to works very well, yet there are platforms and/or devices absence of 'OF' support. Such as virtual display drivers, USB display apapters and ACPI based systems etc. Add fwnode based helpers to fill the niche, this allows part of the display bridge drivers to work across systems. As the fwnode API has wider coverage than DT counterpart and the fwnode graphs are compatible with the OF graph, so the provided helpers can be used on all systems in theory. Assumed that the system has valid fwnode graphs established before drm bridge drivers are probed, and there has fwnode assigned to involved drm bridge instance. Tested on TI BeaglePlay board. v1 -> v2: * Modify it66121 to switch togather * Drop the 'side-by-side' implement * Add drm_bridge_find_next_bridge_by_fwnode() helper * Add drm_bridge_set_node() helper v2 -> v3: * Read kernel-doc and improve function comments (Dmitry) * Drop the 'port' argument of it66121_read_bus_width() (Dmitry) * drm-bridge: sii902x get nuked v3 -> v4: * drm-bridge: tfp410 get nuked * Add platform module alias * Rebase and improve commit message and function comments v4 -> v5: * Modify sii9234, ch7033 and ANX7688 * Trivial fixes Sui Jingfeng (10): drm/bridge: Allow using fwnode APIs to get the next bridge drm/bridge: Add a helper to setup both the of_node and fwnode of drm bridge drm/bridge: simple-bridge: Use fwnode APIs to acquire device properties drm/bridge: display-connector: Use fwnode APIs to acquire device properties drm/bridge: sii902x: Switch to use fwnode APIs to acquire device properties drm-bridge: it66121: Use fwnode APIs to acquire device properties drm/bridge: tfp410: Use fwnode APIs to acquire device properties drm/bridge: sii9234: Use fwnode APIs to abstract DT dependent API away drm/bridge: ch7033: Switch to use fwnode based APIs drm/bridge: anx7688: Switch to use drm_bridge_set_node() helper drivers/gpu/drm/bridge/chrontel-ch7033.c | 14 ++-- drivers/gpu/drm/bridge/cros-ec-anx7688.c | 3 +- drivers/gpu/drm/bridge/display-connector.c | 25 drivers/gpu/drm/bridge/ite-it66121.c | 57 ++--- drivers/gpu/drm/bridge/sii902x.c | 45 + drivers/gpu/drm/bridge/sii9234.c | 8 ++- drivers/gpu/drm/bridge/simple-bridge.c | 23 +++ drivers/gpu/drm/bridge/ti-tfp410.c | 41 ++-- drivers/gpu/drm/drm_bridge.c | 74 ++ include/drm/drm_bridge.h | 18 +- 10 files changed, 201 insertions(+), 107 deletions(-) -- 2.34.1
Re: [v1,1/3] drm/panel: ili9341: Correct use of device property APIs
Hi,
On 2024/5/3 01:28, Andy Shevchenko wrote:
On Fri, May 03, 2024 at 12:25:17AM +0800, Sui Jingfeng wrote:
On 2024/5/2 16:32, Andy Shevchenko wrote:
On Wed, May 01, 2024 at 12:27:14AM +0800, Sui Jingfeng wrote:
On 2024/4/30 22:13, Andy Shevchenko wrote:
On Fri, Apr 26, 2024 at 05:13:43AM +0800, Sui Jingfeng wrote:
...
the former might be subdivided to "is it swnode backed or real fwnode one?"
Yeah,
On non-DT cases, it can be subdivided to swnode backed case and ACPI fwnode
backed case.
- For swnode backed case: the device_get_match_data() don't has a
implemented backend.
- For ACPI fwnode backed case: the device_get_match_data() has a implemented
backend.
But the driver has *neither* software node support
True.
nor ACPI support,
Not true.
Why this is not true? Are you means that the panel-ilitek-ili9341 driver has
ACPI support?
That's the idea (as far as I see the copy of the code from tinyDRM),
but broken over the copy'n'paste. This patch rectifies that to be
in align with the original code, which *does* support ACPI.
I'm asking because I don't see struct acpi_device_id related stuff in that
source file,
am I miss something?
Yes, you are. I leave it for you to research.
After researching a few hours I still don't understand how does
the panel-ilitek-ili9341 driver has the ACPI support and be able
to ACPI probed when compiled as module.
As far as I know, drivers that has the ACPI support *must* has the
.acpi_match_table hooked, so that be able to be probed when the
driver is compiled as a module.
For example, see commit 75a1b44a54bd9 ("spi: tegra210-quad: add acpi support")
to get a feel what a SPI device with *real* ACPI support looks like.
I have double checked the panel-ilitek-ili9341 driver, it doesn't
has acpi_match_table hooked, which means that this driver won't
even be able probed. And probed as pure SPI device still out of
the scope of "correct use of device property APIs". Because SPI
device specific method don't belong to the device property API.
I don't really know what's we are missing, but we already intend
to let it go, thanks.
So, slow down and take your time to get into the code and understand how it
works.
so that the rotation property can not get and ili9341_dpi_probe() will fails.
So in total, this is not a 100% correct use of device property APIs.
But I'm fine that if you want to leave(ignore) those less frequent use cases
temporarily,
there may have programmers want to post patches, to complete the missing in the
future.
So, there do have some gains on non-DT cases.
- As you make it be able to compiled on X86 with the drm-misc-defconfig.
- You cleanup the code up (at least patch 2 in this series is no obvious
problem).
- You allow people to modprobe it, and maybe half right and half undefined.
But you do helps moving something forward, so congratulations for the wake up.
--
Best regards,
Sui
Re: [v1,1/3] drm/panel: ili9341: Correct use of device property APIs
Hi, On 2024/5/2 15:34, Neil Armstrong wrote: On 30/04/2024 11:34, Maxime Ripard wrote: On Tue, Apr 30, 2024 at 12:54:39AM +0800, Sui Jingfeng wrote: On 2024/4/29 19:55, Maxime Ripard wrote: On Sat, Apr 27, 2024 at 01:57:46PM +0800, Sui Jingfeng wrote: On 2024/4/26 14:23, Maxime Ripard wrote: On Fri, Apr 26, 2024 at 04:43:18AM +0800, Sui Jingfeng wrote: On 2024/4/26 03:10, Andy Shevchenko wrote: On Fri, Apr 26, 2024 at 02:08:16AM +0800, Sui Jingfeng wrote: On 2024/4/25 22:26, Andy Shevchenko wrote: It seems driver missed the point of proper use of device property APIs. Correct this by updating headers and calls respectively. You are using the 'seems' here exactly saying that you are not 100% sure. Please allow me to tell you the truth: This patch again has ZERO effect. It fix nothing. And this patch is has the risks to be wrong. Huh?! Really, stop commenting the stuff you do not understand. I'm actually a professional display drivers developer at the downstream in the past, despite my contribution to upstream is less. But I believe that all panel driver developers know what I'm talking about. So please have take a look at my replies. Most of the interactions you had in this series has been uncalled for. You might be against a patch, but there's no need to go to such length. As far as I'm concerned, this patch is fine to me in itself, and I don't see anything that would prevent us from merging it. No one is preventing you, as long as don't misunderstanding what other people's technical replies intentionally. I'm just a usual and normal contributor, I hope the world will better than yesterday. You should seriously consider your tone when replying then. Saying such thing to me may not proper, I guess you may want to talk to peoples who has the push rights I think you misunderstood me. My point was that your several rants were uncalled for and aren't the kind of things we're doing here. I know very well how to get a patch merged, thanks. just make sure it isn't a insult to the professionalism of drm bridge community itself though. I'm not sure why you're bringing the bridge community or its professionalism. It's a panel, not a bridge, and I never doubted the professionalism of anyone. I means that the code itself could be adopted, as newer and younger programmer (like Andy) need to be encouraged to contribute. Andy has thousands of commits in Linux. He's *very* far from being a new contributor. I express no obvious objections, just hints him that something else probably should also be taken into consideration as well. That might be what you wanted to express, but you definitely didn't express it that way. On the other hand, we probably should allow other people participate in discussion so that it is sufficient discussed and ensure that it won't be reverted by someone in the future for some reasons. Backing to out case happens here, we may need to move things forward. Therefore, it definitely deserve to have a try. It is not a big deal even though it gets reverted someday. In the end, I don't mind if you think there is nothing that could prevent you from merge it, but I still suggest you have a glance at peoples siting at the Cc list. I'm busy now and I have a lot of other tasks to do, and may not be able to reply you emails on time. So it up to you and other maintainers to decide. Thank you. So far, you're the only one who reviewed those patches. I'm not sure what you're talking about here. Well I (as drm-panel maintainer) did review them positively [...] because the patches looked perfectly correct in regards of the commit message The point is the 'fixes' tag. Then, can I ask what's the issue it fixes? I'm asking because I see the submitting-patches.html [1] documentation told us that a fixes tag indicates that the patch fixes an issue in a previous commit. Previously, the driver only meant to be used on the DT systems, so what's issue? [1] https://www.kernel.org/doc/html/latest/process/submitting-patches.html#reviewer-s-statement-of-oversight I copy & paste the paragraph from link [1] for easier to read. See below: "A Fixes: tag indicates that the patch fixes an issue in a previous commit. It is used to make it easy to determine where a bug originated, which can help review a bug fix. This tag also assists the stable kernel team in determining which stable kernel versions should receive your fix. This is the preferred method for indicating a bug fixed by the patch." and the patchset motivation and OK, the motivation is good, I agree and I admit. because I trust Andy being a long time contributor with a lot of expertise. Does this means that you are going to merge patches from the experts without have a glance and reject or ignore novice's patches unconditionally? I'm asking because I see there still have a lot of other panel drivers use of_device_get_match_data() function to get a match, and mo
Re: [v1,1/3] drm/panel: ili9341: Correct use of device property APIs
Hi, On 2024/5/2 16:32, Andy Shevchenko wrote: On Wed, May 01, 2024 at 12:27:14AM +0800, Sui Jingfeng wrote: On 2024/4/30 22:13, Andy Shevchenko wrote: On Fri, Apr 26, 2024 at 05:13:43AM +0800, Sui Jingfeng wrote: ... the former might be subdivided to "is it swnode backed or real fwnode one?" Yeah, On non-DT cases, it can be subdivided to swnode backed case and ACPI fwnode backed case. - For swnode backed case: the device_get_match_data() don't has a implemented backend. - For ACPI fwnode backed case: the device_get_match_data() has a implemented backend. But the driver has *neither* software node support True. nor ACPI support, Not true. Why this is not true? Are you means that the panel-ilitek-ili9341 driver has ACPI support? I'm asking because I don't see struct acpi_device_id related stuff in that source file, am I miss something? So, slow down and take your time to get into the code and understand how it works. so that the rotation property can not get and ili9341_dpi_probe() will fails. So in total, this is not a 100% correct use of device property APIs. But I'm fine that if you want to leave(ignore) those less frequent use cases temporarily, there may have programmers want to post patches, to complete the missing in the future. So, there do have some gains on non-DT cases. - As you make it be able to compiled on X86 with the drm-misc-defconfig. - You cleanup the code up (at least patch 2 in this series is no obvious problem). - You allow people to modprobe it, and maybe half right and half undefined. But you do helps moving something forward, so congratulations for the wake up. -- Best regards, Sui
[PATCH v2] drm/panel: ili9341: Remove a superfluous else after return
Because the else clause after the return clause is not useful, remove it to get a better look. Reviewed-by: Jessica Zhang Signed-off-by: Sui Jingfeng --- drivers/gpu/drm/panel/panel-ilitek-ili9341.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/panel/panel-ilitek-ili9341.c b/drivers/gpu/drm/panel/panel-ilitek-ili9341.c index 3574681891e8..433572c4caf9 100644 --- a/drivers/gpu/drm/panel/panel-ilitek-ili9341.c +++ b/drivers/gpu/drm/panel/panel-ilitek-ili9341.c @@ -722,7 +722,8 @@ static int ili9341_probe(struct spi_device *spi) if (!strcmp(id->name, "sf-tc240t-9370-t")) return ili9341_dpi_probe(spi, dc, reset); - else if (!strcmp(id->name, "yx240qv29")) + + if (!strcmp(id->name, "yx240qv29")) return ili9341_dbi_probe(spi, dc, reset); return -1; -- 2.34.1
[PATCH v2] drm/debugfs: Drop conditionals around of_node pointers
Having conditional around the of_node pointer of the drm_bridge structure
is not necessary anymore, since drm_bridge structure always has the of_node
member since the commit d8dfccde2709 ("drm/bridge: Drop conditionals around
of_node pointers").
So drop the conditional, please also note that this patch is following the
convention used by driver core, see commit c9e358dfc4a8 ("driver-core:
remove conditionals around devicetree pointers").
Reviewed-by: Dmitry Baryshkov
Signed-off-by: Sui Jingfeng
---
v2: Update commit message
---
drivers/gpu/drm/drm_debugfs.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/drm_debugfs.c b/drivers/gpu/drm/drm_debugfs.c
index 08fcefd804bc..28a471fe4bc8 100644
--- a/drivers/gpu/drm/drm_debugfs.c
+++ b/drivers/gpu/drm/drm_debugfs.c
@@ -597,10 +597,10 @@ static int bridges_show(struct seq_file *m, void *data)
drm_printf(, "\ttype: [%d] %s\n",
bridge->type,
drm_get_connector_type_name(bridge->type));
-#ifdef CONFIG_OF
+
if (bridge->of_node)
drm_printf(, "\tOF: %pOFfc\n", bridge->of_node);
-#endif
+
drm_printf(, "\tops: [0x%x]", bridge->ops);
if (bridge->ops & DRM_BRIDGE_OP_DETECT)
drm_puts(, " detect");
--
2.34.1
Re: drm/debugfs: Drop conditionals around of_node pointers
Hi,
On 2024/5/1 05:33, Doug Anderson wrote:
Hi,
On Mon, Apr 29, 2024 at 6:16 PM 隋景峰 wrote:
Hi,
-原始邮件-
发件人: "Maxime Ripard"
发送时间: 2024-04-29 19:30:24 (星期一)
收件人: "Sui Jingfeng"
抄送: "Sui Jingfeng" , "Maarten Lankhorst" , "Thomas Zimmermann" , "David
Airlie" , "Daniel Vetter" , "Douglas Anderson" , "Laurent Pinchart"
, "Biju Das" , dri-devel@lists.freedesktop.org, linux-ker...@vger.kernel.org
主题: Re: drm/debugfs: Drop conditionals around of_node pointers
On Sun, Apr 28, 2024 at 04:52:13PM +0800, Sui Jingfeng wrote:
ping
在 2024/3/22 06:22, Sui Jingfeng 写道:
Having conditional around the of_node pointer of the drm_bridge structure
turns out to make driver code use ugly #ifdef blocks.
The code being ugly is an opinion, what problem is it causing exactly?
Drop the conditionals to simplify debugfs.
What does it simplifies?
Fixes: d8dfccde2709 ("drm/bridge: Drop conditionals around of_node pointers")
Signed-off-by: Sui Jingfeng
Why do we want to backport that patch to stable?
Technically it's not CCing stable and so it's not really incorrect.
...but I agree that this is a bit of a stretch to call it a "Fix".
Maybe drop the "Fixes" line?
OK, good idea, acceptable.
Originally, I add the fix tag to hint that my modification belongs to
the commit d8dfccde2709 ("drm/bridge: Drop conditionals around of_node
pointers") too.
But get missed.
My commit message is written based on commit of d8dfccde2709
$ git show c9e358dfc4a8
This patch is based on commit c9e358dfc4a8 ("driver-core: remove
conditionals around devicetree pointers").
Having conditional around the of_node pointer of the drm_bridge
structure turns out to make driver code use ugly #ifdef blocks. Drop the
conditionals to simplify drivers. While this slightly increases the size
of struct drm_bridge on non-OF system, the number of bridges used today
and foreseen tomorrow on those systems is very low, so this shouldn't be
an issue.
So drop #if conditionals by adding struct device_node forward declaration.
Maxime
I'm just start to contribute by mimic other people's tone, there seems no need
to over read.
I think the fact that you skipped the reference to commit c9e358dfc4a8
("driver-core: remove conditionals around devicetree pointers") was
relevant here. Referencing that commit makes it easy for the reader to
see that you are following convention used throughout the kernel and
not just asserting your own opinion about style.
If you add that reference into your commit message and send a v2, I'm
happy to apply it.
OK, thanks a lot.
-Doug
--
Best regards,
Sui
Re: [v1,1/3] drm/panel: ili9341: Correct use of device property APIs
Hi,
On 2024/4/30 22:32, Andy Shevchenko wrote:
On Fri, Apr 26, 2024 at 04:43:18AM +0800, Sui Jingfeng wrote:
On 2024/4/26 03:10, Andy Shevchenko wrote:
On Fri, Apr 26, 2024 at 02:08:16AM +0800, Sui Jingfeng wrote:
On 2024/4/25 22:26, Andy Shevchenko wrote:
It seems driver missed the point of proper use of device property APIs.
Correct this by updating headers and calls respectively.
You are using the 'seems' here exactly saying that you are not 100% sure.
To add here, "seems" is used to show that I have no knowledge on what was
the idea behind this implementation by the original author. Plus my knowledge
in the firmware node / device property APIs and use cases in Linux kernel.
Please allow me to tell you the truth: This patch again has ZERO effect.
It fix nothing. And this patch is has the risks to be wrong.
Huh?! Really, stop commenting the stuff you do not understand.
I'm actually a professional display drivers developer at the downstream
in the past, despite my contribution to upstream is less. But I believe
that all panel driver developers know what I'm talking about. So please
have take a look at my replies.
Okay.
Simple because the "ili9341_probe() ---> ili9341_dbi_prob()" code path
is DT dependent.
First of all, the devm_of_find_backlight() is called in ili9341_dbi_probe()
under *non-DT* environment, devm_of_find_backlight() is just a just a
no-op and will return NULL. NULL is not an error code, so ili9341_dbi_probe()
won't rage quit. But the several side effect is that the backlight will
NOT works at all.
Is it a problem?
Yes, it is.
The core problem is that the driver you are modifying has *implicit*
*dependency* on DT.
The implicit dependency is due to the calling of devm_of_find_backlight(). This
function
is a no-op under non-DT systems.
Okay.
Therefore, before the devm_of_find_backlight() and
the device_get_match_data() function can truly DT independent.
True for the first part, not true for the second.
Removing the "OF" dependency just let the tigers run out from the jail.
It is not really meant to targeting at you, but I thinks, all of drm_panel
drivers
that has the devm_of_find_backlight() invoked will suffer such concerns.
In short, the reason is that the *implicit* *dependency* populates and
the undefined behavior gets triggered.
Still no problem statement. My hardware works nicely on non-DT environment.
(And since it's Arduino-based one, I assume it will work on DT environments
the very same way.)
I'm sure you know that device_get_match_data() is same with
of_device_get_match_data()
for DT based systems. For non DT based systems, device_get_match_data() is just
*undefined*
Note that ACPI is not in the scope of the discussion here, as all of the drm
bridges and
panels driver under drivers/gpu/drm/ hasn't the ACPI support yet.
This patch shows exactly how to bring back the ACPI support to one of them
(as it's done for tinyDRM cases).
Therefore, at present,
it safe to say that device_get_match_data() is *undefined* under no-DT
environment.
This is not true.
Removing the "OF" dependency hints to us that it allows the driver to be probed
as a
pure SPI device under non DT systems. When device_get_match_data() is called,
it returns
NULL to us now. As a result, the drm driver being modified will tears down.
See bellow code snippet extracted frompanel-ilitek-ili9341.c:
```
ili->conf = of_device_get_match_data(dev);
if (!ili->conf) {
dev_err(dev, "missing device configuration\n");
return -ENODEV;
}
```
It is actually considered as fatal bug for *panels* if the backlight of
it is not light up, at least the brightness of *won't* be able to adjust.
What's worse, if there is no sane platform setup code at the firmware
or boot loader stage to set a proper initial state. The screen is complete
dark. Even though the itself panel is refreshing framebuffers, it can not
be seen by human's eye. Simple because of no backlight.
Can you imagine that I may have different hardware that considered
this is non-fatal error?
Yes, I can imagine.
I believe you have the hardware which make you patch correct to run
in 99.9% of all cases. But as long as there one bug happened, you patch
are going to be blamed.
Because its your patch that open the door, both from the perceptive of
practice and from the perceptive of the concept (static analysis).
Second, the ili9341_dbi_probe() requires additional device properties to
be able to works very well on the rotation screen case. See the calling
of "device_property_read_u32(dev, "rotation", )" in
ili9341_dbi_probe() function.
Yes, exactly, and how does it object the purpose of this patch?
Because under *non-DT* environment, your commit message do not give a
valid description, how does the additional device property can be acquired
is not demonstrated.
And it is exactly your patch open
Re: [PATCH] drm: drm_of.c: Using EXPORT_SYMBOL_GPL instead of EXPORT_SYMBOL
Hi, On 2024/4/30 17:26, Maxime Ripard wrote: Hi, On Tue, Apr 30, 2024 at 01:35:21AM +0800, Sui Jingfeng wrote: Linux kernel puts strict limits on which functions and data structures are available to loadable kernel modules; only those that have been explicitly exported with EXPORT_SYMBOL() or EXPORT_SYMBOL_GPL() are accessible. In the case of EXPORT_SYMBOL_GPL(), only modules that declare a GPL-compatible license will be able to see the symbol. Since the whole drm_of.c file is declared with GPL-2.0-only license, so let us keep functions in that source file consistently. You're conflating two things: the license of the code itself (GPL2 here), and the license of the users of the symbols exported in that file (anything). There's no relationship between the two, and you have to make an argument for changing the latter other than just because the license is GPL because, again, those are two different things. Yeah, I think you might be correct. It seems that it is valid to have EXPORT_SYMBOL() in GPL-2.0-only licensed file. Maxime -- Best regards, Sui
Re: [PATCH] drm/panel: ili9341: Remove a superfluous else after return
Hi, On 2024/4/30 07:10, Jessica Zhang wrote: On 4/29/2024 10:12 AM, Sui Jingfeng wrote: The else clause after the ruturn clause is not useful. Hi Sui, Spelling nit: ruturn --> return Thanks for pointed out, will be fixed. Besides that, Reviewed-by: Jessica Zhang Thanks, Jessica Zhang Thanks, Jessica.
Re: [v1,3/3] drm/panel: ili9341: Use predefined error codes
On 2024/4/25 22:26, Andy Shevchenko wrote:
In one case the -1 is returned which is quite confusing code for
the wrong device ID, in another the ret is returning instead of
plain 0 that also confusing as readed may ask the possible meaning
of positive codes, which are never the case there. Convert both
to use explicit predefined error codes to make it clear what's going
on there.
Fixes: 5a04227326b0 ("drm/panel: Add ilitek ili9341 panel driver")
Signed-off-by: Andy Shevchenko
Reviewed-by: Neil Armstrong
Reviewed-by: Sui Jingfeng
--
Best regards,
Sui
Re: [v1,2/3] drm/panel: ili9341: Respect deferred probe
Hi,
On 2024/4/25 22:26, Andy Shevchenko wrote:
GPIO controller might not be available when driver is being probed.
There are plenty of reasons why, one of which is deferred probe.
Since GPIOs are optional, return any error code we got to the upper
layer, including deferred probe. With that in mind, use dev_err_probe()
in order to avoid spamming the logs.
Fixes: 5a04227326b0 ("drm/panel: Add ilitek ili9341 panel driver")
Signed-off-by: Andy Shevchenko
Reviewed-by: Neil Armstrong
Reviewed-by: Sui Jingfeng
--
Best regards,
Sui
Re: [v1,1/3] drm/panel: ili9341: Correct use of device property APIs
On 2024/4/30 22:13, Andy Shevchenko wrote:
On Fri, Apr 26, 2024 at 05:13:43AM +0800, Sui Jingfeng wrote:
On 2024/4/26 03:12, Andy Shevchenko wrote:
On Fri, Apr 26, 2024 at 02:53:22AM +0800, Sui Jingfeng wrote:
On 2024/4/26 02:08, Sui Jingfeng wrote:
...
Are you speaking to yourself? I'm totally lost.
Please, if you want to give a constructive feedback, try to understand
the topic from different aspects and then clearly express it.
OK,
The previous email analysis the non-DT cases exhaustively, this email intend to
demonstrate the more frequently use case.
That is, in the *DT('OF')* based systems,
device_get_match_data() is completely equivalent to
of_device_get_match_data().
So the net results of applying this patch are "no gains and no lost".
This is not true.
Yes, I'm true.
I have mentionedin previous(earlier) paragraph with "in the DT(OF) based
systems" or similar words.
It's only part of the cases, i.e. DT. So, I assume you meant
"So the net results of applying this patch are "no gains and no lost" in DT
case".
Yeah,it is true that this patch are "no gains and no lost" in DT case.
Things will become clear if we divide the whole problem into two cases(DT and
non-DT)
to discuss, that's it. That's all I can tell.
Not really.
I'm very clear before our conversation, really!
non-DT cases can also be divided to "fwnode backed or not",
For non fwnode backed case of non-DT cases, there is not decent way to acquire
large set of device properties. And is out of the scope of "Correct use of
device property APIs".
and
the former might be subdivided to "is it swnode backed or real fwnode one?"
Yeah,
On non-DT cases, it can be subdivided to swnode backed case and ACPI fwnode
backed case.
- For swnode backed case: the device_get_match_data() don't has a implemented
backend.
- For ACPI fwnode backed case: the device_get_match_data() has a implemented
backend.
But the driver has *neither* software node support nor ACPI support, so that
the rotation
property can not get and ili9341_dpi_probe() will fails. So in total, this is
not a 100%
correct use of device property APIs.
But I'm fine that if you want to leave(ignore) those less frequent use cases
temporarily,
there may have programmers want to post patches, to complete the missing in the
future.
So, there do have some gains on non-DT cases.
- As you make it be able to compiled on X86 with the drm-misc-defconfig.
- You cleanup the code up (at least patch 2 in this series is no obvious
problem).
- You allow people to modprobe it, and maybe half right and half undefined.
But you do helps moving something forward, so congratulations for the wake up.
--
Best regards,
Sui
[PATCH] drm: drm_of.c: Using EXPORT_SYMBOL_GPL instead of EXPORT_SYMBOL
Linux kernel puts strict limits on which functions and data structures are available to loadable kernel modules; only those that have been explicitly exported with EXPORT_SYMBOL() or EXPORT_SYMBOL_GPL() are accessible. In the case of EXPORT_SYMBOL_GPL(), only modules that declare a GPL-compatible license will be able to see the symbol. Since the whole drm_of.c file is declared with GPL-2.0-only license, so let us keep functions in that source file consistently. Signed-off-by: Sui Jingfeng --- drivers/gpu/drm/drm_of.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c index 177b600895d3..1ca36d654e61 100644 --- a/drivers/gpu/drm/drm_of.c +++ b/drivers/gpu/drm/drm_of.c @@ -44,7 +44,7 @@ uint32_t drm_of_crtc_port_mask(struct drm_device *dev, return 0; } -EXPORT_SYMBOL(drm_of_crtc_port_mask); +EXPORT_SYMBOL_GPL(drm_of_crtc_port_mask); /** * drm_of_find_possible_crtcs - find the possible CRTCs for an encoder port @@ -77,7 +77,7 @@ uint32_t drm_of_find_possible_crtcs(struct drm_device *dev, return possible_crtcs; } -EXPORT_SYMBOL(drm_of_find_possible_crtcs); +EXPORT_SYMBOL_GPL(drm_of_find_possible_crtcs); /** * drm_of_component_match_add - Add a component helper OF node match rule @@ -181,7 +181,7 @@ int drm_of_component_probe(struct device *dev, return component_master_add_with_match(dev, m_ops, match); } -EXPORT_SYMBOL(drm_of_component_probe); +EXPORT_SYMBOL_GPL(drm_of_component_probe); /* * drm_of_encoder_active_endpoint - return the active encoder endpoint -- 2.34.1
[PATCH] drm/panel: ili9341: Remove a superfluous else after return
The else clause after the ruturn clause is not useful. Signed-off-by: Sui Jingfeng --- drivers/gpu/drm/panel/panel-ilitek-ili9341.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/panel/panel-ilitek-ili9341.c b/drivers/gpu/drm/panel/panel-ilitek-ili9341.c index 3574681891e8..433572c4caf9 100644 --- a/drivers/gpu/drm/panel/panel-ilitek-ili9341.c +++ b/drivers/gpu/drm/panel/panel-ilitek-ili9341.c @@ -722,7 +722,8 @@ static int ili9341_probe(struct spi_device *spi) if (!strcmp(id->name, "sf-tc240t-9370-t")) return ili9341_dpi_probe(spi, dc, reset); - else if (!strcmp(id->name, "yx240qv29")) + + if (!strcmp(id->name, "yx240qv29")) return ili9341_dbi_probe(spi, dc, reset); return -1; -- 2.34.1
Re: [v1,1/3] drm/panel: ili9341: Correct use of device property APIs
Hi, On 2024/4/29 19:55, Maxime Ripard wrote: On Sat, Apr 27, 2024 at 01:57:46PM +0800, Sui Jingfeng wrote: Hi, On 2024/4/26 14:23, Maxime Ripard wrote: Hi, On Fri, Apr 26, 2024 at 04:43:18AM +0800, Sui Jingfeng wrote: On 2024/4/26 03:10, Andy Shevchenko wrote: On Fri, Apr 26, 2024 at 02:08:16AM +0800, Sui Jingfeng wrote: On 2024/4/25 22:26, Andy Shevchenko wrote: It seems driver missed the point of proper use of device property APIs. Correct this by updating headers and calls respectively. You are using the 'seems' here exactly saying that you are not 100% sure. Please allow me to tell you the truth: This patch again has ZERO effect. It fix nothing. And this patch is has the risks to be wrong. Huh?! Really, stop commenting the stuff you do not understand. I'm actually a professional display drivers developer at the downstream in the past, despite my contribution to upstream is less. But I believe that all panel driver developers know what I'm talking about. So please have take a look at my replies. Most of the interactions you had in this series has been uncalled for. You might be against a patch, but there's no need to go to such length. As far as I'm concerned, this patch is fine to me in itself, and I don't see anything that would prevent us from merging it. No one is preventing you, as long as don't misunderstanding what other people's technical replies intentionally. I'm just a usual and normal contributor, I hope the world will better than yesterday. You should seriously consider your tone when replying then. Saying such thing to me may not proper, I guess you may want to talk to peoples who has the push rights I think you misunderstood me. My point was that your several rants were uncalled for and aren't the kind of things we're doing here. I know very well how to get a patch merged, thanks. just make sure it isn't a insult to the professionalism of drm bridge community itself though. I'm not sure why you're bringing the bridge community or its professionalism. It's a panel, not a bridge, and I never doubted the professionalism of anyone. I means that the code itself could be adopted, as newer and younger programmer (like Andy) need to be encouraged to contribute. I express no obvious objections, just hints him that something else probably should also be taken into consideration as well. On the other hand, we probably should allow other people participate in discussion so that it is sufficient discussed and ensure that it won't be reverted by someone in the future for some reasons. Backing to out case happens here, we may need to move things forward. Therefore, it definitely deserve to have a try. It is not a big deal even though it gets reverted someday. In the end, I don't mind if you think there is nothing that could prevent you from merge it, but I still suggest you have a glance at peoples siting at the Cc list. I'm busy now and I have a lot of other tasks to do, and may not be able to reply you emails on time. So it up to you and other maintainers to decide. Thank you. Maxime -- Best regards, Sui
Re: drm/debugfs: Drop conditionals around of_node pointers
ping
在 2024/3/22 06:22, Sui Jingfeng 写道:
Having conditional around the of_node pointer of the drm_bridge structure
turns out to make driver code use ugly #ifdef blocks. Drop the conditionals
to simplify debugfs.
Fixes: d8dfccde2709 ("drm/bridge: Drop conditionals around of_node pointers")
Signed-off-by: Sui Jingfeng
---
drivers/gpu/drm/drm_debugfs.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/drm_debugfs.c b/drivers/gpu/drm/drm_debugfs.c
index 08fcefd804bc..28a471fe4bc8 100644
--- a/drivers/gpu/drm/drm_debugfs.c
+++ b/drivers/gpu/drm/drm_debugfs.c
@@ -597,10 +597,10 @@ static int bridges_show(struct seq_file *m, void *data)
drm_printf(, "\ttype: [%d] %s\n",
bridge->type,
drm_get_connector_type_name(bridge->type));
-#ifdef CONFIG_OF
+
if (bridge->of_node)
drm_printf(, "\tOF: %pOFfc\n", bridge->of_node);
-#endif
+
drm_printf(, "\tops: [0x%x]", bridge->ops);
if (bridge->ops & DRM_BRIDGE_OP_DETECT)
drm_puts(, " detect");
--
Best regards
Sui Jingfeng
[PATCH v3] software node: Implement device_get_match_data fwnode callback
Because the software node backend of the fwnode API framework lacks an
implementation for the .device_get_match_data function callback. This
makes it difficult to use(and/or test) a few drivers that originates
from DT world on the non-DT platform.
Implement the .device_get_match_data fwnode callback, which helps to keep
the three backends of the fwnode API aligned as much as possible. This is
also a fundamental step to make a few drivers OF-independent truely
possible.
Device drivers or platform setup codes are expected to provide a software
node string property, named as "compatible". At this moment, the value of
this string property is being used to match against the compatible entries
in the of_device_id table. It can be extended in the future though.
Fixes: ffb42e64561e ("drm/tiny/repaper: Make driver OF-independent")
Fixes: 5703d6ae9573 ("drm/tiny/st7735r: Make driver OF-independent")
Signed-off-by: Sui Jingfeng
---
Cc: Andy Shevchenko
Cc: Daniel Scally
Cc: Heikki Krogerus
Cc: Sakari Ailus
Cc: Greg Kroah-Hartman
Cc: "Rafael J. Wysocki"
---
v1 -> v2: Update commit message
v2 -> v3: Move a loop invariant conditional out of while clause
---
drivers/base/swnode.c | 29 +
1 file changed, 29 insertions(+)
diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
index eb6eb25b343b..b6f40715c4f8 100644
--- a/drivers/base/swnode.c
+++ b/drivers/base/swnode.c
@@ -15,6 +15,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -390,6 +391,33 @@ static void software_node_put(struct fwnode_handle *fwnode)
kobject_put(>kobj);
}
+static const void *
+software_node_get_match_data(const struct fwnode_handle *fwnode,
+const struct device *dev)
+{
+ struct swnode *swnode = to_swnode(fwnode);
+ const struct of_device_id *matches = dev->driver->of_match_table;
+ const char *val = NULL;
+ int ret;
+
+ ret = property_entry_read_string_array(swnode->node->properties,
+ "compatible", , 1);
+ if (ret < 0 || !val)
+ return NULL;
+
+ if (!matches)
+ return NULL;
+
+ while (matches->compatible[0]) {
+ if (!strcmp(matches->compatible, val))
+ return matches->data;
+
+ ++matches;
+ }
+
+ return NULL;
+}
+
static bool software_node_property_present(const struct fwnode_handle *fwnode,
const char *propname)
{
@@ -676,6 +704,7 @@ software_node_graph_parse_endpoint(const struct
fwnode_handle *fwnode,
static const struct fwnode_operations software_node_ops = {
.get = software_node_get,
.put = software_node_put,
+ .device_get_match_data = software_node_get_match_data,
.property_present = software_node_property_present,
.property_read_int_array = software_node_read_int_array,
.property_read_string_array = software_node_read_string_array,
--
2.34.1
Re: [PATCH v4 8/9] drm/bridge: tfp410: Use fwnode API to acquire device properties
Hi,
On 2024/4/28 03:17, Dmitry Baryshkov wrote:
On Sun, Apr 28, 2024 at 02:43:20AM +0800, Sui Jingfeng wrote:
Hi,
On 2024/4/23 04:08, Dmitry Baryshkov wrote:
On Tue, Apr 23, 2024 at 03:19:02AM +0800, Sui Jingfeng wrote:
Make this driver DT-independent by calling the freshly created helpers,
which reduce boilerplate and open the door for otherwise use cases. No
functional changes for DT based systems.
Signed-off-by: Sui Jingfeng
---
drivers/gpu/drm/bridge/ti-tfp410.c | 41 +++---
1 file changed, 21 insertions(+), 20 deletions(-)
diff --git a/drivers/gpu/drm/bridge/ti-tfp410.c
b/drivers/gpu/drm/bridge/ti-tfp410.c
index c7bef5c23927..58dc7492844f 100644
--- a/drivers/gpu/drm/bridge/ti-tfp410.c
+++ b/drivers/gpu/drm/bridge/ti-tfp410.c
@@ -266,8 +266,9 @@ static const struct drm_bridge_timings
tfp410_default_timings = {
static int tfp410_parse_timings(struct tfp410 *dvi, bool i2c)
{
+ struct fwnode_handle *fwnode = dev_fwnode(dvi->dev);
struct drm_bridge_timings *timings = >timings;
- struct device_node *ep;
+ struct fwnode_handle *ep;
u32 pclk_sample = 0;
u32 bus_width = 24;
u32 deskew = 0;
@@ -288,14 +289,14 @@ static int tfp410_parse_timings(struct tfp410 *dvi, bool
i2c)
* and EDGE pins. They are specified in DT through endpoint properties
* and vendor-specific properties.
*/
- ep = of_graph_get_endpoint_by_regs(dvi->dev->of_node, 0, 0);
+ ep = fwnode_graph_get_endpoint_by_id(fwnode, 0, 0, 0);
if (!ep)
return -EINVAL;
/* Get the sampling edge from the endpoint. */
- of_property_read_u32(ep, "pclk-sample", _sample);
- of_property_read_u32(ep, "bus-width", _width);
- of_node_put(ep);
+ fwnode_property_read_u32(ep, "pclk-sample", _sample);
+ fwnode_property_read_u32(ep, "bus-width", _width);
+ fwnode_handle_put(ep);
timings->input_bus_flags = DRM_BUS_FLAG_DE_HIGH;
@@ -324,7 +325,7 @@ static int tfp410_parse_timings(struct tfp410 *dvi, bool
i2c)
}
/* Get the setup and hold time from vendor-specific properties. */
- of_property_read_u32(dvi->dev->of_node, "ti,deskew", );
+ fwnode_property_read_u32(fwnode, "ti,deskew", );
if (deskew > 7)
return -EINVAL;
@@ -336,12 +337,12 @@ static int tfp410_parse_timings(struct tfp410 *dvi, bool
i2c)
static int tfp410_init(struct device *dev, bool i2c)
{
- struct device_node *node;
+ struct fwnode_handle *fwnode = dev_fwnode(dev);
struct tfp410 *dvi;
int ret;
- if (!dev->of_node) {
- dev_err(dev, "device-tree data is missing\n");
+ if (!fwnode) {
+ dev_err(dev, "firmware data is missing\n");
return -ENXIO;
}
@@ -352,8 +353,8 @@ static int tfp410_init(struct device *dev, bool i2c)
dvi->dev = dev;
dev_set_drvdata(dev, dvi);
+ drm_bridge_set_node(>bridge, fwnode);
dvi->bridge.funcs = _bridge_funcs;
- dvi->bridge.of_node = dev->of_node;
dvi->bridge.timings = >timings;
dvi->bridge.type = DRM_MODE_CONNECTOR_DVID;
@@ -362,15 +363,15 @@ static int tfp410_init(struct device *dev, bool i2c)
return ret;
/* Get the next bridge, connected to port@1. */
- node = of_graph_get_remote_node(dev->of_node, 1, -1);
- if (!node)
- return -ENODEV;
-
- dvi->next_bridge = of_drm_find_bridge(node);
- of_node_put(node);
-
- if (!dvi->next_bridge)
+ dvi->next_bridge = drm_bridge_find_next_bridge_by_fwnode(fwnode, 1);
+ if (IS_ERR(dvi->next_bridge)) {
+ ret = PTR_ERR(dvi->next_bridge);
+ dev_err(dev, "Error in founding the next bridge: %d\n", ret);
+ return ret;
Same comment regarding dev_err_probe().
LGTM otherwise.
My drm_bridge_find_next_bridge_by_fwnode() function won't return -EPROBE_DEFER,
this is known for sure. this can be used as a prior(priori) knowledge. This is
intentionally by design.
Calling the dev_err_probe() just introduce extra overhead on non EPROBE_DEFER
cases. Hence, It is useless to use dev_err_probe() at here.
+ } else if (!dvi->next_bridge) {
+ dev_dbg(dev, "Next bridge not found, deferring probe\n");
return -EPROBE_DEFER;
Looking at the bolerplate code, I think it would be better to make
drm_bridge_find_next_bridge_by_fwnode() reutrn -EPROBE_DEFER on its own.
The drm_bridge_find_next_bridge_by_fwnode() function itself can not
reliable detect if the driver(the remote bridge) already probed or not.
Hence, as a core helper function, we can not guarantee that return
-EPROBE_DEFER is always correct.
While, return NULL is always corre
Re: [PATCH v4 8/9] drm/bridge: tfp410: Use fwnode API to acquire device properties
Hi,
On 2024/4/23 04:08, Dmitry Baryshkov wrote:
On Tue, Apr 23, 2024 at 03:19:02AM +0800, Sui Jingfeng wrote:
Make this driver DT-independent by calling the freshly created helpers,
which reduce boilerplate and open the door for otherwise use cases. No
functional changes for DT based systems.
Signed-off-by: Sui Jingfeng
---
drivers/gpu/drm/bridge/ti-tfp410.c | 41 +++---
1 file changed, 21 insertions(+), 20 deletions(-)
diff --git a/drivers/gpu/drm/bridge/ti-tfp410.c
b/drivers/gpu/drm/bridge/ti-tfp410.c
index c7bef5c23927..58dc7492844f 100644
--- a/drivers/gpu/drm/bridge/ti-tfp410.c
+++ b/drivers/gpu/drm/bridge/ti-tfp410.c
@@ -266,8 +266,9 @@ static const struct drm_bridge_timings
tfp410_default_timings = {
static int tfp410_parse_timings(struct tfp410 *dvi, bool i2c)
{
+ struct fwnode_handle *fwnode = dev_fwnode(dvi->dev);
struct drm_bridge_timings *timings = >timings;
- struct device_node *ep;
+ struct fwnode_handle *ep;
u32 pclk_sample = 0;
u32 bus_width = 24;
u32 deskew = 0;
@@ -288,14 +289,14 @@ static int tfp410_parse_timings(struct tfp410 *dvi, bool
i2c)
* and EDGE pins. They are specified in DT through endpoint properties
* and vendor-specific properties.
*/
- ep = of_graph_get_endpoint_by_regs(dvi->dev->of_node, 0, 0);
+ ep = fwnode_graph_get_endpoint_by_id(fwnode, 0, 0, 0);
if (!ep)
return -EINVAL;
/* Get the sampling edge from the endpoint. */
- of_property_read_u32(ep, "pclk-sample", _sample);
- of_property_read_u32(ep, "bus-width", _width);
- of_node_put(ep);
+ fwnode_property_read_u32(ep, "pclk-sample", _sample);
+ fwnode_property_read_u32(ep, "bus-width", _width);
+ fwnode_handle_put(ep);
timings->input_bus_flags = DRM_BUS_FLAG_DE_HIGH;
@@ -324,7 +325,7 @@ static int tfp410_parse_timings(struct tfp410 *dvi, bool i2c)
}
/* Get the setup and hold time from vendor-specific properties. */
- of_property_read_u32(dvi->dev->of_node, "ti,deskew", );
+ fwnode_property_read_u32(fwnode, "ti,deskew", );
if (deskew > 7)
return -EINVAL;
@@ -336,12 +337,12 @@ static int tfp410_parse_timings(struct tfp410 *dvi, bool i2c)
static int tfp410_init(struct device *dev, bool i2c)
{
- struct device_node *node;
+ struct fwnode_handle *fwnode = dev_fwnode(dev);
struct tfp410 *dvi;
int ret;
- if (!dev->of_node) {
- dev_err(dev, "device-tree data is missing\n");
+ if (!fwnode) {
+ dev_err(dev, "firmware data is missing\n");
return -ENXIO;
}
@@ -352,8 +353,8 @@ static int tfp410_init(struct device *dev, bool i2c)
dvi->dev = dev;
dev_set_drvdata(dev, dvi);
+ drm_bridge_set_node(>bridge, fwnode);
dvi->bridge.funcs = _bridge_funcs;
- dvi->bridge.of_node = dev->of_node;
dvi->bridge.timings = >timings;
dvi->bridge.type = DRM_MODE_CONNECTOR_DVID;
@@ -362,15 +363,15 @@ static int tfp410_init(struct device *dev, bool i2c)
return ret;
/* Get the next bridge, connected to port@1. */
- node = of_graph_get_remote_node(dev->of_node, 1, -1);
- if (!node)
- return -ENODEV;
-
- dvi->next_bridge = of_drm_find_bridge(node);
- of_node_put(node);
-
- if (!dvi->next_bridge)
+ dvi->next_bridge = drm_bridge_find_next_bridge_by_fwnode(fwnode, 1);
+ if (IS_ERR(dvi->next_bridge)) {
+ ret = PTR_ERR(dvi->next_bridge);
+ dev_err(dev, "Error in founding the next bridge: %d\n", ret);
+ return ret;
Same comment regarding dev_err_probe().
LGTM otherwise.
My drm_bridge_find_next_bridge_by_fwnode() function won't return -EPROBE_DEFER,
this is known for sure. this can be used as a prior(priori) knowledge. This is
intentionally by design.
Calling the dev_err_probe() just introduce extra overhead on non EPROBE_DEFER
cases. Hence, It is useless to use dev_err_probe() at here.
+ } else if (!dvi->next_bridge) {
+ dev_dbg(dev, "Next bridge not found, deferring probe\n");
return -EPROBE_DEFER;
Looking at the bolerplate code, I think it would be better to make
drm_bridge_find_next_bridge_by_fwnode() reutrn -EPROBE_DEFER on its own.
The drm_bridge_find_next_bridge_by_fwnode() function itself can not
reliable detect if the driver(the remote bridge) already probed or not.
Hence, as a core helper function, we can not guarantee that return
-EPROBE_DEFER is always correct.
While, return NULL is always correct. The NULL can stand for two meanings.
One is that the next bridge is really don't exist, may happen when th
