On Tue, 28 Jun 2022, Thomas Zimmermann <tzimmermann@xxxxxxx> 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@xxxxxxxxxx>
>> ---
>> 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
