diff mbox

[RFC,v2] regmap: smbus: add support for regmap over SMBus

Message ID 1397480885-11962-1-git-send-email-boris.brezillon@free-electrons.com (mailing list archive)
State New, archived
Headers show

Commit Message

Boris BREZILLON April 14, 2014, 1:08 p.m. UTC
SMBus is a subset of the I2C protocol, oftenly used to access registers on
external devices.

I2C adapters are able to access SMBus devices thanks to the SMBus
emulation layer. In the other hand SMBus adapters may not provide
regular I2C transfers, and thus you may not be able to expose a regmap
if your device is connected to such kind of adapter.
Hence why we need this regmap over SMBus implementation.

Signed-off-by: Boris BREZILLON <boris.brezillon@free-electrons.com>
---
Hello Mark,

Sorry for the noise, but I forgot to add the changes in regmap.h in my
previous commit.

Best Regards,

Boris

 drivers/base/regmap/Kconfig        |   5 +-
 drivers/base/regmap/Makefile       |   1 +
 drivers/base/regmap/regmap-smbus.c | 364 +++++++++++++++++++++++++++++++++++++
 include/linux/regmap.h             |  13 ++
 4 files changed, 382 insertions(+), 1 deletion(-)
 create mode 100644 drivers/base/regmap/regmap-smbus.c

Comments

Mark Brown April 14, 2014, 9:04 p.m. UTC | #1
On Mon, Apr 14, 2014 at 03:08:05PM +0200, Boris BREZILLON wrote:
> SMBus is a subset of the I2C protocol, oftenly used to access registers on
> external devices.

This is basically fine.  However...

