| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CABXOdTefk+CEuPVOwrLOxqmM2JGb-YooGZ-hd-+2r7w2E3=iGQ@mail.gmail.com>
Date: Wed, 27 Jul 2016 10:24:59 -0700
From: Guenter Roeck <groeck@...gle.com>
To: Chanwoo Choi <cw00.choi@...sung.com>
Cc: linux-kernel <linux-kernel@...r.kernel.org>,
myungjoo.ham@...sung.com, Chris Zhong <zyw@...k-chips.com>,
Guenter Roeck <groeck@...omium.org>, cwchoi00@...il.com
Subject: Re: [PATCH 2/6] extcon: Add the support for extcon property according
to extcon type
On Tue, Jul 26, 2016 at 5:09 AM, Chanwoo Choi <cw00.choi@...sung.com> 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@...sung.com>
> ---
> 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,
'id' isn't used.
> + unsigned int index)
> +{
> + return ((!!(edev->state & (1 << index))) == 1) ? true : false;
Minor comment: This is identical to
return !!(edev->state & (1 << index));
or, with bitops
return !!(edev->state & BIT(index));
> +}
> +
> 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;
> +
> + /* Check whether the property is supported or not. */
> + type = get_extcon_type(prop);
> + if (type < 0)
> + return false;
> +
> + /* Check whether a specific extcon id supports the property or not. */
> + if (extcon_info[id].type | type)
> + return true;
> +
> + return false;
simpler:
return !!(extcon_info[id].type & type);
Strictly speaking, the !! isn't even necessary in those mask
operations since C auto-converts to bool, though people sometimes get
confused by that.
> +}
> +
> +#define INIT_PROPERTY(name, name_lower, type) \
> + if (EXTCON_TYPE_##name || type) { \
> + 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);
Just wondering if this would be a bit cleaner/simpler.
switch(type) {
case EXTCON_TYPE_USB:
memset(cable->usb_propval, sizeof(cable->usb_propval), 0);
break;
case EXTCON_TYPE_CHG:
memset(cable->chg_propval, sizeof(cable->chg_propval), 0);
break;
case EXTCON_TYPE_JACK:
memset(cable->jack_propval, sizeof(cable->jack_propval), 0);
break;
case EXTCON_TYPE_DISP:
memset(cable->disp_propval, sizeof(cable->disp_propval), 0);
break;
}
> +}
> +
> 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;
> +
Wonder if this should be inside the lock. Otherwise the cable state
might change after the check, but before the lock is acquired.
> + 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)
> +
> +/* 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)
> +
> +/* 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)
> +
> +/* 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)
> +
> +/*
> + * 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
>
Powered by blists - more mailing lists