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