> +	switch (ctx->transfer_type) {
> +	case REGMAP_SMBUS_BYTE_TRANSFER:
> +		while (count > 0 && !ret) {
> +			ret = i2c_smbus_write_byte_data(ctx->i2c, reg++,
> +							*(u8 *)data++);
> +
> +			count--;
> +		}
> +		break;

The transfer type gets set once per device at init time so why not just
parameterise based on val_bytes?

> +	case REGMAP_SMBUS_I2C_BLOCK_TRANSFER:
> +		while (count > 0 && !ret) {
> +			ret = i2c_smbus_write_i2c_block_data(ctx->i2c,
> +							     reg,
> +							     ctx->val_bytes,
> +							     (const u8 *)data);

Fix the const correctness of the API rather than casting.

> +			reg += ctx->val_bytes;
> +			count -= ctx->val_bytes;
> +			data += ctx->val_bytes;
> +		}

I'm assuming this will only be used if val_bytes isn't 1 or 2?
Boris BREZILLON April 15, 2014, 7:36 a.m. UTC | #2
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1


On 14/04/2014 23:04, Mark Brown wrote:
> On Mon, Apr 14, 2014 at 03:08:05PM +0200, Boris BREZILLON wrote:
>> SMBus is a subset of the I2C protocol, oftenly used to access
registers on
>> external devices.
> 
> This is basically fine.  However...
> 
>> +    switch (ctx->transfer_type) { +    case
>> REGMAP_SMBUS_BYTE_TRANSFER: +        while (count > 0 && !ret) { 
>> +            ret = i2c_smbus_write_byte_data(ctx->i2c, reg++, +
>> *(u8 *)data++); + +            count--; +        } +
>> break;
> 
> The transfer type gets set once per device at init time so why not
> just parameterise based on val_bytes?

Actually, you may want to transfer 1 byte registers using the block
method (if your device only support block transfers). This depends on
the device being accessed and what it supports, but I'm not sure we can
assume 1 byte registers will always be transferred using SMBUS byte
transfers.

> 
> 
>> +    case REGMAP_SMBUS_I2C_BLOCK_TRANSFER: +        while (count
>> > 0 && !ret) { +            ret =
>> i2c_smbus_write_i2c_block_data(ctx->i2c, +
>> reg, +                                 ctx->val_bytes, +
>> (const u8 *)data);
> 
> Fix the const correctness of the API rather than casting.

The API is correct because the i2c_smbus_write_i2c_block does not modify
the data pointer.
Just removing the const keyword in the cast should be enough, because
you can safely cast a non const pointer to a const one.

> 
> 
>> +            reg += ctx->val_bytes; +            count -=
>> ctx->val_bytes; +            data += ctx->val_bytes; +        }
> 
> I'm assuming this will only be used if val_bytes isn't 1 or 2?

As explained earlier, this depends on the device being accessed, but it
might be used for smaller transfers (1 or 2 bytes).

Thanks for the review.

Best Regards,

Boris

- -- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.14 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQEcBAEBAgAGBQJTTOFkAAoJEHLimErDqMKyhlcH/jeKnphnxL9Sc+OKsd1sbvZ4
dgjwm0HVy/oQO4dwGHbGXDrvoqdcuPKjKJv0An1RAW0u8CBQHCHbn2xQZtHkrNhK
C4g+8ws3R050ppDAAvFGgnx2bIoS8+Elbcd0GNCQW+xYU/xOySH9qgPitFF7PgGw
X9wgM+o4x6W2Lo/NOxmkXeZVWl3UrfUL24UTXfqysJK6ciYtW7GDP1uInfK2eH1G
RL7eD7LanEH95bk8eQtVfZZzcoAZCo1ri2z6iPf2WS4Nep84fyAr6Q+B1Ltmhsus
QXZgXWnVY45ZYFRNjMX8axCUUoJf7DSn9ayi3Gil1i7fLmCPKhpxc8HNxWvIMkk=
=FFGW
-----END PGP SIGNATURE-----
Mark Brown April 15, 2014, 10:09 a.m. UTC | #3
On Tue, Apr 15, 2014 at 09:36:13AM +0200, Boris BREZILLON wrote:
> On 14/04/2014 23:04, Mark Brown wrote:

> > The transfer type gets set once per device at init time so why not
> > just parameterise based on val_bytes?

> Actually, you may want to transfer 1 byte registers using the block
> method (if your device only support block transfers). This depends on
> the device being accessed and what it supports, but I'm not sure we can
> assume 1 byte registers will always be transferred using SMBUS byte
> transfers.

OK, so if this a realistic issue then it seems like it's better to
implement three different buses - there is not really any common code
between the various paths.  This would also mean that you avoid having
gather write when it can't be implemented.

The code is also not validating the lengths for two byte values.

> >> +    case REGMAP_SMBUS_I2C_BLOCK_TRANSFER: +        while (count
> >> > 0 && !ret) { +            ret =
> >> i2c_smbus_write_i2c_block_data(ctx->i2c, +
> >> reg, +                                 ctx->val_bytes, +
> >> (const u8 *)data);

> > Fix the const correctness of the API rather than casting.

> The API is correct because the i2c_smbus_write_i2c_block does not modify
> the data pointer.
> Just removing the const keyword in the cast should be enough, because
> you can safely cast a non const pointer to a const one.

If you need to cast away from void at all something is going wrong (and
we appear to be always passing in write buffers as const too, I can't
remember which operation this was though).
Boris BREZILLON April 15, 2014, 11:54 a.m. UTC | #4
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1


On 15/04/2014 12:09, Mark Brown wrote:
> On Tue, Apr 15, 2014 at 09:36:13AM +0200, Boris BREZILLON wrote:
>> On 14/04/2014 23:04, Mark Brown wrote:
>
>>> The transfer type gets set once per device at init time so why not
>>> just parameterise based on val_bytes?
>
>> Actually, you may want to transfer 1 byte registers using the block
>> method (if your device only support block transfers). This depends on
>> the device being accessed and what it supports, but I'm not sure we can
>> assume 1 byte registers will always be transferred using SMBUS byte
>> transfers.
>
> OK, so if this a realistic issue then it seems like it's better to
> implement three different buses - there is not really any common code
> between the various paths.

Okay, I'll create 4 different busses (one for each access type).
BTW, should I keep these implementations in the same source file
(regmap-smbus.c) ?
And, should I keep one method to register an smbus regmap or should I
provide one method per access type and get rid of the
regmap_smbus_transfer_type enum ?

>   This would also mean that you avoid having
> gather write when it can't be implemented.

Correct me if I'm wrong, but they all support gather write.

>
>
> The code is also not validating the lengths for two byte values.

I'm not sure I get this one.
Do you mean I should check that val_size is a 2 byte multiple ?
If this is what you meant, then I should also check it for block transfers.

>
>
>>>> +    case REGMAP_SMBUS_I2C_BLOCK_TRANSFER: +        while (count
>>>>> 0 && !ret) { +            ret =
>>>> i2c_smbus_write_i2c_block_data(ctx->i2c, +
>>>> reg, +                                 ctx->val_bytes, +
>>>> (const u8 *)data);
>
>>> Fix the const correctness of the API rather than casting.
>
>> The API is correct because the i2c_smbus_write_i2c_block does not modify
>> the data pointer.
>> Just removing the const keyword in the cast should be enough, because
>> you can safely cast a non const pointer to a const one.
>
> If you need to cast away from void at all something is going wrong (and
> we appear to be always passing in write buffers as const too, I can't
> remember which operation this was though).

You're right, removing the cast when passing the val argument to the i2c
block transfer functions works.

Best Regards,

Boris

- -- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.14 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQEcBAEBAgAGBQJTTR3aAAoJEHLimErDqMKy3w0H/07ZyPXLB5YB3irMT0+eOqtS
spxzeZDv3OKT2Rggsz7wAjkRhFbFT9rAMtGJyrmo2js7CPh5cXEg+oWWeTeLpqjF
xeCyJW6yoihI0JbF2fsYlkuLqGIKrFYTeGzSWgP1DlLdxwJ//lxqQrayOMzd5exS
KbQglfzUEVJ5W5n65CGjmNBYW2/QWasAQGwzgypv3gWVLfQzd+n/vCnHBvn81bfe
Ry8nTMEOLpV3rJSGiNZZQL1TIsDiAUQuxKh+n4mc2ntIgLZcb2l6mjGA/36ziQaA
Gfpc1zTGTQWaY4ZdQuvljAEOl2yBUZrkuf+ZVrv26l6saIQv9ekxSCmVRP/CmbY=
=Bt4h
-----END PGP SIGNATURE-----
Mark Brown April 15, 2014, 12:10 p.m. UTC | #5
On Tue, Apr 15, 2014 at 01:54:02PM +0200, Boris BREZILLON wrote:
> On 15/04/2014 12:09, Mark Brown wrote:

> > OK, so if this a realistic issue then it seems like it's better to
> > implement three different buses - there is not really any common code
> > between the various paths.

> Okay, I'll create 4 different busses (one for each access type).
> BTW, should I keep these implementations in the same source file
> (regmap-smbus.c) ?
> And, should I keep one method to register an smbus regmap or should I
> provide one method per access type and get rid of the
> regmap_smbus_transfer_type enum ?

Do something that looks tasteful in implementation.

> >   This would also mean that you avoid having
> > gather write when it can't be implemented.

> Correct me if I'm wrong, but they all support gather write.

Everything except for the block transfers is writing one register at a
time, actually looking again everything that can only write one register
at a time ought not to be implementing the buffer based stuff at all and
should instead be hooked into reg_read() and reg_write() - it's not able
to implement the API properly in any of the cases, it's unpacking the
buffer which the upper level code already supports.

> > The code is also not validating the lengths for two byte values.

> I'm not sure I get this one.
> Do you mean I should check that val_size is a 2 byte multiple ?
> If this is what you meant, then I should also check it for block transfers.

Yes.
Lars-Peter Clausen April 15, 2014, 12:25 p.m. UTC | #6
On 04/15/2014 01:54 PM, Boris BREZILLON wrote:
>
>
> On 15/04/2014 12:09, Mark Brown wrote:
>> On Tue, Apr 15, 2014 at 09:36:13AM +0200, Boris BREZILLON wrote:
>>> On 14/04/2014 23:04, Mark Brown wrote:
>
>>>> The transfer type gets set once per device at init time so why not
>>>> just parameterise based on val_bytes?
>
>>> Actually, you may want to transfer 1 byte registers using the block
>>> method (if your device only support block transfers). This depends on
>>> the device being accessed and what it supports, but I'm not sure we can
>>> assume 1 byte registers will always be transferred using SMBUS byte
>>> transfers.
>
>> OK, so if this a realistic issue then it seems like it's better to
>> implement three different buses - there is not really any common code
>> between the various paths.
>
> Okay, I'll create 4 different busses (one for each access type).
> BTW, should I keep these implementations in the same source file
> (regmap-smbus.c) ?
> And, should I keep one method to register an smbus regmap or should I
> provide one method per access type and get rid of the
> regmap_smbus_transfer_type enum ?

I don't think we should leave the decision which bus to use to the driver. 
Neither should the driver have to choose whether to use smbus or native I2C. 
We want to use native I2C when available, because it is more efficient than 
going through the smbus emulation layer. On the other hand we want to 
automatically switch to smbus when native I2C is not available and the 
device can work fine with smbus. Also I'm afraid that we'll otherwise soon 
see code popping up like:

if (of_property_read_bool(np, "use-smbus-regmap"))
	regmap = regmap_init_smbus(...)
else
	regmap = regmap_init_i2c(...)

My suggestion is that in regmap_init_i2c() you check the capabilities of the 
I2C adapter. If it supports native I2C you setup the regmap with the 
regmap_i2c struct just as it does right now. If the adapter does not support 
native I2C, check if the device can be supported by smbus (reg_bytes == 8 && 
val_bytes % 8 == 0). For each type of smbus operations have one regmap_bus 
struct, and if you can fallback to smbus choose the bus depending on the 
config's val_bytes.

- Lars
Boris BREZILLON April 15, 2014, 12:40 p.m. UTC | #7
On 15/04/2014 14:25, Lars-Peter Clausen wrote:
> On 04/15/2014 01:54 PM, Boris BREZILLON wrote:
>>
>>
>> On 15/04/2014 12:09, Mark Brown wrote:
>>> On Tue, Apr 15, 2014 at 09:36:13AM +0200, Boris BREZILLON wrote:
>>>> On 14/04/2014 23:04, Mark Brown wrote:
>>
>>>>> The transfer type gets set once per device at init time so why not
>>>>> just parameterise based on val_bytes?
>>
>>>> Actually, you may want to transfer 1 byte registers using the block
>>>> method (if your device only support block transfers). This depends on
>>>> the device being accessed and what it supports, but I'm not sure we
>>>> can
>>>> assume 1 byte registers will always be transferred using SMBUS byte
>>>> transfers.
>>
>>> OK, so if this a realistic issue then it seems like it's better to
>>> implement three different buses - there is not really any common code
>>> between the various paths.
>>
>> Okay, I'll create 4 different busses (one for each access type).
>> BTW, should I keep these implementations in the same source file
>> (regmap-smbus.c) ?
>> And, should I keep one method to register an smbus regmap or should I
>> provide one method per access type and get rid of the
>> regmap_smbus_transfer_type enum ?
>
> I don't think we should leave the decision which bus to use to the
> driver. Neither should the driver have to choose whether to use smbus
> or native I2C. We want to use native I2C when available, because it is
> more efficient than going through the smbus emulation layer. On the
> other hand we want to automatically switch to smbus when native I2C is
> not available and the device can work fine with smbus. Also I'm afraid
> that we'll otherwise soon see code popping up like:
>
> if (of_property_read_bool(np, "use-smbus-regmap"))
>     regmap = regmap_init_smbus(...)
> else
>     regmap = regmap_init_i2c(...)
>
> My suggestion is that in regmap_init_i2c() you check the capabilities
> of the I2C adapter. If it supports native I2C you setup the regmap
> with the regmap_i2c struct just as it does right now. If the adapter
> does not support native I2C, check if the device can be supported by
> smbus (reg_bytes == 8 && val_bytes % 8 == 0). For each type of smbus
> operations have one regmap_bus struct, and if you can fallback to
> smbus choose the bus depending on the config's val_bytes.
>

What if the device only support SMBus block transfers, but the adapter
support both regular I2C and SMBus block transfers (don't know if such a
device exist :-)) ?

My point is that I'm not sure the core remap-i2c code can decide whether
to use SMBus or I2C transfer only from val_bytes value, but I might be
wrong.

Best Regards,

Boris

> - Lars
Mark Brown April 15, 2014, 12:56 p.m. UTC | #8
On Tue, Apr 15, 2014 at 02:25:15PM +0200, Lars-Peter Clausen wrote:
> On 04/15/2014 01:54 PM, Boris BREZILLON wrote:

> >And, should I keep one method to register an smbus regmap or should I
> >provide one method per access type and get rid of the
> >regmap_smbus_transfer_type enum ?

> I don't think we should leave the decision which bus to use to the driver.
> Neither should the driver have to choose whether to use smbus or native I2C.
> We want to use native I2C when available, because it is more efficient than
> going through the smbus emulation layer. On the other hand we want to
> automatically switch to smbus when native I2C is not available and the
> device can work fine with smbus. Also I'm afraid that we'll otherwise soon
> see code popping up like:

I have to say I'd misread the commit message (due to the multiple copies
that arrived before I actually reviewed it I think I just skimmed it and
went into incremental review) and was reviewing this as being for
*device* limitations not controller ones.  That also matches with what
Boris was saying about not being able to decide to use the word or byte
at a time operations automatically, though the commit message does say
it's about controller limitations.

> My suggestion is that in regmap_init_i2c() you check the capabilities of the
> I2C adapter. If it supports native I2C you setup the regmap with the
> regmap_i2c struct just as it does right now. If the adapter does not support
> native I2C, check if the device can be supported by smbus (reg_bytes == 8 &&
> val_bytes % 8 == 0). For each type of smbus operations have one regmap_bus
> struct, and if you can fallback to smbus choose the bus depending on the
> config's val_bytes.

That'd definitely be useful but potentially orthogonal, we can also do
both and expose smbus explicitly with I2C falling back to it
transparently.

If the device *is* limited to smbus and the controller supports both
(some do) I'd naively have expected that the native smbus support would
do a little better - otherwise why bother using it?  We could identify
the constraint set automatically for I2C devices though it's more for
the client driver to specify.

This means there's two changes to consider here:

 - Providing APIs for registering actual smbus devices as a convenience
   for devices with that constraint, regardless of how that is done
   behind the scenes.

 - Having the I2C implementation automatically use the smbus APIs if
   it can and either the controller is smbus only or it makes sense to
   do so for optimisation.

both of which are independently useful.
Lars-Peter Clausen April 15, 2014, 1:08 p.m. UTC | #9
On 04/15/2014 02:40 PM, Boris BREZILLON wrote:
>
> On 15/04/2014 14:25, Lars-Peter Clausen wrote:
>> On 04/15/2014 01:54 PM, Boris BREZILLON wrote:
>>>
>>>
>>> On 15/04/2014 12:09, Mark Brown wrote:
>>>> On Tue, Apr 15, 2014 at 09:36:13AM +0200, Boris BREZILLON wrote:
>>>>> On 14/04/2014 23:04, Mark Brown wrote:
>>>
>>>>>> The transfer type gets set once per device at init time so why not
>>>>>> just parameterise based on val_bytes?
>>>
>>>>> Actually, you may want to transfer 1 byte registers using the block
>>>>> method (if your device only support block transfers). This depends on
>>>>> the device being accessed and what it supports, but I'm not sure we
>>>>> can
>>>>> assume 1 byte registers will always be transferred using SMBUS byte
>>>>> transfers.
>>>
>>>> OK, so if this a realistic issue then it seems like it's better to
>>>> implement three different buses - there is not really any common code
>>>> between the various paths.
>>>
>>> Okay, I'll create 4 different busses (one for each access type).
>>> BTW, should I keep these implementations in the same source file
>>> (regmap-smbus.c) ?
>>> And, should I keep one method to register an smbus regmap or should I
>>> provide one method per access type and get rid of the
>>> regmap_smbus_transfer_type enum ?
>>
>> I don't think we should leave the decision which bus to use to the
>> driver. Neither should the driver have to choose whether to use smbus
>> or native I2C. We want to use native I2C when available, because it is
>> more efficient than going through the smbus emulation layer. On the
>> other hand we want to automatically switch to smbus when native I2C is
>> not available and the device can work fine with smbus. Also I'm afraid
>> that we'll otherwise soon see code popping up like:
>>
>> if (of_property_read_bool(np, "use-smbus-regmap"))
>>      regmap = regmap_init_smbus(...)
>> else
>>      regmap = regmap_init_i2c(...)
>>
>> My suggestion is that in regmap_init_i2c() you check the capabilities
>> of the I2C adapter. If it supports native I2C you setup the regmap
>> with the regmap_i2c struct just as it does right now. If the adapter
>> does not support native I2C, check if the device can be supported by
>> smbus (reg_bytes == 8 && val_bytes % 8 == 0). For each type of smbus
>> operations have one regmap_bus struct, and if you can fallback to
>> smbus choose the bus depending on the config's val_bytes.
>>
>
> What if the device only support SMBus block transfers, but the adapter
> support both regular I2C and SMBus block transfers (don't know if such a
> device exist :-)) ?

SMBus block transfers prefix the transfer with the number of bytes that are 
in this transfer. We do not support this currently in the I2C backend (but 
we could). If there should ever be a driver that needs support for SMBus 
block transfers this could easily be added. For now I think it is safe to 
ignore this.

>
> My point is that I'm not sure the core remap-i2c code can decide whether
> to use SMBus or I2C transfer only from val_bytes value, but I might be
> wrong.

It must be able to figure out what to do based on the config. If it is not 
we probably need to extend the config. E.g. if we should ever need to 
support devices which require SMBus block transfers we could add a flag that 
tells regmap that this device want's all messages prefixed with the length 
of the message.

- Lars
Lars-Peter Clausen April 15, 2014, 1:34 p.m. UTC | #10
On 04/15/2014 02:56 PM, Mark Brown wrote:
[...]
>> My suggestion is that in regmap_init_i2c() you check the capabilities of the
>> I2C adapter. If it supports native I2C you setup the regmap with the
>> regmap_i2c struct just as it does right now. If the adapter does not support
>> native I2C, check if the device can be supported by smbus (reg_bytes == 8 &&
>> val_bytes % 8 == 0). For each type of smbus operations have one regmap_bus
>> struct, and if you can fallback to smbus choose the bus depending on the
>> config's val_bytes.
>
> That'd definitely be useful but potentially orthogonal, we can also do
> both and expose smbus explicitly with I2C falling back to it
> transparently.
>
> If the device *is* limited to smbus and the controller supports both
> (some do) I'd naively have expected that the native smbus support would
> do a little better - otherwise why bother using it?  We could identify
> the constraint set automatically for I2C devices though it's more for
> the client driver to specify.
>
> This means there's two changes to consider here:
>
>   - Providing APIs for registering actual smbus devices as a convenience
>     for devices with that constraint, regardless of how that is done
>     behind the scenes.
>
>   - Having the I2C implementation automatically use the smbus APIs if
>     it can and either the controller is smbus only or it makes sense to
>     do so for optimisation.
>
> both of which are independently useful.
>

I don't think it makes sense to expose smbus explicitly. We can already 
describe smbus restricted devices just fine with the current regmap_config 
and already support them with the current I2C regmap implementation. Using 
native I2C to access these devices will be more efficient than going through 
the smbus emulation layer. The smbus emulation layer essentially does the 
same as we do in regmap, so using the smbus emulation layer through regmap 
means doing the same thing twice.

What we currently do not support is devices which are restricted to block 
transfers and I think adding support for this should be handled by a 
separate patchset. As far as I understood it this patchset is about making 
it possible to use smbus controllers with regmap devices which are smbus 
compatible.

As I see it there are currently 3 cases:

1) Device is strictly smbus only and the controller supports native smbus
    => Use smbus
2) The device is smbus compatible but has extensions (e.g. support for multi 
register writes) and the controller supports only smbus.
    => Use smbus
3) For every other case
    => Use native I2C.

- Lars
Mark Brown April 15, 2014, 4:46 p.m. UTC | #11
On Tue, Apr 15, 2014 at 03:34:55PM +0200, Lars-Peter Clausen wrote:
> On 04/15/2014 02:56 PM, Mark Brown wrote:

> >  - Providing APIs for registering actual smbus devices as a convenience
> >    for devices with that constraint, regardless of how that is done
> >    behind the scenes.

> >  - Having the I2C implementation automatically use the smbus APIs if
> >    it can and either the controller is smbus only or it makes sense to
> >    do so for optimisation.

> I don't think it makes sense to expose smbus explicitly. We can already
> describe smbus restricted devices just fine with the current regmap_config
> and already support them with the current I2C regmap implementation. Using

Please note what I said about convenience - if lots of devices have
exactly the same set of constraints to set up it's possible sensible to
have a standard way of specifying them.  It's going to be a bit easier
to just say it's smbus than to remember exactly what that means.

> native I2C to access these devices will be more efficient than going through
> the smbus emulation layer. The smbus emulation layer essentially does the
> same as we do in regmap, so using the smbus emulation layer through regmap
> means doing the same thing twice.

