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

[Christian Marangi]

On Mon, Mar 20, 2023 at 05:30:05PM +0100, Michal Kubiak wrote:
> On Sun, Mar 19, 2023 at 08:18:01PM +0100, Christian Marangi wrote:
> > Add LEDs basic support for qca8k Switch Family by adding basic
> > brightness_set() support.
> > 
> > Since these LEDs refelect port status, the default label is set to
> > ":port". DT binding should describe the color, function and number of
> > the leds using standard LEDs api.
> > 
> > These LEDs supports only blocking variant of the brightness_set()
> > function since they can sleep during access of the switch leds to set
> > the brightness.
> > 
> > While at it add to the qca8k header file each mode defined by the Switch
> > Documentation for future use.
> > 
> > Signed-off-by: Christian Marangi <ansuelsmth@xxxxxxxxx>
> > ---
> 
> Hi Christian,
> 
> The patch looks good to me. I just found one nitpick in the comment.
> 
> Thanks,
> Michal
> 
> > +static int
> > +qca8k_parse_port_leds(struct qca8k_priv *priv, struct fwnode_handle *port, int port_num)
> > +{
> > +	struct fwnode_handle *led = NULL, *leds = NULL;
> > +	struct led_init_data init_data = { };
> > +	enum led_default_state state;
> > +	struct qca8k_led *port_led;
> > +	int led_num, led_index;
> > +	int ret;
> > +
> > +	leds = fwnode_get_named_child_node(port, "leds");
> > +	if (!leds) {
> > +		dev_dbg(priv->dev, "No Leds node specified in device tree for port %d!\n",
> > +			port_num);
> > +		return 0;
> > +	}
> > +
> > +	fwnode_for_each_child_node(leds, led) {
> > +		/* Reg represent the led number of the port.
> > +		 * Each port can have at least 3 leds attached
> 
> Nitpick: "at least" -> "at most"
>

Oh god... I added the extra comment and totally missed the other.
Sorry!

Btw ok for the description of the LED mapping? It's a bit complex so
tried to do my best to describe them.

-- 
	Ansuel