-
Notifications
You must be signed in to change notification settings - Fork 5.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
add mira220 image sensor #6717
base: rpi-6.12.y
Are you sure you want to change the base?
add mira220 image sensor #6717
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've started to review this, but I'm noticing a lot of my comments from #6552 haven't been addressed.
If embedded data isn't required, then do not include it.
}; | ||
}; | ||
|
||
fragment@202 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fragment is dormant and not referenced by an override.
}; | ||
}; | ||
|
||
fragment@201 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fragment is dormant and not referenced by an override.
data-lanes = <1 2>; | ||
// clock-noncontinuous; | ||
link-frequencies = | ||
/bits/ 64 <456000000>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this the correct link frequency for this sensor?
|
||
/* Embedded metadata stream structure */ | ||
#define MIRA220_EMBEDDED_LINE_WIDTH 16384 | ||
#define MIRA220_NUM_EMBEDDED_LINES 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this sensor actually have useful embedded metadata? It's disabled in libcamera.
I'd made the same comment in #6552 (comment) that this isn't how mainline are implementing metadata, so it's simpler to not implement it in the driver if not needed.
// 1600_1400_30fps_12b_2lanes | ||
|
||
static const struct mira220_reg full_1600_1400_1500_12b_2lanes_reg[] = { | ||
{0x1003,0x2}, //,Initial Upload |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comment isn't useful.
|
||
// 1600_1400_30fps_12b_2lanes | ||
|
||
static const struct mira220_reg full_1600_1400_1500_12b_2lanes_reg[] = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You have about 18 registers that differ between these 3 huge tables. Break out all the common stuff into a common table, and only send the bits that change per mode
#define MIRA220_PIXEL_ARRAY_WIDTH 1600U | ||
#define MIRA220_PIXEL_ARRAY_HEIGHT 1400U | ||
|
||
#define MIRA220_ANALOG_GAIN_REG 0x400A |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comment made at #6552 (comment) that the V4L2_CCI is preferred rather than bare register defines as it encodes the size of each register.
static const char * const mira220_supply_name[] = { | ||
// TODO(jalv): Check supply names | ||
/* Supplies can be enabled in any order */ | ||
"VANA", /* Analog (2.8V) supply */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Regulators should be named in lower case.
add driver / device tree support for ams mira220
needed changes also done for libcamera.