diff mbox

[v6,2/3] drm: bridge: Add thc63lvd1024 LVDS decoder driver

Message ID 1521213399-31947-3-git-send-email-jacopo+renesas@jmondi.org (mailing list archive)
State New, archived
Headers show

Commit Message

Jacopo Mondi March 16, 2018, 3:16 p.m. UTC 
Add DRM bridge driver for Thine THC63LVD1024 LVDS to digital parallel
output converter.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>
Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
 drivers/gpu/drm/bridge/Kconfig        |   6 +
 drivers/gpu/drm/bridge/Makefile       |   1 +
 drivers/gpu/drm/bridge/thc63lvd1024.c | 255 ++++++++++++++++++++++++++++++++++
 3 files changed, 262 insertions(+)
 create mode 100644 drivers/gpu/drm/bridge/thc63lvd1024.c

Comments

Laurent Pinchart March 20, 2018, 1 p.m. UTC | #1 
Hi Jacopo,

Thank you for the patch.

On Friday, 16 March 2018 17:16:38 EET Jacopo Mondi wrote:
> Add DRM bridge driver for Thine THC63LVD1024 LVDS to digital parallel
> output converter.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>
> Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> ---
>  drivers/gpu/drm/bridge/Kconfig        |   6 +
>  drivers/gpu/drm/bridge/Makefile       |   1 +
>  drivers/gpu/drm/bridge/thc63lvd1024.c | 255 +++++++++++++++++++++++++++++++
>  3 files changed, 262 insertions(+)
>  create mode 100644 drivers/gpu/drm/bridge/thc63lvd1024.c

I'm aware that you started with a generic driver and then moved to a chip-
specific driver and that it caused issues. I however believe it was a good 
design, but we can make the driver generic later when a second similar chip 
will need to be supported.

> diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
> index 3b99d5a..5815a20 100644
> --- a/drivers/gpu/drm/bridge/Kconfig
> +++ b/drivers/gpu/drm/bridge/Kconfig
> @@ -92,6 +92,12 @@ config DRM_SII9234
>  	  It is an I2C driver, that detects connection of MHL bridge
>  	  and starts encapsulation of HDMI signal.
> 
> +config DRM_THINE_THC63LVD1024
> +	tristate "Thine THC63LVD1024 LVDS decoder bridge"
> +	depends on OF
> +	---help---
> +	  Thine THC63LVD1024 LVDS/parallel converter driver.
> +
>  config DRM_TOSHIBA_TC358767
>  	tristate "Toshiba TC358767 eDP bridge"
>  	depends on OF
> diff --git a/drivers/gpu/drm/bridge/Makefile
> b/drivers/gpu/drm/bridge/Makefile index 373eb28..fd90b16 100644
> --- a/drivers/gpu/drm/bridge/Makefile
> +++ b/drivers/gpu/drm/bridge/Makefile
> @@ -8,6 +8,7 @@ obj-$(CONFIG_DRM_PARADE_PS8622) += parade-ps8622.o
>  obj-$(CONFIG_DRM_SIL_SII8620) += sil-sii8620.o
>  obj-$(CONFIG_DRM_SII902X) += sii902x.o
>  obj-$(CONFIG_DRM_SII9234) += sii9234.o
> +obj-$(CONFIG_DRM_THINE_THC63LVD1024) += thc63lvd1024.o
>  obj-$(CONFIG_DRM_TOSHIBA_TC358767) += tc358767.o
>  obj-$(CONFIG_DRM_ANALOGIX_DP) += analogix/
>  obj-$(CONFIG_DRM_I2C_ADV7511) += adv7511/
> diff --git a/drivers/gpu/drm/bridge/thc63lvd1024.c
> b/drivers/gpu/drm/bridge/thc63lvd1024.c new file mode 100644
> index 0000000..02a54c2
> --- /dev/null
> +++ b/drivers/gpu/drm/bridge/thc63lvd1024.c
> @@ -0,0 +1,255 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * THC63LVD1024 LVDS to parallel data DRM bridge driver.
> + *
> + * Copyright (C) 2018 Jacopo Mondi <jacopo+renesas@jmondi.org>
> + */
> +
> +#include <drm/drmP.h>
> +#include <drm/drm_bridge.h>
> +#include <drm/drm_panel.h>
> +
> +#include <linux/gpio/consumer.h>
> +#include <linux/of_graph.h>
> +#include <linux/regulator/consumer.h>
> +
> +enum {

Please name the enumeration.

> +	THC63_LVDS_IN0,
> +	THC63_LVDS_IN1,
> +	THC63_DIGITAL_OUT0,
> +	THC63_DIGITAL_OUT1,

Strictly speaking LVDS is digital too. Maybe THC63_RGB_OUT* ?

> +};
> +
> +static const char * const thc63_reg_names[] = {
> +	"vcc", "lvcc", "pvcc", "cvcc",
> +};
> +
> +struct thc63_dev {
> +	struct device *dev;
> +
> +	struct regulator *vccs[ARRAY_SIZE(thc63_reg_names)];
> +
> +	struct gpio_desc *pdwn;
> +	struct gpio_desc *oe;
> +
> +	struct drm_bridge bridge;
> +	struct drm_bridge *next;
> +};
> +
> +static inline struct thc63_dev *to_thc63(struct drm_bridge *bridge)
> +{
> +	return container_of(bridge, struct thc63_dev, bridge);
> +}
> +
> +static int thc63_attach(struct drm_bridge *bridge)
> +{
> +	struct thc63_dev *thc63 = to_thc63(bridge);
> +
> +	return drm_bridge_attach(bridge->encoder, thc63->next, bridge);
> +}
> +
> +static void thc63_enable(struct drm_bridge *bridge)
> +{
> +	struct thc63_dev *thc63 = to_thc63(bridge);
> +	struct regulator *vcc;
> +	int i;
> + 
> +	for (i = 0; i < ARRAY_SIZE(thc63->vccs); i++) {
> +		vcc = thc63->vccs[i];
> +		if (!vcc)
> +			continue;
> +
> +		if (regulator_enable(vcc))

You can operate on thc63->vccs[i] directly without a local variable, here and 
below in the functions that handle regulators.

> +			goto error_vcc_enable;
> +	}

I thought about proposing usage of the regulators bulk API but it doesn't 
support optional regulators :-( If you agree with my proposal to make all 
regulators mandatory, please consider the bulk API to simplify power supply 
handling. If on the other hand you agree with my proposal to keep the vcc 
supply only, it won't matter.

> +	if (thc63->pdwn)
> +		gpiod_set_value(thc63->pdwn, 0);
> +
> +	if (thc63->oe)
> +		gpiod_set_value(thc63->oe, 1);
> +
> +	return;
> +
> +error_vcc_enable:
> +	dev_err(thc63->dev, "Failed to enable regulator %s\n",
> +		thc63_reg_names[i]);

I'd move the error message right before the goto.

> +	for (i = i - 1; i >= 0; i--) {
> +		vcc = thc63->vccs[i];
> +		if (!vcc)
> +			continue;
> +
> +		regulator_disable(vcc);
> +	}
> +}
> +
> +static void thc63_disable(struct drm_bridge *bridge)
> +{
> +	struct thc63_dev *thc63 = to_thc63(bridge);
> +	struct regulator *vcc;
> +	int i;
> +
> +	if (thc63->oe)
> +		gpiod_set_value(thc63->oe, 0);
> +
> +	if (thc63->pdwn)
> +		gpiod_set_value(thc63->pdwn, 1);
> +
> +	for (i = ARRAY_SIZE(thc63->vccs) - 1; i >= 0; i--) {
> +		vcc = thc63->vccs[i];
> +		if (!vcc)
> +			continue;
> +
> +		if (regulator_disable(vcc))
> +			dev_dbg(thc63->dev,
> +				"Failed to disable regulator %s\n",
> +				thc63_reg_names[i]);
> +	}
> +}
> +
> +struct drm_bridge_funcs thc63_bridge_func = {

static const

> +	.attach	= thc63_attach,
> +	.enable = thc63_enable,
> +	.disable = thc63_disable,
> +};
> +
> +static int thc63_parse_dt(struct thc63_dev *thc63)
> +{
> +	struct device_node *thc63_out;
> +	struct device_node *remote;
> +	int ret = 0;
> +
> +	thc63_out = of_graph_get_endpoint_by_regs(thc63->dev->of_node,
> +						  THC63_DIGITAL_OUT0, -1);
> +	if (!thc63_out) {
> +		dev_err(thc63->dev, "Missing endpoint in Port@%u\n",
> +			THC63_DIGITAL_OUT0);
> +		return -ENODEV;
> +	}
> +
> +	remote = of_graph_get_remote_port_parent(thc63_out);

	of_node_put(thc63_out);

here, it will simplify error handling.

> +	if (!remote) {
> +		dev_err(thc63->dev, "Endpoint in Port@%u unconnected\n",
> +			THC63_DIGITAL_OUT0);
> +		ret = -ENODEV;
> +		goto error_put_out_node;
> +	}
> +
> +	if (!of_device_is_available(remote)) {
> +		dev_err(thc63->dev, "Port@%u remote endpoint is disabled\n",
> +			THC63_DIGITAL_OUT0);
> +		ret = -ENODEV;
> +		goto error_put_remote_node;
> +	}
> +
> +	thc63->next = of_drm_find_bridge(remote);
> +	if (!thc63->next)
> +		ret = -EPROBE_DEFER;

You can add a return 0 here (and a goto in the previous check. Going through 
error labels in the non-error case is confusing. As a result you can avoid 
initializing ret to 0.

> +error_put_remote_node:
> +	of_node_put(remote);
> +error_put_out_node:
> +	of_node_put(thc63_out);
> +
> +	return ret;
> +}
> +
> +static int thc63_gpio_init(struct thc63_dev *thc63)
> +{
> +	thc63->oe = devm_gpiod_get_optional(thc63->dev, "oe", GPIOD_OUT_LOW);
> +	if (IS_ERR(thc63->oe)) {
> +		dev_err(thc63->dev, "Unable to get \"oe-gpios\"\n");

You can add the error code to the message, it could be helpful.

> +		return PTR_ERR(thc63->oe);
> +	}
> +
> +	thc63->pdwn = devm_gpiod_get_optional(thc63->dev, "pdwn",
> +					      GPIOD_OUT_HIGH);
> +	if (IS_ERR(thc63->pdwn)) {
> +		dev_err(thc63->dev, "Unable to get \"pdwn-gpios\"\n");

Same here.

> +		return PTR_ERR(thc63->pdwn);
> +	}
> +
> +	return 0;
> +}
> +
> +static int thc63_regulator_init(struct thc63_dev *thc63)
> +{
> +	struct regulator **reg;
> +	int i;

i is never negative, you can make it an unsigned int.

> +	reg = &thc63->vccs[0];
> +	for (i = 0; i < ARRAY_SIZE(thc63->vccs); i++, reg++) {
> +		*reg = devm_regulator_get_optional(thc63->dev,
> +						   thc63_reg_names[i]);
> +		if (IS_ERR(*reg)) {
> +			if (PTR_ERR(*reg) == -EPROBE_DEFER)
> +				return -EPROBE_DEFER;
> +			*reg = NULL;
> +		}
> +	}

Wouldn't the following be simpler ?

	for (i = 0; i < ARRAY_SIZE(thc63->vccs); i++) {
		struct regulator *reg;

		reg = devm_regulator_get_optional(thc63->dev,
						   thc63_reg_names[i]);
		if (IS_ERR(reg)) {
			if (PTR_ERR(reg) == -EPROBE_DEFER)
				return -EPROBE_DEFER;
			reg = NULL;
		}

		thc63->vccs[i] = reg;
	}

> +	return 0;
> +}
> +
> +static int thc63_probe(struct platform_device *pdev)
> +{
> +	struct thc63_dev *thc63;
> +	int ret;
> +
> +	thc63 = devm_kzalloc(&pdev->dev, sizeof(*thc63), GFP_KERNEL);
> +	if (!thc63)
> +		return -ENOMEM;
> +
> +	thc63->dev = &pdev->dev;
> +	platform_set_drvdata(pdev, thc63);
> +
> +	ret = thc63_regulator_init(thc63);
> +	if (ret)
> +		return ret;
> +
> +	ret = thc63_gpio_init(thc63);
> +	if (ret)
> +		return ret;
> +
> +	ret = thc63_parse_dt(thc63);
> +	if (ret)
> +		return ret;
> +
> +	thc63->bridge.driver_private = thc63;
> +	thc63->bridge.of_node = pdev->dev.of_node;
> +	thc63->bridge.funcs = &thc63_bridge_func;
> +
> +	drm_bridge_add(&thc63->bridge);
> +
> +	return 0;
> +}
> +
> +static int thc63_remove(struct platform_device *pdev)
> +{
> +	struct thc63_dev *thc63 = platform_get_drvdata(pdev);
> +
> +	drm_bridge_remove(&thc63->bridge);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id thc63_match[] = {
> +	{ .compatible = "thine,thc63lvd1024", },
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(of, thc63_match);
> +
> +static struct platform_driver thc63_driver = {
> +	.probe	= thc63_probe,
> +	.remove	= thc63_remove,
> +	.driver	= {
> +		.name		= "thc63lvd1024",
> +		.of_match_table	= thc63_match,
> +	},
> +};
> +module_platform_driver(thc63_driver);
> +
> +MODULE_AUTHOR("Jacopo Mondi <jacopo@jmondi.org>");
> +MODULE_DESCRIPTION("Thine THC63LVD1024 LVDS decoder DRM bridge driver");
> +MODULE_LICENSE("GPL v2");
Vladimir Zapolskiy March 27, 2018, 6:24 a.m. UTC | #2 
Hi Jacopo,

On 03/16/2018 05:16 PM, Jacopo Mondi wrote:
> Add DRM bridge driver for Thine THC63LVD1024 LVDS to digital parallel
> output converter.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>
> Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> ---
>  drivers/gpu/drm/bridge/Kconfig        |   6 +
>  drivers/gpu/drm/bridge/Makefile       |   1 +
>  drivers/gpu/drm/bridge/thc63lvd1024.c | 255 ++++++++++++++++++++++++++++++++++
>  3 files changed, 262 insertions(+)
>  create mode 100644 drivers/gpu/drm/bridge/thc63lvd1024.c
> 
> diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
> index 3b99d5a..5815a20 100644
> --- a/drivers/gpu/drm/bridge/Kconfig
> +++ b/drivers/gpu/drm/bridge/Kconfig
> @@ -92,6 +92,12 @@ config DRM_SII9234
>  	  It is an I2C driver, that detects connection of MHL bridge
>  	  and starts encapsulation of HDMI signal.
>  
> +config DRM_THINE_THC63LVD1024
> +	tristate "Thine THC63LVD1024 LVDS decoder bridge"
> +	depends on OF
> +	---help---
> +	  Thine THC63LVD1024 LVDS/parallel converter driver.
> +
>  config DRM_TOSHIBA_TC358767
>  	tristate "Toshiba TC358767 eDP bridge"
>  	depends on OF
> diff --git a/drivers/gpu/drm/bridge/Makefile b/drivers/gpu/drm/bridge/Makefile
> index 373eb28..fd90b16 100644
> --- a/drivers/gpu/drm/bridge/Makefile
> +++ b/drivers/gpu/drm/bridge/Makefile
> @@ -8,6 +8,7 @@ obj-$(CONFIG_DRM_PARADE_PS8622) += parade-ps8622.o
>  obj-$(CONFIG_DRM_SIL_SII8620) += sil-sii8620.o
>  obj-$(CONFIG_DRM_SII902X) += sii902x.o
>  obj-$(CONFIG_DRM_SII9234) += sii9234.o
> +obj-$(CONFIG_DRM_THINE_THC63LVD1024) += thc63lvd1024.o
>  obj-$(CONFIG_DRM_TOSHIBA_TC358767) += tc358767.o
>  obj-$(CONFIG_DRM_ANALOGIX_DP) += analogix/
>  obj-$(CONFIG_DRM_I2C_ADV7511) += adv7511/
> diff --git a/drivers/gpu/drm/bridge/thc63lvd1024.c b/drivers/gpu/drm/bridge/thc63lvd1024.c
> new file mode 100644
> index 0000000..02a54c2
> --- /dev/null
> +++ b/drivers/gpu/drm/bridge/thc63lvd1024.c
> @@ -0,0 +1,255 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * THC63LVD1024 LVDS to parallel data DRM bridge driver.
> + *
> + * Copyright (C) 2018 Jacopo Mondi <jacopo+renesas@jmondi.org>
> + */
> +
> +#include <drm/drmP.h>
> +#include <drm/drm_bridge.h>
> +#include <drm/drm_panel.h>
> +
> +#include <linux/gpio/consumer.h>
> +#include <linux/of_graph.h>
> +#include <linux/regulator/consumer.h>
> +
> +enum {
> +	THC63_LVDS_IN0,
> +	THC63_LVDS_IN1,
> +	THC63_DIGITAL_OUT0,
> +	THC63_DIGITAL_OUT1,
> +};
> +
> +static const char * const thc63_reg_names[] = {
> +	"vcc", "lvcc", "pvcc", "cvcc",
> +};
> +
> +struct thc63_dev {
> +	struct device *dev;
> +
> +	struct regulator *vccs[ARRAY_SIZE(thc63_reg_names)];
> +
> +	struct gpio_desc *pdwn;
> +	struct gpio_desc *oe;
> +
> +	struct drm_bridge bridge;
> +	struct drm_bridge *next;
> +};
> +
> +static inline struct thc63_dev *to_thc63(struct drm_bridge *bridge)
> +{
> +	return container_of(bridge, struct thc63_dev, bridge);
> +}
> +
> +static int thc63_attach(struct drm_bridge *bridge)
> +{
> +	struct thc63_dev *thc63 = to_thc63(bridge);
> +
> +	return drm_bridge_attach(bridge->encoder, thc63->next, bridge);
> +}
> +
> +static void thc63_enable(struct drm_bridge *bridge)
> +{
> +	struct thc63_dev *thc63 = to_thc63(bridge);
> +	struct regulator *vcc;
> +	int i;

unsigned int i;

> +
> +	for (i = 0; i < ARRAY_SIZE(thc63->vccs); i++) {
> +		vcc = thc63->vccs[i];
> +		if (!vcc)
> +			continue;
> +
> +		if (regulator_enable(vcc))
> +			goto error_vcc_enable;
> +	}
> +
> +	if (thc63->pdwn)
> +		gpiod_set_value(thc63->pdwn, 0);
> +
> +	if (thc63->oe)
> +		gpiod_set_value(thc63->oe, 1);
> +
> +	return;
> +
> +error_vcc_enable:
> +	dev_err(thc63->dev, "Failed to enable regulator %s\n",
> +		thc63_reg_names[i]);
> +
> +	for (i = i - 1; i >= 0; i--) {
> +		vcc = thc63->vccs[i];
> +		if (!vcc)
> +			continue;
> +
> +		regulator_disable(vcc);
> +	}
> +}
> +
> +static void thc63_disable(struct drm_bridge *bridge)
> +{
> +	struct thc63_dev *thc63 = to_thc63(bridge);
> +	struct regulator *vcc;
> +	int i;
> +
> +	if (thc63->oe)
> +		gpiod_set_value(thc63->oe, 0);
> +
> +	if (thc63->pdwn)
> +		gpiod_set_value(thc63->pdwn, 1);
> +
> +	for (i = ARRAY_SIZE(thc63->vccs) - 1; i >= 0; i--) {
> +		vcc = thc63->vccs[i];
> +		if (!vcc)
> +			continue;
> +
> +		if (regulator_disable(vcc))
> +			dev_dbg(thc63->dev,
> +				"Failed to disable regulator %s\n",
> +				thc63_reg_names[i]);
> +	}
> +}
> +
> +struct drm_bridge_funcs thc63_bridge_func = {

Apparently you missed all my comments from v2 review.

If you like to avoid non-NULL function pointers to the void, please 
use static const storage qualifier.

--
With best wishes,
Vladimir
Andrzej Hajda March 27, 2018, 7:28 a.m. UTC | #3 
On 27.03.2018 08:24, Vladimir Zapolskiy wrote:
> Hi Jacopo,
>
> On 03/16/2018 05:16 PM, Jacopo Mondi wrote:
>> Add DRM bridge driver for Thine THC63LVD1024 LVDS to digital parallel
>> output converter.
>>
>> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
>> Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>
>> Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
>> ---
>>  drivers/gpu/drm/bridge/Kconfig        |   6 +
>>  drivers/gpu/drm/bridge/Makefile       |   1 +
>>  drivers/gpu/drm/bridge/thc63lvd1024.c | 255 ++++++++++++++++++++++++++++++++++
>>  3 files changed, 262 insertions(+)
>>  create mode 100644 drivers/gpu/drm/bridge/thc63lvd1024.c
>>
>> diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
>> index 3b99d5a..5815a20 100644
>> --- a/drivers/gpu/drm/bridge/Kconfig
>> +++ b/drivers/gpu/drm/bridge/Kconfig
>> @@ -92,6 +92,12 @@ config DRM_SII9234
>>  	  It is an I2C driver, that detects connection of MHL bridge
>>  	  and starts encapsulation of HDMI signal.
>>  
>> +config DRM_THINE_THC63LVD1024
>> +	tristate "Thine THC63LVD1024 LVDS decoder bridge"
>> +	depends on OF
>> +	---help---
>> +	  Thine THC63LVD1024 LVDS/parallel converter driver.
>> +
>>  config DRM_TOSHIBA_TC358767
>>  	tristate "Toshiba TC358767 eDP bridge"
>>  	depends on OF
>> diff --git a/drivers/gpu/drm/bridge/Makefile b/drivers/gpu/drm/bridge/Makefile
>> index 373eb28..fd90b16 100644
>> --- a/drivers/gpu/drm/bridge/Makefile
>> +++ b/drivers/gpu/drm/bridge/Makefile
>> @@ -8,6 +8,7 @@ obj-$(CONFIG_DRM_PARADE_PS8622) += parade-ps8622.o
>>  obj-$(CONFIG_DRM_SIL_SII8620) += sil-sii8620.o
>>  obj-$(CONFIG_DRM_SII902X) += sii902x.o
>>  obj-$(CONFIG_DRM_SII9234) += sii9234.o
>> +obj-$(CONFIG_DRM_THINE_THC63LVD1024) += thc63lvd1024.o
>>  obj-$(CONFIG_DRM_TOSHIBA_TC358767) += tc358767.o
>>  obj-$(CONFIG_DRM_ANALOGIX_DP) += analogix/
>>  obj-$(CONFIG_DRM_I2C_ADV7511) += adv7511/
>> diff --git a/drivers/gpu/drm/bridge/thc63lvd1024.c b/drivers/gpu/drm/bridge/thc63lvd1024.c
>> new file mode 100644
>> index 0000000..02a54c2
>> --- /dev/null
>> +++ b/drivers/gpu/drm/bridge/thc63lvd1024.c
>> @@ -0,0 +1,255 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * THC63LVD1024 LVDS to parallel data DRM bridge driver.
>> + *
>> + * Copyright (C) 2018 Jacopo Mondi <jacopo+renesas@jmondi.org>
>> + */
>> +
>> +#include <drm/drmP.h>
>> +#include <drm/drm_bridge.h>
>> +#include <drm/drm_panel.h>
>> +
>> +#include <linux/gpio/consumer.h>
>> +#include <linux/of_graph.h>
>> +#include <linux/regulator/consumer.h>
>> +
>> +enum {
>> +	THC63_LVDS_IN0,
>> +	THC63_LVDS_IN1,
>> +	THC63_DIGITAL_OUT0,
>> +	THC63_DIGITAL_OUT1,
>> +};
>> +
>> +static const char * const thc63_reg_names[] = {
>> +	"vcc", "lvcc", "pvcc", "cvcc",
>> +};
>> +
>> +struct thc63_dev {
>> +	struct device *dev;
>> +
>> +	struct regulator *vccs[ARRAY_SIZE(thc63_reg_names)];
>> +
>> +	struct gpio_desc *pdwn;
>> +	struct gpio_desc *oe;
>> +
>> +	struct drm_bridge bridge;
>> +	struct drm_bridge *next;
>> +};
>> +
>> +static inline struct thc63_dev *to_thc63(struct drm_bridge *bridge)
>> +{
>> +	return container_of(bridge, struct thc63_dev, bridge);
>> +}
>> +
>> +static int thc63_attach(struct drm_bridge *bridge)
>> +{
>> +	struct thc63_dev *thc63 = to_thc63(bridge);
>> +
>> +	return drm_bridge_attach(bridge->encoder, thc63->next, bridge);
>> +}
>> +
>> +static void thc63_enable(struct drm_bridge *bridge)
>> +{
>> +	struct thc63_dev *thc63 = to_thc63(bridge);
>> +	struct regulator *vcc;
>> +	int i;
> unsigned int i;

Why? You are introducing silly bug this way, see few lines below.

>
>> +
>> +	for (i = 0; i < ARRAY_SIZE(thc63->vccs); i++) {
>> +		vcc = thc63->vccs[i];
>> +		if (!vcc)
>> +			continue;
>> +
>> +		if (regulator_enable(vcc))
>> +			goto error_vcc_enable;
>> +	}
>> +
>> +	if (thc63->pdwn)
>> +		gpiod_set_value(thc63->pdwn, 0);
>> +
>> +	if (thc63->oe)
>> +		gpiod_set_value(thc63->oe, 1);
>> +
>> +	return;
>> +
>> +error_vcc_enable:
>> +	dev_err(thc63->dev, "Failed to enable regulator %s\n",
>> +		thc63_reg_names[i]);
>> +
>> +	for (i = i - 1; i >= 0; i--) {

Here, the loop will not end if you define i unsigned.

I know one can change the loop, to make it working with unsigned. But
this clearly shows that using unsigned is more risky.
What are advantages of unsigned vs int in this case. Are there some
guidelines/discussions about it?

Regards
Andrzej

>> +		vcc = thc63->vccs[i];
>> +		if (!vcc)
>> +			continue;
>> +
>> +		regulator_disable(vcc);
>> +	}
>> +}
>> +
>> +static void thc63_disable(struct drm_bridge *bridge)
>> +{
>> +	struct thc63_dev *thc63 = to_thc63(bridge);
>> +	struct regulator *vcc;
>> +	int i;
>> +
>> +	if (thc63->oe)
>> +		gpiod_set_value(thc63->oe, 0);
>> +
>> +	if (thc63->pdwn)
>> +		gpiod_set_value(thc63->pdwn, 1);
>> +
>> +	for (i = ARRAY_SIZE(thc63->vccs) - 1; i >= 0; i--) {
>> +		vcc = thc63->vccs[i];
>> +		if (!vcc)
>> +			continue;
>> +
>> +		if (regulator_disable(vcc))
>> +			dev_dbg(thc63->dev,
>> +				"Failed to disable regulator %s\n",
>> +				thc63_reg_names[i]);
>> +	}
>> +}
>> +
>> +struct drm_bridge_funcs thc63_bridge_func = {
> Apparently you missed all my comments from v2 review.
>
> If you like to avoid non-NULL function pointers to the void, please 
> use static const storage qualifier.
>
> --
> With best wishes,
> Vladimir
>
>
>
Jacopo Mondi March 27, 2018, 7:30 a.m. UTC | #4 
HI Vladimir,

On Tue, Mar 27, 2018 at 09:24:28AM +0300, Vladimir Zapolskiy wrote:
> Hi Jacopo,
>
> On 03/16/2018 05:16 PM, Jacopo Mondi wrote:
> > Add DRM bridge driver for Thine THC63LVD1024 LVDS to digital parallel
> > output converter.
> >
> > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>
> > Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> > ---
> >  drivers/gpu/drm/bridge/Kconfig        |   6 +
> >  drivers/gpu/drm/bridge/Makefile       |   1 +
> >  drivers/gpu/drm/bridge/thc63lvd1024.c | 255 ++++++++++++++++++++++++++++++++++
> >  3 files changed, 262 insertions(+)
> >  create mode 100644 drivers/gpu/drm/bridge/thc63lvd1024.c
> >
> > diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
> > index 3b99d5a..5815a20 100644
> > --- a/drivers/gpu/drm/bridge/Kconfig
> > +++ b/drivers/gpu/drm/bridge/Kconfig
> > @@ -92,6 +92,12 @@ config DRM_SII9234
> >  	  It is an I2C driver, that detects connection of MHL bridge
> >  	  and starts encapsulation of HDMI signal.
> >
> > +config DRM_THINE_THC63LVD1024
> > +	tristate "Thine THC63LVD1024 LVDS decoder bridge"
> > +	depends on OF
> > +	---help---
> > +	  Thine THC63LVD1024 LVDS/parallel converter driver.
> > +
> >  config DRM_TOSHIBA_TC358767
> >  	tristate "Toshiba TC358767 eDP bridge"
> >  	depends on OF
> > diff --git a/drivers/gpu/drm/bridge/Makefile b/drivers/gpu/drm/bridge/Makefile
> > index 373eb28..fd90b16 100644
> > --- a/drivers/gpu/drm/bridge/Makefile
> > +++ b/drivers/gpu/drm/bridge/Makefile
> > @@ -8,6 +8,7 @@ obj-$(CONFIG_DRM_PARADE_PS8622) += parade-ps8622.o
> >  obj-$(CONFIG_DRM_SIL_SII8620) += sil-sii8620.o
> >  obj-$(CONFIG_DRM_SII902X) += sii902x.o
> >  obj-$(CONFIG_DRM_SII9234) += sii9234.o
> > +obj-$(CONFIG_DRM_THINE_THC63LVD1024) += thc63lvd1024.o
> >  obj-$(CONFIG_DRM_TOSHIBA_TC358767) += tc358767.o
> >  obj-$(CONFIG_DRM_ANALOGIX_DP) += analogix/
> >  obj-$(CONFIG_DRM_I2C_ADV7511) += adv7511/
> > diff --git a/drivers/gpu/drm/bridge/thc63lvd1024.c b/drivers/gpu/drm/bridge/thc63lvd1024.c
> > new file mode 100644
> > index 0000000..02a54c2
> > --- /dev/null
> > +++ b/drivers/gpu/drm/bridge/thc63lvd1024.c
> > @@ -0,0 +1,255 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * THC63LVD1024 LVDS to parallel data DRM bridge driver.
> > + *
> > + * Copyright (C) 2018 Jacopo Mondi <jacopo+renesas@jmondi.org>
> > + */
> > +
> > +#include <drm/drmP.h>
> > +#include <drm/drm_bridge.h>
> > +#include <drm/drm_panel.h>
> > +
> > +#include <linux/gpio/consumer.h>
> > +#include <linux/of_graph.h>
> > +#include <linux/regulator/consumer.h>
> > +
> > +enum {
> > +	THC63_LVDS_IN0,
> > +	THC63_LVDS_IN1,
> > +	THC63_DIGITAL_OUT0,
> > +	THC63_DIGITAL_OUT1,
> > +};
> > +
> > +static const char * const thc63_reg_names[] = {
> > +	"vcc", "lvcc", "pvcc", "cvcc",
> > +};
> > +
> > +struct thc63_dev {
> > +	struct device *dev;
> > +
> > +	struct regulator *vccs[ARRAY_SIZE(thc63_reg_names)];
> > +
> > +	struct gpio_desc *pdwn;
> > +	struct gpio_desc *oe;
> > +
> > +	struct drm_bridge bridge;
> > +	struct drm_bridge *next;
> > +};
> > +
> > +static inline struct thc63_dev *to_thc63(struct drm_bridge *bridge)
> > +{
> > +	return container_of(bridge, struct thc63_dev, bridge);
> > +}
> > +
> > +static int thc63_attach(struct drm_bridge *bridge)
> > +{
> > +	struct thc63_dev *thc63 = to_thc63(bridge);
> > +
> > +	return drm_bridge_attach(bridge->encoder, thc63->next, bridge);
> > +}
> > +
> > +static void thc63_enable(struct drm_bridge *bridge)
> > +{
> > +	struct thc63_dev *thc63 = to_thc63(bridge);
> > +	struct regulator *vcc;
> > +	int i;
>
> unsigned int i;
>
> > +
> > +	for (i = 0; i < ARRAY_SIZE(thc63->vccs); i++) {
> > +		vcc = thc63->vccs[i];
> > +		if (!vcc)
> > +			continue;
> > +
> > +		if (regulator_enable(vcc))
> > +			goto error_vcc_enable;
> > +	}
> > +
> > +	if (thc63->pdwn)
> > +		gpiod_set_value(thc63->pdwn, 0);
> > +
> > +	if (thc63->oe)
> > +		gpiod_set_value(thc63->oe, 1);
> > +
> > +	return;
> > +
> > +error_vcc_enable:
> > +	dev_err(thc63->dev, "Failed to enable regulator %s\n",
> > +		thc63_reg_names[i]);
> > +
> > +	for (i = i - 1; i >= 0; i--) {
> > +		vcc = thc63->vccs[i];
> > +		if (!vcc)
> > +			continue;
> > +
> > +		regulator_disable(vcc);
> > +	}
> > +}
> > +
> > +static void thc63_disable(struct drm_bridge *bridge)
> > +{
> > +	struct thc63_dev *thc63 = to_thc63(bridge);
> > +	struct regulator *vcc;
> > +	int i;
> > +
> > +	if (thc63->oe)
> > +		gpiod_set_value(thc63->oe, 0);
> > +
> > +	if (thc63->pdwn)
> > +		gpiod_set_value(thc63->pdwn, 1);
> > +
> > +	for (i = ARRAY_SIZE(thc63->vccs) - 1; i >= 0; i--) {
> > +		vcc = thc63->vccs[i];
> > +		if (!vcc)
> > +			continue;
> > +
> > +		if (regulator_disable(vcc))
> > +			dev_dbg(thc63->dev,
> > +				"Failed to disable regulator %s\n",
> > +				thc63_reg_names[i]);
> > +	}
> > +}
> > +
> > +struct drm_bridge_funcs thc63_bridge_func = {
>
> Apparently you missed all my comments from v2 review.
>
I did not :)
v6 was already out when you commented on v2..

> If you like to avoid non-NULL function pointers to the void, please
> use static const storage qualifier.
>
> --
> With best wishes,
> Vladimir
Geert Uytterhoeven March 27, 2018, 7:36 a.m. UTC | #5 
Hi Andrzej,

On Tue, Mar 27, 2018 at 9:28 AM, Andrzej Hajda <a.hajda@samsung.com> wrote:
>>> --- /dev/null
>>> +++ b/drivers/gpu/drm/bridge/thc63lvd1024.c

>>> +static void thc63_enable(struct drm_bridge *bridge)
>>> +{
>>> +    struct thc63_dev *thc63 = to_thc63(bridge);
>>> +    struct regulator *vcc;
>>> +    int i;
>> unsigned int i;
>
> Why? You are introducing silly bug this way, see few lines below.
>
>>
>>> +
>>> +    for (i = 0; i < ARRAY_SIZE(thc63->vccs); i++) {
>>> +            vcc = thc63->vccs[i];
>>> +            if (!vcc)
>>> +                    continue;
>>> +
>>> +            if (regulator_enable(vcc))
>>> +                    goto error_vcc_enable;
>>> +    }
>>> +
>>> +    if (thc63->pdwn)
>>> +            gpiod_set_value(thc63->pdwn, 0);
>>> +
>>> +    if (thc63->oe)
>>> +            gpiod_set_value(thc63->oe, 1);
>>> +
>>> +    return;
>>> +
>>> +error_vcc_enable:
>>> +    dev_err(thc63->dev, "Failed to enable regulator %s\n",
>>> +            thc63_reg_names[i]);
>>> +
>>> +    for (i = i - 1; i >= 0; i--) {
>
> Here, the loop will not end if you define i unsigned.

True.

> I know one can change the loop, to make it working with unsigned. But
> this clearly shows that using unsigned is more risky.
> What are advantages of unsigned vs int in this case. Are there some
> guidelines/discussions about it?

Some people consider signed integers harmful, as they may be converted
silently by the compiler to the "larger" unsigned type when needed.

Gr{oetje,eeting}s,

                        Geert
Andrzej Hajda March 27, 2018, 8:36 a.m. UTC | #6 
On 27.03.2018 09:36, Geert Uytterhoeven wrote:
> Hi Andrzej,
>
> On Tue, Mar 27, 2018 at 9:28 AM, Andrzej Hajda <a.hajda@samsung.com> wrote:
>>>> --- /dev/null
>>>> +++ b/drivers/gpu/drm/bridge/thc63lvd1024.c
>>>> +static void thc63_enable(struct drm_bridge *bridge)
>>>> +{
>>>> +    struct thc63_dev *thc63 = to_thc63(bridge);
>>>> +    struct regulator *vcc;
>>>> +    int i;
>>> unsigned int i;
>> Why? You are introducing silly bug this way, see few lines below.
>>
>>>> +
>>>> +    for (i = 0; i < ARRAY_SIZE(thc63->vccs); i++) {
>>>> +            vcc = thc63->vccs[i];
>>>> +            if (!vcc)
>>>> +                    continue;
>>>> +
>>>> +            if (regulator_enable(vcc))
>>>> +                    goto error_vcc_enable;
>>>> +    }
>>>> +
>>>> +    if (thc63->pdwn)
>>>> +            gpiod_set_value(thc63->pdwn, 0);
>>>> +
>>>> +    if (thc63->oe)
>>>> +            gpiod_set_value(thc63->oe, 1);
>>>> +
>>>> +    return;
>>>> +
>>>> +error_vcc_enable:
>>>> +    dev_err(thc63->dev, "Failed to enable regulator %s\n",
>>>> +            thc63_reg_names[i]);
>>>> +
>>>> +    for (i = i - 1; i >= 0; i--) {
>> Here, the loop will not end if you define i unsigned.
> True.
>
>> I know one can change the loop, to make it working with unsigned. But
>> this clearly shows that using unsigned is more risky.
>> What are advantages of unsigned vs int in this case. Are there some
>> guidelines/discussions about it?
> Some people consider signed integers harmful, as they may be converted
> silently by the compiler to the "larger" unsigned type when needed.

Wow, it sounds crazy, shall we expect gigantic patchsets, converting all
occurrences of int to "unsigned int" ? :)
I know both types have their pros and cons and can behave unexpectedly
in corner cases, but I do not see why unsigned should be preferred over
signed in general, or in this particular case.
I guess there were somewhere discussion about it, could you point me to
it if possible, to avoid unnecessary noise in this thread.

Regards
Andrzej

>
> Gr{oetje,eeting}s,
>
>                         Geert
>
Geert Uytterhoeven March 27, 2018, 9:06 a.m. UTC | #7 
Hi Andrezj,

On Tue, Mar 27, 2018 at 10:36 AM, Andrzej Hajda <a.hajda@samsung.com> wrote:
> On 27.03.2018 09:36, Geert Uytterhoeven wrote:
>> On Tue, Mar 27, 2018 at 9:28 AM, Andrzej Hajda <a.hajda@samsung.com> wrote:
>>>>> --- /dev/null
>>>>> +++ b/drivers/gpu/drm/bridge/thc63lvd1024.c
>>>>> +static void thc63_enable(struct drm_bridge *bridge)
>>>>> +{
>>>>> +    struct thc63_dev *thc63 = to_thc63(bridge);
>>>>> +    struct regulator *vcc;
>>>>> +    int i;
>>>> unsigned int i;
>>> Why? You are introducing silly bug this way, see few lines below.
>>>
>>>>> +
>>>>> +    for (i = 0; i < ARRAY_SIZE(thc63->vccs); i++) {
>>>>> +            vcc = thc63->vccs[i];
>>>>> +            if (!vcc)
>>>>> +                    continue;
>>>>> +
>>>>> +            if (regulator_enable(vcc))
>>>>> +                    goto error_vcc_enable;
>>>>> +    }
>>>>> +
>>>>> +    if (thc63->pdwn)
>>>>> +            gpiod_set_value(thc63->pdwn, 0);
>>>>> +
>>>>> +    if (thc63->oe)
>>>>> +            gpiod_set_value(thc63->oe, 1);
>>>>> +
>>>>> +    return;
>>>>> +
>>>>> +error_vcc_enable:
>>>>> +    dev_err(thc63->dev, "Failed to enable regulator %s\n",
>>>>> +            thc63_reg_names[i]);
>>>>> +
>>>>> +    for (i = i - 1; i >= 0; i--) {
>>> Here, the loop will not end if you define i unsigned.
>> True.
>>
>>> I know one can change the loop, to make it working with unsigned. But
>>> this clearly shows that using unsigned is more risky.
>>> What are advantages of unsigned vs int in this case. Are there some
>>> guidelines/discussions about it?
>> Some people consider signed integers harmful, as they may be converted
>> silently by the compiler to the "larger" unsigned type when needed.
>
> Wow, it sounds crazy, shall we expect gigantic patchsets, converting all
> occurrences of int to "unsigned int" ? :)

No we shall not.

> I know both types have their pros and cons and can behave unexpectedly
> in corner cases, but I do not see why unsigned should be preferred over
> signed in general, or in this particular case.

When looping over array indices, and comparing with ARRAY_SIZE (which
is unsigned), using "unsigned int" is preferred.

However, in this case the error code relies on the index becoming negative,
so a signed integer should be used.

> I guess there were somewhere discussion about it, could you point me to
> it if possible, to avoid unnecessary noise in this thread.

Not here, but Google pointed me to
http://blog.robertelder.org/signed-or-unsigned/

Gr{oetje,eeting}s,

                        Geert
Vladimir Zapolskiy March 27, 2018, 9:57 a.m. UTC | #8 
Hi Andrzej,

On 03/27/2018 10:28 AM, Andrzej Hajda wrote:
> On 27.03.2018 08:24, Vladimir Zapolskiy wrote:
>> Hi Jacopo,
>>
>> On 03/16/2018 05:16 PM, Jacopo Mondi wrote:
>>> Add DRM bridge driver for Thine THC63LVD1024 LVDS to digital parallel
>>> output converter.
>>>
>>> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
>>> Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>
>>> Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
>>> ---
>>>  drivers/gpu/drm/bridge/Kconfig        |   6 +
>>>  drivers/gpu/drm/bridge/Makefile       |   1 +
>>>  drivers/gpu/drm/bridge/thc63lvd1024.c | 255 ++++++++++++++++++++++++++++++++++
>>>  3 files changed, 262 insertions(+)
>>>  create mode 100644 drivers/gpu/drm/bridge/thc63lvd1024.c
>>>
>>> diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
>>> index 3b99d5a..5815a20 100644
>>> --- a/drivers/gpu/drm/bridge/Kconfig
>>> +++ b/drivers/gpu/drm/bridge/Kconfig
>>> @@ -92,6 +92,12 @@ config DRM_SII9234
>>>  	  It is an I2C driver, that detects connection of MHL bridge
>>>  	  and starts encapsulation of HDMI signal.
>>>  
>>> +config DRM_THINE_THC63LVD1024
>>> +	tristate "Thine THC63LVD1024 LVDS decoder bridge"
>>> +	depends on OF
>>> +	---help---
>>> +	  Thine THC63LVD1024 LVDS/parallel converter driver.
>>> +
>>>  config DRM_TOSHIBA_TC358767
>>>  	tristate "Toshiba TC358767 eDP bridge"
>>>  	depends on OF
>>> diff --git a/drivers/gpu/drm/bridge/Makefile b/drivers/gpu/drm/bridge/Makefile
>>> index 373eb28..fd90b16 100644
>>> --- a/drivers/gpu/drm/bridge/Makefile
>>> +++ b/drivers/gpu/drm/bridge/Makefile
>>> @@ -8,6 +8,7 @@ obj-$(CONFIG_DRM_PARADE_PS8622) += parade-ps8622.o
>>>  obj-$(CONFIG_DRM_SIL_SII8620) += sil-sii8620.o
>>>  obj-$(CONFIG_DRM_SII902X) += sii902x.o
>>>  obj-$(CONFIG_DRM_SII9234) += sii9234.o
>>> +obj-$(CONFIG_DRM_THINE_THC63LVD1024) += thc63lvd1024.o
>>>  obj-$(CONFIG_DRM_TOSHIBA_TC358767) += tc358767.o
>>>  obj-$(CONFIG_DRM_ANALOGIX_DP) += analogix/
>>>  obj-$(CONFIG_DRM_I2C_ADV7511) += adv7511/
>>> diff --git a/drivers/gpu/drm/bridge/thc63lvd1024.c b/drivers/gpu/drm/bridge/thc63lvd1024.c
>>> new file mode 100644
>>> index 0000000..02a54c2
>>> --- /dev/null
>>> +++ b/drivers/gpu/drm/bridge/thc63lvd1024.c
>>> @@ -0,0 +1,255 @@
>>> +// SPDX-License-Identifier: GPL-2.0
>>> +/*
>>> + * THC63LVD1024 LVDS to parallel data DRM bridge driver.
>>> + *
>>> + * Copyright (C) 2018 Jacopo Mondi <jacopo+renesas@jmondi.org>
>>> + */
>>> +
>>> +#include <drm/drmP.h>
>>> +#include <drm/drm_bridge.h>
>>> +#include <drm/drm_panel.h>
>>> +
>>> +#include <linux/gpio/consumer.h>
>>> +#include <linux/of_graph.h>
>>> +#include <linux/regulator/consumer.h>
>>> +
>>> +enum {
>>> +	THC63_LVDS_IN0,
>>> +	THC63_LVDS_IN1,
>>> +	THC63_DIGITAL_OUT0,
>>> +	THC63_DIGITAL_OUT1,
>>> +};
>>> +
>>> +static const char * const thc63_reg_names[] = {
>>> +	"vcc", "lvcc", "pvcc", "cvcc",
>>> +};
>>> +
>>> +struct thc63_dev {
>>> +	struct device *dev;
>>> +
>>> +	struct regulator *vccs[ARRAY_SIZE(thc63_reg_names)];
>>> +
>>> +	struct gpio_desc *pdwn;
>>> +	struct gpio_desc *oe;
>>> +
>>> +	struct drm_bridge bridge;
>>> +	struct drm_bridge *next;
>>> +};
>>> +
>>> +static inline struct thc63_dev *to_thc63(struct drm_bridge *bridge)
>>> +{
>>> +	return container_of(bridge, struct thc63_dev, bridge);
>>> +}
>>> +
>>> +static int thc63_attach(struct drm_bridge *bridge)
>>> +{
>>> +	struct thc63_dev *thc63 = to_thc63(bridge);
>>> +
>>> +	return drm_bridge_attach(bridge->encoder, thc63->next, bridge);
>>> +}
>>> +
>>> +static void thc63_enable(struct drm_bridge *bridge)
>>> +{
>>> +	struct thc63_dev *thc63 = to_thc63(bridge);
>>> +	struct regulator *vcc;
>>> +	int i;
>> unsigned int i;
> 
> Why? You are introducing silly bug this way, see few lines below.

You see that the comment was too terse to describe the context in full.
Thank you for exposing a flaw.

>>
>>> +
>>> +	for (i = 0; i < ARRAY_SIZE(thc63->vccs); i++) {
>>> +		vcc = thc63->vccs[i];
>>> +		if (!vcc)
>>> +			continue;
>>> +
>>> +		if (regulator_enable(vcc))
>>> +			goto error_vcc_enable;
>>> +	}
>>> +
>>> +	if (thc63->pdwn)
>>> +		gpiod_set_value(thc63->pdwn, 0);
>>> +
>>> +	if (thc63->oe)
>>> +		gpiod_set_value(thc63->oe, 1);
>>> +
>>> +	return;
>>> +
>>> +error_vcc_enable:
>>> +	dev_err(thc63->dev, "Failed to enable regulator %s\n",
>>> +		thc63_reg_names[i]);
>>> +
>>> +	for (i = i - 1; i >= 0; i--) {
> 
> Here, the loop will not end if you define i unsigned.
> 
> I know one can change the loop, to make it working with unsigned. But
> this clearly shows that using unsigned is more risky.
> What are advantages of unsigned vs int in this case. Are there some
> guidelines/discussions about it?
> 

The reason is pretty simple, like Geert said it might be a personal reason,
but a code pattern 

	int i;
	...
	for (i = 0; i < ARRAY_SIZE(...); i++)

is my own red rag, because I've seen the pattern so many times, and every
time here a compiler produces a W=12 (or -Wsign-compare) warning like
the next one:

drivers/gpu/drm/bridge/thc63lvd1024.c:182:16: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]
  for (i = 0; i < ARRAY_SIZE(thc63->vccs); i++, reg++) {
                ^

Usually a conversion from 'int i' to 'unsigned int i' is trivial, and lowering
of a noise level in compiler's output is seen as beneficial, because it gives
more chances to people to capture a real problem.

--
With best wishes,
Vladimir
diff mbox

Patch

diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
index 3b99d5a..5815a20 100644
--- a/drivers/gpu/drm/bridge/Kconfig
+++ b/drivers/gpu/drm/bridge/Kconfig
@@ -92,6 +92,12 @@  config DRM_SII9234
 	  It is an I2C driver, that detects connection of MHL bridge
 	  and starts encapsulation of HDMI signal.
 
+config DRM_THINE_THC63LVD1024
+	tristate "Thine THC63LVD1024 LVDS decoder bridge"
+	depends on OF
+	---help---
+	  Thine THC63LVD1024 LVDS/parallel converter driver.
+
 config DRM_TOSHIBA_TC358767
 	tristate "Toshiba TC358767 eDP bridge"
 	depends on OF
diff --git a/drivers/gpu/drm/bridge/Makefile b/drivers/gpu/drm/bridge/Makefile
index 373eb28..fd90b16 100644
--- a/drivers/gpu/drm/bridge/Makefile
+++ b/drivers/gpu/drm/bridge/Makefile
@@ -8,6 +8,7 @@  obj-$(CONFIG_DRM_PARADE_PS8622) += parade-ps8622.o
 obj-$(CONFIG_DRM_SIL_SII8620) += sil-sii8620.o
 obj-$(CONFIG_DRM_SII902X) += sii902x.o
 obj-$(CONFIG_DRM_SII9234) += sii9234.o
+obj-$(CONFIG_DRM_THINE_THC63LVD1024) += thc63lvd1024.o
 obj-$(CONFIG_DRM_TOSHIBA_TC358767) += tc358767.o
 obj-$(CONFIG_DRM_ANALOGIX_DP) += analogix/
 obj-$(CONFIG_DRM_I2C_ADV7511) += adv7511/
diff --git a/drivers/gpu/drm/bridge/thc63lvd1024.c b/drivers/gpu/drm/bridge/thc63lvd1024.c
new file mode 100644
index 0000000..02a54c2
--- /dev/null
+++ b/drivers/gpu/drm/bridge/thc63lvd1024.c
@@ -0,0 +1,255 @@ 
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * THC63LVD1024 LVDS to parallel data DRM bridge driver.
+ *
+ * Copyright (C) 2018 Jacopo Mondi <jacopo+renesas@jmondi.org>
+ */
+
+#include <drm/drmP.h>
+#include <drm/drm_bridge.h>
+#include <drm/drm_panel.h>
+
+#include <linux/gpio/consumer.h>
+#include <linux/of_graph.h>
+#include <linux/regulator/consumer.h>
+
+enum {
+	THC63_LVDS_IN0,
+	THC63_LVDS_IN1,
+	THC63_DIGITAL_OUT0,
+	THC63_DIGITAL_OUT1,
+};
+
+static const char * const thc63_reg_names[] = {
+	"vcc", "lvcc", "pvcc", "cvcc",
+};
+
+struct thc63_dev {
+	struct device *dev;
+
+	struct regulator *vccs[ARRAY_SIZE(thc63_reg_names)];
+
+	struct gpio_desc *pdwn;
+	struct gpio_desc *oe;
+
+	struct drm_bridge bridge;
+	struct drm_bridge *next;
+};
+
+static inline struct thc63_dev *to_thc63(struct drm_bridge *bridge)
+{
+	return container_of(bridge, struct thc63_dev, bridge);
+}
+
+static int thc63_attach(struct drm_bridge *bridge)
+{
+	struct thc63_dev *thc63 = to_thc63(bridge);
+
+	return drm_bridge_attach(bridge->encoder, thc63->next, bridge);
+}
+
+static void thc63_enable(struct drm_bridge *bridge)
+{
+	struct thc63_dev *thc63 = to_thc63(bridge);
+	struct regulator *vcc;
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(thc63->vccs); i++) {
+		vcc = thc63->vccs[i];
+		if (!vcc)
+			continue;
+
+		if (regulator_enable(vcc))
+			goto error_vcc_enable;
+	}
+
+	if (thc63->pdwn)
+		gpiod_set_value(thc63->pdwn, 0);
+
+	if (thc63->oe)
+		gpiod_set_value(thc63->oe, 1);
+
+	return;
+
+error_vcc_enable:
+	dev_err(thc63->dev, "Failed to enable regulator %s\n",
+		thc63_reg_names[i]);
+
+	for (i = i - 1; i >= 0; i--) {
+		vcc = thc63->vccs[i];
+		if (!vcc)
+			continue;
+
+		regulator_disable(vcc);
+	}
+}
+
+static void thc63_disable(struct drm_bridge *bridge)
+{
+	struct thc63_dev *thc63 = to_thc63(bridge);
+	struct regulator *vcc;
+	int i;
+
+	if (thc63->oe)
+		gpiod_set_value(thc63->oe, 0);
+
+	if (thc63->pdwn)
+		gpiod_set_value(thc63->pdwn, 1);
+
+	for (i = ARRAY_SIZE(thc63->vccs) - 1; i >= 0; i--) {
+		vcc = thc63->vccs[i];
+		if (!vcc)
+			continue;
+
+		if (regulator_disable(vcc))
+			dev_dbg(thc63->dev,
+				"Failed to disable regulator %s\n",
+				thc63_reg_names[i]);
+	}
+}
+
+struct drm_bridge_funcs thc63_bridge_func = {
+	.attach	= thc63_attach,
+	.enable = thc63_enable,
+	.disable = thc63_disable,
+};
+
+static int thc63_parse_dt(struct thc63_dev *thc63)
+{
+	struct device_node *thc63_out;
+	struct device_node *remote;
+	int ret = 0;
+
+	thc63_out = of_graph_get_endpoint_by_regs(thc63->dev->of_node,
+						  THC63_DIGITAL_OUT0, -1);
+	if (!thc63_out) {
+		dev_err(thc63->dev, "Missing endpoint in Port@%u\n",
+			THC63_DIGITAL_OUT0);
+		return -ENODEV;
+	}
+
+	remote = of_graph_get_remote_port_parent(thc63_out);
+	if (!remote) {
+		dev_err(thc63->dev, "Endpoint in Port@%u unconnected\n",
+			THC63_DIGITAL_OUT0);
+		ret = -ENODEV;
+		goto error_put_out_node;
+	}
+
+	if (!of_device_is_available(remote)) {
+		dev_err(thc63->dev, "Port@%u remote endpoint is disabled\n",
+			THC63_DIGITAL_OUT0);
+		ret = -ENODEV;
+		goto error_put_remote_node;
+	}
+
+	thc63->next = of_drm_find_bridge(remote);
+	if (!thc63->next)
+		ret = -EPROBE_DEFER;
+
+error_put_remote_node:
+	of_node_put(remote);
+error_put_out_node:
+	of_node_put(thc63_out);
+
+	return ret;
+}
+
+static int thc63_gpio_init(struct thc63_dev *thc63)
+{
+	thc63->oe = devm_gpiod_get_optional(thc63->dev, "oe", GPIOD_OUT_LOW);
+	if (IS_ERR(thc63->oe)) {
+		dev_err(thc63->dev, "Unable to get \"oe-gpios\"\n");
+		return PTR_ERR(thc63->oe);
+	}
+
+	thc63->pdwn = devm_gpiod_get_optional(thc63->dev, "pdwn",
+					      GPIOD_OUT_HIGH);
+	if (IS_ERR(thc63->pdwn)) {
+		dev_err(thc63->dev, "Unable to get \"pdwn-gpios\"\n");
+		return PTR_ERR(thc63->pdwn);
+	}
+
+	return 0;
+}
+
+static int thc63_regulator_init(struct thc63_dev *thc63)
+{
+	struct regulator **reg;
+	int i;
+
+	reg = &thc63->vccs[0];
+	for (i = 0; i < ARRAY_SIZE(thc63->vccs); i++, reg++) {
+		*reg = devm_regulator_get_optional(thc63->dev,
+						   thc63_reg_names[i]);
+		if (IS_ERR(*reg)) {
+			if (PTR_ERR(*reg) == -EPROBE_DEFER)
+				return -EPROBE_DEFER;
+			*reg = NULL;
+		}
+	}
+
+	return 0;
+}
+
+static int thc63_probe(struct platform_device *pdev)
+{
+	struct thc63_dev *thc63;
+	int ret;
+
+	thc63 = devm_kzalloc(&pdev->dev, sizeof(*thc63), GFP_KERNEL);
+	if (!thc63)
+		return -ENOMEM;
+
+	thc63->dev = &pdev->dev;
+	platform_set_drvdata(pdev, thc63);
+
+	ret = thc63_regulator_init(thc63);
+	if (ret)
+		return ret;
+
+	ret = thc63_gpio_init(thc63);
+	if (ret)
+		return ret;
+
+	ret = thc63_parse_dt(thc63);
+	if (ret)
+		return ret;
+
+	thc63->bridge.driver_private = thc63;
+	thc63->bridge.of_node = pdev->dev.of_node;
+	thc63->bridge.funcs = &thc63_bridge_func;
+
+	drm_bridge_add(&thc63->bridge);
+
+	return 0;
+}
+
+static int thc63_remove(struct platform_device *pdev)
+{
+	struct thc63_dev *thc63 = platform_get_drvdata(pdev);
+
+	drm_bridge_remove(&thc63->bridge);
+
+	return 0;
+}
+
+static const struct of_device_id thc63_match[] = {
+	{ .compatible = "thine,thc63lvd1024", },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, thc63_match);
+
+static struct platform_driver thc63_driver = {
+	.probe	= thc63_probe,
+	.remove	= thc63_remove,
+	.driver	= {
+		.name		= "thc63lvd1024",
+		.of_match_table	= thc63_match,
+	},
+};
+module_platform_driver(thc63_driver);
+
+MODULE_AUTHOR("Jacopo Mondi <jacopo@jmondi.org>");
+MODULE_DESCRIPTION("Thine THC63LVD1024 LVDS decoder DRM bridge driver");
+MODULE_LICENSE("GPL v2");