diff mbox series

[2/5] dt-bindings: media: max9286: Add overlap window

Message ID 20200316202757.529740-3-jacopo+renesas@jmondi.org (mailing list archive)
State New
Delegated to: Kieran Bingham
Headers show
Series media: i2c: max9286: Add configuration properties | expand

Commit Message

Jacopo Mondi March 16, 2020, 8:27 p.m. UTC 
The MAX9286 chip exposes a way to control the 'overlap window'
parameter, most probably used in calculation of the frame
synchronization interval.

When used in conjunction with some serializers, the overlap window has to
be disabled in order to correctly achieve frame sync locking.

As the exact meaning of that control is not documented in the chip's
manual, require all DTS users to specify the value of the window. When,
and if, in future the meaning of control gets clarified and a default
behaviour (window enabled or disabled) can be established, a new boolean
property could supersede this one while being sure that older DTB are
fully specified to avoid confusion.

Provide a few convenience macros for the window disabled and window
default value.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 .../devicetree/bindings/media/i2c/maxim,max9286.yaml  | 11 +++++++++++
 MAINTAINERS                                           |  1 +
 include/dt-bindings/media/maxim-gmsl.h                |  9 +++++++++
 3 files changed, 21 insertions(+)
 create mode 100644 include/dt-bindings/media/maxim-gmsl.h

Comments

Kieran Bingham March 18, 2020, 9:45 a.m. UTC | #1 
Hi Jacopo,

On 16/03/2020 20:27, Jacopo Mondi wrote:
> The MAX9286 chip exposes a way to control the 'overlap window'
> parameter, most probably used in calculation of the frame
> synchronization interval.
> 
> When used in conjunction with some serializers, the overlap window has to
> be disabled in order to correctly achieve frame sync locking.
> 
> As the exact meaning of that control is not documented in the chip's
> manual, require all DTS users to specify the value of the window. When,
> and if, in future the meaning of control gets clarified and a default

/in future/in the future/
/of control/of the control/

> behaviour (window enabled or disabled) can be established, a new boolean
> property could supersede this one while being sure that older DTB are

/DTB/DTBs/

> fully specified to avoid confusion.
> 
> Provide a few convenience macros for the window disabled and window
> default value.

Well it's not the best solution (putting hardcode values into the DTB)
but I agree, without documentation these are almost 'magic values for
the hardware' which is unfortunate.

I do fear this may be the wrong place still though.

This is dependent upon the 'serializer' connected, so is it a property
of the serializer that the max9286 should query at probe time to see
what it has connected?

But that's just back to the whole topic of bus-parameter negotiations
between the serializer and deserializer ...


But with this solution, any dtb segment or overlay for the serializer
needs to modify the max9286 depending on it's requirements ... that
feels a bit odd. Possible, I think, but odd.

Do we have any precedence in existing DT to reference on this topic?




> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  .../devicetree/bindings/media/i2c/maxim,max9286.yaml  | 11 +++++++++++
>  MAINTAINERS                                           |  1 +
>  include/dt-bindings/media/maxim-gmsl.h                |  9 +++++++++
>  3 files changed, 21 insertions(+)
>  create mode 100644 include/dt-bindings/media/maxim-gmsl.h
> 
> diff --git a/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml b/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
> index f9d3e5712c59..ee8e0418b3f0 100644
> --- a/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
> +++ b/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
> @@ -46,6 +46,14 @@ properties:
>      description: GPIO connected to the \#PWDN pin with inverted polarity
>      maxItems: 1
>  
> +  # Until the overlap window control gets not clarified, require dts
> +  # to set its value explicitly,
> +  maxim,overlap-window:
> +    description: Overlap window duration, in pixel clock cycles.
> +    maxItems: 1
> +    allOf:
> +      - $ref: /schemas/types.yaml#/definitions/uint32
> +
>    ports:
>      type: object
>      description: |
> @@ -146,6 +154,7 @@ properties:
>  required:
>    - compatible
>    - reg
> +  - maxim,overlap-window
>    - ports
>    - i2c-mux
>  
> @@ -154,6 +163,7 @@ additionalProperties: false
>  examples:
>    - |
>      #include <dt-bindings/gpio/gpio.h>
> +    #include <dt-bindings/media/maxim-gmsl.h>
>  
>      i2c@e66d8000 {
>        #address-cells = <1>;
> @@ -166,6 +176,7 @@ examples:
>          reg = <0x2c>;
>          poc-supply = <&camera_poc_12v>;
>          enable-gpios = <&gpio 13 GPIO_ACTIVE_HIGH>;
> +        maxim,overlap-window = MAX9286_OVLP_WINDOW_DISABLED;
>  
>          ports {
>            #address-cells = <1>;
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 21a9ff4fe684..3d2455085c80 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -10190,6 +10190,7 @@ M:	Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
>  L:	linux-media@vger.kernel.org
>  S:	Maintained
>  F:	Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
> +F:	include/dt-bindings/media/maxim-gmsl.h
>  F:	drivers/media/i2c/max9286.c
>  
>  MAX9860 MONO AUDIO VOICE CODEC DRIVER
> diff --git a/include/dt-bindings/media/maxim-gmsl.h b/include/dt-bindings/media/maxim-gmsl.h
> new file mode 100644
> index 000000000000..47945ffc3a4d
> --- /dev/null
> +++ b/include/dt-bindings/media/maxim-gmsl.h
> @@ -0,0 +1,9 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef _DT_BINDINGS_MEDIA_MAXIM_GMSL_H
> +#define _DT_BINDINGS_MEDIA_MAXIM_GMSL_H
> +
> +/* MAX9286 default overlap values. */
> +#define MAX9286_OVLP_WINDOW_DISABLED	<0>
> +#define MAX9286_OVLP_WINDOW_DEFAULT	<0x1680>
> +
> +#endif /* _DT_BINDINGS_MEDIA_MAXIM_GMSL_H */
>
Jacopo Mondi March 18, 2020, 2:19 p.m. UTC | #2 
Hi Kieran,

On Wed, Mar 18, 2020 at 09:45:14AM +0000, Kieran Bingham wrote:
> Hi Jacopo,
>
> On 16/03/2020 20:27, Jacopo Mondi wrote:
> > The MAX9286 chip exposes a way to control the 'overlap window'
> > parameter, most probably used in calculation of the frame
> > synchronization interval.
> >
> > When used in conjunction with some serializers, the overlap window has to
> > be disabled in order to correctly achieve frame sync locking.
> >
> > As the exact meaning of that control is not documented in the chip's
> > manual, require all DTS users to specify the value of the window. When,
> > and if, in future the meaning of control gets clarified and a default
>
> /in future/in the future/
> /of control/of the control/
>
> > behaviour (window enabled or disabled) can be established, a new boolean
> > property could supersede this one while being sure that older DTB are
>
> /DTB/DTBs/
>
> > fully specified to avoid confusion.
> >
> > Provide a few convenience macros for the window disabled and window
> > default value.
>
> Well it's not the best solution (putting hardcode values into the DTB)
> but I agree, without documentation these are almost 'magic values for
> the hardware' which is unfortunate.

>
> I do fear this may be the wrong place still though.
>
> This is dependent upon the 'serializer' connected, so is it a property
> of the serializer that the max9286 should query at probe time to see
> what it has connected?

I see two distinct issues here:
1) overlap window: this is really a property of the deserializer and
should be here specified
2) reverse channel amplitude: this depends on the serializer
configuration. we could make this a serializer property and have the
deserializer read it from the remote end,  which, if I'm not
mistaken is a no-go (not sure I recall why, but I refrained to do so
as I recall it was considered bad practice).

This should work by ideally rewriting the deser properties when
loading overlays, or simply configuring the deserializer correctly
depending on which serializer is connected in your system. I don't see
it working differently than any other endpoint property. If you have a
parallel video port configured with an active high vsync polarity and
you want to connect a camera with a low active signal state you should
change you video input port ni your .dts or rewrite it with an overlay
in case of dynamic loading.

