Re: [PATCH] i2c/busses: (mpc) Add support for SMBUS_READ_BLOCK_DATA
I am not sure if this mail will reach the list as I am not subscribed.
In the v2 patch (actually both version), you wrote
> + byte = readb(i2c->base + MPC_I2C_DR);
> +
> + /* Adjust length if first received byte is length */
> + if (i == 0 && recv_len) {
> + if (byte == 0 || byte > I2C_SMBUS_BLOCK_MAX)
> + return -EPROTO;
> + length += byte;
> + /*
> + * For block reads, generate txack here if data length
> + * is 1 byte (total length is 2 bytes).
> + */
> + if (length == 2)
> + writeccr(i2c, CCR_MIEN | CCR_MEN | CCR_MSTA
> + | CCR_TXAK);
> + }
> + data[i] = byte;
I am wondering if the first byte should be left out for data[] if the
recv_len is non-zero. If I understand correctly, this patch is to
support of SMBus with multiple byte read. The SMBus is different from
I2C bus on multiple byte. The first data is byte count vs slave data for
I2C. So you will receive all data preceded by the byte count, which is
one more than your loop.
York
On Wed, 2011-11-16 at 08:56 -0600, Timur Tabi wrote:
>
> On Nov 16, 2011, at 12:01 AM, sun york-R58495 wrote:
>
> > Timur,
> >
> > Which silicon has the i2c-mpc? Without the user's manual, I can only guess
> > from the code. It looks like the code is going to deal with a block
> > transaction with the first byte is the length. If that's true, there might
> > be a problem with
> >
> > data[i]=byte
> >
> > if the block is true. Since the first byte is the length, it shouldn't be
> > the data at the same time, right?
>
> Can you check thr mailing list thread? It should explain everything.
>
>
> >
> > York
> >
> >
> > From: Tabi Timur-B04825
> > Sent: Tuesday, November 15, 2011 11:04 AM
> > To: sun york-R58495
> > Subject: Fwd: [PATCH] i2c/busses: (mpc) Add support for
> > SMBUS_READ_BLOCK_DATA
> >
> > York,
> >
> > If you have a moment, can you take a look at this patch? I don't know
> > enough about I2C to really understand it.
> >
> > -- Forwarded message --
> > From: Guenter Roeck
> > Date: Tue, Nov 15, 2011 at 12:27 AM
> > Subject: [PATCH] i2c/busses: (mpc) Add support for SMBUS_READ_BLOCK_DATA
> > To: Jean Delvare , Ben Dooks
> > Cc: Grant Likely ,
> > linux-i2c@vger.kernel.org, linux-ker...@vger.kernel.org, Guenter Roeck
> > , Tang Yuantian
> >
> >
> > Add support for SMBUS_READ_BLOCK_DATA to the i2c-mpc bus driver.
> > Required to support the PMBus zl6100 driver with i2c-mpc.
> >
> > Reported-by: Tang Yuantian
> > Cc: Tang Yuantian
> > Signed-off-by: Guenter Roeck
> > ---
> > drivers/i2c/busses/i2c-mpc.c | 33 +
> > 1 files changed, 25 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/i2c/busses/i2c-mpc.c b/drivers/i2c/busses/i2c-mpc.c
> > index 107397a..77aade7 100644
> > --- a/drivers/i2c/busses/i2c-mpc.c
> > +++ b/drivers/i2c/busses/i2c-mpc.c
> > @@ -454,7 +454,7 @@ static int mpc_write(struct mpc_i2c *i2c, int target,
> > }
> >
> > static int mpc_read(struct mpc_i2c *i2c, int target,
> > - u8 *data, int length, int restart)
> > + u8 *data, int length, int restart, bool block)
> > {
> > unsigned timeout = i2c->adap.timeout;
> > int i, result;
> > @@ -470,7 +470,7 @@ static int mpc_read(struct mpc_i2c *i2c, int target,
> > return result;
> >
> > if (length) {
> > - if (length == 1)
> > + if (length == 1 && !block)
> > writeccr(i2c, CCR_MIEN | CCR_MEN | CCR_MSTA |
> > CCR_TXAK);
> > else
> > writeccr(i2c, CCR_MIEN | CCR_MEN | CCR_MSTA);
> > @@ -479,17 +479,28 @@ static int mpc_read(struct mpc_i2c *i2c, int target,
> > }
> >
> > for (i = 0; i < length; i++) {
> > + u8 byte;
> > +
> > result = i2c_wait(i2c, timeout, 0);
> > if (result < 0)
> > return result;
> >
> > + byte = readb(i2c->base + MPC_I2C_DR);
> > + /*
> > +* Adjust length if first received byte is length
> > +*/
> > + if (i == 0 && block) {
> > + if (byte == 0 || byte > I2C_SMBUS_BLOCK_MAX)
> > + return -EPROTO;
> > + length += byte;
> > + }
> > + data[i] = byte;
> > /* Generate txack on next to last byte */
> > if (i == length - 2)
> > writeccr(i2c, CCR_MIEN | CCR_MEN | CCR_MSTA |
> > CCR_TXAK);
> > /* Do not generate stop on last byte */
> > if (i == length - 1)
> > writeccr(i2c
Re: [PATCH] i2c/busses: (mpc) Add support for SMBUS_READ_BLOCK_DATA
On Wed, 2011-11-16 at 09:36 -0800, Guenter Roeck wrote:
> On Wed, 2011-11-16 at 12:28 -0500, York Sun wrote:
> > I am not sure if this mail will reach the list as I am not subscribed.
> >
> > In the v2 patch (actually both version), you wrote
> >
> > > + byte = readb(i2c->base + MPC_I2C_DR);
> > > +
> > > + /* Adjust length if first received byte is length */
> > > + if (i == 0 && recv_len) {
> > > + if (byte == 0 || byte > I2C_SMBUS_BLOCK_MAX)
> > > + return -EPROTO;
> > > + length += byte;
> > > + /*
> > > + * For block reads, generate txack here if data length
> > > + * is 1 byte (total length is 2 bytes).
> > > + */
> > > + if (length == 2)
> > > + writeccr(i2c, CCR_MIEN | CCR_MEN | CCR_MSTA
> > > + | CCR_TXAK);
> > > + }
> > > + data[i] = byte;
> >
> > I am wondering if the first byte should be left out for data[] if the
> > recv_len is non-zero. If I understand correctly, this patch is to
> > support of SMBus with multiple byte read. The SMBus is different from
> > I2C bus on multiple byte. The first data is byte count vs slave data for
> > I2C. So you will receive all data preceded by the byte count, which is
> > one more than your loop.
> >
> York,
>
> The calling code expects the data length in data[0], and the actual data
> in data[1] .. data[]. The initial value for length is 1; the
> byte count is added to it, so bytes are placed into the
> buffer.
>
> Thanks,
> Guenter
Thanks for explanation. I am more confused by the length += byte now.
For I2C bus, if you need length of byte, just keep reading until you get
all of them. Of course you need to deal with the ACK. For SMBus, it is
similar but you shouldn't read more after the byte count which is in the
first data. If you want to read length of data but the block size is
bigger than length, you should call block read at first place. If the
block size is smaller than length, why increase the length? Does your
SMBus controller only support fixed block size and not support single
byte read? If it does, I would do
Block, Block, Block, byte, byte... until length of data
Regards,
York
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] i2c/busses: (mpc) Add support for SMBUS_READ_BLOCK_DATA
On Wed, 2011-11-16 at 19:09 +0100, Jean Delvare wrote:
> On Wed, 16 Nov 2011 09:55:35 -0800, York Sun wrote:
> > On Wed, 2011-11-16 at 09:36 -0800, Guenter Roeck wrote:
> > > York,
> > >
> > > The calling code expects the data length in data[0], and the actual data
> > > in data[1] .. data[]. The initial value for length is 1; the
> > > byte count is added to it, so bytes are placed into the
> > > buffer.
> > >
> > > Thanks,
> > > Guenter
> >
> > Thanks for explanation. I am more confused by the length += byte now.
> > For I2C bus, if you need length of byte, just keep reading until you get
> > all of them. Of course you need to deal with the ACK. For SMBus, it is
> > similar but you shouldn't read more after the byte count which is in the
> > first data.
>
> You shouldn't read less either. The slave tells how much bytes it wants
> to send, and the master must honor that.
>
> > If you want to read length of data but the block size is
> > bigger than length, you should call block read at first place. If the
> > block size is smaller than length, why increase the length? Does your
> > SMBus controller only support fixed block size and not support single
> > byte read? If it does, I would do
> >
> > Block, Block, Block, byte, byte... until length of data
>
> Your thinking is too focused on I2C block reads (or even block read of
> data over the network or on disk). SMBus block read is something
> completely different. It's not about reading 200 bytes of data and
> receiving it in 16-byte chunks (I2C block read works that way, on
> EEPROMs in particular.) There is no "data length" and "block size" to
> compare to each other. It's about reading the value of _one_ register
> and this value happens to be multi-byte. There is typically _no_
> register pointer increment (automatic or not) involved as can happen
> with EEPROMs. If an SMBus block read from register N returns 10 bytes,
> you're not going to read the next 10 bytes from register N+10. There
> are no "next 10 bytes" to read, and register N+10 is something
> completely unrelated.
>
> And for this reason, it is not possible to mix SMBus block reads with
> byte reads, as can be done with I2C block reads.
>
> Also note that there is a limit of 32 bytes for SMBus block transfers,
> per SMBus specification. All slaves and masters must comply with it.
>
> I hope I managed to clarify the case this time...
>
You have made it much clear. If block size is fixed and block read
cannot mix with byte read, shall we do this
if length < block_size
read block_size
else {
while (length) {
read block_size
length -= block_size
}
York
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] i2c/busses: (mpc) Add support for SMBUS_READ_BLOCK_DATA
On Wed, 2011-11-16 at 20:10 +0100, Jean Delvare wrote:
> On Wed, 16 Nov 2011 10:20:38 -0800, York Sun wrote:
> > On Wed, 2011-11-16 at 19:09 +0100, Jean Delvare wrote:
> > > Your thinking is too focused on I2C block reads (or even block read of
> > > data over the network or on disk). SMBus block read is something
> > > completely different. It's not about reading 200 bytes of data and
> > > receiving it in 16-byte chunks (I2C block read works that way, on
> > > EEPROMs in particular.) There is no "data length" and "block size" to
> > > compare to each other. It's about reading the value of _one_ register
> > > and this value happens to be multi-byte. There is typically _no_
> > > register pointer increment (automatic or not) involved as can happen
> > > with EEPROMs. If an SMBus block read from register N returns 10 bytes,
> > > you're not going to read the next 10 bytes from register N+10. There
> > > are no "next 10 bytes" to read, and register N+10 is something
> > > completely unrelated.
> > >
> > > And for this reason, it is not possible to mix SMBus block reads with
> > > byte reads, as can be done with I2C block reads.
> > >
> > > Also note that there is a limit of 32 bytes for SMBus block transfers,
> > > per SMBus specification. All slaves and masters must comply with it.
> > >
> > > I hope I managed to clarify the case this time...
> >
> > You have made it much clear. If block size is fixed and block read
> > cannot mix with byte read, shall we do this
> >
> > if length < block_size
> >read block_size
> > else {
> >while (length) {
> >read block_size
> >length -= block_size
> >}
>
> Which part of
>
> There is no "data length" and "block size" to compare to each other.
>
> did you not understand?
For example, if the length is 40 and the block size is 32, are you going
to read 32, 72 byte or 64 byte?
York
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] i2c/busses: (mpc) Add support for SMBUS_READ_BLOCK_DATA
On Wed, 2011-11-16 at 20:18 +0100, Jean Delvare wrote:
> On Wed, 16 Nov 2011 11:15:15 -0800, York Sun wrote:
> > On Wed, 2011-11-16 at 20:10 +0100, Jean Delvare wrote:
> > > On Wed, 16 Nov 2011 10:20:38 -0800, York Sun wrote:
> > > > On Wed, 2011-11-16 at 19:09 +0100, Jean Delvare wrote:
> > > > > Your thinking is too focused on I2C block reads (or even block read of
> > > > > data over the network or on disk). SMBus block read is something
> > > > > completely different. It's not about reading 200 bytes of data and
> > > > > receiving it in 16-byte chunks (I2C block read works that way, on
> > > > > EEPROMs in particular.) There is no "data length" and "block size" to
> > > > > compare to each other. It's about reading the value of _one_ register
> > > > > and this value happens to be multi-byte. There is typically _no_
> > > > > register pointer increment (automatic or not) involved as can happen
> > > > > with EEPROMs. If an SMBus block read from register N returns 10 bytes,
> > > > > you're not going to read the next 10 bytes from register N+10. There
> > > > > are no "next 10 bytes" to read, and register N+10 is something
> > > > > completely unrelated.
> > > > >
> > > > > And for this reason, it is not possible to mix SMBus block reads with
> > > > > byte reads, as can be done with I2C block reads.
> > > > >
> > > > > Also note that there is a limit of 32 bytes for SMBus block transfers,
> > > > > per SMBus specification. All slaves and masters must comply with it.
> > > > >
> > > > > I hope I managed to clarify the case this time...
> > > >
> > > > You have made it much clear. If block size is fixed and block read
> > > > cannot mix with byte read, shall we do this
> > > >
> > > > if length < block_size
> > > >read block_size
> > > > else {
> > > >while (length) {
> > > >read block_size
> > > >length -= block_size
> > > >}
> > >
> > > Which part of
> > >
> > > There is no "data length" and "block size" to compare to each other.
> > >
> > > did you not understand?
> >
> > For example, if the length is 40 and the block size is 32, are you going
> > to read 32, 72 byte or 64 byte?
>
> The length is not 40. Which part of
>
> Also note that there is a limit of 32 bytes for SMBus block transfers,
> per SMBus specification. All slaves and masters must comply with it.
>
> did you now understand?
>
> Really, please. I understand you're only trying to help and understand
> the patch, but at some point, if you have zero knowledge about the
> topic, you're not the right person to review the patch, period. You are
> still welcome to test the patch if you can and want, that's a
> completely different task.
>
That's the point. Your patch doesn't check if the length is valid. You
rely on the caller to know no more than 32 bytes can be transferred. It
shouldn't limit the subfunction to transfer more than 32 bytes if
hardware can support it by multiple transactions. If not, print out an
error message.
York
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] i2c/busses: (mpc) Add support for SMBUS_READ_BLOCK_DATA
> > That's the point. Your patch doesn't check if the length is valid. You > > rely on the caller to know no more than 32 bytes can be transferred. It > > shouldn't limit the subfunction to transfer more than 32 bytes if > > hardware can support it by multiple transactions. If not, print out an > > error message. > > > > It is customary for kernel functions to only validate parameters > wherever a value enters or leaves the kernel, or at least a logical > boundary. Anything else would blow up the kernel size to a point where > it would be unusable. The patch checks if the block length received from > the SMBus slave is correct, which is exactly what it is supposed to do. > > If you look closely, you may realize that the mpc_read also doesn't > validate of any of the other parameters. Are you going to request that > such validations be added as well ? How about the other functions in > this driver ? Should each function also validate each of its > parameters ? If not, where do you draw the line ? > > Besides, the caller is the SMBus block read function, which does know > that SMBus block reads are limited to 32 bytes (plus length). > I am going to let it go for now. Obviously your patch is working when length=1. Probably it will never be called with length other than 1 for SMBus block read. It would be nicer if you could put a comment there just in case in the future someone runs into a case where length+=byte causes a problem. York -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 4/7] power/mpc85xx: Add delay after enabling I2C master
Erratum A-006037 indicates I2C controller executes the write to I2CCR only
after it sees SCL idle for 64K cycle of internal I2C controller clocks. If
during this waiting period, I2C controller is disabled (I2CCR[MEN] set to
0), then the controller could end in bad state, and hang the future access
to I2C register.
The mpc_i2c_fixup() function tries to recover the bus from a stalled state
where the 9th clock pulse wasn't generated. However, this workaround
disables and enables I2C controller without meeting waiting requirement of
this erratum.
This erratum applies to some 85xx SoCs. It is safe to apply to all of them
for mpc_i2c_fixup().
Signed-off-by: York Sun
Reviewed-by: Wood Scott-B07421
Reviewed-by: Fleming Andrew-AFLEMING
Tested-by: Fleming Andrew-AFLEMING
CC: linux-i2c@vger.kernel.org
---
drivers/i2c/busses/i2c-mpc.c | 11 ++-
1 file changed, 10 insertions(+), 1 deletion(-)
diff --git a/drivers/i2c/busses/i2c-mpc.c b/drivers/i2c/busses/i2c-mpc.c
index b80c768..55dce43 100644
--- a/drivers/i2c/busses/i2c-mpc.c
+++ b/drivers/i2c/busses/i2c-mpc.c
@@ -106,7 +106,12 @@ static irqreturn_t mpc_i2c_isr(int irq, void *dev_id)
static void mpc_i2c_fixup(struct mpc_i2c *i2c)
{
int k;
- u32 delay_val = 100 / i2c->real_clk + 1;
+ u32 delay_val;
+#ifdef CONFIG_PPC_85xx
+ delay_val = 65536 / (fsl_get_sys_freq() / 200); /* 64K cycle */
+#else
+ delay_val = 100 / i2c->real_clk + 1;
+#endif
if (delay_val < 2)
delay_val = 2;
@@ -116,7 +121,11 @@ static void mpc_i2c_fixup(struct mpc_i2c *i2c)
writeccr(i2c, CCR_MSTA | CCR_MTX | CCR_MEN);
udelay(delay_val);
writeccr(i2c, CCR_MEN);
+#ifdef CONFIG_PPC_85xx
+ udelay(delay_val);
+#else
udelay(delay_val << 1);
+#endif
}
}
--
1.7.9.5
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[Patch v2] power/mpc85xx: Add delay after enabling I2C master
Erratum A-006037 indicates I2C controller executes the write to I2CCR only
after it sees SCL idle for 64K cycle of internal I2C controller clocks. If
during this waiting period, I2C controller is disabled (I2CCR[MEN] set to
0), then the controller could end in bad state, and hang the future access
to I2C register.
The mpc_i2c_fixup() function tries to recover the bus from a stalled state
where the 9th clock pulse wasn't generated. However, this workaround
disables and enables I2C controller without meeting waiting requirement of
this erratum.
This erratum applies to some 85xx SoCs. It is safe to apply to all of them
for mpc_i2c_fixup().
Signed-off-by: York Sun
---
Change log:
v2: remote reviewed-by and tested-by lines added by gerrit
send to proper mailing list
drivers/i2c/busses/i2c-mpc.c | 11 ++-
1 file changed, 10 insertions(+), 1 deletion(-)
diff --git a/drivers/i2c/busses/i2c-mpc.c b/drivers/i2c/busses/i2c-mpc.c
index b80c768..55dce43 100644
--- a/drivers/i2c/busses/i2c-mpc.c
+++ b/drivers/i2c/busses/i2c-mpc.c
@@ -106,7 +106,12 @@ static irqreturn_t mpc_i2c_isr(int irq, void *dev_id)
static void mpc_i2c_fixup(struct mpc_i2c *i2c)
{
int k;
- u32 delay_val = 100 / i2c->real_clk + 1;
+ u32 delay_val;
+#ifdef CONFIG_PPC_85xx
+ delay_val = 65536 / (fsl_get_sys_freq() / 200); /* 64K cycle */
+#else
+ delay_val = 100 / i2c->real_clk + 1;
+#endif
if (delay_val < 2)
delay_val = 2;
@@ -116,7 +121,11 @@ static void mpc_i2c_fixup(struct mpc_i2c *i2c)
writeccr(i2c, CCR_MSTA | CCR_MTX | CCR_MEN);
udelay(delay_val);
writeccr(i2c, CCR_MEN);
+#ifdef CONFIG_PPC_85xx
+ udelay(delay_val);
+#else
udelay(delay_val << 1);
+#endif
}
}
--
1.7.9.5
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [linuxppc-release] [Patch v2] power/mpc85xx: Add delay after enabling I2C master
On 09/16/2013 03:06 PM, York Sun wrote: > Erratum A-006037 indicates I2C controller executes the write to I2CCR only > after it sees SCL idle for 64K cycle of internal I2C controller clocks. If > during this waiting period, I2C controller is disabled (I2CCR[MEN] set to > 0), then the controller could end in bad state, and hang the future access > to I2C register. > > The mpc_i2c_fixup() function tries to recover the bus from a stalled state > where the 9th clock pulse wasn't generated. However, this workaround > disables and enables I2C controller without meeting waiting requirement of > this erratum. > > This erratum applies to some 85xx SoCs. It is safe to apply to all of them > for mpc_i2c_fixup(). > > Signed-off-by: York Sun > --- > Change log: > v2: remote reviewed-by and tested-by lines added by gerrit > send to proper mailing list Pardon my typo. I meant to type "remove", instead of "remote". York -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" 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] power/mpc85xx: Add delay after enabling I2C master
On 09/23/2013 12:10 AM, Wolfram Sang wrote:
>
>> diff --git a/drivers/i2c/busses/i2c-mpc.c b/drivers/i2c/busses/i2c-mpc.c
>> index b80c768..55dce43 100644
>> --- a/drivers/i2c/busses/i2c-mpc.c
>> +++ b/drivers/i2c/busses/i2c-mpc.c
>> @@ -106,7 +106,12 @@ static irqreturn_t mpc_i2c_isr(int irq, void *dev_id)
>> static void mpc_i2c_fixup(struct mpc_i2c *i2c)
>> {
>> int k;
>> -u32 delay_val = 100 / i2c->real_clk + 1;
>> +u32 delay_val;
>> +#ifdef CONFIG_PPC_85xx
>> +delay_val = 65536 / (fsl_get_sys_freq() / 200); /* 64K cycle */
>> +#else
>> +delay_val = 100 / i2c->real_clk + 1;
>> +#endif
>
> Please, no unnecessary #ifdefs in code. We have 'struct mpc_i2c_data'
> already.
>
I am not pround of this change. Please elaborate how to use mpc_i2c_data
to separate the mpc85xx from the rest.
York
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" 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] power/mpc85xx: Add delay after enabling I2C master
On 09/23/2013 03:09 PM, Wolfram Sang wrote: > >>> Please, no unnecessary #ifdefs in code. We have 'struct mpc_i2c_data' >>> already. >>> >> >> I am not pround of this change. Please elaborate how to use mpc_i2c_data >> to separate the mpc85xx from the rest. > > Search for 'match->data' and see how the custom setup function and > custom prescaler value is used. > I see. It is using device tree to match. But the device tree doesn't have enough information. Many platforms don't specifiy if compatible with mpc85xx, so they fall into .compatible = "fsl-i2c". Please advise. York -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" 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] power/mpc85xx: Add delay after enabling I2C master
On 09/23/2013 03:32 PM, Wolfram Sang wrote: > >> I see. It is using device tree to match. But the device tree doesn't >> have enough information. Many platforms don't specifiy if compatible >> with mpc85xx, so they fall into .compatible = "fsl-i2c". > > :( Can those be fixed to use proper "fsl,mpc8544-i2c"? > >> Please advise. > > My PPC knowledge is a bit dusty: What about SVR/PVR to detect the 8544 > at runtime? > Wolfram, It is not only about 8544. I wrote this patch to address a timing issue when a workaround is applied. This timing issue applies to many mpc85xx parts, but not all of them. But it is safe to all. So checking one SVR/PVR doesn't work. York -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Need some guidance on i2c-ocores driver
Julia and other experts, I am seeking for help on my device driver. I am working on a module driver for a FPGA design with open core I2C controller memory-mapped to BAR2. I have searched up and down and found only one driver (drivers/mfd/timberdale.c) with similar implementation. Following timberdale driver, I am able to load the driver, but blocked by devm_ioremap_resource(). It is always in conflict with my BAR2. I wonder if I did something wrong. Here is the flow I tracked so far. (By the way, I am using an older kernel 3.12. The new 4.0 kernel crashes when booting on my platform. I plan to move to newer kernel later). mfd_add_devices() | |--mfd_add_device() | |--platform_device_add() | |--insert_resource() /* this passed */ | | | |--insert_resource_conflict() | |--device_add() | |--bus_probe_device() | |--devm_ioremap_resource() | |--devm_request_mem_region() | |--__request_region() /* this always conflicts */ | |--__request_resource() My driver is called RedStone DMA. Here is my debug output > root@p1022ds:~# insmod redstone_mfd.ko > RedStone DMA 0002:05:00.0: pci_enable_device() successful > RedStone DMA 0002:05:00.0: MSI-X init successful > York kernel: Calling devm_ioremap_resource() > York kernel kernel/resource.c __request_region 977: conflict=[??? > 0xc0008-0xc00087fff flags 0x8000] > ocores-i2c ocores-i2c: can't request region for resource [mem > 0xc00086000-0xc0008601f] > ocores-i2c: probe of ocores-i2c failed with error -16 > RedStone DMA 0002:05:00.0: BAR[0] 0x000c-0x000c0007 flags > 0x0014220c, length 524288 > RedStone DMA 0002:05:00.0: BAR[1] 0x-0x flags > 0x, length 0 > RedStone DMA 0002:05:00.0: BAR[2] 0x000c0008-0x000c00087fff flags > 0x00040200, length 32768 > RedStone DMA 0002:05:00.0: BAR[3] 0x-0x flags > 0x, length 0 > RedStone DMA 0002:05:00.0: BAR[4] 0x-0x flags > 0x, length 0 > RedStone DMA 0002:05:00.0: BAR[5] 0x-0x flags > 0x, length 0 > RedStone DMA 0002:05:00.0: Version 1.4, built on 4-16-15, id 0 > root@p1022ds:~# Can you shed some light on this? Thanks. York -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Need some guidance on i2c-ocores driver
Resend to LKML Lee, This question is actually more about MFD. Can you point me to the possible causes for my failure below? Thanks. York On 04/16/2015 05:02 PM, York Sun wrote: > Julia and other experts, > > I am seeking for help on my device driver. > > I am working on a module driver for a FPGA design with open core I2C > controller > memory-mapped to BAR2. I have searched up and down and found only one driver > (drivers/mfd/timberdale.c) with similar implementation. Following timberdale > driver, I am able to load the driver, but blocked by devm_ioremap_resource(). > It > is always in conflict with my BAR2. I wonder if I did something wrong. Here is > the flow I tracked so far. (By the way, I am using an older kernel 3.12. The > new > 4.0 kernel crashes when booting on my platform. I plan to move to newer kernel > later). > > mfd_add_devices() > | > |--mfd_add_device() > | > |--platform_device_add() > | > |--insert_resource() /* this passed */ > | | > | |--insert_resource_conflict() > | > |--device_add() > | > |--bus_probe_device() > | > |--devm_ioremap_resource() > | > |--devm_request_mem_region() > | > |--__request_region() /* this always conflicts */ > | > |--__request_resource() > > My driver is called RedStone DMA. Here is my debug output > >> root@p1022ds:~# insmod redstone_mfd.ko >> RedStone DMA 0002:05:00.0: pci_enable_device() successful >> RedStone DMA 0002:05:00.0: MSI-X init successful >> York kernel: Calling devm_ioremap_resource() >> York kernel kernel/resource.c __request_region 977: conflict=[??? >> 0xc0008-0xc00087fff flags 0x8000] >> ocores-i2c ocores-i2c: can't request region for resource [mem >> 0xc00086000-0xc0008601f] >> ocores-i2c: probe of ocores-i2c failed with error -16 >> RedStone DMA 0002:05:00.0: BAR[0] 0x000c-0x000c0007 >> flags 0x0014220c, length 524288 >> RedStone DMA 0002:05:00.0: BAR[1] 0x-0x >> flags 0x, length 0 >> RedStone DMA 0002:05:00.0: BAR[2] 0x000c0008-0x000c00087fff >> flags 0x00040200, length 32768 >> RedStone DMA 0002:05:00.0: BAR[3] 0x-0x >> flags 0x, length 0 >> RedStone DMA 0002:05:00.0: BAR[4] 0x-0x >> flags 0x, length 0 >> RedStone DMA 0002:05:00.0: BAR[5] 0x-0x >> flags 0x, length 0 >> RedStone DMA 0002:05:00.0: Version 1.4, built on 4-16-15, id 0 >> root@p1022ds:~# > > Can you shed some light on this? > > Thanks. > > York > -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Need some guidance on i2c-ocores driver
On 04/19/2015 11:42 PM, Lee Jones wrote:
> On Fri, 17 Apr 2015, York Sun wrote:
>
>> Resend to LKML
>>
>> Lee,
>>
>> This question is actually more about MFD. Can you point me to the possible
>> causes for my failure below?
>
> It's hard to tell exactly without code, but it looks like you're
> trying to allocate overlapping memory regions. Double check all of
> your addresses. For DT you need to take a look at your 'reg'
> properties, for traditional platform data it's best to grep for
> IORESOURCE_MEM.
>
Lee,
It _is_ overlapping. How could it not be? The resource for the I2C is mapped to
BAR2. So the resource is overlapping with BAR2. It is alway the case, isn't it?
What I don't understand is how MFD works with the resources if it is guaranteed
overlapping. Did I get something wrong?
Look at the reference code I took, drivers/mfd/timberdale.c, when
mfd_add_devices() is called, it uses &dev->resource as the base. So the BAR will
be the parent. Check the code in mfd-core.c, mfd_add_device(),
if ((cell->resources[r].flags & IORESOURCE_MEM) && mem_base) {
res[r].parent = mem_base;
res[r].start = mem_base->start + cell->resources[r].start;
res[r].end = mem_base->start + cell->resources[r].end;
}
So the MFD resource is within its parent. When later the device driver request a
region, will it get conflict with the parent?
York
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Need some guidance on i2c-ocores driver
On 04/20/2015 11:16 AM, Lee Jones wrote:
> On Mon, 20 Apr 2015, York Sun wrote:
>
>>
>>
>> On 04/19/2015 11:42 PM, Lee Jones wrote:
>>> On Fri, 17 Apr 2015, York Sun wrote:
>>>
>>>> Resend to LKML
>>>>
>>>> Lee,
>>>>
>>>> This question is actually more about MFD. Can you point me to the possible
>>>> causes for my failure below?
>>>
>>> It's hard to tell exactly without code, but it looks like you're
>>> trying to allocate overlapping memory regions. Double check all of
>>> your addresses. For DT you need to take a look at your 'reg'
>>> properties, for traditional platform data it's best to grep for
>>> IORESOURCE_MEM.
>>>
>> Lee,
>>
>> It _is_ overlapping. How could it not be? The resource for the I2C is mapped
>> to
>> BAR2. So the resource is overlapping with BAR2. It is alway the case, isn't
>> it?
>> What I don't understand is how MFD works with the resources if it is
>> guaranteed
>> overlapping. Did I get something wrong?
>>
>> Look at the reference code I took, drivers/mfd/timberdale.c, when
>> mfd_add_devices() is called, it uses &dev->resource as the base. So the BAR
>> will
>> be the parent. Check the code in mfd-core.c, mfd_add_device(),
>>
>> if ((cell->resources[r].flags & IORESOURCE_MEM) && mem_base) {
>> res[r].parent = mem_base;
>> res[r].start = mem_base->start + cell->resources[r].start;
>> res[r].end = mem_base->start + cell->resources[r].end;
>> }
>>
>> So the MFD resource is within its parent. When later the device driver
>> request a
>> region, will it get conflict with the parent?
>
> I doubt you'll want to map the same memory area in both the parent and
> the child device drivers. Only map the registers you plan to use in
> the driver you plan to use them. If you need multiple devices to
> access the same registers then you need to create an API, complete
> with locking, in the MFD parent device.
>
Thanks for pointing out. I thought at first the conflict was due to my
pci_ioremap_bar(). I went ahead to remove the mapping but still not working.
Your email inspired me to take a deeper look at my code and I found my error. I
have called pci_request_regions() which reserves all BARs. I think that's my
root cause. Thanks for helping me.
York
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
I2C class bitmask
Lee, Is there any convention regarding I2C class bitmask? I see only three are defined for 3.12.19 and four for 4.0 I2C_CLASS_HWMON, I2C_CLASS_DDC, I2C_CLASS_SPD, I2C_CLASS_DEPRECATED I am working on a clock chip driver (SI5338) and trying to detect them (multiple chips in i2c mux). It would be a lot easier to have its own class, like I2C_CLASS_CLOCK. It is trivial to add a line to i2c.h file. Just checking if this is a bad idea. York -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: I2C class bitmask
On 05/23/2015 09:27 AM, Guenter Roeck wrote: > On Thu, May 21, 2015 at 02:07:31PM -0700, York Sun wrote: >> Lee, >> >> Is there any convention regarding I2C class bitmask? I see only three are >> defined for 3.12.19 and four for 4.0 >> >> I2C_CLASS_HWMON, I2C_CLASS_DDC, I2C_CLASS_SPD, I2C_CLASS_DEPRECATED >> >> I am working on a clock chip driver (SI5338) and trying to detect them >> (multiple >> chips in i2c mux). It would be a lot easier to have its own class, like >> I2C_CLASS_CLOCK. It is trivial to add a line to i2c.h file. Just checking if >> this is a bad idea. >> > > A class is supposed to indicate if a specific chip class is likely to be seen > on an i2c adapter, and that it may be necessary to auto-detect it (an example > are I2C_CLASS_HWMON type devices on PCs). The tendency, though, is to drop > existing markers for I2C_CLASS_xxx from adapter drivers as much as possible > because it slows down the boot process (see upstream commit 0c176170089c3). > > Auto-detection (with the _detect function) is not a preferred means to > instantiate a device. It takes time, and it is more or less unreliable. > For some chips, a read on its i2c register space can result in a chip reset, > or it can cause it to lose its programming. Worst case it can turn a system > into a brick. > > Preferred instantiations are listed in > Documentation/i2c/instantiating-devices. > Instantiation with devicetree, ACPI, or through i2c_register_board_info() > would probably be the best available methods to instantiate a clock chip. > > Given that, first question is why you would want to have the chip > auto-detected > in the first place. Is there any reason to believe that explicit instantiation > would not work in your system ? What are those reasons ? > > On top of that, the SI5338 does not have a clean way to detect the chip. > It does not have a chip ID register, and it is multi-banked. Given the > similarities of the various Silicon Labs clock chips, it may not even be > possible to reliably distinguish it from other SI chips. So even if you > had a good reason to auto-detect the chip, it would be _very_ unreliable. > This seems to be quite undesirable and risky for a clock chip. > Are you really sure that you want and need that ? > Guenter, Thanks for replying. No, I don't have to use autodetect. I was curious why there weren't more classes. I failed to notice which method is preferred in the mentioned document. I used to declare the devices by the bus number but met some issue when they are behind a mux. I temporarily used auto detect before I figure out how to describe the mux with i2c_board_info. Knowing auto-detection is not preferred, I will remove it from the proposed driver. Thanks again. York -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
clock driver
Linux experts, I have rewritten a driver for Silicon Labs SI5338 programmable clock chip. The original driver was written by Andrey (CC'ed), but was floatingn outside of the kernel. The driver was written to use sysfs as the interface, not the common clock framework. I wonder if I have to rewrite the driver following common clock framework. One concern is to support a feature to accept ClockBuilder (TM) output on sysfs. I don't see sysfs support on common clock framework. Please correct me if I am wrong. If not using common clock framework is acceptable, I would like to send a RFC patch for review. Regards, York -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: clock driver
On 05/26/2015 03:38 PM, Guenter Roeck wrote: > On Tue, May 26, 2015 at 12:12:11PM -0700, York Sun wrote: >> Linux experts, >> >> I have rewritten a driver for Silicon Labs SI5338 programmable clock chip. >> The >> original driver was written by Andrey (CC'ed), but was floatingn outside of >> the >> kernel. The driver was written to use sysfs as the interface, not the common >> clock framework. I wonder if I have to rewrite the driver following common >> clock >> framework. One concern is to support a feature to accept ClockBuilder (TM) >> output on sysfs. I don't see sysfs support on common clock framework. Please >> correct me if I am wrong. >> >> If not using common clock framework is acceptable, I would like to send a RFC >> patch for review. >> > My original driver for si570 was rejected because it didn't support the clock > framework, so you might face an uphill battle. > > SI provides a document for SI5338 describing how to configure it without > using clockbuilder [1]. Can that be used to implement generic code which > doesn't need clockbuilder ? > The driver is capable to handle the user's input and enable the clocks. Removing the support of importing is a step back. At least it is a feature I am using. I believe Andrey also used this feature when the driver was first drafted. That being said, my application relies on setting multiple clock chips on a PCIe device. That means I cannot put the configuration into device tree. There may be a way to fill device tree, but I am not ready to explore yet. Without a sysfs interface, can I change the configuration for each clock? I also found COMMON_CLK is a bool, not tristate. It is only selected by others. Is there a reason for doing so? My current platform (P1022DS) doesn't have CONFIG_COMMON_CLK enabled. York -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: clock driver
Michael, Can you give me some guidance here? On 05/26/2015 05:20 PM, York Sun wrote: > > > On 05/26/2015 03:38 PM, Guenter Roeck wrote: >> On Tue, May 26, 2015 at 12:12:11PM -0700, York Sun wrote: >>> Linux experts, >>> >>> I have rewritten a driver for Silicon Labs SI5338 programmable clock chip. >>> The >>> original driver was written by Andrey (CC'ed), but was floatingn outside of >>> the >>> kernel. The driver was written to use sysfs as the interface, not the common >>> clock framework. I wonder if I have to rewrite the driver following common >>> clock >>> framework. One concern is to support a feature to accept ClockBuilder (TM) >>> output on sysfs. I don't see sysfs support on common clock framework. Please >>> correct me if I am wrong. >>> >>> If not using common clock framework is acceptable, I would like to send a >>> RFC >>> patch for review. >>> >> My original driver for si570 was rejected because it didn't support the clock >> framework, so you might face an uphill battle. >> >> SI provides a document for SI5338 describing how to configure it without >> using clockbuilder [1]. Can that be used to implement generic code which >> doesn't need clockbuilder ? >> > > The driver is capable to handle the user's input and enable the clocks. > Removing > the support of importing is a step back. At least it is a feature I am using. > I > believe Andrey also used this feature when the driver was first drafted. > > That being said, my application relies on setting multiple clock chips on a > PCIe > device. That means I cannot put the configuration into device tree. There may > be > a way to fill device tree, but I am not ready to explore yet. Without a sysfs > interface, can I change the configuration for each clock? > > I also found COMMON_CLK is a bool, not tristate. It is only selected by > others. > Is there a reason for doing so? My current platform (P1022DS) doesn't have > CONFIG_COMMON_CLK enabled. > If converting my driver to common clock framework, I need to find a way to configure the clocks without recompiling the kernel. I will have about 30 clock chips (with different frequency) on multiple PCIe cards. York -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: clock driver
On 05/27/2015 12:09 AM, Sebastian Hesselbarth wrote: > On 27.05.2015 02:32, York Sun wrote: >> On 05/26/2015 05:20 PM, York Sun wrote: >>> On 05/26/2015 03:38 PM, Guenter Roeck wrote: >>>> On Tue, May 26, 2015 at 12:12:11PM -0700, York Sun wrote: >>>>> I have rewritten a driver for Silicon Labs SI5338 programmable clock >>>>> chip. The >>>>> original driver was written by Andrey (CC'ed), but was floatingn outside >>>>> of the >>>>> kernel. The driver was written to use sysfs as the interface, not the >>>>> common >>>>> clock framework. I wonder if I have to rewrite the driver following >>>>> common clock >>>>> framework. One concern is to support a feature to accept ClockBuilder (TM) >>>>> output on sysfs. I don't see sysfs support on common clock framework. >>>>> Please >>>>> correct me if I am wrong. >>>>> >>>>> If not using common clock framework is acceptable, I would like to send a >>>>> RFC >>>>> patch for review. >>>>> >>>> My original driver for si570 was rejected because it didn't support the >>>> clock >>>> framework, so you might face an uphill battle. >>>> >>>> SI provides a document for SI5338 describing how to configure it without >>>> using clockbuilder [1]. Can that be used to implement generic code which >>>> doesn't need clockbuilder ? >>>> >>> >>> The driver is capable to handle the user's input and enable the clocks. >>> Removing >>> the support of importing is a step back. At least it is a feature I am >>> using. I >>> believe Andrey also used this feature when the driver was first drafted. > > York, > > IMHO what kind of driver you need for the clock generator clearly > depends on the use case you have in mind. > > Also, why should a user ever be able to mess with the clocks? If you > allow a user to change the clock rate of any output, you have to > consider that he will likely be able to crash your system easily. > > As long as you cannot give a clear requirement for user-configurable > clocks - especially in the detail of the driver you mentioned - > mainline kernel is not the place for such a driver. Sebastian, This driver I am proposing supports SI5338 in a generic way. It can take device tree as its default configuration. However I am using it differently, explained in detail below. > >>> That being said, my application relies on setting multiple clock chips on a >>> PCIe >>> device. That means I cannot put the configuration into device tree. There >>> may be >>> a way to fill device tree, but I am not ready to explore yet. Without a >>> sysfs >>> interface, can I change the configuration for each clock? > > If the clock chip is part of a PCI device, the PCI driver should include > functions to set the clock rate. If you expose each clock with common > clock framework or not depends on how dynamically you want your clocks > to be. IMHO you have these options: > > (a) Clocks are limited to the PCI card and only need a limited set of > configurable clocks. You should add functions to load the registers > with either the full register map or parts of it in a table based > approach. You don't expose the clocks with CCF but deal with rate > change requests internally in the PCI driver. You could also consider > to have the initial clock configuration as part of some firmware blob > you request with the PCI driver. That's right. I only need to change a small portion of the configuration, such as frequency, but keeping the reset the same, including output driver voltage, input clock, etc. > > (b) Clocks are driving the PCI card as a parent clock but are not > dynamic. You should use the boot loader to load the register map or you > could simply use i2c-dev to load the registers from userspace. You'll > need no driver at all. > > (c) Clocks are driving clock inputs of your SoC and should be fully > dynamic, i.e. you cannot tell the rate that will be requested. Expose > each clock with CCF and find some good code to derive si5338 register > values from the requested rate. si5351 driver is an example but I know > silabs documentation isn't that good when it comes to dynamically > changing the clocks. With CCF you'll have no user interface at all > because users just shouldn't be able to mess with the clocks. > > [...] >> If converting my driver to common clock framework, I need to find
Re: clock driver
On 05/27/2015 09:42 AM, Sebastian Hesselbarth wrote: > On 27.05.2015 17:07, York Sun wrote: >> On 05/27/2015 12:09 AM, Sebastian Hesselbarth wrote: > [...] >>> Also, why should a user ever be able to mess with the clocks? If you >>> allow a user to change the clock rate of any output, you have to >>> consider that he will likely be able to crash your system easily. >>> >>> As long as you cannot give a clear requirement for user-configurable >>> clocks - especially in the detail of the driver you mentioned - >>> mainline kernel is not the place for such a driver. >> >> This driver I am proposing supports SI5338 in a generic way. It can take >> device >> tree as its default configuration. However I am using it differently, >> explained >> in detail below. > [...] >>> (a) Clocks are limited to the PCI card and only need a limited set of >>> configurable clocks. You should add functions to load the registers >>> with either the full register map or parts of it in a table based >>> approach. You don't expose the clocks with CCF but deal with rate >>> change requests internally in the PCI driver. You could also consider >>> to have the initial clock configuration as part of some firmware blob >>> you request with the PCI driver. >> >> That's right. I only need to change a small portion of the configuration, >> such >> as frequency, but keeping the reset the same, including output driver >> voltage, >> input clock, etc. > [...] >> My application has a host SoC booting up Linux. Then the clocks on PCIe >> (FPGA) >> cards get initialized with their clocks. The clocks are not used by host >> SoC, so >> setting the wrong clocks doesn't crash the system. Each PCIe card has up to >> four >> clock chips (with four clocks on each chip). It is required for users to be >> able >> to change the clocks after system boots up. > > Consider a userspace configurable clock driver, load the FPGA design > which depends on a specific frequency generated by Si5338 and let the > user mess with your sysfs files - that will certainly crash your system. > > Still, I do not see any requirement for a clock driver for that use > case. You have to load the FPGA design or at least configure it to > use the Si5338 generated clocks _after_ configuring Si5338. You'll > have to have a user interface for FPGA bitfile loading, so you can > add another one for the clock generator config. > >> I wrote my driver for the PCIe cards so the clocks can be initialized using >> the >> data provided. But changing the clocks, or initializing with another set of >> configuration requires an interface. There are many ways to solve this. I >> would >> like to keep the clock driver generic so it can be reused. It looks like CCF >> may >> not be the best fit for such driver. What is an acceptable way to write this >> driver so it can be in the mainline kernel, or other maintained projects (I >> am >> not aware of any though)? > > IMHO "generic" as in a generic mainline kernel clock driver just means > that other _drivers_ can request any clock rate from that chip. If you > want to write a CCF driver for Si5338, you'll have to make your PCIe > driver request that clock and hide the userspace configuration within > your PCIe driver. > > Adding userspace interfaces to generic CCF clock drivers will not happen > just because messing with the clocks will break a running system. As I > said before, AFAIKS i2c-dev should give you enough of an interface to > configure the clock generator from userspace. > Sebastian, Thanks for the insight. Looks like I should give up upstreaming this driver. I will find other ways to make this driver available if anyone wants to use it. York -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: clock driver
On 05/27/2015 10:09 AM, Guenter Roeck wrote: > On Wed, May 27, 2015 at 09:46:02AM -0700, York Sun wrote: >> > [ ... ] >> >> Sebastian, >> >> Thanks for the insight. Looks like I should give up upstreaming this driver. >> I >> will find other ways to make this driver available if anyone wants to use it. >> > > You might consider putting it on github. That is what I did with my si570 > driver. It was then picked up by someone else and ported to the clock > framework. > Win-win for everyone. > > Thanks, > Guenter Thanks for the suggestion. York -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: clock driver
On 05/27/2015 10:30 AM, Michael Turquette wrote: > Quoting York Sun (2015-05-26 17:32:13) >> Michael, >> >> Can you give me some guidance here? >> >> >> On 05/26/2015 05:20 PM, York Sun wrote: >>> >>> >>> On 05/26/2015 03:38 PM, Guenter Roeck wrote: >>>> On Tue, May 26, 2015 at 12:12:11PM -0700, York Sun wrote: >>>>> Linux experts, >>>>> >>>>> I have rewritten a driver for Silicon Labs SI5338 programmable clock >>>>> chip. The >>>>> original driver was written by Andrey (CC'ed), but was floatingn outside >>>>> of the >>>>> kernel. The driver was written to use sysfs as the interface, not the >>>>> common >>>>> clock framework. I wonder if I have to rewrite the driver following >>>>> common clock >>>>> framework. One concern is to support a feature to accept ClockBuilder (TM) >>>>> output on sysfs. I don't see sysfs support on common clock framework. >>>>> Please >>>>> correct me if I am wrong. > > Can you give me a link to some info about ClockBuilder? It is a software provided by Silicon Labs to create configuration. See http://www.silabs.com/products/clocksoscillators/Pages/Timing-Software-Development-Tools.aspx > >>>>> >>>>> If not using common clock framework is acceptable, I would like to send a >>>>> RFC >>>>> patch for review. >>>>> >>>> My original driver for si570 was rejected because it didn't support the >>>> clock >>>> framework, so you might face an uphill battle. >>>> >>>> SI provides a document for SI5338 describing how to configure it without >>>> using clockbuilder [1]. Can that be used to implement generic code which >>>> doesn't need clockbuilder ? >>>> >>> >>> The driver is capable to handle the user's input and enable the clocks. >>> Removing >>> the support of importing is a step back. At least it is a feature I am >>> using. I >>> believe Andrey also used this feature when the driver was first drafted. >>> >>> That being said, my application relies on setting multiple clock chips on a >>> PCIe >>> device. That means I cannot put the configuration into device tree. There >>> may be >>> a way to fill device tree, but I am not ready to explore yet. Without a >>> sysfs >>> interface, can I change the configuration for each clock? > > CCF does not have any dependency on DeviceTree. Zero. If you don't want > to use DT, then that is great! The ARM community has chosen DT, and the > ARM community by and large uses CCF in Linux. But there is no > requirement to use DT if you want to use CCF. That's good to know. > > Regarding sysfs, I really need to understand what interfaces you want > from sysfs. Can you provide a link to the ClockBuilder(TM) stuff? > > It is certainly possible for you to create sysfs entries in your clock > driver. Depending on what you want to do, there may be no need to > involve the framework in this stuff. Do you think you can keep your > sysfs stuff localized in your driver? I understand that I can create sysfs entries in my driver. I brought it up because I remember seeing your plan to add sysfs interface. If I add sysfs for this driver, it will not be a generic driver. > >>> >>> I also found COMMON_CLK is a bool, not tristate. It is only selected by >>> others. >>> Is there a reason for doing so? My current platform (P1022DS) doesn't have >>> CONFIG_COMMON_CLK enabled. >>> >> >> If converting my driver to common clock framework, I need to find a way to >> configure the clocks without recompiling the kernel. I will have about 30 >> clock >> chips (with different frequency) on multiple PCIe cards. > > Sure. Let's figure out your requirements and decide if we can make CCF > work for you. I think we can. > > I know that the Beagle Bone folks have been looking at modifying DT > blobs and changing them/loading them at run-time. That might be > interesting for your on-the-fly clock configuration. > > If you don't like DT then perhaps you can export your sysfs clock stuff > via your clock driver, instead of the framework? > Like I explained in the other email of this thread, I plan to use the clock driver to initialize clocks on PCIe (FPGA) cards which four chips on each card. The clocks will be set by the host SoC for FPGA to use. Messing with the
Re: clock driver
On 05/27/2015 11:15 AM, Guenter Roeck wrote: > On Wed, May 27, 2015 at 10:45:32AM -0700, York Sun wrote: >> >> >> On 05/27/2015 10:30 AM, Michael Turquette wrote: >>> Quoting York Sun (2015-05-26 17:32:13) >>>> Michael, >>>> >>>> Can you give me some guidance here? >>>> >>>> >>>> On 05/26/2015 05:20 PM, York Sun wrote: >>>>> >>>>> >>>>> On 05/26/2015 03:38 PM, Guenter Roeck wrote: >>>>>> On Tue, May 26, 2015 at 12:12:11PM -0700, York Sun wrote: >>>>>>> Linux experts, >>>>>>> >>>>>>> I have rewritten a driver for Silicon Labs SI5338 programmable clock >>>>>>> chip. The >>>>>>> original driver was written by Andrey (CC'ed), but was floatingn >>>>>>> outside of the >>>>>>> kernel. The driver was written to use sysfs as the interface, not the >>>>>>> common >>>>>>> clock framework. I wonder if I have to rewrite the driver following >>>>>>> common clock >>>>>>> framework. One concern is to support a feature to accept ClockBuilder >>>>>>> (TM) >>>>>>> output on sysfs. I don't see sysfs support on common clock framework. >>>>>>> Please >>>>>>> correct me if I am wrong. >>> >>> Can you give me a link to some info about ClockBuilder? >> >> It is a software provided by Silicon Labs to create configuration. See >> http://www.silabs.com/products/clocksoscillators/Pages/Timing-Software-Development-Tools.aspx >> >>> >>>>>>> >>>>>>> If not using common clock framework is acceptable, I would like to send >>>>>>> a RFC >>>>>>> patch for review. >>>>>>> >>>>>> My original driver for si570 was rejected because it didn't support the >>>>>> clock >>>>>> framework, so you might face an uphill battle. >>>>>> >>>>>> SI provides a document for SI5338 describing how to configure it without >>>>>> using clockbuilder [1]. Can that be used to implement generic code which >>>>>> doesn't need clockbuilder ? >>>>>> >>>>> >>>>> The driver is capable to handle the user's input and enable the clocks. >>>>> Removing >>>>> the support of importing is a step back. At least it is a feature I am >>>>> using. I >>>>> believe Andrey also used this feature when the driver was first drafted. >>>>> >>>>> That being said, my application relies on setting multiple clock chips on >>>>> a PCIe >>>>> device. That means I cannot put the configuration into device tree. There >>>>> may be >>>>> a way to fill device tree, but I am not ready to explore yet. Without a >>>>> sysfs >>>>> interface, can I change the configuration for each clock? >>> >>> CCF does not have any dependency on DeviceTree. Zero. If you don't want >>> to use DT, then that is great! The ARM community has chosen DT, and the >>> ARM community by and large uses CCF in Linux. But there is no >>> requirement to use DT if you want to use CCF. >> >> That's good to know. >> >>> >>> Regarding sysfs, I really need to understand what interfaces you want >>> from sysfs. Can you provide a link to the ClockBuilder(TM) stuff? >>> >>> It is certainly possible for you to create sysfs entries in your clock >>> driver. Depending on what you want to do, there may be no need to >>> involve the framework in this stuff. Do you think you can keep your >>> sysfs stuff localized in your driver? >> >> I understand that I can create sysfs entries in my driver. I brought it up >> because I remember seeing your plan to add sysfs interface. >> >> If I add sysfs for this driver, it will not be a generic driver. >> >>> >>>>> >>>>> I also found COMMON_CLK is a bool, not tristate. It is only selected by >>>>> others. >>>>> Is there a reason for doing so? My current platform (P1022DS) doesn't have >>>>> CONFIG_COMMON_CLK enabled. >>>>> >>>> >>>> If converting my driver to common clock framework, I need to fi
Re: clock driver
On 05/27/2015 11:54 AM, Guenter Roeck wrote: > On Wed, May 27, 2015 at 11:24:50AM -0700, York Sun wrote: >> >>> >>> Someone suggested earlier that the clocks should be set from the PCIe >>> driver. >>> Question here is really if you need to control the clocks from user space. >>> Even if you do, the driver for the chip _using_ the clocks would be better >>> suited for changing the clock rates than the clock driver itself. This way >>> it >>> can ensure that the clock rates are sane for the given use case, and the use >>> case is kept out of the clock driver. >>> >>>> I wrote a driver for the PCIe card to load FPGA images. To setup the >>>> clocks, I >>>> rewrote the SI5338 driver and set the desired frequencies using sysfs. This >>>> driver has a feature to import/dump the raw configuration data. That's one >>>> feature I plan to use, at least for debugging. >>>> >>> With the above in mind, the idea would be for the PCIe driver to set the >>> clock >>> rates. >> >> I am interested in this path. Can you explain a little bit more about setting >> the clock rates? I have no experience on CCF. >> > The API functions are documented in include/linux/clk.h. What you are looking > for here would be clk_set_rate() and related functions such as > clk_round_rate(), > and of course support functions such as devm_clk_get(), clk_prepare_enable(), > and clk_disable_unprepare(). > > You can find lots of examples on how this works if you search for > clk_set_rate() > in the kernel source. Yes I saw them. I have no experience with CCF. It will take some learning time. I am trying to find an SI5xx EVB board so I can try it out. > >>> >>>> I don't think device tree is the best for my application because I will >>>> have >>>> about 30 clock chips to program in the complete system. It is easier to >>>> use user >>>> space application to do so from I2C bus. >>>> >>> Devicetree is static, so you might have to use devicetree overlays if the >>> programming changes at runtime. Not sure why the number of clock chips >>> would make that non-feasible, though. >> >> I mean the existence is unknown for many chips. The baseline is I can't use >> static data. >> >>> >>> On a side note, we are using devicetree a lot for PCIe devices. >> >> It's tempting. I want to explore this direction at a later phase of my >> project. >> I will appreciate if you can point me to something. >> > I can send you some devicetree files if you think that would help. Note that > we > are heavily using devicetree overlays, so this is all a bit WIP. Yes, please send them to me. > >>> >>>> Earlier Guenter helped me to comb through the idea and we concluded CCF >>>> may not >>>> be the best fit for this application. I wonder if CCF is the only way to >>>> get >>> >>> Well, you did ;-). I think it would be feasible, but you would have to >>> change >>> your viewpoint (in respect to how to control the clocks). >> >> Right, I did. I will revisit this after bring up the platform initially, >> when I >> have more knowledge about those clocks. Maybe I should add an interface for >> my >> FPGA driver to request clock rates, instead of setting them from clock >> driver. >> It sounds better. >> > Yes, that would be the idea. Essentially your FPGA driver could either > determine > the clock rate(s) it needs internally, or you could set the clock rate(s) > through sysfs attributes attached to the FPGA driver. The FPGA driver would > then > use clk_set_rate() to set the rate in the clock chip. > Sounds promising. Thanks for the guidance. York -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: clock driver
On 05/27/2015 11:11 PM, Michael Turquette wrote:
>
> Hi Andrey,
>
> I think this is a cool problem to solve. As far as frontier devices go I
> really can't say. Other companies create similar clock generators that
> are programmed at run-time over i2c. And we already have support merged
> for Silicon Labs 5351 and 570 devices:
>
> drivers/clk/clk-si5351.c
> drivers/clk/clk-si5351.h
> drivers/clk/clk-si570.c
>
> I'm not sure that we need to group such devices into a new "class". The
> Linux common clock framework (ccf) has two classes: clock providers and
> clock consumers. We haven't needed any more classification than that
> thus far.
>
> I took a look at your github code and it looks like you expose registers
> individually as sysfs files. That is a start, but a better abstraction
> is to consider the clock input signals that your pcie/fpga device will
> take as input. With a proper clock driver for the silabs part your
> pcie/fpga driver could hopefully just call clk_prepare_enable and
> clk_set_rate and clk_set_parent as needed on those clocks to configure
> them for consumption by the downstream fpga.
>
> According to the data sheet[0] it looks like there are not many clock
> outputs (CLK0{A,B}, CLK1{A,B}, CLK2{A,B}, and CLK3{A,B}).
>
> At what point do you know how the clocks should be configured? I am
> guessing that your fpga driver (e.g. the clock consumer) figures that
> out as bytestream blobs are loaded? So that seems like the right call
> site to start enabling clocks and configuring rates, instead of stuffing
> that into the silabs driver (e.g. the clock provider).
I think it works for my case. I will explore this direction.
>
> York,
>
> There is already a way to clock drivers to extend the debugfs interfaces
> for per-driver stuff. The Nvidia Tegra guys do this already in their
> out-of-tree test modules. Do you think such an interface might be
> helpful to you? See:
>
> clk_debugfs_add_file in drivers/clk/clk.c
> and,
> http://lkml.kernel.org/r/<1403794853-16928-1-git-send-email-pdeschrij...@nvidia.com>
>
> So your silabs clock generator driver could export some custom knobs
> which help out in the early phases of development.
>
> Ideally this interface would not be a register-level interface, but an
> output signal type interface. Here is an example of the files you will
> have by default:
>
> $ ls /sys/kernel/debug/clk/clk0a/
> clk_accuracy clk_flags clk_phase clk_rate
> clk_enable_count clk_notifier_count clk_prepare_count
>
> With the custom debugfs interface you might add a "clk_set_rate" file
> where user space can write to it and change the frequency of that clock
> signal. You can do the same to change mux settings and clock gates as
> well.
>
> A userspace tool might even be able to take the clockbuilder data to do
> this, if someone is willing to write it.
>
> [0] https://www.silabs.com/Support%20Documents/TechnicalDocs/Si5338.pdf
Thanks for the suggestion. I think I can limit the features of this clock chip
and fit it into CCF. Earlier I thought too much about exposing all features for
general use, which shouldn't be the case for the clocks. I will see if I can
remove those features or reserve them for debug use.
York
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Common clock framework examples
Michael, Let me start a new thread for more questions regarding common clock framework. Following yours and other experts' suggestion, I start to write a new driver for SI5338. As I explained earlier, I have multiple clock chips. They may have different clock sources. I haven't figured out how to put them into device tree because the clocks chips are on PCIe cards. Let's say I want to initialize without device tree. Is there an example to setup platform data structure so I can put in the clock rate of xtal or clk_in? York -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: clock driver
Michael, I have rewritten the driver to use CCF. It will be sent for review when ready. I have some questions, hoping you can shed some light on them. Q1: What does of_clk_add_provider do? I read the code and comment. It registers a clock provider for a node. How is it used after registration? Q2: What do you suggest to name multiple clocks on PCIe (FPGA) cards? I will have multiple cards with multiple clock chips on each. The clock driver can handle clocks on each chip. Is it recommended to create unique names by the driver? Is there any example to follow? Q3: Is there a guideline or an API to create sub folder under debugfs "clk"? Thanks a lot. York -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] driver/clk/clk-si5338: Add common clock framework driver for si5338
SI5338 is a programmable clock generator. It has 4 sets of inputs, PLL, multisynth and dividers to make 4 outputs. This driver splits them into multiple clocks to comply with common clock framework. See Documentation/devicetree/bindings/clock/silabs,si5338.txt for details. Signed-off-by: York Sun CC: Mike Turquette CC: Sebastian Hesselbarth CC: Guenter Roeck CC: Andrey Filippov --- .../devicetree/bindings/clock/silabs,si5338.txt| 173 + drivers/clk/Kconfig| 14 +- drivers/clk/Makefile |1 + drivers/clk/clk-si5338.c | 3653 drivers/clk/clk-si5338.h | 305 ++ include/dt-bindings/clock/clk-si5338.h | 69 + include/linux/platform_data/si5338.h | 49 + 7 files changed, 4263 insertions(+), 1 deletion(-) create mode 100644 Documentation/devicetree/bindings/clock/silabs,si5338.txt create mode 100644 drivers/clk/clk-si5338.c create mode 100644 drivers/clk/clk-si5338.h create mode 100644 include/dt-bindings/clock/clk-si5338.h create mode 100644 include/linux/platform_data/si5338.h diff --git a/Documentation/devicetree/bindings/clock/silabs,si5338.txt b/Documentation/devicetree/bindings/clock/silabs,si5338.txt new file mode 100644 index 000..aeeb81d --- /dev/null +++ b/Documentation/devicetree/bindings/clock/silabs,si5338.txt @@ -0,0 +1,173 @@ +Binding for Silicon Labs Si5338 programmable i2c clock generator. + +Reference +[1] Si5338 Data Sheet +http://www.silabs.com/Support%20Documents/TechnicalDocs/Si5338.pdf + +The Si5338 is a programmable i2c clock generators with up to 4 output +clocks. It has 4 sets of possible input clocks + +IN1/IN2: differential +IN3: single-ended +IN4: single-ended +IN5/IN6: differential + +Additionally, IN1/IN2 can be used as XTAL with different setting. +The clock tree looks like below (without support of zero-delay) + + + IN1/IN2 IN3 IN4 IN5/IN6 + | | | | + --| | | | + | | | | | + | \ / \ / + | \ / \ / + | \ / \ / + XTAL REFCLKFBCLK + | | \ / | + | | \ /| + | | DIVREFCLK DIVFBCLK | + | | \ / | + | | \ / | + | | \ /| + | |PLL | + | | / | | \ | + | | / / \ \ | + | |/ / \ \ | + | | / | | \| + | | | | | || + | | MS0 MS1 MS2 MS3 | + | | | | | || + + OUT0 OUT1 OUT2 OUT3 + +The output clock can choose from any of the above clock as its source, with +exceptions: MS1 can only be used for OUT1, MS2 can only be used for OUT2, MS3 +can only be used for OUT3. + +==I2C device node== + +Required properties: +- compatible: shall be "silabs,si5338". +- reg: i2c device address, shall be 0x60, 0x61, 0x70, or 0x71 +- #clock-cells: shall be set to 1 for multiple outputs +- clocks: list of parent clocks in the order of , , , , + Note, xtal and in1/2 are mutually exclusive. Only one can be set. +- #address-cells: shall be set to 1. +- #size-cells: shall be set to 0. + +Optional properties if not set by platform driver: +- silab,ref-source: source of refclk, valid value is defined as + #define SI5338_REF_SRC_CLKIN12 0 + #define SI5338_REF_SRC_CLKIN3 1 + #define SI5338_REF_SRC_XTAL 4 +- silab,fb-source: source of fbclk, valid value is defined as + #define SI5338_FB_SRC_CLKIN42 + #define SI5338_FB_SRC_CLKIN56 3 + #define SI5338_FB_SRC_NOCLK 5 +- silabs,pll-source: source of pll, valid value is defined as + #define SI5338_PFD_IN_REF_REFCLK 0 + #define SI5338_PFD_IN_REF_FBCLK1 + #define SI5338_PFD_IN_REF_DIVREFCLK2 + #define SI5338_PFD_IN_REF_DIVFBCLK 3 + #define SI5338_PFD_IN_REF_XOCLK4 + #define SI5338_PFD_IN_REF_NOCLK5 +- silabs,pll-master: Pick one MS (0, 1, 2, or 3) to allow chaning PLL rate + This is arbitrary since MS0/1/2/3 share one PLL. PLL can be calculated + backward to satisfy MS. + +==Child nodes== + +Each of the clock outputs can be configured individually by +using a child node to the I2C device node. If a child node for a clock +output is not set, platform driver has to set up. + +Required child node properties: +- reg: number of clock output. + +Optional child node properties: +- silabs,drive-config: the configuration of output driver + The valid value list is long. Please refer to soruce code. +- silabs,clock-source: source clock of the output divider + #define SI5338_OUT_MUX_FBCLK0 +
[Patch v2] driver/clk/clk-si5338: Add common clock framework driver for si5338
SI5338 is a programmable clock generator. It has 4 sets of inputs, PLL, multisynth and dividers to make 4 outputs. This driver splits them into multiple clocks to comply with common clock framework. See Documentation/devicetree/bindings/clock/silabs,si5338.txt for details. Signed-off-by: York Sun CC: Mike Turquette CC: Sebastian Hesselbarth CC: Guenter Roeck CC: Andrey Filippov --- Change log: v2: Fix handling name prefix if the driver is unloaded and loaded again .../devicetree/bindings/clock/silabs,si5338.txt| 173 + drivers/clk/Kconfig| 14 +- drivers/clk/Makefile |1 + drivers/clk/clk-si5338.c | 3656 drivers/clk/clk-si5338.h | 305 ++ include/dt-bindings/clock/clk-si5338.h | 69 + include/linux/platform_data/si5338.h | 49 + 7 files changed, 4266 insertions(+), 1 deletion(-) create mode 100644 Documentation/devicetree/bindings/clock/silabs,si5338.txt create mode 100644 drivers/clk/clk-si5338.c create mode 100644 drivers/clk/clk-si5338.h create mode 100644 include/dt-bindings/clock/clk-si5338.h create mode 100644 include/linux/platform_data/si5338.h diff --git a/Documentation/devicetree/bindings/clock/silabs,si5338.txt b/Documentation/devicetree/bindings/clock/silabs,si5338.txt new file mode 100644 index 000..aeeb81d --- /dev/null +++ b/Documentation/devicetree/bindings/clock/silabs,si5338.txt @@ -0,0 +1,173 @@ +Binding for Silicon Labs Si5338 programmable i2c clock generator. + +Reference +[1] Si5338 Data Sheet +http://www.silabs.com/Support%20Documents/TechnicalDocs/Si5338.pdf + +The Si5338 is a programmable i2c clock generators with up to 4 output +clocks. It has 4 sets of possible input clocks + +IN1/IN2: differential +IN3: single-ended +IN4: single-ended +IN5/IN6: differential + +Additionally, IN1/IN2 can be used as XTAL with different setting. +The clock tree looks like below (without support of zero-delay) + + + IN1/IN2 IN3 IN4 IN5/IN6 + | | | | + --| | | | + | | | | | + | \ / \ / + | \ / \ / + | \ / \ / + XTAL REFCLKFBCLK + | | \ / | + | | \ /| + | | DIVREFCLK DIVFBCLK | + | | \ / | + | | \ / | + | | \ /| + | |PLL | + | | / | | \ | + | | / / \ \ | + | |/ / \ \ | + | | / | | \| + | | | | | || + | | MS0 MS1 MS2 MS3 | + | | | | | || + + OUT0 OUT1 OUT2 OUT3 + +The output clock can choose from any of the above clock as its source, with +exceptions: MS1 can only be used for OUT1, MS2 can only be used for OUT2, MS3 +can only be used for OUT3. + +==I2C device node== + +Required properties: +- compatible: shall be "silabs,si5338". +- reg: i2c device address, shall be 0x60, 0x61, 0x70, or 0x71 +- #clock-cells: shall be set to 1 for multiple outputs +- clocks: list of parent clocks in the order of , , , , + Note, xtal and in1/2 are mutually exclusive. Only one can be set. +- #address-cells: shall be set to 1. +- #size-cells: shall be set to 0. + +Optional properties if not set by platform driver: +- silab,ref-source: source of refclk, valid value is defined as + #define SI5338_REF_SRC_CLKIN12 0 + #define SI5338_REF_SRC_CLKIN3 1 + #define SI5338_REF_SRC_XTAL 4 +- silab,fb-source: source of fbclk, valid value is defined as + #define SI5338_FB_SRC_CLKIN42 + #define SI5338_FB_SRC_CLKIN56 3 + #define SI5338_FB_SRC_NOCLK 5 +- silabs,pll-source: source of pll, valid value is defined as + #define SI5338_PFD_IN_REF_REFCLK 0 + #define SI5338_PFD_IN_REF_FBCLK1 + #define SI5338_PFD_IN_REF_DIVREFCLK2 + #define SI5338_PFD_IN_REF_DIVFBCLK 3 + #define SI5338_PFD_IN_REF_XOCLK4 + #define SI5338_PFD_IN_REF_NOCLK5 +- silabs,pll-master: Pick one MS (0, 1, 2, or 3) to allow chaning PLL rate + This is arbitrary since MS0/1/2/3 share one PLL. PLL can be calculated + backward to satisfy MS. + +==Child nodes== + +Each of the clock outputs can be configured individually by +using a child node to the I2C device node. If a child node for a clock +output is not set, platform driver has to set up. + +Required child node properties: +- reg: number of clock output. + +Optional child node properties: +- silabs,drive-config: the configuration of output driver + The valid value list is long. Please refer to soruce code. +- silabs,clock-source: source clock of
Re: [Patch v2] driver/clk/clk-si5338: Add common clock framework driver for si5338
Paul,
Thanks for reviewing.
On 06/16/2015 01:21 AM, Paul Bolle wrote:
> One question and a few nits follow.
>
> On Mon, 2015-06-15 at 10:07 -0700, York Sun wrote:
>> SI5338 is a programmable clock generator. It has 4 sets of inputs,
>> PLL, multisynth and dividers to make 4 outputs. This driver splits
>> them into multiple clocks to comply with common clock framework.
>>
>> See Documentation/devicetree/bindings/clock/silabs,si5338.txt for
>> details.
>>
>> Signed-off-by: York Sun
>> CC: Mike Turquette
>
> Apparently that's now mturque...@baylibre.com .
Thanks. Will change.
>
>> CC: Sebastian Hesselbarth
>> CC: Guenter Roeck
>> CC: Andrey Filippov
>
>> --- a/drivers/clk/Kconfig
>> +++ b/drivers/clk/Kconfig
>
>> config COMMON_CLK
>> -bool
>> +tristate "Common Clock"
>> select HAVE_CLK_PREPARE
>> select CLKDEV_LOOKUP
>> select SRCU
>
> Why? The commit explanation doesn't mention this. Did you use an unclean
> tree? If not, you just created over a dozen of new modules:
Thanks for catching this. I was testing building the driver within and outside
of kernel tree for another kernel version. If this driver is built-in, I don't
need to make it tristate. Will revert in next version.
> $ git grep -nw CONFIG_COMMON_CLK -- "*Makefile*"
> arch/arm/mach-mmp/Makefile:13:ifeq ($(CONFIG_COMMON_CLK), )
> arch/arm/mach-shmobile/Makefile:21:ifndef CONFIG_COMMON_CLK
> arch/powerpc/platforms/512x/Makefile:4:obj-$(CONFIG_COMMON_CLK) +=
> clock-commonclk.o
> drivers/clk/Makefile:4:obj-$(CONFIG_COMMON_CLK) += clk.o
> drivers/clk/Makefile:5:obj-$(CONFIG_COMMON_CLK) += clk-divider.o
> drivers/clk/Makefile:6:obj-$(CONFIG_COMMON_CLK) += clk-fixed-factor.o
> drivers/clk/Makefile:7:obj-$(CONFIG_COMMON_CLK) += clk-fixed-rate.o
> drivers/clk/Makefile:8:obj-$(CONFIG_COMMON_CLK) += clk-gate.o
> drivers/clk/Makefile:9:obj-$(CONFIG_COMMON_CLK) += clk-mux.o
> drivers/clk/Makefile:10:obj-$(CONFIG_COMMON_CLK)+= clk-composite.o
> drivers/clk/Makefile:11:obj-$(CONFIG_COMMON_CLK)+=
> clk-fractional-divider.o
> drivers/clk/Makefile:12:obj-$(CONFIG_COMMON_CLK)+= clk-gpio-gate.o
> drivers/clk/Makefile:14:obj-$(CONFIG_COMMON_CLK)+= clk-conf.o
> drivers/clk/Makefile:59:ifeq ($(CONFIG_COMMON_CLK), y)
> drivers/clk/samsung/Makefile:5:obj-$(CONFIG_COMMON_CLK) += clk.o clk-pll.o
> drivers/gpu/drm/msm/Makefile:53:msm-$(CONFIG_COMMON_CLK) +=
> mdp/mdp4/mdp4_lvds_pll.o
> drivers/sh/Makefile:5:ifneq ($(CONFIG_COMMON_CLK),y)
>
>> +config COMMON_CLK_SI5338
>> +tristate "Clock driver for SiLabs 5338"
>> +depends on I2C
>> +select REGMAP_I2C
>> +select RATIONAL
>> +---help---
>> + This driver supports Silicon Labs 5338 programmable clock generators,
>> + using common clock framework. It needs parent clock as input(s).
>> + Internal clocks are registered with unique names in case multiple
>> + devices exist. See devicetree/bindings/clock/silabs,si5338.txt
>> + under Documentation for details.
>
>> --- /dev/null
>> +++ b/drivers/clk/clk-si5338.c
>
>> +unsigned long si5338_divrefclk_recalc_rate(struct clk_hw *hw,
>> + unsigned long parent_rate)
>> +{
>> +[...]
>> +}
>
> Can't this be made static? It compiles cleanly with static too. Is there
> some subtle issue I'm missing?
>
Absolutely. I must have missed them for some functions.
>> +static const struct clk_ops si5338_divrefclk_ops = {
>> +.recalc_rate = si5338_divrefclk_recalc_rate,
>> +};
>
>> +unsigned long si5338_divfbclk_recalc_rate(struct clk_hw *hw,
>> + unsigned long parent_rate)
>> +{
>> +[...]
>> +}
>
> Ditto.
>
>> +static const struct clk_ops si5338_divfbclk_ops = {
>> +.recalc_rate = si5338_divfbclk_recalc_rate,
>> +};
>
>> --- /dev/null
>> +++ b/include/dt-bindings/clock/clk-si5338.h
>
>> +#ifndef _DT_BINDINGS_CLK_DSI5338_H
>> +#define _DT_BINDINGS_CLK_DSI5338_H
>
> (I spotted a D that looks odd here.)
Me, too. It takes fresh eyes to spot this non-sense error.
>
> And git am whines:
> new blank line at EOF.
Thanks.
York
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" 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] driver/clk/clk-si5338: Add common clock framework driver for si5338
On 06/16/2015 08:18 AM, York Sun wrote: > Paul, > > Thanks for reviewing. > > On 06/16/2015 01:21 AM, Paul Bolle wrote: >> One question and a few nits follow. >> >> On Mon, 2015-06-15 at 10:07 -0700, York Sun wrote: >>> SI5338 is a programmable clock generator. It has 4 sets of inputs, >>> PLL, multisynth and dividers to make 4 outputs. This driver splits >>> them into multiple clocks to comply with common clock framework. >>> >>> See Documentation/devicetree/bindings/clock/silabs,si5338.txt for >>> details. >>> >>> Signed-off-by: York Sun >>> CC: Mike Turquette >> >> Apparently that's now mturque...@baylibre.com . > > Thanks. Will change. > >> >>> CC: Sebastian Hesselbarth >>> CC: Guenter Roeck >>> CC: Andrey Filippov >> >>> --- a/drivers/clk/Kconfig >>> +++ b/drivers/clk/Kconfig >> >>> config COMMON_CLK >>> - bool >>> + tristate "Common Clock" >>> select HAVE_CLK_PREPARE >>> select CLKDEV_LOOKUP >>> select SRCU >> >> Why? The commit explanation doesn't mention this. Did you use an unclean >> tree? If not, you just created over a dozen of new modules: > > Thanks for catching this. I was testing building the driver within and outside > of kernel tree for another kernel version. If this driver is built-in, I don't > need to make it tristate. Will revert in next version. > Now I remember why I did this. COMMON_CLK wasn't an option users can select, because it is a bool and only selected by some platforms. I think it should be a tristate so one can build a driver with it. When it is selected, some drivers are built, either into kernel or as modules, up to user's choice. They are needed by common clock framework. I should add explanation in commit message. Or separate it into an individual patch. Which one is preferred? York -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[Patch v3] driver/clk/clk-si5338: Add common clock framework driver for si5338
SI5338 is a programmable clock generator. It has 4 sets of inputs, PLL, multisynth and dividers to make 4 outputs. This driver splits them into multiple clocks to comply with common clock framework. See Documentation/devicetree/bindings/clock/silabs,si5338.txt for details. COMMON_CLK in Kconfig is changed from bool to tristate so all common clock framework drivers can be selected by users. Export __clk_is_prepared from clk.c so drivers can check and unprepare clocks upon removal. Signed-off-by: York Sun CC: Mike Turquette CC: Sebastian Hesselbarth CC: Guenter Roeck CC: Andrey Filippov CC: Paul Bolle --- Change log: v3: Add calling unprepare upon removal Add registering to clkdev so the clk can be acquired when device tree is not in use Add a dev_info message when driver is removed Add missing "static" to two functions in clk-si5338.c Cosmatic fix in dt-bindings.clock/clk-si5338.h v2: Fix handling name prefix if the driver is unloaded and loaded again .../devicetree/bindings/clock/silabs,si5338.txt| 173 + drivers/clk/Kconfig| 14 +- drivers/clk/Makefile |1 + drivers/clk/clk-si5338.c | 3679 drivers/clk/clk-si5338.h | 305 ++ drivers/clk/clk.c |1 + include/dt-bindings/clock/clk-si5338.h | 68 + include/linux/platform_data/si5338.h | 49 + 8 files changed, 4289 insertions(+), 1 deletion(-) create mode 100644 Documentation/devicetree/bindings/clock/silabs,si5338.txt create mode 100644 drivers/clk/clk-si5338.c create mode 100644 drivers/clk/clk-si5338.h create mode 100644 include/dt-bindings/clock/clk-si5338.h create mode 100644 include/linux/platform_data/si5338.h diff --git a/Documentation/devicetree/bindings/clock/silabs,si5338.txt b/Documentation/devicetree/bindings/clock/silabs,si5338.txt new file mode 100644 index 000..aeeb81d --- /dev/null +++ b/Documentation/devicetree/bindings/clock/silabs,si5338.txt @@ -0,0 +1,173 @@ +Binding for Silicon Labs Si5338 programmable i2c clock generator. + +Reference +[1] Si5338 Data Sheet +http://www.silabs.com/Support%20Documents/TechnicalDocs/Si5338.pdf + +The Si5338 is a programmable i2c clock generators with up to 4 output +clocks. It has 4 sets of possible input clocks + +IN1/IN2: differential +IN3: single-ended +IN4: single-ended +IN5/IN6: differential + +Additionally, IN1/IN2 can be used as XTAL with different setting. +The clock tree looks like below (without support of zero-delay) + + + IN1/IN2 IN3 IN4 IN5/IN6 + | | | | + --| | | | + | | | | | + | \ / \ / + | \ / \ / + | \ / \ / + XTAL REFCLKFBCLK + | | \ / | + | | \ /| + | | DIVREFCLK DIVFBCLK | + | | \ / | + | | \ / | + | | \ /| + | |PLL | + | | / | | \ | + | | / / \ \ | + | |/ / \ \ | + | | / | | \| + | | | | | || + | | MS0 MS1 MS2 MS3 | + | | | | | || + + OUT0 OUT1 OUT2 OUT3 + +The output clock can choose from any of the above clock as its source, with +exceptions: MS1 can only be used for OUT1, MS2 can only be used for OUT2, MS3 +can only be used for OUT3. + +==I2C device node== + +Required properties: +- compatible: shall be "silabs,si5338". +- reg: i2c device address, shall be 0x60, 0x61, 0x70, or 0x71 +- #clock-cells: shall be set to 1 for multiple outputs +- clocks: list of parent clocks in the order of , , , , + Note, xtal and in1/2 are mutually exclusive. Only one can be set. +- #address-cells: shall be set to 1. +- #size-cells: shall be set to 0. + +Optional properties if not set by platform driver: +- silab,ref-source: source of refclk, valid value is defined as + #define SI5338_REF_SRC_CLKIN12 0 + #define SI5338_REF_SRC_CLKIN3 1 + #define SI5338_REF_SRC_XTAL 4 +- silab,fb-source: source of fbclk, valid value is defined as + #define SI5338_FB_SRC_CLKIN42 + #define SI5338_FB_SRC_CLKIN56 3 + #define SI5338_FB_SRC_NOCLK 5 +- silabs,pll-source: source of pll, valid value is defined as + #define SI5338_PFD_IN_REF_REFCLK 0 + #define SI5338_PFD_IN_REF_FBCLK1 + #define SI5338_PFD_IN_REF_DIVREFCLK2 + #define SI5338_PFD_IN_REF_DIVFBCLK 3 + #define SI5338_PFD_IN_REF_XOCLK4 + #define SI5338_PFD_IN_REF_NOCLK5 +- silabs,pll-master: Pick one MS (0, 1, 2, or 3)
[PATCH] driver/i2c/mux: Add register based mux i2c-mux-reg
Based on i2c-mux-gpio driver, similarly the register based mux
switch from one bus to another by setting a single register.
The register can be on PCIe bus, local bus, or any memory-mapped
address.
Signed-off-by: York Sun
CC: Wolfram Sang
CC: Peter Korsgaard
---
.../devicetree/bindings/i2c/i2c-mux-reg.txt| 69 ++
drivers/i2c/muxes/Kconfig | 11 +
drivers/i2c/muxes/Makefile |1 +
drivers/i2c/muxes/i2c-mux-reg.c| 239
drivers/i2c/muxes/i2c-mux-reg.h| 38
5 files changed, 358 insertions(+)
create mode 100644 Documentation/devicetree/bindings/i2c/i2c-mux-reg.txt
create mode 100644 drivers/i2c/muxes/i2c-mux-reg.c
create mode 100644 drivers/i2c/muxes/i2c-mux-reg.h
diff --git a/Documentation/devicetree/bindings/i2c/i2c-mux-reg.txt
b/Documentation/devicetree/bindings/i2c/i2c-mux-reg.txt
new file mode 100644
index 000..ad7cc4f
--- /dev/null
+++ b/Documentation/devicetree/bindings/i2c/i2c-mux-reg.txt
@@ -0,0 +1,69 @@
+Register-based I2C Bus Mux
+
+This binding describes an I2C bus multiplexer that uses a single regsiter
+to route the I2C signals.
+
+Required properties:
+- compatible: i2c-mux-reg
+- i2c-parent: The phandle of the I2C bus that this multiplexer's master-side
+ port is connected to.
+* Standard I2C mux properties. See mux.txt in this directory.
+* I2C child bus nodes. See mux.txt in this directory.
+
+Optional properties:
+- reg: this pair of specifies the register to control the mux.
+ The depends on its parent node. It can be any memory-mapped
+ address. If omitted, the resource of this device will be used.
+- idle-state: value to set the muxer to when idle. When no value is
+ given, it defaults to the last value used.
+
+For each i2c child node, an I2C child bus will be created. They will
+be numbered based on their order in the device tree.
+
+Whenever an access is made to a device on a child bus, the value set
+in the revelant node's reg property will be output to the register.
+
+If an idle state is defined, using the idle-state (optional) property,
+whenever an access is not being made to a device on a child bus, the
+register will be set according to the idle value.
+
+If an idle state is not defined, the most recently used value will be
+left programmed into the register.
+
+Example of a mux on PCIe card, the host is a powerpc SoC (big endian):
+
+ i2c-mux {
+ /* the depends on the address translation
+* of the parent device. If omitted, device resource
+* will be used instead.
+*/
+ reg = <0x6028 0x4>;
+ compatible = "i2c-mux-reg";
+ #address-cells = <1>;
+ #size-cells = <0>;
+ i2c-parent = <&i2c1>;
+ i2c@0 {
+ reg = <0>;
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ si5338: clock-generator@70 {
+ compatible = "silabs,si5338";
+ reg = <0x70>;
+ /* other stuff */
+ };
+ };
+
+ i2c@1 {
+ /* data is in little endian on pcie bus */
+ reg = <0x0100>;
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ si5338: clock-generator@70 {
+ compatible = "silabs,si5338";
+ reg = <0x70>;
+ /* other stuff */
+ };
+ };
+ };
diff --git a/drivers/i2c/muxes/Kconfig b/drivers/i2c/muxes/Kconfig
index f6d313e..77c1257 100644
--- a/drivers/i2c/muxes/Kconfig
+++ b/drivers/i2c/muxes/Kconfig
@@ -29,6 +29,17 @@ config I2C_MUX_GPIO
This driver can also be built as a module. If so, the module
will be called i2c-mux-gpio.
+config I2C_MUX_REG
+ tristate "Register-based I2C multiplexer"
+ help
+ If you say yes to this option, support will be included for a
+ register based I2C multiplexer. This driver provides access to
+ I2C busses connected through a MUX, which is controlled
+ by a sinple register.
+
+ This driver can also be built as a module. If so, the module
+ will be called i2c-mux-reg.
+
config I2C_MUX_PCA9541
tristate "NXP PCA9541 I2C Master Selector"
help
diff --git a/drivers/i2c/muxes/Makefile b/drivers/i2c/muxes/Makefile
index 465778b..bc517bb 100644
--- a/drivers/i2c/muxes/Makefile
+++ b/drivers/i2c/muxes/Makefile
@@ -4,6 +4,7 @@
obj-$(CONFIG_I2C_ARB_GPIO_CHALLENGE) += i2c-arb-gpio-challenge.o
obj-
Re: [PATCH] driver/i2c/mux: Add register based mux i2c-mux-reg
On 06/17/2015 08:03 AM, Alexander Sverdlin wrote:
> Hi!
>
> On 16/06/15 19:28, ext York Sun wrote:
>> Based on i2c-mux-gpio driver, similarly the register based mux
>> switch from one bus to another by setting a single register.
>> The register can be on PCIe bus, local bus, or any memory-mapped
>> address.
>>
>> Signed-off-by: York Sun
>> CC: Wolfram Sang
>> CC: Peter Korsgaard
>> ---
>> .../devicetree/bindings/i2c/i2c-mux-reg.txt| 69 ++
>> drivers/i2c/muxes/Kconfig | 11 +
>> drivers/i2c/muxes/Makefile |1 +
>> drivers/i2c/muxes/i2c-mux-reg.c| 239
>>
>> drivers/i2c/muxes/i2c-mux-reg.h| 38
>> 5 files changed, 358 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/i2c/i2c-mux-reg.txt
>> create mode 100644 drivers/i2c/muxes/i2c-mux-reg.c
>> create mode 100644 drivers/i2c/muxes/i2c-mux-reg.h
>>
>> diff --git a/Documentation/devicetree/bindings/i2c/i2c-mux-reg.txt
>> b/Documentation/devicetree/bindings/i2c/i2c-mux-reg.txt
>> new file mode 100644
>> index 000..ad7cc4f
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/i2c/i2c-mux-reg.txt
>> @@ -0,0 +1,69 @@
>> +Register-based I2C Bus Mux
>> +
>> +This binding describes an I2C bus multiplexer that uses a single regsiter
>
Nice catch.
>
>> +to route the I2C signals.
>> +
>> +Required properties:
>> +- compatible: i2c-mux-reg
>> +- i2c-parent: The phandle of the I2C bus that this multiplexer's master-side
>> + port is connected to.
>> +* Standard I2C mux properties. See mux.txt in this directory.
>> +* I2C child bus nodes. See mux.txt in this directory.
>> +
>> +Optional properties:
>> +- reg: this pair of specifies the register to control the mux.
>> + The depends on its parent node. It can be any memory-mapped
>> + address. If omitted, the resource of this device will be used.
>> +- idle-state: value to set the muxer to when idle. When no value is
>> + given, it defaults to the last value used.
>> +
>> +For each i2c child node, an I2C child bus will be created. They will
>> +be numbered based on their order in the device tree.
>> +
>> +Whenever an access is made to a device on a child bus, the value set
>> +in the revelant node's reg property will be output to the register.
>> +
>> +If an idle state is defined, using the idle-state (optional) property,
>> +whenever an access is not being made to a device on a child bus, the
>> +register will be set according to the idle value.
>> +
>> +If an idle state is not defined, the most recently used value will be
>> +left programmed into the register.
>> +
>> +Example of a mux on PCIe card, the host is a powerpc SoC (big endian):
>> +
>> +i2c-mux {
>> +/* the depends on the address translation
>> + * of the parent device. If omitted, device resource
>> + * will be used instead.
>> + */
>> +reg = <0x6028 0x4>;
>> +compatible = "i2c-mux-reg";
>> +#address-cells = <1>;
>> +#size-cells = <0>;
>> +i2c-parent = <&i2c1>;
>> +i2c@0 {
>> +reg = <0>;
>> +#address-cells = <1>;
>> +#size-cells = <0>;
>> +
>> +si5338: clock-generator@70 {
>> +compatible = "silabs,si5338";
>> +reg = <0x70>;
>> +/* other stuff */
>> +};
>> +};
>> +
>> +i2c@1 {
>> +/* data is in little endian on pcie bus */
>> +reg = <0x0100>;
>> +#address-cells = <1>;
>> +#size-cells = <0>;
>> +
>> +si5338: clock-generator@70 {
>> +compatible = "silabs,si5338";
>> +reg = <0x70>;
>> +/* other stuff */
>> +};
>> +};
>> +};
>> diff --git a/drivers/i2c/muxes/Kconfig b/drivers/i2c/muxes/Kconfig
>> index f6d313e..77c1257 100644
>> --- a/drivers/i2
Re: [Patch v3] driver/clk/clk-si5338: Add common clock framework driver for si5338
On 06/17/2015 02:29 AM, Paul Bolle wrote: > On Tue, 2015-06-16 at 09:31 -0700, York Sun wrote: >> COMMON_CLK in Kconfig is changed from bool to tristate so all common >> clock framework drivers can be selected by users. > > A bool to tristate change isn't needed to make it possible to set a > symbol manually. That's achieved by adding a prompt (which the patch > also does). > > This change adds a prompt to the symbol that enables the framework. But, > as far as I can see, clock drivers depending on that framework already > can be set manually. So that's another reason the above looks incorrect > to me. > > Note that the "help" of COMMON_CLK contains: > Architectures utilizing the common struct clk should select > this option. > > Does the architecture this patch targets perhaps not select COMMON_CLK? > If that's the case, it seems you should change that architecture > instead. Paul, I did check the mechanism to select COMMON_CLK. It doesn't fit my application. But it is not in the scope of this patch. I will discuss at the end of this email. > >> --- a/drivers/clk/Kconfig >> +++ b/drivers/clk/Kconfig > >> config COMMON_CLK >> -bool >> +tristate "Common Clock" >> select HAVE_CLK_PREPARE >> select CLKDEV_LOOKUP >> select SRCU > > I told you yesterday that setting this to tristate allows over a dozen > new modules to be created. I'd be surprised if that doesn't break stuff > left and right without additional changes (which this patch lacks). Yes you did. I checked over and over and believe enabling this option doesn't break anything. By enabling it, a dozen modules are built either in the kernel, or as modules (depending on y or m). They support fix-rate clock and others. Here is what I am going to do. I will separate the Kconfig from this patch so it doesn't block this patch from going forward. I will continue exploring the correct way to enable common clock framework, for my application and for general use. York -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[Patch v4] driver/clk/clk-si5338: Add common clock framework driver for si5338
SI5338 is a programmable clock generator. It has 4 sets of inputs, PLL, multisynth and dividers to make 4 outputs. This driver splits them into multiple clocks to comply with common clock framework. See Documentation/devicetree/bindings/clock/silabs,si5338.txt for details. Export __clk_is_prepared from clk.c so drivers can check and unprepare clocks upon removal. Signed-off-by: York Sun CC: Mike Turquette CC: Sebastian Hesselbarth CC: Guenter Roeck CC: Andrey Filippov CC: Paul Bolle --- Change log: v4: Add binding silabs,pll-vco Set pll rate initial value Separate COMMON_CLK change from this patch v3: Add calling unprepare upon removal Add registering to clkdev so the clk can be acquired when device tree is not in use Add a dev_info message when driver is removed Add missing "static" to two functions in clk-si5338.c Cosmatic fix in dt-bindings.clock/clk-si5338.h v2: Fix handling name prefix if the driver is unloaded and loaded again .../devicetree/bindings/clock/silabs,si5338.txt| 178 + drivers/clk/Kconfig| 12 + drivers/clk/Makefile |1 + drivers/clk/clk-si5338.c | 3682 drivers/clk/clk-si5338.h | 305 ++ drivers/clk/clk.c |1 + include/dt-bindings/clock/clk-si5338.h | 68 + include/linux/platform_data/si5338.h | 49 + 8 files changed, 4296 insertions(+) create mode 100644 Documentation/devicetree/bindings/clock/silabs,si5338.txt create mode 100644 drivers/clk/clk-si5338.c create mode 100644 drivers/clk/clk-si5338.h create mode 100644 include/dt-bindings/clock/clk-si5338.h create mode 100644 include/linux/platform_data/si5338.h diff --git a/Documentation/devicetree/bindings/clock/silabs,si5338.txt b/Documentation/devicetree/bindings/clock/silabs,si5338.txt new file mode 100644 index 000..807d5f6 --- /dev/null +++ b/Documentation/devicetree/bindings/clock/silabs,si5338.txt @@ -0,0 +1,178 @@ +Binding for Silicon Labs Si5338 programmable i2c clock generator. + +Reference +[1] Si5338 Data Sheet +http://www.silabs.com/Support%20Documents/TechnicalDocs/Si5338.pdf + +The Si5338 is a programmable i2c clock generators with up to 4 output +clocks. It has 4 sets of possible input clocks + +IN1/IN2: differential +IN3: single-ended +IN4: single-ended +IN5/IN6: differential + +Additionally, IN1/IN2 can be used as XTAL with different setting. +The clock tree looks like below (without support of zero-delay) + + + IN1/IN2 IN3 IN4 IN5/IN6 + | | | | + --| | | | + | | | | | + | \ / \ / + | \ / \ / + | \ / \ / + XTAL REFCLKFBCLK + | | \ / | + | | \ /| + | | DIVREFCLK DIVFBCLK | + | | \ / | + | | \ / | + | | \ /| + | |PLL | + | | / | | \ | + | | / / \ \ | + | |/ / \ \ | + | | / | | \| + | | | | | || + | | MS0 MS1 MS2 MS3 | + | | | | | || + + OUT0 OUT1 OUT2 OUT3 + +The output clock can choose from any of the above clock as its source, with +exceptions: MS1 can only be used for OUT1, MS2 can only be used for OUT2, MS3 +can only be used for OUT3. + +==I2C device node== + +Required properties: +- compatible: shall be "silabs,si5338". +- reg: i2c device address, shall be 0x60, 0x61, 0x70, or 0x71 +- #clock-cells: shall be set to 1 for multiple outputs +- clocks: list of parent clocks in the order of , , , , + Note, xtal and in1/2 are mutually exclusive. Only one can be set. +- #address-cells: shall be set to 1. +- #size-cells: shall be set to 0. + +Optional properties if not set by platform driver: +- silab,ref-source: source of refclk, valid value is defined as + #define SI5338_REF_SRC_CLKIN12 0 + #define SI5338_REF_SRC_CLKIN3 1 + #define SI5338_REF_SRC_XTAL 4 +- silab,fb-source: source of fbclk, valid value is defined as + #define SI5338_FB_SRC_CLKIN42 + #define SI5338_FB_SRC_CLKIN56 3 + #define SI5338_FB_SRC_NOCLK 5 +- silabs,pll-source: source of pll, valid value is defined as + #define SI5338_PFD_IN_REF_REFCLK 0 + #define SI5338_PFD_IN_REF_FBCLK1 + #define SI5338_PFD_IN_REF_DIVREFCLK2 + #define SI5338_PFD_IN_REF_DIVFBCLK 3 + #define SI5338_PFD_IN_REF_XOCLK4 + #define SI5338_PFD_IN_REF_NOCLK5 +- silabs,pll-master: Pick one MS (0, 1, 2, or 3) to allow chaning P
[RFC] drivers/clk/Kconfig: Change COMMON_CLK to tristate
COMMON_CLK has been a bool value, selected by the platforms who need common clock framework. If a CCF driver is needed on an add-on device such as PCIe card, COMMON_CLK can be selected individually as a tristate value. Signed-off-by: York Sun CC: Paul Bolle CC: Mike Turquette --- drivers/clk/Kconfig |2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig index 32b2219..07f0b2f 100644 --- a/drivers/clk/Kconfig +++ b/drivers/clk/Kconfig @@ -10,7 +10,7 @@ config HAVE_MACH_CLKDEV bool config COMMON_CLK - bool + tristate "Common Clock" select HAVE_CLK_PREPARE select CLKDEV_LOOKUP select SRCU -- 1.7.9.5 -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC] drivers/clk/Kconfig: Change COMMON_CLK to tristate
On 06/17/2015 12:05 PM, Wolfram Sang wrote: > On Wed, Jun 17, 2015 at 12:01:47PM -0700, York Sun wrote: >> COMMON_CLK has been a bool value, selected by the platforms who need >> common clock framework. If a CCF driver is needed on an add-on device >> such as PCIe card, COMMON_CLK can be selected individually as a >> tristate value. >> >> Signed-off-by: York Sun >> CC: Paul Bolle >> CC: Mike Turquette > > Why do you CC the I2C list for clk patches?? > My apology for CC the wrong list. It was firstly introduced in this patch http://patchwork.ozlabs.org/patch/485088/. York -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[Patch v2] driver/i2c/mux: Add register based mux i2c-mux-reg
Based on i2c-mux-gpio driver, similarly the register based mux
switch from one bus to another by setting a single register.
The register can be on PCIe bus, local bus, or any memory-mapped
address.
Signed-off-by: York Sun
CC: Wolfram Sang
CC: Paul Bolle
CC: Peter Korsgaard
CC: Alexander Sverdlin
---
Change log:
v2: Update to GPLv2+ licence header
Use iowrite instead of direct dereference the pointer to write register
Add support of difference register size of 1/2/4 bytes
Remove i2c_put_adapter(parent) in probe fucntion
Replace multiple dev_info() with dev_dbg()
Add idle_in_use variable to gate using idle value
Add __iomem for register pointer
Move platform data header file to include/linux/platform_data/
.../devicetree/bindings/i2c/i2c-mux-reg.txt| 71 ++
drivers/i2c/muxes/Kconfig | 11 +
drivers/i2c/muxes/Makefile |1 +
drivers/i2c/muxes/i2c-mux-reg.c| 266
include/linux/platform_data/i2c-mux-reg.h | 40 +++
5 files changed, 389 insertions(+)
create mode 100644 Documentation/devicetree/bindings/i2c/i2c-mux-reg.txt
create mode 100644 drivers/i2c/muxes/i2c-mux-reg.c
create mode 100644 include/linux/platform_data/i2c-mux-reg.h
diff --git a/Documentation/devicetree/bindings/i2c/i2c-mux-reg.txt
b/Documentation/devicetree/bindings/i2c/i2c-mux-reg.txt
new file mode 100644
index 000..b685d6c
--- /dev/null
+++ b/Documentation/devicetree/bindings/i2c/i2c-mux-reg.txt
@@ -0,0 +1,71 @@
+Register-based I2C Bus Mux
+
+This binding describes an I2C bus multiplexer that uses a single register
+to route the I2C signals.
+
+Required properties:
+- compatible: i2c-mux-reg
+- i2c-parent: The phandle of the I2C bus that this multiplexer's master-side
+ port is connected to.
+* Standard I2C mux properties. See mux.txt in this directory.
+* I2C child bus nodes. See mux.txt in this directory.
+
+Optional properties:
+- reg: this pair of specifies the register to control the mux.
+ The depends on its parent node. It can be any memory-mapped
+ address. The size must be either 1, 2, or 4 bytes. If reg is omitted, the
+ resource of this device will be used.
+- idle-state: value to set the muxer to when idle. When no value is
+ given, it defaults to the last value used.
+
+For each i2c child node, an I2C child bus will be created. They will
+be numbered based on their order in the device tree.
+
+Whenever an access is made to a device on a child bus, the value set
+in the revelant node's reg property will be output to the register.
+
+If an idle state is defined, using the idle-state (optional) property,
+whenever an access is not being made to a device on a child bus, the
+register will be set according to the idle value.
+
+If an idle state is not defined, the most recently used value will be
+left programmed into the register.
+
+Example of a mux on PCIe card, the host is a powerpc SoC (big endian):
+
+ i2c-mux {
+ /* the depends on the address translation
+* of the parent device. If omitted, device resource
+* will be used instead. The size is to determine
+* whether iowrite32, iowrite16, or iowrite8 will be used.
+*/
+ reg = <0x6028 0x4>;
+ compatible = "i2c-mux-reg";
+ #address-cells = <1>;
+ #size-cells = <0>;
+ i2c-parent = <&i2c1>;
+ i2c@0 {
+ reg = <0>;
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ si5338: clock-generator@70 {
+ compatible = "silabs,si5338";
+ reg = <0x70>;
+ /* other stuff */
+ };
+ };
+
+ i2c@1 {
+ /* data is written using iowrite32 */
+ reg = <1>;
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ si5338: clock-generator@70 {
+ compatible = "silabs,si5338";
+ reg = <0x70>;
+ /* other stuff */
+ };
+ };
+ };
diff --git a/drivers/i2c/muxes/Kconfig b/drivers/i2c/muxes/Kconfig
index f6d313e..77c1257 100644
--- a/drivers/i2c/muxes/Kconfig
+++ b/drivers/i2c/muxes/Kconfig
@@ -29,6 +29,17 @@ config I2C_MUX_GPIO
This driver can also be built as a module. If so, the module
will be called i2c-mux-gpio.
+config I2C_MUX_REG
+ tristate "Register-based I2C multiplexer"
+ help
+ If you say yes to this option, support will be included for
Re: [Patch v2] driver/i2c/mux: Add register based mux i2c-mux-reg
On 06/18/2015 05:35 AM, Alexander Sverdlin wrote:
> Hello!
>
> On 17/06/15 23:13, ext York Sun wrote:
>> +switch (mux->data.reg_size) {
>> +case 4:
>> +iowrite32(mux->data.values[chan], mux->data.reg);
>> +break;
>> +case 2:
>> +iowrite16(mux->data.values[chan], mux->data.reg);
>> +break;
>> +case 1:
>> +iowrite8(mux->data.values[chan], mux->data.reg);
>> +break;
>
> I'd like to see at least [optional] read-back operation after each write.
Maybe I should add ioread after each write without using an option. I want to
avoid additional option if possible.
> And if you stick with iowrite*(), maybe it desires a comment (in the
> Documentation/ file?),
> that write will be little-Endian, therefore BE users must take care...
I am consulting with my colleagues. If I cannot come up with a native endianess
solution, I will keep using iowrite and add a comment.
>
> Other than that it looks good to me...
>
Thanks.
York
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[Patch v3] driver/i2c/mux: Add register-based mux i2c-mux-reg
Based on i2c-mux-gpio driver, similarly the register-based mux
switch from one bus to another by setting a single register.
The register can be on PCIe bus, local bus, or any memory-mapped
address. The endianness of such register can be specified in device
tree if used, or in platform data.
Signed-off-by: York Sun
CC: Wolfram Sang
CC: Paul Bolle
CC: Peter Korsgaard
CC: Alexander Sverdlin
---
According to Alexander's feedback, readback is added. Different sizes
are supported. I stick with iowrite but adding an option to use iowrite
big endian in in case needed. Both big- and little-endian are tested.
Comments are updated.
Change log:
v3: Add support of both big- and little-endian register
Add readback after writing to register
Add no-read option. By default, readback is alowed.
Fix using chan_id transferred back from i2c-mux. It was mistakenly
used as an index. It is actually the data to be written.
v2: Update to GPLv2+ licence header
Use iowrite instead of direct dereference the pointer to write register
Add support of difference register size of 1/2/4 bytes
Remove i2c_put_adapter(parent) in probe fucntion
Replace multiple dev_info() with dev_dbg()
Add idle_in_use variable to gate using idle value
Add __iomem for register pointer
Move platform data header file to include/linux/platform_data/
.../devicetree/bindings/i2c/i2c-mux-reg.txt| 75 +
drivers/i2c/muxes/Kconfig | 11 +
drivers/i2c/muxes/Makefile |1 +
drivers/i2c/muxes/i2c-mux-reg.c| 299
include/linux/platform_data/i2c-mux-reg.h | 44 +++
5 files changed, 430 insertions(+)
create mode 100644 Documentation/devicetree/bindings/i2c/i2c-mux-reg.txt
create mode 100644 drivers/i2c/muxes/i2c-mux-reg.c
create mode 100644 include/linux/platform_data/i2c-mux-reg.h
diff --git a/Documentation/devicetree/bindings/i2c/i2c-mux-reg.txt
b/Documentation/devicetree/bindings/i2c/i2c-mux-reg.txt
new file mode 100644
index 000..6e3197f
--- /dev/null
+++ b/Documentation/devicetree/bindings/i2c/i2c-mux-reg.txt
@@ -0,0 +1,75 @@
+Register-based I2C Bus Mux
+
+This binding describes an I2C bus multiplexer that uses a single register
+to route the I2C signals.
+
+Required properties:
+- compatible: i2c-mux-reg
+- i2c-parent: The phandle of the I2C bus that this multiplexer's master-side
+ port is connected to.
+* Standard I2C mux properties. See mux.txt in this directory.
+* I2C child bus nodes. See mux.txt in this directory.
+
+Optional properties:
+- reg: this pair of specifies the register to control the mux.
+ The depends on its parent node. It can be any memory-mapped
+ address. The size must be either 1, 2, or 4 bytes. If reg is omitted, the
+ resource of this device will be used.
+- little-endian: The existence indicates the register is in little endian.
+ If omitted, the endianness of the host will be used.
+- no-read: The existence indicates reading the register is not allowed.
+- idle-state: value to set the muxer to when idle. When no value is
+ given, it defaults to the last value used.
+
+For each i2c child node, an I2C child bus will be created. They will
+be numbered based on their order in the device tree.
+
+Whenever an access is made to a device on a child bus, the value set
+in the revelant node's reg property will be output to the register.
+
+If an idle state is defined, using the idle-state (optional) property,
+whenever an access is not being made to a device on a child bus, the
+register will be set according to the idle value.
+
+If an idle state is not defined, the most recently used value will be
+left programmed into the register.
+
+Example of a mux on PCIe card, the host is a powerpc SoC (big endian):
+
+ i2c-mux {
+ /* the depends on the address translation
+* of the parent device. If omitted, device resource
+* will be used instead. The size is to determine
+* whether iowrite32, iowrite16, or iowrite8 will be used.
+*/
+ reg = <0x6028 0x4>;
+ little-endian; /* little endian register on PCIe */
+ compatible = "i2c-mux-reg";
+ #address-cells = <1>;
+ #size-cells = <0>;
+ i2c-parent = <&i2c1>;
+ i2c@0 {
+ reg = <0>;
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ si5338: clock-generator@70 {
+ compatible = "silabs,si5338";
+ reg = <0x70>;
+ /* other stuff */
+ };
+ };
+
+ i2c@1 {
+ /* data is written using
Re: [Patch v4] driver/clk/clk-si5338: Add common clock framework driver for si5338
Dear Maintainers, Please review and advise if any change is needed. Thanks. York On 06/17/2015 11:49 AM, York Sun wrote: > SI5338 is a programmable clock generator. It has 4 sets of inputs, > PLL, multisynth and dividers to make 4 outputs. This driver splits > them into multiple clocks to comply with common clock framework. > > See Documentation/devicetree/bindings/clock/silabs,si5338.txt for > details. > > Export __clk_is_prepared from clk.c so drivers can check and unprepare > clocks upon removal. > > Signed-off-by: York Sun > CC: Mike Turquette > CC: Sebastian Hesselbarth > CC: Guenter Roeck > CC: Andrey Filippov > CC: Paul Bolle > > --- > Change log: > v4: Add binding silabs,pll-vco > Set pll rate initial value > Separate COMMON_CLK change from this patch > > v3: Add calling unprepare upon removal > Add registering to clkdev so the clk can be acquired when device > tree is not in use > Add a dev_info message when driver is removed > Add missing "static" to two functions in clk-si5338.c > Cosmatic fix in dt-bindings.clock/clk-si5338.h > > v2: Fix handling name prefix if the driver is unloaded and loaded again > > .../devicetree/bindings/clock/silabs,si5338.txt| 178 + > drivers/clk/Kconfig| 12 + > drivers/clk/Makefile |1 + > drivers/clk/clk-si5338.c | 3682 > > drivers/clk/clk-si5338.h | 305 ++ > drivers/clk/clk.c |1 + > include/dt-bindings/clock/clk-si5338.h | 68 + > include/linux/platform_data/si5338.h | 49 + > 8 files changed, 4296 insertions(+) > create mode 100644 Documentation/devicetree/bindings/clock/silabs,si5338.txt > create mode 100644 drivers/clk/clk-si5338.c > create mode 100644 drivers/clk/clk-si5338.h > create mode 100644 include/dt-bindings/clock/clk-si5338.h > create mode 100644 include/linux/platform_data/si5338.h > > diff --git a/Documentation/devicetree/bindings/clock/silabs,si5338.txt > b/Documentation/devicetree/bindings/clock/silabs,si5338.txt > new file mode 100644 > index 000..807d5f6 > --- /dev/null > +++ b/Documentation/devicetree/bindings/clock/silabs,si5338.txt > @@ -0,0 +1,178 @@ > +Binding for Silicon Labs Si5338 programmable i2c clock generator. > + > +Reference > +[1] Si5338 Data Sheet > +http://www.silabs.com/Support%20Documents/TechnicalDocs/Si5338.pdf > + > +The Si5338 is a programmable i2c clock generators with up to 4 output > +clocks. It has 4 sets of possible input clocks > + > +IN1/IN2: differential > +IN3: single-ended > +IN4: single-ended > +IN5/IN6: differential > + > +Additionally, IN1/IN2 can be used as XTAL with different setting. > +The clock tree looks like below (without support of zero-delay) > + > + > + IN1/IN2 IN3 IN4 IN5/IN6 > + | | | | > + --| | | | > + | | | | | > + | \ / \ / > + | \ / \ / > + | \ / \ / > + XTAL REFCLKFBCLK > + | | \ / | > + | | \ /| > + | | DIVREFCLK DIVFBCLK | > + | | \ / | > + | | \ / | > + | | \ /| > + | |PLL | > + | | / | | \ | > + | | / / \ \ | > + | |/ / \ \ | > + | | / | | \| > + | | | | | || > + | | MS0 MS1 MS2 MS3 | > + | | | | | || > + > + OUT0 OUT1 OUT2 OUT3 > + > +The output clock can choose from any of the above clock as its source, with > +exceptions: MS1 can only be used for OUT1, MS2 can only be used for OUT2, MS3 > +can only be used for OUT3. > + > +==I2C device node== > + > +Required properties: > +- compatible: shall be "silabs,si5338". > +- reg: i2c device address, shall be 0x60, 0x61, 0x70, or 0x71 > +- #clock-cells: shall be set to 1 for multiple outputs > +- clocks: list of parent clocks in the order of , , , > , > + Note, xtal and in1/2 are mutually exclusive. Only one can be set. > +- #address-cells: shall be set to 1. > +- #size-cells: shall be set to 0. > + > +Optional properties if not set by platform driver: > +- silab,ref-source: source of refclk, valid value is defined as > + #define SI5338_REF_SRC_CLKIN12 0 > + #define SI5338_REF_SRC_CLKIN3
Re: [Patch v3] driver/i2c/mux: Add register-based mux i2c-mux-reg
On 08/11/2015 08:39 AM, Wolfram Sang wrote:
> On Thu, Jun 18, 2015 at 12:57:38PM -0700, York Sun wrote:
>> Based on i2c-mux-gpio driver, similarly the register-based mux
>> switch from one bus to another by setting a single register.
>> The register can be on PCIe bus, local bus, or any memory-mapped
>> address. The endianness of such register can be specified in device
>> tree if used, or in platform data.
>>
>> Signed-off-by: York Sun
>
> Thanks for this driver!
>
> ...
>
>> +- no-read: The existence indicates reading the register is not allowed.
>
> Given that we have "read-only" properties already, I'd prefer this one
> to be "write-only".
Sure. That's easy to fix.
>
>> +For each i2c child node, an I2C child bus will be created. They will
>> +be numbered based on their order in the device tree.
>
> This is a Linux specific detail (which can be overridden by aliases), so
> it should not be in this document, I'd say.
OK. I can remove it.
>
>> diff --git a/drivers/i2c/muxes/Kconfig b/drivers/i2c/muxes/Kconfig
>> index f6d313e..77c1257 100644
>> --- a/drivers/i2c/muxes/Kconfig
>> +++ b/drivers/i2c/muxes/Kconfig
>> @@ -29,6 +29,17 @@ config I2C_MUX_GPIO
>>This driver can also be built as a module. If so, the module
>>will be called i2c-mux-gpio.
>>
>> +config I2C_MUX_REG
>> +tristate "Register-based I2C multiplexer"
>> +help
>> + If you say yes to this option, support will be included for a
>> + register based I2C multiplexer. This driver provides access to
>> + I2C busses connected through a MUX, which is controlled
>> + by a sinple register.
>
> Typo here. And keep the sorting, please.
Will fix.
>
>> obj-$(CONFIG_I2C_MUX_GPIO) += i2c-mux-gpio.o
>> +obj-$(CONFIG_I2C_MUX_REG) += i2c-mux-reg.o
>
> Keep the sorting, please.
>
>> obj-$(CONFIG_I2C_MUX_PCA9541) += i2c-mux-pca9541.o
>> obj-$(CONFIG_I2C_MUX_PCA954x) += i2c-mux-pca954x.o
>> obj-$(CONFIG_I2C_MUX_PINCTRL) += i2c-mux-pinctrl.o
>
>> +adapter = of_find_i2c_adapter_by_node(adapter_np);
>> +if (!adapter) {
>> +dev_err(&pdev->dev, "Cannot find parent bus\n");
>
> I don't think we should print something when deferring.
OK.
>
>> +return -EPROBE_DEFER;
>> +}
>> +mux->parent = adapter;
>> +mux->data.parent = i2c_adapter_id(adapter);
>> +put_device(&adapter->dev);
>> +
>> +mux->data.n_values = of_get_child_count(np);
>> +if (of_find_property(np, "little-endian", NULL)) {
>
> You should check for a "big-endian" property as well, no?
I use the little-endian as an option to indicate the nature of litten-endian
register. It is default to big-endian if this property doesn't exist. I prefer
this way unless you strongly suggest to add both and throw out an error if
neither exists.
>
>> +parent = i2c_get_adapter(mux->data.parent);
>> +if (!parent) {
>> +dev_err(&pdev->dev, "Parent adapter (%d) not found\n",
>> +mux->data.parent);
>> +return -EPROBE_DEFER;
>
> Ditto about printing when deferred probing.
OK.
>
>> +static int i2c_mux_reg_remove(struct platform_device *pdev)
>> +{
>> +struct regmux *mux = platform_get_drvdata(pdev);
>> +int i;
>> +
>> +for (i = 0; i < mux->data.n_values; i++)
>> +i2c_del_mux_adapter(mux->adap[i]);
>> +
>> +i2c_put_adapter(mux->parent);
>> +
>> +dev_dbg(&pdev->dev, "Removed\n");
>
> No need for that debug. The driver core has debug output for that.
Will remove.
Thanks for reviewing. I will send a new version after testing.
York
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Patch v3] driver/i2c/mux: Add register-based mux i2c-mux-reg
On 08/11/2015 09:16 AM, Wolfram Sang wrote:
+ if (of_find_property(np, "little-endian", NULL)) {
>>>
>>> You should check for a "big-endian" property as well, no?
>>
>> I use the little-endian as an option to indicate the nature of litten-endian
>> register. It is default to big-endian if this property doesn't exist. I
>> prefer
>> this way unless you strongly suggest to add both and throw out an error if
>> neither exists.
>
> I'd think that "little-endian" or "big-endian" force a setting. If none
> is present, we shall take the CPU endianess. Or am I overlooking
> something?
You are right. The current code checks for littel-endian property. If missing,
the CPU endianess is used. Do you prefer to check littlen-endian first, if
missing then big-endian, if both missing then use CPU endianess?
>
> Oh, and I forgot the biggest issue: I get build errors, because
> __LITTLE_ENDIAN__ should be __LITTLE_ENDIAN. Is this a recent change or
> why did it work for you?
>
I tested it on 4.0.4 kernel. I see a lot of reference of __LITTLE_ENDIAN__. I
will test the new patch on the latest kernel.
York
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Patch v3] driver/i2c/mux: Add register-based mux i2c-mux-reg
On 08/11/2015 01:02 PM, Wolfram Sang wrote: >>> I'd think that "little-endian" or "big-endian" force a setting. If none >>> is present, we shall take the CPU endianess. Or am I overlooking >>> something? >> >> You are right. The current code checks for littel-endian property. If >> missing, >> the CPU endianess is used. Do you prefer to check littlen-endian first, if >> missing then big-endian, if both missing then use CPU endianess? > > Yes. Do it like this. > OK. Will do. Do I have to add myself to MAINTAINER file for this driver? York -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Patch v3] driver/i2c/mux: Add register-based mux i2c-mux-reg
On 08/11/2015 06:35 PM, Wolfram Sang wrote: > >> Do I have to add myself to MAINTAINER file for this driver? > > Do you want to maintain this driver? > I prefer not, if that is OK. York -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[Patch v4] driver/i2c/mux: Add register-based mux i2c-mux-reg
Based on i2c-mux-gpio driver, similarly the register-based mux
switch from one bus to another by setting a single register.
The register can be on PCIe bus, local bus, or any memory-mapped
address. The endianness of such register can be specified in device
tree if used, or in platform data.
Signed-off-by: York Sun
CC: Wolfram Sang
CC: Paul Bolle
CC: Peter Korsgaard
CC: Alexander Sverdlin
---
Change log:
v4: Rename no-read to write-only
Revise binding document
Revise endianness checking
Reorder in Kconfig and Makefile
Remove print for deferred probing
Remove debug print for unloading driver
v3: Add support of both big- and little-endian register
Add readback after writing to register
Add no-read option. By default, readback is alowed.
Fix using chan_id transferred back from i2c-mux. It was mistakenly
used as an index. It is actually the data to be written.
v2: Update to GPLv2+ licence header
Use iowrite instead of direct dereference the pointer to write register
Add support of difference register size of 1/2/4 bytes
Remove i2c_put_adapter(parent) in probe fucntion
Replace multiple dev_info() with dev_dbg()
Add idle_in_use variable to gate using idle value
Add __iomem for register pointer
Move platform data header file to include/linux/platform_data/
.../devicetree/bindings/i2c/i2c-mux-reg.txt| 74 +
drivers/i2c/muxes/Kconfig | 11 +
drivers/i2c/muxes/Makefile |1 +
drivers/i2c/muxes/i2c-mux-reg.c| 298
include/linux/platform_data/i2c-mux-reg.h | 44 +++
5 files changed, 428 insertions(+)
create mode 100644 Documentation/devicetree/bindings/i2c/i2c-mux-reg.txt
create mode 100644 drivers/i2c/muxes/i2c-mux-reg.c
create mode 100644 include/linux/platform_data/i2c-mux-reg.h
diff --git a/Documentation/devicetree/bindings/i2c/i2c-mux-reg.txt
b/Documentation/devicetree/bindings/i2c/i2c-mux-reg.txt
new file mode 100644
index 000..ecc5323
--- /dev/null
+++ b/Documentation/devicetree/bindings/i2c/i2c-mux-reg.txt
@@ -0,0 +1,74 @@
+Register-based I2C Bus Mux
+
+This binding describes an I2C bus multiplexer that uses a single register
+to route the I2C signals.
+
+Required properties:
+- compatible: i2c-mux-reg
+- i2c-parent: The phandle of the I2C bus that this multiplexer's master-side
+ port is connected to.
+* Standard I2C mux properties. See mux.txt in this directory.
+* I2C child bus nodes. See mux.txt in this directory.
+
+Optional properties:
+- reg: this pair of specifies the register to control the mux.
+ The depends on its parent node. It can be any memory-mapped
+ address. The size must be either 1, 2, or 4 bytes. If reg is omitted, the
+ resource of this device will be used.
+- little-endian: The existence indicates the register is in little endian.
+- big-endian: The existence indicates the register is in big endian.
+ If both little-endian and big-endian are omitted, the endianness of the
+ CPU will be used.
+- write-only: The existence indicates reading the register is write-only.
+- idle-state: value to set the muxer to when idle. When no value is
+ given, it defaults to the last value used.
+
+Whenever an access is made to a device on a child bus, the value set
+in the revelant node's reg property will be output to the register.
+
+If an idle state is defined, using the idle-state (optional) property,
+whenever an access is not being made to a device on a child bus, the
+register will be set according to the idle value.
+
+If an idle state is not defined, the most recently used value will be
+left programmed into the register.
+
+Example of a mux on PCIe card, the host is a powerpc SoC (big endian):
+
+ i2c-mux {
+ /* the depends on the address translation
+* of the parent device. If omitted, device resource
+* will be used instead. The size is to determine
+* whether iowrite32, iowrite16, or iowrite8 will be used.
+*/
+ reg = <0x6028 0x4>;
+ little-endian; /* little endian register on PCIe */
+ compatible = "i2c-mux-reg";
+ #address-cells = <1>;
+ #size-cells = <0>;
+ i2c-parent = <&i2c1>;
+ i2c@0 {
+ reg = <0>;
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ si5338: clock-generator@70 {
+ compatible = "silabs,si5338";
+ reg = <0x70>;
+ /* other stuff */
+ };
+ };
+
+ i2c@1 {
+ /* data is written using iowrite32
Re: [PATCH] driver/i2c/mux: Add register based mux i2c-mux-reg
On 08/15/2015 01:23 PM, Wolfram Sang wrote:
> On Tue, Jun 16, 2015 at 10:28:12AM -0700, York Sun wrote:
>> Based on i2c-mux-gpio driver, similarly the register based mux
>> switch from one bus to another by setting a single register.
>> The register can be on PCIe bus, local bus, or any memory-mapped
>> address.
>>
>> Signed-off-by: York Sun
>> CC: Wolfram Sang
>> CC: Peter Korsgaard
>
> Mostly good.
>
>> +static int i2c_mux_reg_probe(struct platform_device *pdev)
>> +{
>> +struct regmux *mux;
>> +struct i2c_adapter *parent;
>> +struct resource *res;
>> +int (*deselect)(struct i2c_adapter *, void *, u32);
>> +unsigned int initial_state, class;
>
> gcc says:
>
> drivers/i2c/muxes/i2c-mux-reg.c:182:15: warning: variable 'initial_state' set
> but not used [-Wunused-but-set-variable]
>
> It seens you prepared for setting the initial state but don't do the
> actual set?
Thanks for catching this. I set the initial state variable but used another
variable when it is actually used. Kernel Makefile disables
unused-but-set-variable by default. How did you enable this warning without
being flooded by the warnings? (I tried W=1)
>
>> +static struct platform_driver i2c_mux_reg_driver = {
>> +.probe = i2c_mux_reg_probe,
>> +.remove = i2c_mux_reg_remove,
>> +.driver = {
>> +.owner = THIS_MODULE,
>
> coccicheck says:
>
> drivers/i2c/muxes/i2c-mux-reg.c:288:3-8: No need to set .owner here. The core
> will do it.
Will drop it in next version.
York
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[Patch v5] driver/i2c/mux: Add register-based mux i2c-mux-reg
Based on i2c-mux-gpio driver, similarly the register-based mux
switch from one bus to another by setting a single register.
The register can be on PCIe bus, local bus, or any memory-mapped
address. The endianness of such register can be specified in device
tree if used, or in platform data.
Signed-off-by: York Sun
CC: Wolfram Sang
CC: Paul Bolle
CC: Peter Korsgaard
CC: Alexander Sverdlin
---
Change log:
v5: Remove variable initial_state
Remove .owner = THIS_MODULE
v4: Rename no-read to write-only
Revise binding document
Revise endianness checking
Reorder in Kconfig and Makefile
Remove print for deferred probing
Remove debug print for unloading driver
v3: Add support of both big- and little-endian register
Add readback after writing to register
Add no-read option. By default, readback is alowed.
Fix using chan_id transferred back from i2c-mux. It was mistakenly
used as an index. It is actually the data to be written.
v2: Update to GPLv2+ licence header
Use iowrite instead of direct dereference the pointer to write register
Add support of difference register size of 1/2/4 bytes
Remove i2c_put_adapter(parent) in probe fucntion
Replace multiple dev_info() with dev_dbg()
Add idle_in_use variable to gate using idle value
Add __iomem for register pointer
Move platform data header file to include/linux/platform_data/
.../devicetree/bindings/i2c/i2c-mux-reg.txt| 74 +
drivers/i2c/muxes/Kconfig | 11 +
drivers/i2c/muxes/Makefile |1 +
drivers/i2c/muxes/i2c-mux-reg.c| 294
include/linux/platform_data/i2c-mux-reg.h | 44 +++
5 files changed, 424 insertions(+)
create mode 100644 Documentation/devicetree/bindings/i2c/i2c-mux-reg.txt
create mode 100644 drivers/i2c/muxes/i2c-mux-reg.c
create mode 100644 include/linux/platform_data/i2c-mux-reg.h
diff --git a/Documentation/devicetree/bindings/i2c/i2c-mux-reg.txt
b/Documentation/devicetree/bindings/i2c/i2c-mux-reg.txt
new file mode 100644
index 000..ecc5323
--- /dev/null
+++ b/Documentation/devicetree/bindings/i2c/i2c-mux-reg.txt
@@ -0,0 +1,74 @@
+Register-based I2C Bus Mux
+
+This binding describes an I2C bus multiplexer that uses a single register
+to route the I2C signals.
+
+Required properties:
+- compatible: i2c-mux-reg
+- i2c-parent: The phandle of the I2C bus that this multiplexer's master-side
+ port is connected to.
+* Standard I2C mux properties. See mux.txt in this directory.
+* I2C child bus nodes. See mux.txt in this directory.
+
+Optional properties:
+- reg: this pair of specifies the register to control the mux.
+ The depends on its parent node. It can be any memory-mapped
+ address. The size must be either 1, 2, or 4 bytes. If reg is omitted, the
+ resource of this device will be used.
+- little-endian: The existence indicates the register is in little endian.
+- big-endian: The existence indicates the register is in big endian.
+ If both little-endian and big-endian are omitted, the endianness of the
+ CPU will be used.
+- write-only: The existence indicates the register is write-only.
+- idle-state: value to set the muxer to when idle. When no value is
+ given, it defaults to the last value used.
+
+Whenever an access is made to a device on a child bus, the value set
+in the revelant node's reg property will be output to the register.
+
+If an idle state is defined, using the idle-state (optional) property,
+whenever an access is not being made to a device on a child bus, the
+register will be set according to the idle value.
+
+If an idle state is not defined, the most recently used value will be
+left programmed into the register.
+
+Example of a mux on PCIe card, the host is a powerpc SoC (big endian):
+
+ i2c-mux {
+ /* the depends on the address translation
+* of the parent device. If omitted, device resource
+* will be used instead. The size is to determine
+* whether iowrite32, iowrite16, or iowrite8 will be used.
+*/
+ reg = <0x6028 0x4>;
+ little-endian; /* little endian register on PCIe */
+ compatible = "i2c-mux-reg";
+ #address-cells = <1>;
+ #size-cells = <0>;
+ i2c-parent = <&i2c1>;
+ i2c@0 {
+ reg = <0>;
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ si5338: clock-generator@70 {
+ compatible = "silabs,si5338";
+ reg = <0x70>;
+ /* other stuff */
+ };
+ };
+
+ i2c@1 {
+
Re: [PATCH] i2c: mux: reg: simplify register size checking
Sorry for top posting, I am out and replying using web access.
This patch looks OK. I cannot test it until earliest next Friday.
York
From: Wolfram Sang
Sent: Thursday, August 20, 2015 2:40 PM
To: linux-i2c@vger.kernel.org
Cc: Wolfram Sang; Sun York-R58495
Subject: [PATCH] i2c: mux: reg: simplify register size checking
Checking was done at three different locations, just do it once and
properly at probing time.
Signed-off-by: Wolfram Sang
Cc: York Sun
---
York Sun: Can you test this patch? I can only build test.
drivers/i2c/muxes/i2c-mux-reg.c | 17 ++---
1 file changed, 6 insertions(+), 11 deletions(-)
diff --git a/drivers/i2c/muxes/i2c-mux-reg.c b/drivers/i2c/muxes/i2c-mux-reg.c
index 86d41d36a78340..da8926d816c8cc 100644
--- a/drivers/i2c/muxes/i2c-mux-reg.c
+++ b/drivers/i2c/muxes/i2c-mux-reg.c
@@ -59,9 +59,6 @@ static int i2c_mux_reg_set(const struct regmux *mux, unsigned
int chan_id)
if (!mux->data.write_only)
ioread8(mux->data.reg);
break;
- default:
- pr_err("Invalid register size\n");
- return -EINVAL;
}
return 0;
@@ -154,10 +151,6 @@ static int i2c_mux_reg_probe_dt(struct regmux *mux,
/* map address from "reg" if exists */
if (of_address_to_resource(np, 0, &res)) {
mux->data.reg_size = resource_size(&res);
- if (mux->data.reg_size > 4) {
- dev_err(&pdev->dev, "Invalid address size\n");
- return -EINVAL;
- }
mux->data.reg = devm_ioremap_resource(&pdev->dev, &res);
if (IS_ERR(mux->data.reg))
return PTR_ERR(mux->data.reg);
@@ -210,15 +203,17 @@ static int i2c_mux_reg_probe(struct platform_device *pdev)
"Register not set, using platform resource\n");
res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
mux->data.reg_size = resource_size(res);
- if (mux->data.reg_size > 4) {
- dev_err(&pdev->dev, "Invalid resource size\n");
- return -EINVAL;
- }
mux->data.reg = devm_ioremap_resource(&pdev->dev, res);
if (IS_ERR(mux->data.reg))
return PTR_ERR(mux->data.reg);
}
+ if (mux->data.reg_size != 4 && mux->data.reg_size != 2 &&
+ mux->data.reg_size != 1) {
+ dev_err(&pdev->dev, "Invalid register size\n");
+ return -EINVAL;
+ }
+
mux->adap = devm_kzalloc(&pdev->dev,
sizeof(*mux->adap) * mux->data.n_values,
GFP_KERNEL);
--
2.1.4
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] i2c: mux: reg: simplify register size checking
On 08/31/2015 03:24 PM, Wolfram Sang wrote: > On Fri, Aug 21, 2015 at 03:26:23AM +0000, York Sun wrote: >> Sorry for top posting, I am out and replying using web access. >> >> This patch looks OK. I cannot test it until earliest next Friday. > > Did you have the chance to test it? > I am working on it. My hardware was replaced with a new version. I am running my driver now. Will try your patch and confirm. York -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] i2c: mux: reg: simplify register size checking
On 08/20/2015 04:40 PM, Wolfram Sang wrote: > Checking was done at three different locations, just do it once and > properly at probing time. > > Signed-off-by: Wolfram Sang > Cc: York Sun > --- > > York Sun: Can you test this patch? I can only build test. Patch is OK. Verified on hardware with good and intentional bad values. Acked-by: York Sun -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] drivers/i2c/muxes/i2c-mux-reg: Change ioread endianness for readback
Reading the register (if allowed) after writing is to ensure writing
is completed on a posted bus. The endianness of reading doesn't matter.
Signed-off-by: York Sun
---
drivers/i2c/muxes/i2c-mux-reg.c | 28 ++--
1 file changed, 14 insertions(+), 14 deletions(-)
diff --git a/drivers/i2c/muxes/i2c-mux-reg.c b/drivers/i2c/muxes/i2c-mux-reg.c
index 86d41d36..7f2a1ae 100644
--- a/drivers/i2c/muxes/i2c-mux-reg.c
+++ b/drivers/i2c/muxes/i2c-mux-reg.c
@@ -31,28 +31,28 @@ static int i2c_mux_reg_set(const struct regmux *mux,
unsigned int chan_id)
if (!mux->data.reg)
return -EINVAL;
+ /*
+* Write to the register, followed by a read to ensure the write is
+* completed on a "posted" bus, for example PCI or write buffers.
+* The endianness of reading doesn't matter and the return data
+* is not used.
+*/
switch (mux->data.reg_size) {
case 4:
- if (mux->data.little_endian) {
+ if (mux->data.little_endian)
iowrite32(chan_id, mux->data.reg);
- if (!mux->data.write_only)
- ioread32(mux->data.reg);
- } else {
+ else
iowrite32be(chan_id, mux->data.reg);
- if (!mux->data.write_only)
- ioread32(mux->data.reg);
- }
+ if (!mux->data.write_only)
+ ioread32(mux->data.reg);
break;
case 2:
- if (mux->data.little_endian) {
+ if (mux->data.little_endian)
iowrite16(chan_id, mux->data.reg);
- if (!mux->data.write_only)
- ioread16(mux->data.reg);
- } else {
+ else
iowrite16be(chan_id, mux->data.reg);
- if (!mux->data.write_only)
- ioread16be(mux->data.reg);
- }
+ if (!mux->data.write_only)
+ ioread16(mux->data.reg);
break;
case 1:
iowrite8(chan_id, mux->data.reg);
--
1.7.9.5
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
multiple i2c-ocores adapters
Peter, I have a platform (FPGA) with multiple ocores i2c adapter. When I register them using MFD framework, I got a message regarding duplicating name for sysfs. I wonder if this driver (i2c-ocores.c) only supports one adapter. I can try to fix it by adding a name string into ocores_i2c_platform_data and allocate struct i2c_adapter on demand. Am I on the right direction? York -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: multiple i2c-ocores adapters
Peter, On 10/01/2015 11:03 PM, Peter Korsgaard wrote: >>>>>> "York" == York Sun writes: > > > Peter, > > > I have a platform (FPGA) with multiple ocores i2c adapter. When I > > register them using MFD framework, I got a message regarding > > duplicating name for sysfs. I wonder if this driver (i2c-ocores.c) > > only supports one adapter. I can try to fix it by adding a name string > > into ocores_i2c_platform_data and allocate struct i2c_adapter on > > demand. Am I on the right direction? > > I guess your problem is that the driver core is complaining about > duplicate names for your platform devices (generated from the > mfd_cell). Make sure you set the .id member to something unique. > What I got was sysfs: cannot create duplicate filename '/bus/platform/devices/ocores-i2c' I think it is caused by the i2c-ocores driver. Will dig deeper. York -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] driver/i2c: Add API to add new I2C device without registering
Split existing API i2c_new_device(), i2c_new_probed_device() into
two halves. The first half creates new i2c devices and the second
half calls device_register(). It allows additional work to be done
before registering the device.
Signed-off-by: York Sun
CC: Wolfram Sang
CC: Stephen Boyd
Suggested-by: Stephen Boyd
---
drivers/i2c/i2c-core.c | 96 ++--
include/linux/i2c.h|8
2 files changed, 85 insertions(+), 19 deletions(-)
diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index 5f89f1e..ff941b8 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -980,23 +980,19 @@ static void i2c_dev_set_name(struct i2c_adapter *adap,
}
/**
- * i2c_new_device - instantiate an i2c device
+ * i2c_new_device_unregistered -- instantiate an i2c device unregistered
* @adap: the adapter managing the device
* @info: describes one I2C device; bus_num is ignored
* Context: can sleep
*
- * Create an i2c device. Binding is handled through driver model
- * probe()/remove() methods. A driver may be bound to this device when we
- * return from this function, or any later moment (e.g. maybe hotplugging will
- * load the driver module). This call is not appropriate for use by mainboard
- * initialization logic, which usually runs during an arch_initcall() long
- * before any i2c_adapter could exist.
+ * Create an i2c device but leave it unregistered.
*
* This returns the new i2c client, which may be saved for later use with
- * i2c_unregister_device(); or NULL to indicate an error.
+ * device_register() or i2c_unregister_device(); or NULL to indicate an error.
*/
struct i2c_client *
-i2c_new_device(struct i2c_adapter *adap, struct i2c_board_info const *info)
+i2c_new_device_unregistered(struct i2c_adapter *adap,
+ struct i2c_board_info const *info)
{
struct i2c_client *client;
int status;
@@ -1022,13 +1018,16 @@ i2c_new_device(struct i2c_adapter *adap, struct
i2c_board_info const *info)
if (status) {
dev_err(&adap->dev, "Invalid %d-bit I2C address 0x%02hx\n",
client->flags & I2C_CLIENT_TEN ? 10 : 7, client->addr);
- goto out_err_silent;
+ goto out_err;
}
/* Check for address business */
status = i2c_check_addr_busy(adap, i2c_encode_flags_to_addr(client));
- if (status)
+ if (status) {
+ dev_err(&adap->dev, "i2c client %s at 0x%02x busy\n",
+ client->name, client->addr);
goto out_err;
+ }
client->dev.parent = &client->adapter->dev;
client->dev.bus = &i2c_bus_type;
@@ -1037,6 +1036,41 @@ i2c_new_device(struct i2c_adapter *adap, struct
i2c_board_info const *info)
client->dev.fwnode = info->fwnode;
i2c_dev_set_name(adap, client);
+
+ return client;
+
+out_err:
+ kfree(client);
+ return NULL;
+}
+EXPORT_SYMBOL_GPL(i2c_new_device_unregistered);
+
+/**
+ * i2c_new_device - instantiate an i2c device
+ * @adap: the adapter managing the device
+ * @info: describes one I2C device; bus_num is ignored
+ * Context: can sleep
+ *
+ * Create an i2c device. Binding is handled through driver model
+ * probe()/remove() methods. A driver may be bound to this device when we
+ * return from this function, or any later moment (e.g. maybe hotplugging will
+ * load the driver module). This call is not appropriate for use by mainboard
+ * initialization logic, which usually runs during an arch_initcall() long
+ * before any i2c_adapter could exist.
+ *
+ * This returns the new i2c client, which may be saved for later use with
+ * i2c_unregister_device(); or NULL to indicate an error.
+ */
+struct i2c_client *
+i2c_new_device(struct i2c_adapter *adap, struct i2c_board_info const *info)
+{
+ struct i2c_client *client;
+ int status;
+
+ client = i2c_new_device_unregistered(adap, info);
+ if (!client)
+ goto out_err_silent;
+
status = device_register(&client->dev);
if (status)
goto out_err;
@@ -1050,7 +1084,6 @@ out_err:
dev_err(&adap->dev, "Failed to register i2c client %s at 0x%02x "
"(%d)\n", client->name, client->addr, status);
out_err_silent:
- kfree(client);
return NULL;
}
EXPORT_SYMBOL_GPL(i2c_new_device);
@@ -2455,11 +2488,9 @@ int i2c_probe_func_quick_read(struct i2c_adapter *adap,
unsigned short addr)
}
EXPORT_SYMBOL_GPL(i2c_probe_func_quick_read);
-struct i2c_client *
-i2c_new_probed_device(struct i2c_adapter *adap,
- struct i2c_board_info *info,
- unsigned short const *addr_list,
- int (*probe)(struct i2c_adapter *, unsigned short a