Right, that's why I said it's probably only worth it if the controller
does smbus natively.

> As I see it there are currently 3 cases:

> 1) Device is strictly smbus only and the controller supports native smbus
>    => Use smbus
> 2) The device is smbus compatible but has extensions (e.g. support for multi
> register writes) and the controller supports only smbus.
>    => Use smbus
> 3) For every other case
>    => Use native I2C.

It's not clear to me that if the controller supports smbus we shouldn't
use it; presumably it's adding some value to have written the code to
take advantage of it.  That would mean another case for device is smbus
only and controller has explicit smbus support.
Lars-Peter Clausen April 15, 2014, 7:18 p.m. UTC | #12
On 04/15/2014 06:46 PM, Mark Brown wrote:
> On Tue, Apr 15, 2014 at 03:34:55PM +0200, Lars-Peter Clausen wrote:
>> On 04/15/2014 02:56 PM, Mark Brown wrote:
>
>>>   - Providing APIs for registering actual smbus devices as a convenience
>>>     for devices with that constraint, regardless of how that is done
>>>     behind the scenes.
>
>>>   - Having the I2C implementation automatically use the smbus APIs if
>>>     it can and either the controller is smbus only or it makes sense to
>>>     do so for optimisation.
>
>> I don't think it makes sense to expose smbus explicitly. We can already
>> describe smbus restricted devices just fine with the current regmap_config
>> and already support them with the current I2C regmap implementation. Using
>
> Please note what I said about convenience - if lots of devices have
> exactly the same set of constraints to set up it's possible sensible to
> have a standard way of specifying them.  It's going to be a bit easier
> to just say it's smbus than to remember exactly what that means.

Maybe, but the only shared constraint is reg_bits = 8 (and if it is pure 
smbus use_single_rw = true).

>
>> native I2C to access these devices will be more efficient than going through
>> the smbus emulation layer. The smbus emulation layer essentially does the
>> same as we do in regmap, so using the smbus emulation layer through regmap
>> means doing the same thing twice.
>
> Right, that's why I said it's probably only worth it if the controller
> does smbus natively.
>
>> As I see it there are currently 3 cases:
>
>> 1) Device is strictly smbus only and the controller supports native smbus
>>     => Use smbus
>> 2) The device is smbus compatible but has extensions (e.g. support for multi
>> register writes) and the controller supports only smbus.
>>     => Use smbus
>> 3) For every other case
>>     => Use native I2C.
>
> It's not clear to me that if the controller supports smbus we shouldn't
> use it; presumably it's adding some value to have written the code to
> take advantage of it.  That would mean another case for device is smbus
> only and controller has explicit smbus support.

Potentially yes, but not necessarily in the first version of the driver. 
It's a bit of an exotic case, there seem to be only two I2C controller 
drivers in the Linux kernel that have support for both raw I2C and native 
SMBus and neither of them don't seem to be fairly recent (i2c-powermac.c and 
i2c-pasemi.c). And on the other hand most devices are SMBus compatible have 
extensions like being able to write/read multiple register in one transfer 
if raw I2C access is available, which is something we want to make use of if 
available.

And the other thing to consider is that a client driver currently has no way 
to query whether the controller driver supports native SMBus or only 
emulated SMBus. This is surely something that can be added to the I2C core, 
but this is necessarily something that we require before any SMBus support 
is added to regmap.

So in conclusion I think the best way forward is to create a patch that 
checks if native I2C is available, and if not falls back to either 
SMBUS_{WRITE,READ}_BYTE or SMBUS_{WRITE,READ}_WORD operations (Depending on 
whether val_bits is 8 or 16). Such a patch, I think, has the most effect for 
the least amount of work since it is rather simple and self contained and 
allows all devices which are SMBus compatible to be used with SMBus-only 
controllers. Everything else discussed in this thread (optimizations for 
special cases, convince helpers) can then be implemented on top of that.

- Lars
Mark Brown April 15, 2014, 10:38 p.m. UTC | #13
On Tue, Apr 15, 2014 at 09:18:11PM +0200, Lars-Peter Clausen wrote:
> On 04/15/2014 06:46 PM, Mark Brown wrote:

> >Please note what I said about convenience - if lots of devices have
> >exactly the same set of constraints to set up it's possible sensible to
> >have a standard way of specifying them.  It's going to be a bit easier
> >to just say it's smbus than to remember exactly what that means.

> Maybe, but the only shared constraint is reg_bits = 8 (and if it is pure
> smbus use_single_rw = true).

I thought there was some other stuff at the edge of the protocol, though
perhaps I misremember.  We do also have the thing with implementing the
bulk write differently.

> It's a bit of an exotic case, there seem to be only two I2C controller
> drivers in the Linux kernel that have support for both raw I2C and native
> SMBus and neither of them don't seem to be fairly recent (i2c-powermac.c and
> i2c-pasemi.c). And on the other hand most devices are SMBus compatible have

Blackfin was another - I didn't look that far to be honest since the
first few that I found did it.

> extensions like being able to write/read multiple register in one transfer
> if raw I2C access is available, which is something we want to make use of if
> available.

Sure, if the device supports it.

> And the other thing to consider is that a client driver currently has no way
> to query whether the controller driver supports native SMBus or only
> emulated SMBus. This is surely something that can be added to the I2C core,
> but this is necessarily something that we require before any SMBus support
> is added to regmap.

Right, that'll take a couple of minutes though.

> So in conclusion I think the best way forward is to create a patch that
> checks if native I2C is available, and if not falls back to either
> SMBUS_{WRITE,READ}_BYTE or SMBUS_{WRITE,READ}_WORD operations (Depending on
> whether val_bits is 8 or 16). Such a patch, I think, has the most effect for
> the least amount of work since it is rather simple and self contained and
> allows all devices which are SMBus compatible to be used with SMBus-only
> controllers. Everything else discussed in this thread (optimizations for
> special cases, convince helpers) can then be implemented on top of that.

Indeed, like I've been saying there's several orthogonal things going on
here and that's one of them.
diff mbox

Patch

diff --git a/drivers/base/regmap/Kconfig b/drivers/base/regmap/Kconfig
index 4251570..450b4c1 100644
--- a/drivers/base/regmap/Kconfig
+++ b/drivers/base/regmap/Kconfig
@@ -3,7 +3,7 @@ 
 # subsystems should select the appropriate symbols.
 
 config REGMAP
-	default y if (REGMAP_I2C || REGMAP_SPI || REGMAP_SPMI || REGMAP_MMIO || REGMAP_IRQ)
+	default y if (REGMAP_I2C || REGMAP_SMBUS || REGMAP_SPI || REGMAP_SPMI || REGMAP_MMIO || REGMAP_IRQ)
 	select LZO_COMPRESS
 	select LZO_DECOMPRESS
 	select IRQ_DOMAIN if REGMAP_IRQ
@@ -12,6 +12,9 @@  config REGMAP
 config REGMAP_I2C
 	tristate
 
+config REGMAP_SMBUS
+	tristate
+
 config REGMAP_SPI
 	tristate
 
diff --git a/drivers/base/regmap/Makefile b/drivers/base/regmap/Makefile
index a7c670b..e752b9c 100644
--- a/drivers/base/regmap/Makefile
+++ b/drivers/base/regmap/Makefile
@@ -2,6 +2,7 @@  obj-$(CONFIG_REGMAP) += regmap.o regcache.o
 obj-$(CONFIG_REGMAP) += regcache-rbtree.o regcache-lzo.o regcache-flat.o
 obj-$(CONFIG_DEBUG_FS) += regmap-debugfs.o
 obj-$(CONFIG_REGMAP_I2C) += regmap-i2c.o
+obj-$(CONFIG_REGMAP_SMBUS) += regmap-smbus.o
 obj-$(CONFIG_REGMAP_SPI) += regmap-spi.o
 obj-$(CONFIG_REGMAP_SPMI) += regmap-spmi.o
 obj-$(CONFIG_REGMAP_MMIO) += regmap-mmio.o
