Re: [PATCH v2 1/4] dt-bindings: mailbox: Add mediatek,gce-props.yaml

["Jason-JH Lin (林睿祥)" <Jason-JH.Lin@mediatek.com>]

Hi Krzysztof,

Thanks for the reviews.

On Fri, 2024-01-12 at 08:20 +0100, Krzysztof Kozlowski wrote:
>  	 
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
>  On 10/01/2024 07:35, Jason-JH.Lin wrote:
> > Add mediatek,gce-props.yaml for specific GCE properties for both
> > Mailbox Providers and Consumers.
> > 
> > Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com>
> > ---
> >  .../bindings/mailbox/mediatek,gce-props.yaml  | 41
> +++++++++++++++++++
> >  1 file changed, 41 insertions(+)
> >  create mode 100644
> Documentation/devicetree/bindings/mailbox/mediatek,gce-props.yaml
> > 
> > diff --git
> a/Documentation/devicetree/bindings/mailbox/mediatek,gce-props.yaml
> b/Documentation/devicetree/bindings/mailbox/mediatek,gce-props.yaml
> > new file mode 100644
> > index 000000000000..aac776b74e88
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/mailbox/mediatek,gce-
> props.yaml
> > @@ -0,0 +1,41 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: 
> http://devicetree.org/schemas/mailbox/mediatek,gce-props.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: MediaTek Global Command Engine common propertes for both
> Mailbox Providers and Consumers.
> 
> That's way too long. Please observe coding style wrapping. Just
> MediaTek
> Global Command Engine Common Propertes
> (pay attention to capitalization)
> 

OK, Thanks for the fixing title text.
I'll change it.

> > +
> > +maintainers:
> > +  - Houlong Wei <houlong.wei@mediatek.com>
> > +
> > +description:
> > +  The Global Command Engine (GCE) is used to implement a Command
> Queue (CMDQ)
> > +  driver using the Linux Mailbox framework. The GCE is an
> instruction based,
> > +  multi-threaded, single-core command dispatcher for MediaTek
> hardware.
> > +  We use GCE Mailbox binding to define GCE core properties for GCE
> Mailbox Provider.
> > +  A device that uses the CMDQ driver to confige its hardware
> registers by requesting
> 
> configure?

Yes, that typo didn't scan out.
Thanks for the correction.

> 
> > +  the Linux Mailbox Channels in the GCE Mailbox Controller is a
> Mailbox Consumer.
> > +  This binding defines the common GCE properties for both Mailbox
> Providers and Consumers.
> 
> Last four sentences: a lot of text yet absolutely no meaning. I still
> do
> not understand why same property is shared between provider and
> consumer.

OK, I'll add more description for that.

The main reason is that the property is used for GCE HW event ID
corresponding to a HW event signal. If the provider wants to get or set
an event value of the event ID by operating GCE HW register. It needs
to know what the event ID is.

> 
> Also, wrap according to Linux coding style.
> 
OK, I'll shrink them to less then 80 columns.

> > +
> > +properties:
> > +  mediatek,gce-events:
> > +    description:
> > +      Each gce-events is an event id corresponding to a specific
> hardware event
> > +      signal sent to GCE. The event id is defined in the GCE
> header
> > +      include/dt-bindings/gce/<chip>-gce.h of each chips.
> > +      CMDQ client drivers have two usage of GCE event signals,
> > +      one is sfotware tokens and the other is hardware events.
> > +      Software tokens refer to GCE event signals triggered by
> drivers.
> > +      e.g. software drivers append GCE commands to set a GCE event
> after specific
> > +      GCE commands. Or CMDQ client driver uses the CPU to write
> the event id
> > +      into GCE register to trigger the GCE event corresponding to
> the event id.
> > +      Hardware events refer to GCE event signals triggered by
> hardware engines.
> > +      e.g. When OVL fetches all the data in the frame buffer, OVL
> will send
> 
> There is no full stop before "e.g." and no new sentence. Please run
> this
> through grammar check, like Google, ChatGPT or human.
> 
OK. Thanks for the suggestion.
I'll correct them.

Regards,
Jason-JH.Lin

> > +      a frame done irq and send a frame done GCE event via
> hardware bus directly
> > +      at the same time.
> > +    $ref: /schemas/types.yaml#/definitions/uint32-array
> > +    minItems: 1
> > +    maxItems: 1024
> 
> 
> Best regards,
> Krzysztof
> 
>