Re: çå: [PATCH] mfd: rtsx: Add support for RTS5260

[Lee Jones]

On Thu, 14 Sep 2017, åé wrote:

> On Fri, 08 Sep 2017, rui_feng@xxxxxxxxxxxxxx wrote:
> 
> > From: rui_feng <rui_feng@xxxxxxxxxxxxxx>
> > 
> > Add support for new chip rts5260.
> 
> You are adding over 600 lines in this patch.  It desearves a more forthcoming commit message.
> 
> What is it?
> What functionality does it provide?
> What other subsystems are involved?
> Could it break something else?
> Does it have any dependencies?
> Etc etc.
> 
> > Signed-off-by: Rui Feng <rui_feng@xxxxxxxxxxxxxx>
> > ---
> >  drivers/mfd/Makefile         |   2 +-
> >  drivers/mfd/rts5260.c        | 543 +++++++++++++++++++++++++++++++++++++++++++
> >  drivers/mfd/rtsx_pcr.c       |   8 +
> >  drivers/mfd/rtsx_pcr.h       |   1 +
> >  include/linux/mfd/rtsx_pci.h |  81 ++++++-
> >  5 files changed, 631 insertions(+), 4 deletions(-)  create mode 
> > 100644 drivers/mfd/rts5260.c
> > 
> > diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile index 
> > 080793b..d0f06a3 100644
> > --- a/drivers/mfd/Makefile
> > +++ b/drivers/mfd/Makefile
> > @@ -17,7 +17,7 @@ obj-$(CONFIG_MFD_CROS_EC_I2C)	+= cros_ec_i2c.o
> >  obj-$(CONFIG_MFD_CROS_EC_SPI)	+= cros_ec_spi.o
> >  obj-$(CONFIG_MFD_EXYNOS_LPASS)	+= exynos-lpass.o
> >  
> > -rtsx_pci-objs			:= rtsx_pcr.o rts5209.o rts5229.o rtl8411.o rts5227.o rts5249.o
> > +rtsx_pci-objs			:= rtsx_pcr.o rts5209.o rts5229.o rtl8411.o rts5227.o rts5249.o rts5260.o
> >  obj-$(CONFIG_MFD_RTSX_PCI)	+= rtsx_pci.o
> >  obj-$(CONFIG_MFD_RTSX_USB)	+= rtsx_usb.o
> >  
> > diff --git a/drivers/mfd/rts5260.c b/drivers/mfd/rts5260.c new file 
> > mode 100644 index 0000000..53f2838
> > --- /dev/null
> > +++ b/drivers/mfd/rts5260.c
> 
> There is way too much code in this file to be an MFD driver.
> 
> It's a card reader driver (as stated below).
> 
> I think this entire file needs relocating somewhere else.

Your reply is confusing, since it does not quote my previous messages.

Please fix your mailer to quote the text you are replying to.

I think I've asked this before.

> Many other drivers, such as rts5209, rts5227, rts5229, rts5249 and so on, are in the same place.
> If I put rts5260 somewhere else, other drivers should also be relocated, this will lead to a much bigger modification.
> So I think put rts5260.c here is better suited.

Just because we made mistakes before, it doesn't mean we should keep
making them.  I think the correct solution is for them all to be
relocated.

> > @@ -0,0 +1,543 @@
> > +/* 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.
> > + *
> > + * 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@xxxxxxxxxxxxxx>
> > + *   Rui FENG <rui_feng@xxxxxxxxxxxxxx>
> > + *   Wei WANG <wei_wang@xxxxxxxxxxxxxx>
> > + */
> 

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