Re: [PATCH v1 027/107] media: ti-vpe: cal: Name all cal_camerarx pointers consistently

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

Hi Benoit,

On Thu, Jun 18, 2020 at 09:06:09AM -0500, Benoit Parrot wrote:
> Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote on Mon [2020-Jun-15 02:58:24 +0300]:
> > Name all variables htat point to a cal_camerax instance 'phy' instead of
> > 'cc'. The name 'cc' refers to Camera Core, but is not commonly used in
> > the driver or in datasheets.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> >  drivers/media/platform/ti-vpe/cal.c | 102 ++++++++++++++--------------
> >  1 file changed, 51 insertions(+), 51 deletions(-)
> > 
> > diff --git a/drivers/media/platform/ti-vpe/cal.c b/drivers/media/platform/ti-vpe/cal.c
> > index 615e9d97e61f..8864a00a22b0 100644
> > --- a/drivers/media/platform/ti-vpe/cal.c
> > +++ b/drivers/media/platform/ti-vpe/cal.c
> > @@ -312,7 +312,8 @@ static const struct cal_data am654_cal_data = {
> >   * the CAMERARX instances. Instances of struct cal_dev are named cal through the
> >   * driver.
> >   *
> > - * The cal_camerarx structure represents one CAMERARX instance.
> > + * The cal_camerarx structure represents one CAMERARX instance. Instances of
> > + * cal_camerarx are named phy through the driver.
> >   *
> >   * The cal_ctx structure represents the combination of one CSI-2 context, one
> >   * processing context and one DMA context. Instance of struct cal_ctx are named
> > @@ -344,7 +345,7 @@ struct cal_dev {
> >  	u32			syscon_camerrx_offset;
> >  
> >  	/* Camera Core Module handle */
> > -	struct cal_camerarx	*cc[CAL_NUM_CSI2_PORTS];
> > +	struct cal_camerarx	*phy[CAL_NUM_CSI2_PORTS];
> >  
> >  	struct cal_ctx		*ctx[CAL_NUM_CONTEXT];
> >  };
> > @@ -361,7 +362,7 @@ struct cal_ctx {
> >  	struct v4l2_fwnode_endpoint	endpoint;
> >  
> >  	struct cal_dev		*cal;
> > -	struct cal_camerarx	*cc;
> > +	struct cal_camerarx	*phy;
> >  
> >  	/* v4l2_ioctl mutex */
> >  	struct mutex		mutex;
> > @@ -468,7 +469,7 @@ static u32 cal_data_get_num_csi2_phy(struct cal_dev *cal)
> >  }
> >  
> >  static int cal_camerarx_regmap_init(struct cal_dev *cal,
> > -				    struct cal_camerarx *cc,
> > +				    struct cal_camerarx *phy,
> >  				    unsigned int idx)
> >  {
> >  	const struct cal_camerarx_data *phy_data;
> > @@ -490,12 +491,12 @@ static int cal_camerarx_regmap_init(struct cal_dev *cal,
> >  		 * Here we update the reg offset with the
> >  		 * value found in DT
> >  		 */
> > -		cc->phy.fields[i] = devm_regmap_field_alloc(&cal->pdev->dev,
> > -							    cal->syscon_camerrx,
> > -							    field);
> > -		if (IS_ERR(cc->phy.fields[i])) {
> > +		phy->phy.fields[i] = devm_regmap_field_alloc(&cal->pdev->dev,
> > +							     cal->syscon_camerrx,
> > +							     field);
> 
> So we end up with these construct phy->phy.
> Is that really more readable?

I agree with you that it's not very nice. That's why it gets fixed
later, with the 'fields' field being moved from struct cal_camerarx_data
to struct cal_camerarx :-) I could try to rearrange the patches to avoid
this, but as it's an intermediary step only, it would be quite a bit of
rebase and conflict resolution to achieve the exact same result. Would
it be OK with you to keep this intermediate step as-is (assuming you
like the end result of course) ?

-- 
Regards,

Laurent Pinchart