Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1688831 > unrolled thread

[PATCH 1/2] Input: axp20x-pek: add support for AXP221 PEK

Started byQuentin Schulz <quentin.schulz@free-electrons.com>
First post2017-07-17 12:00 +0200
Last post2017-07-18 10:20 +0200
Articles 5 — 2 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 1/2] Input: axp20x-pek: add support for AXP221 PEK Quentin Schulz <quentin.schulz@free-electrons.com> - 2017-07-17 12:00 +0200
    Re: [PATCH 1/2] Input: axp20x-pek: add support for AXP221 PEK Maxime Ripard <maxime.ripard@free-electrons.com> - 2017-07-17 13:30 +0200
      Re: [PATCH 1/2] Input: axp20x-pek: add support for AXP221 PEK Quentin Schulz <quentin.schulz@free-electrons.com> - 2017-07-18 09:40 +0200
        Re: [PATCH 1/2] Input: axp20x-pek: add support for AXP221 PEK Maxime Ripard <maxime.ripard@free-electrons.com> - 2017-07-18 10:00 +0200
          Re: [PATCH 1/2] Input: axp20x-pek: add support for AXP221 PEK Quentin Schulz <quentin.schulz@free-electrons.com> - 2017-07-18 10:20 +0200

#1688831 — [PATCH 1/2] Input: axp20x-pek: add support for AXP221 PEK

FromQuentin Schulz <quentin.schulz@free-electrons.com>
Date2017-07-17 12:00 +0200
Subject[PATCH 1/2] Input: axp20x-pek: add support for AXP221 PEK
Message-ID<u4aYO-3lP-5@gated-at.bofh.it>
The AXP221 has different values for startup time bits from the AXP20X.

This patch introduces a different platform_device_id to the driver and
adds the necessary code to handle the different platform_device_ids.

Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com>
---
 drivers/input/misc/axp20x-pek.c | 62 ++++++++++++++++++++++++++++++++++-------
 1 file changed, 52 insertions(+), 10 deletions(-)

diff --git a/drivers/input/misc/axp20x-pek.c b/drivers/input/misc/axp20x-pek.c
index 38c79ebff033..3efa1de51569 100644
--- a/drivers/input/misc/axp20x-pek.c
+++ b/drivers/input/misc/axp20x-pek.c
@@ -32,6 +32,7 @@
 struct axp20x_pek {
 	struct axp20x_dev *axp20x;
 	struct input_dev *input;
+	struct attribute_group *attribute_group;
 	int irq_dbr;
 	int irq_dbf;
 };
@@ -41,14 +42,21 @@ struct axp20x_time {
 	unsigned int idx;
 };
 
-static const struct axp20x_time startup_time[] = {
+static const struct axp20x_time axp20x_startup_time[] = {
 	{ .time = 128,  .idx = 0 },
 	{ .time = 1000, .idx = 2 },
 	{ .time = 3000, .idx = 1 },
 	{ .time = 2000, .idx = 3 },
 };
 
-static const struct axp20x_time shutdown_time[] = {
+static const struct axp20x_time axp221_startup_time[] = {
+	{ .time = 128,  .idx = 0 },
+	{ .time = 1000, .idx = 1 },
+	{ .time = 2000, .idx = 2 },
+	{ .time = 3000, .idx = 3 },
+};
+
+static const struct axp20x_time axp20x_shutdown_time[] = {
 	{ .time = 4000,  .idx = 0 },
 	{ .time = 6000,  .idx = 1 },
 	{ .time = 8000,  .idx = 2 },
@@ -61,15 +69,20 @@ struct axp20x_pek_ext_attr {
 };
 
 static struct axp20x_pek_ext_attr axp20x_pek_startup_ext_attr = {
-	.p_time	= startup_time,
+	.p_time	= axp20x_startup_time,
 	.mask	= AXP20X_PEK_STARTUP_MASK,
 };
 
 static struct axp20x_pek_ext_attr axp20x_pek_shutdown_ext_attr = {
-	.p_time	= shutdown_time,
+	.p_time	= axp20x_shutdown_time,
 	.mask	= AXP20X_PEK_SHUTDOWN_MASK,
 };
 
+static struct axp20x_pek_ext_attr axp221_pek_startup_ext_attr = {
+	.p_time	= axp221_startup_time,
+	.mask	= AXP20X_PEK_STARTUP_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;
@@ -148,6 +161,11 @@ static struct dev_ext_attribute axp20x_dev_attr_startup = {
 	.var	= &axp20x_pek_startup_ext_attr,
 };
 
+static struct dev_ext_attribute axp221_dev_attr_startup = {
+	.attr	= __ATTR(startup, 0644, axp20x_show_ext_attr, axp20x_store_ext_attr),
+	.var	= &axp221_pek_startup_ext_attr,
+};
+
 static struct dev_ext_attribute axp20x_dev_attr_shutdown = {
 	.attr	= __ATTR(shutdown, 0644, axp20x_show_ext_attr, axp20x_store_ext_attr),
 	.var	= &axp20x_pek_shutdown_ext_attr,
@@ -159,10 +177,20 @@ static struct attribute *axp20x_attributes[] = {
 	NULL,
 };
 
+static struct attribute *axp221_attributes[] = {
+	&axp221_dev_attr_startup.attr.attr,
+	&axp20x_dev_attr_shutdown.attr.attr,
+	NULL,
+};
+
 static const struct attribute_group axp20x_attribute_group = {
 	.attrs = axp20x_attributes,
 };
 
+static const struct attribute_group axp221_attribute_group = {
+	.attrs = axp221_attributes,
+};
+
 static irqreturn_t axp20x_pek_irq(int irq, void *pwr)
 {
 	struct input_dev *idev = pwr;
@@ -184,9 +212,10 @@ static irqreturn_t axp20x_pek_irq(int irq, void *pwr)
 
 static void axp20x_remove_sysfs_group(void *_data)
 {
-	struct device *dev = _data;
+	struct platform_device *pdev = _data;
+	struct axp20x_pek *axp_pek = platform_get_drvdata(pdev);
 
-	sysfs_remove_group(&dev->kobj, &axp20x_attribute_group);
+	sysfs_remove_group(&pdev->dev.kobj, axp_pek->attribute_group);
 }
 
 static int axp20x_pek_probe_input_device(struct axp20x_pek *axp20x_pek,
@@ -313,17 +342,19 @@ static int axp20x_pek_probe(struct platform_device *pdev)
 			return error;
 	}
 
-	error = sysfs_create_group(&pdev->dev.kobj, &axp20x_attribute_group);
+	axp20x_pek->attribute_group = (struct attribute_group *)platform_get_device_id(pdev)->driver_data;
+
+	error = sysfs_create_group(&pdev->dev.kobj,
+				   axp20x_pek->attribute_group);
 	if (error) {
 		dev_err(&pdev->dev, "Failed to create sysfs attributes: %d\n",
 			error);
 		return error;
 	}
 
-	error = devm_add_action(&pdev->dev,
-				axp20x_remove_sysfs_group, &pdev->dev);
+	error = devm_add_action(&pdev->dev, axp20x_remove_sysfs_group, pdev);
 	if (error) {
-		axp20x_remove_sysfs_group(&pdev->dev);
+		axp20x_remove_sysfs_group(pdev);
 		dev_err(&pdev->dev, "Failed to add sysfs cleanup action: %d\n",
 			error);
 		return error;
@@ -358,8 +389,19 @@ static const struct dev_pm_ops axp20x_pek_pm_ops = {
 #endif
 };
 
+static const struct platform_device_id axp_pek_id_match[] = {
+	{
+		.name = "axp20x-pek",
+		.driver_data = (kernel_ulong_t)&axp20x_attribute_group,
+	}, {
+		.name = "axp221-pek",
+		.driver_data = (kernel_ulong_t)&axp221_attribute_group,
+	},
+};
+
 static struct platform_driver axp20x_pek_driver = {
 	.probe		= axp20x_pek_probe,
+	.id_table	= axp_pek_id_match,
 	.driver		= {
 		.name		= "axp20x-pek",
 		.pm		= &axp20x_pek_pm_ops,
-- 
2.11.0

[toc] | [next] | [standalone]

#1688941

FromMaxime Ripard <maxime.ripard@free-electrons.com>
Date2017-07-17 13:30 +0200
Message-ID<u4cnU-4lE-19@gated-at.bofh.it>
In reply to#1688831

[Multipart message — attachments visible in raw view] — view raw

Hi,

On Mon, Jul 17, 2017 at 11:53:06AM +0200, Quentin Schulz wrote:
> The AXP221 has different values for startup time bits from the AXP20X.
> 
> This patch introduces a different platform_device_id to the driver and
> adds the necessary code to handle the different platform_device_ids.
> 
> Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com>
> ---
>  drivers/input/misc/axp20x-pek.c | 62 ++++++++++++++++++++++++++++++++++-------
>  1 file changed, 52 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/input/misc/axp20x-pek.c b/drivers/input/misc/axp20x-pek.c
> index 38c79ebff033..3efa1de51569 100644
> --- a/drivers/input/misc/axp20x-pek.c
> +++ b/drivers/input/misc/axp20x-pek.c
> @@ -32,6 +32,7 @@
>  struct axp20x_pek {
>  	struct axp20x_dev *axp20x;
>  	struct input_dev *input;
> +	struct attribute_group *attribute_group;
>  	int irq_dbr;
>  	int irq_dbf;
>  };
> @@ -41,14 +42,21 @@ struct axp20x_time {
>  	unsigned int idx;
>  };
>  
> -static const struct axp20x_time startup_time[] = {
> +static const struct axp20x_time axp20x_startup_time[] = {
>  	{ .time = 128,  .idx = 0 },
>  	{ .time = 1000, .idx = 2 },
>  	{ .time = 3000, .idx = 1 },
>  	{ .time = 2000, .idx = 3 },
>  };
>  
> -static const struct axp20x_time shutdown_time[] = {
> +static const struct axp20x_time axp221_startup_time[] = {
> +	{ .time = 128,  .idx = 0 },
> +	{ .time = 1000, .idx = 1 },
> +	{ .time = 2000, .idx = 2 },
> +	{ .time = 3000, .idx = 3 },
> +};
> +
> +static const struct axp20x_time axp20x_shutdown_time[] = {
>  	{ .time = 4000,  .idx = 0 },
>  	{ .time = 6000,  .idx = 1 },
>  	{ .time = 8000,  .idx = 2 },
> @@ -61,15 +69,20 @@ struct axp20x_pek_ext_attr {
>  };
>  
>  static struct axp20x_pek_ext_attr axp20x_pek_startup_ext_attr = {
> -	.p_time	= startup_time,
> +	.p_time	= axp20x_startup_time,
>  	.mask	= AXP20X_PEK_STARTUP_MASK,
>  };
>  
>  static struct axp20x_pek_ext_attr axp20x_pek_shutdown_ext_attr = {
> -	.p_time	= shutdown_time,
> +	.p_time	= axp20x_shutdown_time,
>  	.mask	= AXP20X_PEK_SHUTDOWN_MASK,
>  };
>  
> +static struct axp20x_pek_ext_attr axp221_pek_startup_ext_attr = {
> +	.p_time	= axp221_startup_time,
> +	.mask	= AXP20X_PEK_STARTUP_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;
> @@ -148,6 +161,11 @@ static struct dev_ext_attribute axp20x_dev_attr_startup = {
>  	.var	= &axp20x_pek_startup_ext_attr,
>  };
>  
> +static struct dev_ext_attribute axp221_dev_attr_startup = {
> +	.attr	= __ATTR(startup, 0644, axp20x_show_ext_attr, axp20x_store_ext_attr),
> +	.var	= &axp221_pek_startup_ext_attr,
> +};
> +
>  static struct dev_ext_attribute axp20x_dev_attr_shutdown = {
>  	.attr	= __ATTR(shutdown, 0644, axp20x_show_ext_attr, axp20x_store_ext_attr),
>  	.var	= &axp20x_pek_shutdown_ext_attr,
> @@ -159,10 +177,20 @@ static struct attribute *axp20x_attributes[] = {
>  	NULL,
>  };
>  
> +static struct attribute *axp221_attributes[] = {
> +	&axp221_dev_attr_startup.attr.attr,
> +	&axp20x_dev_attr_shutdown.attr.attr,
> +	NULL,
> +};
> +
>  static const struct attribute_group axp20x_attribute_group = {
>  	.attrs = axp20x_attributes,
>  };
>  
> +static const struct attribute_group axp221_attribute_group = {
> +	.attrs = axp221_attributes,
> +};
> +
>  static irqreturn_t axp20x_pek_irq(int irq, void *pwr)
>  {
>  	struct input_dev *idev = pwr;
> @@ -184,9 +212,10 @@ static irqreturn_t axp20x_pek_irq(int irq, void *pwr)
>  
>  static void axp20x_remove_sysfs_group(void *_data)
>  {
> -	struct device *dev = _data;
> +	struct platform_device *pdev = _data;
> +	struct axp20x_pek *axp_pek = platform_get_drvdata(pdev);
>  
> -	sysfs_remove_group(&dev->kobj, &axp20x_attribute_group);
> +	sysfs_remove_group(&pdev->dev.kobj, axp_pek->attribute_group);
>  }
>  
>  static int axp20x_pek_probe_input_device(struct axp20x_pek *axp20x_pek,
> @@ -313,17 +342,19 @@ static int axp20x_pek_probe(struct platform_device *pdev)
>  			return error;
>  	}
>  
> -	error = sysfs_create_group(&pdev->dev.kobj, &axp20x_attribute_group);
> +	axp20x_pek->attribute_group = (struct attribute_group *)platform_get_device_id(pdev)->driver_data;

That line is too long, and you don't check the returned pointer of
platform_get_device_id. You should split it and check for the pointer.

> +
> +	error = sysfs_create_group(&pdev->dev.kobj,
> +				   axp20x_pek->attribute_group);

Wouldn't it make more sense to just store the startup_time structure
in the axp20x_pek structure, rather than duplicating all this?

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[toc] | [prev] | [next] | [standalone]

#1689829

FromQuentin Schulz <quentin.schulz@free-electrons.com>
Date2017-07-18 09:40 +0200
Message-ID<u4vgT-7Vc-29@gated-at.bofh.it>
In reply to#1688941

[Multipart message — attachments visible in raw view] — view raw

Hi Maxime,

On 17/07/2017 13:29, Maxime Ripard wrote:
> Hi,
> 
> On Mon, Jul 17, 2017 at 11:53:06AM +0200, Quentin Schulz wrote:
>> The AXP221 has different values for startup time bits from the AXP20X.
>>
>> This patch introduces a different platform_device_id to the driver and
>> adds the necessary code to handle the different platform_device_ids.
>>
>> Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com>
>> ---
>>  drivers/input/misc/axp20x-pek.c | 62 ++++++++++++++++++++++++++++++++++-------
>>  1 file changed, 52 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/input/misc/axp20x-pek.c b/drivers/input/misc/axp20x-pek.c
>> index 38c79ebff033..3efa1de51569 100644
>> --- a/drivers/input/misc/axp20x-pek.c
>> +++ b/drivers/input/misc/axp20x-pek.c
>> @@ -32,6 +32,7 @@
>>  struct axp20x_pek {
>>  	struct axp20x_dev *axp20x;
>>  	struct input_dev *input;
>> +	struct attribute_group *attribute_group;
>>  	int irq_dbr;
>>  	int irq_dbf;
>>  };
>> @@ -41,14 +42,21 @@ struct axp20x_time {
>>  	unsigned int idx;
>>  };
>>  
>> -static const struct axp20x_time startup_time[] = {
>> +static const struct axp20x_time axp20x_startup_time[] = {
>>  	{ .time = 128,  .idx = 0 },
>>  	{ .time = 1000, .idx = 2 },
>>  	{ .time = 3000, .idx = 1 },
>>  	{ .time = 2000, .idx = 3 },
>>  };
>>  
>> -static const struct axp20x_time shutdown_time[] = {
>> +static const struct axp20x_time axp221_startup_time[] = {
>> +	{ .time = 128,  .idx = 0 },
>> +	{ .time = 1000, .idx = 1 },
>> +	{ .time = 2000, .idx = 2 },
>> +	{ .time = 3000, .idx = 3 },
>> +};
>> +
>> +static const struct axp20x_time axp20x_shutdown_time[] = {
>>  	{ .time = 4000,  .idx = 0 },
>>  	{ .time = 6000,  .idx = 1 },
>>  	{ .time = 8000,  .idx = 2 },
>> @@ -61,15 +69,20 @@ struct axp20x_pek_ext_attr {
>>  };
>>  
>>  static struct axp20x_pek_ext_attr axp20x_pek_startup_ext_attr = {
>> -	.p_time	= startup_time,
>> +	.p_time	= axp20x_startup_time,
>>  	.mask	= AXP20X_PEK_STARTUP_MASK,
>>  };
>>  
>>  static struct axp20x_pek_ext_attr axp20x_pek_shutdown_ext_attr = {
>> -	.p_time	= shutdown_time,
>> +	.p_time	= axp20x_shutdown_time,
>>  	.mask	= AXP20X_PEK_SHUTDOWN_MASK,
>>  };
>>  
>> +static struct axp20x_pek_ext_attr axp221_pek_startup_ext_attr = {
>> +	.p_time	= axp221_startup_time,
>> +	.mask	= AXP20X_PEK_STARTUP_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;
>> @@ -148,6 +161,11 @@ static struct dev_ext_attribute axp20x_dev_attr_startup = {
>>  	.var	= &axp20x_pek_startup_ext_attr,
>>  };
>>  
>> +static struct dev_ext_attribute axp221_dev_attr_startup = {
>> +	.attr	= __ATTR(startup, 0644, axp20x_show_ext_attr, axp20x_store_ext_attr),
>> +	.var	= &axp221_pek_startup_ext_attr,
>> +};
>> +
>>  static struct dev_ext_attribute axp20x_dev_attr_shutdown = {
>>  	.attr	= __ATTR(shutdown, 0644, axp20x_show_ext_attr, axp20x_store_ext_attr),
>>  	.var	= &axp20x_pek_shutdown_ext_attr,
>> @@ -159,10 +177,20 @@ static struct attribute *axp20x_attributes[] = {
>>  	NULL,
>>  };
>>  
>> +static struct attribute *axp221_attributes[] = {
>> +	&axp221_dev_attr_startup.attr.attr,
>> +	&axp20x_dev_attr_shutdown.attr.attr,
>> +	NULL,
>> +};
>> +
>>  static const struct attribute_group axp20x_attribute_group = {
>>  	.attrs = axp20x_attributes,
>>  };
>>  
>> +static const struct attribute_group axp221_attribute_group = {
>> +	.attrs = axp221_attributes,
>> +};
>> +
>>  static irqreturn_t axp20x_pek_irq(int irq, void *pwr)
>>  {
>>  	struct input_dev *idev = pwr;
>> @@ -184,9 +212,10 @@ static irqreturn_t axp20x_pek_irq(int irq, void *pwr)
>>  
>>  static void axp20x_remove_sysfs_group(void *_data)
>>  {
>> -	struct device *dev = _data;
>> +	struct platform_device *pdev = _data;
>> +	struct axp20x_pek *axp_pek = platform_get_drvdata(pdev);
>>  
>> -	sysfs_remove_group(&dev->kobj, &axp20x_attribute_group);
>> +	sysfs_remove_group(&pdev->dev.kobj, axp_pek->attribute_group);
>>  }
>>  
>>  static int axp20x_pek_probe_input_device(struct axp20x_pek *axp20x_pek,
>> @@ -313,17 +342,19 @@ static int axp20x_pek_probe(struct platform_device *pdev)
>>  			return error;
>>  	}
>>  
>> -	error = sysfs_create_group(&pdev->dev.kobj, &axp20x_attribute_group);
>> +	axp20x_pek->attribute_group = (struct attribute_group *)platform_get_device_id(pdev)->driver_data;
> 
> That line is too long, and you don't check the returned pointer of
> platform_get_device_id. You should split it and check for the pointer.
> 

That's what I've done in a previous driver:
http://elixir.free-electrons.com/linux/latest/source/drivers/iio/adc/axp20x_adc.c#L543

I'll do what you said anyway.

>> +
>> +	error = sysfs_create_group(&pdev->dev.kobj,
>> +				   axp20x_pek->attribute_group);
> 
> Wouldn't it make more sense to just store the startup_time structure
> in the axp20x_pek structure, rather than duplicating all this?
> 

I don't know. Basically, you would need to recreate manually all the
structures in the probe function, but why not.

Thanks,
Quentin

-- 
Quentin Schulz, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[toc] | [prev] | [next] | [standalone]

#1689842

FromMaxime Ripard <maxime.ripard@free-electrons.com>
Date2017-07-18 10:00 +0200
Message-ID<u4vAe-81L-19@gated-at.bofh.it>
In reply to#1689829

[Multipart message — attachments visible in raw view] — view raw

On Tue, Jul 18, 2017 at 09:36:04AM +0200, Quentin Schulz wrote:
> >> +
> >> +	error = sysfs_create_group(&pdev->dev.kobj,
> >> +				   axp20x_pek->attribute_group);
> > 
> > Wouldn't it make more sense to just store the startup_time structure
> > in the axp20x_pek structure, rather than duplicating all this?
> > 
> 
> I don't know. Basically, you would need to recreate manually all the
> structures in the probe function, but why not.

Why?

You have access to axp20x_pek in the readout function. You can just
use the table stored in the structure here, and just ignore any
argument you might have.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[toc] | [prev] | [next] | [standalone]

#1689860

FromQuentin Schulz <quentin.schulz@free-electrons.com>
Date2017-07-18 10:20 +0200
Message-ID<u4vTA-8ne-13@gated-at.bofh.it>
In reply to#1689842

[Multipart message — attachments visible in raw view] — view raw

Hi Maxime,

On 18/07/2017 09:57, Maxime Ripard wrote:
> On Tue, Jul 18, 2017 at 09:36:04AM +0200, Quentin Schulz wrote:
>>>> +
>>>> +	error = sysfs_create_group(&pdev->dev.kobj,
>>>> +				   axp20x_pek->attribute_group);
>>>
>>> Wouldn't it make more sense to just store the startup_time structure
>>> in the axp20x_pek structure, rather than duplicating all this?
>>>
>>
>> I don't know. Basically, you would need to recreate manually all the
>> structures in the probe function, but why not.
> 
> Why?
> 
> You have access to axp20x_pek in the readout function. You can just
> use the table stored in the structure here, and just ignore any
> argument you might have.
> 

All I'm saying is that:
struct axp20x_time is referenced in struct axp20x_pek_ext_attr which in
turn is referenced in struct dev_ext_attribute which is referenced in
struct attribute which is reference in const struct attribute_group
which is finally used to add/remove a sysfs group programmatically.

The earlier in the chain you decide to stop duplicating code, the most
structures you have to programmatically create. That was my point.

Quentin

-- 
Quentin Schulz, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web