>
> But that's just back to the whole topic of bus-parameter negotiations
> between the serializer and deserializer ...
>

The real solution here would be to have operations to call on the
remote to get the bus configuration and act accordingly...

I recently proposed
https://patchwork.kernel.org/project/linux-renesas-soc/list/?series=256127
which works on remote pads though, and we have one pad per gmsl
channel, while this should be queried from the device. Yes we can bend
this to interrogate the remote side on the first connected channel and
ignore the pad argument in the callee, but maybe we can do better than
proposing an API and working it around immediately ? :)

>
> But with this solution, any dtb segment or overlay for the serializer
> needs to modify the max9286 depending on it's requirements ... that
> feels a bit odd. Possible, I think, but odd.

Really not someone with any dt overlay experience here, but re-writing
properties of nodes defined in the DTS seems like an intentended
feature of overlays... Is anyone with more experience listenting (says
Jacopo looking at Geert ?)

Thanks
   j
>
> Do we have any precedence in existing DT to reference on this topic?
>
>
>
>
> > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > ---
> >  .../devicetree/bindings/media/i2c/maxim,max9286.yaml  | 11 +++++++++++
> >  MAINTAINERS                                           |  1 +
> >  include/dt-bindings/media/maxim-gmsl.h                |  9 +++++++++
> >  3 files changed, 21 insertions(+)
> >  create mode 100644 include/dt-bindings/media/maxim-gmsl.h
> >
> > diff --git a/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml b/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
> > index f9d3e5712c59..ee8e0418b3f0 100644
> > --- a/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
> > +++ b/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
> > @@ -46,6 +46,14 @@ properties:
> >      description: GPIO connected to the \#PWDN pin with inverted polarity
> >      maxItems: 1
> >
> > +  # Until the overlap window control gets not clarified, require dts
> > +  # to set its value explicitly,
> > +  maxim,overlap-window:
> > +    description: Overlap window duration, in pixel clock cycles.
> > +    maxItems: 1
> > +    allOf:
> > +      - $ref: /schemas/types.yaml#/definitions/uint32
> > +
> >    ports:
> >      type: object
> >      description: |
> > @@ -146,6 +154,7 @@ properties:
> >  required:
> >    - compatible
> >    - reg
> > +  - maxim,overlap-window
> >    - ports
> >    - i2c-mux
> >
> > @@ -154,6 +163,7 @@ additionalProperties: false
> >  examples:
> >    - |
> >      #include <dt-bindings/gpio/gpio.h>
> > +    #include <dt-bindings/media/maxim-gmsl.h>
> >
> >      i2c@e66d8000 {
> >        #address-cells = <1>;
> > @@ -166,6 +176,7 @@ examples:
> >          reg = <0x2c>;
> >          poc-supply = <&camera_poc_12v>;
> >          enable-gpios = <&gpio 13 GPIO_ACTIVE_HIGH>;
> > +        maxim,overlap-window = MAX9286_OVLP_WINDOW_DISABLED;
> >
> >          ports {
> >            #address-cells = <1>;
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 21a9ff4fe684..3d2455085c80 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -10190,6 +10190,7 @@ M:	Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> >  L:	linux-media@vger.kernel.org
> >  S:	Maintained
> >  F:	Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
> > +F:	include/dt-bindings/media/maxim-gmsl.h
> >  F:	drivers/media/i2c/max9286.c
> >
> >  MAX9860 MONO AUDIO VOICE CODEC DRIVER
> > diff --git a/include/dt-bindings/media/maxim-gmsl.h b/include/dt-bindings/media/maxim-gmsl.h
> > new file mode 100644
> > index 000000000000..47945ffc3a4d
> > --- /dev/null
> > +++ b/include/dt-bindings/media/maxim-gmsl.h
> > @@ -0,0 +1,9 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +#ifndef _DT_BINDINGS_MEDIA_MAXIM_GMSL_H
> > +#define _DT_BINDINGS_MEDIA_MAXIM_GMSL_H
> > +
> > +/* MAX9286 default overlap values. */
> > +#define MAX9286_OVLP_WINDOW_DISABLED	<0>
> > +#define MAX9286_OVLP_WINDOW_DEFAULT	<0x1680>
> > +
> > +#endif /* _DT_BINDINGS_MEDIA_MAXIM_GMSL_H */
> >
>
Hyun Kwon March 19, 2020, 1:08 a.m. UTC | #3 
Hi Jacopo,

On Wed, 2020-03-18 at 07:19:39 -0700, Jacopo Mondi wrote:
> Hi Kieran,
> 
> On Wed, Mar 18, 2020 at 09:45:14AM +0000, Kieran Bingham wrote:
> > Hi Jacopo,
> >
> > On 16/03/2020 20:27, Jacopo Mondi wrote:
> > > The MAX9286 chip exposes a way to control the 'overlap window'
> > > parameter, most probably used in calculation of the frame
> > > synchronization interval.
> > >
> > > When used in conjunction with some serializers, the overlap window has to
> > > be disabled in order to correctly achieve frame sync locking.
> > >
> > > As the exact meaning of that control is not documented in the chip's
> > > manual, require all DTS users to specify the value of the window. When,
> > > and if, in future the meaning of control gets clarified and a default
> >
> > /in future/in the future/
> > /of control/of the control/
> >
> > > behaviour (window enabled or disabled) can be established, a new boolean
> > > property could supersede this one while being sure that older DTB are
> >
> > /DTB/DTBs/
> >
> > > fully specified to avoid confusion.
> > >
> > > Provide a few convenience macros for the window disabled and window
> > > default value.
> >
> > Well it's not the best solution (putting hardcode values into the DTB)
> > but I agree, without documentation these are almost 'magic values for
> > the hardware' which is unfortunate.
> 
> >
> > I do fear this may be the wrong place still though.
> >
> > This is dependent upon the 'serializer' connected, so is it a property
> > of the serializer that the max9286 should query at probe time to see
> > what it has connected?
> 
> I see two distinct issues here:
> 1) overlap window: this is really a property of the deserializer and
> should be here specified
> 2) reverse channel amplitude: this depends on the serializer
> configuration. we could make this a serializer property and have the
> deserializer read it from the remote end,  which, if I'm not
> mistaken is a no-go (not sure I recall why, but I refrained to do so
> as I recall it was considered bad practice).
> 
> This should work by ideally rewriting the deser properties when
> loading overlays, or simply configuring the deserializer correctly
> depending on which serializer is connected in your system. I don't see
> it working differently than any other endpoint property. If you have a
> parallel video port configured with an active high vsync polarity and
> you want to connect a camera with a low active signal state you should
> change you video input port ni your .dts or rewrite it with an overlay
> in case of dynamic loading.
> 
> >
> > But that's just back to the whole topic of bus-parameter negotiations
> > between the serializer and deserializer ...
> >
> 
> The real solution here would be to have operations to call on the
> remote to get the bus configuration and act accordingly...
> 
> I recently proposed
> https://patchwork.kernel.org/project/linux-renesas-soc/list/?series=256127
> which works on remote pads though, and we have one pad per gmsl
> channel, while this should be queried from the device. Yes we can bend
> this to interrogate the remote side on the first connected channel and
> ignore the pad argument in the callee, but maybe we can do better than
> proposing an API and working it around immediately ? :)
> 

