Re: [PATCH 01/15] power: supply: olpc_battery: correct the temperature units

[Andy Shevchenko]

On Wed, Oct 10, 2018 at 8:23 PM Lubomir Rintel  wrote:
>
> According to [1] and [2], the temperature values are in tenths of degree
> Celsius. Exposing the Celsius value makes the battery appear on fire:
>
>   $ upower -i /org/freedesktop/UPower/devices/battery_olpc_battery
>   ...
>   temperature: 236.9 degrees C
>
> Tested on OLPC XO-1 and OLPC XO-1.75 laptops.

It's interesting that the very author of that code is not included in
so-o long Cc list :)
Cc: David.

David, do you remember if and how you had tested temperature report of
the battery on OLPC?
I guess this kind of error would be appear immediately.

OTOH it might be that power framework had changed requirements (which
would be noticeable change).
If the latter is true, this patch misses Fixes tag. Actually in any
case it misses it.

>
> [1] include/linux/power_supply.h
> [2] Documentation/power/power_supply_class.txt
>
> Cc: sta...@vger.kernel.org
> Signed-off-by: Lubomir Rintel 
> ---
>  drivers/power/supply/olpc_battery.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/power/supply/olpc_battery.c 
> b/drivers/power/supply/olpc_battery.c
> index 6da79ae14860..5a97e42a3547 100644
> --- a/drivers/power/supply/olpc_battery.c
> +++ b/drivers/power/supply/olpc_battery.c
> @@ -428,14 +428,14 @@ static int olpc_bat_get_property(struct power_supply 
> *psy,
> if (ret)
> return ret;
>
> -   val->intval = (s16)be16_to_cpu(ec_word) * 100 / 256;
> +   val->intval = (s16)be16_to_cpu(ec_word) * 10 / 256;
> break;
> case POWER_SUPPLY_PROP_TEMP_AMBIENT:
> ret = olpc_ec_cmd(EC_AMB_TEMP, NULL, 0, (void *)&ec_word, 2);
> if (ret)
> return ret;
>
> -   val->intval = (int)be16_to_cpu(ec_word) * 100 / 256;
> +   val->intval = (int)be16_to_cpu(ec_word) * 10 / 256;
> break;
> case POWER_SUPPLY_PROP_CHARGE_COUNTER:
> ret = olpc_ec_cmd(EC_BAT_ACR, NULL, 0, (void *)&ec_word, 2);
> --
> 2.19.0
>


-- 
With Best Regards,
Andy Shevchenko
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 02/15] Revert "platform/olpc: Make ec explicitly non-modular"

[Andy Shevchenko]

On Wed, Oct 10, 2018 at 8:23 PM Lubomir Rintel  wrote:
>
> It doesn't make sense to always have this built-in, e.g. on ARM
> multiplatform kernels. A better way to address the problem the original
> commit aimed to solve is to fix Kconfig.
>
> This reverts commit f48d1496b8537d75776478c6942dd87f34d7f270.
>

This change doesn't make any sense when put in _this_ order in the series.

First, you need to show the CONFIG_OLPC as tristate, which doesn't (Am
I missing something?).

> Signed-off-by: Lubomir Rintel 
> ---
>  drivers/platform/olpc/olpc-ec.c | 8 +---
>  1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/platform/olpc/olpc-ec.c b/drivers/platform/olpc/olpc-ec.c
> index 374a8028fec7..f99b183d5296 100644
> --- a/drivers/platform/olpc/olpc-ec.c
> +++ b/drivers/platform/olpc/olpc-ec.c
> @@ -1,8 +1,6 @@
>  /*
>   * Generic driver for the OLPC Embedded Controller.
>   *
> - * Author: Andres Salomon 
> - *
>   * Copyright (C) 2011-2012 One Laptop per Child Foundation.
>   *
>   * Licensed under the GPL v2 or later.
> @@ -14,7 +12,7 @@
>  #include 
>  #include 
>  #include 
> -#include 
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -328,4 +326,8 @@ static int __init olpc_ec_init_module(void)
>  {
> return platform_driver_register(&olpc_ec_plat_driver);
>  }
> +
>  arch_initcall(olpc_ec_init_module);
> +
> +MODULE_AUTHOR("Andres Salomon ");
> +MODULE_LICENSE("GPL");
> --
> 2.19.0
>


-- 
With Best Regards,
Andy Shevchenko
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 04/15] Platform: OLPC: Remove an unused include

[Andy Shevchenko]

On Wed, Oct 10, 2018 at 8:23 PM Lubomir Rintel  wrote:
>
> Also, the header is x86 specific, while there are non-x86 OLPC machines.

Same concern. as per patch 2.

Also, you might want to sort headers in alphabetical order.

>
> Signed-off-by: Lubomir Rintel 
> ---
>  drivers/platform/olpc/olpc-ec.c | 1 -
>  1 file changed, 1 deletion(-)
>
> diff --git a/drivers/platform/olpc/olpc-ec.c b/drivers/platform/olpc/olpc-ec.c
> index f99b183d5296..35a21c66cd0d 100644
> --- a/drivers/platform/olpc/olpc-ec.c
> +++ b/drivers/platform/olpc/olpc-ec.c
> @@ -15,7 +15,6 @@
>  #include 
>  #include 
>  #include 
> -#include 
>
>  struct ec_cmd_desc {
>     u8 cmd;
> --
> 2.19.0
>


-- 
With Best Regards,
Andy Shevchenko
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 05/15] Platform: OLPC: Move OLPC config symbol out of x86 tree

[Andy Shevchenko]

On Wed, Oct 10, 2018 at 8:23 PM Lubomir Rintel  wrote:
>
> There are ARM OLPC machines that use mostly the same drivers, including
> EC infrastructure, DCON and Battery.
>
> While at that, fix Kconfig to allow building this as a module.


> -   depends on MOUSE_PS2 && OLPC
> +   depends on MOUSE_PS2 && X86 && OLPC


> -   depends on OLPC && FB
> +   depends on X86 && OLPC && FB

Looking to this I would rather introduce an idiom

depends on OLPC && X86

on the opposite you may do similar for ARM

depends on OLPC && ARM // or ARM64 or whatever it's called

thus, above would look like

depends on MOUSE_PS2
depends on OLPC && X86

and

depends on FB
depends on OLPC && X86

respectively.

-- 
With Best Regards,
Andy Shevchenko
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 07/15] Platform: OLPC: Avoid a warning if the EC didn't register yet

[Andy Shevchenko]

On Wed, Oct 10, 2018 at 8:23 PM Lubomir Rintel  wrote:
>
> Just return ENODEV, so that whoever attempted to use the EC call can
> defer their work.
>
> Signed-off-by: Lubomir Rintel 
> ---
>  drivers/platform/olpc/olpc-ec.c | 7 +--
>  1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/platform/olpc/olpc-ec.c b/drivers/platform/olpc/olpc-ec.c
> index 35a21c66cd0d..342f5bb7f7a8 100644
> --- a/drivers/platform/olpc/olpc-ec.c
> +++ b/drivers/platform/olpc/olpc-ec.c
> @@ -116,8 +116,11 @@ int olpc_ec_cmd(u8 cmd, u8 *inbuf, size_t inlen, u8 
> *outbuf, size_t outlen)
> struct olpc_ec_priv *ec = ec_priv;
> struct ec_cmd_desc desc;
>
> -   /* Ensure a driver and ec hook have been registered */
> -   if (WARN_ON(!ec_driver || !ec_driver->ec_cmd))
> +   /* Driver not yet registered. */
> +   if (!ec_driver)
> +   return -ENODEV;

Why -ENODEV is preferred over -EPROBE_DEFER?

> +
> +   if (WARN_ON(!ec_driver->ec_cmd))
>     return -ENODEV;
>
> if (!ec)
> --
> 2.19.0
>


-- 
With Best Regards,
Andy Shevchenko
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 08/15] Platform: OLPC: Move EC-specific functionality out from x86

[Andy Shevchenko]

On Wed, Oct 10, 2018 at 8:24 PM Lubomir Rintel  wrote:
>
> It is actually plaform independent. Move it to the olpc-ec driver from
> the X86 OLPC platform, so that it could be used by the ARM based laptops
> too.

What is platform independent exactly?

>  #define OLPC_F_PRESENT 0x01
>  #define OLPC_F_DCON0x02
> -#define OLPC_F_EC_WIDE_SCI 0x04

I think these lines grouped on purpose. Thus, I don't like this.
Either move either all or none.

> +int olpc_ec_mask_write(u16 bits)
> +{
>  #ifdef CONFIG_DEBUG_FS
>
>  /*
> @@ -277,14 +369,17 @@ static int olpc_ec_probe(struct platform_device *pdev)
> ec_priv = ec;
> platform_set_drvdata(pdev, ec);
>

> +   /* EC version 0x5f adds support for wide SCI mask */
> +   if (ec->version >= 0x5f) {
> +   __be16 ec_word = cpu_to_be16(bits);
> +

> +   return olpc_ec_cmd(EC_WRITE_EXT_SCI_MASK, (void *) &ec_word, 
> 2,
> +  NULL, 0);

I would leave it on one line.

> +   } else {

> +   unsigned char ec_byte = bits & 0xff;

You may noticed that the parameter is of type u8, which clearly makes
& 0xff part redundant.

> +   return olpc_ec_cmd(EC_WRITE_SCI_MASK, &ec_byte, 1, NULL, 0);
> +   }
> +}
> +EXPORT_SYMBOL_GPL(olpc_ec_mask_write);

I see that the above is inherited from older code, so, no need to
address those comments in here, but consider a follow up fix.


> +int olpc_ec_sci_query(u16 *sci_value)
> +{

> +}
> +EXPORT_SYMBOL_GPL(olpc_ec_sci_query);

Similar comments are applied here.

> +
> -   err = ec_driver->probe ? ec_driver->probe(pdev) : 0;
> +   /* get the EC revision */
> +   err = olpc_ec_cmd(EC_FIRMWARE_REV, NULL, 0,
> +   (unsigned char *) &ec->version, 1);

Perhaps version should be defined as u8.

> +/* SCI source values */
> +#define EC_SCI_SRC_EMPTY0x00
> +#define EC_SCI_SRC_GAME 0x01
> +#define EC_SCI_SRC_BATTERY  0x02
> +#define EC_SCI_SRC_BATSOC   0x04
> +#define EC_SCI_SRC_BATERR   0x08
> +#define EC_SCI_SRC_EBOOK0x10/* XO-1 only */
> +#define EC_SCI_SRC_WLAN 0x20/* XO-1 only */
> +#define EC_SCI_SRC_ACPWR0x40
> +#define EC_SCI_SRC_BATCRIT  0x80
> +#define EC_SCI_SRC_GPWAKE   0x100   /* XO-1.5 only */

BIT() ?

> +#define EC_SCI_SRC_ALL  0x1FF

GENMASK()?

-- 
With Best Regards,
Andy Shevchenko
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 09/15] Platform: OLPC: add a regulator for the DCON

[Andy Shevchenko]

On Wed, Oct 10, 2018 at 8:23 PM Lubomir Rintel  wrote:
>
> All OLPC ECs are able to turn the power to the DCON on an off. Use the
> regulator framework to expose the functionality.

> +static int olpc_ec_set_dcon_power(struct olpc_ec_priv *ec, bool state)
> +{
> +   unsigned char ec_byte = state;
> +   int ret;
> +
> +   if (ec->dcon_enabled == state)
> +   return 0;
> +

> +   ret = olpc_ec_cmd(EC_DCON_POWER_MODE, &ec_byte, 1, NULL, 0);
> +   if (ret == 0)
> +   ec->dcon_enabled = state;
> +
> +   return ret;

I would prefer to see usual pattern, i.e.

if (ret)
 return ret;

...
return 0;

This comment applies to entire series. (Yes, you might address an old
code in a separate patch to be on consistent side)

> +}

> +   return olpc_ec_set_dcon_power(ec, 1);

defined as boolean, supplied an int. Not good.

> +   return olpc_ec_set_dcon_power(ec, 0);

Ditto.

> +static int dcon_regulator_is_enabled(struct regulator_dev *rdev)
> +   return ec->dcon_enabled;

Ditto.

-- 
With Best Regards,
Andy Shevchenko
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 11/15] x86, olpc: Use a correct version when making up a battery node

[Andy Shevchenko]

On Wed, Oct 10, 2018 at 8:23 PM Lubomir Rintel  wrote:
>
> The XO-1 and XO-1.5 batteries apparently differ in an ability to report
> ambient temperature. Add a different compatible string to the 1.5
> battery.

> +int olpc_dt_compatible_match(phandle node, const char *compat)
>  {
> char buf[64];
> +   int plen;
> +   char *p;
> +   int len;
> +
> +   plen = olpc_dt_getproperty(node, "compatible", buf, sizeof(buf));
> +   if (plen <= 0)
> +   return 0;
> +
> +   len = strlen(compat);
> +   for (p = buf; p < buf + plen; p += strlen(p) + 1) {
> +   if (strcmp(p, compat) == 0)
> +   return 1;
> +   }
> +
> +   return 0;
> +}

DT doesn't still have a helper for that?!
I hardly believe in that.

> +   olpc_dt_interpret("\" /battery@0\" find-device"
> +   " \" olpc,xo1.5-battery\" +compatible"
> +   " device-end");

Please, don't split string literals.

> olpc_dt_interpret("\" /pci/display@1\" find-device"
> " new-device"
>     " \" dcon\" device-name \" olpc,xo1-dcon\" 
> +compatible"
> " finish-device device-end");

Yeah, this and similar also needs to be fixed.

-- 
With Best Regards,
Andy Shevchenko
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 12/15] power: supply: olpc_battery: Use DT to get battery version

[Andy Shevchenko]

On Wed, Oct 10, 2018 at 8:23 PM Lubomir Rintel  wrote:
>
> Avoid using the x86 OLPC platform specific call to get the board
> version. It won't work on FDT-based ARM MMP2 platform.
>
> Signed-off-by: Lubomir Rintel 
> ---
>  drivers/power/supply/olpc_battery.c | 10 +++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/power/supply/olpc_battery.c 
> b/drivers/power/supply/olpc_battery.c
> index 5a97e42a3547..540d44bf536f 100644
> --- a/drivers/power/supply/olpc_battery.c
> +++ b/drivers/power/supply/olpc_battery.c
> @@ -19,6 +19,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 

Keep it sorted, otherwise the change is good!

Reviewed-by: Andy Shevchenko 

>
>
> @@ -622,11 +623,13 @@ static int olpc_battery_probe(struct platform_device 
> *pdev)
> olpc_ac = power_supply_register(&pdev->dev, &olpc_ac_desc, NULL);
> if (IS_ERR(olpc_ac))
> return PTR_ERR(olpc_ac);
> -
> -   if (olpc_board_at_least(olpc_board_pre(0xd0))) { /* XO-1.5 */
> +   if (of_property_match_string(pdev->dev.of_node, "compatible",
> +   "olpc,xo1.5-battery") >= 0) {
> +   /* XO-1.5 */
> olpc_bat_desc.properties = olpc_xo15_bat_props;
> olpc_bat_desc.num_properties = 
> ARRAY_SIZE(olpc_xo15_bat_props);
> -   } else { /* XO-1 */
> +   } else {
> +   /* XO-1 */
> olpc_bat_desc.properties = olpc_xo1_bat_props;
> olpc_bat_desc.num_properties = ARRAY_SIZE(olpc_xo1_bat_props);
> }
> @@ -672,6 +675,7 @@ static int olpc_battery_remove(struct platform_device 
> *pdev)
>
>  static const struct of_device_id olpc_battery_ids[] = {
> { .compatible = "olpc,xo1-battery" },
> +   { .compatible = "olpc,xo1.5-battery" },
> {}
>  };
>  MODULE_DEVICE_TABLE(of, olpc_battery_ids);
> --
> 2.19.0
>


-- 
With Best Regards,
Andy Shevchenko
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 13/15] power: supply: olpc_battery: Move priv data to a struct

[Andy Shevchenko]

On Wed, Oct 10, 2018 at 8:24 PM Lubomir Rintel  wrote:
>
> The global variables for private data are not too nice. I'd like some
> more, and that would clutter the global name space even further.
>

Good change!
Reviewed-by: Andy Shevchenko 

