Re: [PATCH RFC net-next 3/7] net: dsa: use fwnode_get_phy_mode() to get phy interface mode

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu, Mar 23, 2023 at 02:49:01PM +0000, Russell King (Oracle) wrote:
> On Thu, Mar 23, 2023 at 04:38:29PM +0200, Andy Shevchenko wrote:
> > On Thu, Mar 23, 2023 at 02:31:04PM +0000, Russell King (Oracle) wrote:
> > > On Thu, Mar 23, 2023 at 04:03:05PM +0200, Andy Shevchenko wrote:
> > > > On Wed, Mar 22, 2023 at 12:00:06PM +0000, Russell King (Oracle) wrote:

...

> > > > > +	struct fwnode_handle *fwnode;
> > > > 
> > > > > +	fwnode = of_fwnode_handle(dp->dn);
> > > > 
> > > > 	const struct fwnode_handle *fwnode = of_fwnode_handle(dp->dn);
> > > > 
> > > > ?
> > > 
> > > Why const?
> > 
> > Do you modify its content on the fly?
> 
> Do you want to litter code with casts to get rid of the const?
> 
> > For fwnode as a basic object type we want to reduce the scope of the possible
> > modifications. If you don't modify and APIs you call do not require non-const
> > object, use const for fwnode.
> 
> Let's start here. We pass this fwnode to fwnode_get_phy_mode():
> 
> include/linux/property.h:int fwnode_get_phy_mode(struct fwnode_handle *fwnode);
> 
> Does fwnode_get_phy_mode() alter the contents of the fwnode? Probably
> not, but it doesn't take a const pointer. Therefore, to declare my
> fwnode as const, I'd need to cast the const-ness away before calling
> this.

So, fix the fwnode_get_phy_mode(). Is it a problem?

> Then there's phylink_create(). Same problem.

So, fix that. Is it a problem?

> So NAK to this const - until such time that we have a concerted effort
> to making functions we call which do not modify the "fwnode" argument
> constify that argument. Otherwise it's just rediculously crazy to
> declare a variable const only to then litter the code with casts to get
> rid of it at every call site.
> 
> Please do a bit of research before making suggestions. Thanks.

So, MAK to your patch. You can fix that, and you know that.

P.S. Please, move that phy thingy away from property.h, it doesn't belong
there.

-- 
With Best Regards,
Andy Shevchenko





[Index of Archives]     [Linux IBM ACPI]     [Linux Power Management]     [Linux Kernel]     [Linux Laptop]     [Kernel Newbies]     [Share Photos]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Video 4 Linux]     [Device Mapper]     [Linux Resources]
  Powered by Linux