diff mbox

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

Message ID 2329d20396db00b60f4e7d2e783ea46b48eea9c1.1459918214.git.amsfield22@gmail.com (mailing list archive)
State Not Applicable, archived
Headers show

Commit Message

Alison Schofield April 6, 2016, 5:15 a.m. UTC
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@gmail.com>
---
 drivers/iio/adc/exynos_adc.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

Comments

Marek Szyprowski April 6, 2016, 7:03 a.m. UTC | #1
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@gmail.com>

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.

> ---
>   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
Alison Schofield April 6, 2016, 8:33 p.m. UTC | #2
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@gmail.com>
> 
> 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
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Marek Szyprowski April 7, 2016, 5:33 a.m. UTC | #3
Hello,

On 2016-04-06 22:33, Alison Schofield wrote:
> 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@gmail.com>
>> 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?)

Exactly.

> 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.

Thanks.

> 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?

You may check how the regmap is initialized. When it is retrieved by
phandle from device tree, then you might be almost sure that it refers
to the different device.

Best regards
Jonathan Cameron April 10, 2016, 1:45 p.m. UTC | #4
On 07/04/16 06:33, Marek Szyprowski wrote:
> Hello,
> 
> On 2016-04-06 22:33, Alison Schofield wrote:
>> 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@gmail.com>
>>> 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?)
> 
> Exactly.
> 
>> 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.
> 
> Thanks.
> 
>> 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?
> 
> You may check how the regmap is initialized. When it is retrieved by
> phandle from device tree, then you might be almost sure that it refers
> to the different device.
True enough.  Most simple devices that stand alone should be fine for
this sort of cleanup as the regmap is typically initiated directly for them.

Jonathan
> 
> Best regards

--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

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");