RE: [RFC 1/5] media: i2c: max2175: Add MAX2175 support

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Geert,

Thank you for the review. 

> Subject: Re: [RFC 1/5] media: i2c: max2175: Add MAX2175 support
> 
[...]
> 
> > --- /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
> > +  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.
> > +- 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
> 
> For properties involving units, usually the unit is made part of the
> property name, e.g. maxim,refout-load-pF = 40.
> This avoids confusion, and allows for extension later.

Agreed. I have modified it as

- maxim,refout-load-pF: load capacitance value (in pF) on reference
                        output drive level. The default is refout disabled
                        or no load. The possible load values are 
                        10pF
                        20pF
                        30pF
                        40pF
                        60pF
                        70pF

> 
> > +/* A tuner device instance under i2c bus */
> > +max2175_0: tuner@60 {
> > +       #clock-cells = <0>;
> > +       compatible = "maxim,max2175";
> > +       reg = <0x60>;
> > +       clocks = <&maxim_xtal>;
> > +       clock-names = "xtal";
> > +       maxim,refout-load = <10>;
> 
> 10 is not listed above. Perhaps you meant 10 pF?

Yes.

> 
> > --- /dev/null
> > +++ b/drivers/media/i2c/max2175/max2175.c
> > @@ -0,0 +1,1624 @@
> 
> > +/* NOTE: Any addition/deletion in the below list should be reflected
> > +in
> > + * max2175_modetag enum
> > + */
> 
> You can drop the above comment if you make this explicit using C99
> designated initializers, cfr. below.
> 
> > +static const struct max2175_rxmode eu_rx_modes[] = { /* Indexed by EU
> modetag */
> > +       /* EU modes */
> > +       { MAX2175_BAND_VHF, 182640000, 0, { 0, 0, 0, 0 } },
> 
> [MAX2175_DAB_1_2] = { MAX2175_BAND_VHF, 182640000, 0, { 0, 0, 0, 0 } },
> 
> > +};
> > +
> > +static const struct max2175_rxmode na_rx_modes[] = { /* Indexed by NA
> modetag */
> > +       /* NA modes */
> > +       { MAX2175_BAND_FM, 98255520, 1, { 0, 0, 0, 0 } },
> 
> [MAX2175_NA_FM_1_0] = { MAX2175_BAND_FM, 98255520, 1, { 0, 0, 0, 0 } },
> 

Thank you. Using designated initializers now.

> > +struct max2175_ctx {
> > +       struct v4l2_subdev sd;
> > +       struct i2c_client *client;
> > +       struct device *dev;
> > +
> > +       /* Cached configuration */
> > +       u8 regs[256];
> > +       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
> */
> > +};
> 
> Sorting the struct members by decreasing size helps to avoid gaps due to
> alignment restrictions, and may reduce memory consumption.

OK. I have corrected it to certain extent but still having groups (not strict decreasing order - for better readability). I have pushed the u8 group members to the end though.

> 
> > +/* Flush local copy to device from idx to idx+len (inclusive) */
> > +static void max2175_flush_regstore(struct max2175_ctx *ctx, u8 idx,
> > +u8 len) {
> > +       u8 i;
> 
> I'd just use unsigned int for loop counters.
> 

Agreed.

> > +
> > +       for (i = idx; i <= len; i++)
> > +               max2175_reg_write(ctx, i, ctx->regs[i]); }
> 
> > +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)
> 
> Many magic numbers, not only here (29, 2), but everywhere.
> Can you please add #defines for these?

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.

> 
> > +               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);
> > +               return 1;
> > +       }
> 
> switch (i2s_mode) { ... }

Agreed.

