| Message ID | 1425822349-19218-1-git-send-email-laurent.pinchart@ideasonboard.com |
|---|---|
| State | Superseded, archived |
| Headers | show |
Hi Laurent, Looks good. One question, why not deprecate the platform data? I can't see any device using the mt9v032 pdata, I was making a similar patch but deprecating pdata. Some more comment: 2015-03-08 14:45 GMT+01:00 Laurent Pinchart <laurent.pinchart@ideasonboard.com>: > Parse DT properties into a platform data structure when a DT node is > available. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > .../devicetree/bindings/media/i2c/mt9v032.txt | 41 ++++++++++++++ > MAINTAINERS | 1 + > drivers/media/i2c/mt9v032.c | 66 +++++++++++++++++++++- > 3 files changed, 107 insertions(+), 1 deletion(-) > create mode 100644 Documentation/devicetree/bindings/media/i2c/mt9v032.txt > > diff --git a/Documentation/devicetree/bindings/media/i2c/mt9v032.txt b/Documentation/devicetree/bindings/media/i2c/mt9v032.txt > new file mode 100644 > index 0000000..75c97de > --- /dev/null > +++ b/Documentation/devicetree/bindings/media/i2c/mt9v032.txt > @@ -0,0 +1,41 @@ > +* Aptina 1/3-Inch WVGA CMOS Digital Image Sensor > + > +The Aptina MT9V032 is a 1/3-inch CMOS active pixel digital image sensor with > +an active array size of 752H x 480V. It is programmable through a simple > +two-wire serial interface. > + > +Required Properties: > + > +- compatible: value should be either one among the following > + (a) "aptina,mt9v032" for MT9V032 color sensor > + (b) "aptina,mt9v032m" for MT9V032 monochrome sensor > + (c) "aptina,mt9v034" for MT9V034 color sensor > + (d) "aptina,mt9v034m" for MT9V034 monochrome sensor > + > +Optional Properties: > + > +- link-frequencies: List of allowed link frequencies in Hz. Each frequency is > + expressed as a 64-bit big-endian integer. > + > +For further reading on port node refer to > +Documentation/devicetree/bindings/media/video-interfaces.txt. > + > +Example: > + > + i2c0@1c22000 { > + ... > + ... > + mt9v032@5c { > + compatible = "aptina,mt9v032"; > + reg = <0x5c>; > + > + port { > + mt9v032_1: endpoint { > + link-frequencies = > + <0 13000000>, <0 26600000>, > + <0 27000000>; > + }; > + }; > + }; > + ... > + }; > diff --git a/MAINTAINERS b/MAINTAINERS > index ddc5a8c..180f6fb 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -6535,6 +6535,7 @@ M: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > L: linux-media@vger.kernel.org > T: git git://linuxtv.org/media_tree.git > S: Maintained > +F: Documentation/devicetree/bindings/media/i2c/mt9v032.txt > F: drivers/media/i2c/mt9v032.c > F: include/media/mt9v032.h > > diff --git a/drivers/media/i2c/mt9v032.c b/drivers/media/i2c/mt9v032.c > index 3267c18..a6ea091 100644 > --- a/drivers/media/i2c/mt9v032.c > +++ b/drivers/media/i2c/mt9v032.c > @@ -17,6 +17,8 @@ > #include <linux/i2c.h> > #include <linux/log2.h> > #include <linux/mutex.h> > +#include <linux/of.h> > +#include <linux/of_gpio.h> > #include <linux/regmap.h> > #include <linux/slab.h> > #include <linux/videodev2.h> > @@ -26,6 +28,7 @@ > #include <media/mt9v032.h> > #include <media/v4l2-ctrls.h> > #include <media/v4l2-device.h> > +#include <media/v4l2-of.h> > #include <media/v4l2-subdev.h> > > /* The first four rows are black rows. The active area spans 753x481 pixels. */ > @@ -876,10 +879,59 @@ static const struct regmap_config mt9v032_regmap_config = { > * Driver initialization and probing > */ > > +static struct mt9v032_platform_data * > +mt9v032_get_pdata(struct i2c_client *client) > +{ > + struct mt9v032_platform_data *pdata; > + struct v4l2_of_endpoint endpoint; > + struct device_node *np; > + struct property *prop; > + > + if (!IS_ENABLED(CONFIG_OF) || !client->dev.of_node) > + return client->dev.platform_data; > + > + np = v4l2_of_get_next_endpoint(client->dev.of_node, NULL); > + if (!np) > + return NULL; > + > + if (v4l2_of_parse_endpoint(np, &endpoint) < 0) > + goto done; Here I have one little testing: if (endpoint.bus_type != V4L2_MBUS_PARALLEL) { dev_err(dev, "invalid bus type, must be parallel\n"); goto done; } > + > + pdata = devm_kzalloc(&client->dev, sizeof(*pdata), GFP_KERNEL); > + if (!pdata) > + goto done; > + > + prop = of_find_property(np, "link-freqs", NULL); > + if (prop) { > + size_t size = prop->length / 8; > + u64 *link_freqs; > + > + link_freqs = devm_kzalloc(&client->dev, > + size * sizeof(*link_freqs), > + GFP_KERNEL); > + if (!link_freqs) > + goto done; > + > + if (of_property_read_u64_array(np, "link-frequencies", > + link_freqs, size) < 0) > + goto done; > + > + pdata->link_freqs = link_freqs; > + pdata->link_def_freq = link_freqs[0]; > + } > + > + pdata->clk_pol = !!(endpoint.bus.parallel.flags & > + V4L2_MBUS_PCLK_SAMPLE_RISING); > + > +done: > + of_node_put(np); > + return pdata; > +} > + > static int mt9v032_probe(struct i2c_client *client, > const struct i2c_device_id *did) > { > - struct mt9v032_platform_data *pdata = client->dev.platform_data; > + struct mt9v032_platform_data *pdata = mt9v032_get_pdata(client); > struct mt9v032 *mt9v032; > unsigned int i; > int ret; > @@ -1034,9 +1086,21 @@ static const struct i2c_device_id mt9v032_id[] = { > }; > MODULE_DEVICE_TABLE(i2c, mt9v032_id); > > +#if IS_ENABLED(CONFIG_OF) > +static const struct of_device_id mt9v032_of_match[] = { > + { .compatible = "mt9v032" }, > + { .compatible = "mt9v032m" }, > + { .compatible = "mt9v034" }, > + { .compatible = "mt9v034m" }, > + { /* Sentinel */ } > +}; > +MODULE_DEVICE_TABLE(of, mt9v032_of_match); > +#endif I have this: #if IS_ENABLED(CONFIG_OF) static const struct of_device_id mt9v032_of_match[] = { { .compatible = "aptina,mt9v022", }, { .compatible = "aptina,mt9v022m", }, { .compatible = "aptina,mt9v024", }, { .compatible = "aptina,mt9v024m", }, { .compatible = "aptina,mt9v032", }, { .compatible = "aptina,mt9v032m", }, { .compatible = "aptina,mt9v034", }, { .compatible = "aptina,mt9v034m", }, { /* sentinel */ }, }; MODULE_DEVICE_TABLE(of, mt9v032_of_match); #endi > + > static struct i2c_driver mt9v032_driver = { > .driver = { > .name = "mt9v032", > + .of_match_table = of_match_ptr(mt9v032_of_match), > }, > .probe = mt9v032_probe, > .remove = mt9v032_remove, > -- > Regards, > > Laurent Pinchart > > -- > To unsubscribe from this list: send the line "unsubscribe linux-media" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html Regards, Carlos Sanmartín -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Laurent, On 08/03/15 14:45, Laurent Pinchart wrote: > +++ b/Documentation/devicetree/bindings/media/i2c/mt9v032.txt > @@ -0,0 +1,41 @@ > +* Aptina 1/3-Inch WVGA CMOS Digital Image Sensor > + > +The Aptina MT9V032 is a 1/3-inch CMOS active pixel digital image sensor with > +an active array size of 752H x 480V. It is programmable through a simple > +two-wire serial interface. > + > +Required Properties: > + > +- compatible: value should be either one among the following > + (a) "aptina,mt9v032" for MT9V032 color sensor > + (b) "aptina,mt9v032m" for MT9V032 monochrome sensor > + (c) "aptina,mt9v034" for MT9V034 color sensor > + (d) "aptina,mt9v034m" for MT9V034 monochrome sensor It can't be determined at runtime whether the sensor is just monochromatic ? Al in all the color filter array is a physical property of the sensor, still the driver seems to be ignoring the "m" suffix. Hence I suspect the register interfaces for both color and monochromatic versions are compatible. I'm wondering whether using a boolean property to indicate the color filter array type would do as well. > +static struct mt9v032_platform_data * > +mt9v032_get_pdata(struct i2c_client *client) > +{ > + struct mt9v032_platform_data *pdata; > + struct v4l2_of_endpoint endpoint; > + struct device_node *np; > + struct property *prop; > + > + if (!IS_ENABLED(CONFIG_OF) || !client->dev.of_node) > + return client->dev.platform_data; > + > + np = v4l2_of_get_next_endpoint(client->dev.of_node, NULL); > + if (!np) > + return NULL; > + > + if (v4l2_of_parse_endpoint(np, &endpoint) < 0) > + goto done; > + > + pdata = devm_kzalloc(&client->dev, sizeof(*pdata), GFP_KERNEL); > + if (!pdata) > + goto done; > + > + prop = of_find_property(np, "link-freqs", NULL); I suspect you meant "link-frequencies" here ? > + if (prop) { > + size_t size = prop->length / 8; > + u64 *link_freqs; > + > + link_freqs = devm_kzalloc(&client->dev, > + size * sizeof(*link_freqs), > + GFP_KERNEL); > + if (!link_freqs) > + goto done; > + > + if (of_property_read_u64_array(np, "link-frequencies", > + link_freqs, size) < 0) > + goto done; > + > + pdata->link_freqs = link_freqs; > + pdata->link_def_freq = link_freqs[0]; > + } > + > + pdata->clk_pol = !!(endpoint.bus.parallel.flags & > + V4L2_MBUS_PCLK_SAMPLE_RISING); > + > +done: > + of_node_put(np); > + return pdata; > +} > @@ -1034,9 +1086,21 @@ static const struct i2c_device_id mt9v032_id[] = { > }; > MODULE_DEVICE_TABLE(i2c, mt9v032_id); > > +#if IS_ENABLED(CONFIG_OF) > +static const struct of_device_id mt9v032_of_match[] = { > + { .compatible = "mt9v032" }, > + { .compatible = "mt9v032m" }, > + { .compatible = "mt9v034" }, > + { .compatible = "mt9v034m" }, > + { /* Sentinel */ } > +}; > +MODULE_DEVICE_TABLE(of, mt9v032_of_match); > +#endif
Hi Sylwester, On Mon, Mar 09, 2015 at 11:35:52AM +0100, Sylwester Nawrocki wrote: > Hi Laurent, > > On 08/03/15 14:45, Laurent Pinchart wrote: > > +++ b/Documentation/devicetree/bindings/media/i2c/mt9v032.txt > > @@ -0,0 +1,41 @@ > > +* Aptina 1/3-Inch WVGA CMOS Digital Image Sensor > > + > > +The Aptina MT9V032 is a 1/3-inch CMOS active pixel digital image sensor with > > +an active array size of 752H x 480V. It is programmable through a simple > > +two-wire serial interface. > > + > > +Required Properties: > > + > > +- compatible: value should be either one among the following > > + (a) "aptina,mt9v032" for MT9V032 color sensor > > + (b) "aptina,mt9v032m" for MT9V032 monochrome sensor > > + (c) "aptina,mt9v034" for MT9V034 color sensor > > + (d) "aptina,mt9v034m" for MT9V034 monochrome sensor > > It can't be determined at runtime whether the sensor is just monochromatic ? > Al in all the color filter array is a physical property of the sensor, still > the driver seems to be ignoring the "m" suffix. Hence I suspect the register > interfaces for both color and monochromatic versions are compatible. > I'm wondering whether using a boolean property to indicate the color filter > array type would do as well. > > > +static struct mt9v032_platform_data * > > +mt9v032_get_pdata(struct i2c_client *client) > > +{ > > + struct mt9v032_platform_data *pdata; > > + struct v4l2_of_endpoint endpoint; > > + struct device_node *np; > > + struct property *prop; > > + > > + if (!IS_ENABLED(CONFIG_OF) || !client->dev.of_node) > > + return client->dev.platform_data; > > + > > + np = v4l2_of_get_next_endpoint(client->dev.of_node, NULL); > > + if (!np) > > + return NULL; > > + > > + if (v4l2_of_parse_endpoint(np, &endpoint) < 0) > > + goto done; > > + > > + pdata = devm_kzalloc(&client->dev, sizeof(*pdata), GFP_KERNEL); > > + if (!pdata) > > + goto done; > > + > > + prop = of_find_property(np, "link-freqs", NULL); > > I suspect you meant "link-frequencies" here ? Yes. I wonder if it'd make sense to add this to struct v4l2_of_endpoint. I can write a patch for that. > > + if (prop) { > > + size_t size = prop->length / 8; > > + u64 *link_freqs; > > + > > + link_freqs = devm_kzalloc(&client->dev, > > + size * sizeof(*link_freqs), > > + GFP_KERNEL); > > + if (!link_freqs) > > + goto done; > > + > > + if (of_property_read_u64_array(np, "link-frequencies", > > + link_freqs, size) < 0) > > + goto done; > > + > > + pdata->link_freqs = link_freqs; > > + pdata->link_def_freq = link_freqs[0]; > > + } If you're interested in just a single value, you can use of_property_read_u64(). > > + pdata->clk_pol = !!(endpoint.bus.parallel.flags & > > + V4L2_MBUS_PCLK_SAMPLE_RISING); > > + > > +done: > > + of_node_put(np); > > + return pdata; > > +} > > > @@ -1034,9 +1086,21 @@ static const struct i2c_device_id mt9v032_id[] = { > > }; > > MODULE_DEVICE_TABLE(i2c, mt9v032_id); > > > > +#if IS_ENABLED(CONFIG_OF) > > +static const struct of_device_id mt9v032_of_match[] = { > > + { .compatible = "mt9v032" }, > > + { .compatible = "mt9v032m" }, > > + { .compatible = "mt9v034" }, > > + { .compatible = "mt9v034m" }, > > + { /* Sentinel */ } > > +}; > > +MODULE_DEVICE_TABLE(of, mt9v032_of_match); > > +#endif
Hi Carlos, Thank you for the review. On Monday 09 March 2015 10:45:31 Carlos Sanmartín Bustos wrote: > Hi Laurent, > > Looks good. One question, why not deprecate the platform data? I can't > see any device using the mt9v032 pdata, I was making a similar patch > but deprecating pdata. The sensor could be used on non-DT platforms. As keeping platform data support doesn't really make the code more complex and doesn't prevent cleanups or other refactoring, I don't see much harm in keeping it for now. For DT-based platforms, of course, DT should be used. > Some more comment: > > 2015-03-08 14:45 GMT+01:00 Laurent Pinchart: > > Parse DT properties into a platform data structure when a DT node is > > available. > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > --- > > > > .../devicetree/bindings/media/i2c/mt9v032.txt | 41 ++++++++++++++ > > MAINTAINERS | 1 + > > drivers/media/i2c/mt9v032.c | 66 ++++++++++++++++- > > 3 files changed, 107 insertions(+), 1 deletion(-) > > create mode 100644 > > Documentation/devicetree/bindings/media/i2c/mt9v032.txt [snip] > > diff --git a/drivers/media/i2c/mt9v032.c b/drivers/media/i2c/mt9v032.c > > index 3267c18..a6ea091 100644 > > --- a/drivers/media/i2c/mt9v032.c > > +++ b/drivers/media/i2c/mt9v032.c [snip] > > +static struct mt9v032_platform_data * > > +mt9v032_get_pdata(struct i2c_client *client) > > +{ > > + struct mt9v032_platform_data *pdata; > > + struct v4l2_of_endpoint endpoint; > > + struct device_node *np; > > + struct property *prop; > > + > > + if (!IS_ENABLED(CONFIG_OF) || !client->dev.of_node) > > + return client->dev.platform_data; > > + > > + np = v4l2_of_get_next_endpoint(client->dev.of_node, NULL); > > + if (!np) > > + return NULL; > > + > > + if (v4l2_of_parse_endpoint(np, &endpoint) < 0) > > + goto done; > > Here I have one little testing: > > if (endpoint.bus_type != V4L2_MBUS_PARALLEL) { > dev_err(dev, "invalid bus type, must be parallel\n"); > goto done; > } Good question, should drivers check DT properties that they don't use, in order to catch errors in DT ? This would slightly increase the kernel size in order to prevent people from connecting a parallel sensor to a CSI-2 bus, which is obviously impossible in hardware :-) > > + > > + pdata = devm_kzalloc(&client->dev, sizeof(*pdata), GFP_KERNEL); > > + if (!pdata) > > + goto done; > > + > > + prop = of_find_property(np, "link-freqs", NULL); > > + if (prop) { > > + size_t size = prop->length / 8; > > + u64 *link_freqs; > > + > > + link_freqs = devm_kzalloc(&client->dev, > > + size * sizeof(*link_freqs), > > + GFP_KERNEL); > > + if (!link_freqs) > > + goto done; > > + > > + if (of_property_read_u64_array(np, "link-frequencies", > > + link_freqs, size) < 0) > > + goto done; > > + > > + pdata->link_freqs = link_freqs; > > + pdata->link_def_freq = link_freqs[0]; > > + } > > + > > + pdata->clk_pol = !!(endpoint.bus.parallel.flags & > > + V4L2_MBUS_PCLK_SAMPLE_RISING); > > + > > +done: > > + of_node_put(np); > > + return pdata; > > +} [snip] > > @@ -1034,9 +1086,21 @@ static const struct i2c_device_id mt9v032_id[] = { > > }; > > MODULE_DEVICE_TABLE(i2c, mt9v032_id); > > > > +#if IS_ENABLED(CONFIG_OF) > > +static const struct of_device_id mt9v032_of_match[] = { > > + { .compatible = "mt9v032" }, > > + { .compatible = "mt9v032m" }, > > + { .compatible = "mt9v034" }, > > + { .compatible = "mt9v034m" }, > > + { /* Sentinel */ } > > +}; > > +MODULE_DEVICE_TABLE(of, mt9v032_of_match); > > +#endif > > I have this: > #if IS_ENABLED(CONFIG_OF) > static const struct of_device_id mt9v032_of_match[] = { > { .compatible = "aptina,mt9v022", }, > { .compatible = "aptina,mt9v022m", }, > { .compatible = "aptina,mt9v024", }, > { .compatible = "aptina,mt9v024m", }, > { .compatible = "aptina,mt9v032", }, > { .compatible = "aptina,mt9v032m", }, > { .compatible = "aptina,mt9v034", }, > { .compatible = "aptina,mt9v034m", }, Looks like I forgot to update my patch for mt9v02* support. Sorry about that, I'll fix it. And the "aptina," prefix of course belongs there. > { /* sentinel */ }, > }; > > MODULE_DEVICE_TABLE(of, mt9v032_of_match); > #endi
Hi Sylwester, Thank you for the review. On Monday 09 March 2015 11:35:52 Sylwester Nawrocki wrote: > On 08/03/15 14:45, Laurent Pinchart wrote: > > +++ b/Documentation/devicetree/bindings/media/i2c/mt9v032.txt > > @@ -0,0 +1,41 @@ > > +* Aptina 1/3-Inch WVGA CMOS Digital Image Sensor > > + > > +The Aptina MT9V032 is a 1/3-inch CMOS active pixel digital image sensor > > with > > +an active array size of 752H x 480V. It is programmable through a simple > > +two-wire serial interface. > > + > > +Required Properties: > > + > > +- compatible: value should be either one among the following > > + (a) "aptina,mt9v032" for MT9V032 color sensor > > + (b) "aptina,mt9v032m" for MT9V032 monochrome sensor > > + (c) "aptina,mt9v034" for MT9V034 color sensor > > + (d) "aptina,mt9v034m" for MT9V034 monochrome sensor > > It can't be determined at runtime whether the sensor is just monochromatic ? Unfortunately not. As far as I'm aware the only difference between the monochromatic and color sensors is the colour filter array. The register set is identical. > Al in all the color filter array is a physical property of the sensor, > still the driver seems to be ignoring the "m" suffix. No, the driver relies on the I2C core filling returning the I2C device id instance corresponding to the DT compatible string, and gets sensor model information from id->driver_data. > Hence I suspect the > register interfaces for both color and monochromatic versions are > compatible. I'm wondering whether using a boolean property to indicate the > color filter array type would do as well. That's an option as well, yes. I don't have a strong preference at the moment, but it should be noted that the "m" suffix is contained in the chip's part number. MT9V032C12STM MT9V032C12STC MT9V032C12STMD MT9V032C12STMH MT9V032C12STCD MT9V032C12STCH Granted, they use "c" for colour sensors, which the DT bindings don't use, and a "C12ST" that we completely ignore. > > +static struct mt9v032_platform_data * > > +mt9v032_get_pdata(struct i2c_client *client) > > +{ > > + struct mt9v032_platform_data *pdata; > > + struct v4l2_of_endpoint endpoint; > > + struct device_node *np; > > + struct property *prop; > > + > > + if (!IS_ENABLED(CONFIG_OF) || !client->dev.of_node) > > + return client->dev.platform_data; > > + > > + np = v4l2_of_get_next_endpoint(client->dev.of_node, NULL); > > + if (!np) > > + return NULL; > > + > > + if (v4l2_of_parse_endpoint(np, &endpoint) < 0) > > + goto done; > > + > > + pdata = devm_kzalloc(&client->dev, sizeof(*pdata), GFP_KERNEL); > > + if (!pdata) > > + goto done; > > + > > + prop = of_find_property(np, "link-freqs", NULL); > > I suspect you meant "link-frequencies" here ? Indeed, good catch. I'll fix that. > > + if (prop) { > > + size_t size = prop->length / 8; > > + u64 *link_freqs; > > + > > + link_freqs = devm_kzalloc(&client->dev, > > + size * sizeof(*link_freqs), > > + GFP_KERNEL); > > + if (!link_freqs) > > + goto done; > > + > > + if (of_property_read_u64_array(np, "link-frequencies", > > + link_freqs, size) < 0) > > + goto done; > > + > > + pdata->link_freqs = link_freqs; > > + pdata->link_def_freq = link_freqs[0]; > > + } > > + > > + pdata->clk_pol = !!(endpoint.bus.parallel.flags & > > + V4L2_MBUS_PCLK_SAMPLE_RISING); > > + > > +done: > > + of_node_put(np); > > + return pdata; > > +} > > > > @@ -1034,9 +1086,21 @@ static const struct i2c_device_id mt9v032_id[] = { > > > > }; > > MODULE_DEVICE_TABLE(i2c, mt9v032_id); > > > > +#if IS_ENABLED(CONFIG_OF) > > +static const struct of_device_id mt9v032_of_match[] = { > > + { .compatible = "mt9v032" }, > > + { .compatible = "mt9v032m" }, > > + { .compatible = "mt9v034" }, > > + { .compatible = "mt9v034m" }, > > + { /* Sentinel */ } > > +}; > > +MODULE_DEVICE_TABLE(of, mt9v032_of_match); > > +#endif
Hi Sakari, On 09/03/15 11:57, Sakari Ailus wrote: >>> +static struct mt9v032_platform_data * >>> +mt9v032_get_pdata(struct i2c_client *client) >>> +{ >>> + struct mt9v032_platform_data *pdata; >>> + struct v4l2_of_endpoint endpoint; >>> + struct device_node *np; >>> + struct property *prop; >>> + >>> + if (!IS_ENABLED(CONFIG_OF) || !client->dev.of_node) >>> + return client->dev.platform_data; >>> + >>> + np = v4l2_of_get_next_endpoint(client->dev.of_node, NULL); >>> + if (!np) >>> + return NULL; >>> + >>> + if (v4l2_of_parse_endpoint(np, &endpoint) < 0) >>> + goto done; >>> + >>> + pdata = devm_kzalloc(&client->dev, sizeof(*pdata), GFP_KERNEL); >>> + if (!pdata) >>> + goto done; >>> + >>> + prop = of_find_property(np, "link-freqs", NULL); >> >> I suspect you meant "link-frequencies" here ? > > Yes. I wonder if it'd make sense to add this to struct v4l2_of_endpoint. I > can write a patch for that. I don't have strong preference, sooner or later we will need to add it there, so code duplication can be avoided. I guess it makes sense to add such code already with a first user of it. >>> + if (prop) { >>> + size_t size = prop->length / 8; >>> + u64 *link_freqs; >>> + >>> + link_freqs = devm_kzalloc(&client->dev, >>> + size * sizeof(*link_freqs), >>> + GFP_KERNEL); >>> + if (!link_freqs) >>> + goto done; >>> + >>> + if (of_property_read_u64_array(np, "link-frequencies", >>> + link_freqs, size) < 0) >>> + goto done; >>> + >>> + pdata->link_freqs = link_freqs; >>> + pdata->link_def_freq = link_freqs[0]; >>> + } > > If you're interested in just a single value, you can use > of_property_read_u64(). > >>> + pdata->clk_pol = !!(endpoint.bus.parallel.flags & >>> + V4L2_MBUS_PCLK_SAMPLE_RISING); >>> + >>> +done: >>> + of_node_put(np); >>> + return pdata; >>> +}
On 09/03/15 12:29, Laurent Pinchart wrote: > On Monday 09 March 2015 11:35:52 Sylwester Nawrocki wrote: >> On 08/03/15 14:45, Laurent Pinchart wrote: >>> +++ b/Documentation/devicetree/bindings/media/i2c/mt9v032.txt >>> @@ -0,0 +1,41 @@ >>> +* Aptina 1/3-Inch WVGA CMOS Digital Image Sensor >>> + >>> +The Aptina MT9V032 is a 1/3-inch CMOS active pixel digital image sensor >>> with >>> +an active array size of 752H x 480V. It is programmable through a simple >>> +two-wire serial interface. >>> + >>> +Required Properties: >>> + >>> +- compatible: value should be either one among the following >>> + (a) "aptina,mt9v032" for MT9V032 color sensor >>> + (b) "aptina,mt9v032m" for MT9V032 monochrome sensor >>> + (c) "aptina,mt9v034" for MT9V034 color sensor >>> + (d) "aptina,mt9v034m" for MT9V034 monochrome sensor >> >> It can't be determined at runtime whether the sensor is just monochromatic ? > > Unfortunately not. As far as I'm aware the only difference between the > monochromatic and color sensors is the colour filter array. The register set > is identical. > >> Al in all the color filter array is a physical property of the sensor, >> still the driver seems to be ignoring the "m" suffix. > > No, the driver relies on the I2C core filling returning the I2C device id > instance corresponding to the DT compatible string, and gets sensor model > information from id->driver_data. Sorry, I missed the I2C id part. >> Hence I suspect the >> register interfaces for both color and monochromatic versions are >> compatible. I'm wondering whether using a boolean property to indicate the >> color filter array type would do as well. > > That's an option as well, yes. I don't have a strong preference at the moment, > but it should be noted that the "m" suffix is contained in the chip's part > number. > > MT9V032C12STM > MT9V032C12STC > MT9V032C12STMD > MT9V032C12STMH > MT9V032C12STCD > MT9V032C12STCH > > Granted, they use "c" for colour sensors, which the DT bindings don't use, and > a "C12ST" that we completely ignore. OK, deriving the compatible strings from current I2C device ids seems less trouble from the driver's writer POV. However, my feeling is that using same compatible and additional property to indicate colour/monochrome is cleaner as far as device tree binding is concerned. Anyway, I'm not going to object against your current approach, I suppose it's acceptable as well.
Hi Laurent, I am agree with your argumentations. I hope for the v2. Best regards, Carlos Sanmartín 2015-03-09 12:19 GMT+01:00 Laurent Pinchart <laurent.pinchart@ideasonboard.com>: > Hi Carlos, > > Thank you for the review. > > On Monday 09 March 2015 10:45:31 Carlos Sanmartín Bustos wrote: >> Hi Laurent, >> >> Looks good. One question, why not deprecate the platform data? I can't >> see any device using the mt9v032 pdata, I was making a similar patch >> but deprecating pdata. > > The sensor could be used on non-DT platforms. As keeping platform data support > doesn't really make the code more complex and doesn't prevent cleanups or > other refactoring, I don't see much harm in keeping it for now. > > For DT-based platforms, of course, DT should be used. > >> Some more comment: >> >> 2015-03-08 14:45 GMT+01:00 Laurent Pinchart: >> > Parse DT properties into a platform data structure when a DT node is >> > available. >> > >> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> >> > --- >> > >> > .../devicetree/bindings/media/i2c/mt9v032.txt | 41 ++++++++++++++ >> > MAINTAINERS | 1 + >> > drivers/media/i2c/mt9v032.c | 66 ++++++++++++++++- >> > 3 files changed, 107 insertions(+), 1 deletion(-) >> > create mode 100644 >> > Documentation/devicetree/bindings/media/i2c/mt9v032.txt > > [snip] > >> > diff --git a/drivers/media/i2c/mt9v032.c b/drivers/media/i2c/mt9v032.c >> > index 3267c18..a6ea091 100644 >> > --- a/drivers/media/i2c/mt9v032.c >> > +++ b/drivers/media/i2c/mt9v032.c > > [snip] > >> > +static struct mt9v032_platform_data * >> > +mt9v032_get_pdata(struct i2c_client *client) >> > +{ >> > + struct mt9v032_platform_data *pdata; >> > + struct v4l2_of_endpoint endpoint; >> > + struct device_node *np; >> > + struct property *prop; >> > + >> > + if (!IS_ENABLED(CONFIG_OF) || !client->dev.of_node) >> > + return client->dev.platform_data; >> > + >> > + np = v4l2_of_get_next_endpoint(client->dev.of_node, NULL); >> > + if (!np) >> > + return NULL; >> > + >> > + if (v4l2_of_parse_endpoint(np, &endpoint) < 0) >> > + goto done; >> >> Here I have one little testing: >> >> if (endpoint.bus_type != V4L2_MBUS_PARALLEL) { >> dev_err(dev, "invalid bus type, must be parallel\n"); >> goto done; >> } > > Good question, should drivers check DT properties that they don't use, in > order to catch errors in DT ? This would slightly increase the kernel size in > order to prevent people from connecting a parallel sensor to a CSI-2 bus, > which is obviously impossible in hardware :-) > >> > + >> > + pdata = devm_kzalloc(&client->dev, sizeof(*pdata), GFP_KERNEL); >> > + if (!pdata) >> > + goto done; >> > + >> > + prop = of_find_property(np, "link-freqs", NULL); >> > + if (prop) { >> > + size_t size = prop->length / 8; >> > + u64 *link_freqs; >> > + >> > + link_freqs = devm_kzalloc(&client->dev, >> > + size * sizeof(*link_freqs), >> > + GFP_KERNEL); >> > + if (!link_freqs) >> > + goto done; >> > + >> > + if (of_property_read_u64_array(np, "link-frequencies", >> > + link_freqs, size) < 0) >> > + goto done; >> > + >> > + pdata->link_freqs = link_freqs; >> > + pdata->link_def_freq = link_freqs[0]; >> > + } >> > + >> > + pdata->clk_pol = !!(endpoint.bus.parallel.flags & >> > + V4L2_MBUS_PCLK_SAMPLE_RISING); >> > + >> > +done: >> > + of_node_put(np); >> > + return pdata; >> > +} > > [snip] > >> > @@ -1034,9 +1086,21 @@ static const struct i2c_device_id mt9v032_id[] = { >> > }; >> > MODULE_DEVICE_TABLE(i2c, mt9v032_id); >> > >> > +#if IS_ENABLED(CONFIG_OF) >> > +static const struct of_device_id mt9v032_of_match[] = { >> > + { .compatible = "mt9v032" }, >> > + { .compatible = "mt9v032m" }, >> > + { .compatible = "mt9v034" }, >> > + { .compatible = "mt9v034m" }, >> > + { /* Sentinel */ } >> > +}; >> > +MODULE_DEVICE_TABLE(of, mt9v032_of_match); >> > +#endif >> >> I have this: >> #if IS_ENABLED(CONFIG_OF) >> static const struct of_device_id mt9v032_of_match[] = { >> { .compatible = "aptina,mt9v022", }, >> { .compatible = "aptina,mt9v022m", }, >> { .compatible = "aptina,mt9v024", }, >> { .compatible = "aptina,mt9v024m", }, >> { .compatible = "aptina,mt9v032", }, >> { .compatible = "aptina,mt9v032m", }, >> { .compatible = "aptina,mt9v034", }, >> { .compatible = "aptina,mt9v034m", }, > > Looks like I forgot to update my patch for mt9v02* support. Sorry about that, > I'll fix it. And the "aptina," prefix of course belongs there. > >> { /* sentinel */ }, >> }; >> >> MODULE_DEVICE_TABLE(of, mt9v032_of_match); >> #endi > > > -- > Regards, > > Laurent Pinchart > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Sylwester, On Monday 09 March 2015 13:22:12 Sylwester Nawrocki wrote: > On 09/03/15 12:29, Laurent Pinchart wrote: > > On Monday 09 March 2015 11:35:52 Sylwester Nawrocki wrote: > >> On 08/03/15 14:45, Laurent Pinchart wrote: > >>> +++ b/Documentation/devicetree/bindings/media/i2c/mt9v032.txt > >>> @@ -0,0 +1,41 @@ > >>> +* Aptina 1/3-Inch WVGA CMOS Digital Image Sensor > >>> + > >>> +The Aptina MT9V032 is a 1/3-inch CMOS active pixel digital image sensor > >>> with > >>> +an active array size of 752H x 480V. It is programmable through a > >>> simple > >>> +two-wire serial interface. > >>> + > >>> +Required Properties: > >>> + > >>> +- compatible: value should be either one among the following > >>> + (a) "aptina,mt9v032" for MT9V032 color sensor > >>> + (b) "aptina,mt9v032m" for MT9V032 monochrome sensor > >>> + (c) "aptina,mt9v034" for MT9V034 color sensor > >>> + (d) "aptina,mt9v034m" for MT9V034 monochrome sensor > >> > >> It can't be determined at runtime whether the sensor is just > >> monochromatic ? > > > > Unfortunately not. As far as I'm aware the only difference between the > > monochromatic and color sensors is the colour filter array. The register > > set is identical. > > > >> Al in all the color filter array is a physical property of the sensor, > >> still the driver seems to be ignoring the "m" suffix. > > > > No, the driver relies on the I2C core filling returning the I2C device id > > instance corresponding to the DT compatible string, and gets sensor model > > information from id->driver_data. > > Sorry, I missed the I2C id part. > > >> Hence I suspect the > >> register interfaces for both color and monochromatic versions are > >> compatible. I'm wondering whether using a boolean property to indicate > >> the > >> color filter array type would do as well. > > > > That's an option as well, yes. I don't have a strong preference at the > > moment, but it should be noted that the "m" suffix is contained in the > > chip's part number. > > > > MT9V032C12STM > > MT9V032C12STC > > MT9V032C12STMD > > MT9V032C12STMH > > MT9V032C12STCD > > MT9V032C12STCH > > > > Granted, they use "c" for colour sensors, which the DT bindings don't use, > > and a "C12ST" that we completely ignore. > > OK, deriving the compatible strings from current I2C device ids seems less > trouble from the driver's writer POV. However, my feeling is that using same > compatible and additional property to indicate colour/monochrome is cleaner > as far as device tree binding is concerned. > Anyway, I'm not going to object against your current approach, I suppose > it's acceptable as well. It wouldn't take much to convince me about that, I'm really undecided. I'll send v2 to fix other issues, we can continue discussing this point then.
diff --git a/Documentation/devicetree/bindings/media/i2c/mt9v032.txt b/Documentation/devicetree/bindings/media/i2c/mt9v032.txt new file mode 100644 index 0000000..75c97de --- /dev/null +++ b/Documentation/devicetree/bindings/media/i2c/mt9v032.txt @@ -0,0 +1,41 @@ +* Aptina 1/3-Inch WVGA CMOS Digital Image Sensor + +The Aptina MT9V032 is a 1/3-inch CMOS active pixel digital image sensor with +an active array size of 752H x 480V. It is programmable through a simple +two-wire serial interface. + +Required Properties: + +- compatible: value should be either one among the following + (a) "aptina,mt9v032" for MT9V032 color sensor + (b) "aptina,mt9v032m" for MT9V032 monochrome sensor + (c) "aptina,mt9v034" for MT9V034 color sensor + (d) "aptina,mt9v034m" for MT9V034 monochrome sensor + +Optional Properties: + +- link-frequencies: List of allowed link frequencies in Hz. Each frequency is + expressed as a 64-bit big-endian integer. + +For further reading on port node refer to +Documentation/devicetree/bindings/media/video-interfaces.txt. + +Example: + + i2c0@1c22000 { + ... + ... + mt9v032@5c { + compatible = "aptina,mt9v032"; + reg = <0x5c>; + + port { + mt9v032_1: endpoint { + link-frequencies = + <0 13000000>, <0 26600000>, + <0 27000000>; + }; + }; + }; + ... + }; diff --git a/MAINTAINERS b/MAINTAINERS index ddc5a8c..180f6fb 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -6535,6 +6535,7 @@ M: Laurent Pinchart <laurent.pinchart@ideasonboard.com> L: linux-media@vger.kernel.org T: git git://linuxtv.org/media_tree.git S: Maintained +F: Documentation/devicetree/bindings/media/i2c/mt9v032.txt F: drivers/media/i2c/mt9v032.c F: include/media/mt9v032.h diff --git a/drivers/media/i2c/mt9v032.c b/drivers/media/i2c/mt9v032.c index 3267c18..a6ea091 100644 --- a/drivers/media/i2c/mt9v032.c +++ b/drivers/media/i2c/mt9v032.c @@ -17,6 +17,8 @@ #include <linux/i2c.h> #include <linux/log2.h> #include <linux/mutex.h> +#include <linux/of.h> +#include <linux/of_gpio.h> #include <linux/regmap.h> #include <linux/slab.h> #include <linux/videodev2.h> @@ -26,6 +28,7 @@ #include <media/mt9v032.h> #include <media/v4l2-ctrls.h> #include <media/v4l2-device.h> +#include <media/v4l2-of.h> #include <media/v4l2-subdev.h> /* The first four rows are black rows. The active area spans 753x481 pixels. */ @@ -876,10 +879,59 @@ static const struct regmap_config mt9v032_regmap_config = { * Driver initialization and probing */ +static struct mt9v032_platform_data * +mt9v032_get_pdata(struct i2c_client *client) +{ + struct mt9v032_platform_data *pdata; + struct v4l2_of_endpoint endpoint; + struct device_node *np; + struct property *prop; + + if (!IS_ENABLED(CONFIG_OF) || !client->dev.of_node) + return client->dev.platform_data; + + np = v4l2_of_get_next_endpoint(client->dev.of_node, NULL); + if (!np) + return NULL; + + if (v4l2_of_parse_endpoint(np, &endpoint) < 0) + goto done; + + pdata = devm_kzalloc(&client->dev, sizeof(*pdata), GFP_KERNEL); + if (!pdata) + goto done; + + prop = of_find_property(np, "link-freqs", NULL); + if (prop) { + size_t size = prop->length / 8; + u64 *link_freqs; + + link_freqs = devm_kzalloc(&client->dev, + size * sizeof(*link_freqs), + GFP_KERNEL); + if (!link_freqs) + goto done; + + if (of_property_read_u64_array(np, "link-frequencies", + link_freqs, size) < 0) + goto done; + + pdata->link_freqs = link_freqs; + pdata->link_def_freq = link_freqs[0]; + } + + pdata->clk_pol = !!(endpoint.bus.parallel.flags & + V4L2_MBUS_PCLK_SAMPLE_RISING); + +done: + of_node_put(np); + return pdata; +} + static int mt9v032_probe(struct i2c_client *client, const struct i2c_device_id *did) { - struct mt9v032_platform_data *pdata = client->dev.platform_data; + struct mt9v032_platform_data *pdata = mt9v032_get_pdata(client); struct mt9v032 *mt9v032; unsigned int i; int ret; @@ -1034,9 +1086,21 @@ static const struct i2c_device_id mt9v032_id[] = { }; MODULE_DEVICE_TABLE(i2c, mt9v032_id); +#if IS_ENABLED(CONFIG_OF) +static const struct of_device_id mt9v032_of_match[] = { + { .compatible = "mt9v032" }, + { .compatible = "mt9v032m" }, + { .compatible = "mt9v034" }, + { .compatible = "mt9v034m" }, + { /* Sentinel */ } +}; +MODULE_DEVICE_TABLE(of, mt9v032_of_match); +#endif + static struct i2c_driver mt9v032_driver = { .driver = { .name = "mt9v032", + .of_match_table = of_match_ptr(mt9v032_of_match), }, .probe = mt9v032_probe, .remove = mt9v032_remove,
Parse DT properties into a platform data structure when a DT node is available. Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- .../devicetree/bindings/media/i2c/mt9v032.txt | 41 ++++++++++++++ MAINTAINERS | 1 + drivers/media/i2c/mt9v032.c | 66 +++++++++++++++++++++- 3 files changed, 107 insertions(+), 1 deletion(-) create mode 100644 Documentation/devicetree/bindings/media/i2c/mt9v032.txt