| Message ID | 20240910170610.226189-5-prabhakar.mahadev-lad.rj@bp.renesas.com |
|---|---|
| State | New |
| Headers | show |
| Series | media: ov5645: Add support for streams | expand |
Hi Prabhakar, Thank you for the patch. On Tue, Sep 10, 2024 at 06:06:03PM +0100, Prabhakar wrote: > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > Drop dev_err() and use the dev_err_probe() helper on probe path. > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > --- > drivers/media/i2c/ov5645.c | 74 +++++++++++++++----------------------- > 1 file changed, 28 insertions(+), 46 deletions(-) > > diff --git a/drivers/media/i2c/ov5645.c b/drivers/media/i2c/ov5645.c > index 78b86438c798..9e6ff1f1b9ac 100644 > --- a/drivers/media/i2c/ov5645.c > +++ b/drivers/media/i2c/ov5645.c > @@ -1076,51 +1076,37 @@ static int ov5645_probe(struct i2c_client *client) > ov5645->dev = dev; > > endpoint = of_graph_get_endpoint_by_regs(dev->of_node, 0, -1); > - if (!endpoint) { > - dev_err(dev, "endpoint node not found\n"); > - return -EINVAL; > - } > + if (!endpoint) > + return dev_err_probe(dev, -EINVAL, "endpoint node not found\n"); > > ret = v4l2_fwnode_endpoint_parse(of_fwnode_handle(endpoint), > &ov5645->ep); > > of_node_put(endpoint); > > - if (ret < 0) { > - dev_err(dev, "parsing endpoint node failed\n"); > - return ret; > - } > + if (ret < 0) > + return dev_err_probe(dev, ret, "parsing endpoint node failed\n"); > > - if (ov5645->ep.bus_type != V4L2_MBUS_CSI2_DPHY) { > - dev_err(dev, "invalid bus type, must be CSI2\n"); > - return -EINVAL; > - } > + if (ov5645->ep.bus_type != V4L2_MBUS_CSI2_DPHY) > + return dev_err_probe(dev, -EINVAL, "invalid bus type, must be CSI2\n"); > > /* get system clock (xclk) */ > ov5645->xclk = devm_clk_get(dev, NULL); > - if (IS_ERR(ov5645->xclk)) { > - dev_err(dev, "could not get xclk"); > - return PTR_ERR(ov5645->xclk); > - } > + if (IS_ERR(ov5645->xclk)) > + return dev_err_probe(dev, PTR_ERR(ov5645->xclk), "could not get xclk"); > > ret = of_property_read_u32(dev->of_node, "clock-frequency", &xclk_freq); > - if (ret) { > - dev_err(dev, "could not get xclk frequency\n"); > - return ret; > - } > + if (ret) > + return dev_err_probe(dev, ret, "could not get xclk frequency\n"); > > /* external clock must be 24MHz, allow 1% tolerance */ > - if (xclk_freq < 23760000 || xclk_freq > 24240000) { > - dev_err(dev, "external clock frequency %u is not supported\n", > - xclk_freq); > - return -EINVAL; > - } > + if (xclk_freq < 23760000 || xclk_freq > 24240000) > + return dev_err_probe(dev, -EINVAL, "external clock frequency %u is not supported\n", > + xclk_freq); > > ret = clk_set_rate(ov5645->xclk, xclk_freq); > - if (ret) { > - dev_err(dev, "could not set xclk frequency\n"); > - return ret; > - } > + if (ret) > + return dev_err_probe(dev, ret, "could not set xclk frequency\n"); > > for (i = 0; i < OV5645_NUM_SUPPLIES; i++) > ov5645->supplies[i].supply = ov5645_supply_name[i]; > @@ -1131,16 +1117,12 @@ static int ov5645_probe(struct i2c_client *client) > return ret; > > ov5645->enable_gpio = devm_gpiod_get(dev, "enable", GPIOD_OUT_HIGH); > - if (IS_ERR(ov5645->enable_gpio)) { > - dev_err(dev, "cannot get enable gpio\n"); > - return PTR_ERR(ov5645->enable_gpio); > - } > + if (IS_ERR(ov5645->enable_gpio)) > + return dev_err_probe(dev, PTR_ERR(ov5645->enable_gpio), "cannot get enable gpio\n"); Those lines are getting long. We usually try to wrap at 80 columns for sensor drivers: return dev_err_probe(dev, PTR_ERR(ov5645->enable_gpio), "cannot get enable gpio\n"); Same elsewhere. I'll let Sakari decide. > > ov5645->rst_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH); > - if (IS_ERR(ov5645->rst_gpio)) { > - dev_err(dev, "cannot get reset gpio\n"); > - return PTR_ERR(ov5645->rst_gpio); > - } > + if (IS_ERR(ov5645->rst_gpio)) > + return dev_err_probe(dev, PTR_ERR(ov5645->rst_gpio), "cannot get reset gpio\n"); > > mutex_init(&ov5645->power_lock); > > @@ -1177,9 +1159,9 @@ static int ov5645_probe(struct i2c_client *client) > ov5645->sd.ctrl_handler = &ov5645->ctrls; > > if (ov5645->ctrls.error) { > - dev_err(dev, "%s: control initialization error %d\n", > - __func__, ov5645->ctrls.error); > ret = ov5645->ctrls.error; > + dev_err_probe(dev, ret, "%s: control initialization error %d\n", > + __func__, ov5645->ctrls.error); > goto free_ctrl; > } > > @@ -1192,7 +1174,7 @@ static int ov5645_probe(struct i2c_client *client) > > ret = media_entity_pads_init(&ov5645->sd.entity, 1, &ov5645->pad); > if (ret < 0) { > - dev_err(dev, "could not register media entity\n"); > + dev_err_probe(dev, ret, "could not register media entity\n"); > goto free_ctrl; > } > > @@ -1202,14 +1184,14 @@ static int ov5645_probe(struct i2c_client *client) > > ret = ov5645_read_reg(ov5645, OV5645_CHIP_ID_HIGH, &chip_id_high); > if (ret < 0 || chip_id_high != OV5645_CHIP_ID_HIGH_BYTE) { > - dev_err(dev, "could not read ID high\n"); > ret = -ENODEV; > + dev_err_probe(dev, ret, "could not read ID high\n"); > goto power_down; > } > ret = ov5645_read_reg(ov5645, OV5645_CHIP_ID_LOW, &chip_id_low); > if (ret < 0 || chip_id_low != OV5645_CHIP_ID_LOW_BYTE) { > - dev_err(dev, "could not read ID low\n"); > ret = -ENODEV; > + dev_err_probe(dev, ret, "could not read ID low\n"); > goto power_down; > } > > @@ -1218,24 +1200,24 @@ static int ov5645_probe(struct i2c_client *client) > ret = ov5645_read_reg(ov5645, OV5645_AEC_PK_MANUAL, > &ov5645->aec_pk_manual); > if (ret < 0) { > - dev_err(dev, "could not read AEC/AGC mode\n"); > ret = -ENODEV; > + dev_err_probe(dev, ret, "could not read AEC/AGC mode\n"); > goto power_down; > } > > ret = ov5645_read_reg(ov5645, OV5645_TIMING_TC_REG20, > &ov5645->timing_tc_reg20); > if (ret < 0) { > - dev_err(dev, "could not read vflip value\n"); > ret = -ENODEV; > + dev_err_probe(dev, ret, "could not read vflip value\n"); > goto power_down; > } > > ret = ov5645_read_reg(ov5645, OV5645_TIMING_TC_REG21, > &ov5645->timing_tc_reg21); > if (ret < 0) { > - dev_err(dev, "could not read hflip value\n"); > ret = -ENODEV; > + dev_err_probe(dev, ret, "could not read hflip value\n"); > goto power_down; > } > > @@ -1243,7 +1225,7 @@ static int ov5645_probe(struct i2c_client *client) > > ret = v4l2_async_register_subdev(&ov5645->sd); > if (ret < 0) { > - dev_err(dev, "could not register v4l2 device\n"); > + dev_err_probe(dev, ret, "could not register v4l2 device\n"); > goto power_down; > } > The probe function looks really young, I think it would benefit from being broken down in multiple functions.
Hi Laurent, Thank you for the review. On Tue, Sep 24, 2024 at 11:44 PM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Prabhakar, > > Thank you for the patch. > > On Tue, Sep 10, 2024 at 06:06:03PM +0100, Prabhakar wrote: > > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > > > Drop dev_err() and use the dev_err_probe() helper on probe path. > > > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > --- > > drivers/media/i2c/ov5645.c | 74 +++++++++++++++----------------------- > > 1 file changed, 28 insertions(+), 46 deletions(-) > > > > diff --git a/drivers/media/i2c/ov5645.c b/drivers/media/i2c/ov5645.c > > index 78b86438c798..9e6ff1f1b9ac 100644 > > --- a/drivers/media/i2c/ov5645.c > > +++ b/drivers/media/i2c/ov5645.c > > @@ -1076,51 +1076,37 @@ static int ov5645_probe(struct i2c_client *client) > > ov5645->dev = dev; > > > > endpoint = of_graph_get_endpoint_by_regs(dev->of_node, 0, -1); > > - if (!endpoint) { > > - dev_err(dev, "endpoint node not found\n"); > > - return -EINVAL; > > - } > > + if (!endpoint) > > + return dev_err_probe(dev, -EINVAL, "endpoint node not found\n"); > > > > ret = v4l2_fwnode_endpoint_parse(of_fwnode_handle(endpoint), > > &ov5645->ep); > > > > of_node_put(endpoint); > > > > - if (ret < 0) { > > - dev_err(dev, "parsing endpoint node failed\n"); > > - return ret; > > - } > > + if (ret < 0) > > + return dev_err_probe(dev, ret, "parsing endpoint node failed\n"); > > > > - if (ov5645->ep.bus_type != V4L2_MBUS_CSI2_DPHY) { > > - dev_err(dev, "invalid bus type, must be CSI2\n"); > > - return -EINVAL; > > - } > > + if (ov5645->ep.bus_type != V4L2_MBUS_CSI2_DPHY) > > + return dev_err_probe(dev, -EINVAL, "invalid bus type, must be CSI2\n"); > > > > /* get system clock (xclk) */ > > ov5645->xclk = devm_clk_get(dev, NULL); > > - if (IS_ERR(ov5645->xclk)) { > > - dev_err(dev, "could not get xclk"); > > - return PTR_ERR(ov5645->xclk); > > - } > > + if (IS_ERR(ov5645->xclk)) > > + return dev_err_probe(dev, PTR_ERR(ov5645->xclk), "could not get xclk"); > > > > ret = of_property_read_u32(dev->of_node, "clock-frequency", &xclk_freq); > > - if (ret) { > > - dev_err(dev, "could not get xclk frequency\n"); > > - return ret; > > - } > > + if (ret) > > + return dev_err_probe(dev, ret, "could not get xclk frequency\n"); > > > > /* external clock must be 24MHz, allow 1% tolerance */ > > - if (xclk_freq < 23760000 || xclk_freq > 24240000) { > > - dev_err(dev, "external clock frequency %u is not supported\n", > > - xclk_freq); > > - return -EINVAL; > > - } > > + if (xclk_freq < 23760000 || xclk_freq > 24240000) > > + return dev_err_probe(dev, -EINVAL, "external clock frequency %u is not supported\n", > > + xclk_freq); > > > > ret = clk_set_rate(ov5645->xclk, xclk_freq); > > - if (ret) { > > - dev_err(dev, "could not set xclk frequency\n"); > > - return ret; > > - } > > + if (ret) > > + return dev_err_probe(dev, ret, "could not set xclk frequency\n"); > > > > for (i = 0; i < OV5645_NUM_SUPPLIES; i++) > > ov5645->supplies[i].supply = ov5645_supply_name[i]; > > @@ -1131,16 +1117,12 @@ static int ov5645_probe(struct i2c_client *client) > > return ret; > > > > ov5645->enable_gpio = devm_gpiod_get(dev, "enable", GPIOD_OUT_HIGH); > > - if (IS_ERR(ov5645->enable_gpio)) { > > - dev_err(dev, "cannot get enable gpio\n"); > > - return PTR_ERR(ov5645->enable_gpio); > > - } > > + if (IS_ERR(ov5645->enable_gpio)) > > + return dev_err_probe(dev, PTR_ERR(ov5645->enable_gpio), "cannot get enable gpio\n"); > > Those lines are getting long. We usually try to wrap at 80 columns for > sensor drivers: > As there will be a v3 anyway, I'll wrap it to 80 columns. > return dev_err_probe(dev, PTR_ERR(ov5645->enable_gpio), > "cannot get enable gpio\n"); > > Same elsewhere. I'll let Sakari decide. > > > > > ov5645->rst_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH); > > - if (IS_ERR(ov5645->rst_gpio)) { > > - dev_err(dev, "cannot get reset gpio\n"); > > - return PTR_ERR(ov5645->rst_gpio); > > - } > > + if (IS_ERR(ov5645->rst_gpio)) > > + return dev_err_probe(dev, PTR_ERR(ov5645->rst_gpio), "cannot get reset gpio\n"); > > > > mutex_init(&ov5645->power_lock); > > > > @@ -1177,9 +1159,9 @@ static int ov5645_probe(struct i2c_client *client) > > ov5645->sd.ctrl_handler = &ov5645->ctrls; > > > > if (ov5645->ctrls.error) { > > - dev_err(dev, "%s: control initialization error %d\n", > > - __func__, ov5645->ctrls.error); > > ret = ov5645->ctrls.error; > > + dev_err_probe(dev, ret, "%s: control initialization error %d\n", > > + __func__, ov5645->ctrls.error); > > goto free_ctrl; > > } > > > > @@ -1192,7 +1174,7 @@ static int ov5645_probe(struct i2c_client *client) > > > > ret = media_entity_pads_init(&ov5645->sd.entity, 1, &ov5645->pad); > > if (ret < 0) { > > - dev_err(dev, "could not register media entity\n"); > > + dev_err_probe(dev, ret, "could not register media entity\n"); > > goto free_ctrl; > > } > > > > @@ -1202,14 +1184,14 @@ static int ov5645_probe(struct i2c_client *client) > > > > ret = ov5645_read_reg(ov5645, OV5645_CHIP_ID_HIGH, &chip_id_high); > > if (ret < 0 || chip_id_high != OV5645_CHIP_ID_HIGH_BYTE) { > > - dev_err(dev, "could not read ID high\n"); > > ret = -ENODEV; > > + dev_err_probe(dev, ret, "could not read ID high\n"); > > goto power_down; > > } > > ret = ov5645_read_reg(ov5645, OV5645_CHIP_ID_LOW, &chip_id_low); > > if (ret < 0 || chip_id_low != OV5645_CHIP_ID_LOW_BYTE) { > > - dev_err(dev, "could not read ID low\n"); > > ret = -ENODEV; > > + dev_err_probe(dev, ret, "could not read ID low\n"); > > goto power_down; > > } > > > > @@ -1218,24 +1200,24 @@ static int ov5645_probe(struct i2c_client *client) > > ret = ov5645_read_reg(ov5645, OV5645_AEC_PK_MANUAL, > > &ov5645->aec_pk_manual); > > if (ret < 0) { > > - dev_err(dev, "could not read AEC/AGC mode\n"); > > ret = -ENODEV; > > + dev_err_probe(dev, ret, "could not read AEC/AGC mode\n"); > > goto power_down; > > } > > > > ret = ov5645_read_reg(ov5645, OV5645_TIMING_TC_REG20, > > &ov5645->timing_tc_reg20); > > if (ret < 0) { > > - dev_err(dev, "could not read vflip value\n"); > > ret = -ENODEV; > > + dev_err_probe(dev, ret, "could not read vflip value\n"); > > goto power_down; > > } > > > > ret = ov5645_read_reg(ov5645, OV5645_TIMING_TC_REG21, > > &ov5645->timing_tc_reg21); > > if (ret < 0) { > > - dev_err(dev, "could not read hflip value\n"); > > ret = -ENODEV; > > + dev_err_probe(dev, ret, "could not read hflip value\n"); > > goto power_down; > > } > > > > @@ -1243,7 +1225,7 @@ static int ov5645_probe(struct i2c_client *client) > > > > ret = v4l2_async_register_subdev(&ov5645->sd); > > if (ret < 0) { > > - dev_err(dev, "could not register v4l2 device\n"); > > + dev_err_probe(dev, ret, "could not register v4l2 device\n"); > > goto power_down; > > } > > > > The probe function looks really young, I think it would benefit from > being broken down in multiple functions. > I will add this once this initial series gets accepted. Cheers, Prabhakar
diff --git a/drivers/media/i2c/ov5645.c b/drivers/media/i2c/ov5645.c index 78b86438c798..9e6ff1f1b9ac 100644 --- a/drivers/media/i2c/ov5645.c +++ b/drivers/media/i2c/ov5645.c @@ -1076,51 +1076,37 @@ static int ov5645_probe(struct i2c_client *client) ov5645->dev = dev; endpoint = of_graph_get_endpoint_by_regs(dev->of_node, 0, -1); - if (!endpoint) { - dev_err(dev, "endpoint node not found\n"); - return -EINVAL; - } + if (!endpoint) + return dev_err_probe(dev, -EINVAL, "endpoint node not found\n"); ret = v4l2_fwnode_endpoint_parse(of_fwnode_handle(endpoint), &ov5645->ep); of_node_put(endpoint); - if (ret < 0) { - dev_err(dev, "parsing endpoint node failed\n"); - return ret; - } + if (ret < 0) + return dev_err_probe(dev, ret, "parsing endpoint node failed\n"); - if (ov5645->ep.bus_type != V4L2_MBUS_CSI2_DPHY) { - dev_err(dev, "invalid bus type, must be CSI2\n"); - return -EINVAL; - } + if (ov5645->ep.bus_type != V4L2_MBUS_CSI2_DPHY) + return dev_err_probe(dev, -EINVAL, "invalid bus type, must be CSI2\n"); /* get system clock (xclk) */ ov5645->xclk = devm_clk_get(dev, NULL); - if (IS_ERR(ov5645->xclk)) { - dev_err(dev, "could not get xclk"); - return PTR_ERR(ov5645->xclk); - } + if (IS_ERR(ov5645->xclk)) + return dev_err_probe(dev, PTR_ERR(ov5645->xclk), "could not get xclk"); ret = of_property_read_u32(dev->of_node, "clock-frequency", &xclk_freq); - if (ret) { - dev_err(dev, "could not get xclk frequency\n"); - return ret; - } + if (ret) + return dev_err_probe(dev, ret, "could not get xclk frequency\n"); /* external clock must be 24MHz, allow 1% tolerance */ - if (xclk_freq < 23760000 || xclk_freq > 24240000) { - dev_err(dev, "external clock frequency %u is not supported\n", - xclk_freq); - return -EINVAL; - } + if (xclk_freq < 23760000 || xclk_freq > 24240000) + return dev_err_probe(dev, -EINVAL, "external clock frequency %u is not supported\n", + xclk_freq); ret = clk_set_rate(ov5645->xclk, xclk_freq); - if (ret) { - dev_err(dev, "could not set xclk frequency\n"); - return ret; - } + if (ret) + return dev_err_probe(dev, ret, "could not set xclk frequency\n"); for (i = 0; i < OV5645_NUM_SUPPLIES; i++) ov5645->supplies[i].supply = ov5645_supply_name[i]; @@ -1131,16 +1117,12 @@ static int ov5645_probe(struct i2c_client *client) return ret; ov5645->enable_gpio = devm_gpiod_get(dev, "enable", GPIOD_OUT_HIGH); - if (IS_ERR(ov5645->enable_gpio)) { - dev_err(dev, "cannot get enable gpio\n"); - return PTR_ERR(ov5645->enable_gpio); - } + if (IS_ERR(ov5645->enable_gpio)) + return dev_err_probe(dev, PTR_ERR(ov5645->enable_gpio), "cannot get enable gpio\n"); ov5645->rst_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH); - if (IS_ERR(ov5645->rst_gpio)) { - dev_err(dev, "cannot get reset gpio\n"); - return PTR_ERR(ov5645->rst_gpio); - } + if (IS_ERR(ov5645->rst_gpio)) + return dev_err_probe(dev, PTR_ERR(ov5645->rst_gpio), "cannot get reset gpio\n"); mutex_init(&ov5645->power_lock); @@ -1177,9 +1159,9 @@ static int ov5645_probe(struct i2c_client *client) ov5645->sd.ctrl_handler = &ov5645->ctrls; if (ov5645->ctrls.error) { - dev_err(dev, "%s: control initialization error %d\n", - __func__, ov5645->ctrls.error); ret = ov5645->ctrls.error; + dev_err_probe(dev, ret, "%s: control initialization error %d\n", + __func__, ov5645->ctrls.error); goto free_ctrl; } @@ -1192,7 +1174,7 @@ static int ov5645_probe(struct i2c_client *client) ret = media_entity_pads_init(&ov5645->sd.entity, 1, &ov5645->pad); if (ret < 0) { - dev_err(dev, "could not register media entity\n"); + dev_err_probe(dev, ret, "could not register media entity\n"); goto free_ctrl; } @@ -1202,14 +1184,14 @@ static int ov5645_probe(struct i2c_client *client) ret = ov5645_read_reg(ov5645, OV5645_CHIP_ID_HIGH, &chip_id_high); if (ret < 0 || chip_id_high != OV5645_CHIP_ID_HIGH_BYTE) { - dev_err(dev, "could not read ID high\n"); ret = -ENODEV; + dev_err_probe(dev, ret, "could not read ID high\n"); goto power_down; } ret = ov5645_read_reg(ov5645, OV5645_CHIP_ID_LOW, &chip_id_low); if (ret < 0 || chip_id_low != OV5645_CHIP_ID_LOW_BYTE) { - dev_err(dev, "could not read ID low\n"); ret = -ENODEV; + dev_err_probe(dev, ret, "could not read ID low\n"); goto power_down; } @@ -1218,24 +1200,24 @@ static int ov5645_probe(struct i2c_client *client) ret = ov5645_read_reg(ov5645, OV5645_AEC_PK_MANUAL, &ov5645->aec_pk_manual); if (ret < 0) { - dev_err(dev, "could not read AEC/AGC mode\n"); ret = -ENODEV; + dev_err_probe(dev, ret, "could not read AEC/AGC mode\n"); goto power_down; } ret = ov5645_read_reg(ov5645, OV5645_TIMING_TC_REG20, &ov5645->timing_tc_reg20); if (ret < 0) { - dev_err(dev, "could not read vflip value\n"); ret = -ENODEV; + dev_err_probe(dev, ret, "could not read vflip value\n"); goto power_down; } ret = ov5645_read_reg(ov5645, OV5645_TIMING_TC_REG21, &ov5645->timing_tc_reg21); if (ret < 0) { - dev_err(dev, "could not read hflip value\n"); ret = -ENODEV; + dev_err_probe(dev, ret, "could not read hflip value\n"); goto power_down; } @@ -1243,7 +1225,7 @@ static int ov5645_probe(struct i2c_client *client) ret = v4l2_async_register_subdev(&ov5645->sd); if (ret < 0) { - dev_err(dev, "could not register v4l2 device\n"); + dev_err_probe(dev, ret, "could not register v4l2 device\n"); goto power_down; }