| Message ID | 20230616135922.442979-19-tomi.valkeinen@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | i2c-atr and FPDLink | expand |
On Fri, Jun 16, 2023 at 04:59:22PM +0300, Tomi Valkeinen wrote: > Add support for FPD-Link non-sync mode with external clock. The only > thing that needs to be added is the calculation for the clkout. ... > + switch (priv->mode) { > + case UB953_MODE_SYNC: > + if (priv->hw_data->is_ub971) > + return priv->plat_data->bc_rate * 160ull; > + else > + return priv->plat_data->bc_rate / 2 * 160ull; Redundant 'else'. Do I understand correctly you don't want to fallthrough because it will give ±160 in the rate (depending if it's even or odd)? > + case UB953_MODE_NONSYNC_EXT: > + /* CLKIN_DIV = 1 always */ > + return clk_get_rate(priv->clkin) * 80ull; > + > + default: > /* Not supported */ > return 0; > }
On 16/06/2023 17:47, Andy Shevchenko wrote: > On Fri, Jun 16, 2023 at 04:59:22PM +0300, Tomi Valkeinen wrote: >> Add support for FPD-Link non-sync mode with external clock. The only >> thing that needs to be added is the calculation for the clkout. > > ... > >> + switch (priv->mode) { >> + case UB953_MODE_SYNC: >> + if (priv->hw_data->is_ub971) >> + return priv->plat_data->bc_rate * 160ull; >> + else >> + return priv->plat_data->bc_rate / 2 * 160ull; > > Redundant 'else'. True, but I like the symmetry in: if (foo) return 123; else return 321; > Do I understand correctly you don't want to fallthrough because it will give > ±160 in the rate (depending if it's even or odd)? Sorry, can you clarify? Fallthrough to what? >> + case UB953_MODE_NONSYNC_EXT: >> + /* CLKIN_DIV = 1 always */ >> + return clk_get_rate(priv->clkin) * 80ull; >> + >> + default: >> /* Not supported */ >> return 0; >> } >
On Mon, Jun 19, 2023 at 12:00:57PM +0300, Tomi Valkeinen wrote: > On 16/06/2023 17:47, Andy Shevchenko wrote: > > On Fri, Jun 16, 2023 at 04:59:22PM +0300, Tomi Valkeinen wrote: > > > Add support for FPD-Link non-sync mode with external clock. The only > > > thing that needs to be added is the calculation for the clkout. ... > > > + switch (priv->mode) { > > > + case UB953_MODE_SYNC: > > > + if (priv->hw_data->is_ub971) > > > + return priv->plat_data->bc_rate * 160ull; > > > + else > > > + return priv->plat_data->bc_rate / 2 * 160ull; > > > > Redundant 'else'. > > True, but I like the symmetry in: > > if (foo) > return 123; > else > return 321; At the same time it will be symmetry with other switch-case(s). That's why the question about fallthrough below. > > Do I understand correctly you don't want to fallthrough because it will give > > ±160 in the rate (depending if it's even or odd)? > > Sorry, can you clarify? Fallthrough to what? To the below case since '/ 2 * 160 ~= *80'. Why ~ because it might give off-by-one error due to even/odd input. > > > + case UB953_MODE_NONSYNC_EXT: > > > + /* CLKIN_DIV = 1 always */ > > > + return clk_get_rate(priv->clkin) * 80ull;
On 19/06/2023 13:48, Andy Shevchenko wrote: > On Mon, Jun 19, 2023 at 12:00:57PM +0300, Tomi Valkeinen wrote: >> On 16/06/2023 17:47, Andy Shevchenko wrote: >>> On Fri, Jun 16, 2023 at 04:59:22PM +0300, Tomi Valkeinen wrote: >>>> Add support for FPD-Link non-sync mode with external clock. The only >>>> thing that needs to be added is the calculation for the clkout. > > ... > >>>> + switch (priv->mode) { >>>> + case UB953_MODE_SYNC: >>>> + if (priv->hw_data->is_ub971) >>>> + return priv->plat_data->bc_rate * 160ull; >>>> + else >>>> + return priv->plat_data->bc_rate / 2 * 160ull; >>> >>> Redundant 'else'. >> >> True, but I like the symmetry in: >> >> if (foo) >> return 123; >> else >> return 321; > > At the same time it will be symmetry with other switch-case(s). That's why the > question about fallthrough below. > >>> Do I understand correctly you don't want to fallthrough because it will give >>> ±160 in the rate (depending if it's even or odd)? >> >> Sorry, can you clarify? Fallthrough to what? > > To the below case since '/ 2 * 160 ~= *80'. Why ~ because it might give > off-by-one error due to even/odd input. The below case is different. "priv->plat_data->bc_rate" vs "clk_get_rate(priv->clkin)". As to the order of the calculation (/ 2 * 160 versus * 160 / 2), generally speaking, I have never figured out what are the correct ways to calculate clock rates. I wrote "x / 2 * 160" as that's what the documentation gives (there's a hardware /2 divider in non-ub971 chips, followed by a 160 multiplier). But does the documentation presume that the calculation is done precisely, not in integers? If so, "x * 160 / 2" would be better (but then, do we need to round?). Or does the /2 hardware divider basically actually work as a an integer division, in case "x / 2 * 160" is the correct one. >>>> + case UB953_MODE_NONSYNC_EXT: >>>> + /* CLKIN_DIV = 1 always */ >>>> + return clk_get_rate(priv->clkin) * 80ull; > Tomi
diff --git a/drivers/media/i2c/ds90ub953.c b/drivers/media/i2c/ds90ub953.c index ff55740965eb..ae90a647489a 100644 --- a/drivers/media/i2c/ds90ub953.c +++ b/drivers/media/i2c/ds90ub953.c @@ -143,6 +143,7 @@ struct ub953_data { struct i2c_client *client; struct regmap *regmap; + struct clk *clkin; u32 num_data_lanes; bool non_cont_clk; @@ -842,15 +843,21 @@ static int ub953_i2c_master_init(struct ub953_data *priv) static u64 ub953_get_fc_rate(struct ub953_data *priv) { - if (priv->mode != UB953_MODE_SYNC) { + switch (priv->mode) { + case UB953_MODE_SYNC: + if (priv->hw_data->is_ub971) + return priv->plat_data->bc_rate * 160ull; + else + return priv->plat_data->bc_rate / 2 * 160ull; + + case UB953_MODE_NONSYNC_EXT: + /* CLKIN_DIV = 1 always */ + return clk_get_rate(priv->clkin) * 80ull; + + default: /* Not supported */ return 0; } - - if (priv->hw_data->is_ub971) - return priv->plat_data->bc_rate * 160ull; - else - return priv->plat_data->bc_rate / 2 * 160ull; } static unsigned long ub953_calc_clkout_ub953(struct ub953_data *priv, @@ -1189,9 +1196,15 @@ static int ub953_hw_init(struct ub953_data *priv) dev_dbg(dev, "mode from %s: %#x\n", mode_override ? "reg" : "strap", priv->mode); - if (priv->mode != UB953_MODE_SYNC) + if (priv->mode != UB953_MODE_SYNC && + priv->mode != UB953_MODE_NONSYNC_EXT) return dev_err_probe(dev, -ENODEV, - "Only synchronous mode supported\n"); + "Unsupported mode selected: %d\n", + priv->mode); + + if (priv->mode == UB953_MODE_NONSYNC_EXT && !priv->clkin) + return dev_err_probe(dev, -EINVAL, + "clkin required for non-sync ext mode\n"); ret = ub953_read(priv, UB953_REG_REV_MASK_ID, &v); if (ret) @@ -1319,6 +1332,11 @@ static int ub953_probe(struct i2c_client *client) goto err_mutex_destroy; } + priv->clkin = devm_clk_get_optional(dev, "clkin"); + if (IS_ERR(priv->clkin)) + return dev_err_probe(dev, PTR_ERR(priv->clkin), + "failed to parse 'clkin'\n"); + ret = ub953_parse_dt(priv); if (ret) goto err_mutex_destroy;
Add support for FPD-Link non-sync mode with external clock. The only thing that needs to be added is the calculation for the clkout. Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> --- drivers/media/i2c/ds90ub953.c | 34 ++++++++++++++++++++++++++-------- 1 file changed, 26 insertions(+), 8 deletions(-)