Re: [RFC 8/9] drm/client: Add drm_client_init_from_id() and drm_client_modeset_set()

["Noralf Trønnes" <noralf@tronnes.org>]

Den 17.02.2020 10.38, skrev Daniel Vetter:
> On Sun, Feb 16, 2020 at 06:21:16PM +0100, Noralf Trønnes wrote:
>> drm_client_init_from_id() provides a way for clients to add a client based
>> on the minor. drm_client_modeset_set() provides a way to set the modeset
>> for clients that handles connectors and display mode on their own.
>>
>> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
>> ---
>>  drivers/gpu/drm/drm_client.c         | 37 ++++++++++++++++++++
>>  drivers/gpu/drm/drm_client_modeset.c | 52 ++++++++++++++++++++++++++++
>>  include/drm/drm_client.h             |  4 +++
>>  3 files changed, 93 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_client.c b/drivers/gpu/drm/drm_client.c
>> index d9a2e3695525..dbd73fe8d987 100644
>> --- a/drivers/gpu/drm/drm_client.c
>> +++ b/drivers/gpu/drm/drm_client.c
>> @@ -112,6 +112,43 @@ int drm_client_init(struct drm_device *dev, struct drm_client_dev *client,
>>  }
>>  EXPORT_SYMBOL(drm_client_init);
>>  
>> +/**
>> + * drm_client_init_from_id - Initialise a DRM client
>> + * @minor_id: DRM minor id
>> + * @client: DRM client
>> + * @name: Client name
>> + * @funcs: DRM client functions (optional)
>> + *
>> + * This function looks up the drm_device using the minor id and initializes the client.
>> + * It also registeres the client to avoid a possible race with DRM device unregister.
> 
> I think another sentence here would be good, explaining that this is for
> drivers outside of drm to expose a specific drm driver in some fashion,
> and just outright mention your use-case as an example.
> 
> I'm also not sure whether lookup-by-minor is the greatest thing for
> kernel-internal lookups like this, maybe Greg has some idea?
> 

What alternatives do you see? Parent device name?

I did a scan to see what others are doing, and most have a consumer name
lookup on the struct device (Device Tree or lookup tables):

struct reset_control *__reset_control_get(struct device *dev, const char
*id,
					  int index, bool shared,
					  bool optional, bool acquired);

struct iio_channel *iio_channel_get(struct device *dev,
				    const char *consumer_channel);

struct regulator *__must_check regulator_get(struct device *dev,
					     const char *id);

struct pwm_device *pwm_get(struct device *dev, const char *con_id)

struct gpio_desc *__must_check gpiod_get(struct device *dev,
					 const char *con_id,
					 enum gpiod_flags flags);

SPI and I2C use the bus index as lookup:

extern struct i2c_adapter *i2c_get_adapter(int nr);

extern struct spi_controller *spi_busnum_to_master(u16 busnum);


Noralf.

>> + *
>> + * See drm_client_init() and drm_client_register().
>> + *
>> + * Returns:
>> + * Zero on success or negative error code on failure.
>> + */
>> +int drm_client_init_from_id(unsigned int minor_id, struct drm_client_dev *client,
>> +			    const char *name, const struct drm_client_funcs *funcs)
>> +{
>> +	struct drm_minor *minor;
>> +	int ret;
>> +
>> +	minor = drm_minor_acquire(minor_id);
>> +	if (IS_ERR(minor))
>> +		return PTR_ERR(minor);
>> +
>> +	mutex_lock(&minor->dev->clientlist_mutex);
>> +	ret = drm_client_init(minor->dev, client, name, funcs);
>> +	if (!ret)
>> +		list_add(&client->list, &minor->dev->clientlist);
>> +	mutex_unlock(&minor->dev->clientlist_mutex);
>> +
>> +	drm_minor_release(minor);
>> +
>> +	return ret;
>> +}
>> +EXPORT_SYMBOL(drm_client_init_from_id);
>> +
>>  /**
>>   * drm_client_register - Register client
>>   * @client: DRM client
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel