Re: [PATCH 1/9] iio: adc: exynos_adc: use regmap to retrieve struct device

[Alison Schofield]

On Wed, Apr 06, 2016 at 09:03:00AM +0200, Marek Szyprowski wrote:
> Hello,
> 
> On 2016-04-06 07:15, Alison Schofield wrote:
> >Driver includes struct regmap and struct device in its global data.
> >Remove the struct device and use regmap API to retrieve device info.
> >
> >Patch created using Coccinelle plus manual edits.
> >
> >Signed-off-by: Alison Schofield <amsfield22@xxxxxxxxx>
> 
> This patch changes the struct device which is used by the driver to report
> errors. The driver used correctly the struct device associated with its
> device tree node, while after the patch it will use device which is
> associated with PMU regmap, which is a different device. PMU regmap is there
> only to enable/disable the ADC block and it is not the regmap used to access
> registers of the ADC device.
> 
> I would prefer to drop this patch.
> 

Thanks Marek!  Please check my understanding.  Driver is not carrying
a duplicate struct device. The regmap in exynos_adc is *not* this
devices regmap. It belongs to the PMU, (power mgmt unit?) 

It seemed excessive to carry around a struct device just for the
dev_err messages, but, we need that struct to extract the correct
iio_dev struct. Without a regmap belonging to this actual device,
no efficiencies can be gained in exynos, and the patch will be
dropped from set v2.

Now I need to be able to recognize such cases elsewhere. I'm going
back though other patches in this set looking for that, but I'm not
so sure I would recognize it.

Jonathan & all, Any hints on the rule of regmap?

Thanks,
alisons



> >---
> >  drivers/iio/adc/exynos_adc.c | 18 +++++++++---------
> >  1 file changed, 9 insertions(+), 9 deletions(-)
> >
> >diff --git a/drivers/iio/adc/exynos_adc.c b/drivers/iio/adc/exynos_adc.c
> >index c15756d..0313e0f 100644
> >--- a/drivers/iio/adc/exynos_adc.c
> >+++ b/drivers/iio/adc/exynos_adc.c
> >@@ -130,7 +130,6 @@
> >  struct exynos_adc {
> >  	struct exynos_adc_data	*data;
> >-	struct device		*dev;
> >  	struct input_dev	*input;
> >  	void __iomem		*regs;
> >  	struct regmap		*pmu_map;
> >@@ -173,11 +172,12 @@ static void exynos_adc_unprepare_clk(struct exynos_adc *info)
> >  static int exynos_adc_prepare_clk(struct exynos_adc *info)
> >  {
> >+	struct device *dev = regmap_get_device(info->pmu_map);
> >  	int ret;
> >  	ret = clk_prepare(info->clk);
> >  	if (ret) {
> >-		dev_err(info->dev, "failed preparing adc clock: %d\n", ret);
> >+		dev_err(dev, "failed preparing adc clock: %d\n", ret);
> >  		return ret;
> >  	}
> >@@ -185,7 +185,7 @@ static int exynos_adc_prepare_clk(struct exynos_adc *info)
> >  		ret = clk_prepare(info->sclk);
> >  		if (ret) {
> >  			clk_unprepare(info->clk);
> >-			dev_err(info->dev,
> >+			dev_err(dev,
> >  				"failed preparing sclk_adc clock: %d\n", ret);
> >  			return ret;
> >  		}
> >@@ -203,11 +203,12 @@ static void exynos_adc_disable_clk(struct exynos_adc *info)
> >  static int exynos_adc_enable_clk(struct exynos_adc *info)
> >  {
> >+	struct device *dev = regmap_get_device(info->pmu_map);
> >  	int ret;
> >  	ret = clk_enable(info->clk);
> >  	if (ret) {
> >-		dev_err(info->dev, "failed enabling adc clock: %d\n", ret);
> >+		dev_err(dev, "failed enabling adc clock: %d\n", ret);
> >  		return ret;
> >  	}
> >@@ -215,7 +216,7 @@ static int exynos_adc_enable_clk(struct exynos_adc *info)
> >  		ret = clk_enable(info->sclk);
> >  		if (ret) {
> >  			clk_disable(info->clk);
> >-			dev_err(info->dev,
> >+			dev_err(dev,
> >  				"failed enabling sclk_adc clock: %d\n", ret);
> >  			return ret;
> >  		}
> >@@ -610,13 +611,14 @@ static irqreturn_t exynos_adc_isr(int irq, void *dev_id)
> >  static irqreturn_t exynos_ts_isr(int irq, void *dev_id)
> >  {
> >  	struct exynos_adc *info = dev_id;
> >-	struct iio_dev *dev = dev_get_drvdata(info->dev);
> >+	struct device *dev = regmap_get_device(info->pmu_map);
> >+	struct iio_dev *indio_dev = dev_get_drvdata(dev);
> >  	u32 x, y;
> >  	bool pressed;
> >  	int ret;
> >  	while (info->input->users) {
> >-		ret = exynos_read_s3c64xx_ts(dev, &x, &y);
> >+		ret = exynos_read_s3c64xx_ts(indio_dev, &x, &y);
> >  		if (ret == -ETIMEDOUT)
> >  			break;
> >@@ -800,8 +802,6 @@ static int exynos_adc_probe(struct platform_device *pdev)
> >  	info->tsirq = irq;
> >-	info->dev = &pdev->dev;
> >-
> >  	init_completion(&info->completion);
> >  	info->clk = devm_clk_get(&pdev->dev, "adc");
> 
> Best regards
> -- 
> Marek Szyprowski, PhD
> Samsung R&D Institute Poland
>