Date:   Wed, 23 Mar 2022 13:54:01 +0100
From:   Peter Rosin <peda@...ntia.se>
To:     Patrick Rudolph <patrick.rudolph@...ements.com>
Cc:     linux-i2c@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [v6 2/3] i2c: muxes: pca954x: Add MAX735x/MAX736x support

On 2022-03-23 13:41, Patrick Rudolph wrote:
> Hi Peter,
> thanks for the review.
> The MAX7358 has indeed the same registers as the MAX7357, but they
> need to be "unlocked" by sending a magic sequence first.
> As this isn't implemented by the driver it behaves like the MAX7356
> with a single register.
> The additional registers can be hidden again by setting a bit in the
> config space.
> Which wording would you prefer to describe this feature?

Perhaps: "exposed at POR" -> "exposed without extra commands"
or something like that. Without the background that the chips
are different like this at POR, the mention of POR is just
confusing...

But why not send this sequence for MAX7358? Then it occurred to me
that I expect the MAX7357 to behave pretty much like any of the
other chips until you touch the enhanced registers. Isn't that the
case? But why is it then needed to touch the enhanced registers at
all?

I suspect you know the answers, so I'm going the lazy route of just
asking instead of looking it up myself, hope that's ok...

Cheers,
Peter

> I'll change it to maxim_enhanced_mode.