What should be worked around here? To me, this seems better solution than
device tree because those are not really hardened properties, and some even
have to be passed between ser - deser. So adding a gmsl config to the pad
config would do, correct? I can (and would like to) try that out if you don't
ee any obvious issue. :-)

Thanks,
-hyun

> >
> > But with this solution, any dtb segment or overlay for the serializer
> > needs to modify the max9286 depending on it's requirements ... that
> > feels a bit odd. Possible, I think, but odd.
> 
> Really not someone with any dt overlay experience here, but re-writing
> properties of nodes defined in the DTS seems like an intentended
> feature of overlays... Is anyone with more experience listenting (says
> Jacopo looking at Geert ?)
> 
> Thanks
>    j
> >
> > Do we have any precedence in existing DT to reference on this topic?
> >
> >
> >
> >
> > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > > ---
> > >  .../devicetree/bindings/media/i2c/maxim,max9286.yaml  | 11 +++++++++++
> > >  MAINTAINERS                                           |  1 +
> > >  include/dt-bindings/media/maxim-gmsl.h                |  9 +++++++++
> > >  3 files changed, 21 insertions(+)
> > >  create mode 100644 include/dt-bindings/media/maxim-gmsl.h
> > >
> > > diff --git a/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml b/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
> > > index f9d3e5712c59..ee8e0418b3f0 100644
> > > --- a/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
> > > +++ b/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
> > > @@ -46,6 +46,14 @@ properties:
> > >      description: GPIO connected to the \#PWDN pin with inverted polarity
> > >      maxItems: 1
> > >
> > > +  # Until the overlap window control gets not clarified, require dts
> > > +  # to set its value explicitly,
> > > +  maxim,overlap-window:
> > > +    description: Overlap window duration, in pixel clock cycles.
> > > +    maxItems: 1
> > > +    allOf:
> > > +      - $ref: /schemas/types.yaml#/definitions/uint32
> > > +
> > >    ports:
> > >      type: object
> > >      description: |
> > > @@ -146,6 +154,7 @@ properties:
> > >  required:
> > >    - compatible
> > >    - reg
> > > +  - maxim,overlap-window
> > >    - ports
> > >    - i2c-mux
> > >
> > > @@ -154,6 +163,7 @@ additionalProperties: false
> > >  examples:
> > >    - |
> > >      #include <dt-bindings/gpio/gpio.h>
> > > +    #include <dt-bindings/media/maxim-gmsl.h>
> > >
> > >      i2c@e66d8000 {
> > >        #address-cells = <1>;
> > > @@ -166,6 +176,7 @@ examples:
> > >          reg = <0x2c>;
> > >          poc-supply = <&camera_poc_12v>;
> > >          enable-gpios = <&gpio 13 GPIO_ACTIVE_HIGH>;
> > > +        maxim,overlap-window = MAX9286_OVLP_WINDOW_DISABLED;
> > >
> > >          ports {
> > >            #address-cells = <1>;
> > > diff --git a/MAINTAINERS b/MAINTAINERS
> > > index 21a9ff4fe684..3d2455085c80 100644
> > > --- a/MAINTAINERS
> > > +++ b/MAINTAINERS
> > > @@ -10190,6 +10190,7 @@ M:	Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> > >  L:	linux-media@vger.kernel.org
> > >  S:	Maintained
> > >  F:	Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
> > > +F:	include/dt-bindings/media/maxim-gmsl.h
> > >  F:	drivers/media/i2c/max9286.c
> > >
> > >  MAX9860 MONO AUDIO VOICE CODEC DRIVER
> > > diff --git a/include/dt-bindings/media/maxim-gmsl.h b/include/dt-bindings/media/maxim-gmsl.h
> > > new file mode 100644
> > > index 000000000000..47945ffc3a4d
> > > --- /dev/null
> > > +++ b/include/dt-bindings/media/maxim-gmsl.h
> > > @@ -0,0 +1,9 @@
> > > +/* SPDX-License-Identifier: GPL-2.0 */
> > > +#ifndef _DT_BINDINGS_MEDIA_MAXIM_GMSL_H
> > > +#define _DT_BINDINGS_MEDIA_MAXIM_GMSL_H
> > > +
> > > +/* MAX9286 default overlap values. */
> > > +#define MAX9286_OVLP_WINDOW_DISABLED	<0>
> > > +#define MAX9286_OVLP_WINDOW_DEFAULT	<0x1680>
> > > +
> > > +#endif /* _DT_BINDINGS_MEDIA_MAXIM_GMSL_H */
> > >
> >
Jacopo Mondi March 19, 2020, 9 a.m. UTC | #4 
Hi Hyun,

On Wed, Mar 18, 2020 at 06:08:13PM -0700, Hyun Kwon wrote:
> Hi Jacopo,
>
> On Wed, 2020-03-18 at 07:19:39 -0700, Jacopo Mondi wrote:
> > Hi Kieran,
> >
> > On Wed, Mar 18, 2020 at 09:45:14AM +0000, Kieran Bingham wrote:
> > > Hi Jacopo,
> > >
> > > On 16/03/2020 20:27, Jacopo Mondi wrote:
> > > > The MAX9286 chip exposes a way to control the 'overlap window'
> > > > parameter, most probably used in calculation of the frame
> > > > synchronization interval.
> > > >
> > > > When used in conjunction with some serializers, the overlap window has to
> > > > be disabled in order to correctly achieve frame sync locking.
> > > >
> > > > As the exact meaning of that control is not documented in the chip's
> > > > manual, require all DTS users to specify the value of the window. When,
> > > > and if, in future the meaning of control gets clarified and a default
> > >
> > > /in future/in the future/
> > > /of control/of the control/
> > >
> > > > behaviour (window enabled or disabled) can be established, a new boolean
> > > > property could supersede this one while being sure that older DTB are
> > >
> > > /DTB/DTBs/
> > >
> > > > fully specified to avoid confusion.
> > > >
> > > > Provide a few convenience macros for the window disabled and window
> > > > default value.
> > >
> > > Well it's not the best solution (putting hardcode values into the DTB)
> > > but I agree, without documentation these are almost 'magic values for
> > > the hardware' which is unfortunate.
> >
> > >
> > > I do fear this may be the wrong place still though.
> > >
> > > This is dependent upon the 'serializer' connected, so is it a property
> > > of the serializer that the max9286 should query at probe time to see
> > > what it has connected?
> >
> > I see two distinct issues here:
> > 1) overlap window: this is really a property of the deserializer and
> > should be here specified
> > 2) reverse channel amplitude: this depends on the serializer
> > configuration. we could make this a serializer property and have the
> > deserializer read it from the remote end,  which, if I'm not
> > mistaken is a no-go (not sure I recall why, but I refrained to do so
> > as I recall it was considered bad practice).
> >
> > This should work by ideally rewriting the deser properties when
> > loading overlays, or simply configuring the deserializer correctly
> > depending on which serializer is connected in your system. I don't see
> > it working differently than any other endpoint property. If you have a
> > parallel video port configured with an active high vsync polarity and
> > you want to connect a camera with a low active signal state you should
> > change you video input port ni your .dts or rewrite it with an overlay
> > in case of dynamic loading.
> >
> > >
> > > But that's just back to the whole topic of bus-parameter negotiations
> > > between the serializer and deserializer ...
> > >
> >
> > The real solution here would be to have operations to call on the
> > remote to get the bus configuration and act accordingly...
> >
> > I recently proposed
> > https://patchwork.kernel.org/project/linux-renesas-soc/list/?series=256127
> > which works on remote pads though, and we have one pad per gmsl
> > channel, while this should be queried from the device. Yes we can bend
> > this to interrogate the remote side on the first connected channel and
> > ignore the pad argument in the callee, but maybe we can do better than
> > proposing an API and working it around immediately ? :)
> >
>
> What should be worked around here? To me, this seems better solution than
> device tree because those are not really hardened properties, and some even
> have to be passed between ser - deser. So adding a gmsl config to the pad
> config would do, correct? I can (and would like to) try that out if you don't
> ee any obvious issue. :-)

