From mboxrd@z Thu Jan 1 00:00:00 1970 From: Matti Vaittinen Subject: Re: [RFC PATCH v4 07/10] gpio: Initial support for ROHM bd70528 GPIO block Date: Mon, 4 Feb 2019 14:25:11 +0200 Message-ID: <20190204122511.GG23791@localhost.localdomain> References: <5f63684b62c7a320c514088bcf9091ed7efbd2cc.1548935790.git.matti.vaittinen@fi.rohmeurope.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Return-path: Content-Disposition: inline In-Reply-To: Sender: linux-kernel-owner@vger.kernel.org To: Linus Walleij Cc: Matti Vaittinen , heikki.haikola@fi.rohmeurope.com, mikko.mutanen@fi.rohmeurope.com, Lee Jones , Rob Herring , Mark Rutland , Mark Brown , Greg KH , "Rafael J. Wysocki" , Michael Turquette , Stephen Boyd , Bartosz Golaszewski , Sebastian Reichel , Liam Girdwood , Alessandro Zummo , Alexandre Belloni , Wim Van Sebroeck , Guenter Roeck , open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS List-Id: devicetree@vger.kernel.org On Mon, Feb 04, 2019 at 12:29:53PM +0100, Linus Walleij wrote: > On Thu, Jan 31, 2019 at 1:08 PM Matti Vaittinen > wrote: > > > ROHM BD70528 PMIC has 4 GPIO pins. Allow them to be > > controlled by GPIO framework. > > > > IRQs are handled by regmap-irq and GPIO driver is not > > aware of the irq usage. > > > > Signed-off-by: Matti Vaittinen > (...) > > I dropped the review-by from Linus Walleij because I would like to > > get a comment on if locking is required when we check the direction > > in order to detect the correct register for getting the pin state. > > I don't know that. You isn't regmap locking inherently? This direction fetching and then the value reading are two separate operations at regmap-level. I guess the regmap can't be holding any locks during two indpendent operations. > > > My initial feeling is that locking makes no sense. > > Mine too. Yep. I change the comment to explain why I think this race condition is Ok. We can fix it if it ever turns out to be an issue. > > > + bdgpio->gpio.get_direction = &bd70528_get_direction; > > + bdgpio->gpio.direction_input = &bd70528_direction_input; > > + bdgpio->gpio.direction_output = &bd70528_direction_output; > > + bdgpio->gpio.set_config = &bd70528_gpio_set_config; > > + bdgpio->gpio.can_sleep = true; > > + bdgpio->gpio.get = &bd70528_gpio_get; > > + bdgpio->gpio.set = &bd70528_gpio_set; > > Drop the &ersand in from of the functions. All functions > are pointers. Yes they are. This & in front of functions when initializing a pointer with their addresss is my old habit. I liked to consistently use & when taking address of any beast, whether it was a variable or function. Getting rid of old habits is not so easy :/ I'll clean thisi at v5 and put back your Reviewed-by =) Thanks > > With that: > Reviewed-by: Linus Walleij > > Yours, > Linus Walleij -- Matti Vaittinen, Linux device drivers ROHM Semiconductors, Finland SWDC Kiviharjunlenkki 1E 90220 OULU FINLAND ~~~ "I don't think so," said Rene Descartes. Just then, he vanished ~~~