| Message ID | 20201103192226.2455-1-kabel@kernel.org |
|---|---|
| Headers | show |
| Series | r8152 changes | expand |
Marek Behún <kabel@kernel.org> > Sent: Wednesday, November 4, 2020 3:22 AM > diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c > index b1770489aca5..85dda591c838 100644 > --- a/drivers/net/usb/r8152.c > +++ b/drivers/net/usb/r8152.c > @@ -6862,20 +6862,12 @@ static void rtl8152_disconnect(struct > usb_interface *intf) > } > > #define REALTEK_USB_DEVICE(vend, prod) \ > - .match_flags = USB_DEVICE_ID_MATCH_DEVICE | \ > - USB_DEVICE_ID_MATCH_INT_CLASS, \ > - .idVendor = (vend), \ > - .idProduct = (prod), \ > - .bInterfaceClass = USB_CLASS_VENDOR_SPEC \ > + USB_DEVICE_INTERFACE_CLASS(vend, prod, USB_CLASS_VENDOR_SPEC) > \ > }, \ > { \ > - .match_flags = USB_DEVICE_ID_MATCH_INT_INFO | \ > - USB_DEVICE_ID_MATCH_DEVICE, \ > - .idVendor = (vend), \ > - .idProduct = (prod), \ > - .bInterfaceClass = USB_CLASS_COMM, \ > - .bInterfaceSubClass = USB_CDC_SUBCLASS_ETHERNET, \ > - .bInterfaceProtocol = USB_CDC_PROTO_NONE > + USB_DEVICE_AND_INTERFACE_INFO(vend, prod, USB_CLASS_COMM, \ > + USB_CDC_SUBCLASS_ETHERNET, \ > + USB_CDC_PROTO_NONE) > > /* table of devices that work with this driver */ > static const struct usb_device_id rtl8152_table[] = { I don't use these, because checkpatch.pl would show error. $ scripts/checkpatch.pl --file --terse drivers/net/usb/r8152.c ERROR: Macros with complex values should be enclosed in parentheses Best Regards, Hayes
On Wed, 4 Nov 2020 01:57:10 +0000 Hayes Wang <hayeswang@realtek.com> wrote: > Marek Behún <kabel@kernel.org> > > Sent: Wednesday, November 4, 2020 3:22 AM > > diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c > > index b1770489aca5..85dda591c838 100644 > > --- a/drivers/net/usb/r8152.c > > +++ b/drivers/net/usb/r8152.c > > @@ -6862,20 +6862,12 @@ static void rtl8152_disconnect(struct > > usb_interface *intf) > > } > > > > #define REALTEK_USB_DEVICE(vend, prod) \ > > - .match_flags = USB_DEVICE_ID_MATCH_DEVICE | \ > > - USB_DEVICE_ID_MATCH_INT_CLASS, \ > > - .idVendor = (vend), \ > > - .idProduct = (prod), \ > > - .bInterfaceClass = USB_CLASS_VENDOR_SPEC \ > > + USB_DEVICE_INTERFACE_CLASS(vend, prod, USB_CLASS_VENDOR_SPEC) > > \ > > }, \ > > { \ > > - .match_flags = USB_DEVICE_ID_MATCH_INT_INFO | \ > > - USB_DEVICE_ID_MATCH_DEVICE, \ > > - .idVendor = (vend), \ > > - .idProduct = (prod), \ > > - .bInterfaceClass = USB_CLASS_COMM, \ > > - .bInterfaceSubClass = USB_CDC_SUBCLASS_ETHERNET, \ > > - .bInterfaceProtocol = USB_CDC_PROTO_NONE > > + USB_DEVICE_AND_INTERFACE_INFO(vend, prod, USB_CLASS_COMM, \ > > + USB_CDC_SUBCLASS_ETHERNET, \ > > + USB_CDC_PROTO_NONE) > > > > /* table of devices that work with this driver */ > > static const struct usb_device_id rtl8152_table[] = { > > I don't use these, because checkpatch.pl would show error. > > $ scripts/checkpatch.pl --file --terse drivers/net/usb/r8152.c > ERROR: Macros with complex values should be enclosed in parentheses > > Best Regards, > Hayes > Hmm, checkpatch did not emit no warnings for me on these patches. Just two CHECKs for the third patch. BTW Hayes, is it possible for me gaining access to Realtek documentation for these chips under NDA? For example via my employer, CZ.NIC? I can't find any such information on Realtek website. Also I could not download the driver from Realtek's website, I had to find it on github. When clicking the download button on [1], it says: Warning The form #10 does not exist or it is not published. BTW2 I am interested whether we can make the internal PHY visible to the Linux PHY subsystem. Marek [1] https://www.realtek.com/en/component/zoo/category/network-interface-controllers-10-100-1000m-gigabit-ethernet-usb-3-0-software
Marek Behún <kabel@kernel.org> > Sent: Wednesday, November 4, 2020 2:03 PM [...] > BTW Hayes, is it possible for me gaining access to Realtek > documentation for these chips under NDA? For example via my employer, > CZ.NIC? I can't find any such information on Realtek website. I have to ask my boss. Maybe I reply you in private when I get the answer. > Also I could not download the driver from Realtek's website, I had to > find it on github. When clicking the download button on [1], it says: > Warning > The form #10 does not exist or it is not published. I try to download the driver from our website. And it seem to work fine. I will send it to you later. > BTW2 I am interested whether we can make the internal PHY visible to > the Linux PHY subsystem. I think it is possible. I am not familiar with the Linux PHY subsystem, so I have no idea about how to start. Best Regards, Hayes
On Wed, Nov 04, 2020 at 01:57:10AM +0000, Hayes Wang wrote: > Marek Behún <kabel@kernel.org> > > Sent: Wednesday, November 4, 2020 3:22 AM > > diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c > > index b1770489aca5..85dda591c838 100644 > > --- a/drivers/net/usb/r8152.c > > +++ b/drivers/net/usb/r8152.c > > @@ -6862,20 +6862,12 @@ static void rtl8152_disconnect(struct > > usb_interface *intf) > > } > > > > #define REALTEK_USB_DEVICE(vend, prod) \ > > - .match_flags = USB_DEVICE_ID_MATCH_DEVICE | \ > > - USB_DEVICE_ID_MATCH_INT_CLASS, \ > > - .idVendor = (vend), \ > > - .idProduct = (prod), \ > > - .bInterfaceClass = USB_CLASS_VENDOR_SPEC \ > > + USB_DEVICE_INTERFACE_CLASS(vend, prod, USB_CLASS_VENDOR_SPEC) > > \ > > }, \ > > { \ > > - .match_flags = USB_DEVICE_ID_MATCH_INT_INFO | \ > > - USB_DEVICE_ID_MATCH_DEVICE, \ > > - .idVendor = (vend), \ > > - .idProduct = (prod), \ > > - .bInterfaceClass = USB_CLASS_COMM, \ > > - .bInterfaceSubClass = USB_CDC_SUBCLASS_ETHERNET, \ > > - .bInterfaceProtocol = USB_CDC_PROTO_NONE > > + USB_DEVICE_AND_INTERFACE_INFO(vend, prod, USB_CLASS_COMM, \ > > + USB_CDC_SUBCLASS_ETHERNET, \ > > + USB_CDC_PROTO_NONE) > > > > /* table of devices that work with this driver */ > > static const struct usb_device_id rtl8152_table[] = { > > I don't use these, because checkpatch.pl would show error. > > $ scripts/checkpatch.pl --file --terse drivers/net/usb/r8152.c > ERROR: Macros with complex values should be enclosed in parentheses checkpatch is wrong.
On Wed, 4 Nov 2020 10:47:10 +0200 Vladimir Oltean <olteanv@gmail.com> wrote: > > So you aren't complaining about the definition of pla_ and usb_ > > functions, just that they are defined via macros? > > Yes. What if concatenation wasn't used, but the functions were still defined with macro? DEFINE_READ_FUNC(pla_ocp_read_byte, u8, MCU_TYPE_PLA, ocp_read_byte) DEFINE_WRITE_FUNC(pla_ocp_write_byte, u8, MCU_TYPE_PLA, ocp_write_byte) DEFINE_READ_FUNC(pla_ocp_read_word, u16, MCU_TYPE_PLA, ocp_read_word) DEFINE_WRITE_FUNC(pla_ocp_write_word, u16, MCU_TYPE_PLA, ocp_write_word) DEFINE_READ_FUNC(pla_ocp_read_dword, u32, MCU_TYPE_PLA, ocp_read_dword) DEFINE_WRITE_FUNC(pla_ocp_write_dword, u32, MCU_TYPE_PLA, ocp_write_dword) DEFINE_READ_FUNC(usb_ocp_read_byte, u8, MCU_TYPE_USB, ocp_read_byte) DEFINE_WRITE_FUNC(usb_ocp_write_byte, u8, MCU_TYPE_USB, ocp_write_byte) DEFINE_READ_FUNC(usb_ocp_read_word, u16, MCU_TYPE_USB, ocp_read_word) DEFINE_WRITE_FUNC(usb_ocp_write_word, u16, MCU_TYPE_USB, ocp_write_word) DEFINE_READ_FUNC(usb_ocp_read_dword, u32, MCU_TYPE_USB, ocp_read_dword) DEFINE_WRITE_FUNC(usb_ocp_write_dword, u32, MCU_TYPE_USB, ocp_write_dword) This way there is no concantenation. Or should I abandon macros at all? Marek
Or something like this?
#define DEF_R_FUNC(_t, _r, _r_i, _mcu) \
static inline _t _r(struct r8152 *tp, u16 index) \
{ \
return _r_i(tp, _mcu, index); \
}
#define DEF_W_FUNC(_t, _w, _w_i, _mcu) \
static inline void _w(struct r8152 *tp, u16 index, _t data) \
{ \
_w_i(tp, _mcu, index, data); \
}
DEF_R_FUNC(u8, pla_ocp_read_byte, ocp_read_byte, MCU_TYPE_PLA)
DEF_W_FUNC(u8, pla_ocp_write_byte, ocp_write_byte, MCU_TYPE_PLA)
DEF_R_FUNC(u16, pla_ocp_read_word, ocp_read_word, MCU_TYPE_PLA)
DEF_W_FUNC(u16, pla_ocp_write_word, ocp_write_word, MCU_TYPE_PLA)
DEF_R_FUNC(u32, pla_ocp_read_dword, ocp_read_dword, MCU_TYPE_PLA)
DEF_W_FUNC(u32, pla_ocp_write_dword, ocp_write_dword, MCU_TYPE_PLA)
DEF_R_FUNC(u8, usb_ocp_read_byte, ocp_read_byte, MCU_TYPE_USB)
DEF_W_FUNC(u8, usb_ocp_write_byte, ocp_write_byte, MCU_TYPE_USB)
DEF_R_FUNC(u16, usb_ocp_read_word, ocp_read_word, MCU_TYPE_USB)
DEF_W_FUNC(u16, usb_ocp_write_word, ocp_write_word, MCU_TYPE_USB)
DEF_R_FUNC(u32, usb_ocp_read_dword, ocp_read_dword, MCU_TYPE_USB)
DEF_W_FUNC(u32, usb_ocp_write_dword, ocp_write_dword, MCU_TYPE_USB)
On Wed, Nov 04, 2020 at 11:35:45AM +0100, Marek Behún wrote: > Or something like this? > > #define DEF_R_FUNC(_t, _r, _r_i, _mcu) \ > static inline _t _r(struct r8152 *tp, u16 index) \ > { \ > return _r_i(tp, _mcu, index); \ > } > > #define DEF_W_FUNC(_t, _w, _w_i, _mcu) \ > static inline void _w(struct r8152 *tp, u16 index, _t data) \ > { \ > _w_i(tp, _mcu, index, data); \ > } > > DEF_R_FUNC(u8, pla_ocp_read_byte, ocp_read_byte, MCU_TYPE_PLA) > DEF_W_FUNC(u8, pla_ocp_write_byte, ocp_write_byte, MCU_TYPE_PLA) > DEF_R_FUNC(u16, pla_ocp_read_word, ocp_read_word, MCU_TYPE_PLA) > DEF_W_FUNC(u16, pla_ocp_write_word, ocp_write_word, MCU_TYPE_PLA) > DEF_R_FUNC(u32, pla_ocp_read_dword, ocp_read_dword, MCU_TYPE_PLA) > DEF_W_FUNC(u32, pla_ocp_write_dword, ocp_write_dword, MCU_TYPE_PLA) > > DEF_R_FUNC(u8, usb_ocp_read_byte, ocp_read_byte, MCU_TYPE_USB) > DEF_W_FUNC(u8, usb_ocp_write_byte, ocp_write_byte, MCU_TYPE_USB) > DEF_R_FUNC(u16, usb_ocp_read_word, ocp_read_word, MCU_TYPE_USB) > DEF_W_FUNC(u16, usb_ocp_write_word, ocp_write_word, MCU_TYPE_USB) > DEF_R_FUNC(u32, usb_ocp_read_dword, ocp_read_dword, MCU_TYPE_USB) > DEF_W_FUNC(u32, usb_ocp_write_dword, ocp_write_dword, MCU_TYPE_USB) I'm not sure it's worth the change :( Let's put it another way, your diffstat has 338 insertions and 335 deletions. Aka you're saving 3 lines overall. With this new approach that doesn't use token concatenation at all, you're probably not saving anything at all. Also, I'm not sure that you need to make the functions inline. The compiler should be smart enough to not generate functions for usb_ocp_read_byte etc. You can check with "make drivers/net/usb/r8152.lst".
On Wed, 4 Nov 2020 13:00:59 +0200 Vladimir Oltean <olteanv@gmail.com> wrote: > On Wed, Nov 04, 2020 at 11:35:45AM +0100, Marek Behún wrote: > > Or something like this? > > > > #define DEF_R_FUNC(_t, _r, _r_i, _mcu) \ > > static inline _t _r(struct r8152 *tp, u16 index) \ > > { \ > > return _r_i(tp, _mcu, index); \ > > } > > > > #define DEF_W_FUNC(_t, _w, _w_i, _mcu) \ > > static inline void _w(struct r8152 *tp, u16 index, _t data) \ > > { \ > > _w_i(tp, _mcu, index, data); \ > > } > > > > DEF_R_FUNC(u8, pla_ocp_read_byte, ocp_read_byte, MCU_TYPE_PLA) > > DEF_W_FUNC(u8, pla_ocp_write_byte, ocp_write_byte, MCU_TYPE_PLA) > > DEF_R_FUNC(u16, pla_ocp_read_word, ocp_read_word, MCU_TYPE_PLA) > > DEF_W_FUNC(u16, pla_ocp_write_word, ocp_write_word, MCU_TYPE_PLA) > > DEF_R_FUNC(u32, pla_ocp_read_dword, ocp_read_dword, MCU_TYPE_PLA) > > DEF_W_FUNC(u32, pla_ocp_write_dword, ocp_write_dword, MCU_TYPE_PLA) > > > > DEF_R_FUNC(u8, usb_ocp_read_byte, ocp_read_byte, MCU_TYPE_USB) > > DEF_W_FUNC(u8, usb_ocp_write_byte, ocp_write_byte, MCU_TYPE_USB) > > DEF_R_FUNC(u16, usb_ocp_read_word, ocp_read_word, MCU_TYPE_USB) > > DEF_W_FUNC(u16, usb_ocp_write_word, ocp_write_word, MCU_TYPE_USB) > > DEF_R_FUNC(u32, usb_ocp_read_dword, ocp_read_dword, MCU_TYPE_USB) > > DEF_W_FUNC(u32, usb_ocp_write_dword, ocp_write_dword, MCU_TYPE_USB) > > I'm not sure it's worth the change :( > Let's put it another way, your diffstat has 338 insertions and 335 > deletions. Aka you're saving 3 lines overall. > With this new approach that doesn't use token concatenation at all, > you're probably not saving anything at all. > Also, I'm not sure that you need to make the functions inline. The > compiler should be smart enough to not generate functions for > usb_ocp_read_byte etc. You can check with > "make drivers/net/usb/r8152.lst". Vladimir, the purpose of this patch isn't to save lines, but to save us from always writing MCU_TYPE_USB / MCU_TYPE_PLA. It just transforms forms of ocp_read_word(tp, MCU_TYPE_USB, idx); ocp_write_dword(tp, MCU_TYPE_PLA, idx, val); into usb_ocp_read_word(tp, idx); pla_ocp_write_dword(tp, idx, val); The fifth patch of this series saves lines by adding _modify functions, to transform val = *_read(idx); val &= ~clr; val |= set; *_write(idx, val); into *_modify(idx, clr, set);
On Wed, Nov 04, 2020 at 12:10:53PM +0100, Marek Behún wrote: > > I'm not sure it's worth the change :( > > Let's put it another way, your diffstat has 338 insertions and 335 > > deletions. Aka you're saving 3 lines overall. > > With this new approach that doesn't use token concatenation at all, > > you're probably not saving anything at all. > > Also, I'm not sure that you need to make the functions inline. The > > compiler should be smart enough to not generate functions for > > usb_ocp_read_byte etc. You can check with > > "make drivers/net/usb/r8152.lst". > > Vladimir, the purpose of this patch isn't to save lines, but to save us > from always writing MCU_TYPE_USB / MCU_TYPE_PLA. > It just transforms forms of > ocp_read_word(tp, MCU_TYPE_USB, idx); > ocp_write_dword(tp, MCU_TYPE_PLA, idx, val); > into > usb_ocp_read_word(tp, idx); > pla_ocp_write_dword(tp, idx, val); > > The fifth patch of this series saves lines by adding _modify functions, > to transform > val = *_read(idx); > val &= ~clr; > val |= set; > *_write(idx, val); > into > *_modify(idx, clr, set); > So if the point isn't to save lines, then why don't you go for something trivial? static void ocp_modify_byte(struct r8152 *tp, u16 type, u16 index, u8 clr, u8 set) { u8 val = ocp_read_byte(tp, type, index); ocp_write_byte(tp, type, index, (val & ~clr) | set); } static void ocp_modify_word(struct r8152 *tp, u16 type, u16 index, u16 clr, u16 set) { u16 val = ocp_read_word(tp, type, index); ocp_write_word(tp, type, index, (val & ~clr) | set); } static void ocp_modify_dword(struct r8152 *tp, u16 type, u16 index, u32 clr, u32 set) { u32 val = ocp_read_dword(tp, type, index); ocp_write_dword(tp, type, index, (val & ~clr) | set); } #define pla_ocp_read_byte(tp, index) \ ocp_read_byte(tp, MCU_TYPE_PLA, index) #define pla_ocp_write_byte(tp, index, data) \ ocp_write_byte(tp, MCU_TYPE_PLA, index, data) #define pla_ocp_modify_byte(tp, index, clr, set) \ ocp_modify_byte(tp, MCU_TYPE_PLA, index, clr, set) #define pla_ocp_read_word(tp, index) \ ocp_read_word(tp, MCU_TYPE_PLA, index) #define pla_ocp_write_word(tp, index, data) \ ocp_write_word(tp, MCU_TYPE_PLA, index, data) #define pla_ocp_modify_word(tp, index, clr, set) \ ocp_modify_word(tp, MCU_TYPE_PLA, index, clr, set) #define pla_ocp_read_dword(tp, index) \ ocp_read_dword(tp, MCU_TYPE_PLA, index) #define pla_ocp_write_dword(tp, index, data) \ ocp_write_dword(tp, MCU_TYPE_PLA, index, data) #define pla_ocp_modify_dword(tp, index, clr, set) \ ocp_modify_dword(tp, MCU_TYPE_PLA, index, clr, set) #define usb_ocp_read_byte(tp, index) \ ocp_read_byte(tp, MCU_TYPE_USB, index) #define usb_ocp_write_byte(tp, index, data) \ ocp_write_byte(tp, MCU_TYPE_USB, index, data) #define usb_ocp_modify_byte(tp, index, clr, set) \ ocp_modify_byte(tp, MCU_TYPE_USB, index, clr, set) #define usb_ocp_read_word(tp, index) \ ocp_read_word(tp, MCU_TYPE_USB, index) #define usb_ocp_write_word(tp, index, data) \ ocp_write_word(tp, MCU_TYPE_USB, index, data) #define usb_ocp_modify_word(tp, index, clr, set) \ ocp_modify_word(tp, MCU_TYPE_USB, index, clr, set) #define usb_ocp_read_dword(tp, index) \ ocp_read_dword(tp, MCU_TYPE_USB, index) #define usb_ocp_write_dword(tp, index, data) \ ocp_write_dword(tp, MCU_TYPE_USB, index, data) #define usb_ocp_modify_dword(tp, index, clr, set) \ ocp_modify_dword(tp, MCU_TYPE_USB, index, clr, set) To my eyes this is easier to digest. That is, unless you want to go for function pointers and have separate structures for PLA and USB...
On Wed, 4 Nov 2020 14:14:24 +0200 Vladimir Oltean wrote:
> To my eyes this is easier to digest.
+1
On Wed, 4 Nov 2020 14:14:24 +0200 Vladimir Oltean <olteanv@gmail.com> wrote: > On Wed, Nov 04, 2020 at 12:10:53PM +0100, Marek Behún wrote: > > > I'm not sure it's worth the change :( > > > Let's put it another way, your diffstat has 338 insertions and 335 > > > deletions. Aka you're saving 3 lines overall. > > > With this new approach that doesn't use token concatenation at all, > > > you're probably not saving anything at all. > > > Also, I'm not sure that you need to make the functions inline. The > > > compiler should be smart enough to not generate functions for > > > usb_ocp_read_byte etc. You can check with > > > "make drivers/net/usb/r8152.lst". > > > > Vladimir, the purpose of this patch isn't to save lines, but to save us > > from always writing MCU_TYPE_USB / MCU_TYPE_PLA. > > It just transforms forms of > > ocp_read_word(tp, MCU_TYPE_USB, idx); > > ocp_write_dword(tp, MCU_TYPE_PLA, idx, val); > > into > > usb_ocp_read_word(tp, idx); > > pla_ocp_write_dword(tp, idx, val); > > > > The fifth patch of this series saves lines by adding _modify functions, > > to transform > > val = *_read(idx); > > val &= ~clr; > > val |= set; > > *_write(idx, val); > > into > > *_modify(idx, clr, set); > > > > So if the point isn't to save lines, then why don't you go for something > trivial? > > static void ocp_modify_byte(struct r8152 *tp, u16 type, u16 index, u8 clr, > u8 set) > { > u8 val = ocp_read_byte(tp, type, index); > > ocp_write_byte(tp, type, index, (val & ~clr) | set); > } > > static void ocp_modify_word(struct r8152 *tp, u16 type, u16 index, u16 clr, > u16 set) > { > u16 val = ocp_read_word(tp, type, index); > > ocp_write_word(tp, type, index, (val & ~clr) | set); > } > > static void ocp_modify_dword(struct r8152 *tp, u16 type, u16 index, u32 clr, > u32 set) > { > u32 val = ocp_read_dword(tp, type, index); > > ocp_write_dword(tp, type, index, (val & ~clr) | set); > } > > #define pla_ocp_read_byte(tp, index) \ > ocp_read_byte(tp, MCU_TYPE_PLA, index) > #define pla_ocp_write_byte(tp, index, data) \ > ocp_write_byte(tp, MCU_TYPE_PLA, index, data) > #define pla_ocp_modify_byte(tp, index, clr, set) \ > ocp_modify_byte(tp, MCU_TYPE_PLA, index, clr, set) > #define pla_ocp_read_word(tp, index) \ > ocp_read_word(tp, MCU_TYPE_PLA, index) > #define pla_ocp_write_word(tp, index, data) \ > ocp_write_word(tp, MCU_TYPE_PLA, index, data) > #define pla_ocp_modify_word(tp, index, clr, set) \ > ocp_modify_word(tp, MCU_TYPE_PLA, index, clr, set) > #define pla_ocp_read_dword(tp, index) \ > ocp_read_dword(tp, MCU_TYPE_PLA, index) > #define pla_ocp_write_dword(tp, index, data) \ > ocp_write_dword(tp, MCU_TYPE_PLA, index, data) > #define pla_ocp_modify_dword(tp, index, clr, set) \ > ocp_modify_dword(tp, MCU_TYPE_PLA, index, clr, set) > > #define usb_ocp_read_byte(tp, index) \ > ocp_read_byte(tp, MCU_TYPE_USB, index) > #define usb_ocp_write_byte(tp, index, data) \ > ocp_write_byte(tp, MCU_TYPE_USB, index, data) > #define usb_ocp_modify_byte(tp, index, clr, set) \ > ocp_modify_byte(tp, MCU_TYPE_USB, index, clr, set) > #define usb_ocp_read_word(tp, index) \ > ocp_read_word(tp, MCU_TYPE_USB, index) > #define usb_ocp_write_word(tp, index, data) \ > ocp_write_word(tp, MCU_TYPE_USB, index, data) > #define usb_ocp_modify_word(tp, index, clr, set) \ > ocp_modify_word(tp, MCU_TYPE_USB, index, clr, set) > #define usb_ocp_read_dword(tp, index) \ > ocp_read_dword(tp, MCU_TYPE_USB, index) > #define usb_ocp_write_dword(tp, index, data) \ > ocp_write_dword(tp, MCU_TYPE_USB, index, data) > #define usb_ocp_modify_dword(tp, index, clr, set) \ > ocp_modify_dword(tp, MCU_TYPE_USB, index, clr, set) > > To my eyes this is easier to digest. > > That is, unless you want to go for function pointers and have separate > structures for PLA and USB... I thought that static inline functions are preferred to macros, since compiler warns better if they are used incorrectly...
On Thu, Nov 05, 2020 at 10:54:18AM +0100, Marek Behún wrote: > I thought that static inline functions are preferred to macros, since > compiler warns better if they are used incorrectly... Citation needed. Also, how do static inline functions wrapped in macros (i.e. your patch) stack up against your claim about better warnings? I guess ease of maintainership should prevail here, and Hayes should have the final word. I don't really have any stake here.
On Thu, 5 Nov 2020 12:56:42 +0200 Vladimir Oltean <olteanv@gmail.com> wrote: > On Thu, Nov 05, 2020 at 10:54:18AM +0100, Marek Behún wrote: > > I thought that static inline functions are preferred to macros, since > > compiler warns better if they are used incorrectly... > > Citation needed. Just search for substring "instead of macro" in git log, there are multiple such changes that were accepted since it provides better typechecking. I am not saying it is documented anywhere, just that I thought it was preffered. > Also, how do static inline functions wrapped in macros > (i.e. your patch) stack up against your claim about better warnings? If they are defined as functions (they don't have to be inline, of course) instead of macros and they are used incorrectly, the compiler provides more readable warnings. (Yes, in current versions of gcc it is much better than in the past, but still there are more lines of warnings printed: "in expansion of macro"...). > I guess ease of maintainership should prevail here, and Hayes should > have the final word. I don't really have any stake here. Vladimir, your arguments are valid and I accept the reasoning. I too can see that the resulting code is a little awkward. Marek
On Thu, Nov 05, 2020 at 12:30:43PM +0100, Marek Behún wrote: > On Thu, 5 Nov 2020 12:56:42 +0200 > Vladimir Oltean <olteanv@gmail.com> wrote: > > > On Thu, Nov 05, 2020 at 10:54:18AM +0100, Marek Behún wrote: > > > I thought that static inline functions are preferred to macros, since > > > compiler warns better if they are used incorrectly... > > > > Citation needed. > > Just search for substring "instead of macro" in git log, there are > multiple such changes that were accepted since it provides better > typechecking. I am not saying it is documented anywhere, just that I > thought it was preffered. > > > Also, how do static inline functions wrapped in macros > > (i.e. your patch) stack up against your claim about better warnings? > > If they are defined as functions (they don't have to be inline, > of course) instead of macros and they are used incorrectly, the compiler > provides more readable warnings. (Yes, in current versions of gcc it is > much better than in the past, but still there are more lines of > warnings printed: "in expansion of macro"...). Ok, but I mean, we're not even in contradiction at this point? I only provided you macro definitions of pla_ocp_* and usb_ocp_* to prove that they can be defined in a cleaner way than your attempt. If you still think it's worth having the pla_ocp_* and usb_ocp_* helpers defined as separate functions just to avoid passing the extra MCU_TYPE_* argument, then go ahead.
Vladimir Oltean <olteanv@gmail.com> > Sent: Thursday, November 5, 2020 6:57 PM > On Thu, Nov 05, 2020 at 10:54:18AM +0100, Marek Behún wrote: > > I thought that static inline functions are preferred to macros, since > > compiler warns better if they are used incorrectly... > > Citation needed. Also, how do static inline functions wrapped in macros > (i.e. your patch) stack up against your claim about better warnings? > I guess ease of maintainership should prevail here, and Hayes should > have the final word. I don't really have any stake here. I agree with Vladimir Oltean. I prefer to the way of easy maintaining. I don't understand the advantage which you discuss. However, if I am not familiar with the code, this patch would let me take more time to find out the declarations of these functions. This make it harder to trace the code. Best Regards, Hayes
On Fri, 6 Nov 2020 03:01:22 +0000 Hayes Wang <hayeswang@realtek.com> wrote: > Vladimir Oltean <olteanv@gmail.com> > > Sent: Thursday, November 5, 2020 6:57 PM > > On Thu, Nov 05, 2020 at 10:54:18AM +0100, Marek Behún wrote: > > > I thought that static inline functions are preferred to macros, since > > > compiler warns better if they are used incorrectly... > > > > Citation needed. Also, how do static inline functions wrapped in macros > > (i.e. your patch) stack up against your claim about better warnings? > > I guess ease of maintainership should prevail here, and Hayes should > > have the final word. I don't really have any stake here. > > I agree with Vladimir Oltean. > > I prefer to the way of easy maintaining. > I don't understand the advantage which you discuss. > However, if I am not familiar with the code, this patch > would let me take more time to find out the declarations > of these functions. This make it harder to trace the code. Hi Hayes, just to be clear: Are you against defining these functions via macros? If so, I can simply rewrite this so that it does not use macros... Or are you against implementing these functions themselves? Should I abandon this at all? BTW, what about patch 5/5 which introduces *_modify helpers? Patch 5/5 simplifies the driver a lot, IMO, changing this ocp_data = usb_ocp_read_word(tp, USB_PM_CTRL_STATUS); ocp_data &= ~RESUME_INDICATE; usb_ocp_write_word(tp, USB_PM_CTRL_STATUS, ocp_data); into this usb_ocp_modify_word(tp, USB_PM_CTRL_STATUS, RESUME_INDICATE, 0); Marek
Marek Behún <kabel@kernel.org> > Sent: Friday, November 6, 2020 2:40 PM [...] > Hi Hayes, > > just to be clear: > Are you against defining these functions via macros? > If so, I can simply rewrite this so that it does not use macros... I would like the way which let me find the source of the function easily. I don't like that I couldn't find where these functions are defined, when I search whole the source code. For example, for the method which Vladimir Oltean provides, when I search the keyword "pla_ocp_read_byte", I could easily find out that the source function is "ocp_read_byte". However, for your patch, I could only find where the function is used. I think it is not friendly for the newbie who is not familiar with this driver. However, I don't think I am the decision maker. This is just my view. > Or are you against implementing these functions themselves? No. > > BTW, what about patch 5/5 which introduces *_modify helpers? It is fine. Best Regards, Hayes