From mboxrd@z Thu Jan 1 00:00:00 1970 Return-path: Reply-To: Subject: Re: [RFC 4/8] gpio: altera-a10sr: Add A10 System Resource Chip GPIO support. References: <1459278791-3646-1-git-send-email-tthayer@opensource.altera.com> <1459278791-3646-5-git-send-email-tthayer@opensource.altera.com> To: Linus Walleij CC: Lee Jones , Alexandre Courbot , , Guenter Roeck , Rob Herring , , Mark Rutland , "ijc+devicetree@hellion.org.uk" , Dinh Nguyen , "linux-gpio@vger.kernel.org" , , "devicetree@vger.kernel.org" From: Thor Thayer Message-ID: <56FEDB62.1010308@opensource.altera.com> Date: Fri, 1 Apr 2016 15:34:42 -0500 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 7bit List-ID: Hi Linus, On 04/01/2016 07:17 AM, Linus Walleij wrote: > On Tue, Mar 29, 2016 at 9:13 PM, wrote: > >> From: Thor Thayer >> >> Add the GPIO functionality for the Altera Arria10 MAX5 System Resource >> Chip. The A10 MAX5 has 12 bits of GPIO assigned to switches, buttons, >> and LEDs as a GPIO extender on the SPI bus. >> >> Signed-off-by: Thor Thayer > > OK... > > As Lee says: split off the MFD patch so it is a pure GPIO driver > patch. > ACK >> +#include > > You should instead #include > >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include > > Syscalls and uaccess??? I don't think so. > OK. >> +struct altr_a10sr_gpio { >> + struct altr_a10sr *a10sc; >> + struct gpio_chip gp; >> +}; > > Add some kerneldoc. OK. To clarify, is this comment referring to the bindings document or something different? > >> +static inline struct altr_a10sr_gpio *to_altr_a10sr_gpio(struct gpio_chip *chip) >> +{ >> + return container_of(chip, struct altr_a10sr_gpio, gp); >> +} > > Don't use this old design pattern. > > Use [devm_]gpiochip_add_data() and use gpiochip_get_data(gc) to get > a data pointer from the gpiochip. > >> +static int altr_a10sr_gpio_get(struct gpio_chip *gc, unsigned int nr) >> +{ >> + struct altr_a10sr_gpio *gpio = to_altr_a10sr_gpio(gc); > > So this becomes > struct altr_a10sr_gpio *gpio = gpiochip_get_data(gc); > Got it. Thanks! >> + int ret; >> + unsigned char reg = ALTR_A10SR_LED_RD_REG + ALTR_A10SR_REG_OFFSET(nr); >> + >> + ret = altr_a10sr_reg_read(gpio->a10sc, reg); >> + >> + if (ret < 0) >> + return ret; >> + >> + if (ret & (1 << ALTR_A10SR_REG_BIT(nr))) >> + return 1; > > Do this instead: > > return !!(ret & (1 << ALTR_A10SR_REG_BIT(nr))) > > It raises the question whether ALTR_A10SR_REG_BIT > is just a reimplementation of the BIT() macro from > , please check this. > Got it. Yes, I will check that. Thanks. >> +static int altr_a10sr_gpio_direction_input(struct gpio_chip *gc, >> + unsigned int nr) >> +{ >> + if ((nr >= ALTR_A10SR_IN_VALID_RANGE_LO) && >> + (nr <= ALTR_A10SR_IN_VALID_RANGE_HI)) >> + return 0; >> + return -EINVAL; >> +} >> + >> +static int altr_a10sr_gpio_direction_output(struct gpio_chip *gc, >> + unsigned int nr, int value) >> +{ >> + if ((nr >= ALTR_A10SR_OUT_VALID_RANGE_LO) && >> + (nr <= ALTR_A10SR_OUT_VALID_RANGE_HI)) >> + return 0; >> + return -EINVAL; >> +} > > Does this mean that all lines are *always* input and output > at the same time? > > If there is no .set_direction() callback and all lines are both > input and output it kind of implies that all lines are also > implicitly open drain do you agree? > > Please check: > - If there is really no direction setting anywhere > - For example if some lines are hardwired as input and > some lines are hardwired as output > - If that is not the case, verify that all lines are really > open drain, they should be if all are both input and > output at the same time. > I see your point. I'll investigate how to do this properly for your 2nd check above. Registers are hard-wired as input or output so I'll need to handle this properly and is why I didn't implement the .set_direction callback. Thanks for the explanation. In my case, there are 12 valid GPIOs out of the 16 bits (the first 4 bits are unused). Bits 4-7 are output and bits 8-15 are inputs. I was using the IN_VALID range for the inputs and the OUT_VALID range for the outputs. >> + ret = gpiochip_add(&gpio->gp); >> + if (ret < 0) { >> + dev_err(&pdev->dev, "Could not register gpiochip, %d\n", ret); >> + return ret; >> + } > > Use devm_gpiochip_add_data() instead. > OK. Thanks for reviewing! > Yours, > Linus Walleij > From mboxrd@z Thu Jan 1 00:00:00 1970 From: Thor Thayer Subject: Re: [RFC 4/8] gpio: altera-a10sr: Add A10 System Resource Chip GPIO support. Date: Fri, 1 Apr 2016 15:34:42 -0500 Message-ID: <56FEDB62.1010308@opensource.altera.com> References: <1459278791-3646-1-git-send-email-tthayer@opensource.altera.com> <1459278791-3646-5-git-send-email-tthayer@opensource.altera.com> Reply-To: Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: Sender: linux-hwmon-owner@vger.kernel.org To: Linus Walleij Cc: Lee Jones , Alexandre Courbot , jdelvare@suse.com, Guenter Roeck , Rob Herring , pawell.moll@arm.com, Mark Rutland , "ijc+devicetree@hellion.org.uk" , Dinh Nguyen , "linux-gpio@vger.kernel.org" , linux-hwmon@vger.kernel.org, "devicetree@vger.kernel.org" List-Id: linux-gpio@vger.kernel.org Hi Linus, On 04/01/2016 07:17 AM, Linus Walleij wrote: > On Tue, Mar 29, 2016 at 9:13 PM, wrote: > >> From: Thor Thayer >> >> Add the GPIO functionality for the Altera Arria10 MAX5 System Resource >> Chip. The A10 MAX5 has 12 bits of GPIO assigned to switches, buttons, >> and LEDs as a GPIO extender on the SPI bus. >> >> Signed-off-by: Thor Thayer > > OK... > > As Lee says: split off the MFD patch so it is a pure GPIO driver > patch. > ACK >> +#include > > You should instead #include > >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include > > Syscalls and uaccess??? I don't think so. > OK. >> +struct altr_a10sr_gpio { >> + struct altr_a10sr *a10sc; >> + struct gpio_chip gp; >> +}; > > Add some kerneldoc. OK. To clarify, is this comment referring to the bindings document or something different? > >> +static inline struct altr_a10sr_gpio *to_altr_a10sr_gpio(struct gpio_chip *chip) >> +{ >> + return container_of(chip, struct altr_a10sr_gpio, gp); >> +} > > Don't use this old design pattern. > > Use [devm_]gpiochip_add_data() and use gpiochip_get_data(gc) to get > a data pointer from the gpiochip. > >> +static int altr_a10sr_gpio_get(struct gpio_chip *gc, unsigned int nr) >> +{ >> + struct altr_a10sr_gpio *gpio = to_altr_a10sr_gpio(gc); > > So this becomes > struct altr_a10sr_gpio *gpio = gpiochip_get_data(gc); > Got it. Thanks! >> + int ret; >> + unsigned char reg = ALTR_A10SR_LED_RD_REG + ALTR_A10SR_REG_OFFSET(nr); >> + >> + ret = altr_a10sr_reg_read(gpio->a10sc, reg); >> + >> + if (ret < 0) >> + return ret; >> + >> + if (ret & (1 << ALTR_A10SR_REG_BIT(nr))) >> + return 1; > > Do this instead: > > return !!(ret & (1 << ALTR_A10SR_REG_BIT(nr))) > > It raises the question whether ALTR_A10SR_REG_BIT > is just a reimplementation of the BIT() macro from > , please check this. > Got it. Yes, I will check that. Thanks. >> +static int altr_a10sr_gpio_direction_input(struct gpio_chip *gc, >> + unsigned int nr) >> +{ >> + if ((nr >= ALTR_A10SR_IN_VALID_RANGE_LO) && >> + (nr <= ALTR_A10SR_IN_VALID_RANGE_HI)) >> + return 0; >> + return -EINVAL; >> +} >> + >> +static int altr_a10sr_gpio_direction_output(struct gpio_chip *gc, >> + unsigned int nr, int value) >> +{ >> + if ((nr >= ALTR_A10SR_OUT_VALID_RANGE_LO) && >> + (nr <= ALTR_A10SR_OUT_VALID_RANGE_HI)) >> + return 0; >> + return -EINVAL; >> +} > > Does this mean that all lines are *always* input and output > at the same time? > > If there is no .set_direction() callback and all lines are both > input and output it kind of implies that all lines are also > implicitly open drain do you agree? > > Please check: > - If there is really no direction setting anywhere > - For example if some lines are hardwired as input and > some lines are hardwired as output > - If that is not the case, verify that all lines are really > open drain, they should be if all are both input and > output at the same time. > I see your point. I'll investigate how to do this properly for your 2nd check above. Registers are hard-wired as input or output so I'll need to handle this properly and is why I didn't implement the .set_direction callback. Thanks for the explanation. In my case, there are 12 valid GPIOs out of the 16 bits (the first 4 bits are unused). Bits 4-7 are output and bits 8-15 are inputs. I was using the IN_VALID range for the inputs and the OUT_VALID range for the outputs. >> + ret = gpiochip_add(&gpio->gp); >> + if (ret < 0) { >> + dev_err(&pdev->dev, "Could not register gpiochip, %d\n", ret); >> + return ret; >> + } > > Use devm_gpiochip_add_data() instead. > OK. Thanks for reviewing! > Yours, > Linus Walleij >