Re: [PATCHv6 5/5] drm: bridge: anx78xx: Add anx78xx driver support by analogix.
Hi Dan,
Many thanks for your comments.
2015-12-07 9:09 GMT+01:00 Dan Carpenter :
> On Fri, Dec 04, 2015 at 09:35:07AM +0100, Enric Balletbo i Serra wrote:
>> +static int sp_wait_aux_op_finish(struct anx78xx *anx78xx)
>> +{
>> + u8 errcnt;
>> + u8 val;
>> + struct device *dev = &anx78xx->client->dev;
>> +
>> + errcnt = 150;
>> + while (errcnt--) {
>> + sp_reg_read(anx78xx, TX_P0, SP_DP_AUX_CH_CTRL2_REG, &val);
>> + if (!(val & SP_AUX_EN))
>> + break;
>> + usleep_range(2000, 4000);
>> + }
>> +
>> + if (!errcnt) {
>
> This is off by one. It should be:
>
> while (--errcnt) {
> ...
> }
> if (errcnt == 0)
> return -EWHATEVER;
>
> Or:
>
> while (errcnt--) {
> ...
> }
> if (errcnt == -1)
> return -EWHATEVER;
>
> Also "errcnt" is a bad name, it should be retry_cnt or something (or
> maybe it actually is counting errors?). Also -1 is never a correct
> error code, please change all the -1 returns to something better.
>
Ok, I renamed to retry_cnt and changed all the -1 values to something better.
>> + /* Buffer size of AUX CH is 16 */
>> + if (count > 16)
>> + return -1;
>
> Just make a define so that you don't need to add comments about why 16
> is correct.
>
> if (count > SIZE_AUX_CH)
> return -EINVAL;
>
Added a define
>> + errcnt = 10;
>> + while (errcnt--) {
>> + sp_reg_read(anx78xx, TX_P0, SP_DP_AUX_CH_CTRL2_REG, &val);
>> + if (!(val & SP_AUX_EN))
>> + break;
>> + usleep_range(1000, 2000);
>> + }
>> +
>> + if (!errcnt) {
>> + dev_err(dev,
>> + "failed to read DP AUX Channel Control Register 2\n");
>> + sp_reset_aux(anx78xx);
>> + return -1;
>> + }
>
> Off by one again.
>
Ack
>
>> +
>> + sp_reg_write(anx78xx, TX_P0, SP_AUX_ADDR_7_0_REG, SP_I2C_EXTRA_ADDR);
>> + sp_tx_aux_wr(anx78xx, offset);
>> + /* read 16 bytes (MOT = 1) */
>> + sp_tx_aux_rd(anx78xx, 0xf0 | DP_AUX_I2C_MOT | DP_AUX_I2C_READ);
>> +
>> + for (i = 0; i < 16; i++) {
>> + errcnt = 10;
>> + while (errcnt--) {
>> + sp_reg_read(anx78xx, TX_P0, SP_BUF_DATA_COUNT_REG,
>> + &val);
>> + if (val & SP_BUF_DATA_COUNT_MASK)
>> + break;
>> + usleep_range(2000, 4000);
>> + }
>> +
>> + if (!errcnt) {
>> + dev_err(dev,
>> + "failed to read DP Buffer Data Count
>> Register\n");
>> + sp_reset_aux(anx78xx);
>> + return -1;
>> + }
>
> And here.
>
Ack
>> + errcnt = 10;
>> + while (errcnt--) {
>> + sp_reg_read(anx78xx, TX_P0, SP_DP_AUX_CH_CTRL2_REG, &val);
>> + if (!(val & SP_AUX_EN))
>> + break;
>> + usleep_range(1000, 2000);
>> + }
>> +
>> + if (!errcnt) {
>> + dev_err(dev,
>> + "failed to read DP AUX Channel Control Register 2\n");
>> + sp_reset_aux(anx78xx);
>> + return -1;
>> + }
>
> Here.
>
Ack
>
>> +
>> + return 0;
>> +}
>> +
>> +static int sp_edid_block_checksum(const u8 *raw_edid)
>> +{
>> + int i;
>> + u8 csum = 0;
>> +
>> + for (i = 0; i < EDID_LENGTH; i++)
>> + csum += raw_edid[i];
>> +
>> + return csum;
>> +}
>> +
>> +static int sp_tx_edid_read(struct anx78xx *anx78xx)
>> +{
>> + struct device *dev = &anx78xx->client->dev;
>> + u8 val, last_block, offset = 0;
>> + u8 buf[16];
>> + int i, j, count;
>> +
>> + sp_tx_edid_read_initial(anx78xx);
>> + sp_reg_write(anx78xx, TX_P0, SP_DP_AUX_CH_CTRL1_REG, 0x04);
>> + sp_reg_set_bits(anx78xx, TX_P0, SP_DP_AUX_CH_CTRL2_REG,
>> + SP_AUX_EN | SP_ADDR_ONLY);
>> +
>> + if (sp_wait_aux_op_finish(anx78xx))
>> + return -1;
>> +
>> + sp_addronly_set(anx78xx, false);
>> +
>> + /* Read the number of blocks */
>> + sp_tx_aux_wr(anx78xx, 0x7e);
>> + sp_tx_aux_rd(anx78xx, DP_AUX_I2C_READ);
>> + sp_reg_read(anx78xx, TX_P0, SP_DP_BUF_DATA0_REG, &last_block);
>> + dev_dbg(dev, "last EDID block is %d\n", last_block);
>> +
>> + /* FIXME: Why not just cap to 3 if the reported value is >3 */
>> + if (last_block > 3)
>> + last_block = 1;
>> +
>> + /* for every block */
>> + for (count = 0; count <= last_block; count++) {
>> + switch (count) {
>> + case 0:
>> + case 1:
>> + for (i = 0; i < 8; i++) {
>> + offset = (i + count * 8) * 16;
>> + if (sp_edid_read(anx78xx, offset, buf))
>> +
Re: [PATCHv5 1/3] of: Add vendor prefix for Analogix Semiconductor, Inc.
Hi Rob, 2015-11-13 15:38 GMT+01:00 Rob Herring : > On Fri, Nov 13, 2015 at 01:01:02PM +0100, Enric Balletbo i Serra wrote: >> Analogix Semiconductor develops analog and mixed-signal devices for digital >> media and communications interconnect applications. >> >> Signed-off-by: Enric Balletbo i Serra >> Acked-by: Rob Herring >> --- >> Documentation/devicetree/bindings/vendor-prefixes.txt | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/Documentation/devicetree/bindings/vendor-prefixes.txt >> b/Documentation/devicetree/bindings/vendor-prefixes.txt >> index 82d2ac9..8987ee8 100644 >> --- a/Documentation/devicetree/bindings/vendor-prefixes.txt >> +++ b/Documentation/devicetree/bindings/vendor-prefixes.txt >> @@ -22,6 +22,7 @@ ampire Ampire Co., Ltd. >> ams AMS AG >> amstaos AMS-Taos Inc. >> apm Applied Micro Circuits Corporation (APM) >> +analogix Analogix Semiconductor, Inc. > > Not quite alphabetical order. > Yep, sorry, I think I introduced this mistake rebasing my patches ... I will fix in next version, thanks. >> aptina Aptina Imaging >> arasan Arasan Chip Systems >> arm ARM Ltd. >> -- >> 2.1.0 >> >> ___ >> dri-devel mailing list >> dri-de...@lists.freedesktop.org >> http://lists.freedesktop.org/mailman/listinfo/dri-devel ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCHv3 2/3] devicetree: Add new ANX7814 SlimPort transmitter binding.
2015-09-10 18:35 GMT+02:00 Enric Balletbo i Serra :
> The ANX7814 is an ultra-low power Full-HD (1080p60) SlimPort transmitter
> designed for portable devices.
>
> You can add support to your board with current binding.
>
> Example:
>
> anx7814: anx7814@38 {
> compatible = "analogix,anx7814";
> reg = <0x38>;
> pd-gpios = <&gpio0 1 GPIO_ACTIVE_HIGH>;
> reset-gpios = <&gpio0 2 GPIO_ACTIVE_HIGH>;
> };
>
> Signed-off-by: Enric Balletbo i Serra
> ---
> .../devicetree/bindings/video/bridge/anx7814.txt | 22
> ++
> 1 file changed, 22 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/video/bridge/anx7814.txt
>
> diff --git a/Documentation/devicetree/bindings/video/bridge/anx7814.txt
> b/Documentation/devicetree/bindings/video/bridge/anx7814.txt
> new file mode 100644
> index 000..a8cc746
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/video/bridge/anx7814.txt
> @@ -0,0 +1,22 @@
> +Analogix ANX7814 SlimPort (Full-HD Transmitter)
> +---
> +
> +The ANX7814 is an ultra-low power Full-HD (1080p60) SlimPort transmitter
> +designed for portable devices.
> +
> +Required properties:
> +
> + - compatible : "analogix,anx7814"
> + - reg : I2C address of the device
> + - pd-gpios: Which GPIO to use for power down
> + - reset-gpios : Which GPIO to use for reset
> +
> +Example:
> +
> + anx7814: anx7814@38 {
> + compatible = "analogix,anx7814";
> + reg = <0x38>;
> + pd-gpios = <&gpio0 1 GPIO_ACTIVE_HIGH>;
> + reset-gpios = <&gpio0 2 GPIO_ACTIVE_HIGH>;
> + };
> +
> --
> 2.1.0
>
I saw after sending the series that there was some discussion here,
let me paste to not forget it.
> > No ports needed for describing data connections?
>
> IMHO I'm not sure if this is applicable here, in this case the bridge
> is transparent so it's not required another device node. For example,
> I've an evaluation board, whre I connect in one side an HDMI input
> signal an in the other side a DP monitor, the driver only configures
> the chip and waits for different events (cable plug, cable unplug, etc
> ..)
But what if the chip is connected to a display controller, for instance to the
HDMI output of an SoC ? Is that a use case for the hardware ?
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCHv2 2/3] devicetree: Add new ANX7814 SlimPort transmitter binding.
Hi Rob,
2015-09-09 2:40 GMT+02:00 Rob Herring :
> On 09/08/2015 02:25 AM, Enric Balletbo i Serra wrote:
>> The ANX7814 is an ultra-low power Full-HD (1080p60) SlimPort transmitter
>> designed for portable devices.
>>
>> You can add support to your board with current binding.
>>
>> Example:
>>
>> anx7814: anx7814@38 {
>> compatible = "analogix,anx7814";
>> reg = <0x38>;
>> pd-gpios = <&gpio0 1 GPIO_ACTIVE_HIGH>;
>> reset-gpios = <&gpio0 2 GPIO_ACTIVE_HIGH>;
>> };
>>
>> Signed-off-by: Enric Balletbo i Serra
>> ---
>> .../devicetree/bindings/video/anx7814.txt | 22
>> ++
>> 1 file changed, 22 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/video/anx7814.txt
>>
>> diff --git a/Documentation/devicetree/bindings/video/anx7814.txt
>> b/Documentation/devicetree/bindings/video/anx7814.txt
>> new file mode 100644
>> index 000..a8cc746
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/video/anx7814.txt
>> @@ -0,0 +1,22 @@
>> +Analogix ANX7814 SlimPort (Full-HD Transmitter)
>> +---
>> +
>> +The ANX7814 is an ultra-low power Full-HD (1080p60) SlimPort transmitter
>> +designed for portable devices.
>> +
>> +Required properties:
>> +
>> + - compatible: "analogix,anx7814"
>> + - reg : I2C address of the device
>> + - pd-gpios : Which GPIO to use for power down
>> + - reset-gpios : Which GPIO to use for reset
>> +
>> +Example:
>> +
>> + anx7814: anx7814@38 {
>> + compatible = "analogix,anx7814";
>> + reg = <0x38>;
>> + pd-gpios = <&gpio0 1 GPIO_ACTIVE_HIGH>;
>> + reset-gpios = <&gpio0 2 GPIO_ACTIVE_HIGH>;
>
> No ports needed for describing data connections?
>
IMHO I'm not sure if this is applicable here, in this case the bridge
is transparent so it's not required another device node. For example,
I've an evaluation board, whre I connect in one side an HDMI input
signal an in the other side a DP monitor, the driver only configures
the chip and waits for different events (cable plug, cable unplug, etc
..)
Cheers,
Enric
> Rob
>
>> + };
>> +
>>
>
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 3/3] staging: slimport: Add anx7814 driver support by analogix.
Hi Dan,
Many thanks for this first review.
2015-09-09 5:38 GMT+02:00 Daniel Kurtz :
> Hi Eric,
>
> Thanks for starting to upstream this Analogix Slimport driver!
> As Greg says, please move this driver to its intended directory, I presume:
> /drivers/gpu/drm/bridge
>
I sent yesterday another version moving the driver. I'm not sure if
you were aware. In the second version I moved the driver to
drivers/gpu/drm/i2c instead of drivers/gpu/drm/bridge. Do you think
bridge is the better place ? If so I'll move to bridge directory. I
had doubts about it.
> And when you submit, use get_maintainer.pl to add the proper reviewers
> and lists.
> At present, you have no DRM folks, nor dri-devel, so you probably
> won't receive any additional feedback on this version, since the
> relevant folks have not seen your emails.
>
> Some more very detailed feedback inline...
>
> On Sep 7, 2015 05:15, "Enric Balletbo i Serra" wrote:
>>
>> The ANX7814 is an ultra-low power Full-HD (1080p60) SlimPort transmitter
>> designed for portable devices.
>>
>> This driver adds initial support and supports HDMI to DP pass-through mode.
>>
>> Signed-off-by: Enric Balletbo i Serra
>> ---
>> drivers/staging/Kconfig|2 +
>> drivers/staging/Makefile |1 +
>> drivers/staging/slimport/Kconfig |7 +
>> drivers/staging/slimport/Makefile |4 +
>> drivers/staging/slimport/slimport.c| 301 +++
>> drivers/staging/slimport/slimport.h| 49 +
>> drivers/staging/slimport/slimport_tx_drv.c | 3293
>>
>> drivers/staging/slimport/slimport_tx_drv.h | 254 +++
>> drivers/staging/slimport/slimport_tx_reg.h | 786 +++
>> 9 files changed, 4697 insertions(+)
>> create mode 100644 drivers/staging/slimport/Kconfig
>> create mode 100644 drivers/staging/slimport/Makefile
>> create mode 100644 drivers/staging/slimport/slimport.c
>> create mode 100644 drivers/staging/slimport/slimport.h
>> create mode 100644 drivers/staging/slimport/slimport_tx_drv.c
>> create mode 100644 drivers/staging/slimport/slimport_tx_drv.h
>> create mode 100644 drivers/staging/slimport/slimport_tx_reg.h
>>
>> diff --git a/drivers/staging/Kconfig b/drivers/staging/Kconfig
>> index e29293c..24ccd7c 100644
>> --- a/drivers/staging/Kconfig
>> +++ b/drivers/staging/Kconfig
>> @@ -110,4 +110,6 @@ source "drivers/staging/wilc1000/Kconfig"
>>
>> source "drivers/staging/most/Kconfig"
>>
>> +source "drivers/staging/slimport/Kconfig"
>> +
>> endif # STAGING
>> diff --git a/drivers/staging/Makefile b/drivers/staging/Makefile
>> index 50824dd..942e886 100644
>> --- a/drivers/staging/Makefile
>> +++ b/drivers/staging/Makefile
>> @@ -47,3 +47,4 @@ obj-$(CONFIG_FB_TFT) += fbtft/
>> obj-$(CONFIG_FSL_MC_BUS) += fsl-mc/
>> obj-$(CONFIG_WILC1000) += wilc1000/
>> obj-$(CONFIG_MOST) += most/
>> +obj-$(CONFIG_SLIMPORT_ANX78XX) += slimport/
>> diff --git a/drivers/staging/slimport/Kconfig
>> b/drivers/staging/slimport/Kconfig
>> new file mode 100644
>> index 000..f5233ef
>> --- /dev/null
>> +++ b/drivers/staging/slimport/Kconfig
>> @@ -0,0 +1,7 @@
>> +config SLIMPORT_ANX78XX
>
> I think the "SLIMPORT_" here is a bit overkill, and just adds to
> confusion over what name to use where. I'd recommend just
> CONFIG_ANX78XX.
>
> Likewise, for consistency, the rename the files as "anx78xx*" instead
> of "slimport*".
>
>> + tristate "Analogix Slimport transmitter ANX78XX support"
>> + help
>> + Slimport Transmitter is a HD video transmitter chip
>> + over micro-USB connector for smartphone device.
>> +
>> +
>> diff --git a/drivers/staging/slimport/Makefile
>> b/drivers/staging/slimport/Makefile
>> new file mode 100644
>> index 000..9bb6ce2
>> --- /dev/null
>> +++ b/drivers/staging/slimport/Makefile
>> @@ -0,0 +1,4 @@
>> +obj-${CONFIG_SLIMPORT_ANX78XX} := anx78xx.o
>> +
>> +anx78xx-y := slimport.o
>> +anx78xx-y += slimport_tx_drv.o
>> diff --git a/drivers/staging/slimport/slimport.c
>> b/drivers/staging/slimport/slimport.c
>> new file mode 100644
>> index 000..95c5114
>> --- /dev/null
>> +++ b/drivers/staging/slimport/slimport.c
>> @@ -0,0 +1,301 @@
>> +/*
>> + * Copyright(c) 2015, Analogix Semiconductor. All rights reserved.
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 and
>> + * only version 2 as published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>> + * GNU General Public License for more details.
>> + *
>> + */
>> +
>> +#include
>> +#include
>> +#include
>> +#include
>> +#include
>> +#include
>> +#include
>> +#include
>> +#include
>> +#include
>> +
>> +#include "slimport.h
Re: [PATCH 3/3] staging: slimport: Add anx7814 driver support by analogix.
2015-09-07 1:27 GMT+02:00 Greg KH : > On Sun, Sep 06, 2015 at 11:14:02PM +0200, Enric Balletbo i Serra wrote: >> The ANX7814 is an ultra-low power Full-HD (1080p60) SlimPort transmitter >> designed for portable devices. >> >> This driver adds initial support and supports HDMI to DP pass-through mode. >> >> Signed-off-by: Enric Balletbo i Serra >> --- >> drivers/staging/Kconfig|2 + >> drivers/staging/Makefile |1 + >> drivers/staging/slimport/Kconfig |7 + >> drivers/staging/slimport/Makefile |4 + >> drivers/staging/slimport/slimport.c| 301 +++ >> drivers/staging/slimport/slimport.h| 49 + >> drivers/staging/slimport/slimport_tx_drv.c | 3293 >> >> drivers/staging/slimport/slimport_tx_drv.h | 254 +++ >> drivers/staging/slimport/slimport_tx_reg.h | 786 +++ > > Why is this a staging driver? > What prevents it from being merged into the "real" part of the kernel > tree? > I'll be glad to move the driver to their subsystem if you think it's a the better place. Basically there are two reasons why I send the driver to the staging directory. The first one is because my test environment is a bit limited, with my environment I can only test the HDMI to DisplayPort pass-through mode so the driver builds but it's partially tested. The second one is that I expect I'll need to refactor some code, specially in slimport_tx_drv.c file to be accepted, I decided not change too much this file from the original to not break the functionality, so I thought that will be better send first to the staging driver to have first reviews. > All staging drivers need a TODO file, listing what needs to be done and > who is in charge of it. I can't take this without that added. > ok, I'll add in the next series once received some feedback (or move to the video subsystem) Thanks, Enric > thanks, > > greg k-h ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
