Re: [PATCH 2/3] media: Add support for the AM/FM radio chip KT0913 from KT Micro.

[Joe Perches <joe@xxxxxxxxxxx>]

On Mon, 2020-08-31 at 19:06 -0300, Santiago Hormazabal wrote:
> This chip requires almost no support components and can used over I2C.
> The driver uses the I2C bus and exposes the controls as a V4L2 radio.
> Tested with a module that contains this chip (from SZZSJDZ.com,
> part number ZJ-801B, even tho the company seems defunct now), and an H2+
> AllWinner SoC running a kernel built off 07d999f of the media_tree.

Thanks.

style trivia:

[]
> diff --git a/drivers/media/radio/radio-kt0913.c b/drivers/media/radio/radio-kt0913.c
[]
> +static const struct reg_sequence kt0913_init_regs_to_defaults[] = {
> +	/* Standby disabled, volume 0dB */
> +	{ KT0913_REG_RXCFG, 0x881f },

These might be more legible on single lines,
ignoring the 80 column limits.

> +	/* FM Channel spacing = 50kHz, Right & Left unmuted */
> +	{ KT0913_REG_SEEK, 0x000b },

etc...

[]

> +static int __kt0913_set_fm_frequency(struct kt0913_device *radio,
> +				     unsigned int frequency)
> +{
> +	return regmap_write(radio->regmap, KT0913_REG_TUNE,
> +		KT0913_TUNE_FMTUNE_ON | (frequency / KT0913_FMCHAN_MUL));

It might be nicer to align multi-line statements to the
open parenthesis.

[]

> +static int __kt0913_set_au_gain(struct kt0913_device *radio, s32 gain)
> +{
> +	switch (gain) {
> +	case 6:
> +		return regmap_update_bits(radio->regmap,
> +			KT0913_REG_AMSYSCFG, KT0913_AMSYSCFG_AU_GAIN_MASK,
> +			KT0913_AMSYSCFG_AU_GAIN_6DB);
> +	case 3:
> +		return regmap_update_bits(radio->regmap,
> +			KT0913_REG_AMSYSCFG, KT0913_AMSYSCFG_AU_GAIN_MASK,
> +			KT0913_AMSYSCFG_AU_GAIN_3DB);
> +	case 0:
> +		return regmap_update_bits(radio->regmap,
> +			KT0913_REG_AMSYSCFG, KT0913_AMSYSCFG_AU_GAIN_MASK,
> +			KT0913_AMSYSCFG_AU_GAIN_0DB);
> +	case -3:
> +		return regmap_update_bits(radio->regmap,
> +			KT0913_REG_AMSYSCFG, KT0913_AMSYSCFG_AU_GAIN_MASK,
> +			KT0913_AMSYSCFG_AU_GAIN_MIN_3DB);
> +	default:
> +		return -EINVAL;
> +	}
> +}

It's generally more legible to write this with an intermediate
variable holding the changed value.  It's also most commonly
smaller object code.

static int __kt0913_set_au_gain(struct kt0913_device *radio, s32 gain)
{
	int val;

	switch (gain) {
	case 6:
		val = KT0913_AMSYSCFG_AU_GAIN_6DB;
		break;
	case 3:
		val = KT0913_AMSYSCFG_AU_GAIN_3DB;
		break;
	case 0:
		val = KT0913_AMSYSCFG_AU_GAIN_0DB;
		break;
	case -3:
		val = KT0913_AMSYSCFG_AU_GAIN_MIN_3DB;
		break;
	default:
		return -EINVAL;
	}

	return regmap_update_bits(radio->regmap, KT0913_REG_AMSYSCFG,
				  KT0913_AMSYSCFG_AU_GAIN_MASK, val);
}