[RESEND PATCH v2 1/3] drm: dw_hdmi: use of_get_i2c_adapter_by_node interface

[Vladimir Zapolskiy]

Hello Russell,

On 08/18/2016 05:32 PM, Russell King - ARM Linux wrote:
> On Tue, Aug 16, 2016 at 11:26:43PM +0300, Vladimir Zapolskiy wrote:
>> This change is needed to properly lock I2C bus driver, which serves
>> DDC.
>>
>> The change fixes an overflow over zero of I2C bus driver user counter:
>>
>>   root at imx6q:~# lsmod
>>   Not tainted
>>   dw_hdmi_ahb_audio 4082 0 - Live 0xbf02c000
>>   dw_hdmi_imx 3498 0 - Live 0xbf00d000
>>   dw_hdmi 16398 2 dw_hdmi_ahb_audio,dw_hdmi_imx, Live 0xbf004000
>>   i2c_imx 16687 0 - Live 0xbf017000
>>
>>   root at imx6q:~# rmmod dw_hdmi_imx
>>   root at imx6q:~# lsmod
>>   Not tainted
>>   dw_hdmi_ahb_audio 4082 0 - Live 0xbf02c000
>>   dw_hdmi 16398 1 dw_hdmi_ahb_audio, Live 0xbf004000
>>   i2c_imx 16687 -1 - Live 0xbf017000
>>                 ^^
>>
>>   root at imx6q:~# rmmod i2c_imx
>>   rmmod: ERROR: Module i2c_imx is in use
>>
>> Note that prior to this change put_device() coupled with
>> of_find_i2c_adapter_by_node() was missing on error path of
>> dw_hdmi_bind(), added i2c_put_adapter() there along with the change.
>
> I *guess* this is the right thing, but it would be nice to see the
> results with the patch applied in the commit description.  Nevertheless:
>
> Acked-by: Russell King <rmk+kernel at armlinux.org.uk>
>

thank you for review.

For information this is a result after applying the fix (+1 to i2c_imx users):

root at imx6q# lsmod
     Not tainted
dw_hdmi_ahb_audio 4082 0 - Live 0xbf02c000
i2c_imx 16687 1 - Live 0xbf011000
dw_hdmi_imx 3498 0 - Live 0xbf00d000
dw_hdmi 18925 2 dw_hdmi_ahb_audio,dw_hdmi_imx, Live 0xbf004000

root at imx6q:~# rmmod dw_hdmi_imx

root at imx6q:~# lsmod
     Not tainted
dw_hdmi_ahb_audio 4082 0 - Live 0xbf02c000
i2c_imx 16687 0 - Live 0xbf011000
dw_hdmi 18925 1 dw_hdmi_ahb_audio, Live 0xbf004000

No weird negative use count.

I have another pending change against DW HDMI, to avoid git-am conflicts
I'll rebase it on top of this one and resend today later on.

>
> I'd also like to see the DDC bits extracted from the various imx
> drivers, because this is surely common code - I've had this floating
> around for a few years but never got around to finishing it off and
> submitting it.  If folk think this is a good idea, and want to take
> the idea on, that's fine by me.
>
>  drivers/gpu/drm/Makefile            |  2 +
>  drivers/gpu/drm/bridge/dw-hdmi.c    | 98 +++++++++----------------------------
>  drivers/gpu/drm/drm_ddc_connector.c | 91 ++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/imx/imx-tve.c       | 59 ++++++----------------
>  include/drm/drm_ddc_connector.h     | 33 +++++++++++++
>  5 files changed, 163 insertions(+), 120 deletions(-)

I've briefly reviewed the changes and in my opinion this is a good
to have generalization of DDC connector, hopefully DRM people agree.

Moreover I assume that in case of getting modes over I2C/DDC most of
the .get_modes callbacks share almost the same code sequence:

   drm_get_edid()
   drm_detect_hdmi_monitor()
   drm_mode_connector_update_edid_property()
   drm_add_edid_modes()
   drm_edid_to_eld()

I'm not absolutely sure, but probably this can be generalized as well.

--
With best wishes,
Vladimir