Re: [PATCH v2 6/7] media: rcar-isp: Rename base register variable

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

On Tue, Apr 22, 2025 at 01:47:35AM +0300, Laurent Pinchart wrote:
> Hi Niklas,
> 
> Thank you for the patch.
> 
> On Mon, Apr 21, 2025 at 01:12:39PM +0200, Niklas Söderlund wrote:
> > Prepare for extending the driver to in addition to support the channel
> 
> s/support/supporting/
> 
> > selector (CS) also support the core ISP. The two different functions
> > have different base addresses so the driver needs to distinguish between
> > them.
> > 
> > Prepare for this by marking existing base address variable and
> > read/write functions to make it clear it operates on the CS portion of
> 
> s/it operates/they operate/
> 
> > the driver. There is no functional change.
> > 
> > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> > Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > ---
> >  .../media/platform/renesas/rcar-isp/csisp.c   | 46 +++++++++----------
> >  1 file changed, 23 insertions(+), 23 deletions(-)
> > 
> > diff --git a/drivers/media/platform/renesas/rcar-isp/csisp.c b/drivers/media/platform/renesas/rcar-isp/csisp.c
> > index 4bc89d4757fa..f36d43c2e0a2 100644
> > --- a/drivers/media/platform/renesas/rcar-isp/csisp.c
> > +++ b/drivers/media/platform/renesas/rcar-isp/csisp.c
> > @@ -159,7 +159,7 @@ enum rcar_isp_pads {
> >  
> >  struct rcar_isp {
> >  	struct device *dev;
> > -	void __iomem *base;
> > +	void __iomem *csbase;
> >  	struct reset_control *rstc;
> >  
> >  	enum rcar_isp_input csi_input;
> > @@ -184,14 +184,14 @@ static inline struct rcar_isp *notifier_to_isp(struct v4l2_async_notifier *n)
> >  	return container_of(n, struct rcar_isp, notifier);
> >  }
> >  
> > -static void risp_write(struct rcar_isp *isp, u32 offset, u32 value)
> > +static void risp_write_cs(struct rcar_isp *isp, u32 offset, u32 value)
> 
> Not sure what the other write function will be called, but I would have
> called this risp_cs_write(). Up to you.
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

I meant

Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>

> >  {
> > -	iowrite32(value, isp->base + offset);
> > +	iowrite32(value, isp->csbase + offset);
> >  }
> >  
> > -static u32 risp_read(struct rcar_isp *isp, u32 offset)
> > +static u32 risp_read_cs(struct rcar_isp *isp, u32 offset)
> >  {
> > -	return ioread32(isp->base + offset);
> > +	return ioread32(isp->csbase + offset);
> >  }
> >  
> >  static int risp_power_on(struct rcar_isp *isp)
> > @@ -245,31 +245,31 @@ static int risp_start(struct rcar_isp *isp, struct v4l2_subdev_state *state)
> >  	if (isp->csi_input == RISP_CSI_INPUT1)
> >  		sel_csi = ISPINPUTSEL0_SEL_CSI0;
> >  
> > -	risp_write(isp, ISPINPUTSEL0_REG,
> > -		   risp_read(isp, ISPINPUTSEL0_REG) | sel_csi);
> > +	risp_write_cs(isp, ISPINPUTSEL0_REG,
> > +		      risp_read_cs(isp, ISPINPUTSEL0_REG) | sel_csi);
> >  
> >  	/* Configure Channel Selector. */
> >  	for (vc = 0; vc < 4; vc++) {
> >  		u8 ch = vc + 4;
> >  		u8 dt = format->datatype;
> >  
> > -		risp_write(isp, ISPCS_FILTER_ID_CH_REG(ch), BIT(vc));
> > -		risp_write(isp, ISPCS_DT_CODE03_CH_REG(ch),
> > -			   ISPCS_DT_CODE03_EN3 | ISPCS_DT_CODE03_DT3(dt) |
> > -			   ISPCS_DT_CODE03_EN2 | ISPCS_DT_CODE03_DT2(dt) |
> > -			   ISPCS_DT_CODE03_EN1 | ISPCS_DT_CODE03_DT1(dt) |
> > -			   ISPCS_DT_CODE03_EN0 | ISPCS_DT_CODE03_DT0(dt));
> > +		risp_write_cs(isp, ISPCS_FILTER_ID_CH_REG(ch), BIT(vc));
> > +		risp_write_cs(isp, ISPCS_DT_CODE03_CH_REG(ch),
> > +			      ISPCS_DT_CODE03_EN3 | ISPCS_DT_CODE03_DT3(dt) |
> > +			      ISPCS_DT_CODE03_EN2 | ISPCS_DT_CODE03_DT2(dt) |
> > +			      ISPCS_DT_CODE03_EN1 | ISPCS_DT_CODE03_DT1(dt) |
> > +			      ISPCS_DT_CODE03_EN0 | ISPCS_DT_CODE03_DT0(dt));
> >  	}
> >  
> >  	/* Setup processing method. */
> > -	risp_write(isp, ISPPROCMODE_DT_REG(format->datatype),
> > -		   ISPPROCMODE_DT_PROC_MODE_VC3(format->procmode) |
> > -		   ISPPROCMODE_DT_PROC_MODE_VC2(format->procmode) |
> > -		   ISPPROCMODE_DT_PROC_MODE_VC1(format->procmode) |
> > -		   ISPPROCMODE_DT_PROC_MODE_VC0(format->procmode));
> > +	risp_write_cs(isp, ISPPROCMODE_DT_REG(format->datatype),
> > +		      ISPPROCMODE_DT_PROC_MODE_VC3(format->procmode) |
> > +		      ISPPROCMODE_DT_PROC_MODE_VC2(format->procmode) |
> > +		      ISPPROCMODE_DT_PROC_MODE_VC1(format->procmode) |
> > +		      ISPPROCMODE_DT_PROC_MODE_VC0(format->procmode));
> >  
> >  	/* Start ISP. */
> > -	risp_write(isp, ISPSTART_REG, ISPSTART_START);
> > +	risp_write_cs(isp, ISPSTART_REG, ISPSTART_START);
> >  
> >  	ret = v4l2_subdev_enable_streams(isp->remote, isp->remote_pad,
> >  					 BIT_ULL(0));
> > @@ -284,7 +284,7 @@ static void risp_stop(struct rcar_isp *isp)
> >  	v4l2_subdev_disable_streams(isp->remote, isp->remote_pad, BIT_ULL(0));
> >  
> >  	/* Stop ISP. */
> > -	risp_write(isp, ISPSTART_REG, ISPSTART_STOP);
> > +	risp_write_cs(isp, ISPSTART_REG, ISPSTART_STOP);
> >  
> >  	risp_power_off(isp);
> >  }
> > @@ -465,9 +465,9 @@ static const struct media_entity_operations risp_entity_ops = {
> >  static int risp_probe_resources(struct rcar_isp *isp,
> >  				struct platform_device *pdev)
> >  {
> > -	isp->base = devm_platform_get_and_ioremap_resource(pdev, 0, NULL);
> > -	if (IS_ERR(isp->base))
> > -		return PTR_ERR(isp->base);
> > +	isp->csbase = devm_platform_get_and_ioremap_resource(pdev, 0, NULL);
> > +	if (IS_ERR(isp->csbase))
> > +		return PTR_ERR(isp->csbase);
> >  
> >  	isp->rstc = devm_reset_control_get(&pdev->dev, NULL);
> >  

-- 
Regards,

Laurent Pinchart