linux-kernel - Re: [PATCH 09/10] [media] tvp5150: Initialize the chip on probe

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <568CFCD9.1010809@osg.samsung.com>
Date:	Wed, 6 Jan 2016 08:39:05 -0300
From:	Javier Martinez Canillas <javier@....samsung.com>
To:	Laurent Pinchart <laurent.pinchart@...asonboard.com>
Cc:	linux-kernel@...r.kernel.org, devicetree@...r.kernel.org,
	Mauro Carvalho Chehab <mchehab@....samsung.com>,
	Enrico Butera <ebutera@...il.com>,
	Sakari Ailus <sakari.ailus@...ux.intel.com>,
	Enric Balletbo i Serra <eballetbo@...il.com>,
	Rob Herring <robh+dt@...nel.org>,
	Eduard Gavin <egavinc@...il.com>,
	Hans Verkuil <hans.verkuil@...co.com>,
	linux-media@...r.kernel.org
Subject: Re: [PATCH 09/10] [media] tvp5150: Initialize the chip on probe

Hello Laurent,

On 01/06/2016 07:59 AM, Laurent Pinchart wrote:
> Hi Javier,
> 
> Thankk you for the patch.
>

Thanks a lot for your feedback.
 
> On Monday 04 January 2016 09:25:31 Javier Martinez Canillas wrote:
>> After power-up, the tvp5150 decoder is in a unknown state until the
>> RESETB pin is driven LOW which reset all the registers and restarts
>> the chip's internal state machine.
>>
>> The init sequence has some timing constraints and the RESETB signal
>> can only be used if the PDN (Power-down) pin is first released.
>>
>> So, the initialization sequence is as follows:
>>
>> 1- PDN (active-low) is driven HIGH so the chip is power-up
>> 2- A 20 ms delay is needed before sending a RESETB (active-low) signal.
>> 3- The RESETB pulse duration is 500 ns.
>> 4- A 200 us delay is needed for the I2C client to be active after reset.
>>
>> This patch used as a reference the logic in the IGEPv2 board file from
>> the ISEE 2.6.37 vendor tree.
>>
>> Signed-off-by: Javier Martinez Canillas <javier@....samsung.com>
>> ---
>>
>>  drivers/media/i2c/tvp5150.c | 35 +++++++++++++++++++++++++++++++++++
>>  1 file changed, 35 insertions(+)
>>
>> diff --git a/drivers/media/i2c/tvp5150.c b/drivers/media/i2c/tvp5150.c
>> index caac96a577f8..fed89a811ab7 100644
>> --- a/drivers/media/i2c/tvp5150.c
>> +++ b/drivers/media/i2c/tvp5150.c
>> @@ -5,6 +5,7 @@
>>   * This code is placed under the terms of the GNU General Public License v2
>> */
>>
>> +#include <linux/of_gpio.h>
> 
> Let's keep the headers sorted alphabetically if you don't mind :-)
>

Right, sorry about that.
 
>>  #include <linux/i2c.h>
>>  #include <linux/slab.h>
>>  #include <linux/videodev2.h>
>> @@ -1197,6 +1198,36 @@ static int tvp5150_detect_version(struct tvp5150
>> *core) return 0;
>>  }
>>
>> +static inline int tvp5150_init(struct i2c_client *c)
>> +{
>> +	struct gpio_desc *pdn_gpio;
>> +	struct gpio_desc *reset_gpio;
>> +
>> +	pdn_gpio = devm_gpiod_get_optional(&c->dev, "powerdown", GPIOD_OUT_HIGH);
>> +	if (IS_ERR(pdn_gpio))
>> +		return PTR_ERR(pdn_gpio);
>> +
>> +	if (pdn_gpio) {
>> +		gpiod_set_value_cansleep(pdn_gpio, 0);
>> +		/* Delay time between power supplies active and reset */
>> +		msleep(20);
> 
> How about usleep_range() instead ?
>

Documentation/timers/timers-howto.txt says that usleep_range() should be
used for 1ms - 20ms and msleep() for 20ms+ so since this was a boundary
value, I chose for msleep() but I can use usleep_range() if you want.

I've no strong opinion on this.

>> +	}
>> +
>> +	reset_gpio = devm_gpiod_get_optional(&c->dev, "reset", GPIOD_OUT_HIGH);
>> +	if (IS_ERR(reset_gpio))
>> +		return PTR_ERR(reset_gpio);
>> +
>> +	if (reset_gpio) {
>> +		/* RESETB pulse duration */
>> +		ndelay(500);
> 
> Is the timing so sensitive that we need a delay, or could we use 
> usleep_range() ?
>

According to my tests, it is a minimum value but I chose to do a delay since
the time is very short and also Documentation/timers/timers-howto.txt says
that using usleep for very short periods may not be worth it due the overhead
of setting up the hrtimers for usleep.

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/