diff --git a/drivers/base/regmap/regmap-smbus.c b/drivers/base/regmap/regmap-smbus.c
new file mode 100644
index 0000000..c8b8075
--- /dev/null
+++ b/drivers/base/regmap/regmap-smbus.c
@@ -0,0 +1,364 @@ 
+/*
+ * Register map access API - SMBus support
+ *
+ * Copyright 2014 Free Electrons
+ *
+ * Author: Boris Brezillon <boris.brezillon@free-electrons.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/regmap.h>
+#include <linux/i2c.h>
+#include <linux/module.h>
+#include <linux/slab.h>
+
+struct regmap_smbus_context {
+	struct i2c_client *i2c;
+	enum regmap_smbus_transfer_type transfer_type;
+	int val_bytes;
+};
+
+static int regmap_smbus_write(void *context, const void *data, size_t count)
+{
+	struct regmap_smbus_context *ctx = context;
+	int ret = 0;
+	u8 reg = *(u8 *)data++;
+
+	count--;
+
+	switch (ctx->transfer_type) {
+	case REGMAP_SMBUS_BYTE_TRANSFER:
+		while (count > 0 && !ret) {
+			ret = i2c_smbus_write_byte_data(ctx->i2c, reg++,
+							*(u8 *)data++);
+
+			count--;
+		}
+		break;
+
+	case REGMAP_SMBUS_WORD_TRANSFER:
+		while (count > 0 && !ret) {
+			ret = i2c_smbus_write_word_data(ctx->i2c, reg,
+							*(u16 *)data++);
+
+			reg += 2;
+			count -= 2;
+		}
+		break;
+
+	case REGMAP_SMBUS_BLOCK_TRANSFER:
+		while (count > 0 && !ret) {
+			ret = i2c_smbus_write_block_data(ctx->i2c,
+							 reg,
+							 ctx->val_bytes,
+							 (const u8 *)data);
+
+			reg += ctx->val_bytes;
+			count -= ctx->val_bytes;
+			data += ctx->val_bytes;
+		}
+		break;
+
+	case REGMAP_SMBUS_I2C_BLOCK_TRANSFER:
+		while (count > 0 && !ret) {
+			ret = i2c_smbus_write_i2c_block_data(ctx->i2c,
+							     reg,
+							     ctx->val_bytes,
+							     (const u8 *)data);
+
+			reg += ctx->val_bytes;
+			count -= ctx->val_bytes;
+			data += ctx->val_bytes;
+		}
+		break;
+
+	default:
+		return -ENOTSUPP;
+	}
+
+	return ret;
+}
+
+static int regmap_smbus_gather_write(void *context,
+				     const void *reg, size_t reg_size,
+				     const void *val, size_t val_size)
+{
+	struct regmap_smbus_context *ctx = context;
+	u8 smbus_reg;
+	int ret = 0;
+
+	if (reg_size != 1)
+		return -ENOTSUPP;
+
+	smbus_reg = *(u8 *)reg;
+
+	switch (ctx->transfer_type) {
+	case REGMAP_SMBUS_BYTE_TRANSFER:
+		while (val_size && !ret) {
+			ret = i2c_smbus_write_byte_data(ctx->i2c,
+							smbus_reg++,
+							*(u8 *)val++);
+
+			val_size--;
+		}
+		break;
+
+	case REGMAP_SMBUS_WORD_TRANSFER:
+		while (val_size && !ret) {
+			ret = i2c_smbus_write_word_data(ctx->i2c,
+							smbus_reg,
+							*(u16 *)val++);
+
+			smbus_reg += 2;
+			val_size -= 2;
+		}
+		break;
+
+	case REGMAP_SMBUS_BLOCK_TRANSFER:
+		while (val_size && !ret) {
+			ret = i2c_smbus_write_block_data(ctx->i2c,
+							 smbus_reg,
+							 ctx->val_bytes,
+							 (const u8 *)val);
+
+			smbus_reg += ctx->val_bytes;
+			val_size -= ctx->val_bytes;
+			val += ctx->val_bytes;
+		}
+		break;
+
+	case REGMAP_SMBUS_I2C_BLOCK_TRANSFER:
+		if (val_size > I2C_SMBUS_BLOCK_MAX)
+			return -ENOTSUPP;
+
+		while (val_size && !ret) {
+			ret = i2c_smbus_write_i2c_block_data(ctx->i2c,
+							     smbus_reg,
+							     ctx->val_bytes,
+							     (const u8 *)val);
+
+			smbus_reg += ctx->val_bytes;
+			val_size -= ctx->val_bytes;
+			val += ctx->val_bytes;
+		}
+		break;
+
+	default:
+		return -ENOTSUPP;
+	}
+
+	return ret;
+}
+
+static int regmap_smbus_read(void *context,
+			     const void *reg, size_t reg_size,
+			     void *val, size_t val_size)
+{
+	struct regmap_smbus_context *ctx = context;
+	u8 buffer[I2C_SMBUS_BLOCK_MAX];
+	u8 smbus_reg;
+	int ret = 0;
+
+	if (reg_size != 1)
+		return -ENOTSUPP;
+
+	smbus_reg = *(u8 *)reg;
+
+	switch (ctx->transfer_type) {
+	case REGMAP_SMBUS_BYTE_TRANSFER:
+		while (val_size && ret >= 0) {
+			ret = i2c_smbus_read_byte_data(ctx->i2c, smbus_reg++);
+			if (ret >= 0)
+				*((u8 *)val++) = ret;
+
+			val_size--;
+		}
+		break;
+
+	case REGMAP_SMBUS_WORD_TRANSFER:
+		while (val_size && ret >= 0) {
+			ret = i2c_smbus_read_word_data(ctx->i2c, smbus_reg);
+			if (ret >= 0)
+				*(u16 *)val++ = ret;
+
+			smbus_reg += 2;
+			val_size -= 2;
+		}
+		break;
+
+	case REGMAP_SMBUS_BLOCK_TRANSFER:
+		while (val_size && ret >= 0) {
+			ret = i2c_smbus_read_block_data(ctx->i2c,
+							smbus_reg,
+							buffer);
+			if (ret >= 0 && ret < ctx->val_bytes) {
+				ret = -EIO;
+				break;
+			}
+
+			memcpy(val, buffer, ctx->val_bytes);
+			smbus_reg += ctx->val_bytes;
+			val_size -= ctx->val_bytes;
+			val += ctx->val_bytes;
+		}
+		break;
+
+	case REGMAP_SMBUS_I2C_BLOCK_TRANSFER:
+		while (val_size && ret >= 0) {
+			return -ENOTSUPP;
+
+			ret = i2c_smbus_read_i2c_block_data(ctx->i2c,
+							    smbus_reg,
+							    ctx->val_bytes,
+							    (u8 *)val);
+			if (ret >= 0 && ret < val_size) {
+				ret = -EIO;
+				break;
+			}
+
+			smbus_reg += ctx->val_bytes;
+			val_size -= ctx->val_bytes;
+			val += ctx->val_bytes;
+		}
+		break;
+
+	default:
+		return -ENOTSUPP;
+	}
+
+	if (ret < 0)
+		return ret;
+
+	return 0;
+}
+
+static void regmap_smbus_free_context(void *context)
+{
+	kfree(context);
+}
+
+struct regmap_smbus_context *regmap_smbus_gen_context(struct i2c_client *i2c,
+				const struct regmap_config *config,
+				enum regmap_smbus_transfer_type transfer_type)
+{
+	struct regmap_smbus_context *ctx;
+	int val_bytes = 0;
+
+	if (config->reg_bits != 8 || config->pad_bits != 0)
+		return ERR_PTR(-ENOTSUPP);
+
+	switch (transfer_type) {
+	case REGMAP_SMBUS_BYTE_TRANSFER:
+		if (config->val_bits != 8)
+			return ERR_PTR(-EINVAL);
+
+		if (!i2c_check_functionality(i2c->adapter,
+					     I2C_FUNC_SMBUS_BYTE_DATA))
+			return ERR_PTR(-ENOTSUPP);
+		break;
+
+	case REGMAP_SMBUS_WORD_TRANSFER:
+		if (config->val_bits != 16)
+			return ERR_PTR(-EINVAL);
+
+		if (!i2c_check_functionality(i2c->adapter,
+					     I2C_FUNC_SMBUS_WORD_DATA))
+			return ERR_PTR(-ENOTSUPP);
+		break;
+
+	case REGMAP_SMBUS_BLOCK_TRANSFER:
+		if (config->val_bits > (I2C_SMBUS_BLOCK_MAX * 8))
+			return ERR_PTR(-EINVAL);
+
+		val_bytes = DIV_ROUND_UP(config->val_bits, 8);
+		if (!i2c_check_functionality(i2c->adapter,
+					     I2C_FUNC_SMBUS_BLOCK_DATA))
+			return ERR_PTR(-ENOTSUPP);
+		break;
+
+	case REGMAP_SMBUS_I2C_BLOCK_TRANSFER:
+		if (config->val_bits > (I2C_SMBUS_BLOCK_MAX * 8))
+			return ERR_PTR(-EINVAL);
+
+		val_bytes = DIV_ROUND_UP(config->val_bits, 8);
+
+		if (!i2c_check_functionality(i2c->adapter,
+					     I2C_FUNC_SMBUS_I2C_BLOCK))
+			return ERR_PTR(-ENOTSUPP);
+		break;
+
+	default:
+		return ERR_PTR(-ENOTSUPP);
+	}
+
+	ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
+	if (!ctx)
+		return ERR_PTR(-ENOMEM);
+
+	ctx->i2c = i2c;
+	ctx->transfer_type = transfer_type;
+	ctx->val_bytes = val_bytes;
+
+	return ctx;
+}
+
+static struct regmap_bus regmap_smbus = {
+	.write = regmap_smbus_write,
+	.gather_write = regmap_smbus_gather_write,
+	.read = regmap_smbus_read,
+	.free_context = regmap_smbus_free_context,
+};
+
+/**
+ * regmap_init_smbus(): Initialise register map
+ *
+ * @i2c: Device that will be interacted with
+ * @config: Configuration for register map
+ * @transfer_type: SMBUS transfer type
+ *
+ * The return value will be an ERR_PTR() on error or a valid pointer to
+ * a struct regmap.
+ */
+struct regmap *regmap_init_smbus(struct i2c_client *i2c,
+				 const struct regmap_config *config,
+				 enum regmap_smbus_transfer_type transfer_type)
+{
+	struct regmap_smbus_context *ctx =
+		regmap_smbus_gen_context(i2c, config, transfer_type);
+
+	if (IS_ERR(ctx))
+		return ERR_PTR(PTR_ERR(ctx));
+
+	return regmap_init(&i2c->dev, &regmap_smbus, ctx, config);
+}
+EXPORT_SYMBOL_GPL(regmap_init_smbus);
+
+/**
+ * devm_regmap_init_smbus(): Initialise managed register map
+ *
+ * @i2c: Device that will be interacted with
+ * @config: Configuration for register map
+ * @transfer_type: SMBUS transfer type
+ *
+ * The return value will be an ERR_PTR() on error or a valid pointer
+ * to a struct regmap.  The regmap will be automatically freed by the
+ * device management code.
+ */
+struct regmap *devm_regmap_init_smbus(struct i2c_client *i2c,
+				const struct regmap_config *config,
+				enum regmap_smbus_transfer_type transfer_type)
+{
+	struct regmap_smbus_context *ctx =
+		regmap_smbus_gen_context(i2c, config, transfer_type);
+
+	if (IS_ERR(ctx))
+		return ERR_PTR(PTR_ERR(ctx));
+
+	return devm_regmap_init(&i2c->dev, &regmap_smbus, ctx, config);
+}
+EXPORT_SYMBOL_GPL(devm_regmap_init_smbus);
+
+MODULE_LICENSE("GPL");
diff --git a/include/linux/regmap.h b/include/linux/regmap.h
index 85691b9..34ef2c7 100644
--- a/include/linux/regmap.h
+++ b/include/linux/regmap.h
@@ -317,6 +317,13 @@  struct regmap_bus {
 	enum regmap_endian val_format_endian_default;
 };
 
