Re: [PATCH v5 4/9] media: adv748x: add definitions for audio output related registers

[Kieran Bingham]

Hi Alex,

On 02/04/2020 19:34, Alex Riesen wrote:
> Signed-off-by: Alexander Riesen <alexander.riesen@xxxxxxxxxxx>
> ---
>  drivers/media/i2c/adv748x/adv748x.h | 32 +++++++++++++++++++++++++++++
>  1 file changed, 32 insertions(+)
> 
> diff --git a/drivers/media/i2c/adv748x/adv748x.h b/drivers/media/i2c/adv748x/adv748x.h
> index 0a9d78c2870b..1a1ea70086c6 100644
> --- a/drivers/media/i2c/adv748x/adv748x.h
> +++ b/drivers/media/i2c/adv748x/adv748x.h
> @@ -226,6 +226,11 @@ struct adv748x_state {
>  
>  #define ADV748X_IO_VID_STD		0x05
>  
> +#define ADV748X_IO_PAD_CONTROLS		0x0e
> +#define ADV748X_IO_PAD_CONTROLS_TRI_AUD	BIT(5)
> +#define ADV748X_IO_PAD_CONTROLS_PDN_AUD	BIT(1)
> +#define ADV748X_IO_PAD_CONTROLS1	0x1d

What is CONTROLS1 (1d) referenced from here?

There's no 'field' matching for this register, and the 'bits' (0, 2, 3,
4) correspond to "pdn_spi, pdn_pix, '-', tri_spi"

Perhaps we need to define those bit fields accordingly and reference
them where they get used directly?

Perhaps calling bit 3 as:
 #define ADV748X_IO_PAD_CONTROLS_BIT_3	BIT(3)

Or
 #define ADV748X_IO_PAD_CONTROLS_RESVD	BIT(3)

(Unless you have documentation that better describes it?)


> +
>  #define ADV748X_IO_10			0x10	/* io_reg_10 */
>  #define ADV748X_IO_10_CSI4_EN		BIT(7)
>  #define ADV748X_IO_10_CSI1_EN		BIT(6)
> @@ -248,7 +253,21 @@ struct adv748x_state {
>  #define ADV748X_IO_REG_FF		0xff
>  #define ADV748X_IO_REG_FF_MAIN_RESET	0xff
>  
> +/* DPLL Map */
> +#define ADV748X_DPLL_MCLK_FS		0xb5
> +#define ADV748X_DPLL_MCLK_FS_N_MASK	GENMASK(2, 0)
> +
>  /* HDMI RX Map */
> +#define ADV748X_HDMI_I2S		0x03	/* I2S mode and width */

Looks like a more appropriate name than the datasheets
"hdmi_register_03h" :-D


> +#define ADV748X_HDMI_I2SBITWIDTH_MASK	GENMASK(4, 0)
> +#define ADV748X_HDMI_I2SOUTMODE_SHIFT	5
> +#define ADV748X_HDMI_I2SOUTMODE_MASK	\
> +	GENMASK(6, ADV748X_HDMI_I2SOUTMODE_SHIFT)

I'd be very tempted to ignore the 80char limit here and put that on the
line above ... or find a way to remove the 1 character...

In fact, given the entry there - how about just leaving this as:

#define ADV748X_HDMI_I2SOUTMODE_MASK	GENMASK(6, 5)


> +#define ADV748X_I2SOUTMODE_I2S 0
> +#define ADV748X_I2SOUTMODE_RIGHT_J 1
> +#define ADV748X_I2SOUTMODE_LEFT_J 2
> +#define ADV748X_I2SOUTMODE_SPDIF 3

Can we align these value in the column with the other values?

And as much as I hate long define names, it seems a bit odd that these
suddenly lack the HDMI_ part of the define prefix...

Should we either remove the HDMI_ from
 ADV748X_HDMI_I2SBITWIDTH_MASK
 ADV748X_HDMI_I2SOUTMODE_SHIFT
 ADV748X_HDMI_I2SOUTMODE_MASK

or add it to
 ADV748X_I2SOUTMODE_I2S
 ADV748X_I2SOUTMODE_RIGHT_J
 ADV748X_I2SOUTMODE_LEFT_J
 ADV748X_I2SOUTMODE_SPDIF

?

> +
>  #define ADV748X_HDMI_LW1		0x07	/* line width_1 */
>  #define ADV748X_HDMI_LW1_VERT_FILTER	BIT(7)
>  #define ADV748X_HDMI_LW1_DE_REGEN	BIT(5)
> @@ -260,6 +279,16 @@ struct adv748x_state {
>  #define ADV748X_HDMI_F1H1		0x0b	/* field1 height_1 */
>  #define ADV748X_HDMI_F1H1_INTERLACED	BIT(5)
>  
> +#define ADV748X_HDMI_MUTE_CTRL		0x1a
> +#define ADV748X_HDMI_MUTE_CTRL_MUTE_AUDIO BIT(4)
> +#define ADV748X_HDMI_MUTE_CTRL_WAIT_UNMUTE_MASK	GENMASK(3, 1)
> +#define ADV748X_HDMI_MUTE_CTRL_NOT_AUTO_UNMUTE	BIT(0)
> +
> +#define ADV748X_HDMI_AUDIO_MUTE_SPEED	0x0f

Can we keep the register definitions in address order please?

> +#define ADV748X_HDMI_AUDIO_MUTE_SPEED_MASK	GENMASK(4, 0)
> +#define ADV748X_MAN_AUDIO_DL_BYPASS BIT(7)
> +#define ADV748X_AUDIO_DELAY_LINE_BYPASS BIT(6)

Those bits do not describe the register they are in, not sure how to
address that without exceptionally long names though.. :-(

So perhaps how you've got them might be the best option...

> +
>  #define ADV748X_HDMI_HFRONT_PORCH	0x20	/* hsync_front_porch_1 */
>  #define ADV748X_HDMI_HFRONT_PORCH_MASK	0x1fff
>  
> @@ -281,6 +310,9 @@ struct adv748x_state {
>  #define ADV748X_HDMI_TMDS_1		0x51	/* hdmi_reg_51 */
>  #define ADV748X_HDMI_TMDS_2		0x52	/* hdmi_reg_52 */
>  
> +#define ADV748X_HDMI_REG_6D		0x6d	/* hdmi_reg_6d */
> +#define ADV748X_I2S_TDM_MODE_ENABLE BIT(7)
> +
>  /* HDMI RX Repeater Map */
>  #define ADV748X_REPEATER_EDID_SZ	0x70	/* primary_edid_size */
>  #define ADV748X_REPEATER_EDID_SZ_SHIFT	4
> 

-- 
Regards
--
Kieran