Re: [PATCH] Drivers: bcma: Fix three coding style issues, more than 80 characters per line.

Re: [PATCH] Drivers: bcma: Fix three coding style issues, more than 80 characters per line.

[Rafał Miłecki]

On 24 December 2014 at 00:55, Oscar Forner Martinez
<oscar.forner.martinez@gmail.com> wrote:
> Three lines with more than 80 characters per line have been split in several lines.
>
> Signed-off-by: Oscar Forner Martinez <oscar.forner.martinez@gmail.com>
> ---
>  drivers/bcma/driver_chipcommon.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/bcma/driver_chipcommon.c b/drivers/bcma/driver_chipcommon.c
> index 19f6796..1956918 100644
> --- a/drivers/bcma/driver_chipcommon.c
> +++ b/drivers/bcma/driver_chipcommon.c
> @@ -79,7 +79,8 @@ static int bcma_chipco_watchdog_ticks_per_ms(struct bcma_drv_cc *cc)
>
>         if (cc->capabilities & BCMA_CC_CAP_PMU) {
>                 if (bus->chipinfo.id == BCMA_CHIP_ID_BCM4706)
> -                       /* 4706 CC and PMU watchdogs are clocked at 1/4 of ALP clock */
> +                       /* 4706 CC and PMU watchdogs are clocked */
> +                       /* at 1/4 of ALP clock */

I don't think we use comments like this. Use normal multiline comment,
also see which one is preferred for that tree.


> -               bcma_err(cc->core->bus, "serial not supported on this device ccrev: 0x%x\n", ccrev);
> +               bcma_err(cc->core->bus,
> +                        "serial not supported on this device ccrev: 0x%x\n",
> +                        ccrev);

I think some subsystems agreed to keep dev/bus pointed and string in
the one line. It makes more sense for me too.

Re: [PATCH] Drivers: bcma: Fix three coding style issues, more than 80 characters per line.

[Rafał Miłecki]

On 24 December 2014 at 00:55, Oscar Forner Martinez
<oscar.forner.martinez@gmail.com> wrote:
> Three lines with more than 80 characters per line have been split in several lines.

Since you will need V2 anyway, you may also drop this "Drivers:" prefix :)

[PATCH] Drivers: bcma: Fix three coding style issues, more than 80 characters per line.

[Oscar Forner Martinez]

Three lines with more than 80 characters per line have been split in several lines.

Signed-off-by: Oscar Forner Martinez <oscar.forner.martinez@gmail.com>
---
 drivers/bcma/driver_chipcommon.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/bcma/driver_chipcommon.c b/drivers/bcma/driver_chipcommon.c
index 19f6796..1956918 100644
--- a/drivers/bcma/driver_chipcommon.c
+++ b/drivers/bcma/driver_chipcommon.c
@@ -79,7 +79,8 @@ static int bcma_chipco_watchdog_ticks_per_ms(struct bcma_drv_cc *cc)
 
 	if (cc->capabilities & BCMA_CC_CAP_PMU) {
 		if (bus->chipinfo.id == BCMA_CHIP_ID_BCM4706)
-			/* 4706 CC and PMU watchdogs are clocked at 1/4 of ALP clock */
+			/* 4706 CC and PMU watchdogs are clocked */
+			/* at 1/4 of ALP clock */
 			return bcma_chipco_get_alp_clock(cc) / 4000;
 		else
 			/* based on 32KHz ILP clock */
@@ -97,7 +98,8 @@ int bcma_chipco_watchdog_register(struct bcma_drv_cc *cc)
 	wdt.driver_data = cc;
 	wdt.timer_set = bcma_chipco_watchdog_timer_set_wdt;
 	wdt.timer_set_ms = bcma_chipco_watchdog_timer_set_ms_wdt;
-	wdt.max_timer_ms = bcma_chipco_watchdog_get_max_timer(cc) / cc->ticks_per_ms;
+	wdt.max_timer_ms =
+		bcma_chipco_watchdog_get_max_timer(cc) / cc->ticks_per_ms;
 
 	pdev = platform_device_register_data(NULL, "bcm47xx-wdt",
 					     cc->core->bus->num, &wdt,
@@ -335,7 +337,9 @@ void bcma_chipco_serial_init(struct bcma_drv_cc *cc)
 				       | BCMA_CC_CORECTL_UARTCLKEN);
 		}
 	} else {
-		bcma_err(cc->core->bus, "serial not supported on this device ccrev: 0x%x\n", ccrev);
+		bcma_err(cc->core->bus,
+			 "serial not supported on this device ccrev: 0x%x\n",
+			 ccrev);
 		return;
 	}
 
-- 
2.2.1

Re: [PATCH] Drivers: bcma: Fix three coding style issues, more than 80 characters per line.

[Joe Perches]

On Wed, 2014-12-24 at 00:46 +0100, Rafał Miłecki wrote:
> On 24 December 2014 at 00:55, Oscar Forner Martinez
> <oscar.forner.martinez@gmail.com> wrote:
> > Three lines with more than 80 characters per line have been split in several lines.
> >
> > Signed-off-by: Oscar Forner Martinez <oscar.forner.martinez@gmail.com>
> > ---
> >  drivers/bcma/driver_chipcommon.c | 10 +++++++---
> >  1 file changed, 7 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/bcma/driver_chipcommon.c b/drivers/bcma/driver_chipcommon.c
[]
> > -               bcma_err(cc->core->bus, "serial not supported on this device ccrev: 0x%x\n", ccrev);
> > +               bcma_err(cc->core->bus,
> > +                        "serial not supported on this device ccrev: 0x%x\n",
> > +                        ccrev);
> 
> I think some subsystems agreed to keep dev/bus pointed and string in
> the one line. It makes more sense for me too.

And it would probably be better to invert the test above
and error out immediately to reduce the indentation.

Something like:

 drivers/bcma/driver_chipcommon.c | 42 +++++++++++++++++++++-------------------
 1 file changed, 22 insertions(+), 20 deletions(-)

diff --git a/drivers/bcma/driver_chipcommon.c b/drivers/bcma/driver_chipcommon.c
index 19f6796..e63fabfc 100644
--- a/drivers/bcma/driver_chipcommon.c
+++ b/drivers/bcma/driver_chipcommon.c
@@ -316,29 +316,31 @@ void bcma_chipco_serial_init(struct bcma_drv_cc *cc)
 	unsigned int ccrev = cc->core->id.rev;
 	struct bcma_serial_port *ports = cc->serial_ports;
 
-	if (ccrev >= 11 && ccrev != 15) {
-		baud_base = bcma_chipco_get_alp_clock(cc);
-		if (ccrev >= 21) {
-			/* Turn off UART clock before switching clocksource. */
-			bcma_cc_write32(cc, BCMA_CC_CORECTL,
-				       bcma_cc_read32(cc, BCMA_CC_CORECTL)
-				       & ~BCMA_CC_CORECTL_UARTCLKEN);
-		}
-		/* Set the override bit so we don't divide it */
-		bcma_cc_write32(cc, BCMA_CC_CORECTL,
-			       bcma_cc_read32(cc, BCMA_CC_CORECTL)
-			       | BCMA_CC_CORECTL_UARTCLK0);
-		if (ccrev >= 21) {
-			/* Re-enable the UART clock. */
-			bcma_cc_write32(cc, BCMA_CC_CORECTL,
-				       bcma_cc_read32(cc, BCMA_CC_CORECTL)
-				       | BCMA_CC_CORECTL_UARTCLKEN);
-		}
-	} else {
-		bcma_err(cc->core->bus, "serial not supported on this device ccrev: 0x%x\n", ccrev);
+	if (ccrev < 11 || ccrev == 15) {
+		bcma_err(cc->core->bus,
+			 "serial not supported on this device ccrev: 0x%x\n",
+			 ccrev);
 		return;
 	}
 
+	baud_base = bcma_chipco_get_alp_clock(cc);
+	if (ccrev >= 21) {
+		/* Turn off UART clock before switching clocksource */
+		bcma_cc_write32(cc, BCMA_CC_CORECTL,
+				bcma_cc_read32(cc, BCMA_CC_CORECTL)
+				& ~BCMA_CC_CORECTL_UARTCLKEN);
+	}
+	/* Set the override bit so we don't divide it */
+	bcma_cc_write32(cc, BCMA_CC_CORECTL,
+			bcma_cc_read32(cc, BCMA_CC_CORECTL) |
+			BCMA_CC_CORECTL_UARTCLK0);
+	if (ccrev >= 21) {
+		/* Re-enable the UART clock */
+		bcma_cc_write32(cc, BCMA_CC_CORECTL,
+				bcma_cc_read32(cc, BCMA_CC_CORECTL)
+				| BCMA_CC_CORECTL_UARTCLKEN);
+	}
+
 	irq = bcma_core_irq(cc->core, 0);
 
 	/* Determine the registers of the UARTs */

Re: [PATCH] Drivers: bcma: Fix three coding style issues, more than 80 characters per line.

[Kalle Valo]

Oscar Forner Martinez <oscar.forner.martinez@gmail.com> writes:

> Three lines with more than 80 characters per line have been split in several lines.
>
> Signed-off-by: Oscar Forner Martinez <oscar.forner.martinez@gmail.com>
> ---
>  drivers/bcma/driver_chipcommon.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)

Just to handle the bureaucracy before v2 is submitted:

