t the DT side. It will however not be easy work, which
explains why, so far, everybody has ignored the issue hoping that
someone else would be hit by it first. We may have reached that day.
> You could do it like you have below. That would be appropriate if
> there's a separate h/w device controlling the muxing. Say for example
> some board level device controlled by i2c.
--
Regards,
Laurent Pinchart
On Tue, May 26, 2020 at 12:32:01PM -0600, Rob Herring wrote:
> On Wed, 13 May 2020 20:22:37 +0300, Laurent Pinchart wrote:
> > From: Anurag Kumar Vulisha
> >
> > Add DT bindings for the Xilinx ZynqMP PHY. ZynqMP SoCs have a High Speed
> > Processing System Gigabit Tra
quot;, GPIOD_OUT_HIGH);
> + if (IS_ERR(lt9611->reset_gpio)) {
> + dev_err(dev, "failed to acquire reset gpio\n");
> + return PTR_ERR(lt9611->reset_gpio);
> + }
> +
> + lt9611->enable_gpio = devm_gpiod_get_optional(dev, "enable",
> + GPIOD_OUT_LOW);
> + if (IS_ERR(lt9611->enable_gpio)) {
> + dev_err(dev, "failed to acquire enable gpio\n");
> + return PTR_ERR(lt9611->enable_gpio);
> + }
> +
> + return 0;
> +}
> +
> +static int lt9611_read_device_rev(struct lt9611 *lt9611)
> +{
> + unsigned int rev;
> + int ret;
> +
> + regmap_write(lt9611->regmap, 0x80ee, 0x01);
> + ret = regmap_read(lt9611->regmap, 0x8002, );
> + if (ret)
> + dev_err(lt9611->dev, "failed to read revision: %d\n", ret);
> +
> + dev_info(lt9611->dev, "LT9611 revision: 0x%x\n", rev);
> +
> + return ret;
> +}
> +
> +static int lt9611_probe(struct i2c_client *client,
> + const struct i2c_device_id *id)
> +{
> + struct lt9611 *lt9611;
> + struct device *dev = >dev;
> + int ret;
> +
> + if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
> + dev_err(dev, "device doesn't support I2C\n");
> + return -ENODEV;
> + }
> +
> + lt9611 = devm_kzalloc(dev, sizeof(*lt9611), GFP_KERNEL);
> + if (!lt9611)
> + return -ENOMEM;
> +
> + lt9611->dev = >dev;
> + lt9611->client = client;
> + lt9611->sleep = false;
> +
> + lt9611->regmap = devm_regmap_init_i2c(client, _regmap_config);
> + if (IS_ERR(lt9611->regmap)) {
> + DRM_ERROR("regmap i2c init failed\n");
> + return PTR_ERR(lt9611->regmap);
> + }
> +
> + ret = lt9611_parse_dt(>dev, lt9611);
> + if (ret) {
> + dev_err(dev, "failed to parse device tree\n");
> + return ret;
> + }
> +
> + ret = lt9611_gpio_init(lt9611);
> + if (ret < 0)
> + return ret;
> +
> + ret = lt9611_regulator_init(lt9611);
> + if (ret < 0)
> + return ret;
> +
> + lt9611_assert_5v(lt9611);
> +
> + ret = lt9611_regulator_enable(lt9611);
> + if (ret)
> + return ret;
> +
> + lt9611_reset(lt9611);
> +
> + ret = lt9611_read_device_rev(lt9611);
> + if (ret) {
> + dev_err(dev, "failed to read chip rev\n");
> + goto err_disable_regulators;
> + }
> +
> + ret = devm_request_threaded_irq(dev, client->irq, NULL,
> + lt9611_irq_thread_handler,
> + IRQF_ONESHOT, "lt9611", lt9611);
> + if (ret) {
> + dev_err(dev, "failed to request irq\n");
> + goto err_disable_regulators;
> + }
> +
> + i2c_set_clientdata(client, lt9611);
> +
> + lt9611->bridge.funcs = _bridge_funcs;
> + lt9611->bridge.of_node = client->dev.of_node;
> +
> + drm_bridge_add(>bridge);
> +
> + lt9611_enable_hpd_interrupts(lt9611);
> +
> + return 0;
> +
> +err_disable_regulators:
> + regulator_bulk_disable(ARRAY_SIZE(lt9611->supplies), lt9611->supplies);
> +
> + of_node_put(lt9611->dsi0_node);
> + of_node_put(lt9611->dsi1_node);
> +
> + return ret;
> +}
> +
> +static int lt9611_remove(struct i2c_client *client)
> +{
> + struct lt9611 *lt9611 = i2c_get_clientdata(client);
> +
> + disable_irq(client->irq);
> + drm_bridge_remove(>bridge);
> +
> + regulator_bulk_disable(ARRAY_SIZE(lt9611->supplies), lt9611->supplies);
> +
> + of_node_put(lt9611->dsi0_node);
> + of_node_put(lt9611->dsi1_node);
> +
> + return 0;
> +}
> +
> +static struct i2c_device_id lt9611_id[] = {
> + { "lontium,lt9611", 0},
> + {}
> +};
> +
> +static const struct of_device_id lt9611_match_table[] = {
> + {.compatible = "lontium,lt9611"},
> + {}
> +};
> +MODULE_DEVICE_TABLE(of, lt9611_match_table);
> +
> +static struct i2c_driver lt9611_driver = {
> + .driver = {
> + .name = "lt9611",
> + .of_match_table = lt9611_match_table,
> + },
> + .probe = lt9611_probe,
> + .remove = lt9611_remove,
> + .id_table = lt9611_id,
> +};
> +module_i2c_driver(lt9611_driver);
> +
> +MODULE_LICENSE("GPL v2");
--
Regards,
Laurent Pinchart
d-supply = <_1v8>;
> +vcc-supply = <_3v3>;
> +
> +ports {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + port@0 {
> +reg = <0>;
> +lt9611_out: endpoint {
> + remote-endpoint = <_con>;
> +};
> + };
> +
> + port@1 {
> +reg = <1>;
> +lt9611_a: endpoint {
> + remote-endpoint = <_out>;
> +};
> + };
> +
> + port@2 {
> +reg = <2>;
> +lt9611_b: endpoint {
> + remote-endpoint = <_out>;
> +};
> + };
> +};
> + };
> +};
It's customary to end YAML schema files with ... on a separate line.
--
Regards,
Laurent Pinchart
Hi Hyun,
On Wed, May 27, 2020 at 10:54:35AM -0700, Hyun Kwon wrote:
> On Sat, 2020-05-23 at 20:08:13 -0700, Laurent Pinchart wrote:
> > On Mon, May 04, 2020 at 11:43:48AM -0700, Hyun Kwon wrote:
> >> On Mon, 2020-04-20 at 14:20:56 -0700, Venkateshwar Rao Gannavarapu wrote:
>
ormat Bridge.
>
> Signed-off-by: Vishal Sagar
> Reviewed-by: Hyun Kwon
> Reviewed-by: Rob Herring
> Reviewed-by: Luca Ceresoli
> Reviewed-by: Laurent Pinchart
> ---
> v14
> - Removed xlnx,csi-pxl-format from required properties
> - Added dependency of xlnx,csi-px
DMA_USE_IOMMU
> > > #include
> > > #endif
> >
> > Why does this file need at all?
> > It doesn't call any of the flush_*() functions, and seems to compile fine
> > without (on arm32).
> >
> > Perhaps it was included at the top intentionally, to override the
> > definitions
> > of copy_{to,from}_user_page()? Fortunately that doesn't seem to be the
> > case, from a quick look at the assembler output.
> >
> > So let's just remove the #include instead?
>
> Sounds good to me. I can send a patch if needed or I suppose Andrew can
> just make a small fixup patch for it. Let me know what I should do.
--
Regards,
Laurent Pinchart
Hi Vishal,
On Wed, May 27, 2020 at 11:53:01AM +, Vishal Sagar wrote:
> On Sunday, May 24, 2020 7:32 AM, Laurent Pinchart wrote:
> > On Tue, May 12, 2020 at 08:49:46PM +0530, Vishal Sagar wrote:
> > > Add bindings documentation for Xilinx MIPI CSI-2 Rx Subsystem.
> >
ata)
> > +{
> > + struct xlnx_dsi *dsi = dev_get_drvdata(dev);
> > + struct drm_encoder *encoder = >encoder;
> > + struct drm_device *drm_dev = data;
> > + int ret;
> > +
> > + drm_encoder_init(drm_dev, encoder, _dsi_encoder_funcs,
> > +DRM_MODE_ENCODER_DSI, NULL);
> > + drm_encoder_helper_add(encoder, _dsi_encoder_helper_funcs);
> > +
> > + encoder->possible_crtcs = 1;
> > +
> > + ret = xlnx_dsi_create_connector(encoder);
> > + if (ret) {
> > + dev_err(dsi->dev, "fail creating connector, ret = %d\n", ret);
> > + drm_encoder_cleanup(encoder);
> > + return ret;
> > + }
> > +
> > + ret = mipi_dsi_host_register(>dsi_host);
> > + if (ret < 0) {
>
> Just nit. if (ret) would do.
>
> > + dev_err(dsi->dev, "failed to register DSI host: %d\n", ret);
> > + xlnx_dsi_connector_destroy(>connector);
> > + drm_encoder_cleanup(encoder);
> > + return ret;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static void xlnx_dsi_unbind(struct device *dev, struct device *master,
> > + void *data)
> > +{
> > + struct xlnx_dsi *dsi = dev_get_drvdata(dev);
> > +
> > + xlnx_dsi_disable(>encoder);
> > + mipi_dsi_host_unregister(>dsi_host);
> > +}
> > +
> > +static const struct component_ops xlnx_dsi_component_ops = {
> > + .bind = xlnx_dsi_bind,
> > + .unbind = xlnx_dsi_unbind,
> > +};
> > +
> > +static int xlnx_dsi_probe(struct platform_device *pdev)
> > +{
> > + struct device *dev = >dev;
> > + struct resource *res;
> > + struct xlnx_dsi *dsi;
> > + int ret;
> > + unsigned long rate;
> > +
> > + dsi = devm_kzalloc(dev, sizeof(*dsi), GFP_KERNEL);
> > + if (!dsi)
> > + return -ENOMEM;
> > +
> > + dsi->dsi_host.ops = _dsi_ops;
> > + dsi->dsi_host.dev = dev;
> > + dsi->dev = dev;
> > +
> > + ret = xlnx_dsi_parse_dt(dsi);
> > + if (ret)
> > + return ret;
> > +
> > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > + dsi->iomem = devm_ioremap_resource(dev, res);
> > + if (IS_ERR(dsi->iomem)) {
> > + dev_err(dev, "failed to remap io region\n");
> > + return PTR_ERR(dsi->iomem);
> > + }
> > + platform_set_drvdata(pdev, dsi);
> > +
> > + ret = clk_set_rate(dsi->dphy_clk_200M, XDSI_DPHY_CLK_REQ);
> > + if (ret) {
> > + dev_err(dev, "failed to set dphy clk rate %d\n", ret);
> > + return ret;
> > + }
> > +
> > + ret = clk_prepare_enable(dsi->dphy_clk_200M);
> > + if (ret) {
> > + dev_err(dev, "failed to enable dphy clk %d\n", ret);
> > + return ret;
> > + }
> > +
> > + rate = clk_get_rate(dsi->dphy_clk_200M);
> > + if (rate < XDSI_DPHY_CLK_MIN && rate > XDSI_DPHY_CLK_MAX) {
>
> A brief comment for this line wold be helpful.
>
> > + dev_err(dev, "Error DPHY clock = %lu\n", rate);
> > + ret = -EINVAL;
> > + goto err_disable_dphy_clk;
> > + }
> > +
> > + ret = clk_prepare_enable(dsi->video_aclk);
> > + if (ret) {
> > + dev_err(dev, "failed to enable video clk %d\n", ret);
> > + goto err_disable_dphy_clk;
> > + }
> > +
> > + ret = component_add(dev, _dsi_component_ops);
The driver should expose the DSI-TX as a drm_bridge instead of using the
component framework. You shouldn't register a drm_encoder, and I don't
think you should register a drm_connector either. Only bridge operations
should be exposed, and the drm_bridge .attach() operation should return
an error when DRM_BRIDGE_ATTACH_NO_CONNECTOR is not set. The top-level
driver using this bridge should create the drm_encoder and
drm_connector, most likely using drm_bridge_connector_init() to create
the connector.
> > + if (ret < 0)
>
> I prefer if (ret) where possible, but it may be just me. I let you decide.
>
> > + goto err_disable_video_clk;
> > +
> > + return ret;
>
> return 0;? but up to you.
>
> > +
> > +err_disable_video_clk:
> > + clk_disable_unprepare(dsi->video_aclk);
> > +err_disable_dphy_clk:
> > + clk_disable_unprepare(dsi->dphy_clk_200M);
> > + return ret;
> > +}
>
> At least, this initial one better come with a complete display pipeline,
> ex crtc / plane are missing. This won't be functional without those.
> I believe Laurent was planning to make the xlnx drm layer as a library,
> so hopefully this can be a good starting point and come with drivers aligned
> with it.
>
> > +
> > +static int xlnx_dsi_remove(struct platform_device *pdev)
> > +{
> > + struct xlnx_dsi *dsi = platform_get_drvdata(pdev);
> > +
> > + component_del(>dev, _dsi_component_ops);
> > + clk_disable_unprepare(dsi->video_aclk);
> > + clk_disable_unprepare(dsi->dphy_clk_200M);
> > +
> > + return 0;
> > +}
> > +
> > +static const struct of_device_id xlnx_dsi_of_match[] = {
> > + { .compatible = "xlnx-dsi", },
>
> This should change. I believe it should be something like, xlnx, name>-
>
> > + { /* end of table */ },
> > +};
> > +MODULE_DEVICE_TABLE(of, xlnx_dsi_of_match);
> > +
> > +static struct platform_driver dsi_driver = {
> > + .probe = xlnx_dsi_probe,
> > + .remove = xlnx_dsi_remove,
> > + .driver = {
> > + .name = "xlnx-dsi",
> > + .of_match_table = xlnx_dsi_of_match,
> > + },
> > +};
> > +
> > +module_platform_driver(dsi_driver);
> > +
> > +MODULE_AUTHOR("Xilinx, Inc.");
> > +MODULE_DESCRIPTION("Xilinx FPGA MIPI DSI Tx Driver");
> > +MODULE_LICENSE("GPL v2");
--
Regards,
Laurent Pinchart
; > + xlnx,dsi-data-type = ;
> > + #address-cells = <1>;
> > + #size-cells = <0>;
> > + clock-names = "dphy_clk_200M", "s_axis_aclk";
> > + clocks = <_clk_0>, <_clk_1>;
> > + encoder_dsi_port: port@0 {
> > + reg = <0>;
> > + dsi_encoder: endpoint {
> > + remote-endpoint = <_ep>;
> > + };
> > + };
> > + simple_panel: simple-panel@0 {
You can drop unused labels.
> > + compatible = "auo,b101uan01";
> > + reg = <0>;
> > + };
This line doesn't appear to be needed.
> > + };
--
Regards,
Laurent Pinchart
um number of virtual channels becomes 16 from 4.
>
> Signed-off-by: Vishal Sagar
> Reviewed-by: Hyun Kwon
> Reviewed-by: Laurent Pinchart
> ---
> v13
> - Based on Laurent's suggestions
> - Removed unnecessary debug statement for vep
> - Added TODO for c
ormat Bridge.
>
> Signed-off-by: Vishal Sagar
> Reviewed-by: Hyun Kwon
> Reviewed-by: Rob Herring
> Reviewed-by: Luca Ceresoli
> Reviewed-by: Laurent Pinchart
> ---
> v13
> - Based on Laurent's suggestions
> - Fixed the datatypes values as minimum and maximum
> - condition
Hi Vishal,
On Fri, May 08, 2020 at 01:52:36PM +, Vishal Sagar wrote:
> On Tuesday, May 5, 2020 7:53 PM, Laurent Pinchart wrote:
> > On Thu, Apr 23, 2020 at 09:00:37PM +0530, Vishal Sagar wrote:
> > > Add bindings documentation for Xilinx MIPI CSI-2 Rx Subsystem.
> >
r_lvds.o:(.rodata+0xe08): undefined reference to
> `drm_atomic_helper_connector_duplicate_state'
> drivers/gpu/drm/rcar-du/rcar_lvds.o:(.rodata+0xe10): undefined reference to
> `drm_atomic_helper_connector_destroy_state'
>
> Signed-off-by: Daniel Gomez
Reviewed-by: Laurent Pinchart
and taken in my tree for
> We configure the pixel clock based upon the number of cameras connected,
> and their pixel rates etc ...
>
> Are you saying that the frequency of this clock should be validated to
> be a specific range? or are you talking about a different frequency?
>
>
> For now I'll remove the v4l2_fwnode_endpoint_alloc_parse().
>
>
>
> >> +
> >> + continue;
> >> + }
> >> +
> >> + /* Skip if the corresponding GMSL link is unavailable. */
> >> + if (!(i2c_mux_mask & BIT(ep.port)))
> >> + continue;
> >> +
> >> + if (priv->sources[ep.port].fwnode) {
> >> + dev_err(dev,
> >> + "Multiple port endpoints are not supported: %d",
> >> + ep.port);
> >> +
> >> + continue;
> >> + }
> >> +
> >> + source = >sources[ep.port];
> >> + source->fwnode = fwnode_graph_get_remote_endpoint(
> >> + of_fwnode_handle(node));
> >> + if (!source->fwnode) {
> >> + dev_err(dev,
> >> + "Endpoint %pOF has no remote endpoint
> >> connection\n",
> >> + ep.local_node);
> >> +
> >> + continue;
> >> + }
> >> +
> >> + priv->source_mask |= BIT(ep.port);
> >> + priv->nsources++;
> >> + }
> >> + of_node_put(node);
> >> + of_node_put(dev->of_node);
> >> +
> >> + priv->route_mask = priv->source_mask;
> >> +
> >> + return 0;
> >> +}
> >
>
--
Regards,
Laurent Pinchart
st, the next_brige would create one connector and this brige
> >>>> would create another connector.
> >>>>
> >>>> I think in your case, mtk_dsi does not directly connect to a panel, so
> >>>
> >>> Exactly
> >>>
> >>
Add a DT node for the PS-GTR transceivers.
Signed-off-by: Laurent Pinchart
Acked-by: Michal Simek
---
arch/arm64/boot/dts/xilinx/zynqmp.dtsi | 10 ++
1 file changed, 10 insertions(+)
diff --git a/arch/arm64/boot/dts/xilinx/zynqmp.dtsi
b/arch/arm64/boot/dts/xilinx/zynqmp.dtsi
index
ZynqMP PSGTR PHY
phy: zynqmp: Add PHY driver for the Xilinx ZynqMP Gigabit Transceiver
Laurent Pinchart (1):
arm64: dts: zynqmp: Add GTR transceivers
.../bindings/phy/xlnx,zynqmp-psgtr.yaml | 105 ++
MAINTAINERS | 9 +
arch/arm64/boot/dts/xilinx
-by: Anurag Kumar Vulisha
Signed-off-by: Laurent Pinchart
---
Changes since v7:
- Move driver to xilinx subdirectory
- Fix typo in MAINTAINERS
- Remove dash lines in section header comments
- Fix wrong indentation.
Changes since v5:
- Cleanup headers
- Organize the code in sections
- Constify data
-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/phy/xlnx,zynqmp-psgtr.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Xilinx ZynqMP Gigabit Transceiver PHY Device Tree Bindings
+
+maintainers:
+ - Laurent Pinchart
+
+description
du_map_sg() function.
>
> Signed-off-by: Marek Szyprowski
This is a very nice simplification, I've always foudn the dma_map_sg API
cumbersome to use.
Reviewed-by: Laurent Pinchart
I assume you will get the whole series merged in one go. If I need to
pick the patch up at any point,
Hi Kishon,
On Wed, May 13, 2020 at 08:16:19AM +0530, Kishon Vijay Abraham I wrote:
> On 5/8/2020 6:23 AM, Laurent Pinchart wrote:
> > On Thu, May 07, 2020 at 10:14:45AM +0530, Kishon Vijay Abraham I wrote:
> >> On 4/2/2020 3:40 AM, Laurent Pinchart wrote:
> >>&
, "cannot create I2C device for IT6251 LVDS\n");
> + return PTR_ERR(it6251->lvds_client);
> + }
> +
> + it6251->lvds_regmap = regmap_init_i2c(it6251->lvds_client,
> + _regmap_config);
> + if (IS_ERR(it6
ly = <_display>;
> +
> + ports {
> +#address-cells = <1>;
> + #size-cells = <0>;
> +
> +port@0 {
> + reg = <0>;
> + bridge_out_edp0: endpoint {
> +remote-endpoint = <_in_edp0>;
> + };
> +};
> +
> +port@1 {
> + reg = <1>;
> + bridge_in_lvds0: endpoint {
> +remote-endpoint = <_out>;
> + };
> +};
> + };
> +};
--
Regards,
Laurent Pinchart
Hi Kishon,
On Thu, May 07, 2020 at 10:14:45AM +0530, Kishon Vijay Abraham I wrote:
> On 4/2/2020 3:40 AM, Laurent Pinchart wrote:
> > From: Anurag Kumar Vulisha
> >
> > Xilinx ZynqMP SoCs have a Gigabit Transceiver with four lanes. All the
> > high speed peripher
Hi Doug,
On Tue, May 05, 2020 at 05:18:48PM -0700, Doug Anderson wrote:
> On Tue, May 5, 2020 at 2:24 PM Doug Anderson wrote:
> > On Tue, May 5, 2020 at 2:14 PM Laurent Pinchart wrote:
> > >
> > > > I'll add this documentation into the comments of the yaml, bu
Hi Vishal,
Thank you for the patch.
There are a few questions for Hans below.
On Wed, Apr 29, 2020 at 07:47:04PM +0530, Vishal Sagar wrote:
> The Xilinx UHD-SDI Rx subsystem soft IP is used to capture native SDI
> streams from SDI sources like SDI broadcast equipment like cameras and
> mixers.
- |
> +uhdsdirxss: v-smpte-uhdsdi-rxss@8000 {
> + compatible = "xlnx,v-smpte-uhdsdi-rx-ss-2.0";
> + interrupt-parent = <>;
> + interrupts = <0 89 4>;
> + reg = <0x0 0x8000 0x0 0x1>;
> + xlnx,include-edh;
> + xlnx,line-rate = "12G_SDI_8DS";
> + clocks = <_1>, <_1>, <_2>;
> + clock-names = "s_axi_aclk", "sdi_rx_clk", "video_out_clk";
> + xlnx,bpp = <10>;
> +
> + ports {
> +#address-cells = <1>;
> +#size-cells = <0>;
> +port@0 {
> + reg = <0>;
> + sdirx_out: endpoint {
> +remote-endpoint = <_sdirx_in>;
> + };
> +};
> + };
> +};
--
Regards,
Laurent Pinchart
um number of virtual channels becomes 16 from 4.
>
> Signed-off-by: Vishal Sagar
> Reviewed-by: Hyun Kwon
> ---
> v12
> - Changes done as suggested by Laurent Pinchart and Luca Ceresoli
> - Removed unused macros
> - No local storage of supported formats
> - Dropped init
to the device tree bindings.
>
> [1] https://lore.kernel.org/r/20200417180819.ge5...@pendragon.ideasonboard.com
>
> Signed-off-by: Douglas Anderson
> Reviewed-by: Stephen Boyd
> Reviewed-by: Linus Walleij
Reviewed-by: Laurent Pinchart
> ---
>
> Changes in
g/r/20200417180819.ge5...@pendragon.ideasonboard.com
>
> Signed-off-by: Douglas Anderson
> Reviewed-by: Stephen Boyd
> Reviewed-by: Linus Walleij
Reviewed-by: Laurent Pinchart
> ---
>
> Changes in v4:
> - Tacked on "or is otherwise unusable." to description.
>
be
additionalProperties: false
here ?
> +
> +properties:
> + "#address-cells":
> +const: 1
> +
> + "#size-cells":
> +const: 0
> +
> + port@0:
> +type: object
> +additionalProperties: false
> +
> +
Hi Doug,
On Tue, May 05, 2020 at 02:12:20PM -0700, Doug Anderson wrote:
> On Tue, May 5, 2020 at 2:06 PM Laurent Pinchart wrote:
> > On Tue, May 05, 2020 at 10:59:30AM -0700, Doug Anderson wrote:
> >> On Tue, May 5, 2020 at 1:24 AM Laurent Pinchart wrote:
> >>> On M
Hi Doug,
On Tue, May 05, 2020 at 10:59:30AM -0700, Doug Anderson wrote:
> On Tue, May 5, 2020 at 1:24 AM Laurent Pinchart wrote:
> > On Mon, May 04, 2020 at 09:36:31PM -0700, Douglas Anderson wrote:
> > > The ti-sn65dsi86 MIPI DSI to eDP bridge chip supports arbitrary
> >
>;
> +interrupts = <0 95 4>;
> +xlnx,csi-pxl-format = <0x2a>;
> +xlnx,vfb;
> + xlnx,en-active-lanes;
> +xlnx,en-csi-v2-0;
> +xlnx,en-vcx;
> +clock-names = "lite_aclk", "video_aclk";
> +clocks = <_clk_0>, <_clk_1>;
> +video-reset-gpios = < 86 GPIO_ACTIVE_LOW>;
> +
> +ports {
> +#address-cells = <1>;
> +#size-cells = <0>;
> +
> +port@0 {
> +/* Sink port */
> +reg = <0>;
> +csiss_in: endpoint {
> +data-lanes = <1 2 3 4>;
> +/* MIPI CSI-2 Camera handle */
> +remote-endpoint = <_out>;
> +};
> +};
> +port@1 {
> +/* Source port */
> +reg = <1>;
> +csiss_out: endpoint {
> +remote-endpoint = <_in>;
> +};
> +};
> +};
> +};
--
Regards,
Laurent Pinchart
> +
> + /* Stash in our struct for when we power on */
> + pdata->dp_lanes = dp_lanes;
> + pdata->ln_assign = ln_assign;
> + pdata->ln_polrs = ln_polrs;
> +}
> +
> static int ti_sn_bridge_probe(struct i2c_client *client,
> const struct i2c_device_id *id)
> {
> @@ -1105,6 +1152,8 @@ static int ti_sn_bridge_probe(struct i2c_client *client,
> return ret;
> }
>
> + ti_sn_bridge_parse_lanes(pdata, client->dev.of_node);
> +
> ret = ti_sn_bridge_parse_regulators(pdata);
> if (ret) {
> DRM_ERROR("failed to parse regulators\n");
>
--
Regards,
Laurent Pinchart
Hi Sakari,
On Thu, Apr 30, 2020 at 05:18:49PM +0300, Sakari Ailus wrote:
> On Thu, Apr 30, 2020 at 05:17:53PM +0300, Laurent Pinchart wrote:
> > On Thu, Apr 30, 2020 at 05:15:52PM +0300, Sakari Ailus wrote:
> >> On Thu, Apr 30, 2020 at 04:59:04PM +0300, Laurent Pinchart wrote:
Hi Sakari,
On Thu, Apr 30, 2020 at 05:15:52PM +0300, Sakari Ailus wrote:
> On Thu, Apr 30, 2020 at 04:59:04PM +0300, Laurent Pinchart wrote:
> > On Thu, Apr 30, 2020 at 04:31:25PM +0300, Sakari Ailus wrote:
> > > On Thu, Apr 30, 2020 at 02:10:14PM +0300, Laurent Pinchart wro
Hi Sakari,
On Thu, Apr 30, 2020 at 04:31:25PM +0300, Sakari Ailus wrote:
> On Thu, Apr 30, 2020 at 02:10:14PM +0300, Laurent Pinchart wrote:
> > On Thu, Apr 30, 2020 at 12:42:33PM +0300, Sakari Ailus wrote:
> >> On Tue, Apr 14, 2020 at 10:01:49PM +0200, Daniel Gomez wrote:
>
al;
>
> This changes the memory layout of the struct and breaks the ABI. Any new
> fields in the struct may only replace reserved fields while the rest must
> stay unchanged.
>
> > struct v4l2_fract interval;
> > __u32 which;
> > - __u32 reserved[8];
> > + __u32 reserved[4];
> > };
> >
> > /**
--
Regards,
Laurent Pinchart
__u8 bPreferredVersion;
> + __u8 bPreferedVersion __attribute__((deprecated)); /* NOTYPO */
> + } __attribute__((__packed__));
Quite interestingly this typo is part of the USB device class definition
for video devices (UVC) specification. I thus think we should keep using
the field name bPreferedVersion through the code, otherwise it wouldn't
match the spec.
> __u8 bMinVersion;
> __u8 bMaxVersion;
> } __attribute__((__packed__));
[snip]
--
Regards,
Laurent Pinchart
gt; index a708a0340eb1..6e2f4c3eb24f 100644
> --- a/drivers/staging/media/imx/imx7-media-csi.c
> +++ b/drivers/staging/media/imx/imx7-media-csi.c
> @@ -1003,8 +1003,6 @@ static int imx7_csi_try_fmt(struct imx7_csi *csi,
>
> sdformat->format.colorspace = in_fmt->colorspace;
> sdformat->format.xfer_func = in_fmt->xfer_func;
> - sdformat->format.quantization = in_fmt->quantization;
> - sdformat->format.ycbcr_enc = in_fmt->ycbcr_enc;
> break;
> case IMX7_CSI_PAD_SINK:
> *cc = imx_media_find_mbus_format(sdformat->format.code,
> @@ -1015,14 +1013,14 @@ static int imx7_csi_try_fmt(struct imx7_csi *csi,
>false);
> sdformat->format.code = (*cc)->codes[0];
> }
> -
> - imx_media_fill_default_mbus_fields(>format, in_fmt,
> -false);
> break;
> default:
> return -EINVAL;
> break;
> }
> +
> + imx_media_try_colorimetry(>format, false);
> +
> return 0;
> }
>
> --
> 2.17.1
>
--
Regards,
Laurent Pinchart
Hi Jacopo,
On Thu, Oct 17, 2019 at 02:43:49PM +0200, Jacopo Mondi wrote:
> On Wed, Oct 16, 2019 at 04:45:26PM +0300, Laurent Pinchart wrote:
> > On Wed, Oct 16, 2019 at 10:55:43AM +0200, Jacopo Mondi wrote:
> > > Add a driver for the R-Car Display Unit Color Correction Module.
&
Adam Ford
> +
> +allOf:
> + - $ref: panel-common.yaml#
> +
> +properties:
> + compatible:
> +const: logicpd,type28
> +
> + power-supply: true
> + enable-gpios: true
> + backlight: true
> + port: true
> +
> +required:
> + - compatibl
*crtc,
> struct rcar_du_crtc_state *rstate = to_rcar_crtc_state(crtc->state);
> struct rcar_du_device *rcdu = rcrtc->dev;
>
> + if (rcrtc->cmm)
> + rcar_cmm_enable(rcrtc->cmm);
> rcar_du_crtc_get(rcrtc);
>
> /*
> @@
ev, mem);
> + iss->regs[res] = devm_platform_ioremap_resource(pdev, res);
>
> return PTR_ERR_OR_ZERO(iss->regs[res]);
> }
--
Regards,
Laurent Pinchart
elam
The change looks good to me.
Reviewed-by: Laurent Pinchart
and applied to my tree for v5.5.
> ---
> drivers/staging/media/omap4iss/iss.c | 6 +-
> 1 file changed, 1 insertion(+), 5 deletions(-)
>
> diff --git a/drivers/staging/media/omap4iss/iss.c
> b/drivers/s
Hi Will,
On Wed, Oct 02, 2019 at 02:19:29PM +0100, Will Deacon wrote:
> On Wed, Oct 02, 2019 at 04:09:13PM +0300, Laurent Pinchart wrote:
> > Thank you for the patch.
>
> And thank you for the quick response.
>
> > On Wed, Oct 02, 2019 at 12:27:53PM +0100, Will Deacon w
()': once via 'uvc_scan_chain_entity()' when it is
> processed directly from the 'dev->entities' list and then again
> immediately afterwards when trying to follow the source ID in
> 'uvc_scan_chain_forward()'
>
> Add a check before adding an entity to a chain list to ensure that the
> entity
ate should be set to the first upstream sub-device only,
> > not everywhere, for a sub-device driver itself knows how to best control
> > the streaming state of its own upstream sub-devices.
> >
> > Signed-off-by: Sakari Ailus
> > Reviewed-by: Laurent Pinchart
>
dy to assume to be the maintainer if needed.
That would be great ! I used to own a DC10 a long time ago, and the
zoran driver was the first one I tried to hack when I moved to Linux.
I'd love to see it getting maintained. Please let us know if you need
any help getting started.
--
Regards,
Laurent Pinchart
vc/uvcvideo.h
> index c7c1baa90dea..f773dc5d802c 100644
> --- a/drivers/media/usb/uvc/uvcvideo.h
> +++ b/drivers/media/usb/uvc/uvcvideo.h
> @@ -491,7 +491,7 @@ struct uvc_stats_stream {
> unsigned int max_sof; /* Maximum STC.SOF value */
> };
>
> -#define UVC_METATADA_BUF_SIZE 1024
> +#define UVC_METADATA_BUF_SIZE 1024
>
> /**
> * struct uvc_copy_op: Context structure to schedule asynchronous memcpy
--
Regards,
Laurent Pinchart
Hi Pawel,
On Mon, Sep 02, 2019 at 10:27:39AM +0200, Pavel Machek wrote:
> On Mon 2019-09-02 11:19:42, Laurent Pinchart wrote:
> > On Mon, Sep 02, 2019 at 10:06:57AM +0200, Pavel Machek wrote:
> >>>>> Single integer. It's read-only, so it just reports the location
we should control that through a
location control though, as it seems there's more than the rotation of
the camera involved. In any case I wouldn't care about it for now, and
turn the location control from read-only to read-write later if needed.
We need more information and more thought to support that use case.
--
Regards,
Laurent Pinchart
. And
> IIRC
> these devices had LEDs on top of the phone... so neither front nor back side.
I would go for the name "torch" in that case. It really depends on the
device, but in any case, the torch LEDs would have a location (and we
would possibly need to expand this property to
include the top, bottom, left and right sides).
--
Regards,
Laurent Pinchart
g to
> > downward facing, then the driver might change the location from FRONT
> > to DOWN. A convoluted example perhaps, but this is just brainstorming.
>
> There are phones with exactly such camera setup. And yes, it makes
> sense to be writable in that case, as software can move the camera in
> such case.
Out of curiosity, what phones are those ?
--
Regards,
Laurent Pinchart
ht (C) 2015-2017 Helen Koike
> */
>
> -#include
> -#include
> -#include
> -#include
> #include
> #include
> #include
> @@ -18,8 +14,6 @@
>
> #include "vimc-common.h"
>
> -#define VIMC_SEN_DRV_NAME "vimc-sensor"
> -
> struct vimc_sen_device {
> struct vimc_ent_device ved;
> struct v4l2_subdev sd;
> @@ -304,13 +298,15 @@ static const struct v4l2_subdev_internal_ops
> vimc_sen_int_ops = {
> .release = vimc_sen_release,
> };
>
> -static void vimc_sen_comp_unbind(struct device *comp, struct device *master,
> - void *master_data)
> +void vimc_sen_rm(struct vimc_device *vimc, struct vimc_ent_config *vcfg)
> {
> - struct vimc_ent_device *ved = dev_get_drvdata(comp);
> - struct vimc_sen_device *vsen =
> - container_of(ved, struct vimc_sen_device, ved);
> + struct vimc_ent_device *ved = vcfg->ved;
> + struct vimc_sen_device *vsen;
>
> + if (!ved)
> + return;
> +
> + vsen = container_of(ved, struct vimc_sen_device, ved);
> vimc_ent_sd_unregister(ved, >sd);
> }
>
> @@ -331,11 +327,9 @@ static const struct v4l2_ctrl_config
> vimc_sen_ctrl_test_pattern = {
> .qmenu = tpg_pattern_strings,
> };
>
> -static int vimc_sen_comp_bind(struct device *comp, struct device *master,
> - void *master_data)
> +int vimc_sen_add(struct vimc_device *vimc, struct vimc_ent_config *vcfg)
> {
> - struct v4l2_device *v4l2_dev = master_data;
> - struct vimc_platform_data *pdata = comp->platform_data;
> + struct v4l2_device *v4l2_dev = >v4l2_dev;
> struct vimc_sen_device *vsen;
> int ret;
>
> @@ -368,7 +362,7 @@ static int vimc_sen_comp_bind(struct device *comp, struct
> device *master,
>
> /* Initialize ved and sd */
> ret = vimc_ent_sd_register(>ved, >sd, v4l2_dev,
> -pdata->entity_name,
> +vcfg->name,
> MEDIA_ENT_F_CAM_SENSOR, 1,
> (const unsigned long[1])
> {MEDIA_PAD_FL_SOURCE},
> _sen_int_ops, _sen_ops);
> @@ -376,8 +370,7 @@ static int vimc_sen_comp_bind(struct device *comp, struct
> device *master,
> goto err_free_hdl;
>
> vsen->ved.process_frame = vimc_sen_process_frame;
> - dev_set_drvdata(comp, >ved);
> - vsen->dev = comp;
> + vsen->dev = >pdev.dev;
>
> /* Initialize the frame format */
> vsen->mbus_format = fmt_default;
> @@ -389,6 +382,7 @@ static int vimc_sen_comp_bind(struct device *comp, struct
> device *master,
> if (ret)
> goto err_unregister_ent_sd;
>
> + vcfg->ved = >ved;
> return 0;
>
> err_unregister_ent_sd:
> @@ -400,44 +394,3 @@ static int vimc_sen_comp_bind(struct device *comp,
> struct device *master,
>
> return ret;
> }
> -
> -static const struct component_ops vimc_sen_comp_ops = {
> - .bind = vimc_sen_comp_bind,
> - .unbind = vimc_sen_comp_unbind,
> -};
> -
> -static int vimc_sen_probe(struct platform_device *pdev)
> -{
> - return component_add(>dev, _sen_comp_ops);
> -}
> -
> -static int vimc_sen_remove(struct platform_device *pdev)
> -{
> - component_del(>dev, _sen_comp_ops);
> -
> - return 0;
> -}
> -
> -static const struct platform_device_id vimc_sen_driver_ids[] = {
> - {
> - .name = VIMC_SEN_DRV_NAME,
> - },
> - { }
> -};
> -
> -static struct platform_driver vimc_sen_pdrv = {
> - .probe = vimc_sen_probe,
> - .remove = vimc_sen_remove,
> - .id_table = vimc_sen_driver_ids,
> - .driver = {
> - .name = VIMC_SEN_DRV_NAME,
> - },
> -};
> -
> -module_platform_driver(vimc_sen_pdrv);
> -
> -MODULE_DEVICE_TABLE(platform, vimc_sen_driver_ids);
> -
> -MODULE_DESCRIPTION("Virtual Media Controller Driver (VIMC) Sensor");
> -MODULE_AUTHOR("Helen Mae Koike Fornazier ");
> -MODULE_LICENSE("GPL");
--
Regards,
Laurent Pinchart
Hi Andrey,
On Mon, Aug 26, 2019 at 09:24:57PM -0700, Andrey Smirnov wrote:
> On Mon, Aug 26, 2019 at 3:08 PM Laurent Pinchart wrote:
> > On Mon, Aug 26, 2019 at 11:25:24AM -0700, Andrey Smirnov wrote:
> > > Presently, the driver code artificially limits test pattern mode to a
&g
Hi Helen,
On Wed, Aug 21, 2019 at 06:46:15PM -0300, Helen Koike wrote:
> On 8/15/19 2:54 PM, Laurent Pinchart wrote:
> > On Wed, Aug 07, 2019 at 10:37:55AM -0300, Helen Koike wrote:
> >> On 8/7/19 10:05 AM, Sakari Ailus wrote:
> >>> On Tue, Jul 30, 2019 at 03:42
Hi Adam,
On Wed, Aug 21, 2019 at 01:45:28PM -0500, Adam Ford wrote:
> On Wed, Aug 21, 2019 at 1:37 PM Laurent Pinchart wrote:
> > On Sun, Aug 18, 2019 at 05:08:38PM +0300, Aaro Koskinen wrote:
> > > Hi,
> > >
> > > I haven't got display
Hi Aaro,
On Sun, Aug 18, 2019 at 05:08:38PM +0300, Aaro Koskinen wrote:
> Hi,
>
> I haven't got display working on N900 since v5.1. Bisected to:
>
> d17eb4537a7eb16da9eafbfd5717e12b45b77251 is the first bad commit
> commit d17eb4537a7eb16da9eafbfd5717e12b45b77251
> Aut
age enhancement module available on each R-Car DU video
> >>>> channel on R-Car Gen2 and Gen3 SoCs (V3H and V3M excluded).
> >>>>
> >>>> Signed-off-by: Jacopo Mondi
> >>>> Reviewed-by: Laurent Pinchart
> >>>
> >>> Th
+ data-lanes = <1 2>;
> + };
> +
> + mipi_in_ucam: endpoint@1 {
> + reg = <1>;
> + remote-endpoint = <_out>;
> + data-lanes = <1>;
> + };
What are wcam and ucam ? It would help if you showed the sensor nodes in
the example.
> + };
> + };
> + };
--
Regards,
Laurent Pinchart
Hello Helen,
Thank you for the patch.
On Tue, Jul 30, 2019 at 03:42:47PM -0300, Helen Koike wrote:
> From: Jacob Chen
>
> Add the subdev driver for rockchip isp1.
>
> Signed-off-by: Jacob Chen
> Signed-off-by: Shunqian Zheng
> Signed-off-by: Yichong Zhong
> Signed-off-by: Jacob Chen
>
ADATA
> format, although that might be too late (there might be userspace
> complications).
Yes, probably not a good idea.
> >> + fmt->format.field = V4L2_FIELD_NONE;
> >> + return 0;
> >> + }
> >> +
> >> + if (fmt->which == V4L2_SUBDEV_FORMAT_TRY) {
> >> + mf = v4l2_subdev_get_try_format(sd, cfg, fmt->pad);
> >> + fmt->format = *mf;
> >> + return 0;
> >> + }
> >> +
> >> + if (fmt->pad == RKISP1_ISP_PAD_SINK) {
> >> + *mf = isp_sd->in_frm;
> >> + } else if (fmt->pad == RKISP1_ISP_PAD_SOURCE_PATH) {
> >> + /* format of source pad */
> >> + *mf = isp_sd->in_frm;
> >> + mf->code = isp_sd->out_fmt.mbus_code;
> >> + /* window size of source pad */
> >> + mf->width = isp_sd->out_crop.width;
> >> + mf->height = isp_sd->out_crop.height;
> >> + mf->quantization = isp_sd->quantization;
> >> + }
> >> + mf->field = V4L2_FIELD_NONE;
> >> +
> >> + return 0;
> >> +}
--
Regards,
Laurent Pinchart
ured value for Bayer pattern R
> + * @meas_gr: Mean measured value for Bayer pattern Gr
> + * @meas_gb: Mean measured value for Bayer pattern Gb
> + * @meas_b: Mean measured value for Bayer pattern B
> + */
> +struct cifisp_bls_meas_val {
> + __u16 meas_r;
> + __u16 meas_gr;
> + __u16 meas_gb;
> + __u16 meas_b;
> +} __attribute__ ((packed));
> +
> +/**
> + * struct cifisp_ae_stat - statistics auto exposure data
> + *
> + * @exp_mean: Mean luminance value of block xx
> + * @bls_val: BLS measured values
> + *
> + * Image is divided into 5x5 blocks.
> + */
> +struct cifisp_ae_stat {
> + __u8 exp_mean[CIFISP_AE_MEAN_MAX];
> + struct cifisp_bls_meas_val bls_val;
> +} __attribute__ ((packed));
> +
> +/**
> + * struct cifisp_af_meas_val - AF measured values
> + *
> + * @sum: sharpness, refer to REF_01 for definition
> + * @lum: luminance, refer to REF_01 for definition
That will be lovely to use without documentation...
> + */
> +struct cifisp_af_meas_val {
> + __u32 sum;
> + __u32 lum;
> +} __attribute__ ((packed));
> +
> +/**
> + * struct cifisp_af_stat - statistics auto focus data
> + *
> + * @window: AF measured value of window x
> + *
> + * The module measures the sharpness in 3 windows of selectable size via
> + * register settings(ISP_AFM_*_A/B/C)
> + */
> +struct cifisp_af_stat {
> + struct cifisp_af_meas_val window[CIFISP_AFM_MAX_WINDOWS];
> +} __attribute__ ((packed));
> +
> +/**
> + * struct cifisp_hist_stat - statistics histogram data
> + *
> + * @hist_bins: measured bin counters
> + *
> + * Measurement window divided into 25 sub-windows, set
> + * with ISP_HIST_XXX
What is ISP_HIST_XXX ?
> + */
> +struct cifisp_hist_stat {
> + __u16 hist_bins[CIFISP_HIST_BIN_N_MAX];
> +} __attribute__ ((packed));
> +
> +/**
> + * struct rkisp1_stat_buffer - Rockchip ISP1 Statistics Data
> + *
> + * @cifisp_awb_stat: statistics data for automatic white balance
> + * @cifisp_ae_stat: statistics data for auto exposure
> + * @cifisp_af_stat: statistics data for auto focus
> + * @cifisp_hist_stat: statistics histogram data
> + */
> +struct cifisp_stat {
> + struct cifisp_awb_stat awb;
> + struct cifisp_ae_stat ae;
> + struct cifisp_af_stat af;
> + struct cifisp_hist_stat hist;
> +} __attribute__ ((packed));
> +
> +/**
> + * struct rkisp1_stat_buffer - Rockchip ISP1 Statistics Meta Data
> + *
> + * @meas_type: measurement types (CIFISP_STAT_ definitions)
> + * @frame_id: frame ID for sync
> + * @params: statistics data
> + */
> +struct rkisp1_stat_buffer {
> + __u32 meas_type;
> + __u32 frame_id;
> + struct cifisp_stat params;
> +} __attribute__ ((packed));
> +
> +#endif /* _UAPI_RKISP1_CONFIG_H */
--
Regards,
Laurent Pinchart
..
> +
> + dphy: mipi-dphy {
> + compatible = "rockchip,rk3399-mipi-dphy";
> + clocks = < SCLK_MIPIDPHY_REF>,
> + < SCLK_DPHY_RX0_CFG>,
> + < PCLK_VIO_GRF>;
> + clock-names = "dphy-ref", "dphy-cfg", "grf";
> + power-domains = < RK3399_PD_VIO>;
> + #phy-cells = <0>;
> + };
> +};
--
Regards,
Laurent Pinchart
Hi Greg,
On Thu, Aug 15, 2019 at 04:53:00PM +0200, Greg Kroah-Hartman wrote:
> On Thu, Aug 15, 2019 at 02:31:26PM +, Fabrizio Castro wrote:
> > On 15 August 2019 15:15, Laurent Pinchart wrote:
> > > On Thu, Aug 15, 2019 at 04:04:00PM +0200, Greg Kroah-Hartman wrote:
>
t; +}
> >> +
> >> +static const struct phy_ops rockchip_dphy_ops = {
> >> + .power_on = rockchip_dphy_power_on,
> >> + .power_off = rockchip_dphy_power_off,
> >> + .init = rockchip_dphy_init,
> >> + .exit = rockchip_dphy_exit,
> >> + .configure = rockchip_dphy_configure,
> >> + .owner = THIS_MODULE,
> >> +};
> >> +
> >> +static const struct dphy_drv_data rk3399_mipidphy_drv_data = {
> >> + .clks = rk3399_mipidphy_clks,
> >> + .num_clks = ARRAY_SIZE(rk3399_mipidphy_clks),
> >> + .hsfreq_ranges = rk3399_mipidphy_hsfreq_ranges,
> >> + .num_hsfreq_ranges = ARRAY_SIZE(rk3399_mipidphy_hsfreq_ranges),
> >> + .regs = rk3399_grf_dphy_regs,
> >
> > Do you expect to support more of the similar PHY(s) --- are there such? If
> > not, you could put these in the code that uses them.
>
> Yes, for rk3288 in the future.
>
> >> +};
> >> +
> >> +static const struct of_device_id rockchip_dphy_dt_ids[] = {
> >> + {
> >> + .compatible = "rockchip,rk3399-mipi-dphy",
> >> + .data = _mipidphy_drv_data,
> >> + },
> >> + {}
> >> +};
> >> +MODULE_DEVICE_TABLE(of, rockchip_dphy_dt_ids);
> >> +
> >> +static int rockchip_dphy_probe(struct platform_device *pdev)
> >> +{
> >> + struct device *dev = >dev;
> >> + struct device_node *np = dev->of_node;
> >> + const struct dphy_drv_data *drv_data;
> >> + struct phy_provider *phy_provider;
> >> + const struct of_device_id *of_id;
> >> + struct rockchip_dphy *priv;
> >> + struct regmap *grf;
> >> + struct phy *phy;
> >> + unsigned int i;
> >> + int ret;
> >> +
> >> + if (!dev->parent || !dev->parent->of_node)
> >> + return -ENODEV;
> >> +
> >> + if (platform_get_resource(pdev, IORESOURCE_MEM, 0)) {
> >> + dev_err(>dev, "Rockchip DPHY driver only suports rx\n");
You can replace pdev->dev with dev here and below.
s/rx/RX mode/ ?
> >> + return -EINVAL;
> >> + }
> >> +
> >> + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> >> + if (!priv)
> >> + return -ENOMEM;
> >> + priv->dev = dev;
> >> +
> >> + grf = syscon_node_to_regmap(dev->parent->of_node);
> >> + if (IS_ERR(grf)) {
> >> + grf = syscon_regmap_lookup_by_phandle(dev->of_node,
> >> +"rockchip,grf");
> >> + if (IS_ERR(grf)) {
> >> + dev_err(dev, "Can't find GRF syscon\n");
> >> + return -ENODEV;
> >> + }
> >> + }
> >> + priv->grf = grf;
> >> +
> >> + of_id = of_match_device(rockchip_dphy_dt_ids, dev);
> >> + if (!of_id)
> >> + return -EINVAL;
> >> +
> >> + drv_data = of_id->data;
> >> + priv->grf_regs = drv_data->regs;
Do you have to store grf_regs in priv, or could it be accessed through
priv->drv_data->regs ?
> >> + priv->drv_data = drv_data;
> >> + for (i = 0; i < drv_data->num_clks; i++)
> >> + priv->clks[i].id = drv_data->clks[i];
> >> + ret = devm_clk_bulk_get(>dev, drv_data->num_clks, priv->clks);
> >> + if (ret)
> >> + return ret;
> >> +
> >> + phy = devm_phy_create(dev, np, _dphy_ops);
> >> + if (IS_ERR(phy)) {
> >> + dev_err(dev, "failed to create phy\n");
> >> + return PTR_ERR(phy);
> >> + }
> >> + phy_set_drvdata(phy, priv);
> >> +
> >> + phy_provider = devm_of_phy_provider_register(dev, of_phy_simple_xlate);
> >> +
> >> + return PTR_ERR_OR_ZERO(phy_provider);
> >> +}
> >> +
> >> +static struct platform_driver rockchip_dphy_driver = {
> >> + .probe = rockchip_dphy_probe,
> >> + .driver = {
> >> + .name = "rockchip-mipi-dphy",
> >> + .of_match_table = rockchip_dphy_dt_ids,
> >> + },
> >> +};
> >> +module_platform_driver(rockchip_dphy_driver);
> >> +
> >> +MODULE_AUTHOR("Ezequiel Garcia ");
> >> +MODULE_DESCRIPTION("Rockchip MIPI Synopsys DPHY driver");
> >> +MODULE_LICENSE("Dual MIT/GPL");
Overall this is quite good, there are only small issues.
--
Regards,
Laurent Pinchart
tree/bindings/media/rockchip-mipi-dphy.txt
This is missing the include/ files and the custom format documentation.
Apart from that, I'm happy to see that you will maintain this driver :-)
> +
> ROCKCHIP RASTER 2D GRAPHIC ACCELERATION UNIT DRIVER
> M: Jacob chen
> L: linux-me...@vger.kernel.org
--
Regards,
Laurent Pinchart
> +
> +
> +Rockchip ISP1 Statistics Data
> +
> +Description
> +===
> +
> +This format describes image color statistics information generated by the
> Rockchip
> +ISP1.
> +
> +It uses c-struct :c:type:`rkisp1_stat_buffer`, which is defined in
> +the ``linux/rkisp1-config.h`` header file.
Same comment here, I think we need to document alignment/padding
constraints.
> +
> +.. kernel-doc:: include/uapi/linux/rkisp1-config.h
> + :functions: rkisp1_stat_buffer
--
Regards,
Laurent Pinchart
hotographed.
> > +
> > +
> > +
> > +.. flat-table::
> > +:header-rows: 0
> > +:stub-columns: 0
> > +
> > +* - ``V4L2_LOCATION_FRONT``
> > + - The camera device is located on the front side of the device.
> > +* - ``V4L2_LO
ass a large set of controls this way when they have to be
applied atomically, but it's clearly a hack in my opinion. I won't
oppose to it though, as I know there's no way we can support this kind
of feature in V4L2 at the moment without an extensive amount of work, so
Reviewed-by: Laurent Pinchart
On Thu, Aug 15, 2019 at 04:15:10PM +0300, Sakari Ailus wrote:
> On Thu, Aug 15, 2019 at 04:10:53PM +0300, Laurent Pinchart wrote:
> > On Thu, Aug 15, 2019 at 04:08:49PM +0300, Sakari Ailus wrote:
> >> On Thu, Aug 15, 2019 at 03:59:38PM +0300, Laurent Pinchart wrote:
> >
Hi Sakari,
On Thu, Aug 15, 2019 at 04:08:49PM +0300, Sakari Ailus wrote:
> On Thu, Aug 15, 2019 at 03:59:38PM +0300, Laurent Pinchart wrote:
> > On Thu, Aug 15, 2019 at 10:00:25AM +0300, Sakari Ailus wrote:
> > > On Wed, Aug 14, 2019 at 10:28:12PM +0200, Jacopo Mondi
On Thu, Aug 15, 2019 at 03:02:45PM +0200, Jacopo Mondi wrote:
> Hi Laurent,
>
> On Thu, Aug 15, 2019 at 01:53:53AM +0300, Laurent Pinchart wrote:
> > Hi Jacopo,
> >
> > Thank you for the patch.
> >
> > On Wed, Aug 14, 2019 at 10:28:13PM +0200, Jacopo Mondi
f the device.
> > +
> > +
> > +
> > .. [#f1]
> > This control may be changed to a menu control in the future, if more
> > options are required.
>
> There's an effective limit of 64 for menus. ACPI has less than ten
> different locations for a device, I think 64 will be enough here.
>
> So I'd be actually in favour of switching to a menu.
Why ? As you explained yourself, it's a static read-only control, all it
needs to report is a single value.
--
Regards,
Laurent Pinchart
flash, I envision
it will be useful to know more about its physical location relative to
the camera sensor, but that will be a displacement, not a front/back
location as the flash should really be on the same side as the camera
sensor :-) Note that, technically speaking, it will not be the location
of the flash controller itself, but of its LED (or other light source).
A flash controller could possibly control multiple LEDs, for different
sensors, and possibly on different sides of the devices, so we may need
to create subnodes for light sources in the flash controller DT node.
--
Regards,
Laurent Pinchart
do so already though, unless you
think all flash and lens controllers should really use it.
> Flash (torch) devices could be present, at least principle, without a
> camera. There once was even such a Nokia phone, 1100 unless I'm mistaken.
> :-)
--
Regards,
Laurent Pinchart
; + }
> +
As most camera sensors should do this, I think it would make sense to
create a helper function. We could add parsing of other standard
sensor-related controls there (such as the link frequencies), and also
create the pixel rate control.
> if (ctrl_hdlr->error) {
> ret = ctrl_hdlr->error;
> goto error;
--
Regards,
Laurent Pinchart
AMERA_CLASS_BASE+33)
>
> +#define V4L2_CID_LOCATION(V4L2_CID_CAMERA_CLASS_BASE+34)
> +#define V4L2_LOCATION_FRONT (0 << 0)
> +#define V4L2_LOCATION_BACK (1 << 0)
Why not just 0 and 1 ?
> +
> /* FM Modulator class control IDs */
>
> #define V4L2_CID_FM_TX_CLASS_BASE(V4L2_CTRL_CLASS_FM_TX | 0x900)
--
Regards,
Laurent Pinchart
ted on the back side of the device.
> +
> +
> +
> .. [#f1]
> This control may be changed to a menu control in the future, if more
> options are required.
--
Regards,
Laurent Pinchart
; means
would be useful. In particular I would give examples for phones, tablets
and laptops.
>
> Optional endpoint properties
>
--
Regards,
Laurent Pinchart
Hi Helen,
On Tue, Aug 13, 2019 at 09:25:01AM -0300, Helen Koike wrote:
> On 8/13/19 6:56 AM, Laurent Pinchart wrote:
> > On Mon, Aug 12, 2019 at 08:41:33PM -0300, Helen Koike wrote:
> >> On 8/12/19 7:14 PM, Shuah Khan wrote:
> >>> On 8/12/19 1:10 PM, Shuah Khan wro
;>>> 2e4a5ad8ad6d media: vimc: Collapse component structure into a single
> >>>>>>>> monolithic driver
> >>>>>>>> 7c8da1687e92 media: vimc: move private defines to a common header
> >>>>>>>> 97299a303532 media: Remove dev_err() usage after platform_get_irq()
> >>>>>>>> 25a3d6bac6b9 media: adv7511/cobalt: rename driver name to
> >>>>>>>> adv7511-v4l2
> >>>>>
> >>>>> I couldn't reproduce the error, my tree looks the same:
> >>>>>
> >>>>> [I] koike@floko ~/m/o/linux> git log --oneline
> >>>>> e3345155c8ed (HEAD) media: vimc: Fix gpf in rmmod path when stream is
> >>>>> active
> >>>>> 43e9e2fe761f media: vimc: Collapse component structure into a single
> >>>>> monolithic driver
> >>>>> 8a6d0b9adde0 media: vimc: move private defines to a common header
> >>>>> 97299a303532 (media/master) media: Remove dev_err() usage after
> >>>>> platform_get_irq()
> >>>>> 25a3d6bac6b9 media: adv7511/cobalt: rename driver name to adv7511-v4l2
> >>>>
> >>>> Thanks Helen for trying to reproduce and sharing the result.
> >>>
> >>> Me and Helen found out what is the problem. If you follow this call trace:
> >>>
> >>> vimc_ent_sd_unregister()
> >>> v4l2_device_unregister_subdev()
> >>> v4l2_subdev_release()
> >>>
> >>> You'll notice that this last function calls the `release` callback
> >>> implementation of the subdevice. For instance, the `release` of
> >>> vimc-sensor is this one:
> >>>
> >>> static void vimc_sen_release(struct v4l2_subdev *sd)
> >>> {
> >>> struct vimc_sen_device *vsen =
> >>> container_of(sd, struct vimc_sen_device, sd);
> >>>
> >>> v4l2_ctrl_handler_free(>hdl);
> >>> tpg_free(>tpg);
> >>> kfree(vsen);
> >>> }
> >>>
> >>> And then you can see that `vsen` has been freed. Back to
> >>> vimc_ent_sd_unregister(), after v4l2_device_unregister_subdev(), the
> >>> function will call vimc_pads_cleanup(). This is basically a
> >>> kfree(ved->pads), but `ved` has just been freed at
> >>> v4l2_subdev_release(), producing a memory fault.
> >>>
> >>> To fix that, we found two options:
> >>>
> >>> - place the kfree(ved->pads) inside the release callback of each
> >>> subdevice and removing vimc_pads_cleanup() from
> >>> vimc_ent_sd_unregister()
> >>> - use a auxiliary variable to hold the address of the pads, for instance:
> >>>
> >>> void vimc_ent_sd_unregister(...)
> >>> {
> >>> struct media_pad *pads = ved->pads;
> >>> ...
> >>> vimc_pads_cleanup(pads);
> >>> }
> >>>
> >>>
> >>
> >> I fixed a problem in the thirds patch. vimc-capture uses the first
> >> approach - placing the kfree(ved->pads) inside the release callback.
> >>
> >> I am debugging another such problem in unbind path while streaming.
> >> I am working on v2 and I will look for the rmmod problem and fix it.
> >>
> >> thanks again for testing and finding the root cause.
> >> -- Shuah
> >
> > Hi Andre,
> >
> > Here is what's happening.
> >
> > Before this change, you can't really do rmmod vimc, because vimc is in
> > use by other component drivers. With the collapse, now you can actually
> > do rmmod on vimc and this problem in vimc_ent_sd_unregister() that frees
> > pads first and the does v4l2_device_unregister_subdev().
> >
> > I fixed this in the 3/3 patch. I can reproduce the problem with patches 1
> > and 2, and patch 3 fixes it.
> >
> > Did you test with the third patch in this series?
>
> yes, we tested with 3/3, but the new problem now is when doing the following
> in this order:
>
> v4l2_device_unregister_subdev(sd);
> vimc_pads_cleanup(ved->pads);
>
>
> v4l2_device_unregister_subdev() calls the release function of the subdevice
> that
> frees the ved object, so ved->pads is not valid anymore. That is why André
> suggested
> a temporary variable to hold ved->pads and to be able to free it later:
>
> struct media_pad *pads = ved->pads;
>
> v4l2_device_unregister_subdev(sd);
> vimc_pads_cleanup(pads); // So we don't use the ved object here anymore.
Can't you simply call vimc_pads_cleanup() in the release function of the
subdevice before freeing the ved object ?
--
Regards,
Laurent Pinchart
Hi Shua,
On Mon, Aug 12, 2019 at 08:19:27AM -0600, Shuah Khan wrote:
> On 8/10/19 8:14 AM, Laurent Pinchart wrote:
> > On Fri, Aug 09, 2019 at 03:45:41PM -0600, Shuah Khan wrote:
> >> In preparation for collapsing the component driver structure into
> >> a monolith, mo
unsigned int col, unsigned int rgb[3]);
> + /* Values calculated when the stream starts */
> + u8 *src_frame;
> + const struct vimc_deb_pix_map *sink_pix_map;
> + unsigned int sink_bpp;
> +};
> +
> +struct vimc_sen_device {
> + struct vimc_ent_device ved;
> + struct v4l2_subdev sd;
> + struct device *dev;
> + struct tpg_data tpg;
> + struct task_struct *kthread_sen;
> + u8 *frame;
> + /* The active format */
> + struct v4l2_mbus_framefmt mbus_format;
> + struct v4l2_ctrl_handler hdl;
> +};
> +
> +struct vimc_device {
> + /* The platform device */
> + struct platform_device pdev;
> +
> + /* The pipeline configuration */
> + const struct vimc_pipeline_config *pipe_cfg;
> +
> + /* The Associated media_device parent */
> + struct media_device mdev;
> +
> + /* Internal v4l2 parent device*/
> + struct v4l2_device v4l2_dev;
> +
> + /* Subdevices */
> + struct platform_device **subdevs;
> +};
> +
> +#endif
--
Regards,
Laurent Pinchart
Hi Fabrizio,
On Mon, Aug 05, 2019 at 09:32:42AM +, Fabrizio Castro wrote:
> On 02 August 2019 09:06 Laurent Pinchart wrote:
> > On Fri, Aug 02, 2019 at 08:34:02AM +0100, Fabrizio Castro wrote:
> > > When in vertical stripe mode of operation, there is the option
> >
Hi Fabrizio,
On Mon, Aug 05, 2019 at 08:59:51AM +, Fabrizio Castro wrote:
> > From: Laurent Pinchart
> > Sent: 02 August 2019 08:44
> > Subject: Re: [PATCH/RFC 02/12] dt-bindings: display: renesas: lvds:
> > Document renesas,swap-data
> >
> > Hi Fabriz
Hi Sakari,
On Fri, Jul 05, 2019 at 10:55:22AM +0300, Sakari Ailus wrote:
> On Thu, Jun 27, 2019 at 04:38:24PM +0300, Laurent Pinchart wrote:
> > On Thu, Jun 27, 2019 at 12:38:40PM +, Hugues FRUCHET wrote:
> >> On 6/26/19 7:25 PM, Laurent Pinchart wrote:
> >>>
Hi Shuah,
On Wed, Jul 03, 2019 at 10:52:17AM -0600, Shuah Khan wrote:
> On 7/3/19 10:17 AM, Niklas Söderlund wrote:
> > On 2019-06-30 14:41:02 +0300, Laurent Pinchart wrote:
> >> On Fri, Jun 28, 2019 at 10:41:07AM -0600, Shuah Khan wrote:
> >>> On 6/16/19 12
Hi Rob,
On Sun, Jun 30, 2019 at 02:50:59PM -0700, Rob Clark wrote:
> On Sun, Jun 30, 2019 at 2:17 PM Laurent Pinchart wrote:
> > On Sun, Jun 30, 2019 at 01:36:08PM -0700, Rob Clark wrote:
> > > From: Rob Clark
> > >
> > > Use the drm_of_find_panel_id() he
Hi Shuah,
On Fri, Jun 28, 2019 at 10:41:07AM -0600, Shuah Khan wrote:
> On 6/16/19 12:45 PM, Laurent Pinchart wrote:
> > On Fri, Jun 14, 2019 at 05:26:46PM -0600, Shuah Khan wrote:
> >> On 6/13/19 7:24 AM, Helen Koike wrote:
> >>> On 6/13/19 2:44 AM, Hans Verkuil wr
Hi Hugues,
On Thu, Jun 27, 2019 at 12:38:40PM +, Hugues FRUCHET wrote:
> On 6/26/19 7:25 PM, Laurent Pinchart wrote:
> > On Mon, Jun 24, 2019 at 10:10:05AM +, Hugues FRUCHET wrote:
> >> Hi Sakari,
> >>
> >>> - Where's the su
representing the bridge itself?
> >
> > - As the driver becomes MC-centric, crop configuration takes place through
> >V4L2 sub-device interface, not through the video device node.
> >
> > - Same goes for accessing sensor configuration: it does not take place
> >through video node but through the sub-device nodes.
> >
--
Regards,
Laurent Pinchart
RM InfoFrame support on H6
> > >
> > > drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 109
> > > drivers/gpu/drm/bridge/synopsys/dw-hdmi.h | 37 +++
> > > drivers/gpu/drm/meson/meson_dw_hdmi.c | 5 +
> > > drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c | 2 +
> > > drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c | 2 +
> > > drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h | 1 +
> > > include/drm/bridge/dw_hdmi.h| 1 +
> > > 7 files changed, 157 insertions(+)
--
Regards,
Laurent Pinchart
and life-time management happens without vimc
> component drivers being changed other than using the API to get and put
> media device reference.
Our replies crossed each other, please see my reply to Hans. I would
just like to comment here that if having multiple kernel modules causes
issue, they can all be merged together. There's no need for vimc to be
handled through multiple modules (I actually think it's quite
counterproductive, it only makes it more complex, for no added value).
--
Regards,
Laurent Pinchart
4 +-
> > drivers/media/media-dev-allocator.c| 39 ++
> > drivers/media/platform/vimc/vimc-capture.c | 17 +-
> > drivers/media/platform/vimc/vimc-core.c| 60 --
> > include/media/media-dev-allocator.h| 46 ++---
> > 5 files changed, 130 insertions(+), 36 deletions(-)
--
Regards,
Laurent Pinchart
t;, <0x13>,
> + /* set GPIO2 high, backlight PWM (set to 0x50 for normal use) */
> + <0x1E>, <0x50>,
> + /* sets GPIO3 as an output with remote control for touch XRES */
> + <0x1F>, <0x05>,
> + /* set GPIO5 high, backlight enable on new display */
> + <0x20>, <0x09>,
> + /* set GPIO7 and GPIO8 high to enable touch power and prox
> sense */
> + <0x21>, <0x91>;
> };
--
Regards,
Laurent Pinchart
Hi Wolfram,
Thank you for the patch.
On Sat, Jun 08, 2019 at 12:55:48PM +0200, Wolfram Sang wrote:
> We have a dedicated pointer for that, so use it. Much easier to read and
> less computation involved.
>
> Signed-off-by: Wolfram Sang
Reviewed-by: Laurent Pinchart
and take
701 - 800 of 4490 matches
Mail list logo