As you know I'm not yet sure which properties should come from DTS and
which ones negotiated, but for sure some of them fit better in a
ser/deser negotiation and the reverse channel amplitude setting is
indeed one of them.

Why my get_mbus_config() should be "worked around": the proposed
operation is a pad operation. The deserializer has 4 input pads, one
for each gmsl input channel. Currently we configure the reverse
channel amplitude once, then probe all the four channels, and finally
we compensate it. I said "work it around" as in this scenario we have
to query the first enabled remote channel, and apply the reported
settings for all the channels.

BUT I'm now thinking we should actually change how we setup reverse
channel in the deserializer and make it a per-channel procedure, which
is what the programming guide suggests as well.

Using get_mbus_config() with the current implementation would look like

        config = get_mbus_config(first_enabled_channel)
        configure_reverse_channel(config)

        for_each_channel(c):
                setup_serializer(c)

        if (config.amplitude < 170)
                increase_amplitude(config)

And I'm really not thrilled by this.
What we should do is actually

        for_each_channel(c):
                config = get_mbus_config(c)
                configure_reverse_channel(config);

                setup_serializer(c)

                if (config.amplitude < 170)
                        increase_amplitude(config)

And I would like this much better, it also gives get_mbus_config() one
more user which would help having that operation accepted in mainline.

