All of lore.kernel.org
 help / color / mirror / Atom feed
From: Emil Velikov <emil.l.velikov@gmail.com>
To: Vinod Koul <vkoul@kernel.org>
Cc: Daniel Vetter <daniel@ffwll.ch>, David Airlie <airlied@linux.ie>,
	Rob Clark <robdclark@gmail.com>,
	devicetree <devicetree@vger.kernel.org>,
	Jernej Skrabec <jernej.skrabec@siol.net>,
	Neil Armstrong <narmstrong@baylibre.com>,
	linux-arm-msm <linux-arm-msm@vger.kernel.org>,
	Jonas Karlman <jonas@kwiboo.se>,
	"Linux-Kernel@Vger. Kernel. Org" <linux-kernel@vger.kernel.org>,
	ML dri-devel <dri-devel@lists.freedesktop.org>,
	Bjorn Andersson <bjorn.andersson@linaro.org>,
	Andrzej Hajda <a.hajda@samsung.com>,
	Rob Herring <robh+dt@kernel.org>,
	Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
Subject: Re: [PATCH 3/3] drm/bridge: Introduce LT9611 DSI to HDMI bridge
Date: Wed, 13 May 2020 20:20:56 +0100	[thread overview]
Message-ID: <CACvgo502+8YroB5QtnGYFeSu92s_vpe_M3cPFeuC77u9xpanXQ@mail.gmail.com> (raw)
In-Reply-To: <20200513100533.42996-4-vkoul@kernel.org>

Hi Vinod,

Few high-level comments:
 - handful of functions always return 0 and the return value is never
checked - switch to return void
 - annotate all (nearly) arrays as static const
 - consistently use multi_reg_write - in some cases non-const array
will be fine, overwriting a few entries as needed
 - there is very partial comments about the registers/values - missing docs or?

Personally I'm in favour of using symbolic names, instead of
hex+comment. Considering how partial the comments are, current
approach is perfectly fine.

On Wed, 13 May 2020 at 11:06, Vinod Koul <vkoul@kernel.org> wrote:
>
> Lontium Lt9611 is a DSI to HDMI bridge which supports two DSI ports and
> I2S port as an input and HDMI port as output
>
> Co-developed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> Signed-off-by: Vinod Koul <vkoul@kernel.org>

> --- /dev/null
> +++ b/drivers/gpu/drm/bridge/lt9611.c

Please add a vendor prefix to the filename.

> @@ -0,0 +1,1113 @@

> +struct lt9611_mode {
> +       u16 hdisplay;
> +       u16 vdisplay;
> +       u8 fps;
We all enjoy the odd fps game, but let's use vrefresh here.

> +       u8 lanes;
> +       u8 intfs;
> +};
> +


> +static int lt9611_mipi_input_digital(struct lt9611 *lt9611,
> +                                    const struct drm_display_mode *mode)
> +{
> +       regmap_write(lt9611->regmap, 0x8300, LT9611_4LANES);
> +
> +       if (mode->hdisplay == 3840)
> +               regmap_write(lt9611->regmap, 0x830a, 0x03);
> +       else
> +               regmap_write(lt9611->regmap, 0x830a, 0x00);
> +
> +       regmap_write(lt9611->regmap, 0x824f, 0x80);
> +       regmap_write(lt9611->regmap, 0x8250, 0x10);
> +       regmap_write(lt9611->regmap, 0x8302, 0x0a);
> +       regmap_write(lt9611->regmap, 0x8306, 0x0a);
Create an (non-const) array, overwriting the [1] entry for 3840 mode?

> +
> +       return 0;
Kill return type.

> +}

> +static int lt9611_pcr_setup(struct lt9611 *lt9611,
> +                           const struct drm_display_mode *mode)
> +{
> +       struct reg_sequence reg_cfg[] = {
static const?

> +               { 0x830b, 0x01 },
> +               { 0x830c, 0x10 },
> +               { 0x8348, 0x00 },
> +               { 0x8349, 0x81 },
> +
> +               /* stage 1 */
> +               { 0x8321, 0x4a },
> +               { 0x8324, 0x71 },
> +               { 0x8325, 0x30 },
> +               { 0x832a, 0x01 },
> +
> +               /* stage 2 */
> +               { 0x834a, 0x40 },
> +               { 0x831d, 0x10 },
> +
> +               /* MK limit */
> +               { 0x832d, 0x38 },
> +               { 0x8331, 0x08 },
> +       };
> +
> +       regmap_multi_reg_write(lt9611->regmap, reg_cfg, ARRAY_SIZE(reg_cfg));
> +
> +       switch (mode->hdisplay) {
> +       case 640:
> +               regmap_write(lt9611->regmap, 0x8326, 0x14);
> +               break;
> +       case 1920:
> +               regmap_write(lt9611->regmap, 0x8326, 0x37);
> +               break;
> +       case 3840:
> +               regmap_write(lt9611->regmap, 0x830b, 0x03);
> +               regmap_write(lt9611->regmap, 0x830c, 0xd0);
> +               regmap_write(lt9611->regmap, 0x8348, 0x03);
> +               regmap_write(lt9611->regmap, 0x8349, 0xe0);
> +               regmap_write(lt9611->regmap, 0x8324, 0x72);
> +               regmap_write(lt9611->regmap, 0x8325, 0x00);
> +               regmap_write(lt9611->regmap, 0x832a, 0x01);
> +               regmap_write(lt9611->regmap, 0x834a, 0x10);
> +               regmap_write(lt9611->regmap, 0x831d, 0x10);
> +               regmap_write(lt9611->regmap, 0x8326, 0x37);
Throw this in another const array?

> +               break;
> +       }
> +
> +       /* pcr rst */
> +       regmap_write(lt9611->regmap, 0x8011, 0x5a);
> +       regmap_write(lt9611->regmap, 0x8011, 0xfa);
> +
> +       return 0;
> +}


> +       regmap_write(lt9611->regmap, 0x82e3, pclk >> 17); /* pclk[19:16] */
> +       regmap_write(lt9611->regmap, 0x82e4, pclk >> 9);  /* pclk[15:8]  */
> +       regmap_write(lt9611->regmap, 0x82e5, pclk >> 1);  /* pclk[7:0]   */
Comment does not match the code.
We're discarding the LSB, so we cannot realistically be writing
pclk[7:0]. Similar applies for the other two.


> +       /* v_act */
> +       ret = regmap_read(lt9611->regmap, 0x8282, &temp);
> +       if (ret)
> +               goto end;
> +
> +       v_act = temp << 8;
> +       ret = regmap_read(lt9611->regmap, 0x8283, &temp);
> +       if (ret)
> +               goto end;
> +       v_act = v_act + temp;
> +
Having a helper for the above "result = read(x) << 8 | read(x+1)"
would be great.
This way one doesn't have to repeat the pattern 4-5 times.


> +static int lt9611_read_edid(struct lt9611 *lt9611)
> +{
> +       unsigned int temp;
> +       int ret = 0;
> +       int i, j;
> +
> +       memset(lt9611->edid_buf, 0, EDID_SEG_SIZE);
How about:
  memset(lt9611->edid_buf, 0, sizeof(lt9611->edid_buf));

Then again, do we need the memset()? We are allocating the memory with
devm_kzalloc()

> +
> +       regmap_write(lt9611->regmap, 0x8503, 0xc9);
> +
> +       /* 0xA0 is EDID device address */
> +       regmap_write(lt9611->regmap, 0x8504, 0xa0);
> +       /* 0x00 is EDID offset address */
> +       regmap_write(lt9611->regmap, 0x8505, 0x00);
> +       /* length for read */
> +       regmap_write(lt9611->regmap, 0x8506, 0x20);
Is this the same 32 as seen in the loops below? #define and use consistently?

> +       regmap_write(lt9611->regmap, 0x8514, 0x7f);
> +
> +       for (i = 0 ; i < 8 ; i++) {
Add a #define for the magic 8

> +               /* offset address */
> +               regmap_write(lt9611->regmap, 0x8505, i * 32);
> +               regmap_write(lt9611->regmap, 0x8507, 0x36);
> +               regmap_write(lt9611->regmap, 0x8507, 0x31);
> +               regmap_write(lt9611->regmap, 0x8507, 0x37);
> +               usleep_range(5000, 10000);
> +
> +               regmap_read(lt9611->regmap, 0x8540, &temp);
> +
> +               if (temp & 0x02) {  /*KEY_DDC_ACCS_DONE=1*/
Use #define KEY_DDC_ACCS_DONE 0x02

> +                       for (j = 0; j < 32; j++) {
Another #define for 32

> +                               regmap_read(lt9611->regmap, 0x8583, &temp);
> +                               lt9611->edid_buf[i * 32 + j] = temp;
> +                       }
> +               } else if (temp & 0x50) { /* DDC No Ack or Abitration lost */
> +                       dev_err(lt9611->dev, "read edid failed: no ack\n");
> +                       ret = -EIO;
> +                       goto end;
> +               } else {
> +                       dev_err(lt9611->dev,
> +                               "read edid failed: access not done\n");
> +                       ret = -EIO;
> +                       goto end;
> +               }
> +       }
> +
> +       dev_dbg(lt9611->dev, "read edid succeeded, checksum = 0x%x\n",
> +               lt9611->edid_buf[255]);
> +
> +end:
> +       regmap_write(lt9611->regmap, 0x8507, 0x1f);
> +       return ret;
> +}


> +
> +/* TODO: add support for more extension blocks */
> +static int
> +lt9611_get_edid_block(void *data, u8 *buf, unsigned int block, size_t len)
> +{
> +       struct lt9611 *lt9611 = data;
> +       int ret;
> +
> +       dev_dbg(lt9611->dev, "get edid block: block=%d, len=%d\n",
> +               block, (int)len);
> +
> +       if (len > 128)
> +               return -EINVAL;
> +
> +       /* support up to 1 extension block */
Move the TODO here?

> +       if (block > 1)
> +               return -EINVAL;
> +
> +       if (block == 0) {
> +               /* always read 2 edid blocks once */
Please mention why that's a good idea. From memory - there aren't many
other drivers that do this.

> +               ret = lt9611_read_edid(lt9611);
> +               if (ret) {
> +                       dev_err(lt9611->dev, "edid read failed\n");
> +                       return ret;
> +               }
> +       }
> +
> +       if (block % 2 == 0)
> +               memcpy(buf, lt9611->edid_buf, len);
> +       else
> +               memcpy(buf, lt9611->edid_buf + 128, len);
The above can be written as:
   memcpy(buf, lt9611->edid_buf + (block * 128), len);

> +
> +       return 0;
> +}
> +

> +static int lt9611_bridge_attach(struct drm_bridge *bridge,
> +                               enum drm_bridge_attach_flags flags)
> +{

> +       /* Attach secondary DSI, if specified */
> +       if (lt9611->dsi1_node) {
> +               lt9611->dsi1 = lt9611_attach_dsi(lt9611, lt9611->dsi1_node);
> +               if (IS_ERR(lt9611->dsi1)) {
> +                       ret = PTR_ERR(lt9611->dsi1);
> +                       goto err_unregister_dsi0;
> +               }
> +       }
> +
> +       return 0;
> +
> +err_unregister_dsi0:
Missing detach? If possible directly use lt9611_bridge_detach().

> +       mipi_dsi_device_unregister(lt9611->dsi0);
> +
> +       return ret;
> +}
> +


