Re: [RFC v3 05/13] [media] exynos5-fimc-is: Add register definition and context header

[Sylwester Nawrocki <sylvester.nawrocki@gmail.com>]

On 08/02/2013 05:02 PM, Arun Kumar K wrote:
> This patch adds the register definition file for the fimc-is driver
> and also the header file containing the main context for the driver.
>
> Signed-off-by: Arun Kumar K<arun.kk@samsung.com>
> Signed-off-by: Kilyeon Im<kilyeon.im@samsung.com>
> ---
>   drivers/media/platform/exynos5-is/fimc-is-regs.h |  105 +++++++++++++++
>   drivers/media/platform/exynos5-is/fimc-is.h      |  153 ++++++++++++++++++++++
>   2 files changed, 258 insertions(+)
>   create mode 100644 drivers/media/platform/exynos5-is/fimc-is-regs.h
>   create mode 100644 drivers/media/platform/exynos5-is/fimc-is.h
>
> diff --git a/drivers/media/platform/exynos5-is/fimc-is-regs.h b/drivers/media/platform/exynos5-is/fimc-is-regs.h
> new file mode 100644
> index 0000000..06aa466
> --- /dev/null
> +++ b/drivers/media/platform/exynos5-is/fimc-is-regs.h
> @@ -0,0 +1,105 @@
> +/*
> + * Samsung Exynos5 SoC series FIMC-IS driver
> + *
> + * Copyright (c) 2013 Samsung Electronics Co., Ltd
> + * Arun Kumar K<arun.kk@samsung.com>
> + * Kil-yeon Lim<kilyeon.im@samsung.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#ifndef FIMC_IS_REGS_H
> +#define FIMC_IS_REGS_H
> +
> +/* WDT_ISP register */
> +#define WDT			0x00170000
> +/* MCUCTL register */
> +#define MCUCTL			0x00180000
> +/* MCU Controller Register */
> +#define MCUCTLR				(MCUCTL+0x00)
> +#define MCUCTLR_AXI_ISPX_AWCACHE(x)	((x)<<  16)
> +#define MCUCTLR_AXI_ISPX_ARCACHE(x)	((x)<<  12)
> +#define MCUCTLR_MSWRST			(1<<  0)
> +/* Boot Base OFfset Address Register */
> +#define BBOAR				(MCUCTL+0x04)
> +#define BBOAR_BBOA(x)			((x)<<  0)
> +
> +/* Interrupt Generation Register 0 from Host CPU to VIC */
> +#define INTGR0				(MCUCTL+0x08)
> +#define INTGR0_INTGC(n)			(1<<  ((n) + 16))
> +#define INTGR0_INTGD(n)			(1<<  (n))
> +
> +/* Interrupt Clear Register 0 from Host CPU to VIC */
> +#define INTCR0				(MCUCTL+0x0c)
> +#define INTCR0_INTCC(n)			(1<<  ((n) + 16))
> +#define INTCR0_INTCD(n)			(1<<  (n))
> +
> +/* Interrupt Mask Register 0 from Host CPU to VIC */
> +#define INTMR0				(MCUCTL+0x10)
> +#define INTMR0_INTMC(n)			(1<<  ((n) + 16))
> +#define INTMR0_INTMD(n)			(1<<  (n))
> +
> +/* Interrupt Status Register 0 from Host CPU to VIC */
> +#define INTSR0				(MCUCTL+0x14)
> +#define INTSR0_GET_INTSD(n, x)		(((x)>>  (n))&  0x1)
> +#define INTSR0_GET_INTSC(n, x)		(((x)>>  ((n) + 16))&  0x1)
> +
> +/* Interrupt Mask Status Register 0 from Host CPU to VIC */
> +#define INTMSR0				(MCUCTL+0x18)
> +#define INTMSR0_GET_INTMSD(n, x)	(((x)>>  (n))&  0x1)
> +#define INTMSR0_GET_INTMSC(n, x)	(((x)>>  ((n) + 16))&  0x1)
> +
> +/* Interrupt Generation Register 1 from ISP CPU to Host IC */
> +#define INTGR1				(MCUCTL+0x1c)
> +#define INTGR1_INTGC(n)			(1<<  (n))
> +
> +/* Interrupt Clear Register 1 from ISP CPU to Host IC */
> +#define INTCR1				(MCUCTL+0x20)
> +#define INTCR1_INTCC(n)			(1<<  (n))
> +
> +/* Interrupt Mask Register 1 from ISP CPU to Host IC */
> +#define INTMR1				(MCUCTL+0x24)
> +#define INTMR1_INTMC(n)			(1<<  (n))
> +
> +/* Interrupt Status Register 1 from ISP CPU to Host IC */
> +#define INTSR1				(MCUCTL+0x28)
> +/* Interrupt Mask Status Register 1 from ISP CPU to Host IC */
> +#define INTMSR1				(MCUCTL+0x2c)
> +/* Interrupt Clear Register 2 from ISP BLK's interrupts to Host IC */
> +#define INTCR2				(MCUCTL+0x30)
> +#define INTCR2_INTCC(n)			(1<<  (n))
> +
> +/* Interrupt Mask Register 2 from ISP BLK's interrupts to Host IC */
> +#define INTMR2				(MCUCTL+0x34)
> +#define INTMR2_INTMCIS(n)		(1<<  (n))
> +
> +/* Interrupt Status Register 2 from ISP BLK's interrupts to Host IC */
> +#define INTSR2				(MCUCTL+0x38)
> +/* Interrupt Mask Status Register 2 from ISP BLK's interrupts to Host IC */
> +#define INTMSR2				(MCUCTL+0x3c)
> +/* General Purpose Output Control Register (0~17) */
> +#define GPOCTLR				(MCUCTL+0x40)
> +#define GPOCTLR_GPOG(n, x)		((x)<<  (n))
> +
> +/* General Purpose Pad Output Enable Register (0~17) */
> +#define GPOENCTLR			(MCUCTL+0x44)
> +#define GPOENCTLR_GPOEN0(n, x)		((x)<<  (n))
> +
> +/* General Purpose Input Control Register (0~17) */
> +#define GPICTLR				(MCUCTL+0x48)
> +
> +/* IS Shared Registers between ISP CPU and HOST CPU */
> +#define ISSR(n)			(MCUCTL + 0x80 + (n))
> +
> +/* PMU for FIMC-IS*/
> +#define PMUREG_CMU_RESET_ISP_SYS_PWR_REG	0x1584
> +#define PMUREG_ISP_ARM_CONFIGURATION		0x2280
> +#define PMUREG_ISP_ARM_STATUS			0x2284
> +#define PMUREG_ISP_ARM_OPTION			0x2288
> +#define PMUREG_ISP_LOW_POWER_OFF		0x0004
> +#define PMUREG_ISP_CONFIGURATION		0x4020
> +#define PMUREG_ISP_STATUS			0x4024
> +
> +#endif
> diff --git a/drivers/media/platform/exynos5-is/fimc-is.h b/drivers/media/platform/exynos5-is/fimc-is.h
> new file mode 100644
> index 0000000..a7379dc
> --- /dev/null
> +++ b/drivers/media/platform/exynos5-is/fimc-is.h
> @@ -0,0 +1,153 @@
> +/*
> + * Samsung EXYNOS5 FIMC-IS (Imaging Subsystem) driver
> + *
> + * Copyright (C) 2013 Samsung Electronics Co., Ltd.
> + *  Arun Kumar K<arun.kk@samsung.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#ifndef FIMC_IS_H_
> +#define FIMC_IS_H_
> +
> +#include "fimc-is-err.h"
> +#include "fimc-is-core.h"
> +#include "fimc-is-param.h"
> +#include "fimc-is-pipeline.h"
> +#include "fimc-is-interface.h"
> +
> +#define fimc_interface_to_is(p) container_of(p, struct fimc_is, interface)
> +#define fimc_sensor_to_is(p) container_of(p, struct fimc_is, sensor)
> +
> +/* Macros used by media dev to get the subdev and vfd */
> +/* is -->  driver data from pdev
> + * pid -->  pipeline index */

Wrong multi-line comment style. And perhaps s/--> /- ?

> +#define fimc_is_isp_get_sd(is, pid) (&is->pipeline[pid].isp.subdev)
> +#define fimc_is_isp_get_vfd(is, pid) (&is->pipeline[pid].isp.vfd)
> +#define fimc_is_scc_get_sd(is, pid) \
> +	(&is->pipeline[pid].scaler[SCALER_SCC].subdev)
> +#define fimc_is_scc_get_vfd(is, pid) \
> +	(&is->pipeline[pid].scaler[SCALER_SCC].vfd)
> +#define fimc_is_scp_get_sd(is, pid) \
> +	(&is->pipeline[pid].scaler[SCALER_SCP].subdev)
> +#define fimc_is_scp_get_vfd(is, pid) \
> +	(&is->pipeline[pid].scaler[SCALER_SCP].vfd)
> +/* is -->  driver data from pdev
> + * sid -->  sensor index */
> +#define fimc_is_sensor_get_sd(is, sid) (&is->sensor[sid].subdev)
> +
> +
> +/**
> + * struct fimc_is - fimc lite structure

s/fimc lite/fimc-is driver private data ?

> + * @pdev: pointer to FIMC-IS platform device
> + * @pdata: platform data for FIMC-IS
> + * @alloc_ctx: videobuf2 memory allocator context
> + * @clk: FIMC-IS clocks

Actually it's 'clock'.

> + * @pmu_regs: PMU reg base address
> + * @minfo: internal memory organization info
> + * @sensor: FIMC-IS sensor context
> + * @pipeline: hardware pipeline context
> + * @interface: hardware interface context
> + */
> +struct fimc_is {
> +	struct platform_device		*pdev;
> +
> +	struct vb2_alloc_ctx		*alloc_ctx;
> +	struct clk			*clock[IS_CLK_MAX_NUM];
> +	void __iomem			*pmu_regs;
> +
> +	struct fimc_is_meminfo		minfo;
> +
> +	int				num_instance;

It's undocumented, what it is really used for ?

> +	struct fimc_is_sensor		sensor[FIMC_IS_NUM_SENSORS];
> +	struct fimc_is_pipeline		pipeline[FIMC_IS_NUM_PIPELINES];
> +	struct fimc_is_interface	interface;
> +};
> +
> +/* Queue operations for ISP */
> +static inline void fimc_is_isp_wait_queue_add(struct fimc_is_isp *isp,
> +		struct fimc_is_buf *buf)
> +{
> +	list_add_tail(&buf->list,&isp->wait_queue);
> +	isp->wait_queue_cnt++;
> +}
> +
> +static inline struct fimc_is_buf *fimc_is_isp_wait_queue_get(
> +		struct fimc_is_isp *isp)
> +{
> +	struct fimc_is_buf *buf;
> +	buf = list_entry(isp->wait_queue.next,
> +			struct fimc_is_buf, list);
> +	list_del(&buf->list);
> +	isp->wait_queue_cnt--;
> +	return buf;
> +}
> +
> +static inline void fimc_is_isp_run_queue_add(struct fimc_is_isp *isp,
> +		struct fimc_is_buf *buf)
> +{
> +	list_add_tail(&buf->list,&isp->run_queue);
> +	isp->run_queue_cnt++;
> +}
> +
> +static inline struct fimc_is_buf *fimc_is_isp_run_queue_get(
> +		struct fimc_is_isp *isp)
> +{
> +	struct fimc_is_buf *buf;
> +	buf = list_entry(isp->run_queue.next,
> +			struct fimc_is_buf, list);
> +	list_del(&buf->list);
> +	isp->run_queue_cnt--;
> +	return buf;
> +}
> +
> +/* Queue operations for SCALER */

