March 2023 - greybus-dev - lists.linaro.org

[PATCH 0/2] improve arche_platform_wd_irq() function

by Khadija Kamran

Improve arche_platform_wd_irq() function to minimize indentation and fix checkpatch issues. Khadija Kamran (2): staging: greybus: add a single exit path to arche_platform_wd_irq() staging: greybus: refactor arche_platform_wd_irq() drivers/staging/greybus/arche-platform.c | 84 ++++++++++++------------ 1 file changed, 43 insertions(+), 41 deletions(-) -- 2.34.1

1 year, 7 months

5
17
0 0

Multiple undefined configuration options are dependent in Kconfig under the v6.3-rc4 drivers directory

by 孙滢

It has been discovered that the following configuration options are undefined in the current latest version, v6.3-rc4, yet they are being relied upon by other configuration options in multiple Kconfig files: MIPS_BAIKAL_T1 is undefined, used as a 'depends on' condition in multiple files such as drivers/ata/Kconfig, drivers/hwmon/Kconfig, drivers/bus/Kconfig, and drivers/memory/Kconfig. MFD_MAX597X is undefined, used as a 'depends on' condition in Kconfig file drivers/regulator/Kconfig. MFD_SM5703 is undefined, used as a 'depends on' condition in Kconfig file drivers/regulator/Kconfig. ARCH_THUNDERBAY is undefined, used as a 'depends on' condition in Kconfig files drivers/pinctrl/Kconfig and drivers/phy/intel/Kconfig. ARCH_BCM4908 is undefined, used as a 'depends on' condition in Kconfig file drivers/leds/blink/Kconfig. MFD_TN48M_CPLD is undefined, used as a 'depends on' condition in Kconfig files drivers/gpio/Kconfig and drivers/reset/Kconfig. USB_HSIC_USB3613 is undefined, used as a 'depends on' condition in drivers/staging/greybus/Kconfig and drivers/staging/greybus/arche-platform.c. If these 7 configuration options are deprecated, it is recommended to remove the dependencies on them in the Kconfig files. If they are still useful, it is recommended to define them. Best regards, Ying Sun Pengpeng Hou Yanjie Ren

1 year, 8 months

3
2
0 0

Re: [PATCH] staging: greybus: refactor arche_platform_wd_irq() function

by Alison Schofield

On Tue, Mar 28, 2023 at 10:33:23AM +0300, Dan Carpenter wrote: > On Tue, Mar 28, 2023 at 01:18:53AM +0500, Khadija Kamran wrote: > > Refactor function by adding goto statement. This reduces the > > indentation and fixes the issue reported by checkpatch.pl script. > > > > "CHECK: line length of 101 exceeds 100 columns" > > > > Signed-off-by: Khadija Kamran <kamrankhadijadj(a)gmail.com> > > --- > > drivers/staging/greybus/arche-platform.c | 11 ++++++----- > > 1 file changed, 6 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/staging/greybus/arche-platform.c b/drivers/staging/greybus/arche-platform.c > > index fcbd5f71eff2..c7d3b6f7368f 100644 > > --- a/drivers/staging/greybus/arche-platform.c > > +++ b/drivers/staging/greybus/arche-platform.c > > @@ -178,11 +178,7 @@ static irqreturn_t arche_platform_wd_irq(int irq, void *devid) > > */ > > if (arche_pdata->wake_detect_state != > > WD_STATE_COLDBOOT_START) { > > - arche_platform_set_wake_detect_state(arche_pdata, > > - WD_STATE_COLDBOOT_TRIG); > > - spin_unlock_irqrestore(&arche_pdata->wake_lock, > > - flags); > > - return IRQ_WAKE_THREAD; > > + goto out; > > I don't like this goto. > > It suggests that calling arche_platform_set_wake_detect_state(arche_pdata, WD_STATE_COLDBOOT_TRIG); > is a part of a shared exit path when that is not true so it is sort of > lying to the reader and making things harder to understand. > > Unlocking is shared. goto unlock is fine. But that doesn't help with > the very long lines. (I am not saying that goto unlock is a worthwhile > patch to send but I would definitely have found it tolerable). > > regards, > dan carpenter Khadija, While you are reworking this, just wanted to note that, once 'out:' becomes the single exit path for the function, it won't be a lie any more. So you will have addressed Dan's concern. Alison > > > } > > } > > } > > @@ -205,6 +201,11 @@ static irqreturn_t arche_platform_wd_irq(int irq, void *devid) > > spin_unlock_irqrestore(&arche_pdata->wake_lock, flags); > > > > return IRQ_HANDLED; > > + > > +out: > > + arche_platform_set_wake_detect_state(arche_pdata, WD_STATE_COLDBOOT_TRIG); > > + spin_unlock_irqrestore(&arche_pdata->wake_lock, flags); > > + return IRQ_WAKE_THREAD; > > } > > > > /* > > -- > > 2.34.1 > > >

1 year, 9 months

2
1
0 0

Re: [PATCH] staging: greybus: refactor arche_platform_wd_irq() function

by Alison Schofield

On Tue, Mar 28, 2023 at 01:18:53AM +0500, Khadija Kamran wrote: > Refactor function by adding goto statement. This reduces the > indentation and fixes the issue reported by checkpatch.pl script. It seems our emails may have crossed on this issue. See: https://lore.kernel.org/outreachy/ZBrFqtyY%2FHydnBdT@kroah.com/T/#m5ac406a1… Alison > > "CHECK: line length of 101 exceeds 100 columns" > > Signed-off-by: Khadija Kamran <kamrankhadijadj(a)gmail.com> > --- > drivers/staging/greybus/arche-platform.c | 11 ++++++----- > 1 file changed, 6 insertions(+), 5 deletions(-) > > diff --git a/drivers/staging/greybus/arche-platform.c b/drivers/staging/greybus/arche-platform.c > index fcbd5f71eff2..c7d3b6f7368f 100644 > --- a/drivers/staging/greybus/arche-platform.c > +++ b/drivers/staging/greybus/arche-platform.c > @@ -178,11 +178,7 @@ static irqreturn_t arche_platform_wd_irq(int irq, void *devid) > */ > if (arche_pdata->wake_detect_state != > WD_STATE_COLDBOOT_START) { > - arche_platform_set_wake_detect_state(arche_pdata, > - WD_STATE_COLDBOOT_TRIG); > - spin_unlock_irqrestore(&arche_pdata->wake_lock, > - flags); > - return IRQ_WAKE_THREAD; > + goto out; > } > } > } > @@ -205,6 +201,11 @@ static irqreturn_t arche_platform_wd_irq(int irq, void *devid) > spin_unlock_irqrestore(&arche_pdata->wake_lock, flags); > > return IRQ_HANDLED; > + > +out: > + arche_platform_set_wake_detect_state(arche_pdata, WD_STATE_COLDBOOT_TRIG); > + spin_unlock_irqrestore(&arche_pdata->wake_lock, flags); > + return IRQ_WAKE_THREAD; > } > > /* > -- > 2.34.1 > >

1 year, 9 months

1
0
0 0

Re: [PATCH] staging: greybus: refactor arche_platform_wd_irq() function

by Alex Elder

On 3/27/23 3:18 PM, Khadija Kamran wrote: > Refactor function by adding goto statement. This reduces the > indentation and fixes the issue reported by checkpatch.pl script. > > "CHECK: line length of 101 exceeds 100 columns" Looking at this entire function, it seems a great deal of it has somewhat wide lines. Part of the problem is that it relies on arche_platform_set_wake_detect_state(), which is 36 characters long all by itself. In any case, the line that is identified is the widest, of course, by 10 or more characters. But changing that one line doesn't substantially improve things. I'm reluctant to suggest this, because I don't want a lot of "code churn" patches to follow based on this, but... One could rename arche_platform_set_wake_detect_state() to be just set_wake_detect_state(). It's private to its source file (arche-platform.c) and therefore the "arche_plaform_" prefix isn't really necessary. And perhaps the result would be code that is a little more readable, because its lines aren't so long. I'd be happy to hear others' thoughts on this. -Alex > > Signed-off-by: Khadija Kamran <kamrankhadijadj(a)gmail.com> > --- > drivers/staging/greybus/arche-platform.c | 11 ++++++----- > 1 file changed, 6 insertions(+), 5 deletions(-) > > diff --git a/drivers/staging/greybus/arche-platform.c b/drivers/staging/greybus/arche-platform.c > index fcbd5f71eff2..c7d3b6f7368f 100644 > --- a/drivers/staging/greybus/arche-platform.c > +++ b/drivers/staging/greybus/arche-platform.c > @@ -178,11 +178,7 @@ static irqreturn_t arche_platform_wd_irq(int irq, void *devid) > */ > if (arche_pdata->wake_detect_state != > WD_STATE_COLDBOOT_START) { > - arche_platform_set_wake_detect_state(arche_pdata, > - WD_STATE_COLDBOOT_TRIG); > - spin_unlock_irqrestore(&arche_pdata->wake_lock, > - flags); > - return IRQ_WAKE_THREAD; > + goto out; > } > } > } > @@ -205,6 +201,11 @@ static irqreturn_t arche_platform_wd_irq(int irq, void *devid) > spin_unlock_irqrestore(&arche_pdata->wake_lock, flags); > > return IRQ_HANDLED; > + > +out: > + arche_platform_set_wake_detect_state(arche_pdata, WD_STATE_COLDBOOT_TRIG); > + spin_unlock_irqrestore(&arche_pdata->wake_lock, flags); > + return IRQ_WAKE_THREAD; > } > > /*

1 year, 9 months

1
0
0 0

Re: [PATCH v3] staging: greybus: merge split lines

by Alison Schofield

On Mon, Mar 27, 2023 at 03:26:22PM +0500, Khadija Kamran wrote: > On Tue, Mar 21, 2023 at 09:35:42AM -0700, Alison Schofield wrote: > > On Tue, Mar 21, 2023 at 09:21:35PM +0500, Khadija Kamran wrote: > > > On Mon, Mar 20, 2023 at 01:26:33PM +0500, Khadija Kamran wrote: > > > > If condition and spin_unlock_...() call is split into two lines, merge > > > > them to form a single line. > > > > > > > > Suggested-by: Deepak R Varma drv(a)mailo.com > > > > Signed-off-by: Khadija Kamran <kamrankhadijadj(a)gmail.com> > > > > --- > > > > > > > > Changes in v3: > > > > - Removing tab to fix line length results in a new checkpatch warning, > > > > so let the fix length be as it is. > > > > Changes in v2: > > > > - Rephrased he subject and description > > > > - Merged if_condition() and spin_unlock...() into one line > > > > - Link to patch: > > > > https://lore.kernel.org/outreachy/ZAusnKYVTGvO5zoi@khadija-virtual-machine/ > > > > > > > > Link to first patch: > > > > https://lore.kernel.org/outreachy/ZAtkW6g6DwPg%2FpDp@khadija-virtual-machin… > > > > > > > > drivers/staging/greybus/arche-platform.c | 6 ++---- > > > > 1 file changed, 2 insertions(+), 4 deletions(-) > > > > > > > > diff --git a/drivers/staging/greybus/arche-platform.c b/drivers/staging/greybus/arche-platform.c > > > > index fcbd5f71eff2..6890710afdfc 100644 > > > > --- a/drivers/staging/greybus/arche-platform.c > > > > +++ b/drivers/staging/greybus/arche-platform.c > > > > @@ -176,12 +176,10 @@ static irqreturn_t arche_platform_wd_irq(int irq, void *devid) > > > > * Check we are not in middle of irq thread > > > > * already > > > > */ > > > > - if (arche_pdata->wake_detect_state != > > > > - WD_STATE_COLDBOOT_START) { > > > > + if (arche_pdata->wake_detect_state != WD_STATE_COLDBOOT_START) { > > > > arche_platform_set_wake_detect_state(arche_pdata, > > > > WD_STATE_COLDBOOT_TRIG); > > > > - spin_unlock_irqrestore(&arche_pdata->wake_lock, > > > > - flags); > > > > + spin_unlock_irqrestore(&arche_pdata->wake_lock, flags); > > > > return IRQ_WAKE_THREAD; > > > > } > > > > } > > > > -- > > > > 2.34.1 > > > > > > > > > > Hey Outreachy Mentors, > > > > > > Kindly take a look at this patch and let me know if it is okay to work > > > on this file or should I look for other cleanup patches. > > > > Hi Khadija, > > > > I thought you were abandoning *this* patch, and doing a refactor on > > the function. I'd expect that would be a new patch, probably a > > patchset. One where you align the work based on the 'rising' and > > 'falling' detection, > > Hey Alison, > > Can you please elaborate that what do you mean by aligning on the basis > of rising and falling detection. Are you perhaps saying that I should > group the rising detection and group the falling detection separately? > > > and perhaps a second patch that centralizes > > the unlock and return. > > To do this I should make the use of goto statement, right? > > So the next patchset should be: > Patch 1: merge split lines > Patch 2: align on the basis of rising and falling detection > Patch 3: use goto statement to centralize unlock and return > > Kindly guide me. > > Regards, > Khadija Hi, Glad you are picking this back up! I know Ira sent you some links to refactoring info. Go back and look at those. When we submit patches that refactor a function, we try to make the patches obviously correct and easy to review. I'll tell you how I approached this one, and you can see how it works for you: 1. Edit the function until it is just how you'd like it. Hint: no lines over 80, minimal indentation. { --snip-- if (!gpiod_get_value(arche_pdata->wake_detect)) goto falling; /* wake/detect rising */ falling: /* wake/detect falling */ out: spin_unlock_irqrestore(&arche_pdata->wake_lock, flags); return rc; } 2. Figure out how you can present that in patches. This function is just long enough that I think you have to split it up into two or more obvious steps, rather than throwing it into one patch. How about you do Step 1, and send the diff to the Outreachy mailing list (only) for review. Please start a new thread. Alison > > > > > Is there some other concern with working on this file? > > > > Alison > > > > > > > > Thank you for your time. > > > Regards, > > > Khadija > > > > > >

1 year, 9 months

1
0
0 0

[PATCH v4] staging: greybus: Inline pwm_chip_to_gb_pwm_chip()

by Sumitra Sharma

Convert 'pwm_chip_to_gb_pwm_chip' from a macro to a static inline function, to make the relevant types apparent in the definition and to benefit from the type checking performed by the compiler at call sites. Signed-off-by: Sumitra Sharma <sumitraartsy(a)gmail.com> --- Changes in v2: Change patch subjects, noted by Alison Schofield <alison.schofield(a)intel.com> Changes in v3: Change patch subjects to length 80 chars, noted by Alison Schofield <alison.schofield(a)intel.com> Changes in v4: Apply change to fresh git tree, noted by Greg KH <gregkh(a)linuxfoundation.org> drivers/staging/greybus/pwm.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/drivers/staging/greybus/pwm.c b/drivers/staging/greybus/pwm.c index 26d39e08c3b6..88da1d796f13 100644 --- a/drivers/staging/greybus/pwm.c +++ b/drivers/staging/greybus/pwm.c @@ -21,8 +21,11 @@ struct gb_pwm_chip { struct pwm_chip chip; struct pwm_chip *pwm; }; -#define pwm_chip_to_gb_pwm_chip(chip) \ - container_of(chip, struct gb_pwm_chip, chip) + +static inline struct gb_pwm_chip *pwm_chip_to_gb_pwm_chip(struct pwm_chip *chip) +{ + return container_of(chip, struct gb_pwm_chip, chip); +} static int gb_pwm_count_operation(struct gb_pwm_chip *pwmc) { -- 2.25.1

1 year, 9 months

1
0
0 0

[PATCH v3 0/3] staging: greybus: Use Inline fuctions

by Sumitra Sharma

Convert macros to a static inline function, to make the relevant types apparent in the definition and to benefit from the type checking performed by the compiler at call sites. Changes in v2: Change patch subjects, noted by Alison Schofield <alison.schofield(a)intel.com> Changes in v3: Change patch subjects to length 80 chars, noted by Alison Schofield <alison.schofield(a)intel.com> Sumitra Sharma (3): staging: greybus: Inline gpio_chip_to_gb_gpio_controller() staging: greybus: Inline gb_audio_manager_module() staging: greybus: Inline pwm_chip_to_gb_pwm_chip() drivers/staging/greybus/audio_manager_module.c | 7 +++++-- drivers/staging/greybus/gpio.c | 7 +++++-- drivers/staging/greybus/pwm.c | 6 ++++-- 3 files changed, 14 insertions(+), 6 deletions(-) -- 2.25.1

1 year, 9 months

3
7
0 0

[PATCH v2 0/3] Staging: greybus: Use inline functions

by Sumitra Sharma

Convert macros to a static inline function, to make the relevant types apparent in the definition and to benefit from the type checking performed by the compiler at call sites. CHanges in v2: Change patch subjects, noted by Alison Schofield <alison.schofield(a)intel.com> Sumitra Sharma (3): Staging: greybus: Use inline function for macro gpio_chip_to_gb_gpio_controller Staging: greybus: Use inline function for gb_audio_manager_module Staging: greybus: Use inline function for pwm_chip_to_gb_pwm_chip drivers/staging/greybus/audio_manager_module.c | 7 +++++-- drivers/staging/greybus/gpio.c | 7 +++++-- drivers/staging/greybus/pwm.c | 6 ++++-- 3 files changed, 14 insertions(+), 6 deletions(-) -- 2.25.1

1 year, 9 months

2
5
0 0

[PATCH 0/3] Staging: greybus: Convert macro definitions to

by Sumitra Sharma

Convert macros to a static inline function, to make the relevant types apparent in the definition and to benefit from the type checking performed by the compiler at call sites. Sumitra Sharma (3): Staging: greybus: Convert macro gpio_chip_to_gb_gpio_controller to an inline function Staging: greybus: Convert macro struct gb_audio_manager_module to an inline function Staging: greybus: Convert macro struct pwm_chip_to_gb_pwm_chip to an inline function drivers/staging/greybus/audio_manager_module.c | 7 +++++-- drivers/staging/greybus/gpio.c | 7 +++++-- drivers/staging/greybus/pwm.c | 6 ++++-- 3 files changed, 14 insertions(+), 6 deletions(-) -- 2.25.1

1 year, 9 months

2
4
0 0

2024

2023

2022

2021

2020

2019

2018

2017

2016

greybus-dev March 2023