On 18/02/2022 11:58, Dave Stevenson wrote:
> Hi Daniel
>
> On Fri, 18 Feb 2022 at 08:37, Daniel Scally <djrscally@xxxxxxxxx> 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@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.
> 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
>>>>