> +static int lt9611_read_device_rev(struct lt9611 *lt9611)
> +{
> +       unsigned int rev;
> +       int ret;
> +
> +       regmap_write(lt9611->regmap, 0x80ee, 0x01);
> +       ret = regmap_read(lt9611->regmap, 0x8002, &rev);
> +       if (ret)
> +               dev_err(lt9611->dev, "failed to read revision: %d\n", ret);
> +
The "failed" message will be followed by printing random kernel memory.
Initialize rev to some dummy number or omit the dev_info.

> +       dev_info(lt9611->dev, "LT9611 revision: 0x%x\n", rev);
> +
> +       return ret;
> +}
> +
> +static int lt9611_probe(struct i2c_client *client,
> +                       const struct i2c_device_id *id)
> +{

> +       ret = lt9611_parse_dt(&client->dev, lt9611);
> +       if (ret) {
> +               dev_err(dev, "failed to parse device tree\n");
> +               return ret;
> +       }
> +
> +       ret = lt9611_gpio_init(lt9611);
> +       if (ret < 0)
Missing of_node_put() here and for the next few error paths.

> +               return ret;
> +
> +       ret = lt9611_regulator_init(lt9611);
> +       if (ret < 0)
> +               return ret;
> +
> +       lt9611_assert_5v(lt9611);
> +
> +       ret = lt9611_regulator_enable(lt9611);
> +       if (ret)
> +               return ret;
> +

> +       return 0;
> +
> +err_disable_regulators:
> +       regulator_bulk_disable(ARRAY_SIZE(lt9611->supplies), lt9611->supplies);
> +
> +       of_node_put(lt9611->dsi0_node);
> +       of_node_put(lt9611->dsi1_node);
Use the inverse order wrt the get() operation.

> +
> +       return ret;
> +}
> +
> +static int lt9611_remove(struct i2c_client *client)
> +{
> +       struct lt9611 *lt9611 = i2c_get_clientdata(client);
> +
> +       disable_irq(client->irq);
> +       drm_bridge_remove(&lt9611->bridge);
> +
> +       regulator_bulk_disable(ARRAY_SIZE(lt9611->supplies), lt9611->supplies);
> +
> +       of_node_put(lt9611->dsi0_node);
> +       of_node_put(lt9611->dsi1_node);
Flip the order - dsi1, then dsi0

> +
> +       return 0;
> +}
> +
> +static struct i2c_device_id lt9611_id[] = {
> +       { "lontium,lt9611", 0},
> +       {}
> +};
> +
> +static const struct of_device_id lt9611_match_table[] = {
> +       {.compatible = "lontium,lt9611"},
In the above two - add space after { and before }. Pretty sure
./scripts/checkpatch.pl will complain about those.
Might want to double-check for other issues reported by said tool.


-Emil

WARNING: multiple messages have this Message-ID
From: Emil Velikov <emil.l.velikov@gmail.com>
To: Vinod Koul <vkoul@kernel.org>
Cc: devicetree <devicetree@vger.kernel.org>,
	Jernej Skrabec <jernej.skrabec@siol.net>,
	Neil Armstrong <narmstrong@baylibre.com>,
	David Airlie <airlied@linux.ie>,
	linux-arm-msm <linux-arm-msm@vger.kernel.org>,
	Jonas Karlman <jonas@kwiboo.se>,
	"Linux-Kernel@Vger. Kernel. Org" <linux-kernel@vger.kernel.org>,
	ML dri-devel <dri-devel@lists.freedesktop.org>,
	Bjorn Andersson <bjorn.andersson@linaro.org>,
	Andrzej Hajda <a.hajda@samsung.com>,
	Rob Herring <robh+dt@kernel.org>,
	Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
Subject: Re: [PATCH 3/3] drm/bridge: Introduce LT9611 DSI to HDMI bridge
Date: Wed, 13 May 2020 20:20:56 +0100	[thread overview]
Message-ID: <CACvgo502+8YroB5QtnGYFeSu92s_vpe_M3cPFeuC77u9xpanXQ@mail.gmail.com> (raw)
In-Reply-To: <20200513100533.42996-4-vkoul@kernel.org>

Hi Vinod,

Few high-level comments:
 - handful of functions always return 0 and the return value is never
checked - switch to return void
 - annotate all (nearly) arrays as static const
 - consistently use multi_reg_write - in some cases non-const array
will be fine, overwriting a few entries as needed
 - there is very partial comments about the registers/values - missing docs or?

Personally I'm in favour of using symbolic names, instead of
hex+comment. Considering how partial the comments are, current
approach is perfectly fine.

On Wed, 13 May 2020 at 11:06, Vinod Koul <vkoul@kernel.org> wrote:
>
> Lontium Lt9611 is a DSI to HDMI bridge which supports two DSI ports and
> I2S port as an input and HDMI port as output
>
> Co-developed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> Signed-off-by: Vinod Koul <vkoul@kernel.org>

> --- /dev/null
> +++ b/drivers/gpu/drm/bridge/lt9611.c

Please add a vendor prefix to the filename.

> @@ -0,0 +1,1113 @@

> +struct lt9611_mode {
> +       u16 hdisplay;
> +       u16 vdisplay;
> +       u8 fps;
We all enjoy the odd fps game, but let's use vrefresh here.

> +       u8 lanes;
> +       u8 intfs;
> +};
> +


> +static int lt9611_mipi_input_digital(struct lt9611 *lt9611,
> +                                    const struct drm_display_mode *mode)
> +{
> +       regmap_write(lt9611->regmap, 0x8300, LT9611_4LANES);
> +
> +       if (mode->hdisplay == 3840)
> +               regmap_write(lt9611->regmap, 0x830a, 0x03);
> +       else
> +               regmap_write(lt9611->regmap, 0x830a, 0x00);
> +
> +       regmap_write(lt9611->regmap, 0x824f, 0x80);
> +       regmap_write(lt9611->regmap, 0x8250, 0x10);
> +       regmap_write(lt9611->regmap, 0x8302, 0x0a);
> +       regmap_write(lt9611->regmap, 0x8306, 0x0a);
Create an (non-const) array, overwriting the [1] entry for 3840 mode?

> +
> +       return 0;
Kill return type.

> +}

> +static int lt9611_pcr_setup(struct lt9611 *lt9611,
> +                           const struct drm_display_mode *mode)
> +{
> +       struct reg_sequence reg_cfg[] = {
static const?

> +               { 0x830b, 0x01 },
> +               { 0x830c, 0x10 },
> +               { 0x8348, 0x00 },
> +               { 0x8349, 0x81 },
> +
> +               /* stage 1 */
> +               { 0x8321, 0x4a },
> +               { 0x8324, 0x71 },
> +               { 0x8325, 0x30 },
> +               { 0x832a, 0x01 },
> +
> +               /* stage 2 */
> +               { 0x834a, 0x40 },
> +               { 0x831d, 0x10 },
> +
> +               /* MK limit */
> +               { 0x832d, 0x38 },
> +               { 0x8331, 0x08 },
> +       };
> +
> +       regmap_multi_reg_write(lt9611->regmap, reg_cfg, ARRAY_SIZE(reg_cfg));
> +
> +       switch (mode->hdisplay) {
> +       case 640:
> +               regmap_write(lt9611->regmap, 0x8326, 0x14);
> +               break;
> +       case 1920:
> +               regmap_write(lt9611->regmap, 0x8326, 0x37);
> +               break;
> +       case 3840:
> +               regmap_write(lt9611->regmap, 0x830b, 0x03);
> +               regmap_write(lt9611->regmap, 0x830c, 0xd0);
> +               regmap_write(lt9611->regmap, 0x8348, 0x03);
> +               regmap_write(lt9611->regmap, 0x8349, 0xe0);
> +               regmap_write(lt9611->regmap, 0x8324, 0x72);
> +               regmap_write(lt9611->regmap, 0x8325, 0x00);
> +               regmap_write(lt9611->regmap, 0x832a, 0x01);
> +               regmap_write(lt9611->regmap, 0x834a, 0x10);
> +               regmap_write(lt9611->regmap, 0x831d, 0x10);
> +               regmap_write(lt9611->regmap, 0x8326, 0x37);
Throw this in another const array?

> +               break;
> +       }
> +
> +       /* pcr rst */
> +       regmap_write(lt9611->regmap, 0x8011, 0x5a);
> +       regmap_write(lt9611->regmap, 0x8011, 0xfa);
> +
> +       return 0;
> +}


> +       regmap_write(lt9611->regmap, 0x82e3, pclk >> 17); /* pclk[19:16] */
> +       regmap_write(lt9611->regmap, 0x82e4, pclk >> 9);  /* pclk[15:8]  */
> +       regmap_write(lt9611->regmap, 0x82e5, pclk >> 1);  /* pclk[7:0]   */
Comment does not match the code.
We're discarding the LSB, so we cannot realistically be writing
pclk[7:0]. Similar applies for the other two.


> +       /* v_act */
> +       ret = regmap_read(lt9611->regmap, 0x8282, &temp);
> +       if (ret)
> +               goto end;
> +
> +       v_act = temp << 8;
> +       ret = regmap_read(lt9611->regmap, 0x8283, &temp);
> +       if (ret)
> +               goto end;
> +       v_act = v_act + temp;
> +
Having a helper for the above "result = read(x) << 8 | read(x+1)"
would be great.
This way one doesn't have to repeat the pattern 4-5 times.


> +static int lt9611_read_edid(struct lt9611 *lt9611)
> +{
> +       unsigned int temp;
> +       int ret = 0;
> +       int i, j;
> +
> +       memset(lt9611->edid_buf, 0, EDID_SEG_SIZE);
How about:
  memset(lt9611->edid_buf, 0, sizeof(lt9611->edid_buf));

Then again, do we need the memset()? We are allocating the memory with
devm_kzalloc()

> +
> +       regmap_write(lt9611->regmap, 0x8503, 0xc9);
> +
> +       /* 0xA0 is EDID device address */
> +       regmap_write(lt9611->regmap, 0x8504, 0xa0);
> +       /* 0x00 is EDID offset address */
> +       regmap_write(lt9611->regmap, 0x8505, 0x00);
> +       /* length for read */
> +       regmap_write(lt9611->regmap, 0x8506, 0x20);
Is this the same 32 as seen in the loops below? #define and use consistently?

> +       regmap_write(lt9611->regmap, 0x8514, 0x7f);
> +
> +       for (i = 0 ; i < 8 ; i++) {
Add a #define for the magic 8

> +               /* offset address */
> +               regmap_write(lt9611->regmap, 0x8505, i * 32);
> +               regmap_write(lt9611->regmap, 0x8507, 0x36);
> +               regmap_write(lt9611->regmap, 0x8507, 0x31);
> +               regmap_write(lt9611->regmap, 0x8507, 0x37);
> +               usleep_range(5000, 10000);
> +
> +               regmap_read(lt9611->regmap, 0x8540, &temp);
> +
> +               if (temp & 0x02) {  /*KEY_DDC_ACCS_DONE=1*/
Use #define KEY_DDC_ACCS_DONE 0x02

> +                       for (j = 0; j < 32; j++) {
Another #define for 32

> +                               regmap_read(lt9611->regmap, 0x8583, &temp);
> +                               lt9611->edid_buf[i * 32 + j] = temp;
> +                       }
> +               } else if (temp & 0x50) { /* DDC No Ack or Abitration lost */
> +                       dev_err(lt9611->dev, "read edid failed: no ack\n");
> +                       ret = -EIO;
> +                       goto end;
> +               } else {
> +                       dev_err(lt9611->dev,
> +                               "read edid failed: access not done\n");
> +                       ret = -EIO;
> +                       goto end;
> +               }
> +       }
> +
> +       dev_dbg(lt9611->dev, "read edid succeeded, checksum = 0x%x\n",
> +               lt9611->edid_buf[255]);
> +
> +end:
> +       regmap_write(lt9611->regmap, 0x8507, 0x1f);
> +       return ret;
> +}


> +
> +/* TODO: add support for more extension blocks */
> +static int
> +lt9611_get_edid_block(void *data, u8 *buf, unsigned int block, size_t len)
> +{
> +       struct lt9611 *lt9611 = data;
> +       int ret;
> +
> +       dev_dbg(lt9611->dev, "get edid block: block=%d, len=%d\n",
> +               block, (int)len);
> +
> +       if (len > 128)
> +               return -EINVAL;
> +
> +       /* support up to 1 extension block */
Move the TODO here?

> +       if (block > 1)
> +               return -EINVAL;
> +
> +       if (block == 0) {
> +               /* always read 2 edid blocks once */
Please mention why that's a good idea. From memory - there aren't many
other drivers that do this.

> +               ret = lt9611_read_edid(lt9611);
> +               if (ret) {
> +                       dev_err(lt9611->dev, "edid read failed\n");
> +                       return ret;
> +               }
> +       }
> +
> +       if (block % 2 == 0)
> +               memcpy(buf, lt9611->edid_buf, len);
> +       else
> +               memcpy(buf, lt9611->edid_buf + 128, len);
The above can be written as:
   memcpy(buf, lt9611->edid_buf + (block * 128), len);

> +
> +       return 0;
> +}
> +

> +static int lt9611_bridge_attach(struct drm_bridge *bridge,
> +                               enum drm_bridge_attach_flags flags)
> +{

> +       /* Attach secondary DSI, if specified */
> +       if (lt9611->dsi1_node) {
> +               lt9611->dsi1 = lt9611_attach_dsi(lt9611, lt9611->dsi1_node);
> +               if (IS_ERR(lt9611->dsi1)) {
> +                       ret = PTR_ERR(lt9611->dsi1);
> +                       goto err_unregister_dsi0;
> +               }
> +       }
> +
> +       return 0;
> +
> +err_unregister_dsi0:
Missing detach? If possible directly use lt9611_bridge_detach().

> +       mipi_dsi_device_unregister(lt9611->dsi0);
> +
> +       return ret;
> +}
> +


> +static int lt9611_read_device_rev(struct lt9611 *lt9611)
> +{
> +       unsigned int rev;
> +       int ret;
> +
> +       regmap_write(lt9611->regmap, 0x80ee, 0x01);
> +       ret = regmap_read(lt9611->regmap, 0x8002, &rev);
> +       if (ret)
> +               dev_err(lt9611->dev, "failed to read revision: %d\n", ret);
> +
The "failed" message will be followed by printing random kernel memory.
Initialize rev to some dummy number or omit the dev_info.

> +       dev_info(lt9611->dev, "LT9611 revision: 0x%x\n", rev);
> +
> +       return ret;
> +}
> +
> +static int lt9611_probe(struct i2c_client *client,
> +                       const struct i2c_device_id *id)
> +{

> +       ret = lt9611_parse_dt(&client->dev, lt9611);
> +       if (ret) {
> +               dev_err(dev, "failed to parse device tree\n");
> +               return ret;
> +       }
> +
> +       ret = lt9611_gpio_init(lt9611);
> +       if (ret < 0)
Missing of_node_put() here and for the next few error paths.

> +               return ret;
> +
> +       ret = lt9611_regulator_init(lt9611);
> +       if (ret < 0)
> +               return ret;
> +
> +       lt9611_assert_5v(lt9611);
> +
> +       ret = lt9611_regulator_enable(lt9611);
> +       if (ret)
> +               return ret;
> +

> +       return 0;
> +
> +err_disable_regulators:
> +       regulator_bulk_disable(ARRAY_SIZE(lt9611->supplies), lt9611->supplies);
> +
> +       of_node_put(lt9611->dsi0_node);
> +       of_node_put(lt9611->dsi1_node);
Use the inverse order wrt the get() operation.

> +
> +       return ret;
> +}
> +
> +static int lt9611_remove(struct i2c_client *client)
> +{
> +       struct lt9611 *lt9611 = i2c_get_clientdata(client);
> +
> +       disable_irq(client->irq);
> +       drm_bridge_remove(&lt9611->bridge);
> +
> +       regulator_bulk_disable(ARRAY_SIZE(lt9611->supplies), lt9611->supplies);
> +
> +       of_node_put(lt9611->dsi0_node);
> +       of_node_put(lt9611->dsi1_node);
Flip the order - dsi1, then dsi0

> +
> +       return 0;
> +}
> +
> +static struct i2c_device_id lt9611_id[] = {
> +       { "lontium,lt9611", 0},
> +       {}
> +};
> +
> +static const struct of_device_id lt9611_match_table[] = {
> +       {.compatible = "lontium,lt9611"},
In the above two - add space after { and before }. Pretty sure
./scripts/checkpatch.pl will complain about those.
Might want to double-check for other issues reported by said tool.


-Emil
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

  parent reply	other threads:[~2020-05-13 19:23 UTC|newest]

Thread overview: 32+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-05-13 10:05 [PATCH 0/3] Add " Vinod Koul
2020-05-13 10:05 ` Vinod Koul
2020-05-13 10:05 ` [PATCH 1/3] dt-bindings: vendor-prefixes: Add Lontium vendor prefix Vinod Koul
2020-05-13 10:05   ` Vinod Koul
2020-05-28  1:34   ` Rob Herring
2020-05-28  1:34     ` Rob Herring
2020-05-13 10:05 ` [PATCH 2/3] dt-bindings: display: bridge: Add documentation for LT9611 Vinod Koul
2020-05-13 10:05   ` Vinod Koul
2020-05-28  1:37   ` Rob Herring
2020-05-28  1:37     ` Rob Herring
2020-05-28  1:48   ` Laurent Pinchart
2020-05-28  1:48     ` Laurent Pinchart
2020-06-04  7:18     ` Vinod Koul
2020-06-04  7:18       ` Vinod Koul
2020-06-04  7:29       ` Laurent Pinchart
2020-06-04  7:29         ` Laurent Pinchart
2020-05-13 10:05 ` [PATCH 3/3] drm/bridge: Introduce LT9611 DSI to HDMI bridge Vinod Koul
2020-05-13 10:05   ` Vinod Koul
2020-05-13 16:14   ` kbuild test robot
2020-05-13 16:14     ` kbuild test robot
2020-05-13 16:46   ` kbuild test robot
2020-05-13 16:46     ` kbuild test robot
2020-05-13 19:20   ` Emil Velikov [this message]
2020-05-13 19:20     ` Emil Velikov
2020-05-14  7:06     ` Vinod Koul
2020-05-14  7:06       ` Vinod Koul
2020-05-28  1:52   ` Laurent Pinchart
2020-05-28  1:52     ` Laurent Pinchart
2020-06-04  7:25     ` Vinod Koul
2020-06-04  7:25       ` Vinod Koul
2020-06-04  7:38       ` Laurent Pinchart
2020-06-04  7:38         ` Laurent Pinchart

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CACvgo502+8YroB5QtnGYFeSu92s_vpe_M3cPFeuC77u9xpanXQ@mail.gmail.com \
    --to=emil.l.velikov@gmail.com \
    --cc=Laurent.pinchart@ideasonboard.com \
    --cc=a.hajda@samsung.com \
    --cc=airlied@linux.ie \
    --cc=bjorn.andersson@linaro.org \
    --cc=daniel@ffwll.ch \
    --cc=devicetree@vger.kernel.org \
    --cc=dri-devel@lists.freedesktop.org \
    --cc=jernej.skrabec@siol.net \
    --cc=jonas@kwiboo.se \
    --cc=linux-arm-msm@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=narmstrong@baylibre.com \
    --cc=robdclark@gmail.com \
    --cc=robh+dt@kernel.org \
    --cc=vkoul@kernel.org \
    --subject='Re: [PATCH 3/3] drm/bridge: Introduce LT9611 DSI to HDMI bridge' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.