lkml.org 
[lkml]   [2018]   [May]   [30]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v7 3/7] drivers/i2c: Add port structure to FSI algorithm
From
Date


On 05/29/2018 06:19 PM, Andy Shevchenko wrote:
> On Wed, May 30, 2018 at 1:24 AM, Eddie James <eajames@linux.vnet.ibm.com> wrote:
>> From: "Edward A. James" <eajames@us.ibm.com>
>>
>> Add and initialize I2C adapters for each port on the FSI-attached I2C
>> master. Ports for each master are defined in the devicetree.
>> +#include <linux/of.h>
>
>> +static int fsi_i2c_set_port(struct fsi_i2c_port *port)
>> +{
>> + int rc;
>> + struct fsi_device *fsi = port->master->fsi;
>> + u32 mode, dummy = 0;
>> + u16 old_port;
>> +
>> + rc = fsi_i2c_read_reg(fsi, I2C_FSI_MODE, &mode);
>> + if (rc)
>> + return rc;
>> +
>> + old_port = GETFIELD(I2C_MODE_PORT, mode);
>> +
>> + if (old_port != port->port) {
> Why not simple
>
> if (port->port == GETFIELD())
> return 0;
>
> ?
>
>> + /* reset engine when port is changed */
>> + rc = fsi_i2c_write_reg(fsi, I2C_FSI_RESET_ERR, &dummy);
>> + if (rc)
>> + return rc;
>> + }
>> + return rc;
> It's hardly would be non-zero, right?

Sorry, misunderstood your comment here. You are correct, it can only be
zero.

Thanks,
Eddie

>
>> +}
>> static int fsi_i2c_probe(struct device *dev)
>> {
> Isn't below somehow repeats of_i2c_register_devices() ?
> Why not to use it?
>
>> + /* Add adapter for each i2c port of the master. */
>> + for_each_available_child_of_node(dev->of_node, np) {
>> + rc = of_property_read_u32(np, "reg", &port_no);
>> + if (rc || port_no > USHRT_MAX)
>> + continue;
>> +
>> + port = devm_kzalloc(dev, sizeof(*port), GFP_KERNEL);
>> + if (!port)
>> + break;
>> +
>> + port->master = i2c;
>> + port->port = port_no;
>> +
>> + port->adapter.owner = THIS_MODULE;
>> + port->adapter.dev.of_node = np;
>> + port->adapter.dev.parent = dev;
>> + port->adapter.algo = &fsi_i2c_algorithm;
>> + port->adapter.algo_data = port;
>> +
>> + snprintf(port->adapter.name, sizeof(port->adapter.name),
>> + "i2c_bus-%u", port_no);
>> +
>> + rc = i2c_add_adapter(&port->adapter);
>> + if (rc < 0) {
>> + dev_err(dev, "Failed to register adapter: %d\n", rc);
>> + devm_kfree(dev, port);
> This hurts my eyes. Why?!
>
>> + continue;
>> + }
>> +
>> + list_add(&port->list, &i2c->ports);
>> + }
>> +
>> dev_set_drvdata(dev, i2c);
>>
>> return 0;
>> }
>> + if (!list_empty(&i2c->ports)) {
> My gosh, this is done already in list_for_each*()
>
>> + list_for_each_entry(port, &i2c->ports, list) {
>> + i2c_del_adapter(&port->adapter);
>> + }
>> + }
>
>

\
 
 \ /
  Last update: 2018-05-30 17:50    [W:1.129 / U:0.228 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site