| Message ID | 20090312141819.GN425@pengutronix.de (mailing list archive) |
|---|---|
| State | RFC |
| Headers | show |
Hi Sascha, something, that skipped both of us: On Thu, 12 Mar 2009, Sascha Hauer wrote: > Some Phytec cameras have a I2C GPIO expander which allows it to > switch between different sensor bus widths. This was previously > handled in the camera driver. Since handling of this switch > varies on several boards the cameras are used on, the board > support seems a better place to handle the switch > > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de> > --- > arch/arm/mach-pxa/pcm990-baseboard.c | 53 ++++++++++++++++++++++++++++------ > 1 files changed, 44 insertions(+), 9 deletions(-) > > diff --git a/arch/arm/mach-pxa/pcm990-baseboard.c b/arch/arm/mach-pxa/pcm990-baseboard.c > index 34841c7..fd8f786 100644 > --- a/arch/arm/mach-pxa/pcm990-baseboard.c > +++ b/arch/arm/mach-pxa/pcm990-baseboard.c > @@ -381,14 +381,49 @@ static struct pca953x_platform_data pca9536_data = { > .gpio_base = NR_BUILTIN_GPIO + 1, > }; > > -static struct soc_camera_link iclink[] = { > - { > - .bus_id = 0, /* Must match with the camera ID above */ > - .gpio = NR_BUILTIN_GPIO + 1, > - }, { > - .bus_id = 0, /* Must match with the camera ID above */ > - .gpio = -ENXIO, > +static int gpio_bus_switch; > + > +static int pcm990_camera_set_bus_param(struct soc_camera_link *link, > + unsigned long flags) > +{ > + if (gpio_bus_switch <= 0) { > + if (flags == SOCAM_DATAWIDTH_10) > + return 0; > + else > + return -EINVAL; > + } > + > + if (flags & SOCAM_DATAWIDTH_8) > + gpio_set_value(gpio_bus_switch, 1); > + else > + gpio_set_value(gpio_bus_switch, 0); > + > + return 0; > +} > + > +static unsigned long pcm990_camera_query_bus_param(struct soc_camera_link *link) > +{ > + int ret; > + > + if (!gpio_bus_switch) { > + ret = gpio_request(NR_BUILTIN_GPIO + 1, "camera"); There's no gpio_free() now... So, for example, you cannot unload the extender driver any more, unloading i2c adapter driver (i2c-pxa) produces ugly stuff like pca953x 0-0041: gpiochip_remove() failed, -16 So, we either have to request and free the GPIO in each query / set, or we need an explicit .free_bus() call in soc_camera_link. None of the two really pleases me, but maybe the latter is slightly less ugly, what do you think? Thanks Guennadi > + if (!ret) { > + gpio_bus_switch = NR_BUILTIN_GPIO + 1; > + gpio_direction_output(gpio_bus_switch, 0); > + } else > + gpio_bus_switch = -EINVAL; > } > + > + if (gpio_bus_switch > 0) > + return SOCAM_DATAWIDTH_8 | SOCAM_DATAWIDTH_10; > + else > + return SOCAM_DATAWIDTH_10; > +} > + > +static struct soc_camera_link iclink = { > + .bus_id = 0, /* Must match with the camera ID above */ > + .query_bus_param = pcm990_camera_query_bus_param, > + .set_bus_param = pcm990_camera_set_bus_param, > }; > > /* Board I2C devices. */ > @@ -399,10 +434,10 @@ static struct i2c_board_info __initdata pcm990_i2c_devices[] = { > .platform_data = &pca9536_data, > }, { > I2C_BOARD_INFO("mt9v022", 0x48), > - .platform_data = &iclink[0], /* With extender */ > + .platform_data = &iclink, /* With extender */ > }, { > I2C_BOARD_INFO("mt9m001", 0x5d), > - .platform_data = &iclink[0], /* With extender */ > + .platform_data = &iclink, /* With extender */ > }, > }; > #endif /* CONFIG_VIDEO_PXA27x ||CONFIG_VIDEO_PXA27x_MODULE */ > -- > 1.5.6.5 > > -- > Pengutronix e.K. | | > Industrial Linux Solutions | http://www.pengutronix.de/ | > Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | > Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | > --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 9 Apr 2009, Guennadi Liakhovetski wrote: > Hi Sascha, > > something, that skipped both of us: ...and one more: > On Thu, 12 Mar 2009, Sascha Hauer wrote: > > > Some Phytec cameras have a I2C GPIO expander which allows it to > > switch between different sensor bus widths. This was previously > > handled in the camera driver. Since handling of this switch > > varies on several boards the cameras are used on, the board > > support seems a better place to handle the switch > > > > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de> > > --- > > arch/arm/mach-pxa/pcm990-baseboard.c | 53 ++++++++++++++++++++++++++++------ > > 1 files changed, 44 insertions(+), 9 deletions(-) > > > > diff --git a/arch/arm/mach-pxa/pcm990-baseboard.c b/arch/arm/mach-pxa/pcm990-baseboard.c > > index 34841c7..fd8f786 100644 > > --- a/arch/arm/mach-pxa/pcm990-baseboard.c > > +++ b/arch/arm/mach-pxa/pcm990-baseboard.c > > @@ -381,14 +381,49 @@ static struct pca953x_platform_data pca9536_data = { > > .gpio_base = NR_BUILTIN_GPIO + 1, > > }; > > > > -static struct soc_camera_link iclink[] = { > > - { > > - .bus_id = 0, /* Must match with the camera ID above */ > > - .gpio = NR_BUILTIN_GPIO + 1, > > - }, { > > - .bus_id = 0, /* Must match with the camera ID above */ > > - .gpio = -ENXIO, > > +static int gpio_bus_switch; > > + > > +static int pcm990_camera_set_bus_param(struct soc_camera_link *link, > > + unsigned long flags) > > +{ > > + if (gpio_bus_switch <= 0) { > > + if (flags == SOCAM_DATAWIDTH_10) > > + return 0; > > + else > > + return -EINVAL; > > + } > > + > > + if (flags & SOCAM_DATAWIDTH_8) > > + gpio_set_value(gpio_bus_switch, 1); > > + else > > + gpio_set_value(gpio_bus_switch, 0); > > + > > + return 0; > > +} > > + > > +static unsigned long pcm990_camera_query_bus_param(struct soc_camera_link *link) > > +{ > > + int ret; > > + > > + if (!gpio_bus_switch) { > > + ret = gpio_request(NR_BUILTIN_GPIO + 1, "camera"); > > There's no gpio_free() now... So, for example, you cannot unload the > extender driver any more, unloading i2c adapter driver (i2c-pxa) produces > ugly stuff like > > pca953x 0-0041: gpiochip_remove() failed, -16 > > So, we either have to request and free the GPIO in each query / set, or we > need an explicit .free_bus() call in soc_camera_link. None of the two > really pleases me, but maybe the latter is slightly less ugly, what do you > think? > > Thanks > Guennadi > > > + if (!ret) { > > + gpio_bus_switch = NR_BUILTIN_GPIO + 1; > > + gpio_direction_output(gpio_bus_switch, 0); > > + } else > > + gpio_bus_switch = -EINVAL; > > } If you first do not load pca953x and try to start capture, thus calling pcm990_camera_query_bus_param(), gpio_request() will fail and you get gpio_bus_switch < 0. Then if you load pca953x it doesn't help any more - by testing for "if (!gpio_bus_switch)" you do not retry after the first error. Thanks Guennadi > > + > > + if (gpio_bus_switch > 0) > > + return SOCAM_DATAWIDTH_8 | SOCAM_DATAWIDTH_10; > > + else > > + return SOCAM_DATAWIDTH_10; > > +} > > + > > +static struct soc_camera_link iclink = { > > + .bus_id = 0, /* Must match with the camera ID above */ > > + .query_bus_param = pcm990_camera_query_bus_param, > > + .set_bus_param = pcm990_camera_set_bus_param, > > }; > > > > /* Board I2C devices. */ > > @@ -399,10 +434,10 @@ static struct i2c_board_info __initdata pcm990_i2c_devices[] = { > > .platform_data = &pca9536_data, > > }, { > > I2C_BOARD_INFO("mt9v022", 0x48), > > - .platform_data = &iclink[0], /* With extender */ > > + .platform_data = &iclink, /* With extender */ > > }, { > > I2C_BOARD_INFO("mt9m001", 0x5d), > > - .platform_data = &iclink[0], /* With extender */ > > + .platform_data = &iclink, /* With extender */ > > }, > > }; > > #endif /* CONFIG_VIDEO_PXA27x ||CONFIG_VIDEO_PXA27x_MODULE */ > > -- > > 1.5.6.5 > > > > -- > > Pengutronix e.K. | | > > Industrial Linux Solutions | http://www.pengutronix.de/ | > > Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | > > Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | > > > > --- > Guennadi Liakhovetski, Ph.D. > Freelance Open-Source Software Developer > --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Apr 14, 2009 at 10:57:32AM +0200, Guennadi Liakhovetski wrote: > On Thu, 9 Apr 2009, Guennadi Liakhovetski wrote: > > > Hi Sascha, > > > > something, that skipped both of us: > > ...and one more: > > > On Thu, 12 Mar 2009, Sascha Hauer wrote: > > > > > Some Phytec cameras have a I2C GPIO expander which allows it to > > > switch between different sensor bus widths. This was previously > > > handled in the camera driver. Since handling of this switch > > > varies on several boards the cameras are used on, the board > > > support seems a better place to handle the switch > > > > > > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de> > > > --- > > > arch/arm/mach-pxa/pcm990-baseboard.c | 53 ++++++++++++++++++++++++++++------ > > > 1 files changed, 44 insertions(+), 9 deletions(-) > > > > > > diff --git a/arch/arm/mach-pxa/pcm990-baseboard.c b/arch/arm/mach-pxa/pcm990-baseboard.c > > > index 34841c7..fd8f786 100644 > > > --- a/arch/arm/mach-pxa/pcm990-baseboard.c > > > +++ b/arch/arm/mach-pxa/pcm990-baseboard.c > > > @@ -381,14 +381,49 @@ static struct pca953x_platform_data pca9536_data = { > > > .gpio_base = NR_BUILTIN_GPIO + 1, > > > }; > > > > > > -static struct soc_camera_link iclink[] = { > > > - { > > > - .bus_id = 0, /* Must match with the camera ID above */ > > > - .gpio = NR_BUILTIN_GPIO + 1, > > > - }, { > > > - .bus_id = 0, /* Must match with the camera ID above */ > > > - .gpio = -ENXIO, > > > +static int gpio_bus_switch; > > > + > > > +static int pcm990_camera_set_bus_param(struct soc_camera_link *link, > > > + unsigned long flags) > > > +{ > > > + if (gpio_bus_switch <= 0) { > > > + if (flags == SOCAM_DATAWIDTH_10) > > > + return 0; > > > + else > > > + return -EINVAL; > > > + } > > > + > > > + if (flags & SOCAM_DATAWIDTH_8) > > > + gpio_set_value(gpio_bus_switch, 1); > > > + else > > > + gpio_set_value(gpio_bus_switch, 0); > > > + > > > + return 0; > > > +} > > > + > > > +static unsigned long pcm990_camera_query_bus_param(struct soc_camera_link *link) > > > +{ > > > + int ret; > > > + > > > + if (!gpio_bus_switch) { > > > + ret = gpio_request(NR_BUILTIN_GPIO + 1, "camera"); > > > > There's no gpio_free() now... So, for example, you cannot unload the > > extender driver any more, unloading i2c adapter driver (i2c-pxa) produces > > ugly stuff like > > > > pca953x 0-0041: gpiochip_remove() failed, -16 > > > > So, we either have to request and free the GPIO in each query / set, or we > > need an explicit .free_bus() call in soc_camera_link. None of the two > > really pleases me, but maybe the latter is slightly less ugly, what do you > > think? > > > > Thanks > > Guennadi > > > > > + if (!ret) { > > > + gpio_bus_switch = NR_BUILTIN_GPIO + 1; > > > + gpio_direction_output(gpio_bus_switch, 0); > > > + } else > > > + gpio_bus_switch = -EINVAL; > > > } > > If you first do not load pca953x and try to start capture, thus calling > pcm990_camera_query_bus_param(), gpio_request() will fail and you get > gpio_bus_switch < 0. Then if you load pca953x it doesn't help any more - > by testing for "if (!gpio_bus_switch)" you do not retry after the first > error. So how do we proceed? Add init/exit functions for the bus? Sascha > > Thanks > Guennadi > > > > + > > > + if (gpio_bus_switch > 0) > > > + return SOCAM_DATAWIDTH_8 | SOCAM_DATAWIDTH_10; > > > + else > > > + return SOCAM_DATAWIDTH_10; > > > +} > > > + > > > +static struct soc_camera_link iclink = { > > > + .bus_id = 0, /* Must match with the camera ID above */ > > > + .query_bus_param = pcm990_camera_query_bus_param, > > > + .set_bus_param = pcm990_camera_set_bus_param, > > > }; > > > > > > /* Board I2C devices. */ > > > @@ -399,10 +434,10 @@ static struct i2c_board_info __initdata pcm990_i2c_devices[] = { > > > .platform_data = &pca9536_data, > > > }, { > > > I2C_BOARD_INFO("mt9v022", 0x48), > > > - .platform_data = &iclink[0], /* With extender */ > > > + .platform_data = &iclink, /* With extender */ > > > }, { > > > I2C_BOARD_INFO("mt9m001", 0x5d), > > > - .platform_data = &iclink[0], /* With extender */ > > > + .platform_data = &iclink, /* With extender */ > > > }, > > > }; > > > #endif /* CONFIG_VIDEO_PXA27x ||CONFIG_VIDEO_PXA27x_MODULE */ > > > -- > > > 1.5.6.5 > > > > > > -- > > > Pengutronix e.K. | | > > > Industrial Linux Solutions | http://www.pengutronix.de/ | > > > Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | > > > Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | > > > > > > > --- > > Guennadi Liakhovetski, Ph.D. > > Freelance Open-Source Software Developer > > > > --- > Guennadi Liakhovetski, Ph.D. > Freelance Open-Source Software Developer >
On Tue, 14 Apr 2009, Sascha Hauer wrote: > On Tue, Apr 14, 2009 at 10:57:32AM +0200, Guennadi Liakhovetski wrote: > > On Thu, 9 Apr 2009, Guennadi Liakhovetski wrote: [snip] > > > > +static unsigned long pcm990_camera_query_bus_param(struct soc_camera_link *link) > > > > +{ > > > > + int ret; > > > > + > > > > + if (!gpio_bus_switch) { > > > > + ret = gpio_request(NR_BUILTIN_GPIO + 1, "camera"); > > > > > > There's no gpio_free() now... So, for example, you cannot unload the > > > extender driver any more, unloading i2c adapter driver (i2c-pxa) produces > > > ugly stuff like > > > > > > pca953x 0-0041: gpiochip_remove() failed, -16 > > > > > > So, we either have to request and free the GPIO in each query / set, or we > > > need an explicit .free_bus() call in soc_camera_link. None of the two > > > really pleases me, but maybe the latter is slightly less ugly, what do you > > > think? > > > > > > Thanks > > > Guennadi > > > > > > > + if (!ret) { > > > > + gpio_bus_switch = NR_BUILTIN_GPIO + 1; > > > > + gpio_direction_output(gpio_bus_switch, 0); > > > > + } else > > > > + gpio_bus_switch = -EINVAL; > > > > } > > > > If you first do not load pca953x and try to start capture, thus calling > > pcm990_camera_query_bus_param(), gpio_request() will fail and you get > > gpio_bus_switch < 0. Then if you load pca953x it doesn't help any more - > > by testing for "if (!gpio_bus_switch)" you do not retry after the first > > error. > > So how do we proceed? Add init/exit functions for the bus? If you don't object, I'll just add a .free_bus method to soc_camera_link, and switch pcm990 to only use two values: a negative value for a non-allocated gpio - either before the first attempt or after an error, thus re-trying every time if gpio is negative, in case a driver has been loaded in the meantime. Thanks Guennadi --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Apr 14, 2009 at 11:20:03AM +0200, Guennadi Liakhovetski wrote: > On Tue, 14 Apr 2009, Sascha Hauer wrote: > > > On Tue, Apr 14, 2009 at 10:57:32AM +0200, Guennadi Liakhovetski wrote: > > > On Thu, 9 Apr 2009, Guennadi Liakhovetski wrote: > > [snip] > > > > > > +static unsigned long pcm990_camera_query_bus_param(struct soc_camera_link *link) > > > > > +{ > > > > > + int ret; > > > > > + > > > > > + if (!gpio_bus_switch) { > > > > > + ret = gpio_request(NR_BUILTIN_GPIO + 1, "camera"); > > > > > > > > There's no gpio_free() now... So, for example, you cannot unload the > > > > extender driver any more, unloading i2c adapter driver (i2c-pxa) produces > > > > ugly stuff like > > > > > > > > pca953x 0-0041: gpiochip_remove() failed, -16 > > > > > > > > So, we either have to request and free the GPIO in each query / set, or we > > > > need an explicit .free_bus() call in soc_camera_link. None of the two > > > > really pleases me, but maybe the latter is slightly less ugly, what do you > > > > think? > > > > > > > > Thanks > > > > Guennadi > > > > > > > > > + if (!ret) { > > > > > + gpio_bus_switch = NR_BUILTIN_GPIO + 1; > > > > > + gpio_direction_output(gpio_bus_switch, 0); > > > > > + } else > > > > > + gpio_bus_switch = -EINVAL; > > > > > } > > > > > > If you first do not load pca953x and try to start capture, thus calling > > > pcm990_camera_query_bus_param(), gpio_request() will fail and you get > > > gpio_bus_switch < 0. Then if you load pca953x it doesn't help any more - > > > by testing for "if (!gpio_bus_switch)" you do not retry after the first > > > error. > > > > So how do we proceed? Add init/exit functions for the bus? > > If you don't object, I'll just add a .free_bus method to soc_camera_link, > and switch pcm990 to only use two values: a negative value for a > non-allocated gpio - either before the first attempt or after an error, > thus re-trying every time if gpio is negative, in case a driver has been > loaded in the meantime. For the sake of symmetry, shouldn't we have an init function aswell? Sascha
On Tue, 14 Apr 2009, Sascha Hauer wrote: > On Tue, Apr 14, 2009 at 11:20:03AM +0200, Guennadi Liakhovetski wrote: > > > > If you don't object, I'll just add a .free_bus method to soc_camera_link, > > and switch pcm990 to only use two values: a negative value for a > > non-allocated gpio - either before the first attempt or after an error, > > thus re-trying every time if gpio is negative, in case a driver has been > > loaded in the meantime. > > For the sake of symmetry, shouldn't we have an init function aswell? Well, for the sake of symmetry - yes, but I'm not sure we want to bloat this simple API further just to make it symmetric... You think it's worth it? Or do you have any "practical" reasons for that? Of course, we could also add a void *bus_resource to soc_camera_link and get rid of the static gpio_bus_switch and allocate it dynamically in .bus_init and free it in .bus_release, but... Thanks Guennadi --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Apr 14, 2009 at 11:53:54AM +0200, Guennadi Liakhovetski wrote: > On Tue, 14 Apr 2009, Sascha Hauer wrote: > > > On Tue, Apr 14, 2009 at 11:20:03AM +0200, Guennadi Liakhovetski wrote: > > > > > > If you don't object, I'll just add a .free_bus method to soc_camera_link, > > > and switch pcm990 to only use two values: a negative value for a > > > non-allocated gpio - either before the first attempt or after an error, > > > thus re-trying every time if gpio is negative, in case a driver has been > > > loaded in the meantime. > > > > For the sake of symmetry, shouldn't we have an init function aswell? > > Well, for the sake of symmetry - yes, but I'm not sure we want to bloat > this simple API further just to make it symmetric... You think it's worth > it? Or do you have any "practical" reasons for that? Of course, we could > also add a void *bus_resource to soc_camera_link and get rid of the static > gpio_bus_switch and allocate it dynamically in .bus_init and free it in > .bus_release, but... No, I have no practical use for it at the moment. My feeling says it's better to add it, but just follow your feelings instead of mine ;) Sascha
diff --git a/arch/arm/mach-pxa/pcm990-baseboard.c b/arch/arm/mach-pxa/pcm990-baseboard.c index 34841c7..fd8f786 100644 --- a/arch/arm/mach-pxa/pcm990-baseboard.c +++ b/arch/arm/mach-pxa/pcm990-baseboard.c @@ -381,14 +381,49 @@ static struct pca953x_platform_data pca9536_data = { .gpio_base = NR_BUILTIN_GPIO + 1, }; -static struct soc_camera_link iclink[] = { - { - .bus_id = 0, /* Must match with the camera ID above */ - .gpio = NR_BUILTIN_GPIO + 1, - }, { - .bus_id = 0, /* Must match with the camera ID above */ - .gpio = -ENXIO, +static int gpio_bus_switch; + +static int pcm990_camera_set_bus_param(struct soc_camera_link *link, + unsigned long flags) +{ + if (gpio_bus_switch <= 0) { + if (flags == SOCAM_DATAWIDTH_10) + return 0; + else + return -EINVAL; + } + + if (flags & SOCAM_DATAWIDTH_8) + gpio_set_value(gpio_bus_switch, 1); + else + gpio_set_value(gpio_bus_switch, 0); + + return 0; +} + +static unsigned long pcm990_camera_query_bus_param(struct soc_camera_link *link) +{ + int ret; + + if (!gpio_bus_switch) { + ret = gpio_request(NR_BUILTIN_GPIO + 1, "camera"); + if (!ret) { + gpio_bus_switch = NR_BUILTIN_GPIO + 1; + gpio_direction_output(gpio_bus_switch, 0); + } else + gpio_bus_switch = -EINVAL; } + + if (gpio_bus_switch > 0) + return SOCAM_DATAWIDTH_8 | SOCAM_DATAWIDTH_10; + else + return SOCAM_DATAWIDTH_10; +} + +static struct soc_camera_link iclink = { + .bus_id = 0, /* Must match with the camera ID above */ + .query_bus_param = pcm990_camera_query_bus_param, + .set_bus_param = pcm990_camera_set_bus_param, }; /* Board I2C devices. */ @@ -399,10 +434,10 @@ static struct i2c_board_info __initdata pcm990_i2c_devices[] = { .platform_data = &pca9536_data, }, { I2C_BOARD_INFO("mt9v022", 0x48), - .platform_data = &iclink[0], /* With extender */ + .platform_data = &iclink, /* With extender */ }, { I2C_BOARD_INFO("mt9m001", 0x5d), - .platform_data = &iclink[0], /* With extender */ + .platform_data = &iclink, /* With extender */ }, }; #endif /* CONFIG_VIDEO_PXA27x ||CONFIG_VIDEO_PXA27x_MODULE */