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

[Quentin Schulz]

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

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

Attachment: signature.asc
Description: OpenPGP digital signature