From: Lad, Prabhakar <[email protected]>
add OF support for the mt9p031 sensor driver.
Alongside this patch sorts the header inclusion alphabetically.
Signed-off-by: Lad, Prabhakar <[email protected]>
Cc: Hans Verkuil <[email protected]>
Cc: Laurent Pinchart <[email protected]>
Cc: Mauro Carvalho Chehab <[email protected]>
Cc: Guennadi Liakhovetski <[email protected]>
Cc: Sylwester Nawrocki <[email protected]>
Cc: Sakari Ailus <[email protected]>
Cc: Grant Likely <[email protected]>
Cc: Sascha Hauer <[email protected]>
Cc: Rob Herring <[email protected]>
Cc: Rob Landley <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
---
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 0000000..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 = <6000000>;
+ pixel-clock-frequency = <96000000>;
+ };
+ };
+ };
+ ...
+};
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;
+ }
+
+ 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", },
+ {},
+};
+MODULE_DEVICE_TABLE(of, mt9p031_of_match);
+
static struct i2c_driver mt9p031_i2c_driver = {
.driver = {
+ .of_match_table = of_match_ptr(mt9p031_of_match),
.name = "mt9p031",
},
.probe = mt9p031_probe,
--
1.7.4.1
On Thu, May 02, 2013 at 11:52:34AM +0530, Prabhakar Lad wrote:
> From: Lad, Prabhakar <[email protected]>
>
> add OF support for the mt9p031 sensor driver.
> Alongside this patch sorts the header inclusion alphabetically.
>
> Signed-off-by: Lad, Prabhakar <[email protected]>
> Cc: Hans Verkuil <[email protected]>
> Cc: Laurent Pinchart <[email protected]>
> Cc: Mauro Carvalho Chehab <[email protected]>
> Cc: Guennadi Liakhovetski <[email protected]>
> Cc: Sylwester Nawrocki <[email protected]>
> Cc: Sakari Ailus <[email protected]>
> Cc: Grant Likely <[email protected]>
> Cc: Sascha Hauer <[email protected]>
> Cc: Rob Herring <[email protected]>
> Cc: Rob Landley <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> ---
> 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 0000000..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 = <6000000>;
> + pixel-clock-frequency = <96000000>;
> + };
> + };
> + };
> + ...
> +};
> 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;
> +
> + 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.
> +
> + 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.
Sascha
--
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-5555 |
Hi Sascha,
Thanks for the quick review.
On Thu, May 2, 2013 at 12:25 PM, Sascha Hauer <[email protected]> wrote:
> On Thu, May 02, 2013 at 11:52:34AM +0530, Prabhakar Lad wrote:
>> From: Lad, Prabhakar <[email protected]>
>>
>> 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
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 <[email protected]>
> >>
> >> 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
Hi Laurent,
Thanks for the review.
On Thu, May 2, 2013 at 4:32 PM, Laurent Pinchart
<[email protected]> 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 <[email protected]>
>> >>
>> >> 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
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 <[email protected]>
> >> >>
> >> >> 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
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