Re: [PATCH] staging: mt7621-gpio: avoid use of globals and use a drvdata allocated structure
On Wed, May 16, 2018 at 09:59:09AM +1000, NeilBrown wrote:
> On Tue, May 15 2018, Sergio Paracuellos wrote:
>
> > Gpio driver have a some globals which can be avoided just
> > using platform_data in a proper form. This commit adds a new
> > struct mtk_data which includes all of those globals setting them
> > using platform_set_drvdata and devm_gpiochip_add_data functions.
> > With this properly set we are able to retrieve driver data along
> > the code using kernel api's so globals are not needed anymore.
> >
> > Signed-off-by: Sergio Paracuellos
> > ---
> >
> > I have not tested this because I don't hardware to do it but I
> > have compile it just to be sure it compiles. This patch needs
>
> I've tested it. Doesn't work :-( Close though.
> mtk_gpio_w32() needs the gpio_data, so it cannot be called before
> devm_gpiochip_add_data() is called.
> This:
>
> diff --git a/drivers/staging/mt7621-gpio/gpio-mt7621.c
> b/drivers/staging/mt7621-gpio/gpio-mt7621.c
> index be27cbdcbfa8..48141d979059 100644
> --- a/drivers/staging/mt7621-gpio/gpio-mt7621.c
> +++ b/drivers/staging/mt7621-gpio/gpio-mt7621.c
> @@ -185,9 +185,6 @@ mediatek_gpio_bank_probe(struct platform_device *pdev,
> struct device_node *bank)
> rg->chip.to_irq = mediatek_gpio_to_irq;
> rg->bank = be32_to_cpu(*id);
>
> - /* set polarity to low for all gpios */
> - mtk_gpio_w32(rg, GPIO_REG_POL, 0);
> -
> ret = devm_gpiochip_add_data(&pdev->dev, &rg->chip, gpio_data);
> if (ret < 0) {
> dev_err(&pdev->dev, "Could not register gpio %d, ret=%d\n",
> @@ -195,6 +192,9 @@ mediatek_gpio_bank_probe(struct platform_device *pdev,
> struct device_node *bank)
> return ret;
> }
>
> + /* set polarity to low for all gpios */
> + mtk_gpio_w32(rg, GPIO_REG_POL, 0);
> +
> dev_info(&pdev->dev, "registering %d gpios\n", rg->chip.ngpio);
>
> return gpiochip_add(&rg->chip);
>
>
> makes it work. It probably won't work once I try interrupts as
> mediatek_gpio_to_irq() also needs gpio_data, and that is currently
> called early.
>
> Also:
>
> >
> > static inline u32
> > mtk_gpio_r32(struct mtk_gc *rg, u8 reg)
> > {
> > + struct mtk_data *gpio_data = gpiochip_get_data(&rg->chip);
> > u32 offset = (reg * 0x10) + (rg->bank * 0x4);
> >
> > - return ioread32(mediatek_gpio_membase + offset);
> > + return ioread32(gpio_data->gpio_membase + offset);
> > }
> >
>
> This hunk doesn't apply.
> The existing code is:
>
> static inline u32
> mtk_gpio_r32(struct mtk_gc *rg, u8 reg)
> {
>return ioread32(mediatek_gpio_membase + (reg * 0x10) + (rg->bank *
> 0x4));
> }
>
> Thanks!
>
> NeilBrown
I have just sent v2 with things you pointed out here probably fixed and other
remaining
cleanups pointed in this thread.
Hope this helps,
Best regards,
Sergio Paracuellos
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v2 04/11] staging: mt7621-gpio: replace 'mtk' to use correct one 'mediatek'
Gpio driver is using mtk and there is already 'mediatek' binding
defined for this maker. Update driver to use it instead the custom
one 'mtk'.
Signed-off-by: Sergio Paracuellos
---
drivers/staging/mt7621-gpio/gpio-mt7621.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/staging/mt7621-gpio/gpio-mt7621.c
b/drivers/staging/mt7621-gpio/gpio-mt7621.c
index a577381..7d17949 100644
--- a/drivers/staging/mt7621-gpio/gpio-mt7621.c
+++ b/drivers/staging/mt7621-gpio/gpio-mt7621.c
@@ -323,7 +323,7 @@ mediatek_gpio_probe(struct platform_device *pdev)
}
for_each_child_of_node(np, bank)
- if (of_device_is_compatible(bank, "mtk,mt7621-gpio-bank"))
+ if (of_device_is_compatible(bank, "mediatek,mt7621-gpio-bank"))
mediatek_gpio_bank_probe(pdev, bank);
if (mediatek_gpio_irq_domain)
@@ -334,7 +334,7 @@ mediatek_gpio_probe(struct platform_device *pdev)
}
static const struct of_device_id mediatek_gpio_match[] = {
- { .compatible = "mtk,mt7621-gpio" },
+ { .compatible = "mediatek,mt7621-gpio" },
{},
};
MODULE_DEVICE_TABLE(of, mediatek_gpio_match);
--
2.7.4
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v2 07/11] staging: mt7621-gpio: dt-bindings: add interrupt nodes to bindings doc
Interrupt related stuff for gpio controller in mt7621 was missing
in device tree documentation. Add it to complete documentation for
this driver.
Signed-off-by: Sergio Paracuellos
---
drivers/staging/mt7621-gpio/mediatek,mt7621-gpio.txt | 9 -
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/drivers/staging/mt7621-gpio/mediatek,mt7621-gpio.txt
b/drivers/staging/mt7621-gpio/mediatek,mt7621-gpio.txt
index 54de9f7..af64092 100644
--- a/drivers/staging/mt7621-gpio/mediatek,mt7621-gpio.txt
+++ b/drivers/staging/mt7621-gpio/mediatek,mt7621-gpio.txt
@@ -4,12 +4,16 @@ The IP core used inside these SoCs has 3 banks of 32 GPIOs
each.
The registers of all the banks are interwoven inside one single IO range.
We load one GPIO controller instance per bank. To make this possible
we support 2 types of nodes. The parent node defines the memory I/O range and
-has 3 children each describing a single bank.
+has 3 children each describing a single bank. Also the GPIO controller can
receive
+interrupts on any of the GPIOs, either edge or level. It then interrupts the
CPU
+using GIC INT12.
Required properties for the top level node:
- compatible:
- "mediatek,mt7621-gpio" for Mediatek controllers
- reg : Physical base address and length of the controller's registers
+- interrupt-parent : phandle of the parent interrupt controller.
+- interrupts = Interrupt specifier for the controllers interrupt
Required properties for the GPIO bank node:
- compatible:
@@ -28,6 +32,9 @@ Example:
compatible = "mediatek,mt7621-gpio";
reg = <0x600 0x100>;
+ interrupt-parent = <&gic>;
+ interrupts = ;
+
gpio0: bank@0 {
reg = <0>;
compatible = "mediatek,mt7621-gpio-bank";
--
2.7.4
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v2 03/11] staging: mt7621-dts: update gpios related entries to use 'mediatek'
Gpio driver for mt7621 is using 'mtk' as binding but in the kernel
is already defined one for this maker which is 'mediatek'. Update
device tree to use the correct one.
Signed-off-by: Sergio Paracuellos
---
drivers/staging/mt7621-dts/mt7621.dtsi | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/staging/mt7621-dts/mt7621.dtsi
b/drivers/staging/mt7621-dts/mt7621.dtsi
index 9d941b5..115eb04 100644
--- a/drivers/staging/mt7621-dts/mt7621.dtsi
+++ b/drivers/staging/mt7621-dts/mt7621.dtsi
@@ -64,26 +64,26 @@
#address-cells = <1>;
#size-cells = <0>;
- compatible = "mtk,mt7621-gpio";
+ compatible = "mediatek,mt7621-gpio";
reg = <0x600 0x100>;
gpio0: bank@0 {
reg = <0>;
- compatible = "mtk,mt7621-gpio-bank";
+ compatible = "mediatek,mt7621-gpio-bank";
gpio-controller;
#gpio-cells = <2>;
};
gpio1: bank@1 {
reg = <1>;
- compatible = "mtk,mt7621-gpio-bank";
+ compatible = "mediatek,mt7621-gpio-bank";
gpio-controller;
#gpio-cells = <2>;
};
gpio2: bank@2 {
reg = <2>;
- compatible = "mtk,mt7621-gpio-bank";
+ compatible = "mediatek,mt7621-gpio-bank";
gpio-controller;
#gpio-cells = <2>;
};
--
2.7.4
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v2 09/11] staging: mt7621-gpio: use ternary operator in return in mediatek_gpio_get_direction
This commits replaces if statement and two returns in favour of a only one return using a ternary operator. Signed-off-by: Sergio Paracuellos --- drivers/staging/mt7621-gpio/gpio-mt7621.c | 5 + 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/drivers/staging/mt7621-gpio/gpio-mt7621.c b/drivers/staging/mt7621-gpio/gpio-mt7621.c index 2d16d62..379efc4 100644 --- a/drivers/staging/mt7621-gpio/gpio-mt7621.c +++ b/drivers/staging/mt7621-gpio/gpio-mt7621.c @@ -135,10 +135,7 @@ mediatek_gpio_get_direction(struct gpio_chip *chip, unsigned int offset) t = mtk_gpio_r32(rg, GPIO_REG_CTRL); spin_unlock_irqrestore(&rg->lock, flags); - if (t & BIT(offset)) - return 0; - - return 1; + return (t & BIT(offset)) ? 0 : 1; } static int -- 2.7.4 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v2 02/11] staging: mt7621-gpio: update TODO file
This commit updates TODO file with missing things to get this driver ready to be mainlined. Signed-off-by: Sergio Paracuellos --- drivers/staging/mt7621-gpio/TODO | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/drivers/staging/mt7621-gpio/TODO b/drivers/staging/mt7621-gpio/TODO index 7143905..9077b16 100644 --- a/drivers/staging/mt7621-gpio/TODO +++ b/drivers/staging/mt7621-gpio/TODO @@ -1,5 +1,7 @@ -- general code review and clean up +- general code review and clean up +- avoid global variables and use drvdata allocated instead - ensure device-tree requirements are documented +- make sure interrupts work Cc: NeilBrown -- 2.7.4 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v2 11/11] staging: mt7621-gpio: update TODO list
Some of the remaining stuff included in TODO list have been complete. So update this file accordly. Signed-off-by: Sergio Paracuellos --- drivers/staging/mt7621-gpio/TODO | 3 --- 1 file changed, 3 deletions(-) diff --git a/drivers/staging/mt7621-gpio/TODO b/drivers/staging/mt7621-gpio/TODO index 9077b16..9089833 100644 --- a/drivers/staging/mt7621-gpio/TODO +++ b/drivers/staging/mt7621-gpio/TODO @@ -1,7 +1,4 @@ -- general code review and clean up -- avoid global variables and use drvdata allocated instead -- ensure device-tree requirements are documented - make sure interrupts work Cc: NeilBrown -- 2.7.4 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v2 10/11] staging: mt7621-gpio: use MTK_BANK_WIDTH instead of magic number
There are some places where magic number '32' is being used to get the gpio bank. There already exist a definition MTK_BANK_WIDTH with this value, so just use it instead. Signed-off-by: Sergio Paracuellos --- drivers/staging/mt7621-gpio/gpio-mt7621.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/drivers/staging/mt7621-gpio/gpio-mt7621.c b/drivers/staging/mt7621-gpio/gpio-mt7621.c index 379efc4..9124c1d 100644 --- a/drivers/staging/mt7621-gpio/gpio-mt7621.c +++ b/drivers/staging/mt7621-gpio/gpio-mt7621.c @@ -229,7 +229,7 @@ mediatek_gpio_irq_unmask(struct irq_data *d) struct gpio_chip *gc = irq_data_get_irq_chip_data(d); struct mtk_data *gpio_data = gpiochip_get_data(gc); int pin = d->hwirq; - int bank = pin / 32; + int bank = pin / MTK_BANK_WIDTH; struct mtk_gc *rg = gpio_data->gc_map[bank]; unsigned long flags; u32 rise, fall; @@ -252,7 +252,7 @@ mediatek_gpio_irq_mask(struct irq_data *d) struct gpio_chip *gc = irq_data_get_irq_chip_data(d); struct mtk_data *gpio_data = gpiochip_get_data(gc); int pin = d->hwirq; - int bank = pin / 32; + int bank = pin / MTK_BANK_WIDTH; struct mtk_gc *rg = gpio_data->gc_map[bank]; unsigned long flags; u32 rise, fall; @@ -275,7 +275,7 @@ mediatek_gpio_irq_type(struct irq_data *d, unsigned int type) struct gpio_chip *gc = irq_data_get_irq_chip_data(d); struct mtk_data *gpio_data = gpiochip_get_data(gc); int pin = d->hwirq; - int bank = pin / 32; + int bank = pin / MTK_BANK_WIDTH; struct mtk_gc *rg = gpio_data->gc_map[bank]; u32 mask = BIT(d->hwirq); -- 2.7.4 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v2 00/11] staging: mt7621-gpio: use mediatek as binding instead of custom mtk
The following series updated mt7621-gpio driver to use 'mediatek'
which is already defined in kernel bindings documentation instead
of add a new custom one 'mtk' for this company. mt7621-gpio device-tree
documentation has been added also to the staging driver directory.
v2:
- Join some patch ("staging: mt7621-gpio: avoid use of globals
and use platform_data instead") with the needed fixes to make
it work pointed out by NeilBrown
- Add gpio interrupts to documentation and device tree in order to
can try them
- Other minor code cleanups have been added also.
After this changes only testing interrupts should be remaining to
get this out of staging.
Hope this helps.
Best regards,
Sergio
Sergio Paracuellos (11):
staging: mt7621-gpio: dt-bindings: add documentation for mt7621-gpio
staging: mt7621-gpio: update TODO file
staging: mt7621-dts: update gpios related entries to use 'mediatek'
staging: mt7621-gpio: replace 'mtk' to use correct one 'mediatek'
staging: mt7621-gpio: avoid use of globals and use platform_data
instead
staging: mt7621-dts: add interrupt device tree nodes for the gpio
controller
staging: mt7621-gpio: dt-bindings: add interrupt nodes to bindings doc
staging: mt7621-gpio: avoid devm_kzalloc() hidden inside declarations
staging: mt7621-gpio: use ternary operator in return in
mediatek_gpio_get_direction
staging: mt7621-gpio: use MTK_BANK_WIDTH instead of magic number
staging: mt7621-gpio: update TODO list
drivers/staging/mt7621-dts/mt7621.dtsi | 11 +-
drivers/staging/mt7621-gpio/TODO | 3 +-
drivers/staging/mt7621-gpio/gpio-mt7621.c | 111 +
.../staging/mt7621-gpio/mediatek,mt7621-gpio.txt | 58 +++
4 files changed, 137 insertions(+), 46 deletions(-)
create mode 100644 drivers/staging/mt7621-gpio/mediatek,mt7621-gpio.txt
--
2.7.4
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v2 01/11] staging: mt7621-gpio: dt-bindings: add documentation for mt7621-gpio
This commit add missing dt bindings documentation for mt7621-gpio
driver. There is some missing stuff here about interrupts with is
not also being used in the mt7621.dtsi file. So just include in
staging a incomplete version before moving this to kernel's dt-bindings
place.
Signed-off-by: Sergio Paracuellos
---
.../staging/mt7621-gpio/mediatek,mt7621-gpio.txt | 51 ++
1 file changed, 51 insertions(+)
create mode 100644 drivers/staging/mt7621-gpio/mediatek,mt7621-gpio.txt
diff --git a/drivers/staging/mt7621-gpio/mediatek,mt7621-gpio.txt
b/drivers/staging/mt7621-gpio/mediatek,mt7621-gpio.txt
new file mode 100644
index 000..54de9f7
--- /dev/null
+++ b/drivers/staging/mt7621-gpio/mediatek,mt7621-gpio.txt
@@ -0,0 +1,51 @@
+Mediatek SoC GPIO controller bindings
+
+The IP core used inside these SoCs has 3 banks of 32 GPIOs each.
+The registers of all the banks are interwoven inside one single IO range.
+We load one GPIO controller instance per bank. To make this possible
+we support 2 types of nodes. The parent node defines the memory I/O range and
+has 3 children each describing a single bank.
+
+Required properties for the top level node:
+- compatible:
+ - "mediatek,mt7621-gpio" for Mediatek controllers
+- reg : Physical base address and length of the controller's registers
+
+Required properties for the GPIO bank node:
+- compatible:
+ - "mediatek,mt7621-gpio-bank" for Mediatek banks
+- #gpio-cells : Should be two.
+ - first cell is the pin number
+ - second cell is used to specify optional parameters (unused)
+- gpio-controller : Marks the device node as a GPIO controller
+- reg : The id of the bank that the node describes.
+
+Example:
+ gpio@600 {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ compatible = "mediatek,mt7621-gpio";
+ reg = <0x600 0x100>;
+
+ gpio0: bank@0 {
+ reg = <0>;
+ compatible = "mediatek,mt7621-gpio-bank";
+ gpio-controller;
+ #gpio-cells = <2>;
+ };
+
+ gpio1: bank@1 {
+ reg = <1>;
+ compatible = "mediatek,mt7621-gpio-bank";
+ gpio-controller;
+ #gpio-cells = <2>;
+ };
+
+ gpio2: bank@2 {
+ reg = <2>;
+ compatible = "mediatek,mt7621-gpio-bank";
+ gpio-controller;
+ #gpio-cells = <2>;
+ };
+ };
--
2.7.4
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v2 08/11] staging: mt7621-gpio: avoid devm_kzalloc() hidden inside declarations
Driver probe function includes a allocation using devm_kzalloc
which is "hidden" a bit inside the declarations. Extract this
to a better place to increase readability. Also because we are
allocating zeroed memory 'memset' statement is not needed at all.
Signed-off-by: Sergio Paracuellos
---
drivers/staging/mt7621-gpio/gpio-mt7621.c | 11 ++-
1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/drivers/staging/mt7621-gpio/gpio-mt7621.c
b/drivers/staging/mt7621-gpio/gpio-mt7621.c
index c701259..2d16d62 100644
--- a/drivers/staging/mt7621-gpio/gpio-mt7621.c
+++ b/drivers/staging/mt7621-gpio/gpio-mt7621.c
@@ -156,17 +156,18 @@ mediatek_gpio_bank_probe(struct platform_device *pdev,
struct device_node *bank)
{
struct mtk_data *gpio_data = dev_get_drvdata(&pdev->dev);
const __be32 *id = of_get_property(bank, "reg", NULL);
- struct mtk_gc *rg = devm_kzalloc(&pdev->dev,
- sizeof(struct mtk_gc), GFP_KERNEL);
+ struct mtk_gc *rg;
int ret;
- if (!rg || !id || be32_to_cpu(*id) > MTK_MAX_BANK)
+ if (!id || be32_to_cpu(*id) > MTK_MAX_BANK)
+ return -EINVAL;
+
+ rg = devm_kzalloc(&pdev->dev, sizeof(struct mtk_gc), GFP_KERNEL);
+ if (!rg)
return -ENOMEM;
gpio_data->gc_map[be32_to_cpu(*id)] = rg;
- memset(rg, 0, sizeof(struct mtk_gc));
-
spin_lock_init(&rg->lock);
rg->chip.parent = &pdev->dev;
--
2.7.4
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v2 06/11] staging: mt7621-dts: add interrupt device tree nodes for the gpio controller
The GPIO controller of mt7621 can receive interrupts on any
of the GPIOs, either edge or level. It then interrupts the CPU using
GIC INT12. Update device tree accordly.
Signed-off-by: Sergio Paracuellos
---
drivers/staging/mt7621-dts/mt7621.dtsi | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/staging/mt7621-dts/mt7621.dtsi
b/drivers/staging/mt7621-dts/mt7621.dtsi
index 115eb04..240d396 100644
--- a/drivers/staging/mt7621-dts/mt7621.dtsi
+++ b/drivers/staging/mt7621-dts/mt7621.dtsi
@@ -67,6 +67,9 @@
compatible = "mediatek,mt7621-gpio";
reg = <0x600 0x100>;
+ interrupt-parent = <&gic>;
+ interrupts = ;
+
gpio0: bank@0 {
reg = <0>;
compatible = "mediatek,mt7621-gpio-bank";
--
2.7.4
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v2 05/11] staging: mt7621-gpio: avoid use of globals and use platform_data instead
Gpio driver have a some globals which can be avoided just
using platform_data in a proper form. This commit adds a new
struct mtk_data which includes all of those globals setting them
using platform_set_drvdata and devm_gpiochip_add_data functions.
With this properly set we are able to retrieve driver data along
the code using kernel api's so globals are not needed anymore.
Signed-off-by: Sergio Paracuellos
---
drivers/staging/mt7621-gpio/gpio-mt7621.c | 85 +--
1 file changed, 59 insertions(+), 26 deletions(-)
diff --git a/drivers/staging/mt7621-gpio/gpio-mt7621.c
b/drivers/staging/mt7621-gpio/gpio-mt7621.c
index 7d17949..c701259 100644
--- a/drivers/staging/mt7621-gpio/gpio-mt7621.c
+++ b/drivers/staging/mt7621-gpio/gpio-mt7621.c
@@ -31,17 +31,20 @@ enum mediatek_gpio_reg {
GPIO_REG_EDGE,
};
-static void __iomem *mediatek_gpio_membase;
-static int mediatek_gpio_irq;
-static struct irq_domain *mediatek_gpio_irq_domain;
-
-static struct mtk_gc {
+struct mtk_gc {
struct gpio_chip chip;
spinlock_t lock;
int bank;
u32 rising;
u32 falling;
-} *gc_map[MTK_MAX_BANK];
+};
+
+struct mtk_data {
+ void __iomem *gpio_membase;
+ int gpio_irq;
+ struct irq_domain *gpio_irq_domain;
+ struct mtk_gc *gc_map[MTK_MAX_BANK];
+};
static inline struct mtk_gc
*to_mediatek_gpio(struct gpio_chip *chip)
@@ -56,15 +59,19 @@ static inline struct mtk_gc
static inline void
mtk_gpio_w32(struct mtk_gc *rg, u8 reg, u32 val)
{
- iowrite32(val, mediatek_gpio_membase + (reg * 0x10) + (rg->bank * 0x4));
+ struct mtk_data *gpio_data = gpiochip_get_data(&rg->chip);
+ u32 offset = (reg * 0x10) + (rg->bank * 0x4);
+
+ iowrite32(val, gpio_data->gpio_membase + offset);
}
static inline u32
mtk_gpio_r32(struct mtk_gc *rg, u8 reg)
{
+ struct mtk_data *gpio_data = gpiochip_get_data(&rg->chip);
u32 offset = (reg * 0x10) + (rg->bank * 0x4);
- return ioread32(mediatek_gpio_membase + offset);
+ return ioread32(gpio_data->gpio_membase + offset);
}
static void
@@ -137,23 +144,26 @@ mediatek_gpio_get_direction(struct gpio_chip *chip,
unsigned int offset)
static int
mediatek_gpio_to_irq(struct gpio_chip *chip, unsigned int pin)
{
+ struct mtk_data *gpio_data = gpiochip_get_data(chip);
struct mtk_gc *rg = to_mediatek_gpio(chip);
- return irq_create_mapping(mediatek_gpio_irq_domain,
+ return irq_create_mapping(gpio_data->gpio_irq_domain,
pin + (rg->bank * MTK_BANK_WIDTH));
}
static int
mediatek_gpio_bank_probe(struct platform_device *pdev, struct device_node
*bank)
{
+ struct mtk_data *gpio_data = dev_get_drvdata(&pdev->dev);
const __be32 *id = of_get_property(bank, "reg", NULL);
struct mtk_gc *rg = devm_kzalloc(&pdev->dev,
sizeof(struct mtk_gc), GFP_KERNEL);
+ int ret;
if (!rg || !id || be32_to_cpu(*id) > MTK_MAX_BANK)
return -ENOMEM;
- gc_map[be32_to_cpu(*id)] = rg;
+ gpio_data->gc_map[be32_to_cpu(*id)] = rg;
memset(rg, 0, sizeof(struct mtk_gc));
@@ -169,10 +179,17 @@ mediatek_gpio_bank_probe(struct platform_device *pdev,
struct device_node *bank)
rg->chip.get_direction = mediatek_gpio_get_direction;
rg->chip.get = mediatek_gpio_get;
rg->chip.set = mediatek_gpio_set;
- if (mediatek_gpio_irq_domain)
+ if (gpio_data->gpio_irq_domain)
rg->chip.to_irq = mediatek_gpio_to_irq;
rg->bank = be32_to_cpu(*id);
+ ret = devm_gpiochip_add_data(&pdev->dev, &rg->chip, gpio_data);
+ if (ret < 0) {
+ dev_err(&pdev->dev, "Could not register gpio %d, ret=%d\n",
+ rg->chip.ngpio, ret);
+ return ret;
+ }
+
/* set polarity to low for all gpios */
mtk_gpio_w32(rg, GPIO_REG_POL, 0);
@@ -184,10 +201,12 @@ mediatek_gpio_bank_probe(struct platform_device *pdev,
struct device_node *bank)
static void
mediatek_gpio_irq_handler(struct irq_desc *desc)
{
+ struct gpio_chip *gc = irq_desc_get_handler_data(desc);
+ struct mtk_data *gpio_data = gpiochip_get_data(gc);
int i;
for (i = 0; i < MTK_MAX_BANK; i++) {
- struct mtk_gc *rg = gc_map[i];
+ struct mtk_gc *rg = gpio_data->gc_map[i];
unsigned long pending;
int bit;
@@ -197,7 +216,7 @@ mediatek_gpio_irq_handler(struct irq_desc *desc)
pending = mtk_gpio_r32(rg, GPIO_REG_STAT);
for_each_set_bit(bit, &pending, MTK_BANK_WIDTH) {
- u32 map = irq_find_mapping(mediatek_gpio_irq_domain,
+ u32 map = irq_find_mapping(gpio_data->gpio_irq_domain,
(MTK_BANK_WIDTH * i) + bit);
generic_handle_irq(map);
@@ -209,9
Re: [PATCH] staging: mt7621-gpio: avoid use of globals and use a drvdata allocated structure
On Wed, May 16, 2018 at 09:59:09AM +1000, NeilBrown wrote:
> On Tue, May 15 2018, Sergio Paracuellos wrote:
>
> > Gpio driver have a some globals which can be avoided just
> > using platform_data in a proper form. This commit adds a new
> > struct mtk_data which includes all of those globals setting them
> > using platform_set_drvdata and devm_gpiochip_add_data functions.
> > With this properly set we are able to retrieve driver data along
> > the code using kernel api's so globals are not needed anymore.
> >
> > Signed-off-by: Sergio Paracuellos
> > ---
> >
> > I have not tested this because I don't hardware to do it but I
> > have compile it just to be sure it compiles. This patch needs
>
> I've tested it. Doesn't work :-( Close though.
> mtk_gpio_w32() needs the gpio_data, so it cannot be called before
> devm_gpiochip_add_data() is called.
> This:
>
> diff --git a/drivers/staging/mt7621-gpio/gpio-mt7621.c
> b/drivers/staging/mt7621-gpio/gpio-mt7621.c
> index be27cbdcbfa8..48141d979059 100644
> --- a/drivers/staging/mt7621-gpio/gpio-mt7621.c
> +++ b/drivers/staging/mt7621-gpio/gpio-mt7621.c
> @@ -185,9 +185,6 @@ mediatek_gpio_bank_probe(struct platform_device *pdev,
> struct device_node *bank)
> rg->chip.to_irq = mediatek_gpio_to_irq;
> rg->bank = be32_to_cpu(*id);
>
> - /* set polarity to low for all gpios */
> - mtk_gpio_w32(rg, GPIO_REG_POL, 0);
> -
> ret = devm_gpiochip_add_data(&pdev->dev, &rg->chip, gpio_data);
> if (ret < 0) {
> dev_err(&pdev->dev, "Could not register gpio %d, ret=%d\n",
> @@ -195,6 +192,9 @@ mediatek_gpio_bank_probe(struct platform_device *pdev,
> struct device_node *bank)
> return ret;
> }
>
> + /* set polarity to low for all gpios */
> + mtk_gpio_w32(rg, GPIO_REG_POL, 0);
> +
> dev_info(&pdev->dev, "registering %d gpios\n", rg->chip.ngpio);
>
> return gpiochip_add(&rg->chip);
>
>
> makes it work. It probably won't work once I try interrupts as
> mediatek_gpio_to_irq() also needs gpio_data, and that is currently
> called early.
True. I'll change this and send v2.
>
> Also:
>
> >
> > static inline u32
> > mtk_gpio_r32(struct mtk_gc *rg, u8 reg)
> > {
> > + struct mtk_data *gpio_data = gpiochip_get_data(&rg->chip);
> > u32 offset = (reg * 0x10) + (rg->bank * 0x4);
> >
> > - return ioread32(mediatek_gpio_membase + offset);
> > + return ioread32(gpio_data->gpio_membase + offset);
> > }
> >
>
> This hunk doesn't apply.
> The existing code is:
>
> static inline u32
> mtk_gpio_r32(struct mtk_gc *rg, u8 reg)
> {
>return ioread32(mediatek_gpio_membase + (reg * 0x10) + (rg->bank *
> 0x4));
> }
Mmmm... This is rebased on the top of staging-next and the existing code there
is not the one you are pointing out here. The only different code between
this patch and the existing code is the use of 'mediatek' instead of 'mtk' for
the bindings which are in my previous patch series which haven't be applied
yet. Should I join all of them with this stuff fixed and send them all again for
your better review?
Best regards,
Sergio Paracuellos
>
> Thanks!
>
> NeilBrown
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: mt7621-gpio: avoid use of globals and use a drvdata allocated structure
On Tue, May 15 2018, Sergio Paracuellos wrote:
> Gpio driver have a some globals which can be avoided just
> using platform_data in a proper form. This commit adds a new
> struct mtk_data which includes all of those globals setting them
> using platform_set_drvdata and devm_gpiochip_add_data functions.
> With this properly set we are able to retrieve driver data along
> the code using kernel api's so globals are not needed anymore.
>
> Signed-off-by: Sergio Paracuellos
> ---
>
> I have not tested this because I don't hardware to do it but I
> have compile it just to be sure it compiles. This patch needs
I've tested it. Doesn't work :-( Close though.
mtk_gpio_w32() needs the gpio_data, so it cannot be called before
devm_gpiochip_add_data() is called.
This:
diff --git a/drivers/staging/mt7621-gpio/gpio-mt7621.c
b/drivers/staging/mt7621-gpio/gpio-mt7621.c
index be27cbdcbfa8..48141d979059 100644
--- a/drivers/staging/mt7621-gpio/gpio-mt7621.c
+++ b/drivers/staging/mt7621-gpio/gpio-mt7621.c
@@ -185,9 +185,6 @@ mediatek_gpio_bank_probe(struct platform_device *pdev,
struct device_node *bank)
rg->chip.to_irq = mediatek_gpio_to_irq;
rg->bank = be32_to_cpu(*id);
- /* set polarity to low for all gpios */
- mtk_gpio_w32(rg, GPIO_REG_POL, 0);
-
ret = devm_gpiochip_add_data(&pdev->dev, &rg->chip, gpio_data);
if (ret < 0) {
dev_err(&pdev->dev, "Could not register gpio %d, ret=%d\n",
@@ -195,6 +192,9 @@ mediatek_gpio_bank_probe(struct platform_device *pdev,
struct device_node *bank)
return ret;
}
+ /* set polarity to low for all gpios */
+ mtk_gpio_w32(rg, GPIO_REG_POL, 0);
+
dev_info(&pdev->dev, "registering %d gpios\n", rg->chip.ngpio);
return gpiochip_add(&rg->chip);
makes it work. It probably won't work once I try interrupts as
mediatek_gpio_to_irq() also needs gpio_data, and that is currently
called early.
Also:
>
> static inline u32
> mtk_gpio_r32(struct mtk_gc *rg, u8 reg)
> {
> + struct mtk_data *gpio_data = gpiochip_get_data(&rg->chip);
> u32 offset = (reg * 0x10) + (rg->bank * 0x4);
>
> - return ioread32(mediatek_gpio_membase + offset);
> + return ioread32(gpio_data->gpio_membase + offset);
> }
>
This hunk doesn't apply.
The existing code is:
static inline u32
mtk_gpio_r32(struct mtk_gc *rg, u8 reg)
{
return ioread32(mediatek_gpio_membase + (reg * 0x10) + (rg->bank * 0x4));
}
Thanks!
NeilBrown
signature.asc
Description: PGP signature
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 11/23] staging: ks7010: change some cast type from uint16_t to u16 in hostif_data_request
On Tue, May 15, 2018 at 3:59 PM, Dan Carpenter wrote: > We should remove a whole ball of casting really. I will. > > regards, > dan carpenter > Best regards, Sergio Paracuellos ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: mt7621-gpio: avoid use of globals and use a drvdata allocated structure
On Tue, May 15, 2018 at 4:45 PM, Dan Carpenter wrote:
> Not related to your patch...
>
> On Tue, May 15, 2018 at 02:14:02PM +0200, Sergio Paracuellos wrote:
>> static int
>> mediatek_gpio_bank_probe(struct platform_device *pdev, struct device_node
>> *bank)
>> {
>> + struct mtk_data *gpio_data = dev_get_drvdata(&pdev->dev);
>> const __be32 *id = of_get_property(bank, "reg", NULL);
>> struct mtk_gc *rg = devm_kzalloc(&pdev->dev,
>> sizeof(struct mtk_gc), GFP_KERNEL);
>> + int ret;
>
>>
>> if (!rg || !id || be32_to_cpu(*id) > MTK_MAX_BANK)
>> return -ENOMEM;
>>
>> - gc_map[be32_to_cpu(*id)] = rg;
>> + gpio_data->gc_map[be32_to_cpu(*id)] = rg;
>>
>> memset(rg, 0, sizeof(struct mtk_gc));
>>
>
> I hate that devm_kzalloc() hidden in the allocations and now the
> allocation is even further from the NULL check. Allocations inside the
> declaration block are likelier to be leaked. We see that trend looking
> at static checker output. In this case we're using managed memory so
> it's not an issue, but it still makes me itche.
>
> If *id is MTK_MAX_BANK then we end up writing one element beyond the end
> of the gc_map[] array. Normally I would say just change > to >= but
> we're not using the first element of the gc_map[] array so perhaps we
> intended to subtract one? I don't know. Probably changing > to >= is
> correct.
>
> Also this the "memset(rg, 0, sizeof(struct mtk_gc));" line is not
> required since we allocated zeroed memory.
I'll clean this up a bit in next series.
>
> In other words it maybe should be:
>
> struct mtk_gc *rg;
> int ret;
>
> if (!id || be32_to_cpu(*id) >= MTK_MAX_BANK)
> return -EINVAL;
> rg = devm_kzalloc(&pdev->dev, sizeof(struct mtk_gc), GFP_KERNEL);
> if (!rg)
> return -ENOMEM;
>
> gc_map[be32_to_cpu(*id)] = rg;
>
>
> regards,
> dan carpenter
Best regards,
Sergio Paracuellos
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 04/23] staging: ks7010: change cast from uint16_t to u16
On Tue, May 15, 2018 at 3:41 PM, Dan Carpenter wrote: > On Sun, May 13, 2018 at 08:35:39PM +0200, Sergio Paracuellos wrote: >> Header size and event fields of header are declared >> as __le16 and being casted using uint16_t in cpu_to_le16. >> Change cast to use preferred u16. >> >> Signed-off-by: Sergio Paracuellos >> --- >> drivers/staging/ks7010/ks7010_sdio.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/staging/ks7010/ks7010_sdio.c >> b/drivers/staging/ks7010/ks7010_sdio.c >> index f56db07..a51b5e8 100644 >> --- a/drivers/staging/ks7010/ks7010_sdio.c >> +++ b/drivers/staging/ks7010/ks7010_sdio.c >> @@ -1061,8 +1061,8 @@ static int send_stop_request(struct sdio_func *func) >> return -ENOMEM; >> >> size = sizeof(*pp) - sizeof(pp->header.size); >> - pp->header.size = cpu_to_le16((uint16_t)size); >> - pp->header.event = cpu_to_le16((uint16_t)HIF_STOP_REQ); >> + pp->header.size = cpu_to_le16((u16)size); >> + pp->header.event = cpu_to_le16((u16)HIF_STOP_REQ); > > This is already applied, but actually it's better to just remove the > casts entirely. I'll remove them in next series. Thanks, Sergio Paracuellos > > regards, > dan carpenter > ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 4/4] staging: lustre: obdclass: change object lookup to no wait mode
> > /* > > * Allocate new object. This may result in rather complicated > > * operations, including fld queries, inode loading, etc. > > */ > > o = lu_object_alloc(env, dev, f, conf); > > - if (IS_ERR(o)) > > + if (unlikely(IS_ERR(o))) > > return o; > > > > This is an unrelated and totally pointless. likely/unlikely annotations > hurt readability, and they should only be added if it's something which > is going to show up in benchmarking. lu_object_alloc() is already too > slow for the unlikely() to make a difference and anyway IS_ERR() has an > unlikely built in so it's duplicative... Sounds like a good checkpatch case to test for :-) Some people like to try and milk ever cycle they can. Personally for me I never use those annotations. With modern processors I'm skeptical if their benefits. I do cleanup up the patches to some extent to make it compliant with kernel standards but leave the core code in place for people to comment on. > Anyway, I understand that Intel has been ignoring kernel.org instead of > sending forwarding their patches properly so you're doing a difficult > and thankless job... Thanks for that. I'm sure it's frustrating to > look at these patches for you as well. Thank you for the complement. Also thank you for taking time to review these patches. Your feedback is most welcomed and benefitical to the health of the lustre client. Sadly its not just Intel but other vendors that don't directly contribute to the linux lustre client. I have spoke to the vendors about contributing and they all say the same thing. No working with drivers in the staging tree. Sadly all the parties involved are very interested in the success of the lustre client. No one has ever told me directly why they don't get involved but I suspect it has to deal with 2 reasons. One is that staging drivers are not normally enabled by distributions so their clients normally will never deal with the staging lustre client. Secondly vendors just lack the man power to contribute in a meanful way. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 1/5] staging: lustre: llite: add support set_acl method in inode operations
> On Mon, May 14 2018, James Simmons wrote:
>
> > From: Dmitry Eremin
> >
> > Linux kernel v3.14 adds set_acl method to inode operations.
> > This patch adds support to Lustre for proper acl management.
> >
> > Signed-off-by: Dmitry Eremin
> > Signed-off-by: John L. Hammond
> > Signed-off-by: James Simmons
> > Intel-bug-id: https://jira.hpdd.intel.com/browse/LU-9183
> > Reviewed-on: https://review.whamcloud.com/25965
> > Intel-bug-id: https://jira.hpdd.intel.com/browse/LU-10541
> > Reviewed-on: https://review.whamcloud.com/31588
> > Intel-bug-id: https://jira.hpdd.intel.com/browse/LU-10926
> > Reviewed-on: https://review.whamcloud.com/32045
> > Reviewed-by: Bob Glossman
> > Reviewed-by: James Simmons
> > Reviewed-by: Andreas Dilger
> > Reviewed-by: Dmitry Eremin
> > Reviewed-by: Oleg Drokin
> > Signed-off-by: James Simmons
> > ---
> > Changelog:
> >
> > v1) Initial patch ported to staging tree
> > v2) Fixed up goto handling and avoid BUG() when calling
> > forget_cached_acl()with invalid type as pointed out by Dan Carpenter
> >
> > drivers/staging/lustre/lustre/llite/file.c | 62
> > ++
> > .../staging/lustre/lustre/llite/llite_internal.h | 4 ++
> > drivers/staging/lustre/lustre/llite/namei.c| 10 +++-
> > 3 files changed, 74 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/staging/lustre/lustre/llite/file.c
> > b/drivers/staging/lustre/lustre/llite/file.c
> > index 0026fde..64a5698 100644
> > --- a/drivers/staging/lustre/lustre/llite/file.c
> > +++ b/drivers/staging/lustre/lustre/llite/file.c
> > @@ -3030,6 +3030,7 @@ static int ll_fiemap(struct inode *inode, struct
> > fiemap_extent_info *fieinfo,
> > return rc;
> > }
> >
> > +#ifdef CONFIG_FS_POSIX_ACL
>
> Using #ifdef in .c files is generally discouraged.
> The "standard" approach here is:
> - put the acl code in a separate file (acl.c)
> - optionally include it via the Make file
> lustre-$(CONFIG_FS_POSIX_ACL) += acl.o
>
> - in the header where ll_get_acl and ll_set_acl are declared have
> #ifdef CONFIG_FS_POSIX_ACL
>declare the functions
> #else
> #define ll_get_acl NULL
> #define ll_set_acl NULL
> #endif
>
> Now as this is staging and that is (presumably) an upstream patch
> lightly improved it is probably fine to include the patch as-is,
> but in that case we will probably want to fix it up later.
So you want Lustre to be like everyone else :-)
Okay I will fix up and send a new patch series.
> Thanks,
> NeilBrown
>
> > struct posix_acl *ll_get_acl(struct inode *inode, int type)
> > {
> > struct ll_inode_info *lli = ll_i2info(inode);
> > @@ -3043,6 +3044,64 @@ struct posix_acl *ll_get_acl(struct inode *inode,
> > int type)
> > return acl;
> > }
> >
> > +int ll_set_acl(struct inode *inode, struct posix_acl *acl, int type)
> > +{
> > + struct ll_sb_info *sbi = ll_i2sbi(inode);
> > + struct ptlrpc_request *req = NULL;
> > + const char *name = NULL;
> > + size_t value_size = 0;
> > + char *value = NULL;
> > + int rc;
> > +
> > + switch (type) {
> > + case ACL_TYPE_ACCESS:
> > + name = XATTR_NAME_POSIX_ACL_ACCESS;
> > + if (acl)
> > + rc = posix_acl_update_mode(inode, &inode->i_mode, &acl);
> > + break;
> > +
> > + case ACL_TYPE_DEFAULT:
> > + name = XATTR_NAME_POSIX_ACL_DEFAULT;
> > + if (!S_ISDIR(inode->i_mode))
> > + rc = acl ? -EACCES : 0;
> > + break;
> > +
> > + default:
> > + rc = -EINVAL;
> > + break;
> > + }
> > + if (rc)
> > + return rc;
> > +
> > + if (acl) {
> > + value_size = posix_acl_xattr_size(acl->a_count);
> > + value = kmalloc(value_size, GFP_NOFS);
> > + if (!value) {
> > + rc = -ENOMEM;
> > + goto out;
> > + }
> > +
> > + rc = posix_acl_to_xattr(&init_user_ns, acl, value, value_size);
> > + if (rc < 0)
> > + goto out_value;
> > + }
> > +
> > + rc = md_setxattr(sbi->ll_md_exp, ll_inode2fid(inode),
> > +value ? OBD_MD_FLXATTR : OBD_MD_FLXATTRRM,
> > +name, value, value_size, 0, 0, 0, &req);
> > +
> > + ptlrpc_req_finished(req);
> > +out_value:
> > + kfree(value);
> > +out:
> > + if (rc)
> > + forget_cached_acl(inode, type);
> > + else
> > + set_cached_acl(inode, type, acl);
> > + return rc;
> > +}
> > +#endif /* CONFIG_FS_POSIX_ACL */
> > +
> > int ll_inode_permission(struct inode *inode, int mask)
> > {
> > struct ll_sb_info *sbi;
> > @@ -3164,7 +3223,10 @@ int ll_inode_permission(struct inode *inode, int
> > mask)
> > .permission = ll_inode_permission,
> > .listxattr = ll_listxattr,
> > .fiemap = ll_fiemap,
> > +#ifdef CONFIG_FS_POSIX_ACL
> > .get_acl= ll_get_acl,
> > + .set_acl= ll_set_acl,
> > +#endif
> > };
> >
> > /* dynamic
Re: [PATCH v2 1/5] staging: lustre: llite: add support set_acl method in inode operations
> On Mon, May 14, 2018 at 10:16:59PM -0400, James Simmons wrote:
> > +#ifdef CONFIG_FS_POSIX_ACL
> > struct posix_acl *ll_get_acl(struct inode *inode, int type)
> > {
> > struct ll_inode_info *lli = ll_i2info(inode);
> > @@ -3043,6 +3044,64 @@ struct posix_acl *ll_get_acl(struct inode *inode,
> > int type)
> > return acl;
> > }
> >
> > +int ll_set_acl(struct inode *inode, struct posix_acl *acl, int type)
> > +{
> > + struct ll_sb_info *sbi = ll_i2sbi(inode);
> > + struct ptlrpc_request *req = NULL;
> > + const char *name = NULL;
> > + size_t value_size = 0;
> > + char *value = NULL;
> > + int rc;
>
> "rc" needs to be initialized to zero. It's disapppointing that GCC
> doesn't catch this.
Thanks Dan. Will fix.
> > +
> > + switch (type) {
> > + case ACL_TYPE_ACCESS:
> > + name = XATTR_NAME_POSIX_ACL_ACCESS;
> > + if (acl)
> > + rc = posix_acl_update_mode(inode, &inode->i_mode, &acl);
> > + break;
> > +
> > + case ACL_TYPE_DEFAULT:
> > + name = XATTR_NAME_POSIX_ACL_DEFAULT;
> > + if (!S_ISDIR(inode->i_mode))
> > + rc = acl ? -EACCES : 0;
> > + break;
> > +
> > + default:
> > + rc = -EINVAL;
> > + break;
> > + }
> > + if (rc)
> > + return rc;
>
> Otherwise rc can be uninitialized here.
>
> regards,
> dan carpenter
>
>
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: mt7621-gpio: avoid use of globals and use a drvdata allocated structure
On Tue, May 15, 2018 at 05:45:03PM +0300, Dan Carpenter wrote: > Normally I would say just change > to >= but > we're not using the first element of the gc_map[] array so perhaps we > intended to subtract one? Oh. We do use the first element. My bad. I got confused between "id" and "*id". My bad. regards, dan carpenter ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 11/11] docs: fix broken references with multiple hints
On Wed, 9 May 2018 10:18:54 -0300 Mauro Carvalho Chehab wrote: > diff --git a/Documentation/trace/events.rst b/Documentation/trace/events.rst > index 7b6b1236ec2e..c292117b83a9 100644 > --- a/Documentation/trace/events.rst > +++ b/Documentation/trace/events.rst > @@ -8,7 +8,7 @@ Event Tracing > 1. Introduction > === > > -Tracepoints (see Documentation/trace/tracepoints.txt) can be used > +Tracepoints (see Documentation/trace/tracepoints.rst) can be used > without creating custom kernel modules to register probe functions > using the event tracing infrastructure. > > diff --git a/Documentation/trace/tracepoint-analysis.rst > b/Documentation/trace/tracepoint-analysis.rst > index b0c9c21f129d..716326b9f152 100644 > --- a/Documentation/trace/tracepoint-analysis.rst > +++ b/Documentation/trace/tracepoint-analysis.rst > @@ -6,7 +6,7 @@ Notes on Analysing Behaviour Using Events and Tracepoints > 1. Introduction > === > > -Tracepoints (see Documentation/trace/tracepoints.txt) can be used without > +Tracepoints (see Documentation/trace/tracepoints.rst) can be used without > creating custom kernel modules to register probe functions using the event > tracing infrastructure. > > diff --git a/include/linux/tracepoint.h b/include/linux/tracepoint.h > index c94f466d57ef..19a690b559ca 100644 > --- a/include/linux/tracepoint.h > +++ b/include/linux/tracepoint.h > @@ -4,7 +4,7 @@ > /* > * Kernel Tracepoint API. > * > - * See Documentation/trace/tracepoints.txt. > + * See Documentation/trace/tracepoints.rst. > * > * Copyright (C) 2008-2014 Mathieu Desnoyers > * Acked-by: Steven Rostedt (VMware) -- Steve ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 11/40] ipv6/flowlabel: simplify pid namespace lookup
On Sat, May 05, 2018 at 07:37:33AM -0500, Eric W. Biederman wrote:
> Christoph Hellwig writes:
>
> > The shole seq_file sequence already operates under a single RCU lock pair,
> > so move the pid namespace lookup into it, and stop grabbing a reference
> > and remove all kinds of boilerplate code.
>
> This is wrong.
>
> Move task_active_pid_ns(current) from open to seq_start actually means
> that the results if you pass this proc file between callers the results
> will change. So this breaks file descriptor passing.
>
> Open is a bad place to access current. In the middle of read/write is
> broken.
>
>
> In this particular instance looking up the pid namespace with
> task_active_pid_ns was a personal brain fart. What the code should be
> doing (with an appropriate helper) is:
>
> struct pid_namespace *pid_ns = inode->i_sb->s_fs_info;
>
> Because each mount of proc is bound to a pid namespace. Looking up the
> pid namespace from the super_block is a much better way to go.
What do you have in mind for the helper? For now I've thrown it in
opencoded into my working tree, but I'd be glad to add a helper.
struct pid_namespace *proc_pid_namespace(struct inode *inode)
{
// maybe warn on for s_magic not on procfs??
return inode->i_sb->s_fs_info;
}
?
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: mt7621-gpio: avoid use of globals and use a drvdata allocated structure
Not related to your patch...
On Tue, May 15, 2018 at 02:14:02PM +0200, Sergio Paracuellos wrote:
> static int
> mediatek_gpio_bank_probe(struct platform_device *pdev, struct device_node
> *bank)
> {
> + struct mtk_data *gpio_data = dev_get_drvdata(&pdev->dev);
> const __be32 *id = of_get_property(bank, "reg", NULL);
> struct mtk_gc *rg = devm_kzalloc(&pdev->dev,
> sizeof(struct mtk_gc), GFP_KERNEL);
> + int ret;
>
> if (!rg || !id || be32_to_cpu(*id) > MTK_MAX_BANK)
> return -ENOMEM;
>
> - gc_map[be32_to_cpu(*id)] = rg;
> + gpio_data->gc_map[be32_to_cpu(*id)] = rg;
>
> memset(rg, 0, sizeof(struct mtk_gc));
>
I hate that devm_kzalloc() hidden in the allocations and now the
allocation is even further from the NULL check. Allocations inside the
declaration block are likelier to be leaked. We see that trend looking
at static checker output. In this case we're using managed memory so
it's not an issue, but it still makes me itche.
If *id is MTK_MAX_BANK then we end up writing one element beyond the end
of the gc_map[] array. Normally I would say just change > to >= but
we're not using the first element of the gc_map[] array so perhaps we
intended to subtract one? I don't know. Probably changing > to >= is
correct.
Also this the "memset(rg, 0, sizeof(struct mtk_gc));" line is not
required since we allocated zeroed memory.
In other words it maybe should be:
struct mtk_gc *rg;
int ret;
if (!id || be32_to_cpu(*id) >= MTK_MAX_BANK)
return -EINVAL;
rg = devm_kzalloc(&pdev->dev, sizeof(struct mtk_gc), GFP_KERNEL);
if (!rg)
return -ENOMEM;
gc_map[be32_to_cpu(*id)] = rg;
regards,
dan carpenter
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 04/40] proc: introduce proc_create_seq{,_data}
On Mon, Apr 30, 2018 at 02:19:25PM +0100, David Howells wrote:
> Christoph Hellwig wrote:
>
> > +
> > +struct proc_dir_entry *proc_create_seq_data(const char *name, umode_t mode,
> > + struct proc_dir_entry *parent, const struct seq_operations *ops,
> > + void *data)
> > +{
> > ...
> > +EXPORT_SYMBOL(proc_create_seq_data);
>
> Please add documentation comments to exported functions when you add them.
None of the base functions are document, and we really want people to not
use procfs for new code anyway. But if I get some consensus from the
maintainers and the list I can throw in another patch to document
all proc_create* variants.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 23/23] staging: ks7010: replace cast type in assignment in hostif_sme_set_pmksa
cpu_to_le16() already has a cast to u16 built in, so the cast is never required. Generally, the reason that you would cast something to a type smaller than int is when you want to truncate away the high bits. But if you're dealing with sizes and you truncate away bits, that's pretty dangerous. Really the casts are just for playing pretend, but it looks like we're doing something a bit scary with knives. regards, dan carpenter ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 34/40] atm: simplify procfs code
On Sat, May 05, 2018 at 07:51:18AM -0500, Eric W. Biederman wrote: > Christoph Hellwig writes: > > > Use remove_proc_subtree to remove the whole subtree on cleanup, and > > unwind the registration loop into individual calls. Switch to use > > proc_create_seq where applicable. > > Can you please explain why you are removing the error handling when > you are unwinding the registration loop? Because there is no point in handling these errors. The code work perfectly fine without procfs, or without given proc files and the removal works just fine if they don't exist either. This is a very common patter in various parts of the kernel already. I'll document it better in the changelog. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 22/23] staging: ks7010: use 'u16' for casts in assignments in hostif_sme_set_rsn
On Sun, May 13, 2018 at 08:35:57PM +0200, Sergio Paracuellos wrote:
> @@ -1619,7 +1619,7 @@ static void hostif_sme_set_rsn(struct ks_wlan_private
> *priv, int type)
>
> switch (type) {
> case SME_RSN_UCAST_REQUEST:
> - wpa_suite.size = cpu_to_le16((uint16_t)1);
> + wpa_suite.size = cpu_to_le16((u16)1);
Lol...
regards,
dan carpenter
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 11/23] staging: ks7010: change some cast type from uint16_t to u16 in hostif_data_request
We should remove a whole ball of casting really. regards, dan carpenter ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: simplify procfs code for seq_file instances V2
On Sun, May 06, 2018 at 08:19:49PM +0300, Alexey Dobriyan wrote: > On Wed, Apr 25, 2018 at 05:47:47PM +0200, Christoph Hellwig wrote: > > Changes since V1: > > - open code proc_create_data to avoid setting not fully initialized > >entries live > > - use unsigned int for state_size > > Need this to maintain sizeof(struct proc_dir_entry): I'll fold your changes into the relevant patches. > Otherwise ACK fs/proc/ part. I'll take this as a formal ACK-ed by for all patches touching procfs. If I was wrong please scream. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 06/40] proc: introduce proc_create_single{,_data}
On Thu, Apr 26, 2018 at 11:45:50AM +1000, Finn Thain wrote:
> >
> > -/*
> > - * /proc/nubus stuff
> > - */
> > -
>
> I don't think that the introduction of proc_create_single{,_data} alters
> the value of that comment. That comment and similar comments in the same
> file do have a purpose, which is to keep separate the /proc/nubus
> implementation is kept separate from the /proc/bus/nubus/devices
> implementation and so on.
Added back.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 06/23] staging: ks7010: change uint8_t casts to u8 in ks_wlan_set_rate
None of these casts are required. regards, dan carpenter ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: bcm2835-camera: Replace open-coded idr with a struct idr.
Dan Carpenter writes:
> On Thu, May 10, 2018 at 04:31:07PM -0700, Eric Anholt wrote:
>> @@ -258,32 +181,40 @@ get_msg_context(struct vchiq_mmal_instance *instance)
>> if (!msg_context)
>> return ERR_PTR(-ENOMEM);
>>
>> -msg_context->instance = instance;
>> -msg_context->handle =
>> -mmal_context_map_create_handle(&instance->context_map,
>> - msg_context,
>> - GFP_KERNEL);
>> +/* Create an ID that will be passed along with our message so
>> + * that when we service the VCHI reply, we can look up what
>> + * message is being replied to.
>> + */
>> +spin_lock(&instance->context_map_lock);
>> +handle = idr_alloc(&instance->context_map, msg_context,
>> + 0, 0, GFP_KERNEL);
>> +spin_unlock(&instance->context_map_lock);
>>
>> -if (!msg_context->handle) {
>> +if (msg_context->handle < 0) {
>
> This should probably be testing:
>
> if (handle < 0) {
That's what Stefan said and was fixed in v2 already.
signature.asc
Description: PGP signature
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 04/23] staging: ks7010: change cast from uint16_t to u16
On Sun, May 13, 2018 at 08:35:39PM +0200, Sergio Paracuellos wrote: > Header size and event fields of header are declared > as __le16 and being casted using uint16_t in cpu_to_le16. > Change cast to use preferred u16. > > Signed-off-by: Sergio Paracuellos > --- > drivers/staging/ks7010/ks7010_sdio.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/staging/ks7010/ks7010_sdio.c > b/drivers/staging/ks7010/ks7010_sdio.c > index f56db07..a51b5e8 100644 > --- a/drivers/staging/ks7010/ks7010_sdio.c > +++ b/drivers/staging/ks7010/ks7010_sdio.c > @@ -1061,8 +1061,8 @@ static int send_stop_request(struct sdio_func *func) > return -ENOMEM; > > size = sizeof(*pp) - sizeof(pp->header.size); > - pp->header.size = cpu_to_le16((uint16_t)size); > - pp->header.event = cpu_to_le16((uint16_t)HIF_STOP_REQ); > + pp->header.size = cpu_to_le16((u16)size); > + pp->header.event = cpu_to_le16((u16)HIF_STOP_REQ); This is already applied, but actually it's better to just remove the casts entirely. regards, dan carpenter ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 1/1] Drivers: hv: vmbus: enable VMBus protocol version 5.0
On Mon, May 14, 2018 at 11:17:55AM -0700, Stephen Hemminger wrote: > On Mon, 14 May 2018 18:14:15 + > Dexuan Cui wrote: > > > > From: devel On Behalf Of > > > Stephen Hemminger > > > Sent: Sunday, May 13, 2018 10:24 > > > > ... > > > > @@ -372,6 +400,18 @@ int vmbus_post_msg(void *buffer, size_t buflen, > > > bool can_sleep) > > > > ... > > > > + hdr = (struct vmbus_channel_message_header > > > > *)buffer; > > > > > > Hate to pick o the details, but buffer is void * so cast is not necessary > > > here. > > > > Yes, it's unnecessary in C, though it's necessary in C++. > > > > I found the patch went into char-misc 4 hours ago, so it looks we may > > as well leave it as is. IMHO an explicit cast is not a bad thing. :-) > > > > Thanks, > > -- Dexuan > > Kernel developers like to be concise. In fact there is a smatch script that > perodically > gets run and more cleanup patches get sent. It's a Coccinelle script, not Smatch. Coccinelle generates patches automatically so it's a better tool for cleanup than Smatch. I would generate a lot more Smatch information if there was a way to integrate it easily into a code editor. For example, we could highlight unecessary casts or pointer dereferences where Smatch wasn't 100% sure if it was correct. Or you could hover over function name to see what resources it allocates. regards, dan carpenter ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH] staging: mt7621-gpio: avoid use of globals and use a drvdata allocated structure
Gpio driver have a some globals which can be avoided just
using platform_data in a proper form. This commit adds a new
struct mtk_data which includes all of those globals setting them
using platform_set_drvdata and devm_gpiochip_add_data functions.
With this properly set we are able to retrieve driver data along
the code using kernel api's so globals are not needed anymore.
Signed-off-by: Sergio Paracuellos
---
I have not tested this because I don't hardware to do it but I
have compile it just to be sure it compiles. This patch needs
to be applied after the previous series where bindings have been
changed to use existant 'mediatek' instead of custom 'mtk'.
After this changes (if nothing is broken and this is accepted)
I think only make sure gpio interrupts works is remaining in
order to get this out of staging.
drivers/staging/mt7621-gpio/gpio-mt7621.c | 82 +--
1 file changed, 57 insertions(+), 25 deletions(-)
diff --git a/drivers/staging/mt7621-gpio/gpio-mt7621.c
b/drivers/staging/mt7621-gpio/gpio-mt7621.c
index 7d17949..3362ed8 100644
--- a/drivers/staging/mt7621-gpio/gpio-mt7621.c
+++ b/drivers/staging/mt7621-gpio/gpio-mt7621.c
@@ -31,17 +31,20 @@ enum mediatek_gpio_reg {
GPIO_REG_EDGE,
};
-static void __iomem *mediatek_gpio_membase;
-static int mediatek_gpio_irq;
-static struct irq_domain *mediatek_gpio_irq_domain;
-
-static struct mtk_gc {
+struct mtk_gc {
struct gpio_chip chip;
spinlock_t lock;
int bank;
u32 rising;
u32 falling;
-} *gc_map[MTK_MAX_BANK];
+};
+
+struct mtk_data {
+ void __iomem *gpio_membase;
+ int gpio_irq;
+ struct irq_domain *gpio_irq_domain;
+ struct mtk_gc *gc_map[MTK_MAX_BANK];
+};
static inline struct mtk_gc
*to_mediatek_gpio(struct gpio_chip *chip)
@@ -56,15 +59,19 @@ static inline struct mtk_gc
static inline void
mtk_gpio_w32(struct mtk_gc *rg, u8 reg, u32 val)
{
- iowrite32(val, mediatek_gpio_membase + (reg * 0x10) + (rg->bank * 0x4));
+ struct mtk_data *gpio_data = gpiochip_get_data(&rg->chip);
+ u32 offset = (reg * 0x10) + (rg->bank * 0x4);
+
+ iowrite32(val, gpio_data->gpio_membase + offset);
}
static inline u32
mtk_gpio_r32(struct mtk_gc *rg, u8 reg)
{
+ struct mtk_data *gpio_data = gpiochip_get_data(&rg->chip);
u32 offset = (reg * 0x10) + (rg->bank * 0x4);
- return ioread32(mediatek_gpio_membase + offset);
+ return ioread32(gpio_data->gpio_membase + offset);
}
static void
@@ -137,23 +144,26 @@ mediatek_gpio_get_direction(struct gpio_chip *chip,
unsigned int offset)
static int
mediatek_gpio_to_irq(struct gpio_chip *chip, unsigned int pin)
{
+ struct mtk_data *gpio_data = gpiochip_get_data(chip);
struct mtk_gc *rg = to_mediatek_gpio(chip);
- return irq_create_mapping(mediatek_gpio_irq_domain,
+ return irq_create_mapping(gpio_data->gpio_irq_domain,
pin + (rg->bank * MTK_BANK_WIDTH));
}
static int
mediatek_gpio_bank_probe(struct platform_device *pdev, struct device_node
*bank)
{
+ struct mtk_data *gpio_data = dev_get_drvdata(&pdev->dev);
const __be32 *id = of_get_property(bank, "reg", NULL);
struct mtk_gc *rg = devm_kzalloc(&pdev->dev,
sizeof(struct mtk_gc), GFP_KERNEL);
+ int ret;
if (!rg || !id || be32_to_cpu(*id) > MTK_MAX_BANK)
return -ENOMEM;
- gc_map[be32_to_cpu(*id)] = rg;
+ gpio_data->gc_map[be32_to_cpu(*id)] = rg;
memset(rg, 0, sizeof(struct mtk_gc));
@@ -169,13 +179,20 @@ mediatek_gpio_bank_probe(struct platform_device *pdev,
struct device_node *bank)
rg->chip.get_direction = mediatek_gpio_get_direction;
rg->chip.get = mediatek_gpio_get;
rg->chip.set = mediatek_gpio_set;
- if (mediatek_gpio_irq_domain)
+ if (gpio_data->gpio_irq_domain)
rg->chip.to_irq = mediatek_gpio_to_irq;
rg->bank = be32_to_cpu(*id);
/* set polarity to low for all gpios */
mtk_gpio_w32(rg, GPIO_REG_POL, 0);
+ ret = devm_gpiochip_add_data(&pdev->dev, &rg->chip, gpio_data);
+ if (ret < 0) {
+ dev_err(&pdev->dev, "Could not register gpio %d, ret=%d\n",
+ rg->chip.ngpio, ret);
+ return ret;
+ }
+
dev_info(&pdev->dev, "registering %d gpios\n", rg->chip.ngpio);
return gpiochip_add(&rg->chip);
@@ -184,10 +201,12 @@ mediatek_gpio_bank_probe(struct platform_device *pdev,
struct device_node *bank)
static void
mediatek_gpio_irq_handler(struct irq_desc *desc)
{
+ struct gpio_chip *gc = irq_desc_get_handler_data(desc);
+ struct mtk_data *gpio_data = gpiochip_get_data(gc);
int i;
for (i = 0; i < MTK_MAX_BANK; i++) {
- struct mtk_gc *rg = gc_map[i];
+ struct mtk_gc *rg = gpio_data->gc_map[i];
unsigned long pe
Re: [PATCH] staging: bcm2835-camera: Replace open-coded idr with a struct idr.
On Thu, May 10, 2018 at 04:31:07PM -0700, Eric Anholt wrote:
> @@ -258,32 +181,40 @@ get_msg_context(struct vchiq_mmal_instance *instance)
> if (!msg_context)
> return ERR_PTR(-ENOMEM);
>
> - msg_context->instance = instance;
> - msg_context->handle =
> - mmal_context_map_create_handle(&instance->context_map,
> -msg_context,
> -GFP_KERNEL);
> + /* Create an ID that will be passed along with our message so
> + * that when we service the VCHI reply, we can look up what
> + * message is being replied to.
> + */
> + spin_lock(&instance->context_map_lock);
> + handle = idr_alloc(&instance->context_map, msg_context,
> +0, 0, GFP_KERNEL);
> + spin_unlock(&instance->context_map_lock);
>
> - if (!msg_context->handle) {
> + if (msg_context->handle < 0) {
This should probably be testing:
if (handle < 0) {
> kfree(msg_context);
> - return ERR_PTR(-ENOMEM);
> + return ERR_PTR(handle);
> }
>
> + msg_context->instance = instance;
> + msg_context->handle = handle;
> +
> return msg_context;
> }
>
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: speakup: use true/false instead of 1/0
On Mon, May 14, 2018 at 10:57:25PM +0200, Samuel Thibault wrote: > Signed-off-by: Samuel Thibault Greg doesn't accept patches without a commit message. Just say which tool warned for example. regards, dan carpenter ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 24/30] staging: wilc1000: refactor del_station() to avoid parenthesis misalignment
Hi Dan,
On Tue, 15 May 2018 12:01:12 +0300
Dan Carpenter wrote:
> I feel sort of bad complaining about this patchset when your
> co-workers already nit picked it to death... :P
>
> On Mon, May 07, 2018 at 02:13:28PM +0530, Ajay Singh wrote:
> > Refactor the code to fix open parenthesis alignment issue reported
> > by checkpatch.pl script in del_station().
>
> I no idea what an "open parenthesis alignment issue" is. It's sort of
> surprising because I deal with checkpatch patches a lot.
The exact issue description reported by checkpatch.pl was
"CHECK: Alignment should match open parenthesis"
To resolve that issue tried by reducing the leading tabs before
wilc_del_allstations() call, which helped in resolving checkpatch
issue without introducing line over 80 chars.
>
> >
> > Signed-off-by: Ajay Singh
> > ---
> > drivers/staging/wilc1000/wilc_wfi_cfgoperations.c | 18
> > ++ 1 file changed, 10 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
> > b/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c index
> > 4600f4a..7f49d60 100644 ---
> > a/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c +++
> > b/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c @@ -1997,6
> > +1997,7 @@ static int del_station(struct wiphy *wiphy, struct
> > net_device *dev, s32 ret = 0; struct wilc_priv *priv;
> > struct wilc_vif *vif;
> > + struct sta_info *info;
> >
> > if (!wiphy)
> > return -EFAULT;
> > @@ -2004,16 +2005,17 @@ static int del_station(struct wiphy *wiphy,
> > struct net_device *dev, priv = wiphy_priv(wiphy);
> > vif = netdev_priv(dev);
> >
> > - if (vif->iftype == AP_MODE || vif->iftype == GO_MODE) {
> > - if (!mac)
> > - ret = wilc_del_allstation(vif,
> > -
> > priv->assoc_stainfo.sta_associated_bss);
> > + if (!(vif->iftype == AP_MODE || vif->iftype == GO_MODE))
>
> I feel like this is better as:
> if (vif->iftype != AP_MODE && vif->iftype != GO_MODE)
>
Thanks for your suggestion.
Currently the patch is accepted, i will check and try to include it in
future patch as per your inputs.
> > + return ret;
>
> What is "ret" here? I haven't looked at this patch in context, but
> it's probably zero. Just return the literal.
The value for 'ret' is zero till this point, only if there is
failure to post the command in wilc_eneque_cmd() then 'ret' receives
-ENOMEM value in below code.
Regards,
Ajay
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 1/5] staging: lustre: llite: add support set_acl method in inode operations
On Mon, May 14, 2018 at 10:16:59PM -0400, James Simmons wrote:
> +#ifdef CONFIG_FS_POSIX_ACL
> struct posix_acl *ll_get_acl(struct inode *inode, int type)
> {
> struct ll_inode_info *lli = ll_i2info(inode);
> @@ -3043,6 +3044,64 @@ struct posix_acl *ll_get_acl(struct inode *inode, int
> type)
> return acl;
> }
>
> +int ll_set_acl(struct inode *inode, struct posix_acl *acl, int type)
> +{
> + struct ll_sb_info *sbi = ll_i2sbi(inode);
> + struct ptlrpc_request *req = NULL;
> + const char *name = NULL;
> + size_t value_size = 0;
> + char *value = NULL;
> + int rc;
"rc" needs to be initialized to zero. It's disapppointing that GCC
doesn't catch this.
> +
> + switch (type) {
> + case ACL_TYPE_ACCESS:
> + name = XATTR_NAME_POSIX_ACL_ACCESS;
> + if (acl)
> + rc = posix_acl_update_mode(inode, &inode->i_mode, &acl);
> + break;
> +
> + case ACL_TYPE_DEFAULT:
> + name = XATTR_NAME_POSIX_ACL_DEFAULT;
> + if (!S_ISDIR(inode->i_mode))
> + rc = acl ? -EACCES : 0;
> + break;
> +
> + default:
> + rc = -EINVAL;
> + break;
> + }
> + if (rc)
> + return rc;
Otherwise rc can be uninitialized here.
regards,
dan carpenter
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 11/11] docs: fix broken references with multiple hints
On Wednesday, May 09, 2018 10:18:54 AM Mauro Carvalho Chehab wrote: > The script: > ./scripts/documentation-file-ref-check --fix-rst > > Gives multiple hints for broken references on some files. > Manually use the one that applies for some files. > > Signed-off-by: Mauro Carvalho Chehab Acked-by: Bartlomiej Zolnierkiewicz # for fbdev part Best regards, -- Bartlomiej Zolnierkiewicz Samsung R&D Institute Poland Samsung Electronics ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] Staging:Comedi:comedi_compat32.c: Lindent changes
On 15/05/18 06:20, Pratik Jain wrote:
Recommended indentation by Lindent on file comedi_compat32.c
Signed-off-by: Pratik Jain
---
drivers/staging/comedi/comedi_compat32.c | 12 ++--
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/drivers/staging/comedi/comedi_compat32.c
b/drivers/staging/comedi/comedi_compat32.c
index 97fb9388bc22..fa9d239474ee 100644
--- a/drivers/staging/comedi/comedi_compat32.c
+++ b/drivers/staging/comedi/comedi_compat32.c
@@ -34,7 +34,7 @@
struct comedi32_chaninfo_struct {
unsigned int subdev;
compat_uptr_t maxdata_list; /* 32-bit 'unsigned int *' */
- compat_uptr_t flaglist; /* 32-bit 'unsigned int *' */
+ compat_uptr_t flaglist; /* 32-bit 'unsigned int *' */
compat_uptr_t rangelist;/* 32-bit 'unsigned int *' */
unsigned int unused[4];
};
@@ -57,16 +57,16 @@ struct comedi32_cmd_struct {
unsigned int scan_end_arg;
unsigned int stop_src;
unsigned int stop_arg;
- compat_uptr_t chanlist; /* 32-bit 'unsigned int *' */
+ compat_uptr_t chanlist; /* 32-bit 'unsigned int *' */
unsigned int chanlist_len;
- compat_uptr_t data; /* 32-bit 'short *' */
+ compat_uptr_t data; /* 32-bit 'short *' */
unsigned int data_len;
};
struct comedi32_insn_struct {
unsigned int insn;
unsigned int n;
- compat_uptr_t data; /* 32-bit 'unsigned int *' */
+ compat_uptr_t data; /* 32-bit 'unsigned int *' */
unsigned int subdev;
unsigned int chanspec;
unsigned int unused[3];
@@ -74,7 +74,7 @@ struct comedi32_insn_struct {
struct comedi32_insnlist_struct {
unsigned int n_insns;
- compat_uptr_t insns;/* 32-bit 'struct comedi_insn *' */
+ compat_uptr_t insns;/* 32-bit 'struct comedi_insn *' */
};
Those all make the code look less pretty because all those comments are
nicely aligned currently.
/* Handle translated ioctl. */
@@ -194,7 +194,7 @@ static int get_compat_cmd(struct comedi_cmd __user *cmd,
err |= __put_user(temp.uint, &cmd->stop_arg);
err |= __get_user(temp.uptr, &cmd32->chanlist);
err |= __put_user((unsigned int __force *)compat_ptr(temp.uptr),
- &cmd->chanlist);
+ &cmd->chanlist);
err |= __get_user(temp.uint, &cmd32->chanlist_len);
err |= __put_user(temp.uint, &cmd->chanlist_len);
err |= __get_user(temp.uptr, &cmd32->data);
That is the only one that is an improvement, IMHO.
--
-=( Ian Abbott || Web: www.mev.co.uk )=-
-=( MEV Ltd. is a company registered in England & Wales. )=-
-=( Registered number: 02862268. Registered address:)=-
-=( 15 West Park Road, Bramhall, STOCKPORT, SK7 3JZ, UK. )=-
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 13/15] staging: bcm2835-camera: Fix warnings about string ops on v4l2 uapi.
Not related to the patch, but that's weird. I get that it's part of the user API now, but why it u8 and can't it be changed? regards, dan carpenter ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 24/30] staging: wilc1000: refactor del_station() to avoid parenthesis misalignment
I feel sort of bad complaining about this patchset when your co-workers
already nit picked it to death... :P
On Mon, May 07, 2018 at 02:13:28PM +0530, Ajay Singh wrote:
> Refactor the code to fix open parenthesis alignment issue reported by
> checkpatch.pl script in del_station().
I no idea what an "open parenthesis alignment issue" is. It's sort of
surprising because I deal with checkpatch patches a lot.
>
> Signed-off-by: Ajay Singh
> ---
> drivers/staging/wilc1000/wilc_wfi_cfgoperations.c | 18 ++
> 1 file changed, 10 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
> b/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
> index 4600f4a..7f49d60 100644
> --- a/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
> +++ b/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
> @@ -1997,6 +1997,7 @@ static int del_station(struct wiphy *wiphy, struct
> net_device *dev,
> s32 ret = 0;
> struct wilc_priv *priv;
> struct wilc_vif *vif;
> + struct sta_info *info;
>
> if (!wiphy)
> return -EFAULT;
> @@ -2004,16 +2005,17 @@ static int del_station(struct wiphy *wiphy, struct
> net_device *dev,
> priv = wiphy_priv(wiphy);
> vif = netdev_priv(dev);
>
> - if (vif->iftype == AP_MODE || vif->iftype == GO_MODE) {
> - if (!mac)
> - ret = wilc_del_allstation(vif,
> - priv->assoc_stainfo.sta_associated_bss);
> + if (!(vif->iftype == AP_MODE || vif->iftype == GO_MODE))
I feel like this is better as:
if (vif->iftype != AP_MODE && vif->iftype != GO_MODE)
> + return ret;
What is "ret" here? I haven't looked at this patch in context, but
it's probably zero. Just return the literal.
regards,
dan carpenter
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 1/4] staging: mt7621-gpio: dt-bindings: add documentation for mt7621-gpio
This commit add missing dt bindings documentation for mt7621-gpio
driver. There is some missing stuff here about interrupts with is
not also being used in the mt7621.dtsi file. So just include in
staging a incomplete version before moving this to kernel's dt-bindings
place.
Signed-off-by: Sergio Paracuellos
---
.../staging/mt7621-gpio/mediatek,mt7621-gpio.txt | 51 ++
1 file changed, 51 insertions(+)
create mode 100644 drivers/staging/mt7621-gpio/mediatek,mt7621-gpio.txt
diff --git a/drivers/staging/mt7621-gpio/mediatek,mt7621-gpio.txt
b/drivers/staging/mt7621-gpio/mediatek,mt7621-gpio.txt
new file mode 100644
index 000..54de9f7
--- /dev/null
+++ b/drivers/staging/mt7621-gpio/mediatek,mt7621-gpio.txt
@@ -0,0 +1,51 @@
+Mediatek SoC GPIO controller bindings
+
+The IP core used inside these SoCs has 3 banks of 32 GPIOs each.
+The registers of all the banks are interwoven inside one single IO range.
+We load one GPIO controller instance per bank. To make this possible
+we support 2 types of nodes. The parent node defines the memory I/O range and
+has 3 children each describing a single bank.
+
+Required properties for the top level node:
+- compatible:
+ - "mediatek,mt7621-gpio" for Mediatek controllers
+- reg : Physical base address and length of the controller's registers
+
+Required properties for the GPIO bank node:
+- compatible:
+ - "mediatek,mt7621-gpio-bank" for Mediatek banks
+- #gpio-cells : Should be two.
+ - first cell is the pin number
+ - second cell is used to specify optional parameters (unused)
+- gpio-controller : Marks the device node as a GPIO controller
+- reg : The id of the bank that the node describes.
+
+Example:
+ gpio@600 {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ compatible = "mediatek,mt7621-gpio";
+ reg = <0x600 0x100>;
+
+ gpio0: bank@0 {
+ reg = <0>;
+ compatible = "mediatek,mt7621-gpio-bank";
+ gpio-controller;
+ #gpio-cells = <2>;
+ };
+
+ gpio1: bank@1 {
+ reg = <1>;
+ compatible = "mediatek,mt7621-gpio-bank";
+ gpio-controller;
+ #gpio-cells = <2>;
+ };
+
+ gpio2: bank@2 {
+ reg = <2>;
+ compatible = "mediatek,mt7621-gpio-bank";
+ gpio-controller;
+ #gpio-cells = <2>;
+ };
+ };
--
2.7.4
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 4/4] staging: mt7621-gpio: replace 'mtk' to use correct one 'mediatek'
Gpio driver is using mtk and there is already 'mediatek' binding
defined for this maker. Update driver to use it instead the custom
one 'mtk'.
Signed-off-by: Sergio Paracuellos
---
drivers/staging/mt7621-gpio/gpio-mt7621.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/staging/mt7621-gpio/gpio-mt7621.c
b/drivers/staging/mt7621-gpio/gpio-mt7621.c
index a577381..7d17949 100644
--- a/drivers/staging/mt7621-gpio/gpio-mt7621.c
+++ b/drivers/staging/mt7621-gpio/gpio-mt7621.c
@@ -323,7 +323,7 @@ mediatek_gpio_probe(struct platform_device *pdev)
}
for_each_child_of_node(np, bank)
- if (of_device_is_compatible(bank, "mtk,mt7621-gpio-bank"))
+ if (of_device_is_compatible(bank, "mediatek,mt7621-gpio-bank"))
mediatek_gpio_bank_probe(pdev, bank);
if (mediatek_gpio_irq_domain)
@@ -334,7 +334,7 @@ mediatek_gpio_probe(struct platform_device *pdev)
}
static const struct of_device_id mediatek_gpio_match[] = {
- { .compatible = "mtk,mt7621-gpio" },
+ { .compatible = "mediatek,mt7621-gpio" },
{},
};
MODULE_DEVICE_TABLE(of, mediatek_gpio_match);
--
2.7.4
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 3/4] staging: mt7621-dts: update gpios related entries to use 'mediatek'
Gpio driver for mt7621 is using 'mtk' as binding but in the kernel
is already defined one for this maker which is 'mediatek'. Update
device tree to use the correct one.
Signed-off-by: Sergio Paracuellos
---
drivers/staging/mt7621-dts/mt7621.dtsi | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/staging/mt7621-dts/mt7621.dtsi
b/drivers/staging/mt7621-dts/mt7621.dtsi
index 9d941b5..115eb04 100644
--- a/drivers/staging/mt7621-dts/mt7621.dtsi
+++ b/drivers/staging/mt7621-dts/mt7621.dtsi
@@ -64,26 +64,26 @@
#address-cells = <1>;
#size-cells = <0>;
- compatible = "mtk,mt7621-gpio";
+ compatible = "mediatek,mt7621-gpio";
reg = <0x600 0x100>;
gpio0: bank@0 {
reg = <0>;
- compatible = "mtk,mt7621-gpio-bank";
+ compatible = "mediatek,mt7621-gpio-bank";
gpio-controller;
#gpio-cells = <2>;
};
gpio1: bank@1 {
reg = <1>;
- compatible = "mtk,mt7621-gpio-bank";
+ compatible = "mediatek,mt7621-gpio-bank";
gpio-controller;
#gpio-cells = <2>;
};
gpio2: bank@2 {
reg = <2>;
- compatible = "mtk,mt7621-gpio-bank";
+ compatible = "mediatek,mt7621-gpio-bank";
gpio-controller;
#gpio-cells = <2>;
};
--
2.7.4
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 0/4] staging: mt7621-gpio: use mediatek as binding instead of custom mtk
The following series updated mt7621-gpio driver to use 'mediatek' which is already defined in kernel bindings documentation instead of add a new custom one 'mtk' for this company. mt7621-gpio device-tree documentation has been added also but there some missing stuff about gpio interrupts which is not included in dtsi yet. So add temporal documentation to the staging driver directory and update TODO file accordly with missing things to get this driver out of staging. Sergio Paracuellos (4): staging: mt7621-gpio: dt-bindings: add documentation for mt7621-gpio staging: mt7621-gpio: update TODO file staging: mt7621-dts: update gpios related entries to use 'mediatek' staging: mt7621-gpio: replace 'mtk' to use correct one 'mediatek' drivers/staging/mt7621-dts/mt7621.dtsi | 8 ++-- drivers/staging/mt7621-gpio/TODO | 4 +- drivers/staging/mt7621-gpio/gpio-mt7621.c | 4 +- .../staging/mt7621-gpio/mediatek,mt7621-gpio.txt | 51 ++ 4 files changed, 60 insertions(+), 7 deletions(-) create mode 100644 drivers/staging/mt7621-gpio/mediatek,mt7621-gpio.txt -- 2.7.4 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 2/4] staging: mt7621-gpio: update TODO file
This commit updates TODO file with missing things to get this driver ready to be mainlined. Signed-off-by: Sergio Paracuellos --- drivers/staging/mt7621-gpio/TODO | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/drivers/staging/mt7621-gpio/TODO b/drivers/staging/mt7621-gpio/TODO index 7143905..9077b16 100644 --- a/drivers/staging/mt7621-gpio/TODO +++ b/drivers/staging/mt7621-gpio/TODO @@ -1,5 +1,7 @@ -- general code review and clean up +- general code review and clean up +- avoid global variables and use drvdata allocated instead - ensure device-tree requirements are documented +- make sure interrupts work Cc: NeilBrown -- 2.7.4 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 5/5] staging: mt7621-gpio: update TODO file
On Tue, May 15, 2018 at 09:29:04AM +0200, Greg KH wrote: > On Tue, May 15, 2018 at 06:49:57AM +0200, Sergio Paracuellos wrote: > > This commit updates TODO file with missing things to > > get this driver ready to be mainlined. > > > > Signed-off-by: Sergio Paracuellos > > Reviewed-by: NeilBrown > > --- > > drivers/staging/mt7621-gpio/TODO | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/staging/mt7621-gpio/TODO > > b/drivers/staging/mt7621-gpio/TODO > > index 7143905..54e6271 100644 > > --- a/drivers/staging/mt7621-gpio/TODO > > +++ b/drivers/staging/mt7621-gpio/TODO > > @@ -1,5 +1,6 @@ > > > > -- general code review and clean up > > +- general code review and clean up: avoid global variables and use drvdata > > allocated instead > > This now goes beyond 80 columns so someone else will send a patch fixing > that up :) > > How about making this two lines instead please. Ack! I'll include this in my next series. > > thanks, > > greg k-h Best regards, Sergio Paracuellos ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 4/5] dt-bindings: gpio: add documentation for mt7621-gpio
On Tue, May 15, 2018 at 09:25:06AM +0200, Greg KH wrote: > On Mon, May 14, 2018 at 04:02:33PM +0200, Sergio Paracuellos wrote: > > This commit add missing dt bindings documentation for mt7621-gpio > > driver. After this checkpatch script complain about this > > issue dissapears. > > > > Signed-off-by: Sergio Paracuellos > > --- > > .../devicetree/bindings/gpio/mtk,mt7621-gpio.txt | 51 > > ++ > > 1 file changed, 51 insertions(+) > > create mode 100644 > > Documentation/devicetree/bindings/gpio/mtk,mt7621-gpio.txt > > > > diff --git a/Documentation/devicetree/bindings/gpio/mtk,mt7621-gpio.txt > > b/Documentation/devicetree/bindings/gpio/mtk,mt7621-gpio.txt > > new file mode 100644 > > index 000..5fe4bb5 > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/gpio/mtk,mt7621-gpio.txt > > @@ -0,0 +1,51 @@ > > +Mediatek SoC GPIO controller bindings > > + > > +The IP core used inside these SoCs has 3 banks of 32 GPIOs each. > > +The registers of all the banks are interwoven inside one single IO range. > > +We load one GPIO controller instance per bank. To make this possible > > +we support 2 types of nodes. The parent node defines the memory I/O range > > and > > +has 3 children each describing a single bank. > > + > > +Required properties for the top level node: > > +- compatible: > > + - "mtk,mt7621-gpio" for Mediatek controllers > > As stated in the previous review, "mtk" is not valid, please use > "mediatek" like the rest of the kernel does :) I will and send new patch series with those fixed. > > thanks, Best regards, Sergio Paracuellos > > greg k-h ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 03/30] staging: wilc1000: fix line over 80 chars in handle_key()
On Thu, May 10, 2018 at 08:21:52AM +0300, Claudiu Beznea wrote: > >>> + wid.val = (s8 *)key_buf; > >> > >> Is this cast really needed? > >> > > > > This patch is only to address line over 80 chars checkpatch issue. > > It is not good to club these change with type cast related > > modification. For removing unnecessary cast we can submit > > a separate patch series. > > These are my views. What do you think? > > > > I'm ok with this. I was thinking that since you introduced this new function > you may want to also address this, if any. No. Please don't. That kind of thing is sort of an unrelated change from just moving the lines around and it messes up my review scripts. It's really really easy to review the changes when they're split into multiple chunks. regards, dan carpenter ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 3/5] dt-bindings: add compatible string for 'mtk' MediaTek
On Tue, May 15, 2018 at 09:24:30AM +0200, Greg KH wrote: > On Mon, May 14, 2018 at 04:02:32PM +0200, Sergio Paracuellos wrote: > > There is a complain of checkpatch script about the not documented > > DT compatible string for vendor "mtk". Add it to vendor-prefixes.txt > > file. > > > > Signed-off-by: Sergio Paracuellos > > --- > > Documentation/devicetree/bindings/vendor-prefixes.txt | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/Documentation/devicetree/bindings/vendor-prefixes.txt > > b/Documentation/devicetree/bindings/vendor-prefixes.txt > > index b5f978a..a588a29 100644 > > --- a/Documentation/devicetree/bindings/vendor-prefixes.txt > > +++ b/Documentation/devicetree/bindings/vendor-prefixes.txt > > @@ -228,6 +228,7 @@ mqmaker mqmaker Inc. > > mscc Microsemi Corporation > > msiMicro-Star International Co. Ltd. > > mtiImagination Technologies Ltd. (formerly MIPS Technologies Inc.) > > +mtkMediaTek > > MediaTek already has a binding here, it is "mediatek". Please don't > introduce a new one for the same company. True, sorry for this. > > Also, all device tree patches need to cc: the device tree maintainers to > get their review before I can accept them. Thanks for let me know. > > thanks, > > greg k-h Best regards, Sergio Paracuellos ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 10/14] staging: clocking-wizard: Support clk_round_rate
On Tue, May 15, 2018 at 10:52:58AM +0300, Dan Carpenter wrote:
> On Mon, May 07, 2018 at 11:20:36AM +1000, James Kelly wrote:
> > @@ -463,6 +549,16 @@ static int clk_wzrd_debug_init(struct clk_hw *hw,
> > struct dentry *dentry)
> > struct dentry *d;
> > struct clk_wzrd_clk_data *cwc = to_clk_wzrd_clk_data(hw);
> >
> > + d = debugfs_create_u16("clk_ratio_min", 0444, dentry,
> > + (u16 *)&cwc->ratio_limit->min);
> > + if (IS_ERR(d))
> > + return PTR_ERR(d);
> > +
> > + d = debugfs_create_u16("clk_ratio_max", 0444, dentry,
> > + (u16 *)&cwc->ratio_limit->max);
> > + if (IS_ERR(d))
> > + return PTR_ERR(d);
>
>
> All these debugfs stuff should be tests for NULL.
>
> *
> * If debugfs is not enabled in the kernel, the value -%ENODEV will be
> * returned. It is not wise to check for this value, but rather, check for
> * %NULL or !%NULL instead as to eliminate the need for #ifdef in the calling
> * code.
No, you should not check for anything, debugfs calls can work just fine
by ignoring the return value. You should not care what debugfs does, or
does not do, when you call it.
I should update the documentation a bit better, but no one seems to even
read it anyway :(
thanks,
greg k-h
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 10/14] staging: clocking-wizard: Support clk_round_rate
On Mon, May 07, 2018 at 11:20:36AM +1000, James Kelly wrote:
> @@ -463,6 +549,16 @@ static int clk_wzrd_debug_init(struct clk_hw *hw, struct
> dentry *dentry)
> struct dentry *d;
> struct clk_wzrd_clk_data *cwc = to_clk_wzrd_clk_data(hw);
>
> + d = debugfs_create_u16("clk_ratio_min", 0444, dentry,
> +(u16 *)&cwc->ratio_limit->min);
> + if (IS_ERR(d))
> + return PTR_ERR(d);
> +
> + d = debugfs_create_u16("clk_ratio_max", 0444, dentry,
> +(u16 *)&cwc->ratio_limit->max);
> + if (IS_ERR(d))
> + return PTR_ERR(d);
All these debugfs stuff should be tests for NULL.
*
* If debugfs is not enabled in the kernel, the value -%ENODEV will be
* returned. It is not wise to check for this value, but rather, check for
* %NULL or !%NULL instead as to eliminate the need for #ifdef in the calling
* code.
regards,
dan carpenter
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH] staging: wlan-ng: fix coding style issues in p80211netdev.h
Fix two issues with parameters not aligned to opening parenthesis, as
reported by checkpatch. File is now clean for checkpatch.
Signed-off-by: Tim Collier
---
drivers/staging/wlan-ng/p80211netdev.h | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/staging/wlan-ng/p80211netdev.h
b/drivers/staging/wlan-ng/p80211netdev.h
index fbac47311f26..d48466d943b4 100644
--- a/drivers/staging/wlan-ng/p80211netdev.h
+++ b/drivers/staging/wlan-ng/p80211netdev.h
@@ -180,11 +180,11 @@ struct wlandevice {
int (*close)(struct wlandevice *wlandev);
void (*reset)(struct wlandevice *wlandev);
int (*txframe)(struct wlandevice *wlandev, struct sk_buff *skb,
- union p80211_hdr *p80211_hdr,
- struct p80211_metawep *p80211_wep);
+ union p80211_hdr *p80211_hdr,
+ struct p80211_metawep *p80211_wep);
int (*mlmerequest)(struct wlandevice *wlandev, struct p80211msg *msg);
int (*set_multicast_list)(struct wlandevice *wlandev,
- struct net_device *dev);
+ struct net_device *dev);
void (*tx_timeout)(struct wlandevice *wlandev);
/* 802.11 State */
--
2.11.0
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 03/14] staging: clocking-wizard: Split probe function
On Tue, May 15, 2018 at 05:30:15AM +1000, James Kelly wrote:
> Dan,
>
> > On 14 May 2018, at 11:47 pm, Dan Carpenter wrote:
> >
> > On Mon, May 07, 2018 at 11:20:29AM +1000, James Kelly wrote:
> >> +static int clk_wzrd_probe(struct platform_device *pdev)
> >> +{
> >> + int ret;
> >> + struct device *dev = &pdev->dev;
> >> +
> >> + ret = clk_wzrd_get_device_tree_data(dev);
> >> + if (ret)
> >> + return ret;
> >> +
> >> + ret = clk_wzrd_regmap_alloc(dev);
> >> + if (ret)
> >> + return ret;
> >> +
> >> + ret = clk_wzrd_register_ccf(dev);
> >> + if (ret)
> >> + return ret;
> >> +
> >> + return 0;
> >
> > The error handling is a terrible layer violation now... Every
> > allocation function should have a matching free function. But now
> > instead of that if clk_wzrd_register_ccf() fails then it starts cleaning
> > up the clk_wzrd->axi_clk that was allocated in
> > clk_wzrd_get_device_tree_data().
> >
> > regards,
> > dan carpenter
> >
> >
>
> This gets cleaned up in patch 6. There seemed little point in putting a
> whole lot of
> code in patch 3 only to rip it out again in patch 6. I was trying to keep
> the patches
> simple.
I later posted that it's actually buggy so it's not allow to introduce
a bug and then fix it.
I really quarrel with the word "simple". As a reviewer it's my job to
spot bugs being introduced. It took me a long time to review this patch
until I spotted the bug. It's better to not take short cuts, but just
write error handling in the standard way.
regards,
dan carpenter
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] Drivers: hv: vmbus: Removed an unnecessary cast from void *
On Tue, May 15, 2018 at 12:25:01AM +, Dexuan Cui wrote:
>
> In C, we don't need such a cast.
>
> Fixes: ae20b254306a ("Drivers: hv: vmbus: enable VMBus protocol version 5.0")
> Reported-by: Stephen Hemminger
> Signed-off-by: Dexuan Cui
> Cc: K. Y. Srinivasan
> ---
>
> Thanks Stephen Hemminger for pointing this out!
>
> So far, ae20b254306a ("Drivers: hv: vmbus: enable VMBus protocol version
> 5.0") only
> appears in the char-misc tree's char-misc-testing and char-misc-next
> branches. If
> possible, please merge both patches into one.
I can not rebase/merge patches in public branches, sorry, so I'll just
apply this one.
thanks,
greg k-h
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] Drivers: hv: vmbus: Removed an unnecessary cast from void *
On Tue, May 15, 2018 at 12:25:01AM +, Dexuan Cui wrote:
>
> In C, we don't need such a cast.
>
> Fixes: ae20b254306a ("Drivers: hv: vmbus: enable VMBus protocol version 5.0")
> Signed-off-by: Dexuan Cui
> Cc: Stephen Hemminger
Should be "Reported-by:", I'll go edit this by hand :(
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 1/5] staging: lustre: llite: add support set_acl method in inode operations
On Tue, May 15, 2018 at 01:53:02PM +1000, NeilBrown wrote: > On Mon, May 14 2018, James Simmons wrote: > > > From: Dmitry Eremin > > > > Linux kernel v3.14 adds set_acl method to inode operations. > > This patch adds support to Lustre for proper acl management. > > > > Signed-off-by: Dmitry Eremin > > Signed-off-by: John L. Hammond > > Signed-off-by: James Simmons > > Intel-bug-id: https://jira.hpdd.intel.com/browse/LU-9183 > > Reviewed-on: https://review.whamcloud.com/25965 > > Intel-bug-id: https://jira.hpdd.intel.com/browse/LU-10541 > > Reviewed-on: https://review.whamcloud.com/31588 > > Intel-bug-id: https://jira.hpdd.intel.com/browse/LU-10926 > > Reviewed-on: https://review.whamcloud.com/32045 > > Reviewed-by: Bob Glossman > > Reviewed-by: James Simmons > > Reviewed-by: Andreas Dilger > > Reviewed-by: Dmitry Eremin > > Reviewed-by: Oleg Drokin > > Signed-off-by: James Simmons > > --- > > Changelog: > > > > v1) Initial patch ported to staging tree > > v2) Fixed up goto handling and avoid BUG() when calling > > forget_cached_acl()with invalid type as pointed out by Dan Carpenter > > > > drivers/staging/lustre/lustre/llite/file.c | 62 > > ++ > > .../staging/lustre/lustre/llite/llite_internal.h | 4 ++ > > drivers/staging/lustre/lustre/llite/namei.c| 10 +++- > > 3 files changed, 74 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/staging/lustre/lustre/llite/file.c > > b/drivers/staging/lustre/lustre/llite/file.c > > index 0026fde..64a5698 100644 > > --- a/drivers/staging/lustre/lustre/llite/file.c > > +++ b/drivers/staging/lustre/lustre/llite/file.c > > @@ -3030,6 +3030,7 @@ static int ll_fiemap(struct inode *inode, struct > > fiemap_extent_info *fieinfo, > > return rc; > > } > > > > +#ifdef CONFIG_FS_POSIX_ACL > > Using #ifdef in .c files is generally discouraged. > The "standard" approach here is: > - put the acl code in a separate file (acl.c) > - optionally include it via the Make file > lustre-$(CONFIG_FS_POSIX_ACL) += acl.o > > - in the header where ll_get_acl and ll_set_acl are declared have > #ifdef CONFIG_FS_POSIX_ACL >declare the functions > #else > #define ll_get_acl NULL > #define ll_set_acl NULL > #endif > > Now as this is staging and that is (presumably) an upstream patch > lightly improved it is probably fine to include the patch as-is, > but in that case we will probably want to fix it up later. Let's get it right the first time if at all possible please. I'll drop this series from my queue and wait for the next version of it. thanks, greg k-h ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 5/5] staging: mt7621-gpio: update TODO file
On Tue, May 15, 2018 at 06:49:57AM +0200, Sergio Paracuellos wrote: > This commit updates TODO file with missing things to > get this driver ready to be mainlined. > > Signed-off-by: Sergio Paracuellos > Reviewed-by: NeilBrown > --- > drivers/staging/mt7621-gpio/TODO | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/staging/mt7621-gpio/TODO > b/drivers/staging/mt7621-gpio/TODO > index 7143905..54e6271 100644 > --- a/drivers/staging/mt7621-gpio/TODO > +++ b/drivers/staging/mt7621-gpio/TODO > @@ -1,5 +1,6 @@ > > -- general code review and clean up > +- general code review and clean up: avoid global variables and use drvdata > allocated instead This now goes beyond 80 columns so someone else will send a patch fixing that up :) How about making this two lines instead please. thanks, greg k-h ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 0/5] staging: mt7621-gpio: some driver cleanups
On Tue, May 15, 2018 at 04:18:56PM +1000, NeilBrown wrote: > On Tue, May 15 2018, Sergio Paracuellos wrote: > > > The following patch series fix some checkpatch complains > > about this driver. There is still some complains about dt-bindings > > not being added to its correct Documentation place inside the kernel > > but it should be ok when this driver will be move out of staging. > > Changes have not been tested and also compiled but it should not have > > any problem about them. > > > > Changes in v2: > > - mtk,mt7621-gpio.txt dt-bindings description moved inside the > > staging folder because is still uncomplete. > > - TODO has been updated with real stuff missing to get this > > driver out of staging. > > > > Sergio Paracuellos (5): > > staging: mt7621-gpio: fix some warnings because of lines exceeded 80 > > characters > > staging: mt7621-gpio: add SPDX identifier > > dt-bindings: add compatible string for 'mtk' MediaTek > > staging: mt7621-gpio: dt-bindings: add documentation for mt7621-gpio > > staging: mt7621-gpio: update TODO file > > Thanks, > all looks good. > > Reviewed-by: NeilBrown > > for the whole series. I can't take the dt patches for the previously reviewed reasons, but I'll queue up the other 3 now, thanks. greg k-h ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 3/5] dt-bindings: add compatible string for 'mtk' MediaTek
On Tue, May 15, 2018 at 06:49:55AM +0200, Sergio Paracuellos wrote: > There is a complain of checkpatch script about the not documented > DT compatible string for vendor "mtk". Add it to vendor-prefixes.txt > file. > > Signed-off-by: Sergio Paracuellos > --- > Documentation/devicetree/bindings/vendor-prefixes.txt | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/Documentation/devicetree/bindings/vendor-prefixes.txt > b/Documentation/devicetree/bindings/vendor-prefixes.txt > index b5f978a..a588a29 100644 > --- a/Documentation/devicetree/bindings/vendor-prefixes.txt > +++ b/Documentation/devicetree/bindings/vendor-prefixes.txt > @@ -228,6 +228,7 @@ mqmaker mqmaker Inc. > mscc Microsemi Corporation > msi Micro-Star International Co. Ltd. > mti Imagination Technologies Ltd. (formerly MIPS Technologies Inc.) > +mtk MediaTek Same issue as the v1 patch :( ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 4/5] dt-bindings: gpio: add documentation for mt7621-gpio
On Mon, May 14, 2018 at 04:02:33PM +0200, Sergio Paracuellos wrote: > This commit add missing dt bindings documentation for mt7621-gpio > driver. After this checkpatch script complain about this > issue dissapears. > > Signed-off-by: Sergio Paracuellos > --- > .../devicetree/bindings/gpio/mtk,mt7621-gpio.txt | 51 > ++ > 1 file changed, 51 insertions(+) > create mode 100644 Documentation/devicetree/bindings/gpio/mtk,mt7621-gpio.txt > > diff --git a/Documentation/devicetree/bindings/gpio/mtk,mt7621-gpio.txt > b/Documentation/devicetree/bindings/gpio/mtk,mt7621-gpio.txt > new file mode 100644 > index 000..5fe4bb5 > --- /dev/null > +++ b/Documentation/devicetree/bindings/gpio/mtk,mt7621-gpio.txt > @@ -0,0 +1,51 @@ > +Mediatek SoC GPIO controller bindings > + > +The IP core used inside these SoCs has 3 banks of 32 GPIOs each. > +The registers of all the banks are interwoven inside one single IO range. > +We load one GPIO controller instance per bank. To make this possible > +we support 2 types of nodes. The parent node defines the memory I/O range and > +has 3 children each describing a single bank. > + > +Required properties for the top level node: > +- compatible: > + - "mtk,mt7621-gpio" for Mediatek controllers As stated in the previous review, "mtk" is not valid, please use "mediatek" like the rest of the kernel does :) thanks, greg k-h ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 3/5] dt-bindings: add compatible string for 'mtk' MediaTek
On Mon, May 14, 2018 at 04:02:32PM +0200, Sergio Paracuellos wrote: > There is a complain of checkpatch script about the not documented > DT compatible string for vendor "mtk". Add it to vendor-prefixes.txt > file. > > Signed-off-by: Sergio Paracuellos > --- > Documentation/devicetree/bindings/vendor-prefixes.txt | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/Documentation/devicetree/bindings/vendor-prefixes.txt > b/Documentation/devicetree/bindings/vendor-prefixes.txt > index b5f978a..a588a29 100644 > --- a/Documentation/devicetree/bindings/vendor-prefixes.txt > +++ b/Documentation/devicetree/bindings/vendor-prefixes.txt > @@ -228,6 +228,7 @@ mqmaker mqmaker Inc. > mscc Microsemi Corporation > msi Micro-Star International Co. Ltd. > mti Imagination Technologies Ltd. (formerly MIPS Technologies Inc.) > +mtk MediaTek MediaTek already has a binding here, it is "mediatek". Please don't introduce a new one for the same company. Also, all device tree patches need to cc: the device tree maintainers to get their review before I can accept them. thanks, greg k-h ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 01/27] drivers/staging/rtl8192u/ieee80211/rtl819x_HTProc: correct braces
On Mon, May 14, 2018 at 10:24:08PM +0100, John Whitmore wrote:
> Coding style edit to clear the checkpatch.pl errors of the type:
> ERROR: that open brace { should be on the previous line
>
> Signed-off-by: John Whitmore
> ---
Any reason why your subject line has the full path and not the shorter
description that I suggested in my last email?
Also, when resending a patch series, you need to version it, and
describe what changed. The file Documentation/SubmittingPatches goes
into the details on how to do this, see this section specifically:
https://www.kernel.org/doc/html/latest/process/submitting-patches.html#describe-your-changes
Please fix up and try again.
thanks,
greg k-h
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
