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 ��.n��������+%������w��{.n����z�{��ܨ}���Ơz�j:+v�����w����ޙ��&�)ߡ�a����z�ޗ���ݢj��w�f