| Message ID | 20210929084234.1271915-2-maxime@cerno.tech (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/sun4i: Add support for dual-link LVDS on the A20 | expand |
Hi Maxime, Thank you for the patch. On Wed, Sep 29, 2021 at 10:42:28AM +0200, Maxime Ripard wrote: > of_graph_get_port_by_id doesn't modify the device_node pointer it takes > as argument, so we can make it const. From a C point of view that's right, but conceptually speaking, is it right to return a non-const child port node of a const device_node ? > Signed-off-by: Maxime Ripard <maxime@cerno.tech> > --- > drivers/of/property.c | 2 +- > include/linux/of_graph.h | 4 ++-- > 2 files changed, 3 insertions(+), 3 deletions(-) > > diff --git a/drivers/of/property.c b/drivers/of/property.c > index 3fd74bb34819..739d5d1c8c3a 100644 > --- a/drivers/of/property.c > +++ b/drivers/of/property.c > @@ -603,7 +603,7 @@ EXPORT_SYMBOL(of_graph_parse_endpoint); > * Return: A 'port' node pointer with refcount incremented. The caller > * has to use of_node_put() on it when done. > */ > -struct device_node *of_graph_get_port_by_id(struct device_node *parent, u32 id) > +struct device_node *of_graph_get_port_by_id(const struct device_node *parent, u32 id) > { > struct device_node *node, *port; > > diff --git a/include/linux/of_graph.h b/include/linux/of_graph.h > index 4d7756087b6b..87f3f4d0d0df 100644 > --- a/include/linux/of_graph.h > +++ b/include/linux/of_graph.h > @@ -42,7 +42,7 @@ bool of_graph_is_present(const struct device_node *node); > int of_graph_parse_endpoint(const struct device_node *node, > struct of_endpoint *endpoint); > int of_graph_get_endpoint_count(const struct device_node *np); > -struct device_node *of_graph_get_port_by_id(struct device_node *node, u32 id); > +struct device_node *of_graph_get_port_by_id(const struct device_node *node, u32 id); > struct device_node *of_graph_get_next_endpoint(const struct device_node *parent, > struct device_node *previous); > struct device_node *of_graph_get_endpoint_by_regs( > @@ -74,7 +74,7 @@ static inline int of_graph_get_endpoint_count(const struct device_node *np) > } > > static inline struct device_node *of_graph_get_port_by_id( > - struct device_node *node, u32 id) > + const struct device_node *node, u32 id) > { > return NULL; > }
On Tue, Oct 19, 2021 at 03:02:13PM +0300, Laurent Pinchart wrote: > Hi Maxime, > > Thank you for the patch. > > On Wed, Sep 29, 2021 at 10:42:28AM +0200, Maxime Ripard wrote: > > of_graph_get_port_by_id doesn't modify the device_node pointer it takes > > as argument, so we can make it const. > > From a C point of view that's right, but conceptually speaking, is it > right to return a non-const child port node of a const device_node ? I mean, I guess not, but you're the one that asked for it: https://lore.kernel.org/dri-devel/YBAiztjg0Jji9voK@pendragon.ideasonboard.com/ I can change it if you want, but certainly not if the only comment I get on this series for the next year is going to be over whether or not arguments of functions unrelated to the main intent should be constified or not. Maxime
Hi Maxime, On Thu, Oct 21, 2021 at 09:48:43AM +0200, Maxime Ripard wrote: > On Tue, Oct 19, 2021 at 03:02:13PM +0300, Laurent Pinchart wrote: > > On Wed, Sep 29, 2021 at 10:42:28AM +0200, Maxime Ripard wrote: > > > of_graph_get_port_by_id doesn't modify the device_node pointer it takes > > > as argument, so we can make it const. > > > > From a C point of view that's right, but conceptually speaking, is it > > right to return a non-const child port node of a const device_node ? > > I mean, I guess not, but you're the one that asked for it: > https://lore.kernel.org/dri-devel/YBAiztjg0Jji9voK@pendragon.ideasonboard.com/ Oops. Looks like I must be wrong with at least one of the two comments. Please pick the option you like best. > I can change it if you want, but certainly not if the only comment I get > on this series for the next year is going to be over whether or not > arguments of functions unrelated to the main intent should be constified > or not. DRM/KMS seems to suffer from a deficit of reviewers in all non-desktop areas :-(
diff --git a/drivers/of/property.c b/drivers/of/property.c index 3fd74bb34819..739d5d1c8c3a 100644 --- a/drivers/of/property.c +++ b/drivers/of/property.c @@ -603,7 +603,7 @@ EXPORT_SYMBOL(of_graph_parse_endpoint); * Return: A 'port' node pointer with refcount incremented. The caller * has to use of_node_put() on it when done. */ -struct device_node *of_graph_get_port_by_id(struct device_node *parent, u32 id) +struct device_node *of_graph_get_port_by_id(const struct device_node *parent, u32 id) { struct device_node *node, *port; diff --git a/include/linux/of_graph.h b/include/linux/of_graph.h index 4d7756087b6b..87f3f4d0d0df 100644 --- a/include/linux/of_graph.h +++ b/include/linux/of_graph.h @@ -42,7 +42,7 @@ bool of_graph_is_present(const struct device_node *node); int of_graph_parse_endpoint(const struct device_node *node, struct of_endpoint *endpoint); int of_graph_get_endpoint_count(const struct device_node *np); -struct device_node *of_graph_get_port_by_id(struct device_node *node, u32 id); +struct device_node *of_graph_get_port_by_id(const struct device_node *node, u32 id); struct device_node *of_graph_get_next_endpoint(const struct device_node *parent, struct device_node *previous); struct device_node *of_graph_get_endpoint_by_regs( @@ -74,7 +74,7 @@ static inline int of_graph_get_endpoint_count(const struct device_node *np) } static inline struct device_node *of_graph_get_port_by_id( - struct device_node *node, u32 id) + const struct device_node *node, u32 id) { return NULL; }
of_graph_get_port_by_id doesn't modify the device_node pointer it takes as argument, so we can make it const. Signed-off-by: Maxime Ripard <maxime@cerno.tech> --- drivers/of/property.c | 2 +- include/linux/of_graph.h | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-)