I will give this a go! What do you think ?

Thanks
   j

>
> Thanks,
> -hyun
>
> > >
> > > But with this solution, any dtb segment or overlay for the serializer
> > > needs to modify the max9286 depending on it's requirements ... that
> > > feels a bit odd. Possible, I think, but odd.
> >
> > Really not someone with any dt overlay experience here, but re-writing
> > properties of nodes defined in the DTS seems like an intentended
> > feature of overlays... Is anyone with more experience listenting (says
> > Jacopo looking at Geert ?)
> >
> > Thanks
> >    j
> > >
> > > Do we have any precedence in existing DT to reference on this topic?
> > >
> > >
> > >
> > >
> > > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > > > ---
> > > >  .../devicetree/bindings/media/i2c/maxim,max9286.yaml  | 11 +++++++++++
> > > >  MAINTAINERS                                           |  1 +
> > > >  include/dt-bindings/media/maxim-gmsl.h                |  9 +++++++++
> > > >  3 files changed, 21 insertions(+)
> > > >  create mode 100644 include/dt-bindings/media/maxim-gmsl.h
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml b/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
> > > > index f9d3e5712c59..ee8e0418b3f0 100644
> > > > --- a/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
> > > > +++ b/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
> > > > @@ -46,6 +46,14 @@ properties:
> > > >      description: GPIO connected to the \#PWDN pin with inverted polarity
> > > >      maxItems: 1
> > > >
> > > > +  # Until the overlap window control gets not clarified, require dts
> > > > +  # to set its value explicitly,
> > > > +  maxim,overlap-window:
> > > > +    description: Overlap window duration, in pixel clock cycles.
> > > > +    maxItems: 1
> > > > +    allOf:
> > > > +      - $ref: /schemas/types.yaml#/definitions/uint32
> > > > +
> > > >    ports:
> > > >      type: object
> > > >      description: |
> > > > @@ -146,6 +154,7 @@ properties:
> > > >  required:
> > > >    - compatible
> > > >    - reg
> > > > +  - maxim,overlap-window
> > > >    - ports
> > > >    - i2c-mux
> > > >
> > > > @@ -154,6 +163,7 @@ additionalProperties: false
> > > >  examples:
> > > >    - |
> > > >      #include <dt-bindings/gpio/gpio.h>
> > > > +    #include <dt-bindings/media/maxim-gmsl.h>
> > > >
> > > >      i2c@e66d8000 {
> > > >        #address-cells = <1>;
> > > > @@ -166,6 +176,7 @@ examples:
> > > >          reg = <0x2c>;
> > > >          poc-supply = <&camera_poc_12v>;
> > > >          enable-gpios = <&gpio 13 GPIO_ACTIVE_HIGH>;
> > > > +        maxim,overlap-window = MAX9286_OVLP_WINDOW_DISABLED;
> > > >
> > > >          ports {
> > > >            #address-cells = <1>;
> > > > diff --git a/MAINTAINERS b/MAINTAINERS
> > > > index 21a9ff4fe684..3d2455085c80 100644
> > > > --- a/MAINTAINERS
> > > > +++ b/MAINTAINERS
> > > > @@ -10190,6 +10190,7 @@ M:	Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> > > >  L:	linux-media@vger.kernel.org
> > > >  S:	Maintained
> > > >  F:	Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
> > > > +F:	include/dt-bindings/media/maxim-gmsl.h
> > > >  F:	drivers/media/i2c/max9286.c
> > > >
> > > >  MAX9860 MONO AUDIO VOICE CODEC DRIVER
> > > > diff --git a/include/dt-bindings/media/maxim-gmsl.h b/include/dt-bindings/media/maxim-gmsl.h
> > > > new file mode 100644
> > > > index 000000000000..47945ffc3a4d
> > > > --- /dev/null
> > > > +++ b/include/dt-bindings/media/maxim-gmsl.h
> > > > @@ -0,0 +1,9 @@
> > > > +/* SPDX-License-Identifier: GPL-2.0 */
> > > > +#ifndef _DT_BINDINGS_MEDIA_MAXIM_GMSL_H
> > > > +#define _DT_BINDINGS_MEDIA_MAXIM_GMSL_H
> > > > +
> > > > +/* MAX9286 default overlap values. */
> > > > +#define MAX9286_OVLP_WINDOW_DISABLED	<0>
> > > > +#define MAX9286_OVLP_WINDOW_DEFAULT	<0x1680>
> > > > +
> > > > +#endif /* _DT_BINDINGS_MEDIA_MAXIM_GMSL_H */
> > > >
> > >
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml b/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
index f9d3e5712c59..ee8e0418b3f0 100644
--- a/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
+++ b/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
@@ -46,6 +46,14 @@  properties:
     description: GPIO connected to the \#PWDN pin with inverted polarity
     maxItems: 1
 
+  # Until the overlap window control gets not clarified, require dts
+  # to set its value explicitly,
+  maxim,overlap-window:
+    description: Overlap window duration, in pixel clock cycles.
+    maxItems: 1
+    allOf:
+      - $ref: /schemas/types.yaml#/definitions/uint32
+
   ports:
     type: object
     description: |
@@ -146,6 +154,7 @@  properties:
 required:
   - compatible
   - reg
+  - maxim,overlap-window
   - ports
   - i2c-mux
 
@@ -154,6 +163,7 @@  additionalProperties: false
 examples:
   - |
     #include <dt-bindings/gpio/gpio.h>
+    #include <dt-bindings/media/maxim-gmsl.h>
 
     i2c@e66d8000 {
       #address-cells = <1>;
@@ -166,6 +176,7 @@  examples:
         reg = <0x2c>;
         poc-supply = <&camera_poc_12v>;
         enable-gpios = <&gpio 13 GPIO_ACTIVE_HIGH>;
+        maxim,overlap-window = MAX9286_OVLP_WINDOW_DISABLED;
 
         ports {
           #address-cells = <1>;
diff --git a/MAINTAINERS b/MAINTAINERS
index 21a9ff4fe684..3d2455085c80 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -10190,6 +10190,7 @@  M:	Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
 L:	linux-media@vger.kernel.org
 S:	Maintained
 F:	Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
+F:	include/dt-bindings/media/maxim-gmsl.h
 F:	drivers/media/i2c/max9286.c
 
 MAX9860 MONO AUDIO VOICE CODEC DRIVER
diff --git a/include/dt-bindings/media/maxim-gmsl.h b/include/dt-bindings/media/maxim-gmsl.h
new file mode 100644
index 000000000000..47945ffc3a4d
--- /dev/null
+++ b/include/dt-bindings/media/maxim-gmsl.h
@@ -0,0 +1,9 @@ 
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _DT_BINDINGS_MEDIA_MAXIM_GMSL_H
+#define _DT_BINDINGS_MEDIA_MAXIM_GMSL_H
+
+/* MAX9286 default overlap values. */
+#define MAX9286_OVLP_WINDOW_DISABLED	<0>
+#define MAX9286_OVLP_WINDOW_DEFAULT	<0x1680>
+
+#endif /* _DT_BINDINGS_MEDIA_MAXIM_GMSL_H */