mbox series

[v3,0/2] media: gc2145: add basic dvp bus support

Message ID 20240504164115.64603-1-andrej.skvortzov@gmail.com
Headers show
Series media: gc2145: add basic dvp bus support | expand

Message

Andrey Skvortsov May 4, 2024, 4:41 p.m. UTC 
Tested on PinePhone with libcamera-based application GNOME screenshot (45.2).

Andrey Skvortsov (2):
  dt-bindings: media: i2c: add galaxycore,gc2145 DVP bus support
  media: gc2145: implement basic dvp bus support

v3:
 - driver:
  - fixed formatting
  - added short commit description
  - removed unused defines, except GC2145_SYNC_MODE_OPCLK_GATE

v2:
 - bindings:
   - add required bus-type property
   - conditionally require link-frequency property based on bus-type
   - add DVP properties with their default values

 - driver:
  - fix fwnode parsing
  - remove gc2145_is_csi2
  - fix error message for unsupported bus-type

 .../bindings/media/i2c/galaxycore,gc2145.yaml |  65 +++++++++-
 drivers/media/i2c/gc2145.c                    | 112 ++++++++++++++----
 2 files changed, 150 insertions(+), 27 deletions(-)

Comments

Krzysztof Kozlowski May 5, 2024, 8:21 a.m. UTC | #1 
On 04/05/2024 18:41, Andrey Skvortsov wrote:
> That was tested on PinePhone with libcamera-based GNOME
> screenshot (45.2).

This tells me nothing why and what you are doing...


Best regards,
Krzysztof
Sakari Ailus May 22, 2024, 12:21 p.m. UTC | #2 
Hi Andrey,

Thanks for the update.

On Sat, May 04, 2024 at 07:41:15PM +0300, Andrey Skvortsov wrote:
> That was tested on PinePhone with libcamera-based GNOME
> screenshot (45.2).
> 
> Signed-off-by: Andrey Skvortsov <andrej.skvortzov@gmail.com>
> ---
>  drivers/media/i2c/gc2145.c | 112 ++++++++++++++++++++++++++++---------
>  1 file changed, 86 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/media/i2c/gc2145.c b/drivers/media/i2c/gc2145.c
> index bef7b0e056a8..9808bd0ab6ae 100644
> --- 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)
>  #define GC2145_SYNC_MODE_COL_SWITCH	BIT(4)
>  #define GC2145_SYNC_MODE_ROW_SWITCH	BIT(5)
>  #define GC2145_REG_BYPASS_MODE	CCI_REG8(0x89)
> @@ -598,6 +602,7 @@ struct gc2145 {
>  	struct v4l2_subdev sd;
>  	struct media_pad pad;
>  
> +	struct v4l2_fwnode_endpoint ep; /* the parsed DT endpoint info */
>  	struct regmap *regmap;
>  	struct clk *xclk;
>  
> @@ -773,6 +778,36 @@ 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)
> +{
> +	int ret = 0;
> +	u64 sync_mode;
> +	int flags = gc2145->ep.bus.parallel.flags;
> +
> +	ret = cci_read(gc2145->regmap, GC2145_REG_SYNC_MODE, &sync_mode, NULL);
> +	if (ret)
> +		return ret;
> +
> +	sync_mode &= ~(GC2145_SYNC_MODE_VSYNC_POL |
> +		       GC2145_SYNC_MODE_HSYNC_POL |
> +		       GC2145_SYNC_MODE_OPCLK_POL);
> +
> +	if (flags & V4L2_MBUS_VSYNC_ACTIVE_LOW)
> +		sync_mode |= GC2145_SYNC_MODE_VSYNC_POL;
> +
> +	if (flags & V4L2_MBUS_HSYNC_ACTIVE_LOW)
> +		sync_mode |= GC2145_SYNC_MODE_HSYNC_POL;
> +
> +	if (flags & V4L2_MBUS_PCLK_SAMPLE_FALLING)
> +		sync_mode |= GC2145_SYNC_MODE_OPCLK_POL;
> +
> +	cci_write(gc2145->regmap, GC2145_REG_SYNC_MODE, sync_mode, &ret);
> +	cci_write(gc2145->regmap, GC2145_REG_PAD_IO, 0x0f, &ret);
> +
> +	return ret;
> +}
> +
>  static const struct cci_reg_sequence gc2145_common_mipi_regs[] = {
>  	{GC2145_REG_PAGE_SELECT, 0x03},
>  	{GC2145_REG_DPHY_ANALOG_MODE1, GC2145_DPHY_MODE_PHY_CLK_EN |
> @@ -895,10 +930,13 @@ static int gc2145_start_streaming(struct gc2145 *gc2145,
>  		goto err_rpm_put;
>  	}
>  
> -	/* Perform MIPI specific configuration */
> -	ret = gc2145_config_mipi_mode(gc2145, gc2145_format);
> +	/* Perform interface specific configuration */
> +	if (gc2145->ep.bus_type == V4L2_MBUS_CSI2_DPHY)
> +		ret = gc2145_config_mipi_mode(gc2145, gc2145_format);
> +	else
> +		ret = gc2145_config_dvp_mode(gc2145, gc2145_format);
>  	if (ret) {
> -		dev_err(&client->dev, "%s failed to write mipi conf\n",
> +		dev_err(&client->dev, "%s failed to write interface conf\n",
>  			__func__);
>  		goto err_rpm_put;
>  	}
> @@ -924,6 +962,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 +1274,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
> -	};
> +	struct v4l2_subdev *sd = dev_get_drvdata(dev);
> +	struct gc2145 *gc2145 = to_gc2145(sd);

As the bindings default to bus-type 4, you should reflect that here. I.e.
try parsing the endpoint with bus_type set to 4 first before setting it to
V4L2_MBUS_UNKNOWN.

You'll also need to set the parallel defaults here before parsing the
endpoint.

>  	int ret;
>  
>  	endpoint = fwnode_graph_get_next_endpoint(dev_fwnode(dev), NULL);
> @@ -1244,36 +1284,55 @@ static int gc2145_check_hwcfg(struct device *dev)
>  		return -EINVAL;
>  	}
>  
> -	ret = v4l2_fwnode_endpoint_alloc_parse(endpoint, &ep_cfg);
> +	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 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;
> +		}
> +
> +		/* 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;
> +		}
> +
> +		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) {

I guess this works as long as all the ghree frequencies are what the driver
expects but the driver implementation could be improved. Nearly all other
drivers use the available frequencies only.

> +			dev_err(dev, "Invalid link-frequencies provided\n");
> +			ret = -EINVAL;
> +			goto out;
> +		}
> +		break;
> +	case V4L2_MBUS_PARALLEL:
> +		break;
> +	default:
> +		dev_err(dev, "unsupported bus type %u\n", gc2145->ep.bus_type);
>  		ret = -EINVAL;
>  		goto out;
>  	}
>  
> -	if (ep_cfg.nr_of_link_frequencies != 3 ||
> -	    ep_cfg.link_frequencies[0] != GC2145_640_480_LINKFREQ ||
> -	    ep_cfg.link_frequencies[1] != GC2145_1280_720_LINKFREQ ||
> -	    ep_cfg.link_frequencies[2] != GC2145_1600_1200_LINKFREQ) {
> -		dev_err(dev, "Invalid link-frequencies provided\n");
> -		ret = -EINVAL;
> -	}
> +	return 0;
>  
>  out:
> -	v4l2_fwnode_endpoint_free(&ep_cfg);
> -
> +	v4l2_fwnode_endpoint_free(&gc2145->ep);

A newline would be nice here.

>  	return ret;
>  }
>  
> @@ -1421,6 +1480,7 @@ static void gc2145_remove(struct i2c_client *client)
>  	if (!pm_runtime_status_suspended(&client->dev))
>  		gc2145_power_off(&client->dev);
>  	pm_runtime_set_suspended(&client->dev);
> +	v4l2_fwnode_endpoint_free(&gc2145->ep);
>  }
>  
>  static const struct of_device_id gc2145_dt_ids[] = {