linux-kernel - Re: [PATCH v4 2/3] media: dt-bindings: alvium: add document YAML binding

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ZIDfPigYN5CgW65L@tom-HP-ZBook-Fury-15-G7-Mobile-Workstation>
Date:   Wed, 7 Jun 2023 21:49:18 +0200
From:   Tommaso Merciai <tomm.merciai@...il.com>
To:     Conor Dooley <conor@...nel.org>
Cc:     jacopo.mondi@...asonboard.com, laurent.pinchart@...asonboard.com,
        martin.hecht@...et.eu, michael.roeder@...et.eu,
        linuxfancy@...glegroups.com,
        Mauro Carvalho Chehab <mchehab@...nel.org>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Conor Dooley <conor+dt@...nel.org>,
        Liam Girdwood <lgirdwood@...il.com>,
        Mark Brown <broonie@...nel.org>,
        Sakari Ailus <sakari.ailus@...ux.intel.com>,
        Hans Verkuil <hverkuil@...all.nl>,
        Marco Felsch <m.felsch@...gutronix.de>,
        Gerald Loacker <gerald.loacker@...fvision.net>,
        Mikhail Rudenko <mike.rudenko@...il.com>,
        Nicholas Roth <nicholas@...hemail.net>,
        Linus Walleij <linus.walleij@...aro.org>,
        Michael Riesch <michael.riesch@...fvision.net>,
        Benjamin Mugnier <benjamin.mugnier@...s.st.com>,
        Krzysztof Hałasa <khalasa@...p.pl>,
        Shawn Tu <shawnx.tu@...el.com>, linux-media@...r.kernel.org,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4 2/3] media: dt-bindings: alvium: add document YAML
 binding

Hi Conor,

On Wed, Jun 07, 2023 at 06:16:19PM +0100, Conor Dooley wrote:
> 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.

My bad, sry.
My keyboard is too quick :P

> 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.
> 

You are suggesting the following solution:

minimum: 1
maximum: 150000

Right?

Personally I prefer to keep also 0 but never mind
is ok also this solution. :)

Let me know if I have understood correctly pls.
Thanks in advance :)

> > +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.

Then you are suggesting to drop this print right?

Thanks for your review!

Regards,
Tommaso

> 
> 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;
> > +}