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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox