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.an...@vodalys-labs.com>
> >>> Signed-off-by: Jean-Michel Hautbois <jean-michel.hautb...@vodalys.com>
> >>
> >> 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.an...@vodalys-labs.com>
> >> 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.an...@vodalys-labs.com>
> >> Signed-off-by: Jean-Michel Hautbois <jean-michel.hautb...@vodalys.com>
> >> [hans.verk...@cisco.com: rebased and renamed ADV76xx_fsc to ADV76XX_FSC]
> >> [hans.verk...@cisco.com: kept the existing adv7604 driver name]
> >> Signed-off-by: Hans Verkuil <hans.verk...@cisco.com>
> >> ---
> >>
> >> 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-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
