Re: [PATCH] media: i2c: ADV7604: Rename adv7604 prefixes.

[Laurent Pinchart]

Hi Hans,

On Wednesday 04 February 2015 12:35:34 Hans Verkuil wrote:
> On 02/04/15 12:27, Laurent Pinchart wrote:
> > On Wednesday 04 February 2015 10:55:21 Hans Verkuil wrote:
> >> On 02/03/15 18:13, Pablo Anton wrote:
> >>> It is confusing which parts of the driver are adv7604 specific, adv7611
> >>> specific or common for both. This patch renames any adv7604 prefixes
> >>> (both for functions and defines) to adv76xx whenever they are common.
> >>> 
> >>> Signed-off-by: Pablo Anton <pablo.anton@xxxxxxxxxxxxxxxx>
> >>> Signed-off-by: Jean-Michel Hautbois <jean-michel.hautbois@xxxxxxxxxxx>
> >> 
> >> I'm happy with this, except for three small changes:
> >> 
> >> - I had to rebase
> >> - ADV76xx_fsc should be ADV76XX_FSC
> >> - The driver name should stay the same to keep in sync with the module
> >> name. Besides, we might have a future driver for the adv7622/3, so
> >> adv76xx as the driver name is potentially confusing.
> >> 
> >> I've applied these changes and the updated patch is below. If possible I
> >> would like to get this in 3.20 so future patches for 3.21 can all be
> >> based on these renamed functions/defines.
> >> 
> >> Acks from Lars and Laurent would be welcome, though.
> >> 
> >> Regards,
> >> 
> >> 	Hans
> >> 
> >> From bff6f026de4fe276f99be6ca38206720659938dc Mon Sep 17 00:00:00 2001
> >> From: Pablo Anton <pablo.anton@xxxxxxxxxxxxxxxx>
> >> Date: Tue, 3 Feb 2015 18:13:18 +0100
> >> Subject: [PATCH] media: i2c: ADV7604: Rename adv7604 prefixes.
> >> 
> >> It is confusing which parts of the driver are adv7604 specific, adv7611
> >> specific or common for both. This patch renames any adv7604 prefixes
> >> (both for functions and defines) to adv76xx whenever they are common.
> >> 
> >> Signed-off-by: Pablo Anton <pablo.anton@xxxxxxxxxxxxxxxx>
> >> Signed-off-by: Jean-Michel Hautbois <jean-michel.hautbois@xxxxxxxxxxx>
> >> [hans.verkuil@xxxxxxxxx: rebased and renamed ADV76xx_fsc to ADV76XX_FSC]
> >> [hans.verkuil@xxxxxxxxx: kept the existing adv7604 driver name]
> >> Signed-off-by: Hans Verkuil <hans.verkuil@xxxxxxxxx>
> >> ---
> >> 
> >>  drivers/media/i2c/adv7604.c | 898  ++++++++++++++++++-------------------
> >>  include/media/adv7604.h     |  83 ++--
> >>  2 files changed, 491 insertions(+), 490 deletions(-)
> > 
> > [snip]
> > 
> >> diff --git a/include/media/adv7604.h b/include/media/adv7604.h
> >> index aa1c447..9ecf353 100644
> >> --- a/include/media/adv7604.h
> >> +++ b/include/media/adv7604.h
> >> @@ -47,16 +47,16 @@ enum adv7604_bus_order {
> > 
> > [snip]
> > 
> >> -enum adv7604_page {
> >> -	ADV7604_PAGE_IO,
> >> +enum adv76xx_page {
> >> +	ADV76XX_PAGE_IO,
> >>  	ADV7604_PAGE_AVLINK,
> >> -	ADV7604_PAGE_CEC,
> >> -	ADV7604_PAGE_INFOFRAME,
> >> +	ADV76XX_PAGE_CEC,
> >> +	ADV76XX_PAGE_INFOFRAME,
> >>  	ADV7604_PAGE_ESDP,
> >>  	ADV7604_PAGE_DPP,
> >> -	ADV7604_PAGE_AFE,
> >> -	ADV7604_PAGE_REP,
> >> -	ADV7604_PAGE_EDID,
> >> -	ADV7604_PAGE_HDMI,
> >> -	ADV7604_PAGE_TEST,
> >> -	ADV7604_PAGE_CP,
> >> +	ADV76XX_PAGE_AFE,
> >> +	ADV76XX_PAGE_REP,
> >> +	ADV76XX_PAGE_EDID,
> >> +	ADV76XX_PAGE_HDMI,
> >> +	ADV76XX_PAGE_TEST,
> >> +	ADV76XX_PAGE_CP,
> >>  	ADV7604_PAGE_VDP,
> >> -	ADV7604_PAGE_MAX,
> >> +	ADV76XX_PAGE_MAX,
> >>  };
> > 
> > (Taking the above chunk as one particular example, the comment applies to
> > the rest of the driver.)
> > 
> > I'm fine with the change in general, but I wonder how we will handle it
> > going forward. Here the ADV7604-specific pages keep their ADV7604_
> > prefix, while the pages common to all supported chips now use an ADV76XX_
> > prefix. If a new chip comes out tomorrow with support, let's say, for
> > AVLINK, how will you name ADV7604_PAGE_AVLINK ? Renaming it to
> > ADV76XX_PAGE_AVLINK would imply that it's supported on all chips, which
> > wouldn't be true, and keeping the existing name would imply that it's
> > only supported on the ADV7604, which wouldn't be true either.
> 
> I'd probably choose something like: ADV7604_12_PAGE_AVLINK if this was
> supported for e.g. the ADV7604 and ADV7612, but not ADV7611.
> 
> More likely would be scenarios where registers are supported for the adv761x
> but not for the adv7604, and in that case it would be ADV761X of course.

I guess it will be a wait-and-see kind of situation. I'm fine with the patch 
if you believe it improves readability.

-- 
Regards,

Laurent Pinchart

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/