| Message ID | 20240514154045.309925-1-sui.jingfeng@linux.dev (mailing list archive) |
|---|---|
| Headers | show |
| Series | drm/bridge: Add 'struct device *' field to the drm_bridge structure | expand |
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
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
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
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
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
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
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
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
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.
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.
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?
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