| Message ID | 20240514154045.309925-2-sui.jingfeng@linux.dev (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/bridge: Add 'struct device *' field to the drm_bridge structure | expand |
On Tue, 14 May 2024, Sui Jingfeng <sui.jingfeng@linux.dev> 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 <sui.jingfeng@linux.dev> > --- > 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. 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(&bridge_lock); > + > + list_for_each_entry(bridge, &bridge_list, list) { > + if (bridge->kdev == kdev) { > + mutex_unlock(&bridge_lock); > + return bridge; > + } > + } > + > + mutex_unlock(&bridge_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 <ajaykumar.rs@samsung.com>"); > 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
Hi, On 5/15/24 17:39, Jani Nikula wrote: > On Tue, 14 May 2024, Sui Jingfeng <sui.jingfeng@linux.dev> 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 <sui.jingfeng@linux.dev> >> --- >> 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(&bridge_lock); >> + >> + list_for_each_entry(bridge, &bridge_list, list) { >> + if (bridge->kdev == kdev) { >> + mutex_unlock(&bridge_lock); >> + return bridge; >> + } >> + } >> + >> + mutex_unlock(&bridge_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 <ajaykumar.rs@samsung.com>"); >> 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 >
On Wed, 15 May 2024, Sui Jingfeng <sui.jingfeng@linux.dev> wrote: > Hi, > > > On 5/15/24 17:39, Jani Nikula wrote: >> On Tue, 14 May 2024, Sui Jingfeng <sui.jingfeng@linux.dev> 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." "_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? BR, Jani.
Hi, On 5/15/24 18:28, Jani Nikula wrote: > On Wed, 15 May 2024, Sui Jingfeng <sui.jingfeng@linux.dev> wrote: >> Hi, >> >> >> On 5/15/24 17:39, Jani Nikula wrote: >>> On Tue, 14 May 2024, Sui Jingfeng <sui.jingfeng@linux.dev> 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. > >
On Wed, 15 May 2024, Sui Jingfeng <sui.jingfeng@linux.dev> wrote:
> Yes, you are right, I'll back give another try then.
Thanks, but please do wait until you have feedback on whether the whole
thing is a good idea or not. :)
BR,
Jani.
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(&bridge_lock); + + list_for_each_entry(bridge, &bridge_list, list) { + if (bridge->kdev == kdev) { + mutex_unlock(&bridge_lock); + return bridge; + } + } + + mutex_unlock(&bridge_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 <ajaykumar.rs@samsung.com>"); 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
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 <sui.jingfeng@linux.dev> --- drivers/gpu/drm/drm_bridge.c | 39 ++++++++++++++++++++++++++++++++++++ include/drm/drm_bridge.h | 5 +++++ 2 files changed, 44 insertions(+)