All of lore.kernel.org
 help / color / mirror / Atom feed
From: Jacopo Mondi <jacopo+renesas@jmondi.org>
To: "Kieran Bingham" <kieran.bingham+renesas@ideasonboard.com>,
	"Laurent Pinchart" <laurent.pinchart+renesas@ideasonboard.com>,
	"Niklas Söderlund" <niklas.soderlund+renesas@ragnatech.se>
Cc: Jacopo Mondi <jacopo+renesas@jmondi.org>,
	Mauro Carvalho Chehab <mchehab@kernel.org>,
	linux-media@vger.kernel.org, linux-renesas-soc@vger.kernel.org
Subject: [PATCH v2 2/2] media: max9271: Fail loud on bus errors in call sites
Date: Thu,  4 Nov 2021 12:09:24 +0100	[thread overview]
Message-ID: <20211104110924.248444-3-jacopo+renesas@jmondi.org> (raw)
In-Reply-To: <20211104110924.248444-1-jacopo+renesas@jmondi.org>

As not all bus access errors are fatal, as in example reads performed
in a busy loop, it's responsibility of the bus access function caller
to fail louder on fatal errors.

Instrument all functions in the max9271 library driver to fail on fatal
read/write errors and demote the max9271_write() error level to debug
to align it to the one in max9271_read().

While at it, align the style of the existing error messages by removing
"MAX9271" from the output string, as the device log helpers already
identify the driver emitting the message.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 drivers/media/i2c/max9271.c | 105 ++++++++++++++++++++++--------------
 1 file changed, 64 insertions(+), 41 deletions(-)

diff --git a/drivers/media/i2c/max9271.c b/drivers/media/i2c/max9271.c
index aa4add473716..f5f354b8a43c 100644
--- a/drivers/media/i2c/max9271.c
+++ b/drivers/media/i2c/max9271.c
@@ -45,7 +45,7 @@ static int max9271_write(struct max9271_device *dev, u8 reg, u8 val)
 
 	ret = i2c_smbus_write_byte_data(dev->client, reg, val);
 	if (ret < 0)
-		dev_err(&dev->client->dev,
+		dev_dbg(&dev->client->dev,
 			"%s: register 0x%02x write failed (%d)\n",
 			__func__, reg, ret);
 
@@ -120,8 +120,11 @@ int max9271_set_serial_link(struct max9271_device *dev, bool enable)
 	 * Therefore a conservative delay seems best here.
 	 */
 	ret = max9271_write(dev, 0x04, val);
-	if (ret < 0)
+	if (ret < 0) {
+		dev_err(&dev->client->dev, "Failed to set serial link (%d)\n",
+			ret);
 		return ret;
+	}
 
 	usleep_range(5000, 8000);
 
@@ -134,8 +137,11 @@ int max9271_configure_i2c(struct max9271_device *dev, u8 i2c_config)
 	int ret;
 
 	ret = max9271_write(dev, 0x0d, i2c_config);
-	if (ret < 0)
+	if (ret < 0) {
+		dev_err(&dev->client->dev, "Failed to configure I2C (%d)\n",
+			ret);
 		return ret;
+	}
 
 	/* The delay required after an I2C bus configuration change is not
 	 * characterized in the serializer manual. Sleep up to 5msec to
@@ -153,7 +159,7 @@ int max9271_set_high_threshold(struct max9271_device *dev, bool enable)
 
 	ret = max9271_read(dev, 0x08);
 	if (ret < 0)
-		return ret;
+		goto out;
 
 	/*
 	 * Enable or disable reverse channel high threshold to increase
@@ -161,11 +167,15 @@ int max9271_set_high_threshold(struct max9271_device *dev, bool enable)
 	 */
 	ret = max9271_write(dev, 0x08, enable ? ret | BIT(0) : ret & ~BIT(0));
 	if (ret < 0)
-		return ret;
+		goto out;
 
 	usleep_range(2000, 2500);
 
 	return 0;
+
+out:
+	dev_err(&dev->client->dev, "Failed to set high threshold (%d)\n", ret);
+	return ret;
 }
 EXPORT_SYMBOL_GPL(max9271_set_high_threshold);
 
