Re: 答复: 答复: 答复: [PATCH] mfd: rtsx: Add support for RTS5260

[Lee Jones <lee.jones@linaro.org>]

On Thu, 12 Oct 2017, 冯锐 wrote:

> > On Fri, 29 Sep 2017, 冯锐 wrote:
> > > > On Fri, 22 Sep 2017, 冯锐 wrote:
> > > >
> > > > > > On Fri, Sep 22, 2017 at 05:36:24PM +0800, rui_feng@realsil.com.cn
> > wrote:
> > > > > > > From: rui_feng <rui_feng@realsil.com.cn>
> > > > > > >
> > > > > > > Add support for new chip rts5260.
> > > > > > > In order to support rts5260，the definitions of some internal
> > > > > > > registers and workflow have to be modified and are different
> > > > > > > from its predecessors. So we need this patch to ensure RTS5260 can
> > work.
> > > > > > >
> > > > > > > Signed-off-by: Rui Feng <rui_feng@realsil.com.cn>
> > > > > >
> > > > > > Nit, this name needs to match your "From:" line.  It almost does
> > > > > > :)
> > > > > >
> > > > > > Why is this a drivers/misc/ driver?
> > > > > >
> > > > > I want to put this driver in mfd because other drivers like
> > > > > rts5249,rts5227 and so on are in mfd, but lee jones said " There
> > > > > is way too
> > > > much code in this file to be an MFD driver.
> > > >
> > > > There's more to it than that.
> > > >
> > > > It's not an MFD driver because it's a card reader an nothing else.
> > > >
> > > > MFDs cross subsystem boundaries, whereas this is just a card reader.
> > > >
> > > > MFD isn't a dumping ground for devices which don't belong anywhere else.
> > >
> > > The functions in rts5260.c are called by
> > > drivers/mmc/host/rtsx_pci_sdmmc.c, it cross
> > 
> > What subsystem boundary does it cross?  MMC and ... ?
> > 
> Rts5260 support SD card and memstick, it cross mmc and memstick, 
> and it's a multifunction device, so I think put it in mfd is OK.

You're persistent, I'll give you that.

I already know you think it's okay.  But I don't think it is.

There is wayyyyyy to much actual functionality contained in this
driver to be an MFD, and it's all related to reading block devices.
It sounds like there should be somewhere for MMC and Memstick to place
shared code.  MFD is not that place however.

> > > Because many other similar drivers are in mfd, if relocate rts5260 now
> > > will lead to a much bigger modification,
> > 
> > Totally fine.
> > 
> > > and customer is in need of this patch. I think put rts5260 in mfd is
> > > better now.
> > 
> > The upstream kernel is not customer focused.
> > 
> > > If mfd is really not the right place,
> > 
> > MFD is not the right place.
> > 
> > > I will relocate it later not in this patch.
> > > Greg, what do you think of putting rts5260 in mfd?
> > 
> > Trying to leap-frog me will only seek to annoy and will not help your cause.
> > 
> > > > > So I put it here.
> > > >
> > > > Without creating a new subsystem, I can't think of a better place for it
> > either.
> > > >
> > > > > > > --- /dev/null
> > > > > > > +++ b/drivers/misc/rts5260.c
> > > > > > > @@ -0,0 +1,590 @@
> > > > > > > +/* Driver for Realtek PCI-Express card reader
> > > > > > > + *
> > > > > > > + * Copyright(c) 2016-2017 Realtek Semiconductor Corp. All
> > > > > > > +rights
> > > > reserved.
> > > > > > > + *
> > > > > > > + * This program is free software; you can redistribute it
> > > > > > > +and/or modify it
> > > > > > > + * under the terms of the GNU General Public License as
> > > > > > > +published by the
> > > > > > > + * Free Software Foundation; either version 2, or (at your
> > > > > > > +option) any
> > > > > > > + * later version.
> > > > > >
> > > > > > Do you really mean "any later version"?  (I have to ask...)
> > > > > >
> > > > > > > + *
> > > > > > > + * This program is distributed in the hope that it will be
> > > > > > > + useful, but
> > > > > > > + * WITHOUT ANY WARRANTY; without even the implied warranty of
> > > > > > > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
> > See
> > > > the
> > > > > > GNU
> > > > > > > + * General Public License for more details.
> > > > > > > + *
> > > > > > > + * You should have received a copy of the GNU General Public
> > > > > > > + License along
> > > > > > > + * with this program; if not, see <http://www.gnu.org/licenses/>.
> > > > > > > + *
> > > > > > > + * Author:
> > > > > > > + *   Steven FENG <steven_feng@realsil.com.cn>
> > > > > > > + *   Rui FENG <rui_feng@realsil.com.cn>
> > > > > > > + *   Wei WANG <wei_wang@realsil.com.cn>
> > > > > > > + */
> > > > > > > +
> > > > > > > +#include <linux/module.h>
> > > > > > > +#include <linux/delay.h>
> > > > > > > +#include <linux/mfd/rtsx_pci.h>
> > > > > > > +
> > > > > > > +#include "../mfd/rtsx_pcr.h"
> > > > > >
> > > > > > That looks "odd" :(
> > > >
> > > > This is odd.
> > > >
> > > > You should move that file into include/.
> > > >
> > 

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog