Message-ID: <6418a084.1c0a0220.de9c1.53e4@mx.google.com>
Date:   Mon, 20 Mar 2023 19:05:54 +0100
From:   Christian Marangi <ansuelsmth@gmail.com>
To:     Michal Kubiak <michal.kubiak@intel.com>
Cc:     Andrew Lunn <andrew@lunn.ch>,
        Florian Fainelli <f.fainelli@gmail.com>,
        Vladimir Oltean <olteanv@gmail.com>,
        "David S. Miller" <davem@davemloft.net>,
        Eric Dumazet <edumazet@google.com>,
        Jakub Kicinski <kuba@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>,
        Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Heiner Kallweit <hkallweit1@gmail.com>,
        Russell King <linux@armlinux.org.uk>,
        Gregory Clement <gregory.clement@bootlin.com>,
        Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>,
        Andy Gross <agross@kernel.org>,
        Bjorn Andersson <andersson@kernel.org>,
        Konrad Dybcio <konrad.dybcio@linaro.org>,
        Pavel Machek <pavel@ucw.cz>, Lee Jones <lee@kernel.org>,
        John Crispin <john@phrozen.org>, netdev@vger.kernel.org,
        devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-arm-kernel@lists.infradead.org,
        linux-arm-msm@vger.kernel.org, linux-leds@vger.kernel.org
Subject: Re: [net-next PATCH v5 02/15] net: dsa: qca8k: add LEDs basic support
References: <20230319191814.22067-1-ansuelsmth@gmail.com>
 <20230319191814.22067-3-ansuelsmth@gmail.com>
 <ZBiKDX/WJPfJey/+@localhost.localdomain>
 <64188af6.050a0220.c5fe1.1d96@mx.google.com>
 <ZBicg28JwVLugzqz@localhost.localdomain>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <ZBicg28JwVLugzqz@localhost.localdomain>
Precedence: bulk

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