Received-SPF: pass (google.com: domain of linux-kernel+bounces-40166-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Date: Fri, 26 Jan 2024 06:09:43 -0800
From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
To: Dmitry Rokosov <ddrokosov@salutedevices.com>
Cc: Lee Jones <lee@kernel.org>,
	Martin Kurbanov <mmkurbanov@salutedevices.com>,
	Jacek Anaszewski <jacek.anaszewski@gmail.com>,
	Pavel Machek <pavel@ucw.cz>, Rob Herring <robh+dt@kernel.org>,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
	Conor Dooley <conor+dt@kernel.org>,
	Jonathan Corbet <corbet@lwn.net>,
	Andy Shevchenko <andy.shevchenko@gmail.com>,
	linux-kernel@vger.kernel.org, linux-leds@vger.kernel.org,
	devicetree@vger.kernel.org, linux-doc@vger.kernel.org,
	kernel@salutedevices.com
Subject: Re: [PATCH v1 1/2] leds: aw200xx: support for hw pattern controllers
Message-ID: <2024012643-safeness-stipulate-153f@gregkh>
References: <20231207125938.175119-1-mmkurbanov@salutedevices.com>
 <20231207125938.175119-2-mmkurbanov@salutedevices.com>
 <20231221161011.GO10102@google.com>
 <85c89859-ae03-4692-9c09-5779e4c40eae@salutedevices.com>
 <20240125130049.GF74950@google.com>
 <20240126122310.hrs37vybo2wnxto3@CAB-WSD-L081021>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240126122310.hrs37vybo2wnxto3@CAB-WSD-L081021>

On Fri, Jan 26, 2024 at 03:23:10PM +0300, Dmitry Rokosov wrote:
> Hello Lee,
> 
> On Thu, Jan 25, 2024 at 01:00:49PM +0000, Lee Jones wrote:
> > Looping in Jacek (LEDS) and Greg (SYFS) for some knowledgable input.
> > 
> > On Fri, 12 Jan 2024, Martin Kurbanov wrote:
> > > On 21.12.2023 19:10, Lee Jones wrote:
> > > > On Thu, 07 Dec 2023, Martin Kurbanov wrote:
> > > > 
> > > >> This led-controller supports 3 pattern controllers for auto breathing or
> > > >> group dimming control. Each pattern controller can work in auto
> > > >> breathing or manual control mode. All breathing parameters including
> > > >> rising/falling slope, on/off time, repeat times, min/max brightness
> > > >> and so on are configurable.
> > > >>
> > > >> Signed-off-by: Martin Kurbanov <mmkurbanov@salutedevices.com>
> > > >> ---
> > > >>  .../testing/sysfs-class-led-driver-aw200xx    | 108 +++
> > > >>  Documentation/leds/leds-aw200xx.rst           | 274 ++++++++
> > > >>  drivers/leds/leds-aw200xx.c                   | 649 ++++++++++++++++++
> > > >>  3 files changed, 1031 insertions(+)
> > > >>  create mode 100644 Documentation/leds/leds-aw200xx.rst
> > > > 
> > > > This interface is bananas.  Exposing an entire register interface to
> > > > sysfs does not sit will with me at all.  When we add support to a sysfs
> > > > class, we usually require it to be generic and work across all devices.
> > > > Adding device specific interfaces is generally decried and to be
> > > > avoided.  Don't forget, once we commit something to sysfs, it becomes
> > > > ABI and we have to support it forever.
> > > > 
> > > > A far better approach would be to add support for this in userspace
> > > > instead  You can use the standard I2C character device API to achieve
> > > > the same result.  That way we don't have the same level of commitment
> > > > and is generally a much more flexible/future-proof.
> > > > 
> > > 
> > > I used sysfs similarly to other LED drivers (for example, leds-lm3533).
> > > Additionally, the controller has interrupts about the completion of the pattern,
> > > which is best to handle in the kernel. In the case of implementation in user
> > > mode, there may be synchronization problems, as the controller has several
> > > memory pages that can be switched by writing the page number to register 0xF0.
> > 
> > leds-lm3533 is a 12 year old legacy exception AND has less than half of
> > the sysfs exports proposed here.  What makes aw200xx so different it
> > needs to an incomparable interface to any other that we currently
> > support?
> 
> >From my point of view, direct I2C raw requests from userspace are not a
> good solution as well due to synchronization problems, as Martin
> mentioned in the previous message.

Sorry, I missed this, what is the synchronization problem?  This is an
led, shouldn't have any real specific performance issues.

> We have honestly been attempting to integrate this functionality into
> the official LED pattern interface, but it cannot be achieved due to the
> absence of this interface's functionality:
> 1) Page-based access
> 2) Interrupts

I don't understand this, sorry.

> HW patterns are very useful mechanism to draw animation faster without
> any interactions with CPU, so I think we need to find the best architect
> approach for its integration.

The CPU is totally involved here, that should be identical.

> What is an alternative way to access such a hardware pattern interface?
> Debugfs? Or perhaps we should consider extending the LED pattern
> interface?

Or again, userspace.

debugfs is for debugging.

thanks,

greg k-h