Re: [PATCH v3 2/2] pinctrl: Add Intel Thunder Bay pinctrl driver

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

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