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@xxxxxxxxxxxxxxx> >> --- >> >> 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 devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html