[PATCH v1 1/3] iio: imu: adis16480: Make use of device properties

[PATCH v1 1/3] iio: imu: adis16480: Make use of device properties

[Andy Shevchenko]

Convert the module to be property provider agnostic and allow
it to be used on non-OF platforms.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/iio/imu/adis16480.c | 27 ++++++++++++++-------------
 1 file changed, 14 insertions(+), 13 deletions(-)

diff --git a/drivers/iio/imu/adis16480.c b/drivers/iio/imu/adis16480.c
index 44bbe3d19907..68eed088cca6 100644
--- a/drivers/iio/imu/adis16480.c
+++ b/drivers/iio/imu/adis16480.c
@@ -7,14 +7,16 @@
 
 #include <linux/clk.h>
 #include <linux/bitfield.h>
-#include <linux/of_irq.h>
 #include <linux/interrupt.h>
+#include <linux/irq.h>
 #include <linux/math.h>
 #include <linux/device.h>
 #include <linux/kernel.h>
 #include <linux/spi/spi.h>
+#include <linux/mod_devicetable.h>
 #include <linux/module.h>
 #include <linux/lcm.h>
+#include <linux/property.h>
 #include <linux/swab.h>
 #include <linux/crc32.h>
 
@@ -1239,9 +1241,10 @@ static int adis16480_enable_irq(struct adis *adis, bool enable)
 	return __adis_write_reg_16(adis, ADIS16480_REG_FNCTIO_CTRL, val);
 }
 
