[v4,14/23] drm/tilcdc: Provide ddc symlink in connector sysfs directory

Submitted by Andrzej Pietrasiewicz on July 11, 2019, 11:26 a.m.

Details

Message ID d1d415022c598fb7acd033f0f322dd67250adaa9.1562843413.git.andrzej.p@collabora.com
State New
Headers show
Series "Associate ddc adapters with connectors" ( rev: 1 ) in Freedreno

Not browsing as part of any series.

Commit Message

Andrzej Pietrasiewicz July 11, 2019, 11:26 a.m.
Use the ddc pointer provided by the generic connector.

Signed-off-by: Andrzej Pietrasiewicz <andrzej.p@collabora.com>
---
 drivers/gpu/drm/tilcdc/tilcdc_tfp410.c | 1 +
 1 file changed, 1 insertion(+)

Patch hide | download patch | download mbox

diff --git a/drivers/gpu/drm/tilcdc/tilcdc_tfp410.c b/drivers/gpu/drm/tilcdc/tilcdc_tfp410.c
index 62d014c20988..c373edb95666 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_tfp410.c
+++ b/drivers/gpu/drm/tilcdc/tilcdc_tfp410.c
@@ -219,6 +219,7 @@  static struct drm_connector *tfp410_connector_create(struct drm_device *dev,
 	tfp410_connector->mod = mod;
 
 	connector = &tfp410_connector->base;
+	connector->ddc = mod->i2c;
 
 	drm_connector_init(dev, connector, &tfp410_connector_funcs,
 			DRM_MODE_CONNECTOR_DVID);

Comments

Hi Andrzej

On Thu, Jul 11, 2019 at 01:26:41PM +0200, Andrzej Pietrasiewicz wrote:
> Use the ddc pointer provided by the generic connector.
> 
> Signed-off-by: Andrzej Pietrasiewicz <andrzej.p@collabora.com>
> ---
>  drivers/gpu/drm/tilcdc/tilcdc_tfp410.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/gpu/drm/tilcdc/tilcdc_tfp410.c b/drivers/gpu/drm/tilcdc/tilcdc_tfp410.c
> index 62d014c20988..c373edb95666 100644
> --- a/drivers/gpu/drm/tilcdc/tilcdc_tfp410.c
> +++ b/drivers/gpu/drm/tilcdc/tilcdc_tfp410.c
> @@ -219,6 +219,7 @@ static struct drm_connector *tfp410_connector_create(struct drm_device *dev,
>  	tfp410_connector->mod = mod;
>  
>  	connector = &tfp410_connector->base;
> +	connector->ddc = mod->i2c;
>  
>  	drm_connector_init(dev, connector, &tfp410_connector_funcs,
>  			DRM_MODE_CONNECTOR_DVID);

When reading this code, it looks strange that we set connector->ddc
*before* the call to init the connector.
One could risk that drm_connector_init() used memset(..) to clear all
fields or so, and it would break this order.
As it is today the code works as I read it.

	Sam
Hi Sam,

W dniu 23.07.2019 o 11:05, Sam Ravnborg pisze:
> Hi Andrzej
> 
> On Thu, Jul 11, 2019 at 01:26:41PM +0200, Andrzej Pietrasiewicz wrote:
>> Use the ddc pointer provided by the generic connector.
>>
>> Signed-off-by: Andrzej Pietrasiewicz <andrzej.p@collabora.com>
>> ---
>>   drivers/gpu/drm/tilcdc/tilcdc_tfp410.c | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/gpu/drm/tilcdc/tilcdc_tfp410.c b/drivers/gpu/drm/tilcdc/tilcdc_tfp410.c
>> index 62d014c20988..c373edb95666 100644
>> --- a/drivers/gpu/drm/tilcdc/tilcdc_tfp410.c
>> +++ b/drivers/gpu/drm/tilcdc/tilcdc_tfp410.c
>> @@ -219,6 +219,7 @@ static struct drm_connector *tfp410_connector_create(struct drm_device *dev,
>>   	tfp410_connector->mod = mod;
>>   
>>   	connector = &tfp410_connector->base;
>> +	connector->ddc = mod->i2c;
>>   
>>   	drm_connector_init(dev, connector, &tfp410_connector_funcs,
>>   			DRM_MODE_CONNECTOR_DVID);
> 
> When reading this code, it looks strange that we set connector->ddc
> *before* the call to init the connector.
> One could risk that drm_connector_init() used memset(..) to clear all
> fields or so, and it would break this order.

I verified the code of drm_connector_init() and cannot find any memset()
invocations there. What is your actual concern?

Andrzej
Hi Andrej.

On Tue, Jul 23, 2019 at 02:44:50PM +0200, Andrzej Pietrasiewicz wrote:
> Hi Sam,
> 
> W dniu 23.07.2019 o 11:05, Sam Ravnborg pisze:
> > Hi Andrzej
> > 
> > On Thu, Jul 11, 2019 at 01:26:41PM +0200, Andrzej Pietrasiewicz wrote:
> > > Use the ddc pointer provided by the generic connector.
> > > 
> > > Signed-off-by: Andrzej Pietrasiewicz <andrzej.p@collabora.com>
> > > ---
> > >   drivers/gpu/drm/tilcdc/tilcdc_tfp410.c | 1 +
> > >   1 file changed, 1 insertion(+)
> > > 
> > > diff --git a/drivers/gpu/drm/tilcdc/tilcdc_tfp410.c b/drivers/gpu/drm/tilcdc/tilcdc_tfp410.c
> > > index 62d014c20988..c373edb95666 100644
> > > --- a/drivers/gpu/drm/tilcdc/tilcdc_tfp410.c
> > > +++ b/drivers/gpu/drm/tilcdc/tilcdc_tfp410.c
> > > @@ -219,6 +219,7 @@ static struct drm_connector *tfp410_connector_create(struct drm_device *dev,
> > >   	tfp410_connector->mod = mod;
> > >   	connector = &tfp410_connector->base;
> > > +	connector->ddc = mod->i2c;
> > >   	drm_connector_init(dev, connector, &tfp410_connector_funcs,
> > >   			DRM_MODE_CONNECTOR_DVID);
> > 
> > When reading this code, it looks strange that we set connector->ddc
> > *before* the call to init the connector.
> > One could risk that drm_connector_init() used memset(..) to clear all
> > fields or so, and it would break this order.
> 
> I verified the code of drm_connector_init() and cannot find any memset()
> invocations there. What is your actual concern?
My concern is that drm_connector_init() maybe sometime in the future
will init all fileds in drm_connector, so we loose any assingments
done to drm_connector from *before* we called the init function.

Moving the assignment to after drm_connector_init() would not
let us depend on the actual implmentation of drm_connector_init().

	Sam

Hi Thomas,

W dniu 24.07.2019 o 10:01, Thomas Zimmermann pisze:
> Hi
> 


> 
> I think this echoes my concern about the implicit order of operation. It
> seems too easy to get this wrong. If you don't want to add an additional
> interface for setting the ddc field, why not add a dedicated initializer
> function that sets the ddc field? Something like this.
> 
> int drm_connector_init_with_ddc(connector, funcs, ..., ddc)
> {
> 	ret = drm_connector_init(connector, funcs, ...);
> 	if (ret)
> 		return ret;
> 
> 	if (!ddc)
> 		return 0;
> 
> 	connector->ddc = ddc;
> 	/* set up sysfs */
> 
> 	return 0;
> }
> 

True. I will send a v5 soon.

Thanks,

Andrzej