[alsa-devel] [PATCH v4 1/2] ASoC: cs35l36: Add support for Cirrus CS35L36 Amplifier

[Schulman, James]

On Fri, 21 Dec 2018, Charles Keepax wrote:

> On Thu, Dec 20, 2018 at 01:41:15PM -0600, James Schulman wrote:
>> Add driver support for Cirrus Logic CS35L36 boosted
>> speaker amplifier
>>
>> Signed-off-by: James Schulman <james.schulman at cirrus.com>
>> ---
>> +struct cs35l36_platform_data {
>> +	bool sclk_frc;
>> +	bool lrclk_frc;
>
> These two are now unused.
>
>> +	bool multi_amp_mode;
>> +	bool dcm_mode;
>> +	int ldm_mode_sel;
>> +	bool amp_gain_zc;
>
> As is this guy.
>
>> +	bool amp_pcm_inv;
>> +	bool pdm_ldm_exit;
>> +	bool pdm_ldm_enter;
>
> And these two.
>
>> +	bool imon_pol_inv;
>> +	bool vmon_pol_inv;
>> +	int boost_ind;
>> +	int bst_vctl;
>> +	int bst_vctl_sel;
>> +	int bst_ipk;
>> +	bool extern_boost;
>> +	int temp_warn_thld;
>> +	struct cs35l36_vpbr_cfg vpbr_config;
>> +	struct cs35l36_irq_cfg irq_config;
>> +};
>
>> +
>> +	/* INT/GPIO Pin Config */
>> +	irq_gpio = of_get_child_by_name(np, "cirrus,irq-config");
>> +	irq_gpio_config->is_present = irq_gpio ? true : false;
>
> Should this be based off the interrupts property itself? The
> defaults for both of the properties in this block seem quite sane
> so it seems like people might use the IRQ but not specify this
> block.
>

Thanks a ton Charles. I'm just going to get rid of the irq_cfg struct 
altogether. It's not necessary now that we've implemented the majority 
from the 'interrupts' property. I'll keep two properties for the 
irq-drive-select and the irq-gpio-select, but the rest of it we'll just 
optimize out.

>> +	if (irq_gpio_config->is_present) {
>> +		if (of_property_read_u32(irq_gpio, "cirrus,irq-drive-select",
>> +					 &val) >= 0)
>> +			irq_gpio_config->irq_drv_sel = val;
>> +		if (of_property_read_u32(irq_gpio, "cirrus,irq-gpio-select",
>> +					 &val) >= 0)
>> +			irq_gpio_config->irq_gpio_sel = val;
>> +
>> +		irq_d = irq_get_irq_data(i2c_client->irq);
>> +		if (!irq_d) {
>> +			dev_err(&i2c_client->dev, "Invalid IRQ: %d\n",
>> +				i2c_client->irq);
>> +			return -EINVAL;
>> +		}
>> +
>> +		irq_gpio_config->irq_pol = irqd_get_trigger_type(irq_d);
>> +	}
>
>> +
>> +	dev_info(&i2c_client->dev, "Cirrus Logic CS35L%d, Revision: %02X\n",
>> +		 cs35l36->chip_version, reg_revid >> 8);
>> +
>> +	ret =  snd_soc_register_component(dev, &soc_component_dev_cs35l36,
>> +					  cs35l36_dai, ARRAY_SIZE(cs35l36_dai));
>
> Could use devm_ here.
>
> Fix up those and you can add my:
>
> Reviewed-by: Charles Keepax <ckeepax at opensource.cirrus.com>
>
> Thanks,
> Charles
>

Awesome, thank you! I think enough will change that I'll wait before 
adding the 'Reviewed-by'.

Thanks,
James