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

[Vladimir Zapolskiy]

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@xxxxxxxxxx>
>>> Reviewed-by: Andrzej Hajda <a.hajda@xxxxxxxxxxx>
>>> Reviewed-by: Niklas SÃderlund <niklas.soderlund+renesas@xxxxxxxxxxxx>
>>> ---
>>>  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@xxxxxxxxxx>
>>> + */
>>> +
>>> +#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