Re: [PATCH 43/47] adv7604: Control hot-plug detect through a GPIO
On 02/11/14 13:03, Laurent Pinchart wrote: Hi Hans, On Tuesday 11 February 2014 11:09:31 Hans Verkuil wrote: On 02/05/14 17:42, Laurent Pinchart wrote: Replace the ADV7604-specific hotplug notifier with a GPIO to control the HPD pin directly instead of going through the bridge driver. Hmm, that's not going to work for me. I don't have a GPIO pin here, instead it is a bit in a register that I have to set. But that bit controls a GPIO, doesn't it ? In that case it should be exposed as a GPIO controller. I feel unhappy about losing this notifier for two reasons: first adding a GPIO controller just to toggle a bit adds 40 lines to my driver, and that doesn't sit well with me. It's basically completely unnecessary overhead. The second reason is that in some cases you want to do something in addition to just toggling the hotplug pin. In particular for CEC support this could be quite useful. In fact, if the adv7604 supports the ARC feature (Audio Return Channel), then this is really needed because in that case the hotplug toggling would have to be done via CEC CDC commands. However, while this webpage claims that the ARC is supported, I can't find any other information about that. http://www.analog.com/en/audiovideo-products/analoghdmidvi-interfaces/adv7604/products/product.html Lars-Peter, do you know anything about ARC support in the adv7604? Regards, Hans -- 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 43/47] adv7604: Control hot-plug detect through a GPIO
On 02/13/2014 10:47 AM, Hans Verkuil wrote: On 02/11/14 13:03, Laurent Pinchart wrote: Hi Hans, On Tuesday 11 February 2014 11:09:31 Hans Verkuil wrote: On 02/05/14 17:42, Laurent Pinchart wrote: Replace the ADV7604-specific hotplug notifier with a GPIO to control the HPD pin directly instead of going through the bridge driver. Hmm, that's not going to work for me. I don't have a GPIO pin here, instead it is a bit in a register that I have to set. But that bit controls a GPIO, doesn't it ? In that case it should be exposed as a GPIO controller. I feel unhappy about losing this notifier for two reasons: first adding a GPIO controller just to toggle a bit adds 40 lines to my driver, and that doesn't sit well with me. It's basically completely unnecessary overhead. I don't think it's a problem to just keep the hotplug event for now. It is not a block for other features, so we can just leave it in. The second reason is that in some cases you want to do something in addition to just toggling the hotplug pin. In particular for CEC support this could be quite useful. In fact, if the adv7604 supports the ARC feature (Audio Return Channel), then this is really needed because in that case the hotplug toggling would have to be done via CEC CDC commands. However, while this webpage claims that the ARC is supported, I can't find any other information about that. http://www.analog.com/en/audiovideo-products/analoghdmidvi-interfaces/adv7604/products/product.html Lars-Peter, do you know anything about ARC support in the adv7604? No, I think that might be a mistake on the website, but I'll check with somebody who knows more about this than me. - Lars -- 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 43/47] adv7604: Control hot-plug detect through a GPIO
Hi Hans, On Thursday 13 February 2014 10:47:44 Hans Verkuil wrote: On 02/11/14 13:03, Laurent Pinchart wrote: On Tuesday 11 February 2014 11:09:31 Hans Verkuil wrote: On 02/05/14 17:42, Laurent Pinchart wrote: Replace the ADV7604-specific hotplug notifier with a GPIO to control the HPD pin directly instead of going through the bridge driver. Hmm, that's not going to work for me. I don't have a GPIO pin here, instead it is a bit in a register that I have to set. But that bit controls a GPIO, doesn't it ? In that case it should be exposed as a GPIO controller. I feel unhappy about losing this notifier for two reasons: first adding a GPIO controller just to toggle a bit adds 40 lines to my driver, and that doesn't sit well with me. It's basically completely unnecessary overhead. The second reason is that in some cases you want to do something in addition to just toggling the hotplug pin. In particular for CEC support this could be quite useful. As discuss over IRC, I'll keep the HPD notification and add optional GPIO support in v2. In fact, if the adv7604 supports the ARC feature (Audio Return Channel), then this is really needed because in that case the hotplug toggling would have to be done via CEC CDC commands. However, while this webpage claims that the ARC is supported, I can't find any other information about that. http://www.analog.com/en/audiovideo-products/analoghdmidvi-interfaces/adv760 4/products/product.html Lars-Peter, do you know anything about ARC support in the adv7604? -- Regards, Laurent Pinchart -- 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 43/47] adv7604: Control hot-plug detect through a GPIO
On 02/05/14 17:42, Laurent Pinchart wrote:
Replace the ADV7604-specific hotplug notifier with a GPIO to control the
HPD pin directly instead of going through the bridge driver.
Hmm, that's not going to work for me. I don't have a GPIO pin here, instead
it is a bit in a register that I have to set.
Signed-off-by: Laurent Pinchart laurent.pinch...@ideasonboard.com
---
drivers/media/i2c/adv7604.c | 47
+
include/media/adv7604.h | 5 -
2 files changed, 47 insertions(+), 5 deletions(-)
diff --git a/drivers/media/i2c/adv7604.c b/drivers/media/i2c/adv7604.c
index 369cb1e..2f38071 100644
--- a/drivers/media/i2c/adv7604.c
+++ b/drivers/media/i2c/adv7604.c
@@ -28,6 +28,7 @@
*/
#include linux/delay.h
+#include linux/gpio.h
#include linux/i2c.h
#include linux/kernel.h
#include linux/module.h
@@ -608,6 +609,23 @@ static inline int edid_write_block(struct v4l2_subdev
*sd,
return err;
}
+static void adv7604_set_hpd(struct adv7604_state *state, unsigned int hpd)
+{
+ unsigned int i;
+
+ for (i = 0; i state-info-num_dv_ports; ++i) {
+ unsigned int enable = hpd BIT(i);
+
+ if (IS_ERR_VALUE(state-pdata.hpd_gpio[i]))
IS_ERR_VALUE: that's normally used for pointers, not integers. I would
much prefer something simple like 'bool hpd_use_gpio[4]'.
Regards,
Hans
+ continue;
+
+ if (state-pdata.hpd_gpio_low[i])
+ enable = !enable;
+
+ gpio_set_value_cansleep(state-pdata.hpd_gpio[i], enable);
+ }
+}
+
static void adv7604_delayed_work_enable_hotplug(struct work_struct *work)
{
struct delayed_work *dwork = to_delayed_work(work);
@@ -617,7 +635,7 @@ static void adv7604_delayed_work_enable_hotplug(struct
work_struct *work)
v4l2_dbg(2, debug, sd, %s: enable hotplug\n, __func__);
- v4l2_subdev_notify(sd, ADV7604_HOTPLUG, (void *)state-edid.present);
+ adv7604_set_hpd(state, state-edid.present);
}
static inline int hdmi_read(struct v4l2_subdev *sd, u8 reg)
@@ -2022,7 +2040,6 @@ static int adv7604_set_edid(struct v4l2_subdev *sd,
struct v4l2_subdev_edid *edi
struct adv7604_state *state = to_state(sd);
const struct adv7604_chip_info *info = state-info;
int spa_loc;
- int tmp = 0;
int err;
int i;
@@ -2033,7 +2050,7 @@ static int adv7604_set_edid(struct v4l2_subdev *sd,
struct v4l2_subdev_edid *edi
if (edid-blocks == 0) {
/* Disable hotplug and I2C access to EDID RAM from DDC port */
state-edid.present = ~(1 edid-pad);
- v4l2_subdev_notify(sd, ADV7604_HOTPLUG, (void
*)state-edid.present);
+ adv7604_set_hpd(state, state-edid.present);
rep_write_clr_set(sd, info-edid_enable_reg, 0x0f,
state-edid.present);
/* Fall back to a 16:9 aspect ratio */
@@ -2059,7 +2076,7 @@ static int adv7604_set_edid(struct v4l2_subdev *sd,
struct v4l2_subdev_edid *edi
/* Disable hotplug and I2C access to EDID RAM from DDC port */
cancel_delayed_work_sync(state-delayed_work_enable_hotplug);
- v4l2_subdev_notify(sd, ADV7604_HOTPLUG, (void *)tmp);
+ adv7604_set_hpd(state, 0);
rep_write_clr_set(sd, info-edid_enable_reg, 0x0f, 0x00);
spa_loc = get_edid_spa_location(edid-edid);
@@ -2655,6 +2672,28 @@ static int adv7604_probe(struct i2c_client *client,
return -ENODEV;
}
state-pdata = *pdata;
+
+ /* Request GPIOs. */
+ for (i = 0; i state-info-num_dv_ports; ++i) {
+ char name[5];
+
+ if (IS_ERR_VALUE(state-pdata.hpd_gpio[i]))
+ continue;
+
+ sprintf(name, hpd%u, i);
+ err = devm_gpio_request_one(client-dev,
+ state-pdata.hpd_gpio[i],
+ state-pdata.hpd_gpio_low[i] ?
+ GPIOF_OUT_INIT_HIGH :
+ GPIOF_OUT_INIT_LOW,
+ name);
+ if (err 0) {
+ v4l_err(client, Failed to request HPD %u GPIO (%u)\n,
+ i, state-pdata.hpd_gpio[i]);
+ return err;
+ }
+ }
+
state-timings = cea640x480;
state-format = adv7604_format_info(state, V4L2_MBUS_FMT_YUYV8_2X8);
diff --git a/include/media/adv7604.h b/include/media/adv7604.h
index 4da678c..dddb0cb 100644
--- a/include/media/adv7604.h
+++ b/include/media/adv7604.h
@@ -90,6 +90,10 @@ struct adv7604_platform_data {
/* DIS_CABLE_DET_RST: 1 if the 5V pins are unused and unconnected */
unsigned disable_cable_det_rst:1;
+ /* Hot-Plug Detect control GPIOs */
+ int hpd_gpio[4];
+ bool hpd_gpio_low[4];
+
/* Analog
Re: [PATCH 43/47] adv7604: Control hot-plug detect through a GPIO
Hi Hans,
On Tuesday 11 February 2014 11:09:31 Hans Verkuil wrote:
On 02/05/14 17:42, Laurent Pinchart wrote:
Replace the ADV7604-specific hotplug notifier with a GPIO to control the
HPD pin directly instead of going through the bridge driver.
Hmm, that's not going to work for me. I don't have a GPIO pin here, instead
it is a bit in a register that I have to set.
But that bit controls a GPIO, doesn't it ? In that case it should be exposed
as a GPIO controller.
Signed-off-by: Laurent Pinchart laurent.pinch...@ideasonboard.com
---
drivers/media/i2c/adv7604.c | 47
include/media/adv7604.h | 5 -
2 files changed, 47 insertions(+), 5 deletions(-)
diff --git a/drivers/media/i2c/adv7604.c b/drivers/media/i2c/adv7604.c
index 369cb1e..2f38071 100644
--- a/drivers/media/i2c/adv7604.c
+++ b/drivers/media/i2c/adv7604.c
@@ -28,6 +28,7 @@
*/
#include linux/delay.h
+#include linux/gpio.h
#include linux/i2c.h
#include linux/kernel.h
#include linux/module.h
@@ -608,6 +609,23 @@ static inline int edid_write_block(struct v4l2_subdev
*sd,
return err;
}
+static void adv7604_set_hpd(struct adv7604_state *state, unsigned int
hpd)
+{
+ unsigned int i;
+
+ for (i = 0; i state-info-num_dv_ports; ++i) {
+ unsigned int enable = hpd BIT(i);
+
+ if (IS_ERR_VALUE(state-pdata.hpd_gpio[i]))
IS_ERR_VALUE: that's normally used for pointers, not integers. I would
much prefer something simple like 'bool hpd_use_gpio[4]'.
I've reworked this to use the gpiod_* API, I'll post v2 when the whole set
will be reviewed.
+ continue;
+
+ if (state-pdata.hpd_gpio_low[i])
+ enable = !enable;
+
+ gpio_set_value_cansleep(state-pdata.hpd_gpio[i], enable);
+ }
+}
+
static void adv7604_delayed_work_enable_hotplug(struct work_struct *work)
{
struct delayed_work *dwork = to_delayed_work(work);
@@ -617,7 +635,7 @@ static void adv7604_delayed_work_enable_hotplug(struct
work_struct *work)
v4l2_dbg(2, debug, sd, %s: enable hotplug\n, __func__);
- v4l2_subdev_notify(sd, ADV7604_HOTPLUG, (void *)state-
edid.present);
+ adv7604_set_hpd(state, state-edid.present);
}
static inline int hdmi_read(struct v4l2_subdev *sd, u8 reg)
@@ -2022,7 +2040,6 @@ static int adv7604_set_edid(struct v4l2_subdev *sd,
struct v4l2_subdev_edid *edi
struct adv7604_state *state = to_state(sd);
const struct adv7604_chip_info *info = state-info;
int spa_loc;
- int tmp = 0;
int err;
int i;
@@ -2033,7 +2050,7 @@ static int adv7604_set_edid(struct v4l2_subdev *sd,
struct v4l2_subdev_edid *edi
if (edid-blocks == 0) {
/* Disable hotplug and I2C access to EDID RAM from DDC port */
state-edid.present = ~(1 edid-pad);
- v4l2_subdev_notify(sd, ADV7604_HOTPLUG, (void *)state-
edid.present);
+ adv7604_set_hpd(state, state-edid.present);
rep_write_clr_set(sd, info-edid_enable_reg, 0x0f,
state-edid.present);
/* Fall back to a 16:9 aspect ratio */
@@ -2059,7 +2076,7 @@ static int adv7604_set_edid(struct v4l2_subdev *sd,
struct v4l2_subdev_edid *edi
/* Disable hotplug and I2C access to EDID RAM from DDC port */
cancel_delayed_work_sync(state-delayed_work_enable_hotplug);
- v4l2_subdev_notify(sd, ADV7604_HOTPLUG, (void *)tmp);
+ adv7604_set_hpd(state, 0);
rep_write_clr_set(sd, info-edid_enable_reg, 0x0f, 0x00);
spa_loc = get_edid_spa_location(edid-edid);
@@ -2655,6 +2672,28 @@ static int adv7604_probe(struct i2c_client *client,
return -ENODEV;
}
state-pdata = *pdata;
+
+ /* Request GPIOs. */
+ for (i = 0; i state-info-num_dv_ports; ++i) {
+ char name[5];
+
+ if (IS_ERR_VALUE(state-pdata.hpd_gpio[i]))
+ continue;
+
+ sprintf(name, hpd%u, i);
+ err = devm_gpio_request_one(client-dev,
+ state-pdata.hpd_gpio[i],
+ state-pdata.hpd_gpio_low[i] ?
+ GPIOF_OUT_INIT_HIGH :
+ GPIOF_OUT_INIT_LOW,
+ name);
+ if (err 0) {
+ v4l_err(client, Failed to request HPD %u GPIO (%u)\n,
+ i, state-pdata.hpd_gpio[i]);
+ return err;
+ }
+ }
+
state-timings = cea640x480;
state-format = adv7604_format_info(state, V4L2_MBUS_FMT_YUYV8_2X8);
diff --git a/include/media/adv7604.h b/include/media/adv7604.h
index 4da678c..dddb0cb 100644
--- a/include/media/adv7604.h
+++ b/include/media/adv7604.h
@@ -90,6 +90,10 @@ struct
Re: [PATCH 43/47] adv7604: Control hot-plug detect through a GPIO
Hi Lars, On Thursday 06 February 2014 14:10:05 Lars-Peter Clausen wrote: On 02/05/2014 05:42 PM, Laurent Pinchart wrote: Replace the ADV7604-specific hotplug notifier with a GPIO to control the HPD pin directly instead of going through the bridge driver. Signed-off-by: Laurent Pinchart laurent.pinch...@ideasonboard.com This should probably use the new GPIO descriptor API[1]. It will make things a bit simpler as it allows to move the active low handling to the gpio subsystem. The other thing is that it makes integration with devicetree nicer. Agreed. I'll fix that for the next version. -- Regards, Laurent Pinchart -- 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 43/47] adv7604: Control hot-plug detect through a GPIO
On 02/05/2014 05:42 PM, Laurent Pinchart wrote: Replace the ADV7604-specific hotplug notifier with a GPIO to control the HPD pin directly instead of going through the bridge driver. Signed-off-by: Laurent Pinchart laurent.pinch...@ideasonboard.com This should probably use the new GPIO descriptor API[1]. It will make things a bit simpler as it allows to move the active low handling to the gpio subsystem. The other thing is that it makes integration with devicetree nicer. - Lars [1] http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/include/linux/gpio/consumer.h -- 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
