Re: [PATCH v2 1/5] media: Add MIPI CCI register access helper functions

[Hans de Goede <hdegoede@redhat.com>]

Hi,

On 6/19/23 10:13, Tommaso Merciai wrote:
> Hi Hans, Laurent,
> 
> On Fri, Jun 16, 2023 at 06:54:54PM +0200, Hans de Goede wrote:
>> Hi Tommaso,
>>
>> On 6/15/23 18:15, Tommaso Merciai wrote:
>>> Hi Hans,
>>>
>>> On Thu, Jun 15, 2023 at 02:00:46PM +0200, Hans de Goede wrote:
>>>> Hi,
>>>>
>>>> On 6/15/23 13:54, Tommaso Merciai wrote:
>>>>> On Thu, Jun 15, 2023 at 01:26:25PM +0200, Tommaso Merciai wrote:
>>>>>> Hi Hans,
>>>>>>
>>>>>> On Thu, Jun 15, 2023 at 01:10:40PM +0200, Hans de Goede wrote:
>>>>>>> Hi Tommaso,
>>>>>>>
>>>>>>> On 6/15/23 12:05, Tommaso Merciai wrote:
>>>>>>>> Hi Hans, Laurent, Sakari,
>>>>>>>>
>>>>>>>> Can I cherry-pick this patch and use these new functions also
>>>>>>>> for cci regs of the alvium driver?
>>>>>>>
>>>>>>> Yes that sounds like a good plan.
>>>>>>>
>>>>>>>> Are on going to be merge?
>>>>>>>
>>>>>>> Yes this will hopefully get merged upstream soon.
>>>>>>
>>>>>> Thanks for the info!
>>>>>>
>>>>>> I want to ask you your opinion about this:
>>>>>>
>>>>>> Into alvium driver actually I'm using the following defines
>>>>>> manipulations:
>>>>>>
>>>>>> #define REG_BCRM_REG_ADDR_R				REG_BCRM_CCI_16BIT(0x0014)
>>>>>>
>>>>>> #define REG_BCRM_FEATURE_INQUIRY_R			REG_BCRM_V4L2_64BIT(0x0008)
>>>>>> #define REG_BCRM_DEVICE_FIRMWARE_VERSION_R		REG_BCRM_V4L2_64BIT(0x0010)
>>>>>>
>>>>>> My plan is to use your cci API for cci register in this way defines
>>>>>> became like:
>>>>>>
>>>>>> #define REG_BCRM_REG_ADDR_R				CCI_REG16(0x0014)
>>>>>>
>>>>>> And leave v4l2 regs are it are right now:
>>>>>>
>>>>>> #define REG_BCRM_FEATURE_INQUIRY_R			REG_BCRM_V4L2_64BIT(0x0008)
>>>>>> #define REG_BCRM_DEVICE_FIRMWARE_VERSION_R		REG_BCRM_V4L2_64BIT(0x0010)
>>>>>>
>>>>>> What do you think about?
>>>>>
>>>>> Or maybe is worth don't use v4l2 bit for v4l2 regs, I think is implicit
>>>>> that what regs that are not CCI are v4l2, then we return wit the
>>>>> following defines:
>>>>>
>>>>>
>>>>>
>>>>> #define REG_BCRM_REG_ADDR_R                           CCI_REG16(0x0014)
>>>>> ^CCI regs
>>>>>
>>>>> #define REG_BCRM_FEATURE_INQUIRY_R                    0x0008 
>>>>> #define REG_BCRM_DEVICE_FIRMWARE_VERSION_R            0x0010
>>>>> ^v4l2 regs
>>>>
>>>> I'm not sure what you mean with "V4L2" registers ? I guess you mean
>>>> registers which cannot be accessed through the CCI helper functions,
>>>> but starting with v2 this is no longer true. There now is a CCI_REG64()
>>>> so you can simply use that.
>>>
>>> I'm playing a bit with v3 of your cci api :)
>>>
>>> My problem is the following, bcrm regs are not real regs but are offset
>>> from bcrm address (this is not fixed, it depends on the camera).
>>>
>>> Then the workflow is:
>>>
>>>  - read bcrm_address (base address)
>>>  - then sum this to the offset (regs)
>>>
>>> Myabe this clarify:
>>>
>>> static int alvium_read(struct alvium_dev *alvium, u32 reg, u64 *val)
>>> {
>>> 	int ret;
>>>
>>> 	if (reg & REG_BCRM_V4L2)
>>> 		reg += alvium->bcrm_addr;
>>>
>>> 	cci_read(alvium->regmap, reg, val, &ret);
>>> 	if (ret)
>>> 		return ret;
>>>
>>> 	return 0;
>>> }
>>>
>>> int alvium_write(struct alvium_dev *alvium, u32 reg, u64 val)
>>> {
>>> 	int ret;
>>>
>>> 	if (reg & REG_BCRM_V4L2)
>>> 		reg += alvium->bcrm_addr;
>>>
>>> 	cci_write(alvium->regmap, reg, val, &ret);
>>> 	if (ret)
>>> 		return ret;
>>>
>>> 	return 0;
>>> }
>>>
>>> Where for example:
>>>
>>> #define REG_BCRM_V4L2		BIT(31)
>>> #define REG_BCRM_V4L2_64BIT(n)	(REG_BCRM_V4L2 | CCI_REG64(n))
>>>
>>> #define REG_BCRM_WRITE_HANDSHAKE_RW REG_BCRM_V4L2_8BIT(0x0418)
>>>
>>>
>>> But I'm not sure that I'm in the right direction. 
>>>
>>> In real I need first to get the real address then sum the bcrm_address
>>> if this is a bcrm regs(offset) then re-incapsule the address into the
>>> right CCI_REG# defines.
>>
>> Ah I see, so you have a set of windowed registers where
>> the base address of these registers may change.
> 
> Yep, right :)
> 
>>
>> What I don't understand though is why you use V4L2 in the
>> name of the #defines for this? Does the datasheet actually
>> name them like this ? V4L2 stands for video4linux version 2,
>> so unless these registers are somehow Linux specific using
>> V4L2 in the #define names is a bit weird IMHO.
> 
> These registers are offered from the alvium fw for v4l2 API.
> We had a previous discussion with Laurent about this.

Ah, ok that explains. Then the V4L2 in the register #defines makes
sense and I'm fine with it.

Regards,

Hans