From: Pavel Machek <pavel@ucw.cz>
To: Sakari Ailus <sakari.ailus@iki.fi>
Cc: ivo.g.dimitrov.75@gmail.com, sre@kernel.org,
pali.rohar@gmail.com, linux-media@vger.kernel.org,
galak@codeaurora.org, mchehab@osg.samsung.com,
linux-kernel@vger.kernel.org
Subject: Re: [PATCH] media: Add video bus switch
Date: Sun, 5 Feb 2017 23:16:22 +0100 [thread overview]
Message-ID: <20170205221622.GA16107@amd> (raw)
In-Reply-To: <20170203222520.GE12291@valkosipuli.retiisi.org.uk>
[-- Attachment #1: Type: text/plain, Size: 5573 bytes --]
Hi!
I lost my original reply... so this will be slightly brief.
> > > + * This program is distributed in the hope that it will be useful, but
> > > + * WITHOUT ANY WARRANTY; without even the implied warranty of
> > > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> > > + * General Public License for more details.
> > > + */
> > > +
> > > +#define DEBUG
>
> Please remove.
Ok.
> > > +#include <linux/of_graph.h>
> > > +#include <linux/gpio/consumer.h>
>
> Alphabetical order, please.
Ok. (But let me make unhappy noise, because these rules are
inconsistent across kernel.)
> > > + * TODO:
> > > + * isp_subdev_notifier_complete() calls v4l2_device_register_subdev_nodes()
> > > + */
> > > +
> > > +#define CSI_SWITCH_SUBDEVS 2
> > > +#define CSI_SWITCH_PORTS 3
>
> This could go to the enum below.
>
> I guess the CSI_SWITCH_SUBDEVS could be (CSI_SWITCH_PORTS - 1).
>
> I'd just replace CSI_SWITCH by VBS. The bus could be called
> differently.
Ok.
> > > +static int vbs_registered(struct v4l2_subdev *sd)
> > > +{
> > > + struct v4l2_device *v4l2_dev = sd->v4l2_dev;
> > > + struct vbs_data *pdata;
> > > + int err;
> > > +
> > > + dev_dbg(sd->dev, "registered, init notifier...\n");
>
> Looks like a development time debug message. :-)
ex-development message ;-).
> > > + gpiod_set_value(pdata->swgpio, local->index == CSI_SWITCH_PORT_2);
> > > + pdata->state = local->index;
> > > +
> > > + sd = vbs_get_remote_subdev(sd);
> > > + if (IS_ERR(sd))
> > > + return PTR_ERR(sd);
> > > +
> > > + pdata->subdev.ctrl_handler = sd->ctrl_handler;
>
> This is ugly. You're exposing all the controls through another sub-device.
>
> How does link validation work now?
>
> I wonder if it'd be less so if you just pass through the LINK_FREQ and
> PIXEL_RATE controls. It'll certainly be more code though.
>
> I think the link frequency could be something that goes to the frame
> descriptor as well. Then we wouldn't need to worry about the controls
> separately, just passing the frame descriptor would be enough.
>
> I apologise that I don't have patches quite ready for posting to the
> list.
(Plus of course question is "what is link validation".)
Ok, let me play with this one. Solution you are suggesting is to make
a custom harndler that only passes certain data through, right?
> > > + dev_dbg(pdata->subdev.dev, "create link: %s[%d] -> %s[%d])\n",
> > > + src->name, src_pad, sink->name, sink_pad);
> > > + }
> > > +
> > > + return v4l2_device_register_subdev_nodes(pdata->subdev.v4l2_dev);
>
> The ISP driver's complete() callback calls
> v4l2_device_register_subdev_nodes() already. Currently it cannot handle
> being called more than once --- that needs to be fixed.
I may have patches for that. Let me check.
> > > +}
> > > +
> > > +
>
> I'd say that's an extra newline.
Not any more.
> > > + v4l2_subdev_init(&pdata->subdev, &vbs_ops);
> > > + pdata->subdev.dev = &pdev->dev;
> > > + pdata->subdev.owner = pdev->dev.driver->owner;
> > > + strncpy(pdata->subdev.name, dev_name(&pdev->dev), V4L2_SUBDEV_NAME_SIZE);
>
> How about sizeof(pdata->subdev.name) ?
Ok.
> I'm not sure I like V4L2_SUBDEV_NAME_SIZE in general. It could be even
> removed. But not by this patch. :-)
>
> > > + v4l2_set_subdevdata(&pdata->subdev, pdata);
> > > + pdata->subdev.entity.function = MEDIA_ENT_F_SWITCH;
> > > + pdata->subdev.entity.flags |= MEDIA_ENT_F_SWITCH;
>
> MEDIA_ENT_FL_*
Do we actually have a flag here? We already have .function, so this
looks like a duplicate.
> > > + if (err < 0) {
> > > + dev_err(&pdev->dev, "Failed to register v4l2 subdev: %d\n", err);
> > > + media_entity_cleanup(&pdata->subdev.entity);
> > > + return err;
> > > + }
> > > +
> > > + dev_info(&pdev->dev, "video-bus-switch registered\n");
>
> How about dev_dbg()?
Ok.
> > > +static int video_bus_switch_remove(struct platform_device *pdev)
> > > +{
> > > + struct vbs_data *pdata = platform_get_drvdata(pdev);
> > > +
> > > + v4l2_async_notifier_unregister(&pdata->notifier);
>
> Shouldn't you unregister the notifier in the .unregister() callback?
Ok, I guess we can do that for symetry.
> > > /* generic v4l2_device notify callback notification values */
> > > #define V4L2_SUBDEV_IR_RX_NOTIFY _IOW('v', 0, u32)
> > > @@ -415,6 +416,8 @@ struct v4l2_subdev_video_ops {
> > > const struct v4l2_mbus_config *cfg);
> > > int (*s_rx_buffer)(struct v4l2_subdev *sd, void *buf,
> > > unsigned int *size);
> > > + int (*g_endpoint_config)(struct v4l2_subdev *sd,
> > > + struct v4l2_of_endpoint *cfg);
>
> This should be in a separate patch --- assuming we'll add this one.
Hmm. I believe the rest of the driver is quite useful in understanding
this. Ok, lets get the discussion started.
> > > --- a/include/uapi/linux/media.h
> > > +++ b/include/uapi/linux/media.h
> > > @@ -147,6 +147,7 @@ struct media_device_info {
> > > * MEDIA_ENT_F_IF_VID_DECODER and/or MEDIA_ENT_F_IF_AUD_DECODER.
> > > */
> > > #define MEDIA_ENT_F_TUNER (MEDIA_ENT_F_OLD_SUBDEV_BASE + 5)
> > > +#define MEDIA_ENT_F_SWITCH (MEDIA_ENT_F_OLD_SUBDEV_BASE + 6)
>
> I wonder if MEDIA_ENT_F_PROC_ would be a better prefix.
> We shouldn't have new entries in MEDIA_ENT_F_OLD_SUBDEV_BASE anymore.
Ok.
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]
next prev parent reply other threads:[~2017-02-05 22:16 UTC|newest]
Thread overview: 97+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-10-23 20:03 [PATCH v4] media: Driver for Toshiba et8ek8 5MP sensor Pavel Machek
2016-10-23 20:19 ` Sakari Ailus
2016-10-23 20:33 ` Pavel Machek
2016-10-31 22:54 ` Sakari Ailus
2016-11-01 6:36 ` Ivaylo Dimitrov
2016-11-01 20:11 ` Sakari Ailus
2016-11-01 22:14 ` Ivaylo Dimitrov
2016-11-02 8:15 ` Pavel Machek
2016-11-02 8:16 ` Ivaylo Dimitrov
2016-11-01 15:39 ` Pavel Machek
2016-11-01 20:08 ` Sakari Ailus
2016-11-03 8:14 ` Pavel Machek
2016-11-03 22:48 ` Sebastian Reichel
2016-11-03 23:05 ` Sakari Ailus
2016-11-03 23:40 ` Ivaylo Dimitrov
2016-11-04 0:05 ` Sebastian Reichel
2016-11-14 21:58 ` Sakari Ailus
2016-11-15 0:53 ` Sebastian Reichel
2016-11-15 10:54 ` Pavel Machek
2016-11-15 22:55 ` Sakari Ailus
2016-10-23 20:40 ` Pavel Machek
2016-10-31 22:58 ` Sakari Ailus
2016-11-02 0:45 ` Laurent Pinchart
2016-10-23 20:47 ` Pavel Machek
2016-12-13 21:05 ` Pavel Machek
2016-12-18 21:56 ` Sakari Ailus
2016-11-19 23:29 ` Sakari Ailus
2016-11-20 10:02 ` Pavel Machek
2016-11-20 15:20 ` Pavel Machek
2016-11-20 15:21 ` Pavel Machek
2016-11-20 15:31 ` Pavel Machek
2016-12-14 12:24 ` [PATCH v5] " Pavel Machek
2016-12-14 13:03 ` Pali Rohár
2016-12-14 15:52 ` Ivaylo Dimitrov
2016-12-14 20:12 ` Pavel Machek
2016-12-14 22:07 ` Pali Rohár
2016-12-14 22:35 ` Pavel Machek
2016-12-18 22:01 ` Sakari Ailus
2016-12-20 12:37 ` Pavel Machek
2016-12-20 14:01 ` Sakari Ailus
2016-12-20 22:42 ` Pavel Machek
2016-12-21 13:42 ` Sakari Ailus
2016-12-21 22:42 ` Pavel Machek
2016-12-21 23:29 ` Sakari Ailus
2016-12-22 9:34 ` Pavel Machek
2016-12-22 10:01 ` [PATCH v6] " Pavel Machek
2016-12-22 13:39 ` [RFC/PATCH] media: Add video bus switch Pavel Machek
2016-12-22 14:32 ` Sebastian Reichel
2016-12-22 20:53 ` Pavel Machek
2016-12-22 23:11 ` Sebastian Reichel
2016-12-22 22:42 ` Pavel Machek
2016-12-22 23:40 ` Sebastian Reichel
2016-12-23 11:42 ` Pavel Machek
2016-12-23 18:53 ` Ivaylo Dimitrov
2016-12-23 20:56 ` Pavel Machek
2016-12-24 14:26 ` Pavel Machek
2016-12-24 14:43 ` Pavel Machek
2016-12-24 15:20 ` [PATCH] " Pavel Machek
2016-12-24 18:35 ` kbuild test robot
2017-01-12 11:17 ` Pavel Machek
2017-02-03 22:25 ` Sakari Ailus
2017-02-05 22:16 ` Pavel Machek [this message]
2017-02-05 22:44 ` Sakari Ailus
2017-02-03 12:35 ` [PATCH] devicetree: " Pavel Machek
2017-02-03 13:07 ` Sakari Ailus
2017-02-03 21:06 ` Pavel Machek
2017-02-03 21:34 ` Sakari Ailus
2017-02-04 21:56 ` Pavel Machek
2017-02-04 22:33 ` Sakari Ailus
2017-02-05 21:12 ` Pavel Machek
2017-02-05 23:40 ` Sebastian Reichel
2017-02-06 9:37 ` [PATCH] media: add operation to get configuration of "the other side" of the link Pavel Machek
2017-12-19 15:43 ` Sakari Ailus
2017-12-20 17:54 ` [PATCH] devicetree: Add video bus switch Laurent Pinchart
2017-12-21 9:05 ` Sakari Ailus
2017-12-21 16:36 ` Ivaylo Dimitrov
2017-12-22 9:24 ` Pavel Machek
2017-02-03 13:32 ` Pali Rohár
2017-02-03 21:07 ` Pavel Machek
2017-02-04 1:04 ` Sebastian Reichel
2017-02-08 21:36 ` Rob Herring
2017-02-08 22:30 ` Pavel Machek
2017-02-09 23:02 ` Rob Herring
2017-02-09 23:03 ` Rob Herring
2017-02-10 19:54 ` Pavel Machek
2017-02-10 22:17 ` Sakari Ailus
2017-02-13 9:54 ` Pavel Machek
2017-02-13 10:20 ` Sakari Ailus
2017-03-02 8:54 ` Pavel Machek
2017-02-08 22:34 ` Pavel Machek
2017-02-09 22:58 ` Rob Herring
2017-02-10 21:17 ` Pavel Machek
2016-12-27 9:26 ` [PATCH v6] media: Driver for Toshiba et8ek8 5MP sensor Sakari Ailus
2016-12-27 20:45 ` Pavel Machek
2016-12-27 20:59 ` [PATCH] mark myself as mainainer for camera on N900 Pavel Machek
2016-12-27 23:57 ` Sebastian Reichel
2017-01-25 13:48 ` Sakari Ailus
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=20170205221622.GA16107@amd \
--to=pavel@ucw.cz \
--cc=galak@codeaurora.org \
--cc=ivo.g.dimitrov.75@gmail.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-media@vger.kernel.org \
--cc=mchehab@osg.samsung.com \
--cc=pali.rohar@gmail.com \
--cc=sakari.ailus@iki.fi \
--cc=sre@kernel.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).