Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Fri, 15 Feb 2019 10:27:09 +0300
From:   Yauhen Kharuzhy <jekhor@gmail.com>
To:     Pavel Machek <pavel@ucw.cz>
Cc:     Jacek Anaszewski <jacek.anaszewski@gmail.com>,
        Hans de Goede <hdegoede@redhat.com>,
        linux-kernel@vger.kernel.org, linux-leds@vger.kernel.org
Subject: Re: [PATCH v2 1/2] leds: Add Intel Cherry Trail Whiskey Cove PMIC
 LEDs
Message-ID: <20190215072709.GD30250@jeknote.loshitsa1.net>
References: <20190212205901.13037-2-jekhor@gmail.com>
 <1df39a63-533f-bb68-a056-a0241f148be9@redhat.com>
 <20190213230731.GA8557@amd>
 <42078a81-e32e-81b7-528f-d1adb60d31c3@redhat.com>
 <20190213233806.GA11867@amd>
 <562e2acd-a60a-2aea-4050-6d9414d23a4e@redhat.com>
 <20190214111423.GE6132@amd>
 <92cf09b8-726d-4f1b-94ba-368a66af2246@redhat.com>
 <2b6faaa5-b21e-a512-de7d-ca21be5045fc@gmail.com>
 <20190214230307.GA17358@amd>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20190214230307.GA17358@amd>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Fri, Feb 15, 2019 at 12:03:07AM +0100, Pavel Machek wrote:
> Hi!
> 
> > >>>I suggest that we deal with this special case by adding 3 custom
> > >>>sysfs attributes:
> > >>>
> > >>>1) "mode" which when read, prints, e.g. :
> > >>>manual [on-when-charging]
> > >>
> > >>While this allows _user on console_ to control everything using echo,
> > >>it is not suitable for applications trying to control LEDs.
> > >>
> > >>As there's nothing special about the case here, I believe we should
> > >>have generic solution here.
> > >>
> > >>My preffered solution would be "hardware" trigger that leaves the LED
> > >>in hardware control.
> > >
> > >As you explained in the parts which I snipped, there are many
> > >devices which have a similar choice for a LED being under hw or
> > >user control. I can see how this looks like a trigger and how we
> > >could use the trigger API for this.
> > >
> > >I believe though, that if we implement a "virtual" (for lack of
> > >a better word) trigger for this, that this should be done in the
> > >LED core. I can envision this working like this:
> > >
> > >1) Add a:
> > >
> > >hw_control_set(struct led_classdev *led_cdev, bool enable_hw_control);
> > 
> > Please note that we have support for hw patterns in the pattern trigger.
> > (see how drivers/leds/leds-sc27xx-bltc.c makes use of it for its
> > breathing pattern).
> > We have also support for hw blinking in timer trigger via blink_set op.
> > 
> > In addition to that there is brightness_hw_changed sysfs attribute
> > with led_classdev_notify_brightness_hw_changed() LED API.
> > 
> > Couldn't they be used in concert to support the specific features
> > of the device in question?
> 
> I believe main issue here is this:
> 
> Hardware can automatically control the LED according to the charging
> status, or it can be used as normal software-controlled LED.
> 
> I believe we should use trigger to select if hardware controls it or
> not (and then add driver-specific files to describe the
> details). Other proposal is in the mail thread, too.

But there are two kinds of 'hardware control' here in discussion:
1:
	a) PMIC switching LED off/on/breathing/blinking reflected the charging
	state (charger is controlled by PMIC entirely without of
	software intervention) – 'hw controlled' mode
	b) Software controls when LED is on/off/breathing/blinking but
	patterns are generated by PMIC – 'sw controlled' mode.

2:
	a) parameters of lighting (continuous/blinking/breathing) are
	set in the PMIC and the PMIC generates patterns
	b) blinking/breathing patterns are generated by the software
	entirely.

It seems that we sometimes confuse this two kinds of hw control definitions
in the discussion.


The simple use case question: with this hardware and existing LED class
API, what user/app should to do to make LED breathing with hw-generated
pattern?

As I see, user should activate 'pattern' trigger and write to its hw_pattern attribute...
hm... big pattern which describes every step of breathing? Some
simplified pattern which should be interpreted as 'enable the breathing
an set its frequency' by driver? Which level of simplification will be
suitable? Which criteria of pattern rejection should be?

-- 
Yauhen Kharuzhy