| 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: <57980B2D.7090307@rock-chips.com>
Date: Wed, 27 Jul 2016 09:15:25 +0800
From: Chris Zhong <zyw@...k-chips.com>
To: chanwoo@...nel.org, Guenter Roeck <groeck@...gle.com>
Cc: Chanwoo Choi <cw00.choi@...sung.com>,
linux-kernel <linux-kernel@...r.kernel.org>,
"myungjoo.ham@...sung.com" <myungjoo.ham@...sung.com>,
Guenter Roeck <groeck@...omium.org>
Subject: Re: [PATCH 2/6] extcon: Add the support for extcon property according
to extcon type
Hi Chanwoo
On 07/27/2016 08:31 AM, Chanwoo Choi wrote:
> Hi Guenter,
>
> 2016년 7월 27일 수요일, Guenter Roeck<groeck@...gle.com
> <mailto:groeck@...gle.com>>님이 작성한 메시지:
>
> On Tue, Jul 26, 2016 at 5:09 AM, Chanwoo Choi
> <cw00.choi@...sung.com <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@...sung.com <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
Powered by blists - more mailing lists