[PATCH v3 1/3] drm/bridge: parade-ps8640: Improve logging at probing

[PATCH v3 1/3] drm/bridge: parade-ps8640: Improve logging at probing

[Philip Chen]

Use dev_err_probe() to add logs for error cases at probing time.

Signed-off-by: Philip Chen <philipchen@chromium.org>
---

(no changes since v1)

 drivers/gpu/drm/bridge/parade-ps8640.c | 22 ++++++++++++++--------
 1 file changed, 14 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/bridge/parade-ps8640.c b/drivers/gpu/drm/bridge/parade-ps8640.c
index 685e9c38b2db..e340af381e05 100644
--- a/drivers/gpu/drm/bridge/parade-ps8640.c
+++ b/drivers/gpu/drm/bridge/parade-ps8640.c
@@ -332,8 +332,10 @@ static int ps8640_probe(struct i2c_client *client)
 		return -ENODEV;
 
 	ps_bridge->panel_bridge = devm_drm_panel_bridge_add(dev, panel);
-	if (IS_ERR(ps_bridge->panel_bridge))
-		return PTR_ERR(ps_bridge->panel_bridge);
+	if (IS_ERR(ps_bridge->panel_bridge)) {
+		return dev_err_probe(dev, PTR_ERR(ps_bridge->panel_bridge),
+				     "Error creating bridge device\n");
+	}
 
 	ps_bridge->supplies[0].supply = "vdd33";
 	ps_bridge->supplies[1].supply = "vdd12";
@@ -344,16 +346,20 @@ static int ps8640_probe(struct i2c_client *client)
 
 	ps_bridge->gpio_powerdown = devm_gpiod_get(&client->dev, "powerdown",
 						   GPIOD_OUT_HIGH);
-	if (IS_ERR(ps_bridge->gpio_powerdown))
-		return PTR_ERR(ps_bridge->gpio_powerdown);
+	if (IS_ERR(ps_bridge->gpio_powerdown)) {
+		return dev_err_probe(dev, PTR_ERR(ps_bridge->gpio_powerdown),
+				     "Error getting gpio_powerdown\n");
+	}
 
 	/*
 	 * Assert the reset to avoid the bridge being initialized prematurely
 	 */
 	ps_bridge->gpio_reset = devm_gpiod_get(&client->dev, "reset",
 					       GPIOD_OUT_HIGH);
-	if (IS_ERR(ps_bridge->gpio_reset))
-		return PTR_ERR(ps_bridge->gpio_reset);
+	if (IS_ERR(ps_bridge->gpio_reset)) {
+		return dev_err_probe(dev, PTR_ERR(ps_bridge->gpio_reset),
+				     "Error getting gpio_reset\n");
+	}
 
 	ps_bridge->bridge.funcs = &ps8640_bridge_funcs;
 	ps_bridge->bridge.of_node = dev->of_node;
@@ -367,9 +373,9 @@ static int ps8640_probe(struct i2c_client *client)
 							     client->adapter,
 							     client->addr + i);
 		if (IS_ERR(ps_bridge->page[i])) {
-			dev_err(dev, "failed i2c dummy device, address %02x\n",
+			return dev_err_probe(dev, PTR_ERR(ps_bridge->page[i]),
+				"Error initting i2c dummy dev, address %02x\n",
 				client->addr + i);
-			return PTR_ERR(ps_bridge->page[i]);
 		}
 	}
 
-- 
2.33.0.309.g3052b89438-goog

[PATCH v3 2/3] drm/bridge: parade-ps8640: Use regmap APIs

[Philip Chen]

Replace the direct i2c access (i2c_smbus_* functions) with regmap APIs,
which will simplify the future update on ps8640 driver.

Reviewed-by: Douglas Anderson <dianders@chromium.org>

Signed-off-by: Philip Chen <philipchen@chromium.org>
---

Changes in v3:
- Fix the nits from v2 review

Changes in v2:
- Add separate reg map config per page

 drivers/gpu/drm/bridge/parade-ps8640.c | 94 +++++++++++++++++++-------
 1 file changed, 68 insertions(+), 26 deletions(-)

diff --git a/drivers/gpu/drm/bridge/parade-ps8640.c b/drivers/gpu/drm/bridge/parade-ps8640.c
index e340af381e05..8d3e7a147170 100644
--- a/drivers/gpu/drm/bridge/parade-ps8640.c
+++ b/drivers/gpu/drm/bridge/parade-ps8640.c
@@ -9,6 +9,7 @@
 #include <linux/i2c.h>
 #include <linux/module.h>
 #include <linux/of_graph.h>
+#include <linux/regmap.h>
 #include <linux/regulator/consumer.h>
 
 #include <drm/drm_bridge.h>
@@ -31,6 +32,11 @@
 
 #define NUM_MIPI_LANES		4
 
+#define COMMON_PS8640_REGMAP_CONFIG \
+	.reg_bits = 8, \
+	.val_bits = 8, \
+	.cache_type = REGCACHE_NONE
+
 /*
  * PS8640 uses multiple addresses:
  * page[0]: for DP control
@@ -64,12 +70,48 @@ struct ps8640 {
 	struct drm_bridge *panel_bridge;
 	struct mipi_dsi_device *dsi;
 	struct i2c_client *page[MAX_DEVS];
+	struct regmap	*regmap[MAX_DEVS];
 	struct regulator_bulk_data supplies[2];
 	struct gpio_desc *gpio_reset;
 	struct gpio_desc *gpio_powerdown;
 	bool powered;
 };
 
+static const struct regmap_config ps8640_regmap_config[] = {
+	[PAGE0_DP_CNTL] = {
+		COMMON_PS8640_REGMAP_CONFIG,
+		.max_register = 0xbf,
+	},
+	[PAGE1_VDO_BDG] = {
+		COMMON_PS8640_REGMAP_CONFIG,
+		.max_register = 0xff,
+	},
+	[PAGE2_TOP_CNTL] = {
+		COMMON_PS8640_REGMAP_CONFIG,
+		.max_register = 0xff,
+	},
+	[PAGE3_DSI_CNTL1] = {
+		COMMON_PS8640_REGMAP_CONFIG,
+		.max_register = 0xff,
+	},
+	[PAGE4_MIPI_PHY] = {
+		COMMON_PS8640_REGMAP_CONFIG,
+		.max_register = 0xff,
+	},
+	[PAGE5_VPLL] = {
+		COMMON_PS8640_REGMAP_CONFIG,
+		.max_register = 0x7f,
+	},
+	[PAGE6_DSI_CNTL2] = {
+		COMMON_PS8640_REGMAP_CONFIG,
+		.max_register = 0xff,
+	},
+	[PAGE7_SPI_CNTL] = {
+		COMMON_PS8640_REGMAP_CONFIG,
+		.max_register = 0xff,
+	},
+};
+
 static inline struct ps8640 *bridge_to_ps8640(struct drm_bridge *e)
 {
 	return container_of(e, struct ps8640, bridge);
@@ -78,13 +120,13 @@ static inline struct ps8640 *bridge_to_ps8640(struct drm_bridge *e)
 static int ps8640_bridge_vdo_control(struct ps8640 *ps_bridge,
 				     const enum ps8640_vdo_control ctrl)
 {
-	struct i2c_client *client = ps_bridge->page[PAGE3_DSI_CNTL1];
+	struct regmap *map = ps_bridge->regmap[PAGE3_DSI_CNTL1];
 	u8 vdo_ctrl_buf[] = { VDO_CTL_ADD, ctrl };
 	int ret;
 
-	ret = i2c_smbus_write_i2c_block_data(client, PAGE3_SET_ADD,
-					     sizeof(vdo_ctrl_buf),
-					     vdo_ctrl_buf);
+	ret = regmap_bulk_write(map, PAGE3_SET_ADD,
+				vdo_ctrl_buf, sizeof(vdo_ctrl_buf));
+
 	if (ret < 0) {
 		DRM_ERROR("failed to %sable VDO: %d\n",
 			  ctrl == ENABLE ? "en" : "dis", ret);
@@ -96,8 +138,7 @@ static int ps8640_bridge_vdo_control(struct ps8640 *ps_bridge,
 
 static void ps8640_bridge_poweron(struct ps8640 *ps_bridge)
 {
-	struct i2c_client *client = ps_bridge->page[PAGE2_TOP_CNTL];
-	unsigned long timeout;
+	struct regmap *map = ps_bridge->regmap[PAGE2_TOP_CNTL];
 	int ret, status;
 
 	if (ps_bridge->powered)
@@ -121,18 +162,12 @@ static void ps8640_bridge_poweron(struct ps8640 *ps_bridge)
 	 */
 	msleep(200);
 
