Hi Ricardo, On Wed, May 13, 2020 at 01:09:57PM +0200, Ricardo Cañuelo wrote: > On mié 06-05-2020 18:53:20, Laurent Pinchart wrote: > > I didn't if I remember correctly, I just mapped it to the hardware > > features. The hardware register indeed takes a value between 0 and 7, > > and that is mapped to [-4,3] x t(STEP). I don't mind either option. > > I was taking a look at the ti-tfp410.c driver to see if it needs any > changes to support the updated deskew property ranges [0-7], but I don't > fully understand what this does (line 276): > > /* Get the setup and hold time from vendor-specific properties. */ > of_property_read_u32(dvi->dev->of_node, "ti,deskew", (u32 *)&deskew); > if (deskew < -4 || deskew > 3) > return -EINVAL; > > timings->setup_time_ps = min(0, 1200 - 350 * deskew); > timings->hold_time_ps = min(0, 1300 + 350 * deskew); > > It looks like that the driver doesn't really apply the deskew settings > to the device and that this has not been really tested, so it's probably > not a big deal. The driver doesn't apply any setting to the device :-) The ti,deskew property is meant to report the deskew settings selected by the chip's configuration pins, not to set a value to be programmed to the device. > I guess what you wanted to do was to adjust the setup and hold times > around 1200 and 1300 ps respectively in increments/decrements of 350 ps > depending on the deskew value, as the datasheet describes. But this code > would set timings->setup_time_ps to 0 regardless of the deskew value, > and timings->hold_time_ps would be either 0 or a very big integer value > if deskew is -4 (both setup_time_ps and hold_time_ps are u32). > > Am I missing something? Was this intentional? Oops. That's embarassing... It should clearly be a max(), not a min(). And only for hold_time_ps is this required. Would you like to send a patch, or should I do so ? -- Regards, Laurent Pinchart
