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
