On Tue, Dec 13, 2022 at 04:26:57PM +0800, Binbin Zhou wrote:
> This I2C module is integrated into the Loongson-2K SoCs and Loongson
> LS7A bridge chip.
...
> +// SPDX-License-Identifier: GPL-2.0
GPL-2.0-only
> +/*
> + * Loongson-2K/Loongson LS7A I2C master mode driver
> + *
> + * Copyright (C) 2013 Loongson Technology Corporation Limited.
> + * Copyright (C) 2014-2017 Lemote, Inc.
> + * Copyright (C) 2018-2022 Loongson Technology Corporation Limited.
> + *
> + * Originally written by liushaozong
> + *
No need to have this blank line.
> + * Rewritten for mainline by Binbin Zhou <zhoubinbin@xxxxxxxxxxx>
> + */
...
> +#include <linux/bits.h>
> +#include <linux/completion.h>
> +#include <linux/device.h>
> +#include <linux/i2c.h>
> +#include <linux/init.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/platform_device.h>
+ property.h at least for dev_fwnode().
> +#include <linux/units.h>
...
> +#define I2C_LS2X_PRER_LO 0x0 /* Freq Division Low Byte Register */
> +#define I2C_LS2X_PRER_HI 0x1 /* Freq Division High Byte Register */
_HI is not used, can we just drop the suffix?
...
> +struct ls2x_i2c_priv {
> + struct i2c_adapter adapter;
> + struct device *dev;
In some cases you are using adapter.dev, in some this one.
Also isn't it the dev is the same as adapter.dev.parent?
Hence, why do you need this one?
> + void __iomem *base;
> + struct i2c_timings i2c_t;
> + struct completion cmd_complete;
> +};
...
> + return ls2x_i2c_send_byte(adap, (LS2X_CR_START | LS2X_CR_WRITE));
Too many parentheses.
...
> +static int ls2x_i2c_xfer_one(struct i2c_adapter *adap,
> + struct i2c_msg *msg, bool stop)
> +{
> + int ret;
> + bool is_read = msg->flags & I2C_M_RD;
> +
> + /* Contains steps to send start condition and address */
> + ret = ls2x_i2c_start(adap, msg);
> + if (ret)
> + return ret;
> +
> + if (is_read)
> + ret = ls2x_i2c_rx(adap, msg->buf, msg->len);
> + else
> + ret = ls2x_i2c_tx(adap, msg->buf, msg->len);
> +
> + /* could not acquire bus. bail out without STOP */
> + if (ret == -EAGAIN)
> + return ret;
So, if ret is *not* 0 and *not* --EAGAIN, why don't we bail out here? It needs
at least a comment.
> + if (stop)
> + ret = ls2x_i2c_stop(adap);
> +
> + return ret;
> +}
...
> +static int ls2x_i2c_master_xfer(struct i2c_adapter *adap,
> + struct i2c_msg *msgs, int num)
> +{
> + int ret;
> + struct i2c_msg *msg, *emsg = msgs + num;
> +
> + for (msg = msgs; msg < emsg; msg++) {
> + /* Emit STOP if it is the last message or I2C_M_STOP is set */
> + bool stop = (msg + 1 == emsg) || (msg->flags & I2C_M_STOP);
I'm wondering if we can always set the _STOP bit in the flags of the last
message before entering this loop. In such case you can reduce to one part
and supply as a parameter directly.
> +
> + ret = ls2x_i2c_xfer_one(adap, msg, stop);
> + if (ret)
> + return ret;
> + }
> +
> + return num;
> +}
...
> + /* Calculate and set LS2X I2C frequency */
> + writel(LS2X_I2C_PCLK_FREQ / (5 * t->bus_freq_hz) - 1,
> + priv->base + I2C_LS2X_PRER_LO);
writel()?! Shouldn't be writew()?
...
> + r = devm_request_irq(dev, irq, ls2x_i2c_isr,
> + IRQF_SHARED, "ls2x-i2c", priv);
There is a room on the previous line for at least one more argument.
> + if (r < 0)
> + return dev_err_probe(dev, r, "Unable to request irq %d\n", irq);
...
> +subsys_initcall(ls2x_i2c_init_driver);
Non-standard init calls should be commented.
--
With Best Regards,
Andy Shevchenko
