On Thu, Dec 16, 2021 at 08:31:00PM +0530, lakshmi.sowjanya.d@intel.com wrote:

From: Lakshmi Sowjanya D <redacted>

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