Re: [PATCH 0/4] staging: iio: ad7150: improve driver readability
On 05/05, Jonathan Cameron wrote: > 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 > > Hi Alexandru and Jonathan, Thank you for your help! Soon I will send a v2 with the fixes pointed out. I'm also including the ideas above in the work plan for this driver. P.s.: Sorry for having previously sent an email with HTML. Melissa > > > > > 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 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 0/4] staging: iio: ad7150: improve driver readability
On Sat, May 4, 2019 at 2:12 PM 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. > I forgot to mention the wiki page for the driver: https://wiki.analog.com/resources/tools-software/linux-drivers/iio-cdc/ad7150 it may help with a few things > 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 0/4] staging: iio: ad7150: improve driver readability
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. 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
[PATCH 0/4] staging: iio: ad7150: improve driver readability
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. 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
