Re: [PATCH v8 1/4] drm/bridge: add support for sn65dsi86 bridge driver

[spanda-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org]

On 2018-06-06 04:29, Sean Paul wrote:
> On Tue, Jun 05, 2018 at 11:10:15AM +0530, Sandeep Panda wrote:
>> Add support for TI's sn65dsi86 dsi2edp bridge chip.
>> The chip converts DSI transmitted signal to eDP signal,
>> which is fed to the connected eDP panel.
>> 
>> This chip can be controlled via either i2c interface or
>> dsi interface. Currently in driver all the control registers
>> are being accessed through i2c interface only.
>> Also as of now HPD support has not been added to bridge
>> chip driver.
>> 
>> Changes in v1:
>>  - Split the dt-bindings and the driver support into separate patches
>>    (Andrzej Hajda).
>>  - Use of gpiod APIs to parse and configure gpios instead of obsolete 
>> ones
>>    (Andrzej Hajda).
>>  - Use macros to define the register offsets (Andrzej Hajda).
>> 
>> Changes in v2:
>>  - Separate out edp panel specific HW resource handling from bridge
>>    driver and create a separate edp panel drivers to handle panel
>>    specific mode information and HW resources (Sean Paul).
>>  - Replace pr_* APIs to DRM_* APIs to log error or debug information
>>    (Sean Paul).
>>  - Remove some of the unnecessary structure/variable from driver (Sean
>>    Paul).
>>  - Rename the function and structure prefix "sn65dsi86" to 
>> "ti_sn_bridge"
>>    (Sean Paul / Rob Herring).
>>  - Remove most of the hard-coding and modified the bridge init 
>> sequence
>>    based on current mode (Sean Paul).
>>  - Remove the existing function to retrieve the EDID data and
>>    implemented this as an i2c_adapter and use drm_get_edid() (Sean 
>> Paul).
>>  - Remove the dummy irq handler implementation, will add back the
>>    proper irq handling later (Sean Paul).
>>  - Capture the required enable gpios in a single array based on dt 
>> entry
>>    instead of having individual descriptor for each gpio (Sean Paul).
>> 
>> Changes in v3:
>>  - Remove usage of irq_gpio and replace it as "interrupts" property 
>> (Rob
>>    Herring).
>>  - Remove the unnecessary header file inclusions (Sean Paul).
>>  - Rearrange the header files in alphabetical order (Sean Paul).
>>  - Use regmap interface to perform i2c transactions.
>>  - Update Copyright/License field and address other review comments
>>    (Jordan Crouse).
>> 
>> Changes in v4:
>>  - Update License/Copyright (Sean Paul).
>>  - Add Kconfig and Makefile changes (Sean Paul).
>>  - Drop i2c gpio handling from this bridge driver, since i2c sda/scl 
>> gpios
>>    will be handled by i2c master.
>>  - Update required supplies names.
>>  - Remove unnecessary goto statements (Sean Paul).
>>  - Add mutex lock to power_ctrl API to avoid race conditions (Sean
>>    Paul).
>>  - Add support to parse reference clk frequency from dt(optional).
>>  - Update the bridge chip enable/disable sequence.
>> 
>> Changes in v5:
>>  - Fixed Kbuild test service reported warnings.
>> 
>> Changes in v6:
>>  - Use PM runtime based ref-counting instead of local ref_count 
>> mechanism
>>    (Stephen Boyd).
>>  - Clean up some debug logs and indentations (Sean Paul).
>>  - Simplify dp rate calculation (Sean Paul).
>>  - Add support to configure refclk based on input REFCLK pin or DACP/N
>>    pin (Stephen Boyd).
>> 
>> Changes in v7:
>>  - Use static supply entries instead of dynamic allocation (Andrzej
>>    Hajda).
>>  - Defer bridge driver probe if panel is not probed (Andrzej Hajda).
>>  - Update of_graph APIs for correct node reference management. 
>> (Andrzej
>>    Hajda).
>>  - Remove local display_mode object (Andrzej Hajda).
>>  - Remove version id check function from driver.
>> 
>> Changes in v8:
>>  - Move dsi register/attach function to bridge driver probe (Andrzej
>>    Hajda).
>>  - Introduce a new helper function to write 16bit words into 
>> consecutive
>>    registers (Andrzej Hajda).
>>  - Remove unnecessary macros (Andrzej Hajda).
>> 
>> Signed-off-by: Sandeep Panda <spanda@codeaurora.org>
>> ---
>>  drivers/gpu/drm/bridge/Kconfig        |   9 +
>>  drivers/gpu/drm/bridge/Makefile       |   1 +
>>  drivers/gpu/drm/bridge/ti-sn65dsi86.c | 666 
>> ++++++++++++++++++++++++++++++++++
>>  3 files changed, 676 insertions(+)
>>  create mode 100644 drivers/gpu/drm/bridge/ti-sn65dsi86.c
>> 
>> diff --git a/drivers/gpu/drm/bridge/Kconfig 
>> b/drivers/gpu/drm/bridge/Kconfig
>> index 3b99d5a..8153150 100644
>> --- a/drivers/gpu/drm/bridge/Kconfig
>> +++ b/drivers/gpu/drm/bridge/Kconfig
>> @@ -108,6 +108,15 @@ config DRM_TI_TFP410
>>  	---help---
>>  	  Texas Instruments TFP410 DVI/HDMI Transmitter driver
>> 
>> +config DRM_TI_SN65DSI86
>> +	tristate "TI SN65DSI86 DSI to eDP bridge"
>> +	depends on OF
>> +	select DRM_KMS_HELPER
>> +	select REGMAP_I2C
>> +	select DRM_PANEL
>> +	---help---
>> +	  Texas Instruments SN65DSI86 DSI to eDP Bridge driver
>> +
>>  source "drivers/gpu/drm/bridge/analogix/Kconfig"
>> 
>>  source "drivers/gpu/drm/bridge/adv7511/Kconfig"
>> diff --git a/drivers/gpu/drm/bridge/Makefile 
>> b/drivers/gpu/drm/bridge/Makefile
>> index 373eb28..3711be8 100644
>> --- a/drivers/gpu/drm/bridge/Makefile
>> +++ b/drivers/gpu/drm/bridge/Makefile
>> @@ -12,4 +12,5 @@ obj-$(CONFIG_DRM_TOSHIBA_TC358767) += tc358767.o
>>  obj-$(CONFIG_DRM_ANALOGIX_DP) += analogix/
>>  obj-$(CONFIG_DRM_I2C_ADV7511) += adv7511/
>>  obj-$(CONFIG_DRM_TI_TFP410) += ti-tfp410.o
>> +obj-$(CONFIG_DRM_TI_SN65DSI86) += ti-sn65dsi86.o
> 
> SN65DSI86 should be in front of TFP410 here and above.
> 

