Date:   Fri, 17 Mar 2023 12:54:09 +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>,
        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>, "Lee Jones" <lee@kernel.org>,
        <linux-leds@vger.kernel.org>
Subject: Re: [net-next PATCH v4 03/14] net: dsa: qca8k: add LEDs blink_set()
 support
Message-ID: <ZBRU4Xx3kCwbD3Eg@localhost.localdomain>
References: <20230317023125.486-1-ansuelsmth@gmail.com>
 <20230317023125.486-4-ansuelsmth@gmail.com>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <20230317023125.486-4-ansuelsmth@gmail.com>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?OsylApcdvDoCEgyh+ne0gcDw8RGHI8Nhk1q2P5ytg1UMBFtnHyUVTO3a10mO?=
 =?us-ascii?Q?ukJct75Z61p//RjYw0qi2GDjPji3adpYPTnRV1Je2Dcfof4KUm3zNsvXFE6n?=
 =?us-ascii?Q?7P2zykuk8AJkC+mUix93yDDSAdM73pzZfMkNMrBIUbLGP1CFqxWxOXilmlTP?=
 =?us-ascii?Q?5C4r31DD0sziPeXqaFDEg3nKJorpLKO3moeAVTavsN00gse0hOSdKki4NNIk?=
 =?us-ascii?Q?kylj5CSKFqhC8XxGopUajNDP70T859ComvT/zw4k3vmJWPPjZRXnVu1hDpHv?=
 =?us-ascii?Q?gR00DO7GgGjriIcsTvCKf83wrgRboU1pUOF9PjS7x1aOX3PU/Xzzjo104i5v?=
 =?us-ascii?Q?B6Ql5BDifiwX2d3s2/39ZKsIeVKN1hU6YSELzAI/fyVFFOcD8rJA9oapu3dP?=
 =?us-ascii?Q?YnXvbQ+V6HXcwjfXs1bO6mV3b9ibLdgKFDFeb37RMdPWF5wmBzVsud73Yawt?=
 =?us-ascii?Q?bpK1FPkFGm2WDZ8JmrqK3WEr+hlLqba2CDpjwr15yIPlrYMO0rI75nbcQS+3?=
 =?us-ascii?Q?MnS4DCTQEl6iPMOMsSRJt/lqfKAozh5f3klhnyyE7xCHbpucU1GngTy5px/K?=
 =?us-ascii?Q?I5FoMBdW+8r5r9TDTlOW2bzSzreRfU3QE02ujifL0XYzJ+cK5GTdMPuYA2IK?=
 =?us-ascii?Q?1h9T34cAg/YdtRUOJRI6fQo5sWlTfGG9zZi7TVP1eQRM3saDoPjzqsOdF9LA?=
 =?us-ascii?Q?KN9ODTar8M0BQxSjI1lGACaw1puaALngGGF3+EjExoMwdSOz0qOs5EiHLYuJ?=
 =?us-ascii?Q?2Sz75WIbk5sN/e+snIdM1ovw3EDAm+5w7N3Kjb0kcE63AQQbDPdlkZdZUI+n?=
 =?us-ascii?Q?lX35vLwEriB4TXN+lHzkqCk59W2ciwS9BmHT1gxpQfo4k2cA8hTEoiVdEAxE?=
 =?us-ascii?Q?nH6nDS4/eQXvpZlaensfYaxiOsAjq5wsctgBxgQVi35Wmym1j/jLz9mzuvC9?=
 =?us-ascii?Q?3xe/ibo8kaAsofa7+5N7K5vIqPR6D9G8Uc58u1OkJtxhGEHAcL/UcGVHYG/G?=
 =?us-ascii?Q?fwqWOP+dwRzLgParhMCoShUvC7UKLgm2OuXXrJQtMNnAdobr3iXWhB0+1D6k?=
 =?us-ascii?Q?DSzgWfw4nh4+aY0MV9GX1/ehphGaVKVXuivNsOYjeziWqb6PkSbaut9mGcGI?=
 =?us-ascii?Q?9LIfLFw+VpD3SH4OV6I5TQOJbHMQXcXq8DxXdApVh7qBFvnV9qgeneCNt0kx?=
 =?us-ascii?Q?cQBYXwQnbrUlOjLwzzq5z62Oz8GfCQu4l2Dyd6LEHaeVUIciP7nChUrvBl5O?=
 =?us-ascii?Q?0boaQGdYZvyOBYlSb64QwJDWd4Kua1gaj2zz/3gUr6cAqagr5tSXFwJF4svk?=
 =?us-ascii?Q?Yl/7c6+R7ukCdejdkNgNj32KG4OMJAApJbUCoENFMIYCMtG1emOQ84zKXj/o?=
 =?us-ascii?Q?QlrjfDzL14/uk7ochq9CNpkHR0PNCqKP9zNGNlSPsVDybsGEkmDVFLwXQ8LI?=
 =?us-ascii?Q?0pHvSZXRTFj1j6kGPHnc0sh3/RSNR8GV/gGwzTlouOUfcgfGQHeSI5Oum+kw?=
 =?us-ascii?Q?avkor01+Eyr4TsmqBa/OycIl2JRhcZvt4Hdk5aWA6PIbKkH5t7xwV0zHYNVc?=
 =?us-ascii?Q?Apo3cMojwiyxrMygulHp85fLM6ftgNCFfifc0rBO/DILExNEaSlbpYTn6QZD?=
 =?us-ascii?Q?7Q=3D=3D?=
X-MS-Exchange-CrossTenant-Network-Message-Id: b736aefe-eb35-485c-72cd-08db26de5d2d
X-MS-Exchange-CrossTenant-AuthSource: DM6PR11MB2937.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 17 Mar 2023 11:54:29.2474
 (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: 5iDdR1/Z9Ju0X/I6PQBKISXCSOkQr+9JuA7jK00f5NenLzT3l7BWg82b8lXgSYaNplHjPMZ9aJ6hZ9KkNv2b9Q==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SA3PR11MB7979
X-OriginatorOrg: intel.com
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Fri, Mar 17, 2023 at 03:31:14AM +0100, Christian Marangi wrote:
> Add LEDs blink_set() support to qca8k Switch Family.
> These LEDs support hw accellerated blinking at a fixed rate
> of 4Hz.
> 
> Reject any other value since not supported by the LEDs switch.
> 
> Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> Reviewed-by: Andrew Lunn <andrew@lunn.ch>
> ---
>  drivers/net/dsa/qca/qca8k-leds.c | 38 ++++++++++++++++++++++++++++++++
>  1 file changed, 38 insertions(+)
> 
> diff --git a/drivers/net/dsa/qca/qca8k-leds.c b/drivers/net/dsa/qca/qca8k-leds.c
> index adbe7f6e2994..c229575c7e8c 100644
> --- a/drivers/net/dsa/qca/qca8k-leds.c
> +++ b/drivers/net/dsa/qca/qca8k-leds.c
> @@ -92,6 +92,43 @@ qca8k_led_brightness_get(struct qca8k_led *led)
>  	return val == QCA8K_LED_ALWAYS_ON;
>  }
>  
> +static int
> +qca8k_cled_blink_set(struct led_classdev *ldev,
> +		     unsigned long *delay_on,
> +		     unsigned long *delay_off)
> +{
> +	struct qca8k_led *led = container_of(ldev, struct qca8k_led, cdev);
> +	u32 mask, val = QCA8K_LED_ALWAYS_BLINK_4HZ;
> +	struct qca8k_led_pattern_en reg_info;
> +	struct qca8k_priv *priv = led->priv;
> +
> +	if (*delay_on == 0 && *delay_off == 0) {
> +		*delay_on = 125;
> +		*delay_off = 125;
> +	}
> +
> +	if (*delay_on != 125 || *delay_off != 125) {
> +		/* The hardware only supports blinking at 4Hz. Fall back
> +		 * to software implementation in other cases.
> +		 */
> +		return -EINVAL;
> +	}
> +
> +	qca8k_get_enable_led_reg(led->port_num, led->led_num, &reg_info);
> +
> +	if (led->port_num == 0 || led->port_num == 4) {
> +		mask = QCA8K_LED_PATTERN_EN_MASK;
> +		val <<= QCA8K_LED_PATTERN_EN_SHIFT;
> +	} else {
> +		mask = QCA8K_LED_PHY123_PATTERN_EN_MASK;
> +	}

Could you add a comment as to why the HW requires different approaches for
inner and outer ports?

> +
> +	regmap_update_bits(priv->regmap, reg_info.reg, mask << reg_info.shift,
> +			   val << reg_info.shift);
> +
> +	return 0;
> +}
> +
>  static int
>  qca8k_parse_port_leds(struct qca8k_priv *priv, struct fwnode_handle *port, int port_num)
>  {
> @@ -149,6 +186,7 @@ qca8k_parse_port_leds(struct qca8k_priv *priv, struct fwnode_handle *port, int p
>  
>  		port_led->cdev.max_brightness = 1;
>  		port_led->cdev.brightness_set_blocking = qca8k_cled_brightness_set_blocking;
> +		port_led->cdev.blink_set = qca8k_cled_blink_set;
>  		init_data.default_label = ":port";
>  		init_data.devicename = "qca8k";
>  		init_data.fwnode = led;
> -- 
> 2.39.2
> 


Thanks,
Michal