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

[Luis Garcia]

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
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.