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

From: Chanwoo Choi
Date: Sun May 17 2015 - 04:14:14 EST

Next message: Krzysztof Kozlowski: "[PATCH 3/4] ARM: multi_v7_defconfig: Enable TMU for Exynos"
Previous message: Krzysztof Kozlowski: "[PATCH v6 04/13] ARM: dts: Use labels for overriding nodes in exynos4210-smdkv310"
In reply to: Krzysztof Kozlowski: "Re: [PATCH 1/2] extcon: Use the unique id for external connector instead of string"
Next in thread: Krzysztof Kozlowski: "Re: [PATCH 1/2] extcon: Use the unique id for external connector instead of string"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Hi Krzysztof,

On Sun, May 17, 2015 at 4:10 PM, Krzysztof Kozlowski
<k.kozlowski@xxxxxxxxxxx> wrote:
> 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/

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

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

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

Next message: Krzysztof Kozlowski: "[PATCH 3/4] ARM: multi_v7_defconfig: Enable TMU for Exynos"
Previous message: Krzysztof Kozlowski: "[PATCH v6 04/13] ARM: dts: Use labels for overriding nodes in exynos4210-smdkv310"
In reply to: Krzysztof Kozlowski: "Re: [PATCH 1/2] extcon: Use the unique id for external connector instead of string"
Next in thread: Krzysztof Kozlowski: "Re: [PATCH 1/2] extcon: Use the unique id for external connector instead of string"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]