On Fri, 27 Jul 2018, Srinivas Kandagatla wrote:

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.

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?

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog