Re: [PATCH v2 02/11] mfd: wcd934x: add support to wcd9340/wcd9341 codec

On Mon, 21 Oct 2019, Srinivas Kandagatla wrote:

Thanks Lee for taking time to review.

I agree with most of the style related comments, will fix them in next
version. For others I have replied it inline.

[...]

quoted

quoted

+static int wcd934x_bring_up(struct wcd934x_data *wcd)
+{
+	struct regmap *rm = wcd->regmap;

It's much more common to use 'regmap' or 'map'.

Only reason to make it short here is to save some lines!
If you prefer regmap, I will add that in next version!

I doubt it'd save many (any?) lines.

[...]

quoted

quoted

+static int wcd934x_slim_status(struct slim_device *sdev,
+			       enum slim_device_status status)
+{
+	struct device *dev = &sdev->dev;
+	struct wcd934x_data *wcd;
+	int ret;

This is semantically odd!  Why are you doing most of the
initialisation and bring-up in 'status' and not 'probe'.  Seems
broken to me.

SLIMBus device will not be in a state to communicate before enumeration (at
probe), so all the device initialization is done in status callback where it
is ready for communication.

Why do we need the device to be up *before* calling probe?

This is same with SoundWire Bus as well!

[...]

quoted

quoted

+	struct device *dev;
+	struct clk *extclk;
+	struct regmap *regmap;
+	struct slim_device *sdev;

You don't need 'sdev' and 'dev'.

slim_device instance (sdev) is required by audio codec driver to allocate
stream runtime.

You can extrapolate one from the other.

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