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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <640a7775.5d0a0220.110eb.3e41@mx.google.com>
Date:   Fri, 10 Mar 2023 01:18:48 +0100
From:   Christian Marangi <ansuelsmth@...il.com>
To:     Andrew Lunn <andrew@...n.ch>
Cc:     Florian Fainelli <f.fainelli@...il.com>,
        Vladimir Oltean <olteanv@...il.com>,
        "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Heiner Kallweit <hkallweit1@...il.com>,
        Russell King <linux@...linux.org.uk>,
        Gregory Clement <gregory.clement@...tlin.com>,
        Sebastian Hesselbarth <sebastian.hesselbarth@...il.com>,
        Andy Gross <agross@...nel.org>,
        Bjorn Andersson <andersson@...nel.org>,
        Konrad Dybcio <konrad.dybcio@...aro.org>,
        John Crispin <john@...ozen.org>, netdev@...r.kernel.org,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org,
        linux-arm-msm@...r.kernel.org, Lee Jones <lee@...nel.org>,
        linux-leds@...r.kernel.org
Subject: Re: [net-next PATCH v2 02/14] net: dsa: qca8k: add LEDs basic support

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