Re: [PATCH 0/2] drm/bridge: dw-hdmi: disable loading of DW-HDMI CEC sub-driver
On Tue, Apr 20, 2021 at 05:19:52PM +0200, Neil Armstrong wrote: > On 20/04/2021 17:13, Hans Verkuil wrote: > > On 16/04/2021 13:38, Neil Armstrong wrote: > >> On 16/04/2021 11:58, Laurent Pinchart wrote: > >>> Hi Neil, > >>> > >>> On Fri, Apr 16, 2021 at 11:27:35AM +0200, Neil Armstrong wrote: > >>>> This adds DW-HDMI driver a glue option to disable loading of the CEC > >>>> sub-driver. > >>>> > >>>> On some SoCs, the CEC functionality is enabled in the IP config bits, > >>>> but the > >>>> CEC bus is non-functional like on Amlogic SoCs, where the CEC config bit > >>>> is set > >>>> but the DW-HDMI CEC signal is not connected to a physical pin, leading > >>>> to some > >>>> confusion when the DW-HDMI CEC controller can't communicate on the bus. > >>> > >>> If we can't trust the CEC config bit, would it be better to not use it > >>> at all, and instead let each platform glue logic tell whether to enable > >>> CEC or not ? > >> > >> Actually, the CEC config bit is right, the HW exists and should be > >> functional, but > >> this bit doesn't tell if the CEC signal is connected to something. > >> > >> This lies in the IP integration, like other bits under the > >> "amlogic,meson-*-dw-hdmi" > >> umbrella. > >> > >> The first attempt was by Hans using DT, but adding a property in DT for a > >> vendor > >> specific compatible doesn't make sense. Another idea would be to describe > >> the > >> CEC signal endpoint like we do for video signal, but I think this is out > >> of scope and > >> this solution is much simpler and straightforward, and it's more an > >> exception than > >> a general use case to solve. > > > > While a DT property might not make sense in this particular case, I still > > believe that it is a perfectly valid approach in general: whether or not > > the CEC pin is connected is at the hardware level decision, it is not > > something that software can detect. If the designer of the board didn't > > connect it, then the only place you can define that is in the device tree. > > Agreed, we need to define a smart way to declare CEC bus relationship in DT, > the side > effect would be to handle this particular case. I wonder if it would make sense to use the OF graph bindings to describe the connection between the CEC controller and the CEC "device" (which I assume in most cases will be a DT node for a physical connector). Or is this overkill ? > > Anyway, for meson I am fine with this solution. At least it prevents > > creating > > a non-functioning cec device. So for this series: > > > > Acked-by: Hans Verkuil > > Thanks, > > Applying this serie to drm-misc-next > > >>>> Jernej Skrabec (1): > >>>> drm/bridge/synopsys: dw-hdmi: Add an option to suppress loading CEC > >>>> driver > >>>> > >>>> Neil Armstrong (1): > >>>> drm/meson: dw-hdmi: disable DW-HDMI CEC sub-driver > >>>> > >>>> drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 2 +- > >>>> drivers/gpu/drm/meson/meson_dw_hdmi.c | 1 + > >>>> include/drm/bridge/dw_hdmi.h | 2 ++ > >>>> 3 files changed, 4 insertions(+), 1 deletion(-) -- Regards, Laurent Pinchart
Re: [PATCH v3 2/5] media: i2c: max9286: Use "maxim,gpio-poc" property
Hi Jacopo,
On Fri, Apr 16, 2021 at 09:43:07AM +0200, Jacopo Mondi wrote:
> On Thu, Apr 15, 2021 at 10:14:05PM +0300, Laurent Pinchart wrote:
> > > > > + /* GPIO values default to high */
> > > > > + priv->gpio_state = BIT(0) | BIT(1);
> > > >
> > > > Why is that ?
> > > >
> > > As the set/get functions of gpiochip use the gpio_state and I wanted
> > > to use the same functions for the internal gpio handling I used
> > > gpio_state in gpio_set(). My thinking was that in this way altering
> > > the gpio line would be visibile to gpio consumers... which we don't
> > > have as I won't register the gpio-controller :)
> >
> > My question was why they default to high here, when they default to low
> > when there's a gpio-controller property.
>
> Oh, got it now... the two output lines are high by default :)
> Why do you say "they default to low when there's a gpio-controller
> property" ? When does that requirement come from ?
My bad, I thought they were set to 0 in that case, but they're not.
How about moving this initialization from max9286_register_gpio() to the
beginning of this function, as it's shared by both cases ?
> > > > > + priv->regulator = NULL;
> > > >
> > > > As priv is initialized to 0, you can skip this.
> > >
> > > Yes, I liked it explicit as it is used as flag, but it is not
> > > required...
> > >
> > > > > +
> > > > > + return 0;
> > > > > + }
> > > > > +
> > > > > + ret = max9286_register_gpio(priv);
> > > > > + if (ret)
> > > > > + return ret;
> > > > > +
> > > > > + priv->regulator = devm_regulator_get(dev, "poc");
> > > > > + if (IS_ERR(priv->regulator)) {
> > > > > + if (PTR_ERR(priv->regulator) != -EPROBE_DEFER)
> > > > > + dev_err(dev, "Unable to get PoC regulator
> > > > > (%ld)\n",
> > > > > + PTR_ERR(priv->regulator));
> > > > > + return PTR_ERR(priv->regulator);
> > > > > + }
> > > > > +
> > > > > + return 0;
> > > > > +}
> > > > > +
> > > > > +static int max9286_poc_enable(struct max9286_priv *priv, bool enable)
> > > > > +{
> > > > > + int ret;
> > > > > +
> > > > > + /* If "poc-gpio" is used, toggle the line and do not use
> > > > > regulator. */
> > > > > + if (!priv->regulator)
> > > > > + return max9286_gpio_set(priv, priv->gpio_poc,
> > > > > + enable ^ priv->gpio_poc_flags);
> > > > > +
> > > > > + /* Otherwise PoC is controlled using a regulator. */
> > > > > + if (enable) {
> > > > > + ret = regulator_enable(priv->regulator);
> > > > > + if (ret < 0) {
> > > > > + dev_err(>client->dev, "Unable to turn PoC
> > > > > on\n");
> > > >
> > > > As error message when max9286_gpio_set() fails (at least in the enable
> > > > case) would be good too. Bonus points if there's a single dev_err()
> > > > call.
> > >
> > > I'll see how it looks like
> > >
> > > > > + return ret;
> > > > > + }
> > > > > +
> > > > > + return 0;
> > > > > + }
> > > > > +
> > > > > + return regulator_disable(priv->regulator);
> > > > > +}
> > > > > +
> > > > > static int max9286_init(struct device *dev)
> > > > > {
> > > > > struct max9286_priv *priv;
> > > > > @@ -1078,17 +1158,14 @@ static int max9286_init(struct device *dev)
> > > > > client = to_i2c_client(dev);
> > > > > priv = i2c_get_clientdata(client);
> > > > >
> > > > > - /* Enable the bus power. */
> > > > > - ret = regulator_enable(priv->regulator);
> > > > > - if (ret < 0) {
> > > > > - dev_err(>dev, "Unable to turn PoC on\n");
> >
Re: [PATCH 21/40] drm/xlnx/zynqmp_dp: Fix a little potential doc-rot
Hi Lee, Thank you for the patch. On Fri, Apr 16, 2021 at 03:37:06PM +0100, Lee Jones wrote: > Fixes the following W=1 kernel build warning(s): > > drivers/gpu/drm/xlnx/zynqmp_dp.c:806: warning: expecting prototype for > zynqmp_dp_link_train(). Prototype was for zynqmp_dp_train() instead > > Cc: Hyun Kwon > Cc: Laurent Pinchart > Cc: David Airlie > Cc: Daniel Vetter > Cc: Michal Simek > Cc: Philipp Zabel > Cc: dri-de...@lists.freedesktop.org > Cc: linux-arm-ker...@lists.infradead.org > Signed-off-by: Lee Jones Reviewed-by: Laurent Pinchart I assume you'll merge the whole series in one go through drm-misc. If that's not the case, please let me know and I'll take the zynqmp patches in my tree. > --- > drivers/gpu/drm/xlnx/zynqmp_dp.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/xlnx/zynqmp_dp.c > b/drivers/gpu/drm/xlnx/zynqmp_dp.c > index 59d1fb017da01..5ce96421acf40 100644 > --- a/drivers/gpu/drm/xlnx/zynqmp_dp.c > +++ b/drivers/gpu/drm/xlnx/zynqmp_dp.c > @@ -797,7 +797,7 @@ static int zynqmp_dp_link_train_ce(struct zynqmp_dp *dp) > } > > /** > - * zynqmp_dp_link_train - Train the link > + * zynqmp_dp_train - Train the link > * @dp: DisplayPort IP core structure > * > * Return: 0 if all trains are done successfully, or corresponding error > code. -- Regards, Laurent Pinchart
Re: [PATCH 20/40] drm/xlnx/zynqmp_disp: Fix incorrectly documented enum 'zynqmp_disp_id'
Hi Lee,
Thank you for the patch.
On Fri, Apr 16, 2021 at 03:37:05PM +0100, Lee Jones wrote:
> Fixes the following W=1 kernel build warning(s):
>
> drivers/gpu/drm/xlnx/zynqmp_disp.c:101: warning: expecting prototype for
> enum zynqmp_disp_id. Prototype was for enum zynqmp_disp_layer_id instead
>
> Cc: Hyun Kwon
> Cc: Laurent Pinchart
> Cc: David Airlie
> Cc: Daniel Vetter
> Cc: Michal Simek
> Cc: dri-de...@lists.freedesktop.org
> Cc: linux-arm-ker...@lists.infradead.org
> Signed-off-by: Lee Jones
Reviewed-by: Laurent Pinchart
> ---
> drivers/gpu/drm/xlnx/zynqmp_disp.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/xlnx/zynqmp_disp.c
> b/drivers/gpu/drm/xlnx/zynqmp_disp.c
> index 109d627968ac0..ca1161ec9e16f 100644
> --- a/drivers/gpu/drm/xlnx/zynqmp_disp.c
> +++ b/drivers/gpu/drm/xlnx/zynqmp_disp.c
> @@ -91,7 +91,7 @@ struct zynqmp_disp_format {
> };
>
> /**
> - * enum zynqmp_disp_id - Layer identifier
> + * enum zynqmp_disp_layer_id - Layer identifier
> * @ZYNQMP_DISP_LAYER_VID: Video layer
> * @ZYNQMP_DISP_LAYER_GFX: Graphics layer
> */
--
Regards,
Laurent Pinchart
Re: [PATCH 0/2] drm/bridge: dw-hdmi: disable loading of DW-HDMI CEC sub-driver
Hi Neil, On Fri, Apr 16, 2021 at 11:27:35AM +0200, Neil Armstrong wrote: > This adds DW-HDMI driver a glue option to disable loading of the CEC > sub-driver. > > On some SoCs, the CEC functionality is enabled in the IP config bits, but the > CEC bus is non-functional like on Amlogic SoCs, where the CEC config bit is > set > but the DW-HDMI CEC signal is not connected to a physical pin, leading to some > confusion when the DW-HDMI CEC controller can't communicate on the bus. If we can't trust the CEC config bit, would it be better to not use it at all, and instead let each platform glue logic tell whether to enable CEC or not ? > Jernej Skrabec (1): > drm/bridge/synopsys: dw-hdmi: Add an option to suppress loading CEC > driver > > Neil Armstrong (1): > drm/meson: dw-hdmi: disable DW-HDMI CEC sub-driver > > drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 2 +- > drivers/gpu/drm/meson/meson_dw_hdmi.c | 1 + > include/drm/bridge/dw_hdmi.h | 2 ++ > 3 files changed, 4 insertions(+), 1 deletion(-) > -- Regards, Laurent Pinchart
Re: [PATCH v4 3/7] media: i2c: max9286: Use "maxim,gpio-poc" property
t; + priv->gpio_poc, priv->gpio_poc_flags);
> + return -EINVAL;
> + }
> +
> + return 0;
> + }
> +
> + ret = max9286_register_gpio(priv);
> + if (ret)
> + return ret;
> +
> + priv->regulator = devm_regulator_get(dev, "poc");
> + if (IS_ERR(priv->regulator)) {
> + if (PTR_ERR(priv->regulator) != -EPROBE_DEFER)
> + dev_err(dev, "Unable to get PoC regulator (%ld)\n",
> + PTR_ERR(priv->regulator));
> + return PTR_ERR(priv->regulator);
> + }
> +
> + return 0;
> +}
> +
> +static int max9286_poc_enable(struct max9286_priv *priv, bool enable)
> +{
> + int ret;
> +
> + /* If "poc-gpio" is used, toggle the line and do not use regulator. */
> + if (enable)
> + ret = priv->regulator
> + ? regulator_enable(priv->regulator)
> + : max9286_gpio_set(priv, priv->gpio_poc,
> +enable ^ priv->gpio_poc_flags);
> + else
> + ret = priv->regulator
> + ? regulator_disable(priv->regulator)
> + : max9286_gpio_set(priv, priv->gpio_poc,
> +enable ^ priv->gpio_poc_flags);
> +
> + if (ret < 0)
> + dev_err(>client->dev, "Unable to turn PoC %s\n",
> + enable ? "on" : "off");
> +
> + return ret;
> +}
> +
> static int max9286_init(struct device *dev)
> {
> struct max9286_priv *priv;
> @@ -1078,17 +1158,14 @@ static int max9286_init(struct device *dev)
> client = to_i2c_client(dev);
> priv = i2c_get_clientdata(client);
>
> - /* Enable the bus power. */
> - ret = regulator_enable(priv->regulator);
> - if (ret < 0) {
> - dev_err(>dev, "Unable to turn PoC on\n");
> + ret = max9286_poc_enable(priv, true);
> + if (ret)
> return ret;
> - }
>
> ret = max9286_setup(priv);
> if (ret) {
> dev_err(dev, "Unable to setup max9286\n");
> - goto err_regulator;
> + goto err_poc_disable;
> }
>
> /*
> @@ -1098,7 +1175,7 @@ static int max9286_init(struct device *dev)
> ret = max9286_v4l2_register(priv);
> if (ret) {
> dev_err(dev, "Failed to register with V4L2\n");
> - goto err_regulator;
> + goto err_poc_disable;
> }
>
> ret = max9286_i2c_mux_init(priv);
> @@ -1114,8 +1191,8 @@ static int max9286_init(struct device *dev)
>
> err_v4l2_register:
> max9286_v4l2_unregister(priv);
> -err_regulator:
> - regulator_disable(priv->regulator);
> +err_poc_disable:
> + max9286_poc_enable(priv, false);
>
> return ret;
> }
> @@ -1286,20 +1363,10 @@ static int max9286_probe(struct i2c_client *client)
>*/
> max9286_configure_i2c(priv, false);
>
> - ret = max9286_register_gpio(priv);
> + ret = max9286_parse_gpios(priv);
> if (ret)
> goto err_powerdown;
>
> - priv->regulator = devm_regulator_get(>dev, "poc");
> - if (IS_ERR(priv->regulator)) {
> - if (PTR_ERR(priv->regulator) != -EPROBE_DEFER)
> - dev_err(>dev,
> - "Unable to get PoC regulator (%ld)\n",
> - PTR_ERR(priv->regulator));
> - ret = PTR_ERR(priv->regulator);
> - goto err_powerdown;
> - }
> -
> ret = max9286_parse_dt(priv);
> if (ret)
> goto err_powerdown;
> @@ -1326,7 +1393,7 @@ static int max9286_remove(struct i2c_client *client)
>
> max9286_v4l2_unregister(priv);
>
> - regulator_disable(priv->regulator);
> + max9286_poc_enable(priv, false);
>
> gpiod_set_value_cansleep(priv->gpiod_pwdn, 0);
>
--
Regards,
Laurent Pinchart
Re: [PATCH v3 2/5] media: i2c: max9286: Use "maxim,gpio-poc" property
Hi Jacopo,
On Thu, Apr 15, 2021 at 08:58:48AM +0200, Jacopo Mondi wrote:
> On Thu, Apr 15, 2021 at 03:00:56AM +0300, Laurent Pinchart wrote:
> > On Wed, Apr 14, 2021 at 03:51:25PM +0200, Jacopo Mondi wrote:
> > > The 'maxim,gpio-poc' property is used when the remote camera
> > > power-over-coax is controlled by one of the MAX9286 gpio lines,
> > > to instruct the driver about which line to use and what the line
> > > polarity is.
> > >
> > > Add to the max9286 driver support for parsing the newly introduce
> >
> > s/introduce/introduced/
> >
> > > property and use it if available in place of the usual supply, as it is
> > > not possible to establish one as consumer of the max9286 gpio
> > > controller.
> > >
> > > If the new property is present, no gpio controller is registered and
> > > 'poc-supply' is ignored.
> > >
> > > In order to maximize code re-use, break out the max9286 gpio handling
> > > function so that they can be used by the gpio controller through the
> > > gpio-consumer API, or directly by the driver code.
> > >
> > > Wrap the power up and power down routines to their own function to
> > > be able to use either the gpio line directly or the supply. This will
> > > make it easier to control the remote camera power at run time.
> >
> > I would have split the patch in two, with a first patch that refactors
> > the code, and a second one that extends it, but that's no big deal.
> >
> > > Signed-off-by: Jacopo Mondi
> > > ---
> > > drivers/media/i2c/max9286.c | 125 +++-
> > > 1 file changed, 96 insertions(+), 29 deletions(-)
> > >
> > > diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
> > > index 6fd4d59fcc72..0c125f7b3d9b 100644
> > > --- a/drivers/media/i2c/max9286.c
> > > +++ b/drivers/media/i2c/max9286.c
> > > @@ -15,6 +15,7 @@
> > > #include
> > > #include
> > > #include
> > > +#include
> > > #include
> > > #include
> > > #include
> > > @@ -165,6 +166,9 @@ struct max9286_priv {
> > >
> > > u32 reverse_channel_mv;
> > >
> > > + u32 gpio_poc;
> > > + u32 gpio_poc_flags;
> > > +
> > > struct v4l2_ctrl_handler ctrls;
> > > struct v4l2_ctrl *pixelrate;
> > >
> > > @@ -1022,20 +1026,27 @@ static int max9286_setup(struct max9286_priv
> > > *priv)
> > > return 0;
> > > }
> > >
> > > -static void max9286_gpio_set(struct gpio_chip *chip,
> > > - unsigned int offset, int value)
> > > +static int max9286_gpio_set(struct max9286_priv *priv, unsigned int
> > > offset,
> > > + int value)
> > > {
> > > - struct max9286_priv *priv = gpiochip_get_data(chip);
> > > -
> > > if (value)
> > > priv->gpio_state |= BIT(offset);
> > > else
> > > priv->gpio_state &= ~BIT(offset);
> > >
> > > - max9286_write(priv, 0x0f, MAX9286_0X0F_RESERVED | priv->gpio_state);
> > > + return max9286_write(priv, 0x0f,
> > > + MAX9286_0X0F_RESERVED | priv->gpio_state);
> > > +}
> > > +
> > > +static void max9286_gpiochip_set(struct gpio_chip *chip,
> > > + unsigned int offset, int value)
> > > +{
> > > + struct max9286_priv *priv = gpiochip_get_data(chip);
> > > +
> > > + max9286_gpio_set(priv, offset, value);
> > > }
> > >
> > > -static int max9286_gpio_get(struct gpio_chip *chip, unsigned int offset)
> > > +static int max9286_gpiochip_get(struct gpio_chip *chip, unsigned int
> > > offset)
> > > {
> > > struct max9286_priv *priv = gpiochip_get_data(chip);
> > >
> > > @@ -1055,8 +1066,8 @@ static int max9286_register_gpio(struct
> > > max9286_priv *priv)
> > > gpio->of_node = dev->of_node;
> > > gpio->ngpio = 2;
> > > gpio->base = -1;
> > > - gpio->set = max9286_gpio_set;
> > > - gpio->get = max9286_gpio_get;
> > > + gpio->set = max9286_gpiochip_set;
> > > + gpio->get = max9286_gpiochip_get;
> > > gpio->can_sleep = true;
> > >
> > > /* GPIO values default to high */
> > > @@ -1069,6 +1080,75 @@ static int max9286_register_gpio(struct
> > > max9286_priv *priv)
> >
Re: [PATCH v4 4/7] arm64: dts: renesas: r8a77970: Add csi40 port@0
On Thu, Apr 15, 2021 at 06:47:48PM +0200, Geert Uytterhoeven wrote:
> On Thu, Apr 15, 2021 at 4:47 PM Laurent Pinchart wrote:
> > On Thu, Apr 15, 2021 at 02:25:59PM +0200, Jacopo Mondi wrote:
> > > Declare port@0 in the csi40 device node and leave it un-connected.
> > > Each board .dts file will connect the port as it requires.
> > >
> > > Signed-off-by: Jacopo Mondi
> >
> > The port exists at the hardware level, so including it here sounds good.
> > The DT binding even makes the port mandatory :-)
>
> But the latter is subject to change?
>
> [PATCH] media: dt-bindings: media: renesas,csi2: Node port@0 is not mandatory
> https://lore.kernel.org/linux-devicetree/20210413155346.2471776-1-niklas.soderlund+rene...@ragnatech.se/
I've replied to that patch, it's not a good idea.
> > > --- a/arch/arm64/boot/dts/renesas/r8a77970.dtsi
> > > +++ b/arch/arm64/boot/dts/renesas/r8a77970.dtsi
> > > @@ -1106,6 +1106,10 @@ ports {
> > > #address-cells = <1>;
> > > #size-cells = <0>;
> > >
> > > + port@0 {
> > > + reg = <0>;
> > > + };
> > > +
> > > port@1 {
> > > #address-cells = <1>;
> > > #size-cells = <0>;
--
Regards,
Laurent Pinchart
Re: [PATCH v4 4/7] arm64: dts: renesas: r8a77970: Add csi40 port@0
Hi Jacopo,
Thank you for the patch.
On Thu, Apr 15, 2021 at 02:25:59PM +0200, Jacopo Mondi wrote:
> Declare port@0 in the csi40 device node and leave it un-connected.
> Each board .dts file will connect the port as it requires.
>
> Signed-off-by: Jacopo Mondi
The port exists at the hardware level, so including it here sounds good.
The DT binding even makes the port mandatory :-)
Reviewed-by: Laurent Pinchart
> ---
> arch/arm64/boot/dts/renesas/r8a77970.dtsi | 4
> 1 file changed, 4 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/renesas/r8a77970.dtsi
> b/arch/arm64/boot/dts/renesas/r8a77970.dtsi
> index 5a5d5649332a..e8f6352c3665 100644
> --- a/arch/arm64/boot/dts/renesas/r8a77970.dtsi
> +++ b/arch/arm64/boot/dts/renesas/r8a77970.dtsi
> @@ -1106,6 +1106,10 @@ ports {
> #address-cells = <1>;
> #size-cells = <0>;
>
> + port@0 {
> + reg = <0>;
> + };
> +
> port@1 {
> #address-cells = <1>;
> #size-cells = <0>;
--
Regards,
Laurent Pinchart
Re: [PATCH v4 1/7] dt-bindings: media: max9286: Re-indent example
Hi Jacopo,
Thank you for the patch.
On Thu, Apr 15, 2021 at 02:25:56PM +0200, Jacopo Mondi wrote:
> The dt-bindings examples are usually indented with 4 spaces.
>
> The maxim,max9286 schema has the example indented with only
> 2 spaces, re-indent it.
>
> Cosmetic change only.
>
> Signed-off-by: Jacopo Mondi
Tested by applying and verifying that `git show -b` shows an empty diff.
Reviewed-by: Laurent Pinchart
> ---
> .../bindings/media/i2c/maxim,max9286.yaml | 214 +-
> 1 file changed, 107 insertions(+), 107 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
> b/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
> index ee16102fdfe7..0e7162998b77 100644
> --- a/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
> +++ b/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
> @@ -191,140 +191,140 @@ examples:
> #include
>
> i2c@e66d8000 {
> - #address-cells = <1>;
> - #size-cells = <0>;
> +#address-cells = <1>;
> +#size-cells = <0>;
>
> - reg = <0 0xe66d8000>;
> +reg = <0 0xe66d8000>;
>
> - gmsl-deserializer@2c {
> -compatible = "maxim,max9286";
> -reg = <0x2c>;
> -poc-supply = <_poc_12v>;
> -enable-gpios = < 13 GPIO_ACTIVE_HIGH>;
> +gmsl-deserializer@2c {
> +compatible = "maxim,max9286";
> +reg = <0x2c>;
> +poc-supply = <_poc_12v>;
> +enable-gpios = < 13 GPIO_ACTIVE_HIGH>;
>
> -gpio-controller;
> -#gpio-cells = <2>;
> +gpio-controller;
> +#gpio-cells = <2>;
>
> -maxim,reverse-channel-microvolt = <17>;
> +maxim,reverse-channel-microvolt = <17>;
>
> -ports {
> - #address-cells = <1>;
> - #size-cells = <0>;
> +ports {
> +#address-cells = <1>;
> +#size-cells = <0>;
>
> - port@0 {
> -reg = <0>;
> +port@0 {
> +reg = <0>;
>
> -max9286_in0: endpoint {
> - remote-endpoint = <_out0>;
> -};
> - };
> -
> - port@1 {
> -reg = <1>;
> -
> -max9286_in1: endpoint {
> - remote-endpoint = <_out1>;
> -};
> - };
> -
> - port@2 {
> -reg = <2>;
> -
> -max9286_in2: endpoint {
> - remote-endpoint = <_out2>;
> -};
> - };
> +max9286_in0: endpoint {
> +remote-endpoint = <_out0>;
> +};
> +};
>
> - port@3 {
> -reg = <3>;
> +port@1 {
> +reg = <1>;
>
> -max9286_in3: endpoint {
> - remote-endpoint = <_out3>;
> -};
> - };
> +max9286_in1: endpoint {
> +remote-endpoint = <_out1>;
> +};
> +};
>
> - port@4 {
> -reg = <4>;
> +port@2 {
> +reg = <2>;
>
> -max9286_out: endpoint {
> - data-lanes = <1 2 3 4>;
> - remote-endpoint = <_in>;
> -};
> - };
> -};
> +max9286_in2: endpoint {
> +remote-endpoint = <_out2>;
> +};
> +};
>
> -i2c-mux {
> - #address-cells = <1>;
> - #size-cells = <0>;
> +port@3 {
> +reg = <3>;
>
> - i2c@0 {
> -#address-cells = <1>;
> -#size-cells = <0>;
> -reg = <0>;
> +max9286_in3: endpoint {
> +remote-endpoint = <_out3>;
> +};
> +};
>
> -camera@51 {
> - compatible = "imi,rdacm20";
> - reg = <0x51>, <0x61>;
> +port@4 {
> +reg = <4>;
>
> - port {
> -rdacm20_out0: endpoint {
> - remote-end
Re: [PATCH v3 01/12] drm/bridge: Fix the stop condition of drm_bridge_chain_pre_enable()
Hi Doug,
On Wed, Apr 14, 2021 at 06:19:13PM -0700, Doug Anderson wrote:
> On Sun, Apr 4, 2021 at 5:50 PM Laurent Pinchart wrote:
> > On Fri, Apr 02, 2021 at 03:28:35PM -0700, Douglas Anderson wrote:
> > > The drm_bridge_chain_pre_enable() is not the proper opposite of
> > > drm_bridge_chain_post_disable(). It continues along the chain to
> > > _before_ the starting bridge. Let's fix that.
> > >
> > > Fixes: 05193dc38197 ("drm/bridge: Make the bridge chain a double-linked
> > > list")
> > > Signed-off-by: Douglas Anderson
> > > Reviewed-by: Andrzej Hajda
> > > ---
> > >
> > > (no changes since v1)
> > >
> > > drivers/gpu/drm/drm_bridge.c | 3 +++
> > > 1 file changed, 3 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> > > index 64f0effb52ac..044acd07c153 100644
> > > --- a/drivers/gpu/drm/drm_bridge.c
> > > +++ b/drivers/gpu/drm/drm_bridge.c
> > > @@ -522,6 +522,9 @@ void drm_bridge_chain_pre_enable(struct drm_bridge
> > > *bridge)
> > > list_for_each_entry_reverse(iter, >bridge_chain,
> > > chain_node) {
> > > if (iter->funcs->pre_enable)
> > > iter->funcs->pre_enable(iter);
> > > +
> > > + if (iter == bridge)
> > > + break;
> >
> > This looks good as it matches drm_atomic_bridge_chain_disable().
> >
> > Reviewed-by: Laurent Pinchart
>
> Thanks for your review here and several of the other patches. Can you
> suggest any plan for getting them landed? It would at least be nice to
> get the non-controversial ones landed.
Do you have commit access to drm-misc ? If not, given your
contributions, I think you qualify for it.
> > I'm curious though, given that the bridge passed to the function should
> > be the one closest to the encoder, does this make a difference ?
>
> Yes, that's how I discovered it originally. Let's see. So if I don't
> have this patch but have the rest of the series then I get a splat at
> bootup. This shows that dsi_mgr_bridge_pre_enable() must be "earlier"
> in the chain than my bridge chip. Here's the splat:
Right, I think it's caused by a later patch in the series calling this
function with a different bridge than the one closest to the encoder.
> msm_dsi_host_get_phy_clk_req: unable to calc clk rate, -22
> [ cut here ]
> disp_cc_mdss_ahb_clk status stuck at 'off'
> WARNING: CPU: 7 PID: 404 at drivers/clk/qcom/clk-branch.c:92
> clk_branch_toggle+0x194/0x280
> Modules linked in: joydev
> CPU: 7 PID: 404 Comm: frecon Tainted: GB 5.12.0-rc3-lockdep+
> #2
> Hardware name: Google Lazor (rev1 - 2) with LTE (DT)
> pstate: 60400089 (nZCv daIf +PAN -UAO -TCO BTYPE=--)
> pc : clk_branch_toggle+0x194/0x280
> lr : clk_branch_toggle+0x190/0x280
> ...
> Call trace:
> clk_branch_toggle+0x194/0x280
> clk_branch2_enable+0x28/0x34
> clk_core_enable+0x2f4/0x6b4
> clk_enable+0x54/0x74
> dsi_phy_enable_resource+0x80/0xd8
> msm_dsi_phy_enable+0xa8/0x4a8
> enable_phy+0x9c/0xf4
> dsi_mgr_bridge_pre_enable+0x23c/0x4b0
> drm_bridge_chain_pre_enable+0xac/0xe4
> ti_sn_bridge_connector_get_modes+0x134/0x1b8
> drm_helper_probe_single_connector_modes+0x49c/0x1358
> drm_mode_getconnector+0x460/0xe98
> drm_ioctl_kernel+0x144/0x228
> drm_ioctl+0x418/0x7cc
> drm_compat_ioctl+0x1bc/0x230
> __arm64_compat_sys_ioctl+0x14c/0x188
> el0_svc_common+0x128/0x23c
> do_el0_svc_compat+0x50/0x60
> el0_svc_compat+0x24/0x34
> el0_sync_compat_handler+0xc0/0xf0
> el0_sync_compat+0x174/0x180
--
Regards,
Laurent Pinchart
Re: [PATCH v3 12/12] drm/panel: panel-simple: Use runtime pm to avoid excessive unprepare / prepare
Hi Doug, On Wed, Apr 14, 2021 at 06:22:57PM -0700, Doug Anderson wrote: > On Wed, Apr 14, 2021 at 5:58 PM Laurent Pinchart wrote: > > On Fri, Apr 02, 2021 at 03:28:46PM -0700, Douglas Anderson wrote: > > > Unpreparing and re-preparing a panel can be a really heavy > > > operation. Panels datasheets often specify something on the order of > > > 500ms as the delay you should insert after turning off the panel > > > before turning it on again. In addition, turning on a panel can have > > > delays on the order of 100ms - 200ms before the panel will assert HPD > > > (AKA "panel ready"). The above means that we should avoid turning a > > > panel off if we're going to turn it on again shortly. > > > > > > The above becomes a problem when we want to read the EDID of a > > > panel. The way that ordering works is that userspace wants to read the > > > EDID of the panel _before_ fully enabling it so that it can set the > > > initial mode correctly. However, we can't read the EDID until we power > > > it up. This leads to code that does this dance (like > > > ps8640_bridge_get_edid()): > > > > > > 1. When userspace requests EDID / the panel modes (through an ioctl), > > >we power on the panel just enough to read the EDID and then power > > >it off. > > > 2. Userspace then turns the panel on. > > > > > > There's likely not much time between step #1 and #2 and so we want to > > > avoid powering the panel off and on again between those two steps. > > > > > > Let's use Runtime PM to help us. We'll move the existing prepare() and > > > unprepare() to be runtime resume() and runtime suspend(). Now when we > > > want to prepare() or unprepare() we just increment or decrement the > > > refcount. We'll default to a 1 second autosuspend delay which seems > > > sane given the typical delays we see for panels. > > > > > > A few notes: > > > - It seems the existing unprepare() and prepare() are defined to be > > > no-ops if called extra times. We'll preserve that behavior. > > > > The prepare and unprepare calls are supposed to be balanced, which > > should allow us to drop this check. Do you have a reason to suspect that > > it may not be the case ? > > No, it was just code inspection. The old code definitely made an > effort to make enable of an already enabled panel a no-op and disable > of an already disabled panel a no-op. This is even before my > (somewhat) recent patch to make things timing based, though I did > touch the code. > > Can I maybe suggest that getting rid of the extra check should be a > separate patch after this one? Then if it breaks someone it's easy to > just revert that one and we can still keep the runtime pm? Sounds good to me. > > > - This is a slight change in the ABI of simple panel. If something was > > > absolutely relying on the unprepare() to happen instantly that > > > simply won't be the case anymore. I'm not aware of anyone relying on > > > that behavior, but if there is someone then we'll need to figure out > > > how to enable (or disable) this new delayed behavior selectively. > > > - In order for this to work we now have a hard dependency on > > > "PM". From memory this is a legit thing to assume these days and we > > > don't have to find some fallback to keep working if someone wants to > > > build their system without "PM". > > > > Sounds fine to me. > > > > The code looks good to me. Possibly with the prepared check removed, > > > > Reviewed-by: Laurent Pinchart -- Regards, Laurent Pinchart
Re: [PATCH v3 12/12] drm/panel: panel-simple: Use runtime pm to avoid excessive unprepare / prepare
Hi Doug,
Thank you for the patch.
On Fri, Apr 02, 2021 at 03:28:46PM -0700, Douglas Anderson wrote:
> Unpreparing and re-preparing a panel can be a really heavy
> operation. Panels datasheets often specify something on the order of
> 500ms as the delay you should insert after turning off the panel
> before turning it on again. In addition, turning on a panel can have
> delays on the order of 100ms - 200ms before the panel will assert HPD
> (AKA "panel ready"). The above means that we should avoid turning a
> panel off if we're going to turn it on again shortly.
>
> The above becomes a problem when we want to read the EDID of a
> panel. The way that ordering works is that userspace wants to read the
> EDID of the panel _before_ fully enabling it so that it can set the
> initial mode correctly. However, we can't read the EDID until we power
> it up. This leads to code that does this dance (like
> ps8640_bridge_get_edid()):
>
> 1. When userspace requests EDID / the panel modes (through an ioctl),
>we power on the panel just enough to read the EDID and then power
>it off.
> 2. Userspace then turns the panel on.
>
> There's likely not much time between step #1 and #2 and so we want to
> avoid powering the panel off and on again between those two steps.
>
> Let's use Runtime PM to help us. We'll move the existing prepare() and
> unprepare() to be runtime resume() and runtime suspend(). Now when we
> want to prepare() or unprepare() we just increment or decrement the
> refcount. We'll default to a 1 second autosuspend delay which seems
> sane given the typical delays we see for panels.
>
> A few notes:
> - It seems the existing unprepare() and prepare() are defined to be
> no-ops if called extra times. We'll preserve that behavior.
The prepare and unprepare calls are supposed to be balanced, which
should allow us to drop this check. Do you have a reason to suspect that
it may not be the case ?
> - This is a slight change in the ABI of simple panel. If something was
> absolutely relying on the unprepare() to happen instantly that
> simply won't be the case anymore. I'm not aware of anyone relying on
> that behavior, but if there is someone then we'll need to figure out
> how to enable (or disable) this new delayed behavior selectively.
> - In order for this to work we now have a hard dependency on
> "PM". From memory this is a legit thing to assume these days and we
> don't have to find some fallback to keep working if someone wants to
> build their system without "PM".
Sounds fine to me.
The code looks good to me. Possibly with the prepared check removed,
Reviewed-by: Laurent Pinchart
> Signed-off-by: Douglas Anderson
> ---
>
> (no changes since v1)
>
> drivers/gpu/drm/panel/Kconfig| 1 +
> drivers/gpu/drm/panel/panel-simple.c | 93 +---
> 2 files changed, 73 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig
> index 4894913936e9..ef87d92cdf49 100644
> --- a/drivers/gpu/drm/panel/Kconfig
> +++ b/drivers/gpu/drm/panel/Kconfig
> @@ -80,6 +80,7 @@ config DRM_PANEL_SIMPLE
> tristate "support for simple panels"
> depends on OF
> depends on BACKLIGHT_CLASS_DEVICE
> + depends on PM
> select VIDEOMODE_HELPERS
> help
> DRM panel driver for dumb panels that need at most a regulator and
> diff --git a/drivers/gpu/drm/panel/panel-simple.c
> b/drivers/gpu/drm/panel/panel-simple.c
> index be312b5c04dd..6b22872b3281 100644
> --- a/drivers/gpu/drm/panel/panel-simple.c
> +++ b/drivers/gpu/drm/panel/panel-simple.c
> @@ -27,6 +27,7 @@
> #include
> #include
> #include
> +#include
> #include
>
> #include
> @@ -175,6 +176,8 @@ struct panel_simple {
> bool enabled;
> bool no_hpd;
>
> + bool prepared;
> +
> ktime_t prepared_time;
> ktime_t unprepared_time;
>
> @@ -334,19 +337,31 @@ static int panel_simple_disable(struct drm_panel *panel)
> return 0;
> }
>
> +static int panel_simple_suspend(struct device *dev)
> +{
> + struct panel_simple *p = dev_get_drvdata(dev);
> +
> + gpiod_set_value_cansleep(p->enable_gpio, 0);
> + regulator_disable(p->supply);
> + p->unprepared_time = ktime_get();
> +
> + return 0;
> +}
> +
> static int panel_simple_unprepare(struct drm_panel *panel)
> {
> struct panel_simple *p = to_panel_simple(panel);
> + int ret;
>
> - if (p->prepared_time == 0)
> + /* Unpreparing when already unprepared is a no-op */
> + if (!p->prepared)
> return 0;
>
> -
Re: [PATCH v2 11/14] drm/bridge: ti-sn65dsi86: Power things properly for reading the EDID
e connected to the DDC/AUX port to read the EDID and provide modes. The panel driver won't be able to handle it on its own. > >> - ddc driver on i2c request should power up the panel - seems also correct, > > > > Right, so I could put the > > "drm_bridge_chain_pre_enable(>bridge)" into the > > ti_sn_aux_transfer() function. I talked about that a little bit "after > > the cut" in my post where I said: > > > >> - If everyone loves the "runtime PM" in the panel driver then we > >> could, in theory, put the pre-enable chaining straight in the "aux > >> transfer" function. > > > > The reason for the dependence on "runtime PM" in the panel driver is > > that we are doing DDC over AUX and it breaks the EDID reading into > > lots of chunks so if we did the powering up and powering down there it > > would be crazy slow without the delayed poweroff. > > OK, it resembles to me DSI-controlled panel - to query/configure panel > panel driver asks DSI-host to transfer some bytes to the panel and/or > back via DSI-bus. > > In case of eDP panels we could do similar thing to read edid - we call > drm_panel_get_modes - it calls drm_panel_funcs.get_modes callback and it > decides (based on DT) if it should fill modes according to hardcoded > info into the driver or to ask the physical panel via DP-controller - > this way all the players (the panel, AUX/DDC device) will know what to > power-up. > > I guess there is missing pieces - there is no DP bus :), I am not sure > if there is straight way to access panel's aux/ddc from the panel > driver, maybe somehow via drm_connector ??? If the SN65DSI86 has to call drm_panel_get_modes(), which will then call back into the SN65DSI86 driver to perform the EDID read, it seems to me that the panel driver shouldn't be involved at all. DRM bridges have "recently" gained new operations to retrieve EDID, and there's a helper (drm_bridge_connector) that creates a connector for a chain of bridges, delegating connector operations to the appropriate bridge in the chain. This seems a better way forward to me (but I'm biased, as I've authored that code :-)). > Of course this only my idea - to be discussed with others. -- Regards, Laurent Pinchart
Re: [PATCH v3 2/5] media: i2c: max9286: Use "maxim,gpio-poc" property
; + dev_err(dev, "Invalid 'gpio-poc': (%u %u)\n",
> + priv->gpio_poc, priv->gpio_poc_flags);
> + return -EINVAL;
> + }
> +
> + /* GPIO values default to high */
> + priv->gpio_state = BIT(0) | BIT(1);
Why is that ?
> + priv->regulator = NULL;
As priv is initialized to 0, you can skip this.
> +
> + return 0;
> + }
> +
> + ret = max9286_register_gpio(priv);
> + if (ret)
> + return ret;
> +
> + priv->regulator = devm_regulator_get(dev, "poc");
> + if (IS_ERR(priv->regulator)) {
> + if (PTR_ERR(priv->regulator) != -EPROBE_DEFER)
> + dev_err(dev, "Unable to get PoC regulator (%ld)\n",
> + PTR_ERR(priv->regulator));
> + return PTR_ERR(priv->regulator);
> + }
> +
> + return 0;
> +}
> +
> +static int max9286_poc_enable(struct max9286_priv *priv, bool enable)
> +{
> + int ret;
> +
> + /* If "poc-gpio" is used, toggle the line and do not use regulator. */
> + if (!priv->regulator)
> + return max9286_gpio_set(priv, priv->gpio_poc,
> + enable ^ priv->gpio_poc_flags);
> +
> + /* Otherwise PoC is controlled using a regulator. */
> + if (enable) {
> + ret = regulator_enable(priv->regulator);
> + if (ret < 0) {
> + dev_err(>client->dev, "Unable to turn PoC on\n");
As error message when max9286_gpio_set() fails (at least in the enable
case) would be good too. Bonus points if there's a single dev_err()
call.
> + return ret;
> + }
> +
> + return 0;
> + }
> +
> + return regulator_disable(priv->regulator);
> +}
> +
> static int max9286_init(struct device *dev)
> {
> struct max9286_priv *priv;
> @@ -1078,17 +1158,14 @@ static int max9286_init(struct device *dev)
> client = to_i2c_client(dev);
> priv = i2c_get_clientdata(client);
>
> - /* Enable the bus power. */
> - ret = regulator_enable(priv->regulator);
> - if (ret < 0) {
> - dev_err(>dev, "Unable to turn PoC on\n");
> + ret = max9286_poc_enable(priv, true);
> + if (ret)
> return ret;
> - }
>
> ret = max9286_setup(priv);
> if (ret) {
> dev_err(dev, "Unable to setup max9286\n");
> - goto err_regulator;
> + goto err_poc_disable;
> }
>
> /*
> @@ -1098,7 +1175,7 @@ static int max9286_init(struct device *dev)
> ret = max9286_v4l2_register(priv);
> if (ret) {
> dev_err(dev, "Failed to register with V4L2\n");
> - goto err_regulator;
> + goto err_poc_disable;
> }
>
> ret = max9286_i2c_mux_init(priv);
> @@ -1114,8 +1191,8 @@ static int max9286_init(struct device *dev)
>
> err_v4l2_register:
> max9286_v4l2_unregister(priv);
> -err_regulator:
> - regulator_disable(priv->regulator);
> +err_poc_disable:
> + max9286_poc_enable(priv, false);
>
> return ret;
> }
> @@ -1286,20 +1363,10 @@ static int max9286_probe(struct i2c_client *client)
>*/
> max9286_configure_i2c(priv, false);
>
> - ret = max9286_register_gpio(priv);
> + ret = max9286_parse_gpios(priv);
> if (ret)
> goto err_powerdown;
>
> - priv->regulator = devm_regulator_get(>dev, "poc");
> - if (IS_ERR(priv->regulator)) {
> - if (PTR_ERR(priv->regulator) != -EPROBE_DEFER)
> - dev_err(>dev,
> - "Unable to get PoC regulator (%ld)\n",
> - PTR_ERR(priv->regulator));
> - ret = PTR_ERR(priv->regulator);
> - goto err_powerdown;
> - }
> -
> ret = max9286_parse_dt(priv);
> if (ret)
> goto err_powerdown;
> @@ -1326,7 +1393,7 @@ static int max9286_remove(struct i2c_client *client)
>
> max9286_v4l2_unregister(priv);
>
> - regulator_disable(priv->regulator);
> + max9286_poc_enable(priv, false);
>
> gpiod_set_value_cansleep(priv->gpiod_pwdn, 0);
>
--
Regards,
Laurent Pinchart
Re: [PATCH v3 4/5] arm64: dts: renesas: eagle: Add GMSL .dtsi
+ };
> +#endif
> +
> +#ifdef EAGLE_USE_CAMERA_1
> + i2c@1 {
> + status = "okay";
> +
> + camera@52 {
> + compatible = EAGLE_CAMERA_MODEL;
> + reg = <0x52>, <0x62>;
> +
> + port {
> + fakra_con1: endpoint {
> + remote-endpoint =
> <_in1>;
> + };
> + };
> + };
> + };
> +#endif
> +
> +#ifdef EAGLE_USE_CAMERA_2
> + i2c@2 {
> + status = "okay";
> +
> + camera@53 {
> + compatible = EAGLE_CAMERA_MODEL;
> + reg = <0x53>, <0x63>;
> +
> + port {
> + fakra_con2: endpoint {
> + remote-endpoint =
> <_in2>;
> + };
> + };
> + };
> + };
> +#endif
> +
> +#ifdef EAGLE_USE_CAMERA_3
> + i2c@3 {
> + status = "okay";
> +
> + camera@54 {
> + compatible = EAGLE_CAMERA_MODEL;
> + reg = <0x54>, <0x64>;
> +
> + port {
> + fakra_con3: endpoint {
> + remote-endpoint =
> <_in3>;
> + };
> + };
> + };
> + };
> +#endif
> + };
> +};
> +#endif
--
Regards,
Laurent Pinchart
Re: [PATCH v3 5/5] arm64: dts: renesas: eagle: Include eagle-gmsl
Hi Jacopo,
Thank you for the patch.
On Wed, Apr 14, 2021 at 03:51:28PM +0200, Jacopo Mondi wrote:
> From: Kieran Bingham
>
> Include the eagle-gmsl.dtsi to enable GMSL camera support on the
> Eagle-V3M platform.
This is useful for quick testing, but as I don't expect everybody to
have cameras connected to the Eagle board, it should really be handled
as an overlay.
> Signed-off-by: Kieran Bingham
> Signed-off-by: Jacopo Mondi
> ---
> arch/arm64/boot/dts/renesas/r8a77970-eagle.dts | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/renesas/r8a77970-eagle.dts
> b/arch/arm64/boot/dts/renesas/r8a77970-eagle.dts
> index d2b6368d1e72..9b8dfb5132fb 100644
> --- a/arch/arm64/boot/dts/renesas/r8a77970-eagle.dts
> +++ b/arch/arm64/boot/dts/renesas/r8a77970-eagle.dts
> @@ -391,3 +391,6 @@ {
>
> status = "okay";
> };
> +
> +/* FAKRA Overlay */
> +#include "eagle-gmsl.dtsi"
--
Regards,
Laurent Pinchart
Re: [PATCH v3 3/5] arm64: dts: renesas: eagle: Enable MAX9286
Hi Jacopo and Kieran,
Thank you for the patch.
On Wed, Apr 14, 2021 at 03:51:26PM +0200, Jacopo Mondi wrote:
> From: Kieran Bingham
>
> Enable the MAX9286 GMSL deserializer on the Eagle-V3M board.
>
> Connected cameras should be defined in a device-tree overlay or included
> after these definitions.
>
> Signed-off-by: Kieran Bingham
> Signed-off-by: Jacopo Mondi
Reviewed-by: Laurent Pinchart
> ---
> .../arm64/boot/dts/renesas/r8a77970-eagle.dts | 119 ++
> 1 file changed, 119 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/renesas/r8a77970-eagle.dts
> b/arch/arm64/boot/dts/renesas/r8a77970-eagle.dts
> index 874a7fc2730b..d2b6368d1e72 100644
> --- a/arch/arm64/boot/dts/renesas/r8a77970-eagle.dts
> +++ b/arch/arm64/boot/dts/renesas/r8a77970-eagle.dts
> @@ -6,6 +6,8 @@
> * Copyright (C) 2017 Cogent Embedded, Inc.
> */
>
> +#include
> +
> /dts-v1/;
> #include "r8a77970.dtsi"
>
> @@ -188,6 +190,11 @@ i2c0_pins: i2c0 {
> function = "i2c0";
> };
>
> + i2c3_pins: i2c3 {
> + groups = "i2c3_a";
> + function = "i2c3";
> + };
> +
> qspi0_pins: qspi0 {
> groups = "qspi0_ctrl", "qspi0_data4";
> function = "qspi0";
> @@ -266,6 +273,118 @@ {
> status = "okay";
> };
>
> + {
> + status = "okay";
> +
> + ports {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + port@0 {
> + reg = <0>;
> +
> + csi40_in: endpoint {
> + clock-lanes = <0>;
> + data-lanes = <1 2 3 4>;
> + remote-endpoint = <_out0>;
> + };
> + };
> + };
> +};
> +
> + {
> + pinctrl-0 = <_pins>;
> + pinctrl-names = "default";
> +
> + status = "okay";
> + clock-frequency = <40>;
> +
> + gmsl: gmsl-deserializer@48 {
> + compatible = "maxim,max9286";
> + reg = <0x48>;
> +
> + maxim,gpio-poc = <0 GPIO_ACTIVE_LOW>;
> +
> + /* eagle-pca9654-max9286-pwdn */
> + enable-gpios = <_expander 0 GPIO_ACTIVE_HIGH>;
> +
> + ports {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + port@0 {
> + reg = <0>;
> + max9286_in0: endpoint {
> + };
> + };
> +
> + port@1 {
> + reg = <1>;
> + max9286_in1: endpoint {
> + };
> + };
> +
> + port@2 {
> + reg = <2>;
> + max9286_in2: endpoint {
> + };
> + };
> +
> + port@3 {
> + reg = <3>;
> + max9286_in3: endpoint {
> + };
> + };
> +
> + port@4 {
> + reg = <4>;
> + max9286_out0: endpoint {
> + clock-lanes = <0>;
> + data-lanes = <1 2 3 4>;
> + remote-endpoint = <_in>;
> + };
> + };
> + };
> +
> + i2c-mux {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + i2c@0 {
> + #address-cells = <1>;
> + #size-cells = <0>;
> + reg = <0>;
> +
> + status = "disabled";
> + };
> +
> + i2c@1 {
> + #address-cells = <1>;
> + #size-cells = <0>;
> + reg = <1>;
> +
> + status = "disabled";
> + };
> +
> + i2c@2 {
> + #address-cells = <1>;
> + #size-cells = <0>;
> + reg = <2>;
> +
> + status = "disabled";
> + };
> +
> + i2c@3 {
> + #address-cells = <1>;
> + #size-cells = <0>;
> + reg = <3>;
> +
> + status = "disabled";
> + };
> + };
> + };
> +};
> +
> {
> pinctrl-0 = <_pins>;
> pinctrl-names = "default";
--
Regards,
Laurent Pinchart
Re: [PATCH v3 1/5] dt-bindings: media: max9286: Define 'maxim,gpio-poc'
Hi Jacopo,
Thank you for the patch.
On Wed, Apr 14, 2021 at 03:51:24PM +0200, Jacopo Mondi wrote:
> Define a new vendor property in the maxim,max9286 binding schema.
>
> The new property allows to declare that the remote camera
> power-over-coax is controlled by one of the MAX9286 gpio lines.
>
> As it is currently not possible to establish a regulator as consumer
> of the MAX9286 gpio controller for this purpose, the property allows to
> declare that the camera power is controlled by the MAX9286 directly.
>
> The property accepts a gpio-index (0 or 1) and one line polarity
> flag as defined by dt-bindings/gpio/gpio.h.
>
> Signed-off-by: Jacopo Mondi
> ---
> .../bindings/media/i2c/maxim,max9286.yaml | 53 ++-
> 1 file changed, 52 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
> b/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
> index ee16102fdfe7..480a491f3744 100644
> --- a/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
> +++ b/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
> @@ -70,6 +70,24 @@ properties:
>a remote serializer whose high-threshold noise immunity is not enabled
>is 10 micro volts
>
> + maxim,gpio-poc:
I would have written poc-gpio to match the order of the GPIO bindings
syntax.
> +$ref: '/schemas/types.yaml#/definitions/uint32-array'
> +minItems: 2
> +maxItems: 2
> +description: |
> + Identifier of gpio line that controls Power over Coax to the cameras
> and
I'd write "Index of the MAX9286 GPIO output that ..." to make it clear
that this is not a generic GPIO.
> + the associated polarity flag (GPIO_ACTIVE_HIGH and GPIO_ACTIVE_LOW)
> + as defined in .
> +
> + When the remote cameras power is controlled by one of the MAX9286 gpio
> + lines, this property has to be used to specify which line among the two
> + available ones controls the remote camera power enablement.
> +
> + When this property is used it is not possible to register a gpio
> + controller as the gpio lines are controlled directly by the MAX9286 and
> + not available for consumers, nor the 'poc-supply' property should be
> + specified.
Only one of the two lines would be controlled directly. Shouldn't we
still register a GPIO controller for the other line ?
Could you also mention somewhere that the first item in the array should
be 0 or 1 ? It may be hard to express in a YAML schema, so I'm fine just
documenting it in the description.
I've been wondering whether this would be a common enough issue that it
could justify support in the GPIO core to handle consumer-provider
loops, but even if that happens at some point in the future, I think the
proposal here is good enough and we won't need to switch.
> +
>ports:
> $ref: /schemas/graph.yaml#/properties/ports
>
> @@ -182,7 +200,6 @@ required:
>- reg
>- ports
>- i2c-mux
> - - gpio-controller
>
> additionalProperties: false
>
> @@ -327,4 +344,38 @@ examples:
>};
> };
>};
> +
> + /*
> + * Example of a deserializer that controls the camera Power over Coax
> + * through one of its gpio lines.
> + */
> + gmsl-deserializer@6c {
> +compatible = "maxim,max9286";
> +reg = <0x6c>;
> +enable-gpios = < 14 GPIO_ACTIVE_HIGH>;
> +
> +/*
> + * The remote camera power is controlled by MAX9286 GPIO line #0.
> + * No 'poc-supply' nor 'gpio-controller' are specified.
> + */
> +maxim,gpio-poc = <0 GPIO_ACTIVE_LOW>;
> +
> +/*
> + * Do not describe connections as they're the same as in the previous
> + * example.
> + */
> +ports {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + port@4 {
> +reg = <4>;
> + };
> + };
> +
> + i2c-mux {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +};
> + };
> };
It's customary to indent DT examples with 4 spaces. The existing
examples use two spaces, so maybe a patch on top of this would be useful
to increase readability ?
Reviewed-by: Laurent Pinchart
--
Regards,
Laurent Pinchart
Re: [PATCH v4 11/17] media: i2c: rdacm20: Enable noise immunity
Hi Jacopo, Thank you for the patch. On Mon, Apr 12, 2021 at 11:34:45AM +0200, Jacopo Mondi wrote: > Enable the noise immunity threshold at the end of the rdacm20 > initialization routine. > > The rdacm20 camera module has been so far tested with a startup > delay that allowed the embedded MCU to program the serializer. If > the initialization routine is run before the MCU programs the > serializer and the image sensor and their addresses gets changed > by the rdacm20 driver it is required to manually enable the noise > immunity threshold to make the communication on the control channel > more reliable. > > Reviewed-by: Kieran Bingham > Signed-off-by: Jacopo Mondi Reviewed-by: Laurent Pinchart > --- > drivers/media/i2c/rdacm20.c | 14 +- > 1 file changed, 13 insertions(+), 1 deletion(-) > > diff --git a/drivers/media/i2c/rdacm20.c b/drivers/media/i2c/rdacm20.c > index c1a717153303..5e0314a2b1ca 100644 > --- a/drivers/media/i2c/rdacm20.c > +++ b/drivers/media/i2c/rdacm20.c > @@ -539,7 +539,19 @@ static int rdacm20_initialize(struct rdacm20_device *dev) > > dev_info(dev->dev, "Identified MAX9271 + OV10635 device\n"); > > - return 0; > + /* > + * Set reverse channel high threshold to increase noise immunity. > + * > + * This should be compensated by increasing the reverse channel > + * amplitude on the remote deserializer side. > + * > + * TODO Inspect the embedded MCU programming sequence to make sure > + * there are no conflicts with the configuration applied here. > + * > + * TODO Clarify the embedded MCU startup delay to avoid write > + * collisions on the I2C bus. > + */ > + return max9271_set_high_threshold(>serializer, true); > } > > static int rdacm20_probe(struct i2c_client *client) -- Regards, Laurent Pinchart
Re: [PATCH v4 10/17] media: i2c: rdacm21: Power up OV10640 before OV490
Hi Jacopo,
Thank you for the patch.
On Mon, Apr 12, 2021 at 11:34:44AM +0200, Jacopo Mondi wrote:
> The current RDACM21 initialization routine powers up the OV10640 image
> sensor after the OV490 ISP. The ISP is programmed with a firmare loaded
> from an embedded serial flash that (most probably) tries to interact and
> program also the image sensor connected to the ISP.
>
> As described in commit ("media: i2c: rdacm21: Fix OV10640 powerup") the
s/[()]//g
> image sensor powerdown signal is kept high by an internal pull up
> resistor and occasionally fails to startup correctly if the powerdown
> line is not asserted explicitely. Failures in the OV10640 startup causes
> the OV490 firmware to fail to boot correctly resulting in the camera
> module initialization to fail consequentially.
>
> Fix this by powering up the OV10640 image sensor before testing the
> OV490 firmware boot completion, by splitting the ov10640_initialize()
> function in an ov10640_power_up() one and an ov10640_check_id() one.
>
> Also make sure the OV10640 identification procedure gives enough time to
> the image sensor to resume after the programming phase performed by the
> OV490 firmware by repeating the ID read procedure.
>
> This commit fixes a sporadic start-up error triggered by a failure to
> detect the OV490 firmware boot completion:
> rdacm21 8-0054: Timeout waiting for firmware boot
>
> Fixes: a59f853b3b4b ("media: i2c: Add driver for RDACM21 camera module")
> Signed-off-by: Jacopo Mondi
Reviewed-by: Laurent Pinchart
> ---
> drivers/media/i2c/rdacm21.c | 46 ++---
> 1 file changed, 32 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/media/i2c/rdacm21.c b/drivers/media/i2c/rdacm21.c
> index 7c0a4a84340a..43c41cb800a4 100644
> --- a/drivers/media/i2c/rdacm21.c
> +++ b/drivers/media/i2c/rdacm21.c
> @@ -69,6 +69,7 @@
> #define OV490_ISP_VSIZE_LOW 0x80820062
> #define OV490_ISP_VSIZE_HIGH 0x80820063
>
> +#define OV10640_PID_TIMEOUT 20
> #define OV10640_ID_HIGH 0xa6
> #define OV10640_CHIP_ID 0x300a
> #define OV10640_PIXEL_RATE 5500
> @@ -329,10 +330,8 @@ static const struct v4l2_subdev_ops rdacm21_subdev_ops =
> {
> .pad= _subdev_pad_ops,
> };
>
> -static int ov10640_initialize(struct rdacm21_device *dev)
> +static void ov10640_power_up(struct rdacm21_device *dev)
> {
> - u8 val;
> -
> /* Enable GPIO0#0 (reset) and GPIO1#0 (pwdn) as output lines. */
> ov490_write_reg(dev, OV490_GPIO_SEL0, OV490_GPIO0);
> ov490_write_reg(dev, OV490_GPIO_SEL1, OV490_SPWDN0);
> @@ -347,18 +346,35 @@ static int ov10640_initialize(struct rdacm21_device
> *dev)
> usleep_range(1500, 3000);
> ov490_write_reg(dev, OV490_GPIO_OUTPUT_VALUE0, OV490_GPIO0);
> usleep_range(3000, 5000);
> +}
>
> - /* Read OV10640 ID to test communications. */
> - ov490_write_reg(dev, OV490_SCCB_SLAVE0_DIR, OV490_SCCB_SLAVE_READ);
> - ov490_write_reg(dev, OV490_SCCB_SLAVE0_ADDR_HIGH, OV10640_CHIP_ID >> 8);
> - ov490_write_reg(dev, OV490_SCCB_SLAVE0_ADDR_LOW, OV10640_CHIP_ID &
> 0xff);
> -
> - /* Trigger SCCB slave transaction and give it some time to complete. */
> - ov490_write_reg(dev, OV490_HOST_CMD, OV490_HOST_CMD_TRIGGER);
> - usleep_range(1000, 1500);
> +static int ov10640_check_id(struct rdacm21_device *dev)
> +{
> + unsigned int i;
> + u8 val;
>
> - ov490_read_reg(dev, OV490_SCCB_SLAVE0_DIR, );
> - if (val != OV10640_ID_HIGH) {
> + /* Read OV10640 ID to test communications. */
> + for (i = 0; i < OV10640_PID_TIMEOUT; ++i) {
> + ov490_write_reg(dev, OV490_SCCB_SLAVE0_DIR,
> + OV490_SCCB_SLAVE_READ);
> + ov490_write_reg(dev, OV490_SCCB_SLAVE0_ADDR_HIGH,
> + OV10640_CHIP_ID >> 8);
> + ov490_write_reg(dev, OV490_SCCB_SLAVE0_ADDR_LOW,
> + OV10640_CHIP_ID & 0xff);
> +
> + /*
> + * Trigger SCCB slave transaction and give it some time
> + * to complete.
> + */
> + ov490_write_reg(dev, OV490_HOST_CMD, OV490_HOST_CMD_TRIGGER);
> + usleep_range(1000, 1500);
> +
> + ov490_read_reg(dev, OV490_SCCB_SLAVE0_DIR, );
> + if (val == OV10640_ID_HIGH)
> + break;
> + usleep_range(1000, 1500);
> + }
> + if (i == OV10640_PID_TIMEOUT) {
> dev_err(dev->dev, "OV10640 ID mismatch: (0x%02x)\n", val);
>
Re: [PATCH v4 08/17] media: i2c: rdacm21: Add dealy after OV490 reset
Hi Jacopo, Thank you for the patch. On Mon, Apr 12, 2021 at 11:34:42AM +0200, Jacopo Mondi wrote: > Add a delay after the OV490 chip is put in reset state. The reset > signal shall be held for at least 250 useconds. > > Signed-off-by: Jacopo Mondi With s/dealy/delay/ in the subject line, Reviewed-by: Laurent Pinchart > --- > drivers/media/i2c/rdacm21.c | 6 +- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/drivers/media/i2c/rdacm21.c b/drivers/media/i2c/rdacm21.c > index 553e3f03752b..6be8ce130e78 100644 > --- a/drivers/media/i2c/rdacm21.c > +++ b/drivers/media/i2c/rdacm21.c > @@ -469,7 +469,10 @@ static int rdacm21_initialize(struct rdacm21_device *dev) > if (ret) > return ret; > > - /* Enable GPIO1 and hold OV490 in reset during max9271 configuration. */ > + /* > + * Enable GPIO1 and hold OV490 in reset during max9271 configuration. > + * The reset signal has to be asserted for at least 250 useconds. > + */ > ret = max9271_enable_gpios(>serializer, MAX9271_GPIO1OUT); > if (ret) > return ret; > @@ -477,6 +480,7 @@ static int rdacm21_initialize(struct rdacm21_device *dev) > ret = max9271_clear_gpios(>serializer, MAX9271_GPIO1OUT); > if (ret) > return ret; > + usleep_range(250, 500); > > ret = max9271_configure_gmsl_link(>serializer); > if (ret) -- Regards, Laurent Pinchart
Re: [PATCH v3 2/3] drm: bridge: add it66121 driver
Hi Neil,
On Wed, Apr 14, 2021 at 10:08:46AM +0200, Neil Armstrong wrote:
> On 14/04/2021 10:06, Robert Foss wrote:
> > On Wed, 14 Apr 2021 at 08:13, Neil Armstrong
> > wrote:
> >> Le 13/04/2021 à 22:21, Robert Foss a écrit :
> >>> Hey Neil & Phong,
> >>>
> >>> Thanks for submitting this series!
> >>>
> >>>> +
> >>>> +static const struct drm_bridge_funcs it66121_bridge_funcs = {
> >>>> + .attach = it66121_bridge_attach,
> >>>> + .enable = it66121_bridge_enable,
> >>>> + .disable = it66121_bridge_disable,
> >>>> + .mode_set = it66121_bridge_mode_set,
> >>>> + .mode_valid = it66121_bridge_mode_valid,
> >>>> + .detect = it66121_bridge_detect,
> >>>> + .get_edid = it66121_bridge_get_edid,
> >>>> + .atomic_get_output_bus_fmts =
> >>>> it66121_bridge_atomic_get_output_bus_fmts,
> >>>> + .atomic_get_input_bus_fmts =
> >>>> it66121_bridge_atomic_get_input_bus_fmts,
> >>>> +};
> >>>
> >>> I would like to see an implementation of HPD, since it is supported by
> >>> the hardware[1] (and required by the documentation). IRQ status bit 0
> >>> seems to be the responsible for notifying us about hot plug detection
> >>> events.
> >>
> >> It's implemented in the IRQ handler with the
> >> IT66121_INT_STATUS1_HPD_STATUS event.
> >
> > I didn't even get that far :)
> >
> > Either way, the HPD support should be exposed in drm_bridge_funcs
> > (.hpd_enable, .hpd_disable (and possibly .hpd_notify)) and
> > drm_bridge.ops (DRM_BRIDGE_OP_HPD).
>
> Indeed I forgot these calls in the NO_CONNECTOR implementation...
For new bridges, you should no implement connector creation, only the
DRM_BRIDGE_ATTACH_NO_CONNECTOR case should be supported.
--
Regards,
Laurent Pinchart
Re: [PATCH v3 1/3] dt-bindings: display: bridge: add it66121 bindings
Hi Paul,
On Tue, Apr 13, 2021 at 07:09:17PM +0100, Paul Cercueil wrote:
> Le lun. 12 avril 2021 à 17:46, Neil Armstrong a écrit :
> > From: Phong LE
> >
> > Add the ITE bridge HDMI it66121 bindings.
> >
> > Signed-off-by: Phong LE
> > Signed-off-by: Neil Armstrong
> > ---
> > .../bindings/display/bridge/ite,it66121.yaml | 123 ++
> > 1 file changed, 123 insertions(+)
> > create mode 100644
> > Documentation/devicetree/bindings/display/bridge/ite,it66121.yaml
> >
> > diff --git
> > a/Documentation/devicetree/bindings/display/bridge/ite,it66121.yaml
> > b/Documentation/devicetree/bindings/display/bridge/ite,it66121.yaml
> > new file mode 100644
> > index ..61ed6dc7740b
> > --- /dev/null
> > +++
> > b/Documentation/devicetree/bindings/display/bridge/ite,it66121.yaml
> > @@ -0,0 +1,123 @@
> > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/display/bridge/ite,it66121.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: ITE it66121 HDMI bridge Device Tree Bindings
> > +
> > +maintainers:
> > + - Phong LE
> > + - Neil Armstrong
> > +
> > +description: |
> > + The IT66121 is a high-performance and low-power single channel HDMI
> > + transmitter, fully compliant with HDMI 1.3a, HDCP 1.2 and backward
> > compatible
> > + to DVI 1.0 specifications.
> > +
> > +properties:
> > + compatible:
> > +const: ite,it66121
> > +
> > + reg:
> > +maxItems: 1
> > +description: base I2C address of the device
> > +
> > + reset-gpios:
> > +maxItems: 1
> > +description: GPIO connected to active low reset
> > +
> > + vrf12-supply:
> > +description: Regulator for 1.2V analog core power.
> > +
> > + vcn33-supply:
> > +description: Regulator for 3.3V digital core power.
> > +
> > + vcn18-supply:
> > +description: Regulator for 1.8V IO core power.
> > +
> > + interrupts:
> > +maxItems: 1
> > +
> > + ports:
> > +$ref: /schemas/graph.yaml#/properties/ports
> > +
> > +properties:
> > + port@0:
> > +$ref: /schemas/graph.yaml#/$defs/port-base
> > +unevaluatedProperties: false
> > +description: DPI input port.
> > +
> > +properties:
> > + endpoint:
> > +$ref: /schemas/graph.yaml#/$defs/endpoint-base
> > +unevaluatedProperties: false
> > +
> > +properties:
> > + bus-width:
> > +description:
> > + Endpoint bus width.
> > +enum:
> > + - 12 # 12 data lines connected and dual-edge mode
> > + - 24 # 24 data lines connected and single-edge
> > mode
> > +default: 24
> > +
> > + port@1:
> > +$ref: /schemas/graph.yaml#/properties/port
> > +description: HDMI Connector port.
> > +
> > +required:
> > + - port@0
> > + - port@1
>
> Should port@1 really be required? Since the chip itself handles the
> hotplug detection and stuff like DCC, I'm not sure what to connect here.
It should be connected to a DT node that models the connector
(Documentation/devicetree/bindings/display/connector/*).
> > +
> > +required:
> > + - compatible
> > + - reg
> > + - reset-gpios
> > + - vrf12-supply
> > + - vcn33-supply
> > + - vcn18-supply
> > + - interrupts
> > + - ports
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > + - |
> > +i2c {
> > + #address-cells = <1>;
> > + #size-cells = <0>;
> > +
> > + it66121hdmitx: it66121hdmitx@4c {
> > +compatible = "ite,it66121";
> > +pinctrl-names = "default";
> > +pinctrl-0 = <_pins_default>;
> > +vcn33-supply = <_vcn33_wifi_reg>;
> > +vcn18-supply = <_vcn18_reg>;
> > + vrf12-supply = <_vrf12_reg>;
> > +reset-gpios = < 160 1 /* GPIO_ACTIVE_LOW */>;
> > +interrupt-parent = <>;
> > +interrupts = <4 8 /* IRQ_TYPE_LEVEL_LOW */>;
> > +reg = <0x4c>;
> > +
> > +ports {
> > + #address-cells = <1>;
> > + #size-cells = <0>;
> > +
> > + port@0 {
> > +reg = <0>;
> > +it66121_in: endpoint {
> > + bus-width = <12>;
> > + remote-endpoint = <_out>;
> > +};
> > + };
> > +
> > + port@1 {
> > +reg = <1>;
> > +hdmi_conn_out: endpoint {
> > + remote-endpoint = <_conn_in>;
> > +};
> > + };
> > +};
> > + };
> > +};
--
Regards,
Laurent Pinchart
Re: [PATCH v3 1/3] dt-bindings: display: bridge: add it66121 bindings
Hi Neil,
Thank you for the patch.
On Mon, Apr 12, 2021 at 05:46:46PM +0200, Neil Armstrong wrote:
> From: Phong LE
>
> Add the ITE bridge HDMI it66121 bindings.
>
> Signed-off-by: Phong LE
> Signed-off-by: Neil Armstrong
> ---
> .../bindings/display/bridge/ite,it66121.yaml | 123 ++
> 1 file changed, 123 insertions(+)
> create mode 100644
> Documentation/devicetree/bindings/display/bridge/ite,it66121.yaml
>
> diff --git
> a/Documentation/devicetree/bindings/display/bridge/ite,it66121.yaml
> b/Documentation/devicetree/bindings/display/bridge/ite,it66121.yaml
> new file mode 100644
> index ..61ed6dc7740b
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/bridge/ite,it66121.yaml
> @@ -0,0 +1,123 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/display/bridge/ite,it66121.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: ITE it66121 HDMI bridge Device Tree Bindings
> +
> +maintainers:
> + - Phong LE
> + - Neil Armstrong
> +
> +description: |
> + The IT66121 is a high-performance and low-power single channel HDMI
> + transmitter, fully compliant with HDMI 1.3a, HDCP 1.2 and backward
> compatible
> + to DVI 1.0 specifications.
> +
> +properties:
> + compatible:
> +const: ite,it66121
> +
> + reg:
> +maxItems: 1
> +description: base I2C address of the device
You can drop the description.
> +
> + reset-gpios:
> +maxItems: 1
> +description: GPIO connected to active low reset
> +
> + vrf12-supply:
> +description: Regulator for 1.2V analog core power.
> +
> + vcn33-supply:
> +description: Regulator for 3.3V digital core power.
> +
> + vcn18-supply:
> +description: Regulator for 1.8V IO core power.
> +
> + interrupts:
> +maxItems: 1
> +
> + ports:
> +$ref: /schemas/graph.yaml#/properties/ports
> +
> +properties:
> + port@0:
> +$ref: /schemas/graph.yaml#/$defs/port-base
> +unevaluatedProperties: false
> +description: DPI input port.
> +
> +properties:
> + endpoint:
> +$ref: /schemas/graph.yaml#/$defs/endpoint-base
> +unevaluatedProperties: false
> +
> +properties:
> + bus-width:
> +description:
> + Endpoint bus width.
> +enum:
> + - 12 # 12 data lines connected and dual-edge mode
> + - 24 # 24 data lines connected and single-edge mode
> +default: 24
> +
> + port@1:
> +$ref: /schemas/graph.yaml#/properties/port
> +description: HDMI Connector port.
> +
> +required:
> + - port@0
> + - port@1
> +
> +required:
> + - compatible
> + - reg
> + - reset-gpios
> + - vrf12-supply
> + - vcn33-supply
> + - vcn18-supply
> + - interrupts
> + - ports
> +
> +additionalProperties: false
> +
> +examples:
> + - |
> +i2c {
> + #address-cells = <1>;
> + #size-cells = <0>;
It's customary to indent DT examples with 4 spaces.
> +
> + it66121hdmitx: it66121hdmitx@4c {
> +compatible = "ite,it66121";
> +pinctrl-names = "default";
> + pinctrl-0 = <_pins_default>;
> +vcn33-supply = <_vcn33_wifi_reg>;
> +vcn18-supply = <_vcn18_reg>;
> +vrf12-supply = <_vrf12_reg>;
> +reset-gpios = < 160 1 /* GPIO_ACTIVE_LOW */>;
You can #include the necessary headers at the top of the example, and
use GPIO_ACTIVE_LOW and IRQ_TYPE_LEVEL_LOW to replace the numerical
values.
Reviewed-by: Laurent Pinchart
> +interrupt-parent = <>;
> +interrupts = <4 8 /* IRQ_TYPE_LEVEL_LOW */>;
> +reg = <0x4c>;
> +
> +ports {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + port@0 {
> +reg = <0>;
> +it66121_in: endpoint {
> + bus-width = <12>;
> + remote-endpoint = <_out>;
> +};
> + };
> +
> + port@1 {
> +reg = <1>;
> +hdmi_conn_out: endpoint {
> + remote-endpoint = <_conn_in>;
> +};
> + };
> +};
> + };
> +};
--
Regards,
Laurent Pinchart
Re: [PATCH 1/1] staging: ipu3-imgu: Move the UAPI header from include under include/uapi
Hi Sakari,
Thank you for the patch.
On Mon, Apr 12, 2021 at 02:11:20PM +0300, Sakari Ailus wrote:
> The header defines the user space interface but may be mistaken as
> kernel-only header due to its location. Add "uapi" directory under
> driver's include directory and move the header there.
>
> Suggested-by: Greg KH
> Signed-off-by: Sakari Ailus
> ---
> Documentation/admin-guide/media/ipu3.rst | 35 ++-
> .../media/v4l/pixfmt-meta-intel-ipu3.rst | 2 +-
> .../ipu3/include/{ => uapi}/intel-ipu3.h | 0
> drivers/staging/media/ipu3/ipu3-abi.h | 2 +-
> 4 files changed, 20 insertions(+), 19 deletions(-)
> rename drivers/staging/media/ipu3/include/{ => uapi}/intel-ipu3.h (100%)
>
> diff --git a/Documentation/admin-guide/media/ipu3.rst
> b/Documentation/admin-guide/media/ipu3.rst
> index f59697c7b374..d6454f637ff4 100644
> --- a/Documentation/admin-guide/media/ipu3.rst
> +++ b/Documentation/admin-guide/media/ipu3.rst
> @@ -234,22 +234,23 @@ The IPU3 ImgU pipelines can be configured using the
> Media Controller, defined at
> Running mode and firmware binary selection
> --
>
> -ImgU works based on firmware, currently the ImgU firmware support run 2
> pipes in
> -time-sharing with single input frame data. Each pipe can run at certain mode
> -
> -"VIDEO" or "STILL", "VIDEO" mode is commonly used for video frames capture,
> and
> -"STILL" is used for still frame capture. However, you can also select
> "VIDEO" to
> -capture still frames if you want to capture images with less system load and
> -power. For "STILL" mode, ImgU will try to use smaller BDS factor and output
> -larger bayer frame for further YUV processing than "VIDEO" mode to get high
> -quality images. Besides, "STILL" mode need XNR3 to do noise reduction, hence
> -"STILL" mode will need more power and memory bandwidth than "VIDEO" mode. TNR
> -will be enabled in "VIDEO" mode and bypassed by "STILL" mode. ImgU is
> running at
> -“VIDEO” mode by default, the user can use v4l2 control
> V4L2_CID_INTEL_IPU3_MODE
> -(currently defined in drivers/staging/media/ipu3/include/intel-ipu3.h) to
> query
> -and set the running mode. For user, there is no difference for buffer
> queueing
> -between the "VIDEO" and "STILL" mode, mandatory input and main output node
> -should be enabled and buffers need be queued, the statistics and the
> view-finder
> -queues are optional.
> +ImgU works based on firmware, currently the ImgU firmware support run 2 pipes
> +in time-sharing with single input frame data. Each pipe can run at certain
> mode
> +- "VIDEO" or "STILL", "VIDEO" mode is commonly used for video frames capture,
> +and "STILL" is used for still frame capture. However, you can also select
> +"VIDEO" to capture still frames if you want to capture images with less
> system
> +load and power. For "STILL" mode, ImgU will try to use smaller BDS factor and
> +output larger bayer frame for further YUV processing than "VIDEO" mode to get
> +high quality images. Besides, "STILL" mode need XNR3 to do noise reduction,
> +hence "STILL" mode will need more power and memory bandwidth than "VIDEO"
> mode.
> +TNR will be enabled in "VIDEO" mode and bypassed by "STILL" mode. ImgU is
> +running at “VIDEO” mode by default, the user can use v4l2 control
> +V4L2_CID_INTEL_IPU3_MODE (currently defined in
> +drivers/staging/media/ipu3/include/uapi/intel-ipu3.h) to query and set the
> +running mode. For user, there is no difference for buffer queueing between
> the
> +"VIDEO" and "STILL" mode, mandatory input and main output node should be
> +enabled and buffers need be queued, the statistics and the view-finder queues
> +are optional.
The reflow of the whole paragraph is a bit painful to review.
Reviewed-by: Laurent Pinchart
>
> The firmware binary will be selected according to current running mode, such
> log
> "using binary if_to_osys_striped " or "using binary
> if_to_osys_primary_striped"
> @@ -586,7 +587,7 @@ preserved.
> References
> ==
>
> -.. [#f5] drivers/staging/media/ipu3/include/intel-ipu3.h
> +.. [#f5] drivers/staging/media/ipu3/include/uapi/intel-ipu3.h
>
> .. [#f1] https://github.com/intel/nvt
>
> diff --git a/Documentation/userspace-api/media/v4l/pixfmt-meta-intel-ipu3.rst
> b/Documentation/userspace-api/media/v4l/pixfmt-meta-intel-ipu3.rst
> index 5f33d35532ef..84d81dd
Re: [Outreachy kernel][PATCH 1/4 v2] staging: media: omap4iss: Replace macro function by static inline function in file iss.c
Hi Aline, On Mon, Apr 12, 2021 at 10:58:45AM -0300, ascordeiro wrote: > Em seg, 2021-04-12 às 16:40 +0300, Laurent Pinchart escreveu: > > While testing on a device isn't a requirement as you can't be expected > > to have the necessary hardware, changes are expected to be at least > > compile-tested before being submitted. > > Hi Laurent, > > I thought recompiling the kernel would be enough in this case. > I recompiled it in native Ubuntu 16.04 without errors. Did it compile the driver you modified ? -- Regards, Laurent Pinchart
Re: [Outreachy kernel][PATCH 1/4 v2] staging: media: omap4iss: Replace macro function by static inline function in file iss.c
Hi Aline,
While testing on a device isn't a requirement as you can't be expected
to have the necessary hardware, changes are expected to be at least
compile-tested before being submitted.
On Mon, Apr 12, 2021 at 09:57:09AM -0300, Aline Santana Cordeiro wrote:
> Replace macro function by static inline function.
> Issue suggested by Julia.
>
> Signed-off-by: Aline Santana Cordeiro
> ---
>
> Changes since v1:
> - Insert file path in commit message
>
> drivers/staging/media/omap4iss/iss.c | 24 +---
> 1 file changed, 13 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/staging/media/omap4iss/iss.c
> b/drivers/staging/media/omap4iss/iss.c
> index c89f268a..3bbc39e 100644
> --- a/drivers/staging/media/omap4iss/iss.c
> +++ b/drivers/staging/media/omap4iss/iss.c
> @@ -27,22 +27,24 @@
> #include "iss.h"
> #include "iss_regs.h"
>
> -#define ISS_PRINT_REGISTER(iss, name)\
> - dev_dbg(iss->dev, "###ISS " #name "=0x%08x\n", \
> - iss_reg_read(iss, OMAP4_ISS_MEM_TOP, ISS_##name))
> +static inline iss_print_register(iss, name)
> +{
> + dev_dbg(iss->dev, "###ISS " #name "=0x%08x\n",
> + iss_reg_read(iss, OMAP4_ISS_MEM_TOP, ISS_##name));
> +}
>
> static void iss_print_status(struct iss_device *iss)
> {
> dev_dbg(iss->dev, "-ISS HL Register dump-\n");
>
> - ISS_PRINT_REGISTER(iss, HL_REVISION);
> - ISS_PRINT_REGISTER(iss, HL_SYSCONFIG);
> - ISS_PRINT_REGISTER(iss, HL_IRQSTATUS(5));
> - ISS_PRINT_REGISTER(iss, HL_IRQENABLE_SET(5));
> - ISS_PRINT_REGISTER(iss, HL_IRQENABLE_CLR(5));
> - ISS_PRINT_REGISTER(iss, CTRL);
> - ISS_PRINT_REGISTER(iss, CLKCTRL);
> - ISS_PRINT_REGISTER(iss, CLKSTAT);
> + iss_print_register(iss, HL_REVISION);
> + iss_print_register(iss, HL_SYSCONFIG);
> + iss_print_register(iss, HL_IRQSTATUS(5));
> + iss_print_register(iss, HL_IRQENABLE_SET(5));
> + iss_print_register(iss, HL_IRQENABLE_CLR(5));
> + iss_print_register(iss, CTRL);
> + iss_print_register(iss, CLKCTRL);
> + iss_print_register(iss, CLKSTAT);
>
> dev_dbg(iss->dev, "---\n");
> }
--
Regards,
Laurent Pinchart
Re: [PATCH] drm: bridge: rename the function drm_bridge_hpd_notify to drm_bridge_hpd_cb
er_hpd_irq_event(hdmi->bridge.dev);
> - drm_bridge_hpd_notify(>bridge, status);
> + drm_bridge_hpd_cb(>bridge, status);
> }
> }
>
> diff --git a/drivers/gpu/drm/bridge/ti-tpd12s015.c
> b/drivers/gpu/drm/bridge/ti-tpd12s015.c
> index e0e015243a60..2f079b6f51bc 100644
> --- a/drivers/gpu/drm/bridge/ti-tpd12s015.c
> +++ b/drivers/gpu/drm/bridge/ti-tpd12s015.c
> @@ -103,7 +103,7 @@ static irqreturn_t tpd12s015_hpd_isr(int irq, void *data)
> struct tpd12s015_device *tpd = data;
> struct drm_bridge *bridge = >bridge;
>
> - drm_bridge_hpd_notify(bridge, tpd12s015_detect(bridge));
> + drm_bridge_hpd_cb(bridge, tpd12s015_detect(bridge));
>
> return IRQ_HANDLED;
> }
> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> index 64f0effb52ac..653761a0d5f9 100644
> --- a/drivers/gpu/drm/drm_bridge.c
> +++ b/drivers/gpu/drm/drm_bridge.c
> @@ -1173,7 +1173,7 @@ void drm_bridge_hpd_disable(struct drm_bridge *bridge)
> EXPORT_SYMBOL_GPL(drm_bridge_hpd_disable);
>
> /**
> - * drm_bridge_hpd_notify - notify hot plug detection events
> + * drm_bridge_hpd_cb - notify hot plug detection events
This function is still documented as notifying hot plug detection
events, so drm_bridge_hpd_cb() isn't a great name :-S I do agree there's
confusion with the current naming scheme though.
bridge->hpd_cb() is an internal callback, not part of bridge ops, so I'd
rather not expose its name in the public drm_bridge_hpd_notify() API.
Could we find a better naming scheme ?
> * @bridge: bridge control structure
> * @status: output connection status
> *
> @@ -1183,15 +1183,15 @@ EXPORT_SYMBOL_GPL(drm_bridge_hpd_disable);
> *
> * This function shall be called in a context that can sleep.
> */
> -void drm_bridge_hpd_notify(struct drm_bridge *bridge,
> -enum drm_connector_status status)
> +void drm_bridge_hpd_cb(struct drm_bridge *bridge,
> +enum drm_connector_status status)
> {
> mutex_lock(>hpd_mutex);
> if (bridge->hpd_cb)
> bridge->hpd_cb(bridge->hpd_data, status);
> mutex_unlock(>hpd_mutex);
> }
> -EXPORT_SYMBOL_GPL(drm_bridge_hpd_notify);
> +EXPORT_SYMBOL_GPL(drm_bridge_hpd_cb);
>
> #ifdef CONFIG_OF
> /**
> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
> index 2195daa289d2..ab54715eda8b 100644
> --- a/include/drm/drm_bridge.h
> +++ b/include/drm/drm_bridge.h
> @@ -605,7 +605,7 @@ struct drm_bridge_funcs {
>* @hpd_enable:
>*
>* Enable hot plug detection. From now on the bridge shall call
> - * drm_bridge_hpd_notify() each time a change is detected in the output
> + * drm_bridge_hpd_cb() each time a change is detected in the output
>* connection status, until hot plug detection gets disabled with
>* @hpd_disable.
>*
> @@ -620,7 +620,7 @@ struct drm_bridge_funcs {
>* @hpd_disable:
>*
>* Disable hot plug detection. Once this function returns the bridge
> - * shall not call drm_bridge_hpd_notify() when a change in the output
> + * shall not call drm_bridge_hpd_cb() when a change in the output
>* connection status occurs.
>*
>* This callback is optional and shall only be implemented by bridges
> @@ -878,8 +878,8 @@ void drm_bridge_hpd_enable(struct drm_bridge *bridge,
> enum drm_connector_status status),
> void *data);
> void drm_bridge_hpd_disable(struct drm_bridge *bridge);
> -void drm_bridge_hpd_notify(struct drm_bridge *bridge,
> -enum drm_connector_status status);
> +void drm_bridge_hpd_cb(struct drm_bridge *bridge,
> +enum drm_connector_status status);
>
> #ifdef CONFIG_DRM_PANEL_BRIDGE
> struct drm_bridge *drm_panel_bridge_add(struct drm_panel *panel);
--
Regards,
Laurent Pinchart
Re: [PATCH v3 11/12] drm/bridge: ti-sn65dsi86: Print an error if we fallback to panel modes
Hi Doug, Thank you for the patch. On Fri, Apr 02, 2021 at 03:28:45PM -0700, Douglas Anderson wrote: > Now that we can properly read the EDID for modes there should be no > reason to fallback to the fixed modes that our downstream panel driver > provides us. Let's make that clear by: > - Putting an error message in the logs if we fall back. > - Putting a comment in saying what's going on. > > Signed-off-by: Douglas Anderson Reviewed-by: Laurent Pinchart > --- > > (no changes since v1) > > drivers/gpu/drm/bridge/ti-sn65dsi86.c | 7 +++ > 1 file changed, 7 insertions(+) > > diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c > b/drivers/gpu/drm/bridge/ti-sn65dsi86.c > index fb50f9f95b0f..3b61898cf9cb 100644 > --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c > +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c > @@ -303,6 +303,13 @@ static int ti_sn_bridge_connector_get_modes(struct > drm_connector *connector) > return num; > } > > + /* > + * Ideally this should never happen and we could remove the fallback > + * but let's preserve old behavior. > + */ > + DRM_DEV_ERROR(pdata->dev, > + "Failed to read EDID; falling back to panel modes"); > + > exit: > return drm_panel_get_modes(pdata->panel, connector); > } -- Regards, Laurent Pinchart
Re: [PATCH v3 10/12] drm/bridge: ti-sn65dsi86: Read the EDID only if refclk was provided
Hi Doug,
Thank you for the patch.
On Fri, Apr 02, 2021 at 03:28:44PM -0700, Douglas Anderson wrote:
> Though I don't have access to any hardware that uses ti-sn65dsi86 and
> _doesn't_ provide a "refclk", I believe that we'll have trouble
> reading the EDID at bootup in that case. Specifically I believe that
> if there's no "refclk" we need the MIPI source clock to be active
> before we can successfully read the EDID. My evidence here is that, in
> testing, I couldn't read the EDID until I turned on the DPPLL in the
> bridge chip and that the DPPLL needs the input clock to be active.
>
> Since this is hard to support, let's punt trying to read the EDID if
> there's no "refclk".
>
> I don't believe there are any users of the ti-sn65dsi86 bridge chip
> that _don't_ use "refclk". The bridge chip is _very_ inflexible in
> that mode. The only time I've seen that mode used was for some really
> early prototype hardware that was thrown in the e-waste bin years ago
> when we realized how inflexible it was.
>
> Even if someone is using the bridge chip without the "refclk" they're
> in no worse shape than they were before the (fairly recent) commit
> 58074b08c04a ("drm/bridge: ti-sn65dsi86: Read EDID blob over DDC").
>
> Signed-off-by: Douglas Anderson
Reviewed-by: Laurent Pinchart
> ---
>
> (no changes since v1)
>
> drivers/gpu/drm/bridge/ti-sn65dsi86.c | 13 +
> 1 file changed, 13 insertions(+)
>
> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> index a76cac93cb46..fb50f9f95b0f 100644
> --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> @@ -275,6 +275,18 @@ static int ti_sn_bridge_connector_get_modes(struct
> drm_connector *connector)
> bool was_enabled;
> int num;
>
> + /*
> + * Don't try to read the EDID if no refclk. In theory it is possible
> + * to make this work but it's tricky. I believe that we need to get
> + * our upstream MIPI source to provide a pixel clock before we can
> + * do AUX transations but we need to be able to read the EDID before
> + * we've picked a display mode. The bridge is already super limited
> + * if you try to use it without a refclk so presumably limiting to
> + * the fixed modes our downstream panel reports is fine.
> + */
> + if (!pdata->refclk)
> + goto exit;
> +
> if (!edid) {
> was_enabled = pdata->pre_enabled;
>
> @@ -291,6 +303,7 @@ static int ti_sn_bridge_connector_get_modes(struct
> drm_connector *connector)
> return num;
> }
>
> +exit:
> return drm_panel_get_modes(pdata->panel, connector);
> }
>
--
Regards,
Laurent Pinchart
Re: [PATCH v3 07/12] drm/bridge: ti-sn65dsi86: Remove extra call: drm_connector_update_edid_property()
Hi Doug,
Thank you for the patch.
On Fri, Apr 02, 2021 at 03:28:41PM -0700, Douglas Anderson wrote:
> As of commit 5186421cbfe2 ("drm: Introduce epoch counter to
> drm_connector") the drm_get_edid() function calls
> drm_connector_update_edid_property() for us. There's no reason for us
> to call it again.
>
> Signed-off-by: Douglas Anderson
> Reviewed-by: Andrzej Hajda
Reviewed-by: Laurent Pinchart
This looks like a widespread issue, would you be able to send a patch to
address all the other drivers ?
> ---
>
> (no changes since v1)
>
> drivers/gpu/drm/bridge/ti-sn65dsi86.c | 11 ---
> 1 file changed, 4 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> index 51db30d573c1..6390bc58f29a 100644
> --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> @@ -270,7 +270,7 @@ static int ti_sn_bridge_connector_get_modes(struct
> drm_connector *connector)
> {
> struct ti_sn_bridge *pdata = connector_to_ti_sn_bridge(connector);
> struct edid *edid = pdata->edid;
> - int num, ret;
> + int num;
>
> if (!edid) {
> pm_runtime_get_sync(pdata->dev);
> @@ -279,12 +279,9 @@ static int ti_sn_bridge_connector_get_modes(struct
> drm_connector *connector)
> }
>
> if (edid && drm_edid_is_valid(edid)) {
> - ret = drm_connector_update_edid_property(connector, edid);
> - if (!ret) {
> - num = drm_add_edid_modes(connector, edid);
> - if (num)
> - return num;
> - }
> + num = drm_add_edid_modes(connector, edid);
> + if (num)
> + return num;
> }
>
> return drm_panel_get_modes(pdata->panel, connector);
--
Regards,
Laurent Pinchart
Re: [PATCH v3 06/12] drm/bridge: ti-sn65dsi86: Get rid of the useless detect() function
Hi Doug,
Thank you for the patch.
On Fri, Apr 02, 2021 at 03:28:40PM -0700, Douglas Anderson wrote:
> If we just leave the detect() function as NULL then the upper layers
> assume we're always connected. There's no reason for a stub.
>
> Signed-off-by: Douglas Anderson
> Reviewed-by: Andrzej Hajda
Reviewed-by: Laurent Pinchart
> ---
>
> (no changes since v1)
>
> drivers/gpu/drm/bridge/ti-sn65dsi86.c | 12
> 1 file changed, 12 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> index e30460002c48..51db30d573c1 100644
> --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> @@ -306,20 +306,8 @@ static struct drm_connector_helper_funcs
> ti_sn_bridge_connector_helper_funcs = {
> .mode_valid = ti_sn_bridge_connector_mode_valid,
> };
>
> -static enum drm_connector_status
> -ti_sn_bridge_connector_detect(struct drm_connector *connector, bool force)
> -{
> - /**
> - * TODO: Currently if drm_panel is present, then always
> - * return the status as connected. Need to add support to detect
> - * device state for hot pluggable scenarios.
> - */
> - return connector_status_connected;
> -}
> -
> static const struct drm_connector_funcs ti_sn_bridge_connector_funcs = {
> .fill_modes = drm_helper_probe_single_connector_modes,
> - .detect = ti_sn_bridge_connector_detect,
> .destroy = drm_connector_cleanup,
> .reset = drm_atomic_helper_connector_reset,
> .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
--
Regards,
Laurent Pinchart
Re: [PATCH v3 05/12] drm/bridge: ti-sn65dsi86: Move drm_panel_unprepare() to post_disable()
Hi Doug,
Thank you for the patch.
On Fri, Apr 02, 2021 at 03:28:39PM -0700, Douglas Anderson wrote:
> We prepared the panel in pre_enable() so we should unprepare it in
> post_disable() to match.
>
> This becomes important once we start using pre_enable() and
> post_disable() to make sure things are powered on (and then off again)
> when reading the EDID.
>
> Signed-off-by: Douglas Anderson
> Reviewed-by: Andrzej Hajda
Reviewed-by: Laurent Pinchart
> ---
>
> (no changes since v1)
>
> drivers/gpu/drm/bridge/ti-sn65dsi86.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> index c006678c9921..e30460002c48 100644
> --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> @@ -452,8 +452,6 @@ static void ti_sn_bridge_disable(struct drm_bridge
> *bridge)
> regmap_write(pdata->regmap, SN_ML_TX_MODE_REG, 0);
> /* disable DP PLL */
> regmap_write(pdata->regmap, SN_PLL_ENABLE_REG, 0);
> -
> - drm_panel_unprepare(pdata->panel);
> }
>
> static u32 ti_sn_bridge_get_dsi_freq(struct ti_sn_bridge *pdata)
> @@ -869,6 +867,8 @@ static void ti_sn_bridge_post_disable(struct drm_bridge
> *bridge)
> {
> struct ti_sn_bridge *pdata = bridge_to_ti_sn_bridge(bridge);
>
> + drm_panel_unprepare(pdata->panel);
> +
> clk_disable_unprepare(pdata->refclk);
>
> pm_runtime_put_sync(pdata->dev);
--
Regards,
Laurent Pinchart
Re: [PATCH v3 04/12] drm/bridge: ti-sn65dsi86: Reorder remove()
Hi Doug,
Thank you for the patch.
On Fri, Apr 02, 2021 at 03:28:38PM -0700, Douglas Anderson wrote:
> Let's make the remove() function strictly the reverse of the probe()
> function so it's easier to reason about.
>
> This patch was created by code inspection and should move us closer to
> a proper remove.
>
> Signed-off-by: Douglas Anderson
> Reviewed-by: Andrzej Hajda
Reviewed-by: Laurent Pinchart
> ---
>
> Changes in v3:
> - Removed "NOTES" from commit message.
>
> drivers/gpu/drm/bridge/ti-sn65dsi86.c | 15 ---
> 1 file changed, 8 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> index 76f43af6735d..c006678c9921 100644
> --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> @@ -1315,20 +1315,21 @@ static int ti_sn_bridge_remove(struct i2c_client
> *client)
> if (!pdata)
> return -EINVAL;
>
> - kfree(pdata->edid);
> - ti_sn_debugfs_remove(pdata);
> -
> - of_node_put(pdata->host_node);
> -
> - pm_runtime_disable(pdata->dev);
> -
> if (pdata->dsi) {
> mipi_dsi_detach(pdata->dsi);
> mipi_dsi_device_unregister(pdata->dsi);
> }
>
> + kfree(pdata->edid);
> +
> + ti_sn_debugfs_remove(pdata);
> +
> drm_bridge_remove(>bridge);
>
> + pm_runtime_disable(pdata->dev);
> +
> + of_node_put(pdata->host_node);
> +
> return 0;
> }
>
--
Regards,
Laurent Pinchart
Re: [PATCH v3 03/12] drm/bridge: ti-sn65dsi86: Remove incorrectly tagged kerneldoc comment
Hi Doug, Thank you for the patch. On Fri, Apr 02, 2021 at 03:28:37PM -0700, Douglas Anderson wrote: > A random comment inside a function had "/**" in front of it. That > doesn't make sense. Remove. > > Signed-off-by: Douglas Anderson > Reviewed-by: Andrzej Hajda Reviewed-by: Laurent Pinchart > --- > > (no changes since v1) > > drivers/gpu/drm/bridge/ti-sn65dsi86.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c > b/drivers/gpu/drm/bridge/ti-sn65dsi86.c > index 96fe8f2c0ea9..76f43af6735d 100644 > --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c > +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c > @@ -788,7 +788,7 @@ static void ti_sn_bridge_enable(struct drm_bridge *bridge) > /* set dsi clk frequency value */ > ti_sn_bridge_set_dsi_rate(pdata); > > - /** > + /* >* The SN65DSI86 only supports ASSR Display Authentication method and >* this method is enabled by default. An eDP panel must support this > * authentication method. We need to enable this method in the eDP panel -- Regards, Laurent Pinchart
Re: [PATCH v3 01/12] drm/bridge: Fix the stop condition of drm_bridge_chain_pre_enable()
Hi Doug,
Thank you for the patch.
On Fri, Apr 02, 2021 at 03:28:35PM -0700, Douglas Anderson wrote:
> The drm_bridge_chain_pre_enable() is not the proper opposite of
> drm_bridge_chain_post_disable(). It continues along the chain to
> _before_ the starting bridge. Let's fix that.
>
> Fixes: 05193dc38197 ("drm/bridge: Make the bridge chain a double-linked list")
> Signed-off-by: Douglas Anderson
> Reviewed-by: Andrzej Hajda
> ---
>
> (no changes since v1)
>
> drivers/gpu/drm/drm_bridge.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> index 64f0effb52ac..044acd07c153 100644
> --- a/drivers/gpu/drm/drm_bridge.c
> +++ b/drivers/gpu/drm/drm_bridge.c
> @@ -522,6 +522,9 @@ void drm_bridge_chain_pre_enable(struct drm_bridge
> *bridge)
> list_for_each_entry_reverse(iter, >bridge_chain, chain_node) {
> if (iter->funcs->pre_enable)
> iter->funcs->pre_enable(iter);
> +
> + if (iter == bridge)
> + break;
This looks good as it matches drm_atomic_bridge_chain_disable().
Reviewed-by: Laurent Pinchart
I'm curious though, given that the bridge passed to the function should
be the one closest to the encoder, does this make a difference ?
> }
> }
> EXPORT_SYMBOL(drm_bridge_chain_pre_enable);
--
Regards,
Laurent Pinchart
Re: [PATCH 00/16] CSI2RX support on J721E
On Wed, Mar 31, 2021 at 03:03:51PM +0530, Vinod Koul wrote: > On 30-03-21, 23:03, Pratyush Yadav wrote: > > Hi, > > > > This series adds support for CSI2 capture on J721E. It includes some > > fixes to the Cadence CSI2RX driver, adds Rx support to Cadence DPHY > > driver, and finally adds the TI CSI2RX wrapper driver. > > > > Tested on TI's J721E with OV5640 sensor. > > > > Paul Kocialkowski (1): > > phy: Distinguish between Rx and Tx for MIPI D-PHY with submodes > > > > Pratyush Yadav (15): > > phy: cdns-dphy: Prepare for Rx support > > phy: cdns-dphy: Allow setting mode > > phy: cdns-dphy: Add Rx support > > media: cadence: csi2rx: Add external DPHY support > > media: cadence: csi2rx: Soft reset the streams before starting capture > > media: cadence: csi2rx: Set the STOP bit when stopping a stream > > media: cadence: csi2rx: Fix stream data configuration > > media: cadence: csi2rx: Turn subdev power on before starting stream > > media: cadence: csi2rx: Add wrappers for subdev calls > > dmaengine: ti: k3-psil-j721e: Add entry for CSI2RX > > dt-bindings: media: Add DT bindings for TI CSI2RX driver > > media: ti-vpe: csi2rx: Add CSI2RX support > > dt-bindings: phy: Convert Cadence DPHY binding to YAML > > dt-bindings: phy: cdns,dphy: make clocks optional > > dt-bindings: phy: cdns,dphy: add power-domains property > > Is there any dependency between patches to various subsystems, if not > please do consider sending a series per subsystem... Splitting the series per subsystem will facilitate merging, but for the first versions, keeping all patches together facilitate review. I'd prefer if we could have a v2 that still includes everything, until we agree on the interface between the two subsystems. At that point, we can split the series if needed. > > > > .../devicetree/bindings/media/ti,csi2rx.yaml | 70 ++ > > .../devicetree/bindings/phy/cdns,dphy.txt | 20 - > > .../devicetree/bindings/phy/cdns,dphy.yaml| 52 + > > MAINTAINERS | 7 + > > drivers/dma/ti/k3-psil-j721e.c| 10 + > > drivers/media/platform/Kconfig| 11 + > > drivers/media/platform/cadence/cdns-csi2rx.c | 269 - > > drivers/media/platform/ti-vpe/Makefile| 1 + > > drivers/media/platform/ti-vpe/ti-csi2rx.c | 964 ++ > > drivers/phy/cadence/cdns-dphy.c | 407 +++- > > include/linux/phy/phy-mipi-dphy.h | 13 + > > 11 files changed, 1754 insertions(+), 70 deletions(-) > > create mode 100644 Documentation/devicetree/bindings/media/ti,csi2rx.yaml > > delete mode 100644 Documentation/devicetree/bindings/phy/cdns,dphy.txt > > create mode 100644 Documentation/devicetree/bindings/phy/cdns,dphy.yaml > > create mode 100644 drivers/media/platform/ti-vpe/ti-csi2rx.c -- Regards, Laurent Pinchart
Re: [PATCH 09/16] media: cadence: csi2rx: Turn subdev power on before starting stream
Hi Pratyush,
Thank you for the patch.
On Tue, Mar 30, 2021 at 11:03:41PM +0530, Pratyush Yadav wrote:
> The subdevice power needs to be turned on before the stream is started.
> Otherwise it might not be in the proper state to stream the data. Turn
> it off when stopping the stream.
The .s_power() operation is deprecated. Subdev drivers should control
power internally in their .s_stream() operation, and they should use
runtime PM to do so (this will allow usage of runtime PM autosuspend, to
avoid expensive power off/on cycles when stopping and restarting video
capture).
> Signed-off-by: Pratyush Yadav
> ---
> drivers/media/platform/cadence/cdns-csi2rx.c | 8
> 1 file changed, 8 insertions(+)
>
> diff --git a/drivers/media/platform/cadence/cdns-csi2rx.c
> b/drivers/media/platform/cadence/cdns-csi2rx.c
> index 7d1ac51e0698..3385e1bc213e 100644
> --- a/drivers/media/platform/cadence/cdns-csi2rx.c
> +++ b/drivers/media/platform/cadence/cdns-csi2rx.c
> @@ -256,6 +256,10 @@ static int csi2rx_start(struct csi2rx_priv *csi2rx)
>
> writel(reg, csi2rx->base + CSI2RX_STATIC_CFG_REG);
>
> + ret = v4l2_subdev_call(csi2rx->source_subdev, core, s_power, true);
> + if (ret && ret != -ENOIOCTLCMD)
> + goto err_disable_pclk;
> +
> ret = v4l2_subdev_call(csi2rx->source_subdev, video, s_stream, true);
> if (ret)
> goto err_disable_pclk;
> @@ -358,6 +362,10 @@ static void csi2rx_stop(struct csi2rx_priv *csi2rx)
> if (v4l2_subdev_call(csi2rx->source_subdev, video, s_stream, false))
> dev_warn(csi2rx->dev, "Couldn't disable our subdev\n");
>
> + ret = v4l2_subdev_call(csi2rx->source_subdev, core, s_power, false);
> + if (ret && ret != -ENOIOCTLCMD)
> + dev_warn(csi2rx->dev, "Couldn't power off subdev\n");
> +
> if (csi2rx->dphy) {
> writel(0, csi2rx->base + CSI2RX_DPHY_LANE_CTRL_REG);
>
--
Regards,
Laurent Pinchart
Re: [PATCH 12/16] dt-bindings: media: Add DT bindings for TI CSI2RX driver
On Fri, Apr 02, 2021 at 01:01:22PM +0300, Laurent Pinchart wrote:
> On Thu, Apr 01, 2021 at 10:52:01AM -0500, Rob Herring wrote:
> > On Tue, Mar 30, 2021 at 11:03:44PM +0530, Pratyush Yadav wrote:
> > > TI's J721E uses the Cadence CSI2RX and DPHY peripherals to facilitate
> > > capture over a CSI-2 bus. The TI CSI2RX platform driver glues all the
> > > parts together.
> > >
> > > Signed-off-by: Pratyush Yadav
> > > ---
> > > .../devicetree/bindings/media/ti,csi2rx.yaml | 70 +++
> > > 1 file changed, 70 insertions(+)
> > > create mode 100644 Documentation/devicetree/bindings/media/ti,csi2rx.yaml
> > >
> > > diff --git a/Documentation/devicetree/bindings/media/ti,csi2rx.yaml
> > > b/Documentation/devicetree/bindings/media/ti,csi2rx.yaml
> > > new file mode 100644
> > > index ..ebd894364391
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/media/ti,csi2rx.yaml
> > > @@ -0,0 +1,70 @@
> > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > +%YAML 1.2
> > > +---
> > > +$id: http://devicetree.org/schemas/media/ti,csi2rx.yaml#
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +title: TI CSI2RX Wrapper Device Tree Bindings
> > > +
>
> A description would be useful, especially given that the TRM doesn't
> mention "CSI2RX".
>
> > > +maintainers:
> > > + - Pratyush Yadav
> > > +
> > > +properties:
> > > + compatible:
> > > +items:
> > > + - const: ti,csi2rx
> > > +
> > > + dmas:
> > > +description: RX DMA Channel 0
> >
> > items:
> > - description: RX DMA Channel 0
> >
> > Or just 'maxItems: 1'
> >
> > > +
> > > + dma-names:
> > > +items:
> > > + - const: rx0
> > > +
> > > + reg:
> > > +maxItems: 1
> > > +description: Base address and size of the TI wrapper registers.
> >
> > That's all 'reg' properties, drop 'description'.
>
> According to SPRUIL1B, there are four register banks for the CSI_RX_IF,
> and two register banks for the DPHY_RX. What's your plan to support
> these ? Not everything need to be implemented at once, but backward
> compatibility need to be taken into account in the design.
>
> > > +
> > > + power-domains:
> > > +maxItems: 1
> > > +description:
> > > + PM domain provider node and an args specifier containing
> > > + the device id value.
> >
> > Drop.
> >
> > > +
> > > + ranges: true
> > > +
> > > + "#address-cells":
> > > +const: 2
> > > +
> > > + "#size-cells":
> > > +const: 2
> > > +
> > > +patternProperties:
> > > + "csi-bridge@":
> >
> > "^csi-bridge@"
> >
> > > +type: object
> > > +description: CSI2 bridge node.
> >
> > Just an empty node?
>
> Even if the node is optional, it would be useful to include it in the
> example below, to show how it's supposed to be used.
>
> > > +
> > > +required:
> > > + - compatible
> > > + - reg
> > > + - dmas
> > > + - dma-names
> > > + - power-domains
> > > + - "#address-cells"
> > > + - "#size-cells"
> > > +
> > > +additionalProperties: false
> > > +
> > > +examples:
> > > + - |
> > > +#include
> > > +
> > > +ti_csi2rx0: ticsi2rx {
> > > +compatible = "ti,csi2rx";
> > > +dmas = <_udmap 0x4940>;
> > > +dma-names = "rx0";
> > > +reg = <0x0 0x450 0x0 0x1000>;
> > > +power-domains = <_pds 26 TI_SCI_PD_EXCLUSIVE>;
> > > +#address-cells = <2>;
> > > +#size-cells = <2>;
> > > +};
It would also be useful to expand this to a full example that includes
integration with the PHY.
--
Regards,
Laurent Pinchart
Re: [PATCH 10/16] media: cadence: csi2rx: Add wrappers for subdev calls
Hi Pratyush,
Thank you for the patch.
On Tue, Mar 30, 2021 at 11:03:42PM +0530, Pratyush Yadav wrote:
> When this bridge driver is being user by another platform driver, it
> might want to call subdev operations like getting format, setting
> format, enumerating format codes, etc. Add wrapper functions that pass
> that call through to the sensor.
>
> Currently wrappers are added only for the ops used by TI's platform
> driver. More can be added later as needed.
This isn't the direction we want to take. For new platforms, propagation
of subdev configuration should be handled by userspace, using the V4L2
userspace subdev API. This subdev should not call any subdev operation
from its source other than .s_stream().
> Signed-off-by: Pratyush Yadav
> ---
> drivers/media/platform/cadence/cdns-csi2rx.c | 77
> 1 file changed, 77 insertions(+)
>
> diff --git a/drivers/media/platform/cadence/cdns-csi2rx.c
> b/drivers/media/platform/cadence/cdns-csi2rx.c
> index 3385e1bc213e..2e8bbc53cb8b 100644
> --- a/drivers/media/platform/cadence/cdns-csi2rx.c
> +++ b/drivers/media/platform/cadence/cdns-csi2rx.c
> @@ -408,12 +408,89 @@ static int csi2rx_s_stream(struct v4l2_subdev *subdev,
> int enable)
> return ret;
> }
>
> +static int csi2rx_g_frame_interval(struct v4l2_subdev *subdev,
> +struct v4l2_subdev_frame_interval *fi)
> +{
> + struct csi2rx_priv *csi2rx = v4l2_subdev_to_csi2rx(subdev);
> +
> + return v4l2_subdev_call(csi2rx->source_subdev, video, g_frame_interval,
> + fi);
> +}
> +
> +static int csi2rx_s_frame_interval(struct v4l2_subdev *subdev,
> +struct v4l2_subdev_frame_interval *fi)
> +{
> + struct csi2rx_priv *csi2rx = v4l2_subdev_to_csi2rx(subdev);
> +
> + return v4l2_subdev_call(csi2rx->source_subdev, video, s_frame_interval,
> + fi);
> +}
> +
> +static int csi2rx_enum_mbus_code(struct v4l2_subdev *subdev,
> + struct v4l2_subdev_pad_config *cfg,
> + struct v4l2_subdev_mbus_code_enum *code)
> +{
> + struct csi2rx_priv *csi2rx = v4l2_subdev_to_csi2rx(subdev);
> +
> + return v4l2_subdev_call(csi2rx->source_subdev, pad, enum_mbus_code,
> + cfg, code);
> +}
> +
> +static int csi2rx_get_fmt(struct v4l2_subdev *subdev,
> + struct v4l2_subdev_pad_config *cfg,
> + struct v4l2_subdev_format *fmt)
> +{
> + struct csi2rx_priv *csi2rx = v4l2_subdev_to_csi2rx(subdev);
> +
> + return v4l2_subdev_call(csi2rx->source_subdev, pad, get_fmt, cfg, fmt);
> +}
> +
> +static int csi2rx_set_fmt(struct v4l2_subdev *subdev,
> + struct v4l2_subdev_pad_config *cfg,
> + struct v4l2_subdev_format *fmt)
> +{
> + struct csi2rx_priv *csi2rx = v4l2_subdev_to_csi2rx(subdev);
> +
> + return v4l2_subdev_call(csi2rx->source_subdev, pad, set_fmt, cfg, fmt);
> +}
> +
> +static int csi2rx_enum_frame_size(struct v4l2_subdev *subdev,
> + struct v4l2_subdev_pad_config *cfg,
> + struct v4l2_subdev_frame_size_enum *fse)
> +{
> + struct csi2rx_priv *csi2rx = v4l2_subdev_to_csi2rx(subdev);
> +
> + return v4l2_subdev_call(csi2rx->source_subdev, pad, enum_frame_size,
> + cfg, fse);
> +}
> +
> +static int csi2rx_enum_frame_interval(struct v4l2_subdev *subdev,
> + struct v4l2_subdev_pad_config *cfg,
> + struct v4l2_subdev_frame_interval_enum
> *fie)
> +{
> + struct csi2rx_priv *csi2rx = v4l2_subdev_to_csi2rx(subdev);
> +
> + return v4l2_subdev_call(csi2rx->source_subdev, pad, enum_frame_interval,
> + cfg, fie);
> +}
> +
> static const struct v4l2_subdev_video_ops csi2rx_video_ops = {
> .s_stream = csi2rx_s_stream,
> + .g_frame_interval = csi2rx_g_frame_interval,
> + .s_frame_interval = csi2rx_s_frame_interval,
> +};
> +
> +static const struct v4l2_subdev_pad_ops csi2rx_pad_ops = {
> + .enum_mbus_code = csi2rx_enum_mbus_code,
> + .get_fmt= csi2rx_get_fmt,
> + .set_fmt= csi2rx_set_fmt,
> + .enum_frame_size = csi2rx_enum_frame_size,
> + .enum_frame_interval = csi2rx_enum_frame_interval,
> };
>
> static const struct v4l2_subdev_ops csi2rx_subdev_ops = {
> .video = _video_ops,
> + .pad= _pad_ops,
> };
>
> static int csi2rx_async_bound(struct v4l2_async_notifier *notifier,
--
Regards,
Laurent Pinchart
Re: [PATCH 03/16] phy: cdns-dphy: Allow setting mode
Hi Pratyush,
Thank you for the patch.
On Tue, Mar 30, 2021 at 11:03:35PM +0530, Pratyush Yadav wrote:
> Allow callers to set the PHY mode. The main mode should always be
> PHY_MODE_MIPI_DPHY but the submode can either be
> PHY_MIPI_DPHY_SUBMODE_RX or PHY_MIPI_DPHY_SUBMODE_TX. Update the ops
> based on the requested submode.
Isn't a given DPHY instance always meant to work in one particular mode
? I can't really imagine a single instance of this IP core being
integrated in a way that it can be used in either RX or TX mode. It
seems better to select the mode through DT, by describing if the DPHY is
an RX or TX (possibly through different compatible strings).
> Signed-off-by: Pratyush Yadav
> ---
> drivers/phy/cadence/cdns-dphy.c | 30 ++
> 1 file changed, 30 insertions(+)
>
> diff --git a/drivers/phy/cadence/cdns-dphy.c b/drivers/phy/cadence/cdns-dphy.c
> index 8656f2102a91..7d5f7b333893 100644
> --- a/drivers/phy/cadence/cdns-dphy.c
> +++ b/drivers/phy/cadence/cdns-dphy.c
> @@ -365,11 +365,41 @@ static int cdns_dphy_configure(struct phy *phy, union
> phy_configure_opts *opts)
> return 0;
> }
>
> +static int cdns_dphy_set_mode(struct phy *phy, enum phy_mode mode, int
> submode)
> +{
> + struct cdns_dphy *dphy = phy_get_drvdata(phy);
> + const struct cdns_dphy_driver_data *ddata;
> +
> + ddata = of_device_get_match_data(dphy->dev);
> + if (!ddata)
> + return -EINVAL;
> +
> + if (mode != PHY_MODE_MIPI_DPHY)
> + return -EINVAL;
> +
> + if (submode == PHY_MIPI_DPHY_SUBMODE_TX) {
> + if (!ddata->tx)
> + return -EOPNOTSUPP;
> +
> + dphy->ops = ddata->tx;
> + } else if (submode == PHY_MIPI_DPHY_SUBMODE_RX) {
> + if (!ddata->rx)
> + return -EOPNOTSUPP;
> +
> + dphy->ops = ddata->rx;
> + } else {
> + return -EOPNOTSUPP;
> + }
> +
> + return 0;
> +}
> +
> static const struct phy_ops cdns_dphy_ops = {
> .configure = cdns_dphy_configure,
> .validate = cdns_dphy_validate,
> .power_on = cdns_dphy_power_on,
> .power_off = cdns_dphy_power_off,
> + .set_mode = cdns_dphy_set_mode,
> };
>
> static int cdns_dphy_probe(struct platform_device *pdev)
--
Regards,
Laurent Pinchart
Re: [PATCH 16/16] dt-bindings: phy: cdns,dphy: add power-domains property
Hi Pratyush, Thank you for the patch. On Tue, Mar 30, 2021 at 11:03:48PM +0530, Pratyush Yadav wrote: > This property is needed on TI platforms to enable the PD of the DPHY > before it can be used. > > Signed-off-by: Pratyush Yadav > --- > Documentation/devicetree/bindings/phy/cdns,dphy.yaml | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/Documentation/devicetree/bindings/phy/cdns,dphy.yaml > b/Documentation/devicetree/bindings/phy/cdns,dphy.yaml > index 0807ba68284d..ddcd4de0aef6 100644 > --- a/Documentation/devicetree/bindings/phy/cdns,dphy.yaml > +++ b/Documentation/devicetree/bindings/phy/cdns,dphy.yaml > @@ -30,6 +30,9 @@ properties: >"#phy-cells": > const: 0 > > + power-domains: > +maxItems: 1 > + Would it be useful to add power-domains to the example ? Reviewed-by: Laurent Pinchart > required: >- compatible >- reg -- Regards, Laurent Pinchart
Re: [PATCH 15/16] dt-bindings: phy: cdns,dphy: make clocks optional
Hi Pratyush, Thank you for the patch. On Tue, Mar 30, 2021 at 11:03:47PM +0530, Pratyush Yadav wrote: > The clocks are not used by the DPHY when used in Rx mode so make them > optional. Isn't there a main functional clock (DPHY_RX_MAIN_CLK in the J721E TRM) that is needed in RX mode ? > Signed-off-by: Pratyush Yadav > --- > Documentation/devicetree/bindings/phy/cdns,dphy.yaml | 2 -- > 1 file changed, 2 deletions(-) > > diff --git a/Documentation/devicetree/bindings/phy/cdns,dphy.yaml > b/Documentation/devicetree/bindings/phy/cdns,dphy.yaml > index d1bbf96a8250..0807ba68284d 100644 > --- a/Documentation/devicetree/bindings/phy/cdns,dphy.yaml > +++ b/Documentation/devicetree/bindings/phy/cdns,dphy.yaml > @@ -33,8 +33,6 @@ properties: > required: >- compatible >- reg > - - clocks > - - clock-names >- "#phy-cells" > > additionalProperties: false -- Regards, Laurent Pinchart
Re: [PATCH 14/16] dt-bindings: phy: Convert Cadence DPHY binding to YAML
Hi Pratyush,
Thank you for the patch.
On Tue, Mar 30, 2021 at 11:03:46PM +0530, Pratyush Yadav wrote:
> Convert Cadence DPHY binding to YAML.
>
> Signed-off-by: Pratyush Yadav
> ---
> .../devicetree/bindings/phy/cdns,dphy.txt | 20
> .../devicetree/bindings/phy/cdns,dphy.yaml| 51 +++
> 2 files changed, 51 insertions(+), 20 deletions(-)
> delete mode 100644 Documentation/devicetree/bindings/phy/cdns,dphy.txt
> create mode 100644 Documentation/devicetree/bindings/phy/cdns,dphy.yaml
>
> diff --git a/Documentation/devicetree/bindings/phy/cdns,dphy.txt
> b/Documentation/devicetree/bindings/phy/cdns,dphy.txt
> deleted file mode 100644
> index 1095bc4e72d9..
> --- a/Documentation/devicetree/bindings/phy/cdns,dphy.txt
> +++ /dev/null
> @@ -1,20 +0,0 @@
> -Cadence DPHY
> -
> -
> -Cadence DPHY block.
> -
> -Required properties:
> -- compatible: should be set to "cdns,dphy".
> -- reg: physical base address and length of the DPHY registers.
> -- clocks: DPHY reference clocks.
> -- clock-names: must contain "psm" and "pll_ref".
> -- #phy-cells: must be set to 0.
> -
> -Example:
> - dphy0: dphy@fd0e{
> - compatible = "cdns,dphy";
> - reg = <0x0 0xfd0e 0x0 0x1000>;
> - clocks = <_clk>, <_ref_clk>;
> - clock-names = "psm", "pll_ref";
> - #phy-cells = <0>;
> - };
> diff --git a/Documentation/devicetree/bindings/phy/cdns,dphy.yaml
> b/Documentation/devicetree/bindings/phy/cdns,dphy.yaml
> new file mode 100644
> index ..d1bbf96a8250
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/phy/cdns,dphy.yaml
> @@ -0,0 +1,51 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/phy/cdns,dphy.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Cadence DPHY Device Tree Bindings
> +
> +maintainers:
> + - Pratyush Yadav
> +
> +properties:
> + compatible:
> +items:
> + - const: cdns,dphy
> +
> + reg:
> +maxItems: 1
> +description: Physical base address and length of the DPHY registers.
You can drop the description.
> +
> + clocks:
> +maxItems: 2
> +description: DPHY reference clocks.
It's best to describe individual items, which will then allow dropping
the maxItems property:
clocks:
items:
- description: Description of the psm clock
- description: Description of the pll_ref clock
> +
> + clock-names:
> +items:
> + - const: psm
> + - const: pll_ref
> +
> + "#phy-cells":
> +const: 0
> +
> +required:
> + - compatible
> + - reg
> + - clocks
> + - clock-names
> + - "#phy-cells"
> +
> +additionalProperties: false
> +
> +examples:
> + - |
> +
> +dphy0: dphy@fd0e{
This is copied verbatim from the existing description, but while at it,
I'd rename the node from dphy@... to phy@..., as DT node are supposed to
be named according to the class of devices, not the specific device
type.
With these small issues addressed,
Reviewed-by: Laurent Pinchart
> +compatible = "cdns,dphy";
> +reg = <0xfd0e 0x1000>;
> +clocks = <_clk>, <_ref_clk>;
> +clock-names = "psm", "pll_ref";
> +#phy-cells = <0>;
> +};
--
Regards,
Laurent Pinchart
Re: [PATCH 12/16] dt-bindings: media: Add DT bindings for TI CSI2RX driver
On Thu, Apr 01, 2021 at 10:52:01AM -0500, Rob Herring wrote:
> On Tue, Mar 30, 2021 at 11:03:44PM +0530, Pratyush Yadav wrote:
> > TI's J721E uses the Cadence CSI2RX and DPHY peripherals to facilitate
> > capture over a CSI-2 bus. The TI CSI2RX platform driver glues all the
> > parts together.
> >
> > Signed-off-by: Pratyush Yadav
> > ---
> > .../devicetree/bindings/media/ti,csi2rx.yaml | 70 +++
> > 1 file changed, 70 insertions(+)
> > create mode 100644 Documentation/devicetree/bindings/media/ti,csi2rx.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/media/ti,csi2rx.yaml
> > b/Documentation/devicetree/bindings/media/ti,csi2rx.yaml
> > new file mode 100644
> > index ..ebd894364391
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/media/ti,csi2rx.yaml
> > @@ -0,0 +1,70 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/media/ti,csi2rx.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: TI CSI2RX Wrapper Device Tree Bindings
> > +
A description would be useful, especially given that the TRM doesn't
mention "CSI2RX".
> > +maintainers:
> > + - Pratyush Yadav
> > +
> > +properties:
> > + compatible:
> > +items:
> > + - const: ti,csi2rx
> > +
> > + dmas:
> > +description: RX DMA Channel 0
>
> items:
> - description: RX DMA Channel 0
>
> Or just 'maxItems: 1'
>
> > +
> > + dma-names:
> > +items:
> > + - const: rx0
> > +
> > + reg:
> > +maxItems: 1
> > +description: Base address and size of the TI wrapper registers.
>
> That's all 'reg' properties, drop 'description'.
According to SPRUIL1B, there are four register banks for the CSI_RX_IF,
and two register banks for the DPHY_RX. What's your plan to support
these ? Not everything need to be implemented at once, but backward
compatibility need to be taken into account in the design.
> > +
> > + power-domains:
> > +maxItems: 1
> > +description:
> > + PM domain provider node and an args specifier containing
> > + the device id value.
>
> Drop.
>
> > +
> > + ranges: true
> > +
> > + "#address-cells":
> > +const: 2
> > +
> > + "#size-cells":
> > +const: 2
> > +
> > +patternProperties:
> > + "csi-bridge@":
>
> "^csi-bridge@"
>
> > +type: object
> > +description: CSI2 bridge node.
>
> Just an empty node?
Even if the node is optional, it would be useful to include it in the
example below, to show how it's supposed to be used.
> > +
> > +required:
> > + - compatible
> > + - reg
> > + - dmas
> > + - dma-names
> > + - power-domains
> > + - "#address-cells"
> > + - "#size-cells"
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > + - |
> > +#include
> > +
> > +ti_csi2rx0: ticsi2rx {
> > +compatible = "ti,csi2rx";
> > +dmas = <_udmap 0x4940>;
> > +dma-names = "rx0";
> > +reg = <0x0 0x450 0x0 0x1000>;
> > +power-domains = <_pds 26 TI_SCI_PD_EXCLUSIVE>;
> > +#address-cells = <2>;
> > +#size-cells = <2>;
> > +};
--
Regards,
Laurent Pinchart
Re: [PATCH 24/32] MAINTAINERS: update imi,rdacm2x-gmsl.yaml reference
On Fri, Apr 02, 2021 at 12:01:35PM +0300, Laurent Pinchart wrote:
> Hi Mauro,
>
> Thank you for the patch.
>
> On Thu, Apr 01, 2021 at 02:17:44PM +0200, Mauro Carvalho Chehab wrote:
> > The file name: Documentation/devicetree/bindings/media/i2c/rdacm2x-gmsl.yaml
> > should be, instead:
> > Documentation/devicetree/bindings/media/i2c/imi,rdacm2x-gmsl.yaml.
>
> While at it, shouldn't we also rename the file to imi,rdacm2x.yaml ?
I spoke too fast, sorry. There are RDACM2x cameras that use FPD-Link III
(RDACM23, 24, 25, 28, 29).
> > Update its cross-reference accordingly.
> >
> > Fixes: 34009bffc1c6 ("media: i2c: Add RDACM20 driver")
> > Fixes: e9f817689789 ("media: dt-bindings: media: i2c: Add bindings for IMI
> > RDACM2x")
> > Signed-off-by: Mauro Carvalho Chehab
Reviewed-by: Laurent Pinchart
> > ---
> > MAINTAINERS | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 1644b6e9697c..b405ee71f730 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -15258,7 +15258,7 @@ M: Laurent Pinchart
> >
> > M: Niklas Söderlund
> > L: linux-me...@vger.kernel.org
> > S: Maintained
> > -F: Documentation/devicetree/bindings/media/i2c/rdacm2x-gmsl.yaml
> > +F: Documentation/devicetree/bindings/media/i2c/imi,rdacm2x-gmsl.yaml
> > F: drivers/media/i2c/max9271.c
> > F: drivers/media/i2c/max9271.h
> > F: drivers/media/i2c/rdacm21.c
--
Regards,
Laurent Pinchart
Re: [PATCH 24/32] MAINTAINERS: update imi,rdacm2x-gmsl.yaml reference
Hi Mauro,
Thank you for the patch.
On Thu, Apr 01, 2021 at 02:17:44PM +0200, Mauro Carvalho Chehab wrote:
> The file name: Documentation/devicetree/bindings/media/i2c/rdacm2x-gmsl.yaml
> should be, instead:
> Documentation/devicetree/bindings/media/i2c/imi,rdacm2x-gmsl.yaml.
While at it, shouldn't we also rename the file to imi,rdacm2x.yaml ?
> Update its cross-reference accordingly.
>
> Fixes: 34009bffc1c6 ("media: i2c: Add RDACM20 driver")
> Fixes: e9f817689789 ("media: dt-bindings: media: i2c: Add bindings for IMI
> RDACM2x")
> Signed-off-by: Mauro Carvalho Chehab
> ---
> MAINTAINERS | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 1644b6e9697c..b405ee71f730 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -15258,7 +15258,7 @@ M:Laurent Pinchart
>
> M: Niklas Söderlund
> L: linux-me...@vger.kernel.org
> S: Maintained
> -F: Documentation/devicetree/bindings/media/i2c/rdacm2x-gmsl.yaml
> +F: Documentation/devicetree/bindings/media/i2c/imi,rdacm2x-gmsl.yaml
> F: drivers/media/i2c/max9271.c
> F: drivers/media/i2c/max9271.h
> F: drivers/media/i2c/rdacm21.c
--
Regards,
Laurent Pinchart
Re: [PATCH v6 4/5] drm/bridge: anx7625: add HDCP support
Hi Xin,
On Fri, Apr 02, 2021 at 10:27:08AM +0800, Xin Ji wrote:
> On Mon, Mar 29, 2021 at 02:02:08PM -0400, Sean Paul wrote:
> > On Mon, Mar 29, 2021 at 6:27 AM Xin Ji wrote:
> > >
> > > On Thu, Mar 25, 2021 at 02:19:23PM -0400, Sean Paul wrote:
> > > > On Fri, Mar 19, 2021 at 2:35 AM Xin Ji wrote:
> > > > >
> > > > > Add HDCP feature, enable HDCP function through chip internal key
> > > > > and downstream's capability.
> > > > >
> > > > > Signed-off-by: Xin Ji
> > > > > ---
> >
> > /snip
> >
> > > > > static void anx7625_dp_start(struct anx7625_data *ctx)
> > > > > {
> > > > > int ret;
> > > > > @@ -643,6 +787,9 @@ static void anx7625_dp_start(struct anx7625_data
> > > > > *ctx)
> > > > > return;
> > > > > }
> > > > >
> > > > > + /* HDCP config */
> > > > > + anx7625_hdcp_setting(ctx);
> > > >
> > > > You should really use the "Content Protection" property to
> > > > enable/disable HDCP instead of force-enabling it at all times.
> > >
> > > Hi Sean, it's hard to implement "Content Protection" property, we have
> > > implemented HDCP in firmware, it is not compatible with it. We don't
> > > have interface to get Downstream Cert.
> > > Thanks,
> > > Xin
> >
> > Hi Xin,
> > I'm sorry, I don't understand what you mean when you say you don't
> > have an interface to get Downstream Cert.
> >
> > The Content Protection property is just a means through which
> > userspace can turn on and turn off HDCP when it needs. As far as I can
> > tell, your patch turns on HDCP when the display is enabled and leaves
> > it on until it is disabled. This is undesirable since it forces HDCP
> > on the user.
> >
> > Is it impossible to enable/disable HDCP outside of display
> > enable/disable on your hardware?
>
> Hi Sean, I have commit a test patch on google review site, can you
> please help to review it? I'll use Connector's ".atomic_check()"
> interface to detect Content Protection property change.
> (https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/2674580)
Please note that upstream review happens on mailing lists, not in
gerrit. Internal reviews for Chrome OS development are certainly fine
there, but that will not mean the patch will then be accepted upstream
as-is, it will still need to go through the upstream review process,
without any shortcut. I strongly recommend using an upstream-first
strategy, with public review.
> > > > > +
> > > > > if (ctx->pdata.is_dpi)
> > > > > ret = anx7625_dpi_config(ctx);
> > > > > else
> >
> > /snip
--
Regards,
Laurent Pinchart
Re: [PATCH 13/16] media: ti-vpe: csi2rx: Add CSI2RX support
On Wed, Mar 31, 2021 at 10:44:57AM -0500, Benoit Parrot wrote: > Pratyush, > > Tomi Valkeinen wrote on Wed [2021-Mar-31 > 09:06:35 +0300]: > > Hi, > > > > On 30/03/2021 20:33, Pratyush Yadav wrote: > > > TI's J721E uses the Cadence CSI2RX and DPHY peripherals to facilitate > > > capture over a CSI-2 bus. > > > > > > The Cadence CSI2RX IP acts as a bridge between the TI specific parts and > > > the CSI-2 protocol parts. TI then has a wrapper on top of this bridge > > > called the SHIM layer. It takes in data from stream 0, repacks it, and > > > sends it to memory over PSI-L DMA. > > > > > > This driver acts as the "front end" to V4L2 client applications. It > > > implements the required ioctls and buffer operations, passes the > > > necessary calls on to the bridge, programs the SHIM layer, and performs > > > DMA via the dmaengine API to finally return the data to a buffer > > > supplied by the application. > > > > > > Signed-off-by: Pratyush Yadav > > > --- > > > MAINTAINERS | 7 + > > > drivers/media/platform/Kconfig| 11 + > > > drivers/media/platform/ti-vpe/Makefile| 1 + > > > drivers/media/platform/ti-vpe/ti-csi2rx.c | 964 ++ > > > 4 files changed, 983 insertions(+) > > > create mode 100644 drivers/media/platform/ti-vpe/ti-csi2rx.c > > > > Some quick comments: > > > > "ti-vpe" directory is not correct, this has nothing to do with VPE. That > > said, the directory has already been abused by having CAL driver there, > > perhaps we should rename the directory just to "ti". But if we do that, > > I think we should have subdirs for cal, vpe and this new one. > > I agree with Tomi here. This should create a ti directory under > media/platform and then add a directory under that specifically for this > driver/IP as a first step. Not sure what the correct name for that > directory should be but it should meaningful. As a follow on step then the > other drivers can be relocated to a proper directory structure. +1, including for the relocation if possible. > > "ti-csi2rx" is rather generic name. TI has had CSI-2 RX IPs before (CAL) > > and probably will also have new ones in the future. If there's no clear > > model name for the IP, as I think is the case here, it's probably best > > to just use the SoC model in the name. E.g. the DSS on J7 is > > "ti,j721e-dss". > > > > This driver implements the legacy video API. I think it would be better > > (and easier to maintain) to only implement the media-controller API, > > unless you specifically need to support the legacy API for existing > > userspace. > > We just went through a major rework with CAL to make it media controller > compatible in order to be able to handle CSI2 virtual channels. > I think as this is a new driver/IP which perform the same type of service > it makes sense to make use the more current API instead of the legacy one. +2 :-) -- Regards, Laurent Pinchart
Re: RFC: dt-binding: media: document ON Semi AR0521 sensor bindings
Hi Krzysztof,
On Tue, Mar 30, 2021 at 11:17:54AM +0200, Krzysztof Hałasa wrote:
> Laurent Pinchart writes:
>
> >> + reg:
> >> +description: I2C bus address of the sensor device
> >
> > You can drop this, it's implicit for I2C devices.
>
> Do you mean just dropping these two lines (and MaxItems: 1), and leaving
> "reg" in "required" and in the example? E.g.:
I meant dropping the description, sorry. You need to keep
reg:
maxItems: 1
> ...
> required:
> - compatible
> - reg
> - clocks
> - clock-names
> - port
>
> additionalProperties: false
>
> examples:
> - |
> #include
> #include
>
> i2c {
> #address-cells = <1>;
> #size-cells = <0>;
>
> ar0521: camera-sensor@36 {
> compatible = "onnn,ar0521";
> reg = <0x36>;
> pinctrl-names = "default";
>
> ...
>
> It protests with:
>
> Documentation/devicetree/bindings/media/i2c/onnn,ar0521.example.dt.yaml:
> camera-sensor@36: 'reg' does not match any of the regexes: 'pinctrl-[0-9]+'
> From schema:
> /usr/src/linux/imx6/Documentation/devicetree/bindings/media/i2c/onnn,ar0521.yaml
>
> Thus I'm currently leaving it as is.
--
Regards,
Laurent Pinchart
Re: [PATCH 3/3] drm/bridge: ti-sn65dsi86: Properly get the EDID, but only if refclk
Hi Doug,
On Mon, Mar 29, 2021 at 07:57:05PM -0700, Doug Anderson wrote:
> On Tue, Mar 16, 2021 at 5:44 PM Doug Anderson wrote:
> > On Tue, Mar 16, 2021 at 2:46 PM Laurent Pinchart wrote:
> > > On Mon, Mar 15, 2021 at 09:25:37AM -0700, Doug Anderson wrote:
> > > > On Sat, Mar 13, 2021 at 1:17 PM Laurent Pinchart wrote:
> > > > > On Thu, Mar 04, 2021 at 03:52:01PM -0800, Douglas Anderson wrote:
> > > > > > In commit 58074b08c04a ("drm/bridge: ti-sn65dsi86: Read EDID blob
> > > > > > over
> > > > > > DDC") we attempted to make the ti-sn65dsi86 bridge properly read the
> > > > > > EDID from the panel. That commit kinda worked but it had some
> > > > > > serious
> > > > > > problems.
> > > > > >
> > > > > > The problems all stem from the fact that userspace wants to be able
> > > > > > to
> > > > > > read the EDID before it explicitly enables the panel. For eDP
> > > > > > panels,
> > > > > > though, we don't actually power the panel up until the pre-enable
> > > > > > stage and the pre-enable call happens right before the enable call
> > > > > > with no way to interject in-between. For eDP panels, you can't read
> > > > > > the EDID until you power the panel. The result was that
> > > > > > ti_sn_bridge_connector_get_modes() was always failing to read the
> > > > > > EDID
> > > > > > (falling back to what drm_panel_get_modes() returned) until _after_
> > > > > > the EDID was needed.
> > > > > >
> > > > > > To make it concrete, on my system I saw this happen:
> > > > > > 1. We'd attach the bridge.
> > > > > > 2. Userspace would ask for the EDID (several times). We'd try but
> > > > > > fail
> > > > > >to read the EDID over and over again and fall back to the
> > > > > > hardcoded
> > > > > >modes.
> > > > > > 3. Userspace would decide on a mode based only on the hardcoded
> > > > > > modes.
> > > > > > 4. Userspace would ask to turn the panel on.
> > > > > > 5. Userspace would (eventually) check the modes again (in Chrome OS
> > > > > >this happens on the handoff from the boot splash screen to the
> > > > > >browser). Now we'd read them properly and, if they were
> > > > > > different,
> > > > > >userspace would request to change the mode.
> > > > > >
> > > > > > The fact that userspace would always end up using the hardcoded
> > > > > > modes
> > > > > > at first significantly decreases the benefit of the EDID
> > > > > > reading. Also: if the modes were even a tiny bit different we'd end
> > > > > > up
> > > > > > doing a wasteful modeset and at boot.
> > > > >
> > > > > s/and at/at/ ?
> > > >
> > > > Sure, I can correct if/when I respin or it can be corrected when landed.
> > > >
> > > > > > diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > > > > > b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > > > > > index 491c9c4f32d1..af3fb4657af6 100644
> > > > > > --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > > > > > +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > > > > > @@ -16,6 +16,7 @@
> > > > > > #include
> > > > > > #include
> > > > > > #include
> > > > > > +#include
> > > > > >
> > > > > > #include
> > > > > >
> > > > > > @@ -130,6 +131,12 @@
> > > > > > * @ln_assign:Value to program to the LN_ASSIGN register.
> > > > > > * @ln_polrs: Value for the 4-bit LN_POLRS field of
> > > > > > SN_ENH_FRAME_REG.
> > > > > > *
> > > > > > + * @pre_enabled_early: If true we did an early pre_enable at
> > > > > > attach.
> > > > > > + * @pre_enable_timeout_work: Delayed work to undo the pre_enable
> > > > > > from attach
> > > > > > + * if a normal pre_enable never came.
> > > > >
> > > > > Could we simplify this by using the runtime PM autos
Re: [PATCH v2 3/3] drm/mediatek: in struct mtk_hdmi, replace conn field with curr_conn ptr
Hi Dafna,
Thank you for the patch.
On Mon, Mar 29, 2021 at 05:36:32PM +0200, Dafna Hirschfeld wrote:
> The mtk_hdmi does not support creating a bridge with a connector.
> Therefore the field 'conn' should be removed from the mtk_hdmi struct.
> It is replaced with a pointer curr_conn that points to the current
> connector which can be access through the global state.
>
> Signed-off-by: Dafna Hirschfeld
The patch looks good to me, but I'd squash it with 2/3 otherwise I think
you'll break bisection. On the other hand, given that the HDMI support
is already broken... :-)
Reviewed-by: Laurent Pinchart
But you need to make sure this patch will get backported to stable along
2/3, probably by adding a fixes tag. Or squashing it with 2/3, up to
you.
> ---
> drivers/gpu/drm/mediatek/mtk_hdmi.c | 19 ++-
> 1 file changed, 14 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/mediatek/mtk_hdmi.c
> b/drivers/gpu/drm/mediatek/mtk_hdmi.c
> index 1eeb211b1536..0d95d2cfe3de 100644
> --- a/drivers/gpu/drm/mediatek/mtk_hdmi.c
> +++ b/drivers/gpu/drm/mediatek/mtk_hdmi.c
> @@ -153,7 +153,7 @@ struct mtk_hdmi_conf {
> struct mtk_hdmi {
> struct drm_bridge bridge;
> struct drm_bridge *next_bridge;
> - struct drm_connector conn;
> + struct drm_connector *curr_conn;/* current connector (only valid when
> 'enabled') */
> struct device *dev;
> const struct mtk_hdmi_conf *conf;
> struct phy *phy;
> @@ -969,7 +969,7 @@ static int mtk_hdmi_setup_avi_infoframe(struct mtk_hdmi
> *hdmi,
> ssize_t err;
>
> err = drm_hdmi_avi_infoframe_from_display_mode(,
> ->conn, mode);
> +hdmi->curr_conn, mode);
> if (err < 0) {
> dev_err(hdmi->dev,
> "Failed to get AVI infoframe from mode: %zd\n", err);
> @@ -1049,7 +1049,7 @@ static int
> mtk_hdmi_setup_vendor_specific_infoframe(struct mtk_hdmi *hdmi,
> ssize_t err;
>
> err = drm_hdmi_vendor_infoframe_from_display_mode(,
> - >conn, mode);
> + hdmi->curr_conn,
> mode);
> if (err) {
> dev_err(hdmi->dev,
> "Failed to get vendor infoframe from mode: %zd\n", err);
> @@ -1322,6 +1322,8 @@ static void mtk_hdmi_bridge_atomic_disable(struct
> drm_bridge *bridge,
> clk_disable_unprepare(hdmi->clk[MTK_HDMI_CLK_HDMI_PIXEL]);
> clk_disable_unprepare(hdmi->clk[MTK_HDMI_CLK_HDMI_PLL]);
>
> + hdmi->curr_conn = NULL;
> +
> hdmi->enabled = false;
> }
>
> @@ -1385,8 +1387,13 @@ static void mtk_hdmi_send_infoframe(struct mtk_hdmi
> *hdmi,
> static void mtk_hdmi_bridge_atomic_enable(struct drm_bridge *bridge,
> struct drm_bridge_state *old_state)
> {
> + struct drm_atomic_state *state = old_state->base.state;
> struct mtk_hdmi *hdmi = hdmi_ctx_from_bridge(bridge);
>
> + /* Retrieve the connector through the atomic state. */
> + hdmi->curr_conn = drm_atomic_get_new_connector_for_encoder(state,
> +
> bridge->encoder);
> +
> mtk_hdmi_output_set_display_mode(hdmi, >mode);
> clk_prepare_enable(hdmi->clk[MTK_HDMI_CLK_HDMI_PLL]);
> clk_prepare_enable(hdmi->clk[MTK_HDMI_CLK_HDMI_PIXEL]);
> @@ -1625,8 +1632,10 @@ static int mtk_hdmi_audio_get_eld(struct device *dev,
> void *data, uint8_t *buf,
> {
> struct mtk_hdmi *hdmi = dev_get_drvdata(dev);
>
> - memcpy(buf, hdmi->conn.eld, min(sizeof(hdmi->conn.eld), len));
> -
> + if (hdmi->enabled)
> + memcpy(buf, hdmi->curr_conn->eld,
> min(sizeof(hdmi->curr_conn->eld), len));
> + else
> + memset(buf, 0, len);
> return 0;
> }
>
--
Regards,
Laurent Pinchart
Re: [PATCH v2 2/3] drm/mediatek: Don't support hdmi connector creation
.destroy = hdmi_conn_destroy,
> - .reset = drm_atomic_helper_connector_reset,
> - .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
> - .atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
> -};
> -
> -static const struct drm_connector_helper_funcs
> - mtk_hdmi_connector_helper_funcs = {
> - .get_modes = mtk_hdmi_conn_get_modes,
> - .mode_valid = mtk_hdmi_conn_mode_valid,
> - .best_encoder = mtk_hdmi_conn_best_enc,
> -};
> -
> static void mtk_hdmi_hpd_event(bool hpd, struct device *dev)
> {
> struct mtk_hdmi *hdmi = dev_get_drvdata(dev);
>
> - if (hdmi && hdmi->bridge.encoder && hdmi->bridge.encoder->dev)
> + if (hdmi && hdmi->bridge.encoder && hdmi->bridge.encoder->dev) {
> + static enum drm_connector_status status;
> +
> + status = mtk_hdmi_detect(hdmi);
> drm_helper_hpd_irq_event(hdmi->bridge.encoder->dev);
I think you can drop this, as drm_bridge_connector_hpd_cb() calls
drm_kms_helper_hotplug_event().
> + drm_bridge_hpd_notify(>bridge, status);
> + }
> }
>
> /*
> * Bridge callbacks
> */
>
> +static enum drm_connector_status mtk_hdmi_bridge_detect(struct drm_bridge
> *bridge)
> +{
> + struct mtk_hdmi *hdmi = hdmi_ctx_from_bridge(bridge);
> +
> + return mtk_hdmi_detect(hdmi);
> +}
> +
> +static struct edid *mtk_hdmi_bridge_get_edid(struct drm_bridge *bridge,
> + struct drm_connector *connector)
> +{
> + struct mtk_hdmi *hdmi = hdmi_ctx_from_bridge(bridge);
> +
> + return mtk_hdmi_get_edid(hdmi, connector);
As mtk_hdmi_get_edid() is only called here, you could inline it in this
function. Up to you.
> +}
> +
> static int mtk_hdmi_bridge_attach(struct drm_bridge *bridge,
> enum drm_bridge_attach_flags flags)
> {
> struct mtk_hdmi *hdmi = hdmi_ctx_from_bridge(bridge);
> int ret;
>
> - if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR) {
> - DRM_ERROR("Fix bridge driver to make connector optional!");
> + if (!(flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR)) {
> + DRM_ERROR("%s: The flag DRM_BRIDGE_ATTACH_NO_CONNECTOR must be
> supplied\n",
> + __func__);
> return -EINVAL;
> }
>
> - ret = drm_connector_init_with_ddc(bridge->encoder->dev, >conn,
> - _hdmi_connector_funcs,
> - DRM_MODE_CONNECTOR_HDMIA,
> - hdmi->ddc_adpt);
> - if (ret) {
> - dev_err(hdmi->dev, "Failed to initialize connector: %d\n", ret);
> - return ret;
> - }
> - drm_connector_helper_add(>conn, _hdmi_connector_helper_funcs);
> -
> - hdmi->conn.polled = DRM_CONNECTOR_POLL_HPD;
> - hdmi->conn.interlace_allowed = true;
> - hdmi->conn.doublescan_allowed = false;
> -
> - ret = drm_connector_attach_encoder(>conn,
> - bridge->encoder);
> - if (ret) {
> - dev_err(hdmi->dev,
> - "Failed to attach connector to encoder: %d\n", ret);
> - return ret;
> - }
> -
> if (hdmi->next_bridge) {
> ret = drm_bridge_attach(bridge->encoder, hdmi->next_bridge,
> bridge, flags);
> @@ -1444,6 +1397,7 @@ static void mtk_hdmi_bridge_atomic_enable(struct
> drm_bridge *bridge,
> }
>
> static const struct drm_bridge_funcs mtk_hdmi_bridge_funcs = {
> + .mode_valid = mtk_hdmi_bridge_mode_valid,
> .atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,
> .atomic_destroy_state = drm_atomic_helper_bridge_destroy_state,
> .atomic_reset = drm_atomic_helper_bridge_reset,
> @@ -1454,6 +1408,8 @@ static const struct drm_bridge_funcs
> mtk_hdmi_bridge_funcs = {
> .mode_set = mtk_hdmi_bridge_mode_set,
> .atomic_pre_enable = mtk_hdmi_bridge_atomic_pre_enable,
> .atomic_enable = mtk_hdmi_bridge_atomic_enable,
> + .detect = mtk_hdmi_bridge_detect,
> + .get_edid = mtk_hdmi_bridge_get_edid,
> };
>
> static int mtk_hdmi_dt_parse_pdata(struct mtk_hdmi *hdmi,
> @@ -1762,6 +1718,9 @@ static int mtk_drm_hdmi_probe(struct platform_device
> *pdev)
>
> hdmi->bridge.funcs = _hdmi_bridge_funcs;
> hdmi->bridge.of_node = pdev->dev.of_node;
> + hdmi->bridge.ops = DRM_BRIDGE_OP_DETECT | DRM_BRIDGE_OP_EDID
> +| DRM_BRIDGE_OP_HPD;
Nitpicking, I'd align the | to the =.
Reviewed-by: Laurent Pinchart
> + hdmi->bridge.type = DRM_MODE_CONNECTOR_HDMIA;
> drm_bridge_add(>bridge);
>
> ret = mtk_hdmi_clk_enable_audio(hdmi);
--
Regards,
Laurent Pinchart
Re: [PATCH v2 1/3] drm/mediatek: Switch the hdmi bridge ops to the atomic versions
Hi Dafna,
Thank you for the patch.
On Mon, Mar 29, 2021 at 05:36:30PM +0200, Dafna Hirschfeld wrote:
> The bridge operation '.enable' and the audio cb '.get_eld'
> access hdmi->conn. In the future we will want to support
> the flag DRM_BRIDGE_ATTACH_NO_CONNECTOR and then we will
> not have direct access to the connector.
> The atomic version '.atomic_enable' allows accessing the
> current connector from the state.
> This patch switches the bridge to the atomic version to
> prepare access to the connector in later patches.
>
> Signed-off-by: Dafna Hirschfeld
Reviewed-by: Laurent Pinchart
> ---
> drivers/gpu/drm/mediatek/mtk_hdmi.c | 23 +++
> 1 file changed, 15 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/mediatek/mtk_hdmi.c
> b/drivers/gpu/drm/mediatek/mtk_hdmi.c
> index 8ee55f9e2954..f2c810b767ef 100644
> --- a/drivers/gpu/drm/mediatek/mtk_hdmi.c
> +++ b/drivers/gpu/drm/mediatek/mtk_hdmi.c
> @@ -1357,7 +1357,8 @@ static bool mtk_hdmi_bridge_mode_fixup(struct
> drm_bridge *bridge,
> return true;
> }
>
> -static void mtk_hdmi_bridge_disable(struct drm_bridge *bridge)
> +static void mtk_hdmi_bridge_atomic_disable(struct drm_bridge *bridge,
> +struct drm_bridge_state
> *old_bridge_state)
> {
> struct mtk_hdmi *hdmi = hdmi_ctx_from_bridge(bridge);
>
> @@ -1371,7 +1372,8 @@ static void mtk_hdmi_bridge_disable(struct drm_bridge
> *bridge)
> hdmi->enabled = false;
> }
>
> -static void mtk_hdmi_bridge_post_disable(struct drm_bridge *bridge)
> +static void mtk_hdmi_bridge_atomic_post_disable(struct drm_bridge *bridge,
> + struct drm_bridge_state
> *old_state)
> {
> struct mtk_hdmi *hdmi = hdmi_ctx_from_bridge(bridge);
>
> @@ -1406,7 +1408,8 @@ static void mtk_hdmi_bridge_mode_set(struct drm_bridge
> *bridge,
> drm_mode_copy(>mode, adjusted_mode);
> }
>
> -static void mtk_hdmi_bridge_pre_enable(struct drm_bridge *bridge)
> +static void mtk_hdmi_bridge_atomic_pre_enable(struct drm_bridge *bridge,
> + struct drm_bridge_state
> *old_state)
> {
> struct mtk_hdmi *hdmi = hdmi_ctx_from_bridge(bridge);
>
> @@ -1426,7 +1429,8 @@ static void mtk_hdmi_send_infoframe(struct mtk_hdmi
> *hdmi,
> mtk_hdmi_setup_vendor_specific_infoframe(hdmi, mode);
> }
>
> -static void mtk_hdmi_bridge_enable(struct drm_bridge *bridge)
> +static void mtk_hdmi_bridge_atomic_enable(struct drm_bridge *bridge,
> + struct drm_bridge_state *old_state)
> {
> struct mtk_hdmi *hdmi = hdmi_ctx_from_bridge(bridge);
>
> @@ -1440,13 +1444,16 @@ static void mtk_hdmi_bridge_enable(struct drm_bridge
> *bridge)
> }
>
> static const struct drm_bridge_funcs mtk_hdmi_bridge_funcs = {
> + .atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,
> + .atomic_destroy_state = drm_atomic_helper_bridge_destroy_state,
> + .atomic_reset = drm_atomic_helper_bridge_reset,
> .attach = mtk_hdmi_bridge_attach,
> .mode_fixup = mtk_hdmi_bridge_mode_fixup,
> - .disable = mtk_hdmi_bridge_disable,
> - .post_disable = mtk_hdmi_bridge_post_disable,
> + .atomic_disable = mtk_hdmi_bridge_atomic_disable,
> + .atomic_post_disable = mtk_hdmi_bridge_atomic_post_disable,
> .mode_set = mtk_hdmi_bridge_mode_set,
> - .pre_enable = mtk_hdmi_bridge_pre_enable,
> - .enable = mtk_hdmi_bridge_enable,
> + .atomic_pre_enable = mtk_hdmi_bridge_atomic_pre_enable,
> + .atomic_enable = mtk_hdmi_bridge_atomic_enable,
> };
>
> static int mtk_hdmi_dt_parse_pdata(struct mtk_hdmi *hdmi,
--
Regards,
Laurent Pinchart
Re: [PATCH v5 0/5] Add r8a77965 DRIF support
Hi Hans, On Sat, Mar 27, 2021 at 11:05:01AM +0100, Hans Verkuil wrote: > On 21/10/2020 23:43, Laurent Pinchart wrote: > > On Wed, Oct 21, 2020 at 02:53:27PM +0100, Fabrizio Castro wrote: > >> Dear All, > >> > >> this series is to add DRIF support for the r8a77965 > >> (a.k.a. R-Car M3-N). Version 5 fixes a warning reported > >> by 'make dt_binding_check', as reported by Rob. > > > > Patch 1/5 to 4/5 taken in my tree, I'll send a pull request to > > linux-media when the merge window closes. I expect Geert to handle 5/5. > > Patch 5 has been merged, but patches 1-4 aren't. I don't think there > was a PR for it. For some reason these patches are delegated to me in > patchwork. I've now delegated them to you for further processing. I've just sent a pull request, for these and other miscellaneous changes. > >> Fabrizio Castro (5): > >> MAINTAINERS: Update MAINTAINERS for Renesas DRIF driver > >> media: dt-bindings: media: renesas,drif: Convert to json-schema > >> media: dt-bindings: media: renesas,drif: Add r8a77990 support > >> media: dt-bindings: media: renesas,drif: Add r8a77965 support > >> arm64: dts: r8a77965: Add DRIF support > >> > >> .../bindings/media/renesas,drif.txt | 177 --- > >> .../bindings/media/renesas,drif.yaml | 279 ++ > >> MAINTAINERS | 4 +- > >> arch/arm64/boot/dts/renesas/r8a77965.dtsi | 120 > >> 4 files changed, 401 insertions(+), 179 deletions(-) > >> delete mode 100644 > >> Documentation/devicetree/bindings/media/renesas,drif.txt > >> create mode 100644 > >> Documentation/devicetree/bindings/media/renesas,drif.yaml -- Regards, Laurent Pinchart
Re: [PATCH v4 2/3] drm/encoder: Add macro drmm_plain_encoder_alloc()
Hi Paul, Thank you for the patch. On Sat, Mar 27, 2021 at 11:57:41AM +, Paul Cercueil wrote: > This performs the same operation as drmm_encoder_alloc(), but > only allocates and returns a struct drm_encoder instance. > > v4: Rename macro drmm_plain_encoder_alloc() and move to > . Since it's not "simple" anymore it > will now take funcs/name arguments as well. > > Signed-off-by: Paul Cercueil Reviewed-by: Laurent Pinchart > --- > include/drm/drm_encoder.h | 18 ++ > 1 file changed, 18 insertions(+) > > diff --git a/include/drm/drm_encoder.h b/include/drm/drm_encoder.h > index 5bf78b5bcb2b..6e91a0280f31 100644 > --- a/include/drm/drm_encoder.h > +++ b/include/drm/drm_encoder.h > @@ -224,6 +224,24 @@ void *__drmm_encoder_alloc(struct drm_device *dev, > offsetof(type, member), funcs, \ > encoder_type, name, ##__VA_ARGS__)) > > +/** > + * drmm_plain_encoder_alloc - Allocate and initialize an encoder > + * @dev: drm device > + * @funcs: callbacks for this encoder (optional) > + * @encoder_type: user visible type of the encoder > + * @name: printf style format string for the encoder name, or NULL for > default name > + * > + * This is a simplified version of drmm_encoder_alloc(), which only allocates > + * and returns a struct drm_encoder instance, with no subclassing. > + * > + * Returns: > + * Pointer to the new drm_encoder struct, or ERR_PTR on failure. > + */ > +#define drmm_plain_encoder_alloc(dev, funcs, encoder_type, name, ...) \ > + ((struct drm_encoder *) \ > + __drmm_encoder_alloc(dev, sizeof(struct drm_encoder), \ > + 0, funcs, encoder_type, name, ##__VA_ARGS__)) > + > /** > * drm_encoder_index - find the index of a registered encoder > * @encoder: encoder to find index for -- Regards, Laurent Pinchart
Re: [PATCH] [v2] drivers: gpu: drm: Remove duplicate declaration
Hi Wan,
Thank you for the patch.
On Thu, Mar 25, 2021 at 07:10:24PM +0800, Wan Jiabing wrote:
> struct dss_device has been declared. Remove the duplicate.
> And sort these forward declarations alphabetically.
>
> Signed-off-by: Wan Jiabing
Reviewed-by: Laurent Pinchart
Tomi, I assume you'll handle this patch, please let me know if you don't
plan to do so.
> ---
> Changelog:
> v2:
> - Sort forward declarations alphabetically.
> ---
> drivers/gpu/drm/omapdrm/dss/omapdss.h | 9 -
> 1 file changed, 4 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/omapdrm/dss/omapdss.h
> b/drivers/gpu/drm/omapdrm/dss/omapdss.h
> index a40abeafd2e9..040d5a3e33d6 100644
> --- a/drivers/gpu/drm/omapdrm/dss/omapdss.h
> +++ b/drivers/gpu/drm/omapdrm/dss/omapdss.h
> @@ -48,16 +48,15 @@
> #define DISPC_IRQ_ACBIAS_COUNT_STAT3 (1 << 29)
> #define DISPC_IRQ_FRAMEDONE3 (1 << 30)
>
> -struct dss_device;
> -struct omap_drm_private;
> -struct omap_dss_device;
> struct dispc_device;
> +struct drm_connector;
> struct dss_device;
> struct dss_lcd_mgr_config;
> +struct hdmi_avi_infoframe;
> +struct omap_drm_private;
> +struct omap_dss_device;
> struct snd_aes_iec958;
> struct snd_cea_861_aud_if;
> -struct hdmi_avi_infoframe;
> -struct drm_connector;
>
> enum omap_display_type {
> OMAP_DISPLAY_TYPE_NONE = 0,
--
Regards,
Laurent Pinchart
Re: [PATCH 1/2] media: videobuf2: use dmabuf size for length
; plane < vb->num_planes; ++plane) {
> - length = (b->memory == VB2_MEMORY_USERPTR ||
> - b->memory == VB2_MEMORY_DMABUF)
> -? b->m.planes[plane].length
> + length = b->memory == VB2_MEMORY_USERPTR
> + ? b->m.planes[plane].length
> : vb->planes[plane].length;
> bytesused = b->m.planes[plane].bytesused
> ? b->m.planes[plane].bytesused : length;
> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> index 8d15f6ccc4b4..79b3b2893513 100644
> --- a/include/uapi/linux/videodev2.h
> +++ b/include/uapi/linux/videodev2.h
> @@ -968,7 +968,9 @@ struct v4l2_requestbuffers {
> /**
> * struct v4l2_plane - plane info for multi-planar buffers
> * @bytesused: number of bytes occupied by data in the plane
> (payload)
> - * @length: size of this plane (NOT the payload) in bytes
> + * @length: size of this plane (NOT the payload) in bytes. Filled
> + * by userspace for USERPTR and by the driver for DMABUF
> + * and MMAP.
> * @mem_offset: when memory in the associated struct
> v4l2_buffer is
> * V4L2_MEMORY_MMAP, equals the offset from the start of
> * the device memory for this plane (or is a "cookie" that
> @@ -1025,7 +1027,8 @@ struct v4l2_plane {
> * @m: union of @offset, @userptr, @planes and @fd
> * @length: size in bytes of the buffer (NOT its payload) for single-plane
> * buffers (when type != *_MPLANE); number of elements in the
> - * planes array for multi-plane buffers
> + * planes array for multi-plane buffers. Filled by userspace for
> + * USERPTR and by the driver for DMABUF and MMAP.
> * @reserved2: drivers and applications must zero this field
> * @request_fd: fd of the request that this buffer should use
> * @reserved:for backwards compatibility with applications that do
> not know
--
Regards,
Laurent Pinchart
Re: [PATCH] drivers: gpu: drm: Remove duplicate declaration
Hi Wan, Thank you for the patch. On Thu, Mar 25, 2021 at 12:50:19PM +0800, Wan Jiabing wrote: > struct dss_device has been declared at 51st line. > Remove the duplicate. > > Signed-off-by: Wan Jiabing > --- > drivers/gpu/drm/omapdrm/dss/omapdss.h | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/drivers/gpu/drm/omapdrm/dss/omapdss.h > b/drivers/gpu/drm/omapdrm/dss/omapdss.h > index a40abeafd2e9..2658aadee09a 100644 > --- a/drivers/gpu/drm/omapdrm/dss/omapdss.h > +++ b/drivers/gpu/drm/omapdrm/dss/omapdss.h > @@ -52,7 +52,6 @@ struct dss_device; > struct omap_drm_private; > struct omap_dss_device; > struct dispc_device; > -struct dss_device; > struct dss_lcd_mgr_config; > struct snd_aes_iec958; > struct snd_cea_861_aud_if; While at it, could you sort these forward declarations alphabetically, so that duplicates are easier to see ? -- Regards, Laurent Pinchart
Re: [PATCH v2] dt-bindings: media: video-interfaces: Drop the example
Hi Rob,
Thank you for the patch.
On Wed, Mar 24, 2021 at 02:22:53PM -0600, Rob Herring wrote:
> The example in video-interfaces.yaml uses a bunch of undocumented
> bindings which will cause warnings when undocumented compatible checks
> are enabled. The example could be fixed to use documented bindings, but
> doing so would just duplicate other examples. So let's just remove the
> example.
>
> Cc: Mauro Carvalho Chehab
> Cc: Sakari Ailus
> Cc: Laurent Pinchart
> Cc: linux-me...@vger.kernel.org
> Signed-off-by: Rob Herring
Reviewed-by: Laurent Pinchart
> ---
> v2: Drop instead of fixing the example
>
> .../bindings/media/video-interfaces.yaml | 127 --
> 1 file changed, 127 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/media/video-interfaces.yaml
> b/Documentation/devicetree/bindings/media/video-interfaces.yaml
> index 0a7a73fd59f2..4391dce2caee 100644
> --- a/Documentation/devicetree/bindings/media/video-interfaces.yaml
> +++ b/Documentation/devicetree/bindings/media/video-interfaces.yaml
> @@ -215,130 +215,3 @@ properties:
>CCP2, for instance.
>
> additionalProperties: true
> -
> -examples:
> - # The example snippet below describes two data pipelines. ov772x and
> imx074
> - # are camera sensors with a parallel and serial (MIPI CSI-2) video bus
> - # respectively. Both sensors are on the I2C control bus corresponding to
> the
> - # i2c0 controller node. ov772x sensor is linked directly to the ceu0 video
> - # host interface. imx074 is linked to ceu0 through the MIPI CSI-2 receiver
> - # (csi2). ceu0 has a (single) DMA engine writing captured data to memory.
> - # ceu0 node has a single 'port' node which may indicate that at any time
> - # only one of the following data pipelines can be active:
> - # ov772x -> ceu0 or imx074 -> csi2 -> ceu0.
> - - |
> -ceu@fe91 {
> -compatible = "renesas,sh-mobile-ceu";
> -reg = <0xfe91 0xa0>;
> -interrupts = <0x880>;
> -
> -mclk: master_clock {
> -compatible = "renesas,ceu-clock";
> -#clock-cells = <1>;
> -clock-frequency = <5000>; /* Max clock frequency */
> -clock-output-names = "mclk";
> -};
> -
> -port {
> -#address-cells = <1>;
> -#size-cells = <0>;
> -
> -/* Parallel bus endpoint */
> -ceu0_1: endpoint@1 {
> -reg = <1>;/* Local endpoint # */
> -remote-endpoint = <_1_1>; /* Remote phandle */
> -bus-width = <8>; /* Used data lines */
> -data-shift = <2>; /* Lines 9:2 are used */
> -
> -/* If hsync-active/vsync-active are missing,
> - embedded BT.656 sync is used */
> -hsync-active = <0>; /* Active low */
> -vsync-active = <0>; /* Active low */
> -data-active = <1>; /* Active high */
> -pclk-sample = <1>; /* Rising */
> -};
> -
> -/* MIPI CSI-2 bus endpoint */
> -ceu0_0: endpoint@0 {
> -reg = <0>;
> -remote-endpoint = <_2>;
> -};
> -};
> -};
> -
> -i2c {
> -#address-cells = <1>;
> -#size-cells = <0>;
> -
> -camera@21 {
> -compatible = "ovti,ov772x";
> -reg = <0x21>;
> -vddio-supply = <>;
> -vddcore-supply = <>;
> -
> -clock-frequency = <2000>;
> -clocks = < 0>;
> -clock-names = "xclk";
> -
> -port {
> -/* With 1 endpoint per port no need for addresses. */
> -ov772x_1_1: endpoint {
> -bus-width = <8>;
> -remote-endpoint = <_1>;
> -hsync-active = <1>;
> -vsync-active = <0>; /* Who came up with an
> - inverter here ?... */
> -data-active = <1>;
> -pclk-sample = <1>;
> -};
> -};
> -};
> -
> -camera@1a {
> -compatible = "sony,imx074";
> -reg = <0x1a>;
> -vddio-supply = <>;
> -vddcore-supply = <>;
> -
> -clock-frequency = <3000>; /* Shared clock with ov772x_1 */
> -
Re: [PATCH] media: entity: A typo fix
Hi Bhaskar,
Thank you for the patch.
On Wed, Mar 24, 2021 at 06:51:00PM +0530, Bhaskar Chowdhury wrote:
>
> s/cariers/carriers/
>
> Signed-off-by: Bhaskar Chowdhury
Reviewed-by: Laurent Pinchart
> ---
> include/media/media-entity.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/include/media/media-entity.h b/include/media/media-entity.h
> index cbdfcb79d0d0..a90e2bf6baf7 100644
> --- a/include/media/media-entity.h
> +++ b/include/media/media-entity.h
> @@ -155,7 +155,7 @@ struct media_link {
> * uniquely identified by the pad number.
> * @PAD_SIGNAL_ANALOG:
> * The pad contains an analog signal. It can be Radio Frequency,
> - * Intermediate Frequency, a baseband signal or sub-cariers.
> + * Intermediate Frequency, a baseband signal or sub-carriers.
> * Tuner inputs, IF-PLL demodulators, composite and s-video signals
> * should use it.
> * @PAD_SIGNAL_DV:
--
Regards,
Laurent Pinchart
Re: [PATCH v1] MAINTAINERS: Update Maintainers of DRM Bridge Drivers
Hi Rob, On Wed, Mar 24, 2021 at 11:20:19AM +0100, Robert Foss wrote: > Add myself as co-maintainer of DRM Bridge Drivers. Repository > commit access has already been granted. > > https://gitlab.freedesktop.org/freedesktop/freedesktop/-/issues/338 > > Cc: Neil Armstrong > Cc: Laurent Pinchart > Cc: Jonas Karlman > Cc: Andrzej Hajda > Cc: Jernej Škrabec > Cc: Daniel Vetter > Signed-off-by: Robert Foss Acked-by: Laurent Pinchart and welcome :-) > --- > MAINTAINERS | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/MAINTAINERS b/MAINTAINERS > index 4b705ba51c54..16ace8f58649 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -5902,6 +5902,7 @@ F: drivers/gpu/drm/atmel-hlcdc/ > DRM DRIVERS FOR BRIDGE CHIPS > M: Andrzej Hajda > M: Neil Armstrong > +M: Robert Foss > R: Laurent Pinchart > R: Jonas Karlman > R: Jernej Skrabec -- Regards, Laurent Pinchart
Re: [PATCH v4 1/4] drm: sun4i: dsi: Use drm_of_find_panel_or_bridge
Hi Jagan,
On Wed, Mar 24, 2021 at 03:19:10PM +0530, Jagan Teki wrote:
> On Wed, Mar 24, 2021 at 3:09 PM Laurent Pinchart wrote:
> > On Wed, Mar 24, 2021 at 02:44:57PM +0530, Jagan Teki wrote:
> > > On Wed, Mar 24, 2021 at 8:18 AM Samuel Holland wrote:
> > > > On 3/23/21 5:53 PM, Laurent Pinchart wrote:
> > > > > On Mon, Mar 22, 2021 at 07:31:49PM +0530, Jagan Teki wrote:
> > > > >> Replace of_drm_find_panel with drm_of_find_panel_or_bridge
> > > > >> for finding panel, this indeed help to find the bridge if
> > > > >> bridge support added.
> > > > >>
> > > > >> Added NULL in bridge argument, same will replace with bridge
> > > > >> parameter once bridge supported.
> > > > >>
> > > > >> Signed-off-by: Jagan Teki
> > > > >
> > > > > Looks good, there should be no functional change.
> > > >
> > > > Actually this breaks all existing users of this driver, see below.
> > > >
> > > > > Reviewed-by: Laurent Pinchart
> > > > >
> > > > >> ---
> > > > >> Changes for v4, v3:
> > > > >> - none
> > > > >>
> > > > >> drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c | 11 ---
> > > > >> 1 file changed, 8 insertions(+), 3 deletions(-)
> > > > >>
> > > > >> diff --git a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> > > > >> b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> > > > >> index 4f5efcace68e..2e9e7b2d4145 100644
> > > > >> --- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> > > > >> +++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> > > > >> @@ -21,6 +21,7 @@
> > > > >>
> > > > >> #include
> > > > >> #include
> > > > >> +#include
> > > > >> #include
> > > > >> #include
> > > > >> #include
> > > > >> @@ -963,10 +964,14 @@ static int sun6i_dsi_attach(struct
> > > > >> mipi_dsi_host *host,
> > > > >> struct mipi_dsi_device *device)
> > > > >> {
> > > > >> struct sun6i_dsi *dsi = host_to_sun6i_dsi(host);
> > > > >> -struct drm_panel *panel =
> > > > >> of_drm_find_panel(device->dev.of_node);
> > > >
> > > > This is using the OF node of the DSI device, which is a direct child of
> > > > the DSI host's OF node. There is no OF graph involved.
> > > >
> > > > >> +struct drm_panel *panel;
> > > > >> +int ret;
> > > > >> +
> > > > >> +ret = drm_of_find_panel_or_bridge(dsi->dev->of_node, 0, 0,
> > > > >> + , NULL);
> > > >
> > > > However, this function expects to find the panel using OF graph. This
> > > > does not work with existing device trees (PinePhone, PineTab) which do
> > > > not use OF graph to connect the panel. And it cannot work, because the
> > > > DSI host's binding specifies a single port: the input port from the
> > > > display engine.
> > >
> > > Thanks for noticing this. I did understand your point and yes, I did
> > > mention the updated pipeline in previous versions and forgot to add it
> > > to this series.
> > >
> > > Here is the updated pipeline to make it work:
> > >
> > > https://patchwork.kernel.org/project/dri-devel/patch/20190524104252.20236-1-ja...@amarulasolutions.com/
> > >
> > > Let me know your comments on this, so I will add a patch for the
> > > above-affected DTS files.
> >
> > DT is an ABI, we need to ensure backward compatibility. Changes in
> > kernel drivers can't break devices that have an old DT.
>
> Thanks for your point.
>
> So, we need to choose APIs that would compatible with the old DT and
> new DT changes. Am I correct?
Yes, that's correct.
--
Regards,
Laurent Pinchart
Re: [PATCH v2 1/3] drm: bridge/panel: Cleanup connector on bridge detach
On Wed, Mar 24, 2021 at 10:39:52AM +0100, Daniel Vetter wrote:
> On Wed, Mar 24, 2021 at 04:15:37AM +0200, Laurent Pinchart wrote:
> > On Wed, Jan 20, 2021 at 06:38:03PM +0100, Daniel Vetter wrote:
> > > On Wed, Jan 20, 2021 at 6:12 PM Paul Cercueil wrote:
> > > > Le mer. 20 janv. 2021 à 17:03, Daniel Vetter a écrit :
> > > > > On Wed, Jan 20, 2021 at 1:35 PM Paul Cercueil wrote:
> > > > >>
> > > > >> If we don't call drm_connector_cleanup() manually in
> > > > >> panel_bridge_detach(), the connector will be cleaned up with the
> > > > >> other
> > > > >> DRM objects in the call to drm_mode_config_cleanup(). However,
> > > > >> since our
> > > > >> drm_connector is devm-allocated, by the time
> > > > >> drm_mode_config_cleanup()
> > > > >> will be called, our connector will be long gone. Therefore, the
> > > > >> connector must be cleaned up when the bridge is detached to avoid
> > > > >> use-after-free conditions.
> > > > >
> > > > > For -fixes this sounds ok, but for -next I think switching to drmm_
> > > > > would be much better.
> > > >
> > > > The API would need to change to have access to the drm_device struct,
> > > > though. That would be quite a big patch, there are a few dozens source
> > > > files that use this API already.
> > >
> > > Hm right pure drmm_ doesn't work for panel or bridge since it's
> > > usually a separate driver. But devm_ also doesn't work. I think what
> > > we need here is two-stage: first kmalloc the panel (or bridge, it's
> > > really the same) in the panel/bridge driver load. Then when we bind it
> > > to the drm_device we can tie it into the managed resources with
> > > drmm_add_action_or_reset. Passing the drm_device to the point where we
> > > allocate the panel/bridge doesn't work for these.
> > >
> > > I think minimally we need a FIXME here and ack from Laurent on how
> > > this should be solved at least, since panel bridge is used rather
> > > widely.
> >
> > Bridge removal is completely broken. If you unbind a bridge driver from
> > the device, the bridge will be unregistered and resources freed, without
> > the display driver knowing about this. The lifetime of the drm_bridge
> > structure itself isn't the only issue to be addressed here, it's broader
> > than that, and needs to consider that the display driver could be
> > calling the bridge operations concurrently to the removal.
>
> So for the "unloading bridge should first unload display" problem that was
> supposed to get fixed with device links. There was at least a patch for
> that, and I Rafel from pm side did all the core changes to make it work.
> But it didn't land I think, so things keep on sucking.
>
> Ofc the lifetime of the bridge structure is then an additional problem on
> top here.
There's a set of interesting problems. I don't think it's impossible,
but it will require someone with a good understanding of the problem (as
that person would really need to see the big picture, and take all use
cases into account), and a large amount of time and motivation.
> > We need a volunteer with enough motivation to solve this subsystem-wide
> > :-) In the meantime, whatever shortcut addresses immediate issues is
> > probably fine, as yak-shaving in this area would definitely not be
> > reasonable.
>
> I guess drm/bridge keeps on disappointing :-/
I usually blame the x86 folks for not caring enough about bridges
initially, resulting in it being a second class citizen ;-)
> > > > >> v2: Cleanup connector only if it was created
> > > > >>
> > > > >> Fixes: 13dfc0540a57 ("drm/bridge: Refactor out the panel wrapper
> > > > >> from the lvds-encoder bridge.")
> > > > >> Cc: # 4.12+
> > > > >> Cc: Andrzej Hajda
> > > > >> Cc: Neil Armstrong
> > > > >> Cc: Laurent Pinchart
> > > > >> Cc: Jonas Karlman
> > > > >> Cc: Jernej Skrabec
> > > > >> Signed-off-by: Paul Cercueil
> > > > >> ---
> > > > >> drivers/gpu/drm/bridge/panel.c | 6 ++
> > > > >> 1 file changed, 6 insertions(+)
> > > > >>
> > > > >> diff --git a/drivers/gpu/drm/bridge/panel.c
> > > > >> b/drivers/gpu/drm/bridge/panel.c
> > > > >> index 0ddc37551194..df86b0ee0549 100644
> > > > >> --- a/drivers/gpu/drm/bridge/panel.c
> > > > >> +++ b/drivers/gpu/drm/bridge/panel.c
> > > > >> @@ -87,6 +87,12 @@ static int panel_bridge_attach(struct drm_bridge
> > > > >> *bridge,
> > > > >>
> > > > >> static void panel_bridge_detach(struct drm_bridge *bridge)
> > > > >> {
> > > > >> +struct panel_bridge *panel_bridge =
> > > > >> drm_bridge_to_panel_bridge(bridge);
> > > > >> +struct drm_connector *connector = _bridge->connector;
> > > > >> +
> > > > >> +/* Cleanup the connector if we know it was initialized */
> > > > >> +if (!!panel_bridge->connector.dev)
> > > > >> +drm_connector_cleanup(connector);
> > > > >> }
> > > > >>
> > > > >> static void panel_bridge_pre_enable(struct drm_bridge *bridge)
--
Regards,
Laurent Pinchart
Re: [PATCH v4 1/4] drm: sun4i: dsi: Use drm_of_find_panel_or_bridge
Hi Jagan,
On Wed, Mar 24, 2021 at 02:44:57PM +0530, Jagan Teki wrote:
> On Wed, Mar 24, 2021 at 8:18 AM Samuel Holland wrote:
> > On 3/23/21 5:53 PM, Laurent Pinchart wrote:
> > > On Mon, Mar 22, 2021 at 07:31:49PM +0530, Jagan Teki wrote:
> > >> Replace of_drm_find_panel with drm_of_find_panel_or_bridge
> > >> for finding panel, this indeed help to find the bridge if
> > >> bridge support added.
> > >>
> > >> Added NULL in bridge argument, same will replace with bridge
> > >> parameter once bridge supported.
> > >>
> > >> Signed-off-by: Jagan Teki
> > >
> > > Looks good, there should be no functional change.
> >
> > Actually this breaks all existing users of this driver, see below.
> >
> > > Reviewed-by: Laurent Pinchart
> > >
> > >> ---
> > >> Changes for v4, v3:
> > >> - none
> > >>
> > >> drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c | 11 ---
> > >> 1 file changed, 8 insertions(+), 3 deletions(-)
> > >>
> > >> diff --git a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> > >> b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> > >> index 4f5efcace68e..2e9e7b2d4145 100644
> > >> --- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> > >> +++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> > >> @@ -21,6 +21,7 @@
> > >>
> > >> #include
> > >> #include
> > >> +#include
> > >> #include
> > >> #include
> > >> #include
> > >> @@ -963,10 +964,14 @@ static int sun6i_dsi_attach(struct mipi_dsi_host
> > >> *host,
> > >> struct mipi_dsi_device *device)
> > >> {
> > >> struct sun6i_dsi *dsi = host_to_sun6i_dsi(host);
> > >> -struct drm_panel *panel = of_drm_find_panel(device->dev.of_node);
> >
> > This is using the OF node of the DSI device, which is a direct child of
> > the DSI host's OF node. There is no OF graph involved.
> >
> > >> +struct drm_panel *panel;
> > >> +int ret;
> > >> +
> > >> +ret = drm_of_find_panel_or_bridge(dsi->dev->of_node, 0, 0,
> > >> + , NULL);
> >
> > However, this function expects to find the panel using OF graph. This
> > does not work with existing device trees (PinePhone, PineTab) which do
> > not use OF graph to connect the panel. And it cannot work, because the
> > DSI host's binding specifies a single port: the input port from the
> > display engine.
>
> Thanks for noticing this. I did understand your point and yes, I did
> mention the updated pipeline in previous versions and forgot to add it
> to this series.
>
> Here is the updated pipeline to make it work:
>
> https://patchwork.kernel.org/project/dri-devel/patch/20190524104252.20236-1-ja...@amarulasolutions.com/
>
> Let me know your comments on this, so I will add a patch for the
> above-affected DTS files.
DT is an ABI, we need to ensure backward compatibility. Changes in
kernel drivers can't break devices that have an old DT.
--
Regards,
Laurent Pinchart
Re: [V2][PATCH] drm: xlnx: zynqmp: release reset to DP controller before accessing DP registers
Hi Quanyang,
Thank you for the patch.
On Tue, Mar 23, 2021 at 10:55:01AM +0800, quanyang.w...@windriver.com wrote:
> From: Quanyang Wang
>
> When insmod zynqmp-dpsub.ko after rmmod it, system will hang with the
> error log as below:
>
> root@xilinx-zynqmp:~# insmod zynqmp-dpsub.ko
> [ 88.391289] [drm] Initialized zynqmp-dpsub 1.0.0 20130509 for
> fd4a.display on minor 0
> [ 88.529906] Console: switching to colour frame buffer device 128x48
> [ 88.549402] zynqmp-dpsub fd4a.display: [drm] fb0: zynqmp-dpsubdrm
> frame buffer device
> [ 88.571624] zynqmp-dpsub fd4a.display: ZynqMP DisplayPort Subsystem
> driver probed
> root@xilinx-zynqmp:~# rmmod zynqmp_dpsub
> [ 94.023404] Console: switching to colour dummy device 80x25
> root@xilinx-zynqmp:~# insmod zynqmp-dpsub.ko
>
>
> This is because that in zynqmp_dp_probe it tries to access some DP
> registers while the DP controller is still in the reset state. When
> running "rmmod zynqmp_dpsub", zynqmp_dp_reset(dp, true) in
> zynqmp_dp_phy_exit is called to force the DP controller into the reset
> state. Then insmod will call zynqmp_dp_probe to program the DP registers,
> but at this moment the DP controller hasn't been brought out of the reset
> state yet since the function zynqmp_dp_reset(dp, false) is called later and
> this will result the system hang.
>
> Releasing the reset to DP controller before any read/write operation to it
> will fix this issue. And for symmetry, move zynqmp_dp_reset() call from
> zynqmp_dp_phy_exit() to zynqmp_dp_remove().
>
> Signed-off-by: Quanyang Wang
Reviewed-by: Laurent Pinchart
> ---
>
> V2:
> According to Laurent's comments:
> - add zynqmp_dp_reset(dp, true) in error path in zynqmp_dp_probe
> - move the zynqmp_dp_reset() call from zynqmp_dp_phy_exit() to
> zynqmp_dp_remove()
>
> ---
> drivers/gpu/drm/xlnx/zynqmp_dp.c | 22 --
> 1 file changed, 12 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/xlnx/zynqmp_dp.c
> b/drivers/gpu/drm/xlnx/zynqmp_dp.c
> index 99158ee67d02..337adf0769ad 100644
> --- a/drivers/gpu/drm/xlnx/zynqmp_dp.c
> +++ b/drivers/gpu/drm/xlnx/zynqmp_dp.c
> @@ -402,10 +402,6 @@ static int zynqmp_dp_phy_init(struct zynqmp_dp *dp)
> }
> }
>
> - ret = zynqmp_dp_reset(dp, false);
> - if (ret < 0)
> - return ret;
> -
> zynqmp_dp_clr(dp, ZYNQMP_DP_PHY_RESET, ZYNQMP_DP_PHY_RESET_ALL_RESET);
>
> /*
> @@ -441,8 +437,6 @@ static void zynqmp_dp_phy_exit(struct zynqmp_dp *dp)
> ret);
> }
>
> - zynqmp_dp_reset(dp, true);
> -
> for (i = 0; i < dp->num_lanes; i++) {
> ret = phy_exit(dp->phy[i]);
> if (ret)
> @@ -1682,9 +1676,13 @@ int zynqmp_dp_probe(struct zynqmp_dpsub *dpsub, struct
> drm_device *drm)
> return PTR_ERR(dp->reset);
> }
>
> + ret = zynqmp_dp_reset(dp, false);
> + if (ret < 0)
> + return ret;
> +
> ret = zynqmp_dp_phy_probe(dp);
> if (ret)
> - return ret;
> + goto err_reset;
>
> /* Initialize the hardware. */
> zynqmp_dp_write(dp, ZYNQMP_DP_TX_PHY_POWER_DOWN,
> @@ -1696,7 +1694,7 @@ int zynqmp_dp_probe(struct zynqmp_dpsub *dpsub, struct
> drm_device *drm)
>
> ret = zynqmp_dp_phy_init(dp);
> if (ret)
> - return ret;
> + goto err_reset;
>
> zynqmp_dp_write(dp, ZYNQMP_DP_TRANSMITTER_ENABLE, 1);
>
> @@ -1708,15 +1706,18 @@ int zynqmp_dp_probe(struct zynqmp_dpsub *dpsub,
> struct drm_device *drm)
> zynqmp_dp_irq_handler, IRQF_ONESHOT,
> dev_name(dp->dev), dp);
> if (ret < 0)
> - goto error;
> + goto err_phy_exit;
>
> dev_dbg(dp->dev, "ZynqMP DisplayPort Tx probed with %u lanes\n",
> dp->num_lanes);
>
> return 0;
>
> -error:
> +err_phy_exit:
> zynqmp_dp_phy_exit(dp);
> +err_reset:
> + zynqmp_dp_reset(dp, true);
> +
> return ret;
> }
>
> @@ -1734,4 +1735,5 @@ void zynqmp_dp_remove(struct zynqmp_dpsub *dpsub)
> zynqmp_dp_write(dp, ZYNQMP_DP_INT_DS, 0x);
>
> zynqmp_dp_phy_exit(dp);
> + zynqmp_dp_reset(dp, true);
> }
--
Regards,
Laurent Pinchart
Re: [PATCH] arm64: dts: qcom: trogdor: Add no-hpd to DSI bridge node
Hi Stephen,
Thank you for the patch.
On Tue, Mar 23, 2021 at 07:55:34PM -0700, Stephen Boyd wrote:
> We should indicate that we're not using the HPD pin on this device, per
> the binding document. Otherwise if code in the future wants to enable
> HPD in the bridge when this property is absent we'll be wasting power
> powering hpd when we don't use it on trogdor boards. We didn't notice
> this before because the kernel driver blindly disables hpd, but that
> won't be true for much longer.
>
> Cc: Laurent Pinchart
> Cc: Douglas Anderson
> Fixes: 7ec3e67307f8 ("arm64: dts: qcom: sc7180-trogdor: add initial trogdor
> and lazor dt")
> Signed-off-by: Stephen Boyd
Reviewed-by: Laurent Pinchart
> ---
> arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
> b/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
> index 07c8b2c926c0..298af6d7fb4a 100644
> --- a/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
> @@ -595,6 +595,8 @@ sn65dsi86_bridge: bridge@2d {
> clocks = < RPMH_LN_BB_CLK3>;
> clock-names = "refclk";
>
> + no-hpd;
> +
> ports {
> #address-cells = <1>;
> #size-cells = <0>;
--
Regards,
Laurent Pinchart
Re: [PATCH v3] phy: zynqmp: Handle the clock enable/disable properly
Hi Manish,
Thank you for the patch.
On Tue, Mar 23, 2021 at 07:49:47PM +0530, Manish Narani wrote:
> The current driver is not handling the clock enable/disable operations
> properly. The clocks need to be handled correctly by enabling or
> disabling at appropriate places. This patch adds code to handle the
> same.
>
> Signed-off-by: Manish Narani
This looks good to me.
Reviewed-by: Laurent Pinchart
However, it would be really nice to make clock handling dynamic, to only
enable clocks that are needed by active PHYs. Keeping them enabled at
all times will waste power. It can be done on top of this patch. Is it
something you could work on ?
> ---
> drivers/phy/xilinx/phy-zynqmp.c | 57
> -
> 1 file changed, 50 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/phy/xilinx/phy-zynqmp.c b/drivers/phy/xilinx/phy-zynqmp.c
> index 2b65f84..69492a5 100644
> --- a/drivers/phy/xilinx/phy-zynqmp.c
> +++ b/drivers/phy/xilinx/phy-zynqmp.c
> @@ -219,6 +219,7 @@ struct xpsgtr_dev {
> struct mutex gtr_mutex; /* mutex for locking */
> struct xpsgtr_phy phys[NUM_LANES];
> const struct xpsgtr_ssc *refclk_sscs[NUM_LANES];
> + struct clk *clk[NUM_LANES];
> bool tx_term_fix;
> unsigned int saved_icm_cfg0;
> unsigned int saved_icm_cfg1;
> @@ -818,11 +819,15 @@ static struct phy *xpsgtr_xlate(struct device *dev,
> static int __maybe_unused xpsgtr_suspend(struct device *dev)
> {
> struct xpsgtr_dev *gtr_dev = dev_get_drvdata(dev);
> + unsigned int i;
>
> /* Save the snapshot ICM_CFG registers. */
> gtr_dev->saved_icm_cfg0 = xpsgtr_read(gtr_dev, ICM_CFG0);
> gtr_dev->saved_icm_cfg1 = xpsgtr_read(gtr_dev, ICM_CFG1);
>
> + for (i = 0; i < ARRAY_SIZE(gtr_dev->clk); i++)
> + clk_disable_unprepare(gtr_dev->clk[i]);
> +
> return 0;
> }
>
> @@ -832,6 +837,13 @@ static int __maybe_unused xpsgtr_resume(struct device
> *dev)
> unsigned int icm_cfg0, icm_cfg1;
> unsigned int i;
> bool skip_phy_init;
> + int err;
> +
> + for (i = 0; i < ARRAY_SIZE(gtr_dev->clk); i++) {
> + err = clk_prepare_enable(gtr_dev->clk[i]);
> + if (err)
> + goto err_clk_put;
> + }
>
> icm_cfg0 = xpsgtr_read(gtr_dev, ICM_CFG0);
> icm_cfg1 = xpsgtr_read(gtr_dev, ICM_CFG1);
> @@ -852,6 +864,12 @@ static int __maybe_unused xpsgtr_resume(struct device
> *dev)
> gtr_dev->phys[i].skip_phy_init = skip_phy_init;
>
> return 0;
> +
> +err_clk_put:
> + while (i--)
> + clk_disable_unprepare(gtr_dev->clk[i]);
> +
> + return err;
> }
>
> static const struct dev_pm_ops xpsgtr_pm_ops = {
> @@ -865,6 +883,7 @@ static const struct dev_pm_ops xpsgtr_pm_ops = {
> static int xpsgtr_get_ref_clocks(struct xpsgtr_dev *gtr_dev)
> {
> unsigned int refclk;
> + int ret;
>
> for (refclk = 0; refclk < ARRAY_SIZE(gtr_dev->refclk_sscs); ++refclk) {
> unsigned long rate;
> @@ -874,14 +893,22 @@ static int xpsgtr_get_ref_clocks(struct xpsgtr_dev
> *gtr_dev)
>
> snprintf(name, sizeof(name), "ref%u", refclk);
> clk = devm_clk_get_optional(gtr_dev->dev, name);
> - if (IS_ERR(clk))
> - return dev_err_probe(gtr_dev->dev, PTR_ERR(clk),
> - "Failed to get reference clock
> %u\n",
> - refclk);
> + if (IS_ERR(clk)) {
> + ret = dev_err_probe(gtr_dev->dev, PTR_ERR(clk),
> + "Failed to get reference clock
> %u\n",
> + refclk);
> + goto err_clk_put;
> + }
>
> if (!clk)
> continue;
>
> + ret = clk_prepare_enable(clk);
> + if (ret)
> + goto err_clk_put;
> +
> + gtr_dev->clk[refclk] = clk;
> +
> /*
>* Get the spread spectrum (SSC) settings for the reference
>* clock rate.
> @@ -899,11 +926,18 @@ static int xpsgtr_get_ref_clocks(struct xpsgtr_dev
> *gtr_dev)
> dev_err(gtr_dev->dev,
> "Invalid rate %lu for reference clock %u\n",
> rate, refclk);
> - return -EINVAL;
> + ret = -EINVAL;
> + goto err_clk_put;
>
Re: [PATCH v3 4/4] drm/ingenic: Fix non-OSD mode
Hi Paul,
Thank you for the patch.
On Sun, Jan 24, 2021 at 08:55:52AM +, Paul Cercueil wrote:
> Even though the JZ4740 did not have the OSD mode, it had (according to
> the documentation) two DMA channels, but there is absolutely no
> information about how to select the second DMA channel.
>
> Make the ingenic-drm driver work in non-OSD mode by using the
> foreground0 plane (which is bound to the DMA0 channel) as the primary
> plane, instead of the foreground1 plane, which is the primary plane
> when in OSD mode.
>
> Fixes: 3c9bea4ef32b ("drm/ingenic: Add support for OSD mode")
> Cc: # v5.8+
> Signed-off-by: Paul Cercueil
> Acked-by: Daniel Vetter
I don't have much knowledge about this platform, but the change looks
reasonable to me.
Acked-by: Laurent Pinchart
> ---
> drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 11 +++
> 1 file changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
> b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
> index b23011c1c5d9..59ce43862e16 100644
> --- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
> +++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
> @@ -554,7 +554,7 @@ static void ingenic_drm_plane_atomic_update(struct
> drm_plane *plane,
> height = state->src_h >> 16;
> cpp = state->fb->format->cpp[0];
>
> - if (priv->soc_info->has_osd && plane->type ==
> DRM_PLANE_TYPE_OVERLAY)
> + if (!priv->soc_info->has_osd || plane->type ==
> DRM_PLANE_TYPE_OVERLAY)
> hwdesc = >dma_hwdescs->hwdesc_f0;
> else
> hwdesc = >dma_hwdescs->hwdesc_f1;
> @@ -826,6 +826,7 @@ static int ingenic_drm_bind(struct device *dev, bool
> has_components)
> const struct jz_soc_info *soc_info;
> struct ingenic_drm *priv;
> struct clk *parent_clk;
> + struct drm_plane *primary;
> struct drm_bridge *bridge;
> struct drm_panel *panel;
> struct drm_encoder *encoder;
> @@ -940,9 +941,11 @@ static int ingenic_drm_bind(struct device *dev, bool
> has_components)
> if (soc_info->has_osd)
> priv->ipu_plane = drm_plane_from_index(drm, 0);
>
> - drm_plane_helper_add(>f1, _drm_plane_helper_funcs);
> + primary = priv->soc_info->has_osd ? >f1 : >f0;
>
> - ret = drm_universal_plane_init(drm, >f1, 1,
> + drm_plane_helper_add(primary, _drm_plane_helper_funcs);
> +
> + ret = drm_universal_plane_init(drm, primary, 1,
> _drm_primary_plane_funcs,
> priv->soc_info->formats_f1,
> priv->soc_info->num_formats_f1,
> @@ -954,7 +957,7 @@ static int ingenic_drm_bind(struct device *dev, bool
> has_components)
>
> drm_crtc_helper_add(>crtc, _drm_crtc_helper_funcs);
>
> - ret = drm_crtc_init_with_planes(drm, >crtc, >f1,
> + ret = drm_crtc_init_with_planes(drm, >crtc, primary,
> NULL, _drm_crtc_funcs, NULL);
> if (ret) {
> dev_err(dev, "Failed to init CRTC: %i\n", ret);
--
Regards,
Laurent Pinchart
Re: [PATCH v3 0/4] Fixes to bridge/panel and ingenic-drm
On Tue, Mar 23, 2021 at 04:03:00PM +, Paul Cercueil wrote: > Le mer. 24 févr. 2021 à 13:44, Paul Cercueil a écrit : > > Hi, > > > > Some feedback for patches 1-3? Laurent? > > 1-month anniversary ping :) Haaappy birth-day t youuu :-) Patches reviewed. > > Le dim. 24 janv. 2021 à 8:55, Paul Cercueil a écrit : > >> Hi, > >> > >> Here are three independent fixes. The first one addresses a > >> use-after-free in bridge/panel.c; the second one addresses a > >> use-after-free in the ingenic-drm driver; finally, the third one makes > >> the ingenic-drm driver work again on older Ingenic SoCs. > >> > >> Changes from v2: > >> - patch [1/4] added a FIXME. > >> - patch [2/4] is new. It introduces a drmm_plain_simple_encoder_alloc() > >> macro that will be used in patch [3/4]. > >> - patch [3/4] uses the macro introduced in patch [2/4]. > >> - patch [4/4] is unmodified. > >> > >> Note to linux-stable guys: patch [v2 2/3] will only apply on the current > >> drm-misc-next branch, to fix it for v5.11 and older kernels, use the V1 > >> of that patch. > >> > >> Cheers, > >> -Paul > >> > >> Paul Cercueil (4): > >> drm: bridge/panel: Cleanup connector on bridge detach > >> drm/simple_kms_helper: Add macro drmm_plain_simple_encoder_alloc() > >> drm/ingenic: Register devm action to cleanup encoders > >> drm/ingenic: Fix non-OSD mode > >> > >> drivers/gpu/drm/bridge/panel.c| 12 +++ > >> drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 26 +++ > >> include/drm/drm_simple_kms_helper.h | 17 +++ > >> 3 files changed, 42 insertions(+), 13 deletions(-) > >> -- Regards, Laurent Pinchart
Re: [PATCH v3 3/4] drm/ingenic: Register devm action to cleanup encoders
Hi Paul,
Thank you for the patch.
On Sun, Jan 24, 2021 at 08:55:51AM +, Paul Cercueil wrote:
> Since the encoders have been devm-allocated, they will be freed way
> before drm_mode_config_cleanup() is called. To avoid use-after-free
> conditions, we then must ensure that drm_encoder_cleanup() is called
> before the encoders are freed.
>
> v2: Use the new __drmm_simple_encoder_alloc() function
>
> v3: Use the new drmm_plain_simple_encoder_alloc() macro
>
> Fixes: c369cb27c267 ("drm/ingenic: Support multiple panels/bridges")
> Cc: # 5.8+
> Signed-off-by: Paul Cercueil
Reviewed-by: Laurent Pinchart
> ---
>
> Notes:
> Use the V1 of this patch to fix v5.11 and older kernels. This V3 only
> applies on the current drm-misc-next branch.
>
> drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 15 ++-
> 1 file changed, 6 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
> b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
> index 7bb31fbee29d..b23011c1c5d9 100644
> --- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
> +++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
> @@ -1014,20 +1014,17 @@ static int ingenic_drm_bind(struct device *dev, bool
> has_components)
> bridge = devm_drm_panel_bridge_add_typed(dev, panel,
>
> DRM_MODE_CONNECTOR_DPI);
>
> - encoder = devm_kzalloc(dev, sizeof(*encoder), GFP_KERNEL);
> - if (!encoder)
> - return -ENOMEM;
> + encoder = drmm_plain_simple_encoder_alloc(drm,
> DRM_MODE_ENCODER_DPI);
> + if (IS_ERR(encoder)) {
> + ret = PTR_ERR(encoder);
> + dev_err(dev, "Failed to init encoder: %d\n", ret);
> + return ret;
> + }
>
> encoder->possible_crtcs = 1;
>
> drm_encoder_helper_add(encoder,
> _drm_encoder_helper_funcs);
>
> - ret = drm_simple_encoder_init(drm, encoder,
> DRM_MODE_ENCODER_DPI);
> - if (ret) {
> - dev_err(dev, "Failed to init encoder: %d\n", ret);
> - return ret;
> - }
> -
> ret = drm_bridge_attach(encoder, bridge, NULL, 0);
> if (ret) {
> dev_err(dev, "Unable to attach bridge\n");
--
Regards,
Laurent Pinchart
Re: [PATCH v3 2/4] drm/simple_kms_helper: Add macro drmm_plain_simple_encoder_alloc()
Hi Paul, Thank you for the patch. On Sun, Jan 24, 2021 at 08:55:50AM +, Paul Cercueil wrote: > This performs the same operation as drmm_simple_encoder_alloc(), but > only allocates and returns a struct drm_encoder instance. > > Signed-off-by: Paul Cercueil > --- > include/drm/drm_simple_kms_helper.h | 17 + > 1 file changed, 17 insertions(+) > > diff --git a/include/drm/drm_simple_kms_helper.h > b/include/drm/drm_simple_kms_helper.h > index e6dbf3161c2f..f07e70303cfb 100644 > --- a/include/drm/drm_simple_kms_helper.h > +++ b/include/drm/drm_simple_kms_helper.h > @@ -209,4 +209,21 @@ void *__drmm_simple_encoder_alloc(struct drm_device > *dev, size_t size, >offsetof(type, member), \ >encoder_type)) > > +/** > + * drmm_plain_simple_encoder_alloc - Allocate and initialize a drm_encoder > + * struct with basic functionality. > + * @dev: drm device > + * @encoder_type: user visible type of the encoder > + * > + * This performs the same operation as drmm_simple_encoder_alloc(), but > + * only allocates and returns a struct drm_encoder instance. > + * > + * Returns: > + * Pointer to the new drm_encoder struct, or ERR_PTR on failure. > + */ > +#define drmm_plain_simple_encoder_alloc(dev, encoder_type) \ > + ((struct drm_encoder *) \ > + __drmm_simple_encoder_alloc(dev, sizeof(struct drm_encoder), \ > + 0, encoder_type)) > + As this isn't related to the simple encoder helper anymore, how about using __drmm_encoder_alloc instead ? #define drmm_plain_simple_encoder_alloc(dev, encoder_type) \ ((struct drm_encoder *) \ __drmm_encoder_alloc(dev, sizeof(struct drm_encoder), 0, NULL, \ encoder_type, NULL)) I'd also rename the macro to drmm_plain_encoder_alloc(), and move it to drm_encoder.h. That way drivers that don't need the simple KMS helper won't have to select it just for this. > #endif /* __LINUX_DRM_SIMPLE_KMS_HELPER_H */ -- Regards, Laurent Pinchart
Re: [PATCH v3 1/4] drm: bridge/panel: Cleanup connector on bridge detach
Hi Paul,
Thank you for the patch.
On Sun, Jan 24, 2021 at 08:55:49AM +, Paul Cercueil wrote:
> If we don't call drm_connector_cleanup() manually in
> panel_bridge_detach(), the connector will be cleaned up with the other
> DRM objects in the call to drm_mode_config_cleanup(). However, since our
> drm_connector is devm-allocated, by the time drm_mode_config_cleanup()
> will be called, our connector will be long gone. Therefore, the
> connector must be cleaned up when the bridge is detached to avoid
> use-after-free conditions.
>
> v2: Cleanup connector only if it was created
>
> v3: Add FIXME
>
> Fixes: 13dfc0540a57 ("drm/bridge: Refactor out the panel wrapper from the
> lvds-encoder bridge.")
> Cc: # 4.12+
> Cc: Andrzej Hajda
> Cc: Neil Armstrong
> Cc: Laurent Pinchart
> Cc: Jonas Karlman
> Cc: Jernej Skrabec
> Signed-off-by: Paul Cercueil
> ---
> drivers/gpu/drm/bridge/panel.c | 12
> 1 file changed, 12 insertions(+)
>
> diff --git a/drivers/gpu/drm/bridge/panel.c b/drivers/gpu/drm/bridge/panel.c
> index 0ddc37551194..5959e8183cd0 100644
> --- a/drivers/gpu/drm/bridge/panel.c
> +++ b/drivers/gpu/drm/bridge/panel.c
> @@ -87,6 +87,18 @@ static int panel_bridge_attach(struct drm_bridge *bridge,
>
> static void panel_bridge_detach(struct drm_bridge *bridge)
> {
> + struct panel_bridge *panel_bridge = drm_bridge_to_panel_bridge(bridge);
> + struct drm_connector *connector = _bridge->connector;
> +
> + /*
> + * Cleanup the connector if we know it was initialized.
> + *
> + * FIXME: This wouldn't be needed if the panel_bridge structure was
> + * allocated with drmm_kzalloc(). This might be tricky since the
> + * drm_device pointer can only be retrieved when the bridge is attached.
> + */
> + if (!!panel_bridge->connector.dev)
How about simply
if (connector->dev)
? With this change,
Reviewed-by: Laurent Pinchart
> + drm_connector_cleanup(connector);
> }
>
> static void panel_bridge_pre_enable(struct drm_bridge *bridge)
--
Regards,
Laurent Pinchart
Re: [PATCH v2 1/3] drm: bridge/panel: Cleanup connector on bridge detach
On Wed, Jan 20, 2021 at 06:38:03PM +0100, Daniel Vetter wrote:
> On Wed, Jan 20, 2021 at 6:12 PM Paul Cercueil wrote:
> > Le mer. 20 janv. 2021 à 17:03, Daniel Vetter a écrit :
> > > On Wed, Jan 20, 2021 at 1:35 PM Paul Cercueil wrote:
> > >>
> > >> If we don't call drm_connector_cleanup() manually in
> > >> panel_bridge_detach(), the connector will be cleaned up with the other
> > >> DRM objects in the call to drm_mode_config_cleanup(). However, since our
> > >> drm_connector is devm-allocated, by the time drm_mode_config_cleanup()
> > >> will be called, our connector will be long gone. Therefore, the
> > >> connector must be cleaned up when the bridge is detached to avoid
> > >> use-after-free conditions.
> > >
> > > For -fixes this sounds ok, but for -next I think switching to drmm_
> > > would be much better.
> >
> > The API would need to change to have access to the drm_device struct,
> > though. That would be quite a big patch, there are a few dozens source
> > files that use this API already.
>
> Hm right pure drmm_ doesn't work for panel or bridge since it's
> usually a separate driver. But devm_ also doesn't work. I think what
> we need here is two-stage: first kmalloc the panel (or bridge, it's
> really the same) in the panel/bridge driver load. Then when we bind it
> to the drm_device we can tie it into the managed resources with
> drmm_add_action_or_reset. Passing the drm_device to the point where we
> allocate the panel/bridge doesn't work for these.
>
> I think minimally we need a FIXME here and ack from Laurent on how
> this should be solved at least, since panel bridge is used rather
> widely.
Bridge removal is completely broken. If you unbind a bridge driver from
the device, the bridge will be unregistered and resources freed, without
the display driver knowing about this. The lifetime of the drm_bridge
structure itself isn't the only issue to be addressed here, it's broader
than that, and needs to consider that the display driver could be
calling the bridge operations concurrently to the removal.
We need a volunteer with enough motivation to solve this subsystem-wide
:-) In the meantime, whatever shortcut addresses immediate issues is
probably fine, as yak-shaving in this area would definitely not be
reasonable.
> > >> v2: Cleanup connector only if it was created
> > >>
> > >> Fixes: 13dfc0540a57 ("drm/bridge: Refactor out the panel wrapper from
> > >> the lvds-encoder bridge.")
> > >> Cc: # 4.12+
> > >> Cc: Andrzej Hajda
> > >> Cc: Neil Armstrong
> > >> Cc: Laurent Pinchart
> > >> Cc: Jonas Karlman
> > >> Cc: Jernej Skrabec
> > >> Signed-off-by: Paul Cercueil
> > >> ---
> > >> drivers/gpu/drm/bridge/panel.c | 6 ++
> > >> 1 file changed, 6 insertions(+)
> > >>
> > >> diff --git a/drivers/gpu/drm/bridge/panel.c
> > >> b/drivers/gpu/drm/bridge/panel.c
> > >> index 0ddc37551194..df86b0ee0549 100644
> > >> --- a/drivers/gpu/drm/bridge/panel.c
> > >> +++ b/drivers/gpu/drm/bridge/panel.c
> > >> @@ -87,6 +87,12 @@ static int panel_bridge_attach(struct drm_bridge
> > >> *bridge,
> > >>
> > >> static void panel_bridge_detach(struct drm_bridge *bridge)
> > >> {
> > >> +struct panel_bridge *panel_bridge =
> > >> drm_bridge_to_panel_bridge(bridge);
> > >> +struct drm_connector *connector = _bridge->connector;
> > >> +
> > >> +/* Cleanup the connector if we know it was initialized */
> > >> +if (!!panel_bridge->connector.dev)
> > >> +drm_connector_cleanup(connector);
> > >> }
> > >>
> > >> static void panel_bridge_pre_enable(struct drm_bridge *bridge)
--
Regards,
Laurent Pinchart
Re: [PATCH v4 3/4] drm: sun4i: dsi: Convert to bridge driver
ctor_init(drm, >connector,
> - _dsi_connector_funcs,
> - DRM_MODE_CONNECTOR_DSI);
> + ret = drm_bridge_attach(>encoder, >bridge, NULL, 0);
> if (ret) {
> - dev_err(dsi->dev,
> - "Couldn't initialise the DSI connector\n");
> + dev_err(dsi->dev, "Couldn't attach drm bridge\n");
> goto err_cleanup_connector;
> }
>
> - drm_connector_attach_encoder(>connector, >encoder);
> -
> - if (dsi->panel_bridge) {
> - ret = drm_bridge_attach(>encoder, dsi->panel_bridge, NULL,
> 0);
> - if (ret) {
> - dev_err(dsi->dev, "Couldn't attach drm bridge\n");
> - goto err_cleanup_connector;
> - }
> - }
> -
> return 0;
>
> err_cleanup_connector:
> @@ -1199,6 +1213,12 @@ static int sun6i_dsi_probe(struct platform_device
> *pdev)
> goto err_unprotect_clk;
> }
>
> + dsi->bridge.funcs = _dsi_bridge_funcs;
> + dsi->bridge.of_node = dev->of_node;
> + dsi->bridge.type = DRM_MODE_CONNECTOR_DSI;
> +
> + drm_bridge_add(>bridge);
> +
> ret = component_add(>dev, _dsi_ops);
> if (ret) {
> dev_err(dev, "Couldn't register our component\n");
> @@ -1222,6 +1242,7 @@ static int sun6i_dsi_remove(struct platform_device
> *pdev)
> struct device *dev = >dev;
> struct sun6i_dsi *dsi = dev_get_drvdata(dev);
>
> + drm_bridge_remove(>bridge);
> component_del(>dev, _dsi_ops);
> mipi_dsi_host_unregister(>host);
> clk_rate_exclusive_put(dsi->mod_clk);
> diff --git a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h
> b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h
> index 370ecb356a63..5e70666089ad 100644
> --- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h
> +++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h
> @@ -16,6 +16,7 @@
> #define SUN6I_DSI_TCON_DIV 4
>
> struct sun6i_dsi {
> + struct drm_bridge bridge;
> struct drm_connectorconnector;
> struct drm_encoder encoder;
The drm_encoder should be dropped from this driver, the encoder should
be created by the main display driver.
> struct mipi_dsi_hosthost;
> @@ -38,6 +39,11 @@ static inline struct sun6i_dsi *host_to_sun6i_dsi(struct
> mipi_dsi_host *host)
> return container_of(host, struct sun6i_dsi, host);
> };
>
> +static inline struct sun6i_dsi *bridge_to_sun6i_dsi(struct drm_bridge
> *bridge)
> +{
> + return container_of(bridge, struct sun6i_dsi, bridge);
> +}
> +
> static inline struct sun6i_dsi *connector_to_sun6i_dsi(struct drm_connector
> *connector)
> {
> return container_of(connector, struct sun6i_dsi, connector);
--
Regards,
Laurent Pinchart
Re: [PATCH v4 2/4] drm: sun4i: dsi: Add bridge support
;connector, >encoder);
>
> - dsi->drm = drm;
> + if (dsi->panel_bridge) {
> + ret = drm_bridge_attach(>encoder, dsi->panel_bridge, NULL,
> 0);
> + if (ret) {
> + dev_err(dsi->dev, "Couldn't attach drm bridge\n");
> + goto err_cleanup_connector;
> + }
> + }
>
> return 0;
>
> @@ -1096,7 +1106,7 @@ static void sun6i_dsi_unbind(struct device *dev, struct
> device *master,
> {
> struct sun6i_dsi *dsi = dev_get_drvdata(dev);
>
> - dsi->drm = NULL;
> + drm_encoder_cleanup(>encoder);
> }
>
> static const struct component_ops sun6i_dsi_ops = {
> diff --git a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h
> b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h
> index c863900ae3b4..370ecb356a63 100644
> --- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h
> +++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h
> @@ -29,8 +29,8 @@ struct sun6i_dsi {
>
> struct device *dev;
> struct mipi_dsi_device *device;
> - struct drm_device *drm;
> struct drm_panel*panel;
> + struct drm_bridge *panel_bridge;
> };
>
> static inline struct sun6i_dsi *host_to_sun6i_dsi(struct mipi_dsi_host *host)
--
Regards,
Laurent Pinchart
Re: [PATCH v4 1/4] drm: sun4i: dsi: Use drm_of_find_panel_or_bridge
Hi Jagan,
Thank you for the patch.
On Mon, Mar 22, 2021 at 07:31:49PM +0530, Jagan Teki wrote:
> Replace of_drm_find_panel with drm_of_find_panel_or_bridge
> for finding panel, this indeed help to find the bridge if
> bridge support added.
>
> Added NULL in bridge argument, same will replace with bridge
> parameter once bridge supported.
>
> Signed-off-by: Jagan Teki
Looks good, there should be no functional change.
Reviewed-by: Laurent Pinchart
> ---
> Changes for v4, v3:
> - none
>
> drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c | 11 ---
> 1 file changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> index 4f5efcace68e..2e9e7b2d4145 100644
> --- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> +++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> @@ -21,6 +21,7 @@
>
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -963,10 +964,14 @@ static int sun6i_dsi_attach(struct mipi_dsi_host *host,
> struct mipi_dsi_device *device)
> {
> struct sun6i_dsi *dsi = host_to_sun6i_dsi(host);
> - struct drm_panel *panel = of_drm_find_panel(device->dev.of_node);
> + struct drm_panel *panel;
> + int ret;
> +
> + ret = drm_of_find_panel_or_bridge(dsi->dev->of_node, 0, 0,
> + , NULL);
> + if (ret)
> + return ret;
>
> - if (IS_ERR(panel))
> - return PTR_ERR(panel);
> if (!dsi->drm || !dsi->drm->registered)
> return -EPROBE_DEFER;
>
--
Regards,
Laurent Pinchart
Re: [PATCH v3 3/4] drm/bridge: ti-sn65dsi86: Read EDID blob over DDC
Hi Doug,
On Tue, Mar 23, 2021 at 12:07:27PM -0700, Doug Anderson wrote:
> On Mon, Mar 22, 2021 at 8:17 PM Stephen Boyd wrote:
> >
> > Quoting Laurent Pinchart (2021-03-17 17:20:43)
> > > Hi Stephen,
> > >
> > > Reviving a bit of an old thread, for a question.
> > >
> > > On Mon, Nov 02, 2020 at 10:11:43AM -0800, Stephen Boyd wrote:
> > > > @@ -265,6 +267,23 @@ connector_to_ti_sn_bridge(struct drm_connector
> > > > *connector)
> > > > static int ti_sn_bridge_connector_get_modes(struct drm_connector
> > > > *connector)
> > > > {
> > > > struct ti_sn_bridge *pdata = connector_to_ti_sn_bridge(connector);
> > > > + struct edid *edid = pdata->edid;
> > > > + int num, ret;
> > > > +
> > > > + if (!edid) {
> > > > + pm_runtime_get_sync(pdata->dev);
> > > > + edid = pdata->edid = drm_get_edid(connector,
> > > > >aux.ddc);
> > > > + pm_runtime_put(pdata->dev);
> > >
> > > Is there any specific reason to use the indirect access method, compared
> > > to the direct method that translates access to an I2C ancillary address
> > > to an I2C-over-AUX transaction (see page 20 of SLLSEH2B) ? The direct
> > > method seems it would be more efficient.
> >
> > No I don't think it matters. I was just using the existing support code
> > that Sean wrote instead of digging into the details. Maybe Sean ran into
> > something earlier and abandoned that approach?
>
> From reading the docs, it sounds as if there _could_ be a reason to
> use the indirect method. Specifically if the i2c host that the bridge
> is on doesn't support clock stretching then the direct method wouldn't
> work according to the docs. Is that something that we'd have to
> reasonably worry about?
I'm not sure. I'm going through BSP code that uses the direct method,
and I was wondering if it was just an implementation detail. Once I get
the display working on this board, I'll try to find time to compare the
two methods, to see if there's a significatant performance improvement
from the direct method. If there isn't, I won't bother.
--
Regards,
Laurent Pinchart
Re: [PATCH v2 0/4] drm/bridge: ti-sn65dsi86: Support EDID reading
Hi Stephen, On Mon, Nov 02, 2020 at 05:15:24PM -0800, Stephen Boyd wrote: > Quoting Sam Ravnborg (2020-11-01 09:37:41) > > Hi Stephen. > > > > On Thu, Oct 29, 2020 at 06:17:34PM -0700, Stephen Boyd wrote: > > > This patch series cleans up the DDC code a little bit so that > > > it is more efficient time wise and supports grabbing the EDID > > > of the eDP panel over the aux channel. I timed this on a board > > > I have on my desk and it takes about 20ms to grab the EDID out > > > of the panel and make sure it is valid. > > > > > > The first two patches seem less controversial so I stuck them at > > > the beginning. The third patch does the EDID reading and caches > > > it so we don't have to keep grabbing it over and over again. And > > > finally the last patch updates the reply field so that short > > > reads and nacks over the channel are reflected properly instead of > > > treating them as some sort of error that can't be discerned. > > > > > > Stephen Boyd (4): > > > drm/bridge: ti-sn65dsi86: Combine register accesses in > > > ti_sn_aux_transfer() > > > drm/bridge: ti-sn65dsi86: Make polling a busy loop > > > drm/bridge: ti-sn65dsi86: Read EDID blob over DDC > > > drm/bridge: ti-sn65dsi86: Update reply on aux failures > > > > Series looks good. You can add my a-b on the full series. > > Acked-by: Sam Ravnborg > > > > I can apply after Douglas have had a look at the patches he did not r-b > > yet. > > > > Any chance we can convince you to prepare this bridge driver for use in > > a chained bridge setup where the connector is created by the display > > driver and uses drm_bridge_funcs? > > > > First step wuld be to introduce the use of a panel_bridge. > > Then add get_edid to drm_bridge_funcs and maybe more helpers. > > > > Then natural final step would be to move connector creation to the > > display driver - see how other uses drm_bridge_connector_init() to do so > > - it is relatively simple. > > > > Should be doable - and reach out if you need some help. > > I started to look at this and got stuck at ti_sn_bridge_get_bpp(). Where > can I get the details of the bpc for the downstream bridge or panel? Is > there some function that can tell this bridge what the bpc is for the > attached connector? I've posted a patch series to convert to DRM_BRIDGE_ATTACH_NO_CONNECTOR yesterday (and have CC'ed you), but I've overlooked this particular problem :-S You can't get the connector in the .enable() operation, but you can get it in .atomic_enable(), with drm_atomic_get_new_connector_for_encoder(). This being said, it's probably not the right option. What matters here isn't the bpc for the connector, but the format expected by the next bridge in the chain. drm_bridge_funcs has two operations, .atomic_get_output_bus_fmts() and .atomic_get_input_bus_fmts(), to negotiate that format along a chain of bridges. The panel bridge driver (drivers/gpu/drm/bridge/panel.c) doesn't implement those operations, and neither does display-connector.c, so that may be what we should start with. > I see that td_mode_valid() in > drivers/gpu/drm/bridge/tc358775.c stores away the bpc from the incoming > drm_display_info pointer but I'm not sure that is correct because can't > that be called for various and not necessarily the one we're using? You're right, .mode_valid() shouldn't do that. -- Regards, Laurent Pinchart
Re: [PATCH v6 1/5] dt-bindings:drm/bridge:anx7625:add vendor define flags
Hi Xin,
Thank you for the patch.
On Fri, Mar 19, 2021 at 02:32:39PM +0800, Xin Ji wrote:
> Add 'bus-type' and 'data-lanes' define for port0. Define DP tx lane0,
> lane1 swing register array define, and audio enable flag.
>
> Signed-off-by: Xin Ji
> ---
> .../display/bridge/analogix,anx7625.yaml | 58 ++-
> 1 file changed, 57 insertions(+), 1 deletion(-)
>
> diff --git
> a/Documentation/devicetree/bindings/display/bridge/analogix,anx7625.yaml
> b/Documentation/devicetree/bindings/display/bridge/analogix,anx7625.yaml
> index c789784efe30..3f54d5876982 100644
> --- a/Documentation/devicetree/bindings/display/bridge/analogix,anx7625.yaml
> +++ b/Documentation/devicetree/bindings/display/bridge/analogix,anx7625.yaml
> @@ -34,6 +34,26 @@ properties:
> description: used for reset chip control, RESET_N pin B7.
> maxItems: 1
>
> + analogix,lane0-swing:
> +$ref: /schemas/types.yaml#/definitions/uint32-array
> +minItems: 1
> +maxItems: 20
> +description:
> + an array of swing register setting for DP tx lane0 PHY, please don't
> + add this property, or contact vendor.
DT properties need to be documented. Contacting the vendor doesn't count
as documentation I'm afraid.
> +
> + analogix,lane1-swing:
> +$ref: /schemas/types.yaml#/definitions/uint32-array
> +minItems: 1
> +maxItems: 20
> +description:
> + an array of swing register setting for DP tx lane1 PHY, please don't
> + add this property, or contact vendor.
> +
> + analogix,audio-enable:
> +type: boolean
> +description: let the driver enable audio HDMI codec function or not.
> +
>ports:
> $ref: /schemas/graph.yaml#/properties/ports
>
> @@ -41,13 +61,43 @@ properties:
>port@0:
> $ref: /schemas/graph.yaml#/properties/port
> description:
> - Video port for MIPI DSI input.
> + MIPI DSI/DPI input.
> +
> +properties:
> + endpoint:
> +$ref: /schemas/media/video-interfaces.yaml#
> +type: object
> +additionalProperties: false
> +
> +properties:
> + remote-endpoint: true
> + bus-type: true
> + data-lanes: true
> +
> +required:
> + - remote-endpoint
> +
> +required:
> + - endpoint
> +
>
>port@1:
> $ref: /schemas/graph.yaml#/properties/port
> description:
>Video port for panel or connector.
>
> +properties:
> + endpoint:
> +$ref: /schemas/media/video-interfaces.yaml#
> +type: object
> +additionalProperties: false
> +
> +properties:
> + remote-endpoint: true
> +
> +required:
> + - remote-endpoint
> +
> required:
>- port@0
>- port@1
> @@ -73,6 +123,10 @@ examples:
> enable-gpios = < 45 GPIO_ACTIVE_HIGH>;
> reset-gpios = < 73 GPIO_ACTIVE_HIGH>;
>
> +analogix,audio-enable;
> +analogix,lane0-swing = <0x14 0x54 0x64 0x74 0x29 0x7b 0x77 0x5b>;
> +analogix,lane1-swing = <0x14 0x54 0x64 0x74 0x29 0x7b 0x77 0x5b>;
> +
> ports {
> #address-cells = <1>;
> #size-cells = <0>;
> @@ -81,6 +135,8 @@ examples:
> reg = <0>;
> anx7625_in: endpoint {
> remote-endpoint = <_dsi>;
> +bus-type = <5>;
> +data-lanes = <0 1 2 3>;
> };
> };
>
--
Regards,
Laurent Pinchart
Re: [PATCH 1/2] clk: zynqmp: pll: add set_pll_mode to check condition in zynqmp_pll_enable
Hi Quanyang,
Thank you for the patch.
On Fri, Mar 19, 2021 at 06:07:17PM +0800, quanyang.w...@windriver.com wrote:
> From: Quanyang Wang
>
> If there is a IOCTL_SET_PLL_FRAC_MODE request sent to ATF ever,
> we shouldn't skip invoking PM_CLOCK_ENABLE fn even though this
> pll has been enabled. In ATF implementation, it will only assign
> the mode to the variable (struct pm_pll *)pll->mode when handling
> IOCTL_SET_PLL_FRAC_MODE call. Invoking PM_CLOCK_ENABLE can force
> ATF send request to PWU to set the pll mode to PLL's register.
>
> There is a scenario that happens in enabling VPLL_INT(clk_id:96):
> 1) VPLL_INT has been enabled during booting.
> 2) A driver calls clk_set_rate and according to the rate, the VPLL_INT
>should be set to FRAC mode. Then zynqmp_pll_set_mode is called
>to pass IOCTL_SET_PLL_FRAC_MODE to ATF. Note that at this point
>ATF just stores the mode to a variable.
> 3) This driver calls clk_prepare_enable and zynqmp_pll_enable is
>called to try to enable VPLL_INT pll. Because of 1), the function
>zynqmp_pll_enable just returns without doing anything after checking
>that this pll has been enabled.
>
> In the scenario above, the pll mode of VPLL_INT will never be set
> successfully. So adding set_pll_mode to chec condition to fix it.
s/chec/check/
> Signed-off-by: Quanyang Wang
> ---
> drivers/clk/zynqmp/pll.c | 11 ++-
> 1 file changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/clk/zynqmp/pll.c b/drivers/clk/zynqmp/pll.c
> index 92f449ed38e5..f1e8f37d7f52 100644
> --- a/drivers/clk/zynqmp/pll.c
> +++ b/drivers/clk/zynqmp/pll.c
> @@ -14,10 +14,12 @@
> * struct zynqmp_pll - PLL clock
> * @hw: Handle between common and hardware-specific interfaces
> * @clk_id: PLL clock ID
> + * @set_pll_mode:Whether an IOCTL_SET_PLL_FRAC_MODE request be sent to
> ATF
> */
> struct zynqmp_pll {
> struct clk_hw hw;
> u32 clk_id;
> + bool set_pll_mode;
> };
>
> #define to_zynqmp_pll(_hw) container_of(_hw, struct zynqmp_pll, hw)
> @@ -81,6 +83,8 @@ static inline void zynqmp_pll_set_mode(struct clk_hw *hw,
> bool on)
> if (ret)
> pr_warn_once("%s() PLL set frac mode failed for %s, ret = %d\n",
>__func__, clk_name, ret);
> + else
> + clk->set_pll_mode = true;
> }
>
> /**
> @@ -240,9 +244,14 @@ static int zynqmp_pll_enable(struct clk_hw *hw)
> u32 clk_id = clk->clk_id;
> int ret;
>
> - if (zynqmp_pll_is_enabled(hw))
> + /* Don't skip enabling clock if there is an IOCTL_SET_PLL_FRAC_MODE
> request
> + * that has been sent to ATF.
> + */
Very small issue, multiline kerneldoc comments are supposed to start
with a '/*' on its own line:
/*
* Don't skip enabling clock if there is an IOCTL_SET_PLL_FRAC_MODE
* request that has been sent to ATF.
*/
> + if (zynqmp_pll_is_enabled(hw) && (!clk->set_pll_mode))
> return 0;
>
> + clk->set_pll_mode = false;
> +
> ret = zynqmp_pm_clock_enable(clk_id);
> if (ret)
> pr_warn_once("%s() clock enable failed for %s, ret = %d\n",
This fixes the DPSUB clock issue, so
Tested-by: Laurent Pinchart
I however wonder if this is the best solution. Shouldn't we instead fix
it on the ATF side, to program the hardware when zynqmp_pll_set_mode()
is called if the clock is already enabled ?
Just reading the code, I can immediately see another potential issue in
zynqmp_pll_set_mode(). The function is called from
zynqmp_pll_round_rate(), which seems completely wrong, as
zynqmp_pll_round_rate() is supposed to only perform rate calculation,
not program the hardware. Am I missing something, or does the PLL
implementation need to be reworked more extensively than this ?
--
Regards,
Laurent Pinchart
Re: [PATCH] drm: xlnx: zynqmp: release reset to DP controller before accessing DP registers
Hi Quanyang, Thank you for the patch. On Sat, Mar 20, 2021 at 04:37:39PM +0800, quanyang.w...@windriver.com wrote: > From: Quanyang Wang > > When insmod zynqmp-dpsub.ko after rmmod it, system will hang with the > error log as below: > > root@xilinx-zynqmp:~# insmod zynqmp-dpsub.ko > [ 88.391289] [drm] Initialized zynqmp-dpsub 1.0.0 20130509 for > fd4a.display on minor 0 > [ 88.529906] Console: switching to colour frame buffer device 128x48 > [ 88.549402] zynqmp-dpsub fd4a.display: [drm] fb0: zynqmp-dpsubdrm > frame buffer device > [ 88.571624] zynqmp-dpsub fd4a.display: ZynqMP DisplayPort Subsystem > driver probed > root@xilinx-zynqmp:~# rmmod zynqmp_dpsub > [ 94.023404] Console: switching to colour dummy device 80x25 > root@xilinx-zynqmp:~# insmod zynqmp-dpsub.ko > > > This is because that in zynqmp_dp_probe it tries to access some DP > registers while the DP controller is still in the reset state. When > running "rmmod zynqmp_dpsub", zynqmp_dp_reset(dp, true) in > zynqmp_dp_phy_exit is called to force the DP controller into the reset > state. Then insmod will call zynqmp_dp_probe to write to the DP registers, > but at this moment the DP controller isn't brought out of the reset state > since the function zynqmp_dp_reset(dp, false) is called later and this > will result the system hang. > > Releasing the reset to DP controller before any read/write operation to it > will fix this issue. > > Signed-off-by: Quanyang Wang > --- > drivers/gpu/drm/xlnx/zynqmp_dp.c | 8 > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/xlnx/zynqmp_dp.c > b/drivers/gpu/drm/xlnx/zynqmp_dp.c > index 99158ee67d02..bb45b3663d6b 100644 > --- a/drivers/gpu/drm/xlnx/zynqmp_dp.c > +++ b/drivers/gpu/drm/xlnx/zynqmp_dp.c > @@ -402,10 +402,6 @@ static int zynqmp_dp_phy_init(struct zynqmp_dp *dp) > } > } > > - ret = zynqmp_dp_reset(dp, false); > - if (ret < 0) > - return ret; > - > zynqmp_dp_clr(dp, ZYNQMP_DP_PHY_RESET, ZYNQMP_DP_PHY_RESET_ALL_RESET); > > /* > @@ -1682,6 +1678,10 @@ int zynqmp_dp_probe(struct zynqmp_dpsub *dpsub, struct > drm_device *drm) > return PTR_ERR(dp->reset); > } > > + ret = zynqmp_dp_reset(dp, false); > + if (ret < 0) > + return ret; > + This change looks good to me. > ret = zynqmp_dp_phy_probe(dp); > if (ret) > return ret; But shouldn't we call zynqmp_dp_reset(dp, true) here ? Or rather, call it in the error path at the end of the function, with a goto label. For symmetry, should we also move the zynqmp_dp_reset() call from zynqmp_dp_phy_exit() to zynqmp_dp_remove() ? -- Regards, Laurent Pinchart
Re: [PATCH v3 19/19] media: i2c: rdacm20: Re-work ov10635 reset
Hi Jacopo,
Thank you for the patch.
On Fri, Mar 19, 2021 at 05:41:48PM +0100, Jacopo Mondi wrote:
> The OV10635 image sensor embedded in the camera module is currently
> reset after the MAX9271 initialization with two long delays that were
> most probably not correctly characterized.
>
> Re-work the image sensor reset procedure by holding the chip in reset
> during the MAX9271 configuration, removing the long sleep delays and
> only wait after the chip exits from reset for 350-500 microseconds
> interval, which is larger than the minimum (2048 * (1 / XVCLK)) timeout
> characterized in the chip manual.
>
> Reviewed-by: Kieran Bingham
> Reviewed-by: Laurent Pinchart
> Signed-off-by: Jacopo Mondi
> ---
> drivers/media/i2c/rdacm20.c | 29 +
> 1 file changed, 17 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/media/i2c/rdacm20.c b/drivers/media/i2c/rdacm20.c
> index 7ed3866b5335..7ba2d0249da8 100644
> --- a/drivers/media/i2c/rdacm20.c
> +++ b/drivers/media/i2c/rdacm20.c
> @@ -454,6 +454,19 @@ static int rdacm20_init(struct v4l2_subdev *sd, unsigned
> int val)
> if (ret)
> return ret;
>
> + /*
> + * Hold OV10635 in reset during max9271 configuration. The reset signal
> + * has to be asserted for at least 200 microseconds.
> + */
> + ret = max9271_enable_gpios(>serializer, MAX9271_GPIO1OUT);
> + if (ret)
> + return ret;
> +
> + ret = max9271_clear_gpios(>serializer, MAX9271_GPIO1OUT);
enable and clear is very cnonfusing. How about mimicking the GPIO API,
with direction_input(), direction_output() and set_value() functions ?
It would also be nice if the polarity could be handled in a nicer way.
There's no GPIO request API here, but maybe a max9271_gpio_set_flags() ?
> + if (ret)
> + return ret;
> + usleep_range(200, 500);
> +
> ret = max9271_configure_gmsl_link(>serializer);
> if (ret)
> return ret;
> @@ -468,22 +481,14 @@ static int rdacm20_init(struct v4l2_subdev *sd,
> unsigned int val)
> dev->serializer.client->addr = dev->addrs[0];
>
> /*
> - * Reset the sensor by cycling the OV10635 reset signal connected to the
> - * MAX9271 GPIO1 and verify communication with the OV10635.
> + * Release ov10635 from reset and initialize it. The image sensor
> + * requires at least 2048 XVCLK cycles (85 micro-seconds at 24MHz)
> + * before being available. Stay safe and wait up to 500 micro-seconds.
>*/
> - ret = max9271_enable_gpios(>serializer, MAX9271_GPIO1OUT);
> - if (ret)
> - return ret;
> -
> - ret = max9271_clear_gpios(>serializer, MAX9271_GPIO1OUT);
> - if (ret)
> - return ret;
> - usleep_range(1, 15000);
> -
> ret = max9271_set_gpios(>serializer, MAX9271_GPIO1OUT);
> if (ret)
> return ret;
> - usleep_range(1, 15000);
> + usleep_range(100, 500);
>
> for (i = 0; i < OV10635_PID_TIMEOUT; ++i) {
> ret = ov10635_read16(dev, OV10635_PID);
--
Regards,
Laurent Pinchart
Re: [PATCH v3 16/19] media: i2c: rdacm20: Replace goto with a loop
Hi Jacopo,
Thank you for the patch.
On Fri, Mar 19, 2021 at 05:41:45PM +0100, Jacopo Mondi wrote:
> During the camera module initialization the image sensor PID is read to
> verify it can correctly be identified. The current implementation is
> rather confused and uses a loop implemented with a label and a goto.
>
> Replace it with a more compact for() loop.
>
> No functional changes intended.
>
> Reviewed-by: Kieran Bingham
> Reviewed-by: Laurent Pinchart
> Signed-off-by: Jacopo Mondi
> ---
> drivers/media/i2c/rdacm20.c | 29 +
> 1 file changed, 13 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/media/i2c/rdacm20.c b/drivers/media/i2c/rdacm20.c
> index 7bdcfafa6c10..760705dd2918 100644
> --- a/drivers/media/i2c/rdacm20.c
> +++ b/drivers/media/i2c/rdacm20.c
> @@ -59,6 +59,8 @@
> */
> #define OV10635_PIXEL_RATE (4400)
>
> +#define OV10635_PID_TIMEOUT 3
> +
> static const struct ov10635_reg {
> u16 reg;
> u8 val;
> @@ -438,7 +440,7 @@ static int rdacm20_get_fmt(struct v4l2_subdev *sd,
> static int rdacm20_init(struct v4l2_subdev *sd, unsigned int val)
> {
> struct rdacm20_device *dev = sd_to_rdacm20(sd);
> - unsigned int retry = 3;
> + unsigned int i;
> int ret;
>
> /*
> @@ -478,23 +480,18 @@ static int rdacm20_init(struct v4l2_subdev *sd,
> unsigned int val)
> return ret;
> usleep_range(1, 15000);
>
> -again:
> - ret = ov10635_read16(dev, OV10635_PID);
> - if (ret < 0) {
> - if (retry--)
> - goto again;
> + for (i = 0; i < OV10635_PID_TIMEOUT; ++i) {
As commented on a previous patch, this macro is used here only, I would
have made it local.
> + ret = ov10635_read16(dev, OV10635_PID);
> + if (ret == OV10635_VERSION)
> + break;
> + else if (ret >= 0)
> + /* Sometimes we get a successful read but a wrong ID. */
> + dev_dbg(dev->dev, "OV10635 ID mismatch (%d)\n", ret);
>
> - dev_err(dev->dev, "OV10635 ID read failed (%d)\n",
> - ret);
> - return -ENXIO;
> + usleep_range(1000, 2000);
> }
> -
> - if (ret != OV10635_VERSION) {
> - if (retry--)
> - goto again;
> -
> - dev_err(dev->dev, "OV10635 ID mismatch (0x%04x)\n",
> - ret);
Blank line ?
> + if (i == OV10635_PID_TIMEOUT) {
> + dev_err(dev->dev, "OV10635 ID read failed (%d)\n", ret);
> return -ENXIO;
> }
>
--
Regards,
Laurent Pinchart
Re: [PATCH v3 14/19] media: i2c: rdacm20: Enable noise immunity
Hi Jacopo,
Thank you for the patch.
On Fri, Mar 19, 2021 at 05:41:43PM +0100, Jacopo Mondi wrote:
> Enable the noise immunity threshold at the end of the rdacm20
> initialization routine.
>
> The rdacm20 camera module has been so far tested with a startup
> delay that allowed the embedded MCU to program the serializer. If
> the initialization routine is run before the MCU programs the
> serializer and the image sensor and their addresses gets changed
> by the rdacm20 driver it is required to manually enable the noise
> immunity threshold to make the communication on the control channel
> more reliable.
>
> Reviewed-by: Kieran Bingham
> Signed-off-by: Jacopo Mondi
Reviewed-by: Laurent Pinchart
> ---
> drivers/media/i2c/rdacm20.c | 14 +-
> 1 file changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/media/i2c/rdacm20.c b/drivers/media/i2c/rdacm20.c
> index 2881e752efbe..554fd3b3ec3d 100644
> --- a/drivers/media/i2c/rdacm20.c
> +++ b/drivers/media/i2c/rdacm20.c
> @@ -518,7 +518,19 @@ static int rdacm20_init(struct v4l2_subdev *sd, unsigned
> int val)
>
> dev_info(dev->dev, "Identified MAX9271 + OV10635 device\n");
>
> - return 0;
> + /*
> + * Set reverse channel high threshold to increase noise immunity.
> + *
> + * This should be compensated by increasing the reverse channel
> + * amplitude on the remote deserializer side.
> + *
> + * TODO Inspect the embedded MCU programming sequence to make sure
> + * there are no conflicts with the configuration applied here.
> + *
> + * TODO Clarify the embedded MCU startup delay to avoid write
> + * collisions on the I2C bus.
> + */
> + return max9271_set_high_threshold(>serializer, true);
> }
>
> static const struct v4l2_subdev_core_ops rdacm20_core_ops = {
--
Regards,
Laurent Pinchart
Re: [PATCH v3 13/19] media: i2c: rdacm21: Power up OV10640 before OV490
ncapsulate all this.
It would also be nicer to poll an OV490 register instead of sleeping
blindly. That likely requires the OV490 application note (referenced in
the datasheet), which we don't have. It may be useful to try and get
hold of that. I still feel there may be something a bit fishy, but way
less than before.
> +
> + ov490_read_reg(dev, OV490_SCCB_SLAVE0_DIR, );
> + if (val == OV10640_ID_HIGH)
> + break;
> + usleep_range(1000, 1500);
> + }
Blank line ?
> + if (i == OV10640_PID_TIMEOUT) {
> dev_err(dev->dev, "OV10640 ID mismatch: (0x%02x)\n", val);
> return -ENODEV;
> }
> @@ -359,6 +375,8 @@ static int ov490_initialize(struct rdacm21_device *dev)
> unsigned int i;
> int ret;
>
> + ov10640_power_up(dev);
On top of this series, error handling of OV490 writes would make sense.
> +
> /*
>* Read OV490 Id to test communications. Give it up to 40msec to
>* exit from reset.
> @@ -396,7 +414,7 @@ static int ov490_initialize(struct rdacm21_device *dev)
> return -ENODEV;
> }
>
> - ret = ov10640_initialize(dev);
> + ret = ov10640_check_id(dev);
> if (ret)
> return ret;
>
--
Regards,
Laurent Pinchart
Re: [PATCH v3 11/19] media: i2c: rdacm21: Add dealy after OV490 reset
Hi Jacopo, Thank you for the patch. On Fri, Mar 19, 2021 at 04:49:44PM +, Kieran Bingham wrote: > Hi Jacopo, > > s/dealy/delay/ in $SUBJECT > > On 19/03/2021 16:41, Jacopo Mondi wrote: > > Add a delay after the OV490 chip is put in reset state. The reset > > signal shall be held for at least 250 useconds. > > > > Signed-off-by: Jacopo Mondi > > --- > > drivers/media/i2c/rdacm21.c | 6 +- > > 1 file changed, 5 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/media/i2c/rdacm21.c b/drivers/media/i2c/rdacm21.c > > index babd14b21252..875bec9f7904 100644 > > --- a/drivers/media/i2c/rdacm21.c > > +++ b/drivers/media/i2c/rdacm21.c > > @@ -448,7 +448,10 @@ static int rdacm21_init(struct v4l2_subdev *sd, > > unsigned int val) > > if (ret) > > return ret; > > > > - /* Enable GPIO1 and hold OV490 in reset during max9271 configuration. */ > > + /* > > +* Enable GPIO1 and hold OV490 in reset during max9271 configuration. > > +* The reset signal has to be asserted for at least 250 useconds. > > +*/ > > ret = max9271_enable_gpios(>serializer, MAX9271_GPIO1OUT); > > if (ret) > > return ret; > > @@ -456,6 +459,7 @@ static int rdacm21_init(struct v4l2_subdev *sd, > > unsigned int val) > > ret = max9271_clear_gpios(>serializer, MAX9271_GPIO1OUT); > > if (ret) > > return ret; > > + usleep_range(250, 500); > > Aha, for a second there I thought the comment meant it had to be > asserted for 250 uS before clearing it again. But it's 250 uS before > using the OV490. > > Perhaps possible to update the comment a little, but nothing that matters. The commit message and comment should match the code, especially given that I'm not sure here which of the two is actually incorrect. I suspect the sleep is actually in the wrong location. > Reviewed-by: Kieran Bingham > > > > ret = max9271_configure_gmsl_link(>serializer); > > if (ret) > > -- Regards, Laurent Pinchart
Re: [PATCH v3 06/19] media: gmsl: Reimplement initialization sequence
Hi Jacopo,
Thank you for the patch.
On Fri, Mar 19, 2021 at 05:41:35PM +0100, Jacopo Mondi wrote:
> The current probe() procedure of the RDACM20 and RDACM20 GMSL cameras is
s/RDACM20 and RDACM20/RDACM20 and RDACM21/
> performed with the embedded MAX9271 serializer's noise immunity
> threshold disabled. Once the camera has been initialized by probing the
> embedded chips, the threshold is enabled and then compensated on the
> deserializer's side by increasing the reverse channel signal amplitude
> once all cameras have bound.
>
> The probe routine is thus run without noise immunity activated which
> in noisy environment conditions makes the probe sequence less reliable as
> the chips configuration requires a relatively high amount of i2c
> transactions.
>
> Break chip initialization in two:
> - At probe time only configure the serializer's reverse channel with
> noise immunity activated, to reduce the number of transactions
> performed without noise immunity protection enabled
> - Move the chips initialization to the .init() core subdev operation
> called by the deserializer after all camera have probed and
> have increased their noise immunity threshold
>
> The initialization routine looks like the following:
>
> MAX9286 RDACM20/21
>
> probe()
>|
>-> |
> probe() {
> enable_threshold()
> }
>|<|
> v4l2 async bound {
> compensate_amplitude()
> call subdev init()
>|>|
> init() {
> access camera registers()
> }
>|<---
> }
>
> Reviewed-by: Laurent Pinchart
> Reviewed-by: Kieran Bingham
> Signed-off-by: Jacopo Mondi
> ---
> drivers/media/i2c/max9286.c | 19 +++---
> drivers/media/i2c/rdacm20.c | 69 +++--
> drivers/media/i2c/rdacm21.c | 65 --
> 3 files changed, 98 insertions(+), 55 deletions(-)
>
> diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
> index 9124d5fa6ea3..b6347639901e 100644
> --- a/drivers/media/i2c/max9286.c
> +++ b/drivers/media/i2c/max9286.c
> @@ -563,17 +563,28 @@ static int max9286_notify_bound(struct
> v4l2_async_notifier *notifier,
> if (priv->bound_sources != priv->source_mask)
> return 0;
>
> + /*
> + * Initialize all the remote camera. Increase the channel amplitude
> + * to compensate for the remote noise immunity threshold.
> + */
> + max9286_reverse_channel_setup(priv, MAX9286_REV_AMP_HIGH);
> + for_each_source(priv, source) {
> + ret = v4l2_subdev_call(source->sd, core, init, 0);
> + if (ret) {
> + dev_err(>client->dev,
> + "Failed to initialize camera device %u\n",
> + index);
> + return ret;
> + }
> + }
> +
> /*
>* All enabled sources have probed and enabled their reverse control
>* channels:
> - *
> - * - Increase the reverse channel amplitude to compensate for the
> - * remote ends high threshold
>* - Verify all configuration links are properly detected
>* - Disable auto-ack as communication on the control channel are now
>* stable.
>*/
> - max9286_reverse_channel_setup(priv, MAX9286_REV_AMP_HIGH);
> max9286_check_config_link(priv, priv->source_mask);
>
> /*
> diff --git a/drivers/media/i2c/rdacm20.c b/drivers/media/i2c/rdacm20.c
> index 90eb73f0e6e9..3cab6af7eba6 100644
> --- a/drivers/media/i2c/rdacm20.c
> +++ b/drivers/media/i2c/rdacm20.c
> @@ -435,35 +435,12 @@ static int rdacm20_get_fmt(struct v4l2_subdev *sd,
> return 0;
> }
>
> -static const struct v4l2_subdev_video_ops rdacm20_video_ops = {
> - .s_stream = rdacm20_s_stream,
> -};
> -
> -static const struct v4l2_subdev_pad_ops rdacm20_subdev_pad_ops = {
> - .enum_mbus_code = rdacm20_enum_mbus_code,
> - .get_fmt= rdacm20_get_fmt,
> - .set_fmt= rdacm20_get_fmt,
> -};
> -
> -static const struct v4l2_subdev_ops rdacm20_subdev_ops = {
> - .video = _video_ops,
> - .pad= _subdev_pad_ops,
> -};
> -
> -static int rdacm20_initiali
Re: [PATCH v3 05/19] media: v4l2-subdev: De-deprecate init() subdev op
Hi Jacopo,
Thank you for the patch.
CC'ing Sakari on v3 to get feedback.
On Fri, Mar 19, 2021 at 05:41:34PM +0100, Jacopo Mondi wrote:
> The init() subdev core operation is deemed to be deprecated for new
> subdevice drivers. However it could prove useful for complex
> architectures to defer operation that require access to the
> communication bus if said bus is not available (or fully configured)
> at the time when the subdevice probe() function is run.
>
> As an example, the GMSL architecture requires the GMSL configuration
> link to be configured on the host side after the remote subdevice
> has completed its probe function. After the configuration on the host
> side has been performed, the subdevice registers can be accessed through
> the communication bus.
>
> In particular:
>
> HOSTREMOTE
>
> probe()
> |
> -> |
> probe() {
>bus config()
> }
> |<|
> v4l2 async bound {
> bus config()
> call subdev init()
> |>|
>init() {
>access register on the bus()
> }
> |<---
> }
>
> In the GMSL use case the bus configuration requires the enablement of the
> noise immunity threshold on the remote side which ensures reliability
> of communications in electrically noisy environments. After the subdevice
> has enabled the threshold at the end of its probe() sequence the host
> side shall compensate it with an higher signal amplitude. Once this
> sequence has completed the bus can be accessed with noise protection
> enabled and all the operations that require a considerable number of
> transactions on the bus (such as the image sensor configuration
> sequence) are run in the subdevice init() operation implementation.
>
> Signed-off-by: Jacopo Mondi
> ---
> include/media/v4l2-subdev.h | 15 ---
> 1 file changed, 12 insertions(+), 3 deletions(-)
>
> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> index d0e9a5bdb08b..3068d9940669 100644
> --- a/include/media/v4l2-subdev.h
> +++ b/include/media/v4l2-subdev.h
> @@ -148,9 +148,18 @@ struct v4l2_subdev_io_pin_config {
> * each pin being configured. This function could be called at times
> * other than just subdevice initialization.
> *
> - * @init: initialize the sensor registers to some sort of reasonable default
> - * values. Do not use for new drivers and should be removed in existing
> - * drivers.
> + * @init: initialize the subdevice registers to some sort of reasonable
> default
> + * values. Do not use for new drivers (and should be removed in existing
> + * ones) for regular architectures where the image sensor is connected to
> + * the host receiver. For more complex architectures where the subdevice
> + * initialization should be deferred to the completion of the probe
> + * sequence of some intermediate component, or the communication bus
> + * requires configurations on the host side that depend on the completion
> + * of the probe sequence of the remote subdevices, the usage of this
> + * operation could be considered to allow the devices along the pipeline to
> + * probe and register in the media graph and to defer any operation that
> + * require actual access to the communication bus to their init() function
> + * implementation.
> *
> * @load_fw: load firmware.
> *
--
Regards,
Laurent Pinchart
Re: [PATCH] drm/rockchip: Remove unused variable
Hi Maxime,
Thank you for the patch.
On Fri, Mar 19, 2021 at 04:29:20PM +0100, Maxime Ripard wrote:
> Commit 977697e20b3d ("drm/atomic: Pass the full state to planes atomic
> disable and update") added the old_state variable instead of what used
> to be a parameter, but it also removed the sole user of that variable in
> the vop_plane_atomic_update function leading to an usused variable.
> Remove it.
>
> Fixes: 977697e20b3d ("drm/atomic: Pass the full state to planes atomic
> disable and update")
> Reported-by: Stephen Rothwell
> Signed-off-by: Maxime Ripard
Reviewed-by: Laurent Pinchart
> ---
> drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 2 --
> 1 file changed, 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> index 81c70d7a0471..64469439ddf2 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> @@ -879,8 +879,6 @@ static void vop_plane_atomic_disable(struct drm_plane
> *plane,
> static void vop_plane_atomic_update(struct drm_plane *plane,
> struct drm_atomic_state *state)
> {
> - struct drm_plane_state *old_state =
> drm_atomic_get_old_plane_state(state,
> -
> plane);
> struct drm_plane_state *new_state =
> drm_atomic_get_new_plane_state(state,
>
> plane);
> struct drm_crtc *crtc = new_state->crtc;
--
Regards,
Laurent Pinchart
Re: [PATCH v2] phy: zynqmp: Handle the clock enable/disable properly
> return 0;
> +
> +err_clk_put:
> + for (refclk = 0; refclk < ARRAY_SIZE(gtr_dev->clk); refclk++)
> + clk_disable_unprepare(gtr_dev->clk[refclk]);
Here too you will disable a clock that hasn't been enabled if
clk_prepare_enable() failed. Clocks that haven't been retrieved at all
are fine, as clk_disable_unprepare() can be safely called with a NULL
pointer. This could be fixed by replacing the code above with
ret = clk_prepare_enable(clk);
if (ret)
goto err_clk_put;
gtr_dev->clk[refclk] = clk;
> +
> + return ret;
> }
>
> static int xpsgtr_probe(struct platform_device *pdev)
> @@ -912,6 +946,7 @@ static int xpsgtr_probe(struct platform_device *pdev)
> struct xpsgtr_dev *gtr_dev;
> struct phy_provider *provider;
> unsigned int port;
> + unsigned int i;
> int ret;
>
> gtr_dev = devm_kzalloc(>dev, sizeof(*gtr_dev), GFP_KERNEL);
> @@ -951,7 +986,8 @@ static int xpsgtr_probe(struct platform_device *pdev)
> phy = devm_phy_create(>dev, np, _phyops);
> if (IS_ERR(phy)) {
> dev_err(>dev, "failed to create PHY\n");
> - return PTR_ERR(phy);
> + ret = PTR_ERR(phy);
> + goto err_clk_put;
> }
>
> gtr_phy->phy = phy;
> @@ -962,9 +998,16 @@ static int xpsgtr_probe(struct platform_device *pdev)
> provider = devm_of_phy_provider_register(>dev, xpsgtr_xlate);
> if (IS_ERR(provider)) {
> dev_err(>dev, "registering provider failed\n");
> - return PTR_ERR(provider);
> + ret = PTR_ERR(provider);
> + goto err_clk_put;
> }
> return 0;
> +
> +err_clk_put:
> + for (i = 0; i < ARRAY_SIZE(gtr_dev->clk); i++)
> + clk_disable_unprepare(gtr_dev->clk[i]);
> +
> + return ret;
> }
>
> static const struct of_device_id xpsgtr_of_match[] = {
--
Regards,
Laurent Pinchart
Re: [PATCH v2 12/18] media: i2c: rdacm21: Give more time to OV490 to boot
Hi Jacopo,
On Wed, Mar 17, 2021 at 11:04:45AM +0100, Jacopo Mondi wrote:
> On Mon, Mar 15, 2021 at 05:22:37PM +, Kieran Bingham wrote:
> > On 15/03/2021 13:15, Jacopo Mondi wrote:
> > > It has been observed through repeated testing (250 boots) that in the
> > > 10% of the cases the RDACM21 initialization sequence errors out due a
> > > timeout waiting for the OV490 firmware to complete its boot phase.
> > >
> > > Albeit being the current timeout relatively large (300-600 milliseconds),
> > > doubling it reduces the sporadic error rate down to 1 over an 80 boot
> > > sequences test run.
> > >
> > > The firmware boot delay is unfortunately not characterized in the camera
> > > module manual.
> >
> > I wonder if we could characterize this alone by pulling this down until
> > we see failures increase, with all the other fixes in place...
> >
> > I don't think that's required, but it might be something to check later
> > if we don't get rid of that 1/80 failure.
>
> This is actually driving me crazy :/
>
> I had another test run with a surprising 10% failures.
> All the failures were due to the ov490 firmware boot I'm trying to
> mitigate here.
>
> I went up to give it -6 seconds- and I still get failures in the same
> percentage. Another run of 20 boots gave 30% failures with the delay I
> have here in this patch. Just to make sure I was not going crazy I
> reduced the delay to 1msec and I get an 80% failure rate.
>
> Still, I've seen the 1 on 80 failures (I swear! I have logs! :)
>
> I've checked what the BSP does, and if after some 300 attempts the
> ov490 doesn't boot, they simply go an reset it.
> https://github.com/renesas-rcar/linux-bsp/commit/0cf6e36f5bf49e1c2aab87139ec5b588623c56f8#diff-d770cad7d6f04923d9e89dfe7da369bb3006776d6e4fb8ef79353d5fab3cd25aR827
> (sorry, I don't seem to be able to point you to the ov490.c#827 with
> an URL)
It resets both the sensor and the OV490. It could be interested to try
the latter selectively to see what happens.
I also suspect that the OV490 has debugging features (possibly including
a RAM log buffer that we could read over I2C), but we're probably
getting out of scope here.
> I assume we don't want anything like this in an upstream driver, but
> I'm really running out of any plausible explanation :(
As discussed, let's try the reset workaround, to see if it helps.
I wonder if opening the camera and probing signals would be a useful
option :-)
> > > Fixes: a59f853b3b4b ("media: i2c: Add driver for RDACM21 camera module")
> > > Signed-off-by: Jacopo Mondi
> >
> > Reviewed-by: Kieran Bingham
> >
> > > ---
> > > drivers/media/i2c/rdacm21.c | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/media/i2c/rdacm21.c b/drivers/media/i2c/rdacm21.c
> > > index 50a9b0d8255d..07cf077d8efd 100644
> > > --- a/drivers/media/i2c/rdacm21.c
> > > +++ b/drivers/media/i2c/rdacm21.c
> > > @@ -53,7 +53,7 @@
> > > #define OV490_PID0x8080300a
> > > #define OV490_VER0x8080300b
> > > #define OV490_PID_TIMEOUT20
> > > -#define OV490_OUTPUT_EN_TIMEOUT 300
> > > +#define OV490_OUTPUT_EN_TIMEOUT 600
> > >
> > > #define OV490_GPIO0 BIT(0)
> > > #define OV490_SPWDN0 BIT(0)
--
Regards,
Laurent Pinchart
Re: [PATCH v4 1/2] dt-bindings: drm/bridge: MHDP8546 bridge binding changes for HDCP
Hi Parshuram,
Thank you for the patch.
On Thu, Mar 18, 2021 at 07:45:30AM +0100, Parshuram Thombare wrote:
> Add binding changes for HDCP in the MHDP8546 DPI/DP bridge binding.
>
> Signed-off-by: Parshuram Thombare
> ---
> .../display/bridge/cdns,mhdp8546.yaml | 24 +++
> 1 file changed, 14 insertions(+), 10 deletions(-)
>
> diff --git
> a/Documentation/devicetree/bindings/display/bridge/cdns,mhdp8546.yaml
> b/Documentation/devicetree/bindings/display/bridge/cdns,mhdp8546.yaml
> index 63427878715e..8a85768f6202 100644
> --- a/Documentation/devicetree/bindings/display/bridge/cdns,mhdp8546.yaml
> +++ b/Documentation/devicetree/bindings/display/bridge/cdns,mhdp8546.yaml
> @@ -17,8 +17,8 @@ properties:
>- ti,j721e-mhdp8546
>
>reg:
> -minItems: 1
> -maxItems: 2
> +minItems: 2
> +maxItems: 3
> items:
>- description:
>Register block of mhdptx apb registers up to PHY mapped area
> (AUX_CONFIG_P).
> @@ -26,13 +26,16 @@ properties:
>included in the associated PHY.
>- description:
>Register block for DSS_EDP0_INTG_CFG_VP registers in case of TI J7
> SoCs.
> + - description:
> + Register block of mhdptx sapb registers.
>
>reg-names:
> -minItems: 1
> -maxItems: 2
> +minItems: 2
> +maxItems: 3
> items:
>- const: mhdptx
>- const: j721e-intg
> + - const: mhdptx-sapb
>
>clocks:
> maxItems: 1
> @@ -98,15 +101,15 @@ allOf:
> then:
>properties:
> reg:
> - minItems: 2
> + minItems: 3
> reg-names:
> - minItems: 2
> + minItems: 3
> else:
>properties:
> reg:
> - maxItems: 1
> + maxItems: 2
> reg-names:
> - maxItems: 1
> + maxItems: 2
>
> required:
>- compatible
> @@ -129,8 +132,9 @@ examples:
>
> mhdp: dp-bridge@f0fb00 {
> compatible = "cdns,mhdp8546";
> -reg = <0xf0 0xfb00 0x0 0x100>;
> -reg-names = "mhdptx";
> +reg = <0xf0 0xfb00 0x0 0x100>,
> + <0x0 0x4f48000 0x0 0x74>;
> +reg-names = "mhdptx", "mhdptx-sapb";
Running
make dt_binding_check
DT_SCHEMA_FILES=Documentation/devicetree/bindings/display/bridge/cdns,mhdp8546.yaml
produces
LINTDocumentation/devicetree/bindings
CHKDT Documentation/devicetree/bindings/processed-schema-examples.json
SCHEMA Documentation/devicetree/bindings/processed-schema-examples.json
DTEX
Documentation/devicetree/bindings/display/bridge/cdns,mhdp8546.example.dts
DTC
Documentation/devicetree/bindings/display/bridge/cdns,mhdp8546.example.dt.yaml
CHECK
Documentation/devicetree/bindings/display/bridge/cdns,mhdp8546.example.dt.yaml
Documentation/devicetree/bindings/display/bridge/cdns,mhdp8546.example.dt.yaml:
dp-bridge@f0fb00: reg-names:1: 'j721e-intg' was expected
From schema:
Documentation/devicetree/bindings/display/bridge/cdns,mhdp8546.yaml
This is caused by the fact that reg-names is correctly limited to two
elements, but then expects the second element to be "j721e-intg". The
example is good, so it's the bindings that need to be fixed.
> clocks = <_clock>;
> phys = <_phy>;
> phy-names = "dpphy";
--
Regards,
Laurent Pinchart
Re: [PATCH 2/2] media: v4l2-core: explicitly clear ioctl input data
Hi Arnd,
Thank you for the patch.
On Thu, Mar 18, 2021 at 02:43:19PM +0100, Arnd Bergmann wrote:
> From: Arnd Bergmann
>
> As seen from a recent syzbot bug report, mistakes in the compat ioctl
> implementation can lead to uninitialized kernel stack data getting used
> as input for driver ioctl handlers.
>
> The reported bug is now fixed, but it's possible that other related
> bugs are still present or get added in the future. As the drivers need
> to check user input already, the possible impact is fairly low, but it
> might still cause an information leak.
>
> To be on the safe side, always clear the entire ioctl buffer before
> calling the conversion handler functions that are meant to initialize
> them.
>
> Signed-off-by: Arnd Bergmann
Reviewed-by: Laurent Pinchart
> ---
> drivers/media/v4l2-core/v4l2-ioctl.c | 51
> 1 file changed, 29 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c
> b/drivers/media/v4l2-core/v4l2-ioctl.c
> index 2b1bb68dc27f..6cec92d0972c 100644
> --- a/drivers/media/v4l2-core/v4l2-ioctl.c
> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
> @@ -3164,12 +3164,23 @@ static int video_get_user(void __user *arg, void
> *parg,
>
> if (cmd == real_cmd) {
> if (copy_from_user(parg, (void __user *)arg, n))
> - err = -EFAULT;
> - } else if (in_compat_syscall()) {
> - err = v4l2_compat_get_user(arg, parg, cmd);
> - } else {
> - switch (cmd) {
> + return -EFAULT;
> +
> + /* zero out anything we don't copy from userspace */
> + if (n < _IOC_SIZE(real_cmd))
> + memset((u8 *)parg + n, 0, _IOC_SIZE(real_cmd) - n);
> +
> + return 0;
> + }
> +
> + /* zero out whole buffer first to deal with missing emulation */
> + memset(parg, 0, _IOC_SIZE(real_cmd));
> +
> + if (in_compat_syscall())
> + return v4l2_compat_get_user(arg, parg, cmd);
> +
> #if !defined(CONFIG_64BIT) && defined(CONFIG_COMPAT_32BIT_TIME)
> + switch (cmd) {
> case VIDIOC_QUERYBUF_TIME32:
> case VIDIOC_QBUF_TIME32:
> case VIDIOC_DQBUF_TIME32:
> @@ -3182,28 +3193,24 @@ static int video_get_user(void __user *arg, void
> *parg,
>
> *vb = (struct v4l2_buffer) {
> .index = vb32.index,
> - .type = vb32.type,
> - .bytesused = vb32.bytesused,
> - .flags = vb32.flags,
> - .field = vb32.field,
> - .timestamp.tv_sec =
> vb32.timestamp.tv_sec,
> - .timestamp.tv_usec =
> vb32.timestamp.tv_usec,
> - .timecode = vb32.timecode,
> - .sequence = vb32.sequence,
> - .memory = vb32.memory,
> - .m.userptr = vb32.m.userptr,
> - .length = vb32.length,
> - .request_fd = vb32.request_fd,
> + .type = vb32.type,
> + .bytesused = vb32.bytesused,
> + .flags = vb32.flags,
> + .field = vb32.field,
> + .timestamp.tv_sec = vb32.timestamp.tv_sec,
> + .timestamp.tv_usec =
> vb32.timestamp.tv_usec,
> + .timecode = vb32.timecode,
> + .sequence = vb32.sequence,
> + .memory = vb32.memory,
> + .m.userptr = vb32.m.userptr,
> + .length = vb32.length,
> + .request_fd = vb32.request_fd,
> };
> break;
> }
> -#endif
> - }
> }
> +#endif
>
> - /* zero out anything we don't copy from userspace */
> - if (!err && n < _IOC_SIZE(real_cmd))
> - memset((u8 *)parg + n, 0, _IOC_SIZE(real_cmd) - n);
> return err;
> }
>
--
Regards,
Laurent Pinchart
Re: [PATCH 1/2] media: v4l2-core: ignore native time32 ioctls on 64-bit
Hi Arnd,
Thank you for the patch.
On Thu, Mar 18, 2021 at 02:43:18PM +0100, Arnd Bergmann wrote:
> From: Arnd Bergmann
>
> Syzbot found that passing ioctl command 0xc0505609 into a 64-bit
> kernel from a 32-bit process causes uninitialized kernel memory to
> get passed to drivers instead of the user space data:
>
> BUG: KMSAN: uninit-value in check_array_args
> drivers/media/v4l2-core/v4l2-ioctl.c:3041 [inline]
> BUG: KMSAN: uninit-value in video_usercopy+0x1631/0x3d30
> drivers/media/v4l2-core/v4l2-ioctl.c:3315
> CPU: 0 PID: 19595 Comm: syz-executor.4 Not tainted 5.11.0-rc7-syzkaller #0
> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS
> Google 01/01/2011
> Call Trace:
> __dump_stack lib/dump_stack.c:79 [inline]
> dump_stack+0x21c/0x280 lib/dump_stack.c:120
> kmsan_report+0xfb/0x1e0 mm/kmsan/kmsan_report.c:118
> __msan_warning+0x5f/0xa0 mm/kmsan/kmsan_instr.c:197
> check_array_args drivers/media/v4l2-core/v4l2-ioctl.c:3041 [inline]
> video_usercopy+0x1631/0x3d30 drivers/media/v4l2-core/v4l2-ioctl.c:3315
> video_ioctl2+0x9f/0xb0 drivers/media/v4l2-core/v4l2-ioctl.c:3391
> v4l2_ioctl+0x255/0x290 drivers/media/v4l2-core/v4l2-dev.c:360
> v4l2_compat_ioctl32+0x2c6/0x370
> drivers/media/v4l2-core/v4l2-compat-ioctl32.c:1248
> __do_compat_sys_ioctl fs/ioctl.c:842 [inline]
> __se_compat_sys_ioctl+0x53d/0x1100 fs/ioctl.c:793
> __ia32_compat_sys_ioctl+0x4a/0x70 fs/ioctl.c:793
> do_syscall_32_irqs_on arch/x86/entry/common.c:79 [inline]
> __do_fast_syscall_32+0x102/0x160 arch/x86/entry/common.c:141
> do_fast_syscall_32+0x6a/0xc0 arch/x86/entry/common.c:166
> do_SYSENTER_32+0x73/0x90 arch/x86/entry/common.c:209
> entry_SYSENTER_compat_after_hwframe+0x4d/0x5c
>
> The time32 commands are defined but were never meant to be called on
> 64-bit machines, as those have always used time64 interfaces. I missed
> this in my patch that introduced the time64 handling on 32-bit platforms.
>
> The problem in this case is the mismatch of one function checking for
> the numeric value of the command and another function checking for the
> type of process (native vs compat) instead, with the result being that
> for this combination, nothing gets copied into the buffer at all.
>
> Avoid this by only trying to convert the time32 commands when running
> on a 32-bit kernel where these are defined in a meaningful way.
>
> Fixes: 577c89b0ce72 ("media: v4l2-core: fix v4l2_buffer handling for time64
> ABI")
> Reported-by: syzbot+142888ffec98ab194...@syzkaller.appspotmail.com
> Tested-by: Hans Verkuil
> Signed-off-by: Arnd Bergmann
v4l2_event vs. v4l2_event32 vs. v4l2_event_time32 vs.
v4l2_event32_time32 is a bit confusing. Do I understand correctly that
the code below runs for the non-compat path, thus native userspace
(32-bit on 32-bit machines, 64-bit on 64-bit machines), and handles the
case of a native userspace using a 32-bit time ? If so it indeed doesn't
make sense for 64-bit machines.
Reviewed-by: Laurent Pinchart
> ---
> This patch adds two more changes than the version that Hans tested
> ---
> drivers/media/v4l2-core/v4l2-ioctl.c | 6 +++---
> drivers/media/v4l2-core/v4l2-subdev.c | 2 +-
> 2 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c
> b/drivers/media/v4l2-core/v4l2-ioctl.c
> index 31d1342e61e8..2b1bb68dc27f 100644
> --- a/drivers/media/v4l2-core/v4l2-ioctl.c
> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
> @@ -3115,7 +3115,7 @@ static int check_array_args(unsigned int cmd, void
> *parg, size_t *array_size,
> static unsigned int video_translate_cmd(unsigned int cmd)
> {
> switch (cmd) {
> -#ifdef CONFIG_COMPAT_32BIT_TIME
> +#if !defined(CONFIG_64BIT) && defined(CONFIG_COMPAT_32BIT_TIME)
> case VIDIOC_DQEVENT_TIME32:
> return VIDIOC_DQEVENT;
> case VIDIOC_QUERYBUF_TIME32:
> @@ -3169,7 +3169,7 @@ static int video_get_user(void __user *arg, void *parg,
> err = v4l2_compat_get_user(arg, parg, cmd);
> } else {
> switch (cmd) {
> -#ifdef CONFIG_COMPAT_32BIT_TIME
> +#if !defined(CONFIG_64BIT) && defined(CONFIG_COMPAT_32BIT_TIME)
> case VIDIOC_QUERYBUF_TIME32:
> case VIDIOC_QBUF_TIME32:
> case VIDIOC_DQBUF_TIME32:
> @@ -3224,7 +3224,7 @@ static int video_put_user(void __user *arg, void *parg,
> return v4l2_compat_put_user(arg, parg, cmd);
>
> switch (cmd) {
> -#ifdef CONFIG_COMPAT_32BIT_TIME
> +#if !defined(CONFIG_64BIT) && defined(CONFIG_COMPAT_32BIT_TIME)
> case VIDIOC_DQEVENT_TIME32: {
> struct v4l2_event *ev = parg;
> struct v4l2_event_time32 ev32;
>
Re: [PATCH v3 3/4] drm/bridge: ti-sn65dsi86: Read EDID blob over DDC
Hi Stephen,
Reviving a bit of an old thread, for a question.
On Mon, Nov 02, 2020 at 10:11:43AM -0800, Stephen Boyd wrote:
> Use the DDC connection to read the EDID from the eDP panel instead of
> relying on the panel to tell us the modes.
>
> Reviewed-by: Douglas Anderson
> Reviewed-by: Laurent Pinchart
> Cc: Jonas Karlman
> Cc: Jernej Skrabec
> Cc: Sean Paul
> Acked-by: Sam Ravnborg
> Signed-off-by: Stephen Boyd
> ---
> drivers/gpu/drm/bridge/ti-sn65dsi86.c | 20
> 1 file changed, 20 insertions(+)
>
> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> index 8276fa50138f..6b6e98ca2881 100644
> --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> @@ -119,6 +119,7 @@
> * @debugfs: Used for managing our debugfs.
> * @host_node:Remote DSI node.
> * @dsi: Our MIPI DSI source.
> + * @edid: Detected EDID of eDP panel.
> * @refclk: Our reference clock.
> * @panel:Our panel.
> * @enable_gpio: The GPIO we toggle to enable the bridge.
> @@ -144,6 +145,7 @@ struct ti_sn_bridge {
> struct drm_bridge bridge;
> struct drm_connectorconnector;
> struct dentry *debugfs;
> + struct edid *edid;
> struct device_node *host_node;
> struct mipi_dsi_device *dsi;
> struct clk *refclk;
> @@ -265,6 +267,23 @@ connector_to_ti_sn_bridge(struct drm_connector
> *connector)
> static int ti_sn_bridge_connector_get_modes(struct drm_connector *connector)
> {
> struct ti_sn_bridge *pdata = connector_to_ti_sn_bridge(connector);
> + struct edid *edid = pdata->edid;
> + int num, ret;
> +
> + if (!edid) {
> + pm_runtime_get_sync(pdata->dev);
> + edid = pdata->edid = drm_get_edid(connector, >aux.ddc);
> + pm_runtime_put(pdata->dev);
Is there any specific reason to use the indirect access method, compared
to the direct method that translates access to an I2C ancillary address
to an I2C-over-AUX transaction (see page 20 of SLLSEH2B) ? The direct
method seems it would be more efficient.
> + }
> +
> + if (edid && drm_edid_is_valid(edid)) {
> + ret = drm_connector_update_edid_property(connector, edid);
> + if (!ret) {
> + num = drm_add_edid_modes(connector, edid);
> + if (num)
> + return num;
> + }
> + }
>
> return drm_panel_get_modes(pdata->panel, connector);
> }
> @@ -1245,6 +1264,7 @@ static int ti_sn_bridge_remove(struct i2c_client
> *client)
> if (!pdata)
> return -EINVAL;
>
> + kfree(pdata->edid);
> ti_sn_debugfs_remove(pdata);
>
> of_node_put(pdata->host_node);
--
Regards,
Laurent Pinchart
Re: [PATCH v2 1/4] dt-bindings: media: max9286: Describe gpio-hog
Hi Jacopo, On Wed, Mar 17, 2021 at 11:14:12AM +0100, Jacopo Mondi wrote: > On Tue, Mar 16, 2021 at 12:15:16AM +0200, Laurent Pinchart wrote: > > On Mon, Mar 15, 2021 at 05:30:25PM +0100, Jacopo Mondi wrote: > > > The MAX9286 GMSL deserializer features gpio controller capabilities, > > > as it provides 2 GPIO lines. > > > > > > As establishing a regulator that uses one of the GPIO lines and > > > enabling/disabling it at run-time in the max9286 won't work due to > > > a circular dependency on the gpio-controller/regulator creation, allow > > > the usage of a gpio-hog for that purpose. > > > > > > The usage of the gpio-hog is required in designs where the MAX9286 > > > GPIO lines control the remote cameras power. > > > > > > Signed-off-by: Jacopo Mondi > > > > That's really a workaround until we can find a good solution, do we have > > to officially support it in the DT bindings ? > > > > That's an interesting question. The 'good' solution implies resolving > the circular dependency on the regulator/gpio-controller creation and > I feel like it might take a while to find a proper solution. How about not using a regulator in that case ? If a MAX9286 GPIO is used to controlled the camera power, we could describe that specifically in DT, and handle everything within the MAX9286 driver in that case. It could be considered as a bit of a hack, but it would be very localized, and I think that trying to fix it in a way that would involve the regulator framework would actually be worse as the complexity would be bigger, for little gain. > In the meantime, all designs like Eagle that control the camera power > through a MAX9286 gpio have to rely on this. I'll go with the majority > here: either we add this and upstream the gmsl .dtsi for eagle, or we > keep out-of-tree patches :/ I don't mind having it upstream in the driver as a workaround, but I'd like to keep it out of the DT bindings as it shouldn't be considered as an officially supported option. Would that work for you ? > > > --- > > > .../bindings/media/i2c/maxim,max9286.yaml| 16 > > > 1 file changed, 16 insertions(+) > > > > > > diff --git > > > a/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml > > > b/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml > > > index ee16102fdfe7..9038300e373c 100644 > > > --- a/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml > > > +++ b/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml > > > @@ -177,6 +177,22 @@ properties: > > > > > > additionalProperties: false > > > > > > +patternProperties: > > > + "^(hog-[0-9]+|.+-hog(-[0-9]+)?)$": > > > +type: object > > > +properties: > > > + gpio-hog: true > > > + gpios: true > > > + output-low: true > > > + line-name: true > > > + > > > +required: > > > + - gpio-hog > > > + - gpios > > > + - output-low > > > + > > > +additionalProperties: false > > > + > > > required: > > >- compatible > > >- reg -- Regards, Laurent Pinchart
Re: [PATCH 3/3] drm/bridge: ti-sn65dsi86: Properly get the EDID, but only if refclk
Hi Doug,
On Mon, Mar 15, 2021 at 09:25:37AM -0700, Doug Anderson wrote:
> On Sat, Mar 13, 2021 at 1:17 PM Laurent Pinchart wrote:
> > On Thu, Mar 04, 2021 at 03:52:01PM -0800, Douglas Anderson wrote:
> > > In commit 58074b08c04a ("drm/bridge: ti-sn65dsi86: Read EDID blob over
> > > DDC") we attempted to make the ti-sn65dsi86 bridge properly read the
> > > EDID from the panel. That commit kinda worked but it had some serious
> > > problems.
> > >
> > > The problems all stem from the fact that userspace wants to be able to
> > > read the EDID before it explicitly enables the panel. For eDP panels,
> > > though, we don't actually power the panel up until the pre-enable
> > > stage and the pre-enable call happens right before the enable call
> > > with no way to interject in-between. For eDP panels, you can't read
> > > the EDID until you power the panel. The result was that
> > > ti_sn_bridge_connector_get_modes() was always failing to read the EDID
> > > (falling back to what drm_panel_get_modes() returned) until _after_
> > > the EDID was needed.
> > >
> > > To make it concrete, on my system I saw this happen:
> > > 1. We'd attach the bridge.
> > > 2. Userspace would ask for the EDID (several times). We'd try but fail
> > >to read the EDID over and over again and fall back to the hardcoded
> > >modes.
> > > 3. Userspace would decide on a mode based only on the hardcoded modes.
> > > 4. Userspace would ask to turn the panel on.
> > > 5. Userspace would (eventually) check the modes again (in Chrome OS
> > >this happens on the handoff from the boot splash screen to the
> > >browser). Now we'd read them properly and, if they were different,
> > >userspace would request to change the mode.
> > >
> > > The fact that userspace would always end up using the hardcoded modes
> > > at first significantly decreases the benefit of the EDID
> > > reading. Also: if the modes were even a tiny bit different we'd end up
> > > doing a wasteful modeset and at boot.
> >
> > s/and at/at/ ?
>
> Sure, I can correct if/when I respin or it can be corrected when landed.
>
> > > diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > > b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > > index 491c9c4f32d1..af3fb4657af6 100644
> > > --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > > +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > > @@ -16,6 +16,7 @@
> > > #include
> > > #include
> > > #include
> > > +#include
> > >
> > > #include
> > >
> > > @@ -130,6 +131,12 @@
> > > * @ln_assign:Value to program to the LN_ASSIGN register.
> > > * @ln_polrs: Value for the 4-bit LN_POLRS field of SN_ENH_FRAME_REG.
> > > *
> > > + * @pre_enabled_early: If true we did an early pre_enable at attach.
> > > + * @pre_enable_timeout_work: Delayed work to undo the pre_enable from
> > > attach
> > > + * if a normal pre_enable never came.
> >
> > Could we simplify this by using the runtime PM autosuspend feature ? The
> > configuration of the bridge would be moved from pre_enable to the PM
> > runtime resume handler, the clk_disable_unprepare() call moved from
> > post_disable to the runtime suspend handler, and the work queue replaced
> > by usage of pm_runtime_put_autosuspend().
>
> It's an interesting idea but I don't think I can make it work, at
> least not in a generic enough way. Specifically we can also use this
> bridge chip as a generic GPIO provider in Linux. When someone asks us
> to read a GPIO then we have to power the bridge on
> (pm_runtime_get_sync()) and when someone asks us to configure a GPIO
> as an output then we actually leave the bridge powered until they stop
> requesting it as an output. At the moment the only user of this
> functionality (that I know of) is for the HPD pin on trogdor boards
> (long story about why we don't use the dedicated HPD) but the API
> supports using these GPIOs for anything and I've tested that it works.
> It wouldn't be great to have to keep the panel on in order to access
> the GPIOs.
The issue you're trying to fix doesn't seem specific to this bridge, so
handling it in the bridge driver bothers me :-S Is there any way we
could handle this in the DRM core ? I don't want to see similar
implementations duplicated in all HDMI/DP bridges.
> The other problem is that I think the time scales are different. At
> boot time I think we'd want to leave the panel on for tens of seconds
> to give userspace a chance to start up and configure the panel. After
> userspace starts up I think we'd want autosuspend to be much faster.
> This could probably be solved by tweaking the runtime delay in code
> but I didn't fully dig because of the above problem.
--
Regards,
Laurent Pinchart
Re: [PATCH] dt-bindings: media: video-interfaces: Use documented bindings in example
Hi Rob,
On Tue, Mar 16, 2021 at 03:09:10PM -0600, Rob Herring wrote:
> On Tue, Mar 16, 2021 at 2:48 PM Laurent Pinchart wrote:
> > On Tue, Mar 16, 2021 at 01:51:00PM -0600, Rob Herring wrote:
> > > The example in video-interfaces.yaml managed to use a bunch of
> > > undocumented
> > > bindings. Update the example to use real bindings (and ones with a
> > > schema).
> > >
> > > Cc: Mauro Carvalho Chehab
> > > Cc: Sakari Ailus
> > > Cc: Laurent Pinchart
> > > Cc: linux-me...@vger.kernel.org
> > > Signed-off-by: Rob Herring
> > > ---
> > > .../bindings/media/video-interfaces.yaml | 75 ---
> > > 1 file changed, 33 insertions(+), 42 deletions(-)
> > >
> > > diff --git
> > > a/Documentation/devicetree/bindings/media/video-interfaces.yaml
> > > b/Documentation/devicetree/bindings/media/video-interfaces.yaml
> > > index 0a7a73fd59f2..f30b9b91717b 100644
> > > --- a/Documentation/devicetree/bindings/media/video-interfaces.yaml
> > > +++ b/Documentation/devicetree/bindings/media/video-interfaces.yaml
> > > @@ -227,17 +227,12 @@ examples:
> > ># only one of the following data pipelines can be active:
> > ># ov772x -> ceu0 or imx074 -> csi2 -> ceu0.
> > >- |
> > > +#include
> > > +#include
> > > +#include
> > > +
> > > ceu@fe91 {
> > > -compatible = "renesas,sh-mobile-ceu";
> > > reg = <0xfe91 0xa0>;
> > > -interrupts = <0x880>;
> > > -
> > > -mclk: master_clock {
> > > -compatible = "renesas,ceu-clock";
> > > -#clock-cells = <1>;
> > > -clock-frequency = <5000>; /* Max clock frequency */
> > > -clock-output-names = "mclk";
> > > -};
> > >
> > > port {
> > > #address-cells = <1>;
> > > @@ -271,18 +266,14 @@ examples:
> > > #size-cells = <0>;
> > >
> > > camera@21 {
> > > -compatible = "ovti,ov772x";
> > > +compatible = "ovti,ov7720";
> > > reg = <0x21>;
> > > -vddio-supply = <>;
> > > -vddcore-supply = <>;
> > > -
> > > -clock-frequency = <2000>;
> > > clocks = < 0>;
> > > -clock-names = "xclk";
> > >
> > > port {
> > > /* With 1 endpoint per port no need for addresses. */
> > > ov772x_1_1: endpoint {
> > > +bus-type = <5>;
> > > bus-width = <8>;
> > > remote-endpoint = <_1>;
> > > hsync-active = <1>;
> > > @@ -295,48 +286,48 @@ examples:
> > > };
> > >
> > > camera@1a {
> > > -compatible = "sony,imx074";
> > > +compatible = "sony,imx334";
> > > reg = <0x1a>;
> > > -vddio-supply = <>;
> > > -vddcore-supply = <>;
> > >
> > > -clock-frequency = <3000>; /* Shared clock with ov772x_1
> > > */
> > > clocks = < 0>;
> > > -clock-names = "sysclk";/* Assuming this is the
> > > - name in the datasheet */
> > > +
> > > port {
> > > -imx074_1: endpoint {
> > > +imx334_1: endpoint {
> > > clock-lanes = <0>;
> > > data-lanes = <1 2>;
> > > +link-frequencies = /bits/ 64 <89100>;
> > > remote-endpoint = <_1>;
> > > };
> > > };
> > > };
> > > };
> > >
> > > -csi2: csi2@ffc9 {
> > > -compatible = "renesas,sh-mobile-csi2";
> > > -reg = <0xffc9 0x1000>;
> > > -interrupts = <0x17a0>;
> > > -#address-cells = <1>;
> > > -#size-cells = <0>;
> > > +csi2@fea8 {
> > > +compatible = "renesas,r8a7796-csi2";
> >
> > That's certainly better, but the r8a7796 doesn't have a CEU :-) It has a
> > VIN. Maybe we could copy the last example from renesas,vin.yaml to
> > replace the CEU ?
>
> What about just removing the example here? It bothers me to have 2
> copies (maybe 3 with sensor schemas) of an example and we should have
> plenty of examples.
I'd be fine with that.
> On the flip side, it's nice to have this stand on its own. Another
> option would be just remove compatibles and make the example barebones
> with only what's defined in video-interfaces.yaml.
Abstract examples seem good in this context.
> But then it's not validated at all.
But this part isn't nice :-(
If we keep examples that use real bindings, they should match the real
hardware platforms. Other than that, I have no strong preference, it's
up to you.
--
Regards,
Laurent Pinchart
