Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 7 Jun 2023 18:16:19 +0100
From:   Conor Dooley <conor@kernel.org>
To:     Tommaso Merciai <tomm.merciai@gmail.com>
Cc:     jacopo.mondi@ideasonboard.com, laurent.pinchart@ideasonboard.com,
        martin.hecht@avnet.eu, michael.roeder@avnet.eu,
        linuxfancy@googlegroups.com,
        Mauro Carvalho Chehab <mchehab@kernel.org>,
        Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Conor Dooley <conor+dt@kernel.org>,
        Liam Girdwood <lgirdwood@gmail.com>,
        Mark Brown <broonie@kernel.org>,
        Sakari Ailus <sakari.ailus@linux.intel.com>,
        Hans Verkuil <hverkuil@xs4all.nl>,
        Marco Felsch <m.felsch@pengutronix.de>,
        Gerald Loacker <gerald.loacker@wolfvision.net>,
        Mikhail Rudenko <mike.rudenko@gmail.com>,
        Nicholas Roth <nicholas@rothemail.net>,
        Linus Walleij <linus.walleij@linaro.org>,
        Michael Riesch <michael.riesch@wolfvision.net>,
        Benjamin Mugnier <benjamin.mugnier@foss.st.com>,
        Krzysztof =?utf-8?Q?Ha=C5=82asa?= <khalasa@piap.pl>,
        Shawn Tu <shawnx.tu@intel.com>, linux-media@vger.kernel.org,
        devicetree@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v4 2/3] media: dt-bindings: alvium: add document YAML
 binding
Message-ID: <20230607-pyromania-germinate-b5e3dbd54632@spud>
References: <20230607131936.382406-1-tomm.merciai@gmail.com>
 <20230607131936.382406-3-tomm.merciai@gmail.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha256;
        protocol="application/pgp-signature"; boundary="ld8OWH6vGquOMZTs"
Content-Disposition: inline
In-Reply-To: <20230607131936.382406-3-tomm.merciai@gmail.com>
Precedence: bulk


--ld8OWH6vGquOMZTs
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

Hey Tommaso,

On Wed, Jun 07, 2023 at 03:19:24PM +0200, Tommaso Merciai wrote:

> +  alliedvision,lp2hs-delay-us:
> +    maximum: 150000
> +    description: |
> +      Low power to high speed delay time.
> +
> +      If the value is larger than 0, the camera forces a reset of all
> +      D-PHY lanes for the duration specified by this property. All lanes
> +      will transition to the low-power state and back to the high-speed
> +      state after the delay. Otherwise the lanes will transition to and
> +      remain in the high-speed state immediately after power on.
> +
> +      This is meant to help CSI-2 receivers synchronizing their D-PHY
> +      RX.

Since this new version was posted before I got a chance to reply, I
still don't think it makes sense to allow 0 & then special case it,
when testing for the presence of a property is trivial.
The property should describe some behaviour/property of the hardware,
not be a mechanism to convey what you want to write into registers.

I don't really get why you'd not do:
	If present, the camera forces a reset of all D-PHY lanes, for the
	duration specified by this property. All lanes will transition to
	the low-power state and back to the high-speed state after the
	delay.
	Otherwise the lanes will transition to and remain in the high-speed
	state immediately after power on.

> +static int alvium_get_dt_data(struct alvium_dev *alvium)
> +{
> +	struct device *dev = &alvium->i2c_client->dev;
> +	struct device_node *node = dev->of_node;
> +	struct fwnode_handle *endpoint;
> +	int ret = 0;
> +
> +	if (!node)
> +		return -EINVAL;
> +
> +	ret = fwnode_property_read_u32(dev_fwnode(dev),
> +			  "alliedvision,lp2hs-delay-us",
> +			  &alvium->lp2hs_delay);
> +	if (ret)
> +		dev_info(dev, "lp2hs-delay-us not found\n");

And this print, which I also don't understand the presence of as
well behaving driver should be quiet, goes away.

Cheers,
Conor.

> +
> +	endpoint = fwnode_graph_get_next_endpoint(dev_fwnode(dev), NULL);
> +	if (!endpoint) {
> +		dev_err(dev, "endpoint node not found\n");
> +		return -EINVAL;
> +	}
> +
> +	if (v4l2_fwnode_endpoint_alloc_parse(endpoint, &alvium->ep)) {
> +		dev_err(dev, "could not parse endpoint\n");
> +		return 0;
> +	}
> +
> +	if (alvium->ep.bus_type != V4L2_MBUS_CSI2_DPHY) {
> +		dev_err(dev, "unsupported bus type\n");
> +		return -EINVAL;
> +	}
> +
> +	if (!alvium->ep.nr_of_link_frequencies) {
> +		dev_err(dev, "no link frequencies defined");
> +		return -EINVAL;
> +	}
> +
> +	dev_info(dev, "freq: %llu\n",
> +				    alvium->ep.link_frequencies[0]);
> +	dev_info(dev, "lanes: %d\n",
> +				    alvium->ep.bus.mipi_csi2.num_data_lanes);
> +
> +	return 0;
> +}

--ld8OWH6vGquOMZTs
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iHUEABYIAB0WIQRh246EGq/8RLhDjO14tDGHoIJi0gUCZIC7YwAKCRB4tDGHoIJi
0twwAQCpR1pdYb6oE6B9OxPvbNpQ4rolh8veEmj0xSM3IbmboAD/U2uBgBIVJ9n/
SuqJRIhyCNXNwYYHFbO/atVV1TSjJAo=
=hjLR
-----END PGP SIGNATURE-----

--ld8OWH6vGquOMZTs--