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

[Krzysztof Kozlowski]

2015-05-15 23:31 GMT+09:00 Chanwoo Choi <cw00.choi@xxxxxxxxxxx>:
> 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/

> 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@xxxxxxxxxxx>
> + *
>   * Copyright (C) 2012 Samsung Electronics
>   * Author: Donggeun Kim <dg77.kim@xxxxxxxxxxx>
>   * Author: MyungJoo Ham <myungjoo.ham@xxxxxxxxxxx>
> @@ -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?

> +#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?

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

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

>  {
>         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@xxxxxxxxxxx>");
>  MODULE_AUTHOR("Mike Lockwood <lockwood@xxxxxxxxxxx>");
>  MODULE_AUTHOR("Donggeun Kim <dg77.kim@xxxxxxxxxxx>");
>  MODULE_AUTHOR("MyungJoo Ham <myungjoo.ham@xxxxxxxxxxx>");
> 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@xxxxxxxxxxx>
> + *
>   * Copyright (C) 2012 Samsung Electronics
>   * Author: Donggeun Kim <dg77.kim@xxxxxxxxxxx>
>   * Author: MyungJoo Ham <myungjoo.ham@xxxxxxxxxxx>
> @@ -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.

> +
> +       /* 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.

> +       __EXTCON_END,

Why "__" 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?

> -enum extcon_cable_name {
> -       EXTCON_USB = 0,
> -       EXTCON_USB_HOST,
> -       EXTCON_TA,                      /* Travel Adaptor */
> -       EXTCON_FAST_CHARGER,
> -       EXTCON_SLOW_CHARGER,
> -       EXTCON_CHARGE_DOWNSTREAM,       /* Charging an external device */
> -       EXTCON_HDMI,
> -       EXTCON_MHL,
> -       EXTCON_DVI,
> -       EXTCON_VGA,
> -       EXTCON_DOCK,
> -       EXTCON_LINE_IN,
> -       EXTCON_LINE_OUT,
> -       EXTCON_MIC_IN,
> -       EXTCON_HEADPHONE_OUT,
> -       EXTCON_SPDIF_IN,
> -       EXTCON_SPDIF_OUT,
> -       EXTCON_VIDEO_IN,
> -       EXTCON_VIDEO_OUT,
> -       EXTCON_MECHANICAL,
> -};


Best regards,
Krzysztof
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/