Re: [RFC 1/5] media: i2c: max2175: Add MAX2175 support
Hi Ramesh, On Wed, Oct 12, 2016 at 4:10 PM, 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 < for your > patch!amesh.shanmugasunda...@bp.renesas.com> Thanks for your patch! > --- /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. > +/* 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? > --- /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, 18264, 0, { 0, 0, 0, 0 } }, [MAX2175_DAB_1_2] = { MAX2175_BAND_VHF, 18264, 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 } }, > +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
RE: [RFC 1/5] media: i2c: max2175: Add MAX2175 support
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, 18264, 0, { 0, 0, 0, 0 } }, > > [MAX2175_DAB_1_2] = { MAX2175_BAND_VHF, 18264, 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
Re: [RFC 1/5] media: i2c: max2175: Add MAX2175 support
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 > --- > .../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 000..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 000..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 000..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
RE: [RFC 1/5] media: i2c: max2175: Add MAX2175 support
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 > > --- > > .../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 000..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 > > 000..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 n
Re: [RFC 1/5] media: i2c: max2175: Add MAX2175 support
Hi Ramesh, On Friday 21 Oct 2016 14:49:30 Ramesh Shanmugasundaram wrote: > > 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 > >> --- > >> > >> .../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 000..2250d5f > >> --- /dev/null > >> +++ b/Documentation/devicetree/bindings/media/i2c/max2175.txt > >> @@ -0,0 +1,60 @@ [snip] > >> +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. My point is that making the maxim,slave property a phandle now would allow handling such future cases without any change to the DT bindings. [snip] > > > diff --git a/drivers/media/i2c/max2175/max2175.c > > > b/drivers/media/i2c/max2175/max2175.c new file mode 100644 index > > > 000..71b60c2 > > > --- /dev/null > > > +++ b/drivers/media/i2c/max2175/max2175.c [snip] > > > +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. Is it the case that you don't have access to the information, or that you have been forbidden to disclose them by the tuner manufacturer ? > > > +} [snip] > > > +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 = 100ULL; > > > + 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). OK. The code would be more efficient if you made the scaling factor a power of two though. 1048576 could be a good value. > >> + > >> + scaled_lo_freq = lo_freq; > >> + /* Scale to larger number for precision */ > >> + scaled_lo_freq = scaled_lo_freq * scale_factor * 100; I just noticed that you could write the two lines as scaled_lo_freq = lo_freq * scale_factor * 100; By the way, why do you multiply by 100 here, and... > >> + 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 <= 7470) { > >> + loband_bits = 0; > >> + vcodiv_bits = 0; > >> + lo_mult = 16; > >> + } else if ((lo_freq > 7470) && (lo_freq <= 11000)) { > > > > No need for the inner