linux-kernel - Re: [PATCH v2 1/8] leds: Add support for AXP20X CHGLED

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <5fdac512-947d-fd46-d953-22d6fc837423@olimex.com>
Date:   Tue, 19 Feb 2019 09:42:30 +0200
From:   Stefan Mavrodiev <stefan@...mex.com>
To:     Pavel Machek <pavel@....cz>,
        Maxime Ripard <maxime.ripard@...tlin.com>
Cc:     Stefan Mavrodiev <stefan@...mex.com>,
        Jacek Anaszewski <jacek.anaszewski@...il.com>,
        Rob Herring <robh+dt@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        Chen-Yu Tsai <wens@...e.org>, Lee Jones <lee.jones@...aro.org>,
        "open list:LED SUBSYSTEM" <linux-leds@...r.kernel.org>,
        "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
        <devicetree@...r.kernel.org>,
        "open list:X-POWERS MULTIFUNCTION PMIC DEVICE DRIVERS" 
        <linux-kernel@...r.kernel.org>,
        "moderated list:ARM/Allwinner sunXi SoC support" 
        <linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH v2 1/8] leds: Add support for AXP20X CHGLED


On 2/15/19 8:32 PM, Pavel Machek wrote:
> Hi!
>
>> On Fri, Feb 15, 2019 at 01:50:06PM +0200, Stefan Mavrodiev wrote:
>>> +static ssize_t control_store(struct device *dev, struct device_attribute *attr,
>>> +			     const char *buf, size_t size)
>>> +{
>>> +	struct led_classdev *cdev = dev_get_drvdata(dev);
>>> +	struct axp20x_led *priv = to_axp20x_led(cdev);
>>> +	unsigned long val;
>>> +	int ret;
>>> +
>>> +	ret = kstrtoul(buf, 0, &val);
>>> +	if (ret)
>>> +		return ret;
>>> +
>>> +	/**
>>> +	 * Supported values are:
>>> +	 *   - 0 : Manual control
>>> +	 *   - 1 : Charger control
>>> +	 */
> ...
>>> +static struct attribute *axp20x_led_attrs[] = {
>>> +	&dev_attr_control.attr,
>>> +	&dev_attr_mode.attr,
>>> +	NULL,
>>> +};
>>> +ATTRIBUTE_GROUPS(axp20x_led);
>> I can't really say whether adding sysfs handles for this is the right
>> thing to do, but if it is you should document the interface.
> It is not. See "Add Intel Cherry Trail Whiskey Cove PMIC LEDs" thread
> in the last few days.

What I understood is that there will be changes in the LED core, exporting
a new sysfs entry "hw_control". Then I should set HW_CONTROL flag for the
device and add hw_control_get/set callback functions. And this can happen
when the LED core is updated. Right?

However I didn't understand how (in my case) hardware controlled pattern
will be selected?

Best regards,
Stefan Mavrodiev

>>> +	if (!of_property_read_u8(np, "x-powers,charger-mode", &value)) {
>>> +		priv->ctrl = AXP20X_CHGLED_CTRL_CHARGER;
>>> +		priv->mode = (value < 2) ? value : 0;
>>> +	} else {
>>> +		priv->ctrl = AXP20X_CHGLED_CTRL_MANUAL;
>>> +	}
>> I'm not sure we want to make this a property of the device
>> tree. Changing the device tree isn't an option for some users, so we
>> need to make sure we can change it even if we can't change the device
>> tree.
> We want this to be configurable at run time. It can get default from
> the device tree.
>
> If we go for the "hardware" trigger, you'll get it for free.
>
> Best regards,
> 								Pavel