All of lore.kernel.org
 help / color / mirror / Atom feed
From: Wolfram Sang <wsa@the-dreams.de>
To: Arnd Bergmann <arnd@arndb.de>
Cc: linux-i2c@vger.kernel.org,
	"Niklas Söderlund" <niklas.soderlund+renesas@ragnatech.se>,
	linux-sh@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH] i2c: allow building emev2 without slave mode again
Date: Thu, 10 Dec 2015 14:34:46 +0100	[thread overview]
Message-ID: <20151210133446.GC1573@katana> (raw)
In-Reply-To: <2144222.LfzPx9mJ0p@wuerfel>

[-- Attachment #1: Type: text/plain, Size: 1960 bytes --]

On Thu, Dec 10, 2015 at 02:14:49PM +0100, Arnd Bergmann wrote:
> The emev2 driver stopped compiling in today's linux-next kernel:
> 
> drivers/i2c/busses/i2c-emev2.c: In function 'em_i2c_slave_irq':
> drivers/i2c/busses/i2c-emev2.c:233:23: error: storage size of 'event' isn't known
> drivers/i2c/busses/i2c-emev2.c:250:3: error: implicit declaration of function 'i2c_slave_event' [-Werror=implicit-function-declaration]
> drivers/i2c/busses/i2c-emev2.c:250:32: error: 'I2C_SLAVE_STOP' undeclared (first use in this function)
> 
> It works again if we enable CONFIG_I2C_SLAVE, but it seems wrong
> to add a dependency on that symbol:
> 
> * The symbol is user-selectable, but only one or two (including this
>   one) bus drivers actually implement it, and it makes no sense
>   if you don't have one of them.
> 
> * The other driver (R-Car) uses 'select I2C_SLAVE', which seems
>   reasonable in principle, but we should not do that on user
>   visible symbols.
> 
> * I2C slave mode could be implemented in a lot of other drivers
>   as an optional feature, but we shouldn't require enabling it
>   if we don't use it.
> 
> This changes the two drivers that provide I2C slave mode so they
> can again build if the slave mode being disabled. To do this, I
> move the definition of i2c_slave_event() and enum i2c_slave_event
> out of the #ifdef and instead make the assignment of the reg_slave
> and unreg_slave pointers optional in the bus drivers. The functions
> implementing the feature are unused in that case, so they get
> marked as __maybe_unused in order to still give compile-time
> coverage.

Thanks a lot! Making this clear and consistent was on my todo-list,
unfortunately below some other items.

Both drivers have quite orthogonal slave_irq routines. What do you think
about grouping this and the reg/unreg-calls together and compile them
conditionally on I2C_SLAVE? I think the code savings are worth it.


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

WARNING: multiple messages have this Message-ID (diff)
From: Wolfram Sang <wsa@the-dreams.de>
To: linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH] i2c: allow building emev2 without slave mode again
Date: Thu, 10 Dec 2015 13:34:46 +0000	[thread overview]
Message-ID: <20151210133446.GC1573@katana> (raw)
In-Reply-To: <2144222.LfzPx9mJ0p@wuerfel>

[-- Attachment #1: Type: text/plain, Size: 1960 bytes --]

On Thu, Dec 10, 2015 at 02:14:49PM +0100, Arnd Bergmann wrote:
> The emev2 driver stopped compiling in today's linux-next kernel:
> 
> drivers/i2c/busses/i2c-emev2.c: In function 'em_i2c_slave_irq':
> drivers/i2c/busses/i2c-emev2.c:233:23: error: storage size of 'event' isn't known
> drivers/i2c/busses/i2c-emev2.c:250:3: error: implicit declaration of function 'i2c_slave_event' [-Werror=implicit-function-declaration]
> drivers/i2c/busses/i2c-emev2.c:250:32: error: 'I2C_SLAVE_STOP' undeclared (first use in this function)
> 
> It works again if we enable CONFIG_I2C_SLAVE, but it seems wrong
> to add a dependency on that symbol:
> 
> * The symbol is user-selectable, but only one or two (including this
>   one) bus drivers actually implement it, and it makes no sense
>   if you don't have one of them.
> 
> * The other driver (R-Car) uses 'select I2C_SLAVE', which seems
>   reasonable in principle, but we should not do that on user
>   visible symbols.
> 
> * I2C slave mode could be implemented in a lot of other drivers
>   as an optional feature, but we shouldn't require enabling it
>   if we don't use it.
> 
> This changes the two drivers that provide I2C slave mode so they
> can again build if the slave mode being disabled. To do this, I
> move the definition of i2c_slave_event() and enum i2c_slave_event
> out of the #ifdef and instead make the assignment of the reg_slave
> and unreg_slave pointers optional in the bus drivers. The functions
> implementing the feature are unused in that case, so they get
> marked as __maybe_unused in order to still give compile-time
> coverage.

Thanks a lot! Making this clear and consistent was on my todo-list,
unfortunately below some other items.

Both drivers have quite orthogonal slave_irq routines. What do you think
about grouping this and the reg/unreg-calls together and compile them
conditionally on I2C_SLAVE? I think the code savings are worth it.


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

WARNING: multiple messages have this Message-ID (diff)
From: wsa@the-dreams.de (Wolfram Sang)
To: linux-arm-kernel@lists.infradead.org
Subject: [PATCH] i2c: allow building emev2 without slave mode again
Date: Thu, 10 Dec 2015 14:34:46 +0100	[thread overview]
Message-ID: <20151210133446.GC1573@katana> (raw)
In-Reply-To: <2144222.LfzPx9mJ0p@wuerfel>

On Thu, Dec 10, 2015 at 02:14:49PM +0100, Arnd Bergmann wrote:
> The emev2 driver stopped compiling in today's linux-next kernel:
> 
> drivers/i2c/busses/i2c-emev2.c: In function 'em_i2c_slave_irq':
> drivers/i2c/busses/i2c-emev2.c:233:23: error: storage size of 'event' isn't known
> drivers/i2c/busses/i2c-emev2.c:250:3: error: implicit declaration of function 'i2c_slave_event' [-Werror=implicit-function-declaration]
> drivers/i2c/busses/i2c-emev2.c:250:32: error: 'I2C_SLAVE_STOP' undeclared (first use in this function)
> 
> It works again if we enable CONFIG_I2C_SLAVE, but it seems wrong
> to add a dependency on that symbol:
> 
> * The symbol is user-selectable, but only one or two (including this
>   one) bus drivers actually implement it, and it makes no sense
>   if you don't have one of them.
> 
> * The other driver (R-Car) uses 'select I2C_SLAVE', which seems
>   reasonable in principle, but we should not do that on user
>   visible symbols.
> 
> * I2C slave mode could be implemented in a lot of other drivers
>   as an optional feature, but we shouldn't require enabling it
>   if we don't use it.
> 
> This changes the two drivers that provide I2C slave mode so they
> can again build if the slave mode being disabled. To do this, I
> move the definition of i2c_slave_event() and enum i2c_slave_event
> out of the #ifdef and instead make the assignment of the reg_slave
> and unreg_slave pointers optional in the bus drivers. The functions
> implementing the feature are unused in that case, so they get
> marked as __maybe_unused in order to still give compile-time
> coverage.

Thanks a lot! Making this clear and consistent was on my todo-list,
unfortunately below some other items.

Both drivers have quite orthogonal slave_irq routines. What do you think
about grouping this and the reg/unreg-calls together and compile them
conditionally on I2C_SLAVE? I think the code savings are worth it.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20151210/29670801/attachment.sig>

  reply	other threads:[~2015-12-10 13:34 UTC|newest]

Thread overview: 52+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2015-12-10 13:14 [PATCH] i2c: allow building emev2 without slave mode again Arnd Bergmann
2015-12-10 13:14 ` Arnd Bergmann
2015-12-10 13:14 ` Arnd Bergmann
2015-12-10 13:34 ` Wolfram Sang [this message]
2015-12-10 13:34   ` Wolfram Sang
2015-12-10 13:34   ` Wolfram Sang
2015-12-10 13:51   ` Arnd Bergmann
2015-12-10 13:51     ` Arnd Bergmann
2015-12-10 13:51     ` Arnd Bergmann
2015-12-10 13:51     ` Arnd Bergmann
2015-12-10 14:54 ` kbuild test robot
2015-12-10 14:54   ` kbuild test robot
2015-12-10 14:54   ` kbuild test robot
2015-12-10 15:06   ` Arnd Bergmann
2015-12-10 15:06     ` Arnd Bergmann
2015-12-10 15:06     ` Arnd Bergmann
2015-12-10 15:06     ` Arnd Bergmann
2015-12-10 15:17     ` Wolfram Sang
2015-12-10 15:17       ` Wolfram Sang
2015-12-10 15:17       ` Wolfram Sang
2015-12-12 16:20     ` Wolfram Sang
2015-12-12 16:20       ` Wolfram Sang
2015-12-12 16:20       ` Wolfram Sang
2015-12-12 21:05       ` Arnd Bergmann
2015-12-12 21:05         ` Arnd Bergmann
2015-12-12 21:05         ` Arnd Bergmann
2015-12-12 21:05         ` Arnd Bergmann
2015-12-13  9:09         ` Wolfram Sang
2015-12-13  9:09           ` Wolfram Sang
2015-12-13  9:09           ` Wolfram Sang
2015-12-14 12:02           ` Arnd Bergmann
2015-12-14 12:02             ` Arnd Bergmann
2015-12-14 12:02             ` Arnd Bergmann
2015-12-14 13:52             ` Wolfram Sang
2015-12-14 13:52               ` Wolfram Sang
2015-12-14 13:52               ` Wolfram Sang
2015-12-14 22:27               ` Arnd Bergmann
2015-12-14 22:27                 ` Arnd Bergmann
2015-12-14 22:27                 ` Arnd Bergmann
2015-12-17 12:01                 ` Wolfram Sang
2015-12-17 12:01                   ` Wolfram Sang
2015-12-17 12:01                   ` Wolfram Sang
2015-12-17 14:48                   ` Arnd Bergmann
2015-12-17 14:48                     ` Arnd Bergmann
2015-12-17 14:48                     ` Arnd Bergmann
2015-12-17 19:40                     ` Wolfram Sang
2015-12-17 19:40                       ` Wolfram Sang
2015-12-17 19:40                       ` Wolfram Sang
2015-12-17 19:57                       ` Arnd Bergmann
2015-12-17 19:57                         ` Arnd Bergmann
2015-12-17 19:57                         ` Arnd Bergmann
2015-12-17 19:57                         ` Arnd Bergmann

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20151210133446.GC1573@katana \
    --to=wsa@the-dreams.de \
    --cc=arnd@arndb.de \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-i2c@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-sh@vger.kernel.org \
    --cc=niklas.soderlund+renesas@ragnatech.se \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.