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

[Stefan Mavrodiev]

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