RE: RFC: kpc2000 driver naming
>-Original Message- >From: 'gre...@linuxfoundation.org' >On Fri, May 03, 2019 at 10:24:00PM +, Matt Sickler wrote: >> Hello, >> >> Recently Greg KH posted the first set of drivers for our PCIe device >(kpc2000) and shortly after that I posted the kpc2000_dma driver. I was >wondering about naming / structure standards in the Linux kernel. >> First, a real quick background on these devices: Daktronics makes a PCIe >card with an FPGA on it to drive our LED displays (and other processing tasks). >Inside the FPGA, we use something similar to AXI-4 to divide the PCIe BAR >register space [1] into separate "IP cores". The kpc2000 driver is responsible >for probing the PCIe device, doing some basic setup (mapping the BAR, >setting up an IRQ, PCIe configuration, etc) and then enumerating these >"cores". Enumeration of the cores is facilitated by the "board info" core >that is >always at the beginning of the BAR and has a defined format. Most of the >cores are controlled entirely by userspace - the driver will add a UIO sub >device for each one which userspace uses to control FPGA registers. Only 3 >core types are handled by drivers: DMA, I2C, SPI. These are IP cores inside >the FPGA that (in the case of i2c and spi) interact with other physical devices >on the PCIe card. >> Currently, we only have the one PCIe device (the "P2K" card) but we have >more on our roadmap (one we've been calling "p3k" internally). I'm 99% >confident that the I2C and SPI cores will be exactly the same on the new FPGA >design. I'm 80% confident that the DMA engines themselves will be exactly >the same on the new FPGA design. The next card PCIe driver will quite likely >be separate from the kpc2000 driver because how bitstreams are stored / >loaded / configured is changing due to using a newer FPGA. There will likely >be common code between the two. > >Please wrap your emails at a sane column, otherwise this is just a huge wall of >text that is hard to read/understand. We use Outlook and Office 365, so I figured the emails were going to be formatted badly. Just for clarity, are you saying I should hard wrap (insert newlines myself) at an 80-column boundary? >> Now on to my actual questions: Once the drivers are "good enough" to be >moved outside of staging, I'm wondering where the drivers will end up and >what their names will/should be. >> Since the I2C and SPI drivers are single-file, I'm guessing they're going to >move to drivers/i2c/busses/i2c-dak/ and drivers/spi/spi-dak/, respectively. I >tweaked the names, since "i2c-dak" and "spi-dak" make more sense to me >than "kpc_i2c" and "kpc_spi". > >Feel free to rename them to whatever you want, I just randomly picked a >name when I did the import of the drivers. > >> So that leaves the DMA and main PCIe drivers. Where do those end up in >the tree? Would "dak-dma" and "dak-p2k" (and eventually "dak-p3k") make >more sense as names for those drivers? > >Maybe, as long as it is a "unique" name, that's all that should matter. >The subsystem maintainers of those areas might care more, but you can deal >with that when you get closer to moving the code out of staging. > >> The final question relates to how Kconfig entries are setup. The >> I2C, SPI, and DMA drivers could be "selected" on their own (even if >> the "dak-p2k" and "dak-p3k" drivers aren't selected), but that doesn't >> make much sense because they'd never get used in that configuration. >> Conversely, if you select the "dak-p2k" driver, the I2C, SPI, and DMA >> drivers better get selected too, otherwise the device won't function >> correctly. From what I can tell with Kconfig, if A depends on B, you >> can't even see (let alone select) A without already selecting B. >> Right now, the Kconfig entries are setup like this (using the current names, >not the new ones presented above): >> KPC2000_DMA depends on KPC2000 (this compiles the kpc2000_dma >driver) >> KPC2000_I2C depends on KPC2000 && I2C (this compiles the kpc2000_i2c >driver) >> KPC2000_SPI depends on KPC2000 && SPI (this compiles the kpc2000_spi >driver) >> KPC2000_CORE depends on KPC2000 >> KPC2000 depends on PCI (this compiles the kpc2000 driver) Greg, >> what is the purpose of the KPC2000_CORE config option? Nothing (that I >see) depends on it, and it doesn't cause any code to get compiled. > >I don't remember, I guess I thought that was a chunk of code the others all >depended on being present? If that's not the case, please send a patch to fix >that up. The I2C and SPI drivers don't depend on anything other than the I2C and SPI subsystems. Actually, they might be depending on the kp2000 driver having the PCIe registers mapped into kernel space instead of doing that themselves. I'm not sure if that's the correct thing to do or not, so that might be something to look closely at with all these drivers. >> I would have thought something like this makes more sense [2]: >> KPC2000_DMA depends
[PATCH] Staging: kpc2000: Cleanup in kpc_dma_transfer()
Remove unnecessary typecast in kzalloc function. In addition to that
replace kzalloc(sizeof(*acd)) over kzalloc(sizeof(struct aio_cb_data))
to maintain Linux kernel style.
Issue suggested by Coccinelle.
Signed-off-by: Madhumitha Prabakaran
---
drivers/staging/kpc2000/kpc_dma/fileops.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/staging/kpc2000/kpc_dma/fileops.c
b/drivers/staging/kpc2000/kpc_dma/fileops.c
index 5741d2b49a7d..c24329affd3a 100644
--- a/drivers/staging/kpc2000/kpc_dma/fileops.c
+++ b/drivers/staging/kpc2000/kpc_dma/fileops.c
@@ -57,7 +57,7 @@ int kpc_dma_transfer(struct dev_private_data *priv, struct
kiocb *kcb, unsigned
dev_dbg(>ldev->pldev->dev, "kpc_dma_transfer(priv = [%p], kcb =
[%p], iov_base = [%p], iov_len = %ld) ldev = [%p]\n", priv, kcb,
(void*)iov_base, iov_len, ldev);
- acd = (struct aio_cb_data *) kzalloc(sizeof(struct aio_cb_data),
GFP_KERNEL);
+ acd = kzalloc(sizeof(*acd), GFP_KERNEL);
if (!acd){
dev_err(>ldev->pldev->dev, "Couldn't kmalloc space for
for the aio data\n");
return -ENOMEM;
--
2.17.1
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v2 6/6] staging: rtl8723bs: core: Move logical operator to previous line.
Move logical operator to previous line to get rid of checkpatch warning.
Signed-off-by: Vatsala Narang
---
drivers/staging/rtl8723bs/core/rtw_mlme_ext.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c
b/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c
index 0b5bd047a552..b5e355de1199 100644
--- a/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c
+++ b/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c
@@ -5656,9 +5656,9 @@ static u8 chk_ap_is_alive(struct adapter *padapter,
struct sta_info *psta)
);
#endif
- if ((sta_rx_data_pkts(psta) == sta_last_rx_data_pkts(psta))
- && sta_rx_beacon_pkts(psta) == sta_last_rx_beacon_pkts(psta)
- && sta_rx_probersp_pkts(psta) == sta_last_rx_probersp_pkts(psta)
+ if ((sta_rx_data_pkts(psta) == sta_last_rx_data_pkts(psta)) &&
+ sta_rx_beacon_pkts(psta) == sta_last_rx_beacon_pkts(psta) &&
+sta_rx_probersp_pkts(psta) == sta_last_rx_probersp_pkts(psta)
) {
ret = false;
} else {
--
2.17.1
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v2 5/6] staging: rtl8723bs: core: Fix variable constant comparisons.
Swap the terms of comparisons whenever the constant comes first to get rid of checkpatch warning. Signed-off-by: Vatsala Narang --- drivers/staging/rtl8723bs/core/rtw_mlme_ext.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c b/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c index a8ceaa9f8718..0b5bd047a552 100644 --- a/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c +++ b/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c @@ -1276,7 +1276,7 @@ unsigned int OnAssocReq(struct adapter *padapter, union recv_frame *precv_frame) status = _STATS_FAILURE_; } - if (_STATS_SUCCESSFUL_ != status) + if (status != _STATS_SUCCESSFUL_) goto OnAssocReqFail; /* check if the supported rate is ok */ @@ -1372,7 +1372,7 @@ unsigned int OnAssocReq(struct adapter *padapter, union recv_frame *precv_frame) wpa_ie_len = 0; } - if (_STATS_SUCCESSFUL_ != status) + if (status != _STATS_SUCCESSFUL_) goto OnAssocReqFail; pstat->flags &= ~(WLAN_STA_WPS | WLAN_STA_MAYBE_WPS); -- 2.17.1 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v2 3/6] staging: rtl8723bs: core: Remove unnecessary parentheses.
Remove unnecessary parentheses after 'address-of' operator to get rid of
checkpatch warning.
Signed-off-by: Vatsala Narang
---
drivers/staging/rtl8723bs/core/rtw_mlme_ext.c | 10 +-
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c
b/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c
index ac70bbaae722..1cf6229a538b 100644
--- a/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c
+++ b/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c
@@ -277,7 +277,7 @@ void init_mlme_default_rate_set(struct adapter *padapter)
static void init_mlme_ext_priv_value(struct adapter *padapter)
{
struct mlme_ext_priv *pmlmeext = >mlmeextpriv;
- struct mlme_ext_info *pmlmeinfo = &(pmlmeext->mlmext_info);
+ struct mlme_ext_info *pmlmeinfo = >mlmext_info;
atomic_set(>event_seq, 0);
pmlmeext->mgnt_seq = 0;/* reset to zero when disconnect at client mode
*/
@@ -464,8 +464,8 @@ int init_mlme_ext_priv(struct adapter *padapter)
int res = _SUCCESS;
struct registry_priv *pregistrypriv = >registrypriv;
struct mlme_ext_priv *pmlmeext = >mlmeextpriv;
- struct mlme_priv *pmlmepriv = &(padapter->mlmepriv);
- struct mlme_ext_info *pmlmeinfo = &(pmlmeext->mlmext_info);
+ struct mlme_priv *pmlmepriv = >mlmepriv;
+ struct mlme_ext_info *pmlmeinfo = >mlmext_info;
pmlmeext->padapter = padapter;
@@ -609,8 +609,8 @@ unsigned int OnProbeReq(struct adapter *padapter, union
recv_frame *precv_frame)
unsigned char *p;
struct mlme_priv *pmlmepriv = >mlmepriv;
struct mlme_ext_priv *pmlmeext = >mlmeextpriv;
- struct mlme_ext_info *pmlmeinfo = &(pmlmeext->mlmext_info);
- struct wlan_bssid_ex*cur = &(pmlmeinfo->network);
+ struct mlme_ext_info *pmlmeinfo = >mlmext_info;
+ struct wlan_bssid_ex*cur = >network;
u8 *pframe = precv_frame->u.hdr.rx_data;
uint len = precv_frame->u.hdr.len;
u8 is_valid_p2p_probereq = false;
--
2.17.1
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v2 4/6] staging: rtl8723bs: core: Remove braces from single if statement.
Remove braces from single if statement to get rid of checkpatch warning.
Signed-off-by: Vatsala Narang
---
drivers/staging/rtl8723bs/core/rtw_mlme_ext.c | 12
1 file changed, 4 insertions(+), 8 deletions(-)
diff --git a/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c
b/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c
index 1cf6229a538b..a8ceaa9f8718 100644
--- a/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c
+++ b/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c
@@ -370,9 +370,8 @@ static void init_channel_list(struct adapter *padapter,
RT_CHANNEL_INFO *channel
struct p2p_reg_class *reg = NULL;
for (ch = o->min_chan; ch <= o->max_chan; ch += o->inc) {
- if (!has_channel(channel_set, chanset_size, ch)) {
+ if (!has_channel(channel_set, chanset_size, ch))
continue;
- }
if ((0 == padapter->registrypriv.ht_enable) && (8 ==
o->inc))
continue;
@@ -1950,9 +1949,8 @@ unsigned int OnAction_back(struct adapter *padapter,
union recv_frame *precv_fra
category = frame_body[0];
if (category == RTW_WLAN_CATEGORY_BACK) {/* representing Block Ack */
- if (!pmlmeinfo->HT_enable) {
+ if (!pmlmeinfo->HT_enable)
return _SUCCESS;
- }
action = frame_body[1];
DBG_871X("%s, action =%d\n", __func__, action);
@@ -2397,9 +2395,8 @@ s32 dump_mgntframe_and_wait_ack(struct adapter *padapter,
struct xmit_frame *pmg
pxmitpriv->ack_tx = true;
pxmitpriv->seq_no = seq_no++;
pmgntframe->ack_report = 1;
- if (rtw_hal_mgnt_xmit(padapter, pmgntframe) == _SUCCESS) {
+ if (rtw_hal_mgnt_xmit(padapter, pmgntframe) == _SUCCESS)
ret = rtw_ack_tx_wait(pxmitpriv, timeout_ms);
- }
pxmitpriv->ack_tx = false;
mutex_unlock(>ack_tx_mutex);
@@ -6431,9 +6428,8 @@ u8 setauth_hdl(struct adapter *padapter, unsigned char
*pbuf)
struct mlme_ext_priv *pmlmeext = >mlmeextpriv;
struct mlme_ext_info *pmlmeinfo = &(pmlmeext->mlmext_info);
- if (pparm->mode < 4) {
+ if (pparm->mode < 4)
pmlmeinfo->auth_algo = pparm->mode;
- }
return H2C_SUCCESS;
}
--
2.17.1
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v2 2/6] staging: rtl8723bs: core: Replace NULL comparisons.
Replace NULL comparisons in the file to get rid of checkpatch warning.
Signed-off-by: Vatsala Narang
---
drivers/staging/rtl8723bs/core/rtw_mlme_ext.c | 32 +--
1 file changed, 16 insertions(+), 16 deletions(-)
diff --git a/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c
b/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c
index 00d84d34da97..ac70bbaae722 100644
--- a/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c
+++ b/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c
@@ -381,7 +381,7 @@ static void init_channel_list(struct adapter *padapter,
RT_CHANNEL_INFO *channel
((BW40MINUS == o->bw) || (BW40PLUS == o->bw)))
continue;
- if (reg == NULL) {
+ if (!reg) {
reg = _list->reg_class[cla];
cla++;
reg->reg_class = o->op_class;
@@ -659,7 +659,7 @@ unsigned int OnProbeReq(struct adapter *padapter, union
recv_frame *precv_frame)
/* allocate a new one */
DBG_871X("going to alloc stainfo for rc ="MAC_FMT"\n",
MAC_ARG(get_sa(pframe)));
psta = rtw_alloc_stainfo(pstapriv, get_sa(pframe));
- if (psta == NULL) {
+ if (!psta) {
/* TODO: */
DBG_871X(" Exceed the upper limit of supported
clients...\n");
return _SUCCESS;
@@ -1217,7 +1217,7 @@ unsigned int OnAssocReq(struct adapter *padapter, union
recv_frame *precv_frame)
}
pstat = rtw_get_stainfo(pstapriv, GetAddr2Ptr(pframe));
- if (pstat == NULL) {
+ if (!pstat) {
status = _RSON_CLS2_;
goto asoc_class2_error;
}
@@ -1377,7 +1377,7 @@ unsigned int OnAssocReq(struct adapter *padapter, union
recv_frame *precv_frame)
goto OnAssocReqFail;
pstat->flags &= ~(WLAN_STA_WPS | WLAN_STA_MAYBE_WPS);
- if (wpa_ie == NULL) {
+ if (!wpa_ie) {
if (elems.wps_ie) {
DBG_871X("STA included WPS IE in "
"(Re)Association Request - assume WPS is "
@@ -1943,7 +1943,7 @@ unsigned int OnAction_back(struct adapter *padapter,
union recv_frame *precv_fra
addr = GetAddr2Ptr(pframe);
psta = rtw_get_stainfo(pstapriv, addr);
- if (psta == NULL)
+ if (!psta)
return _SUCCESS;
frame_body = (unsigned char *)(pframe + sizeof(struct
ieee80211_hdr_3addr));
@@ -2462,7 +2462,7 @@ void issue_beacon(struct adapter *padapter, int
timeout_ms)
/* DBG_871X("%s\n", __func__); */
pmgntframe = alloc_mgtxmitframe(pxmitpriv);
- if (pmgntframe == NULL) {
+ if (!pmgntframe) {
DBG_871X("%s, alloc mgnt frame fail\n", __func__);
return;
}
@@ -2843,7 +2843,7 @@ static int _issue_probereq(struct adapter *padapter,
RT_TRACE(_module_rtl871x_mlme_c_, _drv_notice_, ("+issue_probereq\n"));
pmgntframe = alloc_mgtxmitframe(pxmitpriv);
- if (pmgntframe == NULL)
+ if (!pmgntframe)
goto exit;
/* update attribute */
@@ -3909,7 +3909,7 @@ void issue_action_BA(struct adapter *padapter, unsigned
char *raddr, unsigned ch
DBG_871X("%s, category =%d, action =%d, status =%d\n", __func__,
category, action, status);
pmgntframe = alloc_mgtxmitframe(pxmitpriv);
- if (pmgntframe == NULL)
+ if (!pmgntframe)
return;
/* update attribute */
@@ -5033,12 +5033,12 @@ void report_survey_event(struct adapter *padapter,
union recv_frame *precv_frame
pcmdpriv = >cmdpriv;
pcmd_obj = rtw_zmalloc(sizeof(struct cmd_obj));
- if (pcmd_obj == NULL)
+ if (!pcmd_obj)
return;
cmdsz = (sizeof(struct survey_event) + sizeof(struct C2HEvent_Header));
pevtcmd = rtw_zmalloc(cmdsz);
- if (pevtcmd == NULL) {
+ if (!pevtcmd) {
kfree(pcmd_obj);
return;
}
@@ -5086,12 +5086,12 @@ void report_surveydone_event(struct adapter *padapter)
struct cmd_priv *pcmdpriv = >cmdpriv;
pcmd_obj = rtw_zmalloc(sizeof(struct cmd_obj));
- if (pcmd_obj == NULL)
+ if (!pcmd_obj)
return;
cmdsz = (sizeof(struct surveydone_event) + sizeof(struct
C2HEvent_Header));
pevtcmd = rtw_zmalloc(cmdsz);
- if (pevtcmd == NULL) {
+ if (!pevtcmd) {
kfree(pcmd_obj);
return;
}
@@ -5133,12 +5133,12 @@ void report_join_res(struct adapter *padapter, int res)
struct cmd_priv *pcmdpriv = >cmdpriv;
pcmd_obj = rtw_zmalloc(sizeof(struct cmd_obj));
- if (pcmd_obj == NULL)
+ if (!pcmd_obj)
return;
cmdsz =
[PATCH v2 1/6] staging: rtl8723bs: core: Remove blank line.
To avoid style issues, remove multiple blank lines.
Signed-off-by: Vatsala Narang
---
drivers/staging/rtl8723bs/core/rtw_mlme_ext.c | 5 -
1 file changed, 5 deletions(-)
diff --git a/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c
b/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c
index d110d4514771..00d84d34da97 100644
--- a/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c
+++ b/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c
@@ -11,7 +11,6 @@
#include
#include
-
static struct mlme_handler mlme_sta_tbl[] = {
{WIFI_ASSOCREQ, "OnAssocReq", },
{WIFI_ASSOCRSP, "OnAssocRsp", },
@@ -51,7 +50,6 @@ static struct action_handler OnAction_tbl[] = {
{RTW_WLAN_CATEGORY_P2P, "ACTION_P2P", },
};
-
static u8 null_addr[ETH_ALEN] = {0, 0, 0, 0, 0, 0};
/**
@@ -1261,7 +1259,6 @@ unsigned int OnAssocReq(struct adapter *padapter, union
recv_frame *precv_frame)
goto OnAssocReqFail;
}
-
/* now we should check all the fields... */
/* checking SSID */
p = rtw_get_ie(pframe + WLAN_HDR_A3_LEN + ie_offset, _SSID_IE_, _len,
@@ -3219,7 +3216,6 @@ void issue_asocrsp(struct adapter *padapter, unsigned
short status, struct sta_i
}
-
if (pmlmeinfo->assoc_AP_vendor == HT_IOT_PEER_REALTEK) {
pframe = rtw_set_ie(pframe, _VENDOR_SPECIFIC_IE_, 6,
REALTEK_96B_IE, &(pattrib->pktlen));
}
@@ -3264,7 +3260,6 @@ void issue_assocreq(struct adapter *padapter)
pattrib = >attrib;
update_mgntframe_attrib(padapter, pattrib);
-
memset(pmgntframe->buf_addr, 0, WLANHDR_OFFSET + TXDESC_OFFSET);
pframe = (u8 *)(pmgntframe->buf_addr) + TXDESC_OFFSET;
--
2.17.1
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v2 0/6] staging: rtl8723bs: core: Fix checkpatch warnings.
This series fix the following warnings: -Remove multiple blank lines. -Replace NULL comparison. -Remove unncessary parentheses. -Remove braces from single if statement. -Fix variable constant comparison. -Move logical operator to previous line. Changes in v2: -Dropped one patch from the series as it had some compilatin error. Vatsala Narang (6): staging: rtl8723bs: core: Remove blank line. staging: rtl8723bs: core: Replace NULL comparisons. staging: rtl8723bs: core: Remove unnecessary parentheses. staging: rtl8723bs: core: Remove braces from single if statement. staging: rtl8723bs: core: Fix variable constant comparisons. staging: rtl8723bs: core: Move logical operator to previous line. drivers/staging/rtl8723bs/core/rtw_mlme_ext.c | 69 --- 1 file changed, 30 insertions(+), 39 deletions(-) -- 2.17.1 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 0/4] staging: iio: ad7150: improve driver readability
On Sat, 4 May 2019 14:12:22 +0300 Alexandru Ardelean wrote: > On Sat, May 4, 2019 at 1:24 AM Melissa Wen wrote: > > > > This patchset solves readability issues in AD7150 code, such as clarify > > register and mask definition, fashion improvement of mask uses, reduce > > tedious operation and useless comments. > > > > Hey, > > Two patches seem a bit noisy/un-needed. > The other 2 are fine from me. > > This driver does need some work to move it out of staging. > I am not sure what would be a big blocker for it, other than maybe it > needs a device-tree binding doc (in YAML format). > Maybe Jonathan remembers. > > Some other low-hanging-fruit ideas would be: > 1) remove the code for platform_data ; that one seems forgotten from > some other time; the interrupts should be coming from device-tree, > from the i2c bindings > 2) you could do a AD7150_EVENT_SPEC() macro (similar to > AD7150_TIMEOUT() macro) and use it in the ad7150_events[] list; that > would reduce a few lines > 3) similar to 2), you could do a AD7150_CHANNEL(x) macro ; > 4) in ad7150_event_handler() the checks could be wrapped into a macro, > or maybe some function ; i am referring to "(int_status & > AD7150_STATUS_OUT1) && (chip->old_state & AD7150_STATUS_OUT1)" checks > ; those seem to be repeated > 5) add of_match_table to the driver > > I (now) suspect that the reason this driver is still in staging is this > comment: > /* Timeouts not currently handled by core */ > > I wonder if things changed since then ? > If not, it would be interesting to implement it in core. Hmm. Timeouts are 'unusual' to put it lightly. I'm thinking the ABI needs to perhaps be more specific but not sure what a good naming is. Otherwise, I just took a quick look and can't see anything much else that needs doing. Obviously something might come up in a thorough review prior to moving it though! Jonathan > > Thanks > Alex > > > > Melissa Wen (4): > > staging: iio: ad7150: organize registers definition > > staging: iio: ad7150: use FIELD_GET and GENMASK > > staging: iio: ad7150: simplify i2c SMBus return treatment > > staging: iio: ad7150: clean up of comments > > > > drivers/staging/iio/cdc/ad7150.c | 102 ++- > > 1 file changed, 47 insertions(+), 55 deletions(-) > > > > -- > > 2.20.1 > > ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 3/4] staging: iio: ad7150: simplify i2c SMBus return treatment
On Sat, 4 May 2019 13:36:43 +0300 Alexandru Ardelean wrote: > On Sat, May 4, 2019 at 1:26 AM Melissa Wen wrote: > > > > Since i2c_smbus_write_byte_data returns no-positive value, this commit > > making the treatment of its return value less verbose. > > > > Signed-off-by: Melissa Wen > > --- > > drivers/staging/iio/cdc/ad7150.c | 10 +++--- > > 1 file changed, 3 insertions(+), 7 deletions(-) > > > > diff --git a/drivers/staging/iio/cdc/ad7150.c > > b/drivers/staging/iio/cdc/ad7150.c > > index 4ba46fb6ac02..3a4572a9e5ec 100644 > > --- a/drivers/staging/iio/cdc/ad7150.c > > +++ b/drivers/staging/iio/cdc/ad7150.c > > @@ -201,16 +201,12 @@ static int ad7150_write_event_params(struct iio_dev > > *indio_dev, > > ret = i2c_smbus_write_byte_data(chip->client, > > ad7150_addresses[chan][4], > > sens); > > - if (ret < 0) > > + if (ret) > > For i2c_smbus_write_byte_data(), checking "ret < 0" or non-zero, is the same. > Changing this doesn't have any added value. The slight note I'd add to that is that if you are (and I think you should) just doing return i2c_smbus_write_byte_data() for the last call then that effectively means we are assuming ret is never positive in some paths and not others. I'd encourage consistency so would would prefer this is changed to if (ret). As in the earlier patch the line between what is noise in a staging (or new driver) and what is noise in a driver that has been outside staging for years is different. Not so good for Alex perhaps if there is a chance Analog will backport fixes for their drivers, but tough luck :) > > > return ret; > > - > > - ret = i2c_smbus_write_byte_data(chip->client, > > + else > > + return i2c_smbus_write_byte_data(chip->client, > > ad7150_addresses[chan][5], > > timeout); > > The introduction of the "else" branch is a bit noisy. > The code was a bit neater (and readable) before the else branch, and > functionally identical. > > Well, when I say neater before, you have to understand, that I (and I > assume that some other people who write drivers) have a slight > fixation for this pattern: > > example1: > ret = fn1(); > > if (ret < 0) // could also be just "if (ret)" >return ret; > > ret = fn2(); > if (ret < 0) // could also be just "if (ret)" >return ret; > > example1a: > +ret = fn3(); > +if (ret < 0) // could also be just "if (ret)" > +return ret; > > > Various higher-level programming languages, will discourage this > pattern in favor of neater patterns. > > I personally, have a few arguments in favor of this pattern: > 1) it is closer to how the machine code ; so, closer to how a > low-level instruction looks like > 2) if (ever) this needs to be patched, the patch could be neat (see > example1a) ; the examle assumes that it's been added via a patch at a > later point in time > 3) it keeps indentation level to a minimum ; this also aligns with > kernel-coding guidelines > (https://www.kernel.org/doc/html/v4.10/process/coding-style.html ) > (indentation seems a bit OCD-like when someone points it out at a > review, but it has it's value over time) Nicely laid out argument. Strongest one is the maintainability and reviewability aspect of it being how kernel code is done and hence takes every so slightly less thought ;) Errors paths are indented, good paths not (in general). Jonathan > > > - if (ret < 0) > > - return ret; > > - > > - return 0; > > } > > > > static int ad7150_write_event_config(struct iio_dev *indio_dev, > > -- > > 2.20.1 > > ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 1/4] staging: iio: ad7150: organize registers definition
On Sat, 4 May 2019 13:13:55 +0300
Alexandru Ardelean wrote:
> On Sat, May 4, 2019 at 1:25 AM Melissa Wen wrote:
> >
> > Use the suffix REG to make the register addresses clear
> > and indentation to highlight field names.
> >
>
> I'm inclined to say that this change is a bit too much noise versus added
> value.
> While the REG suffix does make sense (generally), since it hasn't been
> added from the beginning, it doesn't make much sense to add it now.
>
> It is sufficiently clear (as-is) that these macros refer to registers.
> They should be easy to match with the datasheet as well.
I'd agree for a driver that was outside staging that this is more noise than
it is worth, but one of the aims of moving drivers out is to produce
very clean and nice code. In this particular case there are very few fields
so it's not nearly as much of a mess as some other drivers where it really
hasn't been clear and the _REG suffix added much greater benefits.
Some of the arguments for minimizing noise typically don't apply on
the basis we 'probably' won't backport fixes across the move out of staging.
So I'm a bit on the fence on this one, but probably fall slightly
more in favour of the change.
Hence I would say up to you Melissa on whether you want to keep this
one or not given the arguments in favour and against that Alex
has laid out.
Given there are a few bits needed in later patches, I'll not pick this
one up for now either way!
Jonathan
>
> > Signed-off-by: Melissa Wen
> > ---
> > drivers/staging/iio/cdc/ad7150.c | 75
> > 1 file changed, 37 insertions(+), 38 deletions(-)
> >
> > diff --git a/drivers/staging/iio/cdc/ad7150.c
> > b/drivers/staging/iio/cdc/ad7150.c
> > index dd7fcab8e19e..24601ba7db88 100644
> > --- a/drivers/staging/iio/cdc/ad7150.c
> > +++ b/drivers/staging/iio/cdc/ad7150.c
> > @@ -15,35 +15,34 @@
> > #include
> > #include
> > #include
> > -/*
> > - * AD7150 registers definition
> > - */
> >
> > -#define AD7150_STATUS 0
> > -#define AD7150_STATUS_OUT1 BIT(3)
> > -#define AD7150_STATUS_OUT2 BIT(5)
> > -#define AD7150_CH1_DATA_HIGH 1
> > -#define AD7150_CH2_DATA_HIGH 3
> > -#define AD7150_CH1_AVG_HIGH5
> > -#define AD7150_CH2_AVG_HIGH7
> > -#define AD7150_CH1_SENSITIVITY 9
> > -#define AD7150_CH1_THR_HOLD_H 9
> > -#define AD7150_CH1_TIMEOUT 10
> > -#define AD7150_CH1_SETUP 11
> > -#define AD7150_CH2_SENSITIVITY 12
> > -#define AD7150_CH2_THR_HOLD_H 12
> > -#define AD7150_CH2_TIMEOUT 13
> > -#define AD7150_CH2_SETUP 14
> > -#define AD7150_CFG 15
> > -#define AD7150_CFG_FIX BIT(7)
> > -#define AD7150_PD_TIMER16
> > -#define AD7150_CH1_CAPDAC 17
> > -#define AD7150_CH2_CAPDAC 18
> > -#define AD7150_SN3 19
> > -#define AD7150_SN2 20
> > -#define AD7150_SN1 21
> > -#define AD7150_SN0 22
> > -#define AD7150_ID 23
> > +/* AD7150 registers */
> > +
> > +#define AD7150_STATUS_REG 0x00
> > +#define AD7150_STATUS_OUT1 BIT(3)
> > +#define AD7150_STATUS_OUT2 BIT(5)
> > +#define AD7150_CH1_DATA_HIGH_REG 0x01
> > +#define AD7150_CH2_DATA_HIGH_REG 0x03
> > +#define AD7150_CH1_AVG_HIGH_REG0x05
> > +#define AD7150_CH2_AVG_HIGH_REG0x07
> > +#define AD7150_CH1_SENSITIVITY_REG 0x09
> > +#define AD7150_CH1_THR_HOLD_H_REG 0x09
> > +#define AD7150_CH2_SENSITIVITY_REG 0x0C
> > +#define AD7150_CH1_TIMEOUT_REG 0x0A
> > +#define AD7150_CH1_SETUP_REG 0x0B
> > +#define AD7150_CH2_THR_HOLD_H_REG 0x0C
> > +#define AD7150_CH2_TIMEOUT_REG 0x0D
> > +#define AD7150_CH2_SETUP_REG 0x0E
> > +#define AD7150_CFG_REG 0x0F
> > +#define AD7150_CFG_FIX BIT(7)
> > +#define AD7150_PD_TIMER_REG0x10
> > +#define AD7150_CH1_CAPDAC_REG 0x11
> > +#define AD7150_CH2_CAPDAC_REG 0x12
> > +#define AD7150_SN3_REG 0x13
> > +#define AD7150_SN2_REG 0x14
> > +#define AD7150_SN1_REG 0x15
> > +#define AD7150_SN0_REG 0x16
> > +#define AD7150_ID_REG 0x17
> >
> > /**
> > * struct ad7150_chip_info - instance specific chip data
> > @@ -85,12 +84,12 @@ struct ad7150_chip_info {
> > */
> >
> > static const u8 ad7150_addresses[][6] = {
> > - { AD7150_CH1_DATA_HIGH, AD7150_CH1_AVG_HIGH,
> > - AD7150_CH1_SETUP, AD7150_CH1_THR_HOLD_H,
> > - AD7150_CH1_SENSITIVITY, AD7150_CH1_TIMEOUT },
> > - { AD7150_CH2_DATA_HIGH, AD7150_CH2_AVG_HIGH,
> > -
Re: [PATCH] staging: iio: adc: Add paragraph to describe Kconfig symbol
On Wed, 1 May 2019 12:45:41 +0530 Vatsala Narang wrote: > This patch updates Kconfig with paragraph that describe config symbol > fully.Issue addressed by checkpatch. > > Signed-off-by: Vatsala Narang I'm not a great fan of that particular checkpatch warning as sometimes it leads to silly details being added to things that really can be described in very little text. In this particular case the additional element of the module name seems worth having though so applied to the togreg branch of iio.git and pushed out as testing for the autobuilders to play with it. Thanks, Jonathan > --- > drivers/staging/iio/adc/Kconfig | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/staging/iio/adc/Kconfig b/drivers/staging/iio/adc/Kconfig > index 23d9a655a520..31cd9a12f40f 100644 > --- a/drivers/staging/iio/adc/Kconfig > +++ b/drivers/staging/iio/adc/Kconfig > @@ -12,6 +12,9 @@ config AD7816 > Say yes here to build support for Analog Devices AD7816/7/8 > temperature sensors and ADC. > > + To compile this driver as a module, choose M here: the > + module will be called ad7816. > + > config AD7192 > tristate "Analog Devices AD7190 AD7192 AD7193 AD7195 ADC driver" > depends on SPI ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: iio: adt7316: match parenthesis alignment
On Mon, 29 Apr 2019 14:59:40 -0300
João Seckler wrote:
> This patch solves the following checkpatch.pl message:
> CHECK: Alignment should match open parenthesis.
> This makes the file more compliant with the preferred coding style for
> the Linux kernel.
>
> Signed-off-by: João Seckler
Applied to the togreg branch of iio.git and pushed out as testing for
the autobuilders to play with it.
Thanks,
Jonathan
> ---
> drivers/staging/iio/addac/adt7316.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/staging/iio/addac/adt7316.c
> b/drivers/staging/iio/addac/adt7316.c
> index b6a65ee8d558..37ce563cb0e1 100644
> --- a/drivers/staging/iio/addac/adt7316.c
> +++ b/drivers/staging/iio/addac/adt7316.c
> @@ -2154,7 +2154,7 @@ int adt7316_probe(struct device *dev, struct
> adt7316_bus *bus,
> chip->dac_bits = 8;
>
> chip->ldac_pin = devm_gpiod_get_optional(dev, "adi,ldac",
> - GPIOD_OUT_LOW);
> + GPIOD_OUT_LOW);
> if (IS_ERR(chip->ldac_pin)) {
> ret = PTR_ERR(chip->ldac_pin);
> dev_err(dev, "Failed to request ldac GPIO: %d\n", ret);
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH] staging: rtl8712: get rid of IS_MCAST
Use is_multicast_ether_addr instead of custom IS_MCAST and remove
the now unused IS_MCAST. All buffers are properly aligned.
Signed-off-by: Michael Straube
---
drivers/staging/rtl8712/rtl8712_xmit.c | 2 +-
drivers/staging/rtl8712/rtl871x_recv.c | 14 +++---
drivers/staging/rtl8712/rtl871x_security.c | 4 ++--
drivers/staging/rtl8712/rtl871x_xmit.c | 12 ++--
drivers/staging/rtl8712/wifi.h | 11 ---
5 files changed, 16 insertions(+), 27 deletions(-)
diff --git a/drivers/staging/rtl8712/rtl8712_xmit.c
b/drivers/staging/rtl8712/rtl8712_xmit.c
index 7574a4b569a4..307b0e292976 100644
--- a/drivers/staging/rtl8712/rtl8712_xmit.c
+++ b/drivers/staging/rtl8712/rtl8712_xmit.c
@@ -419,7 +419,7 @@ static void update_txdesc(struct xmit_frame *pxmitframe,
uint *pmem, int sz)
struct cmd_priv *pcmdpriv = >cmdpriv;
#endif
u8 blnSetTxDescOffset;
- sint bmcst = IS_MCAST(pattrib->ra);
+ bool bmcst = is_multicast_ether_addr(pattrib->ra);
struct ht_priv *phtpriv = >htpriv;
struct tx_desc txdesc_mp;
diff --git a/drivers/staging/rtl8712/rtl871x_recv.c
b/drivers/staging/rtl8712/rtl871x_recv.c
index 28f736913292..5298fe603437 100644
--- a/drivers/staging/rtl8712/rtl871x_recv.c
+++ b/drivers/staging/rtl8712/rtl871x_recv.c
@@ -151,7 +151,7 @@ sint r8712_recvframe_chkmic(struct _adapter *adapter,
if (prxattrib->encrypt == _TKIP_) {
/* calculate mic code */
if (stainfo != NULL) {
- if (IS_MCAST(prxattrib->ra)) {
+ if (is_multicast_ether_addr(prxattrib->ra)) {
iv = precvframe->u.hdr.rx_data +
prxattrib->hdrlen;
idx = iv[3];
@@ -180,12 +180,12 @@ sint r8712_recvframe_chkmic(struct _adapter *adapter,
if (bmic_err) {
if (prxattrib->bdecrypted)
r8712_handle_tkip_mic_err(adapter,
- (u8)IS_MCAST(prxattrib->ra));
+
(u8)is_multicast_ether_addr(prxattrib->ra));
res = _FAIL;
} else {
/* mic checked ok */
if (!psecuritypriv->bcheck_grpkey &&
- IS_MCAST(prxattrib->ra))
+ is_multicast_ether_addr(prxattrib->ra))
psecuritypriv->bcheck_grpkey = true;
}
recvframe_pull_tail(precvframe, 8);
@@ -305,7 +305,7 @@ static sint sta2sta_data_frame(struct _adapter *adapter,
u8 *mybssid = get_bssid(pmlmepriv);
u8 *myhwaddr = myid(>eeprompriv);
u8 *sta_addr = NULL;
- sint bmcast = IS_MCAST(pattrib->dst);
+ bool bmcast = is_multicast_ether_addr(pattrib->dst);
if (check_fwstate(pmlmepriv, WIFI_ADHOC_STATE) ||
check_fwstate(pmlmepriv, WIFI_ADHOC_MASTER_STATE)) {
@@ -331,7 +331,7 @@ static sint sta2sta_data_frame(struct _adapter *adapter,
/* For AP mode, if DA == MCAST, then BSSID should
* be also MCAST
*/
- if (!IS_MCAST(pattrib->bssid))
+ if (!is_multicast_ether_addr(pattrib->bssid))
return _FAIL;
} else { /* not mc-frame */
/* For AP mode, if DA is non-MCAST, then it must be
@@ -373,7 +373,7 @@ static sint ap2sta_data_frame(struct _adapter *adapter,
struct mlme_priv *pmlmepriv = >mlmepriv;
u8 *mybssid = get_bssid(pmlmepriv);
u8 *myhwaddr = myid(>eeprompriv);
- sint bmcast = IS_MCAST(pattrib->dst);
+ bool bmcast = is_multicast_ether_addr(pattrib->dst);
if (check_fwstate(pmlmepriv, WIFI_STATION_STATE) &&
check_fwstate(pmlmepriv, _FW_LINKED)) {
@@ -532,7 +532,7 @@ static sint validate_recv_data_frame(struct _adapter
*adapter,
if (pattrib->privacy) {
GET_ENCRY_ALGO(psecuritypriv, psta, pattrib->encrypt,
- IS_MCAST(pattrib->ra));
+ is_multicast_ether_addr(pattrib->ra));
SET_ICE_IV_LEN(pattrib->iv_len, pattrib->icv_len,
pattrib->encrypt);
} else {
diff --git a/drivers/staging/rtl8712/rtl871x_security.c
b/drivers/staging/rtl8712/rtl871x_security.c
index f82645011d02..693008bba83e 100644
--- a/drivers/staging/rtl8712/rtl871x_security.c
+++ b/drivers/staging/rtl8712/rtl871x_security.c
@@ -665,7 +665,7 @@ u32 r8712_tkip_decrypt(struct _adapter *padapter, u8
*precvframe)
length = ((union recv_frame *)precvframe)->
u.hdr.len -
Re: [PATCH] Staging: speakup: Replace return type
Hello, Madhumitha Prabakaran, le dim. 05 mai 2019 02:26:45 -0500, a ecrit: > Replace return type and remove the respective assignment. I prefer to keep it the way it was, it looks more straightforward for the reader. Samuel > Issue found by Coccinelle. > > Signed-off-by: Madhumitha Prabakaran > --- > drivers/staging/speakup/i18n.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/drivers/staging/speakup/i18n.c b/drivers/staging/speakup/i18n.c > index ee240d36f947..a748eb8052d1 100644 > --- a/drivers/staging/speakup/i18n.c > +++ b/drivers/staging/speakup/i18n.c > @@ -470,8 +470,7 @@ static char *find_specifier_end(char *input) > input++;/* Advance over %. */ > input = skip_flags(input); > input = skip_width(input); > - input = skip_conversion(input); > - return input; > + return skip_conversion(input); > } > > /* > -- > 2.17.1 > -- Samuel Progress (n.): The process through which the Internet has evolved from smart people in front of dumb terminals to dumb people in front of smart terminals. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH] Staging: speakup: Replace return type
Replace return type and remove the respective assignment. Issue found by Coccinelle. Signed-off-by: Madhumitha Prabakaran --- drivers/staging/speakup/i18n.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/drivers/staging/speakup/i18n.c b/drivers/staging/speakup/i18n.c index ee240d36f947..a748eb8052d1 100644 --- a/drivers/staging/speakup/i18n.c +++ b/drivers/staging/speakup/i18n.c @@ -470,8 +470,7 @@ static char *find_specifier_end(char *input) input++;/* Advance over %. */ input = skip_flags(input); input = skip_width(input); - input = skip_conversion(input); - return input; + return skip_conversion(input); } /* -- 2.17.1 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
