Re: [PATCH v2 2/7] i3c: Add core I3C infrastructure

[Boris Brezillon]

On Tue, 27 Feb 2018 20:24:43 +
Przemyslaw Sroka  wrote:


> > > SETDASA is simply faster than ENTDAA, but only if there is no
> > > need to collect BCR/DCR/PID of such devices. I think most
> > > applications would like to have them as an status information so
> > > after all ENTDAA can be regarded as an generic approach (unless
> > > I'm mistaken).  
> > 
> > Actually, we could retrieve BCR/DCR/PID (and all other relevant
> > information, like MAXDS) even with the SETDASA approach. We just
> > need to send the according CCC commands after SETDASA.
> >   
> I agree, what I meant by "SETDASA is simply faster than ENTDAA, but
> only if there is no need to collect BCR/DCR/PID of such devices." Is
> that it is faster than DAA but only if not followed by GET CCC
> commands to gather BCR/DCR/PID. I think we are on the same page here.

Yes, but right now it's not the case, see my other reply ;-).

> 
> > But that's also my understanding that ENTDAA should always work, and
> > SETDASA usage is only needed if you want to reserve a dynamic
> > address and assign it to a device before DAA takes place. This way
> > you can enforce the device priority (WRT IBIs). But honestly,
> > that's the only use case I can think of, and to me, it sounds like
> > an advanced feature we may want to support at some point, but don't
> > need in the initial implementation. 
> Still ENTDAA seems to be sufficient for IBI prioritization but I can
> imagine some use cases where people would like to use it for such
> purposes. Note that SETDASA is applicable only for devices with SA so
> it is self-explanatory that it cannot be considered as utility to
> define priorities for all devices before ENTDAA. 

We have SETNEWDA for other use cases: say you want one of your device to
have an higher priority, you can just manually set a new dynamic
address that is lower than any other devices on the bus (I plan to
expose that through sysfs, by making the address file writable).

-- 
Boris Brezillon, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 2/7] i3c: Add core I3C infrastructure

[Boris Brezillon]

Hi Przemek,

On Tue, 27 Feb 2018 17:06:37 +
Przemyslaw Sroka  wrote:

> > > >
> > > > Could you tell me why you think SETDASA is required?  
> > >
> > > Yes, you are right... But in my opinion it is required as it does part
> > > of DAA process.  
> > 
> > SETDASA is simply faster than ENTDAA, but only if there is no need to
> > collect BCR/DCR/PID of such devices. I think most applications would like to
> > have them as an status information so  after all ENTDAA can be regarded as
> > an generic approach (unless I'm mistaken).  
> 
> Below are 2 examples on how DAA can be executed:
> 1st:
> A1) SETDASA to devices with SA

I'm not even sure all devices with a static address needs to be
assigned a dynamic address with SETDASA (actually, I'm almost sure
it's not the case, since, according to section "5.1.9.3 CCC Command
Definitions" of the spec, all I3C slaves have to support ENTDAA). To me,
it looks like you'd want to do that only is you really need to reserve
a specific dynamic address and prevent the DAA step from assigning it
to another device.

> B1) DAA to remaining devices
> C1) GET BCR/DCR/PID to devices that initially had SA
> NOTES: C1 is optional and order of B1 and C1 can be changed

While that's true in principle, in Linux we'll always retrieve
BCR/DCR/PID (and more, like MAXDS), no matter how the device obtained
its dynamic address.

> 
> 2nd:
> A2) DAA to all devices
> NOTES: no need for any follow up steps as all information is collected during 
> DAA

As said above, that's not exactly how the Linux implementation works.
Right now I'm ignoring the information retrieved during DAA and forcing
a GETPID/GETBCR/GETDCR for every discovered device. This approach is
generating a bit more traffic on the bus, but it also makes the
implementation more generic, because we have a single function to add
an I3C device, no matter how it's been assigned a dynamic address.

> 
> As we can see 2nd approach is more generic and do not see any reason to add 
> special handling for SETDASA unless there is any reasonable reason to do 
> otherwise.

I agree on one thing: as long as you don't have to reserve a specific
dynamic address, SETDASA is not required. At least, that's my
understanding.

Regards,

Boris

-- 
Boris Brezillon, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH v2 2/7] i3c: Add core I3C infrastructure

[Przemyslaw Sroka]

Hi Boris

> -Original Message-
> From: Boris Brezillon [mailto:boris.brezil...@bootlin.com]
> Sent: Tuesday, February 27, 2018 9:13 PM
> To: Przemyslaw Sroka <psr...@cadence.com>
> Cc: Vitor Soares <vitor.soa...@synopsys.com>; Boris Brezillon
> <boris.brezil...@free-electrons.com>; Wolfram Sang <wsa@the-
> dreams.de>; linux-...@vger.kernel.org; Jonathan Corbet <cor...@lwn.net>;
> linux-doc@vger.kernel.org; Greg Kroah-Hartman
> <gre...@linuxfoundation.org>; Arnd Bergmann <a...@arndb.de>;
> Arkadiusz Golec <ago...@cadence.com>; Alan Douglas
> <adoug...@cadence.com>; Bartosz Folta <bfo...@cadence.com>; Damian
> Kos <d...@cadence.com>; Alicja Jurasik-Urbaniak <ali...@cadence.com>;
> Cyprian Wronka <cwro...@cadence.com>; Suresh Punnoose
> <sure...@cadence.com>; Thomas Petazzoni <thomas.petazzoni@free-
> electrons.com>; Nishanth Menon <n...@ti.com>; Rob Herring
> <robh...@kernel.org>; Pawel Moll <pawel.m...@arm.com>; Mark Rutland
> <mark.rutl...@arm.com>; Ian Campbell <ijc+devicet...@hellion.org.uk>;
> Kumar Gala <ga...@codeaurora.org>; devicet...@vger.kernel.org; linux-
> ker...@vger.kernel.org; Geert Uytterhoeven <ge...@linux-m68k.org>; Linus
> Walleij <linus.wall...@linaro.org>
> Subject: Re: [PATCH v2 2/7] i3c: Add core I3C infrastructure
> 
> EXTERNAL MAIL
> 
> 
> Hi Przemek,
> 
> On Tue, 27 Feb 2018 16:43:27 +
> Przemyslaw Sroka <psr...@cadence.com> wrote:
> 
> > >
> > > >> Either important is the SETDASA for declared I3C devices. So the
> > > >> DAA process should start by send an SETDASA and them ENTDAA
> CCC
> > > command.
> > > > My understanding was that SETDASA was not mandatory, and was
> only
> > > > useful when one wants to assign a specific dynamic address to a
> > > > slave that has a static address (which is again not mandatory).
> > > > I've tested it, and even devices with a static address participate
> > > > to the DAA procedure if they've not been assigned a dynamic
> > > > address yet, so I don't see the need for this SETDASA step if you
> > > > don't need to assign a particular dynamic address to the device.
> > > >
> > > > Could you tell me why you think SETDASA is required?
> > >
> > > Yes, you are right... But in my opinion it is required as it does
> > > part of DAA process.
> >
> > SETDASA is simply faster than ENTDAA, but only if there is no need to
> > collect BCR/DCR/PID of such devices. I think most applications would
> > like to have them as an status information so  after all ENTDAA can be
> > regarded as an generic approach (unless I'm mistaken).
> 
> Actually, we could retrieve BCR/DCR/PID (and all other relevant
> information, like MAXDS) even with the SETDASA approach. We just need to
> send the according CCC commands after SETDASA.
> 
I agree, what I meant by "SETDASA is simply faster than ENTDAA, but only if 
there is no need to collect BCR/DCR/PID of such devices." Is that it is faster 
than DAA but only if not followed by GET CCC commands to gather BCR/DCR/PID. I 
think we are on the same page here.

> But that's also my understanding that ENTDAA should always work, and
> SETDASA usage is only needed if you want to reserve a dynamic address
> and assign it to a device before DAA takes place. This way you can enforce
> the device priority (WRT IBIs). But honestly, that's the only use case I can
> think of, and to me, it sounds like an advanced feature we may want to
> support at some point, but don't need in the initial implementation.
>
Still ENTDAA seems to be sufficient for IBI prioritization but I can imagine 
some use cases where people would like to use it for such purposes. Note that 
SETDASA is applicable only for devices with SA so it is self-explanatory that 
it cannot be considered as utility to define priorities for all devices before 
ENTDAA. 

> --
> Boris Brezillon, Bootlin (formerly Free Electrons) Embedded Linux and
> Kernel engineering https://urldefense.proofpoint.com/v2/url?u=https-
> 3A__bootlin.com=DwICAg=aUq983L2pue2FqKFoP6PGHMJQyoJ7kl3s3G
> Z-_haXqY=b0WPdqYyu0KH4-vSatt-ViJE1riZ603zdXl3hHHp_TU=wM54-
> BGcSfHEklVRsw02O-bnyNkLTe9c0RyBP_ExzPA=pxQrogG-
> Nq4XOMU7SPZ2FZNbgnbnjdERtMm_h7ZcdCE=

Regards,
Przemek
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 2/7] i3c: Add core I3C infrastructure

[Boris Brezillon]

Hi Przemek,

On Tue, 27 Feb 2018 16:43:27 +
Przemyslaw Sroka  wrote:

> >   
> > >> Either important is the SETDASA for declared I3C devices. So the DAA
> > >> process should start by send an SETDASA and them ENTDAA CCC  
> > command.  
> > > My understanding was that SETDASA was not mandatory, and was only
> > > useful when one wants to assign a specific dynamic address to a slave
> > > that has a static address (which is again not mandatory).
> > > I've tested it, and even devices with a static address participate to
> > > the DAA procedure if they've not been assigned a dynamic address yet,
> > > so I don't see the need for this SETDASA step if you don't need to
> > > assign a particular dynamic address to the device.
> > >
> > > Could you tell me why you think SETDASA is required?  
> > 
> > Yes, you are right... But in my opinion it is required as it does part of 
> > DAA
> > process.  
> 
> SETDASA is simply faster than ENTDAA, but only if there is no need to
> collect BCR/DCR/PID of such devices. I think most applications would
> like to have them as an status information so  after all ENTDAA can 
> be regarded as an generic approach (unless I'm mistaken).

Actually, we could retrieve BCR/DCR/PID (and all other relevant
information, like MAXDS) even with the SETDASA approach. We just
need to send the according CCC commands after SETDASA.

But that's also my understanding that ENTDAA should always work, and
SETDASA usage is only needed if you want to reserve a dynamic address
and assign it to a device before DAA takes place. This way you can
enforce the device priority (WRT IBIs). But honestly, that's the only
use case I can think of, and to me, it sounds like an advanced feature
we may want to support at some point, but don't need in the initial
implementation.

-- 
Boris Brezillon, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 2/7] i3c: Add core I3C infrastructure

[Boris Brezillon]

Hi Vitor,

On Tue, 27 Feb 2018 16:03:58 +
Vitor Soares  wrote:

> >>>
> >>> The speed R/W speed limitation is encoded in the device object, so, if
> >>> the controller has to configure that on a per-transfer basis, one
> >>> solution would be to pass the device to the ->priv_xfers().
> >> The speed R/W limitation is only for private transfers. Also the device 
> >> can have
> >> a limitation to write and not for read data.
> >> This information is obtained with the command GETMXDS which returns the 
> >> Maximum
> >> Sustained Data Rate for non-CCC messages.  
> > And that's exactly what I expose in i3c_device_info, which is embedded
> > in i3c_device, so you should have all the information you need to
> > determine the speed in the controller driver if ->priv_xfer() is passed
> > the device attached to those transfers. Would that be okay if we pass an
> > i3c_device object to ->priv_xfers()?  
> 
> If you pass the i3c_device to ->priv_xfer(), then you won't need the address 
> too.

That's true. So how about we pass the i3c_device to ->priv_xfer() and
drop the address field in i3c_priv_xfer. Or we could remove the address
field add an i3c_device pointer in i3c_priv_xfer, this way, if we ever
need to do cross-device sequence we'll be able to support it.

> 
> Maybe someone else can give other point of view.

That'd be great, but I'd like to hear your opinion, because it's not
clear to me what you think of my suggestion.


> >> Either important is the SETDASA for declared I3C devices. So the DAA 
> >> process
> >> should start by send an SETDASA and them ENTDAA CCC command.  
> > My understanding was that SETDASA was not mandatory, and was only useful
> > when one wants to assign a specific dynamic address to a slave that has
> > a static address (which is again not mandatory).
> > I've tested it, and even devices with a static address participate to
> > the DAA procedure if they've not been assigned a dynamic address yet,
> > so I don't see the need for this SETDASA step if you don't need to
> > assign a particular dynamic address to the device.
> >
> > Could you tell me why you think SETDASA is required?  
> 
> Yes, you are right... But in my opinion it is required as it does part of DAA
> process.

As Przemek said in his reply, I don't think it's required, at least not
for the initial implementation. I'm definitely not saying we should
never support the feature, but it can easily be added afterwards.

BTW, can you give me a scenario where you'll want to assign a specific
dynamic address to a device before DAA takes place (by DAA, I mean what
happens after ENTDAA, which AIUI is what DAA describes)? I know it's
described in the spec, and might become useful at some point, but right
now, for a general purpose OS like Linux, I don't see a good reason to
assign a dynamic address using SETDASA.

> 
> > +static u32 i3c_master_i2c_functionalities(struct i2c_adapter *adap)
> > +{
> > +   return I2C_FUNC_SMBUS_EMUL | I2C_FUNC_I2C | I2C_FUNC_10BIT_ADDR;
> > +}  
>   Is I2C_FUNC_10BIT_ADDR allowed ?
> >>> According to "Table 4 I 2 C Features Allowed in I3C Slaves", yes (at
> >>> least that my understanding). And the Cadence controller supports it.
> >> The table say the oposite. The I2C extended address feature is not used on 
> >> I3C
> >> bus, thus this feature shall be disable.  
> > Actually, I was wrong when initially mentioning this table: it's about
> > I2C features supported on I3C slaves, so not really what we're looking
> > for. Here, we're wondering if I2C-only devices can have 10-bit
> > addresses. The Cadence controller supports that, so there's probably
> > nothing preventing use of 10-bit addresses for I2C transfers, but maybe
> > not all I3C master controllers support that, so we should probably
> > let the I3C master driver implement this method.  
> 
> The spec says that is "not used" so it will not interface with I3C bus.

