Re: [PATCH v3 1/2] misc: rtsx: add to support new card reader rts5264

From: "gregkh@linuxfoundation.org" <gregkh@linuxfoundation.org>
To: Ricky WU <ricky_wu@realtek.com>
Cc: "arnd@arndb.de" <arnd@arndb.de>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"ulf.hansson@linaro.org" <ulf.hansson@linaro.org>,
	"frank.li@vivo.com" <frank.li@vivo.com>,
	"u.kleine-koenig@pengutronix.de" <u.kleine-koenig@pengutronix.de>,
	"yangyingliang@huawei.com" <yangyingliang@huawei.com>,
	"linux-mmc@vger.kernel.org" <linux-mmc@vger.kernel.org>
Subject: Re: [PATCH v3 1/2] misc: rtsx: add to support new card reader rts5264
Date: Fri, 27 Oct 2023 10:44:15 +0200	[thread overview]
Message-ID: <2023102726-voucher-chimp-d798@gregkh> (raw)
In-Reply-To: <ff32ebcd6a1b41d5bd23a028f1a3f88b@realtek.com>

On Fri, Oct 27, 2023 at 08:25:06AM +0000, Ricky WU wrote:
> > On Mon, Oct 23, 2023 at 03:31:24AM +0000, Ricky WU wrote:
> > > > > In order to support new chip rts5264, the definitions of some
> > > > > internal registers and workflow have to be modified.
> > > >
> > > > That is fine, but that should be a single patch, right?
> > > >
> > >
> > > Sorry maybe about misunderstand, The modifications mentioned here, it
> > > talk about some judgment expressions add "PID 5264" to make judgement
> > > in rtsx_pcr.c, so only about 30 line modified in rtsx_pcr.c
> > >
> > > > > Added rts5264.c rts5264.h for independent functions of the new
> > > > > chip rts5264
> > > >
> > > > And then add new support in a new patch, this is still too big as
> > > > one patch to attempt to review it properly.  Would you like to
> > > > review this as-is?
> > > >
> > >
> > > Yes, thank you
> > > Because rts5264.c rts5264.h only for rts5264 (new chip).
> > > The past architecture of this driver was like this, and it will good
> > > for us to maintain the driver different chip maybe has different
> > > functions and register definitions we used to separate different .c .h
> > 
> > Sorry, I don't think I was clear, this needs to be broken up into smaller pieces to
> > be able for us to review it properly.  Please do so and resend a new version of
> > the patch series.
> > 
> 
> Hi Greg k-h，
> 
> I want to confirm with you first
> I will have a new version for this patch, separate it into:
> 1. 2 new file and Makefile for new chip function and definition 
> 2. rtsx_pcr.c and rtsx_pcr.h for driver judgement to call new chip function  
> 3. mmc part
> 
> It is ok for you?

That's a good start, try it and see what that looks like.

Again, create patch series that you yourself would like to be able to
review.  Take some time to think about the reviewer's side of this as
that's the most limited resource we currently have.

thanks,

greg k-h