Re: [PATCH 1/2] [RFC] video: exynos mipi dsi: Making Exynos MIPI Complaint with CDF
Hi Vikas,
Some nitpicks inline
Subject: s/Complaint/Compliant
On 2 January 2013 18:47, Vikas C Sajjan wrote:
> From: Vikas Sajjan
>
Please add some description about this patch here.
> Signed-off-by: Vikas Sajjan
> ---
> drivers/video/exynos/exynos_mipi_dsi.c| 46
> ++---
> drivers/video/exynos/exynos_mipi_dsi_common.c | 22
> drivers/video/exynos/exynos_mipi_dsi_common.h | 12 +++
> include/video/exynos_mipi_dsim.h |5 ++-
> 4 files changed, 56 insertions(+), 29 deletions(-)
>
> diff --git a/drivers/video/exynos/exynos_mipi_dsi.c
> b/drivers/video/exynos/exynos_mipi_dsi.c
> index 07d70a3..88b2aa9 100644
> --- a/drivers/video/exynos/exynos_mipi_dsi.c
> +++ b/drivers/video/exynos/exynos_mipi_dsi.c
> @@ -32,14 +32,13 @@
> #include
> #include
> #include
> -
> +#include
> #include
>
> #include
>
> #include "exynos_mipi_dsi_common.h"
> #include "exynos_mipi_dsi_lowlevel.h"
> -
> struct mipi_dsim_ddi {
> int bus_id;
> struct list_headlist;
> @@ -111,12 +110,13 @@ static void exynos_mipi_update_cfg(struct
> mipi_dsim_device *dsim)
> exynos_mipi_dsi_stand_by(dsim, 1);
> }
>
> -static int exynos_mipi_dsi_early_blank_mode(struct mipi_dsim_device *dsim,
> +static int exynos_mipi_dsi_early_blank_mode(struct video_source
> *video_source,
> int power)
> {
> + struct mipi_dsim_device *dsim = container_of(video_source,
> + struct mipi_dsim_device,
> video_source);
> struct mipi_dsim_lcd_driver *client_drv = dsim->dsim_lcd_drv;
> struct mipi_dsim_lcd_device *client_dev = dsim->dsim_lcd_dev;
> -
> switch (power) {
> case FB_BLANK_POWERDOWN:
> if (dsim->suspended)
> @@ -139,12 +139,13 @@ static int exynos_mipi_dsi_early_blank_mode(struct
> mipi_dsim_device *dsim,
> return 0;
> }
>
> -static int exynos_mipi_dsi_blank_mode(struct mipi_dsim_device *dsim, int
> power)
> +static int exynos_mipi_dsi_blank_mode(struct video_source *video_source, int
> power)
> {
> + struct mipi_dsim_device *dsim = container_of(video_source,
> + struct mipi_dsim_device,
> video_source);
> struct platform_device *pdev = to_platform_device(dsim->dev);
> struct mipi_dsim_lcd_driver *client_drv = dsim->dsim_lcd_drv;
> struct mipi_dsim_lcd_device *client_dev = dsim->dsim_lcd_dev;
> -
> switch (power) {
> case FB_BLANK_UNBLANK:
> if (!dsim->suspended)
> @@ -319,12 +320,19 @@ static struct mipi_dsim_ddi
> *exynos_mipi_dsi_bind_lcd_ddi(
> return NULL;
> }
>
> -/* define MIPI-DSI Master operations. */
> -static struct mipi_dsim_master_ops master_ops = {
> - .cmd_read = exynos_mipi_dsi_rd_data,
> - .cmd_write = exynos_mipi_dsi_wr_data,
> - .get_dsim_frame_done=
> exynos_mipi_dsi_get_frame_done_status,
> - .clear_dsim_frame_done = exynos_mipi_dsi_clear_frame_done,
>
+static void panel_dsi_release(struct video_source *src)
How about exynos_dsi_panel_release in line with other function names?
> +{
> + printk("dsi entity release\n");
Please use pr_* function here (instead of printk).
> +}
> +
> +static const struct common_video_source_ops dsi_common_ops = {
Some comments for this place holder would be good.
> +};
> +
> +static const struct dsi_video_source_ops exynos_dsi_ops = {
> + .dcs_read = exynos_mipi_dsi_rd_data,
> + .dcs_write = exynos_mipi_dsi_wr_data,
> + .get_frame_done =
> exynos_mipi_dsi_get_frame_done_status,
> + .clear_frame_done = exynos_mipi_dsi_clear_frame_done,
> .set_early_blank_mode = exynos_mipi_dsi_early_blank_mode,
> .set_blank_mode = exynos_mipi_dsi_blank_mode,
> };
> @@ -362,7 +370,6 @@ static int exynos_mipi_dsi_probe(struct platform_device
> *pdev)
> }
>
> dsim->dsim_config = dsim_config;
> - dsim->master_ops = &master_ops;
>
> mutex_init(&dsim->lock);
>
> @@ -463,6 +470,19 @@ static int exynos_mipi_dsi_probe(struct platform_device
> *pdev)
>
> dsim->suspended = false;
>
> + dsim->video_source.dev = &pdev->dev;
> + dsim->video_source.release = panel_dsi_release;
> + dsim->video_source.common_ops = &dsi_common_ops;
> + dsim->video_source.ops.dsi = &exynos_dsi_ops;
> + dsim->video_source.name = "exynos";
Can't we get this from pdev?
> +
> + ret = video_source_register(&dsim->video_source);
> + if (ret < 0) {
> + printk("dsi entity register failed\n");
Please use pr_* function here (instead of printk).
> + goto err_bind;
> + }
> + printk("dsi entity registered: %p\n", &dsim->video_sour
Re: DT bindings for subdevices
Hi Guennadi,
Thanks for your response!
On Wed, Jan 2, 2013 at 3:49 PM, Guennadi Liakhovetski
wrote:
> Hi Prabhakar
>
> On Wed, 2 Jan 2013, Prabhakar Lad wrote:
>
>> Hi,
>>
>> This is my first step towards DT support for media, Question might be
>> bit amateur :)
>
> No worries, we're all doing our first steps in this direction right at the
> moment. These two recent threads should give you an idea as to where we
> stand atm:
>
> http://thread.gmane.org/gmane.linux.drivers.video-input-infrastructure/58646
>
> and (optionally, to a lesser extent)
>
> http://www.spinics.net/lists/linux-media/index.html#57836
>
Thanks for the links.
>> In the video pipeline there will be external devices (decoders/camera)
>> connected via
>> i2c, spi, csi. This sub-devices take platform data. So question is
>> moving ahead and
>> adding DT support for this subdevices how should this platform data be
>> passed through.
>> Should it be different properties for different devices.
>
> Mostly, yes.
>
Ok
>> For example the mt9t001 sensor takes following platform data:
>> struct mt9t001_platform_data {
>> unsigned int clk_pol:1;
>
> This would presumably be the standard "pclk-sample" property from the
> first of the above two quoted threads
>
>> unsigned int ext_clk;
>
> Is this the frequency? This should be replaced by a phandle, linking to a
Yes its the external clock frequency.
> clock device-tree node, assuming, your platform is implementing the
> generic clock API. If it isn't yet, not a problem either:-) In either case
> your sensor driver shall be using the v4l2_clk API to retrieve the clock
> rate and your camera host driver should be providing a matching v4l2_clk
> instance and implementing its methods, including retrieving the frequency.
>
Ok.
>> };
>> similarly mt9p031 takes following platform data:
>>
>> struct mt9p031_platform_data {
>> int (*set_xclk)(struct v4l2_subdev *subdev, int hz);
>
> Not sure what the xclk is, but, presumable, this should be ported to
> v4l2_clk too.
>
>> int reset;
>
> This is a GPIO number, used to reset the chip. You should use a property,
> probably, calling it "reset-gpios", specifying the desired GPIO.
>
>> int ext_freq;
>> int target_freq;
>
> Presumably, ext_freq should be retrieved, using v4l2_clk_get_rate() and
> target_freq could be a proprietary property of your device.
>
Ok.
Regards,
--Prabhakar
> Thanks
> Guennadi
>
>> };
>>
>> should this all be individual properties ?
>>
>> Regards,
>> --Prabhakar
>
> ---
> Guennadi Liakhovetski, Ph.D.
> Freelance Open-Source Software Developer
> http://www.open-technology.de/
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: em28xx: msi Digivox ATSC board id [0db0:8810]
On 01/02/2013 03:59 PM, Antti Palosaari wrote: On 12/18/2012 05:08 AM, Matthew Gyurgyik wrote: I can test patches Tue and Wed this week. Afterwards, I probably won't be able to test anything until Dec 28th/29th as I will be away from my workstation. In regards to my issue compiling my kernel, it helps if I include devtmpfs. :) Matthew, test? Both remote and television. http://git.linuxtv.org/anttip/media_tree.git/shortlog/refs/heads/HU345-Q regards Antti So using the HU345-Q branch I get the following results Remote: Using evtest it looks like all the key codes register correctly. (KEY_1, KEY_YELLOW, KEY_VOLUMEUP, etc...) However, ir_keytable fails [root@tux bin]# ./ir-keytable -t Not found device rc0 Tunning: I did a basic test with mplayer and tunning worked. I'll have to do more testing. Scanning: Running a scan resulted in a kernel panic. Scan command: scan -A 2 -t 1 /usr/share/dvb/atsc/us-Cable-Standard-center-frequencies-QAM256 > ~/channels_msidigivox.conf Kernel Messages: http://pyther.net/a/digivox_atsc/jan02/kernel_log.txt Let me know what additional info I can provide. As always, I appreciate the help! Thanks, Matthew -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/3] [media] s5p-mfc: Remove redundant 'break'
Thanks Kamil.
I assume you have 'Acked' this patch as well :)
On 2 January 2013 23:59, Kamil Debski wrote:
> Hi Sachin,
>
> Thank you for your patch.
>
> Best wishes,
> --
> Kamil Debski
> Linux Platform Group
> Samsung Poland R&D Center
>
>> -Original Message-
>> From: linux-media-ow...@vger.kernel.org [mailto:linux-media-
>> ow...@vger.kernel.org] On Behalf Of Sachin Kamat
>> Sent: Friday, December 28, 2012 11:18 AM
>> To: linux-media@vger.kernel.org
>> Cc: k.deb...@samsung.com; s.nawro...@samsung.com;
>> sylvester.nawro...@gmail.com; sachin.ka...@linaro.org;
>> patc...@linaro.org
>> Subject: [PATCH 2/3] [media] s5p-mfc: Remove redundant 'break'
>>
>> The code returns before this statement. Hence not required.
>> Silences the following smatch message:
>> drivers/media/platform/s5p-mfc/s5p_mfc_opr_v5.c:525
>> s5p_mfc_set_dec_frame_buffer_v5() info: ignoring unreachable code.
>>
>> Signed-off-by: Sachin Kamat
>> ---
>> drivers/media/platform/s5p-mfc/s5p_mfc_opr_v5.c |1 -
>> 1 files changed, 0 insertions(+), 1 deletions(-)
>>
>> diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_opr_v5.c
>> b/drivers/media/platform/s5p-mfc/s5p_mfc_opr_v5.c
>> index bb99d3d..b0f277e 100644
>> --- a/drivers/media/platform/s5p-mfc/s5p_mfc_opr_v5.c
>> +++ b/drivers/media/platform/s5p-mfc/s5p_mfc_opr_v5.c
>> @@ -522,7 +522,6 @@ int s5p_mfc_set_dec_frame_buffer_v5(struct
>> s5p_mfc_ctx *ctx)
>> mfc_err("Unknown codec for decoding (%x)\n",
>> ctx->codec_mode);
>> return -EINVAL;
>> - break;
>> }
>> frame_size = ctx->luma_size;
>> frame_size_ch = ctx->chroma_size;
>> --
>> 1.7.4.1
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-media"
>> in the body of a message to majord...@vger.kernel.org More majordomo
>> info at http://vger.kernel.org/majordomo-info.html
>
>
--
With warm regards,
Sachin
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: em28xx: msi Digivox ATSC board id [0db0:8810]
On Tue, Dec 11, 2012 at 10:59:06PM +0200, Antti Palosaari wrote: >Yes, that is. I have said it "million" times I would like to see that >implemented as a one single 4 byte NEC, but it is currently what it >is. What I understand David Härdeman has done some work toward that >too, but it is not ready. Correct. Still working on it. The main problem is providing it in a sensibly backwards-compatible manner. I will post patches to the ML again once it is ready. -- David Härdeman -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/3] [media] winbond-cir: increase IR receiver resolution
On Wed, Oct 24, 2012 at 10:22:41PM +0100, Sean Young wrote: >This is needed for carrier reporting. > >Signed-off-by: Sean Young >--- > drivers/media/rc/winbond-cir.c | 14 +- > 1 file changed, 9 insertions(+), 5 deletions(-) Using a resolution of 2us rather than 10us means that the resolution (and amount of work necessary for decoding a given signal) is about 25x higher than in the windows driver (which uses a 50us resolution IIRC)... Most of it is mitigated by using RLE (which I don't think the windows driver usesagain...IIRC), but it still seems unnecessary for the general case. Wouldn't it be possible to only use the high-res mode when carrier reports are actually enabled? (Yes, I know this patch was sent a long time ago) //David -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] mt9p031: Add support for regulators
Hi Guennadi, Thanks for the review. On Wednesday 02 January 2013 21:49:53 Guennadi Liakhovetski wrote: > On Wed, 2 Jan 2013, Laurent Pinchart wrote: > > Enable the regulators when powering the sensor up, and disable them when > > powering it down. > > > > The regulators are mandatory. Boards that don't allow controlling the > > sensor power lines must provide dummy regulators. > > I have been told several times, that (production) systems shouldn't use > dummy regulators, they can only be used during development until proper > regulators are implemented. Not that this should affect your patch, just > maybe we should avoid wording like "must provide dummy regulators" in > commit descriptions:-) Dummy was indeed a bad choice of word, I meant fixed voltage regulators. I'll fix the commit message. -- Regards, Laurent Pinchart -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RESEND 6/6] clk: s5p-g2d: Fix incorrect usage of IS_ERR_OR_NULL
On 01/02/2013 06:10 AM, Dan Carpenter wrote: clk_get() returns NULL if CONFIG_HAVE_CLK is disabled. It's not a problem for this driver, as it never dereferences what's returned from clk_get(). It would have to include , which it doesn't and which would have clearly indicated abuse of the clock API. Moreover, this driver now depends on architectures that select HAVE_CLK, so it couldn't be build when CONFIG_HAVE_CLK is disabled. I told Tony about this but everyone has been gone with end of year holidays so it hasn't been addressed. Tony, please fix it so people don't apply these patches until clk_get() is updated to not return NULL. It sucks to have to revert patches. As explained by Russell many times, the clock API users should not care whether the value returned from clk_get() is NULL or not. It should only be tested with IS_ERR(). The patches look fine to me, no need to do anything. --- Regards, Sylwester -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/5] em28xx: respect the message size constraints for i2c transfers
Hello Frank,
Am Wed, 02 Jan 2013 22:25:10 +0100
schrieb Frank Schäfer :
> Hi Sascha,
>
> Am 02.01.2013 21:45, schrieb Sascha Sommer:
> > Hello,
> >
> > Am Sat, 22 Dec 2012 22:07:46 -0200
> > schrieb Mauro Carvalho Chehab :
> >
> >> Em Sun, 16 Dec 2012 19:23:28 +0100
> >> Frank Schäfer escreveu:
> >>
> >>> The em2800 can transfer up to 4 bytes per i2c message.
> >>> All other em25xx/em27xx/28xx chips can transfer at least 64 bytes
> >>> per message.
> >>>
> >>> I2C adapters should never split messages transferred via the I2C
> >>> subsystem into multiple message transfers, because the result will
> >>> almost always NOT be the same as when the whole data is
> >>> transferred to the I2C client in a single message.
> >>> If the message size exceeds the capabilities of the I2C adapter,
> >>> -EOPNOTSUPP should be returned.
> >>>
> >>> Signed-off-by: Frank Schäfer
> >>> ---
> >>> drivers/media/usb/em28xx/em28xx-i2c.c | 44
> >>> ++--- 1 Datei geändert, 18 Zeilen
> >>> hinzugefügt(+), 26 Zeilen entfernt(-)
> >>>
> >>> diff --git a/drivers/media/usb/em28xx/em28xx-i2c.c
> >>> b/drivers/media/usb/em28xx/em28xx-i2c.c index 44533e4..c508c12
> >>> 100644 --- a/drivers/media/usb/em28xx/em28xx-i2c.c
> >>> +++ b/drivers/media/usb/em28xx/em28xx-i2c.c
> >>> @@ -50,14 +50,18 @@ do
> >>> { \ } while
> >>> (0)
> >>> /*
> >>> - * em2800_i2c_send_max4()
> >>> - * send up to 4 bytes to the i2c device
> >>> + * em2800_i2c_send_bytes()
> >>> + * send up to 4 bytes to the em2800 i2c device
> >>> */
> >>> -static int em2800_i2c_send_max4(struct em28xx *dev, u8 addr, u8
> >>> *buf, u16 len) +static int em2800_i2c_send_bytes(struct em28xx
> >>> *dev, u8 addr, u8 *buf, u16 len) {
> >>> int ret;
> >>> int write_timeout;
> >>> u8 b2[6];
> >>> +
> >>> + if (len < 1 || len > 4)
> >>> + return -EOPNOTSUPP;
> >>> +
> >> Except if you actually tested it with all em2800 devices, I think
> >> that this change just broke it for em2800.
> >>
> >> Maybe Sascha could review this patch series on his em2800 devices.
> >>
> >> Those devices are limited, and just like other devices (cx231xx for
> >> example), the I2C bus need to split long messages, otherwise the
> >> I2C devices will fail.
> >>
> > The only device that I own is the Terratec Cinergy 200 USB.
> > Unfortunately I left it in my parents house so I won't be able to
> > test the patch within the next two weeks. I don't know if any of the
> > other devices ever transfered more than 4 bytes but as far as I
> > remember the windows driver of the cinergy 200 usb did not do this.
> > The traces obtained from it were the only information I had during
> > development. On first sight, the splitting code looks wrong.
>
> Thanks for your reply !
> I have a Terratec Cinergy 200 USB, too, and I used this device for
> testing the code.
> You are right, the Windows driver never transfers more than 4 bytes
> (verified with USB-logs).
> Do you know if there is something like a control flag for non-stopping
> i2c transfers ?
I never encountered such a flag.
>
> Maybe you also noticed the following tread:
> http://www.spinics.net/lists/linux-media/msg57442.html
>
> Do you remember any details about your device ?
The description in this thread also matches my device.
Maybe an additional hint is that it can't capture at full
resolution because the USB packet size is to small. Therefore
the em28xx driver somewhere limits it to 360x576 for PAL (Some commits
broke this code in the past)
> One thing not mentioned in this tread is, that there seem to be
> multiple chip IDs for the EM2800.
> The em28xx only knows about ID=7 and I assume that's what you device
> uses. But the chip in my device uses ID=4...
I checked some of my old mails. The original driver used the
chip id for device detection because some users had different devices
with different ids but this might have been a coincidence.
Judging from the patch below my card uses chip id 4, too.
---
25/drivers/media/video/em28xx/em28xx-cards.c~v4l-786-chip-id-removed-since-it-isn-t-required
Fri Nov 4 16:20:36 2005 +++
25-akpm/drivers/media/video/em28xx/em28xx-cards.c Fri Nov 4
16:20:37 2005 @@ -160,7 +160,6 @@ struct em2820_board em2820_boards[] =
{ }, [EM2800_BOARD_TERRATEC_CINERGY_200] = { .name = "Terratec
Cinergy 200 USB",
- .chip_id = 0x4,
.is_em2800= 1,
.vchannels= 3,
.norm = VIDEO_MODE_PAL,
@@ -184,7 +183,6 @@ struct em2820_board em2820_boards[] = {
},
[EM2800_BOARD_LEADTEK_WINFAST_USBII] = {
.name = "Leadtek Winfast USB II",
- .chip_id = 0x2,
.is_em2800= 1,
.vchannels= 3,
.norm = VIDEO_MODE_PAL,
@@ -208,7 +206,6 @@ struct em2820_board em2820_boards[] = {
},
[EM2800_BOARD_KWORLD_USB2800] = {
.name
Re: [PATCH RFC v2 02/15] [media] Add a V4L2 OF parser
Hi Guennadi, On 01/02/2013 12:58 PM, Guennadi Liakhovetski wrote: --- /dev/null +++ b/drivers/media/v4l2-core/v4l2-of.c @@ -0,0 +1,249 @@ +/* + * V4L2 OF binding parsing library + * + * Copyright (C) 2012 Renesas Electronics Corp. + * Author: Guennadi Liakhovetski + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of version 2 of the GNU General Public License as + * published by the Free Software Foundation. + */ +#include +#include +#include +#include Is slab.h really needed? I didn't have it in my version. Maybe you meant to include string.h for memset()? I don't think it is needed, it looks like my mistake. I'll check it again and replace it with string.h. I've also noticed there are EXPORT_SYMBOL() missing for the first two functions in this file. I'll fix it in next version. --- Thanks, Sylwester -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RFC v2 01/15] [media] Add common video interfaces OF bindings documentation
Hi Sylwester
On Wed, 2 Jan 2013, Sylwester Nawrocki wrote:
> Hi Guennadi,
>
> On 01/02/2013 12:31 PM, Guennadi Liakhovetski wrote:
> > Hi Sylwester
> >
> > Thanks for picking up these patches! In general both look good to me, just
> > a couple of nit-picks, that I couldn't help remarking:-)
>
> Sure, thanks again for the feedback.
>
> > On Mon, 31 Dec 2012, Sylwester Nawrocki wrote:
> >
> > > From: Guennadi Liakhovetski
> > >
> > > This patch adds a document describing common OF bindings for video
> > > capture, output and video processing devices. It is curently mainly
> > > focused on video capture devices, with data busses defined by
> > > standards like ITU-R BT.656 or MIPI-CSI2.
> > > It also documents a method of describing data links between devices.
> > >
> > > Signed-off-by: Guennadi Liakhovetski
> > > Signed-off-by: Sylwester Nawrocki
> > > Reviewed-by: Stephen Warren
> > >
> > > ---
> > >
> > > This is basically a resend of my previous version of this patch [1],
> > > with just a few typo/grammar issue corrections.
> > >
> > > [1] http://patchwork.linuxtv.org/patch/15911/
> > > ---
> > > .../devicetree/bindings/media/video-interfaces.txt | 198
> > >
> > > 1 file changed, 198 insertions(+)
> > > create mode 100644
> > > Documentation/devicetree/bindings/media/video-interfaces.txt
> > >
> > > diff --git a/Documentation/devicetree/bindings/media/video-interfaces.txt
> > > b/Documentation/devicetree/bindings/media/video-interfaces.txt
> > > new file mode 100644
> > > index 000..d1eea35
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/media/video-interfaces.txt
> > > @@ -0,0 +1,198 @@
> > > +Common bindings for video data receiver and transmitter interfaces
> > > +
> > > +General concept
> > > +---
> > > +
> > > +Video data pipelines usually consist of external devices, e.g. camera
> > > sensors,
> > > +controlled over an I2C, SPI or UART bus, and SoC internal IP blocks,
> > > including
> > > +video DMA engines and video data processors.
> > > +
> > > +SoC internal blocks are described by DT nodes, placed similarly to other
> > > SoC
> > > +blocks. External devices are represented as child nodes of their
> > > respective
> > > +bus controller nodes, e.g. I2C.
> > > +
> > > +Data interfaces on all video devices are described by their child 'port'
> > > nodes.
> > > +Configuration of a port depends on other devices participating in the
> > > data
> > > +transfer and is described by 'endpoint' subnodes.
> > > +
> > > +dev {
> > > + #address-cells =<1>;
> > > + #size-cells =<0>;
> > > + port@0 {
> > > + endpoint@0 { ... };
> > > + endpoint@1 { ... };
> > > + };
> > > + port@1 { ... };
> > > +};
> > > +
> > > +If a port can be configured to work with more than one other device on
> > > the same
> > > +bus, an 'endpoint' child node must be provided for each of them. If more
> > > than
> > > +one port is present in a device node or there is more than one endpoint
> > > at a
> > > +port, a common scheme, using '#address-cells', '#size-cells' and 'reg'
> > > properties
> > > +is used.
> > > +
> > > +Two 'endpoint' nodes are linked with each other through their
> > > 'remote-endpoint'
> > > +phandles. An endpoint subnode of a device contains all properties needed
> > > for
> > > +configuration of this device for data exchange with the other device. In
> > > most
> > > +cases properties at the peer 'endpoint' nodes will be identical, however
> > > +they might need to be different when there is any signal modifications on
> > > the
> > > +bus between two devices, e.g. there are logic signal inverters on the
> > > lines.
> > > +
> > > +Required properties
> > > +---
> > > +
> > > +If there is more than one 'port' or more than one 'endpoint' node
> > > following
> > > +properties are required in relevant parent node:
> > > +
> > > +- #address-cells : number of cells required to define port number, should
> > > be 1.
> > > +- #size-cells: should be zero.
> > > +
> > > +Optional endpoint properties
> > > +
> > > +
> > > +- remote-endpoint : phandle to an 'endpoint' subnode of the other device
> > > node.
> >
> > This spacing before ":" looks strange to me. I personally prefer the
> > normal English rule - "x: y," i.e. no space before and a space after, but
> > I wouldn't remark on your choice of a space on each side in this specific
> > case, if it was consistent. Whereas sometimes having one space and
> > sometimes having none looks weird to me. I would go for "no space before
> > ':'" throughout this document.
>
> Gah, it was so close! ;) Sorry about it, it looked more readable to me that
> way.
> And I've checked other bindings' documentation and there was many files having
> space on both sides of a colon. Nevertheless, I will change it back to the
> original form.
>
> > > +- slave-mode : a boolean property, run the link in slave mode. Default is
> > > master
> > > + m
Re: [PATCH v2 2/5] em28xx: respect the message size constraints for i2c transfers
Am 02.01.2013 22:40, schrieb Antti Palosaari: > On 01/02/2013 11:29 PM, Frank Schäfer wrote: >> Am 02.01.2013 22:15, schrieb Antti Palosaari: >>> On 01/02/2013 11:12 PM, Frank Schäfer wrote: Hi Antti, Am 02.01.2013 20:29, schrieb Antti Palosaari: > On 12/24/2012 01:09 PM, Frank Schäfer wrote: >> Am 23.12.2012 15:46, schrieb Mauro Carvalho Chehab: >>> Em Sun, 23 Dec 2012 14:58:12 +0100 >>> Frank Schäfer escreveu: >>> Am 23.12.2012 01:07, schrieb Mauro Carvalho Chehab: > Em Sun, 16 Dec 2012 19:23:28 +0100 > Frank Schäfer escreveu: >> > Those devices are limited, and just like other devices (cx231xx > for example), > the I2C bus need to split long messages, otherwise the I2C > devices > will > fail. I2C adapters are supposed to fail with -EOPNOTSUPP if the message length exceeds their capabilities. Drivers must be able to handle this error, otherwise they have to be fixed. >>> Currently, afaikt, no V4L2 I2C client knows how to handle it. >> >> Maybe. Fortunately, it seems to cause no trouble. >> >>> Ok, returning >>> -EOPNOTSUPP if the I2C data came from userspace makes sense. >>> > Btw, there was already a long discussion with regards to > splitting > long > I2C messages at the I2C bus or at the I2C adapters. The decision > was > to do it at the I2C bus logic, as it is simpler than making a > code > at each I2C client for them to properly handle -EOPNOTSUPP and > implement > a fallback logic to reduce the transfer window until reach what's > supported by the device. While letting the i2c bus layer split messages sounds like the right thing to do, it is hard to realize that in practice. The reason is, that the needed algorithm depends on the capabilities and behavior of the i2c adapter _and_ the connected i2c client. The three main parameters are: - message size limits - client register width - automatic register index incrementation I don't know what has been discussed in past, >>> You'll need to dig into the ML archives. This is a recurrent theme, >>> and, >>> we have implementations doing I2C split at bus (most cases) and >>> a few >>> ones doing it at the client side. >> >> Yeah, I also have a working implementation of i2c block read/write >> emulation in my experimental code. ;) >> but I talked to Jean Delvare about the message size constraints a few weeks ago. He told me that it doesn't make sense to try to handle this at the i2c subsystem level. The parameters can be different for reading and writing, adapter and client and things are getting complicated quickly. >>> Jean's opinion is to push it to I2C clients (and we actually do it >>> on a >>> few cases), but as I explained before, there are several drivers >>> where >>> this is better done at the I2C bus driver, as the I2C >>> implementation >>> allows doing it easily at bus level by playing with I2C STOP >>> bits/I2C >>> start bits. >>> >>> We simply have too much I2C clients, and -EOPNOTSUPP error code >>> doesn't >>> tell the max size of the I2C messages. Adding a complex split logic >>> for every driver is not a common practice, as just a few I2C bus >>> bridge >>> drivers suffer from very strict limits. >> >> Yes, and even with those who have such a strict limit, it is >> usually not >> exceeded because the clients are too 'simple'. ;) >> >>> Also, clients that split I2C messages don't actually handle >>> -EOPNOTSUPP. >>> Instead, they have an init parameter telling the maximum size of >>> the >>> I2C messages accepted by the bus. >>> >>> The logic there is complex, and may require an additional logic at >>> the >>> bus side, in order to warrant that no I2C stop/start bits will be >>> sent >>> in the middle of a message, or otherwise the device will fail[1]. >>> >>> So, it is generally simpler and more effective to just do it at >>> the bus >>> side. >> >> Maybe. I have no opinion yet. >> My feeling is, that this should be handled by the i2c subsystem as >> much >> as possible, but >> a) it's complex due to the described reasons >> b) I have no complete concept yet >> c) the i2c people seem to be not very interested >> d) there is lots of other stuff with a higher priority on my TODO >> list > > Maybe you already have seen, but I did some initial stuff year or two > ago for implementing that but left it unimplemented as there was so > much stuff to check and discuss in or
Re: [PATCH RFC v2 01/15] [media] Add common video interfaces OF bindings documentation
Hi Guennadi,
On 01/02/2013 12:31 PM, Guennadi Liakhovetski wrote:
Hi Sylwester
Thanks for picking up these patches! In general both look good to me, just
a couple of nit-picks, that I couldn't help remarking:-)
Sure, thanks again for the feedback.
On Mon, 31 Dec 2012, Sylwester Nawrocki wrote:
From: Guennadi Liakhovetski
This patch adds a document describing common OF bindings for video
capture, output and video processing devices. It is curently mainly
focused on video capture devices, with data busses defined by
standards like ITU-R BT.656 or MIPI-CSI2.
It also documents a method of describing data links between devices.
Signed-off-by: Guennadi Liakhovetski
Signed-off-by: Sylwester Nawrocki
Reviewed-by: Stephen Warren
---
This is basically a resend of my previous version of this patch [1],
with just a few typo/grammar issue corrections.
[1] http://patchwork.linuxtv.org/patch/15911/
---
.../devicetree/bindings/media/video-interfaces.txt | 198
1 file changed, 198 insertions(+)
create mode 100644
Documentation/devicetree/bindings/media/video-interfaces.txt
diff --git a/Documentation/devicetree/bindings/media/video-interfaces.txt
b/Documentation/devicetree/bindings/media/video-interfaces.txt
new file mode 100644
index 000..d1eea35
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/video-interfaces.txt
@@ -0,0 +1,198 @@
+Common bindings for video data receiver and transmitter interfaces
+
+General concept
+---
+
+Video data pipelines usually consist of external devices, e.g. camera sensors,
+controlled over an I2C, SPI or UART bus, and SoC internal IP blocks, including
+video DMA engines and video data processors.
+
+SoC internal blocks are described by DT nodes, placed similarly to other SoC
+blocks. External devices are represented as child nodes of their respective
+bus controller nodes, e.g. I2C.
+
+Data interfaces on all video devices are described by their child 'port' nodes.
+Configuration of a port depends on other devices participating in the data
+transfer and is described by 'endpoint' subnodes.
+
+dev {
+ #address-cells =<1>;
+ #size-cells =<0>;
+ port@0 {
+ endpoint@0 { ... };
+ endpoint@1 { ... };
+ };
+ port@1 { ... };
+};
+
+If a port can be configured to work with more than one other device on the same
+bus, an 'endpoint' child node must be provided for each of them. If more than
+one port is present in a device node or there is more than one endpoint at a
+port, a common scheme, using '#address-cells', '#size-cells' and 'reg'
properties
+is used.
+
+Two 'endpoint' nodes are linked with each other through their 'remote-endpoint'
+phandles. An endpoint subnode of a device contains all properties needed for
+configuration of this device for data exchange with the other device. In most
+cases properties at the peer 'endpoint' nodes will be identical, however
+they might need to be different when there is any signal modifications on the
+bus between two devices, e.g. there are logic signal inverters on the lines.
+
+Required properties
+---
+
+If there is more than one 'port' or more than one 'endpoint' node following
+properties are required in relevant parent node:
+
+- #address-cells : number of cells required to define port number, should be 1.
+- #size-cells: should be zero.
+
+Optional endpoint properties
+
+
+- remote-endpoint : phandle to an 'endpoint' subnode of the other device node.
This spacing before ":" looks strange to me. I personally prefer the
normal English rule - "x: y," i.e. no space before and a space after, but
I wouldn't remark on your choice of a space on each side in this specific
case, if it was consistent. Whereas sometimes having one space and
sometimes having none looks weird to me. I would go for "no space before
':'" throughout this document.
Gah, it was so close! ;) Sorry about it, it looked more readable to me
that way.
And I've checked other bindings' documentation and there was many files
having
space on both sides of a colon. Nevertheless, I will change it back to the
original form.
+- slave-mode : a boolean property, run the link in slave mode. Default is
master
+ mode.
+- bus-width : number of data lines, valid for parallel buses.
As we discussed before, both "busses" and "buses" spellings are commonly
used at different locations around the world, but I think we should stick
to only one of them in a single document. It looks weird to have "buses"
in one line and "busses" in the following one.
True, I think that was the one occurrence I'd noticed and have forgotten to
correct then. I'll fix it, thanks for pointing out.
+- data-shift: on parallel data busses, if bus-width is used to specify the
+ number of data lines, data-shift can be used to specify which data lines are
+ used, e.g. "bus-width=<10>; data-shift=<2>;" means, that lines 9:2 are used.
+- hsync-acti
Re: [PATCH v2 2/5] em28xx: respect the message size constraints for i2c transfers
On 01/02/2013 11:29 PM, Frank Schäfer wrote: Am 02.01.2013 22:15, schrieb Antti Palosaari: On 01/02/2013 11:12 PM, Frank Schäfer wrote: Hi Antti, Am 02.01.2013 20:29, schrieb Antti Palosaari: On 12/24/2012 01:09 PM, Frank Schäfer wrote: Am 23.12.2012 15:46, schrieb Mauro Carvalho Chehab: Em Sun, 23 Dec 2012 14:58:12 +0100 Frank Schäfer escreveu: Am 23.12.2012 01:07, schrieb Mauro Carvalho Chehab: Em Sun, 16 Dec 2012 19:23:28 +0100 Frank Schäfer escreveu: Those devices are limited, and just like other devices (cx231xx for example), the I2C bus need to split long messages, otherwise the I2C devices will fail. I2C adapters are supposed to fail with -EOPNOTSUPP if the message length exceeds their capabilities. Drivers must be able to handle this error, otherwise they have to be fixed. Currently, afaikt, no V4L2 I2C client knows how to handle it. Maybe. Fortunately, it seems to cause no trouble. Ok, returning -EOPNOTSUPP if the I2C data came from userspace makes sense. Btw, there was already a long discussion with regards to splitting long I2C messages at the I2C bus or at the I2C adapters. The decision was to do it at the I2C bus logic, as it is simpler than making a code at each I2C client for them to properly handle -EOPNOTSUPP and implement a fallback logic to reduce the transfer window until reach what's supported by the device. While letting the i2c bus layer split messages sounds like the right thing to do, it is hard to realize that in practice. The reason is, that the needed algorithm depends on the capabilities and behavior of the i2c adapter _and_ the connected i2c client. The three main parameters are: - message size limits - client register width - automatic register index incrementation I don't know what has been discussed in past, You'll need to dig into the ML archives. This is a recurrent theme, and, we have implementations doing I2C split at bus (most cases) and a few ones doing it at the client side. Yeah, I also have a working implementation of i2c block read/write emulation in my experimental code. ;) but I talked to Jean Delvare about the message size constraints a few weeks ago. He told me that it doesn't make sense to try to handle this at the i2c subsystem level. The parameters can be different for reading and writing, adapter and client and things are getting complicated quickly. Jean's opinion is to push it to I2C clients (and we actually do it on a few cases), but as I explained before, there are several drivers where this is better done at the I2C bus driver, as the I2C implementation allows doing it easily at bus level by playing with I2C STOP bits/I2C start bits. We simply have too much I2C clients, and -EOPNOTSUPP error code doesn't tell the max size of the I2C messages. Adding a complex split logic for every driver is not a common practice, as just a few I2C bus bridge drivers suffer from very strict limits. Yes, and even with those who have such a strict limit, it is usually not exceeded because the clients are too 'simple'. ;) Also, clients that split I2C messages don't actually handle -EOPNOTSUPP. Instead, they have an init parameter telling the maximum size of the I2C messages accepted by the bus. The logic there is complex, and may require an additional logic at the bus side, in order to warrant that no I2C stop/start bits will be sent in the middle of a message, or otherwise the device will fail[1]. So, it is generally simpler and more effective to just do it at the bus side. Maybe. I have no opinion yet. My feeling is, that this should be handled by the i2c subsystem as much as possible, but a) it's complex due to the described reasons b) I have no complete concept yet c) the i2c people seem to be not very interested d) there is lots of other stuff with a higher priority on my TODO list Maybe you already have seen, but I did some initial stuff year or two ago for implementing that but left it unimplemented as there was so much stuff to check and discuss in order to agree correct solution. http://www.mail-archive.com/linux-media@vger.kernel.org/msg38840.html There is regmap which maybe could do stuff like that, I am not sure as I never tested it. At least it could do some stuff top of I2C bus. Yes, I've read this discussion, but didn't have time to take a deeper look into the regmap stuff yet. For the em28xx driver itself, there is no real need for i2c block read/write emulation at the moment. We could save only a few lines. I'm also burried with lots of other stuff at the moment which has a higher priority for me. Please note that the whole discussion has nothing to do with this patch. It just removes code which isn't and has never been working. Also I heavily disagree you what goes to I2C subsystem integration. That is clearly stuff which resides top of I2C bus and it is *not bus dependent*. There is many other buses too having similar splitting logic like SPI? I don't understand you. In which points do we disagr
Re: [PATCH RFC 05/17] fc0012: use struct for driver config
Hello Antti,
sorry that I didn't react earlier. Your patch series look very good to me. I
am happy to aknowledge it.
Acked-by: Hans-Frieder Vogt
Am Dienstag, 1. Januar 2013 schrieb Antti Palosaari:
> Hans-Frieder,
> Care to ack fc0012 related changes from that patch serie?
>
> http://git.linuxtv.org/anttip/media_tree.git/shortlog/refs/heads/af9035
>
> 6cfc01f fc0012: remove unused callback and correct one comment
> 2e9fffb fc0012: use Kernel dev_foo() logging
> 909d2c0 fc0012: rework attach() to check chip id and I/O errors
> 4a6831e fc0012: use config directly from the config struct
> 52728ff fc0012: enable clock output on attach()
> b6262d2 fc0012: add RF loop through
> cb5bd3d fc0012: use struct for driver config
>
> I will pull request these in next days anyway.
>
> regards
> Antti
>
> On 12/09/2012 09:56 PM, Antti Palosaari wrote:
> > I need even more configuration options and overloading dvb_attach()
> > for all those sounds quite stupid. Due to that switch struct and make
> > room for new options.
> >
> > Cc: Hans-Frieder Vogt
> > Signed-off-by: Antti Palosaari
> > ---
> >
> > drivers/media/tuners/fc0012.c | 9 -
> > drivers/media/tuners/fc0012.h | 20
> > drivers/media/usb/dvb-usb-v2/af9035.c | 10 --
> > drivers/media/usb/dvb-usb-v2/rtl28xxu.c | 7 ++-
> > 4 files changed, 34 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/media/tuners/fc0012.c
> > b/drivers/media/tuners/fc0012.c index 308135a..5ede0c0 100644
> > --- a/drivers/media/tuners/fc0012.c
> > +++ b/drivers/media/tuners/fc0012.c
> > @@ -436,8 +436,7 @@ static const struct dvb_tuner_ops fc0012_tuner_ops =
> > {
> >
> > };
> >
> > struct dvb_frontend *fc0012_attach(struct dvb_frontend *fe,
> >
> > - struct i2c_adapter *i2c, u8 i2c_address, int dual_master,
> > - enum fc001x_xtal_freq xtal_freq)
> > + struct i2c_adapter *i2c, const struct fc0012_config *cfg)
> >
> > {
> >
> > struct fc0012_priv *priv = NULL;
> >
> > @@ -446,9 +445,9 @@ struct dvb_frontend *fc0012_attach(struct
> > dvb_frontend *fe,
> >
> > return NULL;
> >
> > priv->i2c = i2c;
> >
> > - priv->dual_master = dual_master;
> > - priv->addr = i2c_address;
> > - priv->xtal_freq = xtal_freq;
> > + priv->dual_master = cfg->dual_master;
> > + priv->addr = cfg->i2c_address;
> > + priv->xtal_freq = cfg->xtal_freq;
> >
> > info("Fitipower FC0012 successfully attached.");
> >
> > diff --git a/drivers/media/tuners/fc0012.h
> > b/drivers/media/tuners/fc0012.h index 4dbd5ef..41946f8 100644
> > --- a/drivers/media/tuners/fc0012.h
> > +++ b/drivers/media/tuners/fc0012.h
> > @@ -24,17 +24,29 @@
> >
> > #include "dvb_frontend.h"
> > #include "fc001x-common.h"
> >
> > +struct fc0012_config {
> > + /*
> > +* I2C address
> > +*/
> > + u8 i2c_address;
> > +
> > + /*
> > +* clock
> > +*/
> > + enum fc001x_xtal_freq xtal_freq;
> > +
> > + int dual_master;
> > +};
> > +
> >
> > #if defined(CONFIG_MEDIA_TUNER_FC0012) || \
> >
> > (defined(CONFIG_MEDIA_TUNER_FC0012_MODULE) && defined(MODULE))
> >
> > extern struct dvb_frontend *fc0012_attach(struct dvb_frontend *fe,
> >
> > struct i2c_adapter *i2c,
> >
> > - u8 i2c_address, int dual_master,
> > - enum fc001x_xtal_freq xtal_freq);
> > + const struct fc0012_config *cfg);
> >
> > #else
> > static inline struct dvb_frontend *fc0012_attach(struct dvb_frontend
> > *fe,
> >
> > struct i2c_adapter *i2c,
> >
> > - u8 i2c_address, int dual_master,
> > - enum fc001x_xtal_freq xtal_freq)
> > + const struct fc0012_config *cfg)
> >
> > {
> >
> > printk(KERN_WARNING "%s: driver disabled by Kconfig\n", __func__);
> > return NULL;
> >
> > diff --git a/drivers/media/usb/dvb-usb-v2/af9035.c
> > b/drivers/media/usb/dvb-usb-v2/af9035.c index d1beb7f..6cf9ad5 100644
> > --- a/drivers/media/usb/dvb-usb-v2/af9035.c
> > +++ b/drivers/media/usb/dvb-usb-v2/af9035.c
> > @@ -900,6 +900,12 @@ static const struct fc2580_config
> > af9035_fc2580_config = {
> >
> > .clock = 16384000,
> >
> > };
> >
> > +static const struct fc0012_config af9035_fc0012_config = {
> > + .i2c_address = 0x63,
> > + .xtal_freq = FC_XTAL_36_MHZ,
> > + .dual_master = 1,
> > +};
> > +
> >
> > static int af9035_tuner_attach(struct dvb_usb_adapter *adap)
> > {
> >
> > struct state *state = adap_to_priv(adap);
> >
> > @@ -1043,8 +1049,8 @@ static int af9035_tuner_attach(struct
> > dvb_usb_adapter *adap)
> >
> > usleep_range(1, 5);
> >
> > - fe = dvb_attach(fc0012_attach, adap->fe[0], &d->i2c_adap, 0x63,
> > - 1, FC_XTAL
cron job: media_tree daily build: ERRORS
This message is generated daily by a cron job that builds media_tree for the kernels and architectures in the list below. Results of the daily build of media_tree: date:Wed Jan 2 19:00:15 CET 2013 git hash:8cd7085ff460ead3aba6174052a408f4ad52ac36 gcc version: i686-linux-gcc (GCC) 4.7.1 host hardware:x86_64 host os: 3.4.07-marune linux-git-arm-eabi-davinci: WARNINGS linux-git-arm-eabi-exynos: OK linux-git-arm-eabi-omap: ERRORS linux-git-i686: WARNINGS linux-git-m32r: OK linux-git-mips: WARNINGS linux-git-powerpc64: OK linux-git-sh: OK linux-git-x86_64: WARNINGS linux-2.6.31.12-i686: WARNINGS linux-2.6.32.6-i686: WARNINGS linux-2.6.33-i686: WARNINGS linux-2.6.34-i686: WARNINGS linux-2.6.35.3-i686: WARNINGS linux-2.6.36-i686: WARNINGS linux-2.6.37-i686: WARNINGS linux-2.6.38.2-i686: WARNINGS linux-2.6.39.1-i686: WARNINGS linux-3.0-i686: WARNINGS linux-3.1-i686: WARNINGS linux-3.2.1-i686: WARNINGS linux-3.3-i686: WARNINGS linux-3.4-i686: WARNINGS linux-3.5-i686: WARNINGS linux-3.6-i686: WARNINGS linux-3.7-i686: WARNINGS linux-3.8-rc1-i686: WARNINGS linux-2.6.31.12-x86_64: WARNINGS linux-2.6.32.6-x86_64: WARNINGS linux-2.6.33-x86_64: WARNINGS linux-2.6.34-x86_64: WARNINGS linux-2.6.35.3-x86_64: WARNINGS linux-2.6.36-x86_64: WARNINGS linux-2.6.37-x86_64: WARNINGS linux-2.6.38.2-x86_64: WARNINGS linux-2.6.39.1-x86_64: WARNINGS linux-3.0-x86_64: WARNINGS linux-3.1-x86_64: WARNINGS linux-3.2.1-x86_64: WARNINGS linux-3.3-x86_64: WARNINGS linux-3.4-x86_64: WARNINGS linux-3.5-x86_64: WARNINGS linux-3.6-x86_64: WARNINGS linux-3.7-x86_64: WARNINGS linux-3.8-rc1-x86_64: WARNINGS apps: WARNINGS spec-git: OK sparse: ERRORS Detailed results are available here: http://www.xs4all.nl/~hverkuil/logs/Wednesday.log Full logs are available here: http://www.xs4all.nl/~hverkuil/logs/Wednesday.tar.bz2 The V4L-DVB specification from this daily build is here: http://www.xs4all.nl/~hverkuil/spec/media.html -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/5] em28xx: respect the message size constraints for i2c transfers
Am 02.01.2013 22:15, schrieb Antti Palosaari: > On 01/02/2013 11:12 PM, Frank Schäfer wrote: >> Hi Antti, >> >> Am 02.01.2013 20:29, schrieb Antti Palosaari: >>> On 12/24/2012 01:09 PM, Frank Schäfer wrote: Am 23.12.2012 15:46, schrieb Mauro Carvalho Chehab: > Em Sun, 23 Dec 2012 14:58:12 +0100 > Frank Schäfer escreveu: > >> Am 23.12.2012 01:07, schrieb Mauro Carvalho Chehab: >>> Em Sun, 16 Dec 2012 19:23:28 +0100 >>> Frank Schäfer escreveu: >>> Those devices are limited, and just like other devices (cx231xx >>> for example), >>> the I2C bus need to split long messages, otherwise the I2C devices >>> will >>> fail. >> I2C adapters are supposed to fail with -EOPNOTSUPP if the message >> length >> exceeds their capabilities. >> Drivers must be able to handle this error, otherwise they have to >> be fixed. > Currently, afaikt, no V4L2 I2C client knows how to handle it. Maybe. Fortunately, it seems to cause no trouble. >Ok, returning > -EOPNOTSUPP if the I2C data came from userspace makes sense. > >>> Btw, there was already a long discussion with regards to splitting >>> long >>> I2C messages at the I2C bus or at the I2C adapters. The decision >>> was >>> to do it at the I2C bus logic, as it is simpler than making a code >>> at each I2C client for them to properly handle -EOPNOTSUPP and >>> implement >>> a fallback logic to reduce the transfer window until reach what's >>> supported by the device. >> While letting the i2c bus layer split messages sounds like the right >> thing to do, it is hard to realize that in practice. >> The reason is, that the needed algorithm depends on the >> capabilities and >> behavior of the i2c adapter _and_ the connected i2c client. >> The three main parameters are: >> - message size limits >> - client register width >> - automatic register index incrementation >> >> I don't know what has been discussed in past, > You'll need to dig into the ML archives. This is a recurrent theme, > and, > we have implementations doing I2C split at bus (most cases) and a few > ones doing it at the client side. Yeah, I also have a working implementation of i2c block read/write emulation in my experimental code. ;) >> but I talked to Jean >> Delvare about the message size constraints a few weeks ago. >> He told me that it doesn't make sense to try to handle this at >> the i2c >> subsystem level. The parameters can be different for reading and >> writing, adapter and client and things are getting complicated >> quickly. > Jean's opinion is to push it to I2C clients (and we actually do it > on a > few cases), but as I explained before, there are several drivers > where > this is better done at the I2C bus driver, as the I2C implementation > allows doing it easily at bus level by playing with I2C STOP bits/I2C > start bits. > > We simply have too much I2C clients, and -EOPNOTSUPP error code > doesn't > tell the max size of the I2C messages. Adding a complex split logic > for every driver is not a common practice, as just a few I2C bus > bridge > drivers suffer from very strict limits. Yes, and even with those who have such a strict limit, it is usually not exceeded because the clients are too 'simple'. ;) > Also, clients that split I2C messages don't actually handle > -EOPNOTSUPP. > Instead, they have an init parameter telling the maximum size of the > I2C messages accepted by the bus. > > The logic there is complex, and may require an additional logic at > the > bus side, in order to warrant that no I2C stop/start bits will be > sent > in the middle of a message, or otherwise the device will fail[1]. > > So, it is generally simpler and more effective to just do it at > the bus > side. Maybe. I have no opinion yet. My feeling is, that this should be handled by the i2c subsystem as much as possible, but a) it's complex due to the described reasons b) I have no complete concept yet c) the i2c people seem to be not very interested d) there is lots of other stuff with a higher priority on my TODO list >>> >>> Maybe you already have seen, but I did some initial stuff year or two >>> ago for implementing that but left it unimplemented as there was so >>> much stuff to check and discuss in order to agree correct solution. >>> >>> http://www.mail-archive.com/linux-media@vger.kernel.org/msg38840.html >>> >>> There is regmap which maybe could do stuff like that, I am not sure as >>> I never tested it. At least it could do some stuff top of I2C bus. >> >> Yes, I've read this discussion, but didn't have time to take a deeper >> look into the regmap stuff yet. >> >> For the em28xx driver
Re: [PATCH v2 2/5] em28xx: respect the message size constraints for i2c transfers
Hi Sascha,
Am 02.01.2013 21:45, schrieb Sascha Sommer:
> Hello,
>
> Am Sat, 22 Dec 2012 22:07:46 -0200
> schrieb Mauro Carvalho Chehab :
>
>> Em Sun, 16 Dec 2012 19:23:28 +0100
>> Frank Schäfer escreveu:
>>
>>> The em2800 can transfer up to 4 bytes per i2c message.
>>> All other em25xx/em27xx/28xx chips can transfer at least 64 bytes
>>> per message.
>>>
>>> I2C adapters should never split messages transferred via the I2C
>>> subsystem into multiple message transfers, because the result will
>>> almost always NOT be the same as when the whole data is transferred
>>> to the I2C client in a single message.
>>> If the message size exceeds the capabilities of the I2C adapter,
>>> -EOPNOTSUPP should be returned.
>>>
>>> Signed-off-by: Frank Schäfer
>>> ---
>>> drivers/media/usb/em28xx/em28xx-i2c.c | 44
>>> ++--- 1 Datei geändert, 18 Zeilen
>>> hinzugefügt(+), 26 Zeilen entfernt(-)
>>>
>>> diff --git a/drivers/media/usb/em28xx/em28xx-i2c.c
>>> b/drivers/media/usb/em28xx/em28xx-i2c.c index 44533e4..c508c12
>>> 100644 --- a/drivers/media/usb/em28xx/em28xx-i2c.c
>>> +++ b/drivers/media/usb/em28xx/em28xx-i2c.c
>>> @@ -50,14 +50,18 @@ do
>>> { \ } while
>>> (0)
>>> /*
>>> - * em2800_i2c_send_max4()
>>> - * send up to 4 bytes to the i2c device
>>> + * em2800_i2c_send_bytes()
>>> + * send up to 4 bytes to the em2800 i2c device
>>> */
>>> -static int em2800_i2c_send_max4(struct em28xx *dev, u8 addr, u8
>>> *buf, u16 len) +static int em2800_i2c_send_bytes(struct em28xx
>>> *dev, u8 addr, u8 *buf, u16 len) {
>>> int ret;
>>> int write_timeout;
>>> u8 b2[6];
>>> +
>>> + if (len < 1 || len > 4)
>>> + return -EOPNOTSUPP;
>>> +
>> Except if you actually tested it with all em2800 devices, I think that
>> this change just broke it for em2800.
>>
>> Maybe Sascha could review this patch series on his em2800 devices.
>>
>> Those devices are limited, and just like other devices (cx231xx for
>> example), the I2C bus need to split long messages, otherwise the I2C
>> devices will fail.
>>
> The only device that I own is the Terratec Cinergy 200 USB.
> Unfortunately I left it in my parents house so I won't be able to
> test the patch within the next two weeks. I don't know if any of the
> other devices ever transfered more than 4 bytes but as far as I
> remember the windows driver of the cinergy 200 usb did not do this.
> The traces obtained from it were the only information I had during
> development. On first sight, the splitting code looks wrong.
Thanks for your reply !
I have a Terratec Cinergy 200 USB, too, and I used this device for
testing the code.
You are right, the Windows driver never transfers more than 4 bytes
(verified with USB-logs).
Do you know if there is something like a control flag for non-stopping
i2c transfers ?
Maybe you also noticed the following tread:
http://www.spinics.net/lists/linux-media/msg57442.html
Do you remember any details about your device ?
One thing not mentioned in this tread is, that there seem to be multiple
chip IDs for the EM2800.
The em28xx only knows about ID=7 and I assume that's what you device
uses. But the chip in my device uses ID=4...
Regards,
Frank
> Regards
>
> Sascha
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/6] em28xx: refactor the code in em28xx_usb_disconnect()
Am 02.01.2013 20:40, schrieb Antti Palosaari: > On 12/28/2012 01:02 AM, Frank Schäfer wrote: >> The main purpose of this patch is to move the call of >> em28xx_release_resources() >> after the call of em28xx_close_extension(). >> This is necessary, because some resources might be needed/used by the >> extensions >> fini() functions when they get closed. >> Also mark the device as disconnected earlier in this function and >> unify the >> em28xx_uninit_usb_xfer() calls for analog and digital mode. > > This looks like it could fix that one bug I reported earlier. Care to > look these: > > em28xx releases I2C adapter earlier than demod/tuner/sec > http://www.spinics.net/lists/linux-media/msg54693.html Yes, this one gets fixed with the patch. > > em28xx #0: submit of audio urb failed > http://www.spinics.net/lists/linux-media/msg54694.html Seems to describe more than one bug. What do mean with "when I plug em28xx device with analog support" ? Did you mean "unplug" ? Does this device use an external audio IC connected via i2c (e.g. EM202) ? If yes, I think the patch should fix this issue, too. Do you still have access to this device ? Can you test the patch ? Regards, Frank > > > regards > Antti > -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/5] em28xx: respect the message size constraints for i2c transfers
On 01/02/2013 11:12 PM, Frank Schäfer wrote: Hi Antti, Am 02.01.2013 20:29, schrieb Antti Palosaari: On 12/24/2012 01:09 PM, Frank Schäfer wrote: Am 23.12.2012 15:46, schrieb Mauro Carvalho Chehab: Em Sun, 23 Dec 2012 14:58:12 +0100 Frank Schäfer escreveu: Am 23.12.2012 01:07, schrieb Mauro Carvalho Chehab: Em Sun, 16 Dec 2012 19:23:28 +0100 Frank Schäfer escreveu: Those devices are limited, and just like other devices (cx231xx for example), the I2C bus need to split long messages, otherwise the I2C devices will fail. I2C adapters are supposed to fail with -EOPNOTSUPP if the message length exceeds their capabilities. Drivers must be able to handle this error, otherwise they have to be fixed. Currently, afaikt, no V4L2 I2C client knows how to handle it. Maybe. Fortunately, it seems to cause no trouble. Ok, returning -EOPNOTSUPP if the I2C data came from userspace makes sense. Btw, there was already a long discussion with regards to splitting long I2C messages at the I2C bus or at the I2C adapters. The decision was to do it at the I2C bus logic, as it is simpler than making a code at each I2C client for them to properly handle -EOPNOTSUPP and implement a fallback logic to reduce the transfer window until reach what's supported by the device. While letting the i2c bus layer split messages sounds like the right thing to do, it is hard to realize that in practice. The reason is, that the needed algorithm depends on the capabilities and behavior of the i2c adapter _and_ the connected i2c client. The three main parameters are: - message size limits - client register width - automatic register index incrementation I don't know what has been discussed in past, You'll need to dig into the ML archives. This is a recurrent theme, and, we have implementations doing I2C split at bus (most cases) and a few ones doing it at the client side. Yeah, I also have a working implementation of i2c block read/write emulation in my experimental code. ;) but I talked to Jean Delvare about the message size constraints a few weeks ago. He told me that it doesn't make sense to try to handle this at the i2c subsystem level. The parameters can be different for reading and writing, adapter and client and things are getting complicated quickly. Jean's opinion is to push it to I2C clients (and we actually do it on a few cases), but as I explained before, there are several drivers where this is better done at the I2C bus driver, as the I2C implementation allows doing it easily at bus level by playing with I2C STOP bits/I2C start bits. We simply have too much I2C clients, and -EOPNOTSUPP error code doesn't tell the max size of the I2C messages. Adding a complex split logic for every driver is not a common practice, as just a few I2C bus bridge drivers suffer from very strict limits. Yes, and even with those who have such a strict limit, it is usually not exceeded because the clients are too 'simple'. ;) Also, clients that split I2C messages don't actually handle -EOPNOTSUPP. Instead, they have an init parameter telling the maximum size of the I2C messages accepted by the bus. The logic there is complex, and may require an additional logic at the bus side, in order to warrant that no I2C stop/start bits will be sent in the middle of a message, or otherwise the device will fail[1]. So, it is generally simpler and more effective to just do it at the bus side. Maybe. I have no opinion yet. My feeling is, that this should be handled by the i2c subsystem as much as possible, but a) it's complex due to the described reasons b) I have no complete concept yet c) the i2c people seem to be not very interested d) there is lots of other stuff with a higher priority on my TODO list Maybe you already have seen, but I did some initial stuff year or two ago for implementing that but left it unimplemented as there was so much stuff to check and discuss in order to agree correct solution. http://www.mail-archive.com/linux-media@vger.kernel.org/msg38840.html There is regmap which maybe could do stuff like that, I am not sure as I never tested it. At least it could do some stuff top of I2C bus. Yes, I've read this discussion, but didn't have time to take a deeper look into the regmap stuff yet. For the em28xx driver itself, there is no real need for i2c block read/write emulation at the moment. We could save only a few lines. I'm also burried with lots of other stuff at the moment which has a higher priority for me. Please note that the whole discussion has nothing to do with this patch. It just removes code which isn't and has never been working. Also I heavily disagree you what goes to I2C subsystem integration. That is clearly stuff which resides top of I2C bus and it is *not bus dependent*. There is many other buses too having similar splitting logic like SPI? I don't understand you. In which points do we disagree ?? "My feeling is, that this should be handled by the i2c subsystem as much as possible"
Re: [PATCH v2 2/5] em28xx: respect the message size constraints for i2c transfers
Hi Antti, Am 02.01.2013 20:29, schrieb Antti Palosaari: > On 12/24/2012 01:09 PM, Frank Schäfer wrote: >> Am 23.12.2012 15:46, schrieb Mauro Carvalho Chehab: >>> Em Sun, 23 Dec 2012 14:58:12 +0100 >>> Frank Schäfer escreveu: >>> Am 23.12.2012 01:07, schrieb Mauro Carvalho Chehab: > Em Sun, 16 Dec 2012 19:23:28 +0100 > Frank Schäfer escreveu: >> > Those devices are limited, and just like other devices (cx231xx > for example), > the I2C bus need to split long messages, otherwise the I2C devices > will > fail. I2C adapters are supposed to fail with -EOPNOTSUPP if the message length exceeds their capabilities. Drivers must be able to handle this error, otherwise they have to be fixed. >>> Currently, afaikt, no V4L2 I2C client knows how to handle it. >> >> Maybe. Fortunately, it seems to cause no trouble. >> >>> Ok, returning >>> -EOPNOTSUPP if the I2C data came from userspace makes sense. >>> > Btw, there was already a long discussion with regards to splitting > long > I2C messages at the I2C bus or at the I2C adapters. The decision was > to do it at the I2C bus logic, as it is simpler than making a code > at each I2C client for them to properly handle -EOPNOTSUPP and > implement > a fallback logic to reduce the transfer window until reach what's > supported by the device. While letting the i2c bus layer split messages sounds like the right thing to do, it is hard to realize that in practice. The reason is, that the needed algorithm depends on the capabilities and behavior of the i2c adapter _and_ the connected i2c client. The three main parameters are: - message size limits - client register width - automatic register index incrementation I don't know what has been discussed in past, >>> You'll need to dig into the ML archives. This is a recurrent theme, >>> and, >>> we have implementations doing I2C split at bus (most cases) and a few >>> ones doing it at the client side. >> >> Yeah, I also have a working implementation of i2c block read/write >> emulation in my experimental code. ;) >> but I talked to Jean Delvare about the message size constraints a few weeks ago. He told me that it doesn't make sense to try to handle this at the i2c subsystem level. The parameters can be different for reading and writing, adapter and client and things are getting complicated quickly. >>> Jean's opinion is to push it to I2C clients (and we actually do it on a >>> few cases), but as I explained before, there are several drivers where >>> this is better done at the I2C bus driver, as the I2C implementation >>> allows doing it easily at bus level by playing with I2C STOP bits/I2C >>> start bits. >>> >>> We simply have too much I2C clients, and -EOPNOTSUPP error code doesn't >>> tell the max size of the I2C messages. Adding a complex split logic >>> for every driver is not a common practice, as just a few I2C bus bridge >>> drivers suffer from very strict limits. >> >> Yes, and even with those who have such a strict limit, it is usually not >> exceeded because the clients are too 'simple'. ;) >> >>> Also, clients that split I2C messages don't actually handle >>> -EOPNOTSUPP. >>> Instead, they have an init parameter telling the maximum size of the >>> I2C messages accepted by the bus. >>> >>> The logic there is complex, and may require an additional logic at the >>> bus side, in order to warrant that no I2C stop/start bits will be sent >>> in the middle of a message, or otherwise the device will fail[1]. >>> >>> So, it is generally simpler and more effective to just do it at the bus >>> side. >> >> Maybe. I have no opinion yet. >> My feeling is, that this should be handled by the i2c subsystem as much >> as possible, but >> a) it's complex due to the described reasons >> b) I have no complete concept yet >> c) the i2c people seem to be not very interested >> d) there is lots of other stuff with a higher priority on my TODO list > > Maybe you already have seen, but I did some initial stuff year or two > ago for implementing that but left it unimplemented as there was so > much stuff to check and discuss in order to agree correct solution. > > http://www.mail-archive.com/linux-media@vger.kernel.org/msg38840.html > > There is regmap which maybe could do stuff like that, I am not sure as > I never tested it. At least it could do some stuff top of I2C bus. Yes, I've read this discussion, but didn't have time to take a deeper look into the regmap stuff yet. For the em28xx driver itself, there is no real need for i2c block read/write emulation at the moment. We could save only a few lines. I'm also burried with lots of other stuff at the moment which has a higher priority for me. Please note that the whole discussion has nothing to do with this patch. It just removes code which isn't and has never been working. > > Also I heavily disagree you what goes t
Re: [PATCH v2 2/5] em28xx: respect the message size constraints for i2c transfers
Hello,
Am Sat, 22 Dec 2012 22:07:46 -0200
schrieb Mauro Carvalho Chehab :
> Em Sun, 16 Dec 2012 19:23:28 +0100
> Frank Schäfer escreveu:
>
> > The em2800 can transfer up to 4 bytes per i2c message.
> > All other em25xx/em27xx/28xx chips can transfer at least 64 bytes
> > per message.
> >
> > I2C adapters should never split messages transferred via the I2C
> > subsystem into multiple message transfers, because the result will
> > almost always NOT be the same as when the whole data is transferred
> > to the I2C client in a single message.
> > If the message size exceeds the capabilities of the I2C adapter,
> > -EOPNOTSUPP should be returned.
> >
> > Signed-off-by: Frank Schäfer
> > ---
> > drivers/media/usb/em28xx/em28xx-i2c.c | 44
> > ++--- 1 Datei geändert, 18 Zeilen
> > hinzugefügt(+), 26 Zeilen entfernt(-)
> >
> > diff --git a/drivers/media/usb/em28xx/em28xx-i2c.c
> > b/drivers/media/usb/em28xx/em28xx-i2c.c index 44533e4..c508c12
> > 100644 --- a/drivers/media/usb/em28xx/em28xx-i2c.c
> > +++ b/drivers/media/usb/em28xx/em28xx-i2c.c
> > @@ -50,14 +50,18 @@ do
> > { \ } while
> > (0)
> > /*
> > - * em2800_i2c_send_max4()
> > - * send up to 4 bytes to the i2c device
> > + * em2800_i2c_send_bytes()
> > + * send up to 4 bytes to the em2800 i2c device
> > */
> > -static int em2800_i2c_send_max4(struct em28xx *dev, u8 addr, u8
> > *buf, u16 len) +static int em2800_i2c_send_bytes(struct em28xx
> > *dev, u8 addr, u8 *buf, u16 len) {
> > int ret;
> > int write_timeout;
> > u8 b2[6];
> > +
> > + if (len < 1 || len > 4)
> > + return -EOPNOTSUPP;
> > +
>
> Except if you actually tested it with all em2800 devices, I think that
> this change just broke it for em2800.
>
> Maybe Sascha could review this patch series on his em2800 devices.
>
> Those devices are limited, and just like other devices (cx231xx for
> example), the I2C bus need to split long messages, otherwise the I2C
> devices will fail.
>
The only device that I own is the Terratec Cinergy 200 USB.
Unfortunately I left it in my parents house so I won't be able to
test the patch within the next two weeks. I don't know if any of the
other devices ever transfered more than 4 bytes but as far as I
remember the windows driver of the cinergy 200 usb did not do this.
The traces obtained from it were the only information I had during
development. On first sight, the splitting code looks wrong.
Regards
Sascha
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: em28xx: msi Digivox ATSC board id [0db0:8810]
On 12/18/2012 05:08 AM, Matthew Gyurgyik wrote: I can test patches Tue and Wed this week. Afterwards, I probably won't be able to test anything until Dec 28th/29th as I will be away from my workstation. In regards to my issue compiling my kernel, it helps if I include devtmpfs. :) Matthew, test? Both remote and television. http://git.linuxtv.org/anttip/media_tree.git/shortlog/refs/heads/HU345-Q regards Antti -- http://palosaari.fi/ -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] mt9p031: Add support for regulators
Hi Laurent
On Wed, 2 Jan 2013, Laurent Pinchart wrote:
> Enable the regulators when powering the sensor up, and disable them when
> powering it down.
>
> The regulators are mandatory. Boards that don't allow controlling the
> sensor power lines must provide dummy regulators.
I have been told several times, that (production) systems shouldn't use
dummy regulators, they can only be used during development until proper
regulators are implemented. Not that this should affect your patch, just
maybe we should avoid wording like "must provide dummy regulators" in
commit descriptions:-)
Thanks
Guennadi
>
> Signed-off-by: Laurent Pinchart
> ---
> drivers/media/i2c/mt9p031.c | 24
> 1 files changed, 24 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/media/i2c/mt9p031.c b/drivers/media/i2c/mt9p031.c
> index e0bad59..ecf4492 100644
> --- a/drivers/media/i2c/mt9p031.c
> +++ b/drivers/media/i2c/mt9p031.c
> @@ -19,6 +19,7 @@
> #include
> #include
> #include
> +#include
> #include
> #include
>
> @@ -121,6 +122,10 @@ struct mt9p031 {
> struct mutex power_lock; /* lock to protect power_count */
> int power_count;
>
> + struct regulator *vaa;
> + struct regulator *vdd;
> + struct regulator *vdd_io;
> +
> enum mt9p031_model model;
> struct aptina_pll pll;
> int reset;
> @@ -264,6 +269,11 @@ static int mt9p031_power_on(struct mt9p031 *mt9p031)
> usleep_range(1000, 2000);
> }
>
> + /* Bring up the supplies */
> + regulator_enable(mt9p031->vdd);
> + regulator_enable(mt9p031->vdd_io);
> + regulator_enable(mt9p031->vaa);
> +
> /* Emable clock */
> if (mt9p031->pdata->set_xclk)
> mt9p031->pdata->set_xclk(&mt9p031->subdev,
> @@ -285,6 +295,10 @@ static void mt9p031_power_off(struct mt9p031 *mt9p031)
> usleep_range(1000, 2000);
> }
>
> + regulator_disable(mt9p031->vaa);
> + regulator_disable(mt9p031->vdd_io);
> + regulator_disable(mt9p031->vdd);
> +
> if (mt9p031->pdata->set_xclk)
> mt9p031->pdata->set_xclk(&mt9p031->subdev, 0);
> }
> @@ -937,6 +951,16 @@ static int mt9p031_probe(struct i2c_client *client,
> mt9p031->model = did->driver_data;
> mt9p031->reset = -1;
>
> + mt9p031->vaa = devm_regulator_get(&client->dev, "vaa");
> + mt9p031->vdd = devm_regulator_get(&client->dev, "vdd");
> + mt9p031->vdd_io = devm_regulator_get(&client->dev, "vdd_io");
> +
> + if (IS_ERR(mt9p031->vaa) || IS_ERR(mt9p031->vdd) ||
> + IS_ERR(mt9p031->vdd_io)) {
> + dev_err(&client->dev, "Unable to get regulators\n");
> + return -ENODEV;
> + }
> +
> v4l2_ctrl_handler_init(&mt9p031->ctrls, ARRAY_SIZE(mt9p031_ctrls) + 6);
>
> v4l2_ctrl_new_std(&mt9p031->ctrls, &mt9p031_ctrl_ops,
> --
> 1.7.8.6
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/6] em28xx: refactor the code in em28xx_usb_disconnect()
On 12/28/2012 01:02 AM, Frank Schäfer wrote: The main purpose of this patch is to move the call of em28xx_release_resources() after the call of em28xx_close_extension(). This is necessary, because some resources might be needed/used by the extensions fini() functions when they get closed. Also mark the device as disconnected earlier in this function and unify the em28xx_uninit_usb_xfer() calls for analog and digital mode. This looks like it could fix that one bug I reported earlier. Care to look these: em28xx releases I2C adapter earlier than demod/tuner/sec http://www.spinics.net/lists/linux-media/msg54693.html em28xx #0: submit of audio urb failed http://www.spinics.net/lists/linux-media/msg54694.html regards Antti -- http://palosaari.fi/ -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/5] em28xx: respect the message size constraints for i2c transfers
On 12/24/2012 01:09 PM, Frank Schäfer wrote: Am 23.12.2012 15:46, schrieb Mauro Carvalho Chehab: Em Sun, 23 Dec 2012 14:58:12 +0100 Frank Schäfer escreveu: Am 23.12.2012 01:07, schrieb Mauro Carvalho Chehab: Em Sun, 16 Dec 2012 19:23:28 +0100 Frank Schäfer escreveu: Those devices are limited, and just like other devices (cx231xx for example), the I2C bus need to split long messages, otherwise the I2C devices will fail. I2C adapters are supposed to fail with -EOPNOTSUPP if the message length exceeds their capabilities. Drivers must be able to handle this error, otherwise they have to be fixed. Currently, afaikt, no V4L2 I2C client knows how to handle it. Maybe. Fortunately, it seems to cause no trouble. Ok, returning -EOPNOTSUPP if the I2C data came from userspace makes sense. Btw, there was already a long discussion with regards to splitting long I2C messages at the I2C bus or at the I2C adapters. The decision was to do it at the I2C bus logic, as it is simpler than making a code at each I2C client for them to properly handle -EOPNOTSUPP and implement a fallback logic to reduce the transfer window until reach what's supported by the device. While letting the i2c bus layer split messages sounds like the right thing to do, it is hard to realize that in practice. The reason is, that the needed algorithm depends on the capabilities and behavior of the i2c adapter _and_ the connected i2c client. The three main parameters are: - message size limits - client register width - automatic register index incrementation I don't know what has been discussed in past, You'll need to dig into the ML archives. This is a recurrent theme, and, we have implementations doing I2C split at bus (most cases) and a few ones doing it at the client side. Yeah, I also have a working implementation of i2c block read/write emulation in my experimental code. ;) but I talked to Jean Delvare about the message size constraints a few weeks ago. He told me that it doesn't make sense to try to handle this at the i2c subsystem level. The parameters can be different for reading and writing, adapter and client and things are getting complicated quickly. Jean's opinion is to push it to I2C clients (and we actually do it on a few cases), but as I explained before, there are several drivers where this is better done at the I2C bus driver, as the I2C implementation allows doing it easily at bus level by playing with I2C STOP bits/I2C start bits. We simply have too much I2C clients, and -EOPNOTSUPP error code doesn't tell the max size of the I2C messages. Adding a complex split logic for every driver is not a common practice, as just a few I2C bus bridge drivers suffer from very strict limits. Yes, and even with those who have such a strict limit, it is usually not exceeded because the clients are too 'simple'. ;) Also, clients that split I2C messages don't actually handle -EOPNOTSUPP. Instead, they have an init parameter telling the maximum size of the I2C messages accepted by the bus. The logic there is complex, and may require an additional logic at the bus side, in order to warrant that no I2C stop/start bits will be sent in the middle of a message, or otherwise the device will fail[1]. So, it is generally simpler and more effective to just do it at the bus side. Maybe. I have no opinion yet. My feeling is, that this should be handled by the i2c subsystem as much as possible, but a) it's complex due to the described reasons b) I have no complete concept yet c) the i2c people seem to be not very interested d) there is lots of other stuff with a higher priority on my TODO list Maybe you already have seen, but I did some initial stuff year or two ago for implementing that but left it unimplemented as there was so much stuff to check and discuss in order to agree correct solution. http://www.mail-archive.com/linux-media@vger.kernel.org/msg38840.html There is regmap which maybe could do stuff like that, I am not sure as I never tested it. At least it could do some stuff top of I2C bus. Also I heavily disagree you what goes to I2C subsystem integration. That is clearly stuff which resides top of I2C bus and it is *not bus dependent*. There is many other buses too having similar splitting logic like SPI? The reason why I started looking into this is, that the em2765 in the SpeedLink VAD Laplace webcam (and likely all similar chip variants, e.g. em2760, em277x, em25xx) can transfer only 1 byte per message to/from the sensor client when using the 2nd i2c bus. I don't know where this restriction comes from, possible explanations are: - because that the 2nd bus is realized using GPIO pins Just to mention, there is existing framework for GPIO I2C. - because the OV2640 uses SCCB regards Antti -- http://palosaari.fi/ -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-inf
RE: [PATCH 1/3] [media] s5p-mfc: use mfc_err instead of printk
Hi Sachin,
Thank you for your patch.
Best wishes,
--
Kamil Debski
Linux Platform Group
Samsung Poland R&D Center
> From: linux-media-ow...@vger.kernel.org [mailto:linux-media-
> ow...@vger.kernel.org] On Behalf Of Sachin Kamat
> Sent: Friday, December 28, 2012 11:18 AM
> To: linux-media@vger.kernel.org
> Cc: k.deb...@samsung.com; s.nawro...@samsung.com;
> sylvester.nawro...@gmail.com; sachin.ka...@linaro.org;
> patc...@linaro.org
> Subject: [PATCH 1/3] [media] s5p-mfc: use mfc_err instead of printk
>
> Use mfc_err for consistency. Also silences checkpatch warning.
>
Acked-by: Kamil Debski
> Signed-off-by: Sachin Kamat
> ---
> drivers/media/platform/s5p-mfc/s5p_mfc_opr_v5.c |3 +--
> 1 files changed, 1 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_opr_v5.c
> b/drivers/media/platform/s5p-mfc/s5p_mfc_opr_v5.c
> index bf7d010..bb99d3d 100644
> --- a/drivers/media/platform/s5p-mfc/s5p_mfc_opr_v5.c
> +++ b/drivers/media/platform/s5p-mfc/s5p_mfc_opr_v5.c
> @@ -187,8 +187,7 @@ int s5p_mfc_alloc_codec_buffers_v5(struct
> s5p_mfc_ctx *ctx)
> dev->alloc_ctx[MFC_BANK1_ALLOC_CTX], ctx->bank1_size);
> if (IS_ERR(ctx->bank1_buf)) {
> ctx->bank1_buf = NULL;
> - printk(KERN_ERR
> -"Buf alloc for decoding failed (port A)\n");
> + mfc_err("Buf alloc for decoding failed (port A)\n");
> return -ENOMEM;
> }
> ctx->bank1_phys = s5p_mfc_mem_cookie(
> --
> 1.7.4.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media"
> in the body of a message to majord...@vger.kernel.org More majordomo
> info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 2/3] [media] s5p-mfc: Remove redundant 'break'
Hi Sachin,
Thank you for your patch.
Best wishes,
--
Kamil Debski
Linux Platform Group
Samsung Poland R&D Center
> -Original Message-
> From: linux-media-ow...@vger.kernel.org [mailto:linux-media-
> ow...@vger.kernel.org] On Behalf Of Sachin Kamat
> Sent: Friday, December 28, 2012 11:18 AM
> To: linux-media@vger.kernel.org
> Cc: k.deb...@samsung.com; s.nawro...@samsung.com;
> sylvester.nawro...@gmail.com; sachin.ka...@linaro.org;
> patc...@linaro.org
> Subject: [PATCH 2/3] [media] s5p-mfc: Remove redundant 'break'
>
> The code returns before this statement. Hence not required.
> Silences the following smatch message:
> drivers/media/platform/s5p-mfc/s5p_mfc_opr_v5.c:525
> s5p_mfc_set_dec_frame_buffer_v5() info: ignoring unreachable code.
>
> Signed-off-by: Sachin Kamat
> ---
> drivers/media/platform/s5p-mfc/s5p_mfc_opr_v5.c |1 -
> 1 files changed, 0 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_opr_v5.c
> b/drivers/media/platform/s5p-mfc/s5p_mfc_opr_v5.c
> index bb99d3d..b0f277e 100644
> --- a/drivers/media/platform/s5p-mfc/s5p_mfc_opr_v5.c
> +++ b/drivers/media/platform/s5p-mfc/s5p_mfc_opr_v5.c
> @@ -522,7 +522,6 @@ int s5p_mfc_set_dec_frame_buffer_v5(struct
> s5p_mfc_ctx *ctx)
> mfc_err("Unknown codec for decoding (%x)\n",
> ctx->codec_mode);
> return -EINVAL;
> - break;
> }
> frame_size = ctx->luma_size;
> frame_size_ch = ctx->chroma_size;
> --
> 1.7.4.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media"
> in the body of a message to majord...@vger.kernel.org More majordomo
> info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH Resend] [media] s5p-mfc: Fix an error check
Hi Sachin,
Thank you for finding and correcting this.
Best wishes,
--
Kamil Debski
Linux Platform Group
Samsung Poland R&D Center
> -Original Message-
> From: Sachin Kamat [mailto:sachin.ka...@linaro.org]
> Sent: Wednesday, January 02, 2013 11:46 AM
> To: linux-media@vger.kernel.org
> Cc: k.deb...@samsung.com; s.nawro...@samsung.com;
> sylvester.nawro...@gmail.com; sachin.ka...@linaro.org;
> patc...@linaro.org
> Subject: [PATCH Resend] [media] s5p-mfc: Fix an error check
>
> Checking unsigned variable for negative value always returns false.
> Hence make this value signed as we expect it to be negative too.
>
> Fixes the following smatch warning:
> drivers/media/platform/s5p-mfc/s5p_mfc_opr_v6.c:572
> s5p_mfc_set_enc_ref_buffer_v6() warn: unsigned 'buf_size1' is never
> less than zero.
>
> Signed-off-by: Sachin Kamat
Acked-by: Kamil Debski
> ---
> Added additional description in commit message.
> Please ignore the previous patch.
> ---
> drivers/media/platform/s5p-mfc/s5p_mfc_opr_v6.c |4 ++--
> 1 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_opr_v6.c
> b/drivers/media/platform/s5p-mfc/s5p_mfc_opr_v6.c
> index 5f9a5e0..91d5087 100644
> --- a/drivers/media/platform/s5p-mfc/s5p_mfc_opr_v6.c
> +++ b/drivers/media/platform/s5p-mfc/s5p_mfc_opr_v6.c
> @@ -535,8 +535,8 @@ void s5p_mfc_get_enc_frame_buffer_v6(struct
> s5p_mfc_ctx *ctx, int s5p_mfc_set_enc_ref_buffer_v6(struct s5p_mfc_ctx
> *ctx) {
> struct s5p_mfc_dev *dev = ctx->dev;
> - size_t buf_addr1, buf_size1;
> - int i;
> + size_t buf_addr1;
> + int i, buf_size1;
>
> mfc_debug_enter();
>
> --
> 1.7.4.1
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH/RFC 0/4] Re: dma_mmap_coherent / ARCH_HAS_DMA_MMAP_COHERENT
Hello,
On 12/28/2012 8:23 PM, Geert Uytterhoeven wrote:
On Sun, Dec 16, 2012 at 5:03 PM, Geert Uytterhoeven
wrote:
> drivers/media/v4l2-core/videobuf2-dma-contig.c: In function ‘vb2_dc_mmap’:
> drivers/media/v4l2-core/videobuf2-dma-contig.c:204: error: implicit
declaration of function ‘dma_mmap_coherent’
> drivers/media/v4l2-core/videobuf2-dma-contig.c: In function
‘vb2_dc_get_base_sgt’:
> drivers/media/v4l2-core/videobuf2-dma-contig.c:387: error: implicit
declaration of function ‘dma_get_sgtable’
> make[6]: *** [drivers/media/v4l2-core/videobuf2-dma-contig.o] Error 1
> make[6]: Target `__build' not remade because of errors.
> make[5]: *** [drivers/media/v4l2-core] Error 2
>
> Both dma_mmap_coherent() and dma_get_sgtable() are defined in
> include/asm-generic/dma-mapping-common.h only, which is included by
> on alpha, arm, arm64, hexagon, ia64, microblaze, mips,
> openrisc, powerpc, s390, sh, sparc, tile, unicore32, x86.
> Should the remaining architectures include this, too?
> Should it be moved to ?
I came up with an RFC-solution for this in [PATCH/RFC 3/4]
("avr32/bfin/c6x/cris/frv/m68k/mn10300/parisc/xtensa: Add dummy get_dma_ops()")
and [PATCH/RFC 4/4] ("common: dma-mapping: Move dma_common_*() to
") of this series.
> Furthermore, there's ARCH_HAS_DMA_MMAP_COHERENT, which is defined
> by powerpc only:
> arch/powerpc/include/asm/dma-mapping.h:#define ARCH_HAS_DMA_MMAP_COHERENT
>
> and handled in some fishy way in sound/core/pcm_native.c:
>
> #ifndef ARCH_HAS_DMA_MMAP_COHERENT
> /* This should be defined / handled globally! */
> #ifdef CONFIG_ARM
> #define ARCH_HAS_DMA_MMAP_COHERENT
> #endif
> #endif
>
> /*
> * mmap the DMA buffer on RAM
> */
> int snd_pcm_lib_default_mmap(struct snd_pcm_substream *substream,
> struct vm_area_struct *area)
> {
> area->vm_flags |= VM_DONTEXPAND | VM_DONTDUMP;
> #ifdef ARCH_HAS_DMA_MMAP_COHERENT
> if (!substream->ops->page &&
> substream->dma_buffer.dev.type == SNDRV_DMA_TYPE_DEV)
> return dma_mmap_coherent(substream->dma_buffer.dev.dev,
> area,
> substream->runtime->dma_area,
> substream->runtime->dma_addr,
> area->vm_end - area->vm_start);
> #elif defined(CONFIG_MIPS) && defined(CONFIG_DMA_NONCOHERENT)
> if (substream->dma_buffer.dev.type == SNDRV_DMA_TYPE_DEV &&
> !plat_device_is_coherent(substream->dma_buffer.dev.dev))
> area->vm_page_prot = pgprot_noncached(area->vm_page_prot);
> #endif /* ARCH_HAS_DMA_MMAP_COHERENT */
> /* mmap with fault handler */
> area->vm_ops = &snd_pcm_vm_ops_data_fault;
> return 0;
> }
> EXPORT_SYMBOL_GPL(snd_pcm_lib_default_mmap);
>
> What's up here?
Probably an easy solution here is to kill ARCH_HAS_DMA_MMAP_COHERENT and
change the code to
#if defined(CONFIG_MIPS) && defined(CONFIG_DMA_NONCOHERENT)
if (substream->dma_buffer.dev.type == SNDRV_DMA_TYPE_DEV &&
!plat_device_is_coherent(substream->dma_buffer.dev.dev))
area->vm_page_prot = pgprot_noncached(area->vm_page_prot);
#else
if (!substream->ops->page &&
substream->dma_buffer.dev.type == SNDRV_DMA_TYPE_DEV)
return dma_mmap_coherent(substream->dma_buffer.dev.dev,
area,
substream->runtime->dma_area,
substream->runtime->dma_addr,
area->vm_end - area->vm_start);
#endif
but obviously I don't like the test for CONFIG_MIPS in generic code...
I think that the best way of handling it would be to move this code to MIPS
specific dma_mmap_coherent() implementation.
Best regards
--
Marek Szyprowski
Samsung Poland R&D Center
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH/RFC 4/4] common: dma-mapping: Move dma_common_*() to
Hello, On 12/28/2012 8:23 PM, Geert Uytterhoeven wrote: dma_common_mmap() and dma_common_get_sgtable() are defined in drivers/base/dma-mapping.c, and always compiled if CONFIG_HAS_DMA=y. However, their forward declarations and the inline functions defined on top of them (dma_mmap_attrs(), dma_mmap_coherent(), dma_mmap_writecombine(), dma_get_sgtable_attrs()), dma_get_sgtable()) are in , which is not included by all architectures supporting CONFIG_HAS_DMA=y. There exist no alternative implementations. Hence for e.g. m68k allmodconfig, I get: drivers/media/v4l2-core/videobuf2-dma-contig.c: In function ‘vb2_dc_mmap’: drivers/media/v4l2-core/videobuf2-dma-contig.c:204: error: implicit declaration of function ‘dma_mmap_coherent’ drivers/media/v4l2-core/videobuf2-dma-contig.c: In function ‘vb2_dc_get_base_sgt’: drivers/media/v4l2-core/videobuf2-dma-contig.c:387: error: implicit declaration of function ‘dma_get_sgtable’ To fix this - Move the forward declarations and inline definitions to , so all CONFIG_HAS_DMA=y architectures can use them, - Replace the hard "BUG_ON(!ops)" checks for dma_map_ops by soft checks, so architectures can fall back to the common code by returning NULL from their get_dma_ops(). Note that there are no "BUG_ON(!ops)" checks in other functions in , - Make "struct dma_map_ops *ops" const while we're at it. I think that more appropriate way of handling it is to avoid dma_map_ops based calls (those archs probably have some reasons why they don't use it at all) and provide static inline stubs which call dma_common_mmap and dma_common_get_sgtable. It is definitely my fault that I missed the case of non-dma-map-ops based archs while I was adding support for dma_mmap_coherent and dma_get_sgt calls... > (snipped) Best regards -- Marek Szyprowski Samsung Poland R&D Center -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/4] media: mx2_camera: Remove i.mx25 support.
On Wed, Jan 2, 2013 at 10:35 AM, javier Martin wrote: > Sorry Fabio but IMHO that's not enough. The probe() callback may work > properly but it doesn't mean that buffer and HW handling for i.MX25 > are functional. > The condition to keep i.MX25 support in this file was that you were > going to fix it but instead you have just added board support for a > broken driver. > > I'd like to hear Guennadi's view on the matter but I think we've given > plenty of time for this. Sounds fair. Go ahead and remove mx25 camera support then. I would not be able to further debug this driver. Regards, Fabio Estevam -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/2] [RFC] video: display: Adding frame related ops to MIPI DSI video source struct
From: Vikas Sajjan
Signed-off-by: Vikas Sajjan
---
include/video/display.h |6 ++
1 file changed, 6 insertions(+)
diff --git a/include/video/display.h b/include/video/display.h
index b639fd0..fb2f437 100644
--- a/include/video/display.h
+++ b/include/video/display.h
@@ -117,6 +117,12 @@ struct dsi_video_source_ops {
void (*enable_hs)(struct video_source *src, bool enable);
+ /* frame related */
+ int (*get_frame_done)(struct video_source *src);
+ int (*clear_frame_done)(struct video_source *src);
+ int (*set_early_blank_mode)(struct video_source *src, int power);
+ int (*set_blank_mode)(struct video_source *src, int power);
+
/* data transfer */
int (*dcs_write)(struct video_source *src, int channel,
u8 *data, size_t len);
--
1.7.9.5
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/2] [RFC] video: exynos mipi dsi: Making Exynos MIPI Complaint with CDF
From: Vikas Sajjan
Signed-off-by: Vikas Sajjan
---
drivers/video/exynos/exynos_mipi_dsi.c| 46 ++---
drivers/video/exynos/exynos_mipi_dsi_common.c | 22
drivers/video/exynos/exynos_mipi_dsi_common.h | 12 +++
include/video/exynos_mipi_dsim.h |5 ++-
4 files changed, 56 insertions(+), 29 deletions(-)
diff --git a/drivers/video/exynos/exynos_mipi_dsi.c
b/drivers/video/exynos/exynos_mipi_dsi.c
index 07d70a3..88b2aa9 100644
--- a/drivers/video/exynos/exynos_mipi_dsi.c
+++ b/drivers/video/exynos/exynos_mipi_dsi.c
@@ -32,14 +32,13 @@
#include
#include
#include
-
+#include
#include
#include
#include "exynos_mipi_dsi_common.h"
#include "exynos_mipi_dsi_lowlevel.h"
-
struct mipi_dsim_ddi {
int bus_id;
struct list_headlist;
@@ -111,12 +110,13 @@ static void exynos_mipi_update_cfg(struct
mipi_dsim_device *dsim)
exynos_mipi_dsi_stand_by(dsim, 1);
}
-static int exynos_mipi_dsi_early_blank_mode(struct mipi_dsim_device *dsim,
+static int exynos_mipi_dsi_early_blank_mode(struct video_source *video_source,
int power)
{
+ struct mipi_dsim_device *dsim = container_of(video_source,
+ struct mipi_dsim_device, video_source);
struct mipi_dsim_lcd_driver *client_drv = dsim->dsim_lcd_drv;
struct mipi_dsim_lcd_device *client_dev = dsim->dsim_lcd_dev;
-
switch (power) {
case FB_BLANK_POWERDOWN:
if (dsim->suspended)
@@ -139,12 +139,13 @@ static int exynos_mipi_dsi_early_blank_mode(struct
mipi_dsim_device *dsim,
return 0;
}
-static int exynos_mipi_dsi_blank_mode(struct mipi_dsim_device *dsim, int power)
+static int exynos_mipi_dsi_blank_mode(struct video_source *video_source, int
power)
{
+ struct mipi_dsim_device *dsim = container_of(video_source,
+ struct mipi_dsim_device, video_source);
struct platform_device *pdev = to_platform_device(dsim->dev);
struct mipi_dsim_lcd_driver *client_drv = dsim->dsim_lcd_drv;
struct mipi_dsim_lcd_device *client_dev = dsim->dsim_lcd_dev;
-
switch (power) {
case FB_BLANK_UNBLANK:
if (!dsim->suspended)
@@ -319,12 +320,19 @@ static struct mipi_dsim_ddi *exynos_mipi_dsi_bind_lcd_ddi(
return NULL;
}
-/* define MIPI-DSI Master operations. */
-static struct mipi_dsim_master_ops master_ops = {
- .cmd_read = exynos_mipi_dsi_rd_data,
- .cmd_write = exynos_mipi_dsi_wr_data,
- .get_dsim_frame_done= exynos_mipi_dsi_get_frame_done_status,
- .clear_dsim_frame_done = exynos_mipi_dsi_clear_frame_done,
+static void panel_dsi_release(struct video_source *src)
+{
+ printk("dsi entity release\n");
+}
+
+static const struct common_video_source_ops dsi_common_ops = {
+};
+
+static const struct dsi_video_source_ops exynos_dsi_ops = {
+ .dcs_read = exynos_mipi_dsi_rd_data,
+ .dcs_write = exynos_mipi_dsi_wr_data,
+ .get_frame_done = exynos_mipi_dsi_get_frame_done_status,
+ .clear_frame_done = exynos_mipi_dsi_clear_frame_done,
.set_early_blank_mode = exynos_mipi_dsi_early_blank_mode,
.set_blank_mode = exynos_mipi_dsi_blank_mode,
};
@@ -362,7 +370,6 @@ static int exynos_mipi_dsi_probe(struct platform_device
*pdev)
}
dsim->dsim_config = dsim_config;
- dsim->master_ops = &master_ops;
mutex_init(&dsim->lock);
@@ -463,6 +470,19 @@ static int exynos_mipi_dsi_probe(struct platform_device
*pdev)
dsim->suspended = false;
+ dsim->video_source.dev = &pdev->dev;
+ dsim->video_source.release = panel_dsi_release;
+ dsim->video_source.common_ops = &dsi_common_ops;
+ dsim->video_source.ops.dsi = &exynos_dsi_ops;
+ dsim->video_source.name = "exynos";
+
+ ret = video_source_register(&dsim->video_source);
+ if (ret < 0) {
+ printk("dsi entity register failed\n");
+ goto err_bind;
+ }
+ printk("dsi entity registered: %p\n", &dsim->video_source);
+ return 0;
done:
platform_set_drvdata(pdev, dsim);
diff --git a/drivers/video/exynos/exynos_mipi_dsi_common.c
b/drivers/video/exynos/exynos_mipi_dsi_common.c
index 3cd29a4..e59911e 100644
--- a/drivers/video/exynos/exynos_mipi_dsi_common.c
+++ b/drivers/video/exynos/exynos_mipi_dsi_common.c
@@ -153,11 +153,12 @@ static void exynos_mipi_dsi_long_data_wr(struct
mipi_dsim_device *dsim,
}
}
}
-
-int exynos_mipi_dsi_wr_data(struct mipi_dsim_device *dsim, unsigned int
data_id,
- const unsigned char *data0, unsigned int data_size)
+int exynos_mipi_dsi_wr_data(struct video_source *video_source, int data_id,
+
[PATCH 0/2] Making Exynos MIPI Complaint with Common Display Framework
From: Vikas Sajjan This patchset contains 2 RFCs, 1st RFC has modiifications in exynos MIPI DSI driver. 2nd RFC has additions done to video source struct as per exynos requirements. I have NOT tested the patch yet, as i am yet recieve the MIPI DSI panel. This based on Tomi's CDF RFC. I am yet to modify s6e8ax0.c as per CDF and I have NOT tested these patches yet, as i am yet recieve the MIPI DSI panel. Vikas Sajjan (2): [RFC] video: exynos mipi dsi: Making Exynos MIPI Complaint with CDF [RFC] video: display: Adding frame related ops to MIPI DSI video source struct drivers/video/exynos/exynos_mipi_dsi.c| 46 ++--- drivers/video/exynos/exynos_mipi_dsi_common.c | 22 drivers/video/exynos/exynos_mipi_dsi_common.h | 12 +++ include/video/display.h |6 include/video/exynos_mipi_dsim.h |5 ++- 5 files changed, 62 insertions(+), 29 deletions(-) -- 1.7.9.5 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/4] media: mx2_camera: Remove i.mx25 support.
Hi Fabio, On 2 January 2013 13:25, Fabio Estevam wrote: > Hi Javier, > > On Wed, Jan 2, 2013 at 10:18 AM, javier Martin > wrote: > >> That's great. Did you need to change anything in the mx2 camera driver >> for mx25 to work? Have you already submitted the patches? > > I only touched board file code: > http://www.spinics.net/lists/arm-kernel/msg210216.html > > ,and have only verified that camera probe worked on mx25pdk. Sorry Fabio but IMHO that's not enough. The probe() callback may work properly but it doesn't mean that buffer and HW handling for i.MX25 are functional. The condition to keep i.MX25 support in this file was that you were going to fix it but instead you have just added board support for a broken driver. I'd like to hear Guennadi's view on the matter but I think we've given plenty of time for this. Regards. -- Javier Martin Vista Silicon S.L. CDTUC - FASE C - Oficina S-345 Avda de los Castros s/n 39005- Santander. Cantabria. Spain +34 942 25 32 60 www.vista-silicon.com -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/4] media: mx2_camera: Remove i.mx25 support.
Hi Fabio, Guennadi, sorry for the long delay but I've been out of the office for a month and without internet access. On 27 November 2012 14:05, Fabio Estevam wrote: > I just added the camera support to mach-mx25_3ds.c (which I will > submit it soon to arm kernel list) and it works fine: > > soc-camera-pdrv soc-camera-pdrv.0: Probing soc-camera-pdrv.0 > mx2-camera imx25-camera.0: Camera driver attached to camera 0 > ov2640 0-0030: ov2640 Product ID 26:42 Manufacturer ID 7f:a2 > i2c i2c-0: OV2640 Probed > mx2-camera imx25-camera.0: Camera driver detached from camera 0 > mx2-camera imx25-camera.0: MX2 Camera (CSI) driver probed, clock > frequency: 2216 > > Could we please keep the mx25 support? That's great. Did you need to change anything in the mx2 camera driver for mx25 to work? Have you already submitted the patches? Regards. -- Javier Martin Vista Silicon S.L. CDTUC - FASE C - Oficina S-345 Avda de los Castros s/n 39005- Santander. Cantabria. Spain +34 942 25 32 60 www.vista-silicon.com -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/4] media: mx2_camera: Remove i.mx25 support.
Hi Javier, On Wed, Jan 2, 2013 at 10:18 AM, javier Martin wrote: > That's great. Did you need to change anything in the mx2 camera driver > for mx25 to work? Have you already submitted the patches? I only touched board file code: http://www.spinics.net/lists/arm-kernel/msg210216.html ,and have only verified that camera probe worked on mx25pdk. Regards, Fabio Estevam -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] davinci: dm355: Fix uninitialized variable compiler warnings
drivers/media/platform/davinci/dm355_ccdc.c:593:9: warning: ‘val1’ may be
used uninitialized in this function [-Wuninitialized]
drivers/media/platform/davinci/dm355_ccdc.c:560:6: note: ‘val1’ was declared
here
This is a false positive but the compiler has no way to know about it,
so initialize the variable to 0.
Signed-off-by: Lad, Prabhakar
---
drivers/media/platform/davinci/dm355_ccdc.c |2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/drivers/media/platform/davinci/dm355_ccdc.c
b/drivers/media/platform/davinci/dm355_ccdc.c
index ce0e413..3c28aaa 100644
--- a/drivers/media/platform/davinci/dm355_ccdc.c
+++ b/drivers/media/platform/davinci/dm355_ccdc.c
@@ -557,7 +557,7 @@ static int ccdc_config_vdfc(struct ccdc_vertical_dft *dfc)
*/
static void ccdc_config_csc(struct ccdc_csc *csc)
{
- u32 val1, val2;
+ u32 val1 = 0, val2;
int i;
if (!csc->enable)
--
1.7.4.1
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RFC v2 02/15] [media] Add a V4L2 OF parser
Hi Sylwester Just one question to this one: On Mon, 31 Dec 2012, Sylwester Nawrocki wrote: > diff --git a/drivers/media/v4l2-core/v4l2-of.c > b/drivers/media/v4l2-core/v4l2-of.c > new file mode 100644 > index 000..cdac04b > --- /dev/null > +++ b/drivers/media/v4l2-core/v4l2-of.c > @@ -0,0 +1,249 @@ > +/* > + * V4L2 OF binding parsing library > + * > + * Copyright (C) 2012 Renesas Electronics Corp. > + * Author: Guennadi Liakhovetski > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of version 2 of the GNU General Public License as > + * published by the Free Software Foundation. > + */ > +#include > +#include > +#include > +#include Is slab.h really needed? I didn't have it in my version. Maybe you meant to include string.h for memset()? Thanks Guennadi --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer http://www.open-technology.de/ -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] davinci: dm644x: fix enum ccdc_gama_width and enum ccdc_data_size comparision warning
while the effect is harmless this patch fixes following build warning,
drivers/media/platform/davinci/dm644x_ccdc.c: In function ‘validate_ccdc_param’:
drivers/media/platform/davinci/dm644x_ccdc.c:233:32: warning: comparison between
‘enum ccdc_gama_width’ and ‘enum ccdc_data_size’ [-Wenum-compare]
Signed-off-by: Lad, Prabhakar
---
drivers/media/platform/davinci/dm644x_ccdc.c |5 -
1 files changed, 4 insertions(+), 1 deletions(-)
diff --git a/drivers/media/platform/davinci/dm644x_ccdc.c
b/drivers/media/platform/davinci/dm644x_ccdc.c
index ee7942b..42b473a 100644
--- a/drivers/media/platform/davinci/dm644x_ccdc.c
+++ b/drivers/media/platform/davinci/dm644x_ccdc.c
@@ -228,9 +228,12 @@ static void ccdc_readregs(void)
static int validate_ccdc_param(struct ccdc_config_params_raw *ccdcparam)
{
if (ccdcparam->alaw.enable) {
+ u32 gama_wd = ccdcparam->alaw.gama_wd;
+ u32 data_sz = ccdcparam->data_sz;
+
if ((ccdcparam->alaw.gama_wd > CCDC_GAMMA_BITS_09_0) ||
(ccdcparam->alaw.gama_wd < CCDC_GAMMA_BITS_15_6) ||
- (ccdcparam->alaw.gama_wd < ccdcparam->data_sz)) {
+ (gama_wd < data_sz)) {
dev_dbg(ccdc_cfg.dev, "\nInvalid data line select");
return -1;
}
--
1.7.4.1
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] mt9p031: Add support for regulators
Enable the regulators when powering the sensor up, and disable them when
powering it down.
The regulators are mandatory. Boards that don't allow controlling the
sensor power lines must provide dummy regulators.
Signed-off-by: Laurent Pinchart
---
drivers/media/i2c/mt9p031.c | 24
1 files changed, 24 insertions(+), 0 deletions(-)
diff --git a/drivers/media/i2c/mt9p031.c b/drivers/media/i2c/mt9p031.c
index e0bad59..ecf4492 100644
--- a/drivers/media/i2c/mt9p031.c
+++ b/drivers/media/i2c/mt9p031.c
@@ -19,6 +19,7 @@
#include
#include
#include
+#include
#include
#include
@@ -121,6 +122,10 @@ struct mt9p031 {
struct mutex power_lock; /* lock to protect power_count */
int power_count;
+ struct regulator *vaa;
+ struct regulator *vdd;
+ struct regulator *vdd_io;
+
enum mt9p031_model model;
struct aptina_pll pll;
int reset;
@@ -264,6 +269,11 @@ static int mt9p031_power_on(struct mt9p031 *mt9p031)
usleep_range(1000, 2000);
}
+ /* Bring up the supplies */
+ regulator_enable(mt9p031->vdd);
+ regulator_enable(mt9p031->vdd_io);
+ regulator_enable(mt9p031->vaa);
+
/* Emable clock */
if (mt9p031->pdata->set_xclk)
mt9p031->pdata->set_xclk(&mt9p031->subdev,
@@ -285,6 +295,10 @@ static void mt9p031_power_off(struct mt9p031 *mt9p031)
usleep_range(1000, 2000);
}
+ regulator_disable(mt9p031->vaa);
+ regulator_disable(mt9p031->vdd_io);
+ regulator_disable(mt9p031->vdd);
+
if (mt9p031->pdata->set_xclk)
mt9p031->pdata->set_xclk(&mt9p031->subdev, 0);
}
@@ -937,6 +951,16 @@ static int mt9p031_probe(struct i2c_client *client,
mt9p031->model = did->driver_data;
mt9p031->reset = -1;
+ mt9p031->vaa = devm_regulator_get(&client->dev, "vaa");
+ mt9p031->vdd = devm_regulator_get(&client->dev, "vdd");
+ mt9p031->vdd_io = devm_regulator_get(&client->dev, "vdd_io");
+
+ if (IS_ERR(mt9p031->vaa) || IS_ERR(mt9p031->vdd) ||
+ IS_ERR(mt9p031->vdd_io)) {
+ dev_err(&client->dev, "Unable to get regulators\n");
+ return -ENODEV;
+ }
+
v4l2_ctrl_handler_init(&mt9p031->ctrls, ARRAY_SIZE(mt9p031_ctrls) + 6);
v4l2_ctrl_new_std(&mt9p031->ctrls, &mt9p031_ctrl_ops,
--
1.7.8.6
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RFC v2 01/15] [media] Add common video interfaces OF bindings documentation
Hi Sylwester
Thanks for picking up these patches! In general both look good to me, just
a couple of nit-picks, that I couldn't help remarking:-)
On Mon, 31 Dec 2012, Sylwester Nawrocki wrote:
> From: Guennadi Liakhovetski
>
> This patch adds a document describing common OF bindings for video
> capture, output and video processing devices. It is curently mainly
> focused on video capture devices, with data busses defined by
> standards like ITU-R BT.656 or MIPI-CSI2.
> It also documents a method of describing data links between devices.
>
> Signed-off-by: Guennadi Liakhovetski
> Signed-off-by: Sylwester Nawrocki
> Reviewed-by: Stephen Warren
>
> ---
>
> This is basically a resend of my previous version of this patch [1],
> with just a few typo/grammar issue corrections.
>
> [1] http://patchwork.linuxtv.org/patch/15911/
> ---
> .../devicetree/bindings/media/video-interfaces.txt | 198
>
> 1 file changed, 198 insertions(+)
> create mode 100644
> Documentation/devicetree/bindings/media/video-interfaces.txt
>
> diff --git a/Documentation/devicetree/bindings/media/video-interfaces.txt
> b/Documentation/devicetree/bindings/media/video-interfaces.txt
> new file mode 100644
> index 000..d1eea35
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/video-interfaces.txt
> @@ -0,0 +1,198 @@
> +Common bindings for video data receiver and transmitter interfaces
> +
> +General concept
> +---
> +
> +Video data pipelines usually consist of external devices, e.g. camera
> sensors,
> +controlled over an I2C, SPI or UART bus, and SoC internal IP blocks,
> including
> +video DMA engines and video data processors.
> +
> +SoC internal blocks are described by DT nodes, placed similarly to other SoC
> +blocks. External devices are represented as child nodes of their respective
> +bus controller nodes, e.g. I2C.
> +
> +Data interfaces on all video devices are described by their child 'port'
> nodes.
> +Configuration of a port depends on other devices participating in the data
> +transfer and is described by 'endpoint' subnodes.
> +
> +dev {
> + #address-cells = <1>;
> + #size-cells = <0>;
> + port@0 {
> + endpoint@0 { ... };
> + endpoint@1 { ... };
> + };
> + port@1 { ... };
> +};
> +
> +If a port can be configured to work with more than one other device on the
> same
> +bus, an 'endpoint' child node must be provided for each of them. If more
> than
> +one port is present in a device node or there is more than one endpoint at a
> +port, a common scheme, using '#address-cells', '#size-cells' and 'reg'
> properties
> +is used.
> +
> +Two 'endpoint' nodes are linked with each other through their
> 'remote-endpoint'
> +phandles. An endpoint subnode of a device contains all properties needed for
> +configuration of this device for data exchange with the other device. In
> most
> +cases properties at the peer 'endpoint' nodes will be identical, however
> +they might need to be different when there is any signal modifications on the
> +bus between two devices, e.g. there are logic signal inverters on the lines.
> +
> +Required properties
> +---
> +
> +If there is more than one 'port' or more than one 'endpoint' node following
> +properties are required in relevant parent node:
> +
> +- #address-cells : number of cells required to define port number, should be
> 1.
> +- #size-cells: should be zero.
> +
> +Optional endpoint properties
> +
> +
> +- remote-endpoint : phandle to an 'endpoint' subnode of the other device
> node.
This spacing before ":" looks strange to me. I personally prefer the
normal English rule - "x: y," i.e. no space before and a space after, but
I wouldn't remark on your choice of a space on each side in this specific
case, if it was consistent. Whereas sometimes having one space and
sometimes having none looks weird to me. I would go for "no space before
':'" throughout this document.
> +- slave-mode : a boolean property, run the link in slave mode. Default is
> master
> + mode.
> +- bus-width : number of data lines, valid for parallel buses.
As we discussed before, both "busses" and "buses" spellings are commonly
used at different locations around the world, but I think we should stick
to only one of them in a single document. It looks weird to have "buses"
in one line and "busses" in the following one.
> +- data-shift: on parallel data busses, if bus-width is used to specify the
> + number of data lines, data-shift can be used to specify which data lines
> are
> + used, e.g. "bus-width=<10>; data-shift=<2>;" means, that lines 9:2 are
> used.
> +- hsync-active : active state of HSYNC signal, 0/1 for LOW/HIGH respectively.
> +- vsync-active : active state of VSYNC signal, 0/1 for LOW/HIGH respectively.
> + Note, that if HSYNC and VSYNC polarities are not specified, embedded
> + synchronization may be required, where supported.
> +
Re: [PATCH] [media] coda: Fix build due to iram.h rename
Hi Mauro, On Thu, Dec 27, 2012 at 08:15:12PM -0200, Mauro Carvalho Chehab wrote: > Em Mon, 17 Dec 2012 10:37:14 +0100 > Sascha Hauer escreveu: > > > On Wed, Nov 14, 2012 at 11:04:42AM -0200, Fabio Estevam wrote: > > > commit c045e3f13 (ARM: imx: include iram.h rather than mach/iram.h) > > > changed the > > > location of iram.h, which causes the following build error when building > > > the coda > > > driver: > > > > > > drivers/media/platform/coda.c:27:23: error: mach/iram.h: No such file or > > > directory > > > drivers/media/platform/coda.c: In function 'coda_probe': > > > drivers/media/platform/coda.c:2000: error: implicit declaration of > > > function 'iram_alloc' > > > drivers/media/platform/coda.c:2001: warning: assignment makes pointer > > > from integer without a cast > > > drivers/media/platform/coda.c: In function 'coda_remove': > > > drivers/media/platform/coda.c:2024: error: implicit declaration of > > > function 'iram_free > > > > > > Since the content of iram.h is not imx specific, move it to > > > include/linux/iram.h > > > instead. > > > > Generally we need a fix for this, but: > > > > > diff --git a/arch/arm/mach-imx/iram.h b/include/linux/iram.h > > > similarity index 100% > > > rename from arch/arm/mach-imx/iram.h > > > rename to include/linux/iram.h > > > > We shouldn't introduce a file include/linux/iram.h which is purely i.MX > > specific. The name is far too generic. I would rather suggest > > include/linux/platform_data/imx-iram.h (Although it's not exactly > > platform_data, so I'm open for better suggestions). > > > > As a side note this i.MX specific iram stuff (hopefully) is obsolete > > after the next merge window as Philip already has patches for a generic > > iram allocator which didn't make it into this merge window. > > Hi Sasha, > > This compilation breakage seems to still be happening. > > Just tested here with arm32 "allmodconfig", on a tree based on Linus one, > with -next and -media patches applied on it: > > drivers/media//platform/coda.c:27:23: fatal error: mach/iram.h: No such file > or directory > compilation terminated. > > I don't mind how this would be named, but this should be fixed somehow ;) I will prepare a patch for this next week when I'm back in the office. Sascha -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- | -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] mt9p031: Use devm_* managed helpers
Replace kzalloc and gpio_request_one by their managed equivalents.
Signed-off-by: Laurent Pinchart
---
drivers/media/i2c/mt9p031.c | 13 +++--
1 files changed, 3 insertions(+), 10 deletions(-)
diff --git a/drivers/media/i2c/mt9p031.c b/drivers/media/i2c/mt9p031.c
index e328332..e0bad59 100644
--- a/drivers/media/i2c/mt9p031.c
+++ b/drivers/media/i2c/mt9p031.c
@@ -927,7 +927,7 @@ static int mt9p031_probe(struct i2c_client *client,
return -EIO;
}
- mt9p031 = kzalloc(sizeof(*mt9p031), GFP_KERNEL);
+ mt9p031 = devm_kzalloc(&client->dev, sizeof(*mt9p031), GFP_KERNEL);
if (mt9p031 == NULL)
return -ENOMEM;
@@ -1001,8 +1001,8 @@ static int mt9p031_probe(struct i2c_client *client,
mt9p031->format.colorspace = V4L2_COLORSPACE_SRGB;
if (pdata->reset != -1) {
- ret = gpio_request_one(pdata->reset, GPIOF_OUT_INIT_LOW,
- "mt9p031_rst");
+ ret = devm_gpio_request_one(&client->dev, pdata->reset,
+ GPIOF_OUT_INIT_LOW, "mt9p031_rst");
if (ret < 0)
goto done;
@@ -1013,12 +1013,8 @@ static int mt9p031_probe(struct i2c_client *client,
done:
if (ret < 0) {
- if (mt9p031->reset != -1)
- gpio_free(mt9p031->reset);
-
v4l2_ctrl_handler_free(&mt9p031->ctrls);
media_entity_cleanup(&mt9p031->subdev.entity);
- kfree(mt9p031);
}
return ret;
@@ -1032,9 +1028,6 @@ static int mt9p031_remove(struct i2c_client *client)
v4l2_ctrl_handler_free(&mt9p031->ctrls);
v4l2_device_unregister_subdev(subdev);
media_entity_cleanup(&subdev->entity);
- if (mt9p031->reset != -1)
- gpio_free(mt9p031->reset);
- kfree(mt9p031);
return 0;
}
--
Regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/2] mt9m032: Fix PLL setup
The MT9M032 PLL was assumed to be identical to the MT9P031 PLL but
differs significantly. Update the registers definitions and PLL limits
according to the datasheet.
Signed-off-by: Laurent Pinchart
---
drivers/media/i2c/mt9m032.c | 24 +---
1 files changed, 13 insertions(+), 11 deletions(-)
diff --git a/drivers/media/i2c/mt9m032.c b/drivers/media/i2c/mt9m032.c
index f80c1d7e..30d755a 100644
--- a/drivers/media/i2c/mt9m032.c
+++ b/drivers/media/i2c/mt9m032.c
@@ -87,7 +87,7 @@
#define MT9M032_RESTART0x0b
#define MT9M032_RESET 0x0d
#define MT9M032_PLL_CONFIG10x11
-#defineMT9M032_PLL_CONFIG1_OUTDIV_MASK 0x3f
+#defineMT9M032_PLL_CONFIG1_PREDIV_MASK 0x3f
#defineMT9M032_PLL_CONFIG1_MUL_SHIFT 8
#define MT9M032_READ_MODE1 0x1e
#define MT9M032_READ_MODE2 0x20
@@ -106,6 +106,8 @@
#defineMT9M032_GAIN_AMUL_SHIFT 6
#defineMT9M032_GAIN_ANALOG_MASK0x3f
#define MT9M032_FORMATTER1 0x9e
+#defineMT9M032_FORMATTER1_PLL_P1_6 (1 << 8)
+#defineMT9M032_FORMATTER1_PARALLEL (1 << 12)
#define MT9M032_FORMATTER2 0x9f
#defineMT9M032_FORMATTER2_DOUT_EN 0x1000
#defineMT9M032_FORMATTER2_PIXCLK_EN0x2000
@@ -121,8 +123,6 @@
#defineMT9P031_PLL_CONTROL_PWROFF 0x0050
#defineMT9P031_PLL_CONTROL_PWRON 0x0051
#defineMT9P031_PLL_CONTROL_USEPLL 0x0052
-#define MT9P031_PLL_CONFIG20x11
-#defineMT9P031_PLL_CONFIG2_P1_DIV_MASK 0x1f
struct mt9m032 {
struct v4l2_subdev subdev;
@@ -255,13 +255,14 @@ static int mt9m032_setup_pll(struct mt9m032 *sensor)
.n_max = 64,
.m_min = 16,
.m_max = 255,
- .p1_min = 1,
- .p1_max = 128,
+ .p1_min = 6,
+ .p1_max = 7,
};
struct i2c_client *client = v4l2_get_subdevdata(&sensor->subdev);
struct mt9m032_platform_data *pdata = sensor->pdata;
struct aptina_pll pll;
+ u16 reg_val;
int ret;
pll.ext_clock = pdata->ext_clock;
@@ -274,18 +275,19 @@ static int mt9m032_setup_pll(struct mt9m032 *sensor)
sensor->pix_clock = pdata->pix_clock;
ret = mt9m032_write(client, MT9M032_PLL_CONFIG1,
- (pll.m << MT9M032_PLL_CONFIG1_MUL_SHIFT)
- | (pll.p1 - 1));
- if (!ret)
- ret = mt9m032_write(client, MT9P031_PLL_CONFIG2, pll.n - 1);
+ (pll.m << MT9M032_PLL_CONFIG1_MUL_SHIFT) |
+ ((pll.n - 1) & MT9M032_PLL_CONFIG1_PREDIV_MASK));
if (!ret)
ret = mt9m032_write(client, MT9P031_PLL_CONTROL,
MT9P031_PLL_CONTROL_PWRON |
MT9P031_PLL_CONTROL_USEPLL);
if (!ret) /* more reserved, Continuous, Master Mode */
ret = mt9m032_write(client, MT9M032_READ_MODE1, 0x8006);
- if (!ret) /* Set 14-bit mode, select 7 divider */
- ret = mt9m032_write(client, MT9M032_FORMATTER1, 0x111e);
+ if (!ret) {
+ reg_val = (pll.p1 == 6 ? MT9M032_FORMATTER1_PLL_P1_6 : 0)
+ | MT9M032_FORMATTER1_PARALLEL | 0x001e; /* 14-bit */
+ ret = mt9m032_write(client, MT9M032_FORMATTER1, reg_val);
+ }
return ret;
}
--
1.7.8.6
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/2] mt9m032: Define MT9M032_READ_MODE1 bits
Replace hardcoded values with #define's.
Signed-off-by: Laurent Pinchart
---
drivers/media/i2c/mt9m032.c | 22 +-
1 files changed, 21 insertions(+), 1 deletions(-)
diff --git a/drivers/media/i2c/mt9m032.c b/drivers/media/i2c/mt9m032.c
index 30d755a..de150d3 100644
--- a/drivers/media/i2c/mt9m032.c
+++ b/drivers/media/i2c/mt9m032.c
@@ -90,6 +90,24 @@
#defineMT9M032_PLL_CONFIG1_PREDIV_MASK 0x3f
#defineMT9M032_PLL_CONFIG1_MUL_SHIFT 8
#define MT9M032_READ_MODE1 0x1e
+#defineMT9M032_READ_MODE1_OUTPUT_BAD_FRAMES(1 << 13)
+#defineMT9M032_READ_MODE1_MAINTAIN_FRAME_RATE (1 << 12)
+#defineMT9M032_READ_MODE1_XOR_LINE_VALID (1 << 11)
+#defineMT9M032_READ_MODE1_CONT_LINE_VALID (1 << 10)
+#defineMT9M032_READ_MODE1_INVERT_TRIGGER (1 << 9)
+#defineMT9M032_READ_MODE1_SNAPSHOT (1 << 8)
+#defineMT9M032_READ_MODE1_GLOBAL_RESET (1 << 7)
+#defineMT9M032_READ_MODE1_BULB_EXPOSURE(1 << 6)
+#defineMT9M032_READ_MODE1_INVERT_STROBE(1 << 5)
+#defineMT9M032_READ_MODE1_STROBE_ENABLE(1 << 4)
+#defineMT9M032_READ_MODE1_STROBE_START_TRIG1 (0 << 2)
+#defineMT9M032_READ_MODE1_STROBE_START_EXP (1 << 2)
+#defineMT9M032_READ_MODE1_STROBE_START_SHUTTER (2 << 2)
+#defineMT9M032_READ_MODE1_STROBE_START_TRIG2 (3 << 2)
+#defineMT9M032_READ_MODE1_STROBE_END_TRIG1 (0 << 0)
+#defineMT9M032_READ_MODE1_STROBE_END_EXP (1 << 0)
+#defineMT9M032_READ_MODE1_STROBE_END_SHUTTER (2 << 0)
+#defineMT9M032_READ_MODE1_STROBE_END_TRIG2 (3 << 0)
#define MT9M032_READ_MODE2 0x20
#defineMT9M032_READ_MODE2_VFLIP_SHIFT 15
#defineMT9M032_READ_MODE2_HFLIP_SHIFT 14
@@ -282,7 +300,9 @@ static int mt9m032_setup_pll(struct mt9m032 *sensor)
MT9P031_PLL_CONTROL_PWRON |
MT9P031_PLL_CONTROL_USEPLL);
if (!ret) /* more reserved, Continuous, Master Mode */
- ret = mt9m032_write(client, MT9M032_READ_MODE1, 0x8006);
+ ret = mt9m032_write(client, MT9M032_READ_MODE1, 0x8000 |
+ MT9M032_READ_MODE1_STROBE_START_EXP |
+ MT9M032_READ_MODE1_STROBE_END_SHUTTER);
if (!ret) {
reg_val = (pll.p1 == 6 ? MT9M032_FORMATTER1_PLL_P1_6 : 0)
| MT9M032_FORMATTER1_PARALLEL | 0x001e; /* 14-bit */
--
1.7.8.6
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH-Trivial] [media] s5p-mfc: Fix a typo in error message in s5p_mfc_pm.c
Hi Sachin,
Thank you for this patch.
Best wishes,
--
Kamil Debski
Linux Platform Group
Samsung Poland R&D Center
> -Original Message-
> From: Sachin Kamat [mailto:sachin.ka...@linaro.org]
> Sent: Wednesday, January 02, 2013 10:14 AM
> To: linux-media@vger.kernel.org
> Cc: k.deb...@samsung.com; s.nawro...@samsung.com;
> sylvester.nawro...@gmail.com; sachin.ka...@linaro.org;
> patc...@linaro.org
> Subject: [PATCH-Trivial] [media] s5p-mfc: Fix a typo in error message
> in s5p_mfc_pm.c
>
> Fixed a trivial typo.
>
> Signed-off-by: Sachin Kamat
Acked-by: Kamil Debski
> ---
> drivers/media/platform/s5p-mfc/s5p_mfc_pm.c |2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_pm.c
> b/drivers/media/platform/s5p-mfc/s5p_mfc_pm.c
> index 2895333..6aa38a5 100644
> --- a/drivers/media/platform/s5p-mfc/s5p_mfc_pm.c
> +++ b/drivers/media/platform/s5p-mfc/s5p_mfc_pm.c
> @@ -46,7 +46,7 @@ int s5p_mfc_init_pm(struct s5p_mfc_dev *dev)
>
> ret = clk_prepare(pm->clock_gate);
> if (ret) {
> - mfc_err("Failed to preapre clock-gating control\n");
> + mfc_err("Failed to prepare clock-gating control\n");
> goto err_p_ip_clk;
> }
>
> --
> 1.7.4.1
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[GIT PULL FOR v3.9] Subdev registration fix
Hi Mauro, The following changes since commit 8cd7085ff460ead3aba6174052a408f4ad52ac36: [media] get_dvb_firmware: Fix the location of firmware for Terratec HTC (2013-01-01 11:18:26 -0200) are available in the git repository at: git://linuxtv.org/pinchartl/media.git v4l2/core Laurent Pinchart (1): v4l: Reset subdev v4l2_dev field to NULL if registration fails drivers/media/v4l2-core/v4l2-device.c | 30 ++ 1 files changed, 14 insertions(+), 16 deletions(-) -- Regards, Laurent Pinchart -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH Resend] [media] s5p-mfc: Fix an error check
Checking unsigned variable for negative value always returns false.
Hence make this value signed as we expect it to be negative too.
Fixes the following smatch warning:
drivers/media/platform/s5p-mfc/s5p_mfc_opr_v6.c:572
s5p_mfc_set_enc_ref_buffer_v6() warn: unsigned 'buf_size1' is never
less than zero.
Signed-off-by: Sachin Kamat
---
Added additional description in commit message.
Please ignore the previous patch.
---
drivers/media/platform/s5p-mfc/s5p_mfc_opr_v6.c |4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_opr_v6.c
b/drivers/media/platform/s5p-mfc/s5p_mfc_opr_v6.c
index 5f9a5e0..91d5087 100644
--- a/drivers/media/platform/s5p-mfc/s5p_mfc_opr_v6.c
+++ b/drivers/media/platform/s5p-mfc/s5p_mfc_opr_v6.c
@@ -535,8 +535,8 @@ void s5p_mfc_get_enc_frame_buffer_v6(struct s5p_mfc_ctx
*ctx,
int s5p_mfc_set_enc_ref_buffer_v6(struct s5p_mfc_ctx *ctx)
{
struct s5p_mfc_dev *dev = ctx->dev;
- size_t buf_addr1, buf_size1;
- int i;
+ size_t buf_addr1;
+ int i, buf_size1;
mfc_debug_enter();
--
1.7.4.1
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/1] [media] s5p-mfc: Fix an error check
Checking unsigned variable for negative value always returns false.
Hence make this value signed as we expect it to be negative too.
Signed-off-by: Sachin Kamat
---
drivers/media/platform/s5p-mfc/s5p_mfc_opr_v6.c |4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_opr_v6.c
b/drivers/media/platform/s5p-mfc/s5p_mfc_opr_v6.c
index 5f9a5e0..91d5087 100644
--- a/drivers/media/platform/s5p-mfc/s5p_mfc_opr_v6.c
+++ b/drivers/media/platform/s5p-mfc/s5p_mfc_opr_v6.c
@@ -535,8 +535,8 @@ void s5p_mfc_get_enc_frame_buffer_v6(struct s5p_mfc_ctx
*ctx,
int s5p_mfc_set_enc_ref_buffer_v6(struct s5p_mfc_ctx *ctx)
{
struct s5p_mfc_dev *dev = ctx->dev;
- size_t buf_addr1, buf_size1;
- int i;
+ size_t buf_addr1;
+ int i, buf_size1;
mfc_debug_enter();
--
1.7.4.1
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RFC 07/11] it913x: make remote controller optional
On 01/02/2013 05:24 AM, Fabio Estevam wrote:
On Sun, Dec 9, 2012 at 10:45 PM, Antti Palosaari wrote:
Do not compile remote controller when RC-core is disabled by Kconfig.
Cc: Malcolm Priestley
Signed-off-by: Antti Palosaari
---
drivers/media/usb/dvb-usb-v2/it913x.c | 36 +++
1 file changed, 20 insertions(+), 16 deletions(-)
diff --git a/drivers/media/usb/dvb-usb-v2/it913x.c
b/drivers/media/usb/dvb-usb-v2/it913x.c
index 4720428..5dc352b 100644
--- a/drivers/media/usb/dvb-usb-v2/it913x.c
+++ b/drivers/media/usb/dvb-usb-v2/it913x.c
@@ -308,6 +308,7 @@ static struct i2c_algorithm it913x_i2c_algo = {
};
/* Callbacks for DVB USB */
+#if defined(CONFIG_RC_CORE) || defined(CONFIG_RC_CORE_MODULE)
Maybe you could use:
#if IS_ENABLED(CONFIG_RC_CORE)
Regards,
Fabio Estevam
Thanks for the pointing that macro. I will sent new patch top of that
serie which replaces all "defined(CONFIG_RC_CORE) ||
defined(CONFIG_RC_CORE_MODULE)" with "IS_ENABLED(CONFIG_RC_CORE)" what I
added.
regards
Antti
--
http://palosaari.fi/
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: DT bindings for subdevices
Hi Prabhakar
On Wed, 2 Jan 2013, Prabhakar Lad wrote:
> Hi,
>
> This is my first step towards DT support for media, Question might be
> bit amateur :)
No worries, we're all doing our first steps in this direction right at the
moment. These two recent threads should give you an idea as to where we
stand atm:
http://thread.gmane.org/gmane.linux.drivers.video-input-infrastructure/58646
and (optionally, to a lesser extent)
http://www.spinics.net/lists/linux-media/index.html#57836
> In the video pipeline there will be external devices (decoders/camera)
> connected via
> i2c, spi, csi. This sub-devices take platform data. So question is
> moving ahead and
> adding DT support for this subdevices how should this platform data be
> passed through.
> Should it be different properties for different devices.
Mostly, yes.
> For example the mt9t001 sensor takes following platform data:
> struct mt9t001_platform_data {
> unsigned int clk_pol:1;
This would presumably be the standard "pclk-sample" property from the
first of the above two quoted threads
> unsigned int ext_clk;
Is this the frequency? This should be replaced by a phandle, linking to a
clock device-tree node, assuming, your platform is implementing the
generic clock API. If it isn't yet, not a problem either:-) In either case
your sensor driver shall be using the v4l2_clk API to retrieve the clock
rate and your camera host driver should be providing a matching v4l2_clk
instance and implementing its methods, including retrieving the frequency.
> };
> similarly mt9p031 takes following platform data:
>
> struct mt9p031_platform_data {
> int (*set_xclk)(struct v4l2_subdev *subdev, int hz);
Not sure what the xclk is, but, presumable, this should be ported to
v4l2_clk too.
> int reset;
This is a GPIO number, used to reset the chip. You should use a property,
probably, calling it "reset-gpios", specifying the desired GPIO.
> int ext_freq;
> int target_freq;
Presumably, ext_freq should be retrieved, using v4l2_clk_get_rate() and
target_freq could be a proprietary property of your device.
Thanks
Guennadi
> };
>
> should this all be individual properties ?
>
> Regards,
> --Prabhakar
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RESEND 6/6] clk: s5p-g2d: Fix incorrect usage of IS_ERR_OR_NULL
On Wed, Jan 02, 2013 at 10:44:32AM +0100, Julia Lawall wrote: > On Wed, 2 Jan 2013, Russell King - ARM Linux wrote: > > > On Wed, Jan 02, 2013 at 08:10:36AM +0300, Dan Carpenter wrote: > > > clk_get() returns NULL if CONFIG_HAVE_CLK is disabled. > > > > > > I told Tony about this but everyone has been gone with end of year > > > holidays so it hasn't been addressed. > > > > > > Tony, please fix it so people don't apply these patches until > > > clk_get() is updated to not return NULL. It sucks to have to revert > > > patches. > > > > How about people stop using IS_ERR_OR_NULL for stuff which it shouldn't > > be used for? > > Perhaps the cases where clk_get returns NULL could have a comment > indicating that NULL does not represent a failure? No. More documentation is never the answer to people not reading existing documentation. > In 3.7.1, it looks like it might have been possible for NULL to be > returned by clk_get in arch/mips/loongson1/common/clock.c, but that > definition seems to be gone in a recent linux-next. The remaining > definitions look OK. How about people just read the API and comply with it rather than doing their own thing all the time? We've already had at least one instance where someone has tried using IS_ERR() with the ioremap() return value. Really, if you're going to program kernel space, it is very important that you *know* the interfaces that you're using and you comply with them. Otherwise, you have no business saying that you're a kernel programmer. Yes, the odd mistake happens, but that's no excuse for the constant blatent mistakes with stuff like IS_ERR_OR_NULL() with clk_get() which just comes from total laziness on the part of the coder to understand the interfaces being used. Hell, it's even documented in linux/clk.h - that just shows how many people read the documentation which has been around since the clk API came about. -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH -next] [media] s5p-mfc: remove unused variable
Hi Wei,
Thank you for your patch.
Best wishes,
--
Kamil Debski
Linux Platform Group
Samsung Poland R&D Center
> -Original Message-
> From: linux-media-ow...@vger.kernel.org [mailto:linux-media-
> ow...@vger.kernel.org] On Behalf Of Wei Yongjun
> Sent: Sunday, December 02, 2012 1:17 PM
> To: kyungmin.p...@samsung.com; k.deb...@samsung.com;
> jtp.p...@samsung.com; mche...@redhat.com
> Cc: yongjun_...@trendmicro.com.cn; linux-arm-ker...@lists.infradead.org;
> linux-media@vger.kernel.org
> Subject: [PATCH -next] [media] s5p-mfc: remove unused variable
>
> From: Wei Yongjun
>
> The variable index is initialized but never used otherwise, so remove
> the unused variable.
>
> Signed-off-by: Wei Yongjun
Acked-by: Kamil Debski
> ---
> drivers/media/platform/s5p-mfc/s5p_mfc.c| 5 -
> drivers/media/platform/s5p-mfc/s5p_mfc_opr_v6.c | 6 --
> 2 files changed, 11 deletions(-)
>
> diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc.c
> b/drivers/media/platform/s5p-mfc/s5p_mfc.c
> index 3afe879..856cf00 100644
> --- a/drivers/media/platform/s5p-mfc/s5p_mfc.c
> +++ b/drivers/media/platform/s5p-mfc/s5p_mfc.c
> @@ -273,7 +273,6 @@ static void s5p_mfc_handle_frame_new(struct
> s5p_mfc_ctx *ctx, unsigned int err)
> struct s5p_mfc_buf *dst_buf;
> size_t dspl_y_addr;
> unsigned int frame_type;
> - unsigned int index;
>
> dspl_y_addr = s5p_mfc_hw_call(dev->mfc_ops, get_dspl_y_adr, dev);
> frame_type = s5p_mfc_hw_call(dev->mfc_ops, get_dec_frame_type,
> dev); @@ -310,7 +309,6 @@ static void s5p_mfc_handle_frame_new(struct
> s5p_mfc_ctx *ctx, unsigned int err)
> vb2_buffer_done(dst_buf->b,
> err ? VB2_BUF_STATE_ERROR :
VB2_BUF_STATE_DONE);
>
> - index = dst_buf->b->v4l2_buf.index;
> break;
> }
> }
> @@ -326,8 +324,6 @@ static void s5p_mfc_handle_frame(struct s5p_mfc_ctx
> *ctx,
> unsigned long flags;
> unsigned int res_change;
>
> - unsigned int index;
> -
> dst_frame_status = s5p_mfc_hw_call(dev->mfc_ops, get_dspl_status,
> dev)
> & S5P_FIMV_DEC_STATUS_DECODING_STATUS_MASK;
> res_change = (s5p_mfc_hw_call(dev->mfc_ops, get_dspl_status, dev)
> @@ -387,7 +383,6 @@ static void s5p_mfc_handle_frame(struct s5p_mfc_ctx
> *ctx,
> mfc_debug(2, "Running again the same buffer\n");
> ctx->after_packed_pb = 1;
> } else {
> - index = src_buf->b->v4l2_buf.index;
> mfc_debug(2, "MFC needs next buffer\n");
> ctx->consumed_stream = 0;
> list_del(&src_buf->list);
>
> diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_opr_v6.c
> b/drivers/media/platform/s5p-mfc/s5p_mfc_opr_v6.c
> index 3a8cfd9..bf4d2f4 100644
> --- a/drivers/media/platform/s5p-mfc/s5p_mfc_opr_v6.c
> +++ b/drivers/media/platform/s5p-mfc/s5p_mfc_opr_v6.c
> @@ -1408,7 +1408,6 @@ static inline int s5p_mfc_run_dec_frame(struct
> s5p_mfc_ctx *ctx)
> struct s5p_mfc_buf *temp_vb;
> unsigned long flags;
> int last_frame = 0;
> - unsigned int index;
>
> spin_lock_irqsave(&dev->irqlock, flags);
>
> @@ -1427,8 +1426,6 @@ static inline int s5p_mfc_run_dec_frame(struct
> s5p_mfc_ctx *ctx)
> temp_vb->b->v4l2_planes[0].bytesused);
> spin_unlock_irqrestore(&dev->irqlock, flags);
>
> - index = temp_vb->b->v4l2_buf.index;
> -
> dev->curr_ctx = ctx->num;
> s5p_mfc_clean_ctx_int_flags(ctx);
> if (temp_vb->b->v4l2_planes[0].bytesused == 0) { @@ -1452,7
> +1449,6 @@ static inline int s5p_mfc_run_enc_frame(struct s5p_mfc_ctx
> *ctx)
> unsigned int src_y_size, src_c_size;
> */
> unsigned int dst_size;
> - unsigned int index;
>
> spin_lock_irqsave(&dev->irqlock, flags);
>
> @@ -1487,8 +1483,6 @@ static inline int s5p_mfc_run_enc_frame(struct
> s5p_mfc_ctx *ctx)
>
> spin_unlock_irqrestore(&dev->irqlock, flags);
>
> - index = src_mb->b->v4l2_buf.index;
> -
> dev->curr_ctx = ctx->num;
> s5p_mfc_clean_ctx_int_flags(ctx);
> s5p_mfc_encode_one_frame_v6(ctx);
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media"
> in the body of a message to majord...@vger.kernel.org More majordomo
> info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RESEND 6/6] clk: s5p-g2d: Fix incorrect usage of IS_ERR_OR_NULL
On Wed, 2 Jan 2013, Russell King - ARM Linux wrote: > On Wed, Jan 02, 2013 at 08:10:36AM +0300, Dan Carpenter wrote: > > clk_get() returns NULL if CONFIG_HAVE_CLK is disabled. > > > > I told Tony about this but everyone has been gone with end of year > > holidays so it hasn't been addressed. > > > > Tony, please fix it so people don't apply these patches until > > clk_get() is updated to not return NULL. It sucks to have to revert > > patches. > > How about people stop using IS_ERR_OR_NULL for stuff which it shouldn't > be used for? Perhaps the cases where clk_get returns NULL could have a comment indicating that NULL does not represent a failure? In 3.7.1, it looks like it might have been possible for NULL to be returned by clk_get in arch/mips/loongson1/common/clock.c, but that definition seems to be gone in a recent linux-next. The remaining definitions look OK. julia -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RESEND 6/6] clk: s5p-g2d: Fix incorrect usage of IS_ERR_OR_NULL
On Wed, Jan 02, 2013 at 08:29:57AM +0100, Julia Lawall wrote: > There are dereferences to the result of clk_get a few times. I tried the > following semantic patch: And those are buggy; struct clk is _SUPPOSED_ to be an OPAQUE COOKIE and no one other than the clk code should be dereferencing it. I guess this is the danger of the common clk API - we now have a struct clk that is exposed in a linux/*.h include which anyone can include, and now anyone can get a definition for a struct clk and dereference it, destroying opaqueness of this cookie. -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RESEND 6/6] clk: s5p-g2d: Fix incorrect usage of IS_ERR_OR_NULL
On Wed, Jan 02, 2013 at 08:10:36AM +0300, Dan Carpenter wrote: > clk_get() returns NULL if CONFIG_HAVE_CLK is disabled. > > I told Tony about this but everyone has been gone with end of year > holidays so it hasn't been addressed. > > Tony, please fix it so people don't apply these patches until > clk_get() is updated to not return NULL. It sucks to have to revert > patches. How about people stop using IS_ERR_OR_NULL for stuff which it shouldn't be used for? -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH-Trivial] [media] s5p-mfc: Fix a typo in error message in s5p_mfc_pm.c
Fixed a trivial typo.
Signed-off-by: Sachin Kamat
---
drivers/media/platform/s5p-mfc/s5p_mfc_pm.c |2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_pm.c
b/drivers/media/platform/s5p-mfc/s5p_mfc_pm.c
index 2895333..6aa38a5 100644
--- a/drivers/media/platform/s5p-mfc/s5p_mfc_pm.c
+++ b/drivers/media/platform/s5p-mfc/s5p_mfc_pm.c
@@ -46,7 +46,7 @@ int s5p_mfc_init_pm(struct s5p_mfc_dev *dev)
ret = clk_prepare(pm->clock_gate);
if (ret) {
- mfc_err("Failed to preapre clock-gating control\n");
+ mfc_err("Failed to prepare clock-gating control\n");
goto err_p_ip_clk;
}
--
1.7.4.1
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH/RFC] crystalhd-video (vaapi-backend): struct VADriverVTable *vtable; is heap, updated to API 0.32, configure GLX support misdetection
Hi, I've found at http://gitorious.org/crystalhd-video "palatis" has started an
vaapi backend for crystalhd and updated it to API rev. 0.32
but RFC #1, Applicable Linux Media API for Broadcom Crystal HD decoders:
I don't know yet if vaapi or vdpau is the right approach for a acceptable API
for Broadcom crystal hd decoders, since it's a decoder (codec) not just an
accelerator on such GPUs
and as I understand the little documentation so far, both APIs are X-Server
rendering dependant but we've got many decoding only and framebuffer,
transcoding and embedded
systems use cases around to which neither the vdpau nor the vaapi concepts
would apply then and both API were useless for transcoders then?
Altough I don't know either yet if the Broadcom firmwares do allow faster than
realtime movie framerate at all what transcoders usually need?
According to the ISO-OSI model, I would not mix or force decoding and/to X-
Output in one API model and leave such (hw-) codecs in ffmpeg etc.
Fixes:
1. glx ext not detected by configure
crystalhd-video configuration summary:
VA-API version ... : 0.32.0
VA-API drivers path .. : /usr/lib/i386-linux-gnu/dri
CRYSTALHD version : "3"
GLX support .. : no <--- ?
# dpkg -L libva-dev |grep -i glx
/usr/lib/i386-linux-gnu/pkgconfig/libva-glx.pc
/usr/include/va/va_glx.h
/usr/include/va/va_backend_glx.h
/usr/lib/i386-linux-gnu/libva-glx.so
Debug? ... : yes
Tracer? .. : yes
2. struct VADriverVTable *vtable; is heap, not stack allocateable in API 0.32,
patch attached.
libtool: compile: gcc -DHAVE_CONFIG_H -I. -std=c99 -g -O2 -D__LINUX_USER__ -MT
crystalhd_drv_video.lo -MD -MP -MF .deps/crystalhd_drv_video.Tpo -c
crystalhd_drv_video.c -fPIC -DPIC -o .libs/crystalhd_drv_video.o
crystalhd_drv_video.c: In function '__vaDriverInit_0_31':
crystalhd_drv_video.c:1851:13: error: request for member 'vaTerminate' in
something not a structure or union
...
crystalhd_drv_video.c:1894:13: error: request for member 'vaBufferInfo' in
something not a structure or union
make[2]: *** [crystalhd_drv_video.lo] Fehler 1
3. test results:
init: vainfo:
Running DIL (3.22.0) Version
DtsDeviceOpen: Opening HW in mode 0
Clock set to 180
vainfo: VA-API version: 0.32 (libva 1.0.15)
vainfo: Driver version: Broadcom Crystal HD Video Decoder 3.10.0.0
vainfo: Supported profile and entrypoints
VAProfileMPEG2Simple:VAEntrypointVLD
VAProfileMPEG2Simple:VAEntrypointMoComp
VAProfileMPEG2Main :VAEntrypointVLD
VAProfileMPEG2Main :VAEntrypointMoComp
VAProfileH264Baseline :VAEntrypointVLD
VAProfileH264Main :VAEntrypointVLD
VAProfileH264High :VAEntrypointVLD
DtsAllocIoctlData Error
Manually linked to my GPU chipset name
ln -s /usr/lib/i386-linux-gnu/dri/crystalhd_drv_video.so
/usr/lib/i386-linux-gnu/dri/i915_drv_video.so
but nothing is accessing it it seems.
y
tom
-Att: patch against http://gitorious.org/crystalhd-video git master,HEAD at the
timestamp of file.
diff --git a/extra/crystalhd_wrapper/crystalhd_common.c b/extra/crystalhd_wrapper/crystalhd_common.c
index c26d5a5..e0c7eb0 100644
--- a/extra/crystalhd_wrapper/crystalhd_common.c
+++ b/extra/crystalhd_wrapper/crystalhd_common.c
@@ -48,7 +48,7 @@ const char *string_of_BC_STATUS(BC_STATUS status)
STATUS(CERT_VERIFY_ERROR);
STATUS(DEC_EXIST_OPEN);
STATUS(PENDING);
- STATUS(CLK_NOCHG);
+// STATUS(CLK_NOCHG);
STATUS(ERROR);
#undef STATUS
}
diff --git a/src/crystalhd_drv_video.c b/src/crystalhd_drv_video.c
index 5fbae33..8f99f6a 100644
--- a/src/crystalhd_drv_video.c
+++ b/src/crystalhd_drv_video.c
@@ -1828,7 +1828,7 @@ crystalhd_Terminate(
}
VAStatus
-__vaDriverInit_0_31(
+__vaDriverInit_0_32(
VADriverContextP ctx
)
{
@@ -1848,50 +1848,50 @@ __vaDriverInit_0_31(
ctx->max_display_attributes = CRYSTALHD_MAX_DISPLAY_ATTRIBUTES;
ctx->str_vendor = CRYSTALHD_STR_VENDOR;
- ctx->vtable.vaTerminate = crystalhd_Terminate;
- ctx->vtable.vaQueryConfigEntrypoints = crystalhd_QueryConfigEntrypoints;
- ctx->vtable.vaQueryConfigProfiles = crystalhd_QueryConfigProfiles;
- ctx->vtable.vaQueryConfigEntrypoints = crystalhd_QueryConfigEntrypoints;
- ctx->vtable.vaQueryConfigAttributes = crystalhd_QueryConfigAttributes;
- ctx->vtable.vaCreateConfig = crystalhd_CreateConfig;
- ctx->vtable.vaDestroyConfig = crystalhd_DestroyConfig;
- ctx->vtable.vaGetConfigAttributes = crystalhd_GetConfigAttributes;
- ctx->vtable.vaCreateSurfaces = crystalhd_CreateSurfaces;
- ctx->vtable.vaDestroySurfaces = crystalhd_DestroySurfaces;
- ctx->vtable.vaCreateContext = crystalhd_CreateContext;
- ctx->vtable.vaDestroyContext = crystalhd_DestroyContext;
- ctx->vtable.vaCreateBuffer = crystalhd_CreateBuffer;
- ctx->vtable.vaBufferSetNumElements = crystalhd_Buf
Re: [PATCH V3 09/15] [media] marvell-ccic: add get_mcam function for marvell-ccic driver
On Sun, 16 Dec 2012, Jonathan Corbet wrote:
> On Sat, 15 Dec 2012 17:57:58 +0800
> Albert Wang wrote:
>
> > This patch adds get_mcam() inline function which is prepared for
> > adding soc_camera support in marvell-ccic driver
>
> Time for a bikeshed moment: "get" generally is understood to mean
> incrementing a reference count in kernel code. Can it have a name like
> vbq_to_mcam() instead?
>
> Also:
>
> > @@ -1073,14 +1073,17 @@ static int mcam_vb_queue_setup(struct vb2_queue *vq,
> > static void mcam_vb_buf_queue(struct vb2_buffer *vb)
> > {
> > struct mcam_vb_buffer *mvb = vb_to_mvb(vb);
> > - struct mcam_camera *cam = vb2_get_drv_priv(vb->vb2_queue);
> > + struct mcam_camera *cam = get_mcam(vb->vb2_queue);
> > struct v4l2_pix_format *fmt = &cam->pix_format;
> > unsigned long flags;
> > int start;
> > dma_addr_t dma_handle;
> > + unsigned long size;
> > u32 pixel_count = fmt->width * fmt->height;
> >
> > spin_lock_irqsave(&cam->dev_lock, flags);
> > + size = vb2_plane_size(vb, 0);
> > + vb2_set_plane_payload(vb, 0, size);
> > dma_handle = vb2_dma_contig_plane_dma_addr(vb, 0);
> > BUG_ON(!dma_handle);
> > start = (cam->state == S_BUFWAIT) && !list_empty(&cam->buffers);
>
> There is an unrelated change here that belongs in a separate patch.
Right, agree.
Thanks
Guennadi
> > @@ -1138,9 +1141,12 @@ static void mcam_vb_wait_finish(struct vb2_queue *vq)
> > */
> > static int mcam_vb_start_streaming(struct vb2_queue *vq, unsigned int
> > count)
> > {
> > - struct mcam_camera *cam = vb2_get_drv_priv(vq);
> > + struct mcam_camera *cam = get_mcam(vq);
> > unsigned int frame;
> >
> > + if (count < 2)
> > + return -EINVAL;
> > +
>
> Here too - unrelated change.
>
> > if (cam->state != S_IDLE) {
> > INIT_LIST_HEAD(&cam->buffers);
> > return -EINVAL;
> > @@ -1170,7 +1176,7 @@ static int mcam_vb_start_streaming(struct vb2_queue
> > *vq, unsigned int count)
> >
> > static int mcam_vb_stop_streaming(struct vb2_queue *vq)
> > {
> > - struct mcam_camera *cam = vb2_get_drv_priv(vq);
> > + struct mcam_camera *cam = get_mcam(vq);
> > unsigned long flags;
> >
> > if (cam->state == S_BUFWAIT) {
> > @@ -1181,6 +1187,7 @@ static int mcam_vb_stop_streaming(struct vb2_queue
> > *vq)
> > if (cam->state != S_STREAMING)
> > return -EINVAL;
> > mcam_ctlr_stop_dma(cam);
> > + cam->state = S_IDLE;
>
> ...and also here ...
>
> jon
>
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V3 08/15] [media] marvell-ccic: switch to resource managed allocation and request
On Sat, 15 Dec 2012, Albert Wang wrote:
> From: Libin Yang
>
> This patch switchs to resource managed allocation and request in mmp-driver.
> It can remove free resource operations.
>
> Signed-off-by: Albert Wang
> Signed-off-by: Libin Yang
> ---
> drivers/media/platform/marvell-ccic/mmp-driver.c | 56
> --
> 1 file changed, 20 insertions(+), 36 deletions(-)
>
> diff --git a/drivers/media/platform/marvell-ccic/mmp-driver.c
> b/drivers/media/platform/marvell-ccic/mmp-driver.c
> index fec7cd8..40c243e 100755
> --- a/drivers/media/platform/marvell-ccic/mmp-driver.c
> +++ b/drivers/media/platform/marvell-ccic/mmp-driver.c
> @@ -315,7 +315,7 @@ static int mmpcam_probe(struct platform_device *pdev)
> if (!pdata)
> return -ENODEV;
>
> - cam = kzalloc(sizeof(*cam), GFP_KERNEL);
> + cam = devm_kzalloc(&pdev->dev, sizeof(*cam), GFP_KERNEL);
> if (cam == NULL)
> return -ENOMEM;
> cam->pdev = pdev;
> @@ -343,14 +343,12 @@ static int mmpcam_probe(struct platform_device *pdev)
> res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> if (res == NULL) {
> dev_err(&pdev->dev, "no iomem resource!\n");
> - ret = -ENODEV;
> - goto out_free;
> + return -ENODEV;
> }
> - mcam->regs = ioremap(res->start, resource_size(res));
> + mcam->regs = devm_request_and_ioremap(&pdev->dev, res);
> if (mcam->regs == NULL) {
> dev_err(&pdev->dev, "MMIO ioremap fail\n");
> - ret = -ENODEV;
> - goto out_free;
> + return -ENODEV;
> }
> /*
>* Power/clock memory is elsewhere; get it too. Perhaps this
> @@ -359,14 +357,12 @@ static int mmpcam_probe(struct platform_device *pdev)
> res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> if (res == NULL) {
> dev_err(&pdev->dev, "no power resource!\n");
> - ret = -ENODEV;
> - goto out_unmap1;
> + return -ENODEV;
> }
> - cam->power_regs = ioremap(res->start, resource_size(res));
> + cam->power_regs = devm_request_and_ioremap(&pdev->dev, res);
> if (cam->power_regs == NULL) {
> dev_err(&pdev->dev, "power MMIO ioremap fail\n");
> - ret = -ENODEV;
> - goto out_unmap1;
> + return -ENODEV;
> }
>
> mcam_init_clk(mcam, pdata, 1);
> @@ -376,25 +372,27 @@ static int mmpcam_probe(struct platform_device *pdev)
>*/
> mcam->i2c_adapter = platform_get_drvdata(pdata->i2c_device);
> if (mcam->i2c_adapter == NULL) {
> - ret = -ENODEV;
> dev_err(&pdev->dev, "No i2c adapter\n");
> - goto out_unmap2;
> + ret = -ENODEV;
> + goto out_uninit_clk;
Looks good in principle, but it will become a bit simpler yet after you
change your patch 03/15 to not use mcam_init_clk() for clock
deinitialisation, but that's going to be just a minor modification.
Thanks
Guennadi
> }
> /*
>* Sensor GPIO pins.
>*/
> - ret = gpio_request(pdata->sensor_power_gpio, "cam-power");
> + ret = devm_gpio_request(&pdev->dev, pdata->sensor_power_gpio,
> + "cam-power");
> if (ret) {
> dev_err(&pdev->dev, "Can't get sensor power gpio %d",
> pdata->sensor_power_gpio);
> - goto out_unmap2;
> + goto out_uninit_clk;
> }
> gpio_direction_output(pdata->sensor_power_gpio, 0);
> - ret = gpio_request(pdata->sensor_reset_gpio, "cam-reset");
> + ret = devm_gpio_request(&pdev->dev, pdata->sensor_reset_gpio,
> + "cam-reset");
> if (ret) {
> dev_err(&pdev->dev, "Can't get sensor reset gpio %d",
> pdata->sensor_reset_gpio);
> - goto out_gpio;
> + goto out_uninit_clk;
> }
> gpio_direction_output(pdata->sensor_reset_gpio, 0);
>
> @@ -404,7 +402,7 @@ static int mmpcam_probe(struct platform_device *pdev)
> mmpcam_power_up(mcam);
> ret = mccic_register(mcam);
> if (ret)
> - goto out_gpio2;
> + goto out_power_down;
> /*
>* Finally, set up our IRQ now that the core is ready to
>* deal with it.
> @@ -415,8 +413,8 @@ static int mmpcam_probe(struct platform_device *pdev)
> goto out_unregister;
> }
> cam->irq = res->start;
> - ret = request_irq(cam->irq, mmpcam_irq, IRQF_SHARED,
> - "mmp-camera", mcam);
> + ret = devm_request_irq(&pdev->dev, cam->irq, mmpcam_irq, IRQF_SHARED,
> + "mmp-camera", mcam);
> if (ret == 0) {
> mmpcam_add_device(cam);
> return 0;
> @@ -424,18 +422,10 @@ static int mmpcam_probe(struct platform_device *pdev)
>
> out_unregiste
