| Message ID | 20180328174703.19778-1-manivannan.sadhasivam@linaro.org |
|---|---|
| Headers | show |
| Series | Add Actions Semi S900 pinctrl and gpio support | expand |
On Wed, Mar 28, 2018 at 8:47 PM, Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> wrote: > Add gpio driver for Actions Semi OWL family S900 SoC. Set of registers > controlling the gpio shares the same register range with pinctrl block. > > GPIO registers are organized as 6 banks and each bank controls the > maximum of 32 gpios. > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com> > +static void owl_gpio_update_reg(void __iomem *base, unsigned int pin, int flag) > +{ > + u32 val; > + > + val = readl_relaxed(base); > + > + if (flag) > + val |= BIT(pin); > + else > + val &= ~BIT(pin); > + > + writel_relaxed(val, base); > +} Hmm... Just realized that this driver misses locking. Something like owl_gpio_update_reg_locked() { spin_lock(); owl_gpio_update_reg(); spin_unlock(); } > + > +static int owl_gpio_request(struct gpio_chip *chip, unsigned int offset) > +{ > + struct owl_gpio *gpio = gpiochip_get_data(chip); > + > + /* > + * GPIOs have higher priority over other modules, so either setting > + * them as OUT or IN is sufficient > + */ > + owl_gpio_update_reg(gpio->base + GPIO_OUTEN, offset, true); ...to use in such cases. > + > + return 0; > +} > + > +static void owl_gpio_free(struct gpio_chip *chip, unsigned int offset) > +{ > + struct owl_gpio *gpio = gpiochip_get_data(chip); > + > + /* disable gpio output */ > + owl_gpio_update_reg(gpio->base + GPIO_OUTEN, offset, false); > + > + /* disable gpio input */ > + owl_gpio_update_reg(gpio->base + GPIO_INEN, offset, false); ...or spin_lock(); owl_gpio_update_reg(); owl_gpio_update_reg(); spin_unlock(); ...in this and similar cases. > +}
On Wed, Mar 28, 2018 at 8:46 PM, Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> wrote: > Add pinctrl driver for Actions Semi S900 SoC. The driver supports > pinctrl, pinmux and pinconf functionalities through a range of registers > common to both gpio driver and pinctrl driver. > > Pinmux functionality is available only for the pin groups while the > pinconf functionality is available for both pin groups and individual > pins. > +static void owl_update_bits(void __iomem *base, u32 mask, u32 val) > +{ > + u32 reg_val; > + > + reg_val = readl_relaxed(base); > + > + reg_val &= ~mask; > + reg_val |= val; Usual pattern here is reg_val = (reg_val & ~mask) | (val & mask); This will allow to avoid possible overflow. > + > + writel_relaxed(reg_val, base); > +} > + tmp = readl_relaxed(pctrl->base + reg); > + mask = (1 << width) - 1; > + arg = (tmp >> bit) & mask; This looks like a candidate for a helper function. You have at least one more same code. Something like ..._read_field(reg, mask, shift) > + mask = (1 << width) - 1; > + mask = mask << bit; > + > + owl_update_bits(pctrl->base + reg, mask, (arg << bit)); Similar here, ..._write_field(regm mask, shift, arg) > + tmp = readl_relaxed(pctrl->base + reg); > + mask = (1 << width) - 1; > + arg = (tmp >> bit) & mask; > + mask = (1 << width) - 1; > + mask = mask << bit; > + > + owl_update_bits(pctrl->base + reg, mask, (arg << bit)); > +static const struct pinconf_ops owl_pinconf_ops = { > + .is_generic = true, > + .pin_config_get = owl_pin_config_get, > + .pin_config_set = owl_pin_config_set, > + .pin_config_group_get = owl_group_config_get, > + .pin_config_group_set = owl_group_config_set It's still good idea to leave comma here... > +}; > + > +static struct pinctrl_desc owl_pinctrl_desc = { > + .pctlops = &owl_pinctrl_ops, > + .pmxops = &owl_pinmux_ops, > + .confops = &owl_pinconf_ops, > + .owner = THIS_MODULE ...and here, and in all similar places. > +}; > +
Hi Andy, On Sat, Mar 31, 2018 at 12:16:49AM +0300, Andy Shevchenko wrote: > On Wed, Mar 28, 2018 at 8:46 PM, Manivannan Sadhasivam > <manivannan.sadhasivam@linaro.org> wrote: > > Add pinctrl driver for Actions Semi S900 SoC. The driver supports > > pinctrl, pinmux and pinconf functionalities through a range of registers > > common to both gpio driver and pinctrl driver. > > > > Pinmux functionality is available only for the pin groups while the > > pinconf functionality is available for both pin groups and individual > > pins. > > > +static void owl_update_bits(void __iomem *base, u32 mask, u32 val) > > +{ > > + u32 reg_val; > > + > > + reg_val = readl_relaxed(base); > > + > > + reg_val &= ~mask; > > + reg_val |= val; > > Usual pattern here is > > reg_val = (reg_val & ~mask) | (val & mask); > > This will allow to avoid possible overflow. > Ack. > > + > > + writel_relaxed(reg_val, base); > > +} > > > + tmp = readl_relaxed(pctrl->base + reg); > > + mask = (1 << width) - 1; > > + arg = (tmp >> bit) & mask; > > This looks like a candidate for a helper function. You have at least > one more same code. > > Something like > > ..._read_field(reg, mask, shift) > > Okay. Will add owl_read_field helper function. > > + mask = (1 << width) - 1; > > + mask = mask << bit; > > + > > + owl_update_bits(pctrl->base + reg, mask, (arg << bit)); > > Similar here, > > ..._write_field(regm mask, shift, arg) > Will add owl_write_field helper function. > > + tmp = readl_relaxed(pctrl->base + reg); > > + mask = (1 << width) - 1; > > + arg = (tmp >> bit) & mask; > > > + mask = (1 << width) - 1; > > + mask = mask << bit; > > + > > + owl_update_bits(pctrl->base + reg, mask, (arg << bit)); > > > +static const struct pinconf_ops owl_pinconf_ops = { > > + .is_generic = true, > > + .pin_config_get = owl_pin_config_get, > > + .pin_config_set = owl_pin_config_set, > > + .pin_config_group_get = owl_group_config_get, > > + .pin_config_group_set = owl_group_config_set > > It's still good idea to leave comma here... > I'm confused. What is the criteria for removing/keeping comma for last member of struct? I followed your gpio driver suggestion. Thanks, Mani > > +}; > > + > > +static struct pinctrl_desc owl_pinctrl_desc = { > > + .pctlops = &owl_pinctrl_ops, > > + .pmxops = &owl_pinmux_ops, > > + .confops = &owl_pinconf_ops, > > + .owner = THIS_MODULE > > ...and here, and in all similar places. > > > +}; > > + > > -- > With Best Regards, > Andy Shevchenko
Hi Mani, Am 03.04.2018 um 19:00 schrieb Manivannan Sadhasivam: > On Sat, Mar 31, 2018 at 12:16:49AM +0300, Andy Shevchenko wrote: >> On Wed, Mar 28, 2018 at 8:46 PM, Manivannan Sadhasivam >> <manivannan.sadhasivam@linaro.org> wrote: >>> +static const struct pinconf_ops owl_pinconf_ops = { >>> + .is_generic = true, >>> + .pin_config_get = owl_pin_config_get, >>> + .pin_config_set = owl_pin_config_set, >>> + .pin_config_group_get = owl_group_config_get, >>> + .pin_config_group_set = owl_group_config_set >> >> It's still good idea to leave comma here... >> > > I'm confused. What is the criteria for removing/keeping comma for last member > of struct? I followed your gpio driver suggestion. No comma for list terminator ("{}") because nothing goes after it; comma whenever it allows adding a new line without touching the old one, i.e. keeping future diff small. Cheers, Andreas
On Tue, Apr 03, 2018 at 07:03:46PM +0200, Andreas Färber wrote: > Hi Mani, > > Am 03.04.2018 um 19:00 schrieb Manivannan Sadhasivam: > > On Sat, Mar 31, 2018 at 12:16:49AM +0300, Andy Shevchenko wrote: > >> On Wed, Mar 28, 2018 at 8:46 PM, Manivannan Sadhasivam > >> <manivannan.sadhasivam@linaro.org> wrote: > >>> +static const struct pinconf_ops owl_pinconf_ops = { > >>> + .is_generic = true, > >>> + .pin_config_get = owl_pin_config_get, > >>> + .pin_config_set = owl_pin_config_set, > >>> + .pin_config_group_get = owl_group_config_get, > >>> + .pin_config_group_set = owl_group_config_set > >> > >> It's still good idea to leave comma here... > >> > > > > I'm confused. What is the criteria for removing/keeping comma for last member > > of struct? I followed your gpio driver suggestion. > > No comma for list terminator ("{}") because nothing goes after it; comma > whenever it allows adding a new line without touching the old one, i.e. > keeping future diff small. > Thanks for the clarification Andreas! Keeping the future diff small is the key, understood. Thanks, Mani > Cheers, > Andreas > > -- > SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany > GF: Felix Imendörffer, Jane Smithard, Graham Norton > HRB 21284 (AG Nürnberg)
On Tue, Apr 3, 2018 at 8:00 PM, Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> wrote: >> > +static const struct pinconf_ops owl_pinconf_ops = { >> > + .is_generic = true, >> > + .pin_config_get = owl_pin_config_get, >> > + .pin_config_set = owl_pin_config_set, >> > + .pin_config_group_get = owl_group_config_get, >> > + .pin_config_group_set = owl_group_config_set >> >> It's still good idea to leave comma here... > I'm confused. What is the criteria for removing/keeping comma for last member > of struct? I followed your gpio driver suggestion. Just a common sense and experience are talking here: from time to time some structures are being expanded and in some cases it requires to update users. The comma just reduces a possible burden on this expansion. This is common pattern used in kernel. >> > +}; >> > + >> > +static struct pinctrl_desc owl_pinctrl_desc = { >> > + .pctlops = &owl_pinctrl_ops, >> > + .pmxops = &owl_pinmux_ops, >> > + .confops = &owl_pinconf_ops, >> > + .owner = THIS_MODULE >> >> ...and here, and in all similar places. >> >> > +};
On Wed, Mar 28, 2018 at 7:46 PM, Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> wrote: > Add gpio nodes for Actions Semi S900 SoC. > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> This should probably have Subject "add bindings" rather than "add gpio nodes" but it's fine, I can fix it up when applying if I just get Rob's ACK on these bindings (that look entirely uncontroversial). Yours, Linus Walleij