Re: [PATCH v3 2/2] media: i2c: Add driver for Sony IMX219 sensor

[Sakari Ailus <sakari.ailus@linux.intel.com>]

Hi Dave,

On Fri, Jan 10, 2020 at 11:09:15PM +0300, Andrey Konovalov wrote:
> +/* Power/clock management functions */
> +static int imx219_power_on(struct device *dev)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct v4l2_subdev *sd = i2c_get_clientdata(client);
> +	struct imx219 *imx219 = to_imx219(sd);
> +	int ret;
> +
> +	ret = regulator_bulk_enable(IMX219_NUM_SUPPLIES,
> +				    imx219->supplies);
> +	if (ret) {
> +		dev_err(&client->dev, "%s: failed to enable regulators\n",
> +			__func__);
> +		return ret;
> +	}
> +
> +	ret = clk_prepare_enable(imx219->xclk);
> +	if (ret) {
> +		dev_err(&client->dev, "%s: failed to enable clock\n",
> +			__func__);
> +		goto reg_off;
> +	}
> +
> +	gpiod_set_value_cansleep(imx219->reset_gpio, 1);
> +	msleep(IMX219_XCLR_DELAY_MS);
> +
> +	return 0;
> +reg_off:
> +	regulator_bulk_disable(IMX219_NUM_SUPPLIES, imx219->supplies);
> +	return ret;
> +}
> +
> +static int imx219_power_off(struct device *dev)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct v4l2_subdev *sd = i2c_get_clientdata(client);
> +	struct imx219 *imx219 = to_imx219(sd);
> +
> +	gpiod_set_value_cansleep(imx219->reset_gpio, 0);

The polarity of the reset GPIO appears to be wrong above. Given it works
somewhere (arch/arm64/boot/dts/ti/k3-am62x-sk-csi2-imx219.dtso), the
existing DTS files have it wrong, too. The bindings still appear to
document it correctly.

Laurent confirmed xcrl isn't controllable in the RPi imx219 camera module.

How about fixing this? Currently correctly written DTBs including imx219
won't work.

I noticed this while fixing the power sequences in this and a few other
drivers.

> +	regulator_bulk_disable(IMX219_NUM_SUPPLIES, imx219->supplies);
> +	clk_disable_unprepare(imx219->xclk);
> +
> +	return 0;
> +}

...

> +static int imx219_probe(struct i2c_client *client,
> +			const struct i2c_device_id *id)
> +{
> +	struct device *dev = &client->dev;
> +	struct fwnode_handle *endpoint;
> +	struct imx219 *imx219;
> +	int ret;
> +
> +	imx219 = devm_kzalloc(&client->dev, sizeof(*imx219), GFP_KERNEL);
> +	if (!imx219)
> +		return -ENOMEM;
> +
> +	imx219->dev = dev;
> +
> +	v4l2_i2c_subdev_init(&imx219->sd, client, &imx219_subdev_ops);
> +
> +	/* Get CSI2 bus config */
> +	endpoint = fwnode_graph_get_next_endpoint(dev_fwnode(&client->dev),
> +						  NULL);
> +	if (!endpoint) {
> +		dev_err(dev, "endpoint node not found\n");
> +		return -EINVAL;
> +	}
> +
> +	ret = v4l2_fwnode_endpoint_parse(endpoint, &imx219->ep);
> +	fwnode_handle_put(endpoint);
> +	if (ret) {
> +		dev_err(dev, "could not parse endpoint\n");
> +		return ret;
> +	}
> +
> +	/* Check the number of MIPI CSI2 data lanes */
> +	if (imx219->ep.bus_type != V4L2_MBUS_CSI2_DPHY ||
> +	    imx219->ep.bus.mipi_csi2.num_data_lanes != 2) {
> +		dev_err(dev, "only 2 data lanes are currently supported\n");
> +		return -EINVAL;
> +	}
> +
> +	/* Get system clock (xclk) */
> +	imx219->xclk = devm_clk_get(dev, NULL);
> +	if (IS_ERR(imx219->xclk)) {
> +		dev_err(dev, "failed to get xclk\n");
> +		return PTR_ERR(imx219->xclk);
> +	}
> +
> +	imx219->xclk_freq = clk_get_rate(imx219->xclk);
> +	if (imx219->xclk_freq != IMX219_XCLK_FREQ) {
> +		dev_err(dev, "xclk frequency not supported: %d Hz\n",
> +			imx219->xclk_freq);
> +		return -EINVAL;
> +	}
> +
> +	ret = imx219_get_regulators(imx219);
> +	if (ret) {
> +		dev_err(dev, "failed to get regulators\n");
> +		return ret;
> +	}
> +
> +	/* Request optional enable pin */
> +	imx219->reset_gpio = devm_gpiod_get_optional(dev, "reset",
> +						      GPIOD_OUT_HIGH);
> +
> +	/*
> +	 * The sensor must be powered for imx219_identify_module()
> +	 * to be able to read the CHIP_ID register
> +	 */
> +	ret = imx219_power_on(dev);
> +	if (ret)
> +		return ret;
> +
> +	ret = imx219_identify_module(imx219);
> +	if (ret)
> +		goto error_power_off;
> +
> +	/* Set default mode to max resolution */
> +	imx219->mode = &supported_modes[0];
> +
> +	ret = imx219_init_controls(imx219);
> +	if (ret)
> +		goto error_power_off;
> +
> +	/* Initialize subdev */
> +	imx219->sd.internal_ops = &imx219_internal_ops;
> +	imx219->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
> +	imx219->sd.entity.function = MEDIA_ENT_F_CAM_SENSOR;
> +
> +	/* Initialize source pad */
> +	imx219->pad.flags = MEDIA_PAD_FL_SOURCE;
> +
> +	ret = media_entity_pads_init(&imx219->sd.entity, 1, &imx219->pad);
> +	if (ret) {
> +		dev_err(dev, "failed to init entity pads: %d\n", ret);
> +		goto error_handler_free;
> +	}
> +
> +	ret = v4l2_async_register_subdev_sensor_common(&imx219->sd);
> +	if (ret < 0) {
> +		dev_err(dev, "failed to register sensor sub-device: %d\n", ret);
> +		goto error_media_entity;
> +	}
> +
> +	/* Enable runtime PM and turn off the device */
> +	pm_runtime_set_active(dev);
> +	pm_runtime_enable(dev);
> +	pm_runtime_idle(dev);
> +
> +	return 0;
> +
> +error_media_entity:
> +	media_entity_cleanup(&imx219->sd.entity);
> +
> +error_handler_free:
> +	imx219_free_controls(imx219);
> +
> +error_power_off:
> +	imx219_power_off(dev);
> +
> +	return ret;
> +}

-- 
Kind regards,

Sakari Ailus