Hi Daniel
On Tue, 15 Feb 2022 at 23:08, Daniel Scally <djrscally@xxxxxxxxx> wrote:
>
> Move the endpoint checking from .probe() to a dedicated function,
> and additionally check that the firmware provided link frequencies
> are a match for those supported by the driver. Finally, return
> -EPROBE_DEFER if the endpoint is not available, as it could be built
> by the ipu3-cio2 driver if that probes later.
>
> Signed-off-by: Daniel Scally <djrscally@xxxxxxxxx>
> ---
> drivers/media/i2c/ov7251.c | 84 ++++++++++++++++++++++++++++++--------
> 1 file changed, 66 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/media/i2c/ov7251.c b/drivers/media/i2c/ov7251.c
> index d6fe574cb9e0..5c5f7a15a640 100644
> --- a/drivers/media/i2c/ov7251.c
> +++ b/drivers/media/i2c/ov7251.c
> @@ -1255,10 +1255,73 @@ static const struct v4l2_subdev_ops ov7251_subdev_ops = {
> .pad = &ov7251_subdev_pad_ops,
> };
>
> +static int ov7251_check_hwcfg(struct ov7251 *ov7251)
> +{
> + struct fwnode_handle *fwnode = dev_fwnode(ov7251->dev);
> + struct v4l2_fwnode_endpoint bus_cfg = {
> + .bus_type = V4L2_MBUS_CSI2_DPHY,
> + };
> + struct fwnode_handle *endpoint;
> + bool freq_found;
> + int i, j;
> + int ret;
> +
> + endpoint = fwnode_graph_get_next_endpoint(fwnode, NULL);
> + if (!endpoint)
> + return -EPROBE_DEFER; /* could be provided by cio2-bridge */
> +
> + ret = v4l2_fwnode_endpoint_alloc_parse(endpoint, &bus_cfg);
> + fwnode_handle_put(endpoint);
> + if (ret)
> + return dev_err_probe(ov7251->dev, ret,
> + "parsing endpoint node failed\n");
> +
> + if (bus_cfg.bus_type != V4L2_MBUS_CSI2_DPHY) {
> + ret = -EINVAL;
> + dev_err(ov7251->dev, "invalid bus type (%u), must be (%u)\n",
> + bus_cfg.bus_type, V4L2_MBUS_CSI2_DPHY);
> + goto out_free_bus_cfg;
> + }
> +
> + if (bus_cfg.bus.mipi_csi2.num_data_lanes != 1) {
> + dev_err(ov7251->dev, "only a 1-lane CSI2 config is supported");
> + ret = -EINVAL;
> + goto out_free_bus_cfg;
> + }
> +
> + if (!bus_cfg.nr_of_link_frequencies) {
> + dev_err(ov7251->dev, "no link frequencies defined\n");
> + ret = -EINVAL;
> + goto out_free_bus_cfg;
> + }
> +
> + freq_found = false;
> + for (i = 0; i < bus_cfg.nr_of_link_frequencies; i++) {
> + if (freq_found)
> + break;
> +
> + for (j = 0; j < ARRAY_SIZE(link_freq); j++)
> + if (bus_cfg.link_frequencies[i] == link_freq[j]) {
> + freq_found = true;
> + break;
> + }
> + }
> +
> + if (i == bus_cfg.nr_of_link_frequencies) {
This doesn't work if there is only one link frequency defined in the
config and it is valid.
Loop i=0, freq_found gets set to true.
Continue the outer loop.
i++ , so i=1.
i < bus_cfg.nr_of_link_frequencies is no longer true (both are 1), so
we quit the outer loop.
i == bus_cfg.nr_of_link_frequencies, so we now fail the function.
Doesn't this last check want to be if (!freq_found) ?
And/or amend the loop to move the "if (freq_found) break;" to the end,
so that we break out of the outer loop as soon as we have a frequency
found, and thereby avoid the i++.
It all feels a little odd as there is only one link frequency used by
all the modes, and we're not actually filtering the modes that can be
selected based on the configured link frequencies should there be
multiple frequencies in link_freq[].
Dave
> + dev_err(ov7251->dev, "no supported link freq found\n");
> + ret = -EINVAL;
> + goto out_free_bus_cfg;
> + }
> +
> +out_free_bus_cfg:
> + v4l2_fwnode_endpoint_free(&bus_cfg);
> +
> + return ret;
> +}
> +
> static int ov7251_probe(struct i2c_client *client)
> {
> struct device *dev = &client->dev;
> - struct fwnode_handle *endpoint;
> struct ov7251 *ov7251;
> u8 chip_id_high, chip_id_low, chip_rev;
> int ret;
> @@ -1270,24 +1333,9 @@ static int ov7251_probe(struct i2c_client *client)
> ov7251->i2c_client = client;
> ov7251->dev = dev;
>
> - endpoint = fwnode_graph_get_next_endpoint(dev_fwnode(dev), NULL);
> - if (!endpoint) {
> - dev_err(dev, "endpoint node not found\n");
> - return -EINVAL;
> - }
> -
> - ret = v4l2_fwnode_endpoint_parse(endpoint, &ov7251->ep);
> - fwnode_handle_put(endpoint);
> - if (ret < 0) {
> - dev_err(dev, "parsing endpoint node failed\n");
> + ret = ov7251_check_hwcfg(ov7251);
> + if (ret)
> return ret;
> - }
> -
> - if (ov7251->ep.bus_type != V4L2_MBUS_CSI2_DPHY) {
> - dev_err(dev, "invalid bus type (%u), must be CSI2 (%u)\n",
> - ov7251->ep.bus_type, V4L2_MBUS_CSI2_DPHY);
> - return -EINVAL;
> - }
>
> /* get system clock (xclk) */
> ov7251->xclk = devm_clk_get(dev, "xclk");
> --
> 2.25.1
>
