question about drivers/gpu/drm/gma500/oaktrail_lvds.c
On Mon, 9 Jul 2012, Patrik Jakobsson wrote:
> On Sun, Jul 8, 2012 at 10:16 PM, Alan Cox wrote:
>> On Sun, 8 Jul 2012 10:39:43 +0200 (CEST)
>> Julia Lawall wrote:
>>
>>> In the function oaktrail_lvds_mode_set, I don't think that the following
>>> code makes any sense:
>>>
>>> /* Find the connector we're trying to set up */
>>> list_for_each_entry(connector, _config->connector_list, head)
>>> {
>>> if (!connector->encoder || connector->encoder->crtc !=
>>> crtc)
>>> continue;
>>> }
>>>
>>> if (!connector) {
>>> DRM_ERROR("Couldn't find connector when setting mode");
>>> return;
>>> }
>>>
>>> drm_connector_property_get_value(
>>> connector,
>>> dev->mode_config.scaling_mode_property,
>>> );
>>>
>>> The initial loop is a no-op, because it always continues. The test
>>> !connector can never be true, because at the end of a list_for_each_entry
>>> connector points to the list head, and calling
>>> drm_connector_property_get_value on the list head probably does not make
>>> sense.
>>
>> We test !connector->encoder rather than !connector ?
>
> It seems we should break on :
> if (connector->encoder && connector->encoder == encoder)
>
> Then do a check after list iteration:
> if (!connector || connector->encoder != encoder)
>DRM_ERROR("Couldn't find connector when setting mode");
Possible. The !connector is still not needed, but the overall logic seems
better.
julia
question about drivers/gpu/drm/gma500/oaktrail_lvds.c
On Sun, Jul 8, 2012 at 10:16 PM, Alan Cox wrote:
> On Sun, 8 Jul 2012 10:39:43 +0200 (CEST)
> Julia Lawall wrote:
>
>> In the function oaktrail_lvds_mode_set, I don't think that the following
>> code makes any sense:
>>
>> /* Find the connector we're trying to set up */
>> list_for_each_entry(connector, _config->connector_list, head) {
>> if (!connector->encoder || connector->encoder->crtc != crtc)
>> continue;
>> }
>>
>> if (!connector) {
>> DRM_ERROR("Couldn't find connector when setting mode");
>> return;
>> }
>>
>> drm_connector_property_get_value(
>> connector,
>> dev->mode_config.scaling_mode_property,
>> );
>>
>> The initial loop is a no-op, because it always continues. The test
>> !connector can never be true, because at the end of a list_for_each_entry
>> connector points to the list head, and calling
>> drm_connector_property_get_value on the list head probably does not make
>> sense.
>
> We test !connector->encoder rather than !connector ?
It seems we should break on :
if (connector->encoder && connector->encoder == encoder)
Then do a check after list iteration:
if (!connector || connector->encoder != encoder)
DRM_ERROR("Couldn't find connector when setting mode");
But my attention span is less than a jiffie right now... will look at it
with fresh eyes tomorrow.
-Patrik
Re: question about drivers/gpu/drm/gma500/oaktrail_lvds.c
On Sun, 8 Jul 2012, Alan Cox wrote:
On Sun, 8 Jul 2012 10:39:43 +0200 (CEST)
Julia Lawall julia.law...@lip6.fr wrote:
In the function oaktrail_lvds_mode_set, I don't think that the following
code makes any sense:
/* Find the connector we're trying to set up */
list_for_each_entry(connector, mode_config-connector_list, head) {
if (!connector-encoder || connector-encoder-crtc != crtc)
continue;
}
if (!connector) {
DRM_ERROR(Couldn't find connector when setting mode);
return;
}
drm_connector_property_get_value(
connector,
dev-mode_config.scaling_mode_property,
v);
The initial loop is a no-op, because it always continues. The test
!connector can never be true, because at the end of a list_for_each_entry
connector points to the list head, and calling
drm_connector_property_get_value on the list head probably does not make
sense.
We test !connector-encoder rather than !connector ?
There is a test of !connector-encoder inside the loop. But the whole
loop body is a no-op because of the continue. The only effect of the loop
is to set connector to the list head, which is the exit condition for the
loop.
The test of !connector is afterwards, but connector will not be NULL at
that point, it will point to the list head.
julia
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: question about drivers/gpu/drm/gma500/oaktrail_lvds.c
On Mon, 9 Jul 2012, Patrik Jakobsson wrote:
On Sun, Jul 8, 2012 at 10:16 PM, Alan Cox a...@lxorguk.ukuu.org.uk wrote:
On Sun, 8 Jul 2012 10:39:43 +0200 (CEST)
Julia Lawall julia.law...@lip6.fr wrote:
In the function oaktrail_lvds_mode_set, I don't think that the following
code makes any sense:
/* Find the connector we're trying to set up */
list_for_each_entry(connector, mode_config-connector_list, head) {
if (!connector-encoder || connector-encoder-crtc != crtc)
continue;
}
if (!connector) {
DRM_ERROR(Couldn't find connector when setting mode);
return;
}
drm_connector_property_get_value(
connector,
dev-mode_config.scaling_mode_property,
v);
The initial loop is a no-op, because it always continues. The test
!connector can never be true, because at the end of a list_for_each_entry
connector points to the list head, and calling
drm_connector_property_get_value on the list head probably does not make
sense.
We test !connector-encoder rather than !connector ?
It seems we should break on :
if (connector-encoder connector-encoder == encoder)
Then do a check after list iteration:
if (!connector || connector-encoder != encoder)
DRM_ERROR(Couldn't find connector when setting mode);
Possible. The !connector is still not needed, but the overall logic seems
better.
julia
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
question about drivers/gpu/drm/gma500/oaktrail_lvds.c
On Sun, 8 Jul 2012, Alan Cox wrote:
> On Sun, 8 Jul 2012 10:39:43 +0200 (CEST)
> Julia Lawall wrote:
>
>> In the function oaktrail_lvds_mode_set, I don't think that the following
>> code makes any sense:
>>
>> /* Find the connector we're trying to set up */
>> list_for_each_entry(connector, _config->connector_list, head) {
>> if (!connector->encoder || connector->encoder->crtc != crtc)
>> continue;
>> }
>>
>> if (!connector) {
>> DRM_ERROR("Couldn't find connector when setting mode");
>> return;
>> }
>>
>> drm_connector_property_get_value(
>> connector,
>> dev->mode_config.scaling_mode_property,
>> );
>>
>> The initial loop is a no-op, because it always continues. The test
>> !connector can never be true, because at the end of a list_for_each_entry
>> connector points to the list head, and calling
>> drm_connector_property_get_value on the list head probably does not make
>> sense.
>
> We test !connector->encoder rather than !connector ?
There is a test of !connector->encoder inside the loop. But the whole
loop body is a no-op because of the continue. The only effect of the loop
is to set connector to the list head, which is the exit condition for the
loop.
The test of !connector is afterwards, but connector will not be NULL at
that point, it will point to the list head.
julia
question about drivers/gpu/drm/gma500/oaktrail_lvds.c
On Sun, 8 Jul 2012 10:39:43 +0200 (CEST)
Julia Lawall wrote:
> In the function oaktrail_lvds_mode_set, I don't think that the following
> code makes any sense:
>
> /* Find the connector we're trying to set up */
> list_for_each_entry(connector, _config->connector_list, head) {
> if (!connector->encoder || connector->encoder->crtc != crtc)
> continue;
> }
>
> if (!connector) {
> DRM_ERROR("Couldn't find connector when setting mode");
> return;
> }
>
> drm_connector_property_get_value(
> connector,
> dev->mode_config.scaling_mode_property,
> );
>
> The initial loop is a no-op, because it always continues. The test
> !connector can never be true, because at the end of a list_for_each_entry
> connector points to the list head, and calling
> drm_connector_property_get_value on the list head probably does not make
> sense.
We test !connector->encoder rather than !connector ?
Alan
question about drivers/gpu/drm/gma500/oaktrail_lvds.c
In the function oaktrail_lvds_mode_set, I don't think that the following
code makes any sense:
/* Find the connector we're trying to set up */
list_for_each_entry(connector, _config->connector_list, head) {
if (!connector->encoder || connector->encoder->crtc != crtc)
continue;
}
if (!connector) {
DRM_ERROR("Couldn't find connector when setting mode");
return;
}
drm_connector_property_get_value(
connector,
dev->mode_config.scaling_mode_property,
);
The initial loop is a no-op, because it always continues. The test
!connector can never be true, because at the end of a list_for_each_entry
connector points to the list head, and calling
drm_connector_property_get_value on the list head probably does not make
sense.
julia
question about drivers/gpu/drm/gma500/oaktrail_lvds.c
In the function oaktrail_lvds_mode_set, I don't think that the following
code makes any sense:
/* Find the connector we're trying to set up */
list_for_each_entry(connector, mode_config-connector_list, head) {
if (!connector-encoder || connector-encoder-crtc != crtc)
continue;
}
if (!connector) {
DRM_ERROR(Couldn't find connector when setting mode);
return;
}
drm_connector_property_get_value(
connector,
dev-mode_config.scaling_mode_property,
v);
The initial loop is a no-op, because it always continues. The test
!connector can never be true, because at the end of a list_for_each_entry
connector points to the list head, and calling
drm_connector_property_get_value on the list head probably does not make
sense.
julia
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: question about drivers/gpu/drm/gma500/oaktrail_lvds.c
On Sun, 8 Jul 2012 10:39:43 +0200 (CEST)
Julia Lawall julia.law...@lip6.fr wrote:
In the function oaktrail_lvds_mode_set, I don't think that the following
code makes any sense:
/* Find the connector we're trying to set up */
list_for_each_entry(connector, mode_config-connector_list, head) {
if (!connector-encoder || connector-encoder-crtc != crtc)
continue;
}
if (!connector) {
DRM_ERROR(Couldn't find connector when setting mode);
return;
}
drm_connector_property_get_value(
connector,
dev-mode_config.scaling_mode_property,
v);
The initial loop is a no-op, because it always continues. The test
!connector can never be true, because at the end of a list_for_each_entry
connector points to the list head, and calling
drm_connector_property_get_value on the list head probably does not make
sense.
We test !connector-encoder rather than !connector ?
Alan
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: question about drivers/gpu/drm/gma500/oaktrail_lvds.c
On Sun, Jul 8, 2012 at 10:16 PM, Alan Cox a...@lxorguk.ukuu.org.uk wrote:
On Sun, 8 Jul 2012 10:39:43 +0200 (CEST)
Julia Lawall julia.law...@lip6.fr wrote:
In the function oaktrail_lvds_mode_set, I don't think that the following
code makes any sense:
/* Find the connector we're trying to set up */
list_for_each_entry(connector, mode_config-connector_list, head) {
if (!connector-encoder || connector-encoder-crtc != crtc)
continue;
}
if (!connector) {
DRM_ERROR(Couldn't find connector when setting mode);
return;
}
drm_connector_property_get_value(
connector,
dev-mode_config.scaling_mode_property,
v);
The initial loop is a no-op, because it always continues. The test
!connector can never be true, because at the end of a list_for_each_entry
connector points to the list head, and calling
drm_connector_property_get_value on the list head probably does not make
sense.
We test !connector-encoder rather than !connector ?
It seems we should break on :
if (connector-encoder connector-encoder == encoder)
Then do a check after list iteration:
if (!connector || connector-encoder != encoder)
DRM_ERROR(Couldn't find connector when setting mode);
But my attention span is less than a jiffie right now... will look at it
with fresh eyes tomorrow.
-Patrik
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
