From: Chanwoo Choi <cw00.choi@samsung.com>
To: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
Cc: myungjoo.ham@samsung.com, sameo@linux.intel.com,
lee.jones@linaro.org, patches@opensource.wolfsonmicro.com,
linux-kernel@vger.kernel.org
Subject: Re: [PATCH v3 6/7] extcon: arizona: Factor out different headphone detection IPs
Date: Fri, 15 Nov 2013 09:58:55 +0900 [thread overview]
Message-ID: <528571CF.6080405@samsung.com> (raw)
In-Reply-To: <1384445907-12859-6-git-send-email-ckeepax@opensource.wolfsonmicro.com>
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 <ckeepax@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
next prev parent reply other threads:[~2013-11-15 0:59 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-11-14 16:18 [PATCH v3 1/7] extcon: arizona: Add defines for microphone detection levels Charles Keepax
2013-11-14 16:18 ` [PATCH v3 2/7] extcon: arizona: Fix reset of HPDET after race with removal Charles Keepax
2013-11-14 16:18 ` [PATCH v3 3/7] extcon: arizona: Fix race with microphone detection and removal Charles Keepax
2013-11-14 16:18 ` [PATCH v3 4/7] extcon: arizona: No need to switch back down HPDET ranges Charles Keepax
2013-11-14 16:18 ` [PATCH v3 5/7] extcon: arizona: Add support for headphone detection on wm5110 rev D Charles Keepax
2013-11-14 16:18 ` [PATCH v3 6/7] extcon: arizona: Factor out different headphone detection IPs Charles Keepax
2013-11-15 0:58 ` Chanwoo Choi [this message]
2013-11-14 16:18 ` [PATCH v3 7/7] extcon: arizona: Improve headphone detection error handling Charles Keepax
2013-11-15 0:54 ` Chanwoo Choi
2013-11-15 1:03 ` [PATCH v3 1/7] extcon: arizona: Add defines for microphone detection levels Chanwoo Choi
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=528571CF.6080405@samsung.com \
--to=cw00.choi@samsung.com \
--cc=ckeepax@opensource.wolfsonmicro.com \
--cc=lee.jones@linaro.org \
--cc=linux-kernel@vger.kernel.org \
--cc=myungjoo.ham@samsung.com \
--cc=patches@opensource.wolfsonmicro.com \
--cc=sameo@linux.intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.