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.
Then I'm not completely sure that cci fits my use case.
What do you think about?
Btw really great work! :)
Thanks,
Tommaso
>
> Regards,
>
> Hans
>
>
>
> >
> > ?
> >
> >>
> >>>
> >>> Note I'm about to send out a v3 addressing some small
> >>> remarks on this v2. I'll Cc you on that.
> >>
> >> Thanks, in this way I can test that and let you know my feedback.
> >>
> >> Regards,
> >> Tommaso
> >>
> >>>
> >>> Regards,
> >>>
> >>> Hans
> >>>
> >>>
> >>>>
> >>>> Let me know.
> >>>> Thanks! :)
> >>>>
> >>>> Regards,
> >>>> Tommaso
> >>>>
> >>>> On Thu, Jun 15, 2023 at 12:21:00PM +0300, Laurent Pinchart wrote:
> >>>>> On Thu, Jun 15, 2023 at 11:11:20AM +0200, Hans de Goede wrote:
> >>>>>> Hi Sakari,
> >>>>>>
> >>>>>> On 6/14/23 23:48, Sakari Ailus wrote:
> >>>>>>> Hi Laurent,
> >>>>>>>
> >>>>>>> On Thu, Jun 15, 2023 at 12:34:29AM +0300, Laurent Pinchart wrote:
> >>>>>>>> Hello,
> >>>>>>>>
> >>>>>>>> On Wed, Jun 14, 2023 at 08:39:56PM +0000, Sakari Ailus wrote:
> >>>>>>>>> On Wed, Jun 14, 2023 at 09:23:39PM +0200, Hans de Goede wrote:
> >>>>>>>>>> The CSI2 specification specifies a standard method to access camera sensor
> >>>>>>>>>> registers called "Camera Control Interface (CCI)".
> >>>>>>>>>>
> >>>>>>>>>> This uses either 8 or 16 bit (big-endian wire order) register addresses
> >>>>>>>>>> and supports 8, 16, 24 or 32 bit (big-endian wire order) register widths.
> >>>>>>>>>>
> >>>>>>>>>> Currently a lot of Linux camera sensor drivers all have their own custom
> >>>>>>>>>> helpers for this, often copy and pasted from other drivers.
> >>>>>>>>>>
> >>>>>>>>>> Add a set of generic helpers for this so that all sensor drivers can
> >>>>>>>>>> switch to a single common implementation.
> >>>>>>>>>>
> >>>>>>>>>> These helpers take an extra optional "int *err" function parameter,
> >>>>>>>>>> this can be used to chain a bunch of register accesses together with
> >>>>>>>>>> only a single error check at the end, rather then needing to error
> >>>>>>>>>> check each individual register access. The first failing call will
> >>>>>>>>>> set the contents of err to a non 0 value and all other calls will
> >>>>>>>>>> then become no-ops.
> >>>>>>>>>>
> >>>>>>>>>> Link: https://lore.kernel.org/linux-media/59aefa7f-7bf9-6736-6040-39551329cd0a@xxxxxxxxxx/
> >>>>>>>>>> Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx>
> >>>>>>>>>> ---
> >>>>>>>>>> Changes in v2:
> >>>>>>>>>> - Drop cci_reg_type enum
> >>>>>>>>>> - Make having an encoded reg-width mandatory rather then using 0 to encode
> >>>>>>>>>> 8 bit width making reg-addresses without an encoded width default to
> >>>>>>>>>> a width of 8
> >>>>>>>>>> - Add support for 64 bit wide registers
> >>>>>>>>
> >>>>>>>> I'm in two minds about this. This means that the read and write
> >>>>>>>> functions take a u64 argument, which will be less efficient on 32-bit
> >>>>>>>> platforms. I think it would be possible, with some macro magic, to
> >>>>>>>> accept different argument sizes, but maybe that's a micro-optimization
> >>>>>>>> that would actually result in worse code.
> >>>>>>>>
> >>>>>>>> 64-bit support could be useful, but as far as I can tell, it's not used
> >>>>>>>> in this series, so maybe we could leave this for later ?
> >>>>>>>
> >>>>>>> I prefer to have it now, I just told Tommaso working on the Alvium driver
> >>>>>>> to use this, and he needs 64-bit access. :-)
> >>>>>>>
> >>>>>>> You could also easily have 32-bit and 64-bit variant of the functions, with
> >>>>>>> C11 _Generic(). Introducing it now would be easier than later.
> >>>>>>
> >>>>>> I took a quick look at C11 _Generic() and that looks at the type
> >>>>>> of "things" so in this case it would look at type of the val argument.
> >>>>>>
> >>>>>> Problem is that that can still be e.g. an int when doing a
> >>>>>> read/write from a 64 bit registers.
> >>>>>>
> >>>>>> So we would then need to handle the 64 bit width case in the 32
> >>>>>> bit versions of the functions too.
> >>>>>>
> >>>>>> And likewise I can see someone passing a long on a 64 bit
> >>>>>> arch while doing a cci_write() to a non 64 bit register.
> >>>>>>
> >>>>>> So this would basically mean copy and pasting cci_read()
> >>>>>> + cci_write() 2x with the only difference being one
> >>>>>> variant taking a 32 bit val argument and the other a
> >>>>>> 64 bit val argument.
> >>>>>>
> >>>>>> This seems like premature optimization to me.
> >>>>>>
> >>>>>> As mentioned in my reply to Laurent if we want to
> >>>>>> optimize things we really should look at avoiding
> >>>>>> unnecessary i2c transfers, or packing multiple
> >>>>>> writes into a single i2c transfer for writes to
> >>>>>> subsequent registers. That is where significant
> >>>>>> speedups can be made.
> >>>>>
> >>>>> This is something I'd really like to see, but it's way more work.
> >>>>>
> >>>>> There's an important need of applying changes atomically, which is often
> >>>>> not possible to strictly guarantee over I2C. Userspace ends up writing
> >>>>> V4L2 controls as quickly as it can after the start of a frame, hoping
> >>>>> they will all reach the sensor before the end of the frame. Some
> >>>>> platforms have camera-specific I2C controllers that have the ability to
> >>>>> buffer I2C transfers and issue them based on a hardware trigger. How to
> >>>>> fit this in thé kernel I2C API will be an interesting exercise.
> >>>>>
> >>>>> --
> >>>>> Regards,
> >>>>>
> >>>>> Laurent Pinchart
> >>>>
> >>>
> >
>
