Re: [PATCH/RFC v3 5/5] media: Add registration helpers for V4L2 flash sub-devices
Hi Sakari, On 05/02/2014 01:06 PM, Sakari Ailus wrote: [...] +static inline enum led_brightness v4l2_flash_intensity_to_led_brightness( + struct led_ctrl *config, + u32 intensity) Fits on a single line. +{ + return intensity / config-step; Shouldn't you first decrement the minimum before the division? Brightness level 0 means that led is off. Let's consider following case: intensity - 15625 config-step - 15625 intensity / config-step = 1 (the lowest possible current level) In V4L2 controls the minimum is not off, and zero might not be a possible value since minimum isn't divisible by step. I wonder how to best take that into account. I've assumed that in MODE_TORCH a led is always on. Switching the mode to MODE_FLASH or MODE_OFF turns the led off. This way we avoid the problem with converting 0 uA value to led_brightness, as available torch brightness levels start from the minimum current level value and turning the led off is accomplished on transition to MODE_OFF or MODE_FLASH, by calling brightness_set op with led_brightness = 0. I'm not sure if we understood the issue the same way. My concern was that if the intensity isn't a multiple of step (but intensity - min is), the above formula won't return a valid result (unless I miss something). Please note that v4l2_flash_intensity_to_led_brightness is called only from s_ctrl callback, and thus it expects to get the intensity aligned to the step value, so it will always be a multiple of step. Is it possible that s_ctrl callback would be passed a non-aligned control value? Regards, Jacek Anaszewski -- 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 v3 3/5] leds: Add support for max77693 mfd flash cell
Hi Jacek, On Mon, Apr 28, 2014 at 01:27:27PM +0200, Jacek Anaszewski wrote: ... +static void max77693_brightness_set_work(struct work_struct *work) +{ + struct max77693_led *led = + container_of(work, struct max77693_led, work_brightness_set); + int ret; + + mutex_lock(led-lock); + + if (led-torch_brightness == 0) { + ret = max77693_clear_mode(led, MAX77693_MODE_TORCH); + if (ret 0) + dev_dbg(led-pdev-dev, + Failed to clear torch mode (%d)\n, + ret); + goto unlock; + } + + ret = max77693_set_torch_current(led, led-torch_brightness * + MAX77693_TORCH_IOUT_STEP); + if (ret 0) { + dev_dbg(led-pdev-dev, Failed to set torch current (%d)\n, + ret); + goto unlock; + } + + ret = max77693_add_mode(led, MAX77693_MODE_TORCH); + if (ret 0) + dev_dbg(led-pdev-dev, Failed to set torch mode (%d)\n, + ret); +unlock: + mutex_unlock(led-lock); +} + +static void max77693_led_brightness_set(struct led_classdev *led_cdev, + enum led_brightness value) +{ + struct max77693_led *led = ldev_to_led(led_cdev); + + led-torch_brightness = value; + schedule_work(led-work_brightness_set); Is there a reason not to do this right now (but in a work queue instead)? Almost all the drivers in the LED subsystem do it that way. I think that it is caused by the fact that setting led brightness should be as fast as possible and non-blocking. The led may be used e.g. for HD LED (see ledtrig-ide) and activated many times per second, and thus it could have impact on the system performance if it wasn't run in a work queue. Fair enough. But the expectation is that the V4L2 control's value has taken effect when the set control handler returns. That is also what virtually all existing implementations do. Could this be handled in the LED framework instead so that the V4L2 controls would function synchronously? There could be added an op to the led_flash_ops structure, for setting led brightness with guaranteed immediate effect, intended for use only by V4L2 flash sub-devs. The Flash LED driver would have to implement two ops for setting torch brightness - one for use by led class API, using work queue, and the other for use by V4L2 flash sub-dev, without work queue. I think the work queue should be moved to the LED framework. There could be another op for the driver to implement but the driver should need to implement only a single one. Alternatively, the work queue implementation should be moved out of the drivers. Or, if the op is really simple such as a single register access on a fast bus, the work queue would likely not be needed at all. I'm ok for postponing this as long as we agree on how it'd be fixed. Perhaps someone from the LED framework side to comment. +} + +static int max77693_led_flash_strobe_get(struct led_classdev *led_cdev) +{ + struct max77693_led *led = ldev_to_led(led_cdev); + int ret; + + mutex_lock(led-lock); + ret = max77693_strobe_status_get(led); + mutex_unlock(led-lock); + + return ret; +} + +static int max77693_led_flash_fault_get(struct led_classdev *led_cdev, + u32 *fault) +{ + struct max77693_led *led = ldev_to_led(led_cdev); + u8 v; + int ret; + + mutex_lock(led-lock); + + ret = max77693_int_flag_get(led, v); + if (ret 0) + goto unlock; + + *fault = 0; + + if (v MAX77693_LED_FLASH_INT_FLED2_OPEN || + v MAX77693_LED_FLASH_INT_FLED1_OPEN) + *fault |= LED_FAULT_OVER_VOLTAGE; + if (v MAX77693_LED_FLASH_INT_FLED2_SHORT || + v MAX77693_LED_FLASH_INT_FLED1_SHORT) + *fault |= LED_FAULT_SHORT_CIRCUIT; + if (v MAX77693_LED_FLASH_INT_OVER_CURRENT) + *fault |= LED_FAULT_OVER_CURRENT; +unlock: + mutex_unlock(led-lock); + return ret; +} + +static int max77693_led_flash_strobe_set(struct led_classdev *led_cdev, + bool state) +{ + struct max77693_led *led = ldev_to_led(led_cdev); + struct led_flash *flash = led_cdev-flash; + int ret; + + mutex_lock(led-lock); + + if (flash-external_strobe) { + ret = -EBUSY; + goto unlock; + } + + if (!state) { + ret = max77693_clear_mode(led, MAX77693_MODE_FLASH); + goto unlock; + } + + ret = max77693_add_mode(led, MAX77693_MODE_FLASH); + if (ret 0) + goto unlock; +unlock: + mutex_unlock(led-lock); + return ret; +} + +static int max77693_led_external_strobe_set(struct led_classdev *led_cdev, + bool enable) +{ + struct max77693_led *led = ldev_to_led(led_cdev); + int ret; + + mutex_lock(led-lock); + + if (enable) + ret = max77693_add_mode(led, MAX77693_MODE_FLASH_EXTERNAL); + else + ret = max77693_clear_mode(led,
Re: [PATCH/RFC v3 5/5] media: Add registration helpers for V4L2 flash sub-devices
Hi Jacek, On Tue, May 06, 2014 at 08:44:41AM +0200, Jacek Anaszewski wrote: Hi Sakari, On 05/02/2014 01:06 PM, Sakari Ailus wrote: [...] +static inline enum led_brightness v4l2_flash_intensity_to_led_brightness( + struct led_ctrl *config, + u32 intensity) Fits on a single line. +{ + return intensity / config-step; Shouldn't you first decrement the minimum before the division? Brightness level 0 means that led is off. Let's consider following case: intensity - 15625 config-step - 15625 intensity / config-step = 1 (the lowest possible current level) In V4L2 controls the minimum is not off, and zero might not be a possible value since minimum isn't divisible by step. I wonder how to best take that into account. I've assumed that in MODE_TORCH a led is always on. Switching the mode to MODE_FLASH or MODE_OFF turns the led off. This way we avoid the problem with converting 0 uA value to led_brightness, as available torch brightness levels start from the minimum current level value and turning the led off is accomplished on transition to MODE_OFF or MODE_FLASH, by calling brightness_set op with led_brightness = 0. I'm not sure if we understood the issue the same way. My concern was that if the intensity isn't a multiple of step (but intensity - min is), the above formula won't return a valid result (unless I miss something). Please note that v4l2_flash_intensity_to_led_brightness is called only from s_ctrl callback, and thus it expects to get the intensity aligned to the step value, so it will always be a multiple of step. Is it possible that s_ctrl callback would be passed a non-aligned control value? In a nutshell: value - min is aligned but value is not. Please see validate_new() in drivers/media/v4l2-core/v4l2-ctrls.c . -- Regards, Sakari Ailus e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk -- 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
Media build failure, missing module
Hi there, On a newly install OpenMandriva distribution I tried to build the last version of the media tree. However the build failed with the following sequence of messages. ... patch -s -f -N -p1 -i ../backports/pr_fmt.patch The text leading up to this was: -- |diff --git a/drivers/media/usb/gspca/dtcs033.c b/drivers/media/usb/gspca/dtcs033.c |index 5e42c71..ba01a3e 100644 |--- a/drivers/media/usb/gspca/dtcs033.c |+++ b/drivers/media/usb/gspca/dtcs033.c -- No file to patch. Skipping patch. 1 out of 1 hunk ignored make[2]: *** [apply_patches] Error 1 make[2]: Leaving directory `/home/software/media_build/linux' make[1]: *** [allyesconfig] Error 2 make[1]: Leaving directory `/home/software/media_build/v4l' make: *** [allyesconfig] Error 2 can't select all drivers at ./build line 490. The reason being the dtcs033 source code is absent in the tree: # ls linux/drivers/media/usb/gspca autogain_functions.c finepix.c jl2005bcd.c m5602/ ov519.c pac7311.c sn9c2028.h spca500.c spca561.c stk1135.c t613.c w996Xcf.c benq.cgl860/ jpeg.h Makefileov534_9.c pac_common.h sn9c20x.c spca501.c sq905.cstk1135.h topro.c xirlink_cit.c conex.c gspca.cKconfig mars.c ov534.cse401.c sonixb.cspca505.c sq905c.c stv0680.c tv8532.c zc3xx.c cpia1.c gspca.hkinect.c mr97310a.c pac207.c se401.h sonixj.cspca506.c sq930x.c stv06xx/ vc032x.c zc3xx-reg.h etoms.c jeilinj.c konica.c nw80x.c pac7302.c sn9c2028.cspca1528.c spca508.c stk014.c sunplus.c vicam.c Removing dtcs033 patch from backports/pr_fmt.patch (last one) allows to build media sucessfully. René -- 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: Media build failure, missing module
On 05/06/2014 08:10 PM, René wrote: Hi there, On a newly install OpenMandriva distribution I tried to build the last version of the media tree. However the build failed with the following sequence of messages. The archive ./build downloads is missing a file for some reason. It may be a few days before it is fixed since the maintainer of that file is attending a conference. I've told Mauro that it is broken. In the meantime this should work, although it will be slow since it has to clone the git tree: ./build --main-git Regards, Hans ... patch -s -f -N -p1 -i ../backports/pr_fmt.patch The text leading up to this was: -- |diff --git a/drivers/media/usb/gspca/dtcs033.c b/drivers/media/usb/gspca/dtcs033.c |index 5e42c71..ba01a3e 100644 |--- a/drivers/media/usb/gspca/dtcs033.c |+++ b/drivers/media/usb/gspca/dtcs033.c -- No file to patch. Skipping patch. 1 out of 1 hunk ignored make[2]: *** [apply_patches] Error 1 make[2]: Leaving directory `/home/software/media_build/linux' make[1]: *** [allyesconfig] Error 2 make[1]: Leaving directory `/home/software/media_build/v4l' make: *** [allyesconfig] Error 2 can't select all drivers at ./build line 490. The reason being the dtcs033 source code is absent in the tree: # ls linux/drivers/media/usb/gspca autogain_functions.c finepix.c jl2005bcd.c m5602/ ov519.c pac7311.c sn9c2028.h spca500.c spca561.c stk1135.c t613.c w996Xcf.c benq.cgl860/ jpeg.h Makefileov534_9.c pac_common.h sn9c20x.c spca501.c sq905.cstk1135.h topro.c xirlink_cit.c conex.c gspca.cKconfig mars.c ov534.cse401.c sonixb.cspca505.c sq905c.c stv0680.c tv8532.c zc3xx.c cpia1.c gspca.hkinect.c mr97310a.c pac207.c se401.h sonixj.cspca506.c sq930x.c stv06xx/ vc032x.c zc3xx-reg.h etoms.c jeilinj.c konica.c nw80x.c pac7302.c sn9c2028.cspca1528.c spca508.c stk014.c sunplus.c vicam.c Removing dtcs033 patch from backports/pr_fmt.patch (last one) allows to build media sucessfully. René -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
cron job: media_tree daily build: OK
This message is generated daily by a cron job that builds media_tree for the kernels and architectures in the list below. Results of the daily build of media_tree: date: Wed May 7 04:00:20 CEST 2014 git branch: test git hash: 393cbd8dc532c1ebed60719da8d379f50d445f28 gcc version:i686-linux-gcc (GCC) 4.8.2 sparse version: v0.5.0-11-g38d1124 host hardware: x86_64 host os:3.14-1.slh.1-amd64 linux-git-arm-at91: OK linux-git-arm-davinci: OK linux-git-arm-exynos: OK linux-git-arm-mx: OK linux-git-arm-omap: OK linux-git-arm-omap1: OK linux-git-arm-pxa: OK linux-git-blackfin: OK linux-git-i686: OK linux-git-m32r: OK linux-git-mips: OK linux-git-powerpc64: OK linux-git-sh: OK linux-git-x86_64: OK linux-2.6.31.14-i686: OK linux-2.6.32.27-i686: OK linux-2.6.33.7-i686: OK linux-2.6.34.7-i686: OK linux-2.6.35.9-i686: OK linux-2.6.36.4-i686: OK linux-2.6.37.6-i686: OK linux-2.6.38.8-i686: OK linux-2.6.39.4-i686: OK linux-3.0.60-i686: OK linux-3.1.10-i686: OK linux-3.2.37-i686: OK linux-3.3.8-i686: OK linux-3.4.27-i686: OK linux-3.5.7-i686: OK linux-3.6.11-i686: OK linux-3.7.4-i686: OK linux-3.8-i686: OK linux-3.9.2-i686: OK linux-3.10.1-i686: OK linux-3.11.1-i686: OK linux-3.12-i686: OK linux-3.13-i686: OK linux-3.14-i686: OK linux-3.15-rc1-i686: OK linux-2.6.31.14-x86_64: OK linux-2.6.32.27-x86_64: OK linux-2.6.33.7-x86_64: OK linux-2.6.34.7-x86_64: OK linux-2.6.35.9-x86_64: OK linux-2.6.36.4-x86_64: OK linux-2.6.37.6-x86_64: OK linux-2.6.38.8-x86_64: OK linux-2.6.39.4-x86_64: OK linux-3.0.60-x86_64: OK linux-3.1.10-x86_64: OK linux-3.2.37-x86_64: OK linux-3.3.8-x86_64: OK linux-3.4.27-x86_64: OK linux-3.5.7-x86_64: OK linux-3.6.11-x86_64: OK linux-3.7.4-x86_64: OK linux-3.8-x86_64: OK linux-3.9.2-x86_64: OK linux-3.10.1-x86_64: OK linux-3.11.1-x86_64: OK linux-3.12-x86_64: OK linux-3.13-x86_64: OK linux-3.14-x86_64: OK linux-3.15-rc1-x86_64: OK apps: OK spec-git: OK sparse version: v0.5.0-11-g38d1124 sparse: ERRORS Detailed results are available here: http://www.xs4all.nl/~hverkuil/logs/Wednesday.log Full logs are available here: http://www.xs4all.nl/~hverkuil/logs/Wednesday.tar.bz2 The Media Infrastructure API from this daily build is here: http://www.xs4all.nl/~hverkuil/spec/media.html -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] au0828: fix logic of tuner disconnection
From: Changbing Xiong cb.xi...@samsung.com The driver crashed when the tuner was disconnected while restart stream operations are still being performed. Fixed by adding a flag in struct au0828_dvb to indicate whether restart stream operations can be performed. If the stream gets misaligned, the work of restart stream operations are usually scheduled for many times in a row. If tuner is disconnected at this time and some of restart stream operations are still not flushed, then the driver crashed due to accessing the resource which used in restart stream operations has been released. Signed-off-by: Changbing Xiong cb.xi...@samsung.com --- drivers/media/usb/au0828/au0828-dvb.c | 13 + drivers/media/usb/au0828/au0828.h |1 + 2 files changed, 14 insertions(+) mode change 100644 = 100755 drivers/media/usb/au0828/au0828-dvb.c mode change 100644 = 100755 drivers/media/usb/au0828/au0828.h diff --git a/drivers/media/usb/au0828/au0828-dvb.c b/drivers/media/usb/au0828/au0828-dvb.c old mode 100644 new mode 100755 index 9a6f156..90cdde5 --- a/drivers/media/usb/au0828/au0828-dvb.c +++ b/drivers/media/usb/au0828/au0828-dvb.c @@ -280,6 +280,11 @@ static void au0828_restart_dvb_streaming(struct work_struct *work) mutex_lock(dvb-lock); + if (dvb-flag == true) { + mutex_unlock(dvb-lock); + return; + } + /* Stop transport */ stop_urb_transfer(dev); au0828_write(dev, 0x608, 0x00); @@ -304,6 +309,8 @@ static int dvb_register(struct au0828_dev *dev) dprintk(1, %s()\n, __func__); + dvb-flag = false; + INIT_WORK(dev-restart_streaming, au0828_restart_dvb_streaming); /* register adapter */ @@ -403,6 +410,10 @@ void au0828_dvb_unregister(struct au0828_dev *dev) if (dvb-frontend == NULL) return; + mutex_lock(dvb-lock); + dvb-flag = true; + mutex_unlock(dvb-lock); + dvb_net_release(dvb-net); dvb-demux.dmx.remove_frontend(dvb-demux.dmx, dvb-fe_mem); dvb-demux.dmx.remove_frontend(dvb-demux.dmx, dvb-fe_hw); @@ -411,6 +422,8 @@ void au0828_dvb_unregister(struct au0828_dev *dev) dvb_unregister_frontend(dvb-frontend); dvb_frontend_detach(dvb-frontend); dvb_unregister_adapter(dvb-adapter); + + cancel_work_sync(dev-restart_streaming); } /* All the DVB attach calls go here, this function get's modified diff --git a/drivers/media/usb/au0828/au0828.h b/drivers/media/usb/au0828/au0828.h old mode 100644 new mode 100755 index ef1f57f..00255d5 --- a/drivers/media/usb/au0828/au0828.h +++ b/drivers/media/usb/au0828/au0828.h @@ -102,6 +102,7 @@ struct au0828_dvb { struct dmx_frontend fe_mem; struct dvb_net net; int feeding; + bool flag; }; enum au0828_stream_state { -- 1.7.9.5 -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] [media] s2255drv: fix memory leak s2255_probe()
Hello, 2014-05-05 17:38 GMT+09:00 Sakari Ailus sakari.ai...@iki.fi: On Tue, Apr 15, 2014 at 07:54:43PM +0900, DaeSeok Youn wrote: Hi, Sakari 2014-04-15 18:33 GMT+09:00 Sakari Ailus sakari.ai...@iki.fi: Hi Daeseok, On Tue, Apr 15, 2014 at 01:49:34PM +0900, Daeseok Youn wrote: smatch says: drivers/media/usb/s2255/s2255drv.c:2246 s2255_probe() warn: possible memory leak of 'dev' Signed-off-by: Daeseok Youn daeseok.y...@gmail.com --- drivers/media/usb/s2255/s2255drv.c |1 + 1 files changed, 1 insertions(+), 0 deletions(-) diff --git a/drivers/media/usb/s2255/s2255drv.c b/drivers/media/usb/s2255/s2255drv.c index 1d4ba2b..8aca3ef 100644 --- a/drivers/media/usb/s2255/s2255drv.c +++ b/drivers/media/usb/s2255/s2255drv.c @@ -2243,6 +2243,7 @@ static int s2255_probe(struct usb_interface *interface, dev-cmdbuf = kzalloc(S2255_CMDBUF_SIZE, GFP_KERNEL); if (dev-cmdbuf == NULL) { s2255_dev_err(interface-dev, out of memory\n); + kfree(dev); return -ENOMEM; } The rest of the function already uses goto and labels for error handling. I think it'd take adding one more. dev is correctly released in other error cases. I am not sure that adding a new label for error handling when allocation for dev-cmdbuf is failed. I think it is ok to me. :-) Because I think it is not good adding a new label and use goto statement for this. I can ack this if you use the same pattern for error handling that's already there. But it has a redundant kfree() call for dev-cmdbuf when it failed to allocate, right? If it is ok, I send this again after fixing as your comment. Thanks. Daeseok Youn. -- Sakari Ailus e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk -- 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] timblogiw: Introduce the use of the managed version of kzalloc
This patch moves data allocated using kzalloc to managed data allocated using devm_kzalloc and cleans now unnecessary kfrees in probe and remove functions.The label err_register is removed as it is no longer required. The following Coccinelle semantic patch was used for making the change: @platform@ identifier p, probefn, removefn; @@ struct platform_driver p = { .probe = probefn, .remove = removefn, }; @prb@ identifier platform.probefn, pdev; expression e, e1, e2; @@ probefn(struct platform_device *pdev, ...) { +... - e = kzalloc(e1, e2) + e = devm_kzalloc(pdev-dev, e1, e2) ... ?-kfree(e); ...+ } @rem depends on prb@ identifier platform.removefn; expression e; @@ removefn(...) { ... - kfree(e); ... } Signed-off-by: Himangi Saraogi himangi...@gmail.com --- drivers/media/platform/timblogiw.c | 8 ++-- 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/drivers/media/platform/timblogiw.c b/drivers/media/platform/timblogiw.c index ccdadd6..fbfdada 100644 --- a/drivers/media/platform/timblogiw.c +++ b/drivers/media/platform/timblogiw.c @@ -800,7 +800,7 @@ static int timblogiw_probe(struct platform_device *pdev) if (!pdata-encoder.module_name) dev_info(pdev-dev, Running without decoder\n); - lw = kzalloc(sizeof(*lw), GFP_KERNEL); + lw = devm_kzalloc(pdev-dev, sizeof(*lw), GFP_KERNEL); if (!lw) { err = -ENOMEM; goto err; @@ -820,7 +820,7 @@ static int timblogiw_probe(struct platform_device *pdev) strlcpy(lw-v4l2_dev.name, DRIVER_NAME, sizeof(lw-v4l2_dev.name)); err = v4l2_device_register(NULL, lw-v4l2_dev); if (err) - goto err_register; + goto err; lw-video_dev.v4l2_dev = lw-v4l2_dev; @@ -837,8 +837,6 @@ static int timblogiw_probe(struct platform_device *pdev) err_request: v4l2_device_unregister(lw-v4l2_dev); -err_register: - kfree(lw); err: dev_err(pdev-dev, Failed to register: %d\n, err); @@ -853,8 +851,6 @@ static int timblogiw_remove(struct platform_device *pdev) v4l2_device_unregister(lw-v4l2_dev); - kfree(lw); - return 0; } -- 1.9.1 -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html