Message-ID: <570F9E4D.7040401@collabora.com>
Date:	Thu, 14 Apr 2016 15:42:37 +0200
From:	Enric Balletbo i Serra <enric.balletbo@...labora.com>
To:	Thierry Reding <treding@...dia.com>
CC:	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
	dri-devel@...ts.freedesktop.org, airlied@...ux.ie,
	robh+dt@...nel.org, djkurtz@...omium.org, drinkcat@...omium.org,
	dan.carpenter@...cle.com, Emil Velikov <emil.l.velikov@...il.com>,
	Rob Herring <robh@...nel.org>
Subject: Re: [PATCH v3 3/3] drm: bridge: anx78xx: Add anx78xx driver support.

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 <enric.balletbo@...labora.com>
>> Tested-by: Nicolas Boichat <drinkcat@...omium.org>
>> Reviewed-by: Nicolas Boichat <drinkcat@...omium.org>
>> Cc: Emil Velikov <emil.l.velikov@...il.com>
>> Cc: Rob Herring <robh@...nel.org>
>> Cc: Dan Carpenter <dan.carpenter@...cle.com>
>> Cc: Daniel Kurtz <djkurtz@...omium.org>
>> Cc: Nicolas Boichat <drinkcat@...omium.org>
>> ---
>> 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 <drinkcat@...omium.org>
>>   - Add Reviewed-by: Nicolas Boichat <drinkcat@...omium.org>
>>
>> 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 <linux/async.h>
>
> 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

Powered by blists - more mailing lists