diff mbox series

[v2,1/5] media: i2c: imx335: Drop setting of 0x3a00 register

Message ID 20240131055208.170934-2-umang.jain@ideasonboard.com
State New
Headers
Series imx335: Support additional link-freq and TPG |

Commit Message

Umang Jain Jan. 31, 2024, 5:52 a.m. UTC 
  Register 0x3a00 is a reserved field as per the IMX335 datasheet,
hence shouldn't be set by the driver.

Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
---
 drivers/media/i2c/imx335.c | 1 -
 1 file changed, 1 deletion(-)

Comments

Kieran Bingham Jan. 31, 2024, 9:52 a.m. UTC | #1 
Hi Umang,

+ Cc: Matthias

Quoting Umang Jain (2024-01-31 05:52:04)
> Register 0x3a00 is a reserved field as per the IMX335 datasheet,
> hence shouldn't be set by the driver.

We still need to explain more about why we're dropping this register
write, and what effects it causes.

Matthias - I believe this stemmed from the work you did, and I think I
recall that you stated this register write broke the CSI2 configuration?

Can you clarify anything here for us please?

--
Kieran


> 
> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
> ---
>  drivers/media/i2c/imx335.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/media/i2c/imx335.c b/drivers/media/i2c/imx335.c
> index 7a37eb327ff4..927b4806a5d7 100644
> --- a/drivers/media/i2c/imx335.c
> +++ b/drivers/media/i2c/imx335.c
> @@ -249,7 +249,6 @@ static const struct imx335_reg mode_2592x1940_regs[] = {
>         {0x3794, 0x7a},
>         {0x3796, 0xa1},
>         {0x37b0, 0x36},
> -       {0x3a00, 0x01},
>  };
>  
>  static const struct imx335_reg raw10_framefmt_regs[] = {
> -- 
> 2.41.0
>
Matthias Fend Jan. 31, 2024, 10:32 a.m. UTC | #2 
Hi Kieran,

Am 31.01.2024 um 10:52 schrieb Kieran Bingham:
> Hi Umang,
> 
> + Cc: Matthias
> 
> Quoting Umang Jain (2024-01-31 05:52:04)
>> Register 0x3a00 is a reserved field as per the IMX335 datasheet,
>> hence shouldn't be set by the driver.
> 
> We still need to explain more about why we're dropping this register
> write, and what effects it causes.
> 
> Matthias - I believe this stemmed from the work you did, and I think I
> recall that you stated this register write broke the CSI2 configuration?
> 
> Can you clarify anything here for us please?

yes, that's correct.

Since this driver originally did not work in my setup, I came across 
this register while searching for differences to my working reference 
configuration.
With the default value of this register (0x00), the driver works 
perfectly. With the value previously written to it by the driver (0x01), 
I cannot receive any frames.
The problem may depend on the link frequency used.
I can only use and test a frequency of 445.5MHz on my hardware. Since 
only link frequencies of 594MHz were supported so far, this may not have 
been a problem.

Unfortunately I do not have a description of this register, so I can 
only speculate about the exact cause.

~Matthias

> 
> --
> Kieran
> 
> 
>>
>> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
>> ---
>>   drivers/media/i2c/imx335.c | 1 -
>>   1 file changed, 1 deletion(-)
>>
>> diff --git a/drivers/media/i2c/imx335.c b/drivers/media/i2c/imx335.c
>> index 7a37eb327ff4..927b4806a5d7 100644
>> --- a/drivers/media/i2c/imx335.c
>> +++ b/drivers/media/i2c/imx335.c
>> @@ -249,7 +249,6 @@ static const struct imx335_reg mode_2592x1940_regs[] = {
>>          {0x3794, 0x7a},
>>          {0x3796, 0xa1},
>>          {0x37b0, 0x36},
>> -       {0x3a00, 0x01},
>>   };
>>   
>>   static const struct imx335_reg raw10_framefmt_regs[] = {
>> -- 
>> 2.41.0
>>
Umang Jain Feb. 7, 2024, 4:21 a.m. UTC | #3 
Hi,

On 1/31/24 4:02 PM, Matthias Fend wrote:
> Hi Kieran,
>
> Am 31.01.2024 um 10:52 schrieb Kieran Bingham:
>> Hi Umang,
>>
>> + Cc: Matthias
>>
>> Quoting Umang Jain (2024-01-31 05:52:04)
>>> Register 0x3a00 is a reserved field as per the IMX335 datasheet,
>>> hence shouldn't be set by the driver.
>>
>> We still need to explain more about why we're dropping this register
>> write, and what effects it causes.
>>
>> Matthias - I believe this stemmed from the work you did, and I think I
>> recall that you stated this register write broke the CSI2 configuration?
>>
>> Can you clarify anything here for us please?
>
> yes, that's correct.
>
> Since this driver originally did not work in my setup, I came across 
> this register while searching for differences to my working reference 
> configuration.
> With the default value of this register (0x00), the driver works 
> perfectly. With the value previously written to it by the driver 
> (0x01), I cannot receive any frames.
> The problem may depend on the link frequency used.
> I can only use and test a frequency of 445.5MHz on my hardware. Since 
> only link frequencies of 594MHz were supported so far, this may not 
> have been a problem.
>
> Unfortunately I do not have a description of this register, so I can 
> only speculate about the exact cause.

Is it worth to frame the commit message around this speculation ?

My setup has no effect with this register being set or not.
>
> ~Matthias
>
>>
>> -- 
>> Kieran
>>
>>
>>>
>>> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
>>> ---
>>>   drivers/media/i2c/imx335.c | 1 -
>>>   1 file changed, 1 deletion(-)
>>>
>>> diff --git a/drivers/media/i2c/imx335.c b/drivers/media/i2c/imx335.c
>>> index 7a37eb327ff4..927b4806a5d7 100644
>>> --- a/drivers/media/i2c/imx335.c
>>> +++ b/drivers/media/i2c/imx335.c
>>> @@ -249,7 +249,6 @@ static const struct imx335_reg 
>>> mode_2592x1940_regs[] = {
>>>          {0x3794, 0x7a},
>>>          {0x3796, 0xa1},
>>>          {0x37b0, 0x36},
>>> -       {0x3a00, 0x01},
>>>   };
>>>     static const struct imx335_reg raw10_framefmt_regs[] = {
>>> -- 
>>> 2.41.0
>>>
Sakari Ailus Feb. 15, 2024, 12:50 p.m. UTC | #4 
Hi folks,

On Wed, Feb 07, 2024 at 09:51:32AM +0530, Umang Jain wrote:
> Hi,
> 
> On 1/31/24 4:02 PM, Matthias Fend wrote:
> > Hi Kieran,
> > 
> > Am 31.01.2024 um 10:52 schrieb Kieran Bingham:
> > > Hi Umang,
> > > 
> > > + Cc: Matthias
> > > 
> > > Quoting Umang Jain (2024-01-31 05:52:04)
> > > > Register 0x3a00 is a reserved field as per the IMX335 datasheet,
> > > > hence shouldn't be set by the driver.
> > > 
> > > We still need to explain more about why we're dropping this register
> > > write, and what effects it causes.
> > > 
> > > Matthias - I believe this stemmed from the work you did, and I think I
> > > recall that you stated this register write broke the CSI2 configuration?
> > > 
> > > Can you clarify anything here for us please?
> > 
> > yes, that's correct.
> > 
> > Since this driver originally did not work in my setup, I came across
> > this register while searching for differences to my working reference
> > configuration.
> > With the default value of this register (0x00), the driver works
> > perfectly. With the value previously written to it by the driver (0x01),
> > I cannot receive any frames.
> > The problem may depend on the link frequency used.
> > I can only use and test a frequency of 445.5MHz on my hardware. Since
> > only link frequencies of 594MHz were supported so far, this may not have
> > been a problem.
> > 
> > Unfortunately I do not have a description of this register, so I can
> > only speculate about the exact cause.
> 
> Is it worth to frame the commit message around this speculation ?
> 
> My setup has no effect with this register being set or not.

Very interesting case indeed.

Please elaborate this a little in the commit message, but the message
shouldn't be that because the register is marked rewerved, it shouldn't be
written (it's a commonplace when it comes to sensors :().
diff mbox series

Patch

diff --git a/drivers/media/i2c/imx335.c b/drivers/media/i2c/imx335.c
index 7a37eb327ff4..927b4806a5d7 100644
--- a/drivers/media/i2c/imx335.c
+++ b/drivers/media/i2c/imx335.c
@@ -249,7 +249,6 @@  static const struct imx335_reg mode_2592x1940_regs[] = {
 	{0x3794, 0x7a},
 	{0x3796, 0xa1},
 	{0x37b0, 0x36},
-	{0x3a00, 0x01},
 };
 
 static const struct imx335_reg raw10_framefmt_regs[] = {