| Message ID | 1429619636-25478-7-git-send-email-rf@opensource.wolfsonmicro.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Richard, On 04/21/2015 09:33 PM, Richard Fitzgerald wrote: > Signed-off-by: Richard Fitzgerald <rf@opensource.wolfsonmicro.com> > --- > drivers/extcon/extcon-arizona.c | 33 ++++++++++++++++++++++----------- > 1 files changed, 22 insertions(+), 11 deletions(-) > > diff --git a/drivers/extcon/extcon-arizona.c b/drivers/extcon/extcon-arizona.c > index a0ed35b..0e60787 100644 > --- a/drivers/extcon/extcon-arizona.c > +++ b/drivers/extcon/extcon-arizona.c > @@ -1,7 +1,7 @@ > /* > * extcon-arizona.c - Extcon driver Wolfson Arizona devices > * > - * Copyright (C) 2012 Wolfson Microelectronics plc > + * Copyright (C) 2012-2014 Wolfson Microelectronics plc > * > * This program is free software; you can redistribute it and/or modify > * it under the terms of the GNU General Public License as published by > @@ -140,10 +140,14 @@ static void arizona_extcon_hp_clamp(struct arizona_extcon_info *info, > bool clamp) > { > struct arizona *arizona = info->arizona; > - unsigned int mask = 0, val = 0; > + unsigned int mask, val = 0; > int ret; > > switch (arizona->type) { > + case WM8998: > + case WM1814: > + mask = 0; > + break; > case WM5110: > mask = ARIZONA_HP1L_SHRTO | ARIZONA_HP1L_FLWR | > ARIZONA_HP1L_SHRTI; > @@ -175,17 +179,19 @@ static void arizona_extcon_hp_clamp(struct arizona_extcon_info *info, > ret); > } > > - ret = regmap_update_bits(arizona->regmap, ARIZONA_HP_CTRL_1L, > - mask, val); > - if (ret != 0) > - dev_warn(arizona->dev, "Failed to do clamp: %d\n", > + if (mask) { > + ret = regmap_update_bits(arizona->regmap, ARIZONA_HP_CTRL_1L, > + mask, val); > + if (ret != 0) > + dev_warn(arizona->dev, "Failed to do clamp: %d\n", > ret); > > - ret = regmap_update_bits(arizona->regmap, ARIZONA_HP_CTRL_1R, > - mask, val); > - if (ret != 0) > - dev_warn(arizona->dev, "Failed to do clamp: %d\n", > - ret); > + ret = regmap_update_bits(arizona->regmap, ARIZONA_HP_CTRL_1R, > + mask, val); > + if (ret != 0) > + dev_warn(arizona->dev, "Failed to do clamp: %d\n", > + ret); > + } > > /* Restore the desired state while not doing the clamp */ > if (!clamp) { > @@ -1176,6 +1182,11 @@ static int arizona_extcon_probe(struct platform_device *pdev) > break; > } > break; > + case WM8998: > + case WM1814: > + info->micd_clamp = true; > + info->hpdet_ip = 2; What is meaning of '2'? I prefer to use the definition for '2'. Except for upper one comment, looks good to me. Thanks, Chanwoo Choi
On Wed, Apr 22, 2015 at 02:53:42PM +0900, Chanwoo Choi wrote: > Hi Richard, > > > @@ -1176,6 +1182,11 @@ static int arizona_extcon_probe(struct platform_device *pdev) > > break; > > } > > break; > > + case WM8998: > > + case WM1814: > > + info->micd_clamp = true; > > + info->hpdet_ip = 2; > > What is meaning of '2'? I prefer to use the definition for '2'. > '2' is the version number of the hpdet ip block in silicon. We're already using it as a raw number '0', '1' or '2' all over extcon-arizona.c so changing it here would mean making other patches to the file that aren't really part of adding WM8998 support, so I'd prefer not to change that as a side-effect of adding WM8998. > Except for upper one comment, looks good to me. > > Thanks, > Chanwoo Choi
On 04/22/2015 06:19 PM, Richard Fitzgerald wrote: > On Wed, Apr 22, 2015 at 02:53:42PM +0900, Chanwoo Choi wrote: >> Hi Richard, >> >>> @@ -1176,6 +1182,11 @@ static int arizona_extcon_probe(struct platform_device *pdev) >>> break; >>> } >>> break; >>> + case WM8998: >>> + case WM1814: >>> + info->micd_clamp = true; >>> + info->hpdet_ip = 2; >> >> What is meaning of '2'? I prefer to use the definition for '2'. >> > > '2' is the version number of the hpdet ip block in silicon. We're already using > it as a raw number '0', '1' or '2' all over extcon-arizona.c so changing it here > would mean making other patches to the file that aren't really part of adding > WM8998 support, so I'd prefer not to change that as a side-effect of adding WM8998. I think that just you can define following definitions and use HPDET_IP_VER_V2 instead of '2'. #define HPDET_IP_VER_V0 0 #define HPDET_IP_VER_V1 1 #define HPDET_IP_VER_V2 2
On Wed, Apr 22, 2015 at 07:20:09PM +0900, Chanwoo Choi wrote: > On 04/22/2015 06:19 PM, Richard Fitzgerald wrote: > > On Wed, Apr 22, 2015 at 02:53:42PM +0900, Chanwoo Choi wrote: > >> Hi Richard, > >> > >>> @@ -1176,6 +1182,11 @@ static int arizona_extcon_probe(struct platform_device *pdev) > >>> break; > >>> } > >>> break; > >>> + case WM8998: > >>> + case WM1814: > >>> + info->micd_clamp = true; > >>> + info->hpdet_ip = 2; > >> > >> What is meaning of '2'? I prefer to use the definition for '2'. > >> > > > > '2' is the version number of the hpdet ip block in silicon. We're already using > > it as a raw number '0', '1' or '2' all over extcon-arizona.c so changing it here > > would mean making other patches to the file that aren't really part of adding > > WM8998 support, so I'd prefer not to change that as a side-effect of adding WM8998. > > I think that just you can define following definitions and use HPDET_IP_VER_V2 instead of '2'. > > #define HPDET_IP_VER_V0 0 > #define HPDET_IP_VER_V1 1 > #define HPDET_IP_VER_V2 2 > Can we deal with that as a separate patch from this series? Like I said, the code already uses '0' '1' and '2' for the existing codecs so making a change to use #define means patching the code for the other codecs. That is not part of adding WM8998 support and I don't like patches that make unexpected extra side-effect changes that are not relevant to the actual functionality being added by the patch. It's specially annoying when cherry-picking or reverting those patches if they included some extra code change. If we can get this series submitted I can look at making a later patch to improve readbility, but since this really is just a version number I think it would be enough to rename the variable to hpdet_ip_version rather than effectively doing #define TWO 2 > _______________________________________________ > Alsa-devel mailing list > Alsa-devel@alsa-project.org > http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
On 04/23/2015 11:15 PM, Richard Fitzgerald wrote: > On Wed, Apr 22, 2015 at 07:20:09PM +0900, Chanwoo Choi wrote: >> On 04/22/2015 06:19 PM, Richard Fitzgerald wrote: >>> On Wed, Apr 22, 2015 at 02:53:42PM +0900, Chanwoo Choi wrote: >>>> Hi Richard, >>>> >>>>> @@ -1176,6 +1182,11 @@ static int arizona_extcon_probe(struct platform_device *pdev) >>>>> break; >>>>> } >>>>> break; >>>>> + case WM8998: >>>>> + case WM1814: >>>>> + info->micd_clamp = true; >>>>> + info->hpdet_ip = 2; >>>> >>>> What is meaning of '2'? I prefer to use the definition for '2'. >>>> >>> >>> '2' is the version number of the hpdet ip block in silicon. We're already using >>> it as a raw number '0', '1' or '2' all over extcon-arizona.c so changing it here >>> would mean making other patches to the file that aren't really part of adding >>> WM8998 support, so I'd prefer not to change that as a side-effect of adding WM8998. >> >> I think that just you can define following definitions and use HPDET_IP_VER_V2 instead of '2'. >> >> #define HPDET_IP_VER_V0 0 >> #define HPDET_IP_VER_V1 1 >> #define HPDET_IP_VER_V2 2 >> > > Can we deal with that as a separate patch from this series? Like I said, > the code already uses '0' '1' and '2' for the existing codecs so making a > change to use #define means patching the code for the other codecs. That > is not part of adding WM8998 support and I don't like patches that make > unexpected extra side-effect changes that are not relevant to the actual > functionality being added by the patch. It's specially annoying when > cherry-picking or reverting those patches if they included some extra > code change. > > If we can get this series submitted I can look at making a later patch > to improve readbility, but since this really is just a version number I > think it would be enough to rename the variable to hpdet_ip_version rather > than effectively doing #define TWO 2 OK. I agree that rename the variable name (hpdet_ip_version) or add the definitions for version on later patch. Acked-by: Chanwoo Choi <cw00.choi@samsung.com> Thanks, Chanwoo Choi
diff --git a/drivers/extcon/extcon-arizona.c b/drivers/extcon/extcon-arizona.c index a0ed35b..0e60787 100644 --- a/drivers/extcon/extcon-arizona.c +++ b/drivers/extcon/extcon-arizona.c @@ -1,7 +1,7 @@ /* * extcon-arizona.c - Extcon driver Wolfson Arizona devices * - * Copyright (C) 2012 Wolfson Microelectronics plc + * Copyright (C) 2012-2014 Wolfson Microelectronics plc * * This program is free software; you can redistribute it and/or modify * it under the terms of the GNU General Public License as published by @@ -140,10 +140,14 @@ static void arizona_extcon_hp_clamp(struct arizona_extcon_info *info, bool clamp) { struct arizona *arizona = info->arizona; - unsigned int mask = 0, val = 0; + unsigned int mask, val = 0; int ret; switch (arizona->type) { + case WM8998: + case WM1814: + mask = 0; + break; case WM5110: mask = ARIZONA_HP1L_SHRTO | ARIZONA_HP1L_FLWR | ARIZONA_HP1L_SHRTI; @@ -175,17 +179,19 @@ static void arizona_extcon_hp_clamp(struct arizona_extcon_info *info, ret); } - ret = regmap_update_bits(arizona->regmap, ARIZONA_HP_CTRL_1L, - mask, val); - if (ret != 0) - dev_warn(arizona->dev, "Failed to do clamp: %d\n", + if (mask) { + ret = regmap_update_bits(arizona->regmap, ARIZONA_HP_CTRL_1L, + mask, val); + if (ret != 0) + dev_warn(arizona->dev, "Failed to do clamp: %d\n", ret); - ret = regmap_update_bits(arizona->regmap, ARIZONA_HP_CTRL_1R, - mask, val); - if (ret != 0) - dev_warn(arizona->dev, "Failed to do clamp: %d\n", - ret); + ret = regmap_update_bits(arizona->regmap, ARIZONA_HP_CTRL_1R, + mask, val); + if (ret != 0) + dev_warn(arizona->dev, "Failed to do clamp: %d\n", + ret); + } /* Restore the desired state while not doing the clamp */ if (!clamp) { @@ -1176,6 +1182,11 @@ static int arizona_extcon_probe(struct platform_device *pdev) break; } break; + case WM8998: + case WM1814: + info->micd_clamp = true; + info->hpdet_ip = 2; + break; default: break; }
Signed-off-by: Richard Fitzgerald <rf@opensource.wolfsonmicro.com> --- drivers/extcon/extcon-arizona.c | 33 ++++++++++++++++++++++----------- 1 files changed, 22 insertions(+), 11 deletions(-)