Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20220206115939.3091265-1-luca@lucaceresoli.net> <20220206115939.3091265-3-luca@lucaceresoli.net>
In-Reply-To: <20220206115939.3091265-3-luca@lucaceresoli.net>
From:   Andy Shevchenko <andy.shevchenko@gmail.com>
Date:   Tue, 8 Feb 2022 13:16:48 +0200
Message-ID: <CAHp75Vejw86kLUJfwXR_kUn+=UCaixbcy=epO8Foe=9S2LqXTQ@mail.gmail.com>
Subject: Re: [RFCv3 2/6] i2c: add I2C Address Translator (ATR) support
To:     Luca Ceresoli <luca@lucaceresoli.net>
Cc:     Linux Media Mailing List <linux-media@vger.kernel.org>,
        linux-i2c <linux-i2c@vger.kernel.org>,
        devicetree <devicetree@vger.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Rob Herring <robh+dt@kernel.org>,
        Mark Rutland <mark.rutland@arm.com>,
        Wolfram Sang <wsa@the-dreams.de>,
        Sakari Ailus <sakari.ailus@linux.intel.com>,
        Hans Verkuil <hverkuil-cisco@xs4all.nl>,
        Laurent Pinchart <laurent.pinchart@ideasonboard.com>,
        Kieran Bingham <kieran.bingham@ideasonboard.com>,
        Jacopo Mondi <jacopo@jmondi.org>,
        Vladimir Zapolskiy <vz@mleia.com>,
        Peter Rosin <peda@axentia.se>,
        Mauro Carvalho Chehab <mchehab@kernel.org>,
        Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>,
        Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Mon, Feb 7, 2022 at 7:55 PM Luca Ceresoli <luca@lucaceresoli.net> wrote:
>
> An ATR is a device that looks similar to an i2c-mux: it has an I2C
> slave "upstream" port and N master "downstream" ports, and forwards
> transactions from upstream to the appropriate downstream port. But is
> is different in that the forwarded transaction has a different slave
> address. The address used on the upstream bus is called the "alias"
> and is (potentially) different from the physical slave address of the
> downstream chip.
>
> Add a helper file (just like i2c-mux.c for a mux or switch) to allow
> implementing ATR features in a device driver. The helper takes care or
> adapter creation/destruction and translates addresses at each transaction.

Why I2C mux driver can't be updated to support this feature?

...

>  RFCv1 was implemented inside i2c-mux.c and added yet more complexity
>  there. RFCv2 creates a new file on its own, i2c-atr.c. Since many ATR
>  features are not in a MUX and vice versa, the overlapping is low. This was
>  almost a complete rewrite, but for the records here are the main
>  differences from the old implementation:

While this is from a code perspective, maybe i2c mux and this one can
still share some parts?

...

> +config I2C_ATR
> +       tristate "I2C Address Translator (ATR) support"
> +       help
> +         Enable support for I2C Address Translator (ATR) chips.
> +
> +         An ATR allows accessing multiple I2C busses from a single
> +         physical bus via address translation instead of bus selection as
> +         i2c-muxes do.

What would be the module name?

...

