2019-06-13 15:04:29

by Jeffrey Hugo

Subject: [PATCH v3 2/7] drivers: regulator: qcom_spmi: Refactor get_mode/set_mode

spmi_regulator_common_get_mode and spmi_regulator_common_set_mode use
multi-level ifs which mirror a switch statement. Refactor to use a switch
statement to make the code flow more clear.

Signed-off-by: Jeffrey Hugo <[email protected]>
---
drivers/regulator/qcom_spmi-regulator.c | 28 ++++++++++++++++---------
1 file changed, 18 insertions(+), 10 deletions(-)

diff --git a/drivers/regulator/qcom_spmi-regulator.c b/drivers/regulator/qcom_spmi-regulator.c
index fd55438c25d6..1c18fe5969b5 100644
--- a/drivers/regulator/qcom_spmi-regulator.c
+++ b/drivers/regulator/qcom_spmi-regulator.c
@@ -911,13 +911,14 @@ static unsigned int spmi_regulator_common_get_mode(struct regulator_dev *rdev)

spmi_vreg_read(vreg, SPMI_COMMON_REG_MODE, &reg, 1);

- if (reg & SPMI_COMMON_MODE_HPM_MASK)
+ switch (reg) {
+ case SPMI_COMMON_MODE_HPM_MASK:
return REGULATOR_MODE_NORMAL;
-
- if (reg & SPMI_COMMON_MODE_AUTO_MASK)
+ case SPMI_COMMON_MODE_AUTO_MASK:
return REGULATOR_MODE_FAST;
-
- return REGULATOR_MODE_IDLE;
+ default:
+ return REGULATOR_MODE_IDLE;
+ }
}

static int
@@ -925,12 +926,19 @@ spmi_regulator_common_set_mode(struct regulator_dev *rdev, unsigned int mode)
{
struct spmi_regulator *vreg = rdev_get_drvdata(rdev);
u8 mask = SPMI_COMMON_MODE_HPM_MASK | SPMI_COMMON_MODE_AUTO_MASK;
- u8 val = 0;
+ u8 val;

- if (mode == REGULATOR_MODE_NORMAL)
+ switch (mode) {
+ case REGULATOR_MODE_NORMAL:
val = SPMI_COMMON_MODE_HPM_MASK;
- else if (mode == REGULATOR_MODE_FAST)
+ break;
+ case REGULATOR_MODE_FAST:
val = SPMI_COMMON_MODE_AUTO_MASK;
+ break;
+ default:
+ val = 0;
+ break;
+ }

return spmi_vreg_update_bits(vreg, SPMI_COMMON_REG_MODE, val, mask);
}
@@ -1834,9 +1842,9 @@ static int qcom_spmi_regulator_probe(struct platform_device *pdev)
}
}

