Re: [PATCH v4 1/6] dt-bindings: media: platform: visconti: Add Toshiba Visconti Video Input Interface bindings

[Krzysztof Kozlowski <krzk@kernel.org>]

On 11/01/2023 14:21, yuji2.ishikawa@toshiba.co.jp wrote:
> Hello Krzysztof,
> 
> Sorry, I missed your comments following the topic of recipients list.
> 
> Below are the rest of the responses
> 
>> -----Original Message-----
>> From: Krzysztof Kozlowski <krzk@kernel.org>
>> Sent: Wednesday, January 11, 2023 4:31 AM
>> To: ishikawa yuji(石川 悠司 ○ＲＤＣ□ＡＩＴＣ○ＥＡ開)
>> <yuji2.ishikawa@toshiba.co.jp>; Hans Verkuil <hverkuil@xs4all.nl>; Laurent
>> Pinchart <laurent.pinchart@ideasonboard.com>; Mauro Carvalho Chehab
>> <mchehab@kernel.org>; iwamatsu nobuhiro(岩松 信洋 □ＳＷＣ◯ＡＣＴ)
>> <nobuhiro1.iwamatsu@toshiba.co.jp>
>> Cc: linux-media@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
>> linux-kernel@vger.kernel.org; devicetree@vger.kernel.org
>> Subject: Re: [PATCH v4 1/6] dt-bindings: media: platform: visconti: Add Toshiba
>> Visconti Video Input Interface bindings
>>
>> On 10/01/2023 02:41, Yuji Ishikawa wrote:
>>> Adds the Device Tree binding documentation that allows to describe the
>>> Video Input Interface found in Toshiba Visconti SoCs.
>>>
>>> Signed-off-by: Yuji Ishikawa <yuji2.ishikawa@toshiba.co.jp>
>>> Reviewed-by: Nobuhiro Iwamatsu <nobuhiro1.iwamatsu@toshiba.co.jp>
>>
>> Please use scripts/get_maintainers.pl to get a list of necessary people and lists
>> to CC.  It might happen, that command when run on an older kernel, gives you
>> outdated entries.  Therefore please be sure you base your patches on recent
>> Linux kernel.
>>
>> You missed few of them, so clearly this was not sent correctly.
>>
>>
>> Subject: drop second/last, redundant "bindings". The "dt-bindings"
>> prefix is already stating that these are bindings.

Follow all comments.

>>
>>> ---
>>> Changelog v2:
>>> - no change
>>>
>>> Changelog v3:
>>> - no change
>>>
>>> Changelog v4:
>>> - fix style problems at the v3 patch
>>> - remove "index" member
>>> - update example
>>> ---
>>>  .../bindings/media/toshiba,visconti-viif.yaml | 98
>>> +++++++++++++++++++
>>>  1 file changed, 98 insertions(+)
>>>  create mode 100644
>>> Documentation/devicetree/bindings/media/toshiba,visconti-viif.yaml
>>>
>>> diff --git
>>> a/Documentation/devicetree/bindings/media/toshiba,visconti-viif.yaml
>>> b/Documentation/devicetree/bindings/media/toshiba,visconti-viif.yaml
>>> new file mode 100644
>>> index 00000000000..71442724d1a
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/media/toshiba,visconti-viif.ya
>>> +++ ml
>>> @@ -0,0 +1,98 @@
>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) %YAML 1.2
>>> +---
>>> +$id: http://devicetree.org/schemas/media/toshiba,visconti-viif.yaml#
>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>> +
>>> +title: Toshiba Visconti5 SoC Video Input Interface Device Tree
>>> +Bindings
>>
>> Drop "Device Tree Bindings"
> 
> I'll drop it.
> 
>>
>>> +
>>> +maintainers:
>>> +  - Nobuhiro Iwamatsu <nobuhiro1.iwamatsu@toshiba.co.jp>
>>> +
>>> +description:
>>> +  Toshiba Visconti5 SoC Video Input Interface (VIIF)
>>> +  receives MIPI CSI2 video stream,
>>> +  processes the stream with embedded image signal processor (L1ISP,
>>> +L2ISP),
>>> +  then stores pictures to main memory.
>>
>> Fix wrapping.
> 
> I'll fix this.
> I didn't realize a new line has been added through formatting a patch.
>  
>>> +
>>> +properties:
>>> +  compatible:
>>> +    const: toshiba,visconti-viif
>>
>> Compatible must be specific. You called your SoC visconti5, didn't you?
>>
> 
> The Video Input Interface hardware is likely to be used at future SoCs of
> Visconti Architecture.
> Does compatible have to be specific to SoC's model name 
> rather than architecture name?

Compatibles should always be specific to SoC model name. Adding more
generic family fallback is also good idea when it is applicable.

Best regards,
Krzysztof


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel