| Message ID | 20210914162825.v3.1.I85e46da154e3fa570442b496a0363250fff0e44e@changeid (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3,1/3] drm/bridge: parade-ps8640: Improve logging at probing | expand |
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;
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;
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]); } }
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(-)