-static int adis16480_config_irq_pin(struct device_node *of_node,
-				    struct adis16480 *st)
+static int adis16480_config_irq_pin(struct adis16480 *st)
 {
+	struct device *dev = &st->adis.spi->dev;
+	struct fwnode_handle *fwnode = dev_fwnode(dev);
 	struct irq_data *desc;
 	enum adis16480_int_pin pin;
 	unsigned int irq_type;
@@ -1267,7 +1270,7 @@ static int adis16480_config_irq_pin(struct device_node *of_node,
 	 */
 	pin = ADIS16480_PIN_DIO1;
 	for (i = 0; i < ARRAY_SIZE(adis16480_int_pin_names); i++) {
-		irq = of_irq_get_byname(of_node, adis16480_int_pin_names[i]);
+		irq = fwnode_irq_get_byname(fwnode, adis16480_int_pin_names[i]);
 		if (irq > 0) {
 			pin = i;
 			break;
@@ -1295,15 +1298,15 @@ static int adis16480_config_irq_pin(struct device_node *of_node,
 	return adis_write_reg_16(&st->adis, ADIS16480_REG_FNCTIO_CTRL, val);
 }
 
-static int adis16480_of_get_ext_clk_pin(struct adis16480 *st,
-					struct device_node *of_node)
+static int adis16480_fw_get_ext_clk_pin(struct adis16480 *st)
 {
+	struct device *dev = &st->adis.spi->dev;
 	const char *ext_clk_pin;
 	enum adis16480_int_pin pin;
 	int i;
 
 	pin = ADIS16480_PIN_DIO2;
-	if (of_property_read_string(of_node, "adi,ext-clk-pin", &ext_clk_pin))
+	if (device_property_read_string(dev, "adi,ext-clk-pin", &ext_clk_pin))
 		goto clk_input_not_found;
 
 	for (i = 0; i < ARRAY_SIZE(adis16480_int_pin_names); i++) {
@@ -1317,9 +1320,7 @@ static int adis16480_of_get_ext_clk_pin(struct adis16480 *st,
 	return pin;
 }
 
-static int adis16480_ext_clk_config(struct adis16480 *st,
-				    struct device_node *of_node,
-				    bool enable)
+static int adis16480_ext_clk_config(struct adis16480 *st, bool enable)
 {
 	unsigned int mode, mask;
 	enum adis16480_int_pin pin;
@@ -1330,7 +1331,7 @@ static int adis16480_ext_clk_config(struct adis16480 *st,
 	if (ret)
 		return ret;
 
-	pin = adis16480_of_get_ext_clk_pin(st, of_node);
+	pin = adis16480_fw_get_ext_clk_pin(st);
 	/*
 	 * Each DIOx pin supports only one function at a time. When a single pin
 	 * has two assignments, the enable bit for a lower priority function
@@ -1438,7 +1439,7 @@ static int adis16480_probe(struct spi_device *spi)
 			return ret;
 	}
 
-	ret = adis16480_config_irq_pin(spi->dev.of_node, st);
+	ret = adis16480_config_irq_pin(st);
 	if (ret)
 		return ret;
 
@@ -1447,7 +1448,7 @@ static int adis16480_probe(struct spi_device *spi)
 		return ret;
 
 	if (!IS_ERR_OR_NULL(st->ext_clk)) {
-		ret = adis16480_ext_clk_config(st, spi->dev.of_node, true);
+		ret = adis16480_ext_clk_config(st, true);
 		if (ret)
 			return ret;
 
-- 
2.35.1

[PATCH v1 2/3] iio: imu: adis16480: Use temporary variable for struct device

[Andy Shevchenko]

Use temporary variable for struct device to make code neater.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/iio/imu/adis16480.c | 46 ++++++++++++++++++-------------------
 1 file changed, 23 insertions(+), 23 deletions(-)

diff --git a/drivers/iio/imu/adis16480.c b/drivers/iio/imu/adis16480.c
index 68eed088cca6..287914016f28 100644
--- a/drivers/iio/imu/adis16480.c
+++ b/drivers/iio/imu/adis16480.c
@@ -1121,6 +1121,7 @@ static irqreturn_t adis16480_trigger_handler(int irq, void *p)
 	struct iio_dev *indio_dev = pf->indio_dev;
 	struct adis16480 *st = iio_priv(indio_dev);
 	struct adis *adis = &st->adis;
+	struct device *dev = &adis->spi->dev;
 	int ret, bit, offset, i = 0;
 	__be16 *buffer;
 	u32 crc;
@@ -1132,7 +1133,7 @@ static irqreturn_t adis16480_trigger_handler(int irq, void *p)
 		adis->tx[1] = 0;
 		ret = spi_write(adis->spi, adis->tx, 2);
 		if (ret) {
-			dev_err(&adis->spi->dev, "Failed to change device page: %d\n", ret);
+			dev_err(dev, "Failed to change device page: %d\n", ret);
 			adis_dev_unlock(adis);
 			goto irq_done;
 		}
@@ -1142,7 +1143,7 @@ static irqreturn_t adis16480_trigger_handler(int irq, void *p)
 
 	ret = spi_sync(adis->spi, &adis->msg);
 	if (ret) {
-		dev_err(&adis->spi->dev, "Failed to read data: %d\n", ret);
+		dev_err(dev, "Failed to read data: %d\n", ret);
 		adis_dev_unlock(adis);
 		goto irq_done;
 	}
@@ -1170,14 +1171,14 @@ static irqreturn_t adis16480_trigger_handler(int irq, void *p)
 	}
 
 	if (offset == 4) {
-		dev_err(&adis->spi->dev, "Invalid burst data\n");
+		dev_err(dev, "Invalid burst data\n");
 		goto irq_done;
 	}
 
 	crc = be16_to_cpu(buffer[offset + 16]) << 16 | be16_to_cpu(buffer[offset + 15]);
 	valid = adis16480_validate_crc((u16 *)&buffer[offset], 15, crc);
 	if (!valid) {
-		dev_err(&adis->spi->dev, "Invalid crc\n");
+		dev_err(dev, "Invalid crc\n");
 		goto irq_done;
 	}
 
@@ -1216,12 +1217,12 @@ static const struct iio_info adis16480_info = {
 static int adis16480_stop_device(struct iio_dev *indio_dev)
 {
 	struct adis16480 *st = iio_priv(indio_dev);
+	struct device *dev = &st->adis.spi->dev;
 	int ret;
 
 	ret = adis_write_reg_16(&st->adis, ADIS16480_REG_SLP_CNT, BIT(9));
 	if (ret)
-		dev_err(&indio_dev->dev,
-			"Could not power down device: %d\n", ret);
+		dev_err(dev, "Could not power down device: %d\n", ret);
 
 	return ret;
 }
@@ -1253,7 +1254,7 @@ static int adis16480_config_irq_pin(struct adis16480 *st)
 
 	desc = irq_get_irq_data(st->adis.spi->irq);
 	if (!desc) {
-		dev_err(&st->adis.spi->dev, "Could not find IRQ %d\n", irq);
+		dev_err(dev, "Could not find IRQ %d\n", irq);
 		return -EINVAL;
 	}
 
@@ -1290,8 +1291,7 @@ static int adis16480_config_irq_pin(struct adis16480 *st)
 	} else if (irq_type == IRQ_TYPE_EDGE_FALLING) {
 		val |= ADIS16480_DRDY_POL(0);
 	} else {
-		dev_err(&st->adis.spi->dev,
-			"Invalid interrupt type 0x%x specified\n", irq_type);
+		dev_err(dev, "Invalid interrupt type 0x%x specified\n", irq_type);
 		return -EINVAL;
 	}
 	/* Write the data ready configuration to the FNCTIO_CTRL register */
@@ -1315,13 +1315,13 @@ static int adis16480_fw_get_ext_clk_pin(struct adis16480 *st)
 	}
 
 clk_input_not_found:
-	dev_info(&st->adis.spi->dev,
-		"clk input line not specified, using DIO2\n");
+	dev_info(dev, "clk input line not specified, using DIO2\n");
 	return pin;
 }
 
 static int adis16480_ext_clk_config(struct adis16480 *st, bool enable)
 {
+	struct device *dev = &st->adis.spi->dev;
 	unsigned int mode, mask;
 	enum adis16480_int_pin pin;
 	uint16_t val;
@@ -1338,9 +1338,7 @@ static int adis16480_ext_clk_config(struct adis16480 *st, bool enable)
 	 * automatically resets to zero (disabling the lower priority function).
 	 */
 	if (pin == ADIS16480_DRDY_SEL(val))
-		dev_warn(&st->adis.spi->dev,
-			"DIO%x pin supports only one function at a time\n",
-			pin + 1);
+		dev_warn(dev, "DIO%x pin supports only one function at a time\n", pin + 1);
 
 	mode = ADIS16480_SYNC_EN(enable) | ADIS16480_SYNC_SEL(pin);
 	mask = ADIS16480_SYNC_EN_MSK | ADIS16480_SYNC_SEL_MSK;
@@ -1362,27 +1360,29 @@ static int adis16480_ext_clk_config(struct adis16480 *st, bool enable)
 
 static int adis16480_get_ext_clocks(struct adis16480 *st)
 {
+	struct device *dev = &st->adis.spi->dev;
+
 	st->clk_mode = ADIS16480_CLK_INT;
-	st->ext_clk = devm_clk_get(&st->adis.spi->dev, "sync");
+	st->ext_clk = devm_clk_get(dev, "sync");
 	if (!IS_ERR_OR_NULL(st->ext_clk)) {
 		st->clk_mode = ADIS16480_CLK_SYNC;
 		return 0;
 	}
 
 	if (PTR_ERR(st->ext_clk) != -ENOENT) {
-		dev_err(&st->adis.spi->dev, "failed to get ext clk\n");
+		dev_err(dev, "failed to get ext clk\n");
 		return PTR_ERR(st->ext_clk);
 	}
 
 	if (st->chip_info->has_pps_clk_mode) {
-		st->ext_clk = devm_clk_get(&st->adis.spi->dev, "pps");
+		st->ext_clk = devm_clk_get(dev, "pps");
 		if (!IS_ERR_OR_NULL(st->ext_clk)) {
 			st->clk_mode = ADIS16480_CLK_PPS;
 			return 0;
 		}
 
 		if (PTR_ERR(st->ext_clk) != -ENOENT) {
-			dev_err(&st->adis.spi->dev, "failed to get ext clk\n");
+			dev_err(dev, "failed to get ext clk\n");
 			return PTR_ERR(st->ext_clk);
 		}
 	}
@@ -1405,11 +1405,12 @@ static int adis16480_probe(struct spi_device *spi)
 	const struct spi_device_id *id = spi_get_device_id(spi);
 	const struct adis_data *adis16480_data;
 	irq_handler_t trigger_handler = NULL;
+	struct device *dev = &spi->dev;
 	struct iio_dev *indio_dev;
 	struct adis16480 *st;
 	int ret;
 
-	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
+	indio_dev = devm_iio_device_alloc(dev, sizeof(*st));
 	if (indio_dev == NULL)
 		return -ENOMEM;
 
@@ -1433,8 +1434,7 @@ static int adis16480_probe(struct spi_device *spi)
 		return ret;
 
 	if (st->chip_info->has_sleep_cnt) {
-		ret = devm_add_action_or_reset(&spi->dev, adis16480_stop,
-					       indio_dev);
+		ret = devm_add_action_or_reset(dev, adis16480_stop, indio_dev);
 		if (ret)
 			return ret;
 	}
@@ -1452,7 +1452,7 @@ static int adis16480_probe(struct spi_device *spi)
 		if (ret)
 			return ret;
 
-		ret = devm_add_action_or_reset(&spi->dev, adis16480_clk_disable, st->ext_clk);
+		ret = devm_add_action_or_reset(dev, adis16480_clk_disable, st->ext_clk);
 		if (ret)
 			return ret;
 
@@ -1485,7 +1485,7 @@ static int adis16480_probe(struct spi_device *spi)
 	if (ret)
 		return ret;
 
-	ret = devm_iio_device_register(&spi->dev, indio_dev);
+	ret = devm_iio_device_register(dev, indio_dev);
 	if (ret)
 		return ret;
 
-- 
2.35.1

[PATCH v1 3/3] iio: imu: adis16480: Fix getting the optional clocks

[Andy Shevchenko]

The extended clocks are optional and may not be present for some SoCs
supported by this driver. Nevertheless, in case the clock is provided
but some error happens during its getting, that error should be handled
properly. Use devm_clk_get_optional() API for that. Also report possible
errors using dev_err_probe() to handle properly -EPROBE_DEFER error.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/iio/imu/adis16480.c | 26 ++++++++++----------------
 1 file changed, 10 insertions(+), 16 deletions(-)

diff --git a/drivers/iio/imu/adis16480.c b/drivers/iio/imu/adis16480.c
index 287914016f28..fe520194a837 100644
--- a/drivers/iio/imu/adis16480.c
+++ b/drivers/iio/imu/adis16480.c
@@ -1362,31 +1362,25 @@ static int adis16480_get_ext_clocks(struct adis16480 *st)
 {
 	struct device *dev = &st->adis.spi->dev;
 
-	st->clk_mode = ADIS16480_CLK_INT;
-	st->ext_clk = devm_clk_get(dev, "sync");
-	if (!IS_ERR_OR_NULL(st->ext_clk)) {
+	st->ext_clk = devm_clk_get_optional(dev, "sync");
+	if (IS_ERR(st->ext_clk))
+		return dev_err_probe(dev, PTR_ERR(st->ext_clk), "failed to get ext clk\n");
+	if (st->ext_clk) {
 		st->clk_mode = ADIS16480_CLK_SYNC;
 		return 0;
 	}
 
-	if (PTR_ERR(st->ext_clk) != -ENOENT) {
-		dev_err(dev, "failed to get ext clk\n");
-		return PTR_ERR(st->ext_clk);
-	}
-
 	if (st->chip_info->has_pps_clk_mode) {
-		st->ext_clk = devm_clk_get(dev, "pps");
-		if (!IS_ERR_OR_NULL(st->ext_clk)) {
+		st->ext_clk = devm_clk_get_optional(dev, "pps");
+		if (IS_ERR(st->ext_clk))
+			return dev_err_probe(dev, PTR_ERR(st->ext_clk), "failed to get ext clk\n");
+		if (st->ext_clk) {
 			st->clk_mode = ADIS16480_CLK_PPS;
 			return 0;
 		}
-
-		if (PTR_ERR(st->ext_clk) != -ENOENT) {
-			dev_err(dev, "failed to get ext clk\n");
-			return PTR_ERR(st->ext_clk);
-		}
 	}
 
+	st->clk_mode = ADIS16480_CLK_INT;
 	return 0;
 }
 
@@ -1447,7 +1441,7 @@ static int adis16480_probe(struct spi_device *spi)
 	if (ret)
 		return ret;
 
-	if (!IS_ERR_OR_NULL(st->ext_clk)) {
+	if (st->ext_clk) {
 		ret = adis16480_ext_clk_config(st, true);
 		if (ret)
 			return ret;
-- 
2.35.1

RE: [PATCH v1 1/3] iio: imu: adis16480: Make use of device properties

[Sa, Nuno]

> -----Original Message-----
> From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Sent: Wednesday, April 13, 2022 4:41 PM
> To: Sa, Nuno <Nuno.Sa@analog.com>; Andy Shevchenko
> <andriy.shevchenko@linux.intel.com>; linux-iio@vger.kernel.org;
> linux-kernel@vger.kernel.org
> Cc: Lars-Peter Clausen <lars@metafoo.de>; Hennerich, Michael
> <Michael.Hennerich@analog.com>; Jonathan Cameron
> <jic23@kernel.org>
> Subject: [PATCH v1 1/3] iio: imu: adis16480: Make use of device
> properties
> 
> [External]
> 
> Convert the module to be property provider agnostic and allow
> it to be used on non-OF platforms.
> 
> Signed-off-by: Andy Shevchenko
> <andriy.shevchenko@linux.intel.com>
> ---

You beat me to do this. I actually had planned to do this next week
once I saw we already have fwnode_irq_get_byname(). Anyways...

Reviewed-by: Nuno Sá <nuno.sa@analog.com>

(I will still give this a test next week)

RE: [PATCH v1 2/3] iio: imu: adis16480: Use temporary variable for struct device

[Sa, Nuno]

> -----Original Message-----
> From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Sent: Wednesday, April 13, 2022 4:41 PM
> To: Sa, Nuno <Nuno.Sa@analog.com>; Andy Shevchenko
> <andriy.shevchenko@linux.intel.com>; linux-iio@vger.kernel.org;
> linux-kernel@vger.kernel.org
> Cc: Lars-Peter Clausen <lars@metafoo.de>; Hennerich, Michael
> <Michael.Hennerich@analog.com>; Jonathan Cameron
> <jic23@kernel.org>
> Subject: [PATCH v1 2/3] iio: imu: adis16480: Use temporary variable for
> struct device
> 
> [External]
> 
> Use temporary variable for struct device to make code neater.
> 
> Signed-off-by: Andy Shevchenko
> <andriy.shevchenko@linux.intel.com>
> ---

Reviewed-by: Nuno Sá <nuno.sa@analog.com>

RE: [PATCH v1 3/3] iio: imu: adis16480: Fix getting the optional clocks

[Sa, Nuno]

> From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Sent: Wednesday, April 13, 2022 4:41 PM
> To: Sa, Nuno <Nuno.Sa@analog.com>; Andy Shevchenko
> <andriy.shevchenko@linux.intel.com>; linux-iio@vger.kernel.org;
> linux-kernel@vger.kernel.org
> Cc: Lars-Peter Clausen <lars@metafoo.de>; Hennerich, Michael
> <Michael.Hennerich@analog.com>; Jonathan Cameron
> <jic23@kernel.org>
> Subject: [PATCH v1 3/3] iio: imu: adis16480: Fix getting the optional
> clocks
> 
> [External]
> 
> The extended clocks are optional and may not be present for some
> SoCs
> supported by this driver. Nevertheless, in case the clock is provided
> but some error happens during its getting, that error should be
> handled
> properly. Use devm_clk_get_optional() API for that. Also report
> possible
> errors using dev_err_probe() to handle properly -EPROBE_DEFER
> error.
> 
> Signed-off-by: Andy Shevchenko
> <andriy.shevchenko@linux.intel.com>
> ---

This is a nice cleanup patch... But the subject might be a bit
misleading as it says "Fix". So I would expect a Fixes tag which
I'm not sure it's really worth it here. Yes, the code was pretty much
doing clk_get_optional() "by hand" but I think it was still functional.
So to me, this is more an improvement rather than a fix...

Anyways,

Reviewed-by: Nuno Sá <nuno.sa@analog.com>

Re: [PATCH v1 3/3] iio: imu: adis16480: Fix getting the optional clocks

[Andy Shevchenko]

On Wed, Apr 13, 2022 at 03:38:47PM +0000, Sa, Nuno wrote:
> > From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > Sent: Wednesday, April 13, 2022 4:41 PM

> > The extended clocks are optional and may not be present for some
> > SoCs
> > supported by this driver. Nevertheless, in case the clock is provided
> > but some error happens during its getting, that error should be
> > handled
> > properly. Use devm_clk_get_optional() API for that. Also report
> > possible
> > errors using dev_err_probe() to handle properly -EPROBE_DEFER
> > error.

> This is a nice cleanup patch... But the subject might be a bit
> misleading as it says "Fix". So I would expect a Fixes tag which
> I'm not sure it's really worth it here. Yes, the code was pretty much
> doing clk_get_optional() "by hand" but I think it was still functional.
> So to me, this is more an improvement rather than a fix...

Actually it is a fix, but not critical since no-one complains aloud so far.
The problematic part is logs exhausting if repetitive deferred probe happens.

> Anyways,
> 
> Reviewed-by: Nuno Sá <nuno.sa@analog.com>

Thanks!

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 1/3] iio: imu: adis16480: Make use of device properties

[Andy Shevchenko]

On Wed, Apr 13, 2022 at 03:25:49PM +0000, Sa, Nuno wrote:
> > -----Original Message-----
> > From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > Sent: Wednesday, April 13, 2022 4:41 PM

...

> You beat me to do this. I actually had planned to do this next week
> once I saw we already have fwnode_irq_get_byname(). Anyways...
> 
> Reviewed-by: Nuno Sá <nuno.sa@analog.com>
> 
> (I will still give this a test next week)

Thanks!

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 3/3] iio: imu: adis16480: Fix getting the optional clocks

[Nuno Sá]

On Wed, 2022-04-13 at 19:58 +0300, Andy Shevchenko wrote:
> On Wed, Apr 13, 2022 at 03:38:47PM +0000, Sa, Nuno wrote:
> > > From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > > Sent: Wednesday, April 13, 2022 4:41 PM
> 
> > > The extended clocks are optional and may not be present for some
> > > SoCs
> > > supported by this driver. Nevertheless, in case the clock is
> > > provided
> > > but some error happens during its getting, that error should be
> > > handled
> > > properly. Use devm_clk_get_optional() API for that. Also report
> > > possible
> > > errors using dev_err_probe() to handle properly -EPROBE_DEFER
> > > error.
> 
> > This is a nice cleanup patch... But the subject might be a bit
> > misleading as it says "Fix". So I would expect a Fixes tag which
> > I'm not sure it's really worth it here. Yes, the code was pretty
> > much
> > doing clk_get_optional() "by hand" but I think it was still
> > functional.
> > So to me, this is more an improvement rather than a fix...
> 
> Actually it is a fix, but not critical since no-one complains aloud
> so far.
> The problematic part is logs exhausting if repetitive deferred probe
> happens.
> 

Still not really agree with it... In the commit message you state that
errors are not properly handled and so let's use
'devm_clk_get_optional()'. I don't think that is true because If im not
missing nothing there's no fundamental change between the previous code
and using 'devm_clk_get_optional()'. So to me this is an enhancement
because we were doing something "by hand" when we have an API for it.

That said, introducing dev_err_probe() indeed stops possibly annoying
error messages for EPROBE_DEFER (and that could be seen as a fix, not
really devm_clk_get_optional()). I honestly still don't see it as fix
but we are also not adding a Fixes tag so I don't really care :).

(But I still think the commit message is a bit misleading)

- Nuno Sá
> 
> 

Re: [PATCH v1 3/3] iio: imu: adis16480: Fix getting the optional clocks

[Andy Shevchenko]

On Thu, Apr 14, 2022 at 09:07:29AM +0200, Nuno Sá wrote:
> On Wed, 2022-04-13 at 19:58 +0300, Andy Shevchenko wrote:
> > On Wed, Apr 13, 2022 at 03:38:47PM +0000, Sa, Nuno wrote:
> > > > From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > > > Sent: Wednesday, April 13, 2022 4:41 PM
> > 
> > > > The extended clocks are optional and may not be present for some
> > > > SoCs
> > > > supported by this driver. Nevertheless, in case the clock is
> > > > provided
> > > > but some error happens during its getting, that error should be
> > > > handled
> > > > properly. Use devm_clk_get_optional() API for that. Also report
> > > > possible
> > > > errors using dev_err_probe() to handle properly -EPROBE_DEFER
> > > > error.
> > 
> > > This is a nice cleanup patch... But the subject might be a bit
> > > misleading as it says "Fix". So I would expect a Fixes tag which
> > > I'm not sure it's really worth it here. Yes, the code was pretty
> > > much
> > > doing clk_get_optional() "by hand" but I think it was still
> > > functional.
> > > So to me, this is more an improvement rather than a fix...
> > 
> > Actually it is a fix, but not critical since no-one complains aloud
> > so far.
> > The problematic part is logs exhausting if repetitive deferred probe
> > happens.
> 
> Still not really agree with it... In the commit message you state that
> errors are not properly handled and so let's use
> 'devm_clk_get_optional()'. I don't think that is true because If im not
> missing nothing there's no fundamental change between the previous code
> and using 'devm_clk_get_optional()'. So to me this is an enhancement
> because we were doing something "by hand" when we have an API for it.
> 
> That said, introducing dev_err_probe() indeed stops possibly annoying
> error messages for EPROBE_DEFER (and that could be seen as a fix, not
> really devm_clk_get_optional()). I honestly still don't see it as fix
> but we are also not adding a Fixes tag so I don't really care :).
> 
> (But I still think the commit message is a bit misleading)

Yes, the commit message can be amended. I won't split these two since
the fix part is not critical (nobody so far complained aloud about it).
That's why I prefer to set the main point of the change as switching to
devm_clk_get_optional() than deferred probe messages.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 3/3] iio: imu: adis16480: Fix getting the optional clocks

[Andy Shevchenko]

On Thu, Apr 14, 2022 at 03:48:42PM +0300, Andy Shevchenko wrote:
> On Thu, Apr 14, 2022 at 09:07:29AM +0200, Nuno Sá wrote:
> > On Wed, 2022-04-13 at 19:58 +0300, Andy Shevchenko wrote:

...

> > (But I still think the commit message is a bit misleading)
> 
> Yes, the commit message can be amended. I won't split these two since
> the fix part is not critical (nobody so far complained aloud about it).
> That's why I prefer to set the main point of the change as switching to
> devm_clk_get_optional() than deferred probe messages.

That said, I will amend it for v2.
Thank you for review!

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 1/3] iio: imu: adis16480: Make use of device properties

[Jonathan Cameron]

On Wed, 13 Apr 2022 19:58:53 +0300
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> On Wed, Apr 13, 2022 at 03:25:49PM +0000, Sa, Nuno wrote:
> > > -----Original Message-----
> > > From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > > Sent: Wednesday, April 13, 2022 4:41 PM  
> 
> ...
> 
> > You beat me to do this. I actually had planned to do this next week
> > once I saw we already have fwnode_irq_get_byname(). Anyways...
> > 
> > Reviewed-by: Nuno Sá <nuno.sa@analog.com>
> > 
> > (I will still give this a test next week)  
> 
> Thanks!
> 
I'll wait to apply it until you confirm all was good (mind you I haven't
read it yet :)

Thanks,

Jonathan

RE: [PATCH v1 1/3] iio: imu: adis16480: Make use of device properties

[Sa, Nuno]

> -----Original Message-----
> From: Jonathan Cameron <jic23@kernel.org>
> Sent: Friday, April 15, 2022 8:04 PM
> To: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Cc: Sa, Nuno <Nuno.Sa@analog.com>; linux-iio@vger.kernel.org;
> linux-kernel@vger.kernel.org; Lars-Peter Clausen <lars@metafoo.de>;
> Hennerich, Michael <Michael.Hennerich@analog.com>
> Subject: Re: [PATCH v1 1/3] iio: imu: adis16480: Make use of device
> properties
> 
> [External]
> 
> On Wed, 13 Apr 2022 19:58:53 +0300
> Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> 
> > On Wed, Apr 13, 2022 at 03:25:49PM +0000, Sa, Nuno wrote:
> > > > -----Original Message-----
> > > > From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > > > Sent: Wednesday, April 13, 2022 4:41 PM
> >
> > ...
> >
> > > You beat me to do this. I actually had planned to do this next week
> > > once I saw we already have fwnode_irq_get_byname().
> Anyways...
> > >
> > > Reviewed-by: Nuno Sá <nuno.sa@analog.com>
> > >
> > > (I will still give this a test next week)
> >
> > Thanks!
> >
> I'll wait to apply it until you confirm all was good (mind you I haven't
> read it yet :)
> 

Fell free to add

Tested-by: Nuno Sá <nuno.sa@analog.com>