[linux-sunxi] Re: [PATCH v4 4/9] input: misc: Add driver for AXP20x Power Enable Key

[Carlo Caione]

On Sun, Apr 13, 2014 at 10:17 AM, Dmitry Torokhov
<dmitry.torokhov at gmail.com> wrote:

<cut>

>> +static ssize_t axp20x_show_ext_attr(struct device *dev, struct device_attribute *attr,
>> +                                 char *buf)
>> +{
>> +     struct axp20x_pek *axp20x_pek = dev_get_drvdata(dev);
>> +     struct axp20x_pek_ext_attr *axp20x_ea = get_axp_ext_attr(attr);
>> +     unsigned int val;
>> +     int ret, i;
>> +
>> +     ret = regmap_read(axp20x_pek->axp20x->regmap, AXP20X_PEK_KEY, &val);
>> +     if (ret != 0)
>> +             return ret;
>> +
>> +     val &= axp20x_ea->mask;
>> +     val >>= ffs(axp20x_ea->mask) - 1;
>> +
>> +     for (i = 0; i < 4; i++)
>> +             if (val == axp20x_ea->p_time[i].idx)
>> +                     val = axp20x_ea->p_time[i].time;
>> +
>> +     return sprintf(buf, "%ums\n", val);
>
> Please do not print ums and instead document the units in sysfs ABI
> docs.

Ok, I'll fix it.

<cut>

>> +     if (device_create_file(&pdev->dev, &axp20x_dev_attr_startup.attr) ||
>> +         device_create_file(&pdev->dev, &axp20x_dev_attr_shutdown.attr))
>> +             return -ENODEV;
>
> I think you still want to use attribute group here. You also need to
> clean up the attributes when unbinding device. Also, why returning
> -ENODEV instead of the proper error code?

I'll fix the error code and I'll add the clean up code.
I'm still a bit puzzled about the attribute group for the platform
devices. IIRC for the platform devices the default attribute group is
still a bit an issue [1]

[1] http://www.spinics.net/lists/hotplug/msg05775.html

Thanks for the review,

-- 
Carlo Caione