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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <6b225c10b6c71ffbc79c236b64dcc83fc33cc21b.camel@perches.com>
Date:   Thu, 03 Sep 2020 09:45:12 -0700
From:   Joe Perches <joe@...ches.com>
To:     Santiago Hormazabal <santiagohssl@...il.com>,
        linux-media@...r.kernel.org, devicetree@...r.kernel.org,
        Rob Herring <robh+dt@...nel.org>,
        Ezequiel Garcia <ezequiel@...labora.com>,
        Hans Verkuil <hverkuil@...all.nl>,
        Mauro Carvalho Chehab <mchehab@...nel.org>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/3] media: Add support for the AM/FM radio chip KT0913
 from KT Micro.

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);
}