Re: [PATCH 60/60] staging: lustre: libcfs: fix minimum size check for libcfs ioctl
On Tue, Jan 31, 2017 at 02:25:22AM +, James Simmons wrote: > This sounds like a separate patch. I will open a ticket about this and > your comments below. There are a some other places that need a size requirement like LNetCtl(). It really feels like it should be a part of this patch because this patch is introducing a security breakage and it's just fixing a normal bug. regards, dan carpenter ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: Need a second set of eyeballs for a possible startup race condition in vc04_services/vchiq.
On Mon, Jan 30, 2017 at 01:17:55PM -0800, Michael Zoran wrote: > On Tue, 2017-01-31 at 00:01 +0300, Dan Carpenter wrote: > > It's hard to review this because there are no callers and the hash > > you're talking about is an RPI hash... You have no idea how lazy I > > am. > > > > You're right, that code looks racy but it's almost certainly harmless > > depending on how it's called. It has to race at the very very > > beginning > > otherwise it's fine. > > I wonder if out of caution it would make sense to start stripping out > some of these entry points that don't have an obvious caller in the > kernel tree? If you aren't going to need to add them back soon for another one of these drivers, yes. If they are going to be needed, I suggest adding the driver now so we know what should, and should not, be kept. thanks, greg k-h ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 14/60] staging: lustre: lov: Ensure correct operation for large object sizes
On Sat, Jan 28, 2017 at 07:04:42PM -0500, James Simmons wrote:
> From: Nathaniel Clark
>
> If a backing filesystem (ZFS) returns that it supports very large
> (LLONG_MAX) object sizes, that should be correctly supported. This
> fixes the check for unitialized stripe_maxbytes in
> lsm_unpackmd_common(), so that ZFS can return LLONG_MAX and it will be
> okay. This issue is excersized by writing to or past the 2TB boundary
> of a singly stripped file.
>
> Signed-off-by: Nathaniel Clark
> Intel-bug-id: https://jira.hpdd.intel.com/browse/LU-7890
> Reviewed-on: http://review.whamcloud.com/19066
> Reviewed-by: Andreas Dilger
> Reviewed-by: Jinshan Xiong
> Reviewed-by: Oleg Drokin
> Signed-off-by: James Simmons
> ---
> drivers/staging/lustre/lustre/lov/lov_ea.c | 22 --
> 1 file changed, 12 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/staging/lustre/lustre/lov/lov_ea.c
> b/drivers/staging/lustre/lustre/lov/lov_ea.c
> index ac0bf64..07dee87 100644
> --- a/drivers/staging/lustre/lustre/lov/lov_ea.c
> +++ b/drivers/staging/lustre/lustre/lov/lov_ea.c
> @@ -150,7 +150,7 @@ static int lsm_unpackmd_common(struct lov_obd *lov,
> struct lov_mds_md *lmm,
> struct lov_ost_data_v1 *objects)
> {
> - loff_t stripe_maxbytes = LLONG_MAX;
> + loff_t min_stripe_maxbytes = 0, lov_bytes;
I've seen this thing in sevaral patches and I haven't commented on it
but please don't do this.
unsigned long foo = 0, bar;
It took my a long time to find the lov_bytes declaration hiding off the
end here. We haven't made checkpatch.pl complain about it yet but it's
not kernel style. One declaration per line and especially if they
aren't closely related like "int left, right;" and doubly especially if
there is an initialization involved.
> unsigned int stripe_count;
> struct lov_oinfo *loi;
> unsigned int i;
> @@ -168,8 +168,6 @@ static int lsm_unpackmd_common(struct lov_obd *lov,
> stripe_count = lsm_is_released(lsm) ? 0 : lsm->lsm_stripe_count;
>
> for (i = 0; i < stripe_count; i++) {
> - loff_t tgt_bytes;
> -
> loi = lsm->lsm_oinfo[i];
> ostid_le_to_cpu(&objects[i].l_ost_oi, &loi->loi_oi);
> loi->loi_ost_idx = le32_to_cpu(objects[i].l_ost_idx);
> @@ -194,17 +192,21 @@ static int lsm_unpackmd_common(struct lov_obd *lov,
> continue;
> }
>
> - tgt_bytes = lov_tgt_maxbytes(lov->lov_tgts[loi->loi_ost_idx]);
> - stripe_maxbytes = min_t(loff_t, stripe_maxbytes, tgt_bytes);
> + lov_bytes = lov_tgt_maxbytes(lov->lov_tgts[loi->loi_ost_idx]);
> + if (min_stripe_maxbytes == 0 || lov_bytes < min_stripe_maxbytes)
> + min_stripe_maxbytes = lov_bytes;
> }
>
> - if (stripe_maxbytes == LLONG_MAX)
> - stripe_maxbytes = LUSTRE_EXT3_STRIPE_MAXBYTES;
> + if (min_stripe_maxbytes == 0)
> + min_stripe_maxbytes = LUSTRE_EXT3_STRIPE_MAXBYTES;
> +
> + stripe_count = lsm->lsm_stripe_count ?: lov->desc.ld_tgt_count;
> + lov_bytes = min_stripe_maxbytes * stripe_count;
>
> - if (!lsm->lsm_stripe_count)
> - lsm->lsm_maxbytes = stripe_maxbytes * lov->desc.ld_tgt_count;
> + if (lov_bytes < min_stripe_maxbytes) /* handle overflow */
Signed overflows are undefined. I think we use GCC options so that the
compiler does not remove these checks but I also know that I have been
wrong before about GCC options and undefined behavior.
regards,
dan carpenter
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 21/60] staging: lustre: ptlrpc: correct use of list_add_tail()
On Sat, Jan 28, 2017 at 07:04:49PM -0500, James Simmons wrote: > From: "John L. Hammond" > > In sptlrpc_gc_add_sec() swap the arguments to list_add_tail() so that > it does what we meant it to do. > Huh... This is from before lustre was merged into staging. What are the user visible effects of this bug? I would have expected it to get caught earlier. > Signed-off-by: John L. Hammond > Intel-bug-id: https://jira.hpdd.intel.com/browse/LU-8270 I bet the answer to my question is on this link but I'm reviewing offline right now. Plus that's not where the bug description belongs. regards, dan carpenter ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 22/60] staging: lustre: fid: fix race in fid allocation
On Sat, Jan 28, 2017 at 07:04:50PM -0500, James Simmons wrote:
> - if (!fid_is_zero(&seq->lcs_fid) &&
> - fid_oid(&seq->lcs_fid) < seq->lcs_width) {
> + if (unlikely(!fid_is_zero(&seq->lcs_fid) &&
> + fid_oid(&seq->lcs_fid) < seq->lcs_width)) {
What does adding an unlikely have to do with the race condition? Also
only add likely/unlikely when it makes a difference to benchmarks.
Otherwise leave it out.
> /* Just bump last allocated fid and return to caller. */
> - seq->lcs_fid.f_oid += 1;
> + seq->lcs_fid.f_oid++;
Ok... I'm pretty sure the compiler can figure this out on its own.
Stop mixing white space changes into your bug fixes. It just makes
reviewing more complicated.
> rc = 0;
> break;
> }
>
regards,
dan carpenter
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: bcm2835-audio: Strengthen build dependencies
On Sat, Jan 28, 2017 at 11:07:15PM -0800, Michael Zoran wrote: > This driver makes no sense outside of ARM or ARM64. > Add an explicit build dependency on: > (ARM || ARM64 || COMPILE_TEST) > > Also set the default build to n > > Signed-off-by: Michael Zoran > --- > drivers/staging/bcm2835-audio/Kconfig | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/staging/bcm2835-audio/Kconfig > b/drivers/staging/bcm2835-audio/Kconfig > index 32a2ff9ef9b2..840faa21f665 100644 > --- a/drivers/staging/bcm2835-audio/Kconfig > +++ b/drivers/staging/bcm2835-audio/Kconfig > @@ -1,6 +1,8 @@ > config SND_BCM2835 > tristate "BCM2835 ALSA driver" > depends on ARCH_BCM2835 && BCM2835_VCHIQ && SND > + depends on (ARM || ARM64 || COMPILE_TEST) > + default n "default n" is always the default, no need to specify this here. thanks, greg k-h ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3 21/24] media: imx: Add MIPI CSI-2 Receiver subdev driver
On Tue, 2017-01-31 at 00:01 +, Russell King - ARM Linux wrote: [...] > The iMX6 manuals call for a very specific seven sequence of initialisation > for CSI2, which begins with: > > 1. reset the D-PHY. > 2. place MIPI sensor in LP-11 state > 3. perform D-PHY initialisation > 4. configure CSI2 lanes and de-assert resets and shutdown signals > > Since you reset the CSI2 at power up and then release it, how do you > guarantee that the published sequence is followed? > > With Philipp's driver, this is easy, because there is a prepare_stream > callback which gives the sensor an opportunity to get everything > correctly configured according to the negotiated parameters, and place > the sensor in LP-11 state. > > Some sensors do not power up in LP-11 state, but need to be programmed > fully before being asked to momentarily stream. Only at that point is > the sensor guaranteed to be in the required LP-11 state. Do you expect that 1. and 2. could depend on the negotiated parameters in any way on some hardware? I had removed the prepare_stream callback from my driver in v2 because for my use case at least the above sequence could be realized by 1. in imx-mipi-csi2 s_power(1) 2. in MIPI sensor s_power(1) 3./4. in imx-mipi-csi2 s_stream(1) 4. in MIPI sensor s_stream(1) as long as the sensor is correctly put back into LP-11 in s_stream(0). regards Philipp ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3 00/24] i.MX Media Driver
On Mon, 2017-01-30 at 17:22 -0800, Steve Longerbeam wrote: > > On 01/30/2017 04:45 PM, Russell King - ARM Linux wrote: > > > > Hi, > > > > Trying this driver with an imx219 camera (which works with Philipp's > > driver) results in not much happening... no /dev/media* node for it, > > no subdevs, no nothing. No clues as to what's missing either. Only > > messages from imx-media are from registering the various subdevs. > > > > [ 37.444877] imx-media: Registered subdev imx6-mipi-csi2 > > [ 37.444973] imx-media: Registered subdev imx219 0-0010 > > [ 38.868740] imx-media: Registered subdev ipu1_ic_prpenc > > [ 38.869265] imx-media: Registered subdev ipu1_ic_prpvf > > [ 38.869425] imx-media: Registered subdev ipu1_ic_pp0 > > [ 38.870086] imx-media: Registered subdev ipu1_ic_pp1 > > [ 38.871510] imx-media: Registered subdev ipu2_ic_prpenc > > [ 38.871743] imx-media: Registered subdev ipu1_smfc0 > > [ 38.873043] imx-media: Registered subdev ipu1_smfc1 > > [ 38.873225] imx-media: Registered subdev ipu2_ic_prpvf > > [ 38.875027] imx-media: Registered subdev ipu2_smfc0 > > [ 38.875320] imx-media: Registered subdev ipu2_ic_pp0 > > [ 38.877148] imx-media: Registered subdev ipu2_smfc1 > > [ 38.877436] imx-media: Registered subdev ipu2_ic_pp1 > > [ 38.932089] imx-media: Registered subdev camif0 > > [ 38.956538] imx-media: Registered subdev camif1 > > [ 38.959148] imx-media: Registered subdev camif2 > > [ 38.964353] imx-media: Registered subdev camif3 > > [ 206.502077] imx-media: Registered subdev ipu1_csi0 > > [ 206.503304] imx-media: Registered subdev ipu1_csi1 > > [ 206.503814] imx-media: Registered subdev ipu2_csi0 > > [ 206.504281] imx-media: Registered subdev ipu2_csi1 > > > > I also get: > > > > [ 37.200072] imx6-mipi-csi2: data lanes: 2 > > [ 37.200077] imx6-mipi-csi2: flags: 0x0200 > > > > and from what I can see, all modules from drivers/staging/media/imx/ are > > loaded (had to load imx-csi by hand because of the brokenness in the > > drivers/gpu/ipu code attaching an device_node pointer after registering > > the platform device, which changes what userspace sees in the modalias > > file.) > > > > Any clues at what to look at? > > Hi Russell, > > I'm not familiar with IMX219, can you send me the source for the > imx219 subdev? I don't see it in 4.10-rc1. > > I'm also having trouble finding a datasheet for it, but from what > I've read, it has a MIPI CSI-2 interface. It should work fine as long > as it presents a single source pad, registers asynchronously, and > sets its entity function to MEDIA_ENT_F_CAM_SENSOR. What about MEDIA_ENT_F_VID_IF_BRIDGE? regards Philipp ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 6/6] Drivers: hv: Log the negotiated IC versions.
On Sat, Jan 28, 2017 at 12:37:18PM -0700, k...@exchange.microsoft.com wrote: > From: Alex Ng > > Log the negotiated IC versions. I'll take this, but really, don't do this at all. Keep the kernel log quiet, there's no need for this. In fact, want me to just revert it? thanks, greg k-h ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3 00/24] i.MX Media Driver
On Mon, 2017-01-30 at 13:06 +, Russell King - ARM Linux wrote: > > The central issue seems to be that I think media pad links / media bus > > formats should describe physical links, such as parallel or serial > > buses, and the formats of pixels flowing through them, whereas Steve > > would like to extend them to describe software transports and in-memory > > formats. > > This probably isn't the right place to attach this comment in this > thread, but... the issue of media bus formats matching physical buses > is an argument that I think is already lost. It is unfortunate that the parallel format definitions have been reused for the MIPI logical formats, but I suppose we have to live with that. Still, I think this is not a reason to open the floodgates and start describing in-memory formats using MEDIA_BUS_FMT_* Does at least the combination of logical format and number of lanes uniquiely describe the physical format? For the 4-lane LVDS bus formats there are JEIDA/VESA variants where just the bit ordering is different (MEDIA_BUS_FMT_RGB888_1X7X4_SPWG, MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA). > For example, take the 10-bit bayer formats: > > #define MEDIA_BUS_FMT_SBGGR10_1X10 0x3007 > #define MEDIA_BUS_FMT_SGBRG10_1X10 0x300e > #define MEDIA_BUS_FMT_SGRBG10_1X10 0x300a > #define MEDIA_BUS_FMT_SRGGB10_1X10 0x300f > > These are commonly used on CSI serial buses (see the smiapp driver for > example). From the description at the top of the file, it says the > 1X10 means that one pixel is transferred as one 10-bit sample. > > However, the format on wire is somewhat different - four pixels are > transmitted over five bytes: > > P0 P1 P2 P3 P0 P1 P2 P3 > 8-bit 8-bit 8-bit 8-bit 2-bit 2-bit 2-bit 2-bit > > This gives two problems: > 1) it doesn't fit in any sensible kind of "one pixel transferred as >N M-bit samples" description because the pixel/sample values >(depending how you look at them) are broken up. > > 2) changing this will probably be a user visible change, as things >like smiapp are already in use. > > So, I think what we actually have is the media bus formats describing > the _logical_ bus format. Yes, one pixel is transferred as one 10-bit > sample in this case. Yes. I suppose one way to look at it is that the MIPI CSI-2 specified formats are representations of corresponding parallel bus formats. > To help illustrate my point, consider the difference between > MEDIA_BUS_FMT_RGB565_1X16 and MEDIA_BUS_FMT_RGB565_2X8_BE or > MEDIA_BUS_FMT_RGB565_2X8_LE. RGB565_1X16 means 1 pixel over an effective > 16-bit wide bus (if it's not 16-bit, then it has to be broken up into > separate "samples".) RGB565_2X8 means 1 pixel as two 8-bit samples. > > So, the 10-bit bayer is 1 pixel as 1.25 bytes. Or is it, over a serial > bus. Using the RGB565 case, 10-bit bayer over a 4 lane CSI bus becomes > interesting: > > first byte 2nd 3rd > lane 1P0 9:2 S0 P7 9:2 > lane 2P1 9:2 P4 9:2 S1 > lane 3P2 9:2 P5 9:2 P8 9:2 > lane 4P3 9:2 P6 9:2 P9 9:2 > > S0 = P0/P1/P2/P3 least significant two bits > S1 = P4/P5/P6/P7 least significant two bits > > or 2 lane CSI: > first byte 2nd 3rd 4th 5th > lane 1P0 9:2 P2 S0 P5 P7 > lane 2P1 9:2 P3 P4 P6 S1 > > or 1 lane CSI: > lane 1P0 P1 P2 P3 S0 P4 P5 P6 P7 S1 P8 P9 ... These do look like three different media bus formats to me. regards Philipp ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3 00/24] i.MX Media Driver
On Mon, Jan 30, 2017 at 05:22:01PM -0800, Steve Longerbeam wrote: > Edit: I see a subdev that is missing: the video mux. Did you enable > CONFIG_VIDEO_MULTIPLEXER? Yes, and that's where the problem is - the video-multiplexer is missing the module aliases to allow it to be automatically loaded. -- RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up according to speedtest.net. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3 17/24] media: imx: Add CSI subdev driver
On Fri, Jan 06, 2017 at 06:11:35PM -0800, Steve Longerbeam wrote: > This is a media entity subdevice for the i.MX Camera > Serial Interface module. > > Signed-off-by: Steve Longerbeam > --- The lack of s_frame_interval/g_frame_interval in this driver means: media-ctl -v -d /dev/media1 --set-v4l2 '"imx6-mipi-csi2":1[fmt:SGBRG8/512x512@1/30]' Opening media device /dev/media1 Enumerating entities Found 29 entities Enumerating pads and links Setting up format SGBRG8 512x512 on pad imx6-mipi-csi2/1 Format set: SGBRG8 512x512 Setting up frame interval 1/30 on entity imx6-mipi-csi2 Unable to set frame interval: Inappropriate ioctl for device (-25)Unable to setup formats: Inappropriate ioctl for device (25) which causes the setup of the next element in the chain to also fail (because the above media-ctl command doesn't set the sink on the ipu1 csi0 mux.) It seems to me that without the frame interval supported throughout the chain, there's no way for an application to properly negotiate the capture parameters via the "try" mechanism, since it has no idea whether the frame rate it wants is supported throughout the subdev chain. Eg, the sensor may be able to do 100fps but there could be something in the pipeline that restricts it due to bandwidth. -- RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up according to speedtest.net. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3 00/24] i.MX Media Driver
On Mon, Jan 30, 2017 at 05:22:01PM -0800, Steve Longerbeam wrote: > I'm also having trouble finding a datasheet for it, but from what > I've read, it has a MIPI CSI-2 interface. It should work fine as long > as it presents a single source pad, registers asynchronously, and > sets its entity function to MEDIA_ENT_F_CAM_SENSOR. Yes, it is MIPI CSI-2, and yes it has a single source pad, registers asynchronously, but that's about as far as it goes. The structure is a camera sensor followed by some processing. So just like the smiapp code, I've ended up with multiple subdevs describing each stage of the sensors pipeline. Just like smiapp, the camera sensor block (which is the very far end of the pipeline) is marked with MEDIA_ENT_F_CAM_SENSOR. However, in front of that is the binner, which just like smiapp gets a separate entity. It's this entity which is connected to the mipi-csi2 subdev. Unlike smiapp, which does not set an entity function, I set my binner entity as MEDIA_ENT_F_PROC_VIDEO_SCALER on the basis that that is what V4L2 documentation recommend: - .. row 27 .. _MEDIA-ENT-F-PROC-VIDEO-SCALER: - ``MEDIA_ENT_F_PROC_VIDEO_SCALER`` - Video scaler. An entity capable of video scaling must have at least one sink pad and one source pad, and scale the video frame(s) received on its sink pad(s) to a different resolution output on its source pad(s). The range of supported scaling ratios is entity-specific and can differ between the horizontal and vertical directions (in particular scaling can be supported in one direction only). Binning and skipping are considered as scaling. This causes attempts to configure the ipu1_csi0 interface to fail: media-ctl -v -d /dev/media1 --set-v4l2 '"ipu1_csi0":1[fmt:SGBRG8/512x512@1/30]' Opening media device /dev/media1 Enumerating entities Found 29 entities Enumerating pads and links Setting up format SGBRG8 512x512 on pad ipu1_csi0/1 Unable to set format: No such device (-19) Unable to setup formats: No such device (19) and in the kernel log: ipu1_csi0: no sensor attached And yes, I already know that my next problem is going to be that the bayer formats are not supported in your driver (just like Philipp's driver) but adding them should not be difficult... but only once this issue is resolved. -- RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up according to speedtest.net. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 001] staging: wlan-ng: Add tabstop preceding the statement
On Mon, Jan 30, 2017 at 08:00:36PM -0800, Joe Perches wrote:
> On Mon, 2017-01-30 at 17:44 +0100, Greg Kroah-Hartman wrote:
> > On Mon, Jan 30, 2017 at 11:31:42AM -0500, Maksymilian Piechota wrote:
> > > This patch fixes the checkpatch.pl warning:
> > >
> > > WARNING: Statements should start on a tabstop
> > >
> > > Signed-off-by: Maksymilian Piechota
> > > ---
> > > drivers/staging/wlan-ng/prism2mgmt.c | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/staging/wlan-ng/prism2mgmt.c
> > > b/drivers/staging/wlan-ng/prism2mgmt.c
> > > index 16fb2d3..2d67125 100644
> > > --- a/drivers/staging/wlan-ng/prism2mgmt.c
> > > +++ b/drivers/staging/wlan-ng/prism2mgmt.c
> > > @@ -1308,7 +1308,7 @@ int prism2mgmt_wlansniff(struct wlandevice
> > > *wlandev, void *msgp)
> > > hw->sniffhdr = 0;
> > > wlandev->netdev->type = ARPHRD_IEEE80211_PRISM;
> > > } else
> > > - if ((msg->wlanheader.status ==
> > > + if ((msg->wlanheader.status ==
> > >P80211ENUM_msgitem_status_data_ok)
> > > && (msg->wlanheader.data == P80211ENUM_truth_true)) {
> > > hw->sniffhdr = 1;
> >
> > Hm, this all doesn't look correct now, does it? Please fix up the whole
> > if statement here.
>
> Ideally, it'd look something like:
>
> /* Set the driver state */
> /* Do we want the prism2 header? */
> if (msg->prismheader.status ==
> P80211ENUM_msgitem_status_data_ok &&
> msg->prismheader.data == P80211ENUM_truth_true) {
> hw->sniffhdr = 0;
> wlandev->netdev->type = ARPHRD_IEEE80211_PRISM;
> } else if (msg->wlanheader.status ==
> P80211ENUM_msgitem_status_data_ok &&
> msg->wlanheader.data == P80211ENUM_truth_true) {
> hw->sniffhdr = 1;
> wlandev->netdev->type = ARPHRD_IEEE80211_PRISM;
> } else {
> wlandev->netdev->type = ARPHRD_IEEE80211;
> }
>
> with the unnecessary parentheses removed,
> the logical continuations at the end-of-line,
> and the else if on a single line.
>
I must admit it looks better, but this way we get 2 warnings instead of
1 (before my changes). What is the policy? Can we ignore more warnings
in order to get cleaner code?
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3 17/24] media: imx: Add CSI subdev driver
On Fri, Jan 06, 2017 at 06:11:35PM -0800, Steve Longerbeam wrote:
> +static int csi_link_validate(struct v4l2_subdev *sd,
> + struct media_link *link,
> + struct v4l2_subdev_format *source_fmt,
> + struct v4l2_subdev_format *sink_fmt)
> +{
> + struct csi_priv *priv = v4l2_get_subdevdata(sd);
> + bool is_csi2;
> + int ret;
> +
> + ret = v4l2_subdev_link_validate_default(sd, link, source_fmt, sink_fmt);
> + if (ret)
> + return ret;
> +
> + priv->sensor = __imx_media_find_sensor(priv->md, &priv->sd.entity);
> + if (IS_ERR(priv->sensor)) {
> + v4l2_err(&priv->sd, "no sensor attached\n");
> + ret = PTR_ERR(priv->sensor);
> + priv->sensor = NULL;
> + return ret;
> + }
> +
> + ret = v4l2_subdev_call(priv->sensor->sd, video, g_mbus_config,
> +&priv->sensor_mbus_cfg);
> + if (ret)
> + return ret;
> +
> + is_csi2 = (priv->sensor_mbus_cfg.type == V4L2_MBUS_CSI2);
> +
> + if (is_csi2) {
> + int vc_num = 0;
> + /*
> + * NOTE! It seems the virtual channels from the mipi csi-2
> + * receiver are used only for routing by the video mux's,
> + * or for hard-wired routing to the CSI's. Once the stream
> + * enters the CSI's however, they are treated internally
> + * in the IPU as virtual channel 0.
> + */
> +#if 0
> + vc_num = imx_media_find_mipi_csi2_channel(priv->md,
> + &priv->sd.entity);
> + if (vc_num < 0)
> + return vc_num;
> +#endif
> + ipu_csi_set_mipi_datatype(priv->csi, vc_num,
> + &priv->format_mbus[priv->input_pad]);
This seems (at least to me) to go against the spirit of the subdev
negotiation. Here, you seem to bypass the negotiation performed
between the CSI and the attached device, wanting to grab the
format from the CSI2 sensor directly. That bypasses negotiation
performed at the CSI2 subdev and video muxes.
The same goes for the frame rate monitoring code - imho, the frame
rate should be something that results from negotiation with the
neighbouring element, not by poking at some entity that is several
entities away.
--
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 001] staging: wlan-ng: Add tabstop preceding the statement
On Tue, 2017-01-31 at 06:04 -0500, Maksymilian Piechota wrote:
> On Mon, Jan 30, 2017 at 08:00:36PM -0800, Joe Perches wrote:
> > On Mon, 2017-01-30 at 17:44 +0100, Greg Kroah-Hartman wrote:
> > > On Mon, Jan 30, 2017 at 11:31:42AM -0500, Maksymilian Piechota wrote:
> > > > This patch fixes the checkpatch.pl warning:
> > > >
> > > > WARNING: Statements should start on a tabstop
> > > >
> > > > Signed-off-by: Maksymilian Piechota
> > > > ---
> > > > drivers/staging/wlan-ng/prism2mgmt.c | 2 +-
> > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/staging/wlan-ng/prism2mgmt.c
> > > > b/drivers/staging/wlan-ng/prism2mgmt.c
> > > > index 16fb2d3..2d67125 100644
> > > > --- a/drivers/staging/wlan-ng/prism2mgmt.c
> > > > +++ b/drivers/staging/wlan-ng/prism2mgmt.c
> > > > @@ -1308,7 +1308,7 @@ int prism2mgmt_wlansniff(struct wlandevice
> > > > *wlandev, void *msgp)
> > > > hw->sniffhdr = 0;
> > > > wlandev->netdev->type = ARPHRD_IEEE80211_PRISM;
> > > > } else
> > > > - if ((msg->wlanheader.status ==
> > > > + if ((msg->wlanheader.status ==
> > > > P80211ENUM_msgitem_status_data_ok)
> > > > && (msg->wlanheader.data ==
> > > > P80211ENUM_truth_true)) {
> > > > hw->sniffhdr = 1;
> > >
> > > Hm, this all doesn't look correct now, does it? Please fix up the whole
> > > if statement here.
> >
> > Ideally, it'd look something like:
> >
> > /* Set the driver state */
> > /* Do we want the prism2 header? */
> > if (msg->prismheader.status ==
> > P80211ENUM_msgitem_status_data_ok &&
> > msg->prismheader.data == P80211ENUM_truth_true) {
> > hw->sniffhdr = 0;
> > wlandev->netdev->type = ARPHRD_IEEE80211_PRISM;
> > } else if (msg->wlanheader.status ==
> > P80211ENUM_msgitem_status_data_ok &&
> >msg->wlanheader.data == P80211ENUM_truth_true) {
> > hw->sniffhdr = 1;
> > wlandev->netdev->type = ARPHRD_IEEE80211_PRISM;
> > } else {
> > wlandev->netdev->type = ARPHRD_IEEE80211;
> > }
> >
> > with the unnecessary parentheses removed,
> > the logical continuations at the end-of-line,
> > and the else if on a single line.
> >
>
> I must admit it looks better, but this way we get 2 warnings instead of
> 1 (before my changes). What is the policy? Can we ignore more warnings
> in order to get cleaner code?
Yes please.
checkpatch is just a guide, it's brainless.
The reason these lines are > 80 columns is
overly long/verbose identifiers.
If you really want to clean up the code here,
the P90211ENUM_ prefixes are a bit misleading
as they all are #define and not enums at all.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 001] staging: wlan-ng: Add tabstop preceding the statement
On Tue, Jan 31, 2017 at 03:18:45AM -0800, Joe Perches wrote:
> On Tue, 2017-01-31 at 06:04 -0500, Maksymilian Piechota wrote:
> > On Mon, Jan 30, 2017 at 08:00:36PM -0800, Joe Perches wrote:
> > > On Mon, 2017-01-30 at 17:44 +0100, Greg Kroah-Hartman wrote:
> > > > On Mon, Jan 30, 2017 at 11:31:42AM -0500, Maksymilian Piechota wrote:
> > > > > This patch fixes the checkpatch.pl warning:
> > > > >
> > > > > WARNING: Statements should start on a tabstop
> > > > >
> > > > > Signed-off-by: Maksymilian Piechota
> > > > > ---
> > > > > drivers/staging/wlan-ng/prism2mgmt.c | 2 +-
> > > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/drivers/staging/wlan-ng/prism2mgmt.c
> > > > > b/drivers/staging/wlan-ng/prism2mgmt.c
> > > > > index 16fb2d3..2d67125 100644
> > > > > --- a/drivers/staging/wlan-ng/prism2mgmt.c
> > > > > +++ b/drivers/staging/wlan-ng/prism2mgmt.c
> > > > > @@ -1308,7 +1308,7 @@ int prism2mgmt_wlansniff(struct wlandevice
> > > > > *wlandev, void *msgp)
> > > > > hw->sniffhdr = 0;
> > > > > wlandev->netdev->type = ARPHRD_IEEE80211_PRISM;
> > > > > } else
> > > > > - if ((msg->wlanheader.status ==
> > > > > + if ((msg->wlanheader.status ==
> > > > >P80211ENUM_msgitem_status_data_ok)
> > > > > && (msg->wlanheader.data ==
> > > > > P80211ENUM_truth_true)) {
> > > > > hw->sniffhdr = 1;
> > > >
> > > > Hm, this all doesn't look correct now, does it? Please fix up the whole
> > > > if statement here.
> > >
> > > Ideally, it'd look something like:
> > >
> > > /* Set the driver state */
> > > /* Do we want the prism2 header? */
> > > if (msg->prismheader.status ==
> > > P80211ENUM_msgitem_status_data_ok &&
> > > msg->prismheader.data == P80211ENUM_truth_true) {
> > > hw->sniffhdr = 0;
> > > wlandev->netdev->type = ARPHRD_IEEE80211_PRISM;
> > > } else if (msg->wlanheader.status ==
> > > P80211ENUM_msgitem_status_data_ok &&
> > > msg->wlanheader.data == P80211ENUM_truth_true) {
> > > hw->sniffhdr = 1;
> > > wlandev->netdev->type = ARPHRD_IEEE80211_PRISM;
> > > } else {
> > > wlandev->netdev->type = ARPHRD_IEEE80211;
> > > }
> > >
> > > with the unnecessary parentheses removed,
> > > the logical continuations at the end-of-line,
> > > and the else if on a single line.
> > >
> >
> > I must admit it looks better, but this way we get 2 warnings instead of
> > 1 (before my changes). What is the policy? Can we ignore more warnings
> > in order to get cleaner code?
>
> Yes please.
>
> checkpatch is just a guide, it's brainless.
>
> The reason these lines are > 80 columns is
> overly long/verbose identifiers.
>
> If you really want to clean up the code here,
> the P90211ENUM_ prefixes are a bit misleading
> as they all are #define and not enums at all.
>
But would you like me to remove this prefixes for all of the enums from
p80211types.h? Are you sure it won't cause any symbol conflicts?
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 001] staging: wlan-ng: Add tabstop preceding the statement
On Tue, 2017-01-31 at 06:33 -0500, Maksymilian Piechota wrote: > On Tue, Jan 31, 2017 at 03:18:45AM -0800, Joe Perches wrote: > > checkpatch is just a guide, it's brainless. > > > > The reason these lines are > 80 columns is > > overly long/verbose identifiers. > > > > If you really want to clean up the code here, > > the P90211ENUM_ prefixes are a bit misleading > > as they all are #define and not enums at all. > > > But would you like me to remove this prefixes for all of the enums from > p80211types.h? Are you sure it won't cause any symbol conflicts? No, I don't want that, I'd prefer you think about it. Also, a useful effort would be to (from the README) TODO: [] - move to use the in-kernel wireless stack where most all of the P80211 code would be removed. Anyway, sure, use checkpatch as a tool to help when learning the process of how to submit patches properly. Then move on to more thoroughly understand a block of code in the kernel that can be improved with cleaner style and logic and bug fixes you could submit. That's be much more appreciated than random 80 column fixups and strict checkpatch compliance done without a thorough understanding of the code. In any case, welcome, hope you stick around. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 001] staging: wlan-ng: Add tabstop preceding the statement
On Tue, Jan 31, 2017 at 04:07:04AM -0800, Joe Perches wrote: > On Tue, 2017-01-31 at 06:33 -0500, Maksymilian Piechota wrote: > > On Tue, Jan 31, 2017 at 03:18:45AM -0800, Joe Perches wrote: > > > checkpatch is just a guide, it's brainless. > > > > > > The reason these lines are > 80 columns is > > > overly long/verbose identifiers. > > > > > > If you really want to clean up the code here, > > > the P90211ENUM_ prefixes are a bit misleading > > > as they all are #define and not enums at all. > > > > > But would you like me to remove this prefixes for all of the enums from > > p80211types.h? Are you sure it won't cause any symbol conflicts? > > No, I don't want that, I'd prefer you think about it. > > Also, a useful effort would be to (from the README) > > TODO: > [] > - move to use the in-kernel wireless stack > > where most all of the P80211 code would be removed. > > Anyway, sure, use checkpatch as a tool to help when > learning the process of how to submit patches properly. > Then move on to more thoroughly understand a block of > code in the kernel that can be improved with cleaner > style and logic and bug fixes you could submit. > > That's be much more appreciated than random 80 column > fixups and strict checkpatch compliance done without > a thorough understanding of the code. > > In any case, welcome, hope you stick around. It's clear that there are a lot more usefull patches than 80 column fix, but as you said, I learn how to submit patches and there is is constraint to submit only one checkpatch warning fix. Many thanks for advice how to proceed. I will take a look at it when I finish Greg's tutorial. Thank you, I hope to have a significant contribution to Linux Community. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3 00/24] i.MX Media Driver
On Tue, Jan 31, 2017 at 11:09:24AM +0100, Philipp Zabel wrote: > On Mon, 2017-01-30 at 13:06 +, Russell King - ARM Linux wrote: > > To help illustrate my point, consider the difference between > > MEDIA_BUS_FMT_RGB565_1X16 and MEDIA_BUS_FMT_RGB565_2X8_BE or > > MEDIA_BUS_FMT_RGB565_2X8_LE. RGB565_1X16 means 1 pixel over an effective > > 16-bit wide bus (if it's not 16-bit, then it has to be broken up into > > separate "samples".) RGB565_2X8 means 1 pixel as two 8-bit samples. > > > > So, the 10-bit bayer is 1 pixel as 1.25 bytes. Or is it, over a serial > > bus. Using the RGB565 case, 10-bit bayer over a 4 lane CSI bus becomes > > interesting: > > > > first byte 2nd 3rd > > lane 1 P0 9:2 S0 P7 9:2 > > lane 2 P1 9:2 P4 9:2 S1 > > lane 3 P2 9:2 P5 9:2 P8 9:2 > > lane 4 P3 9:2 P6 9:2 P9 9:2 > > > > S0 = P0/P1/P2/P3 least significant two bits > > S1 = P4/P5/P6/P7 least significant two bits > > > > or 2 lane CSI: > > first byte 2nd 3rd 4th 5th > > lane 1 P0 9:2 P2 S0 P5 P7 > > lane 2 P1 9:2 P3 P4 P6 S1 > > > > or 1 lane CSI: > > lane 1 P0 P1 P2 P3 S0 P4 P5 P6 P7 S1 P8 P9 ... > > These do look like three different media bus formats to me. This isn't limited to the serial side - the parallel bus side between the CSI2 interface and CSI2IPU wrapper, and the CSI2IPU wrapper and the CS0/1 interfaces is much the same with 10-bit bayer. Think of the CSI2 <-> CSI2IPU bit as the 4-lane case, lane 0 ending up on the least significant 8 bits of the 32-bit bus, lane 3 on the top 8-bits. Post CSI2IPU, it talks about a 16-bit bus in the diagrams, so that's kind of the 2-lane case above... -- RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up according to speedtest.net. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3 00/24] i.MX Media Driver
On Tue, 2017-01-31 at 13:14 +, Russell King - ARM Linux wrote: > On Tue, Jan 31, 2017 at 11:09:24AM +0100, Philipp Zabel wrote: > > On Mon, 2017-01-30 at 13:06 +, Russell King - ARM Linux wrote: > > > To help illustrate my point, consider the difference between > > > MEDIA_BUS_FMT_RGB565_1X16 and MEDIA_BUS_FMT_RGB565_2X8_BE or > > > MEDIA_BUS_FMT_RGB565_2X8_LE. RGB565_1X16 means 1 pixel over an effective > > > 16-bit wide bus (if it's not 16-bit, then it has to be broken up into > > > separate "samples".) RGB565_2X8 means 1 pixel as two 8-bit samples. > > > > > > So, the 10-bit bayer is 1 pixel as 1.25 bytes. Or is it, over a serial > > > bus. Using the RGB565 case, 10-bit bayer over a 4 lane CSI bus becomes > > > interesting: > > > > > > first byte 2nd 3rd > > > lane 1P0 9:2 S0 P7 9:2 > > > lane 2P1 9:2 P4 9:2 S1 > > > lane 3P2 9:2 P5 9:2 P8 9:2 > > > lane 4P3 9:2 P6 9:2 P9 9:2 > > > > > > S0 = P0/P1/P2/P3 least significant two bits > > > S1 = P4/P5/P6/P7 least significant two bits > > > > > > or 2 lane CSI: > > > first byte 2nd 3rd 4th 5th > > > lane 1P0 9:2 P2 S0 P5 P7 > > > lane 2P1 9:2 P3 P4 P6 S1 > > > > > > or 1 lane CSI: > > > lane 1P0 P1 P2 P3 S0 P4 P5 P6 P7 S1 P8 P9 ... > > > > These do look like three different media bus formats to me. > > This isn't limited to the serial side - the parallel bus side between > the CSI2 interface and CSI2IPU wrapper, and the CSI2IPU wrapper and > the CS0/1 interfaces is much the same with 10-bit bayer. > > Think of the CSI2 <-> CSI2IPU bit as the 4-lane case, lane 0 ending > up on the least significant 8 bits of the 32-bit bus, lane 3 on the > top 8-bits. > > Post CSI2IPU, it talks about a 16-bit bus in the diagrams, so that's > kind of the 2-lane case above... You are right, on the parallel buses the format most definitely is not MEDIA_BUS_FMT_SBGGR10_1X10. We don't have any representation of the 32-bit bus between CSI2 host and CSI2IPU gasket because we model the two as a single entity, but the four 16-bit parallel buses between the CSI2IPU gasket and the IPU1/2 CSI0/1 probably should be set to a custom format describing this accurately. regards Philipp ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3 20/24] media: imx: Add Camera Interface subdev driver
On Fri, Jan 20, 2017 at 03:38:28PM +0100, Hans Verkuil wrote:
> Should be set to something like 'platform:imx-media-camif'. v4l2-compliance
> should complain about this.
... and more.
Driver Info:
Driver name : imx-media-camif
Card type : imx-media-camif
Bus info :
Driver version: 4.10.0
Capabilities : 0x8421
Video Capture
Streaming
Extended Pix Format
Device Capabilities
Device Caps : 0x0421
Video Capture
Streaming
Extended Pix Format
Compliance test for device /dev/video3 (not using libv4l2):
Required ioctls:
fail: v4l2-compliance.cpp(244): string empty
fail: v4l2-compliance.cpp(297): check_ustring(vcap.bus_info,
sizeof(vcap.bus_info))
test VIDIOC_QUERYCAP: FAIL
Allow for multiple opens:
test second video open: OK
fail: v4l2-compliance.cpp(244): string empty
fail: v4l2-compliance.cpp(297): check_ustring(vcap.bus_info,
sizeof(vcap.bus_info))
test VIDIOC_QUERYCAP: FAIL
test VIDIOC_G/S_PRIORITY: OK
Debug ioctls:
test VIDIOC_DBG_G/S_REGISTER: OK
test VIDIOC_LOG_STATUS: OK (Not Supported)
Input ioctls:
test VIDIOC_G/S_TUNER/ENUM_FREQ_BANDS: OK (Not Supported)
test VIDIOC_G/S_FREQUENCY: OK (Not Supported)
test VIDIOC_S_HW_FREQ_SEEK: OK (Not Supported)
test VIDIOC_ENUMAUDIO: OK (Not Supported)
fail: v4l2-test-input-output.cpp(382): std == 0
fail: v4l2-test-input-output.cpp(437): invalid attributes for
input 0
test VIDIOC_G/S/ENUMINPUT: FAIL
test VIDIOC_G/S_AUDIO: OK (Not Supported)
Inputs: 0 Audio Inputs: 0 Tuners: 0
Output ioctls:
test VIDIOC_G/S_MODULATOR: OK (Not Supported)
test VIDIOC_G/S_FREQUENCY: OK (Not Supported)
test VIDIOC_ENUMAUDOUT: OK (Not Supported)
test VIDIOC_G/S/ENUMOUTPUT: OK (Not Supported)
test VIDIOC_G/S_AUDOUT: OK (Not Supported)
Outputs: 0 Audio Outputs: 0 Modulators: 0
Input/Output configuration ioctls:
test VIDIOC_ENUM/G/S/QUERY_STD: OK (Not Supported)
test VIDIOC_ENUM/G/S/QUERY_DV_TIMINGS: OK (Not Supported)
test VIDIOC_DV_TIMINGS_CAP: OK (Not Supported)
test VIDIOC_G/S_EDID: OK (Not Supported)
Control ioctls:
test VIDIOC_QUERY_EXT_CTRL/QUERYMENU: OK
test VIDIOC_QUERYCTRL: OK
test VIDIOC_G/S_CTRL: OK
test VIDIOC_G/S/TRY_EXT_CTRLS: OK
fail: v4l2-test-controls.cpp(779): subscribe event for control
'Camera Controls' failed
test VIDIOC_(UN)SUBSCRIBE_EVENT/DQEVENT: FAIL
test VIDIOC_G/S_JPEGCOMP: OK (Not Supported)
Standard Controls: 13 Private Controls: 0
Format ioctls:
test VIDIOC_ENUM_FMT/FRAMESIZES/FRAMEINTERVALS: OK
test VIDIOC_G/S_PARM: OK (Not Supported)
test VIDIOC_G_FBUF: OK (Not Supported)
fail: v4l2-test-formats.cpp(414): unknown pixelformat 42474752
for buftype 1
test VIDIOC_G_FMT: FAIL
test VIDIOC_TRY_FMT: OK (Not Supported)
test VIDIOC_S_FMT: OK (Not Supported)
test VIDIOC_G_SLICED_VBI_CAP: OK (Not Supported)
Codec ioctls:
test VIDIOC_(TRY_)ENCODER_CMD: OK (Not Supported)
test VIDIOC_G_ENC_INDEX: OK (Not Supported)
test VIDIOC_(TRY_)DECODER_CMD: OK (Not Supported)
Buffer ioctls:
test VIDIOC_REQBUFS/CREATE_BUFS/QUERYBUF: OK
fail: v4l2-test-buffers.cpp(500): q.has_expbuf(node)
test VIDIOC_EXPBUF: FAIL
Total: 39, Succeeded: 33, Failed: 6, Warnings: 0
Not all of these may be a result of Steve's code - this is running against
my gradually modified version to support bayer formats (which seems to be
the cause of the v4l2-test-formats.cpp failure... for some reason the
driver isn't enumerating all the formats.)
And that reason is the way that the formats are enumerated:
static int camif_enum_fmt_vid_cap(struct file *file, void *fh,
struct v4l2_fmtdesc *f)
{
const struct imx_media_pixfmt *cc;
u32 code;
int ret;
ret = imx_media_enum_format(&code, f->index, true, true);
if (ret)
return ret;
cc = imx_media_find_format(0, code, true, true);
if (!cc)
return -EINVAL;
When imx_media_enum_format() hits this entry in the table:
}, {
.fourcc = V4L2_PIX_FMT_BGR24,
.cs = IPUV3_COLORSPACE_RGB,
.bpp= 24,
}, {
becaues there's no .codes defined:
int imx_media_enum_format(u32 *code, u32 index, bool allow_rgb,
bool allow_planar)
Re: [PATCH v3 00/24] i.MX Media Driver
Hi Steve, I have just tested the imx-media-staging-md-wip branch on a Nitrogen6X with a tc358743 (BD_HDMI_MIPI HDMI to MIPI CSI-2 receiver board). Some observations: # Link pipeline media-ctl -l "'tc358743 1-000f':0->'imx6-mipi-csi2':0[1]" media-ctl -l "'imx6-mipi-csi2':1->'ipu1_csi0_mux':0[1]" media-ctl -l "'ipu1_csi0_mux':2->'ipu1_csi0':0[1]" media-ctl -l "'ipu1_csi0':2->'ipu1_csi0 capture':0[1]" # Provide an EDID to the HDMI source v4l2-ctl -d /dev/v4l-subdev2 --set-edid=file=edid-1080p.hex # At this point the HDMI source is enabled and sends a 1080p60 signal # Configure detected DV timings media-ctl --set-dv "'tc358743 1-000f':0" # Set pad formats media-ctl --set-v4l2 "'tc358743 1-000f':0[fmt:UYVY/1920x1080]" media-ctl --set-v4l2 "'imx6-mipi-csi2':1[fmt:UYVY2X8/1920x1080]" media-ctl --set-v4l2 "'ipu1_csi0_mux':2[fmt:UYVY2X8/1920x1080]" media-ctl --set-v4l2 "'ipu1_csi0':2[fmt:AYUV32/1920x1080]" v4l2-ctl -d /dev/video4 -V # This still is configured to 640x480, which is inconsistent with # the 'ipu1_csi0':2 pad format. The pad set_fmt above should # have set this, too. v4l2-ctl --list-formats -d /dev/video4 # This lists all the RGB formats, which it shouldn't. There is # no CSC in this pipeline, so we should be limited to YUV formats # only. # Set capture format v4l2-ctl -d /dev/video4 -v width=1920,height=1080,pixelformat=UYVY v4l2-ctl -d /dev/video4 -V # Now the capture format is correctly configured to 1920x1080. v4l2-ctl -d 4 --list-frameintervals=width=1920,height=1080,pixelformat=UYVY # This lists nothing. We should at least provide the 'ipu1_csi0':2 pad # frame interval. In the future this should list fractions achievable # via frame skipping. v4l2-compliance -d /dev/video4 # This fails two tests: # fail: v4l2-test-input-output.cpp(383): std == 0 # fail: v4l2-test-input-output.cpp(449): invalid attributes for input 0 # test VIDIOC_G/S/ENUMINPUT: FAIL # and # fail: v4l2-test-controls.cpp(782): subscribe event for control 'User Controls' failed # test VIDIOC_(UN)SUBSCRIBE_EVENT/DQEVENT: FAIL # (Slowly) stream JPEG images to a display host: gst-launch-1.0 -v v4l2src device=/dev/video4 ! jpegenc ! rtpjpegpay ! udpsink I've done this a few times, and sometimes I only get a "ipu1_csi0: EOF timeout" message when starting streaming. regards Philipp ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3 00/24] i.MX Media Driver
On Tue, Jan 31, 2017 at 02:35:00PM +0100, Philipp Zabel wrote: > On Tue, 2017-01-31 at 13:14 +, Russell King - ARM Linux wrote: > > This isn't limited to the serial side - the parallel bus side between > > the CSI2 interface and CSI2IPU wrapper, and the CSI2IPU wrapper and > > the CS0/1 interfaces is much the same with 10-bit bayer. > > > > Think of the CSI2 <-> CSI2IPU bit as the 4-lane case, lane 0 ending > > up on the least significant 8 bits of the 32-bit bus, lane 3 on the > > top 8-bits. > > > > Post CSI2IPU, it talks about a 16-bit bus in the diagrams, so that's > > kind of the 2-lane case above... > > You are right, on the parallel buses the format most definitely is not > MEDIA_BUS_FMT_SBGGR10_1X10. We don't have any representation of the > 32-bit bus between CSI2 host and CSI2IPU gasket because we model the two > as a single entity, but the four 16-bit parallel buses between the > CSI2IPU gasket and the IPU1/2 CSI0/1 probably should be set to a custom > format describing this accurately. Yep. I should also point out that there's a very odd transformation going on somewhere, and I don't yet know where. The sensor is definitely outputting GBRG format, but what seems to get written into memory is RGGB format. It's somewhere post CSI, because when I was using the (broken) CSI compander with 10 bit bayer, the red compander channel affected the red channel output from the camera, but changed the green component written to memory... it's very much like either the first line gets lost somewhere, or the odd/even lines are transposed. It could also be a gstreamer bug. As I say, it's not something I've looked into deeply enough yet... too many other issues to chase down! -- RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up according to speedtest.net. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v2] staging: wlan-ng: This patch fixes the checkpatch.pl warning:
Fix checkpatch.pl warning:
WARNING: Statements should start on a tabstop
V2: whole if statement cleared up
Signed-off-by: Maksymilian Piechota
---
drivers/staging/wlan-ng/prism2mgmt.c | 6 ++
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/staging/wlan-ng/prism2mgmt.c
b/drivers/staging/wlan-ng/prism2mgmt.c
index 2d67125..5277f36 100644
--- a/drivers/staging/wlan-ng/prism2mgmt.c
+++ b/drivers/staging/wlan-ng/prism2mgmt.c
@@ -1307,10 +1307,8 @@ int prism2mgmt_wlansniff(struct wlandevice *wlandev,
void *msgp)
&& (msg->prismheader.data == P80211ENUM_truth_true)) {
hw->sniffhdr = 0;
wlandev->netdev->type = ARPHRD_IEEE80211_PRISM;
- } else
- if ((msg->wlanheader.status ==
-P80211ENUM_msgitem_status_data_ok)
- && (msg->wlanheader.data == P80211ENUM_truth_true)) {
+ } else if ((msg->wlanheader.status ==
P80211ENUM_msgitem_status_data_ok) &&
+ (msg->wlanheader.data == P80211ENUM_truth_true)) {
hw->sniffhdr = 1;
wlandev->netdev->type = ARPHRD_IEEE80211_PRISM;
} else {
--
2.1.4
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3 00/24] i.MX Media Driver
On Tue, 2017-01-31 at 14:54 +0100, Philipp Zabel wrote: > Hi Steve, > > I have just tested the imx-media-staging-md-wip branch on a Nitrogen6X > with a tc358743 (BD_HDMI_MIPI HDMI to MIPI CSI-2 receiver board). Some > observations: > > # Link pipeline > media-ctl -l "'tc358743 1-000f':0->'imx6-mipi-csi2':0[1]" > media-ctl -l "'imx6-mipi-csi2':1->'ipu1_csi0_mux':0[1]" > media-ctl -l "'ipu1_csi0_mux':2->'ipu1_csi0':0[1]" > media-ctl -l "'ipu1_csi0':2->'ipu1_csi0 capture':0[1]" > > # Provide an EDID to the HDMI source > v4l2-ctl -d /dev/v4l-subdev2 --set-edid=file=edid-1080p.hex > # At this point the HDMI source is enabled and sends a 1080p60 signal > # Configure detected DV timings > media-ctl --set-dv "'tc358743 1-000f':0" > > # Set pad formats > media-ctl --set-v4l2 "'tc358743 1-000f':0[fmt:UYVY/1920x1080]" > media-ctl --set-v4l2 "'imx6-mipi-csi2':1[fmt:UYVY2X8/1920x1080]" > media-ctl --set-v4l2 "'ipu1_csi0_mux':2[fmt:UYVY2X8/1920x1080]" I noticed this seems to get ignored. The format is incorrectly set to UYVY even though I request UYVY2X8 (see CSI2IPU chapter, Figure 19-10. YUV422-8 data reception in the reference manual), but it seems to work anyway. regards Philipp ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3 00/24] i.MX Media Driver
On Tue, Jan 31, 2017 at 03:25:10PM +0100, Philipp Zabel wrote: > On Tue, 2017-01-31 at 14:54 +0100, Philipp Zabel wrote: > > Hi Steve, > > > > I have just tested the imx-media-staging-md-wip branch on a Nitrogen6X > > with a tc358743 (BD_HDMI_MIPI HDMI to MIPI CSI-2 receiver board). Some > > observations: > > > > # Link pipeline > > media-ctl -l "'tc358743 1-000f':0->'imx6-mipi-csi2':0[1]" > > media-ctl -l "'imx6-mipi-csi2':1->'ipu1_csi0_mux':0[1]" > > media-ctl -l "'ipu1_csi0_mux':2->'ipu1_csi0':0[1]" > > media-ctl -l "'ipu1_csi0':2->'ipu1_csi0 capture':0[1]" > > > > # Provide an EDID to the HDMI source > > v4l2-ctl -d /dev/v4l-subdev2 --set-edid=file=edid-1080p.hex > > # At this point the HDMI source is enabled and sends a 1080p60 signal > > # Configure detected DV timings > > media-ctl --set-dv "'tc358743 1-000f':0" > > > > # Set pad formats > > media-ctl --set-v4l2 "'tc358743 1-000f':0[fmt:UYVY/1920x1080]" > > media-ctl --set-v4l2 "'imx6-mipi-csi2':1[fmt:UYVY2X8/1920x1080]" > > media-ctl --set-v4l2 "'ipu1_csi0_mux':2[fmt:UYVY2X8/1920x1080]" > > I noticed this seems to get ignored. The format is incorrectly set to > UYVY even though I request UYVY2X8 (see CSI2IPU chapter, Figure 19-10. > YUV422-8 data reception in the reference manual), but it seems to work > anyway. That's because the driver at imx-csi level bypasses the proper media pad formats on its sink pads, and instead goes poking about at the "sensor" to get the format. This is one of the reasons it wants to know which entity is the "sensor". The "sensor" stuff in there needs to be scrapped... -- RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up according to speedtest.net. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3 17/24] media: imx: Add CSI subdev driver
On Fri, 2017-01-06 at 18:11 -0800, Steve Longerbeam wrote:
[...]
> +static int csi_set_fmt(struct v4l2_subdev *sd,
> +struct v4l2_subdev_pad_config *cfg,
> +struct v4l2_subdev_format *sdformat)
> +{
> + struct csi_priv *priv = v4l2_get_subdevdata(sd);
> + struct v4l2_mbus_framefmt *infmt, *outfmt;
> + struct v4l2_rect crop;
> + int ret;
> +
> + if (sdformat->pad >= CSI_NUM_PADS)
> + return -EINVAL;
> +
> + if (priv->stream_on)
> + return -EBUSY;
> +
> + infmt = &priv->format_mbus[priv->input_pad];
> + outfmt = &priv->format_mbus[priv->output_pad];
> +
> + if (sdformat->pad == priv->output_pad) {
> + sdformat->format.code = infmt->code;
> + sdformat->format.field = infmt->field;
> + crop.left = priv->crop.left;
> + crop.top = priv->crop.top;
> + crop.width = sdformat->format.width;
> + crop.height = sdformat->format.height;
> + ret = csi_try_crop(priv, &crop);
This is the wrong way around, see also below.
Here the the output sdformat->format.width/height should be set to the
priv->crop.width/height (or priv->crop.width/height / 2, to enable
downscaling). The crop rectangle should not be changed by an output pad
set_fmt.
> + if (ret)
> + return ret;
> + sdformat->format.width = crop.width;
> + sdformat->format.height = crop.height;
> + }
> +
> + if (sdformat->which == V4L2_SUBDEV_FORMAT_TRY) {
> + cfg->try_fmt = sdformat->format;
> + } else {
> + priv->format_mbus[sdformat->pad] = sdformat->format;
> + /* Update the crop window if this is output pad */
> + if (sdformat->pad == priv->output_pad)
> + priv->crop = crop;
The crop rectangle instead should be reset to the full input frame when
the input pad format is set.
regards
Philipp
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3 17/24] media: imx: Add CSI subdev driver
On Tue, 2017-01-31 at 16:51 +0100, Philipp Zabel wrote:
> On Fri, 2017-01-06 at 18:11 -0800, Steve Longerbeam wrote:
> [...]
> > +static int csi_set_fmt(struct v4l2_subdev *sd,
> > + struct v4l2_subdev_pad_config *cfg,
> > + struct v4l2_subdev_format *sdformat)
> > +{
> > + struct csi_priv *priv = v4l2_get_subdevdata(sd);
> > + struct v4l2_mbus_framefmt *infmt, *outfmt;
> > + struct v4l2_rect crop;
> > + int ret;
> > +
> > + if (sdformat->pad >= CSI_NUM_PADS)
> > + return -EINVAL;
> > +
> > + if (priv->stream_on)
> > + return -EBUSY;
> > +
> > + infmt = &priv->format_mbus[priv->input_pad];
> > + outfmt = &priv->format_mbus[priv->output_pad];
> > +
> > + if (sdformat->pad == priv->output_pad) {
> > + sdformat->format.code = infmt->code;
> > + sdformat->format.field = infmt->field;
> > + crop.left = priv->crop.left;
> > + crop.top = priv->crop.top;
> > + crop.width = sdformat->format.width;
> > + crop.height = sdformat->format.height;
> > + ret = csi_try_crop(priv, &crop);
>
> This is the wrong way around, see also below.
>
> Here the the output sdformat->format.width/height should be set to the
> priv->crop.width/height (or priv->crop.width/height / 2, to enable
> downscaling). The crop rectangle should not be changed by an output pad
> set_fmt.
[...]
> The crop rectangle instead should be reset to the full input frame when
> the input pad format is set.
How about this:
--8<--
diff --git a/drivers/staging/media/imx/imx-media-csi.c
b/drivers/staging/media/imx/imx-media-csi.c
index 5e80a0871b139..8220e4204a125 100644
--- a/drivers/staging/media/imx/imx-media-csi.c
+++ b/drivers/staging/media/imx/imx-media-csi.c
@@ -484,6 +484,8 @@ static int csi_setup(struct csi_priv *priv)
if_fmt.field = outfmt->field;
ipu_csi_set_window(priv->csi, &priv->crop);
+ ipu_csi_set_downsize(priv->csi, priv->crop.width == 2 * outfmt->width,
+ priv->crop.height == 2 *
outfmt->height);
ipu_csi_init_interface(priv->csi, &sensor_mbus_cfg, &if_fmt);
@@ -830,15 +832,14 @@ static int csi_set_fmt(struct v4l2_subdev *sd,
switch (sdformat->pad) {
case CSI_SRC_PAD_DIRECT:
case CSI_SRC_PAD_IDMAC:
- crop.left = priv->crop.left;
- crop.top = priv->crop.top;
- crop.width = sdformat->format.width;
- crop.height = sdformat->format.height;
- ret = csi_try_crop(priv, &crop, sensor);
- if (ret)
- return ret;
- sdformat->format.width = crop.width;
- sdformat->format.height = crop.height;
+ if (sdformat->format.width < priv->crop.width * 3 / 4)
+ sdformat->format.width = priv->crop.width / 2;
+ else
+ sdformat->format.width = priv->crop.width;
+ if (sdformat->format.height < priv->crop.height * 3 / 4)
+ sdformat->format.height = priv->crop.height / 2;
+ else
+ sdformat->format.height = priv->crop.height;
if (sdformat->pad == CSI_SRC_PAD_IDMAC) {
cc = imx_media_find_format(0, sdformat->format.code,
@@ -887,6 +888,14 @@ static int csi_set_fmt(struct v4l2_subdev *sd,
}
break;
case CSI_SINK_PAD:
+ crop.left = 0;
+ crop.top = 0;
+ crop.width = sdformat->format.width;
+ crop.height = sdformat->format.height;
+ ret = csi_try_crop(priv, &crop, sensor);
+ if (ret)
+ return ret;
+
cc = imx_media_find_format(0, sdformat->format.code,
true, false);
if (!cc) {
@@ -904,9 +913,8 @@ static int csi_set_fmt(struct v4l2_subdev *sd,
} else {
priv->format_mbus[sdformat->pad] = sdformat->format;
priv->cc[sdformat->pad] = cc;
- /* Update the crop window if this is an output pad */
- if (sdformat->pad == CSI_SRC_PAD_DIRECT ||
- sdformat->pad == CSI_SRC_PAD_IDMAC)
+ /* Reset the crop window if this is the input pad */
+ if (sdformat->pad == CSI_SINK_PAD)
priv->crop = crop;
}
-->8--
regards
Philipp
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3 20/24] media: imx: Add Camera Interface subdev driver
On 01/31/2017 05:42 AM, Russell King - ARM Linux wrote:
On Fri, Jan 20, 2017 at 03:38:28PM +0100, Hans Verkuil wrote:
Should be set to something like 'platform:imx-media-camif'. v4l2-compliance
should complain about this.
... and more.
Right, in version 3 that you are working with, no v4l2-compliance fixes were
in yet. A lot of the compliance errors are fixed, please look in latest
branch
imx-media-staging-md-wip at g...@github.com:slongerbeam/mediatree.git.
Total: 39, Succeeded: 33, Failed: 6, Warnings: 0
Not all of these may be a result of Steve's code - this is running against
my gradually modified version to support bayer formats (which seems to be
the cause of the v4l2-test-formats.cpp failure... for some reason the
driver isn't enumerating all the formats.)
And that reason is the way that the formats are enumerated:
static int camif_enum_fmt_vid_cap(struct file *file, void *fh,
struct v4l2_fmtdesc *f)
{
const struct imx_media_pixfmt *cc;
u32 code;
int ret;
ret = imx_media_enum_format(&code, f->index, true, true);
if (ret)
return ret;
cc = imx_media_find_format(0, code, true, true);
if (!cc)
return -EINVAL;
When imx_media_enum_format() hits this entry in the table:
}, {
.fourcc = V4L2_PIX_FMT_BGR24,
.cs = IPUV3_COLORSPACE_RGB,
.bpp= 24,
}, {
becaues there's no .codes defined:
int imx_media_enum_format(u32 *code, u32 index, bool allow_rgb,
bool allow_planar)
{
...
*code = fmt->codes[0];
return 0;
}
So, we end up calling imx_media_find_format(0, 0, true, true), which
fails, returning NULL. That causes camif_enum_fmt_vid_cap() to
return -EINVAL.
So everything past this entry is unable to be enumerated.
I think this is a really round-about way of enumerating the pixel
formats when there are soo many entries in the table which have no
media bus code - there's absolutely no way that any of those entries
can ever be enumerated in this fashion, so they might as well not be
in the table...
That's my present solution to this problem, to #if 0 out all the
entries without any .codes field. I think the real answer is that
this needs a _separate_ function to enumerate the pixel formats for
camif_enum_fmt_vid_cap(). However, there may be other issues lurking
that I've not yet found (still trying to get this code to work...)
I believe this has been fixed in imx-media-staging-md-wip as well,
see imx-media-capture.c:capture_enum_fmt_vid_cap()
Camif subdev is gone, replaced with a set of exported functions
that allow attaching a capture device (and v4l2 interface) to a
calling subdev's output pad. See imx-media-capture.c.
The subdev's capture device interface is the only subdev that
can request a planar format from imx_media_enum_format().
All the others now (the non-device node pads), request only RGB
or packed YUV, or the IPU internal formats for IPU internal connections,
and these are the first entries in the table. The planar formats all are at
the end, which can only be enumerated by the capture device interfaces.
Steve
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 6/6] staging: bcm2835-v4l2: Apply spelling fixes from checkpatch.
Joe Perches writes:
> On Mon, 2017-01-30 at 12:05 -0800, Eric Anholt wrote:
>> Joe Perches writes:
>>
>> > On Fri, 2017-01-27 at 13:55 -0800, Eric Anholt wrote:
>> > > Generated with checkpatch.pl --fix-inplace and git add -p out of the
>> > > results.
>> >
>> > Maybe another.
>> >
>> > > diff --git a/drivers/staging/media/platform/bcm2835/mmal-vchiq.c
>> > > b/drivers/staging/media/platform/bcm2835/mmal-vchiq.c
>> >
>> > []
>> > > @@ -239,7 +239,7 @@ static int bulk_receive(struct vchiq_mmal_instance
>> > > *instance,
>> > > pr_err("buffer list empty trying to submit bulk
>> > > receive\n");
>> > >
>> > > /* todo: this is a serious error, we should never have
>> > > - * commited a buffer_to_host operation to the mmal
>> > > + * committed a buffer_to_host operation to the mmal
>> > > * port without the buffer to back it up (underflow
>> > > * handling) and there is no obvious way to deal with
>> > > * this - how is the mmal servie going to react when
>> >
>> > Perhaps s/servie/service/ ?
>>
>> I was trying to restrict this patch to just the fixes from checkpatch.
>
> That's the wrong thing to do if you're fixing
> spelling defects. checkpatch is just one mechanism
> to identify some, and definitely not all, typos and
> spelling defects.
>
> If you fixing, fix. Don't just rely on the brainless
> tools, use your decidedly non-mechanical brain.
"if you touch anything, you must fix everything." If that's how things
work, I would just retract the patch.
signature.asc
Description: PGP signature
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH] staging: gdm724x: fix sparse warnings in file gdm_lte.c
drivers/staging/gdm724x/gdm_lte.c:201:33: warning: incorrect type in assignment
(different base types)
drivers/staging/gdm724x/gdm_lte.c:201:33:expected unsigned int [unsigned]
[addressable] [usertype] ph_len
drivers/staging/gdm724x/gdm_lte.c:201:33:got restricted __be16 [usertype]
payload_len
drivers/staging/gdm724x/gdm_lte.c:311:39: warning: incorrect type in assignment
(different base types)
drivers/staging/gdm724x/gdm_lte.c:311:39:expected restricted __sum16
[addressable] [assigned] [usertype] icmp6_cksum
drivers/staging/gdm724x/gdm_lte.c:311:39:got int
Signed-off-by: Javier Rodriguez
---
drivers/staging/gdm724x/gdm_lte.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/staging/gdm724x/gdm_lte.c
b/drivers/staging/gdm724x/gdm_lte.c
index a182757..86b2b4f 100644
--- a/drivers/staging/gdm724x/gdm_lte.c
+++ b/drivers/staging/gdm724x/gdm_lte.c
@@ -198,7 +198,7 @@ static int icmp6_checksum(struct ipv6hdr *ipv6, u16 *ptr,
int len)
memset(&pseudo_header, 0, sizeof(pseudo_header));
memcpy(&pseudo_header.ph.ph_src, &ipv6->saddr.in6_u.u6_addr8, 16);
memcpy(&pseudo_header.ph.ph_dst, &ipv6->daddr.in6_u.u6_addr8, 16);
- pseudo_header.ph.ph_len = ipv6->payload_len;
+ pseudo_header.ph.ph_len = be16_to_cpu(ipv6->payload_len);
pseudo_header.ph.ph_nxt = ipv6->nexthdr;
w = (u16 *)&pseudo_header;
@@ -308,7 +308,7 @@ static int gdm_lte_emulate_ndp(struct sk_buff *skb_in, u32
nic_type)
memcpy(icmp_na + sizeof(struct icmp6hdr), &na,
sizeof(struct neighbour_advertisement));
- icmp6_out.icmp6_cksum = icmp6_checksum(&ipv6_out,
+ icmp6_out.icmp6_cksum = (__force __sum16)
icmp6_checksum(&ipv6_out,
(u16 *)icmp_na, sizeof(icmp_na));
} else {
return -1;
--
1.9.1
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v2] staging: rtl8188eu: remove not necessary braces {} (checkpatch fix)
This is checkpatch fix for hal/bb_cfg.c file:
remove not necessary braces {}
Signed-off-by: Martin Karamihov
---
drivers/staging/rtl8188eu/hal/bb_cfg.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/staging/rtl8188eu/hal/bb_cfg.c
b/drivers/staging/rtl8188eu/hal/bb_cfg.c
index 134fa6c595a8..26e0ef224299 100644
--- a/drivers/staging/rtl8188eu/hal/bb_cfg.c
+++ b/drivers/staging/rtl8188eu/hal/bb_cfg.c
@@ -534,9 +534,8 @@ static void store_pwrindex_offset(struct adapter *adapter,
power_level_offset[11] = data;
if (regaddr == rTxAGC_B_Mcs11_Mcs08)
power_level_offset[12] = data;
- if (regaddr == rTxAGC_B_Mcs15_Mcs12) {
+ if (regaddr == rTxAGC_B_Mcs15_Mcs12)
power_level_offset[13] = data;
- }
}
static void rtl_addr_delay(struct adapter *adapt,
--
2.11.0
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3 20/24] media: imx: Add Camera Interface subdev driver
On Tue, Jan 31, 2017 at 10:21:26AM -0800, Steve Longerbeam wrote: > On 01/31/2017 05:42 AM, Russell King - ARM Linux wrote: > >On Fri, Jan 20, 2017 at 03:38:28PM +0100, Hans Verkuil wrote: > >>Should be set to something like 'platform:imx-media-camif'. v4l2-compliance > >>should complain about this. > >... and more. > > Right, in version 3 that you are working with, no v4l2-compliance fixes were > in yet. A lot of the compliance errors are fixed, please look in latest > branch > imx-media-staging-md-wip at g...@github.com:slongerbeam/mediatree.git. Sorry, I'm not prepared to pull random trees from github as there's no easy way to see what's in the branch. I've always disliked github because its web interface makes it soo difficult to navigate around git trees hosted there. You can see a commit, you can see a diff of the commit. You can get a list of branches. But there seems to be no way to get a list of commits similar to "git log" or even a one-line summary of each commit on a branch. If there is, it's completely non-obvious (which I think is much of the problem with github, it's web interface is horrendous.) Or you can clone/pull the tree without knowing what you're fetching (eg, what the tree is based upon.) Or you can waste time clicking repeatedly on the "parent" commit link on each patch working your way back through the history... Well, it looks like it's bsaed on 4.10-rc1 with who-knows-what work from the linux-media tree (I didn't try and go back any further.) As I don't want to take a whole pile of other changes into my tree, I'm certainly not going to pull from your github tree. Sorry. -- RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up according to speedtest.net. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3 20/24] media: imx: Add Camera Interface subdev driver
On 31/01/17 20:33, Russell King - ARM Linux wrote: On Tue, Jan 31, 2017 at 10:21:26AM -0800, Steve Longerbeam wrote: On 01/31/2017 05:42 AM, Russell King - ARM Linux wrote: On Fri, Jan 20, 2017 at 03:38:28PM +0100, Hans Verkuil wrote: Should be set to something like 'platform:imx-media-camif'. v4l2-compliance should complain about this. ... and more. Right, in version 3 that you are working with, no v4l2-compliance fixes were in yet. A lot of the compliance errors are fixed, please look in latest branch imx-media-staging-md-wip at g...@github.com:slongerbeam/mediatree.git. Sorry, I'm not prepared to pull random trees from github as there's no easy way to see what's in the branch. I've always disliked github because its web interface makes it soo difficult to navigate around git trees hosted there. You can see a commit, you can see a diff of the commit. You can get a list of branches. But there seems to be no way to get a list of commits similar to "git log" or even a one-line summary of each commit on a branch. If there is, it's completely non-obvious (which I think is much of the problem with github, it's web interface is horrendous.) Or you can clone/pull the tree without knowing what you're fetching (eg, what the tree is based upon.) Or you can waste time clicking repeatedly on the "parent" commit link on each patch working your way back through the history... Well, it looks like it's bsaed on 4.10-rc1 with who-knows-what work from the linux-media tree (I didn't try and go back any further.) As I don't want to take a whole pile of other changes into my tree, I'm certainly not going to pull from your github tree. Sorry. https://github.com/slongerbeam/mediatree/compare/master...imx-media-staging-md-wip It's under the "Compare" button from the main view. It would be nice though if the first commit's parent was some clearly tagged start point. Regards, Ian ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3 20/24] media: imx: Add Camera Interface subdev driver
On Tue, Jan 31, 2017 at 09:55:29PM +, Ian Arkver wrote: > On 31/01/17 20:33, Russell King - ARM Linux wrote: > >On Tue, Jan 31, 2017 at 10:21:26AM -0800, Steve Longerbeam wrote: > >>On 01/31/2017 05:42 AM, Russell King - ARM Linux wrote: > >>>On Fri, Jan 20, 2017 at 03:38:28PM +0100, Hans Verkuil wrote: > Should be set to something like 'platform:imx-media-camif'. > v4l2-compliance > should complain about this. > >>>... and more. > >> > >>Right, in version 3 that you are working with, no v4l2-compliance fixes were > >>in yet. A lot of the compliance errors are fixed, please look in latest > >>branch > >>imx-media-staging-md-wip at g...@github.com:slongerbeam/mediatree.git. > > > >Sorry, I'm not prepared to pull random trees from github as there's > >no easy way to see what's in the branch. > > > >I've always disliked github because its web interface makes it soo > >difficult to navigate around git trees hosted there. You can see > >a commit, you can see a diff of the commit. You can get a list of > >branches. But there seems to be no way to get a list of commits > >similar to "git log" or even a one-line summary of each commit on > >a branch. If there is, it's completely non-obvious (which I think is > >much of the problem with github, it's web interface is horrendous.) > > > >Or you can clone/pull the tree without knowing what you're fetching > >(eg, what the tree is based upon.) > > > >Or you can waste time clicking repeatedly on the "parent" commit link > >on each patch working your way back through the history... > > > >Well, it looks like it's bsaed on 4.10-rc1 with who-knows-what work > >from the linux-media tree (I didn't try and go back any further.) > >As I don't want to take a whole pile of other changes into my tree, > >I'm certainly not going to pull from your github tree. Sorry. > > > > https://github.com/slongerbeam/mediatree/compare/master...imx-media-staging-md-wip > > It's under the "Compare" button from the main view. It would be nice though > if the first commit's parent was some clearly tagged start point. I don't want master though, I want v4.10-rc1, and if I ask for that it tells me it knows nothing about v4.10-rc1, despite the fact that's a tag in the mainline kernel repository which was merged into the linux-media tree that this tree is based upon. -- RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up according to speedtest.net. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3 20/24] media: imx: Add Camera Interface subdev driver
On 31/01/17 22:04, Russell King - ARM Linux wrote: On Tue, Jan 31, 2017 at 09:55:29PM +, Ian Arkver wrote: On 31/01/17 20:33, Russell King - ARM Linux wrote: On Tue, Jan 31, 2017 at 10:21:26AM -0800, Steve Longerbeam wrote: On 01/31/2017 05:42 AM, Russell King - ARM Linux wrote: On Fri, Jan 20, 2017 at 03:38:28PM +0100, Hans Verkuil wrote: Should be set to something like 'platform:imx-media-camif'. v4l2-compliance should complain about this. ... and more. Right, in version 3 that you are working with, no v4l2-compliance fixes were in yet. A lot of the compliance errors are fixed, please look in latest branch imx-media-staging-md-wip at g...@github.com:slongerbeam/mediatree.git. Sorry, I'm not prepared to pull random trees from github as there's no easy way to see what's in the branch. I've always disliked github because its web interface makes it soo difficult to navigate around git trees hosted there. You can see a commit, you can see a diff of the commit. You can get a list of branches. But there seems to be no way to get a list of commits similar to "git log" or even a one-line summary of each commit on a branch. If there is, it's completely non-obvious (which I think is much of the problem with github, it's web interface is horrendous.) Or you can clone/pull the tree without knowing what you're fetching (eg, what the tree is based upon.) Or you can waste time clicking repeatedly on the "parent" commit link on each patch working your way back through the history... Well, it looks like it's bsaed on 4.10-rc1 with who-knows-what work >from the linux-media tree (I didn't try and go back any further.) As I don't want to take a whole pile of other changes into my tree, I'm certainly not going to pull from your github tree. Sorry. https://github.com/slongerbeam/mediatree/compare/master...imx-media-staging-md-wip It's under the "Compare" button from the main view. It would be nice though if the first commit's parent was some clearly tagged start point. I don't want master though, I want v4.10-rc1, and if I ask for that it tells me it knows nothing about v4.10-rc1, despite the fact that's a tag in the mainline kernel repository which was merged into the linux-media tree that this tree is based upon. Yeah, that's what I meant about the first parent's commit not being a clearly tagged branch point. At least you get the series on one page. Maybe it's time for a rebase or a v4 series Steve? Personally, I use a bare repo with multiple remotes and fetch branches from various trees. Then gitk --all --since(etc) is pretty good at giving the overview picture. You don't need to pull the commits over into any of your working branches if you don't want to. Regards, Ian ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3 20/24] media: imx: Add Camera Interface subdev driver
On 01/31/2017 02:04 PM, Russell King - ARM Linux wrote: On Tue, Jan 31, 2017 at 09:55:29PM +, Ian Arkver wrote: On 31/01/17 20:33, Russell King - ARM Linux wrote: On Tue, Jan 31, 2017 at 10:21:26AM -0800, Steve Longerbeam wrote: On 01/31/2017 05:42 AM, Russell King - ARM Linux wrote: On Fri, Jan 20, 2017 at 03:38:28PM +0100, Hans Verkuil wrote: Should be set to something like 'platform:imx-media-camif'. v4l2-compliance should complain about this. ... and more. Right, in version 3 that you are working with, no v4l2-compliance fixes were in yet. A lot of the compliance errors are fixed, please look in latest branch imx-media-staging-md-wip at g...@github.com:slongerbeam/mediatree.git. Sorry, I'm not prepared to pull random trees from github as there's no easy way to see what's in the branch. I've always disliked github because its web interface makes it soo difficult to navigate around git trees hosted there. You can see a commit, you can see a diff of the commit. You can get a list of branches. But there seems to be no way to get a list of commits similar to "git log" or even a one-line summary of each commit on a branch. If there is, it's completely non-obvious (which I think is much of the problem with github, it's web interface is horrendous.) Or you can clone/pull the tree without knowing what you're fetching (eg, what the tree is based upon.) Or you can waste time clicking repeatedly on the "parent" commit link on each patch working your way back through the history... Well, it looks like it's bsaed on 4.10-rc1 with who-knows-what work >from the linux-media tree (I didn't try and go back any further.) As I don't want to take a whole pile of other changes into my tree, I'm certainly not going to pull from your github tree. Sorry. https://github.com/slongerbeam/mediatree/compare/master...imx-media-staging-md-wip It's under the "Compare" button from the main view. It would be nice though if the first commit's parent was some clearly tagged start point. I don't want master though, I want v4.10-rc1, and if I ask for that it tells me it knows nothing about v4.10-rc1, despite the fact that's a tag in the mainline kernel repository which was merged into the linux-media tree that this tree is based upon. Hi Russell, yes g...@github.com:slongerbeam/mediatree.git is a fork of the linux-media tree, and the imx-media-staging-md-wip branch is up-to-date with master, currently at 4.10-rc1. You don't need to use the web interface, just git clone the repo. Steve ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2] staging: rtl8188eu: remove not necessary braces {} (checkpatch fix)
On Tue, 2017-01-31 at 21:24 +0200, Martin Karamihov wrote:
> This is checkpatch fix for hal/bb_cfg.c file:
> remove not necessary braces {}
>
> Signed-off-by: Martin Karamihov
> ---
> drivers/staging/rtl8188eu/hal/bb_cfg.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/staging/rtl8188eu/hal/bb_cfg.c
> b/drivers/staging/rtl8188eu/hal/bb_cfg.c
> index 134fa6c595a8..26e0ef224299 100644
> --- a/drivers/staging/rtl8188eu/hal/bb_cfg.c
> +++ b/drivers/staging/rtl8188eu/hal/bb_cfg.c
> @@ -534,9 +534,8 @@ static void store_pwrindex_offset(struct adapter *adapter,
> power_level_offset[11] = data;
> if (regaddr == rTxAGC_B_Mcs11_Mcs08)
> power_level_offset[12] = data;
> - if (regaddr == rTxAGC_B_Mcs15_Mcs12) {
> + if (regaddr == rTxAGC_B_Mcs15_Mcs12)
> power_level_offset[13] = data;
> - }
presumably, this and the regaddr block above it
should be an else if or a switch/case.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3 20/24] media: imx: Add Camera Interface subdev driver
On Tue, Jan 31, 2017 at 02:36:53PM -0800, Steve Longerbeam wrote: > On 01/31/2017 02:04 PM, Russell King - ARM Linux wrote: > >I don't want master though, I want v4.10-rc1, and if I ask for that > >it tells me it knows nothing about v4.10-rc1, despite the fact that's > >a tag in the mainline kernel repository which was merged into the > >linux-media tree that this tree is based upon. > > Hi Russell, yes g...@github.com:slongerbeam/mediatree.git is a fork > of the linux-media tree, and the imx-media-staging-md-wip branch > is up-to-date with master, currently at 4.10-rc1. "up to date" is different from "contains other stuff other than is in 4.10-rc1". What I see in your tree is that your code is based off a merge commit between something called "patchwork" (which I assume is a branch in the media tree containing stuff commited from patch work) and v4.10-rc1. Now, you don't get a commit when merging unless there's changes that aren't in the commit you're merging - if "patchwork" was up to date with v4.10-rc1, then git would have done a "fast forward" to v4.10-rc1. Therefore, while it may be "up to date" with v4.10-rc1 in so far that it's had v4.10-rc1 merged into it, that's not what I've been saying. There are other changes below that merge commit which aren't in v4.10-rc1. It's those other changes that I'm talking about, and it's those other changes I do not want without knowing what they are. It may be that those other changes have since been merged into v4.10-rc6 - but github's web interface can't show me that. In fact, github's web interface is pretty damned useless as far as this stuff goes. So, what I'll get if I clone or pull your imx-media-staging-md-wip branch is, yes, a copy of all your changes, but _also_ all the changes that are in the media tree that _aren't_ in mainline at the point that v4.10-rc1 was merged. > You don't need to use the web interface, just git clone the repo. You're assuming I want to work off the top of your commits. I don't. I've got other dependencies. Then there's yet another problem - lets say that I get a copy of your patches that haven't been on the mailing list, and I then want to make a comment about it. I can't reply to a patch that hasn't been on the mailing list. So, the long established mechanism by which the Linux community does patch review breaks down. So no, sorry, I'm not fetching your tree, and I will persist with your v3 patch set for the time being. -- RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up according to speedtest.net. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3 20/24] media: imx: Add Camera Interface subdev driver
On 01/31/2017 03:30 PM, Russell King - ARM Linux wrote: On Tue, Jan 31, 2017 at 02:36:53PM -0800, Steve Longerbeam wrote: On 01/31/2017 02:04 PM, Russell King - ARM Linux wrote: I don't want master though, I want v4.10-rc1, and if I ask for that it tells me it knows nothing about v4.10-rc1, despite the fact that's a tag in the mainline kernel repository which was merged into the linux-media tree that this tree is based upon. Hi Russell, yes g...@github.com:slongerbeam/mediatree.git is a fork of the linux-media tree, and the imx-media-staging-md-wip branch is up-to-date with master, currently at 4.10-rc1. "up to date" is different from "contains other stuff other than is in 4.10-rc1". What I see in your tree is that your code is based off a merge commit between something called "patchwork" (which I assume is a branch in the media tree containing stuff commited from patch work) and v4.10-rc1. Now, you don't get a commit when merging unless there's changes that aren't in the commit you're merging - if "patchwork" was up to date with v4.10-rc1, then git would have done a "fast forward" to v4.10-rc1. Therefore, while it may be "up to date" with v4.10-rc1 in so far that it's had v4.10-rc1 merged into it, that's not what I've been saying. There are other changes below that merge commit which aren't in v4.10-rc1. It's those other changes that I'm talking about, and it's those other changes I do not want without knowing what they are. It may be that those other changes have since been merged into v4.10-rc6 - but github's web interface can't show me that. In fact, github's web interface is pretty damned useless as far as this stuff goes. So, what I'll get if I clone or pull your imx-media-staging-md-wip branch is, yes, a copy of all your changes, but _also_ all the changes that are in the media tree that _aren't_ in mainline at the point that v4.10-rc1 was merged. You don't need to use the web interface, just git clone the repo. You're assuming I want to work off the top of your commits. I don't. I've got other dependencies. Well, I was suggesting cloning it just to have a look at the new code, but I understand you don't want to attempt to bring in your SMIA/bayer format changes and run from this branch, due to the other changes in the mediatree. I suppose I should post the next version then. Trouble is, I see issues in the current driver that prevents working with your SMIA pipeline. But I guess that will have to be worked out in another version. Steve Then there's yet another problem - lets say that I get a copy of your patches that haven't been on the mailing list, and I then want to make a comment about it. I can't reply to a patch that hasn't been on the mailing list. So, the long established mechanism by which the Linux community does patch review breaks down. So no, sorry, I'm not fetching your tree, and I will persist with your v3 patch set for the time being. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3 00/24] i.MX Media Driver
On 01/31/2017 03:00 AM, Russell King - ARM Linux wrote: On Mon, Jan 30, 2017 at 05:22:01PM -0800, Steve Longerbeam wrote: I'm also having trouble finding a datasheet for it, but from what I've read, it has a MIPI CSI-2 interface. It should work fine as long as it presents a single source pad, registers asynchronously, and sets its entity function to MEDIA_ENT_F_CAM_SENSOR. Yes, it is MIPI CSI-2, and yes it has a single source pad, registers asynchronously, but that's about as far as it goes. The structure is a camera sensor followed by some processing. So just like the smiapp code, I've ended up with multiple subdevs describing each stage of the sensors pipeline. Just like smiapp, the camera sensor block (which is the very far end of the pipeline) is marked with MEDIA_ENT_F_CAM_SENSOR. However, in front of that is the binner, which just like smiapp gets a separate entity. It's this entity which is connected to the mipi-csi2 subdev. wow, ok got it. So the sensor pipeline and binner, and the OF graph connecting them, are described in the device tree I presume. The OF graph AFAIK, has no information about which ports are sinks and which are sources, so of_parse_subdev() tries to determine that based on the compatible string of the device node. So ATM of_parse_subdev() assumes there is nothing but the imx6-mipi-csi2, video-multiplexer, and camera sensors upstream from the CSI ports in the OF graph. I realize that's not a robust solution, and is the reason for the "no sensor attached" below. Is there any way to determine from the OF graph the data-direction of a port (whether it is a sink or a source)? If so it will make of_parse_subdev() much more robust. Steve Unlike smiapp, which does not set an entity function, I set my binner entity as MEDIA_ENT_F_PROC_VIDEO_SCALER on the basis that that is what V4L2 documentation recommend: - .. row 27 .. _MEDIA-ENT-F-PROC-VIDEO-SCALER: - ``MEDIA_ENT_F_PROC_VIDEO_SCALER`` - Video scaler. An entity capable of video scaling must have at least one sink pad and one source pad, and scale the video frame(s) received on its sink pad(s) to a different resolution output on its source pad(s). The range of supported scaling ratios is entity-specific and can differ between the horizontal and vertical directions (in particular scaling can be supported in one direction only). Binning and skipping are considered as scaling. This causes attempts to configure the ipu1_csi0 interface to fail: media-ctl -v -d /dev/media1 --set-v4l2 '"ipu1_csi0":1[fmt:SGBRG8/512x512@1/30]' Opening media device /dev/media1 Enumerating entities Found 29 entities Enumerating pads and links Setting up format SGBRG8 512x512 on pad ipu1_csi0/1 Unable to set format: No such device (-19) Unable to setup formats: No such device (19) and in the kernel log: ipu1_csi0: no sensor attached And yes, I already know that my next problem is going to be that the bayer formats are not supported in your driver (just like Philipp's driver) but adding them should not be difficult... but only once this issue is resolved. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3 00/24] i.MX Media Driver
On Tue, Jan 31, 2017 at 03:43:22PM -0800, Steve Longerbeam wrote: > > > On 01/31/2017 03:00 AM, Russell King - ARM Linux wrote: > >Just like smiapp, the camera sensor block (which is the very far end > >of the pipeline) is marked with MEDIA_ENT_F_CAM_SENSOR. However, in > >front of that is the binner, which just like smiapp gets a separate > >entity. It's this entity which is connected to the mipi-csi2 subdev. > > wow, ok got it. > > So the sensor pipeline and binner, and the OF graph connecting > them, are described in the device tree I presume. No - because the binner and sensor are on the same die, it's even one single device, there's no real separation of the two devices. The reason there's no real separation is because the binning is done as part of the process of reading the array - sometimes before the analog voltage is converted to its digital pixel value representation. The separation comes because of the requirements of the v4l2 media subdevs, which requires scalers to have a sink pad and a source pad. (Please see the v4l2 documentation, I think I've already quoted this: .. _MEDIA-ENT-F-PROC-VIDEO-SCALER: - ``MEDIA_ENT_F_PROC_VIDEO_SCALER`` - Video scaler. An entity capable of video scaling must have at least one sink pad and one source pad, and scale the video frame(s) received on its sink pad(s) to a different resolution output on its source pad(s). The range of supported scaling ratios is entity-specific and can differ between the horizontal and vertical directions (in particular scaling can be supported in one direction only). Binning and skipping are considered as scaling. (Oh yes, I see it was the mail to which you were replying to...) So, in order to configure the scaling (which can be none, /2 and /4) we have to expose two subdevs - one which is the scaler, and has a source pad connected to the upstream (in this case CSI2 receiver) and a sink pad immutably connected to the camera sensor. Since the split is entirely down to the V4L2 implementation requirements, it's not something that should be reflected in DT - we don't put implementation details in DT. It's just the same (as I've already said) as the SMIAPP camera driver, the reason I'm pointing you towards that is because this is an already mainlined camera driver which nicely illustrates how my driver is structured from the v4l2 subdev API point of view. > The OF graph AFAIK, has no information about which ports are sinks > and which are sources, so of_parse_subdev() tries to determine that > based on the compatible string of the device node. So ATM > of_parse_subdev() assumes there is nothing but the imx6-mipi-csi2, > video-multiplexer, and camera sensors upstream from the CSI ports > in the OF graph. > > I realize that's not a robust solution, and is the reason for the > "no sensor attached" below. > > Is there any way to determine from the OF graph the data-direction > of a port (whether it is a sink or a source)? If so it will make > of_parse_subdev() much more robust. I'm not sure why you need to know the data direction. I think the issue here is how you're going about dealing with the subdevs. Here's the subdev setup: +-camera+ | pixel array -> binner |---> csi2 --> ipu1csi0 mux --> csi0 --> smfc --> idmac +---+ How the subdevs are supposed to work is that you start from the first subdev in sequence (in this case the pixel array) and negotiate with the driver through the TRY formats on its source pad, as well as negotiating with the sink pad of the directly neighbouring subdev. The neighbouring subdev propagates the configuration in a driver specific manner from its source pad to the sink pad, giving a default configuration at its source. This process repeats throughout the chain all the way up to the bridge device. Now, where things go wrong is that you want to know what each type of these subdevs are, and the reason you want that is so you can do this (for example - I know similar stuff happens with the "sensor" stuff further up the chain as well): +-camera+ | pixel array -> binner |---> csi2 --> ipu1csi0 mux --> csi0 --> smfc --> idmac +---+| ^--I-want-your-bus-format-and-frame-rate---' which goes against the negotiation mechanism of v4l2 subdevs. This is broken - it bypass the subdev negotiation that has been performed on the intervening subdevs between the "sensor" and the csi0 subdevs, so if there were a subdev in that chain that (eg) reduced the frame rate by discarding the odd frames, you'd be working with the wrong frame interval for your frame interval monitor at csi. Note that the "MEDIA_ENT_F_PROC_VIDEO_SCALER" subdev type is documented as not only supports scaling as in changing the size of the image, but also in terms of skipping frames, which means a reduction in frame rate. S
Re: [PATCH v3 17/24] media: imx: Add CSI subdev driver
On 01/31/2017 03:20 AM, Russell King - ARM Linux wrote:
On Fri, Jan 06, 2017 at 06:11:35PM -0800, Steve Longerbeam wrote:
+static int csi_link_validate(struct v4l2_subdev *sd,
+struct media_link *link,
+struct v4l2_subdev_format *source_fmt,
+struct v4l2_subdev_format *sink_fmt)
+{
+ struct csi_priv *priv = v4l2_get_subdevdata(sd);
+ bool is_csi2;
+ int ret;
+
+ ret = v4l2_subdev_link_validate_default(sd, link, source_fmt, sink_fmt);
+ if (ret)
+ return ret;
+
+ priv->sensor = __imx_media_find_sensor(priv->md, &priv->sd.entity);
+ if (IS_ERR(priv->sensor)) {
+ v4l2_err(&priv->sd, "no sensor attached\n");
+ ret = PTR_ERR(priv->sensor);
+ priv->sensor = NULL;
+ return ret;
+ }
+
+ ret = v4l2_subdev_call(priv->sensor->sd, video, g_mbus_config,
+ &priv->sensor_mbus_cfg);
+ if (ret)
+ return ret;
+
+ is_csi2 = (priv->sensor_mbus_cfg.type == V4L2_MBUS_CSI2);
+
+ if (is_csi2) {
+ int vc_num = 0;
+ /*
+* NOTE! It seems the virtual channels from the mipi csi-2
+* receiver are used only for routing by the video mux's,
+* or for hard-wired routing to the CSI's. Once the stream
+* enters the CSI's however, they are treated internally
+* in the IPU as virtual channel 0.
+*/
+#if 0
+ vc_num = imx_media_find_mipi_csi2_channel(priv->md,
+ &priv->sd.entity);
+ if (vc_num < 0)
+ return vc_num;
+#endif
+ ipu_csi_set_mipi_datatype(priv->csi, vc_num,
+ &priv->format_mbus[priv->input_pad]);
This seems (at least to me) to go against the spirit of the subdev
negotiation. Here, you seem to bypass the negotiation performed
between the CSI and the attached device, wanting to grab the
format from the CSI2 sensor directly. That bypasses negotiation
performed at the CSI2 subdev and video muxes.
This isn't so much grabbing a pad format, it is determining
which source pad at the imx6-mipi-csi2 receiver subdev is
reached from this CSI (which determines the virtual channel
the CSI is receiving).
If there was a way to determine the vc# via a status register
in the CSI, that would be perfect, but there isn't. In fact, the CSIs
seem to be ignoring the meta-data bus sent by the CSI2IPU gasket
which contains this info, or that bus is not being routed to the CSIs.
As the note says, the CSIs only accept vc0 at the CSI_MIPI_DI register.
Any other value programmed there results in no data from the CSI.
And even the presence of CSI_MIPI_DI register makes no sense to me,
the CSIs are given the vc# and MIPI datatype from the CSI2IPU meta-data
bus, so I don't understand why it needs to be told.
Anyway I can just yank this disabled code, it's only there for documentation
purposes.
The same goes for the frame rate monitoring code - imho, the frame
rate should be something that results from negotiation with the
neighbouring element, not by poking at some entity that is several
entities away.
I will fix this once the g_frame_interval op is implemented in every
subdev. I believe you mentioned this already, but g_frame_interval
will need to be chained until it reaches the originating sensor.
Steve
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3 17/24] media: imx: Add CSI subdev driver
On Tue, Jan 31, 2017 at 04:31:55PM -0800, Steve Longerbeam wrote:
>
>
> On 01/31/2017 03:20 AM, Russell King - ARM Linux wrote:
> >On Fri, Jan 06, 2017 at 06:11:35PM -0800, Steve Longerbeam wrote:
> >>+static int csi_link_validate(struct v4l2_subdev *sd,
> >>+struct media_link *link,
> >>+struct v4l2_subdev_format *source_fmt,
> >>+struct v4l2_subdev_format *sink_fmt)
> >>+{
> >>+ struct csi_priv *priv = v4l2_get_subdevdata(sd);
> >>+ bool is_csi2;
> >>+ int ret;
> >>+
> >>+ ret = v4l2_subdev_link_validate_default(sd, link, source_fmt, sink_fmt);
> >>+ if (ret)
> >>+ return ret;
> >>+
> >>+ priv->sensor = __imx_media_find_sensor(priv->md, &priv->sd.entity);
> >>+ if (IS_ERR(priv->sensor)) {
> >>+ v4l2_err(&priv->sd, "no sensor attached\n");
> >>+ ret = PTR_ERR(priv->sensor);
> >>+ priv->sensor = NULL;
> >>+ return ret;
> >>+ }
> >>+
> >>+ ret = v4l2_subdev_call(priv->sensor->sd, video, g_mbus_config,
> >>+ &priv->sensor_mbus_cfg);
> >>+ if (ret)
> >>+ return ret;
> >>+
> >>+ is_csi2 = (priv->sensor_mbus_cfg.type == V4L2_MBUS_CSI2);
> >>+
> >>+ if (is_csi2) {
> >>+ int vc_num = 0;
> >>+ /*
> >>+* NOTE! It seems the virtual channels from the mipi csi-2
> >>+* receiver are used only for routing by the video mux's,
> >>+* or for hard-wired routing to the CSI's. Once the stream
> >>+* enters the CSI's however, they are treated internally
> >>+* in the IPU as virtual channel 0.
> >>+*/
> >>+#if 0
> >>+ vc_num = imx_media_find_mipi_csi2_channel(priv->md,
> >>+ &priv->sd.entity);
> >>+ if (vc_num < 0)
> >>+ return vc_num;
> >>+#endif
> >>+ ipu_csi_set_mipi_datatype(priv->csi, vc_num,
> >>+ &priv->format_mbus[priv->input_pad]);
> >This seems (at least to me) to go against the spirit of the subdev
> >negotiation. Here, you seem to bypass the negotiation performed
> >between the CSI and the attached device, wanting to grab the
> >format from the CSI2 sensor directly. That bypasses negotiation
> >performed at the CSI2 subdev and video muxes.
>
> This isn't so much grabbing a pad format, it is determining
> which source pad at the imx6-mipi-csi2 receiver subdev is
> reached from this CSI (which determines the virtual channel
> the CSI is receiving).
>
> If there was a way to determine the vc# via a status register
> in the CSI, that would be perfect, but there isn't. In fact, the CSIs
> seem to be ignoring the meta-data bus sent by the CSI2IPU gasket
> which contains this info, or that bus is not being routed to the CSIs.
> As the note says, the CSIs only accept vc0 at the CSI_MIPI_DI register.
> Any other value programmed there results in no data from the CSI.
>
> And even the presence of CSI_MIPI_DI register makes no sense to me,
> the CSIs are given the vc# and MIPI datatype from the CSI2IPU meta-data
> bus, so I don't understand why it needs to be told.
The CSI_MIPI_DI register selects the data stream we want from the
CSI2 camera.
CSI2 cameras can produce many different data streams - for example,
a CSI2 camera can, for the same image, produce a YUV encoded frame
and a jpeg-encoded frame. These are sent over the CSI2 serial bus
using two different data types.
The CSI2IPU converts the serial data streams into a parallel data
stream, feeding that into the CSI layer. The CSI layer, in
conjunction with the CSI_MIPI_DI register, selects one of these
streams to pass on to the SMFC and other blocks.
>From what I've read, the CSI can also be programmed to pass other
streams on as well, but I've never tried that.
In my particular case, the IMX219 camera produces a complete frame
using the RAW8 or RAW10 MIPI data type, and also produces two lines
of register data per frame, encoded using another data type. I
think it should be possible to program the CSI to pass this other
data on as "generic data" through a different FIFO and have it
written to memory, which makes it possible to know the camera
settings for each frame. (This isn't something I'm interested in
though, I'm just using it as an example of why, possibly, there's
that register in the CSI block.)
--
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3 21/24] media: imx: Add MIPI CSI-2 Receiver subdev driver
On 01/31/2017 01:49 AM, Philipp Zabel wrote: On Tue, 2017-01-31 at 00:01 +, Russell King - ARM Linux wrote: [...] The iMX6 manuals call for a very specific seven sequence of initialisation for CSI2, which begins with: 1. reset the D-PHY. 2. place MIPI sensor in LP-11 state 3. perform D-PHY initialisation 4. configure CSI2 lanes and de-assert resets and shutdown signals Since you reset the CSI2 at power up and then release it, how do you guarantee that the published sequence is followed? Hi Russell, In "40.3.1 Startup Sequence", it states that step 1 is to "De-assert CSI2 presetn signal (global reset)". I can't find any description of this signal in the manual, that statement is the only mention of it. I don't know if this the D-PHY reset signal, it sounds more like CSI2_RESETN (CSI-2 host controller reset). In any case, I re-reviewed the published sequence in the manual, and it does look like there are a couple problems. The pipeline power up sequence in imx-media driver is as follows: s_power(ON) op is called first on the imx6-mipi-csi2, in which CSI2 and D-PHY resets are asserted and then de-asserted via the CSI2_RESETN and CSI2_DPHY_RSTZ registers, the D-PHY is initialized, and lanes set. At this point the MIPI sensor may be powered down (in fact, in OV5640 case, the PWDN pin is asserted). So there could be a problem here, I don't think the D-PHY is considered in the LP-11 stop state when the D-PHY master is powered off :). A fix might simply be to reverse power on, sensor first so that it can be placed in LP-11, then imx6-mipi-csi2. The following steps are carried out by s_stream() calls. Sensor s_stream(ON) is called first which starts a clock on the clock lane. Then imx6-mipi-csi2 s_stream(ON) in which the PHY_STATE register is polled to confirm the D-PHY stop state, then looks for active clock on lock lane. There could be a problem there too. Again should be fixed simply by calling stream-on on the imx6-mipi-csi2 first, then sensor. So I will try the following sequence: 1. sensor power on (put D-PHY in LP-11 stop state). 2. csi-2 power on (deassert CSI2 and D-PHY resets, D-PHY init, verify LP-11). 3. sensor stream on (starts clock on clock lane). 4. csi-2 stream on (confirm clock on clock lane). That comes closest to meeting the sequence requirements. But this also puts a requirement on MIPI sensors that s_power(ON) should only place the D_PHY in LP-11, and _not_ start the clock lane. But perhaps that is correct behavior anyway. Steve With Philipp's driver, this is easy, because there is a prepare_stream callback which gives the sensor an opportunity to get everything correctly configured according to the negotiated parameters, and place the sensor in LP-11 state. Some sensors do not power up in LP-11 state, but need to be programmed fully before being asked to momentarily stream. Only at that point is the sensor guaranteed to be in the required LP-11 state. Do you expect that 1. and 2. could depend on the negotiated parameters in any way on some hardware? I had removed the prepare_stream callback from my driver in v2 because for my use case at least the above sequence could be realized by 1. in imx-mipi-csi2 s_power(1) 2. in MIPI sensor s_power(1) 3./4. in imx-mipi-csi2 s_stream(1) 4. in MIPI sensor s_stream(1) as long as the sensor is correctly put back into LP-11 in s_stream(0). regards Philipp ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3 21/24] media: imx: Add MIPI CSI-2 Receiver subdev driver
Hi Steve, On Tue, Jan 31, 2017 at 05:02:40PM -0800, Steve Longerbeam wrote: > But this also puts a requirement on MIPI sensors that s_power(ON) > should only place the D_PHY in LP-11, and _not_ start the clock lane. > But perhaps that is correct behavior anyway. If the CSI2 DPHY is held in reset state, it shouldn't matter what the sensor does. In the case of IMX219, it needs a full setup of the device, including enabling it to stream (so it starts the clock lane etc) in order to get it into LP-11 state. Merely disabling the XCLR signal leaves the lanes grounded. I do seem to remember reading in one of the MIPI specs that this is rather expected behaviour, though I can't point at a paragraph this late in the night. So, the only way to satisfy these requirements is this order: - assert PHY reset signals (so blocking any activity on the CSI lanes) - initialise the sensor (including allowing it to start streaming and then stopping the stream - at that point, the lanes will be in LP-11.) - deassert the resets as per the iMX6 documentation and follow the remaining procedure. I'll look at your other points tomorrow. Thanks. -- RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up according to speedtest.net. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3 00/24] i.MX Media Driver
On 01/31/2017 05:54 AM, Philipp Zabel wrote: Hi Steve, I have just tested the imx-media-staging-md-wip branch on a Nitrogen6X with a tc358743 (BD_HDMI_MIPI HDMI to MIPI CSI-2 receiver board). Some observations: # Link pipeline media-ctl -l "'tc358743 1-000f':0->'imx6-mipi-csi2':0[1]" media-ctl -l "'imx6-mipi-csi2':1->'ipu1_csi0_mux':0[1]" media-ctl -l "'ipu1_csi0_mux':2->'ipu1_csi0':0[1]" media-ctl -l "'ipu1_csi0':2->'ipu1_csi0 capture':0[1]" # Provide an EDID to the HDMI source v4l2-ctl -d /dev/v4l-subdev2 --set-edid=file=edid-1080p.hex # At this point the HDMI source is enabled and sends a 1080p60 signal # Configure detected DV timings media-ctl --set-dv "'tc358743 1-000f':0" # Set pad formats media-ctl --set-v4l2 "'tc358743 1-000f':0[fmt:UYVY/1920x1080]" media-ctl --set-v4l2 "'imx6-mipi-csi2':1[fmt:UYVY2X8/1920x1080]" media-ctl --set-v4l2 "'ipu1_csi0_mux':2[fmt:UYVY2X8/1920x1080]" media-ctl --set-v4l2 "'ipu1_csi0':2[fmt:AYUV32/1920x1080]" v4l2-ctl -d /dev/video4 -V # This still is configured to 640x480, which is inconsistent with # the 'ipu1_csi0':2 pad format. The pad set_fmt above should # have set this, too. Because you've only configured the source pads, and not the sink pads. The ipu_csi source format is dependent on the sink format - output crop window is limited by max input sensor frame, and since sink pad is still at 640x480, output is reduced to that. Maybe I'm missing something, is it expected behavior that a source format should be automatically propagated to the sink? v4l2-ctl --list-formats -d /dev/video4 # This lists all the RGB formats, which it shouldn't. There is # no CSC in this pipeline, so we should be limited to YUV formats # only. right, need to fix that. Probably by poking the attached source subdev (csi or prpenc/vf) for its supported formats. # Set capture format v4l2-ctl -d /dev/video4 -v width=1920,height=1080,pixelformat=UYVY v4l2-ctl -d /dev/video4 -V # Now the capture format is correctly configured to 1920x1080. v4l2-ctl -d 4 --list-frameintervals=width=1920,height=1080,pixelformat=UYVY # This lists nothing. We should at least provide the 'ipu1_csi0':2 pad # frame interval. In the future this should list fractions achievable # via frame skipping. yes, need to implement g_frame_interval. v4l2-compliance -d /dev/video4 # This fails two tests: # fail: v4l2-test-input-output.cpp(383): std == 0 # fail: v4l2-test-input-output.cpp(449): invalid attributes for input 0 # test VIDIOC_G/S/ENUMINPUT: FAIL # and # fail: v4l2-test-controls.cpp(782): subscribe event for control 'User Controls' failed # test VIDIOC_(UN)SUBSCRIBE_EVENT/DQEVENT: FAIL # (Slowly) stream JPEG images to a display host: gst-launch-1.0 -v v4l2src device=/dev/video4 ! jpegenc ! rtpjpegpay ! udpsink I've done this a few times, and sometimes I only get a "ipu1_csi0: EOF timeout" message when starting streaming. It's hard to say what is going on there, it would be great if I could get my hands on a Nitrogen6X with the tc35874 to help you debug. Steve ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3 17/24] media: imx: Add CSI subdev driver
On 01/31/2017 08:48 AM, Philipp Zabel wrote:
On Tue, 2017-01-31 at 16:51 +0100, Philipp Zabel wrote:
On Fri, 2017-01-06 at 18:11 -0800, Steve Longerbeam wrote:
[...]
+static int csi_set_fmt(struct v4l2_subdev *sd,
+ struct v4l2_subdev_pad_config *cfg,
+ struct v4l2_subdev_format *sdformat)
+{
+ struct csi_priv *priv = v4l2_get_subdevdata(sd);
+ struct v4l2_mbus_framefmt *infmt, *outfmt;
+ struct v4l2_rect crop;
+ int ret;
+
+ if (sdformat->pad >= CSI_NUM_PADS)
+ return -EINVAL;
+
+ if (priv->stream_on)
+ return -EBUSY;
+
+ infmt = &priv->format_mbus[priv->input_pad];
+ outfmt = &priv->format_mbus[priv->output_pad];
+
+ if (sdformat->pad == priv->output_pad) {
+ sdformat->format.code = infmt->code;
+ sdformat->format.field = infmt->field;
+ crop.left = priv->crop.left;
+ crop.top = priv->crop.top;
+ crop.width = sdformat->format.width;
+ crop.height = sdformat->format.height;
+ ret = csi_try_crop(priv, &crop);
This is the wrong way around, see also below.
Here the the output sdformat->format.width/height should be set to the
priv->crop.width/height (or priv->crop.width/height / 2, to enable
downscaling). The crop rectangle should not be changed by an output pad
set_fmt.
[...]
The crop rectangle instead should be reset to the full input frame when
the input pad format is set.
How about this:
Thanks for the patch, I'll try it tomorrow.
Steve
--8<--
diff --git a/drivers/staging/media/imx/imx-media-csi.c
b/drivers/staging/media/imx/imx-media-csi.c
index 5e80a0871b139..8220e4204a125 100644
--- a/drivers/staging/media/imx/imx-media-csi.c
+++ b/drivers/staging/media/imx/imx-media-csi.c
@@ -484,6 +484,8 @@ static int csi_setup(struct csi_priv *priv)
if_fmt.field = outfmt->field;
ipu_csi_set_window(priv->csi, &priv->crop);
+ ipu_csi_set_downsize(priv->csi, priv->crop.width == 2 * outfmt->width,
+ priv->crop.height == 2 *
outfmt->height);
ipu_csi_init_interface(priv->csi, &sensor_mbus_cfg, &if_fmt);
@@ -830,15 +832,14 @@ static int csi_set_fmt(struct v4l2_subdev *sd,
switch (sdformat->pad) {
case CSI_SRC_PAD_DIRECT:
case CSI_SRC_PAD_IDMAC:
- crop.left = priv->crop.left;
- crop.top = priv->crop.top;
- crop.width = sdformat->format.width;
- crop.height = sdformat->format.height;
- ret = csi_try_crop(priv, &crop, sensor);
- if (ret)
- return ret;
- sdformat->format.width = crop.width;
- sdformat->format.height = crop.height;
+ if (sdformat->format.width < priv->crop.width * 3 / 4)
+ sdformat->format.width = priv->crop.width / 2;
+ else
+ sdformat->format.width = priv->crop.width;
+ if (sdformat->format.height < priv->crop.height * 3 / 4)
+ sdformat->format.height = priv->crop.height / 2;
+ else
+ sdformat->format.height = priv->crop.height;
if (sdformat->pad == CSI_SRC_PAD_IDMAC) {
cc = imx_media_find_format(0, sdformat->format.code,
@@ -887,6 +888,14 @@ static int csi_set_fmt(struct v4l2_subdev *sd,
}
break;
case CSI_SINK_PAD:
+ crop.left = 0;
+ crop.top = 0;
+ crop.width = sdformat->format.width;
+ crop.height = sdformat->format.height;
+ ret = csi_try_crop(priv, &crop, sensor);
+ if (ret)
+ return ret;
+
cc = imx_media_find_format(0, sdformat->format.code,
true, false);
if (!cc) {
@@ -904,9 +913,8 @@ static int csi_set_fmt(struct v4l2_subdev *sd,
} else {
priv->format_mbus[sdformat->pad] = sdformat->format;
priv->cc[sdformat->pad] = cc;
- /* Update the crop window if this is an output pad */
- if (sdformat->pad == CSI_SRC_PAD_DIRECT ||
- sdformat->pad == CSI_SRC_PAD_IDMAC)
+ /* Reset the crop window if this is the input pad */
+ if (sdformat->pad == CSI_SINK_PAD)
priv->crop = crop;
}
-->8--
regards
Philipp
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3 00/24] i.MX Media Driver
On 01/31/2017 04:23 PM, Russell King - ARM Linux wrote: On Tue, Jan 31, 2017 at 03:43:22PM -0800, Steve Longerbeam wrote: On 01/31/2017 03:00 AM, Russell King - ARM Linux wrote: Just like smiapp, the camera sensor block (which is the very far end of the pipeline) is marked with MEDIA_ENT_F_CAM_SENSOR. However, in front of that is the binner, which just like smiapp gets a separate entity. It's this entity which is connected to the mipi-csi2 subdev. wow, ok got it. So the sensor pipeline and binner, and the OF graph connecting them, are described in the device tree I presume. No - because the binner and sensor are on the same die, it's even one single device, there's no real separation of the two devices. The reason there's no real separation is because the binning is done as part of the process of reading the array - sometimes before the analog voltage is converted to its digital pixel value representation. The separation comes because of the requirements of the v4l2 media subdevs, which requires scalers to have a sink pad and a source pad. (Please see the v4l2 documentation, I think I've already quoted this: .. _MEDIA-ENT-F-PROC-VIDEO-SCALER: - ``MEDIA_ENT_F_PROC_VIDEO_SCALER`` - Video scaler. An entity capable of video scaling must have at least one sink pad and one source pad, and scale the video frame(s) received on its sink pad(s) to a different resolution output on its source pad(s). The range of supported scaling ratios is entity-specific and can differ between the horizontal and vertical directions (in particular scaling can be supported in one direction only). Binning and skipping are considered as scaling. (Oh yes, I see it was the mail to which you were replying to...) So, in order to configure the scaling (which can be none, /2 and /4) we have to expose two subdevs - one which is the scaler, and has a source pad connected to the upstream (in this case CSI2 receiver) and a sink pad immutably connected to the camera sensor. Since the split is entirely down to the V4L2 implementation requirements, it's not something that should be reflected in DT - we don't put implementation details in DT. It's just the same (as I've already said) as the SMIAPP camera driver, the reason I'm pointing you towards that is because this is an already mainlined camera driver which nicely illustrates how my driver is structured from the v4l2 subdev API point of view. The OF graph AFAIK, has no information about which ports are sinks and which are sources, so of_parse_subdev() tries to determine that based on the compatible string of the device node. So ATM of_parse_subdev() assumes there is nothing but the imx6-mipi-csi2, video-multiplexer, and camera sensors upstream from the CSI ports in the OF graph. I realize that's not a robust solution, and is the reason for the "no sensor attached" below. Is there any way to determine from the OF graph the data-direction of a port (whether it is a sink or a source)? If so it will make of_parse_subdev() much more robust. I'm not sure why you need to know the data direction. First, thank you for the explanation, it clears up a lot. But of_parse_subdev() is used to parse the OF graph starting from the CSI ports, to discover all the nodes to add to subdev async registration. It also forms the media link info to be used later to create the media graph, after all discovered subdevs have come online (registered themselves). This happens at driver load time, it doesn't have anything to do with pad negotiation. I think the issue here is how you're going about dealing with the subdevs. Here's the subdev setup: +-camera+ | pixel array -> binner |---> csi2 --> ipu1csi0 mux --> csi0 --> smfc --> idmac +---+ How the subdevs are supposed to work is that you start from the first subdev in sequence (in this case the pixel array) and negotiate with the driver through the TRY formats on its source pad, as well as negotiating with the sink pad of the directly neighbouring subdev. The neighbouring subdev propagates the configuration in a driver specific manner from its source pad to the sink pad, giving a default configuration at its source. Let me try to re-phrase. You mean the subdev's set_fmt(), when configured its source pad(s), should call set_fmt() at the connected sink subdev to automatically propagate the format to the sink's pad? This process repeats throughout the chain all the way up to the bridge device. Now, where things go wrong is that you want to know what each type of these subdevs are, and the reason you want that is so you can do this (for example - I know similar stuff happens with the "sensor" stuff further up the chain as well): +-camera+ | pixel array -> binner |---> csi2 --> ipu1csi0 mux --> csi0 --> smfc --> idmac +---+|
Re: [PATCH 6/6] staging: bcm2835-v4l2: Apply spelling fixes from checkpatch.
On Tue, 2017-01-31 at 10:30 -0800, Eric Anholt wrote:
> Joe Perches writes:
>
> > On Mon, 2017-01-30 at 12:05 -0800, Eric Anholt wrote:
> > > Joe Perches writes:
> > >
> > > > On Fri, 2017-01-27 at 13:55 -0800, Eric Anholt wrote:
> > > > > Generated with checkpatch.pl --fix-inplace and git add -p out of the
> > > > > results.
> > > >
> > > > Maybe another.
> > > >
> > > > > diff --git a/drivers/staging/media/platform/bcm2835/mmal-vchiq.c
> > > > > b/drivers/staging/media/platform/bcm2835/mmal-vchiq.c
> > > >
> > > > []
> > > > > @@ -239,7 +239,7 @@ static int bulk_receive(struct
> > > > > vchiq_mmal_instance *instance,
> > > > > pr_err("buffer list empty trying to submit bulk
> > > > > receive\n");
> > > > >
> > > > > /* todo: this is a serious error, we should never have
> > > > > - * commited a buffer_to_host operation to the mmal
> > > > > + * committed a buffer_to_host operation to the mmal
> > > > >* port without the buffer to back it up (underflow
> > > > >* handling) and there is no obvious way to deal with
> > > > >* this - how is the mmal servie going to react when
> > > >
> > > > Perhaps s/servie/service/ ?
> > >
> > > I was trying to restrict this patch to just the fixes from checkpatch.
> >
> > That's the wrong thing to do if you're fixing
> > spelling defects. checkpatch is just one mechanism
> > to identify some, and definitely not all, typos and
> > spelling defects.
> >
> > If you fixing, fix. Don't just rely on the brainless
> > tools, use your decidedly non-mechanical brain.
>
> "if you touch anything, you must fix everything." If that's how things
> work, I would just retract the patch.
I didn't say that,and I don't mean that.
If you notice a similar defect when you are fixing
any arbitrary defect, please try to fix all of similar
defects.
As is, a patch that fixes just servie would cause a
patch conflict with your patch.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Staging: ks7010: clean up ks_hostif.h
From 58defc62d53cb473f9d745127d98df834a10b321 Mon Sep 17 00:00:00 2001
From: Logan Gorence
Date: Tue, 31 Jan 2017 21:51:05 -0800
Subject: [PATCH] Staging: ks7010: clean up ks_hostif.h
Clean up the errors in ks_hostif.h put out by checkpatch.pl.
Signed-off by: Logan Gorence
---
drivers/staging/ks7010/ks_hostif.h | 56 +++---
1 file changed, 28 insertions(+), 28 deletions(-)
diff --git a/drivers/staging/ks7010/ks_hostif.h
b/drivers/staging/ks7010/ks_hostif.h
index 743f31ead56e..ccc2a7742e80 100644
--- a/drivers/staging/ks7010/ks_hostif.h
+++ b/drivers/staging/ks7010/ks_hostif.h
@@ -1,6 +1,6 @@
/*
* Driver for KeyStream wireless LAN
- *
+ *
* Copyright (c) 2005-2008 KeyStream Corp.
* Copyright (C) 2009 Renesas Technology Corp.
*
@@ -360,7 +360,7 @@ struct hostif_ps_adhoc_set_request_t {
#define CTS_MODE_TRUE 1
uint16_t channel;
struct rate_set16_t rate_set;
- uint16_t capability;/* bit5:preamble bit6:pbcc pbcc
not supported always 0
+ uint16_t capability;/* bit5:preamble bit6:pbcc pbcc
not supported always 0
* bit10:ShortSlotTime bit13:DSSS-OFDM
DSSS-OFDM not supported always 0 */
uint16_t scan_type;
} __packed;
@@ -376,7 +376,7 @@ struct hostif_infrastructure_set_request_t {
uint16_t cts_mode;
struct rate_set16_t rate_set;
struct ssid_t ssid;
- uint16_t capability;/* bit5:preamble bit6:pbcc pbcc
not supported always 0
+ uint16_t capability;/* bit5:preamble bit6:pbcc pbcc
not supported always 0
* bit10:ShortSlotTime bit13:DSSS-OFDM
DSSS-OFDM not supported always 0 */
uint16_t beacon_lost_count;
uint16_t auth_type;
@@ -392,7 +392,7 @@ struct hostif_infrastructure_set2_request_t {
uint16_t cts_mode;
struct rate_set16_t rate_set;
struct ssid_t ssid;
- uint16_t capability;/* bit5:preamble bit6:pbcc pbcc
not supported always 0
+ uint16_t capability;/* bit5:preamble bit6:pbcc pbcc
not supported always 0
* bit10:ShortSlotTime bit13:DSSS-OFDM
DSSS-OFDM not supported always 0 */
uint16_t beacon_lost_count;
uint16_t auth_type;
@@ -415,7 +415,7 @@ struct hostif_adhoc_set_request_t {
uint16_t channel;
struct rate_set16_t rate_set;
struct ssid_t ssid;
- uint16_t capability;/* bit5:preamble bit6:pbcc pbcc
not supported always 0
+ uint16_t capability;/* bit5:preamble bit6:pbcc pbcc
not supported always 0
* bit10:ShortSlotTime bit13:DSSS-OFDM
DSSS-OFDM not supported always 0 */
uint16_t scan_type;
} __packed;
@@ -427,7 +427,7 @@ struct hostif_adhoc_set2_request_t {
uint16_t reserved;
struct rate_set16_t rate_set;
struct ssid_t ssid;
- uint16_t capability;/* bit5:preamble bit6:pbcc pbcc
not supported always 0
+ uint16_t capability;/* bit5:preamble bit6:pbcc pbcc
not supported always 0
* bit10:ShortSlotTime bit13:DSSS-OFDM
DSSS-OFDM not supported always 0 */
uint16_t scan_type;
struct channel_list_t channel_list;
@@ -568,19 +568,19 @@ struct hostif_mic_failure_confirm_t {
#define TX_RATE_48M(uint8_t)(480/5)
#define TX_RATE_54M(uint8_t)(540/5)
-#define IS_11B_RATE(A)
(((A&RATE_MASK)==TX_RATE_1M)||((A&RATE_MASK)==TX_RATE_2M)||\
-((A&RATE_MASK)==TX_RATE_5M)||((A&RATE_MASK)==T
X_RATE_11M))
+#define IS_11B_RATE(A) (((A&RATE_MASK) == TX_RATE_1M) ||
((A&RATE_MASK) == TX_RATE_2M) || \
+ ((A&RATE_MASK) == TX_RATE_5M) ||
((A&RATE_MASK) == TX_RATE_11M))
-#define IS_OFDM_RATE(A)
(((A&RATE_MASK)==TX_RATE_6M)||((A&RATE_MASK)==TX_RATE_12M)||\
-((A&RATE_MASK)==TX_RATE_24M)||((A&RATE_MASK)==
TX_RATE_9M)||\
-((A&RATE_MASK)==TX_RATE_18M)||((A&RATE_MASK)==
TX_RATE_36M)||\
-((A&RATE_MASK)==TX_RATE_48M)||((A&RATE_MASK)==
TX_RATE_54M))
+#define IS_OFDM_RATE(A) (((A&RATE_MASK) == TX_RATE_6M) ||
((A&RATE_MASK) == TX_RATE_12M) || \
+ ((A&RATE_MASK) == TX_RATE_24M) ||
((A&RATE_MASK) == TX_RATE_9M) || \
+ ((A&RATE_MASK) == TX_RATE_18M) ||
((A&RATE_MASK) == TX_RATE_36M) || \
+ ((A&RATE_MASK) == TX_RATE_48M) ||
((A&RATE_MASK) == TX_RATE_54M))
-#define IS_11BG_RATE(A) (IS_11B_RATE(A)||IS_OFDM_RATE(A))
+#define IS_11BG_RATE(A) (IS_11B_RATE(A) || IS_OFDM_RATE(A))
-#define
IS_OFDM_EXT_RATE(A) (((A&RATE_MASK)==TX_RATE_9M)||((A&RATE_MASK)==TX_R
ATE_18M)||\
- ((A&RATE_MASK)==TX_RATE_36M)||((A&RATE_MA
SK)==TX_RATE_48M)||\
- ((A&RATE_MASK)==TX_RATE_54M))
+#define IS_OFDM_EXT_RATE(A) (((A&RATE_MASK) == TX_RATE_9M) ||
((A&RATE_MASK) == TX_RATE_18M) || \
+ ((A&RATE_MASK) == TX_RATE
Re: Staging: ks7010: clean up ks_hostif.h
On Tue, Jan 31, 2017 at 09:53:33PM -0800, Logan Gorence wrote: > >From 58defc62d53cb473f9d745127d98df834a10b321 Mon Sep 17 00:00:00 2001 > From: Logan Gorence > Date: Tue, 31 Jan 2017 21:51:05 -0800 > Subject: [PATCH] Staging: ks7010: clean up ks_hostif.h Why is this all here in the body of your email? > > Clean up the errors in ks_hostif.h put out by checkpatch.pl. > > Signed-off by: Logan Gorence > --- > drivers/staging/ks7010/ks_hostif.h | 56 +++--- > Your patch is line-wrapped and can not be applied :( Please fix your email client up (or just use 'git send-email' and try again. Also, you can't fix "all" errors at once, you need to break this up into a "one patch per type of fix" series. thanks, greg k-h ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
