[permalink] [raw]

Subject: [PATCH 02/13] staging: mt7621-pinctrl: ralink: add pinctrl driver

2018-03-14 23:46:54

On Thu, Mar 15 2018, Dan Carpenter wrote:

> This all seems fine. Generally the requirements for staging are that it
> has a TODO, someone to work on it, and it doesn't break the build. But
> some of the patches don't have commit message and those are required and
> some of the commit messages are just the changes you have made not don't
> describe the actual code...

Thanks for having a look.
It seems odd to require detailed commit messages, when we don't require
the same level of quality in the code.
Naturally when the driver is moved out of staging a properly detailed
commit message should be added, but is that needed on the way in to
staging? At this stage I don't know much more than is already there.
After I've cleaned up the code I probably will.

For patch 01/13 you asked "what kind of device this is". The subject
line makes it clear that it is a "pcie driver". What extra detail did
you want? Would it be sufficient to just copy the subject line so that
it appears twice in the commit message?

>
> John Crispin's email is [email protected].

Thanks.

NeilBrown

>
> regards,
> dan carpenter

Attachments:

signature.asc (847.00 B)

2018-03-15 11:08:59

by John Crispin

[permalink] [raw]

Subject: Re: [PATCH 00/13] staging: add drivers to support Mediatek mt7621 in gnubee-pc1

On 15/03/18 11:48, Dan Carpenter wrote:
> This all seems fine. Generally the requirements for staging are that it
> has a TODO, someone to work on it, and it doesn't break the build. But
> some of the patches don't have commit message and those are required and
> some of the commit messages are just the changes you have made not don't
> describe the actual code...
>
> John Crispin's email is [email protected].
>
> regards,
> dan carpenter
>
Hi All,

looks like i was CC'ed on the openwrt addr, which no longer exists. This
series makes no sense. None of the stuff posted is anywhere near ready
to be upstreamed.

