Re: [PATCH 1/3] [media] mt9v032: Add reset and standby gpios

[Laurent Pinchart]

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(>dev, "reset",
> +   GPIOD_OUT_HIGH);
> + if (IS_ERR(mt9v032->reset_gpio))
> + return PTR_ERR(mt9v032->reset_gpio);
> +
> + mt9v032->standby_gpio = devm_gpiod_get_optional(>dev, "standby",
> + GPIOD_OUT_LOW);
> + if (IS_ERR(mt9v032->standby_gpio))
> + return PTR_ERR(mt9v032->standby_gpio);
> +
>   mutex_init(>power_lock);
>   mt9v032->pdata = pdata;
>   mt9v032->model = (const void *)did->driver_data;

-- 
Regards,

Laurent Pinchart

--
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

Re: [PATCH 1/3] [media] mt9v032: Add reset and standby gpios

[Markus Pargmann]

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

[Laurent Pinchart]

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 devicetree" 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

[Markus Pargmann]

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(>dev, "reset",
+ GPIOD_OUT_HIGH);
+   if (IS_ERR(mt9v032->reset_gpio))
+   return PTR_ERR(mt9v032->reset_gpio);
+
+   mt9v032->standby_gpio = devm_gpiod_get_optional(>dev, "standby",
+   GPIOD_OUT_LOW);
+   if (IS_ERR(mt9v032->standby_gpio))
+   return PTR_ERR(mt9v032->standby_gpio);
+
mutex_init(>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

Re: [PATCH 1/3] [media] mt9v032: Add reset and standby gpios

[Rob Herring]

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(>dev, "reset",
> +   GPIOD_OUT_HIGH);
> + if (IS_ERR(mt9v032->reset_gpio))
> + return PTR_ERR(mt9v032->reset_gpio);
> +
> + mt9v032->standby_gpio = devm_gpiod_get_optional(>dev, "standby",
> + GPIOD_OUT_LOW);
> + if (IS_ERR(mt9v032->standby_gpio))
> + return PTR_ERR(mt9v032->standby_gpio);
> +
>   mutex_init(>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 devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html