Re: [REVIEW PATCH 06/14] OMAP: CAM: Add ISP Back end

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Sergio,

> +
> +/* Saved parameters */
> +struct prev_params *prev_config_params;
> +

static?

> +/*
> + * Coeficient Tables for the submodules in Preview.
> + * Array is initialised with the values from.the tables text file.
> + */
> +
> +/*
> + * CFA Filter Coefficient Table
> + *
> + */
> +static u32 cfa_coef_table[] = {
> +#include "cfa_coef_table.h"
> +};
> +
> +/*
> + * Gamma Correction Table - Red
> + */
> +static u32 redgamma_table[] = {
> +#include "redgamma_table.h"
> +};
> +
> +/*
> + * Gamma Correction Table - Green
> + */
> +static u32 greengamma_table[] = {
> +#include "greengamma_table.h"
> +};
> +
> +/*
> + * Gamma Correction Table - Blue
> + */
> +static u32 bluegamma_table[] = {
> +#include "bluegamma_table.h"
> +};
> +
> +/*
> + * Noise Filter Threshold table
> + */
> +static u32 noise_filter_table[] = {
> +#include "noise_filter_table.h"
> +};
> +
> +/*
> + * Luminance Enhancement Table
> + */
> +static u32 luma_enhance_table[] = {
> +#include "luma_enhance_table.h"
> +};

Do want this as table format only can be done like request_firmware?

> +
> +/**
> + * omap34xx_isp_preview_config - Abstraction layer Preview configuration.
> + * @userspace_add: Pointer from Userspace to structure with flags and data to
> + *                 update.
> + **/
> +int omap34xx_isp_preview_config(void *userspace_add)
> +{
> +       struct ispprev_hmed prev_hmed_t;
> +       struct ispprev_cfa prev_cfa_t;
> +       struct ispprev_csup csup_t;
> +       struct ispprev_wbal prev_wbal_t;
> +       struct ispprev_blkadj prev_blkadj_t;
> +       struct ispprev_rgbtorgb rgb2rgb_t;
> +       struct ispprev_yclimit yclimit_t;
> +       struct ispprev_dcor prev_dcor_t;
> +       struct ispprv_update_config *preview_struct;
> +       struct isptables_update isp_table_update;
> +       int yen_t[128];
> +
> +       if (userspace_add == NULL)
> +               return -EINVAL;
> +
> +       preview_struct = (struct ispprv_update_config *)userspace_add;

Unnecessary casting. Please remove all the casts when source is void *.

> +
> +       if (ISP_ABS_PREV_LUMAENH & preview_struct->flag) {
> +               if (ISP_ABS_PREV_LUMAENH & preview_struct->update) {
> +                       if (copy_from_user(yen_t, preview_struct->yen,
> +                                                               sizeof(yen_t)))
> +                               goto err_copy_from_user;
> +                       isppreview_config_luma_enhancement(yen_t);
> +               }
> +               params->features |= PREV_LUMA_ENHANCE;
> +       } else if (ISP_ABS_PREV_LUMAENH & preview_struct->update)
> +                       params->features &= ~PREV_LUMA_ENHANCE;
> +
> +       if (ISP_ABS_PREV_INVALAW & preview_struct->flag) {
> +               isppreview_enable_invalaw(1);
> +               params->features |= PREV_INVERSE_ALAW;
> +       } else {
> +               isppreview_enable_invalaw(0);
> +               params->features &= ~PREV_INVERSE_ALAW;
> +       }
> +
> +       if (ISP_ABS_PREV_HRZ_MED & preview_struct->flag) {
> +               if (ISP_ABS_PREV_HRZ_MED & preview_struct->update) {
> +                       if (copy_from_user(&prev_hmed_t,
> +                                               (struct ispprev_hmed *)
> +                                               preview_struct->prev_hmed,
> +                                               sizeof(struct ispprev_hmed)))
> +                               goto err_copy_from_user;
> +                       isppreview_config_hmed(prev_hmed_t);
> +               }
> +               isppreview_enable_hmed(1);
> +               params->features |= PREV_HORZ_MEDIAN_FILTER;
> +       } else if (ISP_ABS_PREV_HRZ_MED & preview_struct->update) {
> +               isppreview_enable_hmed(0);
> +               params->features &= ~PREV_HORZ_MEDIAN_FILTER;
> +       }
> +
> +       if (ISP_ABS_PREV_CFA & preview_struct->flag) {
> +               if (ISP_ABS_PREV_CFA & preview_struct->update) {
> +                       if (copy_from_user(&prev_cfa_t,
> +                                               (struct ispprev_cfa *)
> +                                               preview_struct->prev_cfa,
> +                                               sizeof(struct ispprev_cfa)))
> +                               goto err_copy_from_user;
> +
> +                       isppreview_config_cfa(prev_cfa_t);
> +               }
> +               isppreview_enable_cfa(1);
> +               params->features |= PREV_CFA;
> +       } else if (ISP_ABS_PREV_CFA & preview_struct->update) {
> +               isppreview_enable_cfa(0);
> +               params->features &= ~PREV_CFA;
> +       }
> +
> +       if (ISP_ABS_PREV_CHROMA_SUPP & preview_struct->flag) {
> +               if (ISP_ABS_PREV_CHROMA_SUPP & preview_struct->update) {
> +                       if (copy_from_user(&csup_t,
> +                                               (struct ispprev_csup *)
> +                                               preview_struct->csup,
> +                                               sizeof(struct ispprev_csup)))
> +                               goto err_copy_from_user;
> +                       isppreview_config_chroma_suppression(csup_t);
> +               }
> +               isppreview_enable_chroma_suppression(1);
> +               params->features |= PREV_CHROMA_SUPPRESS;
> +       } else if (ISP_ABS_PREV_CHROMA_SUPP & preview_struct->update) {
> +               isppreview_enable_chroma_suppression(0);
> +               params->features &= ~PREV_CHROMA_SUPPRESS;
> +       }
> +
> +       if (ISP_ABS_PREV_WB & preview_struct->update) {
> +               if (copy_from_user(&prev_wbal_t, (struct ispprev_wbal *)
> +                                               preview_struct->prev_wbal,
> +                                               sizeof(struct ispprev_wbal)))
> +                       goto err_copy_from_user;
> +               isppreview_config_whitebalance(prev_wbal_t);
> +       }
> +
> +       if (ISP_ABS_PREV_BLKADJ & preview_struct->update) {
> +               if (copy_from_user(&prev_blkadj_t, (struct ispprev_blkadjl *)
> +                                       preview_struct->prev_blkadj,
> +                                       sizeof(struct ispprev_blkadj)))
> +                       goto err_copy_from_user;
> +               isppreview_config_blkadj(prev_blkadj_t);
> +       }
> +
> +       if (ISP_ABS_PREV_RGB2RGB & preview_struct->update) {
> +               if (copy_from_user(&rgb2rgb_t, (struct ispprev_rgbtorgb *)
> +                                       preview_struct->rgb2rgb,
> +                                       sizeof(struct ispprev_rgbtorgb)))
> +                       goto err_copy_from_user;
> +               isppreview_config_rgb_blending(rgb2rgb_t);
> +       }
> +
> +       if (ISP_ABS_PREV_COLOR_CONV & preview_struct->update) {
> +               if (copy_from_user(&prev_csc_t, (struct ispprev_csc *)
> +                                               preview_struct->prev_csc,
> +                                               sizeof(struct ispprev_csc)))
> +                       goto err_copy_from_user;
> +               spin_lock(&ispprev_obj.ispprev_lock);
> +               if (ispprev_obj.stream_on == 0) {
> +                       isppreview_config_rgb_to_ycbcr(prev_csc_t);
> +                       CSC_update = 0;
> +               } else
> +                       CSC_update = 1;
> +
> +               spin_unlock(&ispprev_obj.ispprev_lock);
> +       } else
> +               CSC_update = 0;
> +
> +       if (ISP_ABS_PREV_YC_LIMIT & preview_struct->update) {
> +               if (copy_from_user(&yclimit_t, (struct ispprev_yclimit *)
> +                                       preview_struct->yclimit,
> +                                       sizeof(struct ispprev_yclimit)))
> +                       goto err_copy_from_user;
> +               isppreview_config_yc_range(yclimit_t);
> +       }
> +
> +       if (ISP_ABS_PREV_DEFECT_COR & preview_struct->flag) {
> +               if (ISP_ABS_PREV_DEFECT_COR & preview_struct->update) {
> +                       if (copy_from_user(&prev_dcor_t,
> +                                               (struct ispprev_dcor *)
> +                                               preview_struct->prev_dcor,
> +                                               sizeof(struct ispprev_dcor)))
> +                               goto err_copy_from_user;
> +                       isppreview_config_dcor(prev_dcor_t);
> +               }
> +               isppreview_enable_dcor(1);
> +               params->features |= PREV_DEFECT_COR;
> +       } else if (ISP_ABS_PREV_DEFECT_COR & preview_struct->update) {
> +               isppreview_enable_dcor(0);
> +               params->features &= ~PREV_DEFECT_COR;
> +       }
> +
> +       if (ISP_ABS_PREV_GAMMABYPASS & preview_struct->flag) {
> +               isppreview_enable_gammabypass(1);
> +               params->features |= PREV_GAMMA_BYPASS;
> +       } else {
> +               isppreview_enable_gammabypass(0);
> +               params->features &= ~PREV_GAMMA_BYPASS;
> +       }
> +
> +       isp_table_update.update = preview_struct->update;
> +       isp_table_update.flag = preview_struct->flag;
> +       isp_table_update.prev_nf = preview_struct->prev_nf;
> +       isp_table_update.red_gamma = preview_struct->red_gamma;
> +       isp_table_update.green_gamma = preview_struct->green_gamma;
> +       isp_table_update.blue_gamma = preview_struct->blue_gamma;
> +
> +       if (omap34xx_isp_tables_update(&isp_table_update))
> +               goto err_copy_from_user;


This function is really big and hard to review, I am sure there will
be way to break-down into smaller functions as per functionality.

> +
> +       return 0;
> +
> +err_copy_from_user:
> +       printk(KERN_ERR "Preview Config: Copy From User Error");
> +       return -EINVAL;

This should be -EFAULT.

> +}
> +EXPORT_SYMBOL(omap34xx_isp_preview_config);

