| Message ID | 20240830070008.9486-2-eagle.alexander923@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [1/3] media: i2c: ds90ub960: Convert IC specific variables to flags | expand |
Hi, On 30/08/2024 10:00, Alexander Shiyan wrote: > Add support for TI DS90UB954 FPD-Link III Deserializer. > > Signed-off-by: Alexander Shiyan <eagle.alexander923@gmail.com> > --- > drivers/media/i2c/Kconfig | 2 +- > drivers/media/i2c/ds90ub960.c | 52 +++++++++++++++++++++++++++++------ > 2 files changed, 44 insertions(+), 10 deletions(-) > > diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig > index 8ba096b8ebca..18766898280b 100644 > --- a/drivers/media/i2c/Kconfig > +++ b/drivers/media/i2c/Kconfig > @@ -1604,7 +1604,7 @@ config VIDEO_DS90UB960 > select V4L2_FWNODE > select VIDEO_V4L2_SUBDEV_API > help > - Device driver for the Texas Instruments DS90UB960 > + Device driver for the Texas Instruments DS90UB954/DS90UB960 > FPD-Link III Deserializer and DS90UB9702 FPD-Link IV Deserializer. > > config VIDEO_MAX96714 > diff --git a/drivers/media/i2c/ds90ub960.c b/drivers/media/i2c/ds90ub960.c > index e9f9abf439ee..9edc7e8ceebd 100644 > --- a/drivers/media/i2c/ds90ub960.c > +++ b/drivers/media/i2c/ds90ub960.c > @@ -403,6 +403,7 @@ > #define UB960_NUM_EQ_LEVELS (UB960_MAX_EQ_LEVEL - UB960_MIN_EQ_LEVEL + 1) > > enum chip_type { > + UB954, > UB960, > UB9702, > }; > @@ -1154,10 +1155,17 @@ static int ub960_parse_dt_txport(struct ub960_data *priv, > priv->tx_link_freq[0] = vep.link_frequencies[0]; > priv->tx_data_rate = priv->tx_link_freq[0] * 2; > > - if (priv->tx_data_rate != MHZ(1600) && > - priv->tx_data_rate != MHZ(1200) && > - priv->tx_data_rate != MHZ(800) && > - priv->tx_data_rate != MHZ(400)) { > + switch (priv->tx_data_rate) { > + case MHZ(1600): > + case MHZ(800): > + case MHZ(400): > + break; > + case MHZ(1200): > + /* UB954 does not support 1.2 Gbps */ > + if (priv->hw_data->chip_type != UB954) > + break; Here, and in a few other places, don't check for model != UB954, but rather check for model == UB954. Otherwise if we add a new chip model these won't necessarily go right. > + fallthrough; > + default: > dev_err(dev, "tx%u: invalid 'link-frequencies' value\n", nport); > ret = -EINVAL; > goto err_free_vep; > @@ -1419,7 +1427,7 @@ static void ub960_rxport_config_eq(struct ub960_data *priv, unsigned int nport) > > if (priv->strobe.manual) > ub960_rxport_set_strobe_pos(priv, nport, rxport->eq.strobe_pos); > - else > + else if (priv->hw_data->chip_type != UB954) > ub960_rxport_set_strobe_pos(priv, nport, 0); This looks odd. Manually set strobe pos is ok, but not the default? What is the reason for this if? > > if (rxport->eq.manual_eq) { > @@ -3807,7 +3815,7 @@ static int ub960_enable_core_hw(struct ub960_data *priv) > u8 rev_mask; > int ret; > u8 dev_sts; > - u8 refclk_freq; > + u8 refclk_freq[2]; Instead of an array, I think the code will be clearer if you just add a new variable (refclk_freq_new?). > > ret = regulator_enable(priv->vddio); > if (ret) > @@ -3839,6 +3847,9 @@ static int ub960_enable_core_hw(struct ub960_data *priv) > } > > switch (priv->hw_data->chip_type) { > + case UB954: > + model = "UB954"; > + break; > case UB960: > model = "UB960"; > break; > @@ -3856,12 +3867,26 @@ static int ub960_enable_core_hw(struct ub960_data *priv) > if (ret) > goto err_pd_gpio; > > - ret = ub960_read(priv, UB960_XR_REFCLK_FREQ, &refclk_freq); > + ret = ub960_read(priv, UB960_XR_REFCLK_FREQ, &refclk_freq[0]); > if (ret) > goto err_pd_gpio; > > + /* From DS90UB954-Q1 datasheet: > + * "REFCLK_FREQ measurement is not synchronized. Value in this register > + * should read twice and only considered valid if > + * REFCLK_FREQ is unchanged between reads." > + */ The coding style says the multiline comments are like: /* * Foo */ > + while (priv->hw_data->chip_type == UB954) { > + ret = ub960_read(priv, UB960_XR_REFCLK_FREQ, &refclk_freq[1]); > + if (ret) > + goto err_pd_gpio; > + if (refclk_freq[0] == refclk_freq[1]) > + break; > + refclk_freq[0] = refclk_freq[1]; > + }; This is potentially an infinite loop, which is not a good idea. Also, don't loop with "while (priv->hw_data->chip_type == UB954)"... Just use an if for the chip_type, and loop with a proper condition. > + > dev_dbg(dev, "refclk valid %u freq %u MHz (clk fw freq %lu MHz)\n", > - !!(dev_sts & BIT(4)), refclk_freq, > + !!(dev_sts & BIT(4)), refclk_freq[0], > clk_get_rate(priv->refclk) / 1000000); > > /* Disable all RX ports by default */ > @@ -3923,7 +3948,8 @@ static int ub960_probe(struct i2c_client *client) > */ > priv->reg_current.indirect_target = 0xff; > priv->reg_current.rxport = 0xff; > - priv->reg_current.txport = 0xff; > + /* Avoid using UB960_SR_CSI_PORT_SEL register for single TX channel */ > + priv->reg_current.txport = priv->hw_data->num_txports > 1 ? 0xff : 0x00; No, don't do this. Just do a proper check in ub960_txport_select() and skip the reg write there. Tomi > ret = ub960_get_hw_resources(priv); > if (ret) > @@ -4031,6 +4057,12 @@ static void ub960_remove(struct i2c_client *client) > mutex_destroy(&priv->reg_lock); > } > > +static const struct ub960_hw_data ds90ub954_hw = { > + .chip_type = UB954, > + .num_rxports = 2, > + .num_txports = 1, > +}; > + > static const struct ub960_hw_data ds90ub960_hw = { > .chip_type = UB960, > .num_rxports = 4, > @@ -4045,6 +4077,7 @@ static const struct ub960_hw_data ds90ub9702_hw = { > }; > > static const struct i2c_device_id ub960_id[] = { > + { "ds90ub954-q1", (kernel_ulong_t)&ds90ub954_hw }, > { "ds90ub960-q1", (kernel_ulong_t)&ds90ub960_hw }, > { "ds90ub9702-q1", (kernel_ulong_t)&ds90ub9702_hw }, > {} > @@ -4052,6 +4085,7 @@ static const struct i2c_device_id ub960_id[] = { > MODULE_DEVICE_TABLE(i2c, ub960_id); > > static const struct of_device_id ub960_dt_ids[] = { > + { .compatible = "ti,ds90ub954-q1", .data = &ds90ub954_hw }, > { .compatible = "ti,ds90ub960-q1", .data = &ds90ub960_hw }, > { .compatible = "ti,ds90ub9702-q1", .data = &ds90ub9702_hw }, > {}
Hello. вт, 10 сент. 2024 г. в 11:40, Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>: > On 30/08/2024 10:00, Alexander Shiyan wrote: > > Add support for TI DS90UB954 FPD-Link III Deserializer. ... > > @@ -1419,7 +1427,7 @@ static void ub960_rxport_config_eq(struct ub960_data *priv, unsigned int nport) > > > > if (priv->strobe.manual) > > ub960_rxport_set_strobe_pos(priv, nport, rxport->eq.strobe_pos); > > - else > > + else if (priv->hw_data->chip_type != UB954) > > ub960_rxport_set_strobe_pos(priv, nport, 0); > > This looks odd. Manually set strobe pos is ok, but not the default? > What is the reason for this if? In fact, these registers are described as reserved in the datasheet. (We are speaking about indirect page 1). Here is an excerpt from datasheet UB960: Indirect Access Register Select: Selects target for register access 0000: Pattern Generator and CSI-2 Timing (PATGEN_AND_CSI-2) Registers xxxx: RESERVED In UB954 datasheet this area is described as "FPD-Link III Channel 0 Reserved Registers: Test and Debug registers". I tested the UB954 and when writing to this area an error occurs and the chip no longer responds. When disabling ub960_rxport_set_strobe_pos() everything works as expected.
Hi, On 11/09/2024 09:15, Alexander Shiyan wrote: > Hello. > > вт, 10 сент. 2024 г. в 11:40, Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>: >> On 30/08/2024 10:00, Alexander Shiyan wrote: >>> Add support for TI DS90UB954 FPD-Link III Deserializer. > ... >>> @@ -1419,7 +1427,7 @@ static void ub960_rxport_config_eq(struct ub960_data *priv, unsigned int nport) >>> >>> if (priv->strobe.manual) >>> ub960_rxport_set_strobe_pos(priv, nport, rxport->eq.strobe_pos); >>> - else >>> + else if (priv->hw_data->chip_type != UB954) >>> ub960_rxport_set_strobe_pos(priv, nport, 0); >> >> This looks odd. Manually set strobe pos is ok, but not the default? >> What is the reason for this if? > > In fact, these registers are described as reserved in the datasheet. Yes, but my point was that in your patch you disable the call to ub960_rxport_set_strobe_pos() if manual strobe is not set, but still allow it if manual strobe is set. > (We are speaking about indirect page 1). > Here is an excerpt from datasheet UB960: > Indirect Access Register Select: > Selects target for register access > 0000: Pattern Generator and CSI-2 Timing (PATGEN_AND_CSI-2) Registers > xxxx: RESERVED > > In UB954 datasheet this area is described as "FPD-Link III Channel 0 > Reserved Registers: Test and Debug registers". They're marked reserved in the UB960 datasheet too. > I tested the UB954 and when writing to this area an error occurs and > the chip no longer responds. > When disabling ub960_rxport_set_strobe_pos() everything works as expected. "Margin Analysis Program (MAP) and strobe positions for DS90UB954-Q1 and DS90UB960-Q1" describes the registers: https://www.ti.com/lit/pdf/snla301 It's been a while since I worked on this, but I remember having some trouble fitting the docs to what actually worked. And there was some small diff between UB954 and UB960, if I'm not mistaken. You might be able to sort it out with the doc above, but if not or if you're not interested, I'm fine with marking the relevant code for UB960 only. But be careful to disable the code in all the code paths. Tomi
diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig index 8ba096b8ebca..18766898280b 100644 --- a/drivers/media/i2c/Kconfig +++ b/drivers/media/i2c/Kconfig @@ -1604,7 +1604,7 @@ config VIDEO_DS90UB960 select V4L2_FWNODE select VIDEO_V4L2_SUBDEV_API help - Device driver for the Texas Instruments DS90UB960 + Device driver for the Texas Instruments DS90UB954/DS90UB960 FPD-Link III Deserializer and DS90UB9702 FPD-Link IV Deserializer. config VIDEO_MAX96714 diff --git a/drivers/media/i2c/ds90ub960.c b/drivers/media/i2c/ds90ub960.c index e9f9abf439ee..9edc7e8ceebd 100644 --- a/drivers/media/i2c/ds90ub960.c +++ b/drivers/media/i2c/ds90ub960.c @@ -403,6 +403,7 @@ #define UB960_NUM_EQ_LEVELS (UB960_MAX_EQ_LEVEL - UB960_MIN_EQ_LEVEL + 1) enum chip_type { + UB954, UB960, UB9702, }; @@ -1154,10 +1155,17 @@ static int ub960_parse_dt_txport(struct ub960_data *priv, priv->tx_link_freq[0] = vep.link_frequencies[0]; priv->tx_data_rate = priv->tx_link_freq[0] * 2; - if (priv->tx_data_rate != MHZ(1600) && - priv->tx_data_rate != MHZ(1200) && - priv->tx_data_rate != MHZ(800) && - priv->tx_data_rate != MHZ(400)) { + switch (priv->tx_data_rate) { + case MHZ(1600): + case MHZ(800): + case MHZ(400): + break; + case MHZ(1200): + /* UB954 does not support 1.2 Gbps */ + if (priv->hw_data->chip_type != UB954) + break; + fallthrough; + default: dev_err(dev, "tx%u: invalid 'link-frequencies' value\n", nport); ret = -EINVAL; goto err_free_vep; @@ -1419,7 +1427,7 @@ static void ub960_rxport_config_eq(struct ub960_data *priv, unsigned int nport) if (priv->strobe.manual) ub960_rxport_set_strobe_pos(priv, nport, rxport->eq.strobe_pos); - else + else if (priv->hw_data->chip_type != UB954) ub960_rxport_set_strobe_pos(priv, nport, 0); if (rxport->eq.manual_eq) { @@ -3807,7 +3815,7 @@ static int ub960_enable_core_hw(struct ub960_data *priv) u8 rev_mask; int ret; u8 dev_sts; - u8 refclk_freq; + u8 refclk_freq[2]; ret = regulator_enable(priv->vddio); if (ret) @@ -3839,6 +3847,9 @@ static int ub960_enable_core_hw(struct ub960_data *priv) } switch (priv->hw_data->chip_type) { + case UB954: + model = "UB954"; + break; case UB960: model = "UB960"; break; @@ -3856,12 +3867,26 @@ static int ub960_enable_core_hw(struct ub960_data *priv) if (ret) goto err_pd_gpio; - ret = ub960_read(priv, UB960_XR_REFCLK_FREQ, &refclk_freq); + ret = ub960_read(priv, UB960_XR_REFCLK_FREQ, &refclk_freq[0]); if (ret) goto err_pd_gpio; + /* From DS90UB954-Q1 datasheet: + * "REFCLK_FREQ measurement is not synchronized. Value in this register + * should read twice and only considered valid if + * REFCLK_FREQ is unchanged between reads." + */ + while (priv->hw_data->chip_type == UB954) { + ret = ub960_read(priv, UB960_XR_REFCLK_FREQ, &refclk_freq[1]); + if (ret) + goto err_pd_gpio; + if (refclk_freq[0] == refclk_freq[1]) + break; + refclk_freq[0] = refclk_freq[1]; + }; + dev_dbg(dev, "refclk valid %u freq %u MHz (clk fw freq %lu MHz)\n", - !!(dev_sts & BIT(4)), refclk_freq, + !!(dev_sts & BIT(4)), refclk_freq[0], clk_get_rate(priv->refclk) / 1000000); /* Disable all RX ports by default */ @@ -3923,7 +3948,8 @@ static int ub960_probe(struct i2c_client *client) */ priv->reg_current.indirect_target = 0xff; priv->reg_current.rxport = 0xff; - priv->reg_current.txport = 0xff; + /* Avoid using UB960_SR_CSI_PORT_SEL register for single TX channel */ + priv->reg_current.txport = priv->hw_data->num_txports > 1 ? 0xff : 0x00; ret = ub960_get_hw_resources(priv); if (ret) @@ -4031,6 +4057,12 @@ static void ub960_remove(struct i2c_client *client) mutex_destroy(&priv->reg_lock); } +static const struct ub960_hw_data ds90ub954_hw = { + .chip_type = UB954, + .num_rxports = 2, + .num_txports = 1, +}; + static const struct ub960_hw_data ds90ub960_hw = { .chip_type = UB960, .num_rxports = 4, @@ -4045,6 +4077,7 @@ static const struct ub960_hw_data ds90ub9702_hw = { }; static const struct i2c_device_id ub960_id[] = { + { "ds90ub954-q1", (kernel_ulong_t)&ds90ub954_hw }, { "ds90ub960-q1", (kernel_ulong_t)&ds90ub960_hw }, { "ds90ub9702-q1", (kernel_ulong_t)&ds90ub9702_hw }, {} @@ -4052,6 +4085,7 @@ static const struct i2c_device_id ub960_id[] = { MODULE_DEVICE_TABLE(i2c, ub960_id); static const struct of_device_id ub960_dt_ids[] = { + { .compatible = "ti,ds90ub954-q1", .data = &ds90ub954_hw }, { .compatible = "ti,ds90ub960-q1", .data = &ds90ub960_hw }, { .compatible = "ti,ds90ub9702-q1", .data = &ds90ub9702_hw }, {}
Add support for TI DS90UB954 FPD-Link III Deserializer. Signed-off-by: Alexander Shiyan <eagle.alexander923@gmail.com> --- drivers/media/i2c/Kconfig | 2 +- drivers/media/i2c/ds90ub960.c | 52 +++++++++++++++++++++++++++++------ 2 files changed, 44 insertions(+), 10 deletions(-)