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

[Alex Riesen <alexander.riesen@cetitec.com>]

Hi Kieran,

Kieran Bingham, Tue, Apr 07, 2020 18:21:00 +0200:
> On 02/04/2020 19:34, Alex Riesen wrote:
> > 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?

I wish I knew... I afraid this is a left-over from the early development
attempts. It is obviously a mask of some bits. I don't even use the _CONTROLS1
anymore.

Removed the #define.

> 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)

I would prefer _BIT_3, if only to stay as opaque as the documentation.

> (Unless you have documentation that better describes it?)

Mine matches what you described above.

Do you mind if I describe the other bits of the register even though the
driver does not use them? Just for completeness sake (and while I still have
access to the documentation).

> > @@ -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

It was derived from the map and prefix of its bit-fields: i2soutmode and
i2sbitwidth. I too felt the name hdmi_register_03h lacking of depth.

> > +#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)

No problem. Reformatted with two spaces.

> > +#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?

Alignment corrected.

> 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

Well, I see no reason for them to stand out like this, so perhaps I better add
the prefix. I didn't add the prefix initially because they weren't names of
fields or registers, but names of values of a field (i2soutmode of that
hdmi_register_03h).
But I see there is a precedent for such already:
ADV748X_CP_{CON,SAT,BRI}_{MIN,DEF,MAX}, so prefix is okay.

> > @@ -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?

Done.

> > +#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...

Yes, please. Besides, they aren't even obviously related to the audio mute
speed.

And I corrected the alignment.

> > +#define ADV748X_HDMI_REG_6D		0x6d	/* hdmi_reg_6d */
> > +#define ADV748X_I2S_TDM_MODE_ENABLE BIT(7)

Alignment corrected.

Regards,
Alex