linux-kernel - Re: [net-next PATCH v5 02/15] 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]

Date:   Mon, 20 Mar 2023 19:05:54 +0100
From:   Christian Marangi <ansuelsmth@...il.com>
To:     Michal Kubiak <michal.kubiak@...el.com>
Cc:     Andrew Lunn <andrew@...n.ch>,
        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>,
        Pavel Machek <pavel@....cz>, Lee Jones <lee@...nel.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, linux-leds@...r.kernel.org
Subject: Re: [net-next PATCH v5 02/15] net: dsa: qca8k: add LEDs basic support

On Mon, Mar 20, 2023 at 06:48:51PM +0100, Michal Kubiak wrote:
> On Mon, Mar 20, 2023 at 05:33:56PM +0100, Christian Marangi wrote:
> > 
> > Btw ok for the description of the LED mapping? It's a bit complex so
> > tried to do my best to describe them.
> > 
> 
> Yes, now it is much easier to understand the logic behind LED mapping.
> Thanks for adding that! I think it will save some time for anyone who
> will be working with that code in the future.
> 
> The only thing I still do not understand is the initial 14 bit shift:
> 
> >	if (led->port_num == 0 || led->port_num == 4) {
> >		mask = QCA8K_LED_PATTERN_EN_MASK;
> >		val <<= QCA8K_LED_PATTERN_EN_SHIFT;
> 
> For example, according to the code above, for port 4:
> 	- the value is shifted by 14 bits - to bits (15,14)
> 	- mask is also set to bits (15,14)
> 	- then, both mask and value are shifted again by 16 bits:
> 
> >		return regmap_update_bits(priv->regmap, reg_info.reg,
> >					  mask << reg_info.shift,
> >					  val << reg_info.shift);
> 
> because reg_info.shift == QCA8K_LED_PHY4_CONTROL_RULE_SHIFT == 16 for
> port_num == 4.
> 
> It means, in fact, for controlling port 4 we use bits (31,30) which
> seems to be inconsistent with your comment below.
> 
> >	 * To control port 4:
> >	 * - the 2 bit (17, 16) of:
> >	 *   - QCA8K_LED_CTRL0_REG for led1
> >	 *   - QCA8K_LED_CTRL1_REG for led2
> >	 *   - QCA8K_LED_CTRL2_REG for led3
> >	 *
> 
> Are values for ports 0 and 4 correct in your description in
> "qca8k_led_brightness_set()"?
> 

Code is correct, comment is not.

QCA8K_LED_CTRL0_REG is split in 2 part.
- first 16 bit for phy0
- second part (31, 16) for phy4

In these 16 half there are the bit that control the hw control blink
rules AND on the last 2 part of the half, the bit that control the state
of the LED (off, on, always-blink, hw control)

So I just didn't add on top of that MASK the required shift for
QCA8K_LED_PATTERN_EN_SHIFT.

so for phy0

GENMASK(1, 0) << QCA8K_LED_PATTERN_EN_SHIFT << QCA8K_LED_PHY0123_CONTROL_RULE_SHIFT
GENMASK(1, 0) << 14 << 0 

for phy4

GENMASK(1, 0) << QCA8K_LED_PATTERN_EN_SHIFT << QCA8K_LED_PHY4_CONTROL_RULE_SHIFT
GENMASK(1, 0) << 14 << 16

Thanks for the other review tag, will fix the last bit in v6.

-- 
	Ansuel