Received-SPF: pass (google.com: domain of linux-kernel+bounces-75933-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Date: Thu, 22 Feb 2024 08:20:32 +0300
From: Andrey Skvortsov <andrej.skvortzov@gmail.com>
To: Sakari Ailus <sakari.ailus@linux.intel.com>
Cc: Alain Volmat <alain.volmat@foss.st.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 2/2] media: gc2145: implement basic dvp bus support
Message-ID: <ZdbZoF_uUPga3uek@skv.local>
Mail-Followup-To: Andrey Skvortsov <andrej.skvortzov@gmail.com>,
	Sakari Ailus <sakari.ailus@linux.intel.com>,
	Alain Volmat <alain.volmat@foss.st.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: <20240217220308.594883-1-andrej.skvortzov@gmail.com>
 <20240217220308.594883-3-andrej.skvortzov@gmail.com>
 <ZdSFGaJ9qnayYI5C@kekkonen.localdomain>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <ZdSFGaJ9qnayYI5C@kekkonen.localdomain>

Hi Sakari,

On 24-02-20 10:55, Sakari Ailus wrote:
> Hi Andrey,
> 
> On Sun, Feb 18, 2024 at 01:03:08AM +0300, Andrey Skvortsov wrote:
> > Tested on PinePhone with libcamera-based GNOME screenshot.
> > 
> > Signed-off-by: Andrey Skvortsov <andrej.skvortzov@gmail.com>
> > ---
> >  drivers/media/i2c/gc2145.c | 117 ++++++++++++++++++++++++++++---------
> >  1 file changed, 90 insertions(+), 27 deletions(-)
> > 
> >  
> > @@ -612,6 +623,11 @@ struct gc2145 {
> >  	const struct gc2145_mode *mode;
> >  };
> >  
> > +static inline bool gc2145_is_csi2(const struct gc2145 *gc2145)
> > +{
> > +	return gc2145->ep.bus_type == V4L2_MBUS_CSI2_DPHY;
> 
> This is used in a single place. Could you move this comparison there?
Yes, I'll do.

> 
> > +}
> > +

> >  	}
> > @@ -924,6 +975,9 @@ static void gc2145_stop_streaming(struct gc2145 *gc2145)
> >  			GC2145_CSI2_MODE_EN | GC2145_CSI2_MODE_MIPI_EN, 0,
> >  			&ret);
> >  	cci_write(gc2145->regmap, GC2145_REG_PAGE_SELECT, 0x00, &ret);
> > +
> > +	/* Disable dvp streaming */
> > +	cci_write(gc2145->regmap, GC2145_REG_PAD_IO, 0x00, &ret);
> >  	if (ret)
> >  		dev_err(&client->dev, "%s failed to write regs\n", __func__);
> >  
> > @@ -1233,9 +1287,8 @@ static int gc2145_init_controls(struct gc2145 *gc2145)
> >  static int gc2145_check_hwcfg(struct device *dev)
> >  {
> >  	struct fwnode_handle *endpoint;
> > -	struct v4l2_fwnode_endpoint ep_cfg = {
> > -		.bus_type = V4L2_MBUS_CSI2_DPHY
> > -	};
> 
> First try D-PHY and if that fails, then try PARALLEL.
> 
> > +	struct v4l2_subdev *sd = dev_get_drvdata(dev);
> > +	struct gc2145 *gc2145 = to_gc2145(sd);
> >  	int ret;
> >  
> >  	endpoint = fwnode_graph_get_next_endpoint(dev_fwnode(dev), NULL);
> > @@ -1244,36 +1297,46 @@ static int gc2145_check_hwcfg(struct device *dev)
> >  		return -EINVAL;
> >  	}
> >  
> > -	ret = v4l2_fwnode_endpoint_alloc_parse(endpoint, &ep_cfg);
> > +	ret = v4l2_fwnode_endpoint_parse(endpoint, &gc2145->ep);
> 
> You won't have any link frequencies available with this change.
Thanks for catching this up. I'll fix that.

> 
> >  	fwnode_handle_put(endpoint);
> >  	if (ret)
> >  		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;
> > -	}
> > +	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;
> > +		}
> > +
> > +		/* 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) {
> > +			dev_err(dev, "Invalid link-frequencies provided\n");
> > +			ret = -EINVAL;
> > +			goto out;
> > +		}
> > +		break;
> >  
> > -	/* 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");
> > +	case V4L2_MBUS_PARALLEL:
> > +		break;
> > +	default:
> > +		dev_err(dev, "unsupported bus type %u\n",
> > +			gc2145->ep.bus_type);
> 
> Fits on the previous line.
I'll change this in v2.


-- 
Best regards,
Andrey Skvortsov