Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Wed, 10 Apr 2019 10:47:46 -0700
From:   Guenter Roeck <linux@roeck-us.net>
To:     Joe Perches <joe@perches.com>
Cc:     Wim Van Sebroeck <wim@linux-watchdog.org>,
        linux-watchdog@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 20/22] watchdog: jz4740_wdt: Use 'dev' instead of
 dereferencing it repeatedly
Message-ID: <20190410174746.GA2132@roeck-us.net>
References: <1554913683-25454-1-git-send-email-linux@roeck-us.net>
 <1554913683-25454-21-git-send-email-linux@roeck-us.net>
 <8d44aaf119c286ac18ead30a2174aecef2dbc7e2.camel@perches.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <8d44aaf119c286ac18ead30a2174aecef2dbc7e2.camel@perches.com>
User-Agent: Mutt/1.5.24 (2015-08-30)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi Joe,

On Wed, Apr 10, 2019 at 09:52:09AM -0700, Joe Perches wrote:
> On Wed, 2019-04-10 at 09:28 -0700, Guenter Roeck wrote:
> > Introduce local variable 'struct device *dev' and use it instead of
> > dereferencing it repeatedly. Also, there is no call to dev_get_drvdata()
> > or platform_get_drvdata() in the driver, so drop the unnecessary
> > call to platform_set_drvdata().
> 
> Dropping platform_set_drvdata seems to me like it should
> be a separate patch.
> 

Bundling all changes into one patch per driver already resulted in more than
60 patches total in this series. Splitting that into three sets of patches
over three days already earned me automated replies telling me that I am now
considered to be a spammer. One logical change per patch would have resulted
in hundreds of patches. I don't think that would have scaled well.

I considered other splits, such as one coccinelle rule per patch, affecting
multiple drivers, but that would have had the same result since it would have
needed dozens of Cc: lines per patch. Ultimately, I decided to go with one patch
per file.

> And are you sure no other function uses a get_drvdata call?
> Maybe something in watchdog_dev.c?  Possibly:
> 
>    #ifdef CONFIG_WATCHDOG_SYSFS
>    static ssize_t nowayout_show(struct device *dev, struct device_attribute *attr,
>    				char *buf)
>    {
>    	struct watchdog_device *wdd = dev_get_drvdata(dev);
> 

'dev' in nowayout_show() points to the watchdog device, not to the platform
device. Its drvdata is set in drivers/base/core.c:device_create_groups_vargs().
Not all watchdog drivers are platform drivers, and the watchdog core can not
depend on a watchdog device even having a parent device, much less make
assumptions about its drvdata.

Guenter