linux-kernel - Re: [PATCH v2 1/5] i2c: Introduce common module to instantiate CCGx UCSI

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <57ef9519-23d5-25f0-9385-7646aba52316@amd.com>
Date:   Mon, 27 Dec 2021 12:01:00 +0530
From:   "Shah, Nehal-bakulchandra" <nehal-bakulchandra.shah@....com>
To:     Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        Wolfram Sang <wsa@...nel.org>, linux-kernel@...r.kernel.org,
        linux-i2c@...r.kernel.org
Cc:     Jarkko Nikula <jarkko.nikula@...ux.intel.com>,
        Mika Westerberg <mika.westerberg@...ux.intel.com>,
        Ajay Gupta <ajayg@...dia.com>, sanket.goswami@....com
Subject: Re: [PATCH v2 1/5] i2c: Introduce common module to instantiate CCGx
 UCSI

Hi Andy

Thanks for the this patch series.
On 12/22/2021 9:50 PM, Andy Shevchenko wrote:
> Introduce a common module to provide an API to instantiate UCSI device
> for Cypress CCGx Type-C controller. Individual bus drivers need to select
> this one on demand.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
> ---
> v2: rebased on top of current i2c/for-next
>   drivers/i2c/busses/Kconfig         |  7 +++++++
>   drivers/i2c/busses/Makefile        |  3 +++
>   drivers/i2c/busses/i2c-ccgx-ucsi.c | 27 +++++++++++++++++++++++++++
>   drivers/i2c/busses/i2c-ccgx-ucsi.h | 11 +++++++++++
>   4 files changed, 48 insertions(+)
>   create mode 100644 drivers/i2c/busses/i2c-ccgx-ucsi.c
>   create mode 100644 drivers/i2c/busses/i2c-ccgx-ucsi.h
> 
> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
> index 42da31c1ab70..08e24e396e37 100644
> --- a/drivers/i2c/busses/Kconfig
> +++ b/drivers/i2c/busses/Kconfig
> @@ -9,6 +9,13 @@ menu "I2C Hardware Bus support"
>   comment "PC SMBus host controller drivers"
>   	depends on PCI
>   
> +config I2C_CCGX_UCSI
> +	tristate
> +	help
> +	  A common module to provide an API to instantiate UCSI device
> +	  for Cypress CCGx Type-C controller. Individual bus drivers
> +	  need to select this one on demand.
> +
>   config I2C_ALI1535
>   	tristate "ALI 1535"
>   	depends on PCI
> diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
> index 1d00dce77098..79405cb5d600 100644
> --- a/drivers/i2c/busses/Makefile
> +++ b/drivers/i2c/busses/Makefile
> @@ -6,6 +6,9 @@
>   # ACPI drivers
>   obj-$(CONFIG_I2C_SCMI)		+= i2c-scmi.o
>   
> +# Auxiliary I2C/SMBus modules
> +obj-$(CONFIG_I2C_CCGX_UCSI)	+= i2c-ccgx-ucsi.o
> +
>   # PC SMBus host controller drivers
>   obj-$(CONFIG_I2C_ALI1535)	+= i2c-ali1535.o
>   obj-$(CONFIG_I2C_ALI1563)	+= i2c-ali1563.o
> diff --git a/drivers/i2c/busses/i2c-ccgx-ucsi.c b/drivers/i2c/busses/i2c-ccgx-ucsi.c
> new file mode 100644
> index 000000000000..141c3d1ef752
> --- /dev/null
> +++ b/drivers/i2c/busses/i2c-ccgx-ucsi.c
> @@ -0,0 +1,27 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Instantiate UCSI device for Cypress CCGx Type-C controller.
> + * Derived from i2c-designware-pcidrv.c and i2c-nvidia-gpu.c.
> + */
> +
> +#include <linux/i2c.h>
> +#include <linux/export.h>
> +#include <linux/string.h>
> +
> +#include "i2c-ccgx-ucsi.h"
> +
> +struct software_node;
> +
> +struct i2c_client *i2c_new_ccgx_ucsi(struct i2c_adapter *adapter, int irq,
> +				     const struct software_node *swnode)
> +{
> +	struct i2c_board_info info = {};
> +
> +	strscpy(info.type, "ccgx-ucsi", sizeof(info.type));
> +	info.addr = 0x08;
> +	info.irq = irq;
> +	info.swnode = swnode;
> +
> +	return i2c_new_client_device(adapter, &info);
> +}
> +EXPORT_SYMBOL_GPL(i2c_new_ccgx_ucsi);c
  it needs MODULE_LICENSE("GPL"); else if driver is built as module it 
fails to probe. However after adding this we validated and it is working 
fine.
> diff --git a/drivers/i2c/busses/i2c-ccgx-ucsi.h b/drivers/i2c/busses/i2c-ccgx-ucsi.h
> new file mode 100644
> index 000000000000..739ac7a4b117
> --- /dev/null
> +++ b/drivers/i2c/busses/i2c-ccgx-ucsi.h
> @@ -0,0 +1,11 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +#ifndef __I2C_CCGX_UCSI_H_
> +#define __I2C_CCGX_UCSI_H_
> +
> +struct i2c_adapter;
> +struct i2c_client;
> +struct software_node;
> +
> +struct i2c_client *i2c_new_ccgx_ucsi(struct i2c_adapter *adapter, int irq,
> +				     const struct software_node *swnode);
> +#endif /* __I2C_CCGX_UCSI_H_ */
> 

Here, One more suggestion if can be incorporated , instead of passing 
only irq we should pass irq number and irq type. For example in our next 
generation platform , CCGX driver is using IRQF_TRIGGER_FALLING type 
where is default hard coded is IRQF_TRIGGER_HIGH. So in CCGX driver in 
request_threaded_irq function along with passing irq number , irq type 
also can be passed.

Regards
Nehal Shah