Hi Ramesh,
Thank you for the patch.
On Wednesday 12 Oct 2016 15:10:25 Ramesh Shanmugasundaram wrote:
> This patch adds driver support for MAX2175 chip. This is Maxim
> Integrated's RF to Bits tuner front end chip designed for software-defined
> radio solutions. This driver exposes the tuner as a sub-device instance
> with standard and custom controls to configure the device.
>
> Signed-off-by: Ramesh Shanmugasundaram
> <ramesh.shanmugasundaram@xxxxxxxxxxxxxx> ---
> .../devicetree/bindings/media/i2c/max2175.txt | 60 +
> drivers/media/i2c/Kconfig | 4 +
> drivers/media/i2c/Makefile | 2 +
> drivers/media/i2c/max2175/Kconfig | 8 +
> drivers/media/i2c/max2175/Makefile | 4 +
> drivers/media/i2c/max2175/max2175.c | 1624 +++++++++++++++++
> drivers/media/i2c/max2175/max2175.h | 124 ++
> 7 files changed, 1826 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/media/i2c/max2175.txt
> create mode 100644 drivers/media/i2c/max2175/Kconfig
> create mode 100644 drivers/media/i2c/max2175/Makefile
> create mode 100644 drivers/media/i2c/max2175/max2175.c
> create mode 100644 drivers/media/i2c/max2175/max2175.h
>
> diff --git a/Documentation/devicetree/bindings/media/i2c/max2175.txt
> b/Documentation/devicetree/bindings/media/i2c/max2175.txt new file mode
> 100644
> index 0000000..2250d5f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/i2c/max2175.txt
> @@ -0,0 +1,60 @@
> +Maxim Integrated MAX2175 RF to Bits tuner
> +-----------------------------------------
> +
> +The MAX2175 IC is an advanced analog/digital hybrid-radio receiver with
> +RF to Bits® front-end designed for software-defined radio solutions.
> +
> +Required properties:
> +--------------------
> +- compatible: "maxim,max2175" for MAX2175 RF-to-bits tuner.
> +- clocks: phandle to the fixed xtal clock.
> +- clock-names: name of the fixed xtal clock.
> +- port: video interface child port node of a tuner that defines the local
As Rob pointed out in his review of patch 3/5, this isn't video.
> + and remote endpoints. The remote endpoint is assumed to be an SDR device
> + that is capable of receiving the digital samples from the tuner.
> +
> +Optional properties:
> +--------------------
> +- maxim,slave : empty property indicates this is a slave of
another
> + master tuner. This is used to define two tuners in
> + diversity mode (1 master, 1 slave). By default each
> + tuner is an individual master.
Would it be useful to make that property a phandle to the master tuner, to
give drivers a way to access the master ? I haven't checked whether there
could be use cases for that.
> +- maxim,refout-load: load capacitance value (in pF) on reference output
> + drive level. The mapping of these load values to
> + respective bit values are given below.
> + 0 - Reference output disabled
> + 1 - 10pF load
> + 2 - 20pF load
> + 3 - 30pF load
> + 4 - 40pF load
> + 5 - 60pF load
> + 6 - 70pF load
As Geert pointed out, you can simply specify the value in pF.
> +
> +Example:
> +--------
> +
> +Board specific DTS file
> +
> +/* Fixed XTAL clock node */
> +maxim_xtal: maximextal {
> + compatible = "fixed-clock";
> + #clock-cells = <0>;
> + clock-frequency = <36864000>;
> +};
> +
> +/* A tuner device instance under i2c bus */
> +max2175_0: tuner@60 {
> + #clock-cells = <0>;
Is the tuner a clock provider ? If it isn't you don't need this property.
> + compatible = "maxim,max2175";
> + reg = <0x60>;
> + clocks = <&maxim_xtal>;
> + clock-names = "xtal";
> + maxim,refout-load = <10>;
> +
> + port {
> + max2175_0_ep: endpoint {
> + remote-endpoint = <&slave_rx_v4l2_sdr_device>;
> + };
> + };
> +
> +};
[snip]
> diff --git a/drivers/media/i2c/max2175/Makefile
> b/drivers/media/i2c/max2175/Makefile new file mode 100644
> index 0000000..9bb46ac
> --- /dev/null
> +++ b/drivers/media/i2c/max2175/Makefile
> @@ -0,0 +1,4 @@
> +#
> +# Makefile for Maxim RF to Bits tuner device
> +#
> +obj-$(CONFIG_SDR_MAX2175) += max2175.o
If there's a single source file you might want to move it to
drivers/media/i2c/.
> diff --git a/drivers/media/i2c/max2175/max2175.c
> b/drivers/media/i2c/max2175/max2175.c new file mode 100644
> index 0000000..71b60c2
> --- /dev/null
> +++ b/drivers/media/i2c/max2175/max2175.c
> @@ -0,0 +1,1624 @@
> +/*
> + * Maxim Integrated MAX2175 RF to Bits tuner driver
> + *
> + * This driver & most of the hard coded values are based on the reference
> + * application delivered by Maxim for this chip.
> + *
> + * Copyright (C) 2016 Maxim Integrated Products
> + * Copyright (C) 2016 Renesas Electronics Corporation
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2
> + * as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/errno.h>
> +#include <linux/i2c.h>
> +#include <linux/init.h>
> +#include <linux/interrupt.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/slab.h>
> +#include <linux/delay.h>
You can move delay.h right below clk.h and everything will be in alphabetical
order :-)
> +#include <media/v4l2-ctrls.h>
> +#include <media/v4l2-device.h>
> +#include <media/v4l2-of.h>
> +
> +#include "max2175.h"
> +
> +#define DRIVER_NAME "max2175"
> +
> +static unsigned int max2175_debug;
> +module_param(max2175_debug, uint, 0644);
> +MODULE_PARM_DESC(max2175_debug, "activate debug info");
You can name the parameter "debug".
> +#define mxm_dbg(ctx, fmt, arg...) \
> + v4l2_dbg(1, max2175_debug, &ctx->sd, fmt, ## arg)
[snip]
> +/* Preset values:
> + * Based on Maxim MAX2175 Register Table revision: 130p10
> + */
The preferred multi-line comment style is
/*
* foo
* bar
*/
[snip]
> +struct max2175_ctx {
Nitpicking, such a structure would usually be named max2175 or max2175_device.
Context seems to imply that you can have multiple of them per device.
> + struct v4l2_subdev sd;
> + struct i2c_client *client;
> + struct device *dev;
> +
> + /* Cached configuration */
> + u8 regs[256];
If you want to cache register values you should use regmap.
> + enum max2175_modetag mode; /* Receive mode tag */
> + u32 freq; /* In Hz */
> + struct max2175_rxmode *rx_modes;
> +
> + /* Device settings */
> + bool master;
> + u32 decim_ratio;
> + u64 xtal_freq;
> +
> + /* ROM values */
> + u8 rom_bbf_bw_am;
> + u8 rom_bbf_bw_fm;
> + u8 rom_bbf_bw_dab;
> +
> + /* Local copy of old settings */
> + u8 i2s_test;
> +
> + u8 nbd_gain;
> + u8 nbd_threshold;
> + u8 wbd_gain;
> + u8 wbd_threshold;
> + u8 bbd_threshold;
> + u8 bbdclip_threshold;
> + u8 lt_wbd_threshold;
> + u8 lt_wbd_gain;
> +
> + /* Controls */
> + struct v4l2_ctrl_handler ctrl_hdl;
> + struct v4l2_ctrl *lna_gain; /* LNA gain value */
> + struct v4l2_ctrl *if_gain; /* I/F gain value */
> + struct v4l2_ctrl *pll_lock; /* PLL lock */
> + struct v4l2_ctrl *i2s_en; /* I2S output enable */
> + struct v4l2_ctrl *i2s_mode; /* I2S mode value */
> + struct v4l2_ctrl *am_hiz; /* AM High impledance input */
> + struct v4l2_ctrl *hsls; /* High-side/Low-side polarity */
> + struct v4l2_ctrl *rx_mode; /* Receive mode */
> +
> + /* Driver private variables */
> + bool mode_resolved; /* Flag to sanity check settings */
> +};
[snip]
> +/* Local store bitops helpers */
> +static u8 max2175_get_bits(struct max2175_ctx *ctx, u8 idx, u8 msb, u8 lsb)
> +{
> + if (max2175_debug >= 2)
> + mxm_dbg(ctx, "get_bits: idx:%u msb:%u lsb:%u\n",
> + idx, msb, lsb);
Do we really need such detailed debugging ?
> + return __max2175_get_bits(ctx->regs[idx], msb, lsb);
> +}
> +
> +static bool max2175_get_bit(struct max2175_ctx *ctx, u8 idx, u8 bit)
> +{
> + return !!max2175_get_bits(ctx, idx, bit, bit);
> +}
> +
> +static void max2175_set_bits(struct max2175_ctx *ctx, u8 idx,
> + u8 msb, u8 lsb, u8 newval)
> +{
> + if (max2175_debug >= 2)
> + mxm_dbg(ctx, "set_bits: idx:%u msb:%u lsb:%u newval:%u\n",
> + idx, msb, lsb, newval);
> + ctx->regs[idx] = __max2175_set_bits(ctx->regs[idx], msb, lsb,
> + newval);
> +}
> +
> +static void max2175_set_bit(struct max2175_ctx *ctx, u8 idx, u8 bit, u8
> newval)
> +{
> + max2175_set_bits(ctx, idx, bit, bit, newval);
> +}
> +
> +/* Device register bitops helpers */
> +static u8 max2175_read_bits(struct max2175_ctx *ctx, u8 idx, u8 msb, u8
> lsb)
> +{
> + return __max2175_get_bits(max2175_reg_read(ctx, idx), msb, lsb);
> +}
> +
> +static void max2175_write_bits(struct max2175_ctx *ctx, u8 idx, u8 msb,
> + u8 lsb, u8 newval)
> +{
> + /* Update local copy & write to device */
> + max2175_set_bits(ctx, idx, msb, lsb, newval);
> + max2175_reg_write(ctx, idx, ctx->regs[idx]);
> +}
> +
> +static void max2175_write_bit(struct max2175_ctx *ctx, u8 idx, u8 bit,
> + u8 newval)
> +{
> + if (max2175_debug >= 2)
> + mxm_dbg(ctx, "idx %u, bit %u, newval %u\n", idx, bit, newval);
> + max2175_write_bits(ctx, idx, bit, bit, newval);
> +}
Really, please use regmap :-)
> +static int max2175_poll_timeout(struct max2175_ctx *ctx, u8 idx, u8 msb, u8
> lsb,
> + u8 exp_val, u32 timeout)
> +{
> + unsigned long end = jiffies + msecs_to_jiffies(timeout);
> + int ret;
> +
> + do {
> + ret = max2175_read_bits(ctx, idx, msb, lsb);
> + if (ret < 0)
> + return ret;
> + if (ret == exp_val)
> + return 0;
> +
> + usleep_range(1000, 1500);
> + } while (time_is_after_jiffies(end));
> +
> + return -EBUSY;
> +}
> +
> +static int max2175_poll_csm_ready(struct max2175_ctx *ctx)
> +{
> + return max2175_poll_timeout(ctx, 69, 1, 1, 0, 50);
Please define macros for register addresses and values, this is just
unreadable.
> +}
[snip]
> +
> +#define MAX2175_IS_BAND_AM(ctx) \
> + (max2175_get_bits(ctx, 5, 1, 0) == MAX2175_BAND_AM)
> +
> +#define MAX2175_IS_FM_MODE(ctx) \
> + (max2175_get_bits(ctx, 12, 5, 4) == 0)
> +
> +#define MAX2175_IS_FMHD_MODE(ctx) \
> + (max2175_get_bits(ctx, 12, 5, 4) == 1)
> +
> +#define MAX2175_IS_DAB_MODE(ctx) \
> + (max2175_get_bits(ctx, 12, 5, 4) == 2)
> +
> +static int max2175_band_from_freq(u64 freq)
> +{
> + if (freq >= 144000 && freq <= 26100000)
> + return MAX2175_BAND_AM;
> + else if (freq >= 65000000 && freq <= 108000000)
> + return MAX2175_BAND_FM;
> + else if (freq >= 160000000 && freq <= 240000000)
> + return MAX2175_BAND_VHF;
> +
> + /* Add other bands on need basis */
> + return -ENOTSUPP;
> +}
> +
> +static int max2175_update_i2s_mode(struct max2175_ctx *ctx, u32 i2s_mode)
> +{
> + /* Only change if it's new */
> + if (max2175_read_bits(ctx, 29, 2, 0) == i2s_mode)
> + return 0;
> +
> + max2175_write_bits(ctx, 29, 2, 0, i2s_mode);
> +
> + /* Based on I2S mode value I2S_WORD_CNT values change */
> + if (i2s_mode == MAX2175_I2S_MODE3) {
> + max2175_write_bits(ctx, 30, 6, 0, 1);
> + } else if (i2s_mode == MAX2175_I2S_MODE2 ||
> + i2s_mode == MAX2175_I2S_MODE4) {
> + max2175_write_bits(ctx, 30, 6, 0, 0);
> + } else if (i2s_mode == MAX2175_I2S_MODE0) {
> + max2175_write_bits(ctx, 30, 6, 0,
> + ctx->rx_modes[ctx->mode].i2s_word_size);
> + } else {
> + v4l2_err(ctx->client,
> + "failed: i2s_mode %u unsupported\n", i2s_mode);
This should never happen as the control framework will validate control
values.
> + return 1;
Error codes should be negative.
> + }
> + mxm_dbg(ctx, "updated i2s_mode %u\n", i2s_mode);
> + return 0;
> +}
[snip]
> +static void max2175_load_dab_1p2(struct max2175_ctx *ctx)
> +{
> + u32 i;
unsigned int will do, no need for an explicitly sized type.
> +
> + /* Master is already set on init */
> + for (i = 0; i < ARRAY_SIZE(dab12_map); i++)
> + max2175_reg_write(ctx, dab12_map[i].idx, dab12_map[i].val);
> +
> + /* The default settings assume master */
> + if (!ctx->master)
> + max2175_write_bit(ctx, 30, 7, 1);
> +
> + /* Cache i2s_test value at this point */
> + ctx->i2s_test = max2175_get_bits(ctx, 104, 3, 0);
> + ctx->decim_ratio = 1;
> +
> + /* Load the Channel Filter Coefficients into channel filter bank #2 */
> + max2175_set_filter_coeffs(ctx, MAX2175_CH_MSEL, 2, ch_coeff_dab1);
> +}
[snip]
> +static bool max2175_set_csm_mode(struct max2175_ctx *ctx,
> + enum max2175_csm_mode new_mode)
> +{
> + int ret = max2175_poll_csm_ready(ctx);
> +
> + if (ret) {
> + v4l2_err(ctx->client, "csm not ready: new mode %d\n",
new_mode);
> + return ret;
> + }
> +
> + mxm_dbg(ctx, "set csm mode: new mode %d\n", new_mode);
> +
> + max2175_write_bits(ctx, 0, 2, 0, new_mode);
> +
> + /* Wait for a fixed settle down time depending on new mode and band */
> + switch (new_mode) {
> + case MAX2175_CSM_MODE_JUMP_FAST_TUNE:
> + if (MAX2175_IS_BAND_AM(ctx)) {
> + usleep_range(1250, 1500); /* 1.25ms */
> + } else {
> + if (MAX2175_IS_DAB_MODE(ctx))
> + usleep_range(3000, 3500); /* 3ms */
> + else
> + usleep_range(1250, 1500); /* 1.25ms */
> + }
You can write this as
if (MAX2175_IS_BAND_AM(ctx))
usleep_range(1250, 1500); /* 1.25ms */
else if (MAX2175_IS_DAB_MODE(ctx))
usleep_range(3000, 3500); /* 3ms */
else
usleep_range(1250, 1500); /* 1.25ms */
> + break;
> +
> + /* Other mode switches can be added in the future if needed */
> + default:
> + break;
> + }
> +
> + ret = max2175_poll_csm_ready(ctx);
> + if (ret) {
> + v4l2_err(ctx->client, "csm did not settle: new mode %d\n",
> + new_mode);
> + return ret;
> + }
> + return ret;
> +}
[snip]
> +
> +static int max2175_csm_action(struct max2175_ctx *ctx,
> + enum max2175_csm_mode action)
> +{
> + int ret;
> + int load_buffer = MAX2175_CSM_MODE_LOAD_TO_BUFFER;
> +
> + mxm_dbg(ctx, "csm action: %d\n", action);
> +
> + /* Perform one or two CSM mode commands. */
> + switch (action) {
> + case MAX2175_CSM_MODE_NO_ACTION:
> + /* Take no FSM Action. */
> + return 0;
> + case MAX2175_CSM_MODE_LOAD_TO_BUFFER:
> + case MAX2175_CSM_MODE_PRESET_TUNE:
> + case MAX2175_CSM_MODE_SEARCH:
> + case MAX2175_CSM_MODE_AF_UPDATE:
> + case MAX2175_CSM_MODE_JUMP_FAST_TUNE:
> + case MAX2175_CSM_MODE_CHECK:
> + case MAX2175_CSM_MODE_LOAD_AND_SWAP:
> + case MAX2175_CSM_MODE_END:
> + ret = max2175_set_csm_mode(ctx, action);
> + break;
> + case MAX2175_CSM_MODE_BUFFER_PLUS_PRESET_TUNE:
> + ret = max2175_set_csm_mode(ctx, load_buffer);
> + if (ret) {
> + v4l2_err(ctx->client, "csm action %d load buf
failed\n",
> + action);
> + break;
> + }
> + ret = max2175_set_csm_mode(ctx, MAX2175_CSM_MODE_PRESET_TUNE);
> + break;
> + case MAX2175_CSM_MODE_BUFFER_PLUS_SEARCH:
> + ret = max2175_set_csm_mode(ctx, load_buffer);
> + if (ret) {
> + v4l2_err(ctx->client, "csm action %d load buf
failed\n",
> + action);
> + break;
> + }
Don't duplicate the error messages, move them after the switch statement.
> + ret = max2175_set_csm_mode(ctx, MAX2175_CSM_MODE_SEARCH);
> + break;
> + case MAX2175_CSM_MODE_BUFFER_PLUS_AF_UPDATE:
> + ret = max2175_set_csm_mode(ctx, load_buffer);
> + if (ret) {
> + v4l2_err(ctx->client, "csm action %d load buf
failed\n",
> + action);
> + break;
> + }
> + ret = max2175_set_csm_mode(ctx, MAX2175_CSM_MODE_AF_UPDATE);
> + break;
> + case MAX2175_CSM_MODE_BUFFER_PLUS_JUMP_FAST_TUNE:
This function is only called with action set to
MAX2175_CSM_MODE_BUFFER_PLUS_JUMP_FAST_TUNE. I'd remove the rest of the code
for now, unless you have a plan to use it soon.
> + ret = max2175_set_csm_mode(ctx, load_buffer);
> + if (ret) {
> + v4l2_err(ctx->client, "csm action %d load buf
failed\n",
> + action);
> + break;
> + }
> + ret = max2175_set_csm_mode(ctx,
> + MAX2175_CSM_MODE_JUMP_FAST_TUNE);
> + break;
> + case MAX2175_CSM_MODE_BUFFER_PLUS_CHECK:
> + ret = max2175_set_csm_mode(ctx, load_buffer);
> + if (ret) {
> + v4l2_err(ctx->client, "csm action %d load buf
failed\n",
> + action);
> + break;
> + }
> + ret = max2175_set_csm_mode(ctx, MAX2175_CSM_MODE_CHECK);
> + break;
> + case MAX2175_CSM_MODE_BUFFER_PLUS_LOAD_AND_SWAP:
> + ret = max2175_set_csm_mode(ctx, load_buffer);
> + if (ret) {
> + v4l2_err(ctx->client, "csm action %d load buf
failed\n",
> + action);
> + break;
> + }
> + ret = max2175_set_csm_mode(ctx,
MAX2175_CSM_MODE_LOAD_AND_SWAP);
> + break;
> + default:
> + ret = max2175_set_csm_mode(ctx, MAX2175_CSM_MODE_NO_ACTION);
> + break;
> + }
> + return ret;
> +}
> +
> +static int max2175_set_lo_freq(struct max2175_ctx *ctx, u64 lo_freq)
> +{
> + int ret;
> + u32 lo_mult;
> + u64 scaled_lo_freq;
> + const u64 scale_factor = 1000000ULL;
> + u64 scaled_npf, scaled_integer, scaled_fraction;
> + u32 frac_desired, int_desired;
> + u8 loband_bits, vcodiv_bits;
Do you really support frequencies above 4GHz ? If not most of the 64-bit
values could be stored in 32 bits.
> +
> + scaled_lo_freq = lo_freq;
> + /* Scale to larger number for precision */
> + scaled_lo_freq = scaled_lo_freq * scale_factor * 100;
> +
> + mxm_dbg(ctx, "scaled lo_freq %llu lo_freq %llu\n",
> + scaled_lo_freq, lo_freq);
> +
> + if (MAX2175_IS_BAND_AM(ctx)) {
> + if (max2175_get_bit(ctx, 5, 7) == 0)
> + loband_bits = 0;
> + vcodiv_bits = 0;
> + lo_mult = 16;
> + } else if (max2175_get_bits(ctx, 5, 1, 0) == MAX2175_BAND_FM) {
> + if (lo_freq <= 74700000) {
> + loband_bits = 0;
> + vcodiv_bits = 0;
> + lo_mult = 16;
> + } else if ((lo_freq > 74700000) && (lo_freq <= 110000000)) {
No need for the inner parentheses.
> + loband_bits = 1;
> + vcodiv_bits = 0;
> + } else {
> + loband_bits = 1;
> + vcodiv_bits = 3;
> + }
> + lo_mult = 8;
> + } else if (max2175_get_bits(ctx, 5, 1, 0) == MAX2175_BAND_VHF) {
> + if (lo_freq <= 210000000) {
> + loband_bits = 2;
> + vcodiv_bits = 2;
> + } else {
> + loband_bits = 2;
> + vcodiv_bits = 1;
> + }
> + lo_mult = 4;
> + } else {
> + loband_bits = 3;
> + vcodiv_bits = 2;
> + lo_mult = 2;
> + }
> +
> + if (max2175_get_bits(ctx, 5, 1, 0) == MAX2175_BAND_L)
> + scaled_npf = (scaled_lo_freq / ctx->xtal_freq / lo_mult) /
100;
> + else
> + scaled_npf = (scaled_lo_freq / ctx->xtal_freq * lo_mult) /
100;
> +
> + scaled_integer = scaled_npf / scale_factor * scale_factor;
> + int_desired = (u32)(scaled_npf / scale_factor);
> + scaled_fraction = scaled_npf - scaled_integer;
> + frac_desired = (u32)(scaled_fraction * 1048576 / scale_factor);
> +
> + /* Check CSM is not busy */
> + ret = max2175_poll_csm_ready(ctx);
> + if (ret) {
> + v4l2_err(ctx->client, "lo_freq: csm busy. freq %llu\n",
> + lo_freq);
> + 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_set_bits(ctx, 5, 3, 2, loband_bits);
> + max2175_set_bits(ctx, 6, 7, 6, vcodiv_bits);
> + max2175_set_bits(ctx, 1, 7, 0, (u8)(int_desired & 0xff));
> + max2175_set_bits(ctx, 2, 3, 0, (u8)((frac_desired >> 16) & 0x1f));
> + max2175_set_bits(ctx, 3, 7, 0, (u8)((frac_desired >> 8) & 0xff));
> + max2175_set_bits(ctx, 4, 7, 0, (u8)(frac_desired & 0xff));
> + /* Flush the above registers to device */
> + max2175_flush_regstore(ctx, 1, 6);
> + return ret;
> +}
[snip]
> +static int max2175_set_rf_freq_non_am_bands(struct max2175_ctx *ctx, u64
> freq,
> + u32 lo_pos)
> +{
> + int ret;
> + s64 adj_freq;
> + u64 low_if_freq;
> +
> + mxm_dbg(ctx, "rf_freq: non AM bands\n");
> +
> + if (MAX2175_IS_FM_MODE(ctx))
> + low_if_freq = 128000;
> + else if (MAX2175_IS_FMHD_MODE(ctx))
> + low_if_freq = 228000;
> + else
> + return max2175_set_lo_freq(ctx, freq);
> +
> + if (max2175_get_bits(ctx, 5, 1, 0) == MAX2175_BAND_VHF) {
You perform this check in multiple places, you could create a helper function.
> + if (lo_pos == MAX2175_LO_ABOVE_DESIRED)
> + adj_freq = freq + low_if_freq;
> + else
> + adj_freq = freq - low_if_freq;
> + } else {
> + if (lo_pos == MAX2175_LO_ABOVE_DESIRED)
> + adj_freq = freq - low_if_freq;
> + else
> + adj_freq = freq + low_if_freq;
> + }
This could be written
if ((max2175_get_bits(ctx, 5, 1, 0) == MAX2175_BAND_VHF) ==
(lo_pos == MAX2175_LO_ABOVE_DESIRED))
adj_freq = freq + low_if_freq;
else
adj_freq = freq - low_if_freq;
Same for the other similar constructs in the driver. Up to you.
> +
> + ret = max2175_set_lo_freq(ctx, adj_freq);
> + if (ret)
> + return ret;
> +
> + return max2175_set_nco_freq(ctx, low_if_freq);
> +}
[snip]
> +#define max2175_ctx_from_sd(x) \
> + container_of(x, struct max2175_ctx, sd)
> +#define max2175_ctx_from_ctrl(x) \
> + container_of(x, struct max2175_ctx, ctrl_hdl)
I'd move it right after the structure definition, and turn them into static
inline functions.
> +static int max2175_s_ctrl(struct v4l2_ctrl *ctrl)
> +{
> + struct max2175_ctx *ctx = max2175_ctx_from_ctrl(ctrl->handler);
> + int ret = 0;
> +
> + mxm_dbg(ctx, "s_ctrl: id 0x%x, val %u\n", ctrl->id, ctrl->val);
> + switch (ctrl->id) {
> + case V4L2_CID_MAX2175_I2S_EN:
> + max2175_i2s_enable(ctx, ctrl->val == 1);
> + break;
> + case V4L2_CID_MAX2175_I2S_MODE:
> + max2175_s_ctrl_i2s_mode(ctx, ctrl->val);
> + break;
> + case V4L2_CID_MAX2175_AM_HIZ:
> + v4l2_ctrl_activate(ctx->am_hiz, false);
> + break;
> + case V4L2_CID_MAX2175_HSLS:
> + v4l2_ctrl_activate(ctx->hsls, false);
> + break;
> + case V4L2_CID_MAX2175_RX_MODE:
> + mxm_dbg(ctx, "rx mode %u\n", ctrl->val);
> + max2175_s_ctrl_rx_mode(ctx, ctrl->val);
> + break;
> + default:
> + v4l2_err(ctx->client, "s:invalid ctrl id 0x%x\n", ctrl->id);
> + ret = -EINVAL;
This should never happen. The driver has too many error and debug messages
overall.
> + }
> +
> + return ret;
> +}
> +
> +static int max2175_get_lna_gain(struct max2175_ctx *ctx)
> +{
> + int gain = 0;
> + enum max2175_band band = max2175_get_bits(ctx, 5, 1, 0);
> +
> + switch (band) {
> + case MAX2175_BAND_AM:
> + gain = max2175_read_bits(ctx, 51, 3, 1);
> + break;
> + case MAX2175_BAND_FM:
> + gain = max2175_read_bits(ctx, 50, 3, 1);
> + break;
> + case MAX2175_BAND_VHF:
> + gain = max2175_read_bits(ctx, 52, 3, 0);
> + break;
> + default:
> + v4l2_err(ctx->client, "invalid band %d to get rf gain\n",
band);
Can this happen ?
> + break;
> + }
> + return gain;
> +}
> +
> +static int max2175_g_volatile_ctrl(struct v4l2_ctrl *ctrl)
> +{
> + struct max2175_ctx *ctx = max2175_ctx_from_ctrl(ctrl->handler);
> +
> + /* Only the dynamically changing values need to be in g_volatile_ctrl
*/
> + mxm_dbg(ctx, "g_volatile_ctrl: id 0x%x\n", ctrl->id);
> + switch (ctrl->id) {
> + case V4L2_CID_RF_TUNER_LNA_GAIN:
> + ctrl->val = max2175_get_lna_gain(ctx);
> + break;
> + case V4L2_CID_RF_TUNER_IF_GAIN:
> + ctrl->val = max2175_read_bits(ctx, 49, 4, 0);
> + break;
> + case V4L2_CID_RF_TUNER_PLL_LOCK:
> + ctrl->val = (max2175_read_bits(ctx, 60, 7, 6) == 3);
> + break;
> + default:
> + v4l2_err(ctx->client, "g:invalid ctrl id 0x%x\n", ctrl->id);
This should never happen either.
> + return -EINVAL;
> + }
> + mxm_dbg(ctx, "g_volatile_ctrl: id 0x%x val %d\n", ctrl->id, ctrl-
>val);
> + return 0;
> +};
[snip]
> +static const struct v4l2_ctrl_config max2175_i2s_en = {
> + .ops = &max2175_ctrl_ops,
> + .id = V4L2_CID_MAX2175_I2S_EN,
V4L2_CID_MAX2175_I2S_ENABLE ?
> + .name = "I2S Enable",
> + .type = V4L2_CTRL_TYPE_BOOLEAN,
> + .min = 0,
> + .max = 1,
> + .step = 1,
> + .def = 1,
> +};
> +
> +static const struct v4l2_ctrl_config max2175_i2s_mode = {
> + .ops = &max2175_ctrl_ops,
> + .id = V4L2_CID_MAX2175_I2S_MODE,
> + .name = "I2S_MODE value",
> + .type = V4L2_CTRL_TYPE_INTEGER,
Should this be a menu control ?
> + .min = 0,
> + .max = 4,
> + .step = 1,
> + .def = 0,
> +};
> +
> +static const struct v4l2_ctrl_config max2175_am_hiz = {
> + .ops = &max2175_ctrl_ops,
> + .id = V4L2_CID_MAX2175_AM_HIZ,
> + .name = "AM High impedance input",
> + .type = V4L2_CTRL_TYPE_BOOLEAN,
> + .min = 0,
> + .max = 1,
> + .step = 1,
> + .def = 0,
> +};
> +
> +static const struct v4l2_ctrl_config max2175_hsls = {
> + .ops = &max2175_ctrl_ops,
> + .id = V4L2_CID_MAX2175_HSLS,
> + .name = "HSLS above/below desired",
> + .type = V4L2_CTRL_TYPE_INTEGER,
> + .min = 0,
> + .max = 1,
> + .step = 1,
> + .def = 1,
> +};
> +
> +
> +/* NOTE: Any addition/deletion in the below list should be reflected in
> + * max2175_modetag enum
> + */
> +static const char * const max2175_ctrl_eu_rx_mode_strings[] = {
> + "DAB 1.2",
> + "NULL",
Do you really mean "NULL", not NULL ?
> +};
> +
> +static const char * const max2175_ctrl_na_rx_mode_strings[] = {
> + "NA FM 1.0",
> + "NULL",
> +};
> +
> +static const struct v4l2_ctrl_config max2175_eu_rx_mode = {
> + .ops = &max2175_ctrl_ops,
> + .id = V4L2_CID_MAX2175_RX_MODE,
> + .name = "RX MODE",
> + .type = V4L2_CTRL_TYPE_MENU,
> + .max = ARRAY_SIZE(max2175_ctrl_eu_rx_mode_strings) - 2,
If there's a single mode supported I'd skip adding those controls for now.
> + .def = 0,
> + .qmenu = max2175_ctrl_eu_rx_mode_strings,
> +};
> +
> +static const struct v4l2_ctrl_config max2175_na_rx_mode = {
> + .ops = &max2175_ctrl_ops,
> + .id = V4L2_CID_MAX2175_RX_MODE,
> + .name = "RX MODE",
> + .type = V4L2_CTRL_TYPE_MENU,
> + .max = ARRAY_SIZE(max2175_ctrl_na_rx_mode_strings) - 2,
> + .def = 0,
> + .qmenu = max2175_ctrl_na_rx_mode_strings,
> +};
> +
> +static u32 max2175_refout_load_to_bits(struct i2c_client *client, u32 load)
> +{
> + u32 bits = 0; /* REFOUT disabled */
> +
> + if (load >= 0 && load <= 40)
> + bits = load / 10;
> + else if (load >= 60 && load <= 70)
> + bits = load / 10 - 1;
> + else
> + dev_warn(&client->dev, "invalid refout_load %u\n", load);
Your DT bindings specify 0 as a valid value.
An invalid value specified in DT should be a fatal error.
> +
> + return bits;
> +}
> +
> +static int max2175_probe(struct i2c_client *client,
> + const struct i2c_device_id *id)
> +{
> + struct max2175_ctx *ctx;
> + struct device *dev = &client->dev;
> + struct v4l2_subdev *sd;
> + struct v4l2_ctrl_handler *hdl;
> + struct clk *clk;
> + bool master = true;
> + u32 refout_load, refout_bits = 0; /* REFOUT disabled */
> + int ret;
> +
> + /* Check if the adapter supports the needed features */
> + if (!i2c_check_functionality(client->adapter,
> + I2C_FUNC_SMBUS_BYTE_DATA)) {
> + dev_err(&client->dev, "i2c check failed\n");
> + return -EIO;
> + }
> +
> + if (of_find_property(client->dev.of_node, "maxim,slave", NULL))
> + master = false;
> +
> + if (!of_property_read_u32(client->dev.of_node, "maxim,refout-load",
> + &refout_load))
> + refout_bits = max2175_refout_load_to_bits(client,
refout_load);
> +
> + clk = devm_clk_get(&client->dev, "xtal");
> + if (IS_ERR(clk)) {
> + ret = PTR_ERR(clk);
> + dev_err(&client->dev, "cannot get xtal clock %d\n", ret);
> + return -ENODEV;
> + }
> +
> + ctx = kzalloc(sizeof(struct max2175_ctx),
sizeof(*ctx)
> + GFP_KERNEL);
This fits on one line.
> + if (ctx == NULL)
> + return -ENOMEM;
> +
> + sd = &ctx->sd;
> + ctx->master = master;
> + ctx->mode_resolved = false;
> +
> + /* Set the defaults */
> + ctx->freq = bands_rf.rangelow;
> +
> + ctx->xtal_freq = clk_get_rate(clk);
> + dev_info(&client->dev, "xtal freq %lluHz\n", ctx->xtal_freq);
> +
> + v4l2_i2c_subdev_init(sd, client, &max2175_ops);
> + ctx->client = client;
> +
> + sd->flags = V4L2_SUBDEV_FL_HAS_DEVNODE;
> + ctx->dev = dev;
> +
> + /* Controls */
> + hdl = &ctx->ctrl_hdl;
> + ret = v4l2_ctrl_handler_init(hdl, 8);
> + if (ret) {
> + dev_err(&client->dev, "ctrl handler init failed\n");
> + goto err;
> + }
> +
> + ctx->lna_gain = v4l2_ctrl_new_std(hdl, &max2175_ctrl_ops,
> + V4L2_CID_RF_TUNER_LNA_GAIN,
> + 0, 15, 1, 2);
> + ctx->lna_gain->flags |= (V4L2_CTRL_FLAG_VOLATILE |
> + V4L2_CTRL_FLAG_READ_ONLY);
> + ctx->if_gain = v4l2_ctrl_new_std(hdl, &max2175_ctrl_ops,
> + V4L2_CID_RF_TUNER_IF_GAIN,
> + 0, 31, 1, 0);
> + ctx->if_gain->flags |= (V4L2_CTRL_FLAG_VOLATILE |
> + V4L2_CTRL_FLAG_READ_ONLY);
> + ctx->pll_lock = v4l2_ctrl_new_std(hdl, &max2175_ctrl_ops,
> + V4L2_CID_RF_TUNER_PLL_LOCK,
> + 0, 1, 1, 0);
> + ctx->pll_lock->flags |= (V4L2_CTRL_FLAG_VOLATILE |
> + V4L2_CTRL_FLAG_READ_ONLY);
> + ctx->i2s_en = v4l2_ctrl_new_custom(hdl, &max2175_i2s_en, NULL);
> + ctx->i2s_mode = v4l2_ctrl_new_custom(hdl, &max2175_i2s_mode, NULL);
> + ctx->am_hiz = v4l2_ctrl_new_custom(hdl, &max2175_am_hiz, NULL);
> + ctx->hsls = v4l2_ctrl_new_custom(hdl, &max2175_hsls, NULL);
> +
> + if (ctx->xtal_freq == MAX2175_EU_XTAL_FREQ) {
> + ctx->rx_mode = v4l2_ctrl_new_custom(hdl,
> + &max2175_eu_rx_mode,
NULL);
> + ctx->rx_modes = (struct max2175_rxmode *)eu_rx_modes;
> + } else {
> + ctx->rx_mode = v4l2_ctrl_new_custom(hdl,
> + &max2175_na_rx_mode,
NULL);
> + ctx->rx_modes = (struct max2175_rxmode *)na_rx_modes;
> + }
> + ctx->sd.ctrl_handler = &ctx->ctrl_hdl;
> +
> + ret = v4l2_async_register_subdev(sd);
> + if (ret) {
> + dev_err(&client->dev, "register subdev failed\n");
> + goto err_reg;
> + }
> + dev_info(&client->dev, "subdev registered\n");
> +
> + /* Initialize device */
> + ret = max2175_core_init(ctx, refout_bits);
> + if (ret)
> + goto err_init;
> +
> + mxm_dbg(ctx, "probed\n");
> + return 0;
> +
> +err_init:
> + v4l2_async_unregister_subdev(sd);
> +err_reg:
> + v4l2_ctrl_handler_free(&ctx->ctrl_hdl);
> +err:
> + kfree(ctx);
> + return ret;
> +}
> +
> +static int max2175_remove(struct i2c_client *client)
> +{
> + struct v4l2_subdev *sd = i2c_get_clientdata(client);
> + struct max2175_ctx *ctx = max2175_ctx_from_sd(sd);
> +
> + v4l2_ctrl_handler_free(&ctx->ctrl_hdl);
> + v4l2_async_unregister_subdev(sd);
> + mxm_dbg(ctx, "removed\n");
> + kfree(ctx);
> + return 0;
> +}
> +
> +static const struct i2c_device_id max2175_id[] = {
> + { DRIVER_NAME, 0},
> + {},
> +};
> +
No need for a blank line here.
> +MODULE_DEVICE_TABLE(i2c, max2175_id);
> +
> +static const struct of_device_id max2175_of_ids[] = {
> + { .compatible = "maxim, max2175", },
> + { }
> +};
> +MODULE_DEVICE_TABLE(of, max2175_of_ids);
> +
> +static struct i2c_driver max2175_driver = {
> + .driver = {
> + .name = DRIVER_NAME,
> + .of_match_table = max2175_of_ids,
> + },
> + .probe = max2175_probe,
> + .remove = max2175_remove,
> + .id_table = max2175_id,
> +};
> +
> +module_i2c_driver(max2175_driver);
> +
> +MODULE_DESCRIPTION("Maxim MAX2175 RF to Bits tuner driver");
> +MODULE_LICENSE("GPL v2");
> +MODULE_AUTHOR("Ramesh Shanmugasundaram
> <ramesh.shanmugasundaram@xxxxxxxxxxxxxx>"); diff --git
> a/drivers/media/i2c/max2175/max2175.h b/drivers/media/i2c/max2175/max2175.h
> new file mode 100644
> index 0000000..61a508b
> --- /dev/null
> +++ b/drivers/media/i2c/max2175/max2175.h
> @@ -0,0 +1,124 @@
> +/*
> + * Maxim Integrated MAX2175 RF to Bits tuner driver
> + *
> + * This driver & most of the hard coded values are based on the reference
> + * application delivered by Maxim for this chip.
> + *
> + * Copyright (C) 2016 Maxim Integrated Products
> + * Copyright (C) 2016 Renesas Electronics Corporation
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2
> + * as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + */
> +
> +#ifndef __MAX2175_H__
> +#define __MAX2175_H__
> +
> +#include <linux/types.h>
> +
> +enum max2175_region {
> + MAX2175_REGION_EU = 0, /* Europe */
> + MAX2175_REGION_NA, /* North America */
> +};
> +
> +#define MAX2175_EU_XTAL_FREQ (36864000) /* In Hz */
> +#define MAX2175_NA_XTAL_FREQ (40186125) /* In Hz */
> +
> +enum max2175_band {
> + MAX2175_BAND_AM = 0,
> + MAX2175_BAND_FM,
> + MAX2175_BAND_VHF,
> + MAX2175_BAND_L,
> +};
> +
> +/* NOTE: Any addition/deletion in the below enum should be reflected in
> + * V4L2_CID_MAX2175_RX_MODE ctrl strings
> + */
> +enum max2175_modetag {
> + /* EU modes */
> + MAX2175_DAB_1_2 = 0,
> +
> + /* Other possible modes to add in future
> + * MAX2175_DAB_1_0,
> + * MAX2175_DAB_1_3,
> + * MAX2175_EU_FM_2_2,
> + * MAX2175_EU_FM_1_0,
> + * MAX2175_EU_FMHD_4_0,
> + * MAX2175_EU_AM_1_0,
> + * MAX2175_EU_AM_2_2,
> + */
> +
> + /* NA modes */
> + MAX2175_NA_FM_1_0 = 0,
> +
> + /* Other possible modes to add in future
> + * MAX2175_NA_FM_1_2,
> + * MAX2175_NA_FMHD_1_0,
> + * MAX2175_NA_FMHD_1_2,
> + * MAX2175_NA_AM_1_0,
> + * MAX2175_NA_AM_1_2,
> + */
> +};
> +
> +/* Supported I2S modes */
> +enum {
> + MAX2175_I2S_MODE0 = 0,
> + MAX2175_I2S_MODE1,
> + MAX2175_I2S_MODE2,
> + MAX2175_I2S_MODE3,
> + MAX2175_I2S_MODE4,
> +};
> +
> +/* Coefficient table groups */
> +enum {
> + MAX2175_CH_MSEL = 0,
> + MAX2175_EQ_MSEL,
> + MAX2175_AA_MSEL,
> +};
> +
> +/* HSLS LO injection polarity */
> +enum {
> + MAX2175_LO_BELOW_DESIRED = 0,
> + MAX2175_LO_ABOVE_DESIRED,
> +};
> +
> +/* Channel FSM modes */
> +enum max2175_csm_mode {
> + MAX2175_CSM_MODE_LOAD_TO_BUFFER = 0,
> + MAX2175_CSM_MODE_PRESET_TUNE,
> + MAX2175_CSM_MODE_SEARCH,
> + MAX2175_CSM_MODE_AF_UPDATE,
> + MAX2175_CSM_MODE_JUMP_FAST_TUNE,
> + MAX2175_CSM_MODE_CHECK,
> + MAX2175_CSM_MODE_LOAD_AND_SWAP,
> + MAX2175_CSM_MODE_END,
> + MAX2175_CSM_MODE_BUFFER_PLUS_PRESET_TUNE,
> + MAX2175_CSM_MODE_BUFFER_PLUS_SEARCH,
> + MAX2175_CSM_MODE_BUFFER_PLUS_AF_UPDATE,
> + MAX2175_CSM_MODE_BUFFER_PLUS_JUMP_FAST_TUNE,
> + MAX2175_CSM_MODE_BUFFER_PLUS_CHECK,
> + MAX2175_CSM_MODE_BUFFER_PLUS_LOAD_AND_SWAP,
> + MAX2175_CSM_MODE_NO_ACTION
> +};
> +
> +/* Rx mode */
> +struct max2175_rxmode {
> + enum max2175_band band; /* Associated band */
> + u32 freq; /* Default freq in Hz */
> + u8 i2s_word_size; /* Bit value */
> + u8 i2s_modes[4]; /* Supported modes */
> +};
> +
> +/* Register map */
> +struct max2175_regmap {
> + u8 idx; /* Register index */
> + u8 val; /* Register value */
> +};
As no other file than max2175.c includes this, I would move at least the
structure definitions to the .c file.
> +#endif /* __MAX2175_H__ */
--
Regards,
Laurent Pinchart
