Hi Laurent,
Thanks for your comments.
On Fri, Jun 16, 2023 at 06:07:36PM +0300, Laurent Pinchart wrote:
> On Fri, Jun 16, 2023 at 04:56:15PM +0200, Tommaso Merciai wrote:
> > On Fri, Jun 16, 2023 at 05:17:06PM +0300, Laurent Pinchart wrote:
> > > On Fri, Jun 16, 2023 at 04:15:09PM +0200, Tommaso Merciai wrote:
> > > > On Fri, Jun 16, 2023 at 04:41:24PM +0300, Laurent Pinchart wrote:
> > > > > On Fri, Jun 16, 2023 at 12:20:16AM +0200, Tommaso Merciai wrote:
> > > > > > On Thu, Jun 15, 2023 at 07:52:36PM +0300, Laurent Pinchart wrote:
> > > > > > > On Thu, Jun 15, 2023 at 06:15:43PM +0200, Tommaso Merciai wrote:
> > > > > > > > On Thu, Jun 15, 2023 at 02:00:46PM +0200, Hans de Goede wrote:
> > > > > > > > > On 6/15/23 13:54, Tommaso Merciai wrote:
> > > > > > > > > > On Thu, Jun 15, 2023 at 01:26:25PM +0200, Tommaso Merciai wrote:
> > > > > > > > > >> On Thu, Jun 15, 2023 at 01:10:40PM +0200, Hans de Goede wrote:
> > > > > > > > > >>> 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)
> > > > > > >
> > > > > > > I would add a int *err argument to your read and write wrappers.
> > > > > >
> > > > > >
> > > > > > Thanks for your hint!
> > > > > > What about using:
> > > > > >
> > > > > > static int alvium_write(struct alvium_dev *alvium, u32 reg, u64 val)
> > > > > > {
> > > > > > if (reg & REG_BCRM_V4L2) {
> > > > > > reg &= ~REG_BCRM_V4L2;
> > > > > > reg += alvium->bcrm_addr;
> > > > > > }
> > > > > >
> > > > > > return cci_write(alvium->regmap, reg, val, NULL);
> > > > > > }
> > > > > >
> > > > > > Then:
> > > > > >
> > > > > > ret = alvium_write(alvium, reg, val);
> > > > > > if (ret) {
> > > > > > dev_err(dev, "Fail to write reg\n");
> > > > > > return ret;
> > > > > > }
> > > > > >
> > > > > >
> > > > > > I prefer to use this format if for you is ok.
> > > > > > Let me know.
> > > > >
> > > > > This is fine when you have to write a single register only, but it makes
> > > > > things more complicated when writing multiple registers. Consider this:
> > > > >
> > > > > int ret;
> > > > >
> > > > > ret = alvium_write(alvium, REG_A, val);
> > > > > if (ret)
> > > > > return ret;
> > > > >
> > > > > ret = alvium_write(alvium, REG_B, val);
> > > > > if (ret)
> > > > > return ret;
> > > > >
> > > > > ret = alvium_write(alvium, REG_C, val);
> > > > > if (ret)
> > > > > return ret;
> > > > >
> > > > > ret = alvium_write(alvium, REG_D, val);
> > > > > if (ret)
> > > > > return ret;
> > > > >
> > > > > return 0;
> > > > >
> > > > > and compare it to
> > > > >
> > > > > int ret = 0;
> > > > >
> > > > > alvium_write(alvium, REG_A, val, &ret);
> > > > > alvium_write(alvium, REG_B, val, &ret);
> > > > > alvium_write(alvium, REG_C, val, &ret);
> > > > > alvium_write(alvium, REG_D, val, &ret);
> > > > >
> > > > > return ret;
> > > >
> > > > Is worth to add is also in alvium_write_hshake right?
> >
> > Checking this... :)
> >
> > Most of the driver don't use so much sequence of writes/reads
> > and to be honest I want know where it fail
> >
> >
> > For example:
> >
> > static int alvium_write_hshake(struct alvium_dev *alvium, u32 reg, u64 val,
> > int *err)
> > {
> > struct device *dev = &alvium->i2c_client->dev;
> > u64 hshake_regval;
> > u8 hshake_bit;
> >
> > if (err && *err)
> > return *err;
> >
> > if (!alvium->bcrm_addr)
> > return -EINVAL;
> >
> > /* reset handshake bit */
> > alvium_write(alvium, REG_BCRM_WRITE_HANDSHAKE_RW, 0, err);
> >
> > /* write alvium reg*/
> > alvium_write(alvium, reg, val, err);
>
> You want to return here in case of error, as the poll loop below has no
> chance of succeeding.
>
> >
> > /* poll handshake bit since bit0 = 1*/
> > do {
> > alvium_read(alvium, REG_BCRM_WRITE_HANDSHAKE_RW, &hshake_regval, err);
> > hshake_bit = (hshake_regval & BCRM_HANDSHAKE_W_DONE_EN_BIT);
> >
> > } while (!hshake_bit);
>
> This needs a timeout. The read_poll_timeout() macro can be useful. Same
> below.
Thanks! :)
>
> >
> > /* reset handshake bit, write 0 to bit0 */
> > alvium_write(alvium, REG_BCRM_WRITE_HANDSHAKE_RW, 0, err);
>
> This also needs to return if an error occurs.
>
> >
> > /* poll handshake bit since bit0 = 0 */
> > do {
> > alvium_read(alvium, REG_BCRM_WRITE_HANDSHAKE_RW, &hshake_regval, err);
> > hshake_bit = (hshake_regval & BCRM_HANDSHAKE_W_DONE_EN_BIT);
> >
> > } while (hshake_bit);
> >
> > return *err;
> > }
> >
> > If some write/read fail and have the same regs I want to know where the issue
> > is... Then I have to go back to the previous implementation..
> >
> > I know that also cci API provides some print.
> > But maybe the older version is more straight forwed under debug
> > perspective...
> >
> > Maybe is better to have more code in this case
> > This is my first impression following your way :)
> >
> > Do you agree on this?
>
> My comment was related to the callers of this function, not its internal
> implementation. If there's a common pattern where alvium_write_hshake()
> is called in a sequence of write operations then it should have an int
> *err argument.
My question is:
Except in the alvium_write_hshake function where you can see these
sequence of write/read operations? Same for _hshake_wrtie.
In my opinion there are not to much sequence of read/write operation
into the alvium driver, no?
I'm missing something?
Thanks & Regards,
Tommaso
>
> > > I'd say it's worth everywhere you can have multiple writes.
> > >
> > > > > > > > {
> > > > > > > > int ret;
> > > > > > > >
> > > > > > > > if (reg & REG_BCRM_V4L2)
> > > > > > > > reg += alvium->bcrm_addr;
> > > > > > > >
> > > > > > >
> > > > > > > You should also clear the REG_BCRM_V4L2 bit here:
> > > > > > >
> > > > > > > if (reg & REG_BCRM_V4L2) {
> > > > > > > reg &= ~REG_BCRM_V4L2;
> > > > > > > reg += alvium->bcrm_addr;
> > > > > > > }
> > > > > > >
> > > > > > > > cci_read(alvium->regmap, reg, val, &ret);
> > > > > > > > if (ret)
> > > > > > > > return ret;
> > > > > > > >
> > > > > > > > return 0;
> > > > > > >
> > > > > > > Just
> > > > > > >
> > > > > > > return cci_read(alvium->regmap, reg, val, err);
> > > > > > >
> > > > > > > Same for alvium_write()..
> > > > > > >
> > > > > > > > }
> > > > > > > >
> > > > > > > > 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.
> > > > > > >
> > > > > > > This looks good to me.
> > > > > > >
> > > > > > > The fact that both Hans' helpers and part of the Alvium camera registers
> > > > > > > are named CCI is not a complete coincidence, but it doesn't mean they're
> > > > > > > identical. I would thus keep the REG_BCRM_CCI_* macros for clarity,
> > > > > > > simply defining them as CCI_* wrappers:
> > > > > > >
> > > > > > > #define REG_BCRM_V4L2_8BIT(n) CCI_REG8(n)
> > > > > > > ...
> > > > > > >
> > > > > > > > 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! :)
> > > > > > > >
> > > > > > > > > > ?
> > > > > > > > > >
> > > > > > > > > >>> 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.
> > > > > > > > > >>
> > > > > > > > > >>>> Let me know.
> > > > > > > > > >>>> Thanks! :)
> > > > > > > > > >>>>
> > > > > > > > > >>>> 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:
> > > > > > > > > >>>>>> On 6/14/23 23:48, Sakari Ailus wrote:
> > > > > > > > > >>>>>>> On Thu, Jun 15, 2023 at 12:34:29AM +0300, Laurent Pinchart wrote:
> > > > > > > > > >>>>>>>> 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
>
> --
> Regards,
>
> Laurent Pinchart
