Received-SPF: pass (google.com: domain of linux-kernel+bounces-168751-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Sat, 4 May 2024 19:30:47 +0300
From: Andrey Skvortsov <andrej.skvortzov@gmail.com>
To: Sakari Ailus <sakari.ailus@linux.intel.com>,
	Mauro Carvalho Chehab <mchehab@kernel.org>,
	Rob Herring <robh+dt@kernel.org>,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
	Conor Dooley <conor+dt@kernel.org>, linux-media@vger.kernel.org,
	devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
	=?utf-8?Q?Ond=C5=99ej?= Jirman <megi@xff.cz>,
	Pavel Machek <pavel@ucw.cz>,
	Arnaud Ferraris <arnaud.ferraris@collabora.com>
Subject: Re: [PATCH v2 2/2] media: gc2145: implement basic dvp bus support
Message-ID: <ZjZit7sDmXJXDg56@skv.local>
Mail-Followup-To: Andrey Skvortsov <andrej.skvortzov@gmail.com>,
	Sakari Ailus <sakari.ailus@linux.intel.com>,
	Mauro Carvalho Chehab <mchehab@kernel.org>,
	Rob Herring <robh+dt@kernel.org>,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
	Conor Dooley <conor+dt@kernel.org>, linux-media@vger.kernel.org,
	devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
	=?utf-8?Q?Ond=C5=99ej?= Jirman <megi@xff.cz>,
	Pavel Machek <pavel@ucw.cz>,
	Arnaud Ferraris <arnaud.ferraris@collabora.com>
References: <20240222062214.2627810-1-andrej.skvortzov@gmail.com>
 <20240222062214.2627810-3-andrej.skvortzov@gmail.com>
 <20240426163826.GA3095395@gnbcxd0016.gnb.st.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <20240426163826.GA3095395@gnbcxd0016.gnb.st.com>

Hi Alain,

thanks for the review.

On 24-04-26 18:38, Alain Volmat wrote:
> Hi Andrey,
> 
> > --- a/drivers/media/i2c/gc2145.c
> > +++ b/drivers/media/i2c/gc2145.c
> > @@ -39,6 +39,10 @@
> >  #define GC2145_REG_ANALOG_MODE1	CCI_REG8(0x17)
> >  #define GC2145_REG_OUTPUT_FMT	CCI_REG8(0x84)
> >  #define GC2145_REG_SYNC_MODE	CCI_REG8(0x86)
> > +#define GC2145_SYNC_MODE_VSYNC_POL	BIT(0)
> > +#define GC2145_SYNC_MODE_HSYNC_POL	BIT(1)
> > +#define GC2145_SYNC_MODE_OPCLK_POL	BIT(2)
> > +#define GC2145_SYNC_MODE_OPCLK_GATE	BIT(3)
> 
> OPCLK_GATE added but never used.
All near bits were described. I decided to put description for this bit
as well, so all bits 0-5 are described in source code.

> 
> >  #define GC2145_SYNC_MODE_COL_SWITCH	BIT(4)
> >  #define GC2145_SYNC_MODE_ROW_SWITCH	BIT(5)
> >  #define GC2145_REG_BYPASS_MODE	CCI_REG8(0x89)
> > @@ -53,6 +57,12 @@
> >  #define GC2145_REG_GLOBAL_GAIN	CCI_REG8(0xb0)
> >  #define GC2145_REG_CHIP_ID	CCI_REG16(0xf0)
> >  #define GC2145_REG_PAD_IO	CCI_REG8(0xf2)
> > +#define GC2145_REG_PLL_MODE1	CCI_REG8(0xf7)
> > +#define GC2145_REG_PLL_MODE2	CCI_REG8(0xf8)
> > +#define GC2145_REG_CM_MODE	CCI_REG8(0xf9)
> > +#define GC2145_REG_CLK_DIV_MODE	CCI_REG8(0xfa)
> > +#define GC2145_REG_ANALOG_PWC	CCI_REG8(0xfc)
> 
> All 5 define added but never used, those settings are part of the cci_sequences
> table. Maybe either keep the define and update the tables or drop the
> new define ?

> > +#define GC2145_REG_PAD_IO	CCI_REG8(0xf2)
> 
> Was already defined in the existing code, see above.
Thanks, I'll drop all unneeded defines in v3.


> > @@ -773,6 +784,38 @@ static int gc2145_set_pad_format(struct v4l2_subdev *sd,
> >  	return 0;
> >  }
> >  
> > +static int gc2145_config_dvp_mode(struct gc2145 *gc2145,
> > +				   const struct gc2145_format *gc2145_format)
> 
> alignment ?
Thanks.

> > +{
> > +	int ret = 0;
> > +	u64 sync_mode;
> > +	int flags;
> > +
> > +	flags = gc2145->ep.bus.parallel.flags;
> 
> int flags = gc2145->ep.bus.parallel.flags;
> ?
Fix in v3.

> > @@ -1244,36 +1292,57 @@ static int gc2145_check_hwcfg(struct device *dev)
> >  		return -EINVAL;
> >  	}
> >  
> > -	ret = v4l2_fwnode_endpoint_alloc_parse(endpoint, &ep_cfg);
> > +
> 
> no new line here.
ok.

> > +	gc2145->ep.bus_type = V4L2_MBUS_CSI2_DPHY;
> > +	ret = v4l2_fwnode_endpoint_alloc_parse(endpoint, &gc2145->ep);
> > +	if (ret) {
> > +		gc2145->ep.bus_type = V4L2_MBUS_PARALLEL;
> > +		ret = v4l2_fwnode_endpoint_alloc_parse(endpoint, &gc2145->ep);
> > +	}
> >  	fwnode_handle_put(endpoint);
> > -	if (ret)
> > +	if (ret) {
> > +		dev_err(dev, "could not parse endpoint\n");
> >  		return ret;
> > -
> > -	/* Check the number of MIPI CSI2 data lanes */
> > -	if (ep_cfg.bus.mipi_csi2.num_data_lanes != 2) {
> > -		dev_err(dev, "only 2 data lanes are currently supported\n");
> > -		ret = -EINVAL;
> > -		goto out;
> >  	}
> >  
> > -	/* Check the link frequency set in device tree */
> > -	if (!ep_cfg.nr_of_link_frequencies) {
> > -		dev_err(dev, "link-frequency property not found in DT\n");
> > +	switch (gc2145->ep.bus_type) {
> > +	case V4L2_MBUS_CSI2_DPHY:
> > +		/* Check the link frequency set in device tree */
> > +		if (!gc2145->ep.nr_of_link_frequencies) {
> > +			dev_err(dev, "link-frequencies property not found in DT\n");
> > +			ret = -EINVAL;
> > +			goto out;
> > +		}
> 
> any reason for change of the if order ? before num_data_lanes was
> checked first, then nr_of_link_frequencies, then 3 link_frequencies.

No reason. I'll change order like it was before in v3.

> > +
> > +		/* Check the number of MIPI CSI2 data lanes */
> > +		if (gc2145->ep.bus.mipi_csi2.num_data_lanes != 2) {
> > +			dev_err(dev, "only 2 data lanes are currently supported\n");
> > +			ret = -EINVAL;
> > +			goto out;
> > +		}
> > +
> > +		if (gc2145->ep.nr_of_link_frequencies != 3 ||
> > +			gc2145->ep.link_frequencies[0] != GC2145_640_480_LINKFREQ ||
> > +			gc2145->ep.link_frequencies[1] != GC2145_1280_720_LINKFREQ ||
> > +			gc2145->ep.link_frequencies[2] != GC2145_1600_1200_LINKFREQ) {
> 
> alignment
Thanks.

> 
> > +			dev_err(dev, "Invalid link-frequencies provided\n");
> > +			ret = -EINVAL;
> > +			goto out;
> > +		}
> > +		break;
> > +
> 
> no newline here
ok

-- 
Best regards,
Andrey Skvortsov