Re: [PATCH v4 1/2] media: i2c: adv748x: add adv748x driver

[Laurent Pinchart]

[snip]

On Monday 26 Jun 2017 16:14:47 Kieran Bingham wrote:
> >> +int adv748x_txa_power(struct adv748x_state *state, bool on)
> >> +{
> >> +int val;
> >> +
> >> +val = txa_read(state, ADV748X_CSI_FS_AS_LS);
> >> +if (val < 0)
> >> +return val;
> >> +
> >> +/*
> >> + * This test against BIT(6) is not documented by the datasheet, but
> >> was + * specified in the downstream driver.
> >> + * Track with a WARN_ONCE to determine if it is ever set by HW.
> >> + */
> >> +WARN_ONCE((on && val & ADV748X_CSI_FS_AS_LS_UNKNOWN),
> >> +"Enabling with unknown bit set");
> >> +
> >> +if (on)
> >> +return adv748x_write_regs(state, adv748x_power_up_txa_4lane);
> >> +else
> >
> > 'else' isn't needed.
> 
> That's a shame - I think the code is more elegant (/symmetrical) this way -
> but no worries.
> Adapted. (same for the others)

For what it's worth, I would personally have kept the else here. I'm all for

if (simple_case) {
handle_simple_case();
return 0;
}

/* Complex case */

or similar constructs with s/simple_case/uncommon_case/ or 
s/simple_case/error_case/, but here the two branches are small and symmetric, 
so an else makes sense to me to highlight that symmetry.

> >> +return adv748x_write_regs(state, adv748x_power_down_txa_4lane);
> >> +}

-- 
Regards,

Laurent Pinchart

Re: [PATCH v4 1/2] media: i2c: adv748x: add adv748x driver

[Hans Verkuil]

On 26/06/17 17:14, Kieran Bingham wrote:
> Hi Hans,
> 
> Thankyou for your review, and apologies for the delay - I was OoO last week.
> 
> 
> On 19/06/17 10:13, Hans Verkuil wrote:
>> On 06/13/2017 02:35 AM, Kieran Bingham wrote:
>>> From: Kieran Bingham 
>>>
>>> Provide support for the ADV7481 and ADV7482.
>>>
>>> The driver is modelled with 4 subdevices to allow simultaneous streaming
>>> from the AFE (Analog front end) and HDMI inputs though two CSI TX
>>> entities.
>>>
>>> The HDMI entity is linked to the TXA CSI bus, whilst the AFE is linked
>>> to the TXB CSI bus.
>>>
>>> The driver is based on a prototype by Koji Matsuoka in the Renesas BSP,
>>> and an earlier rework by Niklas Söderlund.
>>>
>>> Signed-off-by: Kieran Bingham 
>>>
>>> ---
>>>
>>> v2:
>>>   - Implement DT parsing
>>>   - adv748x: Add CSI2 entity
>>>   - adv748x: Rework pad allocations and fmts
>>>   - Give AFE 8 input pads and move pad defines
>>>   - Use the enums to ensure pads are referenced correctly.
>>>   - adv748x: Rename AFE/HDMI entities
>>> Now they are 'just afe' and 'just hdmi'
>>>   - Reorder the entity enum and structures
>>>   - Added pad-format for the CSI2 entities
>>>   - CSI2 s_stream pass through
>>>   - CSI2 control pass through (with link following)
>>>
>>> v3:
>>>   - dt: Extend DT documentation to specify interrupt mappings
>>>   - simplify adv748x_parse_dt
>>>   - core: Add banner to header file describing ADV748x variants
>>>   - Use entity structure pointers rather than global state pointers where
>>> possible
>>>   - afe: Reduce indent on afe_status
>>>   - hdmi: Add error checking to the bt->pixelclock values.
>>>   - Remove all unnecessary pad checks: handled by core
>>>   - Fix all probe cleanup paths
>>>   - adv748x_csi2_probe() now fails if it has no endpoint
>>>   - csi2: Fix small oneliners for is_txa and get_remote_sd()
>>>   - csi2: Fix checkpatch warnings
>>>   - csi2: Fix up s_stream pass-through
>>>   - csi2: Fix up Pixel Rate passthrough
>>>   - csi2: simplify adv748x_csi2_get_pad_format()
>>>   - Remove 'async notifiers' from AFE/HDMI
>>> Using async notifiers was overkill, when we have access to the
>>> subdevices internally and can register them directly.
>>>   - Use state lock in control handlers and clean up s_ctrls
>>>   - remove _interruptible mutex locks
>>>
>>> v4:
>>>   - all: Convert hex 0xXX to lowercase
>>>   - all: Use defines instead of hardcoded register values
>>>   - all: Use regmap
>>>   - afe, csi2, hdmi: _probe -> _init
>>>   - afe, csi2, hdmi: _remove -> _cleanup
>>>   - afe, hdmi: Convert pattern generator to a control
>>>   - afe, hdmi: get/set-fmt refactor
>>>   - afe, hdmi, csi2: Convert to internal calls for pixelrate
>>>   - afe: Allow the AFE to configure the Input Select from DT
>>>   - afe: Reduce indent on adv748x_afe_status switch
>>>   - afe: Remove ununsed macro definitions AIN0-7
>>>   - afe: Remove extraneous control checks handled by core
>>>   - afe: Comment fixups
>>>   - afe: Rename error label
>>>   - afe: Correct control names on the SDP
>>>   - afe: Use AIN0-7 rather than AIN1-8 to match ports and MC pads
>>>   - core: adv748x_parse_dt references to nodes, and catch multiple
>>> endpoints in a port.
>>>   - core: adv748x_dt_cleanup to simplify releasing DT nodes
>>>   - core: adv748x_print_info renamed to adv748x_identify_chip
>>>   - core: reorganise ordering of probe sequence
>>>   - core: No need for of_match_ptr
>>>   - core: Fix up i2c read/writes (regmap still on todo list)
>>>   - core: Set specific functions from the entities on subdev-init
>>>   - core: Use kzalloc for state instead of devm
>>>   - core: Improve probe error reporting
>>>   - core: Track unknown BIT(6) in tx{a,b}_power
>>>   - csi2: Improve adv748x_csi2_get_remote_sd as adv748x_csi2_get_source_sd
>>>   - csi2: -EPIPE instead of -ENODEV
>>>   - csi2: adv_dbg, instead of adv_info
>>>   - csi2: adv748x_csi2_set_format fix
>>>   - csi2: remove async notifier and sd member variables
>>>   - csi2: register links from the CSI2
>>>   - csi2: create virtual channel helper function
>>>   - dt: Remove numbering from endpoints
>>>   - dt: describe ports and interrupts as optional
>>>   - dt: Re-tab
>>>   - hdmi: adv748x_hdmi_have_signal -> adv748x_hdmi_has_signal
>>>   - hdmi: fix adv748x_hdmi_read_pixelclock return checks
>>>   - hdmi: improve adv748x_hdmi_set_video_timings
>>>   - hdmi: Fix tmp variable as polarity
>>>   - hdmi: Improve adv748x_hdmi_s_stream
>>>   - hdmi: Clean up adv748x_hdmi_s_ctrl register definitions and usage
>>>   - hdmi: Fix up adv748x_hdmi_s_dv_timings with macro names for register
>>>   - hdmi: Add locking to adv748x_hdmi_g_dv_timings
>>> writes and locking
>>>   - hdmi: adv748x_hdmi_set_de_timings function added to clarify DE writes
>>>   - hdmi: Use CP in control register naming to match component processor
>>>   - hdmi: clean up 

Re: [PATCH v4 1/2] media: i2c: adv748x: add adv748x driver

[Kieran Bingham]

Hi Hans,

Thankyou for your review, and apologies for the delay - I was OoO last week.


On 19/06/17 10:13, Hans Verkuil wrote:
> On 06/13/2017 02:35 AM, Kieran Bingham wrote:
>> From: Kieran Bingham 
>>
>> Provide support for the ADV7481 and ADV7482.
>>
>> The driver is modelled with 4 subdevices to allow simultaneous streaming
>> from the AFE (Analog front end) and HDMI inputs though two CSI TX
>> entities.
>>
>> The HDMI entity is linked to the TXA CSI bus, whilst the AFE is linked
>> to the TXB CSI bus.
>>
>> The driver is based on a prototype by Koji Matsuoka in the Renesas BSP,
>> and an earlier rework by Niklas Söderlund.
>>
>> Signed-off-by: Kieran Bingham 
>>
>> ---
>>
>> v2:
>>   - Implement DT parsing
>>   - adv748x: Add CSI2 entity
>>   - adv748x: Rework pad allocations and fmts
>>   - Give AFE 8 input pads and move pad defines
>>   - Use the enums to ensure pads are referenced correctly.
>>   - adv748x: Rename AFE/HDMI entities
>> Now they are 'just afe' and 'just hdmi'
>>   - Reorder the entity enum and structures
>>   - Added pad-format for the CSI2 entities
>>   - CSI2 s_stream pass through
>>   - CSI2 control pass through (with link following)
>>
>> v3:
>>   - dt: Extend DT documentation to specify interrupt mappings
>>   - simplify adv748x_parse_dt
>>   - core: Add banner to header file describing ADV748x variants
>>   - Use entity structure pointers rather than global state pointers where
>> possible
>>   - afe: Reduce indent on afe_status
>>   - hdmi: Add error checking to the bt->pixelclock values.
>>   - Remove all unnecessary pad checks: handled by core
>>   - Fix all probe cleanup paths
>>   - adv748x_csi2_probe() now fails if it has no endpoint
>>   - csi2: Fix small oneliners for is_txa and get_remote_sd()
>>   - csi2: Fix checkpatch warnings
>>   - csi2: Fix up s_stream pass-through
>>   - csi2: Fix up Pixel Rate passthrough
>>   - csi2: simplify adv748x_csi2_get_pad_format()
>>   - Remove 'async notifiers' from AFE/HDMI
>> Using async notifiers was overkill, when we have access to the
>> subdevices internally and can register them directly.
>>   - Use state lock in control handlers and clean up s_ctrls
>>   - remove _interruptible mutex locks
>>
>> v4:
>>   - all: Convert hex 0xXX to lowercase
>>   - all: Use defines instead of hardcoded register values
>>   - all: Use regmap
>>   - afe, csi2, hdmi: _probe -> _init
>>   - afe, csi2, hdmi: _remove -> _cleanup
>>   - afe, hdmi: Convert pattern generator to a control
>>   - afe, hdmi: get/set-fmt refactor
>>   - afe, hdmi, csi2: Convert to internal calls for pixelrate
>>   - afe: Allow the AFE to configure the Input Select from DT
>>   - afe: Reduce indent on adv748x_afe_status switch
>>   - afe: Remove ununsed macro definitions AIN0-7
>>   - afe: Remove extraneous control checks handled by core
>>   - afe: Comment fixups
>>   - afe: Rename error label
>>   - afe: Correct control names on the SDP
>>   - afe: Use AIN0-7 rather than AIN1-8 to match ports and MC pads
>>   - core: adv748x_parse_dt references to nodes, and catch multiple
>> endpoints in a port.
>>   - core: adv748x_dt_cleanup to simplify releasing DT nodes
>>   - core: adv748x_print_info renamed to adv748x_identify_chip
>>   - core: reorganise ordering of probe sequence
>>   - core: No need for of_match_ptr
>>   - core: Fix up i2c read/writes (regmap still on todo list)
>>   - core: Set specific functions from the entities on subdev-init
>>   - core: Use kzalloc for state instead of devm
>>   - core: Improve probe error reporting
>>   - core: Track unknown BIT(6) in tx{a,b}_power
>>   - csi2: Improve adv748x_csi2_get_remote_sd as adv748x_csi2_get_source_sd
>>   - csi2: -EPIPE instead of -ENODEV
>>   - csi2: adv_dbg, instead of adv_info
>>   - csi2: adv748x_csi2_set_format fix
>>   - csi2: remove async notifier and sd member variables
>>   - csi2: register links from the CSI2
>>   - csi2: create virtual channel helper function
>>   - dt: Remove numbering from endpoints
>>   - dt: describe ports and interrupts as optional
>>   - dt: Re-tab
>>   - hdmi: adv748x_hdmi_have_signal -> adv748x_hdmi_has_signal
>>   - hdmi: fix adv748x_hdmi_read_pixelclock return checks
>>   - hdmi: improve adv748x_hdmi_set_video_timings
>>   - hdmi: Fix tmp variable as polarity
>>   - hdmi: Improve adv748x_hdmi_s_stream
>>   - hdmi: Clean up adv748x_hdmi_s_ctrl register definitions and usage
>>   - hdmi: Fix up adv748x_hdmi_s_dv_timings with macro names for register
>>   - hdmi: Add locking to adv748x_hdmi_g_dv_timings
>> writes and locking
>>   - hdmi: adv748x_hdmi_set_de_timings function added to clarify DE writes
>>   - hdmi: Use CP in control register naming to match component processor
>>   - hdmi: clean up adv748x_hdmi_query_dv_timings()
>>   - KConfig: Fix up dependency and capitalisation
>>
>>
>>   Documentation/devicetree/bindings/media/i2c/adv748x.txt |  96 +-
> 
> This should be 

