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

[Michal Kubiak <michal.kubiak@intel.com>]

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()"?

Thanks,
Michal