From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S965197AbbLRWSG (ORCPT ); Fri, 18 Dec 2015 17:18:06 -0500 Received: from mail-pa0-f43.google.com ([209.85.220.43]:35563 "EHLO mail-pa0-f43.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932486AbbLRWSC (ORCPT ); Fri, 18 Dec 2015 17:18:02 -0500 Date: Fri, 18 Dec 2015 14:17:58 -0800 From: Brian Norris To: Boris Brezillon Cc: David Woodhouse , linux-mtd@lists.infradead.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, Jonathan Corbet , linux-doc@vger.kernel.org, Hartley Sweeten , Ryan Mallon , Shawn Guo , Sascha Hauer , Imre Kaloz , Krzysztof Halasa , Tony Lindgren , linux-omap@vger.kernel.org, Alexander Clouter , Thomas Petazzoni , Gregory CLEMENT , Jason Cooper , Sebastian Hesselbarth , Andrew Lunn , Daniel Mack , Haojian Zhuang , Robert Jarzmik , Marek Vasut , Steven Miao , adi-buildroot-devel@lists.sourceforge.net, Mikael Starvik , Jesper Nilsson , linux-cris-kernel@axis.com, Josh Wu , Wan ZongShun , Ezequiel Garcia , Maxim Levitsky , Kukjin Kim , Krzysztof Kozlowski , linux-samsung-soc@vger.kernel.org, Maxime Ripard , Chen-Yu Tsai , linux-sunxi@googlegroups.com, Stefan Agner , Greg Kroah-Hartman , devel@driverdev.osuosl.org Subject: Re: [PATCH v4 55/58] mtd: nand: add helpers to access ->priv Message-ID: <20151218221758.GQ10460@google.com> References: <1449734442-18672-1-git-send-email-boris.brezillon@free-electrons.com> <1449734442-18672-56-git-send-email-boris.brezillon@free-electrons.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1449734442-18672-56-git-send-email-boris.brezillon@free-electrons.com> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Boris, On Thu, Dec 10, 2015 at 09:00:39AM +0100, Boris Brezillon wrote: > Add two helpers to access the field reserved for private controller data. > This makes it clearer what this field is reserved for and ease future > refactoring. I agree with the refactoring part, but I'm not sure about the name. Is it really "controller" data? That sounds like something that has 1 instance per controller. But the way this is sometimes used, we get 1 instance per NAND chip, and there may be more than one NAND chip per controller. So at the moment, this is more like opaque "driver data", like dev_{get,set}_drvdata(), which doesn't really have a prescribed use -- it's up to the driver. Notably, we already have a (sort of) 1-per-controler-instance field: struct nand_hw_control (I notice we have both the 'controller' and 'hwcontrol' fields in nand_chip; that's pretty ugly too...). Those don't have private data fields, but we could of course extend that if we really want "controller" data. Anyway, I don't feel like this question is resolved well enough to say that we should go change all drivers to use these accessors. I know you have bigger plans for putting more "controller" infrastructure into the core drivers/mtd/nand/ code, so I'd like to see how that fits in here. (If we're going to discuss this much more, I'd suggest a smaller CC list. I'm mostly putting this here to show why I'm not taking the last 4 patches right now.) Regards, Brian > Signed-off-by: Boris Brezillon > --- > include/linux/mtd/nand.h | 10 ++++++++++ > 1 file changed, 10 insertions(+) > > diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h > index 2bee2e4..4aed4b2 100644 > --- a/include/linux/mtd/nand.h > +++ b/include/linux/mtd/nand.h > @@ -739,6 +739,16 @@ static inline struct mtd_info *nand_to_mtd(struct nand_chip *chip) > return &chip->mtd; > } > > +static inline void *nand_get_controller_data(struct nand_chip *chip) > +{ > + return chip->priv; > +} > + > +static inline void nand_set_controller_data(struct nand_chip *chip, void *priv) > +{ > + chip->priv = priv; > +} > + > /* > * NAND Flash Manufacturer ID Codes > */ > -- > 2.1.4 > From mboxrd@z Thu Jan 1 00:00:00 1970 From: Brian Norris Subject: Re: [PATCH v4 55/58] mtd: nand: add helpers to access ->priv Date: Fri, 18 Dec 2015 14:17:58 -0800 Message-ID: <20151218221758.GQ10460@google.com> References: <1449734442-18672-1-git-send-email-boris.brezillon@free-electrons.com> <1449734442-18672-56-git-send-email-boris.brezillon@free-electrons.com> Reply-To: computersforpeace-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Return-path: Content-Disposition: inline In-Reply-To: <1449734442-18672-56-git-send-email-boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> List-Post: , List-Help: , List-Archive: , List-Unsubscribe: , To: Boris Brezillon Cc: David Woodhouse , linux-mtd-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org, linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Jonathan Corbet , linux-doc-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Hartley Sweeten , Ryan Mallon , Shawn Guo , Sascha Hauer , Imre Kaloz , Krzysztof Halasa , Tony Lindgren , linux-omap-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Alexander Clouter , Thomas Petazzoni , Gregory CLEMENT , Jason Cooper , Sebastian Hesselbarth , Andrew Lunn , Daniel Mack , Haojian Zhuang , Robert Jarzmik , Marek List-Id: linux-omap@vger.kernel.org Hi Boris, On Thu, Dec 10, 2015 at 09:00:39AM +0100, Boris Brezillon wrote: > Add two helpers to access the field reserved for private controller data. > This makes it clearer what this field is reserved for and ease future > refactoring. I agree with the refactoring part, but I'm not sure about the name. Is it really "controller" data? That sounds like something that has 1 instance per controller. But the way this is sometimes used, we get 1 instance per NAND chip, and there may be more than one NAND chip per controller. So at the moment, this is more like opaque "driver data", like dev_{get,set}_drvdata(), which doesn't really have a prescribed use -- it's up to the driver. Notably, we already have a (sort of) 1-per-controler-instance field: struct nand_hw_control (I notice we have both the 'controller' and 'hwcontrol' fields in nand_chip; that's pretty ugly too...). Those don't have private data fields, but we could of course extend that if we really want "controller" data. Anyway, I don't feel like this question is resolved well enough to say that we should go change all drivers to use these accessors. I know you have bigger plans for putting more "controller" infrastructure into the core drivers/mtd/nand/ code, so I'd like to see how that fits in here. (If we're going to discuss this much more, I'd suggest a smaller CC list. I'm mostly putting this here to show why I'm not taking the last 4 patches right now.) Regards, Brian > Signed-off-by: Boris Brezillon > --- > include/linux/mtd/nand.h | 10 ++++++++++ > 1 file changed, 10 insertions(+) > > diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h > index 2bee2e4..4aed4b2 100644 > --- a/include/linux/mtd/nand.h > +++ b/include/linux/mtd/nand.h > @@ -739,6 +739,16 @@ static inline struct mtd_info *nand_to_mtd(struct nand_chip *chip) > return &chip->mtd; > } > > +static inline void *nand_get_controller_data(struct nand_chip *chip) > +{ > + return chip->priv; > +} > + > +static inline void nand_set_controller_data(struct nand_chip *chip, void *priv) > +{ > + chip->priv = priv; > +} > + > /* > * NAND Flash Manufacturer ID Codes > */ > -- > 2.1.4 > From mboxrd@z Thu Jan 1 00:00:00 1970 From: computersforpeace@gmail.com (Brian Norris) Date: Fri, 18 Dec 2015 14:17:58 -0800 Subject: [PATCH v4 55/58] mtd: nand: add helpers to access ->priv In-Reply-To: <1449734442-18672-56-git-send-email-boris.brezillon@free-electrons.com> References: <1449734442-18672-1-git-send-email-boris.brezillon@free-electrons.com> <1449734442-18672-56-git-send-email-boris.brezillon@free-electrons.com> Message-ID: <20151218221758.GQ10460@google.com> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org Hi Boris, On Thu, Dec 10, 2015 at 09:00:39AM +0100, Boris Brezillon wrote: > Add two helpers to access the field reserved for private controller data. > This makes it clearer what this field is reserved for and ease future > refactoring. I agree with the refactoring part, but I'm not sure about the name. Is it really "controller" data? That sounds like something that has 1 instance per controller. But the way this is sometimes used, we get 1 instance per NAND chip, and there may be more than one NAND chip per controller. So at the moment, this is more like opaque "driver data", like dev_{get,set}_drvdata(), which doesn't really have a prescribed use -- it's up to the driver. Notably, we already have a (sort of) 1-per-controler-instance field: struct nand_hw_control (I notice we have both the 'controller' and 'hwcontrol' fields in nand_chip; that's pretty ugly too...). Those don't have private data fields, but we could of course extend that if we really want "controller" data. Anyway, I don't feel like this question is resolved well enough to say that we should go change all drivers to use these accessors. I know you have bigger plans for putting more "controller" infrastructure into the core drivers/mtd/nand/ code, so I'd like to see how that fits in here. (If we're going to discuss this much more, I'd suggest a smaller CC list. I'm mostly putting this here to show why I'm not taking the last 4 patches right now.) Regards, Brian > Signed-off-by: Boris Brezillon > --- > include/linux/mtd/nand.h | 10 ++++++++++ > 1 file changed, 10 insertions(+) > > diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h > index 2bee2e4..4aed4b2 100644 > --- a/include/linux/mtd/nand.h > +++ b/include/linux/mtd/nand.h > @@ -739,6 +739,16 @@ static inline struct mtd_info *nand_to_mtd(struct nand_chip *chip) > return &chip->mtd; > } > > +static inline void *nand_get_controller_data(struct nand_chip *chip) > +{ > + return chip->priv; > +} > + > +static inline void nand_set_controller_data(struct nand_chip *chip, void *priv) > +{ > + chip->priv = priv; > +} > + > /* > * NAND Flash Manufacturer ID Codes > */ > -- > 2.1.4 >