Re: [PATCH 3/5] media: i2c: Add TDA1997x HDMI receiver driver

[Hans Verkuil]

On 14/12/17 00:33, Tim Harvey wrote:
> On Tue, Dec 12, 2017 at 4:18 AM, Hans Verkuil  wrote:
>> Hi Tim,
>>
>> Sorry for the delay, I needed to find some time to think about this.
>>
>> On 11/16/17 05:30, Rob Herring wrote:
>>> On Wed, Nov 15, 2017 at 10:31:14AM -0800, Tim Harvey wrote:
 On Wed, Nov 15, 2017 at 7:52 AM, Rob Herring  wrote:
> On Thu, Nov 09, 2017 at 10:45:34AM -0800, Tim Harvey wrote:
>> Add support for the TDA1997x HDMI receivers.
>>
>> Cc: Hans Verkuil 
>> Signed-off-by: Tim Harvey 
>> ---
>> v3:
>>  - use V4L2_DV_BT_FRAME_WIDTH/HEIGHT macros
>>  - fixed missing break
>>  - use only hdmi_infoframe_log for infoframe logging
>>  - simplify tda1997x_s_stream error handling
>>  - add delayed work proc to handle hotplug enable/disable
>>  - fix set_edid (disable HPD before writing, enable after)
>>  - remove enabling edid by default
>>  - initialize timings
>>  - take quant range into account in colorspace conversion
>>  - remove vendor/product tracking (we provide this in log_status via 
>> infoframes)
>>  - add v4l_controls
>>  - add more detail to log_status
>>  - calculate vhref generator timings
>>  - timing detection fixes (rounding errors, hswidth errors)
>>  - rename configure_input/configure_conv functions
>>
>> v2:
>>  - implement dv timings enum/cap
>>  - remove deprecated g_mbus_config op
>>  - fix dv_query_timings
>>  - add EDID get/set handling
>>  - remove max-pixel-rate support
>>  - add audio codec DAI support
>>  - change audio bindings
>> ---
>>  drivers/media/i2c/Kconfig|9 +
>>  drivers/media/i2c/Makefile   |1 +
>>  drivers/media/i2c/tda1997x.c | 3485 
>> ++
>>  include/dt-bindings/media/tda1997x.h |   78 +
>
> This belongs with the binding documentation patch.
>

 Rob,

 Thanks - missed that. I will move it for v4.

 Regarding your previous comment to the v2 series:
> The rest of the binding looks fine, but I have some reservations about
> this. I think this should be common probably. There's been a few
> bindings for display recently that deal with the interface format. Maybe
> some vendor property is needed here to map a standard interface format
> back to pin configuration.

 I take it this is not an 'Ack' for the bindings?

 Which did you feel should be made common? I admit I was surprised
 there wasn't a common binding for audio bus format (i2s|spdif) but if
 you were referring to the video data that would probably be much more
 complicated.
>>>
>>> The video data. Either you have to try to come up with some way to map
>>> color components to signals/pins (and even cycles) or you just enumerate
>>> the formats and keep adding to them when new ones appear. There's h/w
>>> that allows the former, but in the end you have to interoperate, so
>>> enumerating the formats is probably enough.
>>>
 I was hoping one of the media/driver maintainers would respond to your
 comment with thoughts as I'm not familiar with a very wide variety of
 receivers.
>>>
>>> I am hoping, too.
>>
>> I don't think it is right to store this in the DT. How you map the output 
>> pins
>> is a driver thing. So when you are requested to enumerate the mediabus 
>> formats
>> (include/uapi/linux/media-bus-format.h) you support, you do so based on the
>> capabilities of the hardware, and when a format is requested you program the
>> hardware accordingly.
>>
>> The device tree should describe the physical characteristics like the number
>> of pins that are hooked up (i.e. are there 24, 30 or 36 pins connected).
>>
>> These vidout-portcfg mappings do not appear to describe physical properties
>> but really register settings.
> 
> Hans,
> 
> They are register settings that define which bits on the internal data
> bus are mapped to which pins on the package. Internally these parts
> have a 36bit video data bus but externally the tda19971 only has 24
> pins and even then perhaps only 8 are hooked up if using BT656 and the
> registers also define 'which 8' as it could have been connected to the
> upper or lower part of the bus. So while the bindings from
> video-interfaces.txt provide bus-width and details of the sync
> signals, additional hardware-specific interconnect details such as how
> the video bits are shifted/mixed on the output bus are needed here.
> This is why I feel vidout-portcfg should be a dt property vs something
> like a module param.

The video-interfaces.txt bindings text defines bus-width and data-shift.
The last one would define 'which 8'.

Only if the hardware can do weird stuff like using pins 4-7 and 16-19 would
this be insufficient. But in that case we would need something more detailed
in 

Re: [PATCH 3/5] media: i2c: Add TDA1997x HDMI receiver driver

[Hans Verkuil]

On 14/12/17 00:33, Tim Harvey wrote:
> On Tue, Dec 12, 2017 at 4:18 AM, Hans Verkuil  wrote:
>> Hi Tim,
>>
>> Sorry for the delay, I needed to find some time to think about this.
>>
>> On 11/16/17 05:30, Rob Herring wrote:
>>> On Wed, Nov 15, 2017 at 10:31:14AM -0800, Tim Harvey wrote:
 On Wed, Nov 15, 2017 at 7:52 AM, Rob Herring  wrote:
> On Thu, Nov 09, 2017 at 10:45:34AM -0800, Tim Harvey wrote:
>> Add support for the TDA1997x HDMI receivers.
>>
>> Cc: Hans Verkuil 
>> Signed-off-by: Tim Harvey 
>> ---
>> v3:
>>  - use V4L2_DV_BT_FRAME_WIDTH/HEIGHT macros
>>  - fixed missing break
>>  - use only hdmi_infoframe_log for infoframe logging
>>  - simplify tda1997x_s_stream error handling
>>  - add delayed work proc to handle hotplug enable/disable
>>  - fix set_edid (disable HPD before writing, enable after)
>>  - remove enabling edid by default
>>  - initialize timings
>>  - take quant range into account in colorspace conversion
>>  - remove vendor/product tracking (we provide this in log_status via 
>> infoframes)
>>  - add v4l_controls
>>  - add more detail to log_status
>>  - calculate vhref generator timings
>>  - timing detection fixes (rounding errors, hswidth errors)
>>  - rename configure_input/configure_conv functions
>>
>> v2:
>>  - implement dv timings enum/cap
>>  - remove deprecated g_mbus_config op
>>  - fix dv_query_timings
>>  - add EDID get/set handling
>>  - remove max-pixel-rate support
>>  - add audio codec DAI support
>>  - change audio bindings
>> ---
>>  drivers/media/i2c/Kconfig|9 +
>>  drivers/media/i2c/Makefile   |1 +
>>  drivers/media/i2c/tda1997x.c | 3485 
>> ++
>>  include/dt-bindings/media/tda1997x.h |   78 +
>
> This belongs with the binding documentation patch.
>

 Rob,

 Thanks - missed that. I will move it for v4.

 Regarding your previous comment to the v2 series:
> The rest of the binding looks fine, but I have some reservations about
> this. I think this should be common probably. There's been a few
> bindings for display recently that deal with the interface format. Maybe
> some vendor property is needed here to map a standard interface format
> back to pin configuration.

 I take it this is not an 'Ack' for the bindings?

 Which did you feel should be made common? I admit I was surprised
 there wasn't a common binding for audio bus format (i2s|spdif) but if
 you were referring to the video data that would probably be much more
 complicated.
>>>
>>> The video data. Either you have to try to come up with some way to map
>>> color components to signals/pins (and even cycles) or you just enumerate
>>> the formats and keep adding to them when new ones appear. There's h/w
>>> that allows the former, but in the end you have to interoperate, so
>>> enumerating the formats is probably enough.
>>>
 I was hoping one of the media/driver maintainers would respond to your
 comment with thoughts as I'm not familiar with a very wide variety of
 receivers.
>>>
>>> I am hoping, too.
>>
>> I don't think it is right to store this in the DT. How you map the output 
>> pins
>> is a driver thing. So when you are requested to enumerate the mediabus 
>> formats
>> (include/uapi/linux/media-bus-format.h) you support, you do so based on the
>> capabilities of the hardware, and when a format is requested you program the
>> hardware accordingly.
>>
>> The device tree should describe the physical characteristics like the number
>> of pins that are hooked up (i.e. are there 24, 30 or 36 pins connected).
>>
>> These vidout-portcfg mappings do not appear to describe physical properties
>> but really register settings.
> 
> Hans,
> 
> They are register settings that define which bits on the internal data
> bus are mapped to which pins on the package. Internally these parts
> have a 36bit video data bus but externally the tda19971 only has 24
> pins and even then perhaps only 8 are hooked up if using BT656 and the
> registers also define 'which 8' as it could have been connected to the
> upper or lower part of the bus. So while the bindings from
> video-interfaces.txt provide bus-width and details of the sync
> signals, additional hardware-specific interconnect details such as how
> the video bits are shifted/mixed on the output bus are needed here.
> This is why I feel vidout-portcfg should be a dt property vs something
> like a module param.

The video-interfaces.txt bindings text defines bus-width and data-shift.
The last one would define 'which 8'.

Only if the hardware can do weird stuff like using pins 4-7 and 16-19 would
this be insufficient. But in that case we would need something more detailed
in video-interfaces.txt.

Frankly, unless you've seen anyone do something weird like 

Re: [PATCH 3/5] media: i2c: Add TDA1997x HDMI receiver driver

[Tim Harvey]

On Tue, Dec 12, 2017 at 4:18 AM, Hans Verkuil  wrote:
> Hi Tim,
>
> Sorry for the delay, I needed to find some time to think about this.
>
> On 11/16/17 05:30, Rob Herring wrote:
>> On Wed, Nov 15, 2017 at 10:31:14AM -0800, Tim Harvey wrote:
>>> On Wed, Nov 15, 2017 at 7:52 AM, Rob Herring  wrote:
 On Thu, Nov 09, 2017 at 10:45:34AM -0800, Tim Harvey wrote:
> Add support for the TDA1997x HDMI receivers.
>
> Cc: Hans Verkuil 
> Signed-off-by: Tim Harvey 
> ---
> v3:
>  - use V4L2_DV_BT_FRAME_WIDTH/HEIGHT macros
>  - fixed missing break
>  - use only hdmi_infoframe_log for infoframe logging
>  - simplify tda1997x_s_stream error handling
>  - add delayed work proc to handle hotplug enable/disable
>  - fix set_edid (disable HPD before writing, enable after)
>  - remove enabling edid by default
>  - initialize timings
>  - take quant range into account in colorspace conversion
>  - remove vendor/product tracking (we provide this in log_status via 
> infoframes)
>  - add v4l_controls
>  - add more detail to log_status
>  - calculate vhref generator timings
>  - timing detection fixes (rounding errors, hswidth errors)
>  - rename configure_input/configure_conv functions
>
> v2:
>  - implement dv timings enum/cap
>  - remove deprecated g_mbus_config op
>  - fix dv_query_timings
>  - add EDID get/set handling
>  - remove max-pixel-rate support
>  - add audio codec DAI support
>  - change audio bindings
> ---
>  drivers/media/i2c/Kconfig|9 +
>  drivers/media/i2c/Makefile   |1 +
>  drivers/media/i2c/tda1997x.c | 3485 
> ++
>  include/dt-bindings/media/tda1997x.h |   78 +

 This belongs with the binding documentation patch.

>>>
>>> Rob,
>>>
>>> Thanks - missed that. I will move it for v4.
>>>
>>> Regarding your previous comment to the v2 series:
 The rest of the binding looks fine, but I have some reservations about
 this. I think this should be common probably. There's been a few
 bindings for display recently that deal with the interface format. Maybe
 some vendor property is needed here to map a standard interface format
 back to pin configuration.
>>>
>>> I take it this is not an 'Ack' for the bindings?
>>>
>>> Which did you feel should be made common? I admit I was surprised
>>> there wasn't a common binding for audio bus format (i2s|spdif) but if
>>> you were referring to the video data that would probably be much more
>>> complicated.
>>
>> The video data. Either you have to try to come up with some way to map
>> color components to signals/pins (and even cycles) or you just enumerate
>> the formats and keep adding to them when new ones appear. There's h/w
>> that allows the former, but in the end you have to interoperate, so
>> enumerating the formats is probably enough.
>>
>>> I was hoping one of the media/driver maintainers would respond to your
>>> comment with thoughts as I'm not familiar with a very wide variety of
>>> receivers.
>>
>> I am hoping, too.
>
> I don't think it is right to store this in the DT. How you map the output pins
> is a driver thing. So when you are requested to enumerate the mediabus formats
> (include/uapi/linux/media-bus-format.h) you support, you do so based on the
> capabilities of the hardware, and when a format is requested you program the
> hardware accordingly.
>
> The device tree should describe the physical characteristics like the number
> of pins that are hooked up (i.e. are there 24, 30 or 36 pins connected).
>
> These vidout-portcfg mappings do not appear to describe physical properties
> but really register settings.

Hans,

They are register settings that define which bits on the internal data
bus are mapped to which pins on the package. Internally these parts
have a 36bit video data bus but externally the tda19971 only has 24
pins and even then perhaps only 8 are hooked up if using BT656 and the
registers also define 'which 8' as it could have been connected to the
upper or lower part of the bus. So while the bindings from
video-interfaces.txt provide bus-width and details of the sync
signals, additional hardware-specific interconnect details such as how
the video bits are shifted/mixed on the output bus are needed here.
This is why I feel vidout-portcfg should be a dt property vs something
like a module param.

Regards,

Tim

Re: [PATCH 3/5] media: i2c: Add TDA1997x HDMI receiver driver

[Tim Harvey]

On Tue, Dec 12, 2017 at 4:18 AM, Hans Verkuil  wrote:
> Hi Tim,
>
> Sorry for the delay, I needed to find some time to think about this.
>
> On 11/16/17 05:30, Rob Herring wrote:
>> On Wed, Nov 15, 2017 at 10:31:14AM -0800, Tim Harvey wrote:
>>> On Wed, Nov 15, 2017 at 7:52 AM, Rob Herring  wrote:
 On Thu, Nov 09, 2017 at 10:45:34AM -0800, Tim Harvey wrote:
> Add support for the TDA1997x HDMI receivers.
>
> Cc: Hans Verkuil 
> Signed-off-by: Tim Harvey 
> ---
> v3:
>  - use V4L2_DV_BT_FRAME_WIDTH/HEIGHT macros
>  - fixed missing break
>  - use only hdmi_infoframe_log for infoframe logging
>  - simplify tda1997x_s_stream error handling
>  - add delayed work proc to handle hotplug enable/disable
>  - fix set_edid (disable HPD before writing, enable after)
>  - remove enabling edid by default
>  - initialize timings
>  - take quant range into account in colorspace conversion
>  - remove vendor/product tracking (we provide this in log_status via 
> infoframes)
>  - add v4l_controls
>  - add more detail to log_status
>  - calculate vhref generator timings
>  - timing detection fixes (rounding errors, hswidth errors)
>  - rename configure_input/configure_conv functions
>
> v2:
>  - implement dv timings enum/cap
>  - remove deprecated g_mbus_config op
>  - fix dv_query_timings
>  - add EDID get/set handling
>  - remove max-pixel-rate support
>  - add audio codec DAI support
>  - change audio bindings
> ---
>  drivers/media/i2c/Kconfig|9 +
>  drivers/media/i2c/Makefile   |1 +
>  drivers/media/i2c/tda1997x.c | 3485 
> ++
>  include/dt-bindings/media/tda1997x.h |   78 +

 This belongs with the binding documentation patch.

>>>
>>> Rob,
>>>
>>> Thanks - missed that. I will move it for v4.
>>>
>>> Regarding your previous comment to the v2 series:
 The rest of the binding looks fine, but I have some reservations about
 this. I think this should be common probably. There's been a few
 bindings for display recently that deal with the interface format. Maybe
 some vendor property is needed here to map a standard interface format
 back to pin configuration.
>>>
>>> I take it this is not an 'Ack' for the bindings?
>>>
>>> Which did you feel should be made common? I admit I was surprised
>>> there wasn't a common binding for audio bus format (i2s|spdif) but if
>>> you were referring to the video data that would probably be much more
>>> complicated.
>>
>> The video data. Either you have to try to come up with some way to map
>> color components to signals/pins (and even cycles) or you just enumerate
>> the formats and keep adding to them when new ones appear. There's h/w
>> that allows the former, but in the end you have to interoperate, so
>> enumerating the formats is probably enough.
>>
>>> I was hoping one of the media/driver maintainers would respond to your
>>> comment with thoughts as I'm not familiar with a very wide variety of
>>> receivers.
>>
>> I am hoping, too.
>
> I don't think it is right to store this in the DT. How you map the output pins
> is a driver thing. So when you are requested to enumerate the mediabus formats
> (include/uapi/linux/media-bus-format.h) you support, you do so based on the
> capabilities of the hardware, and when a format is requested you program the
> hardware accordingly.
>
> The device tree should describe the physical characteristics like the number
> of pins that are hooked up (i.e. are there 24, 30 or 36 pins connected).
>
> These vidout-portcfg mappings do not appear to describe physical properties
> but really register settings.

Hans,

They are register settings that define which bits on the internal data
bus are mapped to which pins on the package. Internally these parts
have a 36bit video data bus but externally the tda19971 only has 24
pins and even then perhaps only 8 are hooked up if using BT656 and the
registers also define 'which 8' as it could have been connected to the
upper or lower part of the bus. So while the bindings from
video-interfaces.txt provide bus-width and details of the sync
signals, additional hardware-specific interconnect details such as how
the video bits are shifted/mixed on the output bus are needed here.
This is why I feel vidout-portcfg should be a dt property vs something
like a module param.

Regards,

Tim

Re: [PATCH 3/5] media: i2c: Add TDA1997x HDMI receiver driver

[Hans Verkuil]

Hi Tim,

Sorry for the delay, I needed to find some time to think about this.

On 11/16/17 05:30, Rob Herring wrote:
> On Wed, Nov 15, 2017 at 10:31:14AM -0800, Tim Harvey wrote:
>> On Wed, Nov 15, 2017 at 7:52 AM, Rob Herring  wrote:
>>> On Thu, Nov 09, 2017 at 10:45:34AM -0800, Tim Harvey wrote:
 Add support for the TDA1997x HDMI receivers.

 Cc: Hans Verkuil 
 Signed-off-by: Tim Harvey 
 ---
 v3:
  - use V4L2_DV_BT_FRAME_WIDTH/HEIGHT macros
  - fixed missing break
  - use only hdmi_infoframe_log for infoframe logging
  - simplify tda1997x_s_stream error handling
  - add delayed work proc to handle hotplug enable/disable
  - fix set_edid (disable HPD before writing, enable after)
  - remove enabling edid by default
  - initialize timings
  - take quant range into account in colorspace conversion
  - remove vendor/product tracking (we provide this in log_status via 
 infoframes)
  - add v4l_controls
  - add more detail to log_status
  - calculate vhref generator timings
  - timing detection fixes (rounding errors, hswidth errors)
  - rename configure_input/configure_conv functions

 v2:
  - implement dv timings enum/cap
  - remove deprecated g_mbus_config op
  - fix dv_query_timings
  - add EDID get/set handling
  - remove max-pixel-rate support
  - add audio codec DAI support
  - change audio bindings
 ---
  drivers/media/i2c/Kconfig|9 +
  drivers/media/i2c/Makefile   |1 +
  drivers/media/i2c/tda1997x.c | 3485 
 ++
  include/dt-bindings/media/tda1997x.h |   78 +
>>>
>>> This belongs with the binding documentation patch.
>>>
>>
>> Rob,
>>
>> Thanks - missed that. I will move it for v4.
>>
>> Regarding your previous comment to the v2 series:
>>> The rest of the binding looks fine, but I have some reservations about
>>> this. I think this should be common probably. There's been a few
>>> bindings for display recently that deal with the interface format. Maybe
>>> some vendor property is needed here to map a standard interface format
>>> back to pin configuration.
>>
>> I take it this is not an 'Ack' for the bindings?
>>
>> Which did you feel should be made common? I admit I was surprised
>> there wasn't a common binding for audio bus format (i2s|spdif) but if
>> you were referring to the video data that would probably be much more
>> complicated.
> 
> The video data. Either you have to try to come up with some way to map 
> color components to signals/pins (and even cycles) or you just enumerate 
> the formats and keep adding to them when new ones appear. There's h/w 
> that allows the former, but in the end you have to interoperate, so 
> enumerating the formats is probably enough.
> 
>> I was hoping one of the media/driver maintainers would respond to your
>> comment with thoughts as I'm not familiar with a very wide variety of
>> receivers.
> 
> I am hoping, too.

I don't think it is right to store this in the DT. How you map the output pins
is a driver thing. So when you are requested to enumerate the mediabus formats
(include/uapi/linux/media-bus-format.h) you support, you do so based on the
capabilities of the hardware, and when a format is requested you program the
hardware accordingly.

The device tree should describe the physical characteristics like the number
of pins that are hooked up (i.e. are there 24, 30 or 36 pins connected).

These vidout-portcfg mappings do not appear to describe physical properties
but really register settings.

Apologies if I misunderstood any of this.

Regards,

Hans

Re: [PATCH 3/5] media: i2c: Add TDA1997x HDMI receiver driver

[Hans Verkuil]

Hi Tim,

Sorry for the delay, I needed to find some time to think about this.

On 11/16/17 05:30, Rob Herring wrote:
> On Wed, Nov 15, 2017 at 10:31:14AM -0800, Tim Harvey wrote:
>> On Wed, Nov 15, 2017 at 7:52 AM, Rob Herring  wrote:
>>> On Thu, Nov 09, 2017 at 10:45:34AM -0800, Tim Harvey wrote:
 Add support for the TDA1997x HDMI receivers.

 Cc: Hans Verkuil 
 Signed-off-by: Tim Harvey 
 ---
 v3:
  - use V4L2_DV_BT_FRAME_WIDTH/HEIGHT macros
  - fixed missing break
  - use only hdmi_infoframe_log for infoframe logging
  - simplify tda1997x_s_stream error handling
  - add delayed work proc to handle hotplug enable/disable
  - fix set_edid (disable HPD before writing, enable after)
  - remove enabling edid by default
  - initialize timings
  - take quant range into account in colorspace conversion
  - remove vendor/product tracking (we provide this in log_status via 
 infoframes)
  - add v4l_controls
  - add more detail to log_status
  - calculate vhref generator timings
  - timing detection fixes (rounding errors, hswidth errors)
  - rename configure_input/configure_conv functions

 v2:
  - implement dv timings enum/cap
  - remove deprecated g_mbus_config op
  - fix dv_query_timings
  - add EDID get/set handling
  - remove max-pixel-rate support
  - add audio codec DAI support
  - change audio bindings
 ---
  drivers/media/i2c/Kconfig|9 +
  drivers/media/i2c/Makefile   |1 +
  drivers/media/i2c/tda1997x.c | 3485 
 ++
  include/dt-bindings/media/tda1997x.h |   78 +
>>>
>>> This belongs with the binding documentation patch.
>>>
>>
>> Rob,
>>
>> Thanks - missed that. I will move it for v4.
>>
>> Regarding your previous comment to the v2 series:
>>> The rest of the binding looks fine, but I have some reservations about
>>> this. I think this should be common probably. There's been a few
>>> bindings for display recently that deal with the interface format. Maybe
>>> some vendor property is needed here to map a standard interface format
>>> back to pin configuration.
>>
>> I take it this is not an 'Ack' for the bindings?
>>
>> Which did you feel should be made common? I admit I was surprised
>> there wasn't a common binding for audio bus format (i2s|spdif) but if
>> you were referring to the video data that would probably be much more
>> complicated.
> 
> The video data. Either you have to try to come up with some way to map 
> color components to signals/pins (and even cycles) or you just enumerate 
> the formats and keep adding to them when new ones appear. There's h/w 
> that allows the former, but in the end you have to interoperate, so 
> enumerating the formats is probably enough.
> 
>> I was hoping one of the media/driver maintainers would respond to your
>> comment with thoughts as I'm not familiar with a very wide variety of
>> receivers.
> 
> I am hoping, too.

I don't think it is right to store this in the DT. How you map the output pins
is a driver thing. So when you are requested to enumerate the mediabus formats
(include/uapi/linux/media-bus-format.h) you support, you do so based on the
capabilities of the hardware, and when a format is requested you program the
hardware accordingly.

The device tree should describe the physical characteristics like the number
of pins that are hooked up (i.e. are there 24, 30 or 36 pins connected).

These vidout-portcfg mappings do not appear to describe physical properties
but really register settings.

Apologies if I misunderstood any of this.

Regards,

Hans

Re: [PATCH 3/5] media: i2c: Add TDA1997x HDMI receiver driver

[Tim Harvey]

On Thu, Nov 23, 2017 at 12:08 AM, Hans Verkuil  wrote:
> On 11/23/2017 05:27 AM, Tim Harvey wrote:
>> On Mon, Nov 20, 2017 at 7:39 AM, Hans Verkuil  wrote:
>>> Hi Tim,
>>>
>>> Some more review comments:
>>>
>>> On 11/09/2017 07:45 PM, Tim Harvey wrote:
 Add support for the TDA1997x HDMI receivers.
