Messages in this thread | | | Date | Wed, 27 May 2015 17:38:27 +0300 | From | Roger Quadros <> | Subject | Re: [PATCH v2 1/2] extcon: Add extcon_set_cable_line_state() to inform the additional state of external connectors |
| |
Chanwoo,
On 27/05/15 15:15, Chanwoo Choi wrote: > This patch adds the extcon_set_cable_line_state() function to inform > the additional state of each external connector and 'enum extcon_line_state' > enumeration which include the specific states of each external connector. > > The each external connector might need the different line state. So, current > 'extcon_line_state' enumeration contains the specific state for USB as > following: > > - Following the state mean the state of both ID and VBUS line for USB: > enum extcon_line_state { > EXTCON_USB_ID_LOW = BIT(1), /* ID line is low. */ > EXTCON_USB_ID_HIGH = BIT(2), /* ID line is high. */
ID_LOW and ID_HIGH cannot happen simultaneously so they can use the same bit position. if the bit is set means it is high, if it is cleared means it is low.
> EXTCON_USB_VBUS_LOW = BIT(3), /* VBUS line is low. */ > EXTCON_USB_VBUS_HIGH = BIT(4), /* VBUS line is high. */
same here.
enum doesn't look like the right type for extcon_line_state as it is more of a bitmask.
> }; > > Cc: Myungjoo Ham <cw00.choi@samsung.com> > Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com> > --- > drivers/extcon/extcon.c | 74 ++++++++++++++++++++++++++++++++++++++++++++++++- > include/linux/extcon.h | 24 ++++++++++++++++ > 2 files changed, 97 insertions(+), 1 deletion(-) > > diff --git a/drivers/extcon/extcon.c b/drivers/extcon/extcon.c > index 5099c11..142bf0f 100644 > --- a/drivers/extcon/extcon.c > +++ b/drivers/extcon/extcon.c > @@ -279,7 +279,9 @@ int extcon_update_state(struct extcon_dev *edev, u32 mask, u32 state) > > for (index = 0; index < edev->max_supported; index++) { > if (is_extcon_changed(edev->state, state, index, &attached)) > - raw_notifier_call_chain(&edev->nh[index], attached, edev); > + raw_notifier_call_chain(&edev->nh[index], > + attached ? EXTCON_ATTACHED : > + EXTCON_DETACHED, edev); > } > > edev->state &= ~mask; > @@ -418,6 +420,69 @@ int extcon_set_cable_state(struct extcon_dev *edev, > EXPORT_SYMBOL_GPL(extcon_set_cable_state); > > /** > + * extcon_set_cable_line_state() - Set the line state of specific cable. > + * @edev: the extcon device that has the cable. > + * @id: the unique id of each external connector. > + * @state: the line state for specific cable. > + * > + * Note that this function support the only USB connector to inform the state > + * of both ID and VBUS line until now. This function may be extended to support > + * the additional external connectors. > + * > + * If the id is EXTCON_USB, it can support only following line states: > + * - EXTCON_USB_ID_LOW > + * - EXTCON_USB_ID_HIGH, > + * - EXTCON_USB_VBUS_LOW > + * - EXTCON_USB_VBUS_HIGH > + */ > +int extcon_set_cable_line_state(struct extcon_dev *edev, enum extcon id, > + enum extcon_line_state state) > +{ > + unsigned long flags; > + unsigned long line_state; > + int ret = 0, index; > + > + index = find_cable_index_by_id(edev, id); > + if (index < 0) > + return index; > + > + spin_lock_irqsave(&edev->lock, flags); > + line_state = edev->line_state[index]; > + > + switch (id) { > + case EXTCON_USB: > + if (line_state & state) { > + dev_warn(&edev->dev, > + "0x%x state is already set for %s\n", > + state, extcon_name[id]); > + goto err; > + } > + > + if ((state & EXTCON_USB_ID_LOW) || (state & EXTCON_USB_ID_HIGH)) > + line_state &= ~(EXTCON_USB_ID_LOW | EXTCON_USB_ID_HIGH); > + > + if ((state & EXTCON_USB_VBUS_LOW) > + || (state & EXTCON_USB_VBUS_HIGH)) > + line_state &= > + ~(EXTCON_USB_VBUS_LOW | EXTCON_USB_VBUS_HIGH); > + > + line_state |= state; > + break; > + default: > + ret = -EINVAL; > + goto err; > + } > + edev->line_state[index] = line_state; > + > + ret = raw_notifier_call_chain(&edev->nh[index], line_state, edev); > +err: > + spin_unlock_irqrestore(&edev->lock, flags); > + > + return ret; > +} > +EXPORT_SYMBOL_GPL(extcon_set_cable_line_state); > + > +/** > * extcon_get_extcon_dev() - Get the extcon device instance from the name > * @extcon_name: The extcon name provided with extcon_dev_register() > */ > @@ -897,6 +962,13 @@ int extcon_dev_register(struct extcon_dev *edev) > goto err_dev; > } > > + edev->line_state = devm_kzalloc(&edev->dev, > + sizeof(*edev->line_state) * edev->max_supported, GFP_KERNEL); > + if (!edev->line_state) { > + ret = -ENOMEM; > + goto err_dev; > + } > + > for (index = 0; index < edev->max_supported; index++) > RAW_INIT_NOTIFIER_HEAD(&edev->nh[index]); > > diff --git a/include/linux/extcon.h b/include/linux/extcon.h > index be9652b..79e5073 100644 > --- a/include/linux/extcon.h > +++ b/include/linux/extcon.h > @@ -66,6 +66,19 @@ enum extcon { > EXTCON_END, > }; > > +enum extcon_line_state { > + /* Following two definition are used for whether external connectors > + * is attached or detached. */ > + EXTCON_DETACHED = 0x0, > + EXTCON_ATTACHED = 0x1, > + > + /* Following states are only used for EXTCON_USB. */ > + EXTCON_USB_ID_LOW = BIT(1), /* ID line is low. */ > + EXTCON_USB_ID_HIGH = BIT(2), /* ID line is high. */ > + EXTCON_USB_VBUS_LOW = BIT(3), /* VBUS line is low. */ > + EXTCON_USB_VBUS_HIGH = BIT(4), /* VBUS line is high. */ > +};
Why do you use enum? How about the following bit definitions for line state.
#define EXTCON_ATTACHED_DETACHED_BIT BIT(0) #define EXTCON_USB_ID_BIT BIT(1) #define EXTCON_USB_VBUS_BIT BIT(2) ...
code must check if appropriate bit is set or cleared to get the high/low state.
> + > struct extcon_cable; > > /** > @@ -90,6 +103,8 @@ struct extcon_cable; > * @dev: Device of this extcon. > * @state: Attach/detach state of this extcon. Do not provide at > * register-time. > + * @line_state: Line state for each external connecotrs are included in > + * this extcon device. > * @nh: Notifier for the state change events from this extcon > * @entry: To support list of extcon devices so that users can > * search for extcon devices based on the extcon name. > @@ -121,6 +136,7 @@ struct extcon_dev { > int max_supported; > spinlock_t lock; /* could be called by irq handler */ > u32 state; > + unsigned long *line_state; > > /* /sys/class/extcon/.../cable.n/... */ > struct device_type extcon_dev_type; > @@ -217,6 +233,8 @@ extern int extcon_get_cable_state(struct extcon_dev *edev, > const char *cable_name); > extern int extcon_set_cable_state(struct extcon_dev *edev, > const char *cable_name, bool cable_state); > +extern int extcon_set_cable_line_state(struct extcon_dev *edev, enum extcon id, > + enum extcon_line_state state); > > /* > * Following APIs are for notifiees (those who want to be notified) > @@ -324,6 +342,12 @@ static inline int extcon_set_cable_state(struct extcon_dev *edev, > return 0; > } > > +static inline int extcon_set_cable_line_state(struct extcon_dev *edev, > + enum extcon id, enum extcon_line_state state) > +{ > + return 0; > +} > + > static inline struct extcon_dev *extcon_get_extcon_dev(const char *extcon_name) > { > return NULL; >
cheers, -roger
| |