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:

quoted

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:

quoted

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:

quoted

	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.

quoted

 * 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