[PATCH] Staging: iio: adc: fix macro and sysfs files modes in ad7280a.c
Fixes warnings found by checkpatch.pl:
- AD7280A_DEVADDR macro to use typeof, because of possible side effects
- sysfs entries user/group modes to use their octal representation
- coding style (open parenthesis alignment, CamelCase...)
Signed-off-by: Boyan Vladinov
---
drivers/staging/iio/adc/ad7280a.c | 141 +-
drivers/staging/iio/adc/ad7280a.h | 6 ++
2 files changed, 101 insertions(+), 46 deletions(-)
diff --git a/drivers/staging/iio/adc/ad7280a.c
b/drivers/staging/iio/adc/ad7280a.c
index ee679ac0368f..5f687c6aaaeb 100644
--- a/drivers/staging/iio/adc/ad7280a.c
+++ b/drivers/staging/iio/adc/ad7280a.c
@@ -99,9 +99,11 @@
#define AD7280A_DEVADDR_MASTER 0
#define AD7280A_DEVADDR_ALL0x1F
/* 5-bit device address is sent LSB first */
-#define AD7280A_DEVADDR(addr) (((addr & 0x1) << 4) | ((addr & 0x2) << 3) | \
- (addr & 0x4) | ((addr & 0x8) >> 3) | \
- ((addr & 0x10) >> 4))
+#define AD7280A_DEVADDR(addr) \
+ ({ typeof(addr) _addr = (addr); \
+ ((_addr & 0x1) << 4) | ((_addr & 0x2) << 3) | \
+ (_addr & 0x4) | ((_addr & 0x8) >> 3) | \
+ ((_addr & 0x10) >> 4); })
/* During a read a valid write is mandatory.
* So writing to the highest available address (Address 0x1F)
@@ -159,8 +161,8 @@ static unsigned char ad7280_calc_crc8(unsigned char
*crc_tab, unsigned int val)
{
unsigned char crc;
- crc = crc_tab[val >> 16 & 0xFF];
- crc = crc_tab[crc ^ (val >> 8 & 0xFF)];
+ crc = crc_tab[val >> 16 & 0xff];
+ crc = crc_tab[crc ^ (val >> 8 & 0xff)];
return crc ^ (val & 0xFF);
}
@@ -559,7 +561,7 @@ static int ad7280_attr_init(struct ad7280_state *st)
st->iio_attr[cnt].address =
AD7280A_DEVADDR(dev) << 8 | ch;
st->iio_attr[cnt].dev_attr.attr.mode =
- S_IWUSR | S_IRUGO;
+ 0644;
st->iio_attr[cnt].dev_attr.show =
ad7280_show_balance_sw;
st->iio_attr[cnt].dev_attr.store =
@@ -576,7 +578,7 @@ static int ad7280_attr_init(struct ad7280_state *st)
AD7280A_DEVADDR(dev) << 8 |
(AD7280A_CB1_TIMER + ch);
st->iio_attr[cnt].dev_attr.attr.mode =
- S_IWUSR | S_IRUGO;
+ 0644;
st->iio_attr[cnt].dev_attr.show =
ad7280_show_balance_timer;
st->iio_attr[cnt].dev_attr.store =
@@ -679,6 +681,51 @@ static ssize_t ad7280_write_channel_config(struct device
*dev,
return ret ? ret : len;
}
+static void ad7280a_push_event(struct iio_dev *indio_dev,
+ enum event_code_type event_code_t,
+ enum iio_chan_type iio_chan_t,
+ int diff,
+ int modifier,
+ enum iio_event_direction iio_event_dir,
+ enum iio_event_type iio_event_t,
+ int chan,
+ int chan1,
+ int chan2,
+ int number)
+{
+ switch (event_code_t) {
+ case AD7280A_IIO_EVENT_CODE:
+ iio_push_event(indio_dev,
+ IIO_EVENT_CODE(iio_chan_t,
+ diff,
+ modifier,
+ iio_event_dir,
+ iio_event_t,
+ chan,
+ chan1,
+ chan2),
+ iio_get_time_ns(indio_dev));
+ break;
+ case AD7280A_IIO_MOD_EVENT_CODE:
+ iio_push_event(indio_dev,
+ IIO_MOD_EVENT_CODE(iio_chan_t,
+ number,
+ modifier,
+ iio_event_t,
+ iio_event_dir),
+ iio_get_time_ns(indio_dev));
+ break;
+ case AD7280A_IIO_UNMOD_EVENT_CODE:
+ iio_push_event(indio_dev,
+ IIO_UNMOD_EVENT_CODE(iio_chan_t,
+ number,
+ iio_event_t,
+ iio_event_dir),
+ iio_get_time_ns(indio_dev));
+
Re: [PATCH] Staging: iio: adc: fix macro and sysfs files modes in ad7280a.c
On 27/11/16 08:37, Boyan Vladinov wrote:
> Fixes warnings found by checkpatch.pl:
> - AD7280A_DEVADDR macro to use typeof, because of possible side effects
Such as? I can't tell from this description whether this is a bug, or just
some good coding practice stuff.
> - sysfs entries user/group modes to use their octal representation
> - coding style (open parenthesis alignment, CamelCase...)
>
> Signed-off-by: Boyan Vladinov
There are several unrelated changes here. One patch for each unrelated
change please.
Various comments inline.
The content is mostly fine. You just need to work on how it is presented
for review.
Looking forward to the updated series!
Thanks,
Jonathan
> ---
> drivers/staging/iio/adc/ad7280a.c | 141
> +-
> drivers/staging/iio/adc/ad7280a.h | 6 ++
> 2 files changed, 101 insertions(+), 46 deletions(-)
>
> diff --git a/drivers/staging/iio/adc/ad7280a.c
> b/drivers/staging/iio/adc/ad7280a.c
> index ee679ac0368f..5f687c6aaaeb 100644
> --- a/drivers/staging/iio/adc/ad7280a.c
> +++ b/drivers/staging/iio/adc/ad7280a.c
> @@ -99,9 +99,11 @@
> #define AD7280A_DEVADDR_MASTER 0
> #define AD7280A_DEVADDR_ALL 0x1F
> /* 5-bit device address is sent LSB first */
> -#define AD7280A_DEVADDR(addr)(((addr & 0x1) << 4) | ((addr & 0x2) <<
> 3) | \
> - (addr & 0x4) | ((addr & 0x8) >> 3) | \
> - ((addr & 0x10) >> 4))
> +#define AD7280A_DEVADDR(addr)\
> + ({ typeof(addr) _addr = (addr); \
> + ((_addr & 0x1) << 4) | ((_addr & 0x2) << 3) | \
> + (_addr & 0x4) | ((_addr & 0x8) >> 3) | \
> + ((_addr & 0x10) >> 4); })
I'd like an explanation of why this is necessary. Are we looking at an
actual bug, or is this about prevenint plausible issues if this macro is
used with a different data type for address in the future?
Also if this is an actual bug please make it the first patch in the broken
out series of patches. That way we have the option to route it to mainline
faster and it can be applied to stable kernels more easily.
>
> /* During a read a valid write is mandatory.
> * So writing to the highest available address (Address 0x1F)
> @@ -159,8 +161,8 @@ static unsigned char ad7280_calc_crc8(unsigned char
> *crc_tab, unsigned int val)
> {
> unsigned char crc;
>
> - crc = crc_tab[val >> 16 & 0xFF];
> - crc = crc_tab[crc ^ (val >> 8 & 0xFF)];
> + crc = crc_tab[val >> 16 & 0xff];
Why? This is not hurting readability. It's also not camel case if that is
what you meant by camel case in the introduction.
If it is about consistency within the driver then fine, but please state this.
As it's within this large multi part patch it's not obvious how it relates
to the description.
> + crc = crc_tab[crc ^ (val >> 8 & 0xff)];
>
> return crc ^ (val & 0xFF);
> }
> @@ -559,7 +561,7 @@ static int ad7280_attr_init(struct ad7280_state *st)
> st->iio_attr[cnt].address =
> AD7280A_DEVADDR(dev) << 8 | ch;
> st->iio_attr[cnt].dev_attr.attr.mode =
> - S_IWUSR | S_IRUGO;
> + 0644;
hmm. I must admit I'm really disliking the noise that came from a fairly
throwaway comment from Linus intended to stop people patching it the other
way.
I'll probably take these patches but most as a way to stop recieving the same
one every few days/weeks as checkpatch is now highlighting it.
> st->iio_attr[cnt].dev_attr.show =
> ad7280_show_balance_sw;
> st->iio_attr[cnt].dev_attr.store =
> @@ -576,7 +578,7 @@ static int ad7280_attr_init(struct ad7280_state *st)
> AD7280A_DEVADDR(dev) << 8 |
> (AD7280A_CB1_TIMER + ch);
> st->iio_attr[cnt].dev_attr.attr.mode =
> - S_IWUSR | S_IRUGO;
> + 0644;
> st->iio_attr[cnt].dev_attr.show =
> ad7280_show_balance_timer;
> st->iio_attr[cnt].dev_attr.store =
> @@ -679,6 +681,51 @@ static ssize_t ad7280_write_channel_config(struct device
> *dev,
> return ret ? ret : len;
> }
>
This bit of refactoring is reasonable to reduced excessive indentation, but
it should be in a patch on it's own with a description of why you are making
the change.
Actually scratch that, it's not actually making the code any clear at all
looking inline.
> +static void ad7280a_push_event(struct iio_dev *indio_dev,
> +enum event_code_type event_code_t,
> +enum iio_chan_type iio_chan_t,
> +int diff,
> +int modifier,
> +enum iio_event_direction ii
Re: [PATCH v3] Staging: iio: adc: fix sysfs files modes in ad7192.c
On 26/11/16 22:39, Boyan Vladinov wrote:
> Fixes warnings found by checkpatch.pl:
> - sysfs entries user/group modes to use their octal representation
> - use the IIO_DEVICE_ATTR_[RO|RW] macroses
> - coding style
>
> Signed-off-by: Boyan Vladinov
Unfortunately this isn't quite as straight forward as it initially appears.
Making these changes is good if it makes the code simpler of cleaner. In a
couple
of cases here it actually makes it harder to tell what is going on.
Anyhow, a few suggestions inline on how to proceed.
Thanks,
Jonathan
> ---
> drivers/staging/iio/adc/ad7192.c | 39 ---
> 1 file changed, 24 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/staging/iio/adc/ad7192.c
> b/drivers/staging/iio/adc/ad7192.c
> index 1fb68c01abd5..29b8a291fb6a 100644
> --- a/drivers/staging/iio/adc/ad7192.c
> +++ b/drivers/staging/iio/adc/ad7192.c
> @@ -340,15 +340,21 @@ ad7192_show_scale_available(struct device *dev,
> return len;
> }
>
> +static ssize_t
> +in_voltage_scale_available_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + return ad7192_show_scale_available(dev, attr, buf);
> +}
> +
> static IIO_DEVICE_ATTR_NAMED(in_v_m_v_scale_available,
>in_voltage-voltage_scale_available,
> - S_IRUGO, ad7192_show_scale_available, NULL, 0);
> + 0444, ad7192_show_scale_available, NULL, 0);
>
> -static IIO_DEVICE_ATTR(in_voltage_scale_available, S_IRUGO,
> -ad7192_show_scale_available, NULL, 0);
> +static IIO_DEVICE_ATTR_RO(in_voltage_scale_available, 0);
I'd prefer this one was left alone. By using the macro for one of the two cases
we've just obscured the fact that they are really the same thing just with two
different names.
We do have new infrastructure to support available attributes directly in the
core.
If you want to look at implementing both of these using that it would be great.
See the info_mask_*_available elements of struct iio_dev and the read_avail
callback. Documentation is a bit weak on these at the moment unfortunately so
you may want to look back at the descriptions of the patches that introduced
them.
>
> -static ssize_t ad7192_show_ac_excitation(struct device *dev,
> - struct device_attribute *attr,
> +static ssize_t ac_excitation_en_show(struct device *dev,
> + struct device_attribute *attr,
>char *buf)
> {
> struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> @@ -357,8 +363,8 @@ static ssize_t ad7192_show_ac_excitation(struct device
> *dev,
> return sprintf(buf, "%d\n", !!(st->mode & AD7192_MODE_ACX));
> }
>
> -static ssize_t ad7192_show_bridge_switch(struct device *dev,
> - struct device_attribute *attr,
> +static ssize_t bridge_switch_en_show(struct device *dev,
> + struct device_attribute *attr,
>char *buf)
> {
> struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> @@ -367,8 +373,8 @@ static ssize_t ad7192_show_bridge_switch(struct device
> *dev,
> return sprintf(buf, "%d\n", !!(st->gpocon & AD7192_GPOCON_BPDSW));
> }
>
> -static ssize_t ad7192_set(struct device *dev,
> - struct device_attribute *attr,
> +static ssize_t bridge_switch_en_store(struct device *dev,
> + struct device_attribute *attr,
> const char *buf,
> size_t len)
> {
> @@ -412,13 +418,16 @@ static ssize_t ad7192_set(struct device *dev,
> return ret ? ret : len;
> }
>
> -static IIO_DEVICE_ATTR(bridge_switch_en, S_IRUGO | S_IWUSR,
> -ad7192_show_bridge_switch, ad7192_set,
> -AD7192_REG_GPOCON);
> +static ssize_t ac_excitation_en_store(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t len)
> +{
This function naming is confusing. It's not enabling the bridge switch in this
case... Hence if you were going to do this you'd need to wrap a single function
with a name covering both cases, once for each of the cases.
Or take the view that the 'shared' code is trivial and break out the relevant
bits
into each of these two functions directly.
> + return bridge_switch_en_store(dev, attr, buf, len);
> +}
> +
> +static IIO_DEVICE_ATTR_RW(bridge_switch_en, AD7192_REG_GPOCON);
>
> -static IIO_DEVICE_ATTR(ac_excitation_en, S_IRUGO | S_IWUSR,
> -ad7192_show_ac_excitation, ad7192_set,
> -AD7192_REG_MODE);
> +static IIO_DEVICE_ATTR_RW(ac_excitation_en, AD7192_REG_MODE);
>
> static struct attribute *ad7192_attributes[] = {
> &iio_dev_attr_in_v_m_v_scale_available.dev_a
[PATCH] staging: sm750fb: fix permission style warning
Fixes sm750fb user/groups permission style warning found by checkpatch.pl tool Signed-off-by: Andrea Ghittino --- diff --git a/drivers/staging/sm750fb/sm750.c b/drivers/staging/sm750fb/sm750.c index 2d22c51..2d33b80 100644 --- a/drivers/staging/sm750fb/sm750.c +++ b/drivers/staging/sm750fb/sm750.c @@ -1228,7 +1228,7 @@ static void __exit lynxfb_exit(void) } module_exit(lynxfb_exit); -module_param(g_option, charp, S_IRUGO); +module_param(g_option, charp, 0444); MODULE_PARM_DESC(g_option, "\n\t\tCommon options:\n" ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH] staging: sm750fb: fix function definition argument style warning
Fixes sm750fb function definition argument style warning
found by checkpatch.pl tool
Signed-off-by: Andrea Ghittino
---
diff --git a/drivers/staging/sm750fb/ddk750_chip.h
b/drivers/staging/sm750fb/ddk750_chip.h
index e97e859..30653dd 100644
--- a/drivers/staging/sm750fb/ddk750_chip.h
+++ b/drivers/staging/sm750fb/ddk750_chip.h
@@ -91,6 +91,6 @@ void sm750_set_chip_type(unsigned short devId, char revId);
unsigned int sm750_calc_pll_value(unsigned int request, struct pll_value
*pll);
unsigned int sm750_format_pll_reg(struct pll_value *pPLL);
unsigned int ddk750_get_vm_size(void);
-int ddk750_init_hw(struct initchip_param *);
+int ddk750_init_hw(struct initchip_param *pInitParam);
#endif
diff --git a/drivers/staging/sm750fb/ddk750_display.h
b/drivers/staging/sm750fb/ddk750_display.h
index 8abca88..c4a4cbf 100644
--- a/drivers/staging/sm750fb/ddk750_display.h
+++ b/drivers/staging/sm750fb/ddk750_display.h
@@ -107,6 +107,6 @@ typedef enum _disp_output_t {
}
disp_output_t;
-void ddk750_setLogicalDispOut(disp_output_t);
+void ddk750_setLogicalDispOut(disp_output_t output);
#endif
diff --git a/drivers/staging/sm750fb/ddk750_mode.h
b/drivers/staging/sm750fb/ddk750_mode.h
index e846dc2..bdcfe69 100644
--- a/drivers/staging/sm750fb/ddk750_mode.h
+++ b/drivers/staging/sm750fb/ddk750_mode.h
@@ -35,7 +35,7 @@ typedef struct _mode_parameter_t {
}
mode_parameter_t;
-int ddk750_setModeTiming(mode_parameter_t *, clock_type_t);
+int ddk750_setModeTiming(mode_parameter_t *parm, clock_type_t clock);
#endif
diff --git a/drivers/staging/sm750fb/ddk750_power.h
b/drivers/staging/sm750fb/ddk750_power.h
index eb088b0..6de0458 100644
--- a/drivers/staging/sm750fb/ddk750_power.h
+++ b/drivers/staging/sm750fb/ddk750_power.h
@@ -14,7 +14,7 @@ DPMS_t;
(PEEK32(MISC_CTRL) & ~MISC_CTRL_DAC_POWER_OFF) | (off)); \
}
-void ddk750_set_dpms(DPMS_t);
+void ddk750_set_dpms(DPMS_t state);
void sm750_set_power_mode(unsigned int powerMode);
void sm750_set_current_gate(unsigned int gate);
diff --git a/drivers/staging/sm750fb/sm750.h b/drivers/staging/sm750fb/sm750.h
index 28f4b9b..3b7bced 100644
--- a/drivers/staging/sm750fb/sm750.h
+++ b/drivers/staging/sm750fb/sm750.h
@@ -184,19 +184,23 @@ static inline unsigned long ps_to_hz(unsigned int psvalue)
}
int hw_sm750_map(struct sm750_dev *sm750_dev, struct pci_dev *pdev);
-int hw_sm750_inithw(struct sm750_dev*, struct pci_dev *);
-void hw_sm750_initAccel(struct sm750_dev *);
+int hw_sm750_inithw(struct sm750_dev *sm750_dev,, struct pci_dev *pdev);
+void hw_sm750_initAccel(struct sm750_dev *sm750_dev);
int hw_sm750_deWait(void);
int hw_sm750le_deWait(void);
-int hw_sm750_output_setMode(struct lynxfb_output*, struct fb_var_screeninfo*,
- struct fb_fix_screeninfo*);
-int hw_sm750_crtc_checkMode(struct lynxfb_crtc*, struct fb_var_screeninfo*);
-int hw_sm750_crtc_setMode(struct lynxfb_crtc*, struct fb_var_screeninfo*,
- struct fb_fix_screeninfo*);
-int hw_sm750_setColReg(struct lynxfb_crtc*, ushort, ushort, ushort, ushort);
-int hw_sm750_setBLANK(struct lynxfb_output*, int);
-int hw_sm750le_setBLANK(struct lynxfb_output*, int);
+int hw_sm750_output_setMode(struct lynxfb_output *output,
+ struct fb_var_screeninfo *var,
+ struct fb_fix_screeninfo *fix);
+int hw_sm750_crtc_checkMode(struct lynxfb_crtc *crtc,
+ struct fb_var_screeninfo* var);
+int hw_sm750_crtc_setMode(struct lynxfb_crtc *crtc,
+ struct fb_var_screeninfo *var,
+ struct fb_fix_screeninfo *fix);
+int hw_sm750_setColReg(struct lynxfb_crtc *crtc, ushort index,
+ ushort red, ushort green , ushort blue);
+int hw_sm750_setBLANK(struct lynxfb_output *output, int blank);
+int hw_sm750le_setBLANK(struct lynxfb_output *output, int blank);
int hw_sm750_pan_display(struct lynxfb_crtc *crtc,
const struct fb_var_screeninfo *var,
const struct fb_info *info);
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH] staging: sm750fb: fix tabstop style warning
Fixes sm750fb tabstop style warning found by checkpatch.pl tool Signed-off-by: Andrea Ghittino --- diff --git a/drivers/staging/sm750fb/sm750_accel.c b/drivers/staging/sm750fb/sm750_accel.c index 6fd18d2..af0db57 100644 --- a/drivers/staging/sm750fb/sm750_accel.c +++ b/drivers/staging/sm750fb/sm750_accel.c @@ -395,6 +395,6 @@ int sm750_hw_imageblit(struct lynx_accel *accel, pSrcbuf += srcDelta; } - return 0; + return 0; } ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3 net-next 1/2] net: ethernet: slicoss: add slicoss gigabit ethernet driver
Hello Lino,
just some things barely worth mentioning:
On 11/26/2016 01:20 PM, Lino Sanfilippo wrote:
> Add driver for Alacritech gigabit ethernet cards with SLIC (session-layer
> interface control) technology. The driver provides basic support without
> SLIC for the following devices:
>
> - Mojave cards (single port PCI Gigabit) both copper and fiber
> - Oasis cards (single and dual port PCI-x Gigabit) copper and fiber
> - Kalahari cards (dual and quad port PCI-e Gigabit) copper and fiber
>
> Signed-off-by: Lino Sanfilippo
> ---
> drivers/net/ethernet/Kconfig |1 +
> drivers/net/ethernet/Makefile |1 +
> drivers/net/ethernet/alacritech/Kconfig | 28 +
> drivers/net/ethernet/alacritech/Makefile |4 +
> drivers/net/ethernet/alacritech/slic.h| 576 +
> drivers/net/ethernet/alacritech/slicoss.c | 1867
> +
> 6 files changed, 2477 insertions(+)
> create mode 100644 drivers/net/ethernet/alacritech/Kconfig
> create mode 100644 drivers/net/ethernet/alacritech/Makefile
> create mode 100644 drivers/net/ethernet/alacritech/slic.h
> create mode 100644 drivers/net/ethernet/alacritech/slicoss.c
>
[...]
> diff --git a/drivers/net/ethernet/alacritech/slic.h
> b/drivers/net/ethernet/alacritech/slic.h
> new file mode 100644
> index 000..c62d46b
> --- /dev/null
> +++ b/drivers/net/ethernet/alacritech/slic.h
> @@ -0,0 +1,576 @@
> +
> +#ifndef _SLIC_H
> +#define _SLIC_H
I found a bunch of unused #defines in slic.h. I cannot judge if they are
worth keeping:
SLIC_VRHSTATB_LONGE
SLIC_VRHSTATB_PREA
SLIC_ISR_IO
SLIC_ISR_PING_MASK
SLIC_GIG_SPEED_MASK
SLIC_GMCR_RESET
SLIC_XCR_RESET
SLIC_XCR_XMTEN
SLIC_XCR_PAUSEEN
SLIC_XCR_LOADRNG
SLIC_REG_DBAR
SLIC_REG_PING
SLIC_REG_DUMP_CMD
SLIC_REG_DUMP_DATA
SLIC_REG_WRHOSTID
SLIC_REG_LOW_POWER
SLIC_REG_RESET_IFACE
SLIC_REG_ADDR_UPPER
SLIC_REG_HBAR64
SLIC_REG_DBAR64
SLIC_REG_CBAR64
SLIC_REG_RBAR64
SLIC_REG_WRVLANID
SLIC_REG_READ_XF_INFO
SLIC_REG_WRITE_XF_INFO
SLIC_REG_TICKS_PER_SEC
These device IDs are not used, either, but maybe it's good to keep them
for documentation purposes:
PCI_SUBDEVICE_ID_ALACRITECH_1000X1_2
PCI_SUBDEVICE_ID_ALACRITECH_SES1001T
PCI_SUBDEVICE_ID_ALACRITECH_SEN2002XT
PCI_SUBDEVICE_ID_ALACRITECH_SEN2001XT
PCI_SUBDEVICE_ID_ALACRITECH_SEN2104ET
PCI_SUBDEVICE_ID_ALACRITECH_SEN2102ET
[...]
> +
> +/* SLIC EEPROM structure for Oasis */
> +struct slic_mojave_eeprom {
Comment: "for Mojave".
[...]
> +struct slic_device {
> + struct pci_dev *pdev;
> + struct net_device *netdev;
> + void __iomem *regs;
> + /* upper address setting lock */
> + spinlock_t upper_lock;
> + struct slic_shmem shmem;
> + struct napi_struct napi;
> + struct slic_rx_queue rxq;
> + struct slic_tx_queue txq;
> + struct slic_stat_queue stq;
> + struct slic_stats stats;
> + struct slic_upr_list upr_list;
> + /* link configuration lock */
> + spinlock_t link_lock;
> + bool promisc;
> + bool autoneg;
> + int speed;
> + int duplex;
Maybe make speed and duplex unsigned? They are assigned and compared
against unsigned values in slicoss.c, so this would get rid of some
(benign, because of the range of the values) -Wsign-compare warnings in
slic_configure_link_locked. However, in a comparison there SPEED_UNKNOWN
would need to be casted to unsigned to prevent another one popping up.
[...]
> +#endif /* _SLIC_H */
> diff --git a/drivers/net/ethernet/alacritech/slicoss.c
> b/drivers/net/ethernet/alacritech/slicoss.c
> new file mode 100644
> index 000..8cd862a
> --- /dev/null
> +++ b/drivers/net/ethernet/alacritech/slicoss.c
> @@ -0,0 +1,1867 @@
[...]
> +
> +static const struct pci_device_id slic_id_tbl[] = {
> + { PCI_DEVICE(PCI_VENDOR_ID_ALACRITECH,
> + PCI_DEVICE_ID_ALACRITECH_MOAVE) },
I missed this in slic.h, but is this a typo and "MOAVE" should be
"MOJAVE"? There are a couple similar #defines in slic.h.
[...]
> +static void slic_refill_rx_queue(struct slic_device *sdev, gfp_t gfp)
> +{
> + const unsigned int ALIGN_MASK = SLIC_RX_BUFF_ALIGN - 1;
> + unsigned int maplen = SLIC_RX_BUFF_SIZE;
> + struct slic_rx_queue *rxq = &sdev->rxq;
> + struct net_device *dev = sdev->netdev;
> + struct slic_rx_buffer *buff;
> + struct slic_rx_desc *desc;
> + unsigned int misalign;
> + unsigned int offset;
> + struct sk_buff *skb;
> + dma_addr_t paddr;
> +
> + while (slic_get_free_rx_descs(rxq) > SLIC_MAX_REQ_RX_DESCS) {
> + skb = alloc_skb(maplen + ALIGN_MASK, gfp);
> + if (!skb)
> + break;
> +
> + paddr = dma_map_single(&sdev->pdev->dev, skb->data, maplen,
> +
Re: [PATCH] staging: sm750fb: fix function definition argument style warning
Hi Andrea, [auto build test ERROR on staging/staging-testing] [also build test ERROR on next-20161125] [cannot apply to v4.9-rc6] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system] url: https://github.com/0day-ci/linux/commits/Andrea-Ghittino/staging-sm750fb-fix-function-definition-argument-style-warning/20161128-004817 config: i386-allmodconfig (attached as .config) compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901 reproduce: # save the attached .config to linux build tree make ARCH=i386 All errors (new ones prefixed by >>): In file included from drivers/staging/sm750fb/sm750.c:18:0: >> drivers/staging/sm750fb/sm750.h:187:49: error: expected declaration >> specifiers or '...' before ',' token int hw_sm750_inithw(struct sm750_dev *sm750_dev,, struct pci_dev *pdev); ^ drivers/staging/sm750fb/sm750.c: In function 'lynxfb_resume': >> drivers/staging/sm750fb/sm750.c:475:2: error: implicit declaration of >> function 'hw_sm750_inithw' [-Werror=implicit-function-declaration] hw_sm750_inithw(sm750_dev, pdev); ^~~ cc1: some warnings being treated as errors -- In file included from drivers/staging/sm750fb/sm750_hw.c:22:0: >> drivers/staging/sm750fb/sm750.h:187:49: error: expected declaration >> specifiers or '...' before ',' token int hw_sm750_inithw(struct sm750_dev *sm750_dev,, struct pci_dev *pdev); ^ vim +187 drivers/staging/sm750fb/sm750.h 181 /* 10^12 / picosecond period gives frequency in Hz */ 182 do_div(numerator, psvalue); 183 return (unsigned long)numerator; 184 } 185 186 int hw_sm750_map(struct sm750_dev *sm750_dev, struct pci_dev *pdev); > 187 int hw_sm750_inithw(struct sm750_dev *sm750_dev,, struct pci_dev *pdev); 188 void hw_sm750_initAccel(struct sm750_dev *sm750_dev); 189 int hw_sm750_deWait(void); 190 int hw_sm750le_deWait(void); --- 0-DAY kernel test infrastructureOpen Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation .config.gz Description: application/gzip ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH] staging: sm750fb: fix function definition argument style warning
Fixes sm750fb function definition argument style warning
found by checkpatch.pl tool
Signed-off-by: Andrea Ghittino
---
diff --git a/drivers/staging/sm750fb/ddk750_chip.h
b/drivers/staging/sm750fb/ddk750_chip.h
index e97e859..30653dd 100644
--- a/drivers/staging/sm750fb/ddk750_chip.h
+++ b/drivers/staging/sm750fb/ddk750_chip.h
@@ -91,6 +91,6 @@ void sm750_set_chip_type(unsigned short devId, char revId);
unsigned int sm750_calc_pll_value(unsigned int request, struct pll_value
*pll);
unsigned int sm750_format_pll_reg(struct pll_value *pPLL);
unsigned int ddk750_get_vm_size(void);
-int ddk750_init_hw(struct initchip_param *);
+int ddk750_init_hw(struct initchip_param *pInitParam);
#endif
diff --git a/drivers/staging/sm750fb/ddk750_display.h
b/drivers/staging/sm750fb/ddk750_display.h
index 8abca88..c4a4cbf 100644
--- a/drivers/staging/sm750fb/ddk750_display.h
+++ b/drivers/staging/sm750fb/ddk750_display.h
@@ -107,6 +107,6 @@ typedef enum _disp_output_t {
}
disp_output_t;
-void ddk750_setLogicalDispOut(disp_output_t);
+void ddk750_setLogicalDispOut(disp_output_t output);
#endif
diff --git a/drivers/staging/sm750fb/ddk750_mode.h
b/drivers/staging/sm750fb/ddk750_mode.h
index e846dc2..bdcfe69 100644
--- a/drivers/staging/sm750fb/ddk750_mode.h
+++ b/drivers/staging/sm750fb/ddk750_mode.h
@@ -35,7 +35,7 @@ typedef struct _mode_parameter_t {
}
mode_parameter_t;
-int ddk750_setModeTiming(mode_parameter_t *, clock_type_t);
+int ddk750_setModeTiming(mode_parameter_t *parm, clock_type_t clock);
#endif
diff --git a/drivers/staging/sm750fb/ddk750_power.h
b/drivers/staging/sm750fb/ddk750_power.h
index eb088b0..6de0458 100644
--- a/drivers/staging/sm750fb/ddk750_power.h
+++ b/drivers/staging/sm750fb/ddk750_power.h
@@ -14,7 +14,7 @@ DPMS_t;
(PEEK32(MISC_CTRL) & ~MISC_CTRL_DAC_POWER_OFF) | (off)); \
}
-void ddk750_set_dpms(DPMS_t);
+void ddk750_set_dpms(DPMS_t state);
void sm750_set_power_mode(unsigned int powerMode);
void sm750_set_current_gate(unsigned int gate);
diff --git a/drivers/staging/sm750fb/sm750.h b/drivers/staging/sm750fb/sm750.h
index 28f4b9b..3b7bced 100644
--- a/drivers/staging/sm750fb/sm750.h
+++ b/drivers/staging/sm750fb/sm750.h
@@ -184,19 +184,23 @@ static inline unsigned long ps_to_hz(unsigned int psvalue)
}
int hw_sm750_map(struct sm750_dev *sm750_dev, struct pci_dev *pdev);
-int hw_sm750_inithw(struct sm750_dev*, struct pci_dev *);
-void hw_sm750_initAccel(struct sm750_dev *);
+int hw_sm750_inithw(struct sm750_dev *sm750_dev, struct pci_dev *pdev);
+void hw_sm750_initAccel(struct sm750_dev *sm750_dev);
int hw_sm750_deWait(void);
int hw_sm750le_deWait(void);
-int hw_sm750_output_setMode(struct lynxfb_output*, struct fb_var_screeninfo*,
- struct fb_fix_screeninfo*);
-int hw_sm750_crtc_checkMode(struct lynxfb_crtc*, struct fb_var_screeninfo*);
-int hw_sm750_crtc_setMode(struct lynxfb_crtc*, struct fb_var_screeninfo*,
- struct fb_fix_screeninfo*);
-int hw_sm750_setColReg(struct lynxfb_crtc*, ushort, ushort, ushort, ushort);
-int hw_sm750_setBLANK(struct lynxfb_output*, int);
-int hw_sm750le_setBLANK(struct lynxfb_output*, int);
+int hw_sm750_output_setMode(struct lynxfb_output *output,
+ struct fb_var_screeninfo *var,
+ struct fb_fix_screeninfo *fix);
+int hw_sm750_crtc_checkMode(struct lynxfb_crtc *crtc,
+ struct fb_var_screeninfo* var);
+int hw_sm750_crtc_setMode(struct lynxfb_crtc *crtc,
+ struct fb_var_screeninfo *var,
+ struct fb_fix_screeninfo *fix);
+int hw_sm750_setColReg(struct lynxfb_crtc *crtc, ushort index,
+ ushort red, ushort green , ushort blue);
+int hw_sm750_setBLANK(struct lynxfb_output *output, int blank);
+int hw_sm750le_setBLANK(struct lynxfb_output *output, int blank);
int hw_sm750_pan_display(struct lynxfb_crtc *crtc,
const struct fb_var_screeninfo *var,
const struct fb_info *info);
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: sm750fb: fix function definition argument style warning
On 11/27/16 13:11, Andrea Ghittino wrote: > Fixes sm750fb function definition argument style warning > found by checkpatch.pl tool > > Signed-off-by: Andrea Ghittino > --- Does checkpatch not complain about the camelcase variable names and function names? -- ~Randy ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: greybuis: fix line over 80 characters style warnings
On Sun, Nov 27, 2016 at 3:43 AM, Andrea Ghittino wrote:
> Fixes greybus "line over 80 characters" style warnings
> found by checkpatch.pl tool
>
It is always trade-off between readability vs 80 character restriction.
I give preference and precedence to readability, so I kept it as is
during development.
I have marked the changes with "+1", wherever I feel they should be
accepted, and others probably not needed due to readability issue.
> Signed-off-by: Andrea Ghittino
> ---
>
> diff --git a/drivers/staging/greybus/arche-apb-ctrl.c
> b/drivers/staging/greybus/arche-apb-ctrl.c
> index 3fda0cd..755120a 100644
> --- a/drivers/staging/greybus/arche-apb-ctrl.c
> +++ b/drivers/staging/greybus/arche-apb-ctrl.c
> @@ -168,7 +168,10 @@ static int standby_boot_seq(struct platform_device *pdev)
> if (apb->init_disabled)
> return 0;
>
> - /* Even if it is in OFF state, then we do not want to change the
> state */
> + /*
> +* Even if it is in OFF state,
> +* then we do not want to change the state
> +*/
+1
> if (apb->state == ARCHE_PLATFORM_STATE_STANDBY ||
> apb->state == ARCHE_PLATFORM_STATE_OFF)
> return 0;
> diff --git a/drivers/staging/greybus/arche-platform.c
> b/drivers/staging/greybus/arche-platform.c
> index 338c2d3..594fe01 100644
> --- a/drivers/staging/greybus/arche-platform.c
> +++ b/drivers/staging/greybus/arche-platform.c
> @@ -291,15 +291,21 @@ static irqreturn_t arche_platform_wd_irq(int irq, void
> *devid)
> if (arche_pdata->wake_detect_state == WD_STATE_BOOT_INIT) {
> if (time_before(jiffies,
> arche_pdata->wake_detect_start +
> -
> msecs_to_jiffies(WD_COLDBOOT_PULSE_WIDTH_MS))) {
> -
> arche_platform_set_wake_detect_state(arche_pdata,
> -
> WD_STATE_IDLE);
> + msecs_to_jiffies(
> + WD_COLDBOOT_PULSE_WIDTH_MS)))
> {
> + arche_platform_set_wake_detect_state(
> + arche_pdata,
> +
> WD_STATE_IDLE);
This I feel that, lets keep original code, as it impacts the readability.
> } else {
> - /* Check we are not in middle of irq thread
> already */
> + /*
> +* Check we are not in middle
> +* of irq thread already
> +*/
+1
> if (arche_pdata->wake_detect_state !=
> WD_STATE_COLDBOOT_START) {
> -
> arche_platform_set_wake_detect_state(arche_pdata,
> -
> WD_STATE_COLDBOOT_TRIG);
> + arche_platform_set_wake_detect_state(
> +
> arche_pdata,
> +
> WD_STATE_COLDBOOT_TRIG);
Probably keep the original code here as well.
> spin_unlock_irqrestore(
> &arche_pdata->wake_lock,
> flags);
> @@ -312,12 +318,14 @@ static irqreturn_t arche_platform_wd_irq(int irq, void
> *devid)
> if (arche_pdata->wake_detect_state == WD_STATE_IDLE) {
> arche_pdata->wake_detect_start = jiffies;
> /*
> -* In the begining, when wake/detect goes low (first
> time), we assume
> -* it is meant for coldboot and set the flag. If
> wake/detect line stays low
> -* beyond 30msec, then it is coldboot else fallback
> to standby boot.
> +* In the begining, when wake/detect goes low
> +* (first time), we assume it is meant for coldboot
> +* and set the flag. If wake/detect line stays low
> +* beyond 30msec, then it is coldboot else
> +* fallback to standby boot.
> */
> arche_platform_set_wake_detect_state(arche_pdata,
> -
> WD_STATE_BOOT_INIT);
> +
> WD_STATE_BOOT_INIT);
+1
> }
> }
>
> @@ -330,7 +338,8 @@ static irqreturn_t arche_platform_wd_irq(int irq, void
> *devid)
> /*
> * Requ
Re: [PATCH v3 net-next 1/2] net: ethernet: slicoss: add slicoss gigabit ethernet driver
On 11/26/2016 04:20 AM, Lino Sanfilippo wrote:
> Add driver for Alacritech gigabit ethernet cards with SLIC (session-layer
> interface control) technology. The driver provides basic support without
> SLIC for the following devices:
>
> - Mojave cards (single port PCI Gigabit) both copper and fiber
> - Oasis cards (single and dual port PCI-x Gigabit) copper and fiber
> - Kalahari cards (dual and quad port PCI-e Gigabit) copper and fiber
This looks great, a few nits below:
> +#define SLIC_MAX_TX_COMPLETIONS 100
You usually don't want to limit the number of TX completion, if the
entire TX ring needs to be cleaned, you would want to allow that.
[snip]
> + while (slic_get_free_rx_descs(rxq) > SLIC_MAX_REQ_RX_DESCS) {
> + skb = alloc_skb(maplen + ALIGN_MASK, gfp);
> + if (!skb)
> + break;
> +
> + paddr = dma_map_single(&sdev->pdev->dev, skb->data, maplen,
> +DMA_FROM_DEVICE);
> + if (dma_mapping_error(&sdev->pdev->dev, paddr)) {
> + netdev_err(dev, "mapping rx packet failed\n");
> + /* drop skb */
> + dev_kfree_skb_any(skb);
> + break;
> + }
> + /* ensure head buffer descriptors are 256 byte aligned */
> + offset = 0;
> + misalign = paddr & ALIGN_MASK;
> + if (misalign) {
> + offset = SLIC_RX_BUFF_ALIGN - misalign;
> + skb_reserve(skb, offset);
> + }
> + /* the HW expects dma chunks for descriptor + frame data */
> + desc = (struct slic_rx_desc *)skb->data;
> + memset(desc, 0, sizeof(*desc));
Do you really need to zero-out the prepending RX descriptor? Are not you
missing a write barrier here?
[snip]
> +
> + dma_sync_single_for_cpu(&sdev->pdev->dev,
> + dma_unmap_addr(buff, map_addr),
> + buff->addr_offset + sizeof(*desc),
> + DMA_FROM_DEVICE);
> +
> + status = le32_to_cpu(desc->status);
> + if (!(status & SLIC_IRHDDR_SVALID))
> + break;
> +
> + buff->skb = NULL;
> +
> + dma_unmap_single(&sdev->pdev->dev,
> + dma_unmap_addr(buff, map_addr),
> + dma_unmap_len(buff, map_len),
> + DMA_FROM_DEVICE);
This is potentially inefficient, you already did a cache invalidation
for the RX descriptor here, you could be more efficient with just
invalidating the packet length, minus the descriptor length.
> +
> + /* skip rx descriptor that is placed before the frame data */
> + skb_reserve(skb, SLIC_RX_BUFF_HDR_SIZE);
> +
> + if (unlikely(status & SLIC_IRHDDR_ERR)) {
> + slic_handle_frame_error(sdev, skb);
> + dev_kfree_skb_any(skb);
> + } else {
> + struct ethhdr *eh = (struct ethhdr *)skb->data;
> +
> + if (is_multicast_ether_addr(eh->h_dest))
> + SLIC_INC_STATS_COUNTER(&sdev->stats, rx_mcasts);
> +
> + len = le32_to_cpu(desc->length) & SLIC_IRHDDR_FLEN_MSK;
> + skb_put(skb, len);
> + skb->protocol = eth_type_trans(skb, dev);
> + skb->ip_summed = CHECKSUM_UNNECESSARY;
> + skb->dev = dev;
eth_type_trans() already assigns skb->dev = dev;
> +static int slic_poll(struct napi_struct *napi, int todo)
> +{
> + struct slic_device *sdev = container_of(napi, struct slic_device, napi);
> + struct slic_shmem *sm = &sdev->shmem;
> + struct slic_shmem_data *sm_data = sm->shmem_data;
> + u32 isr = le32_to_cpu(sm_data->isr);
> + unsigned int done = 0;
> +
> + slic_handle_irq(sdev, isr, todo, &done);
> +
> + if (done < todo) {
> + napi_complete(napi);
napi_complete_done() since you know how many packets you completed.
> + /* reenable irqs */
> + sm_data->isr = 0;
> + /* make sure sm_data->isr is cleard before irqs are reenabled */
> + wmb();
> + slic_write(sdev, SLIC_REG_ISR, 0);
> + slic_flush_write(sdev);
> + }
> +
> + return done;
> +}
> +
> +static irqreturn_t slic_irq(int irq, void *dev_id)
> +{
> + struct slic_device *sdev = dev_id;
> + struct slic_shmem *sm = &sdev->shmem;
> + struct slic_shmem_data *sm_data = sm->shmem_data;
> +
> + slic_write(sdev, SLIC_REG_ICR, SLIC_ICR_INT_MASK);
> + slic_flush_write(sdev);
> + /* make sure sm_data->isr is read after ICR_INT_MASK is set */
> + wmb();
> +
> + if (!sm_data->isr) {
> + dma_rmb();
> + /* spurious interrupt */
> + slic_write
RE: [PATCH 1/7] hv: acquire vmbus_connection.channel_mutex in vmbus_free_channels()
> From: Vitaly Kuznetsov [mailto:vkuzn...@redhat.com]
> Sent: Friday, November 25, 2016 20:49
> To: de...@linuxdriverproject.org
> Cc: linux-ker...@vger.kernel.org; KY Srinivasan ; Haiyang
> Zhang ; Dexuan Cui ;
> Stephen Hemminger
> Subject: [PATCH 1/7] hv: acquire vmbus_connection.channel_mutex in
> vmbus_free_channels()
>
> "kernel BUG at drivers/hv/channel_mgmt.c:350!" is observed when hv_vmbus
> module is unloaded. BUG_ON() was introduced in commit 85d9aa705184
> ("Drivers: hv: vmbus: add an API vmbus_hvsock_device_unregister()") as
> vmbus_free_channels() codepath was apparently forgotten.
>
> Fixes: 85d9aa705184 ("Drivers: hv: vmbus: add an API
> vmbus_hvsock_device_unregister()")
> Signed-off-by: Vitaly Kuznetsov
Reviewed-by: Dexuan Cui
Thanks for the fix!
-- Dexuan
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
RE: [PATCH 0/7] hv: CPU onlining/offlining fixes and improvements
> From: Stephen Hemminger [mailto:step...@networkplumber.org] > Sent: Sunday, November 27, 2016 01:06 > To: Vitaly Kuznetsov > Cc: de...@linuxdriverproject.org; linux-ker...@vger.kernel.org; KY Srinivasan > ; Haiyang Zhang ; Dexuan Cui > > Subject: Re: [PATCH 0/7] hv: CPU onlining/offlining fixes and improvements > > On Fri, 25 Nov 2016 13:48:36 +0100 > Vitaly Kuznetsov wrote: > > > Some time ago we forbade CPU offlining for Hyper-V and this was sufficient > > if you boot with all CPUs onlined. Turns out, people may want to limit the > > number online CPUs by passing 'maxcpus=' kernel parameter and we hit a > > crash in Hyper-V code in this case. After some thinking, I think we may not > > only fix the crash but also make the offlining prevention fine-grained: we > > need to prevent from offlining CPUs which have VMBus channels attached > > only. All offlined CPUs may always be onlined. > > > > As a temporary solution this is ok, but long term we need to support > dynamic CPU online/offline. If a CPU is bound to some channels, it seems impossible to make it offline, unless Hyper-V supplies a mechanism to dynamically rebind the channels (i.e. without closing & opening the channels) to another CPU, e.g. CPU0. Currently it looks there is no such mechanism. For CPU online, my understanding is: this patchset of Vitaly actually makes it work (see PATCH 6/7). Thanks, -- Dexuan ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
