Re: [PATCH v10 07/17] media: mali-c55: Add Mali-C55 ISP driver

Dan Scally
2025-06-30T10:14:49Z

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...