Re: [PATCH 1/2] extcon: Use the unique id for external connector instead of string

[Chanwoo Choi <cwchoi00@gmail.com>]

Hi Krzysztof,

On Sun, May 17, 2015 at 4:10 PM, Krzysztof Kozlowski
<k.kozlowski@samsung.com> wrote:
> 2015-05-15 23:31 GMT+09:00 Chanwoo Choi <cw00.choi@samsung.com>:
>> This patch uses the unique id to identify the type of external connector instead
>> of string name. The string name have the many potential issues. So, this patch
>> defines the 'extcon' enumeration which includes all supported external connector
>> on EXTCON subsystem. If new external connector is necessary, the unique id of
>> new connector have to be added in 'extcon' enumeration. There are current
>> supported external connector in 'enum extcon' as following:
>
> I like the idea of switching to unique identifier. Some comments below.
>
>>
>> enum extcon {
>>         EXTCON_NONE             = 0x0,  /* NONE */
>>
>>         /* USB external connector */
>>         EXTCON_USB              = 0x1,  /* USB */
>>         EXTCON_USB_HOST         = 0x2,  /* USB-HOST */
>>
>>         /* Charger external connector */
>>         EXTCON_TA               = 0x10, /* TA */
>>         EXTCON_FAST_CHARGER     = 0x11, /* FAST-CHARGER */
>>         EXTCON_SLOW_CHARGER     = 0x12, /* SLOW-CHARGER */
>>         EXTCON_CHARGE_DOWNSTREAM= 0x13, /* CHARGE-DOWNSTREAM */
>>
>>         /* Audio and video external connector */
>>         EXTCON_LINE_IN          = 0x20, /* LINE-IN */
>>         EXTCON_LINE_OUT         = 0x21, /* LINE-OUT */
>>         EXTCON_MICROPHONE       = 0x22, /* MICROPHONE */
>>         EXTCON_HEADPHONE        = 0x23, /* HEADPHONE */
>>
>>         EXTCON_HDMI             = 0x30, /* HDMI */
>>         EXTCON_MHL              = 0x31, /* MHL */
>>         EXTCON_DVI              = 0x32, /* DVI */
>>         EXTCON_VGA              = 0x33, /* VGA */
>>         EXTCON_SPDIF_IN         = 0x34, /* SPDIF-IN */
>>         EXTCON_SPDIF_OUT        = 0x35, /* SPDIF-OUT */
>>         EXTCON_VIDEO_IN         = 0x36, /* VIDEO-IN */
>>         EXTCON_VIDEO_OUT        = 0x37, /* VIDEO-OUT */
>>
>>         /* Miscellaneous external connector */
>>         EXTCON_DOCK             = 0x50, /* DOCK */
>>         EXTCON_JIG              = 0x51, /* JIG */
>>         EXTCON_MECHANICAL       = 0x52, /* MECHANICAL */
>>
>>         __EXTCON_END,
>> };
>>
>> For exmaple in extcon-arizoan.c:
> s/exmaple/example/
> s/arizoan/arizona/

Good catch. I'll fix it.

>
>> diff --git a/drivers/extcon/extcon.c b/drivers/extcon/extcon.c
>> index 2fb5f75..4aeb585 100644
>> --- a/drivers/extcon/extcon.c
>> +++ b/drivers/extcon/extcon.c
>> @@ -3,6 +3,9 @@
>>   *
>>   *  External connector (extcon) class driver
>>   *
>> + * Copyright (C) 2015 Samsung Electronics
>> + * Author: Chanwoo Choi <cw00.choi@samsung.com>
>> + *
>>   * Copyright (C) 2012 Samsung Electronics
>>   * Author: Donggeun Kim <dg77.kim@samsung.com>
>>   * Author: MyungJoo Ham <myungjoo.ham@samsung.com>
>> @@ -32,36 +35,43 @@
>>  #include <linux/slab.h>
>>  #include <linux/sysfs.h>
>>
>> -/*
>> - * extcon_cable_name suggests the standard cable names for commonly used
>> - * cable types.
>> - *
>> - * However, please do not use extcon_cable_name directly for extcon_dev
>> - * struct's supported_cable pointer unless your device really supports
>> - * every single port-type of the following cable names. Please choose cable
>> - * names that are actually used in your extcon device.
>> - */
>> -const char extcon_cable_name[][CABLE_NAME_MAX + 1] = {
>> +#define SUPPORTED_CABLE_MAX    32
>
> Why only 32 cables are supported? I mean what is the reason behind the
> hard limit?

Current extcon core use the 32bit variable to identify the supported
external connectors of each extcon device. But, the number of
supported connectors should be updated. I have the plan about it.

>
>> +#define CABLE_NAME_MAX         30
>> +
>> +static const char *extcon_name[] =  {
>> +       [EXTCON_NONE]           = "NONE",
>> +
>> +       /* USB external connector */
>>         [EXTCON_USB]            = "USB",
>> -       [EXTCON_USB_HOST]       = "USB-Host",
>> +       [EXTCON_USB_HOST]       = "USB-HOST",
>> +
>> +       /* Charger external connector */
>>         [EXTCON_TA]             = "TA",
>> -       [EXTCON_FAST_CHARGER]   = "Fast-charger",
>> -       [EXTCON_SLOW_CHARGER]   = "Slow-charger",
>> -       [EXTCON_CHARGE_DOWNSTREAM]      = "Charge-downstream",
>> +       [EXTCON_FAST_CHARGER]   = "FAST-CHARGER",
>> +       [EXTCON_SLOW_CHARGER]   = "SLOW-CHARGER",
>> +       [EXTCON_CHARGE_DOWNSTREAM] = "CHARGE-DOWNSTREAM",
>> +
>> +       /* Audio/Video external connector */
>> +       [EXTCON_LINE_IN]        = "LINE-IN",
>> +       [EXTCON_LINE_OUT]       = "LINE-OUT",
>> +       [EXTCON_MICROPHONE]     = "MICROPHONE",
>> +       [EXTCON_HEADPHONE]      = "HEADPHONE",
>> +
>>         [EXTCON_HDMI]           = "HDMI",
>>         [EXTCON_MHL]            = "MHL",
>>         [EXTCON_DVI]            = "DVI",
>>         [EXTCON_VGA]            = "VGA",
>> -       [EXTCON_DOCK]           = "Dock",
>> -       [EXTCON_LINE_IN]        = "Line-in",
>> -       [EXTCON_LINE_OUT]       = "Line-out",
>> -       [EXTCON_MIC_IN]         = "Microphone",
>> -       [EXTCON_HEADPHONE_OUT]  = "Headphone",
>> -       [EXTCON_SPDIF_IN]       = "SPDIF-in",
>> -       [EXTCON_SPDIF_OUT]      = "SPDIF-out",
>> -       [EXTCON_VIDEO_IN]       = "Video-in",
>> -       [EXTCON_VIDEO_OUT]      = "Video-out",
>> -       [EXTCON_MECHANICAL]     = "Mechanical",
>> +       [EXTCON_SPDIF_IN]       = "SPDIF-IN",
>> +       [EXTCON_SPDIF_OUT]      = "SPDIF-OUT",
>> +       [EXTCON_VIDEO_IN]       = "VIDEO-IN",
>> +       [EXTCON_VIDEO_OUT]      = "VIDEO-OUT",
>> +
>> +       /* Etc external connector */
>> +       [EXTCON_DOCK]           = "DOCK",
>> +       [EXTCON_JIG]            = "JIG",
>> +       [EXTCON_MECHANICAL]     = "MECHANICAL",
>> +
>> +       NULL,
>>  };
>
> This change does not look related to the topic. Can you split it to
> separate patch?

Do you mean about changing the name of external connectors from small
letter to capital letter?

>
>>
>>  static struct class *extcon_class;
>> @@ -118,11 +128,9 @@ static ssize_t state_show(struct device *dev, struct device_attribute *attr,
>>         if (edev->max_supported == 0)
>>                 return sprintf(buf, "%u\n", edev->state);
>>
>> -       for (i = 0; i < SUPPORTED_CABLE_MAX; i++) {
>> -               if (!edev->supported_cable[i])
>> -                       break;
>> +       for (i = 0; i < edev->max_supported; i++) {
>>                 count += sprintf(buf + count, "%s=%d\n",
>> -                                edev->supported_cable[i],
>> +                               extcon_name[edev->supported_cable[i]],
>>                                  !!(edev->state & (1 << i)));
>>         }
>>
>> @@ -171,9 +179,10 @@ static ssize_t cable_name_show(struct device *dev,
>>  {
>>         struct extcon_cable *cable = container_of(attr, struct extcon_cable,
>>                                                   attr_name);
>> +       int i = cable->cable_index;
>>
>>         return sprintf(buf, "%s\n",
>> -                      cable->edev->supported_cable[cable->cable_index]);
>> +                       extcon_name[cable->edev->supported_cable[i]]);
>>  }
>>
>>  static ssize_t cable_state_show(struct device *dev,
>> @@ -282,39 +291,57 @@ int extcon_set_state(struct extcon_dev *edev, u32 state)
>>  }
>>  EXPORT_SYMBOL_GPL(extcon_set_state);
>>
>> -/**
>> - * extcon_find_cable_index() - Get the cable index based on the cable name.
>> - * @edev:      the extcon device that has the cable.
>> - * @cable_name:        cable name to be searched.
>> - *
>> - * Note that accessing a cable state based on cable_index is faster than
>> - * cable_name because using cable_name induces a loop with strncmp().
>> - * Thus, when get/set_cable_state is repeatedly used, using cable_index
>> - * is recommended.
>> - */
>> -int extcon_find_cable_index(struct extcon_dev *edev, const char *cable_name)
>> +static int extcon_find_cable_index(struct extcon_dev *edev,
>> +                                  const char *cable_name)
>>  {
>> +       enum extcon id = EXTCON_NONE;
>>         int i;
>>
>> -       if (edev->supported_cable) {
>> -               for (i = 0; edev->supported_cable[i]; i++) {
>> -                       if (!strncmp(edev->supported_cable[i],
>> -                               cable_name, CABLE_NAME_MAX))
>> -                               return i;
>> +       if (edev->max_supported == 0)
>> +               return -EINVAL;
>> +
>> +       /* Find the the number of extcon cable */
>> +       for (i = 0; i < __EXTCON_END; i++) {
>> +               if (!extcon_name[i])
>> +                       continue;
>> +               if (!strncmp(extcon_name[i], cable_name, CABLE_NAME_MAX)) {
>> +                       id = i;
>> +                       break;
>>                 }
>>         }
>>
>> +       if (id == EXTCON_NONE)
>> +               return -EINVAL;
>> +
>> +       /* Find the the index of extcon cable in edev->supported_cable */
>> +       for (i = 0; i < edev->max_supported; i++) {
>> +               if (edev->supported_cable[i] == id)
>> +                       return i;
>> +       }
>> +
>>         return -EINVAL;
>>  }
>> -EXPORT_SYMBOL_GPL(extcon_find_cable_index);
>>
>>  /**
>>   * extcon_get_cable_state_() - Get the status of a specific cable.
>>   * @edev:      the extcon device that has the cable.
>> - * @index:     cable index that can be retrieved by extcon_find_cable_index().
>> + * @id:                the unique id of each external connector in extcon enumeration.
>>   */
>> -int extcon_get_cable_state_(struct extcon_dev *edev, int index)
>> +int extcon_get_cable_state_(struct extcon_dev *edev, const enum extcon id)
>>  {
>> +       int i, index = -EINVAL;
>> +
>> +       /* Find the the index of extcon cable in edev->supported_cable */
>> +       for (i = 0; edev->max_supported < i; i++) {
>> +               if (edev->supported_cable[i] == id) {
>> +                       index = i;
>> +                       break;
>> +               }
>> +       }
>> +
>> +       if (i == edev->max_supported)
>> +               return -EINVAL;
>> +
>>         if (index < 0 || (edev->max_supported && edev->max_supported <= index))
>>                 return -EINVAL;
>>
>> @@ -339,15 +366,27 @@ EXPORT_SYMBOL_GPL(extcon_get_cable_state);
>>  /**
>>   * extcon_set_cable_state_() - Set the status of a specific cable.
>>   * @edev:              the extcon device that has the cable.
>> - * @index:             cable index that can be retrieved by
>> - *                     extcon_find_cable_index().
>> - * @cable_state:       the new cable status. The default semantics is
>> + * @id:                        the unique id of each external connector
>> + *                     in extcon enumeration.
>> + * @state:             the new cable status. The default semantics is
>>   *                     true: attached / false: detached.
>>   */
>> -int extcon_set_cable_state_(struct extcon_dev *edev,
>> -                       int index, bool cable_state)
>> +int extcon_set_cable_state_(struct extcon_dev *edev, enum extcon id,
>> +                               bool cable_state)
>>  {
>>         u32 state;
>> +       int i, index = -EINVAL;
>> +
>> +       /* Find the the index of extcon cable in edev->supported_cable */
>> +       for (i = 0; i < edev->max_supported; i++) {
>> +               if (edev->supported_cable[i] == id) {
>> +                       index = i;
>> +                       break;
>> +               }
>> +       }
>
> This loop shows in few places, maybe split it to separate static
> function? Just to avoid duplication of code.

OK.

>
>> +
>> +       if (i == edev->max_supported)
>> +               return -EINVAL;
>>
>>         if (index < 0 || (edev->max_supported && edev->max_supported <= index))
>>                 return -EINVAL;
>> @@ -605,7 +644,7 @@ static void dummy_sysfs_dev_release(struct device *dev)
>>   *
>>   * Return the pointer of extcon device if success or ERR_PTR(err) if fail
>>   */
>> -struct extcon_dev *extcon_dev_allocate(const char **supported_cable)
>> +struct extcon_dev *extcon_dev_allocate(const enum extcon *supported_cable)
>
> I think you also have to update the documentation. At least for
> {devm}_extcon_dev_allocate but maybe in other places too. Previously
> the documentation states that supported_cable is an array of strings.
> Additionally AFAIU now it must end with EXTCON_NONE. This
> sentinel-like info must be clearly documented.

OK.

>
>>  {
>>         struct extcon_dev *edev;
>>
>> @@ -659,7 +698,7 @@ static void devm_extcon_dev_release(struct device *dev, void *res)
>>   * or ERR_PTR(err) if fail
>>   */
>>  struct extcon_dev *devm_extcon_dev_allocate(struct device *dev,
>> -                                           const char **supported_cable)
>> +                                       const enum extcon *supported_cable)
>>  {
>>         struct extcon_dev **ptr, *edev;
>>
>> @@ -709,17 +748,15 @@ int extcon_dev_register(struct extcon_dev *edev)
>>                         return ret;
>>         }
>>
>> -       if (edev->supported_cable) {
>> -               /* Get size of array */
>> -               for (index = 0; edev->supported_cable[index]; index++)
>> -                       ;
>> -               edev->max_supported = index;
>> -       } else {
>> -               edev->max_supported = 0;
>> -       }
>> +       if (!edev->supported_cable)
>> +               return -EINVAL;
>>
>> +       for (; edev->supported_cable[index] != EXTCON_NONE; index++);
>> +
>> +       edev->max_supported = index;
>>         if (index > SUPPORTED_CABLE_MAX) {
>> -               dev_err(&edev->dev, "extcon: maximum number of supported cables exceeded.\n");
>> +               dev_err(&edev->dev,
>> +                       "exceed the maximum number of supported cables\n");
>>                 return -EINVAL;
>>         }
>>
>> @@ -1070,6 +1107,7 @@ static void __exit extcon_class_exit(void)
>>  }
>>  module_exit(extcon_class_exit);
>>
>> +MODULE_AUTHOR("Chanwoo Choi <cw00.choi@samsung.com>");
>>  MODULE_AUTHOR("Mike Lockwood <lockwood@android.com>");
>>  MODULE_AUTHOR("Donggeun Kim <dg77.kim@samsung.com>");
>>  MODULE_AUTHOR("MyungJoo Ham <myungjoo.ham@samsung.com>");
>> diff --git a/include/linux/extcon.h b/include/linux/extcon.h
>> index 799474d9d..de158a1 100644
>> --- a/include/linux/extcon.h
>> +++ b/include/linux/extcon.h
>> @@ -1,6 +1,9 @@
>>  /*
>>   *  External connector (extcon) class driver
>>   *
>> + * Copyright (C) 2015 Samsung Electronics
>> + * Author: Chanwoo Choi <cw00.choi@samsung.com>
>> + *
>>   * Copyright (C) 2012 Samsung Electronics
>>   * Author: Donggeun Kim <dg77.kim@samsung.com>
>>   * Author: MyungJoo Ham <myungjoo.ham@samsung.com>
>> @@ -27,8 +30,41 @@
>>  #include <linux/notifier.h>
>>  #include <linux/sysfs.h>
>>
>> -#define SUPPORTED_CABLE_MAX    32
>> -#define CABLE_NAME_MAX         30
>> +enum extcon {
>> +       EXTCON_NONE             = 0x0,  /* NONE */
>> +
>> +       /* USB external connector */
>> +       EXTCON_USB              = 0x1,  /* USB */
>> +       EXTCON_USB_HOST         = 0x2,  /* USB-HOST */
>> +
>> +       /* Charger external connector */
>> +       EXTCON_TA               = 0x10, /* TA */
>> +       EXTCON_FAST_CHARGER     = 0x11, /* FAST-CHARGER */
>> +       EXTCON_SLOW_CHARGER     = 0x12, /* SLOW-CHARGER */
>> +       EXTCON_CHARGE_DOWNSTREAM= 0x13, /* CHARGE-DOWNSTREAM */
>
> Space before '='. I know it will mess the alignment in indentation but
> it would be still more readable.

OK.

>
>> +
>> +       /* Audio/Video external connector */
>> +       EXTCON_LINE_IN          = 0x20, /* LINE-IN */
>> +       EXTCON_LINE_OUT         = 0x21, /* LINE-OUT */
>> +       EXTCON_MICROPHONE       = 0x22, /* MICROPHONE */
>> +       EXTCON_HEADPHONE        = 0x23, /* HEADPHONE */
>> +
>> +       EXTCON_HDMI             = 0x30, /* HDMI */
>> +       EXTCON_MHL              = 0x31, /* MHL */
>> +       EXTCON_DVI              = 0x32, /* DVI */
>> +       EXTCON_VGA              = 0x33, /* VGA */
>> +       EXTCON_SPDIF_IN         = 0x34, /* SPDIF-IN */
>> +       EXTCON_SPDIF_OUT        = 0x35, /* SPDIF-OUT */
>> +       EXTCON_VIDEO_IN         = 0x36, /* VIDEO-IN */
>> +       EXTCON_VIDEO_OUT        = 0x37, /* VIDEO-OUT */
>> +
>> +       /* Etc external connector */
>> +       EXTCON_DOCK             = 0x50, /* DOCK */
>> +       EXTCON_JIG              = 0x51, /* JIG */
>> +       EXTCON_MECHANICAL       = 0x52, /* MECHANICAL */
>> +
>
> There is no benefit of each comment here. The comment just duplicates
> name, it does not give any additional information. Get rid of it.

OK.

>
>> +       __EXTCON_END,
>
> Why "__" prefix?

This variable should be used in extcon core. So, I just add '__' prefix.
But, It is not clear. I'll define it without '__' prefix.

>
>> +};
>>
>>  /*
>>   * The standard cable name is to help support general notifier
>> @@ -48,29 +84,6 @@
>>   * you don't need such convention. This convention is helpful when
>>   * notifier can distinguish but notifiee cannot.
>>   */
>
> Isn't the comment above related to the enum "extcon_cable_name" which
> you are removing?

OK. I'll modify the upper comment.

Thanks,
Chanwoo Choi