Date:   Mon, 20 Mar 2023 17:30:05 +0100
From:   Michal Kubiak <michal.kubiak@intel.com>
To:     Christian Marangi <ansuelsmth@gmail.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
Message-ID: <ZBiKDX/WJPfJey/+@localhost.localdomain>
References: <20230319191814.22067-1-ansuelsmth@gmail.com>
 <20230319191814.22067-3-ansuelsmth@gmail.com>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <20230319191814.22067-3-ansuelsmth@gmail.com>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?Xs7J2Y4b5yAF+gx+V/PgjyzjD77aINykeli7+eJvDwLm9wGhpnevy/gCn5Mz?=
 =?us-ascii?Q?pewSxcNoalnOVzzfLsGULWKRPjE9oty/2DeYZhTpApOu/plo/65CiaAE7vMF?=
 =?us-ascii?Q?RPl8dU7E7M23mxasRDHvGU4SRgktg9g5dBWpuUM+zHOLC+ekOamZ8lDJWuUe?=
 =?us-ascii?Q?eVWL1YEyTVu2T8dhKqhACs0D29kqEftbbw7EUc7qSNX0vxDJI5SH8lVdLLxW?=
 =?us-ascii?Q?ZuKIaiAwSTnrlSUEEPGxQGD4q4pRVb2ieHY5Knpr+amjnA2LU007t1y5/2Wx?=
 =?us-ascii?Q?Z5cSSoVPymIWJTT/eC3GqNEM+ZFb6NdPLMUGczPp07F0EBhpZEhPup1wZFGs?=
 =?us-ascii?Q?0qVLFnJtUJBvrg0VLveeTIbR15Pso8d2YPxPjpO1IC3ITwwNx1bhYrVvlqRx?=
 =?us-ascii?Q?kebAiXasIriPw6xyql7QJb7E0RiIA2viz5B2clnR39lhAyiKbSXa/KMzuE5p?=
 =?us-ascii?Q?ZI3gCH0hGqGl6nAMyjXPEW+vid7KxCGAojfsC7MZdSWWWWYSaobKpzyFvTfd?=
 =?us-ascii?Q?c9kGAhpTDezkk1BKY+p95QkUQcb8h3S5WunvBlUolzXm+g0VG8H2EHSW50yC?=
 =?us-ascii?Q?8+zT1+N5ykuvQzWjPiNQxkLQkqdPGsErZp8qcCslD9/flMn89CBvcINH4HrQ?=
 =?us-ascii?Q?0Y/C+fu3nDrAcWLjREU+ZHfbDlFry6lhAMJ5FePgqXBKiPEwFWwfrXkk9lRe?=
 =?us-ascii?Q?SBJKxhdE4gl8iZuo06q/6qUF/qOseDUWF01At06nFtz9tV5DnqKHL7mIh2IR?=
 =?us-ascii?Q?ynL4L3jrcFDdvdHQMCDzaAVa+QkD7sc8pLkMiPbYJoSfW0T1WH3LkxVGYCil?=
 =?us-ascii?Q?0bV0kBK4V0AYlwtPQ8LCDY7P9vckEG8Tcv+TpsDpsYVEiNpXZT6yDnfRxazA?=
 =?us-ascii?Q?VdZwRR56kyaPbcRyVpTiN6lXBQp/RCPYnDWhur+xRLiQVcOchc5q67GsvXAN?=
 =?us-ascii?Q?HdJwQBvKV3hMDuSLa6zHTJK8z4Fmm2RHgO/uAGO+h+fVgyVOiqzS0qVY52y9?=
 =?us-ascii?Q?EUeeKKMCMzbBdDJE+Q+t0x9grjc9i+n1zNVqLzSsVhDNZFXGyVkcxW3mTJfo?=
 =?us-ascii?Q?MHyEqJ6wMR34n7vMe0snhW5Z7w2Dt3lO6W/T+gkOAh+c73oOGsbcHZPJL34m?=
 =?us-ascii?Q?CVQ9mVAtcY2S1HmGAMhm4AguCQOfEUCT0sCNj5zBYUpwTurPBCSB0OREtuSX?=
 =?us-ascii?Q?hXwEIlQcQhQHC6bNGe4G/AmDNkFIc980SJMh5f4Ov8fJbbtbxtwVVJZyX8Tm?=
 =?us-ascii?Q?dxHL/xrfZF2X1uvcZwrQbqauVnXT176ppQjwBrciaP0eRVzx34KVgpmJLrLv?=
 =?us-ascii?Q?pRtrF17WyJQz+OtaVdZEJucDss8V3i9MSy0gabLlE7WEfMAgn0F2AR7Bh7xl?=
 =?us-ascii?Q?0GwBou2ULmjsQfjgqk9S67KpHJJU7Wu0SU80+Z0sxR8Bv2/0K0uNY0Eh2Ct+?=
 =?us-ascii?Q?clGXmL77/vTK0YjN7Ix6QBjwk8WCkCc9ne7VAmCiGIzbtuQeaAxrO0FRhYMO?=
 =?us-ascii?Q?SshfvRn9RH1E+j9qCta+VbieWTeb7zD5bm47J0F2ELiHU4ujI1LDNy2Vpy5q?=
 =?us-ascii?Q?jXLyvfe9Y82nKUDvSLtRH37qL/+cE4hqT5bqMhSFD+wIKnDZW74zFCmdd4c4?=
 =?us-ascii?Q?AQ=3D=3D?=
X-MS-Exchange-CrossTenant-Network-Message-Id: add06640-825a-4619-002c-08db29606507
X-MS-Exchange-CrossTenant-AuthSource: DM6PR11MB2937.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 20 Mar 2023 16:30:19.3957
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: neXp1FVLNF7qLbhBem/xKmPv6TTCxBEedwpknf/6oWfYRNfSeyB9oqvccqtvKAhMmsLWh3DDXxs1jE3uGd41jg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM6PR11MB4625
X-OriginatorOrg: intel.com
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

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@gmail.com>
> ---

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"