[PATCH 0/3] tc358743: minor driver fixes

[PATCH 0/3] tc358743: minor driver fixes

[Dave Stevenson]

These 3 patches for TC358743 came out of trying to use the
existing driver with a new Raspberry Pi CSI-2 receiver driver.

A couple of the subdevice API calls were not implemented or
otherwise gave odd results. Those are fixed.

The TC358743 interface board being used didn't have the IRQ
line wired up to the SoC. "interrupts" is listed as being
optional in the DT binding, but the driver didn't actually
function if it wasn't provided.

Dave Stevenson (3):
  [media] tc358743: Add enum_mbus_code
  [media] tc358743: Setup default mbus_fmt before registering
  [media] tc358743: Add support for platforms without IRQ line

 drivers/media/i2c/tc358743.c | 59 +++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 58 insertions(+), 1 deletion(-)

-- 
2.7.4

[PATCH 1/3] [media] tc358743: Add enum_mbus_code

[Dave Stevenson]

There was no way to query the supported mbus formats from this
driver. enum_mbus_code is the function to expose that, so
implement it.

Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.org>
---
 drivers/media/i2c/tc358743.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/drivers/media/i2c/tc358743.c b/drivers/media/i2c/tc358743.c
index 3251cba..06bfdca 100644
--- a/drivers/media/i2c/tc358743.c
+++ b/drivers/media/i2c/tc358743.c
@@ -1473,6 +1473,23 @@ static int tc358743_s_stream(struct v4l2_subdev *sd, int enable)
 
 /* --------------- PAD OPS --------------- */
 
+static int tc358743_enum_mbus_code(struct v4l2_subdev *sd,
+		struct v4l2_subdev_pad_config *cfg,
+		struct v4l2_subdev_mbus_code_enum *code)
+{
+	switch (code->index) {
+	case 0:
+		code->code = MEDIA_BUS_FMT_RGB888_1X24;
+		break;
+	case 1:
+		code->code = MEDIA_BUS_FMT_UYVY8_1X16;
+		break;
+	default:
+		return -EINVAL;
+	}
+	return 0;
+}
+
 static int tc358743_get_fmt(struct v4l2_subdev *sd,
 		struct v4l2_subdev_pad_config *cfg,
 		struct v4l2_subdev_format *format)
