Hi Patrik

Am 02.06.22 um 09:42 schrieb Patrik Jakobsson:
> On Thu, Jun 2, 2022 at 9:25 AM Patrik Jakobsson
> <patrik.r.jakobsson@gmail.com> wrote:
>>
>> On Tue, May 31, 2022 at 1:15 PM Thomas Zimmermann <tzimmermann@suse.de> wrote:
>>>
>>> Expect the hardware to provide a DDC channel. Fail probing if its
>>> initialization fails.
>>>
>>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>>
>> It's funny how I just did the same thing to gma500. Great minds think alike ;)
>>
>> Reviewed-by: Patrik Jakobsson <patrik.r.jakobsson@gmail.com>
> 
> I just realized that the ast_i2c_chan is never freed. Could be fixed
> in a follow-up patch?

In ast_i2c_create(), there's a call to drmm_add_action_or_reset(), which 
sets up ast_i2c_release(), which in turn kfree's the memory. Unless I'm 
totally missing something here.

Best regards
Thomas

> 
>>
>>
>>> ---
>>>   drivers/gpu/drm/ast/ast_drv.h  |  2 --
>>>   drivers/gpu/drm/ast/ast_i2c.c  |  7 ++++---
>>>   drivers/gpu/drm/ast/ast_mode.c | 38 ++++++++++++++++------------------
>>>   3 files changed, 22 insertions(+), 25 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/ast/ast_drv.h b/drivers/gpu/drm/ast/ast_drv.h
>>> index 3055b0be7b67..2a55fc7303b9 100644
>>> --- a/drivers/gpu/drm/ast/ast_drv.h
>>> +++ b/drivers/gpu/drm/ast/ast_drv.h
>>> @@ -132,7 +132,6 @@ struct ast_i2c_chan {
>>>
>>>   struct ast_vga_connector {
>>>          struct drm_connector base;
>>> -       struct ast_i2c_chan *i2c;
>>>   };
>>>
>>>   static inline struct ast_vga_connector *
>>> @@ -143,7 +142,6 @@ to_ast_vga_connector(struct drm_connector *connector)
>>>
>>>   struct ast_sil164_connector {
>>>          struct drm_connector base;
>>> -       struct ast_i2c_chan *i2c;
>>>   };
>>>
>>>   static inline struct ast_sil164_connector *
>>> diff --git a/drivers/gpu/drm/ast/ast_i2c.c b/drivers/gpu/drm/ast/ast_i2c.c
>>> index 93e91c36d649..1d039ff1396e 100644
>>> --- a/drivers/gpu/drm/ast/ast_i2c.c
>>> +++ b/drivers/gpu/drm/ast/ast_i2c.c
>>> @@ -117,7 +117,7 @@ struct ast_i2c_chan *ast_i2c_create(struct drm_device *dev)
>>>
>>>          i2c = kzalloc(sizeof(struct ast_i2c_chan), GFP_KERNEL);
>>>          if (!i2c)
>>> -               return NULL;
>>> +               return ERR_PTR(-ENOMEM);
>>>
>>>          i2c->adapter.owner = THIS_MODULE;
>>>          i2c->adapter.class = I2C_CLASS_DDC;
>>> @@ -143,10 +143,11 @@ struct ast_i2c_chan *ast_i2c_create(struct drm_device *dev)
>>>
>>>          ret = drmm_add_action_or_reset(dev, ast_i2c_release, i2c);
>>>          if (ret)
>>> -               return NULL;
>>> +               return ERR_PTR(ret);
>>> +
>>>          return i2c;
>>>
>>>   out_kfree:
>>>          kfree(i2c);
>>> -       return NULL;
>>> +       return ERR_PTR(ret);
>>>   }
>>> diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c
>>> index bbc566c4c768..5f273b5dd769 100644
>>> --- a/drivers/gpu/drm/ast/ast_mode.c
>>> +++ b/drivers/gpu/drm/ast/ast_mode.c
>>> @@ -1334,19 +1334,18 @@ static int ast_vga_connector_init(struct drm_device *dev,
>>>                                    struct ast_vga_connector *ast_vga_connector)
>>>   {
>>>          struct drm_connector *connector = &ast_vga_connector->base;
>>> +       struct ast_i2c_chan *i2c;
>>>          int ret;
>>>
>>> -       ast_vga_connector->i2c = ast_i2c_create(dev);
>>> -       if (!ast_vga_connector->i2c)
>>> -               drm_err(dev, "failed to add ddc bus for connector\n");
>>> +       i2c = ast_i2c_create(dev);
>>> +       if (IS_ERR(i2c)) {
>>> +               ret = PTR_ERR(i2c);
>>> +               drm_err(dev, "failed to add ddc bus for connector; ret=%d\n", ret);
>>> +               return ret;
>>> +       }
>>>
>>> -       if (ast_vga_connector->i2c)
>>> -               ret = drm_connector_init_with_ddc(dev, connector, &ast_vga_connector_funcs,
>>> -                                                 DRM_MODE_CONNECTOR_VGA,
>>> -                                                 &ast_vga_connector->i2c->adapter);
>>> -       else
>>> -               ret = drm_connector_init(dev, connector, &ast_vga_connector_funcs,
>>> -                                        DRM_MODE_CONNECTOR_VGA);
>>> +       ret = drm_connector_init_with_ddc(dev, connector, &ast_vga_connector_funcs,
>>> +                                         DRM_MODE_CONNECTOR_VGA, &i2c->adapter);
>>>          if (ret)
>>>                  return ret;
>>>
>>> @@ -1406,19 +1405,18 @@ static int ast_sil164_connector_init(struct drm_device *dev,
>>>                                       struct ast_sil164_connector *ast_sil164_connector)
>>>   {
>>>          struct drm_connector *connector = &ast_sil164_connector->base;
>>> +       struct ast_i2c_chan *i2c;
>>>          int ret;
>>>
>>> -       ast_sil164_connector->i2c = ast_i2c_create(dev);
>>> -       if (!ast_sil164_connector->i2c)
>>> -               drm_err(dev, "failed to add ddc bus for connector\n");
>>> +       i2c = ast_i2c_create(dev);
>>> +       if (IS_ERR(i2c)) {
>>> +               ret = PTR_ERR(i2c);
>>> +               drm_err(dev, "failed to add ddc bus for connector; ret=%d\n", ret);
>>> +               return ret;
>>> +       }
>>>
>>> -       if (ast_sil164_connector->i2c)
>>> -               ret = drm_connector_init_with_ddc(dev, connector, &ast_sil164_connector_funcs,
>>> -                                                 DRM_MODE_CONNECTOR_DVII,
>>> -                                                 &ast_sil164_connector->i2c->adapter);
>>> -       else
>>> -               ret = drm_connector_init(dev, connector, &ast_sil164_connector_funcs,
>>> -                                        DRM_MODE_CONNECTOR_DVII);
>>> +       ret = drm_connector_init_with_ddc(dev, connector, &ast_sil164_connector_funcs,
>>> +                                         DRM_MODE_CONNECTOR_DVII, &i2c->adapter);
>>>          if (ret)
>>>                  return ret;
>>>
>>> --
>>> 2.36.1
>>>

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev