Hi Charles,

On 11/15/2013 01:18 AM, Charles Keepax wrote:
> This patch factors out each headphone detection IP in a seperate
> function this makes the code a little more readable and prevents us
> getting to excessive levels of indentation.
> 
> Signed-off-by: Charles Keepax <ckee...@opensource.wolfsonmicro.com>
> ---
>  drivers/extcon/extcon-arizona.c |  215 +++++++++++++++++++++++---------------
>  1 files changed, 130 insertions(+), 85 deletions(-)
> 
> diff --git a/drivers/extcon/extcon-arizona.c b/drivers/extcon/extcon-arizona.c
> index f4fe2d1..d71a738 100644
> --- a/drivers/extcon/extcon-arizona.c
> +++ b/drivers/extcon/extcon-arizona.c
> @@ -354,7 +354,31 @@ static struct {
>       { 1000, 10000 },
>  };
>  
> -static int arizona_hpdet_read(struct arizona_extcon_info *info)
> +static int arizona_hpdet_read_ip0(struct arizona_extcon_info *info)
> +{
> +     struct arizona *arizona = info->arizona;
> +     unsigned int val;
> +     int ret;
> +
> +     ret = regmap_read(arizona->regmap, ARIZONA_HEADPHONE_DETECT_2, &val);
> +     if (ret != 0) {

I prefer following checking statement
        if (ret < 0) instead of if (ret != 0) 

> +             dev_err(arizona->dev, "Failed to read HPDET status: %d\n",
> +                     ret);
> +             return ret;
> +     }
> +
> +
> +     if (!(val & ARIZONA_HP_DONE)) {
> +             dev_err(arizona->dev, "HPDET did not complete: %x\n", val);
> +             return -EAGAIN;
> +     }
> +
> +     val &= ARIZONA_HP_LVL_MASK;
> +
> +     return val;
> +}
> +
> +static int arizona_hpdet_read_ip1(struct arizona_extcon_info *info)
>  {
>       struct arizona *arizona = info->arizona;
>       unsigned int val, range;
> @@ -367,106 +391,127 @@ static int arizona_hpdet_read(struct 
> arizona_extcon_info *info)
>               return ret;
>       }
>  
> -     switch (info->hpdet_ip) {
> -     case 0:
> -             if (!(val & ARIZONA_HP_DONE)) {
> -                     dev_err(arizona->dev, "HPDET did not complete: %x\n",
> -                             val);
> -                     return -EAGAIN;
> -             }
> +     if (!(val & ARIZONA_HP_DONE_B)) {
> +             dev_err(arizona->dev, "HPDET did not complete: %x\n", val);
> +             return -EAGAIN;
> +     }
>  
> -             val &= ARIZONA_HP_LVL_MASK;
> -             break;
> +     ret = regmap_read(arizona->regmap, ARIZONA_HP_DACVAL, &val);
> +     if (ret != 0) {

ditto.

> +             dev_err(arizona->dev, "Failed to read HP value: %d\n", ret);
> +             return -EAGAIN;
> +     }
>  
> -     case 1:
> -             if (!(val & ARIZONA_HP_DONE_B)) {
> -                     dev_err(arizona->dev, "HPDET did not complete: %x\n",
> -                             val);
> -                     return -EAGAIN;
> -             }
> +     regmap_read(arizona->regmap, ARIZONA_HEADPHONE_DETECT_1, &range);
> +     range = (range & ARIZONA_HP_IMPEDANCE_RANGE_MASK)
> +                >> ARIZONA_HP_IMPEDANCE_RANGE_SHIFT;
>  
> -             ret = regmap_read(arizona->regmap, ARIZONA_HP_DACVAL, &val);
> -             if (ret != 0) {
> -                     dev_err(arizona->dev, "Failed to read HP value: %d\n",
> -                             ret);
> -                     return -EAGAIN;
> -             }
> +     if (range < ARRAY_SIZE(arizona_hpdet_b_ranges) - 1 &&
> +         (val < 100 || val >= 0x3fb)) {

If you possible, I'd like you to define '100' and '0x3fb' as defined constant 
with 'define' keyword
to improve readability.

> +             range++;
> +             dev_dbg(arizona->dev, "Moving to HPDET range %d\n", range);
> +             regmap_update_bits(arizona->regmap,
> +                                ARIZONA_HEADPHONE_DETECT_1,
> +                                ARIZONA_HP_IMPEDANCE_RANGE_MASK,
> +                                range <<
> +                                ARIZONA_HP_IMPEDANCE_RANGE_SHIFT);
> +             return -EAGAIN;
> +     }
>  
> -             regmap_read(arizona->regmap, ARIZONA_HEADPHONE_DETECT_1,
> -                         &range);
> -             range = (range & ARIZONA_HP_IMPEDANCE_RANGE_MASK)
> -                        >> ARIZONA_HP_IMPEDANCE_RANGE_SHIFT;
> +     /* If we go out of range report top of range */
> +     if (val < 100 || val >= 0x3fb) {

ditto.

> +             dev_dbg(arizona->dev, "Measurement out of range\n");
> +             return ARIZONA_HPDET_MAX;
> +     }
>  
> -             if (range < ARRAY_SIZE(arizona_hpdet_b_ranges) - 1 &&
> -                 (val < 100 || val >= 0x3fb)) {
> -                     range++;
> -                     dev_dbg(arizona->dev, "Moving to HPDET range %d\n",
> -                             range);
> -                     regmap_update_bits(arizona->regmap,
> -                                        ARIZONA_HEADPHONE_DETECT_1,
> -                                        ARIZONA_HP_IMPEDANCE_RANGE_MASK,
> -                                        range <<
> -                                        ARIZONA_HP_IMPEDANCE_RANGE_SHIFT);
> -                     return -EAGAIN;
> -             }
> +     dev_dbg(arizona->dev, "HPDET read %d in range %d\n", val, range);
>  
> -             /* If we go out of range report top of range */
> -             if (val < 100 || val >= 0x3fb) {
> -                     dev_dbg(arizona->dev, "Measurement out of range\n");
> -                     return ARIZONA_HPDET_MAX;
> -             }
> +     val = arizona_hpdet_b_ranges[range].factor_b
> +             / ((val * 100) - arizona_hpdet_b_ranges[range].factor_a);

ditto.

What is meaning of 100? I prefer to define constant variable.

> +
> +     return val;
> +}
> +
> +static int arizona_hpdet_read_ip2(struct arizona_extcon_info *info)
> +{
> +     struct arizona *arizona = info->arizona;
> +     unsigned int val, range;
> +     int ret;
> +
> +     ret = regmap_read(arizona->regmap, ARIZONA_HEADPHONE_DETECT_2, &val);
> +     if (ret != 0) {

ditto.

> +             dev_err(arizona->dev, "Failed to read HPDET status: %d\n",
> +                     ret);
> +             return ret;
> +     }
> +
> +     if (!(val & ARIZONA_HP_DONE_B)) {
> +             dev_err(arizona->dev, "HPDET did not complete: %x\n", val);
> +             return -EAGAIN;
> +     }
> +
> +     val &= ARIZONA_HP_LVL_B_MASK;
> +     /* Convert to ohms, the value is in 0.5 ohm increments */
> +     val /= 2;
> +
> +     regmap_read(arizona->regmap, ARIZONA_HEADPHONE_DETECT_1, &range);
> +     range = (range & ARIZONA_HP_IMPEDANCE_RANGE_MASK)
> +                >> ARIZONA_HP_IMPEDANCE_RANGE_SHIFT;
> +
> +     /* Skip up a range, or report? */
> +     if (range < ARRAY_SIZE(arizona_hpdet_c_ranges) - 1 &&
> +         (val >= arizona_hpdet_c_ranges[range].max)) {
> +             range++;
> +             dev_dbg(arizona->dev, "Moving to HPDET range %d-%d\n",
> +                     arizona_hpdet_c_ranges[range].min,
> +                     arizona_hpdet_c_ranges[range].max);
> +             regmap_update_bits(arizona->regmap,
> +                                ARIZONA_HEADPHONE_DETECT_1,
> +                                ARIZONA_HP_IMPEDANCE_RANGE_MASK,
> +                                range <<
> +                                ARIZONA_HP_IMPEDANCE_RANGE_SHIFT);
> +             return -EAGAIN;
> +     }
> +
> +     if (range && (val < arizona_hpdet_c_ranges[range].min)) {
> +             dev_dbg(arizona->dev, "Reporting range boundary %d\n",
> +                     arizona_hpdet_c_ranges[range].min);
> +             val = arizona_hpdet_c_ranges[range].min;
> +     }
> +
> +     return val;
> +}
>  
> -             dev_dbg(arizona->dev, "HPDET read %d in range %d\n",
> -                     val, range);
> +static int arizona_hpdet_read(struct arizona_extcon_info *info)
> +{
> +     struct arizona *arizona = info->arizona;
> +     int ret = 0;
>  
> -             val = arizona_hpdet_b_ranges[range].factor_b
> -                     / ((val * 100) -
> -                        arizona_hpdet_b_ranges[range].factor_a);
> +     switch (info->hpdet_ip) {
> +     case 0:
> +             ret = arizona_hpdet_read_ip0(info);
> +             if (ret < 0)
> +                     return ret;
> +             break;
> +
> +     case 1:
> +             ret = arizona_hpdet_read_ip1(info);
> +             if (ret < 0)
> +                     return ret;
>               break;
>  
>       default:
>               dev_warn(arizona->dev, "Unknown HPDET IP revision %d\n",
>                        info->hpdet_ip);
>       case 2:
> -             if (!(val & ARIZONA_HP_DONE_B)) {
> -                     dev_err(arizona->dev, "HPDET did not complete: %x\n",
> -                             val);
> -                     return -EAGAIN;
> -             }
> -
> -             val &= ARIZONA_HP_LVL_B_MASK;
> -             /* Convert to ohms, the value is in 0.5 ohm increments */
> -             val /= 2;
> -
> -             regmap_read(arizona->regmap, ARIZONA_HEADPHONE_DETECT_1,
> -                         &range);
> -             range = (range & ARIZONA_HP_IMPEDANCE_RANGE_MASK)
> -                        >> ARIZONA_HP_IMPEDANCE_RANGE_SHIFT;
> -
> -             /* Skip up a range, or report? */
> -             if (range < ARRAY_SIZE(arizona_hpdet_c_ranges) - 1 &&
> -                 (val >= arizona_hpdet_c_ranges[range].max)) {
> -                     range++;
> -                     dev_dbg(arizona->dev, "Moving to HPDET range %d-%d\n",
> -                             arizona_hpdet_c_ranges[range].min,
> -                             arizona_hpdet_c_ranges[range].max);
> -                     regmap_update_bits(arizona->regmap,
> -                                        ARIZONA_HEADPHONE_DETECT_1,
> -                                        ARIZONA_HP_IMPEDANCE_RANGE_MASK,
> -                                        range <<
> -                                        ARIZONA_HP_IMPEDANCE_RANGE_SHIFT);
> -                     return -EAGAIN;
> -             }
> -
> -             if (range && (val < arizona_hpdet_c_ranges[range].min)) {
> -                     dev_dbg(arizona->dev, "Reporting range boundary %d\n",
> -                             arizona_hpdet_c_ranges[range].min);
> -                     val = arizona_hpdet_c_ranges[range].min;
> -             }
> +             ret = arizona_hpdet_read_ip2(info);
> +             if (ret < 0)
> +                     return ret;
> +             break;
>       }
>  
> -     dev_dbg(arizona->dev, "HP impedance %d ohms\n", val);
> -     return val;
> +     dev_dbg(arizona->dev, "HP impedance %d ohms\n", ret);

Need blank line.

> +     return ret;
>  }
>  
>  static int arizona_hpdet_do_id(struct arizona_extcon_info *info, int 
> *reading,
> 

Thanks,
Chanwoo Choi
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Reply via email to