| Message ID | 20250212163359.2407327-2-andriy.shevchenko@linux.intel.com (mailing list archive) |
|---|---|
| State | Under Review |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | i2c: busses: Introduce and use i2c_10bit_addr_from_msg() | expand |
> From: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > Sent: 12 February 2025 16:32 > Subject: [PATCH v1 1/8] i2c: Introduce i2c_10bit_addr_from_msg() > > There are already a lot of drivers that have been using > i2c_8bit_addr_from_msg() for 7-bit addresses, now it's time > to have the similar for 10-bit addresses. > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> Reviewed-by: Fabrizio Castro <fabrizio.castro.jz@renesas.com> > --- > include/linux/i2c.h | 10 ++++++++++ > 1 file changed, 10 insertions(+) > > diff --git a/include/linux/i2c.h b/include/linux/i2c.h > index 997e80649889..4d281ff5582b 100644 > --- a/include/linux/i2c.h > +++ b/include/linux/i2c.h > @@ -952,6 +952,16 @@ static inline u8 i2c_8bit_addr_from_msg(const struct i2c_msg *msg) > return (msg->addr << 1) | (msg->flags & I2C_M_RD); > } > > +static inline u8 i2c_10bit_addr_from_msg(const struct i2c_msg *msg) > +{ > + /* > + * 10-bit address > + * addr_1: 5'b11110 | addr[9:8] | (R/nW) > + * addr_2: addr[7:0] > + */ > + return 0xf0 | ((msg->addr & GENMASK(9, 8)) >> 7) | (msg->flags & I2C_M_RD); > +} > + > u8 *i2c_get_dma_safe_msg_buf(struct i2c_msg *msg, unsigned int threshold); > void i2c_put_dma_safe_msg_buf(u8 *buf, struct i2c_msg *msg, bool xferred); > > -- > 2.45.1.3035.g276e886db78b
Hi Andy, On Wed, 12 Feb 2025 at 17:35, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > There are already a lot of drivers that have been using > i2c_8bit_addr_from_msg() for 7-bit addresses, now it's time > to have the similar for 10-bit addresses. > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> Thanks for your patch! > --- a/include/linux/i2c.h > +++ b/include/linux/i2c.h > @@ -952,6 +952,16 @@ static inline u8 i2c_8bit_addr_from_msg(const struct i2c_msg *msg) > return (msg->addr << 1) | (msg->flags & I2C_M_RD); > } > > +static inline u8 i2c_10bit_addr_from_msg(const struct i2c_msg *msg) Having never used 10-bit addressing myself, or even looked into it, it took me a while to understand what this helper really does... So this returns the high byte of the artificial 16-bit address that must be used to address a target that uses 10-bit addressing? Hence I think this should be renamed, to better match its purpose. > +{ > + /* > + * 10-bit address > + * addr_1: 5'b11110 | addr[9:8] | (R/nW) > + * addr_2: addr[7:0] I think the second comment line does not belong here, as this function doesn't care about that part. > + */ > + return 0xf0 | ((msg->addr & GENMASK(9, 8)) >> 7) | (msg->flags & I2C_M_RD); > +} Probably you also want to add a similar but much simpler helper to return the low byte? > + > u8 *i2c_get_dma_safe_msg_buf(struct i2c_msg *msg, unsigned int threshold); > void i2c_put_dma_safe_msg_buf(u8 *buf, struct i2c_msg *msg, bool xferred); > Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
On Wed, Feb 12, 2025 at 07:36:46PM +0100, Geert Uytterhoeven wrote: > On Wed, 12 Feb 2025 at 17:35, Andy Shevchenko > <andriy.shevchenko@linux.intel.com> wrote: > > There are already a lot of drivers that have been using > > i2c_8bit_addr_from_msg() for 7-bit addresses, now it's time > > to have the similar for 10-bit addresses. ... > > +static inline u8 i2c_10bit_addr_from_msg(const struct i2c_msg *msg) > > Having never used 10-bit addressing myself, or even looked into it, > it took me a while to understand what this helper really does... > So this returns the high byte of the artificial 16-bit address that > must be used to address a target that uses 10-bit addressing? > Hence I think this should be renamed, to better match its purpose. Since you are giving a constructive feedback, please, propose the name. > > +{ > > + /* > > + * 10-bit address > > + * addr_1: 5'b11110 | addr[9:8] | (R/nW) > > + * addr_2: addr[7:0] > > I think the second comment line does not belong here, as this function > doesn't care about that part. I think the comment is okay to stay. It explains the full picture which is helpful. It may be extended to say that the function returns only addr_1. > > + */ > > + return 0xf0 | ((msg->addr & GENMASK(9, 8)) >> 7) | (msg->flags & I2C_M_RD); > > +} > > Probably you also want to add a similar but much simpler helper to > return the low byte? Wouldn't it be too much?
Just a generic comment: please don't spend too much energy on 10-bit support. I have never seen it used in the wild. It seems more like an academic excercise.
On Thu, Feb 13, 2025 at 11:02:55AM +0100, Wolfram Sang wrote: > > Just a generic comment: please don't spend too much energy on 10-bit > support. I have never seen it used in the wild. It seems more like an > academic excercise. True, but still it makes sense to reduce the respective code base.
Hi Andy, On Wed, 12 Feb 2025 at 20:04, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > On Wed, Feb 12, 2025 at 07:36:46PM +0100, Geert Uytterhoeven wrote: > > On Wed, 12 Feb 2025 at 17:35, Andy Shevchenko > > <andriy.shevchenko@linux.intel.com> wrote: > > > There are already a lot of drivers that have been using > > > i2c_8bit_addr_from_msg() for 7-bit addresses, now it's time > > > to have the similar for 10-bit addresses. > > ... > > > > +static inline u8 i2c_10bit_addr_from_msg(const struct i2c_msg *msg) > > > > Having never used 10-bit addressing myself, or even looked into it, > > it took me a while to understand what this helper really does... > > So this returns the high byte of the artificial 16-bit address that > > must be used to address a target that uses 10-bit addressing? > > Hence I think this should be renamed, to better match its purpose. > > Since you are giving a constructive feedback, please, propose the name. i2c_10bit_addr_hi_from_msg()? > > > +{ > > > + /* > > > + * 10-bit address > > > + * addr_1: 5'b11110 | addr[9:8] | (R/nW) > > > + * addr_2: addr[7:0] > > > > I think the second comment line does not belong here, as this function > > doesn't care about that part. > > I think the comment is okay to stay. It explains the full picture which is > helpful. It may be extended to say that the function returns only addr_1. Or it could be moved outside this function, i.e. at the start of the section listing all 10-bit address helpers? > > > > + */ > > > + return 0xf0 | ((msg->addr & GENMASK(9, 8)) >> 7) | (msg->flags & I2C_M_RD); > > > +} > > > > Probably you also want to add a similar but much simpler helper to > > return the low byte? > > Wouldn't it be too much? I (my OCD ;-) love symmetry... i2c_10bit_addr_lo_from_msg()? Gr{oetje,eeting}s, Geert
On Thu, Feb 13, 2025 at 11:02:55AM +0100, Wolfram Sang wrote: > > Just a generic comment: please don't spend too much energy on 10-bit > support. I have never seen it used in the wild. It seems more like an > academic excercise. Just for the record, AFAICS nomadik, octeon-core, qup, and maybe others (that are not covered in the series) have bugs for 10-bit address mode. FWIW, I2C_M_TEN is mentioned in a dozen of drivers outside of i2c subsystem folder.
Hi Andy, On Wed, Feb 12, 2025 at 06:32:26PM +0200, Andy Shevchenko wrote: > There are already a lot of drivers that have been using > i2c_8bit_addr_from_msg() for 7-bit addresses, now it's time > to have the similar for 10-bit addresses. > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> ... > +static inline u8 i2c_10bit_addr_from_msg(const struct i2c_msg *msg) > +{ > + /* > + * 10-bit address > + * addr_1: 5'b11110 | addr[9:8] | (R/nW) > + * addr_2: addr[7:0] > + */ > + return 0xf0 | ((msg->addr & GENMASK(9, 8)) >> 7) | (msg->flags & I2C_M_RD); > +} > + I personally like this patch and it was an item of my todo list. I'm OK with having it merged. Reviewed-by: Andi Shyti <andi.shyti@kernel.org> Andi
Hi Geert, > > @@ -952,6 +952,16 @@ static inline u8 i2c_8bit_addr_from_msg(const struct i2c_msg *msg) > > return (msg->addr << 1) | (msg->flags & I2C_M_RD); > > } > > > > +static inline u8 i2c_10bit_addr_from_msg(const struct i2c_msg *msg) > > Having never used 10-bit addressing myself, or even looked into it, > it took me a while to understand what this helper really does... > So this returns the high byte of the artificial 16-bit address that > must be used to address a target that uses 10-bit addressing? > Hence I think this should be renamed, to better match its purpose. It's coherent with i2c_8bit_addr_from_msg(), right? Andi
Hi Andi, On Thu, 13 Feb 2025 at 23:41, Andi Shyti <andi.shyti@kernel.org> wrote: > > > @@ -952,6 +952,16 @@ static inline u8 i2c_8bit_addr_from_msg(const struct i2c_msg *msg) > > > return (msg->addr << 1) | (msg->flags & I2C_M_RD); > > > } > > > > > > +static inline u8 i2c_10bit_addr_from_msg(const struct i2c_msg *msg) > > > > Having never used 10-bit addressing myself, or even looked into it, > > it took me a while to understand what this helper really does... > > So this returns the high byte of the artificial 16-bit address that > > must be used to address a target that uses 10-bit addressing? > > Hence I think this should be renamed, to better match its purpose. > > It's coherent with i2c_8bit_addr_from_msg(), right? Is it? Unlike i2c_8bit_addr_from_msg(), it does not return the full address, Gr{oetje,eeting}s, Geert
On Fri, Feb 14, 2025 at 09:04:34AM +0100, Geert Uytterhoeven wrote: ... > > > > +static inline u8 i2c_10bit_addr_from_msg(const struct i2c_msg *msg) > > > > > > Having never used 10-bit addressing myself, or even looked into it, > > > it took me a while to understand what this helper really does... > > > So this returns the high byte of the artificial 16-bit address that > > > must be used to address a target that uses 10-bit addressing? > > > Hence I think this should be renamed, to better match its purpose. > > > > It's coherent with i2c_8bit_addr_from_msg(), right? > > Is it? Unlike i2c_8bit_addr_from_msg(), it does not return the full > address, Yeah, hi/lo together will be coherent, hence I sent a v2 with Geert's suggestion being incorporated.
diff --git a/include/linux/i2c.h b/include/linux/i2c.h index 997e80649889..4d281ff5582b 100644 --- a/include/linux/i2c.h +++ b/include/linux/i2c.h @@ -952,6 +952,16 @@ static inline u8 i2c_8bit_addr_from_msg(const struct i2c_msg *msg) return (msg->addr << 1) | (msg->flags & I2C_M_RD); } +static inline u8 i2c_10bit_addr_from_msg(const struct i2c_msg *msg) +{ + /* + * 10-bit address + * addr_1: 5'b11110 | addr[9:8] | (R/nW) + * addr_2: addr[7:0] + */ + return 0xf0 | ((msg->addr & GENMASK(9, 8)) >> 7) | (msg->flags & I2C_M_RD); +} + u8 *i2c_get_dma_safe_msg_buf(struct i2c_msg *msg, unsigned int threshold); void i2c_put_dma_safe_msg_buf(u8 *buf, struct i2c_msg *msg, bool xferred);
There are already a lot of drivers that have been using i2c_8bit_addr_from_msg() for 7-bit addresses, now it's time to have the similar for 10-bit addresses. Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> --- include/linux/i2c.h | 10 ++++++++++ 1 file changed, 10 insertions(+)