[PATCH v1 1/1] device property: Add const qualifier to device_get_match_data() parameter

[PATCH v1 1/1] device property: Add const qualifier to device_get_match_data() parameter

[Andy Shevchenko]

Add const qualifier to the device_get_match_data() parameter.
Some of the future users may utilize this function without
forcing the type.

All the same, dev_fwnode() may be used with a const qualifier.

Reported-by: kernel test robot <lkp@intel.com>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/base/property.c  | 4 ++--
 include/linux/property.h | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/base/property.c b/drivers/base/property.c
index ed6f449f8e5c..4d6278a84868 100644
--- a/drivers/base/property.c
+++ b/drivers/base/property.c
@@ -17,7 +17,7 @@
 #include <linux/property.h>
 #include <linux/phy.h>
 
-struct fwnode_handle *dev_fwnode(struct device *dev)
+struct fwnode_handle *dev_fwnode(const struct device *dev)
 {
 	return IS_ENABLED(CONFIG_OF) && dev->of_node ?
 		of_fwnode_handle(dev->of_node) : dev->fwnode;
@@ -1200,7 +1200,7 @@ int fwnode_graph_parse_endpoint(const struct fwnode_handle *fwnode,
 }
 EXPORT_SYMBOL(fwnode_graph_parse_endpoint);
 
-const void *device_get_match_data(struct device *dev)
+const void *device_get_match_data(const struct device *dev)
 {
 	return fwnode_call_ptr_op(dev_fwnode(dev), device_get_match_data, dev);
 }
diff --git a/include/linux/property.h b/include/linux/property.h
index a5b429d623f6..117cc200c656 100644
--- a/include/linux/property.h
+++ b/include/linux/property.h
@@ -32,7 +32,7 @@ enum dev_dma_attr {
 	DEV_DMA_COHERENT,
 };
 
-struct fwnode_handle *dev_fwnode(struct device *dev);
+struct fwnode_handle *dev_fwnode(const struct device *dev);
 
 bool device_property_present(struct device *dev, const char *propname);
 int device_property_read_u8_array(struct device *dev, const char *propname,
@@ -387,7 +387,7 @@ bool device_dma_supported(struct device *dev);
 
 enum dev_dma_attr device_get_dma_attr(struct device *dev);
 
-const void *device_get_match_data(struct device *dev);
+const void *device_get_match_data(const struct device *dev);
 
 int device_get_phy_mode(struct device *dev);
 int fwnode_get_phy_mode(struct fwnode_handle *fwnode);
-- 
2.35.1

Re: [PATCH v1 1/1] device property: Add const qualifier to device_get_match_data() parameter

[Heikki Krogerus]

On Thu, Sep 22, 2022 at 04:54:10PM +0300, Andy Shevchenko wrote:
> Add const qualifier to the device_get_match_data() parameter.
> Some of the future users may utilize this function without
> forcing the type.
> 
> All the same, dev_fwnode() may be used with a const qualifier.
> 
> Reported-by: kernel test robot <lkp@intel.com>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Acked-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>

> ---
>  drivers/base/property.c  | 4 ++--
>  include/linux/property.h | 4 ++--
>  2 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/base/property.c b/drivers/base/property.c
> index ed6f449f8e5c..4d6278a84868 100644
> --- a/drivers/base/property.c
> +++ b/drivers/base/property.c
> @@ -17,7 +17,7 @@
>  #include <linux/property.h>
>  #include <linux/phy.h>
>  
> -struct fwnode_handle *dev_fwnode(struct device *dev)
> +struct fwnode_handle *dev_fwnode(const struct device *dev)
>  {
>  	return IS_ENABLED(CONFIG_OF) && dev->of_node ?
>  		of_fwnode_handle(dev->of_node) : dev->fwnode;
> @@ -1200,7 +1200,7 @@ int fwnode_graph_parse_endpoint(const struct fwnode_handle *fwnode,
>  }
>  EXPORT_SYMBOL(fwnode_graph_parse_endpoint);
>  
> -const void *device_get_match_data(struct device *dev)
> +const void *device_get_match_data(const struct device *dev)
>  {
>  	return fwnode_call_ptr_op(dev_fwnode(dev), device_get_match_data, dev);
>  }
> diff --git a/include/linux/property.h b/include/linux/property.h
> index a5b429d623f6..117cc200c656 100644
> --- a/include/linux/property.h
> +++ b/include/linux/property.h
> @@ -32,7 +32,7 @@ enum dev_dma_attr {
>  	DEV_DMA_COHERENT,
>  };
>  
> -struct fwnode_handle *dev_fwnode(struct device *dev);
> +struct fwnode_handle *dev_fwnode(const struct device *dev);
>  
>  bool device_property_present(struct device *dev, const char *propname);
>  int device_property_read_u8_array(struct device *dev, const char *propname,
> @@ -387,7 +387,7 @@ bool device_dma_supported(struct device *dev);
>  
>  enum dev_dma_attr device_get_dma_attr(struct device *dev);
>  
> -const void *device_get_match_data(struct device *dev);
> +const void *device_get_match_data(const struct device *dev);
>  
>  int device_get_phy_mode(struct device *dev);
>  int fwnode_get_phy_mode(struct fwnode_handle *fwnode);
> -- 
> 2.35.1

-- 
heikki

Re: [PATCH v1 1/1] device property: Add const qualifier to device_get_match_data() parameter

[Sakari Ailus]

Hi Andy,

Thanks for cc'ing me.

On Thu, Sep 22, 2022 at 04:54:10PM +0300, Andy Shevchenko wrote:
> Add const qualifier to the device_get_match_data() parameter.
> Some of the future users may utilize this function without
> forcing the type.

From const to non-const? This is what this patch does, right?

> 
> All the same, dev_fwnode() may be used with a const qualifier.
> 
> Reported-by: kernel test robot <lkp@intel.com>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/base/property.c  | 4 ++--
>  include/linux/property.h | 4 ++--
>  2 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/base/property.c b/drivers/base/property.c
> index ed6f449f8e5c..4d6278a84868 100644
> --- a/drivers/base/property.c
> +++ b/drivers/base/property.c
> @@ -17,7 +17,7 @@
>  #include <linux/property.h>
>  #include <linux/phy.h>
>  
> -struct fwnode_handle *dev_fwnode(struct device *dev)
> +struct fwnode_handle *dev_fwnode(const struct device *dev)

If you have const struct device pointer, then the embedded fwnode handle in
that object sure is const, too. Isn't it?

If you really have const struct device pointer (where do you?), then I'd
suggest to add another function, dev_fwnode_const() that is otherwise the
same except the argument as well as the return value are const.

Or alternatively define it as a macro and use _Generic()?

>  {
>  	return IS_ENABLED(CONFIG_OF) && dev->of_node ?
>  		of_fwnode_handle(dev->of_node) : dev->fwnode;
> @@ -1200,7 +1200,7 @@ int fwnode_graph_parse_endpoint(const struct fwnode_handle *fwnode,
>  }
>  EXPORT_SYMBOL(fwnode_graph_parse_endpoint);
>  
> -const void *device_get_match_data(struct device *dev)
> +const void *device_get_match_data(const struct device *dev)
>  {
>  	return fwnode_call_ptr_op(dev_fwnode(dev), device_get_match_data, dev);
>  }
> diff --git a/include/linux/property.h b/include/linux/property.h
> index a5b429d623f6..117cc200c656 100644
> --- a/include/linux/property.h
> +++ b/include/linux/property.h
> @@ -32,7 +32,7 @@ enum dev_dma_attr {
>  	DEV_DMA_COHERENT,
>  };
>  
> -struct fwnode_handle *dev_fwnode(struct device *dev);
> +struct fwnode_handle *dev_fwnode(const struct device *dev);
>  
>  bool device_property_present(struct device *dev, const char *propname);
>  int device_property_read_u8_array(struct device *dev, const char *propname,
> @@ -387,7 +387,7 @@ bool device_dma_supported(struct device *dev);
>  
>  enum dev_dma_attr device_get_dma_attr(struct device *dev);
>  
> -const void *device_get_match_data(struct device *dev);
> +const void *device_get_match_data(const struct device *dev);
>  
>  int device_get_phy_mode(struct device *dev);
>  int fwnode_get_phy_mode(struct fwnode_handle *fwnode);

-- 
Regards,

Sakari Ailus

Re: [PATCH v1 1/1] device property: Add const qualifier to device_get_match_data() parameter

[Andy Shevchenko]

On Thu, Sep 22, 2022 at 08:22:54PM +0000, Sakari Ailus wrote:
> On Thu, Sep 22, 2022 at 04:54:10PM +0300, Andy Shevchenko wrote:
> > Add const qualifier to the device_get_match_data() parameter.
> > Some of the future users may utilize this function without
> > forcing the type.
> 
> From const to non-const? This is what this patch does, right?

Right.

> > All the same, dev_fwnode() may be used with a const qualifier.
> > 
> > Reported-by: kernel test robot <lkp@intel.com>
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

> > -struct fwnode_handle *dev_fwnode(struct device *dev)
> > +struct fwnode_handle *dev_fwnode(const struct device *dev)
> 
> If you have const struct device pointer, then the embedded fwnode handle in
> that object sure is const, too. Isn't it?
> 
> If you really have const struct device pointer (where do you?), then I'd

device_get_match_data() expects the const parameter, but due to dev_fwnode()
it can't be satisfied. This has been reported by LKP when I tried to submit
a wrapper:
https://lore.kernel.org/linux-spi/20220921204520.23984-1-andriy.shevchenko@linux.intel.com/

Yes, I probably can drop const there, but it will be again awkward to see
almost all APIs beneath using const and upper one without it for unclear
(to the reader) reasons.

> suggest to add another function, dev_fwnode_const() that is otherwise the
> same except the argument as well as the return value are const.

Perhaps this is the case, but does it mean we need to duplicate APIs when
we know we don't modify data? Seems road to bloating the code for peanuts.

> Or alternatively define it as a macro and use _Generic()?

Yeah, I have the mixed feelings about _Generic for generic APIs because
it requires to convert some stuff to macros when type checking of the
parameters can be missed (if a target takes two or more of them).

It might work here (we have a single parameter), but in general...

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 1/1] device property: Add const qualifier to device_get_match_data() parameter

[Sakari Ailus]

Hi Andy,

On Fri, Sep 23, 2022 at 01:25:42PM +0300, Andy Shevchenko wrote:
> On Thu, Sep 22, 2022 at 08:22:54PM +0000, Sakari Ailus wrote:
> > On Thu, Sep 22, 2022 at 04:54:10PM +0300, Andy Shevchenko wrote:
> > > Add const qualifier to the device_get_match_data() parameter.
> > > Some of the future users may utilize this function without
> > > forcing the type.
> > 
> > From const to non-const? This is what this patch does, right?
> 
> Right.
> 
> > > All the same, dev_fwnode() may be used with a const qualifier.
> > > 
> > > Reported-by: kernel test robot <lkp@intel.com>
> > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> 
> > > -struct fwnode_handle *dev_fwnode(struct device *dev)
> > > +struct fwnode_handle *dev_fwnode(const struct device *dev)
> > 
> > If you have const struct device pointer, then the embedded fwnode handle in
> > that object sure is const, too. Isn't it?
> > 
> > If you really have const struct device pointer (where do you?), then I'd
> 
> device_get_match_data() expects the const parameter, but due to dev_fwnode()
> it can't be satisfied. This has been reported by LKP when I tried to submit
> a wrapper:
> https://lore.kernel.org/linux-spi/20220921204520.23984-1-andriy.shevchenko@linux.intel.com/
> 
> Yes, I probably can drop const there, but it will be again awkward to see
> almost all APIs beneath using const and upper one without it for unclear
> (to the reader) reasons.

dev could indeed be const there otherwise, I understand, but it's certainly
not better to force it non-const elsewhere for that.

> 
> > suggest to add another function, dev_fwnode_const() that is otherwise the
> > same except the argument as well as the return value are const.
> 
> Perhaps this is the case, but does it mean we need to duplicate APIs when
> we know we don't modify data? Seems road to bloating the code for peanuts.
> 
> > Or alternatively define it as a macro and use _Generic()?
> 
> Yeah, I have the mixed feelings about _Generic for generic APIs because
> it requires to convert some stuff to macros when type checking of the
> parameters can be missed (if a target takes two or more of them).

It's not uncommon to use wrapper functions in addition to get type checking
done properly.

> 
> It might work here (we have a single parameter), but in general...

If it works here, why not to do it then? :-)

-- 
Sakari Ailus