Re: [PATCH 11/12] media: intel-cio2-bridge: Add a runtime-pm device-link between VCM and sensor

[Andy Shevchenko <andy@kernel.org>]

On Tue, Jun 27, 2023 at 07:56:41PM +0200, Hans de Goede wrote:
> In most cases when a VCM is used there is a single integrated module
> with the sensor + VCM + lens. This means that the sensor and VCM often
> share regulators and possibly also something like a powerdown pin.
> 
> In the ACPI tables this is modelled as a single ACPI device with
> multiple I2cSerialBus resources.
> 
> On atomisp devices the regulators and clks are modelled as ACPI
> power-resources, which are controlled by the (ACPI) power state
> of the sensor. So the sensor must be in D0 power state for the VCM
> to work.
> 
> To make this work add a device-link with DL_FLAG_PM_RUNTIME flag
> so that the sensor will automatically be runtime-resumed whenever
> the VCM is runtime-resumed.
> 
> This requires the probing of the VCM and thus the creation of the VCM
> I2C-client to be delayed till after the sensor driver has bound.
> 
> Move the instantiation of the VCM I2C-client to the v4l2_async_notifier
> bound op, so that it is done after the sensor driver has bound; and
> add code to add the device-link.
> 
> This fixes the problem with the shared ACPI power-resources on atomisp2
> and this avoids the need for VCM related workarounds on IPU3.
> 
> E.g. until now the dw9719 driver needed to get and control a Vsio
> (V sensor IO) regulator since that needs to be enabled to enable I2C
> pass-through on the PMIC on the sensor module. So the driver was
> controlling this regulator even though the actual dw9719 chip has no
> Vsio pin / power-plane.
> 
> This also removes the need for intel_cio2_bridge_init() to return
> -EPROBE_DEFER since the VCM is now instantiated later.

...

> +struct intel_cio2_bridge_instantiate_vcm_work_data {

Hmm... A bit long name :-)

> +	struct work_struct work;
> +	struct device *sensor;
>  	char name[16];
> +	struct i2c_board_info board_info;
> +};

...

> +	if (IS_ERR(vcm_client)) {
> +		dev_err(work->sensor, "Error instantiating VCM client: %ld\n",

or even %pe

> +			PTR_ERR(vcm_client));

...

> +out:

out_put_pm: ?

> +	pm_runtime_put(work->sensor);
> +	put_device(work->sensor);
> +	if (put_fwnode)
> +		fwnode_handle_put(work->board_info.fwnode);
> +	kfree(work);

...

> +int intel_cio2_bridge_instantiate_vcm(struct device *sensor)
> +{
> +	struct intel_cio2_bridge_instantiate_vcm_work_data *work;
> +	struct acpi_device *adev = ACPI_COMPANION(sensor);
> +	struct fwnode_handle *vcm_fwnode;
> +	struct i2c_client *vcm_client;
> +	char *sep;


I would rather split assignment, so below won't look a bit puzzling.

	struct acpi_device *adev;
	...
	adev = ACPI_COMPANION(sensor);

> +	if (!adev)
> +		return 0;
> +
> +	vcm_fwnode = fwnode_find_reference(dev_fwnode(sensor), "lens-focus", 0);
> +	if (IS_ERR(vcm_fwnode))
> +		return 0;
> +
> +	/* When reloading modules the client will already exist */
> +	vcm_client = i2c_find_device_by_fwnode(vcm_fwnode);
> +	if (vcm_client) {
> +		fwnode_handle_put(vcm_fwnode);
> +		put_device(&vcm_client->dev);
> +		return 0;
> +	}
> +
> +	work = kzalloc(sizeof(*work), GFP_KERNEL);
> +	if (!work) {
> +		fwnode_handle_put(vcm_fwnode);
> +		return -ENOMEM;
> +	}
> +
> +	INIT_WORK(&work->work, intel_cio2_bridge_instantiate_vcm_work);
> +	work->sensor = get_device(sensor);
> +	snprintf(work->name, sizeof(work->name), "%s-VCM",
> +		 acpi_dev_name(adev));
> +	work->board_info.dev_name = work->name;
> +	work->board_info.fwnode = vcm_fwnode;
> +	strscpy(work->board_info.type, fwnode_get_name(vcm_fwnode),
> +		I2C_NAME_SIZE);
> +	/* Strip "-<link>" postfix */
> +	sep = strchrnul(work->board_info.type, '-');
> +	*sep = 0;
> +
> +	queue_work(system_long_wq, &work->work);
> +
> +	return 0;
> +}

-- 
With Best Regards,
Andy Shevchenko