+enum regmap_smbus_transfer_type {
+	REGMAP_SMBUS_BYTE_TRANSFER,
+	REGMAP_SMBUS_WORD_TRANSFER,
+	REGMAP_SMBUS_BLOCK_TRANSFER,
+	REGMAP_SMBUS_I2C_BLOCK_TRANSFER,
+};
+
 struct regmap *regmap_init(struct device *dev,
 			   const struct regmap_bus *bus,
 			   void *bus_context,
@@ -325,6 +332,9 @@  int regmap_attach_dev(struct device *dev, struct regmap *map,
 				 const struct regmap_config *config);
 struct regmap *regmap_init_i2c(struct i2c_client *i2c,
 			       const struct regmap_config *config);
+struct regmap *regmap_init_smbus(struct i2c_client *i2c,
+				const struct regmap_config *config,
+				enum regmap_smbus_transfer_type transfer_type);
 struct regmap *regmap_init_spi(struct spi_device *dev,
 			       const struct regmap_config *config);
 struct regmap *regmap_init_spmi_base(struct spmi_device *dev,
@@ -341,6 +351,9 @@  struct regmap *devm_regmap_init(struct device *dev,
 				const struct regmap_config *config);
 struct regmap *devm_regmap_init_i2c(struct i2c_client *i2c,
 				    const struct regmap_config *config);
+struct regmap *devm_regmap_init_smbus(struct i2c_client *i2c,
+				const struct regmap_config *config,
+				enum regmap_smbus_transfer_type transfer_type);
 struct regmap *devm_regmap_init_spi(struct spi_device *dev,
 				    const struct regmap_config *config);
 struct regmap *devm_regmap_init_spmi_base(struct spmi_device *dev,