-	timeout = jiffies + msecs_to_jiffies(200) + 1;
-
-	while (time_is_after_jiffies(timeout)) {
-		status = i2c_smbus_read_byte_data(client, PAGE2_GPIO_H);
-		if (status < 0) {
-			DRM_ERROR("failed read PAGE2_GPIO_H: %d\n", status);
-			goto err_regulators_disable;
-		}
-		if ((status & PS_GPIO9) == PS_GPIO9)
-			break;
+	ret = regmap_read_poll_timeout(map, PAGE2_GPIO_H, status,
+			status & PS_GPIO9, 20 * 1000, 200 * 1000);
 
-		msleep(20);
+	if (ret < 0) {
+		DRM_ERROR("failed read PAGE2_GPIO_H: %d\n", ret);
+		goto err_regulators_disable;
 	}
 
 	msleep(50);
@@ -144,22 +179,15 @@ static void ps8640_bridge_poweron(struct ps8640 *ps_bridge)
 	 * disabled by the manufacturer. Once disabled, all MCS commands are
 	 * ignored by the display interface.
 	 */
-	status = i2c_smbus_read_byte_data(client, PAGE2_MCS_EN);
-	if (status < 0) {
-		DRM_ERROR("failed read PAGE2_MCS_EN: %d\n", status);
-		goto err_regulators_disable;
-	}
 
-	ret = i2c_smbus_write_byte_data(client, PAGE2_MCS_EN,
-					status & ~MCS_EN);
+	ret = regmap_update_bits(map, PAGE2_MCS_EN, MCS_EN, 0);
 	if (ret < 0) {
 		DRM_ERROR("failed write PAGE2_MCS_EN: %d\n", ret);
 		goto err_regulators_disable;
 	}
 
 	/* Switch access edp panel's edid through i2c */
-	ret = i2c_smbus_write_byte_data(client, PAGE2_I2C_BYPASS,
-					I2C_BYPASS_EN);
+	ret = regmap_write(map, PAGE2_I2C_BYPASS, I2C_BYPASS_EN);
 	if (ret < 0) {
 		DRM_ERROR("failed write PAGE2_I2C_BYPASS: %d\n", ret);
 		goto err_regulators_disable;
@@ -368,6 +396,12 @@ static int ps8640_probe(struct i2c_client *client)
 
 	ps_bridge->page[PAGE0_DP_CNTL] = client;
 
+	ps_bridge->regmap[PAGE0_DP_CNTL] = devm_regmap_init_i2c(client, ps8640_regmap_config);
+	if (IS_ERR(ps_bridge->regmap[PAGE0_DP_CNTL])) {
+		return dev_err_probe(dev, PTR_ERR(ps_bridge->regmap[PAGE0_DP_CNTL]),
+				     "Error initting page 0 regmap\n");
+	}
+
 	for (i = 1; i < ARRAY_SIZE(ps_bridge->page); i++) {
 		ps_bridge->page[i] = devm_i2c_new_dummy_device(&client->dev,
 							     client->adapter,
@@ -377,6 +411,14 @@ static int ps8640_probe(struct i2c_client *client)
 				"Error initting i2c dummy dev, address %02x\n",
 				client->addr + i);
 		}
+
+		ps_bridge->regmap[i] = devm_regmap_init_i2c(ps_bridge->page[i],
+						ps8640_regmap_config + i);
+		if (IS_ERR(ps_bridge->regmap[i])) {
+			return dev_err_probe(dev,
+				PTR_ERR(ps_bridge->regmap[i]),
+				"Error initting page %d regmap\n", i);
+		}
 	}
 
 	i2c_set_clientdata(client, ps_bridge);
-- 
2.33.0.309.g3052b89438-goog

Re: [PATCH v3 2/3] drm/bridge: parade-ps8640: Use regmap APIs

[Stephen Boyd]

Quoting Philip Chen (2021-09-14 16:28:44)
> diff --git a/drivers/gpu/drm/bridge/parade-ps8640.c b/drivers/gpu/drm/bridge/parade-ps8640.c
> index e340af381e05..8d3e7a147170 100644
> --- a/drivers/gpu/drm/bridge/parade-ps8640.c
> +++ b/drivers/gpu/drm/bridge/parade-ps8640.c
> @@ -368,6 +396,12 @@ static int ps8640_probe(struct i2c_client *client)
>
>         ps_bridge->page[PAGE0_DP_CNTL] = client;
>
> +       ps_bridge->regmap[PAGE0_DP_CNTL] = devm_regmap_init_i2c(client, ps8640_regmap_config);
> +       if (IS_ERR(ps_bridge->regmap[PAGE0_DP_CNTL])) {
> +               return dev_err_probe(dev, PTR_ERR(ps_bridge->regmap[PAGE0_DP_CNTL]),
> +                                    "Error initting page 0 regmap\n");

This one also doesn't return -EPROBE_DEFER? The dev_err_probe() should
really only be used on "get" style APIs that can defer.

> +       }
> +
>         for (i = 1; i < ARRAY_SIZE(ps_bridge->page); i++) {
>                 ps_bridge->page[i] = devm_i2c_new_dummy_device(&client->dev,
>                                                              client->adapter,

Re: [PATCH v3 2/3] drm/bridge: parade-ps8640: Use regmap APIs

[Doug Anderson]

Hi,

On Tue, Sep 14, 2021 at 5:29 PM Stephen Boyd <swboyd@chromium.org> wrote:
>
> Quoting Philip Chen (2021-09-14 16:28:44)
> > diff --git a/drivers/gpu/drm/bridge/parade-ps8640.c b/drivers/gpu/drm/bridge/parade-ps8640.c
> > index e340af381e05..8d3e7a147170 100644
> > --- a/drivers/gpu/drm/bridge/parade-ps8640.c
> > +++ b/drivers/gpu/drm/bridge/parade-ps8640.c
> > @@ -368,6 +396,12 @@ static int ps8640_probe(struct i2c_client *client)
> >
> >         ps_bridge->page[PAGE0_DP_CNTL] = client;
> >
> > +       ps_bridge->regmap[PAGE0_DP_CNTL] = devm_regmap_init_i2c(client, ps8640_regmap_config);
> > +       if (IS_ERR(ps_bridge->regmap[PAGE0_DP_CNTL])) {
> > +               return dev_err_probe(dev, PTR_ERR(ps_bridge->regmap[PAGE0_DP_CNTL]),
> > +                                    "Error initting page 0 regmap\n");
>
> This one also doesn't return -EPROBE_DEFER? The dev_err_probe() should
> really only be used on "get" style APIs that can defer.

Any reason why you say that dev_err_probe() should only be used on
"get" style APIs that can defer? Even if an API can't return
-EPROBE_DEFER, using dev_err_probe() still (IMO) makes the code
cleaner and should be used for any error cases like this during probe.
Why?

* It shows the error code in a standard way for you.
* It returns the error code you passed it so you can make your error
return "one line" instead of 2.

Is there some bad thing about dev_err_probe() that makes it
problematic to use? If not then the above advantages should be a net
win, right?

-Doug

Re: [PATCH v3 2/3] drm/bridge: parade-ps8640: Use regmap APIs

[Stephen Boyd]

Quoting Doug Anderson (2021-09-14 19:17:03)
> Hi,
>
> On Tue, Sep 14, 2021 at 5:29 PM Stephen Boyd <swboyd@chromium.org> wrote:
> >
> > Quoting Philip Chen (2021-09-14 16:28:44)
> > > diff --git a/drivers/gpu/drm/bridge/parade-ps8640.c b/drivers/gpu/drm/bridge/parade-ps8640.c
> > > index e340af381e05..8d3e7a147170 100644
> > > --- a/drivers/gpu/drm/bridge/parade-ps8640.c
> > > +++ b/drivers/gpu/drm/bridge/parade-ps8640.c
> > > @@ -368,6 +396,12 @@ static int ps8640_probe(struct i2c_client *client)
> > >
> > >         ps_bridge->page[PAGE0_DP_CNTL] = client;
> > >
> > > +       ps_bridge->regmap[PAGE0_DP_CNTL] = devm_regmap_init_i2c(client, ps8640_regmap_config);
> > > +       if (IS_ERR(ps_bridge->regmap[PAGE0_DP_CNTL])) {
> > > +               return dev_err_probe(dev, PTR_ERR(ps_bridge->regmap[PAGE0_DP_CNTL]),
> > > +                                    "Error initting page 0 regmap\n");
> >
> > This one also doesn't return -EPROBE_DEFER? The dev_err_probe() should
> > really only be used on "get" style APIs that can defer.
>
> Any reason why you say that dev_err_probe() should only be used on
> "get" style APIs that can defer? Even if an API can't return
> -EPROBE_DEFER, using dev_err_probe() still (IMO) makes the code
> cleaner and should be used for any error cases like this during probe.
> Why?
>
> * It shows the error code in a standard way for you.
> * It returns the error code you passed it so you can make your error
> return "one line" instead of 2.

I'd rather see any sort of error message in getter APIs be pushed into
the callee so that we reduce the text size of the kernel by having one
message instead of hundreds/thousands about "failure to get something".
As far as I can tell this API is designed to skip printing anything when
EPROBE_DEFER is returned, and only print something when it isn't that
particular error code. The other benefit of this API is it sets the
deferred reason in debugfs which is nice to know why some device failed
to probe. Of course now with fw_devlink that almost never triggers so
the feature is becoming useless.

>
> Is there some bad thing about dev_err_probe() that makes it
> problematic to use? If not then the above advantages should be a net
> win, right?
>

I view it as an anti-pattern. We should strive for driver probe to be
fairly simple so that it's basically getting resources and registering
with frameworks. The error messages in probe may help when you're trying
to get the driver to work and the resource APIs don't make any sense but
after that it's basically debug messages hiding as error messages.
They're never supposed to happen in practice, because the code is
tested, right?

Re: [PATCH v3 2/3] drm/bridge: parade-ps8640: Use regmap APIs

[Doug Anderson]

Hi,

On Tue, Sep 14, 2021 at 7:50 PM Stephen Boyd <swboyd@chromium.org> wrote:
>
> Quoting Doug Anderson (2021-09-14 19:17:03)
> > Hi,
> >
> > On Tue, Sep 14, 2021 at 5:29 PM Stephen Boyd <swboyd@chromium.org> wrote:
> > >
> > > Quoting Philip Chen (2021-09-14 16:28:44)
> > > > diff --git a/drivers/gpu/drm/bridge/parade-ps8640.c b/drivers/gpu/drm/bridge/parade-ps8640.c
> > > > index e340af381e05..8d3e7a147170 100644
> > > > --- a/drivers/gpu/drm/bridge/parade-ps8640.c
> > > > +++ b/drivers/gpu/drm/bridge/parade-ps8640.c
> > > > @@ -368,6 +396,12 @@ static int ps8640_probe(struct i2c_client *client)
> > > >
> > > >         ps_bridge->page[PAGE0_DP_CNTL] = client;
> > > >
> > > > +       ps_bridge->regmap[PAGE0_DP_CNTL] = devm_regmap_init_i2c(client, ps8640_regmap_config);
> > > > +       if (IS_ERR(ps_bridge->regmap[PAGE0_DP_CNTL])) {
> > > > +               return dev_err_probe(dev, PTR_ERR(ps_bridge->regmap[PAGE0_DP_CNTL]),
> > > > +                                    "Error initting page 0 regmap\n");
> > >
> > > This one also doesn't return -EPROBE_DEFER? The dev_err_probe() should
> > > really only be used on "get" style APIs that can defer.
> >
> > Any reason why you say that dev_err_probe() should only be used on
> > "get" style APIs that can defer? Even if an API can't return
> > -EPROBE_DEFER, using dev_err_probe() still (IMO) makes the code
> > cleaner and should be used for any error cases like this during probe.
> > Why?
> >
> > * It shows the error code in a standard way for you.
> > * It returns the error code you passed it so you can make your error
> > return "one line" instead of 2.
>
> I'd rather see any sort of error message in getter APIs be pushed into
> the callee so that we reduce the text size of the kernel by having one
> message instead of hundreds/thousands about "failure to get something".
> As far as I can tell this API is designed to skip printing anything when
> EPROBE_DEFER is returned, and only print something when it isn't that
> particular error code. The other benefit of this API is it sets the
> deferred reason in debugfs which is nice to know why some device failed
> to probe. Of course now with fw_devlink that almost never triggers so
> the feature is becoming useless.

I guess we need to split this apart into two issues. One (1) is
whether we should be printing errors like this in probe() and the
other (2) is the use of dev_err_probe() for cases where err could
never be -EPROBE_DEFER.

So the argument about reducing the text size for thousands of slightly
different errors is all about (1), right? In other words, you'd be
equally opposed to a change that added a normal error print with
dev_err(), right? IMO, this is a fair debate to have and it comes down
to a choice that has pros and cons. Yes the error messages are not
needed in the normal case and yes they bloat the kernel size, but when
something inevitably goes wrong then you have a way to track it down
instead of trying to guess or having to recompile the code to add
prints everywhere. Often this can give you a quick clue about a
missing Kconfig or a wrongly coded device tree file without tons of
time adding prints and recompiling code. That seems like it's worth
something...

One could also make the argument that if you don't care about all
these similar errors bloating the text segment that it would be pretty
easy to create a new Kconfig: "CONFIG_I_THINK_PROBE_ERRORS_ARE_BLOAT".
If that config is set then it could throw away the strings for every
dev_err_probe() that you compile in.


I'm not so convinced about the argument (2) that dev_err_probe()
should only be used if the error code could be -EPROBE_DEFER. Compare
these two:

Old:
  ret = do_something_that_cant_defer();
  if (ret < 0) {
    dev_err(dev, "The foo failed to bar (%pe)\n", ERR_PTR(ret));
    return ret;
  }

New:
  ret = do_something_that_cant_defer();
  if (ret < 0)
    return dev_err_probe(dev, ret, "The foo failed to bar\n");

It seems clear to me that the "New" case is better. The error code is
printed in a consistent fashion compared to all other error prints and
the fact that it returns the error code makes it cleaner. It's fine
that the error could never be -EPROBE_DEFER. Certainly we could add a
new function called dev_err_with_code() that worked exactly like
dev_err_probe() except that it didn't have special logic for
-EPROBE_DEFER but why?

Also note that the current function is dev_err_probe(), not
dev_err_might_defer(). By the name, it should be useful / OK to use
for any errors that come up in the probe path.


> > Is there some bad thing about dev_err_probe() that makes it
> > problematic to use? If not then the above advantages should be a net
> > win, right?
> >
>
> I view it as an anti-pattern. We should strive for driver probe to be
> fairly simple so that it's basically getting resources and registering
> with frameworks. The error messages in probe may help when you're trying
> to get the driver to work and the resource APIs don't make any sense but
> after that it's basically debug messages hiding as error messages.
> They're never supposed to happen in practice, because the code is
> tested, right?

IMO they happen even after initial driver bringup. You can trip error
cases from device tree problems and config problems pretty easily. It
could also be that you're bringing up an old / tested / tried and true
driver but on new hardware where some other thing (clock, regulators,
etc) is returning an error. Being able to track these down easily can
justify the error messages long term.

...or maybe what you're saying is that if it's clear that the only
case that an error could be returned is due to a driver error then we
should skip the error message? I guess, so, but only if it's somehow
built-in to the concept of the function that the only error case is a
driver error. Otherwise the function may change to check for more
errors in the future and you're back to where you started with.

In the case of devm_regmap_init_i2c(), the driver could be fine but
you might be trying to instantiate it on a system whose i2c bus lacks
the needed functionality. That's not a bug in the bridge driver but an
error in system integration. Yeah, after bringup of the new system you
probably don't need the error, but it will be useful during people's
bringups year after year.


-Doug

Re: [PATCH v3 2/3] drm/bridge: parade-ps8640: Use regmap APIs

[Stephen Boyd]

TL;DR: Please try to reduce these error messages in drivers and
consolidate them into subsystems so that drivers stay simple.

Quoting Doug Anderson (2021-09-15 09:41:39)
> Hi,
>
> On Tue, Sep 14, 2021 at 7:50 PM Stephen Boyd <swboyd@chromium.org> wrote:
> >
> >
> > I'd rather see any sort of error message in getter APIs be pushed into
> > the callee so that we reduce the text size of the kernel by having one
> > message instead of hundreds/thousands about "failure to get something".
> > As far as I can tell this API is designed to skip printing anything when
> > EPROBE_DEFER is returned, and only print something when it isn't that
> > particular error code. The other benefit of this API is it sets the
> > deferred reason in debugfs which is nice to know why some device failed
> > to probe. Of course now with fw_devlink that almost never triggers so
> > the feature is becoming useless.
>
> I guess we need to split this apart into two issues. One (1) is
> whether we should be printing errors like this in probe() and the
> other (2) is the use of dev_err_probe() for cases where err could
> never be -EPROBE_DEFER.
>
> So the argument about reducing the text size for thousands of slightly
> different errors is all about (1), right? In other words, you'd be
> equally opposed to a change that added a normal error print with
> dev_err(), right? IMO, this is a fair debate to have and it comes down
> to a choice that has pros and cons. Yes the error messages are not
> needed in the normal case and yes they bloat the kernel size, but when
> something inevitably goes wrong then you have a way to track it down
> instead of trying to guess or having to recompile the code to add
> prints everywhere. Often this can give you a quick clue about a
> missing Kconfig or a wrongly coded device tree file without tons of
> time adding prints and recompiling code. That seems like it's worth
> something...

Agreed. dev_err_probe() does that by putting that into the deferred
reason debugfs file. I'm saying that drivers shouldn't really be using
this API unless they're doing something exotic. The subsystems that are
implementing the 'get' operation that may defer should use this function
and then drivers should just return the error value to driver core so
that we can consolidate error messages and shrink the kernel size.

Maybe we can look for the defer reason in call_driver_probe() and print
a warning message if the string is set. Right now -EPROBE_DEFER is
handled but it's a dev_dbg() print that probably nobody enables and it
doesn't print the reason string.

Even better, we could make the defer reason the 'probe failed reason'
instead, and then jam the dev_err_probe() string into there regardless
of EPROBE_DEFER being returned or not. This would elevate this API to
any sort of device probe error. One more crazy idea is that we could
save the stack when the dev_err_probe() call is made and print out the
stacktrace when the error string is printed in driver core. I'm not sure
this is any better than making it a WARN_ON() though.

>
> One could also make the argument that if you don't care about all
> these similar errors bloating the text segment that it would be pretty
> easy to create a new Kconfig: "CONFIG_I_THINK_PROBE_ERRORS_ARE_BLOAT".
> If that config is set then it could throw away the strings for every
> dev_err_probe() that you compile in.

I'll leave this little CONFIG_PRINTK=n sledgehammer here.

>
>
> I'm not so convinced about the argument (2) that dev_err_probe()
> should only be used if the error code could be -EPROBE_DEFER. Compare
> these two:
>
> Old:
>   ret = do_something_that_cant_defer();
>   if (ret < 0) {
>     dev_err(dev, "The foo failed to bar (%pe)\n", ERR_PTR(ret));
>     return ret;
>   }
>
> New:
>   ret = do_something_that_cant_defer();
>   if (ret < 0)
>     return dev_err_probe(dev, ret, "The foo failed to bar\n");
>
> It seems clear to me that the "New" case is better. The error code is
> printed in a consistent fashion compared to all other error prints and
> the fact that it returns the error code makes it cleaner. It's fine
> that the error could never be -EPROBE_DEFER. Certainly we could add a
> new function called dev_err_with_code() that worked exactly like
> dev_err_probe() except that it didn't have special logic for
> -EPROBE_DEFER but why?
>
> Also note that the current function is dev_err_probe(), not
> dev_err_might_defer(). By the name, it should be useful / OK to use
> for any errors that come up in the probe path.

I looked at the documentation for dev_err_probe()

 * This helper implements common pattern present in probe functions for error
 * checking: print debug or error message depending if the error value is
 * -EPROBE_DEFER and propagate error upwards.
 * In case of -EPROBE_DEFER it sets also defer probe reason, which can be
 * checked later by reading devices_deferred debugfs attribute.

This seems to imply that it's all about EPROBE_DEFER. I'm just
reconstructing what I read from kernel-doc. If the intent is to use it
outside of probe defer, then please update the documentation to
alleviate confusion.

>
>
> > > Is there some bad thing about dev_err_probe() that makes it
> > > problematic to use? If not then the above advantages should be a net
> > > win, right?
> > >
> >
> > I view it as an anti-pattern. We should strive for driver probe to be
> > fairly simple so that it's basically getting resources and registering
> > with frameworks. The error messages in probe may help when you're trying
> > to get the driver to work and the resource APIs don't make any sense but
> > after that it's basically debug messages hiding as error messages.
> > They're never supposed to happen in practice, because the code is
> > tested, right?
>
> IMO they happen even after initial driver bringup. You can trip error
> cases from device tree problems and config problems pretty easily. It
> could also be that you're bringing up an old / tested / tried and true
> driver but on new hardware where some other thing (clock, regulators,
> etc) is returning an error. Being able to track these down easily can
> justify the error messages long term.
>
> ...or maybe what you're saying is that if it's clear that the only
> case that an error could be returned is due to a driver error then we
> should skip the error message? I guess, so, but only if it's somehow
> built-in to the concept of the function that the only error case is a
> driver error. Otherwise the function may change to check for more
> errors in the future and you're back to where you started with.

I didn't really follow this paragraph, sorry.

>
> In the case of devm_regmap_init_i2c(), the driver could be fine but
> you might be trying to instantiate it on a system whose i2c bus lacks
> the needed functionality. That's not a bug in the bridge driver but an
> error in system integration. Yeah, after bringup of the new system you
> probably don't need the error, but it will be useful during people's
> bringups year after year.
>

The point I'm trying to make is that these error messages in probe
almost never get printed after the driver is brought up on the hardware
that starts shipping out to non-kernel developers. Of course they happen
when kernel devs are enabling new hardware year after year on the same
tried and tested driver. They're worthwhile messages to have to make our
lives easier at figuring out some misconfiguration, etc. The problem is
they lead to bloat once the bringup/configuration phase is over.

At one point we directed driver authors at dev_dbg() for these prints so
that the strings would be removed from the kernel image if debugging
wasn't enabled. It looks like dev_err_probe() goes in the opposite
direction by printing an error message and passing the string to an
exported function, so dev_dbg() won't reduce the image size. Ugh!

Re: [PATCH v3 2/3] drm/bridge: parade-ps8640: Use regmap APIs

[Doug Anderson]

Hi,

On Thu, Sep 16, 2021 at 3:17 PM Stephen Boyd <swboyd@chromium.org> wrote:
>
> TL;DR: Please try to reduce these error messages in drivers and
> consolidate them into subsystems so that drivers stay simple.
>
> Quoting Doug Anderson (2021-09-15 09:41:39)
> > Hi,
> >
> > On Tue, Sep 14, 2021 at 7:50 PM Stephen Boyd <swboyd@chromium.org> wrote:
> > >
> > >
> > > I'd rather see any sort of error message in getter APIs be pushed into
> > > the callee so that we reduce the text size of the kernel by having one
> > > message instead of hundreds/thousands about "failure to get something".
> > > As far as I can tell this API is designed to skip printing anything when
> > > EPROBE_DEFER is returned, and only print something when it isn't that
> > > particular error code. The other benefit of this API is it sets the
> > > deferred reason in debugfs which is nice to know why some device failed
> > > to probe. Of course now with fw_devlink that almost never triggers so
> > > the feature is becoming useless.
> >
> > I guess we need to split this apart into two issues. One (1) is
> > whether we should be printing errors like this in probe() and the
> > other (2) is the use of dev_err_probe() for cases where err could
> > never be -EPROBE_DEFER.
> >
> > So the argument about reducing the text size for thousands of slightly
> > different errors is all about (1), right? In other words, you'd be
> > equally opposed to a change that added a normal error print with
> > dev_err(), right? IMO, this is a fair debate to have and it comes down
> > to a choice that has pros and cons. Yes the error messages are not
> > needed in the normal case and yes they bloat the kernel size, but when
> > something inevitably goes wrong then you have a way to track it down
> > instead of trying to guess or having to recompile the code to add
> > prints everywhere. Often this can give you a quick clue about a
> > missing Kconfig or a wrongly coded device tree file without tons of
> > time adding prints and recompiling code. That seems like it's worth
> > something...
>
> Agreed. dev_err_probe() does that by putting that into the deferred
> reason debugfs file. I'm saying that drivers shouldn't really be using
> this API unless they're doing something exotic. The subsystems that are
> implementing the 'get' operation that may defer should use this function
> and then drivers should just return the error value to driver core so
> that we can consolidate error messages and shrink the kernel size.
>
> Maybe we can look for the defer reason in call_driver_probe() and print
> a warning message if the string is set. Right now -EPROBE_DEFER is
> handled but it's a dev_dbg() print that probably nobody enables and it
> doesn't print the reason string.

Actually, in recent versions of the kernel it stashes the reason too.
I think there's a debugfs file "devices_deferred"


> Even better, we could make the defer reason the 'probe failed reason'
> instead, and then jam the dev_err_probe() string into there regardless
> of EPROBE_DEFER being returned or not. This would elevate this API to
> any sort of device probe error. One more crazy idea is that we could
> save the stack when the dev_err_probe() call is made and print out the
> stacktrace when the error string is printed in driver core. I'm not sure
> this is any better than making it a WARN_ON() though.
>
> >
> > One could also make the argument that if you don't care about all
> > these similar errors bloating the text segment that it would be pretty
> > easy to create a new Kconfig: "CONFIG_I_THINK_PROBE_ERRORS_ARE_BLOAT".
> > If that config is set then it could throw away the strings for every
> > dev_err_probe() that you compile in.
>
> I'll leave this little CONFIG_PRINTK=n sledgehammer here.
>
> >
> >
> > I'm not so convinced about the argument (2) that dev_err_probe()
> > should only be used if the error code could be -EPROBE_DEFER. Compare
> > these two:
> >
> > Old:
> >   ret = do_something_that_cant_defer();
> >   if (ret < 0) {
> >     dev_err(dev, "The foo failed to bar (%pe)\n", ERR_PTR(ret));
> >     return ret;
> >   }
> >
> > New:
> >   ret = do_something_that_cant_defer();
> >   if (ret < 0)
> >     return dev_err_probe(dev, ret, "The foo failed to bar\n");
> >
> > It seems clear to me that the "New" case is better. The error code is
> > printed in a consistent fashion compared to all other error prints and
> > the fact that it returns the error code makes it cleaner. It's fine
> > that the error could never be -EPROBE_DEFER. Certainly we could add a
> > new function called dev_err_with_code() that worked exactly like
> > dev_err_probe() except that it didn't have special logic for
> > -EPROBE_DEFER but why?
> >
> > Also note that the current function is dev_err_probe(), not
> > dev_err_might_defer(). By the name, it should be useful / OK to use
> > for any errors that come up in the probe path.
>
> I looked at the documentation for dev_err_probe()
>
>  * This helper implements common pattern present in probe functions for error
>  * checking: print debug or error message depending if the error value is
>  * -EPROBE_DEFER and propagate error upwards.
>  * In case of -EPROBE_DEFER it sets also defer probe reason, which can be
>  * checked later by reading devices_deferred debugfs attribute.
>
> This seems to imply that it's all about EPROBE_DEFER. I'm just
> reconstructing what I read from kernel-doc. If the intent is to use it
> outside of probe defer, then please update the documentation to
> alleviate confusion.

Meh. Yeah, it talks a lot about -EPROBE_DEFER, but it doesn't say it's
only for that.

Sure, I'll post a patch.

https://lore.kernel.org/r/20210916161931.1.I32bea713bd6c6fb419a24da73686145742b6c117@changeid


> > In the case of devm_regmap_init_i2c(), the driver could be fine but
> > you might be trying to instantiate it on a system whose i2c bus lacks
> > the needed functionality. That's not a bug in the bridge driver but an
> > error in system integration. Yeah, after bringup of the new system you
> > probably don't need the error, but it will be useful during people's
> > bringups year after year.
> >
>
> The point I'm trying to make is that these error messages in probe
> almost never get printed after the driver is brought up on the hardware
> that starts shipping out to non-kernel developers. Of course they happen
> when kernel devs are enabling new hardware year after year on the same
> tried and tested driver. They're worthwhile messages to have to make our
> lives easier at figuring out some misconfiguration, etc. The problem is
> they lead to bloat once the bringup/configuration phase is over.
>
> At one point we directed driver authors at dev_dbg() for these prints so
> that the strings would be removed from the kernel image if debugging
> wasn't enabled. It looks like dev_err_probe() goes in the opposite
> direction by printing an error message and passing the string to an
> exported function, so dev_dbg() won't reduce the image size. Ugh!

So maybe the key here is that "CONFIG_PRINTK=n" is not the same as
"CONFIG_I_THINK_PROBE_ERRORS_ARE_BLOAT" and it's not just that one has
a more flippant name than the other. I think your argument about the
fact that these errors almost never come up in practice is actually
true for pretty much _all_ probe errors, isn't it? So if you wanted to
keep non-probe errors in your system (keep PRINTK=y) and just do away
with these bloat-y probe errors then dev_err_probe() could really be
the key and there'd be a big benefit for using for all these errors
during probe, not just ones that have a chance of deferring. ...and
yes, you could make this config do something fancy like do a stack
dump or print the return address if you actually hit one of these
errors once you've thrown away the string.

I also wouldn't necessarily agree that dev_dbg() was an amazing fit
for these error messages. They truly were error-level things that were
happening. These are things that are causing the probe to abort, not
just extra spammy debug info. Calling them "error" messages rather
than "debug" messages seems better...


-Doug

Re: [PATCH v3 2/3] drm/bridge: parade-ps8640: Use regmap APIs

[Stephen Boyd]

Quoting Doug Anderson (2021-09-16 16:21:12)
> Hi,
>
> On Thu, Sep 16, 2021 at 3:17 PM Stephen Boyd <swboyd@chromium.org> wrote:
> >
> > TL;DR: Please try to reduce these error messages in drivers and
> > consolidate them into subsystems so that drivers stay simple.
> >
> > Quoting Doug Anderson (2021-09-15 09:41:39)
> > > Hi,
> > >
> > > On Tue, Sep 14, 2021 at 7:50 PM Stephen Boyd <swboyd@chromium.org> wrote:
> > > >
> > > >
> > > > I'd rather see any sort of error message in getter APIs be pushed into
> > > > the callee so that we reduce the text size of the kernel by having one
> > > > message instead of hundreds/thousands about "failure to get something".
> > > > As far as I can tell this API is designed to skip printing anything when
> > > > EPROBE_DEFER is returned, and only print something when it isn't that
> > > > particular error code. The other benefit of this API is it sets the
> > > > deferred reason in debugfs which is nice to know why some device failed
> > > > to probe. Of course now with fw_devlink that almost never triggers so
> > > > the feature is becoming useless.
> > >
> > > I guess we need to split this apart into two issues. One (1) is
> > > whether we should be printing errors like this in probe() and the
> > > other (2) is the use of dev_err_probe() for cases where err could
> > > never be -EPROBE_DEFER.
> > >
> > > So the argument about reducing the text size for thousands of slightly
> > > different errors is all about (1), right? In other words, you'd be
> > > equally opposed to a change that added a normal error print with
> > > dev_err(), right? IMO, this is a fair debate to have and it comes down
> > > to a choice that has pros and cons. Yes the error messages are not
> > > needed in the normal case and yes they bloat the kernel size, but when
> > > something inevitably goes wrong then you have a way to track it down
> > > instead of trying to guess or having to recompile the code to add
> > > prints everywhere. Often this can give you a quick clue about a
> > > missing Kconfig or a wrongly coded device tree file without tons of
> > > time adding prints and recompiling code. That seems like it's worth
> > > something...
> >
> > Agreed. dev_err_probe() does that by putting that into the deferred
> > reason debugfs file. I'm saying that drivers shouldn't really be using
> > this API unless they're doing something exotic. The subsystems that are
> > implementing the 'get' operation that may defer should use this function
> > and then drivers should just return the error value to driver core so
> > that we can consolidate error messages and shrink the kernel size.
> >
> > Maybe we can look for the defer reason in call_driver_probe() and print
> > a warning message if the string is set. Right now -EPROBE_DEFER is
> > handled but it's a dev_dbg() print that probably nobody enables and it
> > doesn't print the reason string.
>
> Actually, in recent versions of the kernel it stashes the reason too.
> I think there's a debugfs file "devices_deferred"

Yep that's what I meant by "putting that into the deferred reason
debugfs file" above.

> > This seems to imply that it's all about EPROBE_DEFER. I'm just
> > reconstructing what I read from kernel-doc. If the intent is to use it
> > outside of probe defer, then please update the documentation to
> > alleviate confusion.
>
> Meh. Yeah, it talks a lot about -EPROBE_DEFER, but it doesn't say it's
> only for that.
>
> Sure, I'll post a patch.
>
> https://lore.kernel.org/r/20210916161931.1.I32bea713bd6c6fb419a24da73686145742b6c117@changeid

Cool thanks.

>
>
> > > In the case of devm_regmap_init_i2c(), the driver could be fine but
> > > you might be trying to instantiate it on a system whose i2c bus lacks
> > > the needed functionality. That's not a bug in the bridge driver but an
> > > error in system integration. Yeah, after bringup of the new system you
> > > probably don't need the error, but it will be useful during people's
> > > bringups year after year.
> > >
> >
> > The point I'm trying to make is that these error messages in probe
> > almost never get printed after the driver is brought up on the hardware
> > that starts shipping out to non-kernel developers. Of course they happen
> > when kernel devs are enabling new hardware year after year on the same
> > tried and tested driver. They're worthwhile messages to have to make our
> > lives easier at figuring out some misconfiguration, etc. The problem is
> > they lead to bloat once the bringup/configuration phase is over.
> >
> > At one point we directed driver authors at dev_dbg() for these prints so
> > that the strings would be removed from the kernel image if debugging
> > wasn't enabled. It looks like dev_err_probe() goes in the opposite
> > direction by printing an error message and passing the string to an
> > exported function, so dev_dbg() won't reduce the image size. Ugh!
>
> So maybe the key here is that "CONFIG_PRINTK=n" is not the same as
> "CONFIG_I_THINK_PROBE_ERRORS_ARE_BLOAT" and it's not just that one has
> a more flippant name than the other. I think your argument about the
> fact that these errors almost never come up in practice is actually
> true for pretty much _all_ probe errors, isn't it? So if you wanted to
> keep non-probe errors in your system (keep PRINTK=y) and just do away
> with these bloat-y probe errors then dev_err_probe() could really be
> the key and there'd be a big benefit for using for all these errors
> during probe, not just ones that have a chance of deferring. ...and
> yes, you could make this config do something fancy like do a stack
> dump or print the return address if you actually hit one of these
> errors once you've thrown away the string.

Yes, but it's also just as important to push similar messages, i.e. "I
failed to get some resource", into the API that hands resources out so
that bloat is minimized further and drivers are kept simple.

>
> I also wouldn't necessarily agree that dev_dbg() was an amazing fit
> for these error messages. They truly were error-level things that were
> happening. These are things that are causing the probe to abort, not
> just extra spammy debug info. Calling them "error" messages rather
> than "debug" messages seems better...
>

Agreed. When all we had was dev_dbg() it was the best option to getting
rid of these types of driver development printks.

Re: [PATCH v3 2/3] drm/bridge: parade-ps8640: Use regmap APIs

[Doug Anderson]

Hi,

On Thu, Sep 16, 2021 at 11:12 PM Stephen Boyd <swboyd@chromium.org> wrote:
>
> > > > In the case of devm_regmap_init_i2c(), the driver could be fine but
> > > > you might be trying to instantiate it on a system whose i2c bus lacks
> > > > the needed functionality. That's not a bug in the bridge driver but an
> > > > error in system integration. Yeah, after bringup of the new system you
> > > > probably don't need the error, but it will be useful during people's
> > > > bringups year after year.
> > > >
> > >
> > > The point I'm trying to make is that these error messages in probe
> > > almost never get printed after the driver is brought up on the hardware
> > > that starts shipping out to non-kernel developers. Of course they happen
> > > when kernel devs are enabling new hardware year after year on the same
> > > tried and tested driver. They're worthwhile messages to have to make our
> > > lives easier at figuring out some misconfiguration, etc. The problem is
> > > they lead to bloat once the bringup/configuration phase is over.
> > >
> > > At one point we directed driver authors at dev_dbg() for these prints so
> > > that the strings would be removed from the kernel image if debugging
> > > wasn't enabled. It looks like dev_err_probe() goes in the opposite
> > > direction by printing an error message and passing the string to an
> > > exported function, so dev_dbg() won't reduce the image size. Ugh!
> >
> > So maybe the key here is that "CONFIG_PRINTK=n" is not the same as
> > "CONFIG_I_THINK_PROBE_ERRORS_ARE_BLOAT" and it's not just that one has
> > a more flippant name than the other. I think your argument about the
> > fact that these errors almost never come up in practice is actually
> > true for pretty much _all_ probe errors, isn't it? So if you wanted to
> > keep non-probe errors in your system (keep PRINTK=y) and just do away
> > with these bloat-y probe errors then dev_err_probe() could really be
> > the key and there'd be a big benefit for using for all these errors
> > during probe, not just ones that have a chance of deferring. ...and
> > yes, you could make this config do something fancy like do a stack
> > dump or print the return address if you actually hit one of these
> > errors once you've thrown away the string.
>
> Yes, but it's also just as important to push similar messages, i.e. "I
> failed to get some resource", into the API that hands resources out so
> that bloat is minimized further and drivers are kept simple.

Sure, but this is a slippery slope. If there's any chance that a
caller might want to know about the error but _not_ want the error
message printed then you can't push the error message into the API.
It's really hard to find error cases (even with "get resource" type
functions) where the caller _always_ wants the error reported. Even
kmalloc() has a nod to this with __GFP_NOWARN, though I'm not
advocating adding a "no warn" flag to all APIs. It's always possible
that the caller is expecting some types of errors and handles the case
elegantly.

Let's pop all the way back up to the original point here, which was
about devm_regmap_init_i2c(). What should happen with errors? Let's
look specifically at the errors that could be returned by
regmap_get_i2c_bus() which is the first thing devm_regmap_init_i2c()
tries to do. Those errors have to do with the i2c bus not supporting
the features needed by our regmap.


a) We could return the error without printing anything like the code does today.

No bloat, but during bringup of this bridge chip on a new i2c bus we'd
have to manually add printouts to the probe function to figure out
this error.


b) We could push error reporting into regmap_get_i2c_bus().

No per-driver bloat, but some drivers might have a legitimate reason
not to have an error print here. Perhaps they have a fallback `regmap`
config that they want to be able to use that works with different bus
capabilities. I don't think we can do this.


c) We could use dev_dbg() to print the error

Only bloat if dynamic debug or DEBUG is defined


d) We could use dev_err_probe() to print the error

Extra bloat, though it could be minimized (without sacrificing all
"printk") with a future patch to drop the string from dev_err_probe()
and perhaps replace it with a WARN_ON(). Also handles the fact that
perhaps someday someone might find a reason that regmap_get_i2c_bus()
and/or devm_regmap_init_i2c() should suddenly start returning
-EPROBE_DEFER.


I'm still advocating for "d)" above and I believe you originally
advocated for "a)" or "c)". It's really not such a huge deal, so if
you're adamant about "a)" then I'll shut up. I'm curious if I've
managed to convince you all about "d)" though.

-Doug

Re: [PATCH v3 2/3] drm/bridge: parade-ps8640: Use regmap APIs

[Philip Chen]

Hi Doug and Stephen,

Thanks for the review.
Before we reach a consensus on the best logging option, I'll just
remove the printouts from this patch and just return PTR_ERR.
Once we reach a consensus, we can probably improve logging in a separate patch.

On Fri, Sep 17, 2021 at 8:02 AM Doug Anderson <dianders@chromium.org> wrote:
>
> Hi,
>
> On Thu, Sep 16, 2021 at 11:12 PM Stephen Boyd <swboyd@chromium.org> wrote:
> >
> > > > > In the case of devm_regmap_init_i2c(), the driver could be fine but
> > > > > you might be trying to instantiate it on a system whose i2c bus lacks
> > > > > the needed functionality. That's not a bug in the bridge driver but an
> > > > > error in system integration. Yeah, after bringup of the new system you
> > > > > probably don't need the error, but it will be useful during people's
> > > > > bringups year after year.
> > > > >
> > > >
> > > > The point I'm trying to make is that these error messages in probe
> > > > almost never get printed after the driver is brought up on the hardware
> > > > that starts shipping out to non-kernel developers. Of course they happen
> > > > when kernel devs are enabling new hardware year after year on the same
> > > > tried and tested driver. They're worthwhile messages to have to make our
> > > > lives easier at figuring out some misconfiguration, etc. The problem is
> > > > they lead to bloat once the bringup/configuration phase is over.
> > > >
> > > > At one point we directed driver authors at dev_dbg() for these prints so
> > > > that the strings would be removed from the kernel image if debugging
> > > > wasn't enabled. It looks like dev_err_probe() goes in the opposite
> > > > direction by printing an error message and passing the string to an
> > > > exported function, so dev_dbg() won't reduce the image size. Ugh!
> > >
> > > So maybe the key here is that "CONFIG_PRINTK=n" is not the same as
> > > "CONFIG_I_THINK_PROBE_ERRORS_ARE_BLOAT" and it's not just that one has
> > > a more flippant name than the other. I think your argument about the
> > > fact that these errors almost never come up in practice is actually
> > > true for pretty much _all_ probe errors, isn't it? So if you wanted to
> > > keep non-probe errors in your system (keep PRINTK=y) and just do away
> > > with these bloat-y probe errors then dev_err_probe() could really be
> > > the key and there'd be a big benefit for using for all these errors
> > > during probe, not just ones that have a chance of deferring. ...and
> > > yes, you could make this config do something fancy like do a stack
> > > dump or print the return address if you actually hit one of these
> > > errors once you've thrown away the string.
> >
> > Yes, but it's also just as important to push similar messages, i.e. "I
> > failed to get some resource", into the API that hands resources out so
> > that bloat is minimized further and drivers are kept simple.
>
> Sure, but this is a slippery slope. If there's any chance that a
> caller might want to know about the error but _not_ want the error
> message printed then you can't push the error message into the API.
> It's really hard to find error cases (even with "get resource" type
> functions) where the caller _always_ wants the error reported. Even
> kmalloc() has a nod to this with __GFP_NOWARN, though I'm not
> advocating adding a "no warn" flag to all APIs. It's always possible
> that the caller is expecting some types of errors and handles the case
> elegantly.
>
> Let's pop all the way back up to the original point here, which was
> about devm_regmap_init_i2c(). What should happen with errors? Let's
> look specifically at the errors that could be returned by
> regmap_get_i2c_bus() which is the first thing devm_regmap_init_i2c()
> tries to do. Those errors have to do with the i2c bus not supporting
> the features needed by our regmap.
>
>
> a) We could return the error without printing anything like the code does today.
>
> No bloat, but during bringup of this bridge chip on a new i2c bus we'd
> have to manually add printouts to the probe function to figure out
> this error.
>
>
> b) We could push error reporting into regmap_get_i2c_bus().
>
> No per-driver bloat, but some drivers might have a legitimate reason
> not to have an error print here. Perhaps they have a fallback `regmap`
> config that they want to be able to use that works with different bus
> capabilities. I don't think we can do this.
>
>
> c) We could use dev_dbg() to print the error
>
> Only bloat if dynamic debug or DEBUG is defined
>
>
> d) We could use dev_err_probe() to print the error
>
> Extra bloat, though it could be minimized (without sacrificing all
> "printk") with a future patch to drop the string from dev_err_probe()
> and perhaps replace it with a WARN_ON(). Also handles the fact that
> perhaps someday someone might find a reason that regmap_get_i2c_bus()
> and/or devm_regmap_init_i2c() should suddenly start returning
> -EPROBE_DEFER.
>
>
> I'm still advocating for "d)" above and I believe you originally
> advocated for "a)" or "c)". It's really not such a huge deal, so if
> you're adamant about "a)" then I'll shut up. I'm curious if I've
> managed to convince you all about "d)" though.
>
> -Doug