To which tree should this go to? I see that earlier John has applied
patches to drivers/bcma/, but what about now? Should I take these? John,
any suggestions?

Oscar, the patchwork entry for this patch looked odd. I'm guessing it
was because your time (or timezone) is wrong:

https://patchwork.kernel.org/patch/5535751/

-- 
Kalle Valo

Re: [PATCH] Drivers: bcma: Fix three coding style issues, more than 80 characters per line.

[Arend van Spriel]

On 12/24/14 08:20, Kalle Valo wrote:
> Oscar Forner Martinez<oscar.forner.martinez@gmail.com>  writes:
>
>> Three lines with more than 80 characters per line have been split in several lines.
>>
>> Signed-off-by: Oscar Forner Martinez<oscar.forner.martinez@gmail.com>
>> ---
>>   drivers/bcma/driver_chipcommon.c | 10 +++++++---
>>   1 file changed, 7 insertions(+), 3 deletions(-)
>
> Just to handle the bureaucracy before v2 is submitted:
>
> To which tree should this go to? I see that earlier John has applied
> patches to drivers/bcma/, but what about now? Should I take these? John,
> any suggestions?

It is a bit of an odd ball, but there are couple of wireless driver 
relying on bcma and it is more than just a bus driver. It also does some 
steps of the device initialization (which is convenient, but not sure I 
like it). So I would hope it can stay with wireless subsystem to catch 
issue caused in that area early on.

Regards,
Arend

> Oscar, the patchwork entry for this patch looked odd. I'm guessing it
> was because your time (or timezone) is wrong:
>
> https://patchwork.kernel.org/patch/5535751/
>

Re: [PATCH] Drivers: bcma: Fix three coding style issues, more than 80 characters per line.

[Òscar Forner Martínez]

Hi Kalle,

I am doing it from London with the timezone of Spain, maybe is that the odd stuff? I can change it and do the patch again. I will check the style guide to do the changes properly this time. I have a couple of questions, as it is my first patch I am not sure how to do them.

Do I have to sent the V2 of the patch as the previous one? or I have to do something to follow that one?

Regards,

Oscar

On 24 Dec 2014, at 07:20, Kalle Valo <kvalo@codeaurora.org> wrote:

> Oscar Forner Martinez <oscar.forner.martinez@gmail.com> writes:
> 
>> Three lines with more than 80 characters per line have been split in several lines.
>> 
>> Signed-off-by: Oscar Forner Martinez <oscar.forner.martinez@gmail.com>
>> ---
>> drivers/bcma/driver_chipcommon.c | 10 +++++++---
>> 1 file changed, 7 insertions(+), 3 deletions(-)
> 
> Just to handle the bureaucracy before v2 is submitted:
> 
> To which tree should this go to? I see that earlier John has applied
> patches to drivers/bcma/, but what about now? Should I take these? John,
> any suggestions?
> 
> Oscar, the patchwork entry for this patch looked odd. I'm guessing it
> was because your time (or timezone) is wrong:
> 
> https://patchwork.kernel.org/patch/5535751/
> 
> -- 
> Kalle Valo

Re: [PATCH] Drivers: bcma: Fix three coding style issues, more than 80 characters per line.

[Rafał Miłecki]

On 24 December 2014 at 08:20, Kalle Valo <kvalo@codeaurora.org> wrote:
> Oscar Forner Martinez <oscar.forner.martinez@gmail.com> writes:
>
>> Three lines with more than 80 characters per line have been split in several lines.
>>
>> Signed-off-by: Oscar Forner Martinez <oscar.forner.martinez@gmail.com>
>> ---
>>  drivers/bcma/driver_chipcommon.c | 10 +++++++---
>>  1 file changed, 7 insertions(+), 3 deletions(-)
>
> Just to handle the bureaucracy before v2 is submitted:
>
> To which tree should this go to? I see that earlier John has applied
> patches to drivers/bcma/, but what about now? Should I take these? John,
> any suggestions?

bcma is a bus with multiple cores (devices)

As some of cores (devices) are very simply to handle and/or they are
needed very early (e.g. for initialization), bcma has:
1) Few bult-in drivers (like MIPS, ChipCommon)
2) Few "external" drivers using standard bus mechanism (e.g. b43,
brcmsmac, bgmac)

I guess the biggest bcma users are wireless drivers, so bcma was also
maintained using wireless tree. Unfortunately bus can't be just setup
once and not touched anymore. Drivers often need to touch ChipCommon
so there are some export_symbol-s in driver_chipcommon.c and you also
can get patchset-s touching bcma and then wireless driver.

Of course there are some problems with such maintenance. Rarely I've
to do changes in arch/mips/bcm47xx/ code, wait for a release and then
change sth in drivers/bcma. But I don't see a better solution for all
of this.

-- 
Rafał