- if (vreg->logical_type == SPMI_REGULATOR_LOGICAL_TYPE_HFS430) {
+ if (vreg->set_points->count == 1) {
/* since there is only one range */
- range = spmi_regulator_find_range(vreg);
+ range = vreg->set_points->range;
vreg->desc.uV_step = range->step_uV;
}

--
2.17.1

2019-06-13 15:14:17

by Bjorn Andersson

Subject: Re: [PATCH v3 2/7] drivers: regulator: qcom_spmi: Refactor get_mode/set_mode

On Thu 13 Jun 07:22 PDT 2019, Jeffrey Hugo wrote:

> spmi_regulator_common_get_mode and spmi_regulator_common_set_mode use
> multi-level ifs which mirror a switch statement. Refactor to use a switch
> statement to make the code flow more clear.
>
> Signed-off-by: Jeffrey Hugo <[email protected]>
> ---
> drivers/regulator/qcom_spmi-regulator.c | 28 ++++++++++++++++---------
> 1 file changed, 18 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/regulator/qcom_spmi-regulator.c b/drivers/regulator/qcom_spmi-regulator.c
> index fd55438c25d6..1c18fe5969b5 100644
> --- a/drivers/regulator/qcom_spmi-regulator.c
> +++ b/drivers/regulator/qcom_spmi-regulator.c
> @@ -911,13 +911,14 @@ static unsigned int spmi_regulator_common_get_mode(struct regulator_dev *rdev)
>
> spmi_vreg_read(vreg, SPMI_COMMON_REG_MODE, &reg, 1);
>
> - if (reg & SPMI_COMMON_MODE_HPM_MASK)
> + switch (reg) {
> + case SPMI_COMMON_MODE_HPM_MASK:
> return REGULATOR_MODE_NORMAL;
> -
> - if (reg & SPMI_COMMON_MODE_AUTO_MASK)
> + case SPMI_COMMON_MODE_AUTO_MASK:
> return REGULATOR_MODE_FAST;
> -
> - return REGULATOR_MODE_IDLE;
> + default:
> + return REGULATOR_MODE_IDLE;
> + }
> }
>
> static int
> @@ -925,12 +926,19 @@ spmi_regulator_common_set_mode(struct regulator_dev *rdev, unsigned int mode)
> {
> struct spmi_regulator *vreg = rdev_get_drvdata(rdev);
> u8 mask = SPMI_COMMON_MODE_HPM_MASK | SPMI_COMMON_MODE_AUTO_MASK;
> - u8 val = 0;
> + u8 val;
>
> - if (mode == REGULATOR_MODE_NORMAL)
> + switch (mode) {
> + case REGULATOR_MODE_NORMAL:
> val = SPMI_COMMON_MODE_HPM_MASK;
> - else if (mode == REGULATOR_MODE_FAST)
> + break;
> + case REGULATOR_MODE_FAST:
> val = SPMI_COMMON_MODE_AUTO_MASK;
> + break;
> + default:
> + val = 0;
> + break;
> + }

For this part:
Reviewed-by: Bjorn Andersson <[email protected]>
>
> return spmi_vreg_update_bits(vreg, SPMI_COMMON_REG_MODE, val, mask);
> }
> @@ -1834,9 +1842,9 @@ static int qcom_spmi_regulator_probe(struct platform_device *pdev)
> }
> }
>
> - if (vreg->logical_type == SPMI_REGULATOR_LOGICAL_TYPE_HFS430) {

Squash this into patch 1.

Regards,
Bjorn

> + if (vreg->set_points->count == 1) {
> /* since there is only one range */
> - range = spmi_regulator_find_range(vreg);
> + range = vreg->set_points->range;
> vreg->desc.uV_step = range->step_uV;
> }
>
> --
> 2.17.1
>

2019-06-13 15:25:50

by Bjorn Andersson

Subject: Re: [PATCH v3 2/7] drivers: regulator: qcom_spmi: Refactor get_mode/set_mode

On Thu 13 Jun 08:12 PDT 2019, Bjorn Andersson wrote:

> On Thu 13 Jun 07:22 PDT 2019, Jeffrey Hugo wrote:
>
> > spmi_regulator_common_get_mode and spmi_regulator_common_set_mode use
> > multi-level ifs which mirror a switch statement. Refactor to use a switch
> > statement to make the code flow more clear.
> >
> > Signed-off-by: Jeffrey Hugo <[email protected]>
> > ---
> > drivers/regulator/qcom_spmi-regulator.c | 28 ++++++++++++++++---------
> > 1 file changed, 18 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/regulator/qcom_spmi-regulator.c b/drivers/regulator/qcom_spmi-regulator.c
> > index fd55438c25d6..1c18fe5969b5 100644
> > --- a/drivers/regulator/qcom_spmi-regulator.c
> > +++ b/drivers/regulator/qcom_spmi-regulator.c
> > @@ -911,13 +911,14 @@ static unsigned int spmi_regulator_common_get_mode(struct regulator_dev *rdev)
> >
> > spmi_vreg_read(vreg, SPMI_COMMON_REG_MODE, &reg, 1);
> >
> > - if (reg & SPMI_COMMON_MODE_HPM_MASK)

Sorry, didn't see the & here. Don't you need to mask out the mode bits
before turning this into a switch?

> > + switch (reg) {
> > + case SPMI_COMMON_MODE_HPM_MASK:
> > return REGULATOR_MODE_NORMAL;
> > -
> > - if (reg & SPMI_COMMON_MODE_AUTO_MASK)
> > + case SPMI_COMMON_MODE_AUTO_MASK:
> > return REGULATOR_MODE_FAST;
> > -
> > - return REGULATOR_MODE_IDLE;
> > + default:
> > + return REGULATOR_MODE_IDLE;
> > + }
> > }
> >
> > static int
> > @@ -925,12 +926,19 @@ spmi_regulator_common_set_mode(struct regulator_dev *rdev, unsigned int mode)
> > {
> > struct spmi_regulator *vreg = rdev_get_drvdata(rdev);
> > u8 mask = SPMI_COMMON_MODE_HPM_MASK | SPMI_COMMON_MODE_AUTO_MASK;
> > - u8 val = 0;
> > + u8 val;
> >
> > - if (mode == REGULATOR_MODE_NORMAL)
> > + switch (mode) {
> > + case REGULATOR_MODE_NORMAL:
> > val = SPMI_COMMON_MODE_HPM_MASK;
> > - else if (mode == REGULATOR_MODE_FAST)
> > + break;
> > + case REGULATOR_MODE_FAST:
> > val = SPMI_COMMON_MODE_AUTO_MASK;
> > + break;
> > + default:
> > + val = 0;
> > + break;
> > + }
>
> For this part:
> Reviewed-by: Bjorn Andersson <[email protected]>
> >
> > return spmi_vreg_update_bits(vreg, SPMI_COMMON_REG_MODE, val, mask);
> > }
> > @@ -1834,9 +1842,9 @@ static int qcom_spmi_regulator_probe(struct platform_device *pdev)
> > }
> > }
> >
> > - if (vreg->logical_type == SPMI_REGULATOR_LOGICAL_TYPE_HFS430) {
>
> Squash this into patch 1.
>
> Regards,
> Bjorn
>
> > + if (vreg->set_points->count == 1) {
> > /* since there is only one range */
> > - range = spmi_regulator_find_range(vreg);
> > + range = vreg->set_points->range;
> > vreg->desc.uV_step = range->step_uV;
> > }
> >
> > --
> > 2.17.1
> >

2019-06-13 15:40:10

by Jeffrey Hugo

Subject: Re: [PATCH v3 2/7] drivers: regulator: qcom_spmi: Refactor get_mode/set_mode

On Thu, Jun 13, 2019 at 9:24 AM Bjorn Andersson
<[email protected]> wrote:
>
> On Thu 13 Jun 08:12 PDT 2019, Bjorn Andersson wrote:
>
> > On Thu 13 Jun 07:22 PDT 2019, Jeffrey Hugo wrote:
> >
> > > spmi_regulator_common_get_mode and spmi_regulator_common_set_mode use
> > > multi-level ifs which mirror a switch statement. Refactor to use a switch
> > > statement to make the code flow more clear.
> > >
> > > Signed-off-by: Jeffrey Hugo <[email protected]>
> > > ---
> > > drivers/regulator/qcom_spmi-regulator.c | 28 ++++++++++++++++---------
> > > 1 file changed, 18 insertions(+), 10 deletions(-)
> > >
> > > diff --git a/drivers/regulator/qcom_spmi-regulator.c b/drivers/regulator/qcom_spmi-regulator.c
> > > index fd55438c25d6..1c18fe5969b5 100644
> > > --- a/drivers/regulator/qcom_spmi-regulator.c
> > > +++ b/drivers/regulator/qcom_spmi-regulator.c
> > > @@ -911,13 +911,14 @@ static unsigned int spmi_regulator_common_get_mode(struct regulator_dev *rdev)
> > >
> > > spmi_vreg_read(vreg, SPMI_COMMON_REG_MODE, &reg, 1);
> > >
> > > - if (reg & SPMI_COMMON_MODE_HPM_MASK)
>
> Sorry, didn't see the & here. Don't you need to mask out the mode bits
> before turning this into a switch?

Ah. Yes. I read the documentation wrong when doing this. Will fix.

>
> > > + switch (reg) {
> > > + case SPMI_COMMON_MODE_HPM_MASK:
> > > return REGULATOR_MODE_NORMAL;
> > > -
> > > - if (reg & SPMI_COMMON_MODE_AUTO_MASK)
> > > + case SPMI_COMMON_MODE_AUTO_MASK:
> > > return REGULATOR_MODE_FAST;
> > > -
> > > - return REGULATOR_MODE_IDLE;
> > > + default:
> > > + return REGULATOR_MODE_IDLE;
> > > + }
> > > }
> > >
> > > static int
> > > @@ -925,12 +926,19 @@ spmi_regulator_common_set_mode(struct regulator_dev *rdev, unsigned int mode)
> > > {
> > > struct spmi_regulator *vreg = rdev_get_drvdata(rdev);
> > > u8 mask = SPMI_COMMON_MODE_HPM_MASK | SPMI_COMMON_MODE_AUTO_MASK;
> > > - u8 val = 0;
> > > + u8 val;
> > >
> > > - if (mode == REGULATOR_MODE_NORMAL)
> > > + switch (mode) {
> > > + case REGULATOR_MODE_NORMAL:
> > > val = SPMI_COMMON_MODE_HPM_MASK;
> > > - else if (mode == REGULATOR_MODE_FAST)
> > > + break;
> > > + case REGULATOR_MODE_FAST:
> > > val = SPMI_COMMON_MODE_AUTO_MASK;
> > > + break;
> > > + default:
> > > + val = 0;
> > > + break;
> > > + }
> >
> > For this part:
> > Reviewed-by: Bjorn Andersson <[email protected]>
> > >
> > > return spmi_vreg_update_bits(vreg, SPMI_COMMON_REG_MODE, val, mask);
> > > }
> > > @@ -1834,9 +1842,9 @@ static int qcom_spmi_regulator_probe(struct platform_device *pdev)
> > > }
> > > }
> > >
> > > - if (vreg->logical_type == SPMI_REGULATOR_LOGICAL_TYPE_HFS430) {
> >
> > Squash this into patch 1.
> >
> > Regards,
> > Bjorn
> >
> > > + if (vreg->set_points->count == 1) {
> > > /* since there is only one range */
> > > - range = spmi_regulator_find_range(vreg);
> > > + range = vreg->set_points->range;
> > > vreg->desc.uV_step = range->step_uV;
> > > }
> > >
> > > --
> > > 2.17.1
> > >