If possible, please convert all the EXPORT_SYMBOLs to EXPORT_SYMBOL_GPL.

> +
> +/**
> + * omap34xx_isp_tables_update - Abstraction layer Tables update.
> + * @isptables_struct: Pointer from Userspace to structure with flags and table
> + *                 data to update.
> + **/
> +int omap34xx_isp_tables_update(struct isptables_update *isptables_struct)
> +{
> +       int ctr;
> +
> +       if (ISP_ABS_TBL_NF & isptables_struct->flag) {
> +               NF_enable = 1;

CamelCAseS not allowed. Please remove them from all places.

> +               params->features |= PREV_NOISE_FILTER;
> +               if (ISP_ABS_TBL_NF & isptables_struct->update) {
> +                       if (copy_from_user(&prev_nf_t, (struct ispprev_nf *)
> +                                               isptables_struct->prev_nf,
> +                                               sizeof(struct ispprev_nf)))
> +                               goto err_copy_from_user;
> +
> +                       spin_lock(&ispprev_obj.ispprev_lock);
> +                       if (!ispprev_obj.stream_on) {
> +                               NF_update = 0;
> +                               isppreview_config_noisefilter(prev_nf_t);
> +                               isppreview_enable_noisefilter(NF_enable);
> +                       } else
> +                               NF_update = 1;
> +
> +                       spin_unlock(&ispprev_obj.ispprev_lock);
> +               } else
> +                       NF_update = 0;
> +       } else {
> +               NF_enable = 0;
> +               params->features &= ~PREV_NOISE_FILTER;
> +               if (ISP_ABS_TBL_NF & isptables_struct->update)
> +                       NF_update = 1;
> +               else
> +                       NF_update = 0;
> +       }
> +
> +       if (ISP_ABS_TBL_REDGAMMA & isptables_struct->update) {
> +               if (copy_from_user(redgamma_table, isptables_struct->red_gamma,
> +                                               sizeof(redgamma_table))) {
> +                       goto err_copy_from_user;
> +               }
> +               spin_lock(&ispprev_obj.ispprev_lock);
> +               if (!ispprev_obj.stream_on) {
> +                       RG_update = 0;
> +                       isp_reg_writel(ISPPRV_TBL_ADDR_RED_G_START,
> +                               OMAP3_ISP_IOMEM_PREV, ISPPRV_SET_TBL_ADDR);
> +                       for (ctr = 0; ctr < ISP_GAMMA_TABLE_SIZE; ctr++)
> +                               isp_reg_writel(redgamma_table[ctr],
> +                               OMAP3_ISP_IOMEM_PREV, ISPPRV_SET_TBL_DATA);
> +               } else
> +                       RG_update = 1;
> +
> +               spin_unlock(&ispprev_obj.ispprev_lock);
> +       } else
> +               RG_update = 0;
> +
> +       if (ISP_ABS_TBL_GREENGAMMA & isptables_struct->update) {
> +               if (copy_from_user(greengamma_table,
> +                                               isptables_struct->green_gamma,
> +                                               sizeof(greengamma_table)))
> +                       goto err_copy_from_user;
> +               spin_lock(&ispprev_obj.ispprev_lock);
> +               if (!ispprev_obj.stream_on) {
> +                       GG_update = 0;
> +                       isp_reg_writel(ISPPRV_TBL_ADDR_GREEN_G_START,
> +                               OMAP3_ISP_IOMEM_PREV, ISPPRV_SET_TBL_ADDR);
> +                       for (ctr = 0; ctr < ISP_GAMMA_TABLE_SIZE; ctr++)
> +                               isp_reg_writel(greengamma_table[ctr],
> +                                               OMAP3_ISP_IOMEM_PREV,
> +                                               ISPPRV_SET_TBL_DATA);
> +               } else
> +                       GG_update = 1;
> +
> +               spin_unlock(&ispprev_obj.ispprev_lock);
> +       } else
> +               GG_update = 0;
> +
> +       if (ISP_ABS_TBL_BLUEGAMMA & isptables_struct->update) {
> +               if (copy_from_user(bluegamma_table,
> +                                       isptables_struct->blue_gamma,
> +                                       sizeof(bluegamma_table))) {
> +                       goto err_copy_from_user;
> +               }
> +               spin_lock(&ispprev_obj.ispprev_lock);
> +               if (!ispprev_obj.stream_on) {
> +                       BG_update = 0;
> +                       isp_reg_writel(ISPPRV_TBL_ADDR_BLUE_G_START,
> +                                       OMAP3_ISP_IOMEM_PREV,
> +                                       ISPPRV_SET_TBL_ADDR);
> +                       for (ctr = 0; ctr < ISP_GAMMA_TABLE_SIZE; ctr++)
> +                               isp_reg_writel(bluegamma_table[ctr],
> +                                               OMAP3_ISP_IOMEM_PREV,
> +                                               ISPPRV_SET_TBL_DATA);
> +               } else
> +                       BG_update = 1;
> +
> +               spin_unlock(&ispprev_obj.ispprev_lock);
> +       } else
> +               BG_update = 0;
> +
> +       return 0;
> +
> +err_copy_from_user:
> +       printk(KERN_ERR "Preview Tables:Copy From User Error");
> +       return -EINVAL;

-EFAULT please.

> +
> +/**
> + * isppreview_config_noisefilter - Configures the Noise Filter.
> + * @prev_nf: Structure containing the noisefilter table, strength to be used
> + *           for the noise filter and the defect correction enable flag.
> + **/
> +void isppreview_config_noisefilter(struct ispprev_nf prev_nf)
> +{
> +       int i = 0;
> +
> +       isp_reg_writel(prev_nf.spread, OMAP3_ISP_IOMEM_PREV, ISPPRV_NF);
> +       isp_reg_writel(ISPPRV_NF_TABLE_ADDR, OMAP3_ISP_IOMEM_PREV,
> +                                                       ISPPRV_SET_TBL_ADDR);
> +       for (i = 0; i < 64; i++) {

What is 64 here?

> +               isp_reg_writel(prev_nf.table[i], OMAP3_ISP_IOMEM_PREV,
> +                                                       ISPPRV_SET_TBL_DATA);
> +       }
> +}
> +EXPORT_SYMBOL(isppreview_config_noisefilter);
> +
> +/**
> + * isppreview_config_cfa - Configures the CFA Interpolation parameters.
> + * @prev_cfa: Structure containing the CFA interpolation table, CFA format
> + *            in the image, vertical and horizontal gradient threshold.
> + **/
> +void isppreview_config_cfa(struct ispprev_cfa prev_cfa)
> +{
> +       int i = 0;
> +
> +       ispprev_obj.cfafmt = prev_cfa.cfafmt;
> +
> +       isp_reg_or(OMAP3_ISP_IOMEM_PREV, ISPPRV_PCR,
> +                               (prev_cfa.cfafmt << ISPPRV_PCR_CFAFMT_SHIFT));
> +
> +       isp_reg_writel(
> +               (prev_cfa.cfa_gradthrs_vert << ISPPRV_CFA_GRADTH_VER_SHIFT) |
> +               (prev_cfa.cfa_gradthrs_horz << ISPPRV_CFA_GRADTH_HOR_SHIFT),
> +               OMAP3_ISP_IOMEM_PREV, ISPPRV_CFA);
> +
> +       isp_reg_writel(ISPPRV_CFA_TABLE_ADDR, OMAP3_ISP_IOMEM_PREV,
> +                                                       ISPPRV_SET_TBL_ADDR);
> +
> +       for (i = 0; i < 576; i++) {

Ditto.

> +               isp_reg_writel(prev_cfa.cfa_table[i],
> +                               OMAP3_ISP_IOMEM_PREV, ISPPRV_SET_TBL_DATA);
> +       }
> +}
> +EXPORT_SYMBOL(isppreview_config_cfa);
> +
> +/**
> + * isppreview_config_gammacorrn - Configures the Gamma Correction table values
> + * @gtable: Structure containing the table for red, blue, green gamma table.
> + **/
> +void isppreview_config_gammacorrn(struct ispprev_gtable gtable)
> +{
> +       int i = 0;
> +
> +       isp_reg_writel(ISPPRV_REDGAMMA_TABLE_ADDR,
> +                               OMAP3_ISP_IOMEM_PREV, ISPPRV_SET_TBL_ADDR);
> +       for (i = 0; i < 1024; i++) {

Ditto.

> +               isp_reg_writel(gtable.redtable[i],
> +                               OMAP3_ISP_IOMEM_PREV, ISPPRV_SET_TBL_DATA);
> +       }
> +
> +       isp_reg_writel(ISPPRV_GREENGAMMA_TABLE_ADDR,
> +                               OMAP3_ISP_IOMEM_PREV, ISPPRV_SET_TBL_ADDR);
> +       for (i = 0; i < 1024; i++) {

Ditto.

> +               isp_reg_writel(gtable.greentable[i],
> +                               OMAP3_ISP_IOMEM_PREV, ISPPRV_SET_TBL_DATA);
> +       }
> +
> +       isp_reg_writel(ISPPRV_BLUEGAMMA_TABLE_ADDR,
> +                               OMAP3_ISP_IOMEM_PREV, ISPPRV_SET_TBL_ADDR);
> +       for (i = 0; i < 1024; i++) {

Ditto.

> +               isp_reg_writel(gtable.bluetable[i],
> +                               OMAP3_ISP_IOMEM_PREV, ISPPRV_SET_TBL_DATA);
> +       }
> +}
> +EXPORT_SYMBOL(isppreview_config_gammacorrn);
> +
> +/**
> + * isppreview_config_luma_enhancement - Sets the Luminance Enhancement table.
> + * @ytable: Structure containing the table for Luminance Enhancement table.
> + **/
> +void isppreview_config_luma_enhancement(u32 *ytable)
> +{
> +       int i = 0;
> +
> +       isp_reg_writel(ISPPRV_YENH_TABLE_ADDR,
> +                               OMAP3_ISP_IOMEM_PREV, ISPPRV_SET_TBL_ADDR);
> +       for (i = 0; i < 128; i++) {

Ditto.

> +               isp_reg_writel(ytable[i],
> +                               OMAP3_ISP_IOMEM_PREV, ISPPRV_SET_TBL_DATA);
> +       }
> +}
> +EXPORT_SYMBOL(isppreview_config_luma_enhancement);
> +

> +
> +/**
> + * isp_preview_cleanup - Module Cleanup.
> + **/
> +void __exit isp_preview_cleanup(void)
> +{
> +       kfree(prev_config_params);
> +       prev_config_params = NULL;

Is this NULL assignment needed?

> +}
> diff --git a/drivers/media/video/isp/isppreview.h b/drivers/media/video/isp/isppreview.h
> new file mode 100644
> index 0000000..630295c
> --- /dev/null
> +++ b/drivers/media/video/isp/isppreview.h
> @@ -0,0 +1,356 @@
> +/*
> + * drivers/media/video/isp/isppreview.h
> + *
> + * Driver header file for Preview module in TI's OMAP3 Camera ISP
> + *
> + * Copyright (C) 2008 Texas Instruments, Inc.
> + *

Please update copyright too. Add 2009 :)

-- 
---Trilok Soni
http://triloksoni.wordpress.com
http://www.linkedin.com/in/triloksoni
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[Index of Archives]     [Linux Arm (vger)]     [ARM Kernel]     [ARM MSM]     [Linux Tegra]     [Linux WPAN Networking]     [Linux Wireless Networking]     [Maemo Users]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite Trails]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux