Re: [PATCH 1/3] [media] mt9v032: Add reset and standby gpios
Hi Markus, On Monday 09 November 2015 16:33:03 Markus Pargmann wrote: > On Monday 09 November 2015 14:28:56 Laurent Pinchart wrote: > > On Friday 06 November 2015 14:13:43 Markus Pargmann wrote: > >> Add optional reset and standby gpios. The reset gpio is used to reset > >> the chip in power_on(). > >> > >> The standby gpio is not used currently. It is just unset, so the chip is > >> not in standby. > > > > We could use a gpio hog for this, but given that the standby signal should > > eventually get used, and given that specifying it in DT is a good hardware > > description, that looks good to me. > > > >> Signed-off-by: Markus Pargmann > >> Reviewed-by: Philipp Zabel > >> --- > >> > >> .../devicetree/bindings/media/i2c/mt9v032.txt | 2 ++ > >> drivers/media/i2c/mt9v032.c| 23 +++ > >> 2 files changed, 25 insertions(+) [snip] > > If you're fine with these changes there's no need to resubmit the patch, I > > can fix it when applying it to my tree. > > Thanks, I am fine with all your changes. But as there will be a v2 for the > other two patches I could as well send an updated version if you wish. As you wish, both options are fine with me. -- Regards, Laurent Pinchart -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] [media] mt9v032: Add reset and standby gpios
Hi, On Monday 09 November 2015 14:28:56 Laurent Pinchart wrote: > Hi Markus, > > Thank you for the patch. > > On Friday 06 November 2015 14:13:43 Markus Pargmann wrote: > > Add optional reset and standby gpios. The reset gpio is used to reset > > the chip in power_on(). > > > > The standby gpio is not used currently. It is just unset, so the chip is > > not in standby. > > We could use a gpio hog for this, but given that the standby signal should > eventually get used, and given that specifying it in DT is a good hardware > description, that looks good to me. > > > Signed-off-by: Markus Pargmann > > Reviewed-by: Philipp Zabel > > --- > > .../devicetree/bindings/media/i2c/mt9v032.txt | 2 ++ > > drivers/media/i2c/mt9v032.c| 23 +++ > > 2 files changed, 25 insertions(+) > > > > diff --git a/Documentation/devicetree/bindings/media/i2c/mt9v032.txt > > b/Documentation/devicetree/bindings/media/i2c/mt9v032.txt index > > 202565313e82..100f0ae43269 100644 > > --- a/Documentation/devicetree/bindings/media/i2c/mt9v032.txt > > +++ b/Documentation/devicetree/bindings/media/i2c/mt9v032.txt > > @@ -20,6 +20,8 @@ Optional Properties: > > > > - link-frequencies: List of allowed link frequencies in Hz. Each frequency > > is expressed as a 64-bit big-endian integer. > > +- reset-gpios: GPIO handle which is connected to the reset pin of the chip. > > +- standby-gpios: GPIO handle which is connected to the standby pin of the > > chip. > > > > For further reading on port node refer to > > Documentation/devicetree/bindings/media/video-interfaces.txt. > > diff --git a/drivers/media/i2c/mt9v032.c b/drivers/media/i2c/mt9v032.c > > index a68ce94ee097..4aefde9634f5 100644 > > --- a/drivers/media/i2c/mt9v032.c > > +++ b/drivers/media/i2c/mt9v032.c > > @@ -24,6 +24,7 @@ > > #include > > #include > > #include > > +#include > > module.h escaped my vigilance, but let's try to keep headers alphabetically > sorted. > > > > #include > > #include > > @@ -251,6 +252,8 @@ struct mt9v032 { > > > > struct regmap *regmap; > > struct clk *clk; > > + struct gpio_desc *reset_gpio; > > + struct gpio_desc *standby_gpio; > > > > struct mt9v032_platform_data *pdata; > > const struct mt9v032_model_info *model; > > @@ -312,16 +315,26 @@ static int mt9v032_power_on(struct mt9v032 *mt9v032) > > struct regmap *map = mt9v032->regmap; > > int ret; > > > > + gpiod_set_value_cansleep(mt9v032->reset_gpio, 1); > > + > > ret = clk_set_rate(mt9v032->clk, mt9v032->sysclk); > > if (ret < 0) > > return ret; > > > > + /* system clock has to be enabled before releasing the reset */ > > Nitpicking, the driver capitalizes the first letter of comments. > > > ret = clk_prepare_enable(mt9v032->clk); > > if (ret) > > return ret; > > > > udelay(1); > > > > + gpiod_set_value_cansleep(mt9v032->reset_gpio, 0); > > + > > + /* > > +* After releasing reset, it can take up to 1us until the chip is done > > +*/ > > + udelay(1); > > + > > The delay isn't necessary if there's no reset GPIO. How about > > if (mt9v032->reset_gpio) { > gpiod_set_value_cansleep(mt9v032->reset_gpio, 0); > > /* After releasing reset, it can take up to 1us until the >* chip is done. >*/ > udelay(1); > } > > And, according to the datasheet, the delay is 10 SYSCLK periods. 1µs should > be > safe as the minimum SYSCLK frequency is 13 MHz. I'd still mention it in a > comment, maybe as > > /* After releasing reset we need to wait 10 clock cycles >* before accessing the sensor over I2C. As the minimum SYSCLK >* frequency is 13MHz, waiting 1µs will be enough in the worst >* case. >*/ > udelay(1); > > If you're fine with these changes there's no need to resubmit the patch, I > can > fix it when applying it to my tree. Thanks, I am fine with all your changes. But as there will be a v2 for the other two patches I could as well send an updated version if you wish. Thanks, Markus -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- | signature.asc Description: This is a digitally signed message part.
Re: [PATCH 1/3] [media] mt9v032: Add reset and standby gpios
Hi Markus, Thank you for the patch. On Friday 06 November 2015 14:13:43 Markus Pargmann wrote: > Add optional reset and standby gpios. The reset gpio is used to reset > the chip in power_on(). > > The standby gpio is not used currently. It is just unset, so the chip is > not in standby. We could use a gpio hog for this, but given that the standby signal should eventually get used, and given that specifying it in DT is a good hardware description, that looks good to me. > Signed-off-by: Markus Pargmann > Reviewed-by: Philipp Zabel > --- > .../devicetree/bindings/media/i2c/mt9v032.txt | 2 ++ > drivers/media/i2c/mt9v032.c| 23 +++ > 2 files changed, 25 insertions(+) > > diff --git a/Documentation/devicetree/bindings/media/i2c/mt9v032.txt > b/Documentation/devicetree/bindings/media/i2c/mt9v032.txt index > 202565313e82..100f0ae43269 100644 > --- a/Documentation/devicetree/bindings/media/i2c/mt9v032.txt > +++ b/Documentation/devicetree/bindings/media/i2c/mt9v032.txt > @@ -20,6 +20,8 @@ Optional Properties: > > - link-frequencies: List of allowed link frequencies in Hz. Each frequency > is expressed as a 64-bit big-endian integer. > +- reset-gpios: GPIO handle which is connected to the reset pin of the chip. > +- standby-gpios: GPIO handle which is connected to the standby pin of the > chip. > > For further reading on port node refer to > Documentation/devicetree/bindings/media/video-interfaces.txt. > diff --git a/drivers/media/i2c/mt9v032.c b/drivers/media/i2c/mt9v032.c > index a68ce94ee097..4aefde9634f5 100644 > --- a/drivers/media/i2c/mt9v032.c > +++ b/drivers/media/i2c/mt9v032.c > @@ -24,6 +24,7 @@ > #include > #include > #include > +#include module.h escaped my vigilance, but let's try to keep headers alphabetically sorted. > > #include > #include > @@ -251,6 +252,8 @@ struct mt9v032 { > > struct regmap *regmap; > struct clk *clk; > + struct gpio_desc *reset_gpio; > + struct gpio_desc *standby_gpio; > > struct mt9v032_platform_data *pdata; > const struct mt9v032_model_info *model; > @@ -312,16 +315,26 @@ static int mt9v032_power_on(struct mt9v032 *mt9v032) > struct regmap *map = mt9v032->regmap; > int ret; > > + gpiod_set_value_cansleep(mt9v032->reset_gpio, 1); > + > ret = clk_set_rate(mt9v032->clk, mt9v032->sysclk); > if (ret < 0) > return ret; > > + /* system clock has to be enabled before releasing the reset */ Nitpicking, the driver capitalizes the first letter of comments. > ret = clk_prepare_enable(mt9v032->clk); > if (ret) > return ret; > > udelay(1); > > + gpiod_set_value_cansleep(mt9v032->reset_gpio, 0); > + > + /* > + * After releasing reset, it can take up to 1us until the chip is done > + */ > + udelay(1); > + The delay isn't necessary if there's no reset GPIO. How about if (mt9v032->reset_gpio) { gpiod_set_value_cansleep(mt9v032->reset_gpio, 0); /* After releasing reset, it can take up to 1us until the * chip is done. */ udelay(1); } And, according to the datasheet, the delay is 10 SYSCLK periods. 1µs should be safe as the minimum SYSCLK frequency is 13 MHz. I'd still mention it in a comment, maybe as /* After releasing reset we need to wait 10 clock cycles * before accessing the sensor over I2C. As the minimum SYSCLK * frequency is 13MHz, waiting 1µs will be enough in the worst * case. */ udelay(1); If you're fine with these changes there's no need to resubmit the patch, I can fix it when applying it to my tree. > /* Reset the chip and stop data read out */ > ret = regmap_write(map, MT9V032_RESET, 1); > if (ret < 0) > @@ -954,6 +967,16 @@ static int mt9v032_probe(struct i2c_client *client, > if (IS_ERR(mt9v032->clk)) > return PTR_ERR(mt9v032->clk); > > + mt9v032->reset_gpio = devm_gpiod_get_optional(&client->dev, "reset", > + GPIOD_OUT_HIGH); > + if (IS_ERR(mt9v032->reset_gpio)) > + return PTR_ERR(mt9v032->reset_gpio); > + > + mt9v032->standby_gpio = devm_gpiod_get_optional(&client->dev, "standby", > + GPIOD_OUT_LOW); > + if (IS_ERR(mt9v032->standby_gpio)) > + return PTR_ERR(mt9v032->standby_gpio); > + > mutex_init(&mt9v032->power_lock); > mt9v032->pdata = pdata; > mt9v032->model = (const void *)did->driver_data; -- Regards, Laurent Pinchart -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] [media] mt9v032: Add reset and standby gpios
On Fri, Nov 06, 2015 at 02:13:43PM +0100, Markus Pargmann wrote: > Add optional reset and standby gpios. The reset gpio is used to reset > the chip in power_on(). > > The standby gpio is not used currently. It is just unset, so the chip is > not in standby. > > Signed-off-by: Markus Pargmann > Reviewed-by: Philipp Zabel > --- > .../devicetree/bindings/media/i2c/mt9v032.txt | 2 ++ Acked-by: Rob Herring > drivers/media/i2c/mt9v032.c| 23 > ++ > 2 files changed, 25 insertions(+) > > diff --git a/Documentation/devicetree/bindings/media/i2c/mt9v032.txt > b/Documentation/devicetree/bindings/media/i2c/mt9v032.txt > index 202565313e82..100f0ae43269 100644 > --- a/Documentation/devicetree/bindings/media/i2c/mt9v032.txt > +++ b/Documentation/devicetree/bindings/media/i2c/mt9v032.txt > @@ -20,6 +20,8 @@ Optional Properties: > > - link-frequencies: List of allowed link frequencies in Hz. Each frequency is > expressed as a 64-bit big-endian integer. > +- reset-gpios: GPIO handle which is connected to the reset pin of the chip. > +- standby-gpios: GPIO handle which is connected to the standby pin of the > chip. > > For further reading on port node refer to > Documentation/devicetree/bindings/media/video-interfaces.txt. > diff --git a/drivers/media/i2c/mt9v032.c b/drivers/media/i2c/mt9v032.c > index a68ce94ee097..4aefde9634f5 100644 > --- a/drivers/media/i2c/mt9v032.c > +++ b/drivers/media/i2c/mt9v032.c > @@ -24,6 +24,7 @@ > #include > #include > #include > +#include > > #include > #include > @@ -251,6 +252,8 @@ struct mt9v032 { > > struct regmap *regmap; > struct clk *clk; > + struct gpio_desc *reset_gpio; > + struct gpio_desc *standby_gpio; > > struct mt9v032_platform_data *pdata; > const struct mt9v032_model_info *model; > @@ -312,16 +315,26 @@ static int mt9v032_power_on(struct mt9v032 *mt9v032) > struct regmap *map = mt9v032->regmap; > int ret; > > + gpiod_set_value_cansleep(mt9v032->reset_gpio, 1); > + > ret = clk_set_rate(mt9v032->clk, mt9v032->sysclk); > if (ret < 0) > return ret; > > + /* system clock has to be enabled before releasing the reset */ > ret = clk_prepare_enable(mt9v032->clk); > if (ret) > return ret; > > udelay(1); > > + gpiod_set_value_cansleep(mt9v032->reset_gpio, 0); > + > + /* > + * After releasing reset, it can take up to 1us until the chip is done > + */ > + udelay(1); > + > /* Reset the chip and stop data read out */ > ret = regmap_write(map, MT9V032_RESET, 1); > if (ret < 0) > @@ -954,6 +967,16 @@ static int mt9v032_probe(struct i2c_client *client, > if (IS_ERR(mt9v032->clk)) > return PTR_ERR(mt9v032->clk); > > + mt9v032->reset_gpio = devm_gpiod_get_optional(&client->dev, "reset", > + GPIOD_OUT_HIGH); > + if (IS_ERR(mt9v032->reset_gpio)) > + return PTR_ERR(mt9v032->reset_gpio); > + > + mt9v032->standby_gpio = devm_gpiod_get_optional(&client->dev, "standby", > + GPIOD_OUT_LOW); > + if (IS_ERR(mt9v032->standby_gpio)) > + return PTR_ERR(mt9v032->standby_gpio); > + > mutex_init(&mt9v032->power_lock); > mt9v032->pdata = pdata; > mt9v032->model = (const void *)did->driver_data; > -- > 2.6.1 > > -- > To unsubscribe from this list: send the line "unsubscribe devicetree" in > the body of a message to majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/3] [media] mt9v032: Add reset and standby gpios
Add optional reset and standby gpios. The reset gpio is used to reset the chip in power_on(). The standby gpio is not used currently. It is just unset, so the chip is not in standby. Signed-off-by: Markus Pargmann Reviewed-by: Philipp Zabel --- .../devicetree/bindings/media/i2c/mt9v032.txt | 2 ++ drivers/media/i2c/mt9v032.c| 23 ++ 2 files changed, 25 insertions(+) diff --git a/Documentation/devicetree/bindings/media/i2c/mt9v032.txt b/Documentation/devicetree/bindings/media/i2c/mt9v032.txt index 202565313e82..100f0ae43269 100644 --- a/Documentation/devicetree/bindings/media/i2c/mt9v032.txt +++ b/Documentation/devicetree/bindings/media/i2c/mt9v032.txt @@ -20,6 +20,8 @@ Optional Properties: - link-frequencies: List of allowed link frequencies in Hz. Each frequency is expressed as a 64-bit big-endian integer. +- reset-gpios: GPIO handle which is connected to the reset pin of the chip. +- standby-gpios: GPIO handle which is connected to the standby pin of the chip. For further reading on port node refer to Documentation/devicetree/bindings/media/video-interfaces.txt. diff --git a/drivers/media/i2c/mt9v032.c b/drivers/media/i2c/mt9v032.c index a68ce94ee097..4aefde9634f5 100644 --- a/drivers/media/i2c/mt9v032.c +++ b/drivers/media/i2c/mt9v032.c @@ -24,6 +24,7 @@ #include #include #include +#include #include #include @@ -251,6 +252,8 @@ struct mt9v032 { struct regmap *regmap; struct clk *clk; + struct gpio_desc *reset_gpio; + struct gpio_desc *standby_gpio; struct mt9v032_platform_data *pdata; const struct mt9v032_model_info *model; @@ -312,16 +315,26 @@ static int mt9v032_power_on(struct mt9v032 *mt9v032) struct regmap *map = mt9v032->regmap; int ret; + gpiod_set_value_cansleep(mt9v032->reset_gpio, 1); + ret = clk_set_rate(mt9v032->clk, mt9v032->sysclk); if (ret < 0) return ret; + /* system clock has to be enabled before releasing the reset */ ret = clk_prepare_enable(mt9v032->clk); if (ret) return ret; udelay(1); + gpiod_set_value_cansleep(mt9v032->reset_gpio, 0); + + /* +* After releasing reset, it can take up to 1us until the chip is done +*/ + udelay(1); + /* Reset the chip and stop data read out */ ret = regmap_write(map, MT9V032_RESET, 1); if (ret < 0) @@ -954,6 +967,16 @@ static int mt9v032_probe(struct i2c_client *client, if (IS_ERR(mt9v032->clk)) return PTR_ERR(mt9v032->clk); + mt9v032->reset_gpio = devm_gpiod_get_optional(&client->dev, "reset", + GPIOD_OUT_HIGH); + if (IS_ERR(mt9v032->reset_gpio)) + return PTR_ERR(mt9v032->reset_gpio); + + mt9v032->standby_gpio = devm_gpiod_get_optional(&client->dev, "standby", + GPIOD_OUT_LOW); + if (IS_ERR(mt9v032->standby_gpio)) + return PTR_ERR(mt9v032->standby_gpio); + mutex_init(&mt9v032->power_lock); mt9v032->pdata = pdata; mt9v032->model = (const void *)did->driver_data; -- 2.6.1 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html