Re: [PATCH] Fixed the schema binding according to test

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

On 02/02/2023 10:55, Ki-Seok Jo wrote:
>> Thank you for your patch. There is something to discuss/improve.
>>
>> On 02/02/2023 10:07, Kiseok Jo wrote:
>>> Modified according to the writing-schema.rst file and tested.
>>
>> Use imperative, not past tense (Fixed->Fix, Modified->Modify).
> 
> Okay. I got it. I'll do that when I rewrite it. Thanks.
> 
>>>
>>> Signed-off-by: Kiseok Jo <kiseok.jo@irondevice.com>
>>
>> Use subject prefixes matching the subsystem (which you can get for example
>> with `git log --oneline -- DIRECTORY_OR_FILE` on the directory your patch
>> is touching). Therefore it should be:
>> "ASoC: dt-bindings: irondevice,sma1303: Rework binding and add missing
>> properties"
>>
> 
> Oh, thank you for good information. I feel like I still lack a lot.
> I'll apply it. Thanks!
> 
>>
>>> ---
>>>  .../bindings/sound/irondevice,sma1303.yaml    | 46 +++++++++++++++++--
>>>  1 file changed, 43 insertions(+), 3 deletions(-)
>>>
>>> diff --git
>>> a/Documentation/devicetree/bindings/sound/irondevice,sma1303.yaml
>>> b/Documentation/devicetree/bindings/sound/irondevice,sma1303.yaml
>>> index 162c52606635..35d9a046ef75 100644
>>> --- a/Documentation/devicetree/bindings/sound/irondevice,sma1303.yaml
>>> +++ b/Documentation/devicetree/bindings/sound/irondevice,sma1303.yaml
>>> @@ -10,22 +10,62 @@ maintainers:
>>>    - Kiseok Jo <kiseok.jo@irondevice.com>
>>>
>>>  description:
>>> -  SMA1303 digital class-D audio amplifier with an integrated boost
>> converter.
>>> +  SMA1303 digital class-D audio amplifier  with an integrated boost
>>> + converter.
>>>
>>>  allOf:
>>> -  - $ref: name-prefix.yaml#
>>> +  - $ref: dai-common.yaml#
>>> +
>>> +properties:
>>> +  compatible:
>>> +    enum:
>>> +      - irondevice,sma1303
>>> +
>>> +  reg:
>>> +    maxItems: 1
>>> +
>>> +  '#sound-dai-cells':
>>> +    const: 1
>>> +
>>> +  i2c-retry:
>>> +    description: number of retries for I2C regmap.
>>
>> Why do you need this? Why this fits the purpose of DT (or IOW why this
>> differs between boards)?
> 
> When working with drivers on mulitiple platforms, there were cases where
> I2C did not work properly dpending on the AP or setting.
> So I made it possible to set a few retry settings, and then check or do
> other actions. Retry is performed only when I2C fails.
> 
> And each device may have a different pull-up resisor or strength,
> so there may be differences between boards.

None of I2C drivers need it (except SBS battery), so it should not be
needed here. If you have wrong pin setup, this one should be corrected
instead.

> 
> Could that property be a problem?
> 
>>> +    maximum: 49
>>> +    default: 3
>>> +
>>> +  tdm-slot-rx:
>>> +    description: set the tdm rx start slot.
>>
>> Aren't you now re-writing dai-tdm-slot-rx-mask property? Same for tx below.
>>
> 
> It can be the same as audio DAI's tdm slot, I think but there are cases
> where it is set differently, so I omitted it separately.

Unfortunately I still do not understand why do you need it. Use generic
DAI/TDM properties.

> 
>>> +    maximum: 7
>>> +    default: 0
>>> +
>>> +  tdm-slot-tx:
>>> +    description: set the tdm tx start slot.
>>> +    maximum: 7
>>> +    default: 0
>>> +
>>> +  sys-clk-id:
>>> +    description: select the using system clock.
>>
>> What does it mean? Why do you need such property instead of clocks?
> 
> This can receive an external clock, but it can use internal clock.
> Should I write all the clock descriptions in case?

How do you configure and enable external clock with this property? I
don't see it. If the device has clock input, this should be "clocks". If
it is omitted, then internal clock is used.



Best regards,
Krzysztof