+ mutex_destroy(&cap_dev->lock);
+ ret = -EINVAL;
+ goto err_destroy_mutex;
+ }
+
+ cap_dev->pad.flags = MEDIA_PAD_FL_SINK;
+ ret = media_entity_pads_init(&cap_dev->vdev.entity, 1, &cap_dev->pad);
+ if (ret) {
+ mutex_destroy(&cap_dev->lock);
+ goto err_destroy_mutex;
+ }
+
+ vb2q->type = V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE;
+ vb2q->io_modes = VB2_MMAP | VB2_DMABUF;
+ vb2q->drv_priv = cap_dev;
+ vb2q->mem_ops = &vb2_dma_contig_memops;
+ vb2q->ops = &mali_c55_vb2_ops;
+ vb2q->buf_struct_size = sizeof(struct mali_c55_buffer);
+ vb2q->min_queued_buffers = 1;
+ vb2q->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
+ vb2q->lock = &cap_dev->lock;
+ vb2q->dev = mali_c55->dev;
+
+ ret = vb2_queue_init(vb2q);
+ if (ret) {
+ dev_err(mali_c55->dev, "%s vb2 queue init failed\n",
+ mali_c55_cap_dev_to_name(cap_dev));
+ goto err_cleanup_media_entity;
+ }
+
+ strscpy(cap_dev->vdev.name, capture_device_names[i],
+ sizeof(cap_dev->vdev.name));
+ vdev->release = video_device_release_empty;
+ vdev->fops = &mali_c55_v4l2_fops;
+ vdev->ioctl_ops = &mali_c55_v4l2_ioctl_ops;
+ vdev->lock = &cap_dev->lock;
+ vdev->v4l2_dev = &mali_c55->v4l2_dev;
+ vdev->queue = &cap_dev->queue;
+ vdev->device_caps = V4L2_CAP_VIDEO_CAPTURE_MPLANE |
+ V4L2_CAP_STREAMING | V4L2_CAP_IO_MC;
+ vdev->entity.ops = &mali_c55_media_ops;
+ video_set_drvdata(vdev, cap_dev);
+
+ memset(&pix_mp, 0, sizeof(pix_mp));
+ pix_mp.pixelformat = V4L2_PIX_FMT_RGB565;
+ pix_mp.width = MALI_C55_DEFAULT_WIDTH;
+ pix_mp.height = MALI_C55_DEFAULT_HEIGHT;
+ mali_c55_set_format(cap_dev, &pix_mp);
+
+ ret = video_register_device(vdev, VFL_TYPE_VIDEO, -1);
+ if (ret) {
+ dev_err(mali_c55->dev,
+ "%s failed to register video device\n",
+ mali_c55_cap_dev_to_name(cap_dev));
+ goto err_release_vb2q;
+ }
+ }
+
+ return 0;
+
+err_release_vb2q:
+ vb2_queue_release(vb2q);
+err_cleanup_media_entity:
+ media_entity_cleanup(&cap_dev->vdev.entity);
+err_destroy_mutex:
+ mutex_destroy(&cap_dev->lock);
+ mali_c55_unregister_capture_devs(mali_c55);
+
+ return ret;
+}
+
+void mali_c55_unregister_capture_devs(struct mali_c55 *mali_c55)
+{
+ struct mali_c55_cap_dev *cap_dev;
+ unsigned int i;
+
+ for (i = 0; i < ARRAY_SIZE(mali_c55->cap_devs); i++) {
+ cap_dev = &mali_c55->cap_devs[i];
+
+ if (!video_is_registered(&cap_dev->vdev))
+ continue;
+
+ vb2_video_unregister_device(&cap_dev->vdev);
+ media_entity_cleanup(&cap_dev->vdev.entity);
+ mutex_destroy(&cap_dev->lock);
+ }
+}
diff --git a/drivers/media/platform/arm/mali-c55/mali-c55-common.h
b/drivers/media/platform/arm/mali-c55/mali-c55-common.h
new file mode 100644
index 000000000000..2d0c4d152beb
--- /dev/null
+++ b/drivers/media/platform/arm/mali-c55/mali-c55-common.h
@@ -0,0 +1,266 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * ARM Mali-C55 ISP Driver - Common definitions
+ *
+ * Copyright (C) 2024 Ideas on Board Oy
+ */
+
+#ifndef _MALI_C55_COMMON_H
+#define _MALI_C55_COMMON_H
+
+#include <linux/clk.h>
+#include <linux/io.h>
+#include <linux/list.h>
+#include <linux/mutex.h>
+#include <linux/scatterlist.h>
I don't think this is needed. You're however missing spinlock.h.
+#include <linux/videodev2.h>
+
+#include <media/media-device.h>
+#include <media/v4l2-async.h>
+#include <media/v4l2-ctrls.h>
+#include <media/v4l2-dev.h>
+#include <media/v4l2-device.h>
+#include <media/v4l2-subdev.h>
+#include <media/videobuf2-core.h>
+#include <media/videobuf2-v4l2.h>
+
+#define MALI_C55_DRIVER_NAME "mali-c55"
+
+/* min and max values for the image sizes */
+#define MALI_C55_MIN_WIDTH 640U
+#define MALI_C55_MIN_HEIGHT 480U
+#define MALI_C55_MAX_WIDTH 8192U
+#define MALI_C55_MAX_HEIGHT 8192U
+#define MALI_C55_DEFAULT_WIDTH 1920U
+#define MALI_C55_DEFAULT_HEIGHT 1080U
+
+#define MALI_C55_DEFAULT_MEDIA_BUS_FMT MEDIA_BUS_FMT_RGB121212_1X36
+
+struct mali_c55;
+struct mali_c55_cap_dev;
+struct platform_device;
You should also forward-declare
struct device;
struct dma_chan;
struct resource;
+
+static const char * const mali_c55_clk_names[] = {
+ "aclk",
+ "hclk",
+};
This will end up duplicating the array in each compilation unit, not
great. Move it to mali-c55-core.c. You use it in this file just for its
size, replace that with a macro that defines the size, or allocate
mali_c55.clks dynamically with devm_kcalloc().
+
+enum mali_c55_interrupts {
+ MALI_C55_IRQ_ISP_START,
+ MALI_C55_IRQ_ISP_DONE,
+ MALI_C55_IRQ_MCM_ERROR,
+ MALI_C55_IRQ_BROKEN_FRAME_ERROR,
+ MALI_C55_IRQ_MET_AF_DONE,
+ MALI_C55_IRQ_MET_AEXP_DONE,
+ MALI_C55_IRQ_MET_AWB_DONE,
+ MALI_C55_IRQ_AEXP_1024_DONE,
+ MALI_C55_IRQ_IRIDIX_MET_DONE,
+ MALI_C55_IRQ_LUT_INIT_DONE,
+ MALI_C55_IRQ_FR_Y_DONE,
+ MALI_C55_IRQ_FR_UV_DONE,
+ MALI_C55_IRQ_DS_Y_DONE,
+ MALI_C55_IRQ_DS_UV_DONE,
+ MALI_C55_IRQ_LINEARIZATION_DONE,
+ MALI_C55_IRQ_RAW_FRONTEND_DONE,
+ MALI_C55_IRQ_NOISE_REDUCTION_DONE,
+ MALI_C55_IRQ_IRIDIX_DONE,
+ MALI_C55_IRQ_BAYER2RGB_DONE,
+ MALI_C55_IRQ_WATCHDOG_TIMER,
+ MALI_C55_IRQ_FRAME_COLLISION,
+ MALI_C55_IRQ_UNUSED,
+ MALI_C55_IRQ_DMA_ERROR,
+ MALI_C55_IRQ_INPUT_STOPPED,
+ MALI_C55_IRQ_MET_AWB_TARGET1_HIT,
+ MALI_C55_IRQ_MET_AWB_TARGET2_HIT,
+ MALI_C55_NUM_IRQ_BITS
Those are register bits, I think they belong to mali-c55-registers.h,
and should probably be macros instead of an enum.
+};
+
+enum mali_c55_isp_pads {
+ MALI_C55_ISP_PAD_SINK_VIDEO,
As there's a single sink pad, maybe MALI_C55_ISP_PAD_SINK ? Ah, you're
probably preparing for ISP parameters support. It's fine.
+ MALI_C55_ISP_PAD_SOURCE,
Then maybe this should be named MALI_C55_ISP_PAD_SOURCE_VIDEO as I
assume there will be a stats source pad.
+ MALI_C55_ISP_PAD_SOURCE_BYPASS,
+ MALI_C55_ISP_NUM_PADS,
+};
+
+struct mali_c55_tpg {
+ struct mali_c55 *mali_c55;
+ struct v4l2_subdev sd;
+ struct media_pad pad;
+ struct mutex lock;
+ struct mali_c55_tpg_ctrls {
+ struct v4l2_ctrl_handler handler;
+ struct v4l2_ctrl *test_pattern;
Set but never used. You can drop it.
+ struct v4l2_ctrl *hblank;
Set and used only once, in the same function. You can make it a local
variable.
+ struct v4l2_ctrl *vblank;
+ } ctrls;
+};
I wonder if this file should be split, with mali-c55-capture.h,
mali-c55-core.h, mali-c55-isp.h, ... I think it could increase
readability by clearly separating the different elements. Up to you.
+
+struct mali_c55_isp {
+ struct mali_c55 *mali_c55;
+ struct v4l2_subdev sd;
+ struct media_pad pads[MALI_C55_ISP_NUM_PADS];
+ struct media_pad *remote_src;
+ struct v4l2_async_notifier notifier;
I'm tempted to move the notifier to mali_c55, as it's related to
components external to the whole ISP, not to the ISP subdev itself.
Could you give it a try, to see if it could be done without any drawback
?
+ struct mutex lock;
Locks require a comment to explain what they protect. Same below where
applicable (for both mutexes and spinlocks).
+ unsigned int frame_sequence;
+};
+
+enum mali_c55_resizer_ids {
+ MALI_C55_RZR_FR,
+ MALI_C55_RZR_DS,
+ MALI_C55_NUM_RZRS,
The usual abbreviation for "resizer" in drivers/media/ is "rsz", not
"rzr". I would have said we can leave it as-is as changing it would be a
bit annoying, but I then realized that "rzr" is not just unusual, it's
actually not used at all. Would you mind applying a sed globally ? :-)
+};
+
+enum mali_c55_rzr_pads {
Same enums/structs use abbreviations, some don't. Consistency would
help.
+ MALI_C55_RZR_SINK_PAD,
+ MALI_C55_RZR_SOURCE_PAD,
+ MALI_C55_RZR_SINK_BYPASS_PAD,
+ MALI_C55_RZR_NUM_PADS
+};
+
+struct mali_c55_resizer {
+ struct mali_c55 *mali_c55;
+ struct mali_c55_cap_dev *cap_dev;
+ enum mali_c55_resizer_ids id;
+ struct v4l2_subdev sd;
+ struct media_pad pads[MALI_C55_RZR_NUM_PADS];
+ unsigned int num_routes;
+ bool streaming;
+};
+
+enum mali_c55_cap_devs {
+ MALI_C55_CAP_DEV_FR,
+ MALI_C55_CAP_DEV_DS,
+ MALI_C55_NUM_CAP_DEVS
+};
+
+struct mali_c55_fmt {
mali_c55_format_info would be a better name I think, as this stores
format information, not formats.
+ u32 fourcc;
+ unsigned int mbus_codes[2];
A comment to explain why we have two media bus codes would be useful.
You can document the whole structure if desired :-)
+ bool is_raw;
+ struct mali_c55_fmt_registers {
Make it an anonymous structure, it's never used anywhere else.
+ unsigned int base_mode;
+ unsigned int uv_plane;
If those are register field values, use u32 instead of unsigned int.
+ } registers;
It's funny, we tend to abbreviate different things, I would have used
"regs" here but written "format" in full in the structure name :-)
+};
+
+enum mali_c55_isp_bayer_order {
+ MALI_C55_BAYER_ORDER_RGGB,
+ MALI_C55_BAYER_ORDER_GRBG,
+ MALI_C55_BAYER_ORDER_GBRG,
+ MALI_C55_BAYER_ORDER_BGGR
These are registers values too, they belong to mali-c55-registers.h.
+};
+
+struct mali_c55_isp_fmt {
mali_c55_isp_format_info
+ u32 code;
+ enum v4l2_pixel_encoding encoding;
Here you use v4l2_pixel_encoding, for mali_c55_fmt you use is_raw. Maybe
pick the same option for both structures ?
+ enum mali_c55_isp_bayer_order order;
+};
+
+enum mali_c55_planes {
+ MALI_C55_PLANE_Y,
+ MALI_C55_PLANE_UV,
+ MALI_C55_NUM_PLANES
+};
+
+struct mali_c55_buffer {
+ struct vb2_v4l2_buffer vb;
+ bool plane_done[MALI_C55_NUM_PLANES];
I think tracking the pending state would simplify the logic in
mali_c55_set_plane_done(), which would become
curr_buf->plane_pending[plane] = false;
if (!curr_buf->plane_pending[0] && !curr_buf->plane_pending[1]) {
mali_c55_handle_buffer(curr_buf, isp->frame_sequence);
cap_dev->buffers.curr = NULL;
isp->frame_sequence++;
}
Or a counter may be even easier (and would consume less memory).
+ struct list_head queue;
+ u32 addrs[MALI_C55_NUM_PLANES];
This stores DMA addresses, use dma_addr_t.
+};
+
+struct mali_c55_cap_dev {
+ struct mali_c55 *mali_c55;
+ struct mali_c55_resizer *rzr;
+ struct video_device vdev;
+ struct media_pad pad;
+ struct vb2_queue queue;
+ struct mutex lock;
+ unsigned int reg_offset;
Manual handling of the offset everywhere, with parametric macros for the
resizer register addresses, isn't very nice. Introduce resizer-specific
accessors and capture-specific accessors (mali_c55_rsz_write(), ...)
that take a mali_c55_resizer or mali_c55_cap_dev pointer, and handle the
offset there. The register macros should loose their offset parameter.
You could also use a single set of accessors that would become
path/output-specific (mali_c55_path_write() ? mali_c55_output_write()
?), that may make the code easier to read.
You can also replace reg_offset with a void __iomem * base, which would
avoid the computation at runtime.
+
+ struct mali_c55_mode {
Make the structure anonymous.
+ const struct mali_c55_fmt *capture_fmt;
+ struct v4l2_pix_format_mplane pix_mp;
+ } mode;
What's a "mode" ? I think I'd name this
struct {
const struct mali_c55_fmt *info;
struct v4l2_pix_format_mplane format;
} format;
Or you could just drop the structure and have
const struct mali_c55_fmt *format_info;
struct v4l2_pix_format_mplane format;
or something similar.
+
+ struct {
+ spinlock_t lock;
+ struct list_head queue;
+ struct mali_c55_buffer *curr;
+ struct mali_c55_buffer *next;
+ } buffers;
+
+ bool streaming;
+};
+
+enum mali_c55_config_spaces {
+ MALI_C55_CONFIG_PING,
+ MALI_C55_CONFIG_PONG,
+ MALI_C55_NUM_CONFIG_SPACES
The last enumerator is not used.
+};
+
+struct mali_c55_ctx {
mali_c55_context ?
+ struct mali_c55 *mali_c55;
+ void *registers;
Please document this structure and explain that this field points to a
copy of the register space in system memory, I was about to write you're
missing __iomem :-)
+ phys_addr_t base;
+ spinlock_t lock;
+ struct list_head list;
+};
+
+struct mali_c55 {
+ struct device *dev;
+ struct resource *res;
You could possibly drop this field by passing the physical address of
the register space from mali_c55_probe() to mali_c55_init_context() as a
function parameter.
+ void __iomem *base;
+ struct dma_chan *channel;
+ struct clk_bulk_data clks[ARRAY_SIZE(mali_c55_clk_names)];
+
+ u16 capabilities;
+ struct media_device media_dev;
+ struct v4l2_device v4l2_dev;
+ struct media_pipeline pipe;
+
+ struct mali_c55_tpg tpg;
+ struct mali_c55_isp isp;
+ struct mali_c55_resizer resizers[MALI_C55_NUM_RZRS];
+ struct mali_c55_cap_dev cap_devs[MALI_C55_NUM_CAP_DEVS];
+
+ struct list_head contexts;
+ enum mali_c55_config_spaces next_config;
+};
+
+void mali_c55_write(struct mali_c55 *mali_c55, unsigned int addr, u32 val);
+u32 mali_c55_read(struct mali_c55 *mali_c55, unsigned int addr,
+ bool force_hardware);
+void mali_c55_update_bits(struct mali_c55 *mali_c55, unsigned int addr,
+ u32 mask, u32 val);
+int mali_c55_config_write(struct mali_c55_ctx *ctx,
+ enum mali_c55_config_spaces cfg_space);
+
+int mali_c55_register_isp(struct mali_c55 *mali_c55);
+int mali_c55_register_tpg(struct mali_c55 *mali_c55);
+void mali_c55_unregister_tpg(struct mali_c55 *mali_c55);
+void mali_c55_unregister_isp(struct mali_c55 *mali_c55);
+int mali_c55_register_resizers(struct mali_c55 *mali_c55);
+void mali_c55_unregister_resizers(struct mali_c55 *mali_c55);
+int mali_c55_register_capture_devs(struct mali_c55 *mali_c55);
+void mali_c55_unregister_capture_devs(struct mali_c55 *mali_c55);
+struct mali_c55_ctx *mali_c55_get_active_context(struct mali_c55 *mali_c55);
+void mali_c55_set_plane_done(struct mali_c55_cap_dev *cap_dev,
+ enum mali_c55_planes plane);
+void mali_c55_set_next_buffer(struct mali_c55_cap_dev *cap_dev);
+void mali_c55_isp_queue_event_sof(struct mali_c55 *mali_c55);
+
+bool mali_c55_format_is_raw(unsigned int mbus_code);
+void mali_c55_rzr_start_stream(struct mali_c55_resizer *rzr);
+void mali_c55_rzr_stop_stream(struct mali_c55_resizer *rzr);
+int mali_c55_isp_start_stream(struct mali_c55_isp *isp);
+void mali_c55_isp_stop_stream(struct mali_c55_isp *isp);
+
+const struct mali_c55_isp_fmt *
+mali_c55_isp_fmt_next(const struct mali_c55_isp_fmt *fmt);
+bool mali_c55_isp_is_format_supported(unsigned int mbus_code);
+#define for_each_mali_isp_fmt(fmt)\
#define for_each_mali_isp_fmt(fmt) \
+ for ((fmt) = NULL; ((fmt) = mali_c55_isp_fmt_next((fmt)));)
Looks like parentheses were on sale :-)
for ((fmt) = NULL; (fmt) = mali_c55_isp_fmt_next(fmt); )
This macro is used in two places only, in the mali-c55-isp.c file where
open-coding the loop without using mali_c55_isp_fmt_next() would be more
efficient, and in mali-c55-resizer.c where a function to return format
i'th would be more efficient. I think you can drop the macro and the
mali_c55_isp_fmt_next() function.
+
+#endif /* _MALI_C55_COMMON_H */
diff --git a/drivers/media/platform/arm/mali-c55/mali-c55-core.c
b/drivers/media/platform/arm/mali-c55/mali-c55-core.c
new file mode 100644
index 000000000000..50caf5ee7474
--- /dev/null
+++ b/drivers/media/platform/arm/mali-c55/mali-c55-core.c
@@ -0,0 +1,767 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * ARM Mali-C55 ISP Driver - Core driver code
+ *
+ * Copyright (C) 2024 Ideas on Board Oy
+ */
+
+#include <linux/bitops.h>
+#include <linux/cleanup.h>
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/device.h>
+#include <linux/dmaengine.h>
+#include <linux/dma-mapping.h>
+#include <linux/interrupt.h>
+#include <linux/iopoll.h>
+#include <linux/ioport.h>
+#include <linux/mod_devicetable.h>
+#include <linux/of.h>
+#include <linux/of_reserved_mem.h>
+#include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
+#include <linux/scatterlist.h>
I don't think this is needed.
Missing slab.h.
+#include <linux/string.h>
+
+#include <media/media-entity.h>
+#include <media/v4l2-device.h>
+#include <media/videobuf2-dma-contig.h>
+
+#include "mali-c55-common.h"
+#include "mali-c55-registers.h"
+
+static const char * const mali_c55_interrupt_names[] = {
+ [MALI_C55_IRQ_ISP_START] = "ISP start",
+ [MALI_C55_IRQ_ISP_DONE] = "ISP done",
+ [MALI_C55_IRQ_MCM_ERROR] = "Multi-context management error",
+ [MALI_C55_IRQ_BROKEN_FRAME_ERROR] = "Broken frame error",
+ [MALI_C55_IRQ_MET_AF_DONE] = "AF metering done",
+ [MALI_C55_IRQ_MET_AEXP_DONE] = "AEXP metering done",
+ [MALI_C55_IRQ_MET_AWB_DONE] = "AWB metering done",
+ [MALI_C55_IRQ_AEXP_1024_DONE] = "AEXP 1024-bit histogram done",
+ [MALI_C55_IRQ_IRIDIX_MET_DONE] = "Iridix metering done",
+ [MALI_C55_IRQ_LUT_INIT_DONE] = "LUT memory init done",
+ [MALI_C55_IRQ_FR_Y_DONE] = "Full resolution Y plane DMA done",
+ [MALI_C55_IRQ_FR_UV_DONE] = "Full resolution U/V plane DMA done",
+ [MALI_C55_IRQ_DS_Y_DONE] = "Downscale Y plane DMA done",
+ [MALI_C55_IRQ_DS_UV_DONE] = "Downscale U/V plane DMA done",
+ [MALI_C55_IRQ_LINEARIZATION_DONE] = "Linearisation done",
+ [MALI_C55_IRQ_RAW_FRONTEND_DONE] = "Raw frontend processing done",
+ [MALI_C55_IRQ_NOISE_REDUCTION_DONE] = "Noise reduction done",
+ [MALI_C55_IRQ_IRIDIX_DONE] = "Iridix done",
+ [MALI_C55_IRQ_BAYER2RGB_DONE] = "Bayer to RGB conversion done",
+ [MALI_C55_IRQ_WATCHDOG_TIMER] = "Watchdog timer timed out",
+ [MALI_C55_IRQ_FRAME_COLLISION] = "Frame collision error",
+ [MALI_C55_IRQ_UNUSED] = "IRQ bit unused",
+ [MALI_C55_IRQ_DMA_ERROR] = "DMA error",
+ [MALI_C55_IRQ_INPUT_STOPPED] = "Input port safely stopped",
+ [MALI_C55_IRQ_MET_AWB_TARGET1_HIT] = "AWB metering target 1 address hit",
+ [MALI_C55_IRQ_MET_AWB_TARGET2_HIT] = "AWB metering target 2 address hit"
+};
+
+static unsigned int config_space_addrs[] = {
const
+ [MALI_C55_CONFIG_PING] = 0x0AB6C,
+ [MALI_C55_CONFIG_PONG] = 0x22B2C,
Lowercase hex constants.
Don't the values belong to mali-c55-registers.h ?
+};
+
+/* System IO
/*
* System IO
+ *
+ * The Mali-C55 ISP has up to two configuration register spaces (called 'ping'
+ * and 'pong'), with the expectation that the 'active' space will be left
s/the /the /
+ * untouched whilst a frame is being processed and the 'inactive' space
+ * configured ready to be passed during the blanking period before the next
s/to be passed/to be switched to/ ?
+ * frame processing starts. These spaces should ideally be set via DMA transfer
+ * from a buffer rather than through individual register set operations. There
+ * is also a shared global register space which should be set normally. Of
+ * course, the ISP might be included in a system which lacks a suitable DMA
+ * engine, and the second configuration space might not be fitted at all, which
+ * means we need to support four scenarios:
+ *
+ * 1. Multi config space, with DMA engine.
+ * 2. Multi config space, no DMA engine.
+ * 3. Single config space, with DMA engine.
+ * 4. Single config space, no DMA engine.
+ *
+ * The first case is very easy, but the rest present annoying problems. The best
+ * way to solve them seems to be simply to replicate the concept of DMAing over
+ * the configuration buffer even if there's no DMA engine on the board, for
+ * which we rely on memcpy. To facilitate this any read/write call that is made
+ * to an address within those config spaces should infact be directed to a
+ * buffer that was allocated to hold them rather than the IO memory itself. The
+ * actual copy of that buffer to IO mem will happen on interrupt.
+ */
+
+void mali_c55_write(struct mali_c55 *mali_c55, unsigned int addr, u32 val)
+{
+ struct mali_c55_ctx *ctx = mali_c55_get_active_context(mali_c55);
+
+ if (addr >= MALI_C55_REG_CONFIG_SPACES_OFFSET) {
+ spin_lock(&ctx->lock);
+ addr = (addr - MALI_C55_REG_CONFIG_SPACES_OFFSET) / 4;
+ ((u32 *)ctx->registers)[addr] = val;
+ spin_unlock(&ctx->lock);
+
+ return;
+ }
Ouch. This is likely the second comment you really won't like (after the
comment regarding mali_c55_update_bits() at the very top). I apologize
in advance.
I really don't like this. Directing writes either to hardware registers
or to the shadow registers in the context makes the callers of the
read/write accessors very hard to read. The probe code, for instance,
mixes writes to hardware registers and writes to the context shadow
registers to initialize the value of some of the shadow registers.
I'd like to split the read/write accessors into functions that access
the hardware registers (that's easy) and functions that access the
shadow registers. I think the latter should receive a mali_c55_ctx
pointer instead of a mali_c55 pointer to prepare for multi-context
support.
You can add WARN_ON() guards to the two sets of functions, to ensure
that no register from the "other" space gets passed to the wrong
function by mistake.
+
+ writel(val, mali_c55->base + addr);
+}
+
+u32 mali_c55_read(struct mali_c55 *mali_c55, unsigned int addr,
+ bool force_hardware)
force_hardware is never set to true.
+{
+ struct mali_c55_ctx *ctx = mali_c55_get_active_context(mali_c55);
+ u32 val;
+
+ if (addr >= MALI_C55_REG_CONFIG_SPACES_OFFSET && !force_hardware) {
+ spin_lock(&ctx->lock);
+ addr = (addr - MALI_C55_REG_CONFIG_SPACES_OFFSET) / 4;
+ val = ((u32 *)ctx->registers)[addr];
+ spin_unlock(&ctx->lock);
+
+ return val;
+ }
+
+ return readl(mali_c55->base + addr);
+}
+
+void mali_c55_update_bits(struct mali_c55 *mali_c55, unsigned int addr,
+ u32 mask, u32 val)
+{
+ u32 orig, tmp;
+
+ orig = mali_c55_read(mali_c55, addr, false);
+
+ tmp = orig & ~mask;
+ tmp |= (val << (ffs(mask) - 1)) & mask;
+
+ if (tmp != orig)
+ mali_c55_write(mali_c55, addr, tmp);
+}
+
+static int mali_c55_dma_xfer(struct mali_c55_ctx *ctx, dma_addr_t src,
+ dma_addr_t dst, enum dma_data_direction dir)
+{
+ struct mali_c55 *mali_c55 = ctx->mali_c55;
+ struct dma_async_tx_descriptor *tx;
+ enum dma_status status;
+ dma_cookie_t cookie;
+
+ tx = dmaengine_prep_dma_memcpy(mali_c55->channel, dst, src,
+ MALI_C55_CONFIG_SPACE_SIZE, 0);
+ if (!tx) {
+ dev_err(mali_c55->dev, "failed to prep DMA memcpy\n");
+ return -EIO;
+ }
+
+ cookie = dmaengine_submit(tx);
+ if (dma_submit_error(cookie)) {
+ dev_err(mali_c55->dev, "error submitting dma transfer\n");
+ return -EIO;
+ }
+
+ status = dma_sync_wait(mali_c55->channel, cookie);
I've just realized this performs a busy-wait :-S See the comment in the
probe function about the threaded IRQ handler. I think we'll need to
rework all this. It could be done on top though.
+ if (status != DMA_COMPLETE) {
+ dev_err(mali_c55->dev, "dma transfer failed\n");
+ return -EIO;
+ }
+
+ return 0;
+}
+
+static int mali_c55_dma_read(struct mali_c55_ctx *ctx,
+ enum mali_c55_config_spaces cfg_space)
+{
+ struct mali_c55 *mali_c55 = ctx->mali_c55;
+ struct device *dma_dev = mali_c55->channel->device->dev;
+ dma_addr_t src = ctx->base + config_space_addrs[cfg_space];
+ dma_addr_t dst;
+ int ret;
+
+ guard(spinlock)(&ctx->lock);
+
+ dst = dma_map_single(dma_dev, ctx->registers,
+ MALI_C55_CONFIG_SPACE_SIZE, DMA_FROM_DEVICE);
+ if (dma_mapping_error(dma_dev, dst)) {
+ dev_err(mali_c55->dev, "failed to map DMA addr\n");
+ return -EIO;
+ }
+
+ ret = mali_c55_dma_xfer(ctx, src, dst, DMA_FROM_DEVICE);
+ dma_unmap_single(dma_dev, dst, MALI_C55_CONFIG_SPACE_SIZE,
+ DMA_FROM_DEVICE);
+
+ return ret;
+}
+
+static int mali_c55_dma_write(struct mali_c55_ctx *ctx,
+ enum mali_c55_config_spaces cfg_space)
+{
+ struct mali_c55 *mali_c55 = ctx->mali_c55;
+ struct device *dma_dev = mali_c55->channel->device->dev;
+ dma_addr_t dst = ctx->base + config_space_addrs[cfg_space];
+ dma_addr_t src;
+ int ret;
+
+ guard(spinlock)(&ctx->lock);
The code below can take a large amount of time, holding a spinlock will
disable interrupts on the local CPU, that's not good :-(
+
+ src = dma_map_single(dma_dev, ctx->registers,
+ MALI_C55_CONFIG_SPACE_SIZE, DMA_TO_DEVICE);
+ if (dma_mapping_error(dma_dev, src)) {
+ dev_err(mali_c55->dev, "failed to map DMA addr\n");
+ return -EIO;
+ }
+
+ ret = mali_c55_dma_xfer(ctx, src, dst, DMA_TO_DEVICE);
+ dma_unmap_single(dma_dev, src, MALI_C55_CONFIG_SPACE_SIZE,
+ DMA_TO_DEVICE);
+
+ return ret;
+}
+
+static int mali_c55_config_read(struct mali_c55_ctx *ctx,
+ enum mali_c55_config_spaces cfg_space)
+{
+ struct mali_c55 *mali_c55 = ctx->mali_c55;
+
+ if (mali_c55->channel) {
+ return mali_c55_dma_read(ctx, cfg_space);
As this function is used at probe time only, to initialize the context,
I think DMA is overkill.
+ } else {
+ memcpy_fromio(ctx->registers,
+ mali_c55->base + config_space_addrs[cfg_space],
+ MALI_C55_CONFIG_SPACE_SIZE);
+ return 0;
+ }
+}
+
+int mali_c55_config_write(struct mali_c55_ctx *ctx,
+ enum mali_c55_config_spaces cfg_space)
+{
+ struct mali_c55 *mali_c55 = ctx->mali_c55;
+
+ if (mali_c55->channel) {
+ return mali_c55_dma_write(ctx, cfg_space);
+ } else {
+ memcpy_toio(mali_c55->base + config_space_addrs[cfg_space],
+ ctx->registers, MALI_C55_CONFIG_SPACE_SIZE);
+ return 0;
+ }
Could you measure the time it typically takes to write the registers
using DMA compared to using memcpy_toio() ?
+}
+
+struct mali_c55_ctx *mali_c55_get_active_context(struct mali_c55 *mali_c55)
+{
+ return list_first_entry(&mali_c55->contexts, struct mali_c55_ctx, list);
I think it's too early to tell how multi-context support will look like.
I'm fine keeping mali_c55_get_active_context() as changing that would be
very intrusive (even if I think it will need to be changed), but the
list of contexts is neither the mechanism we'll use, nor something we
need now. Drop the list, embed the context in struct mali_c55, and
return the pointer to that single context from this function.
+}
+
+static void mali_c55_remove_links(struct mali_c55 *mali_c55)
+{
+ unsigned int i;
+
+ media_entity_remove_links(&mali_c55->tpg.sd.entity);
+ media_entity_remove_links(&mali_c55->isp.sd.entity);
+
+ for (i = 0; i < MALI_C55_NUM_RZRS; i++)
+ media_entity_remove_links(&mali_c55->resizers[i].sd.entity);
+
+ for (i = 0; i < MALI_C55_NUM_CAP_DEVS; i++)
+ media_entity_remove_links(&mali_c55->cap_devs[i].vdev.entity);
+}
+
+static int mali_c55_create_links(struct mali_c55 *mali_c55)
+{
+ int ret;
+
+ /* Test pattern generator to ISP */
+ ret = media_create_pad_link(&mali_c55->tpg.sd.entity, 0,
+ &mali_c55->isp.sd.entity,
+ MALI_C55_ISP_PAD_SINK_VIDEO, 0);
+ if (ret) {
+ dev_err(mali_c55->dev, "failed to link TPG and ISP\n");
+ goto err_remove_links;
+ }
+
+ /* Full resolution resizer pipe. */
+ ret = media_create_pad_link(&mali_c55->isp.sd.entity,
+ MALI_C55_ISP_PAD_SOURCE,
+ &mali_c55->resizers[MALI_C55_RZR_FR].sd.entity,
+ MALI_C55_RZR_SINK_PAD,
+ MEDIA_LNK_FL_ENABLED | MEDIA_LNK_FL_IMMUTABLE);
+ if (ret) {
+ dev_err(mali_c55->dev, "failed to link ISP and FR resizer\n");
+ goto err_remove_links;
+ }
+
+ /* Full resolution bypass. */
+ ret = media_create_pad_link(&mali_c55->isp.sd.entity,
+ MALI_C55_ISP_PAD_SOURCE_BYPASS,
+ &mali_c55->resizers[MALI_C55_RZR_FR].sd.entity,
+ MALI_C55_RZR_SINK_BYPASS_PAD,
+ MEDIA_LNK_FL_ENABLED | MEDIA_LNK_FL_IMMUTABLE);
+ if (ret) {
+ dev_err(mali_c55->dev, "failed to link ISP and FR resizer\n");
+ goto err_remove_links;
+ }
+
+ /* Resizer pipe to video capture nodes. */
+ ret = media_create_pad_link(&mali_c55->resizers[0].sd.entity,
+ MALI_C55_RZR_SOURCE_PAD,
+ &mali_c55->cap_devs[MALI_C55_CAP_DEV_FR].vdev.entity,
+ 0, MEDIA_LNK_FL_ENABLED | MEDIA_LNK_FL_IMMUTABLE);
+ if (ret) {
+ dev_err(mali_c55->dev,
+ "failed to link FR resizer and video device\n");
+ goto err_remove_links;
+ }
+
+ /* The downscale pipe is an optional hardware block */
+ if (mali_c55->capabilities & MALI_C55_GPS_DS_PIPE_FITTED) {
+ ret = media_create_pad_link(&mali_c55->isp.sd.entity,
+ MALI_C55_ISP_PAD_SOURCE,
+ &mali_c55->resizers[MALI_C55_RZR_DS].sd.entity,
+ MALI_C55_RZR_SINK_PAD,
+ MEDIA_LNK_FL_ENABLED | MEDIA_LNK_FL_IMMUTABLE);
+ if (ret) {
+ dev_err(mali_c55->dev,
+ "failed to link ISP and DS resizer\n");
+ goto err_remove_links;
+ }
+
+ ret = media_create_pad_link(&mali_c55->resizers[1].sd.entity,
+ MALI_C55_RZR_SOURCE_PAD,
+ &mali_c55->cap_devs[MALI_C55_CAP_DEV_DS].vdev.entity,
+ 0, MEDIA_LNK_FL_ENABLED | MEDIA_LNK_FL_IMMUTABLE);
+ if (ret) {
+ dev_err(mali_c55->dev,
+ "failed to link DS resizer and video device\n");
+ goto err_remove_links;
+ }
+ }
+
+ return 0;
+
+err_remove_links:
+ mali_c55_remove_links(mali_c55);
+ return ret;
+}
+
+static void mali_c55_unregister_entities(struct mali_c55 *mali_c55)
+{
+ mali_c55_unregister_tpg(mali_c55);
+ mali_c55_unregister_isp(mali_c55);
+ mali_c55_unregister_resizers(mali_c55);
+ mali_c55_unregister_capture_devs(mali_c55);
+}
+
+static int mali_c55_register_entities(struct mali_c55 *mali_c55)
+{
+ int ret;
+
+ ret = mali_c55_register_tpg(mali_c55);
+ if (ret)
+ return ret;
+
+ ret = mali_c55_register_isp(mali_c55);
+ if (ret)
+ goto err_unregister_entities;
+
+ ret = mali_c55_register_resizers(mali_c55);
+ if (ret)
+ goto err_unregister_entities;
+
+ ret = mali_c55_register_capture_devs(mali_c55);
+ if (ret)
+ goto err_unregister_entities;
+
+ ret = mali_c55_create_links(mali_c55);
+ if (ret)
+ goto err_unregister_entities;
+
+ return 0;
+
+err_unregister_entities:
+ mali_c55_unregister_entities(mali_c55);
+
+ return ret;
+}
+
+static u32 mali_c55_check_hwcfg(struct mali_c55 *mali_c55)
+{
+ u32 product, version, revision, capabilities;
+
+ product = mali_c55_read(mali_c55, MALI_C55_REG_PRODUCT, false);
+ version = mali_c55_read(mali_c55, MALI_C55_REG_VERSION, false);
+ revision = mali_c55_read(mali_c55, MALI_C55_REG_REVISION, false);
+
+ dev_info(mali_c55->dev, "Detected Mali-C55 ISP %u.%u.%u\n",
+ product, version, revision);
+
+ capabilities = mali_c55_read(mali_c55,
+ MALI_C55_REG_GLOBAL_PARAMETER_STATUS,
+ false);
+ mali_c55->capabilities = (capabilities & 0xffff);
+
+ /* TODO: Might as well start some debugfs */
If it's just to expose the version and capabilities, I think that's
overkill. It's not needed for debug purpose (you can get it from the
kernel log already). debugfs isn't meant to be accessible in production,
so an application that would need access to the information wouldn't be
able to use it.
+ dev_info(mali_c55->dev, "Mali-C55 capabilities: 0x%04x\n", capabilities);
Combine the two messages into one.
+ return version;
+}
+
+static void mali_c55_swap_next_config(struct mali_c55 *mali_c55)
+{
+ struct mali_c55_ctx *ctx = mali_c55_get_active_context(mali_c55);
+ u32 curr_config, next_config;
+
+ curr_config = mali_c55_read(mali_c55, MALI_C55_REG_PING_PONG_READ, false);
+ curr_config = (curr_config & MALI_C55_REG_PING_PONG_READ_MASK)
+ >> (ffs(MALI_C55_REG_PING_PONG_READ_MASK) - 1);
+ next_config = curr_config ^ 1;
+
+ mali_c55_update_bits(mali_c55, MALI_C55_REG_MCU_CONFIG,
+ MALI_C55_REG_MCU_CONFIG_WRITE_MASK, next_config);
+ mali_c55_config_write(ctx, next_config ?
+ MALI_C55_CONFIG_PING : MALI_C55_CONFIG_PONG);
+}
+
+static irqreturn_t mali_c55_isr(int irq, void *context)
+{
+ struct device *dev = context;
+ struct mali_c55 *mali_c55 = dev_get_drvdata(dev);
+ u32 interrupt_status;
+ unsigned int i, j;
+
+ interrupt_status = mali_c55_read(mali_c55,
+ MALI_C55_REG_INTERRUPT_STATUS_VECTOR,
+ false);
+ if (!interrupt_status)
+ return IRQ_NONE;
+
+ mali_c55_write(mali_c55, MALI_C55_REG_INTERRUPT_CLEAR_VECTOR,
+ interrupt_status);
+ mali_c55_write(mali_c55, MALI_C55_REG_INTERRUPT_CLEAR, 0);
+ mali_c55_write(mali_c55, MALI_C55_REG_INTERRUPT_CLEAR, 1);
+
+ for (i = 0; i < MALI_C55_NUM_IRQ_BITS; i++) {
+ if (!(interrupt_status & (1 << i)))
+ continue;
+
+ switch (i) {
+ case MALI_C55_IRQ_ISP_START:
+ mali_c55_isp_queue_event_sof(mali_c55);
+
+ for (j = i; j < MALI_C55_NUM_CAP_DEVS; j++)
+ mali_c55_set_next_buffer(&mali_c55->cap_devs[j]);
+
+ mali_c55_swap_next_config(mali_c55);
+
+ break;
+ case MALI_C55_IRQ_ISP_DONE:
+ /*
+ * TODO: Where the ISP has no Pong config fitted, we'd
+ * have to do the mali_c55_swap_next_config() call here.
+ */
+ break;
+ case MALI_C55_IRQ_FR_Y_DONE:
+ mali_c55_set_plane_done(
+ &mali_c55->cap_devs[MALI_C55_CAP_DEV_FR],
+ MALI_C55_PLANE_Y);
+ break;
+ case MALI_C55_IRQ_FR_UV_DONE:
+ mali_c55_set_plane_done(
+ &mali_c55->cap_devs[MALI_C55_CAP_DEV_FR],
+ MALI_C55_PLANE_UV);
+ break;
+ case MALI_C55_IRQ_DS_Y_DONE:
+ mali_c55_set_plane_done(
+ &mali_c55->cap_devs[MALI_C55_CAP_DEV_DS],
+ MALI_C55_PLANE_Y);
+ break;
+ case MALI_C55_IRQ_DS_UV_DONE:
+ mali_c55_set_plane_done(
+ &mali_c55->cap_devs[MALI_C55_CAP_DEV_DS],
+ MALI_C55_PLANE_UV);
+ break;
+ default:
+ /*
+ * Only the above interrupts are currently unmasked. If
+ * we receive anything else here then something weird
+ * has gone on.
+ */
+ dev_err(dev, "masked interrupt %s triggered\n",
+ mali_c55_interrupt_names[i]);
+ }
+ }
+
+ return IRQ_HANDLED;
+}
+
+static int mali_c55_init_context(struct mali_c55 *mali_c55)
+{
+ struct mali_c55_ctx *ctx;
+ int ret;
+
+ ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
+ if (!ctx) {
+ dev_err(mali_c55->dev, "failed to allocate new context\n");
No need for an error message when memory allocation fails.
+ return -ENOMEM;
+ }
+
+ ctx->base = mali_c55->res->start;
+ ctx->mali_c55 = mali_c55;
+
+ ctx->registers = kzalloc(MALI_C55_CONFIG_SPACE_SIZE,
+ GFP_KERNEL | GFP_DMA);
+ if (!ctx->registers) {
+ ret = -ENOMEM;
+ goto err_free_ctx;
+ }
+
+ /*
+ * The allocated memory is empty, we need to load the default
+ * register settings. We just read Ping; it's identical to Pong.
+ */
+ ret = mali_c55_config_read(ctx, MALI_C55_CONFIG_PING);
+ if (ret)
+ goto err_free_registers;
+
+ list_add_tail(&ctx->list, &mali_c55->contexts);
+
+ /*
+ * Some features of the ISP need to be disabled by default and only
+ * enabled at the same time as they're configured by a parameters buffer
+ */
+
+ /* Bypass the sqrt and square compression and expansion modules */
+ mali_c55_update_bits(mali_c55, MALI_C55_REG_BYPASS_1,
+ MALI_C55_REG_BYPASS_1_FE_SQRT, 0x01);
+ mali_c55_update_bits(mali_c55, MALI_C55_REG_BYPASS_3,
+ MALI_C55_REG_BYPASS_3_SQUARE_BE, 0x01);
+
+ /* Bypass the temper module */
+ mali_c55_write(mali_c55, MALI_C55_REG_BYPASS_2,
+ MALI_C55_REG_BYPASS_2_TEMPER);
+
+ /* Bypass the colour noise reduction */
+ mali_c55_write(mali_c55, MALI_C55_REG_BYPASS_4,
+ MALI_C55_REG_BYPASS_4_CNR);
+
+ /* Disable the sinter module */
+ mali_c55_update_bits(mali_c55, MALI_C55_REG_SINTER_CONFIG,
+ MALI_C55_SINTER_ENABLE_MASK, 0x00);
+
+ /* Disable the RGB Gamma module for each output */
+ mali_c55_write(
+ mali_c55,
+ MALI_C55_REG_GAMMA_RGB_ENABLE(MALI_C55_CAP_DEV_FR_REG_OFFSET),
+ 0x00);
+ mali_c55_write(
+ mali_c55,
+ MALI_C55_REG_GAMMA_RGB_ENABLE(MALI_C55_CAP_DEV_DS_REG_OFFSET),
+ 0x00);
+
+ /* Disable the colour correction matrix */
+ mali_c55_write(mali_c55, MALI_C55_REG_CCM_ENABLE, 0x00);
+
+ return 0;
+
+err_free_registers:
+ kfree(ctx->registers);
+err_free_ctx:
+ kfree(ctx);
+
+ return ret;
+}
+
+static int mali_c55_runtime_resume(struct device *dev)
+{
+ struct mali_c55 *mali_c55 = dev_get_drvdata(dev);
+ int ret;
+
+ ret = clk_bulk_prepare_enable(ARRAY_SIZE(mali_c55->clks),
+ mali_c55->clks);
+ if (ret)
+ dev_err(mali_c55->dev, "failed to enable clocks\n");
+
+ return ret;
+}
+
+static int mali_c55_runtime_suspend(struct device *dev)
+{
+ struct mali_c55 *mali_c55 = dev_get_drvdata(dev);
+
+ clk_bulk_disable_unprepare(ARRAY_SIZE(mali_c55->clks), mali_c55->clks);
+ return 0;
+}
+
+static const struct dev_pm_ops mali_c55_pm_ops = {
+ SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
+ pm_runtime_force_resume)
+ SET_RUNTIME_PM_OPS(mali_c55_runtime_suspend, mali_c55_runtime_resume,
+ NULL)
+};
+
+static int mali_c55_probe(struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+ struct mali_c55 *mali_c55;
+ dma_cap_mask_t mask;
+ u32 version;
+ int ret;
+ u32 val;
+
+ mali_c55 = devm_kzalloc(dev, sizeof(*mali_c55), GFP_KERNEL);
+ if (!mali_c55)
+ return dev_err_probe(dev, -ENOMEM,
+ "failed to allocate memory\n");
return -ENOMEM;
There's no need to print messages for memory allocation failures.
+
+ mali_c55->dev = dev;
+ platform_set_drvdata(pdev, mali_c55);
+
+ mali_c55->base = devm_platform_get_and_ioremap_resource(pdev, 0,
+ &mali_c55->res);
+ if (IS_ERR(mali_c55->base))
+ return dev_err_probe(dev, PTR_ERR(mali_c55->base),
+ "failed to map IO memory\n");
+
+ ret = platform_get_irq(pdev, 0);
+ if (ret < 0)
+ return dev_err_probe(dev, ret, "failed to get interrupt num\n");
s/ num// or s/num/number/
+
+ ret = devm_request_threaded_irq(&pdev->dev, ret, NULL,
+ mali_c55_isr, IRQF_ONESHOT,
+ dev_driver_string(&pdev->dev),
+ &pdev->dev);
Requested the IRQ should be done much lower, after you have initialized
everything, or an IRQ that would fire early would have really bad
consequences.
A comment to explain why you need a threaded interrupt handler would be
good. I assume it is due only to the need to transfer the registers
using DMA. I wonder if we should then split the interrupt handler in
two, with a non-threaded part for the operations that can run quickly,
and a threaded part for the reprogramming.
It may also be that we could just start the DMA transfer in the
non-threaded handler without waiting synchronously for it to complete.
That would be a bigger change, and would require checking race
conditions carefully. On the other hand, I'm a bit concerned about the
current implementation, have you tested what happens if the DMA transfer
takes too long to complete, and spans frame boundaries ? This part could
be addressed by a patch on top of this one.
+ if (ret)
+ return dev_err_probe(dev, ret, "failed to request irq\n");
+
+ for (unsigned int i = 0; i < ARRAY_SIZE(mali_c55_clk_names); i++)
+ mali_c55->clks[i].id = mali_c55_clk_names[i];
+
+ ret = devm_clk_bulk_get(dev, ARRAY_SIZE(mali_c55->clks), mali_c55->clks);
+ if (ret)
+ return dev_err_probe(dev, ret, "failed to acquire clocks\n");
+
+ pm_runtime_enable(&pdev->dev);
+
+ ret = pm_runtime_resume_and_get(&pdev->dev);
+ if (ret)
+ goto err_pm_runtime_disable;
+
+ of_reserved_mem_device_init(dev);
I'd move this, the vb2_dma_contig_set_max_seg_size() call and the
dma_cap_* calls before pm_runtime_enable() as they don't need the device
to be powered.
+ version = mali_c55_check_hwcfg(mali_c55);
+ vb2_dma_contig_set_max_seg_size(dev, UINT_MAX);
+
+ /* Use "software only" context management. */
+ mali_c55_update_bits(mali_c55, MALI_C55_REG_MCU_CONFIG,
+ MALI_C55_REG_MCU_CONFIG_OVERRIDE_MASK,
+ MALI_C55_REG_MCU_CONFIG_OVERRIDE_MASK);
You handle that in mali_c55_isp_start(), does the register have to be
set here too ?
+
+ dma_cap_zero(mask);
+ dma_cap_set(DMA_MEMCPY, mask);
+
+ /*
+ * No error check, because we will just fallback on memcpy if there is
+ * no usable DMA channel on the system.
+ */
+ mali_c55->channel = dma_request_channel(mask, NULL, NULL);
+
+ INIT_LIST_HEAD(&mali_c55->contexts);
+ ret = mali_c55_init_context(mali_c55);
+ if (ret)
+ goto err_release_dma_channel;
+
I'd move all the code from here ...
+ mali_c55->media_dev.dev = dev;
+ strscpy(mali_c55->media_dev.model, "ARM Mali-C55 ISP",
+ sizeof(mali_c55->media_dev.model));
+ mali_c55->media_dev.hw_revision = version;
+
+ media_device_init(&mali_c55->media_dev);
+ ret = media_device_register(&mali_c55->media_dev);
+ if (ret)
+ goto err_cleanup_media_device;
+
+ mali_c55->v4l2_dev.mdev = &mali_c55->media_dev;
+ ret = v4l2_device_register(dev, &mali_c55->v4l2_dev);
+ if (ret) {
+ dev_err(dev, "failed to register V4L2 device\n");
+ goto err_unregister_media_device;
+ };
+
+ ret = mali_c55_register_entities(mali_c55);
+ if (ret) {
+ dev_err(dev, "failed to register entities\n");
+ goto err_unregister_v4l2_device;
+ }
... to here to a separate function, or maybe fold it all in
mali_c55_register_entities() (which should the be renamed). Same thing
for the cleanup code.
+
+ /* Set safe stop to ensure we're in a non-streaming state */
+ mali_c55_write(mali_c55, MALI_C55_REG_INPUT_MODE_REQUEST,
+ MALI_C55_INPUT_SAFE_STOP);
+ readl_poll_timeout(mali_c55->base + MALI_C55_REG_MODE_STATUS,
+ val, !val, 10 * USEC_PER_MSEC, 250 * USEC_PER_MSEC);
+
+ /*
+ * We're ready to process interrupts. Clear any that are set and then
+ * unmask them for processing.
+ */
+ mali_c55_write(mali_c55, 0x30, 0xffffffff);
+ mali_c55_write(mali_c55, 0x34, 0xffffffff);
+ mali_c55_write(mali_c55, 0x40, 0x01);
+ mali_c55_write(mali_c55, 0x40, 0x00);
Please replace the register addresses with macros.
+ mali_c55_write(mali_c55, MALI_C55_REG_INTERRUPT_MASK_VECTOR, 0x39fc3fc);
The value should use the interrupt bits macros.
+
+ pm_runtime_put(&pdev->dev);
Once power gets cut, the registers your programmed above may be lost. I
think you should programe them in the runtime PM resume handler.
+
+ return 0;
+
+err_unregister_v4l2_device:
+ v4l2_device_unregister(&mali_c55->v4l2_dev);
+err_unregister_media_device:
+ media_device_unregister(&mali_c55->media_dev);
+err_cleanup_media_device:
+ media_device_cleanup(&mali_c55->media_dev);
+err_release_dma_channel:
+ dma_release_channel(mali_c55->channel);
+err_pm_runtime_disable:
+ pm_runtime_disable(&pdev->dev);
+
+ return ret;
+}
+
+static void mali_c55_remove(struct platform_device *pdev)
+{
+ struct mali_c55 *mali_c55 = platform_get_drvdata(pdev);
+ struct mali_c55_ctx *ctx, *tmp;
+
+ list_for_each_entry_safe(ctx, tmp, &mali_c55->contexts, list) {
+ list_del(&ctx->list);
+ kfree(ctx->registers);
+ kfree(ctx);
+ }
+
+ mali_c55_remove_links(mali_c55);
+ mali_c55_unregister_entities(mali_c55);
+ v4l2_device_put(&mali_c55->v4l2_dev);
+ media_device_unregister(&mali_c55->media_dev);
+ media_device_cleanup(&mali_c55->media_dev);
+ dma_release_channel(mali_c55->channel);
+}
+
+static const struct of_device_id mali_c55_of_match[] = {
+ { .compatible = "arm,mali-c55", },
+ {},
{ /* Sentinel */ },
+};
+MODULE_DEVICE_TABLE(of, mali_c55_of_match);
+
+static struct platform_driver mali_c55_driver = {
+ .driver = {
+ .name = "mali-c55",
+ .of_match_table = of_match_ptr(mali_c55_of_match),
Drop of_match_ptr().
+ .pm = &mali_c55_pm_ops,
+ },
+ .probe = mali_c55_probe,
+ .remove_new = mali_c55_remove,
+};
+
+module_platform_driver(mali_c55_driver);
Blank line.
+MODULE_AUTHOR("Daniel Scally <dan.scally@xxxxxxxxxxxxxxxx>");
+MODULE_AUTHOR("Jacopo Mondi <jacopo.mondi@xxxxxxxxxxxxxxxx>");
+MODULE_DESCRIPTION("ARM Mali-C55 ISP platform driver");
+MODULE_LICENSE("GPL");
diff --git a/drivers/media/platform/arm/mali-c55/mali-c55-isp.c
b/drivers/media/platform/arm/mali-c55/mali-c55-isp.c
new file mode 100644
index 000000000000..ea8b7b866e7a
--- /dev/null
+++ b/drivers/media/platform/arm/mali-c55/mali-c55-isp.c
@@ -0,0 +1,611 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * ARM Mali-C55 ISP Driver - Image signal processor
+ *
+ * Copyright (C) 2024 Ideas on Board Oy
+ */
+
+#include <linux/delay.h>
+#include <linux/iopoll.h>
+#include <linux/property.h>
+#include <linux/string.h>
+
+#include <media/media-entity.h>
+#include <media/v4l2-common.h>
+#include <media/v4l2-event.h>
+#include <media/v4l2-mc.h>
+#include <media/v4l2-subdev.h>
+
+#include "mali-c55-common.h"
+#include "mali-c55-registers.h"
+
+static const struct mali_c55_isp_fmt mali_c55_isp_fmts[] = {
+ {
+ .code = MEDIA_BUS_FMT_SRGGB20_1X20,
+ .order = MALI_C55_BAYER_ORDER_RGGB,
+ .encoding = V4L2_PIXEL_ENC_BAYER,
+ },
+ {
+ .code = MEDIA_BUS_FMT_SGRBG20_1X20,
+ .order = MALI_C55_BAYER_ORDER_GRBG,
+ .encoding = V4L2_PIXEL_ENC_BAYER,
+ },
+ {
+ .code = MEDIA_BUS_FMT_SGBRG20_1X20,
+ .order = MALI_C55_BAYER_ORDER_GBRG,
+ .encoding = V4L2_PIXEL_ENC_BAYER,
+ },
+ {
+ .code = MEDIA_BUS_FMT_SBGGR20_1X20,
+ .order = MALI_C55_BAYER_ORDER_BGGR,
+ .encoding = V4L2_PIXEL_ENC_BAYER,
+ },
+ {
+ .code = MEDIA_BUS_FMT_RGB202020_1X60,
+ .order = 0, /* Not relevant for this format */
+ .encoding = V4L2_PIXEL_ENC_RGB,
+ }
+ /*
+ * TODO: Support MEDIA_BUS_FMT_YUV20_1X60 here. This is so that we can
+ * also support YUV input from a sensor passed-through to the output. At
+ * present we have no mechanism to test that though so it may have to
+ * wait a while...
+ */
+};
+
+const struct mali_c55_isp_fmt *
+mali_c55_isp_fmt_next(const struct mali_c55_isp_fmt *fmt)
+{
+ if (!fmt)
+ fmt = &mali_c55_isp_fmts[0];
+ else
+ fmt++;
+
+ for (; fmt < &mali_c55_isp_fmts[ARRAY_SIZE(mali_c55_isp_fmts)]; fmt++)
+ return fmt;
That's peculiar.
if (!fmt)
fmt = &mali_c55_isp_fmts[0];
else if (fmt < &mali_c55_isp_fmts[ARRAY_SIZE(mali_c55_isp_fmts)])
return ++fmt;
else
return NULL;
+
+ return NULL;
+}
+
+bool mali_c55_isp_is_format_supported(unsigned int mbus_code)
+{
+ const struct mali_c55_isp_fmt *isp_fmt;
+
+ for_each_mali_isp_fmt(isp_fmt) {
I would open-code the loop instead of using the macro, like you do
below. It will be more efficient.
+ if (isp_fmt->code == mbus_code)
+ return true;
+ }
+
+ return false;
+}
+
+static const struct mali_c55_isp_fmt *
+mali_c55_isp_get_mbus_config_by_code(u32 code)
+{
+ unsigned int i;
+
+ for (i = 0; i < ARRAY_SIZE(mali_c55_isp_fmts); i++) {
+ if (mali_c55_isp_fmts[i].code == code)
+ return &mali_c55_isp_fmts[i];
+ }
+
+ return NULL;
+}
+
+static void mali_c55_isp_stop(struct mali_c55 *mali_c55)
+{
+ u32 val;
+
+ mali_c55_write(mali_c55, MALI_C55_REG_INPUT_MODE_REQUEST, MALI_C55_INPUT_SAFE_STOP);
mali_c55_write(mali_c55, MALI_C55_REG_INPUT_MODE_REQUEST,
MALI_C55_INPUT_SAFE_STOP);
+ readl_poll_timeout(mali_c55->base + MALI_C55_REG_MODE_STATUS,
+ val, !val, 10 * USEC_PER_MSEC, 250 * USEC_PER_MSEC);
+}
+
+static int mali_c55_isp_start(struct mali_c55 *mali_c55)
+{
+ struct mali_c55_ctx *ctx = mali_c55_get_active_context(mali_c55);
+ const struct mali_c55_isp_fmt *cfg;
+ struct v4l2_mbus_framefmt *format;
const
+ struct v4l2_subdev_state *state;
+ struct v4l2_rect *crop;
const
+ struct v4l2_subdev *sd;
+ u32 val;
+ int ret;
+
+ sd = &mali_c55->isp.sd;
Assign when declaring the variable.
+
+ mali_c55_update_bits(mali_c55, MALI_C55_REG_MCU_CONFIG,
+ MALI_C55_REG_MCU_CONFIG_WRITE_MASK, 0x01);
+
+ /* Apply input windowing */
+ state = v4l2_subdev_get_locked_active_state(sd);
Using .enable_streams() (see below) you'll get this for free.
+ crop = v4l2_subdev_state_get_crop(state, MALI_C55_ISP_PAD_SINK_VIDEO);
+ format = v4l2_subdev_state_get_format(state,
+ MALI_C55_ISP_PAD_SINK_VIDEO);
+ cfg = mali_c55_isp_get_mbus_config_by_code(format->code);
+
+ mali_c55_write(mali_c55, MALI_C55_REG_HC_START,
+ MALI_C55_HC_START(crop->left));
+ mali_c55_write(mali_c55, MALI_C55_REG_HC_SIZE,
+ MALI_C55_HC_SIZE(crop->width));
+ mali_c55_write(mali_c55, MALI_C55_REG_VC_START_SIZE,
+ MALI_C55_VC_START(crop->top) |
+ MALI_C55_VC_SIZE(crop->height));
+ mali_c55_update_bits(mali_c55, MALI_C55_REG_BASE_ADDR,
+ MALI_C55_REG_ACTIVE_WIDTH_MASK, format->width);
+ mali_c55_update_bits(mali_c55, MALI_C55_REG_BASE_ADDR,
+ MALI_C55_REG_ACTIVE_HEIGHT_MASK, format->height);
+ mali_c55_update_bits(mali_c55, MALI_C55_REG_BAYER_ORDER,
+ MALI_C55_BAYER_ORDER_MASK, cfg->order);
+ mali_c55_update_bits(mali_c55, MALI_C55_REG_INPUT_WIDTH,
+ MALI_C55_INPUT_WIDTH_MASK,
+ MALI_C55_INPUT_WIDTH_20BIT);
+
+ mali_c55_update_bits(mali_c55, MALI_C55_REG_ISP_RAW_BYPASS,
+ MALI_C55_ISP_RAW_BYPASS_BYPASS_MASK,
+ cfg->encoding == V4L2_PIXEL_ENC_RGB ?
+ MALI_C55_ISP_RAW_BYPASS_BYPASS_MASK : 0x00);
+
+ ret = mali_c55_config_write(ctx, MALI_C55_CONFIG_PING);
+ if (ret) {
+ dev_err(mali_c55->dev, "failed to DMA config\n");
+ return ret;
+ }
+
+ mali_c55_write(mali_c55, MALI_C55_REG_INPUT_MODE_REQUEST,
+ MALI_C55_INPUT_SAFE_START);
+ readl_poll_timeout(mali_c55->base + MALI_C55_REG_MODE_STATUS,
+ val, !val, 10 * USEC_PER_MSEC, 250 * USEC_PER_MSEC);
Should you return an error in case of timeout ?
+
+ return 0;
+}
+
+void mali_c55_isp_stop_stream(struct mali_c55_isp *isp)
Why is this not handled wired to .s_stream() ? Or better,
.enable_streams() and .disable_streams().
+{
+ struct mali_c55 *mali_c55 = isp->mali_c55;
+ struct v4l2_subdev *sd;
+
+ if (isp->remote_src) {
+ sd = media_entity_to_v4l2_subdev(isp->remote_src->entity);
+ v4l2_subdev_disable_streams(sd, isp->remote_src->index, BIT(0));
+ }
+ isp->remote_src = NULL;
+
+ mali_c55_isp_stop(mali_c55);
+}
+
+int mali_c55_isp_start_stream(struct mali_c55_isp *isp)
+{
+ struct mali_c55 *mali_c55 = isp->mali_c55;
+ struct media_pad *sink_pad;
+ struct v4l2_subdev *sd;
+ int ret;
+
+ sink_pad = &isp->pads[MALI_C55_ISP_PAD_SINK_VIDEO];
+ isp->remote_src = media_pad_remote_pad_unique(sink_pad);
+ if (IS_ERR(isp->remote_src)) {
If you set the MUST_CONNECT flag for the MALI_C55_ISP_PAD_SINK_VIDEO pad
I think you can drop this check.
+ dev_err(mali_c55->dev, "Failed to get source for ISP\n");
+ return PTR_ERR(isp->remote_src);
+ }
+
+ sd = media_entity_to_v4l2_subdev(isp->remote_src->entity);
+
+ isp->frame_sequence = 0;
+ ret = mali_c55_isp_start(mali_c55);
+ if (ret) {
+ dev_err(mali_c55->dev, "Failed to start ISP\n");
+ isp->remote_src = NULL;
+ return ret;
+ }
+
+ /*
+ * We only support a single input stream, so we can just enable the 1st
+ * entry in the streams mask.
+ */
+ ret = v4l2_subdev_enable_streams(sd, isp->remote_src->index, BIT(0));
+ if (ret) {
+ dev_err(mali_c55->dev, "Failed to start ISP source\n");
+ mali_c55_isp_stop(mali_c55);
+ return ret;
+ }
+
+ return 0;
+}
+
+static int mali_c55_isp_enum_mbus_code(struct v4l2_subdev *sd,
+ struct v4l2_subdev_state *state,
+ struct v4l2_subdev_mbus_code_enum *code)
+{
+ /*
+ * Only the internal RGB processed format is allowed on the regular
+ * processing source pad.
+ */
+ if (code->pad == MALI_C55_ISP_PAD_SOURCE) {
+ if (code->index)
+ return -EINVAL;
+
+ code->code = MEDIA_BUS_FMT_RGB121212_1X36;
+ return 0;
+ }
+
+ /* On the sink and bypass pads all the supported formats are allowed. */
+ if (code->index >= ARRAY_SIZE(mali_c55_isp_fmts))
+ return -EINVAL;
+
+ code->code = mali_c55_isp_fmts[code->index].code;
+
+ return 0;
+}
+
+static int mali_c55_isp_enum_frame_size(struct v4l2_subdev *sd,
+ struct v4l2_subdev_state *state,
+ struct v4l2_subdev_frame_size_enum *fse)
+{
+ const struct mali_c55_isp_fmt *cfg;
+
+ if (fse->index > 0)
+ return -EINVAL;
+
+ /*
+ * Only the internal RGB processed format is allowed on the regular
+ * processing source pad.
+ *
+ * On the sink and bypass pads all the supported formats are allowed.
+ */
+ if (fse->pad == MALI_C55_ISP_PAD_SOURCE) {
+ if (fse->code != MEDIA_BUS_FMT_RGB121212_1X36)
+ return -EINVAL;
+ } else {
+ cfg = mali_c55_isp_get_mbus_config_by_code(fse->code);
+ if (!cfg)
+ return -EINVAL;
+ }
+
+ fse->min_width = MALI_C55_MIN_WIDTH;
+ fse->min_height = MALI_C55_MIN_HEIGHT;
+ fse->max_width = MALI_C55_MAX_WIDTH;
+ fse->max_height = MALI_C55_MAX_HEIGHT;
+
+ return 0;
+}
+
+static int mali_c55_isp_set_fmt(struct v4l2_subdev *sd,
+ struct v4l2_subdev_state *state,
+ struct v4l2_subdev_format *format)
+{
+ struct v4l2_mbus_framefmt *fmt = &format->format;
+ struct v4l2_mbus_framefmt *src_fmt, *sink_fmt;
+ const struct mali_c55_isp_fmt *cfg;
+ struct v4l2_rect *crop;
+
+ /*
+ * Disallow set_fmt on the source pads; format is fixed and the sizes
+ * are the result of applying the sink crop rectangle to the sink
+ * format.
+ */
+ if (format->pad)
if (format->pad != MALI_C55_ISP_PAD_SINK_VIDEO)
+ return v4l2_subdev_get_fmt(sd, state, format);
+
+ cfg = mali_c55_isp_get_mbus_config_by_code(fmt->code);
+ if (!cfg)
+ fmt->code = MEDIA_BUS_FMT_SRGGB20_1X20;
+ fmt->field = V4L2_FIELD_NONE;
Do you intentionally allow the colorspace fields to be overwritten to
any value ? rkisp1_isp_set_src_fmt() and rkisp1_isp_set_sink_fmt() may
show you how this could be handled.
+
+ /*
+ * Clamp sizes in the accepted limits and clamp the crop rectangle in
+ * the new sizes.
+ */
+ clamp_t(unsigned int, fmt->width,
+ MALI_C55_MIN_WIDTH, MALI_C55_MAX_WIDTH);
+ clamp_t(unsigned int, fmt->width,
+ MALI_C55_MIN_HEIGHT, MALI_C55_MAX_HEIGHT);
clamp_t() returns a value, which you ignore. Those are no-ops. You meant
fmt->width = clamp_t(unsigned int, fmt->width, MALI_C55_MIN_WIDTH,
MALI_C55_MAX_WIDTH);
fmt->height = clamp_t(unsigned int, fmt->height, MALI_C55_MIN_HEIGHT,
MALI_C55_MAX_HEIGHT);
Same for every use of clamp_t() through the whole driver.
Also, do you need clamp_t() ? I think all values are unsigned int, you
can use clamp().
Are there any alignment constraints, such a multiples of two for bayer
formats ? Same in all the other locations where applicable.
+
+ sink_fmt = v4l2_subdev_state_get_format(state,
+ MALI_C55_ISP_PAD_SINK_VIDEO);
+ *sink_fmt = *fmt;
+
+ crop = v4l2_subdev_state_get_crop(state, MALI_C55_ISP_PAD_SINK_VIDEO);
+ crop->left = 0;
+ crop->top = 0;
+ crop->width = fmt->width;
+ crop->height = fmt->height;
+
+ /*
+ * Propagate format to source pads. On the 'regular' output pad use
+ * the internal RGB processed format, while on the bypass pad simply
+ * replicate the ISP sink format. The sizes on both pads are the same as
+ * the ISP sink crop rectangle.
+ */
Colorspace information will need to be propagated too.
+ src_fmt = v4l2_subdev_state_get_format(state, MALI_C55_ISP_PAD_SOURCE);
+ src_fmt->code = MEDIA_BUS_FMT_RGB121212_1X36;
+ src_fmt->width = crop->width;
+ src_fmt->height = crop->height;
+
+ src_fmt = v4l2_subdev_state_get_format(state,
+ MALI_C55_ISP_PAD_SOURCE_BYPASS);
+ src_fmt->code = fmt->code;
+ src_fmt->width = crop->width;
+ src_fmt->height = crop->height;
+
+ return 0;
+}
+
+static int mali_c55_isp_get_selection(struct v4l2_subdev *sd,
+ struct v4l2_subdev_state *state,
+ struct v4l2_subdev_selection *sel)
+{
+ if (sel->pad || sel->target != V4L2_SEL_TGT_CROP)
sel->pad != MALI_C55_ISP_PAD_SINK_VIDEO
+ return -EINVAL;
+
+ sel->r = *v4l2_subdev_state_get_crop(state, MALI_C55_ISP_PAD_SINK_VIDEO);
+
+ return 0;
+}
+
+static int mali_c55_isp_set_selection(struct v4l2_subdev *sd,
+ struct v4l2_subdev_state *state,
+ struct v4l2_subdev_selection *sel)
+{
+ struct v4l2_mbus_framefmt *src_fmt;
+ struct v4l2_mbus_framefmt *fmt;
const
+ struct v4l2_rect *crop;
+
+ if (sel->pad || sel->target != V4L2_SEL_TGT_CROP)
Ditto.
+ return -EINVAL;
+
+ fmt = v4l2_subdev_state_get_format(state, MALI_C55_ISP_PAD_SINK_VIDEO);
+
+ clamp_t(unsigned int, sel->r.left, 0, fmt->width);
+ clamp_t(unsigned int, sel->r.top, 0, fmt->height);
+ clamp_t(unsigned int, sel->r.width, MALI_C55_MIN_WIDTH,
+ fmt->width - sel->r.left);
+ clamp_t(unsigned int, sel->r.height, MALI_C55_MIN_HEIGHT,
+ fmt->height - sel->r.top);
+
+ crop = v4l2_subdev_state_get_crop(state, MALI_C55_ISP_PAD_SINK_VIDEO);
+ *crop = sel->r;
+
+ /* Propagate the crop rectangle sizes to the source pad format. */
+ src_fmt = v4l2_subdev_state_get_format(state, MALI_C55_ISP_PAD_SOURCE);
+ src_fmt->width = crop->width;
+ src_fmt->height = crop->height;
Can you confirm that cropping doesn't affect the bypass path ? And maybe
add a comment to mention it.
+
+ return 0;
+}
+
+static const struct v4l2_subdev_pad_ops mali_c55_isp_pad_ops = {
+ .enum_mbus_code = mali_c55_isp_enum_mbus_code,
+ .enum_frame_size = mali_c55_isp_enum_frame_size,
+ .get_fmt = v4l2_subdev_get_fmt,
+ .set_fmt = mali_c55_isp_set_fmt,
+ .get_selection = mali_c55_isp_get_selection,
+ .set_selection = mali_c55_isp_set_selection,
+ .link_validate = v4l2_subdev_link_validate_default,
+};
+
+void mali_c55_isp_queue_event_sof(struct mali_c55 *mali_c55)
+{
+ struct v4l2_event event = {
+ .type = V4L2_EVENT_FRAME_SYNC,
+ };
+
+ event.u.frame_sync.frame_sequence = mali_c55->isp.frame_sequence;
+ v4l2_event_queue(mali_c55->isp.sd.devnode, &event);
+}
+
+static int
+mali_c55_isp_subscribe_event(struct v4l2_subdev *sd, struct v4l2_fh *fh,
+ struct v4l2_event_subscription *sub)
+{
+ if (sub->type != V4L2_EVENT_FRAME_SYNC)
+ return -EINVAL;
+
+ /* V4L2_EVENT_FRAME_SYNC doesn't require an id, so zero should be set */
+ if (sub->id != 0)
+ return -EINVAL;
+
+ return v4l2_event_subscribe(fh, sub, 0, NULL);
+}
+
+static const struct v4l2_subdev_core_ops mali_c55_isp_core_ops = {
+ .subscribe_event = mali_c55_isp_subscribe_event,
+ .unsubscribe_event = v4l2_event_subdev_unsubscribe,
+};
+
+static const struct v4l2_subdev_ops mali_c55_isp_ops = {
+ .pad = &mali_c55_isp_pad_ops,
+ .core = &mali_c55_isp_core_ops,
+};
+
+static int mali_c55_isp_init_state(struct v4l2_subdev *sd,
+ struct v4l2_subdev_state *sd_state)
You name this variable state in every other subdev operation handler.
+{
+ struct v4l2_mbus_framefmt *sink_fmt, *src_fmt;
+ struct v4l2_rect *in_crop;
+
+ sink_fmt = v4l2_subdev_state_get_format(sd_state,
+ MALI_C55_ISP_PAD_SINK_VIDEO);
+ src_fmt = v4l2_subdev_state_get_format(sd_state,
+ MALI_C55_ISP_PAD_SOURCE);
+ in_crop = v4l2_subdev_state_get_crop(sd_state,
+ MALI_C55_ISP_PAD_SINK_VIDEO);
+
+ sink_fmt->width = MALI_C55_DEFAULT_WIDTH;
+ sink_fmt->height = MALI_C55_DEFAULT_HEIGHT;
+ sink_fmt->field = V4L2_FIELD_NONE;
+ sink_fmt->code = MEDIA_BUS_FMT_SRGGB20_1X20;
You should initialize the colorspace fields too. Same below.
+
+ *v4l2_subdev_state_get_format(sd_state,
+ MALI_C55_ISP_PAD_SOURCE_BYPASS) = *sink_fmt;
+
+ src_fmt->width = MALI_C55_DEFAULT_WIDTH;
+ src_fmt->height = MALI_C55_DEFAULT_HEIGHT;
+ src_fmt->field = V4L2_FIELD_NONE;
+ src_fmt->code = MEDIA_BUS_FMT_RGB121212_1X36;
+
+ in_crop->top = 0;
+ in_crop->left = 0;
+ in_crop->width = MALI_C55_DEFAULT_WIDTH;
+ in_crop->height = MALI_C55_DEFAULT_HEIGHT;
+
+ return 0;
+}
+
+static const struct v4l2_subdev_internal_ops mali_c55_isp_internal_ops = {
+ .init_state = mali_c55_isp_init_state,
+};
+
+static const struct media_entity_operations mali_c55_isp_media_ops = {
+ .link_validate = v4l2_subdev_link_validate,
.link_validate = v4l2_subdev_link_validate,
to match mali_c55_isp_internal_ops.
+};
+
+static int mali_c55_isp_notifier_bound(struct v4l2_async_notifier *notifier,
+ struct v4l2_subdev *subdev,
+ struct v4l2_async_connection *asc)
+{
+ struct mali_c55_isp *isp = container_of(notifier,
+ struct mali_c55_isp, notifier);
+ struct mali_c55 *mali_c55 = isp->mali_c55;
+ struct media_pad *pad = &isp->pads[MALI_C55_ISP_PAD_SINK_VIDEO];
+ int ret;
+
+ /*
+ * By default we'll flag this link enabled and the TPG disabled, but
+ * no immutable flag because we need to be able to switch between the
+ * two.
+ */
+ ret = v4l2_create_fwnode_links_to_pad(subdev, pad,
+ MEDIA_LNK_FL_ENABLED);
+ if (ret)
+ dev_err(mali_c55->dev, "failed to create link for %s\n",
+ subdev->name);
+
+ return ret;
+}
+
+static int mali_c55_isp_notifier_complete(struct v4l2_async_notifier *notifier)
+{
+ struct mali_c55_isp *isp = container_of(notifier,
+ struct mali_c55_isp, notifier);
+ struct mali_c55 *mali_c55 = isp->mali_c55;
+
+ return v4l2_device_register_subdev_nodes(&mali_c55->v4l2_dev);
+}
+
+static const struct v4l2_async_notifier_operations mali_c55_isp_notifier_ops = {
+ .bound = mali_c55_isp_notifier_bound,
+ .complete = mali_c55_isp_notifier_complete,
+};
+
+static int mali_c55_isp_parse_endpoint(struct mali_c55_isp *isp)
+{
+ struct mali_c55 *mali_c55 = isp->mali_c55;
+ struct v4l2_async_connection *asc;
+ struct fwnode_handle *ep;
+ int ret;
+
+ v4l2_async_nf_init(&isp->notifier, &mali_c55->v4l2_dev);
+
+ /*
+ * The ISP should have a single endpoint pointing to some flavour of
+ * CSI-2 receiver...but for now at least we do want everything to work
+ * normally even with no sensors connected, as we have the TPG. If we
+ * don't find a sensor just warn and return success.
+ */
+ ep = fwnode_graph_get_endpoint_by_id(dev_fwnode(mali_c55->dev),
+ 0, 0, 0);
+ if (!ep) {
+ dev_warn(mali_c55->dev, "no local endpoint found\n");
+ return 0;
+ }
+
+ asc = v4l2_async_nf_add_fwnode_remote(&isp->notifier, ep,
+ struct v4l2_async_connection);
+ if (IS_ERR(asc)) {
+ dev_err(mali_c55->dev, "failed to add remote fwnode\n");
+ ret = PTR_ERR(asc);
+ goto err_put_ep;
+ }
+
+ isp->notifier.ops = &mali_c55_isp_notifier_ops;
+ ret = v4l2_async_nf_register(&isp->notifier);
+ if (ret) {
+ dev_err(mali_c55->dev, "failed to register notifier\n");
+ goto err_cleanup_nf;
+ }
+
+ fwnode_handle_put(ep);
+
+ return 0;
+
+err_cleanup_nf:
+ v4l2_async_nf_cleanup(&isp->notifier);
+err_put_ep:
+ fwnode_handle_put(ep);
+
+ return ret;
+}
+
+int mali_c55_register_isp(struct mali_c55 *mali_c55)
+{
+ struct mali_c55_isp *isp = &mali_c55->isp;
+ struct v4l2_subdev *sd = &isp->sd;
+ int ret;
+
+ isp->mali_c55 = mali_c55;
+
+ v4l2_subdev_init(sd, &mali_c55_isp_ops);
+ sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE | V4L2_SUBDEV_FL_HAS_EVENTS;
+ sd->entity.ops = &mali_c55_isp_media_ops;
+ sd->entity.function = MEDIA_ENT_F_PROC_VIDEO_ISP;
+ sd->internal_ops = &mali_c55_isp_internal_ops;
+ strscpy(sd->name, MALI_C55_DRIVER_NAME " isp", sizeof(sd->name));
+
+ isp->pads[MALI_C55_ISP_PAD_SINK_VIDEO].flags = MEDIA_PAD_FL_SINK;
The MUST_CONNECT flag would make sense here.
+ isp->pads[MALI_C55_ISP_PAD_SOURCE].flags = MEDIA_PAD_FL_SOURCE;
+ isp->pads[MALI_C55_ISP_PAD_SOURCE_BYPASS].flags = MEDIA_PAD_FL_SOURCE;
+
+ ret = media_entity_pads_init(&sd->entity, MALI_C55_ISP_NUM_PADS,
+ isp->pads);
+ if (ret)
+ return ret;
+
+ ret = v4l2_subdev_init_finalize(sd);
+ if (ret)
+ goto err_cleanup_media_entity;
+
+ ret = v4l2_device_register_subdev(&mali_c55->v4l2_dev, sd);
+ if (ret)
+ goto err_cleanup_subdev;
+
+ ret = mali_c55_isp_parse_endpoint(isp);
+ if (ret)
+ goto err_cleanup_subdev;
As noted elsewhere, I think this belongs to mali-c55-core.c.
+
+ mutex_init(&isp->lock);
This lock is used in mali-c55-capture.c only, that seems weird.
+
+ return 0;
+
+err_cleanup_subdev:
+ v4l2_subdev_cleanup(sd);
+err_cleanup_media_entity:
+ media_entity_cleanup(&sd->entity);
+ isp->mali_c55 = NULL;
+
+ return ret;
+}
+
+void mali_c55_unregister_isp(struct mali_c55 *mali_c55)
+{
+ struct mali_c55_isp *isp = &mali_c55->isp;
+
+ if (!isp->mali_c55)
+ return;
+
+ mutex_destroy(&isp->lock);
+ v4l2_async_nf_unregister(&isp->notifier);
+ v4l2_async_nf_cleanup(&isp->notifier);
+ v4l2_device_unregister_subdev(&isp->sd);
+ v4l2_subdev_cleanup(&isp->sd);
+ media_entity_cleanup(&isp->sd.entity);
+}
diff --git a/drivers/media/platform/arm/mali-c55/mali-c55-registers.h
b/drivers/media/platform/arm/mali-c55/mali-c55-registers.h
new file mode 100644
index 000000000000..cb27abde2aa5
--- /dev/null
+++ b/drivers/media/platform/arm/mali-c55/mali-c55-registers.h
@@ -0,0 +1,258 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * ARM Mali-C55 ISP Driver - Register definitions
+ *
+ * Copyright (C) 2024 Ideas on Board Oy
+ */
+
+#ifndef _MALI_C55_REGISTERS_H
+#define _MALI_C55_REGISTERS_H
+
+#include <linux/bits.h>
+
+/* ISP Common 0x00000 - 0x000ff */
+
+#define MALI_C55_REG_API 0x00000
+#define MALI_C55_REG_PRODUCT 0x00004
+#define MALI_C55_REG_VERSION 0x00008
+#define MALI_C55_REG_REVISION 0x0000c
+#define MALI_C55_REG_PULSE_MODE 0x0003c
+#define MALI_C55_REG_INPUT_MODE_REQUEST 0x0009c
+#define MALI_C55_INPUT_SAFE_STOP 0x00
+#define MALI_C55_INPUT_SAFE_START 0x01
+#define MALI_C55_REG_MODE_STATUS 0x000a0
+#define MALI_C55_REG_INTERRUPT_MASK_VECTOR 0x00030
+#define MALI_C55_INTERRUPT_MASK_ALL GENMASK(31, 0)
+
+#define MALI_C55_REG_GLOBAL_MONITOR 0x00050
+
+#define MALI_C55_REG_GEN_VIDEO 0x00080
+#define MALI_C55_REG_GEN_VIDEO_ON_MASK BIT(0)
+#define MALI_C55_REG_GEN_VIDEO_MULTI_MASK BIT(1)
+#define MALI_C55_REG_GEN_PREFETCH_MASK GENMASK(31, 16)
+
+#define MALI_C55_REG_MCU_CONFIG 0x00020
+#define MALI_C55_REG_MCU_CONFIG_OVERRIDE_MASK BIT(0)
#define MALI_C55_REG_MCU_CONFIG_OVERRIDE BIT(0)
Same in other places where applicable.
+#define MALI_C55_REG_MCU_CONFIG_WRITE_MASK BIT(1)
+#define MALI_C55_REG_MCU_CONFIG_WRITE_PING BIT(1)
+#define MALI_C55_REG_MCU_CONFIG_WRITE_PONG 0x00
+#define MALI_C55_REG_MULTI_CONTEXT_MODE_MASK BIT(8)
+#define MALI_C55_REG_PING_PONG_READ 0x00024
+#define MALI_C55_REG_PING_PONG_READ_MASK BIT(2)
+
+#define MALI_C55_REG_INTERRUPT_CLEAR_VECTOR 0x00034
+#define MALI_C55_REG_INTERRUPT_CLEAR 0x00040
+#define MALI_C55_REG_INTERRUPT_STATUS_VECTOR 0x00044
+#define MALI_C55_REG_GLOBAL_PARAMETER_STATUS 0x00068
+#define MALI_C55_GPS_PONG_FITTED BIT(0)
+#define MALI_C55_GPS_WDR_FITTED BIT(1)
+#define MALI_C55_GPS_COMPRESSION_FITTED BIT(2)
+#define MALI_C55_GPS_TEMPER_FITTED BIT(3)
+#define MALI_C55_GPS_SINTER_LITE_FITTED BIT(4)
+#define MALI_C55_GPS_SINTER_FITTED BIT(5)
+#define MALI_C55_GPS_IRIDIX_LTM_FITTED BIT(6)
+#define MALI_C55_GPS_IRIDIX_GTM_FITTED BIT(7)
+#define MALI_C55_GPS_CNR_FITTED BIT(8)
+#define MALI_C55_GPS_FRSCALER_FITTED BIT(9)
+#define MALI_C55_GPS_DS_PIPE_FITTED BIT(10)
+
+#define MALI_C55_REG_BLANKING 0x00084
+#define MALI_C55_REG_HBLANK_MASK GENMASK(15, 0)
+#define MALI_C55_REG_VBLANK_MASK GENMASK(31, 16)
+
+#define MALI_C55_REG_HC_START 0x00088
+#define MALI_C55_HC_START(h) (((h) & 0xffff) << 16)
+#define MALI_C55_REG_HC_SIZE 0x0008c
+#define MALI_C55_HC_SIZE(h) ((h) & 0xffff)
+#define MALI_C55_REG_VC_START_SIZE 0x00094
+#define MALI_C55_VC_START(v) ((v) & 0xffff)
+#define MALI_C55_VC_SIZE(v) (((v) & 0xffff) << 16)
+
+/* Ping/Pong Configuration Space */
+#define MALI_C55_REG_BASE_ADDR 0x18e88
+#define MALI_C55_REG_BYPASS_0 0x18eac
+#define MALI_C55_REG_BYPASS_0_VIDEO_TEST BIT(0)
+#define MALI_C55_REG_BYPASS_0_INPUT_FMT BIT(1)
+#define MALI_C55_REG_BYPASS_0_DECOMPANDER BIT(2)
+#define MALI_C55_REG_BYPASS_0_SENSOR_OFFSET_WDR BIT(3)
+#define MALI_C55_REG_BYPASS_0_GAIN_WDR BIT(4)
+#define MALI_C55_REG_BYPASS_0_FRAME_STITCH BIT(5)
+#define MALI_C55_REG_BYPASS_1 0x18eb0
+#define MALI_C55_REG_BYPASS_1_DIGI_GAIN BIT(0)
+#define MALI_C55_REG_BYPASS_1_FE_SENSOR_OFFS BIT(1)
+#define MALI_C55_REG_BYPASS_1_FE_SQRT BIT(2)
+#define MALI_C55_REG_BYPASS_1_RAW_FE BIT(3)
+#define MALI_C55_REG_BYPASS_2 0x18eb8
+#define MALI_C55_REG_BYPASS_2_SINTER BIT(0)
+#define MALI_C55_REG_BYPASS_2_TEMPER BIT(1)
+#define MALI_C55_REG_BYPASS_3 0x18ebc
+#define MALI_C55_REG_BYPASS_3_SQUARE_BE BIT(0)
+#define MALI_C55_REG_BYPASS_3_SENSOR_OFFSET_PRE_SH BIT(1)
+#define MALI_C55_REG_BYPASS_3_MESH_SHADING BIT(3)
+#define MALI_C55_REG_BYPASS_3_WHITE_BALANCE BIT(4)
+#define MALI_C55_REG_BYPASS_3_IRIDIX BIT(5)
+#define MALI_C55_REG_BYPASS_3_IRIDIX_GAIN BIT(6)
+#define MALI_C55_REG_BYPASS_4 0x18ec0
+#define MALI_C55_REG_BYPASS_4_DEMOSAIC_RGB BIT(1)
+#define MALI_C55_REG_BYPASS_4_PF_CORRECTION BIT(3)
+#define MALI_C55_REG_BYPASS_4_CCM BIT(4)
+#define MALI_C55_REG_BYPASS_4_CNR BIT(5)
+#define MALI_C55_REG_FR_BYPASS 0x18ec4
+#define MALI_C55_REG_DS_BYPASS 0x18ec8
+#define MALI_C55_BYPASS_CROP BIT(0)
+#define MALI_C55_BYPASS_SCALER BIT(1)
+#define MALI_C55_BYPASS_GAMMA_RGB BIT(2)
+#define MALI_C55_BYPASS_SHARPEN BIT(3)
+#define MALI_C55_BYPASS_CS_CONV BIT(4)
+#define MALI_C55_REG_ISP_RAW_BYPASS 0x18ecc
+#define MALI_C55_ISP_RAW_BYPASS_BYPASS_MASK BIT(0)
+#define MALI_C55_ISP_RAW_BYPASS_FR_BYPASS_MASK GENMASK(9, 8)
+#define MALI_C55_ISP_RAW_BYPASS_RAW_FR_BYPASS 2
+#define MALI_C55_ISP_RAW_BYPASS_RGB_FR_BYPASS 1
+#define MALI_C55_ISP_RAW_BYPASS_DS_PIPE_DISABLE BIT(1)
+#define MALI_C55_ISP_RAW_BYPASS_RAW_BYPASS BIT(0)
+
+#define MALI_C55_REG_ACTIVE_WIDTH_MASK 0xffff
+#define MALI_C55_REG_ACTIVE_HEIGHT_MASK 0xffff0000
+#define MALI_C55_REG_BAYER_ORDER 0x18e8c
+#define MALI_C55_BAYER_ORDER_MASK GENMASK(1, 0)
+#define MALI_C55_REG_TPG_CH0 0x18ed8
+#define MALI_C55_TEST_PATTERN_ON_OFF BIT(0)
+#define MALI_C55_TEST_PATTERN_RGB_MASK BIT(1)
+#define MALI_C55_REG_TPG_R_BACKGROUND 0x18ee0
+#define MALI_C55_REG_TPG_G_BACKGROUND 0x18ee4
+#define MALI_C55_REG_TPG_B_BACKGROUND 0x18ee8
+#define MALI_C55_TPG_BACKGROUND_MAX 0xfffff
+#define MALI_C55_REG_INPUT_WIDTH 0x18f98
+#define MALI_C55_INPUT_WIDTH_MASK GENMASK(18, 16)
+#define MALI_C55_INPUT_WIDTH_8BIT 0
+#define MALI_C55_INPUT_WIDTH_10BIT 1
+#define MALI_C55_INPUT_WIDTH_12BIT 2
+#define MALI_C55_INPUT_WIDTH_14BIT 3
+#define MALI_C55_INPUT_WIDTH_16BIT 4
+#define MALI_C55_INPUT_WIDTH_20BIT 5
+#define MALI_C55_REG_SPACE_SIZE 0x4000
+#define MALI_C55_REG_CONFIG_SPACES_OFFSET 0x0ab6c
+#define MALI_C55_CONFIG_SPACE_SIZE 0x1231c
+
+#define MALI_C55_REG_SINTER_CONFIG 0x19348
+#define MALI_C55_SINTER_VIEW_FILTER_MASK GENMASK(1, 0)
+#define MALI_C55_SINTER_SCALE_MODE_MASK GENMASK(3, 2)
+#define MALI_C55_SINTER_ENABLE_MASK BIT(4)
+#define MALI_C55_SINTER_FILTER_SELECT_MASK BIT(5)
+#define MALI_C55_SINTER_INT_SELECT_MASK BIT(6)
+#define MALI_C55_SINTER_RM_ENABLE_MASK BIT(7)
+
+/* Colour Correction Matrix Configuration */
+#define MALI_C55_REG_CCM_ENABLE 0x1b07c
+#define MALI_C55_CCM_ENABLE_MASK BIT(0)
+#define MALI_C55_REG_CCM_COEF_R_R 0x1b080
+#define MALI_C55_REG_CCM_COEF_R_G 0x1b084
+#define MALI_C55_REG_CCM_COEF_R_B 0x1b088
+#define MALI_C55_REG_CCM_COEF_G_R 0x1b090
+#define MALI_C55_REG_CCM_COEF_G_G 0x1b094
+#define MALI_C55_REG_CCM_COEF_G_B 0x1b098
+#define MALI_C55_REG_CCM_COEF_B_R 0x1b0a0
+#define MALI_C55_REG_CCM_COEF_B_G 0x1b0a4
+#define MALI_C55_REG_CCM_COEF_B_B 0x1b0a8
+#define MALI_C55_CCM_COEF_MASK GENMASK(12, 0)
+#define MALI_C55_REG_CCM_ANTIFOG_GAIN_R 0x1b0b0
+#define MALI_C55_REG_CCM_ANTIFOG_GAIN_G 0x1b0b4
+#define MALI_C55_REG_CCM_ANTIFOG_GAIN_B 0x1b0b8
+#define MALI_C55_CCM_ANTIFOG_GAIN_MASK GENMASK(11, 0)
+#define MALI_C55_REG_CCM_ANTIFOG_OFFSET_R 0x1b0c0
+#define MALI_C55_REG_CCM_ANTIFOG_OFFSET_G 0x1b0c4
+#define MALI_C55_REG_CCM_ANTIFOG_OFFSET_B 0x1b0c8
+#define MALI_C55_CCM_ANTIFOG_OFFSET_MASK GENMASK(11, 0)
+
+/*
+ * The Mali-C55 ISP has up to two output pipes; known as full resolution and
+ * down scaled. The register space for these is laid out identically, but offset
+ * by 372 bytes.
+ */
+#define MALI_C55_CAP_DEV_FR_REG_OFFSET 0x0
+#define MALI_C55_CAP_DEV_DS_REG_OFFSET 0x174
+
+#define MALI_C55_REG_CS_CONV_CONFIG(offset) (0x1c098 + (offset))
+#define MALI_C55_CS_CONV_MATRIX_MASK BIT(0)
+#define MALI_C55_CS_CONV_FILTER_MASK BIT(1)
+#define MALI_C55_CS_CONV_HORZ_DOWNSAMPLE_MASK BIT(2)
+#define MALI_C55_CS_CONV_VERT_DOWNSAMPLE_MASK BIT(3)
+#define MALI_C55_REG_Y_WRITER_MODE(offset) (0x1c0ec + (offset))
+#define MALI_C55_REG_UV_WRITER_MODE(offset) (0x1c144 + (offset))
+#define MALI_C55_WRITER_MODE_MASK GENMASK(4, 0)
+#define MALI_C55_WRITER_SUBMODE_MASK GENMASK(7, 6)
+#define MALI_C55_WRITER_FRAME_WRITE_MASK BIT(9)
+#define MALI_C55_REG_ACTIVE_OUT_Y_SIZE(offset) (0x1c0f0 + (offset))
+#define MALI_C55_REG_ACTIVE_OUT_UV_SIZE(offset) (0x1c148 + (offset))
+#define MALI_C55_REG_ACTIVE_OUT_SIZE_W(w) ((w) << 0)
+#define MALI_C55_REG_ACTIVE_OUT_SIZE_H(h) ((h) << 16)
+#define MALI_C55_REG_Y_WRITER_BANKS_BASE(offset) (0x1c0f4 + (offset))
+#define MALI_C55_REG_Y_WRITER_BANKS_CONFIG(offset) (0x1c108 + (offset))
+#define MALI_C55_REG_Y_WRITER_MAX_BANKS_MASK GENMASK(2, 0)
+#define MALI_C55_REG_Y_WRITER_BANKS_RESTART BIT(3)
+#define MALI_C55_REG_Y_WRITER_OFFSET(offset) (0x1c10c + (offset))
+#define MALI_C55_REG_UV_WRITER_BANKS_BASE(offset) (0x1c14c + (offset))
+#define MALI_C55_REG_UV_WRITER_BANKS_CONFIG(offset) (0x1c160 + (offset))
+#define MALI_C55_REG_UV_WRITER_MAX_BANKS_MASK GENMASK(2, 0)
+#define MALI_C55_REG_UV_WRITER_BANKS_RESTART BIT(3)
+#define MALI_C55_REG_UV_WRITER_OFFSET(offset) (0x1c164 + (offset))
+
+#define MALI_C55_REG_TEST_GEN_CH0_OFF_ON
+#define MALI_C55_REG_TEST_GEN_CH0_PATTERN_TYPE 0x18edc
+
+#define MALI_C55_REG_CROP_EN(offset) (0x1c028 + (offset))
+#define MALI_C55_CROP_ENABLE BIT(0)
+#define MALI_C55_REG_CROP_X_START(offset) (0x1c02c + (offset))
+#define MALI_C55_REG_CROP_Y_START(offset) (0x1c030 + (offset))
+#define MALI_C55_REG_CROP_X_SIZE(offset) (0x1c034 + (offset))
+#define MALI_C55_REG_CROP_Y_SIZE(offset) (0x1c038 + (offset))
+#define MALI_C55_REG_SCALER_TIMEOUT_EN(offset) (0x1c040 + (offset))
+#define MALI_C55_SCALER_TIMEOUT_EN BIT(4)
+#define MALI_C55_SCALER_TIMEOUT(t) ((t) << 16)
+#define MALI_C55_REG_SCALER_IN_WIDTH(offset) (0x1c044 + (offset))
+#define MALI_C55_REG_SCALER_IN_HEIGHT(offset) (0x1c048 + (offset))
+#define MALI_C55_REG_SCALER_OUT_WIDTH(offset) (0x1c04c + (offset))
+#define MALI_C55_REG_SCALER_OUT_HEIGHT(offset) (0x1c050 + (offset))
+#define MALI_C55_REG_SCALER_HFILT_TINC(offset) (0x1c054 + (offset))
+#define MALI_C55_REG_SCALER_HFILT_COEF(offset) (0x1c058 + (offset))
+#define MALI_C55_REG_SCALER_VFILT_TINC(offset) (0x1c05c + (offset))
+#define MALI_C55_REG_SCALER_VFILT_COEF(offset) (0x1c060 + (offset))
+
+#define MALI_C55_REG_GAMMA_RGB_ENABLE(offset) (0x1c064 + (offset))
+#define MALI_C55_GAMMA_ENABLE_MASK BIT(0)
+#define MALI_C55_REG_GAMMA_GAINS_1(offset) (0x1c068 + (offset))
+#define MALI_C55_GAMMA_GAIN_R_MASK GENMASK(11, 0)
+#define MALI_C55_GAMMA_GAIN_G_MASK GENMASK(27, 16)
+#define MALI_C55_REG_GAMMA_GAINS_2(offset) (0x1c06c + (offset))
+#define MALI_C55_GAMMA_GAIN_B_MASK GENMASK(11, 0)
+#define MALI_C55_REG_GAMMA_OFFSETS_1(offset) (0x1c070 + (offset))
+#define MALI_C55_GAMMA_OFFSET_R_MASK GENMASK(11, 0)
+#define MALI_C55_GAMMA_OFFSET_G_MASK GENMASK(27, 16)
+#define MALI_C55_REG_GAMMA_OFFSETS_2(offset) (0x1c074 + (offset))
+#define MALI_C55_GAMMA_OFFSET_B_MASK GENMASK(11, 0)
+
+/* Output DMA Writer */
+
+#define MALI_C55_OUTPUT_DISABLED 0
+#define MALI_C55_OUTPUT_RGB32 1
+#define MALI_C55_OUTPUT_A2R10G10B10 2
+#define MALI_C55_OUTPUT_RGB565 3
+#define MALI_C55_OUTPUT_RGB24 4
+#define MALI_C55_OUTPUT_GEN32 5
+#define MALI_C55_OUTPUT_RAW16 6
+#define MALI_C55_OUTPUT_AYUV 8
+#define MALI_C55_OUTPUT_Y410 9
+#define MALI_C55_OUTPUT_YUY2 10
+#define MALI_C55_OUTPUT_UYVY 11
+#define MALI_C55_OUTPUT_Y210 12
+#define MALI_C55_OUTPUT_NV12_21 13
+#define MALI_C55_OUTPUT_YUV_420_422 17
+#define MALI_C55_OUTPUT_P210_P010 19
+#define MALI_C55_OUTPUT_YUV422 20
I'd define this just below the MALI_C55_REG_UV_WRITER_MODE register
macro.
+
+#define MALI_C55_OUTPUT_PLANE_ALT0 0
+#define MALI_C55_OUTPUT_PLANE_ALT1 1
+#define MALI_C55_OUTPUT_PLANE_ALT2 2
Same here ?
+
+#endif /* _MALI_C55_REGISTERS_H */
diff --git a/drivers/media/platform/arm/mali-c55/mali-c55-resizer-coefs.h
b/drivers/media/platform/arm/mali-c55/mali-c55-resizer-coefs.h
new file mode 100644
index 000000000000..8edae87f1e5f
--- /dev/null
+++ b/drivers/media/platform/arm/mali-c55/mali-c55-resizer-coefs.h
@@ -0,0 +1,382 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * ARM Mali-C55 ISP Driver - Resizer Coefficients
+ *
+ * Copyright (C) 2024 Ideas on Board Oy
+ */
+
+#ifndef _MALI_C55_RESIZER_COEFS_H
+#define _MALI_C55_RESIZER_COEFS_H
+
+#include "mali-c55-common.h"
+
+#define MALI_C55_RESIZER_COEFS_NUM_BANKS 8
+#define MALI_C55_RESIZER_COEFS_NUM_ENTRIES 64
Do these belongs to mali-c55-registers.h ?
+
+static const unsigned int mali_c55_scaler_h_filter_coefficients[8][64] = {
+ { /* Bank 0 */
+ 0x24fc0000, 0x0000fc24, 0x27fc0000, 0x0000fc21,
+ 0x28fc0000, 0x0000fd1f, 0x2cfb0000, 0x0000fd1c,
+ 0x2efb0000, 0x0000fd1a, 0x30fb0000, 0x0000fe17,
+ 0x32fb0000, 0x0000fe15, 0x35fb0000, 0x0000fe12,
+ 0x35fc0000, 0x0000ff10, 0x37fc0000, 0x0000ff0e,
+ 0x39fc0000, 0x0000ff0c, 0x3afd0000, 0x0000ff0a,
+ 0x3afe0000, 0x00000008, 0x3cfe0000, 0x00000006,
+ 0x3dff0000, 0x00000004, 0x3d000000, 0x00000003,
+ 0x3c020000, 0x00000002, 0x3d030000, 0x00000000,
+ 0x3d040000, 0x000000ff, 0x3c060000, 0x000000fe,
+ 0x3a080000, 0x000000fe, 0x3a0aff00, 0x000000fd,
+ 0x390cff00, 0x000000fc, 0x370eff00, 0x000000fc,
+ 0x3510ff00, 0x000000fc, 0x3512fe00, 0x000000fb,
+ 0x3215fe00, 0x000000fb, 0x3017fe00, 0x000000fb,
+ 0x2e1afd00, 0x000000fb, 0x2c1cfd00, 0x000000fb,
+ 0x281ffd00, 0x000000fc, 0x2721fc00, 0x000000fc,
+ },
+ { /* Bank 1 */
+ 0x25fb0000, 0x0000fb25, 0x27fb0000, 0x0000fb23,
+ 0x29fb0000, 0x0000fb21, 0x2afc0000, 0x0000fb1f,
+ 0x2cfc0000, 0x0000fb1d, 0x2efc0000, 0x0000fb1b,
+ 0x2ffd0000, 0x0000fb19, 0x2ffe0000, 0x0000fc17,
+ 0x31fe0000, 0x0000fc15, 0x32ff0000, 0x0000fc13,
+ 0x3400ff00, 0x0000fc11, 0x3301ff00, 0x0000fd10,
+ 0x3402ff00, 0x0000fd0e, 0x3503ff00, 0x0000fd0c,
+ 0x3505ff00, 0x0000fd0a, 0x3506fe00, 0x0000fe09,
+ 0x3607fe00, 0x0000fe07, 0x3509fe00, 0x0000fe06,
+ 0x350afd00, 0x0000ff05, 0x350cfd00, 0x0000ff03,
+ 0x340efd00, 0x0000ff02, 0x3310fd00, 0x0000ff01,
+ 0x3411fc00, 0x0000ff00, 0x3213fc00, 0x000000ff,
+ 0x3115fc00, 0x000000fe, 0x2f17fc00, 0x000000fe,
+ 0x2f19fb00, 0x000000fd, 0x2e1bfb00, 0x000000fc,
+ 0x2c1dfb00, 0x000000fc, 0x2a1ffb00, 0x000000fc,
+ 0x2921fb00, 0x000000fb, 0x2723fb00, 0x000000fb,
+ },
+ { /* Bank 2 */
+ 0x1f010000, 0x0000011f, 0x21010000, 0x0000001e,
+ 0x21020000, 0x0000001d, 0x22020000, 0x0000001c,
+ 0x23030000, 0x0000ff1b, 0x2404ff00, 0x0000ff1a,
+ 0x2504ff00, 0x0000ff19, 0x2505ff00, 0x0000ff18,
+ 0x2606ff00, 0x0000fe17, 0x2607ff00, 0x0000fe16,
+ 0x2708ff00, 0x0000fe14, 0x2709ff00, 0x0000fe13,
+ 0x270aff00, 0x0000fe12, 0x280bfe00, 0x0000fe11,
+ 0x280cfe00, 0x0000fe10, 0x280dfe00, 0x0000fe0f,
+ 0x280efe00, 0x0000fe0e, 0x280ffe00, 0x0000fe0d,
+ 0x2810fe00, 0x0000fe0c, 0x2811fe00, 0x0000fe0b,
+ 0x2712fe00, 0x0000ff0a, 0x2713fe00, 0x0000ff09,
+ 0x2714fe00, 0x0000ff08, 0x2616fe00, 0x0000ff07,
+ 0x2617fe00, 0x0000ff06, 0x2518ff00, 0x0000ff05,
+ 0x2519ff00, 0x0000ff04, 0x241aff00, 0x0000ff04,
+ 0x231bff00, 0x00000003, 0x221c0000, 0x00000002,
+ 0x211d0000, 0x00000002, 0x211e0000, 0x00000001,
+ },
+ { /* Bank 3 */
+ 0x1b06ff00, 0x00ff061b, 0x1b07ff00, 0x00ff061a,
+ 0x1c07ff00, 0x00ff051a, 0x1c08ff00, 0x00ff0519,
+ 0x1c09ff00, 0x00ff0419, 0x1d09ff00, 0x00ff0418,
+ 0x1e0aff00, 0x00ff0317, 0x1e0aff00, 0x00ff0317,
+ 0x1e0bff00, 0x00ff0316, 0x1f0cff00, 0x00ff0215,
+ 0x1e0cff00, 0x00000215, 0x1e0dff00, 0x00000214,
+ 0x1e0e0000, 0x00000113, 0x1e0e0000, 0x00000113,
+ 0x1e0f0000, 0x00000112, 0x1f100000, 0x00000011,
+ 0x20100000, 0x00000010, 0x1f110000, 0x00000010,
+ 0x1e120100, 0x0000000f, 0x1e130100, 0x0000000e,
+ 0x1e130100, 0x0000000e, 0x1e140200, 0x0000ff0d,
+ 0x1e150200, 0x0000ff0c, 0x1f1502ff, 0x0000ff0c,
+ 0x1e1603ff, 0x0000ff0b, 0x1e1703ff, 0x0000ff0a,
+ 0x1e1703ff, 0x0000ff0a, 0x1d1804ff, 0x0000ff09,
+ 0x1c1904ff, 0x0000ff09, 0x1c1905ff, 0x0000ff08,
+ 0x1c1a05ff, 0x0000ff07, 0x1b1a06ff, 0x0000ff07,
+ },
+ { /* Bank 4 */
+ 0x17090000, 0x00000917, 0x18090000, 0x00000916,
+ 0x170a0100, 0x00000816, 0x170a0100, 0x00000816,
+ 0x180b0100, 0x00000715, 0x180b0100, 0x00000715,
+ 0x170c0100, 0x00000715, 0x190c0100, 0x00000614,
+ 0x180d0100, 0x00000614, 0x190d0200, 0x00000513,
+ 0x180e0200, 0x00000513, 0x180e0200, 0x00000513,
+ 0x1a0e0200, 0x00000412, 0x190f0200, 0x00000412,
+ 0x190f0300, 0x00000411, 0x18100300, 0x00000411,
+ 0x1a100300, 0x00000310, 0x18110400, 0x00000310,
+ 0x19110400, 0x0000030f, 0x19120400, 0x0000020f,
+ 0x1a120400, 0x0000020e, 0x18130500, 0x0000020e,
+ 0x18130500, 0x0000020e, 0x19130500, 0x0000020d,
+ 0x18140600, 0x0000010d, 0x19140600, 0x0000010c,
+ 0x17150700, 0x0000010c, 0x18150700, 0x0000010b,
+ 0x18150700, 0x0000010b, 0x17160800, 0x0000010a,
+ 0x17160800, 0x0000010a, 0x18160900, 0x00000009,
+ },
+ { /* Bank 5 */
+ 0x120b0300, 0x00030b12, 0x120c0300, 0x00030b11,
+ 0x110c0400, 0x00030b11, 0x110c0400, 0x00030b11,
+ 0x130c0400, 0x00020a11, 0x120d0400, 0x00020a11,
+ 0x110d0500, 0x00020a11, 0x110d0500, 0x00020a11,
+ 0x130d0500, 0x00010911, 0x130e0500, 0x00010910,
+ 0x120e0600, 0x00010910, 0x120e0600, 0x00010910,
+ 0x130e0600, 0x00010810, 0x120f0600, 0x00010810,
+ 0x120f0700, 0x00000810, 0x130f0700, 0x0000080f,
+ 0x140f0700, 0x0000070f, 0x130f0800, 0x0000070f,
+ 0x12100800, 0x0000070f, 0x12100801, 0x0000060f,
+ 0x13100801, 0x0000060e, 0x12100901, 0x0000060e,
+ 0x12100901, 0x0000060e, 0x13100901, 0x0000050e,
+ 0x13110901, 0x0000050d, 0x11110a02, 0x0000050d,
+ 0x11110a02, 0x0000050d, 0x12110a02, 0x0000040d,
+ 0x13110a02, 0x0000040c, 0x11110b03, 0x0000040c,
+ 0x11110b03, 0x0000040c, 0x12110b03, 0x0000030c,
+ },
+ { /* Bank 6 */
+ 0x0b0a0805, 0x00080a0c, 0x0b0a0805, 0x00080a0c,
+ 0x0c0a0805, 0x00080a0b, 0x0c0a0805, 0x00080a0b,
+ 0x0d0a0805, 0x00070a0b, 0x0d0a0805, 0x00070a0b,
+ 0x0d0a0805, 0x00070a0b, 0x0c0a0806, 0x00070a0b,
+ 0x0b0b0806, 0x00070a0b, 0x0c0b0806, 0x0007090b,
+ 0x0b0b0906, 0x0007090b, 0x0b0b0906, 0x0007090b,
+ 0x0b0b0906, 0x0007090b, 0x0b0b0906, 0x0007090b,
+ 0x0b0b0906, 0x0007090b, 0x0c0b0906, 0x0006090b,
+ 0x0c0b0906, 0x0006090b, 0x0c0b0906, 0x0006090b,
+ 0x0b0b0907, 0x0006090b, 0x0b0b0907, 0x0006090b,
+ 0x0b0b0907, 0x0006090b, 0x0b0b0907, 0x0006090b,
+ 0x0b0b0907, 0x0006090b, 0x0c0b0907, 0x0006080b,
+ 0x0b0b0a07, 0x0006080b, 0x0c0b0a07, 0x0006080a,
+ 0x0d0b0a07, 0x0005080a, 0x0d0b0a07, 0x0005080a,
+ 0x0d0b0a07, 0x0005080a, 0x0c0b0a08, 0x0005080a,
+ 0x0c0b0a08, 0x0005080a, 0x0c0b0a08, 0x0005080a,
+ },
+ { /* Bank 7 */
+ 0x0909090a, 0x00090909, 0x0909090a, 0x00090909,
+ 0x0909090a, 0x00090909, 0x0909090a, 0x00090909,
+ 0x0909090a, 0x00090909, 0x0909090a, 0x00090909,
+ 0x0909090a, 0x00090909, 0x0909090a, 0x00090909,
+ 0x0909090a, 0x00090909, 0x0909090a, 0x00090909,
+ 0x0909090a, 0x00090909, 0x0909090a, 0x00090909,
+ 0x0909090a, 0x00090909, 0x0909090a, 0x00090909,
+ 0x0909090a, 0x00090909, 0x0909090a, 0x00090909,
+ 0x0909090a, 0x00090909, 0x0909090a, 0x00090909,
+ 0x0909090a, 0x00090909, 0x0909090a, 0x00090909,
+ 0x0909090a, 0x00090909, 0x0909090a, 0x00090909,
+ 0x0909090a, 0x00090909, 0x0909090a, 0x00090909,
+ 0x0909090a, 0x00090909, 0x0909090a, 0x00090909,
+ 0x0909090a, 0x00090909, 0x0909090a, 0x00090909,
+ 0x0909090a, 0x00090909, 0x0909090a, 0x00090909,
+ 0x0909090a, 0x00090909, 0x0909090a, 0x00090909,
+ }
+};
+
+static const unsigned int mali_c55_scaler_v_filter_coefficients[8][64] = {
+ { /* Bank 0 */
+ 0x2424fc00, 0x000000fc, 0x2721fc00, 0x000000fc,
+ 0x281ffd00, 0x000000fc, 0x2c1cfd00, 0x000000fb,
+ 0x2e1afd00, 0x000000fb, 0x3017fe00, 0x000000fb,
+ 0x3215fe00, 0x000000fb, 0x3512fe00, 0x000000fb,
+ 0x3510ff00, 0x000000fc, 0x370eff00, 0x000000fc,
+ 0x390cff00, 0x000000fc, 0x3a0aff00, 0x000000fd,
+ 0x3a080000, 0x000000fe, 0x3c060000, 0x000000fe,
+ 0x3d040000, 0x000000ff, 0x3d030000, 0x00000000,
+ 0x3c020000, 0x00000002, 0x3d000000, 0x00000003,
+ 0x3dff0000, 0x00000004, 0x3cfe0000, 0x00000006,
+ 0x3afe0000, 0x00000008, 0x3afd0000, 0x0000ff0a,
+ 0x39fc0000, 0x0000ff0c, 0x37fc0000, 0x0000ff0e,
+ 0x35fc0000, 0x0000ff10, 0x35fb0000, 0x0000fe12,
+ 0x32fb0000, 0x0000fe15, 0x30fb0000, 0x0000fe17,
+ 0x2efb0000, 0x0000fd1a, 0x2cfb0000, 0x0000fd1c,
+ 0x28fc0000, 0x0000fd1f, 0x27fc0000, 0x0000fc21,
+ },
+ { /* Bank 1 */
+ 0x2525fb00, 0x000000fb, 0x2723fb00, 0x000000fb,
+ 0x2921fb00, 0x000000fb, 0x2a1ffb00, 0x000000fc,
+ 0x2c1dfb00, 0x000000fc, 0x2e1bfb00, 0x000000fc,
+ 0x2f19fb00, 0x000000fd, 0x2f17fc00, 0x000000fe,
+ 0x3115fc00, 0x000000fe, 0x3213fc00, 0x000000ff,
+ 0x3411fc00, 0x0000ff00, 0x3310fd00, 0x0000ff01,
+ 0x340efd00, 0x0000ff02, 0x350cfd00, 0x0000ff03,
+ 0x350afd00, 0x0000ff05, 0x3509fe00, 0x0000fe06,
+ 0x3607fe00, 0x0000fe07, 0x3506fe00, 0x0000fe09,
+ 0x3505ff00, 0x0000fd0a, 0x3503ff00, 0x0000fd0c,
+ 0x3402ff00, 0x0000fd0e, 0x3301ff00, 0x0000fd10,
+ 0x3400ff00, 0x0000fc11, 0x32ff0000, 0x0000fc13,
+ 0x31fe0000, 0x0000fc15, 0x2ffe0000, 0x0000fc17,
+ 0x2ffd0000, 0x0000fb19, 0x2efc0000, 0x0000fb1b,
+ 0x2cfc0000, 0x0000fb1d, 0x2afc0000, 0x0000fb1f,
+ 0x29fb0000, 0x0000fb21, 0x27fb0000, 0x0000fb23,
+ },
+ { /* Bank 2 */
+ 0x1f1f0100, 0x00000001, 0x211e0000, 0x00000001,
+ 0x211d0000, 0x00000002, 0x221c0000, 0x00000002,
+ 0x231bff00, 0x00000003, 0x241aff00, 0x0000ff04,
+ 0x2519ff00, 0x0000ff04, 0x2518ff00, 0x0000ff05,
+ 0x2617fe00, 0x0000ff06, 0x2616fe00, 0x0000ff07,
+ 0x2714fe00, 0x0000ff08, 0x2713fe00, 0x0000ff09,
+ 0x2712fe00, 0x0000ff0a, 0x2811fe00, 0x0000fe0b,
+ 0x2810fe00, 0x0000fe0c, 0x280ffe00, 0x0000fe0d,
+ 0x280efe00, 0x0000fe0e, 0x280dfe00, 0x0000fe0f,
+ 0x280cfe00, 0x0000fe10, 0x280bfe00, 0x0000fe11,
+ 0x270aff00, 0x0000fe12, 0x2709ff00, 0x0000fe13,
+ 0x2708ff00, 0x0000fe14, 0x2607ff00, 0x0000fe16,
+ 0x2606ff00, 0x0000fe17, 0x2505ff00, 0x0000ff18,
+ 0x2504ff00, 0x0000ff19, 0x2404ff00, 0x0000ff1a,
+ 0x23030000, 0x0000ff1b, 0x22020000, 0x0000001c,
+ 0x21020000, 0x0000001d, 0x21010000, 0x0000001e,
+ },
+ { /* Bank 3 */
+ 0x1b1b06ff, 0x0000ff06, 0x1b1a06ff, 0x0000ff07,
+ 0x1c1a05ff, 0x0000ff07, 0x1c1905ff, 0x0000ff08,
+ 0x1c1904ff, 0x0000ff09, 0x1d1804ff, 0x0000ff09,
+ 0x1e1703ff, 0x0000ff0a, 0x1e1703ff, 0x0000ff0a,
+ 0x1e1603ff, 0x0000ff0b, 0x1f1502ff, 0x0000ff0c,
+ 0x1e150200, 0x0000ff0c, 0x1e140200, 0x0000ff0d,
+ 0x1e130100, 0x0000000e, 0x1e130100, 0x0000000e,
+ 0x1e120100, 0x0000000f, 0x1f110000, 0x00000010,
+ 0x20100000, 0x00000010, 0x1f100000, 0x00000011,
+ 0x1e0f0000, 0x00000112, 0x1e0e0000, 0x00000113,
+ 0x1e0e0000, 0x00000113, 0x1e0dff00, 0x00000214,
+ 0x1e0cff00, 0x00000215, 0x1f0cff00, 0x00ff0215,
+ 0x1e0bff00, 0x00ff0316, 0x1e0aff00, 0x00ff0317,
+ 0x1e0aff00, 0x00ff0317, 0x1d09ff00, 0x00ff0418,
+ 0x1c09ff00, 0x00ff0419, 0x1c08ff00, 0x00ff0519,
+ 0x1c07ff00, 0x00ff051a, 0x1b07ff00, 0x00ff061a,
+ },
+ { /* Bank 4 */
+ 0x17170900, 0x00000009, 0x18160900, 0x00000009,
+ 0x17160800, 0x0000010a, 0x17160800, 0x0000010a,
+ 0x18150700, 0x0000010b, 0x18150700, 0x0000010b,
+ 0x17150700, 0x0000010c, 0x19140600, 0x0000010c,
+ 0x18140600, 0x0000010d, 0x19130500, 0x0000020d,
+ 0x18130500, 0x0000020e, 0x18130500, 0x0000020e,
+ 0x1a120400, 0x0000020e, 0x19120400, 0x0000020f,
+ 0x19110400, 0x0000030f, 0x18110400, 0x00000310,
+ 0x1a100300, 0x00000310, 0x18100300, 0x00000411,
+ 0x190f0300, 0x00000411, 0x190f0200, 0x00000412,
+ 0x1a0e0200, 0x00000412, 0x180e0200, 0x00000513,
+ 0x180e0200, 0x00000513, 0x190d0200, 0x00000513,
+ 0x180d0100, 0x00000614, 0x190c0100, 0x00000614,
+ 0x170c0100, 0x00000715, 0x180b0100, 0x00000715,
+ 0x180b0100, 0x00000715, 0x170a0100, 0x00000816,
+ 0x170a0100, 0x00000816, 0x18090000, 0x00000916,
+ },
+ { /* Bank 5 */
+ 0x12120b03, 0x0000030b, 0x12110b03, 0x0000030c,
+ 0x11110b03, 0x0000040c, 0x11110b03, 0x0000040c,
+ 0x13110a02, 0x0000040c, 0x12110a02, 0x0000040d,
+ 0x11110a02, 0x0000050d, 0x11110a02, 0x0000050d,
+ 0x13110901, 0x0000050d, 0x13100901, 0x0000050e,
+ 0x12100901, 0x0000060e, 0x12100901, 0x0000060e,
+ 0x13100801, 0x0000060e, 0x12100801, 0x0000060f,
+ 0x12100800, 0x0000070f, 0x130f0800, 0x0000070f,
+ 0x140f0700, 0x0000070f, 0x130f0700, 0x0000080f,
+ 0x120f0700, 0x00000810, 0x120f0600, 0x00010810,
+ 0x130e0600, 0x00010810, 0x120e0600, 0x00010910,
+ 0x120e0600, 0x00010910, 0x130e0500, 0x00010910,
+ 0x130d0500, 0x00010911, 0x110d0500, 0x00020a11,
+ 0x110d0500, 0x00020a11, 0x120d0400, 0x00020a11,
+ 0x130c0400, 0x00020a11, 0x110c0400, 0x00030b11,
+ 0x110c0400, 0x00030b11, 0x120c0300, 0x00030b11,
+ },
+ { /* Bank 6 */
+ 0x0b0c0a08, 0x0005080a, 0x0b0c0a08, 0x0005080a,
+ 0x0c0b0a08, 0x0005080a, 0x0c0b0a08, 0x0005080a,
+ 0x0d0b0a07, 0x0005080a, 0x0d0b0a07, 0x0005080a,
+ 0x0d0b0a07, 0x0005080a, 0x0c0b0a07, 0x0006080a,
+ 0x0b0b0a07, 0x0006080b, 0x0c0b0907, 0x0006080b,
+ 0x0b0b0907, 0x0006090b, 0x0b0b0907, 0x0006090b,
+ 0x0b0b0907, 0x0006090b, 0x0b0b0907, 0x0006090b,
+ 0x0b0b0907, 0x0006090b, 0x0c0b0906, 0x0006090b,
+ 0x0c0b0906, 0x0006090b, 0x0c0b0906, 0x0006090b,
+ 0x0b0b0906, 0x0007090b, 0x0b0b0906, 0x0007090b,
+ 0x0b0b0906, 0x0007090b, 0x0b0b0906, 0x0007090b,
+ 0x0b0b0906, 0x0007090b, 0x0c0b0806, 0x0007090b,
+ 0x0b0b0806, 0x00070a0b, 0x0c0a0806, 0x00070a0b,
+ 0x0d0a0805, 0x00070a0b, 0x0d0a0805, 0x00070a0b,
+ 0x0d0a0805, 0x00070a0b, 0x0c0a0805, 0x00080a0b,
+ 0x0c0a0805, 0x00080a0b, 0x0c0a0805, 0x00080a0b,
+ },
+ { /* Bank 7 */
+ 0x09090909, 0x000a0909, 0x09090909, 0x000a0909,
+ 0x09090909, 0x000a0909, 0x09090909, 0x000a0909,
+ 0x09090909, 0x000a0909, 0x09090909, 0x000a0909,
+ 0x09090909, 0x000a0909, 0x09090909, 0x000a0909,
+ 0x09090909, 0x000a0909, 0x09090909, 0x000a0909,
+ 0x09090909, 0x000a0909, 0x09090909, 0x000a0909,
+ 0x09090909, 0x000a0909, 0x09090909, 0x000a0909,
+ 0x09090909, 0x000a0909, 0x09090909, 0x000a0909,
+ 0x09090909, 0x000a0909, 0x09090909, 0x000a0909,
+ 0x09090909, 0x000a0909, 0x09090909, 0x000a0909,
+ 0x09090909, 0x000a0909, 0x09090909, 0x000a0909,
+ 0x09090909, 0x000a0909, 0x09090909, 0x000a0909,
+ 0x09090909, 0x000a0909, 0x09090909, 0x000a0909,
+ 0x09090909, 0x000a0909, 0x09090909, 0x000a0909,
+ 0x09090909, 0x000a0909, 0x09090909, 0x000a0909,
+ 0x09090909, 0x000a0909, 0x09090909, 0x000a0909,
+ }
+};
+
+struct mali_c55_resizer_coef_bank {
+ unsigned int bank;
This is always equal to the index of the entry in the
mali_c55_coefficient_banks array, you can drop it.
+ unsigned int top;
+ unsigned int bottom;
The bottom value of bank N is always equal to the top value of bank N+1.
You can simplify this by storing a single value.
+};
+
+static const struct mali_c55_resizer_coef_bank mali_c55_coefficient_banks[] = {
+ {
+ .bank = 0,
+ .top = 1000,
+ .bottom = 770,
+ },
+ {
+ .bank = 1,
+ .top = 769,
+ .bottom = 600,
+ },
+ {
+ .bank = 2,
+ .top = 599,
+ .bottom = 460,
+ },
+ {
+ .bank = 3,
+ .top = 459,
+ .bottom = 354,
+ },
+ {
+ .bank = 4,
+ .top = 353,
+ .bottom = 273,
+ },
+ {
+ .bank = 5,
+ .top = 272,
+ .bottom = 210,
+ },
+ {
+ .bank = 6,
+ .top = 209,
+ .bottom = 162,
+ },
+ {
+ .bank = 7,
+ .top = 161,
+ .bottom = 125,
+ },
+};
+
A small comment would be nice, such as
/* Select a bank of resizer coefficients, based on the scaling ratio. */
+static unsigned int mali_c55_calculate_bank_num(struct mali_c55 *mali_c55,
This function is related to the resizers. Add "rsz" somewhere in the
function name, and pass a resizer pointer.
+ unsigned int crop,
+ unsigned int scale)
I think those are the input and output sizes to the scaler. Rename them
to make it clearer.
+{
+ unsigned int tmp;
tmp is almost always a bad variable name. Please use a more descriptive
name, size as rsz_ratio.
+ unsigned int i;
+
+ tmp = (scale * 1000U) / crop;
+
+ for (i = 0; i < ARRAY_SIZE(mali_c55_coefficient_banks); i++) {
+ if (tmp >= mali_c55_coefficient_banks[i].bottom &&
+ tmp <= mali_c55_coefficient_banks[i].top)
+ return mali_c55_coefficient_banks[i].bank;
+ }
+
+ /*
+ * We shouldn't ever get here, in theory. As we have no good choices
+ * simply warn the user and use the first bank of coefficients.
+ */
+ dev_warn(mali_c55->dev, "scaling factor outside defined bounds\n");
+ return 0;
+}
And everything else belongs to mali-c55-resizer.c. Drop this header
file.
+
+#endif /* _MALI_C55_RESIZER_COEFS_H */
diff --git a/drivers/media/platform/arm/mali-c55/mali-c55-resizer.c
b/drivers/media/platform/arm/mali-c55/mali-c55-resizer.c
new file mode 100644
index 000000000000..0a5a2969d3ce
--- /dev/null
+++ b/drivers/media/platform/arm/mali-c55/mali-c55-resizer.c
@@ -0,0 +1,779 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * ARM Mali-C55 ISP Driver - Image signal processor
+ *
+ * Copyright (C) 2024 Ideas on Board Oy
+ */
+
+#include <linux/math.h>
+#include <linux/minmax.h>
+
+#include <media/media-entity.h>
+#include <media/v4l2-subdev.h>
+
+#include "mali-c55-common.h"
+#include "mali-c55-registers.h"
+#include "mali-c55-resizer-coefs.h"
+
+/* Scaling factor in Q4.20 format. */
+#define MALI_C55_RZR_SCALER_FACTOR (1U << 20)
+
+static const u32 rzr_non_bypass_src_fmts[] = {
+ MEDIA_BUS_FMT_RGB121212_1X36,
+ MEDIA_BUS_FMT_YUV10_1X30
+};
+
+static const char * const mali_c55_resizer_names[] = {
+ [MALI_C55_RZR_FR] = "resizer fr",
+ [MALI_C55_RZR_DS] = "resizer ds",
+};
+
+static int mali_c55_rzr_program_crop(struct mali_c55_resizer *rzr,
+ struct v4l2_subdev_state *state)
+{
+ unsigned int reg_offset = rzr->cap_dev->reg_offset;
+ struct mali_c55 *mali_c55 = rzr->mali_c55;
+ struct v4l2_mbus_framefmt *fmt;
+ struct v4l2_rect *crop;
