Message-ID: <ceace8d0-d8ec-4010-a65f-bec36833c16f@quicinc.com>
Date: Mon, 14 Apr 2025 11:21:59 +0530
From: Mukesh Kumar Savaliya <quic_msavaliy@...cinc.com>
To: Konrad Dybcio <konrad.dybcio@....qualcomm.com>,
        <alexandre.belloni@...tlin.com>, <robh@...nel.org>,
        <krzk+dt@...nel.org>, <conor+dt@...nel.org>,
        <jarkko.nikula@...ux.intel.com>, <linux-i3c@...ts.infradead.org>,
        <linux-arm-msm@...r.kernel.org>, <devicetree@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>
CC: <andersson@...nel.org>, <konradybcio@...nel.org>
Subject: Re: [PATCH v4 2/3] i3c: master: Add Qualcomm I3C controller driver

Thanks Konrad for detailed review.


On 4/12/2025 4:45 AM, Konrad Dybcio wrote:
> On 4/11/25 1:35 PM, Mukesh Kumar Savaliya wrote:
>> Add support for the Qualcomm I3C controller driver, which implements
>> I3C master functionality as defined in the MIPI Alliance Specification
>> for I3C, Version 1.0.
>>
>> This driver supports master role in SDR mode.
>>
>> Unlike some other I3C master controllers, this implementation
>> does not support In-Band Interrupts (IBI) and Hot-join requests.
>>
>> Signed-off-by: Mukesh Kumar Savaliya <quic_msavaliy@...cinc.com>
>> ---
> 
> [...]
> 
>> +#define SE_I3C_SCL_HIGH                 0x268
>> +#define SE_I3C_TX_TRANS_LEN             0x26C
>> +#define SE_I3C_RX_TRANS_LEN             0x270
>> +#define SE_I3C_DELAY_COUNTER            0x274
>> +#define SE_I2C_SCL_COUNTERS             0x278
>> +#define SE_I3C_SCL_CYCLE                0x27C
> 
> lowercase hex, please
sure, done.
> 
>> +#define SE_GENI_HW_IRQ_EN               0x920
>> +#define SE_GENI_HW_IRQ_IGNORE_ON_ACTIVE 0x924
>> +#define SE_GENI_HW_IRQ_CMD_PARAM_0      0x930
>> +
>> +/* HW I3C IBI interrupt enable */
>> +#define M_IBI_IRQ_EN BIT(0)
>> +
>> +/* M_IBI_IRQ_IGNORE */
>> +#define M_IBI_IRQ_IGNORE BIT(0)
>> +
>> +/* SE_GENI_M_CLK_CFG field shifts */
>> +#define	CLK_DIV_VALUE_MASK	GENMASK(23, 4)
> 
> ' ' after "define", '\t' after
> 
Done. Also added equal tabs for all other macros too.
>> +#define SER_CLK_EN		BIT(0)
>> +
>> +/* SE_GENI_HW_IRQ_CMD_PARAM_0 field bits */
>> +#define M_IBI_IRQ_PARAM_7E BIT(0)
>> +#define M_IBI_IRQ_PARAM_STOP_STALL BIT(1)
>> +/* SE_I2C_SCL_COUNTERS field shifts */
>> +#define I2C_SCL_HIGH_COUNTER_MASK	GENMASK(29, 20)
>> +#define I2C_SCL_LOW_COUNTER_MASK	GENMASK(19, 10)
>> +#define I2C_SCL_CYCLE_COUNTER_MASK	GENMASK(9, 0)
>> +
>> +#define SE_I3C_ERR  (M_CMD_OVERRUN_EN | M_ILLEGAL_CMD_EN | M_CMD_FAILURE_EN |\
>> +	M_CMD_ABORT_EN | M_GP_IRQ_0_EN | M_GP_IRQ_1_EN | M_GP_IRQ_2_EN | \
>> +	M_GP_IRQ_3_EN | M_GP_IRQ_4_EN)
>> +
>> +/* M_CMD OP codes for I2C/I3C */
>> +#define I3C_READ_IBI_HW          0
>> +#define I2C_WRITE                1
>> +#define I2C_READ                 2
>> +#define I2C_WRITE_READ           3
>> +#define I2C_ADDR_ONLY            4
>> +#define I3C_INBAND_RESET         5
>> +#define I2C_BUS_CLEAR            6
>> +#define I2C_STOP_ON_BUS          7
>> +#define I3C_HDR_DDR_EXIT         8
>> +#define I3C_PRIVATE_WRITE        9
>> +#define I3C_PRIVATE_READ         10
>> +#define I3C_HDR_DDR_WRITE        11
>> +#define I3C_HDR_DDR_READ         12
>> +#define I3C_DIRECT_CCC_ADDR_ONLY 13
>> +#define I3C_BCAST_CCC_ADDR_ONLY  14
>> +#define I3C_READ_IBI             15
>> +#define I3C_BCAST_CCC_WRITE      16
>> +#define I3C_DIRECT_CCC_WRITE     17
>> +#define I3C_DIRECT_CCC_READ      18
>> +
>> +/* M_CMD params for I3C */
>> +#define PRE_CMD_DELAY          BIT(0)
>> +#define TIMESTAMP_BEFORE       BIT(1)
>> +#define STOP_STRETCH           BIT(2)
>> +#define TIMESTAMP_AFTER        BIT(3)
>> +#define POST_COMMAND_DELAY     BIT(4)
>> +#define IGNORE_ADD_NACK        BIT(6)
>> +#define READ_FINISHED_WITH_ACK BIT(7)
>> +#define CONTINUOUS_MODE_DAA    BIT(8)
>> +
>> +#define SLAVE_ADDR_MASK           GENMASK(15, 9)
>> +#define SLV_ADDR_SHFT          9
> 
> The shift is now unused
> 
Yes, Removed.
>> +
>> +#define CCC_HDR_CMD_MSK        GENMASK(23, 16)
>> +#define CCC_HDR_CMD_SHFT       16
> 
> And this one *should* be unused, fill in the value with FIELD_PREP instead
Yes, Done.
> 
>> +#define IBI_NACK_TBL_CTRL      BIT(24)
>> +#define USE_7E                 BIT(25)
>> +#define BYPASS_ADDR_PHASE      BIT(26)
>> +
>> +/* GSI callback error fields - DMA_TX_IRQ_STAT */
>> +#define GP_IRQ0	BIT(5)
>> +#define GP_IRQ1	BIT(6)
>> +#define GP_IRQ2	BIT(7)
>> +#define GP_IRQ3	BIT(8)
>> +#define GP_IRQ4	BIT(9)
>> +#define GP_IRQ5	BIT(10)
>> +#define DM_I3C_CB_ERR   GENMASK(10, 5)
>> +
>> +#define I3C_AUTO_SUSPEND_DELAY  250
>> +#define KHZ(freq)               (1000 * (freq))
> 
> You can store the values in kHz and do (x * HZ_PER_KHZ) where they're
> used instead
> 
Sure, Done.

