From: Chris Zhong <zyw@rock-chips.com>
To: chanwoo@kernel.org, Guenter Roeck <groeck@google.com>
Cc: Chanwoo Choi <cw00.choi@samsung.com>,
linux-kernel <linux-kernel@vger.kernel.org>,
"myungjoo.ham@samsung.com" <myungjoo.ham@samsung.com>,
Guenter Roeck <groeck@chromium.org>
Subject: Re: [PATCH 2/6] extcon: Add the support for extcon property according to extcon type
Date: Wed, 27 Jul 2016 09:15:25 +0800 [thread overview]
Message-ID: <57980B2D.7090307@rock-chips.com> (raw)
In-Reply-To: <CAGTfZH2rPyhSiXU=62RKTaE40fok5iiWM+G=+V147gGq8wv_Bw@mail.gmail.com>
Hi Chanwoo
On 07/27/2016 08:31 AM, Chanwoo Choi wrote:
> Hi Guenter,
>
> 2016년 7월 27일 수요일, Guenter Roeck<groeck@google.com
> <mailto:groeck@google.com>>님이 작성한 메시지:
>
> On Tue, Jul 26, 2016 at 5:09 AM, Chanwoo Choi
> <cw00.choi@samsung.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@samsung.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
next prev parent reply other threads:[~2016-07-27 1:15 UTC|newest]
Thread overview: 21+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-07-26 12:09 [PATCH 0/6] extcon: Add the support for extcon type and property Chanwoo Choi
2016-07-26 12:09 ` [PATCH 1/6] extcon: Add the extcon_type to group each connector into five category Chanwoo Choi
2016-07-26 12:09 ` [PATCH 2/6] extcon: Add the support for extcon property according to extcon type Chanwoo Choi
2016-07-26 22:06 ` Guenter Roeck
[not found] ` <CAGTfZH2rPyhSiXU=62RKTaE40fok5iiWM+G=+V147gGq8wv_Bw@mail.gmail.com>
2016-07-27 1:15 ` Chris Zhong [this message]
2016-07-27 1:44 ` Guenter Roeck
2016-07-27 2:09 ` Chris Zhong
2016-07-27 3:42 ` Chanwoo Choi
2016-07-27 3:51 ` Guenter Roeck
2016-07-27 3:57 ` Chanwoo Choi
2016-07-27 4:30 ` Chris Zhong
2016-07-27 17:24 ` Guenter Roeck
2016-07-29 7:02 ` Chanwoo Choi
2016-07-26 12:09 ` [PATCH 3/6] extcon: Add the support for the capability of each property Chanwoo Choi
2016-07-26 12:09 ` [PATCH 4/6] extcon: Rename the extcon_set/get_state() to maintain the function naming pattern Chanwoo Choi
2016-07-26 12:09 ` [PATCH 5/6] extcon: Add the synchronization extcon APIs to support the notification Chanwoo Choi
2016-07-27 7:30 ` Chris Zhong
2016-07-27 7:41 ` Chanwoo Choi
2016-07-26 12:09 ` [PATCH 6/6] extcon: Add EXTCON_DISP_DP and the property for USB Type-C Chanwoo Choi
[not found] ` <CGME20160726120955epcas1p1379233158fe6612133799eb2255a5247@epcas1p1.samsung.com>
2016-07-27 4:27 ` [PATCH 1/6] extcon: Add the extcon_type to group each connector into five category MyungJoo Ham
2016-07-27 5:35 ` Chanwoo Choi
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=57980B2D.7090307@rock-chips.com \
--to=zyw@rock-chips.com \
--cc=chanwoo@kernel.org \
--cc=cw00.choi@samsung.com \
--cc=groeck@chromium.org \
--cc=groeck@google.com \
--cc=linux-kernel@vger.kernel.org \
--cc=myungjoo.ham@samsung.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.