Re: [PATCH 2/6] extcon: Add the support for extcon property according to extcon type

From: Chris Zhong
Date: Tue Jul 26 2016 - 21:15:47 EST

Next message: kbuild test robot: "Re: [PATCH v3 2/7] Split up struct warn_args"
Previous message: Alan Curry: "Re: PROBLEM: network data corruption (bisected to e5a4b0bb803b)"
In reply to: Guenter Roeck: "Re: [PATCH 2/6] extcon: Add the support for extcon property according to extcon type"
Next in thread: Guenter Roeck: "Re: [PATCH 2/6] extcon: Add the support for extcon property according to extcon type"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Hi Chanwoo

On 07/27/2016 08:31 AM, Chanwoo Choi wrote:

Hi Guenter,

2016ë 7ì 27ì ììì, Guenter Roeck<groeck@xxxxxxxxxx <mailto:groeck@xxxxxxxxxx>>ëì ììí ëìì:

On Tue, Jul 26, 2016 at 5:09 AM, Chanwoo Choi
<cw00.choi@xxxxxxxxxxx <javascript:;>> wrote:
> This patch support the extcon property for the external connector
> because each external connector might have the property according to
> the H/W design and the specific characteristics.
>
> - EXTCON_PROP_USB_[property name]
> - EXTCON_PROP_CHG_[property name]
> - EXTCON_PROP_JACK_[property name]
> - EXTCON_PROP_DISP_[property name]
>
> Add the new extcon APIs to get/set the property value as following:
> - int extcon_get_property(struct extcon_dev *edev, unsigned int id,
> unsigned int prop,
> union extcon_property_value *prop_val)
> - int extcon_set_property(struct extcon_dev *edev, unsigned int id,
> unsigned int prop,
> union extcon_property_value prop_val)
>
> Signed-off-by: Chanwoo Choi <cw00.choi@xxxxxxxxxxx <javascript:;>>
> ---
> drivers/extcon/extcon.c | 206
+++++++++++++++++++++++++++++++++++++++++++++++-
> include/linux/extcon.h | 86 ++++++++++++++++++++
> 2 files changed, 291 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/extcon/extcon.c b/drivers/extcon/extcon.c
> index b1e6ee6194bc..2317aaea063f 100644
> --- a/drivers/extcon/extcon.c
> +++ b/drivers/extcon/extcon.c
> @@ -196,6 +196,11 @@ struct extcon_cable {
> struct device_attribute attr_state;
>
> struct attribute *attrs[3]; /* to be fed to attr_g.attrs */
> +
> + union extcon_property_value
usb_propval[EXTCON_PROP_USB_CNT];
> + union extcon_property_value
chg_propval[EXTCON_PROP_CHG_CNT];
> + union extcon_property_value
jack_propval[EXTCON_PROP_JACK_CNT];
> + union extcon_property_value
disp_propval[EXTCON_PROP_DISP_CNT];
> };
>
> static struct class *extcon_class;
> @@ -248,6 +253,28 @@ static int find_cable_index_by_id(struct
extcon_dev *edev, const unsigned int id
> return -EINVAL;
> }
>
> +static int get_extcon_type(unsigned int prop)
> +{
> + switch (prop) {
> + case EXTCON_PROP_USB_MIN ... EXTCON_PROP_USB_MAX:
> + return EXTCON_TYPE_USB;
> + case EXTCON_PROP_CHG_MIN ... EXTCON_PROP_CHG_MAX:
> + return EXTCON_TYPE_CHG;
> + case EXTCON_PROP_JACK_MIN ... EXTCON_PROP_JACK_MAX:
> + return EXTCON_TYPE_JACK;
> + case EXTCON_PROP_DISP_MIN ... EXTCON_PROP_DISP_MAX:
> + return EXTCON_TYPE_DISP;
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +static bool is_extcon_attached(struct extcon_dev *edev,
unsigned int id,
> + unsigned int index)
> +{
> + return ((!!(edev->state & (1 << index))) == 1) ? true :
false;
> +}
> +
> static bool is_extcon_changed(u32 prev, u32 new, int idx, bool
*attached)
> {
> if (((prev >> idx) & 0x1) != ((new >> idx) & 0x1)) {
> @@ -258,6 +285,41 @@ static bool is_extcon_changed(u32 prev, u32
new, int idx, bool *attached)
> return false;
> }
>
> +static bool is_extcon_property_supported(unsigned int id,
unsigned int prop)
> +{
> + unsigned int type;
> +

int

ok.

> + /* Check whether the property is supported or not. */
> + type = get_extcon_type(prop);
> + if (type < 0)

otherwise type is never < 0

you're right.

> + return false;
> +
> + /* Check whether a specific extcon id supports the
property or not. */
> + if (extcon_info[id].type | type)

This is always true ?

It is my mistake. Use '&' instead of '|'.

> + return true;
> +
> + return false;
> +}
> +
> +#define INIT_PROPERTY(name, name_lower, type) \
> + if (EXTCON_TYPE_##name || type) { \

This is always true ?

It is my mistake. Use '&' instead of '||'.

> + for (i = 0; i < EXTCON_PROP_##name##_CNT; i++) \
> + cable->name_lower##_propval[i] = val; \
> + } \
> +
> +static void init_property(struct extcon_dev *edev, unsigned int
id, int index)
> +{
> + unsigned int type = extcon_info[id].type;
> + struct extcon_cable *cable = &edev->cables[index];
> + union extcon_property_value val = (union
extcon_property_value)(0);
> + int i;
> +
> + INIT_PROPERTY(USB, usb, type);
> + INIT_PROPERTY(CHG, chg, type);
> + INIT_PROPERTY(JACK, jack, type);
> + INIT_PROPERTY(DISP, disp, type);
> +}
> +
> static ssize_t state_show(struct device *dev, struct
device_attribute *attr,
> char *buf)
> {
> @@ -421,7 +483,7 @@ int extcon_get_cable_state_(struct
extcon_dev *edev, const unsigned int id)
> if (edev->max_supported && edev->max_supported <= index)
> return -EINVAL;
>
> - return !!(edev->state & (1 << index));
> + return (int)(is_extcon_attached(edev, id, index));
> }
> EXPORT_SYMBOL_GPL(extcon_get_cable_state_);
>
> @@ -449,12 +511,154 @@ int extcon_set_cable_state_(struct
extcon_dev *edev, unsigned int id,
> if (edev->max_supported && edev->max_supported <= index)
> return -EINVAL;
>
> + /*
> + * Initialize the value of extcon property before setting
> + * the detached state for an external connector.
> + */
> + if (!cable_state)
> + init_property(edev, id, index);
> +
> + /* Set the state for external connector as the detached
state. */
> state = cable_state ? (1 << index) : 0;
> return extcon_update_state(edev, 1 << index, state);
> }
> EXPORT_SYMBOL_GPL(extcon_set_cable_state_);
>
> /**
> + * extcon_get_property() - Get the property value of a specific
cable.
> + * @edev: the extcon device that has the cable.
> + * @id: the unique id of each external
connector
> + * in extcon enumeration.
> + * @prop: the property id among enum extcon_property.
> + * @prop_val: the pointer which store the value of
property.
> + *
> + * When getting the property value of external connector, the
external connector
> + * should be attached. If detached state, function just return
0 without
> + * property value. Also, the each property should be included
in the list of
> + * supported properties according to the type of external
connectors.
> + *
> + * Returns 0 if success or error number if fail
> + */
> +int extcon_get_property(struct extcon_dev *edev, unsigned int id,
> + unsigned int prop,
> + union extcon_property_value
*prop_val)
> +{
> + struct extcon_cable *cable;
> + unsigned long flags;
> + int index, ret = 0;
> +
> + *prop_val = (union extcon_property_value)(0);
> +
> + if (!edev)
> + return -EINVAL;
> +
> + /* Check whether the property is supported or not */
> + if (!is_extcon_property_supported(id, prop))
> + return -EINVAL;
> +
> + /* Find the cable index of external connector by using id */
> + index = find_cable_index_by_id(edev, id);
> + if (index < 0)
> + return index;
> +
> + /*
> + * Check whether the external connector is attached.
> + * If external connector is detached, the user can not
> + * get the property value.
> + */
> + if (!is_extcon_attached(edev, id, index))
> + return 0;
> +
> + cable = &edev->cables[index];
> + spin_lock_irqsave(&edev->lock, flags);
> +
> + /* Get the property value according to extcon type */
> + switch (prop) {
> + case EXTCON_PROP_USB_MIN ... EXTCON_PROP_USB_MAX:
> + *prop_val = cable->usb_propval[prop -
EXTCON_PROP_USB_MIN];
> + break;
> + case EXTCON_PROP_CHG_MIN ... EXTCON_PROP_CHG_MAX:
> + *prop_val = cable->chg_propval[prop -
EXTCON_PROP_CHG_MIN];
> + break;
> + case EXTCON_PROP_JACK_MIN ... EXTCON_PROP_JACK_MAX:
> + *prop_val = cable->jack_propval[prop -
EXTCON_PROP_JACK_MIN];
> + break;
> + case EXTCON_PROP_DISP_MIN ... EXTCON_PROP_DISP_MAX:
> + *prop_val = cable->disp_propval[prop -
EXTCON_PROP_DISP_MIN];
> + break;
> + default:
> + ret = -EINVAL;
> + break;
> + }
> +
> + spin_unlock_irqrestore(&edev->lock, flags);
> +
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(extcon_get_property);
> +
> +/**
> + * extcon_set_property() - Set the property value of a specific
cable.
> + * @edev: the extcon device that has the cable.
> + * @id: the unique id of each external
connector
> + * in extcon enumeration.
> + * @prop: the property id among enum extcon_property.
> + * @prop_val: the pointer including the new value of
property.
> + *
> + * The each property should be included in the list of
supported properties
> + * according to the type of external connectors.
> + *
> + * Returns 0 if success or error number if fail
> + */
> +int extcon_set_property(struct extcon_dev *edev, unsigned int id,
> + unsigned int prop,
> + union extcon_property_value
prop_val)
> +{
> + struct extcon_cable *cable;
> + unsigned long flags;
> + int index, ret = 0;
> +
> + if (!edev)
> + return -EINVAL;
> +
> + /* Check whether the property is supported or not */
> + if (!is_extcon_property_supported(id, prop))
> + return -EINVAL;
> +
> + /* Find the cable index of external connector by using id */
> + index = find_cable_index_by_id(edev, id);
> + if (index < 0)
> + return index;
> +
> + cable = &edev->cables[index];
> + spin_lock_irqsave(&edev->lock, flags);
> +
> + /* Set the property value according to extcon type */
> + switch (prop) {
> + case EXTCON_PROP_USB_MIN ... EXTCON_PROP_USB_MAX:
> + cable->usb_propval[prop - EXTCON_PROP_USB_MIN] =
prop_val;
> + break;
> + case EXTCON_PROP_CHG_MIN ... EXTCON_PROP_CHG_MAX:
> + cable->chg_propval[prop - EXTCON_PROP_CHG_MIN] =
prop_val;
> + break;
> + case EXTCON_PROP_JACK_MIN ... EXTCON_PROP_JACK_MAX:
> + cable->jack_propval[prop - EXTCON_PROP_JACK_MIN]
= prop_val;
> + break;
> + case EXTCON_PROP_DISP_MIN ... EXTCON_PROP_DISP_MAX:
> + cable->disp_propval[prop - EXTCON_PROP_DISP_MIN]
= prop_val;
> + break;
> + default:
> + ret = -EINVAL;
> + break;
> + }
> +
> + spin_unlock_irqrestore(&edev->lock, flags);
> +
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(extcon_set_property);
> +
> +/**
> * extcon_get_extcon_dev() - Get the extcon device instance
from the name
> * @extcon_name: The extcon name provided with
extcon_dev_register()
> */
> diff --git a/include/linux/extcon.h b/include/linux/extcon.h
> index 46d802892c82..296d1452dcb4 100644
> --- a/include/linux/extcon.h
> +++ b/include/linux/extcon.h
> @@ -77,6 +77,68 @@
>
> #define EXTCON_NUM 63
>
> +/*
> + * Define the property of supported external connectors.
> + *
> + * When adding the new extcon property, they *must* have
> + * the type/value/default information. Also, you *have to*
> + * modify the EXTCON_PROP_[type]_START/END definitions
> + * which mean the range of the supported properties
> + * for each extcon type.
> + *
> + * The naming style of property
> + * : EXTCON_PROP_[type]_[property name]
> + *
> + * EXTCON_PROP_USB_[property name] : USB property
> + * EXTCON_PROP_CHG_[property name] : Charger property
> + * EXTCON_PROP_JACK_[property name] : Jack property
> + * EXTCON_PROP_DISP_[property name] : Display property
> + */
> +
> +/*
> + * Properties of EXTCON_TYPE_USB.
> + *
> + * - EXTCON_PROP_USB_ID
> + * @type: integer (intval)
> + * @value: 0 (low) or 1 (high)
> + * @default: 0 (low)
> + * - EXTCON_PROP_USB_VBUS
> + * @type: integer (intval)
> + * @value: 0 (low) or 1 (high)
> + * @default: 0 (low)
> + */
> +#define EXTCON_PROP_USB_ID 0
> +#define EXTCON_PROP_USB_VBUS 1
> +
> +#define EXTCON_PROP_USB_MIN 0
> +#define EXTCON_PROP_USB_MAX 1
> +#define EXTCON_PROP_USB_CNT (EXTCON_PROP_USB_MAX -
EXTCON_PROP_USB_MIN)

EXTCON_PROP_USB_MAX - EXTCON_PROP_USB_MIN + 1

Otherwise the array won't have enough entries, and writing the last
property will end up overwriting usb_bits (because all other arrays
have a size of 0)

You're right. I'll fix it.

> +
> +/* Properties of EXTCON_TYPE_CHG. */
> +#define EXTCON_PROP_CHG_MIN 50
> +#define EXTCON_PROP_CHG_MAX 50
> +#define EXTCON_PROP_CHG_CNT (EXTCON_PROP_CHG_MAX -
EXTCON_PROP_CHG_MIN)

EXTCON_PROP_CHG_MAX - EXTCON_PROP_CHG_MIN + 1

Otherwise the array won't have any entries.

ok.

> +/* Properties of EXTCON_TYPE_JACK. */
> +#define EXTCON_PROP_JACK_MIN 100
> +#define EXTCON_PROP_JACK_MAX 100
> +#define EXTCON_PROP_JACK_CNT (EXTCON_PROP_JACK_MAX -
EXTCON_PROP_JACK_MIN)

+ 1

ok.

> +
> +/* Properties of EXTCON_TYPE_DISP. */
> +#define EXTCON_PROP_DISP_MIN 150
> +#define EXTCON_PROP_DISP_MAX 150
> +#define EXTCON_PROP_DISP_CNT (EXTCON_PROP_DISP_MAX -
EXTCON_PROP_DISP_MIN)

+ 1

ok.

Tested with these "+1", it works for my DP patch.

> +
> +/*
> + * Define the type of property's value.
> + *
> + * Define the property's value as union type. Because each property
> + * would need the different data type to store it.
> + */
> +union extcon_property_value {
> + int intval; /* type : interger (intval) */
> +};
> +
> struct extcon_cable;
>
> /**
> @@ -167,6 +229,17 @@ extern int extcon_set_cable_state_(struct
extcon_dev *edev, unsigned int id,
> bool cable_state);
>
> /*
> + * get/set_property access the property value of each external
connector.
> + * They are used to access the property of each cable based on
the property id.
> + */
> +extern int extcon_get_property(struct extcon_dev *edev,
unsigned int id,
> + unsigned int prop,
> + union extcon_property_value
*prop_val);
> +extern int extcon_set_property(struct extcon_dev *edev,
unsigned int id,
> + unsigned int prop,
> + union extcon_property_value
prop_val);
> +
> +/*
> * Following APIs are to monitor every action of a notifier.
> * Registrar gets notified for every external port of a
connection device.
> * Probably this could be used to debug an action of notifier;
however,
> @@ -239,6 +312,19 @@ static inline int
extcon_set_cable_state_(struct extcon_dev *edev,
> return 0;
> }
>
> +static inline int extcon_get_property(struct extcon_dev *edev,
unsigned int id,
> + unsigned int prop,
> + union
extcon_property_value *prop_val)
> +{
> + return 0;
> +}
> +static inline int extcon_set_property(struct extcon_dev *edev,
unsigned int id,
> + unsigned int prop,
> + union
extcon_property_value prop_val)
> +{
> + return 0;
> +}
> +
> static inline struct extcon_dev *extcon_get_extcon_dev(const
char *extcon_name)
> {
> return NULL;
> --
> 1.9.1
>

Regards,
Chanwoo Choi

Next message: kbuild test robot: "Re: [PATCH v3 2/7] Split up struct warn_args"
Previous message: Alan Curry: "Re: PROBLEM: network data corruption (bisected to e5a4b0bb803b)"
In reply to: Guenter Roeck: "Re: [PATCH 2/6] extcon: Add the support for extcon property according to extcon type"
Next in thread: Guenter Roeck: "Re: [PATCH 2/6] extcon: Add the support for extcon property according to extcon type"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]