| Message ID | 20210127144930.2158242-1-robert.foss@linaro.org |
|---|---|
| Headers | show |
| Series | Add support for the SDM845 Camera Subsystem | expand |
On Wed, Jan 27, 2021 at 10:56 PM Robert Foss <robert.foss@linaro.org> wrote: > > In order to support Qualcomm ISP hardware architectures that diverge > from older architectures, the VFE subdevice driver needs to be refactored > to better abstract the different ISP architectures. > > Gen1 represents the CAMSS ISP architecture. The ISP architecture developed > after CAMSS, Titan, will be referred to as Gen2. > > Signed-off-by: Robert Foss <robert.foss@linaro.org> > --- > [snip] > diff --git a/drivers/media/platform/qcom/camss/camss-vfe-4-8.c b/drivers/media/platform/qcom/camss/camss-vfe-4-8.c > new file mode 100644 > index 000000000000..153e0e20664e > --- /dev/null > +++ b/drivers/media/platform/qcom/camss/camss-vfe-4-8.c > [snip] > +/* > + * vfe_isr - VFE module interrupt handler > + * @irq: Interrupt line > + * @dev: VFE device > + * > + * Return IRQ_HANDLED on success > + */ > +static irqreturn_t vfe_isr(int irq, void *dev) > +{ > + struct vfe_device *vfe = dev; > + u32 value0, value1; > + int i, j; > + > + vfe->ops->isr_read(vfe, &value0, &value1); > + > + trace_printk("VFE: status0 = 0x%08x, status1 = 0x%08x\n", > + value0, value1); Please do not use trace_printk in production code [1,2], it is only meant for debug use. Consider using trace events, or dev_dbg. [1] https://elixir.bootlin.com/linux/v5.8/source/kernel/trace/trace.c#L3158 [2] https://elixir.bootlin.com/linux/v5.8/source/include/linux/kernel.h#L766 > [snip]
Hey Nicolas, Thanks for the review! On Thu, 28 Jan 2021 at 01:19, Nicolas Boichat <drinkcat@chromium.org> wrote: > > On Wed, Jan 27, 2021 at 10:56 PM Robert Foss <robert.foss@linaro.org> wrote: > > > > In order to support Qualcomm ISP hardware architectures that diverge > > from older architectures, the VFE subdevice driver needs to be refactored > > to better abstract the different ISP architectures. > > > > Gen1 represents the CAMSS ISP architecture. The ISP architecture developed > > after CAMSS, Titan, will be referred to as Gen2. > > > > Signed-off-by: Robert Foss <robert.foss@linaro.org> > > --- > > [snip] > > diff --git a/drivers/media/platform/qcom/camss/camss-vfe-4-8.c b/drivers/media/platform/qcom/camss/camss-vfe-4-8.c > > new file mode 100644 > > index 000000000000..153e0e20664e > > --- /dev/null > > +++ b/drivers/media/platform/qcom/camss/camss-vfe-4-8.c > > [snip] > > +/* > > + * vfe_isr - VFE module interrupt handler > > + * @irq: Interrupt line > > + * @dev: VFE device > > + * > > + * Return IRQ_HANDLED on success > > + */ > > +static irqreturn_t vfe_isr(int irq, void *dev) > > +{ > > + struct vfe_device *vfe = dev; > > + u32 value0, value1; > > + int i, j; > > + > > + vfe->ops->isr_read(vfe, &value0, &value1); > > + > > + trace_printk("VFE: status0 = 0x%08x, status1 = 0x%08x\n", > > + value0, value1); > > Please do not use trace_printk in production code [1,2], it is only > meant for debug use. Consider using trace events, or dev_dbg. Ack, this is a copy paste error, I'll add a commit fixing all occurrences of this in the driver > > [1] https://elixir.bootlin.com/linux/v5.8/source/kernel/trace/trace.c#L3158 > [2] https://elixir.bootlin.com/linux/v5.8/source/include/linux/kernel.h#L766 > > > [snip]
On Wed, 27 Jan 2021 15:49:23 +0100, Robert Foss wrote: > Add bindings for qcom,sdm660-camss in order to support the camera > subsystem on SDM630/660 and SDA variants. > > Signed-off-by: Robert Foss <robert.foss@linaro.org> > --- > > Changes since v2 > - Rob: Add new line at end of file > - Rob: Remove redundant descriptions > - Rob: Add power domain description > - Rob: Make clock-lanes a constant > - Rob: Rework to conform to new port schema > - Add max & minItems to data-lanes > - Remove ports requirement - endpoint & reg > - Added Angelo as binding maintainer > - Removed Todor as binding maintainer > > > .../bindings/media/qcom,sdm660-camss.yaml | 398 ++++++++++++++++++ > 1 file changed, 398 insertions(+) > create mode 100644 Documentation/devicetree/bindings/media/qcom,sdm660-camss.yaml > My bot found errors running 'make dt_binding_check' on your patch: yamllint warnings/errors: dtschema/dtc warnings/errors: Documentation/devicetree/bindings/media/qcom,sdm660-camss.example.dts:21:18: fatal error: dt-bindings/clock/qcom,mmcc-sdm660.h: No such file or directory 21 | #include <dt-bindings/clock/qcom,mmcc-sdm660.h> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ compilation terminated. make[1]: *** [scripts/Makefile.lib:344: Documentation/devicetree/bindings/media/qcom,sdm660-camss.example.dt.yaml] Error 1 make[1]: *** Waiting for unfinished jobs.... make: *** [Makefile:1370: dt_binding_check] Error 2 See https://patchwork.ozlabs.org/patch/1432255 This check can fail if there are any dependencies. The base for a patch series is generally the most recent rc1. If you already ran 'make dt_binding_check' and didn't see the above error(s), then make sure 'yamllint' is installed and dt-schema is up to date: pip3 install dtschema --upgrade Please check and re-submit.
On Sat, 30 Jan 2021 at 18:23, Rob Herring <robh@kernel.org> wrote: > > On Wed, 27 Jan 2021 15:49:23 +0100, Robert Foss wrote: > > Add bindings for qcom,sdm660-camss in order to support the camera > > subsystem on SDM630/660 and SDA variants. > > > > Signed-off-by: Robert Foss <robert.foss@linaro.org> > > --- > > > > Changes since v2 > > - Rob: Add new line at end of file > > - Rob: Remove redundant descriptions > > - Rob: Add power domain description > > - Rob: Make clock-lanes a constant > > - Rob: Rework to conform to new port schema > > - Add max & minItems to data-lanes > > - Remove ports requirement - endpoint & reg > > - Added Angelo as binding maintainer > > - Removed Todor as binding maintainer > > > > > > .../bindings/media/qcom,sdm660-camss.yaml | 398 ++++++++++++++++++ > > 1 file changed, 398 insertions(+) > > create mode 100644 Documentation/devicetree/bindings/media/qcom,sdm660-camss.yaml > > > > My bot found errors running 'make dt_binding_check' on your patch: > > yamllint warnings/errors: > > dtschema/dtc warnings/errors: > Documentation/devicetree/bindings/media/qcom,sdm660-camss.example.dts:21:18: fatal error: dt-bindings/clock/qcom,mmcc-sdm660.h: No such file or directory > 21 | #include <dt-bindings/clock/qcom,mmcc-sdm660.h> > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > compilation terminated. > make[1]: *** [scripts/Makefile.lib:344: Documentation/devicetree/bindings/media/qcom,sdm660-camss.example.dt.yaml] Error 1 > make[1]: *** Waiting for unfinished jobs.... > make: *** [Makefile:1370: dt_binding_check] Error 2 This is expected and mentioned in the cover letter due to a dependency on an as of yet unmerged mmcc-sdm660 series. > > See https://patchwork.ozlabs.org/patch/1432255 > > This check can fail if there are any dependencies. The base for a patch > series is generally the most recent rc1. > > If you already ran 'make dt_binding_check' and didn't see the above > error(s), then make sure 'yamllint' is installed and dt-schema is up to > date: > > pip3 install dtschema --upgrade > > Please check and re-submit. >
On Wed, 27 Jan 2021 15:49:23 +0100, Robert Foss wrote: > Add bindings for qcom,sdm660-camss in order to support the camera > subsystem on SDM630/660 and SDA variants. > > Signed-off-by: Robert Foss <robert.foss@linaro.org> > --- > > Changes since v2 > - Rob: Add new line at end of file > - Rob: Remove redundant descriptions > - Rob: Add power domain description > - Rob: Make clock-lanes a constant > - Rob: Rework to conform to new port schema > - Add max & minItems to data-lanes > - Remove ports requirement - endpoint & reg > - Added Angelo as binding maintainer > - Removed Todor as binding maintainer > > > .../bindings/media/qcom,sdm660-camss.yaml | 398 ++++++++++++++++++ > 1 file changed, 398 insertions(+) > create mode 100644 Documentation/devicetree/bindings/media/qcom,sdm660-camss.yaml > Reviewed-by: Rob Herring <robh@kernel.org>