Messages in this thread | | | Date | Fri, 17 Dec 2021 13:21:41 +0200 | From | Andy Shevchenko <> | Subject | Re: [PATCH v3 2/2] pinctrl: Add Intel Thunder Bay pinctrl driver |
| |
On Thu, Dec 16, 2021 at 08:31:00PM +0530, lakshmi.sowjanya.d@intel.com wrote: > From: Lakshmi Sowjanya D <lakshmi.sowjanya.d@intel.com> > > About Intel Thunder Bay: > ----------------------- > Intel Thunder Bay is a computer vision AI accelerator SoC based on ARM CPU. > > Pinctrl IP: > ---------- > The SoC has a customised pinmux controller IP which controls pin > multiplexing and configuration. > > Thunder Bay pinctrl IP is not based on and have nothing in common with the > existing pinctrl drivers. The registers used are incompatible with the > existing drivers, so it requires a new driver. > > Add pinctrl driver to enable pin control support in the Intel Thunder Bay > SoC.
...
+ bits.h.
> +#include <linux/device.h> > +#include <linux/err.h> > +#include <linux/gpio/driver.h> > +#include <linux/init.h> > +#include <linux/interrupt.h> > +#include <linux/io.h> > +#include <linux/irq.h> > +#include <linux/module.h> > +#include <linux/of.h> > +#include <linux/of_irq.h>
> +#include <linux/pinctrl/pinconf.h> > +#include <linux/pinctrl/pinconf-generic.h> > +#include <linux/pinctrl/pinctrl.h> > +#include <linux/pinctrl/pinmux.h>
Can you move this...
> +#include <linux/platform_device.h> > +#include <linux/slab.h> > +#include <linux/spinlock.h>
...here?
> +#include "core.h" > +#include "pinconf.h" > +#include "pinctrl-utils.h" > +#include "pinmux.h"
...
> +#define THB_GPIO_REG_OFFSET(pin_num) ((pin_num) * (0x4))
'(0x4)' --> '4'
...
> +static int thb_write_gpio_data(struct gpio_chip *chip, unsigned int offset, unsigned int value) > +{ > + int data_offset; > + u32 data_reg; > + > + data_offset = 0x2000u + (offset / 32); > + > + data_reg = thb_gpio_read_reg(chip, data_offset); > + > + if (value > 0)
if (value)
> + data_reg |= BIT(offset % 32); > + else > + data_reg &= ~BIT(offset % 32); > + > + return thb_gpio_write_reg(chip, data_offset, data_reg); > +}
...
> +static int thunderbay_gpio_set_direction_input(struct gpio_chip *chip, unsigned int offset) > +{ > + u32 reg = thb_gpio_read_reg(chip, offset); > + > + /* set pin as input only if it is GPIO else error */ > + if (reg & THB_GPIO_PORT_SELECT_MASK) {
Can it be
reg = thb_gpio_read_reg(chip, offset); if (!(reg & THB_GPIO_PORT_SELECT_MASK)) return -EINVAL;
?
> + reg &= (~THB_GPIO_PAD_DIRECTION_MASK);
Too many parentheses.
> + thb_gpio_write_reg(chip, offset, reg); > + return 0; > + } > + return -EINVAL; > +}
...
> +static int thunderbay_gpio_set_direction_output(struct gpio_chip *chip, > + unsigned int offset, int value) > +{ > + u32 reg = thb_gpio_read_reg(chip, offset); > + > + /* set pin as output only if it is GPIO else error */ > + if (reg & THB_GPIO_PORT_SELECT_MASK) {
As per above.
> + reg |= THB_GPIO_PAD_DIRECTION_MASK; > + thb_gpio_write_reg(chip, offset, reg); > + thunderbay_gpio_set_value(chip, offset, value); > + return 0; > + } > + return -EINVAL; > +}
...
> +static int thunderbay_gpio_get_value(struct gpio_chip *chip, unsigned int offset) > +{ > + u32 reg = thb_gpio_read_reg(chip, offset);
> + int gpio_dir = 0;
Useless assignment.
> + /* Read pin value only if it is GPIO else error */ > + if (reg & THB_GPIO_PORT_SELECT_MASK) { > + /* 0=in, 1=out */ > + gpio_dir = (reg & THB_GPIO_PAD_DIRECTION_MASK) > 0;
!!(reg & ...)
> + /* Returns negative value when pin is configured as PORT */ > + return thb_read_gpio_data(chip, offset, gpio_dir); > + } > + return -EINVAL;
And as per above.
> +}
...
> + /* identifies the first GPIO number handled by this chip; or, > + * if negative during registration, requests dynamic ID allocation. > + * Please pass -1 as base to let gpiolib select the chip base in all possible cases. > + * We want to get rid of the static GPIO number space in the long run. > + */
/* * Please, fix the style of the * multi-line comments. Pay attention * to the grammar, etc. Everywhere. */
...
> + /* Number of GPIOs handled by this controller; the last GPIO handled is (base + ngpio - 1)*/
Too long comment with missed white space.
...
> + /* Register/add Thunder Bay GPIO chip with Linux framework */ > + ret = gpiochip_add_data(chip, tpc);
Why not devm_*()?
> + if (ret) > + dev_err(tpc->dev, "Failed to add gpiochip\n");
> + return ret;
return 0;
But overall, use dev_err_probe().
I stopped here, since there are too many same comments over all functions in this driver.
...
> + { > + .compatible = "intel,thunderbay-pinctrl", > + .data = &thunderbay_data
+ Comma.
> + },
...
> + of_id = of_match_node(thunderbay_pinctrl_match, pdev->dev.of_node);
You already have dev, use it everywhere in the ->probe().
> + if (!of_id) > + return -ENODEV;
Use of_device_get_match_data() (or how is it called?).
...
> + iomem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + if (!iomem) > + return -ENXIO;
Redundant, but see below.
> + > + tpc->base0 = devm_ioremap_resource(dev, iomem); > + if (IS_ERR(tpc->base0)) > + return PTR_ERR(tpc->base0);
I dunno if you read any of previous comments regarding to other drivers. The above is just one API call. Find it and use.
...
> +static int thunderbay_pinctrl_remove(struct platform_device *pdev) > +{ > + /* thunderbay_pinctrl_remove function to clear the assigned memory */ > + return 0; > +}
Why do you need this stub? What for?
...
> +
Redundant blank line.
> +builtin_platform_driver(thunderbay_pinctrl_driver);
-- With Best Regards, Andy Shevchenko
| |