| Message ID | 20180727121806.18209-1-srinivas.kandagatla@linaro.org |
|---|---|
| Headers | show |
| Series | ASoC: Add support to WCD9335 Audio Codec | expand |
On 27-07-18, 13:17, Srinivas Kandagatla wrote: > +obj-$(CONFIG_MFD_WCD9335) += wcd9335.o > +wcd9335-objs := wcd9335-core.o > + no slimbus objs? > +static int wcd9335_bring_up(struct wcd9335 *wcd) > +{ > + struct regmap *rm = wcd->regmap; > + int val, byte0; > + int ret = 0; > + > + regmap_read(rm, WCD9335_CHIP_TIER_CTRL_EFUSE_VAL_OUT0, &val); > + regmap_read(rm, WCD9335_CHIP_TIER_CTRL_CHIP_ID_BYTE0, &byte0); > + > + if ((val < 0) || (byte0 < 0)) { > + dev_err(wcd->dev, "wcd9335 codec version detection fail!\n"); > + return -EINVAL; > + } > + > + if (byte0 == 0x1) { > + dev_info(wcd->dev, "wcd9335 codec version is v2.0\n"); > + wcd->version = WCD9335_VERSION_2_0; > + regmap_write(rm, WCD9335_CODEC_RPM_RST_CTL, 0x01); > + regmap_write(rm, WCD9335_SIDO_SIDO_TEST_2, 0x00); > + regmap_write(rm, WCD9335_SIDO_SIDO_CCL_8, 0x6F); > + regmap_write(rm, WCD9335_BIAS_VBG_FINE_ADJ, 0x65); > + regmap_write(rm, WCD9335_CODEC_RPM_PWR_CDC_DIG_HM_CTL, 0x5); > + regmap_write(rm, WCD9335_CODEC_RPM_PWR_CDC_DIG_HM_CTL, 0x7); > + regmap_write(rm, WCD9335_CODEC_RPM_PWR_CDC_DIG_HM_CTL, 0x3); > + regmap_write(rm, WCD9335_CODEC_RPM_RST_CTL, 0x3); > + } else { > + dev_err(wcd->dev, "wcd9335 codec version not supported\n"); > + ret = -EINVAL; > + } > + > + return ret; we can do return 0 and remove the variable as it is not used anywhere else. > +static int wcd9335_slim_probe(struct slim_device *slim) > +{ > + struct device *dev = &slim->dev; > + struct wcd9335 *wcd; > + int ret = 0; > + > + /* Interface device */ > + if (slim->e_addr.dev_index == 0) > + return 0; > + > + wcd = devm_kzalloc(dev, sizeof(*wcd), GFP_KERNEL); > + if (!wcd) > + return -ENOMEM; ^^ double space -- ~Vinod -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Jul 27, 2018 at 01:18:01PM +0100, Srinivas Kandagatla wrote: > + res_val = WCD9XXX_CLASSH_CTRL_VCL_VREF_FILT_R_0KOHM; > + switch (mode) { > + case CLS_H_NORMAL: > + res_val = WCD9XXX_CLASSH_CTRL_VCL_VREF_FILT_R_50KOHM; > + val = WCD9XXX_A_ANA_HPH_PWR_LEVEL_NORMAL; > + gain = DAC_GAIN_0DB; > + ipeak = WCD9XXX_CLASSH_CTRL_CCL_1_DELTA_IPEAK_50MA; > + break; > + case CLS_AB: > + val = WCD9XXX_A_ANA_HPH_PWR_LEVEL_NORMAL; > + gain = DAC_GAIN_0DB; > + ipeak = WCD9XXX_CLASSH_CTRL_CCL_1_DELTA_IPEAK_50MA; > + break; > + case CLS_H_HIFI: > + val = WCD9XXX_A_ANA_HPH_PWR_LEVEL_UHQA; > + gain = DAC_GAIN_M0P2DB; > + ipeak = WCD9XXX_CLASSH_CTRL_CCL_1_DELTA_IPEAK_50MA; > + break; > + case CLS_H_LP: > + val = WCD9XXX_A_ANA_HPH_PWR_LEVEL_LP; > + ipeak = WCD9XXX_CLASSH_CTRL_CCL_1_DELTA_IPEAK_30MA; > + break; > + }; This is missing a default case for error checking, and you don't need the semicolon at the end either.
On Fri, Jul 27, 2018 at 01:17:57PM +0100, Srinivas Kandagatla wrote: > This patch adds bindings for wcd9335 audio codec which can support both SLIMbus > and I2S/I2C interface. > > Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> > --- > .../devicetree/bindings/sound/qcom,wcd9335.txt | 123 +++++++++++++++++++++ > 1 file changed, 123 insertions(+) > create mode 100644 Documentation/devicetree/bindings/sound/qcom,wcd9335.txt > > diff --git a/Documentation/devicetree/bindings/sound/qcom,wcd9335.txt b/Documentation/devicetree/bindings/sound/qcom,wcd9335.txt > new file mode 100644 > index 000000000000..1d8d49e30af7 > --- /dev/null > +++ b/Documentation/devicetree/bindings/sound/qcom,wcd9335.txt > @@ -0,0 +1,123 @@ > +QCOM WCD9335 Codec > + > +Qualcomm WCD9335 Codec is a standalone Hi-Fi audio codec IC, supports > +Qualcomm Technologies, Inc. (QTI) multimedia solutions, including > +the MSM8996, MSM8976, and MSM8956 chipsets. It has in-built > +Soundwire controller, interrupt mux. It supports both I2S/I2C and > +SLIMbus audio interfaces. > + > +Required properties with SLIMbus Interface: > + > +- compatible: > + Usage: required > + Value type: <stringlist> > + Definition: For SLIMbus interface it should be "slimMID,PID", > + textual representation of Manufacturer ID, Product Code, > + shall be in lower case hexadecimal with leading zeroes > + suppressed. Refer to slimbus/bus.txt for details. > + Should be: > + "slim217,1a0" for MSM8996 and APQ8096 SoCs with SLIMbus. > + > +- reg > + Usage: required > + Value type: <u32 u32> > + Definition: Should be ('Device index', 'Instance ID') > + > +- interrupts > + Usage: required > + Value type: <prop-encoded-array> > + Definition: Interrupts via WCD INTR1 and INTR2 pins > + > +- interrupt-names: > + Usage: required > + Value type: <String array> > + Definition: Interrupt names of WCD INTR1 and INTR2 > + Should be: "intr1", "intr2" > + > +- reset-gpio: reset-gpios > + Usage: required > + Value type: <String Array> > + Definition: Reset gpio line > + > +- qcom,ifd: > + Usage: required > + Value type: <phandle> > + Definition: SLIM interface device Wouldn't this be the parent? > + > +- clocks: > + Usage: required > + Value type: <prop-encoded-array> > + Definition: See clock-bindings.txt section "consumers". List of > + three clock specifiers for mclk, mclk2 and slimbus clock. > + > +- clock-names: > + Usage: required > + Value type: <string> > + Definition: Must contain "mclk", "mclk2" and "slimbus" strings. > + > +- vdd-buck-supply: > + Usage: required > + Value type: <phandle> > + Definition: Should contain a reference to the 1.8V buck supply > + > +- vdd-buck-sido-supply: > + Usage: required > + Value type: <phandle> > + Definition: Should contain a reference to the 1.8V SIDO buck supply > + > +- vdd-rx-supply: > + Usage: required > + Value type: <phandle> > + Definition: Should contain a reference to the 1.8V rx supply > + > +- vdd-tx-supply: > + Usage: required > + Value type: <phandle> > + Definition: Should contain a reference to the 1.8V tx supply > + > +- vdd-vbat-supply: > + Usage: Optional > + Value type: <phandle> > + Definition: Should contain a reference to the vbat supply > + > +- vdd-micbias-supply: > + Usage: required > + Value type: <phandle> > + Definition: Should contain a reference to the micbias supply > + > +- vdd-io-supply: > + Usage: required > + Value type: <phandle> > + Definition: Should contain a reference to the 1.8V io supply > + > +- interrupt-controller: > + Usage: required > + Definition: Indicating that this is a interrupt controller > + > +- #interrupt-cells: > + Usage: required > + Value type: <int> > + Definition: should be 1 > + > +#sound-dai-cells > + Usage: required > + Value type: <u32> > + Definition: Must be 1 > + > +codec@1{ audio-codec@1 > + compatible = "slim217,1a0"; > + reg = <1 0>; > + interrupts = <&msmgpio 54 IRQ_TYPE_LEVEL_HIGH>; > + interrupt-names = "intr2" > + reset-gpio = <&msmgpio 64 0>; > + qcom,ifd = <&wc9335_ifd>; > + clock-names = "mclk", "native"; > + clocks = <&rpmcc RPM_SMD_DIV_CLK1>, > + <&rpmcc RPM_SMD_BB_CLK1>; > + vdd-buck-supply = <&pm8994_s4>; > + vdd-rx-supply = <&pm8994_s4>; > + vdd-buck-sido-supply = <&pm8994_s4>; > + vdd-tx-supply = <&pm8994_s4>; > + vdd-io-supply = <&pm8994_s4>; > + #sound-dai-cells = <1>; > +} > -- > 2.16.2 > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Thanks for the review. On 31/07/18 21:43, Rob Herring wrote: >> + >> +- reset-gpio: > reset-gpios > Yep, reset-gpios makes more inline with others! >> + Usage: required >> + Value type: <String Array> >> + Definition: Reset gpio line >> + >> +- qcom,ifd: >> + Usage: required >> + Value type: <phandle> >> + Definition: SLIM interface device > Wouldn't this be the parent? > No, Interface device is just like other slim device and is part of SLIMbus Component and has a unique address. Every SLIMbus component has one interface device. ... >> + >> +codec@1{ > audio-codec@1 > Yep, I will fix this in next version. >> + compatible = "slim217,1a0"; >> + reg = <1 0>; Thanks, srini -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Thanks for reviewing, On 30/07/18 05:12, Vinod wrote: > On 27-07-18, 13:17, Srinivas Kandagatla wrote: >> +obj-$(CONFIG_MFD_WCD9335) += wcd9335.o >> +wcd9335-objs := wcd9335-core.o >> + > > no slimbus objs? > Yep, I will try to tidy this up a bit in next version. >> +static int wcd9335_bring_up(struct wcd9335 *wcd) >> +{ >> + struct regmap *rm = wcd->regmap; >> + int val, byte0; >> + int ret = 0; >> + >> + regmap_read(rm, WCD9335_CHIP_TIER_CTRL_EFUSE_VAL_OUT0, &val); >> + regmap_read(rm, WCD9335_CHIP_TIER_CTRL_CHIP_ID_BYTE0, &byte0); >> + >> + if ((val < 0) || (byte0 < 0)) { >> + dev_err(wcd->dev, "wcd9335 codec version detection fail!\n"); >> + return -EINVAL; >> + } >> + >> + if (byte0 == 0x1) { >> + dev_info(wcd->dev, "wcd9335 codec version is v2.0\n"); >> + wcd->version = WCD9335_VERSION_2_0; >> + regmap_write(rm, WCD9335_CODEC_RPM_RST_CTL, 0x01); >> + regmap_write(rm, WCD9335_SIDO_SIDO_TEST_2, 0x00); >> + regmap_write(rm, WCD9335_SIDO_SIDO_CCL_8, 0x6F); >> + regmap_write(rm, WCD9335_BIAS_VBG_FINE_ADJ, 0x65); >> + regmap_write(rm, WCD9335_CODEC_RPM_PWR_CDC_DIG_HM_CTL, 0x5); >> + regmap_write(rm, WCD9335_CODEC_RPM_PWR_CDC_DIG_HM_CTL, 0x7); >> + regmap_write(rm, WCD9335_CODEC_RPM_PWR_CDC_DIG_HM_CTL, 0x3); >> + regmap_write(rm, WCD9335_CODEC_RPM_RST_CTL, 0x3); >> + } else { >> + dev_err(wcd->dev, "wcd9335 codec version not supported\n"); >> + ret = -EINVAL; >> + } >> + >> + return ret; > > we can do return 0 and remove the variable as it is not used anywhere > else. Yes, it makes sense! Thanks, srini -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Thanks for review, On 01/08/18 23:20, Rob Herring wrote: >>>> +- qcom,ifd: >>>> + Usage: required >>>> + Value type: <phandle> >>>> + Definition: SLIM interface device >>> Wouldn't this be the parent? >>> >> No, Interface device is just like other slim device and is part of >> SLIMbus Component and has a unique address. Every SLIMbus component has >> one interface device. > I still don't understand what this means. If this is SLIMbus specific, > then maybe it should be named that way? Or it is QCom specific? It is SLIMbus specific, I will rename this to "slim,ifd" in next version which makes it clear! thanks, srini -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, 27 Jul 2018, Srinivas Kandagatla wrote: > Qualcomm WCD9335 Codec is a standalone Hi-Fi audio codec IC, > It has mulitple blocks like Soundwire controller, codec, > Codec processing engine, ClassH controller, interrupt mux. > It supports both I2S/I2C and SLIMbus audio interfaces. > > This patch adds support to SLIMbus audio interface. > > Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> > --- > drivers/mfd/Kconfig | 18 ++ > drivers/mfd/Makefile | 4 + > drivers/mfd/wcd9335-core.c | 268 ++++++++++++++++ > include/linux/mfd/wcd9335/registers.h | 580 ++++++++++++++++++++++++++++++++++ > include/linux/mfd/wcd9335/wcd9335.h | 42 +++ > 5 files changed, 912 insertions(+) > create mode 100644 drivers/mfd/wcd9335-core.c > create mode 100644 include/linux/mfd/wcd9335/registers.h > create mode 100644 include/linux/mfd/wcd9335/wcd9335.h > > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig > index f3fa516011ec..6e5b5f3cfe20 100644 > --- a/drivers/mfd/Kconfig > +++ b/drivers/mfd/Kconfig > @@ -1807,6 +1807,24 @@ config MFD_WM97xx > support for the WM97xx, in order to use the actual functionaltiy of > the device other drivers must be enabled. > > +config MFD_WCD9335 > + tristate > + select MFD_CORE > + select REGMAP > + select REGMAP_IRQ > + > +config MFD_WCD9335_SLIM > + tristate "Qualcomm WCD9335 with SLIMbus" > + select MFD_WCD9335 > + select REGMAP_SLIMBUS > + depends on SLIMBUS > + help > + The WCD9335 is a standalone Hi-Fi audio codec IC, supports s/codec/CODEC/ > + Qualcomm Technologies, Inc. (QTI) multimedia solutions, including > + the MSM8996, MSM8976, and MSM8956 chipsets. It has inbuild s/inbuild/in-built/ > + Soundwire controller, interrupt mux. It supports both I2S/I2C and > + SLIMbus audio interfaces. This option selects SLIMbus audio interface. The help should be indented. > config MFD_STW481X > tristate "Support for ST Microelectronics STw481x" > depends on I2C && (ARCH_NOMADIK || COMPILE_TEST) > diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile > index 2852a6042ecf..a4697370640b 100644 > --- a/drivers/mfd/Makefile > +++ b/drivers/mfd/Makefile > @@ -56,6 +56,10 @@ endif > ifeq ($(CONFIG_MFD_CS47L24),y) > obj-$(CONFIG_MFD_ARIZONA) += cs47l24-tables.o > endif > + > +obj-$(CONFIG_MFD_WCD9335) += wcd9335.o > +wcd9335-objs := wcd9335-core.o > + > obj-$(CONFIG_MFD_WM8400) += wm8400-core.o > wm831x-objs := wm831x-core.o wm831x-irq.o wm831x-otp.o > wm831x-objs += wm831x-auxadc.o > diff --git a/drivers/mfd/wcd9335-core.c b/drivers/mfd/wcd9335-core.c > new file mode 100644 > index 000000000000..8f746901f4e9 > --- /dev/null > +++ b/drivers/mfd/wcd9335-core.c > @@ -0,0 +1,268 @@ > +// SPDX-License-Identifier: GPL-2.0 > +// Copyright (c) 2018, Linaro Limited > + > +#include <linux/kernel.h> > +#include <linux/module.h> > +#include <linux/slimbus.h> > +#include <linux/regmap.h> > +#include <linux/of.h> > +#include <linux/of_gpio.h> > +#include <linux/clk.h> > +#include <linux/gpio.h> > +#include <linux/mfd/core.h> > +#include <linux/platform_device.h> > +#include <linux/regulator/consumer.h> > +#include <linux/mfd/wcd9335/wcd9335.h> > +#include <linux/mfd/wcd9335/registers.h> Alphabetical. > +static const struct regmap_range_cfg wcd9335_ranges[] = { > + { .name = "WCD9335", What is that? New line please. > + .range_min = 0x0, > + .range_max = WCD9335_MAX_REGISTER, > + .selector_reg = WCD9335_REG(0x0, 0), > + .selector_mask = 0xff, > + .selector_shift = 0, > + .window_start = 0x0, > + .window_len = 0x1000, > + }, > +}; > + > +static struct regmap_config wcd9335_regmap_config = { > + .reg_bits = 16, > + .val_bits = 8, > + .cache_type = REGCACHE_RBTREE, > + .max_register = WCD9335_MAX_REGISTER, > + .can_multi_write = true, > + .ranges = wcd9335_ranges, > + .num_ranges = ARRAY_SIZE(wcd9335_ranges), > +}; > + > +static const struct regmap_range_cfg wcd9335_ifd_ranges[] = { > + { .name = "WCD9335-IFD", As above. > + .range_min = 0x0, > + .range_max = WCD9335_REG(0, 0x7ff), > + .selector_reg = WCD9335_REG(0, 0x0), > + .selector_mask = 0xff, > + .selector_shift = 0, > + .window_start = 0x0, > + .window_len = 0x1000, > + }, > +}; > + > +static struct regmap_config wcd9335_ifd_regmap_config = { > + .reg_bits = 16, > + .val_bits = 8, > + .can_multi_write = true, > + .max_register = WCD9335_REG(0, 0x7FF), > + .ranges = wcd9335_ifd_ranges, > + .num_ranges = ARRAY_SIZE(wcd9335_ifd_ranges), > +}; > + > +static int wcd9335_parse_dt(struct wcd9335 *wcd) > +{ > + struct device *dev = wcd->dev; > + struct device_node *np = dev->of_node; > + int ret; > + > + wcd->reset_gpio = of_get_named_gpio(np, "reset-gpio", 0); > + if (wcd->reset_gpio < 0) { > + dev_err(dev, "Reset gpio missing in DT\n"); s/gpio/GPIO/ s/missing in DT/missing from DT/ > + return wcd->reset_gpio; > + } > + > + wcd->mclk = devm_clk_get(dev, "mclk"); > + if (IS_ERR(wcd->mclk)) { > + dev_err(dev, "mclk not found\n"); > + return PTR_ERR(wcd->mclk); > + } > + > + wcd->native_clk = devm_clk_get(dev, "slimbus"); > + if (IS_ERR(wcd->native_clk)) { > + dev_err(dev, "slimbus clk not found\n"); Any reason for abbreviating 'clock' in the error message? > + return PTR_ERR(wcd->native_clk); > + } > + > + wcd->supplies[0].supply = "vdd-buck"; > + wcd->supplies[1].supply = "vdd-buck-sido"; > + wcd->supplies[2].supply = "vdd-tx"; > + wcd->supplies[3].supply = "vdd-rx"; > + wcd->supplies[4].supply = "vdd-io"; > + > + ret = regulator_bulk_get(dev, WCD9335_MAX_SUPPLY, wcd->supplies); > + if (ret != 0) { "if (ret)" Same for all. I won't comment on the others. > + dev_err(dev, "Failed to get supplies: err = %d\n", ret); > + return ret; > + } > + > + return 0; > +} > + > +static int wcd9335_power_on_reset(struct wcd9335 *wcd) > +{ > + struct device *dev = wcd->dev; > + int ret; > + > + ret = regulator_bulk_enable(WCD9335_MAX_SUPPLY, wcd->supplies); > + if (ret != 0) { > + dev_err(dev, "Failed to get supplies: err = %d\n", ret); > + return ret; > + } > + > + /* > + * For WCD9335, it takes about 600us for the Vout_A and > + * Vout_D to be ready after BUCK_SIDO is powered up. > + * SYS_RST_N shouldn't be pulled high during this time > + */ > + usleep_range(600, 650); > + > + gpio_direction_output(wcd->reset_gpio, 0); > + msleep(20); What's this for? Why can't you just: gpio_direction_output(wcd->reset_gpio, 1); ... and drop the next 2 lines? > + gpio_set_value(wcd->reset_gpio, 1); > + msleep(20); > + > + return 0; > +} > + > +static int wcd9335_bring_up(struct wcd9335 *wcd) > +{ > + struct regmap *rm = wcd->regmap; > + int val, byte0; > + int ret = 0; > + > + regmap_read(rm, WCD9335_CHIP_TIER_CTRL_EFUSE_VAL_OUT0, &val); > + regmap_read(rm, WCD9335_CHIP_TIER_CTRL_CHIP_ID_BYTE0, &byte0); > + > + if ((val < 0) || (byte0 < 0)) { > + dev_err(wcd->dev, "wcd9335 codec version detection fail!\n"); s/wcd9335 codec/WCD9335 CODEC/ ? > + return -EINVAL; > + } > + > + if (byte0 == 0x1) { > + dev_info(wcd->dev, "wcd9335 codec version is v2.0\n"); > + wcd->version = WCD9335_VERSION_2_0; > + regmap_write(rm, WCD9335_CODEC_RPM_RST_CTL, 0x01); > + regmap_write(rm, WCD9335_SIDO_SIDO_TEST_2, 0x00); > + regmap_write(rm, WCD9335_SIDO_SIDO_CCL_8, 0x6F); > + regmap_write(rm, WCD9335_BIAS_VBG_FINE_ADJ, 0x65); > + regmap_write(rm, WCD9335_CODEC_RPM_PWR_CDC_DIG_HM_CTL, 0x5); > + regmap_write(rm, WCD9335_CODEC_RPM_PWR_CDC_DIG_HM_CTL, 0x7); > + regmap_write(rm, WCD9335_CODEC_RPM_PWR_CDC_DIG_HM_CTL, 0x3); > + regmap_write(rm, WCD9335_CODEC_RPM_RST_CTL, 0x3); > + } else { > + dev_err(wcd->dev, "wcd9335 codec version not supported\n"); As above. > + ret = -EINVAL; Why not just return -EINVAL; Then you can just return 0 and avoid pre-initialising ret. > + } > + > + return ret; > +} > + > +static int wcd9335_slim_probe(struct slim_device *slim) > +{ > + struct device *dev = &slim->dev; > + struct wcd9335 *wcd; > + int ret = 0; Why pre-initialise? > + /* Interface device */ > + if (slim->e_addr.dev_index == 0) Is 0 the parent? I think 0 needs defining for clarity. > + return 0; > + > + wcd = devm_kzalloc(dev, sizeof(*wcd), GFP_KERNEL); > + if (!wcd) > + return -ENOMEM; > + > + wcd->dev = dev; '\n' here. > + ret = wcd9335_parse_dt(wcd); > + if (ret) { > + dev_err(dev, "Error parsing DT (%d)\n", ret); This format changes from message to message. Please pick one and stick with it. I personally like: "<MESSAGE>: %d", ret > + return ret; > + } > + > + ret = wcd9335_power_on_reset(wcd); > + if (ret) { > + dev_err(dev, "Error Powering\n"); I think this is superflouous since you already printed a message. > + return ret; > + } > + > + wcd->regmap = regmap_init_slimbus(slim, &wcd9335_regmap_config); > + if (IS_ERR(wcd->regmap)) { > + ret = PTR_ERR(wcd->regmap); > + dev_err(dev, "Failed to allocate register map:%d\n", ret); A different format again. Ensure there is a ' ' after the ':'. > + return ret; > + } > + > + dev_set_drvdata(dev, wcd); Probably nicer to do this at the very end - after a '\n'. > + wcd->slim = slim; > + wcd->intf_type = WCD9335_INTERFACE_TYPE_SLIMBUS; > + > + return 0; > +} > + > +static const struct mfd_cell wcd9335_devices[] = { > + { > + .name = "wcd9335-codec", > + }, > +}; When are you going to add the other devices? By the way, one line entries should be placed on one line. > + { .name = "wcd9335-codec" }, > +static int wcd9335_slim_status(struct slim_device *sdev, > + enum slim_device_status s) 's' is not a good variable name. Suggest 'status'. > +{ > + struct device_node *ifd_np; What's 'ifd'? > + struct wcd9335 *wcd; > + struct device *dev; > + int ret; > + > + /* Interface device */ As previously suggested just define the device ID and drop the comment. > + if (sdev->e_addr.dev_index == 0) > + return 0; > + > + wcd = dev_get_drvdata(&sdev->dev); > + dev = wcd->dev; Odd. You do to the effort of assigning this and use 'wcd->dev' at most call sites anyway. I'd drop 'dev' and just use it from 'wcd' everywhere. > + ifd_np = of_parse_phandle(wcd->dev->of_node, "qcom,ifd", 0); > + if (!ifd_np) { > + dev_err(wcd->dev, "No Interface device found\n"); > + return -EINVAL; > + } > + > + wcd->slim_ifd = of_slim_get_device(sdev->ctrl, ifd_np); > + if (!wcd->slim_ifd) { > + dev_err(wcd->dev, "Unable to get SLIM Interface device\n"); > + return -EINVAL; > + } > + > + wcd->ifd_regmap = regmap_init_slimbus(wcd->slim_ifd, > + &wcd9335_ifd_regmap_config); > + if (IS_ERR(wcd->ifd_regmap)) { > + dev_err(dev, "Failed to allocate register map\n"); > + return PTR_ERR(wcd->ifd_regmap); > + } > + > + ret = wcd9335_bring_up(wcd); > + if (ret) { > + dev_err(dev, "Failed to bringup WCD9335\n"); > + return ret; > + } > + > + wcd->slim_ifd = wcd->slim_ifd; Am I missing something? > + return mfd_add_devices(wcd->dev, 0, wcd9335_devices, > + ARRAY_SIZE(wcd9335_devices), NULL, 0, NULL); No error message if it were to fail? > +} > + > +static const struct slim_device_id wcd9335_slim_id[] = { > + {0x217, 0x1a0, 0x1, 0x0}, Well that's just horrific. Can't these be defined? > + {} > +}; > + > +static struct slim_driver wcd9335_slim_driver = { > + .driver = { > + .name = "wcd9335-slim", > + }, > + .probe = wcd9335_slim_probe, > + .device_status = wcd9335_slim_status, > + .id_table = wcd9335_slim_id, > +}; > + > +module_slim_driver(wcd9335_slim_driver); > +MODULE_DESCRIPTION("WCD9335 slim driver"); > +MODULE_LICENSE("GPL v2"); > diff --git a/include/linux/mfd/wcd9335/registers.h b/include/linux/mfd/wcd9335/registers.h -- Lee Jones [李琼斯] Linaro Services Technical Lead Linaro.org │ Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Thanks for the review, On 02/08/18 09:33, Lee Jones wrote: > On Fri, 27 Jul 2018, Srinivas Kandagatla wrote: > >> Qualcomm WCD9335 Codec is a standalone Hi-Fi audio codec IC, >> It has mulitple blocks like Soundwire controller, codec, >> Codec processing engine, ClassH controller, interrupt mux. >> It supports both I2S/I2C and SLIMbus audio interfaces. >> >> This patch adds support to SLIMbus audio interface. >> >> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> >> --- >> drivers/mfd/Kconfig | 18 ++ >> drivers/mfd/Makefile | 4 + >> drivers/mfd/wcd9335-core.c | 268 ++++++++++++++++ >> include/linux/mfd/wcd9335/registers.h | 580 ++++++++++++++++++++++++++++++++++ >> include/linux/mfd/wcd9335/wcd9335.h | 42 +++ >> 5 files changed, 912 insertions(+) >> create mode 100644 drivers/mfd/wcd9335-core.c >> create mode 100644 include/linux/mfd/wcd9335/registers.h >> create mode 100644 include/linux/mfd/wcd9335/wcd9335.h >> >> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig >> index f3fa516011ec..6e5b5f3cfe20 100644 >> --- a/drivers/mfd/Kconfig >> +++ b/drivers/mfd/Kconfig >> @@ -1807,6 +1807,24 @@ config MFD_WM97xx >> support for the WM97xx, in order to use the actual functionaltiy of >> the device other drivers must be enabled. >> >> +config MFD_WCD9335 >> + tristate >> + select MFD_CORE >> + select REGMAP >> + select REGMAP_IRQ >> + >> +config MFD_WCD9335_SLIM >> + tristate "Qualcomm WCD9335 with SLIMbus" >> + select MFD_WCD9335 >> + select REGMAP_SLIMBUS >> + depends on SLIMBUS >> + help >> + The WCD9335 is a standalone Hi-Fi audio codec IC, supports > > s/codec/CODEC/ Yep. > >> + Qualcomm Technologies, Inc. (QTI) multimedia solutions, including >> + the MSM8996, MSM8976, and MSM8956 chipsets. It has inbuild > > s/inbuild/in-built/ Sure! > >> + Soundwire controller, interrupt mux. It supports both I2S/I2C and >> + SLIMbus audio interfaces. This option selects SLIMbus audio interface. > > The help should be indented. Sure! > >> config MFD_STW481X >> tristate "Support for ST Microelectronics STw481x" >> depends on I2C && (ARCH_NOMADIK || COMPILE_TEST) >> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile >> index 2852a6042ecf..a4697370640b 100644 >> --- a/drivers/mfd/Makefile >> +++ b/drivers/mfd/Makefile >> @@ -56,6 +56,10 @@ endif >> ifeq ($(CONFIG_MFD_CS47L24),y) >> obj-$(CONFIG_MFD_ARIZONA) += cs47l24-tables.o >> endif >> + >> +obj-$(CONFIG_MFD_WCD9335) += wcd9335.o >> +wcd9335-objs := wcd9335-core.o >> + >> obj-$(CONFIG_MFD_WM8400) += wm8400-core.o >> wm831x-objs := wm831x-core.o wm831x-irq.o wm831x-otp.o >> wm831x-objs += wm831x-auxadc.o >> diff --git a/drivers/mfd/wcd9335-core.c b/drivers/mfd/wcd9335-core.c >> new file mode 100644 >> index 000000000000..8f746901f4e9 >> --- /dev/null >> +++ b/drivers/mfd/wcd9335-core.c >> @@ -0,0 +1,268 @@ >> +// SPDX-License-Identifier: GPL-2.0 >> +// Copyright (c) 2018, Linaro Limited >> + >> +#include <linux/kernel.h> >> +#include <linux/module.h> >> +#include <linux/slimbus.h> >> +#include <linux/regmap.h> >> +#include <linux/of.h> >> +#include <linux/of_gpio.h> >> +#include <linux/clk.h> >> +#include <linux/gpio.h> >> +#include <linux/mfd/core.h> >> +#include <linux/platform_device.h> >> +#include <linux/regulator/consumer.h> >> +#include <linux/mfd/wcd9335/wcd9335.h> >> +#include <linux/mfd/wcd9335/registers.h> > > Alphabetical. > Sure will do that in next version. >> +static const struct regmap_range_cfg wcd9335_ranges[] = { >> + { .name = "WCD9335", > > What is that? New line please. Opps, will fix it in next version. > >> + .range_min = 0x0, >> + .range_max = WCD9335_MAX_REGISTER, >> + .selector_reg = WCD9335_REG(0x0, 0), >> + .selector_mask = 0xff, >> + .selector_shift = 0, >> + .window_start = 0x0, >> + .window_len = 0x1000, >> + }, >> +}; >> + >> +static struct regmap_config wcd9335_regmap_config = { >> + .reg_bits = 16, >> + .val_bits = 8, >> + .cache_type = REGCACHE_RBTREE, >> + .max_register = WCD9335_MAX_REGISTER, >> + .can_multi_write = true, >> + .ranges = wcd9335_ranges, >> + .num_ranges = ARRAY_SIZE(wcd9335_ranges), >> +}; >> + >> +static const struct regmap_range_cfg wcd9335_ifd_ranges[] = { >> + { .name = "WCD9335-IFD", > > As above. > Yep. >> + .range_min = 0x0, >> + .range_max = WCD9335_REG(0, 0x7ff), >> + .selector_reg = WCD9335_REG(0, 0x0), >> + .selector_mask = 0xff, >> + .selector_shift = 0, >> + .window_start = 0x0, >> + .window_len = 0x1000, >> + }, >> +}; >> + >> +static struct regmap_config wcd9335_ifd_regmap_config = { >> + .reg_bits = 16, >> + .val_bits = 8, >> + .can_multi_write = true, >> + .max_register = WCD9335_REG(0, 0x7FF), >> + .ranges = wcd9335_ifd_ranges, >> + .num_ranges = ARRAY_SIZE(wcd9335_ifd_ranges), >> +}; >> + >> +static int wcd9335_parse_dt(struct wcd9335 *wcd) >> +{ >> + struct device *dev = wcd->dev; >> + struct device_node *np = dev->of_node; >> + int ret; >> + >> + wcd->reset_gpio = of_get_named_gpio(np, "reset-gpio", 0); >> + if (wcd->reset_gpio < 0) { >> + dev_err(dev, "Reset gpio missing in DT\n"); > > s/gpio/GPIO/ > > s/missing in DT/missing from DT/ Yep! > >> + return wcd->reset_gpio; >> + } >> + >> + wcd->mclk = devm_clk_get(dev, "mclk"); >> + if (IS_ERR(wcd->mclk)) { >> + dev_err(dev, "mclk not found\n"); >> + return PTR_ERR(wcd->mclk); >> + } >> + >> + wcd->native_clk = devm_clk_get(dev, "slimbus"); >> + if (IS_ERR(wcd->native_clk)) { >> + dev_err(dev, "slimbus clk not found\n"); > > Any reason for abbreviating 'clock' in the error message? > No, Will change it to clock in next version. >> + return PTR_ERR(wcd->native_clk); >> + } >> + >> + wcd->supplies[0].supply = "vdd-buck"; >> + wcd->supplies[1].supply = "vdd-buck-sido"; >> + wcd->supplies[2].supply = "vdd-tx"; >> + wcd->supplies[3].supply = "vdd-rx"; >> + wcd->supplies[4].supply = "vdd-io"; >> + >> + ret = regulator_bulk_get(dev, WCD9335_MAX_SUPPLY, wcd->supplies); >> + if (ret != 0) { > > "if (ret)" > > Same for all. I won't comment on the others. Yes, I agree, will fix such instances. > >> + dev_err(dev, "Failed to get supplies: err = %d\n", ret); >> + return ret; >> + } >> + >> + return 0; >> +} >> + >> +static int wcd9335_power_on_reset(struct wcd9335 *wcd) >> +{ >> + struct device *dev = wcd->dev; >> + int ret; >> + >> + ret = regulator_bulk_enable(WCD9335_MAX_SUPPLY, wcd->supplies); >> + if (ret != 0) { >> + dev_err(dev, "Failed to get supplies: err = %d\n", ret); >> + return ret; >> + } >> + >> + /* >> + * For WCD9335, it takes about 600us for the Vout_A and >> + * Vout_D to be ready after BUCK_SIDO is powered up. >> + * SYS_RST_N shouldn't be pulled high during this time >> + */ >> + usleep_range(600, 650); >> + >> + gpio_direction_output(wcd->reset_gpio, 0); >> + msleep(20); > > What's this for? Why can't you just: > This is just making sure that reset line is low before actual the chip is taken out of reset. > gpio_direction_output(wcd->reset_gpio, 1); > > ... and drop the next 2 lines? > >> + gpio_set_value(wcd->reset_gpio, 1); >> + msleep(20); >> + >> + return 0; >> +} >> + >> +static int wcd9335_bring_up(struct wcd9335 *wcd) >> +{ >> + struct regmap *rm = wcd->regmap; >> + int val, byte0; >> + int ret = 0; >> + >> + regmap_read(rm, WCD9335_CHIP_TIER_CTRL_EFUSE_VAL_OUT0, &val); >> + regmap_read(rm, WCD9335_CHIP_TIER_CTRL_CHIP_ID_BYTE0, &byte0); >> + >> + if ((val < 0) || (byte0 < 0)) { >> + dev_err(wcd->dev, "wcd9335 codec version detection fail!\n"); > > s/wcd9335 codec/WCD9335 CODEC/ ? > sure! >> + return -EINVAL; >> + } >> + >> + if (byte0 == 0x1) { >> + dev_info(wcd->dev, "wcd9335 codec version is v2.0\n"); >> + wcd->version = WCD9335_VERSION_2_0; >> + regmap_write(rm, WCD9335_CODEC_RPM_RST_CTL, 0x01); >> + regmap_write(rm, WCD9335_SIDO_SIDO_TEST_2, 0x00); >> + regmap_write(rm, WCD9335_SIDO_SIDO_CCL_8, 0x6F); >> + regmap_write(rm, WCD9335_BIAS_VBG_FINE_ADJ, 0x65); >> + regmap_write(rm, WCD9335_CODEC_RPM_PWR_CDC_DIG_HM_CTL, 0x5); >> + regmap_write(rm, WCD9335_CODEC_RPM_PWR_CDC_DIG_HM_CTL, 0x7); >> + regmap_write(rm, WCD9335_CODEC_RPM_PWR_CDC_DIG_HM_CTL, 0x3); >> + regmap_write(rm, WCD9335_CODEC_RPM_RST_CTL, 0x3); >> + } else { >> + dev_err(wcd->dev, "wcd9335 codec version not supported\n"); > > As above. > Yep. >> + ret = -EINVAL; > > Why not just > > return -EINVAL; > > Then you can just return 0 and avoid pre-initialising ret. Agreed! > >> + } >> + >> + return ret; >> +} >> + >> +static int wcd9335_slim_probe(struct slim_device *slim) >> +{ >> + struct device *dev = &slim->dev; >> + struct wcd9335 *wcd; >> + int ret = 0; > > Why pre-initialise? looks like over done! > >> + /* Interface device */ >> + if (slim->e_addr.dev_index == 0) > > Is 0 the parent? I think 0 needs defining for clarity. Sure, Its interface device index, I will try to make this clear by defining ir. > >> + return 0; >> + >> + wcd = devm_kzalloc(dev, sizeof(*wcd), GFP_KERNEL); >> + if (!wcd) >> + return -ENOMEM; >> + >> + wcd->dev = dev; > > '\n' here. > Yep. >> + ret = wcd9335_parse_dt(wcd); >> + if (ret) { >> + dev_err(dev, "Error parsing DT (%d)\n", ret); > > This format changes from message to message. > > Please pick one and stick with it. > > I personally like: "<MESSAGE>: %d", ret Okay I will keep a close eye on such things before sending next version. > >> + return ret; >> + } >> + >> + ret = wcd9335_power_on_reset(wcd); >> + if (ret) { >> + dev_err(dev, "Error Powering\n"); > > I think this is superflouous since you already printed a message. > Yep! >> + return ret; >> + } >> + >> + wcd->regmap = regmap_init_slimbus(slim, &wcd9335_regmap_config); >> + if (IS_ERR(wcd->regmap)) { >> + ret = PTR_ERR(wcd->regmap); >> + dev_err(dev, "Failed to allocate register map:%d\n", ret); > > A different format again. Ensure there is a ' ' after the ':'. > >> + return ret; >> + } >> + >> + dev_set_drvdata(dev, wcd); > > Probably nicer to do this at the very end - after a '\n'. > Okay. >> + wcd->slim = slim; >> + wcd->intf_type = WCD9335_INTERFACE_TYPE_SLIMBUS; >> + >> + return 0; >> +} >> + >> +static const struct mfd_cell wcd9335_devices[] = { >> + { >> + .name = "wcd9335-codec", >> + }, >> +}; > > When are you going to add the other devices? > We are trying to sort of hw setup to test other devices like soundwire controller, will add these once we are in a position to test them. > By the way, one line entries should be placed on one line. > >> + { .name = "wcd9335-codec" }, > >> +static int wcd9335_slim_status(struct slim_device *sdev, >> + enum slim_device_status s) > > 's' is not a good variable name. Suggest 'status'. > Agreed! >> +{ >> + struct device_node *ifd_np; > > What's 'ifd'? > SLIMbus Interface device. >> + struct wcd9335 *wcd; >> + struct device *dev; >> + int ret; >> + >> + /* Interface device */ > > As previously suggested just define the device ID and drop the > comment. > Yep. >> + if (sdev->e_addr.dev_index == 0) >> + return 0; >> + >> + wcd = dev_get_drvdata(&sdev->dev); >> + dev = wcd->dev; > > Odd. You do to the effort of assigning this and use 'wcd->dev' at > most call sites anyway. I'd drop 'dev' and just use it from 'wcd' > everywhere. > Will cleanup such instances! >> + ifd_np = of_parse_phandle(wcd->dev->of_node, "qcom,ifd", 0); >> + if (!ifd_np) { >> + dev_err(wcd->dev, "No Interface device found\n"); >> + return -EINVAL; >> + } >> + >> + wcd->slim_ifd = of_slim_get_device(sdev->ctrl, ifd_np); >> + if (!wcd->slim_ifd) { >> + dev_err(wcd->dev, "Unable to get SLIM Interface device\n"); >> + return -EINVAL; >> + } >> + >> + wcd->ifd_regmap = regmap_init_slimbus(wcd->slim_ifd, >> + &wcd9335_ifd_regmap_config); >> + if (IS_ERR(wcd->ifd_regmap)) { >> + dev_err(dev, "Failed to allocate register map\n"); >> + return PTR_ERR(wcd->ifd_regmap); >> + } >> + >> + ret = wcd9335_bring_up(wcd); >> + if (ret) { >> + dev_err(dev, "Failed to bringup WCD9335\n"); >> + return ret; >> + } >> + >> + wcd->slim_ifd = wcd->slim_ifd; > > Am I missing something? No, Its my bad! > >> + return mfd_add_devices(wcd->dev, 0, wcd9335_devices, >> + ARRAY_SIZE(wcd9335_devices), NULL, 0, NULL); > > No error message if it were to fail? Will fix this in next version. > >> +} >> + >> +static const struct slim_device_id wcd9335_slim_id[] = { >> + {0x217, 0x1a0, 0x1, 0x0}, > > Well that's just horrific. Can't these be defined? > Possibly, This is 48bit enumeration address of SLIMbus device, These correspond to Manufacture ID, Product-ID, and Device-ID and Instance IDs. >> + {} >> +}; >> + >> +static struct slim_driver wcd9335_slim_driver = { >> + .driver = { >> + .name = "wcd9335-slim", >> + }, >> + .probe = wcd9335_slim_probe, >> + .device_status = wcd9335_slim_status, >> + .id_table = wcd9335_slim_id, >> +}; >> + >> +module_slim_driver(wcd9335_slim_driver); >> +MODULE_DESCRIPTION("WCD9335 slim driver"); >> +MODULE_LICENSE("GPL v2"); >> diff --git a/include/linux/mfd/wcd9335/registers.h b/include/linux/mfd/wcd9335/registers.h > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 02 Aug 2018, Srinivas Kandagatla wrote: > On 02/08/18 09:33, Lee Jones wrote: > > On Fri, 27 Jul 2018, Srinivas Kandagatla wrote: > > > > > Qualcomm WCD9335 Codec is a standalone Hi-Fi audio codec IC, > > > It has mulitple blocks like Soundwire controller, codec, > > > Codec processing engine, ClassH controller, interrupt mux. > > > It supports both I2S/I2C and SLIMbus audio interfaces. > > > > > > This patch adds support to SLIMbus audio interface. > > > > > > Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> > > > --- > > > drivers/mfd/Kconfig | 18 ++ > > > drivers/mfd/Makefile | 4 + > > > drivers/mfd/wcd9335-core.c | 268 ++++++++++++++++ > > > include/linux/mfd/wcd9335/registers.h | 580 ++++++++++++++++++++++++++++++++++ > > > include/linux/mfd/wcd9335/wcd9335.h | 42 +++ > > > 5 files changed, 912 insertions(+) > > > create mode 100644 drivers/mfd/wcd9335-core.c > > > create mode 100644 include/linux/mfd/wcd9335/registers.h > > > create mode 100644 include/linux/mfd/wcd9335/wcd9335.h > > > > > > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig > > > index f3fa516011ec..6e5b5f3cfe20 100644 > > > --- a/drivers/mfd/Kconfig > > > +++ b/drivers/mfd/Kconfig > > > @@ -1807,6 +1807,24 @@ config MFD_WM97xx > > > support for the WM97xx, in order to use the actual functionaltiy of > > > the device other drivers must be enabled. > > > +config MFD_WCD9335 > > > + tristate > > > + select MFD_CORE > > > + select REGMAP > > > + select REGMAP_IRQ > > > + > > > +config MFD_WCD9335_SLIM > > > + tristate "Qualcomm WCD9335 with SLIMbus" > > > + select MFD_WCD9335 > > > + select REGMAP_SLIMBUS > > > + depends on SLIMBUS > > > + help > > > + The WCD9335 is a standalone Hi-Fi audio codec IC, supports > > > > s/codec/CODEC/ > Yep. If you agree with something, best just to snip it. No need to reply at all if you agree with everything. [...] > > > +static int wcd9335_power_on_reset(struct wcd9335 *wcd) > > > +{ > > > + struct device *dev = wcd->dev; > > > + int ret; > > > + > > > + ret = regulator_bulk_enable(WCD9335_MAX_SUPPLY, wcd->supplies); > > > + if (ret != 0) { > > > + dev_err(dev, "Failed to get supplies: err = %d\n", ret); > > > + return ret; > > > + } > > > + > > > + /* > > > + * For WCD9335, it takes about 600us for the Vout_A and > > > + * Vout_D to be ready after BUCK_SIDO is powered up. > > > + * SYS_RST_N shouldn't be pulled high during this time > > > + */ > > > + usleep_range(600, 650); > > > + > > > + gpio_direction_output(wcd->reset_gpio, 0); > > > + msleep(20); > > > > What's this for? Why can't you just: > > > This is just making sure that reset line is low before actual the chip is > taken out of reset. If it confused me, it might confuse others. Best to add a comment to the tune of: "Toggle reset line off/on to ensure ... " > > gpio_direction_output(wcd->reset_gpio, 1); > > > > ... and drop the next 2 lines? [...] > > > +static const struct mfd_cell wcd9335_devices[] = { > > > + { > > > + .name = "wcd9335-codec", > > > + }, > > > +}; > > > > When are you going to add the other devices? > > > We are trying to sort of hw setup to test other devices like soundwire > controller, will add these once we are in a position to test them. Ensure that you do, or I'll revert the whole driver as "not an MFD" :) > > By the way, one line entries should be placed on one line. > > > > > + { .name = "wcd9335-codec" }, [...] > > > +static int wcd9335_slim_status(struct slim_device *sdev, > > > + enum slim_device_status s) > > > > 's' is not a good variable name. Suggest 'status'. > > > Agreed! > > > +{ > > > + struct device_node *ifd_np; > > > > What's 'ifd'? > > > SLIMbus Interface device. That's a terrible variable name. :) -- Lee Jones [李琼斯] Linaro Services Technical Lead Linaro.org │ Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Aug 2, 2018 at 1:33 AM Srinivas Kandagatla <srinivas.kandagatla@linaro.org> wrote: > > Thanks for review, > > On 01/08/18 23:20, Rob Herring wrote: > >>>> +- qcom,ifd: > >>>> + Usage: required > >>>> + Value type: <phandle> > >>>> + Definition: SLIM interface device > >>> Wouldn't this be the parent? > >>> > >> No, Interface device is just like other slim device and is part of > >> SLIMbus Component and has a unique address. Every SLIMbus component has > >> one interface device. > > I still don't understand what this means. If this is SLIMbus specific, > > then maybe it should be named that way? Or it is QCom specific? > > It is SLIMbus specific, I will rename this to "slim,ifd" in next version > which makes it clear! 'slim' is not a vendor. So 'slim-ifc-dev' perhaps. Unless IFD is a well known acronym for someone familiar with SLIMbus. Rob -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html