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.
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.
Regards,
Hans
