On Fri, 17 May 2019, Oleg Vasilev <oleg.vasilev@xxxxxxxxx> wrote: > Currently, the i2c adapter was available only under DP connectors. Currently ... is? > This patch adds i2c adapter symlink to hdmi connector in order to make > this behaviour consistent. Please don't refer to "this patch" in commit messages. Please use imperative style, here as well as in the subject: "drm/i915: add i2c symlink to hdmi connector" "Add i2c adapter symlink to hdmi connector in order to make this behaviour consistent." I understand what you mean, but "add i2c symlink to hdmi connector" could be read as having a symlink point at the connector. Perhaps, "Add symlink from hdmi connector to i2c adapter in order to make this behaviour consistent." > The initial motivation of this patch was to make igt i2c subtest > patch [1] work on all connectors. > > [1]: https://patchwork.freedesktop.org/series/60357/ > > Signed-off-by: Oleg Vasilev <oleg.vasilev@xxxxxxxxx> > Cc: Arkadiusz Hiler <arkadiusz.hiler@xxxxxxxxx> > Cc: Imre Deak <imre.deak@xxxxxxxxx> > Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> > Cc: Jani Nikula <jani.nikula@xxxxxxxxx> It's probably more common to add Cc's before the Signed-off-by. > --- > drivers/gpu/drm/i915/intel_hdmi.c | 35 +++++++++++++++++++++++++++++++ > 1 file changed, 35 insertions(+) > > diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c > index 2a4086cf2692..b8ae67297878 100644 > --- a/drivers/gpu/drm/i915/intel_hdmi.c > +++ b/drivers/gpu/drm/i915/intel_hdmi.c > @@ -2658,6 +2658,37 @@ static void chv_hdmi_pre_enable(struct intel_encoder *encoder, > chv_phy_release_cl2_override(encoder); > } > > +static struct i2c_adapter * > +intel_hdmi_get_i2c_adapter(struct drm_connector *connector) > +{ > + struct intel_encoder *intel_encoder = intel_attached_encoder(connector); > + struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(&intel_encoder->base); > + struct drm_device *dev = intel_encoder->base.dev; > + struct drm_i915_private *dev_priv = to_i915(dev); struct drm_i915_private *dev_priv = to_i915(connector->dev); struct intel_hdmi *intel_hdmi = intel_attached_hdmi(connector); > + > + return intel_gmbus_get_adapter(dev_priv, intel_hdmi->ddc_bus); > +} > + > +static void intel_hdmi_create_i2c_symlink(struct drm_connector *connector) > +{ > + struct i2c_adapter *adapter = intel_hdmi_get_i2c_adapter(connector); > + struct kobject *i2c_kobj = &adapter->dev.kobj; > + struct kobject *connector_kobj = &connector->kdev->kobj; > + > + int ret = sysfs_create_link(connector_kobj, i2c_kobj, i2c_kobj->name); Please keep declarations and initialization separate for all non-trivial initialization. > + > + WARN(ret != 0, "Failed to create i2c symlink\n"); sysfs_create_link already warns when -EEXIST, and anyway I think a WARN is a bit much. DRM_ERROR perhaps. > +} > + > +static void intel_hdmi_delete_i2c_symlink(struct drm_connector *connector) Perhaps follow sysfs_remove_link naming? I.e. remove instead of delete. > +{ > + struct i2c_adapter *adapter = intel_hdmi_get_i2c_adapter(connector); > + struct kobject *i2c_kobj = &adapter->dev.kobj; > + struct kobject *connector_kobj = &connector->kdev->kobj; > + > + sysfs_remove_link(connector_kobj, i2c_kobj->name); > +} > + > static int > intel_hdmi_connector_register(struct drm_connector *connector) > { > @@ -2669,11 +2700,15 @@ intel_hdmi_connector_register(struct drm_connector *connector) > > i915_debugfs_connector_add(connector); > > + intel_hdmi_create_i2c_symlink(connector); > + > return ret; > } > > static void intel_hdmi_destroy(struct drm_connector *connector) > { > + intel_hdmi_delete_i2c_symlink(connector); What Ville said. BR, Jani. > + > if (intel_attached_hdmi(connector)->cec_notifier) > cec_notifier_put(intel_attached_hdmi(connector)->cec_notifier); -- Jani Nikula, Intel Open Source Graphics Center _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