>> 
 + */
 +struct color_matrix_coefs {
 + const char *name;
 + /* Input offsets */
 + s16 offint1;
 + s16 offint2;
 + s16 offint3;
 + /* Coeficients */
 + s16 p11coef;
 + s16 p12coef;
 + s16 p13coef;
 + s16 p21coef;
 + s16 p22coef;
 + s16 p23coef;
 + s16 p31coef;
 + s16 p32coef;
 + s16 p33coef;
 + /* Output offsets */
 + s16 offout1;
 + s16 offout2;
 + s16 offout3;
 +};
 +
 +enum {
 + ITU709_RGBLIMITED,
 + ITU709_RGBFULL,
 + ITU601_RGBLIMITED,
 + ITU601_RGBFULL,
 + RGBLIMITED_RGBFULL,
 + RGBLIMITED_ITU601,
 + RGBFULL_ITU601,
>>>
>>> This can't be right.
>>> ITU709_RGBLIMITED
>>> You have these conversions:
>>>
>>> ITU709_RGBFULL
>>> ITU601_RGBFULL
>>> RGBLIMITED_RGBFULL
>>> RGBLIMITED_ITU601
>>> RGBFULL_ITU601
>>> RGBLIMITED_ITU709
>>> RGBFULL_ITU709
>>>
>>> I.e. on the HDMI receiver side you can receive RGB full/limited or 
>>> ITU601/709.
>>> On the output side you have RGB full or ITU601/709.
>>>
>>> So something like ITU709_RGBLIMITED makes no sense.
>>>
>>
>> I misunderstood the V4L2_CID_DV_RX_RGB_RANGE thinking that it allowed
>> you to configure the output range. If output to the SoC is only ever
>> full quant range for RGB then I can drop the
>> ITU709_RGBLIMITED/ITU601_RGBLIMITED conversions.
>
> Output for RGB is always full range. The reason is simply that the V4L2 API
> has no way of selecting the quantization range it wants to receive. I made
> a patch for that a few years back, but there really is no demand for it (yet).
> Userspace expects full range RGB and limited range YUV.
>
>>
>> However, If the output is YUV how do I know if I need to convert to
>> ITU709 or ITU601 and what are my conversion matrices for
>> RGBLIMITED_ITU709/RGBFULL_ITU709?
>
> You can choose yourself whether you convert to YUV 601 or 709. I would
> recommend to use 601 for SDTV resolutions (i.e. width/height <= 720x576)
> and 709 for HDTV.
>
> I made a little program that calculates the values for RGB lim/full to
> YUV 601/709:
>
> -
> #include 
> #include 
>
> #define COEFF(v, r) ((v) * (r) * 16.0)
>
> static const double bt601[3][3] = {
> { COEFF(0.299, 219),   COEFF(0.587, 219),   COEFF(0.114, 219)   },
> { COEFF(-0.1687, 224), COEFF(-0.3313, 224), COEFF(0.5, 224) },
> { COEFF(0.5, 224), COEFF(-0.4187, 224), COEFF(-0.0813, 224) },
> };
> static const double rec709[3][3] = {
> { COEFF(0.2126, 219),  COEFF(0.7152, 219),  COEFF(0.0722, 219)  },
> { COEFF(-0.1146, 224), COEFF(-0.3854, 224), COEFF(0.5, 224) },
> { COEFF(0.5, 224), COEFF(-0.4542, 224), COEFF(-0.0458, 224) },
> };
>
> int main(int argc, char **argv)
> {
> int i, j;
> int mapi[] = { 0, 2, 1 };
> int mapj[] = { 1, 0, 2 };
>
> printf("rgb full -> 601\n");
> printf("0, 0, 0,\n");
> for (i = 0; i < 3; i++) {
> for (j = 0; j < 3; j++) {
> printf("%5d, ",  (int)(0.5 + 
> bt601[mapi[i]][mapj[j]]));
> }
> printf("\n");
> }
> printf("  256,  2048,  2048,\n\n");
>
> printf("rgb lim -> 601\n");
> printf(" -256,  -256,  -256,\n");
> for (i = 0; i < 3; i++) {
> for (j = 0; j < 3; j++) {
> printf("%5d, ",  (int)(0.5 + 255.0 / 219.0 * 
> bt601[mapi[i]][mapj[j]]));
> }
> printf("\n");
> }
> printf("  256,  2048,  2048,\n\n");
>
> printf("rgb full -> 709\n");
> printf("0, 0, 0,\n");
> for (i = 0; i < 3; i++) {
> for (j = 0; j < 3; j++) {
> printf("%5d, ",  (int)(0.5 + 
> rec709[mapi[i]][mapj[j]]));
> }
> printf("\n");
> }
> printf("  256,  2048,  2048,\n\n");
>
> printf("rgb lim -> 709\n");
> printf(" -256,  -256,  -256,\n");
> for (i = 0; i < 3; i++) {
> for (j = 0; j < 3; j++) {
> printf("%5d, ",  (int)(0.5 + 255.0 / 219.0 * 
> rec709[mapi[i]][mapj[j]]));
> }
> printf("\n");
> }
> printf("  256,  2048,  2048,\n\n");
> return 0;
> }
> -
>
> This should give you the needed matrices. It's up to you whether to keep the
> existing matrices for 601 or replace them with these. Probably 

Re: [PATCH 3/5] media: i2c: Add TDA1997x HDMI receiver driver

[Tim Harvey]

On Thu, Nov 23, 2017 at 12:08 AM, Hans Verkuil  wrote:
> On 11/23/2017 05:27 AM, Tim Harvey wrote:
>> On Mon, Nov 20, 2017 at 7:39 AM, Hans Verkuil  wrote:
>>> Hi Tim,
>>>
>>> Some more review comments:
>>>
>>> On 11/09/2017 07:45 PM, Tim Harvey wrote:
 Add support for the TDA1997x HDMI receivers.
>> 
 + */
 +struct color_matrix_coefs {
 + const char *name;
 + /* Input offsets */
 + s16 offint1;
 + s16 offint2;
 + s16 offint3;
 + /* Coeficients */
 + s16 p11coef;
 + s16 p12coef;
 + s16 p13coef;
 + s16 p21coef;
 + s16 p22coef;
 + s16 p23coef;
 + s16 p31coef;
 + s16 p32coef;
 + s16 p33coef;
 + /* Output offsets */
 + s16 offout1;
 + s16 offout2;
 + s16 offout3;
 +};
 +
 +enum {
 + ITU709_RGBLIMITED,
 + ITU709_RGBFULL,
 + ITU601_RGBLIMITED,
 + ITU601_RGBFULL,
 + RGBLIMITED_RGBFULL,
 + RGBLIMITED_ITU601,
 + RGBFULL_ITU601,
>>>
>>> This can't be right.
>>> ITU709_RGBLIMITED
>>> You have these conversions:
>>>
>>> ITU709_RGBFULL
>>> ITU601_RGBFULL
>>> RGBLIMITED_RGBFULL
>>> RGBLIMITED_ITU601
>>> RGBFULL_ITU601
>>> RGBLIMITED_ITU709
>>> RGBFULL_ITU709
>>>
>>> I.e. on the HDMI receiver side you can receive RGB full/limited or 
>>> ITU601/709.
>>> On the output side you have RGB full or ITU601/709.
>>>
>>> So something like ITU709_RGBLIMITED makes no sense.
>>>
>>
>> I misunderstood the V4L2_CID_DV_RX_RGB_RANGE thinking that it allowed
>> you to configure the output range. If output to the SoC is only ever
>> full quant range for RGB then I can drop the
>> ITU709_RGBLIMITED/ITU601_RGBLIMITED conversions.
>
> Output for RGB is always full range. The reason is simply that the V4L2 API
> has no way of selecting the quantization range it wants to receive. I made
> a patch for that a few years back, but there really is no demand for it (yet).
> Userspace expects full range RGB and limited range YUV.
>
>>
>> However, If the output is YUV how do I know if I need to convert to
>> ITU709 or ITU601 and what are my conversion matrices for
>> RGBLIMITED_ITU709/RGBFULL_ITU709?
>
> You can choose yourself whether you convert to YUV 601 or 709. I would
> recommend to use 601 for SDTV resolutions (i.e. width/height <= 720x576)
> and 709 for HDTV.
>
> I made a little program that calculates the values for RGB lim/full to
> YUV 601/709:
>
> -
> #include 
> #include 
>
> #define COEFF(v, r) ((v) * (r) * 16.0)
>
> static const double bt601[3][3] = {
> { COEFF(0.299, 219),   COEFF(0.587, 219),   COEFF(0.114, 219)   },
> { COEFF(-0.1687, 224), COEFF(-0.3313, 224), COEFF(0.5, 224) },
> { COEFF(0.5, 224), COEFF(-0.4187, 224), COEFF(-0.0813, 224) },
> };
> static const double rec709[3][3] = {
> { COEFF(0.2126, 219),  COEFF(0.7152, 219),  COEFF(0.0722, 219)  },
> { COEFF(-0.1146, 224), COEFF(-0.3854, 224), COEFF(0.5, 224) },
> { COEFF(0.5, 224), COEFF(-0.4542, 224), COEFF(-0.0458, 224) },
> };
>
> int main(int argc, char **argv)
> {
> int i, j;
> int mapi[] = { 0, 2, 1 };
> int mapj[] = { 1, 0, 2 };
>
> printf("rgb full -> 601\n");
> printf("0, 0, 0,\n");
> for (i = 0; i < 3; i++) {
> for (j = 0; j < 3; j++) {
> printf("%5d, ",  (int)(0.5 + 
> bt601[mapi[i]][mapj[j]]));
> }
> printf("\n");
> }
> printf("  256,  2048,  2048,\n\n");
>
> printf("rgb lim -> 601\n");
> printf(" -256,  -256,  -256,\n");
> for (i = 0; i < 3; i++) {
> for (j = 0; j < 3; j++) {
> printf("%5d, ",  (int)(0.5 + 255.0 / 219.0 * 
> bt601[mapi[i]][mapj[j]]));
> }
> printf("\n");
> }
> printf("  256,  2048,  2048,\n\n");
>
> printf("rgb full -> 709\n");
> printf("0, 0, 0,\n");
> for (i = 0; i < 3; i++) {
> for (j = 0; j < 3; j++) {
> printf("%5d, ",  (int)(0.5 + 
> rec709[mapi[i]][mapj[j]]));
> }
> printf("\n");
> }
> printf("  256,  2048,  2048,\n\n");
>
> printf("rgb lim -> 709\n");
> printf(" -256,  -256,  -256,\n");
> for (i = 0; i < 3; i++) {
> for (j = 0; j < 3; j++) {
> printf("%5d, ",  (int)(0.5 + 255.0 / 219.0 * 
> rec709[mapi[i]][mapj[j]]));
> }
> printf("\n");
> }
> printf("  256,  2048,  2048,\n\n");
> return 0;
> }
> -
>
> This should give you the needed matrices. It's up to you whether to keep the
> existing matrices for 601 or replace them with these. Probably best to keep
> them.
>

Hans,

Thanks for 

Re: [PATCH 3/5] media: i2c: Add TDA1997x HDMI receiver driver

[Hans Verkuil]

On 11/23/2017 05:27 AM, Tim Harvey wrote:
> On Mon, Nov 20, 2017 at 7:39 AM, Hans Verkuil  wrote:
>> Hi Tim,
>>
>> Some more review comments:
>>
>> On 11/09/2017 07:45 PM, Tim Harvey wrote:
>>> Add support for the TDA1997x HDMI receivers.
> 
>>> + */
>>> +struct color_matrix_coefs {
>>> + const char *name;
>>> + /* Input offsets */
>>> + s16 offint1;
>>> + s16 offint2;
>>> + s16 offint3;
>>> + /* Coeficients */
>>> + s16 p11coef;
>>> + s16 p12coef;
>>> + s16 p13coef;
>>> + s16 p21coef;
>>> + s16 p22coef;
>>> + s16 p23coef;
>>> + s16 p31coef;
>>> + s16 p32coef;
>>> + s16 p33coef;
>>> + /* Output offsets */
>>> + s16 offout1;
>>> + s16 offout2;
>>> + s16 offout3;
>>> +};
>>> +
>>> +enum {
>>> + ITU709_RGBLIMITED,
>>> + ITU709_RGBFULL,
>>> + ITU601_RGBLIMITED,
>>> + ITU601_RGBFULL,
>>> + RGBLIMITED_RGBFULL,
>>> + RGBLIMITED_ITU601,
>>> + RGBFULL_ITU601,
>>
>> This can't be right.
>> ITU709_RGBLIMITED
>> You have these conversions:
>>
>> ITU709_RGBFULL
>> ITU601_RGBFULL
>> RGBLIMITED_RGBFULL
>> RGBLIMITED_ITU601
>> RGBFULL_ITU601
>> RGBLIMITED_ITU709
>> RGBFULL_ITU709
>>
>> I.e. on the HDMI receiver side you can receive RGB full/limited or 
>> ITU601/709.
>> On the output side you have RGB full or ITU601/709.
>>
>> So something like ITU709_RGBLIMITED makes no sense.
>>
> 
> I misunderstood the V4L2_CID_DV_RX_RGB_RANGE thinking that it allowed
> you to configure the output range. If output to the SoC is only ever
> full quant range for RGB then I can drop the
> ITU709_RGBLIMITED/ITU601_RGBLIMITED conversions.

Output for RGB is always full range. The reason is simply that the V4L2 API
has no way of selecting the quantization range it wants to receive. I made
a patch for that a few years back, but there really is no demand for it (yet).
Userspace expects full range RGB and limited range YUV.

> 
> However, If the output is YUV how do I know if I need to convert to
> ITU709 or ITU601 and what are my conversion matrices for
> RGBLIMITED_ITU709/RGBFULL_ITU709?

You can choose yourself whether you convert to YUV 601 or 709. I would
recommend to use 601 for SDTV resolutions (i.e. width/height <= 720x576)
and 709 for HDTV.

I made a little program that calculates the values for RGB lim/full to
YUV 601/709:

-
#include 
#include 

#define COEFF(v, r) ((v) * (r) * 16.0)

static const double bt601[3][3] = {
{ COEFF(0.299, 219),   COEFF(0.587, 219),   COEFF(0.114, 219)   },
{ COEFF(-0.1687, 224), COEFF(-0.3313, 224), COEFF(0.5, 224) },
{ COEFF(0.5, 224), COEFF(-0.4187, 224), COEFF(-0.0813, 224) },
};
static const double rec709[3][3] = {
{ COEFF(0.2126, 219),  COEFF(0.7152, 219),  COEFF(0.0722, 219)  },
{ COEFF(-0.1146, 224), COEFF(-0.3854, 224), COEFF(0.5, 224) },
{ COEFF(0.5, 224), COEFF(-0.4542, 224), COEFF(-0.0458, 224) },
};

int main(int argc, char **argv)
{
int i, j;
int mapi[] = { 0, 2, 1 };
int mapj[] = { 1, 0, 2 };

printf("rgb full -> 601\n");
printf("0, 0, 0,\n");
for (i = 0; i < 3; i++) {
for (j = 0; j < 3; j++) {
printf("%5d, ",  (int)(0.5 + bt601[mapi[i]][mapj[j]]));
}
printf("\n");
}
printf("  256,  2048,  2048,\n\n");

printf("rgb lim -> 601\n");
printf(" -256,  -256,  -256,\n");
for (i = 0; i < 3; i++) {
for (j = 0; j < 3; j++) {
printf("%5d, ",  (int)(0.5 + 255.0 / 219.0 * 
bt601[mapi[i]][mapj[j]]));
}
printf("\n");
}
printf("  256,  2048,  2048,\n\n");

printf("rgb full -> 709\n");
printf("0, 0, 0,\n");
for (i = 0; i < 3; i++) {
for (j = 0; j < 3; j++) {
printf("%5d, ",  (int)(0.5 + rec709[mapi[i]][mapj[j]]));
}
printf("\n");
}
printf("  256,  2048,  2048,\n\n");

printf("rgb lim -> 709\n");
printf(" -256,  -256,  -256,\n");
for (i = 0; i < 3; i++) {
for (j = 0; j < 3; j++) {
printf("%5d, ",  (int)(0.5 + 255.0 / 219.0 * 
rec709[mapi[i]][mapj[j]]));
}
printf("\n");
}
printf("  256,  2048,  2048,\n\n");
return 0;
}
-

This should give you the needed matrices. It's up to you whether to keep the
existing matrices for 601 or replace them with these. Probably best to keep
them.

> 
> Sorry for all the questions, the colorspace/colorimetry options
> confuse the heck out of me.
> 
>>> +};
>>> +
> 
>>> +
>>> +/* parse an infoframe and do some sanity checks on it */
>>> +static unsigned int
>>> +tda1997x_parse_infoframe(struct tda1997x_state *state, u16 addr)
>>> 

Re: [PATCH 3/5] media: i2c: Add TDA1997x HDMI receiver driver

[Hans Verkuil]

On 11/23/2017 05:27 AM, Tim Harvey wrote:
> On Mon, Nov 20, 2017 at 7:39 AM, Hans Verkuil  wrote:
>> Hi Tim,
>>
>> Some more review comments:
>>
>> On 11/09/2017 07:45 PM, Tim Harvey wrote:
>>> Add support for the TDA1997x HDMI receivers.
> 
>>> + */
>>> +struct color_matrix_coefs {
>>> + const char *name;
>>> + /* Input offsets */
>>> + s16 offint1;
>>> + s16 offint2;
>>> + s16 offint3;
>>> + /* Coeficients */
>>> + s16 p11coef;
>>> + s16 p12coef;
>>> + s16 p13coef;
>>> + s16 p21coef;
>>> + s16 p22coef;
>>> + s16 p23coef;
>>> + s16 p31coef;
>>> + s16 p32coef;
>>> + s16 p33coef;
>>> + /* Output offsets */
>>> + s16 offout1;
>>> + s16 offout2;
>>> + s16 offout3;
>>> +};
>>> +
>>> +enum {
>>> + ITU709_RGBLIMITED,
>>> + ITU709_RGBFULL,
>>> + ITU601_RGBLIMITED,
>>> + ITU601_RGBFULL,
>>> + RGBLIMITED_RGBFULL,
>>> + RGBLIMITED_ITU601,
>>> + RGBFULL_ITU601,
>>
>> This can't be right.
>> ITU709_RGBLIMITED
>> You have these conversions:
>>
>> ITU709_RGBFULL
>> ITU601_RGBFULL
>> RGBLIMITED_RGBFULL
>> RGBLIMITED_ITU601
>> RGBFULL_ITU601
>> RGBLIMITED_ITU709
>> RGBFULL_ITU709
>>
>> I.e. on the HDMI receiver side you can receive RGB full/limited or 
>> ITU601/709.
>> On the output side you have RGB full or ITU601/709.
>>
>> So something like ITU709_RGBLIMITED makes no sense.
>>
> 
> I misunderstood the V4L2_CID_DV_RX_RGB_RANGE thinking that it allowed
> you to configure the output range. If output to the SoC is only ever
> full quant range for RGB then I can drop the
> ITU709_RGBLIMITED/ITU601_RGBLIMITED conversions.

Output for RGB is always full range. The reason is simply that the V4L2 API
has no way of selecting the quantization range it wants to receive. I made
a patch for that a few years back, but there really is no demand for it (yet).
Userspace expects full range RGB and limited range YUV.

> 
> However, If the output is YUV how do I know if I need to convert to
> ITU709 or ITU601 and what are my conversion matrices for
> RGBLIMITED_ITU709/RGBFULL_ITU709?

You can choose yourself whether you convert to YUV 601 or 709. I would
recommend to use 601 for SDTV resolutions (i.e. width/height <= 720x576)
and 709 for HDTV.

I made a little program that calculates the values for RGB lim/full to
YUV 601/709:

-
#include 
#include 

#define COEFF(v, r) ((v) * (r) * 16.0)

static const double bt601[3][3] = {
{ COEFF(0.299, 219),   COEFF(0.587, 219),   COEFF(0.114, 219)   },
{ COEFF(-0.1687, 224), COEFF(-0.3313, 224), COEFF(0.5, 224) },
{ COEFF(0.5, 224), COEFF(-0.4187, 224), COEFF(-0.0813, 224) },
};
static const double rec709[3][3] = {
{ COEFF(0.2126, 219),  COEFF(0.7152, 219),  COEFF(0.0722, 219)  },
{ COEFF(-0.1146, 224), COEFF(-0.3854, 224), COEFF(0.5, 224) },
{ COEFF(0.5, 224), COEFF(-0.4542, 224), COEFF(-0.0458, 224) },
};

int main(int argc, char **argv)
{
int i, j;
int mapi[] = { 0, 2, 1 };
int mapj[] = { 1, 0, 2 };

printf("rgb full -> 601\n");
printf("0, 0, 0,\n");
for (i = 0; i < 3; i++) {
for (j = 0; j < 3; j++) {
printf("%5d, ",  (int)(0.5 + bt601[mapi[i]][mapj[j]]));
}
printf("\n");
}
printf("  256,  2048,  2048,\n\n");

printf("rgb lim -> 601\n");
printf(" -256,  -256,  -256,\n");
for (i = 0; i < 3; i++) {
for (j = 0; j < 3; j++) {
printf("%5d, ",  (int)(0.5 + 255.0 / 219.0 * 
bt601[mapi[i]][mapj[j]]));
}
printf("\n");
}
printf("  256,  2048,  2048,\n\n");

printf("rgb full -> 709\n");
printf("0, 0, 0,\n");
for (i = 0; i < 3; i++) {
for (j = 0; j < 3; j++) {
printf("%5d, ",  (int)(0.5 + rec709[mapi[i]][mapj[j]]));
}
printf("\n");
}
printf("  256,  2048,  2048,\n\n");

printf("rgb lim -> 709\n");
printf(" -256,  -256,  -256,\n");
for (i = 0; i < 3; i++) {
for (j = 0; j < 3; j++) {
printf("%5d, ",  (int)(0.5 + 255.0 / 219.0 * 
rec709[mapi[i]][mapj[j]]));
}
printf("\n");
}
printf("  256,  2048,  2048,\n\n");
return 0;
}
-

This should give you the needed matrices. It's up to you whether to keep the
existing matrices for 601 or replace them with these. Probably best to keep
them.

> 
> Sorry for all the questions, the colorspace/colorimetry options
> confuse the heck out of me.
> 
>>> +};
>>> +
> 
>>> +
>>> +/* parse an infoframe and do some sanity checks on it */
>>> +static unsigned int
>>> +tda1997x_parse_infoframe(struct tda1997x_state *state, u16 addr)
>>> +{
>>> + struct 

Re: [PATCH 3/5] media: i2c: Add TDA1997x HDMI receiver driver

[Tim Harvey]

On Wed, Nov 15, 2017 at 8:30 PM, Rob Herring  wrote:
> On Wed, Nov 15, 2017 at 10:31:14AM -0800, Tim Harvey wrote:
>> On Wed, Nov 15, 2017 at 7:52 AM, Rob Herring  wrote:
>> > On Thu, Nov 09, 2017 at 10:45:34AM -0800, Tim Harvey wrote:
>> >> Add support for the TDA1997x HDMI receivers.
>> >>
>> >> Cc: Hans Verkuil 
>> >> Signed-off-by: Tim Harvey 
>> >> ---
>> >> v3:
>> >>  - use V4L2_DV_BT_FRAME_WIDTH/HEIGHT macros
>> >>  - fixed missing break
>> >>  - use only hdmi_infoframe_log for infoframe logging
>> >>  - simplify tda1997x_s_stream error handling
>> >>  - add delayed work proc to handle hotplug enable/disable
>> >>  - fix set_edid (disable HPD before writing, enable after)
>> >>  - remove enabling edid by default
>> >>  - initialize timings
>> >>  - take quant range into account in colorspace conversion
>> >>  - remove vendor/product tracking (we provide this in log_status via 
>> >> infoframes)
>> >>  - add v4l_controls
>> >>  - add more detail to log_status
>> >>  - calculate vhref generator timings
>> >>  - timing detection fixes (rounding errors, hswidth errors)
>> >>  - rename configure_input/configure_conv functions
>> >>
>> >> v2:
>> >>  - implement dv timings enum/cap
>> >>  - remove deprecated g_mbus_config op
>> >>  - fix dv_query_timings
>> >>  - add EDID get/set handling
>> >>  - remove max-pixel-rate support
>> >>  - add audio codec DAI support
>> >>  - change audio bindings
>> >> ---
>> >>  drivers/media/i2c/Kconfig|9 +
>> >>  drivers/media/i2c/Makefile   |1 +
>> >>  drivers/media/i2c/tda1997x.c | 3485 
>> >> ++
>> >>  include/dt-bindings/media/tda1997x.h |   78 +
>> >
>> > This belongs with the binding documentation patch.
>> >
>>
>> Rob,
>>
>> Thanks - missed that. I will move it for v4.
>>
>> Regarding your previous comment to the v2 series:
>> > The rest of the binding looks fine, but I have some reservations about
>> > this. I think this should be common probably. There's been a few
>> > bindings for display recently that deal with the interface format. Maybe
>> > some vendor property is needed here to map a standard interface format
>> > back to pin configuration.
>>
>> I take it this is not an 'Ack' for the bindings?
>>
>> Which did you feel should be made common? I admit I was surprised
>> there wasn't a common binding for audio bus format (i2s|spdif) but if
>> you were referring to the video data that would probably be much more
>> complicated.
>
> The video data. Either you have to try to come up with some way to map
> color components to signals/pins (and even cycles) or you just enumerate
> the formats and keep adding to them when new ones appear. There's h/w
> that allows the former, but in the end you have to interoperate, so
> enumerating the formats is probably enough.
>
>> I was hoping one of the media/driver maintainers would respond to your
>> comment with thoughts as I'm not familiar with a very wide variety of
>> receivers.
>
> I am hoping, too.
>
> Rob

Hans,

Do you have any comment here regarding Rob's hope that there could be
some generic properties created for video port bindings? Anyone else
you know of who should chime in here?

The TDA1997x allows mapping its internal video output bus to its
physical pin in a fairly flexible way. I don't know how unique this is
to other chips.

Regards,

Tim

Re: [PATCH 3/5] media: i2c: Add TDA1997x HDMI receiver driver

[Tim Harvey]

On Wed, Nov 15, 2017 at 8:30 PM, Rob Herring  wrote:
> On Wed, Nov 15, 2017 at 10:31:14AM -0800, Tim Harvey wrote:
>> On Wed, Nov 15, 2017 at 7:52 AM, Rob Herring  wrote:
>> > On Thu, Nov 09, 2017 at 10:45:34AM -0800, Tim Harvey wrote:
>> >> Add support for the TDA1997x HDMI receivers.
>> >>
>> >> Cc: Hans Verkuil 
>> >> Signed-off-by: Tim Harvey 
>> >> ---
>> >> v3:
>> >>  - use V4L2_DV_BT_FRAME_WIDTH/HEIGHT macros
>> >>  - fixed missing break
>> >>  - use only hdmi_infoframe_log for infoframe logging
>> >>  - simplify tda1997x_s_stream error handling
>> >>  - add delayed work proc to handle hotplug enable/disable
>> >>  - fix set_edid (disable HPD before writing, enable after)
>> >>  - remove enabling edid by default
>> >>  - initialize timings
>> >>  - take quant range into account in colorspace conversion
>> >>  - remove vendor/product tracking (we provide this in log_status via 
>> >> infoframes)
>> >>  - add v4l_controls
>> >>  - add more detail to log_status
>> >>  - calculate vhref generator timings
>> >>  - timing detection fixes (rounding errors, hswidth errors)
>> >>  - rename configure_input/configure_conv functions
>> >>
>> >> v2:
>> >>  - implement dv timings enum/cap
>> >>  - remove deprecated g_mbus_config op
>> >>  - fix dv_query_timings
>> >>  - add EDID get/set handling
>> >>  - remove max-pixel-rate support
>> >>  - add audio codec DAI support
>> >>  - change audio bindings
>> >> ---
>> >>  drivers/media/i2c/Kconfig|9 +
>> >>  drivers/media/i2c/Makefile   |1 +
>> >>  drivers/media/i2c/tda1997x.c | 3485 
>> >> ++
>> >>  include/dt-bindings/media/tda1997x.h |   78 +
>> >
>> > This belongs with the binding documentation patch.
>> >
>>
>> Rob,
>>
>> Thanks - missed that. I will move it for v4.
>>
>> Regarding your previous comment to the v2 series:
>> > The rest of the binding looks fine, but I have some reservations about
>> > this. I think this should be common probably. There's been a few
>> > bindings for display recently that deal with the interface format. Maybe
>> > some vendor property is needed here to map a standard interface format
>> > back to pin configuration.
>>
>> I take it this is not an 'Ack' for the bindings?
>>
>> Which did you feel should be made common? I admit I was surprised
>> there wasn't a common binding for audio bus format (i2s|spdif) but if
>> you were referring to the video data that would probably be much more
>> complicated.
>
> The video data. Either you have to try to come up with some way to map
> color components to signals/pins (and even cycles) or you just enumerate
> the formats and keep adding to them when new ones appear. There's h/w
> that allows the former, but in the end you have to interoperate, so
> enumerating the formats is probably enough.
>
>> I was hoping one of the media/driver maintainers would respond to your
>> comment with thoughts as I'm not familiar with a very wide variety of
>> receivers.
>
> I am hoping, too.
>
> Rob

Hans,

Do you have any comment here regarding Rob's hope that there could be
some generic properties created for video port bindings? Anyone else
you know of who should chime in here?

The TDA1997x allows mapping its internal video output bus to its
physical pin in a fairly flexible way. I don't know how unique this is
to other chips.

Regards,

Tim

Re: [PATCH 3/5] media: i2c: Add TDA1997x HDMI receiver driver

[Tim Harvey]

On Mon, Nov 20, 2017 at 7:39 AM, Hans Verkuil  wrote:
> Hi Tim,
>
> Some more review comments:
>
> On 11/09/2017 07:45 PM, Tim Harvey wrote:
>> Add support for the TDA1997x HDMI receivers.

>> + */
>> +struct color_matrix_coefs {
>> + const char *name;
>> + /* Input offsets */
>> + s16 offint1;
>> + s16 offint2;
>> + s16 offint3;
>> + /* Coeficients */
>> + s16 p11coef;
>> + s16 p12coef;
>> + s16 p13coef;
>> + s16 p21coef;
>> + s16 p22coef;
>> + s16 p23coef;
>> + s16 p31coef;
>> + s16 p32coef;
>> + s16 p33coef;
>> + /* Output offsets */
>> + s16 offout1;
>> + s16 offout2;
>> + s16 offout3;
>> +};
>> +
>> +enum {
>> + ITU709_RGBLIMITED,
>> + ITU709_RGBFULL,
>> + ITU601_RGBLIMITED,
>> + ITU601_RGBFULL,
>> + RGBLIMITED_RGBFULL,
>> + RGBLIMITED_ITU601,
>> + RGBFULL_ITU601,
>
> This can't be right.
>ITU709_RGBLIMITED
> You have these conversions:
>
> ITU709_RGBFULL
> ITU601_RGBFULL
> RGBLIMITED_RGBFULL
> RGBLIMITED_ITU601
> RGBFULL_ITU601
> RGBLIMITED_ITU709
> RGBFULL_ITU709
>
> I.e. on the HDMI receiver side you can receive RGB full/limited or ITU601/709.
> On the output side you have RGB full or ITU601/709.
>
> So something like ITU709_RGBLIMITED makes no sense.
>

I misunderstood the V4L2_CID_DV_RX_RGB_RANGE thinking that it allowed
you to configure the output range. If output to the SoC is only ever
full quant range for RGB then I can drop the
ITU709_RGBLIMITED/ITU601_RGBLIMITED conversions.

However, If the output is YUV how do I know if I need to convert to
ITU709 or ITU601 and what are my conversion matrices for
RGBLIMITED_ITU709/RGBFULL_ITU709?

Sorry for all the questions, the colorspace/colorimetry options
confuse the heck out of me.

>> +};
>> +

>> +
>> +/* parse an infoframe and do some sanity checks on it */
>> +static unsigned int
>> +tda1997x_parse_infoframe(struct tda1997x_state *state, u16 addr)
>> +{
>> + struct v4l2_subdev *sd = >sd;
>> + union hdmi_infoframe frame;
>> + u8 buffer[40];
>> + u8 reg;
>> + int len, err;
>> +
>> + /* read data */
>> + len = io_readn(sd, addr, sizeof(buffer), buffer);
>> + err = hdmi_infoframe_unpack(, buffer);
>> + if (err) {
>> + v4l_err(state->client,
>> + "failed parsing %d byte infoframe: 0x%04x/0x%02x\n",
>> + len, addr, buffer[0]);
>> + return err;
>> + }
>> + hdmi_infoframe_log(KERN_INFO, >client->dev, );
>> + switch (frame.any.type) {
>> + /* Audio InfoFrame: see HDMI spec 8.2.2 */
>> + case HDMI_INFOFRAME_TYPE_AUDIO:
>> + /* sample rate */
>> + switch (frame.audio.sample_frequency) {
>> + case HDMI_AUDIO_SAMPLE_FREQUENCY_32000:
>> + state->audio_samplerate = 32000;
>> + break;
>> + case HDMI_AUDIO_SAMPLE_FREQUENCY_44100:
>> + state->audio_samplerate = 44100;
>> + break;
>> + case HDMI_AUDIO_SAMPLE_FREQUENCY_48000:
>> + state->audio_samplerate = 48000;
>> + break;
>> + case HDMI_AUDIO_SAMPLE_FREQUENCY_88200:
>> + state->audio_samplerate = 88200;
>> + break;
>> + case HDMI_AUDIO_SAMPLE_FREQUENCY_96000:
>> + state->audio_samplerate = 96000;
>> + break;
>> + case HDMI_AUDIO_SAMPLE_FREQUENCY_176400:
>> + state->audio_samplerate = 176400;
>> + break;
>> + case HDMI_AUDIO_SAMPLE_FREQUENCY_192000:
>> + state->audio_samplerate = 192000;
>> + break;
>> + default:
>> + case HDMI_AUDIO_SAMPLE_FREQUENCY_STREAM:
>> + break;
>> + }
>> +
>> + /* sample size */
>> + switch (frame.audio.sample_size) {
>> + case HDMI_AUDIO_SAMPLE_SIZE_16:
>> + state->audio_samplesize = 16;
>> + break;
>> + case HDMI_AUDIO_SAMPLE_SIZE_20:
>> + state->audio_samplesize = 20;
>> + break;
>> + case HDMI_AUDIO_SAMPLE_SIZE_24:
>> + state->audio_samplesize = 24;
>> + break;
>> + case HDMI_AUDIO_SAMPLE_SIZE_STREAM:
>> + default:
>> + break;
>> + }
>> +
>> + /* Channel Count */
>> + state->audio_channels = frame.audio.channels;
>> + if (frame.audio.channel_allocation &&
>> + frame.audio.channel_allocation != state->audio_ch_alloc) {
>> + /* use the channel assignment from the infoframe */
>> + state->audio_ch_alloc = frame.audio.channel_allocation;
>> + 

Re: [PATCH 3/5] media: i2c: Add TDA1997x HDMI receiver driver

[Tim Harvey]

On Mon, Nov 20, 2017 at 7:39 AM, Hans Verkuil  wrote:
> Hi Tim,
>
> Some more review comments:
>
> On 11/09/2017 07:45 PM, Tim Harvey wrote:
>> Add support for the TDA1997x HDMI receivers.

>> + */
>> +struct color_matrix_coefs {
>> + const char *name;
>> + /* Input offsets */
>> + s16 offint1;
>> + s16 offint2;
>> + s16 offint3;
>> + /* Coeficients */
>> + s16 p11coef;
>> + s16 p12coef;
>> + s16 p13coef;
>> + s16 p21coef;
>> + s16 p22coef;
>> + s16 p23coef;
>> + s16 p31coef;
>> + s16 p32coef;
>> + s16 p33coef;
>> + /* Output offsets */
>> + s16 offout1;
>> + s16 offout2;
>> + s16 offout3;
>> +};
>> +
>> +enum {
>> + ITU709_RGBLIMITED,
>> + ITU709_RGBFULL,
>> + ITU601_RGBLIMITED,
>> + ITU601_RGBFULL,
>> + RGBLIMITED_RGBFULL,
>> + RGBLIMITED_ITU601,
>> + RGBFULL_ITU601,
>
> This can't be right.
>ITU709_RGBLIMITED
> You have these conversions:
>
> ITU709_RGBFULL
> ITU601_RGBFULL
> RGBLIMITED_RGBFULL
> RGBLIMITED_ITU601
> RGBFULL_ITU601
> RGBLIMITED_ITU709
> RGBFULL_ITU709
>
> I.e. on the HDMI receiver side you can receive RGB full/limited or ITU601/709.
> On the output side you have RGB full or ITU601/709.
>
> So something like ITU709_RGBLIMITED makes no sense.
>

I misunderstood the V4L2_CID_DV_RX_RGB_RANGE thinking that it allowed
you to configure the output range. If output to the SoC is only ever
full quant range for RGB then I can drop the
ITU709_RGBLIMITED/ITU601_RGBLIMITED conversions.

However, If the output is YUV how do I know if I need to convert to
ITU709 or ITU601 and what are my conversion matrices for
RGBLIMITED_ITU709/RGBFULL_ITU709?

Sorry for all the questions, the colorspace/colorimetry options
confuse the heck out of me.

>> +};
>> +

>> +
>> +/* parse an infoframe and do some sanity checks on it */
>> +static unsigned int
>> +tda1997x_parse_infoframe(struct tda1997x_state *state, u16 addr)
>> +{
>> + struct v4l2_subdev *sd = >sd;
>> + union hdmi_infoframe frame;
>> + u8 buffer[40];
>> + u8 reg;
>> + int len, err;
>> +
>> + /* read data */
>> + len = io_readn(sd, addr, sizeof(buffer), buffer);
>> + err = hdmi_infoframe_unpack(, buffer);
>> + if (err) {
>> + v4l_err(state->client,
>> + "failed parsing %d byte infoframe: 0x%04x/0x%02x\n",
>> + len, addr, buffer[0]);
>> + return err;
>> + }
>> + hdmi_infoframe_log(KERN_INFO, >client->dev, );
>> + switch (frame.any.type) {
>> + /* Audio InfoFrame: see HDMI spec 8.2.2 */
>> + case HDMI_INFOFRAME_TYPE_AUDIO:
>> + /* sample rate */
>> + switch (frame.audio.sample_frequency) {
>> + case HDMI_AUDIO_SAMPLE_FREQUENCY_32000:
>> + state->audio_samplerate = 32000;
>> + break;
>> + case HDMI_AUDIO_SAMPLE_FREQUENCY_44100:
>> + state->audio_samplerate = 44100;
>> + break;
>> + case HDMI_AUDIO_SAMPLE_FREQUENCY_48000:
>> + state->audio_samplerate = 48000;
>> + break;
>> + case HDMI_AUDIO_SAMPLE_FREQUENCY_88200:
>> + state->audio_samplerate = 88200;
>> + break;
>> + case HDMI_AUDIO_SAMPLE_FREQUENCY_96000:
>> + state->audio_samplerate = 96000;
>> + break;
>> + case HDMI_AUDIO_SAMPLE_FREQUENCY_176400:
>> + state->audio_samplerate = 176400;
>> + break;
>> + case HDMI_AUDIO_SAMPLE_FREQUENCY_192000:
>> + state->audio_samplerate = 192000;
>> + break;
>> + default:
>> + case HDMI_AUDIO_SAMPLE_FREQUENCY_STREAM:
>> + break;
>> + }
>> +
>> + /* sample size */
>> + switch (frame.audio.sample_size) {
>> + case HDMI_AUDIO_SAMPLE_SIZE_16:
>> + state->audio_samplesize = 16;
>> + break;
>> + case HDMI_AUDIO_SAMPLE_SIZE_20:
>> + state->audio_samplesize = 20;
>> + break;
>> + case HDMI_AUDIO_SAMPLE_SIZE_24:
>> + state->audio_samplesize = 24;
>> + break;
>> + case HDMI_AUDIO_SAMPLE_SIZE_STREAM:
>> + default:
>> + break;
>> + }
>> +
>> + /* Channel Count */
>> + state->audio_channels = frame.audio.channels;
>> + if (frame.audio.channel_allocation &&
>> + frame.audio.channel_allocation != state->audio_ch_alloc) {
>> + /* use the channel assignment from the infoframe */
>> + state->audio_ch_alloc = frame.audio.channel_allocation;
>> + tda1997x_configure_audout(sd, 

Re: [PATCH 3/5] media: i2c: Add TDA1997x HDMI receiver driver

[Hans Verkuil]

Hi Tim,

Some more review comments:

On 11/09/2017 07:45 PM, Tim Harvey wrote:
> Add support for the TDA1997x HDMI receivers.
> 
> Cc: Hans Verkuil 
> Signed-off-by: Tim Harvey 
> ---
> v3:
>  - use V4L2_DV_BT_FRAME_WIDTH/HEIGHT macros
>  - fixed missing break
>  - use only hdmi_infoframe_log for infoframe logging
>  - simplify tda1997x_s_stream error handling
>  - add delayed work proc to handle hotplug enable/disable
>  - fix set_edid (disable HPD before writing, enable after)
>  - remove enabling edid by default
>  - initialize timings
>  - take quant range into account in colorspace conversion
>  - remove vendor/product tracking (we provide this in log_status via 
> infoframes)
>  - add v4l_controls
>  - add more detail to log_status
>  - calculate vhref generator timings
>  - timing detection fixes (rounding errors, hswidth errors)
>  - rename configure_input/configure_conv functions
> 
> v2:
>  - implement dv timings enum/cap
>  - remove deprecated g_mbus_config op
>  - fix dv_query_timings
>  - add EDID get/set handling
>  - remove max-pixel-rate support
>  - add audio codec DAI support
>  - change audio bindings
> ---
>  drivers/media/i2c/Kconfig|9 +
>  drivers/media/i2c/Makefile   |1 +
>  drivers/media/i2c/tda1997x.c | 3485 
> ++
>  include/dt-bindings/media/tda1997x.h |   78 +
>  include/media/i2c/tda1997x.h |   53 +
>  5 files changed, 3626 insertions(+)
>  create mode 100644 drivers/media/i2c/tda1997x.c
>  create mode 100644 include/dt-bindings/media/tda1997x.h
>  create mode 100644 include/media/i2c/tda1997x.h
> 
> diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
> index 9415389..c2b0400 100644
> --- a/drivers/media/i2c/Kconfig
> +++ b/drivers/media/i2c/Kconfig
> @@ -56,6 +56,15 @@ config VIDEO_TDA9840
> To compile this driver as a module, choose M here: the
> module will be called tda9840.
>  
> +config VIDEO_TDA1997X
> + tristate "NXP TDA1997x HDMI receiver"
> + depends on VIDEO_V4L2 && I2C && VIDEO_V4L2_SUBDEV_API
> + ---help---
> +   V4L2 subdevice driver for the NXP TDA1997x HDMI receivers.
> +
> +   To compile this driver as a module, choose M here: the
> +   module will be called tda1997x.
> +
>  config VIDEO_TEA6415C
>   tristate "Philips TEA6415C audio processor"
>   depends on I2C
> diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
> index c843c18..58f2b2e 100644
> --- a/drivers/media/i2c/Makefile
> +++ b/drivers/media/i2c/Makefile
> @@ -12,6 +12,7 @@ obj-$(CONFIG_VIDEO_TVAUDIO) += tvaudio.o
>  obj-$(CONFIG_VIDEO_TDA7432) += tda7432.o
>  obj-$(CONFIG_VIDEO_SAA6588) += saa6588.o
>  obj-$(CONFIG_VIDEO_TDA9840) += tda9840.o
> +obj-$(CONFIG_VIDEO_TDA1997X) += tda1997x.o
>  obj-$(CONFIG_VIDEO_TEA6415C) += tea6415c.o
>  obj-$(CONFIG_VIDEO_TEA6420) += tea6420.o
>  obj-$(CONFIG_VIDEO_SAA7110) += saa7110.o
> diff --git a/drivers/media/i2c/tda1997x.c b/drivers/media/i2c/tda1997x.c
> new file mode 100644
> index 000..662c3a7
> --- /dev/null
> +++ b/drivers/media/i2c/tda1997x.c
> @@ -0,0 +1,3485 @@
> +/*
> + * Copyright (C) 2017 Gateworks Corporation
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + */
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#include 
> +
> +/* debug level */
> +static int debug;
> +module_param(debug, int, 0644);
> +MODULE_PARM_DESC(debug, "debug level (0-2)");
> +
> +/* Page 0x00 - General Control */
> +#define REG_VERSION  0x
> +#define REG_INPUT_SEL0x0001
> +#define REG_SVC_MODE 0x0002
> +#define REG_HPD_MAN_CTRL 0x0003
> +#define REG_RT_MAN_CTRL  0x0004
> +#define REG_STANDBY_SOFT_RST 0x000A
> +#define REG_HDMI_SOFT_RST0x000B
> +#define REG_HDMI_INFO_RST0x000C
> +#define REG_INT_FLG_CLR_TOP  0x000E
> +#define REG_INT_FLG_CLR_SUS  0x000F
> +#define REG_INT_FLG_CLR_DDC  0x0010
> +#define REG_INT_FLG_CLR_RATE 0x0011
> +#define REG_INT_FLG_CLR_MODE 0x0012
> +#define REG_INT_FLG_CLR_INFO 0x0013
> +#define REG_INT_FLG_CLR_AUDIO0x0014
> +#define REG_INT_FLG_CLR_HDCP 0x0015
> +#define REG_INT_FLG_CLR_AFE  0x0016
> +#define REG_INT_MASK_TOP 0x0017
> +#define REG_INT_MASK_SUS 0x0018
> +#define REG_INT_MASK_DDC 0x0019
> +#define REG_INT_MASK_RATE0x001A
> +#define REG_INT_MASK_MODE0x001B
> +#define REG_INT_MASK_INFO0x001C
> +#define REG_INT_MASK_AUDIO   0x001D
> +#define REG_INT_MASK_HDCP0x001E
> 

Re: [PATCH 3/5] media: i2c: Add TDA1997x HDMI receiver driver

[Hans Verkuil]

Hi Tim,

Some more review comments:

On 11/09/2017 07:45 PM, Tim Harvey wrote:
> Add support for the TDA1997x HDMI receivers.
> 
> Cc: Hans Verkuil 
> Signed-off-by: Tim Harvey 
> ---
> v3:
>  - use V4L2_DV_BT_FRAME_WIDTH/HEIGHT macros
>  - fixed missing break
>  - use only hdmi_infoframe_log for infoframe logging
>  - simplify tda1997x_s_stream error handling
>  - add delayed work proc to handle hotplug enable/disable
>  - fix set_edid (disable HPD before writing, enable after)
>  - remove enabling edid by default
>  - initialize timings
>  - take quant range into account in colorspace conversion
>  - remove vendor/product tracking (we provide this in log_status via 
> infoframes)
>  - add v4l_controls
>  - add more detail to log_status
>  - calculate vhref generator timings
>  - timing detection fixes (rounding errors, hswidth errors)
>  - rename configure_input/configure_conv functions
> 
> v2:
>  - implement dv timings enum/cap
>  - remove deprecated g_mbus_config op
>  - fix dv_query_timings
>  - add EDID get/set handling
>  - remove max-pixel-rate support
>  - add audio codec DAI support
>  - change audio bindings
> ---
>  drivers/media/i2c/Kconfig|9 +
>  drivers/media/i2c/Makefile   |1 +
>  drivers/media/i2c/tda1997x.c | 3485 
> ++
>  include/dt-bindings/media/tda1997x.h |   78 +
>  include/media/i2c/tda1997x.h |   53 +
>  5 files changed, 3626 insertions(+)
>  create mode 100644 drivers/media/i2c/tda1997x.c
>  create mode 100644 include/dt-bindings/media/tda1997x.h
>  create mode 100644 include/media/i2c/tda1997x.h
> 
> diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
> index 9415389..c2b0400 100644
> --- a/drivers/media/i2c/Kconfig
> +++ b/drivers/media/i2c/Kconfig
> @@ -56,6 +56,15 @@ config VIDEO_TDA9840
> To compile this driver as a module, choose M here: the
> module will be called tda9840.
>  
> +config VIDEO_TDA1997X
> + tristate "NXP TDA1997x HDMI receiver"
> + depends on VIDEO_V4L2 && I2C && VIDEO_V4L2_SUBDEV_API
> + ---help---
> +   V4L2 subdevice driver for the NXP TDA1997x HDMI receivers.
> +
> +   To compile this driver as a module, choose M here: the
> +   module will be called tda1997x.
> +
>  config VIDEO_TEA6415C
>   tristate "Philips TEA6415C audio processor"
>   depends on I2C
> diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
> index c843c18..58f2b2e 100644
> --- a/drivers/media/i2c/Makefile
> +++ b/drivers/media/i2c/Makefile
> @@ -12,6 +12,7 @@ obj-$(CONFIG_VIDEO_TVAUDIO) += tvaudio.o
>  obj-$(CONFIG_VIDEO_TDA7432) += tda7432.o
>  obj-$(CONFIG_VIDEO_SAA6588) += saa6588.o
>  obj-$(CONFIG_VIDEO_TDA9840) += tda9840.o
> +obj-$(CONFIG_VIDEO_TDA1997X) += tda1997x.o
>  obj-$(CONFIG_VIDEO_TEA6415C) += tea6415c.o
>  obj-$(CONFIG_VIDEO_TEA6420) += tea6420.o
>  obj-$(CONFIG_VIDEO_SAA7110) += saa7110.o
> diff --git a/drivers/media/i2c/tda1997x.c b/drivers/media/i2c/tda1997x.c
> new file mode 100644
> index 000..662c3a7
> --- /dev/null
> +++ b/drivers/media/i2c/tda1997x.c
> @@ -0,0 +1,3485 @@
> +/*
> + * Copyright (C) 2017 Gateworks Corporation
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + */
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#include 
> +
> +/* debug level */
> +static int debug;
> +module_param(debug, int, 0644);
> +MODULE_PARM_DESC(debug, "debug level (0-2)");
> +
> +/* Page 0x00 - General Control */
> +#define REG_VERSION  0x
> +#define REG_INPUT_SEL0x0001
> +#define REG_SVC_MODE 0x0002
> +#define REG_HPD_MAN_CTRL 0x0003
> +#define REG_RT_MAN_CTRL  0x0004
> +#define REG_STANDBY_SOFT_RST 0x000A
> +#define REG_HDMI_SOFT_RST0x000B
> +#define REG_HDMI_INFO_RST0x000C
> +#define REG_INT_FLG_CLR_TOP  0x000E
> +#define REG_INT_FLG_CLR_SUS  0x000F
> +#define REG_INT_FLG_CLR_DDC  0x0010
> +#define REG_INT_FLG_CLR_RATE 0x0011
> +#define REG_INT_FLG_CLR_MODE 0x0012
> +#define REG_INT_FLG_CLR_INFO 0x0013
> +#define REG_INT_FLG_CLR_AUDIO0x0014
> +#define REG_INT_FLG_CLR_HDCP 0x0015
> +#define REG_INT_FLG_CLR_AFE  0x0016
> +#define REG_INT_MASK_TOP 0x0017
> +#define REG_INT_MASK_SUS 0x0018
> +#define REG_INT_MASK_DDC 0x0019
> +#define REG_INT_MASK_RATE0x001A
> +#define REG_INT_MASK_MODE0x001B
> +#define REG_INT_MASK_INFO0x001C
> +#define REG_INT_MASK_AUDIO   0x001D
> +#define REG_INT_MASK_HDCP0x001E
> +#define REG_INT_MASK_AFE 0x001F
> 

Re: [PATCH 3/5] media: i2c: Add TDA1997x HDMI receiver driver

[Rob Herring]

On Wed, Nov 15, 2017 at 10:31:14AM -0800, Tim Harvey wrote:
> On Wed, Nov 15, 2017 at 7:52 AM, Rob Herring  wrote:
> > On Thu, Nov 09, 2017 at 10:45:34AM -0800, Tim Harvey wrote:
> >> Add support for the TDA1997x HDMI receivers.
> >>
> >> Cc: Hans Verkuil 
> >> Signed-off-by: Tim Harvey 
> >> ---
> >> v3:
> >>  - use V4L2_DV_BT_FRAME_WIDTH/HEIGHT macros
> >>  - fixed missing break
> >>  - use only hdmi_infoframe_log for infoframe logging
> >>  - simplify tda1997x_s_stream error handling
> >>  - add delayed work proc to handle hotplug enable/disable
> >>  - fix set_edid (disable HPD before writing, enable after)
> >>  - remove enabling edid by default
> >>  - initialize timings
> >>  - take quant range into account in colorspace conversion
> >>  - remove vendor/product tracking (we provide this in log_status via 
> >> infoframes)
> >>  - add v4l_controls
> >>  - add more detail to log_status
> >>  - calculate vhref generator timings
> >>  - timing detection fixes (rounding errors, hswidth errors)
> >>  - rename configure_input/configure_conv functions
> >>
> >> v2:
> >>  - implement dv timings enum/cap
> >>  - remove deprecated g_mbus_config op
> >>  - fix dv_query_timings
> >>  - add EDID get/set handling
> >>  - remove max-pixel-rate support
> >>  - add audio codec DAI support
> >>  - change audio bindings
> >> ---
> >>  drivers/media/i2c/Kconfig|9 +
> >>  drivers/media/i2c/Makefile   |1 +
> >>  drivers/media/i2c/tda1997x.c | 3485 
> >> ++
> >>  include/dt-bindings/media/tda1997x.h |   78 +
> >
> > This belongs with the binding documentation patch.
> >
> 
> Rob,
> 
> Thanks - missed that. I will move it for v4.
> 
> Regarding your previous comment to the v2 series:
> > The rest of the binding looks fine, but I have some reservations about
> > this. I think this should be common probably. There's been a few
> > bindings for display recently that deal with the interface format. Maybe
> > some vendor property is needed here to map a standard interface format
> > back to pin configuration.
> 
> I take it this is not an 'Ack' for the bindings?
> 
> Which did you feel should be made common? I admit I was surprised
> there wasn't a common binding for audio bus format (i2s|spdif) but if
> you were referring to the video data that would probably be much more
> complicated.

The video data. Either you have to try to come up with some way to map 
color components to signals/pins (and even cycles) or you just enumerate 
the formats and keep adding to them when new ones appear. There's h/w 
that allows the former, but in the end you have to interoperate, so 
enumerating the formats is probably enough.

> I was hoping one of the media/driver maintainers would respond to your
> comment with thoughts as I'm not familiar with a very wide variety of
> receivers.

I am hoping, too.

Rob

Re: [PATCH 3/5] media: i2c: Add TDA1997x HDMI receiver driver

[Rob Herring]

On Wed, Nov 15, 2017 at 10:31:14AM -0800, Tim Harvey wrote:
> On Wed, Nov 15, 2017 at 7:52 AM, Rob Herring  wrote:
> > On Thu, Nov 09, 2017 at 10:45:34AM -0800, Tim Harvey wrote:
> >> Add support for the TDA1997x HDMI receivers.
> >>
> >> Cc: Hans Verkuil 
> >> Signed-off-by: Tim Harvey 
> >> ---
> >> v3:
> >>  - use V4L2_DV_BT_FRAME_WIDTH/HEIGHT macros
> >>  - fixed missing break
> >>  - use only hdmi_infoframe_log for infoframe logging
> >>  - simplify tda1997x_s_stream error handling
> >>  - add delayed work proc to handle hotplug enable/disable
> >>  - fix set_edid (disable HPD before writing, enable after)
> >>  - remove enabling edid by default
> >>  - initialize timings
> >>  - take quant range into account in colorspace conversion
> >>  - remove vendor/product tracking (we provide this in log_status via 
> >> infoframes)
> >>  - add v4l_controls
> >>  - add more detail to log_status
> >>  - calculate vhref generator timings
> >>  - timing detection fixes (rounding errors, hswidth errors)
> >>  - rename configure_input/configure_conv functions
> >>
> >> v2:
> >>  - implement dv timings enum/cap
> >>  - remove deprecated g_mbus_config op
> >>  - fix dv_query_timings
> >>  - add EDID get/set handling
> >>  - remove max-pixel-rate support
> >>  - add audio codec DAI support
> >>  - change audio bindings
> >> ---
> >>  drivers/media/i2c/Kconfig|9 +
> >>  drivers/media/i2c/Makefile   |1 +
> >>  drivers/media/i2c/tda1997x.c | 3485 
> >> ++
> >>  include/dt-bindings/media/tda1997x.h |   78 +
> >
> > This belongs with the binding documentation patch.
> >
> 
> Rob,
> 
> Thanks - missed that. I will move it for v4.
> 
> Regarding your previous comment to the v2 series:
> > The rest of the binding looks fine, but I have some reservations about
> > this. I think this should be common probably. There's been a few
> > bindings for display recently that deal with the interface format. Maybe
> > some vendor property is needed here to map a standard interface format
> > back to pin configuration.
> 
> I take it this is not an 'Ack' for the bindings?
> 
> Which did you feel should be made common? I admit I was surprised
> there wasn't a common binding for audio bus format (i2s|spdif) but if
> you were referring to the video data that would probably be much more
> complicated.

The video data. Either you have to try to come up with some way to map 
color components to signals/pins (and even cycles) or you just enumerate 
the formats and keep adding to them when new ones appear. There's h/w 
that allows the former, but in the end you have to interoperate, so 
enumerating the formats is probably enough.

> I was hoping one of the media/driver maintainers would respond to your
> comment with thoughts as I'm not familiar with a very wide variety of
> receivers.

I am hoping, too.

Rob

Re: [PATCH 3/5] media: i2c: Add TDA1997x HDMI receiver driver

[Tim Harvey]

On Wed, Nov 15, 2017 at 7:52 AM, Rob Herring  wrote:
> On Thu, Nov 09, 2017 at 10:45:34AM -0800, Tim Harvey wrote:
>> Add support for the TDA1997x HDMI receivers.
>>
>> Cc: Hans Verkuil 
>> Signed-off-by: Tim Harvey 
>> ---
>> v3:
>>  - use V4L2_DV_BT_FRAME_WIDTH/HEIGHT macros
>>  - fixed missing break
>>  - use only hdmi_infoframe_log for infoframe logging
>>  - simplify tda1997x_s_stream error handling
>>  - add delayed work proc to handle hotplug enable/disable
>>  - fix set_edid (disable HPD before writing, enable after)
>>  - remove enabling edid by default
>>  - initialize timings
>>  - take quant range into account in colorspace conversion
>>  - remove vendor/product tracking (we provide this in log_status via 
>> infoframes)
>>  - add v4l_controls
>>  - add more detail to log_status
>>  - calculate vhref generator timings
>>  - timing detection fixes (rounding errors, hswidth errors)
>>  - rename configure_input/configure_conv functions
>>
>> v2:
>>  - implement dv timings enum/cap
>>  - remove deprecated g_mbus_config op
>>  - fix dv_query_timings
>>  - add EDID get/set handling
>>  - remove max-pixel-rate support
>>  - add audio codec DAI support
>>  - change audio bindings
>> ---
>>  drivers/media/i2c/Kconfig|9 +
>>  drivers/media/i2c/Makefile   |1 +
>>  drivers/media/i2c/tda1997x.c | 3485 
>> ++
>>  include/dt-bindings/media/tda1997x.h |   78 +
>
> This belongs with the binding documentation patch.
>

Rob,

Thanks - missed that. I will move it for v4.

Regarding your previous comment to the v2 series:
> The rest of the binding looks fine, but I have some reservations about
> this. I think this should be common probably. There's been a few
> bindings for display recently that deal with the interface format. Maybe
> some vendor property is needed here to map a standard interface format
> back to pin configuration.

I take it this is not an 'Ack' for the bindings?

Which did you feel should be made common? I admit I was surprised
there wasn't a common binding for audio bus format (i2s|spdif) but if
you were referring to the video data that would probably be much more
complicated.

I was hoping one of the media/driver maintainers would respond to your
comment with thoughts as I'm not familiar with a very wide variety of
receivers.

Best regards,

Tim

Re: [PATCH 3/5] media: i2c: Add TDA1997x HDMI receiver driver

[Tim Harvey]

On Wed, Nov 15, 2017 at 7:52 AM, Rob Herring  wrote:
> On Thu, Nov 09, 2017 at 10:45:34AM -0800, Tim Harvey wrote:
>> Add support for the TDA1997x HDMI receivers.
>>
>> Cc: Hans Verkuil 
>> Signed-off-by: Tim Harvey 
>> ---
>> v3:
>>  - use V4L2_DV_BT_FRAME_WIDTH/HEIGHT macros
>>  - fixed missing break
>>  - use only hdmi_infoframe_log for infoframe logging
>>  - simplify tda1997x_s_stream error handling
>>  - add delayed work proc to handle hotplug enable/disable
>>  - fix set_edid (disable HPD before writing, enable after)
>>  - remove enabling edid by default
>>  - initialize timings
>>  - take quant range into account in colorspace conversion
>>  - remove vendor/product tracking (we provide this in log_status via 
>> infoframes)
>>  - add v4l_controls
>>  - add more detail to log_status
>>  - calculate vhref generator timings
>>  - timing detection fixes (rounding errors, hswidth errors)
>>  - rename configure_input/configure_conv functions
>>
>> v2:
>>  - implement dv timings enum/cap
>>  - remove deprecated g_mbus_config op
>>  - fix dv_query_timings
>>  - add EDID get/set handling
>>  - remove max-pixel-rate support
>>  - add audio codec DAI support
>>  - change audio bindings
>> ---
>>  drivers/media/i2c/Kconfig|9 +
>>  drivers/media/i2c/Makefile   |1 +
>>  drivers/media/i2c/tda1997x.c | 3485 
>> ++
>>  include/dt-bindings/media/tda1997x.h |   78 +
>
> This belongs with the binding documentation patch.
>

Rob,

Thanks - missed that. I will move it for v4.

Regarding your previous comment to the v2 series:
> The rest of the binding looks fine, but I have some reservations about
> this. I think this should be common probably. There's been a few
> bindings for display recently that deal with the interface format. Maybe
> some vendor property is needed here to map a standard interface format
> back to pin configuration.

I take it this is not an 'Ack' for the bindings?

Which did you feel should be made common? I admit I was surprised
there wasn't a common binding for audio bus format (i2s|spdif) but if
you were referring to the video data that would probably be much more
complicated.

I was hoping one of the media/driver maintainers would respond to your
comment with thoughts as I'm not familiar with a very wide variety of
receivers.

Best regards,

Tim

Re: [PATCH 3/5] media: i2c: Add TDA1997x HDMI receiver driver

[Rob Herring]

On Thu, Nov 09, 2017 at 10:45:34AM -0800, Tim Harvey wrote:
> Add support for the TDA1997x HDMI receivers.
> 
> Cc: Hans Verkuil 
> Signed-off-by: Tim Harvey 
> ---
> v3:
>  - use V4L2_DV_BT_FRAME_WIDTH/HEIGHT macros
>  - fixed missing break
>  - use only hdmi_infoframe_log for infoframe logging
>  - simplify tda1997x_s_stream error handling
>  - add delayed work proc to handle hotplug enable/disable
>  - fix set_edid (disable HPD before writing, enable after)
>  - remove enabling edid by default
>  - initialize timings
>  - take quant range into account in colorspace conversion
>  - remove vendor/product tracking (we provide this in log_status via 
> infoframes)
>  - add v4l_controls
>  - add more detail to log_status
>  - calculate vhref generator timings
>  - timing detection fixes (rounding errors, hswidth errors)
>  - rename configure_input/configure_conv functions
> 
> v2:
>  - implement dv timings enum/cap
>  - remove deprecated g_mbus_config op
>  - fix dv_query_timings
>  - add EDID get/set handling
>  - remove max-pixel-rate support
>  - add audio codec DAI support
>  - change audio bindings
> ---
>  drivers/media/i2c/Kconfig|9 +
>  drivers/media/i2c/Makefile   |1 +
>  drivers/media/i2c/tda1997x.c | 3485 
> ++
>  include/dt-bindings/media/tda1997x.h |   78 +

This belongs with the binding documentation patch.

>  include/media/i2c/tda1997x.h |   53 +
>  5 files changed, 3626 insertions(+)
>  create mode 100644 drivers/media/i2c/tda1997x.c
>  create mode 100644 include/dt-bindings/media/tda1997x.h
>  create mode 100644 include/media/i2c/tda1997x.h

Re: [PATCH 3/5] media: i2c: Add TDA1997x HDMI receiver driver

[Rob Herring]

On Thu, Nov 09, 2017 at 10:45:34AM -0800, Tim Harvey wrote:
> Add support for the TDA1997x HDMI receivers.
> 
> Cc: Hans Verkuil 
> Signed-off-by: Tim Harvey 
> ---
> v3:
>  - use V4L2_DV_BT_FRAME_WIDTH/HEIGHT macros
>  - fixed missing break
>  - use only hdmi_infoframe_log for infoframe logging
>  - simplify tda1997x_s_stream error handling
>  - add delayed work proc to handle hotplug enable/disable
>  - fix set_edid (disable HPD before writing, enable after)
>  - remove enabling edid by default
>  - initialize timings
>  - take quant range into account in colorspace conversion
>  - remove vendor/product tracking (we provide this in log_status via 
> infoframes)
>  - add v4l_controls
>  - add more detail to log_status
>  - calculate vhref generator timings
>  - timing detection fixes (rounding errors, hswidth errors)
>  - rename configure_input/configure_conv functions
> 
> v2:
>  - implement dv timings enum/cap
>  - remove deprecated g_mbus_config op
>  - fix dv_query_timings
>  - add EDID get/set handling
>  - remove max-pixel-rate support
>  - add audio codec DAI support
>  - change audio bindings
> ---
>  drivers/media/i2c/Kconfig|9 +
>  drivers/media/i2c/Makefile   |1 +
>  drivers/media/i2c/tda1997x.c | 3485 
> ++
>  include/dt-bindings/media/tda1997x.h |   78 +

This belongs with the binding documentation patch.

>  include/media/i2c/tda1997x.h |   53 +
>  5 files changed, 3626 insertions(+)
>  create mode 100644 drivers/media/i2c/tda1997x.c
>  create mode 100644 include/dt-bindings/media/tda1997x.h
>  create mode 100644 include/media/i2c/tda1997x.h