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

Carlo Caione
2014-04-17T12:07:59Z

On Sun, Apr 13, 2014 at 10:17 AM, Dmitry Torokhov
<dmitry.torokhov@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