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

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

Hi Guenter,

On 2016년 08월 02일 04:48, Guenter Roeck wrote:
> On Sun, Jul 31, 2016 at 10:50 PM, 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>
>> Tested-by: Chris Zhong <zyw@rock-chips.com>
> 
> Reviewed-by: Guenter Roeck <groeck@chromium.org>
> 
> [ couple of nitpicks below ]
> 
>> ---
>>  drivers/extcon/extcon.c | 201 +++++++++++++++++++++++++++++++++++++++++++++++-
>>  include/linux/extcon.h  |  86 +++++++++++++++++++++
>>  2 files changed, 286 insertions(+), 1 deletion(-)
>>

[snip]

>>  static ssize_t state_show(struct device *dev, struct device_attribute *attr,
>>                           char *buf)
>>  {
>> @@ -421,7 +475,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, index));
> 
> Conversion from bool to int (0, 1) is automatic, so
> 
>         return is_extcon_attached(edev, index);
> 
> would be identical.

OK. I'll fix it.

[snip]

>> diff --git a/include/linux/extcon.h b/include/linux/extcon.h
>> index 46d802892c82..5a97e52abefb 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 + 1)
>> +
>> +/* 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 + 1)
>> +
>> +/* 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)
>> +
>> +/* 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)
>> +
>> +/*
>> + * 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) */
> 
> integer

OK. I'll fix it.

Thanks for review.

Regards,
Chanwoo Choi