@@ -1642,6 +1659,7 @@ static const struct v4l2_subdev_video_ops tc358743_video_ops = {
 };
 
 static const struct v4l2_subdev_pad_ops tc358743_pad_ops = {
+	.enum_mbus_code = tc358743_enum_mbus_code,
 	.set_fmt = tc358743_set_fmt,
 	.get_fmt = tc358743_get_fmt,
 	.get_edid = tc358743_g_edid,
-- 
2.7.4

[PATCH 2/3] [media] tc358743: Setup default mbus_fmt before registering

[Dave Stevenson]

Previously the mbus_fmt_code was set after the device was
registered. If a connected sub-device called tc358743_get_fmt
prior to that point it would get an invalid code back.

Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.org>
---
 drivers/media/i2c/tc358743.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/media/i2c/tc358743.c b/drivers/media/i2c/tc358743.c
index 06bfdca..2f5763d 100644
--- a/drivers/media/i2c/tc358743.c
+++ b/drivers/media/i2c/tc358743.c
@@ -1905,6 +1905,8 @@ static int tc358743_probe(struct i2c_client *client,
 	if (err < 0)
 		goto err_hdl;
 
+	state->mbus_fmt_code = MEDIA_BUS_FMT_RGB888_1X24;
+
 	sd->dev = &client->dev;
 	err = v4l2_async_register_subdev(sd);
 	if (err < 0)
@@ -1919,7 +1921,6 @@ static int tc358743_probe(struct i2c_client *client,
 
 	tc358743_s_dv_timings(sd, &default_timing);
 
-	state->mbus_fmt_code = MEDIA_BUS_FMT_RGB888_1X24;
 	tc358743_set_csi_color_space(sd);
 
 	tc358743_init_interrupts(sd);
-- 
2.7.4

[PATCH 3/3] [media] tc358743: Add support for platforms without IRQ line

[Dave Stevenson]

interrupts is listed as an optional property in the DT
binding, but in reality the driver didn't work without it.
The existing driver relied on having the interrupt line
connected to the SoC to trigger handling various events.

Add the option to poll the interrupt status register via
a timer if no interrupt source is defined.

Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.org>
---
 drivers/media/i2c/tc358743.c | 38 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 38 insertions(+)

diff --git a/drivers/media/i2c/tc358743.c b/drivers/media/i2c/tc358743.c
index 2f5763d..654aac2 100644
--- a/drivers/media/i2c/tc358743.c
+++ b/drivers/media/i2c/tc358743.c
@@ -33,6 +33,7 @@
 #include <linux/delay.h>
 #include <linux/gpio/consumer.h>
 #include <linux/interrupt.h>
+#include <linux/timer.h>
 #include <linux/videodev2.h>
 #include <linux/workqueue.h>
 #include <linux/v4l2-dv-timings.h>
@@ -61,6 +62,8 @@ MODULE_LICENSE("GPL");
 
 #define I2C_MAX_XFER_SIZE  (EDID_BLOCK_SIZE + 2)
 
+#define POLL_INTERVAL_MS	1000
+
 static const struct v4l2_dv_timings_cap tc358743_timings_cap = {
 	.type = V4L2_DV_BT_656_1120,
 	/* keep this initialization for compatibility with GCC < 4.4.6 */
@@ -91,6 +94,9 @@ struct tc358743_state {
 
 	struct delayed_work delayed_work_enable_hotplug;
 
+	struct timer_list timer;
+	struct work_struct work_i2c_poll;
+
 	/* edid  */
 	u8 edid_blocks_written;
 
@@ -1319,6 +1325,24 @@ static irqreturn_t tc358743_irq_handler(int irq, void *dev_id)
 	return handled ? IRQ_HANDLED : IRQ_NONE;
 }
 
+static void tc358743_irq_poll_timer(unsigned long arg)
+{
+	struct tc358743_state *state = (struct tc358743_state *)arg;
+
+	schedule_work(&state->work_i2c_poll);
+
+	mod_timer(&state->timer, jiffies + msecs_to_jiffies(POLL_INTERVAL_MS));
+}
+
+static void tc358743_work_i2c_poll(struct work_struct *work)
+{
+	struct tc358743_state *state = container_of(work,
+			struct tc358743_state, work_i2c_poll);
+	bool handled;
+
+	tc358743_isr(&state->sd, 0, &handled);
+}
+
 static int tc358743_subscribe_event(struct v4l2_subdev *sd, struct v4l2_fh *fh,
 				    struct v4l2_event_subscription *sub)
 {
@@ -1933,6 +1957,14 @@ static int tc358743_probe(struct i2c_client *client,
 						"tc358743", state);
 		if (err)
 			goto err_work_queues;
+	} else {
+		INIT_WORK(&state->work_i2c_poll,
+			  tc358743_work_i2c_poll);
+		state->timer.data = (unsigned long)state;
+		state->timer.function = tc358743_irq_poll_timer;
+		state->timer.expires = jiffies +
+				       msecs_to_jiffies(POLL_INTERVAL_MS);
+		add_timer(&state->timer);
 	}
 
 	tc358743_enable_interrupts(sd, tx_5v_power_present(sd));
@@ -1948,6 +1980,8 @@ static int tc358743_probe(struct i2c_client *client,
 	return 0;
 
 err_work_queues:
+	if (!state->i2c_client->irq)
+		flush_work(&state->work_i2c_poll);
 	cancel_delayed_work(&state->delayed_work_enable_hotplug);
 	mutex_destroy(&state->confctl_mutex);
 err_hdl:
@@ -1961,6 +1995,10 @@ static int tc358743_remove(struct i2c_client *client)
 	struct v4l2_subdev *sd = i2c_get_clientdata(client);
 	struct tc358743_state *state = to_state(sd);
 
+	if (!state->i2c_client->irq) {
+		del_timer_sync(&state->timer);
+		flush_work(&state->work_i2c_poll);
+	}
 	cancel_delayed_work(&state->delayed_work_enable_hotplug);
 	v4l2_async_unregister_subdev(sd);
 	v4l2_device_unregister_subdev(sd);
-- 
2.7.4

Re: [PATCH 0/3] tc358743: minor driver fixes

[Hans Verkuil]

On 06/02/17 14:18, Dave Stevenson wrote:
> These 3 patches for TC358743 came out of trying to use the
> existing driver with a new Raspberry Pi CSI-2 receiver driver.

Nice! Doing that has been on my todo list for ages but I never got
around to it. I have one of these and using the Raspberry Pi with
the tc358743 would allow me to add a CEC driver as well.

> A couple of the subdevice API calls were not implemented or
> otherwise gave odd results. Those are fixed.
> 
> The TC358743 interface board being used didn't have the IRQ
> line wired up to the SoC. "interrupts" is listed as being
> optional in the DT binding, but the driver didn't actually
> function if it wasn't provided.
> 
> Dave Stevenson (3):
>   [media] tc358743: Add enum_mbus_code
>   [media] tc358743: Setup default mbus_fmt before registering
>   [media] tc358743: Add support for platforms without IRQ line

All looks good, I'll take this for 4.12.

Regards,

	Hans

> 
>  drivers/media/i2c/tc358743.c | 59 +++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 58 insertions(+), 1 deletion(-)
> 

Re: [PATCH 0/3] tc358743: minor driver fixes

[Dave Stevenson]

Hi Hans.

On 2 June 2017 at 13:35, Hans Verkuil <hverkuil@xs4all.nl> wrote:
> On 06/02/17 14:18, Dave Stevenson wrote:
>> These 3 patches for TC358743 came out of trying to use the
>> existing driver with a new Raspberry Pi CSI-2 receiver driver.
>
> Nice! Doing that has been on my todo list for ages but I never got
> around to it. I have one of these and using the Raspberry Pi with
> the tc358743 would allow me to add a CEC driver as well.

It's been on my list for a while too! It's working, but just the final
clean ups needed.
I've got 1 v4l2-compliance failure still outstanding that needs
digging into (subscribe_event), rebasing on top of the fwnode tree,
and a couple of config things to tidy up. RFC hopefully next week.
I'm testing with a demo board designed here at Pi Towers, but there
are others successfully testing it using the auvidea.com B101 board.

Are you aware of the HDMI modes that the TC358743 driver has been used with?
The comments mention 720P60 at 594MHz, but I have had to modify the
fifo_level value from 16 to 110 to get VGA60 or 576P50 to work. (The
value came out of Toshiba's spreadsheet for computing register
settings). It increases the delay by 2.96usecs at 720P60 on 2 lanes,
so not a huge change.
Is it worth going to the effort of dynamically computing the delay, or
is increasing the default acceptable?

>> A couple of the subdevice API calls were not implemented or
>> otherwise gave odd results. Those are fixed.
>>
>> The TC358743 interface board being used didn't have the IRQ
>> line wired up to the SoC. "interrupts" is listed as being
>> optional in the DT binding, but the driver didn't actually
>> function if it wasn't provided.
>>
>> Dave Stevenson (3):
>>   [media] tc358743: Add enum_mbus_code
>>   [media] tc358743: Setup default mbus_fmt before registering
>>   [media] tc358743: Add support for platforms without IRQ line
>
> All looks good, I'll take this for 4.12.

Thanks.

> Regards,
>
>         Hans
>
>>
>>  drivers/media/i2c/tc358743.c | 59 +++++++++++++++++++++++++++++++++++++++++++-
>>  1 file changed, 58 insertions(+), 1 deletion(-)
>>
>

Re: [PATCH 0/3] tc358743: minor driver fixes

[Hans Verkuil]

On 06/02/17 15:03, Dave Stevenson wrote:
> Hi Hans.
> 
> On 2 June 2017 at 13:35, Hans Verkuil <hverkuil@xs4all.nl> wrote:
>> On 06/02/17 14:18, Dave Stevenson wrote:
>>> These 3 patches for TC358743 came out of trying to use the
>>> existing driver with a new Raspberry Pi CSI-2 receiver driver.
>>
>> Nice! Doing that has been on my todo list for ages but I never got
>> around to it. I have one of these and using the Raspberry Pi with
>> the tc358743 would allow me to add a CEC driver as well.
> 
> It's been on my list for a while too! It's working, but just the final
> clean ups needed.
> I've got 1 v4l2-compliance failure still outstanding that needs
> digging into (subscribe_event), rebasing on top of the fwnode tree,
> and a couple of config things to tidy up. RFC hopefully next week.
> I'm testing with a demo board designed here at Pi Towers, but there
> are others successfully testing it using the auvidea.com B101 board.
> 
> Are you aware of the HDMI modes that the TC358743 driver has been used with?
> The comments mention 720P60 at 594MHz, but I have had to modify the
> fifo_level value from 16 to 110 to get VGA60 or 576P50 to work. (The
> value came out of Toshiba's spreadsheet for computing register
> settings). It increases the delay by 2.96usecs at 720P60 on 2 lanes,
> so not a huge change.
> Is it worth going to the effort of dynamically computing the delay, or
> is increasing the default acceptable?

I see that the fifo_level value of 16 was supplied by Philipp Zabel, so
I have CC-ed him as I am not sure where those values came from. This driver
is also used in a Cisco product, but we use platform_data for that. Here are
our settings that we use for reference:

        static struct tc358743_platform_data tc358743_pdata = {
                .refclk_hz = 27000000,
                .ddc5v_delay = DDC5V_DELAY_100_MS,
                .fifo_level = 300,
                .pll_prd = 4,
                .pll_fbd = 122,
                /* CSI */
                .lineinitcnt = 0x00001770,
                .lptxtimecnt = 0x00000005,
                .tclk_headercnt = 0x00001d04,
                .ths_headercnt = 0x00000505,
                .twakeup = 0x00004650,
                .ths_trailcnt = 0x00000004,
                .hstxvregcnt = 0x00000005,
                /* HDMI PHY */
                .hdmi_phy_auto_reset_tmds_detected = true,
                .hdmi_phy_auto_reset_tmds_in_range = true,
                .hdmi_phy_auto_reset_tmds_valid = true,
                .hdmi_phy_auto_reset_hsync_out_of_range = true,
                .hdmi_phy_auto_reset_vsync_out_of_range = true,
                .hdmi_detection_delay = HDMI_MODE_DELAY_25_MS,
        };

I believe these are all calculated from the Toshiba spreadsheet.

Frankly, I have no idea what they mean :-)

I am fine with increasing the default if Philipp is OK as well. Since
Cisco uses a value of 300 I would expect that 16 is indeed too low.

Regards,

	Hans

> 
>>> A couple of the subdevice API calls were not implemented or
>>> otherwise gave odd results. Those are fixed.
>>>
>>> The TC358743 interface board being used didn't have the IRQ
>>> line wired up to the SoC. "interrupts" is listed as being
>>> optional in the DT binding, but the driver didn't actually
>>> function if it wasn't provided.
>>>
>>> Dave Stevenson (3):
>>>   [media] tc358743: Add enum_mbus_code
>>>   [media] tc358743: Setup default mbus_fmt before registering
>>>   [media] tc358743: Add support for platforms without IRQ line
>>
>> All looks good, I'll take this for 4.12.
> 
> Thanks.
> 
>> Regards,
>>
>>         Hans
>>
>>>
>>>  drivers/media/i2c/tc358743.c | 59 +++++++++++++++++++++++++++++++++++++++++++-
>>>  1 file changed, 58 insertions(+), 1 deletion(-)
>>>
>>

Re: [PATCH 0/3] tc358743: minor driver fixes

[Philipp Zabel]

On Fri, 2017-06-02 at 15:27 +0200, Hans Verkuil wrote:
> On 06/02/17 15:03, Dave Stevenson wrote:
> > Hi Hans.
> > 
> > On 2 June 2017 at 13:35, Hans Verkuil <hverkuil@xs4all.nl> wrote:
> >> On 06/02/17 14:18, Dave Stevenson wrote:
> >>> These 3 patches for TC358743 came out of trying to use the
> >>> existing driver with a new Raspberry Pi CSI-2 receiver driver.
> >>
> >> Nice! Doing that has been on my todo list for ages but I never got
> >> around to it. I have one of these and using the Raspberry Pi with
> >> the tc358743 would allow me to add a CEC driver as well.
> > 
> > It's been on my list for a while too! It's working, but just the final
> > clean ups needed.
> > I've got 1 v4l2-compliance failure still outstanding that needs
> > digging into (subscribe_event), rebasing on top of the fwnode tree,
> > and a couple of config things to tidy up. RFC hopefully next week.
> > I'm testing with a demo board designed here at Pi Towers, but there
> > are others successfully testing it using the auvidea.com B101 board.
> > 
> > Are you aware of the HDMI modes that the TC358743 driver has been used with?
> > The comments mention 720P60 at 594MHz, but I have had to modify the
> > fifo_level value from 16 to 110 to get VGA60 or 576P50 to work. (The
> > value came out of Toshiba's spreadsheet for computing register
> > settings). It increases the delay by 2.96usecs at 720P60 on 2 lanes,
> > so not a huge change.
> > Is it worth going to the effort of dynamically computing the delay, or
> > is increasing the default acceptable?
> 
> I see that the fifo_level value of 16 was supplied by Philipp Zabel, so
> I have CC-ed him as I am not sure where those values came from.

I've just chosen a small delay that worked reliably. For 4-lane 1080p60
and for 2-lane 720p60 at 594 Mbps lane speed, the Toshiba spreadsheet
believes that it is ok to decrease the FIFO delay all the way down to 0
(it is not). I think it should be fine to delay transmission for a few
microseconds unconditionally, I'll test this next week.

> This driver is also used in a Cisco product, but we use platform_data for that.
> Here are our settings that we use for reference:
>
>         static struct tc358743_platform_data tc358743_pdata = {
>                 .refclk_hz = 27000000,
>                 .ddc5v_delay = DDC5V_DELAY_100_MS,
>                 .fifo_level = 300,
>                 .pll_prd = 4,
>                 .pll_fbd = 122,
>                 /* CSI */
>                 .lineinitcnt = 0x00001770,
>                 .lptxtimecnt = 0x00000005,
>                 .tclk_headercnt = 0x00001d04,
>                 .ths_headercnt = 0x00000505,
>                 .twakeup = 0x00004650,
>                 .ths_trailcnt = 0x00000004,
>                 .hstxvregcnt = 0x00000005,
>                 /* HDMI PHY */
>                 .hdmi_phy_auto_reset_tmds_detected = true,
>                 .hdmi_phy_auto_reset_tmds_in_range = true,
>                 .hdmi_phy_auto_reset_tmds_valid = true,
>                 .hdmi_phy_auto_reset_hsync_out_of_range = true,
>                 .hdmi_phy_auto_reset_vsync_out_of_range = true,
>                 .hdmi_detection_delay = HDMI_MODE_DELAY_25_MS,
>         };
> 
> I believe these are all calculated from the Toshiba spreadsheet.
> 
> Frankly, I have no idea what they mean :-)
> 
> I am fine with increasing the default if Philipp is OK as well. Since
> Cisco uses a value of 300 I would expect that 16 is indeed too low.
> 
> Regards,
> 
> 	Hans
> 
> > 
> >>> A couple of the subdevice API calls were not implemented or
> >>> otherwise gave odd results. Those are fixed.
> >>>
> >>> The TC358743 interface board being used didn't have the IRQ
> >>> line wired up to the SoC. "interrupts" is listed as being
> >>> optional in the DT binding, but the driver didn't actually
> >>> function if it wasn't provided.
> >>>
> >>> Dave Stevenson (3):
> >>>   [media] tc358743: Add enum_mbus_code
> >>>   [media] tc358743: Setup default mbus_fmt before registering
> >>>   [media] tc358743: Add support for platforms without IRQ line
> >>
> >> All looks good, I'll take this for 4.12.
> > 
> > Thanks.
> > 
> >> Regards,
> >>
> >>         Hans
> >>
> >>>
> >>>  drivers/media/i2c/tc358743.c | 59 +++++++++++++++++++++++++++++++++++++++++++-
> >>>  1 file changed, 58 insertions(+), 1 deletion(-)
> >>>
> >>

regards
Philipp

Re: [PATCH 0/3] tc358743: minor driver fixes

[Dave Stevenson]

On 2 June 2017 at 15:13, Philipp Zabel <p.zabel@pengutronix.de> wrote:
> On Fri, 2017-06-02 at 15:27 +0200, Hans Verkuil wrote:
>> On 06/02/17 15:03, Dave Stevenson wrote:
>> > Hi Hans.
>> >
>> > On 2 June 2017 at 13:35, Hans Verkuil <hverkuil@xs4all.nl> wrote:
>> >> On 06/02/17 14:18, Dave Stevenson wrote:
>> >>> These 3 patches for TC358743 came out of trying to use the
>> >>> existing driver with a new Raspberry Pi CSI-2 receiver driver.
>> >>
>> >> Nice! Doing that has been on my todo list for ages but I never got
>> >> around to it. I have one of these and using the Raspberry Pi with
>> >> the tc358743 would allow me to add a CEC driver as well.
>> >
>> > It's been on my list for a while too! It's working, but just the final
>> > clean ups needed.
>> > I've got 1 v4l2-compliance failure still outstanding that needs
>> > digging into (subscribe_event), rebasing on top of the fwnode tree,
>> > and a couple of config things to tidy up. RFC hopefully next week.
>> > I'm testing with a demo board designed here at Pi Towers, but there
>> > are others successfully testing it using the auvidea.com B101 board.
>> >
>> > Are you aware of the HDMI modes that the TC358743 driver has been used with?
>> > The comments mention 720P60 at 594MHz, but I have had to modify the
>> > fifo_level value from 16 to 110 to get VGA60 or 576P50 to work. (The
>> > value came out of Toshiba's spreadsheet for computing register
>> > settings). It increases the delay by 2.96usecs at 720P60 on 2 lanes,
>> > so not a huge change.
>> > Is it worth going to the effort of dynamically computing the delay, or
>> > is increasing the default acceptable?
>>
>> I see that the fifo_level value of 16 was supplied by Philipp Zabel, so
>> I have CC-ed him as I am not sure where those values came from.
>
> I've just chosen a small delay that worked reliably. For 4-lane 1080p60
> and for 2-lane 720p60 at 594 Mbps lane speed, the Toshiba spreadsheet
> believes that it is ok to decrease the FIFO delay all the way down to 0
> (it is not). I think it should be fine to delay transmission for a few
> microseconds unconditionally, I'll test this next week.

Thanks Philipp. Were 1080p60 and 720p60 the only modes you were really testing?

Did the 594Mbps lane speed come from a specific requirement somewhere?
The standard Pi only has 2 CSI2 lanes exposed, and 1080P30 RGB3 just
tips over into needing 3 lanes with the current link frequency. When I
can find a bit more time I was thinking that an alternate link
frequency would allow us to squeeze it in to 2 lanes. Obviously the
timing values need to be checked carefully, but it should all work and
allow support of multiple link frequencies.
(My calcs say that 1080p50 UYVY can fit down 2 lanes, but that
requires more extensive register mods).

>> This driver is also used in a Cisco product, but we use platform_data for that.
>> Here are our settings that we use for reference:
>>
>>         static struct tc358743_platform_data tc358743_pdata = {
>>                 .refclk_hz = 27000000,
>>                 .ddc5v_delay = DDC5V_DELAY_100_MS,
>>                 .fifo_level = 300,
>>                 .pll_prd = 4,
>>                 .pll_fbd = 122,
>>                 /* CSI */
>>                 .lineinitcnt = 0x00001770,
>>                 .lptxtimecnt = 0x00000005,
>>                 .tclk_headercnt = 0x00001d04,
>>                 .ths_headercnt = 0x00000505,
>>                 .twakeup = 0x00004650,
>>                 .ths_trailcnt = 0x00000004,
>>                 .hstxvregcnt = 0x00000005,
>>                 /* HDMI PHY */
>>                 .hdmi_phy_auto_reset_tmds_detected = true,
>>                 .hdmi_phy_auto_reset_tmds_in_range = true,
>>                 .hdmi_phy_auto_reset_tmds_valid = true,
>>                 .hdmi_phy_auto_reset_hsync_out_of_range = true,
>>                 .hdmi_phy_auto_reset_vsync_out_of_range = true,
>>                 .hdmi_detection_delay = HDMI_MODE_DELAY_25_MS,
>>         };
>>
>> I believe these are all calculated from the Toshiba spreadsheet.
>>
>> Frankly, I have no idea what they mean :-)
>>
>> I am fine with increasing the default if Philipp is OK as well. Since
>> Cisco uses a value of 300 I would expect that 16 is indeed too low.
>>
>> Regards,
>>
>>       Hans
>>
>> >
>> >>> A couple of the subdevice API calls were not implemented or
>> >>> otherwise gave odd results. Those are fixed.
>> >>>
>> >>> The TC358743 interface board being used didn't have the IRQ
>> >>> line wired up to the SoC. "interrupts" is listed as being
>> >>> optional in the DT binding, but the driver didn't actually
>> >>> function if it wasn't provided.
>> >>>
>> >>> Dave Stevenson (3):
>> >>>   [media] tc358743: Add enum_mbus_code
>> >>>   [media] tc358743: Setup default mbus_fmt before registering
>> >>>   [media] tc358743: Add support for platforms without IRQ line
>> >>
>> >> All looks good, I'll take this for 4.12.
>> >
>> > Thanks.
>> >
>> >> Regards,
>> >>
>> >>         Hans
>> >>
>> >>>
>> >>>  drivers/media/i2c/tc358743.c | 59 +++++++++++++++++++++++++++++++++++++++++++-
>> >>>  1 file changed, 58 insertions(+), 1 deletion(-)
>> >>>
>> >>
>
> regards
> Philipp
>

Re: [PATCH 0/3] tc358743: minor driver fixes

[Philipp Zabel]

Hi Dave,

On Fri, 2017-06-02 at 15:36 +0100, Dave Stevenson wrote:
[...]
> >> > Are you aware of the HDMI modes that the TC358743 driver has been used with?
> >> > The comments mention 720P60 at 594MHz, but I have had to modify the
> >> > fifo_level value from 16 to 110 to get VGA60 or 576P50 to work. (The
> >> > value came out of Toshiba's spreadsheet for computing register
> >> > settings). It increases the delay by 2.96usecs at 720P60 on 2 lanes,
> >> > so not a huge change.
> >> > Is it worth going to the effort of dynamically computing the delay, or
> >> > is increasing the default acceptable?
> >>
> >> I see that the fifo_level value of 16 was supplied by Philipp Zabel, so
> >> I have CC-ed him as I am not sure where those values came from.
> >
> > I've just chosen a small delay that worked reliably. For 4-lane 1080p60
> > and for 2-lane 720p60 at 594 Mbps lane speed, the Toshiba spreadsheet
> > believes that it is ok to decrease the FIFO delay all the way down to 0
> > (it is not). I think it should be fine to delay transmission for a few
> > microseconds unconditionally, I'll test this next week.
> 
> Thanks Philipp. Were 1080p60 and 720p60 the only modes you were really testing?

Yes. Since 720p60 needs half the bandwidth of 1080p60, it was possible
to just use the 1080p60 timings by switching from 4 to 2 lanes.

> Did the 594Mbps lane speed come from a specific requirement somewhere?

It is what the spreadsheed suggests for 4-lane 1080p60, I assume because
it is nice and even to generate from the 27 MHz reference clock, and it
fits the 547.28 Mbps/lane requirement (for YCbCr 4:2:2 CSI output) with
a bit of headroom.

> The standard Pi only has 2 CSI2 lanes exposed, and 1080P30 RGB3 just
> tips over into needing 3 lanes with the current link frequency.
>  When I can find a bit more time I was thinking that an alternate link
> frequency would allow us to squeeze it in to 2 lanes. Obviously the
> timing values need to be checked carefully, but it should all work and
> allow support of multiple link frequencies.

See the FIXME comment in tc358743_probe_of, you could calculate and add
the correct pdata for the raised link-frequency.

> (My calcs say that 1080p50 UYVY can fit down 2 lanes, but that
> requires more extensive register mods).

With a lane rate of 911.25 Mbps or higher that should be possible, with
all the CSI timings are relaxed a bit.

regards
Philipp