Hi Laurent,
Thank you for the review comments.
> 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(r) 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.
Agreed & corrected.
>
> > + 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.
As of now, I cannot find any use case for it from the datasheet. In future, we could add if such need arise.
>
> > +- 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.
Agreed & corrected.
>
> > +
> > +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.
Thanks. It's a copy/paste mistake :-(. Corrected.
>
> > + 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/.
MAX2175 is huge with lot more modes and functionality. When more modes are added (it's pre-set hex values), we may have to introduce the new file containing that the hex values alone. Hence, I thought of a folder. However, I cannot tell when the next set of modes will be added. So I'll remove the folder in the next patch.
>
> > 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 :-)
Agreed.
>
> > +#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".
Agreed
>
> > +#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
> */
>
Agreed. I used it because checkpatch moaned but then I see Linus's comment :-).
> [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.
Thanks for the explanation. I have changed it to max2175.
>
> > + 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.
Thanks. I did not know about regmap. I'll try this.
>
> > + 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 ?
Unfortunately yes. I'll try to remove this after some more testing or may be later as a separate patch.
I have converted a Windows GUI app code to this driver. Most of the register details & configuration sequence are not available in the datasheet. I used similar pattern of log message as in GUI logs because it helps in testing :-(
>
> > + 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 :-)
Agreed.
>
> > +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.
The Tuner provider is unwilling to disclose all register details. I agree on the readability issue with this restriction but this is somewhat true for some sensitive IPs in the media subsystem.
>
> > +}
>
> [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.
Agreed.
>
> > + return 1;
>
> Error codes should be negative.
Agreed.
>
> > + }
> > + 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.
Agreed.
>
> > +
> > + /* 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 */
Agreed.
>
>
> > + 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.
Agreed. The error messages are not needed as the csm_mode function logs the mode argument in error log. Cleaned up.
>
> > + 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.
I'll remove. As mentioned earlier, I cannot tell how soon the other modes will be added. There are few other places with placeholders for other modes. I'll try to clean.
>
> > + 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 ?
Nope.
If not most of the 64-bit
> values could be stored in 32 bits.
The 64bit variables are needed to extract the fractional part (upto 6 digit precision) out of floating point divisions (original user space code).
>
> > +
> > + 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.
Agreed.
>
> > + 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.
Agreed.
>
> > + 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.
Agreed. Nice :-)
>
> > +
> > + 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.
Agreed.
>
> > +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.
Agreed :-). Will clean up.
> > + }
> > +
> > + 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 ?
Yes, there is "L-band". It is a paranoia check as I am testing by comparing logs sometimes :-(
>
> > + 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.
I agree.
>
> > + 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 ?
Agreed.
>
> > + .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 ?
Hmm... the strings would be named "i2s mode x"? Will that be OK?
>
> > + .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 ?
Sorry, That's cut/paste from vivid-ctrl. I have cleaned it up with designated initializers as Geert pointed out.
>
> > +};
> > +
> > +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.
I'll try to add one more mode support if time permits. The menu looks much readable with v4l2-ctl & comparing the same with datasheet.
>
> > + .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.
Agreed. The DT values are changed to 10-70 range.
>
> An invalid value specified in DT should be a fatal error.
OK. Will correct this.
>
> > +
> > + 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.
Yes, corrected in last cleanup.
>
> > + 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.
Agreed.
>
> > +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.
Agreed.
Thanks a lot for all the comments and suggestions. I'll post the next version soon.
Thanks,
Ramesh
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
