Re: [PATCH v2] media: imx208: Add imx208 camera sensor driver
On Tue, Jul 31, 2018 at 12:54 PM Chen, Ping-chung
wrote:
>
> Hi Tomasz,
>
> >-Original Message-
> > +/* Get bayer order based on flip setting. */ static __u32
> > +imx208_get_format_code(struct imx208 *imx208)
>
> >Why not just "u32"?
>
> Its return value will be assigned to the variable code which belongs to the
> structure
> v4l2_subdev_mbus_code_enum, and the type of this variable is __u32.
> struct v4l2_subdev_mbus_code_enum {
> __u32 pad;
> __u32 index;
> __u32 code;
> __u32 which;
> __u32 reserved[8];
> };
>
> > +{
> > + /*
> > +* Only one bayer order is supported.
> > +* It depends on the flip settings.
> > +*/
> > + static const __u32 codes[2][2] = {
>
> >Ditto.
>
> > + { MEDIA_BUS_FMT_SRGGB10_1X10, MEDIA_BUS_FMT_SGRBG10_1X10, },
> > + { MEDIA_BUS_FMT_SGBRG10_1X10, MEDIA_BUS_FMT_SBGGR10_1X10, },
> > + };
> > +
> > + return codes[imx208->vflip->val][imx208->hflip->val];
> > +}
> > +
That's okay. __u32 is an equivalent of u32 defined to be used in UAPI
headers. Inside of kernel-only code, u32 should be used.
Best regards,
Tomasz
RE: [PATCH v2] media: imx208: Add imx208 camera sensor driver
Hi Tomasz,
>-Original Message-
> +/* Get bayer order based on flip setting. */ static __u32
> +imx208_get_format_code(struct imx208 *imx208)
>Why not just "u32"?
Its return value will be assigned to the variable code which belongs to the
structure
v4l2_subdev_mbus_code_enum, and the type of this variable is __u32.
struct v4l2_subdev_mbus_code_enum {
__u32 pad;
__u32 index;
__u32 code;
__u32 which;
__u32 reserved[8];
};
> +{
> + /*
> +* Only one bayer order is supported.
> +* It depends on the flip settings.
> +*/
> + static const __u32 codes[2][2] = {
>Ditto.
> + { MEDIA_BUS_FMT_SRGGB10_1X10, MEDIA_BUS_FMT_SGRBG10_1X10, },
> + { MEDIA_BUS_FMT_SGBRG10_1X10, MEDIA_BUS_FMT_SBGGR10_1X10, },
> + };
> +
> + return codes[imx208->vflip->val][imx208->hflip->val];
> +}
> +
cron job: media_tree daily build: WARNINGS
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: Tue Jul 31 05:00:10 CEST 2018 media-tree git hash:1d06352e18ef502e30837cedfe618298816fb48c media_build git hash: e75fbc93fc427f769c3ce5a020bf204e02a45852 v4l-utils git hash: 5583f43ef1a4814c2bd3c43cb06461b7f532b141 edid-decode git hash: ab18befbcacd6cd4dff63faa82e32700369d6f25 gcc version:i686-linux-gcc (GCC) 8.1.0 sparse version: 0.5.2 smatch version: 0.5.1 host hardware: x86_64 host os:4.16.0-1-amd64 linux-git-arm-at91: OK linux-git-arm-davinci: OK linux-git-arm-multi: OK linux-git-arm-pxa: OK linux-git-arm-stm32: OK linux-git-arm64: OK linux-git-i686: WARNINGS linux-git-mips: OK linux-git-powerpc64: OK linux-git-sh: OK linux-git-x86_64: WARNINGS Check COMPILE_TEST: OK linux-2.6.36.4-i686: OK linux-2.6.36.4-x86_64: OK linux-2.6.37.6-i686: OK linux-2.6.37.6-x86_64: OK linux-2.6.38.8-i686: OK linux-2.6.38.8-x86_64: OK linux-2.6.39.4-i686: OK linux-2.6.39.4-x86_64: OK linux-3.0.101-i686: OK linux-3.0.101-x86_64: OK linux-3.1.10-i686: OK linux-3.1.10-x86_64: OK linux-3.2.102-i686: OK linux-3.2.102-x86_64: OK linux-3.3.8-i686: OK linux-3.3.8-x86_64: OK linux-3.4.113-i686: OK linux-3.4.113-x86_64: OK linux-3.5.7-i686: OK linux-3.5.7-x86_64: OK linux-3.6.11-i686: OK linux-3.6.11-x86_64: OK linux-3.7.10-i686: OK linux-3.7.10-x86_64: OK linux-3.8.13-i686: OK linux-3.8.13-x86_64: OK linux-3.9.11-i686: OK linux-3.9.11-x86_64: OK linux-3.10.108-i686: OK linux-3.10.108-x86_64: OK linux-3.11.10-i686: OK linux-3.11.10-x86_64: OK linux-3.12.74-i686: OK linux-3.12.74-x86_64: OK linux-3.13.11-i686: OK linux-3.13.11-x86_64: OK linux-3.14.79-i686: OK linux-3.14.79-x86_64: OK linux-3.15.10-i686: OK linux-3.15.10-x86_64: OK linux-3.16.57-i686: OK linux-3.16.57-x86_64: OK linux-3.17.8-i686: OK linux-3.17.8-x86_64: OK linux-3.18.115-i686: OK linux-3.18.115-x86_64: OK linux-3.19.8-i686: OK linux-3.19.8-x86_64: OK linux-4.0.9-i686: OK linux-4.0.9-x86_64: OK linux-4.1.52-i686: OK linux-4.1.52-x86_64: OK linux-4.2.8-i686: OK linux-4.2.8-x86_64: OK linux-4.3.6-i686: OK linux-4.3.6-x86_64: OK linux-4.4.140-i686: OK linux-4.4.140-x86_64: OK linux-4.5.7-i686: OK linux-4.5.7-x86_64: OK linux-4.6.7-i686: OK linux-4.6.7-x86_64: OK linux-4.7.10-i686: OK linux-4.7.10-x86_64: OK linux-4.8.17-i686: OK linux-4.8.17-x86_64: OK linux-4.9.112-i686: OK linux-4.9.112-x86_64: OK linux-4.10.17-i686: OK linux-4.10.17-x86_64: OK linux-4.11.12-i686: OK linux-4.11.12-x86_64: OK linux-4.12.14-i686: OK linux-4.12.14-x86_64: OK linux-4.13.16-i686: OK linux-4.13.16-x86_64: OK linux-4.14.55-i686: OK linux-4.14.55-x86_64: OK linux-4.15.18-i686: OK linux-4.15.18-x86_64: OK linux-4.16.18-i686: OK linux-4.16.18-x86_64: OK linux-4.17.6-i686: OK linux-4.17.6-x86_64: OK linux-4.18-rc4-i686: OK linux-4.18-rc4-x86_64: OK apps: OK spec-git: OK sparse: WARNINGS Detailed results are available here: http://www.xs4all.nl/~hverkuil/logs/Tuesday.log Full logs are available here: http://www.xs4all.nl/~hverkuil/logs/Tuesday.tar.bz2 The Media Infrastructure API from this daily build is here: http://www.xs4all.nl/~hverkuil/spec/index.html
RE: [PATCH v2] media: imx208: Add imx208 camera sensor driver
Hi Sakari, We will test the new flow on Atlas. If everything is fine we will merge the code into patch v3 or v4. Thanks, PC Chen -Original Message- From: Sakari Ailus [mailto:sakari.ai...@linux.intel.com] Sent: Monday, July 30, 2018 6:39 PM To: Chen, Ping-chung Cc: linux-media@vger.kernel.org; Yeh, Andy ; Lai, Jim ; tf...@google.com; grund...@chromium.org; Mani, Rajmohan Subject: Re: [PATCH v2] media: imx208: Add imx208 camera sensor driver Hi Ping-chung, On Mon, Jul 30, 2018 at 05:26:39PM +0800, Ping-chung Chen wrote: > From: "Chen, Ping-chung" > > Add a V4L2 sub-device driver for the Sony IMX208 image sensor. > This is a camera sensor using the I2C bus for control and the > CSI-2 bus for data. > > Signed-off-by: Ping-Chung Chen Could you add obtaining the valid link frequencies from the firmware, i.e. use v4l2_fwnode_endpoint_alloc_parse()? -- Kind regards, Sakari Ailus sakari.ai...@linux.intel.com
Re: [PATCH 06/22] [media] tvp5150: add FORMAT_TRY support for get/set selection handlers
Em Thu, 28 Jun 2018 18:20:38 +0200
Marco Felsch escreveu:
> Since commit 10d5509c8d50 ("[media] v4l2: remove g/s_crop from video ops")
> the 'which' field for set/get_selection must be FORMAT_ACTIVE. There is
> no way to try different selections. The patch adds a helper function to
> select the correct selection memory space (sub-device file handle or
> driver state) which will be set/returned.
>
> The TVP5150 AVID will be updated if the 'which' field is FORMAT_ACTIVE
> and the requested selection rectangle differs from the already set one.
>
> Signed-off-by: Marco Felsch
> ---
> drivers/media/i2c/tvp5150.c | 107
> 1 file changed, 73 insertions(+), 34 deletions(-)
>
> diff --git a/drivers/media/i2c/tvp5150.c b/drivers/media/i2c/tvp5150.c
> index d150487cc2d1..29eaf8166f25 100644
> --- a/drivers/media/i2c/tvp5150.c
> +++ b/drivers/media/i2c/tvp5150.c
> @@ -18,6 +18,7 @@
> #include
> #include
> #include
> +#include
>
> #include "tvp5150_reg.h"
>
> @@ -846,20 +847,38 @@ static v4l2_std_id tvp5150_read_std(struct v4l2_subdev
> *sd)
> }
> }
>
> +static struct v4l2_rect *
> +__tvp5150_get_pad_crop(struct tvp5150 *decoder,
> +struct v4l2_subdev_pad_config *cfg, unsigned int pad,
> +enum v4l2_subdev_format_whence which)
> +{
> + switch (which) {
> + case V4L2_SUBDEV_FORMAT_TRY:
> + return v4l2_subdev_get_try_crop(>sd, cfg, pad);
This is not ok. It causes compilation breakage if the subdev API is not
selected:
drivers/media/i2c/tvp5150.c: In function ‘__tvp5150_get_pad_crop’:
drivers/media/i2c/tvp5150.c:857:10: error: implicit declaration of function
‘v4l2_subdev_get_try_crop’; did you mean ‘v4l2_subdev_has_op’?
[-Werror=implicit-function-declaration]
return v4l2_subdev_get_try_crop(>sd, cfg, pad);
^~~~
v4l2_subdev_has_op
drivers/media/i2c/tvp5150.c:857:10: warning: returning ‘int’ from a function
with return type ‘struct v4l2_rect *’ makes pointer from integer without a cast
[-Wint-conversion]
return v4l2_subdev_get_try_crop(>sd, cfg, pad);
^~~~
The logic should keep working both with and without subdev API.
> + case V4L2_SUBDEV_FORMAT_ACTIVE:
> + return >rect;
> + default:
> + return NULL;
> + }
> +}
> +
> static int tvp5150_fill_fmt(struct v4l2_subdev *sd,
> struct v4l2_subdev_pad_config *cfg,
> struct v4l2_subdev_format *format)
> {
> struct v4l2_mbus_framefmt *f;
> + struct v4l2_rect *__crop;
> struct tvp5150 *decoder = to_tvp5150(sd);
>
> if (!format || (format->pad != DEMOD_PAD_VID_OUT))
> return -EINVAL;
>
> f = >format;
> + __crop = __tvp5150_get_pad_crop(decoder, cfg, format->pad,
> + format->which);
>
> - f->width = decoder->rect.width;
> - f->height = decoder->rect.height / 2;
> + f->width = __crop->width;
> + f->height = __crop->height / 2;
>
> f->code = MEDIA_BUS_FMT_UYVY8_2X8;
> f->field = V4L2_FIELD_ALTERNATE;
> @@ -870,17 +889,51 @@ static int tvp5150_fill_fmt(struct v4l2_subdev *sd,
> return 0;
> }
>
> +unsigned int tvp5150_get_hmax(struct v4l2_subdev *sd)
> +{
> + struct tvp5150 *decoder = to_tvp5150(sd);
> + v4l2_std_id std;
> +
> + /* Calculate height based on current standard */
> + if (decoder->norm == V4L2_STD_ALL)
> + std = tvp5150_read_std(sd);
> + else
> + std = decoder->norm;
> +
> + return (std & V4L2_STD_525_60) ?
> + TVP5150_V_MAX_525_60 : TVP5150_V_MAX_OTHERS;
> +}
> +
> +static inline void
> +__tvp5150_set_selection(struct v4l2_subdev *sd, struct v4l2_rect rect)
> +{
> + struct tvp5150 *decoder = to_tvp5150(sd);
> + unsigned int hmax = tvp5150_get_hmax(sd);
> +
> + regmap_write(decoder->regmap, TVP5150_VERT_BLANKING_START, rect.top);
> + regmap_write(decoder->regmap, TVP5150_VERT_BLANKING_STOP,
> + rect.top + rect.height - hmax);
> + regmap_write(decoder->regmap, TVP5150_ACT_VD_CROP_ST_MSB,
> + rect.left >> TVP5150_CROP_SHIFT);
> + regmap_write(decoder->regmap, TVP5150_ACT_VD_CROP_ST_LSB,
> + rect.left | (1 << TVP5150_CROP_SHIFT));
> + regmap_write(decoder->regmap, TVP5150_ACT_VD_CROP_STP_MSB,
> + (rect.left + rect.width - TVP5150_MAX_CROP_LEFT) >>
> + TVP5150_CROP_SHIFT);
> + regmap_write(decoder->regmap, TVP5150_ACT_VD_CROP_STP_LSB,
> + rect.left + rect.width - TVP5150_MAX_CROP_LEFT);
> +}
> +
> static int tvp5150_set_selection(struct v4l2_subdev *sd,
>struct v4l2_subdev_pad_config *cfg,
>struct v4l2_subdev_selection *sel)
> {
> struct tvp5150 *decoder = to_tvp5150(sd);
> struct v4l2_rect
Re: [PATCH v3 0/5] Add BPF decoders to ir-keytable
Hi Sean, On Sat, Jul 28, 2018 at 10:29:31AM +0100, Sean Young wrote: > Hi Hias, > > On Sat, Jul 21, 2018 at 08:13:27PM +0200, Matthias Reichl wrote: > > Hi Sean, > > > > thanks a lot, this is a really nice new feature! > > Thank you for testing it and finding all those issues, it has become much > better from your testing. > > > On Fri, Jul 13, 2018 at 03:30:06PM +0100, Sean Young wrote: > > > Once kernel v4.18 is released with IR BPF decoding, this can be merged > > > to v4l-utils. > > > > > > The idea is that IR decoders can be written in C, compiled to BPF > > > relocatable > > > object file. Any global variables can overriden, so we can supports lots > > > of variants of similiar protocols (just like in the lircd.conf file). > > > > > > The existing rc_keymap file format can't be used for variables, so I've > > > converted the format to toml. An alternative would be to use the existing > > > lircd.conf file format, but it's a very awkward file to parse in C and it > > > contains many features which are irrelevant to us. > > > > > > We use libelf to load the bpf relocatable object file. > > > > > > After loading our example grundig keymap with bpf decoder, the output of > > > ir-keytable is: > > > > > > Found /sys/class/rc/rc0/ (/dev/input/event8) with: > > > Name: Winbond CIR > > > Driver: winbond-cir, table: rc-rc6-mce > > > LIRC device: /dev/lirc0 > > > Attached BPF protocols: grundig > > > Supported kernel protocols: lirc rc-5 rc-5-sz jvc sony nec sanyo > > > mce_kbd rc-6 sharp xmp imon > > > Enabled protocols: lirc > > > bus: 25, vendor/product: 10ad:00f1, version: 0x0004 > > > Repeat delay = 500 ms, repeat period = 125 ms > > > > > > Alternatively, you simply specify the path to the object file on the > > > command > > > line: > > > > > > $ ir-keytable -e header_pulse=9000,header_space=4500 -p ./pulse_distance.o > > > > > > Derek, please note that you can now convert the dish lircd.conf to toml > > > and load the keymap; it should just work. It would be great to have your > > > feedback, thank you. > > > > I did a few tests with one of my RC-5 remotes, this lircd.conf file > > https://github.com/PiSupply/JustBoom/blob/master/LIRC/lircd.conf > > and kernel 4.18-rc5 on RPi2, with the 32bit ARM kernel and > > gpio-ir-recv, and on LePotato / aarch64 with meson-ir. > > > > lircd2toml.py did a really good job on converting it, the only > > thing missing was the toggle_bit. > > Right, there was a bug in lirc2html.py. I've added a fix to my bpf branch: > > https://git.linuxtv.org/syoung/v4l-utils.git/log/?h=bpf Thanks a lot, lircd2toml.py now also seems to detect that the lircd.conf uses the rc-5 protocol and uses the rc-5 decoder - that's really nice! > > When testing the converted toml (with "toggle_bit = 11" added > > and the obvious volume keycode fixes) I noticed a couple of issues: > > > > Buttons seem to be "stuck". The scancode is decoded, key_down > > event is generated, but after release the key_down events repeat > > indefinitely - with the built-in rc-5 decoder this works fine. > > > > root@upstream:/home/hias/ir-test# ir-keytable -c -w justboom.toml -t > > Old keytable cleared > > Wrote 12 keycode(s) to driver > > Protocols changed to > > Loaded BPF protocol manchester > > Testing events. Please, press CTRL-C to abort. > > 29.065820: lirc protocol(66): scancode = 0x141b > > 29.065890: event type EV_MSC(0x04): scancode = 0x141b > > 29.065890: event type EV_KEY(0x01) key_down: KEY_DOWN(0x006c) > > 29.065890: event type EV_SYN(0x00). > > 29.570059: event type EV_KEY(0x01) key_down: KEY_DOWN(0x006c) > > 29.570059: event type EV_SYN(0x00). > > 29.710062: event type EV_KEY(0x01) key_down: KEY_DOWN(0x006c) > > 29.710062: event type EV_SYN(0x00). > > 29.850057: event type EV_KEY(0x01) key_down: KEY_DOWN(0x006c) > > 29.850057: event type EV_SYN(0x00). > > 29.990057: event type EV_KEY(0x01) key_down: KEY_DOWN(0x006c) > > 29.990057: event type EV_SYN(0x00). > > 30.130055: event type EV_KEY(0x01) key_down: KEY_DOWN(0x006c) > > 30.130055: event type EV_SYN(0x00). > > ... > > Thanks, I had not seen this yet either. There is a fix here: > > https://www.mail-archive.com/linux-media@vger.kernel.org/msg133813.html Thanks, the patch seems to fix it, I posted a small nit in the thread. > > Even scancodes, eg KEY_UP / scancode 0x141a, aren't decoded at > > all, only odd scancodes work. My guess is the manchester decoder > > could have a problem when the last bit is zero and the message > > doesn't end with a pulse, but a (rather long) timeout. > > Yep, yet another bug! I'v added a fix here: > > https://git.linuxtv.org/syoung/v4l-utils.git/log/?h=bpf Thanks, using the manchester decoder with rc-5 looks fine now! > > (Re-)loading a bpf decoder only works 8 times. The 9th attempt > > gives an error message. > > > > # for i in `seq 1 9` ; do ir-keytable -p manchester ; done > > Protocols changed to > > Loaded BPF
Re: [PATCH] media: rc: read out of bounds if bpf reports high protocol number
Hi Sean,
On Sat, Jul 28, 2018 at 10:11:15AM +0100, Sean Young wrote:
> The repeat period is read from a static array. If a keydown event is
> reported from bpf with a high protocol number, we read out of bounds. This
> is unlikely to end up with a reasonable repeat period at the best of times,
> in which case no timely key up event is generated.
>
> Signed-off-by: Sean Young
> ---
> drivers/media/rc/rc-main.c | 12 ++--
> 1 file changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/media/rc/rc-main.c b/drivers/media/rc/rc-main.c
> index 2e222d9ee01f..a24850be1f4f 100644
> --- a/drivers/media/rc/rc-main.c
> +++ b/drivers/media/rc/rc-main.c
> @@ -679,6 +679,14 @@ static void ir_timer_repeat(struct timer_list *t)
> spin_unlock_irqrestore(>keylock, flags);
> }
>
> +unsigned int repeat_period(int protocol)
> +{
> + if (protocol >= ARRAY_SIZE(protocols))
> + return 100;
100 seems a bit arbitrarily chosen to me. Wouldn't it be better to
(re-)use eg protocols[RC_PROTO_UNKNOWN].repeat_period here?
so long,
Hias
> +
> + return protocols[protocol].repeat_period;
> +}
> +
> /**
> * rc_repeat() - signals that a key is still pressed
> * @dev: the struct rc_dev descriptor of the device
> @@ -691,7 +699,7 @@ void rc_repeat(struct rc_dev *dev)
> {
> unsigned long flags;
> unsigned int timeout = nsecs_to_jiffies(dev->timeout) +
> - msecs_to_jiffies(protocols[dev->last_protocol].repeat_period);
> + msecs_to_jiffies(repeat_period(dev->last_protocol));
> struct lirc_scancode sc = {
> .scancode = dev->last_scancode, .rc_proto = dev->last_protocol,
> .keycode = dev->keypressed ? dev->last_keycode : KEY_RESERVED,
> @@ -803,7 +811,7 @@ void rc_keydown(struct rc_dev *dev, enum rc_proto
> protocol, u32 scancode,
>
> if (dev->keypressed) {
> dev->keyup_jiffies = jiffies + nsecs_to_jiffies(dev->timeout) +
> - msecs_to_jiffies(protocols[protocol].repeat_period);
> + msecs_to_jiffies(repeat_period(protocol));
> mod_timer(>timer_keyup, dev->keyup_jiffies);
> }
> spin_unlock_irqrestore(>keylock, flags);
> --
> 2.17.1
>
Re: [PATCH 19/22] [media] tvp5150: add input source selection of_graph support
Em Thu, 28 Jun 2018 18:20:51 +0200
Marco Felsch escreveu:
> The currrent driver layout had the following layout:
>++
> +---+ |TVP5150 |
> | Comp0 +--+ ||
> +---+ | | +-+
> +---+ | +--+ | Src |
> | Comp1 +--+--|Sink | +-+
> +---+ | +--+ +-+
> ++ | | | Src |
> | SVideo +--+ | +-+
> ++ ++
>
> Since the device tree abstracts the real hardware this layout is not
> correct, because the TVP5150 has 3 physical ports (2 input, 1 output).
> Furthermore this layout assumes that there is an additional external mux
> in front of the TVP5150. This is not correct because the TVP5150 does
> the muxing work. The corresponding of_graph layout will look like:
> tvp5150 {
>
> port {
> reg = <0>;
> endpoint@0 {...};
> endpoint@1 {...};
> endpoint@2 {...};
> };
>
> };
>
> This patch change the layout to:
> ++
> |TVP5150 |
> +---+ +--+ |
> | Comp0 +---+ Sink | |
> +---+ +--+ |
> +---+ +--+ +-+
> | Comp1 +---+ Sink | | Src |
> +---+ +--+ +-+
> ++ +--+ |
> | SVideo +---+ Sink | |
> ++ +--+ |
> ++
>
> To keep things easy an additional 'virtual' S-Video port is added. More
> information about the port mapping can be found in the device tree
> binding documentation. The connector entities Comp0/1, SVideo are created
> only if they are connected to the correct port. If more than one connector
> is available the media_entity_operations.link_setup() callback ensures that
> only one connector is active. To change the input src the link between
> the TVP5150 pad and the connector must be disabled, then a new link can
> be enabled.
>
> The patch tries to reduce the '#ifdef CONFIG_MEDIA_CONTROLLER' usage to
> a minimum.
>
> Signed-off-by: Marco Felsch
> ---
> drivers/media/i2c/tvp5150.c | 322
> 1 file changed, 287 insertions(+), 35 deletions(-)
>
> diff --git a/drivers/media/i2c/tvp5150.c b/drivers/media/i2c/tvp5150.c
> index a6fec569a610..6ac29c62d99b 100644
> --- a/drivers/media/i2c/tvp5150.c
> +++ b/drivers/media/i2c/tvp5150.c
> @@ -44,10 +44,30 @@ MODULE_PARM_DESC(debug, "Debug level (0-2)");
>
> #define dprintk0(__dev, __arg...) dev_dbg_lvl(__dev, 0, 0, __arg)
>
> +enum tvp5150_ports {
> + TVP5150_PORT_AIP1A = TVP5150_COMPOSITE0,
> + TVP5150_PORT_AIP1B,
> + /* s-video port is a virtual port */
> + TVP5150_PORT_SVIDEO,
> + TVP5150_PORT_YOUT,
> + TVP5150_PORT_NUM,
> +};
> +
> +#ifdef CONFIG_MEDIA_CONTROLLER
> +struct tvp5150_connector {
> + struct media_entity con;
> + struct media_pad pad;
> + unsigned int port_num;
> +};
> +#endif
> +
> struct tvp5150 {
> struct v4l2_subdev sd;
> + struct device_node *endpoints[TVP5150_PORT_NUM];
> #ifdef CONFIG_MEDIA_CONTROLLER
> - struct media_pad pads[DEMOD_NUM_PADS];
> + struct media_pad pads[TVP5150_PORT_NUM];
This will cause problems with the current code.
When we designed the MC version 2 code, the idea were to allow
set properties to entities and to the inputs, but the code
was never submitted upstream.
A decoder may have several different types of inputs and outputs.
Several designs allow using different types of decoders, being
saa711x and tvp5150 the most popular ones. Well, depending on
the device, the number of PADs and the signals they carry can
be different.
Without a way to "taint" a pad to the signal it contains,
while waiting for the properties API, we added a code that
"fixed" the PADs to a certain number. This way, Kernelspace could
use the pad "number" as a way to identify the type of signal a
PAD carries.
The PC consumer drivers use those numbers in order to build the
MC graph[1].
A change on this would require adding a property to the pad, in
order to indicate the type of signal it provides (RF, luminance IF,
chroma IF, audio IF, I2S audio, ...), and to change
v4l2_mc_create_media_graph() accordingly.
[1] See drivers/media/v4l2-core/v4l2-mc.c at v4l2_mc_create_media_graph() func.
> + struct tvp5150_connector *connectors;
> + int active;
> #endif
> struct v4l2_ctrl_handler hdl;
> struct v4l2_rect rect;
> @@ -990,7 +1010,7 @@ static int tvp5150_fill_fmt(struct v4l2_subdev *sd,
> struct v4l2_rect *__crop;
> struct tvp5150 *decoder = to_tvp5150(sd);
>
> - if (!format || (format->pad != DEMOD_PAD_VID_OUT))
> + if (!format || (format->pad != TVP5150_PORT_YOUT))
> return -EINVAL;
>
> f = >format;
> @@ -1189,6 +1209,62 @@ static int tvp5150_enum_frame_size(struct
Re: [PATCH 18/22] partial revert of "[media] tvp5150: add HW input connectors support"
Em Thu, 28 Jun 2018 18:20:50 +0200
Marco Felsch escreveu:
> From: Javier Martinez Canillas
>
> Commit f7b4b54e6364 ("[media] tvp5150: add HW input connectors support")
> added input signals support for the tvp5150, but the approach was found
> to be incorrect so the corresponding DT binding commit 82c2ffeb217a
> ("[media] tvp5150: document input connectors DT bindings") was reverted.
>
> This left the driver with an undocumented (and wrong) DT parsing logic,
> so lets get rid of this code as well until the input connectors support
> is implemented properly.
>
> It's a partial revert due other patches added on top of mentioned commit
> not allowing the commit to be reverted cleanly anymore. But all the code
> related to the DT parsing logic and input entities creation are removed.
>
> Suggested-by: Laurent Pinchart
> Signed-off-by: Javier Martinez Canillas
> Acked-by: Laurent Pinchart
> [m.fel...@pengutronix.de: rm TVP5150_INPUT_NUM define]
> Signed-off-by: Marco Felsch
> ---
...
> -static int tvp5150_registered(struct v4l2_subdev *sd)
> -{
> -#ifdef CONFIG_MEDIA_CONTROLLER
> - struct tvp5150 *decoder = to_tvp5150(sd);
> - int ret = 0;
> - int i;
> -
> - for (i = 0; i < TVP5150_INPUT_NUM; i++) {
> - struct media_entity *input = >input_ent[i];
> - struct media_pad *pad = >input_pad[i];
> -
> - if (!input->name)
> - continue;
> -
> - decoder->input_pad[i].flags = MEDIA_PAD_FL_SOURCE;
> -
> - ret = media_entity_pads_init(input, 1, pad);
> - if (ret < 0)
> - return ret;
> -
> - ret = media_device_register_entity(sd->v4l2_dev->mdev, input);
> - if (ret < 0)
> - return ret;
> -
> - ret = media_create_pad_link(input, 0, >entity,
> - DEMOD_PAD_IF_INPUT, 0);
> - if (ret < 0) {
> - media_device_unregister_entity(input);
> - return ret;
> - }
> - }
> -#endif
Hmm... I suspect that reverting this part may cause problems for drivers
like em28xx when compiled with MC, as they rely that the supported demods
will have 3 pads (DEMOD_NUM_PADS).
Thanks,
Mauro
Re: [PATCH 16/22] [media] tvp5150: add querystd
Em Thu, 28 Jun 2018 18:20:48 +0200
Marco Felsch escreveu:
> From: Philipp Zabel
>
> Add the querystd video_op and make it return V4L2_STD_UNKNOWN while the
> TVP5150 is not locked to a signal.
>
> Signed-off-by: Philipp Zabel
> Signed-off-by: Marco Felsch
> ---
> drivers/media/i2c/tvp5150.c | 10 ++
> 1 file changed, 10 insertions(+)
>
> diff --git a/drivers/media/i2c/tvp5150.c b/drivers/media/i2c/tvp5150.c
> index 99d887936ea0..1990aaa17749 100644
> --- a/drivers/media/i2c/tvp5150.c
> +++ b/drivers/media/i2c/tvp5150.c
> @@ -796,6 +796,15 @@ static v4l2_std_id tvp5150_read_std(struct v4l2_subdev
> *sd)
> }
> }
>
> +static int tvp5150_querystd(struct v4l2_subdev *sd, v4l2_std_id *std_id)
> +{
> + struct tvp5150 *decoder = to_tvp5150(sd);
> +
> + *std_id = decoder->lock ? tvp5150_read_std(sd) : V4L2_STD_UNKNOWN;
This patch requires rework. What happens when a device doesn't have
IRQ enabled? Perhaps it should, instead, read some register in order
to check for the locking status, as this would work on both cases.
> +
> + return 0;
> +}
> +
> static const struct v4l2_event tvp5150_ev_fmt = {
> .type = V4L2_EVENT_SOURCE_CHANGE,
> .u.src_change.changes = V4L2_EVENT_SRC_CH_RESOLUTION,
> @@ -1408,6 +1417,7 @@ static const struct v4l2_subdev_tuner_ops
> tvp5150_tuner_ops = {
>
> static const struct v4l2_subdev_video_ops tvp5150_video_ops = {
> .s_std = tvp5150_s_std,
> + .querystd = tvp5150_querystd,
> .s_stream = tvp5150_s_stream,
> .s_routing = tvp5150_s_routing,
> .g_mbus_config = tvp5150_g_mbus_config,
Thanks,
Mauro
Re: [PATCH 13/22] [media] tvp5150: disable output while signal not locked
Em Mon, 30 Jul 2018 15:00:58 -0300
Mauro Carvalho Chehab escreveu:
> Em Thu, 28 Jun 2018 18:20:45 +0200
> Marco Felsch escreveu:
>
> > From: Philipp Zabel
> >
> > To avoid short frames on stream start, keep output pins at high impedance
> > while we are not properly locked onto the input signal.
> >
> > Signed-off-by: Philipp Zabel
> > Signed-off-by: Marco Felsch
> > ---
> > drivers/media/i2c/tvp5150.c | 39 ++---
> > 1 file changed, 28 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/media/i2c/tvp5150.c b/drivers/media/i2c/tvp5150.c
> > index d8bdbedd8826..27cfd08be3d2 100644
> > --- a/drivers/media/i2c/tvp5150.c
> > +++ b/drivers/media/i2c/tvp5150.c
> > @@ -60,6 +60,7 @@ struct tvp5150 {
> > v4l2_std_id detected_norm;
> > u32 input;
> > u32 output;
> > + u32 oe;
> > int enable;
> > bool lock;
> >
> > @@ -799,14 +800,20 @@ static irqreturn_t tvp5150_isr(int irq, void *dev_id)
> > {
> > struct tvp5150 *decoder = dev_id;
> > struct regmap *map = decoder->regmap;
> > - unsigned int active = 0, status = 0;
> > + unsigned int mask, active = 0, status = 0;
> > +
> > + mask = TVP5150_MISC_CTL_YCBCR_OE | TVP5150_MISC_CTL_SYNC_OE |
> > + TVP5150_MISC_CTL_CLOCK_OE;
> >
> > regmap_read(map, TVP5150_INT_STATUS_REG_A, );
> > if (status) {
> > regmap_write(map, TVP5150_INT_STATUS_REG_A, status);
> >
> > - if (status & TVP5150_INT_A_LOCK)
> > + if (status & TVP5150_INT_A_LOCK) {
> > decoder->lock = !!(status & TVP5150_INT_A_LOCK_STATUS);
> > + regmap_update_bits(map, TVP5150_MISC_CTL, mask,
> > + decoder->lock ? decoder->oe : 0);
> > + }
> >
> > return IRQ_HANDLED;
> > }
> > @@ -872,10 +879,26 @@ static int tvp5150_enable(struct v4l2_subdev *sd)
> > /* Disable autoswitch mode */
> > tvp5150_set_std(sd, std);
> >
> > - if (decoder->mbus_type == V4L2_MBUS_PARALLEL)
> > + /*
> > +* Enable the YCbCr and clock outputs. In discrete sync mode
> > +* (non-BT.656) additionally enable the the sync outputs.
> > +*/
> > + switch (decoder->mbus_type) {
> > + case V4L2_MBUS_PARALLEL:
> > /* 8-bit 4:2:2 YUV with discrete sync output */
> > regmap_update_bits(decoder->regmap, TVP5150_DATA_RATE_SEL,
> >0x7, 0x0);
> > + decoder->oe = TVP5150_MISC_CTL_YCBCR_OE |
> > + TVP5150_MISC_CTL_CLOCK_OE |
> > + TVP5150_MISC_CTL_SYNC_OE;
> > + break;
> > + case V4L2_MBUS_BT656:
> > + decoder->oe = TVP5150_MISC_CTL_YCBCR_OE |
> > + TVP5150_MISC_CTL_CLOCK_OE;
> > + break;
> > + default:
> > + return -EINVAL;
> > + }
> >
> > return 0;
> > };
> > @@ -1190,14 +1213,8 @@ static int tvp5150_s_stream(struct v4l2_subdev *sd,
> > int enable)
> > if (enable) {
> > tvp5150_enable(sd);
> >
> > - /*
> > -* Enable the YCbCr and clock outputs. In discrete sync mode
> > -* (non-BT.656) additionally enable the the sync outputs.
> > -*/
> > - val = TVP5150_MISC_CTL_YCBCR_OE | TVP5150_MISC_CTL_CLOCK_OE;
> > - if (decoder->mbus_type == V4L2_MBUS_PARALLEL)
> > - val |= TVP5150_MISC_CTL_SYNC_OE;
> > -
> > + /* Enable outputs if decoder is locked */
> > + val = decoder->lock ? decoder->oe : 0;
>
> Hmm... this only works if the tvp5150 has the interrupts enabled.
>
> The code should be, instead:
>
> if (c->irq)
> val = decoder->lock ? decoder->oe : 0;
> else
> val = decoder->oe;
>
>
> > int_val = TVP5150_INT_A_LOCK;
> > }
> >
In other words, I believe we need the enclosed patch to avoid causing
regressions on existing drivers.
I intend to run a test here with this tvp5150 series before merging,
in order to be sure that em28xx+tvp5150 devices will keep working.
Regards,
Mauro
[PATCH] tvp5150: if IRQ is not enabled, don't disable inputs at s_stream
The new IRQ logic sets decoder->lock if signal is locked.
That should work fine when IRQ is used, but not all tvp5150
clients set it.
So, keep the previous logic if IRQ is not enabled for tvp5150.
Signed-off-by: Mauro Carvalho Chehab
diff --git a/drivers/media/i2c/tvp5150.c b/drivers/media/i2c/tvp5150.c
index f7c9203a3923..777e0e16787e 100644
--- a/drivers/media/i2c/tvp5150.c
+++ b/drivers/media/i2c/tvp5150.c
@@ -1214,7 +1214,10 @@ static int tvp5150_s_stream(struct v4l2_subdev *sd, int
enable)
tvp5150_enable(sd);
/* Enable outputs if decoder is locked */
- val = decoder->lock ? decoder->oe : 0;
+ if (decoder->irq)
+ val = decoder->lock ? decoder->oe : 0;
+ else
+
Re: [PATCH 13/22] [media] tvp5150: disable output while signal not locked
Em Thu, 28 Jun 2018 18:20:45 +0200
Marco Felsch escreveu:
> From: Philipp Zabel
>
> To avoid short frames on stream start, keep output pins at high impedance
> while we are not properly locked onto the input signal.
>
> Signed-off-by: Philipp Zabel
> Signed-off-by: Marco Felsch
> ---
> drivers/media/i2c/tvp5150.c | 39 ++---
> 1 file changed, 28 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/media/i2c/tvp5150.c b/drivers/media/i2c/tvp5150.c
> index d8bdbedd8826..27cfd08be3d2 100644
> --- a/drivers/media/i2c/tvp5150.c
> +++ b/drivers/media/i2c/tvp5150.c
> @@ -60,6 +60,7 @@ struct tvp5150 {
> v4l2_std_id detected_norm;
> u32 input;
> u32 output;
> + u32 oe;
> int enable;
> bool lock;
>
> @@ -799,14 +800,20 @@ static irqreturn_t tvp5150_isr(int irq, void *dev_id)
> {
> struct tvp5150 *decoder = dev_id;
> struct regmap *map = decoder->regmap;
> - unsigned int active = 0, status = 0;
> + unsigned int mask, active = 0, status = 0;
> +
> + mask = TVP5150_MISC_CTL_YCBCR_OE | TVP5150_MISC_CTL_SYNC_OE |
> +TVP5150_MISC_CTL_CLOCK_OE;
>
> regmap_read(map, TVP5150_INT_STATUS_REG_A, );
> if (status) {
> regmap_write(map, TVP5150_INT_STATUS_REG_A, status);
>
> - if (status & TVP5150_INT_A_LOCK)
> + if (status & TVP5150_INT_A_LOCK) {
> decoder->lock = !!(status & TVP5150_INT_A_LOCK_STATUS);
> + regmap_update_bits(map, TVP5150_MISC_CTL, mask,
> +decoder->lock ? decoder->oe : 0);
> + }
>
> return IRQ_HANDLED;
> }
> @@ -872,10 +879,26 @@ static int tvp5150_enable(struct v4l2_subdev *sd)
> /* Disable autoswitch mode */
> tvp5150_set_std(sd, std);
>
> - if (decoder->mbus_type == V4L2_MBUS_PARALLEL)
> + /*
> + * Enable the YCbCr and clock outputs. In discrete sync mode
> + * (non-BT.656) additionally enable the the sync outputs.
> + */
> + switch (decoder->mbus_type) {
> + case V4L2_MBUS_PARALLEL:
> /* 8-bit 4:2:2 YUV with discrete sync output */
> regmap_update_bits(decoder->regmap, TVP5150_DATA_RATE_SEL,
> 0x7, 0x0);
> + decoder->oe = TVP5150_MISC_CTL_YCBCR_OE |
> + TVP5150_MISC_CTL_CLOCK_OE |
> + TVP5150_MISC_CTL_SYNC_OE;
> + break;
> + case V4L2_MBUS_BT656:
> + decoder->oe = TVP5150_MISC_CTL_YCBCR_OE |
> + TVP5150_MISC_CTL_CLOCK_OE;
> + break;
> + default:
> + return -EINVAL;
> + }
>
> return 0;
> };
> @@ -1190,14 +1213,8 @@ static int tvp5150_s_stream(struct v4l2_subdev *sd,
> int enable)
> if (enable) {
> tvp5150_enable(sd);
>
> - /*
> - * Enable the YCbCr and clock outputs. In discrete sync mode
> - * (non-BT.656) additionally enable the the sync outputs.
> - */
> - val = TVP5150_MISC_CTL_YCBCR_OE | TVP5150_MISC_CTL_CLOCK_OE;
> - if (decoder->mbus_type == V4L2_MBUS_PARALLEL)
> - val |= TVP5150_MISC_CTL_SYNC_OE;
> -
> + /* Enable outputs if decoder is locked */
> + val = decoder->lock ? decoder->oe : 0;
Hmm... this only works if the tvp5150 has the interrupts enabled.
The code should be, instead:
if (c->irq)
val = decoder->lock ? decoder->oe : 0;
else
val = decoder->oe;
> int_val = TVP5150_INT_A_LOCK;
> }
>
Thanks,
Mauro
Re: [PATCH] [media] dvb-frontends/tda18271c2dd: silence sparse fall through warning
Em Sun, 24 Jun 2018 15:42:50 +0200 Daniel Scheller escreveu: > From: Daniel Scheller > > Add a break statement in set_params() for the SYS_DVBT(2) case to silence > this sparse warning: > > drivers/media/dvb-frontends/tda18271c2dd.c:1144:3: warning: this > statement may fall through [-Wimplicit-fallthrough=] > > as reported in Hans' daily media_tree builds. > > Signed-off-by: Daniel Scheller > --- > drivers/media/dvb-frontends/tda18271c2dd.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/media/dvb-frontends/tda18271c2dd.c > b/drivers/media/dvb-frontends/tda18271c2dd.c > index 2e1d36ae943b..fcffc7b4acf7 100644 > --- a/drivers/media/dvb-frontends/tda18271c2dd.c > +++ b/drivers/media/dvb-frontends/tda18271c2dd.c > @@ -1154,6 +1154,7 @@ static int set_params(struct dvb_frontend *fe) > default: > return -EINVAL; > } > + break; This actually seems to be a bug. Did you test this patch with an actual hardware? Regards, Mauro Thanks, Mauro
Re: [PATCH 01/19] [media] dvb-frontends/mxl5xx: add break to case DVBS2 in get_frontend()
Em Sat, 23 Jun 2018 17:35:57 +0200
Daniel Scheller escreveu:
> From: Daniel Scheller
>
> Fix one sparse warning:
>
> drivers/media/dvb-frontends/mxl5xx.c:731:3: warning: this statement may
> fall through [-Wimplicit-fallthrough=]
>
> as seen in Hans' daily media_tree builds.
>
> Signed-off-by: Daniel Scheller
> ---
> drivers/media/dvb-frontends/mxl5xx.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/media/dvb-frontends/mxl5xx.c
> b/drivers/media/dvb-frontends/mxl5xx.c
> index 274d8fca0763..a7d08ace11ba 100644
> --- a/drivers/media/dvb-frontends/mxl5xx.c
> +++ b/drivers/media/dvb-frontends/mxl5xx.c
> @@ -739,6 +739,7 @@ static int get_frontend(struct dvb_frontend *fe,
> default:
> break;
> }
> + break;
> case SYS_DVBS:
> switch ((enum MXL_HYDRA_MODULATION_E)
> reg_data[DMD_MODULATION_SCHEME_ADDR]) {
Are you sure this is the right thing to do here? looking at the
code, I suspect that it should, instead, just adding a comment, as
the stuff below the SYS_DVBS case seem to be needed also for DVB-S2.
Thanks,
Mauro
Re: [PATCH 1/1] i2c: Fix pm_runtime_get_if_in_use() usage in sensor drivers
On Mon, Jul 30, 2018 at 9:12 PM Sakari Ailus wrote: > > pm_runtime_get_if_in_use() returns -EINVAL if runtime PM is disabled. This > should not be considered an error. Generally the driver has enabled > runtime PM already so getting this error due to runtime PM being disabled > will not happen. > > Instead of checking for lesser or equal to zero, check for zero only. > Address this for drivers where this pattern exists. > > This patch has been produced using the following command: > > $ git grep -l pm_runtime_get_if_in_use -- drivers/media/i2c/ | \ > xargs perl -i -pe 's/(pm_runtime_get_if_in_use\(.*\)) \<\= 0/!$1/' > > Signed-off-by: Sakari Ailus > --- > drivers/media/i2c/ov13858.c | 2 +- > drivers/media/i2c/ov2685.c | 2 +- > drivers/media/i2c/ov5670.c | 2 +- > drivers/media/i2c/ov5695.c | 2 +- > drivers/media/i2c/ov7740.c | 2 +- > 5 files changed, 5 insertions(+), 5 deletions(-) Thanks! Reviewed-by: Tomasz Figa Best regards, Tomasz
[PATCH 1/1] i2c: Fix pm_runtime_get_if_in_use() usage in sensor drivers
pm_runtime_get_if_in_use() returns -EINVAL if runtime PM is disabled. This
should not be considered an error. Generally the driver has enabled
runtime PM already so getting this error due to runtime PM being disabled
will not happen.
Instead of checking for lesser or equal to zero, check for zero only.
Address this for drivers where this pattern exists.
This patch has been produced using the following command:
$ git grep -l pm_runtime_get_if_in_use -- drivers/media/i2c/ | \
xargs perl -i -pe 's/(pm_runtime_get_if_in_use\(.*\)) \<\= 0/!$1/'
Signed-off-by: Sakari Ailus
---
drivers/media/i2c/ov13858.c | 2 +-
drivers/media/i2c/ov2685.c | 2 +-
drivers/media/i2c/ov5670.c | 2 +-
drivers/media/i2c/ov5695.c | 2 +-
drivers/media/i2c/ov7740.c | 2 +-
5 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/media/i2c/ov13858.c b/drivers/media/i2c/ov13858.c
index a66f6201f53c7..0e7a85c4996c7 100644
--- a/drivers/media/i2c/ov13858.c
+++ b/drivers/media/i2c/ov13858.c
@@ -1230,7 +1230,7 @@ static int ov13858_set_ctrl(struct v4l2_ctrl *ctrl)
* Applying V4L2 control value only happens
* when power is up for streaming
*/
- if (pm_runtime_get_if_in_use(>dev) <= 0)
+ if (!pm_runtime_get_if_in_use(>dev))
return 0;
ret = 0;
diff --git a/drivers/media/i2c/ov2685.c b/drivers/media/i2c/ov2685.c
index 385c1886a9470..98a1f2e312b58 100644
--- a/drivers/media/i2c/ov2685.c
+++ b/drivers/media/i2c/ov2685.c
@@ -549,7 +549,7 @@ static int ov2685_set_ctrl(struct v4l2_ctrl *ctrl)
break;
}
- if (pm_runtime_get_if_in_use(>dev) <= 0)
+ if (!pm_runtime_get_if_in_use(>dev))
return 0;
switch (ctrl->id) {
diff --git a/drivers/media/i2c/ov5670.c b/drivers/media/i2c/ov5670.c
index 7b7c74d773707..53dd30d96e691 100644
--- a/drivers/media/i2c/ov5670.c
+++ b/drivers/media/i2c/ov5670.c
@@ -2016,7 +2016,7 @@ static int ov5670_set_ctrl(struct v4l2_ctrl *ctrl)
}
/* V4L2 controls values will be applied only when power is already up */
- if (pm_runtime_get_if_in_use(>dev) <= 0)
+ if (!pm_runtime_get_if_in_use(>dev))
return 0;
switch (ctrl->id) {
diff --git a/drivers/media/i2c/ov5695.c b/drivers/media/i2c/ov5695.c
index 9a80decd93d3c..5d107c53364d6 100644
--- a/drivers/media/i2c/ov5695.c
+++ b/drivers/media/i2c/ov5695.c
@@ -1110,7 +1110,7 @@ static int ov5695_set_ctrl(struct v4l2_ctrl *ctrl)
break;
}
- if (pm_runtime_get_if_in_use(>dev) <= 0)
+ if (!pm_runtime_get_if_in_use(>dev))
return 0;
switch (ctrl->id) {
diff --git a/drivers/media/i2c/ov7740.c b/drivers/media/i2c/ov7740.c
index 605f3e25ad82b..6e9c233cfbe35 100644
--- a/drivers/media/i2c/ov7740.c
+++ b/drivers/media/i2c/ov7740.c
@@ -510,7 +510,7 @@ static int ov7740_set_ctrl(struct v4l2_ctrl *ctrl)
int ret;
u8 val = 0;
- if (pm_runtime_get_if_in_use(>dev) <= 0)
+ if (!pm_runtime_get_if_in_use(>dev))
return 0;
switch (ctrl->id) {
--
2.11.0
Re: [PATCH v2] media: imx208: Add imx208 camera sensor driver
On Mon, Jul 30, 2018 at 8:39 PM Sakari Ailus
wrote:
>
> Hi Tomasz,
>
> On Mon, Jul 30, 2018 at 07:19:56PM +0900, Tomasz Figa wrote:
> ...
> > > +static int imx208_set_ctrl(struct v4l2_ctrl *ctrl)
> > > +{
> > > + struct imx208 *imx208 =
> > > + container_of(ctrl->handler, struct imx208, ctrl_handler);
> > > + struct i2c_client *client = v4l2_get_subdevdata(>sd);
> > > + int ret;
> > > +
> > > + /*
> > > +* Applying V4L2 control value only happens
> > > +* when power is up for streaming
> > > +*/
> > > + if (pm_runtime_get_if_in_use(>dev) <= 0)
> >
> > This is buggy, because it won't handle the case of runtime PM disabled
> > in kernel config. The check should be
> > (!pm_runtime_get_if_in_use(>dev)).
>
> Good find. This seems to be the case for most other sensor drivers that
> make use of the function. I can submit a fix for those as well.
>
> I suppose most people use these with runtime PM enabled as this hasn't been
> spotted previously.
Yeah, I spotted it first with imx258 and it took us few emails to get
to the right code. :)
These drivers probably don't have too many users yet in general, so I
guess this didn't manage to cause any problems yet, but it became a
good example of bug propagation via copy/paste. ;)
Fixing would be appreciated indeed!
Best regards,
Tomasz
Re: [PATCH v2] media: imx208: Add imx208 camera sensor driver
Hi Tomasz,
On Mon, Jul 30, 2018 at 07:19:56PM +0900, Tomasz Figa wrote:
...
> > +static int imx208_set_ctrl(struct v4l2_ctrl *ctrl)
> > +{
> > + struct imx208 *imx208 =
> > + container_of(ctrl->handler, struct imx208, ctrl_handler);
> > + struct i2c_client *client = v4l2_get_subdevdata(>sd);
> > + int ret;
> > +
> > + /*
> > +* Applying V4L2 control value only happens
> > +* when power is up for streaming
> > +*/
> > + if (pm_runtime_get_if_in_use(>dev) <= 0)
>
> This is buggy, because it won't handle the case of runtime PM disabled
> in kernel config. The check should be
> (!pm_runtime_get_if_in_use(>dev)).
Good find. This seems to be the case for most other sensor drivers that
make use of the function. I can submit a fix for those as well.
I suppose most people use these with runtime PM enabled as this hasn't been
spotted previously.
--
Sakari Ailus
sakari.ai...@linux.intel.com
Re: [PATCH v2] media: imx208: Add imx208 camera sensor driver
Hi Ping-chung, On Mon, Jul 30, 2018 at 05:26:39PM +0800, Ping-chung Chen wrote: > From: "Chen, Ping-chung" > > Add a V4L2 sub-device driver for the Sony IMX208 image sensor. > This is a camera sensor using the I2C bus for control and the > CSI-2 bus for data. > > Signed-off-by: Ping-Chung Chen Could you add obtaining the valid link frequencies from the firmware, i.e. use v4l2_fwnode_endpoint_alloc_parse()? -- Kind regards, Sakari Ailus sakari.ai...@linux.intel.com
Re: [PATCH v2] media: imx208: Add imx208 camera sensor driver
Hi Ping-chung,
On Mon, Jul 30, 2018 at 6:19 PM Ping-chung Chen
wrote:
>
> From: "Chen, Ping-chung"
>
> Add a V4L2 sub-device driver for the Sony IMX208 image sensor.
> This is a camera sensor using the I2C bus for control and the
> CSI-2 bus for data.
>
Please see my comments inline.
[snip]
> diff --git a/drivers/media/i2c/imx208.c b/drivers/media/i2c/imx208.c
> new file mode 100644
> index 000..5adfb79
> --- /dev/null
> +++ b/drivers/media/i2c/imx208.c
> @@ -0,0 +1,984 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// Copyright (C) 2018 Intel Corporation
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#define IMX208_REG_VALUE_08BIT 1
> +#define IMX208_REG_VALUE_16BIT 2
We don't need to define these. It's obvious that 8 bits is 1 byte and
16 bits are 2 bytes.
> +
> +#define IMX208_REG_MODE_SELECT 0x0100
> +#define IMX208_MODE_STANDBY0x00
> +#define IMX208_MODE_STREAMING 0x01
[snip]
> +/* Test Pattern Control */
> +#define IMX208_REG_TEST_PATTERN_MODE 0x0600
> +#define IMX208_TEST_PATTERN_DISABLE0
> +#define IMX208_TEST_PATTERN_SOLID_COLOR1
> +#define IMX208_TEST_PATTERN_COLOR_BARS 2
> +#define IMX208_TEST_PATTERN_GREY_COLOR 3
> +#define IMX208_TEST_PATTERN_PN94
Please use hexadecimal notation for register values (as already done below).
> +#define IMX208_TEST_PATTERN_FIX_1 0x100
> +#define IMX208_TEST_PATTERN_FIX_2 0x101
> +#define IMX208_TEST_PATTERN_FIX_3 0x102
> +#define IMX208_TEST_PATTERN_FIX_4 0x103
> +#define IMX208_TEST_PATTERN_FIX_5 0x104
> +#define IMX208_TEST_PATTERN_FIX_6 0x105
[snip]
> +static const int imx208_test_pattern_val[] = {
> + IMX208_TEST_PATTERN_DISABLE,
> + IMX208_TEST_PATTERN_SOLID_COLOR,
> + IMX208_TEST_PATTERN_COLOR_BARS,
> + IMX208_TEST_PATTERN_GREY_COLOR,
> + IMX208_TEST_PATTERN_PN9,
> + IMX208_TEST_PATTERN_FIX_1,
> + IMX208_TEST_PATTERN_FIX_2,
> + IMX208_TEST_PATTERN_FIX_3,
> + IMX208_TEST_PATTERN_FIX_4,
> + IMX208_TEST_PATTERN_FIX_5,
> + IMX208_TEST_PATTERN_FIX_6,
> +};
> +
> +/* Configurations for supported link frequencies */
> +#define IMX208_LINK_FREQ_384MHZ38400ULL
> +#define IMX208_LINK_FREQ_96MHZ 9600ULL
nit: If we really need defines for these, then at least they should be
somehow useful, e.g.
#define MHZ (1000*1000ULL)
#define IMX208_LINK_FREQ_384MHZ (384ULL * MHZ)
This at least makes it easy to see that there are no mistakes in the
number, e.g. wrong number of zeroes.
> +
> +enum {
> + IMX208_LINK_FREQ_384MHZ_INDEX,
> + IMX208_LINK_FREQ_96MHZ_INDEX,
> +};
> +
> +/*
> + * pixel_rate = link_freq * data-rate * nr_of_lanes / bits_per_sample
> + * data rate => double data rate; number of lanes => 2; bits per pixel => 10
> + */
> +static u64 link_freq_to_pixel_rate(u64 f)
> +{
> + f *= 2 * 2;
> + do_div(f, 10);
Please add macros for those magic numbers.
> +
> + return f;
> +}
> +
> +/* Menu items for LINK_FREQ V4L2 control */
> +static const s64 link_freq_menu_items[] = {
> + IMX208_LINK_FREQ_384MHZ,
> + IMX208_LINK_FREQ_96MHZ,
Since we have an enum already, please use it for explicit indices, to
ensure things are consistent and actually easier to read, i.e.
[IMX208_LINK_FREQ_384MHZ_INDEX] = IMX208_LINK_FREQ_384MHZ,
> +};
> +
> +/* Link frequency configs */
> +static const struct imx208_link_freq_config link_freq_configs[] = {
> + {
Explicit indices, i.e.
[IMX208_LINK_FREQ_384MHZ_INDEX] = {
> + .pixels_per_line = IMX208_PPL_384MHZ,
> + .reg_list = {
> + .num_of_regs = ARRAY_SIZE(mipi_data_rate),
> + .regs = mipi_data_rate,
> + }
> + },
> + {
Ditto.
> + .pixels_per_line = IMX208_PPL_96MHZ,
> + .reg_list = {
> + .num_of_regs = ARRAY_SIZE(mipi_data_rate),
> + .regs = mipi_data_rate,
How comes that both link frequencies have the same register values for
MIPI data rate?
> + }
> + },
> +};
> +
> +/* Mode configs */
> +static const struct imx208_mode supported_modes[] = {
> + {
> + .width = 1936,
> + .height = 1096,
> + .vts_def = IMX208_VTS_60FPS,
> + .vts_min = IMX208_VTS_60FPS_MIN,
> + .reg_list = {
> + .num_of_regs = ARRAY_SIZE(mode_1936x1096_60fps_regs),
> + .regs = mode_1936x1096_60fps_regs,
> + },
> + .link_freq_index = 0,
Please use the index that was defined before - IMX208_LINK_FREQ_384MHZ_INDEX.
> + },
> + {
> + .width = 968,
> + .height = 548,
> + .vts_def = IMX208_VTS_BINNING,
> + .vts_min = IMX208_VTS_BINNING_MIN,
> +
[PATCH v2] media: imx208: Add imx208 camera sensor driver
From: "Chen, Ping-chung" Add a V4L2 sub-device driver for the Sony IMX208 image sensor. This is a camera sensor using the I2C bus for control and the CSI-2 bus for data. Signed-off-by: Ping-Chung Chen --- since v1: -- Update the function media_entity_pads_init for upstreaming. -- Change the structure name mutex as imx208_mx. -- Refine the control flow of test pattern function. -- vflip/hflip control support (will impact the output bayer order) -- support 4 bayer orders output (via change v/hflip) - SRGGB10(default), SGRBG10, SGBRG10, SBGGR10 -- Simplify error handling in the set_stream function. MAINTAINERS| 7 + drivers/media/i2c/Kconfig | 11 + drivers/media/i2c/Makefile | 1 + drivers/media/i2c/imx208.c | 984 + 4 files changed, 1003 insertions(+) create mode 100644 drivers/media/i2c/imx208.c diff --git a/MAINTAINERS b/MAINTAINERS index bbd9b9b..896c1df 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -13268,6 +13268,13 @@ S: Maintained F: drivers/ssb/ F: include/linux/ssb/ +SONY IMX208 SENSOR DRIVER +M: Sakari Ailus +L: linux-media@vger.kernel.org +T: git git://linuxtv.org/media_tree.git +S: Maintained +F: drivers/media/i2c/imx208.c + SONY IMX258 SENSOR DRIVER M: Sakari Ailus L: linux-media@vger.kernel.org diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig index 8669853..ae11f1e 100644 --- a/drivers/media/i2c/Kconfig +++ b/drivers/media/i2c/Kconfig @@ -595,6 +595,17 @@ config VIDEO_APTINA_PLL config VIDEO_SMIAPP_PLL tristate +config VIDEO_IMX208 + tristate "Sony IMX208 sensor support" + depends on I2C && VIDEO_V4L2 && VIDEO_V4L2_SUBDEV_API + depends on MEDIA_CAMERA_SUPPORT + ---help--- + This is a Video4Linux2 sensor driver for the Sony + IMX208 camera. + + To compile this driver as a module, choose M here: the + module will be called imx208. + config VIDEO_IMX258 tristate "Sony IMX258 sensor support" depends on I2C && VIDEO_V4L2 && VIDEO_V4L2_SUBDEV_API diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile index 837c428..47604c2 100644 --- a/drivers/media/i2c/Makefile +++ b/drivers/media/i2c/Makefile @@ -104,6 +104,7 @@ obj-$(CONFIG_VIDEO_I2C) += video-i2c.o obj-$(CONFIG_VIDEO_ML86V7667) += ml86v7667.o obj-$(CONFIG_VIDEO_OV2659) += ov2659.o obj-$(CONFIG_VIDEO_TC358743) += tc358743.o +obj-$(CONFIG_VIDEO_IMX208) += imx208.o obj-$(CONFIG_VIDEO_IMX258) += imx258.o obj-$(CONFIG_VIDEO_IMX274) += imx274.o diff --git a/drivers/media/i2c/imx208.c b/drivers/media/i2c/imx208.c new file mode 100644 index 000..5adfb79 --- /dev/null +++ b/drivers/media/i2c/imx208.c @@ -0,0 +1,984 @@ +// SPDX-License-Identifier: GPL-2.0 +// Copyright (C) 2018 Intel Corporation + +#include +#include +#include +#include +#include +#include +#include +#include + +#define IMX208_REG_VALUE_08BIT 1 +#define IMX208_REG_VALUE_16BIT 2 + +#define IMX208_REG_MODE_SELECT 0x0100 +#define IMX208_MODE_STANDBY0x00 +#define IMX208_MODE_STREAMING 0x01 + +/* Chip ID */ +#define IMX208_REG_CHIP_ID 0x +#define IMX208_CHIP_ID 0x0208 + +/* V_TIMING internal */ +#define IMX208_REG_VTS 0x0340 +#define IMX208_VTS_60FPS 0x0472 +#define IMX208_VTS_BINNING 0x0239 +#define IMX208_VTS_60FPS_MIN 0x0458 +#define IMX208_VTS_BINNING_MIN 0x0230 +#define IMX208_VTS_MAX 0x + +/* HBLANK control - read only */ +#define IMX208_PPL_384MHZ 2248 +#define IMX208_PPL_96MHZ 2248 + +/* Exposure control */ +#define IMX208_REG_EXPOSURE0x0202 +#define IMX208_EXPOSURE_MIN4 +#define IMX208_EXPOSURE_STEP 1 +#define IMX208_EXPOSURE_DEFAULT0x190 +#define IMX208_EXPOSURE_MAX65535 + +/* Analog gain control */ +#define IMX208_REG_ANALOG_GAIN 0x0204 +#define IMX208_ANA_GAIN_MIN0 +#define IMX208_ANA_GAIN_MAX0x00e0 +#define IMX208_ANA_GAIN_STEP 1 +#define IMX208_ANA_GAIN_DEFAULT0x0 + +/* Digital gain control */ +#define IMX208_REG_GR_DIGITAL_GAIN 0x020e +#define IMX208_REG_R_DIGITAL_GAIN 0x0210 +#define IMX208_REG_B_DIGITAL_GAIN 0x0212 +#define IMX208_REG_GB_DIGITAL_GAIN 0x0214 +#define IMX208_DGTL_GAIN_MIN 0 +#define IMX208_DGTL_GAIN_MAX 4096 +#define IMX208_DGTL_GAIN_DEFAULT 0x100 +#define IMX208_DGTL_GAIN_STEP 1 + +/* Orientation */ +#define IMX208_REG_ORIENTATION_CONTROL 0x0101 + +/* Test Pattern Control */ +#define IMX208_REG_TEST_PATTERN_MODE 0x0600 +#define IMX208_TEST_PATTERN_DISABLE0 +#define IMX208_TEST_PATTERN_SOLID_COLOR1 +#define IMX208_TEST_PATTERN_COLOR_BARS 2 +#define IMX208_TEST_PATTERN_GREY_COLOR 3 +#define
Re: [PATCH 1/1] mt9v111: Fix compiler warning by initialising a variable
On Mon, Jul 30, 2018 at 09:51:25AM +0200, jacopo mondi wrote: > Hi Sakari, > > On Mon, Jul 30, 2018 at 10:26:27AM +0300, Sakari Ailus wrote: > > While this isn't a bug, initialise the variable to quash the warning. > > > > Reported-by: Stephen Rothwell > > Signed-off-by: Sakari Ailus > > --- > > drivers/media/i2c/mt9v111.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/media/i2c/mt9v111.c b/drivers/media/i2c/mt9v111.c > > index da8f6ab91307..58d5f2224bff 100644 > > --- a/drivers/media/i2c/mt9v111.c > > +++ b/drivers/media/i2c/mt9v111.c > > @@ -884,7 +884,7 @@ static int mt9v111_set_format(struct v4l2_subdev > > *subdev, > > struct v4l2_mbus_framefmt new_fmt; > > struct v4l2_mbus_framefmt *__fmt; > > unsigned int best_fit = ~0L; > > - unsigned int idx; > > + unsigned int idx = 0; > > unsigned int i; > > > > Thanks for this, but there is already a patch sent on Friday by Jasmin > addressing the warning > > https://patchwork.kernel.org/patch/10547983/ Ah, ok. I'll mark this as rejected then. -- Sakari Ailus sakari.ai...@linux.intel.com
Re: [PATCH] media: i2c: fix warning in Aptina MT9V111
Hi Jasmin, On Sat, Jul 28, 2018 at 03:54:49PM +0200, Jasmin J. wrote: > From: Jasmin Jessich > > This fixes the "'idx' may be used uninitialized in this function" > warning. > > Cc: Jacopo Mondi > Signed-off-by: Jasmin Jessich Thanks for the patch Acked-by: Jacopo Mondi Thanks j > --- > drivers/media/i2c/mt9v111.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/media/i2c/mt9v111.c b/drivers/media/i2c/mt9v111.c > index da8f6ab..58d5f22 100644 > --- a/drivers/media/i2c/mt9v111.c > +++ b/drivers/media/i2c/mt9v111.c > @@ -884,7 +884,7 @@ static int mt9v111_set_format(struct v4l2_subdev *subdev, > struct v4l2_mbus_framefmt new_fmt; > struct v4l2_mbus_framefmt *__fmt; > unsigned int best_fit = ~0L; > - unsigned int idx; > + unsigned int idx = 0; > unsigned int i; > > mutex_lock(>stream_mutex); > -- > 2.7.4 > signature.asc Description: PGP signature
Re: [PATCH 1/1] mt9v111: Fix compiler warning by initialising a variable
Hi Sakari, On Mon, Jul 30, 2018 at 10:26:27AM +0300, Sakari Ailus wrote: > While this isn't a bug, initialise the variable to quash the warning. > > Reported-by: Stephen Rothwell > Signed-off-by: Sakari Ailus > --- > drivers/media/i2c/mt9v111.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/media/i2c/mt9v111.c b/drivers/media/i2c/mt9v111.c > index da8f6ab91307..58d5f2224bff 100644 > --- a/drivers/media/i2c/mt9v111.c > +++ b/drivers/media/i2c/mt9v111.c > @@ -884,7 +884,7 @@ static int mt9v111_set_format(struct v4l2_subdev *subdev, > struct v4l2_mbus_framefmt new_fmt; > struct v4l2_mbus_framefmt *__fmt; > unsigned int best_fit = ~0L; > - unsigned int idx; > + unsigned int idx = 0; > unsigned int i; > Thanks for this, but there is already a patch sent on Friday by Jasmin addressing the warning https://patchwork.kernel.org/patch/10547983/ > mutex_lock(>stream_mutex); > -- > 2.11.0 > signature.asc Description: PGP signature
[PATCH 1/1] mt9v111: Fix compiler warning by initialising a variable
While this isn't a bug, initialise the variable to quash the warning. Reported-by: Stephen Rothwell Signed-off-by: Sakari Ailus --- drivers/media/i2c/mt9v111.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/media/i2c/mt9v111.c b/drivers/media/i2c/mt9v111.c index da8f6ab91307..58d5f2224bff 100644 --- a/drivers/media/i2c/mt9v111.c +++ b/drivers/media/i2c/mt9v111.c @@ -884,7 +884,7 @@ static int mt9v111_set_format(struct v4l2_subdev *subdev, struct v4l2_mbus_framefmt new_fmt; struct v4l2_mbus_framefmt *__fmt; unsigned int best_fit = ~0L; - unsigned int idx; + unsigned int idx = 0; unsigned int i; mutex_lock(>stream_mutex); -- 2.11.0