Again, I should not have pointed you to this chapter, because it does
not describe what the I3C bus accept, but what I3C slave acting like
I2C devices accept (basically what they accept before being assigned a
dynamic address).

If you see in the I3C spec that I2C transfers using 10-bit addresses
is forbidden, could you tell me where it's stated, because I didn't
find it. 

> 
> >> BTW it is optional on I2C devices.  
> > You mean I2C controllers? When an I2C device has a 10bit address, the
> > controller has to support this mode to communicate with the device, at
> > least that's my understanding. But we're digressing a bit. The
> > question is not whether I2C devices can optionally use a 10 bit
> > address, but whether I3C master controller can support this mode for
> > I2C transfers to I2C-only devices.  
> 
> By the i2c spec the 10-bit address is optional, however the 7-bit address is
> mandatory.

The controller is not forced to support this feature, I think I already
said I agree on this aspect. But if 

RE: [PATCH v2 2/7] i3c: Add core I3C infrastructure

[Przemyslaw Sroka]

More detailed explanation...

> -Original Message-
> From: Przemyslaw Sroka
> Sent: Tuesday, February 27, 2018 5:43 PM
> To: 'Vitor Soares' <vitor.soa...@synopsys.com>; Boris Brezillon
> <boris.brezil...@bootlin.com>
> Cc: Boris Brezillon <boris.brezil...@free-electrons.com>; Wolfram Sang
> <w...@the-dreams.de>; linux-...@vger.kernel.org; Jonathan Corbet
> <cor...@lwn.net>; linux-doc@vger.kernel.org; Greg Kroah-Hartman
> <gre...@linuxfoundation.org>; Arnd Bergmann <a...@arndb.de>;
> Arkadiusz Golec <ago...@cadence.com>; Alan Douglas
> <adoug...@cadence.com>; Bartosz Folta <bfo...@cadence.com>; Damian
> Kos <d...@cadence.com>; Alicja Jurasik-Urbaniak <ali...@cadence.com>;
> Cyprian Wronka <cwro...@cadence.com>; Suresh Punnoose
> <sure...@cadence.com>; Thomas Petazzoni <thomas.petazzoni@free-
> electrons.com>; Nishanth Menon <n...@ti.com>; Rob Herring
> <robh...@kernel.org>; Pawel Moll <pawel.m...@arm.com>; Mark Rutland
> <mark.rutl...@arm.com>; Ian Campbell <ijc+devicet...@hellion.org.uk>;
> Kumar Gala <ga...@codeaurora.org>; devicet...@vger.kernel.org; linux-
> ker...@vger.kernel.org; Geert Uytterhoeven <ge...@linux-m68k.org>; Linus
> Walleij <linus.wall...@linaro.org>
> Subject: RE: [PATCH v2 2/7] i3c: Add core I3C infrastructure
> 
> Hi Boris and Vitor
> 
> Find below my comment on DAA procedure.
> 
> > -Original Message-
> > From: Vitor Soares [mailto:vitor.soa...@synopsys.com]
> > Sent: Tuesday, February 27, 2018 5:04 PM
> > To: Boris Brezillon <boris.brezil...@bootlin.com>; Vitor Soares
> > <vitor.soa...@synopsys.com>
> > Cc: Boris Brezillon <boris.brezil...@free-electrons.com>; Wolfram Sang
> > <w...@the-dreams.de>; linux-...@vger.kernel.org; Jonathan Corbet
> > <cor...@lwn.net>; linux-doc@vger.kernel.org; Greg Kroah-Hartman
> > <gre...@linuxfoundation.org>; Arnd Bergmann <a...@arndb.de>;
> > Przemyslaw Sroka <psr...@cadence.com>; Arkadiusz Golec
> > <ago...@cadence.com>; Alan Douglas <adoug...@cadence.com>; Bartosz
> > Folta <bfo...@cadence.com>; Damian Kos <d...@cadence.com>; Alicja
> > Jurasik-Urbaniak <ali...@cadence.com>; Cyprian Wronka
> > <cwro...@cadence.com>; Suresh Punnoose <sure...@cadence.com>;
> Thomas
> > Petazzoni <thomas.petazz...@free-electrons.com>; Nishanth Menon
> > <n...@ti.com>; Rob Herring <robh...@kernel.org>; Pawel Moll
> > <pawel.m...@arm.com>; Mark Rutland <mark.rutl...@arm.com>; Ian
> > Campbell <ijc+devicet...@hellion.org.uk>; Kumar Gala
> > <ga...@codeaurora.org>; devicet...@vger.kernel.org; linux-
> > ker...@vger.kernel.org; Geert Uytterhoeven <ge...@linux-m68k.org>;
> > Linus Walleij <linus.wall...@linaro.org>
> > Subject: Re: [PATCH v2 2/7] i3c: Add core I3C infrastructure
> >
> > EXTERNAL MAIL
> >
> >
> > Hi Boris
> >
> >
> > Às 8:36 PM de 2/26/2018, Boris Brezillon escreveu:
> > > Hi Vitor,
> > >
> > > On Mon, 26 Feb 2018 18:58:15 +
> > > Vitor Soares <vitor.soa...@synopsys.com> wrote:
> > >
> > >>>>> +/**
> > >>>>> + * i3c_device_do_priv_xfers() - do I3C SDR private transfers
> > >>>>> +directed
> > to a
> > >>>>> + *   specific device
> > >>>>> + *
> > >>>>> + * @dev: device with which the transfers should be done
> > >>>>> + * @xfers: array of transfers
> > >>>>> + * @nxfers: number of transfers
> > >>>>> + *
> > >>>>> + * Initiate one or several private SDR transfers with @dev.
> > >>>>> + *
> > >>>>> + * This function can sleep and thus cannot be called in atomic
> > context.
> > >>>>> + *
> > >>>>> + * Return: 0 in case of success, a negative error core otherwise.
> > >>>>> + */
> > >>>>> +int i3c_device_do_priv_xfers(struct i3c_device *dev,
> > >>>>> +  struct i3c_priv_xfer *xfers,
> > >>>>> +  int nxfers)
> > >>>>> +{
> > >>>>> + struct i3c_master_controller *master;
> > >>>>> + int i, ret;
> > >>>>> +
> > >>>>> + master = i3c_device_get_master(dev);
> > >>>>> +

RE: [PATCH v2 2/7] i3c: Add core I3C infrastructure

[Przemyslaw Sroka]

Hi Boris and Vitor

Find below my comment on DAA procedure.

> -Original Message-
> From: Vitor Soares [mailto:vitor.soa...@synopsys.com]
> Sent: Tuesday, February 27, 2018 5:04 PM
> To: Boris Brezillon <boris.brezil...@bootlin.com>; Vitor Soares
> <vitor.soa...@synopsys.com>
> Cc: Boris Brezillon <boris.brezil...@free-electrons.com>; Wolfram Sang
> <w...@the-dreams.de>; linux-...@vger.kernel.org; Jonathan Corbet
> <cor...@lwn.net>; linux-doc@vger.kernel.org; Greg Kroah-Hartman
> <gre...@linuxfoundation.org>; Arnd Bergmann <a...@arndb.de>;
> Przemyslaw Sroka <psr...@cadence.com>; Arkadiusz Golec
> <ago...@cadence.com>; Alan Douglas <adoug...@cadence.com>; Bartosz
> Folta <bfo...@cadence.com>; Damian Kos <d...@cadence.com>; Alicja
> Jurasik-Urbaniak <ali...@cadence.com>; Cyprian Wronka
> <cwro...@cadence.com>; Suresh Punnoose <sure...@cadence.com>;
> Thomas Petazzoni <thomas.petazz...@free-electrons.com>; Nishanth
> Menon <n...@ti.com>; Rob Herring <robh...@kernel.org>; Pawel Moll
> <pawel.m...@arm.com>; Mark Rutland <mark.rutl...@arm.com>; Ian
> Campbell <ijc+devicet...@hellion.org.uk>; Kumar Gala
> <ga...@codeaurora.org>; devicet...@vger.kernel.org; linux-
> ker...@vger.kernel.org; Geert Uytterhoeven <ge...@linux-m68k.org>; Linus
> Walleij <linus.wall...@linaro.org>
> Subject: Re: [PATCH v2 2/7] i3c: Add core I3C infrastructure
> 
> EXTERNAL MAIL
> 
> 
> Hi Boris
> 
> 
> Às 8:36 PM de 2/26/2018, Boris Brezillon escreveu:
> > Hi Vitor,
> >
> > On Mon, 26 Feb 2018 18:58:15 +
> > Vitor Soares <vitor.soa...@synopsys.com> wrote:
> >
> >>>>> +/**
> >>>>> + * i3c_device_do_priv_xfers() - do I3C SDR private transfers directed
> to a
> >>>>> + * specific device
> >>>>> + *
> >>>>> + * @dev: device with which the transfers should be done
> >>>>> + * @xfers: array of transfers
> >>>>> + * @nxfers: number of transfers
> >>>>> + *
> >>>>> + * Initiate one or several private SDR transfers with @dev.
> >>>>> + *
> >>>>> + * This function can sleep and thus cannot be called in atomic
> context.
> >>>>> + *
> >>>>> + * Return: 0 in case of success, a negative error core otherwise.
> >>>>> + */
> >>>>> +int i3c_device_do_priv_xfers(struct i3c_device *dev,
> >>>>> +struct i3c_priv_xfer *xfers,
> >>>>> +int nxfers)
> >>>>> +{
> >>>>> +   struct i3c_master_controller *master;
> >>>>> +   int i, ret;
> >>>>> +
> >>>>> +   master = i3c_device_get_master(dev);
> >>>>> +   if (!master)
> >>>>> +   return -EINVAL;
> >>>>> +
> >>>>> +   i3c_bus_normaluse_lock(master->bus);
> >>>>> +   for (i = 0; i < nxfers; i++)
> >>>>> +   xfers[i].addr = dev->info.dyn_addr;
> >>>>> +
> >>>>> +   ret = i3c_master_do_priv_xfers_locked(master, xfers,
> nxfers);
> >>>>> +   i3c_bus_normaluse_unlock(master->bus);
> >>>>> +
> >>>>> +   return ret;
> >>>>> +}
> >>>>> +EXPORT_SYMBOL_GPL(i3c_device_do_priv_xfers);
> >>>>  The controller should know the speed mode for each xfer. The SDR0
> >>>> mode is used by default but if any device have read or write speed
> >>>> limitations the controller can use SDRx.
> >>> I might be wrong, but that's not my understanding of the spec. A
> >>> device can express a speed limitation for SDR priv transfers, but
> >>> this limitation applies to all SDR transfers.
> >>>
> >>> The speed R/W speed limitation is encoded in the device object, so,
> >>> if the controller has to configure that on a per-transfer basis, one
> >>> solution would be to pass the device to the ->priv_xfers().
> >> The speed R/W limitation is only for private transfers. Also the
> >> device can have a limitation to write and not for read data.
> >> This information is obtained with the command GETMXDS which returns
> >> the Maximum Sustained Data Rate for non-CCC messages.
> > And that's exactly what I expose in i3c_device

Re: [PATCH v2 2/7] i3c: Add core I3C infrastructure

[Vitor Soares]

Hi Boris


Às 8:36 PM de 2/26/2018, Boris Brezillon escreveu:
> Hi Vitor,
>
> On Mon, 26 Feb 2018 18:58:15 +
> Vitor Soares  wrote:
>
> +/**
> + * i3c_device_do_priv_xfers() - do I3C SDR private transfers directed to 
> a
> + *   specific device
> + *
> + * @dev: device with which the transfers should be done
> + * @xfers: array of transfers
> + * @nxfers: number of transfers
> + *
> + * Initiate one or several private SDR transfers with @dev.
> + *
> + * This function can sleep and thus cannot be called in atomic context.
> + *
> + * Return: 0 in case of success, a negative error core otherwise.
> + */
> +int i3c_device_do_priv_xfers(struct i3c_device *dev,
> +  struct i3c_priv_xfer *xfers,
> +  int nxfers)
> +{
> + struct i3c_master_controller *master;
> + int i, ret;
> +
> + master = i3c_device_get_master(dev);
> + if (!master)
> + return -EINVAL;
> +
> + i3c_bus_normaluse_lock(master->bus);
> + for (i = 0; i < nxfers; i++)
> + xfers[i].addr = dev->info.dyn_addr;
> +
> + ret = i3c_master_do_priv_xfers_locked(master, xfers, nxfers);
> + i3c_bus_normaluse_unlock(master->bus);
> +
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(i3c_device_do_priv_xfers);
  The controller should know the speed mode for each xfer. The SDR0 mode 
 is used by default but if any device have read or write speed 
 limitations the controller can use SDRx.  
>>> I might be wrong, but that's not my understanding of the spec. A device
>>> can express a speed limitation for SDR priv transfers, but this
>>> limitation applies to all SDR transfers.
>>>
>>> The speed R/W speed limitation is encoded in the device object, so, if
>>> the controller has to configure that on a per-transfer basis, one
>>> solution would be to pass the device to the ->priv_xfers().  
>> The speed R/W limitation is only for private transfers. Also the device can 
>> have
>> a limitation to write and not for read data.
>> This information is obtained with the command GETMXDS which returns the 
>> Maximum
>> Sustained Data Rate for non-CCC messages.
> And that's exactly what I expose in i3c_device_info, which is embedded
> in i3c_device, so you should have all the information you need to
> determine the speed in the controller driver if ->priv_xfer() is passed
> the device attached to those transfers. Would that be okay if we pass an
> i3c_device object to ->priv_xfers()?

If you pass the i3c_device to ->priv_xfer(), then you won't need the address 
too.

Maybe someone else can give other point of view.

>>>  
 This could be also applied to i2c transfers.  
>>> Not really. The max SCL frequency is something that applies to the
>>> whole bus, because all I2C devices have to decode the address when
>>> messages are sent on the bus to determine if they should ignore or
>>> process the message.
>>>  
> +#endif /* I3C_INTERNAL_H */
> diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
> new file mode 100644
> index ..1c85abac08d5
> --- /dev/null
> +++ b/drivers/i3c/master.c
> @@ -0,0 +1,1433 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2017 Cadence Design Systems Inc.
> + *
> + * Author: Boris Brezillon
> + */
> +
> +#include 
> +
> +#include "internals.h"
> +
> +/**
> + * i3c_master_entdaa_locked() - start a DAA (Dynamic Address Assignment)
> + *   procedure
> + * @master: master used to send frames on the bus
> + *
> + * Send a ENTDAA CCC command to start a DAA procedure.
> + *
> + * Note that this function only sends the ENTDAA CCC command, all the 
> logic
> + * behind dynamic address assignment has to be handled in the I3C master
> + * driver.
> + *
> + * This function must be called with the bus lock held in write mode.
> + *
> + * Return: 0 in case of success, a negative error code otherwise.
> + */
> +int i3c_master_entdaa_locked(struct i3c_master_controller *master)
> +{
> + struct i3c_ccc_cmd_dest dest = { };
> + struct i3c_ccc_cmd cmd = { };
> + int ret;
> +
> + dest.addr = I3C_BROADCAST_ADDR;
> + cmd.dests = 
> + cmd.ndests = 1;
> + cmd.rnw = false;
> + cmd.id = I3C_CCC_ENTDAA;
> +
> + ret = i3c_master_send_ccc_cmd_locked(master, );
> + if (ret)
> + return ret;
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(i3c_master_entdaa_locked);
  can you explain the process?  
>>> Not sure what you mean. The ENTDAA is just a CCC command that is used
>>> to trigger a DAA procedure. What the master controller does when it
>>> sends such a command is likely to be controller 

Re: [PATCH v2 2/7] i3c: Add core I3C infrastructure

[Boris Brezillon]

On Mon, 26 Feb 2018 21:40:32 +0100
Boris Brezillon  wrote:

> On Mon, 26 Feb 2018 21:36:07 +0100
> Boris Brezillon  wrote:
> 
> > > >>> +
> > > >>> +/**
> > > >>> + * struct i3c_master_controller_ops - I3C master methods
> > > >>> + * @bus_init: hook responsible for the I3C bus initialization. This
> > > >>> + * initialization should follow the steps described in the 
> > > >>> I3C
> > > >>> + * specification. This hook is called with the bus lock held 
> > > >>> in
> > > >>> + * write mode, which means all _locked() helpers can safely 
> > > >>> be
> > > >>> + * called from there
> > > >>> + * @bus_cleanup: cleanup everything done in
> > > >>> + *_master_controller_ops->bus_init(). This function 
> > > >>> is
> > > >>> + *optional and should only be implemented if
> > > >>> + *_master_controller_ops->bus_init() attached 
> > > >>> private data
> > > >>> + *to I3C/I2C devices. This hook is called with the bus 
> > > >>> lock
> > > >>> + *held in write mode, which means all _locked() helpers 
> > > >>> can
> > > >>> + *safely be called from there
> > > >>> + * @supports_ccc_cmd: should return true if the CCC command is 
> > > >>> supported, false
> > > >>> + * otherwise
> > > >>> + * @send_ccc_cmd: send a CCC command
> > > >>> + * @send_hdr_cmds: send one or several HDR commands. If there is 
> > > >>> more than one
> > > >>> + *  command, they should ideally be sent in the same HDR
> > > >>> + *  transaction
> > > >>> + * @priv_xfers: do one or several private I3C SDR transfers
> > > >>> + * @i2c_xfers: do one or several I2C transfers
> > > >>> + * @request_ibi: attach an IBI handler to an I3C device. This 
> > > >>> implies defining
> > > >>> + *an IBI handler and the constraints of the IBI (maximum 
> > > >>> payload
> > > >>> + *length and number of pre-allocated slots).
> > > >>> + *Some controllers support less IBI-capable devices than 
> > > >>> regular
> > > >>> + *devices, so this method might return -%EBUSY if 
> > > >>> there's no
> > > >>> + *more space for an extra IBI registration
> > > >>> + * @free_ibi: free an IBI previously requested with ->request_ibi(). 
> > > >>> The IBI
> > > >>> + * should have been disabled with ->disable_irq() prior to 
> > > >>> that
> > > >>> + * @enable_ibi: enable the IBI. Only valid if ->request_ibi() has 
> > > >>> been called
> > > >>> + *   prior to ->enable_ibi(). The controller should first 
> > > >>> enable
> > > >>> + *   the IBI on the controller end (for example, unmask the 
> > > >>> hardware
> > > >>> + *   IRQ) and then send the ENEC CCC command (with the IBI 
> > > >>> flag set)
> > > >>> + *   to the I3C device
> > > >>> + * @disable_ibi: disable an IBI. First send the DISEC CCC command 
> > > >>> with the IBI
> > > >>> + *flag set and then deactivate the hardware IRQ on the
> > > >>> + *controller end
> > > >>> + * @recycle_ibi_slot: recycle an IBI slot. Called every time an IBI 
> > > >>> has been
> > > >>> + * processed by its handler. The IBI slot should be 
> > > >>> put back
> > > >>> + * in the IBI slot pool so that the controller can 
> > > >>> re-use it
> > > >>> + * for a future IBI
> > > >>> + *
> > > >>> + * One of the most important hooks in these ops is
> > > >>> + * _master_controller_ops->bus_init(). Here is a non-exhaustive 
> > > >>> list of
> > > >>> + * things that should be done in 
> > > >>> _master_controller_ops->bus_init():
> > > >>> + *
> > > >>> + * 1) call i3c_master_set_info() with all information describing the 
> > > >>> master
> > > >>> + * 2) ask all slaves to drop their dynamic address by sending the 
> > > >>> RSTDAA CCC
> > > >>> + *with i3c_master_rstdaa_locked()
> > > >>> + * 3) ask all slaves to disable IBIs using i3c_master_disec_locked()
> > > >>> + * 4) start a DDA procedure by sending the ENTDAA CCC with
> > > >>> + *i3c_master_entdaa_locked(), or using the internal DAA logic 
> > > >>> provided by
> > > >>> + *your controller
> > > >> You mean SETDASA CCC command?  
> > > > No, I really mean ENTDAA and DAA. By internal DAA logic I mean that
> > > > some controllers are probably automating the whole DAA procedure, while
> > > > others may let the SW control every step.  
> > > My understanding is that i3c_master_entdaa_locked() will trigger the DAA 
> > > process
> > > and DAA can be done by SETDASA, ENTDAA and later after the bus 
> > > initialization
> > > with SETNEWDA.
> > 
> > No. Only ENTDAA can trigger a DAA procedure. SETDASA is here to assign
> > a single dynamic address to a device that already has a static address
> > but no dynamic address yet, and SETNEWDA is here to modify the dynamic
> > 

Re: [PATCH v2 2/7] i3c: Add core I3C infrastructure

[Boris Brezillon]

On Mon, 26 Feb 2018 21:36:07 +0100
Boris Brezillon  wrote:

> > >>> +
> > >>> +/**
> > >>> + * struct i3c_master_controller_ops - I3C master methods
> > >>> + * @bus_init: hook responsible for the I3C bus initialization. This
> > >>> + *   initialization should follow the steps described in the 
> > >>> I3C
> > >>> + *   specification. This hook is called with the bus lock held 
> > >>> in
> > >>> + *   write mode, which means all _locked() helpers can safely 
> > >>> be
> > >>> + *   called from there
> > >>> + * @bus_cleanup: cleanup everything done in
> > >>> + *  _master_controller_ops->bus_init(). This function 
> > >>> is
> > >>> + *  optional and should only be implemented if
> > >>> + *  _master_controller_ops->bus_init() attached 
> > >>> private data
> > >>> + *  to I3C/I2C devices. This hook is called with the bus 
> > >>> lock
> > >>> + *  held in write mode, which means all _locked() helpers 
> > >>> can
> > >>> + *  safely be called from there
> > >>> + * @supports_ccc_cmd: should return true if the CCC command is 
> > >>> supported, false
> > >>> + *   otherwise
> > >>> + * @send_ccc_cmd: send a CCC command
> > >>> + * @send_hdr_cmds: send one or several HDR commands. If there is more 
> > >>> than one
> > >>> + *command, they should ideally be sent in the same HDR
> > >>> + *transaction
> > >>> + * @priv_xfers: do one or several private I3C SDR transfers
> > >>> + * @i2c_xfers: do one or several I2C transfers
> > >>> + * @request_ibi: attach an IBI handler to an I3C device. This implies 
> > >>> defining
> > >>> + *  an IBI handler and the constraints of the IBI (maximum 
> > >>> payload
> > >>> + *  length and number of pre-allocated slots).
> > >>> + *  Some controllers support less IBI-capable devices than 
> > >>> regular
> > >>> + *  devices, so this method might return -%EBUSY if 
> > >>> there's no
> > >>> + *  more space for an extra IBI registration
> > >>> + * @free_ibi: free an IBI previously requested with ->request_ibi(). 
> > >>> The IBI
> > >>> + *   should have been disabled with ->disable_irq() prior to 
> > >>> that
> > >>> + * @enable_ibi: enable the IBI. Only valid if ->request_ibi() has been 
> > >>> called
> > >>> + * prior to ->enable_ibi(). The controller should first 
> > >>> enable
> > >>> + * the IBI on the controller end (for example, unmask the 
> > >>> hardware
> > >>> + * IRQ) and then send the ENEC CCC command (with the IBI 
> > >>> flag set)
> > >>> + * to the I3C device
> > >>> + * @disable_ibi: disable an IBI. First send the DISEC CCC command with 
> > >>> the IBI
> > >>> + *  flag set and then deactivate the hardware IRQ on the
> > >>> + *  controller end
> > >>> + * @recycle_ibi_slot: recycle an IBI slot. Called every time an IBI 
> > >>> has been
> > >>> + *   processed by its handler. The IBI slot should be 
> > >>> put back
> > >>> + *   in the IBI slot pool so that the controller can 
> > >>> re-use it
> > >>> + *   for a future IBI
> > >>> + *
> > >>> + * One of the most important hooks in these ops is
> > >>> + * _master_controller_ops->bus_init(). Here is a non-exhaustive 
> > >>> list of
> > >>> + * things that should be done in 
> > >>> _master_controller_ops->bus_init():
> > >>> + *
> > >>> + * 1) call i3c_master_set_info() with all information describing the 
> > >>> master
> > >>> + * 2) ask all slaves to drop their dynamic address by sending the 
> > >>> RSTDAA CCC
> > >>> + *with i3c_master_rstdaa_locked()
> > >>> + * 3) ask all slaves to disable IBIs using i3c_master_disec_locked()
> > >>> + * 4) start a DDA procedure by sending the ENTDAA CCC with
> > >>> + *i3c_master_entdaa_locked(), or using the internal DAA logic 
> > >>> provided by
> > >>> + *your controller  
> > >> You mean SETDASA CCC command?
> > > No, I really mean ENTDAA and DAA. By internal DAA logic I mean that
> > > some controllers are probably automating the whole DAA procedure, while
> > > others may let the SW control every step.
> > My understanding is that i3c_master_entdaa_locked() will trigger the DAA 
> > process
> > and DAA can be done by SETDASA, ENTDAA and later after the bus 
> > initialization
> > with SETNEWDA.  
> 
> No. Only ENTDAA can trigger a DAA procedure. SETDASA is here to assign
> a single dynamic address to a device that already has a static address
> but no dynamic address yet, and SETNEWDA is here to modify the dynamic
> address of a device that already has one.
> 
> > 
> > I think the DAA process should be more generic, right now is only made 
> > through
> > the ENTDAA command with (cmd.ndests = 1).
> > I mean, shouldn't this be made by the core? First doing 

Re: [PATCH v2 2/7] i3c: Add core I3C infrastructure

[Boris Brezillon]

Hi Vitor,

On Mon, 26 Feb 2018 18:58:15 +
Vitor Soares  wrote:

> >>> +/**
> >>> + * i3c_device_do_priv_xfers() - do I3C SDR private transfers directed to 
> >>> a
> >>> + *   specific device
> >>> + *
> >>> + * @dev: device with which the transfers should be done
> >>> + * @xfers: array of transfers
> >>> + * @nxfers: number of transfers
> >>> + *
> >>> + * Initiate one or several private SDR transfers with @dev.
> >>> + *
> >>> + * This function can sleep and thus cannot be called in atomic context.
> >>> + *
> >>> + * Return: 0 in case of success, a negative error core otherwise.
> >>> + */
> >>> +int i3c_device_do_priv_xfers(struct i3c_device *dev,
> >>> +  struct i3c_priv_xfer *xfers,
> >>> +  int nxfers)
> >>> +{
> >>> + struct i3c_master_controller *master;
> >>> + int i, ret;
> >>> +
> >>> + master = i3c_device_get_master(dev);
> >>> + if (!master)
> >>> + return -EINVAL;
> >>> +
> >>> + i3c_bus_normaluse_lock(master->bus);
> >>> + for (i = 0; i < nxfers; i++)
> >>> + xfers[i].addr = dev->info.dyn_addr;
> >>> +
> >>> + ret = i3c_master_do_priv_xfers_locked(master, xfers, nxfers);
> >>> + i3c_bus_normaluse_unlock(master->bus);
> >>> +
> >>> + return ret;
> >>> +}
> >>> +EXPORT_SYMBOL_GPL(i3c_device_do_priv_xfers);
> >>  The controller should know the speed mode for each xfer. The SDR0 mode 
> >> is used by default but if any device have read or write speed 
> >> limitations the controller can use SDRx.  
> > I might be wrong, but that's not my understanding of the spec. A device
> > can express a speed limitation for SDR priv transfers, but this
> > limitation applies to all SDR transfers.
> >
> > The speed R/W speed limitation is encoded in the device object, so, if
> > the controller has to configure that on a per-transfer basis, one
> > solution would be to pass the device to the ->priv_xfers().  
> The speed R/W limitation is only for private transfers. Also the device can 
> have
> a limitation to write and not for read data.
> This information is obtained with the command GETMXDS which returns the 
> Maximum
> Sustained Data Rate for non-CCC messages.

And that's exactly what I expose in i3c_device_info, which is embedded
in i3c_device, so you should have all the information you need to
determine the speed in the controller driver if ->priv_xfer() is passed
the device attached to those transfers. Would that be okay if we pass an
i3c_device object to ->priv_xfers()?

> >  
> >> This could be also applied to i2c transfers.  
> > Not really. The max SCL frequency is something that applies to the
> > whole bus, because all I2C devices have to decode the address when
> > messages are sent on the bus to determine if they should ignore or
> > process the message.
> >  
> >>> +#endif /* I3C_INTERNAL_H */
> >>> diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
> >>> new file mode 100644
> >>> index ..1c85abac08d5
> >>> --- /dev/null
> >>> +++ b/drivers/i3c/master.c
> >>> @@ -0,0 +1,1433 @@
> >>> +// SPDX-License-Identifier: GPL-2.0
> >>> +/*
> >>> + * Copyright (C) 2017 Cadence Design Systems Inc.
> >>> + *
> >>> + * Author: Boris Brezillon
> >>> + */
> >>> +
> >>> +#include 
> >>> +
> >>> +#include "internals.h"
> >>> +
> >>> +/**
> >>> + * i3c_master_entdaa_locked() - start a DAA (Dynamic Address Assignment)
> >>> + *   procedure
> >>> + * @master: master used to send frames on the bus
> >>> + *
> >>> + * Send a ENTDAA CCC command to start a DAA procedure.
> >>> + *
> >>> + * Note that this function only sends the ENTDAA CCC command, all the 
> >>> logic
> >>> + * behind dynamic address assignment has to be handled in the I3C master
> >>> + * driver.
> >>> + *
> >>> + * This function must be called with the bus lock held in write mode.
> >>> + *
> >>> + * Return: 0 in case of success, a negative error code otherwise.
> >>> + */
> >>> +int i3c_master_entdaa_locked(struct i3c_master_controller *master)
> >>> +{
> >>> + struct i3c_ccc_cmd_dest dest = { };
> >>> + struct i3c_ccc_cmd cmd = { };
> >>> + int ret;
> >>> +
> >>> + dest.addr = I3C_BROADCAST_ADDR;
> >>> + cmd.dests = 
> >>> + cmd.ndests = 1;
> >>> + cmd.rnw = false;
> >>> + cmd.id = I3C_CCC_ENTDAA;
> >>> +
> >>> + ret = i3c_master_send_ccc_cmd_locked(master, );
> >>> + if (ret)
> >>> + return ret;
> >>> +
> >>> + return 0;
> >>> +}
> >>> +EXPORT_SYMBOL_GPL(i3c_master_entdaa_locked);
> >>  can you explain the process?  
> > Not sure what you mean. The ENTDAA is just a CCC command that is used
> > to trigger a DAA procedure. What the master controller does when it
> > sends such a command is likely to be controller dependent, and it might
> > even be possible that you don't need to call this function in your
> > controller driver to trigger a DAA. If you want more details about the
> > bus initialization steps and how the ENTDAA CCC 

Re: [PATCH v2 2/7] i3c: Add core I3C infrastructure

[Vitor Soares]

Hi Boris


Às 8:30 PM de 2/23/2018, Boris Brezillon escreveu:
> Hi Vitor,
>
> On Fri, 23 Feb 2018 16:56:14 +
> Vitor Soares  wrote:
>
>> Hi Boris,
>>
>> Às 3:16 PM de 12/14/2017, Boris Brezillon escreveu:
>>> +
>>> +enum i3c_addr_slot_status i3c_bus_get_addr_slot_status(struct i3c_bus *bus,
>>> +  u16 addr)
>>> +{
>>> +   int status, bitpos = addr * 2;
>>> +
>>> +   if (addr > I2C_MAX_ADDR)
>>> +   return I3C_ADDR_SLOT_RSVD;
>>> +
>>> +   status = bus->addrslots[bitpos / BITS_PER_LONG];
>>> +   status >>= bitpos % BITS_PER_LONG;
>>> +
>>> +   return status & I3C_ADDR_SLOT_STATUS_MASK;
>>> +}  
>> I don't understand the size of addr. The I3C only allow 7-bit addresses.
>>
>> Is the addrslots used to store the addresses and its status?
> No, slots are used for both I2C and I3C addresses, and an I2C address
> can be 10-bits wide, hence the u16 type.
>
>
> [...]
>
>>> +/**
>>> + * i3c_device_do_priv_xfers() - do I3C SDR private transfers directed to a
>>> + * specific device
>>> + *
>>> + * @dev: device with which the transfers should be done
>>> + * @xfers: array of transfers
>>> + * @nxfers: number of transfers
>>> + *
>>> + * Initiate one or several private SDR transfers with @dev.
>>> + *
>>> + * This function can sleep and thus cannot be called in atomic context.
>>> + *
>>> + * Return: 0 in case of success, a negative error core otherwise.
>>> + */
>>> +int i3c_device_do_priv_xfers(struct i3c_device *dev,
>>> +struct i3c_priv_xfer *xfers,
>>> +int nxfers)
>>> +{
>>> +   struct i3c_master_controller *master;
>>> +   int i, ret;
>>> +
>>> +   master = i3c_device_get_master(dev);
>>> +   if (!master)
>>> +   return -EINVAL;
>>> +
>>> +   i3c_bus_normaluse_lock(master->bus);
>>> +   for (i = 0; i < nxfers; i++)
>>> +   xfers[i].addr = dev->info.dyn_addr;
>>> +
>>> +   ret = i3c_master_do_priv_xfers_locked(master, xfers, nxfers);
>>> +   i3c_bus_normaluse_unlock(master->bus);
>>> +
>>> +   return ret;
>>> +}
>>> +EXPORT_SYMBOL_GPL(i3c_device_do_priv_xfers);  
>>  The controller should know the speed mode for each xfer. The SDR0 mode 
>> is used by default but if any device have read or write speed 
>> limitations the controller can use SDRx.
> I might be wrong, but that's not my understanding of the spec. A device
> can express a speed limitation for SDR priv transfers, but this
> limitation applies to all SDR transfers.
>
> The speed R/W speed limitation is encoded in the device object, so, if
> the controller has to configure that on a per-transfer basis, one
> solution would be to pass the device to the ->priv_xfers().
The speed R/W limitation is only for private transfers. Also the device can have
a limitation to write and not for read data.
This information is obtained with the command GETMXDS which returns the Maximum
Sustained Data Rate for non-CCC messages.
>
>> This could be also applied to i2c transfers.
> Not really. The max SCL frequency is something that applies to the
> whole bus, because all I2C devices have to decode the address when
> messages are sent on the bus to determine if they should ignore or
> process the message.
>
>>> +#endif /* I3C_INTERNAL_H */
>>> diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
>>> new file mode 100644
>>> index ..1c85abac08d5
>>> --- /dev/null
>>> +++ b/drivers/i3c/master.c
>>> @@ -0,0 +1,1433 @@
>>> +// SPDX-License-Identifier: GPL-2.0
>>> +/*
>>> + * Copyright (C) 2017 Cadence Design Systems Inc.
>>> + *
>>> + * Author: Boris Brezillon
>>> + */
>>> +
>>> +#include 
>>> +
>>> +#include "internals.h"
>>> +
>>> +/**
>>> + * i3c_master_entdaa_locked() - start a DAA (Dynamic Address Assignment)
>>> + * procedure
>>> + * @master: master used to send frames on the bus
>>> + *
>>> + * Send a ENTDAA CCC command to start a DAA procedure.
>>> + *
>>> + * Note that this function only sends the ENTDAA CCC command, all the logic
>>> + * behind dynamic address assignment has to be handled in the I3C master
>>> + * driver.
>>> + *
>>> + * This function must be called with the bus lock held in write mode.
>>> + *
>>> + * Return: 0 in case of success, a negative error code otherwise.
>>> + */
>>> +int i3c_master_entdaa_locked(struct i3c_master_controller *master)
>>> +{
>>> +   struct i3c_ccc_cmd_dest dest = { };
>>> +   struct i3c_ccc_cmd cmd = { };
>>> +   int ret;
>>> +
>>> +   dest.addr = I3C_BROADCAST_ADDR;
>>> +   cmd.dests = 
>>> +   cmd.ndests = 1;
>>> +   cmd.rnw = false;
>>> +   cmd.id = I3C_CCC_ENTDAA;
>>> +
>>> +   ret = i3c_master_send_ccc_cmd_locked(master, );
>>> +   if (ret)
>>> +   return ret;
>>> +
>>> +   return 0;
>>> +}
>>> +EXPORT_SYMBOL_GPL(i3c_master_entdaa_locked);  
>>  can you explain the process?
> Not sure what you mean. The ENTDAA is just a CCC command that is used
> to trigger a DAA 

Re: [PATCH v2 2/7] i3c: Add core I3C infrastructure

[Boris Brezillon]

On Fri, 23 Feb 2018 16:56:14 +
Vitor Soares  wrote:


> > +
> > +/**
> > + * struct i3c_ccc_setda - payload passed to ENTTM CCC
> > + *
> > + * @mode: one of the  i3c_ccc_test_mode modes
> > + *
> > + * Information passed to the ENTTM CCC to instruct an I3C device to enter a
> > + * specific test mode.

Oops, copy error. I'll fix the kernel-doc header.

> > + */
> > +struct i3c_ccc_setda {
> > +   u8 addr;
> > +} __packed;  
> 
>  what do you mean with struct? Maybe setdasa? if so, what is the addr?

It's the payload passed to SETDASA and SETNEWDA, hence the generic
_setda suffix. addr is the new dynamic address assigned to the
device.



-- 
Boris Brezillon, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 2/7] i3c: Add core I3C infrastructure

[Boris Brezillon]

Hi Vitor,

On Fri, 23 Feb 2018 16:56:14 +
Vitor Soares  wrote:

> Hi Boris,
> 
> Às 3:16 PM de 12/14/2017, Boris Brezillon escreveu:
> > +
> > +enum i3c_addr_slot_status i3c_bus_get_addr_slot_status(struct i3c_bus *bus,
> > +  u16 addr)
> > +{
> > +   int status, bitpos = addr * 2;
> > +
> > +   if (addr > I2C_MAX_ADDR)
> > +   return I3C_ADDR_SLOT_RSVD;
> > +
> > +   status = bus->addrslots[bitpos / BITS_PER_LONG];
> > +   status >>= bitpos % BITS_PER_LONG;
> > +
> > +   return status & I3C_ADDR_SLOT_STATUS_MASK;
> > +}  
> 
> I don't understand the size of addr. The I3C only allow 7-bit addresses.
> 
> Is the addrslots used to store the addresses and its status?

No, slots are used for both I2C and I3C addresses, and an I2C address
can be 10-bits wide, hence the u16 type.


[...]

> > +/**
> > + * i3c_device_do_priv_xfers() - do I3C SDR private transfers directed to a
> > + * specific device
> > + *
> > + * @dev: device with which the transfers should be done
> > + * @xfers: array of transfers
> > + * @nxfers: number of transfers
> > + *
> > + * Initiate one or several private SDR transfers with @dev.
> > + *
> > + * This function can sleep and thus cannot be called in atomic context.
> > + *
> > + * Return: 0 in case of success, a negative error core otherwise.
> > + */
> > +int i3c_device_do_priv_xfers(struct i3c_device *dev,
> > +struct i3c_priv_xfer *xfers,
> > +int nxfers)
> > +{
> > +   struct i3c_master_controller *master;
> > +   int i, ret;
> > +
> > +   master = i3c_device_get_master(dev);
> > +   if (!master)
> > +   return -EINVAL;
> > +
> > +   i3c_bus_normaluse_lock(master->bus);
> > +   for (i = 0; i < nxfers; i++)
> > +   xfers[i].addr = dev->info.dyn_addr;
> > +
> > +   ret = i3c_master_do_priv_xfers_locked(master, xfers, nxfers);
> > +   i3c_bus_normaluse_unlock(master->bus);
> > +
> > +   return ret;
> > +}
> > +EXPORT_SYMBOL_GPL(i3c_device_do_priv_xfers);  
> 
>  The controller should know the speed mode for each xfer. The SDR0 mode 
> is used by default but if any device have read or write speed 
> limitations the controller can use SDRx.

I might be wrong, but that's not my understanding of the spec. A device
can express a speed limitation for SDR priv transfers, but this
limitation applies to all SDR transfers.

The speed R/W speed limitation is encoded in the device object, so, if
the controller has to configure that on a per-transfer basis, one
solution would be to pass the device to the ->priv_xfers().

> 
> This could be also applied to i2c transfers.

Not really. The max SCL frequency is something that applies to the
whole bus, because all I2C devices have to decode the address when
messages are sent on the bus to determine if they should ignore or
process the message.

> > +#endif /* I3C_INTERNAL_H */
> > diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
> > new file mode 100644
> > index ..1c85abac08d5
> > --- /dev/null
> > +++ b/drivers/i3c/master.c
> > @@ -0,0 +1,1433 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (C) 2017 Cadence Design Systems Inc.
> > + *
> > + * Author: Boris Brezillon
> > + */
> > +
> > +#include 
> > +
> > +#include "internals.h"
> > +
> > +/**
> > + * i3c_master_entdaa_locked() - start a DAA (Dynamic Address Assignment)
> > + * procedure
> > + * @master: master used to send frames on the bus
> > + *
> > + * Send a ENTDAA CCC command to start a DAA procedure.
> > + *
> > + * Note that this function only sends the ENTDAA CCC command, all the logic
> > + * behind dynamic address assignment has to be handled in the I3C master
> > + * driver.
> > + *
> > + * This function must be called with the bus lock held in write mode.
> > + *
> > + * Return: 0 in case of success, a negative error code otherwise.
> > + */
> > +int i3c_master_entdaa_locked(struct i3c_master_controller *master)
> > +{
> > +   struct i3c_ccc_cmd_dest dest = { };
> > +   struct i3c_ccc_cmd cmd = { };
> > +   int ret;
> > +
> > +   dest.addr = I3C_BROADCAST_ADDR;
> > +   cmd.dests = 
> > +   cmd.ndests = 1;
> > +   cmd.rnw = false;
> > +   cmd.id = I3C_CCC_ENTDAA;
> > +
> > +   ret = i3c_master_send_ccc_cmd_locked(master, );
> > +   if (ret)
> > +   return ret;
> > +
> > +   return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(i3c_master_entdaa_locked);  
> 
>  can you explain the process?

Not sure what you mean. The ENTDAA is just a CCC command that is used
to trigger a DAA procedure. What the master controller does when it
sends such a command is likely to be controller dependent, and it might
even be possible that you don't need to call this function in your
controller driver to trigger a DAA. If you want more details about the
bus initialization steps and how the ENTDAA CCC command fits into it I
recommend reading 

Re: [PATCH v2 2/7] i3c: Add core I3C infrastructure

[Vitor Soares]

Hi Boris,

Às 3:16 PM de 12/14/2017, Boris Brezillon escreveu:
> +
> +enum i3c_addr_slot_status i3c_bus_get_addr_slot_status(struct i3c_bus *bus,
> +u16 addr)
> +{
> + int status, bitpos = addr * 2;
> +
> + if (addr > I2C_MAX_ADDR)
> + return I3C_ADDR_SLOT_RSVD;
> +
> + status = bus->addrslots[bitpos / BITS_PER_LONG];
> + status >>= bitpos % BITS_PER_LONG;
> +
> + return status & I3C_ADDR_SLOT_STATUS_MASK;
> +}

I don't understand the size of addr. The I3C only allow 7-bit addresses.

Is the addrslots used to store the addresses and its status?

> +
> +void i3c_bus_set_addr_slot_status(struct i3c_bus *bus, u16 addr,
> +   enum i3c_addr_slot_status status)
> +{
> + int bitpos = addr * 2;
> + unsigned long *ptr;
> +
> + if (addr > I2C_MAX_ADDR)
> + return;
> +
> + ptr = bus->addrslots + (bitpos / BITS_PER_LONG);
> + *ptr &= ~(I3C_ADDR_SLOT_STATUS_MASK << (bitpos % BITS_PER_LONG));
> + *ptr |= status << (bitpos % BITS_PER_LONG);
> +}
> +
> +bool i3c_bus_dev_addr_is_avail(struct i3c_bus *bus, u8 addr)
> +{
> + enum i3c_addr_slot_status status;
> +
> + status = i3c_bus_get_addr_slot_status(bus, addr);
> +
> + return status == I3C_ADDR_SLOT_FREE;
> +}
> +
> +int i3c_bus_get_free_addr(struct i3c_bus *bus, u8 start_addr)
> +{
> + enum i3c_addr_slot_status status;
> + u8 addr;
> +
> + for (addr = start_addr; addr < I3C_MAX_ADDR; addr++) {
> + status = i3c_bus_get_addr_slot_status(bus, addr);
> + if (status == I3C_ADDR_SLOT_FREE)
> + return addr;
> + }
> +
> + return -ENOMEM;
> +}
> +
> +static void i3c_bus_init_addrslots(struct i3c_bus *bus)
> +{
> + int i;
> +
> + /* Addresses 0 to 7 are reserved. */
> + for (i = 0; i < 8; i++)
> + i3c_bus_set_addr_slot_status(bus, i, I3C_ADDR_SLOT_RSVD);
> +
> + /*
> +  * Reserve broadcast address and all addresses that might collide
> +  * with the broadcast address when facing a single bit error.
> +  */
> + i3c_bus_set_addr_slot_status(bus, I3C_BROADCAST_ADDR,
> +  I3C_ADDR_SLOT_RSVD);
> + for (i = 0; i < 7; i++)
> + i3c_bus_set_addr_slot_status(bus, I3C_BROADCAST_ADDR ^ BIT(i),
> +  I3C_ADDR_SLOT_RSVD);
> +}
> +
> +static const char * const i3c_bus_mode_strings[] = {
> + [I3C_BUS_MODE_PURE] = "pure",
> + [I3C_BUS_MODE_MIXED_FAST] = "mixed-fast",
> + [I3C_BUS_MODE_MIXED_SLOW] = "mixed-slow",
> +};
> +
> +static ssize_t mode_show(struct device *dev,
> +  struct device_attribute *da,
> +  char *buf)
> +{
> + struct i3c_bus *i3cbus = container_of(dev, struct i3c_bus, dev);
> + ssize_t ret;
> +
> + i3c_bus_normaluse_lock(i3cbus);
> + if (i3cbus->mode < 0 ||
> + i3cbus->mode > ARRAY_SIZE(i3c_bus_mode_strings) ||
> + !i3c_bus_mode_strings[i3cbus->mode])
> + ret = sprintf(buf, "unknown\n");
> + else
> + ret = sprintf(buf, "%s\n", i3c_bus_mode_strings[i3cbus->mode]);
> + i3c_bus_normaluse_unlock(i3cbus);
> +
> + return ret;
> +}
> +static DEVICE_ATTR_RO(mode);
> +
> +static ssize_t current_master_show(struct device *dev,
> +struct device_attribute *da,
> +char *buf)
> +{
> + struct i3c_bus *i3cbus = container_of(dev, struct i3c_bus, dev);
> + ssize_t ret;
> +
> + i3c_bus_normaluse_lock(i3cbus);
> + ret = sprintf(buf, "%s\n", dev_name(>cur_master->dev));
> + i3c_bus_normaluse_unlock(i3cbus);
> +
> + return ret;
> +}
> +static DEVICE_ATTR_RO(current_master);
> +
> +static ssize_t i3c_scl_frequency_show(struct device *dev,
> +   struct device_attribute *da,
> +   char *buf)
> +{
> + struct i3c_bus *i3cbus = container_of(dev, struct i3c_bus, dev);
> + ssize_t ret;
> +
> + i3c_bus_normaluse_lock(i3cbus);
> + ret = sprintf(buf, "%ld\n", i3cbus->scl_rate.i3c);
> + i3c_bus_normaluse_unlock(i3cbus);
> +
> + return ret;
> +}
> +static DEVICE_ATTR_RO(i3c_scl_frequency);
> +
> +static ssize_t i2c_scl_frequency_show(struct device *dev,
> +   struct device_attribute *da,
> +   char *buf)
> +{
> + struct i3c_bus *i3cbus = container_of(dev, struct i3c_bus, dev);
> + ssize_t ret;
> +
> + i3c_bus_normaluse_lock(i3cbus);
> + ret = sprintf(buf, "%ld\n", i3cbus->scl_rate.i2c);
> + i3c_bus_normaluse_unlock(i3cbus);
> +
> + return ret;
> +}
> +static DEVICE_ATTR_RO(i2c_scl_frequency);
> +
> +static struct attribute *i3c_busdev_attrs[] = {
> + _attr_mode.attr,
> + _attr_current_master.attr,
> + _attr_i3c_scl_frequency.attr,
> + _attr_i2c_scl_frequency.attr,
> + 

Re: [PATCH v2 2/7] i3c: Add core I3C infrastructure

[Greg Kroah-Hartman]

On Wed, Feb 21, 2018 at 03:22:48PM +0100, Boris Brezillon wrote:
> Hi Greg,
> 
> On Tue, 19 Dec 2017 10:36:43 +0100
> Greg Kroah-Hartman  wrote:
> 
> > On Tue, Dec 19, 2017 at 10:28:58AM +0100, Boris Brezillon wrote:
> > > On Tue, 19 Dec 2017 10:21:19 +0100
> > > Greg Kroah-Hartman  wrote:
> > >   
> > > > On Tue, Dec 19, 2017 at 10:13:36AM +0100, Boris Brezillon wrote:  
> > > > > On Tue, 19 Dec 2017 10:09:00 +0100
> > > > > Boris Brezillon  wrote:
> > > > > 
> > > > > > On Tue, 19 Dec 2017 09:52:50 +0100
> > > > > > Greg Kroah-Hartman  wrote:
> > > > > > 
> > > > > > > On Thu, Dec 14, 2017 at 04:16:05PM +0100, Boris Brezillon wrote:  
> > > > > > > 
> > > > > > > > +/**
> > > > > > > > + * i3c_device_match_id() - Find the I3C device ID entry 
> > > > > > > > matching an I3C dev
> > > > > > > > + * @i3cdev: the I3C device we're searching a match for
> > > > > > > > + * @id_table: the I3C device ID table
> > > > > > > > + *
> > > > > > > > + * Return: a pointer to the first entry matching @i3cdev, or 
> > > > > > > > NULL if there's
> > > > > > > > + *no match.
> > > > > > > > + */
> > > > > > > > +const struct i3c_device_id *
> > > > > > > > +i3c_device_match_id(struct i3c_device *i3cdev,
> > > > > > > > +   const struct i3c_device_id *id_table)
> > > > > > > > +{
> > > > > > > > +   const struct i3c_device_id *id;
> > > > > > > > +
> > > > > > > > +   /*
> > > > > > > > +* The lower 32bits of the provisional ID is just 
> > > > > > > > filled with a random
> > > > > > > > +* value, try to match using DCR info.
> > > > > > > > +*/
> > > > > > > > +   if (!I3C_PID_RND_LOWER_32BITS(i3cdev->info.pid)) {
> > > > > > > > +   u16 manuf = I3C_PID_MANUF_ID(i3cdev->info.pid);
> > > > > > > > +   u16 part = I3C_PID_PART_ID(i3cdev->info.pid);
> > > > > > > > +   u16 ext_info = 
> > > > > > > > I3C_PID_EXTRA_INFO(i3cdev->info.pid);
> > > > > > > > +
> > > > > > > > +   /* First try to match by manufacturer/part ID. 
> > > > > > > > */
> > > > > > > > +   for (id = id_table; id->match_flags != 0; id++) 
> > > > > > > > {
> > > > > > > > +   if ((id->match_flags & 
> > > > > > > > I3C_MATCH_MANUF_AND_PART) !=
> > > > > > > > +   I3C_MATCH_MANUF_AND_PART)
> > > > > > > > +   continue;
> > > > > > > > +
> > > > > > > > +   if (manuf != id->manuf_id || part != 
> > > > > > > > id->part_id)
> > > > > > > > +   continue;
> > > > > > > > +
> > > > > > > > +   if ((id->match_flags & 
> > > > > > > > I3C_MATCH_EXTRA_INFO) &&
> > > > > > > > +   ext_info != id->extra_info)
> > > > > > > > +   continue;
> > > > > > > > +
> > > > > > > > +   return id;
> > > > > > > > +   }
> > > > > > > > +   }
> > > > > > > > +
> > > > > > > > +   /* Fallback to DCR match. */
> > > > > > > > +   for (id = id_table; id->match_flags != 0; id++) {
> > > > > > > > +   if ((id->match_flags & I3C_MATCH_DCR) &&
> > > > > > > > +   id->dcr == i3cdev->info.dcr)
> > > > > > > > +   return id;
> > > > > > > > +   }
> > > > > > > > +
> > > > > > > > +   return NULL;
> > > > > > > > +}
> > > > > > > > +EXPORT_SYMBOL_GPL(i3c_device_match_id);
> > > > > > > 
> > > > > > > I just picked one random export here, but it feels like you are
> > > > > > > exporting a bunch of symbols you don't need to.  Why would 
> > > > > > > something
> > > > > > > outside of the i3c "core" need to call this function?  
> > > > > > 
> > > > > > Because I'm not passing the i3c_device_id to the ->probe() method, 
> > > > > > and
> > > > > > if the driver is supporting different variants of the device, it may
> > > > > > want to know which one is being probed.
> > > > > > 
> > > > > > I considered retrieving this information in the core just before 
> > > > > > probing
> > > > > > the driver and passing it to the ->probe() function, but it means
> > > > > > having an extra i3c_device_match_id() call for everyone even those 
> > > > > > who
> > > > > > don't care about the device_id information, so I thought exporting 
> > > > > > this
> > > > > > function was a good alternative (device drivers can use it when they
> > > > > > actually need to retrieve the device_id).
> > > > > > 
> > > > > > Anyway, that's something I can change if you think passing the
> > > > > > i3c_device_id to the ->probe() method is preferable.
> > > > > > 
> > > > > > > Have you looked
> > > > > > > to see if you really have callers for everything you are 
> > > > > > > exporting?  
> > > > > > 
> > > > > > Yes, I tried to only export functions that I 

Re: [PATCH v2 2/7] i3c: Add core I3C infrastructure

[Boris Brezillon]

Hi Greg,

On Tue, 19 Dec 2017 10:36:43 +0100
Greg Kroah-Hartman  wrote:

> On Tue, Dec 19, 2017 at 10:28:58AM +0100, Boris Brezillon wrote:
> > On Tue, 19 Dec 2017 10:21:19 +0100
> > Greg Kroah-Hartman  wrote:
> >   
> > > On Tue, Dec 19, 2017 at 10:13:36AM +0100, Boris Brezillon wrote:  
> > > > On Tue, 19 Dec 2017 10:09:00 +0100
> > > > Boris Brezillon  wrote:
> > > > 
> > > > > On Tue, 19 Dec 2017 09:52:50 +0100
> > > > > Greg Kroah-Hartman  wrote:
> > > > > 
> > > > > > On Thu, Dec 14, 2017 at 04:16:05PM +0100, Boris Brezillon wrote:
> > > > > >   
> > > > > > > +/**
> > > > > > > + * i3c_device_match_id() - Find the I3C device ID entry matching 
> > > > > > > an I3C dev
> > > > > > > + * @i3cdev: the I3C device we're searching a match for
> > > > > > > + * @id_table: the I3C device ID table
> > > > > > > + *
> > > > > > > + * Return: a pointer to the first entry matching @i3cdev, or 
> > > > > > > NULL if there's
> > > > > > > + *  no match.
> > > > > > > + */
> > > > > > > +const struct i3c_device_id *
> > > > > > > +i3c_device_match_id(struct i3c_device *i3cdev,
> > > > > > > + const struct i3c_device_id *id_table)
> > > > > > > +{
> > > > > > > + const struct i3c_device_id *id;
> > > > > > > +
> > > > > > > + /*
> > > > > > > +  * The lower 32bits of the provisional ID is just filled with a 
> > > > > > > random
> > > > > > > +  * value, try to match using DCR info.
> > > > > > > +  */
> > > > > > > + if (!I3C_PID_RND_LOWER_32BITS(i3cdev->info.pid)) {
> > > > > > > + u16 manuf = I3C_PID_MANUF_ID(i3cdev->info.pid);
> > > > > > > + u16 part = I3C_PID_PART_ID(i3cdev->info.pid);
> > > > > > > + u16 ext_info = I3C_PID_EXTRA_INFO(i3cdev->info.pid);
> > > > > > > +
> > > > > > > + /* First try to match by manufacturer/part ID. */
> > > > > > > + for (id = id_table; id->match_flags != 0; id++) {
> > > > > > > + if ((id->match_flags & 
> > > > > > > I3C_MATCH_MANUF_AND_PART) !=
> > > > > > > + I3C_MATCH_MANUF_AND_PART)
> > > > > > > + continue;
> > > > > > > +
> > > > > > > + if (manuf != id->manuf_id || part != 
> > > > > > > id->part_id)
> > > > > > > + continue;
> > > > > > > +
> > > > > > > + if ((id->match_flags & I3C_MATCH_EXTRA_INFO) &&
> > > > > > > + ext_info != id->extra_info)
> > > > > > > + continue;
> > > > > > > +
> > > > > > > + return id;
> > > > > > > + }
> > > > > > > + }
> > > > > > > +
> > > > > > > + /* Fallback to DCR match. */
> > > > > > > + for (id = id_table; id->match_flags != 0; id++) {
> > > > > > > + if ((id->match_flags & I3C_MATCH_DCR) &&
> > > > > > > + id->dcr == i3cdev->info.dcr)
> > > > > > > + return id;
> > > > > > > + }
> > > > > > > +
> > > > > > > + return NULL;
> > > > > > > +}
> > > > > > > +EXPORT_SYMBOL_GPL(i3c_device_match_id);
> > > > > > 
> > > > > > I just picked one random export here, but it feels like you are
> > > > > > exporting a bunch of symbols you don't need to.  Why would something
> > > > > > outside of the i3c "core" need to call this function?  
> > > > > 
> > > > > Because I'm not passing the i3c_device_id to the ->probe() method, and
> > > > > if the driver is supporting different variants of the device, it may
> > > > > want to know which one is being probed.
> > > > > 
> > > > > I considered retrieving this information in the core just before 
> > > > > probing
> > > > > the driver and passing it to the ->probe() function, but it means
> > > > > having an extra i3c_device_match_id() call for everyone even those who
> > > > > don't care about the device_id information, so I thought exporting 
> > > > > this
> > > > > function was a good alternative (device drivers can use it when they
> > > > > actually need to retrieve the device_id).
> > > > > 
> > > > > Anyway, that's something I can change if you think passing the
> > > > > i3c_device_id to the ->probe() method is preferable.
> > > > > 
> > > > > > Have you looked
> > > > > > to see if you really have callers for everything you are exporting? 
> > > > > >  
> > > > > 
> > > > > Yes, I tried to only export functions that I think will be needed by
> > > > > I3C device drivers and I3C master drivers. Note that I didn't post the
> > > > > dummy device driver I developed to test the framework (partly because
> > > > > this is 
> > > > 
> > > > Sorry, I hit the send button before finishing my sentence :-).
> > > > 
> > > > "
> > > > Note that I didn't post the dummy device driver [1] I developed to test
> > > > the framework (partly because the quality of the code does not meet
> > > > mainline standards and I was ashamed of posting it publicly :-)), but
> > > > this 

Re: [PATCH v2 2/7] i3c: Add core I3C infrastructure

[Greg Kroah-Hartman]

On Tue, Dec 19, 2017 at 10:28:58AM +0100, Boris Brezillon wrote:
> On Tue, 19 Dec 2017 10:21:19 +0100
> Greg Kroah-Hartman  wrote:
> 
> > On Tue, Dec 19, 2017 at 10:13:36AM +0100, Boris Brezillon wrote:
> > > On Tue, 19 Dec 2017 10:09:00 +0100
> > > Boris Brezillon  wrote:
> > >   
> > > > On Tue, 19 Dec 2017 09:52:50 +0100
> > > > Greg Kroah-Hartman  wrote:
> > > >   
> > > > > On Thu, Dec 14, 2017 at 04:16:05PM +0100, Boris Brezillon wrote:
> > > > > > +/**
> > > > > > + * i3c_device_match_id() - Find the I3C device ID entry matching 
> > > > > > an I3C dev
> > > > > > + * @i3cdev: the I3C device we're searching a match for
> > > > > > + * @id_table: the I3C device ID table
> > > > > > + *
> > > > > > + * Return: a pointer to the first entry matching @i3cdev, or NULL 
> > > > > > if there's
> > > > > > + *no match.
> > > > > > + */
> > > > > > +const struct i3c_device_id *
> > > > > > +i3c_device_match_id(struct i3c_device *i3cdev,
> > > > > > +   const struct i3c_device_id *id_table)
> > > > > > +{
> > > > > > +   const struct i3c_device_id *id;
> > > > > > +
> > > > > > +   /*
> > > > > > +* The lower 32bits of the provisional ID is just filled with a 
> > > > > > random
> > > > > > +* value, try to match using DCR info.
> > > > > > +*/
> > > > > > +   if (!I3C_PID_RND_LOWER_32BITS(i3cdev->info.pid)) {
> > > > > > +   u16 manuf = I3C_PID_MANUF_ID(i3cdev->info.pid);
> > > > > > +   u16 part = I3C_PID_PART_ID(i3cdev->info.pid);
> > > > > > +   u16 ext_info = I3C_PID_EXTRA_INFO(i3cdev->info.pid);
> > > > > > +
> > > > > > +   /* First try to match by manufacturer/part ID. */
> > > > > > +   for (id = id_table; id->match_flags != 0; id++) {
> > > > > > +   if ((id->match_flags & 
> > > > > > I3C_MATCH_MANUF_AND_PART) !=
> > > > > > +   I3C_MATCH_MANUF_AND_PART)
> > > > > > +   continue;
> > > > > > +
> > > > > > +   if (manuf != id->manuf_id || part != 
> > > > > > id->part_id)
> > > > > > +   continue;
> > > > > > +
> > > > > > +   if ((id->match_flags & I3C_MATCH_EXTRA_INFO) &&
> > > > > > +   ext_info != id->extra_info)
> > > > > > +   continue;
> > > > > > +
> > > > > > +   return id;
> > > > > > +   }
> > > > > > +   }
> > > > > > +
> > > > > > +   /* Fallback to DCR match. */
> > > > > > +   for (id = id_table; id->match_flags != 0; id++) {
> > > > > > +   if ((id->match_flags & I3C_MATCH_DCR) &&
> > > > > > +   id->dcr == i3cdev->info.dcr)
> > > > > > +   return id;
> > > > > > +   }
> > > > > > +
> > > > > > +   return NULL;
> > > > > > +}
> > > > > > +EXPORT_SYMBOL_GPL(i3c_device_match_id);  
> > > > > 
> > > > > I just picked one random export here, but it feels like you are
> > > > > exporting a bunch of symbols you don't need to.  Why would something
> > > > > outside of the i3c "core" need to call this function?
> > > > 
> > > > Because I'm not passing the i3c_device_id to the ->probe() method, and
> > > > if the driver is supporting different variants of the device, it may
> > > > want to know which one is being probed.
> > > > 
> > > > I considered retrieving this information in the core just before probing
> > > > the driver and passing it to the ->probe() function, but it means
> > > > having an extra i3c_device_match_id() call for everyone even those who
> > > > don't care about the device_id information, so I thought exporting this
> > > > function was a good alternative (device drivers can use it when they
> > > > actually need to retrieve the device_id).
> > > > 
> > > > Anyway, that's something I can change if you think passing the
> > > > i3c_device_id to the ->probe() method is preferable.
> > > >   
> > > > > Have you looked
> > > > > to see if you really have callers for everything you are exporting?   
> > > > >  
> > > > 
> > > > Yes, I tried to only export functions that I think will be needed by
> > > > I3C device drivers and I3C master drivers. Note that I didn't post the
> > > > dummy device driver I developed to test the framework (partly because
> > > > this is   
> > > 
> > > Sorry, I hit the send button before finishing my sentence :-).
> > > 
> > > "
> > > Note that I didn't post the dummy device driver [1] I developed to test
> > > the framework (partly because the quality of the code does not meet
> > > mainline standards and I was ashamed of posting it publicly :-)), but
> > > this driver is using some of the exported functions.
> > > "  
> > 
> > We don't export functions that has no in-kernel users :)
> 
> But then, I can't export device driver related functions, because
> there's no official device driver yet :-). So what should I do?

Export them when you have a driver.  Or 

Re: [PATCH v2 2/7] i3c: Add core I3C infrastructure

[Boris Brezillon]

On Tue, 19 Dec 2017 10:21:19 +0100
Greg Kroah-Hartman  wrote:

> On Tue, Dec 19, 2017 at 10:13:36AM +0100, Boris Brezillon wrote:
> > On Tue, 19 Dec 2017 10:09:00 +0100
> > Boris Brezillon  wrote:
> >   
> > > On Tue, 19 Dec 2017 09:52:50 +0100
> > > Greg Kroah-Hartman  wrote:
> > >   
> > > > On Thu, Dec 14, 2017 at 04:16:05PM +0100, Boris Brezillon wrote:
> > > > > +/**
> > > > > + * i3c_device_match_id() - Find the I3C device ID entry matching an 
> > > > > I3C dev
> > > > > + * @i3cdev: the I3C device we're searching a match for
> > > > > + * @id_table: the I3C device ID table
> > > > > + *
> > > > > + * Return: a pointer to the first entry matching @i3cdev, or NULL if 
> > > > > there's
> > > > > + *  no match.
> > > > > + */
> > > > > +const struct i3c_device_id *
> > > > > +i3c_device_match_id(struct i3c_device *i3cdev,
> > > > > + const struct i3c_device_id *id_table)
> > > > > +{
> > > > > + const struct i3c_device_id *id;
> > > > > +
> > > > > + /*
> > > > > +  * The lower 32bits of the provisional ID is just filled with a 
> > > > > random
> > > > > +  * value, try to match using DCR info.
> > > > > +  */
> > > > > + if (!I3C_PID_RND_LOWER_32BITS(i3cdev->info.pid)) {
> > > > > + u16 manuf = I3C_PID_MANUF_ID(i3cdev->info.pid);
> > > > > + u16 part = I3C_PID_PART_ID(i3cdev->info.pid);
> > > > > + u16 ext_info = I3C_PID_EXTRA_INFO(i3cdev->info.pid);
> > > > > +
> > > > > + /* First try to match by manufacturer/part ID. */
> > > > > + for (id = id_table; id->match_flags != 0; id++) {
> > > > > + if ((id->match_flags & 
> > > > > I3C_MATCH_MANUF_AND_PART) !=
> > > > > + I3C_MATCH_MANUF_AND_PART)
> > > > > + continue;
> > > > > +
> > > > > + if (manuf != id->manuf_id || part != 
> > > > > id->part_id)
> > > > > + continue;
> > > > > +
> > > > > + if ((id->match_flags & I3C_MATCH_EXTRA_INFO) &&
> > > > > + ext_info != id->extra_info)
> > > > > + continue;
> > > > > +
> > > > > + return id;
> > > > > + }
> > > > > + }
> > > > > +
> > > > > + /* Fallback to DCR match. */
> > > > > + for (id = id_table; id->match_flags != 0; id++) {
> > > > > + if ((id->match_flags & I3C_MATCH_DCR) &&
> > > > > + id->dcr == i3cdev->info.dcr)
> > > > > + return id;
> > > > > + }
> > > > > +
> > > > > + return NULL;
> > > > > +}
> > > > > +EXPORT_SYMBOL_GPL(i3c_device_match_id);  
> > > > 
> > > > I just picked one random export here, but it feels like you are
> > > > exporting a bunch of symbols you don't need to.  Why would something
> > > > outside of the i3c "core" need to call this function?
> > > 
> > > Because I'm not passing the i3c_device_id to the ->probe() method, and
> > > if the driver is supporting different variants of the device, it may
> > > want to know which one is being probed.
> > > 
> > > I considered retrieving this information in the core just before probing
> > > the driver and passing it to the ->probe() function, but it means
> > > having an extra i3c_device_match_id() call for everyone even those who
> > > don't care about the device_id information, so I thought exporting this
> > > function was a good alternative (device drivers can use it when they
> > > actually need to retrieve the device_id).
> > > 
> > > Anyway, that's something I can change if you think passing the
> > > i3c_device_id to the ->probe() method is preferable.
> > >   
> > > > Have you looked
> > > > to see if you really have callers for everything you are exporting?
> > > 
> > > Yes, I tried to only export functions that I think will be needed by
> > > I3C device drivers and I3C master drivers. Note that I didn't post the
> > > dummy device driver I developed to test the framework (partly because
> > > this is   
> > 
> > Sorry, I hit the send button before finishing my sentence :-).
> > 
> > "
> > Note that I didn't post the dummy device driver [1] I developed to test
> > the framework (partly because the quality of the code does not meet
> > mainline standards and I was ashamed of posting it publicly :-)), but
> > this driver is using some of the exported functions.
> > "  
> 
> We don't export functions that has no in-kernel users :)

But then, I can't export device driver related functions, because
there's no official device driver yet :-). So what should I do?
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 2/7] i3c: Add core I3C infrastructure

