Hi Alex,
With all the changes you've described below:
Reviewed-by: Kieran Bingham <kieran.bingham+renesas@xxxxxxxxxxxxxxxx>
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