* we dont need a dedicated pinctrl driver, pinctrl-single will work fine
on these SoCs
* the DMA/sdhci driver is a hacked up version of the SDK driver.
* drivers/net/ethernet/mediatek/* works on mt7623 and is easily portable
to mt7621, same goes for the gsw driver.

John

2018-03-15 11:27:32

by Dan Carpenter

[permalink] [raw]

Subject: Re: [PATCH 00/13] staging: add drivers to support Mediatek mt7621 in gnubee-pc1

On Thu, Mar 15, 2018 at 10:04:33PM +1100, NeilBrown wrote:
> On Thu, Mar 15 2018, Dan Carpenter wrote:
>
> > This all seems fine. Generally the requirements for staging are that it
> > has a TODO, someone to work on it, and it doesn't break the build. But
> > some of the patches don't have commit message and those are required and
> > some of the commit messages are just the changes you have made not don't
> > describe the actual code...
>
> Thanks for having a look.
> It seems odd to require detailed commit messages, when we don't require
> the same level of quality in the code.
> Naturally when the driver is moved out of staging a properly detailed
> commit message should be added, but is that needed on the way in to
> staging? At this stage I don't know much more than is already there.
> After I've cleaned up the code I probably will.
>
> For patch 01/13 you asked "what kind of device this is". The subject
> line makes it clear that it is a "pcie driver". What extra detail did
> you want? Would it be sufficient to just copy the subject line so that
> it appears twice in the commit message?
>

Ah... Sorry. It's literally a pcie driver. For some reason I thought
it was a device that ran over pcie.

We don't require a detailed changelog, but you have to put something...
Probably just restating the subject and adding that it's for the gnubee1
is fine.

regards,
dan carpenter

2018-03-15 11:32:56

by Dan Carpenter

[permalink] [raw]

Subject: Re: [PATCH 00/13] staging: add drivers to support Mediatek mt7621 in gnubee-pc1

On Thu, Mar 15, 2018 at 12:07:26PM +0100, John Crispin wrote:
> None of the stuff posted is anywhere near ready to be
> upstreamed.

Yeah. This is staging so we accept any quality...

The rest of the information is very useful. Thanks!

regards,
dan carpenter

2018-03-15 20:04:30

by NeilBrown

[permalink] [raw]

Subject: Re: [PATCH 00/13] staging: add drivers to support Mediatek mt7621 in gnubee-pc1

On Thu, Mar 15 2018, Dan Carpenter wrote:

> On Thu, Mar 15, 2018 at 10:04:33PM +1100, NeilBrown wrote:
>> On Thu, Mar 15 2018, Dan Carpenter wrote:
>>
>> > This all seems fine. Generally the requirements for staging are that it
>> > has a TODO, someone to work on it, and it doesn't break the build. But
>> > some of the patches don't have commit message and those are required and
>> > some of the commit messages are just the changes you have made not don't
>> > describe the actual code...
>>
>> Thanks for having a look.
>> It seems odd to require detailed commit messages, when we don't require
>> the same level of quality in the code.
>> Naturally when the driver is moved out of staging a properly detailed
>> commit message should be added, but is that needed on the way in to
>> staging? At this stage I don't know much more than is already there.
>> After I've cleaned up the code I probably will.
>>
>> For patch 01/13 you asked "what kind of device this is". The subject
>> line makes it clear that it is a "pcie driver". What extra detail did
>> you want? Would it be sufficient to just copy the subject line so that
>> it appears twice in the commit message?
>>
>
> Ah... Sorry. It's literally a pcie driver. For some reason I thought
> it was a device that ran over pcie.
>
> We don't require a detailed changelog, but you have to put something...
> Probably just restating the subject and adding that it's for the gnubee1
> is fine.

I'll resend sometime next week with more words. However could you
please clarify a couple of things for me?

1/ Why do you (sometimes) call the commit message a "change log". When
I see the term "change log" in the context of a patch, my first
thought is that it it means a log of changes that have been made to
the patch - typically through the review cycle. But that isn't what
you mean. This has confused me a couple of times.

2/ Why don't you consider the first line of the commit message to be
part of the commit message? Why is duplication required?
(You said "some of the patches don't have commit message[s]",
which isn't true, though some of the messages are only one line).

Maybe the requirements on the commit message (including this
duplication) could be included in the "Staging trees" section of
Documentation/process/2.process.rst.
That file only lists the TODO and "doesn't break the build"
requirements. It doesn't metion the "someone to work on it" requirement.
That might seem obvious, but it doesn't hurt to be explicit.

Thanks,
NeilBrown

Attachments:

signature.asc (847.00 B)

2018-03-15 20:13:36

On Thu, Mar 15 2018, John Crispin wrote:

> On 15/03/18 21:12, NeilBrown wrote:
>> On Thu, Mar 15 2018, John Crispin wrote:
>>
>>> On 15/03/18 11:48, Dan Carpenter wrote:
>>>> This all seems fine. Generally the requirements for staging are that it
>>>> has a TODO, someone to work on it, and it doesn't break the build. But
>>>> some of the patches don't have commit message and those are required and
>>>> some of the commit messages are just the changes you have made not don't
>>>> describe the actual code...
>>>>
>>>> John Crispin's email is [email protected].
>>>>
>>>> regards,
>>>> dan carpenter
>>>>
>>> Hi All,
>>>
>>> looks like i was CC'ed on the openwrt addr, which no longer exists. This
>>> series makes no sense. None of the stuff posted is anywhere near ready
>>> to be upstreamed.
>>>
>>> * we dont need a dedicated pinctrl driver, pinctrl-single will work fine
>>> on these SoCs
>>> * the DMA/sdhci driver is a hacked up version of the SDK driver.
>>> * drivers/net/ethernet/mediatek/* works on mt7623 and is easily portable
>>> to mt7621, same goes for the gsw driver.
>> Hi John,
>> I think it makes sense in that, with the patches, the hardware works, and
>> without the patches (at least the first) you cannot even build with
>> CONFIG_SOC_MT7621=y as pcibios_map_irq() is undefined. Having
>> working code is a great starting point for further development.
>> It certainly isn't ready for upstream, which is why it is heading for
>> drivers/staging. This is explicitly for code that isn't yet ready.
>> By putting the code there it should be safe from bit-rot, and can be
>> worked on by multiple people. It gets increased visibility so people
>> can say how bad it is (as you have done - thanks). This feed back is a
>> valuable part of improving the code and getting it out of staging.
>>
>> I'll add notes to various TODO files based on your comments. If you
>> have anything else to add, it would be most welcome. Thank you for
>> making these patches available in the first place, so that my hardware
>> can work!
>>
>> Thanks,
>> NeilBrown
>
> Hi Neil,
>
> I understand your reasoning, however ...
> only the pcie driver is worth merging. all other drivers are already
> inside the kernel for mt7623 and can be easily adapted to work on
> mt7621. having duplicate drivers is a certain no-go.
> cleaning up the pci driver is a matter of a few days work. merging a
> shitty pci driver just to postpone doing the 3-5 days work involved to
> polish it seems a rally bad trade-off.
> i strongly oppose having any of this code merged into the kernel, even
> if it is only the staging area.

Hi John,
I don't understand why you would want to deny people easy access to code
which makes their hardware work. I'm sure that isn't your intention,
but it could well be the effect. If you think you can make it work in
3-5 days, then please go right ahead. I will happily test anything you
submit. I suspect the most likely outcome, however, is that I'll end
up doing all the work, and I'm sure it will take a lot more than 3-5
days and will involve a lot of learning. I'll be happy if I can get
all of this back out of staging in 6 months. That is an extra 6 months
(at least) that people will be able to use a mainline kernel on their
gnubee.

With respect to your suggestion that "having duplicate drivers is a
certain no-go", I don't think that is correct. As an approximate
counter-point, I'm in the process of cleaning up the
drivers/staging/lustre filesystem with the hope of eventually moving it
out of staging. It had duplicate wait_event() code, duplicate PRNG,
duplicate workqueues, duplicate resizeable hashtable, duplicate tracing
infrastructure, and more that I haven't had a chance to look closely at
yet. This duplication certainly kept it out of linux/fs/, but has no
bearing on whether it belong in linux/drivers/staging/.

Thanks,
NeilBrown

Attachments:

signature.asc (847.00 B)

2018-03-16 07:44:09

by Dan Carpenter

[permalink] [raw]

Subject: Re: [PATCH 00/13] staging: add drivers to support Mediatek mt7621 in gnubee-pc1

On Fri, Mar 16, 2018 at 07:02:51AM +1100, NeilBrown wrote:
> On Thu, Mar 15 2018, Dan Carpenter wrote:
>
> > On Thu, Mar 15, 2018 at 10:04:33PM +1100, NeilBrown wrote:
> >> On Thu, Mar 15 2018, Dan Carpenter wrote:
> >>
> >> > This all seems fine. Generally the requirements for staging are that it
> >> > has a TODO, someone to work on it, and it doesn't break the build. But
> >> > some of the patches don't have commit message and those are required and
> >> > some of the commit messages are just the changes you have made not don't
> >> > describe the actual code...
> >>
> >> Thanks for having a look.
> >> It seems odd to require detailed commit messages, when we don't require
> >> the same level of quality in the code.
> >> Naturally when the driver is moved out of staging a properly detailed
> >> commit message should be added, but is that needed on the way in to
> >> staging? At this stage I don't know much more than is already there.
> >> After I've cleaned up the code I probably will.
> >>
> >> For patch 01/13 you asked "what kind of device this is". The subject
> >> line makes it clear that it is a "pcie driver". What extra detail did
> >> you want? Would it be sufficient to just copy the subject line so that
> >> it appears twice in the commit message?
> >>
> >
> > Ah... Sorry. It's literally a pcie driver. For some reason I thought
> > it was a device that ran over pcie.
> >
> > We don't require a detailed changelog, but you have to put something...
> > Probably just restating the subject and adding that it's for the gnubee1
> > is fine.
>
> I'll resend sometime next week with more words. However could you
> please clarify a couple of things for me?
>
> 1/ Why do you (sometimes) call the commit message a "change log". When
> I see the term "change log" in the context of a patch, my first
> thought is that it it means a log of changes that have been made to
> the patch - typically through the review cycle. But that isn't what
> you mean. This has confused me a couple of times.
>

Sorry. Yeah. I do use them interchangeably which is probably not the
right thing.

> 2/ Why don't you consider the first line of the commit message to be
> part of the commit message? Why is duplication required?
> (You said "some of the patches don't have commit message[s]",
> which isn't true, though some of the messages are only one line).
>

First of all, you're a writer. If you don't like to do things then you
need to keep those skills hidden away. I never tell anyone that I know
how to program in COBOL (True story. COBOL is great for formatting text
on dot matrix printers and for doing decimal math.)

This is a hard requirement from Greg, not something that specific to
*me* although I do agree with the requirement as well. The idea of
forcing everyone to write a commit message is that we're hoping they
take a little time to tell a story about what the patch is. Sometimes
they're not English majors or whatever and they just restate the subject
and whatever, that's fine.

The first patch I reviewed in this series was:
[PATCH 03/13] staging: mt7621-gpio: ralink: add mt7621 gpio controller
A good commit message might be:

This adds Mediatek GPIO support for the mt7621 chip. It's used on
the Gnubee NAS hardware and a couple other MIPS SoCs. This code
originally came from the OpenWRT project.

That information was all there in the patch 0 commit but that gets
dropped. It's feels a bit weird to put that boilerplate information in
every commit, but it means that it's there when we do a git log on the
file so I think it's a good idea.

Also, and this is my fault not yours of course, but my email client is
all text and looks exactly like marc.info:
https://marc.info/?l=linux-driver-devel&m=152105965413484&w=2

It's hard to see the subject so I normally don't even read it when I'm
looking at patches.

regards,
dan carpenter