Hi Niklas,
On Tuesday, 18 September 2018 22:29:32 EEST 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;
>
> So the overview of what happens become much harder to read.
Error checking may indeed make the code more difficult to read. One option
would be, in pseudo-code, something like the following.
struct adv748x_state {
int write_error;
};
static int adv748x_error_check(struct adv748x_state *state)
{
int error = state->write_error;
state->write_error = 0;
return error;
}
static int adv748x_write(struct adv748x_state *state, ...)
{
int ret;
if (state->write_error)
return write_error;
ret = write(...);
state->write_error = ret;
return ret;
}
...
adv748x_write(state, ...);
adv748x_write(state, ...);
adv748x_write(state, ...);
adv748x_write(state, ...);
ret = adv748x_error_check(state);
if (ret)
return ret;
...
> 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.
I would at least try reordering writes where we think it would make sense.
> 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
> adv748x_init_txa_4lane: ~50
>
> > > 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".
>
> > > +
> > >
> > > {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
