[PATCH v2 2/5] lib: utils/i2c: Add generic I2C configuration library
Alexandre Ghiti
alexandre.ghiti at canonical.com
Wed Oct 20 01:19:43 PDT 2021
On Wed, Oct 20, 2021 at 8:41 AM Nikita Shubin <nikita.shubin at maquefel.me> wrote:
>
> Hello Alexandre!
>
> On Tue, 19 Oct 2021 14:04:28 +0200
> Alexandre Ghiti <alexandre.ghiti at canonical.com> wrote:
>
> > On Fri, Oct 15, 2021 at 3:20 PM Nikita Shubin
> > <nikita.shubin at maquefel.me> wrote:>
> > > From: Nikita Shubin <n.shubin at yadro.com>
> > >
> > > Helper library to keep track of registered I2C adapters,
> > > identified by dts offset, basic send/read functions and
> > > adapter configuration (enable, set dividers, etc...).
> > >
> > > Signed-off-by: Nikita Shubin <n.shubin at yadro.com>
> > > ---
> > > v1 -> v2:
> > > - switch to lists instead of array
> > > - rename send to write
> > > - use buffers instead of single byte
> > > - add single byte read/write
> > > ---
> > > include/sbi_utils/i2c/i2c.h | 99
> > > +++++++++++++++++++++++++++++++++++++ lib/utils/i2c/i2c.c |
> > > 85 +++++++++++++++++++++++++++++++ lib/utils/i2c/objects.mk | 10
> > > ++++ 3 files changed, 194 insertions(+)
> > > create mode 100644 include/sbi_utils/i2c/i2c.h
> > > create mode 100644 lib/utils/i2c/i2c.c
> > > create mode 100644 lib/utils/i2c/objects.mk
> > >
> > > diff --git a/include/sbi_utils/i2c/i2c.h
> > > b/include/sbi_utils/i2c/i2c.h new file mode 100644
> > > index 0000000..76cdb66
> > > --- /dev/null
> > > +++ b/include/sbi_utils/i2c/i2c.h
> > > @@ -0,0 +1,99 @@
> > > +/*
> > > + * SPDX-License-Identifier: BSD-2-Clause
> > > + *
> > > + * Copyright (c) 2021 YADRO
> > > + *
> > > + * Authors:
> > > + * Nikita Shubin <nshubin at yadro.com>
> > > + */
> > > +
> > > +#ifndef __I2C_H__
> > > +#define __I2C_H__
> > > +
> > > +#include <sbi/sbi_types.h>
> > > +#include <sbi/sbi_list.h>
> > > +
> > > +/** Representation of a I2C adapter */
> > > +struct i2c_adapter {
> > > + /** Pointer to I2C driver owning this I2C adapter */
> > > + void *driver;
> > > +
> > > + /** Unique ID of the I2C adapter assigned by the driver */
> > > + int id;
> > > +
> > > + /**
> > > + * Configure I2C adapter
> > > + *
> > > + * Enable, set dividers, etc...
> > > + *
> > > + * @return 0 on success and negative error code on failure
> > > + */
> > > + int (*configure)(struct i2c_adapter *ia);
> > > +
> > > + /**
> > > + * Send buffer to given address, register
> > > + *
> > > + * @return 0 on success and negative error code on failure
> > > + */
> > > + int (*smbus_write)(struct i2c_adapter *ia, uint8_t addr,
> > > uint8_t reg,
> > > + uint8_t *buffer, int len);
> >
> > Why did you rename those functions with smbus prefix? And I would use
> > size_t for the len argument.
>
> Cause, i think, actually specifying a register to read/write as a part
> of transaction is an smbus thing, as i stated in cover letter, raw
> read/write i2c don't use repeated start.
Ok!
>
> And may be size_t indeed.
>
> >
> > > +
> > > + /**
> > > + * Read buffer from given address, register
> > > + *
> > > + * @return 0 on success and negative error code on failure
> > > + */
> > > + int (*smbus_read)(struct i2c_adapter *ia, uint8_t addr,
> > > uint8_t reg,
> > > + uint8_t *buffer, int len);
> > > +
> > > + /** List */
> > > + struct sbi_dlist node;
> > > +};
> > > +
> > > +static inline struct i2c_adapter *to_i2c_adapter(struct sbi_dlist
> > > *node) +{
> > > + return container_of(node, struct i2c_adapter, node);
> > > +}
> > > +
> > > +/** Find a registered I2C adapter */
> > > +struct i2c_adapter *i2c_adapter_find(int id);
> > > +
> > > +/** Register I2C adapter */
> > > +int i2c_adapter_add(struct i2c_adapter *ia);
> > > +
> > > +/** Un-register I2C adapter */
> > > +void i2c_adapter_remove(struct i2c_adapter *ia);
> > > +
> > > +/** Configure I2C adapter prior to send/read */
> > > +int i2c_adapter_configure(struct i2c_adapter *ia);
> > > +
> > > +/** Write to device on I2C adapter bus */
> > > +int i2c_adapter_smbus_write(struct i2c_adapter *ia, uint8_t addr,
> > > + uint8_t reg, uint8_t *buffer, int len);
> > > +
> > > +/** Read from device on I2C adapter bus */
> > > +int i2c_adapter_smbus_read(struct i2c_adapter *ia, uint8_t addr,
> > > + uint8_t reg, uint8_t *buffer, int len);
> > > +
> > > +/** Write single byte from device on I2C adapter bus */
> > > +static inline int i2c_adapter_smbus_reg_write(struct i2c_adapter
> > > *ia,
> > > + uint8_t addr, uint8_t reg, uint8_t
> > > val) +{
> > > + return i2c_adapter_smbus_write(ia, addr, reg, &val, 1);
> > > +}
> > > +
> > > +/** Read single byte from device on I2C adapter bus */
> > > +static inline int i2c_adapter_smbus_reg_read(struct i2c_adapter
> > > *ia,
> > > + uint8_t addr, uint8_t reg, uint8_t
> > > *val) +{
> > > + uint8_t buf;
> > > + int ret = i2c_adapter_smbus_read(ia, addr, reg, &buf, 1);
> > > +
> > > + if (ret)
> > > + return ret;
> > > +
> > > + *val = buf;
> > > + return 0;
> > > +}
> >
> > Nice to add the helpers, though I'm not sure "reg_read" is the right
> > suffix as it may not be quite explicit: what about simply read_byte?
>
> I think byte_read/byte_write should be "raw" and omit the
> uint8_t reg and have naming like:
>
> i2c_adapter_byte_read
> i2c_adapter_byte_write
>
> Without smbus prefix, but that's a bit different story.
As I'm far from being an i2c/smbus expert, are we sure the value of a
register always holds in an uint8_t?
>
>
> >
> > > +
> > > +#endif
> > > diff --git a/lib/utils/i2c/i2c.c b/lib/utils/i2c/i2c.c
> > > new file mode 100644
> > > index 0000000..d23ac91
> > > --- /dev/null
> > > +++ b/lib/utils/i2c/i2c.c
> > > @@ -0,0 +1,85 @@
> > > +/*
> > > + * SPDX-License-Identifier: BSD-2-Clause
> > > + *
> > > + * Copyright (c) 2021 YADRO
> > > + *
> > > + * Authors:
> > > + * Nikita Shubin <nshubin at yadro.com>
> > > + *
> > > + * derivate: lib/utils/gpio/gpio.c
> > > + * Authors:
> > > + * Anup Patel <anup.patel at wdc.com>
> > > + */
> > > +
> > > +#include <sbi/sbi_error.h>
> > > +#include <sbi_utils/i2c/i2c.h>
> > > +
> > > +static SBI_LIST_HEAD(i2c_adapters_list);
> > > +
> > > +struct i2c_adapter *i2c_adapter_find(int id)
> > > +{
> > > + struct sbi_dlist *pos;
> > > +
> > > + sbi_list_for_each(pos, &(i2c_adapters_list)) {
> > > + struct i2c_adapter *adap = to_i2c_adapter(pos);
> > > +
> > > + if (adap->id == id)
> > > + return adap;
> > > + }
> > > +
> > > + return NULL;
> > > +}
> > > +
> > > +int i2c_adapter_add(struct i2c_adapter *ia)
> > > +{
> > > + if (!ia)
> > > + return SBI_EINVAL;
> > > +
> > > + if (i2c_adapter_find(ia->id))
> > > + return SBI_EALREADY;
> > > +
> > > + sbi_list_add(&(ia->node), &(i2c_adapters_list));
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +void i2c_adapter_remove(struct i2c_adapter *ia)
> > > +{
> > > + if (!ia)
> > > + return;
> > > +
> > > + sbi_list_del(&(ia->node));
> > > +}
> > > +
> > > +int i2c_adapter_configure(struct i2c_adapter *ia)
> > > +{
> > > + if (!ia)
> > > + return SBI_EINVAL;
> > > + if (!ia->configure)
> > > + return 0;
> > > +
> > > + return ia->configure(ia);
> > > +}
> > > +
> > > +int i2c_adapter_smbus_write(struct i2c_adapter *ia, uint8_t addr,
> > > + uint8_t reg, uint8_t *buffer, int len)
> > > +{
> > > + if (!ia)
> > > + return SBI_EINVAL;
> > > + if (!ia->smbus_write)
> > > + return SBI_ENOSYS;
> > > +
> > > + return ia->smbus_write(ia, addr, reg, buffer, len);
> > > +}
> > > +
> > > +
> > > +int i2c_adapter_smbus_read(struct i2c_adapter *ia, uint8_t addr,
> > > + uint8_t reg, uint8_t *buffer, int len)
> > > +{
> > > + if (!ia)
> > > + return SBI_EINVAL;
> > > + if (!ia->smbus_read)
> > > + return SBI_ENOSYS;
> > > +
> > > + return ia->smbus_read(ia, addr, reg, buffer, len);
> > > +}
> >
> > Again, I don't find those helpers really useful, IMO the adapter
> > should be hidden from the device, see my previous review. But you can
> > convince me we don't need it, I'm open to hearing what you think :)
>
> let's move this to 0 patch disucssion.
>
> >
> > Alex
> >
> >
> > > diff --git a/lib/utils/i2c/objects.mk b/lib/utils/i2c/objects.mk
> > > new file mode 100644
> > > index 0000000..16a70da
> > > --- /dev/null
> > > +++ b/lib/utils/i2c/objects.mk
> > > @@ -0,0 +1,10 @@
> > > +#
> > > +# SPDX-License-Identifier: BSD-2-Clause
> > > +#
> > > +# Copyright (c) 2021 YADRO
> > > +#
> > > +# Authors:
> > > +# Nikita Shubin <nshubin at yadro.com>
> > > +#
> > > +
> > > +libsbiutils-objs-y += i2c/i2c.o
> > > --
> > > 2.31.1
> > >
> > >
> > > --
> > > opensbi mailing list
> > > opensbi at lists.infradead.org
> > > http://lists.infradead.org/mailman/listinfo/opensbi
>
More information about the opensbi
mailing list