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
