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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox