Re: [net-next PATCH v12 09/13] mfd: an8855: Add support for Airoha AN8855 Switch MFD

[Andrew Lunn <andrew@lunn.ch>]

> > +static int an8855_mii_set_page(struct an8855_mfd_priv *priv, u8 phy_id,
> > +			       u8 page) __must_hold(&priv->bus->mdio_lock)
> > +{
> > +	struct mii_bus *bus = priv->bus;
> > +	int ret;
> > +
> > +	ret = __mdiobus_write(bus, phy_id, AN8855_PHY_SELECT_PAGE, page);
> 
> Calling functions with '__' is a red flag.

In this case, it is correct. It was probably a bad decision to call
this __mdiobus_write() in the first place, but we were not expecting
it to be used in real driver code, just in core code, where calls to
it are wrapped with a mutex lock/unlock.

But drivers started to need to perform multiple read/write operations
in an atomic sequence, so they started doing there own taking of the
lock, and using these unlocked low level helpers.

We should probably rename __mdiobus_write() to _mdiobus_write() to
avoid compiler namespace issues.

> > +	if (ret < 0)
> > +		dev_err_ratelimited(&bus->dev,
> > +				    "failed to set an8855 mii page\n");
> 
> Use 100-chars if it avoids these kind of line breaks.

I _guess_ the code is following networking coding style, even though
it is outside of normal networking directories. netdev keeps with the
old 80 limit.

> > +static int an8855_regmap_read(void *ctx, uint32_t reg, uint32_t *val)
> > +{
> > +	struct an8855_mfd_priv *priv = ctx;
> > +	struct mii_bus *bus = priv->bus;
> > +	u16 addr = priv->switch_addr;
> > +	int ret;
> > +
> > +	mutex_lock_nested(&bus->mdio_lock, MDIO_MUTEX_NESTED);
> 
> guard()?

netdev people consider guard() too magical. scoped_guard() is however
O.K.

> > +	return ret < 0 ? ret : 0;
> 
> Doesn't the caller already expect possible >0 results?

It should not happen. Networking has a very small number of functions
which do return 1 on a different sort of success. Such functions are
generally called foo_changed(), e.g. mdiobus_modify_changed(). This
will do a read/modify/write. If it did not need to modify anything,
because of the current value, it will return 0. If something did
actually change, it returns 1. If something did actually change, you
sometimes need to take further actions. But many callers don't care,
so we have wrappers e.g. mdiobus_modify() calls
mdiobus_modify_changed() and then converts 0 or 1 to just 0. I'm
guessing this code copied such a helper when it should not of done.

	Andrew