Hi Bryan,
---
drivers/media/platform/qcom/camss/Makefile | 1 +
.../platform/qcom/camss/camss-csid-gen3.c | 339 ++++++++++++++++++
.../platform/qcom/camss/camss-csid-gen3.h | 26 ++
So this "gen2" and "gen3" stuff would make sense if we had a number of
SoCs based on gen2 and gen3 which were controlled from the upper-level
gen2.c and gen3.c.
What you're submitting here is csid-780 so the file should be named
csid-780.
When we add 680 or 880 then it makes sense to try to encapsulate a class
of generation into one file - potentially.
I'd guess that was the intent behind gen2.c.
TL;DR please name your file csid-xxx.c
Sure, I will use csid-780.c
+
+ writel(val, csid->base + CSID_CSI2_RX_CFG0);
+
+ val = 1 << CSI2_RX_CFG1_ECC_CORRECTION_EN;
+ if (vc > 3)
+ val |= 1 << CSI2_RX_CFG1_VC_MODE;
So again these are needless bit-shifts.
#define CSI2_RX_CFG1_PACKET_ECC_CORRECTION_EN BIT(0)
val = CSI2_RX_CFG1_PACKET_ECC_CORRECTION_EN;
You posted same comments in v3 series, I also replied it.
https://lore.kernel.org/all/eeaf4f4e-5200-4b13-b38f-3f3385fc2a2b@xxxxxxxxxxx/
Some of register bits which just need to be configured to 0 or 1, then
can use BIT(X), but some register bits need to configure a specific
value, e.g., CSID_RDI_CFG0 bits[22:26] need to configure a vc vaule,
bits[16:21] need to configure a dt value, then we can't use BIT(x) to
handle this.
+
+static void csid_subdev_reg_update(struct csid_device *csid, int
port_id, bool is_clear)
+{
+ if (is_clear) {
+ csid->reg_update &= ~REG_UPDATE_RDI(csid, port_id);
+ } else {
+ csid->reg_update |= REG_UPDATE_RDI(csid, port_id);
+ writel(csid->reg_update, csid->base + CSID_REG_UPDATE_CMD);
+ }
+}
Right so this function should
1. Write the register
2. Wait on a completion
See camss-vfe-480.c::vfe_isr_reg_update()
3. Have that completion fire in the CSID ISR
4. Or timeout
5. Returning either 0 for success or -ETIMEDOUT
to the calling function so that we can be sure the RUP interrupt has
fired and completed - or we have appropriately timed out and captured
the failure.
Also - in camss-vfe-480.c the ISR clears the RUP which one assumes is
still the required logical flow with the RUP now residing in CSID.
Sure, I forget to add this, will add them in next series.
case MSM_CSID_PAD_SRC:
- if (csid->testgen_mode->cur.val == 0) {
+ if (!csid->testgen_mode || csid->testgen_mode->cur.val == 0) {
See my comments on adding new guards to core functionality.
Is this sm8550 specific or generic ?
It is sm8550 specific, since we don't have testgen mode in sm8550 csid,
so need to add some guards, the guards are added for similar reason.
/* Test generator is disabled, */
/* keep pad formats in sync */
u32 code = fmt->code;
@@ -1042,6 +1042,7 @@ static int csid_init_formats(struct v4l2_subdev
*sd, struct v4l2_subdev_fh *fh)
static int csid_set_test_pattern(struct csid_device *csid, s32 value)
{
struct csid_testgen_config *tg = &csid->testgen;
+ const struct csid_hw_ops *hw_ops = csid->res->hw_ops;
/* If CSID is linked to CSIPHY, do not allow to enable test
generator */
if (value && media_pad_remote_pad_first(&csid-
>pads[MSM_CSID_PAD_SINK]))
@@ -1049,7 +1050,10 @@ static int csid_set_test_pattern(struct
csid_device *csid, s32 value)
tg->enabled = !!value;
- return csid->res->hw_ops->configure_testgen_pattern(csid, value);
+ if (hw_ops->configure_testgen_pattern)
+ return -EOPNOTSUPP;
+ else
+ return hw_ops->configure_testgen_pattern(csid, value);
If you just add a dummy configure_testgen_pattern we can get rid of this
branching stuff.
Do you mean add dummy function in csid-780/gen3.c? How about the other
ops in vfe_ops_780, add dummy function or use NULL? We need to guards if
we set it as NULL.
static int csid_configure_testgen_pattern(struct csid_device *csid, s32 val)
{
return 0;
}
}
/*
@@ -1121,6 +1125,19 @@ int msm_csid_subdev_init(struct camss *camss,
struct csid_device *csid,
csid->base = devm_platform_ioremap_resource_byname(pdev,
res->reg[0]);
if (IS_ERR(csid->base))
return PTR_ERR(csid->base);
+
+ /* CSID "top" is a new function in new version HW,
+ * CSID can connect to VFE & SFE(Sensor Front End).
+ * this connection is controlled by CSID "top" registers.
+ * There is only one CSID "top" region for all CSIDs.
+ */
+ if (!csid_is_lite(csid) && res->reg[1] && !camss-
>csid_top_base) {
+ camss->csid_top_base =
+ devm_platform_ioremap_resource_byname(pdev, res-
>reg[1]);
That's a complex clause.
Let me send you a patch to do it a different way.
I was also thinking to addd it in camss level, then I thought it is in
csid block, so I moved it to csid, but it is also fine to add it in
camss. Can I add your patch into this series? Just like the csiphy patches.
Thanks,
Depeng
