MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII;
 format=flowed
Content-Transfer-Encoding: 7bit
Date:   Mon, 20 Dec 2021 09:01:49 +0100
From:   Florian Eckert <fe@dev.tdt.de>
To:     Pavel Machek <pavel@ucw.cz>
Cc:     robh+dt@kernel.org, linux-kernel@vger.kernel.org,
        linux-leds@vger.kernel.org, devicetree@vger.kernel.org
Subject: Re: [PATCH v2 1/2] leds: ktd20xx: Add the KTD20xx family of the RGB
 LEDs driver from Kinetic
In-Reply-To: <20211215204606.GH28336@duo.ucw.cz>
References: <20211123101826.9069-1-fe@dev.tdt.de>
 <20211123101826.9069-2-fe@dev.tdt.de> <20211215204606.GH28336@duo.ucw.cz>
Message-ID: <350ab383a3a40439c73a2769b9034ec6@dev.tdt.de>
User-Agent: Roundcube Webmail/1.3.17
Precedence: bulk

Hello Pavel,

thanks for your reply.

>> Variant 1:
>> The device has the ability to group LED outputs into two banks so that
>> the two LED banks can be controlled with the same color. This could 
>> not
>> be done via the LEDs 'sysfs' entry because of the limitation on the 
>> color
>> register count. The color of the two banks can be configured via 
>> device
>> 'sysfs' entry for all LEDs at once [current_color0|current_color1].
>> Which color the LED is to be used can be set via the 'sysfs' of the
>> individual LEDs via the 'multi_intensity' file. Valid values for the
>> colors (RGB) are 0 | 1. The value 0 selects the color register 0 and 
>> the
>> value 1 selects the color register 1.
>> 
>> Variant 2:
>> The device can also set the LED color independently. Since the chip 
>> only
>> has two color registers, but we want to control the 12 LEDs
>> independently via the 'led-class-multicolour' sysfs entry,
>> the full RGB color depth cannot be used. Due to this limitation, only 
>> 7
>> colors and the color black (off) can be set. To use this mode the 
>> color
>> registers must be preset via the device tree or the device 'sysfs'. 
>> The
>> color registers 0 must be preset with 0x00 (Red=0x00 Green=0x00 
>> Blue=0x00).
>> The color register1 should be preset all with the same value. This 
>> value
>> depends on which light intensity is to be used in the setup.
> 
> Summary: some crazy hardware.

I agree with you completely.
But there was no other hardware to choose it was already on the board.

> 
>> +static ssize_t current_color0_store(struct device *dev,
>> +		struct device_attribute *a,
>> +		const char *buf, size_t size)
>> +{
> 
> And now we have custom interface. Undocumented.

I just wanted to implement all possibilities features for the chip and
not commit to one. It may be that someone else can use the hardware for
something else. If that is the problem, then I can document that.

> That is not acceptable, sorry.
> Find a way to squeeze it into current RGB framework, perhaps with
> reduced feature set.

Ok I'll try and focus on variant 2.

> AFAICT you could either pretend it is 2-LED driver with full 8bit RGB
> on each, or you could pretend it is 12-LED driver with 1bit
> RGB. Select one and implement that.

On my board the LED driver is used for 12 LEDs and 1bit RGB colour 
depth.
So I could select 7 colors and (off).
For my understanding, does this mean that I remove variant 1 from the
source and send a v3 patchset? Or do I have to take anything else into
account when I use variant 2 now?

 From my point of view, the driver should fit if I remove variant 1?

Best regards

Florian