| Message ID | 1394020227-16738-1-git-send-email-jani.nikula@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Mar 05, 2014 at 01:50:27PM +0200, Jani Nikula wrote: [...] > I am not entirely happy with adding an extra name field, but I also > didn't like the caller having to set up aux.ddc.name in advance. Ideas > welcome. What you propose is a whole lot better than having to modify the internals of struct i2c_adapter in advance, and I can't think of a better alternative either. I have two small comments below. > diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c > index 35251af3b14e..17832d048147 100644 > --- a/drivers/gpu/drm/drm_dp_helper.c > +++ b/drivers/gpu/drm/drm_dp_helper.c > @@ -726,7 +726,8 @@ int drm_dp_aux_register_i2c_bus(struct drm_dp_aux *aux) > aux->ddc.dev.parent = aux->dev; > aux->ddc.dev.of_node = aux->dev->of_node; > > - strncpy(aux->ddc.name, dev_name(aux->dev), sizeof(aux->ddc.name)); > + strlcpy(aux->ddc.name, aux->name ? aux->name : dev_name(aux->dev), > + sizeof(aux->ddc.name)); I don't see why the change from strncpy() to strlcpy() would be necessary. > diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h [...] > @@ -438,6 +438,9 @@ struct drm_dp_aux_msg { > * The .dev field should be set to a pointer to the device that implements > * the AUX channel. > * > + * The .name field may be used to specify the name of the I2C adapter. If set to > + * NULL, dev_name() of .dev will be used. > + * Nit: That new paragraph sticks out because it's wider than the rest, even though it still fits into 80 characters. I don't feel all that strongly about either of the above, so: Reviewed-by: Thierry Reding <treding@nvidia.com>
On Mon, 10 Mar 2014, Thierry Reding <thierry.reding@gmail.com> wrote: > On Wed, Mar 05, 2014 at 01:50:27PM +0200, Jani Nikula wrote: > [...] >> I am not entirely happy with adding an extra name field, but I also >> didn't like the caller having to set up aux.ddc.name in advance. Ideas >> welcome. > > What you propose is a whole lot better than having to modify the > internals of struct i2c_adapter in advance, and I can't think of > a better alternative either. I have two small comments below. > >> diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c >> index 35251af3b14e..17832d048147 100644 >> --- a/drivers/gpu/drm/drm_dp_helper.c >> +++ b/drivers/gpu/drm/drm_dp_helper.c >> @@ -726,7 +726,8 @@ int drm_dp_aux_register_i2c_bus(struct drm_dp_aux *aux) >> aux->ddc.dev.parent = aux->dev; >> aux->ddc.dev.of_node = aux->dev->of_node; >> >> - strncpy(aux->ddc.name, dev_name(aux->dev), sizeof(aux->ddc.name)); >> + strlcpy(aux->ddc.name, aux->name ? aux->name : dev_name(aux->dev), >> + sizeof(aux->ddc.name)); > > I don't see why the change from strncpy() to strlcpy() would be > necessary. To always null terminate aux->ddc.name. >> diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h > [...] >> @@ -438,6 +438,9 @@ struct drm_dp_aux_msg { >> * The .dev field should be set to a pointer to the device that implements >> * the AUX channel. >> * >> + * The .name field may be used to specify the name of the I2C adapter. If set to >> + * NULL, dev_name() of .dev will be used. >> + * > > Nit: That new paragraph sticks out because it's wider than the rest, > even though it still fits into 80 characters. It seems emacs isn't (yet!) clever enough to reflow with the same fill-column as the neighbouring paragraphs. ;) > I don't feel all that strongly about either of the above, so: > > Reviewed-by: Thierry Reding <treding@nvidia.com> Thanks for the review, Jani.
diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c index 35251af3b14e..17832d048147 100644 --- a/drivers/gpu/drm/drm_dp_helper.c +++ b/drivers/gpu/drm/drm_dp_helper.c @@ -726,7 +726,8 @@ int drm_dp_aux_register_i2c_bus(struct drm_dp_aux *aux) aux->ddc.dev.parent = aux->dev; aux->ddc.dev.of_node = aux->dev->of_node; - strncpy(aux->ddc.name, dev_name(aux->dev), sizeof(aux->ddc.name)); + strlcpy(aux->ddc.name, aux->name ? aux->name : dev_name(aux->dev), + sizeof(aux->ddc.name)); return i2c_add_adapter(&aux->ddc); } diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h index 42947566e755..b4f58914bf7d 100644 --- a/include/drm/drm_dp_helper.h +++ b/include/drm/drm_dp_helper.h @@ -438,6 +438,9 @@ struct drm_dp_aux_msg { * The .dev field should be set to a pointer to the device that implements * the AUX channel. * + * The .name field may be used to specify the name of the I2C adapter. If set to + * NULL, dev_name() of .dev will be used. + * * Drivers provide a hardware-specific implementation of how transactions * are executed via the .transfer() function. A pointer to a drm_dp_aux_msg * structure describing the transaction is passed into this function. Upon @@ -455,6 +458,7 @@ struct drm_dp_aux_msg { * should call drm_dp_aux_unregister_i2c_bus() to remove the I2C adapter. */ struct drm_dp_aux { + const char *name; struct i2c_adapter ddc; struct device *dev;
Let the drivers specify the name of the I2C-over-AUX adapter to maintain backwards compatibility in the sysfs when converting to the new I2C-over-AUX helper infrastructure. The i915 driver currently uses DPDDC-A to DPDDC-D as names for the DP i2c adapters. These names show up in the i2c sysfs name attribute. We'd like to be able to maintain that when switching over to the new helpers. Due to i2c device and connector cleanup ordering issues we also recently made the drm device (instead of connector) the parent of the i2c adapters: commit 80f65de3c9b8101c1613fa82df500ba6a099a11c Author: Imre Deak <imre.deak@intel.com> Date: Tue Feb 11 17:12:49 2014 +0200 drm/i915: dp: fix order of dp aux i2c device cleanup With the name picked up from the adapter parent using dev_name(), it would be the same for all i2c adapters with the current I2C-over-AUX helpers. Signed-off-by: Jani Nikula <jani.nikula@intel.com> --- I am not entirely happy with adding an extra name field, but I also didn't like the caller having to set up aux.ddc.name in advance. Ideas welcome. --- drivers/gpu/drm/drm_dp_helper.c | 3 ++- include/drm/drm_dp_helper.h | 4 ++++ 2 files changed, 6 insertions(+), 1 deletion(-)