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@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;
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");
--
Kind regards,
Sakari Ailus
