[PATCH RFC v2] media: i2c: mt9p031: add OF support

2013-05-02 Thread Prabhakar Lad
From: Lad, Prabhakar prabhakar.cse...@gmail.com

add OF support for the mt9p031 sensor driver.
Alongside this patch sorts the header inclusion alphabetically.

Signed-off-by: Lad, Prabhakar prabhakar.cse...@gmail.com
Cc: Hans Verkuil hans.verk...@cisco.com
Cc: Laurent Pinchart laurent.pinch...@ideasonboard.com
Cc: Mauro Carvalho Chehab mche...@redhat.com
Cc: Guennadi Liakhovetski g.liakhovet...@gmx.de
Cc: Sylwester Nawrocki s.nawro...@samsung.com
Cc: Sakari Ailus sakari.ai...@iki.fi
Cc: Grant Likely grant.lik...@secretlab.ca
Cc: Sascha Hauer s.ha...@pengutronix.de
Cc: Rob Herring rob.herr...@calxeda.com
Cc: Rob Landley r...@landley.net
Cc: devicetree-disc...@lists.ozlabs.org
Cc: davinci-linux-open-sou...@linux.davincidsp.com
Cc: linux-...@vger.kernel.org
Cc: linux-ker...@vger.kernel.org
---
 Changes for v2:
 1: Used '-' instead of '_' for device properties.
 2: Specified gpio reset pin as phandle in device node.
 3: Handle platform data properly even if kernel is compiled with
devicetree support.
 4: Used dev_* for messages in drivers instead of pr_*.
 5: Changed compatible property to aptina,mt9p031 and aptina,mt9p031m.
 6: Sorted the header inclusion alphabetically and fixed some minor code nits.

 .../devicetree/bindings/media/i2c/mt9p031.txt  |   37 
 drivers/media/i2c/mt9p031.c|   62 +++-
 2 files changed, 97 insertions(+), 2 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/media/i2c/mt9p031.txt

diff --git a/Documentation/devicetree/bindings/media/i2c/mt9p031.txt 
b/Documentation/devicetree/bindings/media/i2c/mt9p031.txt
new file mode 100644
index 000..cbe352b
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/i2c/mt9p031.txt
@@ -0,0 +1,37 @@
+* Aptina 1/2.5-Inch 5Mp CMOS Digital Image Sensor
+
+The Aptina MT9P031 is a 1/2.5-inch CMOS active pixel digital image sensor with
+an active array size of 2592H x 1944V. It is programmable through a simple
+two-wire serial interface.
+
+Required Properties :
+- compatible : value should be either one among the following
+   (a) aptina,mt9p031 for mt9p031 sensor
+   (b) aptina,mt9p031m for mt9p031m sensor
+
+- input-clock-frequency : Input clock frequency.
+
+- pixel-clock-frequency : Pixel clock frequency.
+
+Optional Properties :
+-gpio-reset: Chip reset GPIO
+
+Example:
+
+i2c0@1c22000 {
+   ...
+   ...
+   mt9p031@5d {
+   compatible = aptina,mt9p031;
+   reg = 0x5d;
+   gpio-reset = gpio3 30 0;
+
+   port {
+   mt9p031_1: endpoint {
+   input-clock-frequency = 600;
+   pixel-clock-frequency = 9600;
+   };
+   };
+   };
+   ...
+};
diff --git a/drivers/media/i2c/mt9p031.c b/drivers/media/i2c/mt9p031.c
index 28cf95b..8ce3561 100644
--- a/drivers/media/i2c/mt9p031.c
+++ b/drivers/media/i2c/mt9p031.c
@@ -16,9 +16,11 @@
 #include linux/delay.h
 #include linux/device.h
 #include linux/gpio.h
-#include linux/module.h
 #include linux/i2c.h
 #include linux/log2.h
+#include linux/module.h
+#include linux/of_device.h
+#include linux/of_gpio.h
 #include linux/pm.h
 #include linux/regulator/consumer.h
 #include linux/slab.h
