lkml.org 
[lkml]   [2021]   [Dec]   [17]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [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


    \
     
     \ /
      Last update: 2021-12-17 12:23    [W:2.920 / U:0.000 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site