Re: [alsa-devel] [PATCH 06/10] mfd: Add core driver for AD242x A2B transceivers

[Daniel Mack]

Hi Pierre,

Thanks for looking into this!

On 12/17/19 8:16 PM, Pierre-Louis Bossart wrote:
> is the datasheet public? I thought it was only available under NDA.

It was until recently, but it is now public:


https://www.analog.com/media/en/technical-documentation/user-guides/AD242x_TRM_Rev1.1.pdf

>> +ÂÂÂ master->sync_clk = devm_clk_get(dev, "sync");
>> +ÂÂÂ if (IS_ERR(master->sync_clk)) {
>> +ÂÂÂÂÂÂÂ ret = PTR_ERR(master->sync_clk);
>> +ÂÂÂÂÂÂÂ if (ret != -EPROBE_DEFER)
>> +ÂÂÂÂÂÂÂÂÂÂÂ dev_err(dev, "failed to get sync clk: %d\n", ret);
>> +
>> +ÂÂÂÂÂÂÂ return ret;
>> +ÂÂÂ }
>> +
>> +ÂÂÂ if (of_property_read_u32(dev->of_node, "clock-frequency",
>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ &master->sync_clk_rate)) {
>> +ÂÂÂÂÂÂÂ ret = clk_set_rate(master->sync_clk, master->sync_clk_rate);
> 
> shouldn't you check the rate before setting it?
>
>> +ÂÂÂÂÂÂÂ if (ret < 0) {
>> +ÂÂÂÂÂÂÂÂÂÂÂ dev_err(dev, "Cannot set sync clock rate: %d\n", ret);
>> +ÂÂÂÂÂÂÂÂÂÂÂ return ret;
>> +ÂÂÂÂÂÂÂ }
>> +ÂÂÂ }
>> +
>> +ÂÂÂ master->sync_clk_rate = clk_get_rate(master->sync_clk);
>> +ÂÂÂ if (master->sync_clk_rate != 44100 && master->sync_clk_rate !=
>> 48000) {
>> +ÂÂÂÂÂÂÂ dev_err(dev, "SYNC clock rate %d is invalid\n",
>> +ÂÂÂÂÂÂÂÂÂÂÂ master->sync_clk_rate);
>> +ÂÂÂÂÂÂÂ return -EINVAL;
>> +ÂÂÂ }
> 
> this is a bit odd, you set the rate in case there is a property but get
> it anyways. the last block could be an else?

The idea is: if 'clock-frequency' is given, we use it to set the clock,
otherwise we rely on the clock having one of the two allowed rates. This
way, we also catch setups where the clock provider cannot generated the
desired frequency, or where the value of 'clock-frequency' is illegal.

>> +ÂÂÂ ret = clk_prepare_enable(master->sync_clk);
>> +ÂÂÂ if (ret < 0) {
>> +ÂÂÂÂÂÂÂ dev_err(dev, "failed to enable sync clk: %d\n", ret);
>> +ÂÂÂÂÂÂÂ return ret;
>> +ÂÂÂ }
>> +
>> +ÂÂÂ /* Master node setup */
>> +
>> +ÂÂÂ ret = regmap_write(regmap, AD242X_CONTROL,
>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂ AD242X_CONTROL_MSTR | AD242X_CONTROL_SOFTRST);
>> +ÂÂÂ if (ret < 0)
>> +ÂÂÂÂÂÂÂ return ret;
>> +
>> +ÂÂÂ ret = ad242x_wait_for_irq(master, &master->run_completion, 10);
> 
> what is 10?

Milliseconds. The parameter needs to get a better name I figure.


Thanks,
Daniel