Re: Re: [PATCH v3 04/10] input: misc: Add driver for AXP20x Power Enable Key

[Carlo Caione <carlo-KA+7E9HrN00dnm+yROfE0A@public.gmane.org>]

On Fri, Mar 28, 2014 at 12:38:03AM -0700, Dmitry Torokhov wrote:
> Hi Carlo,

Hi Dmitry,

> 
> On Thu, Mar 27, 2014 at 10:29:18PM +0100, Carlo Caione wrote:

[...]

> > +#include <linux/errno.h>
> > +#include <linux/irq.h>
> > +#include <linux/init.h>
> > +#include <linux/input.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/kernel.h>
> > +#include <linux/mfd/axp20x.h>
> > +#include <linux/module.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/regmap.h>
> > +#include <linux/slab.h>
> > +
> > +#define AXP20X_PEK_STARTUP_MASK		(0xc0)
> > +#define AXP20X_PEK_SHUTDOWN_MASK	(0x03)
> > +
> > +static const char const *startup_time[] = { "128ms", "3s" , "1s", "2s" };
> > +static const char const *shutdown_time[] = { "4s", "6s" , "8s", "10s" };
> 
> Why not have everything expressed in milliseconds and have sysfs
> attribute apply the closest one possible?

Yep, I agree that could be nice. Fix in v3.

> By the way, do you want to plumb these through device tree as well?

Hum, not sure about that. AFAIK device tree is used for hardware
description whereas these are configuration parameters. Moreover you
would have these parameters saved through reboots.

> > +
> > +struct axp20x_pek {
> > +	struct axp20x_dev *axp20x;
> > +	struct input_dev *input;
> > +	int irq_dbr;
> > +	int irq_dbf;
> > +};
> > +
> > +struct axp20x_pek_ext_attr {
> > +	const char const **str;
> > +	unsigned int mask;
> > +};
> > +
> > +static struct axp20x_pek_ext_attr axp20x_pek_startup_ext_attr = {
> > +	.str	= startup_time,
> > +	.mask	= AXP20X_PEK_STARTUP_MASK,
> > +};
> > +
> > +static struct axp20x_pek_ext_attr axp20x_pek_shutdown_ext_attr = {
> > +	.str	= shutdown_time,
> > +	.mask	= AXP20X_PEK_SHUTDOWN_MASK,
> > +};
> > +
> > +static struct axp20x_pek_ext_attr *get_axp_ext_attr(struct device_attribute *attr)
> > +{
> > +	return container_of(attr, struct dev_ext_attribute, attr)->var;
> > +}
> > +
> > +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;
> > +	int cnt = 0;
> > +
> > +	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 == i)
> > +			cnt += sprintf(buf + cnt, "[%s] ", axp20x_ea->str[i]);
> > +		else
> > +			cnt += sprintf(buf + cnt, "%s ", axp20x_ea->str[i]);
> 
> Please just return the current value; why do we need pretty-printing?

It was done to have the possibility to look at the accepted values. With
the modification you suggested before this is not necessary anymore.
I'll change it.

> > +	}
> > +
> > +	cnt += sprintf(buf + cnt, "\n");
> > +
> > +	return cnt;
> > +}
> > +
> > +static ssize_t axp20x_store_ext_attr(struct device *dev, struct device_attribute *attr,
> > +			      const char *buf, size_t count)
> > +{
> > +	struct axp20x_pek *axp20x_pek = dev_get_drvdata(dev);
> > +	struct axp20x_pek_ext_attr *axp20x_ea = get_axp_ext_attr(attr);
> > +	char val_str[20];
> > +	int ret, i;
> > +	size_t len;
> > +
> > +	val_str[sizeof(val_str) - 1] = '\0';
> > +	strncpy(val_str, buf, sizeof(val_str) - 1);
> > +	len = strlen(val_str);
> > +
> > +	if (len && val_str[len - 1] == '\n')
> > +		val_str[len - 1] = '\0';
> > +
> > +	for (i = 0; i < 4; i++) {
> > +		if (!strcmp(val_str, axp20x_ea->str[i])) {
> > +
> > +			i <<= ffs(axp20x_ea->mask) - 1;
> > +			ret = regmap_update_bits(axp20x_pek->axp20x->regmap,
> > +						 AXP20X_PEK_KEY,
> > +						 axp20x_ea->mask, i);
> > +			if (ret != 0)
> > +				return -EINVAL;
> > +			return count;
> > +		}
> > +	}
> > +
> > +	return -EINVAL;
> > +}
> > +
> > +static struct dev_ext_attribute axp20x_dev_attr_startup = {
> > +	.attr	= __ATTR(startup, 0644, axp20x_show_ext_attr, axp20x_store_ext_attr),
> > +	.var	= &axp20x_pek_startup_ext_attr
> > +};
> > +
> > +static struct dev_ext_attribute axp20x_dev_attr_shutdown = {
> > +	__ATTR(shutdown, 0644, axp20x_show_ext_attr, axp20x_store_ext_attr),
> > +	&axp20x_pek_shutdown_ext_attr
> > +};
> > +
> > +static struct attribute *dev_attrs[] = {
> > +	&axp20x_dev_attr_startup.attr.attr,
> > +	&axp20x_dev_attr_shutdown.attr.attr,
> > +	NULL,
> > +};
> > +
> > +static struct attribute_group dev_attr_group = {
> > +	.attrs	= dev_attrs,
> > +};
> > +
> > +static const struct attribute_group *dev_attr_groups[] = {
> > +	&dev_attr_group,
> > +	NULL,
> > +};
> > +
> > +static irqreturn_t axp20x_pek_irq(int irq, void *pwr)
> > +{
> > +	struct input_dev *idev = pwr;
> > +	struct axp20x_pek *axp20x_pek = input_get_drvdata(idev);
> > +
> > +	if (irq == axp20x_pek->irq_dbr)
> > +		input_report_key(idev, KEY_POWER, true);
> > +	else if (irq == axp20x_pek->irq_dbf)
> > +		input_report_key(idev, KEY_POWER, false);
> > +
> > +	input_sync(idev);
> > +
> > +	return IRQ_HANDLED;
> > +}
> > +
> > +static int axp20x_pek_probe(struct platform_device *pdev)
> > +{
> > +	struct axp20x_pek *axp20x_pek;
> > +	struct axp20x_dev *axp20x;
> > +	struct input_dev *idev;
> > +	int error;
> > +
> > +	axp20x_pek = devm_kzalloc(&pdev->dev, sizeof(struct axp20x_pek),
> > +				  GFP_KERNEL);
> > +	if (!axp20x_pek)
> > +		return -ENOMEM;
> > +
> > +	axp20x_pek->axp20x = dev_get_drvdata(pdev->dev.parent);
> > +	axp20x = axp20x_pek->axp20x;
> > +
> > +	axp20x_pek->irq_dbr = platform_get_irq_byname(pdev, "PEK_DBR");
> > +	if (axp20x_pek->irq_dbr < 0) {
> > +		dev_err(&pdev->dev, "No IRQ for PEK_DBR, error=%d\n",
> > +				axp20x_pek->irq_dbr);
> > +		return axp20x_pek->irq_dbr;
> > +	}
> > +	axp20x_pek->irq_dbr = regmap_irq_get_virq(axp20x->regmap_irqc,
> > +						  axp20x_pek->irq_dbr);
> > +
> > +	axp20x_pek->irq_dbf = platform_get_irq_byname(pdev, "PEK_DBF");
> > +	if (axp20x_pek->irq_dbf < 0) {
> > +		dev_err(&pdev->dev, "No IRQ for PEK_DBF, error=%d\n",
> > +				axp20x_pek->irq_dbf);
> > +		return axp20x_pek->irq_dbf;
> > +	}
> > +	axp20x_pek->irq_dbf = regmap_irq_get_virq(axp20x->regmap_irqc,
> > +						  axp20x_pek->irq_dbf);
> > +
> > +	axp20x_pek->input = devm_input_allocate_device(&pdev->dev);
> > +	if (!axp20x_pek->input)
> > +		return -ENOMEM;
> > +
> > +	idev = axp20x_pek->input;
> > +
> > +	idev->name = "axp20x-pek";
> > +	idev->phys = "m1kbd/input2";
> > +	idev->dev.parent = &pdev->dev;
> > +
> > +	input_set_capability(idev, EV_KEY, KEY_POWER);
> > +
> > +	input_set_drvdata(idev, axp20x_pek);
> > +
> > +	error = devm_request_threaded_irq(&pdev->dev, axp20x_pek->irq_dbr,
> > +					  NULL, axp20x_pek_irq, 0,
> > +					  "axp20x-pek-dbr", idev);
> 
> Why does it have to be threaded IRQ?

Because this is already handled as a nested irq from a irq thread. The
parent threaded irq is installed by regmap_add_irq_chip() in the MFD
driver core.

> > +	if (error) {
> > +		dev_err(axp20x->dev, "Failed to request dbr IRQ#%d: %d\n",
> > +			axp20x_pek->irq_dbr, error);
> > +
> > +		return error;
> > +	}
> > +
> > +	error = devm_request_threaded_irq(&pdev->dev, axp20x_pek->irq_dbf,
> > +					  NULL, axp20x_pek_irq, 0,
> > +					  "axp20x-pek-dbf", idev);
> > +	if (error) {
> > +		dev_err(axp20x->dev, "Failed to request dbf IRQ#%d: %d\n",
> > +			axp20x_pek->irq_dbf, error);
> > +		return error;
> > +	}
> > +
> > +	idev->dev.groups = dev_attr_groups;
> 
> These are not generic input attributes so they should belong to the platform
> device, not input device.

Ok. Can I ask why they cannot be considered an input attributes?

> > +	error = input_register_device(idev);
> > +	if (error) {
> > +		dev_err(axp20x->dev, "Can't register input device: %d\n", error);
> > +		return error;
> > +	}
> > +
> > +	platform_set_drvdata(pdev, axp20x_pek);
> > +
> > +	return 0;
> > +}
> > +
> > +static int axp20x_pek_remove(struct platform_device *pdev)
> > +{
> > +	struct axp20x_pek *axp20x_pek = platform_get_drvdata(pdev);
> > +
> > +	input_unregister_device(axp20x_pek->input);
> 
> No need to call input_unregister_device() for managed input devices.

Right. Fix in v3.

> > +
> > +	return 0;
> > +}
> > +
> > +static struct platform_driver axp20x_pek_driver = {
> > +	.probe		= axp20x_pek_probe,
> > +	.remove		= axp20x_pek_remove,
> > +	.driver		= {
> > +		.name		= "axp20x-pek",
> > +		.owner		= THIS_MODULE,
> > +	},
> > +};
> > +module_platform_driver(axp20x_pek_driver);
> > +
> > +MODULE_DESCRIPTION("axp20x Power Button");
> > +MODULE_AUTHOR("Carlo Caione <carlo-KA+7E9HrN00dnm+yROfE0A@public.gmane.org>");
> > +MODULE_LICENSE("GPL");
> > -- 
> > 1.8.3.2
> > 
> 
> Thanks.

Thank you,

-- 
Carlo Caione