mbox series

[v16,0/4] Add Intel LJCA device driver

Message ID 1694569212-10080-1-git-send-email-wentong.wu@intel.com
Headers show
Series Add Intel LJCA device driver | expand

Message

Wu, Wentong Sept. 13, 2023, 1:40 a.m. UTC
Add driver for Intel La Jolla Cove Adapter (LJCA) device. This
IO-expander adds additional functions to the host system such
as GPIO, I2C and SPI with USB host interface. We add 4 drivers
to support this device: a USB driver, a GPIO chip driver, a I2C
controller driver and a SPI controller driver.

---
v16:
 - drop all void * and use real types in the exported apis and internal ljca_send()
 - remove #ifdef in usb-ljca.c file
 - add documentation in ljca.h for the public structures
 - add error message in ljca_handle_cmd_ack() if error happens and remove blank line
 - use the functionality in cleanup.h for spinlock to make function much simpler
 - change the type of ex_buf in struct ljca_adapter to u8 *

v15:
 - enhance disconnect() of usb-ljca driver
 - change memchr to strchr in ljca_match_device_ids() of usb-ljca driver

v14:
 - fix build error: implicit declaration of function 'acpi_dev_clear_dependencies'

v13:
 - make ljca-usb more robust with the help of Hans de Goede
 - call acpi_dev_clear_dependencies() to mark _DEP ACPI dependencies on the I2C controller as satisfied, and patch is from Hans de Goede

v12:
 - switch dev_err to dev_dbg for i2c-ljca driver
 - avoid err printing because of calling usb_kill_urb when attempts to resubmit the rx urb

v11:
 - switch dev_err to dev_dbg for i2c-ljca driver
 - remove message length check because of defined quirk structure
 - remove I2C_FUNC_SMBUS_EMUL support

v10:
 - remove ljca_i2c_format_slave_addr
 - remove memset before write write w_packet
 - make ljca_i2c_stop void and print err message in case failure
 - use dev_err_probe in ljca_i2c_probe function

v9:
 - overhaul usb-ljca driver to make it more structured and easy understand
 - fix memory leak issue for usb-ljca driver
 - add spinlock to protect tx_buf and ex_buf
 - change exported APIs for usb-ljca driver
 - unify prefix for structures and functions for i2c-ljca driver
 - unify prefix for structures and functions for spi-ljca driver
 - unify prefix for structures and functions for gpio-ljca driver
 - update gpio-ljca, i2c-ljca and spi-ljca drivers according to usb-ljca's changes

Wentong Wu (4):
  usb: Add support for Intel LJCA device
  i2c: Add support for Intel LJCA USB I2C driver
  spi: Add support for Intel LJCA USB SPI driver
  gpio: update Intel LJCA USB GPIO driver

 drivers/gpio/Kconfig          |   4 +-
 drivers/gpio/gpio-ljca.c      | 246 +++++++------
 drivers/i2c/busses/Kconfig    |  11 +
 drivers/i2c/busses/Makefile   |   1 +
 drivers/i2c/busses/i2c-ljca.c | 342 +++++++++++++++++
 drivers/spi/Kconfig           |  11 +
 drivers/spi/Makefile          |   1 +
 drivers/spi/spi-ljca.c        | 297 +++++++++++++++
 drivers/usb/misc/Kconfig      |  14 +
 drivers/usb/misc/Makefile     |   1 +
 drivers/usb/misc/usb-ljca.c   | 835 ++++++++++++++++++++++++++++++++++++++++++
 include/linux/usb/ljca.h      | 145 ++++++++
 12 files changed, 1803 insertions(+), 105 deletions(-)
 create mode 100644 drivers/i2c/busses/i2c-ljca.c
 create mode 100644 drivers/spi/spi-ljca.c
 create mode 100644 drivers/usb/misc/usb-ljca.c
 create mode 100644 include/linux/usb/ljca.h

Comments

Oliver Neukum Sept. 13, 2023, 9:03 a.m. UTC | #1
On 13.09.23 03:40, Wentong Wu wrote:

> +struct ljca_bank_descriptor {
> +	u8 bank_id;
> +	u8 pin_num;
> +
> +	/* 1 bit for each gpio, 1 means valid */
> +	u32 valid_pins;

No endianness?

> +} __packed;

	Regards
		Oliver
Wu, Wentong Sept. 14, 2023, 1:05 a.m. UTC | #2
> From: Oliver Neukum <oneukum@suse.com>
>
> On 13.09.23 03:40, Wentong Wu wrote:
> 
> > +struct ljca_bank_descriptor {
> > +	u8 bank_id;
> > +	u8 pin_num;
> > +
> > +	/* 1 bit for each gpio, 1 means valid */
> > +	u32 valid_pins;
> 
> No endianness?

On both sides, the endianness is same.

BTW, the code has been tested on several real productions.

BR,
Wentong
> 
> > +} __packed;
> 
> 	Regards
> 		Oliver
Hans de Goede Sept. 14, 2023, 8:21 a.m. UTC | #3
Hi,

On 9/14/23 03:05, Wu, Wentong wrote:
>> From: Oliver Neukum <oneukum@suse.com>
>>
>> On 13.09.23 03:40, Wentong Wu wrote:
>>
>>> +struct ljca_bank_descriptor {
>>> +	u8 bank_id;
>>> +	u8 pin_num;
>>> +
>>> +	/* 1 bit for each gpio, 1 means valid */
>>> +	u32 valid_pins;
>>
>> No endianness?
> 
> On both sides, the endianness is same.

Right, but normally USB drivers are also written so that they can work on big-endian CPUs.

I realize that this driver will likely never be used with a big-endian CPU but still it is good practice to make the driver work on big-endian CPUs too. Even if it is just to set a good example when other drivers copy the code.

So this should be:

struct ljca_bank_descriptor {
	u8 bank_id;
	u8 pin_num;

	/* 1 bit for each gpio, 1 means valid */
	__le32 valid_pins;
} __packed;

And then when reading valid_pins you should use:

	u32 valid_pins = get_unaligned_le32(&ljca_bank_descriptor.valid_pins);

On x86_64 the compiler should optimize all of this away to
just a regular read.

Regards,

Hans
Wu, Wentong Sept. 14, 2023, 8:58 a.m. UTC | #4
> From: Hans de Goede <hdegoede@redhat.com>
> 
> Hi,
> 
> On 9/14/23 03:05, Wu, Wentong wrote:
> >> From: Oliver Neukum <oneukum@suse.com>
> >>
> >> On 13.09.23 03:40, Wentong Wu wrote:
> >>
> >>> +struct ljca_bank_descriptor {
> >>> +	u8 bank_id;
> >>> +	u8 pin_num;
> >>> +
> >>> +	/* 1 bit for each gpio, 1 means valid */
> >>> +	u32 valid_pins;
> >>
> >> No endianness?
> >
> > On both sides, the endianness is same.
> 
> Right, but normally USB drivers are also written so that they can work on big-
> endian CPUs.
> 
> I realize that this driver will likely never be used with a big-endian CPU but still it
> is good practice to make the driver work on big-endian CPUs too. Even if it is just
> to set a good example when other drivers copy the code.

Thanks, I agree the point here.

And I will update the driver after the test, thanks again.

BR,
Wentong
>
> 
> So this should be:
> 
> struct ljca_bank_descriptor {
> 	u8 bank_id;
> 	u8 pin_num;
> 
> 	/* 1 bit for each gpio, 1 means valid */
> 	__le32 valid_pins;
> } __packed;
> 
> And then when reading valid_pins you should use:
> 
> 	u32 valid_pins = get_unaligned_le32(&ljca_bank_descriptor.valid_pins);
> 
> On x86_64 the compiler should optimize all of this away to just a regular read.
> 
> Regards,
> 
> Hans