From: Petr Cvek <petrcvekcz@gmail.com>
To: Sakari Ailus <sakari.ailus@iki.fi>
Cc: hans.verkuil@cisco.com, jacopo@jmondi.org, mchehab@kernel.org,
marek.vasut@gmail.com, linux-media@vger.kernel.org,
robert.jarzmik@free.fr, slapin@ossfans.org,
philipp.zabel@gmail.com
Subject: Re: [PATCH v2 1/4] media: soc_camera: ov9640: move ov9640 out of soc_camera
Date: Fri, 14 Sep 2018 22:54:51 +0200 [thread overview]
Message-ID: <e12dab78-5a85-a94f-e892-0723592cd2dd@gmail.com> (raw)
In-Reply-To: <20180914125910.4ju2utqdlk3klmoz@valkosipuli.retiisi.org.uk>
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
>> +
>> + 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.
>> + 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 :-).
>
>> +};
>> +
>> +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
best regards,
pc2005
next prev parent reply other threads:[~2018-09-15 2:10 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-08-15 13:30 [PATCH v2 0/4] media: soc_camera: ov9640: switch driver to v4l2_async petrcvekcz
2018-08-15 13:30 ` [PATCH v2 1/4] media: soc_camera: ov9640: move ov9640 out of soc_camera petrcvekcz
2018-08-15 13:35 ` Petr Cvek
2018-08-16 10:23 ` jacopo mondi
2018-08-16 10:38 ` Petr Cvek
2018-09-14 12:59 ` Sakari Ailus
2018-09-14 20:54 ` Petr Cvek [this message]
2018-12-09 22:04 ` Sakari Ailus
2018-12-10 12:30 ` Petr Cvek
2018-08-15 13:30 ` [PATCH v2 2/4] media: i2c: ov9640: drop soc_camera code and switch to v4l2_async petrcvekcz
2018-08-15 13:30 ` [PATCH v2 3/4] media: i2c: ov9640: add missing SPDX identifiers petrcvekcz
2018-08-15 13:30 ` [PATCH v2 4/4] MAINTAINERS: Add Petr Cvek as a maintainer for the ov9640 driver petrcvekcz
2018-09-14 12:59 ` [PATCH v2 0/4] media: soc_camera: ov9640: switch driver to v4l2_async Sakari Ailus
2018-09-14 21:00 ` Petr Cvek
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=e12dab78-5a85-a94f-e892-0723592cd2dd@gmail.com \
--to=petrcvekcz@gmail.com \
--cc=hans.verkuil@cisco.com \
--cc=jacopo@jmondi.org \
--cc=linux-media@vger.kernel.org \
--cc=marek.vasut@gmail.com \
--cc=mchehab@kernel.org \
--cc=philipp.zabel@gmail.com \
--cc=robert.jarzmik@free.fr \
--cc=sakari.ailus@iki.fi \
--cc=slapin@ossfans.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).