From: Sakari Ailus <sakari.ailus@iki.fi>
To: Petr Cvek <petrcvekcz@gmail.com>
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: Mon, 10 Dec 2018 00:04:08 +0200 [thread overview]
Message-ID: <20181209220408.mkm244qqcii7s3r3@valkosipuli.retiisi.org.uk> (raw)
In-Reply-To: <e12dab78-5a85-a94f-e892-0723592cd2dd@gmail.com>
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.
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
--
Kind regards,
Sakari Ailus
next prev parent reply other threads:[~2018-12-09 22:04 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
2018-12-09 22:04 ` Sakari Ailus [this message]
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=20181209220408.mkm244qqcii7s3r3@valkosipuli.retiisi.org.uk \
--to=sakari.ailus@iki.fi \
--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=petrcvekcz@gmail.com \
--cc=philipp.zabel@gmail.com \
--cc=robert.jarzmik@free.fr \
--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).