> Signed-off-by: Lubomir Rintel 
> ---
>  drivers/power/supply/olpc_battery.c | 73 +++--
>  1 file changed, 38 insertions(+), 35 deletions(-)
>
> diff --git a/drivers/power/supply/olpc_battery.c 
> b/drivers/power/supply/olpc_battery.c
> index 540d44bf536f..2a2d7cc995f0 100644
> --- a/drivers/power/supply/olpc_battery.c
> +++ b/drivers/power/supply/olpc_battery.c
> @@ -53,6 +53,12 @@
>
>  #define BAT_ADDR_MFR_TYPE  0x5F
>
> +struct olpc_battery_data {
> +   struct power_supply *olpc_ac;
> +   struct power_supply *olpc_bat;
> +   char bat_serial[17];
> +};
> +
>  /*
>   * Power
>   */
> @@ -91,11 +97,8 @@ static const struct power_supply_desc olpc_ac_desc = {
> .get_property = olpc_ac_get_prop,
>  };
>
> -static struct power_supply *olpc_ac;
> -
> -static char bat_serial[17]; /* Ick */
> -
> -static int olpc_bat_get_status(union power_supply_propval *val, uint8_t 
> ec_byte)
> +static int olpc_bat_get_status(struct olpc_battery_data *data,
> +   union power_supply_propval *val, uint8_t ec_byte)
>  {
> if (olpc_platform_info.ecver > 0x44) {
> if (ec_byte & (BAT_STAT_CHARGING | BAT_STAT_TRICKLE))
> @@ -326,6 +329,7 @@ static int olpc_bat_get_property(struct power_supply *psy,
>  enum power_supply_property psp,
>  union power_supply_propval *val)
>  {
> +   struct olpc_battery_data *data = power_supply_get_drvdata(psy);
> int ret = 0;
> __be16 ec_word;
> uint8_t ec_byte;
> @@ -347,7 +351,7 @@ static int olpc_bat_get_property(struct power_supply *psy,
>
> switch (psp) {
> case POWER_SUPPLY_PROP_STATUS:
> -   ret = olpc_bat_get_status(val, ec_byte);
> +   ret = olpc_bat_get_status(data, val, ec_byte);
> if (ret)
> return ret;
> break;
> @@ -450,8 +454,8 @@ static int olpc_bat_get_property(struct power_supply *psy,
> if (ret)
> return ret;
>
> -   sprintf(bat_serial, "%016llx", (long 
> long)be64_to_cpu(ser_buf));
> -   val->strval = bat_serial;
> +   sprintf(data->bat_serial, "%016llx", (long 
> long)be64_to_cpu(ser_buf));
> +   val->strval = data->bat_serial;
> break;
> case POWER_SUPPLY_PROP_VOLTAGE_MAX_DESIGN:
> ret = olpc_bat_get_voltage_max_design(val);
> @@ -579,17 +583,17 @@ static struct power_supply_desc olpc_bat_desc = {
> .use_for_apm = 1,
>  };
>
> -static struct power_supply *olpc_bat;
> -
>  static int olpc_battery_suspend(struct platform_device *pdev,
> pm_message_t state)
>  {
> -   if (device_may_wakeup(&olpc_ac->dev))
> +   struct olpc_battery_data *data = platform_get_drvdata(pdev);
> +
> +   if (device_may_wakeup(&data->olpc_ac->dev))
> olpc_ec_wakeup_set(EC_SCI_SRC_ACPWR);
> else
> olpc_ec_wakeup_clear(EC_SCI_SRC_ACPWR);
>
> -   if (device_may_wakeup(&olpc_bat->dev))
> +   if (device_may_wakeup(&data->olpc_bat->dev))
> olpc_ec_wakeup_set(EC_SCI_SRC_BATTERY | EC_SCI_SRC_BATSOC
>| EC_SCI_SRC_BATERR);
> else
> @@ -601,7 +605,8 @@ static int olpc_battery_suspend(struct platform_device 
> *pdev,
>
>  static int olpc_battery_probe(struct platform_device *pdev)
>  {
> -   int ret;
> +   struct power_supply_config psy_cfg = {};
> +   struct olpc_battery_data *data;
> uint8_t status;
>
> /*
> @@ -620,9 +625,13 @@ static int olpc_battery_probe(struct platform_device 
> *pdev)
>
> /* Ignore the status. It doesn't actually matter */
>
> -   olpc_ac = power_supply_register(&pdev->dev, &olpc_ac_desc, NULL);
> -   if (IS_ERR(olpc_ac))
> -   return PTR_ERR(olpc_ac);
> +   psy_cfg.of_node = pdev->dev.of_node;
> +   psy_cfg.drv_data = data;
> +
> +   data->olpc_ac = devm_power_supply_register(&pdev->dev, &olpc_ac_desc, 
> &psy_cf

Re: [PATCH 14/15] power: supply: olpc_battery: Avoid using platform_info

[Andy Shevchenko]

On Wed, Oct 10, 2018 at 8:24 PM Lubomir Rintel  wrote:
>
> This wouldn't work on the DT-based ARM platform. Let's read the EC version
> directly from the EC driver instead.
>
> This makes the driver no longer x86 specific.
>
> Signed-off-by: Lubomir Rintel 
> ---
>  drivers/power/supply/Kconfig|  2 +-
>  drivers/power/supply/olpc_battery.c | 35 +
>  2 files changed, 27 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/power/supply/Kconfig b/drivers/power/supply/Kconfig
> index ff6dab0bf0dd..f0361e4dd457 100644
> --- a/drivers/power/supply/Kconfig
> +++ b/drivers/power/supply/Kconfig
> @@ -151,7 +151,7 @@ config BATTERY_PMU
>
>  config BATTERY_OLPC
> tristate "One Laptop Per Child battery"
> -   depends on X86_32 && OLPC
> +   depends on OLPC
> help
>   Say Y to enable support for the battery on the OLPC laptop.
>
> diff --git a/drivers/power/supply/olpc_battery.c 
> b/drivers/power/supply/olpc_battery.c
> index 2a2d7cc995f0..dde9863e5c4a 100644
> --- a/drivers/power/supply/olpc_battery.c
> +++ b/drivers/power/supply/olpc_battery.c
> @@ -20,8 +20,6 @@
>  #include 
>  #include 

>  #include 

Btw, Kconfig might miss
depends on OF
part.

> -#include 
> -
>
>  #define EC_BAT_VOLTAGE 0x10/* uint16_t,*9.76/32,mV   */
>  #define EC_BAT_CURRENT 0x11/* int16_t, *15.625/120, mA   */
> @@ -57,6 +55,7 @@ struct olpc_battery_data {
> struct power_supply *olpc_ac;
> struct power_supply *olpc_bat;
> char bat_serial[17];
> +   int new_proto;
>  };
>
>  /*
> @@ -100,7 +99,7 @@ static const struct power_supply_desc olpc_ac_desc = {
>  static int olpc_bat_get_status(struct olpc_battery_data *data,
> union power_supply_propval *val, uint8_t ec_byte)
>  {
> -   if (olpc_platform_info.ecver > 0x44) {
> +   if (data->new_proto) {
> if (ec_byte & (BAT_STAT_CHARGING | BAT_STAT_TRICKLE))
> val->intval = POWER_SUPPLY_STATUS_CHARGING;
> else if (ec_byte & BAT_STAT_DISCHARGING)
> @@ -608,14 +607,32 @@ static int olpc_battery_probe(struct platform_device 
> *pdev)
> struct power_supply_config psy_cfg = {};
> struct olpc_battery_data *data;
> uint8_t status;

> +   unsigned char ecver[1];

isn't it simple
 uint8_t ecver;
?

> +   int ret;
> +
> +   data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
> +   if (!data)
> +   return -ENOMEM;
> +   platform_set_drvdata(pdev, data);
> +
> +   /* See if the EC is already there and get the EC revision */
> +   ret = olpc_ec_cmd(EC_FIRMWARE_REV, NULL, 0, ecver, ARRAY_SIZE(ecver));

> +   if (ret) {

> +   if (ret == -ENODEV)
> +   return -EPROBE_DEFER;

Yeah, exactly a question I asked somewhere in the first part of the series.

> +   return ret;
> +   }
>
> -   /*
> -* We've seen a number of EC protocol changes; this driver requires
> -* the latest EC protocol, supported by 0x44 and above.
> -*/
> -   if (olpc_platform_info.ecver < 0x44) {
> +   if (ecver[0] > 0x44) {
> +   /* XO 1 or 1.5 with a new EC firmware. */
> +   data->new_proto = 1;
> +   } else if (ecver[0] < 0x44) {
> +   /*
> +* We've seen a number of EC protocol changes; this driver
> +* requires the latest EC protocol, supported by 0x44 and 
> above.
> +*/
> printk(KERN_NOTICE "OLPC EC version 0x%02x too old for "
> -   "battery driver.\n", olpc_platform_info.ecver);
> +   "battery driver.\n", ecver[0]);
> return -ENXIO;
> }
>
> --
> 2.19.0
>


-- 
With Best Regards,
Andy Shevchenko
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 15/15] power: supply: olpc_battery: Add OLPC XO 1.75 support

[Andy Shevchenko]

On Wed, Oct 10, 2018 at 8:24 PM Lubomir Rintel  wrote:
>
> The battery and the protocol are essentially the same as OLPC XO 1.5,
> but the responses from the EC are LSB first.
>
> Signed-off-by: Lubomir Rintel 
> ---
>  drivers/power/supply/olpc_battery.c | 23 ++-
>  1 file changed, 18 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/power/supply/olpc_battery.c 
> b/drivers/power/supply/olpc_battery.c
> index dde9863e5c4a..2adf33b9f641 100644
> --- a/drivers/power/supply/olpc_battery.c
> +++ b/drivers/power/supply/olpc_battery.c
> @@ -56,6 +56,7 @@ struct olpc_battery_data {
> struct power_supply *olpc_bat;
> char bat_serial[17];
> int new_proto;
> +   int little_endian;
>  };
>
>  /*
> @@ -321,6 +322,14 @@ static int olpc_bat_get_voltage_max_design(union 
> power_supply_propval *val)
> return ret;
>  }
>
> +static s16 ecword_to_cpu(struct olpc_battery_data *data, u16 ec_byte)

ec_byte is misleading name. It's a 16-bit word. and since we called in
I/O accessor it as word, you may do the similar here.

And why result is s16 and not u16?

> +{
> +   if (data->little_endian)
> +   return le16_to_cpu(ec_byte);
> +   else
> +   return be16_to_cpu(ec_byte);

> +
>  /*
>   * Battery properties
>   */
> @@ -393,7 +402,7 @@ static int olpc_bat_get_property(struct power_supply *psy,
> if (ret)
> return ret;
>
> -   val->intval = (s16)be16_to_cpu(ec_word) * 9760L / 32;
> +   val->intval = ecword_to_cpu(data, ec_word) * 9760L / 32;
> break;
> case POWER_SUPPLY_PROP_CURRENT_AVG:
> case POWER_SUPPLY_PROP_CURRENT_NOW:
> @@ -401,7 +410,7 @@ static int olpc_bat_get_property(struct power_supply *psy,
> if (ret)
> return ret;
>
> -   val->intval = (s16)be16_to_cpu(ec_word) * 15625L / 120;
> +   val->intval = ecword_to_cpu(data, ec_word) * 15625L / 120;
> break;
> case POWER_SUPPLY_PROP_CAPACITY:
> ret = olpc_ec_cmd(EC_BAT_SOC, NULL, 0, &ec_byte, 1);
> @@ -432,21 +441,21 @@ static int olpc_bat_get_property(struct power_supply 
> *psy,
> if (ret)
> return ret;
>
> -   val->intval = (s16)be16_to_cpu(ec_word) * 10 / 256;
> +   val->intval = ecword_to_cpu(data, ec_word) * 10 / 256;
> break;
> case POWER_SUPPLY_PROP_TEMP_AMBIENT:
> ret = olpc_ec_cmd(EC_AMB_TEMP, NULL, 0, (void *)&ec_word, 2);
> if (ret)
> return ret;
>
> -   val->intval = (int)be16_to_cpu(ec_word) * 10 / 256;
> +   val->intval = (int)ecword_to_cpu(data, ec_word) * 10 / 256;
> break;
> case POWER_SUPPLY_PROP_CHARGE_COUNTER:
> ret = olpc_ec_cmd(EC_BAT_ACR, NULL, 0, (void *)&ec_word, 2);
> if (ret)
> return ret;
>
> -   val->intval = (s16)be16_to_cpu(ec_word) * 6250 / 15;
> +   val->intval = ecword_to_cpu(data, ec_word) * 6250 / 15;
> break;
> case POWER_SUPPLY_PROP_SERIAL_NUMBER:
> ret = olpc_ec_cmd(EC_BAT_SERIAL, NULL, 0, (void *)&ser_buf, 
> 8);
> @@ -626,6 +635,10 @@ static int olpc_battery_probe(struct platform_device 
> *pdev)
> if (ecver[0] > 0x44) {
> /* XO 1 or 1.5 with a new EC firmware. */
> data->new_proto = 1;
> +   } else if (of_find_compatible_node(NULL, NULL, "olpc,xo1.75-ec")) {
> +   /* XO 1.75 */
> +   data->new_proto = 1;
> +   data->little_endian = 1;
> } else if (ecver[0] < 0x44) {
> /*
>  * We've seen a number of EC protocol changes; this driver
> --
> 2.19.0
>


-- 
With Best Regards,
Andy Shevchenko
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 0/15] Add support for OLPC XO 1.75 Embedded Controller

[Andy Shevchenko]

On Wed, Oct 10, 2018 at 8:23 PM Lubomir Rintel  wrote:
>
> Hi.
>
> This patchset adds support for the Embedded Controller on an OLPC XO
> 1.75 machine. OLPC XO 1.75 is a MMP2 based ARM laptop. It plugs into
> the existing OLPC platform infrastructure, currently used by the x86
> based models.
>
> The EC operates in SPI master mode, meaning the SOC is the SPI slave. It
> uses extra handshake signal to signal readiness of SOC to accept data
> from EC and to initiate a transaction if SOC wishes to submit data.
>
> The SPI slave support for MMP2 was submitted separately:
> https://lore.kernel.org/lkml/20181010170936.316862-1-lkund...@v3.sk/T/#t
>

> THe "power: supply: olpc_battery: correct the temperature" patch was

The

> already sent out separately, but I'm including it because the last
> commit of the set depends on it.
>
> Tested to work on an OLPC XO 1.75 and also tested not to break x86
> support with an OLPC XO 1 machine. I don't have a XO 1.5, but it's
> unlikely this breaks it when XO 1 works.
>
> Thanks in advance for reviews and feedback of any kind.

Thanks for the series.
I'm about to review the patch 6, otherwise read my comments for the
rest and consider addressing them.

>
> Lubo
>


-- 
With Best Regards,
Andy Shevchenko
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 06/15] Platform: OLPC: Add XO-1.75 EC driver

[Andy Shevchenko]

LL, 0, NULL, 0);
> +   mdelay(1000);
> +   }
> +}
> +

> +static void olpc_xo175_ec_power_off(void)
> +{
> +   while (1) {
> +   olpc_ec_cmd(CMD_POWER_OFF, NULL, 0, NULL, 0);
> +   mdelay(1000);
> +   }
> +}
> +

> +#ifdef CONFIG_PM
> +static int olpc_xo175_ec_suspend(struct device *dev)

__maybe_unused  instead of ugly #ifdef?

> +{

> +   struct platform_device *pdev = to_platform_device(dev);
> +   struct olpc_xo175_ec *priv = platform_get_drvdata(pdev);

dev_get_drvdata() or how is it called?

> +   unsigned char hintargs[5];

struct olpc_ec_hint_cmd {
u8 ...
u32 ...
};

?

> +   static unsigned int suspend_count;

u32 I suppose.

> +
> +   suspend_count++;
> +   dev_dbg(dev, "%s: suspend sync %08x\n", __func__, suspend_count);

__func__ can be issued if user asked for via Dynamic Debug interface.

> +   /*
> +* First byte is 1 to indicate suspend, the rest is an integer
> +* counter.
> +*/
> +   hintargs[0] = 1;
> +   memcpy(&hintargs[1], &suspend_count, 4);
> +   olpc_ec_cmd(CMD_SUSPEND_HINT, hintargs, 5, NULL, 0);

What do you need this counter for?

> +   priv->suspended = true;

Hmm... Who is the user of it?

> +   return 0;
> +}
> +
> +static int olpc_xo175_ec_resume_noirq(struct device *dev)
> +{
> +   struct platform_device *pdev = to_platform_device(dev);
> +   struct olpc_xo175_ec *priv = platform_get_drvdata(pdev);
> +
> +   priv->suspended = false;
> +
> +   return 0;
> +}
> +
> +static int olpc_xo175_ec_resume(struct device *dev)
> +{
> +   struct platform_device *pdev = to_platform_device(dev);
> +   struct olpc_xo175_ec *priv = platform_get_drvdata(pdev);

> +   unsigned char x = 0;

u8

> +   priv->suspended = false;

Isn't it redundant since noirq callback above?

> +   /*
> +* The resume hint is only needed if no other commands are
> +* being sent during resume. all it does is tell the EC
> +* the SoC is definitely awake.
> +*/
> +   olpc_ec_cmd(CMD_SUSPEND_HINT, &x, 1, NULL, 0);
> +

> +   /* Enable all EC events while we're awake */
> +   olpc_xo175_ec_set_event_mask(0x);

#define EC_ALL_EVENTS GENMASK(15, 0)

> +}
> +#endif

> +static struct platform_device *olpc_ec;

I would rather see this at the top of file.

> +static int olpc_xo175_ec_probe(struct spi_device *spi)
> +{

> +   if (olpc_ec) {
> +   dev_err(&spi->dev, "OLPC EC already registered.\n");
> +   return -EBUSY;
> +   }

It's racy against parallel probe called. I don't think it would be a
real issue, just let you know.


> +   /* Set up power button input device */
> +   priv->pwrbtn = devm_input_allocate_device(&spi->dev);
> +   if (!priv->pwrbtn)
> +   return -ENOMEM;
> +   priv->pwrbtn->name = "Power Button";
> +   priv->pwrbtn->dev.parent = &spi->dev;
> +   input_set_capability(priv->pwrbtn, EV_KEY, KEY_POWER);
> +   ret = input_register_device(priv->pwrbtn);
> +   if (ret) {
> +   dev_err(&spi->dev, "error registering input device: %d\n", 
> ret);
> +   return ret;
> +   }

I would split out power button driver, but it's up to you.


> +   /* Enable all EC events while we're awake */
> +   olpc_xo175_ec_set_event_mask(0x);

See above about this magic.

> +}

> +#ifdef CONFIG_PM
> +   .suspend= olpc_xo175_ec_suspend,
> +   .resume_noirq   = olpc_xo175_ec_resume_noirq,
> +   .resume = olpc_xo175_ec_resume,
> +#endif

SET_SYSTEM_SLEEP_PM_OPS() ?
SET_NOIRQ_SYSTEM_SLEEP_PM_OPS() ?

> +static const struct of_device_id olpc_xo175_ec_of_match[] = {
> +   { .compatible = "olpc,xo1.75-ec" },

> +   { },

No comma for terminators.

> +};

-- 
With Best Regards,
Andy Shevchenko
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2 06/17] Platform: OLPC: Add XO-1.75 EC driver

[Andy Shevchenko]

On Mon, Dec 3, 2018 at 1:13 AM Darren Hart  wrote:
> On Fri, Nov 16, 2018 at 05:23:52PM +0100, Lubomir Rintel wrote:
> > It's based off the driver from the OLPC kernel sources. Somewhat
> > modernized and cleaned up, for better or worse.
> >
> > Modified to plug into the olpc-ec driver infrastructure (so that battery
> > interface and debugfs could be reused) and the SPI slave framework.

> > - Count the terminating NUL in LOG_BUF_SIZE
> > - Make olpc_xo175_ec_is_valid_cmd() return -EINVAL instead of -1
> >   on error
> > - Spell keyboard/touchpad in full for CHAN_KEYBOARD/TOUCHPAD messages
> > - Use a #define for PM wakeup processing time
> > - Log a message on unknown event
> > - Escape logging payload with %*pE
> > - Replace an open-coded min()
> > - Correct an error code on short read
> > - replaced PM callback #ifdefs with __maybe_unusedm SET_RUNTIME_PM_OPS
> >   and SET_NOIRQ_SYSTEM_SLEEP_PM_OPS
> > - dev_get_drvdata() instead of a round-trip through platform device
> > - s/unsigned char x/u8 x/ in olpc_xo175_ec_resume()
> > - Use GENMASK() instead of 0x for the event mask
> > - Replace cmd tx/resp rx buffers with structures
> > - Turned suspend hint arguments into a struct, and tidied up the comment
>
> Just from these comments, each of these could be a separate patch. You
> can group related things together, or those that change the same line or
> function for example. Order them with cleanups / non-functional-changes
> first, followed by functional changes.
>
> >
> > Basically all of the above is based on the review by Andy Shevchenko.
>
> Andy, what was your intent for Lubomir here? From the above, this looks
> like it should be several patches to me.

This is a new module, I don't see why it can't be one patch. For the
existing code I agree with you.

-- 
With Best Regards,
Andy Shevchenko
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH v1 0/9] x86/platform: Remove SFI framework and users

[Andy Shevchenko]

This is last part of Intel MID (SFI based) removal. We have no more users of it
in the kernel and since SFI has been marked Obsolete for a few years already,
Remove all the stuff altogether.

Note, the more recent platforms (Intel Merrifield and Moorefield) still work as
long as they provide correct ACPI tables.

The series requires two prerequisite branches to be pulled first, i.e.
- one form Rafael's PM tree (currently bleeding-edge)
- one form TIP tree (x86/platform), actually only one patch is needed from it

Due to above it's convenient to proceed all of these via Rafael's PM tree,

Note, atomisp change is tagged by Sakari on behalf of media tree maintainers.

Andy Shevchenko (9):
  media: atomisp: Remove unused header
  cpufreq: sfi-cpufreq: Remove driver for deprecated firmware
  sfi: Remove framework for deprecated firmware
  x86/PCI: Get rid of custom x86 model comparison
  x86/PCI: Describe @reg for type1_access_ok()
  x86/platform/intel-mid: Get rid of intel_scu_ipc_legacy.h
  x86/platform/intel-mid: Drop unused __intel_mid_cpu_chip and Co.
  x86/platform/intel-mid: Remove unused header inclusion in intel-mid.h
  x86/platform/intel-mid: Update Copyright year and drop file names

 Documentation/ABI/testing/sysfs-firmware-sfi  |  15 -
 Documentation/ABI/testing/sysfs-platform-kim  |   2 +-
 MAINTAINERS   |   7 -
 arch/x86/Kconfig  |   7 +-
 arch/x86/include/asm/intel-mid.h  |  65 +--
 arch/x86/include/asm/intel_scu_ipc.h  |   2 -
 arch/x86/include/asm/intel_scu_ipc_legacy.h   |  74 ---
 arch/x86/include/asm/platform_sst_audio.h |   2 -
 arch/x86/kernel/apic/io_apic.c|   4 +-
 arch/x86/kernel/setup.c   |   2 -
 arch/x86/pci/intel_mid_pci.c  |  18 +-
 arch/x86/pci/mmconfig-shared.c|   6 +-
 arch/x86/platform/Makefile|   1 -
 arch/x86/platform/intel-mid/Makefile  |   5 -
 .../platform/intel-mid/device_libs/Makefile   |  23 -
 .../intel-mid/device_libs/platform_bcm43xx.c  | 101 
 .../intel-mid/device_libs/platform_bma023.c   |  16 -
 .../intel-mid/device_libs/platform_bt.c   | 101 
 .../intel-mid/device_libs/platform_emc1403.c  |  39 --
 .../device_libs/platform_gpio_keys.c  |  81 ---
 .../intel-mid/device_libs/platform_lis331.c   |  37 --
 .../intel-mid/device_libs/platform_max7315.c  |  77 ---
 .../intel-mid/device_libs/platform_mpu3050.c  |  32 --
 .../device_libs/platform_mrfld_pinctrl.c  |  39 --
 .../device_libs/platform_mrfld_rtc.c  |  44 --
 .../intel-mid/device_libs/platform_mrfld_sd.c |  43 --
 .../device_libs/platform_mrfld_spidev.c   |  50 --
 .../device_libs/platform_pcal9555a.c  |  95 
 .../intel-mid/device_libs/platform_tc35876x.c |  42 --
 .../intel-mid/device_libs/platform_tca6416.c  |  53 --
 arch/x86/platform/intel-mid/intel-mid.c   |  27 +-
 arch/x86/platform/intel-mid/sfi.c | 419 --
 arch/x86/platform/sfi/Makefile|   2 -
 arch/x86/platform/sfi/sfi.c   | 100 
 drivers/Makefile  |   2 +-
 drivers/cpufreq/Kconfig.x86   |  10 -
 drivers/cpufreq/Makefile  |   1 -
 drivers/cpufreq/sfi-cpufreq.c | 127 -
 drivers/platform/x86/intel_scu_pcidrv.c   |  22 +-
 drivers/sfi/Kconfig   |  18 -
 drivers/sfi/Makefile  |   4 -
 drivers/sfi/sfi_acpi.c| 214 ---
 drivers/sfi/sfi_core.c| 522 --
 drivers/sfi/sfi_core.h|  81 ---
 .../atomisp/include/linux/atomisp_platform.h  |   1 -
 include/linux/sfi.h   | 210 ---
 include/linux/sfi_acpi.h  |  93 
 init/main.c   |   2 -
 48 files changed, 37 insertions(+), 2901 deletions(-)
 delete mode 100644 Documentation/ABI/testing/sysfs-firmware-sfi
 delete mode 100644 arch/x86/include/asm/intel_scu_ipc_legacy.h
 delete mode 100644 arch/x86/platform/intel-mid/device_libs/Makefile
 delete mode 100644 arch/x86/platform/intel-mid/device_libs/platform_bcm43xx.c
 delete mode 100644 arch/x86/platform/intel-mid/device_libs/platform_bma023.c
 delete mode 100644 arch/x86/platform/intel-mid/device_libs/platform_bt.c
 delete mode 100644 arch/x86/platform/intel-mid/device_libs/platform_emc1403.c
 delete mode 100644 arch/x86/platform/intel-mid/device_libs/platform_gpio_keys.c
 delete mode 100644 arch/x86/platform/intel-mid/device_libs/platform_lis331.c
 delete mode 100644 arch/x86/platform/intel-mid/device_libs/platform_max7315.c
 delete mode 100644 arch/x86/platform/intel-mid/device_libs/platform_mpu3050.c
 delete mode 100644 
arch/x86/platform/intel-mid/device_libs/platform_mrfld_pinctrl.c
 delete mode 100644 arch/x86/platform/intel-mid/device_libs/platform_mrfld_rtc

[PATCH v1 4/9] x86/PCI: Get rid of custom x86 model comparison

[Andy Shevchenko]

Switch the platform code to use x86_id_table and accompanying API
instead of custom comparison against x86 CPU model.

This is one of the last users of custom API for that and following
changes will remove it for the good.

Signed-off-by: Andy Shevchenko 
---
 arch/x86/pci/intel_mid_pci.c | 17 +++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/arch/x86/pci/intel_mid_pci.c b/arch/x86/pci/intel_mid_pci.c
index 95e2e6bd8d8c..938a8b7bfe7f 100644
--- a/arch/x86/pci/intel_mid_pci.c
+++ b/arch/x86/pci/intel_mid_pci.c
@@ -28,10 +28,12 @@
 #include 
 #include 
 
+#include 
 #include 
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 
@@ -212,10 +214,17 @@ static int pci_write(struct pci_bus *bus, unsigned int 
devfn, int where,
   where, size, value);
 }
 
+static const struct x86_cpu_id intel_mid_cpu_ids[] = {
+   X86_MATCH_INTEL_FAM6_MODEL(ATOM_SILVERMONT_MID, NULL),
+   {}
+};
+
 static int intel_mid_pci_irq_enable(struct pci_dev *dev)
 {
+   const struct x86_cpu_id *id;
struct irq_alloc_info info;
bool polarity_low;
+   u16 model = 0;
int ret;
u8 gsi;
 
@@ -228,8 +237,12 @@ static int intel_mid_pci_irq_enable(struct pci_dev *dev)
return ret;
}
 
-   switch (intel_mid_identify_cpu()) {
-   case INTEL_MID_CPU_CHIP_TANGIER:
+   id = x86_match_cpu(intel_mid_cpu_ids);
+   if (id)
+   model = id->model;
+
+   switch (model) {
+   case INTEL_FAM6_ATOM_SILVERMONT_MID:
polarity_low = false;
 
/* Special treatment for IRQ0 */
-- 
2.30.0

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH v1 2/9] cpufreq: sfi-cpufreq: Remove driver for deprecated firmware

[Andy Shevchenko]

SFI-based platforms are gone. So does this driver.

Signed-off-by: Andy Shevchenko 
Acked-by: Linus Walleij 
---
 drivers/cpufreq/Kconfig.x86   |  10 ---
 drivers/cpufreq/Makefile  |   1 -
 drivers/cpufreq/sfi-cpufreq.c | 127 --
 3 files changed, 138 deletions(-)
 delete mode 100644 drivers/cpufreq/sfi-cpufreq.c

diff --git a/drivers/cpufreq/Kconfig.x86 b/drivers/cpufreq/Kconfig.x86
index 399526289320..92701a18bdd9 100644
--- a/drivers/cpufreq/Kconfig.x86
+++ b/drivers/cpufreq/Kconfig.x86
@@ -62,16 +62,6 @@ config X86_ACPI_CPUFREQ_CPB
  By enabling this option the acpi_cpufreq driver provides the old
  entry in addition to the new boost ones, for compatibility reasons.
 
-config X86_SFI_CPUFREQ
-   tristate "SFI Performance-States driver"
-   depends on X86_INTEL_MID && SFI
-   help
- This adds a CPUFreq driver for some Silvermont based Intel Atom
- architectures like Z34xx and Z35xx which enumerate processor
- performance states through SFI.
-
- If in doubt, say N.
-
 config ELAN_CPUFREQ
tristate "AMD Elan SC400 and SC410"
depends on MELAN
diff --git a/drivers/cpufreq/Makefile b/drivers/cpufreq/Makefile
index f1b7e3dd6e5d..18c9b0eafd09 100644
--- a/drivers/cpufreq/Makefile
+++ b/drivers/cpufreq/Makefile
@@ -43,7 +43,6 @@ obj-$(CONFIG_X86_P4_CLOCKMOD) += p4-clockmod.o
 obj-$(CONFIG_X86_CPUFREQ_NFORCE2)  += cpufreq-nforce2.o
 obj-$(CONFIG_X86_INTEL_PSTATE) += intel_pstate.o
 obj-$(CONFIG_X86_AMD_FREQ_SENSITIVITY) += amd_freq_sensitivity.o
-obj-$(CONFIG_X86_SFI_CPUFREQ)  += sfi-cpufreq.o
 
 
##
 # ARM SoC drivers
diff --git a/drivers/cpufreq/sfi-cpufreq.c b/drivers/cpufreq/sfi-cpufreq.c
deleted file mode 100644
index 45cfdf67cf03..
--- a/drivers/cpufreq/sfi-cpufreq.c
+++ /dev/null
@@ -1,127 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0-only
-/*
- *  SFI Performance States Driver
- *
- *  Author: Vishwesh M Rudramuni 
- *  Author: Srinidhi Kasagar 
- */
-
-#include 
-#include 
-#include 
-#include 
-#include 
-#include 
-#include 
-
-#include 
-
-static struct cpufreq_frequency_table *freq_table;
-static struct sfi_freq_table_entry *sfi_cpufreq_array;
-static int num_freq_table_entries;
-
-static int sfi_parse_freq(struct sfi_table_header *table)
-{
-   struct sfi_table_simple *sb;
-   struct sfi_freq_table_entry *pentry;
-   int totallen;
-
-   sb = (struct sfi_table_simple *)table;
-   num_freq_table_entries = SFI_GET_NUM_ENTRIES(sb,
-   struct sfi_freq_table_entry);
-   if (num_freq_table_entries <= 1) {
-   pr_err("No p-states discovered\n");
-   return -ENODEV;
-   }
-
-   pentry = (struct sfi_freq_table_entry *)sb->pentry;
-   totallen = num_freq_table_entries * sizeof(*pentry);
-
-   sfi_cpufreq_array = kmemdup(pentry, totallen, GFP_KERNEL);
-   if (!sfi_cpufreq_array)
-   return -ENOMEM;
-
-   return 0;
-}
-
-static int sfi_cpufreq_target(struct cpufreq_policy *policy, unsigned int 
index)
-{
-   unsigned int next_perf_state = 0; /* Index into perf table */
-   u32 lo, hi;
-
-   next_perf_state = policy->freq_table[index].driver_data;
-
-   rdmsr_on_cpu(policy->cpu, MSR_IA32_PERF_CTL, &lo, &hi);
-   lo = (lo & ~INTEL_PERF_CTL_MASK) |
-   ((u32) sfi_cpufreq_array[next_perf_state].ctrl_val &
-   INTEL_PERF_CTL_MASK);
-   wrmsr_on_cpu(policy->cpu, MSR_IA32_PERF_CTL, lo, hi);
-
-   return 0;
-}
-
-static int sfi_cpufreq_cpu_init(struct cpufreq_policy *policy)
-{
-   policy->shared_type = CPUFREQ_SHARED_TYPE_HW;
-   policy->cpuinfo.transition_latency = 10;/* 100us */
-   policy->freq_table = freq_table;
-
-   return 0;
-}
-
-static struct cpufreq_driver sfi_cpufreq_driver = {
-   .flags  = CPUFREQ_CONST_LOOPS,
-   .verify = cpufreq_generic_frequency_table_verify,
-   .target_index   = sfi_cpufreq_target,
-   .init   = sfi_cpufreq_cpu_init,
-   .name   = "sfi-cpufreq",
-   .attr   = cpufreq_generic_attr,
-};
-
-static int __init sfi_cpufreq_init(void)
-{
-   int ret, i;
-
-   /* parse the freq table from SFI */
-   ret = sfi_table_parse(SFI_SIG_FREQ, NULL, NULL, sfi_parse_freq);
-   if (ret)
-   return ret;
-
-   freq_table = kcalloc(num_freq_table_entries + 1, sizeof(*freq_table),
-GFP_KERNEL);
-   if (!freq_table) {
-   ret = -ENOMEM;
-   goto err_free_array;
-   }
-
-   for (i = 0; i < num_freq_table_entries; i++) {
-   freq_table[i].driver_data = i;
-   freq_table[i].frequency = sfi_cpufreq_array[i].freq_mhz * 1000;
-   }
-

[PATCH v1 1/9] media: atomisp: Remove unused header

[Andy Shevchenko]

sfi.h is not anyhow used by the driver. Remove it.

Signed-off-by: Andy Shevchenko 
Acked-by: Sakari Ailus 
Acked-by: Linus Walleij 
---
 drivers/staging/media/atomisp/include/linux/atomisp_platform.h | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/staging/media/atomisp/include/linux/atomisp_platform.h 
b/drivers/staging/media/atomisp/include/linux/atomisp_platform.h
index 5a5121d958ed..8c65733e0255 100644
--- a/drivers/staging/media/atomisp/include/linux/atomisp_platform.h
+++ b/drivers/staging/media/atomisp/include/linux/atomisp_platform.h
@@ -22,7 +22,6 @@
 #include 
 
 #include 
-#include 
 #include 
 #include "atomisp.h"
 
-- 
2.30.0

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH v1 9/9] x86/platform/intel-mid: Update Copyright year and drop file names

[Andy Shevchenko]

Update Copyright year and drop file names from files themselves.

Signed-off-by: Andy Shevchenko 
---
 arch/x86/include/asm/intel-mid.h| 4 ++--
 arch/x86/platform/intel-mid/intel-mid.c | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/x86/include/asm/intel-mid.h b/arch/x86/include/asm/intel-mid.h
index c043e91f45ad..c201083b34f6 100644
--- a/arch/x86/include/asm/intel-mid.h
+++ b/arch/x86/include/asm/intel-mid.h
@@ -1,8 +1,8 @@
 /* SPDX-License-Identifier: GPL-2.0-only */
 /*
- * intel-mid.h: Intel MID specific setup code
+ * Intel MID specific setup code
  *
- * (C) Copyright 2009 Intel Corporation
+ * (C) Copyright 2009, 2021 Intel Corporation
  */
 #ifndef _ASM_X86_INTEL_MID_H
 #define _ASM_X86_INTEL_MID_H
diff --git a/arch/x86/platform/intel-mid/intel-mid.c 
b/arch/x86/platform/intel-mid/intel-mid.c
index 2802b5e4637b..f4592dc7a1c1 100644
--- a/arch/x86/platform/intel-mid/intel-mid.c
+++ b/arch/x86/platform/intel-mid/intel-mid.c
@@ -1,8 +1,8 @@
 // SPDX-License-Identifier: GPL-2.0-only
 /*
- * intel-mid.c: Intel MID platform setup code
+ * Intel MID platform setup code
  *
- * (C) Copyright 2008, 2012 Intel Corporation
+ * (C) Copyright 2008, 2012, 2021 Intel Corporation
  * Author: Jacob Pan (jacob.jun@intel.com)
  * Author: Sathyanarayanan Kuppuswamy 
  */
-- 
2.30.0

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH v1 5/9] x86/PCI: Describe @reg for type1_access_ok()

[Andy Shevchenko]

Describe missed parameter in documentation of type1_access_ok().
Otherwise "make W=1 arch/x86/pci/" produces the following warning:
  CHECK   arch/x86/pci/intel_mid_pci.c
  CC  arch/x86/pci/intel_mid_pci.o
  arch/x86/pci/intel_mid_pci.c:152: warning: Function parameter or member 'reg' 
not described in 'type1_access_ok'

Signed-off-by: Andy Shevchenko 
---
 arch/x86/pci/intel_mid_pci.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/x86/pci/intel_mid_pci.c b/arch/x86/pci/intel_mid_pci.c
index 938a8b7bfe7f..8edd62206604 100644
--- a/arch/x86/pci/intel_mid_pci.c
+++ b/arch/x86/pci/intel_mid_pci.c
@@ -142,6 +142,7 @@ static int pci_device_update_fixed(struct pci_bus *bus, 
unsigned int devfn,
  * type1_access_ok - check whether to use type 1
  * @bus: bus number
  * @devfn: device & function in question
+ * @reg: configuration register offset
  *
  * If the bus is on a Lincroft chip and it exists, or is not on a Lincroft at
  * all, the we can go ahead with any reads & writes.  If it's on a Lincroft,
-- 
2.30.0

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH v1 7/9] x86/platform/intel-mid: Drop unused __intel_mid_cpu_chip and Co.

[Andy Shevchenko]

Since there is no more user of this global variable and associated custom API,
we may safely drop this legacy reinvented a wheel from the kernel sources.

Signed-off-by: Andy Shevchenko 
---
 arch/x86/include/asm/intel-mid.h| 23 ---
 arch/x86/platform/intel-mid/intel-mid.c | 17 -
 2 files changed, 40 deletions(-)

diff --git a/arch/x86/include/asm/intel-mid.h b/arch/x86/include/asm/intel-mid.h
index 6306fe3e5c4a..c2c7da4c60cf 100644
--- a/arch/x86/include/asm/intel-mid.h
+++ b/arch/x86/include/asm/intel-mid.h
@@ -21,36 +21,13 @@ extern void intel_mid_pwr_power_off(void);
 
 extern int intel_mid_pwr_get_lss_id(struct pci_dev *pdev);
 
-/*
- * Medfield is the follow-up of Moorestown, it combines two chip solution into
- * one. Other than that it also added always-on and constant tsc and lapic
- * timers. Medfield is the platform name, and the chip name is called Penwell
- * we treat Medfield/Penwell as a variant of Moorestown. Penwell can be
- * identified via MSRs.
- */
-enum intel_mid_cpu_type {
-   /* 1 was Moorestown */
-   INTEL_MID_CPU_CHIP_PENWELL = 2,
-   INTEL_MID_CPU_CHIP_CLOVERVIEW,
-   INTEL_MID_CPU_CHIP_TANGIER,
-};
-
-extern enum intel_mid_cpu_type __intel_mid_cpu_chip;
-
 #ifdef CONFIG_X86_INTEL_MID
 
-static inline enum intel_mid_cpu_type intel_mid_identify_cpu(void)
-{
-   return __intel_mid_cpu_chip;
-}
-
 extern void intel_scu_devices_create(void);
 extern void intel_scu_devices_destroy(void);
 
 #else /* !CONFIG_X86_INTEL_MID */
 
-#define intel_mid_identify_cpu()   0
-
 static inline void intel_scu_devices_create(void) { }
 static inline void intel_scu_devices_destroy(void) { }
 
diff --git a/arch/x86/platform/intel-mid/intel-mid.c 
b/arch/x86/platform/intel-mid/intel-mid.c
index 846b2ded39d9..2802b5e4637b 100644
--- a/arch/x86/platform/intel-mid/intel-mid.c
+++ b/arch/x86/platform/intel-mid/intel-mid.c
@@ -32,9 +32,6 @@
 #define IPCMSG_COLD_OFF0x80/* Only for Tangier */
 #define IPCMSG_COLD_RESET  0xF1
 
-enum intel_mid_cpu_type __intel_mid_cpu_chip;
-EXPORT_SYMBOL_GPL(__intel_mid_cpu_chip);
-
 static void intel_mid_power_off(void)
 {
/* Shut down South Complex via PWRMU */
@@ -58,29 +55,15 @@ static void __init intel_mid_time_init(void)
 
 static void intel_mid_arch_setup(void)
 {
-   if (boot_cpu_data.x86 != 6) {
-   pr_err("Unknown Intel MID CPU (%d:%d), default to Penwell\n",
-   boot_cpu_data.x86, boot_cpu_data.x86_model);
-   __intel_mid_cpu_chip = INTEL_MID_CPU_CHIP_PENWELL;
-   goto out;
-   }
-
switch (boot_cpu_data.x86_model) {
-   case 0x35:
-   __intel_mid_cpu_chip = INTEL_MID_CPU_CHIP_CLOVERVIEW;
-   break;
case 0x3C:
case 0x4A:
-   __intel_mid_cpu_chip = INTEL_MID_CPU_CHIP_TANGIER;
x86_platform.legacy.rtc = 1;
break;
-   case 0x27:
default:
-   __intel_mid_cpu_chip = INTEL_MID_CPU_CHIP_PENWELL;
break;
}
 
-out:
/*
 * Intel MID platforms are using explicitly defined regulators.
 *
-- 
2.30.0

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH v1 6/9] x86/platform/intel-mid: Get rid of intel_scu_ipc_legacy.h

[Andy Shevchenko]

The header is used by a single user. Move header content to that user.

Signed-off-by: Andy Shevchenko 
Reviewed-by: Mika Westerberg 
---
 arch/x86/include/asm/intel_scu_ipc.h|  2 --
 arch/x86/include/asm/intel_scu_ipc_legacy.h | 18 --
 arch/x86/platform/intel-mid/intel-mid.c |  7 +--
 3 files changed, 5 insertions(+), 22 deletions(-)
 delete mode 100644 arch/x86/include/asm/intel_scu_ipc_legacy.h

diff --git a/arch/x86/include/asm/intel_scu_ipc.h 
b/arch/x86/include/asm/intel_scu_ipc.h
index 11d457af68c5..8537f597d20a 100644
--- a/arch/x86/include/asm/intel_scu_ipc.h
+++ b/arch/x86/include/asm/intel_scu_ipc.h
@@ -65,6 +65,4 @@ static inline int intel_scu_ipc_dev_command(struct 
intel_scu_ipc_dev *scu, int c
   inlen, out, outlen);
 }
 
-#include 
-
 #endif
diff --git a/arch/x86/include/asm/intel_scu_ipc_legacy.h 
b/arch/x86/include/asm/intel_scu_ipc_legacy.h
deleted file mode 100644
index fa529e5ec142..
--- a/arch/x86/include/asm/intel_scu_ipc_legacy.h
+++ /dev/null
@@ -1,18 +0,0 @@
-/* SPDX-License-Identifier: GPL-2.0 */
-#ifndef _ASM_X86_INTEL_SCU_IPC_LEGACY_H_
-#define _ASM_X86_INTEL_SCU_IPC_LEGACY_H_
-
-#include 
-
-#define IPCMSG_COLD_OFF0x80/* Only for Tangier */
-#define IPCMSG_COLD_RESET  0xF1
-
-/* Don't call these in new code - they will be removed eventually */
-
-/* Issue commands to the SCU with or without data */
-static inline int intel_scu_ipc_simple_command(int cmd, int sub)
-{
-   return intel_scu_ipc_dev_simple_command(NULL, cmd, sub);
-}
-
-#endif
diff --git a/arch/x86/platform/intel-mid/intel-mid.c 
b/arch/x86/platform/intel-mid/intel-mid.c
index 2c044e260b4b..846b2ded39d9 100644
--- a/arch/x86/platform/intel-mid/intel-mid.c
+++ b/arch/x86/platform/intel-mid/intel-mid.c
@@ -29,6 +29,9 @@
 #include 
 #include 
 
+#define IPCMSG_COLD_OFF0x80/* Only for Tangier */
+#define IPCMSG_COLD_RESET  0xF1
+
 enum intel_mid_cpu_type __intel_mid_cpu_chip;
 EXPORT_SYMBOL_GPL(__intel_mid_cpu_chip);
 
@@ -38,12 +41,12 @@ static void intel_mid_power_off(void)
intel_mid_pwr_power_off();
 
/* Only for Tangier, the rest will ignore this command */
-   intel_scu_ipc_simple_command(IPCMSG_COLD_OFF, 1);
+   intel_scu_ipc_dev_simple_command(NULL, IPCMSG_COLD_OFF, 1);
 };
 
 static void intel_mid_reboot(void)
 {
-   intel_scu_ipc_simple_command(IPCMSG_COLD_RESET, 0);
+   intel_scu_ipc_dev_simple_command(NULL, IPCMSG_COLD_RESET, 0);
 }
 
 static void __init intel_mid_time_init(void)
-- 
2.30.0

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH v1 8/9] x86/platform/intel-mid: Remove unused header inclusion in intel-mid.h

[Andy Shevchenko]

After the commit f1be6cdaf57c ("x86/platform/intel-mid: Make
intel_scu_device_register() static") the platform_device.h is not being
used anymore by intel-mid.h. Remove it.

Signed-off-by: Andy Shevchenko 
---
 arch/x86/include/asm/intel-mid.h | 1 -
 1 file changed, 1 deletion(-)

diff --git a/arch/x86/include/asm/intel-mid.h b/arch/x86/include/asm/intel-mid.h
index c2c7da4c60cf..c043e91f45ad 100644
--- a/arch/x86/include/asm/intel-mid.h
+++ b/arch/x86/include/asm/intel-mid.h
@@ -8,7 +8,6 @@
 #define _ASM_X86_INTEL_MID_H
 
 #include 
-#include 
 
 extern int intel_mid_pci_init(void);
 extern int intel_mid_pci_set_power_state(struct pci_dev *pdev, pci_power_t 
state);
-- 
2.30.0

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH v1 3/9] sfi: Remove framework for deprecated firmware

[Andy Shevchenko]

SFI-based platforms are gone. So does this framework.

This removes mention of SFI through the drivers and other code as well.

Signed-off-by: Andy Shevchenko 
Reviewed-by: Hans de Goede 
Acked-by: Linus Walleij 
---
 Documentation/ABI/testing/sysfs-firmware-sfi  |  15 -
 Documentation/ABI/testing/sysfs-platform-kim  |   2 +-
 MAINTAINERS   |   7 -
 arch/x86/Kconfig  |   7 +-
 arch/x86/include/asm/intel-mid.h  |  37 --
 arch/x86/include/asm/intel_scu_ipc_legacy.h   |  58 +-
 arch/x86/include/asm/platform_sst_audio.h |   2 -
 arch/x86/kernel/apic/io_apic.c|   4 +-
 arch/x86/kernel/setup.c   |   2 -
 arch/x86/pci/mmconfig-shared.c|   6 +-
 arch/x86/platform/Makefile|   1 -
 arch/x86/platform/intel-mid/Makefile  |   5 -
 .../platform/intel-mid/device_libs/Makefile   |  23 -
 .../intel-mid/device_libs/platform_bcm43xx.c  | 101 
 .../intel-mid/device_libs/platform_bma023.c   |  16 -
 .../intel-mid/device_libs/platform_bt.c   | 101 
 .../intel-mid/device_libs/platform_emc1403.c  |  39 --
 .../device_libs/platform_gpio_keys.c  |  81 ---
 .../intel-mid/device_libs/platform_lis331.c   |  37 --
 .../intel-mid/device_libs/platform_max7315.c  |  77 ---
 .../intel-mid/device_libs/platform_mpu3050.c  |  32 --
 .../device_libs/platform_mrfld_pinctrl.c  |  39 --
 .../device_libs/platform_mrfld_rtc.c  |  44 --
 .../intel-mid/device_libs/platform_mrfld_sd.c |  43 --
 .../device_libs/platform_mrfld_spidev.c   |  50 --
 .../device_libs/platform_pcal9555a.c  |  95 
 .../intel-mid/device_libs/platform_tc35876x.c |  42 --
 .../intel-mid/device_libs/platform_tca6416.c  |  53 --
 arch/x86/platform/intel-mid/intel-mid.c   |   1 -
 arch/x86/platform/intel-mid/sfi.c | 419 --
 arch/x86/platform/sfi/Makefile|   2 -
 arch/x86/platform/sfi/sfi.c   | 100 
 drivers/Makefile  |   2 +-
 drivers/platform/x86/intel_scu_pcidrv.c   |  22 +-
 drivers/sfi/Kconfig   |  18 -
 drivers/sfi/Makefile  |   4 -
 drivers/sfi/sfi_acpi.c| 214 ---
 drivers/sfi/sfi_core.c| 522 --
 drivers/sfi/sfi_core.h|  81 ---
 include/linux/sfi.h   | 210 ---
 include/linux/sfi_acpi.h  |  93 
 init/main.c   |   2 -
 42 files changed, 14 insertions(+), 2695 deletions(-)
 delete mode 100644 Documentation/ABI/testing/sysfs-firmware-sfi
 delete mode 100644 arch/x86/platform/intel-mid/device_libs/Makefile
 delete mode 100644 arch/x86/platform/intel-mid/device_libs/platform_bcm43xx.c
 delete mode 100644 arch/x86/platform/intel-mid/device_libs/platform_bma023.c
 delete mode 100644 arch/x86/platform/intel-mid/device_libs/platform_bt.c
 delete mode 100644 arch/x86/platform/intel-mid/device_libs/platform_emc1403.c
 delete mode 100644 arch/x86/platform/intel-mid/device_libs/platform_gpio_keys.c
 delete mode 100644 arch/x86/platform/intel-mid/device_libs/platform_lis331.c
 delete mode 100644 arch/x86/platform/intel-mid/device_libs/platform_max7315.c
 delete mode 100644 arch/x86/platform/intel-mid/device_libs/platform_mpu3050.c
 delete mode 100644 
arch/x86/platform/intel-mid/device_libs/platform_mrfld_pinctrl.c
 delete mode 100644 arch/x86/platform/intel-mid/device_libs/platform_mrfld_rtc.c
 delete mode 100644 arch/x86/platform/intel-mid/device_libs/platform_mrfld_sd.c
 delete mode 100644 
arch/x86/platform/intel-mid/device_libs/platform_mrfld_spidev.c
 delete mode 100644 arch/x86/platform/intel-mid/device_libs/platform_pcal9555a.c
 delete mode 100644 arch/x86/platform/intel-mid/device_libs/platform_tc35876x.c
 delete mode 100644 arch/x86/platform/intel-mid/device_libs/platform_tca6416.c
 delete mode 100644 arch/x86/platform/intel-mid/sfi.c
 delete mode 100644 arch/x86/platform/sfi/Makefile
 delete mode 100644 arch/x86/platform/sfi/sfi.c
 delete mode 100644 drivers/sfi/Kconfig
 delete mode 100644 drivers/sfi/Makefile
 delete mode 100644 drivers/sfi/sfi_acpi.c
 delete mode 100644 drivers/sfi/sfi_core.c
 delete mode 100644 drivers/sfi/sfi_core.h
 delete mode 100644 include/linux/sfi.h
 delete mode 100644 include/linux/sfi_acpi.h

diff --git a/Documentation/ABI/testing/sysfs-firmware-sfi 
b/Documentation/ABI/testing/sysfs-firmware-sfi
deleted file mode 100644
index 5210e0f06ddb..
--- a/Documentation/ABI/testing/sysfs-firmware-sfi
+++ /dev/null
@@ -1,15 +0,0 @@
-What:  /sys/firmware/sfi/tables/
-Date:  May 2010
-Contact:   Len Brown 
-Description:
-   SFI defines a number of small static memory tables
-   so the kernel can get platform information from firmware.
-
-   The tables are defined in the latest SFI

Re: [PATCH v12] staging: fbtft: add tearing signal detect

[Andy Shevchenko]

On Tue, Feb 2, 2021 at 3:52 AM Carlis  wrote:
> On Mon, 1 Feb 2021 19:40:21 +0200
> Andy Shevchenko  wrote:
>
> > On Sat, Jan 30, 2021 at 8:39 AM carlis  wrote:
> > > On Fri, 29 Jan 2021 16:26:12 +0200
> > > Andy Shevchenko  wrote:
> > > > On Fri, Jan 29, 2021 at 3:56 PM carlis 
> > > > wrote:
> > > > > On Fri, 29 Jan 2021 12:23:08 +0200
> > > > > Andy Shevchenko  wrote:
> >
> > ...
> >
> > > > > Hi, I apologize for what I said in the previous two emails. I
> > > > > missed one email you sent before, and now I probably understand
> > > > > what you meant. Here is a version I modified according to your
> > > > > suggestion:
> >
> > I have realized that you are mocking stuff in the generic fbtft
> > structure for all drivers while only a single one is going to use
> > that. Consider moving everything to the driver in question.

>Do you mean that i define the TE completion and irq_te in the
>fb_st7789v.c as i did before?

Not in global variables. Perhaps it will require to add/update the
custom (to the specific driver) data structure.
But the idea is that all changes should be isolated to that driver.

-- 
With Best Regards,
Andy Shevchenko
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: atomisp: reduce kernel stack usage

[Andy Shevchenko]

On Fri, Feb 26, 2021 at 03:05:02PM +0100, Arnd Bergmann wrote:
> From: Arnd Bergmann 
> 
> The atomisp_set_fmt() function has multiple copies of the large
> v4l2_format structure on its stack, resulting in a warning about
> excessive stack usage in some rare randconfig builds.
> 
> drivers/staging/media/atomisp/pci/atomisp_cmd.c:5600:5: error: stack frame 
> size of 1084 bytes in function 'atomisp_set_fmt' 
> [-Werror,-Wframe-larger-than=]
> 
> Of this structure, only three members in the 'fmt.pix' member are
> used, so simplify it by using the smaller v4l2_pix_format structure
> directly. This reduces the stack usage to 612 bytes, and it could
> be reduced further by only storing the three members that are used.

Good to me!
Reviewed-by: Andy Shevchenko 

> Signed-off-by: Arnd Bergmann 
> ---
>  .../staging/media/atomisp/pci/atomisp_cmd.c   | 65 +--
>  .../staging/media/atomisp/pci/atomisp_cmd.h   |  2 +-
>  .../staging/media/atomisp/pci/atomisp_ioctl.c |  2 +-
>  3 files changed, 33 insertions(+), 36 deletions(-)
> 
> diff --git a/drivers/staging/media/atomisp/pci/atomisp_cmd.c 
> b/drivers/staging/media/atomisp/pci/atomisp_cmd.c
> index 592ea990d4ca..3192c0716eb9 100644
> --- a/drivers/staging/media/atomisp/pci/atomisp_cmd.c
> +++ b/drivers/staging/media/atomisp/pci/atomisp_cmd.c
> @@ -4837,7 +4837,7 @@ static void __atomisp_init_stream_info(u16 stream_index,
>  }
>  
>  /* This function looks up the closest available resolution. */
> -int atomisp_try_fmt(struct video_device *vdev, struct v4l2_format *f,
> +int atomisp_try_fmt(struct video_device *vdev, struct v4l2_pix_format *f,
>   bool *res_overflow)
>  {
>   struct atomisp_device *isp = video_get_drvdata(vdev);
> @@ -4859,18 +4859,18 @@ int atomisp_try_fmt(struct video_device *vdev, struct 
> v4l2_format *f,
>   return -EINVAL;
>  
>   stream_index = atomisp_source_pad_to_stream_id(asd, source_pad);
> - fmt = atomisp_get_format_bridge(f->fmt.pix.pixelformat);
> + fmt = atomisp_get_format_bridge(f->pixelformat);
>   if (!fmt) {
>   dev_err(isp->dev, "unsupported pixelformat!\n");
>   fmt = atomisp_output_fmts;
>   }
>  
> - if (f->fmt.pix.width <= 0 || f->fmt.pix.height <= 0)
> + if (f->width <= 0 || f->height <= 0)
>   return -EINVAL;
>  
>   snr_mbus_fmt->code = fmt->mbus_code;
> - snr_mbus_fmt->width = f->fmt.pix.width;
> - snr_mbus_fmt->height = f->fmt.pix.height;
> + snr_mbus_fmt->width = f->width;
> + snr_mbus_fmt->height = f->height;
>  
>   __atomisp_init_stream_info(stream_index, stream_info);
>  
> @@ -4892,7 +4892,7 @@ int atomisp_try_fmt(struct video_device *vdev, struct 
> v4l2_format *f,
>   return -EINVAL;
>   }
>  
> - f->fmt.pix.pixelformat = fmt->pixelformat;
> + f->pixelformat = fmt->pixelformat;
>  
>   /*
>* If the format is jpeg or custom RAW, then the width and height will
> @@ -4900,17 +4900,17 @@ int atomisp_try_fmt(struct video_device *vdev, struct 
> v4l2_format *f,
>* the below conditions. So just assign to what is being returned from
>* the sensor driver.
>*/
> - if (f->fmt.pix.pixelformat == V4L2_PIX_FMT_JPEG ||
> - f->fmt.pix.pixelformat == V4L2_PIX_FMT_CUSTOM_M10MO_RAW) {
> - f->fmt.pix.width = snr_mbus_fmt->width;
> - f->fmt.pix.height = snr_mbus_fmt->height;
> + if (f->pixelformat == V4L2_PIX_FMT_JPEG ||
> + f->pixelformat == V4L2_PIX_FMT_CUSTOM_M10MO_RAW) {
> + f->width = snr_mbus_fmt->width;
> + f->height = snr_mbus_fmt->height;
>   return 0;
>   }
>  
> - if (snr_mbus_fmt->width < f->fmt.pix.width
> - && snr_mbus_fmt->height < f->fmt.pix.height) {
> - f->fmt.pix.width = snr_mbus_fmt->width;
> - f->fmt.pix.height = snr_mbus_fmt->height;
> + if (snr_mbus_fmt->width < f->width
> + && snr_mbus_fmt->height < f->height) {
> + f->width = snr_mbus_fmt->width;
> + f->height = snr_mbus_fmt->height;
>   /* Set the flag when resolution requested is
>* beyond the max value supported by sensor
>*/
> @@ -4919,12 +4919,10 @@ int atomisp_try_fmt(struct video_device *vdev, struct 
> v4l2_format *f,
>   }
>  
>   /* app vs isp */
> - f->fmt.pix.width = rounddown(
> -clamp_t(u32, f

Re: [driver-core:driver-core-testing 28/31] WARNING: modpost: vmlinux.o(.text.unlikely+0x156c): Section mismatch in reference from the function bitmap_equal() to the variable .init.data:initcall_level

[Andy Shevchenko]

On Sat, Aug 14, 2021 at 4:36 PM Greg Kroah-Hartman
 wrote:
> On Sat, Aug 14, 2021 at 07:03:00PM +0800, kernel test robot wrote:
> > tree:   
> > https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/driver-core.git 
> > driver-core-testing
> > head:   3b35f2a6a625126c57475aa56b5357d8e80b404c
> > commit: 291f93ca339f5b5e6e90ad037bb8271f0f618165 [28/31] lib: test_bitmap: 
> > add bitmap_print_bitmask/list_to_buf test cases
> > config: xtensa-randconfig-r004-20210814 (attached as .config)
> > compiler: xtensa-linux-gcc (GCC) 11.2.0
> > reproduce (this is a W=1 build):
> > wget 
> > https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O 
> > ~/bin/make.cross
> > chmod +x ~/bin/make.cross
> > # 
> > https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/driver-core.git/commit/?id=291f93ca339f5b5e6e90ad037bb8271f0f618165
> > git remote add driver-core 
> > https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/driver-core.git
> > git fetch --no-tags driver-core driver-core-testing
> > git checkout 291f93ca339f5b5e6e90ad037bb8271f0f618165
> > # save the attached .config to linux build tree
> > mkdir build_dir
> > COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross 
> > O=build_dir ARCH=xtensa SHELL=/bin/bash
> >
> > If you fix the issue, kindly add following tag as appropriate
> > Reported-by: kernel test robot 
> >
> > All warnings (new ones prefixed by >>, old ones prefixed by <<):
> >
> > >> WARNING: modpost: vmlinux.o(.text.unlikely+0x156c): Section mismatch in 
> > >> reference from the function bitmap_equal() to the variable 
> > >> .init.data:initcall_level_names
> > The function bitmap_equal() references
> > the variable __initdata initcall_level_names.
> > This is often because bitmap_equal lacks a __initdata
> > annotation or the annotation of initcall_level_names is wrong.
> >
> > The below error/warnings are from parent commit:
> > << WARNING: modpost: vmlinux.o(.data+0x1a86d8): Section mismatch in 
> > reference from the variable qed_mfw_legacy_maps to the variable 
> > .init.rodata:__setup_str_initcall_blacklist
> > << WARNING: modpost: vmlinux.o(.data+0x1a87c8): Section mismatch in 
> > reference from the variable qed_mfw_ext_maps to the variable 
> > .init.rodata:__setup_str_initcall_blacklist
> > << WARNING: modpost: vmlinux.o(.data+0x1a8948): Section mismatch in 
> > reference from the variable qede_forced_speed_maps to the variable 
> > .init.rodata:__setup_str_initcall_blacklist

> Barry, can I get a fix for this?

Max already pointed out, but I guess you were not in Cc list, that
it's a GCC bug in his opinion, but GCC people don't ack it.
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=92938

-- 
With Best Regards,
Andy Shevchenko
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH][next] staging: media: atomisp: fix memory leak of object flash

[Andy Shevchenko]

On Wed, Sep 2, 2020 at 8:02 PM Colin King  wrote:
>
> From: Colin Ian King 
>
> In the case where the call to lm3554_platform_data_func returns an
> error there is a memory leak on the error return path of object
> flash.  Fix this by adding an error return path that will free
> flash and rename labels fail2 to fail3 and fail1 to fail2.
>

Wouldn't be proper fix to move to devm_kmalloc() and return
dev_err_probe() where appropriate?

> Fixes: 9289cdf39992 ("staging: media: atomisp: Convert to GPIO descriptors")
> Signed-off-by: Colin Ian King 
> ---
>  .../media/atomisp/i2c/atomisp-lm3554.c| 19 +++
>  1 file changed, 11 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/staging/media/atomisp/i2c/atomisp-lm3554.c 
> b/drivers/staging/media/atomisp/i2c/atomisp-lm3554.c
> index 7ca7378b1859..5516c98f63bc 100644
> --- a/drivers/staging/media/atomisp/i2c/atomisp-lm3554.c
> +++ b/drivers/staging/media/atomisp/i2c/atomisp-lm3554.c
> @@ -843,8 +843,10 @@ static int lm3554_probe(struct i2c_client *client)
> return -ENOMEM;
>
> flash->pdata = lm3554_platform_data_func(client);
> -   if (IS_ERR(flash->pdata))
> -   return PTR_ERR(flash->pdata);
> +   if (IS_ERR(flash->pdata)) {
> +   err = PTR_ERR(flash->pdata);
> +   goto fail1;
> +   }
>
> v4l2_i2c_subdev_init(&flash->sd, client, &lm3554_ops);
> flash->sd.internal_ops = &lm3554_internal_ops;
> @@ -856,7 +858,7 @@ static int lm3554_probe(struct i2c_client *client)
>ARRAY_SIZE(lm3554_controls));
> if (ret) {
> dev_err(&client->dev, "error initialize a ctrl_handler.\n");
> -   goto fail2;
> +   goto fail3;
> }
>
> for (i = 0; i < ARRAY_SIZE(lm3554_controls); i++)
> @@ -865,14 +867,14 @@ static int lm3554_probe(struct i2c_client *client)
>
> if (flash->ctrl_handler.error) {
> dev_err(&client->dev, "ctrl_handler error.\n");
> -   goto fail2;
> +   goto fail3;
> }
>
> flash->sd.ctrl_handler = &flash->ctrl_handler;
> err = media_entity_pads_init(&flash->sd.entity, 0, NULL);
> if (err) {
> dev_err(&client->dev, "error initialize a media entity.\n");
> -   goto fail1;
> +   goto fail2;
> }
>
> flash->sd.entity.function = MEDIA_ENT_F_FLASH;
> @@ -884,14 +886,15 @@ static int lm3554_probe(struct i2c_client *client)
> err = lm3554_gpio_init(client);
> if (err) {
> dev_err(&client->dev, "gpio request/direction_output fail");
> -   goto fail2;
> +   goto fail3;
> }
> return atomisp_register_i2c_module(&flash->sd, NULL, LED_FLASH);
> -fail2:
> +fail3:
> media_entity_cleanup(&flash->sd.entity);
> v4l2_ctrl_handler_free(&flash->ctrl_handler);
> -fail1:
> +fail2:
> v4l2_device_unregister_subdev(&flash->sd);
> +fail1:
> kfree(flash);
>
> return err;
> --
> 2.27.0
>


-- 
With Best Regards,
Andy Shevchenko
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 5/5] media: atomisp: get rid of -Wsuggest-attribute=format warnings

[Andy Shevchenko]

On Thu, Sep 03, 2020 at 03:57:32PM +0200, Mauro Carvalho Chehab wrote:
> There are some warnings reported by gcc:
>   drivers/staging/media/atomisp//pci/atomisp_compat_css20.c:164:2: 
> warning: function ‘atomisp_css2_dbg_print’ might be a candidate for 
> ‘gnu_printf’ format attribute [-Wsuggest-attribute=format]
>   drivers/staging/media/atomisp//pci/atomisp_compat_css20.c:170:2: 
> warning: function ‘atomisp_css2_dbg_ftrace_print’ might be a candidate for 
> ‘gnu_printf’ format attribute [-Wsuggest-attribute=format]
>   drivers/staging/media/atomisp//pci/atomisp_compat_css20.c:170:2: 
> warning: function ‘atomisp_css2_dbg_ftrace_print’ might be a candidate for 
> ‘gnu_printf’ format attribute [-Wsuggest-attribute=format]
>   drivers/staging/media/atomisp//pci/atomisp_compat_css20.c:176:2: 
> warning: function ‘atomisp_css2_err_print’ might be a candidate for 
> ‘gnu_printf’ format attribute [-Wsuggest-attribute=format]
> 
> That are due to the usage of printf-like messages without
> enabling the error checking logic.
> 
> Add the proper attributes in order to shut up such warnings.

> +static int  __attribute__((format (printf, 1, 0)))
> +atomisp_css2_dbg_ftrace_print(const char *fmt, va_list args)
>  {
>   ftrace_vprintk(fmt, args);
>   return 0;
>  }
>  

Why not to drop it completely as well?

> -static int atomisp_css2_err_print(const char *fmt, va_list args)
> -{
> - vprintk(fmt, args);
> - return 0;
> -}


-- 
With Best Regards,
Andy Shevchenko


___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [RFC PATCH] Add bridge driver to connect sensors to CIO2 device via software nodes on ACPI platforms

[Andy Shevchenko]

On Thu, Sep 17, 2020 at 01:49:41PM +0300, Dan Carpenter wrote:
> On Thu, Sep 17, 2020 at 01:33:43PM +0300, Sakari Ailus wrote:

> > > + int i, ret;
> > 
> > unsigned int i
> > 
> 
> Why?
> 
> For list iterators then "int i;" is best...  For sizes then unsigned is
> sometimes best.  Or if it's part of the hardware spec or network spec
> unsigned is best.  Otherwise unsigned variables cause a ton of bugs.
> They're not as intuitive as signed variables.  Imagine if there is an
> error in this loop and you want to unwind.  With a signed variable you
> can do:
> 
>   while (--i >= 0)
>   cleanup(&bridge.sensors[i]);

Ha-ha. It's actually a counter argument to your stuff because above is the same 
as

while (i--)
cleanup(&bridge.sensors[i]);

with pretty much unsigned int i.

> There are very few times where raising the type maximum from 2 billion
> to 4 billion fixes anything.

-- 
With Best Regards,
Andy Shevchenko


___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [RFC PATCH] Add bridge driver to connect sensors to CIO2 device via software nodes on ACPI platforms

[Andy Shevchenko]

  goto fail_clean_notifier;
> >> +  }

How comes?

...

> >> \ No newline at end of file

???

Be sure you are using good editor.

...

> >> +#include 

Redundant. ACPI headers are designed the way that you are using a single header
in Linux kernel for all. It might be different in drivers/acpi stuff, but not
in general.

> >> +#include 
> >> +#include 
> >> +#include 
> >> +#include 
> >> +#include 
> >> +#include 
> >> +
> >> +#include 
> >> +#include 

Please, keep them sorted. And since it's for media, the media inclusion may be
placed last in a separate group.

...

> >> +#define PROPERTY_ENTRY_NULL   \
> >> +((const struct property_entry) { })
> > Alignment. Same appears to apply to other macros (please indent).
> Yep
> >
> >> +#define SOFTWARE_NODE_NULL\
> >> +((const struct software_node) { })

Why?!

...

> >> +struct software_node cio2_hid_node = { CIO2_HID, };

static ?

Same for other global variables.

...

> >> +struct cio2_bridge bridge = { 0, };

When define as static the assignment will not be needed.

...

> >> +static int read_acpi_block(struct device *dev, char *id, void *data, u32 
> >> size)
> >> +{
> >> +  union acpi_object *obj;
> >> +  struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };

> >> +  struct acpi_handle *dev_handle = ACPI_HANDLE(dev);

Usually we use simple handle if there is no ambiguous reading.

> >> +  int status;

Should be acpi_status

> >> +  u32 buffer_length;
> >> +
> >> +  status = acpi_evaluate_object(dev_handle, id, NULL, &buffer);

> >> +  if (!ACPI_SUCCESS(status))

ACPI_FAILURE()

> >> +  return -ENODEV;
> >> +
> >> +  obj = (union acpi_object *)buffer.pointer;

Why explicit casting?

> >> +  if (!obj || obj->type != ACPI_TYPE_BUFFER) {
> >> +  dev_err(dev, "Could't read acpi buffer\n");

> >> +  status = -ENODEV;

Should have different int type variable for that.

> >> +  goto err;

If there is no obj, you may return directly without freeing.

> >> +  }
> >> +
> >> +  if (obj->buffer.length > size) {
> >> +  dev_err(dev, "Given buffer is too small\n");
> >> +  status = -ENODEV;
> >> +  goto err;
> >> +  }
> >> +
> >> +  memcpy(data, obj->buffer.pointer, min(size, obj->buffer.length));

Does type of size and length the same? Otherwise you need min_t().

> >> +  buffer_length = obj->buffer.length;
> >> +  kfree(buffer.pointer);
> >> +
> >> +  return buffer_length;

> >> +err:

Consider naming labels by what they are about to do. Like
err_free:
here.

> >> +  kfree(buffer.pointer);
> >> +  return status;
> >> +}

> >> +static int get_acpi_ssdb_sensor_data(struct device *dev,
> >> +   struct sensor_bios_data *sensor)
> >> +{
> >> +  struct sensor_bios_data_packed sensor_data;

> >> +  int ret = read_acpi_block(dev, "SSDB", &sensor_data,
> >> +sizeof(sensor_data));

Please, split declaration and assignment especially in the cases where it
requires long lines.

> >> +  if (ret < 0) {
> >> +  dev_err(dev, "Failed to fetch SSDB data\n");
> >> +  return ret;
> >> +  }
> >> +
> >> +  sensor->link = sensor_data.link;
> >> +  sensor->lanes = sensor_data.lanes;
> >> +  sensor->mclkspeed = sensor_data.mclkspeed;
> >> +
> >> +  return 0;
> >> +}

...

> >> +  if (!dev->driver_data) {
> >> +  pr_info("ACPI match for %s, but it has no driver\n",
> >> +  supported_devices[i]);
> >> +  continue;
> >> +  } else {
> >> +  pr_info("Found supported device %s\n",
> >> +  supported_devices[i]);
> >> +  }

Positive conditions are easier to read, but on the other hand 'else' is
redundant in such conditionals (where if branch bails out from the flow).

-- 
With Best Regards,
Andy Shevchenko


___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [RFC PATCH] Add bridge driver to connect sensors to CIO2 device via software nodes on ACPI platforms

[Andy Shevchenko]

On Thu, Sep 17, 2020 at 4:31 PM Kieran Bingham
 wrote:
> On 17/09/2020 10:47, Dan Scally wrote:
> > On 17/09/2020 08:53, Greg KH wrote:
> >> On Wed, Sep 16, 2020 at 10:36:18PM +0100, Daniel Scally wrote:

> >>>  drivers/staging/media/ipu3/Kconfig   |  15 +
> >>>  drivers/staging/media/ipu3/Makefile  |   1 +
> >>>  drivers/staging/media/ipu3/cio2-bridge.c | 448 +++
> >> Why does this have to be in drivers/staging/ at all?  Why not spend the
> >> time to fix it up properly and get it merged correctly?  It's a very
> >> small driver, and should be smaller, so it should not be a lot of work
> >> to do.  And it would be faster to do that than to take it through
> >> staging...
> > I was just under the impression that that was the process to be honest,
> > if that's not right I'll just move it directly to drivers/media/ipu3
>
> The IPU3 driver is still in staging (unless I've missed something), so I
> think this cio2-bridge should stay with it.

You missed something.
https://elixir.bootlin.com/linux/v5.9-rc5/source/drivers/media/pci/intel

IPU3 from Freescale (IIRC) is a different story.

> Hopefully with more users of the IPU3 brought in by this cio2-bridge,
> that will help gather momentum to get the IPU3 developments required
> completed and moved into drivers/media.

-- 
With Best Regards,
Andy Shevchenko
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [RFC PATCH] Add bridge driver to connect sensors to CIO2 device via software nodes on ACPI platforms

[Andy Shevchenko]

On Thu, Sep 17, 2020 at 4:53 PM Dan Scally  wrote:
>
> Hi Andy, thanks for input (as always)

You're welcome! I'm really impressed by your activity in this area.

> On 17/09/2020 13:45, Andy Shevchenko wrote:
> > On Thu, Sep 17, 2020 at 11:52:28AM +0100, Dan Scally wrote:
> >> On 17/09/2020 11:33, Sakari Ailus wrote:

To the point of placement, I think this should go under
drivers/platform/x86 (Adding Hans and Mark, who can express their
opinions).

...

> > Ah, I think you misinterpreted the meaning of above. The above is a switch 
> > how
> > camera device appears either as PCI or an ACPI. So, it effectively means you
> > should *not* have any relation for this HID until you find a platform where 
> > the
> > device is for real enumerated via ACPI.
> >
> Ah, ok. So that was never going to work. Thanks. That does raise another
> question; we have had some testers report failure, which turns out to be
> because on their platforms the definition of their cameras in ACPI is
> never translated into an i2c_client so the cio2-bridge doesn't bind.
> Those have a similar conditional in the _STA method, see CAM1 in this
> DSDT for example:
> https://raw.githubusercontent.com/linux-surface/acpidumps/master/surface_go/dsdt.dsl.
> Is there anything we can do to enable those cameras to be discovered too?

It means that this

...

> >>>> +#define PROPERTY_ENTRY_NULL   \
> >>>> +((const struct property_entry) { })
> >>> Alignment. Same appears to apply to other macros (please indent).
> >> Yep
> >>>> +#define SOFTWARE_NODE_NULL\
> >>>> +((const struct software_node) { })
> > Why?!
> >
> It felt ugly to have the other definitions be macros and not this one,
> but I can change it.

My point is that those macros are simply redundant. The point is to
have a terminator record (all 0:s in the last entry of an array) which
is usually being achieved by allocating memory with kcalloc() which
does implicitly this for you.

-- 
With Best Regards,
Andy Shevchenko
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [RFC PATCH] Add bridge driver to connect sensors to CIO2 device via software nodes on ACPI platforms

[Andy Shevchenko]

On Thu, Sep 17, 2020 at 5:19 PM Kieran Bingham
 wrote:
> On 17/09/2020 15:08, Andy Shevchenko wrote:

...

> Ayee, ok so we have 'half' the driver for IPU3 out of staging.

Correct. And your below analysis is correct.

> From my understanding, the IPU3 consists of two components, the CIO2
> (CSI2 capture), and the IMGU (the ISP).
>
> - drivers/media/pci/intel/ipu3
>
> This is indeed the CIO2 component (config VIDEO_IPU3_CIO2), and that is
> the part that this bridge relates to, so in fact this cio2-bridge should
> probably go there indeed. No need to go through staging.
>
> The files remaining at:
>
> - drivers/staging/media/ipu3
>
> are in fact also for the IPU3 but the ISP component (VIDEO_IPU3_IMGU).
>
> I'm sorry for the confusion, I knew that the ISP was still in staging, I
> hadn't realised the CSI2 receiver (CIO2) was not.
>
> >> Hopefully with more users of the IPU3 brought in by this cio2-bridge,
> >> that will help gather momentum to get the IPU3 developments required
> >> completed and moved into drivers/media.

-- 
With Best Regards,
Andy Shevchenko
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [RFC PATCH] Add bridge driver to connect sensors to CIO2 device via software nodes on ACPI platforms

[Andy Shevchenko]

On Thu, Sep 17, 2020 at 02:36:22PM +0100, Dan Scally wrote:
> On 17/09/2020 13:45, Andy Shevchenko wrote:
> > On Thu, Sep 17, 2020 at 11:52:28AM +0100, Dan Scally wrote:
> >> On 17/09/2020 11:33, Sakari Ailus wrote:
> > I will do better review for next version, assuming you will Cc reviewers and
> > TWIMC people. Below is like small part of comments I may give to the code.
> TWIMC?

Missed this. To Whom It May Concern.

-- 
With Best Regards,
Andy Shevchenko


___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [RFC PATCH] Add bridge driver to connect sensors to CIO2 device via software nodes on ACPI platforms

[Andy Shevchenko]

On Fri, Sep 18, 2020 at 10:51:57AM +0300, Sakari Ailus wrote:
> On Thu, Sep 17, 2020 at 03:45:14PM +0300, Andy Shevchenko wrote:
> > On Thu, Sep 17, 2020 at 11:52:28AM +0100, Dan Scally wrote:
> > > On 17/09/2020 11:33, Sakari Ailus wrote:
> > > > a module and not enlarge everyone's kernel, and the initialisation 
> > > > would at
> > > > the same time take place before the rest of what the CIO2 driver does in
> > > > probe.
> > > I thought of that as well, but wasn't sure which was preferable. I can
> > > compress it into the CIO2 driver though sure.
> > 
> > Sakari, I tend to agree with Dan and have the board file separated from the
> > driver and even framework.
> 
> And it'll be linked to the kernel binary then I suppose?

Solely depends to your Kconfig dependencies and declaration.

>From code perspective you may do it before enumeration of the certain device or
after with reprobe.

> I don't have a strong opinion either way, just thought that this will
> affect anyone using x86 machines, whether or not they have IPU3. I guess it
> could be compiled in if the ipu3-cio2 driver is enabled?

Of course!

-- 
With Best Regards,
Andy Shevchenko


___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [RFC PATCH] Add bridge driver to connect sensors to CIO2 device via software nodes on ACPI platforms

[Andy Shevchenko]

On Mon, Sep 21, 2020 at 02:33:57PM +0100, Dan Scally wrote:
> On 18/09/2020 14:07, Andy Shevchenko wrote:
> > On Fri, Sep 18, 2020 at 10:51:57AM +0300, Sakari Ailus wrote:
> >> On Thu, Sep 17, 2020 at 03:45:14PM +0300, Andy Shevchenko wrote:
> >>> On Thu, Sep 17, 2020 at 11:52:28AM +0100, Dan Scally wrote:
> >>>> On 17/09/2020 11:33, Sakari Ailus wrote:
> >>>>> a module and not enlarge everyone's kernel, and the initialisation 
> >>>>> would at
> >>>>> the same time take place before the rest of what the CIO2 driver does in
> >>>>> probe.
> >>>> I thought of that as well, but wasn't sure which was preferable. I can
> >>>> compress it into the CIO2 driver though sure.
> >>> Sakari, I tend to agree with Dan and have the board file separated from 
> >>> the
> >>> driver and even framework.
> >> And it'll be linked to the kernel binary then I suppose?
> > Solely depends to your Kconfig dependencies and declaration.
> >
> > From code perspective you may do it before enumeration of the certain 
> > device or
> > after with reprobe.
> >
> >> I don't have a strong opinion either way, just thought that this will
> >> affect anyone using x86 machines, whether or not they have IPU3. I guess it
> >> could be compiled in if the ipu3-cio2 driver is enabled?
> > Of course!
> 
> Apologies both - my inexperience is showing here: I thought you couldn't
> make it compile into the kernel where it's dependent on another piece of
> code that's configured to be a module? In my case, ipu3-cio2 plus some
> other dependencies are configured as modules; VIDEO_DEV and VIDEO_V4L2
> notably. Is that not right?

It's not correct.

We specifically have export.h (usually implied by module.h) which provides an
API for symbols that may be used in modules independently on where they are
(in-kernel or in a module).

So, provider does

bar.h:
int foo();

bar.c:
int foo()
{
}
EXPORT_SYMBOL(foo); // normally is EXPORT_SYMBOL_GPL() in use

Consumer:
#include 

ret = foo();

> It would probably make the probe() ordering issues easier if it could be
> compiled in, since I could just rely on late_initcall() in that case and
> I guess that should work.

So, you may have two cases, your module went first, the other module went
first. Some similar problems as your bridge is trying to address are mitigated
in intel_cht_int33fe_typec.c. Look at it. It's not an ideal example, but that's
due to miserable ACPI tables BIOS provided.

-- 
With Best Regards,
Andy Shevchenko


___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v3] media: atomisp: fixes build breakage for ISP2400 due to a cleanup

[Andy Shevchenko]

On Thu, Oct 1, 2020 at 2:17 PM Mauro Carvalho Chehab
 wrote:
>
> A temporary var needed for building with ISP2400 was removed
> by accident on a cleanup patch.
>
> Fix the breakage.

Is this in any of your branches?

>
> Fixes: 852a53a02cf0 ("media: atomisp: get rid of unused vars")
> Signed-off-by: Mauro Carvalho Chehab 
> ---
>  drivers/staging/media/atomisp/pci/sh_css.c | 14 +++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/staging/media/atomisp/pci/sh_css.c 
> b/drivers/staging/media/atomisp/pci/sh_css.c
> index e8c5caf3dfe6..ddee04c8248d 100644
> --- a/drivers/staging/media/atomisp/pci/sh_css.c
> +++ b/drivers/staging/media/atomisp/pci/sh_css.c
> @@ -1365,7 +1365,6 @@ start_binary(struct ia_css_pipe *pipe,
>  {
> assert(pipe);
> /* Acceleration uses firmware, the binary thus can be NULL */
> -   /* assert(binary != NULL); */
>
> if (binary)
> sh_css_metrics_start_binary(&binary->metrics);
> @@ -1381,10 +1380,10 @@ start_binary(struct ia_css_pipe *pipe,
>  #endif
>
>  #if !defined(ISP2401)
> -   if (stream->reconfigure_css_rx) {
> +   if (pipe->stream->reconfigure_css_rx) {
> ia_css_isys_rx_configure(&pipe->stream->csi_rx_config,
>  pipe->stream->config.mode);
> -   stream->reconfigure_css_rx = false;
> +   pipe->stream->reconfigure_css_rx = false;
> }
>  #endif
>  }
> @@ -2820,6 +2819,8 @@ load_preview_binaries(struct ia_css_pipe *pipe) {
> bool need_isp_copy_binary = false;
>  #ifdef ISP2401
> bool sensor = false;
> +#else
> +   bool continuous;
>  #endif
> /* preview only have 1 output pin now */
> struct ia_css_frame_info *pipe_out_info = &pipe->output_info[0];
> @@ -2833,6 +2834,8 @@ load_preview_binaries(struct ia_css_pipe *pipe) {
> online = pipe->stream->config.online;
>  #ifdef ISP2401
> sensor = pipe->stream->config.mode == IA_CSS_INPUT_MODE_SENSOR;
> +#else
> +   continuous = pipe->stream->config.continuous;
>  #endif
>
> if (mycs->preview_binary.info)
> @@ -5987,6 +5990,8 @@ static int load_primary_binaries(
> bool need_ldc = false;
>  #ifdef ISP2401
> bool sensor = false;
> +#else
> +   bool memory, continuous;
>  #endif
> struct ia_css_frame_info prim_in_info,
>prim_out_info,
> @@ -6009,6 +6014,9 @@ static int load_primary_binaries(
> online = pipe->stream->config.online;
>  #ifdef ISP2401
> sensor = (pipe->stream->config.mode == IA_CSS_INPUT_MODE_SENSOR);
> +#else
> +   memory = pipe->stream->config.mode == IA_CSS_INPUT_MODE_MEMORY;
> +   continuous = pipe->stream->config.continuous;
>  #endif
>
> mycs = &pipe->pipe_settings.capture;
> --
> 2.26.2
>


-- 
With Best Regards,
Andy Shevchenko
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v3] media: atomisp: fixes build breakage for ISP2400 due to a cleanup

[Andy Shevchenko]

On Thu, Oct 1, 2020 at 6:55 PM Mauro Carvalho Chehab
 wrote:
> Em Thu, 1 Oct 2020 18:50:12 +0300
> Andy Shevchenko  escreveu:
>
> > On Thu, Oct 1, 2020 at 2:17 PM Mauro Carvalho Chehab
> >  wrote:
> > >
> > > A temporary var needed for building with ISP2400 was removed
> > > by accident on a cleanup patch.
> > >
> > > Fix the breakage.
> >
> > Is this in any of your branches?
>
> I added v3 of the fixes today at the media tree master branch.
>
> If no merge issues happen, it should be at tomorrow's linux-next.

Fixes the issue to me, thanks!
Tested-by: Andy Shevchenko 

-- 
With Best Regards,
Andy Shevchenko
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2] media: atomisp: remove compat_ioctl32 code

[Andy Shevchenko]

On Wed, Oct 07, 2020 at 05:34:15PM +0200, Arnd Bergmann wrote:
> This is one of the last remaining users of compat_alloc_user_space()
> and copy_in_user(), which are in the process of getting removed.
> 
> As of commit 57e6b6f2303e ("media: atomisp_fops.c: disable
> atomisp_compat_ioctl32"), nothing in this file is actually getting used
> as the only reference has been stubbed out.
> 
> Remove the entire file -- anyone willing to restore the functionality
> can equally well just look up the contents in the git history if needed.

Good one!
Reviewed-by: Andy Shevchenko 

> Cc: Sakari Ailus 
> Cc: Hans Verkuil 
> Suggested-by: Christoph Hellwig 
> Signed-off-by: Arnd Bergmann 
> ---
> This is the alternative approach for the patch, removing the already
> dead code instead of just not compiling it.
> ---
>  drivers/staging/media/atomisp/Makefile|1 -
>  drivers/staging/media/atomisp/TODO|5 +
>  .../atomisp/pci/atomisp_compat_ioctl32.c  | 1202 -
>  .../staging/media/atomisp/pci/atomisp_fops.c  |8 +-
>  4 files changed, 8 insertions(+), 1208 deletions(-)
>  delete mode 100644 drivers/staging/media/atomisp/pci/atomisp_compat_ioctl32.c
> 
> diff --git a/drivers/staging/media/atomisp/Makefile 
> b/drivers/staging/media/atomisp/Makefile
> index 1dfad0dd02d0..3bbd4bf4408c 100644
> --- a/drivers/staging/media/atomisp/Makefile
> +++ b/drivers/staging/media/atomisp/Makefile
> @@ -16,7 +16,6 @@ atomisp-objs += \
>   pci/atomisp_acc.o \
>   pci/atomisp_cmd.o \
>   pci/atomisp_compat_css20.o \
> - pci/atomisp_compat_ioctl32.o \
>   pci/atomisp_csi2.o \
>   pci/atomisp_drvfs.o \
>   pci/atomisp_file.o \
> diff --git a/drivers/staging/media/atomisp/TODO 
> b/drivers/staging/media/atomisp/TODO
> index 6987bb2d32cf..2d1ef9eb262a 100644
> --- a/drivers/staging/media/atomisp/TODO
> +++ b/drivers/staging/media/atomisp/TODO
> @@ -120,6 +120,11 @@ TODO
>  for this driver until the other work is done, as there will be a lot
>  of code churn until this driver becomes functional again.
>  
> +16. Fix private ioctls to not need a compat_ioctl handler for running
> +32-bit tasks. The compat code has been removed because of bugs,
> +and should not be needed for modern drivers. Fixing this properly
> +unfortunately means an incompatible ABI change.
> +
>  Limitations
>  ===
>  
> diff --git a/drivers/staging/media/atomisp/pci/atomisp_compat_ioctl32.c 
> b/drivers/staging/media/atomisp/pci/atomisp_compat_ioctl32.c
> deleted file mode 100644
> index e5553df5bad4..
> --- a/drivers/staging/media/atomisp/pci/atomisp_compat_ioctl32.c
> +++ /dev/null
> @@ -1,1202 +0,0 @@
> -// SPDX-License-Identifier: GPL-2.0
> -/*
> - * Support for Intel Camera Imaging ISP subsystem.
> - *
> - * Copyright (c) 2013 Intel Corporation. All Rights Reserved.
> - *
> - * This program is free software; you can redistribute it and/or
> - * modify it under the terms of the GNU General Public License version
> - * 2 as published by the Free Software Foundation.
> - *
> - * 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.
> - *
> - *
> - */
> -#ifdef CONFIG_COMPAT
> -#include 
> -
> -#include 
> -
> -#include "atomisp_internal.h"
> -#include "atomisp_compat.h"
> -#include "atomisp_ioctl.h"
> -#include "atomisp_compat_ioctl32.h"
> -
> -/* Macros borrowed from v4l2-compat-ioctl32.c */
> -
> -#define get_user_cast(__x, __ptr)\
> -({   \
> - get_user(__x, (typeof(*__ptr) __user *)(__ptr));\
> -})
> -
> -#define put_user_force(__x, __ptr)   \
> -({   \
> - put_user((typeof(*__x) __force *)(__x), __ptr); \
> -})
> -
> -/* Use the same argument order as copy_in_user */
> -#define assign_in_user(to, from) \
> -({   \
> - typeof(*from) __assign_tmp; \
> - \
> - get_user_cast(__assign_tmp, from) || put_user(__assign_tmp, to);\
> -})
> -
> -static int get_atomisp_histogram32(struct atomisp_histogram __user *kp,
> -   

Re: [PATCH v2 01/10] firmware: raspberrypi: Introduce rpi_firmware_put()

[Andy Shevchenko]

On Thu, Oct 22, 2020 at 9:06 PM Nicolas Saenz Julienne
 wrote:
>
> When unbinding the firmware device we need to make sure it has no
> consumers left. Otherwise we'd leave them with a firmware handle
> pointing at freed memory.
>
> Keep a reference count of all consumers and make sure they all finished
> unbinding before we do.

Wait, if it's a device, why do we need all these?
get_device() / put_device() along with module_get() / module_put()
should be sufficient, no?

-- 
With Best Regards,
Andy Shevchenko
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2 10/10] pwm: Add Raspberry Pi Firmware based PWM bus

[Andy Shevchenko]

On Thu, Oct 22, 2020 at 9:05 PM Nicolas Saenz Julienne
 wrote:
>
> Adds support to control the PWM bus available in official Raspberry Pi
> PoE HAT. Only RPi's co-processor has access to it, so commands have to
> be sent through RPi's firmware mailbox interface.

>  drivers/pwm/pwm-raspberrypi.c | 221 ++

Name is completely confusing.
Please, make it unique enough to understand that this is exactly the
device it serves for.

For example, pwm-rpi-poe is better.

...

> + *  - Only normal polarity

Can't it be emulated? Isn't it 100% - duty cycle % ?


> +#include 
> +#include 
> +#include 
> +#include 

...

> +   ret = rpi_firmware_property(firmware, RPI_FIRMWARE_SET_POE_HAT_VAL,
> +   &msg, sizeof(msg));
> +   if (ret)
> +   return ret;

> +   else if (msg.ret)

Redundant 'else'

> +   return -EIO;

...

> +   ret = rpi_firmware_property(firmware, RPI_FIRMWARE_GET_POE_HAT_VAL,
> +   &msg, sizeof(msg));
> +   if (ret)
> +   return ret;

> +   else if (msg.ret)

Ditto.

> +   return -EIO;

...

> +   firmware_node = of_get_parent(dev->of_node);
> +   if (!firmware_node) {
> +   dev_err(dev, "Missing firmware node\n");
> +   return -ENOENT;
> +   }
> +
> +   firmware = rpi_firmware_get(firmware_node);
> +   of_node_put(firmware_node);
> +   if (!firmware)
> +   return -EPROBE_DEFER;

Looks like a hack.

...

> +   ret = pwmchip_remove(&rpipwm->chip);
> +   if (!ret)
> +   rpi_firmware_put(rpipwm->firmware);
> +
> +   return ret;

Can't you use the usual pattern?

  ret = ...
  if (ret)
return ret;
  ...
  return 0;

-- 
With Best Regards,
Andy Shevchenko
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH v2] staging: fieldbus: Use %pM format specifier for MAC addresses

[Andy Shevchenko]

Convert to %pM instead of using custom code.

Signed-off-by: Andy Shevchenko 
---
v2: dropped struct removal (Sven), rebased on top of v5.10-rc1
 drivers/staging/fieldbus/anybuss/hms-profinet.c | 5 +
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/staging/fieldbus/anybuss/hms-profinet.c 
b/drivers/staging/fieldbus/anybuss/hms-profinet.c
index 31c43a0a5776..eca7d97b8e85 100644
--- a/drivers/staging/fieldbus/anybuss/hms-profinet.c
+++ b/drivers/staging/fieldbus/anybuss/hms-profinet.c
@@ -66,10 +66,7 @@ static int profi_id_get(struct fieldbus_dev *fbdev, char 
*buf,
   sizeof(response));
if (ret < 0)
return ret;
-   return snprintf(buf, max_size, "%02X:%02X:%02X:%02X:%02X:%02X\n",
-   response.addr[0], response.addr[1],
-   response.addr[2], response.addr[3],
-   response.addr[4], response.addr[5]);
+   return snprintf(buf, max_size, "%pM\n", response.addr);
 }
 
 static bool profi_enable_get(struct fieldbus_dev *fbdev)
-- 
2.28.0

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2] staging: fieldbus: Use %pM format specifier for MAC addresses

[Andy Shevchenko]

On Wed, Oct 28, 2020 at 07:39:58PM -0400, Sven Van Asbroeck wrote:
> On Tue, Oct 27, 2020 at 2:34 PM Andy Shevchenko
>  wrote:
> >
> > +   return snprintf(buf, max_size, "%pM\n", response.addr);
> 
> Judging from a few Outreachy patches that have hit my inbox, snprintf() is
> considered unsafe in a sysfs_get callback. It should be replaced by
> scnprintf() or even better, sysfs_emit(), which was recently introduced
> to address sprintf-variant issues in sysfs callbacks.

It's already applied, I think you can send a follow up for above,
which is good point, but not the scope of the patch (although
they might have been unified together).

-- 
With Best Regards,
Andy Shevchenko


___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v4 01/11] firmware: raspberrypi: Keep count of all consumers

[Andy Shevchenko]

On Thu, Nov 12, 2020 at 6:40 PM Nicolas Saenz Julienne
 wrote:
>
> When unbinding the firmware device we need to make sure it has no
> consumers left. Otherwise we'd leave them with a firmware handle
> pointing at freed memory.
>
> Keep a reference count of all consumers and introduce rpi_firmware_put()
> which will permit automatically decrease the reference count upon
> unbinding consumer drivers.

...

>  /**
> - * rpi_firmware_get - Get pointer to rpi_firmware structure.
>   * @firmware_node:Pointer to the firmware Device Tree node.
>   *
> + * The reference to rpi_firmware has to be released with rpi_firmware_put().
> + *
>   * Returns NULL is the firmware device is not ready.
>   */
>  struct rpi_firmware *rpi_firmware_get(struct device_node *firmware_node)
>  {
> struct platform_device *pdev = of_find_device_by_node(firmware_node);
> +   struct rpi_firmware *fw;
>
> if (!pdev)
> return NULL;
>
> -   return platform_get_drvdata(pdev);
> +   fw = platform_get_drvdata(pdev);
> +   if (!fw)
> +   return NULL;
> +
> +   if (!kref_get_unless_zero(&fw->consumers))
> +   return NULL;

Don't we have a more traditional way of doing this, i.e.
try_module_get() coupled with get_device() ?

> +   return fw;
>  }


-- 
With Best Regards,
Andy Shevchenko
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v12] staging: fbtft: add tearing signal detect

[Andy Shevchenko]

 size_t to_copy;
> +   size_t tx_array_size;
> +   int i;
> +   int ret = 0;
> +   size_t startbyte_size = 0;

Reversed xmas tree order.

> +   fbtft_par_dbg(DEBUG_WRITE_VMEM, par, "st7789v ---%s(offset=%zu, 
> len=%zu)\n",
> + __func__, offset, len);
> +
> +   remain = len / 2;
> +   vmem16 = (u16 *)(par->info->screen_buffer + offset);

> +   if (par->gpio.dc)

Useless duplicate check.

> +   gpiod_set_value(par->gpio.dc, 1);

> +   if (par->gpio.te) {
> +   enable_irq(gpiod_to_irq(par->gpio.te));

Here you should use the IRQ line rather than the GPIO descriptor. See above.

> +   reinit_completion(&spi_panel_te);
> +   ret = wait_for_completion_timeout(&spi_panel_te,
> + 
> msecs_to_jiffies(SPI_PANEL_TE_TIMEOUT));
> +   if (ret == 0)
> +   dev_err(par->info->device, "wait panel TE time 
> out\n");
> +
> +   disable_irq(gpiod_to_irq(par->gpio.te));
> +   }

> +
> +   while (remain) {
> +   to_copy = min(tx_array_size, remain);

> +   dev_dbg(par->info->device, "to_copy=%zu, remain=%zu\n",
> +   to_copy, remain - to_copy);

Like in previous functions create a temporary variable to keep a
pointer to struct device and use it here and everywhere else. It might
save you LOCs and make code easier to read and understand.

> +   for (i = 0; i < to_copy; i++)
> +   txbuf16[i] = cpu_to_be16(vmem16[i]);

If both of them are 16-bit wide, consider moving this to a helper
which somebody can move to byteorder/generic.h in the future.

> +   vmem16 = vmem16 + to_copy;
> +   ret = par->fbtftops.write(par, par->txbuf.buf,
> +startbyte_size + to_copy * 2);
> +   if (ret < 0)
> +   return ret;
> +   remain -= to_copy;
> +   }
> +
> +   return ret;
> +}

-- 
With Best Regards,
Andy Shevchenko
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v12] staging: fbtft: add tearing signal detect

[Andy Shevchenko]

On Thu, Jan 28, 2021 at 4:33 PM Andy Shevchenko
 wrote:
> On Thu, Jan 28, 2021 at 2:58 PM Carlis  wrote:

...

> Taking all together you probably need to create a helper and use it
> inside init_display(), like
>
> static int init_tearing_effect_line(struct fbtft_par *par)
> {
>   struct device *dev = par->info->device;
>   struct gpio_desc *te;
>   int irq, rc;
>
>   te = gpiod_get_optional(dev, "te", GPIOD_IN);
>   if (IS_ERR(te))
>return dev_err_probe(dev, PTR_ERR(te), "Failed to request
> te GPIO\n");

Sorry, here I missed the following:

  /* Absence of TE IRQ is not critical */
  if (!te)
return 0;

>   irq = gpiod_to_irq(te); // this value you have to save in the
> driver's (per device) data structure.
>
>   /* GPIO is locked as an IRQ, we may drop the reference */
>   gpiod_put(te);

...and here:

  if (irq < 0)
return irq;

>   init_completion(&spi_panel_te); // should be in the (per device)
> data structure
>   rc = devm_request_irq(dev, irq,  spi_panel_te_handler,
> IRQF_TRIGGER_RISING, "TE_GPIO", par);
>   if (rc)
> return dev_err_probe(dev, rc, "TE IRQ request failed.\n");
>   disable_irq_nosync(irq);
>   return irq;
> }

-- 
With Best Regards,
Andy Shevchenko
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v12] staging: fbtft: add tearing signal detect

[Andy Shevchenko]

On Fri, Jan 29, 2021 at 7:01 AM carlis  wrote:
> On Thu, 28 Jan 2021 16:33:02 +0200
> Andy Shevchenko  wrote:
> > On Thu, Jan 28, 2021 at 2:58 PM Carlis  wrote:
> >
> > Thanks for your contribution, my comments below.
> >
> > > From: zhangxuezhi 
> >
> > You probably have to configure your Git to use the same account for
> > author and committer.
>
> hi,you mean like below:
> Carlis 
> ?

I meant that you shouldn't probably have a From: line in the commit message.

...

> hi, i have modified it according to your suggestion like below:

Please, go again thru my comments and comments from others and
carefully address all of them everywhere in your contribution. If you
have questions, ask them in reply in the corresponding context.

...

> /**
>  * init_tearing_effect_line() - init tearing effect line

>  *

For example, above was commented on and hasn't been addressed here.

>  * @par: FBTFT parameter object
>  *
>  * Return: 0 on success, < 0 if error occurred.
>  */
> static int init_tearing_effect_line(struct fbtft_par *par)
> {
> struct device *dev = par->info->device;
> struct gpio_desc *te;
> int rc;
>
> te = gpiod_get_optional(dev, "te", GPIOD_IN);
> if (IS_ERR(te))
> return dev_err_probe(dev, PTR_ERR(te), "Failed to
> request te GPIO\n");
>

> if (te) {

This one is not like I suggested.

> par->irq_te = gpiod_to_irq(te);
> gpiod_put(te);
>

> if (par->irq_te) {

This is wrong.

> rc = devm_request_irq(dev,
>   par->irq_te,
> panel_te_handler,
>   IRQF_TRIGGER_RISING,
> "TE_GPIO", par);

Try to use less LOCs.

> if (rc)
> return dev_err_probe(dev, rc, "TE IRQ
> request failed.\n");
>
> disable_irq_nosync(par->irq_te);
> init_completion(&par->panel_te);

> } else {
> return dev_err_probe(dev, par->irq_te, "gpiod
> to TE IRQ failed.\n");
> }

Again, it is not what had been suggested.

> }
>
> return 0;
> }

The rest is better, but we will see later on when you submit a new
version (And I feel it won't be last).

-- 
With Best Regards,
Andy Shevchenko
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v12] staging: fbtft: add tearing signal detect

[Andy Shevchenko]

On Fri, Jan 29, 2021 at 2:47 PM carlis  wrote:
> On Fri, 29 Jan 2021 12:23:08 +0200
> Andy Shevchenko  wrote:
> > On Fri, Jan 29, 2021 at 7:01 AM carlis  wrote:
> > > On Thu, 28 Jan 2021 16:33:02 +0200
> > > Andy Shevchenko  wrote:
> > > > On Thu, Jan 28, 2021 at 2:58 PM Carlis 
> > > > wrote:

...

> > Please, go again thru my comments and comments from others and
> > carefully address all of them everywhere in your contribution. If you
> > have questions, ask them in reply in the corresponding context.

...

> > > /**
> > >  * init_tearing_effect_line() - init tearing effect line
> >
> > >  *
> >
> > For example, above was commented on and hasn't been addressed here.
> >
> hi,here i can not get you.

The above is a blank line which is redundant. It also applied to the
other function in the code.

> > >  * @par: FBTFT parameter object
> > >  *
> > >  * Return: 0 on success, < 0 if error occurred.
> > >  */
> > > static int init_tearing_effect_line(struct fbtft_par *par)
> > > {
> > > struct device *dev = par->info->device;
> > > struct gpio_desc *te;
> > > int rc;
> > >
> > > te = gpiod_get_optional(dev, "te", GPIOD_IN);
> > > if (IS_ERR(te))
> > > return dev_err_probe(dev, PTR_ERR(te), "Failed to
> > > request te GPIO\n");
> > >
> >
> > > if (te) {
> >
> > This one is not like I suggested.
> Why? My thinking is that if the TE is not configured and NULL is
> returned, the initialization can still proceed.

I have suggested to bail out immediately. It will reduce an
indentation level on the below code.

> > > par->irq_te = gpiod_to_irq(te);
> > > gpiod_put(te);
> > >
> >
> > > if (par->irq_te) {
> >
> > This is wrong.
>
> Why? i have read gpiod_to_irq code, if an error occurs, a negative
> value is returned, and zero is not possible,so I need this value to
> determine if TE IRQ is configured

It returns two possible cases:
 - error code (when negative)
 - Linux IRQ number otherwise

You check makes a no-op since in either variant it will proceed to the
request of IRQ, which is wrong in an error case.

> > > rc = devm_request_irq(dev,
> > >   par->irq_te,
> > > panel_te_handler,
> > >   IRQF_TRIGGER_RISING,
> > > "TE_GPIO", par);
> >
> > Try to use less LOCs.
> >
> > > if (rc)
> > > return dev_err_probe(dev, rc, "TE
> > > IRQ request failed.\n");
> > >
> > > disable_irq_nosync(par->irq_te);
> > > init_completion(&par->panel_te);
> >
> > > } else {
> > > return dev_err_probe(dev, par->irq_te,
> > > "gpiod to TE IRQ failed.\n");
> > > }
> >
> > Again, it is not what had been suggested.
> >
> > > }

-- 
With Best Regards,
Andy Shevchenko
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v12] staging: fbtft: add tearing signal detect

[Andy Shevchenko]

On Fri, Jan 29, 2021 at 2:54 PM carlis  wrote:
> On Fri, 29 Jan 2021 12:23:08 +0200
> Andy Shevchenko  wrote:
> > On Fri, Jan 29, 2021 at 7:01 AM carlis  wrote:
> > > On Thu, 28 Jan 2021 16:33:02 +0200
> > > Andy Shevchenko  wrote:

...

> > This one is not like I suggested.
> I don't think I have a problem here, if te GPIO is not configured, it
> should return NULL, if it is configured, it should be greater than 0

Pointers are always greater than 0 or a special NULL case. The
rationale I explained in the previous mail.

...

> > > rc = devm_request_irq(dev,
> > >   par->irq_te,
> > > panel_te_handler,
> > >   IRQF_TRIGGER_RISING,
> > > "TE_GPIO", par);
> >
> > Try to use less LOCs.
>
> LOCs i can not get you

Lines Of Code. Above can occupy less LOCs.

-- 
With Best Regards,
Andy Shevchenko
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v12] staging: fbtft: add tearing signal detect

[Andy Shevchenko]

On Fri, Jan 29, 2021 at 3:56 PM carlis  wrote:
> On Fri, 29 Jan 2021 12:23:08 +0200
> Andy Shevchenko  wrote:

We are almost there, I have no idea what Noralf or others are going to
say though.

...

> Hi, I apologize for what I said in the previous two emails. I missed
> one email you sent before, and now I probably understand what you
> meant. Here is a version I modified according to your suggestion:
>
> From 399e7fb91d1dcba4924cd38cc8283393c80b97e4 Mon Sep 17 00:00:00 2001
> From: Carlis 
> Date: Sun, 24 Jan 2021 22:43:21 +0800
> Subject: [PATCH v13] staging: fbtft: add tearing signal detect
>
> For st7789v IC,when we need continuous full screen refresh, it is best

Missed space after comma.

> to wait for the tearing effect line signal arrive to avoid screen

to arrive

> tearing.

...

> +#define PANEL_TE_TIMEOUT_MS  34 /* 60Hz for 16.6ms, configured as
> 2*16.6ms */ +

Move comment before the definition
/* comment */
#define DEFINITION

Also consider to use 33 ms as closest to what you mentioned in the comment.
Or leave it with mention that you are using ceil() value.

...

> +/**
> + * init_tearing_effect_line() - init tearing effect line

> + *

As per a few previous reviews.
Okay, I have noticed that the existing kernel-doc is written like
this, but it doesn't prevent you from avoiding this little mistake.

> + * @par: FBTFT parameter object
> + *
> + * Return: 0 on success, or a negative error code otherwise.
> + */
> +static int init_tearing_effect_line(struct fbtft_par *par)
> +{
> +   struct device *dev = par->info->device;
> +   struct gpio_desc *te;
> +   int rc;
> +
> +   te = gpiod_get_optional(dev, "te", GPIOD_IN);
> +   if (IS_ERR(te))
> +   return dev_err_probe(dev, PTR_ERR(te), "Failed to
> request te GPIO\n"); +

Below is okay, but needs a comment explaining why we return a success.

> +   if (!te)
> +   return 0;
> +
> +   par->irq_te = gpiod_to_irq(te);
> +
> +   /* GPIO is locked as an IRQ, we may drop the reference */
> +   gpiod_put(te);
> +
> +   if (par->irq_te < 0)
> +   return par->irq_te;

I recommend using a temporary variable. In such a case you won't need
to specifically check for negative error code. So, something like

int irq;

irq = ...

if (irq < 0)
  return irq;

->irq_te = irq;

> +   init_completion(&par->panel_te);
> +   rc = devm_request_irq(dev, par->irq_te, panel_te_handler,
> + IRQF_TRIGGER_RISING, "TE_GPIO", par);

Right. Now it needs a comment explaining the choice of rising edge type of IRQ.

> +   if (rc)
> +   return dev_err_probe(dev, rc, "TE IRQ request
> failed.\n"); +
> +   disable_irq_nosync(par->irq_te);
> +
> +   return 0;
> +}

...

> +   rc = init_tearing_effect_line(par);

> +   if (rc < 0)

Here is no need to specifically check against less than 0,
  if (ret)
will work nicely.

> +   return rc;

...

> +   if (par->irq_te)
> +   write_reg(par, MIPI_DCS_SET_TEAR_ON, 0x00);

Do you need to call MIPI_DCS_SET_TEAR_SCANLINE in this case?

Alos, when there is no IRQ, shouldn't we explicitly call
   write_reg(par, MIPI_DCS_SET_TEAR_OFF);
?

...

>  /**
> + * st7789v_write_vmem16_bus8() - write data to display

> + *

Redundant.

> + * @par: FBTFT parameter object
> + * @offset: offset from screen_buffer
> + * @len: the length of data to be written
> + *

> + * 16 bit pixel over 8-bit databus

Write 16-bit pixels over 8-bit data bus.

> + * Return: 0 on success, or a negative error code otherwise.
> + */

...

> +   if (par->irq_te) {
> +   enable_irq(par->irq_te);
> +   reinit_completion(&par->panel_te);
> +   ret = wait_for_completion_timeout(&par->panel_te,
> +
> msecs_to_jiffies(PANEL_TE_TIMEOUT_MS));
> +   if (ret == 0)
> +   dev_err(dev, "wait panel TE time out\n");

timeout

> +
> +   disable_irq(par->irq_te);
> +   }

...

> + * @panel_te: completion for panel te line

TE line

> + * @irq_te: LCD Chip tearing effect line

"Linux IRQ for LCD..."

-- 
With Best Regards,
Andy Shevchenko
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v12] staging: fbtft: add tearing signal detect

[Andy Shevchenko]

On Sat, Jan 30, 2021 at 8:39 AM carlis  wrote:
> On Fri, 29 Jan 2021 16:26:12 +0200
> Andy Shevchenko  wrote:
> > On Fri, Jan 29, 2021 at 3:56 PM carlis  wrote:
> > > On Fri, 29 Jan 2021 12:23:08 +0200
> > > Andy Shevchenko  wrote:

...

> > > Hi, I apologize for what I said in the previous two emails. I missed
> > > one email you sent before, and now I probably understand what you
> > > meant. Here is a version I modified according to your suggestion:

I have realized that you are mocking stuff in the generic fbtft
structure for all drivers while only a single one is going to use
that. Consider moving everything to the driver in question.


-- 
With Best Regards,
Andy Shevchenko
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH v2] vmbus: Switch to use new generic UUID API

[Andy Shevchenko]

There are new types and helpers that are supposed to be used in new code.

As a preparation to get rid of legacy types and API functions do
the conversion here.

Cc: "K. Y. Srinivasan" 
Cc: Haiyang Zhang 
Cc: Stephen Hemminger 
Cc: de...@linuxdriverproject.org
Signed-off-by: Andy Shevchenko 
---

v2:
- leave uapi untouched (Christoph, Haiyang)
- rebase on top of latest linux-next

 drivers/hv/channel.c  |  4 +-
 drivers/hv/channel_mgmt.c | 18 +++
 drivers/hv/hyperv_vmbus.h |  4 +-
 drivers/hv/vmbus_drv.c| 48 +++
 include/linux/hyperv.h| 98 +++
 5 files changed, 79 insertions(+), 93 deletions(-)

diff --git a/drivers/hv/channel.c b/drivers/hv/channel.c
index ce0ba2062723..7f3f9994e55e 100644
--- a/drivers/hv/channel.c
+++ b/drivers/hv/channel.c
@@ -282,8 +282,8 @@ int vmbus_open(struct vmbus_channel *newchannel,
 EXPORT_SYMBOL_GPL(vmbus_open);
 
 /* Used for Hyper-V Socket: a guest client's connect() to the host */
-int vmbus_send_tl_connect_request(const uuid_le *shv_guest_servie_id,
- const uuid_le *shv_host_servie_id)
+int vmbus_send_tl_connect_request(const guid_t *shv_guest_servie_id,
+ const guid_t *shv_host_servie_id)
 {
struct vmbus_channel_tl_connect_request conn_msg;
int ret;
diff --git a/drivers/hv/channel_mgmt.c b/drivers/hv/channel_mgmt.c
index d01689079e9b..62703b354d6d 100644
--- a/drivers/hv/channel_mgmt.c
+++ b/drivers/hv/channel_mgmt.c
@@ -141,7 +141,7 @@ static const struct vmbus_device vmbus_devs[] = {
 };
 
 static const struct {
-   uuid_le guid;
+   guid_t guid;
 } vmbus_unsupported_devs[] = {
{ HV_AVMA1_GUID },
{ HV_AVMA2_GUID },
@@ -171,26 +171,26 @@ static void vmbus_rescind_cleanup(struct vmbus_channel 
*channel)
spin_unlock_irqrestore(&vmbus_connection.channelmsg_lock, flags);
 }
 
-static bool is_unsupported_vmbus_devs(const uuid_le *guid)
+static bool is_unsupported_vmbus_devs(const guid_t *guid)
 {
int i;
 
for (i = 0; i < ARRAY_SIZE(vmbus_unsupported_devs); i++)
-   if (!uuid_le_cmp(*guid, vmbus_unsupported_devs[i].guid))
+   if (guid_equal(guid, &vmbus_unsupported_devs[i].guid))
return true;
return false;
 }
 
 static u16 hv_get_dev_type(const struct vmbus_channel *channel)
 {
-   const uuid_le *guid = &channel->offermsg.offer.if_type;
+   const guid_t *guid = &channel->offermsg.offer.if_type;
u16 i;
 
if (is_hvsock_channel(channel) || is_unsupported_vmbus_devs(guid))
return HV_UNKNOWN;
 
for (i = HV_IDE; i < HV_UNKNOWN; i++) {
-   if (!uuid_le_cmp(*guid, vmbus_devs[i].guid))
+   if (guid_equal(guid, &vmbus_devs[i].guid))
return i;
}
pr_info("Unknown GUID: %pUl\n", guid);
@@ -561,10 +561,10 @@ static void vmbus_process_offer(struct vmbus_channel 
*newchannel)
atomic_dec(&vmbus_connection.offer_in_progress);
 
list_for_each_entry(channel, &vmbus_connection.chn_list, listentry) {
-   if (!uuid_le_cmp(channel->offermsg.offer.if_type,
-newchannel->offermsg.offer.if_type) &&
-   !uuid_le_cmp(channel->offermsg.offer.if_instance,
-newchannel->offermsg.offer.if_instance)) {
+   if (guid_equal(&channel->offermsg.offer.if_type,
+  &newchannel->offermsg.offer.if_type) &&
+   guid_equal(&channel->offermsg.offer.if_instance,
+  &newchannel->offermsg.offer.if_instance)) {
fnew = false;
break;
}
diff --git a/drivers/hv/hyperv_vmbus.h b/drivers/hv/hyperv_vmbus.h
index a1f6ce6e5974..cb86b133eb4d 100644
--- a/drivers/hv/hyperv_vmbus.h
+++ b/drivers/hv/hyperv_vmbus.h
@@ -312,8 +312,8 @@ extern const struct vmbus_channel_message_table_entry
 
 /* General vmbus interface */
 
-struct hv_device *vmbus_device_create(const uuid_le *type,
- const uuid_le *instance,
+struct hv_device *vmbus_device_create(const guid_t *type,
+ const guid_t *instance,
  struct vmbus_channel *channel);
 
 int vmbus_device_register(struct hv_device *child_device_obj);
diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
index 403fee01572c..126c2de39e35 100644
--- a/drivers/hv/vmbus_drv.c
+++ b/drivers/hv/vmbus_drv.c
@@ -654,38 +654,28 @@ static int vmbus_uevent(struct device *device, struct 
kobj_uevent_env *env)
return ret;
 }
 
-static const uuid_le null_guid;
-
-static inline bool is_null_guid(const uuid_le *guid)
-{
-   if (uuid_le_cmp(*guid, null_guid))
-

Re: [PATCH 04/22] media: Move v4l2_fwnode_parse_link from v4l2 to driver base

[Andy Shevchenko]

On Tue, Aug 6, 2019 at 2:37 AM Steve Longerbeam  wrote:
>
> There is nothing v4l2-specific about v4l2_fwnode_{parse|put}_link().
> Make these functions more generally available by moving them to driver
> base, with the appropriate name changes to the functions and struct.
>
> In the process embed a 'struct fwnode_endpoint' in 'struct fwnode_link'
> for both sides of the link, and make use of fwnode_graph_parse_endpoint()
> to fully parse both endpoints. Rename members local_node and
> remote_node to more descriptive local_port_parent and
> remote_port_parent.
>

May I ask if it's going to be used outside of v4l2?
Any user in mind?

-- 
With Best Regards,
Andy Shevchenko
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [staging:staging-testing 314/401] drivers/iio/common/hid-sensors/hid-sensor-attributes.c:312: undefined reference to `__udivdi3'

[Andy Shevchenko]

On Wed, Sep 04, 2019 at 11:33:50AM +0800, kbuild test robot wrote:
> tree:   
> https://kernel.googlesource.com/pub/scm/linux/kernel/git/gregkh/staging.git 
> staging-testing
> head:   74eb9c06b1d722468db397595ac6834b9e4ac235
> commit: 473d12f7638c93acbd9296a8cd455b203d5eb528 [314/401] iio: 
> hid-sensor-attributes: Convert to use int_pow()
> config: i386-randconfig-e004-201935 (attached as .config)
> compiler: gcc-7 (Debian 7.4.0-11) 7.4.0
> reproduce:
> git checkout 473d12f7638c93acbd9296a8cd455b203d5eb528
> # save the attached .config to linux build tree
> make ARCH=i386 
> 
> If you fix the issue, kindly add following tag
> Reported-by: kbuild test robot 
> 
> All errors (new ones prefixed by >>):

So, as far as I understood it wasn't compiled on 32-bit before, so, it's not a
new error and thus would (has to?) be fixed separately.

>ld: drivers/iio/common/hid-sensors/hid-sensor-attributes.o: in function 
> `adjust_exponent_nano':
> >> drivers/iio/common/hid-sensors/hid-sensor-attributes.c:312: undefined 
> >> reference to `__udivdi3'
> >> ld: drivers/iio/common/hid-sensors/hid-sensor-attributes.c:314: undefined 
> >> reference to `__umoddi3'
> >> ld: drivers/iio/common/hid-sensors/hid-sensor-attributes.c:324: undefined 
> >> reference to `__udivdi3'
>ld: drivers/iio/common/hid-sensors/hid-sensor-attributes.c:325: undefined 
> reference to `__umoddi3'


-- 
With Best Regards,
Andy Shevchenko


___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH v1 2/5] fbtft: Describe function parameters in kernel-doc

[Andy Shevchenko]

Kernel documentation script complains that some of the function parameters
are not described:

drivers/staging/fbtft/fbtft-core.c:543: warning: Function parameter or member 
'pdata' not described in 'fbtft_framebuffer_alloc'

Describe function parameters where it's appropriate.

Signed-off-by: Andy Shevchenko 
---
 drivers/staging/fbtft/fbtft-core.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/staging/fbtft/fbtft-core.c 
b/drivers/staging/fbtft/fbtft-core.c
index 61f0286fb157..2122c4407bdb 100644
--- a/drivers/staging/fbtft/fbtft-core.c
+++ b/drivers/staging/fbtft/fbtft-core.c
@@ -529,6 +529,7 @@ static void fbtft_merge_fbtftops(struct fbtft_ops *dst, 
struct fbtft_ops *src)
  *
  * @display: pointer to structure describing the display
  * @dev: pointer to the device for this fb, this can be NULL
+ * @pdata: platform data for the display in use
  *
  * Creates a new frame buffer info structure.
  *
-- 
2.24.0

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v3 00/26] compat_ioctl: cleanups

[Andy Shevchenko]

On Tue, Apr 16, 2019 at 11:23 PM Arnd Bergmann  wrote:
>
> Hi Al,
>
> It took me way longer than I had hoped to revisit this series, see
> https://lore.kernel.org/lkml/20180912150142.157913-1-a...@arndb.de/
> for the previously posted version.
>
> I've come to the point where all conversion handlers and most
> COMPATIBLE_IOCTL() entries are gone from this file, but for
> now, this series only has the parts that have either been reviewed
> previously, or that are simple enough to include.
>
> The main missing piece is the SG_IO/SG_GET_REQUEST_TABLE conversion.
> I'll post the patches I made for that later, as they need more
> testing and review from the scsi maintainers.
>
> I hope you can still take these for the coming merge window, unless
> new problems come up.

>  drivers/platform/x86/wmi.c  |   2 +-

Acked-by: Andy Shevchenko 

-- 
With Best Regards,
Andy Shevchenko
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 03/16] lib,treewide: add new match_string() helper/macro

[Andy Shevchenko]

  
>  static int name_card(struct snd_oxfw *oxfw)
> diff --git a/sound/soc/codecs/max98088.c b/sound/soc/codecs/max98088.c
> index 3ef743075bda..911ffe84c37e 100644
> --- a/sound/soc/codecs/max98088.c
> +++ b/sound/soc/codecs/max98088.c
> @@ -1405,7 +1405,7 @@ static int max98088_get_channel(struct 
> snd_soc_component *component, const char
>  {
>   int ret;
>  
> - ret = __match_string(eq_mode_name, ARRAY_SIZE(eq_mode_name), name);
> + ret = match_string(eq_mode_name, name);
>   if (ret < 0)
>   dev_err(component->dev, "Bad EQ channel name '%s'\n", name);
>   return ret;
> diff --git a/sound/soc/codecs/max98095.c b/sound/soc/codecs/max98095.c
> index cd69916d5dcb..d182d45d0c83 100644
> --- a/sound/soc/codecs/max98095.c
> +++ b/sound/soc/codecs/max98095.c
> @@ -1636,7 +1636,7 @@ static int max98095_get_bq_channel(struct 
> snd_soc_component *component,
>  {
>   int ret;
>  
> - ret = __match_string(bq_mode_name, ARRAY_SIZE(bq_mode_name), name);
> + ret = match_string(bq_mode_name, name);
>   if (ret < 0)
>   dev_err(component->dev, "Bad biquad channel name '%s'\n", name);
>   return ret;
> -- 
> 2.17.1
> 

-- 
With Best Regards,
Andy Shevchenko


___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 48/87] rtl8723bs: os_dep: replace rtw_malloc and memset with rtw_zmalloc

[Andy Shevchenko]

On Thu, Jun 27, 2019 at 8:41 PM Fuqian Huang  wrote:
>
> rtw_malloc + memset(0) -> rtw_zmalloc

I have a feeling that everything under os_dep folder should be
replaced with native kernel APIs.

>  drivers/staging/rtl8723bs/os_dep/ioctl_cfg80211.c |  8 ++--
>  drivers/staging/rtl8723bs/os_dep/ioctl_linux.c| 12 +++-

-- 
With Best Regards,
Andy Shevchenko
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: atomisp: add a driver for ov5648 camera sensor

[Andy Shevchenko]

On Mon, 2017-09-11 at 20:03 +0200, Devid Antonio Filoni wrote:
> On Mon, Sep 11, 2017 at 05:55:29PM +0300, Sakari Ailus wrote:
> > Hi Devid,
> > 
> > Please see my comments below.
> > 
> > Andy: please look for "INT5648".
> 
> Hi Sakari,
> I'm replying below to your comments. I'll work on a v2 patch as soon
> as we get
> more comments.
> 
> About "INT5648", I extracted it from the DSDT of my Lenovo Miix 310,
> take a look
> at https://pastebin.com/ExHWYr8g .

First of all, thank you, Sakari, to raise a flag here.

Second, Devid, please answer to the following:
is it an official BIOS which is available in the wild?

If it's so, please, add a paragraph to the commit message explaining how do you 
get this and point to the DSDT excerpt.
Put an answer to above question to the commit message as well.

-- 
Andy Shevchenko 
Intel Finland Oy
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] [media] staging: atomisp: use clock framework for camera clocks

[Andy Shevchenko]

On Tue, 2017-09-19 at 15:45 -0500, Pierre-Louis Bossart wrote:
> The Atom ISP driver initializes and configures PMC clocks which are
> already handled by the clock framework.
> 
> Remove all legacy vlv2_platform_clock stuff and move to the clk API to
> avoid conflicts, e.g. with audio machine drivers enabling the MCLK for
> external codecs
> 

I think it might have a Fixes: tag as well (though I dunno which commit
could be considered as anchor).

(I doubt Git is so clever to remove files based on information out of
the diff, can you check it and if needed to resend without -D implied?)

Other than that - nice clean up!

Reviewed-by: Andy Shevchenko 


> Tested-by: Carlo Caione 
> Signed-off-by: Pierre-Louis Bossart  com>
> ---
>  drivers/staging/media/atomisp/Kconfig  |   1 +
>  drivers/staging/media/atomisp/platform/Makefile|   1 -
>  .../staging/media/atomisp/platform/clock/Makefile  |   6 -
>  .../platform/clock/platform_vlv2_plat_clk.c|  40 
>  .../platform/clock/platform_vlv2_plat_clk.h|  27 ---
>  .../media/atomisp/platform/clock/vlv2_plat_clock.c | 247 
> -
>  .../platform/intel-mid/atomisp_gmin_platform.c |  63 +-
>  7 files changed, 52 insertions(+), 333 deletions(-)
>  delete mode 100644
> drivers/staging/media/atomisp/platform/clock/Makefile
>  delete mode 100644
> drivers/staging/media/atomisp/platform/clock/platform_vlv2_plat_clk.c
>  delete mode 100644
> drivers/staging/media/atomisp/platform/clock/platform_vlv2_plat_clk.h
>  delete mode 100644
> drivers/staging/media/atomisp/platform/clock/vlv2_plat_clock.c
> 
> diff --git a/drivers/staging/media/atomisp/Kconfig
> b/drivers/staging/media/atomisp/Kconfig
> index 8eb13c3ba29c..7cdebea35ccf 100644
> --- a/drivers/staging/media/atomisp/Kconfig
> +++ b/drivers/staging/media/atomisp/Kconfig
> @@ -1,6 +1,7 @@
>  menuconfig INTEL_ATOMISP
>  bool "Enable support to Intel MIPI camera drivers"
>  depends on X86 && EFI && MEDIA_CONTROLLER && PCI && ACPI
> + select COMMON_CLK
>  help
>Enable support for the Intel ISP2 camera interfaces and
> MIPI
>sensor drivers.
> diff --git a/drivers/staging/media/atomisp/platform/Makefile
> b/drivers/staging/media/atomisp/platform/Makefile
> index df157630bda9..0e3b7e1c81c6 100644
> --- a/drivers/staging/media/atomisp/platform/Makefile
> +++ b/drivers/staging/media/atomisp/platform/Makefile
> @@ -2,5 +2,4 @@
>  # Makefile for camera drivers.
>  #
>  
> -obj-$(CONFIG_INTEL_ATOMISP) += clock/
>  obj-$(CONFIG_INTEL_ATOMISP) += intel-mid/
> diff --git a/drivers/staging/media/atomisp/platform/clock/Makefile
> b/drivers/staging/media/atomisp/platform/clock/Makefile
> deleted file mode 100644
> index 82fbe8b6968a..
> diff --git
> a/drivers/staging/media/atomisp/platform/clock/platform_vlv2_plat_clk.
> c
> b/drivers/staging/media/atomisp/platform/clock/platform_vlv2_plat_clk.
> c
> deleted file mode 100644
> index 0aae9b0283bb..
> diff --git
> a/drivers/staging/media/atomisp/platform/clock/platform_vlv2_plat_clk.
> h
> b/drivers/staging/media/atomisp/platform/clock/platform_vlv2_plat_clk.
> h
> deleted file mode 100644
> index b730ab0e8223..
> diff --git
> a/drivers/staging/media/atomisp/platform/clock/vlv2_plat_clock.c
> b/drivers/staging/media/atomisp/platform/clock/vlv2_plat_clock.c
> deleted file mode 100644
> index f96789a31819..
> diff --git a/drivers/staging/media/atomisp/platform/intel-
> mid/atomisp_gmin_platform.c
> b/drivers/staging/media/atomisp/platform/intel-
> mid/atomisp_gmin_platform.c
> index edaae93af8f9..17b4cfae5abf 100644
> --- a/drivers/staging/media/atomisp/platform/intel-
> mid/atomisp_gmin_platform.c
> +++ b/drivers/staging/media/atomisp/platform/intel-
> mid/atomisp_gmin_platform.c
> @@ -4,10 +4,10 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
> -#include "../../include/linux/vlv2_plat_clock.h"
>  #include 
>  #include 
>  #include 
> @@ -17,11 +17,7 @@
>  
>  #define MAX_SUBDEVS 8
>  
> -/* Should be defined in vlv2_plat_clock API, isn't: */
> -#define VLV2_CLK_PLL_19P2MHZ 1
> -#define VLV2_CLK_XTAL_19P2MHZ 0
> -#define VLV2_CLK_ON  1
> -#define VLV2_CLK_OFF 2
> +#define VLV2_CLK_PLL_19P2MHZ 1 /* XTAL on CHT */
>  #define ELDO1_SEL_REG0x19
>  #define ELDO1_1P8V   0x16
>  #define ELDO1_CTRL_SHIFT 0x00
> @@ -33,6 +29,7 @@ struct gmin_subdev {
>   struct v4l2_subdev *subdev;
>   int clock_num;
>   int clock_src;
> + struct clk *pmc_clk;
>   struct gp

Re: [PATCH] [media] staging: atomisp: use clock framework for camera clocks

[Andy Shevchenko]

On Wed, 2017-09-20 at 12:01 -0500, Pierre-Louis Bossart wrote:
> 
> On 09/20/2017 04:12 AM, Andy Shevchenko wrote:
> > On Tue, 2017-09-19 at 15:45 -0500, Pierre-Louis Bossart wrote:
> > > The Atom ISP driver initializes and configures PMC clocks which
> > > are
> > > already handled by the clock framework.
> > > 
> > > Remove all legacy vlv2_platform_clock stuff and move to the clk
> > > API to
> > > avoid conflicts, e.g. with audio machine drivers enabling the MCLK
> > > for
> > > external codecs
> > > 
> > 
> > I think it might have a Fixes: tag as well (though I dunno which
> > commit
> > could be considered as anchor).
> 
> The initial integration of the atomisp driver already had this
> problem, 
> i'll add a reference to
> 'a49d25364dfb9 ("staging/atomisp: Add support for the Intel IPU v2")'

...which seems to be the best choice (you can check how many new commits
use that one as an origin for Fixes: tag).

> > 
> > (I doubt Git is so clever to remove files based on information out
> > of
> > the diff, can you check it and if needed to resend without -D
> > implied?)
> 
> Gee, I thought -C -M -D were the standard options to checkpatch,
> never 
> realized it'd prevent patches from applying. Thanks for the tip.

-C -M — yes for sure.

Last time I checked patches, generated with help of -D, do not remove
the files when you do git am. So, I don't know if it still the case.
Safe option is to use -C -M for public (+ -D locally only to see less
noise).

-- 
Andy Shevchenko 
Intel Finland Oy
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2] staging: atomisp: add a driver for ov5648 camera sensor

[Andy Shevchenko]

IO_MISMATCH 900
> +static int distance(struct ov5648_resolution *res, u32 w, u32 h)
> +{
> + unsigned int w_ratio = ((res->width << 13) / w);

Redundant parens.

> + unsigned int h_ratio;
> + int match;
> +
> + if (h == 0)
> + return -1;
> + h_ratio = ((res->height << 13) / h);

Ditto.

> + if (h_ratio == 0)
> + return -1;
> + match   = abs(((w_ratio << 13) / h_ratio) - ((int)8192));

Ditto.

> +
> + if ((w_ratio < (int)8192) || (h_ratio < (int)8192)  ||
> + (match > LARGEST_ALLOWED_RATIO_MISMATCH))
> + return -1;

Why (int) casting here and above?!

> +
> + return w_ratio + h_ratio;
> +}

> +
> +/* Return the nearest higher resolution index */
> +static int nearest_resolution_index(int w, int h)
> +{
> + int i;
> + int idx = -1;
> + int dist;
> + int min_dist = INT_MAX;
> + struct ov5648_resolution *tmp_res = NULL;

Reversed xmas tree order.

> +
> + for (i = 0; i < N_RES; i++) {
> + tmp_res = &ov5648_res[i];
> + dist = distance(tmp_res, w, h);
> + if (dist == -1)
> + continue;
> + if (dist < min_dist) {
> + min_dist = dist;
> + idx = i;
> + }
> + }
> +
> + return idx;
> +}
> 

> + id = u16) high) << 8) | (u16) low);

Redundant parens. Please, check all lines to avoid such.

> +
> 

> + revision = (u8) high & 0x0f;
> +
> + dev_dbg(&client->dev, "sensor_revision = 0x%x\n", revision);
> + dev_dbg(&client->dev, "detect ov5648 success\n");

It would be one line, moreover, instead of above, you may use

...("...%c...",  hex_asc_lo[high]);


> + return 0;
> +}

> +
> +static int ov5648_s_stream(struct v4l2_subdev *sd, int enable)
> +{
> + struct ov5648_device *dev = to_ov5648_sensor(sd);
> + struct i2c_client *client = v4l2_get_subdevdata(sd);
> + int ret;

+ empty line.
Please check all functions.

> + dev_dbg(&client->dev, "@%s:\n", __func__);

Noise. Remove.
Do this for all similar cases.

> + mutex_lock(&dev->input_lock);
> +
> + ret = ov5648_write_reg(client, OV5648_8BIT, OV5648_SW_STREAM,
> + enable ? OV5648_START_STREAMING :
> + OV5648_STOP_STREAMING);
> +
> + mutex_unlock(&dev->input_lock);
> +
> + return ret;
> +}

> +static int ov5648_remove(struct i2c_client *client)
> +{
> + struct v4l2_subdev *sd = i2c_get_clientdata(client);
> + struct ov5648_device *dev = to_ov5648_sensor(sd);

> + dev_dbg(&client->dev, "ov5648_remove...\n");

Noise, remove.

> +
> + dev->platform_data->csi_cfg(sd, 0);
> +
> + v4l2_device_unregister_subdev(sd);
> + media_entity_cleanup(&dev->sd.entity);
> + v4l2_ctrl_handler_free(&dev->ctrl_handler);
> + kfree(dev);
> +
> + return 0;
> +}

> +
> +static int ov5648_probe(struct i2c_client *client,
> + const struct i2c_device_id *id)
> +{
> + struct ov5648_device *dev;
> + size_t len = CAMERA_MODULE_ID_LEN * sizeof(char);
> + int ret;
> + void *pdata;
> + unsigned int i;
> +
> + dev = kzalloc(sizeof(*dev), GFP_KERNEL);
> + if (!dev) {
> + dev_err(&client->dev, "out of memory\n");
> + return -ENOMEM;
> + }
> +
> + dev->camera_module = kzalloc(len, GFP_KERNEL);
> + if (!dev->camera_module) {
> + kfree(dev);
> + dev_err(&client->dev, "out of memory\n");
> + return -ENOMEM;
> + }
> +
> + mutex_init(&dev->input_lock);
> +
> + dev->fmt_idx = 0;

> + //otp functions

Wrong style, noisy comment. Remove.

> + dev->current_otp.otp_en = 1;// enable otp functions

Ditto.

> + v4l2_i2c_subdev_init(&(dev->sd), client, &ov5648_ops);
> +
> + if (gmin_get_config_var(&client->dev, "CameraModule",
> + dev->camera_module, &len)) {
> + kfree(dev->camera_module);
> + dev->camera_module = NULL;
> + dev_info(&client->dev, "Camera module id is
> missing\n");
> + }
> +
> + if (ACPI_COMPANION(&client->dev))
> + pdata = gmin_camera_platform_data(&dev->sd,
> +   ATOMISP_INPUT_FORMA
> T_RAW_10,
> +   atomisp_bayer_order
> _bggr);
> + else

> + pdata = client->dev.platform_data;

What kind of platforms will use platform_data?

> +out_free:
> + v4l2_device_unregister_subdev(&dev->sd);

Doesn't v4l2 have devm_*() helpers?

> + kfree(dev->camera_module);
> + kfree(dev);

Shouldn't those be devm_kzalloc() ?

> + return ret;
> +}

> +
> +static const struct acpi_device_id ov5648_acpi_match[] = {

> + {"XXOV5648"},

WTF is that?

> + {"INT5648"},
> + {},
> +};
> +MODULE_DEVICE_TABLE(acpi, ov5648_acpi_match);
> +

> +MODULE_DEVICE_TABLE(i2c, ov5648_id);

Where is the table?

> +static struct i2c_driver ov5648_driver = {
> + .driver = {

> + .owner = THIS_MODULE,

Redundant.

> + .name = OV5648_NAME,
> + .acpi_match_table = ACPI_PTR(ov5648_acpi_match),
> + },
> + .probe = ov5648_probe,
> + .remove = ov5648_remove,
> + .id_table = ov5648_id,
> +};
> +

> +static int init_ov5648(void)
> +{
> + return i2c_add_driver(&ov5648_driver);
> +}
> +
> +static void exit_ov5648(void)
> +{
> +
> + i2c_del_driver(&ov5648_driver);
> +}
> +
> +module_init(init_ov5648);
> +module_exit(exit_ov5648);

module_i2c_driver();

> +#ifndef __OV5648_H__
> +#define __OV5648_H__

+ empty line.

> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include  "../include/linux/atomisp_platform.h"

Why? Are they all needed for definitions below?
I'm pretty sure you may leave only couple out of them.

> +
> +#define OV5648_NAME  "ov5648"

Why it's here?

> +static const struct i2c_device_id ov5648_id[] = {
> + {OV5648_NAME, 0},
> + {}
> +};

WTF?! It shouldn't be in the header!

-- 
Andy Shevchenko 
Intel Finland Oy
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2] staging: atomisp: add a driver for ov5648 camera sensor

[Andy Shevchenko]

On Sun, Sep 24, 2017 at 5:59 PM, Devid Antonio Floni
 wrote:
> The ov5648 5-megapixel camera sensor from OmniVision supports up to 2592x1944
> resolution and MIPI CSI-2 interface. Output format is raw sRGB/Bayer with
> 10 bits per colour (SGRBG10_1X10).
>
> This patch is a port of ov5648 driver after applying following
> 01org/ProductionKernelQuilts patches:
>  - 0004-ov2680-ov5648-Fork-lift-source-from-CTS.patch
>  - 0005-ov2680-ov5648-gminification.patch
>  - 0006-ov5648-Focus-support.patch
>  - 0007-Fix-resolution-issues-on-rear-camera.patch
>  - 0008-ov2680-ov5648-Enabled-the-set_exposure-functions.patch
>  - 0010-IRDA-cameras-mode-list-cleanup-unification.patch
>  - 0012-ov5648-Add-1296x972-binned-mode.patch
>  - 0014-ov5648-Adapt-to-Atomisp2-Gmin-VCM-framework.patch
>  - 0015-dw9714-Gmin-Atomisp-specific-VCM-driver.patch
>  - 0017-ov5648-Fix-deadlock-on-I2C-error.patch
>  - 0018-gc2155-Fix-deadlock-on-error.patch
>  - 0019-ov5648-Add-1280x960-binned-mode.patch
>  - 0020-ov5648-Make-1280x960-as-default-video-resolution.patch
>  - 0021-MALATA-Fix-testCameraToSurfaceTextureMetadata-CTS.patch
>  - 0023-OV5648-Added-5MP-video-resolution.patch
>
> New changes introduced during the port:
>  - Rename entity types to entity functions
>  - Replace v4l2_subdev_fh by v4l2_subdev_pad_config
>  - Make use of media_bus_format enum
>  - Rename media_entity_init function to media_entity_pads_init
>  - Replace try_mbus_fmt by set_fmt
>  - Replace s_mbus_fmt by set_fmt
>  - Replace g_mbus_fmt by get_fmt
>  - Use s_ctrl/g_volatile_ctrl instead of ctrl core ops
>  - Update gmin platform API path
>  - Constify acpi_device_id
>  - Add "INT5648" value to acpi_device_id
>  - Fix some checkpatch errors and warnings
>  - Remove FSF's mailing address from the sample GPL notice
>
> Changes in v2:
>  - Fix indentation
>  - Add atomisp prefix to Kconfig option

I discussed with Sakari about my review and we agreed that two things
that I have mentioned (converting to smbus calls and regulator
framework) would be a material for future changes.
Other than that, please, address the rest of comments and we will be fine.

You may also refer to my last patch series WRT atomisp driver where I
tried to address my own comments to the rest of the code.

-- 
With Best Regards,
Andy Shevchenko
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v3] staging: atomisp: add a driver for ov5648 camera sensor

[Andy Shevchenko]

On Mon, 2017-10-02 at 21:30 +0200, Devid Antonio Filoni wrote:
> The ov5648 5-megapixel camera sensor from OmniVision supports up to
> 2592x1944
> resolution and MIPI CSI-2 interface. Output format is raw sRGB/Bayer
> with
> 10 bits per colour (SGRBG10_1X10).
> 
> This patch is a port of ov5648 driver after applying following
> 01org/ProductionKernelQuilts patches:
>  - 0004-ov2680-ov5648-Fork-lift-source-from-CTS.patch
>  - 0005-ov2680-ov5648-gminification.patch
>  - 0006-ov5648-Focus-support.patch
>  - 0007-Fix-resolution-issues-on-rear-camera.patch
>  - 0008-ov2680-ov5648-Enabled-the-set_exposure-functions.patch
>  - 0010-IRDA-cameras-mode-list-cleanup-unification.patch
>  - 0012-ov5648-Add-1296x972-binned-mode.patch
>  - 0014-ov5648-Adapt-to-Atomisp2-Gmin-VCM-framework.patch
>  - 0015-dw9714-Gmin-Atomisp-specific-VCM-driver.patch
>  - 0017-ov5648-Fix-deadlock-on-I2C-error.patch
>  - 0018-gc2155-Fix-deadlock-on-error.patch
>  - 0019-ov5648-Add-1280x960-binned-mode.patch
>  - 0020-ov5648-Make-1280x960-as-default-video-resolution.patch
>  - 0021-MALATA-Fix-testCameraToSurfaceTextureMetadata-CTS.patch
>  - 0023-OV5648-Added-5MP-video-resolution.patch
> 
> New changes introduced during the port:
>  - Rename entity types to entity functions
>  - Replace v4l2_subdev_fh by v4l2_subdev_pad_config
>  - Make use of media_bus_format enum
>  - Rename media_entity_init function to media_entity_pads_init
>  - Replace try_mbus_fmt by set_fmt
>  - Replace s_mbus_fmt by set_fmt
>  - Replace g_mbus_fmt by get_fmt
>  - Use s_ctrl/g_volatile_ctrl instead of ctrl core ops
>  - Update gmin platform API path
>  - Constify acpi_device_id
>  - Add "INT5648" value to acpi_device_id
>  - Fix some checkpatch errors and warnings
>  - Remove FSF's mailing address from the sample GPL notice
> 
> "INT5648" ACPI device id can be found in following production
> hardware:
> BIOS Information
> Vendor: LENOVO
> Version: 1HCN40WW
> Release Date: 11/04/2016
> ...
> BIOS Revision: 0.40
> Firmware Revision: 0.0
> ...
> System Information
> Manufacturer: LENOVO
> Product Name: 80SG
> Version: MIIX 310-10ICR
> ...
> SKU Number: LENOVO_MT_80SG_BU_idea_FM_MIIX 310-10ICR
> Family: IDEAPAD
> ...
> 
> Device DSDT excerpt:
> Device (CA00)
> {
> Name (_ADR, Zero)  // _ADR: Address
> Name (_HID, "INT5648")  // _HID: Hardware ID
> Name (_CID, "INT5648")  // _CID: Compatible ID
> Name (_SUB, "INTL")  // _SUB: Subsystem ID
> Name (_DDN, "ov5648")  // _DDN: DOS Device Name
> ...
> 
> I was not able to properly test this patch on my Lenovo Miix 310 due
> to other
> issues with atomisp,

Can you elaborate this a bit?

>  the output is the same as ov2680 driver (OVTI2680)
> which is very similar.
> 

Other than above the patch looks good enough for staging, though still
requires a lot of clean up work.

Reviewed-by: Andy Shevchenko 

> Signed-off-by: Devid Antonio Filoni 
> ---
> Changes in v2:
>  - Fix indentation
>  - Add atomisp prefix to Kconfig option
> 
> Changes in v3:
>  - Use module_i2c_driver() macro
>  - Switch i2c drivers to use ->probe_new()
>  - Remove unused ->gpio_ctrl() callback
>  - Remove unused ->power_ctrl() callback
>  - Remove unneeded header inclusions
>  - Sort header inclusions alphabetically
>  - Replace kzalloc with devm_kzalloc
>  - Remove "XXOV5648" acpi_device_id, we don't know if it's used in any
> production device
>  - Use reverse XMAS tree declarations
>  - Fix comments style
>  - Remove __func__ from all dev_{dbg,info,err}() calls
>  - Add missing new line chars in all dev_{dbg,info,err}() calls
>  - Remove useless dev_{dbg,err}() calls
>  - Fix all checkpatch.pl issues
> 
>  drivers/staging/media/atomisp/i2c/Kconfig  |   12 +
>  drivers/staging/media/atomisp/i2c/Makefile |1 +
>  drivers/staging/media/atomisp/i2c/atomisp-ov5648.c | 1787
> 
>  drivers/staging/media/atomisp/i2c/ov5648.h |  828 +
>  4 files changed, 2628 insertions(+)
>  create mode 100644 drivers/staging/media/atomisp/i2c/atomisp-ov5648.c
>  create mode 100644 drivers/staging/media/atomisp/i2c/ov5648.h
> 
> diff --git a/drivers/staging/media/atomisp/i2c/Kconfig
> b/drivers/staging/media/atomisp/i2c/Kconfig
> index a76f17d..6bd849d 100644
> --- a/drivers/staging/media/atomisp/i2c/Kconfig
> +++ b/drivers/staging/media/atomisp/i2c/Kconfig
> @@ -83,6 +83,18 @@ config VIDEO_ATOMISP_OV2680
>  
>It currently only work

Re: [PATCH 7/8] [media] staging: atomisp: convert timestamps to ktime_t

[Andy Shevchenko]

On Mon, 2017-11-27 at 14:19 +0100, Arnd Bergmann wrote:
> timespec overflows in 2038 on 32-bit architectures, and the
> getnstimeofday() suffers from possible time jumps, so the
> timestamps here are better done using ktime_get(), which has
> neither of those problems.
> 
> In case of ov2680, we don't seem to use the timestamp at
> all, so I just remove it.
> 

> + ktime_t timedelay = ns_to_ktime(
>   min((u32)abs(dev->number_of_steps) *
> DELAY_PER_STEP_NS,
> - (u32)DELAY_MAX_PER_STEP_NS),
> - };
> + (u32)DELAY_MAX_PER_STEP_NS));

Since you are touching this, it might make sense to convert to

min_t(u32, ...)

...and locate lines something like:

ktime_t timeday = ns_to_ktime(min_t(u32,
 param1,
 param2));

>From my pov will make readability better.

-- 
Andy Shevchenko 
Intel Finland Oy
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2] [media] staging: atomisp: convert timestamps to ktime_t

[Andy Shevchenko]

On Mon, 2017-11-27 at 16:21 +0100, Arnd Bergmann wrote:
> timespec overflows in 2038 on 32-bit architectures, and the
> getnstimeofday() suffers from possible time jumps, so the
> timestamps here are better done using ktime_get(), which has
> neither of those problems.
> 
> In case of ov2680, we don't seem to use the timestamp at
> all, so I just remove it.
> 

Yep,

Reviewed-by: Andy Shevchenko 

> Signed-off-by: Arnd Bergmann 
> ---
> v2: use min_t() as suggested by Andy Shevchenko
> ---
>  drivers/staging/media/atomisp/i2c/ov2680.h|  1 -
>  .../staging/media/atomisp/i2c/ov5693/atomisp-ov5693.c | 19 
> ---
>  drivers/staging/media/atomisp/i2c/ov5693/ov5693.h |  2 +-
>  3 files changed, 9 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/staging/media/atomisp/i2c/ov2680.h
> b/drivers/staging/media/atomisp/i2c/ov2680.h
> index bf4897347df7..03f75dd80f87 100644
> --- a/drivers/staging/media/atomisp/i2c/ov2680.h
> +++ b/drivers/staging/media/atomisp/i2c/ov2680.h
> @@ -174,7 +174,6 @@ struct ov2680_format {
>   struct mutex input_lock;
>   struct v4l2_ctrl_handler ctrl_handler;
>   struct camera_sensor_platform_data *platform_data;
> - struct timespec timestamp_t_focus_abs;
>   int vt_pix_clk_freq_mhz;
>   int fmt_idx;
>   int run_mode;
> diff --git a/drivers/staging/media/atomisp/i2c/ov5693/atomisp-ov5693.c 
> b/drivers/staging/media/atomisp/i2c/ov5693/atomisp-ov5693.c
> index 3e7c3851280f..9fa25bb8f1ee 100644
> --- a/drivers/staging/media/atomisp/i2c/ov5693/atomisp-ov5693.c
> +++ b/drivers/staging/media/atomisp/i2c/ov5693/atomisp-ov5693.c
> @@ -973,7 +973,7 @@ static int ov5693_t_focus_abs(struct v4l2_subdev
> *sd, s32 value)
>   if (ret == 0) {
>   dev->number_of_steps = value - dev->focus;
>   dev->focus = value;
> - getnstimeofday(&(dev->timestamp_t_focus_abs));
> + dev->timestamp_t_focus_abs = ktime_get();
>   } else
>   dev_err(&client->dev,
>   "%s: i2c failed. ret %d\n", __func__, ret);
> @@ -993,16 +993,13 @@ static int ov5693_q_focus_status(struct
> v4l2_subdev *sd, s32 *value)
>  {
>   u32 status = 0;
>   struct ov5693_device *dev = to_ov5693_sensor(sd);
> - struct timespec temptime;
> - const struct timespec timedelay = {
> - 0,
> - min((u32)abs(dev->number_of_steps) *
> DELAY_PER_STEP_NS,
> - (u32)DELAY_MAX_PER_STEP_NS),
> - };
> -
> - getnstimeofday(&temptime);
> - temptime = timespec_sub(temptime, (dev-
> >timestamp_t_focus_abs));
> - if (timespec_compare(&temptime, &timedelay) <= 0) {
> + ktime_t temptime;
> + ktime_t timedelay = ns_to_ktime(min_t(u32,
> + abs(dev->number_of_steps) *
> DELAY_PER_STEP_NS,
> + DELAY_MAX_PER_STEP_NS));
> +
> + temptime = ktime_sub(ktime_get(), (dev-
> >timestamp_t_focus_abs));
> + if (ktime_compare(temptime, timedelay) <= 0) {
>   status |= ATOMISP_FOCUS_STATUS_MOVING;
>   status |= ATOMISP_FOCUS_HP_IN_PROGRESS;
>   } else {
> diff --git a/drivers/staging/media/atomisp/i2c/ov5693/ov5693.h
> b/drivers/staging/media/atomisp/i2c/ov5693/ov5693.h
> index 2ea63807c56d..68cfcb4a6c3c 100644
> --- a/drivers/staging/media/atomisp/i2c/ov5693/ov5693.h
> +++ b/drivers/staging/media/atomisp/i2c/ov5693/ov5693.h
> @@ -221,7 +221,7 @@ struct ov5693_device {
>   struct v4l2_ctrl_handler ctrl_handler;
>  
>   struct camera_sensor_platform_data *platform_data;
> - struct timespec timestamp_t_focus_abs;
> + ktime_t timestamp_t_focus_abs;
>   int vt_pix_clk_freq_mhz;
>   int fmt_idx;
>   int run_mode;

-- 
Andy Shevchenko 
Intel Finland Oy
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v3 3/8] staging: rtl8192u: User memset to initialize memory, instead of loop.

[Andy Shevchenko]

On Sun, Jun 24, 2018 at 6:34 PM, John Whitmore  wrote:
> Replaced memory initialising loop with memset, as suggested by Andy Shevchenko
>

Suggested-by ?

> Signed-off-by: John Whitmore 
> ---
>  drivers/staging/rtl8192u/ieee80211/rtl819x_HTProc.c | 5 +
>  1 file changed, 1 insertion(+), 4 deletions(-)
>
> diff --git a/drivers/staging/rtl8192u/ieee80211/rtl819x_HTProc.c 
> b/drivers/staging/rtl8192u/ieee80211/rtl819x_HTProc.c
> index 4bfadb49c363..ccb7bdf5ad5d 100644
> --- a/drivers/staging/rtl8192u/ieee80211/rtl819x_HTProc.c
> +++ b/drivers/staging/rtl8192u/ieee80211/rtl819x_HTProc.c
> @@ -742,8 +742,6 @@ void HTConstructRT2RTAggElement(struct ieee80211_device 
> *ieee, u8 *posRT2RTAgg,
>   */
>  static u8 HT_PickMCSRate(struct ieee80211_device *ieee, u8 *pOperateMCS)
>  {
> -   u8  i;
> -
> if (!pOperateMCS) {
> IEEE80211_DEBUG(IEEE80211_DL_ERR, "pOperateMCS can't be null 
> in HT_PickMCSRate()\n");
> return false;
> @@ -756,8 +754,7 @@ static u8 HT_PickMCSRate(struct ieee80211_device *ieee, 
> u8 *pOperateMCS)
> //legacy rate routine handled at selectedrate
>
> //no MCS rate
> -   for (i = 0; i <= 15; i++)
> -   pOperateMCS[i] = 0;
> +   memset(pOperateMCS, 0, 16);
>     break;
>
> case IEEE_N_24G://assume CCK rate ok
> --
> 2.17.1
>



-- 
With Best Regards,
Andy Shevchenko
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: rtl8723bs: fix brace coding style issues

[Andy Shevchenko]

On Fri, Jun 22, 2018 at 1:28 PM, Dan Carpenter  wrote:
> On Thu, Jun 21, 2018 at 08:21:55PM +0200, Michael Straube wrote:
>> Remove braces from single line if statements.
>> Also fix a comparsion to NULL in one of the conditions.
>> Issues found by checkpatch.
>>
>> Signed-off-by: Michael Straube 
>> ---
>>  drivers/staging/rtl8723bs/core/rtw_debug.c | 6 ++
>>  1 file changed, 2 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/staging/rtl8723bs/core/rtw_debug.c 
>> b/drivers/staging/rtl8723bs/core/rtw_debug.c
>> index f852fde47350..2244ed72ab9c 100644
>> --- a/drivers/staging/rtl8723bs/core/rtw_debug.c
>> +++ b/drivers/staging/rtl8723bs/core/rtw_debug.c
>> @@ -618,9 +618,8 @@ ssize_t proc_set_wait_hiq_empty(struct file *file, const 
>> char __user *buffer, si
>>   if (count < 1)
>>   return -EFAULT;
>>
>> - if (buffer && !copy_from_user(tmp, buffer, sizeof(tmp))) {
>> + if (buffer && !copy_from_user(tmp, buffer, sizeof(tmp)))
>>   sscanf(tmp, "%u", &g_wait_hiq_empty);
>> - }
>
>
> The original code is kind of bad.  The NULL check isn't required.
> The sscanf call should have error checking.  The error code is wrong if
> the copy from user fails.  The tmp buffer isn't NUL terminated.
>
> if (copy_from_user(tmp, buffer, sizeof(tmp)))
> return -EFAULT;
> tmp[sizeof(tmp) - 1] = '\0';
>
> if (sscanf(tmp, "%u", &g_wait_hiq_empty) != 1)
> return -EINVAL;
>
> return count;

Shouldn't this be

kstrtouint_from_user()

instead of all those lines?

-- 
With Best Regards,
Andy Shevchenko
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: rtl8723bs: do not use assignment in if condition

[Andy Shevchenko]

On Fri, Jun 22, 2018 at 8:28 PM, Joe Perches  wrote:
> On Fri, 2018-06-22 at 14:48 +0200, Michael Straube wrote:
>> On 06/22/18 12:57, Dan Carpenter wrote:

> Output from checkpatch is not gospel and can be ignored
> whenever appropriate.
>
> I think the below is ok:
>
> if ((is_broadcast_ether_addr(mac) || is_zero_ether_addr(mac)) &&
> ((addr = of_get_property(np, "local-mac-address", &len)) &&
>  len == ETH_ALEN))
> memcpy(mac_addr, addr, ETH_ALEN);
> else
> memcpy(mac_addr, ""\x00\xe0\x4c\x87\x00\x00", ETH_ALEN);
>
> Although the last memcpy of a fixed mac address could
> probably use eth_random_addr to reduce the likelihood
> of mac address collision

...and first one looks like ether_addr_copy().

> so maybe
>
> if ((is_broadcast_ether_addr(mac) || is_zero_ether_addr(mac)) &&
> ((addr = of_get_property(np, "local-mac-address", &len)) &&
>
>  len == ETH_ALEN))
> memcpy(mac_addr, addr, ETH_ALEN);
> else
> eth_random_addr(mac_addr);
>
>> If yes, I'm not sure how to proceed as these are the very first patches I 
>> send.
>> Should I send a v2 patch with both changes or just a v2 with "np" removed and
>> another one for adding 'is_broadcast_ether_addr' and 'is_zero_ether_addr' 
>> checks?



-- 
With Best Regards,
Andy Shevchenko
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: rtl8723bs: refactor rtw_macaddr_cfg()

[Andy Shevchenko]

On Sat, Jun 23, 2018 at 12:45 PM, Michael Straube
 wrote:
> Using is_broadcast_ether_addr() and is_zero_ether_addr() instead of
> testing each byte of the mac[] array for 0xff and 0x00 shortens the
> code and improves readability.
>
> If np == NULL, of_get_property() returns NULL, hence the "np" check
> is not needed.
>
> Instead of a fixed default mac address use a random one to reduce the
> likelihood of mac address collision.
>
> Thanks to Joe Perches and Dan Carpenter.

> +   if ((addr = of_get_property(np, "local-mac-address", &len)) &&
> len == ETH_ALEN) {
> memcpy(mac_addr, addr, ETH_ALEN);

ether_addr_copy()?

> } else {

> +   eth_random_addr(mac_addr);
> +   DBG_871X("MAC Address from efuse error, assign random 
> one !!!\n");
> }

-- 
With Best Regards,
Andy Shevchenko
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v3 2/4] staging: rtl8723bs: refactor rtw_macaddr_cfg()

[Andy Shevchenko]

On Tue, Jun 26, 2018 at 11:14 AM, Michael Straube
 wrote:
> Using is_broadcast_ether_addr() and is_zero_ether_addr() instead of
> testing each byte of the mac[] array for 0xff and 0x00 shortens the
> code and improves readability.
>
> If np == NULL, of_get_property() returns NULL, hence the "np" check
> is not needed.
>
> Instead of a fixed default mac address use a random one to reduce the
> likelihood of mac address collision.
>

> Thanks to Joe Perches and Dan Carpenter.

I guess you may use Suggested-by tag as well.

>
> Signed-off-by: Michael Straube 
> ---
>  .../staging/rtl8723bs/core/rtw_ieee80211.c| 19 ---
>  1 file changed, 4 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/staging/rtl8723bs/core/rtw_ieee80211.c 
> b/drivers/staging/rtl8723bs/core/rtw_ieee80211.c
> index e55895632921..7aa00d1391f7 100644
> --- a/drivers/staging/rtl8723bs/core/rtw_ieee80211.c
> +++ b/drivers/staging/rtl8723bs/core/rtw_ieee80211.c
> @@ -1177,24 +1177,13 @@ void rtw_macaddr_cfg(struct device *dev, u8 *mac_addr)
> memcpy(mac, mac_addr, ETH_ALEN);
> }
>
> -   if (((mac[0] == 0xff) && (mac[1] == 0xff) && (mac[2] == 0xff) &&
> -(mac[3] == 0xff) && (mac[4] == 0xff) && (mac[5] == 0xff)) ||
> -   ((mac[0] == 0x00) && (mac[1] == 0x00) && (mac[2] == 0x00) &&
> -(mac[3] == 0x00) && (mac[4] == 0x00) && (mac[5] == 0x00))) {
> -   if (np &&
> -   (addr = of_get_property(np, "local-mac-address", &len)) &&
> +   if (is_broadcast_ether_addr(mac) || is_zero_ether_addr(mac)) {
> +   if ((addr = of_get_property(np, "local-mac-address", &len)) &&
> len == ETH_ALEN) {
> memcpy(mac_addr, addr, ETH_ALEN);
> } else {
> -   mac[0] = 0x00;
> -   mac[1] = 0xe0;
> -   mac[2] = 0x4c;
> -   mac[3] = 0x87;
> -   mac[4] = 0x00;
> -   mac[5] = 0x00;
> -   /*  use default mac addresss */
> -   memcpy(mac_addr, mac, ETH_ALEN);
> -   DBG_871X("MAC Address from efuse error, assign 
> default one !!!\n");
> +   eth_random_addr(mac_addr);
> +   DBG_871X("MAC Address from efuse error, assign random 
> one !!!\n");
> }
> }
>
> --
> 2.18.0
>



-- 
With Best Regards,
Andy Shevchenko
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v3 3/4] staging: rtl8723bs: use ether_addr_copy() in rtw_macaddr_cfg()

[Andy Shevchenko]

On Tue, Jun 26, 2018 at 11:14 AM, Michael Straube
 wrote:
> Use ether_addr_copy() instead of memcpy() to copy the mac address.
>

Suggested-by ?

Btw, ensure that the source and destination buffers are aligned to u16
as required by API.

(It applies to all changes to ether_*addr_*() replacements)

> Signed-off-by: Michael Straube 
> ---
>  drivers/staging/rtl8723bs/core/rtw_ieee80211.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/staging/rtl8723bs/core/rtw_ieee80211.c 
> b/drivers/staging/rtl8723bs/core/rtw_ieee80211.c
> index 7aa00d1391f7..8af4a89e632f 100644
> --- a/drivers/staging/rtl8723bs/core/rtw_ieee80211.c
> +++ b/drivers/staging/rtl8723bs/core/rtw_ieee80211.c
> @@ -1172,15 +1172,15 @@ void rtw_macaddr_cfg(struct device *dev, u8 *mac_addr)
> for (jj = 0, kk = 0; jj < ETH_ALEN; jj++, kk += 3) {
> mac[jj] = key_2char2num(rtw_initmac[kk], 
> rtw_initmac[kk + 1]);
> }
> -   memcpy(mac_addr, mac, ETH_ALEN);
> +   ether_addr_copy(mac_addr, mac);
> } else{ /*  Use the mac address stored in the Efuse */
> -   memcpy(mac, mac_addr, ETH_ALEN);
> +   ether_addr_copy(mac, mac_addr);
> }
>
> if (is_broadcast_ether_addr(mac) || is_zero_ether_addr(mac)) {
> if ((addr = of_get_property(np, "local-mac-address", &len)) &&
> len == ETH_ALEN) {
> -   memcpy(mac_addr, addr, ETH_ALEN);
> +   ether_addr_copy(mac_addr, addr);
> } else {
> eth_random_addr(mac_addr);
> DBG_871X("MAC Address from efuse error, assign random 
> one !!!\n");
> --
> 2.18.0
>



-- 
With Best Regards,
Andy Shevchenko
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v3 4/4] staging: rtl8723bs: use mac_pton() in rtw_macaddr_cfg()

[Andy Shevchenko]

On Tue, Jun 26, 2018 at 11:14 AM, Michael Straube
 wrote:
> Use the mac_pton() helper to convert the mac address string.
>
> The functions key_char2num() and key_2char2num() are not used
> anywhere else and can be removed.
>
> This also has the benefit of validating the input since mac_pton()
> returns false if the string is not valid.
>

FWIW,
Reviewed-by: Andy Shevchenko 

> Signed-off-by: Michael Straube 
> ---
>  .../staging/rtl8723bs/core/rtw_ieee80211.c| 30 +++
>  .../staging/rtl8723bs/os_dep/ioctl_linux.c|  3 --
>  2 files changed, 4 insertions(+), 29 deletions(-)
>
> diff --git a/drivers/staging/rtl8723bs/core/rtw_ieee80211.c 
> b/drivers/staging/rtl8723bs/core/rtw_ieee80211.c
> index 8af4a89e632f..8e0025e1ff14 100644
> --- a/drivers/staging/rtl8723bs/core/rtw_ieee80211.c
> +++ b/drivers/staging/rtl8723bs/core/rtw_ieee80211.c
> @@ -1137,25 +1137,6 @@ ParseRes rtw_ieee802_11_parse_elems(u8 *start, uint 
> len,
>
>  }
>
> -static u8 key_char2num(u8 ch);
> -static u8 key_char2num(u8 ch)
> -{
> -   if ((ch >= '0') && (ch <= '9'))
> -   return ch - '0';
> -   else if ((ch >= 'a') && (ch <= 'f'))
> -   return ch - 'a' + 10;
> -   else if ((ch >= 'A') && (ch <= 'F'))
> -   return ch - 'A' + 10;
> -   else
> -   return 0xff;
> -}
> -
> -u8 key_2char2num(u8 hch, u8 lch);
> -u8 key_2char2num(u8 hch, u8 lch)
> -{
> -   return ((key_char2num(hch) << 4) | key_char2num(lch));
> -}
> -
>  void rtw_macaddr_cfg(struct device *dev, u8 *mac_addr)
>  {
> u8 mac[ETH_ALEN];
> @@ -1166,14 +1147,11 @@ void rtw_macaddr_cfg(struct device *dev, u8 *mac_addr)
> if (!mac_addr)
> return;
>
> -   if (rtw_initmac) {  /*  Users specify the mac address */
> -   int jj, kk;
> -
> -   for (jj = 0, kk = 0; jj < ETH_ALEN; jj++, kk += 3) {
> -   mac[jj] = key_2char2num(rtw_initmac[kk], 
> rtw_initmac[kk + 1]);
> -   }
> +   if (rtw_initmac && mac_pton(rtw_initmac, mac)) {
> +   /* Users specify the mac address */
> ether_addr_copy(mac_addr, mac);
> -   } else{ /*  Use the mac address stored in the Efuse */
> +   } else {
> +   /* Use the mac address stored in the Efuse */
> ether_addr_copy(mac, mac_addr);
> }
>
> diff --git a/drivers/staging/rtl8723bs/os_dep/ioctl_linux.c 
> b/drivers/staging/rtl8723bs/os_dep/ioctl_linux.c
> index 39502156f652..4eb51f45b387 100644
> --- a/drivers/staging/rtl8723bs/os_dep/ioctl_linux.c
> +++ b/drivers/staging/rtl8723bs/os_dep/ioctl_linux.c
> @@ -32,9 +32,6 @@
>  #define WEXT_CSCAN_HOME_DWELL_SECTION  'H'
>  #define WEXT_CSCAN_TYPE_SECTION'T'
>
> -
> -extern u8 key_2char2num(u8 hch, u8 lch);
> -
>  static u32 rtw_rates[] = {100, 200, 550, 1100,
> 600, 900, 1200, 1800, 2400, 3600, 4800, 
> 5400};
>
> --
> 2.18.0
>



-- 
With Best Regards,
Andy Shevchenko
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: rtl8723bs: fix brace coding style issues

[Andy Shevchenko]

On Mon, Jun 25, 2018 at 3:24 PM, Dan Carpenter  wrote:
> On Mon, Jun 25, 2018 at 12:47:44PM +0300, Andy Shevchenko wrote:
>> On Fri, Jun 22, 2018 at 1:28 PM, Dan Carpenter  
>> wrote:
>> > On Thu, Jun 21, 2018 at 08:21:55PM +0200, Michael Straube wrote:
>> >> Remove braces from single line if statements.
>> >> Also fix a comparsion to NULL in one of the conditions.
>> >> Issues found by checkpatch.

>> >> --- a/drivers/staging/rtl8723bs/core/rtw_debug.c
>> >> +++ b/drivers/staging/rtl8723bs/core/rtw_debug.c
>> >> @@ -618,9 +618,8 @@ ssize_t proc_set_wait_hiq_empty(struct file *file, 
>> >> const char __user *buffer, si
>> >>   if (count < 1)
>> >>   return -EFAULT;
>> >>
>> >> - if (buffer && !copy_from_user(tmp, buffer, sizeof(tmp))) {
>> >> + if (buffer && !copy_from_user(tmp, buffer, sizeof(tmp)))
>> >>   sscanf(tmp, "%u", &g_wait_hiq_empty);
>> >> - }
>> >
>> >
>> > The original code is kind of bad.  The NULL check isn't required.
>> > The sscanf call should have error checking.  The error code is wrong if
>> > the copy from user fails.  The tmp buffer isn't NUL terminated.
>> >
>> > if (copy_from_user(tmp, buffer, sizeof(tmp)))
>> > return -EFAULT;
>> > tmp[sizeof(tmp) - 1] = '\0';
>> >
>> > if (sscanf(tmp, "%u", &g_wait_hiq_empty) != 1)
>> > return -EINVAL;
>> >
>> > return count;
>>
>> Shouldn't this be
>>
>> kstrtouint_from_user()
>>
>> instead of all those lines?
>
> I wasn't sure kstrtoint() was the exact same as sscanf()...  If so then
> sure.

In this case it's very close to what sscanf() is doing here including
all user buffer handing.

-- 
With Best Regards,
Andy Shevchenko
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v3 3/4] staging: rtl8723bs: use ether_addr_copy() in rtw_macaddr_cfg()

[Andy Shevchenko]

On Wed, Jun 27, 2018 at 4:11 PM, Dan Carpenter  wrote:
> On Wed, Jun 27, 2018 at 02:56:00PM +0200, Michael Straube wrote:
>> Should I add a thanks line to the commit message:
>>
>> Thanks to Dan Carpenter, Joe Perches and Andy Shevchenko.
>>
>> Or would that be considered as too much?
>
> You can write whatever the heck you want...  :P  No one cares.
>
> When it comes to credit, I do appreciate Reported-by tags because LWN
> and employers do count those sometimes.

In some cases Suggested-by fits better.

-- 
With Best Regards,
Andy Shevchenko
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: rtl8188eu: remove rtw_ioctl_rtl.h

[Andy Shevchenko]

On Sat, Jun 30, 2018 at 5:59 PM, Michael Straube
 wrote:
> The header rtw_ioctl_rtl.h is not used anywhere.
> Running 'grep -r rtw_ioctl_rtl *' from kernel root
> directory returns nothing, remove the file.

Just a side note, using `git grep` is much more efficient against
kernel source tree.
Something like `git grep -n rtw_ioctl_rtl.h` in this case.

-- 
With Best Regards,
Andy Shevchenko
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v5 1/4] resource: Move reparent_resources() to kernel/resource.c and make it public

[Andy Shevchenko]

On Tue, Jul 3, 2018 at 5:55 PM, Baoquan He  wrote:
> On 06/12/18 at 05:24pm, Andy Shevchenko wrote:
>> On Tue, Jun 12, 2018 at 5:20 PM, Andy Shevchenko
>>  wrote:

>> > I briefly looked at the code and error codes we have, so, my proposal
>> > is one of the following
>>
>> >  - use -ECANCELED (not the best choice for first occurrence here,
>> > though I can't find better)
>>
>> Actually -ENOTSUPP might suit the first case (although the actual
>> would be something like -EOVERLAP, which we don't have)
>
> Sorry for late reply, and many thanks for your great suggestion.
>

> I am fine to use -ENOTSUPP as the first returned value, and -ECANCELED
> for the 2nd one.

I have no strong opinion, but I like (slightly better) this approach ^^^

> Or define an enum as you suggested inside the function
> or in header file.

>
> Or use -EBUSY for the first case because existing resource is
> overlapping but not fully contained by 'res'; and -EINVAL for
> the 2nd case since didn't find any one resources which is contained by
> 'res', means we passed in a invalid resource.
>
> All is fine to me, I can repost with each of them.

>> >  - use positive integers (or enum), like
>> >   #define RES_REPARENTED 0
>> >   #define RES_OVERLAPPED 1
>> >   #define RES_NOCONFLICT 2

-- 
With Best Regards,
Andy Shevchenko
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v6 1/4] resource: Move reparent_resources() to kernel/resource.c and make it public

[Andy Shevchenko]

On Wed, Jul 4, 2018 at 7:10 AM, Baoquan He  wrote:
> reparent_resources() is duplicated in arch/microblaze/pci/pci-common.c
> and arch/powerpc/kernel/pci-common.c, so move it to kernel/resource.c
> so that it's shared.

With couple of comments below,

Reviewed-by: Andy Shevchenko 

P.S. In some commit message in this series you used 'likt' instead of 'like'.

>
> Signed-off-by: Baoquan He 
> ---
>  arch/microblaze/pci/pci-common.c | 37 -
>  arch/powerpc/kernel/pci-common.c | 35 ---
>  include/linux/ioport.h   |  1 +
>  kernel/resource.c| 39 +++
>  4 files changed, 40 insertions(+), 72 deletions(-)
>
> diff --git a/arch/microblaze/pci/pci-common.c 
> b/arch/microblaze/pci/pci-common.c
> index f34346d56095..7899bafab064 100644
> --- a/arch/microblaze/pci/pci-common.c
> +++ b/arch/microblaze/pci/pci-common.c
> @@ -619,43 +619,6 @@ int pcibios_add_device(struct pci_dev *dev)
>  EXPORT_SYMBOL(pcibios_add_device);
>
>  /*
> - * Reparent resource children of pr that conflict with res
> - * under res, and make res replace those children.
> - */
> -static int __init reparent_resources(struct resource *parent,
> -struct resource *res)
> -{
> -   struct resource *p, **pp;
> -   struct resource **firstpp = NULL;
> -
> -   for (pp = &parent->child; (p = *pp) != NULL; pp = &p->sibling) {
> -   if (p->end < res->start)
> -   continue;
> -   if (res->end < p->start)
> -   break;
> -   if (p->start < res->start || p->end > res->end)
> -   return -1;  /* not completely contained */
> -   if (firstpp == NULL)
> -   firstpp = pp;
> -   }
> -   if (firstpp == NULL)
> -   return -1;  /* didn't find any conflicting entries? */
> -   res->parent = parent;
> -   res->child = *firstpp;
> -   res->sibling = *pp;
> -   *firstpp = res;
> -   *pp = NULL;
> -   for (p = res->child; p != NULL; p = p->sibling) {
> -   p->parent = res;
> -   pr_debug("PCI: Reparented %s [%llx..%llx] under %s\n",
> -p->name,
> -(unsigned long long)p->start,
> -(unsigned long long)p->end, res->name);
> -   }
> -   return 0;
> -}
> -
> -/*
>   *  Handle resources of PCI devices.  If the world were perfect, we could
>   *  just allocate all the resource regions and do nothing more.  It isn't.
>   *  On the other hand, we cannot just re-allocate all devices, as it would
> diff --git a/arch/powerpc/kernel/pci-common.c 
> b/arch/powerpc/kernel/pci-common.c
> index fe9733aa..926035bb378d 100644
> --- a/arch/powerpc/kernel/pci-common.c
> +++ b/arch/powerpc/kernel/pci-common.c
> @@ -1088,41 +1088,6 @@ resource_size_t pcibios_align_resource(void *data, 
> const struct resource *res,
>  EXPORT_SYMBOL(pcibios_align_resource);
>
>  /*
> - * Reparent resource children of pr that conflict with res
> - * under res, and make res replace those children.
> - */
> -static int reparent_resources(struct resource *parent,
> -struct resource *res)
> -{
> -   struct resource *p, **pp;
> -   struct resource **firstpp = NULL;
> -
> -   for (pp = &parent->child; (p = *pp) != NULL; pp = &p->sibling) {
> -   if (p->end < res->start)
> -   continue;
> -   if (res->end < p->start)
> -   break;
> -   if (p->start < res->start || p->end > res->end)
> -   return -1;  /* not completely contained */
> -   if (firstpp == NULL)
> -   firstpp = pp;
> -   }
> -   if (firstpp == NULL)
> -   return -1;  /* didn't find any conflicting entries? */
> -   res->parent = parent;
> -   res->child = *firstpp;
> -   res->sibling = *pp;
> -   *firstpp = res;
> -   *pp = NULL;
> -   for (p = res->child; p != NULL; p = p->sibling) {
> -   p->parent = res;
> -   pr_debug("PCI: Reparented %s %pR under %s\n",
> -p->name, p, res->name);
> -   }
> -   return 0;
> -}
> -
> -/*
>   *  Handle resources of PCI devices.  If the world were perfect, we could
&g

Re: [PATCH v6 2/4] resource: Use list_head to link sibling resource

[Andy Shevchenko]

On Sun, Jul 8, 2018 at 5:59 AM, Baoquan He  wrote:
> On 07/05/18 at 01:00am, kbuild test robot wrote:

> However, I didn't find below branch. And tried to open it in web
> broswer, also failed.

While this is kinda valid point...

> Could you help have a look at this?

...isn't obvious that you didn't change the file mentioned in a report?
Just take latest linux-next and you will see.


>> All error/warnings (new ones prefixed by >>):
>>
>> >> arch/mips/pci/pci-rc32434.c:57:11: error: initialization from 
>> >> incompatible pointer type [-Werror=incompatible-pointer-types]
>>  .child = &rc32434_res_pci_mem2
>>   ^
>>arch/mips/pci/pci-rc32434.c:57:11: note: (near initialization for 
>> 'rc32434_res_pci_mem1.child.next')
>> >> arch/mips/pci/pci-rc32434.c:51:47: warning: missing braces around 
>> >> initializer [-Wmissing-braces]
>> static struct resource rc32434_res_pci_mem1 = {
>>   ^
>>arch/mips/pci/pci-rc32434.c:60:47: warning: missing braces around 
>> initializer [-Wmissing-braces]
>> static struct resource rc32434_res_pci_mem2 = {
>>   ^
>>cc1: some warnings being treated as errors
>>
>> vim +57 arch/mips/pci/pci-rc32434.c
>>
>> 73b4390f Ralf Baechle 2008-07-16  50
>> 73b4390f Ralf Baechle 2008-07-16 @51  static struct resource 
>> rc32434_res_pci_mem1 = {
>> 73b4390f Ralf Baechle 2008-07-16  52  .name = "PCI MEM1",
>> 73b4390f Ralf Baechle 2008-07-16  53  .start = 0x5000,
>> 73b4390f Ralf Baechle 2008-07-16  54  .end = 0x5FFF,
>> 73b4390f Ralf Baechle 2008-07-16  55  .flags = IORESOURCE_MEM,
>> 73b4390f Ralf Baechle 2008-07-16  56  .sibling = NULL,
>> 73b4390f Ralf Baechle 2008-07-16 @57  .child = &rc32434_res_pci_mem2
>> 73b4390f Ralf Baechle 2008-07-16  58  };
>> 73b4390f Ralf Baechle 2008-07-16  59
>>
>> :: The code at line 57 was first introduced by commit
>> :: 73b4390fb23456964201abda79f1210fe337d01a [MIPS] Routerboard 532: 
>> Support for base system
>>
>> :: TO: Ralf Baechle 
>> :: CC: Ralf Baechle 
>>
>> ---
>> 0-DAY kernel test infrastructureOpen Source Technology Center
>> https://lists.01.org/pipermail/kbuild-all   Intel Corporation
>
>



-- 
With Best Regards,
Andy Shevchenko
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v7 1/4] resource: Move reparent_resources() to kernel/resource.c and make it public

[Andy Shevchenko]

On Wed, Jul 18, 2018 at 5:49 AM, Baoquan He  wrote:
> reparent_resources() is duplicated in arch/microblaze/pci/pci-common.c
> and arch/powerpc/kernel/pci-common.c, so move it to kernel/resource.c
> so that it's shared.

Some minor stuff.

> +/**
> + * reparent_resources - reparent resource children of parent that res covers
> + * @parent: parent resource descriptor
> + * @res: resource descriptor desired by caller
> + *
> + * Returns 0 on success, -ENOTSUPP if child resource is not completely
> + * contained by 'res', -ECANCELED if no any conflicting entry found.

'res' -> @res

> + *
> + * Reparent resource children of 'parent' that conflict with 'res'

Ditto + 'parent' -> @parent

> + * under 'res', and make 'res' replace those children.

Ditto.

> + */
> +int reparent_resources(struct resource *parent, struct resource *res)
> +{
> +   struct resource *p, **pp;
> +   struct resource **firstpp = NULL;
> +
> +   for (pp = &parent->child; (p = *pp) != NULL; pp = &p->sibling) {
> +   if (p->end < res->start)
> +   continue;
> +   if (res->end < p->start)
> +   break;
> +   if (p->start < res->start || p->end > res->end)
> +   return -ENOTSUPP;   /* not completely contained */
> +   if (firstpp == NULL)
> +   firstpp = pp;
> +   }
> +   if (firstpp == NULL)
> +   return -ECANCELED; /* didn't find any conflicting entries? */
> +   res->parent = parent;
> +   res->child = *firstpp;
> +   res->sibling = *pp;
> +   *firstpp = res;
> +   *pp = NULL;
> +   for (p = res->child; p != NULL; p = p->sibling) {
> +   p->parent = res;

> +   pr_debug("PCI: Reparented %s %pR under %s\n",
> +p->name, p, res->name);

Now, PCI is a bit confusing here.

> +   }
> +   return 0;
> +}
> +EXPORT_SYMBOL(reparent_resources);
> +
>  static void __init __reserve_region_with_split(struct resource *root,
> resource_size_t start, resource_size_t end,
> const char *name)
> --
> 2.13.6
>



-- 
With Best Regards,
Andy Shevchenko
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v7 1/4] resource: Move reparent_resources() to kernel/resource.c and make it public

[Andy Shevchenko]

On Wed, Jul 18, 2018 at 7:36 PM, Andy Shevchenko
 wrote:
> On Wed, Jul 18, 2018 at 5:49 AM, Baoquan He  wrote:
>> reparent_resources() is duplicated in arch/microblaze/pci/pci-common.c
>> and arch/powerpc/kernel/pci-common.c, so move it to kernel/resource.c
>> so that it's shared.

>> + * Returns 0 on success, -ENOTSUPP if child resource is not completely
>> + * contained by 'res', -ECANCELED if no any conflicting entry found.

You also can refer to constants by prefixing them with %, e.g. %-ENOTSUPP.
But this is up to you completely.

-- 
With Best Regards,
Andy Shevchenko
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: fbtft: Replace mdelay() with msleep() and usleep_range()

[Andy Shevchenko]

On Fri, Jul 27, 2018 at 12:21 PM, Jia-Ju Bai  wrote:
> reset() and init_display() are never called in atomic context.
> They call mdelay() to busily wait, which is not necessary.
> mdelay() can be replaced with msleep().

> gpio_set_value(par->gpio.reset, 0);
> udelay(20);
> gpio_set_value(par->gpio.reset, 1);
> -   mdelay(120);
> +   msleep(120);

I didn't look to the rest, but this one will be inconsistent after your patch.

The question here is why udelay() is needed, while mdelay() changed?

-- 
With Best Regards,
Andy Shevchenko
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v5] staging: rts5208: add error handling into rtsx_probe

[Andy Shevchenko]

On Wed, Aug 1, 2018 at 2:55 PM, Anton Vasilyev  wrote:
> If rtsx_probe() fails to allocate dev->chip, then release_everything()
> will crash on uninitialized dev->cmnd_ready complete.
>
> Patch adds an error handling into rtsx_probe.
> Found by Linux Driver Verification project (linuxtesting.org).

Have you based your change on staging-next?

Seems not. You need to rebase and resend.

-- 
With Best Regards,
Andy Shevchenko
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v5] staging: rts5208: add error handling into rtsx_probe

[Andy Shevchenko]

On Wed, Aug 1, 2018 at 5:08 PM, Anton Vasilyev  wrote:
> I found that staging-next already contains my patch v3, committed by Greg
> Kroah-Hartman.
>
> Do I need to send a new patch

Yes. Based on staging-next.

> with a label renaming based on Dan Carpenter
> comments?

Dan is talking for himself :-)

-- 
With Best Regards,
Andy Shevchenko
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [RFC PATCH v2 19/32] crypto: ccp: Introduce the AMD Secure Processor device

[Andy Shevchenko]

On Thu, 2017-03-02 at 13:11 -0600, Brijesh Singh wrote:
> Hi Mark,
> 
> On 03/02/2017 11:39 AM, Mark Rutland wrote:
> > On Thu, Mar 02, 2017 at 10:16:15AM -0500, Brijesh Singh wrote:

> > > 
> > > +ccp-$(CONFIG_CRYPTO_DEV_CCP) += ccp-dev.o \
> > >   ccp-ops.o \
> > >   ccp-dev-v3.o \
> > >   ccp-dev-v5.o \
> > > - ccp-platform.o \
> > >   ccp-dmaengine.o
> > 
> > It looks like ccp-platform.c has morphed into sp-platform.c (judging
> > by
> > the compatible string and general shape of the code), and the
> > original
> > ccp-platform.c is no longer built.
> > 
> > Shouldn't ccp-platform.c be deleted by this patch?
> > 
> 
> Good catch. Both ccp-platform.c and ccp-pci.c should have been
> deleted 
> by this patch. I missed deleting it, will fix in next rev.

Don't forget to use -M -C when preparing / sending patches.

-- 
Andy Shevchenko 
Intel Finland Oy
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: fbtft: Replace a bit shift by a use of BIT.

[Andy Shevchenko]

On Wed, Mar 22, 2017 at 4:37 AM, Arushi Singhal
 wrote:
> This patch replaces bit shifting on 1 with the BIT(x) macro.
> This was done with coccinelle:
> @@
> constant c;
> @@
>
> -1 << c
> +BIT(c)
>

While using BIT() macro is a good idea, you make it inconsistent here.
There are at least two options:
- do nothing
- define _MASK:s with GENMASK() and reuse in the code (looking at the
code they are masks, not just separate bits).


> Signed-off-by: Arushi Singhal 
> ---
>  drivers/staging/fbtft/fb_agm1264k-fl.c | 4 ++--
>  drivers/staging/fbtft/fb_ili9163.c | 2 +-
>  drivers/staging/fbtft/fb_ili9325.c | 2 +-
>  drivers/staging/fbtft/fb_ssd1289.c | 2 +-
>  4 files changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/staging/fbtft/fb_agm1264k-fl.c 
> b/drivers/staging/fbtft/fb_agm1264k-fl.c
> index 4ee76dbd30b5..d31deeab69b2 100644
> --- a/drivers/staging/fbtft/fb_agm1264k-fl.c
> +++ b/drivers/staging/fbtft/fb_agm1264k-fl.c
> @@ -369,7 +369,7 @@ static int write_vmem(struct fbtft_par *par, size_t 
> offset, size_t len)
> /* select left side (sc0)
>  * set addr
>  */
> -   write_reg(par, 0x00, (1 << 6) | (u8)addr_win.xs);
> +   write_reg(par, 0x00, (BIT(6)) | (u8)addr_win.xs);
> write_reg(par, 0x00, (0x17 << 3) | (u8)y);
>
> /* write bitmap */
> @@ -392,7 +392,7 @@ static int write_vmem(struct fbtft_par *par, size_t 
> offset, size_t len)
> /* select right side (sc1)
>  * set addr
>  */
> -   write_reg(par, 0x01, 1 << 6);
> +   write_reg(par, 0x01, BIT(6));
> write_reg(par, 0x01, (0x17 << 3) | (u8)y);
>
> /* write bitmap */
> diff --git a/drivers/staging/fbtft/fb_ili9163.c 
> b/drivers/staging/fbtft/fb_ili9163.c
> index 579e17734612..06a5a5f6e33e 100644
> --- a/drivers/staging/fbtft/fb_ili9163.c
> +++ b/drivers/staging/fbtft/fb_ili9163.c
> @@ -194,7 +194,7 @@ static int set_var(struct fbtft_par *par)
>
> /* Colorspcae */
> if (par->bgr)
> -   mactrl_data |= (1 << 2);
> +   mactrl_data |= (BIT(2));
> write_reg(par, MIPI_DCS_SET_ADDRESS_MODE, mactrl_data);
> write_reg(par, MIPI_DCS_WRITE_MEMORY_START);
> return 0;
> diff --git a/drivers/staging/fbtft/fb_ili9325.c 
> b/drivers/staging/fbtft/fb_ili9325.c
> index 7189de5ae4b3..ab1267a9cd11 100644
> --- a/drivers/staging/fbtft/fb_ili9325.c
> +++ b/drivers/staging/fbtft/fb_ili9325.c
> @@ -126,7 +126,7 @@ static int init_display(struct fbtft_par *par)
> write_reg(par, 0x0013, 0x); /* VDV[4:0] for VCOM amplitude */
> mdelay(200); /* Dis-charge capacitor power voltage */
> write_reg(par, 0x0010, /* SAP, BT[3:0], AP, DSTB, SLP, STB */
> -   (1 << 12) | (bt << 8) | (1 << 7) | (0x01 << 4));
> +   (BIT(12)) | (bt << 8) | (BIT(7)) | (BIT(4)));
> write_reg(par, 0x0011, 0x220 | vc); /* DC1[2:0], DC0[2:0], VC[2:0] */
> mdelay(50); /* Delay 50ms */
> write_reg(par, 0x0012, vrh); /* Internal reference voltage= Vci; */
> diff --git a/drivers/staging/fbtft/fb_ssd1289.c 
> b/drivers/staging/fbtft/fb_ssd1289.c
> index c603e1516e64..b22a07d79b34 100644
> --- a/drivers/staging/fbtft/fb_ssd1289.c
> +++ b/drivers/staging/fbtft/fb_ssd1289.c
> @@ -47,7 +47,7 @@ static int init_display(struct fbtft_par *par)
> write_reg(par, 0x0E, 0x2B00);
> write_reg(par, 0x1E, 0x00B7);
> write_reg(par, 0x01,
> -   (1 << 13) | (par->bgr << 11) | (1 << 9) | (HEIGHT - 1));
> +   (BIT(13)) | (par->bgr << 11) | (BIT(9)) | (HEIGHT - 1));
> write_reg(par, 0x02, 0x0600);
> write_reg(par, 0x10, 0x);
> write_reg(par, 0x05, 0x);
> --
> 2.11.0
>



-- 
With Best Regards,
Andy Shevchenko
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: fbtft: Replace a bit shift by a use of BIT.

[Andy Shevchenko]

On Wed, Mar 22, 2017 at 6:18 PM, Andy Shevchenko
 wrote:
> On Wed, Mar 22, 2017 at 4:37 AM, Arushi Singhal
>  wrote:
>> This patch replaces bit shifting on 1 with the BIT(x) macro.
>> This was done with coccinelle:
>> @@
>> constant c;
>> @@
>>
>> -1 << c
>> +BIT(c)
>>
>
> While using BIT() macro is a good idea, you make it inconsistent here.
> There are at least two options:
> - do nothing
> - define _MASK:s with GENMASK() and reuse in the code (looking at the
> code they are masks, not just separate bits).

Looking a bit more, not masks, but values.
So, best would be define for bits and values like 0x17.

One more thing, you don't need extra parens surround BIT() macro.

-- 
With Best Regards,
Andy Shevchenko
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 5/9] staging: atomisp: Augment TODO file with GPIO work item

[Andy Shevchenko]

On Thu, 2018-04-19 at 10:41 +0200, Linus Walleij wrote:
> To make sure that these drivers do not leave staging before they
> are properly converted to use the new GPIO descriptor API,
> augment the TODO file with this work item.

Fine by me.

Acked-by: Andy Shevchenko 

> Cc: Alan Cox 
> Cc: Andy Shevchenko 
> Signed-off-by: Linus Walleij 
> ---
> I'm happy to help to move this forward, however Andy is the
> authority on x86 platform drivers and probably knows best
> how to deal with these GPIOs going forward.
> ---
>  drivers/staging/media/atomisp/TODO | 5 +
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/staging/media/atomisp/TODO
> b/drivers/staging/media/atomisp/TODO
> index 255ce3630c2a..683da0cfc313 100644
> --- a/drivers/staging/media/atomisp/TODO
> +++ b/drivers/staging/media/atomisp/TODO
> @@ -50,6 +50,11 @@
>  
>  11. Switch from videobuf1 to videobuf2. Videobuf1 is being removed!
>  
> +12. Convert all uses of the old GPIO API from  to the
> +GPIO descriptor API in  and look up GPIO
> +lines from device properties. See other platform drivers for
> +examples.
> +
>  Limitations:
>  
>  1. To test the patches, you also need the ISP firmware

-- 
Andy Shevchenko 
Intel Finland Oy
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [RESEND PATCH] staging: bcm2835-audio: Disconnect and free vchi_instance on module_exit()

[Andy Shevchenko]

On Tue, Apr 24, 2018 at 10:44 AM, Kirill Marinushkin
 wrote:
> In the current implementation, vchi_instance is inited during the first
> call of bcm2835_audio_open_connection(), and is never freed. It causes a
> memory leak when the module `snd_bcm2835` is removed.


> Signed-off-by: Kirill Marinushkin 
> Cc: Eric Anholt 
> Cc: Stefan Wahren 
> Cc: Greg Kroah-Hartman 
> Cc: Florian Fainelli 
> Cc: Ray Jui 
> Cc: Scott Branden 
> Cc: Andy Shevchenko 
> Cc: bcm-kernel-feedback-l...@broadcom.com
> Cc: linux-rpi-ker...@lists.infradead.org
> Cc: linux-arm-ker...@lists.infradead.org
> Cc: de...@driverdev.osuosl.org
> Cc: linux-ker...@vger.kernel.org

AFAIR I gave you a tag and you again missed it.
Before sending anything just check twice if all prerequisites are fulfilled.

And yes, kbuild bot is right. You need to return known value.

-- 
With Best Regards,
Andy Shevchenko
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [RESEND PATCH] staging: bcm2835-audio: Disconnect and free vchi_instance on module_exit()

[Andy Shevchenko]

On Tue, Apr 24, 2018 at 9:27 PM, Kirill Marinushkin
 wrote:

> @Andy
>
>> AFAIR I gave you a tag and you again missed it.
>> Before sending anything just check twice if all prerequisites are fulfilled.
>
> I think you mix it up. This is a new patch, you didn't review it before.

Ah, okay, send new version as a separate thread and if I have time I
would review it.

-- 
With Best Regards,
Andy Shevchenko
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] media: staging: atomisp: fix a potential missing-check bug

[Andy Shevchenko]

On Fri, 2018-05-04 at 02:29 -0500, Wenwen Wang wrote:
> At the end of atomisp_subdev_set_selection(), the function
> atomisp_subdev_get_rect() is invoked to get the pointer to v4l2_rect.
> Since
> this function may return a NULL pointer, it is firstly invoked to
> check
> the returned pointer. If the returned pointer is not NULL, then the
> function is invoked again to obtain the pointer and the memory content
> at the location of the returned pointer is copied to the memory
> location of
> r. In most cases, the pointers returned by the two invocations are
> same.
> However, given that the pointer returned by the function
> atomisp_subdev_get_rect() is not a constant, it is possible that the
> two
> invocations return two different pointers. For example, another thread
> may
> race to modify the related pointers during the two invocations. In
> that
> case, even if the first returned pointer is not null, the second
> returned
> pointer might be null, which will cause issues such as null pointer
> dereference.
> 
> This patch saves the pointer returned by the first invocation and
> removes
> the second invocation. If the returned pointer is not NULL, the memory
> content is copied according to the original code.
> 

The driver will be gone soon, don't bother with it anymore.
Thanks!

> Signed-off-by: Wenwen Wang 
> ---
>  drivers/staging/media/atomisp/pci/atomisp2/atomisp_subdev.c | 6 -
> -
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git
> a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_subdev.c
> b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_subdev.c
> index 49a9973..d5fa513 100644
> --- a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_subdev.c
> +++ b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_subdev.c
> @@ -366,6 +366,7 @@ int atomisp_subdev_set_selection(struct
> v4l2_subdev *sd,
>   unsigned int i;
>   unsigned int padding_w = pad_w;
>   unsigned int padding_h = pad_h;
> + struct v4l2_rect *p;
>  
>   stream_id = atomisp_source_pad_to_stream_id(isp_sd,
> vdev_pad);
>  
> @@ -536,9 +537,10 @@ int atomisp_subdev_set_selection(struct
> v4l2_subdev *sd,
>   ffmt[pad]->height = comp[pad]->height;
>   }
>  
> - if (!atomisp_subdev_get_rect(sd, cfg, which, pad, target))
> + p = atomisp_subdev_get_rect(sd, cfg, which, pad, target);
> + if (!p)
>   return -EINVAL;
> - *r = *atomisp_subdev_get_rect(sd, cfg, which, pad, target);
> + *r = *p;
>  
>   dev_dbg(isp->dev, "sel actual: l %d t %d w %d h %d\n",
>   r->left, r->top, r->width, r->height);

-- 
Andy Shevchenko 
Intel Finland Oy
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 03/33] Staging: gdm724x: use match_string() helper

[Andy Shevchenko]

On Mon, May 21, 2018 at 2:57 PM, Yisheng Xie  wrote:
> match_string() returns the index of an array for a matching string,
> which can be used intead of open coded variant.


> +   ret = match_string((const char **)DRIVER_STRING,
> +  TTY_MAX_COUNT, tty->driver->driver_name);

It looks like DRIVER_STRING should be converted to const * const.

-- 
With Best Regards,
Andy Shevchenko
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] PCI: hv: Do not wait forever on a device that has disappeared

[Andy Shevchenko]

On Thu, May 24, 2018 at 12:12 AM, Dexuan Cui  wrote:
>
> Before the guest finishes the device initialization, the device can be
> removed anytime by the host, and after that the host won't respond to
> the guest's request, so the guest should be prepared to handle this
> case.

> +   while (true) {
> +   if (hdev->channel->rescind) {
> +   dev_warn_once(&hdev->device, "The device is gone.\n");
> +   return -ENODEV;
> +   }
> +
> +   if (wait_for_completion_timeout(comp, HZ / 10))
> +   break;
> +   }

Infinite loops are usually a red flags.

What's wrong with simple:

do {
  ...
} while (wait_...(...) == 0);

?

> +   if (!ret)
> +   ret = wait_for_response(hdev, &comp);

Better to use well established patterns, i.e.

if (ret)
 return ret;

> +   if (!ret)
> +   ret = wait_for_response(hdev, &comp_pkt.host_event);

Here it looks okay on the first glance, but better to think about it
again and refactor.

-- 
With Best Regards,
Andy Shevchenko
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: rts5208: add check on NULL before dereference

[Andy Shevchenko]

On Sat, Jun 9, 2018 at 7:58 PM,   wrote:
> On 2018-06-09 12:38, Anton Vasilyev wrote:
>>
>> If rtsx_probe fails to allocate dev->chip, then NULL pointer
>> dereference occurs at rtsx_release_resources().
>>
>> Patch adds checks chip on NULL before its dereference at
>> rtsx_release_resources and passing with dereference inside
>> rtsx_release_chip.
>>
>> Found by Linux Driver Verification project (linuxtesting.org).

> I think you should bail out if dev->chip is null rather than adding
> conditiinals.

I'm wondering if it's false positive. At which circumstances that may happen?

-- 
With Best Regards,
Andy Shevchenko
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v5 1/4] resource: Move reparent_resources() to kernel/resource.c and make it public

[Andy Shevchenko]

On Tue, Jun 12, 2018 at 6:28 AM, Baoquan He  wrote:
> reparent_resources() is duplicated in arch/microblaze/pci/pci-common.c
> and arch/powerpc/kernel/pci-common.c, so move it to kernel/resource.c
> so that it's shared. Later its code also need be updated using list_head
> to replace singly linked list.

While this is a good deduplication of the code, some requirements for
public functions would be good to satisfy.

> +/*
> + * Reparent resource children of pr that conflict with res
> + * under res, and make res replace those children.
> + */

kernel doc format, though...

> +static int reparent_resources(struct resource *parent,
> +struct resource *res)

...is it really public with static keyword?!



> +{

> +   for (pp = &parent->child; (p = *pp) != NULL; pp = &p->sibling) {
> +   if (p->end < res->start)
> +   continue;
> +   if (res->end < p->start)
> +   break;

> +   if (p->start < res->start || p->end > res->end)
> +   return -1;  /* not completely contained */

Usually we are expecting real eeror codes.

> +   if (firstpp == NULL)
> +   firstpp = pp;
> +   }

> +   if (firstpp == NULL)
> +   return -1;  /* didn't find any conflicting entries? */

Ditto.

> +}
> +EXPORT_SYMBOL(reparent_resources);

-- 
With Best Regards,
Andy Shevchenko
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v5 1/4] resource: Move reparent_resources() to kernel/resource.c and make it public

[Andy Shevchenko]

On Tue, Jun 12, 2018 at 12:38 PM, Baoquan He  wrote:
> On 06/12/18 at 11:29am, Andy Shevchenko wrote:
>> On Tue, Jun 12, 2018 at 6:28 AM, Baoquan He  wrote:

>> > +{
>>
>> > +   for (pp = &parent->child; (p = *pp) != NULL; pp = &p->sibling) {
>> > +   if (p->end < res->start)
>> > +   continue;
>> > +   if (res->end < p->start)
>> > +   break;
>>
>> > +   if (p->start < res->start || p->end > res->end)
>> > +   return -1;  /* not completely contained */
>>
>> Usually we are expecting real eeror codes.
>
> Hmm, I just copied it from arch/powerpc/kernel/pci-common.c. The
> function interface expects an integer returned value, not sure what a
> real error codes look like, could you give more hints? Will change
> accordingly.

I briefly looked at the code and error codes we have, so, my proposal
is one of the following
 - use -ECANCELED (not the best choice for first occurrence here,
though I can't find better)
 - use positive integers (or enum), like
  #define RES_REPARENTED 0
  #define RES_OVERLAPPED 1
  #define RES_NOCONFLICT 2


>> > +   if (firstpp == NULL)
>> > +   firstpp = pp;
>> > +   }
>>
>> > +   if (firstpp == NULL)
>> > +   return -1;  /* didn't find any conflicting entries? */
>>
>> Ditto.

Ditto.

>>
>> > +}
>> > +EXPORT_SYMBOL(reparent_resources);

-- 
With Best Regards,
Andy Shevchenko
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v5 1/4] resource: Move reparent_resources() to kernel/resource.c and make it public

[Andy Shevchenko]

On Tue, Jun 12, 2018 at 5:20 PM, Andy Shevchenko
 wrote:
> On Tue, Jun 12, 2018 at 12:38 PM, Baoquan He  wrote:
>> On 06/12/18 at 11:29am, Andy Shevchenko wrote:
>>> On Tue, Jun 12, 2018 at 6:28 AM, Baoquan He  wrote:
>
>>> > +{
>>>
>>> > +   for (pp = &parent->child; (p = *pp) != NULL; pp = &p->sibling) {
>>> > +   if (p->end < res->start)
>>> > +   continue;
>>> > +   if (res->end < p->start)
>>> > +   break;
>>>
>>> > +   if (p->start < res->start || p->end > res->end)
>>> > +   return -1;  /* not completely contained */
>>>
>>> Usually we are expecting real eeror codes.
>>
>> Hmm, I just copied it from arch/powerpc/kernel/pci-common.c. The
>> function interface expects an integer returned value, not sure what a
>> real error codes look like, could you give more hints? Will change
>> accordingly.
>
> I briefly looked at the code and error codes we have, so, my proposal
> is one of the following

>  - use -ECANCELED (not the best choice for first occurrence here,
> though I can't find better)

Actually -ENOTSUPP might suit the first case (although the actual
would be something like -EOVERLAP, which we don't have)

>  - use positive integers (or enum), like
>   #define RES_REPARENTED 0
>   #define RES_OVERLAPPED 1
>   #define RES_NOCONFLICT 2
>
>
>>> > +   if (firstpp == NULL)
>>> > +   firstpp = pp;
>>> > +   }
>>>
>>> > +   if (firstpp == NULL)
>>> > +   return -1;  /* didn't find any conflicting entries? */
>>>
>>> Ditto.
>
> Ditto.
>
>>>
>>> > +}
>>> > +EXPORT_SYMBOL(reparent_resources);
>
> --
> With Best Regards,
> Andy Shevchenko



-- 
With Best Regards,
Andy Shevchenko
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 1/4] Staging:rtl8192e Replace function names by using __func__ identifier

[Andy Shevchenko]

On Fri, Jun 8, 2018 at 7:32 AM, Janani Sankara Babu  wrote:
> This patch is created to solve the warning shown by checkpatch script
> Prefer using '"%s...", __func__' to using ', this function's name,
> in a string

Have you even tried to compile it?

NAK

>
> Signed-off-by: Janani Sankara Babu 
> ---
>  drivers/staging/rtl8192e/rtl819x_BAProc.c | 8 
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/staging/rtl8192e/rtl819x_BAProc.c 
> b/drivers/staging/rtl8192e/rtl819x_BAProc.c
> index c466a5e7..3c7ba33 100644
> --- a/drivers/staging/rtl8192e/rtl819x_BAProc.c
> +++ b/drivers/staging/rtl8192e/rtl819x_BAProc.c
> @@ -127,7 +127,7 @@ static struct sk_buff *rtllib_ADDBA(struct rtllib_device 
> *ieee, u8 *Dst,
> }
>
>  #ifdef VERBOSE_DEBUG
> -   print_hex_dump_bytes("rtllib_ADDBA(): ", DUMP_PREFIX_NONE, skb->data,
> +   print_hex_dump_bytes("%s(): ", __func__, DUMP_PREFIX_NONE, skb->data,
>  skb->len);
>  #endif
> return skb;
> @@ -178,7 +178,7 @@ static struct sk_buff *rtllib_DELBA(struct rtllib_device 
> *ieee, u8 *dst,
> tag += 2;
>
>  #ifdef VERBOSE_DEBUG
> -   print_hex_dump_bytes("rtllib_DELBA(): ", DUMP_PREFIX_NONE, skb->data,
> +   print_hex_dump_bytes("%s(): ", __func__, DUMP_PREFIX_NONE, skb->data,
>  skb->len);
>  #endif
> return skb;
> @@ -243,7 +243,7 @@ int rtllib_rx_ADDBAReq(struct rtllib_device *ieee, struct 
> sk_buff *skb)
> }
>
>  #ifdef VERBOSE_DEBUG
> -   print_hex_dump_bytes("rtllib_rx_ADDBAReq(): ", DUMP_PREFIX_NONE,
> +   print_hex_dump_bytes("%s(): ", __func__, DUMP_PREFIX_NONE,
>  skb->data, skb->len);
>  #endif
>
> @@ -441,7 +441,7 @@ int rtllib_rx_DELBA(struct rtllib_device *ieee, struct 
> sk_buff *skb)
> }
>
>  #ifdef VERBOSE_DEBUG
> -   print_hex_dump_bytes("rtllib_rx_DELBA(): ", DUMP_PREFIX_NONE, 
> skb->data,
> +   print_hex_dump_bytes("%s():", __func__, DUMP_PREFIX_NONE, skb->data,
>  skb->len);
>  #endif
> delba = (struct rtllib_hdr_3addr *)skb->data;
> --
> 1.9.1
>



-- 
With Best Regards,
Andy Shevchenko
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel