Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Tue, 9 Nov 2021 21:49:35 +0100
From:   Andrew Lunn <andrew@lunn.ch>
To:     Ansuel Smith <ansuelsmth@gmail.com>
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: <YYre31rVDcs8OWre@lunn.ch>
References: <20211109022608.11109-1-ansuelsmth@gmail.com>
 <20211109022608.11109-3-ansuelsmth@gmail.com>
 <20211109040103.7b56bf82@thinkpad>
 <YYqEPZpGmjNgFj0L@Ansuel-xps.localdomain>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <YYqEPZpGmjNgFj0L@Ansuel-xps.localdomain>
Precedence: bulk

> > > +#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