Hi Peter,
> -----Original Message-----
> From: Peter Rosin <peda@xxxxxxxxxx>
> Sent: Friday, January 08, 2021 10:02 AM
> To: Vadim Pasternak <vadimp@xxxxxxxxxx>; wsa@xxxxxxxxxxxxx
> Cc: linux-i2c@xxxxxxxxxxxxxxx
> Subject: Re: [Re-send: PATCH i2c-next 5/6] i2c: mux: mlxcpld: Extend driver to
> support word address space devices
>
> Hi!
>
> On 2021-01-07 21:43, Vadim Pasternak wrote:
> > Hi Peter,
> >
> > Thank you very much for review.
> >
> >> -----Original Message-----
> >> From: Peter Rosin <peda@xxxxxxxxxx>
> >> Sent: Thursday, January 07, 2021 12:04 PM
> >> To: Vadim Pasternak <vadimp@xxxxxxxxxx>; wsa@xxxxxxxxxxxxx
> >> Cc: linux-i2c@xxxxxxxxxxxxxxx
> >> Subject: Re: [Re-send: PATCH i2c-next 5/6] i2c: mux: mlxcpld: Extend
> >> driver to support word address space devices
> >>
> >> Hi!
> >>
> >> Again, sorry for the late review.
> >>
> >> On 2020-11-18 15:44, Vadim Pasternak wrote:
> >>> Extend driver to allow I2C routing control through CPLD devices with
> >>> word address space. Till now only CPLD devices with byte address
> >>> space have been supported.
> >>>
> >>> Signed-off-by: Vadim Pasternak <vadimp@xxxxxxxxxx>
> >>> Reviewed-by: Michael Shych <michaelsh@xxxxxxxxxx>
> >>> ---
> >>> drivers/i2c/muxes/i2c-mux-mlxcpld.c | 57
> >> +++++++++++++++++++++++++++--------
> >>> include/linux/platform_data/mlxcpld.h | 2 ++
> >>> 2 files changed, 47 insertions(+), 12 deletions(-)
> >>>
> >>> diff --git a/drivers/i2c/muxes/i2c-mux-mlxcpld.c
> >>> b/drivers/i2c/muxes/i2c-mux-mlxcpld.c
> >>> index 6bb8caecf8e8..c76180919fc3 100644
> >>> --- a/drivers/i2c/muxes/i2c-mux-mlxcpld.c
> >>> +++ b/drivers/i2c/muxes/i2c-mux-mlxcpld.c
> >>> @@ -21,11 +21,13 @@
> >>> * @last_chan - last register value
> >>> * @client - I2C device client
> >>> * @pdata: platform data
> >>> + * @sel_buf: I2C message buffer for mux select 16 bits transactions
> >>> */
> >>> struct mlxcpld_mux {
> >>> u8 last_chan;
> >>> struct i2c_client *client;
> >>> struct mlxcpld_mux_plat_data pdata;
> >>> + u8 sel_buf[3];
> >>
> >> I think it's a mistake to have this buffer here. I'd rather create a
> >> buffer on the stack in mlxcpld_mux_reg_write() and fill it with values for
> every xfer.
> >> Sure, I bet there are external locks that prevent any clobbering of
> >> the buffer, but it's so small that it really can be on the stack.
> >>
> >>> };
> >>>
> >>> /* MUX logic description.
> >>> @@ -60,26 +62,42 @@ struct mlxcpld_mux {
> >>> * for this as they will try to lock adapter a second time.
> >>> */
> >>> static int mlxcpld_mux_reg_write(struct i2c_adapter *adap,
> >>> - struct mlxcpld_mux *mux, u8 val)
> >>> + struct mlxcpld_mux *mux, int chan)
> >>> {
> >>> struct i2c_client *client = mux->client;
> >>> - union i2c_smbus_data data = { .byte = val };
> >>> -
> >>> - return __i2c_smbus_xfer(adap, client->addr, client->flags,
> >>> - I2C_SMBUS_WRITE, mux-
> >>> pdata.sel_reg_addr,
> >>> - I2C_SMBUS_BYTE_DATA, &data);
> >>> + union i2c_smbus_data data;
> >>> + struct i2c_msg msg;
> >>> +
> >>> + switch (mux->pdata.reg_size) {
> >>> + case 1:
> >>> + data.byte = (chan < 0) ? 0 : chan;
> >>> + return __i2c_smbus_xfer(adap, client->addr, client->flags,
> >>> + I2C_SMBUS_WRITE,
> >>> + mux->pdata.sel_reg_addr,
> >>> + I2C_SMBUS_BYTE_DATA, &data);
> >>> + case 2:
> >>> + mux->sel_buf[mux->pdata.reg_size] = (chan < 0) ? 0 :
> >>> + mux-
> >>> pdata.adap_ids[chan];
> >>
> >> I get the feeling that you are desperatly trying to get some specific
> >> numbering in user space.
> >>
> >> The adapter id is one thing.
> >> The mux channel is one thing.
> >> The value in the register is one thing.
> >>
> >> Often, it can make things easier with an easy mapping between the
> >> latter two, but you program the system global I2C adapter id into the
> >> channel selection register of the mux. That is problematic. Just don't.
> >
> > OK, I will explain what I am trying to get.
> > This is not something related to the user space.
> >
> > I want to access some device, located on a line card, which is replaceable.
> > This is for modular system, which can be equipped with the different
> > type of line cards.
> >
> > I have mux selector register in line card CPLD, which is located at
> > some offset in CPLD register space, f.e. 0x25dc. On other systems it could
> different offset.
> >
> > For this line card type in pdata.adap_ids[] channels mapping looks like:
> > {
> > 0x04, 0x05, 0x06, 0x07, 0x08, 0x10, 0x20, 0x21, 0x22, 0x23, 0x40, 0x41,
> > 0x42, 0x43, 0x44, 0x45, 0x46, 0x47, 0x48, 0x49, 0x4a, 0x4b, 0x4c, 0x4d,
> > 0x4e, 0x4f
> > };
> > Ids from 0x01 - 0x0f are used for access to devices like voltage regulators,
> hotswap,
> > EEPROMs, iio, etcetera.
> > Ids from 0x10 are used for FPGAs.
> > Ids from 0x20 are used for gearboxes.
> > Ids from 0x40 are used for QSFP.
> > On other line card type it could be different device tree, but it
> > still will follow the same convention.
> >
> > CPLD is connected to some upper adapter at address 0x32, and device on
> > line card is connected to adapter = base_nr * slot + pdata.adap_ids[channel].
> > For example, base_nr is 100, slot, at which line card is inserted is
> > 1, channel 0 will be be configured for adapter 104.
> >
> > And access will be as below:
> > cat /sys/bus/i2c/devices/104-0062/hwmon/hwmon5/in1_input
> > 11750
> >
> > cat-17623 [004] .... 1152583.810824: i2c_write: i2c-1 #0 a=032 f=0000
> l=3 [25-dc-04]
> > cat-17623 [004] .... 1152583.811276: i2c_result: i2c-1 n=1 ret=1
> > cat-17623 [004] .... 1152583.811281: i2c_write: i2c-1 #0 a=062 f=0000
> l=1 [88]
> > cat-17623 [004] .... 1152583.811281: i2c_read: i2c-1 #1 a=062 f=0001
> l=2
> > cat-17623 [004] .... 1152583.811700: i2c_reply: i2c-1 #1 a=062 f=0001
> l=2 [2f-f0]
> > cat-17623 [004] .... 1152583.811700: i2c_result: i2c-1 n=2 ret=2
> > cat-17623 [004] .... 1152583.811704: i2c_write: i2c-1 #0 a=032 f=0000
> l=3 [25-dc-00]
> >
> > When the same line card is inserted for example at slot 3, the
> > adapter, to which this device is connected will be 304.
>
> Yes, I think I get it. You are however not introducing base_nr until 6/6, so at
> this point the code makes no sense. But even after 6/6 with base_nr in place, I
> suggest the following:
>
> - The adap_ids array is for forceing the system global adapter id. Leave this
> variable
> alone and let it continue to do what it does, and only that. Or...
> - Since you stated somewhere that there are no users of this drivers, I'd be
> happy to just
> see the adap_ids variable deleted, i.e. I see no need to force the adapter id.
> - Instead of reusing adap_ids, intruduce a new "channel" array and fill it with
> the same
> values that you provide in adap_ids, and then have the driver feed them to
> the 3rd arg
> of i2c_mux_add_adapter(), i.e. chan_id.
>
> Would that work for you? Or do you somehow depend on predictable adapter
> ids?
I can drop adap_id[]s, use chan_ids[] instead and modify loop for adding
Adapters like:
for (num = 0; num < pdata->num_adaps; num++) {;
err = i2c_mux_add_adapter(muxc, pdata->base_nr + num,
pdata->chan_ids[num], 0);
if (err)
goto virt_reg_failed;
}
In such way I can keep convention for adapters numbering for card in slot 'n',
nrs will be:
Form 100 * n + 1 - for voltage regulators, hotswap, EEPROMs, iio, ...
>From 100 *n + 16 - for FPGAs.
>From 100 * n + 32 - for gearboxes.
>From 100 * n + 64 - for QSFP.
Would it be OK?
>
> > So I am using preconfigured buffer with mux address, which I want to right,
> here it is 0x25dc.
> > On select I write channel Id to this register (sel_buf[] = { 0x25 0xdc
> > <adap_ids[channel]>} ), on deselect zero (sel_buf[] = { 0x25 0dc 0x00 }).
> >
> > I can have a buffer on stack and set it each time
> > mlxcpld_mux_reg_write() is called, as you suggested.
> >
> > Which API you suggest to use here for sending I2C transaction?
>
> I suspect that this driver will only be used with a very limited list of I2C
> adapters, and that all of those support whatever method you use. I also
> supsect that in practice, the i2c_check_functionality() checks will always
> succeed because of this, so my comments in regard to this are probably mainly
> cosmetic. But it's easier to read code when things fit, and problems like that
> tend to "escape" when someone reuses the code.
>
> So, use whatever suits you, but make it consistent. :-)
>
> However, SMBUS has 8-bit commands/registers, so it doesn't really fit. You
> could still shoe-horn your xfers in there, if you desperately needed to support
> some SMBUS-only adapter, but I think I would have stayed with
> __i2c_transfer() for the 16-bit case.
>
> >>
> >>> + msg.addr = client->addr;
> >>> + msg.buf = mux->sel_buf;
> >>> + msg.len = mux->pdata.reg_size + 1;
> >>> + msg.flags = 0;
> >>> + return __i2c_transfer(adap, &msg, 1);
> >>
> >> Here you use I2C xfers for the 16-bit case...
> >>
> >>> + default:
> >>> + return -EINVAL;
> >>> + }
> >>> }
> >>>
> >>> static int mlxcpld_mux_select_chan(struct i2c_mux_core *muxc, u32
> >>> chan) {
> >>> struct mlxcpld_mux *mux = i2c_mux_priv(muxc);
> >>> - u8 regval = chan + 1;
> >>> int err = 0;
> >>>
> >>> /* Only select the channel if its different from the last channel */
> >>> - if (mux->last_chan != regval) {
> >>> - err = mlxcpld_mux_reg_write(muxc->parent, mux, regval);
> >>> - mux->last_chan = err < 0 ? 0 : regval;
> >>> + chan++;
> >>
> >> I question the removal of the regval variable. See above.
> >
> > I will return back 'regval' and make assignment base on register size.
> >
> >>
> >>> + if (mux->last_chan != chan) {
> >>> + err = mlxcpld_mux_reg_write(muxc->parent, mux, chan);
> >>> + mux->last_chan = err < 0 ? 0 : chan;
> >>> }
> >>>
> >>> return err;
> >>> @@ -103,13 +121,26 @@ static int mlxcpld_mux_probe(struct
> >> platform_device *pdev)
> >>> struct i2c_mux_core *muxc;
> >>> int num, force;
> >>> struct mlxcpld_mux *data;
> >>> + u16 sel_reg_addr = 0;
> >>> + u32 func;
> >>> int err;
> >>>
> >>> if (!pdata)
> >>> return -EINVAL;
> >>>
> >>> - if (!i2c_check_functionality(client->adapter,
> >>> - I2C_FUNC_SMBUS_WRITE_BYTE_DATA))
> >>> + switch (pdata->reg_size) {
> >>> + case 1:
> >>> + func = I2C_FUNC_SMBUS_WRITE_BYTE_DATA;
> >>> + break;
> >>> + case 2:
> >>> + func = I2C_FUNC_SMBUS_WRITE_WORD_DATA;
> >>
> >> ...and here you setup to check for SMBUS for the 16-bit case. And the
> >> type of SMBUS xfer is not compatible with the xfer that is actually taking
> place.
> >> WRITE_WORD_DATA is 8-bit register and 16-bit data. You have the
> opposite.
> >> So, this check is broken.
> >
> > Yes. I have to check for I2C functionality, so it should I2C_FUNC_I2C, yes?
>
> Yes.
>
> Cheers,
> Peter
>
> >>
> >>> + sel_reg_addr = cpu_to_be16(pdata->sel_reg_addr);
> >>> + break;
> >>> + default:
> >>> + return -EINVAL;
> >>> + }
> >>> +
> >>> + if (!i2c_check_functionality(client->adapter, func))
> >>> return -ENODEV;
> >>>
> >>> muxc = i2c_mux_alloc(client->adapter, &pdev->dev,
> >>> CPLD_MUX_MAX_NCHANS, @@ -122,6 +153,8 @@ static int
> >> mlxcpld_mux_probe(struct platform_device *pdev)
> >>> data = i2c_mux_priv(muxc);
> >>> data->client = client;
> >>> memcpy(&data->pdata, pdata, sizeof(*pdata));
> >>> + /* Save mux select address for 16 bits transaction size. */
> >>> + memcpy(data->sel_buf, &sel_reg_addr, 2);
> >>> data->last_chan = 0; /* force the first selection */
> >>>
> >>> /* Create an adapter for each channel. */ diff --git
> >>> a/include/linux/platform_data/mlxcpld.h
> >>> b/include/linux/platform_data/mlxcpld.h
> >>> index e6c18bf017dd..da4f7e8f5721 100644
> >>> --- a/include/linux/platform_data/mlxcpld.h
> >>> +++ b/include/linux/platform_data/mlxcpld.h
> >>> @@ -14,11 +14,13 @@
> >>> * @adap_ids - adapter array
> >>> * @num_adaps - number of adapters
> >>> * @sel_reg_addr - mux select register offset in CPLD space
> >>> + * @reg_size: register size in bytes (default 0 - 1 byte data, 1 -
> >>> + 2 bytes data
> >>
> >> The reg_size isn't in bytes according to the brackeded info. Missing
> >> end bracket as well...
> >
> > Will fix it.
> >
> > Thank you very much,
> > Vadim.
> >
> >>
> >> Cheers,
> >> Peter
> >>
> >>> */
> >>> struct mlxcpld_mux_plat_data {
> >>> int *adap_ids;
> >>> int num_adaps;
> >>> int sel_reg_addr;
> >>> + u8 reg_size;
> >>> };
> >>>
> >>> #endif /* _LINUX_I2C_MLXCPLD_H */
> >>>
