* Re: [PATCH] i2c: i801: Register optional lis3lv02d i2c device on Dell machines
From: Pali Rohár @ 2016-12-29 14:17 UTC (permalink / raw)
To: Michał Kępień
Cc: Jean Delvare, Wolfram Sang, Steven Honeyman, Valdis.Kletnieks,
Jochen Eisinger, Gabriele Mazzotta, Andy Lutomirski,
Mario_Limonciello, Alex Hung, Takashi Iwai, linux-i2c,
linux-kernel, platform-driver-x86
In-Reply-To: <20161229134719.GA941@ozzy.nask.waw.pl>
[-- Attachment #1: Type: Text/Plain, Size: 10463 bytes --]
On Thursday 29 December 2016 14:47:19 Michał Kępień wrote:
> > On Thursday 29 December 2016 09:29:36 Michał Kępień wrote:
> > > > Dell platform team told us that some (DMI whitelisted) Dell
> > > > Latitude machines have ST microelectronics accelerometer at i2c
> > > > address 0x29. That i2c address is not specified in DMI or ACPI,
> > > > so runtime detection without whitelist which is below is not
> > > > possible.
> > > >
> > > > Presence of that ST microelectronics accelerometer is verified
> > > > by existence of SMO88xx ACPI device which represent that
> > > > accelerometer. Unfortunately without i2c address.
> > >
> > > This part of the commit message sounded a bit confusing to me at
> > > first because there is already an ACPI driver which handles
> > > SMO88xx
> > >
> > > devices (dell-smo8800). My understanding is that:
> > > * the purpose of this patch is to expose a richer interface (as
> > >
> > > provided by lis3lv02d) to these devices on some machines,
> > >
> > > * on whitelisted machines, dell-smo8800 and lis3lv02d can work
> > >
> > > simultaneously (even though dell-smo8800 effectively
> > > duplicates the work that lis3lv02d does).
> >
> > No. dell-smo8800 reads from ACPI irq number and exports
> > /dev/freefall device which notify userspace about falls. lis3lv02d
> > is i2c driver which exports axes of accelerometer. Additionaly
> > lis3lv02d can export also /dev/freefall if registerer of i2c
> > device provides irq number -- which is not case of this patch.
> >
> > So both drivers are doing different things and both are useful.
> >
> > IIRC both dell-smo8800 and lis3lv02d represent one HW device (that
> > ST microelectronics accelerometer) but due to complicated HW
> > abstraction and layers on Dell laptops it is handled by two
> > drivers, one ACPI and one i2c.
> >
> > Yes, in ideal world irq number should be passed to lis3lv02d driver
> > and that would export whole device (with /dev/freefall too), but
> > due to HW abstraction it is too much complicated...
>
> Why? AFAICT, all that is required to pass that IRQ number all the
> way down to lis3lv02d is to set the irq field of the struct
> i2c_board_info you are passing to i2c_new_device(). And you can
> extract that IRQ number e.g. in check_acpi_smo88xx_device().
> However, you would then need to make sure dell-smo8800 does not
> attempt to request the same IRQ on whitelisted machines. This got
> me thinking about a way to somehow incorporate your changes into
> dell-smo8800 using Wolfram's bus_notifier suggestion, but I do not
> have a working solution for now. What is tempting about this
> approach is that you would not have to scan the ACPI namespace in
> search of SMO88xx devices, because smo8800_add() is automatically
> called for them. However, I fear that the resulting solution may be
> more complicated than the one you submitted.
Then we need to deal with lot of problems. Order of loading .ko modules
is undefined. Binding devices to drivers registered by .ko module is
also in "random" order. At any time any of those .ko module can be
unloaded or at least device unbind (via sysfs) from driver... And there
can be some pathological situation (thanks to adding ACPI layer as Andy
pointed) that there will be more SMO88xx devices in ACPI. Plus you can
compile kernel with and without those modules and also you can blacklist
loading them (so compile time check is not enough). And still some
correct message notifier must be used.
I think such solution is much much more complicated, there are lot of
combinations of kernel configuration and available dell devices...
> > > If I got something wrong, please correct me. If I got it right,
> > > it might make sense to rephrase the commit message a bit so that
> > > the first bullet point above is immediately clear to the reader.
> > >
> > > > This patch registers lis3lv02d device at i2c address 0x29 if is
> > > > detected.
> > > >
> > > > Finally commit a7ae81952cda ("i2c: i801: Allow ACPI SystemIO
> > > > OpRegion to conflict with PCI BAR") allowed to use i2c-i801
> > > > driver on Dell machines so lis3lv02d correctly initialize
> > > > accelerometer.
> > > >
> > > > Tested on Dell Latitude E6440.
> > > >
> > > > Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
> > > > ---
> > > >
> > > > drivers/i2c/busses/i2c-i801.c | 98
> > > > +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 98
> > > > insertions(+)
> > > >
> > > > diff --git a/drivers/i2c/busses/i2c-i801.c
> > > > b/drivers/i2c/busses/i2c-i801.c index eb3627f..188cfd4 100644
> > > > --- a/drivers/i2c/busses/i2c-i801.c
> > > > +++ b/drivers/i2c/busses/i2c-i801.c
> > > > @@ -1118,6 +1118,101 @@ static void
> > > > dmi_check_onboard_devices(const struct dmi_header *dm, void
> > > > *adap)
> > > >
> > > > }
> > > >
> > > > }
> > > >
> > > > +static acpi_status check_acpi_smo88xx_device(acpi_handle
> > > > obj_handle, + u32 nesting_level,
> > > > + void *context,
> > > > + void **return_value)
> > > > +{
> > > > + struct acpi_device_info *info;
> > > > + acpi_status status;
> > > > + char *hid;
> > > > +
> > > > + status = acpi_get_object_info(obj_handle, &info);
> > >
> > > acpi_get_object_info() allocates the returned buffer, which the
> > > caller has to free.
> >
> > Ok, I will fix it in next patch iteration.
> >
> > > > + if (!ACPI_SUCCESS(status) || !(info->valid & ACPI_VALID_HID))
> > > > + return AE_OK;
> > > > +
> > > > + hid = info->hardware_id.string;
> > > > + if (!hid)
> > > > + return AE_OK;
> > > > +
> > > > + if (strlen(hid) < 7)
> > > > + return AE_OK;
> > > > +
> > > > + if (memcmp(hid, "SMO88", 5) != 0)
> > > > + return AE_OK;
> > > > +
> > > > + *((bool *)return_value) = true;
> > > > + return AE_CTRL_TERMINATE;
> > > > +}
> > > > +
> > > > +static bool is_dell_system_with_lis3lv02d(void)
> > > > +{
> > > > + bool found;
> > > > + acpi_status status;
> > > > + const char *vendor;
> > > > +
> > > > + vendor = dmi_get_system_info(DMI_SYS_VENDOR);
> > > > + if (strcmp(vendor, "Dell Inc.") != 0)
> > > > + return false;
> > > > +
> > > > + /*
> > > > + * Check if ACPI device SMO88xx exists and if is enabled.
> > > > That ACPI + * device represent our ST microelectronics
> > > > lis3lv02d accelerometer but + * unfortunately without any
> > > > other additional information. + */
> > > > + found = false;
> > > > + status = acpi_get_devices(NULL, check_acpi_smo88xx_device,
> > > > NULL, + (void **)&found);
> > > > + if (!ACPI_SUCCESS(status) || !found)
> > > > + return false;
> > > > +
> > > > + return true;
> > > > +}
> > > > +
> > > > +/*
> > > > + * Dell platform team told us that these Latitude devices have
> > > > + * ST microelectronics accelerometer at i2c address 0x29.
> > > > + * That i2c address is not specified in DMI or ACPI, so
> > > > runtime + * detection without whitelist which is below is not
> > > > possible. + */
> > > > +static const char * const dmi_dell_product_names[] = {
> > > > + "Latitude E5250",
> > > > + "Latitude E5450",
> > > > + "Latitude E5550",
> > > > + "Latitude E6440",
> > > > + "Latitude E6440 ATG",
> > > > + "Latitude E6540",
> > > > +};
> > > > +
> > > > +static void register_dell_lis3lv02d_i2c_device(struct
> > > > i801_priv *priv) +{
> > > > + struct i2c_board_info info;
> > > > + const char *product_name;
> > > > + bool known_i2c_address;
> > > > + int i;
> > > > +
> > > > + known_i2c_address = false;
> > > > + product_name = dmi_get_system_info(DMI_PRODUCT_NAME);
> > > > + for (i = 0; i < ARRAY_SIZE(dmi_dell_product_names); ++i) {
> > > > + if (strcmp(product_name, dmi_dell_product_names[i]) == 0)
> > > > {
> > > > + known_i2c_address = true;
> > > > + break;
> > > > + }
> > > > + }
> > > > +
> > > > + if (!known_i2c_address) {
> > > > + dev_warn(&priv->pci_dev->dev,
> > > > + "Accelerometer lis3lv02d i2c device is present "
> > > > + "but its i2c address is unknown, skipping ...\n");
> > >
> > > You are probably well aware of this, but checkpatch prefers
> > > keeping long log messages in one line. I am pointing it out
> > > just in case.
> >
> > Yes, but I do not know how to fix it. Splitting message into two
> > lines generates warning. Having long line generates warning too.
>
> Weird, checkpatch does not protest on my machine when the log message
> is written on a single line...
I hope that i2c maintainers decide how to format that line.
> > > > + return;
> > > > + }
> > > > +
> > > > + memset(&info, 0, sizeof(struct i2c_board_info));
> > >
> > > How about just doing "struct i2c_board_info info = { 0 };"
> > > instead?
> >
> > Ok.
> >
> > > > + info.addr = 0x29;
> > > > + strlcpy(info.type, "lis3lv02d", I2C_NAME_SIZE);
> > > > + i2c_new_device(&priv->adapter, &info);
> > > > +}
> > > > +
> > > >
> > > > /* Register optional slaves */
> > > > static void i801_probe_optional_slaves(struct i801_priv *priv)
> > > > {
> > > >
> > > > @@ -1136,6 +1231,9 @@ static void
> > > > i801_probe_optional_slaves(struct i801_priv *priv)
> > > >
> > > > if (dmi_name_in_vendors("FUJITSU"))
> > > >
> > > > dmi_walk(dmi_check_onboard_devices, &priv->adapter);
> > > >
> > > > +
> > > > + if (is_dell_system_with_lis3lv02d())
> > > > + register_dell_lis3lv02d_i2c_device(priv);
> > > >
> > > > }
> > > > #else
> > > > static void __init input_apanel_init(void) {}
> > >
> > > I tested this patch on a Vostro V131, which is not on the
> > > whitelist, so all I got was the warning message, but to this
> > > extent, it works for me.
> >
> > Hm... That means your notebook has ST microelectronics
> > accelerometer too. You could try to find it on i2c-i801 bus with
> > userspace i2cdetect program (part of i2c-tools) and get i2c
> > address.
>
> Bingo, it is at 0x1d. I modified your patch to set the i2c address
> to 0x1d and at least free fall detection seems to be working
> correctly.
lis3lv02d exports input device, you should find its number lsinput. You
can then test accelerometer with e.g. program input-events.
If it is working fine, I can add your machine to whitelist with i2c
address 0x1d.
--
Pali Rohár
pali.rohar@gmail.com
[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]
^ permalink raw reply
* Re: [PATCH] i2c: i801: Register optional lis3lv02d i2c device on Dell machines
From: Michał Kępień @ 2016-12-29 13:47 UTC (permalink / raw)
To: Pali Rohár
Cc: Jean Delvare, Wolfram Sang, Steven Honeyman, Valdis.Kletnieks,
Jochen Eisinger, Gabriele Mazzotta, Andy Lutomirski,
Mario_Limonciello, Alex Hung, Takashi Iwai, linux-i2c,
linux-kernel, platform-driver-x86
In-Reply-To: <201612291000.58489@pali>
> On Thursday 29 December 2016 09:29:36 Michał Kępień wrote:
> > > Dell platform team told us that some (DMI whitelisted) Dell
> > > Latitude machines have ST microelectronics accelerometer at i2c
> > > address 0x29. That i2c address is not specified in DMI or ACPI, so
> > > runtime detection without whitelist which is below is not
> > > possible.
> > >
> > > Presence of that ST microelectronics accelerometer is verified by
> > > existence of SMO88xx ACPI device which represent that
> > > accelerometer. Unfortunately without i2c address.
> >
> > This part of the commit message sounded a bit confusing to me at
> > first because there is already an ACPI driver which handles SMO88xx
> > devices (dell-smo8800). My understanding is that:
> >
> > * the purpose of this patch is to expose a richer interface (as
> > provided by lis3lv02d) to these devices on some machines,
> >
> > * on whitelisted machines, dell-smo8800 and lis3lv02d can work
> > simultaneously (even though dell-smo8800 effectively duplicates
> > the work that lis3lv02d does).
>
> No. dell-smo8800 reads from ACPI irq number and exports /dev/freefall
> device which notify userspace about falls. lis3lv02d is i2c driver which
> exports axes of accelerometer. Additionaly lis3lv02d can export also
> /dev/freefall if registerer of i2c device provides irq number -- which
> is not case of this patch.
>
> So both drivers are doing different things and both are useful.
>
> IIRC both dell-smo8800 and lis3lv02d represent one HW device (that ST
> microelectronics accelerometer) but due to complicated HW abstraction
> and layers on Dell laptops it is handled by two drivers, one ACPI and
> one i2c.
>
> Yes, in ideal world irq number should be passed to lis3lv02d driver and
> that would export whole device (with /dev/freefall too), but due to HW
> abstraction it is too much complicated...
Why? AFAICT, all that is required to pass that IRQ number all the way
down to lis3lv02d is to set the irq field of the struct i2c_board_info
you are passing to i2c_new_device(). And you can extract that IRQ
number e.g. in check_acpi_smo88xx_device(). However, you would then
need to make sure dell-smo8800 does not attempt to request the same IRQ
on whitelisted machines. This got me thinking about a way to somehow
incorporate your changes into dell-smo8800 using Wolfram's bus_notifier
suggestion, but I do not have a working solution for now. What is
tempting about this approach is that you would not have to scan the ACPI
namespace in search of SMO88xx devices, because smo8800_add() is
automatically called for them. However, I fear that the resulting
solution may be more complicated than the one you submitted.
>
> > If I got something wrong, please correct me. If I got it right, it
> > might make sense to rephrase the commit message a bit so that the
> > first bullet point above is immediately clear to the reader.
> >
> > > This patch registers lis3lv02d device at i2c address 0x29 if is
> > > detected.
> > >
> > > Finally commit a7ae81952cda ("i2c: i801: Allow ACPI SystemIO
> > > OpRegion to conflict with PCI BAR") allowed to use i2c-i801 driver
> > > on Dell machines so lis3lv02d correctly initialize accelerometer.
> > >
> > > Tested on Dell Latitude E6440.
> > >
> > > Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
> > > ---
> > >
> > > drivers/i2c/busses/i2c-i801.c | 98
> > > +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 98
> > > insertions(+)
> > >
> > > diff --git a/drivers/i2c/busses/i2c-i801.c
> > > b/drivers/i2c/busses/i2c-i801.c index eb3627f..188cfd4 100644
> > > --- a/drivers/i2c/busses/i2c-i801.c
> > > +++ b/drivers/i2c/busses/i2c-i801.c
> > > @@ -1118,6 +1118,101 @@ static void dmi_check_onboard_devices(const
> > > struct dmi_header *dm, void *adap)
> > >
> > > }
> > >
> > > }
> > >
> > > +static acpi_status check_acpi_smo88xx_device(acpi_handle
> > > obj_handle, + u32 nesting_level,
> > > + void *context,
> > > + void **return_value)
> > > +{
> > > + struct acpi_device_info *info;
> > > + acpi_status status;
> > > + char *hid;
> > > +
> > > + status = acpi_get_object_info(obj_handle, &info);
> >
> > acpi_get_object_info() allocates the returned buffer, which the
> > caller has to free.
>
> Ok, I will fix it in next patch iteration.
>
> > > + if (!ACPI_SUCCESS(status) || !(info->valid & ACPI_VALID_HID))
> > > + return AE_OK;
> > > +
> > > + hid = info->hardware_id.string;
> > > + if (!hid)
> > > + return AE_OK;
> > > +
> > > + if (strlen(hid) < 7)
> > > + return AE_OK;
> > > +
> > > + if (memcmp(hid, "SMO88", 5) != 0)
> > > + return AE_OK;
> > > +
> > > + *((bool *)return_value) = true;
> > > + return AE_CTRL_TERMINATE;
> > > +}
> > > +
> > > +static bool is_dell_system_with_lis3lv02d(void)
> > > +{
> > > + bool found;
> > > + acpi_status status;
> > > + const char *vendor;
> > > +
> > > + vendor = dmi_get_system_info(DMI_SYS_VENDOR);
> > > + if (strcmp(vendor, "Dell Inc.") != 0)
> > > + return false;
> > > +
> > > + /*
> > > + * Check if ACPI device SMO88xx exists and if is enabled. That
> > > ACPI + * device represent our ST microelectronics lis3lv02d
> > > accelerometer but + * unfortunately without any other additional
> > > information. + */
> > > + found = false;
> > > + status = acpi_get_devices(NULL, check_acpi_smo88xx_device, NULL,
> > > + (void **)&found);
> > > + if (!ACPI_SUCCESS(status) || !found)
> > > + return false;
> > > +
> > > + return true;
> > > +}
> > > +
> > > +/*
> > > + * Dell platform team told us that these Latitude devices have
> > > + * ST microelectronics accelerometer at i2c address 0x29.
> > > + * That i2c address is not specified in DMI or ACPI, so runtime
> > > + * detection without whitelist which is below is not possible.
> > > + */
> > > +static const char * const dmi_dell_product_names[] = {
> > > + "Latitude E5250",
> > > + "Latitude E5450",
> > > + "Latitude E5550",
> > > + "Latitude E6440",
> > > + "Latitude E6440 ATG",
> > > + "Latitude E6540",
> > > +};
> > > +
> > > +static void register_dell_lis3lv02d_i2c_device(struct i801_priv
> > > *priv) +{
> > > + struct i2c_board_info info;
> > > + const char *product_name;
> > > + bool known_i2c_address;
> > > + int i;
> > > +
> > > + known_i2c_address = false;
> > > + product_name = dmi_get_system_info(DMI_PRODUCT_NAME);
> > > + for (i = 0; i < ARRAY_SIZE(dmi_dell_product_names); ++i) {
> > > + if (strcmp(product_name, dmi_dell_product_names[i]) == 0) {
> > > + known_i2c_address = true;
> > > + break;
> > > + }
> > > + }
> > > +
> > > + if (!known_i2c_address) {
> > > + dev_warn(&priv->pci_dev->dev,
> > > + "Accelerometer lis3lv02d i2c device is present "
> > > + "but its i2c address is unknown, skipping ...\n");
> >
> > You are probably well aware of this, but checkpatch prefers keeping
> > long log messages in one line. I am pointing it out just in case.
>
> Yes, but I do not know how to fix it. Splitting message into two lines
> generates warning. Having long line generates warning too.
Weird, checkpatch does not protest on my machine when the log message is
written on a single line...
>
> > > + return;
> > > + }
> > > +
> > > + memset(&info, 0, sizeof(struct i2c_board_info));
> >
> > How about just doing "struct i2c_board_info info = { 0 };" instead?
>
> Ok.
>
> > > + info.addr = 0x29;
> > > + strlcpy(info.type, "lis3lv02d", I2C_NAME_SIZE);
> > > + i2c_new_device(&priv->adapter, &info);
> > > +}
> > > +
> > >
> > > /* Register optional slaves */
> > > static void i801_probe_optional_slaves(struct i801_priv *priv)
> > > {
> > >
> > > @@ -1136,6 +1231,9 @@ static void i801_probe_optional_slaves(struct
> > > i801_priv *priv)
> > >
> > > if (dmi_name_in_vendors("FUJITSU"))
> > >
> > > dmi_walk(dmi_check_onboard_devices, &priv->adapter);
> > >
> > > +
> > > + if (is_dell_system_with_lis3lv02d())
> > > + register_dell_lis3lv02d_i2c_device(priv);
> > >
> > > }
> > > #else
> > > static void __init input_apanel_init(void) {}
> >
> > I tested this patch on a Vostro V131, which is not on the whitelist,
> > so all I got was the warning message, but to this extent, it works
> > for me.
>
> Hm... That means your notebook has ST microelectronics accelerometer
> too. You could try to find it on i2c-i801 bus with userspace i2cdetect
> program (part of i2c-tools) and get i2c address.
Bingo, it is at 0x1d. I modified your patch to set the i2c address to
0x1d and at least free fall detection seems to be working correctly.
--
Best regards,
Michał Kępień
^ permalink raw reply
* Re: [PATCH] i2c: i801: Register optional lis3lv02d i2c device on Dell machines
From: Pali Rohár @ 2016-12-29 9:00 UTC (permalink / raw)
To: Michał Kępień
Cc: Jean Delvare, Wolfram Sang, Steven Honeyman, Valdis.Kletnieks,
Jochen Eisinger, Gabriele Mazzotta, Andy Lutomirski,
Mario_Limonciello, Alex Hung, Takashi Iwai, linux-i2c,
linux-kernel, platform-driver-x86
In-Reply-To: <20161229082936.GA4114@ozzy.nask.waw.pl>
[-- Attachment #1: Type: Text/Plain, Size: 7708 bytes --]
On Thursday 29 December 2016 09:29:36 Michał Kępień wrote:
> > Dell platform team told us that some (DMI whitelisted) Dell
> > Latitude machines have ST microelectronics accelerometer at i2c
> > address 0x29. That i2c address is not specified in DMI or ACPI, so
> > runtime detection without whitelist which is below is not
> > possible.
> >
> > Presence of that ST microelectronics accelerometer is verified by
> > existence of SMO88xx ACPI device which represent that
> > accelerometer. Unfortunately without i2c address.
>
> This part of the commit message sounded a bit confusing to me at
> first because there is already an ACPI driver which handles SMO88xx
> devices (dell-smo8800). My understanding is that:
>
> * the purpose of this patch is to expose a richer interface (as
> provided by lis3lv02d) to these devices on some machines,
>
> * on whitelisted machines, dell-smo8800 and lis3lv02d can work
> simultaneously (even though dell-smo8800 effectively duplicates
> the work that lis3lv02d does).
No. dell-smo8800 reads from ACPI irq number and exports /dev/freefall
device which notify userspace about falls. lis3lv02d is i2c driver which
exports axes of accelerometer. Additionaly lis3lv02d can export also
/dev/freefall if registerer of i2c device provides irq number -- which
is not case of this patch.
So both drivers are doing different things and both are useful.
IIRC both dell-smo8800 and lis3lv02d represent one HW device (that ST
microelectronics accelerometer) but due to complicated HW abstraction
and layers on Dell laptops it is handled by two drivers, one ACPI and
one i2c.
Yes, in ideal world irq number should be passed to lis3lv02d driver and
that would export whole device (with /dev/freefall too), but due to HW
abstraction it is too much complicated...
> If I got something wrong, please correct me. If I got it right, it
> might make sense to rephrase the commit message a bit so that the
> first bullet point above is immediately clear to the reader.
>
> > This patch registers lis3lv02d device at i2c address 0x29 if is
> > detected.
> >
> > Finally commit a7ae81952cda ("i2c: i801: Allow ACPI SystemIO
> > OpRegion to conflict with PCI BAR") allowed to use i2c-i801 driver
> > on Dell machines so lis3lv02d correctly initialize accelerometer.
> >
> > Tested on Dell Latitude E6440.
> >
> > Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
> > ---
> >
> > drivers/i2c/busses/i2c-i801.c | 98
> > +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 98
> > insertions(+)
> >
> > diff --git a/drivers/i2c/busses/i2c-i801.c
> > b/drivers/i2c/busses/i2c-i801.c index eb3627f..188cfd4 100644
> > --- a/drivers/i2c/busses/i2c-i801.c
> > +++ b/drivers/i2c/busses/i2c-i801.c
> > @@ -1118,6 +1118,101 @@ static void dmi_check_onboard_devices(const
> > struct dmi_header *dm, void *adap)
> >
> > }
> >
> > }
> >
> > +static acpi_status check_acpi_smo88xx_device(acpi_handle
> > obj_handle, + u32 nesting_level,
> > + void *context,
> > + void **return_value)
> > +{
> > + struct acpi_device_info *info;
> > + acpi_status status;
> > + char *hid;
> > +
> > + status = acpi_get_object_info(obj_handle, &info);
>
> acpi_get_object_info() allocates the returned buffer, which the
> caller has to free.
Ok, I will fix it in next patch iteration.
> > + if (!ACPI_SUCCESS(status) || !(info->valid & ACPI_VALID_HID))
> > + return AE_OK;
> > +
> > + hid = info->hardware_id.string;
> > + if (!hid)
> > + return AE_OK;
> > +
> > + if (strlen(hid) < 7)
> > + return AE_OK;
> > +
> > + if (memcmp(hid, "SMO88", 5) != 0)
> > + return AE_OK;
> > +
> > + *((bool *)return_value) = true;
> > + return AE_CTRL_TERMINATE;
> > +}
> > +
> > +static bool is_dell_system_with_lis3lv02d(void)
> > +{
> > + bool found;
> > + acpi_status status;
> > + const char *vendor;
> > +
> > + vendor = dmi_get_system_info(DMI_SYS_VENDOR);
> > + if (strcmp(vendor, "Dell Inc.") != 0)
> > + return false;
> > +
> > + /*
> > + * Check if ACPI device SMO88xx exists and if is enabled. That
> > ACPI + * device represent our ST microelectronics lis3lv02d
> > accelerometer but + * unfortunately without any other additional
> > information. + */
> > + found = false;
> > + status = acpi_get_devices(NULL, check_acpi_smo88xx_device, NULL,
> > + (void **)&found);
> > + if (!ACPI_SUCCESS(status) || !found)
> > + return false;
> > +
> > + return true;
> > +}
> > +
> > +/*
> > + * Dell platform team told us that these Latitude devices have
> > + * ST microelectronics accelerometer at i2c address 0x29.
> > + * That i2c address is not specified in DMI or ACPI, so runtime
> > + * detection without whitelist which is below is not possible.
> > + */
> > +static const char * const dmi_dell_product_names[] = {
> > + "Latitude E5250",
> > + "Latitude E5450",
> > + "Latitude E5550",
> > + "Latitude E6440",
> > + "Latitude E6440 ATG",
> > + "Latitude E6540",
> > +};
> > +
> > +static void register_dell_lis3lv02d_i2c_device(struct i801_priv
> > *priv) +{
> > + struct i2c_board_info info;
> > + const char *product_name;
> > + bool known_i2c_address;
> > + int i;
> > +
> > + known_i2c_address = false;
> > + product_name = dmi_get_system_info(DMI_PRODUCT_NAME);
> > + for (i = 0; i < ARRAY_SIZE(dmi_dell_product_names); ++i) {
> > + if (strcmp(product_name, dmi_dell_product_names[i]) == 0) {
> > + known_i2c_address = true;
> > + break;
> > + }
> > + }
> > +
> > + if (!known_i2c_address) {
> > + dev_warn(&priv->pci_dev->dev,
> > + "Accelerometer lis3lv02d i2c device is present "
> > + "but its i2c address is unknown, skipping ...\n");
>
> You are probably well aware of this, but checkpatch prefers keeping
> long log messages in one line. I am pointing it out just in case.
Yes, but I do not know how to fix it. Splitting message into two lines
generates warning. Having long line generates warning too.
> > + return;
> > + }
> > +
> > + memset(&info, 0, sizeof(struct i2c_board_info));
>
> How about just doing "struct i2c_board_info info = { 0 };" instead?
Ok.
> > + info.addr = 0x29;
> > + strlcpy(info.type, "lis3lv02d", I2C_NAME_SIZE);
> > + i2c_new_device(&priv->adapter, &info);
> > +}
> > +
> >
> > /* Register optional slaves */
> > static void i801_probe_optional_slaves(struct i801_priv *priv)
> > {
> >
> > @@ -1136,6 +1231,9 @@ static void i801_probe_optional_slaves(struct
> > i801_priv *priv)
> >
> > if (dmi_name_in_vendors("FUJITSU"))
> >
> > dmi_walk(dmi_check_onboard_devices, &priv->adapter);
> >
> > +
> > + if (is_dell_system_with_lis3lv02d())
> > + register_dell_lis3lv02d_i2c_device(priv);
> >
> > }
> > #else
> > static void __init input_apanel_init(void) {}
>
> I tested this patch on a Vostro V131, which is not on the whitelist,
> so all I got was the warning message, but to this extent, it works
> for me.
Hm... That means your notebook has ST microelectronics accelerometer
too. You could try to find it on i2c-i801 bus with userspace i2cdetect
program (part of i2c-tools) and get i2c address. If it will work we can
extend DMI name --> i2c address mapping and include your notebook too.
I have that list of confirmed Latitude devices since April 2015 but due
to problem with ACPI resource conflicts it was not possible to load i2c-
i801.ko bus driver. It was fixed only this year by commit a7ae81952cda.
So list of devices is probably not up-to-date and new appeared.
--
Pali Rohár
pali.rohar@gmail.com
[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]
^ permalink raw reply
* Re: [PATCH] i2c: i801: Register optional lis3lv02d i2c device on Dell machines
From: Michał Kępień @ 2016-12-29 8:29 UTC (permalink / raw)
To: Pali Rohár
Cc: Jean Delvare, Wolfram Sang, Steven Honeyman, Valdis.Kletnieks,
Jochen Eisinger, Gabriele Mazzotta, Andy Lutomirski,
Mario_Limonciello, Alex Hung, Takashi Iwai, linux-i2c,
linux-kernel, platform-driver-x86
In-Reply-To: <1482843136-12838-1-git-send-email-pali.rohar@gmail.com>
> Dell platform team told us that some (DMI whitelisted) Dell Latitude
> machines have ST microelectronics accelerometer at i2c address 0x29. That
> i2c address is not specified in DMI or ACPI, so runtime detection without
> whitelist which is below is not possible.
>
> Presence of that ST microelectronics accelerometer is verified by existence
> of SMO88xx ACPI device which represent that accelerometer. Unfortunately
> without i2c address.
This part of the commit message sounded a bit confusing to me at first
because there is already an ACPI driver which handles SMO88xx devices
(dell-smo8800). My understanding is that:
* the purpose of this patch is to expose a richer interface (as
provided by lis3lv02d) to these devices on some machines,
* on whitelisted machines, dell-smo8800 and lis3lv02d can work
simultaneously (even though dell-smo8800 effectively duplicates the
work that lis3lv02d does).
If I got something wrong, please correct me. If I got it right, it
might make sense to rephrase the commit message a bit so that the first
bullet point above is immediately clear to the reader.
>
> This patch registers lis3lv02d device at i2c address 0x29 if is detected.
>
> Finally commit a7ae81952cda ("i2c: i801: Allow ACPI SystemIO OpRegion to
> conflict with PCI BAR") allowed to use i2c-i801 driver on Dell machines so
> lis3lv02d correctly initialize accelerometer.
>
> Tested on Dell Latitude E6440.
>
> Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
> ---
> drivers/i2c/busses/i2c-i801.c | 98 +++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 98 insertions(+)
>
> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
> index eb3627f..188cfd4 100644
> --- a/drivers/i2c/busses/i2c-i801.c
> +++ b/drivers/i2c/busses/i2c-i801.c
> @@ -1118,6 +1118,101 @@ static void dmi_check_onboard_devices(const struct dmi_header *dm, void *adap)
> }
> }
>
> +static acpi_status check_acpi_smo88xx_device(acpi_handle obj_handle,
> + u32 nesting_level,
> + void *context,
> + void **return_value)
> +{
> + struct acpi_device_info *info;
> + acpi_status status;
> + char *hid;
> +
> + status = acpi_get_object_info(obj_handle, &info);
acpi_get_object_info() allocates the returned buffer, which the caller
has to free.
> + if (!ACPI_SUCCESS(status) || !(info->valid & ACPI_VALID_HID))
> + return AE_OK;
> +
> + hid = info->hardware_id.string;
> + if (!hid)
> + return AE_OK;
> +
> + if (strlen(hid) < 7)
> + return AE_OK;
> +
> + if (memcmp(hid, "SMO88", 5) != 0)
> + return AE_OK;
> +
> + *((bool *)return_value) = true;
> + return AE_CTRL_TERMINATE;
> +}
> +
> +static bool is_dell_system_with_lis3lv02d(void)
> +{
> + bool found;
> + acpi_status status;
> + const char *vendor;
> +
> + vendor = dmi_get_system_info(DMI_SYS_VENDOR);
> + if (strcmp(vendor, "Dell Inc.") != 0)
> + return false;
> +
> + /*
> + * Check if ACPI device SMO88xx exists and if is enabled. That ACPI
> + * device represent our ST microelectronics lis3lv02d accelerometer but
> + * unfortunately without any other additional information.
> + */
> + found = false;
> + status = acpi_get_devices(NULL, check_acpi_smo88xx_device, NULL,
> + (void **)&found);
> + if (!ACPI_SUCCESS(status) || !found)
> + return false;
> +
> + return true;
> +}
> +
> +/*
> + * Dell platform team told us that these Latitude devices have
> + * ST microelectronics accelerometer at i2c address 0x29.
> + * That i2c address is not specified in DMI or ACPI, so runtime
> + * detection without whitelist which is below is not possible.
> + */
> +static const char * const dmi_dell_product_names[] = {
> + "Latitude E5250",
> + "Latitude E5450",
> + "Latitude E5550",
> + "Latitude E6440",
> + "Latitude E6440 ATG",
> + "Latitude E6540",
> +};
> +
> +static void register_dell_lis3lv02d_i2c_device(struct i801_priv *priv)
> +{
> + struct i2c_board_info info;
> + const char *product_name;
> + bool known_i2c_address;
> + int i;
> +
> + known_i2c_address = false;
> + product_name = dmi_get_system_info(DMI_PRODUCT_NAME);
> + for (i = 0; i < ARRAY_SIZE(dmi_dell_product_names); ++i) {
> + if (strcmp(product_name, dmi_dell_product_names[i]) == 0) {
> + known_i2c_address = true;
> + break;
> + }
> + }
> +
> + if (!known_i2c_address) {
> + dev_warn(&priv->pci_dev->dev,
> + "Accelerometer lis3lv02d i2c device is present "
> + "but its i2c address is unknown, skipping ...\n");
You are probably well aware of this, but checkpatch prefers keeping long
log messages in one line. I am pointing it out just in case.
> + return;
> + }
> +
> + memset(&info, 0, sizeof(struct i2c_board_info));
How about just doing "struct i2c_board_info info = { 0 };" instead?
> + info.addr = 0x29;
> + strlcpy(info.type, "lis3lv02d", I2C_NAME_SIZE);
> + i2c_new_device(&priv->adapter, &info);
> +}
> +
> /* Register optional slaves */
> static void i801_probe_optional_slaves(struct i801_priv *priv)
> {
> @@ -1136,6 +1231,9 @@ static void i801_probe_optional_slaves(struct i801_priv *priv)
>
> if (dmi_name_in_vendors("FUJITSU"))
> dmi_walk(dmi_check_onboard_devices, &priv->adapter);
> +
> + if (is_dell_system_with_lis3lv02d())
> + register_dell_lis3lv02d_i2c_device(priv);
> }
> #else
> static void __init input_apanel_init(void) {}
> --
> 1.7.9.5
>
I tested this patch on a Vostro V131, which is not on the whitelist, so
all I got was the warning message, but to this extent, it works for me.
--
Best regards,
Michał Kępień
^ permalink raw reply
* Re: [PATCH] i2c: i801: Register optional lis3lv02d i2c device on Dell machines
From: Valdis.Kletnieks @ 2016-12-29 4:37 UTC (permalink / raw)
To: Wolfram Sang
Cc: Pali Rohár, Andy Shevchenko, Mika Westerberg,
Jean Delvare, Steven Honeyman, Jochen Eisinger, Gabriele Mazzotta,
Andy Lutomirski, Mario Limonciello, Alex Hung,
Michał Kępień,
Takashi Iwai, linux-i2c, linux-kernel, platform-driver-x86
In-Reply-To: <92e01f42db5129122dcf87f26f5e9917@the-dreams.de>
[-- Attachment #1: Type: text/plain, Size: 595 bytes --]
On Wed, 28 Dec 2016 15:03:02 +0100, Wolfram Sang said:
> > I have absolutely no idea how to you want to achieve calling that
> > i2c_new_device() registration
> > without kernel patches.
>
> Documentation/i2c/instantiating-devices lists all supported methods.
> Method 4 is userspace instantiation.
I'd be totally OK with userspace doing it, except for the question "How good
will distros be about shipping it"? I don't have any sense of how good Fedora
and Ubuntu and so on will be about making sure the userspace part is already
done for the user.
Anybody got evidence one way or another?
[-- Attachment #2: Type: application/pgp-signature, Size: 484 bytes --]
^ permalink raw reply
* Re: [PATCH v7 2/5] i2c: Add STM32F4 I2C driver
From: M'boumba Cedric Madianga @ 2016-12-28 22:20 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: Wolfram Sang, Rob Herring, Maxime Coquelin, Alexandre Torgue,
Linus Walleij, Patrice Chotard, linux, linux-i2c, devicetree,
linux-arm-kernel, linux-kernel
In-Reply-To: <20161228212139.adkixdgvmtj2ukjs@pengutronix.de>
Hello Uwe,
2016-12-28 22:21 GMT+01:00 Uwe Kleine-König <u.kleine-koenig@pengutronix.de>:
> Hello Cedric,
>
> On Fri, Dec 23, 2016 at 01:41:15PM +0100, M'boumba Cedric Madianga wrote:
>> > I don't understand why DUTY is required to reach 400 kHz. Given a parent
>> > freq of 30 MHz, with CCR = 25 and DUTY = 0 we have:
>> >
>> > t_high = 25 * 33.333 ns = 833.333 ns
>> > t_low = 2 * 25 * 33.333 ns = 1666.667 ns
>> >
>> > then t_high and t_low satisfy the i2c bus specification
>> > (t_low > 1300 ns, t_high > 600 ns) and we have t_low + t_high = 2500 ns
>> > = 1 / 400 kHz.
>> >
>> > Where is the error?
>> Hum ok you are right. I was a bad interpretation of the datasheet.
>> So now it is clearer. Thanks for that.
>> I will correct and improve my comments in the V8.
>
> The benefit of DUTY=1 is (I think) that you can reach 400 kHz also with
> lower parent frequencies. And so it't probably sensible to make use of
> it unconditionally for fast mode.
Ok I agree.
>
>> >> + * So, in order to cover both SCL high/low with Duty = 1,
>> >> + * CCR = 16 * SCL period * I2C CLK frequency
>> >
>> > I don't get that. Actually you need to use low + high, so
>> > CCR = parentrate / (25 * 400 kHz), right?
>> With your new inputs above, I think I could use a simpler implementation:
>> CCR = scl_high_period * parent_rate
>> with scl_high_period = 5 µs in standard mode to reach 100khz
>> and scl_high_period = 1 µs in fast mode to reach 400khz with 1/2 or
>> 16/9 duty cycle.
>> So, I am wondering if I have to let the customer setting the duty
>> cycle in the DT for example with "st,duty=0" or "st,duty=1" property
>> (0 for 1/2 and 1 for 16/9).
>> Or perhaps the best option it to use a default value. What is your
>> feeling regarding this point ?
>
> No, don't add a property to the device tree. Just take pencil and paper
> and think a while about the right algorithm to choose the right register
> settings.
Ok thanks
>
>
>> >> + cr2 = readl_relaxed(i2c_dev->base + STM32F4_I2C_CR2);
>> >> + freq = cr2 & STM32F4_I2C_CR2_FREQ_MASK;
>> >> +
>> >> + if (i2c_dev->speed == STM32F4_I2C_SPEED_STANDARD)
>> >> + trise = freq + 1;
>> >> + else
>> >> + trise = freq * 300 / 1000 + 1;
>> >
>> > if freq is big such that freq * 300 overflows does this result in a
>> > wrong result, or does the compiler optimize correctly?
>> For sure the compiler will never exceeds u32 max value
>
> If I compile
> -------->8--------
> #include <stdio.h>
>
> void myfunc(unsigned freq)
> {
> unsigned trise = freq * 300 / 1000 + 1;
> printf("trise = %u", trise);
> }
>
> -------->8--------
>
> for arm with -O3 I get:
>
> 00000000 <myfunc>:
> 0: e3a01f4b mov r1, #300 ; 0x12c
> 4: e0010190 mul r1, r0, r1
> 8: e59f3010 ldr r3, [pc, #16] ; 20 <myfunc+0x20>
> c: e59f0010 ldr r0, [pc, #16] ; 24 <myfunc+0x24>
> 10: e0812193 umull r2, r1, r3, r1
> 14: e1a01321 lsr r1, r1, #6
> 18: e2811001 add r1, r1, #1
> 1c: eafffffe b 0 <printf>
> 20: 10624dd3 .word 0x10624dd3
> 24: 00000000 .word 0x00000000
>
> The mul instruction at offset 4 writes the least significant 32 bits of
> 300 * r0 to r1 and so doesn't handle overflow correctly.
In case of overflow, the parent frequency has to be at least at 1431657 Mhz.
The STM32F4 SoC will never reach this value for any parent clock.
So, I think that this point is not really critical and you can
probably keep these simple lines of code without adding u32 overflow
checking for big frequency.
>
>> > This is still not really understandable.
>> I have already added some comments from datasheet to explain the
>> different cases.
>> I don't see how I could be more understandable as it is clearly the
>> hardware way of working...
>
> You need to comment the check for the length, something like:
>
> /*
> * To end the transfer correctly the foo bit has to be cleared
> * already on the last but one byte to be transferred.
> */
>
OK I will add more comments.
> and bonus points if you can shrink the number of functions that check
> for this stuff.
There are only 2 functions to handle this stuff: handle_read() for
RXNE event and handle_rx_btf() for BTF event
I would prefer to keep 2 seperate functions to handle each event
according to buffer length as I don't have to do the same thing.
For example:
- for RXNE event with count = 2, I have to disable buffer interrupts
- for BTF event with msg count = 2, I have to .generate stop or
repeated start and disable all remaining interrupts.
I think that if I gather this stuff in one function, the code will be
less understandable.
>
>> > Just do:
>> >
>> > if (status & STM32F4_I2C_SR1_SB) {
>> > ...
>> > }
>> >
>> > if (status & ...) {
>> >
>> > }
>> ok but I would prefer something like that:
>> flag = status & possible_status
>> if (flag & STM32F4_I2C_SR1_SB) {
>> ...
>> }
>>
>> if (flag & ...) {
>> }
>
> Ok, if you really need possible_status.
>
>> > Then it's obvious by reading the code in which order they are handled
>> > without the need to check the definitions. Do you really need to jugle
>> > with possible_status?
>> I think I have to use possible_status as some events could occur
>> whereas the corresponding interrupt is disabled.
>> For example, for a 2 byte-reception, we don't have to take into accout
>> RXNE event so the corresponding interrupt is disabled.
>
> Is it possible to make it more obvious by doing:
>
> status = read_from_status_register() & read_from_interrupt_enable_register();
>
> at a single place?
Ok I will add these 2 functions in order to only use one status variable.
>
>> >> + /* Use __fls() to check error bits first */
>> >> + flag = __fls(status & possible_status);
>> >> +
>> >> + switch (1 << flag) {
>> >> + case STM32F4_I2C_SR1_BERR:
>> >> + reg = i2c_dev->base + STM32F4_I2C_SR1;
>> >> + stm32f4_i2c_clr_bits(reg, STM32F4_I2C_SR1_BERR);
>> >> + msg->result = -EIO;
>> >> + break;
>> >> +
>> >> + case STM32F4_I2C_SR1_ARLO:
>> >> + reg = i2c_dev->base + STM32F4_I2C_SR1;
>> >> + stm32f4_i2c_clr_bits(reg, STM32F4_I2C_SR1_ARLO);
>> >> + msg->result = -EAGAIN;
>> >> + break;
>> >> +
>> >> + case STM32F4_I2C_SR1_AF:
>> >> + reg = i2c_dev->base + STM32F4_I2C_CR1;
>> >> + stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_STOP);
>> >> + msg->result = -EIO;
>> >> + break;
>> >> +
>> >> + default:
>> >> + dev_err(i2c_dev->dev,
>> >> + "err it unhandled: status=0x%08x)\n", status);
>> >> + return IRQ_NONE;
>> >> + }
>> >
>> > You only check a single irq flag here.
>> Yes only the first error could be reported to the i2c clients via
>> msg->result that's why I don't check all errors.
>> Moreover, as soon as an error occurs, the I2C device is reset.
>
> The the "first" in the comment for __fls is misleading. Also do:
>
> if (status & MOST_IMPORTANT_ERROR) {
> ...
> }
>
> if (status & SECOND_MOST_IMPORTANT_ERROR) {
> ...
> }
>
> (if you need including possible_status stuff).
OK
>
> Best regards
> Uwe
>
> --
> Pengutronix e.K. | Uwe Kleine-König |
> Industrial Linux Solutions | http://www.pengutronix.de/ |
Best regards,
Cedric
^ permalink raw reply
* Re: [PATCH v7 2/5] i2c: Add STM32F4 I2C driver
From: Uwe Kleine-König @ 2016-12-28 21:21 UTC (permalink / raw)
To: M'boumba Cedric Madianga
Cc: devicetree, Alexandre Torgue, Wolfram Sang, linux-kernel,
Linus Walleij, Patrice Chotard, linux, Rob Herring, linux-i2c,
Maxime Coquelin, linux-arm-kernel
In-Reply-To: <CAOAejn3sc1F8T_rbSTr6-wBNncNwObeK+c6=_Q0BMahDdHXxQA@mail.gmail.com>
Hello Cedric,
On Fri, Dec 23, 2016 at 01:41:15PM +0100, M'boumba Cedric Madianga wrote:
> > I don't understand why DUTY is required to reach 400 kHz. Given a parent
> > freq of 30 MHz, with CCR = 25 and DUTY = 0 we have:
> >
> > t_high = 25 * 33.333 ns = 833.333 ns
> > t_low = 2 * 25 * 33.333 ns = 1666.667 ns
> >
> > then t_high and t_low satisfy the i2c bus specification
> > (t_low > 1300 ns, t_high > 600 ns) and we have t_low + t_high = 2500 ns
> > = 1 / 400 kHz.
> >
> > Where is the error?
> Hum ok you are right. I was a bad interpretation of the datasheet.
> So now it is clearer. Thanks for that.
> I will correct and improve my comments in the V8.
The benefit of DUTY=1 is (I think) that you can reach 400 kHz also with
lower parent frequencies. And so it't probably sensible to make use of
it unconditionally for fast mode.
> >> + * So, in order to cover both SCL high/low with Duty = 1,
> >> + * CCR = 16 * SCL period * I2C CLK frequency
> >
> > I don't get that. Actually you need to use low + high, so
> > CCR = parentrate / (25 * 400 kHz), right?
> With your new inputs above, I think I could use a simpler implementation:
> CCR = scl_high_period * parent_rate
> with scl_high_period = 5 µs in standard mode to reach 100khz
> and scl_high_period = 1 µs in fast mode to reach 400khz with 1/2 or
> 16/9 duty cycle.
> So, I am wondering if I have to let the customer setting the duty
> cycle in the DT for example with "st,duty=0" or "st,duty=1" property
> (0 for 1/2 and 1 for 16/9).
> Or perhaps the best option it to use a default value. What is your
> feeling regarding this point ?
No, don't add a property to the device tree. Just take pencil and paper
and think a while about the right algorithm to choose the right register
settings.
> >> + cr2 = readl_relaxed(i2c_dev->base + STM32F4_I2C_CR2);
> >> + freq = cr2 & STM32F4_I2C_CR2_FREQ_MASK;
> >> +
> >> + if (i2c_dev->speed == STM32F4_I2C_SPEED_STANDARD)
> >> + trise = freq + 1;
> >> + else
> >> + trise = freq * 300 / 1000 + 1;
> >
> > if freq is big such that freq * 300 overflows does this result in a
> > wrong result, or does the compiler optimize correctly?
> For sure the compiler will never exceeds u32 max value
If I compile
-------->8--------
#include <stdio.h>
void myfunc(unsigned freq)
{
unsigned trise = freq * 300 / 1000 + 1;
printf("trise = %u", trise);
}
-------->8--------
for arm with -O3 I get:
00000000 <myfunc>:
0: e3a01f4b mov r1, #300 ; 0x12c
4: e0010190 mul r1, r0, r1
8: e59f3010 ldr r3, [pc, #16] ; 20 <myfunc+0x20>
c: e59f0010 ldr r0, [pc, #16] ; 24 <myfunc+0x24>
10: e0812193 umull r2, r1, r3, r1
14: e1a01321 lsr r1, r1, #6
18: e2811001 add r1, r1, #1
1c: eafffffe b 0 <printf>
20: 10624dd3 .word 0x10624dd3
24: 00000000 .word 0x00000000
The mul instruction at offset 4 writes the least significant 32 bits of
300 * r0 to r1 and so doesn't handle overflow correctly.
> > This is still not really understandable.
> I have already added some comments from datasheet to explain the
> different cases.
> I don't see how I could be more understandable as it is clearly the
> hardware way of working...
You need to comment the check for the length, something like:
/*
* To end the transfer correctly the foo bit has to be cleared
* already on the last but one byte to be transferred.
*/
and bonus points if you can shrink the number of functions that check
for this stuff.
> > Just do:
> >
> > if (status & STM32F4_I2C_SR1_SB) {
> > ...
> > }
> >
> > if (status & ...) {
> >
> > }
> ok but I would prefer something like that:
> flag = status & possible_status
> if (flag & STM32F4_I2C_SR1_SB) {
> ...
> }
>
> if (flag & ...) {
> }
Ok, if you really need possible_status.
> > Then it's obvious by reading the code in which order they are handled
> > without the need to check the definitions. Do you really need to jugle
> > with possible_status?
> I think I have to use possible_status as some events could occur
> whereas the corresponding interrupt is disabled.
> For example, for a 2 byte-reception, we don't have to take into accout
> RXNE event so the corresponding interrupt is disabled.
Is it possible to make it more obvious by doing:
status = read_from_status_register() & read_from_interrupt_enable_register();
at a single place?
> >> + /* Use __fls() to check error bits first */
> >> + flag = __fls(status & possible_status);
> >> +
> >> + switch (1 << flag) {
> >> + case STM32F4_I2C_SR1_BERR:
> >> + reg = i2c_dev->base + STM32F4_I2C_SR1;
> >> + stm32f4_i2c_clr_bits(reg, STM32F4_I2C_SR1_BERR);
> >> + msg->result = -EIO;
> >> + break;
> >> +
> >> + case STM32F4_I2C_SR1_ARLO:
> >> + reg = i2c_dev->base + STM32F4_I2C_SR1;
> >> + stm32f4_i2c_clr_bits(reg, STM32F4_I2C_SR1_ARLO);
> >> + msg->result = -EAGAIN;
> >> + break;
> >> +
> >> + case STM32F4_I2C_SR1_AF:
> >> + reg = i2c_dev->base + STM32F4_I2C_CR1;
> >> + stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_STOP);
> >> + msg->result = -EIO;
> >> + break;
> >> +
> >> + default:
> >> + dev_err(i2c_dev->dev,
> >> + "err it unhandled: status=0x%08x)\n", status);
> >> + return IRQ_NONE;
> >> + }
> >
> > You only check a single irq flag here.
> Yes only the first error could be reported to the i2c clients via
> msg->result that's why I don't check all errors.
> Moreover, as soon as an error occurs, the I2C device is reset.
The the "first" in the comment for __fls is misleading. Also do:
if (status & MOST_IMPORTANT_ERROR) {
...
}
if (status & SECOND_MOST_IMPORTANT_ERROR) {
...
}
(if you need including possible_status stuff).
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
^ permalink raw reply
* Re: [PATCH v5 6/7] i2c: designware: enable SLAVE in platform module
From: Luis Oliveira @ 2016-12-28 18:10 UTC (permalink / raw)
To: Andy Shevchenko, Luis Oliveira, wsa-z923LK4zBo2bacvFa/9K2g,
robh+dt-DgEjT+Ai2ygdnm+yROfE0A, mark.rutland-5wv7dgnIgG8,
jarkko.nikula-VuQAYsv1563Yd54FQh9/CA,
mika.westerberg-VuQAYsv1563Yd54FQh9/CA,
linux-i2c-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
Cc: Ramiro.Oliveira-HKixBCOQz3hWk0Htik3J/w,
Joao.Pinto-HKixBCOQz3hWk0Htik3J/w,
CARLOS.PALMINHA-HKixBCOQz3hWk0Htik3J/w
In-Reply-To: <1482945043.9552.174.camel-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
On 28-Dec-16 17:10, Andy Shevchenko wrote:
> On Wed, 2016-12-28 at 16:41 +0000, Luis Oliveira wrote:
>> On 28-Dec-16 16:31, Andy Shevchenko wrote:
>>> On Wed, 2016-12-28 at 15:53 +0000, Luis Oliveira wrote:
>>>> On 28-Dec-16 15:44, Andy Shevchenko wrote:
>>>>> On Wed, 2016-12-28 at 14:43 +0000, Luis Oliveira wrote:
>>>>>> - Slave mode selected in platform module (devicetree support
>>>>>> only)
>>>>>> - Check for ACPI - not supported in SLAVE mode:
>>>>>> - Changed the ifndef style to the use of ACPI_HANDLE that
>>>>>> returns
>>>>>> NULL
>>>>>> if the device was not enumerated from ACPI namespace.
>>>>>
>>>>> I'm not sure what is wrong with ACPI?
>>>>
>>>> I dont have a way to test it. Just that.
>>>
>>> Okay, can you provide an excerpt to see how it will look like in
>>> DTS?
>>
>> Yes, it looks like this now:
>>
>> i2c@0x2000 {
>> compatible = "snps,designware-i2c";
>> #address-cells = <1>;
>> #size-cells = <0>;
>> reg = <0x2000 0x100>;
>> clock-frequency = <400000>;
>> clocks = <&i2cclk>;
>> interrupts = <0>;
>>
>> eeprom@64 {
>> compatible = "linux,slave-24c02";
>> reg = <0x40000064>;
>> };
>> };
>
> +1 to Carlos' comment.
Agree, I'm on it.
>
>>>
>>>>>> - dev->functionality = I2C_FUNC_10BIT_ADDR |
>>>>>> DW_IC_DEFAULT_FUNCTIONALITY;
>>>>>> -
>>>>>> - i2c_dw_configure_master(pdev);
>>>>>> + if (ACPI_HANDLE(&pdev->dev) == NULL) {
>>>>>
>>>>> I don't think you need this at all.
>>>>
>>>> This is to avoid the use of the "ifdef" style I used before.
>>>
>>> My point is to drop it completely.
>>>
>>>>>
>>>>>> + device_for_each_child_node(&pdev->dev, child)
>>>>>> {
>>>>>
>>>>> This is resource agnostic.
>>>>>
>>>>>> + fwnode_property_read_u32(child,
>>>>>> "reg",
>>>>>> ®);
>>>>>
>>>>> This is as well.
>>>>
>>>> Are you suggesting I use of_ functions?
>>>
>>> Nope. See above.
>
> So, ACPI has a property to support slave mode for I2CSerialBus() macro.
>
> I would propose to create a helper function in i2c-core.c which will be
> responsible for mode detection
>
> ... i2c_slave_mode_detect()
> {
> ...
> if (IS_BUILTIN(CONFIG_OF) && dev->of_node) {
> ... (use of_*() here) ...
> } else if (IS_BUILTIN(CONFIG_ACPI) && ACPI_HANDLE(dev))
> dev_dbg(..., "ACPI slave is not supported yet\n");
> ... to master ...
> } else {
> ... default to master ...
> }
> }
> EXPORT_...();
>
> Make it as a separate patch.
>
Oh I see, yes it looks good. I will check it. Thanks
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v5 6/7] i2c: designware: enable SLAVE in platform module
From: Andy Shevchenko @ 2016-12-28 17:10 UTC (permalink / raw)
To: Luis Oliveira, wsa, robh+dt, mark.rutland, jarkko.nikula,
mika.westerberg, linux-i2c, devicetree, linux-kernel
Cc: Ramiro.Oliveira, Joao.Pinto, CARLOS.PALMINHA
In-Reply-To: <c41db9e0-756d-6697-a845-0d5b1efcaedc@synopsys.com>
On Wed, 2016-12-28 at 16:41 +0000, Luis Oliveira wrote:
> On 28-Dec-16 16:31, Andy Shevchenko wrote:
> > On Wed, 2016-12-28 at 15:53 +0000, Luis Oliveira wrote:
> > > On 28-Dec-16 15:44, Andy Shevchenko wrote:
> > > > On Wed, 2016-12-28 at 14:43 +0000, Luis Oliveira wrote:
> > > > > - Slave mode selected in platform module (devicetree support
> > > > > only)
> > > > > - Check for ACPI - not supported in SLAVE mode:
> > > > > - Changed the ifndef style to the use of ACPI_HANDLE that
> > > > > returns
> > > > > NULL
> > > > > if the device was not enumerated from ACPI namespace.
> > > >
> > > > I'm not sure what is wrong with ACPI?
> > >
> > > I dont have a way to test it. Just that.
> >
> > Okay, can you provide an excerpt to see how it will look like in
> > DTS?
>
> Yes, it looks like this now:
>
> i2c@0x2000 {
> compatible = "snps,designware-i2c";
> #address-cells = <1>;
> #size-cells = <0>;
> reg = <0x2000 0x100>;
> clock-frequency = <400000>;
> clocks = <&i2cclk>;
> interrupts = <0>;
>
> eeprom@64 {
> compatible = "linux,slave-24c02";
> reg = <0x40000064>;
> };
> };
+1 to Carlos' comment.
> >
> > > > > - dev->functionality = I2C_FUNC_10BIT_ADDR |
> > > > > DW_IC_DEFAULT_FUNCTIONALITY;
> > > > > -
> > > > > - i2c_dw_configure_master(pdev);
> > > > > + if (ACPI_HANDLE(&pdev->dev) == NULL) {
> > > >
> > > > I don't think you need this at all.
> > >
> > > This is to avoid the use of the "ifdef" style I used before.
> >
> > My point is to drop it completely.
> >
> > > >
> > > > > + device_for_each_child_node(&pdev->dev, child)
> > > > > {
> > > >
> > > > This is resource agnostic.
> > > >
> > > > > + fwnode_property_read_u32(child,
> > > > > "reg",
> > > > > ®);
> > > >
> > > > This is as well.
> > >
> > > Are you suggesting I use of_ functions?
> >
> > Nope. See above.
So, ACPI has a property to support slave mode for I2CSerialBus() macro.
I would propose to create a helper function in i2c-core.c which will be
responsible for mode detection
... i2c_slave_mode_detect()
{
...
if (IS_BUILTIN(CONFIG_OF) && dev->of_node) {
... (use of_*() here) ...
} else if (IS_BUILTIN(CONFIG_ACPI) && ACPI_HANDLE(dev))
dev_dbg(..., "ACPI slave is not supported yet\n");
... to master ...
} else {
... default to master ...
}
}
EXPORT_...();
Make it as a separate patch.
--
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy
^ permalink raw reply
* Re: [PATCH v5 6/7] i2c: designware: enable SLAVE in platform module
From: Carlos Palminha @ 2016-12-28 16:49 UTC (permalink / raw)
To: Luis Oliveira, Andy Shevchenko
Cc: Ramiro.Oliveira, Joao.Pinto, CARLOS.PALMINHA
In-Reply-To: <c41db9e0-756d-6697-a845-0d5b1efcaedc@synopsys.com>
On 28-12-2016 16:41, Luis Oliveira wrote:
> On 28-Dec-16 16:31, Andy Shevchenko wrote:
>> On Wed, 2016-12-28 at 15:53 +0000, Luis Oliveira wrote:
>>> On 28-Dec-16 15:44, Andy Shevchenko wrote:
>>>> On Wed, 2016-12-28 at 14:43 +0000, Luis Oliveira wrote:
>>>>> - Slave mode selected in platform module (devicetree support only)
>>>>> - Check for ACPI - not supported in SLAVE mode:
>>>>> - Changed the ifndef style to the use of ACPI_HANDLE that
>>>>> returns
>>>>> NULL
>>>>> if the device was not enumerated from ACPI namespace.
>>>>
>>>> I'm not sure what is wrong with ACPI?
>>>
>>> I dont have a way to test it. Just that.
>>
>> Okay, can you provide an excerpt to see how it will look like in DTS?
>
> Yes, it looks like this now:
>
> i2c@0x2000 {
> compatible = "snps,designware-i2c";
> #address-cells = <1>;
> #size-cells = <0>;
> reg = <0x2000 0x100>;
> clock-frequency = <400000>;
> clocks = <&i2cclk>;
> interrupts = <0>;
>
> eeprom@64 {
> compatible = "linux,slave-24c02";
> reg = <0x40000064>;
> };
> };
Probably this can be included as example in the device tree binding document.
>>
>>>>> - dev->functionality = I2C_FUNC_10BIT_ADDR |
>>>>> DW_IC_DEFAULT_FUNCTIONALITY;
>>>>> -
>>>>> - i2c_dw_configure_master(pdev);
>>>>> + if (ACPI_HANDLE(&pdev->dev) == NULL) {
>>>>
>>>> I don't think you need this at all.
>>>
>>> This is to avoid the use of the "ifdef" style I used before.
>>
>> My point is to drop it completely.
>>
>>>>
>>>>> + device_for_each_child_node(&pdev->dev, child) {
>>>>
>>>> This is resource agnostic.
>>>>
>>>>> + fwnode_property_read_u32(child, "reg",
>>>>> ®);
>>>>
>>>> This is as well.
>>>
>>> Are you suggesting I use of_ functions?
>>
>> Nope. See above.
>>
>>
>
^ permalink raw reply
* Re: [PATCH v5 6/7] i2c: designware: enable SLAVE in platform module
From: Luis Oliveira @ 2016-12-28 16:41 UTC (permalink / raw)
To: Andy Shevchenko, Luis Oliveira, wsa, robh+dt, mark.rutland,
jarkko.nikula, mika.westerberg, linux-i2c, devicetree,
linux-kernel
Cc: Ramiro.Oliveira, Joao.Pinto, CARLOS.PALMINHA
In-Reply-To: <1482942696.9552.172.camel@linux.intel.com>
On 28-Dec-16 16:31, Andy Shevchenko wrote:
> On Wed, 2016-12-28 at 15:53 +0000, Luis Oliveira wrote:
>> On 28-Dec-16 15:44, Andy Shevchenko wrote:
>>> On Wed, 2016-12-28 at 14:43 +0000, Luis Oliveira wrote:
>>>> - Slave mode selected in platform module (devicetree support only)
>>>> - Check for ACPI - not supported in SLAVE mode:
>>>> - Changed the ifndef style to the use of ACPI_HANDLE that
>>>> returns
>>>> NULL
>>>> if the device was not enumerated from ACPI namespace.
>>>
>>> I'm not sure what is wrong with ACPI?
>>
>> I dont have a way to test it. Just that.
>
> Okay, can you provide an excerpt to see how it will look like in DTS?
Yes, it looks like this now:
i2c@0x2000 {
compatible = "snps,designware-i2c";
#address-cells = <1>;
#size-cells = <0>;
reg = <0x2000 0x100>;
clock-frequency = <400000>;
clocks = <&i2cclk>;
interrupts = <0>;
eeprom@64 {
compatible = "linux,slave-24c02";
reg = <0x40000064>;
};
};
>
>
>>>> - dev->functionality = I2C_FUNC_10BIT_ADDR |
>>>> DW_IC_DEFAULT_FUNCTIONALITY;
>>>> -
>>>> - i2c_dw_configure_master(pdev);
>>>> + if (ACPI_HANDLE(&pdev->dev) == NULL) {
>>>
>>> I don't think you need this at all.
>>
>> This is to avoid the use of the "ifdef" style I used before.
>
> My point is to drop it completely.
>
>>>
>>>> + device_for_each_child_node(&pdev->dev, child) {
>>>
>>> This is resource agnostic.
>>>
>>>> + fwnode_property_read_u32(child, "reg",
>>>> ®);
>>>
>>> This is as well.
>>
>> Are you suggesting I use of_ functions?
>
> Nope. See above.
>
>
^ permalink raw reply
* Re: [PATCH v5 6/7] i2c: designware: enable SLAVE in platform module
From: Andy Shevchenko @ 2016-12-28 16:31 UTC (permalink / raw)
To: Luis Oliveira, wsa-z923LK4zBo2bacvFa/9K2g,
robh+dt-DgEjT+Ai2ygdnm+yROfE0A, mark.rutland-5wv7dgnIgG8,
jarkko.nikula-VuQAYsv1563Yd54FQh9/CA,
mika.westerberg-VuQAYsv1563Yd54FQh9/CA,
linux-i2c-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
Cc: Ramiro.Oliveira-HKixBCOQz3hWk0Htik3J/w,
Joao.Pinto-HKixBCOQz3hWk0Htik3J/w,
CARLOS.PALMINHA-HKixBCOQz3hWk0Htik3J/w
In-Reply-To: <20e47113-efd7-787c-b2f8-39e9fd8b83d2-HKixBCOQz3hWk0Htik3J/w@public.gmane.org>
On Wed, 2016-12-28 at 15:53 +0000, Luis Oliveira wrote:
> On 28-Dec-16 15:44, Andy Shevchenko wrote:
> > On Wed, 2016-12-28 at 14:43 +0000, Luis Oliveira wrote:
> > > - Slave mode selected in platform module (devicetree support only)
> > > - Check for ACPI - not supported in SLAVE mode:
> > > - Changed the ifndef style to the use of ACPI_HANDLE that
> > > returns
> > > NULL
> > > if the device was not enumerated from ACPI namespace.
> >
> > I'm not sure what is wrong with ACPI?
>
> I dont have a way to test it. Just that.
Okay, can you provide an excerpt to see how it will look like in DTS?
> > > - dev->functionality = I2C_FUNC_10BIT_ADDR |
> > > DW_IC_DEFAULT_FUNCTIONALITY;
> > > -
> > > - i2c_dw_configure_master(pdev);
> > > + if (ACPI_HANDLE(&pdev->dev) == NULL) {
> >
> > I don't think you need this at all.
>
> This is to avoid the use of the "ifdef" style I used before.
My point is to drop it completely.
> >
> > > + device_for_each_child_node(&pdev->dev, child) {
> >
> > This is resource agnostic.
> >
> > > + fwnode_property_read_u32(child, "reg",
> > > ®);
> >
> > This is as well.
>
> Are you suggesting I use of_ functions?
Nope. See above.
--
Andy Shevchenko <andriy.shevchenko-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
Intel Finland Oy
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v5 5/7] i2c: designware: add SLAVE mode functions
From: Luis Oliveira @ 2016-12-28 16:00 UTC (permalink / raw)
To: Andy Shevchenko, Luis Oliveira, wsa-z923LK4zBo2bacvFa/9K2g,
robh+dt-DgEjT+Ai2ygdnm+yROfE0A, mark.rutland-5wv7dgnIgG8,
jarkko.nikula-VuQAYsv1563Yd54FQh9/CA,
mika.westerberg-VuQAYsv1563Yd54FQh9/CA,
linux-i2c-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
Cc: Ramiro.Oliveira-HKixBCOQz3hWk0Htik3J/w,
Joao.Pinto-HKixBCOQz3hWk0Htik3J/w,
CARLOS.PALMINHA-HKixBCOQz3hWk0Htik3J/w
In-Reply-To: <1482939369.9552.162.camel-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
On 28-Dec-16 15:36, Andy Shevchenko wrote:
> On Wed, 2016-12-28 at 14:43 +0000, Luis Oliveira wrote:
>> - Changes in Kconfig to enable I2C_SLAVE support
>> - Slave functions added to core library file
>> - Slave abort sources added to common source file
>> - New driver: i2c-designware-slave added
>> - Changes in the Makefile to compile it all
>>
>> All the SLAVE flow is added but it is not enabled via platform
>> driver.
>
>> --- a/drivers/i2c/busses/i2c-designware-common.c
>> +++ b/drivers/i2c/busses/i2c-designware-common.c
>> @@ -30,6 +30,7 @@
>> #include <linux/pm_runtime.h>
>> #include <linux/delay.h>
>> #include <linux/module.h>
>> +
>> #include "i2c-designware-core.h"
>>
>> static char *abort_sources[] = {
>> @@ -42,7 +43,7 @@ static char *abort_sources[] = {
>> [ABRT_TXDATA_NOACK] =
>> "data not acknowledged",
>> [ABRT_GCALL_NOACK] =
>> - "no acknowledgement for a general call",
>> + "no acknowledgment for a general call",
>
> So, what's the point after your confirmation that both variants are
> okay?
>
It shouldn't be changed. I must have skip revert it to the original
>
>> +#include <linux/errno.h>
>> +#include <linux/err.h>
>> +#include <linux/i2c.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/io.h>
>> +#include <linux/pm_runtime.h>
>
>> +#include <linux/delay.h>
>> +#include <linux/module.h>
>
> Alphabetical order?
>
Also here.
>> +int i2c_dw_init_slave(struct dw_i2c_dev *dev)
>> +{
>> + u32 sda_falling_time, scl_falling_time;
>> + u32 reg, comp_param1;
>> + u32 hcnt, lcnt;
>> + int ret;
>> +
>> + ret = i2c_dw_acquire_lock(dev);
>> + if (ret)
>> + return ret;
>> +
>> + reg = dw_readl(dev, DW_IC_COMP_TYPE);
>> + if (reg == ___constant_swab32(DW_IC_COMP_TYPE_VALUE)) {
>> + /* Configure register endianness access. */
>> + dev->accessor_flags |= ACCESS_SWAP;
>> + } else if (reg == (DW_IC_COMP_TYPE_VALUE & 0x0000ffff)) {
>> + /* Configure register access mode 16bit. */
>> + dev->accessor_flags |= ACCESS_16BIT;
>> + } else if (reg != DW_IC_COMP_TYPE_VALUE) {
>
>> + dev_err(dev->dev,
>> + "Unknown Synopsys component type: 0x%08x\n",
>> reg);
>
> Is it correct indentation?
I will fix it.
>
>> + i2c_dw_release_lock(dev);
>> + return -ENODEV;
>> + }
>> +
>> + comp_param1 = dw_readl(dev, DW_IC_COMP_PARAM_1);
>> +
>> + /* Disable the adapter. */
>> + __i2c_dw_enable_and_wait(dev, false);
>> +
>> + /* Set standard and fast speed deviders for high/low periods.
>> */
>> + sda_falling_time = dev->sda_falling_time ?: 300; /* ns */
>> + scl_falling_time = dev->scl_falling_time ?: 300; /* ns */
>> +
>> + /* Set SCL timing parameters for standard-mode. */
>> + if (dev->ss_hcnt && dev->ss_lcnt) {
>> + hcnt = dev->ss_hcnt;
>> + lcnt = dev->ss_lcnt;
>> + } else {
>> + hcnt = i2c_dw_scl_hcnt(i2c_dw_clk_rate(dev),
>> + 4000, /* tHD;STA =
>> tHIGH = 4.0 us */
>> + sda_falling_time,
>> + 0, /* 0: DW default,
>> 1: Ideal */
>> + 0); /* No offset */
>> + lcnt = i2c_dw_scl_lcnt(i2c_dw_clk_rate(dev),
>> + 4700, /* tLOW = 4.7 us
>> */
>> + scl_falling_time,
>> + 0); /* No offset */
>> + }
>> + dw_writel(dev, hcnt, DW_IC_SS_SCL_HCNT);
>> + dw_writel(dev, lcnt, DW_IC_SS_SCL_LCNT);
>> + dev_dbg(dev->dev, "Standard-mode HCNT:LCNT = %d:%d\n", hcnt,
>> lcnt);
>> +
>> + /* Set SCL timing parameters for fast-mode or fast-mode plus.
>> */
>> + if ((dev->clk_freq == 1000000) && dev->fp_hcnt && dev-
>>> fp_lcnt) {
>> + hcnt = dev->fp_hcnt;
>> + lcnt = dev->fp_lcnt;
>> + } else if (dev->fs_hcnt && dev->fs_lcnt) {
>> + hcnt = dev->fs_hcnt;
>> + lcnt = dev->fs_lcnt;
>> + } else {
>> + hcnt = i2c_dw_scl_hcnt(i2c_dw_clk_rate(dev),
>> + 600, /* tHD;STA =
>> tHIGH = 0.6 us */
>> + sda_falling_time,
>> + 0, /* 0: DW default,
>> 1: Ideal */
>> + 0); /* No offset */
>> + lcnt = i2c_dw_scl_lcnt(i2c_dw_clk_rate(dev),
>> + 1300, /* tLOW = 1.3 us
>> */
>> + scl_falling_time,
>> + 0); /* No offset */
>> + }
>> + dw_writel(dev, hcnt, DW_IC_FS_SCL_HCNT);
>> + dw_writel(dev, lcnt, DW_IC_FS_SCL_LCNT);
>> + dev_dbg(dev->dev, "Fast-mode HCNT:LCNT = %d:%d\n", hcnt,
>> lcnt);
>> +
>> + if ((dev->slave_cfg & DW_IC_CON_SPEED_MASK) ==
>> + DW_IC_CON_SPEED_HIGH) {
>> + if ((comp_param1 &
>> DW_IC_COMP_PARAM_1_SPEED_MODE_MASK)
>> + != DW_IC_COMP_PARAM_1_SPEED_MODE_HIGH) {
>> + dev_err(dev->dev, "High Speed not
>> supported!\n");
>> + dev->slave_cfg &= ~DW_IC_CON_SPEED_MASK;
>> + dev->slave_cfg |= DW_IC_CON_SPEED_FAST;
>> + } else if (dev->hs_hcnt && dev->hs_lcnt) {
>> + hcnt = dev->hs_hcnt;
>> + lcnt = dev->hs_lcnt;
>> + dw_writel(dev, hcnt, DW_IC_HS_SCL_HCNT);
>> + dw_writel(dev, lcnt, DW_IC_HS_SCL_LCNT);
>> + dev_dbg(dev->dev, "HighSpeed-mode HCNT:LCNT =
>> %d:%d\n",
>> + hcnt, lcnt);
>> + }
>> + }
>> +
>> + /* Configure SDA Hold Time if required. */
>> + reg = dw_readl(dev, DW_IC_COMP_VERSION);
>> + if (reg >= DW_IC_SDA_HOLD_MIN_VERS) {
>> + if (!dev->sda_hold_time) {
>> + /* Keep previous hold time setting if no one
>> set it. */
>> + dev->sda_hold_time = dw_readl(dev,
>> DW_IC_SDA_HOLD);
>> + }
>> + /*
>> + * Workaround for avoiding TX arbitration lost in
>> case I2C
>> + * slave pulls SDA down "too quickly" after falling
>> egde of
>> + * SCL by enabling non-zero SDA RX hold.
>> Specification says it
>> + * extends incoming SDA low to high transition while
>> SCL is
>> + * high but it apprears to help also above issue.
>> + */
>> + if (!(dev->sda_hold_time & DW_IC_SDA_HOLD_RX_MASK))
>> + dev->sda_hold_time |= 1 <<
>> DW_IC_SDA_HOLD_RX_SHIFT;
>> + dw_writel(dev, dev->sda_hold_time, DW_IC_SDA_HOLD);
>> + } else {
>> + dev_warn(dev->dev,
>> + "Hardware too old to adjust SDA hold
>> time.\n");
>> + }
>> +
>> + i2c_dw_configure_fifo_slave(dev);
>> + i2c_dw_release_lock(dev);
>> +
>> + return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(i2c_dw_init_slave);
>
> Can we introduce ops structure for those? (private callbacks)
>
I will check that.
> You increase noise in namespace by several i2c_dw_*() functions.
> Introduction of ops will keep everything private.
>
>
>> +
>> +int i2c_dw_reg_slave(struct i2c_client *slave)
>> +{
>> + struct dw_i2c_dev *dev = i2c_get_adapdata(slave->adapter);
>> +
>> + if (dev->slave)
>> + return -EBUSY;
>> + if (slave->flags & I2C_CLIENT_TEN)
>> + return -EAFNOSUPPORT;
>
>> + /*
>> + * Set slave address in the IC_SAR register,
>> + * the address to which the DW_apb_i2c responds.
>> + */
>
> Wrong indentation?
>
Yes, I will fix it.
>> +
>> + __i2c_dw_enable(dev, false);
>> + dw_writel(dev, slave->addr, DW_IC_SAR);
>> + dev->slave = slave;
>> +
>> + __i2c_dw_enable(dev, true);
>> +
>> + dev->cmd_err = 0;
>> + dev->msg_write_idx = 0;
>> + dev->msg_read_idx = 0;
>> + dev->msg_err = 0;
>> + dev->status = STATUS_IDLE;
>> + dev->abort_source = 0;
>> + dev->rx_outstanding = 0;
>> +
>> + return 0;
>> +}
>> +
>> +static int i2c_dw_unreg_slave(struct i2c_client *slave)
>> +{
>> + struct dw_i2c_dev *dev = i2c_get_adapdata(slave->adapter);
>> +
>> + i2c_dw_disable_int_slave(dev);
>> + i2c_dw_disable_slave(dev);
>
>> + dev->slave = NULL;
>
> Extra spaces, remove.
>
>> +
>> + return 0;
>> +}
>>
>
>> +static int i2c_dw_irq_handler_slave(struct dw_i2c_dev *dev)
>> +{
>> + u32 raw_stat, stat, enabled;
>> + u8 val, slave_activity;
>> +
>> + stat = dw_readl(dev, DW_IC_INTR_STAT);
>> + enabled = dw_readl(dev, DW_IC_ENABLE);
>> + raw_stat = dw_readl(dev, DW_IC_RAW_INTR_STAT);
>> + slave_activity = ((dw_readl(dev, DW_IC_STATUS) &
>> + DW_IC_STATUS_SLAVE_ACTIVITY) >> 6);
>> +
>> + if (!enabled || !(raw_stat & ~DW_IC_INTR_ACTIVITY))
>> + return 0;
>> +
>> + dev_dbg(dev->dev,
>> + "%s: %#x SLAVE_ACTV=%#x : RAW_INTR_STAT=%#x :
>> INTR_STAT=%#x\n",
>> + __func__, enabled, slave_activity, raw_stat, stat);
>
> __func__ is redundant.
>
>> +
>> + if (stat & DW_IC_INTR_RESTART_DET)
>> + dw_readl(dev, DW_IC_CLR_RESTART_DET);
>> + if (stat & DW_IC_INTR_START_DET)
>> + dw_readl(dev, DW_IC_CLR_START_DET);
>> + if (stat & DW_IC_INTR_ACTIVITY)
>> + dw_readl(dev, DW_IC_CLR_ACTIVITY);
>> + if (stat & DW_IC_INTR_RX_OVER)
>> + dw_readl(dev, DW_IC_CLR_RX_OVER);
>> + if ((stat & DW_IC_INTR_RX_FULL) && (stat &
>> DW_IC_INTR_STOP_DET))
>> + i2c_slave_event(dev->slave,
>> I2C_SLAVE_WRITE_REQUESTED, &val);
>> +
>> + if (slave_activity) {
>> + if (stat & DW_IC_INTR_RD_REQ) {
>> + if (stat & DW_IC_INTR_RX_FULL) {
>> + val = dw_readl(dev, DW_IC_DATA_CMD);
>> + if (!i2c_slave_event(dev->slave,
>> + I2C_SLAVE_WRITE_RECEIVED, &val)) {
>> + dev_dbg(dev->dev, "Byte %X
>> acked!",
>> + val);
>
> Perhaps dev_vdbg() ?
>
>> + }
>> + dw_readl(dev, DW_IC_CLR_RD_REQ);
>> + stat =
>> i2c_dw_read_clear_intrbits_slave(dev);
>> + } else {
>> + dw_readl(dev, DW_IC_CLR_RD_REQ);
>> + dw_readl(dev, DW_IC_CLR_RX_UNDER);
>> + stat =
>> i2c_dw_read_clear_intrbits_slave(dev);
>> + }
>> + if (!i2c_slave_event(dev->slave,
>> + I2C_SLAVE_READ_REQUESTED,
>> &val))
>> + dw_writel(dev, val, DW_IC_DATA_CMD);
>> + }
>> + }
>> +
>> + if (stat & DW_IC_INTR_RX_DONE) {
>> + if (!i2c_slave_event(dev->slave,
>> I2C_SLAVE_READ_PROCESSED,
>> + &val))
>> + dw_readl(dev, DW_IC_CLR_RX_DONE);
>> +
>> + i2c_slave_event(dev->slave, I2C_SLAVE_STOP, &val);
>> + stat = i2c_dw_read_clear_intrbits_slave(dev);
>
>> + return true;
>
> Mistype of value. Should be 1?
Yes:
i2c_slave_event(dev->slave,I2C_SLAVE_READ_PROCESSED) always returns 0 and
updates &val. I can not use "if" if you think its better.
>
>> + }
>> +
>> + if (stat & DW_IC_INTR_RX_FULL) {
>> + val = dw_readl(dev, DW_IC_DATA_CMD);
>> + if (!i2c_slave_event(dev->slave,
>> I2C_SLAVE_WRITE_RECEIVED,
>> + &val))
>> + dev_dbg(dev->dev, "Byte %X acked!", val);
>
> dev_vdbg() ?
>
>> + } else {
>> + i2c_slave_event(dev->slave, I2C_SLAVE_STOP, &val);
>> + stat = i2c_dw_read_clear_intrbits_slave(dev);
>> + }
>> +
>> + if (stat & DW_IC_INTR_TX_OVER)
>> + dw_readl(dev, DW_IC_CLR_TX_OVER);
>> +
>> + return 1;
>> +}
>> +
>> +static irqreturn_t i2c_dw_isr_slave(int this_irq, void *dev_id)
>> +{
>> + struct dw_i2c_dev *dev = dev_id;
>> + int ret;
>> +
>> + i2c_dw_read_clear_intrbits_slave(dev);
>>
>
>> + ret = i2c_dw_irq_handler_slave(dev);
>> +
>
> Swap these lines.
>
>> + if (ret > 0)
>> + complete(&dev->cmd_complete);
>> +
>> + return IRQ_RETVAL(ret);
>> +}
>
>
>> +
>> +int i2c_dw_probe_slave(struct dw_i2c_dev *dev)
>> +{
>> + struct i2c_adapter *adap = &dev->adapter;
>> + int ret;
>> +
>> + init_completion(&dev->cmd_complete);
>> +
>> + ret = i2c_dw_init_slave(dev);
>> + if (ret)
>> + return ret;
>> +
>>
>
>> + ret = i2c_dw_acquire_lock(dev);
>> + if (ret)
>> + return ret;
>
> I'm not sure you need this in slave code.
I will check that.
>
>> +
>> + i2c_dw_release_lock(dev);
>> + snprintf(adap->name, sizeof(adap->name),
>> + "Synopsys DesignWare I2C Slave adapter");
>> + adap->retries = 3;
>> + adap->algo = &i2c_dw_algo;
>> + adap->dev.parent = dev->dev;
>> + i2c_set_adapdata(adap, dev);
>> +
>> + ret = devm_request_irq(dev->dev, dev->irq, i2c_dw_isr_slave,
>> + IRQF_SHARED, dev_name(dev->dev), dev);
>> + if (ret) {
>> + dev_err(dev->dev, "failure requesting irq %i: %d\n",
>> + dev->irq, ret);
>> + return ret;
>> + }
>
>> + /*
>> + * Increment PM usage count during adapter registration in
>> order to
>> + * avoid possible spurious runtime suspend when adapter
>> device is
>> + * registered to the device core and immediate resume in case
>> bus has
>> + * registered I2C slaves that do I2C transfers in their
>> probe.
>> + */
>> + pm_runtime_get_noresume(dev->dev);
>
> Looks like you blindly copied this from master code. This is about slave
> enumeration. How does it related to slave mode?
Yes, huge mistake. Sorry
>
>> + ret = i2c_add_numbered_adapter(adap);
>> + if (ret)
>> + dev_err(dev->dev, "failure adding adapter: %d\n",
>> ret);
>> + pm_runtime_put_noidle(dev->dev);
>> +
>> + return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(i2c_dw_probe_slave);
>
>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v5 6/7] i2c: designware: enable SLAVE in platform module
From: Luis Oliveira @ 2016-12-28 15:53 UTC (permalink / raw)
To: Andy Shevchenko, Luis Oliveira, wsa-z923LK4zBo2bacvFa/9K2g,
robh+dt-DgEjT+Ai2ygdnm+yROfE0A, mark.rutland-5wv7dgnIgG8,
jarkko.nikula-VuQAYsv1563Yd54FQh9/CA,
mika.westerberg-VuQAYsv1563Yd54FQh9/CA,
linux-i2c-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
Cc: Ramiro.Oliveira-HKixBCOQz3hWk0Htik3J/w,
Joao.Pinto-HKixBCOQz3hWk0Htik3J/w,
CARLOS.PALMINHA-HKixBCOQz3hWk0Htik3J/w
In-Reply-To: <1482939844.9552.165.camel-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
On 28-Dec-16 15:44, Andy Shevchenko wrote:
> On Wed, 2016-12-28 at 14:43 +0000, Luis Oliveira wrote:
>> - Slave mode selected in platform module (devicetree support only)
>> - Check for ACPI - not supported in SLAVE mode:
>> - Changed the ifndef style to the use of ACPI_HANDLE that returns
>> NULL
>> if the device was not enumerated from ACPI namespace.
>
> I'm not sure what is wrong with ACPI?
I dont have a way to test it. Just that.
>
>> @@ -264,9 +297,16 @@ static int dw_i2c_plat_probe(struct
>> platform_device *pdev)
>> if (r)
>> return r;
>>
>> - dev->functionality = I2C_FUNC_10BIT_ADDR |
>> DW_IC_DEFAULT_FUNCTIONALITY;
>> -
>> - i2c_dw_configure_master(pdev);
>
>> + if (ACPI_HANDLE(&pdev->dev) == NULL) {
>
> I don't think you need this at all.
This is to avoid the use of the "ifdef" style I used before.
>
>> + device_for_each_child_node(&pdev->dev, child) {
>
> This is resource agnostic.
>
>> + fwnode_property_read_u32(child, "reg", ®);
>
> This is as well.
Are you suggesting I use of_ functions?
>
>> + if (reg & I2C_OWN_SLAVE_ADDRESS)
>> + i2c_dw_configure_slave(pdev);
>> + else
>> + i2c_dw_configure_master(pdev);
>> + }
>> + } else
>> + i2c_dw_configure_master(pdev);
>
>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v5 7/7] i2c: designware: style changes in existing code
From: Andy Shevchenko @ 2016-12-28 15:50 UTC (permalink / raw)
To: Luis Oliveira, wsa-z923LK4zBo2bacvFa/9K2g,
robh+dt-DgEjT+Ai2ygdnm+yROfE0A, mark.rutland-5wv7dgnIgG8,
jarkko.nikula-VuQAYsv1563Yd54FQh9/CA,
mika.westerberg-VuQAYsv1563Yd54FQh9/CA,
linux-i2c-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
Cc: Ramiro.Oliveira-HKixBCOQz3hWk0Htik3J/w,
Joao.Pinto-HKixBCOQz3hWk0Htik3J/w,
CARLOS.PALMINHA-HKixBCOQz3hWk0Htik3J/w
In-Reply-To: <4ff96e7320ebaef28c0ccb47c29bd9a2fe16027c.1482934380.git.lolivei-HKixBCOQz3hWk0Htik3J/w@public.gmane.org>
On Wed, 2016-12-28 at 14:43 +0000, Luis Oliveira wrote:
> Replaced all the return variables 'r' in the existing
> code by 'ret' to make the code easier to read (and
> more standard).
I'm not sure it makes sense as a separate change.
>
> Signed-off-by: Luis Oliveira <lolivei-HKixBCOQz3hWk0Htik3J/w@public.gmane.org>
> ---
> Changes V4->V5: (Andy Shevchenko)
> - Replaced all the old code using "r" as return to "ret". For
> consistency
> purposes.
>
> drivers/i2c/busses/i2c-designware-master.c | 30 ++++++++++++++----
> -----------
> drivers/i2c/busses/i2c-designware-platdrv.c | 16 +++++++--------
> 2 files changed, 23 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-designware-master.c
> b/drivers/i2c/busses/i2c-designware-master.c
> index b55a7f4c5149..0d5aca6edb48 100644
> --- a/drivers/i2c/busses/i2c-designware-master.c
> +++ b/drivers/i2c/busses/i2c-designware-master.c
> @@ -644,18 +644,18 @@ EXPORT_SYMBOL_GPL(i2c_dw_read_comp_param);
> int i2c_dw_probe(struct dw_i2c_dev *dev)
> {
> struct i2c_adapter *adap = &dev->adapter;
> - int r;
> + int ret;
> u32 reg;
>
> init_completion(&dev->cmd_complete);
>
> - r = i2c_dw_init(dev);
> - if (r)
> - return r;
> + ret = i2c_dw_init(dev);
> + if (ret)
> + return ret;
>
> - r = i2c_dw_acquire_lock(dev);
> - if (r)
> - return r;
> + ret = i2c_dw_acquire_lock(dev);
> + if (ret)
> + return ret;
>
> /*
> * Test if dynamic TAR update is enabled in this controller
> by writing
> @@ -681,13 +681,13 @@ int i2c_dw_probe(struct dw_i2c_dev *dev)
> i2c_set_adapdata(adap, dev);
>
> i2c_dw_disable_int(dev);
> - r = devm_request_irq(dev->dev, dev->irq, i2c_dw_isr,
> + ret = devm_request_irq(dev->dev, dev->irq, i2c_dw_isr,
> IRQF_SHARED | IRQF_COND_SUSPEND,
> dev_name(dev->dev), dev);
> - if (r) {
> + if (ret) {
> dev_err(dev->dev, "failure requesting irq %i: %d\n",
> - dev->irq, r);
> - return r;
> + dev->irq, ret);
> + return ret;
> }
>
> /*
> @@ -697,12 +697,12 @@ int i2c_dw_probe(struct dw_i2c_dev *dev)
> * registered I2C slaves that do I2C transfers in their
> probe.
> */
> pm_runtime_get_noresume(dev->dev);
> - r = i2c_add_numbered_adapter(adap);
> - if (r)
> - dev_err(dev->dev, "failure adding adapter: %d\n", r);
> + ret = i2c_add_numbered_adapter(adap);
> + if (ret)
> + dev_err(dev->dev, "failure adding adapter: %d\n",
> ret);
> pm_runtime_put_noidle(dev->dev);
>
> - return r;
> + return ret;
> }
> EXPORT_SYMBOL_GPL(i2c_dw_probe);
>
> diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c
> b/drivers/i2c/busses/i2c-designware-platdrv.c
> index ef75031f8a62..785f4380c9a9 100644
> --- a/drivers/i2c/busses/i2c-designware-platdrv.c
> +++ b/drivers/i2c/busses/i2c-designware-platdrv.c
> @@ -234,7 +234,7 @@ static int dw_i2c_plat_probe(struct
> platform_device *pdev)
> struct fwnode_handle *child;
> u32 acpi_speed, ht = 0;
> struct resource *mem;
> - int irq, r;
> + int irq, ret;
> u32 reg;
>
> irq = platform_get_irq(pdev, 0);
> @@ -293,9 +293,9 @@ static int dw_i2c_plat_probe(struct
> platform_device *pdev)
> return -EINVAL;
> }
>
> - r = i2c_dw_eval_lock_support(dev);
> - if (r)
> - return r;
> + ret = i2c_dw_eval_lock_support(dev);
> + if (ret)
> + return ret;
>
> if (ACPI_HANDLE(&pdev->dev) == NULL) {
> device_for_each_child_node(&pdev->dev, child) {
> @@ -336,14 +336,14 @@ static int dw_i2c_plat_probe(struct
> platform_device *pdev)
> }
>
> if (dev->mode == DW_IC_SLAVE)
> - r = i2c_dw_probe_slave(dev);
> + ret = i2c_dw_probe_slave(dev);
> else
> - r = i2c_dw_probe(dev);
> + ret = i2c_dw_probe(dev);
>
> - if (r && !dev->pm_runtime_disabled)
> + if (ret && !dev->pm_runtime_disabled)
> pm_runtime_disable(&pdev->dev);
>
> - return r;
> + return ret;
> }
>
> static int dw_i2c_plat_remove(struct platform_device *pdev)
--
Andy Shevchenko <andriy.shevchenko-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
Intel Finland Oy
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v5 6/7] i2c: designware: enable SLAVE in platform module
From: Andy Shevchenko @ 2016-12-28 15:44 UTC (permalink / raw)
To: Luis Oliveira, wsa, robh+dt, mark.rutland, jarkko.nikula,
mika.westerberg, linux-i2c, devicetree, linux-kernel
Cc: Ramiro.Oliveira, Joao.Pinto, CARLOS.PALMINHA
In-Reply-To: <ea8b611c3b7c2af8088e2dbc16554af5c8aed368.1482934380.git.lolivei@synopsys.com>
On Wed, 2016-12-28 at 14:43 +0000, Luis Oliveira wrote:
> - Slave mode selected in platform module (devicetree support only)
> - Check for ACPI - not supported in SLAVE mode:
> - Changed the ifndef style to the use of ACPI_HANDLE that returns
> NULL
> if the device was not enumerated from ACPI namespace.
I'm not sure what is wrong with ACPI?
> @@ -264,9 +297,16 @@ static int dw_i2c_plat_probe(struct
> platform_device *pdev)
> if (r)
> return r;
>
> - dev->functionality = I2C_FUNC_10BIT_ADDR |
> DW_IC_DEFAULT_FUNCTIONALITY;
> -
> - i2c_dw_configure_master(pdev);
> + if (ACPI_HANDLE(&pdev->dev) == NULL) {
I don't think you need this at all.
> + device_for_each_child_node(&pdev->dev, child) {
This is resource agnostic.
> + fwnode_property_read_u32(child, "reg", ®);
This is as well.
> + if (reg & I2C_OWN_SLAVE_ADDRESS)
> + i2c_dw_configure_slave(pdev);
> + else
> + i2c_dw_configure_master(pdev);
> + }
> + } else
> + i2c_dw_configure_master(pdev);
--
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy
^ permalink raw reply
* Re: [PATCH v5 5/7] i2c: designware: add SLAVE mode functions
From: Andy Shevchenko @ 2016-12-28 15:36 UTC (permalink / raw)
To: Luis Oliveira, wsa, robh+dt, mark.rutland, jarkko.nikula,
mika.westerberg, linux-i2c, devicetree, linux-kernel
Cc: Ramiro.Oliveira, Joao.Pinto, CARLOS.PALMINHA
In-Reply-To: <15538bd29b3ab62608a4e9153af6ee5d4bdc79e2.1482934380.git.lolivei@synopsys.com>
On Wed, 2016-12-28 at 14:43 +0000, Luis Oliveira wrote:
> - Changes in Kconfig to enable I2C_SLAVE support
> - Slave functions added to core library file
> - Slave abort sources added to common source file
> - New driver: i2c-designware-slave added
> - Changes in the Makefile to compile it all
>
> All the SLAVE flow is added but it is not enabled via platform
> driver.
> --- a/drivers/i2c/busses/i2c-designware-common.c
> +++ b/drivers/i2c/busses/i2c-designware-common.c
> @@ -30,6 +30,7 @@
> #include <linux/pm_runtime.h>
> #include <linux/delay.h>
> #include <linux/module.h>
> +
> #include "i2c-designware-core.h"
>
> static char *abort_sources[] = {
> @@ -42,7 +43,7 @@ static char *abort_sources[] = {
> [ABRT_TXDATA_NOACK] =
> "data not acknowledged",
> [ABRT_GCALL_NOACK] =
> - "no acknowledgement for a general call",
> + "no acknowledgment for a general call",
So, what's the point after your confirmation that both variants are
okay?
> +#include <linux/errno.h>
> +#include <linux/err.h>
> +#include <linux/i2c.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/delay.h>
> +#include <linux/module.h>
Alphabetical order?
> +int i2c_dw_init_slave(struct dw_i2c_dev *dev)
> +{
> + u32 sda_falling_time, scl_falling_time;
> + u32 reg, comp_param1;
> + u32 hcnt, lcnt;
> + int ret;
> +
> + ret = i2c_dw_acquire_lock(dev);
> + if (ret)
> + return ret;
> +
> + reg = dw_readl(dev, DW_IC_COMP_TYPE);
> + if (reg == ___constant_swab32(DW_IC_COMP_TYPE_VALUE)) {
> + /* Configure register endianness access. */
> + dev->accessor_flags |= ACCESS_SWAP;
> + } else if (reg == (DW_IC_COMP_TYPE_VALUE & 0x0000ffff)) {
> + /* Configure register access mode 16bit. */
> + dev->accessor_flags |= ACCESS_16BIT;
> + } else if (reg != DW_IC_COMP_TYPE_VALUE) {
> + dev_err(dev->dev,
> + "Unknown Synopsys component type: 0x%08x\n",
> reg);
Is it correct indentation?
> + i2c_dw_release_lock(dev);
> + return -ENODEV;
> + }
> +
> + comp_param1 = dw_readl(dev, DW_IC_COMP_PARAM_1);
> +
> + /* Disable the adapter. */
> + __i2c_dw_enable_and_wait(dev, false);
> +
> + /* Set standard and fast speed deviders for high/low periods.
> */
> + sda_falling_time = dev->sda_falling_time ?: 300; /* ns */
> + scl_falling_time = dev->scl_falling_time ?: 300; /* ns */
> +
> + /* Set SCL timing parameters for standard-mode. */
> + if (dev->ss_hcnt && dev->ss_lcnt) {
> + hcnt = dev->ss_hcnt;
> + lcnt = dev->ss_lcnt;
> + } else {
> + hcnt = i2c_dw_scl_hcnt(i2c_dw_clk_rate(dev),
> + 4000, /* tHD;STA =
> tHIGH = 4.0 us */
> + sda_falling_time,
> + 0, /* 0: DW default,
> 1: Ideal */
> + 0); /* No offset */
> + lcnt = i2c_dw_scl_lcnt(i2c_dw_clk_rate(dev),
> + 4700, /* tLOW = 4.7 us
> */
> + scl_falling_time,
> + 0); /* No offset */
> + }
> + dw_writel(dev, hcnt, DW_IC_SS_SCL_HCNT);
> + dw_writel(dev, lcnt, DW_IC_SS_SCL_LCNT);
> + dev_dbg(dev->dev, "Standard-mode HCNT:LCNT = %d:%d\n", hcnt,
> lcnt);
> +
> + /* Set SCL timing parameters for fast-mode or fast-mode plus.
> */
> + if ((dev->clk_freq == 1000000) && dev->fp_hcnt && dev-
> >fp_lcnt) {
> + hcnt = dev->fp_hcnt;
> + lcnt = dev->fp_lcnt;
> + } else if (dev->fs_hcnt && dev->fs_lcnt) {
> + hcnt = dev->fs_hcnt;
> + lcnt = dev->fs_lcnt;
> + } else {
> + hcnt = i2c_dw_scl_hcnt(i2c_dw_clk_rate(dev),
> + 600, /* tHD;STA =
> tHIGH = 0.6 us */
> + sda_falling_time,
> + 0, /* 0: DW default,
> 1: Ideal */
> + 0); /* No offset */
> + lcnt = i2c_dw_scl_lcnt(i2c_dw_clk_rate(dev),
> + 1300, /* tLOW = 1.3 us
> */
> + scl_falling_time,
> + 0); /* No offset */
> + }
> + dw_writel(dev, hcnt, DW_IC_FS_SCL_HCNT);
> + dw_writel(dev, lcnt, DW_IC_FS_SCL_LCNT);
> + dev_dbg(dev->dev, "Fast-mode HCNT:LCNT = %d:%d\n", hcnt,
> lcnt);
> +
> + if ((dev->slave_cfg & DW_IC_CON_SPEED_MASK) ==
> + DW_IC_CON_SPEED_HIGH) {
> + if ((comp_param1 &
> DW_IC_COMP_PARAM_1_SPEED_MODE_MASK)
> + != DW_IC_COMP_PARAM_1_SPEED_MODE_HIGH) {
> + dev_err(dev->dev, "High Speed not
> supported!\n");
> + dev->slave_cfg &= ~DW_IC_CON_SPEED_MASK;
> + dev->slave_cfg |= DW_IC_CON_SPEED_FAST;
> + } else if (dev->hs_hcnt && dev->hs_lcnt) {
> + hcnt = dev->hs_hcnt;
> + lcnt = dev->hs_lcnt;
> + dw_writel(dev, hcnt, DW_IC_HS_SCL_HCNT);
> + dw_writel(dev, lcnt, DW_IC_HS_SCL_LCNT);
> + dev_dbg(dev->dev, "HighSpeed-mode HCNT:LCNT =
> %d:%d\n",
> + hcnt, lcnt);
> + }
> + }
> +
> + /* Configure SDA Hold Time if required. */
> + reg = dw_readl(dev, DW_IC_COMP_VERSION);
> + if (reg >= DW_IC_SDA_HOLD_MIN_VERS) {
> + if (!dev->sda_hold_time) {
> + /* Keep previous hold time setting if no one
> set it. */
> + dev->sda_hold_time = dw_readl(dev,
> DW_IC_SDA_HOLD);
> + }
> + /*
> + * Workaround for avoiding TX arbitration lost in
> case I2C
> + * slave pulls SDA down "too quickly" after falling
> egde of
> + * SCL by enabling non-zero SDA RX hold.
> Specification says it
> + * extends incoming SDA low to high transition while
> SCL is
> + * high but it apprears to help also above issue.
> + */
> + if (!(dev->sda_hold_time & DW_IC_SDA_HOLD_RX_MASK))
> + dev->sda_hold_time |= 1 <<
> DW_IC_SDA_HOLD_RX_SHIFT;
> + dw_writel(dev, dev->sda_hold_time, DW_IC_SDA_HOLD);
> + } else {
> + dev_warn(dev->dev,
> + "Hardware too old to adjust SDA hold
> time.\n");
> + }
> +
> + i2c_dw_configure_fifo_slave(dev);
> + i2c_dw_release_lock(dev);
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(i2c_dw_init_slave);
Can we introduce ops structure for those? (private callbacks)
You increase noise in namespace by several i2c_dw_*() functions.
Introduction of ops will keep everything private.
> +
> +int i2c_dw_reg_slave(struct i2c_client *slave)
> +{
> + struct dw_i2c_dev *dev = i2c_get_adapdata(slave->adapter);
> +
> + if (dev->slave)
> + return -EBUSY;
> + if (slave->flags & I2C_CLIENT_TEN)
> + return -EAFNOSUPPORT;
> + /*
> + * Set slave address in the IC_SAR register,
> + * the address to which the DW_apb_i2c responds.
> + */
Wrong indentation?
> +
> + __i2c_dw_enable(dev, false);
> + dw_writel(dev, slave->addr, DW_IC_SAR);
> + dev->slave = slave;
> +
> + __i2c_dw_enable(dev, true);
> +
> + dev->cmd_err = 0;
> + dev->msg_write_idx = 0;
> + dev->msg_read_idx = 0;
> + dev->msg_err = 0;
> + dev->status = STATUS_IDLE;
> + dev->abort_source = 0;
> + dev->rx_outstanding = 0;
> +
> + return 0;
> +}
> +
> +static int i2c_dw_unreg_slave(struct i2c_client *slave)
> +{
> + struct dw_i2c_dev *dev = i2c_get_adapdata(slave->adapter);
> +
> + i2c_dw_disable_int_slave(dev);
> + i2c_dw_disable_slave(dev);
> + dev->slave = NULL;
Extra spaces, remove.
> +
> + return 0;
> +}
>
> +static int i2c_dw_irq_handler_slave(struct dw_i2c_dev *dev)
> +{
> + u32 raw_stat, stat, enabled;
> + u8 val, slave_activity;
> +
> + stat = dw_readl(dev, DW_IC_INTR_STAT);
> + enabled = dw_readl(dev, DW_IC_ENABLE);
> + raw_stat = dw_readl(dev, DW_IC_RAW_INTR_STAT);
> + slave_activity = ((dw_readl(dev, DW_IC_STATUS) &
> + DW_IC_STATUS_SLAVE_ACTIVITY) >> 6);
> +
> + if (!enabled || !(raw_stat & ~DW_IC_INTR_ACTIVITY))
> + return 0;
> +
> + dev_dbg(dev->dev,
> + "%s: %#x SLAVE_ACTV=%#x : RAW_INTR_STAT=%#x :
> INTR_STAT=%#x\n",
> + __func__, enabled, slave_activity, raw_stat, stat);
__func__ is redundant.
> +
> + if (stat & DW_IC_INTR_RESTART_DET)
> + dw_readl(dev, DW_IC_CLR_RESTART_DET);
> + if (stat & DW_IC_INTR_START_DET)
> + dw_readl(dev, DW_IC_CLR_START_DET);
> + if (stat & DW_IC_INTR_ACTIVITY)
> + dw_readl(dev, DW_IC_CLR_ACTIVITY);
> + if (stat & DW_IC_INTR_RX_OVER)
> + dw_readl(dev, DW_IC_CLR_RX_OVER);
> + if ((stat & DW_IC_INTR_RX_FULL) && (stat &
> DW_IC_INTR_STOP_DET))
> + i2c_slave_event(dev->slave,
> I2C_SLAVE_WRITE_REQUESTED, &val);
> +
> + if (slave_activity) {
> + if (stat & DW_IC_INTR_RD_REQ) {
> + if (stat & DW_IC_INTR_RX_FULL) {
> + val = dw_readl(dev, DW_IC_DATA_CMD);
> + if (!i2c_slave_event(dev->slave,
> + I2C_SLAVE_WRITE_RECEIVED, &val)) {
> + dev_dbg(dev->dev, "Byte %X
> acked!",
> + val);
Perhaps dev_vdbg() ?
> + }
> + dw_readl(dev, DW_IC_CLR_RD_REQ);
> + stat =
> i2c_dw_read_clear_intrbits_slave(dev);
> + } else {
> + dw_readl(dev, DW_IC_CLR_RD_REQ);
> + dw_readl(dev, DW_IC_CLR_RX_UNDER);
> + stat =
> i2c_dw_read_clear_intrbits_slave(dev);
> + }
> + if (!i2c_slave_event(dev->slave,
> + I2C_SLAVE_READ_REQUESTED,
> &val))
> + dw_writel(dev, val, DW_IC_DATA_CMD);
> + }
> + }
> +
> + if (stat & DW_IC_INTR_RX_DONE) {
> + if (!i2c_slave_event(dev->slave,
> I2C_SLAVE_READ_PROCESSED,
> + &val))
> + dw_readl(dev, DW_IC_CLR_RX_DONE);
> +
> + i2c_slave_event(dev->slave, I2C_SLAVE_STOP, &val);
> + stat = i2c_dw_read_clear_intrbits_slave(dev);
> + return true;
Mistype of value. Should be 1?
> + }
> +
> + if (stat & DW_IC_INTR_RX_FULL) {
> + val = dw_readl(dev, DW_IC_DATA_CMD);
> + if (!i2c_slave_event(dev->slave,
> I2C_SLAVE_WRITE_RECEIVED,
> + &val))
> + dev_dbg(dev->dev, "Byte %X acked!", val);
dev_vdbg() ?
> + } else {
> + i2c_slave_event(dev->slave, I2C_SLAVE_STOP, &val);
> + stat = i2c_dw_read_clear_intrbits_slave(dev);
> + }
> +
> + if (stat & DW_IC_INTR_TX_OVER)
> + dw_readl(dev, DW_IC_CLR_TX_OVER);
> +
> + return 1;
> +}
> +
> +static irqreturn_t i2c_dw_isr_slave(int this_irq, void *dev_id)
> +{
> + struct dw_i2c_dev *dev = dev_id;
> + int ret;
> +
> + i2c_dw_read_clear_intrbits_slave(dev);
>
> + ret = i2c_dw_irq_handler_slave(dev);
> +
Swap these lines.
> + if (ret > 0)
> + complete(&dev->cmd_complete);
> +
> + return IRQ_RETVAL(ret);
> +}
> +
> +int i2c_dw_probe_slave(struct dw_i2c_dev *dev)
> +{
> + struct i2c_adapter *adap = &dev->adapter;
> + int ret;
> +
> + init_completion(&dev->cmd_complete);
> +
> + ret = i2c_dw_init_slave(dev);
> + if (ret)
> + return ret;
> +
>
> + ret = i2c_dw_acquire_lock(dev);
> + if (ret)
> + return ret;
I'm not sure you need this in slave code.
> +
> + i2c_dw_release_lock(dev);
> + snprintf(adap->name, sizeof(adap->name),
> + "Synopsys DesignWare I2C Slave adapter");
> + adap->retries = 3;
> + adap->algo = &i2c_dw_algo;
> + adap->dev.parent = dev->dev;
> + i2c_set_adapdata(adap, dev);
> +
> + ret = devm_request_irq(dev->dev, dev->irq, i2c_dw_isr_slave,
> + IRQF_SHARED, dev_name(dev->dev), dev);
> + if (ret) {
> + dev_err(dev->dev, "failure requesting irq %i: %d\n",
> + dev->irq, ret);
> + return ret;
> + }
> + /*
> + * Increment PM usage count during adapter registration in
> order to
> + * avoid possible spurious runtime suspend when adapter
> device is
> + * registered to the device core and immediate resume in case
> bus has
> + * registered I2C slaves that do I2C transfers in their
> probe.
> + */
> + pm_runtime_get_noresume(dev->dev);
Looks like you blindly copied this from master code. This is about slave
enumeration. How does it related to slave mode?
> + ret = i2c_add_numbered_adapter(adap);
> + if (ret)
> + dev_err(dev->dev, "failure adding adapter: %d\n",
> ret);
> + pm_runtime_put_noidle(dev->dev);
> +
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(i2c_dw_probe_slave);
--
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy
^ permalink raw reply
* Re: [PATCH v5 2/7] i2c: designware: refactoring of the i2c-designware
From: Luis Oliveira @ 2016-12-28 15:30 UTC (permalink / raw)
To: Andy Shevchenko, Luis Oliveira, wsa, robh+dt, mark.rutland,
jarkko.nikula, mika.westerberg, linux-i2c, devicetree,
linux-kernel
Cc: Ramiro.Oliveira, Joao.Pinto, CARLOS.PALMINHA
In-Reply-To: <1482937968.9552.159.camel@linux.intel.com>
On 28-Dec-16 15:12, Andy Shevchenko wrote:
> On Wed, 2016-12-28 at 14:43 +0000, Luis Oliveira wrote:
>> - Factor out all _master() part of code from i2c-designware-core
>> and i2c-designware-platdrv to separate functions.
>> - Standardize all code related with MASTER mode.
>> - I have to take off DW_IC_INTR_TX_EMPTY from DW_IC_INTR_DEFAULT_MASK
>> because it is master specific.
>>
>> The purpose of this is to prepare the controller to have is I2C MASTER
>> flow in a separate driver. To do this first all the
>> functions/definitions related to the MASTER flow were identified.
>
> Thanks for an update.
> Some style related comments below (For the code related is up to you, my
> tag still stands).
>
>>
>> Signed-off-by: Luis Oliveira <lolivei@synopsys.com>
>> ---
>> Changes V4->V5: (ACK by Andy)
>
> When you get an Ack, or other tag (Reviewed-by, Tested-by, etc), and you
> send new version, include this tag to your commit message (it applies to
> all affected patches in your series).
>
Thank you. I didn't knew.
> It would be also good to have some high level changelog in the cover
> letter, from this series I don't see, for example, which base you did
> use (i2c-next? linux-next? v4.9? v4.10-rc1?).
>
>> + dev_dbg(dev->dev,
>> + "%s: enabled=%#x stat=%#x\n", __func__, enabled,
> stat);
>
> I hope you can fit format string on the first line. __func__ is
> redundant when you are using debug printing (Dynamic Debug would include
> it if asked for).
I will check that.
>
>> +static void i2c_dw_configure_master(struct platform_device *pdev)
>> +{
>> + struct dw_i2c_dev *dev = platform_get_drvdata(pdev);
>
> By the way, does it make sense to pass struct dw_i2c_dev * as a
> parameter of the function?
>
Yes, by looking at it now I think I can pass just the struct dw_i2c_dev
to this function. And probably the same with the i2c_dw_configure_slave.
>> +
>> + dev->master_cfg = DW_IC_CON_MASTER | DW_IC_CON_SLAVE_DISABLE
>> |
>> + DW_IC_CON_RESTART_EN;
>> +
>> + dev_dbg(&pdev->dev, "I am registed as a I2C Master!\n");
>> +
>> + switch (dev->clk_freq) {
>> + case 100000:
>> + dev->master_cfg |= DW_IC_CON_SPEED_STD;
>> + break;
>> + case 3400000:
>> + dev->master_cfg |= DW_IC_CON_SPEED_HIGH;
>> + break;
>> + default:
>> + dev->master_cfg |= DW_IC_CON_SPEED_FAST;
>> + }
>> +}
>> +
>>
>
>
^ permalink raw reply
* Re: [PATCH v5 4/7] i2c: designware: introducing I2C_SLAVE definitions
From: Andy Shevchenko @ 2016-12-28 15:17 UTC (permalink / raw)
To: Luis Oliveira, wsa, robh+dt, mark.rutland, jarkko.nikula,
mika.westerberg, linux-i2c, devicetree, linux-kernel
Cc: Ramiro.Oliveira, Joao.Pinto, CARLOS.PALMINHA
In-Reply-To: <08964037b735ed646a5bd56e29c7922d6172a1dd.1482934380.git.lolivei@synopsys.com>
On Wed, 2016-12-28 at 14:43 +0000, Luis Oliveira wrote:
> - Definitions were added
>
> SLAVE related definitions were added to the core of the controller.
>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Signed-off-by: Luis Oliveira <lolivei@synopsys.com>
> ---
> Changes V4->V5: (Andy Shevchenko)
> - This patch just introduces SLAVE definitions (as suggested in V4)
>
> drivers/i2c/busses/i2c-designware-core.h | 27
> +++++++++++++++++++++++++++
> 1 file changed, 27 insertions(+)
>
> diff --git a/drivers/i2c/busses/i2c-designware-core.h
> b/drivers/i2c/busses/i2c-designware-core.h
> index 8bba7a37c3ce..5080f1d2d2ec 100644
> --- a/drivers/i2c/busses/i2c-designware-core.h
> +++ b/drivers/i2c/busses/i2c-designware-core.h
> @@ -36,15 +36,20 @@
> #define DW_IC_CON_SPEED_FAST 0x4
> #define DW_IC_CON_SPEED_HIGH 0x6
> #define DW_IC_CON_SPEED_MASK 0x6
> +#define DW_IC_CON_10BITADDR_SLAVE 0x8
> #define DW_IC_CON_10BITADDR_MASTER 0x10
> #define DW_IC_CON_RESTART_EN 0x20
> #define DW_IC_CON_SLAVE_DISABLE 0x40
> +#define DW_IC_CON_STOP_DET_IFADDRESSED 0x80
> +#define DW_IC_CON_TX_EMPTY_CTRL 0x100
> +#define DW_IC_CON_RX_FIFO_FULL_HLD_CTRL 0x200
>
> /*
> * Registers offset
> */
> #define DW_IC_CON 0x0
> #define DW_IC_TAR 0x4
> +#define DW_IC_SAR 0x8
> #define DW_IC_DATA_CMD 0x10
> #define DW_IC_SS_SCL_HCNT 0x14
> #define DW_IC_SS_SCL_LCNT 0x18
> @@ -75,6 +80,7 @@
> #define DW_IC_SDA_HOLD 0x7c
> #define DW_IC_TX_ABRT_SOURCE 0x80
> #define DW_IC_ENABLE_STATUS 0x9c
> +#define DW_IC_CLR_RESTART_DET 0xa8
> #define DW_IC_COMP_PARAM_1 0xf4
> #define DW_IC_COMP_VERSION 0xf8
> #define DW_IC_SDA_HOLD_MIN_VERS 0x3131312A
> @@ -93,15 +99,22 @@
> #define DW_IC_INTR_STOP_DET 0x200
> #define DW_IC_INTR_START_DET 0x400
> #define DW_IC_INTR_GEN_CALL 0x800
> +#define DW_IC_INTR_RESTART_DET 0x1000
>
> #define DW_IC_INTR_DEFAULT_MASK (DW_IC_INTR_RX_FULL |
> \
> DW_IC_INTR_TX_ABRT | \
> DW_IC_INTR_STOP_DET)
> #define DW_IC_INTR_MASTER_MASK (DW_IC_INTR_DEFAULT_MAS
> K | \
> DW_IC_INTR_TX_EMPTY)
> +#define DW_IC_INTR_SLAVE_MASK (DW_IC_INTR_DEFAULT_MASK
> | \
> + DW_IC_INTR_RX_DONE | \
> + DW_IC_INTR_RX_UNDER | \
> + DW_IC_INTR_RD_REQ)
> +
> #define DW_IC_STATUS_ACTIVITY 0x1
> #define DW_IC_STATUS_TFE BIT(2)
> #define DW_IC_STATUS_MASTER_ACTIVITY BIT(5)
> +#define DW_IC_STATUS_SLAVE_ACTIVITY BIT(6)
>
> #define DW_IC_SDA_HOLD_RX_SHIFT 16
> #define DW_IC_SDA_HOLD_RX_MASK GENMASK(23,
> DW_IC_SDA_HOLD_RX_SHIFT)
> @@ -123,6 +136,12 @@
> #define TIMEOUT 20 /* ms */
>
> /*
> + * operation modes
> + */
> +#define DW_IC_MASTER 0
> +#define DW_IC_SLAVE 1
> +
> +/*
> * hardware abort codes from the DW_IC_TX_ABRT_SOURCE register
> *
> * only expected abort codes are listed here
> @@ -139,6 +158,9 @@
> #define ABRT_10B_RD_NORSTRT 10
> #define ABRT_MASTER_DIS 11
> #define ARB_LOST 12
> +#define ABRT_SLAVE_FLUSH_TXFIFO 13
> +#define ABRT_SLAVE_ARBLOST 14
> +#define ABRT_SLAVE_RD_INTX 15
>
> #define DW_IC_TX_ABRT_7B_ADDR_NOACK (1UL <<
> ABRT_7B_ADDR_NOACK)
> #define DW_IC_TX_ABRT_10ADDR1_NOACK (1UL <<
> ABRT_10ADDR1_NOACK)
> @@ -151,6 +173,9 @@
> #define DW_IC_TX_ABRT_10B_RD_NORSTRT (1UL <<
> ABRT_10B_RD_NORSTRT)
> #define DW_IC_TX_ABRT_MASTER_DIS (1UL << ABRT_MASTER_DIS)
> #define DW_IC_TX_ARB_LOST (1UL << ARB_LOST)
> +#define DW_IC_RX_ABRT_SLAVE_RD_INTX (1UL <<
> ABRT_SLAVE_RD_INTX)
> +#define DW_IC_RX_ABRT_SLAVE_ARBLOST (1UL <<
> ABRT_SLAVE_ARBLOST)
> +#define DW_IC_RX_ABRT_SLAVE_FLUSH_TXFIFO (1UL <<
> ABRT_SLAVE_FLUSH_TXFIFO)
>
> #define DW_IC_TX_ABRT_NOACK (DW_IC_TX_ABRT_7B_ADDR_NOA
> CK | \
> DW_IC_TX_ABRT_10ADDR1_NOACK
> | \
> @@ -206,6 +231,7 @@ struct dw_i2c_dev {
> void __iomem *base;
> struct completion cmd_complete;
> struct clk *clk;
> + struct i2c_client *slave;
> u32 (*get_clk_rate_khz) (struct
> dw_i2c_dev *dev);
> struct dw_pci_controller *controller;
> int cmd_err;
> @@ -225,6 +251,7 @@ struct dw_i2c_dev {
> struct i2c_adapter adapter;
> u32 functionality;
> u32 master_cfg;
> + u32 slave_cfg;
> unsigned int tx_fifo_depth;
> unsigned int rx_fifo_depth;
> int rx_outstanding;
--
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy
^ permalink raw reply
* Re: [PATCH v5 2/7] i2c: designware: refactoring of the i2c-designware
From: Andy Shevchenko @ 2016-12-28 15:12 UTC (permalink / raw)
To: Luis Oliveira, wsa, robh+dt, mark.rutland, jarkko.nikula,
mika.westerberg, linux-i2c, devicetree, linux-kernel
Cc: Ramiro.Oliveira, Joao.Pinto, CARLOS.PALMINHA
In-Reply-To: <0dbbce3bb2ffd47d5a78ea49f992b48d7ea2f35d.1482934380.git.lolivei@synopsys.com>
On Wed, 2016-12-28 at 14:43 +0000, Luis Oliveira wrote:
> - Factor out all _master() part of code from i2c-designware-core
> and i2c-designware-platdrv to separate functions.
> - Standardize all code related with MASTER mode.
> - I have to take off DW_IC_INTR_TX_EMPTY from DW_IC_INTR_DEFAULT_MASK
> because it is master specific.
>
> The purpose of this is to prepare the controller to have is I2C MASTER
> flow in a separate driver. To do this first all the
> functions/definitions related to the MASTER flow were identified.
Thanks for an update.
Some style related comments below (For the code related is up to you, my
tag still stands).
>
> Signed-off-by: Luis Oliveira <lolivei@synopsys.com>
> ---
> Changes V4->V5: (ACK by Andy)
When you get an Ack, or other tag (Reviewed-by, Tested-by, etc), and you
send new version, include this tag to your commit message (it applies to
all affected patches in your series).
It would be also good to have some high level changelog in the cover
letter, from this series I don't see, for example, which base you did
use (i2c-next? linux-next? v4.9? v4.10-rc1?).
> + dev_dbg(dev->dev,
> + "%s: enabled=%#x stat=%#x\n", __func__, enabled,
stat);
I hope you can fit format string on the first line. __func__ is
redundant when you are using debug printing (Dynamic Debug would include
it if asked for).
> +static void i2c_dw_configure_master(struct platform_device *pdev)
> +{
> + struct dw_i2c_dev *dev = platform_get_drvdata(pdev);
By the way, does it make sense to pass struct dw_i2c_dev * as a
parameter of the function?
> +
> + dev->master_cfg = DW_IC_CON_MASTER | DW_IC_CON_SLAVE_DISABLE
> |
> + DW_IC_CON_RESTART_EN;
> +
> + dev_dbg(&pdev->dev, "I am registed as a I2C Master!\n");
> +
> + switch (dev->clk_freq) {
> + case 100000:
> + dev->master_cfg |= DW_IC_CON_SPEED_STD;
> + break;
> + case 3400000:
> + dev->master_cfg |= DW_IC_CON_SPEED_HIGH;
> + break;
> + default:
> + dev->master_cfg |= DW_IC_CON_SPEED_FAST;
> + }
> +}
> +
>
--
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy
^ permalink raw reply
* [PATCH v5 6/7] i2c: designware: enable SLAVE in platform module
From: Luis Oliveira @ 2016-12-28 14:43 UTC (permalink / raw)
To: wsa, robh+dt, mark.rutland, jarkko.nikula, andriy.shevchenko,
mika.westerberg, linux-i2c, devicetree, linux-kernel
Cc: Luis.Oliveira, Ramiro.Oliveira, Joao.Pinto, CARLOS.PALMINHA
In-Reply-To: <cover.1482934380.git.lolivei@synopsys.com>
- Slave mode selected in platform module (devicetree support only)
- Check for ACPI - not supported in SLAVE mode:
- Changed the ifndef style to the use of ACPI_HANDLE that returns NULL
if the device was not enumerated from ACPI namespace.
Signed-off-by: Luis Oliveira <lolivei@synopsys.com>
---
Changes V4->V5: (Andy Shevchenko, Rob Herring, Mark Rutland)
- This is the patch that actually enable SLAVE mode in the platform
module by probing the DT nodes (Rob suggestion).
- Changed my device tree. Now I'm using: <reg | I2C_OWN_SLAVE_ADDRESS>
to identify a Slave.
- I have a new way of checking if ACPI is not enabled using the
ACPI_HANDLE and not ifdef.
drivers/i2c/busses/i2c-designware-platdrv.c | 70 +++++++++++++++++++++++++----
1 file changed, 62 insertions(+), 8 deletions(-)
diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
index 5cf4df63dbe8..ef75031f8a62 100644
--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -21,6 +21,7 @@
* ----------------------------------------------------------------------------
*
*/
+#include <dt-bindings/i2c/i2c.h>
#include <linux/acpi.h>
#include <linux/clk-provider.h>
#include <linux/clk.h>
@@ -143,11 +144,15 @@ static void i2c_dw_configure_master(struct platform_device *pdev)
{
struct dw_i2c_dev *dev = platform_get_drvdata(pdev);
+ dev->functionality = I2C_FUNC_10BIT_ADDR | DW_IC_DEFAULT_FUNCTIONALITY;
+
dev->master_cfg = DW_IC_CON_MASTER | DW_IC_CON_SLAVE_DISABLE |
DW_IC_CON_RESTART_EN;
dev_dbg(&pdev->dev, "I am registed as a I2C Master!\n");
+ dev->mode = DW_IC_MASTER;
+
switch (dev->clk_freq) {
case 100000:
dev->master_cfg |= DW_IC_CON_SPEED_STD;
@@ -160,6 +165,32 @@ static void i2c_dw_configure_master(struct platform_device *pdev)
}
}
+static void i2c_dw_configure_slave(struct platform_device *pdev)
+{
+ struct dw_i2c_dev *dev = platform_get_drvdata(pdev);
+
+ dev->functionality = I2C_FUNC_SLAVE | DW_IC_DEFAULT_FUNCTIONALITY;
+
+ dev->slave_cfg = DW_IC_CON_RX_FIFO_FULL_HLD_CTRL |
+ DW_IC_CON_RESTART_EN | DW_IC_CON_STOP_DET_IFADDRESSED |
+ DW_IC_CON_SPEED_FAST;
+
+ dev_dbg(&pdev->dev, "I am registed as a I2C Slave!\n");
+
+ dev->mode = DW_IC_SLAVE;
+
+ switch (dev->clk_freq) {
+ case 100000:
+ dev->slave_cfg |= DW_IC_CON_SPEED_STD;
+ break;
+ case 3400000:
+ dev->slave_cfg |= DW_IC_CON_SPEED_HIGH;
+ break;
+ default:
+ dev->slave_cfg |= DW_IC_CON_SPEED_FAST;
+ }
+}
+
static int i2c_dw_plat_prepare_clk(struct dw_i2c_dev *i_dev, bool prepare)
{
if (IS_ERR(i_dev->clk))
@@ -200,9 +231,11 @@ static int dw_i2c_plat_probe(struct platform_device *pdev)
struct dw_i2c_platform_data *pdata = dev_get_platdata(&pdev->dev);
struct i2c_adapter *adap;
struct dw_i2c_dev *dev;
+ struct fwnode_handle *child;
u32 acpi_speed, ht = 0;
struct resource *mem;
int irq, r;
+ u32 reg;
irq = platform_get_irq(pdev, 0);
if (irq < 0)
@@ -264,9 +297,16 @@ static int dw_i2c_plat_probe(struct platform_device *pdev)
if (r)
return r;
- dev->functionality = I2C_FUNC_10BIT_ADDR | DW_IC_DEFAULT_FUNCTIONALITY;
-
- i2c_dw_configure_master(pdev);
+ if (ACPI_HANDLE(&pdev->dev) == NULL) {
+ device_for_each_child_node(&pdev->dev, child) {
+ fwnode_property_read_u32(child, "reg", ®);
+ if (reg & I2C_OWN_SLAVE_ADDRESS)
+ i2c_dw_configure_slave(pdev);
+ else
+ i2c_dw_configure_master(pdev);
+ }
+ } else
+ i2c_dw_configure_master(pdev);
dev->clk = devm_clk_get(&pdev->dev, NULL);
if (!i2c_dw_plat_prepare_clk(dev, true)) {
@@ -295,7 +335,11 @@ static int dw_i2c_plat_probe(struct platform_device *pdev)
pm_runtime_enable(&pdev->dev);
}
- r = i2c_dw_probe(dev);
+ if (dev->mode == DW_IC_SLAVE)
+ r = i2c_dw_probe_slave(dev);
+ else
+ r = i2c_dw_probe(dev);
+
if (r && !dev->pm_runtime_disabled)
pm_runtime_disable(&pdev->dev);
@@ -310,7 +354,10 @@ static int dw_i2c_plat_remove(struct platform_device *pdev)
i2c_del_adapter(&dev->adapter);
- i2c_dw_disable(dev);
+ if (dev->mode == DW_IC_SLAVE)
+ i2c_dw_disable_slave(dev);
+ else
+ i2c_dw_disable(dev);
pm_runtime_dont_use_autosuspend(&pdev->dev);
pm_runtime_put_sync(&pdev->dev);
@@ -350,7 +397,10 @@ static int dw_i2c_plat_suspend(struct device *dev)
struct platform_device *pdev = to_platform_device(dev);
struct dw_i2c_dev *i_dev = platform_get_drvdata(pdev);
- i2c_dw_disable(i_dev);
+ if (i_dev->mode == DW_IC_SLAVE)
+ i2c_dw_disable_slave(i_dev);
+ else
+ i2c_dw_disable(i_dev);
i2c_dw_plat_prepare_clk(i_dev, false);
return 0;
@@ -363,8 +413,12 @@ static int dw_i2c_plat_resume(struct device *dev)
i2c_dw_plat_prepare_clk(i_dev, true);
- if (!i_dev->pm_runtime_disabled)
- i2c_dw_init(i_dev);
+ if (!i_dev->pm_runtime_disabled) {
+ if (i_dev->mode == DW_IC_SLAVE)
+ i2c_dw_init_slave(i_dev);
+ else
+ i2c_dw_init(i_dev);
+ }
return 0;
}
--
2.11.0
^ permalink raw reply related
* [PATCH v5 4/7] i2c: designware: introducing I2C_SLAVE definitions
From: Luis Oliveira @ 2016-12-28 14:43 UTC (permalink / raw)
To: wsa, robh+dt, mark.rutland, jarkko.nikula, andriy.shevchenko,
mika.westerberg, linux-i2c, devicetree, linux-kernel
Cc: Luis.Oliveira, Ramiro.Oliveira, Joao.Pinto, CARLOS.PALMINHA
In-Reply-To: <cover.1482934380.git.lolivei@synopsys.com>
- Definitions were added
SLAVE related definitions were added to the core of the controller.
Signed-off-by: Luis Oliveira <lolivei@synopsys.com>
---
Changes V4->V5: (Andy Shevchenko)
- This patch just introduces SLAVE definitions (as suggested in V4)
drivers/i2c/busses/i2c-designware-core.h | 27 +++++++++++++++++++++++++++
1 file changed, 27 insertions(+)
diff --git a/drivers/i2c/busses/i2c-designware-core.h b/drivers/i2c/busses/i2c-designware-core.h
index 8bba7a37c3ce..5080f1d2d2ec 100644
--- a/drivers/i2c/busses/i2c-designware-core.h
+++ b/drivers/i2c/busses/i2c-designware-core.h
@@ -36,15 +36,20 @@
#define DW_IC_CON_SPEED_FAST 0x4
#define DW_IC_CON_SPEED_HIGH 0x6
#define DW_IC_CON_SPEED_MASK 0x6
+#define DW_IC_CON_10BITADDR_SLAVE 0x8
#define DW_IC_CON_10BITADDR_MASTER 0x10
#define DW_IC_CON_RESTART_EN 0x20
#define DW_IC_CON_SLAVE_DISABLE 0x40
+#define DW_IC_CON_STOP_DET_IFADDRESSED 0x80
+#define DW_IC_CON_TX_EMPTY_CTRL 0x100
+#define DW_IC_CON_RX_FIFO_FULL_HLD_CTRL 0x200
/*
* Registers offset
*/
#define DW_IC_CON 0x0
#define DW_IC_TAR 0x4
+#define DW_IC_SAR 0x8
#define DW_IC_DATA_CMD 0x10
#define DW_IC_SS_SCL_HCNT 0x14
#define DW_IC_SS_SCL_LCNT 0x18
@@ -75,6 +80,7 @@
#define DW_IC_SDA_HOLD 0x7c
#define DW_IC_TX_ABRT_SOURCE 0x80
#define DW_IC_ENABLE_STATUS 0x9c
+#define DW_IC_CLR_RESTART_DET 0xa8
#define DW_IC_COMP_PARAM_1 0xf4
#define DW_IC_COMP_VERSION 0xf8
#define DW_IC_SDA_HOLD_MIN_VERS 0x3131312A
@@ -93,15 +99,22 @@
#define DW_IC_INTR_STOP_DET 0x200
#define DW_IC_INTR_START_DET 0x400
#define DW_IC_INTR_GEN_CALL 0x800
+#define DW_IC_INTR_RESTART_DET 0x1000
#define DW_IC_INTR_DEFAULT_MASK (DW_IC_INTR_RX_FULL | \
DW_IC_INTR_TX_ABRT | \
DW_IC_INTR_STOP_DET)
#define DW_IC_INTR_MASTER_MASK (DW_IC_INTR_DEFAULT_MASK | \
DW_IC_INTR_TX_EMPTY)
+#define DW_IC_INTR_SLAVE_MASK (DW_IC_INTR_DEFAULT_MASK | \
+ DW_IC_INTR_RX_DONE | \
+ DW_IC_INTR_RX_UNDER | \
+ DW_IC_INTR_RD_REQ)
+
#define DW_IC_STATUS_ACTIVITY 0x1
#define DW_IC_STATUS_TFE BIT(2)
#define DW_IC_STATUS_MASTER_ACTIVITY BIT(5)
+#define DW_IC_STATUS_SLAVE_ACTIVITY BIT(6)
#define DW_IC_SDA_HOLD_RX_SHIFT 16
#define DW_IC_SDA_HOLD_RX_MASK GENMASK(23, DW_IC_SDA_HOLD_RX_SHIFT)
@@ -123,6 +136,12 @@
#define TIMEOUT 20 /* ms */
/*
+ * operation modes
+ */
+#define DW_IC_MASTER 0
+#define DW_IC_SLAVE 1
+
+/*
* hardware abort codes from the DW_IC_TX_ABRT_SOURCE register
*
* only expected abort codes are listed here
@@ -139,6 +158,9 @@
#define ABRT_10B_RD_NORSTRT 10
#define ABRT_MASTER_DIS 11
#define ARB_LOST 12
+#define ABRT_SLAVE_FLUSH_TXFIFO 13
+#define ABRT_SLAVE_ARBLOST 14
+#define ABRT_SLAVE_RD_INTX 15
#define DW_IC_TX_ABRT_7B_ADDR_NOACK (1UL << ABRT_7B_ADDR_NOACK)
#define DW_IC_TX_ABRT_10ADDR1_NOACK (1UL << ABRT_10ADDR1_NOACK)
@@ -151,6 +173,9 @@
#define DW_IC_TX_ABRT_10B_RD_NORSTRT (1UL << ABRT_10B_RD_NORSTRT)
#define DW_IC_TX_ABRT_MASTER_DIS (1UL << ABRT_MASTER_DIS)
#define DW_IC_TX_ARB_LOST (1UL << ARB_LOST)
+#define DW_IC_RX_ABRT_SLAVE_RD_INTX (1UL << ABRT_SLAVE_RD_INTX)
+#define DW_IC_RX_ABRT_SLAVE_ARBLOST (1UL << ABRT_SLAVE_ARBLOST)
+#define DW_IC_RX_ABRT_SLAVE_FLUSH_TXFIFO (1UL << ABRT_SLAVE_FLUSH_TXFIFO)
#define DW_IC_TX_ABRT_NOACK (DW_IC_TX_ABRT_7B_ADDR_NOACK | \
DW_IC_TX_ABRT_10ADDR1_NOACK | \
@@ -206,6 +231,7 @@ struct dw_i2c_dev {
void __iomem *base;
struct completion cmd_complete;
struct clk *clk;
+ struct i2c_client *slave;
u32 (*get_clk_rate_khz) (struct dw_i2c_dev *dev);
struct dw_pci_controller *controller;
int cmd_err;
@@ -225,6 +251,7 @@ struct dw_i2c_dev {
struct i2c_adapter adapter;
u32 functionality;
u32 master_cfg;
+ u32 slave_cfg;
unsigned int tx_fifo_depth;
unsigned int rx_fifo_depth;
int rx_outstanding;
--
2.11.0
^ permalink raw reply related
* [PATCH v5 1/7] i2c: designware: Cleaning and comment style fixes.
From: Luis Oliveira @ 2016-12-28 14:43 UTC (permalink / raw)
To: wsa, robh+dt, mark.rutland, jarkko.nikula, andriy.shevchenko,
mika.westerberg, linux-i2c, devicetree, linux-kernel
Cc: Luis.Oliveira, Ramiro.Oliveira, Joao.Pinto, CARLOS.PALMINHA
In-Reply-To: <cover.1482934380.git.lolivei@synopsys.com>
The purpose of this commit is to fix some comments and styling issues
in the existing code due to the need of reuse this code. What is being
made here is:
- Sorted the headers files
- Corrected some comments style
- Reverse tree in the variables declaration
- Add/remove empty lines and tabs where needed
- Fix of a misspelled word
The value of this, besides the rules of coding style, is because I
will use this code after and it will make my future patch a lot bigger
and complicated to review. The work here won't bring any additional work
to backported fixes because is just style and reordering.
Signed-off-by: Luis Oliveira <lolivei@synopsys.com>
---
Changes V5 (Jarkko):
- This patch was submitted by it self and it god ACK. I need it in the
patchset because the next patch relly on this one.
- Also fixed the commit length.
drivers/i2c/busses/i2c-designware-core.c | 36 ++++++++++++++---------------
drivers/i2c/busses/i2c-designware-platdrv.c | 27 +++++++++++-----------
2 files changed, 32 insertions(+), 31 deletions(-)
diff --git a/drivers/i2c/busses/i2c-designware-core.c b/drivers/i2c/busses/i2c-designware-core.c
index 6d81c56184d3..9d724a6a7ec8 100644
--- a/drivers/i2c/busses/i2c-designware-core.c
+++ b/drivers/i2c/busses/i2c-designware-core.c
@@ -21,17 +21,17 @@
* ----------------------------------------------------------------------------
*
*/
+#include <linux/delay.h>
#include <linux/export.h>
#include <linux/errno.h>
#include <linux/err.h>
#include <linux/i2c.h>
#include <linux/interrupt.h>
#include <linux/io.h>
-#include <linux/pm_runtime.h>
-#include <linux/delay.h>
#include <linux/module.h>
-#include "i2c-designware-core.h"
+#include <linux/pm_runtime.h>
+#include "i2c-designware-core.h"
/*
* Registers offset
*/
@@ -98,7 +98,7 @@
#define DW_IC_ERR_TX_ABRT 0x1
-#define DW_IC_TAR_10BITADDR_MASTER BIT(12)
+#define DW_IC_TAR_10BITADDR_MASTER BIT(12)
#define DW_IC_COMP_PARAM_1_SPEED_MODE_HIGH (BIT(2) | BIT(3))
#define DW_IC_COMP_PARAM_1_SPEED_MODE_MASK GENMASK(3, 2)
@@ -113,10 +113,10 @@
#define TIMEOUT 20 /* ms */
/*
- * hardware abort codes from the DW_IC_TX_ABRT_SOURCE register
+ * Hardware abort codes from the DW_IC_TX_ABRT_SOURCE register
*
- * only expected abort codes are listed here
- * refer to the datasheet for the full list
+ * Only expected abort codes are listed here,
+ * refer to the datasheet for the full list.
*/
#define ABRT_7B_ADDR_NOACK 0
#define ABRT_10ADDR1_NOACK 1
@@ -338,14 +338,14 @@ int i2c_dw_init(struct dw_i2c_dev *dev)
reg = dw_readl(dev, DW_IC_COMP_TYPE);
if (reg == ___constant_swab32(DW_IC_COMP_TYPE_VALUE)) {
- /* Configure register endianess access */
+ /* Configure register endianness access */
dev->accessor_flags |= ACCESS_SWAP;
} else if (reg == (DW_IC_COMP_TYPE_VALUE & 0x0000ffff)) {
/* Configure register access mode 16bit */
dev->accessor_flags |= ACCESS_16BIT;
} else if (reg != DW_IC_COMP_TYPE_VALUE) {
- dev_err(dev->dev, "Unknown Synopsys component type: "
- "0x%08x\n", reg);
+ dev_err(dev->dev,
+ "Unknown Synopsys component type: 0x%08x\n", reg);
i2c_dw_release_lock(dev);
return -ENODEV;
}
@@ -355,7 +355,7 @@ int i2c_dw_init(struct dw_i2c_dev *dev)
/* Disable the adapter */
__i2c_dw_enable_and_wait(dev, false);
- /* set standard and fast speed deviders for high/low periods */
+ /* Set standard and fast speed deviders for high/low periods */
sda_falling_time = dev->sda_falling_time ?: 300; /* ns */
scl_falling_time = dev->scl_falling_time ?: 300; /* ns */
@@ -444,7 +444,7 @@ int i2c_dw_init(struct dw_i2c_dev *dev)
dw_writel(dev, dev->tx_fifo_depth / 2, DW_IC_TX_TL);
dw_writel(dev, 0, DW_IC_RX_TL);
- /* configure the i2c master */
+ /* Configure the I2C master */
dw_writel(dev, dev->master_cfg , DW_IC_CON);
i2c_dw_release_lock(dev);
@@ -480,7 +480,7 @@ static void i2c_dw_xfer_init(struct dw_i2c_dev *dev)
/* Disable the adapter */
__i2c_dw_enable_and_wait(dev, false);
- /* if the slave address is ten bit address, enable 10BITADDR */
+ /* If the slave address is ten bit address, enable 10BITADDR */
if (dev->dynamic_tar_update_enabled) {
/*
* If I2C_DYNAMIC_TAR_UPDATE is set, the 10-bit addressing
@@ -505,7 +505,7 @@ static void i2c_dw_xfer_init(struct dw_i2c_dev *dev)
*/
dw_writel(dev, msgs[dev->msg_write_idx].addr | ic_tar, DW_IC_TAR);
- /* enforce disabled interrupts (due to HW issues) */
+ /* Enforce disabled interrupts (due to HW issues) */
i2c_dw_disable_int(dev);
/* Enable the adapter */
@@ -539,7 +539,7 @@ i2c_dw_xfer_msg(struct dw_i2c_dev *dev)
u32 flags = msgs[dev->msg_write_idx].flags;
/*
- * if target address has changed, we need to
+ * If target address has changed, we need to
* reprogram the target address in the i2c
* adapter when we are done with this transfer
*/
@@ -601,7 +601,7 @@ i2c_dw_xfer_msg(struct dw_i2c_dev *dev)
if (msgs[dev->msg_write_idx].flags & I2C_M_RD) {
- /* avoid rx buffer overrun */
+ /* Avoid rx buffer overrun */
if (dev->rx_outstanding >= dev->rx_fifo_depth)
break;
@@ -905,7 +905,7 @@ static irqreturn_t i2c_dw_isr(int this_irq, void *dev_id)
/*
* Anytime TX_ABRT is set, the contents of the tx/rx
- * buffers are flushed. Make sure to skip them.
+ * buffers are flushed. Make sure to skip them.
*/
dw_writel(dev, 0, DW_IC_INTR_MASK);
goto tx_aborted;
@@ -927,7 +927,7 @@ static irqreturn_t i2c_dw_isr(int this_irq, void *dev_id)
if ((stat & (DW_IC_INTR_TX_ABRT | DW_IC_INTR_STOP_DET)) || dev->msg_err)
complete(&dev->cmd_complete);
else if (unlikely(dev->accessor_flags & ACCESS_INTR_MASK)) {
- /* workaround to trigger pending interrupt */
+ /* Workaround to trigger pending interrupt */
stat = dw_readl(dev, DW_IC_INTR_MASK);
i2c_dw_disable_int(dev);
dw_writel(dev, stat, DW_IC_INTR_MASK);
diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
index 6ce431323125..4070baea4fb9 100644
--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -21,26 +21,27 @@
* ----------------------------------------------------------------------------
*
*/
-#include <linux/kernel.h>
-#include <linux/module.h>
+#include <linux/acpi.h>
+#include <linux/clk-provider.h>
+#include <linux/clk.h>
#include <linux/delay.h>
#include <linux/dmi.h>
-#include <linux/i2c.h>
-#include <linux/clk.h>
-#include <linux/clk-provider.h>
-#include <linux/errno.h>
-#include <linux/sched.h>
#include <linux/err.h>
+#include <linux/errno.h>
+#include <linux/i2c.h>
#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
#include <linux/of.h>
+#include <linux/platform_data/i2c-designware.h>
#include <linux/platform_device.h>
#include <linux/pm.h>
#include <linux/pm_runtime.h>
#include <linux/property.h>
-#include <linux/io.h>
+#include <linux/sched.h>
#include <linux/slab.h>
-#include <linux/acpi.h>
-#include <linux/platform_data/i2c-designware.h>
+
#include "i2c-designware-core.h"
static u32 i2c_dw_get_clk_rate_khz(struct dw_i2c_dev *dev)
@@ -176,11 +177,11 @@ static void dw_i2c_set_fifo_size(struct dw_i2c_dev *dev, int id)
static int dw_i2c_plat_probe(struct platform_device *pdev)
{
struct dw_i2c_platform_data *pdata = dev_get_platdata(&pdev->dev);
- struct dw_i2c_dev *dev;
struct i2c_adapter *adap;
+ struct dw_i2c_dev *dev;
+ u32 acpi_speed, ht = 0;
struct resource *mem;
int irq, r;
- u32 acpi_speed, ht = 0;
irq = platform_get_irq(pdev, 0);
if (irq < 0)
@@ -371,7 +372,7 @@ static const struct dev_pm_ops dw_i2c_dev_pm_ops = {
#define DW_I2C_DEV_PMOPS NULL
#endif
-/* work with hotplug and coldplug */
+/* Work with hotplug and coldplug */
MODULE_ALIAS("platform:i2c_designware");
static struct platform_driver dw_i2c_driver = {
--
2.11.0
^ permalink raw reply related
* [PATCH v5 7/7] i2c: designware: style changes in existing code
From: Luis Oliveira @ 2016-12-28 14:43 UTC (permalink / raw)
To: wsa, robh+dt, mark.rutland, jarkko.nikula, andriy.shevchenko,
mika.westerberg, linux-i2c, devicetree, linux-kernel
Cc: Luis.Oliveira, Ramiro.Oliveira, Joao.Pinto, CARLOS.PALMINHA
In-Reply-To: <cover.1482934380.git.lolivei@synopsys.com>
Replaced all the return variables 'r' in the existing
code by 'ret' to make the code easier to read (and
more standard).
Signed-off-by: Luis Oliveira <lolivei@synopsys.com>
---
Changes V4->V5: (Andy Shevchenko)
- Replaced all the old code using "r" as return to "ret". For consistency
purposes.
drivers/i2c/busses/i2c-designware-master.c | 30 ++++++++++++++---------------
drivers/i2c/busses/i2c-designware-platdrv.c | 16 +++++++--------
2 files changed, 23 insertions(+), 23 deletions(-)
diff --git a/drivers/i2c/busses/i2c-designware-master.c b/drivers/i2c/busses/i2c-designware-master.c
index b55a7f4c5149..0d5aca6edb48 100644
--- a/drivers/i2c/busses/i2c-designware-master.c
+++ b/drivers/i2c/busses/i2c-designware-master.c
@@ -644,18 +644,18 @@ EXPORT_SYMBOL_GPL(i2c_dw_read_comp_param);
int i2c_dw_probe(struct dw_i2c_dev *dev)
{
struct i2c_adapter *adap = &dev->adapter;
- int r;
+ int ret;
u32 reg;
init_completion(&dev->cmd_complete);
- r = i2c_dw_init(dev);
- if (r)
- return r;
+ ret = i2c_dw_init(dev);
+ if (ret)
+ return ret;
- r = i2c_dw_acquire_lock(dev);
- if (r)
- return r;
+ ret = i2c_dw_acquire_lock(dev);
+ if (ret)
+ return ret;
/*
* Test if dynamic TAR update is enabled in this controller by writing
@@ -681,13 +681,13 @@ int i2c_dw_probe(struct dw_i2c_dev *dev)
i2c_set_adapdata(adap, dev);
i2c_dw_disable_int(dev);
- r = devm_request_irq(dev->dev, dev->irq, i2c_dw_isr,
+ ret = devm_request_irq(dev->dev, dev->irq, i2c_dw_isr,
IRQF_SHARED | IRQF_COND_SUSPEND,
dev_name(dev->dev), dev);
- if (r) {
+ if (ret) {
dev_err(dev->dev, "failure requesting irq %i: %d\n",
- dev->irq, r);
- return r;
+ dev->irq, ret);
+ return ret;
}
/*
@@ -697,12 +697,12 @@ int i2c_dw_probe(struct dw_i2c_dev *dev)
* registered I2C slaves that do I2C transfers in their probe.
*/
pm_runtime_get_noresume(dev->dev);
- r = i2c_add_numbered_adapter(adap);
- if (r)
- dev_err(dev->dev, "failure adding adapter: %d\n", r);
+ ret = i2c_add_numbered_adapter(adap);
+ if (ret)
+ dev_err(dev->dev, "failure adding adapter: %d\n", ret);
pm_runtime_put_noidle(dev->dev);
- return r;
+ return ret;
}
EXPORT_SYMBOL_GPL(i2c_dw_probe);
diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
index ef75031f8a62..785f4380c9a9 100644
--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -234,7 +234,7 @@ static int dw_i2c_plat_probe(struct platform_device *pdev)
struct fwnode_handle *child;
u32 acpi_speed, ht = 0;
struct resource *mem;
- int irq, r;
+ int irq, ret;
u32 reg;
irq = platform_get_irq(pdev, 0);
@@ -293,9 +293,9 @@ static int dw_i2c_plat_probe(struct platform_device *pdev)
return -EINVAL;
}
- r = i2c_dw_eval_lock_support(dev);
- if (r)
- return r;
+ ret = i2c_dw_eval_lock_support(dev);
+ if (ret)
+ return ret;
if (ACPI_HANDLE(&pdev->dev) == NULL) {
device_for_each_child_node(&pdev->dev, child) {
@@ -336,14 +336,14 @@ static int dw_i2c_plat_probe(struct platform_device *pdev)
}
if (dev->mode == DW_IC_SLAVE)
- r = i2c_dw_probe_slave(dev);
+ ret = i2c_dw_probe_slave(dev);
else
- r = i2c_dw_probe(dev);
+ ret = i2c_dw_probe(dev);
- if (r && !dev->pm_runtime_disabled)
+ if (ret && !dev->pm_runtime_disabled)
pm_runtime_disable(&pdev->dev);
- return r;
+ return ret;
}
static int dw_i2c_plat_remove(struct platform_device *pdev)
--
2.11.0
^ permalink raw reply related
* [PATCH v5 5/7] i2c: designware: add SLAVE mode functions
From: Luis Oliveira @ 2016-12-28 14:43 UTC (permalink / raw)
To: wsa, robh+dt, mark.rutland, jarkko.nikula, andriy.shevchenko,
mika.westerberg, linux-i2c, devicetree, linux-kernel
Cc: Luis.Oliveira, Ramiro.Oliveira, Joao.Pinto, CARLOS.PALMINHA
In-Reply-To: <cover.1482934380.git.lolivei@synopsys.com>
- Changes in Kconfig to enable I2C_SLAVE support
- Slave functions added to core library file
- Slave abort sources added to common source file
- New driver: i2c-designware-slave added
- Changes in the Makefile to compile it all
All the SLAVE flow is added but it is not enabled via platform
driver.
Signed-off-by: Luis Oliveira <lolivei@synopsys.com>
---
Changes V4->V5: (Andy Shevchenko)
- This patch adds the SLAVE support functions but it still not enable it
via platform module. I think it is more readable now
drivers/i2c/busses/Kconfig | 3 +-
drivers/i2c/busses/Makefile | 2 +-
drivers/i2c/busses/i2c-designware-common.c | 10 +-
drivers/i2c/busses/i2c-designware-core.h | 6 +
drivers/i2c/busses/i2c-designware-slave.c | 434 +++++++++++++++++++++++++++++
5 files changed, 452 insertions(+), 3 deletions(-)
create mode 100644 drivers/i2c/busses/i2c-designware-slave.c
diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index 0cdc8443deab..b58fdcde93a7 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -469,11 +469,12 @@ config I2C_DESIGNWARE_CORE
config I2C_DESIGNWARE_PLATFORM
tristate "Synopsys DesignWare Platform"
+ select I2C_SLAVE
select I2C_DESIGNWARE_CORE
depends on (ACPI && COMMON_CLK) || !ACPI
help
If you say yes to this option, support will be included for the
- Synopsys DesignWare I2C adapter. Only master mode is supported.
+ Synopsys DesignWare I2C adapter.
This driver can also be built as a module. If so, the module
will be called i2c-designware-platform.
diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
index 4f8f6a2b9346..c2ed84a86f49 100644
--- a/drivers/i2c/busses/Makefile
+++ b/drivers/i2c/busses/Makefile
@@ -40,7 +40,7 @@ obj-$(CONFIG_I2C_CBUS_GPIO) += i2c-cbus-gpio.o
obj-$(CONFIG_I2C_CPM) += i2c-cpm.o
obj-$(CONFIG_I2C_DAVINCI) += i2c-davinci.o
obj-$(CONFIG_I2C_DESIGNWARE_CORE) += i2c-designware-core.o
-i2c-designware-core-objs := i2c-designware-common.o i2c-designware-master.o
+i2c-designware-core-objs := i2c-designware-common.o i2c-designware-slave.o i2c-designware-master.o
obj-$(CONFIG_I2C_DESIGNWARE_PLATFORM) += i2c-designware-platform.o
i2c-designware-platform-objs := i2c-designware-platdrv.o
i2c-designware-platform-$(CONFIG_I2C_DESIGNWARE_BAYTRAIL) += i2c-designware-baytrail.o
diff --git a/drivers/i2c/busses/i2c-designware-common.c b/drivers/i2c/busses/i2c-designware-common.c
index 012b8f9dec15..b3beba639e98 100644
--- a/drivers/i2c/busses/i2c-designware-common.c
+++ b/drivers/i2c/busses/i2c-designware-common.c
@@ -30,6 +30,7 @@
#include <linux/pm_runtime.h>
#include <linux/delay.h>
#include <linux/module.h>
+
#include "i2c-designware-core.h"
static char *abort_sources[] = {
@@ -42,7 +43,7 @@ static char *abort_sources[] = {
[ABRT_TXDATA_NOACK] =
"data not acknowledged",
[ABRT_GCALL_NOACK] =
- "no acknowledgement for a general call",
+ "no acknowledgment for a general call",
[ABRT_GCALL_READ] =
"read after general call",
[ABRT_SBYTE_ACKDET] =
@@ -55,6 +56,13 @@ static char *abort_sources[] = {
"trying to use disabled adapter",
[ARB_LOST] =
"lost arbitration",
+ [ABRT_SLAVE_FLUSH_TXFIFO] =
+ "read command so flush old data in the TX FIFO",
+ [ABRT_SLAVE_ARBLOST] =
+ "slave lost the bus while transmitting data to a remote master",
+ [ABRT_SLAVE_RD_INTX] =
+ "slave request for data to be transmitted and there is a 1 in "
+ "bit 8 of IC_DATA_CMD",
};
u32 dw_readl(struct dw_i2c_dev *dev, int offset)
diff --git a/drivers/i2c/busses/i2c-designware-core.h b/drivers/i2c/busses/i2c-designware-core.h
index 5080f1d2d2ec..1729b6bb5239 100644
--- a/drivers/i2c/busses/i2c-designware-core.h
+++ b/drivers/i2c/busses/i2c-designware-core.h
@@ -271,6 +271,7 @@ struct dw_i2c_dev {
void (*release_lock)(struct dw_i2c_dev *dev);
bool pm_runtime_disabled;
bool dynamic_tar_update_enabled;
+ bool mode;
};
#define ACCESS_SWAP 0x00000001
@@ -295,6 +296,11 @@ extern void i2c_dw_disable(struct dw_i2c_dev *dev);
extern void i2c_dw_disable_int(struct dw_i2c_dev *dev);
extern u32 i2c_dw_read_comp_param(struct dw_i2c_dev *dev);
extern int i2c_dw_probe(struct dw_i2c_dev *dev);
+extern int i2c_dw_init_slave(struct dw_i2c_dev *dev);
+extern void i2c_dw_disable_slave(struct dw_i2c_dev *dev);
+extern void i2c_dw_disable_int_slave(struct dw_i2c_dev *dev);
+extern u32 i2c_dw_read_comp_param_slave(struct dw_i2c_dev *dev);
+extern int i2c_dw_probe_slave(struct dw_i2c_dev *dev);
#if IS_ENABLED(CONFIG_I2C_DESIGNWARE_BAYTRAIL)
extern int i2c_dw_eval_lock_support(struct dw_i2c_dev *dev);
diff --git a/drivers/i2c/busses/i2c-designware-slave.c b/drivers/i2c/busses/i2c-designware-slave.c
new file mode 100644
index 000000000000..5afc88d9e6b7
--- /dev/null
+++ b/drivers/i2c/busses/i2c-designware-slave.c
@@ -0,0 +1,434 @@
+/*
+ * Synopsys DesignWare I2C adapter driver (slave only).
+ *
+ * Based on the Synopsys DesignWare I2C adapter driver (master).
+ *
+ * Copyright (C) 2016 Synopsys Inc.
+ *
+ * ----------------------------------------------------------------------------
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ * ----------------------------------------------------------------------------
+ *
+ */
+#include <linux/errno.h>
+#include <linux/err.h>
+#include <linux/i2c.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/pm_runtime.h>
+#include <linux/delay.h>
+#include <linux/module.h>
+
+#include "i2c-designware-core.h"
+
+static void i2c_dw_configure_fifo_slave(struct dw_i2c_dev *dev)
+{
+ /* Configure Tx/Rx FIFO threshold levels. */
+ dw_writel(dev, 0, DW_IC_TX_TL);
+ dw_writel(dev, 0, DW_IC_RX_TL);
+
+ /* Configure the I2C slave. */
+ dw_writel(dev, dev->slave_cfg, DW_IC_CON);
+ dw_writel(dev, DW_IC_INTR_SLAVE_MASK, DW_IC_INTR_MASK);
+}
+
+/**
+ * i2c_dw_init_slave() - Initialize the designware i2c slave hardware
+ * @dev: device private data
+ *
+ * This functions configures and enables the I2C.
+ * This function is called during I2C init function, and in case of timeout at
+ * run time.
+ */
+int i2c_dw_init_slave(struct dw_i2c_dev *dev)
+{
+ u32 sda_falling_time, scl_falling_time;
+ u32 reg, comp_param1;
+ u32 hcnt, lcnt;
+ int ret;
+
+ ret = i2c_dw_acquire_lock(dev);
+ if (ret)
+ return ret;
+
+ reg = dw_readl(dev, DW_IC_COMP_TYPE);
+ if (reg == ___constant_swab32(DW_IC_COMP_TYPE_VALUE)) {
+ /* Configure register endianness access. */
+ dev->accessor_flags |= ACCESS_SWAP;
+ } else if (reg == (DW_IC_COMP_TYPE_VALUE & 0x0000ffff)) {
+ /* Configure register access mode 16bit. */
+ dev->accessor_flags |= ACCESS_16BIT;
+ } else if (reg != DW_IC_COMP_TYPE_VALUE) {
+ dev_err(dev->dev,
+ "Unknown Synopsys component type: 0x%08x\n", reg);
+ i2c_dw_release_lock(dev);
+ return -ENODEV;
+ }
+
+ comp_param1 = dw_readl(dev, DW_IC_COMP_PARAM_1);
+
+ /* Disable the adapter. */
+ __i2c_dw_enable_and_wait(dev, false);
+
+ /* Set standard and fast speed deviders for high/low periods. */
+ sda_falling_time = dev->sda_falling_time ?: 300; /* ns */
+ scl_falling_time = dev->scl_falling_time ?: 300; /* ns */
+
+ /* Set SCL timing parameters for standard-mode. */
+ if (dev->ss_hcnt && dev->ss_lcnt) {
+ hcnt = dev->ss_hcnt;
+ lcnt = dev->ss_lcnt;
+ } else {
+ hcnt = i2c_dw_scl_hcnt(i2c_dw_clk_rate(dev),
+ 4000, /* tHD;STA = tHIGH = 4.0 us */
+ sda_falling_time,
+ 0, /* 0: DW default, 1: Ideal */
+ 0); /* No offset */
+ lcnt = i2c_dw_scl_lcnt(i2c_dw_clk_rate(dev),
+ 4700, /* tLOW = 4.7 us */
+ scl_falling_time,
+ 0); /* No offset */
+ }
+ dw_writel(dev, hcnt, DW_IC_SS_SCL_HCNT);
+ dw_writel(dev, lcnt, DW_IC_SS_SCL_LCNT);
+ dev_dbg(dev->dev, "Standard-mode HCNT:LCNT = %d:%d\n", hcnt, lcnt);
+
+ /* Set SCL timing parameters for fast-mode or fast-mode plus. */
+ if ((dev->clk_freq == 1000000) && dev->fp_hcnt && dev->fp_lcnt) {
+ hcnt = dev->fp_hcnt;
+ lcnt = dev->fp_lcnt;
+ } else if (dev->fs_hcnt && dev->fs_lcnt) {
+ hcnt = dev->fs_hcnt;
+ lcnt = dev->fs_lcnt;
+ } else {
+ hcnt = i2c_dw_scl_hcnt(i2c_dw_clk_rate(dev),
+ 600, /* tHD;STA = tHIGH = 0.6 us */
+ sda_falling_time,
+ 0, /* 0: DW default, 1: Ideal */
+ 0); /* No offset */
+ lcnt = i2c_dw_scl_lcnt(i2c_dw_clk_rate(dev),
+ 1300, /* tLOW = 1.3 us */
+ scl_falling_time,
+ 0); /* No offset */
+ }
+ dw_writel(dev, hcnt, DW_IC_FS_SCL_HCNT);
+ dw_writel(dev, lcnt, DW_IC_FS_SCL_LCNT);
+ dev_dbg(dev->dev, "Fast-mode HCNT:LCNT = %d:%d\n", hcnt, lcnt);
+
+ if ((dev->slave_cfg & DW_IC_CON_SPEED_MASK) ==
+ DW_IC_CON_SPEED_HIGH) {
+ if ((comp_param1 & DW_IC_COMP_PARAM_1_SPEED_MODE_MASK)
+ != DW_IC_COMP_PARAM_1_SPEED_MODE_HIGH) {
+ dev_err(dev->dev, "High Speed not supported!\n");
+ dev->slave_cfg &= ~DW_IC_CON_SPEED_MASK;
+ dev->slave_cfg |= DW_IC_CON_SPEED_FAST;
+ } else if (dev->hs_hcnt && dev->hs_lcnt) {
+ hcnt = dev->hs_hcnt;
+ lcnt = dev->hs_lcnt;
+ dw_writel(dev, hcnt, DW_IC_HS_SCL_HCNT);
+ dw_writel(dev, lcnt, DW_IC_HS_SCL_LCNT);
+ dev_dbg(dev->dev, "HighSpeed-mode HCNT:LCNT = %d:%d\n",
+ hcnt, lcnt);
+ }
+ }
+
+ /* Configure SDA Hold Time if required. */
+ reg = dw_readl(dev, DW_IC_COMP_VERSION);
+ if (reg >= DW_IC_SDA_HOLD_MIN_VERS) {
+ if (!dev->sda_hold_time) {
+ /* Keep previous hold time setting if no one set it. */
+ dev->sda_hold_time = dw_readl(dev, DW_IC_SDA_HOLD);
+ }
+ /*
+ * Workaround for avoiding TX arbitration lost in case I2C
+ * slave pulls SDA down "too quickly" after falling egde of
+ * SCL by enabling non-zero SDA RX hold. Specification says it
+ * extends incoming SDA low to high transition while SCL is
+ * high but it apprears to help also above issue.
+ */
+ if (!(dev->sda_hold_time & DW_IC_SDA_HOLD_RX_MASK))
+ dev->sda_hold_time |= 1 << DW_IC_SDA_HOLD_RX_SHIFT;
+ dw_writel(dev, dev->sda_hold_time, DW_IC_SDA_HOLD);
+ } else {
+ dev_warn(dev->dev,
+ "Hardware too old to adjust SDA hold time.\n");
+ }
+
+ i2c_dw_configure_fifo_slave(dev);
+ i2c_dw_release_lock(dev);
+
+ return 0;
+}
+EXPORT_SYMBOL_GPL(i2c_dw_init_slave);
+
+int i2c_dw_reg_slave(struct i2c_client *slave)
+{
+ struct dw_i2c_dev *dev = i2c_get_adapdata(slave->adapter);
+
+ if (dev->slave)
+ return -EBUSY;
+ if (slave->flags & I2C_CLIENT_TEN)
+ return -EAFNOSUPPORT;
+ /*
+ * Set slave address in the IC_SAR register,
+ * the address to which the DW_apb_i2c responds.
+ */
+
+ __i2c_dw_enable(dev, false);
+ dw_writel(dev, slave->addr, DW_IC_SAR);
+ dev->slave = slave;
+
+ __i2c_dw_enable(dev, true);
+
+ dev->cmd_err = 0;
+ dev->msg_write_idx = 0;
+ dev->msg_read_idx = 0;
+ dev->msg_err = 0;
+ dev->status = STATUS_IDLE;
+ dev->abort_source = 0;
+ dev->rx_outstanding = 0;
+
+ return 0;
+}
+
+static int i2c_dw_unreg_slave(struct i2c_client *slave)
+{
+ struct dw_i2c_dev *dev = i2c_get_adapdata(slave->adapter);
+
+ i2c_dw_disable_int_slave(dev);
+ i2c_dw_disable_slave(dev);
+ dev->slave = NULL;
+
+ return 0;
+}
+
+static u32 i2c_dw_read_clear_intrbits_slave(struct dw_i2c_dev *dev)
+{
+ u32 stat;
+
+ /*
+ * The IC_INTR_STAT register just indicates "enabled" interrupts.
+ * Ths unmasked raw version of interrupt status bits are available
+ * in the IC_RAW_INTR_STAT register.
+ *
+ * That is,
+ * stat = dw_readl(IC_INTR_STAT);
+ * equals to,
+ * stat = dw_readl(IC_RAW_INTR_STAT) & dw_readl(IC_INTR_MASK);
+ *
+ * The raw version might be useful for debugging purposes.
+ */
+ stat = dw_readl(dev, DW_IC_INTR_STAT);
+
+ /*
+ * Do not use the IC_CLR_INTR register to clear interrupts, or
+ * you'll miss some interrupts, triggered during the period from
+ * dw_readl(IC_INTR_STAT) to dw_readl(IC_CLR_INTR).
+ *
+ * Instead, use the separately-prepared IC_CLR_* registers.
+ */
+ if (stat & DW_IC_INTR_TX_ABRT)
+ dw_readl(dev, DW_IC_CLR_TX_ABRT);
+ if (stat & DW_IC_INTR_RX_UNDER)
+ dw_readl(dev, DW_IC_CLR_RX_UNDER);
+ if (stat & DW_IC_INTR_RX_OVER)
+ dw_readl(dev, DW_IC_CLR_RX_OVER);
+ if (stat & DW_IC_INTR_TX_OVER)
+ dw_readl(dev, DW_IC_CLR_TX_OVER);
+ if (stat & DW_IC_INTR_RX_DONE)
+ dw_readl(dev, DW_IC_CLR_RX_DONE);
+ if (stat & DW_IC_INTR_ACTIVITY)
+ dw_readl(dev, DW_IC_CLR_ACTIVITY);
+ if (stat & DW_IC_INTR_STOP_DET)
+ dw_readl(dev, DW_IC_CLR_STOP_DET);
+ if (stat & DW_IC_INTR_START_DET)
+ dw_readl(dev, DW_IC_CLR_START_DET);
+ if (stat & DW_IC_INTR_GEN_CALL)
+ dw_readl(dev, DW_IC_CLR_GEN_CALL);
+
+ return stat;
+}
+
+/*
+ * Interrupt service routine. This gets called whenever an I2C slave interrupt
+ * occurs.
+ */
+
+static int i2c_dw_irq_handler_slave(struct dw_i2c_dev *dev)
+{
+ u32 raw_stat, stat, enabled;
+ u8 val, slave_activity;
+
+ stat = dw_readl(dev, DW_IC_INTR_STAT);
+ enabled = dw_readl(dev, DW_IC_ENABLE);
+ raw_stat = dw_readl(dev, DW_IC_RAW_INTR_STAT);
+ slave_activity = ((dw_readl(dev, DW_IC_STATUS) &
+ DW_IC_STATUS_SLAVE_ACTIVITY) >> 6);
+
+ if (!enabled || !(raw_stat & ~DW_IC_INTR_ACTIVITY))
+ return 0;
+
+ dev_dbg(dev->dev,
+ "%s: %#x SLAVE_ACTV=%#x : RAW_INTR_STAT=%#x : INTR_STAT=%#x\n",
+ __func__, enabled, slave_activity, raw_stat, stat);
+
+ if (stat & DW_IC_INTR_RESTART_DET)
+ dw_readl(dev, DW_IC_CLR_RESTART_DET);
+ if (stat & DW_IC_INTR_START_DET)
+ dw_readl(dev, DW_IC_CLR_START_DET);
+ if (stat & DW_IC_INTR_ACTIVITY)
+ dw_readl(dev, DW_IC_CLR_ACTIVITY);
+ if (stat & DW_IC_INTR_RX_OVER)
+ dw_readl(dev, DW_IC_CLR_RX_OVER);
+ if ((stat & DW_IC_INTR_RX_FULL) && (stat & DW_IC_INTR_STOP_DET))
+ i2c_slave_event(dev->slave, I2C_SLAVE_WRITE_REQUESTED, &val);
+
+ if (slave_activity) {
+ if (stat & DW_IC_INTR_RD_REQ) {
+ if (stat & DW_IC_INTR_RX_FULL) {
+ val = dw_readl(dev, DW_IC_DATA_CMD);
+ if (!i2c_slave_event(dev->slave,
+ I2C_SLAVE_WRITE_RECEIVED, &val)) {
+ dev_dbg(dev->dev, "Byte %X acked!",
+ val);
+ }
+ dw_readl(dev, DW_IC_CLR_RD_REQ);
+ stat = i2c_dw_read_clear_intrbits_slave(dev);
+ } else {
+ dw_readl(dev, DW_IC_CLR_RD_REQ);
+ dw_readl(dev, DW_IC_CLR_RX_UNDER);
+ stat = i2c_dw_read_clear_intrbits_slave(dev);
+ }
+ if (!i2c_slave_event(dev->slave,
+ I2C_SLAVE_READ_REQUESTED, &val))
+ dw_writel(dev, val, DW_IC_DATA_CMD);
+ }
+ }
+
+ if (stat & DW_IC_INTR_RX_DONE) {
+ if (!i2c_slave_event(dev->slave, I2C_SLAVE_READ_PROCESSED,
+ &val))
+ dw_readl(dev, DW_IC_CLR_RX_DONE);
+
+ i2c_slave_event(dev->slave, I2C_SLAVE_STOP, &val);
+ stat = i2c_dw_read_clear_intrbits_slave(dev);
+ return true;
+ }
+
+ if (stat & DW_IC_INTR_RX_FULL) {
+ val = dw_readl(dev, DW_IC_DATA_CMD);
+ if (!i2c_slave_event(dev->slave, I2C_SLAVE_WRITE_RECEIVED,
+ &val))
+ dev_dbg(dev->dev, "Byte %X acked!", val);
+ } else {
+ i2c_slave_event(dev->slave, I2C_SLAVE_STOP, &val);
+ stat = i2c_dw_read_clear_intrbits_slave(dev);
+ }
+
+ if (stat & DW_IC_INTR_TX_OVER)
+ dw_readl(dev, DW_IC_CLR_TX_OVER);
+
+ return 1;
+}
+
+static irqreturn_t i2c_dw_isr_slave(int this_irq, void *dev_id)
+{
+ struct dw_i2c_dev *dev = dev_id;
+ int ret;
+
+ i2c_dw_read_clear_intrbits_slave(dev);
+ ret = i2c_dw_irq_handler_slave(dev);
+
+ if (ret > 0)
+ complete(&dev->cmd_complete);
+
+ return IRQ_RETVAL(ret);
+}
+
+static struct i2c_algorithm i2c_dw_algo = {
+ .functionality = i2c_dw_func,
+ .reg_slave = i2c_dw_reg_slave,
+ .unreg_slave = i2c_dw_unreg_slave,
+};
+
+void i2c_dw_disable_slave(struct dw_i2c_dev *dev)
+{
+ /* Disable controller. */
+ __i2c_dw_enable_and_wait(dev, false);
+
+ /* Disable all interupts. */
+ dw_writel(dev, 0, DW_IC_INTR_MASK);
+ dw_readl(dev, DW_IC_CLR_INTR);
+}
+EXPORT_SYMBOL_GPL(i2c_dw_disable_slave);
+
+void i2c_dw_disable_int_slave(struct dw_i2c_dev *dev)
+{
+ dw_writel(dev, 0, DW_IC_INTR_MASK);
+}
+EXPORT_SYMBOL_GPL(i2c_dw_disable_int_slave);
+
+u32 i2c_dw_read_comp_param_slave(struct dw_i2c_dev *dev)
+{
+ return dw_readl(dev, DW_IC_COMP_PARAM_1);
+}
+EXPORT_SYMBOL_GPL(i2c_dw_read_comp_param_slave);
+
+int i2c_dw_probe_slave(struct dw_i2c_dev *dev)
+{
+ struct i2c_adapter *adap = &dev->adapter;
+ int ret;
+
+ init_completion(&dev->cmd_complete);
+
+ ret = i2c_dw_init_slave(dev);
+ if (ret)
+ return ret;
+
+ ret = i2c_dw_acquire_lock(dev);
+ if (ret)
+ return ret;
+
+ i2c_dw_release_lock(dev);
+ snprintf(adap->name, sizeof(adap->name),
+ "Synopsys DesignWare I2C Slave adapter");
+ adap->retries = 3;
+ adap->algo = &i2c_dw_algo;
+ adap->dev.parent = dev->dev;
+ i2c_set_adapdata(adap, dev);
+
+ ret = devm_request_irq(dev->dev, dev->irq, i2c_dw_isr_slave,
+ IRQF_SHARED, dev_name(dev->dev), dev);
+ if (ret) {
+ dev_err(dev->dev, "failure requesting irq %i: %d\n",
+ dev->irq, ret);
+ return ret;
+ }
+ /*
+ * Increment PM usage count during adapter registration in order to
+ * avoid possible spurious runtime suspend when adapter device is
+ * registered to the device core and immediate resume in case bus has
+ * registered I2C slaves that do I2C transfers in their probe.
+ */
+ pm_runtime_get_noresume(dev->dev);
+ ret = i2c_add_numbered_adapter(adap);
+ if (ret)
+ dev_err(dev->dev, "failure adding adapter: %d\n", ret);
+ pm_runtime_put_noidle(dev->dev);
+
+ return ret;
+}
+EXPORT_SYMBOL_GPL(i2c_dw_probe_slave);
+
+MODULE_AUTHOR("Luis Oliveira <lolivei@synopsys.com>");
+MODULE_DESCRIPTION("Synopsys DesignWare I2C bus slave adapter");
+MODULE_LICENSE("GPL v2");
--
2.11.0
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox