Hi Dave
On 17/02/2022 15:54, Dave Stevenson wrote:
> 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++.
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
>>
