Re: [RFC 10/12] exynos-fimc-is: Adds the hardware interface module

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

On 03/08/2013 03:59 PM, Arun Kumar K wrote:
> The hardware interface module finally sends the commands to the
> FIMC-IS firmware and runs the interrupt handler for getting the
> responses.
>
> Signed-off-by: Arun Kumar K<arun.kk@samsung.com>
> Signed-off-by: Kilyeon Im<kilyeon.im@samsung.com>
> ---
>   .../media/platform/exynos5-is/fimc-is-interface.c  | 1003 ++++++++++++++++++++
>   .../media/platform/exynos5-is/fimc-is-interface.h  |  130 +++
>   2 files changed, 1133 insertions(+)
>   create mode 100644 drivers/media/platform/exynos5-is/fimc-is-interface.c
>   create mode 100644 drivers/media/platform/exynos5-is/fimc-is-interface.h
[...]
> +static void itf_set_state(struct fimc_is_interface *itf,
> +		unsigned long state)
> +{
> +	unsigned long flags;
> +	spin_lock_irqsave(&itf->slock_state, flags);
> +	set_bit(state,&itf->state);

If itf->state is always modified with itf->slock_state spinlock
held you could use non-atomic variant, i.e. __set_bit().

> +	spin_unlock_irqrestore(&itf->slock_state, flags);
> +}
> +
> +static void itf_clr_state(struct fimc_is_interface *itf,
> +		unsigned long state)
> +{
> +	unsigned long flags;
> +	spin_lock_irqsave(&itf->slock_state, flags);
> +	clear_bit(state,&itf->state);
> +	spin_unlock_irqrestore(&itf->slock_state, flags);
> +}
> +
> +static int itf_get_state(struct fimc_is_interface *itf,
> +		unsigned long state)
> +{
> +	int ret = 0;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&itf->slock_state, flags);
> +	ret = test_bit(state,&itf->state);
> +	spin_unlock_irqrestore(&itf->slock_state, flags);
> +	return ret;
> +}

> +int fimc_is_itf_hw_dump(struct fimc_is_interface *itf)
> +{
> +	struct fimc_is *is = fimc_interface_to_is(itf);
> +	int debug_cnt;
> +	char *debug;
> +	char letter;
> +	int count = 0, i;
> +
> +	debug = (char *)(is->minfo.fw_vaddr + DEBUG_OFFSET);
> +	debug_cnt = *((int *)(is->minfo.fw_vaddr + DEBUGCTL_OFFSET))
> +			- DEBUG_OFFSET;
> +
> +	if (itf->debug_cnt>  debug_cnt)
> +		count = (DEBUG_CNT - itf->debug_cnt) + debug_cnt;
> +	else
> +		count = debug_cnt - itf->debug_cnt;
> +
> +	if (count) {
> +		pr_info("start(%d %d)\n", debug_cnt, count);
> +		for (i = itf->debug_cnt; count>  0; count--) {
> +			letter = debug[i];
> +			if (letter)
> +				pr_cont("%c", letter);
> +			i++;
> +			if (i>  DEBUG_CNT)
> +				i = 0;
> +		}
> +		itf->debug_cnt = debug_cnt;
> +		pr_info("end\n");
> +	}
> +	return count;
> +}

Why don't you use debugfs for dumping this log buffer ? I found it much
more convenient and the logs appear exactly as written by the firmware.

This is what I have in the Exynos4x12 FIMC-IS driver:


static int fimc_is_log_show(struct seq_file *s, void *data)
{
	struct fimc_is *is = s->private;
	const u8 *buf = is->memory.vaddr + FIMC_IS_DEBUG_REGION_OFFSET;

	if (is->memory.vaddr == NULL) {
		dev_err(&is->pdev->dev, "Firmware memory is not initialized\n");
		return -EIO;
	}

	seq_printf(s, "%s\n", buf);
	return 0;
}

static int fimc_is_debugfs_open(struct inode *inode, struct file *file)
{
	return single_open(file, fimc_is_log_show, inode->i_private);
}

static const struct file_operations fimc_is_debugfs_fops = {
	.open		= fimc_is_debugfs_open,
	.read		= seq_read,
	.llseek		= seq_lseek,
	.release	= single_release,
};

static void fimc_is_debugfs_remove(struct fimc_is *is)
{
	debugfs_remove(is->debugfs_entry);
	is->debugfs_entry = NULL;
}

static int fimc_is_debugfs_create(struct fimc_is *is)
{
	struct dentry *dentry;

	is->debugfs_entry = debugfs_create_dir("fimc_is", NULL);

	dentry = debugfs_create_file("fw_log", S_IRUGO, is->debugfs_entry,
				     is, &fimc_is_debugfs_fops);
	if (!dentry)
		fimc_is_debugfs_remove(is);

	return is->debugfs_entry == NULL ? -EIO : 0;
}