Message ID | 20210725172458.487343-3-jic23@kernel.org (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | iio:adc:ad7124: Convert to generic firmware handling | expand |
On Sun, Jul 25, 2021 at 8:22 PM Jonathan Cameron <jic23@kernel.org> wrote: > > From: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > Also use device_get_match_data() rather than of specific variant. > These changes enable use of this binding on ACPI platforms via PRP0001. > Whilst it's possible no one will ever do so, this is part of a general > effort to clear out examples from IIO that might be copied into new > drivers. > > It may appear that this change drops the check for status = disabled, > but in reality it does not because the of property code uses > of_get_next_available_child(). This driver may well fail to probe > if disabled is ever actually set though due to the need for > complete concurrent child nodes. A future series might resolve > that restriction. Perhaps we need to have ... > + device_for_each_child_node(dev, child) > + st->num_channels++; > + device_get_child_node_count() ? ... > - for_each_available_child_of_node(np, child) { > + device_for_each_child_node(dev, child) { Isn't this fwnode_for_each_available_child_node() better to use? ... So the gaps I see are device_get_available_child_node_count() and device_for_each_available_child_node() Both of them I think are easy to add and avoid possible breakage.
On Sun, 25 Jul 2021 23:33:12 +0300 Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Sun, Jul 25, 2021 at 8:22 PM Jonathan Cameron <jic23@kernel.org> wrote: > > > > From: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > > > Also use device_get_match_data() rather than of specific variant. > > These changes enable use of this binding on ACPI platforms via PRP0001. > > Whilst it's possible no one will ever do so, this is part of a general > > effort to clear out examples from IIO that might be copied into new > > drivers. > > > > It may appear that this change drops the check for status = disabled, > > but in reality it does not because the of property code uses > > of_get_next_available_child(). This driver may well fail to probe > > if disabled is ever actually set though due to the need for > > complete concurrent child nodes. A future series might resolve > > that restriction. > > Perhaps we need to have > > ... > > > + device_for_each_child_node(dev, child) > > + st->num_channels++; > > + > > device_get_child_node_count() ? > Gah. Not sure how I missed that one when looking for it... > ... > > > - for_each_available_child_of_node(np, child) { > > + device_for_each_child_node(dev, child) { > > Isn't this > fwnode_for_each_available_child_node() > better to use? Given we would be extracting the fwnode just to call this loop, I'd say no, device version makes more sense.. > > ... > > So the gaps I see are > device_get_available_child_node_count() > and > device_for_each_available_child_node() Do we then fix the fact that device_for_each_child_node() will call the _available() form for device tree? That seems inconsistent currently and I was assuming that was deliberate... Jonathan > > Both of them I think are easy to add and avoid possible breakage. >
On Tue, Jul 27, 2021 at 4:52 PM Jonathan Cameron <Jonathan.Cameron@huawei.com> wrote: > On Sun, 25 Jul 2021 23:33:12 +0300 > Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > On Sun, Jul 25, 2021 at 8:22 PM Jonathan Cameron <jic23@kernel.org> wrote: ... > > > - for_each_available_child_of_node(np, child) { > > > + device_for_each_child_node(dev, child) { > > > > Isn't this > > fwnode_for_each_available_child_node() > > better to use? > > Given we would be extracting the fwnode just to call this > loop, I'd say no, device version makes more sense.. > > > > > ... > > > > So the gaps I see are > > device_get_available_child_node_count() > > and > > device_for_each_available_child_node() > > Do we then fix the fact that > device_for_each_child_node() will call the _available() form > for device tree? That seems inconsistent currently and > I was assuming that was deliberate... I'm not sure I got your point. Mine (see below) is to add the APIs that you want to use as a direct replacement of the corresponding OF counterparts. > > Both of them I think are easy to add and avoid possible breakage.
On Tue, 27 Jul 2021 17:16:07 +0300 Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Tue, Jul 27, 2021 at 4:52 PM Jonathan Cameron > <Jonathan.Cameron@huawei.com> wrote: > > On Sun, 25 Jul 2021 23:33:12 +0300 > > Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > > On Sun, Jul 25, 2021 at 8:22 PM Jonathan Cameron <jic23@kernel.org> wrote: > > ... > > > > > - for_each_available_child_of_node(np, child) { > > > > + device_for_each_child_node(dev, child) { > > > > > > Isn't this > > > fwnode_for_each_available_child_node() > > > better to use? > > > > Given we would be extracting the fwnode just to call this > > loop, I'd say no, device version makes more sense.. > > > > > > > > ... > > > > > > So the gaps I see are > > > device_get_available_child_node_count() > > > and > > > device_for_each_available_child_node() > > > > Do we then fix the fact that > > device_for_each_child_node() will call the _available() form > > for device tree? That seems inconsistent currently and > > I was assuming that was deliberate... > > I'm not sure I got your point. Mine (see below) is to add the APIs > that you want to use as a direct replacement of the corresponding OF > counterparts. +CC Rafael, The oddity is that device_for_each_child_node() is a direct replacement of the for_each_available_child_of_node() other than the obvious use of device rather than the of node. https://elixir.bootlin.com/linux/v5.14-rc3/source/drivers/of/property.c#L939 static struct fwnode_handle * of_fwnode_get_next_child_node(const struct fwnode_handle *fwnode, struct fwnode_handle *child) { return of_fwnode_handle(of_get_next_available_child(to_of_node(fwnode), to_of_node(child))); } So the question becomes whether there is any desire at all to have a version of the device_for_each_child_node() that does not check if it is available or not. Looks like it goes all the way back. Rafael, any comment on why the available for is used here and whether it makes sense to introduce separate versions for looping over children that cover the _available_ and everything cases? https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/base/property.c?id=8a0662d9ed2968e1186208336a8e1fab3fdfea63 I'm kind of assuming this was deliberate as we don't want to encourage accessing disabled firmware nodes. Jonathan > > > > Both of them I think are easy to add and avoid possible breakage. > >
On Tue, 27 Jul 2021 19:20:13 +0100 Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote: > On Tue, 27 Jul 2021 17:16:07 +0300 > Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > > On Tue, Jul 27, 2021 at 4:52 PM Jonathan Cameron > > <Jonathan.Cameron@huawei.com> wrote: > > > On Sun, 25 Jul 2021 23:33:12 +0300 > > > Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > > > On Sun, Jul 25, 2021 at 8:22 PM Jonathan Cameron <jic23@kernel.org> wrote: > > > > ... > > > > > > > - for_each_available_child_of_node(np, child) { > > > > > + device_for_each_child_node(dev, child) { > > > > > > > > Isn't this > > > > fwnode_for_each_available_child_node() > > > > better to use? > > > > > > Given we would be extracting the fwnode just to call this > > > loop, I'd say no, device version makes more sense.. > > > > > > > > > > > ... > > > > > > > > So the gaps I see are > > > > device_get_available_child_node_count() > > > > and > > > > device_for_each_available_child_node() > > > > > > Do we then fix the fact that > > > device_for_each_child_node() will call the _available() form > > > for device tree? That seems inconsistent currently and > > > I was assuming that was deliberate... > > > > I'm not sure I got your point. Mine (see below) is to add the APIs > > that you want to use as a direct replacement of the corresponding OF > > counterparts. > +CC Rafael, Rafael, if you have a chance to give input on the questions below it would be much appreciated. Thanks, Jonathan > > The oddity is that device_for_each_child_node() is a direct replacement > of the for_each_available_child_of_node() other than the obvious > use of device rather than the of node. > > https://elixir.bootlin.com/linux/v5.14-rc3/source/drivers/of/property.c#L939 > > static struct fwnode_handle * > of_fwnode_get_next_child_node(const struct fwnode_handle *fwnode, > struct fwnode_handle *child) > { > return of_fwnode_handle(of_get_next_available_child(to_of_node(fwnode), > to_of_node(child))); > } > > So the question becomes whether there is any desire at all to have a > version of the device_for_each_child_node() that does not check > if it is available or not. > > Looks like it goes all the way back. Rafael, any comment on why the available > for is used here and whether it makes sense to introduce separate > versions for looping over children that cover the _available_ and everything > cases? > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/base/property.c?id=8a0662d9ed2968e1186208336a8e1fab3fdfea63 > > I'm kind of assuming this was deliberate as we don't want to encourage > accessing disabled firmware nodes. > > Jonathan > > > > > > > Both of them I think are easy to add and avoid possible breakage. > > > > >
On Sun, 15 Aug 2021 17:09:51 +0100 Jonathan Cameron <jic23@kernel.org> wrote: > On Tue, 27 Jul 2021 19:20:13 +0100 > Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote: > > > On Tue, 27 Jul 2021 17:16:07 +0300 > > Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > > > > On Tue, Jul 27, 2021 at 4:52 PM Jonathan Cameron > > > <Jonathan.Cameron@huawei.com> wrote: > > > > On Sun, 25 Jul 2021 23:33:12 +0300 > > > > Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > > > > On Sun, Jul 25, 2021 at 8:22 PM Jonathan Cameron <jic23@kernel.org> wrote: > > > > > > ... > > > > > > > > > - for_each_available_child_of_node(np, child) { > > > > > > + device_for_each_child_node(dev, child) { > > > > > > > > > > Isn't this > > > > > fwnode_for_each_available_child_node() > > > > > better to use? > > > > > > > > Given we would be extracting the fwnode just to call this > > > > loop, I'd say no, device version makes more sense.. > > > > > > > > > > > > > > ... > > > > > > > > > > So the gaps I see are > > > > > device_get_available_child_node_count() > > > > > and > > > > > device_for_each_available_child_node() > > > > > > > > Do we then fix the fact that > > > > device_for_each_child_node() will call the _available() form > > > > for device tree? That seems inconsistent currently and > > > > I was assuming that was deliberate... > > > > > > I'm not sure I got your point. Mine (see below) is to add the APIs > > > that you want to use as a direct replacement of the corresponding OF > > > counterparts. > > +CC Rafael, > > Rafael, if you have a chance to give input on the questions below it would > be much appreciated. Rafael, if you have a chance to look at this it would be great. > > Thanks, > > Jonathan > > > > > The oddity is that device_for_each_child_node() is a direct replacement > > of the for_each_available_child_of_node() other than the obvious > > use of device rather than the of node. > > > > https://elixir.bootlin.com/linux/v5.14-rc3/source/drivers/of/property.c#L939 > > > > static struct fwnode_handle * > > of_fwnode_get_next_child_node(const struct fwnode_handle *fwnode, > > struct fwnode_handle *child) > > { > > return of_fwnode_handle(of_get_next_available_child(to_of_node(fwnode), > > to_of_node(child))); > > } > > > > So the question becomes whether there is any desire at all to have a > > version of the device_for_each_child_node() that does not check > > if it is available or not. > > > > Looks like it goes all the way back. Rafael, any comment on why the available > > for is used here and whether it makes sense to introduce separate > > versions for looping over children that cover the _available_ and everything > > cases? > > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/base/property.c?id=8a0662d9ed2968e1186208336a8e1fab3fdfea63 > > > > I'm kind of assuming this was deliberate as we don't want to encourage > > accessing disabled firmware nodes. > > > > Jonathan > > > > > > > > > > Both of them I think are easy to add and avoid possible breakage. > > > > > > > > >
Rafael, looking for your input on this. Just converted it again, having forgotten this patch set was outstanding until I saw the same line of code and it all came back.. Thanks, Jonathan On Sun, 3 Oct 2021 16:45:41 +0100 Jonathan Cameron <jic23@kernel.org> wrote: > On Sun, 15 Aug 2021 17:09:51 +0100 > Jonathan Cameron <jic23@kernel.org> wrote: > > > On Tue, 27 Jul 2021 19:20:13 +0100 > > Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote: > > > > > On Tue, 27 Jul 2021 17:16:07 +0300 > > > Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > > > > > > On Tue, Jul 27, 2021 at 4:52 PM Jonathan Cameron > > > > <Jonathan.Cameron@huawei.com> wrote: > > > > > On Sun, 25 Jul 2021 23:33:12 +0300 > > > > > Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > > > > > On Sun, Jul 25, 2021 at 8:22 PM Jonathan Cameron <jic23@kernel.org> wrote: > > > > > > > > ... > > > > > > > > > > > - for_each_available_child_of_node(np, child) { > > > > > > > + device_for_each_child_node(dev, child) { > > > > > > > > > > > > Isn't this > > > > > > fwnode_for_each_available_child_node() > > > > > > better to use? > > > > > > > > > > Given we would be extracting the fwnode just to call this > > > > > loop, I'd say no, device version makes more sense.. > > > > > > > > > > > > > > > > > ... > > > > > > > > > > > > So the gaps I see are > > > > > > device_get_available_child_node_count() > > > > > > and > > > > > > device_for_each_available_child_node() > > > > > > > > > > Do we then fix the fact that > > > > > device_for_each_child_node() will call the _available() form > > > > > for device tree? That seems inconsistent currently and > > > > > I was assuming that was deliberate... > > > > > > > > I'm not sure I got your point. Mine (see below) is to add the APIs > > > > that you want to use as a direct replacement of the corresponding OF > > > > counterparts. > > > +CC Rafael, > > > > Rafael, if you have a chance to give input on the questions below it would > > be much appreciated. > > Rafael, if you have a chance to look at this it would be great. > > > > Thanks, > > > > Jonathan > > > > > > > > The oddity is that device_for_each_child_node() is a direct replacement > > > of the for_each_available_child_of_node() other than the obvious > > > use of device rather than the of node. > > > > > > https://elixir.bootlin.com/linux/v5.14-rc3/source/drivers/of/property.c#L939 > > > > > > static struct fwnode_handle * > > > of_fwnode_get_next_child_node(const struct fwnode_handle *fwnode, > > > struct fwnode_handle *child) > > > { > > > return of_fwnode_handle(of_get_next_available_child(to_of_node(fwnode), > > > to_of_node(child))); > > > } > > > > > > So the question becomes whether there is any desire at all to have a > > > version of the device_for_each_child_node() that does not check > > > if it is available or not. > > > > > > Looks like it goes all the way back. Rafael, any comment on why the available > > > for is used here and whether it makes sense to introduce separate > > > versions for looping over children that cover the _available_ and everything > > > cases? > > > > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/base/property.c?id=8a0662d9ed2968e1186208336a8e1fab3fdfea63 > > > > > > I'm kind of assuming this was deliberate as we don't want to encourage > > > accessing disabled firmware nodes. > > > > > > Jonathan > > > > > > > > > > > > > Both of them I think are easy to add and avoid possible breakage. > > > > > > > > > > > > > >
diff --git a/drivers/iio/adc/ad7124.c b/drivers/iio/adc/ad7124.c index b2e49386d7dc..bbb9830e13c2 100644 --- a/drivers/iio/adc/ad7124.c +++ b/drivers/iio/adc/ad7124.c @@ -14,7 +14,7 @@ #include <linux/kernel.h> #include <linux/kfifo.h> #include <linux/module.h> -#include <linux/of_device.h> +#include <linux/property.h> #include <linux/regulator/consumer.h> #include <linux/spi/spi.h> @@ -733,18 +733,20 @@ static int ad7124_check_chip_id(struct ad7124_state *st) return 0; } -static int ad7124_of_parse_channel_config(struct iio_dev *indio_dev, - struct device_node *np) +static int ad7124_parse_channel_config(struct iio_dev *indio_dev, + struct device *dev) { struct ad7124_state *st = iio_priv(indio_dev); struct ad7124_channel_config *cfg; struct ad7124_channel *channels; - struct device_node *child; + struct fwnode_handle *child; struct iio_chan_spec *chan; unsigned int ain[2], channel = 0, tmp; int ret; - st->num_channels = of_get_available_child_count(np); + device_for_each_child_node(dev, child) + st->num_channels++; + if (!st->num_channels) { dev_err(indio_dev->dev.parent, "no channel children\n"); return -ENODEV; @@ -764,9 +766,8 @@ 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) { - - ret = of_property_read_u32(child, "reg", &channel); + device_for_each_child_node(dev, child) { + ret = fwnode_property_read_u32(child, "reg", &channel); if (ret) goto err; @@ -779,8 +780,8 @@ static int ad7124_of_parse_channel_config(struct iio_dev *indio_dev, cfg = &st->channels[channel].cfg; - ret = of_property_read_u32_array(child, "diff-channels", - ain, 2); + ret = fwnode_property_read_u32_array(child, "diff-channels", + ain, 2); if (ret) goto err; @@ -788,16 +789,16 @@ static int ad7124_of_parse_channel_config(struct iio_dev *indio_dev, st->channels[channel].ain = AD7124_CHANNEL_AINP(ain[0]) | AD7124_CHANNEL_AINM(ain[1]); - cfg->bipolar = of_property_read_bool(child, "bipolar"); + cfg->bipolar = fwnode_property_read_bool(child, "bipolar"); - ret = of_property_read_u32(child, "adi,reference-select", &tmp); + ret = fwnode_property_read_u32(child, "adi,reference-select", &tmp); if (ret) cfg->refsel = AD7124_INT_REF; else cfg->refsel = tmp; - cfg->buf_positive = of_property_read_bool(child, "adi,buffered-positive"); - cfg->buf_negative = of_property_read_bool(child, "adi,buffered-negative"); + cfg->buf_positive = fwnode_property_read_bool(child, "adi,buffered-positive"); + cfg->buf_negative = fwnode_property_read_bool(child, "adi,buffered-negative"); chan[channel] = ad7124_channel_template; chan[channel].address = channel; @@ -808,7 +809,7 @@ static int ad7124_of_parse_channel_config(struct iio_dev *indio_dev, return 0; err: - of_node_put(child); + fwnode_handle_put(child); return ret; } @@ -875,7 +876,7 @@ static int ad7124_probe(struct spi_device *spi) struct iio_dev *indio_dev; int i, ret; - info = of_device_get_match_data(&spi->dev); + info = device_get_match_data(&spi->dev); if (!info) return -ENODEV; @@ -893,7 +894,7 @@ static int ad7124_probe(struct spi_device *spi) indio_dev->modes = INDIO_DIRECT_MODE; indio_dev->info = &ad7124_info; - ret = ad7124_of_parse_channel_config(indio_dev, spi->dev.of_node); + ret = ad7124_parse_channel_config(indio_dev, &spi->dev); if (ret < 0) return ret;