From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755773AbcDNNmz (ORCPT ); Thu, 14 Apr 2016 09:42:55 -0400 Received: from bhuna.collabora.co.uk ([46.235.227.227]:49505 "EHLO bhuna.collabora.co.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754423AbcDNNmx (ORCPT ); Thu, 14 Apr 2016 09:42:53 -0400 Message-ID: <570F9E4D.7040401@collabora.com> Date: Thu, 14 Apr 2016 15:42:37 +0200 From: Enric Balletbo i Serra User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.8.0 MIME-Version: 1.0 To: Thierry Reding CC: devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org, airlied@linux.ie, robh+dt@kernel.org, djkurtz@chromium.org, drinkcat@chromium.org, dan.carpenter@oracle.com, Emil Velikov , Rob Herring Subject: Re: [PATCH v3 3/3] drm: bridge: anx78xx: Add anx78xx driver support. References: <1460119972-8658-1-git-send-email-enric.balletbo@collabora.com> <1460119972-8658-4-git-send-email-enric.balletbo@collabora.com> <20160414131013.GA32237@ulmo.ba.sec> In-Reply-To: <20160414131013.GA32237@ulmo.ba.sec> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Thierry, Many thanks for answering and do this accurate report. I'd add a comment on something you (see below). Apart from this I'll add your changes and send a new version. On 14/04/16 15:10, Thierry Reding wrote: > On Fri, Apr 08, 2016 at 02:52:52PM +0200, Enric Balletbo i Serra wrote: >> Although there are other chips from the same family that can reuse this >> driver, at the moment we only tested ANX7814 chip. >> >> The ANX7814 is an ultra-low power Full-HD (1080p60) SlimPort transmitter >> designed for portable devices. This driver adds initial support for HDMI >> to DP pass-through mode. >> >> Signed-off-by: Enric Balletbo i Serra >> Tested-by: Nicolas Boichat >> Reviewed-by: Nicolas Boichat >> Cc: Emil Velikov >> Cc: Rob Herring >> Cc: Dan Carpenter >> Cc: Daniel Kurtz >> Cc: Nicolas Boichat >> --- >> Changes since v2: >> - Nicolas Boichat: >> - Get rid of wait_for macro since is only used once. >> - Do not replace the error code if it's readily available to you. >> - Add Tested-by: Nicolas Boichat >> - Add Reviewed-by: Nicolas Boichat >> >> Changes since v1: >> - Dan Carpenter: >> - Fix missing error code >> - Use meaningful names for goto exit paths >> - Rob Herring: >> - Use hpd instead cable_det as is the more standard name. >> - Daniel Kurtz: >> - Use regmap_bulk in aux_transfer >> - Fix gpio reset polarity. >> - Turn off v10 last so we mirror poweron sequence >> - Fix some error paths. >> - Remove mutex in anx78xx_detect >> - kbuild: >> - WARNING: PTR_ERR_OR_ZERO can be used >> >> drivers/gpu/drm/bridge/Kconfig | 8 + >> drivers/gpu/drm/bridge/Makefile | 1 + >> drivers/gpu/drm/bridge/anx78xx.c | 1403 ++++++++++++++++++++++++++++++++++++++ >> drivers/gpu/drm/bridge/anx78xx.h | 719 +++++++++++++++++++ >> 4 files changed, 2131 insertions(+) >> create mode 100644 drivers/gpu/drm/bridge/anx78xx.c >> create mode 100644 drivers/gpu/drm/bridge/anx78xx.h >> >> diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig >> index 27e2022..0f595ae 100644 >> --- a/drivers/gpu/drm/bridge/Kconfig >> +++ b/drivers/gpu/drm/bridge/Kconfig >> @@ -40,4 +40,12 @@ config DRM_PARADE_PS8622 >> ---help--- >> Parade eDP-LVDS bridge chip driver. >> >> +config DRM_ANX78XX > > The symbol name should include the vendor (DRM_ANALOGIX_ANX78XX) and the > entry needs to be ordered alphabetically (by vendor, then name). > >> + tristate "Analogix ANX78XX bridge" >> + select DRM_KMS_HELPER >> + select REGMAP_I2C >> + ---help--- >> + ANX78XX is a HD video transmitter chip over micro-USB connector >> + for smartphone device. > > The commit description says the device is a FullHD video transmitter, > but here you say HD. Pick one. Preferably the correct one. > >> endmenu >> diff --git a/drivers/gpu/drm/bridge/Makefile b/drivers/gpu/drm/bridge/Makefile >> index f13c33d..8f0d69e 100644 >> --- a/drivers/gpu/drm/bridge/Makefile >> +++ b/drivers/gpu/drm/bridge/Makefile >> @@ -4,3 +4,4 @@ obj-$(CONFIG_DRM_DW_HDMI) += dw-hdmi.o >> obj-$(CONFIG_DRM_DW_HDMI_AHB_AUDIO) += dw-hdmi-ahb-audio.o >> obj-$(CONFIG_DRM_NXP_PTN3460) += nxp-ptn3460.o >> obj-$(CONFIG_DRM_PARADE_PS8622) += parade-ps8622.o >> +obj-$(CONFIG_DRM_ANX78XX) += anx78xx.o > > Same here, the source file should be named analogix-anx78xx.c, and this > needs to be sorted by vendor, then name as well. > >> diff --git a/drivers/gpu/drm/bridge/anx78xx.c b/drivers/gpu/drm/bridge/anx78xx.c > [...] >> +#include > > At least this one doesn't seem to be needed. > >> +static int anx78xx_aux_wait(struct anx78xx *anx78xx) >> +{ >> + int err; >> + unsigned int status; >> + unsigned long timeout; >> + >> + /* >> + * Does the right thing for modeset paths when run under kdgb or >> + * similar atomic contexts. Note that it's important that we check the >> + * condition again after having timed out, since the timeout could be >> + * due to preemption or similar and we've never had a chance to check >> + * the condition before the timeout. >> + */ > > I don't think this is necessary. The AUX code should never be called > from atomic context, there are various other places where we take a > mutex that would trigger warnings. > >> + err = 0; > > You can move this up to where the variable is declared. > >> + timeout = jiffies + msecs_to_jiffies(AUX_WAIT_TIMEOUT_MS) + 1; >> + while (anx78xx_aux_op_finished(anx78xx)) { >> + if (time_after(jiffies, timeout)) { >> + if (anx78xx_aux_op_finished(anx78xx)) >> + err = -ETIMEDOUT; > > Should this not be !cond? Ah, anx78xx_aux_op_finished() returns an error > code on failure. Perhaps it would be clearer if this either returned a > boolean or the name was changed to reflect the fact that it returns an > error code. _finished() sounds too much like it would return boolean. > > To make it clearer what I mean, try reading the above aloud: > > if aux_op_finished, return error > > That's the wrong way around. > >> + break; >> + } >> + if (drm_can_sleep()) >> + usleep_range(1000, 2000); >> + else >> + cpu_relax(); >> + } >> + >> + if (err) { >> + DRM_ERROR("Timed out waiting AUX to finish\n"); >> + return -ETIMEDOUT; >> + } >> + >> + /* Read the AUX channel access status */ >> + err = regmap_read(anx78xx->map[I2C_IDX_TX_P0], SP_AUX_CH_STATUS_REG, >> + &status); >> + if (err) >> + return err; >> + >> + if (status & SP_AUX_STATUS) { >> + DRM_ERROR("Failed to read AUX channel: 0x%02x\n", status); >> + return -ETIMEDOUT; >> + } > > Would it make sense to disambiguate these two errors using different > error codes? As it is this function will either return success or > timeout, even though the latter is obviously not a timeout. > >> +static int anx78xx_aux_address(struct anx78xx *anx78xx, unsigned int addr) >> +{ >> + int err = 0; >> + >> + err |= regmap_write(anx78xx->map[I2C_IDX_TX_P0], SP_AUX_ADDR_7_0_REG, >> + addr & 0xff); >> + err |= regmap_write(anx78xx->map[I2C_IDX_TX_P0], SP_AUX_ADDR_15_8_REG, >> + (addr & 0xff00) >> 8); >> + >> + /* >> + * DP AUX CH Address Register #2, only update bits[3:0] >> + * [7:4] RESERVED >> + * [3:0] AUX_ADDR[19:16], Register control AUX CH address. >> + */ >> + err |= regmap_update_bits(anx78xx->map[I2C_IDX_TX_P0], >> + SP_AUX_ADDR_19_16_REG, >> + SP_AUX_ADDR_19_16_MASK, >> + (addr & 0xf0000) >> 16); > > I'm not at all a fan of OR'ing error codes. Presumably if any of these > register accesses fails there's no reason to attempt the subsequent > accesses because the end result will be failure anyway. > The patch was implemented first without OR'ing error codes. The reason why I changed this is because I received the comments that checking the error on every regmap_* didn't help the readability of the driver and is likely to not fail if the first call doesn't fail. For example, originally the code was like this: http://pastebin.com/rPgyji8k but I changed to this http://pastebin.com/rPgyji8k I don't have a hard opinion on this so I'll do what you prefer, but before reverting this I just want to make sure that you prefer the first option. It's right? >> + /* Write address and request */ >> + err = anx78xx_aux_address(anx78xx, msg->address); >> + err |= regmap_write(anx78xx->map[I2C_IDX_TX_P0], SP_DP_AUX_CH_CTRL1_REG, >> + ctrl1); >> + if (err) >> + return -EIO; >> + >> + /* Start transaction */ >> + err = regmap_update_bits(anx78xx->map[I2C_IDX_TX_P0], >> + SP_DP_AUX_CH_CTRL2_REG, SP_ADDR_ONLY | >> + SP_AUX_EN, ctrl2); >> + if (err) >> + return err; >> + >> + err = anx78xx_aux_wait(anx78xx); >> + >> + msg->reply = err ? DP_AUX_I2C_REPLY_NACK : DP_AUX_I2C_REPLY_ACK; > > This looks wrong. What if anx78xx_aux_wait() return -ETIMEDOUT? You'll > be setting msg->reply to NACK and return success That doesn't sound > right at all. > >> +static int anx78xx_set_hpd(struct anx78xx *anx78xx) >> +{ >> + int err = 0; >> + >> + err |= anx78xx_clear_bits(anx78xx->map[I2C_IDX_RX_P0], >> + SP_TMDS_CTRL_BASE + 7, SP_PD_RT); >> + err |= anx78xx_set_bits(anx78xx->map[I2C_IDX_TX_P2], SP_VID_CTRL3_REG, >> + SP_HPD_OUT); > > Again, OR'ing of error codes, please don't do it. There are some more > occurrences below. > >> +static int anx78xx_init_gpio(struct anx78xx *anx78xx) >> +{ >> + struct device *dev = &anx78xx->client->dev; >> + struct anx78xx_platform_data *pdata = &anx78xx->pdata; >> + >> + /* GPIO for hpd */ > > HPD being an abbreviation it should be capitalized. Similar for a couple > of other abbreviations, some of which are inconsistently capitalized. In > variable names of consumer names, the lowercase variant is fine, but the > variant used in text (messages, comments) should be the all caps one. > >> + pdata->gpiod_hpd = devm_gpiod_get(dev, "hpd", GPIOD_IN); >> + if (IS_ERR(pdata->gpiod_hpd)) >> + return PTR_ERR(pdata->gpiod_hpd); >> + >> + /* GPIO for chip power down */ >> + pdata->gpiod_pd = devm_gpiod_get(dev, "pd", GPIOD_OUT_HIGH); >> + if (IS_ERR(pdata->gpiod_pd)) >> + return PTR_ERR(pdata->gpiod_pd); >> + >> + /* GPIO for chip reset */ >> + pdata->gpiod_reset = devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW); >> + if (IS_ERR(pdata->gpiod_reset)) >> + return PTR_ERR(pdata->gpiod_reset); >> + >> + /* GPIO for V10 power control */ >> + pdata->gpiod_v10 = devm_gpiod_get_optional(dev, "v10", GPIOD_OUT_LOW); > > Does this actually supply power? If so it should be modelled as a > regulator. > >> +static int anx78xx_dp_link_training(struct anx78xx *anx78xx) >> +{ >> + int err = 0; >> + u8 dp_bw, regval; >> + >> + err |= regmap_write(anx78xx->map[I2C_IDX_RX_P0], SP_HDMI_MUTE_CTRL_REG, >> + 0x0); >> + err |= anx78xx_clear_bits(anx78xx->map[I2C_IDX_TX_P2], >> + SP_POWERDOWN_CTRL_REG, >> + SP_TOTAL_PD); >> + if (err) >> + return -EIO; >> + >> + err = drm_dp_dpcd_readb(&anx78xx->aux, DP_MAX_LINK_RATE, &dp_bw); >> + if (err < 0) >> + return err; >> + >> + switch (dp_bw) { >> + case DP_LINK_BW_1_62: >> + case DP_LINK_BW_2_7: >> + case DP_LINK_BW_5_4: >> + break; >> + default: >> + DRM_DEBUG_KMS("Waiting to read DP bandwidth.\n"); >> + return -EAGAIN; >> + } > > That sounds wrong. Either you can read the content and it should be a > valid value (albeit one which you may not support) or you can't. Why do > you need to potentially repeat this read? > >> + >> + err = anx78xx_set_bits(anx78xx->map[I2C_IDX_TX_P2], SP_VID_CTRL1_REG, >> + SP_VIDEO_MUTE); >> + err |= anx78xx_clear_bits(anx78xx->map[I2C_IDX_TX_P2], >> + SP_VID_CTRL1_REG, SP_VIDEO_EN); >> + if (err) >> + return -EIO; >> + >> + /* Get dpcd info */ > > s/dpcd/DPCD/ > >> + err = drm_dp_dpcd_read(&anx78xx->aux, DP_DPCD_REV, >> + &anx78xx->dpcd, DP_RECEIVER_CAP_SIZE); >> + if (err < 0) { >> + DRM_ERROR("Failed to read DPCD\n"); > > It's often useful to output the error code as part of the error message > to make it easier for developers to diagnose problems. > >> + return err; >> + } >> + >> + /* Clear channel x SERDES power down */ >> + err = anx78xx_clear_bits(anx78xx->map[I2C_IDX_TX_P0], >> + SP_DP_ANALOG_POWER_DOWN_REG, SP_CH0_PD); >> + if (err) >> + return -EIO; >> + >> + /* Check link capabilities */ >> + err = drm_dp_link_probe(&anx78xx->aux, &anx78xx->link); >> + if (err < 0) { >> + DRM_ERROR("Failed to probe link capabilities\n"); >> + return err; >> + } >> + >> + /* Power up the sink */ >> + err = drm_dp_link_power_up(&anx78xx->aux, &anx78xx->link); >> + if (err < 0) { >> + DRM_ERROR("Failed to power up DisplayPort link\n"); >> + return err; >> + } >> + >> + /* Possibly enable downspread on the sink */ >> + err = regmap_write(anx78xx->map[I2C_IDX_TX_P0], >> + SP_DP_DOWNSPREAD_CTRL1_REG, 0); >> + if (err) >> + return err; >> + >> + if (anx78xx->dpcd[3] & 0x1) { > > This should use the symbolic constants defined in drm_dp_helper.h. > Actually, it should probably add a symbolic constant because we don't > have one yet for bit 0 in the DP_MAX_DOWNSPREAD register. > >> +static inline struct anx78xx * >> + connector_to_anx78xx(struct drm_connector *connector) > > The function name should start on the first column. Also you might want > to move this inline function after the struct anx78xx declaration, which > is more consistent with other drivers. > >> +static const struct drm_connector_helper_funcs >> + anx78xx_connector_helper_funcs = { > > The structure name should start in the first column as well. > >> + .get_modes = anx78xx_get_modes, >> + .best_encoder = anx78xx_best_encoder, >> +}; >> + >> +static enum drm_connector_status anx78xx_detect(struct drm_connector *connector, >> + bool force) >> +{ >> + struct anx78xx *anx78xx = container_of(connector, struct anx78xx, >> + connector); > > Didn't you introduce an inline function for this just a few lines above? > >> + DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n", connector->base.id, >> + connector->name); >> + >> + if (!gpiod_get_value(anx78xx->pdata.gpiod_hpd)) { >> + DRM_DEBUG_KMS("CONNECTOR STATUS DISCONNECTED\n"); >> + return connector_status_disconnected; >> + } >> + >> + return connector_status_connected; >> +} > > Is it really necessary to add two debug messages here? The DRM core will > already output a message for connector status changes, so this is > unnecessarily noisy in my opinion. > >> +static inline struct anx78xx *bridge_to_anx78xx(struct drm_bridge *bridge) >> +{ >> + return container_of(bridge, struct anx78xx, bridge); >> +} > > Put this together with the connector cast function. > >> +static void anx78xx_bridge_disable(struct drm_bridge *bridge) >> +{ >> + struct anx78xx *anx78xx = bridge_to_anx78xx(bridge); >> + >> + mutex_lock(&anx78xx->lock); > > Do you really need the locking here and below? I think the DRM core > already ensures that these calls are always serialized. > >> +static void anx78xx_bridge_mode_set(struct drm_bridge *bridge, >> + struct drm_display_mode *mode, >> + struct drm_display_mode *adjusted_mode) >> +{ >> + int err; >> + struct hdmi_avi_infoframe frame; >> + struct anx78xx *anx78xx = bridge_to_anx78xx(bridge); >> + >> + if (WARN_ON(!anx78xx->powered)) >> + return; >> + >> + mutex_lock(&anx78xx->lock); >> + >> + mode = adjusted_mode; >> + >> + err = drm_hdmi_avi_infoframe_from_display_mode(&frame, mode); > > This seems like jumping through hoops. Why not simply pass adjusted_mode > to the function? > >> +static const struct drm_bridge_funcs anx78xx_bridge_funcs = { >> + .attach = anx78xx_bridge_attach, >> + .mode_fixup = anx78xx_bridge_mode_fixup, >> + .disable = anx78xx_bridge_disable, >> + .mode_set = anx78xx_bridge_mode_set, >> + .enable = anx78xx_bridge_enable, >> +}; > > I'd leave out the tab-padding. Simple spaces will do just fine. > >> +static irqreturn_t anx78xx_hpd_threaded_handler(int unused, void *data) > > Might as well give the first parameter the proper name (irq). > >> +static bool anx78xx_handle_common_int_4(struct anx78xx *anx78xx, u8 irq) >> +{ >> + int err; >> + bool event = false; >> + >> + DRM_DEBUG_KMS("Handle common interrupt 4: %02x\n", irq); >> + >> + err = regmap_write(anx78xx->map[I2C_IDX_TX_P2], >> + SP_COMMON_INT_STATUS4_REG, irq); >> + if (err) { >> + DRM_ERROR("Failed to write SP_COMMON_INT_STATUS4 %d\n", err); >> + return event; >> + } >> + >> + if (irq & SP_HPD_LOST) { >> + DRM_DEBUG_KMS("IRQ: Hot plug detect - cable is pulled out\n"); >> + event = true; >> + anx78xx_poweroff(anx78xx); >> + } else if (irq & SP_HPD_PLUG) { >> + DRM_DEBUG_KMS("IRQ: Hot plug detect - cable plug\n"); >> + event = true; >> + /* Free previous cached EDID if any */ >> + kfree(anx78xx->edid); >> + anx78xx->edid = NULL; > > I think you can free the EDID on unplug, since it becomes stale at that > point already. The DRM core will also remove the EDID data on unplug. > >> +static void unregister_i2c_dummy_clients(struct anx78xx *anx78xx) >> +{ >> + int i; > > unsigned int > >> + >> + for (i = 0; i < ARRAY_SIZE(anx78xx->i2c_dummy); i++) >> + if (anx78xx->i2c_dummy[i]) >> + i2c_unregister_device(anx78xx->i2c_dummy[i]); >> +} >> + >> +static const struct regmap_config anx78xx_regmap_config = { >> + .reg_bits = 8, >> + .val_bits = 8, >> +}; >> + >> +static const u16 anx78xx_chipid_list[] = { >> + 0x7812, >> + 0x7814, >> + 0x7818, >> +}; >> + >> +static int anx78xx_i2c_probe(struct i2c_client *client, >> + const struct i2c_device_id *id) >> +{ >> + struct anx78xx *anx78xx; >> + struct anx78xx_platform_data *pdata; >> + int err, i; > > i should be unsigned int. > >> + unsigned int idl, idh, version; >> + bool found = false; >> + >> + anx78xx = devm_kzalloc(&client->dev, sizeof(*anx78xx), GFP_KERNEL); >> + if (!anx78xx) >> + return -ENOMEM; >> + >> + pdata = &anx78xx->pdata; >> + >> + mutex_init(&anx78xx->lock); >> + >> +#if IS_ENABLED(CONFIG_OF) >> + anx78xx->bridge.of_node = client->dev.of_node; >> +#endif >> + >> + anx78xx->client = client; >> + i2c_set_clientdata(client, anx78xx); >> + >> + err = anx78xx_init_gpio(anx78xx); >> + if (err) { >> + DRM_ERROR("Failed to initialize gpios\n"); >> + return err; >> + } >> + >> + pdata->hpd_irq = gpiod_to_irq(pdata->gpiod_hpd); >> + if (pdata->hpd_irq < 0) { >> + DRM_ERROR("Failed to get hpd irq %d\n", >> + pdata->hpd_irq); >> + return -ENODEV; >> + } >> + >> + pdata->intp_irq = client->irq; >> + if (!pdata->intp_irq) { >> + DRM_ERROR("Failed to get CABLE_DET and INTP irq\n"); >> + return -ENODEV; >> + } >> + >> + /* Map slave addresses of ANX7814 */ >> + for (i = 0; i < I2C_NUM_ADDRESSES; i++) { >> + anx78xx->i2c_dummy[i] = i2c_new_dummy(client->adapter, >> + anx78xx_i2c_addresses[i] >> 1); >> + if (!anx78xx->i2c_dummy[i]) { >> + err = -ENOMEM; >> + DRM_ERROR("Failed to reserve i2c bus %02x.\n", >> + anx78xx_i2c_addresses[i]); >> + goto err_unregister_i2c; >> + } >> + >> + anx78xx->map[i] = devm_regmap_init_i2c(anx78xx->i2c_dummy[i], >> + &anx78xx_regmap_config); >> + if (IS_ERR(anx78xx->map[i])) { >> + err = PTR_ERR(anx78xx->map[i]); >> + DRM_ERROR("Failed regmap initialization %02x.\n", >> + anx78xx_i2c_addresses[i]); >> + goto err_unregister_i2c; >> + } >> + } > > That's quite some overhead merely to use regmap... Perhaps there's room > to enhance regmap-i2c to support multiple addresses for the same device? > >> +static int anx78xx_i2c_remove(struct i2c_client *client) >> +{ >> + struct anx78xx *anx78xx = i2c_get_clientdata(client); >> + >> + drm_bridge_remove(&anx78xx->bridge); >> + >> + unregister_i2c_dummy_clients(anx78xx); >> + >> + kfree(anx78xx->edid); >> + anx78xx->edid = NULL; > > The memory pointed at by anx78xx will be freed a couple of instructions > later, there's no need to set ->edid to NULL. > > Thierry > Enric From mboxrd@z Thu Jan 1 00:00:00 1970 From: Enric Balletbo i Serra Subject: Re: [PATCH v3 3/3] drm: bridge: anx78xx: Add anx78xx driver support. Date: Thu, 14 Apr 2016 15:42:37 +0200 Message-ID: <570F9E4D.7040401@collabora.com> References: <1460119972-8658-1-git-send-email-enric.balletbo@collabora.com> <1460119972-8658-4-git-send-email-enric.balletbo@collabora.com> <20160414131013.GA32237@ulmo.ba.sec> Mime-Version: 1.0 Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20160414131013.GA32237-EkSeR96xj6Pcmrwk2tT4+A@public.gmane.org> Sender: devicetree-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org To: Thierry Reding Cc: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org, airlied-cv59FeDIM0c@public.gmane.org, robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org, djkurtz-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org, drinkcat-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org, dan.carpenter-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org, Emil Velikov , Rob Herring List-Id: devicetree@vger.kernel.org Hi Thierry, Many thanks for answering and do this accurate report. I'd add a comment on something you (see below). Apart from this I'll add your changes and send a new version. On 14/04/16 15:10, Thierry Reding wrote: > On Fri, Apr 08, 2016 at 02:52:52PM +0200, Enric Balletbo i Serra wrote: >> Although there are other chips from the same family that can reuse this >> driver, at the moment we only tested ANX7814 chip. >> >> The ANX7814 is an ultra-low power Full-HD (1080p60) SlimPort transmitter >> designed for portable devices. This driver adds initial support for HDMI >> to DP pass-through mode. >> >> Signed-off-by: Enric Balletbo i Serra >> Tested-by: Nicolas Boichat >> Reviewed-by: Nicolas Boichat >> Cc: Emil Velikov >> Cc: Rob Herring >> Cc: Dan Carpenter >> Cc: Daniel Kurtz >> Cc: Nicolas Boichat >> --- >> Changes since v2: >> - Nicolas Boichat: >> - Get rid of wait_for macro since is only used once. >> - Do not replace the error code if it's readily available to you. >> - Add Tested-by: Nicolas Boichat >> - Add Reviewed-by: Nicolas Boichat >> >> Changes since v1: >> - Dan Carpenter: >> - Fix missing error code >> - Use meaningful names for goto exit paths >> - Rob Herring: >> - Use hpd instead cable_det as is the more standard name. >> - Daniel Kurtz: >> - Use regmap_bulk in aux_transfer >> - Fix gpio reset polarity. >> - Turn off v10 last so we mirror poweron sequence >> - Fix some error paths. >> - Remove mutex in anx78xx_detect >> - kbuild: >> - WARNING: PTR_ERR_OR_ZERO can be used >> >> drivers/gpu/drm/bridge/Kconfig | 8 + >> drivers/gpu/drm/bridge/Makefile | 1 + >> drivers/gpu/drm/bridge/anx78xx.c | 1403 ++++++++++++++++++++++++++++++++++++++ >> drivers/gpu/drm/bridge/anx78xx.h | 719 +++++++++++++++++++ >> 4 files changed, 2131 insertions(+) >> create mode 100644 drivers/gpu/drm/bridge/anx78xx.c >> create mode 100644 drivers/gpu/drm/bridge/anx78xx.h >> >> diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig >> index 27e2022..0f595ae 100644 >> --- a/drivers/gpu/drm/bridge/Kconfig >> +++ b/drivers/gpu/drm/bridge/Kconfig >> @@ -40,4 +40,12 @@ config DRM_PARADE_PS8622 >> ---help--- >> Parade eDP-LVDS bridge chip driver. >> >> +config DRM_ANX78XX > > The symbol name should include the vendor (DRM_ANALOGIX_ANX78XX) and the > entry needs to be ordered alphabetically (by vendor, then name). > >> + tristate "Analogix ANX78XX bridge" >> + select DRM_KMS_HELPER >> + select REGMAP_I2C >> + ---help--- >> + ANX78XX is a HD video transmitter chip over micro-USB connector >> + for smartphone device. > > The commit description says the device is a FullHD video transmitter, > but here you say HD. Pick one. Preferably the correct one. > >> endmenu >> diff --git a/drivers/gpu/drm/bridge/Makefile b/drivers/gpu/drm/bridge/Makefile >> index f13c33d..8f0d69e 100644 >> --- a/drivers/gpu/drm/bridge/Makefile >> +++ b/drivers/gpu/drm/bridge/Makefile >> @@ -4,3 +4,4 @@ obj-$(CONFIG_DRM_DW_HDMI) += dw-hdmi.o >> obj-$(CONFIG_DRM_DW_HDMI_AHB_AUDIO) += dw-hdmi-ahb-audio.o >> obj-$(CONFIG_DRM_NXP_PTN3460) += nxp-ptn3460.o >> obj-$(CONFIG_DRM_PARADE_PS8622) += parade-ps8622.o >> +obj-$(CONFIG_DRM_ANX78XX) += anx78xx.o > > Same here, the source file should be named analogix-anx78xx.c, and this > needs to be sorted by vendor, then name as well. > >> diff --git a/drivers/gpu/drm/bridge/anx78xx.c b/drivers/gpu/drm/bridge/anx78xx.c > [...] >> +#include > > At least this one doesn't seem to be needed. > >> +static int anx78xx_aux_wait(struct anx78xx *anx78xx) >> +{ >> + int err; >> + unsigned int status; >> + unsigned long timeout; >> + >> + /* >> + * Does the right thing for modeset paths when run under kdgb or >> + * similar atomic contexts. Note that it's important that we check the >> + * condition again after having timed out, since the timeout could be >> + * due to preemption or similar and we've never had a chance to check >> + * the condition before the timeout. >> + */ > > I don't think this is necessary. The AUX code should never be called > from atomic context, there are various other places where we take a > mutex that would trigger warnings. > >> + err = 0; > > You can move this up to where the variable is declared. > >> + timeout = jiffies + msecs_to_jiffies(AUX_WAIT_TIMEOUT_MS) + 1; >> + while (anx78xx_aux_op_finished(anx78xx)) { >> + if (time_after(jiffies, timeout)) { >> + if (anx78xx_aux_op_finished(anx78xx)) >> + err = -ETIMEDOUT; > > Should this not be !cond? Ah, anx78xx_aux_op_finished() returns an error > code on failure. Perhaps it would be clearer if this either returned a > boolean or the name was changed to reflect the fact that it returns an > error code. _finished() sounds too much like it would return boolean. > > To make it clearer what I mean, try reading the above aloud: > > if aux_op_finished, return error > > That's the wrong way around. > >> + break; >> + } >> + if (drm_can_sleep()) >> + usleep_range(1000, 2000); >> + else >> + cpu_relax(); >> + } >> + >> + if (err) { >> + DRM_ERROR("Timed out waiting AUX to finish\n"); >> + return -ETIMEDOUT; >> + } >> + >> + /* Read the AUX channel access status */ >> + err = regmap_read(anx78xx->map[I2C_IDX_TX_P0], SP_AUX_CH_STATUS_REG, >> + &status); >> + if (err) >> + return err; >> + >> + if (status & SP_AUX_STATUS) { >> + DRM_ERROR("Failed to read AUX channel: 0x%02x\n", status); >> + return -ETIMEDOUT; >> + } > > Would it make sense to disambiguate these two errors using different > error codes? As it is this function will either return success or > timeout, even though the latter is obviously not a timeout. > >> +static int anx78xx_aux_address(struct anx78xx *anx78xx, unsigned int addr) >> +{ >> + int err = 0; >> + >> + err |= regmap_write(anx78xx->map[I2C_IDX_TX_P0], SP_AUX_ADDR_7_0_REG, >> + addr & 0xff); >> + err |= regmap_write(anx78xx->map[I2C_IDX_TX_P0], SP_AUX_ADDR_15_8_REG, >> + (addr & 0xff00) >> 8); >> + >> + /* >> + * DP AUX CH Address Register #2, only update bits[3:0] >> + * [7:4] RESERVED >> + * [3:0] AUX_ADDR[19:16], Register control AUX CH address. >> + */ >> + err |= regmap_update_bits(anx78xx->map[I2C_IDX_TX_P0], >> + SP_AUX_ADDR_19_16_REG, >> + SP_AUX_ADDR_19_16_MASK, >> + (addr & 0xf0000) >> 16); > > I'm not at all a fan of OR'ing error codes. Presumably if any of these > register accesses fails there's no reason to attempt the subsequent > accesses because the end result will be failure anyway. > The patch was implemented first without OR'ing error codes. The reason why I changed this is because I received the comments that checking the error on every regmap_* didn't help the readability of the driver and is likely to not fail if the first call doesn't fail. For example, originally the code was like this: http://pastebin.com/rPgyji8k but I changed to this http://pastebin.com/rPgyji8k I don't have a hard opinion on this so I'll do what you prefer, but before reverting this I just want to make sure that you prefer the first option. It's right? >> + /* Write address and request */ >> + err = anx78xx_aux_address(anx78xx, msg->address); >> + err |= regmap_write(anx78xx->map[I2C_IDX_TX_P0], SP_DP_AUX_CH_CTRL1_REG, >> + ctrl1); >> + if (err) >> + return -EIO; >> + >> + /* Start transaction */ >> + err = regmap_update_bits(anx78xx->map[I2C_IDX_TX_P0], >> + SP_DP_AUX_CH_CTRL2_REG, SP_ADDR_ONLY | >> + SP_AUX_EN, ctrl2); >> + if (err) >> + return err; >> + >> + err = anx78xx_aux_wait(anx78xx); >> + >> + msg->reply = err ? DP_AUX_I2C_REPLY_NACK : DP_AUX_I2C_REPLY_ACK; > > This looks wrong. What if anx78xx_aux_wait() return -ETIMEDOUT? You'll > be setting msg->reply to NACK and return success That doesn't sound > right at all. > >> +static int anx78xx_set_hpd(struct anx78xx *anx78xx) >> +{ >> + int err = 0; >> + >> + err |= anx78xx_clear_bits(anx78xx->map[I2C_IDX_RX_P0], >> + SP_TMDS_CTRL_BASE + 7, SP_PD_RT); >> + err |= anx78xx_set_bits(anx78xx->map[I2C_IDX_TX_P2], SP_VID_CTRL3_REG, >> + SP_HPD_OUT); > > Again, OR'ing of error codes, please don't do it. There are some more > occurrences below. > >> +static int anx78xx_init_gpio(struct anx78xx *anx78xx) >> +{ >> + struct device *dev = &anx78xx->client->dev; >> + struct anx78xx_platform_data *pdata = &anx78xx->pdata; >> + >> + /* GPIO for hpd */ > > HPD being an abbreviation it should be capitalized. Similar for a couple > of other abbreviations, some of which are inconsistently capitalized. In > variable names of consumer names, the lowercase variant is fine, but the > variant used in text (messages, comments) should be the all caps one. > >> + pdata->gpiod_hpd = devm_gpiod_get(dev, "hpd", GPIOD_IN); >> + if (IS_ERR(pdata->gpiod_hpd)) >> + return PTR_ERR(pdata->gpiod_hpd); >> + >> + /* GPIO for chip power down */ >> + pdata->gpiod_pd = devm_gpiod_get(dev, "pd", GPIOD_OUT_HIGH); >> + if (IS_ERR(pdata->gpiod_pd)) >> + return PTR_ERR(pdata->gpiod_pd); >> + >> + /* GPIO for chip reset */ >> + pdata->gpiod_reset = devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW); >> + if (IS_ERR(pdata->gpiod_reset)) >> + return PTR_ERR(pdata->gpiod_reset); >> + >> + /* GPIO for V10 power control */ >> + pdata->gpiod_v10 = devm_gpiod_get_optional(dev, "v10", GPIOD_OUT_LOW); > > Does this actually supply power? If so it should be modelled as a > regulator. > >> +static int anx78xx_dp_link_training(struct anx78xx *anx78xx) >> +{ >> + int err = 0; >> + u8 dp_bw, regval; >> + >> + err |= regmap_write(anx78xx->map[I2C_IDX_RX_P0], SP_HDMI_MUTE_CTRL_REG, >> + 0x0); >> + err |= anx78xx_clear_bits(anx78xx->map[I2C_IDX_TX_P2], >> + SP_POWERDOWN_CTRL_REG, >> + SP_TOTAL_PD); >> + if (err) >> + return -EIO; >> + >> + err = drm_dp_dpcd_readb(&anx78xx->aux, DP_MAX_LINK_RATE, &dp_bw); >> + if (err < 0) >> + return err; >> + >> + switch (dp_bw) { >> + case DP_LINK_BW_1_62: >> + case DP_LINK_BW_2_7: >> + case DP_LINK_BW_5_4: >> + break; >> + default: >> + DRM_DEBUG_KMS("Waiting to read DP bandwidth.\n"); >> + return -EAGAIN; >> + } > > That sounds wrong. Either you can read the content and it should be a > valid value (albeit one which you may not support) or you can't. Why do > you need to potentially repeat this read? > >> + >> + err = anx78xx_set_bits(anx78xx->map[I2C_IDX_TX_P2], SP_VID_CTRL1_REG, >> + SP_VIDEO_MUTE); >> + err |= anx78xx_clear_bits(anx78xx->map[I2C_IDX_TX_P2], >> + SP_VID_CTRL1_REG, SP_VIDEO_EN); >> + if (err) >> + return -EIO; >> + >> + /* Get dpcd info */ > > s/dpcd/DPCD/ > >> + err = drm_dp_dpcd_read(&anx78xx->aux, DP_DPCD_REV, >> + &anx78xx->dpcd, DP_RECEIVER_CAP_SIZE); >> + if (err < 0) { >> + DRM_ERROR("Failed to read DPCD\n"); > > It's often useful to output the error code as part of the error message > to make it easier for developers to diagnose problems. > >> + return err; >> + } >> + >> + /* Clear channel x SERDES power down */ >> + err = anx78xx_clear_bits(anx78xx->map[I2C_IDX_TX_P0], >> + SP_DP_ANALOG_POWER_DOWN_REG, SP_CH0_PD); >> + if (err) >> + return -EIO; >> + >> + /* Check link capabilities */ >> + err = drm_dp_link_probe(&anx78xx->aux, &anx78xx->link); >> + if (err < 0) { >> + DRM_ERROR("Failed to probe link capabilities\n"); >> + return err; >> + } >> + >> + /* Power up the sink */ >> + err = drm_dp_link_power_up(&anx78xx->aux, &anx78xx->link); >> + if (err < 0) { >> + DRM_ERROR("Failed to power up DisplayPort link\n"); >> + return err; >> + } >> + >> + /* Possibly enable downspread on the sink */ >> + err = regmap_write(anx78xx->map[I2C_IDX_TX_P0], >> + SP_DP_DOWNSPREAD_CTRL1_REG, 0); >> + if (err) >> + return err; >> + >> + if (anx78xx->dpcd[3] & 0x1) { > > This should use the symbolic constants defined in drm_dp_helper.h. > Actually, it should probably add a symbolic constant because we don't > have one yet for bit 0 in the DP_MAX_DOWNSPREAD register. > >> +static inline struct anx78xx * >> + connector_to_anx78xx(struct drm_connector *connector) > > The function name should start on the first column. Also you might want > to move this inline function after the struct anx78xx declaration, which > is more consistent with other drivers. > >> +static const struct drm_connector_helper_funcs >> + anx78xx_connector_helper_funcs = { > > The structure name should start in the first column as well. > >> + .get_modes = anx78xx_get_modes, >> + .best_encoder = anx78xx_best_encoder, >> +}; >> + >> +static enum drm_connector_status anx78xx_detect(struct drm_connector *connector, >> + bool force) >> +{ >> + struct anx78xx *anx78xx = container_of(connector, struct anx78xx, >> + connector); > > Didn't you introduce an inline function for this just a few lines above? > >> + DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n", connector->base.id, >> + connector->name); >> + >> + if (!gpiod_get_value(anx78xx->pdata.gpiod_hpd)) { >> + DRM_DEBUG_KMS("CONNECTOR STATUS DISCONNECTED\n"); >> + return connector_status_disconnected; >> + } >> + >> + return connector_status_connected; >> +} > > Is it really necessary to add two debug messages here? The DRM core will > already output a message for connector status changes, so this is > unnecessarily noisy in my opinion. > >> +static inline struct anx78xx *bridge_to_anx78xx(struct drm_bridge *bridge) >> +{ >> + return container_of(bridge, struct anx78xx, bridge); >> +} > > Put this together with the connector cast function. > >> +static void anx78xx_bridge_disable(struct drm_bridge *bridge) >> +{ >> + struct anx78xx *anx78xx = bridge_to_anx78xx(bridge); >> + >> + mutex_lock(&anx78xx->lock); > > Do you really need the locking here and below? I think the DRM core > already ensures that these calls are always serialized. > >> +static void anx78xx_bridge_mode_set(struct drm_bridge *bridge, >> + struct drm_display_mode *mode, >> + struct drm_display_mode *adjusted_mode) >> +{ >> + int err; >> + struct hdmi_avi_infoframe frame; >> + struct anx78xx *anx78xx = bridge_to_anx78xx(bridge); >> + >> + if (WARN_ON(!anx78xx->powered)) >> + return; >> + >> + mutex_lock(&anx78xx->lock); >> + >> + mode = adjusted_mode; >> + >> + err = drm_hdmi_avi_infoframe_from_display_mode(&frame, mode); > > This seems like jumping through hoops. Why not simply pass adjusted_mode > to the function? > >> +static const struct drm_bridge_funcs anx78xx_bridge_funcs = { >> + .attach = anx78xx_bridge_attach, >> + .mode_fixup = anx78xx_bridge_mode_fixup, >> + .disable = anx78xx_bridge_disable, >> + .mode_set = anx78xx_bridge_mode_set, >> + .enable = anx78xx_bridge_enable, >> +}; > > I'd leave out the tab-padding. Simple spaces will do just fine. > >> +static irqreturn_t anx78xx_hpd_threaded_handler(int unused, void *data) > > Might as well give the first parameter the proper name (irq). > >> +static bool anx78xx_handle_common_int_4(struct anx78xx *anx78xx, u8 irq) >> +{ >> + int err; >> + bool event = false; >> + >> + DRM_DEBUG_KMS("Handle common interrupt 4: %02x\n", irq); >> + >> + err = regmap_write(anx78xx->map[I2C_IDX_TX_P2], >> + SP_COMMON_INT_STATUS4_REG, irq); >> + if (err) { >> + DRM_ERROR("Failed to write SP_COMMON_INT_STATUS4 %d\n", err); >> + return event; >> + } >> + >> + if (irq & SP_HPD_LOST) { >> + DRM_DEBUG_KMS("IRQ: Hot plug detect - cable is pulled out\n"); >> + event = true; >> + anx78xx_poweroff(anx78xx); >> + } else if (irq & SP_HPD_PLUG) { >> + DRM_DEBUG_KMS("IRQ: Hot plug detect - cable plug\n"); >> + event = true; >> + /* Free previous cached EDID if any */ >> + kfree(anx78xx->edid); >> + anx78xx->edid = NULL; > > I think you can free the EDID on unplug, since it becomes stale at that > point already. The DRM core will also remove the EDID data on unplug. > >> +static void unregister_i2c_dummy_clients(struct anx78xx *anx78xx) >> +{ >> + int i; > > unsigned int > >> + >> + for (i = 0; i < ARRAY_SIZE(anx78xx->i2c_dummy); i++) >> + if (anx78xx->i2c_dummy[i]) >> + i2c_unregister_device(anx78xx->i2c_dummy[i]); >> +} >> + >> +static const struct regmap_config anx78xx_regmap_config = { >> + .reg_bits = 8, >> + .val_bits = 8, >> +}; >> + >> +static const u16 anx78xx_chipid_list[] = { >> + 0x7812, >> + 0x7814, >> + 0x7818, >> +}; >> + >> +static int anx78xx_i2c_probe(struct i2c_client *client, >> + const struct i2c_device_id *id) >> +{ >> + struct anx78xx *anx78xx; >> + struct anx78xx_platform_data *pdata; >> + int err, i; > > i should be unsigned int. > >> + unsigned int idl, idh, version; >> + bool found = false; >> + >> + anx78xx = devm_kzalloc(&client->dev, sizeof(*anx78xx), GFP_KERNEL); >> + if (!anx78xx) >> + return -ENOMEM; >> + >> + pdata = &anx78xx->pdata; >> + >> + mutex_init(&anx78xx->lock); >> + >> +#if IS_ENABLED(CONFIG_OF) >> + anx78xx->bridge.of_node = client->dev.of_node; >> +#endif >> + >> + anx78xx->client = client; >> + i2c_set_clientdata(client, anx78xx); >> + >> + err = anx78xx_init_gpio(anx78xx); >> + if (err) { >> + DRM_ERROR("Failed to initialize gpios\n"); >> + return err; >> + } >> + >> + pdata->hpd_irq = gpiod_to_irq(pdata->gpiod_hpd); >> + if (pdata->hpd_irq < 0) { >> + DRM_ERROR("Failed to get hpd irq %d\n", >> + pdata->hpd_irq); >> + return -ENODEV; >> + } >> + >> + pdata->intp_irq = client->irq; >> + if (!pdata->intp_irq) { >> + DRM_ERROR("Failed to get CABLE_DET and INTP irq\n"); >> + return -ENODEV; >> + } >> + >> + /* Map slave addresses of ANX7814 */ >> + for (i = 0; i < I2C_NUM_ADDRESSES; i++) { >> + anx78xx->i2c_dummy[i] = i2c_new_dummy(client->adapter, >> + anx78xx_i2c_addresses[i] >> 1); >> + if (!anx78xx->i2c_dummy[i]) { >> + err = -ENOMEM; >> + DRM_ERROR("Failed to reserve i2c bus %02x.\n", >> + anx78xx_i2c_addresses[i]); >> + goto err_unregister_i2c; >> + } >> + >> + anx78xx->map[i] = devm_regmap_init_i2c(anx78xx->i2c_dummy[i], >> + &anx78xx_regmap_config); >> + if (IS_ERR(anx78xx->map[i])) { >> + err = PTR_ERR(anx78xx->map[i]); >> + DRM_ERROR("Failed regmap initialization %02x.\n", >> + anx78xx_i2c_addresses[i]); >> + goto err_unregister_i2c; >> + } >> + } > > That's quite some overhead merely to use regmap... Perhaps there's room > to enhance regmap-i2c to support multiple addresses for the same device? > >> +static int anx78xx_i2c_remove(struct i2c_client *client) >> +{ >> + struct anx78xx *anx78xx = i2c_get_clientdata(client); >> + >> + drm_bridge_remove(&anx78xx->bridge); >> + >> + unregister_i2c_dummy_clients(anx78xx); >> + >> + kfree(anx78xx->edid); >> + anx78xx->edid = NULL; > > The memory pointed at by anx78xx will be freed a couple of instructions > later, there's no need to set ->edid to NULL. > > Thierry > Enric -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html