RE: [PATCH net-next 05/12] net: dsa: b53: Use a macro to define I/O operations
> >>> +#define b53_build_op(type, op_size, val_type)\ > >>> +static inline int b53_##type##op_size(struct b53_device *dev, u8 > >page,\ > >>> + u8 reg, val_type val) > >>> \ > >>> +{ > >>> \ > >>> + int ret; > >>> \ > >>> + > >>> \ > >>> + mutex_lock(>reg_mutex); > >>> \ > >>> + ret = dev->ops->type##op_size(dev, page, reg, val); > >>> \ > >>> + mutex_unlock(>reg_mutex); > >>> \ > >>> + > >>> \ > >>> + return ret; > >>> \ > >>> } > >> > >> Why separate the 'type' and 'op_size' arguments since they > >> are always pasted together? > > > >For read/write48, the value type is u64. > > The way I read David's comment is that instead of calling the macro with > read, 48, just combine that > in a single argument: read48. I don't have a preference about that and can > respin eventually. Indeed, factoring in the type is harder because reads want 'u64 *' not 'u64'. While that could be factored, it would take more source lines and make things very obfuscated. David
RE: [PATCH net-next 05/12] net: dsa: b53: Use a macro to define I/O operations
On September 19, 2017 7:19:35 AM PDT, Vivien Didelotwrote: >Hi David, > >David Laight writes: > >> From: Florian Fainelli >>> Sent: 18 September 2017 22:41 >>> Instead of repeating the same pattern: acquire mutex, read/write, >release >>> mutex, define a macro: b53_build_op() which takes the type >(read|write), I/O >>> size, and value (scalar or pointer). This helps with fixing bugs >that could >>> exit (e.g: missing barrier, lock etc.). >> >>> +#define b53_build_op(type, op_size, val_type) \ >>> +static inline int b53_##type##op_size(struct b53_device *dev, u8 >page, \ >>> + u8 reg, val_type val) >>> \ >>> +{ >>> \ >>> + int ret; >>> \ >>> + >>> \ >>> + mutex_lock(>reg_mutex); >>> \ >>> + ret = dev->ops->type##op_size(dev, page, reg, val); >>> \ >>> + mutex_unlock(>reg_mutex); >>> \ >>> + >>> \ >>> + return ret; >>> \ >>> } >> >> Why separate the 'type' and 'op_size' arguments since they >> are always pasted together? > >For read/write48, the value type is u64. The way I read David's comment is that instead of calling the macro with read, 48, just combine that in a single argument: read48. I don't have a preference about that and can respin eventually. -- Florian
RE: [PATCH net-next 05/12] net: dsa: b53: Use a macro to define I/O operations
Hi David, David Laightwrites: > From: Florian Fainelli >> Sent: 18 September 2017 22:41 >> Instead of repeating the same pattern: acquire mutex, read/write, release >> mutex, define a macro: b53_build_op() which takes the type (read|write), I/O >> size, and value (scalar or pointer). This helps with fixing bugs that could >> exit (e.g: missing barrier, lock etc.). > >> +#define b53_build_op(type, op_size, val_type) \ >> +static inline int b53_##type##op_size(struct b53_device *dev, u8 page, >> \ >> + u8 reg, val_type val) >> \ >> +{ >> \ >> +int ret; >> \ >> + >> \ >> +mutex_lock(>reg_mutex); >> \ >> +ret = dev->ops->type##op_size(dev, page, reg, val); >> \ >> +mutex_unlock(>reg_mutex); >> \ >> + >> \ >> +return ret; >> \ >> } > > Why separate the 'type' and 'op_size' arguments since they > are always pasted together? For read/write48, the value type is u64. Thanks, Vivien
RE: [PATCH net-next 05/12] net: dsa: b53: Use a macro to define I/O operations
From: Florian Fainelli > Sent: 18 September 2017 22:41 > Instead of repeating the same pattern: acquire mutex, read/write, release > mutex, define a macro: b53_build_op() which takes the type (read|write), I/O > size, and value (scalar or pointer). This helps with fixing bugs that could > exit (e.g: missing barrier, lock etc.). > +#define b53_build_op(type, op_size, val_type)\ > +static inline int b53_##type##op_size(struct b53_device *dev, u8 page, > \ > + u8 reg, val_type val) > \ > +{ > \ > + int ret; > \ > + > \ > + mutex_lock(>reg_mutex); > \ > + ret = dev->ops->type##op_size(dev, page, reg, val); > \ > + mutex_unlock(>reg_mutex); > \ > + > \ > + return ret; > \ > } Why separate the 'type' and 'op_size' arguments since they are always pasted together? David
Re: [PATCH net-next 05/12] net: dsa: b53: Use a macro to define I/O operations
Hi Florian, Florian Fainelliwrites: > Instead of repeating the same pattern: acquire mutex, read/write, release > mutex, define a macro: b53_build_op() which takes the type (read|write), I/O > size, and value (scalar or pointer). This helps with fixing bugs that could > exit (e.g: missing barrier, lock etc.). > > Signed-off-by: Florian Fainelli Reviewed-by: Vivien Didelot
[PATCH net-next 05/12] net: dsa: b53: Use a macro to define I/O operations
Instead of repeating the same pattern: acquire mutex, read/write, release mutex, define a macro: b53_build_op() which takes the type (read|write), I/O size, and value (scalar or pointer). This helps with fixing bugs that could exit (e.g: missing barrier, lock etc.). Signed-off-by: Florian Fainelli--- drivers/net/dsa/b53/b53_priv.h | 133 +++-- 1 file changed, 22 insertions(+), 111 deletions(-) diff --git a/drivers/net/dsa/b53/b53_priv.h b/drivers/net/dsa/b53/b53_priv.h index 7528b22aeb03..f1136619e0e4 100644 --- a/drivers/net/dsa/b53/b53_priv.h +++ b/drivers/net/dsa/b53/b53_priv.h @@ -199,119 +199,30 @@ static inline void b53_switch_remove(struct b53_device *dev) dsa_unregister_switch(dev->ds); } -static inline int b53_read8(struct b53_device *dev, u8 page, u8 reg, u8 *val) -{ - int ret; - - mutex_lock(>reg_mutex); - ret = dev->ops->read8(dev, page, reg, val); - mutex_unlock(>reg_mutex); - - return ret; -} - -static inline int b53_read16(struct b53_device *dev, u8 page, u8 reg, u16 *val) -{ - int ret; - - mutex_lock(>reg_mutex); - ret = dev->ops->read16(dev, page, reg, val); - mutex_unlock(>reg_mutex); - - return ret; -} - -static inline int b53_read32(struct b53_device *dev, u8 page, u8 reg, u32 *val) -{ - int ret; - - mutex_lock(>reg_mutex); - ret = dev->ops->read32(dev, page, reg, val); - mutex_unlock(>reg_mutex); - - return ret; -} - -static inline int b53_read48(struct b53_device *dev, u8 page, u8 reg, u64 *val) -{ - int ret; - - mutex_lock(>reg_mutex); - ret = dev->ops->read48(dev, page, reg, val); - mutex_unlock(>reg_mutex); - - return ret; +#define b53_build_op(type, op_size, val_type) \ +static inline int b53_##type##op_size(struct b53_device *dev, u8 page, \ + u8 reg, val_type val) \ +{ \ + int ret; \ + \ + mutex_lock(>reg_mutex); \ + ret = dev->ops->type##op_size(dev, page, reg, val); \ + mutex_unlock(>reg_mutex); \ + \ + return ret; \ } -static inline int b53_read64(struct b53_device *dev, u8 page, u8 reg, u64 *val) -{ - int ret; - - mutex_lock(>reg_mutex); - ret = dev->ops->read64(dev, page, reg, val); - mutex_unlock(>reg_mutex); - - return ret; -} - -static inline int b53_write8(struct b53_device *dev, u8 page, u8 reg, u8 value) -{ - int ret; - - mutex_lock(>reg_mutex); - ret = dev->ops->write8(dev, page, reg, value); - mutex_unlock(>reg_mutex); - - return ret; -} - -static inline int b53_write16(struct b53_device *dev, u8 page, u8 reg, - u16 value) -{ - int ret; - - mutex_lock(>reg_mutex); - ret = dev->ops->write16(dev, page, reg, value); - mutex_unlock(>reg_mutex); - - return ret; -} - -static inline int b53_write32(struct b53_device *dev, u8 page, u8 reg, - u32 value) -{ - int ret; - - mutex_lock(>reg_mutex); - ret = dev->ops->write32(dev, page, reg, value); - mutex_unlock(>reg_mutex); - - return ret; -} - -static inline int b53_write48(struct b53_device *dev, u8 page, u8 reg, - u64 value) -{ - int ret; - - mutex_lock(>reg_mutex); - ret = dev->ops->write48(dev, page, reg, value); - mutex_unlock(>reg_mutex); - - return ret; -} - -static inline int b53_write64(struct b53_device *dev, u8 page, u8 reg, - u64 value) -{ - int ret; - - mutex_lock(>reg_mutex); - ret = dev->ops->write64(dev, page, reg, value); - mutex_unlock(>reg_mutex); - - return ret; -} +b53_build_op(read, 8, u8 *); +b53_build_op(read, 16, u16 *); +b53_build_op(read, 32, u32 *); +b53_build_op(read, 48, u64 *); +b53_build_op(read, 64, u64 *); + +b53_build_op(write, 8, u8); +b53_build_op(write, 16, u16); +b53_build_op(write, 32, u32); +b53_build_op(write, 48, u64); +b53_build_op(write, 64, u64); struct b53_arl_entry { u8 port; -- 2.9.3