Re: [PATCH 2/6] ASoC: wcd934x: add support to wcd9340/wcd9341 codec

[Srinivas Kandagatla <srinivas.kandagatla@linaro.org>]

Thanks Mark for taking time to review this patch.

On 02/07/2019 15:44, Mark Brown wrote:
> On Tue, Jul 02, 2019 at 09:09:16AM +0100, Srinivas Kandagatla wrote:
> 
>> +#define WCD_VERSION_WCD9341_1_1     5
>> +#define WCD_IS_1_0(wcd) \
>> +	((wcd->type == WCD934X) ? \
>> +	 ((wcd->version == WCD_VERSION_1_0 || \
>> +	   wcd->version == WCD_VERSION_WCD9340_1_0 || \
>> +	   wcd->version == WCD_VERSION_WCD9341_1_0) ? 1 : 0) : 0)
> 
> Eew.  If you really need these make them functions and write
> normal code with switch statements rather than abusing the
> ternery operator like this, it's really not terribly readable.
> 
I agree, will fix this and such use of ternary operators in the code.

>> +static void wcd934x_update_reg_defaults(struct wcd934x_codec *wcd)
>> +{
>> +	struct regmap *rm = wcd->regmap;
>> +
>> +	regmap_update_bits(rm, WCD934X_BIAS_VBG_FINE_ADJ, 0xFF, 0x75);
>> +	regmap_update_bits(rm, WCD934X_CODEC_CPR_SVS_CX_VDD, 0xFF, 0x7C);
> 
> What's all this stuff doing?  Should you be uing a regmap patch?
> 
I will try that in next version.
>> +static int wcd934x_disable_master_bias(struct wcd934x_codec *data)
>> +{
>> +	if (data->master_bias_users <= 0)
>> +		return 0;
>> +
>> +	data->master_bias_users--;
> 
> There's an awful lot of these refcounted things - are you sure
> none of them could be supply widgets?
I did not like it either, will remove them as this might be redundant. 
"MCLK" is already a supply widget.
> 
>> +static void wcd934x_get_version(struct wcd934x_codec *wcd)
>> +{
>> +	int val1, val2, version, ret;
>> +	struct regmap *regmap;
>> +	u16 id_minor;
>> +	u32 version_mask = 0;
>> +
>> +	regmap = wcd->regmap;
>> +	version = 0;
>> +
>> +	ret = regmap_bulk_read(regmap, WCD934X_CHIP_TIER_CTRL_CHIP_ID_BYTE0,
>> +			       (u8 *)&id_minor, sizeof(u16));
>> +
>> +	if (ret)
>> +		return;
> 
> No error reporting at all?
I agree, will fix this in next version.
> 
>> +	regmap_read(regmap, WCD934X_CHIP_TIER_CTRL_EFUSE_VAL_OUT14, &val1);
>> +	regmap_read(regmap, WCD934X_CHIP_TIER_CTRL_EFUSE_VAL_OUT15, &val2);
>> +
>> +	dev_info(wcd->dev, "%s: chip version :0x%x 0x:%x\n",
>> +		 __func__, val1, val2);
> 
> We don't report id_minor as part of the version?  Also the format
> string there just seems mangled and not even internally
> consistent.

> 
>> +	version_mask |= (!!((u8)val1 & 0x80)) << DSD_DISABLED_MASK;
>> +	version_mask |= (!!((u8)val2 & 0x01)) << SLNQ_DISABLED_MASK;
>> +
>> +	switch (version_mask) {
>> +	case DSD_DISABLED | SLNQ_DISABLED:
>> +		if (id_minor == 0)
>> +			version = WCD_VERSION_WCD9340_1_0;
>> +		else if (id_minor == 0x01)
>> +			version = WCD_VERSION_WCD9340_1_1;
> 
> This looks like you're trying to write a switch statement on the
> minor version...
> 
Will move to switch and any such occurrences.

>> +static void wcd934x_update_cpr_defaults(struct wcd934x_codec *data)
>> +{
>> +	int i;
>> +
>> +	__wcd934x_cdc_mclk_enable(data, true);
>> +
>> +	wcd934x_set_sido_input_src(data, SIDO_SOURCE_RCO_BG);
>> +	regmap_write(data->regmap, WCD934X_CODEC_CPR_SVS2_MIN_CX_VDD, 0x2C);
>> +	regmap_update_bits(data->regmap, WCD934X_CODEC_RPM_CLK_GATE,
>> +			   0x10, 0x00);
>> +
>> +	for (i = 0; i < ARRAY_SIZE(cpr_defaults); i++) {
>> +		regmap_bulk_write(data->regmap,
>> +				  WCD934X_CODEC_CPR_WR_DATA_0,
>> +				(u8 *)&cpr_defaults[i].wr_data, 4);
>> +		regmap_bulk_write(data->regmap,
>> +				  WCD934X_CODEC_CPR_WR_ADDR_0,
>> +				(u8 *)&cpr_defaults[i].wr_addr, 4);
> 
> What is "cpr" and should you be using a regmap patch here?  Why
> is this not with the other default updates?  You've got loads of
> random undocumented sequences like this all through the driver,
> are they patches or are they things that should be controllable
> by the user?
It makes sense to have a single default map here, I will do the in next 
version. And regarding user controllable, I will go thru the list once 
again in detail and remove user controllable registers.
> 
> 
>> +static int wcd934x_get_micbias_val(struct device *dev, const char *micbias)
>> +{
>> +	int mv;
>> +
>> +	if (of_property_read_u32(dev->of_node, micbias, &mv))
>> +		mv = WCD934X_DEF_MICBIAS_MV;
>> +
>> +	if (mv < 1000 || mv > 2850)
>> +		mv = WCD934X_DEF_MICBIAS_MV;

> 
>> +	return of_platform_populate(wcd->dev->of_node, NULL, NULL, wcd->dev);
> 
> Why are we doing this?

I will not be using MFD in this instance as most of the resources like 
interrupts, pins, clks, SoundWire are modeled as proper drivers with 
their respective subsystems.

This gives a advantage of reusing those drivers like SoundWire, pinctrl 
on other Qualcomm IPs as well!
Also I did not wanted to have a custom functions or hooks in the 
drivers, so platform bus made much sense for me to use here, which can 
take care of bringing up and tearing down the devices with proper parent 
child relationship.
This will instantiate all the child devices like pinctrl, SoundWire 
Controller and so on.

> 
>> +{
>> +	struct device *dev = wcd->dev;
>> +	struct device_node *np = dev->of_node;
>> +	int ret;
>> +	/*
>> +	 * INTR1 consists of all possible interrupt sources Ear OCP,
> 
> Missing blank line.
> 
Yes, I will fix such instances in the driver in next version.

>> +	 * HPH OCP, MBHC, MAD, VBAT, and SVA
>> +	 * INTR2 is a subset of first interrupt sources MAD, VBAT, and SVA
>> +	 */
>> +	wcd->irq = of_irq_get_byname(wcd->dev->of_node, "intr1");
>> +	if (wcd->irq < 0) {
>> +		if (wcd->irq != -EPROBE_DEFER)
>> +			dev_err(wcd->dev, "Unable to configure IRQ\n");
> 
> It's helpful to print what the error code was, it can help people
> debug things.
I agree!
> 
>> +	wcd->reset_gpio = of_get_named_gpio(np,	"reset-gpios", 0);
>> +	if (wcd->reset_gpio < 0) {
>> +		dev_err(dev, "Reset gpio missing in DT\n");
>> +		return wcd->reset_gpio;
>> +	}
> 
> devm_gpiod_get()
Make sense!

> 
>> +static int wcd934x_bring_up(struct wcd934x_codec *wcd)
>> +{
>> +	struct regmap *wcd_regmap = wcd->regmap;
>> +	u16 id_minor, id_major;
>> +	int ret;
> 
>> +	dev_info(wcd->dev, "%s: wcd9xxx chip id major 0x%x, minor 0x%x\n",
>> +		 __func__, id_major, id_minor);
>> +
> 
> What was with the other verison parsing and printing code?
I will fix this in next version with single place to print the version 
number.


Thanks,
srini
>