Re: [PATCH net-next v0 2/3] net: phy: phy_device: Call into the PHY driver to set LED offload

[Simon Horman <horms@kernel.org>]

On Mon, Jun 19, 2023 at 07:11:04PM +0100, Russell King (Oracle) wrote:
> On Mon, Jun 19, 2023 at 04:18:29PM +0200, Simon Horman wrote:
> > On Sun, Jun 18, 2023 at 07:39:36PM +0200, Andrew Lunn wrote:
> > > Linux LEDs can be requested to perform hardware accelerated blinking
> > > to indicate link, RX, TX etc. Pass the rules for blinking to the PHY
> > > driver, if it implements the ops needed to determine if a given
> > > pattern can be offloaded, to offload it, and what the current offload
> > > is. Additionally implement the op needed to get what device the LED is
> > > for.
> > > 
> > > Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> > 
> > ...
> > 
> > > diff --git a/include/linux/phy.h b/include/linux/phy.h
> > > index 11c1e91563d4..1db63fb905c5 100644
> > > --- a/include/linux/phy.h
> > > +++ b/include/linux/phy.h
> > > @@ -1104,6 +1104,20 @@ struct phy_driver {
> > >  	int (*led_blink_set)(struct phy_device *dev, u8 index,
> > >  			     unsigned long *delay_on,
> > >  			     unsigned long *delay_off);
> > > +	/* Can the HW support the given rules. Return 0 if yes,
> > > +	 * -EOPNOTSUPP if not, or an error code.
> > > +	 */
> > > +	int (*led_hw_is_supported)(struct phy_device *dev, u8 index,
> > > +				   unsigned long rules);
> > > +	/* Set the HW to control the LED as described by rules. */
> > > +	int (*led_hw_control_set)(struct phy_device *dev, u8 index,
> > > +				  unsigned long rules);
> > > +	/* Get the rules used to describe how the HW is currently
> > > +	 * configure.
> > > +	 */
> > > +	int (*led_hw_control_get)(struct phy_device *dev, u8 index,
> > > +				  unsigned long *rules);
> > > +
> > 
> > Hi Andrew,
> > 
> > for consistency it would be nice if the comments for
> > the new members above was in kernel doc format.
> 
> Unfortunately, kerneldoc doesn't understand structures-of-function-
> pointers, so one can't document each operation and its parameters
> without playing games such as I've done in linux/phylink.h. It involves
> listing the prototypes not as function pointers but as normal function
> prototypes in a #if 0..#endif section and preceeding each with a
> kerneldoc comment describing the function and its parameters in the
> normal way.

Ok. I'll confess that I wasn't aware of that problem.
But could we use one of the approaches approach already taken
for existing members of this structure?

e.g.:

        /**
         * @led_blink_set: Set a PHY LED brightness.  Index indicates
         * which of the PHYs led should be configured to blink. Delays
         * are in milliseconds and if both are zero then a sensible
         * default should be chosen.  The call should adjust the
         * timings in that case and if it can't match the values
         * specified exactly.
         */
        int (*led_blink_set)(struct phy_device *dev, u8 index,
                             unsigned long *delay_on,
                             unsigned long *delay_off);

Or the more minimalist approach:

        /** @get_plca_status: Return the current PLCA status info */
        int (*get_plca_status)(struct phy_device *dev,
                               struct phy_plca_status *plca_st);

I was going to say to be consistent. But the above aren't consistent with
each other.  I guess that I feel something is better than nothing.  But if
you think otherwise then let's let it rest.