回复: [PATCH v5 12/14] staging: media: starfive: Add ISP parameters hardware configure

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

 

Hi Jacopo

Thanks for your comments.

> Hi Changhuang
> 
> On Tue, Jul 09, 2024 at 01:38:22AM GMT, Changhuang Liang wrote:
> > Add ISP parameters hardware configure.
> >
> > Signed-off-by: Changhuang Liang <changhuang.liang@xxxxxxxxxxxxxxxx>
> > ---
> >  .../staging/media/starfive/camss/stf-camss.h  |   8 +
> >  .../media/starfive/camss/stf-isp-hw-ops.c     | 573
> ++++++++++++++++++
> >  .../staging/media/starfive/camss/stf-isp.h    | 135 +++++
> >  3 files changed, 716 insertions(+)
> >
> > diff --git a/drivers/staging/media/starfive/camss/stf-camss.h
> > b/drivers/staging/media/starfive/camss/stf-camss.h
> > index 3f84f1a1e997..328318d61c6b 100644
> > --- a/drivers/staging/media/starfive/camss/stf-camss.h
> > +++ b/drivers/staging/media/starfive/camss/stf-camss.h
> > @@ -106,6 +106,14 @@ static inline void stf_isp_reg_set(struct stfcamss
> *stfcamss, u32 reg, u32 mask)
> >  		  stfcamss->isp_base + reg);
> >  }
> >
> > +static inline void stf_isp_reg_fill_zero(struct stfcamss *stfcamss,
> > +u32 reg, u32 size) {
> > +	u32 i;
> > +
> > +	for (i = 0; i < size; i++, reg += 4)
> > +		iowrite32(0, stfcamss->isp_base + reg); }
> > +
> 
> Why in the header file ? Also could memset_io() replace this ?
> 

OK, I will try this.

> >  static inline u32 stf_syscon_reg_read(struct stfcamss *stfcamss, u32
> > reg)  {
> >  	return ioread32(stfcamss->syscon_base + reg); diff --git
> > a/drivers/staging/media/starfive/camss/stf-isp-hw-ops.c
> > b/drivers/staging/media/starfive/camss/stf-isp-hw-ops.c
> > index 0bc5e36f952e..e54368910906 100644
> > --- a/drivers/staging/media/starfive/camss/stf-isp-hw-ops.c
> > +++ b/drivers/staging/media/starfive/camss/stf-isp-hw-ops.c
> > @@ -10,6 +10,25 @@
> >
> >  #include "stf-camss.h"
> >
> > +static const struct stf_isp_module_info mod_info[] = {
> > +	{ ISP_REG_CSI_MODULE_CFG, 2 },
> > +	{ ISP_REG_CSI_MODULE_CFG, 4 },
> > +	{ ISP_REG_CSI_MODULE_CFG, 6 },
> > +	{ ISP_REG_CSI_MODULE_CFG, 7 },
> > +	{ ISP_REG_CSI_MODULE_CFG, 17 },
> > +	{ ISP_REG_ISP_CTRL_1, 1 },
> > +	{ ISP_REG_ISP_CTRL_1, 2 },
> > +	{ ISP_REG_ISP_CTRL_1, 3 },
> > +	{ ISP_REG_ISP_CTRL_1, 4 },
> > +	{ ISP_REG_ISP_CTRL_1, 5 },
> > +	{ ISP_REG_ISP_CTRL_1, 7 },
> > +	{ ISP_REG_ISP_CTRL_1, 8 },
> > +	{ ISP_REG_ISP_CTRL_1, 17 },
> > +	{ ISP_REG_ISP_CTRL_1, 19 },
> > +	{ ISP_REG_ISP_CTRL_1, 21 },
> > +	{ ISP_REG_ISP_CTRL_1, 22 },
> > +};
> > +
> >  static void stf_isp_config_obc(struct stfcamss *stfcamss)  {
> >  	u32 reg_val, reg_add;
> > @@ -517,6 +536,59 @@ static void stf_isp_fill_flag(struct stfcamss
> *stfcamss, void *vaddr,
> >  	}
> >  }
> >
> > +static void stf_isp_set_params(struct stfcamss *stfcamss, void
> > +*vaddr) {
> 
> Shouldn't all these operations be in the -params.c file ?
> 
> > +	struct jh7110_isp_params_buffer *params = (struct
> > +jh7110_isp_params_buffer *)vaddr;
> > +
> > +	if (params->enable_setting & JH7110_ISP_MODULE_WB_SETTING)
> > +		isp_set_ctrl_wb(stfcamss, &params->wb_setting);
> 
> Also this function is exported in stf-isp.h but are only called from within this
> file ? Same for all the other ones I guess ?
> 

Yes, but we have a file stf-isp-hw-ops.c which store the ISP hardware operations.
Should these operations move into -params.c?

> > +
> > +	if (params->enable_setting & JH7110_ISP_MODULE_CAR_SETTING)
> > +		isp_set_ctrl_car(stfcamss, &params->car_setting);
> > +
> > +	if (params->enable_setting & JH7110_ISP_MODULE_CCM_SETTING)
> > +		isp_set_ctrl_ccm(stfcamss, &params->ccm_setting);
> > +
> > +	if (params->enable_setting & JH7110_ISP_MODULE_CFA_SETTING)
> > +		isp_set_ctrl_cfa(stfcamss, &params->cfa_setting);
> > +
> > +	if (params->enable_setting & JH7110_ISP_MODULE_CTC_SETTING)
> > +		isp_set_ctrl_ctc(stfcamss, &params->ctc_setting);
> > +
> > +	if (params->enable_setting & JH7110_ISP_MODULE_DBC_SETTING)
> > +		isp_set_ctrl_dbc(stfcamss, &params->dbc_setting);
> > +
> > +	if (params->enable_setting & JH7110_ISP_MODULE_DNYUV_SETTING)
> > +		isp_set_ctrl_dnyuv(stfcamss, &params->dnyuv_setting);
> > +
> > +	if (params->enable_setting & JH7110_ISP_MODULE_GMARGB_SETTING)
> > +		isp_set_ctrl_gmargb(stfcamss, &params->gmargb_setting);
> > +
> > +	if (params->enable_setting & JH7110_ISP_MODULE_LCCF_SETTING)
> > +		isp_set_ctrl_lccf(stfcamss, &params->lccf_setting);
> > +
> > +	if (params->enable_setting & JH7110_ISP_MODULE_OBC_SETTING)
> > +		isp_set_ctrl_obc(stfcamss, &params->obc_setting);
> > +
> > +	if (params->enable_setting & JH7110_ISP_MODULE_OECF_SETTING)
> > +		isp_set_ctrl_oecf(stfcamss, &params->oecf_setting);
> > +
> > +	if (params->enable_setting & JH7110_ISP_MODULE_R2Y_SETTING)
> > +		isp_set_ctrl_r2y(stfcamss, &params->r2y_setting);
> > +
> > +	if (params->enable_setting & JH7110_ISP_MODULE_SAT_SETTING)
> > +		isp_set_ctrl_sat(stfcamss, &params->sat_setting);
> > +
> > +	if (params->enable_setting & JH7110_ISP_MODULE_SHARP_SETTING)
> > +		isp_set_ctrl_sharp(stfcamss, &params->sharp_setting);
> > +
> > +	if (params->enable_setting & JH7110_ISP_MODULE_YCRV_SETTING)
> > +		isp_set_ctrl_ycrv(stfcamss, &params->ycrv_setting);
> > +
> > +	if (params->enable_setting & JH7110_ISP_MODULE_SC_SETTING)
> > +		isp_set_ctrl_sc(stfcamss, &params->sc_setting); }
> > +
> >  irqreturn_t stf_line_irq_handler(int irq, void *priv)  {
> >  	struct stfcamss *stfcamss = priv;
> > @@ -566,11 +638,20 @@ irqreturn_t stf_isp_irq_handler(int irq, void *priv)
> >  	struct stfcamss *stfcamss = priv;
> >  	struct stf_capture *cap = &stfcamss->captures[STF_CAPTURE_YUV];
> >  	struct stf_capture *cap_scd = &stfcamss->captures[STF_CAPTURE_SCD];
> > +	struct stf_output *output = &stfcamss->output;
> >  	struct stfcamss_buffer *ready_buf;
> >  	u32 status;
> >
> >  	status = stf_isp_reg_read(stfcamss, ISP_REG_ISP_CTRL_0);
> >  	if (status & ISPC_ISP) {
> > +		ready_buf = stf_buf_get_ready(&output->buffers);
> > +		if (ready_buf) {
> > +			stf_isp_set_params(stfcamss, ready_buf->vaddr);
> > +			ready_buf->vb.vb2_buf.timestamp = ktime_get_ns();
> > +			ready_buf->vb.sequence = output->buffers.sequence++;
> > +			vb2_buffer_done(&ready_buf->vb.vb2_buf,
> VB2_BUF_STATE_DONE);
> > +		}
> > +
> >  		if (status & ISPC_ENUO) {
> >  			ready_buf = stf_buf_done(&cap->buffers);
> >  			if (ready_buf)
> > @@ -591,4 +672,496 @@ irqreturn_t stf_isp_irq_handler(int irq, void *priv)
> >  	}
> >
> >  	return IRQ_HANDLED;
> > +};
> > +
> > +int isp_set_ctrl_wb(struct stfcamss *stfcamss, const void *value)
> 
> This function is generic, there's no need to pass a const void * here and cast it
> below, you can use const struct jh7110_isp_wb_setting * as the second
> argument type.
> 

OK

Regards,
Changhuang
[Index of Archives]     [Linux Driver Development]     [Linux Driver Backports]     [DMA Engine]     [Linux GPIO]     [Linux SPI]     [Video for Linux]     [Linux USB Devel]     [Linux Coverity]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]
  Powered by Linux