From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1759961AbcIWNPR (ORCPT ); Fri, 23 Sep 2016 09:15:17 -0400 Received: from anholt.net ([50.246.234.109]:44993 "EHLO anholt.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1758684AbcIWNPP (ORCPT ); Fri, 23 Sep 2016 09:15:15 -0400 From: Eric Anholt To: Linus Walleij Cc: linux-rpi-kernel , "linux-arm-kernel\@lists.infradead.org" , "linux-kernel\@vger.kernel.org" , Stephen Warren , Lee Jones , bcm-kernel-feedback-list , Alexandre Courbot , Rob Herring , Mark Rutland , Gerd Hoffmann Subject: Re: [PATCH 2/3] gpio: Add a driver for the Raspberry Pi's firmware GPIO calls. In-Reply-To: References: <20160919161314.25858-1-eric@anholt.net> <20160919161314.25858-2-eric@anholt.net> User-Agent: Notmuch/0.22.2 (http://notmuchmail.org) Emacs/24.5.1 (x86_64-pc-linux-gnu) Date: Fri, 23 Sep 2016 16:15:03 +0300 Message-ID: <87mviy92qw.fsf@eliezer.anholt.net> MIME-Version: 1.0 Content-Type: multipart/signed; boundary="=-=-="; micalg=pgp-sha512; protocol="application/pgp-signature" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org --=-=-= Content-Type: text/plain Linus Walleij writes: > On Mon, Sep 19, 2016 at 6:13 PM, Eric Anholt wrote: > >> This driver will be used for accessing the FXL6408 GPIO exander on the >> Pi3. We can't drive it directly from Linux because the firmware is >> continuously polling one of the expander's lines to do its >> undervoltage detection. >> >> Signed-off-by: Eric Anholt > (...) > >> +config GPIO_RASPBERRYPI >> + tristate "Raspberry Pi firmware-based GPIO access" >> + depends on OF_GPIO && RASPBERRYPI_FIRMWARE && (ARCH_BCM2835 || COMPILE_TEST) >> + help >> + Turns on support for using the Raspberry Pi firmware to >> + control GPIO pins. Used for access to the FXL6408 GPIO >> + expander on the Pi 3. > > Maybe it should be named GPIO_RPI_FXL6408 ? > > (No strong opinion.) See DT binding comment -- I think since this driver has no dependency on being to the 6408 on the pi3, we shouldn't needlessly bind it to the FXL6408. (the help comment was just context for why you would want the driver today). >> +static int rpi_gpio_dir_in(struct gpio_chip *gc, unsigned off) >> +{ >> + /* We don't have direction control. */ >> + return -EINVAL; >> +} >> + >> +static int rpi_gpio_dir_out(struct gpio_chip *gc, unsigned off, int val) >> +{ >> + /* We don't have direction control. */ >> + return -EINVAL; >> +} > > IMO this should return OK if you try to set it to the direction > that the line is hardwired for in that case, not just fail everything. > > If you return errors here, any generic driver that tries to > set the line as input or output will fail and then require a > second workaround in that driver if it is used on rpi etc etc. > > Try to return zero if the consumer asks for the direction that > the line is set to. > > Also implement .get_direction(). Doing so will show how to > do the above two calls in the right way. I was worried about the lack of direction support. The firmware interface doesn't give me anything for direction, and a get or set of the value doesn't try to set direction. I can try to bother them to give me support for that, but if they cooperate on that it means that users will only get HDMI detection once they update firmware. The alternative to new firmware interface would be to provide a bunch of DT saying which of these should be in/out at boot time and refuse to change them after that. That seems like a mess, though. >> +static void rpi_gpio_set(struct gpio_chip *gc, unsigned off, int val) >> +{ >> + struct rpi_gpio *rpi = container_of(gc, struct rpi_gpio, gc); >> + u32 packet[2] = { rpi->offset + off, val }; >> + int ret; >> + >> + ret = rpi_firmware_property(rpi->firmware, >> + RPI_FIRMWARE_SET_GPIO_STATE, >> + &packet, sizeof(packet)); >> + if (ret) >> + dev_err(rpi->dev, "Error setting GPIO %d state: %d\n", off, ret); >> +} >> + >> +static int rpi_gpio_get(struct gpio_chip *gc, unsigned off) >> +{ >> + struct rpi_gpio *rpi = container_of(gc, struct rpi_gpio, gc); >> + u32 packet[2] = { rpi->offset + off, 0 }; >> + int ret; >> + >> + ret = rpi_firmware_property(rpi->firmware, >> + RPI_FIRMWARE_GET_GPIO_STATE, >> + &packet, sizeof(packet)); >> + if (ret) { >> + dev_err(rpi->dev, "Error getting GPIO state: %d\n", ret); >> + return ret; >> + } else if (packet[0]) { >> + dev_err(rpi->dev, "Firmware error getting GPIO state: %d\n", >> + packet[0]); >> + return -EINVAL; >> + } else { >> + return packet[1]; >> + } > > You can just remove the last } else { } clause and return on a > single line. > > Please do it like this though: > > return !!packet[1]; > > So you clamp the returned value to a boolean. Will do. >> + rpi->gc.get = rpi_gpio_get; >> + rpi->gc.set = rpi_gpio_set; >> + rpi->gc.can_sleep = true; >> + >> + ret = gpiochip_add(&rpi->gc); >> + if (ret) >> + return ret; > > Use devm_gpiochip_add_data() and pass NULL as data > so you can still use the devm* function. Oh, nice. >> diff --git a/include/soc/bcm2835/raspberrypi-firmware.h b/include/soc/bcm2835/raspberrypi-firmware.h >> index 3fb357193f09..671ccd00aea2 100644 >> --- a/include/soc/bcm2835/raspberrypi-firmware.h >> +++ b/include/soc/bcm2835/raspberrypi-firmware.h >> @@ -73,11 +73,13 @@ enum rpi_firmware_property_tag { >> RPI_FIRMWARE_GET_DISPMANX_RESOURCE_MEM_HANDLE = 0x00030014, >> RPI_FIRMWARE_GET_EDID_BLOCK = 0x00030020, >> RPI_FIRMWARE_GET_DOMAIN_STATE = 0x00030030, >> + RPI_FIRMWARE_GET_GPIO_STATE = 0x00030041, >> RPI_FIRMWARE_SET_CLOCK_STATE = 0x00038001, >> RPI_FIRMWARE_SET_CLOCK_RATE = 0x00038002, >> RPI_FIRMWARE_SET_VOLTAGE = 0x00038003, >> RPI_FIRMWARE_SET_TURBO = 0x00038009, >> RPI_FIRMWARE_SET_DOMAIN_STATE = 0x00038030, >> + RPI_FIRMWARE_SET_GPIO_STATE = 0x00038041, > > Can you please merge this orthogonally into the rpi tree to ARM SoC? This driver would appear in the rpi downstream tree once we settle the driver here. Or are you asking me to delay this series until I can get them to pull just a patch extending the set of packets? --=-=-= Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIcBAEBCgAGBQJX5SrXAAoJELXWKTbR/J7oOfIP/0Y1HeAzDmz1mSexO9XF+3eh ctSrIRc/YD0ZwNR8VrB4qoIxDkWTChKDps63C/osvxx2PjE/XG/OBG/CXE+eX7z1 TJIW2YbOToPcVwPmS1g5pxVnZqvE7AdjOG9h5UZbVUwf+Ka4iOAiA5xNdD4Nyu+H MteWn8VgwowF7nv45RRcd4ElFD1WcRLEkPuXHlOhFfVDAHF9gWn1ACZ2yZYPVPrN gs8HjySO+HByRcPBUikw8AGC0NrBaOjPpIXl8bLbCm1XQG/xOdZnV2fsM8Z89YCY WvfZDh9jQmnSR5bVLYVNtS4KgcB9vcIrJ/0Mkr6ajp8QD7EISkOuwI7hgYT7SKAr 1e61gL7BdO9q9Z2epaZsaCeVt9O3j0AheAT0v36I5wtPlSIPFbBu97vXtDi1nIgo cG6bg3i3NkP09iU7WyAopPO6xUETLVK/H9HpsCLTepvBAx6995Gd2EE9ijGCKoGD WQYM7fORjDR87v/xvztN/KkzLIK4ZzDI0Fswa6pFQ7LzyO0vppEkXEmlmb6NNy+r 6WBrpKemp6wIA4SPDWkDr0ddXO2YRbq1+wreQaL/MRgq5h2ljf8jdiiH+pmIqgKJ 9PfwJWUBdvrAU9qtfA3VQ+/lPs7yVyFlhj6bgO3OBMJS3XEy3aoBLVFnl2H99M6v ztpUD5ldi6z/cgcHyVAg =VIxD -----END PGP SIGNATURE----- --=-=-=--