| Message ID | 20171211165811.5806-1-adrian.fiergolski@cern.ch |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v3] i2c: Add support for NXP PCA984x family. | expand |
On 2017-12-11 17:58, Adrian Fiergolski wrote: > This patch exetends the current i2c-mux-pca954x driver and adds suuport for support > a newer PCA984x family of the I2C switches and multiplexers from NXP. > In probe function, the patch supports device ID function, introduced in the > new family, and checks it against configuration provided by the > devicetree. Moreover, it also performs software reset which is now > available for the new devices. > The reset of the code remains common with the legacy PCA954x devices. > > Signed-off-by: Adrian Fiergolski <adrian.fiergolski@cern.ch> > --- > Apply Peter's and Rodolfo's comments. > Add missing part in pca984x_family_probe which checks deviceID. > > Peter addressing your main comments: > > Quoting the PCA9848 manual: "The Software Reset Call allows all the devices in > the I2C-bus to be reset to the power-up state value through a specific formatted > I2C-bus command. To be performed correctly, it implies that the I2C-bus is > functional and that there is no device hanging the bus." > This call should reset the multiplexer and all devices which are below it in the > I2C address space tree to the default state. Yes, I read all that in the datasheet, but why do *you* need it? Because it is wrong to do it in this driver. It is doubly wrong to do it unconditionally. I was asking so that I could help you find a solution for your problem, because it is simply not acceptable to add a bus-wide reset to a random driver. > > The manufacturer ID and device ID checks in pca984x_family_probe ensures that > we actually communicate with a device we intended to (according to the > devicetree) and that communication with this device works properly (this device > ID and manufacturer ID can be seen as a fixed pattern). Yes yes yes, but do you actually need it? Because it complicates the driver for little gain (in my opinion). > .../devicetree/bindings/i2c/i2c-mux-pca954x.txt | 5 +- > drivers/i2c/muxes/Kconfig | 2 +- > drivers/i2c/muxes/i2c-mux-pca954x.c | 157 +++++++++++++++++++-- > 3 files changed, 153 insertions(+), 11 deletions(-) > > diff --git a/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt b/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt > index aa097045a10e..b428bc0d81b1 100644 > --- a/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt > +++ b/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt > @@ -1,10 +1,13 @@ > * NXP PCA954x I2C bus switch > > +The driver supports NXP PCA954x and PCA984x I2C mux/switch devices. > + > Required Properties: > > - compatible: Must contain one of the following. > "nxp,pca9540", "nxp,pca9542", "nxp,pca9543", "nxp,pca9544", > - "nxp,pca9545", "nxp,pca9546", "nxp,pca9547", "nxp,pca9548" > + "nxp,pca9545", "nxp,pca9546", "nxp,pca9547", "nxp,pca9548", > + "nxp,pca9846", "nxp,pca9847", "nxp,pca9848", "nxp,pca9849" > > - reg: The I2C address of the device. > > diff --git a/drivers/i2c/muxes/Kconfig b/drivers/i2c/muxes/Kconfig > index 0f5c8fc36625..5da2e04585a9 100644 > --- a/drivers/i2c/muxes/Kconfig > +++ b/drivers/i2c/muxes/Kconfig > @@ -68,7 +68,7 @@ config I2C_MUX_PCA954x > depends on GPIOLIB || COMPILE_TEST > help > If you say yes here you get support for the Philips PCA954x > - I2C mux/switch devices. > + and PCA984x I2C mux/switch devices. lets update Philips to NXP while at it. > > This driver can also be built as a module. If so, the module > will be called i2c-mux-pca954x. > diff --git a/drivers/i2c/muxes/i2c-mux-pca954x.c b/drivers/i2c/muxes/i2c-mux-pca954x.c > index 2ca068d8b92d..6205ae52aa4d 100644 > --- a/drivers/i2c/muxes/i2c-mux-pca954x.c > +++ b/drivers/i2c/muxes/i2c-mux-pca954x.c > @@ -1,14 +1,15 @@ > /* > * I2C multiplexer > * > + * Copyright (c) 2017 Adrian Fiergolski <Adrian.Fiergolski@cern.ch> > * Copyright (c) 2008-2009 Rodolfo Giometti <giometti@linux.it> > * Copyright (c) 2008-2009 Eurotech S.p.A. <info@eurotech.it> > * > - * This module supports the PCA954x series of I2C multiplexer/switch chips > - * made by Philips Semiconductors. > + * This module supports the PCA954x and PCA954x series of I2C multiplexer/switch > + * chips made by Philips Semiconductors. NXP. > * This includes the: > - * PCA9540, PCA9542, PCA9543, PCA9544, PCA9545, PCA9546, PCA9547 > - * and PCA9548. > + * PCA9540, PCA9542, PCA9543, PCA9544, PCA9545, PCA9546, PCA9547, > + * PCA9548, PCA9846, PCA9847, PCA9848 and PCA9849 > * > * These chips are all controlled via the I2C bus itself, and all have a > * single 8-bit register. The upstream "parent" bus fans out to two, > @@ -54,6 +55,12 @@ > > #define PCA954X_IRQ_OFFSET 4 > > +/* PCA984x family I2C other addresses */ > +#define GENERAL_CALL 0x00 > +#define SOFTWARE_RESET 0x06 > +#define DEVICE_ID_ADDRESS 0x7C > +#define NXP_ID 0x00 > + > enum pca_type { > pca_9540, > pca_9542, > @@ -63,6 +70,10 @@ enum pca_type { > pca_9546, > pca_9547, > pca_9548, > + pca_9846, > + pca_9847, > + pca_9848, > + pca_9849, > }; > > struct chip_desc { > @@ -73,6 +84,7 @@ struct chip_desc { > pca954x_ismux = 0, > pca954x_isswi > } muxtype; > + u16 deviceID; /* used by PCA984x family only */ Please name it device_id, if you insist on keeping it. > }; > > struct pca954x { > @@ -129,6 +141,29 @@ static const struct chip_desc chips[] = { > .nchans = 8, > .muxtype = pca954x_isswi, > }, > + [pca_9846] = { > + .nchans = 4, > + .muxtype = pca954x_isswi, > + .deviceID = 0x10B, > + }, > + As requested for v2, please drop these empty lines, the blocks above the hunk don't have them. Let's keep things consistent. > + [pca_9847] = { > + .nchans = 8, > + .muxtype = pca954x_ismux, > + .deviceID = 0x108, > + }, > + > + [pca_9848] = { > + .nchans = 8, > + .muxtype = pca954x_isswi, > + .deviceID = 0x10A, > + }, > + > + [pca_9849] = { > + .nchans = 4, > + .muxtype = pca954x_ismux, > + .deviceID = 0x109, > + }, > }; > > static const struct i2c_device_id pca954x_id[] = { > @@ -140,6 +175,10 @@ static const struct i2c_device_id pca954x_id[] = { > { "pca9546", pca_9546 }, > { "pca9547", pca_9547 }, > { "pca9548", pca_9548 }, > + { "pca9846", pca_9846 }, > + { "pca9847", pca_9847 }, > + { "pca9848", pca_9848 }, > + { "pca9849", pca_9849 }, > { } > }; > MODULE_DEVICE_TABLE(i2c, pca954x_id); > @@ -154,6 +193,10 @@ static const struct of_device_id pca954x_of_match[] = { > { .compatible = "nxp,pca9546", .data = &chips[pca_9546] }, > { .compatible = "nxp,pca9547", .data = &chips[pca_9547] }, > { .compatible = "nxp,pca9548", .data = &chips[pca_9548] }, > + { .compatible = "nxp,pca9846", .data = &chips[pca_9846] }, > + { .compatible = "nxp,pca9847", .data = &chips[pca_9847] }, > + { .compatible = "nxp,pca9848", .data = &chips[pca_9848] }, > + { .compatible = "nxp,pca9849", .data = &chips[pca_9849] }, > {} > }; > MODULE_DEVICE_TABLE(of, pca954x_of_match); > @@ -304,6 +347,83 @@ static void pca954x_cleanup(struct i2c_mux_core *muxc) > i2c_mux_del_adapters(muxc); > } > > +/* > + * Part of probe function specific for pca954x family > + */ > +static int pca954x_family_probe(struct i2c_client *client, > + const struct i2c_device_id *id) > +{ > + Drop the blank line. > + /* Write the mux register at addr to verify > + * that the mux is in fact present. This also > + * initializes the mux to disconnected state. > + */ /* * Use this comment style for all multiline comments that you write * in the kernel source. */ I realize that you just copied this one, but let's clean it up while at it. > + if (i2c_smbus_write_byte(client, 0) < 0) > + return -ENODEV; > + > + return 0; > +} > + > +/* > + * Part of probe function specific for pca984x family > + */ > +static int pca984x_family_probe(struct i2c_client *client, > + const struct i2c_device_id *id) Ok, you seem determined to keep the device/manufacturer check. In that case, it is odd to have functions that does not follow the driver prefix rule. So, please rename this one to e.g. pca954x_pca984x_probe and the one above to pca954x_default_probe. Or something such. > +{ > + struct i2c_adapter *adap = to_i2c_adapter(client->dev.parent); > + union i2c_smbus_data device_id_raw; > + u16 manufacturer_id; /* 12 bits */ > + u16 device_id; /* 9 bits */ > + > + if (!i2c_check_functionality(adap, I2C_FUNC_SMBUS_WRITE_BYTE_DATA)) { > + dev_warn(&client->dev, "PCA9846 family: I2c adapter doesn't support I2C_FUNC_SMBUS_WRITE_BYTE_DATA"); > + return -ENODEV; > + } > + > + if (!i2c_check_functionality(adap, I2C_FUNC_SMBUS_READ_I2C_BLOCK)) { > + dev_warn(&client->dev, "PCA9846 family: I2c adapter doesn't support I2C_FUNC_SMBUS_READ_I2C_BLOCK"); > + return -ENODEV; > + } > + > + /* Software reset */ > + if (i2c_smbus_xfer(client->adapter, GENERAL_CALL, client->flags, > + I2C_SMBUS_WRITE, SOFTWARE_RESET, I2C_SMBUS_BYTE, NULL) < 0) { > + dev_warn(&client->dev, "PCA9846 family: Sofrware reset failed\n"); > + return -ENODEV; > + } As stated above, this just has to go. > + > + /* Get device ID */ > + device_id_raw.block[0] = 3; /* read 3 bytes */ > + if (i2c_smbus_xfer(client->adapter, DEVICE_ID_ADDRESS, client->flags, > + I2C_SMBUS_READ, client->addr << 1, > + I2C_SMBUS_I2C_BLOCK_DATA, &device_id_raw)) { Whooa, I managed to miss this last time... Why are you not using the i2c_smbus_read_i2c_block_data helper? (and even if you have some good reason to open-code this, you seem to have some seriously confused arguments, so if those are indeed correct you need to document what the hell is going on) > + dev_warn(&client->dev, "PCA9846 family: Get device ID failed\n"); > + return -ENODEV; > + } > + > + /* Device ID contains only 3 bytes */ > + if (device_id_raw.block[0] != 3) { > + dev_warn(&client->dev, "PCA9846 family: Get device ID failed\n"); > + return -ENODEV; > + } > + > + /* Check manufacturer ID (12 bits) */ > + manufacturer_id = ((u16) device_id_raw.block[1] << 4) | (device_id_raw.block[2] >> 4); This assignment is still broken... > + if (manufacturer_id != NXP_ID) { > + dev_warn(&client->dev, "PCA9846 family: Manufacturer ID does not match NXP\n"); > + return -ENODEV; > + } > + > + /* Check device ID (9 bits) */ > + device_id = ((u16) device_id_raw.block[2] << 5) | (device_id_raw.block[3] >> 3); ...and this is also broken. > + if (device_id != chips[id->driver_data].deviceID) { > + dev_warn(&client->dev, "PCA9846 family: Device ID does not match %s\n", id->name); > + return -ENODEV; > + } > + > + return 0; > +} > + > /* > * I2C init/probing/exit functions > */ > @@ -339,11 +459,29 @@ static int pca954x_probe(struct i2c_client *client, > if (IS_ERR(gpio)) > return PTR_ERR(gpio); > > - /* Write the mux register at addr to verify > - * that the mux is in fact present. This also > - * initializes the mux to disconnected state. > - */ > - if (i2c_smbus_write_byte(client, 0) < 0) { > + switch (id->driver_data) { > + case pca_9540: > + case pca_9542: > + case pca_9543: > + case pca_9544: > + case pca_9545: > + case pca_9546: > + case pca_9547: > + case pca_9548: Please drop these and do the 954x probe in default. > + ret = pca954x_family_probe(client, id); > + break; > + case pca_9846: > + case pca_9847: > + case pca_9848: > + case pca_9849: > + ret = pca984x_family_probe(client, id); > + break; > + default: /* unknown device */ > + ret = -ENODEV; > + break; > + } > + > + if (ret < 0) { > dev_warn(&client->dev, "probe failed\n"); > return -ENODEV; return ret; Cheers, Peter > } > @@ -443,6 +581,7 @@ static struct i2c_driver pca954x_driver = { > > module_i2c_driver(pca954x_driver); > > +MODULE_AUTHOR("Adrian Fiergolski <Adrian.Fiergolski@cern.ch>"); > MODULE_AUTHOR("Rodolfo Giometti <giometti@linux.it>"); > MODULE_DESCRIPTION("PCA954x I2C mux/switch driver"); > MODULE_LICENSE("GPL v2"); >
On 11.12.2017 at 20:14, Peter Rosin wrote: > >> a newer PCA984x family of the I2C switches and multiplexers from NXP. >> In probe function, the patch supports device ID function, introduced in the >> new family, and checks it against configuration provided by the >> devicetree. Moreover, it also performs software reset which is now >> available for the new devices. >> The reset of the code remains common with the legacy PCA954x devices. >> >> Signed-off-by: Adrian Fiergolski <adrian.fiergolski@cern.ch> >> --- >> Apply Peter's and Rodolfo's comments. >> Add missing part in pca984x_family_probe which checks deviceID. >> >> Peter addressing your main comments: >> >> Quoting the PCA9848 manual: "The Software Reset Call allows all the devices in >> the I2C-bus to be reset to the power-up state value through a specific formatted >> I2C-bus command. To be performed correctly, it implies that the I2C-bus is >> functional and that there is no device hanging the bus." >> This call should reset the multiplexer and all devices which are below it in the >> I2C address space tree to the default state. > Yes, I read all that in the datasheet, but why do *you* need it? Because > it is wrong to do it in this driver. It is doubly wrong to do it > unconditionally. I was asking so that I could help you find a solution > for your problem, because it is simply not acceptable to add a bus-wide > reset to a random driver. In a normal run scenario, I don't need it. I can imagine it may be a problem, if the module is suddenly unloaded and loaded: the mux remains set to some not default bus and the module is not aware of it. However, I agree then the bus could be reset somewhere at higher level. >> The manufacturer ID and device ID checks in pca984x_family_probe ensures that >> we actually communicate with a device we intended to (according to the >> devicetree) and that communication with this device works properly (this device >> ID and manufacturer ID can be seen as a fixed pattern). > Yes yes yes, but do you actually need it? Because it complicates the driver > for little gain (in my opinion). Well, yes... I want to be sure that the driver is actually able to serve the device which is going to be assigned to it. The best, of course, would be if I2C could enumerate itself (like PCIe), but as it's not a case and we need to rely on devicetree, I understand the driver should do "maximum" to cross-check the static configuration with the actual hardware. Moreover, in case of a configuration problem, one will be able to trace it immediately. By default, the driver performs a dummy access anyway. Since new chips come with those ID registers, I think it's reasonable to do something useful. >> + >> + /* Get device ID */ >> + device_id_raw.block[0] = 3; /* read 3 bytes */ >> + if (i2c_smbus_xfer(client->adapter, DEVICE_ID_ADDRESS, client->flags, >> + I2C_SMBUS_READ, client->addr << 1, >> + I2C_SMBUS_I2C_BLOCK_DATA, &device_id_raw)) { >> > Whooa, I managed to miss this last time... Why are you not using the > i2c_smbus_read_i2c_block_data helper? (and even if you have some good > reason to open-code this, you seem to have some seriously confused > arguments, so if those are indeed correct you need to document what > the hell is going on) > The deviceID access is described in paragraph 6.2.2 of the manual (https://www.nxp.com/docs/en/data-sheet/PCA9846.pdf). The fixed DEVICE_ID_ADDRESS (0x7C) is used as address. The i2c_smbus_read_i2c_block_data function uses address of a client (client->addr). Cheers, Adrian
On 2017-12-12 13:06, Adrian Fiergolski wrote: > On 11.12.2017 at 20:14, Peter Rosin wrote: >> >>> a newer PCA984x family of the I2C switches and multiplexers from NXP. >>> In probe function, the patch supports device ID function, introduced in the >>> new family, and checks it against configuration provided by the >>> devicetree. Moreover, it also performs software reset which is now >>> available for the new devices. >>> The reset of the code remains common with the legacy PCA954x devices. >>> >>> Signed-off-by: Adrian Fiergolski <adrian.fiergolski@cern.ch> >>> --- >>> Apply Peter's and Rodolfo's comments. >>> Add missing part in pca984x_family_probe which checks deviceID. >>> >>> Peter addressing your main comments: >>> >>> Quoting the PCA9848 manual: "The Software Reset Call allows all the devices in >>> the I2C-bus to be reset to the power-up state value through a specific formatted >>> I2C-bus command. To be performed correctly, it implies that the I2C-bus is >>> functional and that there is no device hanging the bus." >>> This call should reset the multiplexer and all devices which are below it in the >>> I2C address space tree to the default state. >> Yes, I read all that in the datasheet, but why do *you* need it? Because >> it is wrong to do it in this driver. It is doubly wrong to do it >> unconditionally. I was asking so that I could help you find a solution >> for your problem, because it is simply not acceptable to add a bus-wide >> reset to a random driver. > In a normal run scenario, I don't need it. I can imagine it may be a > problem, if the module > is suddenly unloaded and loaded: the mux remains set to some not default > bus and > the module is not aware of it. However, I agree then the bus could be > reset somewhere > at higher level. Think about what happens if you have more than one chip on some i2c bus that uses this reset mechanism. First you instantiate one of the drivers and both chips are reset. Then you carry on with careful configuration of that first chip. Then you instantiate the driver for the other chip and all you careful configuration of the first chip is lost as it is reset a second time. So, you just can't reset everything on the bus in any random driver, that has to be done on some other level. >>> The manufacturer ID and device ID checks in pca984x_family_probe ensures that >>> we actually communicate with a device we intended to (according to the >>> devicetree) and that communication with this device works properly (this device >>> ID and manufacturer ID can be seen as a fixed pattern). >> Yes yes yes, but do you actually need it? Because it complicates the driver >> for little gain (in my opinion). > Well, yes... I want to be sure that the driver is actually able to serve > the device which is going > to be assigned to it. The best, of course, would be if I2C could > enumerate itself (like PCIe), but > as it's not a case and we need to rely on devicetree, I understand the > driver should do "maximum" > to cross-check the static configuration with the actual hardware. > Moreover, in case of a configuration problem, one will be able to trace it > immediately. > By default, the driver performs a dummy access anyway. Since new chips > come with those ID > registers, I think it's reasonable to do something useful. Sure, there are benefits, but is it worth it? >>> + >>> + /* Get device ID */ >>> + device_id_raw.block[0] = 3; /* read 3 bytes */ >>> + if (i2c_smbus_xfer(client->adapter, DEVICE_ID_ADDRESS, client->flags, >>> + I2C_SMBUS_READ, client->addr << 1, >>> + I2C_SMBUS_I2C_BLOCK_DATA, &device_id_raw)) { >>> >> Whooa, I managed to miss this last time... Why are you not using the >> i2c_smbus_read_i2c_block_data helper? (and even if you have some good >> reason to open-code this, you seem to have some seriously confused >> arguments, so if those are indeed correct you need to document what >> the hell is going on) >> > The deviceID access is described in paragraph 6.2.2 of the manual > (https://www.nxp.com/docs/en/data-sheet/PCA9846.pdf). > The fixed DEVICE_ID_ADDRESS (0x7C) is used as address. The > i2c_smbus_read_i2c_block_data function uses > address of a client (client->addr). Ahh, ok, I see what's going on. I think this should be a helper function in the i2c core that returns the manufacturer, the device id and the revision fields, thus eliminating the need to do bit-fiddling in every driver that needs this info. And there should be a common "database" of manufacturers. It would be a disservice to everybody to have that info scattered all over the place. Cheers, Peter
On 12.12.2017 at 16:25, Peter Rosin wrote: > On 2017-12-12 13:06, Adrian Fiergolski wrote: >> On 11.12.2017 at 20:14, Peter Rosin wrote: >>>> a newer PCA984x family of the I2C switches and multiplexers from NXP. >>>> In probe function, the patch supports device ID function, introduced in the >>>> new family, and checks it against configuration provided by the >>>> devicetree. Moreover, it also performs software reset which is now >>>> available for the new devices. >>>> The reset of the code remains common with the legacy PCA954x devices. >>>> >>>> Signed-off-by: Adrian Fiergolski <adrian.fiergolski@cern.ch> >>>> --- >>>> Apply Peter's and Rodolfo's comments. >>>> Add missing part in pca984x_family_probe which checks deviceID. >>>> >>>> Peter addressing your main comments: >>>> >>>> Quoting the PCA9848 manual: "The Software Reset Call allows all the devices in >>>> the I2C-bus to be reset to the power-up state value through a specific formatted >>>> I2C-bus command. To be performed correctly, it implies that the I2C-bus is >>>> functional and that there is no device hanging the bus." >>>> This call should reset the multiplexer and all devices which are below it in the >>>> I2C address space tree to the default state. >>> Yes, I read all that in the datasheet, but why do *you* need it? Because >>> it is wrong to do it in this driver. It is doubly wrong to do it >>> unconditionally. I was asking so that I could help you find a solution >>> for your problem, because it is simply not acceptable to add a bus-wide >>> reset to a random driver. >> In a normal run scenario, I don't need it. I can imagine it may be a >> problem, if the module >> is suddenly unloaded and loaded: the mux remains set to some not default >> bus and >> the module is not aware of it. However, I agree then the bus could be >> reset somewhere >> at higher level. > Think about what happens if you have more than one chip on some i2c bus > that uses this reset mechanism. First you instantiate one of the drivers > and both chips are reset. Then you carry on with careful configuration > of that first chip. Then you instantiate the driver for the other chip > and all you careful configuration of the first chip is lost as it is > reset a second time. So, you just can't reset everything on the bus in > any random driver, that has to be done on some other level. That's all true. However, we are discussing an I2C mux/switch which is a root of an I2C sub-tree. It is initialized first, so the SOFTWARE_RESET command would reset only mux/switch and all its nodes. The command would be followed by the configuration of all its nodes, which anyway can't be performed earlier. > >>>> The manufacturer ID and device ID checks in pca984x_family_probe ensures that >>>> we actually communicate with a device we intended to (according to the >>>> devicetree) and that communication with this device works properly (this device >>>> ID and manufacturer ID can be seen as a fixed pattern). >>> Yes yes yes, but do you actually need it? Because it complicates the driver >>> for little gain (in my opinion). >> Well, yes... I want to be sure that the driver is actually able to serve >> the device which is going >> to be assigned to it. The best, of course, would be if I2C could >> enumerate itself (like PCIe), but >> as it's not a case and we need to rely on devicetree, I understand the >> driver should do "maximum" >> to cross-check the static configuration with the actual hardware. >> Moreover, in case of a configuration problem, one will be able to trace it >> immediately. >> By default, the driver performs a dummy access anyway. Since new chips >> come with those ID >> registers, I think it's reasonable to do something useful. > Sure, there are benefits, but is it worth it? As we perform anyway a dummy access, we suffer anyway I2C bus delay which will be, in case of PCA984x family, in the same order of magnitude (1 vs 3 words access). On another hand, one extra call doesn't change significantly, in my opinion, the code complexity. >>>> + >>>> + /* Get device ID */ >>>> + device_id_raw.block[0] = 3; /* read 3 bytes */ >>>> + if (i2c_smbus_xfer(client->adapter, DEVICE_ID_ADDRESS, client->flags, >>>> + I2C_SMBUS_READ, client->addr << 1, >>>> + I2C_SMBUS_I2C_BLOCK_DATA, &device_id_raw)) { >>>> >>> Whooa, I managed to miss this last time... Why are you not using the >>> i2c_smbus_read_i2c_block_data helper? (and even if you have some good >>> reason to open-code this, you seem to have some seriously confused >>> arguments, so if those are indeed correct you need to document what >>> the hell is going on) >>> >> The deviceID access is described in paragraph 6.2.2 of the manual >> (https://www.nxp.com/docs/en/data-sheet/PCA9846.pdf). >> The fixed DEVICE_ID_ADDRESS (0x7C) is used as address. The >> i2c_smbus_read_i2c_block_data function uses >> address of a client (client->addr). > Ahh, ok, I see what's going on. I think this should be a helper > function in the i2c core that returns the manufacturer, the device id > and the revision fields, thus eliminating the need to do bit-fiddling > in every driver that needs this info. > > And there should be a common "database" of manufacturers. It would be > a disservice to everybody to have that info scattered all over the > place. So how would we like to proceed? We keep for a time being an i2c_smbus_xfer call and in parallel start implementation of a new helper function in I2C core which would substitute this call in the future? Cheers, Adrian
[Adding Wolfram] On 2017-12-12 18:14, Adrian Fiergolski wrote: > On 12.12.2017 at 16:25, Peter Rosin wrote: >> On 2017-12-12 13:06, Adrian Fiergolski wrote: >>> In a normal run scenario, I don't need it. I can imagine it may be a >>> problem, if the module >>> is suddenly unloaded and loaded: the mux remains set to some not default >>> bus and >>> the module is not aware of it. However, I agree then the bus could be >>> reset somewhere >>> at higher level. >> Think about what happens if you have more than one chip on some i2c bus >> that uses this reset mechanism. First you instantiate one of the drivers >> and both chips are reset. Then you carry on with careful configuration >> of that first chip. Then you instantiate the driver for the other chip >> and all you careful configuration of the first chip is lost as it is >> reset a second time. So, you just can't reset everything on the bus in >> any random driver, that has to be done on some other level. > That's all true. However, we are discussing an I2C mux/switch which is a > root > of an I2C sub-tree. It is initialized first, so the SOFTWARE_RESET command > would reset only mux/switch and all its nodes. The command would be followed > by the configuration of all its nodes, which anyway can't be performed > earlier. Have a look at Documentation/i2c/i2c-topology and think again. >>> Well, yes... I want to be sure that the driver is actually able to serve >>> the device which is going >>> to be assigned to it. The best, of course, would be if I2C could >>> enumerate itself (like PCIe), but >>> as it's not a case and we need to rely on devicetree, I understand the >>> driver should do "maximum" >>> to cross-check the static configuration with the actual hardware. >>> Moreover, in case of a configuration problem, one will be able to trace it >>> immediately. >>> By default, the driver performs a dummy access anyway. Since new chips >>> come with those ID >>> registers, I think it's reasonable to do something useful. >> Sure, there are benefits, but is it worth it? > As we perform anyway a dummy access, we suffer anyway I2C bus delay > which will be, in case of PCA984x family, in the same order of magnitude > (1 vs 3 words access). On another hand, one extra call doesn't change > significantly, in my opinion, the code complexity. The cost I'm concerned about isn't the time it takes to probe, the concern is code. Given that I don't believe the code to check the manufacturer/device using the special 0x7c address belongs in this file at all, it seems like the best plan is to... >>> The deviceID access is described in paragraph 6.2.2 of the manual >>> (https://www.nxp.com/docs/en/data-sheet/PCA9846.pdf). >>> The fixed DEVICE_ID_ADDRESS (0x7C) is used as address. The >>> i2c_smbus_read_i2c_block_data function uses >>> address of a client (client->addr). >> Ahh, ok, I see what's going on. I think this should be a helper >> function in the i2c core that returns the manufacturer, the device id >> and the revision fields, thus eliminating the need to do bit-fiddling >> in every driver that needs this info. >> >> And there should be a common "database" of manufacturers. It would be >> a disservice to everybody to have that info scattered all over the >> place. > So how would we like to proceed? We keep for a time being > an i2c_smbus_xfer call and in parallel start implementation of > a new helper function in I2C core which would substitute this > call in the future? ...simply skip it until someone has implemented support for it in the I2C core. You could be the one to do that. Or not, if you don't feel that you want to. Wolfram, do you have an opinion on the device id check using the 0x7c address? Cheers, Peter
On 2017-12-11 20:14, Peter Rosin wrote: > On 2017-12-11 17:58, Adrian Fiergolski wrote: >> + /* Check manufacturer ID (12 bits) */ >> + manufacturer_id = ((u16) device_id_raw.block[1] << 4) | (device_id_raw.block[2] >> 4); > > This assignment is still broken... Ooops, no, this one is fine. Sorry about that. >> + if (manufacturer_id != NXP_ID) { >> + dev_warn(&client->dev, "PCA9846 family: Manufacturer ID does not match NXP\n"); >> + return -ENODEV; >> + } >> + >> + /* Check device ID (9 bits) */ >> + device_id = ((u16) device_id_raw.block[2] << 5) | (device_id_raw.block[3] >> 3); > > ...and this is also broken. But this one is broken. You need to mask out the upper half of ...block[2] so that the lower bits of the manufacturer don't end up above the device id bits. Cheers, Peter >> + if (device_id != chips[id->driver_data].deviceID) { >> + dev_warn(&client->dev, "PCA9846 family: Device ID does not match %s\n", id->name); >> + return -ENODEV; >> + }
> Wolfram, do you have an opinion on the device id check using the > 0x7c address? I always thought there should be some core support for that feature, but IIRC I never knew of any device which had this implemented. But since it is described in the I2C Specification, it should be in core IMO.
On 12.12.2017 at 20:03, Peter Rosin wrote: > [Adding Wolfram] > > On 2017-12-12 18:14, Adrian Fiergolski wrote: >> On 12.12.2017 at 16:25, Peter Rosin wrote: >>> On 2017-12-12 13:06, Adrian Fiergolski wrote: >>>> In a normal run scenario, I don't need it. I can imagine it may be a >>>> problem, if the module >>>> is suddenly unloaded and loaded: the mux remains set to some not default >>>> bus and >>>> the module is not aware of it. However, I agree then the bus could be >>>> reset somewhere >>>> at higher level. >>> Think about what happens if you have more than one chip on some i2c bus >>> that uses this reset mechanism. First you instantiate one of the drivers >>> and both chips are reset. Then you carry on with careful configuration >>> of that first chip. Then you instantiate the driver for the other chip >>> and all you careful configuration of the first chip is lost as it is >>> reset a second time. So, you just can't reset everything on the bus in >>> any random driver, that has to be done on some other level. >> That's all true. However, we are discussing an I2C mux/switch which is a >> root >> of an I2C sub-tree. It is initialized first, so the SOFTWARE_RESET command >> would reset only mux/switch and all its nodes. The command would be followed >> by the configuration of all its nodes, which anyway can't be performed >> earlier. > Have a look at Documentation/i2c/i2c-topology and think again. And... ? Cheers, Adrian
On 2017-12-13 09:47, Adrian Fiergolski wrote: > On 12.12.2017 at 20:03, Peter Rosin wrote: >> [Adding Wolfram] >> >> On 2017-12-12 18:14, Adrian Fiergolski wrote: >>> On 12.12.2017 at 16:25, Peter Rosin wrote: >>>> On 2017-12-12 13:06, Adrian Fiergolski wrote: >>>>> In a normal run scenario, I don't need it. I can imagine it may be a >>>>> problem, if the module >>>>> is suddenly unloaded and loaded: the mux remains set to some not default >>>>> bus and >>>>> the module is not aware of it. However, I agree then the bus could be >>>>> reset somewhere >>>>> at higher level. >>>> Think about what happens if you have more than one chip on some i2c bus >>>> that uses this reset mechanism. First you instantiate one of the drivers >>>> and both chips are reset. Then you carry on with careful configuration >>>> of that first chip. Then you instantiate the driver for the other chip >>>> and all you careful configuration of the first chip is lost as it is >>>> reset a second time. So, you just can't reset everything on the bus in >>>> any random driver, that has to be done on some other level. >>> That's all true. However, we are discussing an I2C mux/switch which is a >>> root >>> of an I2C sub-tree. It is initialized first, so the SOFTWARE_RESET command >>> would reset only mux/switch and all its nodes. The command would be followed >>> by the configuration of all its nodes, which anyway can't be performed >>> earlier. >> Have a look at Documentation/i2c/i2c-topology and think again. > And... ? If you have a topology like this (use a fixed-width font): .-- deviceA | i2c-root--+ deviceB | / '--pca984x \ deviceC and linux for some reason starts with instantiating the driver for deviceA, and after that instantiates the pca984x driver, then you obviously have a problem if deviceA reacts to a general call to reset done by the probe in the pca984x driver. Or if you have this: deviceA / pca984x / \ i2c-root----pca984x deviceB \ deviceC then the driver for the first level pca984x device is instantiated first, but the general call to reset when the second level pca984x device is instantiated will trash the already configured first level pca984x device. Cheers, Peter
On 13.12.2017 at 10:39, Peter Rosin wrote: > On 2017-12-13 09:47, Adrian Fiergolski wrote: >> On 12.12.2017 at 20:03, Peter Rosin wrote: >>> [Adding Wolfram] >>> >>> On 2017-12-12 18:14, Adrian Fiergolski wrote: >>>> On 12.12.2017 at 16:25, Peter Rosin wrote: >>>>> On 2017-12-12 13:06, Adrian Fiergolski wrote: >>>>>> In a normal run scenario, I don't need it. I can imagine it may be a >>>>>> problem, if the module >>>>>> is suddenly unloaded and loaded: the mux remains set to some not default >>>>>> bus and >>>>>> the module is not aware of it. However, I agree then the bus could be >>>>>> reset somewhere >>>>>> at higher level. >>>>> Think about what happens if you have more than one chip on some i2c bus >>>>> that uses this reset mechanism. First you instantiate one of the drivers >>>>> and both chips are reset. Then you carry on with careful configuration >>>>> of that first chip. Then you instantiate the driver for the other chip >>>>> and all you careful configuration of the first chip is lost as it is >>>>> reset a second time. So, you just can't reset everything on the bus in >>>>> any random driver, that has to be done on some other level. >>>> That's all true. However, we are discussing an I2C mux/switch which is a >>>> root >>>> of an I2C sub-tree. It is initialized first, so the SOFTWARE_RESET command >>>> would reset only mux/switch and all its nodes. The command would be followed >>>> by the configuration of all its nodes, which anyway can't be performed >>>> earlier. >>> Have a look at Documentation/i2c/i2c-topology and think again. >> And... ? > If you have a topology like this (use a fixed-width font): > > .-- deviceA > | > i2c-root--+ deviceB > | / > '--pca984x > \ > deviceC > > and linux for some reason starts with instantiating the driver for deviceA, > and after that instantiates the pca984x driver, then you obviously have a > problem if deviceA reacts to a general call to reset done by the probe in > the pca984x driver. > > Or if you have this: > > deviceA > / > pca984x > / \ > i2c-root----pca984x deviceB > \ > deviceC > > then the driver for the first level pca984x device is instantiated first, > but the general call to reset when the second level pca984x device is > instantiated will trash the already configured first level pca984x > device. > You are right. I checked addressing in this RESET call again. For some reasons, I was sure that this GENERAL_CALL is performed on the I2C buses mastered by the pca984x device. Of course, this call is performed on its slave interface. It couldn't be different. Thanks, Adrian
On 12.12.2017 at 23:05, Wolfram Sang wrote: > >> Wolfram, do you have an opinion on the device id check using the >> 0x7c address? > I always thought there should be some core support for that feature, but > IIRC I never knew of any device which had this implemented. But since it > is described in the I2C Specification, it should be in core IMO. > I think one could simply use the code from my patch v3. However, I haven't found any example of such dedicated function (i.e GENERAL_CALL) in the core. Do you have in mind sth like i2c_smbus_write_i2c_device_device_id function ? Cheers, Adrian
> Do you have in mind sth like i2c_smbus_write_i2c_device_device_id function ?
SMBus? It's I2C. And write? I'd call it:
u32 i2c_get_device_id(struct client* client)
Instead of returning an u32, we could return a struct where the bits are
already decoded, too.
diff --git a/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt b/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt index aa097045a10e..b428bc0d81b1 100644 --- a/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt +++ b/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt @@ -1,10 +1,13 @@ * NXP PCA954x I2C bus switch +The driver supports NXP PCA954x and PCA984x I2C mux/switch devices. + Required Properties: - compatible: Must contain one of the following. "nxp,pca9540", "nxp,pca9542", "nxp,pca9543", "nxp,pca9544", - "nxp,pca9545", "nxp,pca9546", "nxp,pca9547", "nxp,pca9548" + "nxp,pca9545", "nxp,pca9546", "nxp,pca9547", "nxp,pca9548", + "nxp,pca9846", "nxp,pca9847", "nxp,pca9848", "nxp,pca9849" - reg: The I2C address of the device. diff --git a/drivers/i2c/muxes/Kconfig b/drivers/i2c/muxes/Kconfig index 0f5c8fc36625..5da2e04585a9 100644 --- a/drivers/i2c/muxes/Kconfig +++ b/drivers/i2c/muxes/Kconfig @@ -68,7 +68,7 @@ config I2C_MUX_PCA954x depends on GPIOLIB || COMPILE_TEST help If you say yes here you get support for the Philips PCA954x - I2C mux/switch devices. + and PCA984x I2C mux/switch devices. This driver can also be built as a module. If so, the module will be called i2c-mux-pca954x. diff --git a/drivers/i2c/muxes/i2c-mux-pca954x.c b/drivers/i2c/muxes/i2c-mux-pca954x.c index 2ca068d8b92d..6205ae52aa4d 100644 --- a/drivers/i2c/muxes/i2c-mux-pca954x.c +++ b/drivers/i2c/muxes/i2c-mux-pca954x.c @@ -1,14 +1,15 @@ /* * I2C multiplexer * + * Copyright (c) 2017 Adrian Fiergolski <Adrian.Fiergolski@cern.ch> * Copyright (c) 2008-2009 Rodolfo Giometti <giometti@linux.it> * Copyright (c) 2008-2009 Eurotech S.p.A. <info@eurotech.it> * - * This module supports the PCA954x series of I2C multiplexer/switch chips - * made by Philips Semiconductors. + * This module supports the PCA954x and PCA954x series of I2C multiplexer/switch + * chips made by Philips Semiconductors. * This includes the: - * PCA9540, PCA9542, PCA9543, PCA9544, PCA9545, PCA9546, PCA9547 - * and PCA9548. + * PCA9540, PCA9542, PCA9543, PCA9544, PCA9545, PCA9546, PCA9547, + * PCA9548, PCA9846, PCA9847, PCA9848 and PCA9849 * * These chips are all controlled via the I2C bus itself, and all have a * single 8-bit register. The upstream "parent" bus fans out to two, @@ -54,6 +55,12 @@ #define PCA954X_IRQ_OFFSET 4 +/* PCA984x family I2C other addresses */ +#define GENERAL_CALL 0x00 +#define SOFTWARE_RESET 0x06 +#define DEVICE_ID_ADDRESS 0x7C +#define NXP_ID 0x00 + enum pca_type { pca_9540, pca_9542, @@ -63,6 +70,10 @@ enum pca_type { pca_9546, pca_9547, pca_9548, + pca_9846, + pca_9847, + pca_9848, + pca_9849, }; struct chip_desc { @@ -73,6 +84,7 @@ struct chip_desc { pca954x_ismux = 0, pca954x_isswi } muxtype; + u16 deviceID; /* used by PCA984x family only */ }; struct pca954x { @@ -129,6 +141,29 @@ static const struct chip_desc chips[] = { .nchans = 8, .muxtype = pca954x_isswi, }, + [pca_9846] = { + .nchans = 4, + .muxtype = pca954x_isswi, + .deviceID = 0x10B, + }, + + [pca_9847] = { + .nchans = 8, + .muxtype = pca954x_ismux, + .deviceID = 0x108, + }, + + [pca_9848] = { + .nchans = 8, + .muxtype = pca954x_isswi, + .deviceID = 0x10A, + }, + + [pca_9849] = { + .nchans = 4, + .muxtype = pca954x_ismux, + .deviceID = 0x109, + }, }; static const struct i2c_device_id pca954x_id[] = { @@ -140,6 +175,10 @@ static const struct i2c_device_id pca954x_id[] = { { "pca9546", pca_9546 }, { "pca9547", pca_9547 }, { "pca9548", pca_9548 }, + { "pca9846", pca_9846 }, + { "pca9847", pca_9847 }, + { "pca9848", pca_9848 }, + { "pca9849", pca_9849 }, { } }; MODULE_DEVICE_TABLE(i2c, pca954x_id); @@ -154,6 +193,10 @@ static const struct of_device_id pca954x_of_match[] = { { .compatible = "nxp,pca9546", .data = &chips[pca_9546] }, { .compatible = "nxp,pca9547", .data = &chips[pca_9547] }, { .compatible = "nxp,pca9548", .data = &chips[pca_9548] }, + { .compatible = "nxp,pca9846", .data = &chips[pca_9846] }, + { .compatible = "nxp,pca9847", .data = &chips[pca_9847] }, + { .compatible = "nxp,pca9848", .data = &chips[pca_9848] }, + { .compatible = "nxp,pca9849", .data = &chips[pca_9849] }, {} }; MODULE_DEVICE_TABLE(of, pca954x_of_match); @@ -304,6 +347,83 @@ static void pca954x_cleanup(struct i2c_mux_core *muxc) i2c_mux_del_adapters(muxc); } +/* + * Part of probe function specific for pca954x family + */ +static int pca954x_family_probe(struct i2c_client *client, + const struct i2c_device_id *id) +{ + + /* Write the mux register at addr to verify + * that the mux is in fact present. This also + * initializes the mux to disconnected state. + */ + if (i2c_smbus_write_byte(client, 0) < 0) + return -ENODEV; + + return 0; +} + +/* + * Part of probe function specific for pca984x family + */ +static int pca984x_family_probe(struct i2c_client *client, + const struct i2c_device_id *id) +{ + struct i2c_adapter *adap = to_i2c_adapter(client->dev.parent); + union i2c_smbus_data device_id_raw; + u16 manufacturer_id; /* 12 bits */ + u16 device_id; /* 9 bits */ + + if (!i2c_check_functionality(adap, I2C_FUNC_SMBUS_WRITE_BYTE_DATA)) { + dev_warn(&client->dev, "PCA9846 family: I2c adapter doesn't support I2C_FUNC_SMBUS_WRITE_BYTE_DATA"); + return -ENODEV; + } + + if (!i2c_check_functionality(adap, I2C_FUNC_SMBUS_READ_I2C_BLOCK)) { + dev_warn(&client->dev, "PCA9846 family: I2c adapter doesn't support I2C_FUNC_SMBUS_READ_I2C_BLOCK"); + return -ENODEV; + } + + /* Software reset */ + if (i2c_smbus_xfer(client->adapter, GENERAL_CALL, client->flags, + I2C_SMBUS_WRITE, SOFTWARE_RESET, I2C_SMBUS_BYTE, NULL) < 0) { + dev_warn(&client->dev, "PCA9846 family: Sofrware reset failed\n"); + return -ENODEV; + } + + /* Get device ID */ + device_id_raw.block[0] = 3; /* read 3 bytes */ + if (i2c_smbus_xfer(client->adapter, DEVICE_ID_ADDRESS, client->flags, + I2C_SMBUS_READ, client->addr << 1, + I2C_SMBUS_I2C_BLOCK_DATA, &device_id_raw)) { + dev_warn(&client->dev, "PCA9846 family: Get device ID failed\n"); + return -ENODEV; + } + + /* Device ID contains only 3 bytes */ + if (device_id_raw.block[0] != 3) { + dev_warn(&client->dev, "PCA9846 family: Get device ID failed\n"); + return -ENODEV; + } + + /* Check manufacturer ID (12 bits) */ + manufacturer_id = ((u16) device_id_raw.block[1] << 4) | (device_id_raw.block[2] >> 4); + if (manufacturer_id != NXP_ID) { + dev_warn(&client->dev, "PCA9846 family: Manufacturer ID does not match NXP\n"); + return -ENODEV; + } + + /* Check device ID (9 bits) */ + device_id = ((u16) device_id_raw.block[2] << 5) | (device_id_raw.block[3] >> 3); + if (device_id != chips[id->driver_data].deviceID) { + dev_warn(&client->dev, "PCA9846 family: Device ID does not match %s\n", id->name); + return -ENODEV; + } + + return 0; +} + /* * I2C init/probing/exit functions */ @@ -339,11 +459,29 @@ static int pca954x_probe(struct i2c_client *client, if (IS_ERR(gpio)) return PTR_ERR(gpio); - /* Write the mux register at addr to verify - * that the mux is in fact present. This also - * initializes the mux to disconnected state. - */ - if (i2c_smbus_write_byte(client, 0) < 0) { + switch (id->driver_data) { + case pca_9540: + case pca_9542: + case pca_9543: + case pca_9544: + case pca_9545: + case pca_9546: + case pca_9547: + case pca_9548: + ret = pca954x_family_probe(client, id); + break; + case pca_9846: + case pca_9847: + case pca_9848: + case pca_9849: + ret = pca984x_family_probe(client, id); + break; + default: /* unknown device */ + ret = -ENODEV; + break; + } + + if (ret < 0) { dev_warn(&client->dev, "probe failed\n"); return -ENODEV; } @@ -443,6 +581,7 @@ static struct i2c_driver pca954x_driver = { module_i2c_driver(pca954x_driver); +MODULE_AUTHOR("Adrian Fiergolski <Adrian.Fiergolski@cern.ch>"); MODULE_AUTHOR("Rodolfo Giometti <giometti@linux.it>"); MODULE_DESCRIPTION("PCA954x I2C mux/switch driver"); MODULE_LICENSE("GPL v2");
This patch exetends the current i2c-mux-pca954x driver and adds suuport for a newer PCA984x family of the I2C switches and multiplexers from NXP. In probe function, the patch supports device ID function, introduced in the new family, and checks it against configuration provided by the devicetree. Moreover, it also performs software reset which is now available for the new devices. The reset of the code remains common with the legacy PCA954x devices. Signed-off-by: Adrian Fiergolski <adrian.fiergolski@cern.ch> --- Apply Peter's and Rodolfo's comments. Add missing part in pca984x_family_probe which checks deviceID. Peter addressing your main comments: Quoting the PCA9848 manual: "The Software Reset Call allows all the devices in the I2C-bus to be reset to the power-up state value through a specific formatted I2C-bus command. To be performed correctly, it implies that the I2C-bus is functional and that there is no device hanging the bus." This call should reset the multiplexer and all devices which are below it in the I2C address space tree to the default state. The manufacturer ID and device ID checks in pca984x_family_probe ensures that we actually communicate with a device we intended to (according to the devicetree) and that communication with this device works properly (this device ID and manufacturer ID can be seen as a fixed pattern). .../devicetree/bindings/i2c/i2c-mux-pca954x.txt | 5 +- drivers/i2c/muxes/Kconfig | 2 +- drivers/i2c/muxes/i2c-mux-pca954x.c | 157 +++++++++++++++++++-- 3 files changed, 153 insertions(+), 11 deletions(-)