Re: [PATCH net-next v1 1/3] led: trig: netdev: Fix requesting offload device

[Simon Horman <simon.horman@corigine.com>]

On Wed, Jun 21, 2023 at 05:19:01PM +0200, Andrew Lunn wrote:
> > >  			set_device_name(trigger_data, name, strlen(name));
> > >  			trigger_data->hw_control = true;
> > > -			trigger_data->mode = mode;
> > > +
> > > +			rc = led_cdev->hw_control_get(led_cdev, &mode);
> > > +			if (!rc)
> > > +				trigger_data->mode = mode;
> > 
> > Is the case where trigger_data->hw_control is set to true
> > but trigger_data->mode is not set ok?
> > 
> > I understand that is the whole point is not to return an error in this case.
> > But I'm concerned about the value of trigger_data->mode.
> 
> Yes, its something Christian and I talked about off-list.
> trigger_data->mode is 0 by default due to the kzalloc(). 0 is a valid
> value, it means don't blink for any reason. So in effect the LED
> should be off. And any LED driver which the ledtrig-netdev.c supports
> must support software control of the LED, so does support setting the
> LED off.
> 
> In the normal case hw_control_get() returns indicating the current
> blink mode, and the trigger sets its initial state to that. If
> however, it returns an error, it probably means its current state
> cannot be represented by the netdev trigger. PHY vendors do all sort
> of odd things, and we don't want to support all the craziness. So
> setting the LED off and leaving the user to configure the LED how they
> want seems like a reasonable thing to do.
> 
> And i tested this because my initial implementation of the Marvell
> driver was FUBAR and it returned an error here.

Thanks Andrew,

sounds good to me.
Especially,

	"we don't want to support all the craziness"

Reviewed-by: Simon Horman <simon.horman@corigine.com>