> +/**

Is this a kernel doc formatted documentation?
Haven't you got a warning?

> + * I2C Address Translator
> + *
> + * Copyright (c) 2019 Luca Ceresoli <luca@lucaceresoli.net>

2019,2022?

> + *
> + * An I2C Address Translator (ATR) is a device with an I2C slave parent
> + * ("upstream") port and N I2C master child ("downstream") ports, and
> + * forwards transactions from upstream to the appropriate downstream port
> + * with a modified slave address. The address used on the parent bus is
> + * called the "alias" and is (potentially) different from the physical
> + * slave address of the child bus. Address translation is done by the
> + * hardware.
> + *
> + * An ATR looks similar to an i2c-mux except:
> + * - the address on the parent and child busses can be different
> + * - there is normally no need to select the child port; the alias used on
> + *   the parent bus implies it
> + *
> + * The ATR functionality can be provided by a chip with many other
> + * features. This file provides a helper to implement an ATR within your
> + * driver.
> + *
> + * The ATR creates a new I2C "child" adapter on each child bus. Adding
> + * devices on the child bus ends up in invoking the driver code to select
> + * an available alias. Maintaining an appropriate pool of available aliases
> + * and picking one for each new device is up to the driver implementer. The
> + * ATR maintains an table of currently assigned alias and uses it to modify
> + * all I2C transactions directed to devices on the child buses.
> + *
> + * A typical example follows.
> + *
> + * Topology:
> + *
> + *                       Slave X @ 0x10
> + *               .-----.   |
> + *   .-----.     |     |---+---- B
> + *   | CPU |--A--| ATR |
> + *   `-----'     |     |---+---- C
> + *               `-----'   |
> + *                       Slave Y @ 0x10
> + *
> + * Alias table:
> + *
> + *   Client  Alias
> + *   -------------
> + *      X    0x20
> + *      Y    0x30
> + *
> + * Transaction:
> + *
> + *  - Slave X driver sends a transaction (on adapter B), slave address 0x10
> + *  - ATR driver rewrites messages with address 0x20, forwards to adapter A
> + *  - Physical I2C transaction on bus A, slave address 0x20
> + *  - ATR chip propagates transaction on bus B with address translated to 0x10
> + *  - Slave X chip replies on bus B
> + *  - ATR chip forwards reply on bus A
> + *  - ATR driver rewrites messages with address 0x10
> + *  - Slave X driver gets back the msgs[], with reply and address 0x10
> + *
> + * Usage:
> + *
> + *  1. In your driver (typically in the probe function) add an ATR by
> + *     calling i2c_atr_new() passing your attach/detach callbacks
> + *  2. When the attach callback is called pick an appropriate alias,
> + *     configure it in your chip and return the chosen alias in the
> + *     alias_id parameter
> + *  3. When the detach callback is called, deconfigure the alias from
> + *     your chip and put it back in the pool for later usage
> + *
> + * Originally based on i2c-mux.c
> + */

Shouldn't this comment be somewhere under Documentation/ ?

...

> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/of.h>
> +#include <linux/slab.h>


> +static int i2c_atr_map_msgs(struct i2c_atr_chan *chan,
> +                           struct i2c_msg msgs[], int num)

foo[] makes not much sense in the function parameter. *foo is what
will be used and it's explicit.

Can this be located on one line (similar question to make compact the
rest of the function declarations)?

> +

Redundant blank line.

...

> +       /* Ensure we have enough room to save the original addresses */
> +       if (unlikely(chan->orig_addrs_size < num)) {

> +               void *new_buf = kmalloc(num * sizeof(chan->orig_addrs[0]),
> +                                       GFP_KERNEL);

Use kmalloc_array()

> +               if (new_buf == NULL)
> +                       return -ENOMEM;
> +
> +               kfree(chan->orig_addrs);

Hmm... is it a reimplementation of krealloc_array()?

> +               chan->orig_addrs = new_buf;
> +               chan->orig_addrs_size = num;
> +       }

...

> +               if (c2a) {
> +                       msgs[i].addr = c2a->alias;
> +               } else {
> +                       dev_err(atr->dev, "client 0x%02x not mapped!\n",
> +                               msgs[i].addr);
> +                       return -ENXIO;
> +               }

'else' would be redundant if you switch to the traditional pattern,
i.e. check for errors first.

...

> +/*
> + * Restore all message address aliases with the original addresses.
> + *
> + * This function is internal for use in i2c_atr_master_xfer().
> + *
> + * @see i2c_atr_map_msgs()
> + */

Too sparse formatting of the comment. Can you make it compact?

...

> +       int ret = 0;

Unneeded assignment.

> +       /* Switch to the right atr port */
> +       if (atr->ops->select) {
> +               ret = atr->ops->select(atr, chan->chan_id);
> +               if (ret < 0)
> +                       goto out;
> +       }
> +
> +       /* Translate addresses */
> +       mutex_lock(&chan->orig_addrs_lock);
> +       ret = i2c_atr_map_msgs(chan, msgs, num);
> +       if (ret < 0) {

> +               mutex_unlock(&chan->orig_addrs_lock);
> +               goto out;

goto out_unlock_deselect;

> +       }
> +
> +       /* Perform the transfer */
> +       ret = i2c_transfer(parent, msgs, num);
> +
> +       /* Restore addresses */
> +       i2c_atr_unmap_msgs(chan, msgs, num);

