Re: [PATCH v8 0/2] Imagination Technologies PWM support

[Thierry Reding <thierry.reding-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]

[-- Attachment #1: Type: text/plain, Size: 3941 bytes --]

On Fri, Jan 30, 2015 at 08:01:00AM -0300, Ezequiel Garcia wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA256
> 
> Hi Thierry,
> 
> On 01/30/2015 06:18 AM, Thierry Reding wrote:
> > On Thu, Jan 29, 2015 at 01:32:09PM -0300, Ezequiel Garcia wrote:
> >> On 01/20/2015 07:52 AM, Ezequiel Garcia wrote:
> >>> 
> >>> 
> >>> On 01/09/2015 02:54 PM, Ezequiel Garcia wrote:
> >>>> A new round for the IMG PWM driver.
> >>>> 
> >>>> The IMG PWM controller is muxed with a PDM controller,
> >>>> through a shared so-called periph register bit, which sets
> >>>> the output as PWM or PDM. Because this register is not part
> >>>> of the pin controller block, but rather PWM/PDM specific, and
> >>>> because the register is also used to set the PDM value, it is
> >>>> simpler to use a regmap-based syscon to deal with it.
> >>>> 
> >>>> This time, I'm removing the PDM driver from the submission
> >>>> and submitting the PWM alone. The PDM was written as a misc
> >>>> driver, but had some design issues, so for now I'm proposing
> >>>> to merge the PWM only.
> >>>> 
> >>>> The series is based on v3.19-rc3. If at all possible I'd like
> >>>> to see this merged for v3.20.
> >>>> 
> >>> 
> >>> Thierry,
> >>> 
> >>> Any comments on this? Any chance we merge it in time for
> >>> v3.20? It's -rc5 already and I've started to worry.
> >>> 
> >> 
> >> Thierry,
> >> 
> >> I'm very sorry to be so bothering, but I got no news from you and
> >> it's -rc6 already. I'd say I'll resend this series to Andrew
> >> Morton, hoping we can merge this for v3.20.
> >> 
> >> Please let me know if you'll be able to pick this.
> > 
> > I can pick it up if you make up your mind about the license. The
> > header comment says GPL v2 or later, but MODULE_LICENSE has "GPL
> > v2", which does not include the "or later" part.
> > 
> 
> License should be GPL v2, sorry about that. Need me to fix it and
> resend or can you amend it before pushing this?

I've fixed it up.

> > Also you're making it especially difficult to build-test by not 
> > providing even the basic bits of your SoC support first. All even 
> > linux-next seems to have for the Pistachio SoC is the addition of
> > a compatible string to the dw-mmc driver.
> > 
> 
> Well, we've added COMPILE_TEST for precisely this reason, so you only
> need to select COMPILE_TEST on any arch and you'll be able to build
> test the driver.

I'm not too big a fan of COMPILE_TEST. Sometimes build errors happen
only on one architecture, so if I build test your driver for ARM I may
miss issues and people will later complain to me that my tree broke
their build. So I prefer building on the target architecture if at all
possible.

> > I'll take the PWM driver, but I'll assume that you'll eventually
> > have more pieces available, in which case I'd appreciate a note so
> > I can update my build scripts.
> > 
> 
> If you can pick it, it would be great. I understand it's hard to
> accept patches for drivers, when there's little testing possibilities.
> But on the other side, isn't it a positive thing that a vendor is
> pushing drivers this early?

I wonder what's keeping the basic SoC support from being merged. As it
is the driver is completely unused, so I'm merging dead code. I'm going
to trust Andrew when he says that he plans on merging the SoC bits soon
so it's okay for now.

The problem with merging drivers first is that you have no possibility
of testing them in an upstream environment. You can't even test them
yourself even if you have local patches to make things actually boot.
I've seen that lead to unnecessary churn later on.

Yes, vendors contributing early is a good thing, but submitting code
that can't be tested in an upstream environment isn't very useful. You
can never be entirely certain that it's going to work before you have
the basic SoC support merged first.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]