[5/5] media: i2c: max9286: Parse channel amplitude
diff mbox series

Message ID 20200316202757.529740-6-jacopo+renesas@jmondi.org
State New
Delegated to: Kieran Bingham
Headers show
Series
  • media: i2c: max9286: Add configuration properties
Related show

Commit Message

Jacopo Mondi March 16, 2020, 8:27 p.m. UTC
Parse the 'maxim,reverse-channel-amplitude' property value and cache its
content to later program the initial reverse channel amplitude.

Only support 100mV and 170mV values for the moment. The property could
be easily expanded to support more values.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 drivers/media/i2c/max9286.c | 39 ++++++++++++++++++++++++++++++++-----
 1 file changed, 34 insertions(+), 5 deletions(-)

Comments

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

On 16/03/2020 20:27, Jacopo Mondi wrote:
> Parse the 'maxim,reverse-channel-amplitude' property value and cache its
> content to later program the initial reverse channel amplitude.
> 
> Only support 100mV and 170mV values for the moment. The property could
> be easily expanded to support more values.

Can we (in the future) support arbitrary values from a range, or only
from a fixed list?

> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  drivers/media/i2c/max9286.c | 39 ++++++++++++++++++++++++++++++++-----
>  1 file changed, 34 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
> index 0357515860b2..24af8002535e 100644
> --- a/drivers/media/i2c/max9286.c
> +++ b/drivers/media/i2c/max9286.c
> @@ -168,6 +168,7 @@ struct max9286_priv {
>  	struct max9286_source sources[MAX9286_NUM_GMSL];
>  	struct v4l2_async_notifier notifier;
>  
> +	u32 reverse_chan_amp;
>  	u32 overlap_window;
>  };
>  
> @@ -479,10 +480,15 @@ static int max9286_notify_bound(struct v4l2_async_notifier *notifier,
>  	 * All enabled sources have probed and enabled their reverse control
>  	 * channels:
>  	 *
> -	 * - Verify all configuration links are properly detected
> +	 * - Increase reverse channel amplitude to 170mV if not initially
> +	 *   compensated
>  	 * - Disable auto-ack as communication on the control channel are now
>  	 *   stable.
>  	 */
> +	if (priv->reverse_chan_amp == 100)
> +		max9286_write(priv, 0x3b, MAX9286_REV_TRF(1) |
> +			      MAX9286_REV_AMP(70) | MAX9286_REV_AMP_X);
> +
>  	max9286_check_config_link(priv, priv->source_mask);
>  
>  	/*
> @@ -830,6 +836,8 @@ static void max9286_v4l2_unregister(struct max9286_priv *priv)
>  
>  static int max9286_setup(struct max9286_priv *priv)
>  {
> +	u8 chan_amp = MAX9286_REV_TRF(1);
> +
>  	/*
>  	 * Link ordering values for all enabled links combinations. Orders must
>  	 * be assigned sequentially from 0 to the number of enabled links
> @@ -869,12 +877,18 @@ static int max9286_setup(struct max9286_priv *priv)
>  	 *
>  	 * - Enable custom reverse channel configuration (through register 0x3f)
>  	 *   and set the first pulse length to 35 clock cycles.
> -	 * - Increase the reverse channel amplitude to 170mV to accommodate the
> -	 *   high threshold enabled by the serializer driver.
> +	 * - Set initial reverse channel amplitude according the DTS property.
> +	 *   If the initial channel amplitude is 100mV it should be increase
> +	 *   later after the serializers high threshold have been enabled.
> +	 *   If the initial value is 170mV the serializer has been
> +	 *   pre-programmed and we can compensate immediately.>  	 */
>  	max9286_write(priv, 0x3f, MAX9286_EN_REV_CFG | MAX9286_REV_FLEN(35));
> -	max9286_write(priv, 0x3b, MAX9286_REV_TRF(1) | MAX9286_REV_AMP(70) |
> -		      MAX9286_REV_AMP_X);
> +	if (priv->reverse_chan_amp == 100)
> +		chan_amp |= MAX9286_REV_AMP(100);
> +	else
> +		chan_amp |= MAX9286_REV_AMP(70) | MAX9286_REV_AMP_X;
> +	max9286_write(priv, 0x3b, chan_amp);
>  	usleep_range(2000, 2500);
>  
>  	/*
> @@ -1069,6 +1083,21 @@ static int max9286_parse_dt(struct max9286_priv *priv)
>  		return -EINVAL;
>  	}
>  
> +	ret = of_property_read_u32(dev->of_node, "maxim,reverse-channel-amplitude",
> +				   &priv->reverse_chan_amp);
> +	if (ret) {
> +		dev_err(dev,
> +			"Missing property \"maxim,reverse-channel-amplitude\"\n");
> +		of_node_put(dev->of_node);
> +		return -EINVAL;
> +	}
> +	if (priv->reverse_chan_amp != 100 && priv->reverse_chan_amp != 170) {
> +		dev_err(dev, "Unsupported  channel amplitude %umV\n",
> +			priv->reverse_chan_amp);
> +		of_node_put(dev->of_node);
> +		return -EINVAL;
> +	}
> +
>  	i2c_mux = of_find_node_by_name(dev->of_node, "i2c-mux");
>  	if (!i2c_mux) {
>  		dev_err(dev, "Failed to find i2c-mux node\n");
>
Jacopo Mondi March 18, 2020, 2:32 p.m. UTC | #2
Hi Kieran,

On Wed, Mar 18, 2020 at 09:57:48AM +0000, Kieran Bingham wrote:
> Hi Jacopo,
>
> On 16/03/2020 20:27, Jacopo Mondi wrote:
> > Parse the 'maxim,reverse-channel-amplitude' property value and cache its
> > content to later program the initial reverse channel amplitude.
> >
> > Only support 100mV and 170mV values for the moment. The property could
> > be easily expanded to support more values.
>
> Can we (in the future) support arbitrary values from a range, or only
> from a fixed list?

Good question. The 0x3b register of the deserializer is not documented
in my datasheet version, I got this from the application developer
guide that reports

        Increase reverse amplitude from 100mV to
        170mV. This compensates for the higher
        threshold of step 5.

and reports the following list of supported values in the 0x3b
register description.

        Reverse channel amplitude
        000 = 30mV
        001 = 40mV
        010 = 50mV
        011 = 60mV
        100 = 70mV
        101 = 80mV
        110 = 90mV
        111 = 100mV

with an optional +100mV boost option.

Going forward we can add more values to the list of supported ones in
the bindings and control their configuration in the driver.

Maybe worth noting it down with a fixme note ?

>
> > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > ---
> >  drivers/media/i2c/max9286.c | 39 ++++++++++++++++++++++++++++++++-----
> >  1 file changed, 34 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
> > index 0357515860b2..24af8002535e 100644
> > --- a/drivers/media/i2c/max9286.c
> > +++ b/drivers/media/i2c/max9286.c
> > @@ -168,6 +168,7 @@ struct max9286_priv {
> >  	struct max9286_source sources[MAX9286_NUM_GMSL];
> >  	struct v4l2_async_notifier notifier;
> >
> > +	u32 reverse_chan_amp;
> >  	u32 overlap_window;
> >  };
> >
> > @@ -479,10 +480,15 @@ static int max9286_notify_bound(struct v4l2_async_notifier *notifier,
> >  	 * All enabled sources have probed and enabled their reverse control
> >  	 * channels:
> >  	 *
> > -	 * - Verify all configuration links are properly detected
> > +	 * - Increase reverse channel amplitude to 170mV if not initially
> > +	 *   compensated
> >  	 * - Disable auto-ack as communication on the control channel are now
> >  	 *   stable.
> >  	 */
> > +	if (priv->reverse_chan_amp == 100)
> > +		max9286_write(priv, 0x3b, MAX9286_REV_TRF(1) |
> > +			      MAX9286_REV_AMP(70) | MAX9286_REV_AMP_X);
> > +
> >  	max9286_check_config_link(priv, priv->source_mask);
> >
> >  	/*
> > @@ -830,6 +836,8 @@ static void max9286_v4l2_unregister(struct max9286_priv *priv)
> >
> >  static int max9286_setup(struct max9286_priv *priv)
> >  {
> > +	u8 chan_amp = MAX9286_REV_TRF(1);
> > +
> >  	/*
> >  	 * Link ordering values for all enabled links combinations. Orders must
> >  	 * be assigned sequentially from 0 to the number of enabled links
> > @@ -869,12 +877,18 @@ static int max9286_setup(struct max9286_priv *priv)
> >  	 *
> >  	 * - Enable custom reverse channel configuration (through register 0x3f)
> >  	 *   and set the first pulse length to 35 clock cycles.
> > -	 * - Increase the reverse channel amplitude to 170mV to accommodate the
> > -	 *   high threshold enabled by the serializer driver.
> > +	 * - Set initial reverse channel amplitude according the DTS property.
> > +	 *   If the initial channel amplitude is 100mV it should be increase
> > +	 *   later after the serializers high threshold have been enabled.
> > +	 *   If the initial value is 170mV the serializer has been
> > +	 *   pre-programmed and we can compensate immediately.>  	 */
> >  	max9286_write(priv, 0x3f, MAX9286_EN_REV_CFG | MAX9286_REV_FLEN(35));
> > -	max9286_write(priv, 0x3b, MAX9286_REV_TRF(1) | MAX9286_REV_AMP(70) |
> > -		      MAX9286_REV_AMP_X);
> > +	if (priv->reverse_chan_amp == 100)
> > +		chan_amp |= MAX9286_REV_AMP(100);
> > +	else
> > +		chan_amp |= MAX9286_REV_AMP(70) | MAX9286_REV_AMP_X;
> > +	max9286_write(priv, 0x3b, chan_amp);
> >  	usleep_range(2000, 2500);
> >
> >  	/*
> > @@ -1069,6 +1083,21 @@ static int max9286_parse_dt(struct max9286_priv *priv)
> >  		return -EINVAL;
> >  	}
> >
> > +	ret = of_property_read_u32(dev->of_node, "maxim,reverse-channel-amplitude",
> > +				   &priv->reverse_chan_amp);
> > +	if (ret) {
> > +		dev_err(dev,
> > +			"Missing property \"maxim,reverse-channel-amplitude\"\n");
> > +		of_node_put(dev->of_node);
> > +		return -EINVAL;
> > +	}
> > +	if (priv->reverse_chan_amp != 100 && priv->reverse_chan_amp != 170) {
> > +		dev_err(dev, "Unsupported  channel amplitude %umV\n",
> > +			priv->reverse_chan_amp);
> > +		of_node_put(dev->of_node);
> > +		return -EINVAL;
> > +	}
> > +
> >  	i2c_mux = of_find_node_by_name(dev->of_node, "i2c-mux");
> >  	if (!i2c_mux) {
> >  		dev_err(dev, "Failed to find i2c-mux node\n");
> >
>
Kieran Bingham March 19, 2020, 11:13 a.m. UTC | #3
On 18/03/2020 14:32, Jacopo Mondi wrote:
> Hi Kieran,
> 
> On Wed, Mar 18, 2020 at 09:57:48AM +0000, Kieran Bingham wrote:
>> Hi Jacopo,
>>
>> On 16/03/2020 20:27, Jacopo Mondi wrote:
>>> Parse the 'maxim,reverse-channel-amplitude' property value and cache its
>>> content to later program the initial reverse channel amplitude.
>>>
>>> Only support 100mV and 170mV values for the moment. The property could
>>> be easily expanded to support more values.
>>
>> Can we (in the future) support arbitrary values from a range, or only
>> from a fixed list?
> 
> Good question. The 0x3b register of the deserializer is not documented
> in my datasheet version, I got this from the application developer
> guide that reports
> 
>         Increase reverse amplitude from 100mV to
>         170mV. This compensates for the higher
>         threshold of step 5.
> 
> and reports the following list of supported values in the 0x3b
> register description.
> 
>         Reverse channel amplitude
>         000 = 30mV
>         001 = 40mV
>         010 = 50mV
>         011 = 60mV
>         100 = 70mV
>         101 = 80mV
>         110 = 90mV
>         111 = 100mV
> 
> with an optional +100mV boost option.

Ok, so we have two supported 'ranges'
 30-100mV and 130-200mV.

Indeed it's probably best not to express that as a single range :-)


> Going forward we can add more values to the list of supported ones in
> the bindings and control their configuration in the driver.
> 
> Maybe worth noting it down with a fixme note ?

I wonder if it should be expressed as a supported range, with a boost,
which matches the hardware, and presumably will match the requirements
on the serializer side too ?

Presumably if the boost is needed, we 'know' when it's needed? although
that doesn't quite fit either. I see you're going from 100mV to 70mV so
you're actually onnly applying a 'boost' of 70mV not 100 ?

Hrm ... I'll have to do more digging to understand what's going here.

--
Kieran


> 
>>
>>> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
>>> ---
>>>  drivers/media/i2c/max9286.c | 39 ++++++++++++++++++++++++++++++++-----
>>>  1 file changed, 34 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
>>> index 0357515860b2..24af8002535e 100644
>>> --- a/drivers/media/i2c/max9286.c
>>> +++ b/drivers/media/i2c/max9286.c
>>> @@ -168,6 +168,7 @@ struct max9286_priv {
>>>  	struct max9286_source sources[MAX9286_NUM_GMSL];
>>>  	struct v4l2_async_notifier notifier;
>>>
>>> +	u32 reverse_chan_amp;
>>>  	u32 overlap_window;
>>>  };
>>>
>>> @@ -479,10 +480,15 @@ static int max9286_notify_bound(struct v4l2_async_notifier *notifier,
>>>  	 * All enabled sources have probed and enabled their reverse control
>>>  	 * channels:
>>>  	 *
>>> -	 * - Verify all configuration links are properly detected
>>> +	 * - Increase reverse channel amplitude to 170mV if not initially
>>> +	 *   compensated
>>>  	 * - Disable auto-ack as communication on the control channel are now
>>>  	 *   stable.
>>>  	 */
>>> +	if (priv->reverse_chan_amp == 100)
>>> +		max9286_write(priv, 0x3b, MAX9286_REV_TRF(1) |
>>> +			      MAX9286_REV_AMP(70) | MAX9286_REV_AMP_X);
>>> +
>>>  	max9286_check_config_link(priv, priv->source_mask);
>>>
>>>  	/*
>>> @@ -830,6 +836,8 @@ static void max9286_v4l2_unregister(struct max9286_priv *priv)
>>>
>>>  static int max9286_setup(struct max9286_priv *priv)
>>>  {
>>> +	u8 chan_amp = MAX9286_REV_TRF(1);
>>> +
>>>  	/*
>>>  	 * Link ordering values for all enabled links combinations. Orders must
>>>  	 * be assigned sequentially from 0 to the number of enabled links
>>> @@ -869,12 +877,18 @@ static int max9286_setup(struct max9286_priv *priv)
>>>  	 *
>>>  	 * - Enable custom reverse channel configuration (through register 0x3f)
>>>  	 *   and set the first pulse length to 35 clock cycles.
>>> -	 * - Increase the reverse channel amplitude to 170mV to accommodate the
>>> -	 *   high threshold enabled by the serializer driver.
>>> +	 * - Set initial reverse channel amplitude according the DTS property.
>>> +	 *   If the initial channel amplitude is 100mV it should be increase
>>> +	 *   later after the serializers high threshold have been enabled.
>>> +	 *   If the initial value is 170mV the serializer has been
>>> +	 *   pre-programmed and we can compensate immediately.>  	 */
>>>  	max9286_write(priv, 0x3f, MAX9286_EN_REV_CFG | MAX9286_REV_FLEN(35));
>>> -	max9286_write(priv, 0x3b, MAX9286_REV_TRF(1) | MAX9286_REV_AMP(70) |
>>> -		      MAX9286_REV_AMP_X);
>>> +	if (priv->reverse_chan_amp == 100)
>>> +		chan_amp |= MAX9286_REV_AMP(100);
>>> +	else
>>> +		chan_amp |= MAX9286_REV_AMP(70) | MAX9286_REV_AMP_X;
>>> +	max9286_write(priv, 0x3b, chan_amp);
>>>  	usleep_range(2000, 2500);
>>>
>>>  	/*
>>> @@ -1069,6 +1083,21 @@ static int max9286_parse_dt(struct max9286_priv *priv)
>>>  		return -EINVAL;
>>>  	}
>>>
>>> +	ret = of_property_read_u32(dev->of_node, "maxim,reverse-channel-amplitude",
>>> +				   &priv->reverse_chan_amp);
>>> +	if (ret) {
>>> +		dev_err(dev,
>>> +			"Missing property \"maxim,reverse-channel-amplitude\"\n");
>>> +		of_node_put(dev->of_node);
>>> +		return -EINVAL;
>>> +	}
>>> +	if (priv->reverse_chan_amp != 100 && priv->reverse_chan_amp != 170) {
>>> +		dev_err(dev, "Unsupported  channel amplitude %umV\n",
>>> +			priv->reverse_chan_amp);
>>> +		of_node_put(dev->of_node);
>>> +		return -EINVAL;
>>> +	}
>>> +
>>>  	i2c_mux = of_find_node_by_name(dev->of_node, "i2c-mux");
>>>  	if (!i2c_mux) {
>>>  		dev_err(dev, "Failed to find i2c-mux node\n");
>>>
>>

Patch
diff mbox series

diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
index 0357515860b2..24af8002535e 100644
--- a/drivers/media/i2c/max9286.c
+++ b/drivers/media/i2c/max9286.c
@@ -168,6 +168,7 @@  struct max9286_priv {
 	struct max9286_source sources[MAX9286_NUM_GMSL];
 	struct v4l2_async_notifier notifier;
 
+	u32 reverse_chan_amp;
 	u32 overlap_window;
 };
 
@@ -479,10 +480,15 @@  static int max9286_notify_bound(struct v4l2_async_notifier *notifier,
 	 * All enabled sources have probed and enabled their reverse control
 	 * channels:
 	 *
-	 * - Verify all configuration links are properly detected
+	 * - Increase reverse channel amplitude to 170mV if not initially
+	 *   compensated
 	 * - Disable auto-ack as communication on the control channel are now
 	 *   stable.
 	 */
+	if (priv->reverse_chan_amp == 100)
+		max9286_write(priv, 0x3b, MAX9286_REV_TRF(1) |
+			      MAX9286_REV_AMP(70) | MAX9286_REV_AMP_X);
+
 	max9286_check_config_link(priv, priv->source_mask);
 
 	/*
@@ -830,6 +836,8 @@  static void max9286_v4l2_unregister(struct max9286_priv *priv)
 
 static int max9286_setup(struct max9286_priv *priv)
 {
+	u8 chan_amp = MAX9286_REV_TRF(1);
+
 	/*
 	 * Link ordering values for all enabled links combinations. Orders must
 	 * be assigned sequentially from 0 to the number of enabled links
@@ -869,12 +877,18 @@  static int max9286_setup(struct max9286_priv *priv)
 	 *
 	 * - Enable custom reverse channel configuration (through register 0x3f)
 	 *   and set the first pulse length to 35 clock cycles.
-	 * - Increase the reverse channel amplitude to 170mV to accommodate the
-	 *   high threshold enabled by the serializer driver.
+	 * - Set initial reverse channel amplitude according the DTS property.
+	 *   If the initial channel amplitude is 100mV it should be increase
+	 *   later after the serializers high threshold have been enabled.
+	 *   If the initial value is 170mV the serializer has been
+	 *   pre-programmed and we can compensate immediately.
 	 */
 	max9286_write(priv, 0x3f, MAX9286_EN_REV_CFG | MAX9286_REV_FLEN(35));
-	max9286_write(priv, 0x3b, MAX9286_REV_TRF(1) | MAX9286_REV_AMP(70) |
-		      MAX9286_REV_AMP_X);
+	if (priv->reverse_chan_amp == 100)
+		chan_amp |= MAX9286_REV_AMP(100);
+	else
+		chan_amp |= MAX9286_REV_AMP(70) | MAX9286_REV_AMP_X;
+	max9286_write(priv, 0x3b, chan_amp);
 	usleep_range(2000, 2500);
 
 	/*
@@ -1069,6 +1083,21 @@  static int max9286_parse_dt(struct max9286_priv *priv)
 		return -EINVAL;
 	}
 
+	ret = of_property_read_u32(dev->of_node, "maxim,reverse-channel-amplitude",
+				   &priv->reverse_chan_amp);
+	if (ret) {
+		dev_err(dev,
+			"Missing property \"maxim,reverse-channel-amplitude\"\n");
+		of_node_put(dev->of_node);
+		return -EINVAL;
+	}
+	if (priv->reverse_chan_amp != 100 && priv->reverse_chan_amp != 170) {
+		dev_err(dev, "Unsupported  channel amplitude %umV\n",
+			priv->reverse_chan_amp);
+		of_node_put(dev->of_node);
+		return -EINVAL;
+	}
+
 	i2c_mux = of_find_node_by_name(dev->of_node, "i2c-mux");
 	if (!i2c_mux) {
 		dev_err(dev, "Failed to find i2c-mux node\n");