Re: [PATCH v4 1/2] media: i2c: adv748x: add adv748x driver

[Hans Verkuil]

On 06/13/2017 02:35 AM, Kieran Bingham wrote:

From: Kieran Bingham 

Provide support for the ADV7481 and ADV7482.

The driver is modelled with 4 subdevices to allow simultaneous streaming
from the AFE (Analog front end) and HDMI inputs though two CSI TX
entities.

The HDMI entity is linked to the TXA CSI bus, whilst the AFE is linked
to the TXB CSI bus.

The driver is based on a prototype by Koji Matsuoka in the Renesas BSP,
and an earlier rework by Niklas Söderlund.

Signed-off-by: Kieran Bingham 

---

v2:
  - Implement DT parsing
  - adv748x: Add CSI2 entity
  - adv748x: Rework pad allocations and fmts
  - Give AFE 8 input pads and move pad defines
  - Use the enums to ensure pads are referenced correctly.
  - adv748x: Rename AFE/HDMI entities
Now they are 'just afe' and 'just hdmi'
  - Reorder the entity enum and structures
  - Added pad-format for the CSI2 entities
  - CSI2 s_stream pass through
  - CSI2 control pass through (with link following)

v3:
  - dt: Extend DT documentation to specify interrupt mappings
  - simplify adv748x_parse_dt
  - core: Add banner to header file describing ADV748x variants
  - Use entity structure pointers rather than global state pointers where
possible
  - afe: Reduce indent on afe_status
  - hdmi: Add error checking to the bt->pixelclock values.
  - Remove all unnecessary pad checks: handled by core
  - Fix all probe cleanup paths
  - adv748x_csi2_probe() now fails if it has no endpoint
  - csi2: Fix small oneliners for is_txa and get_remote_sd()
  - csi2: Fix checkpatch warnings
  - csi2: Fix up s_stream pass-through
  - csi2: Fix up Pixel Rate passthrough
  - csi2: simplify adv748x_csi2_get_pad_format()
  - Remove 'async notifiers' from AFE/HDMI
Using async notifiers was overkill, when we have access to the
subdevices internally and can register them directly.
  - Use state lock in control handlers and clean up s_ctrls
  - remove _interruptible mutex locks

v4:
  - all: Convert hex 0xXX to lowercase
  - all: Use defines instead of hardcoded register values
  - all: Use regmap
  - afe, csi2, hdmi: _probe -> _init
  - afe, csi2, hdmi: _remove -> _cleanup
  - afe, hdmi: Convert pattern generator to a control
  - afe, hdmi: get/set-fmt refactor
  - afe, hdmi, csi2: Convert to internal calls for pixelrate
  - afe: Allow the AFE to configure the Input Select from DT
  - afe: Reduce indent on adv748x_afe_status switch
  - afe: Remove ununsed macro definitions AIN0-7
  - afe: Remove extraneous control checks handled by core
  - afe: Comment fixups
  - afe: Rename error label
  - afe: Correct control names on the SDP
  - afe: Use AIN0-7 rather than AIN1-8 to match ports and MC pads
  - core: adv748x_parse_dt references to nodes, and catch multiple