[PATCH v3 3/3] drm/bridge: parade-ps8640: Add support for AUX channel

[Philip Chen]

Implement the first version of AUX support, which will be useful as
we expand the driver to support varied use cases.

Signed-off-by: Philip Chen <philipchen@chromium.org>
---

Changes in v3:
- Verify with HW and thus remove WARNING from the patch description
- Fix the reg names to better match the manual
- Fix aux_transfer function:
  - Fix the switch statement which handles aux request
  - Write the original (unstripped) aux request code to the register
  - Replace DRM_ERROR with dev_err
  - Remove goto and just return ret
  - Fix the switch statement which handles aux status
  - When reading returned data, read from RDATA instead of WDATA
- Fix attach function:
  - Call mipi_dsi_detach() when aux_register fails

Changes in v2:
- Handle the case where an AUX transaction has no payload
- Add a reg polling for p0.0x83 to confirm AUX cmd is issued and
  read data is returned
- Replace regmap_noinc_read/write with looped regmap_read/write,
  as regmap_noinc_read/write doesn't read one byte at a time unless
  max_raw_read/write is set to 1.
- Register/Unregister the AUX device explicitly when the bridge is
  attached/detached
- Remove the use of runtime PM
- Program AUX addr/cmd/len in a single regmap_bulk_write()
- Add newlines for DRM_ERROR mesages

 drivers/gpu/drm/bridge/parade-ps8640.c | 174 ++++++++++++++++++++++++-
 1 file changed, 173 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/bridge/parade-ps8640.c b/drivers/gpu/drm/bridge/parade-ps8640.c
index 8d3e7a147170..dc349d729f5a 100644
--- a/drivers/gpu/drm/bridge/parade-ps8640.c
+++ b/drivers/gpu/drm/bridge/parade-ps8640.c
@@ -13,11 +13,37 @@
 #include <linux/regulator/consumer.h>
 
 #include <drm/drm_bridge.h>
+#include <drm/drm_dp_helper.h>
 #include <drm/drm_mipi_dsi.h>
 #include <drm/drm_of.h>
 #include <drm/drm_panel.h>
 #include <drm/drm_print.h>
 
+#define PAGE0_AUXCH_CFG3	0x76
+#define  AUXCH_CFG3_RESET	0xff
+#define PAGE0_SWAUX_ADDR_7_0	0x7d
+#define PAGE0_SWAUX_ADDR_15_8	0x7e
+#define PAGE0_SWAUX_ADDR_23_16	0x7f
+#define  SWAUX_ADDR_19_16_MASK	GENMASK(3, 0)
+#define  SWAUX_CMD_MASK		GENMASK(7, 4)
+#define PAGE0_SWAUX_LENGTH	0x80
+#define  SWAUX_LENGTH_MASK	GENMASK(3, 0)
+#define  SWAUX_NO_PAYLOAD	BIT(7)
+#define PAGE0_SWAUX_WDATA	0x81
+#define PAGE0_SWAUX_RDATA	0x82
+#define PAGE0_SWAUX_CTRL	0x83
+#define  SWAUX_SEND		BIT(0)
+#define PAGE0_SWAUX_STATUS	0x84
+#define  SWAUX_M_MASK		GENMASK(4, 0)
+#define  SWAUX_STATUS_MASK	GENMASK(7, 5)
+#define  SWAUX_STATUS_NACK	(0x1 << 5)
+#define  SWAUX_STATUS_DEFER	(0x2 << 5)
+#define  SWAUX_STATUS_ACKM	(0x3 << 5)
+#define  SWAUX_STATUS_INVALID	(0x4 << 5)
+#define  SWAUX_STATUS_I2C_NACK	(0x5 << 5)
+#define  SWAUX_STATUS_I2C_DEFER	(0x6 << 5)
+#define  SWAUX_STATUS_TIMEOUT	(0x7 << 5)
+
 #define PAGE2_GPIO_H		0xa7
 #define  PS_GPIO9		BIT(1)
 #define PAGE2_I2C_BYPASS	0xea
@@ -68,6 +94,7 @@ enum ps8640_vdo_control {
 struct ps8640 {
 	struct drm_bridge bridge;
 	struct drm_bridge *panel_bridge;
+	struct drm_dp_aux aux;
 	struct mipi_dsi_device *dsi;
 	struct i2c_client *page[MAX_DEVS];
 	struct regmap	*regmap[MAX_DEVS];
@@ -117,6 +144,129 @@ static inline struct ps8640 *bridge_to_ps8640(struct drm_bridge *e)
 	return container_of(e, struct ps8640, bridge);
 }
 
+static inline struct ps8640 *aux_to_ps8640(struct drm_dp_aux *aux)
+{
+	return container_of(aux, struct ps8640, aux);
+}
+
+static ssize_t ps8640_aux_transfer(struct drm_dp_aux *aux,
+				   struct drm_dp_aux_msg *msg)
+{
+	struct ps8640 *ps_bridge = aux_to_ps8640(aux);
+	struct regmap *map = ps_bridge->regmap[PAGE0_DP_CNTL];
+	struct device *dev = &ps_bridge->page[PAGE0_DP_CNTL]->dev;
+
+	unsigned int len = msg->size;
+	unsigned int data;
+	int ret;
+	u8 request = msg->request &
+		     ~(DP_AUX_I2C_MOT | DP_AUX_I2C_WRITE_STATUS_UPDATE);
+	u8 *buf = msg->buffer;
+	u8 addr_len[PAGE0_SWAUX_LENGTH + 1 - PAGE0_SWAUX_ADDR_7_0];
+	u8 i;
+	bool is_native_aux = false;
+
+	if (len > DP_AUX_MAX_PAYLOAD_BYTES)
+		return -EINVAL;
+
+	switch (request) {
+	case DP_AUX_NATIVE_WRITE:
+	case DP_AUX_NATIVE_READ:
+		is_native_aux = true;
+		fallthrough;
+	case DP_AUX_I2C_WRITE:
+	case DP_AUX_I2C_READ:
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	ret = regmap_write(map, PAGE0_AUXCH_CFG3, AUXCH_CFG3_RESET);
+	if (ret) {
+		dev_err(dev, "failed to write PAGE0_AUXCH_CFG3: %d\n", ret);
+		return ret;
+	}
+
+	/* Assume it's good */
+	msg->reply = 0;
+
+	addr_len[0] = msg->address & 0xff;
+	addr_len[1] = (msg->address >> 8) & 0xff;
+	addr_len[2] = ((msg->request << 4) & SWAUX_CMD_MASK) |
+		((msg->address >> 16) & SWAUX_ADDR_19_16_MASK);
+	addr_len[3] = (len == 0) ? SWAUX_NO_PAYLOAD :
+			((len - 1) & SWAUX_LENGTH_MASK);
+
+	regmap_bulk_write(map, PAGE0_SWAUX_ADDR_7_0, addr_len,
+			  ARRAY_SIZE(addr_len));
+
+	if (len && (request == DP_AUX_NATIVE_WRITE ||
+		    request == DP_AUX_I2C_WRITE)) {
+		/* Write to the internal FIFO buffer */
+		for (i = 0; i < len; i++) {
+			ret = regmap_write(map, PAGE0_SWAUX_WDATA, buf[i]);
+			if (ret) {
+				dev_err(dev, "failed to write WDATA: %d\n",
+					ret);
+				return ret;
+			}
+		}
+	}
+
+	regmap_write(map, PAGE0_SWAUX_CTRL, SWAUX_SEND);
+
+	/* Zero delay loop because i2c transactions are slow already */
+	regmap_read_poll_timeout(map, PAGE0_SWAUX_CTRL, data,
+				 !(data & SWAUX_SEND), 0, 50 * 1000);
+
+	regmap_read(map, PAGE0_SWAUX_STATUS, &data);
+	if (ret) {
+		dev_err(dev, "failed to read PAGE0_SWAUX_STATUS: %d\n", ret);
+		return ret;
+	}
+
+	switch (data & SWAUX_STATUS_MASK) {
+	/* Ignore the DEFER cases as they are already handled in hardware */
+	case SWAUX_STATUS_NACK:
+	case SWAUX_STATUS_I2C_NACK:
+		/*
+		 * The programming guide is not clear about whether a I2C NACK
+		 * would trigger SWAUX_STATUS_NACK or SWAUX_STATUS_I2C_NACK. So
+		 * we handle both cases together.
+		 */
+		if (is_native_aux)
+			msg->reply |= DP_AUX_NATIVE_REPLY_NACK;
+		else
+			msg->reply |= DP_AUX_I2C_REPLY_NACK;
+
+		len = data & SWAUX_M_MASK;
+		return len;
+	case SWAUX_STATUS_ACKM:
+		len = data & SWAUX_M_MASK;
+		return len;
+	case SWAUX_STATUS_INVALID:
+		return -EOPNOTSUPP;
+	case SWAUX_STATUS_TIMEOUT:
+		return -ETIMEDOUT;
+	}
+
+	if (len && (request == DP_AUX_NATIVE_READ ||
+		    request == DP_AUX_I2C_READ)) {
+		/* Read from the internal FIFO buffer */
+		for (i = 0; i < len; i++) {
+			ret = regmap_read(map, PAGE0_SWAUX_RDATA, &data);
+			buf[i] = data;
+			if (ret) {
+				dev_err(dev, "failed to read RDATA: %d\n",
+					ret);
+				return ret;
+			}
+		}
+	}
+
+	return len;
+}
+
 static int ps8640_bridge_vdo_control(struct ps8640 *ps_bridge,
 				     const enum ps8640_vdo_control ctrl)
 {
@@ -286,18 +436,34 @@ static int ps8640_bridge_attach(struct drm_bridge *bridge,
 	dsi->format = MIPI_DSI_FMT_RGB888;
 	dsi->lanes = NUM_MIPI_LANES;
 	ret = mipi_dsi_attach(dsi);
-	if (ret)
+	if (ret) {
+		dev_err(dev, "failed to attach dsi device: %d\n", ret);
 		goto err_dsi_attach;
+	}
+
+	ret = drm_dp_aux_register(&ps_bridge->aux);
+	if (ret) {
+		dev_err(dev, "failed to register DP AUX channel: %d\n", ret);
+		goto err_aux_register;
+	}
 
 	/* Attach the panel-bridge to the dsi bridge */
 	return drm_bridge_attach(bridge->encoder, ps_bridge->panel_bridge,
 				 &ps_bridge->bridge, flags);
 
+err_aux_register:
+	mipi_dsi_detach(dsi);
 err_dsi_attach:
 	mipi_dsi_device_unregister(dsi);
 	return ret;
 }
 
+
+static void ps8640_bridge_detach(struct drm_bridge *bridge)
+{
+	drm_dp_aux_unregister(&bridge_to_ps8640(bridge)->aux);
+}
+
 static struct edid *ps8640_bridge_get_edid(struct drm_bridge *bridge,
 					   struct drm_connector *connector)
 {
@@ -334,6 +500,7 @@ static struct edid *ps8640_bridge_get_edid(struct drm_bridge *bridge,
 
 static const struct drm_bridge_funcs ps8640_bridge_funcs = {
 	.attach = ps8640_bridge_attach,
+	.detach = ps8640_bridge_detach,
 	.get_edid = ps8640_bridge_get_edid,
 	.post_disable = ps8640_post_disable,
 	.pre_enable = ps8640_pre_enable,
@@ -423,6 +590,11 @@ static int ps8640_probe(struct i2c_client *client)
 
 	i2c_set_clientdata(client, ps_bridge);
 
+	ps_bridge->aux.name = "parade-ps8640-aux";
+	ps_bridge->aux.dev = dev;
+	ps_bridge->aux.transfer = ps8640_aux_transfer;
+	drm_dp_aux_init(&ps_bridge->aux);
+
 	drm_bridge_add(&ps_bridge->bridge);
 
 	return 0;
-- 
2.33.0.309.g3052b89438-goog

Re: [PATCH v3 3/3] drm/bridge: parade-ps8640: Add support for AUX channel

[Stephen Boyd]

Quoting Philip Chen (2021-09-14 16:28:45)
> diff --git a/drivers/gpu/drm/bridge/parade-ps8640.c b/drivers/gpu/drm/bridge/parade-ps8640.c
> index 8d3e7a147170..dc349d729f5a 100644
> --- a/drivers/gpu/drm/bridge/parade-ps8640.c
> +++ b/drivers/gpu/drm/bridge/parade-ps8640.c
> @@ -117,6 +144,129 @@ static inline struct ps8640 *bridge_to_ps8640(struct drm_bridge *e)
[...]
> +       case DP_AUX_I2C_WRITE:
> +       case DP_AUX_I2C_READ:
> +               break;
> +       default:
> +               return -EINVAL;
> +       }
> +
> +       ret = regmap_write(map, PAGE0_AUXCH_CFG3, AUXCH_CFG3_RESET);
> +       if (ret) {
> +               dev_err(dev, "failed to write PAGE0_AUXCH_CFG3: %d\n", ret);

Can we use DRM_DEV_ERROR()?

> +               return ret;
> +       }
> +
> +       /* Assume it's good */
> +       msg->reply = 0;
> +
> +       addr_len[0] = msg->address & 0xff;
> +       addr_len[1] = (msg->address >> 8) & 0xff;
> +       addr_len[2] = ((msg->request << 4) & SWAUX_CMD_MASK) |
> +               ((msg->address >> 16) & SWAUX_ADDR_19_16_MASK);

It really feels like this out to be possible with some sort of
cpu_to_le32() API. We're shoving msg->address into 3 bytes and then
adding in the request and some length. So we could do something like:

	u32 addr_len;

	addr_len = FIELD_PREP(SWAUX_ADDR_MASK, msg->address);
	addr_len |= FIELD_PREP(SWAUX_CMD_MASK, msg->request);
	if (len)
		addr_len |= FIELD_PREP(LEN_MASK, len - 1);
	else
		addr_len |= FIELD_PREP(LEN_MASK, SWAUX_NO_PAYLOAD );
	
	cpu_to_le32s(&addr_len);

	regmap_bulk_write(map, PAGE0_SWAUX_ADDR_7_0, &addr_len, sizeof(addr_len));

> +       addr_len[3] = (len == 0) ? SWAUX_NO_PAYLOAD :
> +                       ((len - 1) & SWAUX_LENGTH_MASK);
> +
> +       regmap_bulk_write(map, PAGE0_SWAUX_ADDR_7_0, addr_len,
> +                         ARRAY_SIZE(addr_len));
> +
> +       if (len && (request == DP_AUX_NATIVE_WRITE ||
> +                   request == DP_AUX_I2C_WRITE)) {
> +               /* Write to the internal FIFO buffer */
> +               for (i = 0; i < len; i++) {
> +                       ret = regmap_write(map, PAGE0_SWAUX_WDATA, buf[i]);
> +                       if (ret) {
> +                               dev_err(dev, "failed to write WDATA: %d\n",

DRM_DEV_ERROR?

> +                                       ret);
> +                               return ret;
> +                       }
> +               }
> +       }
> +
> +       regmap_write(map, PAGE0_SWAUX_CTRL, SWAUX_SEND);
> +
> +       /* Zero delay loop because i2c transactions are slow already */
> +       regmap_read_poll_timeout(map, PAGE0_SWAUX_CTRL, data,
> +                                !(data & SWAUX_SEND), 0, 50 * 1000);
> +
> +       regmap_read(map, PAGE0_SWAUX_STATUS, &data);
> +       if (ret) {
> +               dev_err(dev, "failed to read PAGE0_SWAUX_STATUS: %d\n", ret);

DRM_DEV_ERROR?

> +               return ret;
> +       }
> +
> +       switch (data & SWAUX_STATUS_MASK) {
> +       /* Ignore the DEFER cases as they are already handled in hardware */
> +       case SWAUX_STATUS_NACK:
> +       case SWAUX_STATUS_I2C_NACK:
> +               /*
> +                * The programming guide is not clear about whether a I2C NACK
> +                * would trigger SWAUX_STATUS_NACK or SWAUX_STATUS_I2C_NACK. So
> +                * we handle both cases together.
> +                */
> +               if (is_native_aux)
> +                       msg->reply |= DP_AUX_NATIVE_REPLY_NACK;
> +               else
> +                       msg->reply |= DP_AUX_I2C_REPLY_NACK;
> +
> +               len = data & SWAUX_M_MASK;
> +               return len;

Why no 'return data & SWAUX_M_MASK;' and skip the assignment?

> +       case SWAUX_STATUS_ACKM:

Move this up and add fallthrough?

> +               len = data & SWAUX_M_MASK;
> +               return len;
> +       case SWAUX_STATUS_INVALID:
> +               return -EOPNOTSUPP;
> +       case SWAUX_STATUS_TIMEOUT:
> +               return -ETIMEDOUT;
> +       }
> +
> +       if (len && (request == DP_AUX_NATIVE_READ ||
> +                   request == DP_AUX_I2C_READ)) {
> +               /* Read from the internal FIFO buffer */
> +               for (i = 0; i < len; i++) {
> +                       ret = regmap_read(map, PAGE0_SWAUX_RDATA, &data);
> +                       buf[i] = data;

Can drop a line

		ret = regmap_read(map, PAGE0_SWAUX_RDATA, buf + i);

> +                       if (ret) {
> +                               dev_err(dev, "failed to read RDATA: %d\n",
> +                                       ret);
> +                               return ret;
> +                       }
> +               }
> +       }
> +
> +       return len;
> +}

Re: [PATCH v3 3/3] drm/bridge: parade-ps8640: Add support for AUX channel

[Philip Chen]

Hi

On Tue, Sep 14, 2021 at 5:57 PM Stephen Boyd <swboyd@chromium.org> wrote:
>
> Quoting Philip Chen (2021-09-14 16:28:45)
> > diff --git a/drivers/gpu/drm/bridge/parade-ps8640.c b/drivers/gpu/drm/bridge/parade-ps8640.c
> > index 8d3e7a147170..dc349d729f5a 100644
> > --- a/drivers/gpu/drm/bridge/parade-ps8640.c
> > +++ b/drivers/gpu/drm/bridge/parade-ps8640.c
> > @@ -117,6 +144,129 @@ static inline struct ps8640 *bridge_to_ps8640(struct drm_bridge *e)
> [...]
> > +       case DP_AUX_I2C_WRITE:
> > +       case DP_AUX_I2C_READ:
> > +               break;
> > +       default:
> > +               return -EINVAL;
> > +       }
> > +
> > +       ret = regmap_write(map, PAGE0_AUXCH_CFG3, AUXCH_CFG3_RESET);
> > +       if (ret) {
> > +               dev_err(dev, "failed to write PAGE0_AUXCH_CFG3: %d\n", ret);
>
> Can we use DRM_DEV_ERROR()?
Sure.
>
> > +               return ret;
> > +       }
> > +
> > +       /* Assume it's good */
> > +       msg->reply = 0;
> > +
> > +       addr_len[0] = msg->address & 0xff;
> > +       addr_len[1] = (msg->address >> 8) & 0xff;
> > +       addr_len[2] = ((msg->request << 4) & SWAUX_CMD_MASK) |
> > +               ((msg->address >> 16) & SWAUX_ADDR_19_16_MASK);
>
> It really feels like this out to be possible with some sort of
> cpu_to_le32() API. We're shoving msg->address into 3 bytes and then
> adding in the request and some length. So we could do something like:
>
>         u32 addr_len;
>
>         addr_len = FIELD_PREP(SWAUX_ADDR_MASK, msg->address);
>         addr_len |= FIELD_PREP(SWAUX_CMD_MASK, msg->request);
>         if (len)
>                 addr_len |= FIELD_PREP(LEN_MASK, len - 1);
>         else
>                 addr_len |= FIELD_PREP(LEN_MASK, SWAUX_NO_PAYLOAD );
>
>         cpu_to_le32s(&addr_len);
>
>         regmap_bulk_write(map, PAGE0_SWAUX_ADDR_7_0, &addr_len, sizeof(addr_len));
>
Yes, thanks for the advice.
Will add this change to v4.

> > +       addr_len[3] = (len == 0) ? SWAUX_NO_PAYLOAD :
> > +                       ((len - 1) & SWAUX_LENGTH_MASK);
> > +
> > +       regmap_bulk_write(map, PAGE0_SWAUX_ADDR_7_0, addr_len,
> > +                         ARRAY_SIZE(addr_len));
> > +
> > +       if (len && (request == DP_AUX_NATIVE_WRITE ||
> > +                   request == DP_AUX_I2C_WRITE)) {
> > +               /* Write to the internal FIFO buffer */
> > +               for (i = 0; i < len; i++) {
> > +                       ret = regmap_write(map, PAGE0_SWAUX_WDATA, buf[i]);
> > +                       if (ret) {
> > +                               dev_err(dev, "failed to write WDATA: %d\n",
>
> DRM_DEV_ERROR?
Sure.
>
> > +                                       ret);
> > +                               return ret;
> > +                       }
> > +               }
> > +       }
> > +
> > +       regmap_write(map, PAGE0_SWAUX_CTRL, SWAUX_SEND);
> > +
> > +       /* Zero delay loop because i2c transactions are slow already */
> > +       regmap_read_poll_timeout(map, PAGE0_SWAUX_CTRL, data,
> > +                                !(data & SWAUX_SEND), 0, 50 * 1000);
> > +
> > +       regmap_read(map, PAGE0_SWAUX_STATUS, &data);
> > +       if (ret) {
> > +               dev_err(dev, "failed to read PAGE0_SWAUX_STATUS: %d\n", ret);
>
> DRM_DEV_ERROR?
Sure.
>
> > +               return ret;
> > +       }
> > +
> > +       switch (data & SWAUX_STATUS_MASK) {
> > +       /* Ignore the DEFER cases as they are already handled in hardware */
> > +       case SWAUX_STATUS_NACK:
> > +       case SWAUX_STATUS_I2C_NACK:
> > +               /*
> > +                * The programming guide is not clear about whether a I2C NACK
> > +                * would trigger SWAUX_STATUS_NACK or SWAUX_STATUS_I2C_NACK. So
> > +                * we handle both cases together.
> > +                */
> > +               if (is_native_aux)
> > +                       msg->reply |= DP_AUX_NATIVE_REPLY_NACK;
> > +               else
> > +                       msg->reply |= DP_AUX_I2C_REPLY_NACK;
> > +
> > +               len = data & SWAUX_M_MASK;
> > +               return len;
>
> Why no 'return data & SWAUX_M_MASK;' and skip the assignment?
I want to make it clear that we are returning the number of bytes that
we have read/written instead of some error code.
If you think it's not super helpful, I can just return data & SWAUX_M_MASK.

>
> > +       case SWAUX_STATUS_ACKM:
>
> Move this up and add fallthrough?
Thanks.
Will add this change to v4.
>
> > +               len = data & SWAUX_M_MASK;
> > +               return len;
> > +       case SWAUX_STATUS_INVALID:
> > +               return -EOPNOTSUPP;
> > +       case SWAUX_STATUS_TIMEOUT:
> > +               return -ETIMEDOUT;
> > +       }
> > +
> > +       if (len && (request == DP_AUX_NATIVE_READ ||
> > +                   request == DP_AUX_I2C_READ)) {
> > +               /* Read from the internal FIFO buffer */
> > +               for (i = 0; i < len; i++) {
> > +                       ret = regmap_read(map, PAGE0_SWAUX_RDATA, &data);
> > +                       buf[i] = data;
>
> Can drop a line
regmap_read() wants to read to an unsigned int, but buf is an u8 array.
To be safe, I read RDATA to data and then assign data to buf[i].

As regmap_read() should always read 1 byte at a time, should I just do:
regmap_read(map, PAGE0_SWAUX_RDATA, (unsigned int*)(buf + i))

>
>                 ret = regmap_read(map, PAGE0_SWAUX_RDATA, buf + i);
>
> > +                       if (ret) {
> > +                               dev_err(dev, "failed to read RDATA: %d\n",
> > +                                       ret);
> > +                               return ret;
> > +                       }
> > +               }
> > +       }
> > +
> > +       return len;
> > +}

Re: [PATCH v3 3/3] drm/bridge: parade-ps8640: Add support for AUX channel

[Fabio Estevam]

On Wed, Sep 15, 2021 at 5:41 PM Philip Chen <philipchen@chromium.org> wrote:

> As regmap_read() should always read 1 byte at a time, should I just do:
> regmap_read(map, PAGE0_SWAUX_RDATA, (unsigned int*)(buf + i))

There is also regmap_bulk_read() if you need to read more data.

Re: [PATCH v3 3/3] drm/bridge: parade-ps8640: Add support for AUX channel

[Philip Chen]

Hi Fabio

On Wed, Sep 15, 2021 at 2:00 PM Fabio Estevam <festevam@gmail.com> wrote:
>
> On Wed, Sep 15, 2021 at 5:41 PM Philip Chen <philipchen@chromium.org> wrote:
>
> > As regmap_read() should always read 1 byte at a time, should I just do:
> > regmap_read(map, PAGE0_SWAUX_RDATA, (unsigned int*)(buf + i))
>
> There is also regmap_bulk_read() if you need to read more data.

Thanks for the review.
PAGE0_SWAUX_RDATA is a single-byte FIFO buffer.
So I'll need to read one byte at a time cyclically.

Re: [PATCH v3 3/3] drm/bridge: parade-ps8640: Add support for AUX channel

[Doug Anderson]

Hi,

On Tue, Sep 14, 2021 at 5:57 PM Stephen Boyd <swboyd@chromium.org> wrote:
>
> Quoting Philip Chen (2021-09-14 16:28:45)
> > diff --git a/drivers/gpu/drm/bridge/parade-ps8640.c b/drivers/gpu/drm/bridge/parade-ps8640.c
> > index 8d3e7a147170..dc349d729f5a 100644
> > --- a/drivers/gpu/drm/bridge/parade-ps8640.c
> > +++ b/drivers/gpu/drm/bridge/parade-ps8640.c
> > @@ -117,6 +144,129 @@ static inline struct ps8640 *bridge_to_ps8640(struct drm_bridge *e)
> [...]
> > +       case DP_AUX_I2C_WRITE:
> > +       case DP_AUX_I2C_READ:
> > +               break;
> > +       default:
> > +               return -EINVAL;
> > +       }
> > +
> > +       ret = regmap_write(map, PAGE0_AUXCH_CFG3, AUXCH_CFG3_RESET);
> > +       if (ret) {
> > +               dev_err(dev, "failed to write PAGE0_AUXCH_CFG3: %d\n", ret);
>
> Can we use DRM_DEV_ERROR()?

I've never gotten clear guidance here. For instance, in some other
review I suggested using the DRM wrapper and got told "no" [1]. ;-)
The driver landed without the DRM_ERROR versions. I don't really care
lots so it's fine with me to use use DRM_DEV_ERROR, I just wish I
understood the rules...

[1] https://lore.kernel.org/all/49db7ef3-fa53-a274-7c69-c2d840b13058@denx.de/


> > +               return ret;
> > +       }
> > +
> > +       /* Assume it's good */
> > +       msg->reply = 0;
> > +
> > +       addr_len[0] = msg->address & 0xff;
> > +       addr_len[1] = (msg->address >> 8) & 0xff;
> > +       addr_len[2] = ((msg->request << 4) & SWAUX_CMD_MASK) |
> > +               ((msg->address >> 16) & SWAUX_ADDR_19_16_MASK);
>
> It really feels like this out to be possible with some sort of
> cpu_to_le32() API. We're shoving msg->address into 3 bytes and then
> adding in the request and some length. So we could do something like:
>
>         u32 addr_len;
>
>         addr_len = FIELD_PREP(SWAUX_ADDR_MASK, msg->address);
>         addr_len |= FIELD_PREP(SWAUX_CMD_MASK, msg->request);
>         if (len)
>                 addr_len |= FIELD_PREP(LEN_MASK, len - 1);
>         else
>                 addr_len |= FIELD_PREP(LEN_MASK, SWAUX_NO_PAYLOAD );
>
>         cpu_to_le32s(&addr_len);
>
>         regmap_bulk_write(map, PAGE0_SWAUX_ADDR_7_0, &addr_len, sizeof(addr_len));

You're arguing that your version of the code is more efficient? Easier
to understand? Something else? To me, Philip's initial version is
crystal clear and easy to map to the bridge datasheet but I need to
think more to confirm that your version is right. Thinking is hard and
I like to avoid it when possible.

In any case, it's definitely bikeshedding and I'll yield if everyone
likes the other version better. ;-)


> > +               return ret;
> > +       }
> > +
> > +       switch (data & SWAUX_STATUS_MASK) {
> > +       /* Ignore the DEFER cases as they are already handled in hardware */
> > +       case SWAUX_STATUS_NACK:
> > +       case SWAUX_STATUS_I2C_NACK:
> > +               /*
> > +                * The programming guide is not clear about whether a I2C NACK
> > +                * would trigger SWAUX_STATUS_NACK or SWAUX_STATUS_I2C_NACK. So
> > +                * we handle both cases together.
> > +                */
> > +               if (is_native_aux)
> > +                       msg->reply |= DP_AUX_NATIVE_REPLY_NACK;
> > +               else
> > +                       msg->reply |= DP_AUX_I2C_REPLY_NACK;
> > +
> > +               len = data & SWAUX_M_MASK;
> > +               return len;
>
> Why no 'return data & SWAUX_M_MASK;' and skip the assignment?

Actually, I think it's the "return" that's a bug, isn't it? If we're
doing a "read" and we're returning a positive number of bytes then we
need to actually _read_ them. Reading happens below, doesn't it?


-Doug

Re: [PATCH v3 3/3] drm/bridge: parade-ps8640: Add support for AUX channel

[Stephen Boyd]

Quoting Doug Anderson (2021-09-15 14:27:40)
> Hi,
>
> On Tue, Sep 14, 2021 at 5:57 PM Stephen Boyd <swboyd@chromium.org> wrote:
> >
> > Quoting Philip Chen (2021-09-14 16:28:45)
> > > diff --git a/drivers/gpu/drm/bridge/parade-ps8640.c b/drivers/gpu/drm/bridge/parade-ps8640.c
> > > index 8d3e7a147170..dc349d729f5a 100644
> > > --- a/drivers/gpu/drm/bridge/parade-ps8640.c
> > > +++ b/drivers/gpu/drm/bridge/parade-ps8640.c
> > > @@ -117,6 +144,129 @@ static inline struct ps8640 *bridge_to_ps8640(struct drm_bridge *e)
> > [...]
> > > +       case DP_AUX_I2C_WRITE:
> > > +       case DP_AUX_I2C_READ:
> > > +               break;
> > > +       default:
> > > +               return -EINVAL;
> > > +       }
> > > +
> > > +       ret = regmap_write(map, PAGE0_AUXCH_CFG3, AUXCH_CFG3_RESET);
> > > +       if (ret) {
> > > +               dev_err(dev, "failed to write PAGE0_AUXCH_CFG3: %d\n", ret);
> >
> > Can we use DRM_DEV_ERROR()?
>
> I've never gotten clear guidance here. For instance, in some other
> review I suggested using the DRM wrapper and got told "no" [1]. ;-)
> The driver landed without the DRM_ERROR versions. I don't really care
> lots so it's fine with me to use use DRM_DEV_ERROR, I just wish I
> understood the rules...
>
> [1] https://lore.kernel.org/all/49db7ef3-fa53-a274-7c69-c2d840b13058@denx.de/

