Re: [PATCH v2 1/4] drm/fl2000: Add USB driver with register and I2C access

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] drm/fl2000: unchecked regmap writes in int register helpers
--

commit 1083af596ecfae2d3d352f9d352bcede51d3408e
Author: Angel Gabriel Crispin Valdivia <angelcrispinvaldivia@gmail.com>

drm/fl2000: Add USB driver with register and I2C access

This commit adds the initial USB driver for Fresco Logic FL2000 display
adapters, including regmap-backed register helpers and an I2C adapter.

> diff --git a/drivers/gpu/drm/fl2000/fl2000_registers.c b/drivers/gpu/drm/fl2000/fl2000_registers.c
> new file mode 100644
> index 0000000000000..f1cd28434a520
> --- /dev/null
> +++ b/drivers/gpu/drm/fl2000/fl2000_registers.c

[ ... ]

> +int fl2000_set_pll(struct regmap *regmap, struct fl2000_pll *pll)
> +{
> +	u32 val;
> +
> +	val = FIELD_PREP(FL2000_VGA_PLL_PRESCALER, pll->prescaler) |
> +	      FIELD_PREP(FL2000_VGA_PLL_MULTIPLIER, pll->multiplier) |
> +	      FIELD_PREP(FL2000_VGA_PLL_DIVISOR, pll->divisor) |
> +	      FIELD_PREP(FL2000_VGA_PLL_FUNCTION, pll->function);
> +	regmap_write(regmap, FL2000_VGA_PLL_REG, val);
> +
> +	val = FL2000_VGA_CTRL_ACLK_FORCE_PLL_UP;
> +	regmap_write_bits(regmap, FL2000_VGA_CTRL_REG_ACLK, val, val);
> +
> +	return 0;
> +}

[ ... ]

> +int fl2000_set_timings(struct regmap *regmap, struct fl2000_timings *timings)
> +{
> +	regmap_write(regmap, FL2000_VGA_HSYNC_REG1,
> +		     FIELD_PREP(FL2000_VGA_HSYNC1_HACTIVE, timings->hactive) |
> +		     FIELD_PREP(FL2000_VGA_HSYNC1_HTOTAL, timings->htotal));
> +
> +	regmap_write(regmap, FL2000_VGA_HSYNC_REG2,
> +		     FIELD_PREP(FL2000_VGA_HSYNC2_HSYNC_WIDTH, timings->hsync_width) |
> +		     FIELD_PREP(FL2000_VGA_HSYNC2_HSTART, timings->hstart));
> +
> +	regmap_write(regmap, FL2000_VGA_VSYNC_REG1,
> +		     FIELD_PREP(FL2000_VGA_VSYNC1_VACTIVE, timings->vactive) |
> +		     FIELD_PREP(FL2000_VGA_VSYNC1_VTOTAL, timings->vtotal));
> +
> +	regmap_write(regmap, FL2000_VGA_VSYNC_REG2,
> +		     FIELD_PREP(FL2000_VGA_VSYNC2_VSYNC_WIDTH, timings->vsync_width) |
> +		     FIELD_PREP(FL2000_VGA_VSYNC2_VSTART, timings->vstart) |
> +		     FIELD_PREP(FL2000_VGA_VSYNC2_START_LATENCY, timings->vstart));
> +
> +	return 0;
> +}
> +
> +int fl2000_set_pixfmt(struct regmap *regmap, u32 bytes_pix)
> +{
> +	u32 mask, val;
> +
> +	mask = FL2000_VGA_CTRL_PXCLK_DAC_OUTPUT_EN | FL2000_VGA_CTRL_PXCLK_DROP_CNT |
> +	       FL2000_VGA_CTRL_PXCLK_VGA565_MODE | FL2000_VGA_CTRL_PXCLK_VGA332_MODE |
> +	       FL2000_VGA_CTRL_PXCLK_VGA555_MODE | FL2000_VGA_CTRL_PXCLK_VGA_COMPRESS |
> +	       FL2000_VGA_CTRL_PXCLK_CLEAR_WATERMARK;
> +	val = FL2000_VGA_CTRL_PXCLK_DAC_OUTPUT_EN | FL2000_VGA_CTRL_PXCLK_CLEAR_WATERMARK;
> +	if (bytes_pix == 2)
> +		val |= FL2000_VGA_CTRL_PXCLK_VGA565_MODE;
> +	regmap_write_bits(regmap, FL2000_VGA_CTRL_REG_PXCLK, mask, val);
> +
> +	return 0;
> +}
> +
> +int fl2000_set_transfers(struct regmap *regmap)
> +{
> +	u32 mask;
> +
> +	mask = FL2000_VGA_CTRL_ACLK_USE_PKT_PENDING | FL2000_VGA_CTRL_ACLK_USE_ZERO_TD |
> +	       FL2000_VGA_CTRL_ACLK_USE_ZERO_PKT_LEN;
> +	regmap_write_bits(regmap, FL2000_VGA_CTRL_REG_ACLK, mask,
> +			  FL2000_VGA_CTRL_ACLK_USE_ZERO_PKT_LEN);
> +
> +	regmap_write_bits(regmap, FL2000_VGA_ISOCH_REG, FL2000_VGA_ISOCH_MFRAME_CNT, 0);
> +
> +	return 0;
> +}
> +
> +int fl2000_reset(struct regmap *regmap)
> +{
> +	regmap_write_bits(regmap, FL2000_RST_CTRL_REG, FL2000_RST_CTRL_SW_RESET,
> +			  FL2000_RST_CTRL_SW_RESET);
> +
> +	msleep(FL2000_HW_RST_MDELAY);
> +
> +	return 0;
> +}
> +
> +int fl2000_afe_magic(struct regmap *regmap)
> +{
> +	regmap_write_bits(regmap, FL2000_USB_LPM_REG, FL2000_USB_LPM_MAGIC, FL2000_USB_LPM_MAGIC);
> +
> +	return 0;
> +}
> +
> +int fl2000_usb_magic(struct regmap *regmap)
> +{
> +	u32 val;
> +
> +	/* Writing DONE as 0 would start an I2C operation, see fl2000_i2c_dword() */
> +	val = FL2000_VGA_I2C_SC_MONITOR_DETECT | FL2000_VGA_I2C_SC_EDID_DETECT |
> +	      FL2000_VGA_I2C_SC_DONE;
> +	regmap_write_bits(regmap, FL2000_VGA_I2C_SC_REG, val, val);
> +
> +	regmap_write_bits(regmap, FL2000_VGA_CTRL_REG_3, FL2000_VGA_CTRL3_WAKEUP_CLR_EN, 0);
> +
> +	val = FL2000_USB_LPM_U1_REJECT | FL2000_USB_LPM_U2_REJECT;
> +	regmap_write_bits(regmap, FL2000_USB_LPM_REG, val, val);
> +
> +	regmap_write_bits(regmap, FL2000_USB_CTRL_REG, FL2000_USB_CTRL_WAKE_NRDY, 0);
> +
> +	return 0;
> +}
> +
> +/* The interrupts about monitors: they matter from the moment the device is up, mode or no mode */
> +int fl2000_enable_sink_interrupts(struct regmap *regmap)
> +{
> +	regmap_write_bits(regmap, FL2000_VGA_CTRL_REG_ACLK, FL2000_VGA_CTRL_ACLK_EDID_MON_INT_EN,
> +			  FL2000_VGA_CTRL_ACLK_EDID_MON_INT_EN);
> +
> +	regmap_write_bits(regmap, FL2000_VGA_CTRL2_REG_ACLK, FL2000_VGA_CTRL2_ACLK_HDMI_INT_EN,
> +			  FL2000_VGA_CTRL2_ACLK_HDMI_INT_EN);
> +
> +	return 0;
> +}

[Severity: Medium]
Should these int-returning register helpers check and propagate errors from
regmap_write() and regmap_write_bits(), like fl2000_force_connect() and
fl2000_stream_interrupts() do?

Each regmap access runs a synchronous USB control transfer via
usb_control_msg_recv() or usb_control_msg_send().  When a transfer fails or
times out, these helpers still return 0 and continue issuing further USB
control transfers, hiding initialization or modeset failures from callers.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261111-fl2000-upstream-v2-0-201f87708cb0@gmail.com?part=1