From: Jonathan Cameron <[email protected]>
Since RFC:
- Provide a for_each_available_child_of_node_scoped() variant and
use that whenever we aren't specifically trying to include disabled
nodes.
- Fix the for_each_child_of_node_scoped() to not use a mix of
_available_ and other calls.
- Include a few more examples. The last one is there to show that
not all uses of the __free(device_node) call are due to the loops.
Thanks to Julia Lawal who also posted coccinelle for both types (loop and
non loop cases)
https://lore.kernel.org/all/alpine.DEB.2.22.394.2401312234250.3245@hadrien/
https://lore.kernel.org/all/alpine.DEB.2.22.394.2401291455430.8649@hadrien/
The cover letter of the RFC includes information on the various approaches
considered.
https://lore.kernel.org/all/[email protected]/
Whilst these macros profduce nice reductions in complexity the loops
still have the unfortunate side effect of hiding the local declaration
of a struct device_node * which is then used inside the loop.
Julia suggested making that a little more visible via
#define for_each_child_of_node_scoped(parent, struct device_node *, child)
but in discussion we both expressed that this doesn't really make things
all that clear either so I haven't adopted this suggestion.
If the responses to this series are positive I can put the first few
patches on an immutable branch, allowing rapid adoption in other trees
if people want to move quickly. If not we can wait for next cycle and
just take this infrastructure through the IIO tree ready for the 6.9
merge cycle.
I'll be optimistic that we are converging and send out an equivalent
series for fwnode_handle / property.h to replace the previous proposal:
https://lore.kernel.org/all/[email protected]/
Jonathan Cameron (8):
of: Add cleanup.h based auto release via __free(device_node) markings.
of: Introduce for_each_*_child_of_node_scoped() to automate
of_node_put() handling
of: unittest: Use for_each_child_of_node_scoped()
iio: adc: fsl-imx25-gcq: Use for_each_available_child_node_scoped()
iio: adc: rcar-gyroadc: use for_each_available_child_node_scoped()
iio: adc: ad7124: Use for_each_available_child_of_node_scoped()
iio: adc: ad7292: Use for_each_available_child_of_node_scoped()
iio: adc: adi-axi-adc: Use __free(device_node) and guard(mutex)
drivers/iio/adc/ad7124.c | 20 ++++++--------------
drivers/iio/adc/ad7292.c | 7 ++-----
drivers/iio/adc/adi-axi-adc.c | 16 ++++------------
drivers/iio/adc/fsl-imx25-gcq.c | 13 +++----------
drivers/iio/adc/rcar-gyroadc.c | 21 ++++++---------------
drivers/of/unittest.c | 11 +++--------
include/linux/of.h | 15 +++++++++++++++
7 files changed, 39 insertions(+), 64 deletions(-)
--
2.43.1
From: Jonathan Cameron <[email protected]>
The recent addition of scope based cleanup support to the kernel
provides a convenient tool to reduce the chances of leaking reference
counts where of_node_put() should have been called in an error path.
This enables
struct device_node *child __free(device_node) = NULL;
for_each_child_of_node(np, child) {
if (test)
return test;
}
with no need for a manual call of of_node_put().
A following patch will reduce the scope of the child variable to the
for loop, to avoid an issues with ordering of autocleanup, and make it
obvious when this assigned a non NULL value.
In this simple example the gains are small but there are some very
complex error handling cases buried in these loops that will be
greatly simplified by enabling early returns with out the need
for this manual of_node_put() call.
Note that there are coccinelle checks in
scripts/coccinelle/iterators/for_each_child.cocci to detect a failure
to call of_node_put(). This new approach does not cause false positives.
Longer term we may want to add scripting to check this new approach is
done correctly with no double of_node_put() calls being introduced due
to the auto cleanup. It may also be useful to script finding places
this new approach is useful.
Signed-off-by: Jonathan Cameron <[email protected]>
---
include/linux/of.h | 2 ++
1 file changed, 2 insertions(+)
diff --git a/include/linux/of.h b/include/linux/of.h
index 6a9ddf20e79a..50e882ee91da 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -13,6 +13,7 @@
*/
#include <linux/types.h>
#include <linux/bitops.h>
+#include <linux/cleanup.h>
#include <linux/errno.h>
#include <linux/kobject.h>
#include <linux/mod_devicetable.h>
@@ -134,6 +135,7 @@ static inline struct device_node *of_node_get(struct device_node *node)
}
static inline void of_node_put(struct device_node *node) { }
#endif /* !CONFIG_OF_DYNAMIC */
+DEFINE_FREE(device_node, struct device_node *, if (_T) of_node_put(_T))
/* Pointer for first entry in chain of all nodes. */
extern struct device_node *of_root;
--
2.43.1
From: Jonathan Cameron <[email protected]>
To avoid issues with out of order cleanup, or ambiguity about when the
auto freed data is first instantiated, do it within the for loop definition.
The disadvantage is that the struct device_node *child variable creation
is not immediately obvious where this is used.
However, in many cases, if there is another definition of
struct device_node *child; the compiler / static analysers will notify us
that it is unused, or uninitialized.
Note that, in the vast majority of cases, the _available_ form should be
used and as code is converted to these scoped handers, we should confirm
that any cases that do not check for available have a good reason not
to.
Signed-off-by: Jonathan Cameron <[email protected]>
---
include/linux/of.h | 13 +++++++++++++
1 file changed, 13 insertions(+)
diff --git a/include/linux/of.h b/include/linux/of.h
index 50e882ee91da..024dda54b9c7 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -1430,10 +1430,23 @@ static inline int of_property_read_s32(const struct device_node *np,
#define for_each_child_of_node(parent, child) \
for (child = of_get_next_child(parent, NULL); child != NULL; \
child = of_get_next_child(parent, child))
+
+#define for_each_child_of_node_scoped(parent, child) \
+ for (struct device_node *child __free(device_node) = \
+ of_get_next_child(parent, NULL); \
+ child != NULL; \
+ child = of_get_next_child(parent, child))
+
#define for_each_available_child_of_node(parent, child) \
for (child = of_get_next_available_child(parent, NULL); child != NULL; \
child = of_get_next_available_child(parent, child))
+#define for_each_available_child_of_node_scoped(parent, child) \
+ for (struct device_node *child __free(device_node) = \
+ of_get_next_available_child(parent, NULL); \
+ child != NULL; \
+ child = of_get_next_available_child(parent, child))
+
#define for_each_of_cpu_node(cpu) \
for (cpu = of_get_next_cpu_node(NULL); cpu != NULL; \
cpu = of_get_next_cpu_node(cpu))
--
2.43.1
From: Jonathan Cameron <[email protected]>
A simple example of the utility of this autocleanup approach to
handling of_node_put().
In this particular case some of the nodes needed for the test are
not available and the _available_ version would cause them to be
skipped resulting in a test failure.
Signed-off-by: Jonathan Cameron <[email protected]>
---
drivers/of/unittest.c | 11 +++--------
1 file changed, 3 insertions(+), 8 deletions(-)
diff --git a/drivers/of/unittest.c b/drivers/of/unittest.c
index cfd60e35a899..d353327767b3 100644
--- a/drivers/of/unittest.c
+++ b/drivers/of/unittest.c
@@ -233,27 +233,22 @@ static void __init of_unittest_dynamic(void)
static int __init of_unittest_check_node_linkage(struct device_node *np)
{
- struct device_node *child;
int count = 0, rc;
- for_each_child_of_node(np, child) {
+ for_each_child_of_node_scoped(np, child) {
if (child->parent != np) {
pr_err("Child node %pOFn links to wrong parent %pOFn\n",
child, np);
- rc = -EINVAL;
- goto put_child;
+ return -EINVAL;
}
rc = of_unittest_check_node_linkage(child);
if (rc < 0)
- goto put_child;
+ return rc;
count += rc;
}
return count + 1;
-put_child:
- of_node_put(child);
- return rc;
}
static void __init of_unittest_check_tree_linkage(void)
--
2.43.1
From: Jonathan Cameron <[email protected]>
Using automated cleanup reduces chance of an reference count leak
and simplfies the code.
Child nodes should only ever have been considered if they were available
(either status is okay, or it is not defined).
Signed-off-by: Jonathan Cameron <[email protected]>
---
drivers/iio/adc/fsl-imx25-gcq.c | 13 +++----------
1 file changed, 3 insertions(+), 10 deletions(-)
diff --git a/drivers/iio/adc/fsl-imx25-gcq.c b/drivers/iio/adc/fsl-imx25-gcq.c
index 68c813de0605..1b0003230306 100644
--- a/drivers/iio/adc/fsl-imx25-gcq.c
+++ b/drivers/iio/adc/fsl-imx25-gcq.c
@@ -199,7 +199,6 @@ static int mx25_gcq_setup_cfgs(struct platform_device *pdev,
struct mx25_gcq_priv *priv)
{
struct device_node *np = pdev->dev.of_node;
- struct device_node *child;
struct device *dev = &pdev->dev;
int ret, i;
@@ -216,7 +215,7 @@ static int mx25_gcq_setup_cfgs(struct platform_device *pdev,
MX25_ADCQ_CFG_IN(i) |
MX25_ADCQ_CFG_REFN_NGND2);
- for_each_child_of_node(np, child) {
+ for_each_available_child_of_node_scoped(np, child) {
u32 reg;
u32 refp = MX25_ADCQ_CFG_REFP_INT;
u32 refn = MX25_ADCQ_CFG_REFN_NGND2;
@@ -224,14 +223,12 @@ static int mx25_gcq_setup_cfgs(struct platform_device *pdev,
ret = of_property_read_u32(child, "reg", ®);
if (ret) {
dev_err(dev, "Failed to get reg property\n");
- of_node_put(child);
return ret;
}
if (reg >= MX25_NUM_CFGS) {
dev_err(dev,
"reg value is greater than the number of available configuration registers\n");
- of_node_put(child);
return -EINVAL;
}
@@ -243,10 +240,9 @@ static int mx25_gcq_setup_cfgs(struct platform_device *pdev,
case MX25_ADC_REFP_XP:
case MX25_ADC_REFP_YP:
ret = mx25_gcq_ext_regulator_setup(&pdev->dev, priv, refp);
- if (ret) {
- of_node_put(child);
+ if (ret)
return ret;
- }
+
priv->channel_vref_mv[reg] =
regulator_get_voltage(priv->vref[refp]);
/* Conversion from uV to mV */
@@ -257,7 +253,6 @@ static int mx25_gcq_setup_cfgs(struct platform_device *pdev,
break;
default:
dev_err(dev, "Invalid positive reference %d\n", refp);
- of_node_put(child);
return -EINVAL;
}
@@ -270,12 +265,10 @@ static int mx25_gcq_setup_cfgs(struct platform_device *pdev,
if ((refp & MX25_ADCQ_CFG_REFP_MASK) != refp) {
dev_err(dev, "Invalid fsl,adc-refp property value\n");
- of_node_put(child);
return -EINVAL;
}
if ((refn & MX25_ADCQ_CFG_REFN_MASK) != refn) {
dev_err(dev, "Invalid fsl,adc-refn property value\n");
- of_node_put(child);
return -EINVAL;
}
--
2.43.1
From: Jonathan Cameron <[email protected]>
Using automated cleanup to replace of_node_put() handling allows for
a simplfied flow by enabling direct returns on errors.
Non available child nodes should never have been considered; that
is ones where status != okay and was defined.
Signed-off-by: Jonathan Cameron <[email protected]>
---
drivers/iio/adc/rcar-gyroadc.c | 21 ++++++---------------
1 file changed, 6 insertions(+), 15 deletions(-)
diff --git a/drivers/iio/adc/rcar-gyroadc.c b/drivers/iio/adc/rcar-gyroadc.c
index d524f2e8e927..15a21d2860e7 100644
--- a/drivers/iio/adc/rcar-gyroadc.c
+++ b/drivers/iio/adc/rcar-gyroadc.c
@@ -318,7 +318,6 @@ static int rcar_gyroadc_parse_subdevs(struct iio_dev *indio_dev)
struct rcar_gyroadc *priv = iio_priv(indio_dev);
struct device *dev = priv->dev;
struct device_node *np = dev->of_node;
- struct device_node *child;
struct regulator *vref;
unsigned int reg;
unsigned int adcmode = -1, childmode;
@@ -326,7 +325,7 @@ static int rcar_gyroadc_parse_subdevs(struct iio_dev *indio_dev)
unsigned int num_channels;
int ret, first = 1;
- for_each_child_of_node(np, child) {
+ for_each_available_child_of_node_scoped(np, child) {
of_id = of_match_node(rcar_gyroadc_child_match, child);
if (!of_id) {
dev_err(dev, "Ignoring unsupported ADC \"%pOFn\".",
@@ -352,7 +351,7 @@ static int rcar_gyroadc_parse_subdevs(struct iio_dev *indio_dev)
num_channels = ARRAY_SIZE(rcar_gyroadc_iio_channels_3);
break;
default:
- goto err_e_inval;
+ return -EINVAL;
}
/*
@@ -369,7 +368,7 @@ static int rcar_gyroadc_parse_subdevs(struct iio_dev *indio_dev)
dev_err(dev,
"Failed to get child reg property of ADC \"%pOFn\".\n",
child);
- goto err_of_node_put;
+ return ret;
}
/* Channel number is too high. */
@@ -377,7 +376,7 @@ static int rcar_gyroadc_parse_subdevs(struct iio_dev *indio_dev)
dev_err(dev,
"Only %i channels supported with %pOFn, but reg = <%i>.\n",
num_channels, child, reg);
- goto err_e_inval;
+ return -EINVAL;
}
}
@@ -386,7 +385,7 @@ static int rcar_gyroadc_parse_subdevs(struct iio_dev *indio_dev)
dev_err(dev,
"Channel %i uses different ADC mode than the rest.\n",
reg);
- goto err_e_inval;
+ return -EINVAL;
}
/* Channel is valid, grab the regulator. */
@@ -396,8 +395,7 @@ static int rcar_gyroadc_parse_subdevs(struct iio_dev *indio_dev)
if (IS_ERR(vref)) {
dev_dbg(dev, "Channel %i 'vref' supply not connected.\n",
reg);
- ret = PTR_ERR(vref);
- goto err_of_node_put;
+ return PTR_ERR(vref);
}
priv->vref[reg] = vref;
@@ -422,7 +420,6 @@ static int rcar_gyroadc_parse_subdevs(struct iio_dev *indio_dev)
* we can stop parsing here.
*/
if (childmode == RCAR_GYROADC_MODE_SELECT_1_MB88101A) {
- of_node_put(child);
break;
}
}
@@ -433,12 +430,6 @@ static int rcar_gyroadc_parse_subdevs(struct iio_dev *indio_dev)
}
return 0;
-
-err_e_inval:
- ret = -EINVAL;
-err_of_node_put:
- of_node_put(child);
- return ret;
}
static void rcar_gyroadc_deinit_supplies(struct iio_dev *indio_dev)
--
2.43.1
From: Jonathan Cameron <[email protected]>
Avoids the need for manual cleanup of_node_put() in early exits from
the loop.
Signed-off-by: Jonathan Cameron <[email protected]>
---
drivers/iio/adc/ad7292.c | 7 ++-----
1 file changed, 2 insertions(+), 5 deletions(-)
diff --git a/drivers/iio/adc/ad7292.c b/drivers/iio/adc/ad7292.c
index cccacec5db6d..a9354bd1ce54 100644
--- a/drivers/iio/adc/ad7292.c
+++ b/drivers/iio/adc/ad7292.c
@@ -260,7 +260,6 @@ static int ad7292_probe(struct spi_device *spi)
{
struct ad7292_state *st;
struct iio_dev *indio_dev;
- struct device_node *child;
bool diff_channels = false;
int ret;
@@ -305,12 +304,10 @@ static int ad7292_probe(struct spi_device *spi)
indio_dev->modes = INDIO_DIRECT_MODE;
indio_dev->info = &ad7292_info;
- for_each_available_child_of_node(spi->dev.of_node, child) {
+ for_each_available_child_of_node_scoped(spi->dev.of_node, child) {
diff_channels = of_property_read_bool(child, "diff-channels");
- if (diff_channels) {
- of_node_put(child);
+ if (diff_channels)
break;
- }
}
if (diff_channels) {
--
2.43.1
From: Jonathan Cameron <[email protected]>
Avoid need to manually handle of_node_put() or the unlocking of the
mutex.
Signed-off-by: Jonathan Cameron <[email protected]>
---
drivers/iio/adc/adi-axi-adc.c | 16 ++++------------
1 file changed, 4 insertions(+), 12 deletions(-)
diff --git a/drivers/iio/adc/adi-axi-adc.c b/drivers/iio/adc/adi-axi-adc.c
index c247ff1541d2..3c85e8a6467b 100644
--- a/drivers/iio/adc/adi-axi-adc.c
+++ b/drivers/iio/adc/adi-axi-adc.c
@@ -248,19 +248,19 @@ static struct adi_axi_adc_client *adi_axi_adc_attach_client(struct device *dev)
{
const struct adi_axi_adc_core_info *info;
struct adi_axi_adc_client *cl;
- struct device_node *cln;
info = of_device_get_match_data(dev);
if (!info)
return ERR_PTR(-ENODEV);
- cln = of_parse_phandle(dev->of_node, "adi,adc-dev", 0);
+ struct device_node *cln __free(device_node) =
+ of_parse_phandle(dev->of_node, "adi,adc-dev", 0);
if (!cln) {
dev_err(dev, "No 'adi,adc-dev' node defined\n");
return ERR_PTR(-ENODEV);
}
- mutex_lock(®istered_clients_lock);
+ guard(mutex)(®istered_clients_lock);
list_for_each_entry(cl, ®istered_clients, entry) {
if (!cl->dev)
@@ -269,22 +269,14 @@ static struct adi_axi_adc_client *adi_axi_adc_attach_client(struct device *dev)
if (cl->dev->of_node != cln)
continue;
- if (!try_module_get(cl->dev->driver->owner)) {
- mutex_unlock(®istered_clients_lock);
- of_node_put(cln);
+ if (!try_module_get(cl->dev->driver->owner))
return ERR_PTR(-ENODEV);
- }
get_device(cl->dev);
cl->info = info;
- mutex_unlock(®istered_clients_lock);
- of_node_put(cln);
return cl;
}
- mutex_unlock(®istered_clients_lock);
- of_node_put(cln);
-
return ERR_PTR(-EPROBE_DEFER);
}
--
2.43.1
From: Jonathan Cameron <[email protected]>
Avoids the need for manual cleanup of_node_put() in early exits
from the loop.
Signed-off-by: Jonathan Cameron <[email protected]>
---
drivers/iio/adc/ad7124.c | 20 ++++++--------------
1 file changed, 6 insertions(+), 14 deletions(-)
diff --git a/drivers/iio/adc/ad7124.c b/drivers/iio/adc/ad7124.c
index b9b206fcd748..67ccdad752c5 100644
--- a/drivers/iio/adc/ad7124.c
+++ b/drivers/iio/adc/ad7124.c
@@ -813,7 +813,6 @@ static int ad7124_of_parse_channel_config(struct iio_dev *indio_dev,
struct ad7124_state *st = iio_priv(indio_dev);
struct ad7124_channel_config *cfg;
struct ad7124_channel *channels;
- struct device_node *child;
struct iio_chan_spec *chan;
unsigned int ain[2], channel = 0, tmp;
int ret;
@@ -838,24 +837,21 @@ static int ad7124_of_parse_channel_config(struct iio_dev *indio_dev,
indio_dev->num_channels = st->num_channels;
st->channels = channels;
- for_each_available_child_of_node(np, child) {
+ for_each_available_child_of_node_scoped(np, child) {
cfg = &st->channels[channel].cfg;
ret = of_property_read_u32(child, "reg", &channel);
if (ret)
- goto err;
+ return ret;
- if (channel >= indio_dev->num_channels) {
- dev_err(indio_dev->dev.parent,
- "Channel index >= number of channels\n");
- ret = -EINVAL;
- goto err;
- }
+ if (channel >= indio_dev->num_channels)
+ return dev_err_probe(indio_dev->dev.parent, -EINVAL,
+ "Channel index >= number of channels\n");
ret = of_property_read_u32_array(child, "diff-channels",
ain, 2);
if (ret)
- goto err;
+ return ret;
st->channels[channel].nr = channel;
st->channels[channel].ain = AD7124_CHANNEL_AINP(ain[0]) |
@@ -880,10 +876,6 @@ static int ad7124_of_parse_channel_config(struct iio_dev *indio_dev,
}
return 0;
-err:
- of_node_put(child);
-
- return ret;
}
static int ad7124_setup(struct ad7124_state *st)
--
2.43.1
On Sun, 11 Feb 2024, Jonathan Cameron wrote:
> From: Jonathan Cameron <[email protected]>
>
> To avoid issues with out of order cleanup, or ambiguity about when the
> auto freed data is first instantiated, do it within the for loop definition.
>
> The disadvantage is that the struct device_node *child variable creation
> is not immediately obvious where this is used.
> However, in many cases, if there is another definition of
> struct device_node *child; the compiler / static analysers will notify us
> that it is unused, or uninitialized.
>
> Note that, in the vast majority of cases, the _available_ form should be
> used and as code is converted to these scoped handers, we should confirm
> that any cases that do not check for available have a good reason not
> to.
Is it a good idea to make the two changes at once? Maybe it would slow
down the use of the scoped form, which can really simplify the code.
julia
>
> Signed-off-by: Jonathan Cameron <[email protected]>
> ---
> include/linux/of.h | 13 +++++++++++++
> 1 file changed, 13 insertions(+)
>
> diff --git a/include/linux/of.h b/include/linux/of.h
> index 50e882ee91da..024dda54b9c7 100644
> --- a/include/linux/of.h
> +++ b/include/linux/of.h
> @@ -1430,10 +1430,23 @@ static inline int of_property_read_s32(const struct device_node *np,
> #define for_each_child_of_node(parent, child) \
> for (child = of_get_next_child(parent, NULL); child != NULL; \
> child = of_get_next_child(parent, child))
> +
> +#define for_each_child_of_node_scoped(parent, child) \
> + for (struct device_node *child __free(device_node) = \
> + of_get_next_child(parent, NULL); \
> + child != NULL; \
> + child = of_get_next_child(parent, child))
> +
> #define for_each_available_child_of_node(parent, child) \
> for (child = of_get_next_available_child(parent, NULL); child != NULL; \
> child = of_get_next_available_child(parent, child))
>
> +#define for_each_available_child_of_node_scoped(parent, child) \
> + for (struct device_node *child __free(device_node) = \
> + of_get_next_available_child(parent, NULL); \
> + child != NULL; \
> + child = of_get_next_available_child(parent, child))
> +
> #define for_each_of_cpu_node(cpu) \
> for (cpu = of_get_next_cpu_node(NULL); cpu != NULL; \
> cpu = of_get_next_cpu_node(cpu))
> --
> 2.43.1
>
>
On Sun, Feb 11, 2024 at 05:42:28PM +0000, Jonathan Cameron wrote:
> From: Jonathan Cameron <[email protected]>
>
> Since RFC:
> - Provide a for_each_available_child_of_node_scoped() variant and
> use that whenever we aren't specifically trying to include disabled
> nodes.
> - Fix the for_each_child_of_node_scoped() to not use a mix of
> _available_ and other calls.
> - Include a few more examples. The last one is there to show that
> not all uses of the __free(device_node) call are due to the loops.
I'm a bit skeptical about need of this work. What I would prefer to see
is getting rid of OF-centric drivers in IIO. With that, we would need
only fwnode part to be properly implemented.
--
With Best Regards,
Andy Shevchenko
On Mon, 12 Feb 2024 14:03:29 +0200
Andy Shevchenko <[email protected]> wrote:
> On Sun, Feb 11, 2024 at 05:42:28PM +0000, Jonathan Cameron wrote:
> > From: Jonathan Cameron <[email protected]>
> >
> > Since RFC:
> > - Provide a for_each_available_child_of_node_scoped() variant and
> > use that whenever we aren't specifically trying to include disabled
> > nodes.
> > - Fix the for_each_child_of_node_scoped() to not use a mix of
> > _available_ and other calls.
> > - Include a few more examples. The last one is there to show that
> > not all uses of the __free(device_node) call are due to the loops.
>
> I'm a bit skeptical about need of this work. What I would prefer to see
> is getting rid of OF-centric drivers in IIO. With that, we would need
> only fwnode part to be properly implemented.
>
To be honest main reason for doing of first was that they have unit tests :)
The IIO drivers were more of a proving ground than cases I really cared
out cleaning up. However I'm always of the view that better to make
some improvement now than wait for a perfect improvement later.
However one or two are not going to be converted to fwnode handling
any time soon because they make use of phandle based referencing for
driver specific hook ups that isn't going to get generic handling any
time soon.
I'll probably focus on getting the fwnode version of this moving
forwards first though and 'maybe' convert a few of the easier ones
of these over to that framework to reduce how many users of this
we end up with in IIO.
Jonathan
On Fri, Feb 16, 2024 at 02:47:56PM +0000, Jonathan Cameron wrote:
> On Mon, 12 Feb 2024 14:03:29 +0200
> Andy Shevchenko <[email protected]> wrote:
> > On Sun, Feb 11, 2024 at 05:42:28PM +0000, Jonathan Cameron wrote:
..
> > I'm a bit skeptical about need of this work. What I would prefer to see
> > is getting rid of OF-centric drivers in IIO. With that, we would need
> > only fwnode part to be properly implemented.
>
> To be honest main reason for doing of first was that they have unit tests :)
fwnode also has KUnit test. Have you considered adding test cases there?
> The IIO drivers were more of a proving ground than cases I really cared
> out cleaning up. However I'm always of the view that better to make
> some improvement now than wait for a perfect improvement later.
Yes, but in my opinion _in this particular case_ it brings more churn and
some maybe even not good from educational purposes, i.e. one can look at
the current series and think "oh, OF is still in use, let me provide my
driver OF-only (for whatever reasons behind)", while targeting conversion
first will tell people: "hey, there is an agnostic device property framework
that should be used in a new code and that's why we have been converting old
drivers too".
> However one or two are not going to be converted to fwnode handling
> any time soon because they make use of phandle based referencing for
> driver specific hook ups that isn't going to get generic handling any
> time soon.
Sure, exceptions happen.
> I'll probably focus on getting the fwnode version of this moving
> forwards first though and 'maybe' convert a few of the easier ones
> of these over to that framework to reduce how many users of this
> we end up with in IIO.
Thanks!
--
With Best Regards,
Andy Shevchenko
On Fri, 16 Feb 2024 17:25:45 +0200
Andy Shevchenko <[email protected]> wrote:
> On Fri, Feb 16, 2024 at 02:47:56PM +0000, Jonathan Cameron wrote:
> > On Mon, 12 Feb 2024 14:03:29 +0200
> > Andy Shevchenko <[email protected]> wrote:
> > > On Sun, Feb 11, 2024 at 05:42:28PM +0000, Jonathan Cameron wrote:
>
> ...
>
> > > I'm a bit skeptical about need of this work. What I would prefer to see
> > > is getting rid of OF-centric drivers in IIO. With that, we would need
> > > only fwnode part to be properly implemented.
> >
> > To be honest main reason for doing of first was that they have unit tests :)
>
> fwnode also has KUnit test. Have you considered adding test cases there?
>
> > The IIO drivers were more of a proving ground than cases I really cared
> > out cleaning up. However I'm always of the view that better to make
> > some improvement now than wait for a perfect improvement later.
>
> Yes, but in my opinion _in this particular case_ it brings more churn and
> some maybe even not good from educational purposes, i.e. one can look at
> the current series and think "oh, OF is still in use, let me provide my
> driver OF-only (for whatever reasons behind)", while targeting conversion
> first will tell people: "hey, there is an agnostic device property framework
> that should be used in a new code and that's why we have been converting old
> drivers too".
>
> > However one or two are not going to be converted to fwnode handling
> > any time soon because they make use of phandle based referencing for
> > driver specific hook ups that isn't going to get generic handling any
> > time soon.
>
> Sure, exceptions happen.
After the series converting over most of the cases this patch set touched
in IIO, I have
rcar-gyroadc and the unit test left, which are enough to show the purpose
of the patch and put a few real users in place.
Will submit a v2 with just those 2 users. Ideal would be to get these in
for the merge window so it is available for other subsystems next cycle.
>
> > I'll probably focus on getting the fwnode version of this moving
> > forwards first though and 'maybe' convert a few of the easier ones
> > of these over to that framework to reduce how many users of this
> > we end up with in IIO.
>
> Thanks!
>