Hi Kieran,
On Tuesday, 18 September 2018 23:35:16 EEST Kieran Bingham wrote:
> On 18/09/18 20:29, Niklas Söderlund wrote:
> > On 2018-09-18 11:13:45 +0100, Kieran Bingham wrote:
> >> On 18/09/18 02:45, Niklas Söderlund wrote:
> >>> The driver fixed the TXA CSI-2 transmitter in 4-lane mode while it could
> >>> operate using 1-, 2- and 4-lanes. Update the driver to support all modes
> >>> the hardware does.
> >>>
> >>> The driver make use of large tables of static register/value writes when
> >>> configuring the hardware, some writing to undocumented registers.
> >>> Instead of creating 3 sets of the register tables for the different
> >>> modes catch when the register containing NUM_LANES[2:0] is written to
> >>> and inject the correct number of lanes.
> >>
> >> Aye aye aye.
> >>
> >> Neat solution to avoid adding more tables - but is it necessary? And I
> >> can't find it easy to overlook the hack value in checking every register
> >> write against a specific value :-(
> >
> > I agree it's not the most obvious nice solution.
> >
> >> Couldn't we create a function called "adv748x_configure_tx_lanes(...)"
> >> or such and just write this value as appropriate after the tables are
> >> written?
> >>
> >> That will then hopefully take us a step towards not needing (as much of)
> >> these tables.
> >
> > This was my starting point :-) But the register is referenced multiple
> > times in the tables and converting the tables to individual register
> > writes turned out such a mess I judged this solution to be less of a
> > maintenance burden. I'm however open to your views on how you would
> > prefer this to be handled. Keep in mind that each row in the register
> > tables needs to be turned into a:
> >
> > /* Write registerx */
> > ret = adv748x_write(...)
> > if (ret)
> >
> > return ret;
>
> Yes, that construct for each register is certainly painful, compared to
> a table...
>
> I wonder if we can be 'clever/naughty' with macros, or perhaps just do a
> best effort set of writes and catch if any fail.. (this also might be a
> bit ugly)
>
> ret |= adv748x_write(A, a);
> ret |= adv748x_write(B, b);
> ret |= adv748x_write(C, c);
> ret |= adv748x_write(D, d);
> if (ret)
> return -EIO;
I'm not very fond of such constructs as it can hide the original error code. I
proposed an alternative in this mail thread. If that ends up adding too much
overhead the above construct could be considered.
> Or - we could programmatically create the tables of registers to write
> in an array, and easily turn the table generation into code which we can
> manipulate without checking errors after each value.
>
> Then the whole table would get written in a single batch...
>
> (Sort of like how we treat the display lists in the VSP1)
We could do that, but my gut feeling is that it would be too complex and over-
engineered.
> I'm not sure how much code that would take yet, or if it will be more or
> less readable :) Needs some thought....
>
> I wonder what Laurent's take on this is ....
Hopefully you're not wondering anymore :-)
> > So the overview of what happens become much harder to read. Other
> > options such as creating copies of the tables and injecting the NUM_LANE
> > value at probe time I feel just hides the behavior even more.
> >
> > Another option I tried was to splice the tables whenever the register in
> > question was referenced. This became hard to read but less lines of
> > code.
> >
> >> However - *I fully understand ordering may be important here* so
> >> actually it looks like we can't write this after writing the table.
> >>
> >> But it does look conveniently early in the tables, so we could split the
> >> tables out and start functionalising them with the information we do
> >> know.
> >
> > I have not tested if ordering is important or not, the documentation we
> > have is just a sequential list of register writes, The register is used
> > in multiple places in the tables making things even more ugly.
>
> yeouch.
>
> > adv748x_power_up_txa_4lane: 3 times, beginning and middle
> > adv748x_power_down_txa_4lane: 1 time, middle
> > adv748x_init_txa_4lane: 3 times, middle and end
> >
> >> I.e. We could have our init function enable the lanes, and handle the
> >> auto DPHY, then write the rest through the tables.
> >
> > If only that where possible :-)
> >
> > I hold off posting v2 until I know how you wish to handle this. To help
> > you make a decision the number of register writes in the tables involved
> >
> > :-)
> >
> > adv748x_power_up_txa_4lane: 11
> > adv748x_power_down_txa_4lane: 5
>
> I feel like the powerup and down, are good targets for converting to
> functions, and merging to support both TXA and TXB (I appreciate this is
> not a five minute job, but something on my wish-list)
>
> > adv748x_init_txa_4lane: ~50
>
> Yes, 50 writes is probably a lot more painful ...
>
> >>> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@xxxxxxxxxxxx>
> >>> ---
> >>>
> >>> drivers/media/i2c/adv748x/adv748x-core.c | 38 +++++++++++++++++++-----
> >>> 1 file changed, 30 insertions(+), 8 deletions(-)
> >>>
> >>> diff --git a/drivers/media/i2c/adv748x/adv748x-core.c
> >>> b/drivers/media/i2c/adv748x/adv748x-core.c index
> >>> a93f8ea89a228474..9a82cdf301bccb41 100644
> >>> --- a/drivers/media/i2c/adv748x/adv748x-core.c
> >>> +++ b/drivers/media/i2c/adv748x/adv748x-core.c
> >>> @@ -207,13 +207,23 @@ static int adv748x_write_regs(struct adv748x_state
> >>> *state,>>>
> >>> const struct adv748x_reg_value *regs)
> >>>
> >>> {
> >>>
> >>> int ret;
> >>>
> >>> + u8 value;
> >>>
> >>> while (regs->page != ADV748X_PAGE_EOR) {
> >>>
> >>> if (regs->page == ADV748X_PAGE_WAIT) {
> >>>
> >>> msleep(regs->value);
> >>>
> >>> } else {
> >>>
> >>> + value = regs->value;
> >>> +
> >>> + /*
> >>> + * Register 0x00 in TXA needs to bei injected with
> >>
> >> s/bei/be/
> >>
> >>> + * the number of CSI-2 lanes used to transmitt.
> >>
> >> s/transmitt/transmit/
> >>
> >>> + */
> >>> + if (regs->page == ADV748X_PAGE_TXA && regs->reg == 0x00)
> >>> + value = (value & ~7) | state->txa.num_lanes;
> >>> +
> >>>
> >>> ret = adv748x_write(state, regs->page, regs->reg,
> >>>
> >>> - regs->value);
> >>> + value);
> >>>
> >>> if (ret < 0) {
> >>>
> >>> adv_err(state,
> >>>
> >>> "Error regs page: 0x%02x reg: 0x%02x\n",
> >>>
> >>> @@ -233,14 +243,18 @@ static int adv748x_write_regs(struct adv748x_state
> >>> *state,>>>
> >>> static const struct adv748x_reg_value adv748x_power_up_txa_4lane[] = {
> >>>
> >>> - {ADV748X_PAGE_TXA, 0x00, 0x84}, /* Enable 4-lane MIPI */
> >>> - {ADV748X_PAGE_TXA, 0x00, 0xa4}, /* Set Auto DPHY Timing */
> >>> + /* NOTE: NUM_LANES[2:0] in TXA register 0x00 is injected on write.
*/
> >>> + {ADV748X_PAGE_TXA, 0x00, 0x80}, /* Enable n-lane MIPI */
> >>> + {ADV748X_PAGE_TXA, 0x00, 0xa0}, /* Set Auto DPHY Timing */
> >>>
> >>> {ADV748X_PAGE_TXA, 0x31, 0x82}, /* ADI Required Write */
> >>> {ADV748X_PAGE_TXA, 0x1e, 0x40}, /* ADI Required Write */
> >>> {ADV748X_PAGE_TXA, 0xda, 0x01}, /* i2c_mipi_pll_en - 1'b1 */
> >>> {ADV748X_PAGE_WAIT, 0x00, 0x02},/* delay 2 */
> >>>
> >>> - {ADV748X_PAGE_TXA, 0x00, 0x24 },/* Power-up CSI-TX */
> >>> +
> >>> + /* NOTE: NUM_LANES[2:0] in TXA register 0x00 is injected on write.
*/
> >>> + {ADV748X_PAGE_TXA, 0x00, 0x20 },/* Power-up CSI-TX */
> >>> +
> >>>
> >>> {ADV748X_PAGE_WAIT, 0x00, 0x01},/* delay 1 */
> >>> {ADV748X_PAGE_TXA, 0xc1, 0x2b}, /* ADI Required Write */
> >>> {ADV748X_PAGE_WAIT, 0x00, 0x01},/* delay 1 */
> >>>
> >>> @@ -253,7 +267,10 @@ static const struct adv748x_reg_value
> >>> adv748x_power_down_txa_4lane[] = {>>>
> >>> {ADV748X_PAGE_TXA, 0x31, 0x82}, /* ADI Required Write */
> >>> {ADV748X_PAGE_TXA, 0x1e, 0x00}, /* ADI Required Write */
> >>>
> >>> - {ADV748X_PAGE_TXA, 0x00, 0x84}, /* Enable 4-lane MIPI */
> >>> +
> >>> + /* NOTE: NUM_LANES[2:0] in TXA register 0x00 is injected on write.
*/
> >>> + {ADV748X_PAGE_TXA, 0x00, 0x80}, /* Enable n-lane MIPI */
> >>
> >> If we're in power down - shouldn't this be "Disable n-lane MIPI */ ??
> >
> > Well the register write enables the lanes. IIRC the comments here come
> > from the register tables text files found in the "documentation".
>
> Isn't the documentation fabulous :-)
>
> "Write these values, don't ask any questions..."
>
> >>> +
> >>>
> >>> {ADV748X_PAGE_TXA, 0xda, 0x01}, /* i2c_mipi_pll_en - 1'b1 */
> >>> {ADV748X_PAGE_TXA, 0xc1, 0x3b}, /* ADI Required Write */
> >>>
> >>> @@ -399,8 +416,10 @@ static const struct adv748x_reg_value
> >>> adv748x_init_txa_4lane[] = {>>>
> >>> /* Outputs Enabled */
> >>> {ADV748X_PAGE_IO, 0x10, 0xa0}, /* Enable 4-lane CSI Tx & Pixel Port
*/
> >>>
> >>> - {ADV748X_PAGE_TXA, 0x00, 0x84}, /* Enable 4-lane MIPI */
> >>> - {ADV748X_PAGE_TXA, 0x00, 0xa4}, /* Set Auto DPHY Timing */
> >>> + /* NOTE: NUM_LANES[2:0] in TXA register 0x00 is injected on write.
*/
> >>> + {ADV748X_PAGE_TXA, 0x00, 0x80}, /* Enable n-lane MIPI */
> >>> + {ADV748X_PAGE_TXA, 0x00, 0xa0}, /* Set Auto DPHY Timing */
> >>> +
> >>>
> >>> {ADV748X_PAGE_TXA, 0xdb, 0x10}, /* ADI Required Write */
> >>> {ADV748X_PAGE_TXA, 0xd6, 0x07}, /* ADI Required Write */
> >>> {ADV748X_PAGE_TXA, 0xc4, 0x0a}, /* ADI Required Write */
> >>>
> >>> @@ -412,7 +431,10 @@ static const struct adv748x_reg_value
> >>> adv748x_init_txa_4lane[] = {>>>
> >>> {ADV748X_PAGE_TXA, 0x1e, 0x40}, /* ADI Required Write */
> >>> {ADV748X_PAGE_TXA, 0xda, 0x01}, /* i2c_mipi_pll_en - 1'b1 */
> >>> {ADV748X_PAGE_WAIT, 0x00, 0x02},/* delay 2 */
> >>>
> >>> - {ADV748X_PAGE_TXA, 0x00, 0x24 },/* Power-up CSI-TX */
> >>> +
> >>> + /* NOTE: NUM_LANES[2:0] in TXA register 0x00 is injected on write.
*/
> >>> + {ADV748X_PAGE_TXA, 0x00, 0x20 },/* Power-up CSI-TX */
> >>> +
> >>>
> >>> {ADV748X_PAGE_WAIT, 0x00, 0x01},/* delay 1 */
> >>> {ADV748X_PAGE_TXA, 0xc1, 0x2b}, /* ADI Required Write */
> >>> {ADV748X_PAGE_WAIT, 0x00, 0x01},/* delay 1 */
--
Regards,
Laurent Pinchart