Ok.

>>  obj-y += synopsys/
>> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c 
>> b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
>> new file mode 100644
>> index 0000000..add6e0f
>> --- /dev/null
>> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> 
> /snip
> 
>> +
>> +#define REFCLK_LUT_SIZE	5
>> +
>> +/* clk frequencies supported by bridge in Hz in case derived from 
>> REFCLK pin */
>> +static const u32 ti_sn_bridge_refclk_lut[] = {
>> +	12000000,
>> +	19200000,
>> +	26000000,
>> +	27000000,
>> +	38400000,
>> +};
>> +
>> +/* clk frequencies supported by bridge in Hz in case derived from 
>> DACP/N pin */
>> +static const u32 ti_sn_bridge_dsiclk_lut[] = {
>> +	468000000,
>> +	384000000,
>> +	416000000,
>> +	486000000,
>> +	460800000,
>> +};
>> +
>> +static void ti_sn_bridge_set_refclk(struct ti_sn_bridge *pdata)
>> +{
>> +	int i = 0;
>> +	u8 refclk_src;
>> +	u32 refclk_rate;
>> +	const u32 *refclk_lut;
>> +
>> +	if (pdata->refclk) {
>> +		refclk_src = DPPLL_CLK_SRC_REFCLK;
>> +		refclk_rate = clk_get_rate(pdata->refclk);
>> +		refclk_lut = ti_sn_bridge_refclk_lut;
>> +		clk_prepare_enable(pdata->refclk);
>> +	} else {
>> +		refclk_src = DPPLL_CLK_SRC_DSICLK;
>> +		refclk_rate = ti_sn_bridge_get_dsi_freq(pdata) * 1000;
>> +		refclk_lut = ti_sn_bridge_dsiclk_lut;
>> +	}
>> +
>> +	/* for i equals to REFCLK_LUT_SIZE means default frequency */
>> +	for (i = 0; i < REFCLK_LUT_SIZE; i++)
> 
> Instead of REFCLK_LUT_SIZE, use a local variable, ie:
> 
>         size_t refclk_lut_size;
> 
>         ...
> 
>         if(pdata->refclk) {
>                 ...
>                 refclk_lut = ti_sn_bridge_refclk_lut;
>                 refclk_lut_size = ARRAY_SIZE(ti_sn_bridge_refclk_lut);
>                 ...
>         } else {
>                 ...
>                 refclk_lut = ti_sn_bridge_dsiclk_lut;
>                 refclk_lut_size = ARRAY_SIZE(ti_sn_bridge_dsiclk_lut);
>         }
> 
> And remove REFCLK_LUT_SIZE.

Ok.

> 
>> +		if (refclk_lut[i] == refclk_rate)
>> +			break;
>> +
>> +	regmap_write(pdata->regmap, SN_REFCLK_FREQ_REG,
>> +		     (refclk_src | (i << SN_DSIA_REFCLK_OFFSET)));
>> +}
>> +
>> +/**
>> + * LUT index corresponds to register value and
>> + * LUT values corresponds to dp data rate supported
>> + * by the bridge in Mbps unit.
>> + */
>> +static const unsigned int ti_sn_bridge_dp_rate_lut[] = {
>> +	0, 1620, 2160, 2430, 2700, 3240, 4320, 5400
>> +};
>> +
>> +static void ti_sn_bridge_set_dsi_dp_rate(struct ti_sn_bridge *pdata)
>> +{
>> +	unsigned int bit_rate_mhz, clk_freq_mhz, dp_rate_mhz;
>> +	unsigned int val = 0, i = 0;
>> +	struct drm_display_mode *mode =
>> +		&pdata->bridge.encoder->crtc->state->adjusted_mode;
>> +
>> +	/* set DSIA clk frequency */
>> +	bit_rate_mhz = (mode->clock / 1000) *
>> +			mipi_dsi_pixel_format_to_bpp(pdata->dsi->format);
>> +	clk_freq_mhz = bit_rate_mhz / (pdata->dsi->lanes * 2);
>> +
>> +	/* for each increment in val, frequency increases by 5MHz */
>> +	val = (MIN_DSI_CLK_FREQ_MHZ / 5) +
>> +		(((clk_freq_mhz - MIN_DSI_CLK_FREQ_MHZ) / 5) & 0xFF);
>> +	regmap_write(pdata->regmap, SN_DSIA_CLK_FREQ_REG, val);
>> +
>> +	/* set DP data rate */
>> +	dp_rate_mhz = ((bit_rate_mhz / pdata->dsi->lanes) * 
>> DP_CLK_FUDGE_NUM) /
>> +							DP_CLK_FUDGE_DEN;
>> +	for (i = 0; i < ARRAY_SIZE(ti_sn_bridge_dp_rate_lut) - 1; i++)
>> +		if (ti_sn_bridge_dp_rate_lut[i] > dp_rate_mhz)
>> +			break;
>> +
>> +	regmap_update_bits(pdata->regmap, SN_DATARATE_CONFIG_REG,
>> +			   SN_DP_DATA_RATE_BITS, i << SN_DP_DATA_RATE_OFFSET);
>> +}
>> +
>> +static void ti_sn_bridge_set_video_timings(struct ti_sn_bridge 
>> *pdata)
>> +{
>> +	struct drm_display_mode *mode =
>> +		&pdata->bridge.encoder->crtc->state->adjusted_mode;
>> +
>> +	ti_sn_bridge_write_u16(pdata, SN_CHA_ACTIVE_LINE_LENGTH_LOW_REG,
>> +			       mode->hdisplay);
>> +	ti_sn_bridge_write_u16(pdata, SN_CHA_VERTICAL_DISPLAY_SIZE_LOW_REG,
>> +			       mode->vdisplay);
>> +	ti_sn_bridge_write_u16(pdata, SN_CHA_HSYNC_PULSE_WIDTH_LOW_REG,
>> +			       mode->hsync_end - mode->hsync_start);
>> +	ti_sn_bridge_write_u16(pdata, SN_CHA_VSYNC_PULSE_WIDTH_LOW_REG,
>> +			       mode->vsync_end - mode->vsync_start);
>> +
>> +	regmap_write(pdata->regmap, SN_CHA_HORIZONTAL_BACK_PORCH_REG,
>> +		     (mode->htotal - mode->hsync_end) & 0xFF);
>> +	regmap_write(pdata->regmap, SN_CHA_VERTICAL_BACK_PORCH_REG,
>> +		     (mode->vtotal - mode->vsync_end) & 0xFF);
>> +
>> +	regmap_write(pdata->regmap, SN_CHA_HORIZONTAL_FRONT_PORCH_REG,
>> +		     (mode->hsync_start - mode->hdisplay) & 0xFF);
>> +	regmap_write(pdata->regmap, SN_CHA_VERTICAL_FRONT_PORCH_REG,
>> +		     (mode->vsync_start - mode->vdisplay) & 0xFF);
>> +
>> +	usleep_range(10000, 10500); /* 10ms delay recommended by spec */
>> +}
>> +
>> +static void ti_sn_bridge_enable(struct drm_bridge *bridge)
>> +{
>> +	struct ti_sn_bridge *pdata = bridge_to_ti_sn_bridge(bridge);
>> +	unsigned int val = 0;
>> +
>> +	if (pdata->panel) {
>> +		drm_panel_prepare(pdata->panel);
>> +		/* in case drm_panel is connected then HPD is not supported */
>> +		regmap_update_bits(pdata->regmap, SN_HPD_DISABLE_REG,
>> +				   SN_HPD_DISABLE_BIT, !0);
> 
> What is !0?

There was a comment to make it like this, but i think SN_HPD_DISABLE_BIT 
makes it more readable. I will change this as it was previously.
> 
> Sean
> 
> /snip
_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno