Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[RFC]: extend gpio and i2c #22

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

[RFC]: extend gpio and i2c #22

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0df379c
gpio-ch341.c: enable all output GPIOs
mr-nice Nov 17, 2023
1da27a9
gpio-ch341.c: check if i2c is enabled
mr-nice Nov 18, 2023
fccaba8
i2c-ch341.c: request and release the I2C GPIOs.
mr-nice Nov 18, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
80 changes: 67 additions & 13 deletions gpio-ch341.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ static const struct mfd_cell ch341_gpio_devs[] = {
{ .name = "ch341-spi", },
};

#define CH341_GPIO_NUM_PINS 16 /* Number of GPIO pins */
#define CH341_GPIO_NUM_PINS 20 /* Number of GPIO pins */

/* GPIO chip commands */
#define CH341_PARA_CMD_STS 0xA0 /* Get pins status */
Expand All @@ -42,9 +42,10 @@ static const struct mfd_cell ch341_gpio_devs[] = {
struct ch341_gpio {
struct gpio_chip gpio;
struct mutex gpio_lock;
u16 gpio_dir; /* 1 bit per pin, 0=IN, 1=OUT. */
u32 gpio_dir; /* 1 bit per pin, 0=IN, 1=OUT. */
u16 gpio_last_read; /* last GPIO values read */
u16 gpio_last_written; /* last GPIO values written */
u32 gpio_last_written; /* last GPIO values written */
bool i2c_active;
union {
u8 *gpio_buf;
__le16 *gpio_buf_status;
Expand All @@ -58,10 +59,13 @@ struct ch341_gpio {
};

/*
* Masks to describe the 16 GPIOs. Pins D0 to D5 (mapped to GPIOs 0 to
* 5) can do input/output, but the other pins are input-only.
* Masks to describe the 20 (0-19) GPIOs.
* All pins exept pin 12 can output.
* Pins 16 to 20 can only be used as output.
* All other pins can be also used as inputs.
*/
static const u16 pin_can_output = 0b111111;
static const u32 pin_can_output = 0b11111110111111111111;
static const u32 pin_can_input = 0b1111111111111111;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add some leading zeros to align the numbers so we can better see?


/* Only GPIO 10 (INT# line) has hardware interrupt */
#define CH341_GPIO_INT_LINE 10
Expand Down Expand Up @@ -190,16 +194,39 @@ static int ch341_gpio_get_multiple(struct gpio_chip *chip,

static void write_outputs(struct ch341_gpio *dev)
{
/*
* Direction and data enable.
* Bit 0: if set to 1, data out is enabled for bit 8 to 15
* Bit 1: if set to 1, direction is enabled for bit 8 to 15
* Bit 2: if set to 1, set data out is enabled for bit 0 to 7
* Bit 3: if set to 1, set direction is enabled for bit 0 to 7
* Bit 4: if set to 1, set data out is enabled for bit 16-19
*
* NOTE: there is no way to set the direction for bit 16 to 19
* if bit 4 is enabled the outputs are allways written.
* As 18 and 19 are used by the I2C driver we disable
* the data out for the bits 16 to 19. Otherwise
* writing to 16 or 17 will toggle 18 and 19 as well.
*/
u8 gpio_enable = 0x1f;
if(dev->i2c_active)
gpio_enable = 0x0f;

mutex_lock(&dev->gpio_lock);

dev->gpio_buf[0] = CH341_CMD_SET_OUTPUT;
dev->gpio_buf[1] = 0x6a;
dev->gpio_buf[2] = 0x0c; // Keep the interface and only change the output pins D0-D5
dev->gpio_buf[3] = 0; // This affects the bits from 8 to 15 and we want to keep the interface therefore set to anything
dev->gpio_buf[4] = 0; // This affects the bits from 8 to 15 and we want to keep the interface therefore set to anything
dev->gpio_buf[5] = dev->gpio_last_written & dev->gpio_dir & pin_can_output & 0xff; // D5|D4|D3|D2|D1|D0 set to 1 -> output set to 0 -> input
dev->gpio_buf[6] = dev->gpio_dir & pin_can_output & 0xff; // D7|D6|D5|D4|D3|D2|D1|D0 set to 1 -> high active, set to 0 -> low active
dev->gpio_buf[7] = 0; // This affects the bits from 16-20 and we want to keep the interface, set to 1 -> output set to 0 input
dev->gpio_buf[2] = gpio_enable;
/* Set output Pins from 8 to 15 */
dev->gpio_buf[3] = (dev->gpio_last_written >> 8) & (dev->gpio_dir >> 8);
/* Set GPIO direction for pins from 8 to 15. Input = 0, output = 1 */
dev->gpio_buf[4] = dev->gpio_dir >> 8;
/* Set output Pins from 0 to 7 */
dev->gpio_buf[5] = dev->gpio_last_written & dev->gpio_dir;
/* Set GPIO direction for pins from 0 to 7. Input = 0, output = 1 */
dev->gpio_buf[6] = dev->gpio_dir;
/* Set output Pins from 16 to 19 */
dev->gpio_buf[7] = (dev->gpio_last_written & dev->gpio_dir) >> 16;
dev->gpio_buf[8] = 0;
dev->gpio_buf[9] = 0;
dev->gpio_buf[10] = 0;
Expand All @@ -209,6 +236,23 @@ static void write_outputs(struct ch341_gpio *dev)
mutex_unlock(&dev->gpio_lock);
}

static bool i2c_active(struct gpio_chip *chip)
{
/*
* Use gpiochip_is_requestet as suggested in the manual
* to find out if the I2C driver requested the I2C GPIOs.
* https://www.kernel.org/doc/html/latest/driver-api/gpio/index.html
* This function is for use by GPIO controller drivers.
* The label can help with diagnostics, and knowing that the signal is
* used as a GPIO can help avoid accidentally multiplexing
* it to another controller.
*/
const char * scl = gpiochip_is_requested(chip, 18);
if(!scl)
return false;
return !strcmp("SCL", scl);
}

static void ch341_gpio_set(struct gpio_chip *chip, unsigned int offset, int value)
{
struct ch341_gpio *dev = gpiochip_get_data(chip);
Expand Down Expand Up @@ -243,6 +287,10 @@ static int ch341_gpio_direction_input(struct gpio_chip *chip,
unsigned int offset)
{
struct ch341_gpio *dev = gpiochip_get_data(chip);
u32 mask = BIT(offset);

if (!(pin_can_input & mask))
return -EINVAL;

dev->gpio_dir &= ~BIT(offset);

Expand All @@ -255,13 +303,14 @@ static int ch341_gpio_direction_output(struct gpio_chip *chip,
unsigned int offset, int value)
{
struct ch341_gpio *dev = gpiochip_get_data(chip);
u16 mask = BIT(offset);
u32 mask = BIT(offset);

if (!(pin_can_output & mask))
return -EINVAL;

dev->gpio_dir |= mask;

dev->i2c_active = i2c_active(chip);
ch341_gpio_set(chip, offset, value);

return 0;
Expand Down Expand Up @@ -361,6 +410,11 @@ static int ch341_gpio_probe(struct platform_device *pdev)
if (dev->irq_buf == NULL)
return -ENOMEM;

/* Pins 16-19 can only be used as output */
dev->gpio_dir = 0xf0000;

dev->i2c_active = false;

platform_set_drvdata(pdev, dev);
dev->ddata = ddata;
mutex_init(&dev->gpio_lock);
Expand Down
90 changes: 90 additions & 0 deletions i2c-ch341.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,15 @@
* I2C cell driver for the CH341A, CH341B and CH341T.
*
* Copyright 2022, Frank Zago
* Copyright (c) 2023 Bernhard Guillon (Bernhard.Guillon@begu.org)
* Copyright (c) 2016 Tse Lun Bien
* Copyright (c) 2014 Marco Gittler
* Copyright (C) 2006-2007 Till Harbaum (Till@Harbaum.org)
*/

#include <linux/gpio/driver.h>
#include <linux/gpio/consumer.h>
#include <linux/gpio/machine.h>
#include <linux/i2c.h>
#include <linux/kernel.h>
#include "ch341.h"
Expand Down Expand Up @@ -36,9 +40,29 @@
/* Limit the transfer read size in one go to 32 bytes. */
#define MAX_RD_LENGTH 32

/*
* Pin configuration. The I2C interfaces use 2 pins
* allocated to GPIOs. But we need to grab 4 pins
* to make sure that we don't mix signals.
*/
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe the gpio driver should use the new method if i2c isn't loaded/used and use the previous method if it is.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I made a missleading comment here. We can still use the new method as it will still have the advantage to have more pins availabe without any interfering. The only problem are the last 4 GPIOs.
Do you prefer a check that the driver is not loaded over the is the line used in the gpio code? This would have the advantage that we might not need to change the i2c driver at all. But the disadvantage that the amount of exported GPIOs differs. And what should happen if one of the GPIO lines is already used? I'm not sure what's easier to understand for the user.
But it might be a good first solution to get this PR done.

struct i2c_gpio {
unsigned int hwnum;
const char *label;
enum gpiod_flags dflags;
};

struct ch341_i2c {
struct i2c_adapter adapter;
u8 *i2c_buf;
struct gpio_desc *i2c_gpio_desc[4];
struct gpio_chip *gpiochip;
};

static const struct i2c_gpio i2c_gpios[] = {
{ 16, "reserved", GPIOD_OUT_HIGH },
{ 17, "reserved", GPIOD_OUT_HIGH },
{ 18, "SCL", GPIOD_OUT_HIGH },
{ 19, "SDA", GPIOD_OUT_HIGH },
};

/*
Expand Down Expand Up @@ -257,6 +281,50 @@ static void devm_i2c_del_adapter(void *adapter)
}
#endif

static void release_gpios(struct ch341_i2c *dev)
{
int i;
for (i = 0; i < ARRAY_SIZE(dev->i2c_gpio_desc); i++) {
gpiochip_free_own_desc(dev->i2c_gpio_desc[i]);
dev->i2c_gpio_desc[i] = NULL;
}
}

static int request_gpios(struct platform_device *pdev, struct ch341_i2c *dev)
{
int i;
for (i = 0; i < ARRAY_SIZE(i2c_gpios); i++) {
struct gpio_desc *desc;
const struct i2c_gpio *gpio = &i2c_gpios[i];
desc = gpiochip_request_own_desc(dev->gpiochip,
gpio->hwnum,
gpio->label,
GPIO_LOOKUP_FLAGS_DEFAULT,
gpio->dflags);
if (IS_ERR(desc)) {
dev_warn(&pdev->dev,
"Unable to reserve GPIO %s for I2C\n",
gpio->label);
release_gpios(dev);
return PTR_ERR(desc);
}
dev->i2c_gpio_desc[i] = desc;
}
return 0;
}

static int match_gpiochip_parent(struct gpio_chip *gc, void *data)
{
/*
* FIMXE: as this is a child of a mfd driver just return
* 1 for now :/
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should do like the spi driver; be child of gpio.

*/

printk("gc->parent %p| data %p|\n", gc->parent, data);
// return gc->parent == data;
return 1;
}

static int ch341_i2c_probe(struct platform_device *pdev)
{
struct ch341_ddata *ddata = dev_get_drvdata(pdev->dev.parent);
Expand Down Expand Up @@ -285,6 +353,20 @@ static int ch341_i2c_probe(struct platform_device *pdev)
i2c_set_adapdata(&ch341_i2c->adapter, ch341_i2c);
platform_set_drvdata(pdev, ch341_i2c);

/*
* FIMXE as this is a child of a mfd driver we cannot use pdev->dev.parent
* unfortunate pdev->mfd_cell->platform_data is a null pointer
* so for now just pass the wrong dev.parent here.
*/
/* Find the parent's gpiochip */
ch341_i2c->gpiochip = gpiochip_find(pdev->dev.parent, match_gpiochip_parent);
if (!ch341_i2c->gpiochip) {
dev_err(&pdev->dev, "Parent GPIO chip not found!\n");
return -ENODEV;
}

request_gpios(pdev, ch341_i2c);

/* Set ch341 i2c speed */
ch341_i2c->i2c_buf[0] = CH341_CMD_I2C_STREAM;
ch341_i2c->i2c_buf[1] = CH341_CMD_I2C_STM_SET | CH341_I2C_100KHZ;
Expand All @@ -311,9 +393,17 @@ static int ch341_i2c_probe(struct platform_device *pdev)
#endif
}

static int ch341_i2c_remove(struct platform_device *pdev)
{
struct ch341_i2c *ch341_i2c = platform_get_drvdata(pdev);
release_gpios(ch341_i2c);
return 0;
}

static struct platform_driver ch341_i2c_driver = {
.driver.name = "ch341-i2c",
.probe = ch341_i2c_probe,
.remove = ch341_i2c_remove,
};
module_platform_driver(ch341_i2c_driver);

Expand Down