Re: [PATCH] General fix for Palm27x aSoC driver

From: Eric Miao <eric.y.miao@gmail.com>
To: Marek Vasut <marek.vasut@gmail.com>
Cc: alsa-devel@alsa-project.org,
	Russell King - ARM Linux <linux@arm.linux.org.uk>,
	Mark Brown <broonie@sirena.org.uk>,
	linux-arm-kernel@lists.arm.linux.org.uk
Subject: Re: [PATCH] General fix for Palm27x aSoC driver
Date: Wed, 15 Apr 2009 13:11:59 +0800	[thread overview]
Message-ID: <f17812d70904142211peb61ea9yd42e7b3375874a39@mail.gmail.com> (raw)
In-Reply-To: <200904150700.14772.marek.vasut@gmail.com>

On Wed, Apr 15, 2009 at 1:00 PM, Marek Vasut <marek.vasut@gmail.com> wrote:
> On Wednesday 15 of April 2009 04:23:49 Eric Miao wrote:
>> On Wed, Apr 15, 2009 at 5:42 AM, Marek Vasut <marek.vasut@gmail.com> wrote:
>> > On Tuesday 14 of April 2009 21:50:36 Mark Brown wrote:
>> >> On Sun, Apr 12, 2009 at 08:51:19PM +0200, Marek Vasut wrote:
>> >> > +static struct pxa2xx_ac97_platform_data palmld_ac97_pdata = {
>> >> > +   .reset_gpio     = 95,
>> >> > +};
>> >>
>> >> The type of this will need changing to reflect the patch that got merged
>> >> for this but other than that minor point this approach is fine.
>> >
>> > OK, shall I change it and resend (ps. to what if you dont mind telling
>> > me?) ? Also, do you want to push it through also tree or ARM tree ? I'm
>> > for the second option as it's more of a bugfix suitable for that tree.
>>
>> Sorry, late on this. The changes to the platform part look OK to me,
>>
>> and some minor things you may have another look:
>> > -static int __init palm27x_asoc_init(void)
>> > +static int palm27x_asoc_probe(struct platform_device *pdev)
>>
>> __devinit
>
> Thanks, true, will revise it later today and resend ...
>>
>> >  {
>> >     int ret;
>> >
>> > @@ -208,6 +208,10 @@ static int __init palm27x_asoc_init(void)
>> >             machine_is_palmld()))
>> >             return -ENODEV;
>> >
>> > +   if (pdev->dev.platform_data)
>> > +           palm27x_ep_gpio = ((struct palm27x_asoc_info *)
>> > +                   (pdev->dev.platform_data))->jack_gpio;
>> > +
>>
>> This is not so readable, I'd prefer to introduce a variable for the
>> 'struct palm27x_asoc_info *' pointer.
>
> Come on, we are not doing the kernel only for ub...u so this should be OK for
> everyone who can code in C. :-)
>>

OK, I have to admit I'm sometimes a bit nitpicking. But when it comes
to the time that your platform_data introduces more members other
than jack_gpio, you may remember this thread :-)

Anyway, it's up to you. I don't want myself to be too captious.

>> >     ret = gpio_request(palm27x_ep_gpio, "Headphone Jack");
>> >     if (ret)
>> >             return ret;
>
>
>

-- 
Cheers
- eric
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel