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

[Christian Marangi]

On Fri, Mar 10, 2023 at 01:12:03AM +0100, Andrew Lunn wrote:
> > +config NET_DSA_QCA8K_LEDS_SUPPORT
> > +	tristate "Qualcomm Atheros QCA8K Ethernet switch family LEDs support"
> 
> Is tristate correct here? That means the code can either be built in,
> a module, or not built at all. Is that what you want?
> 
> It seems more normal to use a bool, not a tristate.
>

Think you are right, can't really be a module.

> > +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?
> 
>        Andrew

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?

-- 
	Ansuel