* [PATCH v2 0/4] iio: adc: xilinx: use even more devres
@ 2020-11-02 14:22 ` Bartosz Golaszewski
0 siblings, 0 replies; 26+ messages in thread
From: Bartosz Golaszewski @ 2020-11-02 14:22 UTC (permalink / raw)
To: Jonathan Cameron, Lars-Peter Clausen, Peter Meerwald-Stadler,
Michal Simek
Cc: linux-iio, linux-arm-kernel, linux-kernel, Bartosz Golaszewski
From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
This is a follow-up to commit 750628c79bb1 ("iio: adc: xilinx-xadc: use
devm_krealloc()"). I noticed we can use even more devres helpers and entirely
drop the remove() callback.
v1 -> v2:
- squash three patches adding more devres calls into one for easier review
- don't insist on the 80 characters limit
- add a new helper: devm_krealloc_array() and use it
Bartosz Golaszewski (4):
device: provide devm_krealloc_array()
iio: adc: xilinx: use helper variable for &pdev->dev
iio: adc: xilinx: use devm_krealloc_array() instead of kfree() +
kcalloc()
iio: adc: xilinx: use more devres helpers and remove remove()
drivers/iio/adc/xilinx-xadc-core.c | 151 +++++++++++++----------------
include/linux/device.h | 11 +++
2 files changed, 80 insertions(+), 82 deletions(-)
--
2.29.1
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v2 0/4] iio: adc: xilinx: use even more devres
@ 2020-11-02 14:22 ` Bartosz Golaszewski
0 siblings, 0 replies; 26+ messages in thread
From: Bartosz Golaszewski @ 2020-11-02 14:22 UTC (permalink / raw)
To: Jonathan Cameron, Lars-Peter Clausen, Peter Meerwald-Stadler,
Michal Simek
Cc: linux-iio, Bartosz Golaszewski, linux-kernel, linux-arm-kernel
From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
This is a follow-up to commit 750628c79bb1 ("iio: adc: xilinx-xadc: use
devm_krealloc()"). I noticed we can use even more devres helpers and entirely
drop the remove() callback.
v1 -> v2:
- squash three patches adding more devres calls into one for easier review
- don't insist on the 80 characters limit
- add a new helper: devm_krealloc_array() and use it
Bartosz Golaszewski (4):
device: provide devm_krealloc_array()
iio: adc: xilinx: use helper variable for &pdev->dev
iio: adc: xilinx: use devm_krealloc_array() instead of kfree() +
kcalloc()
iio: adc: xilinx: use more devres helpers and remove remove()
drivers/iio/adc/xilinx-xadc-core.c | 151 +++++++++++++----------------
include/linux/device.h | 11 +++
2 files changed, 80 insertions(+), 82 deletions(-)
--
2.29.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v2 1/4] device: provide devm_krealloc_array()
2020-11-02 14:22 ` Bartosz Golaszewski
@ 2020-11-02 14:22 ` Bartosz Golaszewski
-1 siblings, 0 replies; 26+ messages in thread
From: Bartosz Golaszewski @ 2020-11-02 14:22 UTC (permalink / raw)
To: Jonathan Cameron, Lars-Peter Clausen, Peter Meerwald-Stadler,
Michal Simek
Cc: linux-iio, linux-arm-kernel, linux-kernel, Bartosz Golaszewski
From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
When allocating an array of elements, users should check for
multiplication overflow or preferably use one of the provided helpers
like: devm_kmalloc_array().
This provides devm_krealloc_array() for users who want to reallocate
managed arrays.
Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
include/linux/device.h | 11 +++++++++++
1 file changed, 11 insertions(+)
diff --git a/include/linux/device.h b/include/linux/device.h
index 5ed101be7b2e..e77203faea55 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -226,6 +226,17 @@ static inline void *devm_kmalloc_array(struct device *dev,
return devm_kmalloc(dev, bytes, flags);
}
+static inline void *
+devm_krealloc_array(struct device *dev, void *ptr, size_t new_n,
+ size_t new_size, gfp_t gfp)
+{
+ size_t bytes;
+
+ if (unlikely(check_mul_overflow(new_n, new_size, &bytes)))
+ return NULL;
+
+ return devm_krealloc(dev, ptr, bytes, gfp);
+}
static inline void *devm_kcalloc(struct device *dev,
size_t n, size_t size, gfp_t flags)
{
--
2.29.1
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH v2 1/4] device: provide devm_krealloc_array()
@ 2020-11-02 14:22 ` Bartosz Golaszewski
0 siblings, 0 replies; 26+ messages in thread
From: Bartosz Golaszewski @ 2020-11-02 14:22 UTC (permalink / raw)
To: Jonathan Cameron, Lars-Peter Clausen, Peter Meerwald-Stadler,
Michal Simek
Cc: linux-iio, Bartosz Golaszewski, linux-kernel, linux-arm-kernel
From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
When allocating an array of elements, users should check for
multiplication overflow or preferably use one of the provided helpers
like: devm_kmalloc_array().
This provides devm_krealloc_array() for users who want to reallocate
managed arrays.
Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
include/linux/device.h | 11 +++++++++++
1 file changed, 11 insertions(+)
diff --git a/include/linux/device.h b/include/linux/device.h
index 5ed101be7b2e..e77203faea55 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -226,6 +226,17 @@ static inline void *devm_kmalloc_array(struct device *dev,
return devm_kmalloc(dev, bytes, flags);
}
+static inline void *
+devm_krealloc_array(struct device *dev, void *ptr, size_t new_n,
+ size_t new_size, gfp_t gfp)
+{
+ size_t bytes;
+
+ if (unlikely(check_mul_overflow(new_n, new_size, &bytes)))
+ return NULL;
+
+ return devm_krealloc(dev, ptr, bytes, gfp);
+}
static inline void *devm_kcalloc(struct device *dev,
size_t n, size_t size, gfp_t flags)
{
--
2.29.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH v2 2/4] iio: adc: xilinx: use helper variable for &pdev->dev
2020-11-02 14:22 ` Bartosz Golaszewski
@ 2020-11-02 14:22 ` Bartosz Golaszewski
-1 siblings, 0 replies; 26+ messages in thread
From: Bartosz Golaszewski @ 2020-11-02 14:22 UTC (permalink / raw)
To: Jonathan Cameron, Lars-Peter Clausen, Peter Meerwald-Stadler,
Michal Simek
Cc: linux-iio, linux-arm-kernel, linux-kernel, Bartosz Golaszewski
From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
It's more elegant to use a helper local variable to store the address
of the underlying struct device than to dereference pdev everywhere.
Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
drivers/iio/adc/xilinx-xadc-core.c | 13 +++++++------
1 file changed, 7 insertions(+), 6 deletions(-)
diff --git a/drivers/iio/adc/xilinx-xadc-core.c b/drivers/iio/adc/xilinx-xadc-core.c
index f93c34fe5873..8494eb424b33 100644
--- a/drivers/iio/adc/xilinx-xadc-core.c
+++ b/drivers/iio/adc/xilinx-xadc-core.c
@@ -1186,6 +1186,7 @@ static int xadc_parse_dt(struct iio_dev *indio_dev, struct device_node *np,
static int xadc_probe(struct platform_device *pdev)
{
+ struct device *dev = &pdev->dev;
const struct of_device_id *id;
struct iio_dev *indio_dev;
unsigned int bipolar_mask;
@@ -1195,10 +1196,10 @@ static int xadc_probe(struct platform_device *pdev)
int irq;
int i;
- if (!pdev->dev.of_node)
+ if (!dev->of_node)
return -ENODEV;
- id = of_match_node(xadc_of_match_table, pdev->dev.of_node);
+ id = of_match_node(xadc_of_match_table, dev->of_node);
if (!id)
return -EINVAL;
@@ -1206,7 +1207,7 @@ static int xadc_probe(struct platform_device *pdev)
if (irq <= 0)
return -ENXIO;
- indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*xadc));
+ indio_dev = devm_iio_device_alloc(dev, sizeof(*xadc));
if (!indio_dev)
return -ENOMEM;
@@ -1226,7 +1227,7 @@ static int xadc_probe(struct platform_device *pdev)
indio_dev->modes = INDIO_DIRECT_MODE;
indio_dev->info = &xadc_info;
- ret = xadc_parse_dt(indio_dev, pdev->dev.of_node, &conf0);
+ ret = xadc_parse_dt(indio_dev, dev->of_node, &conf0);
if (ret)
return ret;
@@ -1250,7 +1251,7 @@ static int xadc_probe(struct platform_device *pdev)
}
}
- xadc->clk = devm_clk_get(&pdev->dev, NULL);
+ xadc->clk = devm_clk_get(dev, NULL);
if (IS_ERR(xadc->clk)) {
ret = PTR_ERR(xadc->clk);
goto err_free_samplerate_trigger;
@@ -1276,7 +1277,7 @@ static int xadc_probe(struct platform_device *pdev)
}
ret = request_irq(xadc->irq, xadc->ops->interrupt_handler, 0,
- dev_name(&pdev->dev), indio_dev);
+ dev_name(dev), indio_dev);
if (ret)
goto err_clk_disable_unprepare;
--
2.29.1
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH v2 2/4] iio: adc: xilinx: use helper variable for &pdev->dev
@ 2020-11-02 14:22 ` Bartosz Golaszewski
0 siblings, 0 replies; 26+ messages in thread
From: Bartosz Golaszewski @ 2020-11-02 14:22 UTC (permalink / raw)
To: Jonathan Cameron, Lars-Peter Clausen, Peter Meerwald-Stadler,
Michal Simek
Cc: linux-iio, Bartosz Golaszewski, linux-kernel, linux-arm-kernel
From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
It's more elegant to use a helper local variable to store the address
of the underlying struct device than to dereference pdev everywhere.
Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
drivers/iio/adc/xilinx-xadc-core.c | 13 +++++++------
1 file changed, 7 insertions(+), 6 deletions(-)
diff --git a/drivers/iio/adc/xilinx-xadc-core.c b/drivers/iio/adc/xilinx-xadc-core.c
index f93c34fe5873..8494eb424b33 100644
--- a/drivers/iio/adc/xilinx-xadc-core.c
+++ b/drivers/iio/adc/xilinx-xadc-core.c
@@ -1186,6 +1186,7 @@ static int xadc_parse_dt(struct iio_dev *indio_dev, struct device_node *np,
static int xadc_probe(struct platform_device *pdev)
{
+ struct device *dev = &pdev->dev;
const struct of_device_id *id;
struct iio_dev *indio_dev;
unsigned int bipolar_mask;
@@ -1195,10 +1196,10 @@ static int xadc_probe(struct platform_device *pdev)
int irq;
int i;
- if (!pdev->dev.of_node)
+ if (!dev->of_node)
return -ENODEV;
- id = of_match_node(xadc_of_match_table, pdev->dev.of_node);
+ id = of_match_node(xadc_of_match_table, dev->of_node);
if (!id)
return -EINVAL;
@@ -1206,7 +1207,7 @@ static int xadc_probe(struct platform_device *pdev)
if (irq <= 0)
return -ENXIO;
- indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*xadc));
+ indio_dev = devm_iio_device_alloc(dev, sizeof(*xadc));
if (!indio_dev)
return -ENOMEM;
@@ -1226,7 +1227,7 @@ static int xadc_probe(struct platform_device *pdev)
indio_dev->modes = INDIO_DIRECT_MODE;
indio_dev->info = &xadc_info;
- ret = xadc_parse_dt(indio_dev, pdev->dev.of_node, &conf0);
+ ret = xadc_parse_dt(indio_dev, dev->of_node, &conf0);
if (ret)
return ret;
@@ -1250,7 +1251,7 @@ static int xadc_probe(struct platform_device *pdev)
}
}
- xadc->clk = devm_clk_get(&pdev->dev, NULL);
+ xadc->clk = devm_clk_get(dev, NULL);
if (IS_ERR(xadc->clk)) {
ret = PTR_ERR(xadc->clk);
goto err_free_samplerate_trigger;
@@ -1276,7 +1277,7 @@ static int xadc_probe(struct platform_device *pdev)
}
ret = request_irq(xadc->irq, xadc->ops->interrupt_handler, 0,
- dev_name(&pdev->dev), indio_dev);
+ dev_name(dev), indio_dev);
if (ret)
goto err_clk_disable_unprepare;
--
2.29.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH v2 3/4] iio: adc: xilinx: use devm_krealloc_array() instead of kfree() + kcalloc()
2020-11-02 14:22 ` Bartosz Golaszewski
@ 2020-11-02 14:22 ` Bartosz Golaszewski
-1 siblings, 0 replies; 26+ messages in thread
From: Bartosz Golaszewski @ 2020-11-02 14:22 UTC (permalink / raw)
To: Jonathan Cameron, Lars-Peter Clausen, Peter Meerwald-Stadler,
Michal Simek
Cc: linux-iio, linux-arm-kernel, linux-kernel, Bartosz Golaszewski
From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
We now have devm_krealloc_array() in the kernel Use it indstead of calling
kfree() and kcalloc() separately.
Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
drivers/iio/adc/xilinx-xadc-core.c | 11 +++++++----
1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/drivers/iio/adc/xilinx-xadc-core.c b/drivers/iio/adc/xilinx-xadc-core.c
index 8494eb424b33..7792aa4cf9cb 100644
--- a/drivers/iio/adc/xilinx-xadc-core.c
+++ b/drivers/iio/adc/xilinx-xadc-core.c
@@ -586,14 +586,18 @@ static int xadc_update_scan_mode(struct iio_dev *indio_dev,
{
struct xadc *xadc = iio_priv(indio_dev);
unsigned int n;
+ void *data;
n = bitmap_weight(mask, indio_dev->masklength);
- kfree(xadc->data);
- xadc->data = kcalloc(n, sizeof(*xadc->data), GFP_KERNEL);
- if (!xadc->data)
+ data = devm_krealloc_array(indio_dev->dev.parent, xadc->data, n,
+ sizeof(*xadc->data), GFP_KERNEL);
+ if (!data)
return -ENOMEM;
+ memset(data, 0, n * sizeof(*xadc->data));
+ xadc->data = data;
+
return 0;
}
@@ -1372,7 +1376,6 @@ static int xadc_remove(struct platform_device *pdev)
free_irq(xadc->irq, indio_dev);
cancel_delayed_work_sync(&xadc->zynq_unmask_work);
clk_disable_unprepare(xadc->clk);
- kfree(xadc->data);
return 0;
}
--
2.29.1
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH v2 3/4] iio: adc: xilinx: use devm_krealloc_array() instead of kfree() + kcalloc()
@ 2020-11-02 14:22 ` Bartosz Golaszewski
0 siblings, 0 replies; 26+ messages in thread
From: Bartosz Golaszewski @ 2020-11-02 14:22 UTC (permalink / raw)
To: Jonathan Cameron, Lars-Peter Clausen, Peter Meerwald-Stadler,
Michal Simek
Cc: linux-iio, Bartosz Golaszewski, linux-kernel, linux-arm-kernel
From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
We now have devm_krealloc_array() in the kernel Use it indstead of calling
kfree() and kcalloc() separately.
Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
drivers/iio/adc/xilinx-xadc-core.c | 11 +++++++----
1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/drivers/iio/adc/xilinx-xadc-core.c b/drivers/iio/adc/xilinx-xadc-core.c
index 8494eb424b33..7792aa4cf9cb 100644
--- a/drivers/iio/adc/xilinx-xadc-core.c
+++ b/drivers/iio/adc/xilinx-xadc-core.c
@@ -586,14 +586,18 @@ static int xadc_update_scan_mode(struct iio_dev *indio_dev,
{
struct xadc *xadc = iio_priv(indio_dev);
unsigned int n;
+ void *data;
n = bitmap_weight(mask, indio_dev->masklength);
- kfree(xadc->data);
- xadc->data = kcalloc(n, sizeof(*xadc->data), GFP_KERNEL);
- if (!xadc->data)
+ data = devm_krealloc_array(indio_dev->dev.parent, xadc->data, n,
+ sizeof(*xadc->data), GFP_KERNEL);
+ if (!data)
return -ENOMEM;
+ memset(data, 0, n * sizeof(*xadc->data));
+ xadc->data = data;
+
return 0;
}
@@ -1372,7 +1376,6 @@ static int xadc_remove(struct platform_device *pdev)
free_irq(xadc->irq, indio_dev);
cancel_delayed_work_sync(&xadc->zynq_unmask_work);
clk_disable_unprepare(xadc->clk);
- kfree(xadc->data);
return 0;
}
--
2.29.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH v2 4/4] iio: adc: xilinx: use more devres helpers and remove remove()
2020-11-02 14:22 ` Bartosz Golaszewski
@ 2020-11-02 14:22 ` Bartosz Golaszewski
-1 siblings, 0 replies; 26+ messages in thread
From: Bartosz Golaszewski @ 2020-11-02 14:22 UTC (permalink / raw)
To: Jonathan Cameron, Lars-Peter Clausen, Peter Meerwald-Stadler,
Michal Simek
Cc: linux-iio, linux-arm-kernel, linux-kernel, Bartosz Golaszewski
From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
In order to simplify resource management and error paths in probe() and
entirely drop the remove() callback - use devres helpers wherever
possible. Define devm actions for cancelling the delayed work and
disabling the clock.
Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
drivers/iio/adc/xilinx-xadc-core.c | 129 +++++++++++++----------------
1 file changed, 56 insertions(+), 73 deletions(-)
diff --git a/drivers/iio/adc/xilinx-xadc-core.c b/drivers/iio/adc/xilinx-xadc-core.c
index 7792aa4cf9cb..6ce1745b4995 100644
--- a/drivers/iio/adc/xilinx-xadc-core.c
+++ b/drivers/iio/adc/xilinx-xadc-core.c
@@ -709,11 +709,12 @@ static const struct iio_trigger_ops xadc_trigger_ops = {
static struct iio_trigger *xadc_alloc_trigger(struct iio_dev *indio_dev,
const char *name)
{
+ struct device *dev = indio_dev->dev.parent;
struct iio_trigger *trig;
int ret;
- trig = iio_trigger_alloc("%s%d-%s", indio_dev->name,
- indio_dev->id, name);
+ trig = devm_iio_trigger_alloc(dev, "%s%d-%s", indio_dev->name,
+ indio_dev->id, name);
if (trig == NULL)
return ERR_PTR(-ENOMEM);
@@ -721,15 +722,11 @@ static struct iio_trigger *xadc_alloc_trigger(struct iio_dev *indio_dev,
trig->ops = &xadc_trigger_ops;
iio_trigger_set_drvdata(trig, iio_priv(indio_dev));
- ret = iio_trigger_register(trig);
+ ret = devm_iio_trigger_register(dev, trig);
if (ret)
- goto error_free_trig;
+ return ERR_PTR(ret);
return trig;
-
-error_free_trig:
- iio_trigger_free(trig);
- return ERR_PTR(ret);
}
static int xadc_power_adc_b(struct xadc *xadc, unsigned int seq_mode)
@@ -1188,6 +1185,20 @@ static int xadc_parse_dt(struct iio_dev *indio_dev, struct device_node *np,
return 0;
}
+static void xadc_clk_disable_unprepare(void *data)
+{
+ struct clk *clk = data;
+
+ clk_disable_unprepare(clk);
+}
+
+static void xadc_cancel_delayed_work(void *data)
+{
+ struct delayed_work *work = data;
+
+ cancel_delayed_work_sync(work);
+}
+
static int xadc_probe(struct platform_device *pdev)
{
struct device *dev = &pdev->dev;
@@ -1236,34 +1247,35 @@ static int xadc_probe(struct platform_device *pdev)
return ret;
if (xadc->ops->flags & XADC_FLAGS_BUFFERED) {
- ret = iio_triggered_buffer_setup(indio_dev,
- &iio_pollfunc_store_time, &xadc_trigger_handler,
- &xadc_buffer_ops);
+ ret = devm_iio_triggered_buffer_setup(dev, indio_dev,
+ &iio_pollfunc_store_time,
+ &xadc_trigger_handler,
+ &xadc_buffer_ops);
if (ret)
return ret;
xadc->convst_trigger = xadc_alloc_trigger(indio_dev, "convst");
- if (IS_ERR(xadc->convst_trigger)) {
- ret = PTR_ERR(xadc->convst_trigger);
- goto err_triggered_buffer_cleanup;
- }
+ if (IS_ERR(xadc->convst_trigger))
+ return PTR_ERR(xadc->convst_trigger);
+
xadc->samplerate_trigger = xadc_alloc_trigger(indio_dev,
"samplerate");
- if (IS_ERR(xadc->samplerate_trigger)) {
- ret = PTR_ERR(xadc->samplerate_trigger);
- goto err_free_convst_trigger;
- }
+ if (IS_ERR(xadc->samplerate_trigger))
+ return PTR_ERR(xadc->samplerate_trigger);
}
xadc->clk = devm_clk_get(dev, NULL);
- if (IS_ERR(xadc->clk)) {
- ret = PTR_ERR(xadc->clk);
- goto err_free_samplerate_trigger;
- }
+ if (IS_ERR(xadc->clk))
+ return PTR_ERR(xadc->clk);
ret = clk_prepare_enable(xadc->clk);
if (ret)
- goto err_free_samplerate_trigger;
+ return ret;
+
+ ret = devm_add_action_or_reset(dev,
+ xadc_clk_disable_unprepare, xadc->clk);
+ if (ret)
+ return ret;
/*
* Make sure not to exceed the maximum samplerate since otherwise the
@@ -1272,22 +1284,28 @@ static int xadc_probe(struct platform_device *pdev)
if (xadc->ops->flags & XADC_FLAGS_BUFFERED) {
ret = xadc_read_samplerate(xadc);
if (ret < 0)
- goto err_free_samplerate_trigger;
+ return ret;
+
if (ret > XADC_MAX_SAMPLERATE) {
ret = xadc_write_samplerate(xadc, XADC_MAX_SAMPLERATE);
if (ret < 0)
- goto err_free_samplerate_trigger;
+ return ret;
}
}
- ret = request_irq(xadc->irq, xadc->ops->interrupt_handler, 0,
- dev_name(dev), indio_dev);
+ ret = devm_request_irq(dev, xadc->irq, xadc->ops->interrupt_handler, 0,
+ dev_name(dev), indio_dev);
+ if (ret)
+ return ret;
+
+ ret = devm_add_action_or_reset(dev, xadc_cancel_delayed_work,
+ &xadc->zynq_unmask_work);
if (ret)
- goto err_clk_disable_unprepare;
+ return ret;
ret = xadc->ops->setup(pdev, indio_dev, xadc->irq);
if (ret)
- goto err_free_irq;
+ return ret;
for (i = 0; i < 16; i++)
xadc_read_adc_reg(xadc, XADC_REG_THRESHOLD(i),
@@ -1295,7 +1313,7 @@ static int xadc_probe(struct platform_device *pdev)
ret = xadc_write_adc_reg(xadc, XADC_REG_CONF0, conf0);
if (ret)
- goto err_free_irq;
+ return ret;
bipolar_mask = 0;
for (i = 0; i < indio_dev->num_channels; i++) {
@@ -1305,17 +1323,18 @@ static int xadc_probe(struct platform_device *pdev)
ret = xadc_write_adc_reg(xadc, XADC_REG_INPUT_MODE(0), bipolar_mask);
if (ret)
- goto err_free_irq;
+ return ret;
+
ret = xadc_write_adc_reg(xadc, XADC_REG_INPUT_MODE(1),
bipolar_mask >> 16);
if (ret)
- goto err_free_irq;
+ return ret;
/* Disable all alarms */
ret = xadc_update_adc_reg(xadc, XADC_REG_CONF1, XADC_CONF1_ALARM_MASK,
XADC_CONF1_ALARM_MASK);
if (ret)
- goto err_free_irq;
+ return ret;
/* Set thresholds to min/max */
for (i = 0; i < 16; i++) {
@@ -1330,59 +1349,23 @@ static int xadc_probe(struct platform_device *pdev)
ret = xadc_write_adc_reg(xadc, XADC_REG_THRESHOLD(i),
xadc->threshold[i]);
if (ret)
- goto err_free_irq;
+ return ret;
}
/* Go to non-buffered mode */
xadc_postdisable(indio_dev);
- ret = iio_device_register(indio_dev);
+ ret = devm_iio_device_register(dev, indio_dev);
if (ret)
- goto err_free_irq;
+ return ret;
platform_set_drvdata(pdev, indio_dev);
- return 0;
-
-err_free_irq:
- free_irq(xadc->irq, indio_dev);
- cancel_delayed_work_sync(&xadc->zynq_unmask_work);
-err_clk_disable_unprepare:
- clk_disable_unprepare(xadc->clk);
-err_free_samplerate_trigger:
- if (xadc->ops->flags & XADC_FLAGS_BUFFERED)
- iio_trigger_free(xadc->samplerate_trigger);
-err_free_convst_trigger:
- if (xadc->ops->flags & XADC_FLAGS_BUFFERED)
- iio_trigger_free(xadc->convst_trigger);
-err_triggered_buffer_cleanup:
- if (xadc->ops->flags & XADC_FLAGS_BUFFERED)
- iio_triggered_buffer_cleanup(indio_dev);
-
- return ret;
-}
-
-static int xadc_remove(struct platform_device *pdev)
-{
- struct iio_dev *indio_dev = platform_get_drvdata(pdev);
- struct xadc *xadc = iio_priv(indio_dev);
-
- iio_device_unregister(indio_dev);
- if (xadc->ops->flags & XADC_FLAGS_BUFFERED) {
- iio_trigger_free(xadc->samplerate_trigger);
- iio_trigger_free(xadc->convst_trigger);
- iio_triggered_buffer_cleanup(indio_dev);
- }
- free_irq(xadc->irq, indio_dev);
- cancel_delayed_work_sync(&xadc->zynq_unmask_work);
- clk_disable_unprepare(xadc->clk);
-
return 0;
}
static struct platform_driver xadc_driver = {
.probe = xadc_probe,
- .remove = xadc_remove,
.driver = {
.name = "xadc",
.of_match_table = xadc_of_match_table,
--
2.29.1
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH v2 4/4] iio: adc: xilinx: use more devres helpers and remove remove()
@ 2020-11-02 14:22 ` Bartosz Golaszewski
0 siblings, 0 replies; 26+ messages in thread
From: Bartosz Golaszewski @ 2020-11-02 14:22 UTC (permalink / raw)
To: Jonathan Cameron, Lars-Peter Clausen, Peter Meerwald-Stadler,
Michal Simek
Cc: linux-iio, Bartosz Golaszewski, linux-kernel, linux-arm-kernel
From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
In order to simplify resource management and error paths in probe() and
entirely drop the remove() callback - use devres helpers wherever
possible. Define devm actions for cancelling the delayed work and
disabling the clock.
Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
drivers/iio/adc/xilinx-xadc-core.c | 129 +++++++++++++----------------
1 file changed, 56 insertions(+), 73 deletions(-)
diff --git a/drivers/iio/adc/xilinx-xadc-core.c b/drivers/iio/adc/xilinx-xadc-core.c
index 7792aa4cf9cb..6ce1745b4995 100644
--- a/drivers/iio/adc/xilinx-xadc-core.c
+++ b/drivers/iio/adc/xilinx-xadc-core.c
@@ -709,11 +709,12 @@ static const struct iio_trigger_ops xadc_trigger_ops = {
static struct iio_trigger *xadc_alloc_trigger(struct iio_dev *indio_dev,
const char *name)
{
+ struct device *dev = indio_dev->dev.parent;
struct iio_trigger *trig;
int ret;
- trig = iio_trigger_alloc("%s%d-%s", indio_dev->name,
- indio_dev->id, name);
+ trig = devm_iio_trigger_alloc(dev, "%s%d-%s", indio_dev->name,
+ indio_dev->id, name);
if (trig == NULL)
return ERR_PTR(-ENOMEM);
@@ -721,15 +722,11 @@ static struct iio_trigger *xadc_alloc_trigger(struct iio_dev *indio_dev,
trig->ops = &xadc_trigger_ops;
iio_trigger_set_drvdata(trig, iio_priv(indio_dev));
- ret = iio_trigger_register(trig);
+ ret = devm_iio_trigger_register(dev, trig);
if (ret)
- goto error_free_trig;
+ return ERR_PTR(ret);
return trig;
-
-error_free_trig:
- iio_trigger_free(trig);
- return ERR_PTR(ret);
}
static int xadc_power_adc_b(struct xadc *xadc, unsigned int seq_mode)
@@ -1188,6 +1185,20 @@ static int xadc_parse_dt(struct iio_dev *indio_dev, struct device_node *np,
return 0;
}
+static void xadc_clk_disable_unprepare(void *data)
+{
+ struct clk *clk = data;
+
+ clk_disable_unprepare(clk);
+}
+
+static void xadc_cancel_delayed_work(void *data)
+{
+ struct delayed_work *work = data;
+
+ cancel_delayed_work_sync(work);
+}
+
static int xadc_probe(struct platform_device *pdev)
{
struct device *dev = &pdev->dev;
@@ -1236,34 +1247,35 @@ static int xadc_probe(struct platform_device *pdev)
return ret;
if (xadc->ops->flags & XADC_FLAGS_BUFFERED) {
- ret = iio_triggered_buffer_setup(indio_dev,
- &iio_pollfunc_store_time, &xadc_trigger_handler,
- &xadc_buffer_ops);
+ ret = devm_iio_triggered_buffer_setup(dev, indio_dev,
+ &iio_pollfunc_store_time,
+ &xadc_trigger_handler,
+ &xadc_buffer_ops);
if (ret)
return ret;
xadc->convst_trigger = xadc_alloc_trigger(indio_dev, "convst");
- if (IS_ERR(xadc->convst_trigger)) {
- ret = PTR_ERR(xadc->convst_trigger);
- goto err_triggered_buffer_cleanup;
- }
+ if (IS_ERR(xadc->convst_trigger))
+ return PTR_ERR(xadc->convst_trigger);
+
xadc->samplerate_trigger = xadc_alloc_trigger(indio_dev,
"samplerate");
- if (IS_ERR(xadc->samplerate_trigger)) {
- ret = PTR_ERR(xadc->samplerate_trigger);
- goto err_free_convst_trigger;
- }
+ if (IS_ERR(xadc->samplerate_trigger))
+ return PTR_ERR(xadc->samplerate_trigger);
}
xadc->clk = devm_clk_get(dev, NULL);
- if (IS_ERR(xadc->clk)) {
- ret = PTR_ERR(xadc->clk);
- goto err_free_samplerate_trigger;
- }
+ if (IS_ERR(xadc->clk))
+ return PTR_ERR(xadc->clk);
ret = clk_prepare_enable(xadc->clk);
if (ret)
- goto err_free_samplerate_trigger;
+ return ret;
+
+ ret = devm_add_action_or_reset(dev,
+ xadc_clk_disable_unprepare, xadc->clk);
+ if (ret)
+ return ret;
/*
* Make sure not to exceed the maximum samplerate since otherwise the
@@ -1272,22 +1284,28 @@ static int xadc_probe(struct platform_device *pdev)
if (xadc->ops->flags & XADC_FLAGS_BUFFERED) {
ret = xadc_read_samplerate(xadc);
if (ret < 0)
- goto err_free_samplerate_trigger;
+ return ret;
+
if (ret > XADC_MAX_SAMPLERATE) {
ret = xadc_write_samplerate(xadc, XADC_MAX_SAMPLERATE);
if (ret < 0)
- goto err_free_samplerate_trigger;
+ return ret;
}
}
- ret = request_irq(xadc->irq, xadc->ops->interrupt_handler, 0,
- dev_name(dev), indio_dev);
+ ret = devm_request_irq(dev, xadc->irq, xadc->ops->interrupt_handler, 0,
+ dev_name(dev), indio_dev);
+ if (ret)
+ return ret;
+
+ ret = devm_add_action_or_reset(dev, xadc_cancel_delayed_work,
+ &xadc->zynq_unmask_work);
if (ret)
- goto err_clk_disable_unprepare;
+ return ret;
ret = xadc->ops->setup(pdev, indio_dev, xadc->irq);
if (ret)
- goto err_free_irq;
+ return ret;
for (i = 0; i < 16; i++)
xadc_read_adc_reg(xadc, XADC_REG_THRESHOLD(i),
@@ -1295,7 +1313,7 @@ static int xadc_probe(struct platform_device *pdev)
ret = xadc_write_adc_reg(xadc, XADC_REG_CONF0, conf0);
if (ret)
- goto err_free_irq;
+ return ret;
bipolar_mask = 0;
for (i = 0; i < indio_dev->num_channels; i++) {
@@ -1305,17 +1323,18 @@ static int xadc_probe(struct platform_device *pdev)
ret = xadc_write_adc_reg(xadc, XADC_REG_INPUT_MODE(0), bipolar_mask);
if (ret)
- goto err_free_irq;
+ return ret;
+
ret = xadc_write_adc_reg(xadc, XADC_REG_INPUT_MODE(1),
bipolar_mask >> 16);
if (ret)
- goto err_free_irq;
+ return ret;
/* Disable all alarms */
ret = xadc_update_adc_reg(xadc, XADC_REG_CONF1, XADC_CONF1_ALARM_MASK,
XADC_CONF1_ALARM_MASK);
if (ret)
- goto err_free_irq;
+ return ret;
/* Set thresholds to min/max */
for (i = 0; i < 16; i++) {
@@ -1330,59 +1349,23 @@ static int xadc_probe(struct platform_device *pdev)
ret = xadc_write_adc_reg(xadc, XADC_REG_THRESHOLD(i),
xadc->threshold[i]);
if (ret)
- goto err_free_irq;
+ return ret;
}
/* Go to non-buffered mode */
xadc_postdisable(indio_dev);
- ret = iio_device_register(indio_dev);
+ ret = devm_iio_device_register(dev, indio_dev);
if (ret)
- goto err_free_irq;
+ return ret;
platform_set_drvdata(pdev, indio_dev);
- return 0;
-
-err_free_irq:
- free_irq(xadc->irq, indio_dev);
- cancel_delayed_work_sync(&xadc->zynq_unmask_work);
-err_clk_disable_unprepare:
- clk_disable_unprepare(xadc->clk);
-err_free_samplerate_trigger:
- if (xadc->ops->flags & XADC_FLAGS_BUFFERED)
- iio_trigger_free(xadc->samplerate_trigger);
-err_free_convst_trigger:
- if (xadc->ops->flags & XADC_FLAGS_BUFFERED)
- iio_trigger_free(xadc->convst_trigger);
-err_triggered_buffer_cleanup:
- if (xadc->ops->flags & XADC_FLAGS_BUFFERED)
- iio_triggered_buffer_cleanup(indio_dev);
-
- return ret;
-}
-
-static int xadc_remove(struct platform_device *pdev)
-{
- struct iio_dev *indio_dev = platform_get_drvdata(pdev);
- struct xadc *xadc = iio_priv(indio_dev);
-
- iio_device_unregister(indio_dev);
- if (xadc->ops->flags & XADC_FLAGS_BUFFERED) {
- iio_trigger_free(xadc->samplerate_trigger);
- iio_trigger_free(xadc->convst_trigger);
- iio_triggered_buffer_cleanup(indio_dev);
- }
- free_irq(xadc->irq, indio_dev);
- cancel_delayed_work_sync(&xadc->zynq_unmask_work);
- clk_disable_unprepare(xadc->clk);
-
return 0;
}
static struct platform_driver xadc_driver = {
.probe = xadc_probe,
- .remove = xadc_remove,
.driver = {
.name = "xadc",
.of_match_table = xadc_of_match_table,
--
2.29.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH v2 4/4] iio: adc: xilinx: use more devres helpers and remove remove()
2020-11-02 14:22 ` Bartosz Golaszewski
@ 2020-11-08 17:08 ` Jonathan Cameron
-1 siblings, 0 replies; 26+ messages in thread
From: Jonathan Cameron @ 2020-11-08 17:08 UTC (permalink / raw)
To: Bartosz Golaszewski
Cc: Lars-Peter Clausen, Peter Meerwald-Stadler, Michal Simek,
linux-iio, linux-arm-kernel, linux-kernel, Bartosz Golaszewski
On Mon, 2 Nov 2020 15:22:28 +0100
Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
>
> In order to simplify resource management and error paths in probe() and
> entirely drop the remove() callback - use devres helpers wherever
> possible. Define devm actions for cancelling the delayed work and
> disabling the clock.
>
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
One addition inline.
thanks,
Jonathan
> ---
> drivers/iio/adc/xilinx-xadc-core.c | 129 +++++++++++++----------------
> 1 file changed, 56 insertions(+), 73 deletions(-)
>
> diff --git a/drivers/iio/adc/xilinx-xadc-core.c b/drivers/iio/adc/xilinx-xadc-core.c
> index 7792aa4cf9cb..6ce1745b4995 100644
> --- a/drivers/iio/adc/xilinx-xadc-core.c
> +++ b/drivers/iio/adc/xilinx-xadc-core.c
> @@ -709,11 +709,12 @@ static const struct iio_trigger_ops xadc_trigger_ops = {
> static struct iio_trigger *xadc_alloc_trigger(struct iio_dev *indio_dev,
> const char *name)
> {
> + struct device *dev = indio_dev->dev.parent;
> struct iio_trigger *trig;
> int ret;
>
> - trig = iio_trigger_alloc("%s%d-%s", indio_dev->name,
> - indio_dev->id, name);
> + trig = devm_iio_trigger_alloc(dev, "%s%d-%s", indio_dev->name,
> + indio_dev->id, name);
> if (trig == NULL)
> return ERR_PTR(-ENOMEM);
>
> @@ -721,15 +722,11 @@ static struct iio_trigger *xadc_alloc_trigger(struct iio_dev *indio_dev,
> trig->ops = &xadc_trigger_ops;
> iio_trigger_set_drvdata(trig, iio_priv(indio_dev));
>
> - ret = iio_trigger_register(trig);
> + ret = devm_iio_trigger_register(dev, trig);
> if (ret)
> - goto error_free_trig;
> + return ERR_PTR(ret);
>
> return trig;
> -
> -error_free_trig:
> - iio_trigger_free(trig);
> - return ERR_PTR(ret);
> }
>
> static int xadc_power_adc_b(struct xadc *xadc, unsigned int seq_mode)
> @@ -1188,6 +1185,20 @@ static int xadc_parse_dt(struct iio_dev *indio_dev, struct device_node *np,
> return 0;
> }
>
> +static void xadc_clk_disable_unprepare(void *data)
> +{
> + struct clk *clk = data;
> +
> + clk_disable_unprepare(clk);
> +}
> +
> +static void xadc_cancel_delayed_work(void *data)
> +{
> + struct delayed_work *work = data;
> +
> + cancel_delayed_work_sync(work);
> +}
> +
> static int xadc_probe(struct platform_device *pdev)
> {
> struct device *dev = &pdev->dev;
> @@ -1236,34 +1247,35 @@ static int xadc_probe(struct platform_device *pdev)
> return ret;
>
> if (xadc->ops->flags & XADC_FLAGS_BUFFERED) {
> - ret = iio_triggered_buffer_setup(indio_dev,
> - &iio_pollfunc_store_time, &xadc_trigger_handler,
> - &xadc_buffer_ops);
> + ret = devm_iio_triggered_buffer_setup(dev, indio_dev,
> + &iio_pollfunc_store_time,
> + &xadc_trigger_handler,
> + &xadc_buffer_ops);
> if (ret)
> return ret;
>
> xadc->convst_trigger = xadc_alloc_trigger(indio_dev, "convst");
> - if (IS_ERR(xadc->convst_trigger)) {
> - ret = PTR_ERR(xadc->convst_trigger);
> - goto err_triggered_buffer_cleanup;
> - }
> + if (IS_ERR(xadc->convst_trigger))
> + return PTR_ERR(xadc->convst_trigger);
> +
> xadc->samplerate_trigger = xadc_alloc_trigger(indio_dev,
> "samplerate");
> - if (IS_ERR(xadc->samplerate_trigger)) {
> - ret = PTR_ERR(xadc->samplerate_trigger);
> - goto err_free_convst_trigger;
> - }
> + if (IS_ERR(xadc->samplerate_trigger))
> + return PTR_ERR(xadc->samplerate_trigger);
> }
>
> xadc->clk = devm_clk_get(dev, NULL);
> - if (IS_ERR(xadc->clk)) {
> - ret = PTR_ERR(xadc->clk);
> - goto err_free_samplerate_trigger;
> - }
> + if (IS_ERR(xadc->clk))
> + return PTR_ERR(xadc->clk);
>
> ret = clk_prepare_enable(xadc->clk);
> if (ret)
> - goto err_free_samplerate_trigger;
> + return ret;
> +
> + ret = devm_add_action_or_reset(dev,
> + xadc_clk_disable_unprepare, xadc->clk);
> + if (ret)
> + return ret;
>
> /*
> * Make sure not to exceed the maximum samplerate since otherwise the
> @@ -1272,22 +1284,28 @@ static int xadc_probe(struct platform_device *pdev)
> if (xadc->ops->flags & XADC_FLAGS_BUFFERED) {
> ret = xadc_read_samplerate(xadc);
> if (ret < 0)
> - goto err_free_samplerate_trigger;
> + return ret;
> +
> if (ret > XADC_MAX_SAMPLERATE) {
> ret = xadc_write_samplerate(xadc, XADC_MAX_SAMPLERATE);
> if (ret < 0)
> - goto err_free_samplerate_trigger;
> + return ret;
> }
> }
>
> - ret = request_irq(xadc->irq, xadc->ops->interrupt_handler, 0,
> - dev_name(dev), indio_dev);
> + ret = devm_request_irq(dev, xadc->irq, xadc->ops->interrupt_handler, 0,
> + dev_name(dev), indio_dev);
> + if (ret)
> + return ret;
> +
> + ret = devm_add_action_or_reset(dev, xadc_cancel_delayed_work,
> + &xadc->zynq_unmask_work);
> if (ret)
> - goto err_clk_disable_unprepare;
> + return ret;
>
> ret = xadc->ops->setup(pdev, indio_dev, xadc->irq);
> if (ret)
> - goto err_free_irq;
> + return ret;
>
> for (i = 0; i < 16; i++)
> xadc_read_adc_reg(xadc, XADC_REG_THRESHOLD(i),
> @@ -1295,7 +1313,7 @@ static int xadc_probe(struct platform_device *pdev)
>
> ret = xadc_write_adc_reg(xadc, XADC_REG_CONF0, conf0);
> if (ret)
> - goto err_free_irq;
> + return ret;
>
> bipolar_mask = 0;
> for (i = 0; i < indio_dev->num_channels; i++) {
> @@ -1305,17 +1323,18 @@ static int xadc_probe(struct platform_device *pdev)
>
> ret = xadc_write_adc_reg(xadc, XADC_REG_INPUT_MODE(0), bipolar_mask);
> if (ret)
> - goto err_free_irq;
> + return ret;
> +
> ret = xadc_write_adc_reg(xadc, XADC_REG_INPUT_MODE(1),
> bipolar_mask >> 16);
> if (ret)
> - goto err_free_irq;
> + return ret;
>
> /* Disable all alarms */
> ret = xadc_update_adc_reg(xadc, XADC_REG_CONF1, XADC_CONF1_ALARM_MASK,
> XADC_CONF1_ALARM_MASK);
> if (ret)
> - goto err_free_irq;
> + return ret;
>
> /* Set thresholds to min/max */
> for (i = 0; i < 16; i++) {
> @@ -1330,59 +1349,23 @@ static int xadc_probe(struct platform_device *pdev)
> ret = xadc_write_adc_reg(xadc, XADC_REG_THRESHOLD(i),
> xadc->threshold[i]);
> if (ret)
> - goto err_free_irq;
> + return ret;
> }
>
> /* Go to non-buffered mode */
> xadc_postdisable(indio_dev);
>
> - ret = iio_device_register(indio_dev);
> + ret = devm_iio_device_register(dev, indio_dev);
> if (ret)
> - goto err_free_irq;
> + return ret;
>
> platform_set_drvdata(pdev, indio_dev);
I haven't checked carefully, but it looks like you don't need
this any more as there are no remaining calls to
platform_get_drvdata().
I can clean that up whilst applying as long as you confirm
I'm right!
Thanks,
Jonathan
>
> - return 0;
> -
> -err_free_irq:
> - free_irq(xadc->irq, indio_dev);
> - cancel_delayed_work_sync(&xadc->zynq_unmask_work);
> -err_clk_disable_unprepare:
> - clk_disable_unprepare(xadc->clk);
> -err_free_samplerate_trigger:
> - if (xadc->ops->flags & XADC_FLAGS_BUFFERED)
> - iio_trigger_free(xadc->samplerate_trigger);
> -err_free_convst_trigger:
> - if (xadc->ops->flags & XADC_FLAGS_BUFFERED)
> - iio_trigger_free(xadc->convst_trigger);
> -err_triggered_buffer_cleanup:
> - if (xadc->ops->flags & XADC_FLAGS_BUFFERED)
> - iio_triggered_buffer_cleanup(indio_dev);
> -
> - return ret;
> -}
> -
> -static int xadc_remove(struct platform_device *pdev)
> -{
> - struct iio_dev *indio_dev = platform_get_drvdata(pdev);
> - struct xadc *xadc = iio_priv(indio_dev);
> -
> - iio_device_unregister(indio_dev);
> - if (xadc->ops->flags & XADC_FLAGS_BUFFERED) {
> - iio_trigger_free(xadc->samplerate_trigger);
> - iio_trigger_free(xadc->convst_trigger);
> - iio_triggered_buffer_cleanup(indio_dev);
> - }
> - free_irq(xadc->irq, indio_dev);
> - cancel_delayed_work_sync(&xadc->zynq_unmask_work);
> - clk_disable_unprepare(xadc->clk);
> -
> return 0;
> }
>
> static struct platform_driver xadc_driver = {
> .probe = xadc_probe,
> - .remove = xadc_remove,
> .driver = {
> .name = "xadc",
> .of_match_table = xadc_of_match_table,
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2 4/4] iio: adc: xilinx: use more devres helpers and remove remove()
@ 2020-11-08 17:08 ` Jonathan Cameron
0 siblings, 0 replies; 26+ messages in thread
From: Jonathan Cameron @ 2020-11-08 17:08 UTC (permalink / raw)
To: Bartosz Golaszewski
Cc: Lars-Peter Clausen, linux-iio, linux-kernel, Michal Simek,
Bartosz Golaszewski, Peter Meerwald-Stadler, linux-arm-kernel
On Mon, 2 Nov 2020 15:22:28 +0100
Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
>
> In order to simplify resource management and error paths in probe() and
> entirely drop the remove() callback - use devres helpers wherever
> possible. Define devm actions for cancelling the delayed work and
> disabling the clock.
>
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
One addition inline.
thanks,
Jonathan
> ---
> drivers/iio/adc/xilinx-xadc-core.c | 129 +++++++++++++----------------
> 1 file changed, 56 insertions(+), 73 deletions(-)
>
> diff --git a/drivers/iio/adc/xilinx-xadc-core.c b/drivers/iio/adc/xilinx-xadc-core.c
> index 7792aa4cf9cb..6ce1745b4995 100644
> --- a/drivers/iio/adc/xilinx-xadc-core.c
> +++ b/drivers/iio/adc/xilinx-xadc-core.c
> @@ -709,11 +709,12 @@ static const struct iio_trigger_ops xadc_trigger_ops = {
> static struct iio_trigger *xadc_alloc_trigger(struct iio_dev *indio_dev,
> const char *name)
> {
> + struct device *dev = indio_dev->dev.parent;
> struct iio_trigger *trig;
> int ret;
>
> - trig = iio_trigger_alloc("%s%d-%s", indio_dev->name,
> - indio_dev->id, name);
> + trig = devm_iio_trigger_alloc(dev, "%s%d-%s", indio_dev->name,
> + indio_dev->id, name);
> if (trig == NULL)
> return ERR_PTR(-ENOMEM);
>
> @@ -721,15 +722,11 @@ static struct iio_trigger *xadc_alloc_trigger(struct iio_dev *indio_dev,
> trig->ops = &xadc_trigger_ops;
> iio_trigger_set_drvdata(trig, iio_priv(indio_dev));
>
> - ret = iio_trigger_register(trig);
> + ret = devm_iio_trigger_register(dev, trig);
> if (ret)
> - goto error_free_trig;
> + return ERR_PTR(ret);
>
> return trig;
> -
> -error_free_trig:
> - iio_trigger_free(trig);
> - return ERR_PTR(ret);
> }
>
> static int xadc_power_adc_b(struct xadc *xadc, unsigned int seq_mode)
> @@ -1188,6 +1185,20 @@ static int xadc_parse_dt(struct iio_dev *indio_dev, struct device_node *np,
> return 0;
> }
>
> +static void xadc_clk_disable_unprepare(void *data)
> +{
> + struct clk *clk = data;
> +
> + clk_disable_unprepare(clk);
> +}
> +
> +static void xadc_cancel_delayed_work(void *data)
> +{
> + struct delayed_work *work = data;
> +
> + cancel_delayed_work_sync(work);
> +}
> +
> static int xadc_probe(struct platform_device *pdev)
> {
> struct device *dev = &pdev->dev;
> @@ -1236,34 +1247,35 @@ static int xadc_probe(struct platform_device *pdev)
> return ret;
>
> if (xadc->ops->flags & XADC_FLAGS_BUFFERED) {
> - ret = iio_triggered_buffer_setup(indio_dev,
> - &iio_pollfunc_store_time, &xadc_trigger_handler,
> - &xadc_buffer_ops);
> + ret = devm_iio_triggered_buffer_setup(dev, indio_dev,
> + &iio_pollfunc_store_time,
> + &xadc_trigger_handler,
> + &xadc_buffer_ops);
> if (ret)
> return ret;
>
> xadc->convst_trigger = xadc_alloc_trigger(indio_dev, "convst");
> - if (IS_ERR(xadc->convst_trigger)) {
> - ret = PTR_ERR(xadc->convst_trigger);
> - goto err_triggered_buffer_cleanup;
> - }
> + if (IS_ERR(xadc->convst_trigger))
> + return PTR_ERR(xadc->convst_trigger);
> +
> xadc->samplerate_trigger = xadc_alloc_trigger(indio_dev,
> "samplerate");
> - if (IS_ERR(xadc->samplerate_trigger)) {
> - ret = PTR_ERR(xadc->samplerate_trigger);
> - goto err_free_convst_trigger;
> - }
> + if (IS_ERR(xadc->samplerate_trigger))
> + return PTR_ERR(xadc->samplerate_trigger);
> }
>
> xadc->clk = devm_clk_get(dev, NULL);
> - if (IS_ERR(xadc->clk)) {
> - ret = PTR_ERR(xadc->clk);
> - goto err_free_samplerate_trigger;
> - }
> + if (IS_ERR(xadc->clk))
> + return PTR_ERR(xadc->clk);
>
> ret = clk_prepare_enable(xadc->clk);
> if (ret)
> - goto err_free_samplerate_trigger;
> + return ret;
> +
> + ret = devm_add_action_or_reset(dev,
> + xadc_clk_disable_unprepare, xadc->clk);
> + if (ret)
> + return ret;
>
> /*
> * Make sure not to exceed the maximum samplerate since otherwise the
> @@ -1272,22 +1284,28 @@ static int xadc_probe(struct platform_device *pdev)
> if (xadc->ops->flags & XADC_FLAGS_BUFFERED) {
> ret = xadc_read_samplerate(xadc);
> if (ret < 0)
> - goto err_free_samplerate_trigger;
> + return ret;
> +
> if (ret > XADC_MAX_SAMPLERATE) {
> ret = xadc_write_samplerate(xadc, XADC_MAX_SAMPLERATE);
> if (ret < 0)
> - goto err_free_samplerate_trigger;
> + return ret;
> }
> }
>
> - ret = request_irq(xadc->irq, xadc->ops->interrupt_handler, 0,
> - dev_name(dev), indio_dev);
> + ret = devm_request_irq(dev, xadc->irq, xadc->ops->interrupt_handler, 0,
> + dev_name(dev), indio_dev);
> + if (ret)
> + return ret;
> +
> + ret = devm_add_action_or_reset(dev, xadc_cancel_delayed_work,
> + &xadc->zynq_unmask_work);
> if (ret)
> - goto err_clk_disable_unprepare;
> + return ret;
>
> ret = xadc->ops->setup(pdev, indio_dev, xadc->irq);
> if (ret)
> - goto err_free_irq;
> + return ret;
>
> for (i = 0; i < 16; i++)
> xadc_read_adc_reg(xadc, XADC_REG_THRESHOLD(i),
> @@ -1295,7 +1313,7 @@ static int xadc_probe(struct platform_device *pdev)
>
> ret = xadc_write_adc_reg(xadc, XADC_REG_CONF0, conf0);
> if (ret)
> - goto err_free_irq;
> + return ret;
>
> bipolar_mask = 0;
> for (i = 0; i < indio_dev->num_channels; i++) {
> @@ -1305,17 +1323,18 @@ static int xadc_probe(struct platform_device *pdev)
>
> ret = xadc_write_adc_reg(xadc, XADC_REG_INPUT_MODE(0), bipolar_mask);
> if (ret)
> - goto err_free_irq;
> + return ret;
> +
> ret = xadc_write_adc_reg(xadc, XADC_REG_INPUT_MODE(1),
> bipolar_mask >> 16);
> if (ret)
> - goto err_free_irq;
> + return ret;
>
> /* Disable all alarms */
> ret = xadc_update_adc_reg(xadc, XADC_REG_CONF1, XADC_CONF1_ALARM_MASK,
> XADC_CONF1_ALARM_MASK);
> if (ret)
> - goto err_free_irq;
> + return ret;
>
> /* Set thresholds to min/max */
> for (i = 0; i < 16; i++) {
> @@ -1330,59 +1349,23 @@ static int xadc_probe(struct platform_device *pdev)
> ret = xadc_write_adc_reg(xadc, XADC_REG_THRESHOLD(i),
> xadc->threshold[i]);
> if (ret)
> - goto err_free_irq;
> + return ret;
> }
>
> /* Go to non-buffered mode */
> xadc_postdisable(indio_dev);
>
> - ret = iio_device_register(indio_dev);
> + ret = devm_iio_device_register(dev, indio_dev);
> if (ret)
> - goto err_free_irq;
> + return ret;
>
> platform_set_drvdata(pdev, indio_dev);
I haven't checked carefully, but it looks like you don't need
this any more as there are no remaining calls to
platform_get_drvdata().
I can clean that up whilst applying as long as you confirm
I'm right!
Thanks,
Jonathan
>
> - return 0;
> -
> -err_free_irq:
> - free_irq(xadc->irq, indio_dev);
> - cancel_delayed_work_sync(&xadc->zynq_unmask_work);
> -err_clk_disable_unprepare:
> - clk_disable_unprepare(xadc->clk);
> -err_free_samplerate_trigger:
> - if (xadc->ops->flags & XADC_FLAGS_BUFFERED)
> - iio_trigger_free(xadc->samplerate_trigger);
> -err_free_convst_trigger:
> - if (xadc->ops->flags & XADC_FLAGS_BUFFERED)
> - iio_trigger_free(xadc->convst_trigger);
> -err_triggered_buffer_cleanup:
> - if (xadc->ops->flags & XADC_FLAGS_BUFFERED)
> - iio_triggered_buffer_cleanup(indio_dev);
> -
> - return ret;
> -}
> -
> -static int xadc_remove(struct platform_device *pdev)
> -{
> - struct iio_dev *indio_dev = platform_get_drvdata(pdev);
> - struct xadc *xadc = iio_priv(indio_dev);
> -
> - iio_device_unregister(indio_dev);
> - if (xadc->ops->flags & XADC_FLAGS_BUFFERED) {
> - iio_trigger_free(xadc->samplerate_trigger);
> - iio_trigger_free(xadc->convst_trigger);
> - iio_triggered_buffer_cleanup(indio_dev);
> - }
> - free_irq(xadc->irq, indio_dev);
> - cancel_delayed_work_sync(&xadc->zynq_unmask_work);
> - clk_disable_unprepare(xadc->clk);
> -
> return 0;
> }
>
> static struct platform_driver xadc_driver = {
> .probe = xadc_probe,
> - .remove = xadc_remove,
> .driver = {
> .name = "xadc",
> .of_match_table = xadc_of_match_table,
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2 0/4] iio: adc: xilinx: use even more devres
2020-11-02 14:22 ` Bartosz Golaszewski
@ 2020-11-08 17:08 ` Jonathan Cameron
-1 siblings, 0 replies; 26+ messages in thread
From: Jonathan Cameron @ 2020-11-08 17:08 UTC (permalink / raw)
To: Bartosz Golaszewski
Cc: Lars-Peter Clausen, Peter Meerwald-Stadler, Michal Simek,
linux-iio, linux-arm-kernel, linux-kernel, Bartosz Golaszewski
On Mon, 2 Nov 2020 15:22:24 +0100
Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
>
> This is a follow-up to commit 750628c79bb1 ("iio: adc: xilinx-xadc: use
> devm_krealloc()"). I noticed we can use even more devres helpers and entirely
> drop the remove() callback.
Given the generic nature of the additions to device.h I'm going to
let this one sit a little longer for review.
Remind me in a few weeks time if I seem to have lost it.
Also, do a quick sanity check on whether I am fine to drop
the platform_set_drvdata in the final patch.
Thanks,
Jonathan
>
> v1 -> v2:
> - squash three patches adding more devres calls into one for easier review
> - don't insist on the 80 characters limit
> - add a new helper: devm_krealloc_array() and use it
>
> Bartosz Golaszewski (4):
> device: provide devm_krealloc_array()
> iio: adc: xilinx: use helper variable for &pdev->dev
> iio: adc: xilinx: use devm_krealloc_array() instead of kfree() +
> kcalloc()
> iio: adc: xilinx: use more devres helpers and remove remove()
>
> drivers/iio/adc/xilinx-xadc-core.c | 151 +++++++++++++----------------
> include/linux/device.h | 11 +++
> 2 files changed, 80 insertions(+), 82 deletions(-)
>
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2 0/4] iio: adc: xilinx: use even more devres
@ 2020-11-08 17:08 ` Jonathan Cameron
0 siblings, 0 replies; 26+ messages in thread
From: Jonathan Cameron @ 2020-11-08 17:08 UTC (permalink / raw)
To: Bartosz Golaszewski
Cc: Lars-Peter Clausen, linux-iio, linux-kernel, Michal Simek,
Bartosz Golaszewski, Peter Meerwald-Stadler, linux-arm-kernel
On Mon, 2 Nov 2020 15:22:24 +0100
Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
>
> This is a follow-up to commit 750628c79bb1 ("iio: adc: xilinx-xadc: use
> devm_krealloc()"). I noticed we can use even more devres helpers and entirely
> drop the remove() callback.
Given the generic nature of the additions to device.h I'm going to
let this one sit a little longer for review.
Remind me in a few weeks time if I seem to have lost it.
Also, do a quick sanity check on whether I am fine to drop
the platform_set_drvdata in the final patch.
Thanks,
Jonathan
>
> v1 -> v2:
> - squash three patches adding more devres calls into one for easier review
> - don't insist on the 80 characters limit
> - add a new helper: devm_krealloc_array() and use it
>
> Bartosz Golaszewski (4):
> device: provide devm_krealloc_array()
> iio: adc: xilinx: use helper variable for &pdev->dev
> iio: adc: xilinx: use devm_krealloc_array() instead of kfree() +
> kcalloc()
> iio: adc: xilinx: use more devres helpers and remove remove()
>
> drivers/iio/adc/xilinx-xadc-core.c | 151 +++++++++++++----------------
> include/linux/device.h | 11 +++
> 2 files changed, 80 insertions(+), 82 deletions(-)
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2 4/4] iio: adc: xilinx: use more devres helpers and remove remove()
2020-11-08 17:08 ` Jonathan Cameron
@ 2020-11-09 10:52 ` Bartosz Golaszewski
-1 siblings, 0 replies; 26+ messages in thread
From: Bartosz Golaszewski @ 2020-11-09 10:52 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Lars-Peter Clausen, Peter Meerwald-Stadler, Michal Simek,
linux-iio, Linux ARM, Linux Kernel Mailing List,
Bartosz Golaszewski
On Sun, Nov 8, 2020 at 6:08 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
[snip]
>
> I haven't checked carefully, but it looks like you don't need
> this any more as there are no remaining calls to
> platform_get_drvdata().
>
This looks to be correct.
There's a call to iio_trigger_get_drvdata() and it accesses the same
drvdata but it's called on the trigger device and there's a
corresponding call to iio_trigger_set_drvdata() for it.
>
> I can clean that up whilst applying as long as you confirm
> I'm right!
>
Yes please do, thanks!
Bartosz
> Thanks,
>
> Jonathan
>
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2 4/4] iio: adc: xilinx: use more devres helpers and remove remove()
@ 2020-11-09 10:52 ` Bartosz Golaszewski
0 siblings, 0 replies; 26+ messages in thread
From: Bartosz Golaszewski @ 2020-11-09 10:52 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Lars-Peter Clausen, linux-iio, Linux Kernel Mailing List,
Michal Simek, Bartosz Golaszewski, Peter Meerwald-Stadler,
Linux ARM
On Sun, Nov 8, 2020 at 6:08 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
[snip]
>
> I haven't checked carefully, but it looks like you don't need
> this any more as there are no remaining calls to
> platform_get_drvdata().
>
This looks to be correct.
There's a call to iio_trigger_get_drvdata() and it accesses the same
drvdata but it's called on the trigger device and there's a
corresponding call to iio_trigger_set_drvdata() for it.
>
> I can clean that up whilst applying as long as you confirm
> I'm right!
>
Yes please do, thanks!
Bartosz
> Thanks,
>
> Jonathan
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2 1/4] device: provide devm_krealloc_array()
2020-11-02 14:22 ` Bartosz Golaszewski
@ 2020-11-14 15:46 ` Jonathan Cameron
-1 siblings, 0 replies; 26+ messages in thread
From: Jonathan Cameron @ 2020-11-14 15:46 UTC (permalink / raw)
To: Bartosz Golaszewski
Cc: Lars-Peter Clausen, Peter Meerwald-Stadler, Michal Simek,
linux-iio, linux-arm-kernel, linux-kernel, Bartosz Golaszewski,
gregkh
On Mon, 2 Nov 2020 15:22:25 +0100
Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
>
> When allocating an array of elements, users should check for
> multiplication overflow or preferably use one of the provided helpers
> like: devm_kmalloc_array().
>
> This provides devm_krealloc_array() for users who want to reallocate
> managed arrays.
>
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
+CC Greg KH.
As this is going into a very generic place I'd like a relevant ack.
That file is a bit of a wild west for acks, but Greg seems most
appropriate person.
So Greg, any comments on this one?
Thanks,
Jonathan
> ---
> include/linux/device.h | 11 +++++++++++
> 1 file changed, 11 insertions(+)
>
> diff --git a/include/linux/device.h b/include/linux/device.h
> index 5ed101be7b2e..e77203faea55 100644
> --- a/include/linux/device.h
> +++ b/include/linux/device.h
> @@ -226,6 +226,17 @@ static inline void *devm_kmalloc_array(struct device *dev,
>
> return devm_kmalloc(dev, bytes, flags);
> }
> +static inline void *
> +devm_krealloc_array(struct device *dev, void *ptr, size_t new_n,
> + size_t new_size, gfp_t gfp)
> +{
> + size_t bytes;
> +
> + if (unlikely(check_mul_overflow(new_n, new_size, &bytes)))
> + return NULL;
> +
> + return devm_krealloc(dev, ptr, bytes, gfp);
> +}
> static inline void *devm_kcalloc(struct device *dev,
> size_t n, size_t size, gfp_t flags)
> {
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2 1/4] device: provide devm_krealloc_array()
@ 2020-11-14 15:46 ` Jonathan Cameron
0 siblings, 0 replies; 26+ messages in thread
From: Jonathan Cameron @ 2020-11-14 15:46 UTC (permalink / raw)
To: Bartosz Golaszewski
Cc: Lars-Peter Clausen, linux-iio, gregkh, linux-kernel,
Michal Simek, Bartosz Golaszewski, Peter Meerwald-Stadler,
linux-arm-kernel
On Mon, 2 Nov 2020 15:22:25 +0100
Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
>
> When allocating an array of elements, users should check for
> multiplication overflow or preferably use one of the provided helpers
> like: devm_kmalloc_array().
>
> This provides devm_krealloc_array() for users who want to reallocate
> managed arrays.
>
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
+CC Greg KH.
As this is going into a very generic place I'd like a relevant ack.
That file is a bit of a wild west for acks, but Greg seems most
appropriate person.
So Greg, any comments on this one?
Thanks,
Jonathan
> ---
> include/linux/device.h | 11 +++++++++++
> 1 file changed, 11 insertions(+)
>
> diff --git a/include/linux/device.h b/include/linux/device.h
> index 5ed101be7b2e..e77203faea55 100644
> --- a/include/linux/device.h
> +++ b/include/linux/device.h
> @@ -226,6 +226,17 @@ static inline void *devm_kmalloc_array(struct device *dev,
>
> return devm_kmalloc(dev, bytes, flags);
> }
> +static inline void *
> +devm_krealloc_array(struct device *dev, void *ptr, size_t new_n,
> + size_t new_size, gfp_t gfp)
> +{
> + size_t bytes;
> +
> + if (unlikely(check_mul_overflow(new_n, new_size, &bytes)))
> + return NULL;
> +
> + return devm_krealloc(dev, ptr, bytes, gfp);
> +}
> static inline void *devm_kcalloc(struct device *dev,
> size_t n, size_t size, gfp_t flags)
> {
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2 1/4] device: provide devm_krealloc_array()
2020-11-14 15:46 ` Jonathan Cameron
@ 2020-11-14 16:17 ` Greg KH
-1 siblings, 0 replies; 26+ messages in thread
From: Greg KH @ 2020-11-14 16:17 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Bartosz Golaszewski, Lars-Peter Clausen, Peter Meerwald-Stadler,
Michal Simek, linux-iio, linux-arm-kernel, linux-kernel,
Bartosz Golaszewski
On Sat, Nov 14, 2020 at 03:46:41PM +0000, Jonathan Cameron wrote:
> On Mon, 2 Nov 2020 15:22:25 +0100
> Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>
> > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> >
> > When allocating an array of elements, users should check for
> > multiplication overflow or preferably use one of the provided helpers
> > like: devm_kmalloc_array().
> >
> > This provides devm_krealloc_array() for users who want to reallocate
> > managed arrays.
> >
> > Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
>
> +CC Greg KH.
>
> As this is going into a very generic place I'd like a relevant ack.
> That file is a bit of a wild west for acks, but Greg seems most
> appropriate person.
>
> So Greg, any comments on this one?
As there is only 1 user of this function in the patch series, you don't
save any extra code space here, I don't think this is worth it.
We are seeing less and less gains from these new devm_* additions, and
only more confusion and problems with them. So perhaps don't add this?
I don't think it is needed.
thanks,
greg k-h
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2 1/4] device: provide devm_krealloc_array()
@ 2020-11-14 16:17 ` Greg KH
0 siblings, 0 replies; 26+ messages in thread
From: Greg KH @ 2020-11-14 16:17 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Lars-Peter Clausen, linux-iio, Bartosz Golaszewski, Michal Simek,
linux-kernel, Bartosz Golaszewski, Peter Meerwald-Stadler,
linux-arm-kernel
On Sat, Nov 14, 2020 at 03:46:41PM +0000, Jonathan Cameron wrote:
> On Mon, 2 Nov 2020 15:22:25 +0100
> Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>
> > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> >
> > When allocating an array of elements, users should check for
> > multiplication overflow or preferably use one of the provided helpers
> > like: devm_kmalloc_array().
> >
> > This provides devm_krealloc_array() for users who want to reallocate
> > managed arrays.
> >
> > Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
>
> +CC Greg KH.
>
> As this is going into a very generic place I'd like a relevant ack.
> That file is a bit of a wild west for acks, but Greg seems most
> appropriate person.
>
> So Greg, any comments on this one?
As there is only 1 user of this function in the patch series, you don't
save any extra code space here, I don't think this is worth it.
We are seeing less and less gains from these new devm_* additions, and
only more confusion and problems with them. So perhaps don't add this?
I don't think it is needed.
thanks,
greg k-h
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2 1/4] device: provide devm_krealloc_array()
2020-11-14 16:17 ` Greg KH
@ 2020-11-16 10:18 ` Bartosz Golaszewski
-1 siblings, 0 replies; 26+ messages in thread
From: Bartosz Golaszewski @ 2020-11-16 10:18 UTC (permalink / raw)
To: Greg KH
Cc: Jonathan Cameron, Bartosz Golaszewski, Lars-Peter Clausen,
Peter Meerwald-Stadler, Michal Simek, linux-iio, arm-soc, LKML
On Sat, Nov 14, 2020 at 5:16 PM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Sat, Nov 14, 2020 at 03:46:41PM +0000, Jonathan Cameron wrote:
> > On Mon, 2 Nov 2020 15:22:25 +0100
> > Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> >
> > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> > >
> > > When allocating an array of elements, users should check for
> > > multiplication overflow or preferably use one of the provided helpers
> > > like: devm_kmalloc_array().
> > >
> > > This provides devm_krealloc_array() for users who want to reallocate
> > > managed arrays.
> > >
> > > Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> >
> > +CC Greg KH.
> >
> > As this is going into a very generic place I'd like a relevant ack.
> > That file is a bit of a wild west for acks, but Greg seems most
> > appropriate person.
> >
> > So Greg, any comments on this one?
>
> As there is only 1 user of this function in the patch series, you don't
> save any extra code space here, I don't think this is worth it.
>
It's worth it in that the overflow check before allocation doesn't
seem to belong in a driver IMO but is a general check that should live
in common code.
> We are seeing less and less gains from these new devm_* additions, and
> only more confusion and problems with them. So perhaps don't add this?
> I don't think it is needed.
>
I think you're referring to the discussion on
devm_platform_ioremap_resource()? I would argue that consolidation of
common operations in helpers is rarely a bad thing but it's a
discussion for another thread.
I'm not too attached to this patch - if you think this should be
dropped then fine, but I don't see how the name devm_krealloc_array()
can confuse anyone.
Bartosz
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2 1/4] device: provide devm_krealloc_array()
@ 2020-11-16 10:18 ` Bartosz Golaszewski
0 siblings, 0 replies; 26+ messages in thread
From: Bartosz Golaszewski @ 2020-11-16 10:18 UTC (permalink / raw)
To: Greg KH
Cc: Lars-Peter Clausen, linux-iio, Bartosz Golaszewski, Michal Simek,
LKML, arm-soc, Peter Meerwald-Stadler, Jonathan Cameron
On Sat, Nov 14, 2020 at 5:16 PM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Sat, Nov 14, 2020 at 03:46:41PM +0000, Jonathan Cameron wrote:
> > On Mon, 2 Nov 2020 15:22:25 +0100
> > Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> >
> > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> > >
> > > When allocating an array of elements, users should check for
> > > multiplication overflow or preferably use one of the provided helpers
> > > like: devm_kmalloc_array().
> > >
> > > This provides devm_krealloc_array() for users who want to reallocate
> > > managed arrays.
> > >
> > > Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> >
> > +CC Greg KH.
> >
> > As this is going into a very generic place I'd like a relevant ack.
> > That file is a bit of a wild west for acks, but Greg seems most
> > appropriate person.
> >
> > So Greg, any comments on this one?
>
> As there is only 1 user of this function in the patch series, you don't
> save any extra code space here, I don't think this is worth it.
>
It's worth it in that the overflow check before allocation doesn't
seem to belong in a driver IMO but is a general check that should live
in common code.
> We are seeing less and less gains from these new devm_* additions, and
> only more confusion and problems with them. So perhaps don't add this?
> I don't think it is needed.
>
I think you're referring to the discussion on
devm_platform_ioremap_resource()? I would argue that consolidation of
common operations in helpers is rarely a bad thing but it's a
discussion for another thread.
I'm not too attached to this patch - if you think this should be
dropped then fine, but I don't see how the name devm_krealloc_array()
can confuse anyone.
Bartosz
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2 1/4] device: provide devm_krealloc_array()
2020-11-16 10:18 ` Bartosz Golaszewski
@ 2020-11-23 11:38 ` Bartosz Golaszewski
-1 siblings, 0 replies; 26+ messages in thread
From: Bartosz Golaszewski @ 2020-11-23 11:38 UTC (permalink / raw)
To: Bartosz Golaszewski
Cc: Greg KH, Jonathan Cameron, Lars-Peter Clausen,
Peter Meerwald-Stadler, Michal Simek, linux-iio, arm-soc, LKML
On Mon, Nov 16, 2020 at 11:18 AM Bartosz Golaszewski
<bgolaszewski@baylibre.com> wrote:
>
> On Sat, Nov 14, 2020 at 5:16 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> >
> > On Sat, Nov 14, 2020 at 03:46:41PM +0000, Jonathan Cameron wrote:
> > > On Mon, 2 Nov 2020 15:22:25 +0100
> > > Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> > >
> > > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> > > >
> > > > When allocating an array of elements, users should check for
> > > > multiplication overflow or preferably use one of the provided helpers
> > > > like: devm_kmalloc_array().
> > > >
> > > > This provides devm_krealloc_array() for users who want to reallocate
> > > > managed arrays.
> > > >
> > > > Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> > >
> > > +CC Greg KH.
> > >
> > > As this is going into a very generic place I'd like a relevant ack.
> > > That file is a bit of a wild west for acks, but Greg seems most
> > > appropriate person.
> > >
> > > So Greg, any comments on this one?
> >
> > As there is only 1 user of this function in the patch series, you don't
> > save any extra code space here, I don't think this is worth it.
> >
>
> It's worth it in that the overflow check before allocation doesn't
> seem to belong in a driver IMO but is a general check that should live
> in common code.
>
> > We are seeing less and less gains from these new devm_* additions, and
> > only more confusion and problems with them. So perhaps don't add this?
> > I don't think it is needed.
> >
>
> I think you're referring to the discussion on
> devm_platform_ioremap_resource()? I would argue that consolidation of
> common operations in helpers is rarely a bad thing but it's a
> discussion for another thread.
>
> I'm not too attached to this patch - if you think this should be
> dropped then fine, but I don't see how the name devm_krealloc_array()
> can confuse anyone.
>
Greg: what's the final call on this?
Bartosz
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2 1/4] device: provide devm_krealloc_array()
@ 2020-11-23 11:38 ` Bartosz Golaszewski
0 siblings, 0 replies; 26+ messages in thread
From: Bartosz Golaszewski @ 2020-11-23 11:38 UTC (permalink / raw)
To: Bartosz Golaszewski
Cc: Lars-Peter Clausen, linux-iio, Greg KH, Michal Simek, LKML,
arm-soc, Peter Meerwald-Stadler, Jonathan Cameron
On Mon, Nov 16, 2020 at 11:18 AM Bartosz Golaszewski
<bgolaszewski@baylibre.com> wrote:
>
> On Sat, Nov 14, 2020 at 5:16 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> >
> > On Sat, Nov 14, 2020 at 03:46:41PM +0000, Jonathan Cameron wrote:
> > > On Mon, 2 Nov 2020 15:22:25 +0100
> > > Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> > >
> > > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> > > >
> > > > When allocating an array of elements, users should check for
> > > > multiplication overflow or preferably use one of the provided helpers
> > > > like: devm_kmalloc_array().
> > > >
> > > > This provides devm_krealloc_array() for users who want to reallocate
> > > > managed arrays.
> > > >
> > > > Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> > >
> > > +CC Greg KH.
> > >
> > > As this is going into a very generic place I'd like a relevant ack.
> > > That file is a bit of a wild west for acks, but Greg seems most
> > > appropriate person.
> > >
> > > So Greg, any comments on this one?
> >
> > As there is only 1 user of this function in the patch series, you don't
> > save any extra code space here, I don't think this is worth it.
> >
>
> It's worth it in that the overflow check before allocation doesn't
> seem to belong in a driver IMO but is a general check that should live
> in common code.
>
> > We are seeing less and less gains from these new devm_* additions, and
> > only more confusion and problems with them. So perhaps don't add this?
> > I don't think it is needed.
> >
>
> I think you're referring to the discussion on
> devm_platform_ioremap_resource()? I would argue that consolidation of
> common operations in helpers is rarely a bad thing but it's a
> discussion for another thread.
>
> I'm not too attached to this patch - if you think this should be
> dropped then fine, but I don't see how the name devm_krealloc_array()
> can confuse anyone.
>
Greg: what's the final call on this?
Bartosz
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2 1/4] device: provide devm_krealloc_array()
2020-11-23 11:38 ` Bartosz Golaszewski
@ 2020-11-28 16:00 ` Jonathan Cameron
-1 siblings, 0 replies; 26+ messages in thread
From: Jonathan Cameron @ 2020-11-28 16:00 UTC (permalink / raw)
To: Bartosz Golaszewski
Cc: Bartosz Golaszewski, Greg KH, Lars-Peter Clausen,
Peter Meerwald-Stadler, Michal Simek, linux-iio, arm-soc, LKML
On Mon, 23 Nov 2020 12:38:26 +0100
Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> On Mon, Nov 16, 2020 at 11:18 AM Bartosz Golaszewski
> <bgolaszewski@baylibre.com> wrote:
> >
> > On Sat, Nov 14, 2020 at 5:16 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> > >
> > > On Sat, Nov 14, 2020 at 03:46:41PM +0000, Jonathan Cameron wrote:
> > > > On Mon, 2 Nov 2020 15:22:25 +0100
> > > > Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> > > >
> > > > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> > > > >
> > > > > When allocating an array of elements, users should check for
> > > > > multiplication overflow or preferably use one of the provided helpers
> > > > > like: devm_kmalloc_array().
> > > > >
> > > > > This provides devm_krealloc_array() for users who want to reallocate
> > > > > managed arrays.
> > > > >
> > > > > Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> > > >
> > > > +CC Greg KH.
> > > >
> > > > As this is going into a very generic place I'd like a relevant ack.
> > > > That file is a bit of a wild west for acks, but Greg seems most
> > > > appropriate person.
> > > >
> > > > So Greg, any comments on this one?
> > >
> > > As there is only 1 user of this function in the patch series, you don't
> > > save any extra code space here, I don't think this is worth it.
> > >
> >
> > It's worth it in that the overflow check before allocation doesn't
> > seem to belong in a driver IMO but is a general check that should live
> > in common code.
> >
> > > We are seeing less and less gains from these new devm_* additions, and
> > > only more confusion and problems with them. So perhaps don't add this?
> > > I don't think it is needed.
> > >
> >
> > I think you're referring to the discussion on
> > devm_platform_ioremap_resource()? I would argue that consolidation of
> > common operations in helpers is rarely a bad thing but it's a
> > discussion for another thread.
> >
> > I'm not too attached to this patch - if you think this should be
> > dropped then fine, but I don't see how the name devm_krealloc_array()
> > can confuse anyone.
> >
>
> Greg: what's the final call on this?
Reroll the series without this patch. If it turns out to be a good idea
in the long run we can always bring it back, but for now it's blocking
the rest of the series.
Thanks,
Jonathan
>
> Bartosz
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2 1/4] device: provide devm_krealloc_array()
@ 2020-11-28 16:00 ` Jonathan Cameron
0 siblings, 0 replies; 26+ messages in thread
From: Jonathan Cameron @ 2020-11-28 16:00 UTC (permalink / raw)
To: Bartosz Golaszewski
Cc: Lars-Peter Clausen, linux-iio, Greg KH, Michal Simek, LKML,
Bartosz Golaszewski, Peter Meerwald-Stadler, arm-soc
On Mon, 23 Nov 2020 12:38:26 +0100
Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> On Mon, Nov 16, 2020 at 11:18 AM Bartosz Golaszewski
> <bgolaszewski@baylibre.com> wrote:
> >
> > On Sat, Nov 14, 2020 at 5:16 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> > >
> > > On Sat, Nov 14, 2020 at 03:46:41PM +0000, Jonathan Cameron wrote:
> > > > On Mon, 2 Nov 2020 15:22:25 +0100
> > > > Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> > > >
> > > > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> > > > >
> > > > > When allocating an array of elements, users should check for
> > > > > multiplication overflow or preferably use one of the provided helpers
> > > > > like: devm_kmalloc_array().
> > > > >
> > > > > This provides devm_krealloc_array() for users who want to reallocate
> > > > > managed arrays.
> > > > >
> > > > > Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> > > >
> > > > +CC Greg KH.
> > > >
> > > > As this is going into a very generic place I'd like a relevant ack.
> > > > That file is a bit of a wild west for acks, but Greg seems most
> > > > appropriate person.
> > > >
> > > > So Greg, any comments on this one?
> > >
> > > As there is only 1 user of this function in the patch series, you don't
> > > save any extra code space here, I don't think this is worth it.
> > >
> >
> > It's worth it in that the overflow check before allocation doesn't
> > seem to belong in a driver IMO but is a general check that should live
> > in common code.
> >
> > > We are seeing less and less gains from these new devm_* additions, and
> > > only more confusion and problems with them. So perhaps don't add this?
> > > I don't think it is needed.
> > >
> >
> > I think you're referring to the discussion on
> > devm_platform_ioremap_resource()? I would argue that consolidation of
> > common operations in helpers is rarely a bad thing but it's a
> > discussion for another thread.
> >
> > I'm not too attached to this patch - if you think this should be
> > dropped then fine, but I don't see how the name devm_krealloc_array()
> > can confuse anyone.
> >
>
> Greg: what's the final call on this?
Reroll the series without this patch. If it turns out to be a good idea
in the long run we can always bring it back, but for now it's blocking
the rest of the series.
Thanks,
Jonathan
>
> Bartosz
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 26+ messages in thread
end of thread, other threads:[~2020-11-28 22:05 UTC | newest]
Thread overview: 26+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-11-02 14:22 [PATCH v2 0/4] iio: adc: xilinx: use even more devres Bartosz Golaszewski
2020-11-02 14:22 ` Bartosz Golaszewski
2020-11-02 14:22 ` [PATCH v2 1/4] device: provide devm_krealloc_array() Bartosz Golaszewski
2020-11-02 14:22 ` Bartosz Golaszewski
2020-11-14 15:46 ` Jonathan Cameron
2020-11-14 15:46 ` Jonathan Cameron
2020-11-14 16:17 ` Greg KH
2020-11-14 16:17 ` Greg KH
2020-11-16 10:18 ` Bartosz Golaszewski
2020-11-16 10:18 ` Bartosz Golaszewski
2020-11-23 11:38 ` Bartosz Golaszewski
2020-11-23 11:38 ` Bartosz Golaszewski
2020-11-28 16:00 ` Jonathan Cameron
2020-11-28 16:00 ` Jonathan Cameron
2020-11-02 14:22 ` [PATCH v2 2/4] iio: adc: xilinx: use helper variable for &pdev->dev Bartosz Golaszewski
2020-11-02 14:22 ` Bartosz Golaszewski
2020-11-02 14:22 ` [PATCH v2 3/4] iio: adc: xilinx: use devm_krealloc_array() instead of kfree() + kcalloc() Bartosz Golaszewski
2020-11-02 14:22 ` Bartosz Golaszewski
2020-11-02 14:22 ` [PATCH v2 4/4] iio: adc: xilinx: use more devres helpers and remove remove() Bartosz Golaszewski
2020-11-02 14:22 ` Bartosz Golaszewski
2020-11-08 17:08 ` Jonathan Cameron
2020-11-08 17:08 ` Jonathan Cameron
2020-11-09 10:52 ` Bartosz Golaszewski
2020-11-09 10:52 ` Bartosz Golaszewski
2020-11-08 17:08 ` [PATCH v2 0/4] iio: adc: xilinx: use even more devres Jonathan Cameron
2020-11-08 17:08 ` Jonathan Cameron
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.