Re: [PATCH 2/6] extcon: Add the support for extcon property according to extcon type

[Chanwoo Choi <cw00.choi@samsung.com>]

Hi Guenter,

On 2016년 07월 28일 02:24, Guenter Roeck wrote:
> On Tue, Jul 26, 2016 at 5:09 AM, Chanwoo Choi <cw00.choi@samsung.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@samsung.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.

I'll remove it on parameter list.

> 
>> +                               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));

I'll use the bitops as you comment.
	
	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);


OK. I'll use it as you comment.

> 
> Strictly speaking, the !! isn't even necessary in those mask
> operations since C auto-converts to bool, though people sometimes get
> confused by that.

Thanks for your explanation. For readability, I remain the '!!' operation.

> 
>> +}
>> +
>> +#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;
>         }

As you comment, I'll modify it as following:
But the each id is able to have the one more extcon type. So, I use the
'if' instead of 'switch'.

	if (EXTCON_TYPE_USB & type)
		memset(cable->usb_propval, sizeof(cable->usb_propval), 0);
	if (EXTCON_TYPE_CHG & type)
		memset(cable->chg_propval, sizeof(cable->chg_propval), 0);
	if (EXTCON_TYPE_JACK & type)
		memset(cable->jack_propval, sizeof(cable->jack_propval), 0);
	if (EXTCON_TYPE_DISP & type)
		memset(cable->disp_propval, sizeof(cable->disp_propval), 0);

> 
>> +}
>> +
>>  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);

You're right. I'll fix it as following.

	spin_lock_irqsave(&edev->lock, flags);
	if (!is_extcon_attached(edev, id, index)) {
		spin_unlock_irqrestore(&edev->lock, flags);
		return 0;
	}

	cable = &edev->cables[index];

>> +
>> +       /* 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);

[snip]

Regards,
Chanwoo Choi