Re: [alsa-devel] [PATCH v3 2/5] soundwire: fix style issues
On 4/30/19 9:54 AM, Vfiinod Koul wrote: On 30-04-19, 08:38, Pierre-Louis Bossart wrote: On 4/30/19 3:51 AM, Vinod Koul wrote: On 15-04-19, 08:09, Pierre-Louis Bossart wrote: Signed-off-by: Pierre-Louis Bossart --- drivers/soundwire/Kconfig | 2 +- drivers/soundwire/bus.c| 87 drivers/soundwire/bus.h| 16 +-- drivers/soundwire/bus_type.c | 4 +- drivers/soundwire/cadence_master.c | 87 drivers/soundwire/cadence_master.h | 22 ++-- drivers/soundwire/intel.c | 87 drivers/soundwire/intel.h | 4 +- drivers/soundwire/intel_init.c | 12 +-- drivers/soundwire/mipi_disco.c | 116 +++-- drivers/soundwire/slave.c | 10 +- drivers/soundwire/stream.c | 161 +++-- I would prefer this to be a patch per module. It doesnt help to have a single patch for all the files! It would be great to have cleanup done per logical group, for example typos in a patch, aligns in another etc... You've got to be kidding. I've never seen people ask for this sort of detail. Nope this is the way it should be. A patch is patch and which should do one thing! Even if it is a cleanup one. I dislike a patch which touches everything, core, modules, so please split up. As a said in review it takes guesswork to find why a change was done, was it whitespace fix, indentation or not, so please split up based on type of fixes. With all due respect, you are not helping here but rather slowing things down. I've done dozens of cleanups in the ALSA tree and I didn't go in this sort of details. Thats fine, it is upto people, everyone has different views, mine is different from Takashi's. We all know for example networking has different stable and code style rule. That is how it is and I dont think we would have one rule for all kernel. All I ask is to be able to review and split up accordingly, I guess that is a fair request The fact that the series was tagged as Reviewed by Takashi on April 11 and we are still discussing trivial changes tells me the integration model is broken. Is it? you got feedback on 15th (that too after my 2 week conf/vacation break) and I got called crazy for that, not helping!! It's not just me the patches related to runtime-pm from your own Linaro colleagues posted on March 28 went nowhere either. Does it matter it was a Linaro colleague or not, a patch was posted, feedback given (similar to cadence one) we agreed that the fix is not correct and so patch was not applied. I don't think Srini cried over it! Moving forward, I suggest we merge SoundWire-related patches through the sound tree. There will be dependencies in the coming weeks between SOF and SoundWire and it makes no sense to have separate maintainers and make the life of early adopters more complicated than it needs to be. If we have 3-week delays for trivial stuff, I can't imagine what the pace will be when I publish the next 20-odd patches I am still working on, and the code needed for the SoundWire audio device class being standardized as we speak. Things were fine up to now since no one was actually using the code, we are in a different model now. I disagree and wont accept it. I dont think you understand that you are not the most important person in the whole world, the 20 patches series you are cooking would sure be greatest ever, but that is not the point. The kernel has a process, you got a feedback, please fix that and post v2 rather than cribbing, complaining and calling crazy. The energy would have been better spent on fixing the feedback provided. Dependencies are _always_ there in kernel development and we know how to deal with it. Am sure Takashi, Mark and me can come to reasonable agreement, I wouldn't worry about that! What we dont do is create new model for your 20 patches. And I guess I dont have anything more to say on this thread, so I wont bother replying, please feel free to post v2 and I shall review. Friends have disagreements. We remain friends and I will provide a v2. I still believe it makes no sense to split the integration of SoundWire-related patches in two different trees. The only rationale for it might be that SoundWire is a 'bus' than could be used in other areas. Except that for now and the foreseeable future (2022+) it's only for audio as a replacement of HDaudio, so the pragmatic way of dealing with SoundWire is to merge the code through the audio tree. And given that the code is not in a usable state at the moment, dealing with the audio tree would not have any negative impact on anyone.
Re: [alsa-devel] [PATCH v3 2/5] soundwire: fix style issues
On Tue, Apr 30, 2019 at 09:13:55AM -0500, Pierre-Louis Bossart wrote: > > > My patch-bot would reject a patch that tried to do multiple types of > > different cleanups on the same file(s). Has done so for _years_, this > > is not a new thing. > > If there are tools let's use them (all the fixes in this series were > reported by tools). Can you share pointers and location of this patch-bot? I talked about my bot a long time ago in one of my presentations, the source isn't around anywhere public, sorry. But here's the template for what it can spit out, depending on the patch input, feel free to cut/paste from it for your use when reviewing patches. thanks, greg k-h - Hi, This is the friendly patch-bot of Greg Kroah-Hartman. You have sent him a patch that has triggered this response. He used to manually respond to these common problems, but in order to save his sanity (he kept writing the same thing over and over, yet to different people), I was created. Hopefully you will not take offence and will fix the problem in your patch and resubmit it so that it can be accepted into the Linux kernel tree. You are receiving this message because of the following common error(s) as indicated below: - Your patch breaks the build. - Your patch contains warnings and/or errors noticed by the scripts/checkpatch.pl tool. - Your patch is malformed (tabs converted to spaces, linewrapped, etc.) and can not be applied. Please read the file, Documentation/email-clients.txt in order to fix this. - Your patch was attached, please place it inline so that it can be applied directly from the email message itself. - Your patch does not have a Signed-off-by: line. Please read the kernel file, Documentation/SubmittingPatches and resend it after adding that line. Note, the line needs to be in the body of the email, before the patch, not at the bottom of the patch or in the email signature. - Your patch was sent privately to Greg. Kernel development is done in public, please always cc: a public mailing list with a patch submission. Using the tool, scripts/get_maintainer.pl on the patch will tell you what mailing list to cc. - Your patch did many different things all at once, making it difficult to review. All Linux kernel patches need to only do one thing at a time. If you need to do multiple things (such as clean up all coding style issues in a file/driver), do it in a sequence of patches, each one doing only one thing. This will make it easier to review the patches to ensure that they are correct, and to help alleviate any merge issues that larger patches can cause. - Your patch did not apply to any known trees that Greg is in control of. Possibly this is because you made it against Linus's tree, not the linux-next tree, which is where all of the development for the next version of the kernel is at. Please refresh your patch against the linux-next tree, or even better yet, the development tree specified in the MAINTAINERS file for the subsystem you are submitting a patch for, and resend it. - You sent multiple patches, yet no indication of which ones should be applied in which order. Greg could just guess, but if you are receiving this email, he guessed wrong and the patches didn't apply. Please read the section entitled "The canonical patch format" in the kernel file, Documentation/SubmittingPatches for a description of how to do this so that Greg has a chance to apply these correctly. - You did not specify a description of why the patch is needed, or possibly, any description at all, in the email body. Please read the section entitled "The canonical patch format" in the kernel file, Documentation/SubmittingPatches for what is needed in order to properly describe the change. - You did not write a descriptive Subject: for the patch, allowing Greg, and everyone else, to know what this patch is all about. Please read the section entitled "The canonical patch format" in the kernel file, Documentation/SubmittingPatches for what a proper Subject: line should look like. If you wish to discuss this problem further, or you have questions about how to resolve this issue, please feel free to respond to this email and Greg will reply once he has dug out from the pending patches received from other developers. thanks, greg k-h's patch email bot
Re: [alsa-devel] [PATCH v3 2/5] soundwire: fix style issues
My patch-bot would reject a patch that tried to do multiple types of different cleanups on the same file(s). Has done so for _years_, this is not a new thing. If there are tools let's use them (all the fixes in this series were reported by tools). Can you share pointers and location of this patch-bot?
Re: [alsa-devel] [PATCH v3 2/5] soundwire: fix style issues
On Tue, Apr 30, 2019 at 08:38:01AM -0500, Pierre-Louis Bossart wrote: > On 4/30/19 3:51 AM, Vinod Koul wrote: > > On 15-04-19, 08:09, Pierre-Louis Bossart wrote: > > > > > > > > > > > > > Signed-off-by: Pierre-Louis Bossart > > > > > > > > > > --- > > > > >drivers/soundwire/Kconfig | 2 +- > > > > >drivers/soundwire/bus.c| 87 > > > > >drivers/soundwire/bus.h| 16 +-- > > > > >drivers/soundwire/bus_type.c | 4 +- > > > > >drivers/soundwire/cadence_master.c | 87 > > > > >drivers/soundwire/cadence_master.h | 22 ++-- > > > > >drivers/soundwire/intel.c | 87 > > > > >drivers/soundwire/intel.h | 4 +- > > > > >drivers/soundwire/intel_init.c | 12 +-- > > > > >drivers/soundwire/mipi_disco.c | 116 +++-- > > > > >drivers/soundwire/slave.c | 10 +- > > > > >drivers/soundwire/stream.c | 161 > > > > > +++-- > > > > > > > > I would prefer this to be a patch per module. It doesnt help to have a > > > > single patch for all the files! > > > > > > > > It would be great to have cleanup done per logical group, for example > > > > typos in a patch, aligns in another etc... > > > > > > You've got to be kidding. I've never seen people ask for this sort of > > > detail. > > > > Nope this is the way it should be. A patch is patch and which > > should do one thing! Even if it is a cleanup one. > > > > I dislike a patch which touches everything, core, modules, so please > > split up. As a said in review it takes guesswork to find why a change > > was done, was it whitespace fix, indentation or not, so please split up > > based on type of fixes. > > With all due respect, you are not helping here but rather slowing things > down. I've done dozens of cleanups in the ALSA tree and I didn't go in this > sort of details. The fact that the series was tagged as Reviewed by Takashi > on April 11 and we are still discussing trivial changes tells me the > integration model is broken. It's not just me, the patches related to > runtime-pm from your own Linaro colleagues posted on March 28 went nowhere > either. My patch-bot would reject a patch that tried to do multiple types of different cleanups on the same file(s). Has done so for _years_, this is not a new thing. Remember, maintainer/reviewer time is scarce, engineer time is prolific, we optimize for reviewers, not the people writing the patches. thanks, greg k-h
Re: [alsa-devel] [PATCH v3 2/5] soundwire: fix style issues
On 4/30/19 3:51 AM, Vinod Koul wrote: On 15-04-19, 08:09, Pierre-Louis Bossart wrote: Signed-off-by: Pierre-Louis Bossart --- drivers/soundwire/Kconfig | 2 +- drivers/soundwire/bus.c| 87 drivers/soundwire/bus.h| 16 +-- drivers/soundwire/bus_type.c | 4 +- drivers/soundwire/cadence_master.c | 87 drivers/soundwire/cadence_master.h | 22 ++-- drivers/soundwire/intel.c | 87 drivers/soundwire/intel.h | 4 +- drivers/soundwire/intel_init.c | 12 +-- drivers/soundwire/mipi_disco.c | 116 +++-- drivers/soundwire/slave.c | 10 +- drivers/soundwire/stream.c | 161 +++-- I would prefer this to be a patch per module. It doesnt help to have a single patch for all the files! It would be great to have cleanup done per logical group, for example typos in a patch, aligns in another etc... You've got to be kidding. I've never seen people ask for this sort of detail. Nope this is the way it should be. A patch is patch and which should do one thing! Even if it is a cleanup one. I dislike a patch which touches everything, core, modules, so please split up. As a said in review it takes guesswork to find why a change was done, was it whitespace fix, indentation or not, so please split up based on type of fixes. With all due respect, you are not helping here but rather slowing things down. I've done dozens of cleanups in the ALSA tree and I didn't go in this sort of details. The fact that the series was tagged as Reviewed by Takashi on April 11 and we are still discussing trivial changes tells me the integration model is broken. It's not just me, the patches related to runtime-pm from your own Linaro colleagues posted on March 28 went nowhere either. Moving forward, I suggest we merge SoundWire-related patches through the sound tree. There will be dependencies in the coming weeks between SOF and SoundWire and it makes no sense to have separate maintainers and make the life of early adopters more complicated than it needs to be. If we have 3-week delays for trivial stuff, I can't imagine what the pace will be when I publish the next 20-odd patches I am still working on, and the code needed for the SoundWire audio device class being standardized as we speak. Things were fine up to now since no one was actually using the code, we are in a different model now.
Re: [alsa-devel] [PATCH v3 2/5] soundwire: fix style issues
On 19-04-19, 12:14, Pierre-Louis Bossart wrote: > > > > > enum sdw_command_response > > > > cdns_xfer_msg_defer(struct sdw_bus *bus, > > > > - struct sdw_msg *msg, struct sdw_defer *defer) > > > > + struct sdw_msg *msg, struct sdw_defer *defer) > > > > > > this one too.. > > > > > > > static int cdns_port_params(struct sdw_bus *bus, > > > > - struct sdw_port_params *p_params, unsigned int bank) > > > > + struct sdw_port_params *p_params, unsigned int bank) > > > > > > here as well.. (and giving up on rest) > > > > Please check for yourself that this is a diff illusion w/ tab space. > > Vinod, can you please double-check, the alignment issues you reported don't > exist, see e.g. below what the code looks like after merge. > > > int sdw_transfer_defer(struct sdw_bus *bus, struct sdw_msg *msg, >struct sdw_defer *defer) > > int sdw_fill_msg(struct sdw_msg *msg, struct sdw_slave *slave, > u32 addr, size_t count, u16 dev_num, u8 flags, u8 *buf) Sure, please split up as requested and I shall test apply and check alignment before reporting... -- ~Vinod
Re: [alsa-devel] [PATCH v3 2/5] soundwire: fix style issues
On 15-04-19, 08:09, Pierre-Louis Bossart wrote:
>
> > >
> > > Signed-off-by: Pierre-Louis Bossart
> > > ---
> > > drivers/soundwire/Kconfig | 2 +-
> > > drivers/soundwire/bus.c| 87
> > > drivers/soundwire/bus.h| 16 +--
> > > drivers/soundwire/bus_type.c | 4 +-
> > > drivers/soundwire/cadence_master.c | 87
> > > drivers/soundwire/cadence_master.h | 22 ++--
> > > drivers/soundwire/intel.c | 87
> > > drivers/soundwire/intel.h | 4 +-
> > > drivers/soundwire/intel_init.c | 12 +--
> > > drivers/soundwire/mipi_disco.c | 116 +++--
> > > drivers/soundwire/slave.c | 10 +-
> > > drivers/soundwire/stream.c | 161 +++--
> >
> > I would prefer this to be a patch per module. It doesnt help to have a
> > single patch for all the files!
> >
> > It would be great to have cleanup done per logical group, for example
> > typos in a patch, aligns in another etc...
>
> You've got to be kidding. I've never seen people ask for this sort of
> detail.
Nope this is the way it should be. A patch is patch and which
should do one thing! Even if it is a cleanup one.
I dislike a patch which touches everything, core, modules, so please
split up. As a said in review it takes guesswork to find why a change
was done, was it whitespace fix, indentation or not, so please split up
based on type of fixes.
>
> >
> > > 12 files changed, 313 insertions(+), 295 deletions(-)
> > >
> > > diff --git a/drivers/soundwire/Kconfig b/drivers/soundwire/Kconfig
> > > index 19c8efb9a5ee..84876a74874f 100644
> > > --- a/drivers/soundwire/Kconfig
> > > +++ b/drivers/soundwire/Kconfig
> > > @@ -4,7 +4,7 @@
> > > menuconfig SOUNDWIRE
> > > bool "SoundWire support"
> > > - ---help---
> > > + help
> >
> > Not sure if this is a style issue, kernel seems to have 2990 instances
> > of this!
>
> this is reported by checkpatch.pl --strict.
>
> >
> > > if (msg->page)
> > > sdw_reset_page(bus, msg->dev_num);
> > > @@ -243,7 +244,7 @@ int sdw_transfer(struct sdw_bus *bus, struct sdw_msg
> > > *msg)
> > >* Caller needs to hold the msg_lock lock while calling this
> > >*/
> > > int sdw_transfer_defer(struct sdw_bus *bus, struct sdw_msg *msg,
> > > - struct sdw_defer *defer)
> > > +struct sdw_defer *defer)
> >
> > this does not seem aligned to me!
>
> It is, I checked. 2 tabs and 7 spaces.
>
> >
> > > int sdw_fill_msg(struct sdw_msg *msg, struct sdw_slave *slave,
> > > - u32 addr, size_t count, u16 dev_num, u8 flags, u8 *buf)
> > > + u32 addr, size_t count, u16 dev_num, u8 flags, u8 *buf)
> >
> > this one too
>
> 2 tabs and one space.
>
> >
> > > @@ -458,13 +458,13 @@ static int sdw_assign_device_num(struct sdw_slave
> > > *slave)
> > > mutex_unlock(>bus->bus_lock);
> > > if (dev_num < 0) {
> > > dev_err(slave->bus->dev, "Get dev_num failed:
> > > %d",
> > > - dev_num);
> > > + dev_num);
> >
> > It might read better if we move the log to second line along with
> > dev_num...
?
> >
> > > int sdw_configure_dpn_intr(struct sdw_slave *slave,
> > > - int port, bool enable, int mask)
> > > +int port, bool enable, int mask)
> >
> > not aligned
>
> it is in the code. It's a diff illusion.
>
> >
> > > void sdw_extract_slave_id(struct sdw_bus *bus,
> > > - u64 addr, struct sdw_slave_id *id);
> > > + u64 addr, struct sdw_slave_id *id);
> >
> > Not aligned
>
> it is in the code. It's a diff illusion.
>
> > > enum sdw_command_response
> > > cdns_xfer_msg_defer(struct sdw_bus *bus,
> > > - struct sdw_msg *msg, struct sdw_defer *defer)
> > > + struct sdw_msg *msg, struct sdw_defer *defer)
> >
> > this one too..
> >
> > > static int cdns_port_params(struct sdw_bus *bus,
> > > - struct sdw_port_params *p_params, unsigned int bank)
> > > + struct sdw_port_params *p_params, unsigned int bank)
> >
> > here as well.. (and giving up on rest)
>
> Please check for yourself that this is a diff illusion w/ tab space.
--
~Vinod
Re: [alsa-devel] [PATCH v3 2/5] soundwire: fix style issues
enum sdw_command_response cdns_xfer_msg_defer(struct sdw_bus *bus, - struct sdw_msg *msg, struct sdw_defer *defer) + struct sdw_msg *msg, struct sdw_defer *defer) this one too.. static int cdns_port_params(struct sdw_bus *bus, - struct sdw_port_params *p_params, unsigned int bank) + struct sdw_port_params *p_params, unsigned int bank) here as well.. (and giving up on rest) Please check for yourself that this is a diff illusion w/ tab space. Vinod, can you please double-check, the alignment issues you reported don't exist, see e.g. below what the code looks like after merge. int sdw_transfer_defer(struct sdw_bus *bus, struct sdw_msg *msg, struct sdw_defer *defer) int sdw_fill_msg(struct sdw_msg *msg, struct sdw_slave *slave, u32 addr, size_t count, u16 dev_num, u8 flags, u8 *buf)
Re: [alsa-devel] [PATCH v3 2/5] soundwire: fix style issues
On Wed, Apr 17, 2019 at 12:18:22PM -0500, Pierre-Louis Bossart wrote: > > diff --git a/drivers/soundwire/Kconfig b/drivers/soundwire/Kconfig > index 19c8efb9a5ee..84876a74874f 100644 > --- a/drivers/soundwire/Kconfig > +++ b/drivers/soundwire/Kconfig > @@ -4,7 +4,7 @@ > > menuconfig SOUNDWIRE > bool "SoundWire support" > ----help--- > +help > >>> > >>> Not sure if this is a style issue, kernel seems to have 2990 instances > >>> of this! > >> > >> this is reported by checkpatch.pl --strict. > > > > Please don't run checkpatch on code that's already in the kernel, and > > especially not with the --strict (a.k.a. --subjective) option enabled. > > > > Don't try to fix what isn't broken. > > I would agree in general, but this case is different: the SoundWire code > in the upstream kernel is missing parts left and right and isn't fully > functional as is. I will soon be posting what's missing, so this cleanup > is an opportunity to bring SoundWire to the latest coding standards > before adding the missing pieces which will be compliant with --strict. > For the record using --strict already exposed 3 major issues in the > yet-to-be-released code, so it's not as subjective as you describe it. It's not just me calling it subjective; --subjective is literally another name for the same switch which enables checks that are specifically *not* part of the coding standard. By all my means use it on your own patches before you submit them if you agree with all or some of those checks, but I doubt all that open-parenthesis re-alignment is going to expose any major issues. ;) It does add noise, and makes code forensic and backports harder though. Johan
Re: [alsa-devel] [PATCH v3 2/5] soundwire: fix style issues
diff --git a/drivers/soundwire/Kconfig b/drivers/soundwire/Kconfig index 19c8efb9a5ee..84876a74874f 100644 --- a/drivers/soundwire/Kconfig +++ b/drivers/soundwire/Kconfig @@ -4,7 +4,7 @@ menuconfig SOUNDWIRE bool "SoundWire support" - ---help--- + help Not sure if this is a style issue, kernel seems to have 2990 instances of this! this is reported by checkpatch.pl --strict. Please don't run checkpatch on code that's already in the kernel, and especially not with the --strict (a.k.a. --subjective) option enabled. Don't try to fix what isn't broken. I would agree in general, but this case is different: the SoundWire code in the upstream kernel is missing parts left and right and isn't fully functional as is. I will soon be posting what's missing, so this cleanup is an opportunity to bring SoundWire to the latest coding standards before adding the missing pieces which will be compliant with --strict. For the record using --strict already exposed 3 major issues in the yet-to-be-released code, so it's not as subjective as you describe it.
Re: [alsa-devel] [PATCH v3 2/5] soundwire: fix style issues
On Mon, Apr 15, 2019 at 08:09:59AM -0500, Pierre-Louis Bossart wrote: > > >> > >> Signed-off-by: Pierre-Louis Bossart > >> --- > >> drivers/soundwire/Kconfig | 2 +- > >> drivers/soundwire/bus.c| 87 > >> drivers/soundwire/bus.h| 16 +-- > >> drivers/soundwire/bus_type.c | 4 +- > >> drivers/soundwire/cadence_master.c | 87 > >> drivers/soundwire/cadence_master.h | 22 ++-- > >> drivers/soundwire/intel.c | 87 > >> drivers/soundwire/intel.h | 4 +- > >> drivers/soundwire/intel_init.c | 12 +-- > >> drivers/soundwire/mipi_disco.c | 116 +++-- > >> drivers/soundwire/slave.c | 10 +- > >> drivers/soundwire/stream.c | 161 +++-- > > > > I would prefer this to be a patch per module. It doesnt help to have a > > single patch for all the files! > > > > It would be great to have cleanup done per logical group, for example > > typos in a patch, aligns in another etc... > > You've got to be kidding. I've never seen people ask for this sort of > detail. > > > > >> 12 files changed, 313 insertions(+), 295 deletions(-) > >> > >> diff --git a/drivers/soundwire/Kconfig b/drivers/soundwire/Kconfig > >> index 19c8efb9a5ee..84876a74874f 100644 > >> --- a/drivers/soundwire/Kconfig > >> +++ b/drivers/soundwire/Kconfig > >> @@ -4,7 +4,7 @@ > >> > >> menuconfig SOUNDWIRE > >>bool "SoundWire support" > >> - ---help--- > >> + help > > > > Not sure if this is a style issue, kernel seems to have 2990 instances > > of this! > > this is reported by checkpatch.pl --strict. Please don't run checkpatch on code that's already in the kernel, and especially not with the --strict (a.k.a. --subjective) option enabled. Don't try to fix what isn't broken. Thanks, Johan
Re: [alsa-devel] [PATCH v3 2/5] soundwire: fix style issues
Signed-off-by: Pierre-Louis Bossart
---
drivers/soundwire/Kconfig | 2 +-
drivers/soundwire/bus.c| 87
drivers/soundwire/bus.h| 16 +--
drivers/soundwire/bus_type.c | 4 +-
drivers/soundwire/cadence_master.c | 87
drivers/soundwire/cadence_master.h | 22 ++--
drivers/soundwire/intel.c | 87
drivers/soundwire/intel.h | 4 +-
drivers/soundwire/intel_init.c | 12 +--
drivers/soundwire/mipi_disco.c | 116 +++--
drivers/soundwire/slave.c | 10 +-
drivers/soundwire/stream.c | 161 +++--
I would prefer this to be a patch per module. It doesnt help to have a
single patch for all the files!
It would be great to have cleanup done per logical group, for example
typos in a patch, aligns in another etc...
You've got to be kidding. I've never seen people ask for this sort of
detail.
12 files changed, 313 insertions(+), 295 deletions(-)
diff --git a/drivers/soundwire/Kconfig b/drivers/soundwire/Kconfig
index 19c8efb9a5ee..84876a74874f 100644
--- a/drivers/soundwire/Kconfig
+++ b/drivers/soundwire/Kconfig
@@ -4,7 +4,7 @@
menuconfig SOUNDWIRE
bool "SoundWire support"
- ---help---
+ help
Not sure if this is a style issue, kernel seems to have 2990 instances
of this!
this is reported by checkpatch.pl --strict.
if (msg->page)
sdw_reset_page(bus, msg->dev_num);
@@ -243,7 +244,7 @@ int sdw_transfer(struct sdw_bus *bus, struct sdw_msg *msg)
* Caller needs to hold the msg_lock lock while calling this
*/
int sdw_transfer_defer(struct sdw_bus *bus, struct sdw_msg *msg,
- struct sdw_defer *defer)
+ struct sdw_defer *defer)
this does not seem aligned to me!
It is, I checked. 2 tabs and 7 spaces.
int sdw_fill_msg(struct sdw_msg *msg, struct sdw_slave *slave,
- u32 addr, size_t count, u16 dev_num, u8 flags, u8 *buf)
+u32 addr, size_t count, u16 dev_num, u8 flags, u8 *buf)
this one too
2 tabs and one space.
@@ -458,13 +458,13 @@ static int sdw_assign_device_num(struct sdw_slave *slave)
mutex_unlock(>bus->bus_lock);
if (dev_num < 0) {
dev_err(slave->bus->dev, "Get dev_num failed: %d",
- dev_num);
+ dev_num);
It might read better if we move the log to second line along with
dev_num...
int sdw_configure_dpn_intr(struct sdw_slave *slave,
- int port, bool enable, int mask)
+ int port, bool enable, int mask)
not aligned
it is in the code. It's a diff illusion.
void sdw_extract_slave_id(struct sdw_bus *bus,
- u64 addr, struct sdw_slave_id *id);
+ u64 addr, struct sdw_slave_id *id);
Not aligned
it is in the code. It's a diff illusion.
enum sdw_command_response
cdns_xfer_msg_defer(struct sdw_bus *bus,
- struct sdw_msg *msg, struct sdw_defer *defer)
+ struct sdw_msg *msg, struct sdw_defer *defer)
this one too..
static int cdns_port_params(struct sdw_bus *bus,
- struct sdw_port_params *p_params, unsigned int bank)
+ struct sdw_port_params *p_params, unsigned int bank)
here as well.. (and giving up on rest)
Please check for yourself that this is a diff illusion w/ tab space.
Re: [PATCH v3 2/5] soundwire: fix style issues
On 10-04-19, 22:16, Pierre-Louis Bossart wrote:
> Visual inspections confirmed by checkpatch.pl --strict expose a number
> of style issues, specifically parameter alignment is inconsistent as
> if different contributors used different styles. Before we restart
> support for SoundWire with Sound Open Firmware on Intel platforms,
> let's clean all this.
>
> Fix Kconfig help, spelling, SPDX format, alignment, spurious
> parentheses, bool comparisons to true/false, macro argument
> protection.
Thanks for the cleanup Pierre :)
>
> No new functionality added.
>
> Signed-off-by: Pierre-Louis Bossart
> ---
> drivers/soundwire/Kconfig | 2 +-
> drivers/soundwire/bus.c| 87
> drivers/soundwire/bus.h| 16 +--
> drivers/soundwire/bus_type.c | 4 +-
> drivers/soundwire/cadence_master.c | 87
> drivers/soundwire/cadence_master.h | 22 ++--
> drivers/soundwire/intel.c | 87
> drivers/soundwire/intel.h | 4 +-
> drivers/soundwire/intel_init.c | 12 +--
> drivers/soundwire/mipi_disco.c | 116 +++--
> drivers/soundwire/slave.c | 10 +-
> drivers/soundwire/stream.c | 161 +++--
I would prefer this to be a patch per module. It doesnt help to have a
single patch for all the files!
It would be great to have cleanup done per logical group, for example
typos in a patch, aligns in another etc...
> 12 files changed, 313 insertions(+), 295 deletions(-)
>
> diff --git a/drivers/soundwire/Kconfig b/drivers/soundwire/Kconfig
> index 19c8efb9a5ee..84876a74874f 100644
> --- a/drivers/soundwire/Kconfig
> +++ b/drivers/soundwire/Kconfig
> @@ -4,7 +4,7 @@
>
> menuconfig SOUNDWIRE
> bool "SoundWire support"
> - ---help---
> + help
Not sure if this is a style issue, kernel seems to have 2990 instances
of this!
> if (msg->page)
> sdw_reset_page(bus, msg->dev_num);
> @@ -243,7 +244,7 @@ int sdw_transfer(struct sdw_bus *bus, struct sdw_msg *msg)
> * Caller needs to hold the msg_lock lock while calling this
> */
> int sdw_transfer_defer(struct sdw_bus *bus, struct sdw_msg *msg,
> - struct sdw_defer *defer)
> +struct sdw_defer *defer)
this does not seem aligned to me!
> int sdw_fill_msg(struct sdw_msg *msg, struct sdw_slave *slave,
> - u32 addr, size_t count, u16 dev_num, u8 flags, u8 *buf)
> + u32 addr, size_t count, u16 dev_num, u8 flags, u8 *buf)
this one too
> @@ -458,13 +458,13 @@ static int sdw_assign_device_num(struct sdw_slave
> *slave)
> mutex_unlock(>bus->bus_lock);
> if (dev_num < 0) {
> dev_err(slave->bus->dev, "Get dev_num failed: %d",
> - dev_num);
> + dev_num);
It might read better if we move the log to second line along with
dev_num...
> int sdw_configure_dpn_intr(struct sdw_slave *slave,
> - int port, bool enable, int mask)
> +int port, bool enable, int mask)
not aligned
> void sdw_extract_slave_id(struct sdw_bus *bus,
> - u64 addr, struct sdw_slave_id *id);
> + u64 addr, struct sdw_slave_id *id);
>
Not aligned
> enum sdw_command_response
> cdns_xfer_msg_defer(struct sdw_bus *bus,
> - struct sdw_msg *msg, struct sdw_defer *defer)
> + struct sdw_msg *msg, struct sdw_defer *defer)
this one too..
> static int cdns_port_params(struct sdw_bus *bus,
> - struct sdw_port_params *p_params, unsigned int bank)
> + struct sdw_port_params *p_params, unsigned int bank)
here as well.. (and giving up on rest)
--
~Vinod
[PATCH v3 2/5] soundwire: fix style issues
Visual inspections confirmed by checkpatch.pl --strict expose a number
of style issues, specifically parameter alignment is inconsistent as
if different contributors used different styles. Before we restart
support for SoundWire with Sound Open Firmware on Intel platforms,
let's clean all this.
Fix Kconfig help, spelling, SPDX format, alignment, spurious
parentheses, bool comparisons to true/false, macro argument
protection.
No new functionality added.
Signed-off-by: Pierre-Louis Bossart
---
drivers/soundwire/Kconfig | 2 +-
drivers/soundwire/bus.c| 87
drivers/soundwire/bus.h| 16 +--
drivers/soundwire/bus_type.c | 4 +-
drivers/soundwire/cadence_master.c | 87
drivers/soundwire/cadence_master.h | 22 ++--
drivers/soundwire/intel.c | 87
drivers/soundwire/intel.h | 4 +-
drivers/soundwire/intel_init.c | 12 +--
drivers/soundwire/mipi_disco.c | 116 +++--
drivers/soundwire/slave.c | 10 +-
drivers/soundwire/stream.c | 161 +++--
12 files changed, 313 insertions(+), 295 deletions(-)
diff --git a/drivers/soundwire/Kconfig b/drivers/soundwire/Kconfig
index 19c8efb9a5ee..84876a74874f 100644
--- a/drivers/soundwire/Kconfig
+++ b/drivers/soundwire/Kconfig
@@ -4,7 +4,7 @@
menuconfig SOUNDWIRE
bool "SoundWire support"
- ---help---
+ help
SoundWire is a 2-Pin interface with data and clock line ratified
by the MIPI Alliance. SoundWire is used for transporting data
typically related to audio functions. SoundWire interface is
diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c
index 1cbfedfc20ef..691a31df9732 100644
--- a/drivers/soundwire/bus.c
+++ b/drivers/soundwire/bus.c
@@ -49,7 +49,7 @@ int sdw_add_bus_master(struct sdw_bus *bus)
}
/*
-* Device numbers in SoundWire are 0 thru 15. Enumeration device
+* Device numbers in SoundWire are 0 through 15. Enumeration device
* number (0), Broadcast device number (15), Group numbers (12 and
* 13) and Master device number (14) are not used for assignment so
* mask these and other higher bits.
@@ -172,7 +172,8 @@ static inline int do_transfer(struct sdw_bus *bus, struct
sdw_msg *msg)
}
static inline int do_transfer_defer(struct sdw_bus *bus,
- struct sdw_msg *msg, struct sdw_defer *defer)
+ struct sdw_msg *msg,
+ struct sdw_defer *defer)
{
int retry = bus->prop.err_threshold;
enum sdw_command_response resp;
@@ -224,7 +225,7 @@ int sdw_transfer(struct sdw_bus *bus, struct sdw_msg *msg)
ret = do_transfer(bus, msg);
if (ret != 0 && ret != -ENODATA)
dev_err(bus->dev, "trf on Slave %d failed:%d\n",
- msg->dev_num, ret);
+ msg->dev_num, ret);
if (msg->page)
sdw_reset_page(bus, msg->dev_num);
@@ -243,7 +244,7 @@ int sdw_transfer(struct sdw_bus *bus, struct sdw_msg *msg)
* Caller needs to hold the msg_lock lock while calling this
*/
int sdw_transfer_defer(struct sdw_bus *bus, struct sdw_msg *msg,
- struct sdw_defer *defer)
+ struct sdw_defer *defer)
{
int ret;
@@ -253,7 +254,7 @@ int sdw_transfer_defer(struct sdw_bus *bus, struct sdw_msg
*msg,
ret = do_transfer_defer(bus, msg, defer);
if (ret != 0 && ret != -ENODATA)
dev_err(bus->dev, "Defer trf on Slave %d failed:%d\n",
- msg->dev_num, ret);
+ msg->dev_num, ret);
if (msg->page)
sdw_reset_page(bus, msg->dev_num);
@@ -261,9 +262,8 @@ int sdw_transfer_defer(struct sdw_bus *bus, struct sdw_msg
*msg,
return ret;
}
-
int sdw_fill_msg(struct sdw_msg *msg, struct sdw_slave *slave,
- u32 addr, size_t count, u16 dev_num, u8 flags, u8 *buf)
+u32 addr, size_t count, u16 dev_num, u8 flags, u8 *buf)
{
memset(msg, 0, sizeof(*msg));
msg->addr = addr; /* addr is 16 bit and truncated here */
@@ -284,7 +284,7 @@ int sdw_fill_msg(struct sdw_msg *msg, struct sdw_slave
*slave,
if (addr < SDW_REG_OPTIONAL_PAGE) { /* 32k but no page */
if (slave && !slave->prop.paging_support)
return 0;
- /* no need for else as that will fall thru to paging */
+ /* no need for else as that will fall-through to paging */
}
/* paging mandatory */
@@ -323,7 +323,7 @@ int sdw_nread(struct sdw_slave *slave, u32 addr, size_t
count, u8 *val)
int ret;
ret = sdw_fill_msg(, slave, addr, count,
- slave->dev_num, SDW_MSG_FLAG_READ, val);
+ slave->dev_num,
