Guennadi Liakhovetski <g.liakhovetski@xxxxxx> writes:
> This is more or less the final version of the first step of the
> v4l2-subdev conversion, hence, all affected driver authors / platform
> maintainers are encouraged to review and test. I have eliminated
OK, here goes a preliminary review for the bits I maintain. I'll test fully this
weekend.
As a side note, I tried to apply your patch on top of linux-next-20090406. I was
a bit tedious. Would you tell me which tree you're based against, or even better
some git url ?
<snip>
> diff --git a/arch/arm/mach-pxa/mioa701.c b/arch/arm/mach-pxa/mioa701.c
> index 97c93a7..5c8aabf 100644
> --- a/arch/arm/mach-pxa/mioa701.c
> +++ b/arch/arm/mach-pxa/mioa701.c
> @@ -724,19 +724,19 @@ struct pxacamera_platform_data mioa701_pxacamera_platform_data = {
> .mclk_10khz = 5000,
> };
>
> -static struct soc_camera_link iclink = {
> - .bus_id = 0, /* Must match id in pxa27x_device_camera in device.c */
> -};
> -
> /* Board I2C devices. */
I would rather have :
/*
* Board I2C devices
*/
The remaining /* blurpblurg */ forms are a leftover in device comments.
<snip>
> @@ -754,20 +754,21 @@ static struct platform_device var = { \
> .platform_data = pdata, \
> .parent = tparent, \
> }, \
> -};
> +}
No cookie for you for removing that semi-colon.
> #define MIO_SIMPLE_DEV(var, strname, pdata) \
> MIO_PARENT_DEV(var, strname, NULL, pdata)
>
> -MIO_SIMPLE_DEV(mioa701_gpio_keys, "gpio-keys", &mioa701_gpio_keys_data)
> +MIO_SIMPLE_DEV(mioa701_gpio_keys, "gpio-keys", &mioa701_gpio_keys_data);
> MIO_PARENT_DEV(mioa701_backlight, "pwm-backlight", &pxa27x_device_pwm0.dev,
> &mioa701_backlight_data);
> -MIO_SIMPLE_DEV(mioa701_led, "leds-gpio", &gpio_led_info)
> -MIO_SIMPLE_DEV(pxa2xx_pcm, "pxa2xx-pcm", NULL)
> -MIO_SIMPLE_DEV(pxa2xx_ac97, "pxa2xx-ac97", NULL)
> -MIO_PARENT_DEV(mio_wm9713_codec, "wm9713-codec", &pxa2xx_ac97.dev, NULL)
> -MIO_SIMPLE_DEV(mioa701_sound, "mioa701-wm9713", NULL)
> -MIO_SIMPLE_DEV(mioa701_board, "mioa701-board", NULL)
> +MIO_SIMPLE_DEV(mioa701_led, "leds-gpio", &gpio_led_info);
> +MIO_SIMPLE_DEV(pxa2xx_pcm, "pxa2xx-pcm", NULL);
> +MIO_SIMPLE_DEV(pxa2xx_ac97, "pxa2xx-ac97", NULL);
> +MIO_PARENT_DEV(mio_wm9713_codec, "wm9713-codec", &pxa2xx_ac97.dev, NULL);
> +MIO_SIMPLE_DEV(mioa701_sound, "mioa701-wm9713", NULL);
> +MIO_SIMPLE_DEV(mioa701_board, "mioa701-board", NULL);
> MIO_SIMPLE_DEV(gpio_vbus, "gpio-vbus", &gpio_vbus_data);
Please, don't change the indentation. You will face :
(a) conficts with patches in this merge window
(b) that's not the object of your patch anyway
(c) I like that indentation in that very specific case
> +MIO_SIMPLE_DEV(mioa701_camera, "soc-camera-pdrv",&&iclink[0]);
^
isn't &iclink enough ?
>
> static struct platform_device *devices[] __initdata = {
> &mioa701_gpio_keys,
> @@ -781,6 +782,7 @@ static struct platform_device *devices[] __initdata = {
> &strataflash,
> &gpio_vbus,
> &mioa701_board,
> + &mioa701_camera,
Please invert mioa701_board and mioa701_camera. The board should always be last
for suspend/resume purpose (yes, that would have deserved a comment, I hear you
:))
> };
>
> static void mioa701_machine_exit(void);
> @@ -825,7 +827,6 @@ static void __init mioa701_machine_init(void)
>
> pxa_set_i2c_info(&i2c_pdata);
> pxa_set_camera_info(&mioa701_pxacamera_platform_data);
> - i2c_register_board_info(0, ARRAY_AND_SIZE(mioa701_i2c_devices));
I'm wondering which version of mioa701.c had that line ... strange ...
> diff --git a/drivers/media/video/mt9m111.c b/drivers/media/video/mt9m111.c
> index cdd1ddb..425aec2 100644
> --- a/drivers/media/video/mt9m111.c
> +++ b/drivers/media/video/mt9m111.c
<snip>
> @@ -938,40 +955,42 @@ static int mt9m111_video_probe(struct soc_camera_device *icd)
>
> dev_info(&icd->dev, "Detected a MT9M11x chip ID %x\n", data);
>
> - ret = soc_camera_video_start(icd);
> - if (ret)
> - goto eisis;
> -
> mt9m111->autoexposure = 1;
> mt9m111->autowhitebalance = 1;
>
> mt9m111->swap_rgb_even_odd = 1;
> mt9m111->swap_rgb_red_blue = 1;
>
> - return 0;
> -eisis:
> ei2c:
> + soc_camera_video_stop(icd);
> +
> return ret;
> }
>
> static void mt9m111_video_remove(struct soc_camera_device *icd)
> {
> - struct mt9m111 *mt9m111 = container_of(icd, struct mt9m111, icd);
> + struct i2c_client *client = to_i2c_client(icd->control);
>
> - dev_dbg(&icd->dev, "Video %x removed: %p, %p\n", mt9m111->client->addr,
> - mt9m111->icd.dev.parent, mt9m111->icd.vdev);
> - soc_camera_video_stop(&mt9m111->icd);
> + dev_dbg(&icd->dev, "Video %x removed: %p, %p\n", client->addr,
> + icd->dev.parent, icd->vdev);
> + icd->ops = NULL;
I don't understand the icd->ops = NULL here. It's not symmetrical with
mt9m111_video_probe. Shouldn't that be in mt9m111_remove ?
More generally, I saw all the heavy work on mt9m111 driver conversion. I
wondered if there shouldn't be a wrapper function to convert an icd structure
into an mt9m111 structre. If I had done that straight away, you wouldn't have
had that much work ...
Cheers.
--
Robert
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
