On Fri, Nov 1, 2013 at 5:49 AM, Wolfram Sang <wsa@xxxxxxxxxxxxx> wrote:
> On Wed, Oct 16, 2013 at 03:01:46PM -0700, Tim Kryger wrote:
>> Introduce support for Broadcom Serial Controller (BSC) I2C bus found
>> in the Kona family of Mobile SoCs. FIFO hardware is utilized but only
>> standard mode (100kHz), fast mode (400kHz), and fast mode plus (1MHz)
>> bus speeds are supported.
>>
>> Signed-off-by: Tim Kryger <tim.kryger@xxxxxxxxxx>
>> Reviewed-by: Matt Porter <matt.porter@xxxxxxxxxx>
>> Reviewed-by: Markus Mayer <markus.mayer@xxxxxxxxxx>
>
> Looks mostly good, some remarks:
>
>> +struct bcm_kona_i2c_dev {
>> + /* Pointer to linux device struct */
>> + struct device *device;
>> +
>> + /* Virtual address where registers are mapped */
>> + void __iomem *base;
>> +
>> + /* Interrupt */
>> + int irq;
>> +
>> + /* Standard Speed configuration */
>> + const struct bus_speed_cfg *std_cfg;
>> +
>> + /* Linux I2C adapter struct */
>> + struct i2c_adapter adapter;
>> +
>> + /* Lock for the I2C device */
>> + struct mutex i2c_bcm_lock;
>> +
>> + /* Completion to signal an operation finished */
>> + struct completion done;
>> +
>> + /* Handle for external clock */
>> + struct clk *external_clk;
>> +};
>
> IMO most of the comments could go. Kind of stating the obvious :)
Sure thing.
>
>> +/* Read any amount of data using the RX FIFO from the i2c bus */
>> +static int bcm_kona_i2c_read_fifo(struct bcm_kona_i2c_dev *dev,
>> + struct i2c_msg *msg)
>> +{
>> + unsigned int bytes_to_read = MAX_RX_FIFO_SIZE;
>> + unsigned int last_byte_nak = 0;
>> + unsigned int bytes_read = 0;
>> + unsigned int rc;
>
> Should be signed.
Absolutely. Thanks for catching this one.
>
>> +/* Write any amount of data using TX FIFO to the i2c bus */
>> +static int bcm_kona_i2c_write_fifo(struct bcm_kona_i2c_dev *dev,
>> + struct i2c_msg *msg)
>> +{
>> + unsigned int bytes_to_write = MAX_TX_FIFO_SIZE;
>> + unsigned int bytes_written = 0;
>> + unsigned int rc;
>
> Ditto signed.
I shall fix it.
>
>> +/* Master transfer function */
>> +static int bcm_kona_i2c_xfer(struct i2c_adapter *adapter,
>> + struct i2c_msg msgs[], int num)
>> +{
>> + struct bcm_kona_i2c_dev *dev = i2c_get_adapdata(adapter);
>> + struct i2c_msg *pmsg;
>> + int rc = 0;
>> + int i;
>> +
>> + mutex_lock(&dev->i2c_bcm_lock);
>
> Huh? Why do you need that? The core has locks per bus.
In that case, I will remove the local mutex.
>> +static int bcm_kona_i2c_assign_bus_speed(struct bcm_kona_i2c_dev *dev)
>> +{
>> + unsigned int bus_speed;
>> + int ret = of_property_read_u32(dev->device->of_node, "clock-frequency",
>> + &bus_speed);
>> + if (ret < 0) {
>> + dev_err(dev->device, "missing clock-frequency property\n");
>> + return -ENODEV;
>> + }
>> +
>> + switch (bus_speed) {
>> + case 100000:
>> + dev->std_cfg = &std_cfg_table[BCM_SPD_100K];
>> + break;
>> + case 400000:
>> + dev->std_cfg = &std_cfg_table[BCM_SPD_400K];
>> + break;
>> + case 1000000:
>> + dev->std_cfg = &std_cfg_table[BCM_SPD_1MHZ];
>> + break;
>> + default:
>> + pr_err("%d hz bus speed not supported\n", bus_speed);
>> + return -EINVAL;
>> + };
>
> Unneeded semicolon.
>
Sure. It will be removed.
>> +
>> + return 0;
>> +}
>> +
>> +static int bcm_kona_i2c_probe(struct platform_device *pdev)
>> +{
>> + int rc = 0;
>> + struct bcm_kona_i2c_dev *dev;
>> + struct i2c_adapter *adap;
>> + struct resource *iomem;
>> +
>> + /* Allocate memory for private data structure */
>> + dev = devm_kzalloc(&pdev->dev, sizeof(*dev), GFP_KERNEL);
>> + if (!dev) {
>> + rc = -ENOMEM;
>> + goto probe_return;
>
> I'd prefer simply return -Esomething. The printout at probe_return is
> also available via driver core.
Agreed.
> ...
>
>> + /* Add the i2c adapter */
>> + adap = &dev->adapter;
>> + i2c_set_adapdata(adap, dev);
>> + adap->owner = THIS_MODULE;
>> + adap->class = UINT_MAX; /* can be used by any I2C device */
>
> Why do you need class based instantiation. It will most likely cost
> boot-time and you have devicetree means for doing instantiation.
>
>
Agreed.
>> + strlcpy(adap->name, "Broadcom I2C adapter", sizeof(adap->name));
>> + adap->algo = &bcm_algo;
>> + adap->dev.parent = &pdev->dev;
>> + adap->nr = pdev->id;
>> + adap->dev.of_node = pdev->dev.of_node;
>> +
>> + rc = i2c_add_numbered_adapter(adap);
>
> Maybe you want simply i2c_add_adapter here since the core has no
> built-in support for of aliases?
>
Okay. I will switch to i2c_add_adapter and remove the adap->nr assignment.
>> +
>> +MODULE_AUTHOR("Broadcom");
>
> Some email address please.
>
>> +MODULE_DESCRIPTION("Broadcom Kona I2C Driver");
>> +MODULE_LICENSE("GPL v2");
>
> Thanks,
>
> Wolfram
>
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