> Regards,
> Patrick
> 
> On Sat, Mar 19, 2022 at 3:41 PM Peter Rosin <peda@...ntia.se> wrote:
>>
>> Hi!
>>
>> Sorry for the slow review and thanks for your patience...
>>
>> On 2022-02-16 08:46, Patrick Rudolph wrote:
>>> Add support for the following Maxim chips using the existing PCA954x
>>> driver:
>>> - MAX7356
>>> - MAX7357
>>> - MAX7358
>>> - MAX7367
>>> - MAX7368
>>> - MAX7369
>>>
>>> All added Maxim chips behave like the PCA954x, where a single SMBUS byte
>>> write selects up to 8 channels to be bridged to the primary bus.
>>>
>>> The MAX7357 exposes 6 additional registers at Power-On-Reset and is
>>
>> MAX7358 also has the same enhanced mode as the 7357, no?
>>
>> And what do you mean that they are exposed at POR? I can see why they
>> are not exposed /before/ POR, but are they ever /not/ exposed? If they
>> are always exposed when the chip is "alive", then I suggest that the
>> POR wording is dropped, otherwise that the above is reworded to
>> describe when the register are no longer exposed.
>>
>>> configured to:
>>>  - Disabled interrupts on bus locked up detection
>>>  - Enable bus locked-up clearing
>>>  - Disconnect only locked bus instead of all channels
>>>
>>> While the MAX7357/MAX7358 have interrupt support, they don't act as
>>> interrupt controller like the PCA9545 does. Thus don't enable IRQ support
>>> and handle them like the PCA9548.
>>>
>>> Tested using the MAX7357 and verified that the stalled bus is disconnected
>>> while the other channels remain operational.
>>>
>>> Signed-off-by: Patrick Rudolph <patrick.rudolph@...ements.com>
>>> ---
>>>  drivers/i2c/muxes/Kconfig           |  4 +-
>>>  drivers/i2c/muxes/i2c-mux-pca954x.c | 92 +++++++++++++++++++++++++++--
>>>  2 files changed, 90 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/i2c/muxes/Kconfig b/drivers/i2c/muxes/Kconfig
>>> index 1708b1a82da2..2ac99d044199 100644
>>> --- a/drivers/i2c/muxes/Kconfig
>>> +++ b/drivers/i2c/muxes/Kconfig
>>> @@ -65,11 +65,11 @@ config I2C_MUX_PCA9541
>>>         will be called i2c-mux-pca9541.
>>>
>>>  config I2C_MUX_PCA954x
>>> -     tristate "NXP PCA954x and PCA984x I2C Mux/switches"
>>> +     tristate "NXP PCA954x/PCA984x and Maxim MAX735x/MAX736x I2C Mux/switches"
>>>       depends on GPIOLIB || COMPILE_TEST
>>>       help
>>>         If you say yes here you get support for the NXP PCA954x
>>> -       and PCA984x I2C mux/switch devices.
>>> +       and PCA984x and Maxim MAX735x/MAX736x I2C mux/switch devices.
>>
>> and and and... :-) Maybe like this?
>>
>>           If you say yes here you get support for NXP PCA954x/PCA984x
>>           and Maxim MAX735x/MAX736x 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 4ad665757dd8..33b9a6a1fffa 100644
>>> --- a/drivers/i2c/muxes/i2c-mux-pca954x.c
>>> +++ b/drivers/i2c/muxes/i2c-mux-pca954x.c
>>> @@ -4,6 +4,7 @@
>>>   *
>>>   * Copyright (c) 2008-2009 Rodolfo Giometti <giometti@...ux.it>
>>>   * Copyright (c) 2008-2009 Eurotech S.p.A. <info@...otech.it>
>>> + * Copyright (c) 2022 Patrick Rudolph <patrick.rudolph@...ements.com>
>>>   *
>>>   * This module supports the PCA954x and PCA984x series of I2C multiplexer/switch
>>>   * chips made by NXP Semiconductors.
>>> @@ -11,6 +12,12 @@
>>>   *    PCA9540, PCA9542, PCA9543, PCA9544, PCA9545, PCA9546, PCA9547,
>>>   *    PCA9548, PCA9846, PCA9847, PCA9848 and PCA9849.
>>>   *
>>> + * It's also compatible to Maxims MAX735x I2C switch chips, which are controlled
>>> + * as the NXP PCA9548 and the MAX736x chips that act like the PCA9544.
>>> + *
>>> + * This includes the:
>>> + *    MAX7356, MAX7357, MAX7358, MAX7367, MAX7368 and MAX7369
>>> + *
>>>   * 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,
>>>   * four, or eight downstream busses or channels; which of these
>>> @@ -50,7 +57,30 @@
>>>
>>>  #define PCA954X_IRQ_OFFSET 4
>>>
>>> +/*
>>> + * MAX7357 exposes 7 registers on POR which allow to configure additional
>>> + * features. Disable interrupts, enable bus locked-up clearing,
>>> + * isolate only the locked channel instead of all channels.
>>
>> Same MAX7358 and POR comments as above.
>>
>> The way I understands things are:
>>
>>  * MAX7357/MAX7358 exposes 7 registers which allow setup of
>>  * enhanced mode features. The first of these registers is the
>>  * switch control register that is present in some form on all
>>  * chips supported by this driver.
>>  * The second register is the configuration register, which allows
>>  * to configure additional features. E.g. disable interrupts,
>>  * enable bus locked-up clearing and isolate only the locked
>>  * channel instead of all channels.
>>  * The remaining 5 registers are left as is by this driver.
>>
>>> + */
>>> +#define MAX7357_CONF_INT_ENABLE                      BIT(0)
>>> +#define MAX7357_CONF_FLUSH_OUT                       BIT(1)
>>> +#define MAX7357_CONF_RELEASE_INT             BIT(2)
>>> +#define MAX7357_CONF_LOCK_UP_CLEAR           BIT(3)
>>> +#define MAX7357_CONF_DISCON_SINGLE_CHAN              BIT(4)
>>> +#define MAX7357_CONF_BUS_LOCKUP_DETECTION    BIT(5)
>>> +#define MAX7357_CONF_ENABLE_BASIC_MODE               BIT(6)
>>> +#define MAX7357_CONF_PRECONNECT_TEST         BIT(7)
>>> +
>>> +#define MAX7357_CONF_DEFAULTS (MAX7357_CONF_FLUSH_OUT | \
>>> +      MAX7357_CONF_DISCON_SINGLE_CHAN)
>>> +
>>>  enum pca_type {
>>> +     max_7367,
>>> +     max_7368,
>>> +     max_7369,
>>> +     max_7356,
>>> +     max_7357,
>>> +     max_7358,
>>>       pca_9540,
>>>       pca_9542,
>>>       pca_9543,
>>> @@ -69,6 +99,7 @@ struct chip_desc {
>>>       u8 nchans;
>>>       u8 enable;      /* used for muxes only */
>>>       u8 has_irq;
>>> +     u8 max7357;
>>
>> Perhaps maxim_enhanced_mode is a better name?
>>
>>>       enum muxtype {
>>>               pca954x_ismux = 0,
>>>               pca954x_isswi
>>> @@ -90,8 +121,42 @@ struct pca954x {
>>>       raw_spinlock_t lock;
>>>  };
>>>
>>> -/* Provide specs for the PCA954x types we know about */
>>> +/* Provide specs for the PCA954x and MAX735x types we know about */
>>>  static const struct chip_desc chips[] = {
>>> +     [max_7356] = {
>>> +             .nchans = 8,
>>> +             .muxtype = pca954x_isswi,
>>> +             .id = { .manufacturer_id = I2C_DEVICE_ID_NONE },
>>> +     },
>>> +     [max_7357] = {
>>> +             .nchans = 8,
>>> +             .muxtype = pca954x_isswi,
>>> +             .max7357 = 1,
>>> +             .id = { .manufacturer_id = I2C_DEVICE_ID_NONE },
>>> +     },
>>> +     [max_7358] = {
>>> +             .nchans = 8,
>>> +             .muxtype = pca954x_isswi,
>>> +             .id = { .manufacturer_id = I2C_DEVICE_ID_NONE },
>>> +     },
>>> +     [max_7367] = {
>>> +             .nchans = 4,
>>> +             .muxtype = pca954x_isswi,
>>> +             .has_irq = 1,
>>> +             .id = { .manufacturer_id = I2C_DEVICE_ID_NONE },
>>> +     },
>>> +     [max_7368] = {
>>> +             .nchans = 4,
>>> +             .muxtype = pca954x_isswi,
>>> +             .id = { .manufacturer_id = I2C_DEVICE_ID_NONE },
>>> +     },
>>> +     [max_7369] = {
>>> +             .nchans = 4,
>>> +             .enable = 0x4,
>>> +             .muxtype = pca954x_ismux,
>>> +             .has_irq = 1,
>>> +             .id = { .manufacturer_id = I2C_DEVICE_ID_NONE },
>>> +     },
>>>       [pca_9540] = {
>>>               .nchans = 2,
>>>               .enable = 0x4,
>>> @@ -177,6 +242,12 @@ static const struct chip_desc chips[] = {
>>>  };
>>>
>>>  static const struct i2c_device_id pca954x_id[] = {
>>> +     { "max7356", max_7356 },
>>> +     { "max7357", max_7357 },
>>> +     { "max7358", max_7358 },
>>> +     { "max7367", max_7367 },
>>> +     { "max7368", max_7368 },
>>> +     { "max7369", max_7369 },
>>>       { "pca9540", pca_9540 },
>>>       { "pca9542", pca_9542 },
>>>       { "pca9543", pca_9543 },
>>> @@ -194,6 +265,12 @@ static const struct i2c_device_id pca954x_id[] = {
>>>  MODULE_DEVICE_TABLE(i2c, pca954x_id);
>>>
>>>  static const struct of_device_id pca954x_of_match[] = {
>>> +     { .compatible = "maxim,max7356", .data = &chips[max_7356] },
>>> +     { .compatible = "maxim,max7357", .data = &chips[max_7357] },
>>> +     { .compatible = "maxim,max7358", .data = &chips[max_7358] },
>>> +     { .compatible = "maxim,max7367", .data = &chips[max_7367] },
>>> +     { .compatible = "maxim,max7368", .data = &chips[max_7368] },
>>> +     { .compatible = "maxim,max7369", .data = &chips[max_7369] },
>>>       { .compatible = "nxp,pca9540", .data = &chips[pca_9540] },
>>>       { .compatible = "nxp,pca9542", .data = &chips[pca_9542] },
>>>       { .compatible = "nxp,pca9543", .data = &chips[pca_9543] },
>>> @@ -401,9 +478,16 @@ static int pca954x_init(struct i2c_client *client, struct pca954x *data)
>>>       else
>>>               data->last_chan = 0; /* Disconnect multiplexer */
>>>
>>> -     ret = i2c_smbus_write_byte(client, data->last_chan);
>>> -     if (ret < 0)
>>> -             data->last_chan = 0;
>>> +     if (data->chip->max7357) {
>>> +             ret = i2c_smbus_write_byte_data(client, data->last_chan,
>>> +                                             MAX7357_CONF_DEFAULTS);
>>> +             if (ret < 0)
>>> +                     data->last_chan = 0;
>>> +     } else {
>>> +             ret = i2c_smbus_write_byte(client, data->last_chan);
>>> +             if (ret < 0)
>>> +                     data->last_chan = 0;
>>> +     }
>>>
>>>       return ret;
>>>  }
>>
>> The actual code is simple enough, and looks good.
>>
>> Cheers,
>> Peter

Powered by blists - more mailing lists