| Message ID | 2329d20396db00b60f4e7d2e783ea46b48eea9c1.1459918214.git.amsfield22@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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 >
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
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
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");
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(-)