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

[Daniel Mack <daniel@zonque.org>]

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