@@ -28,6 +30,7 @@
 #include media/v4l2-chip-ident.h
 #include media/v4l2-ctrls.h
 #include media/v4l2-device.h
+#include media/v4l2-of.h
 #include media/v4l2-subdev.h
 
 #include aptina-pll.h
@@ -928,10 +931,57 @@ static const struct v4l2_subdev_internal_ops 
mt9p031_subdev_internal_ops = {
  * Driver initialization and probing
  */
 
+#if defined(CONFIG_OF)
+static struct mt9p031_platform_data *
+   mt9p031_get_pdata(struct i2c_client *client)
+
+{
+   if (client-dev.of_node) {
+   struct device_node *np;
+   struct mt9p031_platform_data *pdata;
+
+   np = v4l2_of_get_next_endpoint(client-dev.of_node, NULL);
+   if (!np)
+   return NULL;
+
+   pdata = devm_kzalloc(client-dev,
+sizeof(struct mt9p031_platform_data),
+GFP_KERNEL);
+   if (!pdata) {
+   dev_err(client-dev,
+   mt9p031 failed allocate memeory\n);
+   return NULL;
+   }
+   pdata-reset = of_get_named_gpio(client-dev.of_node,
+gpio-reset, 0);
+   if (!gpio_is_valid(pdata-reset))
+   pdata-reset = -1;
+
+   if (of_property_read_u32(np, input-clock-frequency,
+pdata-ext_freq))
+   return NULL;
+
+   if (of_property_read_u32(np, pixel-clock-frequency,
+pdata-target_freq))
+   return NULL;
+
+   return pdata;
+   }
+
+ 

Re: [PATCH RFC v2] media: i2c: mt9p031: add OF support

2013-05-02 Thread Sascha Hauer
On Thu, May 02, 2013 at 11:52:34AM +0530, Prabhakar Lad wrote:
 From: Lad, Prabhakar prabhakar.cse...@gmail.com
 
 add OF support for the mt9p031 sensor driver.
 Alongside this patch sorts the header inclusion alphabetically.
 
 Signed-off-by: Lad, Prabhakar prabhakar.cse...@gmail.com
 Cc: Hans Verkuil hans.verk...@cisco.com
 Cc: Laurent Pinchart laurent.pinch...@ideasonboard.com
 Cc: Mauro Carvalho Chehab mche...@redhat.com
 Cc: Guennadi Liakhovetski g.liakhovet...@gmx.de
 Cc: Sylwester Nawrocki s.nawro...@samsung.com
 Cc: Sakari Ailus sakari.ai...@iki.fi
 Cc: Grant Likely grant.lik...@secretlab.ca
 Cc: Sascha Hauer s.ha...@pengutronix.de
 Cc: Rob Herring rob.herr...@calxeda.com
 Cc: Rob Landley r...@landley.net
 Cc: devicetree-disc...@lists.ozlabs.org
 Cc: davinci-linux-open-sou...@linux.davincidsp.com
 Cc: linux-...@vger.kernel.org
 Cc: linux-ker...@vger.kernel.org
 ---
  Changes for v2:
  1: Used '-' instead of '_' for device properties.
  2: Specified gpio reset pin as phandle in device node.
  3: Handle platform data properly even if kernel is compiled with
 devicetree support.
  4: Used dev_* for messages in drivers instead of pr_*.
  5: Changed compatible property to aptina,mt9p031 and aptina,mt9p031m.
  6: Sorted the header inclusion alphabetically and fixed some minor code nits.
 
  .../devicetree/bindings/media/i2c/mt9p031.txt  |   37 
  drivers/media/i2c/mt9p031.c|   62 
 +++-
  2 files changed, 97 insertions(+), 2 deletions(-)
  create mode 100644 Documentation/devicetree/bindings/media/i2c/mt9p031.txt
 
 diff --git a/Documentation/devicetree/bindings/media/i2c/mt9p031.txt 
 b/Documentation/devicetree/bindings/media/i2c/mt9p031.txt
 new file mode 100644
 index 000..cbe352b
 --- /dev/null
 +++ b/Documentation/devicetree/bindings/media/i2c/mt9p031.txt
 @@ -0,0 +1,37 @@
 +* Aptina 1/2.5-Inch 5Mp CMOS Digital Image Sensor
 +
 +The Aptina MT9P031 is a 1/2.5-inch CMOS active pixel digital image sensor 
 with
 +an active array size of 2592H x 1944V. It is programmable through a simple
 +two-wire serial interface.
 +
 +Required Properties :
 +- compatible : value should be either one among the following
 + (a) aptina,mt9p031 for mt9p031 sensor
 + (b) aptina,mt9p031m for mt9p031m sensor
 +
 +- input-clock-frequency : Input clock frequency.
 +
 +- pixel-clock-frequency : Pixel clock frequency.
 +
 +Optional Properties :
 +-gpio-reset: Chip reset GPIO
 +
 +Example:
 +
 +i2c0@1c22000 {
 + ...
 + ...
 + mt9p031@5d {
 + compatible = aptina,mt9p031;
 + reg = 0x5d;
 + gpio-reset = gpio3 30 0;
 +
 + port {
 + mt9p031_1: endpoint {
 + input-clock-frequency = 600;
 + pixel-clock-frequency = 9600;
 + };
 + };
 + };
 + ...
 +};
 diff --git a/drivers/media/i2c/mt9p031.c b/drivers/media/i2c/mt9p031.c
 index 28cf95b..8ce3561 100644
 --- a/drivers/media/i2c/mt9p031.c
 +++ b/drivers/media/i2c/mt9p031.c
 @@ -16,9 +16,11 @@
  #include linux/delay.h
  #include linux/device.h
  #include linux/gpio.h
 -#include linux/module.h
  #include linux/i2c.h
  #include linux/log2.h
 +#include linux/module.h
 +#include linux/of_device.h
 +#include linux/of_gpio.h
  #include linux/pm.h
  #include linux/regulator/consumer.h
  #include linux/slab.h
 @@ -28,6 +30,7 @@
  #include media/v4l2-chip-ident.h
  #include media/v4l2-ctrls.h
  #include media/v4l2-device.h
 +#include media/v4l2-of.h
  #include media/v4l2-subdev.h
  
  #include aptina-pll.h
 @@ -928,10 +931,57 @@ static const struct v4l2_subdev_internal_ops 
 mt9p031_subdev_internal_ops = {
   * Driver initialization and probing
   */
  
 +#if defined(CONFIG_OF)
 +static struct mt9p031_platform_data *
 + mt9p031_get_pdata(struct i2c_client *client)
 +
 +{
 + if (client-dev.of_node) {

By inverting the logic here and returning immediately you can safe an
indention level for the bulk of this function.

 + struct device_node *np;
 + struct mt9p031_platform_data *pdata;
 +
 + np = v4l2_of_get_next_endpoint(client-dev.of_node, NULL);
 + if (!np)
 + return NULL;
 +
 + pdata = devm_kzalloc(client-dev,
 +  sizeof(struct mt9p031_platform_data),
 +  GFP_KERNEL);
 + if (!pdata) {
 + dev_err(client-dev,
 + mt9p031 failed allocate memeory\n);
 + return NULL;

s/memeory/memory/

Better drop this message completely. If you are really out of memory
you'll notice it quite fast anyway.

 + }
 + pdata-reset = of_get_named_gpio(client-dev.of_node,
 +  gpio-reset, 0);
 + if (!gpio_is_valid(pdata-reset))
 + pdata-reset = -1;
 +

Re: [PATCH RFC v2] media: i2c: mt9p031: add OF support

2013-05-02 Thread Prabhakar Lad
Hi Sascha,

Thanks for the quick review.

On Thu, May 2, 2013 at 12:25 PM, Sascha Hauer s.ha...@pengutronix.de wrote:
 On Thu, May 02, 2013 at 11:52:34AM +0530, Prabhakar Lad wrote:
 From: Lad, Prabhakar prabhakar.cse...@gmail.com

 add OF support for the mt9p031 sensor driver.
 Alongside this patch sorts the header inclusion alphabetically.
[Snip]

 +#if defined(CONFIG_OF)
 +static struct mt9p031_platform_data *
 + mt9p031_get_pdata(struct i2c_client *client)
 +
 +{
 + if (client-dev.of_node) {

 By inverting the logic here and returning immediately you can safe an
 indention level for the bulk of this function.

OK

 + struct device_node *np;
 + struct mt9p031_platform_data *pdata;
 +
 + np = v4l2_of_get_next_endpoint(client-dev.of_node, NULL);
 + if (!np)
 + return NULL;
 +
 + pdata = devm_kzalloc(client-dev,
 +  sizeof(struct mt9p031_platform_data),
 +  GFP_KERNEL);
 + if (!pdata) {
 + dev_err(client-dev,
 + mt9p031 failed allocate memeory\n);
 + return NULL;

 s/memeory/memory/

 Better drop this message completely. If you are really out of memory
 you'll notice it quite fast anyway.

alright I'll drop the message.

 + }
 + pdata-reset = of_get_named_gpio(client-dev.of_node,
 +  gpio-reset, 0);
 + if (!gpio_is_valid(pdata-reset))
 + pdata-reset = -1;
 +
 + if (of_property_read_u32(np, input-clock-frequency,
 +  pdata-ext_freq))
 + return NULL;
 +
 + if (of_property_read_u32(np, pixel-clock-frequency,
 +  pdata-target_freq))
 + return NULL;

 returning NULL here means that when these properties are missing the
 driver bails out with the message No platform data\n which is not
 very helpful for users. How about just ignoring this here and return
 pdata? The driver will probably print a more useful message later when
 it notices that the clock params are invalid.

Yes would be good idea of not returning NULL.

 +
 + return pdata;
 + }
 +
 + return client-dev.platform_data;
 +}
 +#else
 +static struct mt9p031_platform_data *
 + mt9p031_get_pdata(struct i2c_client *client)
 +{
 + return client-dev.platform_data;
 +}
 +#endif
 +
  static int mt9p031_probe(struct i2c_client *client,
const struct i2c_device_id *did)
  {
 - struct mt9p031_platform_data *pdata = client-dev.platform_data;
 + struct mt9p031_platform_data *pdata = mt9p031_get_pdata(client);
   struct i2c_adapter *adapter = to_i2c_adapter(client-dev.parent);
   struct mt9p031 *mt9p031;
   unsigned int i;
 @@ -1070,8 +1120,16 @@ static const struct i2c_device_id mt9p031_id[] = {
  };
  MODULE_DEVICE_TABLE(i2c, mt9p031_id);

 +static const struct of_device_id mt9p031_of_match[] = {
 + { .compatible = aptina,mt9p031, },
 + { .compatible = aptina,mt9p031m, },
 + {},
 +};

 I would have expected something like:

 static const struct of_device_id mt9p031_of_match[] = {
 {
 .compatible = aptina,mt9p031-sensor,
 .data = (void *)MT9P031_MODEL_COLOR,
 }, {
 .compatible = aptina,mt9p031m-sensor,
 .data = (void *)MT9P031_MODEL_MONOCHROME,
 }, {
 /* sentinel */
 },
 };

 of_id = of_match_device(mt9p031_of_match, client-dev);
 if (of_id)
 mt9p031-model = (enum mt9p031_model)of_id-data;

 To handle monochrome sensors.

OK will do the same.

Regards,
--Prabhakar Lad
--
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 RFC v2] media: i2c: mt9p031: add OF support

2013-05-02 Thread Laurent Pinchart
Hi Prabhakar,

Thank you for the patch.

On Thursday 02 May 2013 12:34:25 Prabhakar Lad wrote:
 On Thu, May 2, 2013 at 12:25 PM, Sascha Hauer wrote:
  On Thu, May 02, 2013 at 11:52:34AM +0530, Prabhakar Lad wrote:
  From: Lad, Prabhakar prabhakar.cse...@gmail.com
  
  add OF support for the mt9p031 sensor driver.
  Alongside this patch sorts the header inclusion alphabetically.
 
 [Snip]
 
  +#if defined(CONFIG_OF)
  +static struct mt9p031_platform_data *
  + mt9p031_get_pdata(struct i2c_client *client)

Please align this line on the left.

  +{
  + if (client-dev.of_node) {
  
  By inverting the logic here and returning immediately you can safe an
  indention level for the bulk of this function.
 
 OK
 
  + struct device_node *np;
  + struct mt9p031_platform_data *pdata;
  +
  + np = v4l2_of_get_next_endpoint(client-dev.of_node, NULL);
  + if (!np)
  + return NULL;
  +
  + pdata = devm_kzalloc(client-dev,
  +  sizeof(struct mt9p031_platform_data),
  +  GFP_KERNEL);
  + if (!pdata) {
  + dev_err(client-dev,
  + mt9p031 failed allocate memeory\n);
  + return NULL;
  
  s/memeory/memory/
  
  Better drop this message completely. If you are really out of memory
  you'll notice it quite fast anyway.
 
 alright I'll drop the message.
 
  + }
  + pdata-reset = of_get_named_gpio(client-dev.of_node,
  +  gpio-reset, 0);
  + if (!gpio_is_valid(pdata-reset))
  + pdata-reset = -1;

I've just sent a patch that converts the manual -1 checks in the driver to 
gpio_is_valid(). You can thus drop these two lines.

  + if (of_property_read_u32(np, input-clock-frequency,
  +  pdata-ext_freq))
  + return NULL;
  +
  + if (of_property_read_u32(np, pixel-clock-frequency,
  +  pdata-target_freq))
  + return NULL;
  
  returning NULL here means that when these properties are missing the
  driver bails out with the message No platform data\n which is not
  very helpful for users. How about just ignoring this here and return
  pdata? The driver will probably print a more useful message later when
  it notices that the clock params are invalid.
 
 Yes would be good idea of not returning NULL.
 
  +
  + return pdata;
  + }
  +
  + return client-dev.platform_data;
  +}
  +#else
  +static struct mt9p031_platform_data *
  + mt9p031_get_pdata(struct i2c_client *client)
  +{
  + return client-dev.platform_data;
  +}
  +#endif
  +
  
   static int mt9p031_probe(struct i2c_client *client,
 const struct i2c_device_id *did)
   {
  - struct mt9p031_platform_data *pdata = client-dev.platform_data;
  + struct mt9p031_platform_data *pdata = mt9p031_get_pdata(client);
  
struct i2c_adapter *adapter = to_i2c_adapter(client-dev.parent);
struct mt9p031 *mt9p031;
unsigned int i;
  @@ -1070,8 +1120,16 @@ static const struct i2c_device_id mt9p031_id[] = {
  
   };
   MODULE_DEVICE_TABLE(i2c, mt9p031_id);
  
  +static const struct of_device_id mt9p031_of_match[] = {
  + { .compatible = aptina,mt9p031, },
  + { .compatible = aptina,mt9p031m, },
  + {},
  +};
  
  I would have expected something like:
  
  static const struct of_device_id mt9p031_of_match[] = {
  {
  .compatible = aptina,mt9p031-sensor,
  .data = (void *)MT9P031_MODEL_COLOR,
  }, {
  .compatible = aptina,mt9p031m-sensor,
  .data = (void *)MT9P031_MODEL_MONOCHROME,
  }, {
  /* sentinel */
  },
  };
  
  of_id = of_match_device(mt9p031_of_match, client-dev);
  if (of_id)
  mt9p031-model = (enum mt9p031_model)of_id-data;
  
  To handle monochrome sensors.
 
 OK will do the same.

And please guard the table with #ifdef CONFIG_OF.

-- 
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 RFC v2] media: i2c: mt9p031: add OF support

2013-05-02 Thread Prabhakar Lad
Hi Laurent,

Thanks for the review.

On Thu, May 2, 2013 at 4:32 PM, Laurent Pinchart
laurent.pinch...@ideasonboard.com wrote:
 Hi Prabhakar,

 Thank you for the patch.

 On Thursday 02 May 2013 12:34:25 Prabhakar Lad wrote:
 On Thu, May 2, 2013 at 12:25 PM, Sascha Hauer wrote:
  On Thu, May 02, 2013 at 11:52:34AM +0530, Prabhakar Lad wrote:
  From: Lad, Prabhakar prabhakar.cse...@gmail.com
 
  add OF support for the mt9p031 sensor driver.
  Alongside this patch sorts the header inclusion alphabetically.

 [Snip]

  +#if defined(CONFIG_OF)
  +static struct mt9p031_platform_data *
  + mt9p031_get_pdata(struct i2c_client *client)

 Please align this line on the left.

OK

  +{
  + if (client-dev.of_node) {
 
  By inverting the logic here and returning immediately you can safe an
  indention level for the bulk of this function.

 OK

  + struct device_node *np;
  + struct mt9p031_platform_data *pdata;
  +
  + np = v4l2_of_get_next_endpoint(client-dev.of_node, NULL);
  + if (!np)
  + return NULL;
  +
  + pdata = devm_kzalloc(client-dev,
  +  sizeof(struct mt9p031_platform_data),
  +  GFP_KERNEL);
  + if (!pdata) {
  + dev_err(client-dev,
  + mt9p031 failed allocate memeory\n);
  + return NULL;
 
  s/memeory/memory/
 
  Better drop this message completely. If you are really out of memory
  you'll notice it quite fast anyway.

 alright I'll drop the message.

  + }
  + pdata-reset = of_get_named_gpio(client-dev.of_node,
  +  gpio-reset, 0);
  + if (!gpio_is_valid(pdata-reset))
  + pdata-reset = -1;

 I've just sent a patch that converts the manual -1 checks in the driver to
 gpio_is_valid(). You can thus drop these two lines.

OK I'll drop this check.

  + if (of_property_read_u32(np, input-clock-frequency,
  +  pdata-ext_freq))
  + return NULL;
  +
  + if (of_property_read_u32(np, pixel-clock-frequency,
  +  pdata-target_freq))
  + return NULL;
 
  returning NULL here means that when these properties are missing the
  driver bails out with the message No platform data\n which is not
  very helpful for users. How about just ignoring this here and return
  pdata? The driver will probably print a more useful message later when
  it notices that the clock params are invalid.

 Yes would be good idea of not returning NULL.

  +
  + return pdata;
  + }
  +
  + return client-dev.platform_data;
  +}
  +#else
  +static struct mt9p031_platform_data *
  + mt9p031_get_pdata(struct i2c_client *client)
  +{
  + return client-dev.platform_data;
  +}
  +#endif
  +
 
   static int mt9p031_probe(struct i2c_client *client,
 const struct i2c_device_id *did)
   {
  - struct mt9p031_platform_data *pdata = client-dev.platform_data;
  + struct mt9p031_platform_data *pdata = mt9p031_get_pdata(client);
 
struct i2c_adapter *adapter = to_i2c_adapter(client-dev.parent);
struct mt9p031 *mt9p031;
unsigned int i;
  @@ -1070,8 +1120,16 @@ static const struct i2c_device_id mt9p031_id[] = {
 
   };
   MODULE_DEVICE_TABLE(i2c, mt9p031_id);
 
  +static const struct of_device_id mt9p031_of_match[] = {
  + { .compatible = aptina,mt9p031, },
  + { .compatible = aptina,mt9p031m, },
  + {},
  +};
 
  I would have expected something like:
 
  static const struct of_device_id mt9p031_of_match[] = {
  {
  .compatible = aptina,mt9p031-sensor,
  .data = (void *)MT9P031_MODEL_COLOR,
  }, {
  .compatible = aptina,mt9p031m-sensor,
  .data = (void *)MT9P031_MODEL_MONOCHROME,
  }, {
  /* sentinel */
  },
  };
 
  of_id = of_match_device(mt9p031_of_match, client-dev);
  if (of_id)
  mt9p031-model = (enum mt9p031_model)of_id-data;
 
  To handle monochrome sensors.

 OK will do the same.

 And please guard the table with #ifdef CONFIG_OF.

But guarding the table #ifdef CONFIG_OF would cause compilation failure
for below code when CONFIG_OF is undefined in probe

of_id = of_match_device(of_match_ptr(mt9p031_of_match),
client-dev);
if (of_id)
mt9p031-model = (enum mt9p031_model)of_id-data;

and also in mt9p031_i2c_driver structure,
of_match_table = of_match_ptr(mt9p031_of_match),

which force me to define mt9p031_of_match to NULL when
CONFIG_OF is undefined

Regards,
--Prabhakar Lad
--
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  

Re: [PATCH RFC v2] media: i2c: mt9p031: add OF support

2013-05-02 Thread Laurent Pinchart
Hi Prabhakar,

On Thursday 02 May 2013 18:48:37 Prabhakar Lad wrote:
 On Thu, May 2, 2013 at 4:32 PM, Laurent Pinchart wrote:
  On Thursday 02 May 2013 12:34:25 Prabhakar Lad wrote:
  On Thu, May 2, 2013 at 12:25 PM, Sascha Hauer wrote:
   On Thu, May 02, 2013 at 11:52:34AM +0530, Prabhakar Lad wrote:
   From: Lad, Prabhakar prabhakar.cse...@gmail.com
   
   add OF support for the mt9p031 sensor driver.
   Alongside this patch sorts the header inclusion alphabetically.

[snip]

   @@ -1070,8 +1120,16 @@ static const struct i2c_device_id mt9p031_id[]
   = {
};
MODULE_DEVICE_TABLE(i2c, mt9p031_id);
   
   +static const struct of_device_id mt9p031_of_match[] = {
   + { .compatible = aptina,mt9p031, },
   + { .compatible = aptina,mt9p031m, },
   + {},
   +};
   
   I would have expected something like:
   
   static const struct of_device_id mt9p031_of_match[] = {
   {
   .compatible = aptina,mt9p031-sensor,
   .data = (void *)MT9P031_MODEL_COLOR,
   }, {
   .compatible = aptina,mt9p031m-sensor,
   .data = (void *)MT9P031_MODEL_MONOCHROME,
   }, {
   /* sentinel */
   },
   };
   
   of_id = of_match_device(mt9p031_of_match, client-dev);
   if (of_id)
   mt9p031-model = (enum mt9p031_model)of_id-data;
   
   To handle monochrome sensors.
  
  OK will do the same.
  
  And please guard the table with #ifdef CONFIG_OF.
 
 But guarding the table #ifdef CONFIG_OF would cause compilation failure
 for below code when CONFIG_OF is undefined in probe
 
 of_id = of_match_device(of_match_ptr(mt9p031_of_match),
   client-dev);
 if (of_id)
   mt9p031-model = (enum mt9p031_model)of_id-data;

You could guard the above code with an #ifdef CONFIG_OF as well.

 and also in mt9p031_i2c_driver structure,
 of_match_table = of_match_ptr(mt9p031_of_match),
 
 which force me to define mt9p031_of_match to NULL when
 CONFIG_OF is undefined

of_match_ptr is defined as NULL when CONFIG_OF is disabled.

-- 
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 RFC v2] media: i2c: mt9p031: add OF support

2013-05-02 Thread Arnd Bergmann
On Thursday 02 May 2013 08:55:18 Sascha Hauer wrote:
  +#if defined(CONFIG_OF)
  +static struct mt9p031_platform_data *
  + mt9p031_get_pdata(struct i2c_client *client)
  +
  +{
  + if (client-dev.of_node) {
 
 By inverting the logic here and returning immediately you can safe an
 indention level for the bulk of this function.

Right, also make this

if (!IS_ENABLED(CONFIG_OF) || !client-dev.of_node)
return client-dev.platform_data;

Then the rest of the function gets discarded by the compiler when CONFIG_OF
is not set, and you can kill the #ifdef around the function.

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