| Message ID | 20220129230043.12422-5-samuel@sholland.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Pine64 PinePhone keyboard support | expand |
Hello Samuel, On Sat, Jan 29, 2022 at 05:00:41PM -0600, Samuel Holland wrote: > The PinePhone keyboard case contains a battery managed by an integrated > power bank IC. The power bank IC communicates over I2C, and the keyboard > MCU firmware provides an interface to read and write its registers. > Let's use this interface to implement a SMBus adapter, so we can reuse > the driver for the power bank IC. > > Signed-off-by: Samuel Holland <samuel@sholland.org> > --- > > drivers/input/keyboard/pinephone-keyboard.c | 73 +++++++++++++++++++++ > 1 file changed, 73 insertions(+) > > diff --git a/drivers/input/keyboard/pinephone-keyboard.c b/drivers/input/keyboard/pinephone-keyboard.c > index 8065bc3e101a..7d2e16e588a0 100644 > --- a/drivers/input/keyboard/pinephone-keyboard.c > +++ b/drivers/input/keyboard/pinephone-keyboard.c > @@ -3,6 +3,7 @@ > // Copyright (C) 2021-2022 Samuel Holland <samuel@sholland.org> > > #include <linux/crc8.h> > +#include <linux/delay.h> > #include <linux/i2c.h> > #include <linux/input/matrix_keypad.h> > #include <linux/interrupt.h> > @@ -23,6 +24,11 @@ > #define PPKB_SCAN_DATA 0x08 > #define PPKB_SYS_CONFIG 0x20 > #define PPKB_SYS_CONFIG_DISABLE_SCAN BIT(0) > +#define PPKB_SYS_SMBUS_COMMAND 0x21 > +#define PPKB_SYS_SMBUS_DATA 0x22 > +#define PPKB_SYS_COMMAND 0x23 > +#define PPKB_SYS_COMMAND_SMBUS_READ 0x91 > +#define PPKB_SYS_COMMAND_SMBUS_WRITE 0xa1 > > #define PPKB_DEFAULT_KEYMAP_ROWS 6 > #define PPKB_DEFAULT_KEYMAP_COLS 12 > @@ -132,6 +138,7 @@ static const struct matrix_keymap_data ppkb_default_keymap_data = { > }; > > struct pinephone_keyboard { > + struct i2c_adapter adapter; > struct input_dev *input; > unsigned short *fn_keymap; > u8 crc_table[CRC8_TABLE_SIZE]; > @@ -143,6 +150,57 @@ struct pinephone_keyboard { > u8 buf[]; > }; > > +static int ppkb_adap_smbus_xfer(struct i2c_adapter *adap, u16 addr, > + unsigned short flags, char read_write, > + u8 command, int size, > + union i2c_smbus_data *data) > +{ > + struct i2c_client *client = adap->algo_data; > + u8 buf[3]; > + int ret; > + > + buf[0] = command; > + buf[1] = data->byte; > + buf[2] = read_write == I2C_SMBUS_READ ? PPKB_SYS_COMMAND_SMBUS_READ > + : PPKB_SYS_COMMAND_SMBUS_WRITE; > + > + ret = i2c_smbus_write_i2c_block_data(client, PPKB_SYS_SMBUS_COMMAND, > + sizeof(buf), buf); > + if (ret) > + return ret; [1] > + /* Read back the command status until it passes or fails. */ > + do { > + usleep_range(300, 500); > + ret = i2c_smbus_read_byte_data(client, PPKB_SYS_COMMAND); > + } while (ret == buf[2]); > + if (ret < 0) > + return ret; > + /* Commands return 0x00 on success and 0xff on failure. */ > + if (ret) > + return -EIO; > + > + if (read_write == I2C_SMBUS_READ) { > + ret = i2c_smbus_read_byte_data(client, PPKB_SYS_SMBUS_DATA); > + if (ret < 0) > + return ret; Please use a single read transfer to get both command result and data. There will be less risk that some userspace app will issue another command in between command status being read as 0 and data byte being read. Otherwise if you use this in some read/modify/write operation, you may write unexpected value to PMIC. I2C register layout is designed to make this as optimal as possible in a single I2C transaction, so you only need 3 bytes to start command and 2 bytes to read the result and data, both in a single xfer. There's very high likelihood the command will complete in those 300 - 500 us anyway, because the timing is predictable. If this delay is set right, it's almost guaranteed, only two xfers will be necessary to run the command and get the result+ status. And if possible, it would be best if the bus was somehow made busy for other users, until the whole comand/result sequence completes, to eliminate the possibility of another command being issued by other bus users around [1]. Thank you and kind regards, o. > + data->byte = ret; > + } > + > + return 0; > +} > + > +static u32 ppkg_adap_functionality(struct i2c_adapter *adap) > +{ > + return I2C_FUNC_SMBUS_BYTE_DATA; > +} > + > +static const struct i2c_algorithm ppkb_adap_algo = { > + .smbus_xfer = ppkb_adap_smbus_xfer, > + .functionality = ppkg_adap_functionality, > +}; > + > static int ppkb_set_scan(struct i2c_client *client, bool enable) > { > struct device *dev = &client->dev; > @@ -265,6 +323,7 @@ static int ppkb_probe(struct i2c_client *client) > unsigned int map_rows, map_cols; > struct pinephone_keyboard *ppkb; > u8 info[PPKB_MATRIX_SIZE + 1]; > + struct device_node *i2c_bus; > int ret; > > ret = i2c_smbus_read_i2c_block_data(client, 0, sizeof(info), info); > @@ -312,6 +371,20 @@ static int ppkb_probe(struct i2c_client *client) > > i2c_set_clientdata(client, ppkb); > > + i2c_bus = of_get_child_by_name(dev->of_node, "i2c-bus"); > + if (i2c_bus) { > + ppkb->adapter.owner = THIS_MODULE; > + ppkb->adapter.algo = &ppkb_adap_algo; > + ppkb->adapter.algo_data = client; > + ppkb->adapter.dev.parent = dev; > + ppkb->adapter.dev.of_node = i2c_bus; > + strscpy(ppkb->adapter.name, DRV_NAME, sizeof(ppkb->adapter.name)); > + > + ret = devm_i2c_add_adapter(dev, &ppkb->adapter); > + if (ret) > + return dev_err_probe(dev, ret, "Failed to add I2C adapter\n"); > + } > + > crc8_populate_msb(ppkb->crc_table, PPKB_CRC8_POLYNOMIAL); > ppkb->row_shift = get_count_order(map_cols); > ppkb->rows = map_rows; > -- > 2.33.1 >
On 1/29/22 8:05 PM, Ondřej Jirman wrote: > Hello Samuel, > > On Sat, Jan 29, 2022 at 05:00:41PM -0600, Samuel Holland wrote: >> The PinePhone keyboard case contains a battery managed by an integrated >> power bank IC. The power bank IC communicates over I2C, and the keyboard >> MCU firmware provides an interface to read and write its registers. >> Let's use this interface to implement a SMBus adapter, so we can reuse >> the driver for the power bank IC. >> >> Signed-off-by: Samuel Holland <samuel@sholland.org> >> --- >> >> drivers/input/keyboard/pinephone-keyboard.c | 73 +++++++++++++++++++++ >> 1 file changed, 73 insertions(+) >> >> diff --git a/drivers/input/keyboard/pinephone-keyboard.c b/drivers/input/keyboard/pinephone-keyboard.c >> index 8065bc3e101a..7d2e16e588a0 100644 >> --- a/drivers/input/keyboard/pinephone-keyboard.c >> +++ b/drivers/input/keyboard/pinephone-keyboard.c >> @@ -3,6 +3,7 @@ >> // Copyright (C) 2021-2022 Samuel Holland <samuel@sholland.org> >> >> #include <linux/crc8.h> >> +#include <linux/delay.h> >> #include <linux/i2c.h> >> #include <linux/input/matrix_keypad.h> >> #include <linux/interrupt.h> >> @@ -23,6 +24,11 @@ >> #define PPKB_SCAN_DATA 0x08 >> #define PPKB_SYS_CONFIG 0x20 >> #define PPKB_SYS_CONFIG_DISABLE_SCAN BIT(0) >> +#define PPKB_SYS_SMBUS_COMMAND 0x21 >> +#define PPKB_SYS_SMBUS_DATA 0x22 >> +#define PPKB_SYS_COMMAND 0x23 >> +#define PPKB_SYS_COMMAND_SMBUS_READ 0x91 >> +#define PPKB_SYS_COMMAND_SMBUS_WRITE 0xa1 >> >> #define PPKB_DEFAULT_KEYMAP_ROWS 6 >> #define PPKB_DEFAULT_KEYMAP_COLS 12 >> @@ -132,6 +138,7 @@ static const struct matrix_keymap_data ppkb_default_keymap_data = { >> }; >> >> struct pinephone_keyboard { >> + struct i2c_adapter adapter; >> struct input_dev *input; >> unsigned short *fn_keymap; >> u8 crc_table[CRC8_TABLE_SIZE]; >> @@ -143,6 +150,57 @@ struct pinephone_keyboard { >> u8 buf[]; >> }; >> >> +static int ppkb_adap_smbus_xfer(struct i2c_adapter *adap, u16 addr, >> + unsigned short flags, char read_write, >> + u8 command, int size, >> + union i2c_smbus_data *data) >> +{ >> + struct i2c_client *client = adap->algo_data; >> + u8 buf[3]; >> + int ret; >> + >> + buf[0] = command; >> + buf[1] = data->byte; >> + buf[2] = read_write == I2C_SMBUS_READ ? PPKB_SYS_COMMAND_SMBUS_READ >> + : PPKB_SYS_COMMAND_SMBUS_WRITE; >> + >> + ret = i2c_smbus_write_i2c_block_data(client, PPKB_SYS_SMBUS_COMMAND, >> + sizeof(buf), buf); >> + if (ret) >> + return ret; > > [1] > >> + /* Read back the command status until it passes or fails. */ >> + do { >> + usleep_range(300, 500); >> + ret = i2c_smbus_read_byte_data(client, PPKB_SYS_COMMAND); >> + } while (ret == buf[2]); >> + if (ret < 0) >> + return ret; >> + /* Commands return 0x00 on success and 0xff on failure. */ >> + if (ret) >> + return -EIO; >> + >> + if (read_write == I2C_SMBUS_READ) { >> + ret = i2c_smbus_read_byte_data(client, PPKB_SYS_SMBUS_DATA); >> + if (ret < 0) >> + return ret; > > Please use a single read transfer to get both command result and data. > There will be less risk that some userspace app will issue another command > in between command status being read as 0 and data byte being read. > > Otherwise if you use this in some read/modify/write operation, you > may write unexpected value to PMIC. I2C register layout is designed > to make this as optimal as possible in a single I2C transaction, so > you only need 3 bytes to start command and 2 bytes to read the result > and data, both in a single xfer. There's very high likelihood the command > will complete in those 300 - 500 us anyway, because the timing is > predictable. If this delay is set right, it's almost guaranteed, > only two xfers will be necessary to run the command and get the result+ > status. I did this originally, but it causes a different race condition: since the data is read first, the command can complete between when the data is read and when the result is read. If this happens, the command will be seen as complete, but the data will be garbage. This caused occasional read errors for the charger's power supply properties, because I2C reads sometimes returned nonsensical values for those bytes. > And if possible, it would be best if the bus was somehow made busy for > other users, until the whole comand/result sequence completes, to eliminate > the possibility of another command being issued by other bus users > around [1]. Yes, I can add a call to i2c_lock_bus() here. Regards, Samuel > Thank you and kind regards, > o. > >> + data->byte = ret; >> + } >> + >> + return 0; >> +} >> + >> +static u32 ppkg_adap_functionality(struct i2c_adapter *adap) >> +{ >> + return I2C_FUNC_SMBUS_BYTE_DATA; >> +} >> + >> +static const struct i2c_algorithm ppkb_adap_algo = { >> + .smbus_xfer = ppkb_adap_smbus_xfer, >> + .functionality = ppkg_adap_functionality, >> +}; >> + >> static int ppkb_set_scan(struct i2c_client *client, bool enable) >> { >> struct device *dev = &client->dev; >> @@ -265,6 +323,7 @@ static int ppkb_probe(struct i2c_client *client) >> unsigned int map_rows, map_cols; >> struct pinephone_keyboard *ppkb; >> u8 info[PPKB_MATRIX_SIZE + 1]; >> + struct device_node *i2c_bus; >> int ret; >> >> ret = i2c_smbus_read_i2c_block_data(client, 0, sizeof(info), info); >> @@ -312,6 +371,20 @@ static int ppkb_probe(struct i2c_client *client) >> >> i2c_set_clientdata(client, ppkb); >> >> + i2c_bus = of_get_child_by_name(dev->of_node, "i2c-bus"); >> + if (i2c_bus) { >> + ppkb->adapter.owner = THIS_MODULE; >> + ppkb->adapter.algo = &ppkb_adap_algo; >> + ppkb->adapter.algo_data = client; >> + ppkb->adapter.dev.parent = dev; >> + ppkb->adapter.dev.of_node = i2c_bus; >> + strscpy(ppkb->adapter.name, DRV_NAME, sizeof(ppkb->adapter.name)); >> + >> + ret = devm_i2c_add_adapter(dev, &ppkb->adapter); >> + if (ret) >> + return dev_err_probe(dev, ret, "Failed to add I2C adapter\n"); >> + } >> + >> crc8_populate_msb(ppkb->crc_table, PPKB_CRC8_POLYNOMIAL); >> ppkb->row_shift = get_count_order(map_cols); >> ppkb->rows = map_rows; >> -- >> 2.33.1 >>
On Sat, Jan 29, 2022 at 08:43:30PM -0600, Samuel Holland wrote: > On 1/29/22 8:05 PM, Ondřej Jirman wrote: > > > > Please use a single read transfer to get both command result and data. > > There will be less risk that some userspace app will issue another command > > in between command status being read as 0 and data byte being read. > > > > Otherwise if you use this in some read/modify/write operation, you > > may write unexpected value to PMIC. I2C register layout is designed > > to make this as optimal as possible in a single I2C transaction, so > > you only need 3 bytes to start command and 2 bytes to read the result > > and data, both in a single xfer. There's very high likelihood the command > > will complete in those 300 - 500 us anyway, because the timing is > > predictable. If this delay is set right, it's almost guaranteed, > > only two xfers will be necessary to run the command and get the result+ > > status. > > I did this originally, but it causes a different race condition: since the data > is read first, the command can complete between when the data is read and when > the result is read. If this happens, the command will be seen as complete, but > the data will be garbage. > > This caused occasional read errors for the charger's power supply properties, > because I2C reads sometimes returned nonsensical values for those bytes. Oh, well. :) I guess the firmware would need to wait for any ongoing I2C tranfer to finish before setting the command status to 0, for this to work. Another lesson learned. :( > > And if possible, it would be best if the bus was somehow made busy for > > other users, until the whole comand/result sequence completes, to eliminate > > the possibility of another command being issued by other bus users > > around [1]. > > Yes, I can add a call to i2c_lock_bus() here. Perfect. thank you, o. > Regards, > Samuel
diff --git a/drivers/input/keyboard/pinephone-keyboard.c b/drivers/input/keyboard/pinephone-keyboard.c index 8065bc3e101a..7d2e16e588a0 100644 --- a/drivers/input/keyboard/pinephone-keyboard.c +++ b/drivers/input/keyboard/pinephone-keyboard.c @@ -3,6 +3,7 @@ // Copyright (C) 2021-2022 Samuel Holland <samuel@sholland.org> #include <linux/crc8.h> +#include <linux/delay.h> #include <linux/i2c.h> #include <linux/input/matrix_keypad.h> #include <linux/interrupt.h> @@ -23,6 +24,11 @@ #define PPKB_SCAN_DATA 0x08 #define PPKB_SYS_CONFIG 0x20 #define PPKB_SYS_CONFIG_DISABLE_SCAN BIT(0) +#define PPKB_SYS_SMBUS_COMMAND 0x21 +#define PPKB_SYS_SMBUS_DATA 0x22 +#define PPKB_SYS_COMMAND 0x23 +#define PPKB_SYS_COMMAND_SMBUS_READ 0x91 +#define PPKB_SYS_COMMAND_SMBUS_WRITE 0xa1 #define PPKB_DEFAULT_KEYMAP_ROWS 6 #define PPKB_DEFAULT_KEYMAP_COLS 12 @@ -132,6 +138,7 @@ static const struct matrix_keymap_data ppkb_default_keymap_data = { }; struct pinephone_keyboard { + struct i2c_adapter adapter; struct input_dev *input; unsigned short *fn_keymap; u8 crc_table[CRC8_TABLE_SIZE]; @@ -143,6 +150,57 @@ struct pinephone_keyboard { u8 buf[]; }; +static int ppkb_adap_smbus_xfer(struct i2c_adapter *adap, u16 addr, + unsigned short flags, char read_write, + u8 command, int size, + union i2c_smbus_data *data) +{ + struct i2c_client *client = adap->algo_data; + u8 buf[3]; + int ret; + + buf[0] = command; + buf[1] = data->byte; + buf[2] = read_write == I2C_SMBUS_READ ? PPKB_SYS_COMMAND_SMBUS_READ + : PPKB_SYS_COMMAND_SMBUS_WRITE; + + ret = i2c_smbus_write_i2c_block_data(client, PPKB_SYS_SMBUS_COMMAND, + sizeof(buf), buf); + if (ret) + return ret; + + /* Read back the command status until it passes or fails. */ + do { + usleep_range(300, 500); + ret = i2c_smbus_read_byte_data(client, PPKB_SYS_COMMAND); + } while (ret == buf[2]); + if (ret < 0) + return ret; + /* Commands return 0x00 on success and 0xff on failure. */ + if (ret) + return -EIO; + + if (read_write == I2C_SMBUS_READ) { + ret = i2c_smbus_read_byte_data(client, PPKB_SYS_SMBUS_DATA); + if (ret < 0) + return ret; + + data->byte = ret; + } + + return 0; +} + +static u32 ppkg_adap_functionality(struct i2c_adapter *adap) +{ + return I2C_FUNC_SMBUS_BYTE_DATA; +} + +static const struct i2c_algorithm ppkb_adap_algo = { + .smbus_xfer = ppkb_adap_smbus_xfer, + .functionality = ppkg_adap_functionality, +}; + static int ppkb_set_scan(struct i2c_client *client, bool enable) { struct device *dev = &client->dev; @@ -265,6 +323,7 @@ static int ppkb_probe(struct i2c_client *client) unsigned int map_rows, map_cols; struct pinephone_keyboard *ppkb; u8 info[PPKB_MATRIX_SIZE + 1]; + struct device_node *i2c_bus; int ret; ret = i2c_smbus_read_i2c_block_data(client, 0, sizeof(info), info); @@ -312,6 +371,20 @@ static int ppkb_probe(struct i2c_client *client) i2c_set_clientdata(client, ppkb); + i2c_bus = of_get_child_by_name(dev->of_node, "i2c-bus"); + if (i2c_bus) { + ppkb->adapter.owner = THIS_MODULE; + ppkb->adapter.algo = &ppkb_adap_algo; + ppkb->adapter.algo_data = client; + ppkb->adapter.dev.parent = dev; + ppkb->adapter.dev.of_node = i2c_bus; + strscpy(ppkb->adapter.name, DRV_NAME, sizeof(ppkb->adapter.name)); + + ret = devm_i2c_add_adapter(dev, &ppkb->adapter); + if (ret) + return dev_err_probe(dev, ret, "Failed to add I2C adapter\n"); + } + crc8_populate_msb(ppkb->crc_table, PPKB_CRC8_POLYNOMIAL); ppkb->row_shift = get_count_order(map_cols); ppkb->rows = map_rows;
The PinePhone keyboard case contains a battery managed by an integrated power bank IC. The power bank IC communicates over I2C, and the keyboard MCU firmware provides an interface to read and write its registers. Let's use this interface to implement a SMBus adapter, so we can reuse the driver for the power bank IC. Signed-off-by: Samuel Holland <samuel@sholland.org> --- drivers/input/keyboard/pinephone-keyboard.c | 73 +++++++++++++++++++++ 1 file changed, 73 insertions(+)