[PATCH v10 07/17] media: mali-c55: Add Mali-C55 ISP driver
Dan Scally
dan.scally at ideasonboard.com
Mon Jun 30 03:14:43 PDT 2025
Morning Sakari
On 28/06/2025 21:06, Sakari Ailus wrote:
> Hi Daniel,
>
> On 6/24/25 13:21, Daniel Scally wrote:
>
> ...
>
>> +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;
>> +
>> + interrupt_status = mali_c55_read(mali_c55,
>> + MALI_C55_REG_INTERRUPT_STATUS_VECTOR);
>> + 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)))
>
> BIT(), please!
>
> Although, use __ffs() and this becomes redundant.
Ack...or now that I've found it maybe also for_each_set_bit()
>
> ...
>
>> +static void __mali_c55_power_off(struct mali_c55 *mali_c55)
>> +{
>> + reset_control_bulk_assert(ARRAY_SIZE(mali_c55->resets), mali_c55->resets);
>> + clk_bulk_disable_unprepare(ARRAY_SIZE(mali_c55->clks), mali_c55->clks);
>> +}
>> +
>> +static int mali_c55_runtime_suspend(struct device *dev)
>> +{
>> + struct mali_c55 *mali_c55 = dev_get_drvdata(dev);
>> +
>> + free_irq(mali_c55->irqnum, dev);
>
> Do you really free the IRQ on device suspend? The law probably doesn't forbid that though.
Jacopo queried the same operation in the rzv2h-ivc driver. My apparently mistaken understanding was
that not holding the IRQ from .probe() time was best practice, which I think I've gotten from
reading this page [1], which says:
"Although installing the interrupt handler from within the module’s initialization function might
sound like a good idea, it actually isn’t. Because the number of interrupt lines is limited, you
don’t want to waste them. You can easily end up with more devices in your computer than there are
interrupts. If a module requests an IRQ at initialization, it prevents any other driver from using
the interrupt, even if the device holding it is never used. Requesting the interrupt at device open,
on the other hand, allows some sharing of resources."
[1] https://www.oreilly.com/library/view/linux-device-drivers/0596000081/ch09s03.html
>
>> + __mali_c55_power_off(mali_c55);
>> +
>> + return 0;
>> +}
>> +
>> +static int __mali_c55_power_on(struct mali_c55 *mali_c55)
>> +{
>> + int ret;
>> + u32 val;
>> +
>> + 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;
>> + }
>> +
>> + ret = reset_control_bulk_deassert(ARRAY_SIZE(mali_c55->resets),
>> + mali_c55->resets);
>> + if (ret) {
>> + dev_err(mali_c55->dev, "failed to deassert resets\n");
>> + return ret;
>> + }
>> +
>> + /* Use "software only" context management. */
>> + mali_c55_update_bits(mali_c55, MALI_C55_REG_MCU_CONFIG,
>> + MALI_C55_REG_MCU_CONFIG_OVERRIDE_MASK, 0x01);
>> +
>> + /*
>> + * Mask the interrupts and clear any that were set, then unmask the ones
>> + * that we actually want to handle.
>> + */
>> + mali_c55_write(mali_c55, MALI_C55_REG_INTERRUPT_MASK_VECTOR,
>> + MALI_C55_INTERRUPT_MASK_ALL);
>> + mali_c55_write(mali_c55, MALI_C55_REG_INTERRUPT_CLEAR_VECTOR,
>> + MALI_C55_INTERRUPT_MASK_ALL);
>> + mali_c55_write(mali_c55, MALI_C55_REG_INTERRUPT_CLEAR, 0x01);
>> + mali_c55_write(mali_c55, MALI_C55_REG_INTERRUPT_CLEAR, 0x00);
>> +
>> + mali_c55_update_bits(mali_c55, MALI_C55_REG_INTERRUPT_MASK_VECTOR,
>> + MALI_C55_INTERRUPT_BIT(MALI_C55_IRQ_ISP_START) |
>> + MALI_C55_INTERRUPT_BIT(MALI_C55_IRQ_ISP_DONE) |
>> + MALI_C55_INTERRUPT_BIT(MALI_C55_IRQ_FR_Y_DONE) |
>> + MALI_C55_INTERRUPT_BIT(MALI_C55_IRQ_FR_UV_DONE) |
>> + MALI_C55_INTERRUPT_BIT(MALI_C55_IRQ_DS_Y_DONE) |
>> + MALI_C55_INTERRUPT_BIT(MALI_C55_IRQ_DS_UV_DONE),
>> + 0x00);
>> +
>> + /* 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);
>> +
>> + return 0;
>> +}
>> +
>> +static int mali_c55_runtime_resume(struct device *dev)
>> +{
>> + struct mali_c55 *mali_c55 = dev_get_drvdata(dev);
>> + int ret;
>> +
>> + ret = __mali_c55_power_on(mali_c55);
>> + if (ret)
>> + return ret;
>> +
>> + /*
>> + * The driver needs to transfer large amounts of register settings to
>> + * the ISP each frame, using either a DMA transfer or memcpy. We use a
>> + * threaded IRQ to avoid disabling interrupts the entire time that's
>> + * happening.
>> + */
>> + ret = request_threaded_irq(mali_c55->irqnum, NULL, mali_c55_isr,
>> + IRQF_ONESHOT, dev_driver_string(dev), dev);
>> + if (ret) {
>> + __mali_c55_power_off(mali_c55);
>> + dev_err(dev, "failed to request irq\n");
>> + }
>> +
>> + return ret;
>> +}
>> +
>> +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;
>> + struct resource *res;
>> + dma_cap_mask_t mask;
>> + int ret;
>> +
>> + mali_c55 = devm_kzalloc(dev, sizeof(*mali_c55), GFP_KERNEL);
>> + if (!mali_c55)
>> + return -ENOMEM;
>> +
>> + mali_c55->dev = dev;
>> + platform_set_drvdata(pdev, mali_c55);
>> +
>> + mali_c55->base = devm_platform_get_and_ioremap_resource(pdev, 0,
>> + &res);
>> + if (IS_ERR(mali_c55->base))
>> + return dev_err_probe(dev, PTR_ERR(mali_c55->base),
>> + "failed to map IO memory\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");
>> +
>> + for (unsigned int i = 0; i < ARRAY_SIZE(mali_c55_reset_names); i++)
>> + mali_c55->resets[i].id = mali_c55_reset_names[i];
>> +
>> + ret = devm_reset_control_bulk_get_optional_shared(
>> + dev, ARRAY_SIZE(mali_c55_reset_names), mali_c55->resets);
>> + if (ret)
>> + return dev_err_probe(dev, ret, "failed to acquire resets\n");
>> +
>> + of_reserved_mem_device_init(dev);
>> + vb2_dma_contig_set_max_seg_size(dev, UINT_MAX);
>> +
>> + dma_cap_zero(mask);
>> + dma_cap_set(DMA_MEMCPY, mask);
>> +
>> + ret = __mali_c55_power_on(mali_c55);
>> + if (ret)
>> + return dev_err_probe(dev, ret, "failed to power on\n");
>> +
>> + ret = mali_c55_check_hwcfg(mali_c55);
>> + if (ret)
>> + goto err_power_off;
>> +
>> + /*
>> + * No failure here 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);
>> + dev_dbg(mali_c55->dev,
>> + "No DMA channel for config, falling back to memcpy\n");
>> +
>> + ret = mali_c55_init_context(mali_c55, res);
>> + if (ret)
>> + goto err_release_dma_channel;
>> +
>> + mali_c55->media_dev.dev = dev;
>> +
>> + mali_c55->inline_mode = device_property_read_bool(dev, "arm,inline_mode");
>> +
>> + ret = mali_c55_media_frameworks_init(mali_c55);
>> + if (ret)
>> + goto err_free_context_registers;
>> +
>> + __mali_c55_power_off(mali_c55);
>> +
>> + pm_runtime_set_autosuspend_delay(&pdev->dev, 2000);
>> + pm_runtime_use_autosuspend(&pdev->dev);
>> + pm_runtime_enable(&pdev->dev);
>
> Note that runtime PM resume fails before this so accessing UAPI would fail. Please enable runtime
> PM before registering anything outside the driver.
>
>> +
>> + mali_c55->irqnum = platform_get_irq(pdev, 0);
>
> Wouldn't it make sense to read this earlier? For the same reason than above, actually.
Yes to both
>
>> + if (mali_c55->irqnum < 0) {
>> + dev_err(dev, "failed to get interrupt\n");
>> + goto err_pm_runtime_disable;
>> + }
>> +
>> + return 0;
>> +
>> +err_pm_runtime_disable:
>> + pm_runtime_disable(&pdev->dev);
>> + mali_c55_media_frameworks_deinit(mali_c55);
>> +err_free_context_registers:
>> + kfree(mali_c55->context.registers);
>> +err_release_dma_channel:
>> + if (mali_c55->channel)
>> + dma_release_channel(mali_c55->channel);
>> +err_power_off:
>> + __mali_c55_power_off(mali_c55);
>> +
>> + return ret;
>> +}
>> +
>
> ...
>
>> 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 0000000000000000000000000000000000000000..20d4d16c75fbf0d5519ecadb5ed1d080bdae05de
>> --- /dev/null
>> +++ b/drivers/media/platform/arm/mali-c55/mali-c55-isp.c
>> @@ -0,0 +1,656 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * ARM Mali-C55 ISP Driver - Image signal processor
>> + *
>> + * Copyright (C) 2024 Ideas on Board Oy
>
> It's 2025 already.
>
>> + */
>> +
>> +#include <linux/delay.h>
>> +#include <linux/iopoll.h>
>> +#include <linux/property.h>
>> +#include <linux/string.h>
>> +
>> +#include <linux/media/arm/mali-c55-config.h>
>
> If this is a UAPI header, please include uapi in the path, too.
Ack
>
> Earlier such headers have been under include/uapi/linux, I don't object putting new ones elsewhere
> in principle though. Just check with Hans and Laurent, too... I don't have an opinion yet really.
>
>> +/* NOT const because the default needs to be filled in at runtime */
>> +static struct v4l2_ctrl_config mali_c55_isp_v4l2_custom_ctrls[] = {
>> + {
>> + .ops = &mali_c55_isp_ctrl_ops,
>> + .id = V4L2_CID_MALI_C55_CAPABILITIES,
>> + .name = "Mali-C55 ISP Capabilities",
>> + .type = V4L2_CTRL_TYPE_BITMASK,
>> + .min = 0,
>> + .max = MALI_C55_GPS_PONG_FITTED |
>> + MALI_C55_GPS_WDR_FITTED |
>> + MALI_C55_GPS_COMPRESSION_FITTED |
>> + MALI_C55_GPS_TEMPER_FITTED |
>> + MALI_C55_GPS_SINTER_LITE_FITTED |
>> + MALI_C55_GPS_SINTER_FITTED |
>> + MALI_C55_GPS_IRIDIX_LTM_FITTED |
>> + MALI_C55_GPS_IRIDIX_GTM_FITTED |
>> + MALI_C55_GPS_CNR_FITTED |
>> + MALI_C55_GPS_FRSCALER_FITTED |
>> + MALI_C55_GPS_DS_PIPE_FITTED,
>> + .def = 0,
>> + },
>> +};
>> +
>> +static int mali_c55_isp_init_controls(struct mali_c55 *mali_c55)
>> +{
>> + struct v4l2_ctrl_handler *handler = &mali_c55->isp.handler;
>> + struct v4l2_ctrl *capabilities;
>> + int ret;
>> +
>> + ret = v4l2_ctrl_handler_init(handler, 1);
>> + if (ret)
>> + return ret;
>> +
>> + mali_c55_isp_v4l2_custom_ctrls[0].def = mali_c55->capabilities;
>
> The capabilities here are still specific to a device, not global, in principle at least. Can you
> move it here, as a local variable?
The capabilities member of struct mali_c55 you mean? It's used in a couple of other places
throughout the driver, so I'm not sure that makes sense - or am I misunderstanding you?
>
>> +
>> + capabilities = v4l2_ctrl_new_custom(handler,
>> + &mali_c55_isp_v4l2_custom_ctrls[0],
>> + NULL);
>> + if (capabilities)
>> + capabilities->flags |= V4L2_CTRL_FLAG_READ_ONLY;
>> +
>> + if (handler->error) {
>> + dev_err(mali_c55->dev, "failed to register capabilities control\n");
>> + v4l2_ctrl_handler_free(handler);
>> + return handler->error;
>
> v4l2_ctrl_handler_free() will return the error soon, presumably sooner than the above code makes
> it to upstream. Before that, this pattern won't work as v4l2_ctrl_handler_free() also resets the
> handler's error field. :-)
Ah, ok. Thanks
>
>> 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 0000000000000000000000000000000000000000..36a81be0191a15da91809dd2da5d279716f6d725
>> --- /dev/null
>> +++ b/drivers/media/platform/arm/mali-c55/mali-c55-registers.h
>> @@ -0,0 +1,318 @@
>> +/* 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_WRITE_MASK BIT(1)
>> +#define MALI_C55_MCU_CONFIG_WRITE(x) ((x) << 1)
>
> Is x unsigned?
>
>> +#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_INTERRUPT_BIT(x) BIT(x)
>> +
>> +#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_VBLANK(x) ((x) << 16)
>
> Same question for the bit shifts left elsewhere in the header.
>
>> +
>> +#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 << 8)
>> +#define MALI_C55_ISP_RAW_BYPASS_RGB_FR_BYPASS (1 << 8)
>
> BIT() or make these unsigned.
>
> ...
>
>> 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 0000000000000000000000000000000000000000..f0b079a2322125ad6313d6cf9651afaf2180b96c
>> --- /dev/null
>> +++ b/drivers/media/platform/arm/mali-c55/mali-c55-resizer.c
>
> ...
>
>> +static unsigned int mali_c55_rsz_calculate_bank(struct mali_c55 *mali_c55,
>> + unsigned int rsz_in,
>> + unsigned int rsz_out)
>> +{
>> + unsigned int rsz_ratio = (rsz_out * 1000U) / rsz_in;
>
> Can this overflow?
I don't think so; maximum value that should be calculated is 1,000
>
>> + unsigned int i;
>> +
>> + for (i = 0; i < ARRAY_SIZE(mali_c55_rsz_coef_banks_range_start); i++)
>> + if (rsz_ratio >= mali_c55_rsz_coef_banks_range_start[i])
>> + break;
>> +
>> + return i;
>> +}
>
> ...
>
>> +static int mali_c55_rsz_set_sink_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 *sink_fmt;
>> + unsigned int active_sink;
>> + struct v4l2_rect *rect;
>> +
>> + sink_fmt = v4l2_subdev_state_get_format(state, format->pad, 0);
>> +
>> + /*
>> + * Clamp to min/max and then reset crop and compose rectangles to the
>> + * newly applied size.
>> + */
>> + sink_fmt->width = clamp_t(unsigned int, fmt->width,
>> + MALI_C55_MIN_WIDTH, MALI_C55_MAX_WIDTH);
>> + sink_fmt->height = clamp_t(unsigned int, fmt->height,
>> + MALI_C55_MIN_HEIGHT, MALI_C55_MAX_HEIGHT);
>> +
>> + /*
>> + * Make sure the media bus code for the bypass pad is one of the
>> + * supported ISP input media bus codes. Default it to SRGGB otherwise.
>> + */
>> + if (format->pad == MALI_C55_RSZ_SINK_BYPASS_PAD)
>> + sink_fmt->code = mali_c55_isp_get_mbus_config_by_code(fmt->code) ?
>> + fmt->code : MEDIA_BUS_FMT_SRGGB20_1X20;
>> +
>> + *fmt = *sink_fmt;
>> +
>> + if (format->pad == MALI_C55_RSZ_SINK_PAD) {
>> + rect = v4l2_subdev_state_get_crop(state, format->pad);
>> + rect->left = 0;
>> + rect->top = 0;
>> + rect->width = fmt->width;
>> + rect->height = fmt->height;
>> +
>> + rect = v4l2_subdev_state_get_compose(state, format->pad);
>> + rect->left = 0;
>> + rect->top = 0;
>> + rect->width = fmt->width;
>> + rect->height = fmt->height;
>
> If both of the rects are the same, you can simply assign the former to the latter.
Ack
>
> Overall, this seems like a nicely written driver. It's a very big one, too...
>
Thanks! It will grow a bit too I expect...
More information about the linux-arm-kernel
mailing list