Re: [PATCH v3 2/3] i2c: add support for HiSilicon I2C controller

[Dmitry Osipenko <digetx@gmail.com>]

24.03.2021 12:30, Yicong Yang пишет:
...
>>> +static void hisi_i2c_enable_int(struct hisi_i2c_controller *ctlr, u32 mask)
>>> +{
>>> +	writel(mask, ctlr->iobase + HISI_I2C_INT_MASK);
>>
>> Why you don't use relaxed versions of readl/writel? Do you really need
>> to insert memory barriers?
>>
> 
> this will not be used during the transfer process, so a relaxed version of readl/writel
> will not have performance enhancement.
> 
> the barriers are necessary as i want to make the operations in order to avoid potential
> problems caused by reordering.

The iomap is strongly ordered, hence register accesses are always
ordered. The barrier ensures that CPU memory accesses are finished
before h/w registers are touched. Looks like you don't need to worry
about the memory barrier in the case of this driver.

>>> +}
>>> +
>>> +static void hisi_i2c_disable_int(struct hisi_i2c_controller *ctlr, u32 mask)
>>> +{
>>> +	writel((~mask) & HISI_I2C_INT_ALL, ctlr->iobase + HISI_I2C_INT_MASK);
>>> +}
>>> +
>>> +static void hisi_i2c_clear_int(struct hisi_i2c_controller *ctlr, u32 mask)
>>> +{
>>> +	writel(mask, ctlr->iobase + HISI_I2C_INT_CLR);
>>> +}
>>> +
>>> +static void hisi_i2c_handle_errors(struct hisi_i2c_controller *ctlr)
>>> +{
>>> +	u32 int_err = ctlr->xfer_err, reg;
>>> +
>>> +	if (int_err & HISI_I2C_INT_FIFO_ERR) {
>>> +		reg = readl(ctlr->iobase + HISI_I2C_FIFO_STATE);
>>> +
>>> +		if (reg & HISI_I2C_FIFO_STATE_RX_RERR)
>>> +			dev_err(ctlr->dev, "rx fifo error read.\n");
>>
>> The dot "." in the end of error messages is unnecessary.
>>
> 
> i'd like to keep this, as i think this is rather driver specific and not
> violating any rules.

The common kernel style is *not* to have the dot + some other messages
in this driver already don't have it. Should be better if you could
remove it.

>>> +/*
>>> + * Initialize the transfer information and start the I2C bus transfer.
>>> + * We only configure the transfer and do some pre/post works here, and
>>> + * wait for the transfer done. The major transfer process is performed
>>> + * in the IRQ handler.
>>> + */
>>> +static int hisi_i2c_master_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
>>> +				int num)
>>> +{
>>> +	struct hisi_i2c_controller *ctlr = i2c_get_adapdata(adap);
>>> +	DECLARE_COMPLETION_ONSTACK(done);
>>> +	int ret = num;
>>> +
>>> +	hisi_i2c_reset_xfer(ctlr);
>>> +	ctlr->completion = &done;
>>> +	ctlr->msg_num = num;
>>> +	ctlr->msgs = msgs;
>>> +
>>> +	hisi_i2c_start_xfer(ctlr);
>>> +
>>> +	if (!wait_for_completion_timeout(ctlr->completion, adap->timeout)) {
>>> +		hisi_i2c_disable_int(ctlr, HISI_I2C_INT_ALL);
>>
>> This doesn't save you from racing with the interrupt handler. It looks
>> like you need to enable/disable IRQ around the completion, similarly to
>> what NVIDIA Tegra I2C driver does.
>>
> 
> thanks for suggestion.
> 
> the hardware between tegra and this one is a little different as we don't provide
> a way to reinit the controller. so {synchronize,disable}_irq() after mask
> the interrupt here will avoid the race.

The disable/enable will be ideal, but synchronize should be good enough
as well.