out_unlock_deselct:

> +       mutex_unlock(&chan->orig_addrs_lock);

> +out:

out_deselect:

> +       if (atr->ops->deselect)
> +               atr->ops->deselect(atr, chan->chan_id);
> +
> +       return ret;
> +}

...

> +       int err = 0;

Be consistent with ret vs. err across the functions.

> +       if (atr->ops->select)
> +               err = atr->ops->select(atr, chan->chan_id);

> +       if (!err)

Perhaps

       int ret;

       ret = 0;
       if (atr->ops->select)
               ret = atr->ops->select(atr, chan->chan_id);
       if (ret)
               goto out_deselect;


> +               err = i2c_smbus_xfer(parent, c2a->alias, flags,
> +                                    read_write, command, size, data);

out_deselect:

> +       if (atr->ops->deselect)
> +               atr->ops->deselect(atr, chan->chan_id);
> +
> +       return err;
> +}

...

> +       int err = 0;

Same as above: naming, useless assignment.

...

> +       c2a = kzalloc(sizeof(struct i2c_atr_cli2alias_pair), GFP_KERNEL);

sizeof(*c2a)

> +       if (!c2a) {
> +               err = -ENOMEM;
> +               goto err_alloc;

Useless label, return directly.

> +       }

...

> +       c2a = i2c_atr_find_mapping_by_client(&chan->alias_list, client);
> +       if (c2a != NULL) {

if (c2a)

> +               list_del(&c2a->node);
> +               kfree(c2a);
> +       }

...

> +       char symlink_name[20];

Why 20? Do we have a predefined constant for that?


> +       if (dev->of_node) {

This check can be dropped, also please use device property and fwnode
APIs. No good of having OF-centric generic modules nowadays.

> +               struct device_node *atr_node;
> +               struct device_node *child;
> +               u32 reg;
> +
> +               atr_node = of_get_child_by_name(dev->of_node, "i2c-atr");

atr_node = device_get_named_child_node(...);

fwnode_for_each_child_node() {
}

> +               for_each_child_of_node(atr_node, child) {
> +                       err = of_property_read_u32(child, "reg", &reg);
> +                       if (err)
> +                               continue;
> +                       if (chan_id == reg)
> +                               break;
> +               }
> +
> +               chan->adap.dev.of_node = child;
> +               of_node_put(atr_node);
> +       }

On the second thought can you utilize the parser from I2C mux?

...

> +       WARN(sysfs_create_link(&chan->adap.dev.kobj, &dev->kobj, "atr_device"),
> +            "can't create symlink to atr device\n");
> +       snprintf(symlink_name, sizeof(symlink_name), "channel-%u", chan_id);
> +       WARN(sysfs_create_link(&dev->kobj, &chan->adap.dev.kobj, symlink_name),
> +            "can't create symlink for channel %u\n", chan_id);

Doesn't sysfs already has a warning when it's really needed?

...

> +       if (atr->adapter[chan_id] == NULL) {
> +               dev_err(dev, "Adapter %d does not exist\n", chan_id);

Noisy message. On freeing we usually don't issue such when we try to
free already freeed resource.

> +               return;
> +       }

...

> +       atr = devm_kzalloc(dev, sizeof(*atr)
> +                           + max_adapters * sizeof(atr->adapter[0]),
> +                           GFP_KERNEL);

Check overflow.h and use respective macro here.

> +       if (!atr)
> +               return ERR_PTR(-ENOMEM);

...

> +/**

It's not a kernel doc.

> + * drivers/i2c/i2c-atr.h -- I2C Address Translator

Please, no names of the files inside the files.

> + * Copyright (c) 2019 Luca Ceresoli <luca@lucaceresoli.net>

2019,2022 ?

> + * Based on i2c-mux.h
> + */

...

> +#ifdef __KERNEL__

Why?

...

> +#include <linux/i2c.h>
> +#include <linux/mutex.h>

Missed types.h

Missed struct device;

-- 
With Best Regards,
Andy Shevchenko