| Message ID | 20240223124432.26443-1-Jonathan.Cameron@huawei.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | of: automate of_node_put() - new approach to loops. | expand |
On Fri, Feb 23, 2024 at 12:44:28PM +0000, Jonathan Cameron wrote: > The equivalent device_for_each_child_node_scoped() series for > fwnode will be queued up in IIO for the merge window shortly as > it has gathered sufficient tags. Hopefully the precdent set there > for the approach will reassure people that instantiating the > child variable inside the macro definition is the best approach. > https://lore.kernel.org/linux-iio/20240217164249.921878-1-jic23@kernel.org/ > > v2: Andy suggested most of the original converted set should move to > generic fwnode / property.h handling. Within IIO that was > a reasonable observation given we've been trying to move away from > firmware specific handling for some time. Patches making that change > to appropriate drivers posted. > As we discussed there are cases which are not suitable for such > conversion and this infrastructure still provides clear benefits > for them. > iio: adc: rcar-gyroadc: use for_each_available_child_node_scoped() Is this the only one so far? Or do we have more outside of IIO? I'm fine with the code if OF maintainers think it's useful. My concern is to make as many as possible drivers to be converted to use fwnode instead of OF one.
On Fri, 23 Feb 2024 17:52:46 +0200 Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > On Fri, Feb 23, 2024 at 12:44:28PM +0000, Jonathan Cameron wrote: > > The equivalent device_for_each_child_node_scoped() series for > > fwnode will be queued up in IIO for the merge window shortly as > > it has gathered sufficient tags. Hopefully the precdent set there > > for the approach will reassure people that instantiating the > > child variable inside the macro definition is the best approach. > > https://lore.kernel.org/linux-iio/20240217164249.921878-1-jic23@kernel.org/ > > > > v2: Andy suggested most of the original converted set should move to > > generic fwnode / property.h handling. Within IIO that was > > a reasonable observation given we've been trying to move away from > > firmware specific handling for some time. Patches making that change > > to appropriate drivers posted. > > As we discussed there are cases which are not suitable for such > > conversion and this infrastructure still provides clear benefits > > for them. > > > iio: adc: rcar-gyroadc: use for_each_available_child_node_scoped() > > Is this the only one so far? Or do we have more outside of IIO? > > I'm fine with the code if OF maintainers think it's useful. > My concern is to make as many as possible drivers to be converted to > use fwnode instead of OF one. > Julia wrote a coccinelle script __free() cases https://lore.kernel.org/all/alpine.DEB.2.22.394.2401291455430.8649@hadrien/
On Fri, 23 Feb 2024, Jonathan Cameron wrote: > On Fri, 23 Feb 2024 17:52:46 +0200 > Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > > On Fri, Feb 23, 2024 at 12:44:28PM +0000, Jonathan Cameron wrote: > > > The equivalent device_for_each_child_node_scoped() series for > > > fwnode will be queued up in IIO for the merge window shortly as > > > it has gathered sufficient tags. Hopefully the precdent set there > > > for the approach will reassure people that instantiating the > > > child variable inside the macro definition is the best approach. > > > https://lore.kernel.org/linux-iio/20240217164249.921878-1-jic23@kernel.org/ > > > > > > v2: Andy suggested most of the original converted set should move to > > > generic fwnode / property.h handling. Within IIO that was > > > a reasonable observation given we've been trying to move away from > > > firmware specific handling for some time. Patches making that change > > > to appropriate drivers posted. > > > As we discussed there are cases which are not suitable for such > > > conversion and this infrastructure still provides clear benefits > > > for them. > > > > > iio: adc: rcar-gyroadc: use for_each_available_child_node_scoped() > > > > Is this the only one so far? Or do we have more outside of IIO? > > > > I'm fine with the code if OF maintainers think it's useful. > > My concern is to make as many as possible drivers to be converted to > > use fwnode instead of OF one. > > > Julia wrote a coccinelle script > __free() cases > https://lore.kernel.org/all/alpine.DEB.2.22.394.2401291455430.8649@hadrien/ The script doesn't use fwnode. It gets rid of of_node_put, asssuming that someone has already set that up for __free. julia
On Fri, 23 Feb 2024 16:36:02 +0000 Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote: > On Fri, 23 Feb 2024 17:52:46 +0200 > Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > > On Fri, Feb 23, 2024 at 12:44:28PM +0000, Jonathan Cameron wrote: > > > The equivalent device_for_each_child_node_scoped() series for > > > fwnode will be queued up in IIO for the merge window shortly as > > > it has gathered sufficient tags. Hopefully the precdent set there > > > for the approach will reassure people that instantiating the > > > child variable inside the macro definition is the best approach. > > > https://lore.kernel.org/linux-iio/20240217164249.921878-1-jic23@kernel.org/ > > > > > > v2: Andy suggested most of the original converted set should move to > > > generic fwnode / property.h handling. Within IIO that was > > > a reasonable observation given we've been trying to move away from > > > firmware specific handling for some time. Patches making that change > > > to appropriate drivers posted. > > > As we discussed there are cases which are not suitable for such > > > conversion and this infrastructure still provides clear benefits > > > for them. > > > > > iio: adc: rcar-gyroadc: use for_each_available_child_node_scoped() > > > > Is this the only one so far? Or do we have more outside of IIO? > > > > I'm fine with the code if OF maintainers think it's useful. > > My concern is to make as many as possible drivers to be converted to > > use fwnode instead of OF one. > > > Julia wrote a coccinelle script > __free() cases (53) > https://lore.kernel.org/all/alpine.DEB.2.22.394.2401291455430.8649@hadrien/ Gah. Second time today I've hit the wrong key whilst pasting. loop cases. (73) https://lore.kernel.org/all/alpine.DEB.2.22.394.2401312234250.3245@hadrien/ Scattered right across the kernel. No others submitted yet and there is just the one left in IIO after fwnode conversions. There are other drivers I haven't converted to fwnode for various reasons, but this isn't useful for them as not all call of_node_put(). Some of the other cases will be suitable for fwnode conversion and that is definitely the right first choice in many cases. Agreed, it's down to the OF maintainers to take a call on whether they want this infrastructure or not. No longer matters much to me for IIO as you can see. Jonathan
On Fri, 23 Feb 2024 17:38:31 +0100 (CET) Julia Lawall <julia.lawall@inria.fr> wrote: > On Fri, 23 Feb 2024, Jonathan Cameron wrote: > > > On Fri, 23 Feb 2024 17:52:46 +0200 > > Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > > > > On Fri, Feb 23, 2024 at 12:44:28PM +0000, Jonathan Cameron wrote: > > > > The equivalent device_for_each_child_node_scoped() series for > > > > fwnode will be queued up in IIO for the merge window shortly as > > > > it has gathered sufficient tags. Hopefully the precdent set there > > > > for the approach will reassure people that instantiating the > > > > child variable inside the macro definition is the best approach. > > > > https://lore.kernel.org/linux-iio/20240217164249.921878-1-jic23@kernel.org/ > > > > > > > > v2: Andy suggested most of the original converted set should move to > > > > generic fwnode / property.h handling. Within IIO that was > > > > a reasonable observation given we've been trying to move away from > > > > firmware specific handling for some time. Patches making that change > > > > to appropriate drivers posted. > > > > As we discussed there are cases which are not suitable for such > > > > conversion and this infrastructure still provides clear benefits > > > > for them. > > > > > > > iio: adc: rcar-gyroadc: use for_each_available_child_node_scoped() > > > > > > Is this the only one so far? Or do we have more outside of IIO? > > > > > > I'm fine with the code if OF maintainers think it's useful. > > > My concern is to make as many as possible drivers to be converted to > > > use fwnode instead of OF one. > > > > > Julia wrote a coccinelle script > > __free() cases > > https://lore.kernel.org/all/alpine.DEB.2.22.394.2401291455430.8649@hadrien/ > > The script doesn't use fwnode. It gets rid of of_node_put, asssuming that > someone has already set that up for __free. Question I was addressing was a few lines up. "Or do we have more outside of IIO?" I should have addressed it immediately after the question + not sent half an answer :( > > julia
On Fri, 23 Feb 2024 12:44:28 +0000 Jonathan Cameron <Jonathan.Cameron@huawei.com> wrote: > The equivalent device_for_each_child_node_scoped() series for > fwnode will be queued up in IIO for the merge window shortly as > it has gathered sufficient tags. Hopefully the precdent set there > for the approach will reassure people that instantiating the > child variable inside the macro definition is the best approach. > https://lore.kernel.org/linux-iio/20240217164249.921878-1-jic23@kernel.org/ I missed the devicetree list. Will resend with a brief summary of the discussion on v2 so far. Sorry for the noise! > > v2: Andy suggested most of the original converted set should move to > generic fwnode / property.h handling. Within IIO that was > a reasonable observation given we've been trying to move away from > firmware specific handling for some time. Patches making that change > to appropriate drivers posted. > As we discussed there are cases which are not suitable for such > conversion and this infrastructure still provides clear benefits > for them. > > Ideally it would be good if this introductory series adding the > infrastructure makes the 6.9 merge window. There are no dependencies > on work queued in the IIO tree, so this can go via devicetree > if the maintainers would prefer. I've had some off list messages > asking when this would be merged, as there is interest in building > on it next cycle for other parts of the kernel (where conversion to > fwnode handling may be less appropriate). > > The outputs of Julia's scripts linked below show how widely this can be > easily applied and give a conservative estimate of the complexity reduction > and code savings. In some cases those drivers should move to fwnode > and use the equivalent infrastructure there, but many will be unsuitable > for conversion so this is still good win. > > Edited cover letter from v1: > > 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/20240128160542.178315-1-jic23@kernel.org/ > > Whilst these macros produce 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. > > > > Jonathan Cameron (4): > 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: rcar-gyroadc: use for_each_available_child_node_scoped() > > drivers/iio/adc/rcar-gyroadc.c | 21 ++++++--------------- > drivers/of/unittest.c | 11 +++-------- > include/linux/of.h | 15 +++++++++++++++ > 3 files changed, 24 insertions(+), 23 deletions(-) >
The equivalent device_for_each_child_node_scoped() series for fwnode will be queued up in IIO for the merge window shortly as it has gathered sufficient tags. Hopefully the precdent set there for the approach will reassure people that instantiating the child variable inside the macro definition is the best approach. https://lore.kernel.org/linux-iio/20240217164249.921878-1-jic23@kernel.org/ v2: Andy suggested most of the original converted set should move to generic fwnode / property.h handling. Within IIO that was a reasonable observation given we've been trying to move away from firmware specific handling for some time. Patches making that change to appropriate drivers posted. As we discussed there are cases which are not suitable for such conversion and this infrastructure still provides clear benefits for them. Ideally it would be good if this introductory series adding the infrastructure makes the 6.9 merge window. There are no dependencies on work queued in the IIO tree, so this can go via devicetree if the maintainers would prefer. I've had some off list messages asking when this would be merged, as there is interest in building on it next cycle for other parts of the kernel (where conversion to fwnode handling may be less appropriate). The outputs of Julia's scripts linked below show how widely this can be easily applied and give a conservative estimate of the complexity reduction and code savings. In some cases those drivers should move to fwnode and use the equivalent infrastructure there, but many will be unsuitable for conversion so this is still good win. Edited cover letter from v1: 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/20240128160542.178315-1-jic23@kernel.org/ Whilst these macros produce 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. Jonathan Cameron (4): 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: rcar-gyroadc: use for_each_available_child_node_scoped() drivers/iio/adc/rcar-gyroadc.c | 21 ++++++--------------- drivers/of/unittest.c | 11 +++-------- include/linux/of.h | 15 +++++++++++++++ 3 files changed, 24 insertions(+), 23 deletions(-)