endpoints in a port.
  - core: adv748x_dt_cleanup to simplify releasing DT nodes
  - core: adv748x_print_info renamed to adv748x_identify_chip
  - core: reorganise ordering of probe sequence
  - core: No need for of_match_ptr
  - core: Fix up i2c read/writes (regmap still on todo list)
  - core: Set specific functions from the entities on subdev-init
  - core: Use kzalloc for state instead of devm
  - core: Improve probe error reporting
  - core: Track unknown BIT(6) in tx{a,b}_power
  - csi2: Improve adv748x_csi2_get_remote_sd as adv748x_csi2_get_source_sd
  - csi2: -EPIPE instead of -ENODEV
  - csi2: adv_dbg, instead of adv_info
  - csi2: adv748x_csi2_set_format fix
  - csi2: remove async notifier and sd member variables
  - csi2: register links from the CSI2
  - csi2: create virtual channel helper function
  - dt: Remove numbering from endpoints
  - dt: describe ports and interrupts as optional
  - dt: Re-tab
  - hdmi: adv748x_hdmi_have_signal -> adv748x_hdmi_has_signal
  - hdmi: fix adv748x_hdmi_read_pixelclock return checks
  - hdmi: improve adv748x_hdmi_set_video_timings
  - hdmi: Fix tmp variable as polarity
  - hdmi: Improve adv748x_hdmi_s_stream
  - hdmi: Clean up adv748x_hdmi_s_ctrl register definitions and usage
  - hdmi: Fix up adv748x_hdmi_s_dv_timings with macro names for register
  - hdmi: Add locking to adv748x_hdmi_g_dv_timings
writes and locking
  - hdmi: adv748x_hdmi_set_de_timings function added to clarify DE writes
  - hdmi: Use CP in control register naming to match component processor
  - hdmi: clean up adv748x_hdmi_query_dv_timings()
  - KConfig: Fix up dependency and capitalisation


  Documentation/devicetree/bindings/media/i2c/adv748x.txt |  96 +-


This should be a separate patch cross posted to the devicetree mailinglist.


  MAINTAINERS |   6 +-


This should also be a separate patch.


  drivers/media/i2c/Kconfig   |  11 +-
  drivers/media/i2c/Makefile  |   1 +-
  drivers/media/i2c/adv748x/Makefile  |   7 +-
  drivers/media/i2c/adv748x/adv748x-afe.c | 571 

Re: [PATCH v4 1/2] media: i2c: adv748x: add adv748x driver

[Kieran Bingham]

Hi Niklas

On 13/06/17 08:33, Niklas Söderlund wrote:
> Hi Kieran,
> 
> Thanks for your patch, and great work!

Thanks for taking a look.

> On 2017-06-13 01:35:07 +0100, Kieran Bingham wrote:
>> From: Kieran Bingham 
>>
>> Provide support for the ADV7481 and ADV7482.
>>
>> The driver is modelled with 4 subdevices to allow simultaneous streaming
>> from the AFE (Analog front end) and HDMI inputs though two CSI TX
>> entities.
>>
>> The HDMI entity is linked to the TXA CSI bus, whilst the AFE is linked
>> to the TXB CSI bus.
>>
>> The driver is based on a prototype by Koji Matsuoka in the Renesas BSP,
>> and an earlier rework by Niklas Söderlund.
>>
>> Signed-off-by: Kieran Bingham 
>>



>> +static int adv748x_afe_get_format(struct v4l2_subdev *sd,
>> +  struct v4l2_subdev_pad_config *cfg,
>> +  struct v4l2_subdev_format *sdformat)
>> +{
>> +struct adv748x_afe *afe = adv748x_sd_to_afe(sd);
>> +struct v4l2_mbus_framefmt *mbusformat;
>> +
>> +/* The format of the analog sink pads is nonsensical */
>> +if (sdformat->pad != ADV748X_AFE_SOURCE)
>> +return -EINVAL;
>> +
>> +if (sdformat->which == V4L2_SUBDEV_FORMAT_TRY) {
>> +mbusformat = v4l2_subdev_get_try_format(sd, cfg, sdformat->pad);
>> +sdformat->format = *mbusformat;
>> +} else {
>> +adv748x_afe_fill_format(afe, >format);
>> +adv748x_afe_set_pixelrate(afe);
>> +}
>> +
>> +return 0;
>> +}
>> +
>> +static int adv748x_afe_set_format(struct v4l2_subdev *sd,
>> +  struct v4l2_subdev_pad_config *cfg,
>> +  struct v4l2_subdev_format *sdformat)
>> +{
>> +struct v4l2_mbus_framefmt *mbusformat;
>> +
>> +/* The format of the analog sink pads is nonsensical */
>> +if (sdformat->pad != ADV748X_AFE_SOURCE)
>> +return -EINVAL;
>> +
>> +if (sdformat->which == V4L2_SUBDEV_FORMAT_ACTIVE)
>> +return adv748x_afe_get_format(sd, cfg, sdformat);
>> +
>> +mbusformat = v4l2_subdev_get_try_format(sd, cfg, sdformat->pad);
>> +*mbusformat = sdformat->format;
> 
> Hum, for the V4L2_SUBDEV_FORMAT_TRY case will this not accept any format 
> provided to the function? Should you not limit this to the device 
> capabilities before assigning it to mbusformat ?

Hrmmm maybe it got too late last night :)

I was trying to remove the effect of the active setting on the TRY format, and
I've gone too far :)

> 
>> +
>> +return 0;
>> +}




>> +
>> +static int adv748x_setup_links(struct adv748x_state *state)
>> +{
>> +int ret;
>> +int enabled = MEDIA_LNK_FL_ENABLED;
>> +
>> +/*
>> + * HACK/Workaround:
>> + *
>> + * Currently non-immutable link resets go through the RVin
>> + * driver, and cause the links to fail, due to not being part of RVIN.
>> + * As a temporary workaround until the RVIN driver knows better than to 
>> parse
>> + * links that do not belong to it, use static immutable links for our 
>> internal
>> + * media paths.
>> + */
> 
> The problem is a bigger then just the VIN ignoring the links not 
> belonging to it self I think. The ADV7482 driver must have a link 
> notification handler to deal with the links that belong to it.
> 
> Else if all links of the media device is reset there is no way to setup 
> the links between the different ADV7482 subdevices, or am I missing 
> something?

Ahhh -- this function shouldn't even be in here ! It's not meant to be used -
Links are now created in adv748x_csi2_register_link() so now I'm concerned why I
didn't get an unused function compiler warning :)

However - your point still stands.

> 
>> +#define ADV748x_DEV_STATIC_LINKS
>> +#ifdef ADV748x_DEV_STATIC_LINKS
>> +enabled |= MEDIA_LNK_FL_IMMUTABLE;
>> +#endif
>> +
>> +/* TXA - Default link is with HDMI */
>> +ret = media_create_pad_link(>hdmi.sd.entity, 1,
>> +>txa.sd.entity, 0, enabled);
>> +if (ret) {
>> +adv_err(state, "Failed to create HDMI-TXA pad link");
>> +return ret;
>> +}
>> +
>> +#ifndef ADV748x_DEV_STATIC_LINKS
>> +ret = media_create_pad_link(>afe.sd.entity, ADV748X_AFE_SOURCE,
>> +>txa.sd.entity, 0, 0);
>> +if (ret) {
>> +adv_err(state, "Failed to create AFE-TXA pad link");
>> +return ret;
>> +}
>> +#endif
>> +
>> +/* TXB - Can only output from the AFE */
>> +ret = media_create_pad_link(>afe.sd.entity, ADV748X_AFE_SOURCE,
>> +>txb.sd.entity, 0, enabled);
>> +if (ret) {
>> +adv_err(state, "Failed to create AFE-TXB pad link");
>> +return ret;
>> +}
>> +
>> +return 0;
>> +}
>> +
>> +int adv748x_register_subdevs(struct adv748x_state *state,
>> + struct v4l2_device *v4l2_dev)

And 

Re: [PATCH v4 1/2] media: i2c: adv748x: add adv748x driver

[Geert Uytterhoeven]

Hi Kieran,

On Tue, Jun 13, 2017 at 11:32 AM, Kieran Bingham
 wrote:
> On 13/06/17 10:24, Geert Uytterhoeven wrote:
>> On Tue, Jun 13, 2017 at 2:35 AM, Kieran Bingham  wrote:
>>> From: Kieran Bingham 
>>>
>>> Provide support for the ADV7481 and ADV7482.
>>>
>>> The driver is modelled with 4 subdevices to allow simultaneous streaming
>>> from the AFE (Analog front end) and HDMI inputs though two CSI TX
>>> entities.
>>>
>>> The HDMI entity is linked to the TXA CSI bus, whilst the AFE is linked
>>> to the TXB CSI bus.
>>>
>>> The driver is based on a prototype by Koji Matsuoka in the Renesas BSP,
>>> and an earlier rework by Niklas Söderlund.
>>>
>>> Signed-off-by: Kieran Bingham 
>>
>>> --- /dev/null
>>> +++ b/drivers/media/i2c/adv748x/adv748x-hdmi.c
>>
>>> +static int adv748x_hdmi_set_pixelrate(struct adv748x_hdmi *hdmi)
>>> +{
>>> +   struct v4l2_subdev *tx;
>>> +   struct v4l2_dv_timings timings;
>>> +   struct v4l2_bt_timings *bt = 
>>> +   unsigned int fps;
>>> +
>>> +   tx = adv748x_get_remote_sd(>pads[ADV748X_HDMI_SOURCE]);
>>> +   if (!tx)
>>> +   return -ENOLINK;
>>> +
>>> +   adv748x_hdmi_query_dv_timings(>sd, );
>>> +
>>> +   fps = DIV_ROUND_CLOSEST(bt->pixelclock,
>>> +   V4L2_DV_BT_FRAME_WIDTH(bt) *
>>> +   V4L2_DV_BT_FRAME_HEIGHT(bt));
>>
>> On arm32:
>>
>> drivers/built-in.o: In function `adv748x_hdmi_set_pixelrate':
>> :(.text+0x1b8b1c): undefined reference to `__aeabi_uldivmod'
>>
>> v4l2_bt_timings.pixelclock is u64, so you should use DIV_ROUND_CLOSEST_ULL()
>> instead.
>
> Aha, thanks.
>
> /me ponders why I didn't get spammed from the bot-builders about this?

-EBUSY?

> Fix applied locally ready for v5.
>
> Would you like the remote updated for renesas-drivers or will you patch 
> locally?

I'll patch it locally just to avoid receiving more spam from the builders soon
(we don't use adv748x on arm32 boards).

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

Re: [PATCH v4 1/2] media: i2c: adv748x: add adv748x driver

[Kieran Bingham]

On 13/06/17 10:24, Geert Uytterhoeven wrote:
> Hi Kieran,
> 
> On Tue, Jun 13, 2017 at 2:35 AM, Kieran Bingham  wrote:
>> From: Kieran Bingham 
>>
>> Provide support for the ADV7481 and ADV7482.
>>
>> The driver is modelled with 4 subdevices to allow simultaneous streaming
>> from the AFE (Analog front end) and HDMI inputs though two CSI TX
>> entities.
>>
>> The HDMI entity is linked to the TXA CSI bus, whilst the AFE is linked
>> to the TXB CSI bus.
>>
>> The driver is based on a prototype by Koji Matsuoka in the Renesas BSP,
>> and an earlier rework by Niklas Söderlund.
>>
>> Signed-off-by: Kieran Bingham 
> 
>> --- /dev/null
>> +++ b/drivers/media/i2c/adv748x/adv748x-hdmi.c
> 
>> +static int adv748x_hdmi_set_pixelrate(struct adv748x_hdmi *hdmi)
>> +{
>> +   struct v4l2_subdev *tx;
>> +   struct v4l2_dv_timings timings;
>> +   struct v4l2_bt_timings *bt = 
>> +   unsigned int fps;
>> +
>> +   tx = adv748x_get_remote_sd(>pads[ADV748X_HDMI_SOURCE]);
>> +   if (!tx)
>> +   return -ENOLINK;
>> +
>> +   adv748x_hdmi_query_dv_timings(>sd, );
>> +
>> +   fps = DIV_ROUND_CLOSEST(bt->pixelclock,
>> +   V4L2_DV_BT_FRAME_WIDTH(bt) *
>> +   V4L2_DV_BT_FRAME_HEIGHT(bt));
> 
> On arm32:
> 
> drivers/built-in.o: In function `adv748x_hdmi_set_pixelrate':
> :(.text+0x1b8b1c): undefined reference to `__aeabi_uldivmod'
> 
> v4l2_bt_timings.pixelclock is u64, so you should use DIV_ROUND_CLOSEST_ULL()
> instead.

Aha, thanks.

/me ponders why I didn't get spammed from the bot-builders about this?

Fix applied locally ready for v5.

Would you like the remote updated for renesas-drivers or will you patch locally?

--
Kieran

> Gr{oetje,eeting}s,
> 
> Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- 
> ge...@linux-m68k.org
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like 
> that.
> -- Linus Torvalds
> 

Re: [PATCH v4 1/2] media: i2c: adv748x: add adv748x driver

[Geert Uytterhoeven]

Hi Kieran,

On Tue, Jun 13, 2017 at 2:35 AM, Kieran Bingham  wrote:
> From: Kieran Bingham 
>
> Provide support for the ADV7481 and ADV7482.
>
> The driver is modelled with 4 subdevices to allow simultaneous streaming
> from the AFE (Analog front end) and HDMI inputs though two CSI TX
> entities.
>
> The HDMI entity is linked to the TXA CSI bus, whilst the AFE is linked
> to the TXB CSI bus.
>
> The driver is based on a prototype by Koji Matsuoka in the Renesas BSP,
> and an earlier rework by Niklas Söderlund.
>
> Signed-off-by: Kieran Bingham 

> --- /dev/null
> +++ b/drivers/media/i2c/adv748x/adv748x-hdmi.c

> +static int adv748x_hdmi_set_pixelrate(struct adv748x_hdmi *hdmi)
> +{
> +   struct v4l2_subdev *tx;
> +   struct v4l2_dv_timings timings;
> +   struct v4l2_bt_timings *bt = 
> +   unsigned int fps;
> +
> +   tx = adv748x_get_remote_sd(>pads[ADV748X_HDMI_SOURCE]);
> +   if (!tx)
> +   return -ENOLINK;
> +
> +   adv748x_hdmi_query_dv_timings(>sd, );
> +
> +   fps = DIV_ROUND_CLOSEST(bt->pixelclock,
> +   V4L2_DV_BT_FRAME_WIDTH(bt) *
> +   V4L2_DV_BT_FRAME_HEIGHT(bt));

On arm32:

drivers/built-in.o: In function `adv748x_hdmi_set_pixelrate':
:(.text+0x1b8b1c): undefined reference to `__aeabi_uldivmod'

v4l2_bt_timings.pixelclock is u64, so you should use DIV_ROUND_CLOSEST_ULL()
instead.

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

Re: [PATCH v4 1/2] media: i2c: adv748x: add adv748x driver

[Niklas Söderlund]

Hi Kieran,

Thanks for your patch, and great work!

On 2017-06-13 01:35:07 +0100, Kieran Bingham wrote:
> From: Kieran Bingham 
> 
> Provide support for the ADV7481 and ADV7482.
> 
> The driver is modelled with 4 subdevices to allow simultaneous streaming
> from the AFE (Analog front end) and HDMI inputs though two CSI TX
> entities.
> 
> The HDMI entity is linked to the TXA CSI bus, whilst the AFE is linked
> to the TXB CSI bus.
> 
> The driver is based on a prototype by Koji Matsuoka in the Renesas BSP,
> and an earlier rework by Niklas Söderlund.
> 
> Signed-off-by: Kieran Bingham 
> 
> ---
> 
> v2:
>  - Implement DT parsing
>  - adv748x: Add CSI2 entity
>  - adv748x: Rework pad allocations and fmts
>  - Give AFE 8 input pads and move pad defines
>  - Use the enums to ensure pads are referenced correctly.
>  - adv748x: Rename AFE/HDMI entities
>Now they are 'just afe' and 'just hdmi'
>  - Reorder the entity enum and structures
>  - Added pad-format for the CSI2 entities
>  - CSI2 s_stream pass through
>  - CSI2 control pass through (with link following)
> 
> v3:
>  - dt: Extend DT documentation to specify interrupt mappings
>  - simplify adv748x_parse_dt
>  - core: Add banner to header file describing ADV748x variants
>  - Use entity structure pointers rather than global state pointers where
>possible
>  - afe: Reduce indent on afe_status
>  - hdmi: Add error checking to the bt->pixelclock values.
>  - Remove all unnecessary pad checks: handled by core
>  - Fix all probe cleanup paths
>  - adv748x_csi2_probe() now fails if it has no endpoint
>  - csi2: Fix small oneliners for is_txa and get_remote_sd()
>  - csi2: Fix checkpatch warnings
>  - csi2: Fix up s_stream pass-through
>  - csi2: Fix up Pixel Rate passthrough
>  - csi2: simplify adv748x_csi2_get_pad_format()
>  - Remove 'async notifiers' from AFE/HDMI
>Using async notifiers was overkill, when we have access to the
>subdevices internally and can register them directly.
>  - Use state lock in control handlers and clean up s_ctrls
>  - remove _interruptible mutex locks
> 
> v4:
>  - all: Convert hex 0xXX to lowercase
>  - all: Use defines instead of hardcoded register values
>  - all: Use regmap
>  - afe, csi2, hdmi: _probe -> _init
>  - afe, csi2, hdmi: _remove -> _cleanup
>  - afe, hdmi: Convert pattern generator to a control
>  - afe, hdmi: get/set-fmt refactor
>  - afe, hdmi, csi2: Convert to internal calls for pixelrate
>  - afe: Allow the AFE to configure the Input Select from DT
>  - afe: Reduce indent on adv748x_afe_status switch
>  - afe: Remove ununsed macro definitions AIN0-7
>  - afe: Remove extraneous control checks handled by core
>  - afe: Comment fixups
>  - afe: Rename error label
>  - afe: Correct control names on the SDP
>  - afe: Use AIN0-7 rather than AIN1-8 to match ports and MC pads
>  - core: adv748x_parse_dt references to nodes, and catch multiple
>endpoints in a port.
>  - core: adv748x_dt_cleanup to simplify releasing DT nodes
>  - core: adv748x_print_info renamed to adv748x_identify_chip
>  - core: reorganise ordering of probe sequence
>  - core: No need for of_match_ptr
>  - core: Fix up i2c read/writes (regmap still on todo list)
>  - core: Set specific functions from the entities on subdev-init
>  - core: Use kzalloc for state instead of devm
>  - core: Improve probe error reporting
>  - core: Track unknown BIT(6) in tx{a,b}_power
>  - csi2: Improve adv748x_csi2_get_remote_sd as adv748x_csi2_get_source_sd
>  - csi2: -EPIPE instead of -ENODEV
>  - csi2: adv_dbg, instead of adv_info
>  - csi2: adv748x_csi2_set_format fix
>  - csi2: remove async notifier and sd member variables
>  - csi2: register links from the CSI2
>  - csi2: create virtual channel helper function
>  - dt: Remove numbering from endpoints
>  - dt: describe ports and interrupts as optional
>  - dt: Re-tab
>  - hdmi: adv748x_hdmi_have_signal -> adv748x_hdmi_has_signal
>  - hdmi: fix adv748x_hdmi_read_pixelclock return checks
>  - hdmi: improve adv748x_hdmi_set_video_timings
>  - hdmi: Fix tmp variable as polarity
>  - hdmi: Improve adv748x_hdmi_s_stream
>  - hdmi: Clean up adv748x_hdmi_s_ctrl register definitions and usage
>  - hdmi: Fix up adv748x_hdmi_s_dv_timings with macro names for register
>  - hdmi: Add locking to adv748x_hdmi_g_dv_timings
>writes and locking
>  - hdmi: adv748x_hdmi_set_de_timings function added to clarify DE writes
>  - hdmi: Use CP in control register naming to match component processor
>  - hdmi: clean up adv748x_hdmi_query_dv_timings()
>  - KConfig: Fix up dependency and capitalisation
> 
> 
>  Documentation/devicetree/bindings/media/i2c/adv748x.txt |  96 +-
>  MAINTAINERS |   6 +-
>  drivers/media/i2c/Kconfig   |  11 +-
>  drivers/media/i2c/Makefile  |   1 +-
>  drivers/media/i2c/adv748x/Makefile  |   7 

[PATCH v4 1/2] media: i2c: adv748x: add adv748x driver

[Kieran Bingham]

From: Kieran Bingham 

Provide support for the ADV7481 and ADV7482.

The driver is modelled with 4 subdevices to allow simultaneous streaming
from the AFE (Analog front end) and HDMI inputs though two CSI TX
entities.

The HDMI entity is linked to the TXA CSI bus, whilst the AFE is linked
to the TXB CSI bus.

The driver is based on a prototype by Koji Matsuoka in the Renesas BSP,
and an earlier rework by Niklas Söderlund.

Signed-off-by: Kieran Bingham 

---

v2:
 - Implement DT parsing
 - adv748x: Add CSI2 entity
 - adv748x: Rework pad allocations and fmts
 - Give AFE 8 input pads and move pad defines
 - Use the enums to ensure pads are referenced correctly.
 - adv748x: Rename AFE/HDMI entities
   Now they are 'just afe' and 'just hdmi'
 - Reorder the entity enum and structures
 - Added pad-format for the CSI2 entities
 - CSI2 s_stream pass through
 - CSI2 control pass through (with link following)

v3:
 - dt: Extend DT documentation to specify interrupt mappings
 - simplify adv748x_parse_dt
 - core: Add banner to header file describing ADV748x variants
 - Use entity structure pointers rather than global state pointers where
   possible
 - afe: Reduce indent on afe_status
 - hdmi: Add error checking to the bt->pixelclock values.
 - Remove all unnecessary pad checks: handled by core
 - Fix all probe cleanup paths
 - adv748x_csi2_probe() now fails if it has no endpoint
 - csi2: Fix small oneliners for is_txa and get_remote_sd()
 - csi2: Fix checkpatch warnings
 - csi2: Fix up s_stream pass-through
 - csi2: Fix up Pixel Rate passthrough
 - csi2: simplify adv748x_csi2_get_pad_format()
 - Remove 'async notifiers' from AFE/HDMI
   Using async notifiers was overkill, when we have access to the
   subdevices internally and can register them directly.
 - Use state lock in control handlers and clean up s_ctrls
 - remove _interruptible mutex locks

v4:
 - all: Convert hex 0xXX to lowercase
 - all: Use defines instead of hardcoded register values
 - all: Use regmap
 - afe, csi2, hdmi: _probe -> _init
 - afe, csi2, hdmi: _remove -> _cleanup
 - afe, hdmi: Convert pattern generator to a control
 - afe, hdmi: get/set-fmt refactor
 - afe, hdmi, csi2: Convert to internal calls for pixelrate
 - afe: Allow the AFE to configure the Input Select from DT
 - afe: Reduce indent on adv748x_afe_status switch
 - afe: Remove ununsed macro definitions AIN0-7
 - afe: Remove extraneous control checks handled by core
 - afe: Comment fixups
 - afe: Rename error label
 - afe: Correct control names on the SDP
 - afe: Use AIN0-7 rather than AIN1-8 to match ports and MC pads
 - core: adv748x_parse_dt references to nodes, and catch multiple
   endpoints in a port.
 - core: adv748x_dt_cleanup to simplify releasing DT nodes
 - core: adv748x_print_info renamed to adv748x_identify_chip
 - core: reorganise ordering of probe sequence
 - core: No need for of_match_ptr
 - core: Fix up i2c read/writes (regmap still on todo list)
 - core: Set specific functions from the entities on subdev-init
 - core: Use kzalloc for state instead of devm
 - core: Improve probe error reporting
 - core: Track unknown BIT(6) in tx{a,b}_power
 - csi2: Improve adv748x_csi2_get_remote_sd as adv748x_csi2_get_source_sd
 - csi2: -EPIPE instead of -ENODEV
 - csi2: adv_dbg, instead of adv_info
 - csi2: adv748x_csi2_set_format fix
 - csi2: remove async notifier and sd member variables
 - csi2: register links from the CSI2
 - csi2: create virtual channel helper function
 - dt: Remove numbering from endpoints
 - dt: describe ports and interrupts as optional
 - dt: Re-tab
 - hdmi: adv748x_hdmi_have_signal -> adv748x_hdmi_has_signal
 - hdmi: fix adv748x_hdmi_read_pixelclock return checks
 - hdmi: improve adv748x_hdmi_set_video_timings
 - hdmi: Fix tmp variable as polarity
 - hdmi: Improve adv748x_hdmi_s_stream
 - hdmi: Clean up adv748x_hdmi_s_ctrl register definitions and usage
 - hdmi: Fix up adv748x_hdmi_s_dv_timings with macro names for register
 - hdmi: Add locking to adv748x_hdmi_g_dv_timings
   writes and locking
 - hdmi: adv748x_hdmi_set_de_timings function added to clarify DE writes
 - hdmi: Use CP in control register naming to match component processor
 - hdmi: clean up adv748x_hdmi_query_dv_timings()
 - KConfig: Fix up dependency and capitalisation


 Documentation/devicetree/bindings/media/i2c/adv748x.txt |  96 +-
 MAINTAINERS |   6 +-
 drivers/media/i2c/Kconfig   |  11 +-
 drivers/media/i2c/Makefile  |   1 +-
 drivers/media/i2c/adv748x/Makefile  |   7 +-
 drivers/media/i2c/adv748x/adv748x-afe.c | 571 ++-
 drivers/media/i2c/adv748x/adv748x-core.c| 907 +-
 drivers/media/i2c/adv748x/adv748x-csi2.c| 323 +++-
 drivers/media/i2c/adv748x/adv748x-hdmi.c| 652 ++-
 drivers/media/i2c/adv748x/adv748x.h