It would be much neater to define some generic data structure and have
single queue operations for both ISP and SCALER. Maybe it would help
rewriting those as macros ? But this is something that could be well
done in a follow up patch(es).

> +static inline void fimc_is_scaler_wait_queue_add(struct fimc_is_scaler *scp,
> +		struct fimc_is_buf *buf)
> +{
> +	list_add_tail(&buf->list,&scp->wait_queue);
> +	scp->wait_queue_cnt++;
> +}
> +
> +static inline struct fimc_is_buf *fimc_is_scaler_wait_queue_get(
> +		struct fimc_is_scaler *scp)
> +{
> +	struct fimc_is_buf *buf;
> +	buf = list_entry(scp->wait_queue.next,
> +			struct fimc_is_buf, list);
> +	list_del(&buf->list);
> +	scp->wait_queue_cnt--;
> +	return buf;
> +}
> +
> +static inline void fimc_is_scaler_run_queue_add(struct fimc_is_scaler *scp,
> +		struct fimc_is_buf *buf)
> +{
> +	list_add_tail(&buf->list,&scp->run_queue);
> +	scp->run_queue_cnt++;
> +}
> +
> +static inline struct fimc_is_buf *fimc_is_scaler_run_queue_get(
> +		struct fimc_is_scaler *scp)
> +{
> +	struct fimc_is_buf *buf;
> +	buf = list_entry(scp->run_queue.next,
> +			struct fimc_is_buf, list);
> +	list_del(&buf->list);
> +	scp->run_queue_cnt--;
> +	return buf;
> +}
> +
> +static inline void pmu_is_write(u32 v, struct fimc_is *is, unsigned int offset)
> +{
> +	writel(v, is->pmu_regs + offset);
> +}
> +
> +static inline u32 pmu_is_read(struct fimc_is *is, unsigned int offset)
> +{
> +	return readl(is->pmu_regs + offset);
> +}
> +
> +#endif

--
Thanks,
Sylwester