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.shanmugasunda...@bp.renesas.com> ---
>  .../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.shanmugasunda...@bp.renesas.com>"); 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

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to