Re: [PATCH 2/5] media: i2c: max2175: Add MAX2175 support

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Antti,

On Friday 11 Nov 2016 09:16:04 Antti Palosaari wrote:
> Hello
> 
> On 11/09/2016 05:44 PM, Ramesh Shanmugasundaram wrote:
> > +static int max2175_set_lo_freq(struct max2175 *ctx, u64 lo_freq)
> > +{
> > +	u64 scaled_lo_freq, scaled_npf, scaled_integer, scaled_fraction;
> > +	u32 frac_desired, int_desired, lo_mult = 1;
> > +	const u32 scale_factor = 1000000U;
> > +	u8 loband_bits = 0, vcodiv_bits = 0;
> > +	enum max2175_band band;
> > +	int ret;
> > +
> > +	/* Scale to larger number for precision */
> > +	scaled_lo_freq = lo_freq * scale_factor * 100;
> > +	band = max2175_read_bits(ctx, 5, 1, 0);
> > +
> > +	mxm_dbg(ctx, "set_lo_freq: scaled lo_freq %llu lo_freq %llu band 
%d\n",
> > +		scaled_lo_freq, lo_freq, band);
> > +
> > +	switch (band) {
> > +	case MAX2175_BAND_AM:
> > +		if (max2175_read_bit(ctx, 5, 7) == 0)
> > +			lo_mult = 16;
> 
> else is lo_mult = 1. No idea if it is correct, but sounds very small
> output divider for low freq like am band. And on the other-hand local
> oscillator output divider, which I expect this to be, is usually 2 or more.
> 
> > +		break;
> > +	case MAX2175_BAND_FM:
> > +		if (lo_freq <= 74700000) {
> > +			lo_mult = 16;
> 
> No meaning as you set it later 8.
> 
> > +		} else if (lo_freq > 74700000 && lo_freq <= 110000000) {
> > +			loband_bits = 1;
> > +		} else {
> > +			loband_bits = 1;
> > +			vcodiv_bits = 3;
> > +		}
> > +		lo_mult = 8;
> > +		break;
> > +	case MAX2175_BAND_VHF:
> > +		if (lo_freq <= 210000000) {
> > +			loband_bits = 2;
> > +			vcodiv_bits = 2;
> > +		} else {
> > +			loband_bits = 2;
> > +			vcodiv_bits = 1;
> > +		}
> > +		lo_mult = 4;
> > +		break;
> > +	default:
> > +		loband_bits = 3;
> > +		vcodiv_bits = 2;
> > +		lo_mult = 2;
> > +		break;
> > +	}
> > +
> > +	if (band == MAX2175_BAND_L)
> > +		scaled_npf = div_u64(div_u64(scaled_lo_freq, ctx->xtal_freq),
> > +				     lo_mult);
> > +	else
> > +		scaled_npf = div_u64(scaled_lo_freq, ctx->xtal_freq) * 
lo_mult;
> > +
> > +	scaled_npf = div_u64(scaled_npf, 100);
> > +	scaled_integer = div_u64(scaled_npf, scale_factor) * scale_factor;
> > +	int_desired = div_u64(scaled_npf, scale_factor);
> > +	scaled_fraction = scaled_npf - scaled_integer;
> > +	frac_desired = div_u64(scaled_fraction << 20, scale_factor);
> > +
> > +	/* Check CSM is not busy */
> > +	ret = max2175_poll_csm_ready(ctx);
> > +	if (ret)
> > +		return ret;
> > +
> > +	mxm_dbg(ctx, "loband %u vcodiv %u lo_mult %u scaled_npf %llu\n",
> > +		loband_bits, vcodiv_bits, lo_mult, scaled_npf);
> > +	mxm_dbg(ctx, "scaled int %llu frac %llu desired int %u frac %u\n",
> > +		scaled_integer, scaled_fraction, int_desired, frac_desired);
> > +
> > +	/* Write the calculated values to the appropriate registers */
> > +	max2175_write(ctx, 1, int_desired);
> > +	max2175_write_bits(ctx, 2, 3, 0, (frac_desired >> 16) & 0xf);
> > +	max2175_write(ctx, 3, frac_desired >> 8);
> > +	max2175_write(ctx, 4, frac_desired);
> > +	max2175_write_bits(ctx, 5, 3, 2, loband_bits);
> > +	max2175_write_bits(ctx, 6, 7, 6, vcodiv_bits);
> > +	return ret;
> > +}
> 
> That synthesizer config is hard to understand. It seems to be
> fractional-N, with configurable N, K and output divider - like a school
> book example.
> 
>                +----------------------------+
>                v                            |
>       Fref   +----+     +-------+         +------+
>      ------> | PD | --> |  VCO  | ------> | /N.F |
>              +----+     +-------+         +------+
>                           |
>                           |
>                           v
>                         +-------+  Fout
>                         | /Rout | ------>
>                         +-------+
>
> I made following look-up table in order to understand it:
> 
> band      lo freq band vcodiv div_out
>    AM  <  50000000    0      0      16 // reg 5 bit 7 ?
>    FM  <  74700000    0      0      16
>    FM  < 110000000    1      0       8
>    FM  < 160000000    1      3       8
>   VHF  < 210000000    2      2       4
>   VHF  < 600000000    2      1       4
>     L  <2000000000    3      2       2
> 
> "vcodiv" looks unrelated to synth calculation, dunno what it is.
> 
> One which makes calculation very complex looking is that it is based of
> floating point calculus. On integer mathematics you should replace
> fractional part with fractional modulus (usually letter "K" is used for
> fractional modulus on PLL calc).
> 
> So that ends up something like:
> 1) select suitable lo output divider from desired output frequency
> 2) calculate vco frequency
> 3) convert vco frequency to N and K
> * N = Fvco/Fref
> * K = Fvco%Fref
> 4) convert K to control word (looks like << 20)
> 5) program values
> 
> Result should be calculus without scaling.

Thanks for reviewing this part.

I'd like to add that we already have two PLL helpers in the media subsystem, 
in drivers/media/i2c/aptina-pll.c and drivers/media/i2c/smiapp-pll.c. As the 
PLL used here seems to be a classic one, it would make sense to also extract 
the code in a helper function that could be shared between drivers.

-- 
Regards,

Laurent Pinchart