Re: [PATCH v3 21/25] drivers: media: i2c: imx258: Use macros

From: Luis Garcia
Date: Sat Apr 06 2024 - 01:26:43 EST

On 4/5/24 08:11, Tommaso Merciai wrote:
> Hi Luis,
>
> On Fri, Apr 05, 2024 at 04:33:38AM -0600, Luis Garcia wrote:
>> On 4/4/24 00:46, Sakari Ailus wrote:
>>> On Wed, Apr 03, 2024 at 01:17:26PM -0600, Luigi311 wrote:
>>>> On 4/3/24 10:23, Sakari Ailus wrote:
>>>>> Hi Luis,
>>>>>
>>>>> On Wed, Apr 03, 2024 at 09:03:50AM -0600, git@xxxxxxxxxxxx wrote:
>>>>>> From: Luis Garcia <git@xxxxxxxxxxxx>
>>>>>>
>>>>>> Use understandable macros instead of raw values.
>>>>>>
>>>>>> Signed-off-by: Ondrej Jirman <megi@xxxxxx>
>>>>>> Signed-off-by: Luis Garcia <git@xxxxxxxxxxxx>
>>>>>> ---
>>>>>> drivers/media/i2c/imx258.c | 434 ++++++++++++++++++-------------------
>>>>>> 1 file changed, 207 insertions(+), 227 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/media/i2c/imx258.c b/drivers/media/i2c/imx258.c
>>>>>> index e2ecf6109516..30352c33f63c 100644
>>>>>> --- a/drivers/media/i2c/imx258.c
>>>>>> +++ b/drivers/media/i2c/imx258.c
>>>>>> @@ -33,8 +33,6 @@
>>>>>> #define IMX258_VTS_30FPS_VGA 0x034c
>>>>>> #define IMX258_VTS_MAX 65525
>>>>>>
>>>>>> -#define IMX258_REG_VTS 0x0340
>>>>>> -
>>>>>> /* HBLANK control - read only */
>>>>>> #define IMX258_PPL_DEFAULT 5352
>>>>>>
>>>>>> @@ -90,6 +88,53 @@
>>>>>> #define IMX258_PIXEL_ARRAY_WIDTH 4208U
>>>>>> #define IMX258_PIXEL_ARRAY_HEIGHT 3120U
>>>>>>
>>>>>> +/* regs */
>>>>>> +#define IMX258_REG_PLL_MULT_DRIV 0x0310
>>>>>> +#define IMX258_REG_IVTPXCK_DIV 0x0301
>>>>>> +#define IMX258_REG_IVTSYCK_DIV 0x0303
>>>>>> +#define IMX258_REG_PREPLLCK_VT_DIV 0x0305
>>>>>> +#define IMX258_REG_IOPPXCK_DIV 0x0309
>>>>>> +#define IMX258_REG_IOPSYCK_DIV 0x030b
>>>>>> +#define IMX258_REG_PREPLLCK_OP_DIV 0x030d
>>>>>> +#define IMX258_REG_PHASE_PIX_OUTEN 0x3030
>>>>>> +#define IMX258_REG_PDPIX_DATA_RATE 0x3032
>>>>>> +#define IMX258_REG_SCALE_MODE 0x0401
>>>>>> +#define IMX258_REG_SCALE_MODE_EXT 0x3038
>>>>>> +#define IMX258_REG_AF_WINDOW_MODE 0x7bcd
>>>>>> +#define IMX258_REG_FRM_LENGTH_CTL 0x0350
>>>>>> +#define IMX258_REG_CSI_LANE_MODE 0x0114
>>>>>> +#define IMX258_REG_X_EVN_INC 0x0381
>>>>>> +#define IMX258_REG_X_ODD_INC 0x0383
>>>>>> +#define IMX258_REG_Y_EVN_INC 0x0385
>>>>>> +#define IMX258_REG_Y_ODD_INC 0x0387
>>>>>> +#define IMX258_REG_BINNING_MODE 0x0900
>>>>>> +#define IMX258_REG_BINNING_TYPE_V 0x0901
>>>>>> +#define IMX258_REG_FORCE_FD_SUM 0x300d
>>>>>> +#define IMX258_REG_DIG_CROP_X_OFFSET 0x0408
>>>>>> +#define IMX258_REG_DIG_CROP_Y_OFFSET 0x040a
>>>>>> +#define IMX258_REG_DIG_CROP_IMAGE_WIDTH 0x040c
>>>>>> +#define IMX258_REG_DIG_CROP_IMAGE_HEIGHT 0x040e
>>>>>> +#define IMX258_REG_SCALE_M 0x0404
>>>>>> +#define IMX258_REG_X_OUT_SIZE 0x034c
>>>>>> +#define IMX258_REG_Y_OUT_SIZE 0x034e
>>>>>> +#define IMX258_REG_X_ADD_STA 0x0344
>>>>>> +#define IMX258_REG_Y_ADD_STA 0x0346
>>>>>> +#define IMX258_REG_X_ADD_END 0x0348
>>>>>> +#define IMX258_REG_Y_ADD_END 0x034a
>>>>>> +#define IMX258_REG_EXCK_FREQ 0x0136
>>>>>> +#define IMX258_REG_CSI_DT_FMT 0x0112
>>>>>> +#define IMX258_REG_LINE_LENGTH_PCK 0x0342
>>>>>> +#define IMX258_REG_SCALE_M_EXT 0x303a
>>>>>> +#define IMX258_REG_FRM_LENGTH_LINES 0x0340
>>>>>> +#define IMX258_REG_FINE_INTEG_TIME 0x0200
>>>>>> +#define IMX258_REG_PLL_IVT_MPY 0x0306
>>>>>> +#define IMX258_REG_PLL_IOP_MPY 0x030e
>>>>>> +#define IMX258_REG_REQ_LINK_BIT_RATE_MBPS_H 0x0820
>>>>>> +#define IMX258_REG_REQ_LINK_BIT_RATE_MBPS_L 0x0822
>>>>>> +
>>>>>> +#define REG8(a, v) { a, v }
>>>>>> +#define REG16(a, v) { a, ((v) >> 8) & 0xff }, { (a) + 1, (v) & 0xff }
>>>>>
>>>>> The patch is nice but these macros are better replaced by the V4L2 CCI
>>>>> helper that also offers register access functions. Could you add a patch to
>>>>> convert the driver to use it (maybe after this one)?
>>>>>
>>>>
>>>> Ohh perfect, using something else would be great. Ill go ahead and see
>>>> if I can get that working.
>>>
>>> Thanks. It may be easier to just do it in this one actually. Up to you.
>>>
>>
>> I've made the swap but looks like its not playing nice with my ppp,
>> its causing a crash and showing a call trace as soon as it does its
>> first read to check the identity. I went in and dropped the cci_read
>> and left it with the original implementation and I'm getting a very
>> similar crash with cci_write too so it looks like its not liking
>> how I'm implementing it. Looking at the few other drivers that were
>> swapped over to use that, I don't seem to be missing anything. It's
>> a big change so its not really something I can describe what I've
>> changed but I do have the change on my github here
>> https://github.com/luigi311/linux/commit/840593acb20eee87ce361e6929edf51eefbbe737
>
> I checked your commit to switch to cci helper.
> I think you are missing the right cci regmap initialization.
>
> Please take care to use: devm_cci_regmap_init_i2c
>
> /**
> * devm_cci_regmap_init_i2c() - Create regmap to use with cci_*() register
> * access functions
> *
> * @client: i2c_client to create the regmap for
> * @reg_addr_bits: register address width to use (8 or 16)
> *
> * Note the memory for the created regmap is devm() managed, tied to the client.
> *
> * Return: %0 on success or a negative error code on failure.
> */
>
> Check drivers/media/i2c/imx290.c:1530
> Hope this help :)
>
> Note:
> Somewhere into the github commit you are reading 16bit reg and storing
> that into 64bit val. Take care! :)
>
> Thanks & Regards,
> Tommaso
>
>

Ohh my god your right, looks like i missed that when i was replicating
the cci patch for that same driver lol. Looks like that fixed it and
the driver is now loading so i can continue to debug it and make sure
everything is set correct since i had a few that were messed up since
i couldn't test it.

>> if you can provide some guidance, if not I can skip this change
>> all together and we can do a separate attempt at swapping over to it.
>>