Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1392045 > unrolled thread

[RESEND PATCH v4 0/4] Add ANX7814 I2C bridge driver

Back to article view | Back to linux.kernel

Contents

  [RESEND PATCH v4 0/4] Add ANX7814 I2C bridge driver Enric Balletbo i Serra <enric.balletbo@collabora.com> - 2016-05-02 10:00 +0200
    Re: [RESEND PATCH v4 4/4] drm: bridge: anx78xx: Add anx78xx driver  support. Thierry Reding <thierry.reding@gmail.com> - 2016-05-02 16:30 +0200
      Re: [RESEND PATCH v4 4/4] drm: bridge: anx78xx: Add anx78xx driver support. Enric Balletbo Serra <eballetbo@gmail.com> - 2016-05-02 17:30 +0200

#1392045 — [RESEND PATCH v4 0/4] Add ANX7814 I2C bridge driver

Hi all,

This is the fourth version of the patch, thanks to all that did some comments.

In this version I basically added the comments made by Thierry, also it
introduces a new patch in the series to add a new helper constant in
drm_dp_helper.h. The devicetree binding also suffered a slight modification, one
gpio is modelled as a regulator now, for that reason, I removed the ack from Rob,
so please if you're ok with the change ack again.

This patch set to introduces the anx7814 slimport transmitter driver. These
new series will replace the old series that can be found here [1]. The reason
why I introduce these new series is because the driver changed significantly.
The old approach used a polled state machine ans was not really well using the
kernel mode setting API. With this new driver I tried to use better the drm API
and use an interrupt driven model.

Waiting for new comments ...

Changes since v3:
 - Introduce new patch to define the DP_PERCENT_DOWNSPREAD_0_5 constant needed
   by the driver.
 - Thierry Reding:
   - Add vendor name on symbols and files
   - Remove <linux/async.h> include
   - Improve anx78xx_aux_wait code (remove atomic case)
   - Fix msg->reply handling
   - Capitalize HPD/HDMI/DP
   - Don't return -EAGAIN on link training
   - Use symbolic constants defined instead of magic numbers
   - Move connector_to_anx78xx after the anx78xx declaration
   - Improve some coding style
     - Give the first parameter the proper name (irq)
     - Leave out the tab-padding in anx78xx_bridge_funcs
     - Put bridge_to_anx78xx together with the connector cast function.
      - The structure name should start in the first column
   - Remove messages for connector status changes
   - Free the EDID on unplug instead of on plug event
   - Use unsigned int for i in for loops.
   - Model v10 as regulator.
   - Remove the use of OR'ed errors
   - Simply pass adjusted_mode in bridge mode set.
   - Remove bridge_enable/disable mutex protection, not needed.
   - There's no need to set ->edid to NULL when remove the driver.

Changes since v2:
 - Add Acked-by: Rob Herring <robh@kernel.org> for patch 0002
 - Add Tested-by: Nicolas Boichat <drinkcat@chromium.org>
 - Add Reviewed-by: Nicolas Boichat <drinkcat@chromium.org>
 - 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.

Changes since v1:
 - Add Acked-by: Rob Herring <robh@kernel.org> for patch 0001
 - Dan Carpenter: 
   - Fix missing error code
   - Use meaningful names for goto exit paths
 - Rob Herring:
   - Rename cable-det-gpios for hpd-gpios as is more standard
   - Fix HDMI output for HDMI input
   - 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

[1] https://lwn.net/Articles/666885/


Enric Balletbo i Serra (4):
  drm/dp: Add define to set 0.5% down-spread in MAX_DOWNSPREAD register.
  of: Add vendor prefix for Analogix Semiconductor
  devicetree: Add ANX7814 SlimPort transmitter binding.
  drm: bridge: anx78xx: Add anx78xx driver support.

 .../devicetree/bindings/vendor-prefixes.txt        |    1 +
 .../devicetree/bindings/video/bridge/anx7814.txt   |   40 +
 drivers/gpu/drm/bridge/Kconfig                     |   10 +
 drivers/gpu/drm/bridge/Makefile                    |    1 +
 drivers/gpu/drm/bridge/analogix-anx78xx.c          | 1499 ++++++++++++++++++++
 drivers/gpu/drm/bridge/analogix-anx78xx.h          |  719 ++++++++++
 include/drm/drm_dp_helper.h                        |    1 +
 7 files changed, 2271 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/video/bridge/anx7814.txt
 create mode 100644 drivers/gpu/drm/bridge/analogix-anx78xx.c
 create mode 100644 drivers/gpu/drm/bridge/analogix-anx78xx.h

-- 
2.1.0

[toc] | [next] | [standalone]

#1392234 — Re: [RESEND PATCH v4 4/4] drm: bridge: anx78xx: Add anx78xx driver support.

[Multipart message — attachments visible in raw view] — view raw

On Mon, May 02, 2016 at 09:54:26AM +0200, Enric Balletbo i Serra wrote:
[...]
> diff --git a/drivers/gpu/drm/bridge/analogix-anx78xx.c b/drivers/gpu/drm/bridge/analogix-anx78xx.c
[...]
> +static int anx78xx_init_pdata(struct anx78xx *anx78xx)
> +{
> +	struct device *dev = &anx78xx->client->dev;
> +	struct anx78xx_platform_data *pdata = &anx78xx->pdata;
> +
> +	/* 1.0V digital core power regulator (optional) */
> +	pdata->dvdd10 = devm_regulator_get(dev, "dvdd10");
> +	if (IS_ERR(pdata->dvdd10)) {
> +		DRM_INFO("DVDD10 regulator not found\n");
> +		pdata->dvdd10 = NULL;
> +	}

I'm almost sure that this isn't what you want. What if the regulator is
hooked up but the bridge driver probes before the regulator. I think
what you really want is to simply propagate the error code via:

	return PTR_ERR(pdata->dvdd10);

My understanding is that the regulator core will give you a dummy one if
there's really nothing hooked up. I think you're also supposed to call
regulator_get_optional() (or the devm_*() equivalent) if this is truly
an optional supply. Given that it's the "core power" regulator I doubt
that it's really optional; it's more likely that you may not be able to
control it, and that it's therefore always on. In that case you're
supposed to model it in DT as a fixed regulator that's always on.

This is fairly minor and it's really the only thing I could find, so no
need to respin just for that. If you're fine with the above solution (to
propagate the error code) I can make the change manually while applying.

Thierry

[toc] | [prev] | [next] | [standalone]

#1392290 — Re: [RESEND PATCH v4 4/4] drm: bridge: anx78xx: Add anx78xx driver support.

Hi Thierry,

2016-05-02 16:22 GMT+02:00 Thierry Reding <thierry.reding@gmail.com>:
> On Mon, May 02, 2016 at 09:54:26AM +0200, Enric Balletbo i Serra wrote:
> [...]
>> diff --git a/drivers/gpu/drm/bridge/analogix-anx78xx.c b/drivers/gpu/drm/bridge/analogix-anx78xx.c
> [...]
>> +static int anx78xx_init_pdata(struct anx78xx *anx78xx)
>> +{
>> +     struct device *dev = &anx78xx->client->dev;
>> +     struct anx78xx_platform_data *pdata = &anx78xx->pdata;
>> +
>> +     /* 1.0V digital core power regulator (optional) */
>> +     pdata->dvdd10 = devm_regulator_get(dev, "dvdd10");
>> +     if (IS_ERR(pdata->dvdd10)) {
>> +             DRM_INFO("DVDD10 regulator not found\n");
>> +             pdata->dvdd10 = NULL;
>> +     }
>
> I'm almost sure that this isn't what you want. What if the regulator is
> hooked up but the bridge driver probes before the regulator. I think
> what you really want is to simply propagate the error code via:
>
>         return PTR_ERR(pdata->dvdd10);
>
> My understanding is that the regulator core will give you a dummy one if
> there's really nothing hooked up. I think you're also supposed to call
> regulator_get_optional() (or the devm_*() equivalent) if this is truly
> an optional supply. Given that it's the "core power" regulator I doubt
> that it's really optional; it's more likely that you may not be able to
> control it, and that it's therefore always on. In that case you're
> supposed to model it in DT as a fixed regulator that's always on.
>

Yes, I was thinking in the case you're are not able to control it and
I make it as optional, and yes, you have reason so It's fine with me
the above solution.

> This is fairly minor and it's really the only thing I could find, so no
> need to respin just for that. If you're fine with the above solution (to
> propagate the error code) I can make the change manually while applying.
>

It's up to you. Whichever you prefer.


Thanks,
 Enric


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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web