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
