[net-next PATCH v2 02/14] net: dsa: qca8k: add LEDs basic support

[Andrew Lunn]

> > > +static enum led_brightness
> > > +qca8k_led_brightness_get(struct qca8k_led *led)
> > > +{
> > > +	struct qca8k_led_pattern_en reg_info;
> > > +	struct qca8k_priv *priv = led->priv;
> > > +	u32 val;
> > > +	int ret;
> > > +
> > > +	qca8k_get_enable_led_reg(led->port_num, led->led_num, &reg_info);
> > > +
> > > +	ret = regmap_read(priv->regmap, reg_info.reg, &val);
> > > +	if (ret)
> > > +		return 0;
> > > +
> > > +	val >>= reg_info.shift;
> > > +
> > > +	if (led->port_num == 0 || led->port_num == 4) {
> > > +		val &= QCA8K_LED_PATTERN_EN_MASK;
> > > +		val >>= QCA8K_LED_PATTERN_EN_SHIFT;
> > > +	} else {
> > > +		val &= QCA8K_LED_PHY123_PATTERN_EN_MASK;
> > > +	}
> > > +
> > > +	return val > 0 ? 1 : 0;
> > > +}
> > 
> > What will this return when in the future you add hardware offload, and
> > the LED is actually blinking because of frames being sent etc?
> > 
> > Is it better to not implement _get() when it is unclear what it should
> > return when offload is in operation?
> > 
> 
> My idea was that anything that is not 'always off' will have brightness
> 1. So also in accelerated blink brightness should be 1.
> 
> My idea of get was that it should reflect if the led is active or always
> off. Is it wrong?
 
brigntness_get seems to be used in two situations:

When the LED is first registered, it can be called to get the current
state of the LED. This then initialized cdev->brightness.

When the brightness sysfs file is read, there is first a call to
brightness_get to allow it to update the value in cdev->brightness
before returning the value in the read.

I think always returning 1 could be confusing. Take the example that
the LED is indicating link, there is no link, so it is off. Yet a read
of the brightness sysfs file will return 1?

I would say, it either needs to return the instantaneous brightness,
or it should not be implemented at all. When we come to implement
offloading, we might want to consider hiding the brightness sysfs
file. But we can solve that later.

      Andrew