| Message ID | 20210825152518.379386-21-miquel.raynal@bootlin.com |
|---|---|
| State | New |
| Headers | show |
| Series | TI AM437X ADC1 | expand |
On Wed, 25 Aug 2021 17:24:58 +0200 Miquel Raynal <miquel.raynal@bootlin.com> wrote: > Instead of deriving in the probe and in the resume path the value of the > ctrl register, let's do it only once in the probe, save the value of > this register in the driver's structure and use it from the resume > callback. > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> A few minor things inline. J > --- > drivers/mfd/ti_am335x_tscadc.c | 31 ++++++++-------------------- > include/linux/mfd/ti_am335x_tscadc.h | 2 +- > 2 files changed, 10 insertions(+), 23 deletions(-) > > diff --git a/drivers/mfd/ti_am335x_tscadc.c b/drivers/mfd/ti_am335x_tscadc.c > index 7071344ad18e..d661e8ae66c9 100644 > --- a/drivers/mfd/ti_am335x_tscadc.c > +++ b/drivers/mfd/ti_am335x_tscadc.c > @@ -122,7 +122,7 @@ static int ti_tscadc_probe(struct platform_device *pdev) > struct clk *clk; > u32 val; > int tsc_wires = 0, adc_channels = 0, readouts = 0, cell_idx = 0; > - int total_channels, ctrl, err; > + int total_channels, err; > > /* Allocate memory for device */ > tscadc = devm_kzalloc(&pdev->dev, sizeof(*tscadc), GFP_KERNEL); > @@ -215,22 +215,21 @@ static int ti_tscadc_probe(struct platform_device *pdev) > regmap_write(tscadc->regmap, REG_CLKDIV, tscadc->clk_div); > > /* Set the control register bits */ > - ctrl = CNTRLREG_STEPCONFIGWRT | CNTRLREG_STEPID; > - regmap_write(tscadc->regmap, REG_CTRL, ctrl); > + tscadc->ctrl = CNTRLREG_STEPCONFIGWRT | CNTRLREG_STEPID; > + regmap_write(tscadc->regmap, REG_CTRL, tscadc->ctrl); > > if (tsc_wires > 0) { > - tscadc->tsc_wires = tsc_wires; > + tscadc->ctrl |= CNTRLREG_TSCENB; > if (tsc_wires == 5) > - ctrl |= CNTRLREG_5WIRE | CNTRLREG_TSCENB; > + tscadc->ctrl |= CNTRLREG_5WIRE; > else > - ctrl |= CNTRLREG_4WIRE | CNTRLREG_TSCENB; > + tscadc->ctrl |= CNTRLREG_4WIRE; > } > > tscadc_idle_config(tscadc); > > /* Enable the TSC module enable bit */ > - ctrl |= CNTRLREG_TSCSSENB; > - regmap_write(tscadc->regmap, REG_CTRL, ctrl); > + regmap_write(tscadc->regmap, REG_CTRL, tscadc->ctrl | CNTRLREG_TSCSSENB); > > /* TSC Cell */ > if (tsc_wires > 0) { > @@ -305,25 +304,13 @@ static int __maybe_unused tscadc_suspend(struct device *dev) > static int __maybe_unused tscadc_resume(struct device *dev) > { > struct ti_tscadc_dev *tscadc = dev_get_drvdata(dev); > - u32 ctrl; > > pm_runtime_get_sync(dev); > > - /* context restore */ > - ctrl = CNTRLREG_STEPCONFIGWRT | CNTRLREG_STEPID; > - regmap_write(tscadc->regmap, REG_CTRL, ctrl); > - > - if (tscadc->tsc_wires > 0) { > - if (tscadc->tsc_wires == 5) > - ctrl |= CNTRLREG_5WIRE | CNTRLREG_TSCENB; > - else > - ctrl |= CNTRLREG_4WIRE | CNTRLREG_TSCENB; > - } > - ctrl |= CNTRLREG_TSCSSENB; > - regmap_write(tscadc->regmap, REG_CTRL, ctrl); > - > regmap_write(tscadc->regmap, REG_CLKDIV, tscadc->clk_div); > + regmap_write(tscadc->regmap, REG_CTRL, tscadc->ctrl); Patch description should mention why this ordering change is here. > tscadc_idle_config(tscadc); > + regmap_write(tscadc->regmap, REG_CTRL, tscadc->ctrl | CNTRLREG_TSCSSENB); As the value of tscadc->ctrl is not the same as REG_CTRL this is a bit non obvious. You might be better off keeping them in sync, but masking that bit out and then resetting it as appropriate. > > return 0; > } > diff --git a/include/linux/mfd/ti_am335x_tscadc.h b/include/linux/mfd/ti_am335x_tscadc.h > index e734fb97dff8..02963b6ebbac 100644 > --- a/include/linux/mfd/ti_am335x_tscadc.h > +++ b/include/linux/mfd/ti_am335x_tscadc.h > @@ -177,8 +177,8 @@ struct ti_tscadc_dev { > phys_addr_t tscadc_phys_base; > const struct ti_tscadc_data *data; > int irq; > - int tsc_wires; > struct mfd_cell cells[TSCADC_CELLS]; > + u32 ctrl; > u32 reg_se_cache; > bool adc_waiting; > bool adc_in_use;
Hi Jonathan, Jonathan Cameron <jic23@kernel.org> wrote on Mon, 30 Aug 2021 14:56:08 +0100: > On Wed, 25 Aug 2021 17:24:58 +0200 > Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > > Instead of deriving in the probe and in the resume path the value of the > > ctrl register, let's do it only once in the probe, save the value of > > this register in the driver's structure and use it from the resume > > callback. > > > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > A few minor things inline. > > J > > > --- > > drivers/mfd/ti_am335x_tscadc.c | 31 ++++++++-------------------- > > include/linux/mfd/ti_am335x_tscadc.h | 2 +- > > 2 files changed, 10 insertions(+), 23 deletions(-) > > > > diff --git a/drivers/mfd/ti_am335x_tscadc.c b/drivers/mfd/ti_am335x_tscadc.c > > index 7071344ad18e..d661e8ae66c9 100644 > > --- a/drivers/mfd/ti_am335x_tscadc.c > > +++ b/drivers/mfd/ti_am335x_tscadc.c > > @@ -122,7 +122,7 @@ static int ti_tscadc_probe(struct platform_device *pdev) > > struct clk *clk; > > u32 val; > > int tsc_wires = 0, adc_channels = 0, readouts = 0, cell_idx = 0; > > - int total_channels, ctrl, err; > > + int total_channels, err; > > > > /* Allocate memory for device */ > > tscadc = devm_kzalloc(&pdev->dev, sizeof(*tscadc), GFP_KERNEL); > > @@ -215,22 +215,21 @@ static int ti_tscadc_probe(struct platform_device *pdev) > > regmap_write(tscadc->regmap, REG_CLKDIV, tscadc->clk_div); > > > > /* Set the control register bits */ > > - ctrl = CNTRLREG_STEPCONFIGWRT | CNTRLREG_STEPID; > > - regmap_write(tscadc->regmap, REG_CTRL, ctrl); > > + tscadc->ctrl = CNTRLREG_STEPCONFIGWRT | CNTRLREG_STEPID; > > + regmap_write(tscadc->regmap, REG_CTRL, tscadc->ctrl); > > > > if (tsc_wires > 0) { > > - tscadc->tsc_wires = tsc_wires; > > + tscadc->ctrl |= CNTRLREG_TSCENB; > > if (tsc_wires == 5) > > - ctrl |= CNTRLREG_5WIRE | CNTRLREG_TSCENB; > > + tscadc->ctrl |= CNTRLREG_5WIRE; > > else > > - ctrl |= CNTRLREG_4WIRE | CNTRLREG_TSCENB; > > + tscadc->ctrl |= CNTRLREG_4WIRE; > > } > > > > tscadc_idle_config(tscadc); > > > > /* Enable the TSC module enable bit */ > > - ctrl |= CNTRLREG_TSCSSENB; > > - regmap_write(tscadc->regmap, REG_CTRL, ctrl); > > + regmap_write(tscadc->regmap, REG_CTRL, tscadc->ctrl | CNTRLREG_TSCSSENB); > > > > /* TSC Cell */ > > if (tsc_wires > 0) { > > @@ -305,25 +304,13 @@ static int __maybe_unused tscadc_suspend(struct device *dev) > > static int __maybe_unused tscadc_resume(struct device *dev) > > { > > struct ti_tscadc_dev *tscadc = dev_get_drvdata(dev); > > - u32 ctrl; > > > > pm_runtime_get_sync(dev); > > > > - /* context restore */ > > - ctrl = CNTRLREG_STEPCONFIGWRT | CNTRLREG_STEPID; > > - regmap_write(tscadc->regmap, REG_CTRL, ctrl); > > - > > - if (tscadc->tsc_wires > 0) { > > - if (tscadc->tsc_wires == 5) > > - ctrl |= CNTRLREG_5WIRE | CNTRLREG_TSCENB; > > - else > > - ctrl |= CNTRLREG_4WIRE | CNTRLREG_TSCENB; > > - } > > - ctrl |= CNTRLREG_TSCSSENB; > > - regmap_write(tscadc->regmap, REG_CTRL, ctrl); > > - > > regmap_write(tscadc->regmap, REG_CLKDIV, tscadc->clk_div); > > + regmap_write(tscadc->regmap, REG_CTRL, tscadc->ctrl); > > Patch description should mention why this ordering change is here. I actually moved the patch that reorders things earlier so that the reviewer is not bothered by the order changes later on. > > > tscadc_idle_config(tscadc); > > + regmap_write(tscadc->regmap, REG_CTRL, tscadc->ctrl | CNTRLREG_TSCSSENB); > > As the value of tscadc->ctrl is not the same as REG_CTRL this is a bit non obvious. > > You might be better off keeping them in sync, but masking that bit out and then resetting > it as appropriate. I honestly find more readable doing: ctrl = flags; writel(ctrl); writel(ctrl | en_bit); than ctrl = flags; writel(ctrl & ~en_bit); writel(ctrl); because the second version emphasis the fact that we reset the en_bit (which is wrong, the point of this first write is to actually write all the configuration but not the en_bit yet) while the first version clearly shows that the second write includes an additional "enable bit". Thanks, Miquèl
On Thu, 2 Sep 2021 21:42:47 +0200 Miquel Raynal <miquel.raynal@bootlin.com> wrote: > Hi Jonathan, > > Jonathan Cameron <jic23@kernel.org> wrote on Mon, 30 Aug 2021 14:56:08 > +0100: > > > On Wed, 25 Aug 2021 17:24:58 +0200 > > Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > > > > Instead of deriving in the probe and in the resume path the value of the > > > ctrl register, let's do it only once in the probe, save the value of > > > this register in the driver's structure and use it from the resume > > > callback. > > > > > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > > A few minor things inline. > > > > J > > > > > --- > > > drivers/mfd/ti_am335x_tscadc.c | 31 ++++++++-------------------- > > > include/linux/mfd/ti_am335x_tscadc.h | 2 +- > > > 2 files changed, 10 insertions(+), 23 deletions(-) > > > > > > diff --git a/drivers/mfd/ti_am335x_tscadc.c b/drivers/mfd/ti_am335x_tscadc.c > > > index 7071344ad18e..d661e8ae66c9 100644 > > > --- a/drivers/mfd/ti_am335x_tscadc.c > > > +++ b/drivers/mfd/ti_am335x_tscadc.c > > > @@ -122,7 +122,7 @@ static int ti_tscadc_probe(struct platform_device *pdev) > > > struct clk *clk; > > > u32 val; > > > int tsc_wires = 0, adc_channels = 0, readouts = 0, cell_idx = 0; > > > - int total_channels, ctrl, err; > > > + int total_channels, err; > > > > > > /* Allocate memory for device */ > > > tscadc = devm_kzalloc(&pdev->dev, sizeof(*tscadc), GFP_KERNEL); > > > @@ -215,22 +215,21 @@ static int ti_tscadc_probe(struct platform_device *pdev) > > > regmap_write(tscadc->regmap, REG_CLKDIV, tscadc->clk_div); > > > > > > /* Set the control register bits */ > > > - ctrl = CNTRLREG_STEPCONFIGWRT | CNTRLREG_STEPID; > > > - regmap_write(tscadc->regmap, REG_CTRL, ctrl); > > > + tscadc->ctrl = CNTRLREG_STEPCONFIGWRT | CNTRLREG_STEPID; > > > + regmap_write(tscadc->regmap, REG_CTRL, tscadc->ctrl); > > > > > > if (tsc_wires > 0) { > > > - tscadc->tsc_wires = tsc_wires; > > > + tscadc->ctrl |= CNTRLREG_TSCENB; > > > if (tsc_wires == 5) > > > - ctrl |= CNTRLREG_5WIRE | CNTRLREG_TSCENB; > > > + tscadc->ctrl |= CNTRLREG_5WIRE; > > > else > > > - ctrl |= CNTRLREG_4WIRE | CNTRLREG_TSCENB; > > > + tscadc->ctrl |= CNTRLREG_4WIRE; > > > } > > > > > > tscadc_idle_config(tscadc); > > > > > > /* Enable the TSC module enable bit */ > > > - ctrl |= CNTRLREG_TSCSSENB; > > > - regmap_write(tscadc->regmap, REG_CTRL, ctrl); > > > + regmap_write(tscadc->regmap, REG_CTRL, tscadc->ctrl | CNTRLREG_TSCSSENB); > > > > > > /* TSC Cell */ > > > if (tsc_wires > 0) { > > > @@ -305,25 +304,13 @@ static int __maybe_unused tscadc_suspend(struct device *dev) > > > static int __maybe_unused tscadc_resume(struct device *dev) > > > { > > > struct ti_tscadc_dev *tscadc = dev_get_drvdata(dev); > > > - u32 ctrl; > > > > > > pm_runtime_get_sync(dev); > > > > > > - /* context restore */ > > > - ctrl = CNTRLREG_STEPCONFIGWRT | CNTRLREG_STEPID; > > > - regmap_write(tscadc->regmap, REG_CTRL, ctrl); > > > - > > > - if (tscadc->tsc_wires > 0) { > > > - if (tscadc->tsc_wires == 5) > > > - ctrl |= CNTRLREG_5WIRE | CNTRLREG_TSCENB; > > > - else > > > - ctrl |= CNTRLREG_4WIRE | CNTRLREG_TSCENB; > > > - } > > > - ctrl |= CNTRLREG_TSCSSENB; > > > - regmap_write(tscadc->regmap, REG_CTRL, ctrl); > > > - > > > regmap_write(tscadc->regmap, REG_CLKDIV, tscadc->clk_div); > > > + regmap_write(tscadc->regmap, REG_CTRL, tscadc->ctrl); > > > > Patch description should mention why this ordering change is here. > > I actually moved the patch that reorders things earlier so that the > reviewer is not bothered by the order changes later on. > > > > > > tscadc_idle_config(tscadc); > > > + regmap_write(tscadc->regmap, REG_CTRL, tscadc->ctrl | CNTRLREG_TSCSSENB); > > > > As the value of tscadc->ctrl is not the same as REG_CTRL this is a bit non obvious. > > > > You might be better off keeping them in sync, but masking that bit out and then resetting > > it as appropriate. > > I honestly find more readable doing: > > ctrl = flags; > writel(ctrl); > writel(ctrl | en_bit); > > than > > ctrl = flags; > writel(ctrl & ~en_bit); > writel(ctrl); > > because the second version emphasis the fact that we reset the en_bit > (which is wrong, the point of this first write is to actually write all > the configuration but not the en_bit yet) while the first version > clearly shows that the second write includes an additional "enable bit". Fair enough. Perhaps it's worth throwing in a comment though if you happen to be respining to say tcsadc->ctrl isn't actually the contents of the register, but rather of 'most' of it. Jonathan > > Thanks, > Miquèl
diff --git a/drivers/mfd/ti_am335x_tscadc.c b/drivers/mfd/ti_am335x_tscadc.c index 7071344ad18e..d661e8ae66c9 100644 --- a/drivers/mfd/ti_am335x_tscadc.c +++ b/drivers/mfd/ti_am335x_tscadc.c @@ -122,7 +122,7 @@ static int ti_tscadc_probe(struct platform_device *pdev) struct clk *clk; u32 val; int tsc_wires = 0, adc_channels = 0, readouts = 0, cell_idx = 0; - int total_channels, ctrl, err; + int total_channels, err; /* Allocate memory for device */ tscadc = devm_kzalloc(&pdev->dev, sizeof(*tscadc), GFP_KERNEL); @@ -215,22 +215,21 @@ static int ti_tscadc_probe(struct platform_device *pdev) regmap_write(tscadc->regmap, REG_CLKDIV, tscadc->clk_div); /* Set the control register bits */ - ctrl = CNTRLREG_STEPCONFIGWRT | CNTRLREG_STEPID; - regmap_write(tscadc->regmap, REG_CTRL, ctrl); + tscadc->ctrl = CNTRLREG_STEPCONFIGWRT | CNTRLREG_STEPID; + regmap_write(tscadc->regmap, REG_CTRL, tscadc->ctrl); if (tsc_wires > 0) { - tscadc->tsc_wires = tsc_wires; + tscadc->ctrl |= CNTRLREG_TSCENB; if (tsc_wires == 5) - ctrl |= CNTRLREG_5WIRE | CNTRLREG_TSCENB; + tscadc->ctrl |= CNTRLREG_5WIRE; else - ctrl |= CNTRLREG_4WIRE | CNTRLREG_TSCENB; + tscadc->ctrl |= CNTRLREG_4WIRE; } tscadc_idle_config(tscadc); /* Enable the TSC module enable bit */ - ctrl |= CNTRLREG_TSCSSENB; - regmap_write(tscadc->regmap, REG_CTRL, ctrl); + regmap_write(tscadc->regmap, REG_CTRL, tscadc->ctrl | CNTRLREG_TSCSSENB); /* TSC Cell */ if (tsc_wires > 0) { @@ -305,25 +304,13 @@ static int __maybe_unused tscadc_suspend(struct device *dev) static int __maybe_unused tscadc_resume(struct device *dev) { struct ti_tscadc_dev *tscadc = dev_get_drvdata(dev); - u32 ctrl; pm_runtime_get_sync(dev); - /* context restore */ - ctrl = CNTRLREG_STEPCONFIGWRT | CNTRLREG_STEPID; - regmap_write(tscadc->regmap, REG_CTRL, ctrl); - - if (tscadc->tsc_wires > 0) { - if (tscadc->tsc_wires == 5) - ctrl |= CNTRLREG_5WIRE | CNTRLREG_TSCENB; - else - ctrl |= CNTRLREG_4WIRE | CNTRLREG_TSCENB; - } - ctrl |= CNTRLREG_TSCSSENB; - regmap_write(tscadc->regmap, REG_CTRL, ctrl); - regmap_write(tscadc->regmap, REG_CLKDIV, tscadc->clk_div); + regmap_write(tscadc->regmap, REG_CTRL, tscadc->ctrl); tscadc_idle_config(tscadc); + regmap_write(tscadc->regmap, REG_CTRL, tscadc->ctrl | CNTRLREG_TSCSSENB); return 0; } diff --git a/include/linux/mfd/ti_am335x_tscadc.h b/include/linux/mfd/ti_am335x_tscadc.h index e734fb97dff8..02963b6ebbac 100644 --- a/include/linux/mfd/ti_am335x_tscadc.h +++ b/include/linux/mfd/ti_am335x_tscadc.h @@ -177,8 +177,8 @@ struct ti_tscadc_dev { phys_addr_t tscadc_phys_base; const struct ti_tscadc_data *data; int irq; - int tsc_wires; struct mfd_cell cells[TSCADC_CELLS]; + u32 ctrl; u32 reg_se_cache; bool adc_waiting; bool adc_in_use;
Instead of deriving in the probe and in the resume path the value of the ctrl register, let's do it only once in the probe, save the value of this register in the driver's structure and use it from the resume callback. Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> --- drivers/mfd/ti_am335x_tscadc.c | 31 ++++++++-------------------- include/linux/mfd/ti_am335x_tscadc.h | 2 +- 2 files changed, 10 insertions(+), 23 deletions(-)