Hi Jacopo,
On Fri, Oct 07, 2022 at 09:17:25AM +0200, Jacopo Mondi wrote:
> On Fri, Oct 07, 2022 at 07:20:51AM +0200, Krzysztof Hałasa wrote:
> > Jacopo Mondi writes:
> >
> > > +static int ar0521_read_reg(struct ar0521_dev *sensor, u16 reg, u16 *val)
> > > +{
> > > + struct i2c_client *client = sensor->i2c_client;
> > > + unsigned char buf[2];
> > > + struct i2c_msg msg;
> > > + int ret;
> > > +
> > > + msg.addr = client->addr;
> > > + msg.flags = client->flags;
> > > + msg.len = sizeof(u16);
> > > + msg.buf = buf;
> > > + put_unaligned_be16(reg, buf);
> > > +
> > > + ret = i2c_transfer(client->adapter, &msg, 1);
> > > + if (ret < 0)
> > > + return ret;
> > > +
> > > + msg.len = sizeof(u16);
> > > + msg.flags = client->flags | I2C_M_RD;
> > > + msg.buf = buf;
> > > +
> > > + ret = i2c_transfer(client->adapter, &msg, 1);
> > > + if (ret < 0)
> > > + return ret;
> > > +
> > > + *val = get_unaligned_be16(buf);
> > > +
> > > + return 0;
> > > +}
> >
> > Why not simply use a shadow register?
>
> Sorry I didn't get you. Care to expand ?
I think Krzysztof meant caching the value in the ar0521_dev structure,
so it doesn't have to be read back. I2C is slow, let's avoid reads as
much as possible.
This being said, if all gain controls are in the same cluster, you won't
need to read back or cache anything yourself, the control framework will
handle that for you.
> > > +static int ar0521_set_analog_gain(struct ar0521_dev *sensor)
> > > +{
> > > + u16 global_gain;
> > > + int ret;
> > > +
> > > + ret = ar0521_read_reg(sensor, AR0521_REG_GLOBAL_GAIN, &global_gain);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + global_gain &= ~AR0521_REG_GLOBAL_GAIN_ANA_MASK;
> > > + global_gain |= sensor->ctrls.ana_gain->val & AR0521_REG_GLOBAL_GAIN_ANA_MASK;
> > > +
> > > + return ar0521_write_reg(sensor, AR0521_REG_GLOBAL_GAIN, global_gain);
> >
> > This one is simple: you can't write to AR0521_REG_GLOBAL_GAIN.
>
> Uh
>
> I can guarantee you it works :)
>
> > You can write to individual color gain registers (any will do for analog
> > gain), but writing to AR0521_REG_GLOBAL_GAIN will reset all the digital
> > gains as well. Reading the register doesn't give you anything
>
> I think that's ok, isn't it ? If one wants to control the global gain
> it goes through this register, if individual gains need to be
> configured one should not set the global gain ?
The issue is that if the user has set different digital gains for the
different channels, you will overwrite them with the same below for all
channels. That's not good.
What you could experiment with is register 0x0204
(analog_gain_code_global) which seem to provide a global analog gain
without overwriting the digital gains, but it's not entirely clear from
the documentation if it will work. The register name comes from
SMIA++/CCS, but the documentation doesn't match the coarse/fine gain
model, experiments would be needed. Another option is register 0x3028,
which is also named analog_gain_code_global, but is documented
differently.
Could you btw read registers 0x0000 to 0x00ff and provide the data ?
> > interesting, either (the analog gain which you overwrite anyway).
>
> The high bits are the global digital gain, and I need to read its value in
> order not to overwrite them.
>
> > BTW ISP can't really do that color balancing for you, since the sensor
> > operates at its native bit resolution and ISP is limited to the output
> > format, which is currently only 8-bit.
>
> I'm not sure what do you mean here either :)
I'm also not sure to see the problem.
--
Regards,
Laurent Pinchart
