Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
References: <20230510112349.939991-1-aidanmacdonald.0x0@gmail.com>
 <20230510112349.939991-3-aidanmacdonald.0x0@gmail.com>
 <7b3a37e8-0210-c539-5b5b-bf8e587707ea@linaro.org>
 <7Zga7RBqLNcaG5ylTIY4Qn7CUdE1IJsW@localhost>
 <8e98b31a-ef4d-a8bd-32f3-e6f496f2b138@linaro.org>
From:   Aidan MacDonald <aidanmacdonald.0x0@gmail.com>
To:     Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Cc:     lgirdwood@gmail.com, broonie@kernel.org, robh+dt@kernel.org,
        krzysztof.kozlowski+dt@linaro.org, conor+dt@kernel.org,
        perex@perex.cz, tiwai@suse.com, alsa-devel@alsa-project.org,
        devicetree@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v1 3/3] dt-bindings: ASoC: Add ESS ES9218P codec bindings
Date:   Thu, 11 May 2023 12:26:50 +0100
In-reply-to: <8e98b31a-ef4d-a8bd-32f3-e6f496f2b138@linaro.org>
Message-ID: <zfuvar6MiqIaW9nvjgvUIhA1t3DkZ9tE@localhost>
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk


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.

Regards,
Aidan