| Message ID | 1236765976-20581-3-git-send-email-s.hauer@pengutronix.de (mailing list archive) |
|---|---|
| State | RFC |
| Headers | show |
On Wed, 11 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 | 50 +++++++++++++++++++++++++++------ > 1 files changed, 41 insertions(+), 9 deletions(-) > > diff --git a/arch/arm/mach-pxa/pcm990-baseboard.c b/arch/arm/mach-pxa/pcm990-baseboard.c > index 34841c7..e9feb89 100644 > --- a/arch/arm/mach-pxa/pcm990-baseboard.c > +++ b/arch/arm/mach-pxa/pcm990-baseboard.c > @@ -381,14 +381,46 @@ 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 device *dev, The prototype will change to use "struct soc_camera_link *" > + unsigned long flags) > +{ > + if (gpio_bus_switch <= 0) > + return 0; > + > + if (flags & SOCAM_DATAWIDTH_8) > + gpio_set_value(NR_BUILTIN_GPIO + 1, 1); > + else > + gpio_set_value(NR_BUILTIN_GPIO + 1, 0); You wanted to use gpio_bus_switch for these. > + > + return 0; > +} > + > +static unsigned long pcm990_camera_query_bus_param(struct device *dev) > +{ > + 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 = -1; This is a purely internal variable, so, I won't insist if you disagree, but, I think, a scheme "non-negative for a valid value or a negative error code" looks better, cf. If you want to initialize a structure with an invalid GPIO number, use some negative number (perhaps "-EINVAL"); that will never be valid. (Documentation/gpio.txt). "-1" looks like you're going to perform calculations with it. > } > + > + 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, > + .gpio = NR_BUILTIN_GPIO + 1, There's one patch missing in your patch series: [PATCH 5/5] Remove the "gpio" member from the struct soc_camera_link > }; > > /* Board I2C devices. */ > @@ -399,10 +431,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 > 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 Thu, Mar 12, 2009 at 09:31:55AM +0100, Guennadi Liakhovetski wrote: > On Wed, 11 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 | 50 +++++++++++++++++++++++++++------ > > 1 files changed, 41 insertions(+), 9 deletions(-) > > > > diff --git a/arch/arm/mach-pxa/pcm990-baseboard.c b/arch/arm/mach-pxa/pcm990-baseboard.c > > index 34841c7..e9feb89 100644 > > --- a/arch/arm/mach-pxa/pcm990-baseboard.c > > +++ b/arch/arm/mach-pxa/pcm990-baseboard.c > > @@ -381,14 +381,46 @@ 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 device *dev, > > The prototype will change to use "struct soc_camera_link *" OK > > > + unsigned long flags) > > +{ > > + if (gpio_bus_switch <= 0) > > + return 0; > > + > > + if (flags & SOCAM_DATAWIDTH_8) > > + gpio_set_value(NR_BUILTIN_GPIO + 1, 1); > > + else > > + gpio_set_value(NR_BUILTIN_GPIO + 1, 0); > > You wanted to use gpio_bus_switch for these. s/wanted to/should/? OK > > > + > > + return 0; > > +} > > + > > +static unsigned long pcm990_camera_query_bus_param(struct device *dev) > > +{ > > + 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 = -1; > > This is a purely internal variable, so, I won't insist if you disagree, > but, I think, a scheme "non-negative for a valid value or a negative error > code" looks better, cf. > > If you want to initialize a structure with an invalid GPIO number, use > some negative number (perhaps "-EINVAL"); that will never be valid. > > (Documentation/gpio.txt). "-1" looks like you're going to perform > calculations with it. OK > > > } > > + > > + 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, > > + .gpio = NR_BUILTIN_GPIO + 1, > > There's one patch missing in your patch series: > > [PATCH 5/5] Remove the "gpio" member from the struct soc_camera_link OK. I saw this member is unnecessary now and forgot it a minute later... Sascha
One more thing I noticed while looking at your patch 3/4: > +static int pcm990_camera_set_bus_param(struct device *dev, > + unsigned long flags) > +{ > + if (gpio_bus_switch <= 0) > + return 0; > + > + if (flags & SOCAM_DATAWIDTH_8) > + gpio_set_value(NR_BUILTIN_GPIO + 1, 1); > + else > + gpio_set_value(NR_BUILTIN_GPIO + 1, 0); Originally the logic here was "only if flags == SOCAM_DATAWIDTH_8, switch to 8 bits, otherwise do 10 bits. I.e., if flags == SOCAM_DATAWIDTH_8 | SOCAM_DATAWIDTH_10, it would still do the default (and wider) 10 bits. Do you have any reason to change that logic? 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 Thu, Mar 12, 2009 at 09:40:46AM +0100, Guennadi Liakhovetski wrote: > One more thing I noticed while looking at your patch 3/4: > > > +static int pcm990_camera_set_bus_param(struct device *dev, > > + unsigned long flags) > > +{ > > + if (gpio_bus_switch <= 0) > > + return 0; > > + > > + if (flags & SOCAM_DATAWIDTH_8) > > + gpio_set_value(NR_BUILTIN_GPIO + 1, 1); > > + else > > + gpio_set_value(NR_BUILTIN_GPIO + 1, 0); > > Originally the logic here was "only if flags == SOCAM_DATAWIDTH_8, switch > to 8 bits, otherwise do 10 bits. I.e., if flags == SOCAM_DATAWIDTH_8 | > SOCAM_DATAWIDTH_10, it would still do the default (and wider) 10 bits. Do > you have any reason to change that logic? I was not aware that I changed any logic. I thought I would get here with only one of the SOCAM_DATAWIDTH_* set. Isn't it a bug when we get here with more than one width flags set? The mt9v022 driver has this in set_bus_param: > > /* Only one width bit may be set */ > if (!is_power_of_2(width_flag)) > return -EINVAL; > And I think it makes sense. Sascha
On Thu, 12 Mar 2009, Sascha Hauer wrote: > On Thu, Mar 12, 2009 at 09:40:46AM +0100, Guennadi Liakhovetski wrote: > > One more thing I noticed while looking at your patch 3/4: > > > > > +static int pcm990_camera_set_bus_param(struct device *dev, > > > + unsigned long flags) > > > +{ > > > + if (gpio_bus_switch <= 0) > > > + return 0; > > > + > > > + if (flags & SOCAM_DATAWIDTH_8) > > > + gpio_set_value(NR_BUILTIN_GPIO + 1, 1); > > > + else > > > + gpio_set_value(NR_BUILTIN_GPIO + 1, 0); > > > > Originally the logic here was "only if flags == SOCAM_DATAWIDTH_8, switch > > to 8 bits, otherwise do 10 bits. I.e., if flags == SOCAM_DATAWIDTH_8 | > > SOCAM_DATAWIDTH_10, it would still do the default (and wider) 10 bits. Do > > you have any reason to change that logic? > > I was not aware that I changed any logic. I thought I would get here > with only one of the SOCAM_DATAWIDTH_* set. Isn't it a bug when we get > here with more than one width flags set? > > The mt9v022 driver has this in set_bus_param: > > > > > /* Only one width bit may be set */ > > if (!is_power_of_2(width_flag)) > > return -EINVAL; > > > > And I think it makes sense. Ok, then, could you, please add the same test to mt9m001? And, as I mentioned in a comment to 3/4, please, return an error if switching is requested but unsupported. 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 Thu, Mar 12, 2009 at 10:25:35AM +0100, Guennadi Liakhovetski wrote: > On Thu, 12 Mar 2009, Sascha Hauer wrote: > > > On Thu, Mar 12, 2009 at 09:40:46AM +0100, Guennadi Liakhovetski wrote: > > > One more thing I noticed while looking at your patch 3/4: > > > > > > > +static int pcm990_camera_set_bus_param(struct device *dev, > > > > + unsigned long flags) > > > > +{ > > > > + if (gpio_bus_switch <= 0) > > > > + return 0; > > > > + > > > > + if (flags & SOCAM_DATAWIDTH_8) > > > > + gpio_set_value(NR_BUILTIN_GPIO + 1, 1); > > > > + else > > > > + gpio_set_value(NR_BUILTIN_GPIO + 1, 0); > > > > > > Originally the logic here was "only if flags == SOCAM_DATAWIDTH_8, switch > > > to 8 bits, otherwise do 10 bits. I.e., if flags == SOCAM_DATAWIDTH_8 | > > > SOCAM_DATAWIDTH_10, it would still do the default (and wider) 10 bits. Do > > > you have any reason to change that logic? > > > > I was not aware that I changed any logic. I thought I would get here > > with only one of the SOCAM_DATAWIDTH_* set. Isn't it a bug when we get > > here with more than one width flags set? > > > > The mt9v022 driver has this in set_bus_param: > > > > > > > > /* Only one width bit may be set */ > > > if (!is_power_of_2(width_flag)) > > > return -EINVAL; > > > > > > > And I think it makes sense. > > Ok, then, could you, please add the same test to mt9m001? And, as I > mentioned in a comment to 3/4, please, return an error if switching is > requested but unsupported. Ok, will do. It may be that we need a different approach to the bus width setting in the longer term. For example the mt9m001 can support any thinkable bus width of 1 to inf. bits. This depends on the hardware designer who wired up the connection and not the physical count of data lines the chip has. I have no idea how to implement this though. Sascha
diff --git a/arch/arm/mach-pxa/pcm990-baseboard.c b/arch/arm/mach-pxa/pcm990-baseboard.c index 34841c7..e9feb89 100644 --- a/arch/arm/mach-pxa/pcm990-baseboard.c +++ b/arch/arm/mach-pxa/pcm990-baseboard.c @@ -381,14 +381,46 @@ 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 device *dev, + unsigned long flags) +{ + if (gpio_bus_switch <= 0) + return 0; + + if (flags & SOCAM_DATAWIDTH_8) + gpio_set_value(NR_BUILTIN_GPIO + 1, 1); + else + gpio_set_value(NR_BUILTIN_GPIO + 1, 0); + + return 0; +} + +static unsigned long pcm990_camera_query_bus_param(struct device *dev) +{ + 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 = -1; } + + 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, + .gpio = NR_BUILTIN_GPIO + 1, }; /* Board I2C devices. */ @@ -399,10 +431,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 */
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 | 50 +++++++++++++++++++++++++++------ 1 files changed, 41 insertions(+), 9 deletions(-)