[PATCH] media: cx23885: check allocation return
Checking of kmalloc() seems to have been committed - as
cx23885_dvb_register() is checking for != 0 return, returning
-ENOMEM should be fine here. While at it address the coccicheck
suggestion to move to kmemdup rather than using kmalloc+memcpy.
Signed-off-by: Nicholas Mc Guire
Fixes: 46b21bbaa8a8 ("[media] Add support for DViCO FusionHDTV DVB-T Dual
Express2")
---
Problem located with an experimental coccinelle script
Patch was compile tested with: x86_64_defconfig + MEDIA_SUPPORT=y
MEDIA_PCI_SUPPORT=y, MEDIA_DIGITAL_TV_SUPPORT=y,
MEDIA_ANALOG_TV_SUPPORT=y (for VIDEO_DEV), RC_CORE=y
VIDEO_CX23885=y
The coccicheck on the initial fix for kmalloc only reported:
drivers/media/pci/cx23885//cx23885-dvb.c:1477:33-40: WARNING opportunity for
kmemdup
so that was merged into this patch - the return value still needs to
be checked.
Patch is against 5.0-rc2 (localversion-next is next-20190118)
drivers/media/pci/cx23885/cx23885-dvb.c | 7 +--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/drivers/media/pci/cx23885/cx23885-dvb.c
b/drivers/media/pci/cx23885/cx23885-dvb.c
index 0d0929c..225cdfe 100644
--- a/drivers/media/pci/cx23885/cx23885-dvb.c
+++ b/drivers/media/pci/cx23885/cx23885-dvb.c
@@ -1474,8 +1474,11 @@ static int dvb_register(struct cx23885_tsport *port)
if (fe0->dvb.frontend != NULL) {
struct i2c_adapter *tun_i2c;
- fe0->dvb.frontend->sec_priv =
kmalloc(sizeof(dib7000p_ops), GFP_KERNEL);
- memcpy(fe0->dvb.frontend->sec_priv, &dib7000p_ops,
sizeof(dib7000p_ops));
+ fe0->dvb.frontend->sec_priv = kmemdup(&dib7000p_ops,
+ sizeof(dib7000p_ops),
+ GFP_KERNEL);
+ if (!fe0->dvb.frontend->sec_priv)
+ return -ENOMEM;
tun_i2c =
dib7000p_ops.get_i2c_master(fe0->dvb.frontend, DIBX000_I2C_INTERFACE_TUNER, 1);
if (!dvb_attach(dib0070_attach, fe0->dvb.frontend,
tun_i2c, &dib7070p_dib0070_config))
return -ENODEV;
--
2.1.4
Re: [SIL2review] [PATCH] media: tc358743: release device_node in tc358743_probe_of()
On Sat, May 26, 2018 at 12:54:00AM +0300, Alexey Khoroshilov wrote:
> of_graph_get_next_endpoint() returns device_node with refcnt increased,
> but these is no of_node_put() for it.
I think this is correct - but would it not be simpler to do
endpoint = v4l2_fwnode_endpoint_alloc_parse(of_fwnode_handle(ep));
of_node_put(ep);
if (IS_ERR(endpoint)) {
As the of_node_put(np) actually is unconditional anyway I think this
should be semantically equivalent.
>
> The patch adds one on error and normal paths.
>
> Found by Linux Driver Verification project (linuxtesting.org).
>
> Signed-off-by: Alexey Khoroshilov
Reviewed-by: Nicholas Mc Guire
> ---
> drivers/media/i2c/tc358743.c | 5 -
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/media/i2c/tc358743.c b/drivers/media/i2c/tc358743.c
> index 393baad7..44c41933415a 100644
> --- a/drivers/media/i2c/tc358743.c
> +++ b/drivers/media/i2c/tc358743.c
> @@ -1918,7 +1918,8 @@ static int tc358743_probe_of(struct tc358743_state
> *state)
> endpoint = v4l2_fwnode_endpoint_alloc_parse(of_fwnode_handle(ep));
> if (IS_ERR(endpoint)) {
> dev_err(dev, "failed to parse endpoint\n");
> - return PTR_ERR(endpoint);
> + ret = PTR_ERR(endpoint);
> + goto put_node;
> }
>
> if (endpoint->bus_type != V4L2_MBUS_CSI2 ||
> @@ -2013,6 +2014,8 @@ static int tc358743_probe_of(struct tc358743_state
> *state)
> clk_disable_unprepare(refclk);
> free_endpoint:
> v4l2_fwnode_endpoint_free(endpoint);
> +put_node:
> + of_node_put(ep);
> return ret;
> }
> #else
> --
> 2.7.4
>
> ___
> SIL2review mailing list
> sil2rev...@lists.osadl.org
> https://lists.osadl.org/mailman/listinfo/sil2review
[PATCH] media: davinci_vpfe: bail out if kmalloc failed
The kmalloc is passed indirectly to from but with an offset
which if not 0 will cause the null check if (to && from && size)
to succeed. An explicit !NULL check is thus added for params here.
ipipe_s_config and ipipe_g_config - both fail to check kmalloc
are called from ipipe_ioctl where a negative return is a valid
indication of error so simply setting rval = -ENOMEM seems ok.
Signed-off-by: Nicholas Mc Guire
Fixes: da43b6ccadcf ("[media] davinci: vpfe: dm365: add IPIPE support for media
controller driver")
---
Problem located with experimental coccinelle patch
Patch was compile tested with: davinci_all_defconfig + SAGING=y,
STAGING_MEDIA=y, MEDIA_SUPPORT=m, MEDIA_CONTROLLER=y,
VIDEO_V4L2_SUBDEV_API=y, VIDEO_DAVINCI_VPBE_DISPLAY=m,
VIDEO_DM365_VPFE=m
(with some coccicheck findings unrelated to the proposed change)
Patch is against 4.20-rc3 (localversion-next is next-20181120)
drivers/staging/media/davinci_vpfe/dm365_ipipe.c | 10 ++
1 file changed, 10 insertions(+)
diff --git a/drivers/staging/media/davinci_vpfe/dm365_ipipe.c
b/drivers/staging/media/davinci_vpfe/dm365_ipipe.c
index 3d910b8..0150aed 100644
--- a/drivers/staging/media/davinci_vpfe/dm365_ipipe.c
+++ b/drivers/staging/media/davinci_vpfe/dm365_ipipe.c
@@ -1266,6 +1266,11 @@ static int ipipe_s_config(struct v4l2_subdev *sd, struct
vpfe_ipipe_config *cfg)
params = kmalloc(sizeof(struct ipipe_module_params),
GFP_KERNEL);
+ if (!params) {
+ rval = -ENOMEM;
+ goto error;
+ }
+
to = (void *)params + module_if->param_offset;
size = module_if->param_size;
@@ -1308,6 +1313,11 @@ static int ipipe_g_config(struct v4l2_subdev *sd, struct
vpfe_ipipe_config *cfg)
params = kmalloc(sizeof(struct ipipe_module_params),
GFP_KERNEL);
+ if (!params) {
+ rval = -ENOMEM;
+ goto error;
+ }
+
from = (void *)params + module_if->param_offset;
size = module_if->param_size;
--
2.1.4
[PATCH] [media] ov9650: use msleep() for uncritical long delay
ulseep_range() uses hrtimers and provides no advantage over msleep()
for larger delays. Fix up the 25ms delays here to use msleep() and
reduce the load on the hrtimer subsystem.
Link: http://lkml.org/lkml/2017/1/11/377
Signed-off-by: Nicholas Mc Guire
---
Problem found by coccinelle script
Patch was compile tested with: x86_64_defconfig + CONFIG_MEDIA_SUPPORT=m
CONFIG_MEDIA_ANALOG_TV_SUPPORT=y, CONFIG_MEDIA_CONTROLLER=y
CONFIG_VIDEO_V4L2_SUBDEV_API=y, CONFIG_MEDIA_SUBDRV_AUTOSELECT=n
CONFIG_VIDEO_OV9650=m
Patch is aginast 4.10-rc3 (localversion-next is next-20170116)
drivers/media/i2c/ov9650.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/media/i2c/ov9650.c b/drivers/media/i2c/ov9650.c
index 502c722..2de2fbb 100644
--- a/drivers/media/i2c/ov9650.c
+++ b/drivers/media/i2c/ov9650.c
@@ -522,7 +522,7 @@ static void __ov965x_set_power(struct ov965x *ov965x, int
on)
if (on) {
ov965x_gpio_set(ov965x->gpios[GPIO_PWDN], 0);
ov965x_gpio_set(ov965x->gpios[GPIO_RST], 0);
- usleep_range(25000, 26000);
+ msleep(25);
} else {
ov965x_gpio_set(ov965x->gpios[GPIO_RST], 1);
ov965x_gpio_set(ov965x->gpios[GPIO_PWDN], 1);
@@ -1438,7 +1438,7 @@ static int ov965x_detect_sensor(struct v4l2_subdev *sd)
mutex_lock(&ov965x->lock);
__ov965x_set_power(ov965x, 1);
- usleep_range(25000, 26000);
+ msleep(25);
/* Check sensor revision */
ret = ov965x_read(client, REG_PID, &pid);
--
2.1.4
--
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] media: add GFP flag to media_*() that could get called in atomic context
On Sat, Mar 12, 2016 at 06:48:09PM -0700, Shuah Khan wrote:
> Add GFP flags to media_create_pad_link(), media_create_intf_link(),
> media_devnode_create(), and media_add_link() that could get called
> in atomic context to allow callers to pass in the right flags for
> memory allocation.
>
> tree-wide driver changes for media_*() GFP flags change:
> Change drivers to add gfpflags to interffaces, media_create_pad_link(),
> media_create_intf_link() and media_devnode_create().
>
in two cases (media_add_link,media_devnode_create) it is passing in
gfpflags but then checking for in_atomic() and in case it is, dumping
stack - so does it make sense in those cases to allow GFP_ATOMIC to be
passed in ? could not figure out why that would be needed (current
callers in media-entity.c do not seem to be under a spin_lock).
and in a few cases there seems to be a little glitch with indentation
dvb_create_media_graph
__fimc_md_create_fimc_sink_links
__fimc_md_create_fimc_is_links
vsp1_create_sink_links
v4l2_mc_create_media_graph
vpfe_ipipeif_register_entities
thx!
hofrat
> Signed-off-by: Shuah Khan
> Suggested-by: Mauro Carvalho Chehab
> ---
> Ran through kbuild-all compile testing.
> Tested the changes in Win-TV HVR-950Q device
>
> drivers/media/dvb-core/dvbdev.c| 26 +++-
> drivers/media/i2c/s5c73m3/s5c73m3-core.c | 6 ++-
> drivers/media/i2c/s5k5baf.c| 3 +-
> drivers/media/i2c/smiapp/smiapp-core.c | 3 +-
> drivers/media/i2c/tvp5150.c| 3 +-
> drivers/media/media-entity.c | 30 --
> drivers/media/platform/exynos4-is/media-dev.c | 19 +
> drivers/media/platform/omap3isp/isp.c | 47
> ++
> drivers/media/platform/s3c-camif/camif-core.c | 4 +-
> drivers/media/platform/vsp1/vsp1_drm.c | 6 +--
> drivers/media/platform/vsp1/vsp1_drv.c | 9 +++--
> drivers/media/platform/xilinx/xilinx-vipp.c| 4 +-
> drivers/media/usb/au0828/au0828-core.c | 3 +-
> drivers/media/usb/uvc/uvc_entity.c | 2 +-
> drivers/media/v4l2-core/v4l2-dev.c | 5 ++-
> drivers/media/v4l2-core/v4l2-device.c | 3 +-
> drivers/media/v4l2-core/v4l2-mc.c | 25 +++-
> drivers/staging/media/davinci_vpfe/dm365_ipipeif.c | 3 +-
> drivers/staging/media/davinci_vpfe/dm365_isif.c| 2 +-
> drivers/staging/media/davinci_vpfe/dm365_resizer.c | 10 +++--
> .../staging/media/davinci_vpfe/vpfe_mc_capture.c | 10 ++---
> drivers/staging/media/omap4iss/iss.c | 17 +---
> drivers/staging/media/omap4iss/iss_csi2.c | 6 ++-
> drivers/staging/media/omap4iss/iss_ipipeif.c | 3 +-
> drivers/staging/media/omap4iss/iss_resizer.c | 3 +-
> include/media/media-entity.h | 9 +++--
> sound/usb/media.c | 15 ---
> 27 files changed, 170 insertions(+), 106 deletions(-)
>
> diff --git a/drivers/media/dvb-core/dvbdev.c b/drivers/media/dvb-core/dvbdev.c
> index e1684c5..57f3e1e 100644
> --- a/drivers/media/dvb-core/dvbdev.c
> +++ b/drivers/media/dvb-core/dvbdev.c
> @@ -399,7 +399,8 @@ static int dvb_register_media_device(struct dvb_device
> *dvbdev,
>
> dvbdev->intf_devnode = media_devnode_create(dvbdev->adapter->mdev,
> intf_type, 0,
> - DVB_MAJOR, minor);
> + DVB_MAJOR, minor,
> + GFP_KERNEL);
>
> if (!dvbdev->intf_devnode)
> return -ENOMEM;
> @@ -416,7 +417,7 @@ static int dvb_register_media_device(struct dvb_device
> *dvbdev,
> return 0;
>
> link = media_create_intf_link(dvbdev->entity,
> &dvbdev->intf_devnode->intf,
> - MEDIA_LNK_FL_ENABLED);
> + MEDIA_LNK_FL_ENABLED, GFP_KERNEL);
> if (!link)
> return -ENOMEM;
> #endif
> @@ -558,7 +559,8 @@ static int dvb_create_io_intf_links(struct dvb_adapter
> *adap,
> if (strncmp(entity->name, name, strlen(name)))
> continue;
> link = media_create_intf_link(entity, intf,
> - MEDIA_LNK_FL_ENABLED);
> + MEDIA_LNK_FL_ENABLED,
> + GFP_KERNEL);
> if (!link)
> return -ENOMEM;
> }
> @@ -680,7 +682,8 @@ int dvb_create_media_graph(struct dvb_adapter *adap,
> }
> if (demux && ca) {
> ret = media_create_pad_link(demux, 1, ca,
> - 0, MEDIA_LNK
Re: [PATCH 07/22] media: s5k6aa: describe some function parameters
On Wed, Nov 29, 2017 at 02:08:25PM -0500, Mauro Carvalho Chehab wrote: > as warned: > drivers/media/i2c/s5k6aa.c:429: warning: No description found for parameter > 's5k6aa' > drivers/media/i2c/s5k6aa.c:679: warning: No description found for parameter > 's5k6aa' > drivers/media/i2c/s5k6aa.c:733: warning: No description found for parameter > 's5k6aa' > drivers/media/i2c/s5k6aa.c:733: warning: No description found for parameter > 'preset' > drivers/media/i2c/s5k6aa.c:787: warning: No description found for parameter > 'sd' > > Signed-off-by: Mauro Carvalho Chehab > --- > drivers/media/i2c/s5k6aa.c | 5 + > 1 file changed, 5 insertions(+) > > diff --git a/drivers/media/i2c/s5k6aa.c b/drivers/media/i2c/s5k6aa.c > index 9fd254a8e20d..13c10b5e2b45 100644 > --- a/drivers/media/i2c/s5k6aa.c > +++ b/drivers/media/i2c/s5k6aa.c > @@ -421,6 +421,7 @@ static int s5k6aa_set_ahb_address(struct i2c_client > *client) > > /** > * s5k6aa_configure_pixel_clock - apply ISP main clock/PLL configuration > + * @s5k6aa: pointer to &struct s5k6aa describing the device > * > * Configure the internal ISP PLL for the required output frequency. > * Locking: called with s5k6aa.lock mutex held. > @@ -669,6 +670,7 @@ static int s5k6aa_set_input_params(struct s5k6aa *s5k6aa) > > /** > * s5k6aa_configure_video_bus - configure the video output interface > + * @s5k6aa: pointer to &struct s5k6aa describing the device > * @bus_type: video bus type: parallel or MIPI-CSI > * @nlanes: number of MIPI lanes to be used (MIPI-CSI only) > * > @@ -724,6 +726,8 @@ static int s5k6aa_new_config_sync(struct i2c_client > *client, int timeout, > > /** > * s5k6aa_set_prev_config - write user preview register set > + * @s5k6aa: pointer to &struct s5k6aa describing the device > + * @preset: s5kaa preset to be applied that looks like a minor typo s5kaa should be s5k6aa also it might be more meaningful to describe its content e.g. * @preset: s5k6aa preset pixel format and resolution to be applied > * > * Configure output resolution and color fromat, pixel clock > * frequency range, device frame rate type and frame period range. > @@ -777,6 +781,7 @@ static int s5k6aa_set_prev_config(struct s5k6aa *s5k6aa, > > /** > * s5k6aa_initialize_isp - basic ISP MCU initialization > + * @sd: pointer to V4L2 sub-device descriptor > * > * Configure AHB addresses for registers read/write; configure PLLs for > * required output pixel clock. The ISP power supply needs to be already > -- > 2.14.3 > thx! hofrat
Re: [PATCH RFC] [media] m5mols: add missing dependency on VIDEO_IR_I2C
On Wed, Mar 29, 2017 at 11:56:08AM +0200, Sylwester Nawrocki wrote:
> On 12/13/2016 06:44 AM, Nicholas Mc Guire wrote:
> >The Depends on: tag in Kconfig for CONFIG_VIDEO_M5MOLS does not list
> >VIDEO_IR_I2C so Kconfig displays the dependencies needed so the M-5MOLS
> >driver can not be found.
> >
> >Fixes: commit cb7a01ac324b ("[media] move i2c files into drivers/media/i2c")
> >Signed-off-by: Nicholas Mc Guire
> >---
> >
> >searching for VIDEO_M5MOLS in menuconfig currently shows the following
> >dependencies
> > Depends on: MEDIA_SUPPORT [=m] && I2C [=y] && VIDEO_V4L2 [=m] && \
> > VIDEO_V4L2_SUBDEV_API [=y] && MEDIA_CAMERA_SUPPORT [=y]
> >but as the default settings include MEDIA_SUBDRV_AUTOSELECT=y the
> >"I2C module for IR" submenu (CONFIG_VIDEO_IR_I2C) is not displayed
> >adding the VIDEO_IR_I2C to the dependency list makes this clear
>
> > drivers/media/i2c/m5mols/Kconfig | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> >diff --git a/drivers/media/i2c/m5mols/Kconfig
> >b/drivers/media/i2c/m5mols/Kconfig
> >index dc8c250..6847a1b 100644
> >--- a/drivers/media/i2c/m5mols/Kconfig
> >+++ b/drivers/media/i2c/m5mols/Kconfig
> >@@ -1,6 +1,6 @@
> > config VIDEO_M5MOLS
> > tristate "Fujitsu M-5MOLS 8MP sensor support"
> >-depends on I2C && VIDEO_V4L2 && VIDEO_V4L2_SUBDEV_API
> >+depends on I2C && VIDEO_IR_I2C && VIDEO_V4L2 && VIDEO_V4L2_SUBDEV_API
>
> There should be no need to enable the "I2C module for IR" to use m5mols
> driver, so the bug fix needs to be somewhere else.
>
yup - my bad - not clear how I came to that conclusion, guess it was due
to the indirection of VIDEO_M5MOLS needing !CONFIG_MEDIA_SUBDRV_AUTOSELECT
Step-by-step its:
0) x86_64_defconfig
Depends on: MEDIA_SUPPORT [=n] && I2C [=y] && VIDEO_V4L2 [=n] &&
VIDEO_V4L2_SUBDEV_API [=n] && MEDIA_CAMERA_SUPPORT [=n]
1) Multimedia support --->
Depends on: MEDIA_SUPPORT [=m] && I2C [=y] && VIDEO_V4L2 [=n] &&
VIDEO_V4L2_SUBDEV_API [=n] && MEDIA_CAMERA_SUPPORT [=n]
2)[*] Cameras/video grabbers support
Depends on: MEDIA_SUPPORT [=m] && I2C [=y] && VIDEO_V4L2 [=m] &&
VIDEO_V4L2_SUBDEV_API [=n] && MEDIA_CAMERA_SUPPORT [=y]
3)[*] Media Controller API (NEW)
[*] V4L2 sub-device userspace API (NEW)
Depends on: MEDIA_SUPPORT [=m] && I2C [=y] && VIDEO_V4L2 [=m] &&
VIDEO_V4L2_SUBDEV_API [=y] && MEDIA_CAMERA_SUPPORT [=y]
So now all listed dependencies are satisfied but the M-5MOLS drive is not
visible du to default CONFIG_MEDIA_SUBDRV_AUTOSELECT=y
Not sure how I ended up with the VIDEO_IR_I2C dependency - which as you
state - is wrong. though VIDEO_M5MOLS probably needs a
!CONFIG_MEDIA_SUBDRV_AUTOSELECT in the dependency list though.
thx!
hofrat
Re: [PATCH v2 19/26] media: ov9650: fix bogus warnings
On Wed, Nov 01, 2017 at 05:05:56PM -0400, Mauro Carvalho Chehab wrote:
> The smatch logic gets confused with the syntax used to check if the
> ov9650x_read() reads succedded:
> drivers/media/i2c/ov9650.c:895 __g_volatile_ctrl() error: uninitialized
> symbol 'reg2'.
> drivers/media/i2c/ov9650.c:895 __g_volatile_ctrl() error: uninitialized
> symbol 'reg1'.
>
> There's nothing wrong with the original logic, except that
> it is a little more harder to review.
Maybe I do not understand the original logic correctly but
ov965x_read(...) is passing on the return value from
i2c_transfer() -> __i2c_transfer() and thus if one of those
calls would have been a negativ error status it would have simply
executed the next call to ov965x_read() and if that
call would have suceeded it would eventually reach the
line " exposure = ((reg2 & 0x3f) << 10) | (reg1 << 2) |..."
with the potential to operate on uninitialized registers reg0/1/2
the current code sems only to handle error conditions in the last
call to ov965x_read() correctly.
I think this is actually not an equivalent transform but a bug-fix
of case V4L2_CID_EXPOSURE_AUTO: (aside from being a code inconsistency)
So should this not carry a
Fixes: 84a15ded76ec ("[media] V4L: Add driver for OV9650/52 image sensors")
tag ?
thx!
hofrat
>
> So, let's stick with the syntax that won't cause read
> issues.
>
> Signed-off-by: Mauro Carvalho Chehab
Reviewed-by: Nicholas Mc Guire
> ---
> drivers/media/i2c/ov9650.c | 10 ++
> 1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/media/i2c/ov9650.c b/drivers/media/i2c/ov9650.c
> index 69433e1e2533..e519f278d5f9 100644
> --- a/drivers/media/i2c/ov9650.c
> +++ b/drivers/media/i2c/ov9650.c
> @@ -886,10 +886,12 @@ static int __g_volatile_ctrl(struct ov965x *ov965x,
> struct v4l2_ctrl *ctrl)
> if (ctrl->val == V4L2_EXPOSURE_MANUAL)
> return 0;
> ret = ov965x_read(client, REG_COM1, ®0);
> - if (!ret)
> - ret = ov965x_read(client, REG_AECH, ®1);
> - if (!ret)
> - ret = ov965x_read(client, REG_AECHM, ®2);
> + if (ret < 0)
> + return ret;
> + ret = ov965x_read(client, REG_AECH, ®1);
> + if (ret < 0)
> + return ret;
> + ret = ov965x_read(client, REG_AECHM, ®2);
> if (ret < 0)
> return ret;
> exposure = ((reg2 & 0x3f) << 10) | (reg1 << 2) |
> --
> 2.13.6
>
Re: [PATCH v2 19/26] media: ov9650: fix bogus warnings
On Thu, Nov 02, 2017 at 10:06:06AM +, Nicholas Mc Guire wrote:
> On Wed, Nov 01, 2017 at 05:05:56PM -0400, Mauro Carvalho Chehab wrote:
> > The smatch logic gets confused with the syntax used to check if the
> > ov9650x_read() reads succedded:
> > drivers/media/i2c/ov9650.c:895 __g_volatile_ctrl() error: uninitialized
> > symbol 'reg2'.
> > drivers/media/i2c/ov9650.c:895 __g_volatile_ctrl() error: uninitialized
> > symbol 'reg1'.
> >
> > There's nothing wrong with the original logic, except that
> > it is a little more harder to review.
>
> Maybe I do not understand the original logic correctly but
> ov965x_read(...) is passing on the return value from
> i2c_transfer() -> __i2c_transfer() and thus if one of those
> calls would have been a negativ error status it would have simply
> executed the next call to ov965x_read() and if that
> call would have suceeded it would eventually reach the
> line " exposure = ((reg2 & 0x3f) << 10) | (reg1 << 2) |..."
> with the potential to operate on uninitialized registers reg0/1/2
> the current code sems only to handle error conditions in the last
> call to ov965x_read() correctly.
sorry - sent that out too fast - the logic is equivalent the negative
returns are treated correctly because only the success case is
being returned explicidly as 0 in ov965x_read() return statement
by checking for the expecte return of 1 from i2c_transfer() wrapping
it to 0.
sorry for the noise.
thx!
hofrat
>
> I think this is actually not an equivalent transform but a bug-fix
> of case V4L2_CID_EXPOSURE_AUTO: (aside from being a code inconsistency)
> So should this not carry a
> Fixes: 84a15ded76ec ("[media] V4L: Add driver for OV9650/52 image sensors")
> tag ?
>
> thx!
> hofrat
>
> >
> > So, let's stick with the syntax that won't cause read
> > issues.
> >
> > Signed-off-by: Mauro Carvalho Chehab
>
> Reviewed-by: Nicholas Mc Guire
>
> > ---
> > drivers/media/i2c/ov9650.c | 10 ++
> > 1 file changed, 6 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/media/i2c/ov9650.c b/drivers/media/i2c/ov9650.c
> > index 69433e1e2533..e519f278d5f9 100644
> > --- a/drivers/media/i2c/ov9650.c
> > +++ b/drivers/media/i2c/ov9650.c
> > @@ -886,10 +886,12 @@ static int __g_volatile_ctrl(struct ov965x *ov965x,
> > struct v4l2_ctrl *ctrl)
> > if (ctrl->val == V4L2_EXPOSURE_MANUAL)
> > return 0;
> > ret = ov965x_read(client, REG_COM1, ®0);
> > - if (!ret)
> > - ret = ov965x_read(client, REG_AECH, ®1);
> > - if (!ret)
> > - ret = ov965x_read(client, REG_AECHM, ®2);
> > + if (ret < 0)
> > + return ret;
> > + ret = ov965x_read(client, REG_AECH, ®1);
> > + if (ret < 0)
> > + return ret;
> > + ret = ov965x_read(client, REG_AECHM, ®2);
> > if (ret < 0)
> > return ret;
> > exposure = ((reg2 & 0x3f) << 10) | (reg1 << 2) |
> > --
> > 2.13.6
> >
[PATCH RFC] [media] s5k6aa: set usleep_range greater 0
As this is not in atomic context and it does not seem like a critical
timing setting a range of 1ms allows the timer subsystem to optimize
the hrtimer here.
Fixes: commit bfa8dd3a0524 ("[media] v4l: Add v4l2 subdev driver for S5K6AAFX
sensor")
Signed-off-by: Nicholas Mc Guire
---
problem was located by coccinelle spatch
The problem is that usleep_range is calculating the delay by
exp = ktime_add_us(ktime_get(), min)
delta = (u64)(max - min) * NSEC_PER_USEC
so delta is set to 0
and then calls
schedule_hrtimeout_range(exp, 0,...)
effectively this means that the clock subsystem has no room to
optimize which makes little sense as this is not atomic context
anyway so there is not guaratee of precision here.
As this is not a critical delay it is set to a range of 4 to 5
milliseconds - this change needs a review by someone that knows
the details of the device though and preferably would increase
that range.
Patch was only compile tested with: x86_64_defconfig + MEDIA_SUPPORT=m
MEDIA_CAMERA_SUPPORT=y (implies VIDEO_V4L2=m)
MEDIA_DIGITAL_TV_SUPPORT=y, CONFIG_MEDIA_CONTROLLER=y,
CONFIG_VIDEO_V4L2_SUBDEV_API=y
Patch is against 4.9.0 (localversion-next is next-20161212)
drivers/media/i2c/s5k6aa.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/media/i2c/s5k6aa.c b/drivers/media/i2c/s5k6aa.c
index faee113..9fd254a 100644
--- a/drivers/media/i2c/s5k6aa.c
+++ b/drivers/media/i2c/s5k6aa.c
@@ -838,7 +838,7 @@ static int __s5k6aa_power_on(struct s5k6aa *s5k6aa)
if (s5k6aa->s_power)
ret = s5k6aa->s_power(1);
- usleep_range(4000, 4000);
+ usleep_range(4000, 5000);
if (s5k6aa_gpio_deassert(s5k6aa, RST))
msleep(20);
--
2.1.4
--
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] [media] m5mols: set usleep_range delta greater 0
This delay is in non-atomic context and it does not seem to be time-critical so relax it to allow the timer subsystem to optimize hrtimers. Signed-off-by: Nicholas Mc Guire --- problem was located by coccinelle spatch The problem is that usleep_range is calculating the delay by exp = ktime_add_us(ktime_get(), min) delta = (u64)(max - min) * NSEC_PER_USEC so delta is set to 0 and then calls schedule_hrtimeout_range(exp, 0,...) effectively this means that the clock subsystem has no room to optimize which makes little sense as this is not atomic context anyway so there is not guarantee of precision here. As this is not a critical delay and the jitter of any system is in the 10s of microseconds range anyway the range is set to 200 to 300 microseconds - this change cold have a negligible impact on bandwidth (though I doubt this is relevant or even measurable here) thus it needs a review by someone that knows the details of the device and preferably would increase that range. A comment in the second case was added to clarify the intent of the delay as time between i2c transfers. Patch was only compile tested against: x86_64_defconfig + CONFIG_MEDIA_SUPPORT=m MEDIA_CAMERA_SUPPORT=y, VIDEO_V4L2_SUBDEV_API, MEDIA_DIGITAL_TV-_SUPPORT=y MEDIA_RC_SUPPORT=y, MEDIA_CONTROLLER=y, VIDEO_V4L2_SUBDEV_API=y MEDIA_SUBDRV_AUTOSELECT=n, CONFIG_VIDEO_M5MOLS=m Patch is against 4.9.0 (localversion-next is next-20161212) drivers/media/i2c/m5mols/m5mols_core.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/drivers/media/i2c/m5mols/m5mols_core.c b/drivers/media/i2c/m5mols/m5mols_core.c index acb804b..23e8616 100644 --- a/drivers/media/i2c/m5mols/m5mols_core.c +++ b/drivers/media/i2c/m5mols/m5mols_core.c @@ -168,7 +168,7 @@ static int m5mols_read(struct v4l2_subdev *sd, u32 size, u32 reg, u32 *val) msg[1].buf = rbuf; /* minimum stabilization time */ - usleep_range(200, 200); + usleep_range(200, 300); ret = i2c_transfer(client->adapter, msg, 2); @@ -268,7 +268,8 @@ int m5mols_write(struct v4l2_subdev *sd, u32 reg, u32 val) *buf = m5mols_swap_byte((u8 *)&val, size); - usleep_range(200, 200); + /* minimum stabilization time */ + usleep_range(200, 300); ret = i2c_transfer(client->adapter, msg, 1); if (ret == 1) -- 2.1.4 -- 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] [media] m5mols: add missing dependency on VIDEO_IR_I2C
The Depends on: tag in Kconfig for CONFIG_VIDEO_M5MOLS does not list
VIDEO_IR_I2C so Kconfig displays the dependencies needed so the M-5MOLS
driver can not be found.
Fixes: commit cb7a01ac324b ("[media] move i2c files into drivers/media/i2c")
Signed-off-by: Nicholas Mc Guire
---
searching for VIDEO_M5MOLS in menuconfig currently shows the following
dependencies
Depends on: MEDIA_SUPPORT [=m] && I2C [=y] && VIDEO_V4L2 [=m] && \
VIDEO_V4L2_SUBDEV_API [=y] && MEDIA_CAMERA_SUPPORT [=y]
but as the default settings include MEDIA_SUBDRV_AUTOSELECT=y the
"I2C module for IR" submenu (CONFIG_VIDEO_IR_I2C) is not displayed
adding the VIDEO_IR_I2C to the dependency list makes this clear
Q: should a patch like this carry a Fixes: tag ?
Patch was tested against: x86_64_defconfig
Patch is against 4.9.0 (localversion-next is next-20161212)
drivers/media/i2c/m5mols/Kconfig | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/media/i2c/m5mols/Kconfig b/drivers/media/i2c/m5mols/Kconfig
index dc8c250..6847a1b 100644
--- a/drivers/media/i2c/m5mols/Kconfig
+++ b/drivers/media/i2c/m5mols/Kconfig
@@ -1,6 +1,6 @@
config VIDEO_M5MOLS
tristate "Fujitsu M-5MOLS 8MP sensor support"
- depends on I2C && VIDEO_V4L2 && VIDEO_V4L2_SUBDEV_API
+ depends on I2C && VIDEO_IR_I2C && VIDEO_V4L2 && VIDEO_V4L2_SUBDEV_API
depends on MEDIA_CAMERA_SUPPORT
---help---
This driver supports Fujitsu M-5MOLS camera sensor with ISP
--
2.1.4
--
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] [media] s5k6aa: set usleep_range greater 0
On Tue, Dec 13, 2016 at 03:53:47PM +0100, Sylwester Nawrocki wrote: > Hi Laurent, > > On 12/13/2016 03:10 PM, Laurent Pinchart wrote: > > As pointed out by Ian Arkver, the datasheet states the delay should be > > >50µs. > > Would it make sense to reduce the sleep duration to (3000, 4000) for > > instance > > (or possibly even lower), instead of increasing it ? > > Theoretically it would make sense, I believe the delay call should really > be part of the set_power callback. I think it is safe to decrease the > delay value now, the boards using that driver have been dropped with commit > > commit ca9143501c30a2ce5886757961408488fac2bb4c > ARM: EXYNOS: Remove unused board files > > As far as I am concerned you can do whatever you want with that delay > call, remove it or decrease value, whatever helps to stop triggering > warnings from the static analysis tools. > if its actually unused then it might be best to completely drop the code raher than fixing up dead-code. Is the EXYNOS the only system that had this device in use ? If it shold stay in then setting it to the above proposed 3000, 4000 would seem the most resonable to me as I asume this change would stay untested. thx! hofrat -- 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] [media] gscpa_m5602: use msecs_to_jiffies for conversions
API compliance scanning with coccinelle flagged:
./drivers/media/usb/gspca/m5602/m5602_s5k83a.c:180:9-25:
WARNING: timeout (100) seems HZ dependent
Numeric constants passed to schedule_timeout() make the effective
timeout HZ dependent which makes little sense in a polling loop for
the cameras rotation state.
Fixed up by converting the constant to jiffies with msecs_to_jiffies()
Signed-off-by: Nicholas Mc Guire
---
Patch was compile tested with i386_defconfig +
Patch is against 4.1-rc6 (localversion-next is -next-20150605)
drivers/media/usb/gspca/m5602/m5602_s5k83a.c |2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/media/usb/gspca/m5602/m5602_s5k83a.c
b/drivers/media/usb/gspca/m5602/m5602_s5k83a.c
index 7cbc3a0..bf6b215 100644
--- a/drivers/media/usb/gspca/m5602/m5602_s5k83a.c
+++ b/drivers/media/usb/gspca/m5602/m5602_s5k83a.c
@@ -177,7 +177,7 @@ static int rotation_thread_function(void *data)
__s32 vflip, hflip;
set_current_state(TASK_INTERRUPTIBLE);
- while (!schedule_timeout(100)) {
+ while (!schedule_timeout(msecs_to_jiffies(100))) {
if (mutex_lock_interruptible(&sd->gspca_dev.usb_lock))
break;
--
1.7.10.4
--
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] [media] ddbridge: fix wait_event_timeout return handling
API conformance testing for completions with coccinelle spatches are being
used to locate API usage inconsistencies:
./drivers/media/pci/ddbridge/ddbridge-core.c:89
incorrect check for negative return
Return type of wait_event_timeout is signed long not int and the
return type is >=0 always thus the negative check is unnecessary..
As stat is used here exclusively its type is simply changed and the
negative return check dropped.
Signed-off-by: Nicholas Mc Guire
---
Patch was compile tested with x86_64_defconfig + CONFIG_MEDIA_SUPPORT=m,
MEDIA_DIGITAL_TV_SUPPORT=y, CONFIG_MEDIA_PCI_SUPPORT=y,
CONFIG_DVB_DDBRIDGE=m
Patch is against 4.1-rc7 (localversion-next is -next-20150609)
drivers/media/pci/ddbridge/ddbridge-core.c |4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/media/pci/ddbridge/ddbridge-core.c
b/drivers/media/pci/ddbridge/ddbridge-core.c
index 9e3492e..7f1b3b3 100644
--- a/drivers/media/pci/ddbridge/ddbridge-core.c
+++ b/drivers/media/pci/ddbridge/ddbridge-core.c
@@ -81,13 +81,13 @@ static int i2c_read_reg16(struct i2c_adapter *adapter, u8
adr,
static int ddb_i2c_cmd(struct ddb_i2c *i2c, u32 adr, u32 cmd)
{
struct ddb *dev = i2c->dev;
- int stat;
+ long stat;
u32 val;
i2c->done = 0;
ddbwritel((adr << 9) | cmd, i2c->regs + I2C_COMMAND);
stat = wait_event_timeout(i2c->wq, i2c->done == 1, HZ);
- if (stat <= 0) {
+ if (stat == 0) {
printk(KERN_ERR "I2C timeout\n");
{ /* MSI debugging*/
u32 istat = ddbreadl(INTERRUPT_STATUS);
--
1.7.10.4
--
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] [media] s5p-tv: fix wait_event_timeout return handling
event API conformance testing with coccinelle spatches are being
used to locate API usage inconsistencies this triggert with:
./drivers/media/platform/s5p-tv/mixer_reg.c:364
incorrect check for negative return
Return type of wait_event_timeout is signed long not int and the
return type is >=0 always thus the negative check is unnecessary.
An appropriately named variable of type long is inserted and the
call fixed up aswell as the negative return check dropped.
Signed-off-by: Nicholas Mc Guire
---
Minor change of indentation to make checkpatch happy - not sure if
this is much more readable though.
Patch was compile tested with exynos_defconfig + CONFIG_MEDIA_SUPPORT=m,
CONFIG_MEDIA_CAMERA_SUPPORT=y, CONFIG_MEDIA_ANALOG_TV_SUPPORT=y,
CONFIG_V4L_PLATFORM_DRIVERS=y, CONFIG_VIDEO_SAMSUNG_S5P_TV=y,
CONFIG_VIDEO_SAMSUNG_S5P_MIXER=m
Patch is against 4.1-rc7 (localversion-next is -next-20150609)
drivers/media/platform/s5p-tv/mixer_reg.c | 12 +---
1 file changed, 5 insertions(+), 7 deletions(-)
diff --git a/drivers/media/platform/s5p-tv/mixer_reg.c
b/drivers/media/platform/s5p-tv/mixer_reg.c
index b713403..5127acb 100644
--- a/drivers/media/platform/s5p-tv/mixer_reg.c
+++ b/drivers/media/platform/s5p-tv/mixer_reg.c
@@ -357,17 +357,15 @@ void mxr_reg_streamoff(struct mxr_device *mdev)
int mxr_reg_wait4vsync(struct mxr_device *mdev)
{
- int ret;
+ long time_left;
clear_bit(MXR_EVENT_VSYNC, &mdev->event_flags);
/* TODO: consider adding interruptible */
- ret = wait_event_timeout(mdev->event_queue,
- test_bit(MXR_EVENT_VSYNC, &mdev->event_flags),
- msecs_to_jiffies(1000));
- if (ret > 0)
+ time_left = wait_event_timeout(mdev->event_queue,
+ test_bit(MXR_EVENT_VSYNC, &mdev->event_flags),
+msecs_to_jiffies(1000));
+ if (time_left > 0)
return 0;
- if (ret < 0)
- return ret;
mxr_warn(mdev, "no vsync detected - timeout\n");
return -ETIME;
}
--
1.7.10.4
--
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] pvrusb2: use msecs_to_jiffies for conversion
This is only an API consolidation and should make things more readable
Signed-off-by: Nicholas Mc Guire
---
Converting milliseconds to jiffies by val * HZ / 1000 is technically
not wrong but msecs_to_jiffies(val) is the cleaner solution and handles
all corner cases correctly. This is a minor API cleanup only.
This patch was only compile tested with x86_64_defconfig +
CONFIG_MEDIA_SUPPORT=m, CONFIG_MEDIA_DIGITAL_TV_SUPPORT=y,
CONFIG_MEDIA_ANALOG_TV_SUPPORT=y, CONFIG_MEDIA_USB_SUPPORT=y,
CONFIG_VIDEO_PVRUSB2=m, CONFIG_VIDEO_PVRUSB2_DVB=y
Patch is against 3.19.0-rc5 -next-20150119
drivers/media/usb/pvrusb2/pvrusb2-hdw.c | 19 ---
1 file changed, 8 insertions(+), 11 deletions(-)
diff --git a/drivers/media/usb/pvrusb2/pvrusb2-hdw.c
b/drivers/media/usb/pvrusb2/pvrusb2-hdw.c
index 2fd9b5e..ee187c1 100644
--- a/drivers/media/usb/pvrusb2/pvrusb2-hdw.c
+++ b/drivers/media/usb/pvrusb2/pvrusb2-hdw.c
@@ -4301,9 +4301,8 @@ static int state_eval_encoder_config(struct pvr2_hdw *hdw)
the encoder. */
if (!hdw->state_encoder_waitok) {
hdw->encoder_wait_timer.expires =
- jiffies +
- (HZ * TIME_MSEC_ENCODER_WAIT
-/ 1000);
+ jiffies + msecs_to_jiffies(
+ TIME_MSEC_ENCODER_WAIT);
add_timer(&hdw->encoder_wait_timer);
}
}
@@ -4426,8 +4425,8 @@ static int state_eval_encoder_run(struct pvr2_hdw *hdw)
if (pvr2_encoder_start(hdw) < 0) return !0;
hdw->state_encoder_run = !0;
if (!hdw->state_encoder_runok) {
- hdw->encoder_run_timer.expires =
- jiffies + (HZ * TIME_MSEC_ENCODER_OK / 1000);
+ hdw->encoder_run_timer.expires = jiffies +
+msecs_to_jiffies(TIME_MSEC_ENCODER_OK);
add_timer(&hdw->encoder_run_timer);
}
}
@@ -4518,9 +4517,8 @@ static int state_eval_decoder_run(struct pvr2_hdw *hdw)
but before we did the pending check. */
if (!hdw->state_decoder_quiescent) {
hdw->quiescent_timer.expires =
- jiffies +
- (HZ * TIME_MSEC_DECODER_WAIT
-/ 1000);
+ jiffies + msecs_to_jiffies(
+ TIME_MSEC_DECODER_WAIT);
add_timer(&hdw->quiescent_timer);
}
}
@@ -4544,9 +4542,8 @@ static int state_eval_decoder_run(struct pvr2_hdw *hdw)
hdw->state_decoder_run = !0;
if (hdw->decoder_client_id == PVR2_CLIENT_ID_SAA7115) {
hdw->decoder_stabilization_timer.expires =
- jiffies +
- (HZ * TIME_MSEC_DECODER_STABILIZATION_WAIT /
-1000);
+ jiffies + msecs_to_jiffies(
+ TIME_MSEC_DECODER_STABILIZATION_WAIT);
add_timer(&hdw->decoder_stabilization_timer);
} else {
hdw->state_decoder_ready = !0;
--
1.7.10.4
--
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] staging: media: davinci_vpfe: drop condition with no effect
As the if and else branch body are identical the condition has no effect and
can be dropped.
Signed-off-by: Nicholas Mc Guire
---
As the if and the else branch of the inner conditional paths are the same
the condition is without effect. Given the comments indicate that
the else branch *should* be handling a specific case this may indicate
a bug, in which case the below patch is *wrong*. This needs a review by
someone that knows the specifics of this driver.
If the inner if/else is a placeholder for planed updates then it should
be commented so this is clear.
Patch was only compile tested with davinci_all_defconfig + CONFIG_STAGING=y
CONFIG_STAGING_MEDIA=y, CONFIG_MEDIA_SUPPORT=m,
CONFIG_MEDIA_ANALOG_TV_SUPPORT=y, CONFIG_MEDIA_CAMERA_SUPPORT=y
CONFIG_MEDIA_CONTROLLER=y, CONFIG_VIDEO_V4L2_SUBDEV_API=y
CONFIG_VIDEO_DM365_VPFE=m
Patch is against 3.0.19-rc5 -next-20150123
drivers/staging/media/davinci_vpfe/dm365_resizer.c |9 ++---
1 file changed, 2 insertions(+), 7 deletions(-)
diff --git a/drivers/staging/media/davinci_vpfe/dm365_resizer.c
b/drivers/staging/media/davinci_vpfe/dm365_resizer.c
index 75e70e1..bf2cb7a 100644
--- a/drivers/staging/media/davinci_vpfe/dm365_resizer.c
+++ b/drivers/staging/media/davinci_vpfe/dm365_resizer.c
@@ -63,16 +63,11 @@ resizer_calculate_line_length(u32 pix, int width, int
height,
if (pix == MEDIA_BUS_FMT_UYVY8_2X8 ||
pix == MEDIA_BUS_FMT_SGRBG12_1X12) {
*line_len = width << 1;
- } else if (pix == MEDIA_BUS_FMT_Y8_1X8 ||
- pix == MEDIA_BUS_FMT_UV8_1X8) {
- *line_len = width;
- *line_len_c = width;
- } else {
- /* YUV 420 */
- /* round width to upper 32 byte boundary */
+ } else {
*line_len = width;
*line_len_c = width;
}
+
/* adjust the line len to be a multiple of 32 */
*line_len += 31;
*line_len &= ~0x1f;
--
1.7.10.4
--
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] [media] cx231xx: drop condition with no effect
The if and the else code are identical - so the condition has no effect
on the effective code.
This patch removes the condition and the duplicated code.
Signed-off-by: Nicholas Mc Guire
---
The if and the else code are identical - so the condition has no effect.
The value of the assignment was placed into () for readability as other long
bit-wise constructs in this file also do so.
Patch was only compile tested with imx_v6_v7_defconfig
CONFIG_MEDIA_ANALOG_TV_SUPPORT=y, CONFIG_MEDIA_DIGITAL_TV_SUPPORT=y,
CONFIG_VIDEO_CX231XX=m
Patch is against 3.19.0-rc7 (localversion-next is -next-20150204)
drivers/media/usb/cx231xx/cx231xx-core.c | 13 +++--
1 file changed, 3 insertions(+), 10 deletions(-)
diff --git a/drivers/media/usb/cx231xx/cx231xx-core.c
b/drivers/media/usb/cx231xx/cx231xx-core.c
index 4a3f28c..832ba99 100644
--- a/drivers/media/usb/cx231xx/cx231xx-core.c
+++ b/drivers/media/usb/cx231xx/cx231xx-core.c
@@ -176,16 +176,9 @@ int cx231xx_send_usb_command(struct cx231xx_i2c *i2c_bus,
saddr_len = req_data->saddr_len;
/* Set wValue */
- if (saddr_len == 1) /* need check saddr_len == 0 */
- ven_req.wValue =
- req_data->
- dev_addr << 9 | _i2c_period << 4 | saddr_len << 2 |
- _i2c_nostop << 1 | I2C_SYNC | _i2c_reserve << 6;
- else
- ven_req.wValue =
- req_data->
- dev_addr << 9 | _i2c_period << 4 | saddr_len << 2 |
- _i2c_nostop << 1 | I2C_SYNC | _i2c_reserve << 6;
+ ven_req.wValue = (req_data->dev_addr << 9 | _i2c_period << 4 |
+ saddr_len << 2 | _i2c_nostop << 1 | I2C_SYNC |
+ _i2c_reserve << 6);
/* set channel number */
if (req_data->direction & I2C_M_RD) {
--
1.7.10.4
--
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] media: radio: assign wait_for_completion_timeout to appropriately typed var
wait_for_completion_timeout() returns unsigned long not int. This assigns
the return value to an appropriately typed variable (also helps keep
static code checkers happy).
Signed-off-by: Nicholas Mc Guire
---
Patch was only compile tested with x86_64_defconfig + CONFIG_MEDIA_SUPPORT=m
CONFIG_MEDIA_CAMERA_SUPPORT=y, CONFIG_V4L_PLATFORM_DRIVERS=y,
CONFIG_MEDIA_RADIO_SUPPORT=y, RADIO_ADAPTER=y, CONFIG_RADIO_WL1273=m
Patch is against 3.19.0-rc7 (localversion-next is -next-20150204)
drivers/media/radio/radio-wl1273.c | 18 ++
1 files changed, 10 insertions(+), 8 deletions(-)
diff --git a/drivers/media/radio/radio-wl1273.c
b/drivers/media/radio/radio-wl1273.c
index b8f3644..571c7f6 100644
--- a/drivers/media/radio/radio-wl1273.c
+++ b/drivers/media/radio/radio-wl1273.c
@@ -347,6 +347,7 @@ static int wl1273_fm_set_tx_freq(struct wl1273_device
*radio, unsigned int freq)
{
struct wl1273_core *core = radio->core;
int r = 0;
+ unsigned long t;
if (freq < WL1273_BAND_TX_LOW) {
dev_err(radio->dev,
@@ -378,11 +379,11 @@ static int wl1273_fm_set_tx_freq(struct wl1273_device
*radio, unsigned int freq)
reinit_completion(&radio->busy);
/* wait for the FR IRQ */
- r = wait_for_completion_timeout(&radio->busy, msecs_to_jiffies(2000));
- if (!r)
+ t = wait_for_completion_timeout(&radio->busy, msecs_to_jiffies(2000));
+ if (!t)
return -ETIMEDOUT;
- dev_dbg(radio->dev, "WL1273_CHANL_SET: %d\n", r);
+ dev_dbg(radio->dev, "WL1273_CHANL_SET: %lu\n", t);
/* Enable the output power */
r = core->write(core, WL1273_POWER_ENB_SET, 1);
@@ -392,12 +393,12 @@ static int wl1273_fm_set_tx_freq(struct wl1273_device
*radio, unsigned int freq)
reinit_completion(&radio->busy);
/* wait for the POWER_ENB IRQ */
- r = wait_for_completion_timeout(&radio->busy, msecs_to_jiffies(1000));
- if (!r)
+ t = wait_for_completion_timeout(&radio->busy, msecs_to_jiffies(1000));
+ if (!t)
return -ETIMEDOUT;
radio->tx_frequency = freq;
- dev_dbg(radio->dev, "WL1273_POWER_ENB_SET: %d\n", r);
+ dev_dbg(radio->dev, "WL1273_POWER_ENB_SET: %lu\n", t);
return 0;
}
@@ -406,6 +407,7 @@ static int wl1273_fm_set_rx_freq(struct wl1273_device
*radio, unsigned int freq)
{
struct wl1273_core *core = radio->core;
int r, f;
+ unsigned long t;
if (freq < radio->rangelow) {
dev_err(radio->dev,
@@ -446,8 +448,8 @@ static int wl1273_fm_set_rx_freq(struct wl1273_device
*radio, unsigned int freq)
reinit_completion(&radio->busy);
- r = wait_for_completion_timeout(&radio->busy, msecs_to_jiffies(2000));
- if (!r) {
+ t = wait_for_completion_timeout(&radio->busy, msecs_to_jiffies(2000));
+ if (!t) {
dev_err(radio->dev, "%s: TIMEOUT\n", __func__);
return -ETIMEDOUT;
}
--
1.7.10.4
--
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] media: radio: handle timeouts
Add handling for timeout case.
Signed-off-by: Nicholas Mc Guire
---
Some error state/error information seems be get lost int the current code.
(line-numbers are from 3.19.0-rc7.
Assume that on line 827 core->write succeeds but the following
wait_for_completion_timeout times out and the radio->irq_received condition
is not satisfied resulting in goto out;
827 r = core->write(core, WL1273_TUNER_MODE_SET, TUNER_MODE_AUTO_SEEK);
828 if (r)
829 goto out;
830
831 wait_for_completion_timeout(&radio->busy, msecs_to_jiffies(1000));
832 if (!(radio->irq_received & WL1273_BL_EVENT))
833 goto out;
A similar situation is at line 955 - 859 where a tiemout could occure
and the reported value would be the success value from core->write.
852 reinit_completion(&radio->busy);
853 dev_dbg(radio->dev, "%s: BUSY\n", __func__);
854
855 r = core->write(core, WL1273_TUNER_MODE_SET, TUNER_MODE_AUTO_SEEK);
856 if (r)
857 goto out;
858
859 wait_for_completion_timeout(&radio->busy, msecs_to_jiffies(1000)
the problem is that the value of r now is the "success" value from core->write
and any timeout and/or failure to detect the expected interrupt is not
reported in
860 out:
861 dev_dbg(radio->dev, "%s: Err: %d\n", __func__, r);
862 return r;
Should the wait_for_completion_timeout not report the timeout event by setting
r to -ETIMEOUT ? respectively use if (!(radio->irq_received & WL1273_BL_EVENT))
to check and set -ETIMEOUT there ?
Comparing this with wl1273_fm_set_tx_freq - the below patch might be suitable
way to handle timeout - but this needs a review by someone who knows the
details of the driver - so this is really just a guess.
Patch was only compile tested with x86_64_defconfig + CONFIG_MEDIA_SUPPORT=m
CONFIG_MEDIA_CAMERA_SUPPORT=y, CONFIG_V4L_PLATFORM_DRIVERS=y,
CONFIG_MEDIA_RADIO_SUPPORT=y, RADIO_ADAPTER=y, CONFIG_RADIO_WL1273=m
Patch is against 3.19.0-rc7 (localversion-next is -next-20150204)
drivers/media/radio/radio-wl1273.c |9 +++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/drivers/media/radio/radio-wl1273.c
b/drivers/media/radio/radio-wl1273.c
index 571c7f6..6830523 100644
--- a/drivers/media/radio/radio-wl1273.c
+++ b/drivers/media/radio/radio-wl1273.c
@@ -828,9 +828,12 @@ static int wl1273_fm_set_seek(struct wl1273_device *radio,
if (r)
goto out;
+ /* wait for the FR IRQ */
wait_for_completion_timeout(&radio->busy, msecs_to_jiffies(1000));
- if (!(radio->irq_received & WL1273_BL_EVENT))
+ if (!(radio->irq_received & WL1273_BL_EVENT)) {
+ r = -ETIMEDOUT;
goto out;
+ }
radio->irq_received &= ~WL1273_BL_EVENT;
@@ -856,7 +859,9 @@ static int wl1273_fm_set_seek(struct wl1273_device *radio,
if (r)
goto out;
- wait_for_completion_timeout(&radio->busy, msecs_to_jiffies(1000));
+ /* wait for the FR IRQ */
+ if (!wait_for_completion_timeout(&radio->busy, msecs_to_jiffies(1000)))
+ r = -ETIMEDOUT;
out:
dev_dbg(radio->dev, "%s: Err: %d\n", __func__, r);
return r;
--
1.7.10.4
--
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] [media] si470x: fixup wait_for_completion_timeout return handling
return type of wait_for_completion_timeout is unsigned long not int. A
appropriately named variable of type unsigned long is added and the
assignments fixed up.
Signed-off-by: Nicholas Mc Guire
---
Patch was only compile tested with
Patch is against 3.19.0 (localversion-next is -next-20150211)
drivers/media/radio/si470x/radio-si470x-common.c | 14 --
1 file changed, 8 insertions(+), 6 deletions(-)
diff --git a/drivers/media/radio/si470x/radio-si470x-common.c
b/drivers/media/radio/si470x/radio-si470x-common.c
index 909c3f9..1d827ad 100644
--- a/drivers/media/radio/si470x/radio-si470x-common.c
+++ b/drivers/media/radio/si470x/radio-si470x-common.c
@@ -208,6 +208,7 @@ static int si470x_set_band(struct si470x_device *radio, int
band)
static int si470x_set_chan(struct si470x_device *radio, unsigned short chan)
{
int retval;
+ unsigned long time_left;
bool timed_out = false;
/* start tuning */
@@ -219,9 +220,9 @@ static int si470x_set_chan(struct si470x_device *radio,
unsigned short chan)
/* wait till tune operation has completed */
reinit_completion(&radio->completion);
- retval = wait_for_completion_timeout(&radio->completion,
- msecs_to_jiffies(tune_timeout));
- if (!retval)
+ time_left = wait_for_completion_timeout(&radio->completion,
+ msecs_to_jiffies(tune_timeout));
+ if (time_left == 0)
timed_out = true;
if ((radio->registers[STATUSRSSI] & STATUSRSSI_STC) == 0)
@@ -301,6 +302,7 @@ static int si470x_set_seek(struct si470x_device *radio,
int band, retval;
unsigned int freq;
bool timed_out = false;
+ unsigned long time_left;
/* set band */
if (seek->rangelow || seek->rangehigh) {
@@ -342,9 +344,9 @@ static int si470x_set_seek(struct si470x_device *radio,
/* wait till tune operation has completed */
reinit_completion(&radio->completion);
- retval = wait_for_completion_timeout(&radio->completion,
- msecs_to_jiffies(seek_timeout));
- if (!retval)
+ time_left = wait_for_completion_timeout(&radio->completion,
+ msecs_to_jiffies(seek_timeout));
+ if (time_left == 0)
timed_out = true;
if ((radio->registers[STATUSRSSI] & STATUSRSSI_STC) == 0)
--
1.7.10.4
--
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 v2] [media] si470x: fixup wait_for_completion_timeout return handling
return type of wait_for_completion_timeout is unsigned long not int. A
appropriately named variable of type unsigned long is added and the
assignments fixed up.
Signed-off-by: Nicholas Mc Guire
---
v2: added config used for compile testing.
Patch was only compile tested with x86_64_defconfig + CONFIG_USB=m,
CONFIG_MEDIA_SUPPORT=m, CONFIG_MEDIA_ANALOG_TV_SUPPORT=y
CONFIG_MEDIA_DIGITAL_TV_SUPPORT=y, (implies VIDEO_V4L2=m)
CONFIG_MEDIA_RADIO_SUPPORT=y, CONFIG_RADIO_SI470X=y
CONFIG_USB_SI470X=m
Patch is against 3.19.0 (localversion-next is -next-20150211)
drivers/media/radio/si470x/radio-si470x-common.c | 14 --
1 file changed, 8 insertions(+), 6 deletions(-)
diff --git a/drivers/media/radio/si470x/radio-si470x-common.c
b/drivers/media/radio/si470x/radio-si470x-common.c
index 909c3f9..1d827ad 100644
--- a/drivers/media/radio/si470x/radio-si470x-common.c
+++ b/drivers/media/radio/si470x/radio-si470x-common.c
@@ -208,6 +208,7 @@ static int si470x_set_band(struct si470x_device *radio, int
band)
static int si470x_set_chan(struct si470x_device *radio, unsigned short chan)
{
int retval;
+ unsigned long time_left;
bool timed_out = false;
/* start tuning */
@@ -219,9 +220,9 @@ static int si470x_set_chan(struct si470x_device *radio,
unsigned short chan)
/* wait till tune operation has completed */
reinit_completion(&radio->completion);
- retval = wait_for_completion_timeout(&radio->completion,
- msecs_to_jiffies(tune_timeout));
- if (!retval)
+ time_left = wait_for_completion_timeout(&radio->completion,
+ msecs_to_jiffies(tune_timeout));
+ if (time_left == 0)
timed_out = true;
if ((radio->registers[STATUSRSSI] & STATUSRSSI_STC) == 0)
@@ -301,6 +302,7 @@ static int si470x_set_seek(struct si470x_device *radio,
int band, retval;
unsigned int freq;
bool timed_out = false;
+ unsigned long time_left;
/* set band */
if (seek->rangelow || seek->rangehigh) {
@@ -342,9 +344,9 @@ static int si470x_set_seek(struct si470x_device *radio,
/* wait till tune operation has completed */
reinit_completion(&radio->completion);
- retval = wait_for_completion_timeout(&radio->completion,
- msecs_to_jiffies(seek_timeout));
- if (!retval)
+ time_left = wait_for_completion_timeout(&radio->completion,
+ msecs_to_jiffies(seek_timeout));
+ if (time_left == 0)
timed_out = true;
if ((radio->registers[STATUSRSSI] & STATUSRSSI_STC) == 0)
--
1.7.10.4
--
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] [media] dib0700: remove redundant else
The if and else are identical and can be consolidated here.
Fixes: commit 91be260faaf8 ("[media] dib8000: Add support for Mygica/Geniatech
S2870")
Signed-off-by: Nicholas Mc Guire
---
Problem found by coccinelle script
Based only on reviewing this driver it seems that the dib0090_config
is not an array and thus this is a cut&past bug - but not having access
to the driver I can not say. Other cases that have the
conditioning on (adap->id == 0) e.g. dib7070p_dib0070 actually have
a config array (dib7070p_dib0070_config[]). So the if/else here most
likely is unnecessary.
The patch is actually a partial revert of commit 91be260faaf8 ("[media]
dib8000: Add support for Mygica/Geniatech S2870") where this if/else
was deliberately introduced but without any specific comments.
This needs a review by someone that has access to the details of the driver.
Patch was compile tested with: x86_64_defconfig + CONFIG_MEDIA_SUPPORT=m,
CONFIG_MEDIA_USB_SUPPORT=y, CONFIG_MEDIA_DIGITAL_TV_SUPPORT=y,
CONFIG_DVB_USB=m, CONFIG_DVB_USB_V2=m, CONFIG_MEDIA_RC_SUPPORT=y,
CONFIG_DVB_USB_DIB0700=m
Patch is against 4.8.0 (localversion-next is -next-20161006)
drivers/media/usb/dvb-usb/dib0700_devices.c | 10 +++---
1 file changed, 3 insertions(+), 7 deletions(-)
diff --git a/drivers/media/usb/dvb-usb/dib0700_devices.c
b/drivers/media/usb/dvb-usb/dib0700_devices.c
index 0857b56..3cd8566 100644
--- a/drivers/media/usb/dvb-usb/dib0700_devices.c
+++ b/drivers/media/usb/dvb-usb/dib0700_devices.c
@@ -1736,13 +1736,9 @@ static int dib809x_tuner_attach(struct dvb_usb_adapter
*adap)
struct dib0700_adapter_state *st = adap->priv;
struct i2c_adapter *tun_i2c =
st->dib8000_ops.get_i2c_master(adap->fe_adap[0].fe,
DIBX000_I2C_INTERFACE_TUNER, 1);
- if (adap->id == 0) {
- if (dvb_attach(dib0090_register, adap->fe_adap[0].fe, tun_i2c,
&dib809x_dib0090_config) == NULL)
- return -ENODEV;
- } else {
- if (dvb_attach(dib0090_register, adap->fe_adap[0].fe, tun_i2c,
&dib809x_dib0090_config) == NULL)
- return -ENODEV;
- }
+ if (dvb_attach(dib0090_register, adap->fe_adap[0].fe,
+ tun_i2c, &dib809x_dib0090_config) == NULL)
+ return -ENODEV;
st->set_param_save = adap->fe_adap[0].fe->ops.tuner_ops.set_params;
adap->fe_adap[0].fe->ops.tuner_ops.set_params =
dib8096_set_param_override;
--
2.1.4
--
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
RFC - unclear change in "[media] DiBxxxx: Codingstype updates"
Hi Olivier !
in your commit 28fafca78797b ("[media] DiB0090: misc improvements")
with commit message:
This patch adds several performance improvements and prepares the
usage of firmware-based devices.
it seems you changed the logic of an if/else in dib0090_tune() in a way
that I do not understand:
- lo6 |= (1 << 2) | 2;
- else
- lo6 |= (1 << 2) | 1;
+ lo6 |= (1 << 2) | 2;//SigmaDelta and Dither
+ else {
+ if (state->identity.in_soc)
+ lo6 |= (1 << 2) | 2;//SigmaDelta and Dither
+ else
+ lo6 |= (1 << 2) | 2;//SigmaDelta and Dither
+ }
resulting in the current code-base of:
if (Rest > 0) {
if (state->config->analog_output)
lo6 |= (1 << 2) | 2;
else {
if (state->identity.in_soc)
lo6 |= (1 << 2) | 2;
else
lo6 |= (1 << 2) | 2;
}
Den = 255;
}
The problem now is that the if and the else(if/else) are
all the same and thus the conditions have no effect. Further
the origninal code actually had different if/else - so I
wonder if this is a cut&past bug here ?
With no knowlege of the device providing a patch makes
no sense as it would just be guessing - in any case this looks
wrong (or atleast should have a comment if it actually is correct)
What am I missing ?
thx!
hofrat
--
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: RFC - unclear change in "[media] DiBxxxx: Codingstype updates"
On Mon, Oct 10, 2016 at 08:31:12AM +0200, Patrick Boettcher wrote:
> Hi, der Herr Hofrat ;-)
>
> On Sat, 8 Oct 2016 13:57:14 +
> Nicholas Mc Guire wrote:
> > - lo6 |= (1 << 2) | 2;
> > - else
> > - lo6 |= (1 << 2) | 1;
> > + lo6 |= (1 << 2) | 2;//SigmaDelta and Dither
> > + else {
> > + if (state->identity.in_soc)
> > + lo6 |= (1 << 2) | 2;//SigmaDelta and
> > Dither
> > + else
> > + lo6 |= (1 << 2) | 2;//SigmaDelta and
> > Dither
> > + }
> >
> > resulting in the current code-base of:
> >
> >if (Rest > 0) {
> >if (state->config->analog_output)
> >lo6 |= (1 << 2) | 2;
> >else {
> >if (state->identity.in_soc)
> >lo6 |= (1 << 2) | 2;
> >else
> >lo6 |= (1 << 2) | 2;
> >}
> >Den = 255;
> >}
> >
> > The problem now is that the if and the else(if/else) are
> > all the same and thus the conditions have no effect. Further
> > the origninal code actually had different if/else - so I
> > wonder if this is a cut&past bug here ?
>
> I may answer on behalf of Olivier (didn't his address bounce?).
>
> I don't remember the details, this patch must date from 2011 or
> before, but at that time we generated the linux-driver from our/their
> internal sources.
>
> Updates in this area were achieved by a lot of thinking + a mix of trial
> and error (after hours/days/weeks of RF hardware validation).
>
> This logic above has 3 possibilities:
>
> - we use the analog-output, or
> - we are using the digital one, then there is whether we are being in
> a SoC or not (SIP or sinlge chip).
>
> At some point in time all values have been different. In the end, they
> aren't anymore, but in case someone wants to try a different value,
> there are placeholders in the code to easily inject these values.
>
> Now the device is stable, maybe even obsolete. We could remove all the
> branches resulting in the same value for lo6.
>
ok - so as the value for lo6 effectively is the same for all conditions
given (1 << 2) | 2 == 6
this might be simplified to and commented as:
if (Rest > 0) {
/* Based on trial and error */
lo6 |= 6;
Den = 255;
}
let me know if that sounds resonable - just plugging in a magic number
sounds like a bad idea - even if this comment might not be wildly enlightening
it atleast indicates that it is known "magic".
thx!
Der Herr Hofrat
--
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
