Date:   Wed, 10 Nov 2021 20:51:13 +0100
From:   Ansuel Smith <ansuelsmth@gmail.com>
To:     Andrew Lunn <andrew@lunn.ch>
Cc:     Marek =?iso-8859-1?Q?Beh=FAn?= <kabel@kernel.org>,
        Vivien Didelot <vivien.didelot@gmail.com>,
        Florian Fainelli <f.fainelli@gmail.com>,
        Vladimir Oltean <olteanv@gmail.com>,
        "David S. Miller" <davem@davemloft.net>,
        Jakub Kicinski <kuba@kernel.org>,
        Rob Herring <robh+dt@kernel.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
Subject: Re: [RFC PATCH v3 2/8] leds: add function to configure hardware
 controlled LED
Message-ID: <YYwisR8XLL7TnwCB@Ansuel-xps.localdomain>
References: <20211109022608.11109-1-ansuelsmth@gmail.com>
 <20211109022608.11109-3-ansuelsmth@gmail.com>
 <20211109040103.7b56bf82@thinkpad>
 <YYqEPZpGmjNgFj0L@Ansuel-xps.localdomain>
 <YYre31rVDcs8OWre@lunn.ch>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <YYre31rVDcs8OWre@lunn.ch>
Precedence: bulk

On Tue, Nov 09, 2021 at 09:49:35PM +0100, Andrew Lunn wrote:
> > > > +#ifdef CONFIG_LEDS_HARDWARE_CONTROL
> > > > +enum blink_mode_cmd {
> > > > +	BLINK_MODE_ENABLE, /* Enable the hardware blink mode */
> > > > +	BLINK_MODE_DISABLE, /* Disable the hardware blink mode */
> > > > +	BLINK_MODE_READ, /* Read the status of the hardware blink mode */
> > > > +	BLINK_MODE_SUPPORTED, /* Ask the driver if the hardware blink mode is supported */
> > > > +	BLINK_MODE_ZERO, /* Disable any hardware blink active */
> > > > +};
> > > > +#endif
> > > 
> > > this is a strange proposal for the API.
> > > 
> > > Anyway, led_classdev already has the blink_set() method, which is documented as
> > > 	/*
> > > 	  * Activate hardware accelerated blink, delays are in milliseconds
> > > 	  * and if both are zero then a sensible default should be chosen.
> > > 	  * The call should adjust the timings in that case and if it can't
> > > 	  * match the values specified exactly.
> > > 	  * Deactivate blinking again when the brightness is set to LED_OFF
> > > 	  * via the brightness_set() callback.
> > > 	  */
> > > 	int		(*blink_set)(struct led_classdev *led_cdev,
> > > 				     unsigned long *delay_on,
> > > 				     unsigned long *delay_off);
> > > 
> > > So we already have a method to set hardware blkinking, we don't need
> > > another one.
> > > 
> > > Marek
> > 
> > But that is about hardware blink, not a LED controlled by hardware based
> > on some rules/modes.
> > Doesn't really match the use for the hardware control.
> > Blink_set makes the LED blink contantly at the declared delay.
> > The blink_mode_cmd are used to request stuff to a LED in hardware mode.
> > 
> > Doesn't seem correct to change/enhance the blink_set function with
> > something that would do something completely different.
> 
> Humm. I can see merits for both.
> 
> What i like about reusing blink_set() is that it is well understood.
> There is a defined sysfs API for it. ledtrig-oneshot.c also uses it,
> for a non-repeating blink. So i think that also fits the PHY LED use
> case.
> 
> 	Andrew

If we should reuse blink_set to control hw blink I need to understand 2
thing.

The idea to implement the function hw_control_configure was to provide
the triggers some way to configure the various blink_mode. (and by using
the cmd enum provide different info based on the return value)

The advised path from Marek is to make the changes in the trigger_data
and the LED driver will then use that to configure blink mode.

I need to call an example to explain my concern:
qca8k switch. Can both run in hardware mode and software mode (by
controlling the reg to trigger manual blink) and also there is an extra
mode to blink permanently at 4hz.

Now someone would declare the brightness_set to control the led
manually and blink_set (for the permanent 4hz blink feature)
So that's where my idea comes about introducting another function and
the fact that it wouldn't match that well with blink_set with some LED.

I mean if we really want to use blink_set also for this(hw_control), we
can consider adding a bool to understand when hw_control is active or not.
So blink_set can be used for both feature.

Is that acceptable?

Also if we want to use blink_set I think we will have to drop all the
cmd mess and remove some error handling. Don't like that but if that's
what is needed for the feature, it's ok for me.

-- 
	Ansuel