Re: [PATCH v1 3/3] dt-bindings: ASoC: Add ESS ES9218P codec bindings

[Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>]

On 11/05/2023 13:26, Aidan MacDonald wrote:
> 
> Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> writes:
> 
>> On 11/05/2023 12:15, Aidan MacDonald wrote:
>>>
>>> Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> writes:
>>>
>>>> On 10/05/2023 13:23, Aidan MacDonald wrote:
>>>>> ...
>>>>> +  ess,max-clock-div:
>>>>> +    $ref: /schemas/types.yaml#/definitions/uint32
>>>>> +    description:
>>>>> +      Sets the maximum MCLK divider for generating the internal CLK.
>>>>> +      CLK must be at least 20x the I2C bus speed or I2C transactions
>>>>> +      will fail. The maximum divider should be chosen to ensure that
>>>>> +      CLK will not fall below the limit.
>>>>> +    enum:
>>>>> +      - 1
>>>>> +      - 2
>>>>> +      - 4
>>>>> +      - 8
>>>>> +    default: 1
>>>>
>>>> Why do you need to customize it per board?
>>>>
>>>
>>> There's no generic API to read or change the bus speed (SCL frequency)
>>> for I2C adapters, so it's impossible to calculate a limit on the MCLK
>>> divider automatically.
>>>
>>> Defaulting to 1 is always safe, but means wasting power at lower sample
>>> rates. If you know what the bus speed is you can use a higher divider
>>> limit to save power, and it has to be done at the board/firmware level
>>> because that's the only place where the bus speed is known.
>>
>> If I understand correctly, you only miss a way to get bus_freq_hz from
>> i2c_timings to calculate the divider by yourself? This looks like Linux
>> limitation, so we shouldn't push it into DT, but rather fix Linux. The
>> I2C bus rate is known, the MCLK rate as well, so divider is possible to
>> deduce.
>>
>> I am actually surprised that I2C core does not store the timings
>> anywhere and each bus host has to deal with it on its own.
>>
>> Best regards,
>> Krzysztof
> 
> I agree it'd be better if Linux provided access the bus frequency, but
> even if that API was added it will take time for every I2C adapter to
> support it. So in that case we would still need a DT property to provide
> a safe limit or use a safe default, and miss potential power savings.
> 
> I'd prefer to add the DT property to allow power savings to be had now,
> and drop it if/when the kernel gets an API for bus frequency. That will
> be safe from a compatibility point of view -- the property won't be
> providing any useful information so it won't matter if old DTs have it.

OK, sounds good.

Best regards,
Krzysztof