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

From: Kieran Bingham <kieran.bingham@ideasonboard.com>
To: Alex Riesen <alexander.riesen@cetitec.com>,
	Geert Uytterhoeven <geert@linux-m68k.org>,
	Mauro Carvalho Chehab <mchehab@kernel.org>,
	Hans Verkuil <hverkuil-cisco@xs4all.nl>,
	Laurent Pinchart <laurent.pinchart@ideasonboard.com>,
	Rob Herring <robh+dt@kernel.org>,
	Mark Rutland <mark.rutland@arm.com>,
	Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>,
	devel@driverdev.osuosl.org, linux-media@vger.kernel.org,
	linux-kernel@vger.kernel.org, devicetree@vger.kernel.org,
	linux-renesas-soc@vger.kernel.org
Subject: Re: [PATCH v5 4/9] media: adv748x: add definitions for audio output related registers
Date: Tue, 7 Apr 2020 19:44:04 +0100	[thread overview]
Message-ID: <9bdf0c48-ca1c-addc-aca4-5f1889d0ae93@ideasonboard.com> (raw)
In-Reply-To: <20200407171327.GA4711@pflmari>

Hi Alex,

With all the changes you've described below:

Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>

On 07/04/2020 18:13, Alex Riesen wrote:
> 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).

I'm fine describing those extra BIT()s correctly.

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

-- 
Regards
--
Kieran