[PATCH 0/2] Rewrite the IIO_DEVICE_ATTR_NAMED API to pass name as string.
This patchset is to rewrite the IIO_DEVICE_ATTR_NAMED API to pass name as string. Himanshi Jain (2): include: linux: sysfs: Add __ATTR_NAMED macro iio: Use __ATTR_NAMED to allow passing name as string to IIO_DEVICE_ATTR_NAMED and change usage to pass string drivers/iio/adc/ad7793.c | 2 +- drivers/staging/iio/adc/ad7192.c | 2 +- drivers/staging/iio/adc/ad7280a.c | 4 ++-- include/linux/iio/sysfs.h | 6 +- include/linux/sysfs.h | 7 +++ 5 files changed, 16 insertions(+), 5 deletions(-) -- 1.9.1 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 1/2] include: linux: sysfs: Add __ATTR_NAMED macro
Add __ATTR_NAMED macro similar to __ATTR but taking name as a
string instead of implicit conversion of argument to string using
the macro _stringify(_name).
Signed-off-by: Himanshi Jain
---
include/linux/sysfs.h | 7 +++
1 file changed, 7 insertions(+)
diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
index aa02c32..20321cf 100644
--- a/include/linux/sysfs.h
+++ b/include/linux/sysfs.h
@@ -104,6 +104,13 @@ struct attribute_group {
.store = _store, \
}
+#define __ATTR_NAMED(_name, _mode, _show, _store) {\
+ .attr = {.name = _name, \
+.mode = VERIFY_OCTAL_PERMISSIONS(_mode) }, \
+ .show = _show, \
+ .store = _store,\
+}
+
#define __ATTR_PREALLOC(_name, _mode, _show, _store) { \
.attr = {.name = __stringify(_name),\
.mode = SYSFS_PREALLOC | VERIFY_OCTAL_PERMISSIONS(_mode) },\
--
1.9.1
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 2/2] iio: Use __ATTR_NAMED to allow passing name as string to IIO_DEVICE_ATTR_NAMED and change usage to pass string
Add IIO_ATTR_NAMED macro to use __ATTR_NAMED to allow passing name as
string to IIO_DEVICE_ATTR_NAMED. Change current usage of
IIO_DEVICE_ATTR_NAMED to pass name as string.
Signed-off-by: Himanshi Jain
---
drivers/iio/adc/ad7793.c | 2 +-
drivers/staging/iio/adc/ad7192.c | 2 +-
drivers/staging/iio/adc/ad7280a.c | 4 ++--
include/linux/iio/sysfs.h | 6 +-
4 files changed, 9 insertions(+), 5 deletions(-)
diff --git a/drivers/iio/adc/ad7793.c b/drivers/iio/adc/ad7793.c
index e6706a0..d74e324 100644
--- a/drivers/iio/adc/ad7793.c
+++ b/drivers/iio/adc/ad7793.c
@@ -420,7 +420,7 @@ static ssize_t ad7793_show_scale_available(struct device
*dev,
}
static IIO_DEVICE_ATTR_NAMED(in_m_in_scale_available,
- in_voltage-voltage_scale_available, S_IRUGO,
+ "in_voltage-voltage_scale_available", S_IRUGO,
ad7793_show_scale_available, NULL, 0);
static struct attribute *ad7793_attributes[] = {
diff --git a/drivers/staging/iio/adc/ad7192.c b/drivers/staging/iio/adc/ad7192.c
index d11c6de..daff38c 100644
--- a/drivers/staging/iio/adc/ad7192.c
+++ b/drivers/staging/iio/adc/ad7192.c
@@ -341,7 +341,7 @@ static int ad7192_setup(struct ad7192_state *st,
}
static IIO_DEVICE_ATTR_NAMED(in_v_m_v_scale_available,
-in_voltage-voltage_scale_available,
+"in_voltage-voltage_scale_available",
0444, ad7192_show_scale_available, NULL, 0);
static IIO_DEVICE_ATTR(in_voltage_scale_available, 0444,
diff --git a/drivers/staging/iio/adc/ad7280a.c
b/drivers/staging/iio/adc/ad7280a.c
index f85dde9..fd32e9a 100644
--- a/drivers/staging/iio/adc/ad7280a.c
+++ b/drivers/staging/iio/adc/ad7280a.c
@@ -750,14 +750,14 @@ static irqreturn_t ad7280_event_handler(int irq, void
*private)
}
static IIO_DEVICE_ATTR_NAMED(in_thresh_low_value,
- in_voltage-voltage_thresh_low_value,
+ "in_voltage-voltage_thresh_low_value",
0644,
ad7280_read_channel_config,
ad7280_write_channel_config,
AD7280A_CELL_UNDERVOLTAGE);
static IIO_DEVICE_ATTR_NAMED(in_thresh_high_value,
- in_voltage-voltage_thresh_high_value,
+ "in_voltage-voltage_thresh_high_value",
0644,
ad7280_read_channel_config,
ad7280_write_channel_config,
diff --git a/include/linux/iio/sysfs.h b/include/linux/iio/sysfs.h
index ce9426c..49c81a4 100644
--- a/include/linux/iio/sysfs.h
+++ b/include/linux/iio/sysfs.h
@@ -55,6 +55,10 @@ struct iio_const_attr {
{ .dev_attr = __ATTR(_name, _mode, _show, _store), \
.address = _addr }
+#define IIO_ATTR_NAMED(_name, _mode, _show, _store, _addr) \
+ { .dev_attr = __ATTR_NAMED(_name, _mode, _show, _store),\
+ .address = _addr }
+
#define IIO_ATTR_RO(_name, _addr) \
{ .dev_attr = __ATTR_RO(_name), \
.address = _addr }
@@ -85,7 +89,7 @@ struct iio_const_attr {
#define IIO_DEVICE_ATTR_NAMED(_vname, _name, _mode, _show, _store, _addr) \
struct iio_dev_attr iio_dev_attr_##_vname \
- = IIO_ATTR(_name, _mode, _show, _store, _addr)
+ = IIO_ATTR_NAMED(_name, _mode, _show, _store, _addr)
#define IIO_CONST_ATTR(_name, _string) \
struct iio_const_attr iio_const_attr_##_name\
--
1.9.1
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH] drivers:staging/media:Use ARRAY_SIZE() for the size calculation of the array
Signed-off-by: Allen Pais
---
drivers/staging/media/atomisp/pci/atomisp2/css2400/sh_css.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/staging/media/atomisp/pci/atomisp2/css2400/sh_css.c
b/drivers/staging/media/atomisp/pci/atomisp2/css2400/sh_css.c
index e882b55..d822918 100644
--- a/drivers/staging/media/atomisp/pci/atomisp2/css2400/sh_css.c
+++ b/drivers/staging/media/atomisp/pci/atomisp2/css2400/sh_css.c
@@ -451,7 +451,7 @@ static enum ia_css_frame_format yuv422_copy_formats[] = {
IA_CSS_FRAME_FORMAT_YUYV
};
-#define array_length(array) (sizeof(array)/sizeof(array[0]))
+#define array_length(array) (ARRAY_SIZE(array))
/* Verify whether the selected output format is can be produced
* by the copy binary given the stream format.
--
2.7.4
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 2/2] iio: Use __ATTR_NAMED to allow passing name as string to IIO_DEVICE_ATTR_NAMED and change usage to pass string
On Wed, Sep 13, 2017 at 01:26:27PM +0530, Himanshi Jain wrote: > Add IIO_ATTR_NAMED macro to use __ATTR_NAMED to allow passing name as > string to IIO_DEVICE_ATTR_NAMED. Change current usage of > IIO_DEVICE_ATTR_NAMED to pass name as string. > > Signed-off-by: Himanshi Jain This version looks nice. The subject is just *way* too long, though. Also you have put a v2 in the subject and a little changelog after the --- cut off. Just call it something like: [PATCH 2/2 v2] iio: Change to __ATTR_NAMED() regards, dan carpenter ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] drivers:staging/media:Use ARRAY_SIZE() for the size calculation of the array
You need a changelog.
On Wed, Sep 13, 2017 at 01:34:39PM +0530, Allen Pais wrote:
> Signed-off-by: Allen Pais
> ---
> drivers/staging/media/atomisp/pci/atomisp2/css2400/sh_css.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/staging/media/atomisp/pci/atomisp2/css2400/sh_css.c
> b/drivers/staging/media/atomisp/pci/atomisp2/css2400/sh_css.c
> index e882b55..d822918 100644
> --- a/drivers/staging/media/atomisp/pci/atomisp2/css2400/sh_css.c
> +++ b/drivers/staging/media/atomisp/pci/atomisp2/css2400/sh_css.c
> @@ -451,7 +451,7 @@ static enum ia_css_frame_format yuv422_copy_formats[] = {
> IA_CSS_FRAME_FORMAT_YUYV
> };
>
> -#define array_length(array) (sizeof(array)/sizeof(array[0]))
> +#define array_length(array) (ARRAY_SIZE(array))
Just get rid of this array_length macro and use ARRAY_SIZE() directly.
regards,
dan carpenter
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v2] drivers:staging/media:Use ARRAY_SIZE() for the size calculation of the array
Signed-off-by: Allen Pais
---
drivers/staging/media/atomisp/pci/atomisp2/css2400/sh_css.c | 6 ++
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/staging/media/atomisp/pci/atomisp2/css2400/sh_css.c
b/drivers/staging/media/atomisp/pci/atomisp2/css2400/sh_css.c
index e882b55..bee3043 100644
--- a/drivers/staging/media/atomisp/pci/atomisp2/css2400/sh_css.c
+++ b/drivers/staging/media/atomisp/pci/atomisp2/css2400/sh_css.c
@@ -451,8 +451,6 @@ static enum ia_css_frame_format yuv422_copy_formats[] = {
IA_CSS_FRAME_FORMAT_YUYV
};
-#define array_length(array) (sizeof(array)/sizeof(array[0]))
-
/* Verify whether the selected output format is can be produced
* by the copy binary given the stream format.
* */
@@ -468,7 +466,7 @@ verify_copy_out_frame_format(struct ia_css_pipe *pipe)
switch (pipe->stream->config.input_config.format) {
case IA_CSS_STREAM_FORMAT_YUV420_8_LEGACY:
case IA_CSS_STREAM_FORMAT_YUV420_8:
- for (i=0; ihttp://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] drivers:staging/media:Use ARRAY_SIZE() for the size calculation of the array
>> >> -#define array_length(array) (sizeof(array)/sizeof(array[0])) >> +#define array_length(array) (ARRAY_SIZE(array)) > > Just get rid of this array_length macro and use ARRAY_SIZE() directly. > Sure. - Allen ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v2 0/2] Rewrite the IIO_DEVICE_ATTR_NAMED API to pass name as string.
This patchset is to rewrite the IIO_DEVICE_ATTR_NAMED API to pass name as string. Himanshi Jain (2): include: linux: sysfs: Add __ATTR_NAMED macro iio: Change to __ATTR_NAMED() drivers/iio/adc/ad7793.c | 2 +- drivers/staging/iio/adc/ad7192.c | 2 +- drivers/staging/iio/adc/ad7280a.c | 4 ++-- include/linux/iio/sysfs.h | 6 +- include/linux/sysfs.h | 7 +++ 5 files changed, 16 insertions(+), 5 deletions(-) -- 1.9.1 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v2 1/2] include: linux: sysfs: Add __ATTR_NAMED macro
Add __ATTR_NAMED macro similar to __ATTR but taking name as a
string instead of implicit conversion of argument to string using
the macro _stringify(_name).
Signed-off-by: Himanshi Jain
---
include/linux/sysfs.h | 7 +++
1 file changed, 7 insertions(+)
diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
index aa02c32..20321cf 100644
--- a/include/linux/sysfs.h
+++ b/include/linux/sysfs.h
@@ -104,6 +104,13 @@ struct attribute_group {
.store = _store, \
}
+#define __ATTR_NAMED(_name, _mode, _show, _store) {\
+ .attr = {.name = _name, \
+.mode = VERIFY_OCTAL_PERMISSIONS(_mode) }, \
+ .show = _show, \
+ .store = _store,\
+}
+
#define __ATTR_PREALLOC(_name, _mode, _show, _store) { \
.attr = {.name = __stringify(_name),\
.mode = SYSFS_PREALLOC | VERIFY_OCTAL_PERMISSIONS(_mode) },\
--
1.9.1
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v2 2/2] iio: Change to __ATTR_NAMED()
Add IIO_ATTR_NAMED macro to use __ATTR_NAMED to allow passing
name as string to IIO_DEVICE_ATTR_NAMED. Change current usage
of IIO_DEVICE_ATTR_NAMED to pass name as string.
Signed-off-by: Himanshi Jain
---
drivers/iio/adc/ad7793.c | 2 +-
drivers/staging/iio/adc/ad7192.c | 2 +-
drivers/staging/iio/adc/ad7280a.c | 4 ++--
include/linux/iio/sysfs.h | 6 +-
4 files changed, 9 insertions(+), 5 deletions(-)
diff --git a/drivers/iio/adc/ad7793.c b/drivers/iio/adc/ad7793.c
index e6706a0..d74e324 100644
--- a/drivers/iio/adc/ad7793.c
+++ b/drivers/iio/adc/ad7793.c
@@ -420,7 +420,7 @@ static ssize_t ad7793_show_scale_available(struct device
*dev,
}
static IIO_DEVICE_ATTR_NAMED(in_m_in_scale_available,
- in_voltage-voltage_scale_available, S_IRUGO,
+ "in_voltage-voltage_scale_available", S_IRUGO,
ad7793_show_scale_available, NULL, 0);
static struct attribute *ad7793_attributes[] = {
diff --git a/drivers/staging/iio/adc/ad7192.c b/drivers/staging/iio/adc/ad7192.c
index d11c6de..daff38c 100644
--- a/drivers/staging/iio/adc/ad7192.c
+++ b/drivers/staging/iio/adc/ad7192.c
@@ -341,7 +341,7 @@ static int ad7192_setup(struct ad7192_state *st,
}
static IIO_DEVICE_ATTR_NAMED(in_v_m_v_scale_available,
-in_voltage-voltage_scale_available,
+"in_voltage-voltage_scale_available",
0444, ad7192_show_scale_available, NULL, 0);
static IIO_DEVICE_ATTR(in_voltage_scale_available, 0444,
diff --git a/drivers/staging/iio/adc/ad7280a.c
b/drivers/staging/iio/adc/ad7280a.c
index f85dde9..fd32e9a 100644
--- a/drivers/staging/iio/adc/ad7280a.c
+++ b/drivers/staging/iio/adc/ad7280a.c
@@ -750,14 +750,14 @@ static irqreturn_t ad7280_event_handler(int irq, void
*private)
}
static IIO_DEVICE_ATTR_NAMED(in_thresh_low_value,
- in_voltage-voltage_thresh_low_value,
+ "in_voltage-voltage_thresh_low_value",
0644,
ad7280_read_channel_config,
ad7280_write_channel_config,
AD7280A_CELL_UNDERVOLTAGE);
static IIO_DEVICE_ATTR_NAMED(in_thresh_high_value,
- in_voltage-voltage_thresh_high_value,
+ "in_voltage-voltage_thresh_high_value",
0644,
ad7280_read_channel_config,
ad7280_write_channel_config,
diff --git a/include/linux/iio/sysfs.h b/include/linux/iio/sysfs.h
index ce9426c..49c81a4 100644
--- a/include/linux/iio/sysfs.h
+++ b/include/linux/iio/sysfs.h
@@ -55,6 +55,10 @@ struct iio_const_attr {
{ .dev_attr = __ATTR(_name, _mode, _show, _store), \
.address = _addr }
+#define IIO_ATTR_NAMED(_name, _mode, _show, _store, _addr) \
+ { .dev_attr = __ATTR_NAMED(_name, _mode, _show, _store),\
+ .address = _addr }
+
#define IIO_ATTR_RO(_name, _addr) \
{ .dev_attr = __ATTR_RO(_name), \
.address = _addr }
@@ -85,7 +89,7 @@ struct iio_const_attr {
#define IIO_DEVICE_ATTR_NAMED(_vname, _name, _mode, _show, _store, _addr) \
struct iio_dev_attr iio_dev_attr_##_vname \
- = IIO_ATTR(_name, _mode, _show, _store, _addr)
+ = IIO_ATTR_NAMED(_name, _mode, _show, _store, _addr)
#define IIO_CONST_ATTR(_name, _string) \
struct iio_const_attr iio_const_attr_##_name\
--
1.9.1
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] drivers:staging/media:Use ARRAY_SIZE() for the size calculation of the array
Also change the subject prefix to: [media] atomisp: so it's: Subject: [PATCH v2] [media] atomisp: Use ARRAY_SIZE() instead of open coding it regards, dan carpenter ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2] drivers:staging/media:Use ARRAY_SIZE() for the size calculation of the array
On Wed, Sep 13, 2017 at 02:09:25PM +0530, Allen Pais wrote: > Signed-off-by: Allen Pais This is going through linux-media and maybe they won't insist on some kind of commit message, but I know Greg does. regards, dan carpenter ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 2/2] iio: Use __ATTR_NAMED to allow passing name as string to IIO_DEVICE_ATTR_NAMED and change usage to pass string
On Wed, Sep 13, 2017 at 11:12:21AM +0300, Dan Carpenter wrote: > On Wed, Sep 13, 2017 at 01:26:27PM +0530, Himanshi Jain wrote: > > Add IIO_ATTR_NAMED macro to use __ATTR_NAMED to allow passing name as > > string to IIO_DEVICE_ATTR_NAMED. Change current usage of > > IIO_DEVICE_ATTR_NAMED to pass name as string. > > > > Signed-off-by: Himanshi Jain > > This version looks nice. The subject is just *way* too long, though. > Also you have put a v2 in the subject and a little changelog after the > --- cut off. Just call it something like: > [PATCH 2/2 v2] iio: Change to __ATTR_NAMED() > > regards, > dan carpenter Thank you for the review Dan! Does it look fine now in v2? ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [Outreachy kernel] [PATCH v2 0/2] Rewrite the IIO_DEVICE_ATTR_NAMED API to pass name as string.
On Wed, 13 Sep 2017, Himanshi Jain wrote: > This patchset is to rewrite the IIO_DEVICE_ATTR_NAMED API to pass name > as string. You need to indicate what has changed in the v2, either here or in the individual patches. julia > > Himanshi Jain (2): > include: linux: sysfs: Add __ATTR_NAMED macro > iio: Change to __ATTR_NAMED() > > drivers/iio/adc/ad7793.c | 2 +- > drivers/staging/iio/adc/ad7192.c | 2 +- > drivers/staging/iio/adc/ad7280a.c | 4 ++-- > include/linux/iio/sysfs.h | 6 +- > include/linux/sysfs.h | 7 +++ > 5 files changed, 16 insertions(+), 5 deletions(-) > > -- > 1.9.1 > > -- > You received this message because you are subscribed to the Google Groups > "outreachy-kernel" group. > To unsubscribe from this group and stop receiving emails from it, send an > email to outreachy-kernel+unsubscr...@googlegroups.com. > To post to this group, send email to outreachy-ker...@googlegroups.com. > To view this discussion on the web visit > https://groups.google.com/d/msgid/outreachy-kernel/cover.1505291907.git.himshijain.hj%40gmail.com. > For more options, visit https://groups.google.com/d/optout. > ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 10/11] staging: typec: fusb302: Hook up mux support using tcpc_gen_mux support
Hi, On 13-09-17 00:20, Rob Herring wrote: On Tue, Sep 05, 2017 at 06:42:20PM +0200, Hans de Goede wrote: Add mux support to the fusb302 driver, call devm_tcpc_gen_mux_create() to let the generic tcpc_mux_dev code create a tcpc_mux_dev for us. Also document the mux-names used by the generic tcpc_mux_dev code in our devicetree bindings. Cc: Rob Herring Cc: Mark Rutland Cc: devicet...@vger.kernel.org Signed-off-by: Hans de Goede --- Documentation/devicetree/bindings/usb/fcs,fusb302.txt | 3 +++ drivers/staging/typec/fusb302/fusb302.c | 11 ++- 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/Documentation/devicetree/bindings/usb/fcs,fusb302.txt b/Documentation/devicetree/bindings/usb/fcs,fusb302.txt index 472facfa5a71..63d639eadacd 100644 --- a/Documentation/devicetree/bindings/usb/fcs,fusb302.txt +++ b/Documentation/devicetree/bindings/usb/fcs,fusb302.txt @@ -6,6 +6,9 @@ Required properties : - interrupts : Interrupt specifier Optional properties : +- mux-controls : List of mux-ctrl-specifiers containing 1 or 2 muxes +- mux-names : "type-c-mode-mux" when using 1 mux, or + "type-c-mode-mux", "usb-role-mux" when using 2 muxes I'm not sure this is the right place for this. The mux is outside the FUSB302. In a type-C connector node or USB phy node would make more sense to me. The mux certainly does not belong in the USB phy node, it sits between the USB PHY and the Type-C connector and can for example also mux the Type-C pins to a Display Port PHY. As for putting it in a type-C connector node, currently we do not have such a node, the closest thing we do have to a node describing it is actually the fusb302 node itself. E.g. it may also contain a regulator to turn Vbus on / off (already there in the code, but I forgot to document this when I added the missing DT bindings doc for the fusb302 with a previous patch). Also these properties: - fcs,max-sink-microvolt : Maximum voltage to negotiate when configured as sink - fcs,max-sink-microamp : Maximum current to negotiate when configured as sink - fcs,max-sink-microwatt : Maximum power to negotiate when configured as sink Have more to do with the charger-IC used (which determines the limits) then with the fusb302 itself, but the fusb302 needs to know these as it negotiates the limits. Likewise the fusb302 negotiates how the data pins will be used and thus to which pins on the SoC the mux should mux the data pins. TL;DR: The fusb302 does all the negotiation and ties all the Type-C connected ICs together, so this seems like the right place for it (it certainly is the natural place to put these from a driver code pov). Regards, Hans ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v3] drivers/staging:[media]atomisp:use ARRAY_SIZE() instead of open coding.
Signed-off-by: Allen Pais
---
drivers/staging/media/atomisp/pci/atomisp2/css2400/sh_css.c | 6 ++
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/staging/media/atomisp/pci/atomisp2/css2400/sh_css.c
b/drivers/staging/media/atomisp/pci/atomisp2/css2400/sh_css.c
index e882b55..bee3043 100644
--- a/drivers/staging/media/atomisp/pci/atomisp2/css2400/sh_css.c
+++ b/drivers/staging/media/atomisp/pci/atomisp2/css2400/sh_css.c
@@ -451,8 +451,6 @@ static enum ia_css_frame_format yuv422_copy_formats[] = {
IA_CSS_FRAME_FORMAT_YUYV
};
-#define array_length(array) (sizeof(array)/sizeof(array[0]))
-
/* Verify whether the selected output format is can be produced
* by the copy binary given the stream format.
* */
@@ -468,7 +466,7 @@ verify_copy_out_frame_format(struct ia_css_pipe *pipe)
switch (pipe->stream->config.input_config.format) {
case IA_CSS_STREAM_FORMAT_YUV420_8_LEGACY:
case IA_CSS_STREAM_FORMAT_YUV420_8:
- for (i=0; ihttp://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2] drivers:staging/media:Use ARRAY_SIZE() for the size calculation of the array
> > This is going through linux-media and maybe they won't insist on some > kind of commit message, but I know Greg does. Okay. I sent out a V3. -- - Allen ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 1/1] x86/hyper-V: Allocate the IDT entry early in boot
* KY Srinivasan wrote: > > > > -Original Message- > > From: Dan Carpenter [mailto:dan.carpen...@oracle.com] > > Sent: Saturday, September 9, 2017 4:04 AM > > To: KY Srinivasan > > Cc: x...@kernel.org; gre...@linuxfoundation.org; linux- > > ker...@vger.kernel.org; de...@linuxdriverproject.org; o...@aepfle.de; > > a...@canonical.com; jasow...@redhat.com; t...@linutronix.de; > > h...@zytor.com; mi...@kernel.org > > Subject: Re: [PATCH 1/1] x86/hyper-V: Allocate the IDT entry early in boot > > > > On Fri, Sep 08, 2017 at 04:15:57PM -0700, k...@exchange.microsoft.com > > wrote: > > > From: "K. Y. Srinivasan" > > > > > > Allocate the hypervisor callback IDT entry early in the boot sequence. > > > > > > > I'm guessing this fixes a NULL dereference or something? The changelog > > doesn't really say why we are doing this. > > The changelog does say what we are doing - allocating the IDT entry early in > the boot sequence. But the question was the 'why', not the 'what' - so Dan's question is fully justified ... > The current code would allocate the entry as part of registering the handler > when vmbus driver loaded and this caused a problem for the cleanup Thomas had > implemented. I've put this explanation into the changelog. Thanks, Ingo ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3] drivers/staging:[media]atomisp:use ARRAY_SIZE() instead of open coding.
On Wed, Sep 13, 2017 at 02:27:53PM +0530, Allen Pais wrote: > Signed-off-by: Allen Pais Sorry, the patch is right, but the commit is still totally messed up. bad: [PATCH v3] drivers/staging:[media]atomisp:use ARRAY_SIZE() instead of open coding. good: [PATCH v4] [media] atomisp: use ARRAY_SIZE() instead of open coding. Please, copy the "[media] atomisp: " prefix exactly as I wrote it. Then the commit message can say something like: The array_length() macro just duplicates ARRAY_SIZE() so we can delete it. > Signed-off-by: Allen Pais > --- ^^^ Then under the --- line put: v4: Update the commit message. regards, dan carpenter ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v4] [media]atomisp:use ARRAY_SIZE() instead of open coding.
The array_length() macro just duplicates ARRAY_SIZE(), so we can
delete it.
v4: Update the commit message.
Signed-off-by: Allen Pais
---
drivers/staging/media/atomisp/pci/atomisp2/css2400/sh_css.c | 6 ++
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/staging/media/atomisp/pci/atomisp2/css2400/sh_css.c
b/drivers/staging/media/atomisp/pci/atomisp2/css2400/sh_css.c
index e882b55..bee3043 100644
--- a/drivers/staging/media/atomisp/pci/atomisp2/css2400/sh_css.c
+++ b/drivers/staging/media/atomisp/pci/atomisp2/css2400/sh_css.c
@@ -451,8 +451,6 @@ static enum ia_css_frame_format yuv422_copy_formats[] = {
IA_CSS_FRAME_FORMAT_YUYV
};
-#define array_length(array) (sizeof(array)/sizeof(array[0]))
-
/* Verify whether the selected output format is can be produced
* by the copy binary given the stream format.
* */
@@ -468,7 +466,7 @@ verify_copy_out_frame_format(struct ia_css_pipe *pipe)
switch (pipe->stream->config.input_config.format) {
case IA_CSS_STREAM_FORMAT_YUV420_8_LEGACY:
case IA_CSS_STREAM_FORMAT_YUV420_8:
- for (i=0; ihttp://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3] drivers/staging:[media]atomisp:use ARRAY_SIZE() instead of open coding.
> bad: [PATCH v3] drivers/staging:[media]atomisp:use ARRAY_SIZE() instead of > open coding. > good: [PATCH v4] [media] atomisp: use ARRAY_SIZE() instead of open coding. My bad. Fixed it in V4. Thanks. - Allen ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 10/11] staging: typec: fusb302: Hook up mux support using tcpc_gen_mux support
On Wed, Sep 13, 2017 at 3:56 AM, Hans de Goede wrote: > Hi, > > > On 13-09-17 00:20, Rob Herring wrote: >> >> On Tue, Sep 05, 2017 at 06:42:20PM +0200, Hans de Goede wrote: >>> >>> Add mux support to the fusb302 driver, call devm_tcpc_gen_mux_create() >>> to let the generic tcpc_mux_dev code create a tcpc_mux_dev for us. >>> >>> Also document the mux-names used by the generic tcpc_mux_dev code in >>> our devicetree bindings. >>> >>> Cc: Rob Herring >>> Cc: Mark Rutland >>> Cc: devicet...@vger.kernel.org >>> Signed-off-by: Hans de Goede >>> --- >>> Documentation/devicetree/bindings/usb/fcs,fusb302.txt | 3 +++ >>> drivers/staging/typec/fusb302/fusb302.c | 11 ++- >>> 2 files changed, 13 insertions(+), 1 deletion(-) >>> >>> diff --git a/Documentation/devicetree/bindings/usb/fcs,fusb302.txt >>> b/Documentation/devicetree/bindings/usb/fcs,fusb302.txt >>> index 472facfa5a71..63d639eadacd 100644 >>> --- a/Documentation/devicetree/bindings/usb/fcs,fusb302.txt >>> +++ b/Documentation/devicetree/bindings/usb/fcs,fusb302.txt >>> @@ -6,6 +6,9 @@ Required properties : >>> - interrupts : Interrupt specifier >>> Optional properties : >>> +- mux-controls : List of mux-ctrl-specifiers containing 1 or 2 >>> muxes >>> +- mux-names : "type-c-mode-mux" when using 1 mux, or >>> + "type-c-mode-mux", "usb-role-mux" when using >>> 2 muxes >> >> >> I'm not sure this is the right place for this. The mux is outside the >> FUSB302. In a type-C connector node or USB phy node would make more >> sense to me. > > > The mux certainly does not belong in the USB phy node, it sits between the > USB PHY > and the Type-C connector and can for example also mux the Type-C pins to a > Display > Port PHY. Thinking about this some more, the mux(es) should be its own node(s). Then the question is where to put the nodes. > As for putting it in a type-C connector node, currently we do not have such > a node, Well, you should. Type-C connectors are certainly complicated enough that we'll need one. Plus we already require connector nodes for display outputs, so what do we do once you add display muxing? > the closest thing we do have to a node describing it is actually the fusb302 > node > itself. E.g. it may also contain a regulator to turn Vbus on / off (already > there > in the code, but I forgot to document this when I added the missing DT > bindings > doc for the fusb302 with a previous patch). Either you can have a vbus-supply property in connector node or it can be implied that the controller chip provides that. For example, HDMI connectors have a hpd-gpios property if HPD is connected to GPIO or they have nothing and it's implicit that the HDMI encoder handles HPD. > Also these properties: > >>> - fcs,max-sink-microvolt : Maximum voltage to negotiate when configured >>> as sink >>> - fcs,max-sink-microamp : Maximum current to negotiate when configured >>> as sink >>> - fcs,max-sink-microwatt : Maximum power to negotiate when configured >>> as sink > > > Have more to do with the charger-IC used (which determines the limits) then > with > the fusb302 itself, but the fusb302 needs to know these as it negotiates the > limits. Those should probably be elsewhere and not be fusb302 specific. I did ack that, but it was a single node for a single component which is fine. But once we start adding more external pieces we need to pay more attention to the overall structure. > Likewise the fusb302 negotiates how the data pins will be used and thus to > which pins > on the SoC the mux should mux the data pins. > > TL;DR: The fusb302 does all the negotiation and ties all the Type-C > connected > ICs together, so this seems like the right place for it (it certainly is the > natural place to put these from a driver code pov). Things in DT should follow what the h/w design looks like which is not necessarily aligned with the driver structure. If the USB PD chip needs information from the charger, then we need a kernel interface for that. My concern here is not so much this binding in particular, but rather that we handle Type-C connectors in a common way and not adhoc with each platform doing things their own way. Otherwise, we end up with a mess of platform specific bindings like charger/battery bindings (though there's some work improving those). Rob ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 10/11] staging: typec: fusb302: Hook up mux support using tcpc_gen_mux support
Hi, On 13-09-17 15:38, Rob Herring wrote: On Wed, Sep 13, 2017 at 3:56 AM, Hans de Goede wrote: Hi, On 13-09-17 00:20, Rob Herring wrote: On Tue, Sep 05, 2017 at 06:42:20PM +0200, Hans de Goede wrote: Add mux support to the fusb302 driver, call devm_tcpc_gen_mux_create() to let the generic tcpc_mux_dev code create a tcpc_mux_dev for us. Also document the mux-names used by the generic tcpc_mux_dev code in our devicetree bindings. Cc: Rob Herring Cc: Mark Rutland Cc: devicet...@vger.kernel.org Signed-off-by: Hans de Goede --- Documentation/devicetree/bindings/usb/fcs,fusb302.txt | 3 +++ drivers/staging/typec/fusb302/fusb302.c | 11 ++- 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/Documentation/devicetree/bindings/usb/fcs,fusb302.txt b/Documentation/devicetree/bindings/usb/fcs,fusb302.txt index 472facfa5a71..63d639eadacd 100644 --- a/Documentation/devicetree/bindings/usb/fcs,fusb302.txt +++ b/Documentation/devicetree/bindings/usb/fcs,fusb302.txt @@ -6,6 +6,9 @@ Required properties : - interrupts : Interrupt specifier Optional properties : +- mux-controls : List of mux-ctrl-specifiers containing 1 or 2 muxes +- mux-names : "type-c-mode-mux" when using 1 mux, or + "type-c-mode-mux", "usb-role-mux" when using 2 muxes I'm not sure this is the right place for this. The mux is outside the FUSB302. In a type-C connector node or USB phy node would make more sense to me. The mux certainly does not belong in the USB phy node, it sits between the USB PHY and the Type-C connector and can for example also mux the Type-C pins to a Display Port PHY. Thinking about this some more, the mux(es) should be its own node(s). Then the question is where to put the nodes. Right, the mux will be its own node per Documentation/devicetree/bindings/mux/mux-controller.txt the bindings bit this patch is adding is only adding phandles pointing to that mux-node as the fusb302 "consumes" the mux functionality. So as such (the fusb302 is the component which should control the mux) I do believe that the bindings this patch adds are correct. As for putting it in a type-C connector node, currently we do not have such a node, Well, you should. Type-C connectors are certainly complicated enough that we'll need one. Plus we already require connector nodes for display outputs, so what do we do once you add display muxing? An interesting question, I'm working on this for x86 + ACPI boards actually, not a board using DT I've been adding DT bindings docs for device-properties I use because that seems like the right thing to do where the binding is obvious (which I believe it is in this case as the fusb302 is the mux consumer) and because the device-property code should work the same on x86 + ACPI (where some platform-specific drivers attach the device properties) and on e.g. ARM + DT. The rest should probably be left to be figured out when an actual DT using device using the fusb302 or another Type-C controller shows up. the closest thing we do have to a node describing it is actually the fusb302 node itself. E.g. it may also contain a regulator to turn Vbus on / off (already there in the code, but I forgot to document this when I added the missing DT bindings doc for the fusb302 with a previous patch). Either you can have a vbus-supply property in connector node or it can be implied that the controller chip provides that. For example, HDMI connectors have a hpd-gpios property if HPD is connected to GPIO or they have nothing and it's implicit that the HDMI encoder handles HPD. Also these properties: - fcs,max-sink-microvolt : Maximum voltage to negotiate when configured as sink - fcs,max-sink-microamp : Maximum current to negotiate when configured as sink - fcs,max-sink-microwatt : Maximum power to negotiate when configured as sink Have more to do with the charger-IC used (which determines the limits) then with the fusb302 itself, but the fusb302 needs to know these as it negotiates the limits. Those should probably be elsewhere and not be fusb302 specific. I did ack that, but it was a single node for a single component which is fine. But once we start adding more external pieces we need to pay more attention to the overall structure. Likewise the fusb302 negotiates how the data pins will be used and thus to which pins on the SoC the mux should mux the data pins. TL;DR: The fusb302 does all the negotiation and ties all the Type-C connected ICs together, so this seems like the right place for it (it certainly is the natural place to put these from a driver code pov). Things in DT should follow what the h/w design looks like which is not necessarily aligned with the driver structure. If the USB PD chip needs information from the charger, then we need a kernel interface for that. Well this really is board specific data, the charger IC may be handle for example up to 17V, but
Re: [PATCH v2 10/11] staging: typec: fusb302: Hook up mux support using tcpc_gen_mux support
On Wed, Sep 13, 2017 at 9:06 AM, Hans de Goede wrote: > Hi, > > > On 13-09-17 15:38, Rob Herring wrote: >> >> On Wed, Sep 13, 2017 at 3:56 AM, Hans de Goede >> wrote: >>> >>> Hi, >>> >>> >>> On 13-09-17 00:20, Rob Herring wrote: On Tue, Sep 05, 2017 at 06:42:20PM +0200, Hans de Goede wrote: > > > Add mux support to the fusb302 driver, call devm_tcpc_gen_mux_create() > to let the generic tcpc_mux_dev code create a tcpc_mux_dev for us. > > Also document the mux-names used by the generic tcpc_mux_dev code in > our devicetree bindings. > > Cc: Rob Herring > Cc: Mark Rutland > Cc: devicet...@vger.kernel.org > Signed-off-by: Hans de Goede > --- >Documentation/devicetree/bindings/usb/fcs,fusb302.txt | 3 +++ >drivers/staging/typec/fusb302/fusb302.c | 11 > ++- >2 files changed, 13 insertions(+), 1 deletion(-) > > diff --git a/Documentation/devicetree/bindings/usb/fcs,fusb302.txt > b/Documentation/devicetree/bindings/usb/fcs,fusb302.txt > index 472facfa5a71..63d639eadacd 100644 > --- a/Documentation/devicetree/bindings/usb/fcs,fusb302.txt > +++ b/Documentation/devicetree/bindings/usb/fcs,fusb302.txt > @@ -6,6 +6,9 @@ Required properties : >- interrupts : Interrupt specifier > Optional properties : > +- mux-controls : List of mux-ctrl-specifiers containing 1 or > 2 > muxes > +- mux-names : "type-c-mode-mux" when using 1 mux, or > + "type-c-mode-mux", "usb-role-mux" when > using > 2 muxes I'm not sure this is the right place for this. The mux is outside the FUSB302. In a type-C connector node or USB phy node would make more sense to me. >>> >>> >>> >>> The mux certainly does not belong in the USB phy node, it sits between >>> the >>> USB PHY >>> and the Type-C connector and can for example also mux the Type-C pins to >>> a >>> Display >>> Port PHY. >> >> >> Thinking about this some more, the mux(es) should be its own node(s). >> Then the question is where to put the nodes. > > > Right, the mux will be its own node per > Documentation/devicetree/bindings/mux/mux-controller.txt > the bindings bit this patch is adding is only adding phandles pointing > to that mux-node as the fusb302 "consumes" the mux functionality. > > So as such (the fusb302 is the component which should control the mux) > I do believe that the bindings this patch adds are correct. Humm, that's not how the mux binding works. The mux controller is what drives the mux select lines and is the provider. The consumer is the mux device itself. What decides the mux state is determined by what you are muxing, not which node has mux-controls property. By putting mux-controls in fusb302 node, you are saying fusb302 is a mux (or contains a mux). >>> As for putting it in a type-C connector node, currently we do not have >>> such >>> a node, >> >> >> Well, you should. Type-C connectors are certainly complicated enough >> that we'll need one. Plus we already require connector nodes for >> display outputs, so what do we do once you add display muxing? > > > An interesting question, I'm working on this for x86 + ACPI boards actually, > not a board using DT I've been adding DT bindings docs for device-properties > I use because that seems like the right thing to do where the binding is > obvious > (which I believe it is in this case as the fusb302 is the mux consumer) and > because the device-property code should work the same on x86 + ACPI > (where some platform-specific drivers attach the device properties) and > on e.g. ARM + DT. > > The rest should probably be left to be figured out when an actual DT > using device using the fusb302 or another Type-C controller shows up. Well this is a new one (maybe, I suppose others have sneaked by). If ACPI folks want to use DT bindings, then what do I care. But I have no interest in reviewing ACPI properties. The whole notion of sharing bindings between DT and ACPI beyond anything trivial is flawed IMO. The ptifalls have been discussed multiple times before, so I'm not going to repeat them here. >>> the closest thing we do have to a node describing it is actually the >>> fusb302 >>> node >>> itself. E.g. it may also contain a regulator to turn Vbus on / off >>> (already >>> there >>> in the code, but I forgot to document this when I added the missing DT >>> bindings >>> doc for the fusb302 with a previous patch). >> >> >> Either you can have a vbus-supply property in connector node or it can >> be implied that the controller chip provides that. For example, HDMI >> connectors have a hpd-gpios property if HPD is connected to GPIO or >> they have nothing and it's implicit that the HDMI encoder handles HPD. >> >> >>> Also these properties: >>> >- fcs,max-sink-microvolt : Maximum voltage to negotiate when > configured > as sin
Re: [PATCH] staging: rtl8712: Fix unbalanced braces around else statement
On Wed, Sep 13, 2017 at 08:47:39AM +0200, Frans Klaver wrote:
> On Tue, Sep 12, 2017 at 2:40 AM, Liam Ryan wrote:
> > Fix checkpath-reported unbalanced braces in the following areas
> >
> > 221: FILE: drivers/staging/rtl8712/hal_init.c:221:
> > 392: FILE: drivers/staging/rtl8712/os_intfs.c:392:
> > 363: FILE: drivers/staging/rtl8712/rtl8712_cmd.c:363:
> > 889: FILE: drivers/staging/rtl8712/rtl8712_recv.c:889:
> > 902: FILE: drivers/staging/rtl8712/rtl871x_cmd.c:902:
> > 84: FILE: drivers/staging/rtl8712/rtl871x_ioctl_set.c:84:
> > 580: FILE: drivers/staging/rtl8712/rtl871x_mlme.c:580:
> > 593: FILE: drivers/staging/rtl8712/usb_intf.c:593:
> >
> > Signed-off-by: Liam Ryan
> > ---
> > This is my first patch and I have several doubts about style choices
> >
> > At line 216 of hal_init.c should opening brace follow comment instead?
> >
> > At line 577 of rtl871x_mlme.c should I bring logic to one line instead of
> > opening the brace on the continued line?
> >
> > At line 353 of rtl8712_cmd.c the if/else is technically only 1 line each.
> > Should the braces still have been added per checkpath for readability?
> >
> > drivers/staging/rtl8712/hal_init.c | 4 ++--
> > drivers/staging/rtl8712/os_intfs.c | 4 ++--
> > drivers/staging/rtl8712/rtl8712_cmd.c | 4 ++--
> > drivers/staging/rtl8712/rtl8712_recv.c | 4 ++--
> > drivers/staging/rtl8712/rtl871x_cmd.c | 3 ++-
> > drivers/staging/rtl8712/rtl871x_ioctl_set.c | 4 ++--
> > drivers/staging/rtl8712/rtl871x_mlme.c | 4 ++--
> > drivers/staging/rtl8712/usb_intf.c | 3 ++-
> > 8 files changed, 16 insertions(+), 14 deletions(-)
> >
> > diff --git a/drivers/staging/rtl8712/hal_init.c
> > b/drivers/staging/rtl8712/hal_init.c
> > index c83d7eb..de832b0b5 100644
> > --- a/drivers/staging/rtl8712/hal_init.c
> > +++ b/drivers/staging/rtl8712/hal_init.c
> > @@ -216,9 +216,9 @@ static u8 rtl8712_dl_fw(struct _adapter *padapter)
> > emem_sz = fwhdr.img_SRAM_size;
> > do {
> > memset(ptx_desc, 0, TXDESC_SIZE);
> > - if (emem_sz > MAX_DUMP_FWSZ) /* max=48k */
> > + if (emem_sz > MAX_DUMP_FWSZ) { /* max=48k */
>
> I would not have the comment there in the first place. It doesn't add
> any new information and just messes up the style.
I'm happy to remove the comment, the patch was accepted so I'm not sure
of procedure, should it be a new patch or a revision to this patch?
In general I don't have the knowledge to decide which comments are worth
keeping in the source
code, is there a rule of thumb for brace placement near inline comments such as
this? There's
another example in rtl8712_cmd.c below for reference.
>
> > dump_emem_sz = MAX_DUMP_FWSZ;
> > - else {
> > + } else {
> > dump_emem_sz = emem_sz;
> > ptx_desc->txdw0 |= cpu_to_le32(BIT(28));
> > }
> > diff --git a/drivers/staging/rtl8712/os_intfs.c
> > b/drivers/staging/rtl8712/os_intfs.c
> > index e698f6e..990a343 100644
> > --- a/drivers/staging/rtl8712/os_intfs.c
> > +++ b/drivers/staging/rtl8712/os_intfs.c
> > @@ -385,11 +385,11 @@ static int netdev_open(struct net_device *pnetdev)
> > padapter->bup = true;
> > if (rtl871x_hal_init(padapter) != _SUCCESS)
> > goto netdev_open_error;
> > - if (!r8712_initmac)
> > + if (!r8712_initmac) {
> > /* Use the mac address stored in the Efuse */
> > memcpy(pnetdev->dev_addr,
> >padapter->eeprompriv.mac_addr, ETH_ALEN);
> > - else {
> > + } else {
> > /* We have to inform f/w to use user-supplied MAC
> > * address.
> > */
> > diff --git a/drivers/staging/rtl8712/rtl8712_cmd.c
> > b/drivers/staging/rtl8712/rtl8712_cmd.c
> > index 0104ace..3c88994 100644
> > --- a/drivers/staging/rtl8712/rtl8712_cmd.c
> > +++ b/drivers/staging/rtl8712/rtl8712_cmd.c
> > @@ -356,11 +356,11 @@ int r8712_cmd_thread(void *context)
> > if ((wr_sz % 64) == 0)
> > blnPending = 1;
> > }
> > - if (blnPending) /* 32 bytes for TX Desc - 8 offset
> > */
> > + if (blnPending) { /* 32 bytes for TX Desc - 8
> > offset */
> > pdesc->txdw0 |= cpu_to_le32(((TXDESC_SIZE +
> > OFFSET_SZ + 8) <<
> > OFFSET_SHT) &
> > 0x00ff);
> > - else {
> > + } else {
> > pdesc->txdw0 |= cpu_to_le32(((TXDESC
Re: [PATCH v2 10/11] staging: typec: fusb302: Hook up mux support using tcpc_gen_mux support
Hi, On 13-09-17 17:07, Rob Herring wrote: On Wed, Sep 13, 2017 at 9:06 AM, Hans de Goede wrote: Hi, On 13-09-17 15:38, Rob Herring wrote: On Wed, Sep 13, 2017 at 3:56 AM, Hans de Goede wrote: Hi, On 13-09-17 00:20, Rob Herring wrote: On Tue, Sep 05, 2017 at 06:42:20PM +0200, Hans de Goede wrote: Add mux support to the fusb302 driver, call devm_tcpc_gen_mux_create() to let the generic tcpc_mux_dev code create a tcpc_mux_dev for us. Also document the mux-names used by the generic tcpc_mux_dev code in our devicetree bindings. Cc: Rob Herring Cc: Mark Rutland Cc: devicet...@vger.kernel.org Signed-off-by: Hans de Goede --- Documentation/devicetree/bindings/usb/fcs,fusb302.txt | 3 +++ drivers/staging/typec/fusb302/fusb302.c | 11 ++- 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/Documentation/devicetree/bindings/usb/fcs,fusb302.txt b/Documentation/devicetree/bindings/usb/fcs,fusb302.txt index 472facfa5a71..63d639eadacd 100644 --- a/Documentation/devicetree/bindings/usb/fcs,fusb302.txt +++ b/Documentation/devicetree/bindings/usb/fcs,fusb302.txt @@ -6,6 +6,9 @@ Required properties : - interrupts : Interrupt specifier Optional properties : +- mux-controls : List of mux-ctrl-specifiers containing 1 or 2 muxes +- mux-names : "type-c-mode-mux" when using 1 mux, or + "type-c-mode-mux", "usb-role-mux" when using 2 muxes I'm not sure this is the right place for this. The mux is outside the FUSB302. In a type-C connector node or USB phy node would make more sense to me. The mux certainly does not belong in the USB phy node, it sits between the USB PHY and the Type-C connector and can for example also mux the Type-C pins to a Display Port PHY. Thinking about this some more, the mux(es) should be its own node(s). Then the question is where to put the nodes. Right, the mux will be its own node per Documentation/devicetree/bindings/mux/mux-controller.txt the bindings bit this patch is adding is only adding phandles pointing to that mux-node as the fusb302 "consumes" the mux functionality. So as such (the fusb302 is the component which should control the mux) I do believe that the bindings this patch adds are correct. Humm, that's not how the mux binding works. The mux controller is what drives the mux select lines and is the provider. The consumer is the mux device itself. What decides the mux state is determined by what you are muxing, not which node has mux-controls property. By putting mux-controls in fusb302 node, you are saying fusb302 is a mux (or contains a mux). As for putting it in a type-C connector node, currently we do not have such a node, Well, you should. Type-C connectors are certainly complicated enough that we'll need one. Plus we already require connector nodes for display outputs, so what do we do once you add display muxing? An interesting question, I'm working on this for x86 + ACPI boards actually, not a board using DT I've been adding DT bindings docs for device-properties I use because that seems like the right thing to do where the binding is obvious (which I believe it is in this case as the fusb302 is the mux consumer) and because the device-property code should work the same on x86 + ACPI (where some platform-specific drivers attach the device properties) and on e.g. ARM + DT. The rest should probably be left to be figured out when an actual DT using device using the fusb302 or another Type-C controller shows up. Well this is a new one (maybe, I suppose others have sneaked by). If ACPI folks want to use DT bindings, then what do I care. But I have no interest in reviewing ACPI properties. The whole notion of sharing bindings between DT and ACPI beyond anything trivial is flawed IMO. The ptifalls have been discussed multiple times before, so I'm not going to repeat them here. Ok, so shall I just drop the Documentation/devicetree/bindings/usb/fcs,fusb302.txt part of this patch then ? Regards, Hans ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: rtl8712: Fix unbalanced braces around else statement
On Wed, Sep 13, 2017 at 04:14:29PM +0100, Liam Ryan wrote:
> On Wed, Sep 13, 2017 at 08:47:39AM +0200, Frans Klaver wrote:
> > On Tue, Sep 12, 2017 at 2:40 AM, Liam Ryan wrote:
> > > Fix checkpath-reported unbalanced braces in the following areas
> > >
> > > 221: FILE: drivers/staging/rtl8712/hal_init.c:221:
> > > 392: FILE: drivers/staging/rtl8712/os_intfs.c:392:
> > > 363: FILE: drivers/staging/rtl8712/rtl8712_cmd.c:363:
> > > 889: FILE: drivers/staging/rtl8712/rtl8712_recv.c:889:
> > > 902: FILE: drivers/staging/rtl8712/rtl871x_cmd.c:902:
> > > 84: FILE: drivers/staging/rtl8712/rtl871x_ioctl_set.c:84:
> > > 580: FILE: drivers/staging/rtl8712/rtl871x_mlme.c:580:
> > > 593: FILE: drivers/staging/rtl8712/usb_intf.c:593:
> > >
> > > Signed-off-by: Liam Ryan
> > > ---
> > > This is my first patch and I have several doubts about style choices
> > >
> > > At line 216 of hal_init.c should opening brace follow comment instead?
> > >
> > > At line 577 of rtl871x_mlme.c should I bring logic to one line instead of
> > > opening the brace on the continued line?
> > >
> > > At line 353 of rtl8712_cmd.c the if/else is technically only 1 line each.
> > > Should the braces still have been added per checkpath for readability?
> > >
> > > drivers/staging/rtl8712/hal_init.c | 4 ++--
> > > drivers/staging/rtl8712/os_intfs.c | 4 ++--
> > > drivers/staging/rtl8712/rtl8712_cmd.c | 4 ++--
> > > drivers/staging/rtl8712/rtl8712_recv.c | 4 ++--
> > > drivers/staging/rtl8712/rtl871x_cmd.c | 3 ++-
> > > drivers/staging/rtl8712/rtl871x_ioctl_set.c | 4 ++--
> > > drivers/staging/rtl8712/rtl871x_mlme.c | 4 ++--
> > > drivers/staging/rtl8712/usb_intf.c | 3 ++-
> > > 8 files changed, 16 insertions(+), 14 deletions(-)
> > >
> > > diff --git a/drivers/staging/rtl8712/hal_init.c
> > > b/drivers/staging/rtl8712/hal_init.c
> > > index c83d7eb..de832b0b5 100644
> > > --- a/drivers/staging/rtl8712/hal_init.c
> > > +++ b/drivers/staging/rtl8712/hal_init.c
> > > @@ -216,9 +216,9 @@ static u8 rtl8712_dl_fw(struct _adapter *padapter)
> > > emem_sz = fwhdr.img_SRAM_size;
> > > do {
> > > memset(ptx_desc, 0, TXDESC_SIZE);
> > > - if (emem_sz > MAX_DUMP_FWSZ) /* max=48k */
> > > + if (emem_sz > MAX_DUMP_FWSZ) { /* max=48k */
> >
> > I would not have the comment there in the first place. It doesn't add
> > any new information and just messes up the style.
> I'm happy to remove the comment, the patch was accepted so I'm not sure
> of procedure, should it be a new patch or a revision to this patch?
>
> In general I don't have the knowledge to decide which comments are worth
> keeping in the source
> code.
While you are getting started, if you are unsure of things I tend to lean
towards making the minimal
change to improve the code. A patch will be rejected if there are obvious extra
fixes that need
doing. Review will point these out, reviewers generally don't mind doing this
because that is how you
learn. Please be sure to carefully study these suggestions and learn from them
so review does not
have to point them out again unnecessarily.
> is there a rule of thumb for brace placement near inline comments such as
> this?
Like Frans said; If there is more than one line of code (irrelevant to whether
it is comments or a
single statement over two lines) braces make the code cleaner.
Hope this helps,
Tobin.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 10/11] staging: typec: fusb302: Hook up mux support using tcpc_gen_mux support
On Wed, Sep 13, 2017 at 05:48:25PM +0200, Hans de Goede wrote: > Hi, > > On 13-09-17 17:07, Rob Herring wrote: > >On Wed, Sep 13, 2017 at 9:06 AM, Hans de Goede wrote: > >>Hi, > >> > >> > >>On 13-09-17 15:38, Rob Herring wrote: > >>> > >>>On Wed, Sep 13, 2017 at 3:56 AM, Hans de Goede > >>>wrote: > > Hi, > > > On 13-09-17 00:20, Rob Herring wrote: > > > > > >On Tue, Sep 05, 2017 at 06:42:20PM +0200, Hans de Goede wrote: > >> > >> > >>Add mux support to the fusb302 driver, call devm_tcpc_gen_mux_create() > >>to let the generic tcpc_mux_dev code create a tcpc_mux_dev for us. > >> > >>Also document the mux-names used by the generic tcpc_mux_dev code in > >>our devicetree bindings. > >> > >>Cc: Rob Herring > >>Cc: Mark Rutland > >>Cc: devicet...@vger.kernel.org > >>Signed-off-by: Hans de Goede > >>--- > >>Documentation/devicetree/bindings/usb/fcs,fusb302.txt | 3 +++ > >>drivers/staging/typec/fusb302/fusb302.c | 11 > >>++- > >>2 files changed, 13 insertions(+), 1 deletion(-) > >> > >>diff --git a/Documentation/devicetree/bindings/usb/fcs,fusb302.txt > >>b/Documentation/devicetree/bindings/usb/fcs,fusb302.txt > >>index 472facfa5a71..63d639eadacd 100644 > >>--- a/Documentation/devicetree/bindings/usb/fcs,fusb302.txt > >>+++ b/Documentation/devicetree/bindings/usb/fcs,fusb302.txt > >>@@ -6,6 +6,9 @@ Required properties : > >>- interrupts : Interrupt specifier > >> Optional properties : > >>+- mux-controls : List of mux-ctrl-specifiers containing 1 or > >>2 > >>muxes > >>+- mux-names : "type-c-mode-mux" when using 1 mux, or > >>+ "type-c-mode-mux", "usb-role-mux" when > >>using > >>2 muxes > > > > > > > >I'm not sure this is the right place for this. The mux is outside the > >FUSB302. In a type-C connector node or USB phy node would make more > >sense to me. > > > > The mux certainly does not belong in the USB phy node, it sits between > the > USB PHY > and the Type-C connector and can for example also mux the Type-C pins to > a > Display > Port PHY. > >>> > >>> > >>>Thinking about this some more, the mux(es) should be its own node(s). > >>>Then the question is where to put the nodes. > >> > >> > >>Right, the mux will be its own node per > >>Documentation/devicetree/bindings/mux/mux-controller.txt > >>the bindings bit this patch is adding is only adding phandles pointing > >>to that mux-node as the fusb302 "consumes" the mux functionality. > >> > >>So as such (the fusb302 is the component which should control the mux) > >>I do believe that the bindings this patch adds are correct. > > > >Humm, that's not how the mux binding works. The mux controller is what > >drives the mux select lines and is the provider. The consumer is the > >mux device itself. What decides the mux state is determined by what > >you are muxing, not which node has mux-controls property. > > > >By putting mux-controls in fusb302 node, you are saying fusb302 is a > >mux (or contains a mux). > > > > > As for putting it in a type-C connector node, currently we do not have > such > a node, > >>> > >>> > >>>Well, you should. Type-C connectors are certainly complicated enough > >>>that we'll need one. Plus we already require connector nodes for > >>>display outputs, so what do we do once you add display muxing? > >> > >> > >>An interesting question, I'm working on this for x86 + ACPI boards actually, > >>not a board using DT I've been adding DT bindings docs for device-properties > >>I use because that seems like the right thing to do where the binding is > >>obvious > >>(which I believe it is in this case as the fusb302 is the mux consumer) and > >>because the device-property code should work the same on x86 + ACPI > >>(where some platform-specific drivers attach the device properties) and > >>on e.g. ARM + DT. > >> > >>The rest should probably be left to be figured out when an actual DT > >>using device using the fusb302 or another Type-C controller shows up. > > > >Well this is a new one (maybe, I suppose others have sneaked by). If > >ACPI folks want to use DT bindings, then what do I care. But I have no > >interest in reviewing ACPI properties. The whole notion of sharing > >bindings between DT and ACPI beyond anything trivial is flawed IMO. > >The ptifalls have been discussed multiple times before, so I'm not > >going to repeat them here. > > Ok, so shall I just drop the > Documentation/devicetree/bindings/usb/fcs,fusb302.txt > part of this patch then ? > FWIW, Android (where the fusb302 driver is coming from) does use dt. On the other side I assume they won't jump on the new mux handling immediately. I won't have time to look into it myself, so whatever is done here may not match the "real"
[PATCH] staging: iio: ade7759: fix signed extension bug on shift of a u8
From: Colin Ian King
The current shift of st->rx[2] left shifts a u8 24 bits left,
promotes the integer to a an int and then to a unsigned u64. If
the top bit of st->rx[2] is set then we end up with all the upper
bits being set to 1. Fix this by casting st->rx[2] to a u64 before
the 24 bit left shift.
Detected by CoverityScan CID#144940 ("Unintended sign extension")
Fixes: 2919fa54ef64 ("staging: iio: meter: new driver for ADE7759 devices")
Signed-off-by: Colin Ian King
---
drivers/staging/iio/meter/ade7759.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/staging/iio/meter/ade7759.c
b/drivers/staging/iio/meter/ade7759.c
index 1691760339da..02573c517d9d 100644
--- a/drivers/staging/iio/meter/ade7759.c
+++ b/drivers/staging/iio/meter/ade7759.c
@@ -172,7 +172,7 @@ static int ade7759_spi_read_reg_40(struct device *dev,
reg_address);
goto error_ret;
}
- *val = ((u64)st->rx[1] << 32) | (st->rx[2] << 24) |
+ *val = ((u64)st->rx[1] << 32) | ((u64)st->rx[2] << 24) |
(st->rx[3] << 16) | (st->rx[4] << 8) | st->rx[5];
error_ret:
--
2.14.1
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 1/2] include: linux: sysfs: Add __ATTR_NAMED macro
On Wed, 13 Sep 2017 14:14:07 +0530
Himanshi Jain wrote:
> Add __ATTR_NAMED macro similar to __ATTR but taking name as a
> string instead of implicit conversion of argument to string using
> the macro _stringify(_name).
>
> Signed-off-by: Himanshi Jain
> ---
> include/linux/sysfs.h | 7 +++
> 1 file changed, 7 insertions(+)
>
> diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
> index aa02c32..20321cf 100644
> --- a/include/linux/sysfs.h
> +++ b/include/linux/sysfs.h
> @@ -104,6 +104,13 @@ struct attribute_group {
> .store = _store, \
> }
>
> +#define __ATTR_NAMED(_name, _mode, _show, _store) { \
I'm not sure about the naming here. The normal __ATTR macro is also
'named'. Maybe something as awful as
__ATTR_STRING_NAME ?
Greg what do you think?
This is all to allow us to have names with operators in them without
checkpatch complaining about them... A worthwhile aim just to stop
more people wasting time trying to 'fix' those cases by adding spaces.
Jonathan
> + .attr = {.name = _name, \
> + .mode = VERIFY_OCTAL_PERMISSIONS(_mode) }, \
> + .show = _show, \
> + .store = _store,\
> +}
> +
> #define __ATTR_PREALLOC(_name, _mode, _show, _store) {
> \
> .attr = {.name = __stringify(_name),\
>.mode = SYSFS_PREALLOC | VERIFY_OCTAL_PERMISSIONS(_mode) },\
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [Outreachy kernel] [PATCH v2 0/2] Rewrite the IIO_DEVICE_ATTR_NAMED API to pass name as string.
On Wed, 13 Sep 2017 10:46:59 +0200 (CEST) Julia Lawall wrote: > On Wed, 13 Sep 2017, Himanshi Jain wrote: > > > This patchset is to rewrite the IIO_DEVICE_ATTR_NAMED API to pass name > > as string. > > You need to indicate what has changed in the v2, either here or in the > individual patches. > > julia Absolutely agree with Julia. However, I'd wait for a day or so to see if anything else comes up rather than immediately sending a v3 with a correct change log. Thanks, Jonathan > > > > > > Himanshi Jain (2): > > include: linux: sysfs: Add __ATTR_NAMED macro > > iio: Change to __ATTR_NAMED() > > > > drivers/iio/adc/ad7793.c | 2 +- > > drivers/staging/iio/adc/ad7192.c | 2 +- > > drivers/staging/iio/adc/ad7280a.c | 4 ++-- > > include/linux/iio/sysfs.h | 6 +- > > include/linux/sysfs.h | 7 +++ > > 5 files changed, 16 insertions(+), 5 deletions(-) > > > > -- > > 1.9.1 > > > > -- > > You received this message because you are subscribed to the Google Groups > > "outreachy-kernel" group. > > To unsubscribe from this group and stop receiving emails from it, send an > > email to outreachy-kernel+unsubscr...@googlegroups.com. > > To post to this group, send email to outreachy-ker...@googlegroups.com. > > To view this discussion on the web visit > > https://groups.google.com/d/msgid/outreachy-kernel/cover.1505291907.git.himshijain.hj%40gmail.com. > > For more options, visit https://groups.google.com/d/optout. > > ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
RE: [PATCH 1/1] x86/hyper-V: Allocate the IDT entry early in boot
> -Original Message- > From: Ingo Molnar [mailto:mingo.kernel@gmail.com] On Behalf Of Ingo > Molnar > Sent: Wednesday, September 13, 2017 2:01 AM > To: KY Srinivasan > Cc: Dan Carpenter ; x...@kernel.org; > gre...@linuxfoundation.org; linux-ker...@vger.kernel.org; > de...@linuxdriverproject.org; o...@aepfle.de; a...@canonical.com; > jasow...@redhat.com; t...@linutronix.de; h...@zytor.com > Subject: Re: [PATCH 1/1] x86/hyper-V: Allocate the IDT entry early in boot > > > * KY Srinivasan wrote: > > > > > > > > -Original Message- > > > From: Dan Carpenter [mailto:dan.carpen...@oracle.com] > > > Sent: Saturday, September 9, 2017 4:04 AM > > > To: KY Srinivasan > > > Cc: x...@kernel.org; gre...@linuxfoundation.org; linux- > > > ker...@vger.kernel.org; de...@linuxdriverproject.org; o...@aepfle.de; > > > a...@canonical.com; jasow...@redhat.com; t...@linutronix.de; > > > h...@zytor.com; mi...@kernel.org > > > Subject: Re: [PATCH 1/1] x86/hyper-V: Allocate the IDT entry early in boot > > > > > > On Fri, Sep 08, 2017 at 04:15:57PM -0700, k...@exchange.microsoft.com > > > wrote: > > > > From: "K. Y. Srinivasan" > > > > > > > > Allocate the hypervisor callback IDT entry early in the boot sequence. > > > > > > > > > > I'm guessing this fixes a NULL dereference or something? The changelog > > > doesn't really say why we are doing this. > > > > The changelog does say what we are doing - allocating the IDT entry early in > the boot sequence. > > But the question was the 'why', not the 'what' - so Dan's question is > fully justified ... > > > The current code would allocate the entry as part of registering the handler > > when vmbus driver loaded and this caused a problem for the cleanup > Thomas had > > implemented. > > I've put this explanation into the changelog. You are right. Thanks, Ingo. K. Y > > Thanks, > > Ingo ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH] tools: hv: hv_kvp_daemon: fix usage of realloc()
realloc() returns NULL in case it fails. Since we don't save the
pointer in question elsewhere, we leak memory by assigning NULL
to the original memory in the heap.
realloc() doesn't free memory in case of failure, so let's do
it manually.
Signed-off-by: Martin Kepplinger
---
tools/hv/hv_kvp_daemon.c | 17 +
1 file changed, 13 insertions(+), 4 deletions(-)
diff --git a/tools/hv/hv_kvp_daemon.c b/tools/hv/hv_kvp_daemon.c
index eaa3bec273c8..0b3b18d0a6e3 100644
--- a/tools/hv/hv_kvp_daemon.c
+++ b/tools/hv/hv_kvp_daemon.c
@@ -231,6 +231,7 @@ static int kvp_file_init(void)
size_t records_read;
char *fname;
struct kvp_record *record;
+ struct kvp_record *record_tmp;
struct kvp_record *readp;
int num_blocks;
int i;
@@ -284,12 +285,15 @@ static int kvp_file_init(void)
* We have more data to read.
*/
num_blocks++;
- record = realloc(record, alloc_unit *
+ record_tmp = realloc(record, alloc_unit *
num_blocks);
- if (record == NULL) {
+ if (!record_tmp) {
+ free(record);
fclose(filep);
close(fd);
return 1;
+ } else {
+ record = record_tmp;
}
continue;
}
@@ -355,6 +359,7 @@ static int kvp_key_add_or_modify(int pool, const __u8 *key,
int key_size,
int i;
int num_records;
struct kvp_record *record;
+ struct kvp_record *record_tmp;
int num_blocks;
if ((key_size > HV_KVP_EXCHANGE_MAX_KEY_SIZE) ||
@@ -387,11 +392,15 @@ static int kvp_key_add_or_modify(int pool, const __u8
*key, int key_size,
*/
if (num_records == (ENTRIES_PER_BLOCK * num_blocks)) {
/* Need to allocate a larger array for reg entries. */
- record = realloc(record, sizeof(struct kvp_record) *
+ record_tmp = realloc(record, sizeof(struct kvp_record) *
ENTRIES_PER_BLOCK * (num_blocks + 1));
- if (record == NULL)
+ if (!record_tmp) {
+ free(record);
return 1;
+ } else {
+ record = record_tmp;
+ }
kvp_file_info[pool].num_blocks++;
}
--
2.11.0
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 1/2] include: linux: sysfs: Add __ATTR_NAMED macro
On Wed, Sep 13, 2017 at 06:03:10PM +0100, Jonathan Cameron wrote:
> On Wed, 13 Sep 2017 14:14:07 +0530
> Himanshi Jain wrote:
>
> > Add __ATTR_NAMED macro similar to __ATTR but taking name as a
> > string instead of implicit conversion of argument to string using
> > the macro _stringify(_name).
> >
> > Signed-off-by: Himanshi Jain
> > ---
> > include/linux/sysfs.h | 7 +++
> > 1 file changed, 7 insertions(+)
> >
> > diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
> > index aa02c32..20321cf 100644
> > --- a/include/linux/sysfs.h
> > +++ b/include/linux/sysfs.h
> > @@ -104,6 +104,13 @@ struct attribute_group {
> > .store = _store, \
> > }
> >
> > +#define __ATTR_NAMED(_name, _mode, _show, _store) {
> > \
>
> I'm not sure about the naming here. The normal __ATTR macro is also
> 'named'. Maybe something as awful as
>
> __ATTR_STRING_NAME ?
>
> Greg what do you think?
ick ick ick.
> This is all to allow us to have names with operators in them without
> checkpatch complaining about them... A worthwhile aim just to stop
> more people wasting time trying to 'fix' those cases by adding spaces.
Yeah, but this really seems "heavy" for just a crazy sysfs name in a
macro. Adding a whole new "core" define for that is a hard sell...
I also want to get rid of the "generic" __ATTR type macros, and force
people to use the proper _RW and friends instead. I don't want to add
another new one that people will start to use that I later have to
change...
So no, I don't like this, how about just changing your macros instead?
No one else has this problem :)
thanks,
greg k-h
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 1/2] include: linux: sysfs: Add __ATTR_NAMED macro
On 09/13/2017 08:58 PM, Greg KH wrote:
> On Wed, Sep 13, 2017 at 06:03:10PM +0100, Jonathan Cameron wrote:
>> On Wed, 13 Sep 2017 14:14:07 +0530
>> Himanshi Jain wrote:
>>
>>> Add __ATTR_NAMED macro similar to __ATTR but taking name as a
>>> string instead of implicit conversion of argument to string using
>>> the macro _stringify(_name).
>>>
>>> Signed-off-by: Himanshi Jain
>>> ---
>>> include/linux/sysfs.h | 7 +++
>>> 1 file changed, 7 insertions(+)
>>>
>>> diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
>>> index aa02c32..20321cf 100644
>>> --- a/include/linux/sysfs.h
>>> +++ b/include/linux/sysfs.h
>>> @@ -104,6 +104,13 @@ struct attribute_group {
>>> .store = _store, \
>>> }
>>>
>>> +#define __ATTR_NAMED(_name, _mode, _show, _store) {
>>> \
>>
>> I'm not sure about the naming here. The normal __ATTR macro is also
>> 'named'. Maybe something as awful as
>>
>> __ATTR_STRING_NAME ?
>>
>> Greg what do you think?
>
> ick ick ick.
>
>> This is all to allow us to have names with operators in them without
>> checkpatch complaining about them... A worthwhile aim just to stop
>> more people wasting time trying to 'fix' those cases by adding spaces.
>
> Yeah, but this really seems "heavy" for just a crazy sysfs name in a
> macro. Adding a whole new "core" define for that is a hard sell...
>
> I also want to get rid of the "generic" __ATTR type macros, and force
> people to use the proper _RW and friends instead. I don't want to add
> another new one that people will start to use that I later have to
> change...
>
> So no, I don't like this, how about just changing your macros instead?
> No one else has this problem :)
Nobody else realized they have this problem yet. E.g. there are a few users
of __ATTR in block/genhd.c that have the same issue and are likely to
generate the same false positives from static checkers.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 1/2] include: linux: sysfs: Add __ATTR_NAMED macro
On Wed, Sep 13, 2017 at 09:23:31PM +0200, Lars-Peter Clausen wrote:
> On 09/13/2017 08:58 PM, Greg KH wrote:
> > On Wed, Sep 13, 2017 at 06:03:10PM +0100, Jonathan Cameron wrote:
> >> On Wed, 13 Sep 2017 14:14:07 +0530
> >> Himanshi Jain wrote:
> >>
> >>> Add __ATTR_NAMED macro similar to __ATTR but taking name as a
> >>> string instead of implicit conversion of argument to string using
> >>> the macro _stringify(_name).
> >>>
> >>> Signed-off-by: Himanshi Jain
> >>> ---
> >>> include/linux/sysfs.h | 7 +++
> >>> 1 file changed, 7 insertions(+)
> >>>
> >>> diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
> >>> index aa02c32..20321cf 100644
> >>> --- a/include/linux/sysfs.h
> >>> +++ b/include/linux/sysfs.h
> >>> @@ -104,6 +104,13 @@ struct attribute_group {
> >>> .store = _store, \
> >>> }
> >>>
> >>> +#define __ATTR_NAMED(_name, _mode, _show, _store) {
> >>> \
> >>
> >> I'm not sure about the naming here. The normal __ATTR macro is also
> >> 'named'. Maybe something as awful as
> >>
> >> __ATTR_STRING_NAME ?
> >>
> >> Greg what do you think?
> >
> > ick ick ick.
> >
> >> This is all to allow us to have names with operators in them without
> >> checkpatch complaining about them... A worthwhile aim just to stop
> >> more people wasting time trying to 'fix' those cases by adding spaces.
> >
> > Yeah, but this really seems "heavy" for just a crazy sysfs name in a
> > macro. Adding a whole new "core" define for that is a hard sell...
> >
> > I also want to get rid of the "generic" __ATTR type macros, and force
> > people to use the proper _RW and friends instead. I don't want to add
> > another new one that people will start to use that I later have to
> > change...
> >
> > So no, I don't like this, how about just changing your macros instead?
> > No one else has this problem :)
>
> Nobody else realized they have this problem yet. E.g. there are a few users
> of __ATTR in block/genhd.c that have the same issue and are likely to
> generate the same false positives from static checkers.
Then fix the broken static checkers :)
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 1/2] include: linux: sysfs: Add __ATTR_NAMED macro
On Wed, Sep 13, 2017 at 02:29:52PM -0700, Greg KH wrote:
> On Wed, Sep 13, 2017 at 09:23:31PM +0200, Lars-Peter Clausen wrote:
> > On 09/13/2017 08:58 PM, Greg KH wrote:
> > > On Wed, Sep 13, 2017 at 06:03:10PM +0100, Jonathan Cameron wrote:
> > >> On Wed, 13 Sep 2017 14:14:07 +0530
> > >> Himanshi Jain wrote:
> > >>
> > >>> Add __ATTR_NAMED macro similar to __ATTR but taking name as a
> > >>> string instead of implicit conversion of argument to string using
> > >>> the macro _stringify(_name).
> > >>>
> > >>> Signed-off-by: Himanshi Jain
> > >>> ---
> > >>> include/linux/sysfs.h | 7 +++
> > >>> 1 file changed, 7 insertions(+)
> > >>>
> > >>> diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
> > >>> index aa02c32..20321cf 100644
> > >>> --- a/include/linux/sysfs.h
> > >>> +++ b/include/linux/sysfs.h
> > >>> @@ -104,6 +104,13 @@ struct attribute_group {
> > >>> .store = _store,
> > >>> \
> > >>> }
> > >>>
> > >>> +#define __ATTR_NAMED(_name, _mode, _show, _store) {
> > >>> \
> > >>
> > >> I'm not sure about the naming here. The normal __ATTR macro is also
> > >> 'named'. Maybe something as awful as
> > >>
> > >> __ATTR_STRING_NAME ?
> > >>
> > >> Greg what do you think?
> > >
> > > ick ick ick.
> > >
> > >> This is all to allow us to have names with operators in them without
> > >> checkpatch complaining about them... A worthwhile aim just to stop
> > >> more people wasting time trying to 'fix' those cases by adding spaces.
> > >
> > > Yeah, but this really seems "heavy" for just a crazy sysfs name in a
> > > macro. Adding a whole new "core" define for that is a hard sell...
> > >
> > > I also want to get rid of the "generic" __ATTR type macros, and force
> > > people to use the proper _RW and friends instead. I don't want to add
> > > another new one that people will start to use that I later have to
> > > change...
> > >
> > > So no, I don't like this, how about just changing your macros instead?
> > > No one else has this problem :)
> >
> > Nobody else realized they have this problem yet. E.g. there are a few users
> > of __ATTR in block/genhd.c that have the same issue and are likely to
> > generate the same false positives from static checkers.
>
> Then fix the broken static checkers :)
He was exagerating a bit to call it a "static checker" warning...
It's just checkpatch.pl complaining about adding spaces around the -
operator. The sysfs file has a hyphen in the middle.
regards,
dan carpenter
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 1/2] include: linux: sysfs: Add __ATTR_NAMED macro
On 13 September 2017 12:23:31 GMT-07:00, Lars-Peter Clausen
wrote:
>On 09/13/2017 08:58 PM, Greg KH wrote:
>> On Wed, Sep 13, 2017 at 06:03:10PM +0100, Jonathan Cameron wrote:
>>> On Wed, 13 Sep 2017 14:14:07 +0530
>>> Himanshi Jain wrote:
>>>
Add __ATTR_NAMED macro similar to __ATTR but taking name as a
string instead of implicit conversion of argument to string using
the macro _stringify(_name).
Signed-off-by: Himanshi Jain
---
include/linux/sysfs.h | 7 +++
1 file changed, 7 insertions(+)
diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
index aa02c32..20321cf 100644
--- a/include/linux/sysfs.h
+++ b/include/linux/sysfs.h
@@ -104,6 +104,13 @@ struct attribute_group {
.store = _store, \
}
+#define __ATTR_NAMED(_name, _mode, _show, _store) {
\
>>>
>>> I'm not sure about the naming here. The normal __ATTR macro is also
>>> 'named'. Maybe something as awful as
>>>
>>> __ATTR_STRING_NAME ?
>>>
>>> Greg what do you think?
>>
>> ick ick ick.
>>
>>> This is all to allow us to have names with operators in them without
>>> checkpatch complaining about them... A worthwhile aim just to stop
>>> more people wasting time trying to 'fix' those cases by adding
>spaces.
>>
>> Yeah, but this really seems "heavy" for just a crazy sysfs name in a
>> macro. Adding a whole new "core" define for that is a hard sell...
>>
>> I also want to get rid of the "generic" __ATTR type macros, and force
>> people to use the proper _RW and friends instead. I don't want to
>add
>> another new one that people will start to use that I later have to
>> change...
>>
>> So no, I don't like this, how about just changing your macros
>instead?
>> No one else has this problem :)
>
>Nobody else realized they have this problem yet. E.g. there are a few
>users
>of __ATTR in block/genhd.c that have the same issue and are likely to
>generate the same false positives from static checkers.
For IIO there is the option of moving these over to the core generated
available callbacks, but
that won't work in every case and is a more major change. I need to shift a
few more drivers
over to the available callbacks and see how well it works out. Might find time
to do one in a
gap between interesting talks this afternoon...
If I am feeling really keen I might write this missing docs I promised a while
back on that stuff. Jet lag dependant...
Jonathan
>
>--
>To unsubscribe from this list: send the line "unsubscribe linux-iio" in
>the body of a message to majord...@vger.kernel.org
>More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 1/2] include: linux: sysfs: Add __ATTR_NAMED macro
On 13 September 2017 14:58:23 GMT-07:00, Joe Perches wrote: >On Thu, 2017-09-14 at 00:43 +0300, Dan Carpenter wrote: >> He was exagerating a bit to call it a "static checker" warning... > >Not really. > >False positives and false negatives exist in just about >every static >checker. > >> It's just checkpatch.pl complaining about adding spaces around the - >> operator. > >checkpatch is a brain-damaged by design static checker. >regexes can only be sensitive to patterns, not compiled code. > >> The sysfs file has a hyphen in the middle. > >Another option would be to use an underscore instead. Userspace ABI plus it really does mean subtract. Used in description of differential channels. Most such IIO ABI is generated by the IIO core, this just comes up with corner cases of the ABI. Jonathan -- Sent from my Android device with K-9 Mail. Please excuse my brevity. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 1/2] include: linux: sysfs: Add __ATTR_NAMED macro
On Thu, 2017-09-14 at 00:43 +0300, Dan Carpenter wrote: > He was exagerating a bit to call it a "static checker" warning... Not really. False positives and false negatives exist in just about every static checker. > It's just checkpatch.pl complaining about adding spaces around the - > operator. checkpatch is a brain-damaged by design static checker. regexes can only be sensitive to patterns, not compiled code. > The sysfs file has a hyphen in the middle. Another option would be to use an underscore instead. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH] staging: irda: Remove typedef struct
This patch remove typedef from a structure with all its ocurrences
since using typedefs for structures is discouraged.
Issue found using Coccinelle:
@r1@
type T;
@@
typedef struct { ... } T;
@script:python c1@
T2;
T << r1.T;
@@
if T[-2:] =="_t" or T[-2:] == "_T":
coccinelle.T2 = T[:-2];
else:
coccinelle.T2 = T;
print T, coccinelle.T2
@r2@
type r1.T;
identifier c1.T2;
@@
-typedef
struct
+ T2
{ ... }
-T
;
@r3@
type r1.T;
identifier c1.T2;
@@
-T
+struct T2
Signed-off-by: Haneen Mohammed
---
drivers/staging/irda/include/net/irda/qos.h | 20 ++--
1 file changed, 10 insertions(+), 10 deletions(-)
diff --git a/drivers/staging/irda/include/net/irda/qos.h
b/drivers/staging/irda/include/net/irda/qos.h
index 05a5a24..a0315b5 100644
--- a/drivers/staging/irda/include/net/irda/qos.h
+++ b/drivers/staging/irda/include/net/irda/qos.h
@@ -58,23 +58,23 @@
#define IR_1600 0x02
/* Quality of Service information */
-typedef struct {
+struct qos_value {
__u32 value;
__u16 bits; /* LSB is first byte, MSB is second byte */
-} qos_value_t;
+};
struct qos_info {
magic_t magic;
- qos_value_t baud_rate; /* IR_11520O | ... */
- qos_value_t max_turn_time;
- qos_value_t data_size;
- qos_value_t window_size;
- qos_value_t additional_bofs;
- qos_value_t min_turn_time;
- qos_value_t link_disc_time;
+ struct qos_value baud_rate; /* IR_11520O | ... */
+ struct qos_value max_turn_time;
+ struct qos_value data_size;
+ struct qos_value window_size;
+ struct qos_value additional_bofs;
+ struct qos_value min_turn_time;
+ struct qos_value link_disc_time;
- qos_value_t power;
+ struct qos_value power;
};
extern int sysctl_max_baud_rate;
--
2.7.4
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [Outreachy kernel] Re: [PATCH v2 1/2] include: linux: sysfs: Add __ATTR_NAMED macro
On Wed, 13 Sep 2017, Joe Perches wrote: > On Thu, 2017-09-14 at 00:43 +0300, Dan Carpenter wrote: > > He was exagerating a bit to call it a "static checker" warning... > > Not really. > > False positives and false negatives exist in just about > every static > checker. > > > It's just checkpatch.pl complaining about adding spaces around the - > > operator. > > checkpatch is a brain-damaged by design static checker. > regexes can only be sensitive to patterns, not compiled code. > > > The sysfs file has a hyphen in the middle. > > Another option would be to use an underscore instead. There are already underscores on either side. julia > > -- > You received this message because you are subscribed to the Google Groups > "outreachy-kernel" group. > To unsubscribe from this group and stop receiving emails from it, send an > email to outreachy-kernel+unsubscr...@googlegroups.com. > To post to this group, send email to outreachy-ker...@googlegroups.com. > To view this discussion on the web visit > https://groups.google.com/d/msgid/outreachy-kernel/1505339903.8969.20.camel%40perches.com. > For more options, visit https://groups.google.com/d/optout. > ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [Outreachy kernel] [PATCH] staging: irda: Remove typedef struct
On Wed, 13 Sep 2017, Haneen Mohammed wrote:
> This patch remove typedef from a structure with all its ocurrences
> since using typedefs for structures is discouraged.
> Issue found using Coccinelle:
>
> @r1@
> type T;
> @@
>
> typedef struct { ... } T;
>
> @script:python c1@
> T2;
> T << r1.T;
> @@
> if T[-2:] =="_t" or T[-2:] == "_T":
> coccinelle.T2 = T[:-2];
> else:
> coccinelle.T2 = T;
>
> print T, coccinelle.T2
>
> @r2@
> type r1.T;
> identifier c1.T2;
> @@
> -typedef
> struct
> + T2
> { ... }
> -T
> ;
>
> @r3@
> type r1.T;
> identifier c1.T2;
> @@
> -T
> +struct T2
>
> Signed-off-by: Haneen Mohammed
Acked-by: Julia Lawall
> ---
> drivers/staging/irda/include/net/irda/qos.h | 20 ++--
> 1 file changed, 10 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/staging/irda/include/net/irda/qos.h
> b/drivers/staging/irda/include/net/irda/qos.h
> index 05a5a24..a0315b5 100644
> --- a/drivers/staging/irda/include/net/irda/qos.h
> +++ b/drivers/staging/irda/include/net/irda/qos.h
> @@ -58,23 +58,23 @@
> #define IR_1600 0x02
>
> /* Quality of Service information */
> -typedef struct {
> +struct qos_value {
> __u32 value;
> __u16 bits; /* LSB is first byte, MSB is second byte */
> -} qos_value_t;
> +};
>
> struct qos_info {
> magic_t magic;
>
> - qos_value_t baud_rate; /* IR_11520O | ... */
> - qos_value_t max_turn_time;
> - qos_value_t data_size;
> - qos_value_t window_size;
> - qos_value_t additional_bofs;
> - qos_value_t min_turn_time;
> - qos_value_t link_disc_time;
> + struct qos_value baud_rate; /* IR_11520O | ... */
> + struct qos_value max_turn_time;
> + struct qos_value data_size;
> + struct qos_value window_size;
> + struct qos_value additional_bofs;
> + struct qos_value min_turn_time;
> + struct qos_value link_disc_time;
>
> - qos_value_t power;
> + struct qos_value power;
> };
>
> extern int sysctl_max_baud_rate;
> --
> 2.7.4
>
> --
> You received this message because you are subscribed to the Google Groups
> "outreachy-kernel" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to outreachy-kernel+unsubscr...@googlegroups.com.
> To post to this group, send email to outreachy-ker...@googlegroups.com.
> To view this discussion on the web visit
> https://groups.google.com/d/msgid/outreachy-kernel/20170914045538.GA24121%40Haneen.
> For more options, visit https://groups.google.com/d/optout.
>
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
