Received-SPF: pass (google.com: domain of linux-kernel+bounces-8742-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Thu, 21 Dec 2023 16:10:11 +0000
From: Lee Jones <lee@kernel.org>
To: Martin Kurbanov <mmkurbanov@salutedevices.com>
Cc: 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,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Subject: Re: [PATCH v1 1/2] leds: aw200xx: support for hw pattern controllers
Message-ID: <20231221161011.GO10102@google.com>
References: <20231207125938.175119-1-mmkurbanov@salutedevices.com>
 <20231207125938.175119-2-mmkurbanov@salutedevices.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20231207125938.175119-2-mmkurbanov@salutedevices.com>

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.

-- 
Lee Jones [李琼斯]