linux-kernel - Re: [PATCH 2/2] extcon: max77843: support USB accessories as external USB hosts

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAJKOXPduOUF9rLXhQmKPG0uyDwuMex5FXVtS82XBE-CoHkZ28A@mail.gmail.com>
Date:   Wed, 29 Mar 2017 17:27:10 +0300
From:   Krzysztof Kozlowski <krzk@...nel.org>
To:     Andi Shyti <andi.shyti@...sung.com>
Cc:     MyungJoo Ham <myungjoo.ham@...sung.com>,
        Chanwoo Choi <cw00.choi@...sung.com>,
        Jaewon Kim <jaewon02.kim@...sung.com>,
        Seung-Woo Kim <sw0312.kim@...sung.com>,
        linux-kernel@...r.kernel.org, Andi Shyti <andi@...zian.org>
Subject: Re: [PATCH 2/2] extcon: max77843: support USB accessories as external
 USB hosts

On Wed, Mar 29, 2017 at 2:54 PM, Andi Shyti <andi.shyti@...sung.com> wrote:
> The ADC state defines the resistance that a USB device has in
> order to distinguish between devices.
>
> The external accessories (like the Gear VR) are defined as:
>
> MAX77843_MUIC_ADC_RESERVED_ACC_1
> MAX77843_MUIC_ADC_RESERVED_ACC_2
> MAX77843_MUIC_ADC_RESERVED_ACC_3
> MAX77843_MUIC_ADC_RESERVED_ACC_4
> MAX77843_MUIC_ADC_RESERVED_ACC_5
>
> and they should be set as such in the extcon driver.
>
> Do not ignore interrupts generated by external accessories by
> handling cables of the type from any of the above.
>
> Do not handle any charging related action. This can be
> misleading as also external devices might have some power and in
> such cases the MUIC might consider them as power providers. In
> this case check the value of the adc and if it corresponds to
> one or the MAX77843_MUIC_ADC_RESERVED_ACC_*, return a no-action
> value (MAX77843_MUIC_CHG_NONE).
>
> Signed-off-by: Andi Shyti <andi.shyti@...sung.com>
> ---
>  drivers/extcon/extcon-max77843.c | 17 ++++++++++++-----
>  1 file changed, 12 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/extcon/extcon-max77843.c b/drivers/extcon/extcon-max77843.c
> index fcdabc4b4025..00795fe9ab60 100644
> --- a/drivers/extcon/extcon-max77843.c
> +++ b/drivers/extcon/extcon-max77843.c
> @@ -271,6 +271,13 @@ static int max77843_muic_get_cable_type(struct max77843_muic_info *info,
>                 } else {
>                         *attached = true;
>                         switch (adc) {
> +                       case MAX77843_MUIC_ADC_RESERVED_ACC_1:
> +                       case MAX77843_MUIC_ADC_RESERVED_ACC_2:
> +                       case MAX77843_MUIC_ADC_RESERVED_ACC_3:
> +                       case MAX77843_MUIC_ADC_RESERVED_ACC_4:
> +                       case MAX77843_MUIC_ADC_RESERVED_ACC_5:

In commit msg you mentioned it but in code not. It looks weird to
handle a vague "reserved" type in a specific way. How about adding
some meaningful names instead of "reserved"? Or at least documenting,
that they match Gear VR. Really, imagine later one would be looking at
the code and trying to figure out why "reserved" means something

Best regards,
Krzysztof

> +                               info->prev_chg_type = MAX77843_MUIC_CHG_NONE;
> +                               break;
>                         case MAX77843_MUIC_ADC_GROUND:
>                                 info->prev_chg_type = MAX77843_MUIC_CHG_GND;
>                                 break;
> @@ -403,6 +410,11 @@ static int max77843_muic_adc_handler(struct max77843_muic_info *info)
>
>         switch (cable_type) {
>         case MAX77843_MUIC_ADC_GROUND:
> +       case MAX77843_MUIC_ADC_RESERVED_ACC_1:
> +       case MAX77843_MUIC_ADC_RESERVED_ACC_2:
> +       case MAX77843_MUIC_ADC_RESERVED_ACC_3:
> +       case MAX77843_MUIC_ADC_RESERVED_ACC_4:
> +       case MAX77843_MUIC_ADC_RESERVED_ACC_5:
>                 ret = max77843_muic_adc_gnd_handler(info);
>                 if (ret < 0)
>                         return ret;
> @@ -427,11 +439,6 @@ static int max77843_muic_adc_handler(struct max77843_muic_info *info)
>         case MAX77843_MUIC_ADC_REMOTE_S10_BUTTON:
>         case MAX77843_MUIC_ADC_REMOTE_S11_BUTTON:
>         case MAX77843_MUIC_ADC_REMOTE_S12_BUTTON:
> -       case MAX77843_MUIC_ADC_RESERVED_ACC_1:
> -       case MAX77843_MUIC_ADC_RESERVED_ACC_2:
> -       case MAX77843_MUIC_ADC_RESERVED_ACC_3:
> -       case MAX77843_MUIC_ADC_RESERVED_ACC_4:
> -       case MAX77843_MUIC_ADC_RESERVED_ACC_5:
>         case MAX77843_MUIC_ADC_AUDIO_DEVICE_TYPE2:
>         case MAX77843_MUIC_ADC_PHONE_POWERED_DEV:
>         case MAX77843_MUIC_ADC_TTY_CONVERTER:
> --
> 2.11.0
>