Re: [PATCH 01/30] drm: Introduce drm_bridge_init()
On Wed, Nov 27, 2019 at 11:05:56AM +, Mihail Atanassov wrote: > On Tuesday, 26 November 2019 19:24:45 GMT Sam Ravnborg wrote: > > Hi Mihail. > > Hi Sam, > > > > > > Ack, but with one caveat: bridge->dev is the struct drm_device that is > > > the bridge client, we need to add a bridge->device (patch 29 in this > > > series) which is the struct device that will manage the bridge lifetime. > > Other places uses the variable name "drm" for a drm_device. > > This is less confusion than the "dev" name. > > > > It seems a recent trend to use the variable name "drm" so you can find a > > lot of places using "dev". > > > > bike-shedding - but also about readability. > > > > Sam > > > > I'm okay with the idea, I can do a follow-up patch or series for the > rename; I expect it would be a bit hefty to do it prior to this. > > @Daniel, thoughts on s/bridge.dev/bridge.drm/ and > s/bridge.device/bridge.dev/ after this series? I'm firmly in the "no opionon" on this. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 01/30] drm: Introduce drm_bridge_init()
On Tuesday, 26 November 2019 19:24:45 GMT Sam Ravnborg wrote: > Hi Mihail. Hi Sam, > > > Ack, but with one caveat: bridge->dev is the struct drm_device that is > > the bridge client, we need to add a bridge->device (patch 29 in this > > series) which is the struct device that will manage the bridge lifetime. > Other places uses the variable name "drm" for a drm_device. > This is less confusion than the "dev" name. > > It seems a recent trend to use the variable name "drm" so you can find a > lot of places using "dev". > > bike-shedding - but also about readability. > > Sam > I'm okay with the idea, I can do a follow-up patch or series for the rename; I expect it would be a bit hefty to do it prior to this. @Daniel, thoughts on s/bridge.dev/bridge.drm/ and s/bridge.device/bridge.dev/ after this series? -- Mihail ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 01/30] drm: Introduce drm_bridge_init()
Hi Mihail. > Ack, but with one caveat: bridge->dev is the struct drm_device that is > the bridge client, we need to add a bridge->device (patch 29 in this > series) which is the struct device that will manage the bridge lifetime. Other places uses the variable name "drm" for a drm_device. This is less confusion than the "dev" name. It seems a recent trend to use the variable name "drm" so you can find a lot of places using "dev". bike-shedding - but also about readability. Sam ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 01/30] drm: Introduce drm_bridge_init()
On Tue, Nov 26, 2019 at 4:55 PM Mihail Atanassov
wrote:
>
> Hi Daniel,
>
> Thanks for the quick review.
>
> On Tuesday, 26 November 2019 14:26:10 GMT Daniel Vetter wrote:
> > On Tue, Nov 26, 2019 at 01:15:59PM +, Mihail Atanassov wrote:
> > > A simple convenience function to initialize the struct drm_bridge.
> > >
> > > Signed-off-by: Mihail Atanassov
> >
> > The commit message here leaves figuring out why we need this to the
> > reader. Reading ahead the reasons seems to be to roll out bridge->dev
> > setting for everyone, so that we can set the device_link. Please explain
> > that in the commit message so the patch is properly motivated.
>
> Ack, but with one caveat: bridge->dev is the struct drm_device that is
> the bridge client, we need to add a bridge->device (patch 29 in this
> series) which is the struct device that will manage the bridge lifetime.
Ah yes, ->dev is for drm_bridge_attach.
> >
> > > ---
> > > drivers/gpu/drm/drm_bridge.c | 29 +
> > > include/drm/drm_bridge.h | 4
> > > 2 files changed, 33 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> > > index cba537c99e43..cbe680aa6eac 100644
> > > --- a/drivers/gpu/drm/drm_bridge.c
> > > +++ b/drivers/gpu/drm/drm_bridge.c
> > > @@ -89,6 +89,35 @@ void drm_bridge_remove(struct drm_bridge *bridge)
> > > }
> > > EXPORT_SYMBOL(drm_bridge_remove);
> > >
> > > +/**
> > > + * drm_bridge_init - initialise a drm_bridge structure
> > > + *
> > > + * @bridge: bridge control structure
> > > + * @funcs: control functions
> > > + * @dev: device
> > > + * @timings: timing specification for the bridge; optional (may be NULL)
> > > + * @driver_private: pointer to the bridge driver internal context (may
> > > be NULL)
> >
> > Please also sprinkle some links to this new function to relevant places,
> > I'd add at least:
> >
> > "Drivers should call drm_bridge_init() first." to the kerneldoc for
> > drm_bridge_add. drm_bridge_add should also mention drm_bridge_remove as
> > the undo function.
> >
> > And perhaps a longer paragraph to &struct drm_bridge:
> >
> > "Bridge drivers should call drm_bridge_init() to initialized a bridge
> > driver, and then register it with drm_bridge_add().
> >
> > "Users of bridges link a bridge driver into their overall display output
> > pipeline by calling drm_bridge_attach()."
>
> Will do.
>
> >
> > > + */
> > > +void drm_bridge_init(struct drm_bridge *bridge, struct device *dev,
> > > +const struct drm_bridge_funcs *funcs,
> > > +const struct drm_bridge_timings *timings,
> > > +void *driver_private)
> > > +{
> > > + WARN_ON(!funcs);
> > > +
> > > + bridge->dev = NULL;
> >
> > Given that the goal here is to get bridge->dev set up, why not
> >
> > WARN_ON(!dev);
> > bridge->dev = dev;
>
> See above struct device vs struct drm_device. I add a
>
> bridge->device = dev;
>
> in patch 29, which takes care of that. I skipped the warn since
> there's a dereference of dev, but I now realized it's behind CONFIG_OF,
> so I'll add it in for v2.
Ok, sounds good. Having the WARN_ON in patch 1 should also help making
sure all the conversion patches dtrt (and any future users).
-Daniel
> Yes, 'device' isn't the best of names, but I took Russell's patch
> almost as-is, I didn't have any better ideas for bikeshedding.
>
> >
> > That should help us to really move forward with all this.
> > -Daniel
> >
> > > + bridge->encoder = NULL;
> > > + bridge->next = NULL;
> > > +
> > > +#ifdef CONFIG_OF
> > > + bridge->of_node = dev->of_node;
> > > +#endif
> > > + bridge->timings = timings;
> > > + bridge->funcs = funcs;
> > > + bridge->driver_private = driver_private;
> > > +}
> > > +EXPORT_SYMBOL(drm_bridge_init);
> > > +
> > > /**
> > > * drm_bridge_attach - attach the bridge to an encoder's chain
> > > *
> > > diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
> > > index c0a2286a81e9..d6d9d5301551 100644
> > > --- a/include/drm/drm_bridge.h
> > > +++ b/include/drm/drm_bridge.h
> > > @@ -402,6 +402,10 @@ struct drm_bridge {
> > >
> > > void drm_bridge_add(struct drm_bridge *bridge);
> > > void drm_bridge_remove(struct drm_bridge *bridge);
> > > +void drm_bridge_init(struct drm_bridge *bridge, struct device *dev,
> > > +const struct drm_bridge_funcs *funcs,
> > > +const struct drm_bridge_timings *timings,
> > > +void *driver_private);
> > > struct drm_bridge *of_drm_find_bridge(struct device_node *np);
> > > int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge
> > > *bridge,
> > > struct drm_bridge *previous);
> >
> >
>
>
> --
> Mihail
>
>
>
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org
Re: [PATCH 01/30] drm: Introduce drm_bridge_init()
Hi Daniel,
Thanks for the quick review.
On Tuesday, 26 November 2019 14:26:10 GMT Daniel Vetter wrote:
> On Tue, Nov 26, 2019 at 01:15:59PM +, Mihail Atanassov wrote:
> > A simple convenience function to initialize the struct drm_bridge.
> >
> > Signed-off-by: Mihail Atanassov
>
> The commit message here leaves figuring out why we need this to the
> reader. Reading ahead the reasons seems to be to roll out bridge->dev
> setting for everyone, so that we can set the device_link. Please explain
> that in the commit message so the patch is properly motivated.
Ack, but with one caveat: bridge->dev is the struct drm_device that is
the bridge client, we need to add a bridge->device (patch 29 in this
series) which is the struct device that will manage the bridge lifetime.
>
> > ---
> > drivers/gpu/drm/drm_bridge.c | 29 +
> > include/drm/drm_bridge.h | 4
> > 2 files changed, 33 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> > index cba537c99e43..cbe680aa6eac 100644
> > --- a/drivers/gpu/drm/drm_bridge.c
> > +++ b/drivers/gpu/drm/drm_bridge.c
> > @@ -89,6 +89,35 @@ void drm_bridge_remove(struct drm_bridge *bridge)
> > }
> > EXPORT_SYMBOL(drm_bridge_remove);
> >
> > +/**
> > + * drm_bridge_init - initialise a drm_bridge structure
> > + *
> > + * @bridge: bridge control structure
> > + * @funcs: control functions
> > + * @dev: device
> > + * @timings: timing specification for the bridge; optional (may be NULL)
> > + * @driver_private: pointer to the bridge driver internal context (may be
> > NULL)
>
> Please also sprinkle some links to this new function to relevant places,
> I'd add at least:
>
> "Drivers should call drm_bridge_init() first." to the kerneldoc for
> drm_bridge_add. drm_bridge_add should also mention drm_bridge_remove as
> the undo function.
>
> And perhaps a longer paragraph to &struct drm_bridge:
>
> "Bridge drivers should call drm_bridge_init() to initialized a bridge
> driver, and then register it with drm_bridge_add().
>
> "Users of bridges link a bridge driver into their overall display output
> pipeline by calling drm_bridge_attach()."
Will do.
>
> > + */
> > +void drm_bridge_init(struct drm_bridge *bridge, struct device *dev,
> > +const struct drm_bridge_funcs *funcs,
> > +const struct drm_bridge_timings *timings,
> > +void *driver_private)
> > +{
> > + WARN_ON(!funcs);
> > +
> > + bridge->dev = NULL;
>
> Given that the goal here is to get bridge->dev set up, why not
>
> WARN_ON(!dev);
> bridge->dev = dev;
See above struct device vs struct drm_device. I add a
bridge->device = dev;
in patch 29, which takes care of that. I skipped the warn since
there's a dereference of dev, but I now realized it's behind CONFIG_OF,
so I'll add it in for v2.
Yes, 'device' isn't the best of names, but I took Russell's patch
almost as-is, I didn't have any better ideas for bikeshedding.
>
> That should help us to really move forward with all this.
> -Daniel
>
> > + bridge->encoder = NULL;
> > + bridge->next = NULL;
> > +
> > +#ifdef CONFIG_OF
> > + bridge->of_node = dev->of_node;
> > +#endif
> > + bridge->timings = timings;
> > + bridge->funcs = funcs;
> > + bridge->driver_private = driver_private;
> > +}
> > +EXPORT_SYMBOL(drm_bridge_init);
> > +
> > /**
> > * drm_bridge_attach - attach the bridge to an encoder's chain
> > *
> > diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
> > index c0a2286a81e9..d6d9d5301551 100644
> > --- a/include/drm/drm_bridge.h
> > +++ b/include/drm/drm_bridge.h
> > @@ -402,6 +402,10 @@ struct drm_bridge {
> >
> > void drm_bridge_add(struct drm_bridge *bridge);
> > void drm_bridge_remove(struct drm_bridge *bridge);
> > +void drm_bridge_init(struct drm_bridge *bridge, struct device *dev,
> > +const struct drm_bridge_funcs *funcs,
> > +const struct drm_bridge_timings *timings,
> > +void *driver_private);
> > struct drm_bridge *of_drm_find_bridge(struct device_node *np);
> > int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge
> > *bridge,
> > struct drm_bridge *previous);
>
>
--
Mihail
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 01/30] drm: Introduce drm_bridge_init()
On Tue, Nov 26, 2019 at 01:15:59PM +, Mihail Atanassov wrote:
> A simple convenience function to initialize the struct drm_bridge.
>
> Signed-off-by: Mihail Atanassov
The commit message here leaves figuring out why we need this to the
reader. Reading ahead the reasons seems to be to roll out bridge->dev
setting for everyone, so that we can set the device_link. Please explain
that in the commit message so the patch is properly motivated.
> ---
> drivers/gpu/drm/drm_bridge.c | 29 +
> include/drm/drm_bridge.h | 4
> 2 files changed, 33 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> index cba537c99e43..cbe680aa6eac 100644
> --- a/drivers/gpu/drm/drm_bridge.c
> +++ b/drivers/gpu/drm/drm_bridge.c
> @@ -89,6 +89,35 @@ void drm_bridge_remove(struct drm_bridge *bridge)
> }
> EXPORT_SYMBOL(drm_bridge_remove);
>
> +/**
> + * drm_bridge_init - initialise a drm_bridge structure
> + *
> + * @bridge: bridge control structure
> + * @funcs: control functions
> + * @dev: device
> + * @timings: timing specification for the bridge; optional (may be NULL)
> + * @driver_private: pointer to the bridge driver internal context (may be
> NULL)
Please also sprinkle some links to this new function to relevant places,
I'd add at least:
"Drivers should call drm_bridge_init() first." to the kerneldoc for
drm_bridge_add. drm_bridge_add should also mention drm_bridge_remove as
the undo function.
And perhaps a longer paragraph to &struct drm_bridge:
"Bridge drivers should call drm_bridge_init() to initialized a bridge
driver, and then register it with drm_bridge_add().
"Users of bridges link a bridge driver into their overall display output
pipeline by calling drm_bridge_attach()."
> + */
> +void drm_bridge_init(struct drm_bridge *bridge, struct device *dev,
> + const struct drm_bridge_funcs *funcs,
> + const struct drm_bridge_timings *timings,
> + void *driver_private)
> +{
> + WARN_ON(!funcs);
> +
> + bridge->dev = NULL;
Given that the goal here is to get bridge->dev set up, why not
WARN_ON(!dev);
bridge->dev = dev;
That should help us to really move forward with all this.
-Daniel
> + bridge->encoder = NULL;
> + bridge->next = NULL;
> +
> +#ifdef CONFIG_OF
> + bridge->of_node = dev->of_node;
> +#endif
> + bridge->timings = timings;
> + bridge->funcs = funcs;
> + bridge->driver_private = driver_private;
> +}
> +EXPORT_SYMBOL(drm_bridge_init);
> +
> /**
> * drm_bridge_attach - attach the bridge to an encoder's chain
> *
> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
> index c0a2286a81e9..d6d9d5301551 100644
> --- a/include/drm/drm_bridge.h
> +++ b/include/drm/drm_bridge.h
> @@ -402,6 +402,10 @@ struct drm_bridge {
>
> void drm_bridge_add(struct drm_bridge *bridge);
> void drm_bridge_remove(struct drm_bridge *bridge);
> +void drm_bridge_init(struct drm_bridge *bridge, struct device *dev,
> + const struct drm_bridge_funcs *funcs,
> + const struct drm_bridge_timings *timings,
> + void *driver_private);
> struct drm_bridge *of_drm_find_bridge(struct device_node *np);
> int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge,
> struct drm_bridge *previous);
> --
> 2.23.0
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 01/30] drm: Introduce drm_bridge_init()
A simple convenience function to initialize the struct drm_bridge.
Signed-off-by: Mihail Atanassov
---
drivers/gpu/drm/drm_bridge.c | 29 +
include/drm/drm_bridge.h | 4
2 files changed, 33 insertions(+)
diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
index cba537c99e43..cbe680aa6eac 100644
--- a/drivers/gpu/drm/drm_bridge.c
+++ b/drivers/gpu/drm/drm_bridge.c
@@ -89,6 +89,35 @@ void drm_bridge_remove(struct drm_bridge *bridge)
}
EXPORT_SYMBOL(drm_bridge_remove);
+/**
+ * drm_bridge_init - initialise a drm_bridge structure
+ *
+ * @bridge: bridge control structure
+ * @funcs: control functions
+ * @dev: device
+ * @timings: timing specification for the bridge; optional (may be NULL)
+ * @driver_private: pointer to the bridge driver internal context (may be NULL)
+ */
+void drm_bridge_init(struct drm_bridge *bridge, struct device *dev,
+const struct drm_bridge_funcs *funcs,
+const struct drm_bridge_timings *timings,
+void *driver_private)
+{
+ WARN_ON(!funcs);
+
+ bridge->dev = NULL;
+ bridge->encoder = NULL;
+ bridge->next = NULL;
+
+#ifdef CONFIG_OF
+ bridge->of_node = dev->of_node;
+#endif
+ bridge->timings = timings;
+ bridge->funcs = funcs;
+ bridge->driver_private = driver_private;
+}
+EXPORT_SYMBOL(drm_bridge_init);
+
/**
* drm_bridge_attach - attach the bridge to an encoder's chain
*
diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
index c0a2286a81e9..d6d9d5301551 100644
--- a/include/drm/drm_bridge.h
+++ b/include/drm/drm_bridge.h
@@ -402,6 +402,10 @@ struct drm_bridge {
void drm_bridge_add(struct drm_bridge *bridge);
void drm_bridge_remove(struct drm_bridge *bridge);
+void drm_bridge_init(struct drm_bridge *bridge, struct device *dev,
+const struct drm_bridge_funcs *funcs,
+const struct drm_bridge_timings *timings,
+void *driver_private);
struct drm_bridge *of_drm_find_bridge(struct device_node *np);
int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge,
struct drm_bridge *previous);
--
2.23.0
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