[Greg Kroah-Hartman]

On Tue, Dec 19, 2017 at 10:13:36AM +0100, Boris Brezillon wrote:
> On Tue, 19 Dec 2017 10:09:00 +0100
> Boris Brezillon  wrote:
> 
> > On Tue, 19 Dec 2017 09:52:50 +0100
> > Greg Kroah-Hartman  wrote:
> > 
> > > On Thu, Dec 14, 2017 at 04:16:05PM +0100, Boris Brezillon wrote:  
> > > > +/**
> > > > + * i3c_device_match_id() - Find the I3C device ID entry matching an 
> > > > I3C dev
> > > > + * @i3cdev: the I3C device we're searching a match for
> > > > + * @id_table: the I3C device ID table
> > > > + *
> > > > + * Return: a pointer to the first entry matching @i3cdev, or NULL if 
> > > > there's
> > > > + *no match.
> > > > + */
> > > > +const struct i3c_device_id *
> > > > +i3c_device_match_id(struct i3c_device *i3cdev,
> > > > +   const struct i3c_device_id *id_table)
> > > > +{
> > > > +   const struct i3c_device_id *id;
> > > > +
> > > > +   /*
> > > > +* The lower 32bits of the provisional ID is just filled with a 
> > > > random
> > > > +* value, try to match using DCR info.
> > > > +*/
> > > > +   if (!I3C_PID_RND_LOWER_32BITS(i3cdev->info.pid)) {
> > > > +   u16 manuf = I3C_PID_MANUF_ID(i3cdev->info.pid);
> > > > +   u16 part = I3C_PID_PART_ID(i3cdev->info.pid);
> > > > +   u16 ext_info = I3C_PID_EXTRA_INFO(i3cdev->info.pid);
> > > > +
> > > > +   /* First try to match by manufacturer/part ID. */
> > > > +   for (id = id_table; id->match_flags != 0; id++) {
> > > > +   if ((id->match_flags & 
> > > > I3C_MATCH_MANUF_AND_PART) !=
> > > > +   I3C_MATCH_MANUF_AND_PART)
> > > > +   continue;
> > > > +
> > > > +   if (manuf != id->manuf_id || part != 
> > > > id->part_id)
> > > > +   continue;
> > > > +
> > > > +   if ((id->match_flags & I3C_MATCH_EXTRA_INFO) &&
> > > > +   ext_info != id->extra_info)
> > > > +   continue;
> > > > +
> > > > +   return id;
> > > > +   }
> > > > +   }
> > > > +
> > > > +   /* Fallback to DCR match. */
> > > > +   for (id = id_table; id->match_flags != 0; id++) {
> > > > +   if ((id->match_flags & I3C_MATCH_DCR) &&
> > > > +   id->dcr == i3cdev->info.dcr)
> > > > +   return id;
> > > > +   }
> > > > +
> > > > +   return NULL;
> > > > +}
> > > > +EXPORT_SYMBOL_GPL(i3c_device_match_id);
> > > 
> > > I just picked one random export here, but it feels like you are
> > > exporting a bunch of symbols you don't need to.  Why would something
> > > outside of the i3c "core" need to call this function?  
> > 
> > Because I'm not passing the i3c_device_id to the ->probe() method, and
> > if the driver is supporting different variants of the device, it may
> > want to know which one is being probed.
> > 
> > I considered retrieving this information in the core just before probing
> > the driver and passing it to the ->probe() function, but it means
> > having an extra i3c_device_match_id() call for everyone even those who
> > don't care about the device_id information, so I thought exporting this
> > function was a good alternative (device drivers can use it when they
> > actually need to retrieve the device_id).
> > 
> > Anyway, that's something I can change if you think passing the
> > i3c_device_id to the ->probe() method is preferable.
> > 
> > > Have you looked
> > > to see if you really have callers for everything you are exporting?  
> > 
> > Yes, I tried to only export functions that I think will be needed by
> > I3C device drivers and I3C master drivers. Note that I didn't post the
> > dummy device driver I developed to test the framework (partly because
> > this is 
> 
> Sorry, I hit the send button before finishing my sentence :-).
> 
> "
> Note that I didn't post the dummy device driver [1] I developed to test
> the framework (partly because the quality of the code does not meet
> mainline standards and I was ashamed of posting it publicly :-)), but
> this driver is using some of the exported functions.
> "

We don't export functions that has no in-kernel users :)

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 2/7] i3c: Add core I3C infrastructure

[Boris Brezillon]

On Tue, 19 Dec 2017 10:09:00 +0100
Boris Brezillon  wrote:

> On Tue, 19 Dec 2017 09:52:50 +0100
> Greg Kroah-Hartman  wrote:
> 
> > On Thu, Dec 14, 2017 at 04:16:05PM +0100, Boris Brezillon wrote:  
> > > +/**
> > > + * i3c_device_match_id() - Find the I3C device ID entry matching an I3C 
> > > dev
> > > + * @i3cdev: the I3C device we're searching a match for
> > > + * @id_table: the I3C device ID table
> > > + *
> > > + * Return: a pointer to the first entry matching @i3cdev, or NULL if 
> > > there's
> > > + *  no match.
> > > + */
> > > +const struct i3c_device_id *
> > > +i3c_device_match_id(struct i3c_device *i3cdev,
> > > + const struct i3c_device_id *id_table)
> > > +{
> > > + const struct i3c_device_id *id;
> > > +
> > > + /*
> > > +  * The lower 32bits of the provisional ID is just filled with a random
> > > +  * value, try to match using DCR info.
> > > +  */
> > > + if (!I3C_PID_RND_LOWER_32BITS(i3cdev->info.pid)) {
> > > + u16 manuf = I3C_PID_MANUF_ID(i3cdev->info.pid);
> > > + u16 part = I3C_PID_PART_ID(i3cdev->info.pid);
> > > + u16 ext_info = I3C_PID_EXTRA_INFO(i3cdev->info.pid);
> > > +
> > > + /* First try to match by manufacturer/part ID. */
> > > + for (id = id_table; id->match_flags != 0; id++) {
> > > + if ((id->match_flags & I3C_MATCH_MANUF_AND_PART) !=
> > > + I3C_MATCH_MANUF_AND_PART)
> > > + continue;
> > > +
> > > + if (manuf != id->manuf_id || part != id->part_id)
> > > + continue;
> > > +
> > > + if ((id->match_flags & I3C_MATCH_EXTRA_INFO) &&
> > > + ext_info != id->extra_info)
> > > + continue;
> > > +
> > > + return id;
> > > + }
> > > + }
> > > +
> > > + /* Fallback to DCR match. */
> > > + for (id = id_table; id->match_flags != 0; id++) {
> > > + if ((id->match_flags & I3C_MATCH_DCR) &&
> > > + id->dcr == i3cdev->info.dcr)
> > > + return id;
> > > + }
> > > +
> > > + return NULL;
> > > +}
> > > +EXPORT_SYMBOL_GPL(i3c_device_match_id);
> > 
> > I just picked one random export here, but it feels like you are
> > exporting a bunch of symbols you don't need to.  Why would something
> > outside of the i3c "core" need to call this function?  
> 
> Because I'm not passing the i3c_device_id to the ->probe() method, and
> if the driver is supporting different variants of the device, it may
> want to know which one is being probed.
> 
> I considered retrieving this information in the core just before probing
> the driver and passing it to the ->probe() function, but it means
> having an extra i3c_device_match_id() call for everyone even those who
> don't care about the device_id information, so I thought exporting this
> function was a good alternative (device drivers can use it when they
> actually need to retrieve the device_id).
> 
> Anyway, that's something I can change if you think passing the
> i3c_device_id to the ->probe() method is preferable.
> 
> > Have you looked
> > to see if you really have callers for everything you are exporting?  
> 
> Yes, I tried to only export functions that I think will be needed by
> I3C device drivers and I3C master drivers. Note that I didn't post the
> dummy device driver I developed to test the framework (partly because
> this is 

Sorry, I hit the send button before finishing my sentence :-).

