On Sat, May 10, 2025 at 02:48:02PM +0700, Nam Tran wrote: > On Thu, 8 May 2025 Lee Jones wrote: > > On Thu, 08 May 2025, Andy Shevchenko wrote: > > > On Thu, May 8, 2025 at 5:27 PM Nam Tran <trannamatk@xxxxxxxxx> wrote: > > > > On Thu, 8 May 2025 Lee Jones wrote: > > > > > On Thu, 08 May 2025, Andy Shevchenko wrote: > > > > > > On Wed, May 7, 2025 at 7:42 PM Nam Tran <trannamatk@xxxxxxxxx> wrote: ... > > > > > > At least, based on the above it's my formal NAK from an auxdisplay perspective. > > > > > > > > > > This is fine. > > > > > > > > > > Just be aware, before you submit to LEDs again, that you need to use > > > > > what is available in the LEDs subsystem to it's fullest, before > > > > > hand-rolling all of your own APIs. The first submission didn't use a > > > > > single LED API. This, as before, would be a big NACK also. > > > > > > > > Thanks for the clarification. > > > > > > > > Just to confirm — the current version of the driver is customized to allow > > > > user space to directly manipulate LP5812 registers and to support the > > > > device’s full feature set. Because of this, it doesn’t follow the standard > > > > LED interfaces. > > > > > > But why? What's wrong with the LED ABI? (see also below question > > > before answering to this one) > > > > > > > Given that, would it be acceptable to submit this driver under the misc subsystem instead? > > > > > > But these are LEDs in the hardware and you can access them as 4 > > > individual LEDs, right? > > > > Right. Please work with the API you are offered in the first instance. > > My first assumption is always that this driver isn't as special as you > > think it might be. > > In direct mode, we can access them as individual LEDS. User doesn't need > to select LEDs. In this mode, it is a simple LED driver. > > However, user must select LEDs in scan mode. The hardware uses 4 pin to > display 12 LEDs (or 4 RGB LEDs). Ordering of LED selection also impact > to display capacibility. That why, I need to support another interface > for user to controll hardware's registers. > > In mix mode, we can control an individual LED and up to 6 scan LEDs. > However, user must select the order of single LED and which LEDs will be > use for scan function. > > The main point is user must have capacibility in write information to > hardware's registers to select LEDs in scan mode and mix mode. > > Besides system modes (direct mode, scan mode, mix mode), each LED has > manual mode and autonomous mode. > > A example steps to display a LED in manual mode > # Set drive mode is Scan mode with 4 scan. Scan order 0 is out_0, > # Scan order 1 is out_1, Scan order 2 is out_2, and Scan order 3 is out_3 > echo tcmscan:4:0:1:2:3 > /sys/bus/i2c/drivers/lp5812/1-001b/lp5812_chip_setup/dev_config > # Enable led_A0 > echo 1 > /sys/bus/i2c/drivers/lp5812/1-001b/led_A0/activate > # Enable manual mode > echo manual > /sys/bus/i2c/drivers/lp5812/1-001b/led_A0/mode > # Set Dot Current > echo 100 > /sys/bus/i2c/drivers/lp5812/1-001b/led_A0/manual_dc > # Set Manual PWM > echo 100 > /sys/bus/i2c/drivers/lp5812/1-001b/led_A0/manual_pwm > > However in autonomous mode, the steps are complicated > # Set drive mode is Scan mode with 4 scan. Scan order 0 is out_0, > # Scan order 1 is out_1, Scan order 2 is out_2, and Scan order 3 is out_3 > echo tcmscan:4:0:1:2:3 > /sys/bus/i2c/drivers/lp5812/1-001b/lp5812_chip_setup/dev_config > # Enable led_A0 > echo 1 > /sys/bus/i2c/drivers/lp5812/1-001b/led_A0/activate > # Enable autonomous mode > echo autonomous > /sys/bus/i2c/drivers/lp5812/1-001b/led_A0/mode > # Config autonomous animation mode: (only use AEU1, start pause time: 3.04s, > # stop pause time: 3.04s, playback time: infinite time) > echo 1:10:10:15 > /sys/bus/i2c/drivers/lp5812/1-001b/led_A0/mode > # Config AEU1 playback times > echo 1 > /sys/bus/i2c/drivers/lp5812/1-001b/led_A0/AEU1/playback_time > # Config PWM > echo 100 > /sys/bus/i2c/drivers/lp5812/1-001b/led_A0/AEU1/pwm1 > echo 100 > /sys/bus/i2c/drivers/lp5812/1-001b/led_A0/AEU1/pwm2 > echo 100 > /sys/bus/i2c/drivers/lp5812/1-001b/led_A0/AEU1/pwm3 > echo 100 > /sys/bus/i2c/drivers/lp5812/1-001b/led_A0/AEU1/pwm4 > echo 100 > /sys/bus/i2c/drivers/lp5812/1-001b/led_A0/AEU1/pwm5 > # Config slope time > echo 5 > /sys/bus/i2c/drivers/lp5812/1-001b/led_A0/AEU1/slope_time_t1 > echo 5 > /sys/bus/i2c/drivers/lp5812/1-001b/led_A0/AEU1/slope_time_t2 > echo 5 > /sys/bus/i2c/drivers/lp5812/1-001b/led_A0/AEU1/slope_time_t3 > echo 5 > /sys/bus/i2c/drivers/lp5812/1-001b/led_A0/AEU1/slope_time_t4 > # Start autonomous > echo start > /sys/bus/i2c/drivers/lp5812/1-001b/lp5812_chip_setup/device_command > > I think setting PWM also same as brightness_set API. However, there are > many PWM config for a LED and it is one of other config to make autonomous mode work. > Therefore, standard led API can use in some use cases only. > > Please see the link below for a better visualization of how to configure the LP5812. > https://dev.ti.com/gallery/view/LED/LP581x/ver/0.10.0/ To me it sounds like you should start from the small steps, i.e. do not implement everything at once. And starting point of the 4 RGB LEDs sounds the best approach to me. Then, if needed, you can always move on with fancy features of this hardware on top of the existing code. -- With Best Regards, Andy Shevchenko
