Re: [PATCH v2 08/68] drm/connector: Introduce drmm_connector_init_with_ddc

[Jani Nikula <jani.nikula@linux.intel.com>]

On Tue, 28 Jun 2022, Thomas Zimmermann <tzimmermann@suse.de> wrote:
> Hi
>
> Am 22.06.22 um 16:31 schrieb Maxime Ripard:
>> Let's create a DRM-managed variant of drm_connector_init_with_ddc that will
>> take care of an action of the connector cleanup.
>> 
>> Signed-off-by: Maxime Ripard <maxime@cerno.tech>
>> ---
>>   drivers/gpu/drm/drm_connector.c | 74 ++++++++++++++++++++++++++++-----
>>   include/drm/drm_connector.h     |  5 +++
>>   2 files changed, 69 insertions(+), 10 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
>> index 0fec2d87178f..076ca247c6d0 100644
>> --- a/drivers/gpu/drm/drm_connector.c
>> +++ b/drivers/gpu/drm/drm_connector.c
>> @@ -354,6 +354,30 @@ int drm_connector_init(struct drm_device *dev,
>>   }
>>   EXPORT_SYMBOL(drm_connector_init);
>>   
>> +typedef int (*connector_init_t)(struct drm_device *dev,
>> +				struct drm_connector *connector,
>> +				const struct drm_connector_funcs *funcs,
>> +				int connector_type);
>> +
>> +static int __drm_connector_init_with_ddc(struct drm_device *dev,
>> +					 struct drm_connector *connector,
>> +					 connector_init_t init_func,
>> +					 const struct drm_connector_funcs *funcs,
>> +					 int connector_type,
>> +					 struct i2c_adapter *ddc)
>
> Back in the days when drm_connector_init_with_ddc() was added, there was 
> a small controversy about whether we should simply extend the regular 
> drm_connector_init() to support the extra ddc argument. That would have 
> meant a lot of churn, but the idea was probably sound.
>
> Maybe it would be better to provide a single function 
> drmm_connector_init() that receives a ddc argument. If the argument is 
> NULL, no DDC channel would be set. This would make 
> drmm_connector_init_with_ddc() unnnecessary.

FWIW I really dislike the "_with_ddc" variant as a function name. Like,
what if you add another thing you'd need to pass in at init. Add
functions "_with_ddc_and_foo" and "_with_foo", and so on.

I'd take the churn to convert to drm_connector_init() and
drmm_connector_init() only.

BR,
Jani.


>
> Best regards
> Thomas
>
>> +{
>> +	int ret;
>> +
>> +	ret = init_func(dev, connector, funcs, connector_type);
>> +	if (ret)
>> +		return ret;
>> +
>> +	/* provide ddc symlink in sysfs */
>> +	connector->ddc = ddc;
>> +
>> +	return ret;
>> +}
>> +
>>   /**
>>    * drm_connector_init_with_ddc - Init a preallocated connector
>>    * @dev: DRM device
>> @@ -371,6 +395,10 @@ EXPORT_SYMBOL(drm_connector_init);
>>    *
>>    * Ensures that the ddc field of the connector is correctly set.
>>    *
>> + * Note: consider using drmm_connector_init_with_ddc() instead of
>> + * drm_connector_init_with_ddc() to let the DRM managed resource
>> + * infrastructure take care of cleanup and deallocation.
>> + *
>>    * Returns:
>>    * Zero on success, error code on failure.
>>    */
>> @@ -380,16 +408,9 @@ int drm_connector_init_with_ddc(struct drm_device *dev,
>>   				int connector_type,
>>   				struct i2c_adapter *ddc)
>>   {
>> -	int ret;
>> -
>> -	ret = drm_connector_init(dev, connector, funcs, connector_type);
>> -	if (ret)
>> -		return ret;
>> -
>> -	/* provide ddc symlink in sysfs */
>> -	connector->ddc = ddc;
>> -
>> -	return ret;
>> +	return __drm_connector_init_with_ddc(dev, connector,
>> +					     drm_connector_init,
>> +					     funcs, connector_type, ddc);
>>   }
>>   EXPORT_SYMBOL(drm_connector_init_with_ddc);
>>   
>> @@ -443,6 +464,39 @@ int drmm_connector_init(struct drm_device *dev,
>>   }
>>   EXPORT_SYMBOL(drmm_connector_init);
>>   
>> +/**
>> + * drmm_connector_init_with_ddc - Init a preallocated connector
>> + * @dev: DRM device
>> + * @connector: the connector to init
>> + * @funcs: callbacks for this connector
>> + * @connector_type: user visible type of the connector
>> + * @ddc: pointer to the associated ddc adapter
>> + *
>> + * Initialises a preallocated connector. Connectors should be
>> + * subclassed as part of driver connector objects.
>> + *
>> + * Cleanup is automatically handled with a call to
>> + * drm_connector_cleanup() in a DRM-managed action.
>> + *
>> + * The connector structure should be allocated with drmm_kzalloc().
>> + *
>> + * Ensures that the ddc field of the connector is correctly set.
>> + *
>> + * Returns:
>> + * Zero on success, error code on failure.
>> + */
>> +int drmm_connector_init_with_ddc(struct drm_device *dev,
>> +				 struct drm_connector *connector,
>> +				 const struct drm_connector_funcs *funcs,
>> +				 int connector_type,
>> +				 struct i2c_adapter *ddc)
>> +{
>> +	return __drm_connector_init_with_ddc(dev, connector,
>> +					     drmm_connector_init,
>> +					     funcs, connector_type, ddc);
>> +}
>> +EXPORT_SYMBOL(drmm_connector_init_with_ddc);
>> +
>>   /**
>>    * drm_connector_attach_edid_property - attach edid property.
>>    * @connector: the connector
>> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
>> index 35a6b6e944b7..2565541f2c10 100644
>> --- a/include/drm/drm_connector.h
>> +++ b/include/drm/drm_connector.h
>> @@ -1676,6 +1676,11 @@ int drmm_connector_init(struct drm_device *dev,
>>   			struct drm_connector *connector,
>>   			const struct drm_connector_funcs *funcs,
>>   			int connector_type);
>> +int drmm_connector_init_with_ddc(struct drm_device *dev,
>> +				 struct drm_connector *connector,
>> +				 const struct drm_connector_funcs *funcs,
>> +				 int connector_type,
>> +				 struct i2c_adapter *ddc);
>>   void drm_connector_attach_edid_property(struct drm_connector *connector);
>>   int drm_connector_register(struct drm_connector *connector);
>>   void drm_connector_unregister(struct drm_connector *connector);

-- 
Jani Nikula, Intel Open Source Graphics Center