| Message ID | 20220215230737.1870630-5-djrscally@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Support OVTI7251 on Microsoft Surface line | expand |
Hi Daniel, Thanks for the set. On Tue, Feb 15, 2022 at 11:07:31PM +0000, Daniel Scally 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@gmail.com> > --- > 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; unsigned int ? > + 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) { You can drop this check as v4l2_fwnode_endpoint_alloc_parse() only parses D-PHY bus type and returns error otherwise, as you've (correctly) specified D-PHY in bus_cfg. > + 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) { Is this a driver or hardware limitation? If it's hardware, you could also ignore it --- there's nothing to configure. > + 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; You could do this in initialisation. > + 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) { > + 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");
Hi Sakari - thanks for the comments On 16/02/2022 15:26, Sakari Ailus wrote: > Hi Daniel, > > Thanks for the set. > > On Tue, Feb 15, 2022 at 11:07:31PM +0000, Daniel Scally 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@gmail.com> >> --- >> 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; > unsigned int ? Ack > >> + 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) { > You can drop this check as v4l2_fwnode_endpoint_alloc_parse() only parses > D-PHY bus type and returns error otherwise, as you've (correctly) specified > D-PHY in bus_cfg. Ah-ha, ok useful to know thanks. > >> + 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) { > Is this a driver or hardware limitation? > > If it's hardware, you could also ignore it --- there's nothing to > configure. Good point, it is a hardware limitation yes. > >> + 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; > You could do this in initialisation. > >> + for (i = 0; i < bus_cfg.nr_of_link_frequencies; i++) { Ack >> + 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) { >> + 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");
Hi Daniel On Tue, 15 Feb 2022 at 23:08, Daniel Scally <djrscally@gmail.com> 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@gmail.com> > --- > 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 >
Hi Dave On 17/02/2022 15:54, Dave Stevenson wrote: > Hi Daniel > > On Tue, 15 Feb 2022 at 23:08, Daniel Scally <djrscally@gmail.com> 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@gmail.com> >> --- >> 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++. Ah, damn you're right. Sorry - I originally broke the double loop with a goto and then decided at the last minute that it was too ugly so I changed it. Thought I was careful enough there to not need to test it - lesson learned. > 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[]. At present no, but I was thinking about adding one (the windows driver for my platform uses a different link freq, and I'd like to support it) - it'll just mean a different set of ov7251_pll_configs. > > 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 >>
Hi Daniel On Fri, 18 Feb 2022 at 08:37, Daniel Scally <djrscally@gmail.com> wrote: > > Hi Dave > > On 17/02/2022 15:54, Dave Stevenson wrote: > > Hi Daniel > > > > On Tue, 15 Feb 2022 at 23:08, Daniel Scally <djrscally@gmail.com> 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@gmail.com> > >> --- > >> 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++. > > > Ah, damn you're right. Sorry - I originally broke the double loop with a > goto and then decided at the last minute that it was too ugly so I > changed it. Thought I was careful enough there to not need to test it - > lesson learned. Been there, done that :-) > > 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[]. > > > At present no, but I was thinking about adding one (the windows driver > for my platform uses a different link freq, and I'd like to support it) > - it'll just mean a different set of ov7251_pll_configs. OK, that seems reasonable. Someone recently asked about running ov7251 with libcamera on a Pi which was why I was testing your patches. I've a branch[1] that takes your patches and then adds the relevant controls to make it work - I'll send them once your current changes are merged to avoid conflicts. I've removed the link freq per mode as generally all modes run at the same link freq, and there's really only one mode anyway. Is that valid for your alternate link freq? Does it alter the pixel rate as well, or just the CSI config? There's no point in removing options if you'll want them back again. Cheers Dave [1] https://github.com/6by9/linux/tree/rpi-5.15.y-cam > > > > 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 > >>
On 18/02/2022 11:58, Dave Stevenson wrote: > Hi Daniel > > On Fri, 18 Feb 2022 at 08:37, Daniel Scally <djrscally@gmail.com> wrote: >> Hi Dave >> >> On 17/02/2022 15:54, Dave Stevenson wrote: >>> Hi Daniel >>> >>> On Tue, 15 Feb 2022 at 23:08, Daniel Scally <djrscally@gmail.com> 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@gmail.com> >>>> --- >>>> 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++. >> >> Ah, damn you're right. Sorry - I originally broke the double loop with a >> goto and then decided at the last minute that it was too ugly so I >> changed it. Thought I was careful enough there to not need to test it - >> lesson learned. > Been there, done that :-) Hah hoping that this will sink in strongly for at least a little while! > >>> 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[]. >> >> At present no, but I was thinking about adding one (the windows driver >> for my platform uses a different link freq, and I'd like to support it) >> - it'll just mean a different set of ov7251_pll_configs. > OK, that seems reasonable. > > Someone recently asked about running ov7251 with libcamera on a Pi > which was why I was testing your patches. I've a branch[1] that takes > your patches and then adds the relevant controls to make it work - > I'll send them once your current changes are merged to avoid > conflicts. Ah excellent, hadn't gotten as far as trying to run this through libcamera yet, it's connected to an IPU3 for me so there's a bit more work to do there first. > I've removed the link freq per mode as generally all modes run at the > same link freq, and there's really only one mode anyway. Is that valid > for your alternate link freq? Does it alter the pixel rate as well, or > just the CSI config? There's no point in removing options if you'll > want them back again. Can't tell what Windows does as I have no way of forcing it to change the mode (in fact I can only make it turn on by starting Windows Hello set up and then waving my head around like a lunatic so it doesn't complete / fail out before I get the chance to read the registers) but I'd expect to implement it as a static (I.E. not changing per mode) pixel rate that depends on the selected link freq. I think it's fine to drop both options from the mode struct Cheers Dan > > Cheers > Dave > > [1] https://github.com/6by9/linux/tree/rpi-5.15.y-cam > > >>> 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 >>>>
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) { + 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");
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@gmail.com> --- drivers/media/i2c/ov7251.c | 84 ++++++++++++++++++++++++++++++-------- 1 file changed, 66 insertions(+), 18 deletions(-)