From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754132AbdFNB1u (ORCPT ); Tue, 13 Jun 2017 21:27:50 -0400 Received: from regular1.263xmail.com ([211.150.99.138]:49955 "EHLO regular1.263xmail.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754096AbdFNB1t (ORCPT ); Tue, 13 Jun 2017 21:27:49 -0400 X-263anti-spam: KSV:0;BIG:0; X-MAIL-GRAY: 0 X-MAIL-DELIVERY: 1 X-KSVirus-check: 0 X-ADDR-CHECKED4: 1 X-ABS-CHECKED: 1 X-SKE-CHECKED: 1 X-ANTISPAM-LEVEL: 2 X-RL-SENDER: jeffy.chen@rock-chips.com X-FST-TO: briannorris@chromium.org X-SENDER-IP: 103.29.142.67 X-LOGIN-NAME: jeffy.chen@rock-chips.com X-UNIQUE-TAG: X-ATTACHMENT-NUM: 0 X-DNS-TYPE: 0 Message-ID: <5940910D.1020504@rock-chips.com> Date: Wed, 14 Jun 2017 09:27:41 +0800 From: jeffy User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:19.0) Gecko/20130126 Thunderbird/19.0 MIME-Version: 1.0 To: Brian Norris CC: linux-kernel@vger.kernel.org, broonie@kernel.org, dianders@chromium.org, heiko@sntech.de, linux-spi@vger.kernel.org, linux-rockchip@lists.infradead.org, linux-arm-kernel@lists.infradead.org Subject: Re: [PATCH v2 2/4] spi: rockchip: add support for "cs-gpios" dts property References: <1497331543-8565-1-git-send-email-jeffy.chen@rock-chips.com> <1497331543-8565-2-git-send-email-jeffy.chen@rock-chips.com> <20170613173346.GB9026@google.com> In-Reply-To: <20170613173346.GB9026@google.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Brian, Thanx for your comments :) On 06/14/2017 01:33 AM, Brian Norris wrote: > Hi Jeffy, > > On Tue, Jun 13, 2017 at 01:25:41PM +0800, Jeffy Chen wrote: >> Support using "cs-gpios" property to specify cs gpios. >> >> Signed-off-by: Jeffy Chen >> 1/ request cs gpios in probe for better error handling >> 2/ use gpiod* function >> (suggested by Heiko Stuebner) >> >> 3/ split dt-binding changes to new patch >> (suggested by Shawn Lin & Heiko Stuebner) >> >> --- >> >> Changes in v2: None >> >> drivers/spi/spi-rockchip.c | 30 +++++++++++++++++++++++++++++- >> 1 file changed, 29 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/spi/spi-rockchip.c b/drivers/spi/spi-rockchip.c >> index bab9b13..ad8997b 100644 >> --- a/drivers/spi/spi-rockchip.c >> +++ b/drivers/spi/spi-rockchip.c >> @@ -16,7 +16,7 @@ >> #include >> #include >> #include >> -#include >> +#include >> #include >> #include >> #include >> @@ -663,6 +663,27 @@ static bool rockchip_spi_can_dma(struct spi_master *master, >> return (xfer->len > rs->fifo_len); >> } >> >> +static int rockchip_spi_setup_cs_gpios(struct device *dev) >> +{ >> + struct device_node *np = dev->of_node; >> + struct gpio_desc *cs_gpio; >> + int i, nb; >> + >> + if (!np) >> + return 0; >> + >> + nb = of_gpio_named_count(np, "cs-gpios"); >> + for (i = 0; i < nb; i++) { >> + /* We support both GPIO CS and HW CS */ >> + cs_gpio = devm_gpiod_get_index_optional(dev, "cs", >> + i, GPIOD_ASIS); >> + if (IS_ERR(cs_gpio)) >> + return PTR_ERR(cs_gpio); > > I'm a bit confused why you need this function at all. You aren't using > the references that you're grabbing here, so essentially this is just > error-checking. actually this is error-checking plus gpiod_request(see gpiod_get_index in gpiolib.c) > > Are you doing anything here that isn't covered in > of_spi_register_master()? expect for gpiod_request, another difference would be when the of_spi_register_master calls of_get_named_gpio to parse cs-gpios, it would not do error handling here(fallback to HW CS): for (i = 0; i < nb; i++) cs[i] = of_get_named_gpio(np, "cs-gpios", i); but in our case, if something wrong happens(except for ENOENT), we cannot fallback to HW CS, because we already let pinctrl config GPIO CS. > >> + } >> + >> + return 0; >> +} >> + >> static int rockchip_spi_probe(struct platform_device *pdev) >> { >> int ret = 0; >> @@ -749,6 +770,7 @@ static int rockchip_spi_probe(struct platform_device *pdev) >> master->transfer_one = rockchip_spi_transfer_one; >> master->max_transfer_size = rockchip_spi_max_transfer_size; >> master->handle_err = rockchip_spi_handle_err; >> + master->flags = SPI_MASTER_GPIO_SS; > > I'm curious, do you actually need to assert both the HW and GPIO CS? yes, it would hang if we do spi xfer with wrong HW CS register config(seems to be another controller limit)... > > Brian > >> >> rs->dma_tx.ch = dma_request_chan(rs->dev, "tx"); >> if (IS_ERR(rs->dma_tx.ch)) { >> @@ -783,6 +805,12 @@ static int rockchip_spi_probe(struct platform_device *pdev) >> master->dma_rx = rs->dma_rx.ch; >> } >> >> + ret = rockchip_spi_setup_cs_gpios(&pdev->dev); >> + if (ret) { >> + dev_err(&pdev->dev, "Failed to setup cs gpios\n"); >> + goto err_free_dma_rx; >> + } >> + >> ret = devm_spi_register_master(&pdev->dev, master); >> if (ret) { >> dev_err(&pdev->dev, "Failed to register master\n"); >> -- >> 2.1.4 >> >> > > >