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

[Maxime Ripard]

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@xxxxxxxxxxxxxxxxxx>
> ---
>  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

Attachment: signature.asc
Description: PGP signature