lkml.org 
[lkml]   [2020]   [May]   [18]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH v2 18/19] spi: dw: Use regset32 DebugFS method to create regdump file
    On Mon, May 18, 2020 at 05:08:25PM +0300, Serge Semin wrote:
    > On Mon, May 18, 2020 at 02:18:22PM +0300, Andy Shevchenko wrote:
    > > On Sat, May 16, 2020 at 11:46:34PM +0300, Serge Semin wrote:
    > > > On Fri, May 15, 2020 at 06:10:56PM +0300, Andy Shevchenko wrote:
    > > > > On Fri, May 15, 2020 at 01:47:57PM +0300, Serge Semin wrote:
    > > > > > DebugFS kernel interface provides a dedicated method to create the
    > > > > > registers dump file. Use it instead of creating a generic DebugFS
    > > > > > file with manually written read callback function.

    > > > > With below nit addressed,
    > > > > Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
    > >
    > > > > > +#define DW_SPI_DBGFS_REG(_name, _off) \
    > > > > > +{ \
    > > > > > + .name = _name, \
    > > > >
    > > > > > + .offset = _off \
    > > > >
    > > > > As previously discussed (did I miss your answer?) the comma at the end leaves
    > > > > better pattern for maintenance prospective.
    > > >
    > > > Ah, sorry. Missed that. This comma is hardly needed seeing the structure
    > > > consists of just two elements. So I'd rather leave it as is.
    > >
    > > While it's a really small thing, I consider that it's not good to make
    > > someone's else problem what can be done here. So, please, consider to add a
    > > comma. Look at the other drivers and code in the kernel. This is at least
    > > defacto preferred style.
    >
    > Andy, you never give up, don't you? =)

    First of all, I really appreciate work you have done so far (I mean it).
    Now to the point.

    You see, I always try to have a rationale behind any proposed comment. I agree,
    that some of them can be considered as a bikeshedding for a certain developer,
    but on big picture with this scale of project even small change (being made or
    being rejected to be made) can provoke additional churn with a good magnitude,
    if we consider all possible cases where somebody, e.g., can take into account
    existing code to copy'n'paste from). So, I would easy give up on something if
    there will be a stronger (than mine) argument why the proposed thing is not
    good to be done (at least as a part of the discussed patch set). I also want
    to and will learn from the developers as a reviewer.

    Hope that above will clarify my reviewer's point of view to the code.

    > Agreed then. I'll add comma to the
    > initializer and also after the last member here:
    > DW_SPI_DBGFS_REG("ISR", DW_SPI_ISR),
    > DW_SPI_DBGFS_REG("DMACR", DW_SPI_DMACR),
    > DW_SPI_DBGFS_REG("DMATDLR", DW_SPI_DMATDLR),
    > - DW_SPI_DBGFS_REG("DMARDLR", DW_SPI_DMARDLR)
    > + DW_SPI_DBGFS_REG("DMARDLR", DW_SPI_DMARDLR),

    Good catch, thanks for taking it into consideration as well.

    > };
    >
    > static int dw_spi_debugfs_init(struct dw_spi *dws)

    --
    With Best Regards,
    Andy Shevchenko


    \
     
     \ /
      Last update: 2020-05-18 17:09    [W:3.312 / U:0.800 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site