I think the rule is that the DRM specific printk stuff should be used so
that they can be stuck into the drm logs. On chromeOS we also have a
record of the drm logs that we can use to debug things, split away from
the general kernel printk logs. So using DRM prints when there's a DRM
device around is a good thing to do.

>
>
> > > +               return ret;
> > > +       }
> > > +
> > > +       /* Assume it's good */
> > > +       msg->reply = 0;
> > > +
> > > +       addr_len[0] = msg->address & 0xff;
> > > +       addr_len[1] = (msg->address >> 8) & 0xff;
> > > +       addr_len[2] = ((msg->request << 4) & SWAUX_CMD_MASK) |
> > > +               ((msg->address >> 16) & SWAUX_ADDR_19_16_MASK);
> >
> > It really feels like this out to be possible with some sort of
> > cpu_to_le32() API. We're shoving msg->address into 3 bytes and then
> > adding in the request and some length. So we could do something like:
> >
> >         u32 addr_len;
> >
> >         addr_len = FIELD_PREP(SWAUX_ADDR_MASK, msg->address);
> >         addr_len |= FIELD_PREP(SWAUX_CMD_MASK, msg->request);
> >         if (len)
> >                 addr_len |= FIELD_PREP(LEN_MASK, len - 1);
> >         else
> >                 addr_len |= FIELD_PREP(LEN_MASK, SWAUX_NO_PAYLOAD );
> >
> >         cpu_to_le32s(&addr_len);
> >
> >         regmap_bulk_write(map, PAGE0_SWAUX_ADDR_7_0, &addr_len, sizeof(addr_len));
>
> You're arguing that your version of the code is more efficient? Easier
> to understand? Something else? To me, Philip's initial version is
> crystal clear and easy to map to the bridge datasheet but I need to
> think more to confirm that your version is right. Thinking is hard and
> I like to avoid it when possible.
>
> In any case, it's definitely bikeshedding and I'll yield if everyone
> likes the other version better. ;-)

Yeah it's bikeshedding. I don't really care about this either but I find
it easier to read when the assignment isn't wrapped across multiple
lines. If the buffer approach is preferable then maybe use the address
macros to clarify which register is being set?

	unsigned int base = PAGE0_SWAUX_ADDR_7_0;

	addr_len[PAGE0_SWAUX_ADDR_7_0 - base] = msg->address;
	addr_len[PAGE0_SWAUX_ADDR_15_8 - base] = msg->address >> 8;
	addr_len[PAGE0_SWAUX_ADDR_23_16 - base] = msg->address >> 16;
	addr_len[PAGE0_SWAUX_ADDR_23_16 - base] |= msg->request << 4;
	...

>
>
> > > +               return ret;
> > > +       }
> > > +
> > > +       switch (data & SWAUX_STATUS_MASK) {
> > > +       /* Ignore the DEFER cases as they are already handled in hardware */
> > > +       case SWAUX_STATUS_NACK:
> > > +       case SWAUX_STATUS_I2C_NACK:
> > > +               /*
> > > +                * The programming guide is not clear about whether a I2C NACK
> > > +                * would trigger SWAUX_STATUS_NACK or SWAUX_STATUS_I2C_NACK. So
> > > +                * we handle both cases together.
> > > +                */
> > > +               if (is_native_aux)
> > > +                       msg->reply |= DP_AUX_NATIVE_REPLY_NACK;
> > > +               else
> > > +                       msg->reply |= DP_AUX_I2C_REPLY_NACK;
> > > +
> > > +               len = data & SWAUX_M_MASK;
> > > +               return len;
> >
> > Why no 'return data & SWAUX_M_MASK;' and skip the assignment?
>
> Actually, I think it's the "return" that's a bug, isn't it? If we're
> doing a "read" and we're returning a positive number of bytes then we
> need to actually _read_ them. Reading happens below, doesn't it?
>

Oh I missed that. We're still supposed to return data to upper
layers on a NACKed read?

Re: [PATCH v3 3/3] drm/bridge: parade-ps8640: Add support for AUX channel

[Doug Anderson]

Hi,

On Thu, Sep 16, 2021 at 1:15 PM Stephen Boyd <swboyd@chromium.org> wrote:
>
> > > > +               return ret;
> > > > +       }
> > > > +
> > > > +       /* Assume it's good */
> > > > +       msg->reply = 0;
> > > > +
> > > > +       addr_len[0] = msg->address & 0xff;
> > > > +       addr_len[1] = (msg->address >> 8) & 0xff;
> > > > +       addr_len[2] = ((msg->request << 4) & SWAUX_CMD_MASK) |
> > > > +               ((msg->address >> 16) & SWAUX_ADDR_19_16_MASK);
> > >
> > > It really feels like this out to be possible with some sort of
> > > cpu_to_le32() API. We're shoving msg->address into 3 bytes and then
> > > adding in the request and some length. So we could do something like:
> > >
> > >         u32 addr_len;
> > >
> > >         addr_len = FIELD_PREP(SWAUX_ADDR_MASK, msg->address);
> > >         addr_len |= FIELD_PREP(SWAUX_CMD_MASK, msg->request);
> > >         if (len)
> > >                 addr_len |= FIELD_PREP(LEN_MASK, len - 1);
> > >         else
> > >                 addr_len |= FIELD_PREP(LEN_MASK, SWAUX_NO_PAYLOAD );
> > >
> > >         cpu_to_le32s(&addr_len);
> > >
> > >         regmap_bulk_write(map, PAGE0_SWAUX_ADDR_7_0, &addr_len, sizeof(addr_len));
> >
> > You're arguing that your version of the code is more efficient? Easier
> > to understand? Something else? To me, Philip's initial version is
> > crystal clear and easy to map to the bridge datasheet but I need to
> > think more to confirm that your version is right. Thinking is hard and
> > I like to avoid it when possible.
> >
> > In any case, it's definitely bikeshedding and I'll yield if everyone
> > likes the other version better. ;-)
>
> Yeah it's bikeshedding. I don't really care about this either but I find
> it easier to read when the assignment isn't wrapped across multiple
> lines. If the buffer approach is preferable then maybe use the address
> macros to clarify which register is being set?
>
>         unsigned int base = PAGE0_SWAUX_ADDR_7_0;
>
>         addr_len[PAGE0_SWAUX_ADDR_7_0 - base] = msg->address;
>         addr_len[PAGE0_SWAUX_ADDR_15_8 - base] = msg->address >> 8;
>         addr_len[PAGE0_SWAUX_ADDR_23_16 - base] = msg->address >> 16;
>         addr_len[PAGE0_SWAUX_ADDR_23_16 - base] |= msg->request << 4;

Sure, this looks nice to me. :-)


> > > > +               return ret;
> > > > +       }
> > > > +
> > > > +       switch (data & SWAUX_STATUS_MASK) {
> > > > +       /* Ignore the DEFER cases as they are already handled in hardware */
> > > > +       case SWAUX_STATUS_NACK:
> > > > +       case SWAUX_STATUS_I2C_NACK:
> > > > +               /*
> > > > +                * The programming guide is not clear about whether a I2C NACK
> > > > +                * would trigger SWAUX_STATUS_NACK or SWAUX_STATUS_I2C_NACK. So
> > > > +                * we handle both cases together.
> > > > +                */
> > > > +               if (is_native_aux)
> > > > +                       msg->reply |= DP_AUX_NATIVE_REPLY_NACK;
> > > > +               else
> > > > +                       msg->reply |= DP_AUX_I2C_REPLY_NACK;
> > > > +
> > > > +               len = data & SWAUX_M_MASK;
> > > > +               return len;
> > >
> > > Why no 'return data & SWAUX_M_MASK;' and skip the assignment?
> >
> > Actually, I think it's the "return" that's a bug, isn't it? If we're
> > doing a "read" and we're returning a positive number of bytes then we
> > need to actually _read_ them. Reading happens below, doesn't it?
> >
>
> Oh I missed that. We're still supposed to return data to upper
> layers on a NACKed read?

It seems highly likely not to matter in practice, but:

* The docs from parade clearly state that when a NAK is returned that
the number of bytes transferred before the NAK will be provided to us.

* The i2c interface specifies NAK not as a return code but as a bit in
the "reply". That presumably is to let us return the number of bytes
transferred before the NAK to the next level up.

* If we're returning the number of bytes and it's a read then we'd
better return the data too!

It looks like we handled this OK in the TI bridge driver. From reading
the TI docs we'll get both AUX_IRQ_STATUS_AUX_SHORT and
AUX_IRQ_STATUS_NAT_I2C_FAIL in the case of a partial transfer and so
we'll do the right thing.

-Doug

Re: [PATCH v3 3/3] drm/bridge: parade-ps8640: Add support for AUX channel

[Philip Chen]

Hi

On Thu, Sep 16, 2021 at 1:31 PM Doug Anderson <dianders@chromium.org> wrote:
>
> Hi,
>
> On Thu, Sep 16, 2021 at 1:15 PM Stephen Boyd <swboyd@chromium.org> wrote:
> >
> > > > > +               return ret;
> > > > > +       }
> > > > > +
> > > > > +       /* Assume it's good */
> > > > > +       msg->reply = 0;
> > > > > +
> > > > > +       addr_len[0] = msg->address & 0xff;
> > > > > +       addr_len[1] = (msg->address >> 8) & 0xff;
> > > > > +       addr_len[2] = ((msg->request << 4) & SWAUX_CMD_MASK) |
> > > > > +               ((msg->address >> 16) & SWAUX_ADDR_19_16_MASK);
> > > >
> > > > It really feels like this out to be possible with some sort of
> > > > cpu_to_le32() API. We're shoving msg->address into 3 bytes and then
> > > > adding in the request and some length. So we could do something like:
> > > >
> > > >         u32 addr_len;
> > > >
> > > >         addr_len = FIELD_PREP(SWAUX_ADDR_MASK, msg->address);
> > > >         addr_len |= FIELD_PREP(SWAUX_CMD_MASK, msg->request);
> > > >         if (len)
> > > >                 addr_len |= FIELD_PREP(LEN_MASK, len - 1);
> > > >         else
> > > >                 addr_len |= FIELD_PREP(LEN_MASK, SWAUX_NO_PAYLOAD );
> > > >
> > > >         cpu_to_le32s(&addr_len);
> > > >
> > > >         regmap_bulk_write(map, PAGE0_SWAUX_ADDR_7_0, &addr_len, sizeof(addr_len));
> > >
> > > You're arguing that your version of the code is more efficient? Easier
> > > to understand? Something else? To me, Philip's initial version is
> > > crystal clear and easy to map to the bridge datasheet but I need to
> > > think more to confirm that your version is right. Thinking is hard and
> > > I like to avoid it when possible.
> > >
> > > In any case, it's definitely bikeshedding and I'll yield if everyone
> > > likes the other version better. ;-)
> >
> > Yeah it's bikeshedding. I don't really care about this either but I find
> > it easier to read when the assignment isn't wrapped across multiple
> > lines. If the buffer approach is preferable then maybe use the address
> > macros to clarify which register is being set?
> >
> >         unsigned int base = PAGE0_SWAUX_ADDR_7_0;
> >
> >         addr_len[PAGE0_SWAUX_ADDR_7_0 - base] = msg->address;
> >         addr_len[PAGE0_SWAUX_ADDR_15_8 - base] = msg->address >> 8;
> >         addr_len[PAGE0_SWAUX_ADDR_23_16 - base] = msg->address >> 16;
> >         addr_len[PAGE0_SWAUX_ADDR_23_16 - base] |= msg->request << 4;
>
> Sure, this looks nice to me. :-)
Thanks.
Implemented the fix in v4.
PTAL.