@@ -186,7 +196,7 @@ int max9271_configure_gmsl_link(struct max9271_device *dev)
 	ret = max9271_write(dev, 0x07, MAX9271_DBL | MAX9271_HVEN |
 			    MAX9271_EDC_1BIT_PARITY);
 	if (ret < 0)
-		return ret;
+		goto out;
 
 	usleep_range(5000, 8000);
 
@@ -199,11 +209,15 @@ int max9271_configure_gmsl_link(struct max9271_device *dev)
 			    MAX9271_PCLK_AUTODETECT |
 			    MAX9271_SERIAL_AUTODETECT);
 	if (ret < 0)
-		return ret;
+		goto out;
 
 	usleep_range(5000, 8000);
 
 	return 0;
+
+out:
+	dev_err(&dev->client->dev, "Failed to configure GMSL link (%d)\n", ret);
+	return ret;
 }
 EXPORT_SYMBOL_GPL(max9271_configure_gmsl_link);
 
@@ -213,18 +227,20 @@ int max9271_set_gpios(struct max9271_device *dev, u8 gpio_mask)
 
 	ret = max9271_read(dev, 0x0f);
 	if (ret < 0)
-		return 0;
+		goto out;
 
 	ret |= gpio_mask;
 	ret = max9271_write(dev, 0x0f, ret);
-	if (ret < 0) {
-		dev_err(&dev->client->dev, "Failed to set gpio (%d)\n", ret);
-		return ret;
-	}
+	if (ret < 0)
+		goto out;
 
 	usleep_range(3500, 5000);
 
 	return 0;
+
+out:
+	dev_err(&dev->client->dev, "Failed to set gpio (%d)\n", ret);
+	return ret;
 }
 EXPORT_SYMBOL_GPL(max9271_set_gpios);
 
@@ -234,18 +250,20 @@ int max9271_clear_gpios(struct max9271_device *dev, u8 gpio_mask)
 
 	ret = max9271_read(dev, 0x0f);
 	if (ret < 0)
-		return 0;
+		goto out;
 
 	ret &= ~gpio_mask;
 	ret = max9271_write(dev, 0x0f, ret);
-	if (ret < 0) {
-		dev_err(&dev->client->dev, "Failed to clear gpio (%d)\n", ret);
-		return ret;
-	}
+	if (ret < 0)
+		goto out;
 
 	usleep_range(3500, 5000);
 
 	return 0;
+
+out:
+	dev_err(&dev->client->dev, "Failed to clear gpio (%d)\n", ret);
+	return ret;
 }
 EXPORT_SYMBOL_GPL(max9271_clear_gpios);
 
@@ -255,19 +273,21 @@ int max9271_enable_gpios(struct max9271_device *dev, u8 gpio_mask)
 
 	ret = max9271_read(dev, 0x0e);
 	if (ret < 0)
-		return 0;
+		goto out;
 
 	/* BIT(0) reserved: GPO is always enabled. */
 	ret |= (gpio_mask & ~BIT(0));
 	ret = max9271_write(dev, 0x0e, ret);
-	if (ret < 0) {
-		dev_err(&dev->client->dev, "Failed to enable gpio (%d)\n", ret);
-		return ret;
-	}
+	if (ret < 0)
+		goto out;
 
 	usleep_range(3500, 5000);
 
 	return 0;
+
+out:
+	dev_err(&dev->client->dev, "Failed to enable gpio (%d)\n", ret);
+	return ret;
 }
 EXPORT_SYMBOL_GPL(max9271_enable_gpios);
 
@@ -277,19 +297,21 @@ int max9271_disable_gpios(struct max9271_device *dev, u8 gpio_mask)
 
 	ret = max9271_read(dev, 0x0e);
 	if (ret < 0)
-		return 0;
+		goto out;
 
 	/* BIT(0) reserved: GPO cannot be disabled */
 	ret &= ~(gpio_mask | BIT(0));
 	ret = max9271_write(dev, 0x0e, ret);
-	if (ret < 0) {
-		dev_err(&dev->client->dev, "Failed to disable gpio (%d)\n", ret);
-		return ret;
-	}
+	if (ret < 0)
+		goto out;
 
 	usleep_range(3500, 5000);
 
 	return 0;
