LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Hans Verkuil <hverkuil@xs4all.nl>
To: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: Pablo Anton <pablo.anton@vodalys-labs.com>,
	hans.verkuil@cisco.com, linux-media@vger.kernel.org,
	linux-kernel@vger.kernel.org, mchehab@osg.samsung.com,
	lars@metafoo.de,
	Jean-Michel Hautbois <jean-michel.hautbois@vodalys.com>
Subject: Re: [PATCH] media: i2c: ADV7604: Rename adv7604 prefixes.
Date: Wed, 04 Feb 2015 12:35:34 +0100	[thread overview]
Message-ID: <54D20406.9000300@xs4all.nl> (raw)
In-Reply-To: <9008824.dHMd7MRA5e@avalon>

On 02/04/15 12:27, Laurent Pinchart wrote:
> Hi Hans,
> 
> Thank you for the patch.
> 
> 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@vodalys-labs.com>
>>> Signed-off-by: Jean-Michel Hautbois <jean-michel.hautbois@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.anton@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.anton@vodalys-labs.com>
>> Signed-off-by: Jean-Michel Hautbois <jean-michel.hautbois@vodalys.com>
>> [hans.verkuil@cisco.com: rebased and renamed ADV76xx_fsc to ADV76XX_FSC]
>> [hans.verkuil@cisco.com: kept the existing adv7604 driver name]
>> Signed-off-by: Hans Verkuil <hans.verkuil@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.

Regards,

	Hans

  reply	other threads:[~2015-02-04 11:35 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2015-02-03 17:13 Pablo Anton
2015-02-04  9:55 ` Hans Verkuil
2015-02-04 10:03   ` Jean-Michel Hautbois
2015-02-04 11:27   ` Laurent Pinchart
2015-02-04 11:35     ` Hans Verkuil [this message]
2015-02-04 14:17       ` Laurent Pinchart

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=54D20406.9000300@xs4all.nl \
    --to=hverkuil@xs4all.nl \
    --cc=hans.verkuil@cisco.com \
    --cc=jean-michel.hautbois@vodalys.com \
    --cc=lars@metafoo.de \
    --cc=laurent.pinchart@ideasonboard.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-media@vger.kernel.org \
    --cc=mchehab@osg.samsung.com \
    --cc=pablo.anton@vodalys-labs.com \
    --subject='Re: [PATCH] media: i2c: ADV7604: Rename adv7604 prefixes.' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).