>
>
> > > > > +               return ret;
> > > > > +       }
> > > > > +
> > > > > +       switch (data & SWAUX_STATUS_MASK) {
> > > > > +       /* Ignore the DEFER cases as they are already handled in hardware */
> > > > > +       case SWAUX_STATUS_NACK:
> > > > > +       case SWAUX_STATUS_I2C_NACK:
> > > > > +               /*
> > > > > +                * The programming guide is not clear about whether a I2C NACK
> > > > > +                * would trigger SWAUX_STATUS_NACK or SWAUX_STATUS_I2C_NACK. So
> > > > > +                * we handle both cases together.
> > > > > +                */
> > > > > +               if (is_native_aux)
> > > > > +                       msg->reply |= DP_AUX_NATIVE_REPLY_NACK;
> > > > > +               else
> > > > > +                       msg->reply |= DP_AUX_I2C_REPLY_NACK;
> > > > > +
> > > > > +               len = data & SWAUX_M_MASK;
> > > > > +               return len;
> > > >
> > > > Why no 'return data & SWAUX_M_MASK;' and skip the assignment?
> > >
> > > Actually, I think it's the "return" that's a bug, isn't it? If we're
> > > doing a "read" and we're returning a positive number of bytes then we
> > > need to actually _read_ them. Reading happens below, doesn't it?
> > >
> >
> > Oh I missed that. We're still supposed to return data to upper
> > layers on a NACKed read?
>
> It seems highly likely not to matter in practice, but:
>
> * The docs from parade clearly state that when a NAK is returned that
> the number of bytes transferred before the NAK will be provided to us.
>
> * The i2c interface specifies NAK not as a return code but as a bit in
> the "reply". That presumably is to let us return the number of bytes
> transferred before the NAK to the next level up.
>
> * If we're returning the number of bytes and it's a read then we'd
> better return the data too!
>
> It looks like we handled this OK in the TI bridge driver. From reading
> the TI docs we'll get both AUX_IRQ_STATUS_AUX_SHORT and
> AUX_IRQ_STATUS_NAT_I2C_FAIL in the case of a partial transfer and so
> we'll do the right thing.

Thanks for catching the bug.
In v4, I made SWAUX_STATUS_I2C_NACK fall through to SWAUX_STATUS_ACKM and
store the number of read/written bytes in len w/o returning immediately.
PTAL.

>
> -Doug

Re: [PATCH v3 3/3] drm/bridge: parade-ps8640: Add support for AUX channel

[Philip Chen]

Hi

On Tue, Sep 14, 2021 at 5:57 PM Stephen Boyd <swboyd@chromium.org> wrote:
>
> Quoting Philip Chen (2021-09-14 16:28:45)
> > diff --git a/drivers/gpu/drm/bridge/parade-ps8640.c b/drivers/gpu/drm/bridge/parade-ps8640.c
> > index 8d3e7a147170..dc349d729f5a 100644
> > --- a/drivers/gpu/drm/bridge/parade-ps8640.c
> > +++ b/drivers/gpu/drm/bridge/parade-ps8640.c
> > @@ -117,6 +144,129 @@ static inline struct ps8640 *bridge_to_ps8640(struct drm_bridge *e)
> [...]
> > +       case DP_AUX_I2C_WRITE:
> > +       case DP_AUX_I2C_READ:
> > +               break;
> > +       default:
> > +               return -EINVAL;
> > +       }
> > +
> > +       ret = regmap_write(map, PAGE0_AUXCH_CFG3, AUXCH_CFG3_RESET);
> > +       if (ret) {
> > +               dev_err(dev, "failed to write PAGE0_AUXCH_CFG3: %d\n", ret);
>
> Can we use DRM_DEV_ERROR()?
Sure. Done in v4.
PTAL.
>
> > +               return ret;
> > +       }
> > +
> > +       /* Assume it's good */
> > +       msg->reply = 0;
> > +
> > +       addr_len[0] = msg->address & 0xff;
> > +       addr_len[1] = (msg->address >> 8) & 0xff;
> > +       addr_len[2] = ((msg->request << 4) & SWAUX_CMD_MASK) |
> > +               ((msg->address >> 16) & SWAUX_ADDR_19_16_MASK);
>
> It really feels like this out to be possible with some sort of
> cpu_to_le32() API. We're shoving msg->address into 3 bytes and then
> adding in the request and some length. So we could do something like:
>
>         u32 addr_len;
>
>         addr_len = FIELD_PREP(SWAUX_ADDR_MASK, msg->address);
>         addr_len |= FIELD_PREP(SWAUX_CMD_MASK, msg->request);
>         if (len)
>                 addr_len |= FIELD_PREP(LEN_MASK, len - 1);
>         else
>                 addr_len |= FIELD_PREP(LEN_MASK, SWAUX_NO_PAYLOAD );
>
>         cpu_to_le32s(&addr_len);
>
>         regmap_bulk_write(map, PAGE0_SWAUX_ADDR_7_0, &addr_len, sizeof(addr_len));
>
> > +       addr_len[3] = (len == 0) ? SWAUX_NO_PAYLOAD :
> > +                       ((len - 1) & SWAUX_LENGTH_MASK);
> > +
> > +       regmap_bulk_write(map, PAGE0_SWAUX_ADDR_7_0, addr_len,
> > +                         ARRAY_SIZE(addr_len));
> > +
> > +       if (len && (request == DP_AUX_NATIVE_WRITE ||
> > +                   request == DP_AUX_I2C_WRITE)) {
> > +               /* Write to the internal FIFO buffer */
> > +               for (i = 0; i < len; i++) {
> > +                       ret = regmap_write(map, PAGE0_SWAUX_WDATA, buf[i]);
> > +                       if (ret) {
> > +                               dev_err(dev, "failed to write WDATA: %d\n",
>
> DRM_DEV_ERROR?
Sure. Done in v4.
PTAL.
>
> > +                                       ret);
> > +                               return ret;
> > +                       }
> > +               }
> > +       }
> > +
> > +       regmap_write(map, PAGE0_SWAUX_CTRL, SWAUX_SEND);
> > +
> > +       /* Zero delay loop because i2c transactions are slow already */
> > +       regmap_read_poll_timeout(map, PAGE0_SWAUX_CTRL, data,
> > +                                !(data & SWAUX_SEND), 0, 50 * 1000);
> > +
> > +       regmap_read(map, PAGE0_SWAUX_STATUS, &data);
> > +       if (ret) {
> > +               dev_err(dev, "failed to read PAGE0_SWAUX_STATUS: %d\n", ret);
>
> DRM_DEV_ERROR?
Sure. Done in v4.
PTAL.
>
> > +               return ret;
> > +       }
> > +
> > +       switch (data & SWAUX_STATUS_MASK) {
> > +       /* Ignore the DEFER cases as they are already handled in hardware */
> > +       case SWAUX_STATUS_NACK:
> > +       case SWAUX_STATUS_I2C_NACK:
> > +               /*
> > +                * The programming guide is not clear about whether a I2C NACK
> > +                * would trigger SWAUX_STATUS_NACK or SWAUX_STATUS_I2C_NACK. So
> > +                * we handle both cases together.
> > +                */
> > +               if (is_native_aux)
> > +                       msg->reply |= DP_AUX_NATIVE_REPLY_NACK;
> > +               else
> > +                       msg->reply |= DP_AUX_I2C_REPLY_NACK;
> > +
> > +               len = data & SWAUX_M_MASK;
> > +               return len;
>
> Why no 'return data & SWAUX_M_MASK;' and skip the assignment?
>
> > +       case SWAUX_STATUS_ACKM:
>
> Move this up and add fallthrough?
>
> > +               len = data & SWAUX_M_MASK;
> > +               return len;
> > +       case SWAUX_STATUS_INVALID:
> > +               return -EOPNOTSUPP;
> > +       case SWAUX_STATUS_TIMEOUT:
> > +               return -ETIMEDOUT;
> > +       }
> > +
> > +       if (len && (request == DP_AUX_NATIVE_READ ||
> > +                   request == DP_AUX_I2C_READ)) {
> > +               /* Read from the internal FIFO buffer */
> > +               for (i = 0; i < len; i++) {
> > +                       ret = regmap_read(map, PAGE0_SWAUX_RDATA, &data);
> > +                       buf[i] = data;
>
> Can drop a line
Sure. Done in v4.
PTAL.
>
>                 ret = regmap_read(map, PAGE0_SWAUX_RDATA, buf + i);
>
> > +                       if (ret) {
> > +                               dev_err(dev, "failed to read RDATA: %d\n",
> > +                                       ret);
> > +                               return ret;
> > +                       }
> > +               }
> > +       }
> > +
> > +       return len;
> > +}

Re: [PATCH v3 1/3] drm/bridge: parade-ps8640: Improve logging at probing

[Stephen Boyd]

Quoting Philip Chen (2021-09-14 16:28:43)
> Use dev_err_probe() to add logs for error cases at probing time.
>
> Signed-off-by: Philip Chen <philipchen@chromium.org>
> ---
>

Can you use a cover-letter for more than one patch series?

> (no changes since v1)
>
>  drivers/gpu/drm/bridge/parade-ps8640.c | 22 ++++++++++++++--------
>  1 file changed, 14 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/parade-ps8640.c b/drivers/gpu/drm/bridge/parade-ps8640.c
> index 685e9c38b2db..e340af381e05 100644
> --- a/drivers/gpu/drm/bridge/parade-ps8640.c
> +++ b/drivers/gpu/drm/bridge/parade-ps8640.c
> @@ -332,8 +332,10 @@ static int ps8640_probe(struct i2c_client *client)
>                 return -ENODEV;
>
>         ps_bridge->panel_bridge = devm_drm_panel_bridge_add(dev, panel);
> -       if (IS_ERR(ps_bridge->panel_bridge))
> -               return PTR_ERR(ps_bridge->panel_bridge);
> +       if (IS_ERR(ps_bridge->panel_bridge)) {
> +               return dev_err_probe(dev, PTR_ERR(ps_bridge->panel_bridge),
> +                                    "Error creating bridge device\n");

From what I can tell it never returns -EPROBE_DEFER? So this isn't
useful.

> +       }
>
>         ps_bridge->supplies[0].supply = "vdd33";
>         ps_bridge->supplies[1].supply = "vdd12";
> @@ -344,16 +346,20 @@ static int ps8640_probe(struct i2c_client *client)
>
>         ps_bridge->gpio_powerdown = devm_gpiod_get(&client->dev, "powerdown",
>                                                    GPIOD_OUT_HIGH);
> -       if (IS_ERR(ps_bridge->gpio_powerdown))
> -               return PTR_ERR(ps_bridge->gpio_powerdown);
> +       if (IS_ERR(ps_bridge->gpio_powerdown)) {
> +               return dev_err_probe(dev, PTR_ERR(ps_bridge->gpio_powerdown),
> +                                    "Error getting gpio_powerdown\n");

This looks ok, except we don't want braces on single statement ifs.

> +       }
>
>         /*
>          * Assert the reset to avoid the bridge being initialized prematurely
>          */
>         ps_bridge->gpio_reset = devm_gpiod_get(&client->dev, "reset",
>                                                GPIOD_OUT_HIGH);
> -       if (IS_ERR(ps_bridge->gpio_reset))
> -               return PTR_ERR(ps_bridge->gpio_reset);
> +       if (IS_ERR(ps_bridge->gpio_reset)) {
> +               return dev_err_probe(dev, PTR_ERR(ps_bridge->gpio_reset),
> +                                    "Error getting gpio_reset\n");

Same.

> +       }
>
>         ps_bridge->bridge.funcs = &ps8640_bridge_funcs;
>         ps_bridge->bridge.of_node = dev->of_node;

Re: [PATCH v3 1/3] drm/bridge: parade-ps8640: Improve logging at probing

[Philip Chen]

Hi Stephen,

Based on the discussion in [1], this patch is not really needed for now.
So I'll just remove this patch from v4.
Thanks.

[1] https://patchwork.kernel.org/project/dri-devel/patch/20210914162825.v3.2.Ib06997ddd73e2ac29e185f039d85cfa8e760d641@changeid/

On Tue, Sep 14, 2021 at 5:27 PM Stephen Boyd <swboyd@chromium.org> wrote:
>
> Quoting Philip Chen (2021-09-14 16:28:43)
> > Use dev_err_probe() to add logs for error cases at probing time.
> >
> > Signed-off-by: Philip Chen <philipchen@chromium.org>
> > ---
> >
>
> Can you use a cover-letter for more than one patch series?
>
> > (no changes since v1)
> >
> >  drivers/gpu/drm/bridge/parade-ps8640.c | 22 ++++++++++++++--------
> >  1 file changed, 14 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/bridge/parade-ps8640.c b/drivers/gpu/drm/bridge/parade-ps8640.c
> > index 685e9c38b2db..e340af381e05 100644
> > --- a/drivers/gpu/drm/bridge/parade-ps8640.c
> > +++ b/drivers/gpu/drm/bridge/parade-ps8640.c
> > @@ -332,8 +332,10 @@ static int ps8640_probe(struct i2c_client *client)
> >                 return -ENODEV;
> >
> >         ps_bridge->panel_bridge = devm_drm_panel_bridge_add(dev, panel);
> > -       if (IS_ERR(ps_bridge->panel_bridge))
> > -               return PTR_ERR(ps_bridge->panel_bridge);
> > +       if (IS_ERR(ps_bridge->panel_bridge)) {
> > +               return dev_err_probe(dev, PTR_ERR(ps_bridge->panel_bridge),
> > +                                    "Error creating bridge device\n");
>
> From what I can tell it never returns -EPROBE_DEFER? So this isn't
> useful.
>
> > +       }
> >
> >         ps_bridge->supplies[0].supply = "vdd33";
> >         ps_bridge->supplies[1].supply = "vdd12";
> > @@ -344,16 +346,20 @@ static int ps8640_probe(struct i2c_client *client)
> >
> >         ps_bridge->gpio_powerdown = devm_gpiod_get(&client->dev, "powerdown",
> >                                                    GPIOD_OUT_HIGH);
> > -       if (IS_ERR(ps_bridge->gpio_powerdown))
> > -               return PTR_ERR(ps_bridge->gpio_powerdown);
> > +       if (IS_ERR(ps_bridge->gpio_powerdown)) {
> > +               return dev_err_probe(dev, PTR_ERR(ps_bridge->gpio_powerdown),
> > +                                    "Error getting gpio_powerdown\n");
>
> This looks ok, except we don't want braces on single statement ifs.
>
> > +       }
> >
> >         /*
> >          * Assert the reset to avoid the bridge being initialized prematurely
> >          */
> >         ps_bridge->gpio_reset = devm_gpiod_get(&client->dev, "reset",
> >                                                GPIOD_OUT_HIGH);
> > -       if (IS_ERR(ps_bridge->gpio_reset))
> > -               return PTR_ERR(ps_bridge->gpio_reset);
> > +       if (IS_ERR(ps_bridge->gpio_reset)) {
> > +               return dev_err_probe(dev, PTR_ERR(ps_bridge->gpio_reset),
> > +                                    "Error getting gpio_reset\n");
>
> Same.
>
> > +       }
> >
> >         ps_bridge->bridge.funcs = &ps8640_bridge_funcs;
> >         ps_bridge->bridge.of_node = dev->of_node;