Messages in this thread | | | From | Linus Walleij <> | Date | Mon, 10 Apr 2017 22:04:04 +0200 | Subject | Re: [PATCH v4 2/2] i2c: mux: ltc4306: LTC4306 and LTC4305 I2C multiplexer/switch |
| |
On Wed, Apr 5, 2017 at 3:07 PM, <michael.hennerich@analog.com> wrote:
> From: Michael Hennerich <michael.hennerich@analog.com> > > This patch adds support for the Analog Devices / Linear Technology > LTC4306 and LTC4305 4/2 Channel I2C Bus Multiplexer/Switches. > The LTC4306 optionally provides two general purpose input/output pins > (GPIOs) that can be configured as logic inputs, opendrain outputs or > push-pull outputs via the generic GPIOLIB framework. > > Signed-off-by: Michael Hennerich <michael.hennerich@analog.com>
Okay!
> +#include <linux/device.h> > +#include <linux/gpio.h> > +#include <linux/gpio/consumer.h> > +#include <linux/gpio/driver.h>
Why are you including all these? Normally a GPIO driver should just include <linux/gpio/driver.h>
> +#include <linux/i2c-mux.h> > +#include <linux/i2c.h> > +#include <linux/module.h> > +#include <linux/of.h> > +#include <linux/of_device.h> > +#include <linux/regmap.h> > +#include <linux/slab.h>
> +static int ltc4306_gpio_get(struct gpio_chip *chip, unsigned int offset) > +{ > + struct ltc4306 *data = gpiochip_get_data(chip); > + unsigned int val; > + int ret; > + > + ret = regmap_read(data->regmap, LTC_REG_CONFIG, &val); > + if (ret < 0) > + return ret; > + > + return (val & BIT(1 - offset));
Do this:
return !!(val & BIT(1 - offset));
So you clamp the return value to [0,1]
> +static int ltc4306_gpio_set_config(struct gpio_chip *chip, > + unsigned int offset, unsigned long config) > +{ > + struct ltc4306 *data = gpiochip_get_data(chip); > + unsigned int val; > + > + switch (pinconf_to_config_param(config)) { > + case PIN_CONFIG_DRIVE_OPEN_DRAIN: > + val = 0; > + break; > + case PIN_CONFIG_DRIVE_PUSH_PULL: > + val = BIT(4 - offset); > + break; > + default: > + return -ENOTSUPP; > + } > + > + return regmap_update_bits(data->regmap, LTC_REG_MODE, > + BIT(4 - offset), val); > +}
Nice!
> + data->gpiochip.label = dev_name(dev); > + data->gpiochip.base = -1; > + data->gpiochip.ngpio = data->chip->num_gpios; > + data->gpiochip.parent = dev; > + data->gpiochip.can_sleep = true; > + data->gpiochip.direction_input = ltc4306_gpio_direction_input; > + data->gpiochip.direction_output = ltc4306_gpio_direction_output; > + data->gpiochip.get = ltc4306_gpio_get; > + data->gpiochip.set = ltc4306_gpio_set; > + data->gpiochip.set_config = ltc4306_gpio_set_config; > + data->gpiochip.owner = THIS_MODULE;
Please implement .get_direction(). This is very helpful to userspace, have you tested to use tools/gpio/* from the kernel? Like lsgpio?
Yours, Linus Walleij
| |