"
Note that I didn't post the dummy device driver [1] I developed to test
the framework (partly because the quality of the code does not meet
mainline standards and I was ashamed of posting it publicly :-)), but
this driver is using some of the exported functions.
"

[1]https://github.com/bbrezillon/linux-0day/commit/10054caf3493524b6ae352a1bdcb71e82c885a6e
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 2/7] i3c: Add core I3C infrastructure

[Boris Brezillon]

On Tue, 19 Dec 2017 09:52:50 +0100
Greg Kroah-Hartman  wrote:

> On Thu, Dec 14, 2017 at 04:16:05PM +0100, Boris Brezillon wrote:
> > +/**
> > + * i3c_device_match_id() - Find the I3C device ID entry matching an I3C dev
> > + * @i3cdev: the I3C device we're searching a match for
> > + * @id_table: the I3C device ID table
> > + *
> > + * Return: a pointer to the first entry matching @i3cdev, or NULL if 
> > there's
> > + *no match.
> > + */
> > +const struct i3c_device_id *
> > +i3c_device_match_id(struct i3c_device *i3cdev,
> > +   const struct i3c_device_id *id_table)
> > +{
> > +   const struct i3c_device_id *id;
> > +
> > +   /*
> > +* The lower 32bits of the provisional ID is just filled with a random
> > +* value, try to match using DCR info.
> > +*/
> > +   if (!I3C_PID_RND_LOWER_32BITS(i3cdev->info.pid)) {
> > +   u16 manuf = I3C_PID_MANUF_ID(i3cdev->info.pid);
> > +   u16 part = I3C_PID_PART_ID(i3cdev->info.pid);
> > +   u16 ext_info = I3C_PID_EXTRA_INFO(i3cdev->info.pid);
> > +
> > +   /* First try to match by manufacturer/part ID. */
> > +   for (id = id_table; id->match_flags != 0; id++) {
> > +   if ((id->match_flags & I3C_MATCH_MANUF_AND_PART) !=
> > +   I3C_MATCH_MANUF_AND_PART)
> > +   continue;
> > +
> > +   if (manuf != id->manuf_id || part != id->part_id)
> > +   continue;
> > +
> > +   if ((id->match_flags & I3C_MATCH_EXTRA_INFO) &&
> > +   ext_info != id->extra_info)
> > +   continue;
> > +
> > +   return id;
> > +   }
> > +   }
> > +
> > +   /* Fallback to DCR match. */
> > +   for (id = id_table; id->match_flags != 0; id++) {
> > +   if ((id->match_flags & I3C_MATCH_DCR) &&
> > +   id->dcr == i3cdev->info.dcr)
> > +   return id;
> > +   }
> > +
> > +   return NULL;
> > +}
> > +EXPORT_SYMBOL_GPL(i3c_device_match_id);  
> 
> I just picked one random export here, but it feels like you are
> exporting a bunch of symbols you don't need to.  Why would something
> outside of the i3c "core" need to call this function?