>> +#define PACKING_BYTES_PW        4
> 
> PW -> "PER_WORD", if that's what it means, please
Yes, changed to PACKING_BYTES_PER_WORD.
> 
>> +#define XFER_TIMEOUT            250
>> +#define DFS_INDEX_MAX           7
>> +
>> +#define I3C_ADDR_MASK 0x7f
> 
> This could go to some i3c common header, IIUC it's part of the spec
> 
Yes, let me use I2C_ADDR_MASK from i3c/master.h and Used same macro name 
to maintain I3C context here.
> [...]
> 
>> +static inline struct geni_i3c_dev *to_geni_i3c_master(struct i3c_master_controller
>> +							*master)
>> +{
>> +	return container_of(master, struct geni_i3c_dev, ctrlr);
>> +}
> 
> #define instead
> 
i see all i3c vendors are using same definitions, so for now can we keep 
it similar way if it's minor ?
>> +
>> +struct geni_i3c_clk_settings {
>> +	u32 clk_freq_out;
>> +	u32 clk_src_freq;
>> +	u8  clk_div;
>> +	u8  i2c_t_high_cnt;
>> +	u8  i2c_t_low_cnt;
>> +	u8  i3c_t_high_cnt;
>> +	u8  i3c_t_cycle_cnt;
>> +	u8  i2c_t_cycle_cnt;
>> +};
>> +
>> +/*
>> + * Hardware uses the underlying formula to calculate time periods of
>> + * SCL clock cycle. Firmware uses some additional cycles excluded from the
>> + * below formula and it is confirmed that the time periods are within
>> + * specification limits.
>> + *
>> + * time of high period of I2C SCL:
>> + *         i2c_t_high = (i2c_t_high_cnt * clk_div) / source_clock
>> + * time of low period of I2C SCL:
>> + *         i2c_t_low = (i2c_t_low_cnt * clk_div) / source_clock
>> + * time of full period of I2C SCL:
>> + *         i2c_t_cycle = (i2c_t_cycle_cnt * clk_div) / source_clock
>> + * time of high period of I3C SCL:
>> + *         i3c_t_high = (i3c_t_high_cnt * clk_div) / source_clock
>> + * time of full period of I3C SCL:
>> + *         i3c_t_cycle = (i3c_t_cycle_cnt * clk_div) / source_clock
>> + * clk_freq_out = t / t_cycle
>> + */
>> +static const struct geni_i3c_clk_settings geni_i3c_clk_map[] = {
>> +/* op-freq,  src-freq,  div,  i2c_high,  i2c_low, i3c_high, i3c_cyc i2c_cyc */
>> +	{ KHZ(100),    KHZ(19200), 1, 76, 90, 7, 8, 192},
>> +	{ KHZ(400),    KHZ(19200), 1, 12, 24, 7, 8, 48},
>> +	{ KHZ(1000),   KHZ(19200), 1,  4,  9, 7, 0, 19},
>> +	{ KHZ(12500), KHZ(100000), 1, 45, 63, 6, 7, 110},
> 
> It's gonna take up considerably more space, but you may want to write
> this using C99 designated initializers:
> 
> {
> 	.op_freq = ...,
> 	.src_freq = ...,
> }, {
> 	...
> }
> 
> etc.
Sure, changed accordingly.
> 
>> +};
>> +
>> +static int geni_i3c_clk_map_idx(struct geni_i3c_dev *gi3c)
>> +{
>> +	const struct geni_i3c_clk_settings *clk_idx = geni_i3c_clk_map;
>> +	struct i3c_master_controller *m = &gi3c->ctrlr;
>> +	struct i3c_bus *bus = i3c_master_get_bus(m);
>> +	int i;
>> +
>> +	for (i = 0; i < ARRAY_SIZE(geni_i3c_clk_map); i++, clk_idx++) {
>> +		if ((!bus || clk_idx->clk_freq_out == bus->scl_rate.i3c) &&
> 
> Move this nullcheck out of the loop, please.
> Or get rid of it, the master registration ensures that there's a bus bound
> to it, IIUC.
> 
Sure, makes sense! Removed it.

>> +		    clk_idx->clk_src_freq == gi3c->clk_src_freq) {
>> +			gi3c->clk_cfg = clk_idx;
>> +		}
>> +
>> +		if (clk_idx->clk_freq_out == bus->scl_rate.i2c)
>> +			gi3c->clk_od_cfg = clk_idx;
>> +	}
>> +
>> +	if (!gi3c->clk_cfg || !gi3c->clk_od_cfg)
>> +		return -EINVAL;
> 
> IIUC this makes the 100 kHz configuration invalid (index 0)
> 
Yes, for i3c, 100KHz is not possible. i2c can work but i3c can't.
>> +
>> +	return 0;
>> +}
>> +
>> +static void set_new_addr_slot(unsigned long *addrslot, u8 addr)
>> +{
>> +	if (addr > I3C_ADDR_MASK)
>> +		return;
>> +
>> +	set_bit(addr, addrslot);
>> +}
> 
> Inline this single-use function, please
> 
Done
>> +
>> +static void clear_new_addr_slot(unsigned long *addrslot, u8 addr)
>> +{
>> +	if (addr > I3C_ADDR_MASK)
>> +		return;
>> +
>> +	clear_bit(addr, addrslot);
>> +}
> 
> And this one
> 
Done
>> +
>> +static bool is_new_addr_slot_set(unsigned long *addrslot, u8 addr)
>> +{
>> +	if (addr > I3C_ADDR_MASK)
>> +		return false;
> 
> This condition is impossible given the user
Removed it.
> 
>> +
>> +	return test_bit(addr, addrslot);
> 
> Inline this check
> 
To have the function name meaning in the action by caller, i am keeping 
it. But i have inlined this function also. Hope its fine.
> [...]
> 
>> +static void qcom_geni_i3c_conf(struct geni_i3c_dev *gi3c, enum i3c_bus_phase bus_phase)
>> +{
>> +	const struct geni_i3c_clk_settings *clk_idx = gi3c->clk_cfg;
>> +	unsigned long freq;
>> +	u32 val, dfs_idx;
>> +	int ret;
>> +
>> +	if (bus_phase == OPEN_DRAIN_MODE)
>> +		clk_idx = gi3c->clk_od_cfg;
>> +
>> +	ret = geni_se_clk_freq_match(&gi3c->se, clk_idx->clk_src_freq,
>> +				     &dfs_idx, &freq, false);
>> +	if (ret)
>> +		dfs_idx = 0;
> 
> Shouldn't we throw some sort of error here instead?
> 
No, dfs_idx = 0 should be fine too. As default source frequency 19.2MHz 
can work at index 0.
> [...]
> 
>> +static void geni_i3c_handle_err(struct geni_i3c_dev *gi3c, u32 status)
>> +{
>> +	dev_dbg(gi3c->se.dev, "len:%d, %s\n",
>> +		gi3c->cur_len, gi3c->cur_is_write ? "write" : "read");
>> +
>> +	if (status & M_GP_IRQ_0_EN) {
>> +		gi3c->err = -EINVAL;
>> +		dev_dbg(gi3c->se.dev, "%s\n", "I3C slave early read termination");
> 
> Whenever there's just a string literal, please just print it directly.
> 
Sure, Done for all the places.
>> +	}
> 
> Please separate these with a newline
> 
Sure, Done for all the conditions.
>> +	if (status & M_GP_IRQ_1_EN) {
>> +		gi3c->err = -ENOTCONN;
>> +		dev_dbg(gi3c->se.dev, "%s\n", "NACK: slave unresponsive, check power/reset");
>> +	}
> 
> [...]
> 
>> +static irqreturn_t geni_i3c_irq(int irq, void *dev)
>> +{
>> +	u32 m_stat_mask, dm_tx_st, dm_rx_st, m_stat, rx_st, dma;
>> +	struct geni_i3c_dev *gi3c = dev;
>> +	unsigned long flags;
>> +	int j;
>> +
>> +	m_stat = readl_relaxed(gi3c->se.base + SE_GENI_M_IRQ_STATUS);
>> +	m_stat_mask = readl_relaxed(gi3c->se.base + SE_GENI_M_IRQ_EN);
>> +	rx_st = readl_relaxed(gi3c->se.base + SE_GENI_RX_FIFO_STATUS);
>> +	dm_tx_st = readl_relaxed(gi3c->se.base + SE_DMA_TX_IRQ_STAT);
>> +	dm_rx_st = readl_relaxed(gi3c->se.base + SE_DMA_RX_IRQ_STAT);
>> +	dma = readl_relaxed(gi3c->se.base + SE_GENI_DMA_MODE_EN);
>> +
>> +	spin_lock_irqsave(&gi3c->irq_lock, flags);
>> +
>> +	if (m_stat & SE_I3C_ERR || dm_rx_st & DM_I3C_CB_ERR) {
>> +		geni_i3c_handle_err(gi3c, m_stat);
>> +		/* Disable the TX Watermark interrupt to stop TX */
>> +		if (!dma)
>> +			writel_relaxed(0, gi3c->se.base + SE_GENI_TX_WATERMARK_REG);
>> +		goto irqret;
>> +	}
>> +
>> +	if (dma) {
>> +		dev_dbg(gi3c->se.dev, "i3c dma tx:0x%x, dma rx:0x%x\n", dm_tx_st, dm_rx_st);
>> +		goto irqret;
>> +	}
>> +
>> +	if ((m_stat &
>> +		(M_RX_FIFO_WATERMARK_EN | M_RX_FIFO_LAST_EN)) &&
>> +		!gi3c->cur_is_write && gi3c->cur_buf) {
> 
> The indentation here is confusing, please align the `(M_RX..` with `m_stat &`
> 
Sure, Done.
>> +		u32 rxcnt = rx_st & RX_FIFO_WC_MSK;
>> +
>> +		for (j = 0; j < rxcnt; j++) {
>> +			u32 val;
>> +			int p = 0;
>> +
>> +			val = readl_relaxed(gi3c->se.base + SE_GENI_RX_FIFOn);
>> +			while (gi3c->cur_idx < gi3c->cur_len && p < sizeof(val)) {
> 
> sizeof(val) is 4, please unroll this loop into something more readable
> 
Replaced directly with 4.
>> +				gi3c->cur_buf[gi3c->cur_idx++] = val & 0xff;
>> +				val >>= 8;
>> +				p++;
>> +			}
>> +			if (gi3c->cur_idx == gi3c->cur_len)
>> +				break;
>> +		}
>> +	} else if (m_stat & M_TX_FIFO_WATERMARK_EN &&
>> +		gi3c->cur_is_write && gi3c->cur_buf) {
>> +		for (j = 0; j < gi3c->tx_wm; j++) {
>> +			u32 temp;
>> +			u32 val = 0;
>> +			int p = 0;
>> +
>> +			while (gi3c->cur_idx < gi3c->cur_len && p < sizeof(val)) {
> 
> ditto
Done
> 
>> +				temp = gi3c->cur_buf[gi3c->cur_idx++];
>> +				val |= temp << (p * 8);
>> +				p++;
>> +			}
>> +
>> +			writel_relaxed(val, gi3c->se.base + SE_GENI_TX_FIFOn);
>> +			if (gi3c->cur_idx == gi3c->cur_len) {
>> +				writel_relaxed(0, gi3c->se.base + SE_GENI_TX_WATERMARK_REG);
>> +				break;
>> +			}
>> +		}
>> +	}
>> +
>> +irqret:
>> +	if (m_stat)
>> +		writel_relaxed(m_stat, gi3c->se.base + SE_GENI_M_IRQ_CLEAR);
>> +
>> +	if (dma) {
>> +		if (dm_tx_st)
>> +			writel_relaxed(dm_tx_st, gi3c->se.base + SE_DMA_TX_IRQ_CLR);
>> +		if (dm_rx_st)
>> +			writel_relaxed(dm_rx_st, gi3c->se.base + SE_DMA_RX_IRQ_CLR);
>> +	}
>> +
>> +	/* if this is err with done-bit not set, handle that through timeout. */
>> +	if (m_stat & M_CMD_DONE_EN || m_stat & M_CMD_ABORT_EN) {
>> +		writel_relaxed(0, gi3c->se.base + SE_GENI_TX_WATERMARK_REG);
>> +		complete(&gi3c->done);
>> +	} else if (dm_tx_st & TX_DMA_DONE || dm_rx_st & RX_DMA_DONE	||
> 
> odd '\t'
> 
yes, Aligned.
>> +		dm_rx_st & RX_RESET_DONE) {
>> +		complete(&gi3c->done);
>> +	}
>> +
>> +	spin_unlock_irqrestore(&gi3c->irq_lock, flags);
>> +	return IRQ_HANDLED;
> 
> a '\n' before return is customary
> 
Done
> [...]
> 
>> +static int _i3c_geni_execute_command(struct geni_i3c_dev *gi3c, struct geni_i3c_xfer_params *xfer)
>> +{
>> +	bool is_write = gi3c->cur_is_write;
>> +	dma_addr_t tx_dma = 0, rx_dma = 0;
>> +	unsigned long time_remaining;
>> +	u32 len = gi3c->cur_len;
>> +	int ret;
>> +
>> +	geni_se_select_mode(&gi3c->se, xfer->mode);
>> +
>> +	gi3c->err = 0;
>> +	gi3c->cur_idx = 0;
>> +
>> +	if (!is_write) {
> 
> Nit: if (is_write) {} .. else {} is more natural> +		dev_dbg(gi3c->se.dev, "I3C cmd:0x%x param:0x%x READ len:%d\n",
Sure, Reversed with natural and positive check.
I didn't get about debug log suggestion. Do you want to optimize it to 
one for both if/else condition ?
>> +			xfer->m_cmd, xfer->m_param, len);
>> +		writel_relaxed(len, gi3c->se.base + SE_I3C_RX_TRANS_LEN);
>> +		geni_se_setup_m_cmd(&gi3c->se, xfer->m_cmd, xfer->m_param);
>> +		if (xfer->mode == GENI_SE_DMA) {
>> +			ret = geni_se_rx_dma_prep(&gi3c->se, gi3c->cur_buf, len, &rx_dma);
>> +			if (ret) {
> Why would it fail? And why should we fall back silently to FIFO mode then?
> 
DMA mapping can fail OR input validation can also fail. So we want to 
continue with FIFO mode.
>> +				xfer->mode = GENI_SE_FIFO;
>> +				geni_se_select_mode(&gi3c->se, xfer->mode);
>> +			}
>> +		}
>> +	} else {
>> +		dev_dbg(gi3c->se.dev, "I3C cmd:0x%x param:0x%x WRITE len:%d\n",
>> +			xfer->m_cmd, xfer->m_param, len);
>> +
>> +		writel_relaxed(len, gi3c->se.base + SE_I3C_TX_TRANS_LEN);
>> +		geni_se_setup_m_cmd(&gi3c->se, xfer->m_cmd, xfer->m_param);
>> +
>> +		if (xfer->mode == GENI_SE_DMA) {
>> +			ret = geni_se_tx_dma_prep(&gi3c->se, gi3c->cur_buf, len, &tx_dma);
>> +			if (ret) {
>> +				xfer->mode = GENI_SE_FIFO;
>> +				geni_se_select_mode(&gi3c->se, xfer->mode);
>> +			}
>> +		}
>> +
>> +		if (xfer->mode == GENI_SE_FIFO && len > 0) /* Get FIFO IRQ */
>> +			writel_relaxed(1, gi3c->se.base + SE_GENI_TX_WATERMARK_REG);
>> +	}
>> +
>> +	time_remaining = wait_for_completion_timeout(&gi3c->done, XFER_TIMEOUT);
>> +	if (!time_remaining) {
>> +		unsigned long flags;
>> +
>> +		dev_dbg(gi3c->se.dev, "Timeout completing FIFO transfer\n");
> 
> Can it not be DMA mode here too?
> 
Good find, it's common timeout error. Removed FIFO word.
> [...]
> 
>> +static void geni_i3c_perform_daa(struct geni_i3c_dev *gi3c)
>> +{
>> +	u8 last_dyn_addr = 0;
>> +	int ret;
>> +
>> +	while (1) {
>> +		u8 rx_buf[8], tx_buf[8];
>> +		struct geni_i3c_xfer_params xfer = { GENI_SE_FIFO };
>> +		struct i3c_device_info info = { 0 };
>> +		struct i3c_dev_desc *i3cdev;
>> +		bool new_device = true;
>> +		u64 pid;
>> +		u8 bcr, dcr, addr;
>> +
>> +		xfer.m_cmd = I2C_READ;
>> +		xfer.m_param = STOP_STRETCH | CONTINUOUS_MODE_DAA | USE_7E;
>> +		ret = i3c_geni_execute_read_command(gi3c, &xfer, rx_buf, 8);
>> +		if (ret)
>> +			break;
>> +
>> +		dcr = rx_buf[7];
>> +		bcr = rx_buf[6];
>> +		pid = ((u64)rx_buf[0] << 40) |
>> +			((u64)rx_buf[1] << 32) |
>> +			((u64)rx_buf[2] << 24) |
>> +			((u64)rx_buf[3] << 16) |
>> +			((u64)rx_buf[4] <<  8) |
>> +			((u64)rx_buf[5]);
> 
> FIELD_PREP + GENMASK, please
> 
Sure, Done.
>> +
>> +		i3c_bus_for_each_i3cdev(&gi3c->ctrlr.bus, i3cdev) {
>> +			i3c_device_get_info(i3cdev->dev, &info);
>> +			if (pid == info.pid && dcr == info.dcr && bcr == info.bcr) {
>> +				new_device = false;
>> +				addr = (info.dyn_addr) ? info.dyn_addr :
> 
> addr = info.dyn_addr ?: info.static_addr;
> 
Yes, Done.
>> +					info.static_addr;
>> +				break;
>> +			}
>> +		}
>> +
>> +		if (new_device) {
>> +			ret = i3c_master_get_free_addr(&gi3c->ctrlr, last_dyn_addr + 1);
>> +			if (ret < 0)
>> +				break;
>> +			addr = (u8)ret;
>> +			last_dyn_addr = (u8)ret;
> 
> nit: while logically the same, last_dyn_addr = addr would make sense here
> 
Sure, Done.
>> +			set_new_addr_slot(gi3c->newaddrslots, addr);
>> +		}
>> +
> 
> suppose addr=0x38
> 
>> +		tx_buf[0] = (addr & I3C_ADDR_MASK) << 1;
> 
> tx_buf[0] = (0x38 & 0x7f) << 1 = 0x38<<1 = 0x70 = 0b1110000
> 
>> +		tx_buf[0] |= ~(hweight8(addr & I3C_ADDR_MASK) & 1);
> 
> 0x70 | ~(hweight8(0x70 & 0x7f) & 1) = 0x70 | ~(3 & 1) = 0x70 | ~BIT(0) = 0xfe
> 
> is that the intended result?
> 
Yes, thats right.
It can have either 0xfe OR 0xff.

Mainly for error detection purpose. This parity bit in tx_buf[0] is set 
correctly based on nos set bits in the Masked addr is odd or even.
I have simplified it using parity8().
>> +
>> +		xfer.m_cmd = I2C_WRITE;
>> +		xfer.m_param = STOP_STRETCH | BYPASS_ADDR_PHASE | USE_7E;
>> +
>> +		ret = i3c_geni_execute_write_command(gi3c, &xfer, tx_buf, 1);
>> +		if (ret)
>> +			break;
>> +	}
>> +}
>> +
>> +static int geni_i3c_master_send_ccc_cmd(struct i3c_master_controller *m,
>> +					struct i3c_ccc_cmd *cmd)
>> +{
>> +	struct geni_i3c_dev *gi3c = to_geni_i3c_master(m);
>> +	int i, ret;
>> +
>> +	if (!(cmd->id & I3C_CCC_DIRECT) && cmd->ndests != 1)
>> +		return -EINVAL;
>> +
>> +	ret = i3c_geni_runtime_get_mutex_lock(gi3c);
>> +	if (ret)
>> +		return ret;
>> +
>> +	qcom_geni_i3c_conf(gi3c, OPEN_DRAIN_MODE);
>> +	for (i = 0; i < cmd->ndests; i++) {
>> +		int stall = (i < (cmd->ndests - 1)) ||
>> +			(cmd->id == I3C_CCC_ENTDAA);
> 
> bool
> 
Sorry, Didn't get it where to keep bool ?
> Konrad

Powered by blists - more mailing lists