Re: [PATCH 0/2] drm/bridge: Add 'struct device *' field to the drm_bridge structure
On Thu, May 16, 2024 at 08:04:59PM GMT, Sui Jingfeng wrote: > > > 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. So that's what you're going for. > 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. Please make that happen once the device_property_read_* API is there, and we can get rid of of_node, swnode, and fwnode then. Maxime signature.asc Description: PGP signature
Re: [PATCH 0/2] drm/bridge: Add 'struct device *' field to the drm_bridge structure
On Thu, May 16, 2024 at 06:40:45PM GMT, Sui Jingfeng wrote: > 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. But *why*? What benefit does it bring that outweights the downsides I listed earlier? > 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. Sure, we could use a function instead of another one. But again, what benefit does that bring exactly? signature.asc Description: PGP signature
Re: [PATCH 0/2] drm/bridge: Add 'struct device *' field to the drm_bridge structure
On Thu, May 16, 2024 at 08:04:59PM +0800, Sui Jingfeng wrote: > > > 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. This doesn't sound 100% correct. This is going to drop the possibile case when bridge driver uses child DT or FW node under the main device node. For example of such usecase see drivers/usb/typec/tcpm/qcom/qcom_pmic_typec.c > > We could do the programming around the 'struct device *.', remove > a batch of boilerplate. -- With best wishes Dmitry
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 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 0/2] drm/bridge: Add 'struct device *' field to the drm_bridge structure
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. Maxime signature.asc Description: PGP signature
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
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. Maxime signature.asc Description: PGP signature
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 0/2] drm/bridge: Add 'struct device *' field to the drm_bridge structure
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. Maxime signature.asc Description: PGP signature
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
Re: [PATCH 0/2] drm/bridge: Add 'struct device *' field to the drm_bridge structure
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? Maxime signature.asc Description: PGP signature
[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
