Dne 09. 12. 18 v 23:04 Sakari Ailus napsal(a):
> Hi Petr,
>
> What's the status of this set? It would seem that addressing the issues
> is fairly trivial. Please also see a few comments below.
>
Hi,
Gonna work on it this week. I've had to work on higher priority stuff
last two months some of which blocked me from playing with my phone
(with ov9640 camera).
best regards,
Petr
> On Fri, Sep 14, 2018 at 10:54:51PM +0200, Petr Cvek wrote:
>> Dne 14.9.2018 v 14:59 Sakari Ailus napsal(a):
>>> Hi Petr,
>>>
>>> Thanks for the patchset, and my apologies for reviewing it so late!
>>>
>>> I'm commenting this one but feel free to add patches to address the
>>> comments.
>>>
>>
>> Hi and thanks for the review. I would like to have this patch set to be
>> as much as possible only a conversion from soc-camera, but I guess I can
>> fix the error handling in probe and the missing newlines. For the
>> enhanced functionality, I would like to have a new patch set after I'll
>> patch the controller (pxa camera) on my testing platform.
>>
>>>> +/* Start/Stop streaming from the device */
>>>> +static int ov9640_s_stream(struct v4l2_subdev *sd, int enable)
>>>> +{
>>>> + return 0;
>>>
>>> Doesn't the sensor provide any control over the streaming state? Just
>>> wondering...
>>>
>>
>> Before the PXA camera switch from soc-camera I've found some
>> deficiencies in register settings so I'm planning to test them all. With
>> the current state of vanilla I wouldn't be able to test the change.
>> After the quick search in datasheet I wasn't able to find any (stream,
>> capture, start) flag. It may be controlled by just setting the output
>> format flags, but these registers are some of those I will be changing
>> in the future.
>>
>>>> +static int ov9640_s_power(struct v4l2_subdev *sd, int on)
>>>> +{
>>>> + struct i2c_client *client = v4l2_get_subdevdata(sd);
>>>> + struct soc_camera_subdev_desc *ssdd = soc_camera_i2c_to_desc(client);
>>>> + struct ov9640_priv *priv = to_ov9640_sensor(sd);
>>>> +
>>>> + return soc_camera_set_power(&client->dev, ssdd, priv->clk, on);
>>>
>>> Runtime PM support would be nice --- but not needed in this set IMO.
>>>
>>
>> If I remember correctly a suspend to mem will freeze the whole machine,
>> so in the future :-/.
>>
>>
>>>> +}
>>>> +
>>>> +/* select nearest higher resolution for capture */
>>>> +static void ov9640_res_roundup(u32 *width, u32 *height)
>>>> +{
>>>> + int i;
>>>
>>> unsigned int
>>>
>>> Same for other loops where no negative values or test below zero are
>>> needed (or where the value which is compared against is signed).
>>>
>> Just re-declaring: unsigned int i; ... OK
>
> Yes.
>
>>
>>>> +
>>>> + cfg->try_fmt = *mf;
>>>
>>> Newline here?
>>>
>>>> + return 0;
>>>> +}
>>>> +
>>>> +static int ov9640_enum_mbus_code(struct v4l2_subdev *sd,
>>>> + struct v4l2_subdev_pad_config *cfg,
>>>> + struct v4l2_subdev_mbus_code_enum *code)
>>>> +{
>>>> + if (code->pad || code->index >= ARRAY_SIZE(ov9640_codes))
>>>> + return -EINVAL;
>>>> +
>>>> + code->code = ov9640_codes[code->index];
>>>
>>> And here.
>>>
>>
>> np
>>
>>>> +/* Request bus settings on camera side */
>>>> +static int ov9640_g_mbus_config(struct v4l2_subdev *sd,
>>>> + struct v4l2_mbus_config *cfg)
>>>> +{
>>>> + struct i2c_client *client = v4l2_get_subdevdata(sd);
>>>> + struct soc_camera_subdev_desc *ssdd = soc_camera_i2c_to_desc(client);
>>>> +
>>>> + cfg->flags = V4L2_MBUS_PCLK_SAMPLE_RISING | V4L2_MBUS_MASTER |
>>>> + V4L2_MBUS_VSYNC_ACTIVE_HIGH | V4L2_MBUS_HSYNC_ACTIVE_HIGH |
>>>> + V4L2_MBUS_DATA_ACTIVE_HIGH;
>>>
>>> This should come from DT instead. Could you add DT binding documentation
>>> for the sensor, please? There's already some for ov9650; I wonder how
>>> similar that one is.
>>
>> The platform doesn't support it yet, so I have no way to test any DT
>> patches.
>
> Ack. It's fine to leave that out now.
>
>>
>>>> + cfg->type = V4L2_MBUS_PARALLEL;
>>>> + cfg->flags = soc_camera_apply_board_flags(ssdd, cfg);
>>>> +
>>>> + return 0;
>>>> +}
>>>> +
>>>> +static const struct v4l2_subdev_video_ops ov9640_video_ops = {
>>>> + .s_stream = ov9640_s_stream,
>>>> + .g_mbus_config = ov9640_g_mbus_config,
>>>> +};
>>>> +
>>>> +static const struct v4l2_subdev_pad_ops ov9640_pad_ops = {
>>>> + .enum_mbus_code = ov9640_enum_mbus_code,
>>>> + .get_selection = ov9640_get_selection,
>>>> + .set_fmt = ov9640_set_fmt,
>>>
>>> Please add an operating to get the format as well.
>>
>> OK, but it will be tested on a preliminary hacked pxa-camera :-).
>
> That's fine.
>
>>
>>>
>>>> +};
>>>> +
>>>> +static const struct v4l2_subdev_ops ov9640_subdev_ops = {
>>>> + .core = &ov9640_core_ops,
>>>> + .video = &ov9640_video_ops,
>>>> + .pad = &ov9640_pad_ops,
>>>> +};
>>>> +
>>>> +/*
>>>> + * i2c_driver function
>>>> + */
>>>> +static int ov9640_probe(struct i2c_client *client,
>>>> + const struct i2c_device_id *did)
>>>> +{
>>>> + struct ov9640_priv *priv;
>>>> + struct soc_camera_subdev_desc *ssdd = soc_camera_i2c_to_desc(client);
>>>> + int ret;
>>>> +
>>>> + if (!ssdd) {
>>>> + dev_err(&client->dev, "Missing platform_data for driver\n");
>>>> + return -EINVAL;
>>>> + }
>>>> +
>>>> + priv = devm_kzalloc(&client->dev, sizeof(*priv), GFP_KERNEL);
>>>> + if (!priv)
>>>> + return -ENOMEM;
>>>> +
>>>> + v4l2_i2c_subdev_init(&priv->subdev, client, &ov9640_subdev_ops);
>>>> +
>>>> + v4l2_ctrl_handler_init(&priv->hdl, 2);
>>>> + v4l2_ctrl_new_std(&priv->hdl, &ov9640_ctrl_ops,
>>>> + V4L2_CID_VFLIP, 0, 1, 1, 0);
>>>> + v4l2_ctrl_new_std(&priv->hdl, &ov9640_ctrl_ops,
>>>> + V4L2_CID_HFLIP, 0, 1, 1, 0);
>>>> + priv->subdev.ctrl_handler = &priv->hdl;
>>>> + if (priv->hdl.error)
>>>
>>> v4l2_ctrl_handler_free() is missing here. The function would benefit from
>>> goto-based error handling.
>>>
>>
>> + rest -> np
>
