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@xxxxxxxxx>
> > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
> 
> > > -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@xxxxxxxxxxxxxxx/
> 
> 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