> 
> > +       mxm_dbg(ctx, "updated i2s_mode %u\n", i2s_mode);
> > +       return 0;
> > +}
> 
> > +static void max2175_set_filter_coeffs(struct max2175_ctx *ctx, u8
> m_sel,
> > +                                     u8 bank, const u16 *coeffs) {
> > +       u8 i, coeff_addr, upper_address;
> 
> I'd just use unsigned int for these.
> 

Agreed.

> > +
> > +       mxm_dbg(ctx, "start: m_sel %d bank %d\n", m_sel, bank);
> > +       max2175_write_bits(ctx, 114, 5, 4, m_sel);
> > +
> > +       if (m_sel == 2)
> > +               upper_address = 12;
> > +       else
> > +               upper_address = 24;
> > +
> > +       max2175_set_bit(ctx, 117, 7, 1);
> > +       for (i = 0; i < upper_address; i++) {
> > +               coeff_addr = i + (bank * 24);
> > +               max2175_set_bits(ctx, 115, 7, 0,
> > +                                (u8)((coeffs[i] >> 8) & 0xff));
> > +               max2175_set_bits(ctx, 116, 7, 0, (u8)(coeffs[i] &
> > + 0xff));
> 
> I don't think you need the casts to u8, or the masking with 0xff.

Yes. Corrected.

> 
> > +               max2175_set_bits(ctx, 117, 6, 0, coeff_addr);
> > +               max2175_flush_regstore(ctx, 115, 3);
> > +       }
> > +       max2175_write_bit(ctx, 117, 7, 0); }
> > +
> > +static void max2175_load_dab_1p2(struct max2175_ctx *ctx) {
> > +       u32 i;
> 
> unsigned int?

Agreed.

> 
> > +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;
> > +
> > +       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)) {
> > +                       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;
> 
> Please use one of the div64*() functions for divisions involving 64-bit
> quantities (try to build for 32-bit and see). More of these below...

Thanks. All 64bit divisions are corrected to div_x64 variants. You are right - kbuild reported MIPS build failure because of this. I tried only x86 for 32-bit.

> 
> > +       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);
> 
> > +       /* 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));
> 
> No need for casts etc.

Yes, corrected.

> 
> > +       /* Flush the above registers to device */
> > +       max2175_flush_regstore(ctx, 1, 6);
> > +       return ret;
> > +}
> > +
> > +static int max2175_set_nco_freq(struct max2175_ctx *ctx, s64
> > +nco_freq_desired) {
> > +       int ret;
> > +       u64 clock_rate, abs_nco_freq;
> > +       s64  nco_freq, nco_val_desired;
> > +       u32 nco_reg;
> > +       const u64 scale_factor = 1000000ULL;
> > +
> > +       mxm_dbg(ctx, "nco_freq: freq = %lld\n", nco_freq_desired);
> > +       clock_rate = ctx->xtal_freq / ctx->decim_ratio;
> > +       nco_freq = -nco_freq_desired;
> > +
> > +       if (nco_freq < 0)
> > +               abs_nco_freq = -nco_freq;
> > +       else
> > +               abs_nco_freq = nco_freq;
> > +
> > +       /* Scale up the values for precision */
> > +       if (abs_nco_freq < (clock_rate / 2)) {
> > +               nco_val_desired = (2 * nco_freq * scale_factor) /
> clock_rate;
> > +       } else {
> > +               if (nco_freq < 0)
> > +                       nco_val_desired = (-2 * (clock_rate -
> abs_nco_freq) *
> > +                               scale_factor) / clock_rate;
> > +               else
> > +                       nco_val_desired = (2 * (clock_rate -
> abs_nco_freq) *
> > +                               scale_factor) / clock_rate;
> > +       }
> > +
> > +       /* Scale down to get the fraction */
> > +       if (nco_freq < 0)
> > +               nco_reg = 0x200000 + ((nco_val_desired * 1048576) /
> > +                                     scale_factor);
> > +       else
> > +               nco_reg = (nco_val_desired * 1048576) / scale_factor;
> 
> More 64-bit divisions. In addition, the dividers are 64-bit too.
> Can't they be 32-bit?
> 

Agreed. The dividers need not be 32-bit. Corrected everywhere.

> > +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),
> > +                            GFP_KERNEL);
> 
> devm_kzalloc()?

Agreed & corrected. 

> 
> > +       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;
> > +       }
> 
> The casts are meant to cast away constness. Can that be avoided?

Agreed. Made ctx->rx_modes member as const and avoided the cast here.
> 
> > --- /dev/null
> > +++ b/drivers/media/i2c/max2175/max2175.h
> 
> > +/* NOTE: Any addition/deletion in the below enum should be reflected
> > +in
> > + * V4L2_CID_MAX2175_RX_MODE ctrl strings
> 
> Which strings exactly?

It is supposed to be these two

+ */
+static const char * const max2175_ctrl_eu_rx_mode_strings[] = {
+	"DAB 1.2",
+	"NULL",
+};
+
+static const char * const max2175_ctrl_na_rx_mode_strings[] = {
+	"NA FM 1.0",
+	"NULL",
+};
+

I have corrected them now to use designated initializers. Thanks.

> 
> > + */
> > +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,
> > +        */
> 
> The MAX2175_NA_* definitions share their values with the MAX2175_DAB_* and
> future MAX2175_EU_* values. Do you have to use a single enum for both?
> 

I have made them separate enums now. NA_ modes were not tested yet.

I have also fixed few bugs in the meantime. I'll wait for this week for other media maintainers & experts to comment on the driver model & the new SDR format open item. Based on the response, I will post a new [PATCH] or [RFC v2] with these comments updated.

Thanks,
Ramesh




[Index of Archives]     [Linux Samsung SOC]     [Linux Wireless]     [Linux Kernel]     [ATH6KL]     [Linux Bluetooth]     [Linux Netdev]     [Kernel Newbies]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Samba]     [Device Mapper]

  Powered by Linux