+
+out:
+	dev_err(&dev->client->dev, "Failed to disable gpio (%d)\n", ret);
+	return ret;
 }
 EXPORT_SYMBOL_GPL(max9271_disable_gpios);
 
@@ -299,13 +321,13 @@ int max9271_verify_id(struct max9271_device *dev)
 
 	ret = max9271_read(dev, 0x1e);
 	if (ret < 0) {
-		dev_err(&dev->client->dev, "MAX9271 ID read failed (%d)\n",
+		dev_err(&dev->client->dev, "Failed to read the chip ID (%d)\n",
 			ret);
 		return ret;
 	}
 
 	if (ret != MAX9271_ID) {
-		dev_err(&dev->client->dev, "MAX9271 ID mismatch (0x%02x)\n",
+		dev_err(&dev->client->dev, "Chip ID mismatch (0x%02x)\n",
 			ret);
 		return -ENXIO;
 	}
@@ -321,7 +343,7 @@ int max9271_set_address(struct max9271_device *dev, u8 addr)
 	ret = max9271_write(dev, 0x00, addr << 1);
 	if (ret < 0) {
 		dev_err(&dev->client->dev,
-			"MAX9271 I2C address change failed (%d)\n", ret);
+			"Failed to change I2C address (%d)\n", ret);
 		return ret;
 	}
 	usleep_range(3500, 5000);
@@ -337,7 +359,7 @@ int max9271_set_deserializer_address(struct max9271_device *dev, u8 addr)
 	ret = max9271_write(dev, 0x01, addr << 1);
 	if (ret < 0) {
 		dev_err(&dev->client->dev,
-			"MAX9271 deserializer address set failed (%d)\n", ret);
+			"Failed to set deser address (%d)\n", ret);
 		return ret;
 	}
 	usleep_range(3500, 5000);
@@ -351,22 +373,23 @@ int max9271_set_translation(struct max9271_device *dev, u8 source, u8 dest)
 	int ret;
 
 	ret = max9271_write(dev, 0x09, source << 1);
-	if (ret < 0) {
-		dev_err(&dev->client->dev,
-			"MAX9271 I2C translation setup failed (%d)\n", ret);
-		return ret;
-	}
+	if (ret < 0)
+		goto out;
+
 	usleep_range(3500, 5000);
 
 	ret = max9271_write(dev, 0x0a, dest << 1);
-	if (ret < 0) {
-		dev_err(&dev->client->dev,
-			"MAX9271 I2C translation setup failed (%d)\n", ret);
-		return ret;
-	}
+	if (ret < 0)
+		goto out;
+
 	usleep_range(3500, 5000);
 
 	return 0;
+
+out:
+	dev_err(&dev->client->dev,
+		"Failed to set I2C addresses translation (%d)\n", ret);
+	return ret;
 }
 EXPORT_SYMBOL_GPL(max9271_set_translation);
 
-- 
2.33.1


  parent reply	other threads:[~2021-11-04 11:08 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-11-04 11:09 [PATCH v2 0/2] max9271: Fix pclk_detect silent failure Jacopo Mondi
2021-11-04 11:09 ` [PATCH v2 1/2] media: max9271: Ignore busy loop read errors Jacopo Mondi
2021-11-04 23:04   ` Kieran Bingham
2021-11-08 12:11     ` Jacopo Mondi
2021-11-08 10:45   ` Geert Uytterhoeven
2021-11-08 11:22     ` Jacopo Mondi
2021-11-04 11:09 ` Jacopo Mondi [this message]
2021-11-04 23:12   ` [PATCH v2 2/2] media: max9271: Fail loud on bus errors in call sites Kieran Bingham

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20211104110924.248444-3-jacopo+renesas@jmondi.org \
    --to=jacopo+renesas@jmondi.org \
    --cc=kieran.bingham+renesas@ideasonboard.com \
    --cc=laurent.pinchart+renesas@ideasonboard.com \
    --cc=linux-media@vger.kernel.org \
    --cc=linux-renesas-soc@vger.kernel.org \
    --cc=mchehab@kernel.org \
    --cc=niklas.soderlund+renesas@ragnatech.se \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
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.