| Message ID | 20230114171802.13878-8-laurent.pinchart@ideasonboard.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: i2c: imx290: Miscellaneous improvements | expand |
Hi Laurent, thanks for the update. Am Samstag, 14. Januar 2023, 18:17:53 CET schrieb Laurent Pinchart: > Make the probe() function more readable by factoring out the DT parsing > code to a separate function. No functional change intended. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > drivers/media/i2c/imx290.c | 95 +++++++++++++++++++++----------------- > 1 file changed, 52 insertions(+), 43 deletions(-) > > diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c > index 0dc536893ebf..18c1e5c429a2 100644 > --- a/drivers/media/i2c/imx290.c > +++ b/drivers/media/i2c/imx290.c > @@ -1144,111 +1144,124 @@ static s64 imx290_check_link_freqs(const struct > imx290 *imx290, return 0; > } > > -static int imx290_probe(struct i2c_client *client) > +static int imx290_parse_dt(struct imx290 *imx290) > { > - struct device *dev = &client->dev; > - struct fwnode_handle *endpoint; > /* Only CSI2 is supported for now: */ > struct v4l2_fwnode_endpoint ep = { > .bus_type = V4L2_MBUS_CSI2_DPHY > }; > - struct imx290 *imx290; > - u32 xclk_freq; > + struct fwnode_handle *endpoint; > + int ret; > s64 fq; > - int ret; > > - imx290 = devm_kzalloc(dev, sizeof(*imx290), GFP_KERNEL); > - if (!imx290) > - return -ENOMEM; > - > - imx290->dev = dev; > - imx290->regmap = devm_regmap_init_i2c(client, &imx290_regmap_config); > - if (IS_ERR(imx290->regmap)) { > - dev_err(dev, "Unable to initialize I2C\n"); > - return -ENODEV; > - } > - > - endpoint = fwnode_graph_get_next_endpoint(dev_fwnode(dev), NULL); > + endpoint = fwnode_graph_get_next_endpoint(dev_fwnode(imx290->dev), NULL); > if (!endpoint) { > - dev_err(dev, "Endpoint node not found\n"); > + dev_err(imx290->dev, "Endpoint node not found\n"); > return -EINVAL; > } > > ret = v4l2_fwnode_endpoint_alloc_parse(endpoint, &ep); > fwnode_handle_put(endpoint); > if (ret == -ENXIO) { > - dev_err(dev, "Unsupported bus type, should be CSI2\n"); > - goto err_endpoint; > + dev_err(imx290->dev, "Unsupported bus type, should be CSI2\n"); > + goto done; > } else if (ret) { > - dev_err(dev, "Parsing endpoint node failed\n"); > - goto err_endpoint; > + dev_err(imx290->dev, "Parsing endpoint node failed\n"); > + goto done; > } > > /* Get number of data lanes */ > imx290->nlanes = ep.bus.mipi_csi2.num_data_lanes; > if (imx290->nlanes != 2 && imx290->nlanes != 4) { > - dev_err(dev, "Invalid data lanes: %d\n", imx290->nlanes); > + dev_err(imx290->dev, "Invalid data lanes: %d\n", imx290- >nlanes); > ret = -EINVAL; > - goto err_endpoint; > + goto done; > } > > - dev_dbg(dev, "Using %u data lanes\n", imx290->nlanes); > + dev_dbg(imx290->dev, "Using %u data lanes\n", imx290->nlanes); > > if (!ep.nr_of_link_frequencies) { > - dev_err(dev, "link-frequency property not found in DT\n"); > + dev_err(imx290->dev, "link-frequency property not found in DT\n"); > ret = -EINVAL; > - goto err_endpoint; > + goto done; > } > > /* Check that link frequences for all the modes are in device tree */ > fq = imx290_check_link_freqs(imx290, &ep); > if (fq) { > - dev_err(dev, "Link frequency of %lld is not supported\n", fq); > + dev_err(imx290->dev, "Link frequency of %lld is not supported\n", > + fq); > ret = -EINVAL; > - goto err_endpoint; > + goto done; > } > > + ret = 0; > + > +done: > + v4l2_fwnode_endpoint_free(&ep); > + return ret; > +} > + > +static int imx290_probe(struct i2c_client *client) > +{ > + struct device *dev = &client->dev; > + struct imx290 *imx290; > + u32 xclk_freq; > + int ret; > + > + imx290 = devm_kzalloc(dev, sizeof(*imx290), GFP_KERNEL); > + if (!imx290) > + return -ENOMEM; > + > + imx290->dev = dev; > + imx290->regmap = devm_regmap_init_i2c(client, &imx290_regmap_config); > + if (IS_ERR(imx290->regmap)) { > + dev_err(dev, "Unable to initialize I2C\n"); > + return -ENODEV; > + } > + > + ret = imx290_parse_dt(imx290); > + if (ret) > + return ret; > + > /* get system clock (xclk) */ > imx290->xclk = devm_clk_get(dev, "xclk"); > if (IS_ERR(imx290->xclk)) { > dev_err(dev, "Could not get xclk"); > - ret = PTR_ERR(imx290->xclk); > - goto err_endpoint; > + return PTR_ERR(imx290->xclk); Please use dev_err_probe() here. > } > > ret = fwnode_property_read_u32(dev_fwnode(dev), "clock-frequency", > &xclk_freq); > if (ret) { > dev_err(dev, "Could not get xclk frequency\n"); > - goto err_endpoint; > + return ret; > } > > /* external clock must be 37.125 MHz */ > if (xclk_freq != 37125000) { > dev_err(dev, "External clock frequency %u is not supported\n", > xclk_freq); > - ret = -EINVAL; > - goto err_endpoint; > + return -EINVAL; > } > > ret = clk_set_rate(imx290->xclk, xclk_freq); > if (ret) { > dev_err(dev, "Could not set xclk frequency\n"); > - goto err_endpoint; > + return ret; > } > > ret = imx290_get_regulators(dev, imx290); > if (ret < 0) { > dev_err(dev, "Cannot get regulators\n"); > - goto err_endpoint; > + return ret; Please use dev_err_probe() here. Regards, Alexander > } > > imx290->rst_gpio = devm_gpiod_get_optional(dev, "reset", > GPIOD_OUT_HIGH); > if (IS_ERR(imx290->rst_gpio)) { > dev_err(dev, "Cannot get reset gpio\n"); > - ret = PTR_ERR(imx290->rst_gpio); > - goto err_endpoint; > + return PTR_ERR(imx290->rst_gpio); > } > > mutex_init(&imx290->lock); > @@ -1274,16 +1287,12 @@ static int imx290_probe(struct i2c_client *client) > pm_runtime_enable(dev); > pm_runtime_idle(dev); > > - v4l2_fwnode_endpoint_free(&ep); > - > return 0; > > err_subdev: > imx290_subdev_cleanup(imx290); > err_mutex: > mutex_destroy(&imx290->lock); > -err_endpoint: > - v4l2_fwnode_endpoint_free(&ep); > > return ret; > }
Hi Alexander, On Mon, Jan 16, 2023 at 11:33:47AM +0100, Alexander Stein wrote: > Am Samstag, 14. Januar 2023, 18:17:53 CET schrieb Laurent Pinchart: > > Make the probe() function more readable by factoring out the DT parsing > > code to a separate function. No functional change intended. > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > --- > > drivers/media/i2c/imx290.c | 95 +++++++++++++++++++++----------------- > > 1 file changed, 52 insertions(+), 43 deletions(-) > > > > diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c > > index 0dc536893ebf..18c1e5c429a2 100644 > > --- a/drivers/media/i2c/imx290.c > > +++ b/drivers/media/i2c/imx290.c > > @@ -1144,111 +1144,124 @@ static s64 imx290_check_link_freqs(const struct > > imx290 *imx290, return 0; > > } > > > > -static int imx290_probe(struct i2c_client *client) > > +static int imx290_parse_dt(struct imx290 *imx290) > > { > > - struct device *dev = &client->dev; > > - struct fwnode_handle *endpoint; > > /* Only CSI2 is supported for now: */ > > struct v4l2_fwnode_endpoint ep = { > > .bus_type = V4L2_MBUS_CSI2_DPHY > > }; > > - struct imx290 *imx290; > > - u32 xclk_freq; > > + struct fwnode_handle *endpoint; > > + int ret; > > s64 fq; > > - int ret; > > > > - imx290 = devm_kzalloc(dev, sizeof(*imx290), GFP_KERNEL); > > - if (!imx290) > > - return -ENOMEM; > > - > > - imx290->dev = dev; > > - imx290->regmap = devm_regmap_init_i2c(client, &imx290_regmap_config); > > - if (IS_ERR(imx290->regmap)) { > > - dev_err(dev, "Unable to initialize I2C\n"); > > - return -ENODEV; > > - } > > - > > - endpoint = fwnode_graph_get_next_endpoint(dev_fwnode(dev), NULL); > > + endpoint = fwnode_graph_get_next_endpoint(dev_fwnode(imx290->dev), NULL); > > if (!endpoint) { > > - dev_err(dev, "Endpoint node not found\n"); > > + dev_err(imx290->dev, "Endpoint node not found\n"); > > return -EINVAL; > > } > > > > ret = v4l2_fwnode_endpoint_alloc_parse(endpoint, &ep); > > fwnode_handle_put(endpoint); > > if (ret == -ENXIO) { > > - dev_err(dev, "Unsupported bus type, should be CSI2\n"); > > - goto err_endpoint; > > + dev_err(imx290->dev, "Unsupported bus type, should be CSI2\n"); > > + goto done; > > } else if (ret) { > > - dev_err(dev, "Parsing endpoint node failed\n"); > > - goto err_endpoint; > > + dev_err(imx290->dev, "Parsing endpoint node failed\n"); > > + goto done; > > } > > > > /* Get number of data lanes */ > > imx290->nlanes = ep.bus.mipi_csi2.num_data_lanes; > > if (imx290->nlanes != 2 && imx290->nlanes != 4) { > > - dev_err(dev, "Invalid data lanes: %d\n", imx290->nlanes); > > + dev_err(imx290->dev, "Invalid data lanes: %d\n", imx290- > >nlanes); > > ret = -EINVAL; > > - goto err_endpoint; > > + goto done; > > } > > > > - dev_dbg(dev, "Using %u data lanes\n", imx290->nlanes); > > + dev_dbg(imx290->dev, "Using %u data lanes\n", imx290->nlanes); > > > > if (!ep.nr_of_link_frequencies) { > > - dev_err(dev, "link-frequency property not found in DT\n"); > > + dev_err(imx290->dev, "link-frequency property not found in DT\n"); > > ret = -EINVAL; > > - goto err_endpoint; > > + goto done; > > } > > > > /* Check that link frequences for all the modes are in device tree */ > > fq = imx290_check_link_freqs(imx290, &ep); > > if (fq) { > > - dev_err(dev, "Link frequency of %lld is not supported\n", fq); > > + dev_err(imx290->dev, "Link frequency of %lld is not supported\n", > > + fq); > > ret = -EINVAL; > > - goto err_endpoint; > > + goto done; > > } > > > > + ret = 0; > > + > > +done: > > + v4l2_fwnode_endpoint_free(&ep); > > + return ret; > > +} > > + > > +static int imx290_probe(struct i2c_client *client) > > +{ > > + struct device *dev = &client->dev; > > + struct imx290 *imx290; > > + u32 xclk_freq; > > + int ret; > > + > > + imx290 = devm_kzalloc(dev, sizeof(*imx290), GFP_KERNEL); > > + if (!imx290) > > + return -ENOMEM; > > + > > + imx290->dev = dev; > > + imx290->regmap = devm_regmap_init_i2c(client, &imx290_regmap_config); > > + if (IS_ERR(imx290->regmap)) { > > + dev_err(dev, "Unable to initialize I2C\n"); > > + return -ENODEV; > > + } > > + > > + ret = imx290_parse_dt(imx290); > > + if (ret) > > + return ret; > > + > > /* get system clock (xclk) */ > > imx290->xclk = devm_clk_get(dev, "xclk"); > > if (IS_ERR(imx290->xclk)) { > > dev_err(dev, "Could not get xclk"); > > - ret = PTR_ERR(imx290->xclk); > > - goto err_endpoint; > > + return PTR_ERR(imx290->xclk); > > Please use dev_err_probe() here. It's done in the next patch in this series, which converts the driver to dev_err_probe(). This patch only factors out code. > > } > > > > ret = fwnode_property_read_u32(dev_fwnode(dev), "clock-frequency", > > &xclk_freq); > > if (ret) { > > dev_err(dev, "Could not get xclk frequency\n"); > > - goto err_endpoint; > > + return ret; > > } > > > > /* external clock must be 37.125 MHz */ > > if (xclk_freq != 37125000) { > > dev_err(dev, "External clock frequency %u is not supported\n", > > xclk_freq); > > - ret = -EINVAL; > > - goto err_endpoint; > > + return -EINVAL; > > } > > > > ret = clk_set_rate(imx290->xclk, xclk_freq); > > if (ret) { > > dev_err(dev, "Could not set xclk frequency\n"); > > - goto err_endpoint; > > + return ret; > > } > > > > ret = imx290_get_regulators(dev, imx290); > > if (ret < 0) { > > dev_err(dev, "Cannot get regulators\n"); > > - goto err_endpoint; > > + return ret; > > Please use dev_err_probe() here. > > > } > > > > imx290->rst_gpio = devm_gpiod_get_optional(dev, "reset", > > GPIOD_OUT_HIGH); > > if (IS_ERR(imx290->rst_gpio)) { > > dev_err(dev, "Cannot get reset gpio\n"); > > - ret = PTR_ERR(imx290->rst_gpio); > > - goto err_endpoint; > > + return PTR_ERR(imx290->rst_gpio); > > } > > > > mutex_init(&imx290->lock); > > @@ -1274,16 +1287,12 @@ static int imx290_probe(struct i2c_client *client) > > pm_runtime_enable(dev); > > pm_runtime_idle(dev); > > > > - v4l2_fwnode_endpoint_free(&ep); > > - > > return 0; > > > > err_subdev: > > imx290_subdev_cleanup(imx290); > > err_mutex: > > mutex_destroy(&imx290->lock); > > -err_endpoint: > > - v4l2_fwnode_endpoint_free(&ep); > > > > return ret; > > }
Hi Laurent, Am Montag, 16. Januar 2023, 11:48:28 CET schrieb Laurent Pinchart: > Hi Alexander, > > On Mon, Jan 16, 2023 at 11:33:47AM +0100, Alexander Stein wrote: > > Am Samstag, 14. Januar 2023, 18:17:53 CET schrieb Laurent Pinchart: > > > Make the probe() function more readable by factoring out the DT parsing > > > code to a separate function. No functional change intended. > > > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > --- > > > > > > drivers/media/i2c/imx290.c | 95 +++++++++++++++++++++----------------- > > > 1 file changed, 52 insertions(+), 43 deletions(-) > > > > > > diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c > > > index 0dc536893ebf..18c1e5c429a2 100644 > > > --- a/drivers/media/i2c/imx290.c > > > +++ b/drivers/media/i2c/imx290.c > > > @@ -1144,111 +1144,124 @@ static s64 imx290_check_link_freqs(const > > > struct > > > imx290 *imx290, return 0; > > > > > > } > > > > > > -static int imx290_probe(struct i2c_client *client) > > > +static int imx290_parse_dt(struct imx290 *imx290) > > > > > > { > > > > > > - struct device *dev = &client->dev; > > > - struct fwnode_handle *endpoint; > > > > > > /* Only CSI2 is supported for now: */ > > > struct v4l2_fwnode_endpoint ep = { > > > > > > .bus_type = V4L2_MBUS_CSI2_DPHY > > > > > > }; > > > > > > - struct imx290 *imx290; > > > - u32 xclk_freq; > > > + struct fwnode_handle *endpoint; > > > + int ret; > > > > > > s64 fq; > > > > > > - int ret; > > > > > > - imx290 = devm_kzalloc(dev, sizeof(*imx290), GFP_KERNEL); > > > - if (!imx290) > > > - return -ENOMEM; > > > - > > > - imx290->dev = dev; > > > - imx290->regmap = devm_regmap_init_i2c(client, &imx290_regmap_config); > > > - if (IS_ERR(imx290->regmap)) { > > > - dev_err(dev, "Unable to initialize I2C\n"); > > > - return -ENODEV; > > > - } > > > - > > > - endpoint = fwnode_graph_get_next_endpoint(dev_fwnode(dev), NULL); > > > + endpoint = fwnode_graph_get_next_endpoint(dev_fwnode(imx290->dev), > > > NULL); > > > > > > if (!endpoint) { > > > > > > - dev_err(dev, "Endpoint node not found\n"); > > > + dev_err(imx290->dev, "Endpoint node not found\n"); > > > > > > return -EINVAL; > > > > > > } > > > > > > ret = v4l2_fwnode_endpoint_alloc_parse(endpoint, &ep); > > > fwnode_handle_put(endpoint); > > > if (ret == -ENXIO) { > > > > > > - dev_err(dev, "Unsupported bus type, should be CSI2\n"); > > > - goto err_endpoint; > > > + dev_err(imx290->dev, "Unsupported bus type, should be CSI2\n"); > > > + goto done; > > > > > > } else if (ret) { > > > > > > - dev_err(dev, "Parsing endpoint node failed\n"); > > > - goto err_endpoint; > > > + dev_err(imx290->dev, "Parsing endpoint node failed\n"); > > > + goto done; > > > > > > } > > > > > > /* Get number of data lanes */ > > > imx290->nlanes = ep.bus.mipi_csi2.num_data_lanes; > > > if (imx290->nlanes != 2 && imx290->nlanes != 4) { > > > > > > - dev_err(dev, "Invalid data lanes: %d\n", imx290->nlanes); > > > + dev_err(imx290->dev, "Invalid data lanes: %d\n", imx290- > > > > > >nlanes); > > > > > > ret = -EINVAL; > > > > > > - goto err_endpoint; > > > + goto done; > > > > > > } > > > > > > - dev_dbg(dev, "Using %u data lanes\n", imx290->nlanes); > > > + dev_dbg(imx290->dev, "Using %u data lanes\n", imx290->nlanes); > > > > > > if (!ep.nr_of_link_frequencies) { > > > > > > - dev_err(dev, "link-frequency property not found in DT\n"); > > > + dev_err(imx290->dev, "link-frequency property not found in DT\n"); > > > > > > ret = -EINVAL; > > > > > > - goto err_endpoint; > > > + goto done; > > > > > > } > > > > > > /* Check that link frequences for all the modes are in device tree */ > > > fq = imx290_check_link_freqs(imx290, &ep); > > > if (fq) { > > > > > > - dev_err(dev, "Link frequency of %lld is not supported\n", fq); > > > + dev_err(imx290->dev, "Link frequency of %lld is not supported\n", > > > + fq); > > > > > > ret = -EINVAL; > > > > > > - goto err_endpoint; > > > + goto done; > > > > > > } > > > > > > + ret = 0; > > > + > > > +done: > > > + v4l2_fwnode_endpoint_free(&ep); > > > + return ret; > > > +} > > > + > > > +static int imx290_probe(struct i2c_client *client) > > > +{ > > > + struct device *dev = &client->dev; > > > + struct imx290 *imx290; > > > + u32 xclk_freq; > > > + int ret; > > > + > > > + imx290 = devm_kzalloc(dev, sizeof(*imx290), GFP_KERNEL); > > > + if (!imx290) > > > + return -ENOMEM; > > > + > > > + imx290->dev = dev; > > > + imx290->regmap = devm_regmap_init_i2c(client, &imx290_regmap_config); > > > + if (IS_ERR(imx290->regmap)) { > > > + dev_err(dev, "Unable to initialize I2C\n"); > > > + return -ENODEV; > > > + } > > > + > > > + ret = imx290_parse_dt(imx290); > > > + if (ret) > > > + return ret; > > > + > > > > > > /* get system clock (xclk) */ > > > imx290->xclk = devm_clk_get(dev, "xclk"); > > > if (IS_ERR(imx290->xclk)) { > > > > > > dev_err(dev, "Could not get xclk"); > > > > > > - ret = PTR_ERR(imx290->xclk); > > > - goto err_endpoint; > > > + return PTR_ERR(imx290->xclk); > > > > Please use dev_err_probe() here. > > It's done in the next patch in this series, which converts the driver to > dev_err_probe(). This patch only factors out code. I just noticed :-/ Reviewed-by: Alexander Stein <alexander.stein@ew.tq-group.com> > > > } > > > > > > ret = fwnode_property_read_u32(dev_fwnode(dev), "clock-frequency", > > > > > > &xclk_freq); > > > > > > if (ret) { > > > > > > dev_err(dev, "Could not get xclk frequency\n"); > > > > > > - goto err_endpoint; > > > + return ret; > > > > > > } > > > > > > /* external clock must be 37.125 MHz */ > > > if (xclk_freq != 37125000) { > > > > > > dev_err(dev, "External clock frequency %u is not supported\n", > > > > > > xclk_freq); > > > > > > - ret = -EINVAL; > > > - goto err_endpoint; > > > + return -EINVAL; > > > > > > } > > > > > > ret = clk_set_rate(imx290->xclk, xclk_freq); > > > if (ret) { > > > > > > dev_err(dev, "Could not set xclk frequency\n"); > > > > > > - goto err_endpoint; > > > + return ret; > > > > > > } > > > > > > ret = imx290_get_regulators(dev, imx290); > > > if (ret < 0) { > > > > > > dev_err(dev, "Cannot get regulators\n"); > > > > > > - goto err_endpoint; > > > + return ret; > > > > Please use dev_err_probe() here. > > > > > } > > > > > > imx290->rst_gpio = devm_gpiod_get_optional(dev, "reset", > > > > > > GPIOD_OUT_HIGH); > > > > > > if (IS_ERR(imx290->rst_gpio)) { > > > > > > dev_err(dev, "Cannot get reset gpio\n"); > > > > > > - ret = PTR_ERR(imx290->rst_gpio); > > > - goto err_endpoint; > > > + return PTR_ERR(imx290->rst_gpio); > > > > > > } > > > > > > mutex_init(&imx290->lock); > > > > > > @@ -1274,16 +1287,12 @@ static int imx290_probe(struct i2c_client > > > *client) > > > > > > pm_runtime_enable(dev); > > > pm_runtime_idle(dev); > > > > > > - v4l2_fwnode_endpoint_free(&ep); > > > - > > > > > > return 0; > > > > > > err_subdev: > > > imx290_subdev_cleanup(imx290); > > > > > > err_mutex: > > > mutex_destroy(&imx290->lock); > > > > > > -err_endpoint: > > > - v4l2_fwnode_endpoint_free(&ep); > > > > > > return ret; > > > > > > }
diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c index 0dc536893ebf..18c1e5c429a2 100644 --- a/drivers/media/i2c/imx290.c +++ b/drivers/media/i2c/imx290.c @@ -1144,111 +1144,124 @@ static s64 imx290_check_link_freqs(const struct imx290 *imx290, return 0; } -static int imx290_probe(struct i2c_client *client) +static int imx290_parse_dt(struct imx290 *imx290) { - struct device *dev = &client->dev; - struct fwnode_handle *endpoint; /* Only CSI2 is supported for now: */ struct v4l2_fwnode_endpoint ep = { .bus_type = V4L2_MBUS_CSI2_DPHY }; - struct imx290 *imx290; - u32 xclk_freq; + struct fwnode_handle *endpoint; + int ret; s64 fq; - int ret; - imx290 = devm_kzalloc(dev, sizeof(*imx290), GFP_KERNEL); - if (!imx290) - return -ENOMEM; - - imx290->dev = dev; - imx290->regmap = devm_regmap_init_i2c(client, &imx290_regmap_config); - if (IS_ERR(imx290->regmap)) { - dev_err(dev, "Unable to initialize I2C\n"); - return -ENODEV; - } - - endpoint = fwnode_graph_get_next_endpoint(dev_fwnode(dev), NULL); + endpoint = fwnode_graph_get_next_endpoint(dev_fwnode(imx290->dev), NULL); if (!endpoint) { - dev_err(dev, "Endpoint node not found\n"); + dev_err(imx290->dev, "Endpoint node not found\n"); return -EINVAL; } ret = v4l2_fwnode_endpoint_alloc_parse(endpoint, &ep); fwnode_handle_put(endpoint); if (ret == -ENXIO) { - dev_err(dev, "Unsupported bus type, should be CSI2\n"); - goto err_endpoint; + dev_err(imx290->dev, "Unsupported bus type, should be CSI2\n"); + goto done; } else if (ret) { - dev_err(dev, "Parsing endpoint node failed\n"); - goto err_endpoint; + dev_err(imx290->dev, "Parsing endpoint node failed\n"); + goto done; } /* Get number of data lanes */ imx290->nlanes = ep.bus.mipi_csi2.num_data_lanes; if (imx290->nlanes != 2 && imx290->nlanes != 4) { - dev_err(dev, "Invalid data lanes: %d\n", imx290->nlanes); + dev_err(imx290->dev, "Invalid data lanes: %d\n", imx290->nlanes); ret = -EINVAL; - goto err_endpoint; + goto done; } - dev_dbg(dev, "Using %u data lanes\n", imx290->nlanes); + dev_dbg(imx290->dev, "Using %u data lanes\n", imx290->nlanes); if (!ep.nr_of_link_frequencies) { - dev_err(dev, "link-frequency property not found in DT\n"); + dev_err(imx290->dev, "link-frequency property not found in DT\n"); ret = -EINVAL; - goto err_endpoint; + goto done; } /* Check that link frequences for all the modes are in device tree */ fq = imx290_check_link_freqs(imx290, &ep); if (fq) { - dev_err(dev, "Link frequency of %lld is not supported\n", fq); + dev_err(imx290->dev, "Link frequency of %lld is not supported\n", + fq); ret = -EINVAL; - goto err_endpoint; + goto done; } + ret = 0; + +done: + v4l2_fwnode_endpoint_free(&ep); + return ret; +} + +static int imx290_probe(struct i2c_client *client) +{ + struct device *dev = &client->dev; + struct imx290 *imx290; + u32 xclk_freq; + int ret; + + imx290 = devm_kzalloc(dev, sizeof(*imx290), GFP_KERNEL); + if (!imx290) + return -ENOMEM; + + imx290->dev = dev; + imx290->regmap = devm_regmap_init_i2c(client, &imx290_regmap_config); + if (IS_ERR(imx290->regmap)) { + dev_err(dev, "Unable to initialize I2C\n"); + return -ENODEV; + } + + ret = imx290_parse_dt(imx290); + if (ret) + return ret; + /* get system clock (xclk) */ imx290->xclk = devm_clk_get(dev, "xclk"); if (IS_ERR(imx290->xclk)) { dev_err(dev, "Could not get xclk"); - ret = PTR_ERR(imx290->xclk); - goto err_endpoint; + return PTR_ERR(imx290->xclk); } ret = fwnode_property_read_u32(dev_fwnode(dev), "clock-frequency", &xclk_freq); if (ret) { dev_err(dev, "Could not get xclk frequency\n"); - goto err_endpoint; + return ret; } /* external clock must be 37.125 MHz */ if (xclk_freq != 37125000) { dev_err(dev, "External clock frequency %u is not supported\n", xclk_freq); - ret = -EINVAL; - goto err_endpoint; + return -EINVAL; } ret = clk_set_rate(imx290->xclk, xclk_freq); if (ret) { dev_err(dev, "Could not set xclk frequency\n"); - goto err_endpoint; + return ret; } ret = imx290_get_regulators(dev, imx290); if (ret < 0) { dev_err(dev, "Cannot get regulators\n"); - goto err_endpoint; + return ret; } imx290->rst_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH); if (IS_ERR(imx290->rst_gpio)) { dev_err(dev, "Cannot get reset gpio\n"); - ret = PTR_ERR(imx290->rst_gpio); - goto err_endpoint; + return PTR_ERR(imx290->rst_gpio); } mutex_init(&imx290->lock); @@ -1274,16 +1287,12 @@ static int imx290_probe(struct i2c_client *client) pm_runtime_enable(dev); pm_runtime_idle(dev); - v4l2_fwnode_endpoint_free(&ep); - return 0; err_subdev: imx290_subdev_cleanup(imx290); err_mutex: mutex_destroy(&imx290->lock); -err_endpoint: - v4l2_fwnode_endpoint_free(&ep); return ret; }
Make the probe() function more readable by factoring out the DT parsing code to a separate function. No functional change intended. Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- drivers/media/i2c/imx290.c | 95 +++++++++++++++++++++----------------- 1 file changed, 52 insertions(+), 43 deletions(-)