| Message ID | 20200905204151.25343-1-digetx@gmail.com |
|---|---|
| Headers | show |
| Series | Improvements for Tegra I2C driver | expand |
06.09.2020 00:56, Michał Mirosław пишет: > On Sat, Sep 05, 2020 at 11:41:30PM +0300, Dmitry Osipenko wrote: >> Use clk-bulk helpers and factor out clocks initialization into separate >> function in order to make code cleaner. > [...] >> --- a/drivers/i2c/busses/i2c-tegra.c >> +++ b/drivers/i2c/busses/i2c-tegra.c > [...] >> static const struct tegra_i2c_hw_feature tegra194_i2c_hw = { >> .has_continue_xfer_support = true, >> .has_per_pkt_xfer_complete_irq = true, >> - .has_single_clk_source = true, >> .clk_divisor_hs_mode = 1, >> .clk_divisor_std_mode = 0x4f, >> .clk_divisor_fast_mode = 0x3c, > [...] >> +static int tegra_i2c_init_clocks(struct tegra_i2c_dev *i2c_dev) >> +{ >> + unsigned int i; >> + int err; >> + >> + err = devm_clk_bulk_get_all(i2c_dev->dev, &i2c_dev->clocks); >> + if (err < 0) >> + return err; >> + >> + i2c_dev->nclocks = err > [...] > > You loose checking whether number of clocks matches the device version. > Is this intended? Yes, it's not needed. The check wasn't really needed in the first place.
On Sat, Sep 05, 2020 at 11:41:36PM +0300, Dmitry Osipenko wrote: > The "dma" variable of tegra_i2c_xfer_msg() function doesn't bring much in > regards to readability and generation of the code, hence let's remove it > to clean up code a tad. [...] > --- a/drivers/i2c/busses/i2c-tegra.c > +++ b/drivers/i2c/busses/i2c-tegra.c [...] > + if (i2c_dev->is_curr_dma_xfer) { [...] In this case I like the previous code better: just because there are less letters to read. :-) Best Regards, Michał Mirosław
06.09.2020 01:23, Michał Mirosław пишет: > On Sat, Sep 05, 2020 at 11:41:36PM +0300, Dmitry Osipenko wrote: >> The "dma" variable of tegra_i2c_xfer_msg() function doesn't bring much in >> regards to readability and generation of the code, hence let's remove it >> to clean up code a tad. > [...] >> --- a/drivers/i2c/busses/i2c-tegra.c >> +++ b/drivers/i2c/busses/i2c-tegra.c > [...] >> + if (i2c_dev->is_curr_dma_xfer) { > [...] > > In this case I like the previous code better: just because there are > less letters to read. :-) Besides readability, I also don't like much that the is_curr_dma_xfer is initialized in tegra_i2c_xfer_msg() and then could be overridden by tegra_i2c_config_fifo_trig(). In a result the "dma" variable confuses me since it's not instantly obvious why it's set after tegra_i2c_config_fifo_trig(). Looking at the final result, I think it's better to have the variable removed. It makes code more consistent, IMO.
On Sun, Sep 06, 2020 at 01:36:20AM +0300, Dmitry Osipenko wrote: > 06.09.2020 01:23, Michał Mirosław пишет: > > On Sat, Sep 05, 2020 at 11:41:36PM +0300, Dmitry Osipenko wrote: > >> The "dma" variable of tegra_i2c_xfer_msg() function doesn't bring much in > >> regards to readability and generation of the code, hence let's remove it > >> to clean up code a tad. > > [...] > >> --- a/drivers/i2c/busses/i2c-tegra.c > >> +++ b/drivers/i2c/busses/i2c-tegra.c > > [...] > >> + if (i2c_dev->is_curr_dma_xfer) { > > [...] > > > > In this case I like the previous code better: just because there are > > less letters to read. :-) > > Besides readability, I also don't like much that the is_curr_dma_xfer is > initialized in tegra_i2c_xfer_msg() and then could be overridden by > tegra_i2c_config_fifo_trig(). In a result the "dma" variable confuses me > since it's not instantly obvious why it's set after > tegra_i2c_config_fifo_trig(). > > Looking at the final result, I think it's better to have the variable > removed. It makes code more consistent, IMO. If it could be changed in some callee, then indeed it is better. In this case I would include this information in the commit msg. Best Regards, Michał Mirosław
06.09.2020 01:47, Michał Mirosław пишет: > On Sat, Sep 05, 2020 at 11:41:50PM +0300, Dmitry Osipenko wrote: >> Make all comments to be consistent in regards to capitalization and >> punctuation, correct spelling and grammar errors. > [...] >> - /* Rounds down to not include partial word at the end of buf */ >> + /* rounds down to not include partial word at the end of buffer */ >> words_to_transfer = buf_remaining / BYTES_PER_FIFO_WORD; >> >> - /* It's very common to have < 4 bytes, so optimize that case. */ >> + /* it's very common to have < 4 bytes, so optimize that case */ >> if (words_to_transfer) { >> if (words_to_transfer > tx_fifo_avail) >> words_to_transfer = tx_fifo_avail; >> >> /* >> - * Update state before writing to FIFO. If this casues us >> + * Update state before writing to FIFO. If this causes us >> * to finish writing all bytes (AKA buf_remaining goes to 0) we >> * have a potential for an interrupt (PACKET_XFER_COMPLETE is >> * not maskable). We need to make sure that the isr sees >> @@ -800,8 +799,8 @@ static int tegra_i2c_fill_tx_fifo(struct tegra_i2c_dev *i2c_dev) >> } > > Those first letters don't look consistently capitalized. :-) In my experience, the more common kernel style is a lowercase for single-line comments and the opposite for the multi-line comments. Hence, should be good. If you're meaning something else, then please clarify.
On Sun, Sep 06, 2020 at 01:53:56AM +0300, Dmitry Osipenko wrote: > 06.09.2020 01:47, Michał Mirosław пишет: > > On Sat, Sep 05, 2020 at 11:41:50PM +0300, Dmitry Osipenko wrote: > >> Make all comments to be consistent in regards to capitalization and > >> punctuation, correct spelling and grammar errors. > > [...] > >> - /* Rounds down to not include partial word at the end of buf */ > >> + /* rounds down to not include partial word at the end of buffer */ > >> words_to_transfer = buf_remaining / BYTES_PER_FIFO_WORD; > >> > >> - /* It's very common to have < 4 bytes, so optimize that case. */ > >> + /* it's very common to have < 4 bytes, so optimize that case */ > >> if (words_to_transfer) { > >> if (words_to_transfer > tx_fifo_avail) > >> words_to_transfer = tx_fifo_avail; > >> > >> /* > >> - * Update state before writing to FIFO. If this casues us > >> + * Update state before writing to FIFO. If this causes us > >> * to finish writing all bytes (AKA buf_remaining goes to 0) we > >> * have a potential for an interrupt (PACKET_XFER_COMPLETE is > >> * not maskable). We need to make sure that the isr sees > >> @@ -800,8 +799,8 @@ static int tegra_i2c_fill_tx_fifo(struct tegra_i2c_dev *i2c_dev) > >> } > > > > Those first letters don't look consistently capitalized. :-) > > In my experience, the more common kernel style is a lowercase for > single-line comments and the opposite for the multi-line comments. > Hence, should be good. If you're meaning something else, then please > clarify. I don't have a strong opinion about this, but my preference is: If it's a full sentence, make it look so. Best Regards, Michał Mirosław
On Sat, Sep 05, 2020 at 11:41:20PM +0300, Dmitry Osipenko wrote: > Hello! > > This series performs refactoring of the Tegra I2C driver code and hardens > the atomic-transfer mode. [...] Pending comments, all LGTM. Best Regards, Michał Mirosław
06.09.2020 02:16, Michał Mirosław пишет: > On Sat, Sep 05, 2020 at 11:41:20PM +0300, Dmitry Osipenko wrote: >> Hello! >> >> This series performs refactoring of the Tegra I2C driver code and hardens >> the atomic-transfer mode. > [...] > > Pending comments, all LGTM. Thank you!
Hello! This series performs refactoring of the Tegra I2C driver code and hardens the atomic-transfer mode. Changelog: v4: - Reordered patches in the fixes/features/cleanups order like it was suggested by Andy Shevchenko. - Now using clk-bulk API, which was suggested by Andy Shevchenko. - Reworked "Make tegra_i2c_flush_fifos() usable in atomic transfer" patch to use iopoll API, which was suggested by Andy Shevchenko. - Separated "Clean up probe function" into several smaller patches. - Squashed "Add missing newline before returns" patch into "Clean up whitespaces, newlines and indentation". - The "Drop '_timeout' from wait/poll function names" is renamed to "Rename wait/poll functions". - The "Use reset_control_reset()" is changed to not fail tegra_i2c_init(), but only emit warning. This should be more friendly behaviour in oppose to having a non-bootable machine if reset-control fails. - New patches: i2c: tegra: Remove error message used for devm_request_irq() failure i2c: tegra: Use devm_platform_get_and_ioremap_resource() i2c: tegra: Use platform_get_irq() i2c: tegra: Use clk-bulk helpers i2c: tegra: Remove bogus barrier() i2c: tegra: Factor out register polling into separate function i2c: tegra: Consolidate error handling in tegra_i2c_xfer_msg() i2c: tegra: Clean up and improve comments i2c: tegra: Rename couple "ret" variables to "err" v3: - Optimized "Make tegra_i2c_flush_fifos() usable in atomic transfer" patch by pre-checking FIFO state before starting to poll using ktime API, which may be expensive under some circumstances. - The "Clean up messages in the code" patch now makes all messages to use proper capitalization of abbreviations. Thanks to Andy Shevchenko and Michał Mirosław for the suggestion. - The "Remove unnecessary whitespaces and newlines" patch is transformed into "Clean up whitespaces and newlines", it now also adds missing newlines and spaces. - Reworked the "Clean up probe function" patch in accordance to suggestion from Michał Mirosław by factoring out only parts of the code that make error unwinding cleaner. - Added r-b from Michał Mirosław. - Added more patches: i2c: tegra: Reorder location of functions in the code i2c: tegra: Factor out packet header setup from tegra_i2c_xfer_msg() i2c: tegra: Remove "dma" variable i2c: tegra: Initialization div-clk rate unconditionally i2c: tegra: Remove i2c_dev.clk_divisor_non_hs_mode member v2: - Cleaned more messages in the "Clean up messages in the code" patch. - The error code of reset_control_reset() is checked now. - Added these new patches to clean up couple more things: i2c: tegra: Check errors for both positive and negative values i2c: tegra: Improve coding style of tegra_i2c_wait_for_config_load() i2c: tegra: Remove unnecessary whitespaces and newlines i2c: tegra: Rename variable in tegra_i2c_issue_bus_clear() i2c: tegra: Improve driver module description Dmitry Osipenko (31): i2c: tegra: Make tegra_i2c_flush_fifos() usable in atomic transfer i2c: tegra: Handle potential error of tegra_i2c_flush_fifos() i2c: tegra: Initialization div-clk rate unconditionally i2c: tegra: Remove i2c_dev.clk_divisor_non_hs_mode member i2c: tegra: Runtime PM always available on Tegra i2c: tegra: Remove error message used for devm_request_irq() failure i2c: tegra: Use reset_control_reset() i2c: tegra: Use devm_platform_get_and_ioremap_resource() i2c: tegra: Use platform_get_irq() i2c: tegra: Use clk-bulk helpers i2c: tegra: Factor out runtime PM and hardware initialization i2c: tegra: Move out all device-tree parsing into tegra_i2c_parse_dt() i2c: tegra: Clean up probe function i2c: tegra: Remove likely/unlikely from the code i2c: tegra: Remove bogus barrier() i2c: tegra: Remove "dma" variable from tegra_i2c_xfer_msg() i2c: tegra: Improve formatting of function variables i2c: tegra: Improve coding style of tegra_i2c_wait_for_config_load() i2c: tegra: Rename wait/poll functions i2c: tegra: Rename variable in tegra_i2c_issue_bus_clear() i2c: tegra: Factor out error recovery from tegra_i2c_xfer_msg() i2c: tegra: Factor out packet header setup from tegra_i2c_xfer_msg() i2c: tegra: Factor out register polling into separate function i2c: tegra: Reorder location of functions in the code i2c: tegra: Check errors for both positive and negative values i2c: tegra: Consolidate error handling in tegra_i2c_xfer_msg() i2c: tegra: Clean up printk messages i2c: tegra: Clean up whitespaces, newlines and indentation i2c: tegra: Improve driver module description i2c: tegra: Clean up and improve comments i2c: tegra: Rename couple "ret" variables to "err" drivers/i2c/busses/i2c-tegra.c | 1272 +++++++++++++++----------------- 1 file changed, 612 insertions(+), 660 deletions(-)