Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 15 Dec 2022 17:49:57 +0000
From:   "Russell King (Oracle)" <linux@armlinux.org.uk>
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>,
        Jonathan Corbet <corbet@lwn.net>, Pavel Machek <pavel@ucw.cz>,
        John Crispin <john@phrozen.org>, netdev@vger.kernel.org,
        devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-doc@vger.kernel.org, linux-leds@vger.kernel.org,
        Tim Harvey <tharvey@gateworks.com>,
        Alexander Stein <alexander.stein@ew.tq-group.com>,
        Rasmus Villemoes <rasmus.villemoes@prevas.dk>
Subject: Re: [PATCH v7 10/11] net: dsa: qca8k: add LEDs support
Message-ID: <Y5teRQ5mv1aTix4w@shell.armlinux.org.uk>
References: <20221214235438.30271-1-ansuelsmth@gmail.com>
 <20221214235438.30271-11-ansuelsmth@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20221214235438.30271-11-ansuelsmth@gmail.com>
Sender: Russell King (Oracle) <linux@armlinux.org.uk>
Precedence: bulk

Hi,

On Thu, Dec 15, 2022 at 12:54:37AM +0100, Christian Marangi wrote:
> +static int
> +qca8k_cled_hw_control_configure(struct led_classdev *ldev, unsigned long rules,
> +				enum blink_mode_cmd cmd)
> +{
> +	struct qca8k_led *led = container_of(ldev, struct qca8k_led, cdev);
> +	struct led_trigger *trigger = ldev->trigger;
> +	struct qca8k_led_pattern_en reg_info;
> +	struct qca8k_priv *priv = led->priv;
> +	u32 offload_trigger = 0, mask, val;
> +	int ret;
> +
> +	/* Check trigger compatibility */
> +	if (strcmp(trigger->name, "netdev"))
> +		return -EOPNOTSUPP;
> +
> +	if (!strcmp(trigger->name, "netdev"))
> +		ret = qca8k_parse_netdev(rules, &offload_trigger, &mask);

I'm not sure how well the compiler will spot that, but as far as
readability goes, that second if() statement appears to be redundant.

> +
> +	if (ret)
> +		return ret;
> +
> +	qca8k_get_control_led_reg(led->port_num, led->led_num, &reg_info);
> +
> +	switch (cmd) {
> +	case BLINK_MODE_SUPPORTED:
> +		/* We reach this point, we are sure the trigger is supported */
> +		return 1;
> +	case BLINK_MODE_ZERO:
> +		/* We set 4hz by default */
> +		u32 default_reg = QCA8K_LED_BLINK_4HZ;
> +
> +		ret = regmap_update_bits(priv->regmap, reg_info.reg,
> +					 QCA8K_LED_RULE_MASK << reg_info.shift,
> +					 default_reg << reg_info.shift);
> +		break;
> +	case BLINK_MODE_ENABLE:
> +		ret = regmap_update_bits(priv->regmap, reg_info.reg,
> +					 mask << reg_info.shift,
> +					 offload_trigger << reg_info.shift);
> +		break;
> +	case BLINK_MODE_DISABLE:
> +		ret = regmap_update_bits(priv->regmap, reg_info.reg,
> +					 mask << reg_info.shift,
> +					 0);
> +		break;

I think it needs to be made more clear in the documentation that if
this is called with ENABLE, then _only_ those modes in flags... (or
is it now called "rules"?) are to be enabled and all other modes
should be disabled. Conversely, DISABLE is used to disable all
modes. However, if that's the case, then ZERO was misdescribed, and
should probably be called DEFAULT. At least that's the impression
that I get from the above code.

> +	case BLINK_MODE_READ:
> +		ret = regmap_read(priv->regmap, reg_info.reg, &val);
> +		if (ret)
> +			return ret;
> +
> +		val >>= reg_info.shift;
> +		val &= offload_trigger;
> +
> +		/* Special handling for LED_BLINK_2HZ */
> +		if (!val && offload_trigger == QCA8K_LED_BLINK_2HZ)
> +			val = 1;

Hmm, so if a number of different modes is in flags or rules, then
as long as one matches, this returns 1? So it's an "any of these
modes is enabled" test.

> +
> +		return val;
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +
> +	return ret;
> +}
> +
> +static void
> +qca8k_led_brightness_set(struct qca8k_led *led,
> +			 enum led_brightness b)
> +{
> +	struct qca8k_led_pattern_en reg_info;
> +	struct qca8k_priv *priv = led->priv;
> +	u32 val = QCA8K_LED_ALWAYS_OFF;
> +
> +	qca8k_get_enable_led_reg(led->port_num, led->led_num, &reg_info);
> +
> +	if (b)
> +		val = QCA8K_LED_ALWAYS_ON;
> +
> +	regmap_update_bits(priv->regmap, reg_info.reg,
> +			   GENMASK(1, 0) << reg_info.shift,
> +			   val << reg_info.shift);
> +}
> +
> +static void
> +qca8k_cled_brightness_set(struct led_classdev *ldev,
> +			  enum led_brightness b)
> +{
> +	struct qca8k_led *led = container_of(ldev, struct qca8k_led, cdev);
> +
> +	return qca8k_led_brightness_set(led, b);
> +}
> +
> +static enum led_brightness
> +qca8k_led_brightness_get(struct qca8k_led *led)
> +{
> +	struct qca8k_led_pattern_en reg_info;
> +	struct qca8k_priv *priv = led->priv;
> +	u32 val;
> +	int ret;
> +
> +	qca8k_get_enable_led_reg(led->port_num, led->led_num, &reg_info);
> +
> +	ret = regmap_read(priv->regmap, reg_info.reg, &val);
> +	if (ret)
> +		return 0;
> +
> +	val >>= reg_info.shift;
> +	val &= GENMASK(1, 0);
> +
> +	return val > 0 ? 1 : 0;
> +}
> +
> +static enum led_brightness
> +qca8k_cled_brightness_get(struct led_classdev *ldev)
> +{
> +	struct qca8k_led *led = container_of(ldev, struct qca8k_led, cdev);
> +
> +	return qca8k_led_brightness_get(led);
> +}
> +
> +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);
> +	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);
> +
> +	regmap_update_bits(priv->regmap, reg_info.reg,
> +			   GENMASK(1, 0) << reg_info.shift,
> +			   QCA8K_LED_ALWAYS_BLINK_4HZ << reg_info.shift);
> +
> +	return 0;
> +}
> +
> +static int
> +qca8k_cled_trigger_offload(struct led_classdev *ldev, bool enable)
> +{
> +	struct qca8k_led *led = container_of(ldev, struct qca8k_led, cdev);
> +
> +	struct qca8k_led_pattern_en reg_info;
> +	struct qca8k_priv *priv = led->priv;
> +	u32 val = QCA8K_LED_ALWAYS_OFF;
> +
> +	qca8k_get_enable_led_reg(led->port_num, led->led_num, &reg_info);
> +
> +	if (enable)
> +		val = QCA8K_LED_RULE_CONTROLLED;
> +
> +	return regmap_update_bits(priv->regmap, reg_info.reg,
> +				  GENMASK(1, 0) << reg_info.shift,
> +				  val << reg_info.shift);

88e151x doesn't have the ability to change in this way - we have
a register with a 4-bit field which selects the LED mode from one
of many, or forces the LED on/off/hi-z/blink.

Not specifically for this patch, but talking generally about this
approach, the other issue I forsee with this is that yes, 88e151x has
three LEDs, but the LED modes are also used to implement control
signals (e.g., on a SFP, LOS can be implemented by programming mode
0 on LED2 (which makes it indicate link or not.) If we expose all the
LEDs we run the risk of the LED subsystem trampling over that
configuration and essentially messing up such modules. So the Marvell
PHY driver would need to know when it is appropriate to expose these
things to the LED subsystem.

I guess doing it dependent on firmware description as you do in
this driver would work - if there's no firmware description, they're
not exposed.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!