Because I'm not passing the i3c_device_id to the ->probe() method, and
if the driver is supporting different variants of the device, it may
want to know which one is being probed.

I considered retrieving this information in the core just before probing
the driver and passing it to the ->probe() function, but it means
having an extra i3c_device_match_id() call for everyone even those who
don't care about the device_id information, so I thought exporting this
function was a good alternative (device drivers can use it when they
actually need to retrieve the device_id).

Anyway, that's something I can change if you think passing the
i3c_device_id to the ->probe() method is preferable.

> Have you looked
> to see if you really have callers for everything you are exporting?

Yes, I tried to only export functions that I think will be needed by
I3C device drivers and I3C master drivers. Note that I didn't post the
dummy device driver I developed to test the framework (partly because
this is 

> 
> Other than that, the driver core interaction looks good now, nice job.

Thanks.

Boris
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 2/7] i3c: Add core I3C infrastructure

[Greg Kroah-Hartman]

On Thu, Dec 14, 2017 at 04:16:05PM +0100, Boris Brezillon wrote:
> +/**
> + * i3c_device_match_id() - Find the I3C device ID entry matching an I3C dev
> + * @i3cdev: the I3C device we're searching a match for
> + * @id_table: the I3C device ID table
> + *
> + * Return: a pointer to the first entry matching @i3cdev, or NULL if there's
> + *  no match.
> + */
> +const struct i3c_device_id *
> +i3c_device_match_id(struct i3c_device *i3cdev,
> + const struct i3c_device_id *id_table)
> +{
> + const struct i3c_device_id *id;
> +
> + /*
> +  * The lower 32bits of the provisional ID is just filled with a random
> +  * value, try to match using DCR info.
> +  */
> + if (!I3C_PID_RND_LOWER_32BITS(i3cdev->info.pid)) {
> + u16 manuf = I3C_PID_MANUF_ID(i3cdev->info.pid);
> + u16 part = I3C_PID_PART_ID(i3cdev->info.pid);
> + u16 ext_info = I3C_PID_EXTRA_INFO(i3cdev->info.pid);
> +
> + /* First try to match by manufacturer/part ID. */
> + for (id = id_table; id->match_flags != 0; id++) {
> + if ((id->match_flags & I3C_MATCH_MANUF_AND_PART) !=
> + I3C_MATCH_MANUF_AND_PART)
> + continue;
> +
> + if (manuf != id->manuf_id || part != id->part_id)
> + continue;
> +
> + if ((id->match_flags & I3C_MATCH_EXTRA_INFO) &&
> + ext_info != id->extra_info)
> + continue;
> +
> + return id;
> + }
> + }
> +
> + /* Fallback to DCR match. */
> + for (id = id_table; id->match_flags != 0; id++) {
> + if ((id->match_flags & I3C_MATCH_DCR) &&
> + id->dcr == i3cdev->info.dcr)
> + return id;
> + }
> +
> + return NULL;
> +}
> +EXPORT_SYMBOL_GPL(i3c_device_match_id);

I just picked one random export here, but it feels like you are
exporting a bunch of symbols you don't need to.  Why would something
outside of the i3c "core" need to call this function?  Have you looked
to see if you really have callers for everything you are exporting?

Other than that, the driver core interaction looks good now, nice job.

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 2/7] i3c: Add core I3C infrastructure

[Randy Dunlap]

On 12/18/2017 12:37 AM, Boris Brezillon wrote:
> On Sun, 17 Dec 2017 14:32:04 -0800
> Randy Dunlap  wrote:
> 
>> On 12/14/17 07:16, Boris Brezillon wrote:
>>> Add core infrastructure to support I3C in Linux and document it.
>>>
>>> Signed-off-by: Boris Brezillon 
>>> ---
>>>  drivers/Kconfig |2 +
>>>  drivers/Makefile|2 +-
>>>  drivers/i3c/Kconfig |   24 +
>>>  drivers/i3c/Makefile|4 +
>>>  drivers/i3c/core.c  |  573 
>>>  drivers/i3c/device.c|  344 ++
>>>  drivers/i3c/internals.h |   34 +
>>>  drivers/i3c/master.c| 1433 
>>> +++
>>>  drivers/i3c/master/Kconfig  |0
>>>  drivers/i3c/master/Makefile |0
>>>  include/linux/i3c/ccc.h |  380 +++
>>>  include/linux/i3c/device.h  |  321 +
>>>  include/linux/i3c/master.h  |  564 +++
>>>  include/linux/mod_devicetable.h |   17 +
>>>  14 files changed, 3697 insertions(+), 1 deletion(-)
>>>  create mode 100644 drivers/i3c/Kconfig
>>>  create mode 100644 drivers/i3c/Makefile
>>>  create mode 100644 drivers/i3c/core.c
>>>  create mode 100644 drivers/i3c/device.c
>>>  create mode 100644 drivers/i3c/internals.h
>>>  create mode 100644 drivers/i3c/master.c
>>>  create mode 100644 drivers/i3c/master/Kconfig
>>>  create mode 100644 drivers/i3c/master/Makefile
>>>  create mode 100644 include/linux/i3c/ccc.h
>>>  create mode 100644 include/linux/i3c/device.h
>>>  create mode 100644 include/linux/i3c/master.h


>>> diff --git a/drivers/i3c/core.c b/drivers/i3c/core.c
>>> new file mode 100644
>>> index ..7eb8e84acd33
>>> --- /dev/null
>>> +++ b/drivers/i3c/core.c
>>> @@ -0,0 +1,573 @@
>>> +// SPDX-License-Identifier: GPL-2.0
>>> +/*
>>> + * Copyright (C) 2017 Cadence Design Systems Inc.
>>> + *
>>> + * Author: Boris Brezillon 
>>> + */
>>> +
>>> +#include 
>>> +#include 
>>> +#include 
>>> +#include   
>>
>> #include 
>> #include 
>> #include 
>> #include 
>> #include 
> 
> Do you have a tool to detect those missing inclusions?

Nope, I've often wanted one, but for now it's just slow reading.



>>> diff --git a/include/linux/i3c/master.h b/include/linux/i3c/master.h
>>> new file mode 100644
>>> index ..7ec9a4821bac
>>> --- /dev/null
>>> +++ b/include/linux/i3c/master.h
>>> @@ -0,0 +1,564 @@
>>> +/* SPDX-License-Identifier: GPL-2.0 */
>>> +/*
>>> + * Copyright (C) 2017 Cadence Design Systems Inc.
>>> + *
>>> + * Author: Boris Brezillon 
>>> + */
>>> +
>>> +#ifndef I3C_MASTER_H
>>> +#define I3C_MASTER_H
>>> +
>>> +#include 
>>> +#include 
>>> +#include 
>>> +#include 
>>> +
>>> +#define I3C_HOT_JOIN_ADDR  0x2
>>> +#define I3C_BROADCAST_ADDR 0x7e
>>> +#define I3C_MAX_ADDR   GENMASK(6, 0)
>>> +  
>>
>> Needs bitops.h, workqueue.h, rwsem.h
>>
>>
>> Needs 
> 
> Okay, that's really weird to directly include a header from the
> asm-generic directory, are you sure this is the right thing to do here?

Looks like it should be , which will grab the correct one.


-- 
~Randy
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 2/7] i3c: Add core I3C infrastructure

[Boris Brezillon]

On Sun, 17 Dec 2017 14:32:04 -0800
Randy Dunlap  wrote:

> On 12/14/17 07:16, Boris Brezillon wrote:
> > Add core infrastructure to support I3C in Linux and document it.
> > 
> > Signed-off-by: Boris Brezillon 
> > ---
> >  drivers/Kconfig |2 +
> >  drivers/Makefile|2 +-
> >  drivers/i3c/Kconfig |   24 +
> >  drivers/i3c/Makefile|4 +
> >  drivers/i3c/core.c  |  573 
> >  drivers/i3c/device.c|  344 ++
> >  drivers/i3c/internals.h |   34 +
> >  drivers/i3c/master.c| 1433 
> > +++
> >  drivers/i3c/master/Kconfig  |0
> >  drivers/i3c/master/Makefile |0
> >  include/linux/i3c/ccc.h |  380 +++
> >  include/linux/i3c/device.h  |  321 +
> >  include/linux/i3c/master.h  |  564 +++
> >  include/linux/mod_devicetable.h |   17 +
> >  14 files changed, 3697 insertions(+), 1 deletion(-)
> >  create mode 100644 drivers/i3c/Kconfig
> >  create mode 100644 drivers/i3c/Makefile
> >  create mode 100644 drivers/i3c/core.c
> >  create mode 100644 drivers/i3c/device.c
> >  create mode 100644 drivers/i3c/internals.h
> >  create mode 100644 drivers/i3c/master.c
> >  create mode 100644 drivers/i3c/master/Kconfig
> >  create mode 100644 drivers/i3c/master/Makefile
> >  create mode 100644 include/linux/i3c/ccc.h
> >  create mode 100644 include/linux/i3c/device.h
> >  create mode 100644 include/linux/i3c/master.h
> > 
> > diff --git a/drivers/i3c/Kconfig b/drivers/i3c/Kconfig
> > new file mode 100644
> > index ..cf3752412ae9
> > --- /dev/null
> > +++ b/drivers/i3c/Kconfig
> > @@ -0,0 +1,24 @@
> > +# SPDX-License-Identifier: GPL-2.0
> > +
> > +menuconfig I3C
> > +   tristate "I3C support"
> > +   select I2C
> > +   help
> > + I3C is a serial protocol standardized by the MIPI alliance.
> > +
> > + It's supposed to be backward compatible with I2C while providing
> > + support for high speed transfers and native interrupt support
> > + without the need for extra pins.
> > +
> > + The I3C protocol also standardizes the slave device types and is
> > + mainly design to communicate with sensors.
> > +
> > + If you want I3C support, you should say Y here and also to the
> > + specific driver for your bus adapter(s) below.
> > +
> > + This I3C support can also be built as a module.  If so, the module
> > + will be called i3c.
> > +
> > +if I3C
> > +source "drivers/i3c/master/Kconfig"
> > +endif # I3C  
> 
> > diff --git a/drivers/i3c/core.c b/drivers/i3c/core.c
> > new file mode 100644
> > index ..7eb8e84acd33
> > --- /dev/null
> > +++ b/drivers/i3c/core.c
> > @@ -0,0 +1,573 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (C) 2017 Cadence Design Systems Inc.
> > + *
> > + * Author: Boris Brezillon 
> > + */
> > +
> > +#include 
> > +#include 
> > +#include 
> > +#include   
> 
> #include 
> #include 
> #include 
> #include 
> #include 

Do you have a tool to detect those missing inclusions?

> 
> 
> > +#include "internals.h"
> > +
> > +static DEFINE_IDR(i3c_bus_idr);
> > +static DEFINE_MUTEX(i3c_core_lock);
> > +  
> 
> > +/**
> > + * i3c_bus_maintenance_lock - Release the bus lock after a maintenance  
> 
>   unlock
> 

Will fix that.

> > + *   operation
> > + * @bus: I3C bus to release the lock on
> > + *
> > + * Should be called when the bus maintenance operation is done. See
> > + * i3c_bus_maintenance_lock() for more details on what these maintenance
> > + * operations are.
> > + */
> > +void i3c_bus_maintenance_unlock(struct i3c_bus *bus)
> > +{
> > +   up_write(>lock);
> > +}
> > +EXPORT_SYMBOL_GPL(i3c_bus_maintenance_unlock);
> > +  
> 
> > +/**
> > + * i3c_bus_normaluse_lock - Release the bus lock after a normal operation  
> 
> unlock

Ditto.

> 
> > + * @bus: I3C bus to release the lock on
> > + *
> > + * Should be called when a normal operation is done. See
> > + * i3c_bus_normaluse_lock() for more details on what these normal 
> > operations
> > + * are.
> > + */
> > +void i3c_bus_normaluse_unlock(struct i3c_bus *bus)
> > +{
> > +   up_read(>lock);
> > +}
> > +EXPORT_SYMBOL_GPL(i3c_bus_normaluse_unlock);  
> 
> 
> 
> > +static int i3c_device_match(struct device *dev, struct device_driver *drv) 
> >  
> 
> bool?
> 
> > +{
> > +   struct i3c_device *i3cdev;
> > +   struct i3c_driver *i3cdrv;
> > +
> > +   if (dev->type != _device_type)
> > +   return 0;
> > +
> > +   i3cdev = dev_to_i3cdev(dev);
> > +   i3cdrv = drv_to_i3cdrv(drv);
> > +   if (i3c_device_match_id(i3cdev, i3cdrv->id_table))
> > +   return 1;
> > +
> > +   return 0;
> > +}  
> 
> 
> > diff --git a/drivers/i3c/device.c b/drivers/i3c/device.c
> > new file mode 100644
> > index 

Re: [PATCH v2 2/7] i3c: Add core I3C infrastructure

[Randy Dunlap]

On 12/14/17 07:16, Boris Brezillon wrote:
> Add core infrastructure to support I3C in Linux and document it.
> 
> Signed-off-by: Boris Brezillon 
> ---
>  drivers/Kconfig |2 +
>  drivers/Makefile|2 +-
>  drivers/i3c/Kconfig |   24 +
>  drivers/i3c/Makefile|4 +
>  drivers/i3c/core.c  |  573 
>  drivers/i3c/device.c|  344 ++
>  drivers/i3c/internals.h |   34 +
>  drivers/i3c/master.c| 1433 
> +++
>  drivers/i3c/master/Kconfig  |0
>  drivers/i3c/master/Makefile |0
>  include/linux/i3c/ccc.h |  380 +++
>  include/linux/i3c/device.h  |  321 +
>  include/linux/i3c/master.h  |  564 +++
>  include/linux/mod_devicetable.h |   17 +
>  14 files changed, 3697 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/i3c/Kconfig
>  create mode 100644 drivers/i3c/Makefile
>  create mode 100644 drivers/i3c/core.c
>  create mode 100644 drivers/i3c/device.c
>  create mode 100644 drivers/i3c/internals.h
>  create mode 100644 drivers/i3c/master.c
>  create mode 100644 drivers/i3c/master/Kconfig
>  create mode 100644 drivers/i3c/master/Makefile
>  create mode 100644 include/linux/i3c/ccc.h
>  create mode 100644 include/linux/i3c/device.h
>  create mode 100644 include/linux/i3c/master.h
> 
> diff --git a/drivers/i3c/Kconfig b/drivers/i3c/Kconfig
> new file mode 100644
> index ..cf3752412ae9
> --- /dev/null
> +++ b/drivers/i3c/Kconfig
> @@ -0,0 +1,24 @@
> +# SPDX-License-Identifier: GPL-2.0
> +
> +menuconfig I3C
> + tristate "I3C support"
> + select I2C
> + help
> +   I3C is a serial protocol standardized by the MIPI alliance.
> +
> +   It's supposed to be backward compatible with I2C while providing
> +   support for high speed transfers and native interrupt support
> +   without the need for extra pins.
> +
> +   The I3C protocol also standardizes the slave device types and is
> +   mainly design to communicate with sensors.
> +
> +   If you want I3C support, you should say Y here and also to the
> +   specific driver for your bus adapter(s) below.
> +
> +   This I3C support can also be built as a module.  If so, the module
> +   will be called i3c.
> +
> +if I3C
> +source "drivers/i3c/master/Kconfig"
> +endif # I3C

> diff --git a/drivers/i3c/core.c b/drivers/i3c/core.c
> new file mode 100644
> index ..7eb8e84acd33
> --- /dev/null
> +++ b/drivers/i3c/core.c
> @@ -0,0 +1,573 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2017 Cadence Design Systems Inc.
> + *
> + * Author: Boris Brezillon 
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 

#include 
#include 
#include 
#include 
#include 


> +#include "internals.h"
> +
> +static DEFINE_IDR(i3c_bus_idr);
> +static DEFINE_MUTEX(i3c_core_lock);
> +

> +/**
> + * i3c_bus_maintenance_lock - Release the bus lock after a maintenance

  unlock

> + * operation
> + * @bus: I3C bus to release the lock on
> + *
> + * Should be called when the bus maintenance operation is done. See
> + * i3c_bus_maintenance_lock() for more details on what these maintenance
> + * operations are.
> + */
> +void i3c_bus_maintenance_unlock(struct i3c_bus *bus)
> +{
> + up_write(>lock);
> +}
> +EXPORT_SYMBOL_GPL(i3c_bus_maintenance_unlock);
> +

> +/**
> + * i3c_bus_normaluse_lock - Release the bus lock after a normal operation

unlock

> + * @bus: I3C bus to release the lock on
> + *
> + * Should be called when a normal operation is done. See
> + * i3c_bus_normaluse_lock() for more details on what these normal operations
> + * are.
> + */
> +void i3c_bus_normaluse_unlock(struct i3c_bus *bus)
> +{
> + up_read(>lock);
> +}
> +EXPORT_SYMBOL_GPL(i3c_bus_normaluse_unlock);



> +static int i3c_device_match(struct device *dev, struct device_driver *drv)

bool?

> +{
> + struct i3c_device *i3cdev;
> + struct i3c_driver *i3cdrv;
> +
> + if (dev->type != _device_type)
> + return 0;
> +
> + i3cdev = dev_to_i3cdev(dev);
> + i3cdrv = drv_to_i3cdrv(drv);
> + if (i3c_device_match_id(i3cdev, i3cdrv->id_table))
> + return 1;
> +
> + return 0;
> +}


> diff --git a/drivers/i3c/device.c b/drivers/i3c/device.c
> new file mode 100644
> index ..dcf51150b7cb
> --- /dev/null
> +++ b/drivers/i3c/device.c
> @@ -0,0 +1,344 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2017 Cadence Design Systems Inc.
> + *
> + * Author: Boris Brezillon 
> + */
> +
> +#include 

#include 
#include 
#include 
#include 
#include 

> +#include "internals.h"



> diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
> new file mode 100644
>