| Message ID | 36f4b1736e36b039af15b13645df0de492f1b6e1.1476462204.git.lolivei@synopsys.com |
|---|---|
| State | Superseded |
| Headers | show |
On Fri, Oct 14, 2016 at 05:52:50PM +0100, Luis.Oliveira@synopsys.com wrote: > - is_slave = device_property_read_bool(&pdev->dev, "isslave"); Which tree is this based on? I cant see the existing isslave property in mainline HEAD (commit 29fbff8698fc0ac1). > +#ifndef CONFIG_ACPI > + is_slave = device_property_read_bool(&pdev->dev, "is-slave"); > +#endif This ifdef is broken. At least for arm64, a single kernel image can be booted with either ACPI or DT. We need separate accessors for DT and ACPI to handle these differently, or you need to explicitly check whether or not you have ACPI or DT at runtime. Thanks, Mark. -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Oct 14, 2016 at 06:30:15PM +0100, Mark Rutland wrote: > On Fri, Oct 14, 2016 at 05:52:50PM +0100, Luis.Oliveira@synopsys.com wrote: > > - is_slave = device_property_read_bool(&pdev->dev, "isslave"); > > Which tree is this based on? I cant see the existing isslave property in > mainline HEAD (commit 29fbff8698fc0ac1). Same surprise here. Because I likely would have NAKed the binding. Why is it needed? Doesn't the driver allow to be master/slave on the same bus?
On Fri, Oct 14, 2016 at 05:52:50PM +0100, Luis.Oliveira@synopsys.com wrote: > From: Luis Oliveira <lolivei@synopsys.com> > > Signed-off-by: Luis Oliveira <lolivei@synopsys.com> > --- > Documentation/devicetree/bindings/i2c/i2c-designware.txt | 5 ++++- > drivers/i2c/busses/i2c-designware-platdrv.c | 4 +++- > 2 files changed, 7 insertions(+), 2 deletions(-) > > diff --git a/Documentation/devicetree/bindings/i2c/i2c-designware.txt b/Documentation/devicetree/bindings/i2c/i2c-designware.txt > index fee26dc..d3d163c 100644 > --- a/Documentation/devicetree/bindings/i2c/i2c-designware.txt > +++ b/Documentation/devicetree/bindings/i2c/i2c-designware.txt > @@ -19,6 +19,8 @@ Optional properties : > > - i2c-sda-falling-time-ns : should contain the SDA falling time in nanoseconds. > This value which is by default 300ns is used to compute the tHIGH period. > + > + - is-slave : indicates if the block is a I2C slave device. This should be documented as a generic property. > > Example : > > @@ -30,7 +32,7 @@ Example : > interrupts = <11>; > clock-frequency = <400000>; > }; > - > + Adding trailing whitespace... > i2c@1120000 { > #address-cells = <1>; > #size-cells = <0>; > @@ -42,4 +44,5 @@ Example : > i2c-sda-hold-time-ns = <300>; > i2c-sda-falling-time-ns = <300>; > i2c-scl-falling-time-ns = <300>; > + is-slave; > }; -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 10/14/2016 6:30 PM, Mark Rutland wrote: > On Fri, Oct 14, 2016 at 05:52:50PM +0100, Luis.Oliveira@synopsys.com wrote: >> - is_slave = device_property_read_bool(&pdev->dev, "isslave"); > Which tree is this based on? I cant see the existing isslave property in > mainline HEAD (commit 29fbff8698fc0ac1). I'm replying for Luís, since at the moment he's not able to reply. This is a custom property since this I2C core doesn't support both master and slave at the same time. > >> +#ifndef CONFIG_ACPI >> + is_slave = device_property_read_bool(&pdev->dev, "is-slave"); >> +#endif > This ifdef is broken. At least for arm64, a single kernel image can be > booted with either ACPI or DT. We need separate accessors for DT and > ACPI to handle these differently, or you need to explicitly check > whether or not you have ACPI or DT at runtime. Thanks for the feedback, I'll take a look at this > > Thanks, > Mark. Thanks, Ramiro -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> This is needed because the configuration is different and the i2c-designware > cannot be master/slave without a reset. To resolve that I added this property > to bind it as a slave when needed. Aww, pity that the HW can't do that. Do you have details why? If that is really a HW limitation, then I'd suggest having a seperate driver for slave-only mode so we can differentiate by compatible strings.
Since practically 90% of the code is shared between master and slave, I was thinking if it will be acceptable to use the same driver for both but differentiate the master/slave mode by the compatible strings. Thanks, Luis On 10/18/2016 16:17, Wolfram Sang wrote: >> This is needed because the configuration is different and the i2c-designware >> cannot be master/slave without a reset. To resolve that I added this property >> to bind it as a slave when needed. > Aww, pity that the HW can't do that. Do you have details why? > > If that is really a HW limitation, then I'd suggest having a seperate > driver for slave-only mode so we can differentiate by compatible > strings. > -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, 2016-10-21 at 10:56 +0100, Luis Oliveira wrote: > Since practically 90% of the code is shared between master and slave, > I was > thinking if it will be acceptable to use the same driver for both but > differentiate the master/slave mode by the compatible strings. It might be possible to split like other drivers do: 1. Core part (i2c-designware-core.c) 2. Master part (i2c-designware-master.c) 3. Slave part (i2c-designware-slave.c) 4. Glue drivers (like: i2c-designware-platdrv.c) > > Thanks, > Luis > > On 10/18/2016 16:17, Wolfram Sang wrote: > > > This is needed because the configuration is different and the i2c- > > > designware > > > cannot be master/slave without a reset. To resolve that I added > > > this property > > > to bind it as a slave when needed. > > > > Aww, pity that the HW can't do that. Do you have details why? > > > > If that is really a HW limitation, then I'd suggest having a > > seperate > > driver for slave-only mode so we can differentiate by compatible > > strings. > > > >
Hi, As you suggested I will split the drivers. I am thinking of doing 5 patches: - factor out master() parts - separate Master part to i2c-designware-master.c (changes in i2c-designware-core.c) - enable Slave part to i2c-designware-slave (changes in i2c-designware-core.c) - glue drivers and device bindings - cleaning Regards, Luis On 21-Oct-16 11:54, Andy Shevchenko wrote: > On Fri, 2016-10-21 at 10:56 +0100, Luis Oliveira wrote: >> Since practically 90% of the code is shared between master and slave, >> I was >> thinking if it will be acceptable to use the same driver for both but >> differentiate the master/slave mode by the compatible strings. > It might be possible to split like other drivers do: > > 1. Core part (i2c-designware-core.c) > 2. Master part (i2c-designware-master.c) > 3. Slave part (i2c-designware-slave.c) > 4. Glue drivers (like: i2c-designware-platdrv.c) > >> Thanks, >> Luis >> >> On 10/18/2016 16:17, Wolfram Sang wrote: >>>> This is needed because the configuration is different and the i2c- >>>> designware >>>> cannot be master/slave without a reset. To resolve that I added >>>> this property >>>> to bind it as a slave when needed. >>> Aww, pity that the HW can't do that. Do you have details why? >>> >>> If that is really a HW limitation, then I'd suggest having a >>> seperate >>> driver for slave-only mode so we can differentiate by compatible >>> strings. >>> >> -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/Documentation/devicetree/bindings/i2c/i2c-designware.txt b/Documentation/devicetree/bindings/i2c/i2c-designware.txt index fee26dc..d3d163c 100644 --- a/Documentation/devicetree/bindings/i2c/i2c-designware.txt +++ b/Documentation/devicetree/bindings/i2c/i2c-designware.txt @@ -19,6 +19,8 @@ Optional properties : - i2c-sda-falling-time-ns : should contain the SDA falling time in nanoseconds. This value which is by default 300ns is used to compute the tHIGH period. + + - is-slave : indicates if the block is a I2C slave device. Example : @@ -30,7 +32,7 @@ Example : interrupts = <11>; clock-frequency = <400000>; }; - + i2c@1120000 { #address-cells = <1>; #size-cells = <0>; @@ -42,4 +44,5 @@ Example : i2c-sda-hold-time-ns = <300>; i2c-sda-falling-time-ns = <300>; i2c-scl-falling-time-ns = <300>; + is-slave; }; diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c index b9076da..f29e657 100644 --- a/drivers/i2c/busses/i2c-designware-platdrv.c +++ b/drivers/i2c/busses/i2c-designware-platdrv.c @@ -238,7 +238,9 @@ static int dw_i2c_plat_probe(struct platform_device *pdev) &dev->scl_falling_time); device_property_read_u32(&pdev->dev, "clock-frequency", &dev->clk_freq); - is_slave = device_property_read_bool(&pdev->dev, "isslave"); +#ifndef CONFIG_ACPI + is_slave = device_property_read_bool(&pdev->dev, "is-slave"); +#endif } acpi_speed = i2c_acpi_find_bus_speed(&pdev->dev);