Re: [PATCH 2/4] i2c: at91: implement i2c bus recovery

[Phil Reid]

On 25/08/2020 21:28, Wolfram Sang wrote:

Hi Phil,

yes, this thread is old but a similar issue came up again...

On Fri, Oct 25, 2019 at 09:14:00AM +0800, Phil Reid wrote:




So at the beginning of a new transfer, we should check if SDA (or SCL?)
is low and, if it's true, only then we should try recover the bus.


Yes, this is the proper time to do it. Remember, I2C does not define a
timeout.



FYI: Just a single poll at the start of the transfer, for it being low, will 
cause problems with multi-master buses.
Bus recovery should be attempted after a timeout when trying to communicate, 
even thou i2c doesn't define a timeout.

I'm trying to fix the designware drivers handling of this at the moment.


I wonder what you ended up with? You are right, a single poll is not
enough. It only might be if one applies the new "single-master" binding
for a given bus. If that is not present, my best idea so far is to poll
SDA for the time defined in adapter->timeout and if it is all low, then
initiate a recovery.



On my todo list still.

Our system eventually recovers at the moment and the multi-master bus
doesn't contain anything that's time critical to our systems operation.


--
Regards
Phil Reid

ElectroMagnetic Imaging Technology Pty Ltd
Development of Geophysical Instrumentation & Software
www.electromag.com.au

3 The Avenue, Midland WA 6056, AUSTRALIA
Ph: +61 8 9250 8100
Fax: +61 8 9250 7100
Email: pr...@electromag.com.au

Re: [PATCH 4.19 32/79] fpga: altera-ps-spi: Fix getting of optional confd gpio

[Phil Reid]

On 22/09/2019 04:46, Pavel Machek wrote:

Hi!


Currently the driver does not handle EPROBE_DEFER for the confd gpio.
Use devm_gpiod_get_optional() instead of devm_gpiod_get() and return
error codes from altera_ps_probe().



@@ -265,10 +265,13 @@ static int altera_ps_probe(struct spi_device *spi)
return PTR_ERR(conf->status);
}
  
-	conf->confd = devm_gpiod_get(>dev, "confd", GPIOD_IN);

+   conf->confd = devm_gpiod_get_optional(>dev, "confd", GPIOD_IN);
if (IS_ERR(conf->confd)) {
-   dev_warn(>dev, "Not using confd gpio: %ld\n",
-PTR_ERR(conf->confd));
+   dev_err(>dev, "Failed to get confd gpio: %ld\n",
+   PTR_ERR(conf->confd));
+   return PTR_ERR(conf->confd);


Will this result in repeated errors in dmesg in case of EPROBE_DEFER?
We often avoid printing such messages in EPROBE_DEFER case.


Yes it will. I can submit a patch for that if required.




+   } else if (!conf->confd) {
+   dev_warn(>dev, "Not using confd gpio");
}
  
  	/* Register manager with unique name */


Best regards,
    Pavel




--
Regards
Phil Reid

Re: [PATCH 4/4] iio: adc: ina2xx: Use label proper for device identification

[Phil Reid]

On 26/08/2019 02:07, Jonathan Cameron wrote:

On Wed, 21 Aug 2019 11:12:00 +0200
Michal Simek  wrote:


On 21. 08. 19 4:11, Phil Reid wrote:

On 20/08/2019 22:11, Michal Simek wrote:

Add support for using label property for easier device identification via
iio framework.

Signed-off-by: Michal Simek 
---

   drivers/iio/adc/ina2xx-adc.c | 2 +-
   1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iio/adc/ina2xx-adc.c b/drivers/iio/adc/ina2xx-adc.c
index 7c7c63677bf4..077c54915f70 100644
--- a/drivers/iio/adc/ina2xx-adc.c
+++ b/drivers/iio/adc/ina2xx-adc.c
@@ -1033,7 +1033,7 @@ static int ina2xx_probe(struct i2c_client *client,
   snprintf(chip->name, sizeof(chip->name), "%s-%s",
    client->name, dev_name(>dev));
   -    indio_dev->name = chip->name;
+    indio_dev->name = of_get_property(np, "label", NULL) ? : chip->name;
   indio_dev->setup_ops = _setup_ops;
     buffer = devm_iio_kfifo_allocate(_dev->dev);
  

I like this personally. It'd be nice if it was a core function so
it could be an opt in to any iio device.

Don't know how well received that'd be thou.
   

I'm not particularly keen on changing the semantics of existing
ABI, but how about adding new ABI to provide this?

/sys/bus/iio/devices/iio\:device0/label for example?

I haven't thought about it in depth yet though.  If you spin
a patch with that and the DT docs we'll be more likely to get
a view from DT maintainers if this is acceptable use of label.



I've sent "iio: core: Add optional symbolic label to device attributes"
for further discussion.



Thanks

Jonathan



Something like this?

diff --git a/drivers/iio/industrialio-core.c
b/drivers/iio/industrialio-core.c
index 524a686077ca..d21b495d36a1 100644
--- a/drivers/iio/industrialio-core.c
+++ b/drivers/iio/industrialio-core.c
@@ -1647,6 +1647,9 @@ int __iio_device_register(struct iio_dev
*indio_dev, struct module *this_mod)
 if (!indio_dev->dev.of_node && indio_dev->dev.parent)
 indio_dev->dev.of_node = indio_dev->dev.parent->of_node;

+   indio_dev->name = of_get_property(indio_dev->dev.of_node,
"label", NULL) ? :
+ indio_dev->name;
+
 ret = iio_check_unique_scan_index(indio_dev);
 if (ret < 0)
 return ret;


M







--
Regards
Phil Reid

ElectroMagnetic Imaging Technology Pty Ltd
Development of Geophysical Instrumentation & Software
www.electromag.com.au

3 The Avenue, Midland WA 6056, AUSTRALIA
Ph: +61 8 9250 8100
Fax: +61 8 9250 7100
Email: pr...@electromag.com.au

Re: iio: Is storing output values to non volatile registers something we should do automatically or leave to userspace action. [was Re: [PATCH] iio: potentiometer: max5432: update the non-volatile pos

[Phil Reid]

On 19/08/2019 03:32, Jonathan Cameron wrote:

On Mon, 12 Aug 2019 19:08:12 +0800
Phil Reid  wrote:


G'day Martin / Jonathan,

On 12/08/2019 18:37, Martin Kaiser wrote:

Hi Jonathan,

Thus wrote Jonathan Cameron (ji...@kernel.org):
   

The patch is fine, but I'm wondering about whether we need some element
of policy control on this restore to current value.
   

A few things to take into account.
   

* Some devices don't have a non volatile store.  So userspace will be
responsible for doing the restore on reboot.
* This may be one of several related devices, and it may make no sense
to restore this one if the others aren't going to be in the same
state as before the reboot.
* Some devices only have non volatile registers for this sort of value
(or save to non volatile on removal of power). Hence any policy to
not store the value can't apply to this class of device.


I see your point. You'd like a consistent bahaviour across all
potentiometer drivers. Or at least a way for user space to figure out
whether a chip uses non-volatile storage or not.
This property doesn't quite fit into the channel info that are defined
in the arrays in drivers/iio/industrialio-core.c. Is there any other way
to set such a property?
   

My initial thought is that these probably don't matter that much and
we should apply this, but I would like to seek input from others!
   

I thought there were some other drivers doing similar store to no
volatile but I can't find one.


drivers/iio/potentiometer/max5481.c and max5487.c do something similar.

They use a command to transfer the setting from volatile to non-volatile
register in the spi remove function. I guess that the intention is to
save the current setting when the system is rebooted. However, my
understanding is that the remove function is called only when a module
is unloaded or when user space does explicitly unbind the device from
the bus via sysfs. That's why I tried using the shutdown function
instead.

Still, this approach has some disadvantages. For many systems, there's a
soft reboot (shutdown -r) where the driver's shutdown function is called
and a "hard reboot" where the power from the CPU and the potentiometer
chip is removed and reapplied. In this case, the current value would not
be transfered to the non-volatile register.

At least for the max5432 family, there's no way to read the current
setting. The only option for user space to have a well-defined setting
is to set the wiper position explicitly at startup.

I guess the only sensible way to use a non-volatile register would be a
write operation where user space gets a response about successful
completion.

We could have two channels to write to the volatile or to non-volatile
register. Or we stick to one channel and update both volatile and
non-volatile registers when user space changes the value. This assumes
that the setting does not change frequently, which is prabably not true
for all applications...


I'm not keen on multiple channels as that is a fairly non obvious interface.
Definitely want to avoid writing all the time.



Whatever we come up with, we should at least make the max* chips behave
the same way.
   

The AD5272/AD5274 Digital Rheostat has a 50-times limit for programming the NV 
register.
So you want to be real sure that you want to set it.


Ouch, I new some were limited to a few thousand cycles, but 50 is rather nasty!



I'd rather my system default to a known "safe" value for next boot than
set to whatever the last write was. So some kind of policy on setting this would
be nice. I personally think it's something that userspace should initiate via 
an explicit
command.

Agreed. I think we should look at an explicit write.

Perhaps an extra attribute on the channels?  Hence a shared_by_all version
could be used when there is only one write command.


Yes, now the only question is what should it be called.





Writing the NV for the AD5272 is something I planned to add at some stage.
But so far the default factory values have worked ok.
It'd be nice for cross device consistency for any interface for this.


Agreed. This is an area that crept up on me, so we haven't enforced any
consistency on it yet.  However, we definitely should!

Thanks,

Jonathan






--
Regards
Phil Reid

Re: [PATCH 4/4] iio: adc: ina2xx: Use label proper for device identification

[Phil Reid]

On 20/08/2019 22:11, Michal Simek wrote:

Add support for using label property for easier device identification via
iio framework.

Signed-off-by: Michal Simek 
---

  drivers/iio/adc/ina2xx-adc.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iio/adc/ina2xx-adc.c b/drivers/iio/adc/ina2xx-adc.c
index 7c7c63677bf4..077c54915f70 100644
--- a/drivers/iio/adc/ina2xx-adc.c
+++ b/drivers/iio/adc/ina2xx-adc.c
@@ -1033,7 +1033,7 @@ static int ina2xx_probe(struct i2c_client *client,
snprintf(chip->name, sizeof(chip->name), "%s-%s",
 client->name, dev_name(>dev));
  
-	indio_dev->name = chip->name;

+   indio_dev->name = of_get_property(np, "label", NULL) ? : chip->name;
indio_dev->setup_ops = _setup_ops;
  
  	buffer = devm_iio_kfifo_allocate(_dev->dev);



I like this personally. It'd be nice if it was a core function so
it could be an opt in to any iio device.

Don't know how well received that'd be thou.


--
Regards
Phil Reid

Re: [PATCH 2/4] iio: adc: ina2xx: Setup better name then simple ina2xx

[Phil Reid]

On 20/08/2019 22:11, Michal Simek wrote:

On systems with multiple ina2xx chips it is impossible to find out which
iio device is which one based on probe order. That's why it is necessary to
setup better name based on possition.
The patch is reusing dev_name which is setup by core with client->name.

name char array was setup to 128 byte length to correspond the same name
length by HID device.

Before this patch:
iio:device9: ina226 (buffer capable)
After:
iio:device9: ina226-3-004a (buffer capable)


Could this break existing user space code that's just looking for just ina226.
I2c bus numbers aren't all that great at id'ing devices either. It's better than
nothing but depending on what cards we have plugged into our system the same 
device gets
a different bus number.




Signed-off-by: Michal Simek 
---

Also id->name can be used as prefix. On ina226 output is the same.

Also I am happy to change that space for name will be dynamicky allocated
to save a space if needed.
---
  drivers/iio/adc/ina2xx-adc.c | 8 +++-
  1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/iio/adc/ina2xx-adc.c b/drivers/iio/adc/ina2xx-adc.c
index 37058d9c2054..7c7c63677bf4 100644
--- a/drivers/iio/adc/ina2xx-adc.c
+++ b/drivers/iio/adc/ina2xx-adc.c
@@ -146,6 +146,7 @@ struct ina2xx_chip_info {
int range_vbus; /* Bus voltage maximum in V */
int pga_gain_vshunt; /* Shunt voltage PGA gain */
bool allow_async_readout;
+   char name[128];
  };
  
  static const struct ina2xx_config ina2xx_config[] = {

@@ -1027,7 +1028,12 @@ static int ina2xx_probe(struct i2c_client *client,
indio_dev->num_channels = ARRAY_SIZE(ina219_channels);
indio_dev->info = _info;
}
-   indio_dev->name = id->name;
+
+   /* Compose chip name to unified i2c format */
+   snprintf(chip->name, sizeof(chip->name), "%s-%s",
+client->name, dev_name(>dev));
+
+   indio_dev->name = chip->name;
indio_dev->setup_ops = _setup_ops;
  
  	buffer = devm_iio_kfifo_allocate(_dev->dev);





--
Regards
Phil Reid

Re: [PATCH 1/4] iio: adc: ina2xx: Define *device_node only once

[Phil Reid]

On 20/08/2019 22:11, Michal Simek wrote:

There is no reason to c full client->dev.of_node link when simple
variable can keep it.

One comment



Signed-off-by: Michal Simek 
---

  drivers/iio/adc/ina2xx-adc.c | 7 ---
  1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/iio/adc/ina2xx-adc.c b/drivers/iio/adc/ina2xx-adc.c
index bdd7cba6f6b0..37058d9c2054 100644
--- a/drivers/iio/adc/ina2xx-adc.c
+++ b/drivers/iio/adc/ina2xx-adc.c
@@ -951,6 +951,7 @@ static int ina2xx_probe(struct i2c_client *client,
struct ina2xx_chip_info *chip;
struct iio_dev *indio_dev;
struct iio_buffer *buffer;
+   struct device_node *np = client->dev.of_node;
unsigned int val;
enum ina2xx_ids type;
int ret;
@@ -970,7 +971,7 @@ static int ina2xx_probe(struct i2c_client *client,
return PTR_ERR(chip->regmap);
}
  
-	if (client->dev.of_node)

+   if (np)
type = (enum ina2xx_ids)of_device_get_match_data(>dev);
else
type = id->driver_data;
@@ -978,7 +979,7 @@ static int ina2xx_probe(struct i2c_client *client,
  
  	mutex_init(>state_lock);
  
-	if (of_property_read_u32(client->dev.of_node,

+   if (of_property_read_u32(np,
 "shunt-resistor", ) < 0) {


This will fit on one line <80 now.


struct ina2xx_platform_data *pdata =
dev_get_platdata(>dev);
@@ -1016,7 +1017,7 @@ static int ina2xx_probe(struct i2c_client *client,
  
  	indio_dev->modes = INDIO_DIRECT_MODE | INDIO_BUFFER_SOFTWARE;

indio_dev->dev.parent = >dev;
-   indio_dev->dev.of_node = client->dev.of_node;
+   indio_dev->dev.of_node = np;
if (id->driver_data == ina226) {
indio_dev->channels = ina226_channels;
indio_dev->num_channels = ARRAY_SIZE(ina226_channels);




--
Regards
Phil Reid

ElectroMagnetic Imaging Technology Pty Ltd
Development of Geophysical Instrumentation & Software
www.electromag.com.au

3 The Avenue, Midland WA 6056, AUSTRALIA
Ph: +61 8 9250 8100
Fax: +61 8 9250 7100
Email: pr...@electromag.com.au

Re: iio: Is storing output values to non volatile registers something we should do automatically or leave to userspace action. [was Re: [PATCH] iio: potentiometer: max5432: update the non-volatile pos

[Phil Reid]

G'day Martin / Jonathan,

On 12/08/2019 18:37, Martin Kaiser wrote:

Hi Jonathan,

Thus wrote Jonathan Cameron (ji...@kernel.org):


The patch is fine, but I'm wondering about whether we need some element
of policy control on this restore to current value.



A few things to take into account.



* Some devices don't have a non volatile store.  So userspace will be
   responsible for doing the restore on reboot.
* This may be one of several related devices, and it may make no sense
   to restore this one if the others aren't going to be in the same
   state as before the reboot.
* Some devices only have non volatile registers for this sort of value
   (or save to non volatile on removal of power). Hence any policy to
   not store the value can't apply to this class of device.


I see your point. You'd like a consistent bahaviour across all
potentiometer drivers. Or at least a way for user space to figure out
whether a chip uses non-volatile storage or not.
This property doesn't quite fit into the channel info that are defined
in the arrays in drivers/iio/industrialio-core.c. Is there any other way
to set such a property?


My initial thought is that these probably don't matter that much and
we should apply this, but I would like to seek input from others!



I thought there were some other drivers doing similar store to no
volatile but I can't find one.


drivers/iio/potentiometer/max5481.c and max5487.c do something similar.

They use a command to transfer the setting from volatile to non-volatile
register in the spi remove function. I guess that the intention is to
save the current setting when the system is rebooted. However, my
understanding is that the remove function is called only when a module
is unloaded or when user space does explicitly unbind the device from
the bus via sysfs. That's why I tried using the shutdown function
instead.

Still, this approach has some disadvantages. For many systems, there's a
soft reboot (shutdown -r) where the driver's shutdown function is called
and a "hard reboot" where the power from the CPU and the potentiometer
chip is removed and reapplied. In this case, the current value would not
be transfered to the non-volatile register.

At least for the max5432 family, there's no way to read the current
setting. The only option for user space to have a well-defined setting
is to set the wiper position explicitly at startup.

I guess the only sensible way to use a non-volatile register would be a
write operation where user space gets a response about successful
completion.

We could have two channels to write to the volatile or to non-volatile
register. Or we stick to one channel and update both volatile and
non-volatile registers when user space changes the value. This assumes
that the setting does not change frequently, which is prabably not true
for all applications...

Whatever we come up with, we should at least make the max* chips behave
the same way.


The AD5272/AD5274 Digital Rheostat has a 50-times limit for programming the NV 
register.
So you want to be real sure that you want to set it.

I'd rather my system default to a known "safe" value for next boot than
set to whatever the last write was. So some kind of policy on setting this would
be nice. I personally think it's something that userspace should initiate via 
an explicit
command.

Writing the NV for the AD5272 is something I planned to add at some stage.
But so far the default factory values have worked ok.
It'd be nice for cross device consistency for any interface for this.


--
Regards
Phil Reid

Re: [PATCH v6 19/57] iio: Remove dev_err() usage after platform_get_irq()

[Phil Reid]

G'day Stephen,

One comment below.

On 31/07/2019 22:32, Stephen Boyd wrote:

Quoting Phil Reid (2019-07-30 23:42:16)

G'day Stephen,

A comment unrelated to your change.

On 31/07/2019 02:15, Stephen Boyd wrote:



diff --git a/drivers/iio/adc/at91_adc.c b/drivers/iio/adc/at91_adc.c
index 32f1c4a33b20..abe99856c823 100644
--- a/drivers/iio/adc/at91_adc.c
+++ b/drivers/iio/adc/at91_adc.c
@@ -1179,10 +1179,8 @@ static int at91_adc_probe(struct platform_device *pdev)
   idev->info = _adc_info;
   
   st->irq = platform_get_irq(pdev, 0);

- if (st->irq < 0) {
- dev_err(>dev, "No IRQ ID is designated\n");
+ if (st->irq < 0)
   return -ENODEV;

Should this be returning st->irq instead of -ENODEV?
eg: platform_get_irq can return -EPROBE_DEFER

Pattern is repeated in a number of other places.


Probably? Here's a patch.

8<
From: Stephen Boyd 
Subject: [PATCH] iio: Return error values from platform_get_irq*()

Sometimes platform_get_irq*() can return -EPROBE_DEFER, so it's best to
return the actual error value from calling this function instead of
overriding the value to something like -EINVAL or -ENXIO. Except for in
the case when the irq value is 0 and the driver knows that irq 0 isn't
valid. In such a situation, return whatever error value was returned
before this change.

Reported-by: Phil Reid 
Cc: Phil Reid 
Cc: Jonathan Cameron 
Cc: Hartmut Knaack 
Cc: Lars-Peter Clausen 
Cc: Peter Meerwald-Stadler 
Cc: linux-...@vger.kernel.org
Cc: Greg Kroah-Hartman 
Signed-off-by: Stephen Boyd 
---
  drivers/iio/adc/at91_adc.c  | 2 +-
  drivers/iio/adc/bcm_iproc_adc.c | 2 +-
  drivers/iio/adc/fsl-imx25-gcq.c | 4 +---
  drivers/iio/adc/lpc32xx_adc.c   | 2 +-
  drivers/iio/adc/npcm_adc.c  | 2 +-
  drivers/iio/adc/spear_adc.c | 2 +-
  6 files changed, 6 insertions(+), 8 deletions(-)

diff --git a/drivers/iio/adc/at91_adc.c b/drivers/iio/adc/at91_adc.c
index abe99856c823..2c604198c4b7 100644
--- a/drivers/iio/adc/at91_adc.c
+++ b/drivers/iio/adc/at91_adc.c
@@ -1180,7 +1180,7 @@ static int at91_adc_probe(struct platform_device *pdev)
  
  	st->irq = platform_get_irq(pdev, 0);

if (st->irq < 0)
-   return -ENODEV;
+   return st->irq;
  
  	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
  
diff --git a/drivers/iio/adc/bcm_iproc_adc.c b/drivers/iio/adc/bcm_iproc_adc.c

index 646ebdc0a8b4..6c05ea510c40 100644
--- a/drivers/iio/adc/bcm_iproc_adc.c
+++ b/drivers/iio/adc/bcm_iproc_adc.c
@@ -541,7 +541,7 @@ static int iproc_adc_probe(struct platform_device *pdev)
  
  	adc_priv->irqno = platform_get_irq(pdev, 0);

if (adc_priv->irqno <= 0)
-   return -ENODEV;
+   return adc_priv->irqno;


return adc_priv->irqno ? : -ENODEV;

  
  	ret = regmap_update_bits(adc_priv->regmap, IPROC_REGCTL2,

IPROC_ADC_AUXIN_SCAN_ENA, 0);
diff --git a/drivers/iio/adc/fsl-imx25-gcq.c b/drivers/iio/adc/fsl-imx25-gcq.c
index fa71489195c6..ee20ab09abe5 100644
--- a/drivers/iio/adc/fsl-imx25-gcq.c
+++ b/drivers/iio/adc/fsl-imx25-gcq.c
@@ -340,9 +340,7 @@ static int mx25_gcq_probe(struct platform_device *pdev)
  
  	priv->irq = platform_get_irq(pdev, 0);

if (priv->irq <= 0) {
-   ret = priv->irq;
-   if (!ret)
-   ret = -ENXIO;
+   ret = priv->irq ? : -ENXIO;
goto err_clk_unprepare;
}
  
diff --git a/drivers/iio/adc/lpc32xx_adc.c b/drivers/iio/adc/lpc32xx_adc.c

index b896f7ff4572..edbb58212fba 100644
--- a/drivers/iio/adc/lpc32xx_adc.c
+++ b/drivers/iio/adc/lpc32xx_adc.c
@@ -173,7 +173,7 @@ static int lpc32xx_adc_probe(struct platform_device *pdev)
  
  	irq = platform_get_irq(pdev, 0);

if (irq <= 0)
-   return -ENXIO;
+   return irq ? : -ENXIO;
  
  	retval = devm_request_irq(>dev, irq, lpc32xx_adc_isr, 0,

  LPC32XXAD_NAME, st);
diff --git a/drivers/iio/adc/npcm_adc.c b/drivers/iio/adc/npcm_adc.c
index 910f3585fa54..1e54a64a4534 100644
--- a/drivers/iio/adc/npcm_adc.c
+++ b/drivers/iio/adc/npcm_adc.c
@@ -225,7 +225,7 @@ static int npcm_adc_probe(struct platform_device *pdev)
  
  	irq = platform_get_irq(pdev, 0);

if (irq <= 0) {
-   ret = -EINVAL;
+   ret = irq ? : -EINVAL;
goto err_disable_clk;
}
  
diff --git a/drivers/iio/adc/spear_adc.c b/drivers/iio/adc/spear_adc.c

index 592b97c464da..9b16717ac7e7 100644
--- a/drivers/iio/adc/spear_adc.c
+++ b/drivers/iio/adc/spear_adc.c
@@ -301,7 +301,7 @@ static int spear_adc_probe(struct platform_device *pdev)
  
  	irq = platform_get_irq(pdev, 0);

if (irq <= 0) {
-   ret = -EINVAL;
+   ret = irq ? : -EINVAL;
goto errout2;
}
  




--
Regards
Phil Reid

Re: [PATCH v6 19/57] iio: Remove dev_err() usage after platform_get_irq()

[Phil Reid]

G'day Stephen,

A comment unrelated to your change.

On 31/07/2019 02:15, Stephen Boyd wrote:



diff --git a/drivers/iio/adc/at91_adc.c b/drivers/iio/adc/at91_adc.c
index 32f1c4a33b20..abe99856c823 100644
--- a/drivers/iio/adc/at91_adc.c
+++ b/drivers/iio/adc/at91_adc.c
@@ -1179,10 +1179,8 @@ static int at91_adc_probe(struct platform_device *pdev)
idev->info = _adc_info;
  
  	st->irq = platform_get_irq(pdev, 0);

-   if (st->irq < 0) {
-   dev_err(>dev, "No IRQ ID is designated\n");
+   if (st->irq < 0)
return -ENODEV;

Should this be returning st->irq instead of -ENODEV?
eg: platform_get_irq can return -EPROBE_DEFER

Pattern is repeated in a number of other places.



-   }
  
  	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
  

Regards
Phil Reid

Re: [PATCH 1/2] [PATCH] gpio: Replace usage of bare 'unsigned' with 'unsigned int'

[Phil Reid]

G'day Hennie,

patch title should be:
gpio: viperboard: Replace usage of bare 'unsigned' with 'unsigned int'

On 21/07/2019 20:52, Hennie Muller wrote:

Fixes a couple of warnings by checkpatch and sparse.

Signed-off-by: Hennie Muller 
---
  drivers/gpio/gpio-viperboard.c | 20 ++--
  1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/gpio/gpio-viperboard.c b/drivers/gpio/gpio-viperboard.c
index 9b604f13e302..c301c1d56dd2 100644
--- a/drivers/gpio/gpio-viperboard.c
+++ b/drivers/gpio/gpio-viperboard.c
@@ -79,7 +79,7 @@ MODULE_PARM_DESC(gpioa_freq,
  /* - begin of gipo a chip  */
  
  static int vprbrd_gpioa_get(struct gpio_chip *chip,

-   unsigned offset)
+   unsigned int offset)


I've encountered these checkpatch warnings as well.

However 'struct gpio_chip' callbacks define the function signatures
as 'unsigned', not 'unsigned int'. So I've also left them as is, to explicitly
match the struct definition.

Be interested to know what the official take on this is.



  {
int ret, answer, error = 0;
struct vprbrd_gpio *gpio = gpiochip_get_data(chip);
@@ -129,7 +129,7 @@ static int vprbrd_gpioa_get(struct gpio_chip *chip,
  }
  
  static void vprbrd_gpioa_set(struct gpio_chip *chip,

-   unsigned offset, int value)
+   unsigned int offset, int value)
  {
int ret;
struct vprbrd_gpio *gpio = gpiochip_get_data(chip);
@@ -170,7 +170,7 @@ static void vprbrd_gpioa_set(struct gpio_chip *chip,
  }
  
  static int vprbrd_gpioa_direction_input(struct gpio_chip *chip,

-   unsigned offset)
+   unsigned int offset)
  {
int ret;
struct vprbrd_gpio *gpio = gpiochip_get_data(chip);
@@ -207,7 +207,7 @@ static int vprbrd_gpioa_direction_input(struct gpio_chip 
*chip,
  }
  
  static int vprbrd_gpioa_direction_output(struct gpio_chip *chip,

-   unsigned offset, int value)
+   unsigned int offset, int value)
  {
int ret;
struct vprbrd_gpio *gpio = gpiochip_get_data(chip);
@@ -251,8 +251,8 @@ static int vprbrd_gpioa_direction_output(struct gpio_chip 
*chip,
  
  /* - begin of gipo b chip  */
  
-static int vprbrd_gpiob_setdir(struct vprbrd *vb, unsigned offset,

-   unsigned dir)
+static int vprbrd_gpiob_setdir(struct vprbrd *vb, unsigned int offset,
+   unsigned int dir)
  {
struct vprbrd_gpiob_msg *gbmsg = (struct vprbrd_gpiob_msg *)vb->buf;
int ret;
@@ -273,7 +273,7 @@ static int vprbrd_gpiob_setdir(struct vprbrd *vb, unsigned 
offset,
  }
  
  static int vprbrd_gpiob_get(struct gpio_chip *chip,

-   unsigned offset)
+   unsigned int offset)
  {
int ret;
u16 val;
@@ -305,7 +305,7 @@ static int vprbrd_gpiob_get(struct gpio_chip *chip,
  }
  
  static void vprbrd_gpiob_set(struct gpio_chip *chip,

-   unsigned offset, int value)
+   unsigned int offset, int value)
  {
int ret;
struct vprbrd_gpio *gpio = gpiochip_get_data(chip);
@@ -338,7 +338,7 @@ static void vprbrd_gpiob_set(struct gpio_chip *chip,
  }
  
  static int vprbrd_gpiob_direction_input(struct gpio_chip *chip,

-   unsigned offset)
+   unsigned int offset)
  {
int ret;
struct vprbrd_gpio *gpio = gpiochip_get_data(chip);
@@ -359,7 +359,7 @@ static int vprbrd_gpiob_direction_input(struct gpio_chip 
*chip,
  }
  
  static int vprbrd_gpiob_direction_output(struct gpio_chip *chip,

-   unsigned offset, int value)
+   unsigned int offset, int value)
  {
int ret;
struct vprbrd_gpio *gpio = gpiochip_get_data(chip);




--
Regards
Phil Reid

Re: [PATCH RFC] gpio: Add Virtual Aggregator GPIO Driver

[Phil Reid]

On 10/07/2019 18:21, Geert Uytterhoeven wrote:

Hi Phil,

On Wed, Jul 10, 2019 at 4:00 AM Phil Reid  wrote:

On 6/07/2019 00:05, Geert Uytterhoeven wrote:

GPIO controllers are exported to userspace using /dev/gpiochip*
character devices.  Access control to these devices is provided by
standard UNIX file system permissions, on an all-or-nothing basis:
either a GPIO controller is accessible for a user, or it is not.
Currently no mechanism exists to control access to individual GPIOs.

Hence add a virtual GPIO driver to aggregate existing GPIOs (up to 32),
and expose them as a new gpiochip.  This is useful for implementing
access control, and assigning a set of GPIOs to a specific user.
Furthermore, it would simplify and harden exporting GPIOs to a virtual
machine, as the VM can just grab the full virtual GPIO controller, and
no longer needs to care about which GPIOs to grab and which not,
reducing the attack surface.

Virtual GPIO controllers are instantiated by writing to the "new_device"
attribute file in sysfs:

  $ echo "  [ ...]"
 "[,   [ ...]] ...]"
  > /sys/bus/platform/drivers/gpio-virt-agg/new_device

Likewise, virtual GPIO controllers can be destroyed after use:

  $ echo gpio-virt-agg. \
  > /sys/bus/platform/drivers/gpio-virt-agg/delete_device



Nice.
This provides similar functionality to the "gpio inverter" driver currently on 
the list.
Other than being just a buffer.


Indeed, both drivers forward GPIO calls, but the gpio inverter modifies
some parameters passed.

The way the drivers obtain references to GPIOs is different, though: the
inverter driver obtains a fixed description from DT, while the virtual
aggregator receives the description at runtime, from sysfs.

But perhaps both drivers could share some code?

Other than probing they're almost the same, except the inversion.
This one's more complete for set / get multiple etc.




Would it be possible to do the lookup via line names?


Doesn't the fact that a GPIO has a line name means that it is in use, and
thus cannot be aggregated and exported to another user?



They can be given line names via the dt property gpio-line-names.
Which can be used by user space to find a gpio. Not sure if there's an 
equivalent api inkerenl.
But it looks like we can find the info via struct gpiochip_info / gpioline_info 
linfo and work
out the chip name and line offsets. So probably not required.

Find the right gpio always seems tricky.
We have systems with multiple i2c gpio behind muxes that may or may not be 
present.
So i2c bus numbers are never consistent. And then different board revisions 
move the
same gpio line to a different pin (or cahnge the gpio chip type completely) to 
make routing easier etc.




--
Regards
Phil Reid

Re: [PATCH 1/2] gpio: em: remove the gpiochip before removing the irq domain

[Phil Reid]

G'day Bartosz,

One comment below

On 10/07/2019 17:08, Bartosz Golaszewski wrote:

From: Bartosz Golaszewski 

In commit 8764c4ca5049 ("gpio: em: use the managed version of
gpiochip_add_data()") we implicitly altered the ordering of resource
freeing: since gpiochip_remove() calls gpiochip_irqchip_remove()
internally, we now can potentially use the irq_domain after it was
destroyed in the remove() callback (as devm resources are freed after
remove() has returned).

Use devm_add_action() to keep the ordering right and entirely kill
the remove() callback in the driver.

Reported-by: Geert Uytterhoeven 
Fixes: 8764c4ca5049 ("gpio: em: use the managed version of gpiochip_add_data()")
Cc: sta...@vger.kernel.org
Signed-off-by: Bartosz Golaszewski 
---
  drivers/gpio/gpio-em.c | 35 +--
  1 file changed, 17 insertions(+), 18 deletions(-)

diff --git a/drivers/gpio/gpio-em.c b/drivers/gpio/gpio-em.c
index b6af705a4e5f..c88028ac66f2 100644
--- a/drivers/gpio/gpio-em.c
+++ b/drivers/gpio/gpio-em.c
@@ -259,6 +259,13 @@ static const struct irq_domain_ops em_gio_irq_domain_ops = 
{
.xlate  = irq_domain_xlate_twocell,
  };
  
+static void em_gio_irq_domain_remove(void *data)

+{
+   struct irq_domain *domain = data;
+
+   irq_domain_remove(domain);
+}
+
  static int em_gio_probe(struct platform_device *pdev)
  {
struct em_gio_priv *p;
@@ -333,39 +340,32 @@ static int em_gio_probe(struct platform_device *pdev)
return -ENXIO;
}
  
+	ret = devm_add_action(>dev,

+ em_gio_irq_domain_remove, p->irq_domain);


Could devm_add_action_or_reset be used?


+   if (ret) {
+   irq_domain_remove(p->irq_domain);
+   return ret;
+   }
+
if (devm_request_irq(>dev, irq[0]->start,
 em_gio_irq_handler, 0, name, p)) {
dev_err(>dev, "failed to request low IRQ\n");
-   ret = -ENOENT;
-   goto err1;
+   return -ENOENT;
}
  
  	if (devm_request_irq(>dev, irq[1]->start,

 em_gio_irq_handler, 0, name, p)) {
dev_err(>dev, "failed to request high IRQ\n");
-   ret = -ENOENT;
-   goto err1;
+   return -ENOENT;
}
  
  	ret = devm_gpiochip_add_data(>dev, gpio_chip, p);

if (ret) {
dev_err(>dev, "failed to add GPIO controller\n");
-   goto err1;
+   return ret;
}
  
  	return 0;

-
-err1:
-   irq_domain_remove(p->irq_domain);
-   return ret;
-}
-
-static int em_gio_remove(struct platform_device *pdev)
-{
-   struct em_gio_priv *p = platform_get_drvdata(pdev);
-
-   irq_domain_remove(p->irq_domain);
-   return 0;
  }
  
  static const struct of_device_id em_gio_dt_ids[] = {

@@ -376,7 +376,6 @@ MODULE_DEVICE_TABLE(of, em_gio_dt_ids);
  
  static struct platform_driver em_gio_device_driver = {

.probe  = em_gio_probe,
-   .remove = em_gio_remove,
.driver = {
    .name   = "em_gio",
.of_match_table = em_gio_dt_ids,




--
Regards
Phil Reid

Re: [PATCH RFC] gpio: Add Virtual Aggregator GPIO Driver

[Phil Reid]

gt;gdev->chip->need_valid_mask) {
+   priv->chip.need_valid_mask = true;
+   priv->chip.init_valid_mask =
+   gpio_virt_agg_init_valid_mask;
+   }
+
+   for (param = s; *param == ' '; param++) ;
+   }
+   if (i == MAX_GPIOS)
+   dev_warn(>dev,
+"Too many gpios specified, truncating to %u\n",
+MAX_GPIOS);
+
+   priv->chip.label = dev_name(dev);
+   priv->chip.parent = dev;
+   priv->chip.owner = THIS_MODULE;
+   priv->chip.get_direction = gpio_virt_agg_get_direction;
+   priv->chip.direction_input = gpio_virt_agg_direction_input;
+   priv->chip.direction_output = gpio_virt_agg_direction_output;
+   priv->chip.get = gpio_virt_agg_get;
+   priv->chip.get_multiple = gpio_virt_agg_get_multiple;
+   priv->chip.set = gpio_virt_agg_set;
+   priv->chip.set_multiple = gpio_virt_agg_set_multiple;
+   priv->chip.base = -1;
+   priv->chip.ngpio = i;
+   platform_set_drvdata(pdev, priv);
+
+   error = gpiochip_add_data(>chip, priv);
+   if (error)
+   goto fail;
+
+   return 0;
+
+fail:
+   while (i-- > 0)
+   gpiod_free(priv->desc[i]);
+
+   return error;
+}
+
+static int gpio_virt_agg_remove(struct platform_device *pdev)
+{
+   struct gpio_virt_agg_priv *priv = platform_get_drvdata(pdev);
+   unsigned int i;
+
+   gpiochip_remove(>chip);
+
+   for (i = 0; i < priv->chip.ngpio; i++)
+   gpiod_free(priv->desc[i]);
+
+   return 0;
+}
+
+static struct platform_driver gpio_virt_agg_driver = {
+   .probe = gpio_virt_agg_probe,
+   .remove = gpio_virt_agg_remove,
+   .driver = {
+   .name = DRV_NAME,
+   .groups = gpio_virt_agg_groups,
+   },
+};
+
+static int __init gpio_virt_agg_init(void)
+{
+   return platform_driver_register(_virt_agg_driver);
+}
+module_init(gpio_virt_agg_init);
+
+static int __exit gpio_virt_agg_idr_remove(int id, void *p, void *data)
+{
+   struct gpio_virt_agg_entry *gva = p;
+
+   platform_device_unregister(gva->pdev);
+   kfree(gva);
+   return 0;
+}
+
+static void __exit gpio_virt_agg_exit(void)
+{
+   mutex_lock(_virt_agg_lock);
+   idr_for_each(_virt_agg_idr, gpio_virt_agg_idr_remove, NULL);
+   idr_destroy(_virt_agg_idr);
+   mutex_unlock(_virt_agg_lock);
+
+   platform_driver_unregister(_virt_agg_driver);
+}
+module_exit(gpio_virt_agg_exit);
+
+MODULE_AUTHOR("Geert Uytterhoeven ");
+MODULE_DESCRIPTION("GPIO Virtual Aggregator");
+MODULE_LICENSE("GPL v2");




--
Regards
Phil Reid

ElectroMagnetic Imaging Technology Pty Ltd
Development of Geophysical Instrumentation & Software
www.electromag.com.au

3 The Avenue, Midland WA 6056, AUSTRALIA
Ph: +61 8 9250 8100
Fax: +61 8 9250 7100
Email: pr...@electromag.com.au

Re: [PATCH V4 2/2] iio: light: isl29018: add optional vdd/vdda regulator operation support

[Phil Reid]

On 11/12/2018 12:29 pm, Anson Huang wrote:

G'day Anson,

Just pulled up the datasheet for this chip.

The absolute max for Vdda is speced as Vddd ±0.5 With a note that Vdda
should be externally shorted to Vddd.

The data sheet says vdda should be connected to vdd externally, then I think we 
should just
use one regulator vdd, then it will be much easier for the driver, what do you 
think?



Yes I think that makes sense.



--
Regards
Phil

Re: [PATCH V4 2/2] iio: light: isl29018: add optional vdd/vdda regulator operation support

[Phil Reid]

G'day Anson,

Just pulled up the datasheet for this chip.

The absolute max for Vdda is speced as Vddd +/-0.5
With a note that Vdda should be externally shorted to Vddd.


On 11/12/2018 11:43 am, Anson Huang wrote:

Hi, Phil

Best Regards!
Anson Huang


-Original Message-
From: Phil Reid [mailto:pr...@electromag.com.au]
Sent: 2018年12月11日 11:36
To: Anson Huang ; ji...@kernel.org;
knaac...@gmx.de; l...@metafoo.de; pme...@pmeerw.net;
robh...@kernel.org; mark.rutl...@arm.com; linux-...@vger.kernel.org;
devicet...@vger.kernel.org; linux-kernel@vger.kernel.org;
feste...@gmail.com
Cc: dl-linux-imx 
Subject: Re: [PATCH V4 2/2] iio: light: isl29018: add optional vdd/vdda
regulator operation support

On 11/12/2018 11:24 am, Anson Huang wrote:

The light sensor's power supply could be controlled by regulator on
some platforms, such as i.MX6Q-SABRESD board, the light sensor
isl29023's power supply is controlled by a GPIO fixed regulator, need
to make sure the regulator is enabled before any operation of sensor,
this patch adds optional vdd/vdda regulator operation support.

Signed-off-by: Anson Huang 
---
ChangeLog since V3:
- improve the error handling of devm_regulator_get_optional;
- make sure regulators are disabled in error path.
---
   drivers/iio/light/isl29018.c | 83

++--

   1 file changed, 80 insertions(+), 3 deletions(-)

diff --git a/drivers/iio/light/isl29018.c
b/drivers/iio/light/isl29018.c index b45400f..a21652b 100644
--- a/drivers/iio/light/isl29018.c
+++ b/drivers/iio/light/isl29018.c
@@ -23,6 +23,7 @@
   #include 
   #include 
   #include 
+#include 
   #include 
   #include 
   #include 
@@ -95,6 +96,8 @@ struct isl29018_chip {
struct isl29018_scale   scale;
int prox_scheme;
boolsuspended;
+   struct regulator*vdd_reg;
+   struct regulator*vdda_reg;
   };

   static int isl29018_set_integration_time(struct isl29018_chip *chip,
@@ -735,6 +738,34 @@ static int isl29018_probe(struct i2c_client
*client,

mutex_init(>lock);

+   chip->vdd_reg = devm_regulator_get_optional(>dev, "vdd");
+   if (!IS_ERR(chip->vdd_reg)) {
+   err = regulator_enable(chip->vdd_reg);
+   if (err) {
+   dev_err(>dev, "failed to enable VDD 
regulator\n");
+   return err;
+   }
+   } else {
+   err = PTR_ERR(chip->vdd_reg);
+   if (err != -ENODEV)
+   return err;
+   }
+
+   chip->vdda_reg = devm_regulator_get_optional(>dev, "vdda");
+   if (!IS_ERR(chip->vdda_reg)) {
+   err = regulator_enable(chip->vdda_reg);
+   if (err) {
+   dev_err(>dev, "failed to enable VDDA 
regulator\n");
+   if (!IS_ERR(chip->vdd_reg))
+   regulator_disable(chip->vdd_reg);
+   return err;

Not sure about this case at the call to enable failed so I think
you only want the first one to be disabled.
You could add another goto statement thou, see below.



+   }
+   } else {
+   err = PTR_ERR(chip->vdda_reg);
+   if (err != -ENODEV)
+   return err;

maybe goto disable_regulators; to disable vdd.


Agree, I will use " goto disable_regulators;" in both here and upper regulator 
enable fail case.
Please help review V5, thanks.


Here its safe to call both as vdda_reg will be an error ptr.



Anson




+   }
+
chip->type = dev_id;
chip->calibscale = 1;
chip->ucalibscale = 0;
@@ -747,12 +778,12 @@ static int isl29018_probe(struct i2c_client *client,
if (IS_ERR(chip->regmap)) {
err = PTR_ERR(chip->regmap);
dev_err(>dev, "regmap initialization fails: %d\n", err);
-   return err;
+   goto disable_regulators;
}

err = isl29018_chip_init(chip);
if (err)
-   return err;
+   goto disable_regulators;

indio_dev->info = isl29018_chip_info_tbl[dev_id].indio_info;
indio_dev->channels = isl29018_chip_info_tbl[dev_id].channels;
@@ -761,13 +792,24 @@ static int isl29018_probe(struct i2c_client *client,
indio_dev->dev.parent = >dev;
indio_dev->modes = INDIO_DIRECT_MODE;

-   return devm_iio_device_register(>dev, indio_dev);
+   err = devm_iio_device_register(>dev, indio_dev);
+   if (!err)
+   return 0;
+
+disable_regulators:
+   if (!IS_ERR(chip->vdd_reg))
+   regulator_disable(chip->vdd_reg);
+   if (!IS_ERR(chip->vdda_reg))
+   regulator_disable(chip->vdda_reg);
+

eg: extra label here.
Order is changed thou, which may 

Re: [PATCH V4 2/2] iio: light: isl29018: add optional vdd/vdda regulator operation support

[Phil Reid]

On 11/12/2018 11:24 am, Anson Huang wrote:

The light sensor's power supply could be controlled by regulator
on some platforms, such as i.MX6Q-SABRESD board, the light sensor
isl29023's power supply is controlled by a GPIO fixed regulator,
need to make sure the regulator is enabled before any operation of
sensor, this patch adds optional vdd/vdda regulator operation support.

Signed-off-by: Anson Huang 
---
ChangeLog since V3:
- improve the error handling of devm_regulator_get_optional;
- make sure regulators are disabled in error path.
---
  drivers/iio/light/isl29018.c | 83 ++--
  1 file changed, 80 insertions(+), 3 deletions(-)

diff --git a/drivers/iio/light/isl29018.c b/drivers/iio/light/isl29018.c
index b45400f..a21652b 100644
--- a/drivers/iio/light/isl29018.c
+++ b/drivers/iio/light/isl29018.c
@@ -23,6 +23,7 @@
  #include 
  #include 
  #include 
+#include 
  #include 
  #include 
  #include 
@@ -95,6 +96,8 @@ struct isl29018_chip {
struct isl29018_scale   scale;
int prox_scheme;
boolsuspended;
+   struct regulator*vdd_reg;
+   struct regulator*vdda_reg;
  };
  
  static int isl29018_set_integration_time(struct isl29018_chip *chip,

@@ -735,6 +738,34 @@ static int isl29018_probe(struct i2c_client *client,
  
  	mutex_init(>lock);
  
+	chip->vdd_reg = devm_regulator_get_optional(>dev, "vdd");

+   if (!IS_ERR(chip->vdd_reg)) {
+   err = regulator_enable(chip->vdd_reg);
+   if (err) {
+   dev_err(>dev, "failed to enable VDD 
regulator\n");
+   return err;
+   }
+   } else {
+   err = PTR_ERR(chip->vdd_reg);
+   if (err != -ENODEV)
+   return err;
+   }
+
+   chip->vdda_reg = devm_regulator_get_optional(>dev, "vdda");
+   if (!IS_ERR(chip->vdda_reg)) {
+   err = regulator_enable(chip->vdda_reg);
+   if (err) {
+   dev_err(>dev, "failed to enable VDDA 
regulator\n");
+   if (!IS_ERR(chip->vdd_reg))
+   regulator_disable(chip->vdd_reg);
+   return err;
+   }
+   } else {
+   err = PTR_ERR(chip->vdda_reg);
+   if (err != -ENODEV)
+   return err;

maybe goto disable_regulators; to disable vdd.


+   }
+
chip->type = dev_id;
chip->calibscale = 1;
chip->ucalibscale = 0;
@@ -747,12 +778,12 @@ static int isl29018_probe(struct i2c_client *client,
if (IS_ERR(chip->regmap)) {
err = PTR_ERR(chip->regmap);
dev_err(>dev, "regmap initialization fails: %d\n", err);
-   return err;
+   goto disable_regulators;
}
  
  	err = isl29018_chip_init(chip);

if (err)
-   return err;
+   goto disable_regulators;
  
  	indio_dev->info = isl29018_chip_info_tbl[dev_id].indio_info;

indio_dev->channels = isl29018_chip_info_tbl[dev_id].channels;
@@ -761,13 +792,24 @@ static int isl29018_probe(struct i2c_client *client,
indio_dev->dev.parent = >dev;
indio_dev->modes = INDIO_DIRECT_MODE;
  
-	return devm_iio_device_register(>dev, indio_dev);

+   err = devm_iio_device_register(>dev, indio_dev);
+   if (!err)
+   return 0;
+
+disable_regulators:
+   if (!IS_ERR(chip->vdd_reg))
+   regulator_disable(chip->vdd_reg);
+   if (!IS_ERR(chip->vdda_reg))
+   regulator_disable(chip->vdda_reg);
+
+   return err;
  }
  

[snip]

--
Regards
Phil

Re: [PATCH V2] iio: magnetometer: mag3110: add optional vdd/vddio regulator operation support

[Phil Reid]

G'day Anson,

On 10/12/2018 3:17 PM, Anson Huang wrote:

The magnetometer's power supply could be controlled by regulator
on some platforms, such as i.MX6Q-SABRESD board, the mag3110's
power supply is controlled by a GPIO fixed regulator, need to make
sure the regulator is enabled before any communication with mag3110,
this patch adds optional vdd/vddio regulator operation support.



[snip]


+
+   data->vdd_reg = devm_regulator_get_optional(>dev, "vdd");
+   if (!IS_ERR(data->vdd_reg)) {
+   ret = regulator_enable(data->vdd_reg);
+   if (ret) {
+   dev_err(>dev, "failed to enable VDD 
regulator\n");
+   return ret;
+   }
+   } else if (data->vdd_reg == ERR_PTR(-EPROBE_DEFER)) {
+   return -EPROBE_DEFER;
+   }
+

IANAE, but normally the return pattern for devm_regulator_get_optional

if (!IS_ERR(reg)) {
regulator_enable ...
} else {
ret = PTR_ERR(reg);
if (ret != -ENODEV)
return ret;
}



--
Regards
Phil

Re: [PATCH v2] staging: iio: ad7816: Switch to the gpio descriptor interface

[Phil Reid]

t;rdwr", GPIOD_IN);
+   if (IS_ERR(chip->rdwr_pin)) {
+   ret = PTR_ERR(chip->rdwr_pin);
+   dev_err(_dev->dev, "Failed to request rdwr GPIO: %d\n",
+   ret);
return ret;
}
-   gpio_direction_input(chip->rdwr_pin);


The RD/WR pin is an input to the AD78xx. So this doesn't make sense being 
GPIOD_IN.



-   ret = devm_gpio_request(_dev->dev, chip->convert_pin,
-   spi_get_device_id(spi_dev)->name);
-   if (ret) {
-   dev_err(_dev->dev, "Fail to request convert gpio PIN %d.\n",
-   chip->convert_pin);
+   chip->convert_pin = devm_gpiod_get(_dev->dev, "convert", GPIOD_IN);
+   if (IS_ERR(chip->convert_pin)) {
+   ret = PTR_ERR(chip->convert_pin);
+   dev_err(_dev->dev, "Failed to request convert GPIO: %d\n",
+   ret);
return ret;
}
-   gpio_direction_input(chip->convert_pin);


ditto, the CONVST pin is an input to the AD78xx.


-   ret = devm_gpio_request(_dev->dev, chip->busy_pin,
-   spi_get_device_id(spi_dev)->name);
-   if (ret) {
-   dev_err(_dev->dev, "Fail to request busy gpio PIN %d.\n",
-   chip->busy_pin);
+   chip->busy_pin = devm_gpiod_get(_dev->dev, "busy", GPIOD_IN);
+   if (IS_ERR(chip->busy_pin)) {
+   ret = PTR_ERR(chip->busy_pin);
+   dev_err(_dev->dev, "Failed to request busy GPIO: %d\n",
+   ret);
return ret;
}
-   gpio_direction_input(chip->busy_pin);



The busy pin doesn't exist on the ad7818.
Which the driver claims to support in the id table:

> static const struct spi_device_id ad7816_id[] = {
>{ "ad7816", 0 },
>{ "ad7817", 0 },
>{ "ad7818", 0 },
>{}
> };

See:
https://www.analog.com/media/en/technical-documentation/data-sheets/AD7817_7818.pdf
Page 9.


  
  	indio_dev->name = spi_get_device_id(spi_dev)->name;

indio_dev->dev.parent = _dev->dev;



Also should the pin names be documented in a device tree binding doc?


--
Regards
Phil Reid

Re: [PATCH v2] staging: iio: ad7816: Switch to the gpio descriptor interface

[Phil Reid]

t;rdwr", GPIOD_IN);
+   if (IS_ERR(chip->rdwr_pin)) {
+   ret = PTR_ERR(chip->rdwr_pin);
+   dev_err(_dev->dev, "Failed to request rdwr GPIO: %d\n",
+   ret);
return ret;
}
-   gpio_direction_input(chip->rdwr_pin);


The RD/WR pin is an input to the AD78xx. So this doesn't make sense being 
GPIOD_IN.



-   ret = devm_gpio_request(_dev->dev, chip->convert_pin,
-   spi_get_device_id(spi_dev)->name);
-   if (ret) {
-   dev_err(_dev->dev, "Fail to request convert gpio PIN %d.\n",
-   chip->convert_pin);
+   chip->convert_pin = devm_gpiod_get(_dev->dev, "convert", GPIOD_IN);
+   if (IS_ERR(chip->convert_pin)) {
+   ret = PTR_ERR(chip->convert_pin);
+   dev_err(_dev->dev, "Failed to request convert GPIO: %d\n",
+   ret);
return ret;
}
-   gpio_direction_input(chip->convert_pin);


ditto, the CONVST pin is an input to the AD78xx.


-   ret = devm_gpio_request(_dev->dev, chip->busy_pin,
-   spi_get_device_id(spi_dev)->name);
-   if (ret) {
-   dev_err(_dev->dev, "Fail to request busy gpio PIN %d.\n",
-   chip->busy_pin);
+   chip->busy_pin = devm_gpiod_get(_dev->dev, "busy", GPIOD_IN);
+   if (IS_ERR(chip->busy_pin)) {
+   ret = PTR_ERR(chip->busy_pin);
+   dev_err(_dev->dev, "Failed to request busy GPIO: %d\n",
+   ret);
return ret;
}
-   gpio_direction_input(chip->busy_pin);



The busy pin doesn't exist on the ad7818.
Which the driver claims to support in the id table:

> static const struct spi_device_id ad7816_id[] = {
>{ "ad7816", 0 },
>{ "ad7817", 0 },
>{ "ad7818", 0 },
>{}
> };

See:
https://www.analog.com/media/en/technical-documentation/data-sheets/AD7817_7818.pdf
Page 9.


  
  	indio_dev->name = spi_get_device_id(spi_dev)->name;

indio_dev->dev.parent = _dev->dev;



Also should the pin names be documented in a device tree binding doc?


--
Regards
Phil Reid

Re: [PATCH v3 3/3] iio: magnetometer: Add driver support for PNI RM3100

[Phil Reid]

+
+static int rm3100_probe(struct spi_device *spi)
+{
+   struct regmap *regmap;
+   int ret;
+
+   /* Actually this device supports both mode 0 and mode 3. */
+   spi->mode = SPI_MODE_0;
+   /* Data rates cannot exceed 1Mbits. */
+   spi->max_speed_hz = 100;
+   spi->bits_per_word = 8;
+   ret = spi_setup(spi);
+   if (ret)
+   return ret;
+
+   regmap = devm_regmap_init_spi(spi, _regmap_config);
+   if (IS_ERR(regmap))
+   return PTR_ERR(regmap);
+
+   return rm3100_common_probe(>dev, regmap, spi->irq);
+}
+
+static const struct of_device_id rm3100_dt_match[] = {
+   { .compatible = "pni,rm3100", },
+   { }
+};
+MODULE_DEVICE_TABLE(of, rm3100_dt_match);
+
+static struct spi_driver rm3100_driver = {
+   .driver = {
+   .name = "rm3100-spi",
+   .of_match_table = rm3100_dt_match,
+   },
+   .probe = rm3100_probe,
+};
+module_spi_driver(rm3100_driver);
+
+MODULE_AUTHOR("Song Qiang ");
+MODULE_DESCRIPTION("PNI RM3100 3-axis magnetometer spi driver");
+MODULE_LICENSE("GPL v2");
diff --git a/drivers/iio/magnetometer/rm3100.h 
b/drivers/iio/magnetometer/rm3100.h
new file mode 100644
index ..c3508218bc77
--- /dev/null
+++ b/drivers/iio/magnetometer/rm3100.h
@@ -0,0 +1,17 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright (C) 2018 Song Qiang 
+ */
+
+#ifndef RM3100_CORE_H
+#define RM3100_CORE_H
+
+#include 
+
+extern const struct regmap_access_table rm3100_readable_table;
+extern const struct regmap_access_table rm3100_writable_table;
+extern const struct regmap_access_table rm3100_volatile_table;
+
+int rm3100_common_probe(struct device *dev, struct regmap *regmap, int irq);
+
+#endif /* RM3100_CORE_H */




--
Regards
Phil Reid

ElectroMagnetic Imaging Technology Pty Ltd
Development of Geophysical Instrumentation & Software
www.electromag.com.au

3 The Avenue, Midland WA 6056, AUSTRALIA
Ph: +61 8 9250 8100
Fax: +61 8 9250 7100
Email: pr...@electromag.com.au

Re: [PATCH v3 3/3] iio: magnetometer: Add driver support for PNI RM3100

[Phil Reid]

+
+static int rm3100_probe(struct spi_device *spi)
+{
+   struct regmap *regmap;
+   int ret;
+
+   /* Actually this device supports both mode 0 and mode 3. */
+   spi->mode = SPI_MODE_0;
+   /* Data rates cannot exceed 1Mbits. */
+   spi->max_speed_hz = 100;
+   spi->bits_per_word = 8;
+   ret = spi_setup(spi);
+   if (ret)
+   return ret;
+
+   regmap = devm_regmap_init_spi(spi, _regmap_config);
+   if (IS_ERR(regmap))
+   return PTR_ERR(regmap);
+
+   return rm3100_common_probe(>dev, regmap, spi->irq);
+}
+
+static const struct of_device_id rm3100_dt_match[] = {
+   { .compatible = "pni,rm3100", },
+   { }
+};
+MODULE_DEVICE_TABLE(of, rm3100_dt_match);
+
+static struct spi_driver rm3100_driver = {
+   .driver = {
+   .name = "rm3100-spi",
+   .of_match_table = rm3100_dt_match,
+   },
+   .probe = rm3100_probe,
+};
+module_spi_driver(rm3100_driver);
+
+MODULE_AUTHOR("Song Qiang ");
+MODULE_DESCRIPTION("PNI RM3100 3-axis magnetometer spi driver");
+MODULE_LICENSE("GPL v2");
diff --git a/drivers/iio/magnetometer/rm3100.h 
b/drivers/iio/magnetometer/rm3100.h
new file mode 100644
index ..c3508218bc77
--- /dev/null
+++ b/drivers/iio/magnetometer/rm3100.h
@@ -0,0 +1,17 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright (C) 2018 Song Qiang 
+ */
+
+#ifndef RM3100_CORE_H
+#define RM3100_CORE_H
+
+#include 
+
+extern const struct regmap_access_table rm3100_readable_table;
+extern const struct regmap_access_table rm3100_writable_table;
+extern const struct regmap_access_table rm3100_volatile_table;
+
+int rm3100_common_probe(struct device *dev, struct regmap *regmap, int irq);
+
+#endif /* RM3100_CORE_H */




--
Regards
Phil Reid

ElectroMagnetic Imaging Technology Pty Ltd
Development of Geophysical Instrumentation & Software
www.electromag.com.au

3 The Avenue, Midland WA 6056, AUSTRALIA
Ph: +61 8 9250 8100
Fax: +61 8 9250 7100
Email: pr...@electromag.com.au

Re: [PATCH 2/2] iio: magnetometer: Add driver support for PNI RM3100

[Phil Reid]

On 26/09/2018 4:09 PM, Song Qiang wrote:

On Wed, Sep 26, 2018 at 10:30:34AM +0800, Phil Reid wrote:

On 26/09/2018 9:49 AM, Song Qiang wrote:

On Tue, Sep 25, 2018 at 10:36:54PM +0800, Phil Reid wrote:

On 25/09/2018 9:30 PM, Jonathan Cameron wrote:

+static irqreturn_t rm3100_trigger_handler(int irq, void *p)
+{
+   struct iio_poll_func *pf = p;
+   struct iio_dev *indio_dev = pf->indio_dev;
+   struct rm3100_data *data = iio_priv(indio_dev);
+   struct regmap *regmap = data->regmap;
+   u8 buffer[9];
+   int ret;
+   int i;
+
+   mutex_lock(>lock);
+   ret = rm3100_wait_measurement(data);
+   if (ret < 0) {
+   mutex_unlock(>lock);
+   goto done;
+   }
+
+   ret = regmap_bulk_read(regmap, RM3100_REG_MX2, buffer, sizeof(buffer));
+   mutex_unlock(>lock);
+   if (ret < 0)
+   goto done;
+
+   /* Convert XXXYYYZZZxxx to XXXxYYYxZZZx. x for padding. */
+   for (i = 0; i < 3; i++)
+   memcpy(data->buffer + i * 4, buffer + i * 3, 3);

Firstly X doesn't need copying.
Secondly the copy of Y actually overwrites the value of Z
XXXYYYZZZxxx
XXXxYYYZZxxx
XXXxYYYxYZZx

I think...


+
+   iio_push_to_buffers_with_timestamp(indio_dev, data->buffer,
+   iio_get_time_ns(indio_dev));


memcpy target is a different buffer so should be ok.

But that raises the question of does it need to be?
'buffer' could be 12 bytes long and just shuffle Z then Y.
Do the unused bytes need to be zeroed? or does libiio mask them anyway?



--
Regards
Phil Reid


Hi Phil,

This is interesting, last patch I submitted uses three transactions and
shuffles X, Y and Z respectively. You said it should be better to use one
transactions, I thought it makes point, and one transaction may reduce
IO pressure of the i2c bus. :)
And that's not necessary for unused bytes to be zero. I'm not familiar
with libiio, actually just been studying it, can't say anything about
it.

yours,
Song Qiang



G'day Song,

yes the one transaction suggestion was to reduce pressure on the bus.
I think also with 3 calls you can up up with other devices taking over
the i2c / spi bus in between.

We've got a devkit for this part, but haven't got to wiring it up to our system 
as yet.
We're looking at using the i2c interface which could push things at max 
samplerate, so yes I'm
keen to see bus pressure reduced as much as possible.

I was thinking something like the following:

u8 buffer[12];
regmap_bulk_read(regmap, RM3100_REG_MX2, buffer, 9);

buffer[10] = buffer[8]; // or memcpy or some other fancy shuffle code.
buffer[9]  = buffer[7];
buffer[8]  = buffer[6];

buffer[6]  = buffer[5];
buffer[5]  = buffer[4];
buffer[4]  = buffer[3];

iio_push_to_buffers_with_timestamp(indio_dev, buffer, 
iio_get_time_ns(indio_dev));

but I'm unsure if this would be needed:
buffer[7] = 0
buffer[3] = 0

What you've got does the job I think.

I haven't dug into the datasheet in great detail, and my iio knownledge is 
limited.
Are you sure the RM3100_CHANNEL scantype endianness is IIO_LE.
rm3100_read_mag looks to be doing big endian conversion and the datasheet 
agrees with that.


--
Regards
Phil Reid



Hi Phil,

You're absolutely right!
This should be big endian, I think I probably just want something there
when I was writing this code, planned to change it later, but apparently
I've forgotten it...

AFAIK, filling places we do not need with 0 is not needed, we just
extract valid data from valid bit field(24 here).

Both one transaction and three transactions way have their point, but
this is a OS, probably the spiltted one is better, I need some real
thinking about this...

The one transaction is better.
Reduces i2c traffic by 6 bytes and ensure the measurements are tightly
coupled in time for the 3 axis.



I could have use the same buffer to read from the sensor and send it to
userspace like this:

int i = 0;
ret = regmap_bulk_read(regmap, RM3100_REG_MX2, 9);
if(ret)
...
/* Convert XXXYYYZZZxxx to XXXxYYYxZZZx. */
for (i = 0; i < 2; i++)
memcpy(buffer + (2 - i) * 4, buffer + (2 - i) * 3), 3);

This code snippet will use the same buffer, actually that's what I was
using the first time. Jonathan must thinks so, from what he commented,
he assumed I was using the same buffer, also what you want.
But I changed this due to Peter's comment, maybe not a big deal, he
suggests to use sizeof(buffer), this makes me use an additional size 9
buffer. I thought this doesn't matter too much, just some additional
space from the stack, but now I think maybe less memory using would be
better...
After all, this length 9 seems like never shouldn't be changed...


yes that does the job.


--
Regards
Phil Reid

Re: [PATCH 2/2] iio: magnetometer: Add driver support for PNI RM3100

[Phil Reid]

On 26/09/2018 4:09 PM, Song Qiang wrote:

On Wed, Sep 26, 2018 at 10:30:34AM +0800, Phil Reid wrote:

On 26/09/2018 9:49 AM, Song Qiang wrote:

On Tue, Sep 25, 2018 at 10:36:54PM +0800, Phil Reid wrote:

On 25/09/2018 9:30 PM, Jonathan Cameron wrote:

+static irqreturn_t rm3100_trigger_handler(int irq, void *p)
+{
+   struct iio_poll_func *pf = p;
+   struct iio_dev *indio_dev = pf->indio_dev;
+   struct rm3100_data *data = iio_priv(indio_dev);
+   struct regmap *regmap = data->regmap;
+   u8 buffer[9];
+   int ret;
+   int i;
+
+   mutex_lock(>lock);
+   ret = rm3100_wait_measurement(data);
+   if (ret < 0) {
+   mutex_unlock(>lock);
+   goto done;
+   }
+
+   ret = regmap_bulk_read(regmap, RM3100_REG_MX2, buffer, sizeof(buffer));
+   mutex_unlock(>lock);
+   if (ret < 0)
+   goto done;
+
+   /* Convert XXXYYYZZZxxx to XXXxYYYxZZZx. x for padding. */
+   for (i = 0; i < 3; i++)
+   memcpy(data->buffer + i * 4, buffer + i * 3, 3);

Firstly X doesn't need copying.
Secondly the copy of Y actually overwrites the value of Z
XXXYYYZZZxxx
XXXxYYYZZxxx
XXXxYYYxYZZx

I think...


+
+   iio_push_to_buffers_with_timestamp(indio_dev, data->buffer,
+   iio_get_time_ns(indio_dev));


memcpy target is a different buffer so should be ok.

But that raises the question of does it need to be?
'buffer' could be 12 bytes long and just shuffle Z then Y.
Do the unused bytes need to be zeroed? or does libiio mask them anyway?



--
Regards
Phil Reid


Hi Phil,

This is interesting, last patch I submitted uses three transactions and
shuffles X, Y and Z respectively. You said it should be better to use one
transactions, I thought it makes point, and one transaction may reduce
IO pressure of the i2c bus. :)
And that's not necessary for unused bytes to be zero. I'm not familiar
with libiio, actually just been studying it, can't say anything about
it.

yours,
Song Qiang



G'day Song,

yes the one transaction suggestion was to reduce pressure on the bus.
I think also with 3 calls you can up up with other devices taking over
the i2c / spi bus in between.

We've got a devkit for this part, but haven't got to wiring it up to our system 
as yet.
We're looking at using the i2c interface which could push things at max 
samplerate, so yes I'm
keen to see bus pressure reduced as much as possible.

I was thinking something like the following:

u8 buffer[12];
regmap_bulk_read(regmap, RM3100_REG_MX2, buffer, 9);

buffer[10] = buffer[8]; // or memcpy or some other fancy shuffle code.
buffer[9]  = buffer[7];
buffer[8]  = buffer[6];

buffer[6]  = buffer[5];
buffer[5]  = buffer[4];
buffer[4]  = buffer[3];

iio_push_to_buffers_with_timestamp(indio_dev, buffer, 
iio_get_time_ns(indio_dev));

but I'm unsure if this would be needed:
buffer[7] = 0
buffer[3] = 0

What you've got does the job I think.

I haven't dug into the datasheet in great detail, and my iio knownledge is 
limited.
Are you sure the RM3100_CHANNEL scantype endianness is IIO_LE.
rm3100_read_mag looks to be doing big endian conversion and the datasheet 
agrees with that.


--
Regards
Phil Reid



Hi Phil,

You're absolutely right!
This should be big endian, I think I probably just want something there
when I was writing this code, planned to change it later, but apparently
I've forgotten it...

AFAIK, filling places we do not need with 0 is not needed, we just
extract valid data from valid bit field(24 here).

Both one transaction and three transactions way have their point, but
this is a OS, probably the spiltted one is better, I need some real
thinking about this...

The one transaction is better.
Reduces i2c traffic by 6 bytes and ensure the measurements are tightly
coupled in time for the 3 axis.



I could have use the same buffer to read from the sensor and send it to
userspace like this:

int i = 0;
ret = regmap_bulk_read(regmap, RM3100_REG_MX2, 9);
if(ret)
...
/* Convert XXXYYYZZZxxx to XXXxYYYxZZZx. */
for (i = 0; i < 2; i++)
memcpy(buffer + (2 - i) * 4, buffer + (2 - i) * 3), 3);

This code snippet will use the same buffer, actually that's what I was
using the first time. Jonathan must thinks so, from what he commented,
he assumed I was using the same buffer, also what you want.
But I changed this due to Peter's comment, maybe not a big deal, he
suggests to use sizeof(buffer), this makes me use an additional size 9
buffer. I thought this doesn't matter too much, just some additional
space from the stack, but now I think maybe less memory using would be
better...
After all, this length 9 seems like never shouldn't be changed...


yes that does the job.


--
Regards
Phil Reid

Re: [PATCH 2/2] iio: magnetometer: Add driver support for PNI RM3100

[Phil Reid]

On 26/09/2018 9:49 AM, Song Qiang wrote:

On Tue, Sep 25, 2018 at 10:36:54PM +0800, Phil Reid wrote:

On 25/09/2018 9:30 PM, Jonathan Cameron wrote:

+static irqreturn_t rm3100_trigger_handler(int irq, void *p)
+{
+   struct iio_poll_func *pf = p;
+   struct iio_dev *indio_dev = pf->indio_dev;
+   struct rm3100_data *data = iio_priv(indio_dev);
+   struct regmap *regmap = data->regmap;
+   u8 buffer[9];
+   int ret;
+   int i;
+
+   mutex_lock(>lock);
+   ret = rm3100_wait_measurement(data);
+   if (ret < 0) {
+   mutex_unlock(>lock);
+   goto done;
+   }
+
+   ret = regmap_bulk_read(regmap, RM3100_REG_MX2, buffer, sizeof(buffer));
+   mutex_unlock(>lock);
+   if (ret < 0)
+   goto done;
+
+   /* Convert XXXYYYZZZxxx to XXXxYYYxZZZx. x for padding. */
+   for (i = 0; i < 3; i++)
+   memcpy(data->buffer + i * 4, buffer + i * 3, 3);

Firstly X doesn't need copying.
Secondly the copy of Y actually overwrites the value of Z
XXXYYYZZZxxx
XXXxYYYZZxxx
XXXxYYYxYZZx

I think...


+
+   iio_push_to_buffers_with_timestamp(indio_dev, data->buffer,
+   iio_get_time_ns(indio_dev));


memcpy target is a different buffer so should be ok.

But that raises the question of does it need to be?
'buffer' could be 12 bytes long and just shuffle Z then Y.
Do the unused bytes need to be zeroed? or does libiio mask them anyway?



--
Regards
Phil Reid


Hi Phil,

This is interesting, last patch I submitted uses three transactions and
shuffles X, Y and Z respectively. You said it should be better to use one
transactions, I thought it makes point, and one transaction may reduce
IO pressure of the i2c bus. :)
And that's not necessary for unused bytes to be zero. I'm not familiar
with libiio, actually just been studying it, can't say anything about
it.

yours,
Song Qiang



G'day Song,

yes the one transaction suggestion was to reduce pressure on the bus.
I think also with 3 calls you can up up with other devices taking over
the i2c / spi bus in between.

We've got a devkit for this part, but haven't got to wiring it up to our system 
as yet.
We're looking at using the i2c interface which could push things at max 
samplerate, so yes I'm
keen to see bus pressure reduced as much as possible.

I was thinking something like the following:

u8 buffer[12];
regmap_bulk_read(regmap, RM3100_REG_MX2, buffer, 9);

buffer[10] = buffer[8]; // or memcpy or some other fancy shuffle code.
buffer[9]  = buffer[7];
buffer[8]  = buffer[6];

buffer[6]  = buffer[5];
buffer[5]  = buffer[4];
buffer[4]  = buffer[3];

iio_push_to_buffers_with_timestamp(indio_dev, buffer, 
iio_get_time_ns(indio_dev));

but I'm unsure if this would be needed:
buffer[7] = 0
buffer[3] = 0

What you've got does the job I think.

I haven't dug into the datasheet in great detail, and my iio knownledge is 
limited.
Are you sure the RM3100_CHANNEL scantype endianness is IIO_LE.
rm3100_read_mag looks to be doing big endian conversion and the datasheet 
agrees with that.


--
Regards
Phil Reid

Re: [PATCH 2/2] iio: magnetometer: Add driver support for PNI RM3100

[Phil Reid]

On 26/09/2018 9:49 AM, Song Qiang wrote:

On Tue, Sep 25, 2018 at 10:36:54PM +0800, Phil Reid wrote:

On 25/09/2018 9:30 PM, Jonathan Cameron wrote:

+static irqreturn_t rm3100_trigger_handler(int irq, void *p)
+{
+   struct iio_poll_func *pf = p;
+   struct iio_dev *indio_dev = pf->indio_dev;
+   struct rm3100_data *data = iio_priv(indio_dev);
+   struct regmap *regmap = data->regmap;
+   u8 buffer[9];
+   int ret;
+   int i;
+
+   mutex_lock(>lock);
+   ret = rm3100_wait_measurement(data);
+   if (ret < 0) {
+   mutex_unlock(>lock);
+   goto done;
+   }
+
+   ret = regmap_bulk_read(regmap, RM3100_REG_MX2, buffer, sizeof(buffer));
+   mutex_unlock(>lock);
+   if (ret < 0)
+   goto done;
+
+   /* Convert XXXYYYZZZxxx to XXXxYYYxZZZx. x for padding. */
+   for (i = 0; i < 3; i++)
+   memcpy(data->buffer + i * 4, buffer + i * 3, 3);

Firstly X doesn't need copying.
Secondly the copy of Y actually overwrites the value of Z
XXXYYYZZZxxx
XXXxYYYZZxxx
XXXxYYYxYZZx

I think...


+
+   iio_push_to_buffers_with_timestamp(indio_dev, data->buffer,
+   iio_get_time_ns(indio_dev));


memcpy target is a different buffer so should be ok.

But that raises the question of does it need to be?
'buffer' could be 12 bytes long and just shuffle Z then Y.
Do the unused bytes need to be zeroed? or does libiio mask them anyway?



--
Regards
Phil Reid


Hi Phil,

This is interesting, last patch I submitted uses three transactions and
shuffles X, Y and Z respectively. You said it should be better to use one
transactions, I thought it makes point, and one transaction may reduce
IO pressure of the i2c bus. :)
And that's not necessary for unused bytes to be zero. I'm not familiar
with libiio, actually just been studying it, can't say anything about
it.

yours,
Song Qiang



G'day Song,

yes the one transaction suggestion was to reduce pressure on the bus.
I think also with 3 calls you can up up with other devices taking over
the i2c / spi bus in between.

We've got a devkit for this part, but haven't got to wiring it up to our system 
as yet.
We're looking at using the i2c interface which could push things at max 
samplerate, so yes I'm
keen to see bus pressure reduced as much as possible.

I was thinking something like the following:

u8 buffer[12];
regmap_bulk_read(regmap, RM3100_REG_MX2, buffer, 9);

buffer[10] = buffer[8]; // or memcpy or some other fancy shuffle code.
buffer[9]  = buffer[7];
buffer[8]  = buffer[6];

buffer[6]  = buffer[5];
buffer[5]  = buffer[4];
buffer[4]  = buffer[3];

iio_push_to_buffers_with_timestamp(indio_dev, buffer, 
iio_get_time_ns(indio_dev));

but I'm unsure if this would be needed:
buffer[7] = 0
buffer[3] = 0

What you've got does the job I think.

I haven't dug into the datasheet in great detail, and my iio knownledge is 
limited.
Are you sure the RM3100_CHANNEL scantype endianness is IIO_LE.
rm3100_read_mag looks to be doing big endian conversion and the datasheet 
agrees with that.


--
Regards
Phil Reid

Re: [PATCH 2/2] iio: magnetometer: Add driver support for PNI RM3100

[Phil Reid]

On 25/09/2018 9:30 PM, Jonathan Cameron wrote:

+static irqreturn_t rm3100_trigger_handler(int irq, void *p)
+{
+   struct iio_poll_func *pf = p;
+   struct iio_dev *indio_dev = pf->indio_dev;
+   struct rm3100_data *data = iio_priv(indio_dev);
+   struct regmap *regmap = data->regmap;
+   u8 buffer[9];
+   int ret;
+   int i;
+
+   mutex_lock(>lock);
+   ret = rm3100_wait_measurement(data);
+   if (ret < 0) {
+   mutex_unlock(>lock);
+   goto done;
+   }
+
+   ret = regmap_bulk_read(regmap, RM3100_REG_MX2, buffer, sizeof(buffer));
+   mutex_unlock(>lock);
+   if (ret < 0)
+   goto done;
+
+   /* Convert XXXYYYZZZxxx to XXXxYYYxZZZx. x for padding. */
+   for (i = 0; i < 3; i++)
+   memcpy(data->buffer + i * 4, buffer + i * 3, 3);

Firstly X doesn't need copying.
Secondly the copy of Y actually overwrites the value of Z
XXXYYYZZZxxx
XXXxYYYZZxxx
XXXxYYYxYZZx

I think...


+
+   iio_push_to_buffers_with_timestamp(indio_dev, data->buffer,
+   iio_get_time_ns(indio_dev));


memcpy target is a different buffer so should be ok.

But that raises the question of does it need to be?
'buffer' could be 12 bytes long and just shuffle Z then Y.
Do the unused bytes need to be zeroed? or does libiio mask them anyway?



--
Regards
Phil Reid

Re: [PATCH 2/2] iio: magnetometer: Add driver support for PNI RM3100

[Phil Reid]

On 25/09/2018 9:30 PM, Jonathan Cameron wrote:

+static irqreturn_t rm3100_trigger_handler(int irq, void *p)
+{
+   struct iio_poll_func *pf = p;
+   struct iio_dev *indio_dev = pf->indio_dev;
+   struct rm3100_data *data = iio_priv(indio_dev);
+   struct regmap *regmap = data->regmap;
+   u8 buffer[9];
+   int ret;
+   int i;
+
+   mutex_lock(>lock);
+   ret = rm3100_wait_measurement(data);
+   if (ret < 0) {
+   mutex_unlock(>lock);
+   goto done;
+   }
+
+   ret = regmap_bulk_read(regmap, RM3100_REG_MX2, buffer, sizeof(buffer));
+   mutex_unlock(>lock);
+   if (ret < 0)
+   goto done;
+
+   /* Convert XXXYYYZZZxxx to XXXxYYYxZZZx. x for padding. */
+   for (i = 0; i < 3; i++)
+   memcpy(data->buffer + i * 4, buffer + i * 3, 3);

Firstly X doesn't need copying.
Secondly the copy of Y actually overwrites the value of Z
XXXYYYZZZxxx
XXXxYYYZZxxx
XXXxYYYxYZZx

I think...


+
+   iio_push_to_buffers_with_timestamp(indio_dev, data->buffer,
+   iio_get_time_ns(indio_dev));


memcpy target is a different buffer so should be ok.

But that raises the question of does it need to be?
'buffer' could be 12 bytes long and just shuffle Z then Y.
Do the unused bytes need to be zeroed? or does libiio mask them anyway?



--
Regards
Phil Reid

Re: [PATCH] iio: magnetometer: Add support for PNI RM3100 9-axis magnetometer

[Phil Reid]

00-spi",
+   .of_match_table = rm3100_dt_match,
+   },
+   .probe = rm3100_probe,
+   .remove = rm3100_remove,
+};
+module_spi_driver(rm3100_driver);
+
+MODULE_AUTHOR("Song Qiang ");
+MODULE_DESCRIPTION("PNI RM3100 9-axis magnetometer spi driver");
+MODULE_LICENSE("GPL v2");
diff --git a/drivers/iio/magnetometer/rm3100.h 
b/drivers/iio/magnetometer/rm3100.h
new file mode 100644
index ..5e30bc0f5149
--- /dev/null
+++ b/drivers/iio/magnetometer/rm3100.h
@@ -0,0 +1,90 @@
+/* SPDX-License-Identifier: GPL-2.0+ */
+/*
+ * Header file for PNI RM3100 driver
+ *
+ * Copyright (C) 2018 Song Qiang 
+ */
+
+#ifndef RM3100_CORE_H
+#define RM3100_CORE_H
+
+#include 
+#include 
+
+#define RM_REG_REV_ID  0x36
+
+/* Cycle Count Registers MSBs and LSBs. */
+#define RM_REG_CCXM0x04
+#define RM_REG_CCXL0x05
+#define RM_REG_CCYM0x06
+#define RM_REG_CCYL0x07
+#define RM_REG_CCZM0x08
+#define RM_REG_CCZL0x09
+
+/* Single Measurement Mode register. */
+#define RM_REG_POLL0x00
+#define RM_POLL_PMXBIT(4)
+#define RM_POLL_PMYBIT(5)
+#define RM_POLL_PMZBIT(6)
+
+/* Continues Measurement Mode register. */
+#define RM_REG_CMM 0x01
+#define RM_CMM_START   BIT(0)
+#define RM_CMM_DRDMBIT(2)
+#define RM_CMM_PMX BIT(4)
+#define RM_CMM_PMY BIT(5)
+#define RM_CMM_PMZ BIT(6)
+
+/* TiMe Rate Configuration register. */
+#define RM_REG_TMRC0x0B
+#define RM_TMRC_OFFSET 0x92
+
+/* Result Status register. */
+#define RM_REG_STATUS  0x34
+#define RM_STATUS_DRDY BIT(7)
+
+/* Measurement result registers. */
+#define RM_REG_MX2 0x24
+#define RM_REG_MX1 0x25
+#define RM_REG_MX0 0x26
+#define RM_REG_MY2 0x27
+#define RM_REG_MY1 0x28
+#define RM_REG_MY0 0x29
+#define RM_REG_MZ2 0x2a
+#define RM_REG_MZ1 0x2b
+#define RM_REG_MZ0 0x2c
+
+#define RM_REG_HSHAKE  0x35
+
+#define RM_W_REG_START RM_REG_POLL
+#define RM_W_REG_END   RM_REG_REV_ID
+#define RM_R_REG_START RM_REG_POLL
+#define RM_R_REG_END   RM_REG_HSHAKE
+#define RM_V_REG_START RM_REG_MX2
+#define RM_V_REG_END   RM_REG_HSHAKE
+
+/* Built-In Self Test reigister. */
+#define RM_REG_BIST0x33
+
+struct rm3100_data {
+   struct device *dev;
+   struct regmap *regmap;
+   struct completion measuring_done;
+   bool use_interrupt;
+
+   int conversion_time;
+
+   /* To protect consistency of every measurement and sampling
+* frequency change operations.
+*/
+   struct mutex lock;
+};
+
+extern const struct regmap_access_table rm3100_readable_table;
+extern const struct regmap_access_table rm3100_writable_table;
+extern const struct regmap_access_table rm3100_volatile_table;
+
+int rm3100_common_probe(struct device *dev, struct regmap *regmap, int irq);
+int rm3100_common_remove(struct device *dev);
+
+#endif /* RM3100_CORE_H */






--
Regards
Phil Reid

Re: [PATCH] iio: magnetometer: Add support for PNI RM3100 9-axis magnetometer

[Phil Reid]

00-spi",
+   .of_match_table = rm3100_dt_match,
+   },
+   .probe = rm3100_probe,
+   .remove = rm3100_remove,
+};
+module_spi_driver(rm3100_driver);
+
+MODULE_AUTHOR("Song Qiang ");
+MODULE_DESCRIPTION("PNI RM3100 9-axis magnetometer spi driver");
+MODULE_LICENSE("GPL v2");
diff --git a/drivers/iio/magnetometer/rm3100.h 
b/drivers/iio/magnetometer/rm3100.h
new file mode 100644
index ..5e30bc0f5149
--- /dev/null
+++ b/drivers/iio/magnetometer/rm3100.h
@@ -0,0 +1,90 @@
+/* SPDX-License-Identifier: GPL-2.0+ */
+/*
+ * Header file for PNI RM3100 driver
+ *
+ * Copyright (C) 2018 Song Qiang 
+ */
+
+#ifndef RM3100_CORE_H
+#define RM3100_CORE_H
+
+#include 
+#include 
+
+#define RM_REG_REV_ID  0x36
+
+/* Cycle Count Registers MSBs and LSBs. */
+#define RM_REG_CCXM0x04
+#define RM_REG_CCXL0x05
+#define RM_REG_CCYM0x06
+#define RM_REG_CCYL0x07
+#define RM_REG_CCZM0x08
+#define RM_REG_CCZL0x09
+
+/* Single Measurement Mode register. */
+#define RM_REG_POLL0x00
+#define RM_POLL_PMXBIT(4)
+#define RM_POLL_PMYBIT(5)
+#define RM_POLL_PMZBIT(6)
+
+/* Continues Measurement Mode register. */
+#define RM_REG_CMM 0x01
+#define RM_CMM_START   BIT(0)
+#define RM_CMM_DRDMBIT(2)
+#define RM_CMM_PMX BIT(4)
+#define RM_CMM_PMY BIT(5)
+#define RM_CMM_PMZ BIT(6)
+
+/* TiMe Rate Configuration register. */
+#define RM_REG_TMRC0x0B
+#define RM_TMRC_OFFSET 0x92
+
+/* Result Status register. */
+#define RM_REG_STATUS  0x34
+#define RM_STATUS_DRDY BIT(7)
+
+/* Measurement result registers. */
+#define RM_REG_MX2 0x24
+#define RM_REG_MX1 0x25
+#define RM_REG_MX0 0x26
+#define RM_REG_MY2 0x27
+#define RM_REG_MY1 0x28
+#define RM_REG_MY0 0x29
+#define RM_REG_MZ2 0x2a
+#define RM_REG_MZ1 0x2b
+#define RM_REG_MZ0 0x2c
+
+#define RM_REG_HSHAKE  0x35
+
+#define RM_W_REG_START RM_REG_POLL
+#define RM_W_REG_END   RM_REG_REV_ID
+#define RM_R_REG_START RM_REG_POLL
+#define RM_R_REG_END   RM_REG_HSHAKE
+#define RM_V_REG_START RM_REG_MX2
+#define RM_V_REG_END   RM_REG_HSHAKE
+
+/* Built-In Self Test reigister. */
+#define RM_REG_BIST0x33
+
+struct rm3100_data {
+   struct device *dev;
+   struct regmap *regmap;
+   struct completion measuring_done;
+   bool use_interrupt;
+
+   int conversion_time;
+
+   /* To protect consistency of every measurement and sampling
+* frequency change operations.
+*/
+   struct mutex lock;
+};
+
+extern const struct regmap_access_table rm3100_readable_table;
+extern const struct regmap_access_table rm3100_writable_table;
+extern const struct regmap_access_table rm3100_volatile_table;
+
+int rm3100_common_probe(struct device *dev, struct regmap *regmap, int irq);
+int rm3100_common_remove(struct device *dev);
+
+#endif /* RM3100_CORE_H */






--
Regards
Phil Reid

Re: [PATCH] iio: magnetometer: Add support for PNI RM3100 9-axis magnetometer

[Phil Reid]

On 20/09/2018 9:13 PM, Song Qiang wrote:

PNI RM3100 magnetometer is a high resolution, large signal immunity
magnetometer, composed of 3 single sensors and a processing chip.
PNI is currently not in the vendors list, so this is also adding it.



In the subject: Isn't the RM3100 a 3axis mag.
The 9axis bit comes when you combine it with an accel / gryo I think.

... snip


+++ b/Documentation/devicetree/bindings/iio/magnetometer/pni,rm3100.txt
@@ -0,0 +1,57 @@
+* PNI RM3100 9-axis magnetometer sensor
+
+I2C Bus:
+
+Required properties:
+
+- compatible : should be "pni,rm3100-i2c"
+- reg : the I2C address of the magnetometer
+


... snip


+SPI Bus:
+
+Required properties:
+
+- compatible : should be "pni,rm3100-spi"
+- reg : address of sensor, usually 0 or 1.
+


Looking at other drivers supporting i2c / spi.
They use the same compatible for both.

eg: see iio/accel/adxl345_*.c

and it's binding doc:

Required properties:
 - compatible : should be "adi,adxl345"
 - reg : the I2C address or SPI chip select number of the sensor

Re: [PATCH] iio: magnetometer: Add support for PNI RM3100 9-axis magnetometer

[Phil Reid]

On 20/09/2018 9:13 PM, Song Qiang wrote:

PNI RM3100 magnetometer is a high resolution, large signal immunity
magnetometer, composed of 3 single sensors and a processing chip.
PNI is currently not in the vendors list, so this is also adding it.



In the subject: Isn't the RM3100 a 3axis mag.
The 9axis bit comes when you combine it with an accel / gryo I think.

... snip


+++ b/Documentation/devicetree/bindings/iio/magnetometer/pni,rm3100.txt
@@ -0,0 +1,57 @@
+* PNI RM3100 9-axis magnetometer sensor
+
+I2C Bus:
+
+Required properties:
+
+- compatible : should be "pni,rm3100-i2c"
+- reg : the I2C address of the magnetometer
+


... snip


+SPI Bus:
+
+Required properties:
+
+- compatible : should be "pni,rm3100-spi"
+- reg : address of sensor, usually 0 or 1.
+


Looking at other drivers supporting i2c / spi.
They use the same compatible for both.

eg: see iio/accel/adxl345_*.c

and it's binding doc:

Required properties:
 - compatible : should be "adi,adxl345"
 - reg : the I2C address or SPI chip select number of the sensor

Re: [PATCH/RFT 1/6] i2c: designware: use open drain for recovery GPIO

[Phil Reid]

On 24/07/2018 15:26, Wolfram Sang wrote:

Hi Phil,


So, it is not possible to read SCL status then? Hmm, currently a working
get_scl is required...

...

Well, I don't know much about this IP core and how/where it is used. I
just wonder what happens if another user comes along using an
open-drain GPIO. Is that possible?

I assume it is the same with SDA? Non open-drain? Output only?



Just had a closer look at how it's setup here.
Maybe the following helps.


Thanks for the detailed explanation. I am just afraid it is a litle too
detailed for me. I am not sure if I can read it correctly:

When you read the SCL/SDA GPIO, does it return the true state of the
SCL/SDA line or does it just reflect the value it was set to output?

Yes it returns the true state of the output pin.
I admit it's a bit odd from the classic GPIO point of view.




There's no concept of HiZ internally in the FPGA.


Which probably means SDA is to be treated the same as SCL -> push/pull.

Yes. They're both driven push/pull, but the try state of the line is available.




If there was some kinda of OpenDrain gpio driver that modelled a FET
driven by a push pull GPIO I guess it could be made to work.


Still, that sounds quite unlikely to me, so we can for now assume that
all designware users will have push/pull?

I know of one other doing the same thing with the core in the Altera SocFPGA 
platform.
As they put me on to this solution for doing the recovery when the i2c was 
routed thru the SOC's fpga.

In other hard configurations they may have a 'proper' GPIO available that needs 
to be OpenDrain.





Disclaimer: I have zero experience with this core, I don't know how hard
it is to modify or which versions are out there.




--
Regards
Phil Reid

Re: [PATCH/RFT 1/6] i2c: designware: use open drain for recovery GPIO

[Phil Reid]

On 24/07/2018 15:26, Wolfram Sang wrote:

Hi Phil,


So, it is not possible to read SCL status then? Hmm, currently a working
get_scl is required...

...

Well, I don't know much about this IP core and how/where it is used. I
just wonder what happens if another user comes along using an
open-drain GPIO. Is that possible?

I assume it is the same with SDA? Non open-drain? Output only?



Just had a closer look at how it's setup here.
Maybe the following helps.


Thanks for the detailed explanation. I am just afraid it is a litle too
detailed for me. I am not sure if I can read it correctly:

When you read the SCL/SDA GPIO, does it return the true state of the
SCL/SDA line or does it just reflect the value it was set to output?

Yes it returns the true state of the output pin.
I admit it's a bit odd from the classic GPIO point of view.




There's no concept of HiZ internally in the FPGA.


Which probably means SDA is to be treated the same as SCL -> push/pull.

Yes. They're both driven push/pull, but the try state of the line is available.




If there was some kinda of OpenDrain gpio driver that modelled a FET
driven by a push pull GPIO I guess it could be made to work.


Still, that sounds quite unlikely to me, so we can for now assume that
all designware users will have push/pull?

I know of one other doing the same thing with the core in the Altera SocFPGA 
platform.
As they put me on to this solution for doing the recovery when the i2c was 
routed thru the SOC's fpga.

In other hard configurations they may have a 'proper' GPIO available that needs 
to be OpenDrain.





Disclaimer: I have zero experience with this core, I don't know how hard
it is to modify or which versions are out there.




--
Regards
Phil Reid

Re: [PATCH/RFT 1/6] i2c: designware: use open drain for recovery GPIO

[Phil Reid]

On 14/07/2018 05:09, Wolfram Sang wrote:

I2C is open drain, so set up the GPIO accordingly.

Signed-off-by: Wolfram Sang 
---
  drivers/i2c/busses/i2c-designware-master.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-designware-master.c 
b/drivers/i2c/busses/i2c-designware-master.c
index fc7c255c80af..a546db80f53e 100644
--- a/drivers/i2c/busses/i2c-designware-master.c
+++ b/drivers/i2c/busses/i2c-designware-master.c
@@ -665,7 +665,7 @@ static int i2c_dw_init_recovery_info(struct dw_i2c_dev *dev)
struct gpio_desc *gpio;
int r;
  
-	gpio = devm_gpiod_get(dev->dev, "scl", GPIOD_OUT_HIGH);

+   gpio = devm_gpiod_get(dev->dev, "scl", GPIOD_OUT_HIGH_OPEN_DRAIN);
if (IS_ERR(gpio)) {
r = PTR_ERR(gpio);
if (r == -ENOENT || r == -ENOSYS)


G'day Wolfram,

This was intentional. The gpio we use to drive the i2c line is implemented in an
FPGA and signals a buffer attached to the GPIO to drive scl OPEN drain. The 
GPIO is output
only.
The gpio setup can still specify the the GPIO be allocated OPEN drain if 
someone wishes
to use a "smarter" gpio.

So while the scl is open drain, there may be hardware in between that isn't.
What would the correct way be to deal with that now?


--
Regards
Phil

Re: [PATCH/RFT 1/6] i2c: designware: use open drain for recovery GPIO

[Phil Reid]

On 14/07/2018 05:09, Wolfram Sang wrote:

I2C is open drain, so set up the GPIO accordingly.

Signed-off-by: Wolfram Sang 
---
  drivers/i2c/busses/i2c-designware-master.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-designware-master.c 
b/drivers/i2c/busses/i2c-designware-master.c
index fc7c255c80af..a546db80f53e 100644
--- a/drivers/i2c/busses/i2c-designware-master.c
+++ b/drivers/i2c/busses/i2c-designware-master.c
@@ -665,7 +665,7 @@ static int i2c_dw_init_recovery_info(struct dw_i2c_dev *dev)
struct gpio_desc *gpio;
int r;
  
-	gpio = devm_gpiod_get(dev->dev, "scl", GPIOD_OUT_HIGH);

+   gpio = devm_gpiod_get(dev->dev, "scl", GPIOD_OUT_HIGH_OPEN_DRAIN);
if (IS_ERR(gpio)) {
r = PTR_ERR(gpio);
if (r == -ENOENT || r == -ENOSYS)


G'day Wolfram,

This was intentional. The gpio we use to drive the i2c line is implemented in an
FPGA and signals a buffer attached to the GPIO to drive scl OPEN drain. The 
GPIO is output
only.
The gpio setup can still specify the the GPIO be allocated OPEN drain if 
someone wishes
to use a "smarter" gpio.

So while the scl is open drain, there may be hardware in between that isn't.
What would the correct way be to deal with that now?


--
Regards
Phil

Re: [PATCH v4 1/2] power: supply: sbs-battery: don't assume MANUFACTURER_DATA formats

[Phil Reid]

On 9/06/2018 06:36, Brian Norris wrote:

This driver was originally submitted for the TI BQ20Z75 battery IC
(commit a7640bfa10c5 ("power_supply: Add driver for TI BQ20Z75 gas gauge
IC")) and later renamed to express generic SBS support. While it's
mostly true that this driver implemented a standard SBS command set, it
takes liberties with the REG_MANUFACTURER_DATA register. This register
is specified in the SBS spec, but it doesn't make any mention of what
its actual contents are.

We've sort of noticed this optionality previously, with commit
17c6d3979e5b ("sbs-battery: make writes to ManufacturerAccess
optional"), where we found that some batteries NAK writes to this
register.

What this really means is that so far, we've just been lucky that most
batteries have either been compatible with the TI chip, or else at least
haven't reported highly-unexpected values.

For instance, one battery I have here seems to report either 0x or
0x0100 to the MANUFACTURER_ACCESS_STATUS command -- while this seems to
match either Wake Up (bits[11:8] = b) or Normal Discharge
(bits[11:8] = 0001b) status for the TI part [1], they don't seem to
actually correspond to real states (for instance, I never see 0101b =
Charge, even when charging).

On other batteries, I'm getting apparently random data in return, which
means that occasionally, we interpret this as "battery not present" or
"battery is not healthy".

All in all, it seems to be a really bad idea to make assumptions about
REG_MANUFACTURER_DATA, unless we already know what battery we're using.
Therefore, this patch reimplements the "present" and "health" checks to
the following on most SBS batteries:

1. HEALTH: report "unknown" -- I couldn't find a standard SBS command
that gives us much useful here
2. PRESENT: just send a REG_STATUS command; if it succeeds, then the
battery is present

Also, we stop sending MANUFACTURER_ACCESS_SLEEP to non-TI parts. I have
no proof that this is useful and supported.

If someone explicitly provided a 'ti,bq20z75' compatible property, then
we continue to use the existing TI command behaviors, and we effectively
revert commit 17c6d3979e5b ("sbs-battery: make writes to
ManufacturerAccess optional") to again make these commands required.

[1] http://www.ti.com/lit/er/sluu265a/sluu265a.pdf

Signed-off-by: Brian Norris 
Reviewed-by: Guenter Roeck 
Acked-by: Rhyland Klein 

Reviewed-by: Phil Reid 

---
v2:
  * don't stub out POWER_SUPPLY_PROP_PRESENT from sbs_data[]
  * use if/else instead of switch/case

v3:
  * pull 'return 0' out of if/else, to satisfy braindead tooling

v4:
  * make ManufacturerAccess non-optional for TI parts again
---
  drivers/power/supply/sbs-battery.c | 67 --
  1 file changed, 54 insertions(+), 13 deletions(-)

diff --git a/drivers/power/supply/sbs-battery.c 
b/drivers/power/supply/sbs-battery.c
index 83d7b4115857..8ba6abf584de 100644
--- a/drivers/power/supply/sbs-battery.c
+++ b/drivers/power/supply/sbs-battery.c
@@ -23,6 +23,7 @@
  #include 
  #include 
  #include 
+#include 
  #include 
  #include 
  #include 
@@ -156,6 +157,9 @@ static enum power_supply_property sbs_properties[] = {
POWER_SUPPLY_PROP_MODEL_NAME
  };
  
+/* Supports special manufacturer commands from TI BQ20Z75 IC. */

+#define SBS_FLAGS_TI_BQ20Z75   BIT(0)
+
  struct sbs_info {
struct i2c_client   *client;
struct power_supply *power_supply;
@@ -168,6 +172,7 @@ struct sbs_info {
u32 poll_retry_count;
struct delayed_work work;
struct mutexmode_lock;
+   u32 flags;
  };
  
  static char model_name[I2C_SMBUS_BLOCK_MAX + 1];

@@ -315,17 +320,41 @@ static int sbs_status_correct(struct i2c_client *client, 
int *intval)
  static int sbs_get_battery_presence_and_health(
struct i2c_client *client, enum power_supply_property psp,
union power_supply_propval *val)
+{
+   int ret;
+
+   if (psp == POWER_SUPPLY_PROP_PRESENT) {
+   /* Dummy command; if it succeeds, battery is present. */
+   ret = sbs_read_word_data(client, sbs_data[REG_STATUS].addr);
+   if (ret < 0)
+   val->intval = 0; /* battery disconnected */
+   else
+   val->intval = 1; /* battery present */
+   } else { /* POWER_SUPPLY_PROP_HEALTH */
+   /* SBS spec doesn't have a general health command. */
+   val->intval = POWER_SUPPLY_HEALTH_UNKNOWN;
+   }
+
+   return 0;
+}
+
+static int sbs_get_ti_battery_presence_and_health(
+   struct i2c_client *client, enum power_supply_property psp,
+   union power_supply_propval *val)
  {
s32 ret;
  
  	/*

 * Write to ManufacturerAccess with ManufacturerAccess command
-* and 

Re: [PATCH v4 1/2] power: supply: sbs-battery: don't assume MANUFACTURER_DATA formats

[Phil Reid]

On 9/06/2018 06:36, Brian Norris wrote:

This driver was originally submitted for the TI BQ20Z75 battery IC
(commit a7640bfa10c5 ("power_supply: Add driver for TI BQ20Z75 gas gauge
IC")) and later renamed to express generic SBS support. While it's
mostly true that this driver implemented a standard SBS command set, it
takes liberties with the REG_MANUFACTURER_DATA register. This register
is specified in the SBS spec, but it doesn't make any mention of what
its actual contents are.

We've sort of noticed this optionality previously, with commit
17c6d3979e5b ("sbs-battery: make writes to ManufacturerAccess
optional"), where we found that some batteries NAK writes to this
register.

What this really means is that so far, we've just been lucky that most
batteries have either been compatible with the TI chip, or else at least
haven't reported highly-unexpected values.

For instance, one battery I have here seems to report either 0x or
0x0100 to the MANUFACTURER_ACCESS_STATUS command -- while this seems to
match either Wake Up (bits[11:8] = b) or Normal Discharge
(bits[11:8] = 0001b) status for the TI part [1], they don't seem to
actually correspond to real states (for instance, I never see 0101b =
Charge, even when charging).

On other batteries, I'm getting apparently random data in return, which
means that occasionally, we interpret this as "battery not present" or
"battery is not healthy".

All in all, it seems to be a really bad idea to make assumptions about
REG_MANUFACTURER_DATA, unless we already know what battery we're using.
Therefore, this patch reimplements the "present" and "health" checks to
the following on most SBS batteries:

1. HEALTH: report "unknown" -- I couldn't find a standard SBS command
that gives us much useful here
2. PRESENT: just send a REG_STATUS command; if it succeeds, then the
battery is present

Also, we stop sending MANUFACTURER_ACCESS_SLEEP to non-TI parts. I have
no proof that this is useful and supported.

If someone explicitly provided a 'ti,bq20z75' compatible property, then
we continue to use the existing TI command behaviors, and we effectively
revert commit 17c6d3979e5b ("sbs-battery: make writes to
ManufacturerAccess optional") to again make these commands required.

[1] http://www.ti.com/lit/er/sluu265a/sluu265a.pdf

Signed-off-by: Brian Norris 
Reviewed-by: Guenter Roeck 
Acked-by: Rhyland Klein 

Reviewed-by: Phil Reid 

---
v2:
  * don't stub out POWER_SUPPLY_PROP_PRESENT from sbs_data[]
  * use if/else instead of switch/case

v3:
  * pull 'return 0' out of if/else, to satisfy braindead tooling

v4:
  * make ManufacturerAccess non-optional for TI parts again
---
  drivers/power/supply/sbs-battery.c | 67 --
  1 file changed, 54 insertions(+), 13 deletions(-)

diff --git a/drivers/power/supply/sbs-battery.c 
b/drivers/power/supply/sbs-battery.c
index 83d7b4115857..8ba6abf584de 100644
--- a/drivers/power/supply/sbs-battery.c
+++ b/drivers/power/supply/sbs-battery.c
@@ -23,6 +23,7 @@
  #include 
  #include 
  #include 
+#include 
  #include 
  #include 
  #include 
@@ -156,6 +157,9 @@ static enum power_supply_property sbs_properties[] = {
POWER_SUPPLY_PROP_MODEL_NAME
  };
  
+/* Supports special manufacturer commands from TI BQ20Z75 IC. */

+#define SBS_FLAGS_TI_BQ20Z75   BIT(0)
+
  struct sbs_info {
struct i2c_client   *client;
struct power_supply *power_supply;
@@ -168,6 +172,7 @@ struct sbs_info {
u32 poll_retry_count;
struct delayed_work work;
struct mutexmode_lock;
+   u32 flags;
  };
  
  static char model_name[I2C_SMBUS_BLOCK_MAX + 1];

@@ -315,17 +320,41 @@ static int sbs_status_correct(struct i2c_client *client, 
int *intval)
  static int sbs_get_battery_presence_and_health(
struct i2c_client *client, enum power_supply_property psp,
union power_supply_propval *val)
+{
+   int ret;
+
+   if (psp == POWER_SUPPLY_PROP_PRESENT) {
+   /* Dummy command; if it succeeds, battery is present. */
+   ret = sbs_read_word_data(client, sbs_data[REG_STATUS].addr);
+   if (ret < 0)
+   val->intval = 0; /* battery disconnected */
+   else
+   val->intval = 1; /* battery present */
+   } else { /* POWER_SUPPLY_PROP_HEALTH */
+   /* SBS spec doesn't have a general health command. */
+   val->intval = POWER_SUPPLY_HEALTH_UNKNOWN;
+   }
+
+   return 0;
+}
+
+static int sbs_get_ti_battery_presence_and_health(
+   struct i2c_client *client, enum power_supply_property psp,
+   union power_supply_propval *val)
  {
s32 ret;
  
  	/*

 * Write to ManufacturerAccess with ManufacturerAccess command
-* and 

Re: [PATCH v3 1/2] power: supply: sbs-battery: don't assume MANUFACTURER_DATA formats

[Phil Reid]

S_TI_BQ20Z75)
+   ret = sbs_get_ti_battery_presence_and_health(client,
+psp, val);
+   else
+   ret = sbs_get_battery_presence_and_health(client, psp,
+ val);
if (psp == POWER_SUPPLY_PROP_PRESENT)
return 0;
break;
@@ -806,6 +837,7 @@ static int sbs_probe(struct i2c_client *client,
if (!chip)
return -ENOMEM;
  
+	chip->flags = (u32)(uintptr_t)of_device_get_match_data(>dev);

chip->client = client;
chip->enable_detection = false;
psy_cfg.of_node = client->dev.of_node;
@@ -915,12 +947,15 @@ static int sbs_suspend(struct device *dev)
if (chip->poll_time > 0)
cancel_delayed_work_sync(>work);
  
-	/*

-* Write to manufacturer access with sleep command.
-* Support is manufacturer dependend, so ignore errors.
-*/
-   sbs_write_word_data(client, sbs_data[REG_MANUFACTURER_DATA].addr,
-   MANUFACTURER_ACCESS_SLEEP);
+   if (chip->flags & SBS_FLAGS_TI_BQ20Z75) {
+   /*
+* Write to manufacturer access with sleep command.
+* Support is manufacturer dependent, so ignore errors.
+*/
+   sbs_write_word_data(client,
+   sbs_data[REG_MANUFACTURER_DATA].addr,
+   MANUFACTURER_ACCESS_SLEEP);
+   }


Now that this is only being done for only TI devices that support this I would 
think the comment
about ignoring errors needs to go, and errors checks added.
Ditto in the new sbs_get_ti_battery_presence_and_health()



  
  	return 0;

  }
@@ -941,7 +976,10 @@ MODULE_DEVICE_TABLE(i2c, sbs_id);
  
  static const struct of_device_id sbs_dt_ids[] = {

{ .compatible = "sbs,sbs-battery" },
-   { .compatible = "ti,bq20z75" },
+   {
+   .compatible = "ti,bq20z75",
+   .data = (void *)SBS_FLAGS_TI_BQ20Z75,
+   },
{ }
  };
  MODULE_DEVICE_TABLE(of, sbs_dt_ids);




--
Regards
Phil Reid

Re: [PATCH v3 1/2] power: supply: sbs-battery: don't assume MANUFACTURER_DATA formats

[Phil Reid]

S_TI_BQ20Z75)
+   ret = sbs_get_ti_battery_presence_and_health(client,
+psp, val);
+   else
+   ret = sbs_get_battery_presence_and_health(client, psp,
+ val);
if (psp == POWER_SUPPLY_PROP_PRESENT)
return 0;
break;
@@ -806,6 +837,7 @@ static int sbs_probe(struct i2c_client *client,
if (!chip)
return -ENOMEM;
  
+	chip->flags = (u32)(uintptr_t)of_device_get_match_data(>dev);

chip->client = client;
chip->enable_detection = false;
psy_cfg.of_node = client->dev.of_node;
@@ -915,12 +947,15 @@ static int sbs_suspend(struct device *dev)
if (chip->poll_time > 0)
cancel_delayed_work_sync(>work);
  
-	/*

-* Write to manufacturer access with sleep command.
-* Support is manufacturer dependend, so ignore errors.
-*/
-   sbs_write_word_data(client, sbs_data[REG_MANUFACTURER_DATA].addr,
-   MANUFACTURER_ACCESS_SLEEP);
+   if (chip->flags & SBS_FLAGS_TI_BQ20Z75) {
+   /*
+* Write to manufacturer access with sleep command.
+* Support is manufacturer dependent, so ignore errors.
+*/
+   sbs_write_word_data(client,
+   sbs_data[REG_MANUFACTURER_DATA].addr,
+   MANUFACTURER_ACCESS_SLEEP);
+   }


Now that this is only being done for only TI devices that support this I would 
think the comment
about ignoring errors needs to go, and errors checks added.
Ditto in the new sbs_get_ti_battery_presence_and_health()



  
  	return 0;

  }
@@ -941,7 +976,10 @@ MODULE_DEVICE_TABLE(i2c, sbs_id);
  
  static const struct of_device_id sbs_dt_ids[] = {

{ .compatible = "sbs,sbs-battery" },
-   { .compatible = "ti,bq20z75" },
+   {
+   .compatible = "ti,bq20z75",
+   .data = (void *)SBS_FLAGS_TI_BQ20Z75,
+   },
{ }
  };
  MODULE_DEVICE_TABLE(of, sbs_dt_ids);




--
Regards
Phil Reid

Re: [PATCHv6] gpio: Remove VLA from gpiolib

[Phil Reid]

/drivers/gpio/gpiolib.h
@@ -188,7 +188,7 @@ int gpiod_get_array_value_complex(bool raw, bool can_sleep,
  unsigned int array_size,
  struct gpio_desc **desc_array,
  int *value_array);
-void gpiod_set_array_value_complex(bool raw, bool can_sleep,
+int gpiod_set_array_value_complex(bool raw, bool can_sleep,
   unsigned int array_size,
   struct gpio_desc **desc_array,
   int *value_array);
diff --git a/include/linux/gpio/consumer.h b/include/linux/gpio/consumer.h
index dbd065963296..243112c7fa7d 100644
--- a/include/linux/gpio/consumer.h
+++ b/include/linux/gpio/consumer.h
@@ -116,7 +116,7 @@ int gpiod_get_raw_array_value(unsigned int array_size,
  struct gpio_desc **desc_array,
  int *value_array);
  void gpiod_set_raw_value(struct gpio_desc *desc, int value);
-void gpiod_set_raw_array_value(unsigned int array_size,
+int gpiod_set_raw_array_value(unsigned int array_size,
   struct gpio_desc **desc_array,
   int *value_array);
  
@@ -134,7 +134,7 @@ int gpiod_get_raw_array_value_cansleep(unsigned int array_size,

   struct gpio_desc **desc_array,
   int *value_array);
  void gpiod_set_raw_value_cansleep(struct gpio_desc *desc, int value);
-void gpiod_set_raw_array_value_cansleep(unsigned int array_size,
+int gpiod_set_raw_array_value_cansleep(unsigned int array_size,
struct gpio_desc **desc_array,
int *value_array);
  
@@ -369,12 +369,13 @@ static inline void gpiod_set_raw_value(struct gpio_desc *desc, int value)

/* GPIO can never have been requested */
WARN_ON(1);
  }
-static inline void gpiod_set_raw_array_value(unsigned int array_size,
+static inline int gpiod_set_raw_array_value(unsigned int array_size,
 struct gpio_desc **desc_array,
 int *value_array)
  {
/* GPIO can never have been requested */
WARN_ON(1);
+   return 0;
  }
  
  static inline int gpiod_get_value_cansleep(const struct gpio_desc *desc)

@@ -423,12 +424,13 @@ static inline void gpiod_set_raw_value_cansleep(struct 
gpio_desc *desc,
/* GPIO can never have been requested */
WARN_ON(1);
  }
-static inline void gpiod_set_raw_array_value_cansleep(unsigned int array_size,
+static inline int gpiod_set_raw_array_value_cansleep(unsigned int array_size,
struct gpio_desc **desc_array,
int *value_array)
  {
/* GPIO can never have been requested */
WARN_ON(1);
+   return 0;
  }
  
  static inline int gpiod_set_debounce(struct gpio_desc *desc, unsigned debounce)



G'day Laura,

Looks good to me.

Reviewed-by: Phil Reid <pr...@electromag.com.au>
--
Regards
Phil Reid

Re: [PATCHv6] gpio: Remove VLA from gpiolib

[Phil Reid]

raw, bool can_sleep,
  unsigned int array_size,
  struct gpio_desc **desc_array,
  int *value_array);
-void gpiod_set_array_value_complex(bool raw, bool can_sleep,
+int gpiod_set_array_value_complex(bool raw, bool can_sleep,
   unsigned int array_size,
   struct gpio_desc **desc_array,
   int *value_array);
diff --git a/include/linux/gpio/consumer.h b/include/linux/gpio/consumer.h
index dbd065963296..243112c7fa7d 100644
--- a/include/linux/gpio/consumer.h
+++ b/include/linux/gpio/consumer.h
@@ -116,7 +116,7 @@ int gpiod_get_raw_array_value(unsigned int array_size,
  struct gpio_desc **desc_array,
  int *value_array);
  void gpiod_set_raw_value(struct gpio_desc *desc, int value);
-void gpiod_set_raw_array_value(unsigned int array_size,
+int gpiod_set_raw_array_value(unsigned int array_size,
   struct gpio_desc **desc_array,
   int *value_array);
  
@@ -134,7 +134,7 @@ int gpiod_get_raw_array_value_cansleep(unsigned int array_size,

   struct gpio_desc **desc_array,
   int *value_array);
  void gpiod_set_raw_value_cansleep(struct gpio_desc *desc, int value);
-void gpiod_set_raw_array_value_cansleep(unsigned int array_size,
+int gpiod_set_raw_array_value_cansleep(unsigned int array_size,
struct gpio_desc **desc_array,
int *value_array);
  
@@ -369,12 +369,13 @@ static inline void gpiod_set_raw_value(struct gpio_desc *desc, int value)

/* GPIO can never have been requested */
WARN_ON(1);
  }
-static inline void gpiod_set_raw_array_value(unsigned int array_size,
+static inline int gpiod_set_raw_array_value(unsigned int array_size,
 struct gpio_desc **desc_array,
 int *value_array)
  {
/* GPIO can never have been requested */
WARN_ON(1);
+   return 0;
  }
  
  static inline int gpiod_get_value_cansleep(const struct gpio_desc *desc)

@@ -423,12 +424,13 @@ static inline void gpiod_set_raw_value_cansleep(struct 
gpio_desc *desc,
/* GPIO can never have been requested */
WARN_ON(1);
  }
-static inline void gpiod_set_raw_array_value_cansleep(unsigned int array_size,
+static inline int gpiod_set_raw_array_value_cansleep(unsigned int array_size,
struct gpio_desc **desc_array,
int *value_array)
  {
/* GPIO can never have been requested */
WARN_ON(1);
+   return 0;
  }
  
  static inline int gpiod_set_debounce(struct gpio_desc *desc, unsigned debounce)



G'day Laura,

Looks good to me.

Reviewed-by: Phil Reid 
--
Regards
Phil Reid

Re: [PATCHv5] gpio: Remove VLA from gpiolib

[Phil Reid]

On 15/05/2018 15:10, Geert Uytterhoeven wrote:

Hi Laura,

On Tue, May 15, 2018 at 12:49 AM, Laura Abbott <labb...@redhat.com> wrote:

On 04/20/2018 02:02 AM, Geert Uytterhoeven wrote:

On Fri, Apr 13, 2018 at 11:24 PM, Laura Abbott <labb...@redhat.com> wrote:

The new challenge is to remove VLAs from the kernel
(see https://lkml.org/lkml/2018/3/7/621) to eventually
turn on -Wvla.

Using a kmalloc array is the easy way to fix this but kmalloc is still
more expensive than stack allocation. Introduce a fast path with a
fixed size stack array to cover most chip with gpios below some fixed
amount. The slow path dynamically allocates an array to cover those
chips with a large number of gpios.



Blindly replacing VLAs by allocated arrays is IMHO not such a good
solution.
On the largest systems, NR_GPIOS is 2048, so that makes 2 arrays of 256
bytes. That's an uppper limit, and assumes they are all on the same
gpiochip,
which they probably aren't.

Still, 2 x 256 bytes is a lot, so I agree it should be fixed.

So, wouldn't it make more sense to not allocate memory, but just process
the request in chunks (say, at most 128 gpios per chunk, i.e. 2 x
16 bytes)? The code already caters for handling chunks due to not all
gpios
belonging to the same gpiochip. That will probably also be faster than
allocating memory, which is the main idea behind this API.


Reviewed-and-tested-by: Lukas Wunner <lu...@wunner.de>
Signed-off-by: Lukas Wunner <lu...@wunner.de>
Signed-off-by: Laura Abbott <labb...@redhat.com>




--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c




@@ -1192,6 +1196,10 @@ int gpiochip_add_data_with_key(struct gpio_chip
*chip, void *data,
  goto err_free_descs;
  }

+   if (chip->ngpio > FASTPATH_NGPIO)
+   chip_warn(chip, "line cnt %d is greater than fast path
cnt %d\n",
+   chip->ngpio, FASTPATH_NGPIO);



FWIW, can this warning be triggered from userspace?


@@ -2662,16 +2670,28 @@ int gpiod_get_array_value_complex(bool raw, bool
can_sleep,

  while (i < array_size) {
  struct gpio_chip *chip = desc_array[i]->gdev->chip;
-   unsigned long mask[BITS_TO_LONGS(chip->ngpio)];
-   unsigned long bits[BITS_TO_LONGS(chip->ngpio)];



Hence just use a fixed size here...


+   unsigned long fastpath[2 *
BITS_TO_LONGS(FASTPATH_NGPIO)];
+   unsigned long *mask, *bits;
  int first, j, ret;

+   if (likely(chip->ngpio <= FASTPATH_NGPIO)) {
+   memset(fastpath, 0, sizeof(fastpath));
+   mask = fastpath;
+   bits = fastpath + BITS_TO_LONGS(FASTPATH_NGPIO);
+   } else {
+   mask = kcalloc(2 * BITS_TO_LONGS(chip->ngpio),
+  sizeof(*mask),
+  can_sleep ? GFP_KERNEL :
GFP_ATOMIC);
+   if (!mask)
+   return -ENOMEM;
+   bits = mask + BITS_TO_LONGS(chip->ngpio);
+   }
+
  if (!can_sleep)
  WARN_ON(chip->can_sleep);

  /* collect all inputs belonging to the same chip */
  first = i;
-   memset(mask, 0, sizeof(mask));
  do {
  const struct gpio_desc *desc = desc_array[i];
  int hwgpio = gpio_chip_hwgpio(desc);



Out-of-context, the code does:

|   __set_bit(hwgpio, mask);
|   i++;
| } while ((i < array_size) &&

... and change this limit to "(i < min(array_size, first +
ARRAY_SIZE(mask) * BITS_PER_BYTE))"

| (desc_array[i]->gdev->chip == chip));

... and you're done?


I don't think this approach will work since gpio_chip_{get,set}_multiple
expect to be working on arrays for the entire chip. There doesn't seem
to be a nice way to work on a subset of GPIOs without defeating the point
of the multiple API.


You're right, sorry for missing that.


is 2*256 = 512 bytes really too much stack space? I guess we could
switch to a Kconfig to allow for better bounds.


That's a good question.

As long as a VLA is used, it's probably fine, as chip->ngpio is quite small.
If you would change it to an array that can accommodate NR_GPIOS bits, you
have to start caring about recursion (e.g. gpio-74x164 driven from spi-gpio,
where I can extend the chain to increase the level of recursion arbitrarily).



I think a config option for FASTPATH_NGPIO is preferable.

As I've mentioned ARCH_NR_GPIOS is much greater than any chip->ngpio on
my platform.
It's at least one order of magnitude, almost 2.


--
Regards
Phil Reid

Re: [PATCHv5] gpio: Remove VLA from gpiolib

[Phil Reid]

On 15/05/2018 15:10, Geert Uytterhoeven wrote:

Hi Laura,

On Tue, May 15, 2018 at 12:49 AM, Laura Abbott  wrote:

On 04/20/2018 02:02 AM, Geert Uytterhoeven wrote:

On Fri, Apr 13, 2018 at 11:24 PM, Laura Abbott  wrote:

The new challenge is to remove VLAs from the kernel
(see https://lkml.org/lkml/2018/3/7/621) to eventually
turn on -Wvla.

Using a kmalloc array is the easy way to fix this but kmalloc is still
more expensive than stack allocation. Introduce a fast path with a
fixed size stack array to cover most chip with gpios below some fixed
amount. The slow path dynamically allocates an array to cover those
chips with a large number of gpios.



Blindly replacing VLAs by allocated arrays is IMHO not such a good
solution.
On the largest systems, NR_GPIOS is 2048, so that makes 2 arrays of 256
bytes. That's an uppper limit, and assumes they are all on the same
gpiochip,
which they probably aren't.

Still, 2 x 256 bytes is a lot, so I agree it should be fixed.

So, wouldn't it make more sense to not allocate memory, but just process
the request in chunks (say, at most 128 gpios per chunk, i.e. 2 x
16 bytes)? The code already caters for handling chunks due to not all
gpios
belonging to the same gpiochip. That will probably also be faster than
allocating memory, which is the main idea behind this API.


Reviewed-and-tested-by: Lukas Wunner 
Signed-off-by: Lukas Wunner 
Signed-off-by: Laura Abbott 




--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c




@@ -1192,6 +1196,10 @@ int gpiochip_add_data_with_key(struct gpio_chip
*chip, void *data,
  goto err_free_descs;
  }

+   if (chip->ngpio > FASTPATH_NGPIO)
+   chip_warn(chip, "line cnt %d is greater than fast path
cnt %d\n",
+   chip->ngpio, FASTPATH_NGPIO);



FWIW, can this warning be triggered from userspace?


@@ -2662,16 +2670,28 @@ int gpiod_get_array_value_complex(bool raw, bool
can_sleep,

  while (i < array_size) {
  struct gpio_chip *chip = desc_array[i]->gdev->chip;
-   unsigned long mask[BITS_TO_LONGS(chip->ngpio)];
-   unsigned long bits[BITS_TO_LONGS(chip->ngpio)];



Hence just use a fixed size here...


+   unsigned long fastpath[2 *
BITS_TO_LONGS(FASTPATH_NGPIO)];
+   unsigned long *mask, *bits;
  int first, j, ret;

+   if (likely(chip->ngpio <= FASTPATH_NGPIO)) {
+   memset(fastpath, 0, sizeof(fastpath));
+   mask = fastpath;
+   bits = fastpath + BITS_TO_LONGS(FASTPATH_NGPIO);
+   } else {
+   mask = kcalloc(2 * BITS_TO_LONGS(chip->ngpio),
+  sizeof(*mask),
+  can_sleep ? GFP_KERNEL :
GFP_ATOMIC);
+   if (!mask)
+   return -ENOMEM;
+   bits = mask + BITS_TO_LONGS(chip->ngpio);
+   }
+
  if (!can_sleep)
  WARN_ON(chip->can_sleep);

  /* collect all inputs belonging to the same chip */
  first = i;
-   memset(mask, 0, sizeof(mask));
  do {
  const struct gpio_desc *desc = desc_array[i];
  int hwgpio = gpio_chip_hwgpio(desc);



Out-of-context, the code does:

|   __set_bit(hwgpio, mask);
|   i++;
| } while ((i < array_size) &&

... and change this limit to "(i < min(array_size, first +
ARRAY_SIZE(mask) * BITS_PER_BYTE))"

| (desc_array[i]->gdev->chip == chip));

... and you're done?


I don't think this approach will work since gpio_chip_{get,set}_multiple
expect to be working on arrays for the entire chip. There doesn't seem
to be a nice way to work on a subset of GPIOs without defeating the point
of the multiple API.


You're right, sorry for missing that.


is 2*256 = 512 bytes really too much stack space? I guess we could
switch to a Kconfig to allow for better bounds.


That's a good question.

As long as a VLA is used, it's probably fine, as chip->ngpio is quite small.
If you would change it to an array that can accommodate NR_GPIOS bits, you
have to start caring about recursion (e.g. gpio-74x164 driven from spi-gpio,
where I can extend the chain to increase the level of recursion arbitrarily).



I think a config option for FASTPATH_NGPIO is preferable.

As I've mentioned ARCH_NR_GPIOS is much greater than any chip->ngpio on
my platform.
It's at least one order of magnitude, almost 2.


--
Regards
Phil Reid

Re: [PATCHv5] gpio: Remove VLA from gpiolib

[Phil Reid]

On 16/04/2018 13:19, Phil Reid wrote:

G'day Laura,

One more comment.
On 16/04/2018 12:41, Phil Reid wrote:

G'day Laura,

On 14/04/2018 05:24, Laura Abbott wrote:

The new challenge is to remove VLAs from the kernel
(see https://lkml.org/lkml/2018/3/7/621) to eventually
turn on -Wvla.

Using a kmalloc array is the easy way to fix this but kmalloc is still
more expensive than stack allocation. Introduce a fast path with a
fixed size stack array to cover most chip with gpios below some fixed
amount. The slow path dynamically allocates an array to cover those
chips with a large number of gpios.

Reviewed-and-tested-by: Lukas Wunner <lu...@wunner.de>
Signed-off-by: Lukas Wunner <lu...@wunner.de>
Signed-off-by: Laura Abbott <labb...@redhat.com>
---
v5: Dropped some outdated comments and extra whitespace. Switched to
ARCH_NR_GPIOS per suggestion of Linus Walleij.
---
  drivers/gpio/gpiolib.c    | 76 +--
  drivers/gpio/gpiolib.h    |  2 +-
  include/linux/gpio/consumer.h | 10 +++---
  3 files changed, 66 insertions(+), 22 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index d66de67ef307..79ec7a29b684 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -61,6 +61,11 @@ static struct bus_type gpio_bus_type = {
  .name = "gpio",
  };
+/*
+ * Number of GPIOs to use for the fast path in set array
+ */
+#define FASTPATH_NGPIO ARCH_NR_GPIOS


Also wouldn't this mean that fast path will never be triggered now...

Just to be clearer. That this will always be true. (chip->ngpio <= 
FASTPATH_NGPIO)




+
  /* gpio_lock prevents conflicts during gpio_desc[] table updates.
   * While any GPIO is requested, its gpio_chip is not removable;
   * each GPIO's "requested" flag serves as a lock and refcount.
@@ -399,12 +404,11 @@ static long linehandle_ioctl(struct file *filep, unsigned 
int cmd,
  vals[i] = !!ghd.values[i];
  /* Reuse the array setting function */
-    gpiod_set_array_value_complex(false,
+    return gpiod_set_array_value_complex(false,
    true,
    lh->numdescs,
    lh->descs,
    vals);
-    return 0;
  }
  return -EINVAL;
  }
@@ -1192,6 +1196,10 @@ int gpiochip_add_data_with_key(struct gpio_chip *chip, 
void *data,
  goto err_free_descs;
  }
+    if (chip->ngpio > FASTPATH_NGPIO)
+    chip_warn(chip, "line cnt %d is greater than fast path cnt %d\n",
+    chip->ngpio, FASTPATH_NGPIO);
+
  gdev->label = kstrdup_const(chip->label ?: "unknown", GFP_KERNEL);
  if (!gdev->label) {
  status = -ENOMEM;
@@ -2662,16 +2670,28 @@ int gpiod_get_array_value_complex(bool raw, bool 
can_sleep,
  while (i < array_size) {
  struct gpio_chip *chip = desc_array[i]->gdev->chip;
-    unsigned long mask[BITS_TO_LONGS(chip->ngpio)];
-    unsigned long bits[BITS_TO_LONGS(chip->ngpio)];
+    unsigned long fastpath[2 * BITS_TO_LONGS(FASTPATH_NGPIO)];
+    unsigned long *mask, *bits;
  int first, j, ret;
+    if (likely(chip->ngpio <= FASTPATH_NGPIO)) {
+    memset(fastpath, 0, sizeof(fastpath));
+    mask = fastpath;
+    bits = fastpath + BITS_TO_LONGS(FASTPATH_NGPIO);

Previously it looks like just mask was zeroed.
So could this just be:
   memset(mask, 0, BITS_TO_LONGS(chip->ngpio));

I'm guessing it's not a huge additional overhead as it is, but it's more in 
line with what was there.



+    } else {
+    mask = kcalloc(2 * BITS_TO_LONGS(chip->ngpio),
+   sizeof(*mask),
+   can_sleep ? GFP_KERNEL : GFP_ATOMIC);
+    if (!mask)
+    return -ENOMEM;
+    bits = mask + BITS_TO_LONGS(chip->ngpio);
+    }
+
  if (!can_sleep)
  WARN_ON(chip->can_sleep);
  /* collect all inputs belonging to the same chip */
  first = i;
-    memset(mask, 0, sizeof(mask));
  do {
  const struct gpio_desc *desc = desc_array[i];
  int hwgpio = gpio_chip_hwgpio(desc);
@@ -2682,8 +2702,11 @@ int gpiod_get_array_value_complex(bool raw, bool 
can_sleep,
   (desc_array[i]->gdev->chip == chip));
  ret = gpio_chip_get_multiple(chip, mask, bits);
-    if (ret)
+    if (ret) {
+    if (mask != fastpath)
+    kfree(mask);
  return ret;
+    }
  for (j = first; j < i; j++) {
  const struct gpio_desc *desc = desc_array[j];
@@ -2695,6 +2718,9 @@ int gpiod_get_array_value_complex(bool raw, bool 
can_sleep,
  value_array[j] = value;
  trace_gpio_value(desc_to_gpio(desc), 1, value);
  }
+
+    if (mask != fastpath)
+    kfree(mas

Re: [PATCHv5] gpio: Remove VLA from gpiolib

[Phil Reid]

On 16/04/2018 13:19, Phil Reid wrote:

G'day Laura,

One more comment.
On 16/04/2018 12:41, Phil Reid wrote:

G'day Laura,

On 14/04/2018 05:24, Laura Abbott wrote:

The new challenge is to remove VLAs from the kernel
(see https://lkml.org/lkml/2018/3/7/621) to eventually
turn on -Wvla.

Using a kmalloc array is the easy way to fix this but kmalloc is still
more expensive than stack allocation. Introduce a fast path with a
fixed size stack array to cover most chip with gpios below some fixed
amount. The slow path dynamically allocates an array to cover those
chips with a large number of gpios.

Reviewed-and-tested-by: Lukas Wunner 
Signed-off-by: Lukas Wunner 
Signed-off-by: Laura Abbott 
---
v5: Dropped some outdated comments and extra whitespace. Switched to
ARCH_NR_GPIOS per suggestion of Linus Walleij.
---
  drivers/gpio/gpiolib.c    | 76 +--
  drivers/gpio/gpiolib.h    |  2 +-
  include/linux/gpio/consumer.h | 10 +++---
  3 files changed, 66 insertions(+), 22 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index d66de67ef307..79ec7a29b684 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -61,6 +61,11 @@ static struct bus_type gpio_bus_type = {
  .name = "gpio",
  };
+/*
+ * Number of GPIOs to use for the fast path in set array
+ */
+#define FASTPATH_NGPIO ARCH_NR_GPIOS


Also wouldn't this mean that fast path will never be triggered now...

Just to be clearer. That this will always be true. (chip->ngpio <= 
FASTPATH_NGPIO)




+
  /* gpio_lock prevents conflicts during gpio_desc[] table updates.
   * While any GPIO is requested, its gpio_chip is not removable;
   * each GPIO's "requested" flag serves as a lock and refcount.
@@ -399,12 +404,11 @@ static long linehandle_ioctl(struct file *filep, unsigned 
int cmd,
  vals[i] = !!ghd.values[i];
  /* Reuse the array setting function */
-    gpiod_set_array_value_complex(false,
+    return gpiod_set_array_value_complex(false,
    true,
    lh->numdescs,
    lh->descs,
    vals);
-    return 0;
  }
  return -EINVAL;
  }
@@ -1192,6 +1196,10 @@ int gpiochip_add_data_with_key(struct gpio_chip *chip, 
void *data,
  goto err_free_descs;
  }
+    if (chip->ngpio > FASTPATH_NGPIO)
+    chip_warn(chip, "line cnt %d is greater than fast path cnt %d\n",
+    chip->ngpio, FASTPATH_NGPIO);
+
  gdev->label = kstrdup_const(chip->label ?: "unknown", GFP_KERNEL);
  if (!gdev->label) {
  status = -ENOMEM;
@@ -2662,16 +2670,28 @@ int gpiod_get_array_value_complex(bool raw, bool 
can_sleep,
  while (i < array_size) {
  struct gpio_chip *chip = desc_array[i]->gdev->chip;
-    unsigned long mask[BITS_TO_LONGS(chip->ngpio)];
-    unsigned long bits[BITS_TO_LONGS(chip->ngpio)];
+    unsigned long fastpath[2 * BITS_TO_LONGS(FASTPATH_NGPIO)];
+    unsigned long *mask, *bits;
  int first, j, ret;
+    if (likely(chip->ngpio <= FASTPATH_NGPIO)) {
+    memset(fastpath, 0, sizeof(fastpath));
+    mask = fastpath;
+    bits = fastpath + BITS_TO_LONGS(FASTPATH_NGPIO);

Previously it looks like just mask was zeroed.
So could this just be:
   memset(mask, 0, BITS_TO_LONGS(chip->ngpio));

I'm guessing it's not a huge additional overhead as it is, but it's more in 
line with what was there.



+    } else {
+    mask = kcalloc(2 * BITS_TO_LONGS(chip->ngpio),
+   sizeof(*mask),
+   can_sleep ? GFP_KERNEL : GFP_ATOMIC);
+    if (!mask)
+    return -ENOMEM;
+    bits = mask + BITS_TO_LONGS(chip->ngpio);
+    }
+
  if (!can_sleep)
  WARN_ON(chip->can_sleep);
  /* collect all inputs belonging to the same chip */
  first = i;
-    memset(mask, 0, sizeof(mask));
  do {
  const struct gpio_desc *desc = desc_array[i];
  int hwgpio = gpio_chip_hwgpio(desc);
@@ -2682,8 +2702,11 @@ int gpiod_get_array_value_complex(bool raw, bool 
can_sleep,
   (desc_array[i]->gdev->chip == chip));
  ret = gpio_chip_get_multiple(chip, mask, bits);
-    if (ret)
+    if (ret) {
+    if (mask != fastpath)
+    kfree(mask);
  return ret;
+    }
  for (j = first; j < i; j++) {
  const struct gpio_desc *desc = desc_array[j];
@@ -2695,6 +2718,9 @@ int gpiod_get_array_value_complex(bool raw, bool 
can_sleep,
  value_array[j] = value;
  trace_gpio_value(desc_to_gpio(desc), 1, value);
  }
+
+    if (mask != fastpath)
+    kfree(mask);
  }
  return 0;
  }
@@ -2878,7 +2904,7 @@ static void gpio_chip_set

Re: [PATCHv5] gpio: Remove VLA from gpiolib

[Phil Reid]

G'day Laura,

One more comment.
On 16/04/2018 12:41, Phil Reid wrote:

G'day Laura,

On 14/04/2018 05:24, Laura Abbott wrote:

The new challenge is to remove VLAs from the kernel
(see https://lkml.org/lkml/2018/3/7/621) to eventually
turn on -Wvla.

Using a kmalloc array is the easy way to fix this but kmalloc is still
more expensive than stack allocation. Introduce a fast path with a
fixed size stack array to cover most chip with gpios below some fixed
amount. The slow path dynamically allocates an array to cover those
chips with a large number of gpios.

Reviewed-and-tested-by: Lukas Wunner <lu...@wunner.de>
Signed-off-by: Lukas Wunner <lu...@wunner.de>
Signed-off-by: Laura Abbott <labb...@redhat.com>
---
v5: Dropped some outdated comments and extra whitespace. Switched to
ARCH_NR_GPIOS per suggestion of Linus Walleij.
---
  drivers/gpio/gpiolib.c    | 76 +--
  drivers/gpio/gpiolib.h    |  2 +-
  include/linux/gpio/consumer.h | 10 +++---
  3 files changed, 66 insertions(+), 22 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index d66de67ef307..79ec7a29b684 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -61,6 +61,11 @@ static struct bus_type gpio_bus_type = {
  .name = "gpio",
  };
+/*
+ * Number of GPIOs to use for the fast path in set array
+ */
+#define FASTPATH_NGPIO ARCH_NR_GPIOS


Also wouldn't this mean that fast path will never be triggered now...



+
  /* gpio_lock prevents conflicts during gpio_desc[] table updates.
   * While any GPIO is requested, its gpio_chip is not removable;
   * each GPIO's "requested" flag serves as a lock and refcount.
@@ -399,12 +404,11 @@ static long linehandle_ioctl(struct file *filep, unsigned 
int cmd,
  vals[i] = !!ghd.values[i];
  /* Reuse the array setting function */
-    gpiod_set_array_value_complex(false,
+    return gpiod_set_array_value_complex(false,
    true,
    lh->numdescs,
    lh->descs,
    vals);
-    return 0;
  }
  return -EINVAL;
  }
@@ -1192,6 +1196,10 @@ int gpiochip_add_data_with_key(struct gpio_chip *chip, 
void *data,
  goto err_free_descs;
  }
+    if (chip->ngpio > FASTPATH_NGPIO)
+    chip_warn(chip, "line cnt %d is greater than fast path cnt %d\n",
+    chip->ngpio, FASTPATH_NGPIO);
+
  gdev->label = kstrdup_const(chip->label ?: "unknown", GFP_KERNEL);
  if (!gdev->label) {
  status = -ENOMEM;
@@ -2662,16 +2670,28 @@ int gpiod_get_array_value_complex(bool raw, bool 
can_sleep,
  while (i < array_size) {
  struct gpio_chip *chip = desc_array[i]->gdev->chip;
-    unsigned long mask[BITS_TO_LONGS(chip->ngpio)];
-    unsigned long bits[BITS_TO_LONGS(chip->ngpio)];
+    unsigned long fastpath[2 * BITS_TO_LONGS(FASTPATH_NGPIO)];
+    unsigned long *mask, *bits;
  int first, j, ret;
+    if (likely(chip->ngpio <= FASTPATH_NGPIO)) {
+    memset(fastpath, 0, sizeof(fastpath));
+    mask = fastpath;
+    bits = fastpath + BITS_TO_LONGS(FASTPATH_NGPIO);

Previously it looks like just mask was zeroed.
So could this just be:
   memset(mask, 0, BITS_TO_LONGS(chip->ngpio));

I'm guessing it's not a huge additional overhead as it is, but it's more in 
line with what was there.



+    } else {
+    mask = kcalloc(2 * BITS_TO_LONGS(chip->ngpio),
+   sizeof(*mask),
+   can_sleep ? GFP_KERNEL : GFP_ATOMIC);
+    if (!mask)
+    return -ENOMEM;
+    bits = mask + BITS_TO_LONGS(chip->ngpio);
+    }
+
  if (!can_sleep)
  WARN_ON(chip->can_sleep);
  /* collect all inputs belonging to the same chip */
  first = i;
-    memset(mask, 0, sizeof(mask));
  do {
  const struct gpio_desc *desc = desc_array[i];
  int hwgpio = gpio_chip_hwgpio(desc);
@@ -2682,8 +2702,11 @@ int gpiod_get_array_value_complex(bool raw, bool 
can_sleep,
   (desc_array[i]->gdev->chip == chip));
  ret = gpio_chip_get_multiple(chip, mask, bits);
-    if (ret)
+    if (ret) {
+    if (mask != fastpath)
+    kfree(mask);
  return ret;
+    }
  for (j = first; j < i; j++) {
  const struct gpio_desc *desc = desc_array[j];
@@ -2695,6 +2718,9 @@ int gpiod_get_array_value_complex(bool raw, bool 
can_sleep,
  value_array[j] = value;
  trace_gpio_value(desc_to_gpio(desc), 1, value);
  }
+
+    if (mask != fastpath)
+    kfree(mask);
  }
  return 0;
  }
@@ -2878,7 +2904,7 @@ static void gpio_chip_set_multiple(struct gpio_chip *chip,
  }
 

Re: [PATCHv5] gpio: Remove VLA from gpiolib

[Phil Reid]

G'day Laura,

One more comment.
On 16/04/2018 12:41, Phil Reid wrote:

G'day Laura,

On 14/04/2018 05:24, Laura Abbott wrote:

The new challenge is to remove VLAs from the kernel
(see https://lkml.org/lkml/2018/3/7/621) to eventually
turn on -Wvla.

Using a kmalloc array is the easy way to fix this but kmalloc is still
more expensive than stack allocation. Introduce a fast path with a
fixed size stack array to cover most chip with gpios below some fixed
amount. The slow path dynamically allocates an array to cover those
chips with a large number of gpios.

Reviewed-and-tested-by: Lukas Wunner 
Signed-off-by: Lukas Wunner 
Signed-off-by: Laura Abbott 
---
v5: Dropped some outdated comments and extra whitespace. Switched to
ARCH_NR_GPIOS per suggestion of Linus Walleij.
---
  drivers/gpio/gpiolib.c    | 76 +--
  drivers/gpio/gpiolib.h    |  2 +-
  include/linux/gpio/consumer.h | 10 +++---
  3 files changed, 66 insertions(+), 22 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index d66de67ef307..79ec7a29b684 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -61,6 +61,11 @@ static struct bus_type gpio_bus_type = {
  .name = "gpio",
  };
+/*
+ * Number of GPIOs to use for the fast path in set array
+ */
+#define FASTPATH_NGPIO ARCH_NR_GPIOS


Also wouldn't this mean that fast path will never be triggered now...



+
  /* gpio_lock prevents conflicts during gpio_desc[] table updates.
   * While any GPIO is requested, its gpio_chip is not removable;
   * each GPIO's "requested" flag serves as a lock and refcount.
@@ -399,12 +404,11 @@ static long linehandle_ioctl(struct file *filep, unsigned 
int cmd,
  vals[i] = !!ghd.values[i];
  /* Reuse the array setting function */
-    gpiod_set_array_value_complex(false,
+    return gpiod_set_array_value_complex(false,
    true,
    lh->numdescs,
    lh->descs,
    vals);
-    return 0;
  }
  return -EINVAL;
  }
@@ -1192,6 +1196,10 @@ int gpiochip_add_data_with_key(struct gpio_chip *chip, 
void *data,
  goto err_free_descs;
  }
+    if (chip->ngpio > FASTPATH_NGPIO)
+    chip_warn(chip, "line cnt %d is greater than fast path cnt %d\n",
+    chip->ngpio, FASTPATH_NGPIO);
+
  gdev->label = kstrdup_const(chip->label ?: "unknown", GFP_KERNEL);
  if (!gdev->label) {
  status = -ENOMEM;
@@ -2662,16 +2670,28 @@ int gpiod_get_array_value_complex(bool raw, bool 
can_sleep,
  while (i < array_size) {
  struct gpio_chip *chip = desc_array[i]->gdev->chip;
-    unsigned long mask[BITS_TO_LONGS(chip->ngpio)];
-    unsigned long bits[BITS_TO_LONGS(chip->ngpio)];
+    unsigned long fastpath[2 * BITS_TO_LONGS(FASTPATH_NGPIO)];
+    unsigned long *mask, *bits;
  int first, j, ret;
+    if (likely(chip->ngpio <= FASTPATH_NGPIO)) {
+    memset(fastpath, 0, sizeof(fastpath));
+    mask = fastpath;
+    bits = fastpath + BITS_TO_LONGS(FASTPATH_NGPIO);

Previously it looks like just mask was zeroed.
So could this just be:
   memset(mask, 0, BITS_TO_LONGS(chip->ngpio));

I'm guessing it's not a huge additional overhead as it is, but it's more in 
line with what was there.



+    } else {
+    mask = kcalloc(2 * BITS_TO_LONGS(chip->ngpio),
+   sizeof(*mask),
+   can_sleep ? GFP_KERNEL : GFP_ATOMIC);
+    if (!mask)
+    return -ENOMEM;
+    bits = mask + BITS_TO_LONGS(chip->ngpio);
+    }
+
  if (!can_sleep)
  WARN_ON(chip->can_sleep);
  /* collect all inputs belonging to the same chip */
  first = i;
-    memset(mask, 0, sizeof(mask));
  do {
  const struct gpio_desc *desc = desc_array[i];
  int hwgpio = gpio_chip_hwgpio(desc);
@@ -2682,8 +2702,11 @@ int gpiod_get_array_value_complex(bool raw, bool 
can_sleep,
   (desc_array[i]->gdev->chip == chip));
  ret = gpio_chip_get_multiple(chip, mask, bits);
-    if (ret)
+    if (ret) {
+    if (mask != fastpath)
+    kfree(mask);
  return ret;
+    }
  for (j = first; j < i; j++) {
  const struct gpio_desc *desc = desc_array[j];
@@ -2695,6 +2718,9 @@ int gpiod_get_array_value_complex(bool raw, bool 
can_sleep,
  value_array[j] = value;
  trace_gpio_value(desc_to_gpio(desc), 1, value);
  }
+
+    if (mask != fastpath)
+    kfree(mask);
  }
  return 0;
  }
@@ -2878,7 +2904,7 @@ static void gpio_chip_set_multiple(struct gpio_chip *chip,
  }
  }
-void gpiod_set_array_value_complex(bool raw, bool can_sleep,

Re: [PATCHv5] gpio: Remove VLA from gpiolib

[Phil Reid]

  struct gpio_desc **desc_array,
  int *value_array);
  void gpiod_set_raw_value(struct gpio_desc *desc, int value);
-void gpiod_set_raw_array_value(unsigned int array_size,
+int gpiod_set_raw_array_value(unsigned int array_size,
   struct gpio_desc **desc_array,
   int *value_array);
  
@@ -134,7 +134,7 @@ int gpiod_get_raw_array_value_cansleep(unsigned int array_size,

   struct gpio_desc **desc_array,
   int *value_array);
  void gpiod_set_raw_value_cansleep(struct gpio_desc *desc, int value);
-void gpiod_set_raw_array_value_cansleep(unsigned int array_size,
+int gpiod_set_raw_array_value_cansleep(unsigned int array_size,
struct gpio_desc **desc_array,
int *value_array);
  
@@ -369,12 +369,13 @@ static inline void gpiod_set_raw_value(struct gpio_desc *desc, int value)

/* GPIO can never have been requested */
WARN_ON(1);
  }
-static inline void gpiod_set_raw_array_value(unsigned int array_size,
+static inline int gpiod_set_raw_array_value(unsigned int array_size,
 struct gpio_desc **desc_array,
 int *value_array)
  {
/* GPIO can never have been requested */
WARN_ON(1);
+   return 0;
  }
  
  static inline int gpiod_get_value_cansleep(const struct gpio_desc *desc)

@@ -423,12 +424,13 @@ static inline void gpiod_set_raw_value_cansleep(struct 
gpio_desc *desc,
/* GPIO can never have been requested */
WARN_ON(1);
  }
-static inline void gpiod_set_raw_array_value_cansleep(unsigned int array_size,
+static inline int gpiod_set_raw_array_value_cansleep(unsigned int array_size,
struct gpio_desc **desc_array,
int *value_array)
  {
/* GPIO can never have been requested */
WARN_ON(1);
+   return 0;
  }
  
  static inline int gpiod_set_debounce(struct gpio_desc *desc, unsigned debounce)





--
Regards
Phil Reid

ElectroMagnetic Imaging Technology Pty Ltd
Development of Geophysical Instrumentation & Software
www.electromag.com.au

3 The Avenue, Midland WA 6056, AUSTRALIA
Ph: +61 8 9250 8100
Fax: +61 8 9250 7100
Email: pr...@electromag.com.au

Re: [PATCHv5] gpio: Remove VLA from gpiolib

[Phil Reid]

lue_array);
  void gpiod_set_raw_value(struct gpio_desc *desc, int value);
-void gpiod_set_raw_array_value(unsigned int array_size,
+int gpiod_set_raw_array_value(unsigned int array_size,
   struct gpio_desc **desc_array,
   int *value_array);
  
@@ -134,7 +134,7 @@ int gpiod_get_raw_array_value_cansleep(unsigned int array_size,

   struct gpio_desc **desc_array,
   int *value_array);
  void gpiod_set_raw_value_cansleep(struct gpio_desc *desc, int value);
-void gpiod_set_raw_array_value_cansleep(unsigned int array_size,
+int gpiod_set_raw_array_value_cansleep(unsigned int array_size,
struct gpio_desc **desc_array,
int *value_array);
  
@@ -369,12 +369,13 @@ static inline void gpiod_set_raw_value(struct gpio_desc *desc, int value)

/* GPIO can never have been requested */
WARN_ON(1);
  }
-static inline void gpiod_set_raw_array_value(unsigned int array_size,
+static inline int gpiod_set_raw_array_value(unsigned int array_size,
 struct gpio_desc **desc_array,
 int *value_array)
  {
/* GPIO can never have been requested */
WARN_ON(1);
+   return 0;
  }
  
  static inline int gpiod_get_value_cansleep(const struct gpio_desc *desc)

@@ -423,12 +424,13 @@ static inline void gpiod_set_raw_value_cansleep(struct 
gpio_desc *desc,
/* GPIO can never have been requested */
WARN_ON(1);
  }
-static inline void gpiod_set_raw_array_value_cansleep(unsigned int array_size,
+static inline int gpiod_set_raw_array_value_cansleep(unsigned int array_size,
struct gpio_desc **desc_array,
int *value_array)
  {
/* GPIO can never have been requested */
    WARN_ON(1);
+   return 0;
  }
  
  static inline int gpiod_set_debounce(struct gpio_desc *desc, unsigned debounce)





--
Regards
Phil Reid

ElectroMagnetic Imaging Technology Pty Ltd
Development of Geophysical Instrumentation & Software
www.electromag.com.au

3 The Avenue, Midland WA 6056, AUSTRALIA
Ph: +61 8 9250 8100
Fax: +61 8 9250 7100
Email: pr...@electromag.com.au

Re: [PATCHv4] gpio: Remove VLA from gpiolib

[Phil Reid]

On 14/04/2018 05:10, Laura Abbott wrote:

On 04/12/2018 05:39 PM, Phil Reid wrote:

On 12/04/2018 16:38, Linus Walleij wrote:

On Wed, Apr 11, 2018 at 3:03 AM, Laura Abbott <labb...@redhat.com> wrote:


The new challenge is to remove VLAs from the kernel
(see https://lkml.org/lkml/2018/3/7/621) to eventually
turn on -Wvla.

Using a kmalloc array is the easy way to fix this but kmalloc is still
more expensive than stack allocation. Introduce a fast path with a
fixed size stack array to cover most chip with gpios below some fixed
amount. The slow path dynamically allocates an array to cover those
chips with a large number of gpios.

Reviewed-and-tested-by: Lukas Wunner <lu...@wunner.de>
Signed-off-by: Lukas Wunner <lu...@wunner.de>
Signed-off-by: Laura Abbott <labb...@redhat.com>
---
v4: Changed some local variables to avoid coccinelle warnings. Added a
warning if the number of GPIOs exceeds the current fast path define.

Lukas, I kept your Tested-by because the changes were pretty minimal.
Let me know if you want to run the tests again.


This patch is starting to look really good.


+/*
+ * Number of GPIOs to use for the fast path in set array
+ */
+#define FASTPATH_NGPIO 256


There is still some comment about this.

And now that I am also tryint to think I wonder about it, we
have a global ARCH_NR_GPIOS that is typically 512.
Some archs set it up.

This define is something of an abomination, in the ARM
case it comes from arch/arm/include/asm/gpio.h
where #define ARCH_NR_GPIOS CONFIG_ARCH_NR_GPIO
where the latter is a Kconfig option that is mostly 512 for
most ARM systems.

Well, ARM looks like this:

config ARCH_NR_GPIO
 int
 default 2048 if ARCH_SOCFPGA
 default 1024 if ARCH_BRCMSTB || ARCH_SHMOBILE || ARCH_TEGRA || \
 ARCH_ZYNQ
 default 512 if ARCH_EXYNOS || ARCH_KEYSTONE || SOC_OMAP5 || \
 SOC_DRA7XX || ARCH_S3C24XX || ARCH_S3C64XX || ARCH_S5PV210
 default 416 if ARCH_SUNXI
 default 392 if ARCH_U8500
 default 352 if ARCH_VT8500
 default 288 if ARCH_ROCKCHIP
 default 264 if MACH_H4700
 default 0
 help
   Maximum number of GPIOs in the system.

   If unsure, leave the default value.

So if FASTPATH_NGPIO should be anything else than
ARCH_NR_GPIO this has to be established somewhere
as a floor or half or something, but I would just set it as
the same as ARCH_NR_GPIOS...

The main reason this define exist is for this function
from :

/* Convert between the old gpio_ and new gpiod_ interfaces */
struct gpio_desc *gpio_to_desc(unsigned gpio);

Nowadays that fact is a bit obscured since the variable is
only used when assigning the base (in the global GPIO
number space, which is what we want to get rid of but
sigh) in gpiochip_find_base() where it attempts to place
a newly allocated gpiochip in the higher region of this
numberspace since the embedded SoC GPIO base tends
to be 0, on old platforms.

So I don't know about this.

Can't we just use ARCH_NR_GPIOS?

Very few systems have more than 512 assigned global
GPIO numbers and those are FPGA experimental machines.

In the long run obviously I want to get rid of these defines
altogether and only allocate GPIO descriptos dynamically
so as you see I am reluctant to add new numberspace weirdness
around here.

Isn't that for total GPIO's in the system?
And the arrays just need to cater for max per chip?
 From what I can understand of the code which is admittedly limited.




Yeah the switch back to 256 was a mistake on my end (I think I
grabbed an incorrect version for my base). ARCH_NR_GPIOs
is the total number in the system which may be multiple
chips so yes we would be possibly allocating more space
than necessary.

unsigned long fastpath[2 * BITS_TO_LONGS(FASTPATH_NGPIO)]

unsigned long fastpath[2 * BITS_TO_LONGS(512)]
unsigned long fastpath[2 * DIV_ROUND_UP(512, 8 * sizeof(long))]

so we end up with 128 bytes on the stack total assuming I
can do math correctly. I think this a fairly reasonable
amount though, even if we are over-estimating if there are
multiple chips.



Yeah that's not too bad.
My system is a SOCFPGA so it'd be 2048 / 8 = 512.
Still not unreasonable.

But the system doesn't have a single gpio close to that.
The largest chip is 32.


--
Regards
Phil Reid

Re: [PATCHv4] gpio: Remove VLA from gpiolib

[Phil Reid]

On 14/04/2018 05:10, Laura Abbott wrote:

On 04/12/2018 05:39 PM, Phil Reid wrote:

On 12/04/2018 16:38, Linus Walleij wrote:

On Wed, Apr 11, 2018 at 3:03 AM, Laura Abbott  wrote:


The new challenge is to remove VLAs from the kernel
(see https://lkml.org/lkml/2018/3/7/621) to eventually
turn on -Wvla.

Using a kmalloc array is the easy way to fix this but kmalloc is still
more expensive than stack allocation. Introduce a fast path with a
fixed size stack array to cover most chip with gpios below some fixed
amount. The slow path dynamically allocates an array to cover those
chips with a large number of gpios.

Reviewed-and-tested-by: Lukas Wunner 
Signed-off-by: Lukas Wunner 
Signed-off-by: Laura Abbott 
---
v4: Changed some local variables to avoid coccinelle warnings. Added a
warning if the number of GPIOs exceeds the current fast path define.

Lukas, I kept your Tested-by because the changes were pretty minimal.
Let me know if you want to run the tests again.


This patch is starting to look really good.


+/*
+ * Number of GPIOs to use for the fast path in set array
+ */
+#define FASTPATH_NGPIO 256


There is still some comment about this.

And now that I am also tryint to think I wonder about it, we
have a global ARCH_NR_GPIOS that is typically 512.
Some archs set it up.

This define is something of an abomination, in the ARM
case it comes from arch/arm/include/asm/gpio.h
where #define ARCH_NR_GPIOS CONFIG_ARCH_NR_GPIO
where the latter is a Kconfig option that is mostly 512 for
most ARM systems.

Well, ARM looks like this:

config ARCH_NR_GPIO
 int
 default 2048 if ARCH_SOCFPGA
 default 1024 if ARCH_BRCMSTB || ARCH_SHMOBILE || ARCH_TEGRA || \
 ARCH_ZYNQ
 default 512 if ARCH_EXYNOS || ARCH_KEYSTONE || SOC_OMAP5 || \
 SOC_DRA7XX || ARCH_S3C24XX || ARCH_S3C64XX || ARCH_S5PV210
 default 416 if ARCH_SUNXI
 default 392 if ARCH_U8500
 default 352 if ARCH_VT8500
 default 288 if ARCH_ROCKCHIP
 default 264 if MACH_H4700
 default 0
 help
   Maximum number of GPIOs in the system.

   If unsure, leave the default value.

So if FASTPATH_NGPIO should be anything else than
ARCH_NR_GPIO this has to be established somewhere
as a floor or half or something, but I would just set it as
the same as ARCH_NR_GPIOS...

The main reason this define exist is for this function
from :

/* Convert between the old gpio_ and new gpiod_ interfaces */
struct gpio_desc *gpio_to_desc(unsigned gpio);

Nowadays that fact is a bit obscured since the variable is
only used when assigning the base (in the global GPIO
number space, which is what we want to get rid of but
sigh) in gpiochip_find_base() where it attempts to place
a newly allocated gpiochip in the higher region of this
numberspace since the embedded SoC GPIO base tends
to be 0, on old platforms.

So I don't know about this.

Can't we just use ARCH_NR_GPIOS?

Very few systems have more than 512 assigned global
GPIO numbers and those are FPGA experimental machines.

In the long run obviously I want to get rid of these defines
altogether and only allocate GPIO descriptos dynamically
so as you see I am reluctant to add new numberspace weirdness
around here.

Isn't that for total GPIO's in the system?
And the arrays just need to cater for max per chip?
 From what I can understand of the code which is admittedly limited.




Yeah the switch back to 256 was a mistake on my end (I think I
grabbed an incorrect version for my base). ARCH_NR_GPIOs
is the total number in the system which may be multiple
chips so yes we would be possibly allocating more space
than necessary.

unsigned long fastpath[2 * BITS_TO_LONGS(FASTPATH_NGPIO)]

unsigned long fastpath[2 * BITS_TO_LONGS(512)]
unsigned long fastpath[2 * DIV_ROUND_UP(512, 8 * sizeof(long))]

so we end up with 128 bytes on the stack total assuming I
can do math correctly. I think this a fairly reasonable
amount though, even if we are over-estimating if there are
multiple chips.



Yeah that's not too bad.
My system is a SOCFPGA so it'd be 2048 / 8 = 512.
Still not unreasonable.

But the system doesn't have a single gpio close to that.
The largest chip is 32.


--
Regards
Phil Reid

Re: [PATCHv4] gpio: Remove VLA from gpiolib

[Phil Reid]

On 12/04/2018 16:38, Linus Walleij wrote:

On Wed, Apr 11, 2018 at 3:03 AM, Laura Abbott <labb...@redhat.com> wrote:


The new challenge is to remove VLAs from the kernel
(see https://lkml.org/lkml/2018/3/7/621) to eventually
turn on -Wvla.

Using a kmalloc array is the easy way to fix this but kmalloc is still
more expensive than stack allocation. Introduce a fast path with a
fixed size stack array to cover most chip with gpios below some fixed
amount. The slow path dynamically allocates an array to cover those
chips with a large number of gpios.

Reviewed-and-tested-by: Lukas Wunner <lu...@wunner.de>
Signed-off-by: Lukas Wunner <lu...@wunner.de>
Signed-off-by: Laura Abbott <labb...@redhat.com>
---
v4: Changed some local variables to avoid coccinelle warnings. Added a
warning if the number of GPIOs exceeds the current fast path define.

Lukas, I kept your Tested-by because the changes were pretty minimal.
Let me know if you want to run the tests again.


This patch is starting to look really good.


+/*
+ * Number of GPIOs to use for the fast path in set array
+ */
+#define FASTPATH_NGPIO 256


There is still some comment about this.

And now that I am also tryint to think I wonder about it, we
have a global ARCH_NR_GPIOS that is typically 512.
Some archs set it up.

This define is something of an abomination, in the ARM
case it comes from arch/arm/include/asm/gpio.h
where #define ARCH_NR_GPIOS CONFIG_ARCH_NR_GPIO
where the latter is a Kconfig option that is mostly 512 for
most ARM systems.

Well, ARM looks like this:

config ARCH_NR_GPIO
 int
 default 2048 if ARCH_SOCFPGA
 default 1024 if ARCH_BRCMSTB || ARCH_SHMOBILE || ARCH_TEGRA || \
 ARCH_ZYNQ
 default 512 if ARCH_EXYNOS || ARCH_KEYSTONE || SOC_OMAP5 || \
 SOC_DRA7XX || ARCH_S3C24XX || ARCH_S3C64XX || ARCH_S5PV210
 default 416 if ARCH_SUNXI
 default 392 if ARCH_U8500
 default 352 if ARCH_VT8500
 default 288 if ARCH_ROCKCHIP
 default 264 if MACH_H4700
 default 0
 help
   Maximum number of GPIOs in the system.

   If unsure, leave the default value.

So if FASTPATH_NGPIO should be anything else than
ARCH_NR_GPIO this has to be established somewhere
as a floor or half or something, but I would just set it as
the same as ARCH_NR_GPIOS...

The main reason this define exist is for this function
from :

/* Convert between the old gpio_ and new gpiod_ interfaces */
struct gpio_desc *gpio_to_desc(unsigned gpio);

Nowadays that fact is a bit obscured since the variable is
only used when assigning the base (in the global GPIO
number space, which is what we want to get rid of but
sigh) in gpiochip_find_base() where it attempts to place
a newly allocated gpiochip in the higher region of this
numberspace since the embedded SoC GPIO base tends
to be 0, on old platforms.

So I don't know about this.

Can't we just use ARCH_NR_GPIOS?

Very few systems have more than 512 assigned global
GPIO numbers and those are FPGA experimental machines.

In the long run obviously I want to get rid of these defines
altogether and only allocate GPIO descriptos dynamically
so as you see I am reluctant to add new numberspace weirdness
around here.

Isn't that for total GPIO's in the system?
And the arrays just need to cater for max per chip?
From what I can understand of the code which is admittedly limited.


--
Regards
Phil Reid

Re: [PATCHv4] gpio: Remove VLA from gpiolib

[Phil Reid]

On 12/04/2018 16:38, Linus Walleij wrote:

On Wed, Apr 11, 2018 at 3:03 AM, Laura Abbott  wrote:


The new challenge is to remove VLAs from the kernel
(see https://lkml.org/lkml/2018/3/7/621) to eventually
turn on -Wvla.

Using a kmalloc array is the easy way to fix this but kmalloc is still
more expensive than stack allocation. Introduce a fast path with a
fixed size stack array to cover most chip with gpios below some fixed
amount. The slow path dynamically allocates an array to cover those
chips with a large number of gpios.

Reviewed-and-tested-by: Lukas Wunner 
Signed-off-by: Lukas Wunner 
Signed-off-by: Laura Abbott 
---
v4: Changed some local variables to avoid coccinelle warnings. Added a
warning if the number of GPIOs exceeds the current fast path define.

Lukas, I kept your Tested-by because the changes were pretty minimal.
Let me know if you want to run the tests again.


This patch is starting to look really good.


+/*
+ * Number of GPIOs to use for the fast path in set array
+ */
+#define FASTPATH_NGPIO 256


There is still some comment about this.

And now that I am also tryint to think I wonder about it, we
have a global ARCH_NR_GPIOS that is typically 512.
Some archs set it up.

This define is something of an abomination, in the ARM
case it comes from arch/arm/include/asm/gpio.h
where #define ARCH_NR_GPIOS CONFIG_ARCH_NR_GPIO
where the latter is a Kconfig option that is mostly 512 for
most ARM systems.

Well, ARM looks like this:

config ARCH_NR_GPIO
 int
 default 2048 if ARCH_SOCFPGA
 default 1024 if ARCH_BRCMSTB || ARCH_SHMOBILE || ARCH_TEGRA || \
 ARCH_ZYNQ
 default 512 if ARCH_EXYNOS || ARCH_KEYSTONE || SOC_OMAP5 || \
 SOC_DRA7XX || ARCH_S3C24XX || ARCH_S3C64XX || ARCH_S5PV210
 default 416 if ARCH_SUNXI
 default 392 if ARCH_U8500
 default 352 if ARCH_VT8500
 default 288 if ARCH_ROCKCHIP
 default 264 if MACH_H4700
 default 0
 help
   Maximum number of GPIOs in the system.

   If unsure, leave the default value.

So if FASTPATH_NGPIO should be anything else than
ARCH_NR_GPIO this has to be established somewhere
as a floor or half or something, but I would just set it as
the same as ARCH_NR_GPIOS...

The main reason this define exist is for this function
from :

/* Convert between the old gpio_ and new gpiod_ interfaces */
struct gpio_desc *gpio_to_desc(unsigned gpio);

Nowadays that fact is a bit obscured since the variable is
only used when assigning the base (in the global GPIO
number space, which is what we want to get rid of but
sigh) in gpiochip_find_base() where it attempts to place
a newly allocated gpiochip in the higher region of this
numberspace since the embedded SoC GPIO base tends
to be 0, on old platforms.

So I don't know about this.

Can't we just use ARCH_NR_GPIOS?

Very few systems have more than 512 assigned global
GPIO numbers and those are FPGA experimental machines.

In the long run obviously I want to get rid of these defines
altogether and only allocate GPIO descriptos dynamically
so as you see I am reluctant to add new numberspace weirdness
around here.

Isn't that for total GPIO's in the system?
And the arrays just need to cater for max per chip?
From what I can understand of the code which is admittedly limited.


--
Regards
Phil Reid

Re: [PATCH v2] net: dsa: b53: Using sleep-able operations in b53_switch_reset_gpio

[Phil Reid]

On 12/04/2018 09:48, Jia-Ju Bai wrote:

b53_switch_reset_gpio() is never called in atomic context.

The call chain ending up at b53_switch_reset_gpio() is:
[1] b53_switch_reset_gpio() <- b53_switch_reset() <-
 b53_reset_switch() <- b53_setup()

b53_switch_reset_gpio() is set as ".setup" in struct dsa_switch_ops.
This function is not called in atomic context.

Despite never getting called from atomic context, b53_switch_reset_gpio()
calls non-sleep operations mdelay() and gpio_set_value().
They are not necessary and can be replaced with msleep()
and gpio_set_value_cansleep().

This is found by a static analysis tool named DCNS written by myself.
And I also manually check it.

Signed-off-by: Jia-Ju Bai <baijiaju1...@gmail.com>
---
v2:
* Use gpio_set_value_cansleep() to replace gpio_set_value() additionally.
   Thanks for Florian and Phil for good advice.
---
  drivers/net/dsa/b53/b53_common.c | 8 
  1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/net/dsa/b53/b53_common.c b/drivers/net/dsa/b53/b53_common.c
index 274f367..36cc60d 100644
--- a/drivers/net/dsa/b53/b53_common.c
+++ b/drivers/net/dsa/b53/b53_common.c
@@ -596,11 +596,11 @@ static void b53_switch_reset_gpio(struct b53_device *dev)
  
  	/* Reset sequence: RESET low(50ms)->high(20ms)

 */
-   gpio_set_value(gpio, 0);
-   mdelay(50);
+   gpio_set_value_cansleep(gpio, 0);
+   msleep(50);
  
-	gpio_set_value(gpio, 1);

-   mdelay(20);
+   gpio_set_value_cansleep(gpio, 1);
+   msleep(20);
  
  	dev->current_page = 0xff;

  }



FWIW:
Reviewed-by: Phil Reid <pr...@electromag.com.au>

Re: [PATCH v2] net: dsa: b53: Using sleep-able operations in b53_switch_reset_gpio

[Phil Reid]

On 12/04/2018 09:48, Jia-Ju Bai wrote:

b53_switch_reset_gpio() is never called in atomic context.

The call chain ending up at b53_switch_reset_gpio() is:
[1] b53_switch_reset_gpio() <- b53_switch_reset() <-
 b53_reset_switch() <- b53_setup()

b53_switch_reset_gpio() is set as ".setup" in struct dsa_switch_ops.
This function is not called in atomic context.

Despite never getting called from atomic context, b53_switch_reset_gpio()
calls non-sleep operations mdelay() and gpio_set_value().
They are not necessary and can be replaced with msleep()
and gpio_set_value_cansleep().

This is found by a static analysis tool named DCNS written by myself.
And I also manually check it.

Signed-off-by: Jia-Ju Bai 
---
v2:
* Use gpio_set_value_cansleep() to replace gpio_set_value() additionally.
   Thanks for Florian and Phil for good advice.
---
  drivers/net/dsa/b53/b53_common.c | 8 
  1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/net/dsa/b53/b53_common.c b/drivers/net/dsa/b53/b53_common.c
index 274f367..36cc60d 100644
--- a/drivers/net/dsa/b53/b53_common.c
+++ b/drivers/net/dsa/b53/b53_common.c
@@ -596,11 +596,11 @@ static void b53_switch_reset_gpio(struct b53_device *dev)
  
  	/* Reset sequence: RESET low(50ms)->high(20ms)

 */
-   gpio_set_value(gpio, 0);
-   mdelay(50);
+   gpio_set_value_cansleep(gpio, 0);
+   msleep(50);
  
-	gpio_set_value(gpio, 1);

-   mdelay(20);
+   gpio_set_value_cansleep(gpio, 1);
+   msleep(20);
  
  	dev->current_page = 0xff;

  }



FWIW:
Reviewed-by: Phil Reid 

Re: [PATCH] net: dsa: b53: Replace mdelay with msleep in b53_switch_reset_gpio

[Phil Reid]

On 11/04/2018 09:51, Jia-Ju Bai wrote:

b53_switch_reset_gpio() is never called in atomic context.

The call chain ending up at b53_switch_reset_gpio() is:
[1] b53_switch_reset_gpio() <- b53_switch_reset() <-
b53_reset_switch() <- b53_setup()

b53_switch_reset_gpio() is set as ".setup" in struct dsa_switch_ops.
This function is not called in atomic context.

Despite never getting called from atomic context, b53_switch_reset_gpio()
calls mdelay() to busily wait.
This is not necessary and can be replaced with msleep() to
avoid busy waiting.

This is found by a static analysis tool named DCNS written by myself.
And I also manually check it.

Signed-off-by: Jia-Ju Bai <baijiaju1...@gmail.com>
---
  drivers/net/dsa/b53/b53_common.c | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/dsa/b53/b53_common.c b/drivers/net/dsa/b53/b53_common.c
index 274f367..e070ff6 100644
--- a/drivers/net/dsa/b53/b53_common.c
+++ b/drivers/net/dsa/b53/b53_common.c
@@ -597,10 +597,10 @@ static void b53_switch_reset_gpio(struct b53_device *dev)
/* Reset sequence: RESET low(50ms)->high(20ms)
 */
gpio_set_value(gpio, 0);
-   mdelay(50);
+   msleep(50);
  
  	gpio_set_value(gpio, 1);

-   mdelay(20);
+   msleep(20);
  
  	dev->current_page = 0xff;

  }


Would that also imply gpio_set_value could be gpio_set_value_cansleep?


--
Regards
Phil Reid

Re: [PATCH] net: dsa: b53: Replace mdelay with msleep in b53_switch_reset_gpio

[Phil Reid]

On 11/04/2018 09:51, Jia-Ju Bai wrote:

b53_switch_reset_gpio() is never called in atomic context.

The call chain ending up at b53_switch_reset_gpio() is:
[1] b53_switch_reset_gpio() <- b53_switch_reset() <-
b53_reset_switch() <- b53_setup()

b53_switch_reset_gpio() is set as ".setup" in struct dsa_switch_ops.
This function is not called in atomic context.

Despite never getting called from atomic context, b53_switch_reset_gpio()
calls mdelay() to busily wait.
This is not necessary and can be replaced with msleep() to
avoid busy waiting.

This is found by a static analysis tool named DCNS written by myself.
And I also manually check it.

Signed-off-by: Jia-Ju Bai 
---
  drivers/net/dsa/b53/b53_common.c | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/dsa/b53/b53_common.c b/drivers/net/dsa/b53/b53_common.c
index 274f367..e070ff6 100644
--- a/drivers/net/dsa/b53/b53_common.c
+++ b/drivers/net/dsa/b53/b53_common.c
@@ -597,10 +597,10 @@ static void b53_switch_reset_gpio(struct b53_device *dev)
/* Reset sequence: RESET low(50ms)->high(20ms)
 */
gpio_set_value(gpio, 0);
-   mdelay(50);
+   msleep(50);
  
  	gpio_set_value(gpio, 1);

-   mdelay(20);
+   msleep(20);
  
  	dev->current_page = 0xff;

  }


Would that also imply gpio_set_value could be gpio_set_value_cansleep?


--
Regards
Phil Reid

Re: [PATCH v3] gpio: Remove VLA from stmpe driver

[Phil Reid]

On 29/03/2018 01:59, Laura Abbott wrote:

The new challenge is to remove VLAs from the kernel
(see https://lkml.org/lkml/2018/3/7/621)

The number of GPIOs on the supported chips is fairly small
so stack allocate to a known upper bound and spit out a warning
if any new chips have more gpios.

Signed-off-by: Laura Abbott <labb...@redhat.com>
---
v3: Split this off from the rest of the series since some of the
patches had been picked up. Switched to just hardcoding an upper
bound for the stack array since it's only a few extra bytes
of stack space.
---
  drivers/gpio/gpio-stmpe.c | 9 -
  1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/gpio/gpio-stmpe.c b/drivers/gpio/gpio-stmpe.c
index f8d7d1cd8488..8d6a5a7e612d 100644
--- a/drivers/gpio/gpio-stmpe.c
+++ b/drivers/gpio/gpio-stmpe.c
@@ -363,13 +363,15 @@ static struct irq_chip stmpe_gpio_irq_chip = {
.irq_set_type   = stmpe_gpio_irq_set_type,
  };
  
+#define MAX_GPIOS 24

+
  static irqreturn_t stmpe_gpio_irq(int irq, void *dev)
  {
struct stmpe_gpio *stmpe_gpio = dev;
struct stmpe *stmpe = stmpe_gpio->stmpe;
u8 statmsbreg;
int num_banks = DIV_ROUND_UP(stmpe->num_gpios, 8);
-   u8 status[num_banks];
+   u8 status[DIV_ROUND_UP(MAX_GPIOS, 8)];
int ret;
int i;
  
@@ -434,6 +436,11 @@ static int stmpe_gpio_probe(struct platform_device *pdev)

struct stmpe_gpio *stmpe_gpio;
int ret, irq;
  
+	if (stmpe->num_gpios > MAX_GPIOS) {

+   dev_err(>dev, "Need to increase maximum GPIO number\n");
+   return -EINVAL;
+   }
+
stmpe_gpio = kzalloc(sizeof(*stmpe_gpio), GFP_KERNEL);
if (!stmpe_gpio)
return -ENOMEM;


FWIW
Reviewed-by: Phil Reid <pr...@electromag.com.au>

--
Regards
Phil Reid

Re: [PATCH v3] gpio: Remove VLA from stmpe driver

[Phil Reid]

On 29/03/2018 01:59, Laura Abbott wrote:

The new challenge is to remove VLAs from the kernel
(see https://lkml.org/lkml/2018/3/7/621)

The number of GPIOs on the supported chips is fairly small
so stack allocate to a known upper bound and spit out a warning
if any new chips have more gpios.

Signed-off-by: Laura Abbott 
---
v3: Split this off from the rest of the series since some of the
patches had been picked up. Switched to just hardcoding an upper
bound for the stack array since it's only a few extra bytes
of stack space.
---
  drivers/gpio/gpio-stmpe.c | 9 -
  1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/gpio/gpio-stmpe.c b/drivers/gpio/gpio-stmpe.c
index f8d7d1cd8488..8d6a5a7e612d 100644
--- a/drivers/gpio/gpio-stmpe.c
+++ b/drivers/gpio/gpio-stmpe.c
@@ -363,13 +363,15 @@ static struct irq_chip stmpe_gpio_irq_chip = {
.irq_set_type   = stmpe_gpio_irq_set_type,
  };
  
+#define MAX_GPIOS 24

+
  static irqreturn_t stmpe_gpio_irq(int irq, void *dev)
  {
struct stmpe_gpio *stmpe_gpio = dev;
struct stmpe *stmpe = stmpe_gpio->stmpe;
u8 statmsbreg;
int num_banks = DIV_ROUND_UP(stmpe->num_gpios, 8);
-   u8 status[num_banks];
+   u8 status[DIV_ROUND_UP(MAX_GPIOS, 8)];
int ret;
int i;
  
@@ -434,6 +436,11 @@ static int stmpe_gpio_probe(struct platform_device *pdev)

struct stmpe_gpio *stmpe_gpio;
int ret, irq;
  
+	if (stmpe->num_gpios > MAX_GPIOS) {

+   dev_err(>dev, "Need to increase maximum GPIO number\n");
+   return -EINVAL;
+   }
+
stmpe_gpio = kzalloc(sizeof(*stmpe_gpio), GFP_KERNEL);
if (!stmpe_gpio)
return -ENOMEM;


FWIW
Reviewed-by: Phil Reid 

--
Regards
Phil Reid

Re: [PATCH 2/3] dt-bindings: iio: wrapper: add io-channel-unit-converter

[Phil Reid]

On 27/03/2018 16:01, Peter Rosin wrote:

On 2018-03-27 00:23, Rob Herring wrote:

On Mon, Mar 19, 2018 at 06:02:45PM +0100, Peter Rosin wrote:

Allow linear scaling and modification of the type of an io-channel.

When an ADC channel measures the midpoint of a voltage divider, the
interesting voltage is often the voltage over the full resistance
of the divider. Likewise, measuring the voltage over a resistor is
often a way to get to the current through it.

This binding allows description of such hardware which is external
to the ADC.



*snip*


+++ 
b/Documentation/devicetree/bindings/iio/wrapper/io-channel-unit-converter.txt
@@ -0,0 +1,84 @@
+I/O channel unit converter bindings
+
+Allow linear scaling and modification of the type of an io-channel.
+
+When an ADC channel measures the midpoint of a voltage divider, the
+interesting voltage is often the voltage over the full resistance
+of the divider. Likewise, measuring the voltage over a resistor is
+often a way to get to the current through it.
+
+Required properties:
+- compatible : "io-channel-unit-converter"

Would this apply to something besides ADCs?

Not that I can think of. At the moment.



I like the concept. I can think of use case on my end to set a RADC (digital 
pot)
to set a threshold voltage. Being able to define the hardware scaling in the dt 
would be nice.
Which would allow all the hardware definition to be in the dt which would nice.
So this would be voltage -> resistance.

Setting a DAC voltage to set output current is also a distinct possibility.


--
Regards
Phil Reid

Re: [PATCH 2/3] dt-bindings: iio: wrapper: add io-channel-unit-converter

[Phil Reid]

On 27/03/2018 16:01, Peter Rosin wrote:

On 2018-03-27 00:23, Rob Herring wrote:

On Mon, Mar 19, 2018 at 06:02:45PM +0100, Peter Rosin wrote:

Allow linear scaling and modification of the type of an io-channel.

When an ADC channel measures the midpoint of a voltage divider, the
interesting voltage is often the voltage over the full resistance
of the divider. Likewise, measuring the voltage over a resistor is
often a way to get to the current through it.

This binding allows description of such hardware which is external
to the ADC.



*snip*


+++ 
b/Documentation/devicetree/bindings/iio/wrapper/io-channel-unit-converter.txt
@@ -0,0 +1,84 @@
+I/O channel unit converter bindings
+
+Allow linear scaling and modification of the type of an io-channel.
+
+When an ADC channel measures the midpoint of a voltage divider, the
+interesting voltage is often the voltage over the full resistance
+of the divider. Likewise, measuring the voltage over a resistor is
+often a way to get to the current through it.
+
+Required properties:
+- compatible : "io-channel-unit-converter"

Would this apply to something besides ADCs?

Not that I can think of. At the moment.



I like the concept. I can think of use case on my end to set a RADC (digital 
pot)
to set a threshold voltage. Being able to define the hardware scaling in the dt 
would be nice.
Which would allow all the hardware definition to be in the dt which would nice.
So this would be voltage -> resistance.

Setting a DAC voltage to set output current is also a distinct possibility.


--
Regards
Phil Reid

Re: [PATCHv2 4/4] gpio: Remove VLA from stmpe driver

[Phil Reid]

On 23/03/2018 05:43, Laura Abbott wrote:

On 03/18/2018 06:29 PM, Phil Reid wrote:

On 16/03/2018 02:00, Laura Abbott wrote:

The new challenge is to remove VLAs from the kernel
(see https://lkml.org/lkml/2018/3/7/621)

This patch replaces a VLA with an appropriate call to kmalloc_array.

Signed-off-by: Laura Abbott <labb...@redhat.com>
---
v2: Switch to GFP_KERNEL. There was some discussion about if we should
be doing the allocation at all but given a) the allocation is pretty
small and b) we can possibly take a mutex in a called function I think
this is fine.


I still think it's a bad idea. It's simple to preallocate the buffer.
But it's up to the maintainer.



I'd feel a lot more confident about doing the global buffer with
guidance from the maintainer. But looking at the platform data, the
maximum number of GPIOs is 24, or 3 banks. Maybe we should just always
stack allocate the maximum since it's fairly small.


That's the other way to go.



---
  drivers/gpio/gpio-stmpe.c | 7 ++-
  1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/gpio/gpio-stmpe.c b/drivers/gpio/gpio-stmpe.c
index f8d7d1cd8488..c2bb20ace6f5 100644
--- a/drivers/gpio/gpio-stmpe.c
+++ b/drivers/gpio/gpio-stmpe.c
@@ -369,10 +369,14 @@ static irqreturn_t stmpe_gpio_irq(int irq, void *dev)
  struct stmpe *stmpe = stmpe_gpio->stmpe;
  u8 statmsbreg;
  int num_banks = DIV_ROUND_UP(stmpe->num_gpios, 8);
-    u8 status[num_banks];
+    u8 *status;
  int ret;
  int i;
+    status = kmalloc_array(num_banks, sizeof(*status), GFP_KERNEL);
+    if (!status)
+    return IRQ_NONE;
+
  /*
   * the stmpe_block_read() call below, imposes to set statmsbreg
   * with the register located at the lowest address. As STMPE1600
@@ -424,6 +428,7 @@ static irqreturn_t stmpe_gpio_irq(int irq, void *dev)
  }
  }
+    kfree(status);
  return IRQ_HANDLED;
  }











--
Regards
Phil Reid

Re: [PATCHv2 4/4] gpio: Remove VLA from stmpe driver

[Phil Reid]

On 23/03/2018 05:43, Laura Abbott wrote:

On 03/18/2018 06:29 PM, Phil Reid wrote:

On 16/03/2018 02:00, Laura Abbott wrote:

The new challenge is to remove VLAs from the kernel
(see https://lkml.org/lkml/2018/3/7/621)

This patch replaces a VLA with an appropriate call to kmalloc_array.

Signed-off-by: Laura Abbott 
---
v2: Switch to GFP_KERNEL. There was some discussion about if we should
be doing the allocation at all but given a) the allocation is pretty
small and b) we can possibly take a mutex in a called function I think
this is fine.


I still think it's a bad idea. It's simple to preallocate the buffer.
But it's up to the maintainer.



I'd feel a lot more confident about doing the global buffer with
guidance from the maintainer. But looking at the platform data, the
maximum number of GPIOs is 24, or 3 banks. Maybe we should just always
stack allocate the maximum since it's fairly small.


That's the other way to go.



---
  drivers/gpio/gpio-stmpe.c | 7 ++-
  1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/gpio/gpio-stmpe.c b/drivers/gpio/gpio-stmpe.c
index f8d7d1cd8488..c2bb20ace6f5 100644
--- a/drivers/gpio/gpio-stmpe.c
+++ b/drivers/gpio/gpio-stmpe.c
@@ -369,10 +369,14 @@ static irqreturn_t stmpe_gpio_irq(int irq, void *dev)
  struct stmpe *stmpe = stmpe_gpio->stmpe;
  u8 statmsbreg;
  int num_banks = DIV_ROUND_UP(stmpe->num_gpios, 8);
-    u8 status[num_banks];
+    u8 *status;
  int ret;
  int i;
+    status = kmalloc_array(num_banks, sizeof(*status), GFP_KERNEL);
+    if (!status)
+    return IRQ_NONE;
+
  /*
   * the stmpe_block_read() call below, imposes to set statmsbreg
   * with the register located at the lowest address. As STMPE1600
@@ -424,6 +428,7 @@ static irqreturn_t stmpe_gpio_irq(int irq, void *dev)
  }
  }
+    kfree(status);
  return IRQ_HANDLED;
  }











--
Regards
Phil Reid

Re: [RESEND PATCH v1 6/6] i2c: i2c-stm32f7: Implement I2C recovery mechanism

[Phil Reid]

On 12/03/2018 18:53, Pierre-Yves MORDRET wrote:

Feature prevents I2C lock-ups. Mechanism resets I2C state machine
and releases SCL/SDA signals but preserves I2C registers.

Signed-off-by: Pierre-Yves MORDRET 
---
   Version history:
 v1:
* Initial
---
---
  drivers/i2c/busses/i2c-stm32f7.c | 32 +---
  1 file changed, 29 insertions(+), 3 deletions(-)

diff --git a/drivers/i2c/busses/i2c-stm32f7.c b/drivers/i2c/busses/i2c-stm32f7.c
index 69a2e5e..3808bc9 100644
--- a/drivers/i2c/busses/i2c-stm32f7.c
+++ b/drivers/i2c/busses/i2c-stm32f7.c
@@ -718,6 +718,20 @@ static void stm32f7_i2c_smbus_reload(struct 
stm32f7_i2c_dev *i2c_dev)
writel_relaxed(cr2, i2c_dev->base + STM32F7_I2C_CR2);
  }
  
+static int stm32f7_i2c_recover_bus(struct i2c_adapter *i2c_adap)

+{
+   struct stm32f7_i2c_dev *i2c_dev = i2c_get_adapdata(i2c_adap);
+
+   dev_info(i2c_dev->dev, "Trying to recover bus\n");
+
+   stm32f7_i2c_clr_bits(i2c_dev->base + STM32F7_I2C_CR1,
+STM32F7_I2C_CR1_PE);


This only "releases" the scl / sda on the stm32f7 end (outputs are hiz I guess).
It doesn't trigger the scl clocking needed to recover a stuck device  via i2c 
recovery
from what I can see in the data sheet.



+
+   stm32f7_i2c_hw_config(i2c_dev);


Nothing in here either I think.



+
+   return 0;
+}
+
  static int stm32f7_i2c_wait_free_bus(struct stm32f7_i2c_dev *i2c_dev)
  {
u32 status;
@@ -727,12 +741,18 @@ static int stm32f7_i2c_wait_free_bus(struct 
stm32f7_i2c_dev *i2c_dev)
 status,
 !(status & STM32F7_I2C_ISR_BUSY),
 10, 1000);
+   if (!ret)
+   return 0;
+
+   dev_info(i2c_dev->dev, "bus busy\n");
+
+   ret = i2c_recover_bus(_dev->adap);
if (ret) {
-   dev_dbg(i2c_dev->dev, "bus busy\n");
-   ret = -EBUSY;
+   dev_err(i2c_dev->dev, "Failed to recover the bus (%d)\n", ret);
+   return ret;
}
  
-	return ret;

+   return -EBUSY;
  }
  
  static void stm32f7_i2c_xfer_msg(struct stm32f7_i2c_dev *i2c_dev,

@@ -1476,6 +1496,7 @@ static irqreturn_t stm32f7_i2c_isr_error(int irq, void 
*data)
if (status & STM32F7_I2C_ISR_BERR) {
dev_err(dev, "<%s>: Bus error\n", __func__);
writel_relaxed(STM32F7_I2C_ICR_BERRCF, base + STM32F7_I2C_ICR);
+   i2c_recover_bus(_dev->adap);
f7_msg->result = -EIO;
}
  
@@ -1760,6 +1781,10 @@ static struct i2c_algorithm stm32f7_i2c_algo = {

.unreg_slave = stm32f7_i2c_unreg_slave,
  };
  
+static struct i2c_bus_recovery_info stm32f7_i2c_recovery_info = {

+   .recover_bus = stm32f7_i2c_recover_bus,
+};
+
  static int stm32f7_i2c_probe(struct platform_device *pdev)
  {
struct device_node *np = pdev->dev.of_node;
@@ -1879,6 +1904,7 @@ static int stm32f7_i2c_probe(struct platform_device *pdev)
adap->algo = _i2c_algo;
adap->dev.parent = >dev;
adap->dev.of_node = pdev->dev.of_node;
+   adap->bus_recovery_info = _i2c_recovery_info;
  
  	init_completion(_dev->complete);
  




--
Regards
Phil

Re: [RESEND PATCH v1 6/6] i2c: i2c-stm32f7: Implement I2C recovery mechanism

[Phil Reid]

On 12/03/2018 18:53, Pierre-Yves MORDRET wrote:

Feature prevents I2C lock-ups. Mechanism resets I2C state machine
and releases SCL/SDA signals but preserves I2C registers.

Signed-off-by: Pierre-Yves MORDRET 
---
   Version history:
 v1:
* Initial
---
---
  drivers/i2c/busses/i2c-stm32f7.c | 32 +---
  1 file changed, 29 insertions(+), 3 deletions(-)

diff --git a/drivers/i2c/busses/i2c-stm32f7.c b/drivers/i2c/busses/i2c-stm32f7.c
index 69a2e5e..3808bc9 100644
--- a/drivers/i2c/busses/i2c-stm32f7.c
+++ b/drivers/i2c/busses/i2c-stm32f7.c
@@ -718,6 +718,20 @@ static void stm32f7_i2c_smbus_reload(struct 
stm32f7_i2c_dev *i2c_dev)
writel_relaxed(cr2, i2c_dev->base + STM32F7_I2C_CR2);
  }
  
+static int stm32f7_i2c_recover_bus(struct i2c_adapter *i2c_adap)

+{
+   struct stm32f7_i2c_dev *i2c_dev = i2c_get_adapdata(i2c_adap);
+
+   dev_info(i2c_dev->dev, "Trying to recover bus\n");
+
+   stm32f7_i2c_clr_bits(i2c_dev->base + STM32F7_I2C_CR1,
+STM32F7_I2C_CR1_PE);


This only "releases" the scl / sda on the stm32f7 end (outputs are hiz I guess).
It doesn't trigger the scl clocking needed to recover a stuck device  via i2c 
recovery
from what I can see in the data sheet.



+
+   stm32f7_i2c_hw_config(i2c_dev);


Nothing in here either I think.



+
+   return 0;
+}
+
  static int stm32f7_i2c_wait_free_bus(struct stm32f7_i2c_dev *i2c_dev)
  {
u32 status;
@@ -727,12 +741,18 @@ static int stm32f7_i2c_wait_free_bus(struct 
stm32f7_i2c_dev *i2c_dev)
 status,
 !(status & STM32F7_I2C_ISR_BUSY),
 10, 1000);
+   if (!ret)
+   return 0;
+
+   dev_info(i2c_dev->dev, "bus busy\n");
+
+   ret = i2c_recover_bus(_dev->adap);
if (ret) {
-   dev_dbg(i2c_dev->dev, "bus busy\n");
-   ret = -EBUSY;
+   dev_err(i2c_dev->dev, "Failed to recover the bus (%d)\n", ret);
+   return ret;
}
  
-	return ret;

+   return -EBUSY;
  }
  
  static void stm32f7_i2c_xfer_msg(struct stm32f7_i2c_dev *i2c_dev,

@@ -1476,6 +1496,7 @@ static irqreturn_t stm32f7_i2c_isr_error(int irq, void 
*data)
if (status & STM32F7_I2C_ISR_BERR) {
dev_err(dev, "<%s>: Bus error\n", __func__);
writel_relaxed(STM32F7_I2C_ICR_BERRCF, base + STM32F7_I2C_ICR);
+   i2c_recover_bus(_dev->adap);
f7_msg->result = -EIO;
}
  
@@ -1760,6 +1781,10 @@ static struct i2c_algorithm stm32f7_i2c_algo = {

.unreg_slave = stm32f7_i2c_unreg_slave,
  };
  
+static struct i2c_bus_recovery_info stm32f7_i2c_recovery_info = {

+   .recover_bus = stm32f7_i2c_recover_bus,
+};
+
  static int stm32f7_i2c_probe(struct platform_device *pdev)
  {
struct device_node *np = pdev->dev.of_node;
@@ -1879,6 +1904,7 @@ static int stm32f7_i2c_probe(struct platform_device *pdev)
adap->algo = _i2c_algo;
adap->dev.parent = >dev;
adap->dev.of_node = pdev->dev.of_node;
+   adap->bus_recovery_info = _i2c_recovery_info;
  
  	init_completion(_dev->complete);
  




--
Regards
Phil

Re: [PATCHv2 4/4] gpio: Remove VLA from stmpe driver

[Phil Reid]

On 16/03/2018 02:00, Laura Abbott wrote:

The new challenge is to remove VLAs from the kernel
(see https://lkml.org/lkml/2018/3/7/621)

This patch replaces a VLA with an appropriate call to kmalloc_array.

Signed-off-by: Laura Abbott <labb...@redhat.com>
---
v2: Switch to GFP_KERNEL. There was some discussion about if we should
be doing the allocation at all but given a) the allocation is pretty
small and b) we can possibly take a mutex in a called function I think
this is fine.


I still think it's a bad idea. It's simple to preallocate the buffer.
But it's up to the maintainer.



---
  drivers/gpio/gpio-stmpe.c | 7 ++-
  1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/gpio/gpio-stmpe.c b/drivers/gpio/gpio-stmpe.c
index f8d7d1cd8488..c2bb20ace6f5 100644
--- a/drivers/gpio/gpio-stmpe.c
+++ b/drivers/gpio/gpio-stmpe.c
@@ -369,10 +369,14 @@ static irqreturn_t stmpe_gpio_irq(int irq, void *dev)
struct stmpe *stmpe = stmpe_gpio->stmpe;
u8 statmsbreg;
int num_banks = DIV_ROUND_UP(stmpe->num_gpios, 8);
-   u8 status[num_banks];
+   u8 *status;
int ret;
int i;
  
+	status = kmalloc_array(num_banks, sizeof(*status), GFP_KERNEL);

+   if (!status)
+   return IRQ_NONE;
+
/*
 * the stmpe_block_read() call below, imposes to set statmsbreg
 * with the register located at the lowest address. As STMPE1600
@@ -424,6 +428,7 @@ static irqreturn_t stmpe_gpio_irq(int irq, void *dev)
}
}
  
+	kfree(status);

return IRQ_HANDLED;
  }
  




--
Regards
Phil Reid

Re: [PATCHv2 4/4] gpio: Remove VLA from stmpe driver

[Phil Reid]

On 16/03/2018 02:00, Laura Abbott wrote:

The new challenge is to remove VLAs from the kernel
(see https://lkml.org/lkml/2018/3/7/621)

This patch replaces a VLA with an appropriate call to kmalloc_array.

Signed-off-by: Laura Abbott 
---
v2: Switch to GFP_KERNEL. There was some discussion about if we should
be doing the allocation at all but given a) the allocation is pretty
small and b) we can possibly take a mutex in a called function I think
this is fine.


I still think it's a bad idea. It's simple to preallocate the buffer.
But it's up to the maintainer.



---
  drivers/gpio/gpio-stmpe.c | 7 ++-
  1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/gpio/gpio-stmpe.c b/drivers/gpio/gpio-stmpe.c
index f8d7d1cd8488..c2bb20ace6f5 100644
--- a/drivers/gpio/gpio-stmpe.c
+++ b/drivers/gpio/gpio-stmpe.c
@@ -369,10 +369,14 @@ static irqreturn_t stmpe_gpio_irq(int irq, void *dev)
struct stmpe *stmpe = stmpe_gpio->stmpe;
u8 statmsbreg;
int num_banks = DIV_ROUND_UP(stmpe->num_gpios, 8);
-   u8 status[num_banks];
+   u8 *status;
int ret;
int i;
  
+	status = kmalloc_array(num_banks, sizeof(*status), GFP_KERNEL);

+   if (!status)
+   return IRQ_NONE;
+
/*
 * the stmpe_block_read() call below, imposes to set statmsbreg
 * with the register located at the lowest address. As STMPE1600
@@ -424,6 +428,7 @@ static irqreturn_t stmpe_gpio_irq(int irq, void *dev)
}
}
  
+	kfree(status);

return IRQ_HANDLED;
  }
  




--
Regards
Phil Reid

Re: [PATCH 4/4] gpio: Remove VLA from stmpe driver

[Phil Reid]

On 14/03/2018 09:16, Laura Abbott wrote:

On 03/13/2018 05:18 PM, Laura Abbott wrote:

On 03/13/2018 02:13 AM, Phil Reid wrote:

On 10/03/2018 08:10, Laura Abbott wrote:

The new challenge is to remove VLAs from the kernel
(see https://lkml.org/lkml/2018/3/7/621)

This patch replaces a VLA with an appropriate call to kmalloc_array.

Signed-off-by: Laura Abbott <labb...@redhat.com>
---
  drivers/gpio/gpio-stmpe.c | 7 ++-
  1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/gpio/gpio-stmpe.c b/drivers/gpio/gpio-stmpe.c
index f8d7d1cd8488..b7854850bcdb 100644
--- a/drivers/gpio/gpio-stmpe.c
+++ b/drivers/gpio/gpio-stmpe.c
@@ -369,10 +369,14 @@ static irqreturn_t stmpe_gpio_irq(int irq, void *dev)
  struct stmpe *stmpe = stmpe_gpio->stmpe;
  u8 statmsbreg;
  int num_banks = DIV_ROUND_UP(stmpe->num_gpios, 8);
-    u8 status[num_banks];
+    u8 *status;
  int ret;
  int i;
+    status = kmalloc_array(num_banks, sizeof(*status), GFP_ATOMIC);
+    if (!status)
+    return IRQ_NONE;
+
  /*
   * the stmpe_block_read() call below, imposes to set statmsbreg
   * with the register located at the lowest address. As STMPE1600
@@ -424,6 +428,7 @@ static irqreturn_t stmpe_gpio_irq(int irq, void *dev)
  }
  }
+    kfree(status);
  return IRQ_HANDLED;
  }



Doing this in an irq handler seems wrong.
Perhaps better if a buffer is pre-allocated in stmpe_gpio


Sure, I can pre-allocate the buffer in the probe.


Actually I wonder if there would be concurrency problems if we
tried to pre-allocate a global buffer. But the IRQ handler
calls stmpe_block_read which takes a mutex to sleep so I think
doing the (small) kmalloc should be fine and I can change it to
a GFP_KERNEL.


I'm no expert on this driver, but I wouldn't think there'd be any concurrency
problem if the buffer is only used by the irq handler.
It should never get called concurrently for the same device.

As to the impact, I'll admit I've really got no idea how much potential 
overhead a
kmalloc has compared to a mutex for the linux kernel.
Just a general rule of thumb, that memory allocations are usually expensive.
--
Regards
Phil Reid

Re: [PATCH 4/4] gpio: Remove VLA from stmpe driver

[Phil Reid]

On 14/03/2018 09:16, Laura Abbott wrote:

On 03/13/2018 05:18 PM, Laura Abbott wrote:

On 03/13/2018 02:13 AM, Phil Reid wrote:

On 10/03/2018 08:10, Laura Abbott wrote:

The new challenge is to remove VLAs from the kernel
(see https://lkml.org/lkml/2018/3/7/621)

This patch replaces a VLA with an appropriate call to kmalloc_array.

Signed-off-by: Laura Abbott 
---
  drivers/gpio/gpio-stmpe.c | 7 ++-
  1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/gpio/gpio-stmpe.c b/drivers/gpio/gpio-stmpe.c
index f8d7d1cd8488..b7854850bcdb 100644
--- a/drivers/gpio/gpio-stmpe.c
+++ b/drivers/gpio/gpio-stmpe.c
@@ -369,10 +369,14 @@ static irqreturn_t stmpe_gpio_irq(int irq, void *dev)
  struct stmpe *stmpe = stmpe_gpio->stmpe;
  u8 statmsbreg;
  int num_banks = DIV_ROUND_UP(stmpe->num_gpios, 8);
-    u8 status[num_banks];
+    u8 *status;
  int ret;
  int i;
+    status = kmalloc_array(num_banks, sizeof(*status), GFP_ATOMIC);
+    if (!status)
+    return IRQ_NONE;
+
  /*
   * the stmpe_block_read() call below, imposes to set statmsbreg
   * with the register located at the lowest address. As STMPE1600
@@ -424,6 +428,7 @@ static irqreturn_t stmpe_gpio_irq(int irq, void *dev)
  }
  }
+    kfree(status);
  return IRQ_HANDLED;
  }



Doing this in an irq handler seems wrong.
Perhaps better if a buffer is pre-allocated in stmpe_gpio


Sure, I can pre-allocate the buffer in the probe.


Actually I wonder if there would be concurrency problems if we
tried to pre-allocate a global buffer. But the IRQ handler
calls stmpe_block_read which takes a mutex to sleep so I think
doing the (small) kmalloc should be fine and I can change it to
a GFP_KERNEL.


I'm no expert on this driver, but I wouldn't think there'd be any concurrency
problem if the buffer is only used by the irq handler.
It should never get called concurrently for the same device.

As to the impact, I'll admit I've really got no idea how much potential 
overhead a
kmalloc has compared to a mutex for the linux kernel.
Just a general rule of thumb, that memory allocations are usually expensive.
--
Regards
Phil Reid

Re: [PATCH 4/4] gpio: Remove VLA from stmpe driver

[Phil Reid]

On 10/03/2018 08:10, Laura Abbott wrote:

The new challenge is to remove VLAs from the kernel
(see https://lkml.org/lkml/2018/3/7/621)

This patch replaces a VLA with an appropriate call to kmalloc_array.

Signed-off-by: Laura Abbott <labb...@redhat.com>
---
  drivers/gpio/gpio-stmpe.c | 7 ++-
  1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/gpio/gpio-stmpe.c b/drivers/gpio/gpio-stmpe.c
index f8d7d1cd8488..b7854850bcdb 100644
--- a/drivers/gpio/gpio-stmpe.c
+++ b/drivers/gpio/gpio-stmpe.c
@@ -369,10 +369,14 @@ static irqreturn_t stmpe_gpio_irq(int irq, void *dev)
struct stmpe *stmpe = stmpe_gpio->stmpe;
u8 statmsbreg;
int num_banks = DIV_ROUND_UP(stmpe->num_gpios, 8);
-   u8 status[num_banks];
+   u8 *status;
int ret;
int i;
  
+	status = kmalloc_array(num_banks, sizeof(*status), GFP_ATOMIC);

+   if (!status)
+   return IRQ_NONE;
+
/*
 * the stmpe_block_read() call below, imposes to set statmsbreg
 * with the register located at the lowest address. As STMPE1600
@@ -424,6 +428,7 @@ static irqreturn_t stmpe_gpio_irq(int irq, void *dev)
}
}
  
+	kfree(status);

return IRQ_HANDLED;
  }
  



Doing this in an irq handler seems wrong.
Perhaps better if a buffer is pre-allocated in stmpe_gpio


--
Regards
Phil Reid

Re: [PATCH 4/4] gpio: Remove VLA from stmpe driver

[Phil Reid]

On 10/03/2018 08:10, Laura Abbott wrote:

The new challenge is to remove VLAs from the kernel
(see https://lkml.org/lkml/2018/3/7/621)

This patch replaces a VLA with an appropriate call to kmalloc_array.

Signed-off-by: Laura Abbott 
---
  drivers/gpio/gpio-stmpe.c | 7 ++-
  1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/gpio/gpio-stmpe.c b/drivers/gpio/gpio-stmpe.c
index f8d7d1cd8488..b7854850bcdb 100644
--- a/drivers/gpio/gpio-stmpe.c
+++ b/drivers/gpio/gpio-stmpe.c
@@ -369,10 +369,14 @@ static irqreturn_t stmpe_gpio_irq(int irq, void *dev)
struct stmpe *stmpe = stmpe_gpio->stmpe;
u8 statmsbreg;
int num_banks = DIV_ROUND_UP(stmpe->num_gpios, 8);
-   u8 status[num_banks];
+   u8 *status;
int ret;
int i;
  
+	status = kmalloc_array(num_banks, sizeof(*status), GFP_ATOMIC);

+   if (!status)
+   return IRQ_NONE;
+
/*
 * the stmpe_block_read() call below, imposes to set statmsbreg
 * with the register located at the lowest address. As STMPE1600
@@ -424,6 +428,7 @@ static irqreturn_t stmpe_gpio_irq(int irq, void *dev)
}
}
  
+	kfree(status);

return IRQ_HANDLED;
  }
  



Doing this in an irq handler seems wrong.
Perhaps better if a buffer is pre-allocated in stmpe_gpio


--
Regards
Phil Reid

Re: [PATCH 3/3] i2c: xlp9xx: Add support for SMBAlert

[Phil Reid]

On 6/03/2018 16:36, Jan Glauber wrote:

On Tue, Feb 27, 2018 at 01:26:20PM +, George Cherian wrote:

Add support for SMBus alert mechanism to i2c-xlp9xx driver.
The second interrupt is parsed to use for SMBus alert.
The first interrupt is the i2c controller main interrupt.

Signed-off-by: Kamlakant Patel <kamlakant.pa...@cavium.com>
Signed-off-by: George Cherian <george.cher...@cavium.com>
---
  drivers/i2c/busses/i2c-xlp9xx.c | 26 ++
  1 file changed, 26 insertions(+)

diff --git a/drivers/i2c/busses/i2c-xlp9xx.c b/drivers/i2c/busses/i2c-xlp9xx.c
index eb8913e..9462eab 100644
--- a/drivers/i2c/busses/i2c-xlp9xx.c
+++ b/drivers/i2c/busses/i2c-xlp9xx.c
@@ -10,6 +10,7 @@
  #include 
  #include 
  #include 
+#include 
  #include 
  #include 
  #include 
@@ -84,6 +85,8 @@ struct xlp9xx_i2c_dev {
struct device *dev;
struct i2c_adapter adapter;
struct completion msg_complete;
+   struct i2c_smbus_alert_setup alert_data;
+   struct i2c_client *ara;
int irq;
bool msg_read;
bool len_recv;
@@ -447,6 +450,21 @@ static int xlp9xx_i2c_get_frequency(struct platform_device 
*pdev,
return 0;
  }
  
+static int xlp9xx_i2c_smbus_setup(struct xlp9xx_i2c_dev *priv,

+ struct platform_device *pdev)
+{
+   if (!priv->alert_data.irq)
+   return -EINVAL;
+
+   priv->alert_data.alert_edge_triggered = 0;


Hi George,

I think this is not needed anymore, see:
9b9f2b8bc2ac i2c: i2c-smbus: Use threaded irq for smbalert

--Jan


Yes.

And also all of this is not needed if named interrupts.
- interrupt-names
"irq", "wakeup" and "smbus_alert" names are recognized by I2C core,
other names are left to individual drivers.

presence of named irq smbus_alert should result in alert handler being
created for that bus by the core




+
+   priv->ara = i2c_setup_smbus_alert(>adapter, >alert_data);
+   if (!priv->ara)
+   return -ENODEV;
+
+   return 0;
+}
+
  static int xlp9xx_i2c_probe(struct platform_device *pdev)
  {
struct xlp9xx_i2c_dev *priv;
@@ -467,6 +485,10 @@ static int xlp9xx_i2c_probe(struct platform_device *pdev)
dev_err(>dev, "invalid irq!\n");
return priv->irq;
}
+   /* SMBAlert irq */
+   priv->alert_data.irq = platform_get_irq(pdev, 1);
+   if (priv->alert_data.irq <= 0)
+   priv->alert_data.irq = 0;
  
  	xlp9xx_i2c_get_frequency(pdev, priv);

xlp9xx_i2c_init(priv);
@@ -493,6 +515,10 @@ static int xlp9xx_i2c_probe(struct platform_device *pdev)
if (err)
return err;
  
+	err = xlp9xx_i2c_smbus_setup(priv, pdev);

+   if (err)
+   dev_info(>dev, "No active SMBus alert %d\n", err);
+
platform_set_drvdata(pdev, priv);
    dev_dbg(>dev, "I2C bus:%d added\n", priv->adapter.nr);
  
--

2.1.4






--
Regards
Phil Reid

Re: [PATCH 3/3] i2c: xlp9xx: Add support for SMBAlert

[Phil Reid]

On 6/03/2018 16:36, Jan Glauber wrote:

On Tue, Feb 27, 2018 at 01:26:20PM +, George Cherian wrote:

Add support for SMBus alert mechanism to i2c-xlp9xx driver.
The second interrupt is parsed to use for SMBus alert.
The first interrupt is the i2c controller main interrupt.

Signed-off-by: Kamlakant Patel 
Signed-off-by: George Cherian 
---
  drivers/i2c/busses/i2c-xlp9xx.c | 26 ++
  1 file changed, 26 insertions(+)

diff --git a/drivers/i2c/busses/i2c-xlp9xx.c b/drivers/i2c/busses/i2c-xlp9xx.c
index eb8913e..9462eab 100644
--- a/drivers/i2c/busses/i2c-xlp9xx.c
+++ b/drivers/i2c/busses/i2c-xlp9xx.c
@@ -10,6 +10,7 @@
  #include 
  #include 
  #include 
+#include 
  #include 
  #include 
  #include 
@@ -84,6 +85,8 @@ struct xlp9xx_i2c_dev {
struct device *dev;
struct i2c_adapter adapter;
struct completion msg_complete;
+   struct i2c_smbus_alert_setup alert_data;
+   struct i2c_client *ara;
int irq;
bool msg_read;
bool len_recv;
@@ -447,6 +450,21 @@ static int xlp9xx_i2c_get_frequency(struct platform_device 
*pdev,
return 0;
  }
  
+static int xlp9xx_i2c_smbus_setup(struct xlp9xx_i2c_dev *priv,

+ struct platform_device *pdev)
+{
+   if (!priv->alert_data.irq)
+   return -EINVAL;
+
+   priv->alert_data.alert_edge_triggered = 0;


Hi George,

I think this is not needed anymore, see:
9b9f2b8bc2ac i2c: i2c-smbus: Use threaded irq for smbalert

--Jan


Yes.

And also all of this is not needed if named interrupts.
- interrupt-names
"irq", "wakeup" and "smbus_alert" names are recognized by I2C core,
other names are left to individual drivers.

presence of named irq smbus_alert should result in alert handler being
created for that bus by the core




+
+   priv->ara = i2c_setup_smbus_alert(>adapter, >alert_data);
+   if (!priv->ara)
+   return -ENODEV;
+
+   return 0;
+}
+
  static int xlp9xx_i2c_probe(struct platform_device *pdev)
  {
struct xlp9xx_i2c_dev *priv;
@@ -467,6 +485,10 @@ static int xlp9xx_i2c_probe(struct platform_device *pdev)
dev_err(>dev, "invalid irq!\n");
return priv->irq;
}
+   /* SMBAlert irq */
+   priv->alert_data.irq = platform_get_irq(pdev, 1);
+   if (priv->alert_data.irq <= 0)
+   priv->alert_data.irq = 0;
  
  	xlp9xx_i2c_get_frequency(pdev, priv);

xlp9xx_i2c_init(priv);
@@ -493,6 +515,10 @@ static int xlp9xx_i2c_probe(struct platform_device *pdev)
if (err)
return err;
  
+	err = xlp9xx_i2c_smbus_setup(priv, pdev);

+   if (err)
+   dev_info(>dev, "No active SMBus alert %d\n", err);
+
platform_set_drvdata(pdev, priv);
dev_dbg(>dev, "I2C bus:%d added\n", priv->adapter.nr);
  
--

2.1.4






--
Regards
Phil Reid

Re: [PATCH v2] iio:dummy: Replace S_IWUSR by 0200

[Phil Reid]

On 22/02/2018 04:01, Julia Lawall wrote:


On Wed, 21 Feb 2018, Rodrigo Siqueira wrote:


This patch fixes the checkpatch.pl warning:

drivers/iio/dummy/iio_dummy_evgen.c:151: WARNING: Symbolic permissions
'S_IWUSR' are not preferred. Consider using octal permissions '0200'.

I haven't studied up on it in great detail, but isn't there a more
specific macro that doesn't need a permission argument at all?



Probably thinking of IIO_DEVICE_ATTR_RO / IIO_DEVICE_ATTR_WO
But they don't provide flexibility for the show / store method.

--
Regards
Phil Reid

Re: [PATCH v2] iio:dummy: Replace S_IWUSR by 0200

[Phil Reid]

On 22/02/2018 04:01, Julia Lawall wrote:


On Wed, 21 Feb 2018, Rodrigo Siqueira wrote:


This patch fixes the checkpatch.pl warning:

drivers/iio/dummy/iio_dummy_evgen.c:151: WARNING: Symbolic permissions
'S_IWUSR' are not preferred. Consider using octal permissions '0200'.

I haven't studied up on it in great detail, but isn't there a more
specific macro that doesn't need a permission argument at all?



Probably thinking of IIO_DEVICE_ATTR_RO / IIO_DEVICE_ATTR_WO
But they don't provide flexibility for the show / store method.

--
Regards
Phil Reid

Re: [PATCH 0/2] net: dsa: lan9303: check error value from devm_gpiod_get_optional()

[Phil Reid]

On 7/01/2018 00:54, Egil Hjelmeland wrote:

Den 13. nov. 2017 09:07, skrev Phil Reid:

Replaces Pan Bian <bianpan2...@163.com> patch
"net: dsa: lan9303: correctly check return value of devm_gpiod_get_optional"

Errors need to be prograted back from probe.

Note: I have only compile tested the code as I don't have the hardware.

Phil Reid (2):
   net: dsa: lan9303: make lan9303_handle_reset() a void function
   net: dsa: lan9303: check error value from devm_gpiod_get_optional()

  drivers/net/dsa/lan9303-core.c | 21 -
  1 file changed, 12 insertions(+), 9 deletions(-)



Evidently this one did not make it through. Do you care to rebase and try again?

Thanks
Egil



Sure I'll give it another go.

--
Regards
Phil Reid

Re: [PATCH 0/2] net: dsa: lan9303: check error value from devm_gpiod_get_optional()

[Phil Reid]

On 7/01/2018 00:54, Egil Hjelmeland wrote:

Den 13. nov. 2017 09:07, skrev Phil Reid:

Replaces Pan Bian  patch
"net: dsa: lan9303: correctly check return value of devm_gpiod_get_optional"

Errors need to be prograted back from probe.

Note: I have only compile tested the code as I don't have the hardware.

Phil Reid (2):
   net: dsa: lan9303: make lan9303_handle_reset() a void function
   net: dsa: lan9303: check error value from devm_gpiod_get_optional()

  drivers/net/dsa/lan9303-core.c | 21 -
  1 file changed, 12 insertions(+), 9 deletions(-)



Evidently this one did not make it through. Do you care to rebase and try again?

Thanks
Egil



Sure I'll give it another go.

--
Regards
Phil Reid

Re: [PATCH] pinctrl: mcp23s08: fix irq setup order

[Phil Reid]

On 2/01/2018 17:10, Linus Walleij wrote:

On Thu, Dec 28, 2017 at 4:19 PM, Dmitry Mastykin <masti...@gmail.com> wrote:


When using mcp23s08 module with gpio-keys, often (50% of boots)
it fails to get irq numbers with message:
"gpio-keys keys: Unable to get irq number for GPIO 0, error -6".
Seems that irqs must be setup before devm_gpiochip_add_data().

Signed-off-by: Dmitry Mastykin <masti...@gmail.com>


Patch applied, albeit for devel.

Should it be tagged for stable or go into fixes?


I'm no expert on this one, but looking at other drivers they're using a mixture 
of ordering
for irq setup and devm_gpiochip_add_data() calls.

But should probably go into stable if it is the fix.


--
Regards
Phil Reid

Re: [PATCH] pinctrl: mcp23s08: fix irq setup order

[Phil Reid]

On 2/01/2018 17:10, Linus Walleij wrote:

On Thu, Dec 28, 2017 at 4:19 PM, Dmitry Mastykin  wrote:


When using mcp23s08 module with gpio-keys, often (50% of boots)
it fails to get irq numbers with message:
"gpio-keys keys: Unable to get irq number for GPIO 0, error -6".
Seems that irqs must be setup before devm_gpiochip_add_data().

Signed-off-by: Dmitry Mastykin 


Patch applied, albeit for devel.

Should it be tagged for stable or go into fixes?


I'm no expert on this one, but looking at other drivers they're using a mixture 
of ordering
for irq setup and devm_gpiochip_add_data() calls.

But should probably go into stable if it is the fix.


--
Regards
Phil Reid

Re: [PATCH v4] iio : Add cm3218 smbus ara and acpi support

[Phil Reid]

On 25/11/2017 21:57, Jonathan Cameron wrote:

On Tue, 21 Nov 2017 09:22:16 +0800
Phil Reid <pr...@electromag.com.au> wrote:


On 20/11/2017 18:57, Mika Westerberg wrote:

+Jarkko

On Sun, Nov 19, 2017 at 04:35:51PM +, Jonathan Cameron wrote:

On Thu, 2 Nov 2017 16:04:07 +0100
Wolfram Sang <w...@the-dreams.de> wrote:
  

On Thu, Nov 02, 2017 at 02:35:50PM +, Jonathan Cameron wrote:

On Fri, 27 Oct 2017 18:27:02 +0200
Marc CAPDEVILLE <m.capdevi...@no-log.org> wrote:
  

On asus T100, Capella cm3218 chip is implemented as ambiant light
sensor. This chip expose an smbus ARA protocol device on standard
address 0x0c. The chip is not functional before all alerts are
acknowledged.
On asus T100, this device is enumerated on ACPI bus and the
description give tow I2C connection. The first is the connection to
the ARA device and the second gives the real address of the device.

So, on device probe,If the i2c address is the ARA address and the
device is enumerated via acpi, we lookup for the real address in
the ACPI resource list and change it in the client structure.
if an interrupt resource is given, and only for cm3218 chip,
we declare an smbus_alert device.

Signed-off-by: Marc CAPDEVILLE <m.capdevi...@no-log.org>


Wolfram - this needs input from you on how to neatly handle
an ACPI registered ARA.


ACPI is really not my field. Try asking the I2C ACPI maintainers or
Benjamin Tissoires <benjamin.tissoi...@redhat.com> who did work on SMBus
interrupts recently.
  

Hi Mika, Benjamin,

So we've lost most of the context in this thread, but the basic question
is how to handle smbus ARA support with ACPI.

https://patchwork.kernel.org/patch/10030309/

Has the proposal made in this driver which is really not terribly nice
(as it registers the ARA device by messing with the address and registering
a second device).

As I understood it the ARA device registration should be handled by the
i2c master, but there are very few examples.

Phil pointed out that equivalent OF support recently got taken from him..
https://www.spinics.net/lists/devicetree/msg191947.html
https://www.spinics.net/lists/linux-i2c/msg31173.html

Any thoughts on the right way to do this?


There does not seem to be any way in ACPI to tell which "connection" is
used to describe ARA so that part currently is something each driver
needs to handle as they know the device the best. I don't think we have
any means to handle it in generic way in I2C core except to provide some
helpers that work on top of i2c_setup_smbus_alert() but understand ACPI
resources. Say provide function like this:

int acpi_i2c_setup_smbus_alert(struct i2c_adapter *adapter, int index);

Which then extracts automatically I2cSerialBus connection from "index"
and calls i2c_setup_smbus_alert() accordingly.

In the long run we could introduce _DSD property that can be used to
name the connection in the same way DT does;

  Name (_CRS, ResourceTemplate () {
  I2cSerialBus () { ... } // ARA
  I2cSerialBus () { ... } // normal device address
  })

  Name (_DSD, Package () {
  ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
  Package () {
  Package () {"smbus_alert", 0} // Where 0 means the first 
I2cSerialBus
  ...
  }
  })
   


I wonder if it's worth involving the smbus_alert driver in this case.
The cm3218 driver doesn't appear to be using the alert callback in strcut 
i2c_driver.


True - though it really should be..


So the smbus_alert driver is not going to notifiy the cm3218 driver.
Are there more than one alert/ara capable devices on the bus?


I'm not keen on taking a work around for one board on the basis that
it only has this one device on the bus - we will probably get a different
one down the line where this isn't true - then we end up ripping up what
has been done so far and starting again.

I don't mind having some ACPI matching code in the driver but it needs
to use the ARA infrastructure to actually handle things...


I'd agree, I only suggested the approach as the driver didn't seem to be using 
the alert
callback. Without using that callback there seems little point using it IMO.

As you say the better approach is to have some generic ACPI code for the alert.



If we then get nice generic ACPI code via some other means in the future
we can move the driver over to that.

Sometimes I wonder if some people just write ACPI tables with no though
to generalization at all, or that the code running might be shared across
different machines. (sometimes that assumption is valid, but not that
often... oh well - usual case of if we all work together everyone wins
but not worth one company trying to do things right...)


Perhaps a workaround in this case is if that acpi entry is defined the cm3218 
driver
handles that ara request directly to clear the interrupt.



Jonathan





--
Regards
Phil Reid

Re: [PATCH v4] iio : Add cm3218 smbus ara and acpi support

[Phil Reid]

On 25/11/2017 21:57, Jonathan Cameron wrote:

On Tue, 21 Nov 2017 09:22:16 +0800
Phil Reid  wrote:


On 20/11/2017 18:57, Mika Westerberg wrote:

+Jarkko

On Sun, Nov 19, 2017 at 04:35:51PM +, Jonathan Cameron wrote:

On Thu, 2 Nov 2017 16:04:07 +0100
Wolfram Sang  wrote:
  

On Thu, Nov 02, 2017 at 02:35:50PM +, Jonathan Cameron wrote:

On Fri, 27 Oct 2017 18:27:02 +0200
Marc CAPDEVILLE  wrote:
  

On asus T100, Capella cm3218 chip is implemented as ambiant light
sensor. This chip expose an smbus ARA protocol device on standard
address 0x0c. The chip is not functional before all alerts are
acknowledged.
On asus T100, this device is enumerated on ACPI bus and the
description give tow I2C connection. The first is the connection to
the ARA device and the second gives the real address of the device.

So, on device probe,If the i2c address is the ARA address and the
device is enumerated via acpi, we lookup for the real address in
the ACPI resource list and change it in the client structure.
if an interrupt resource is given, and only for cm3218 chip,
we declare an smbus_alert device.

Signed-off-by: Marc CAPDEVILLE 


Wolfram - this needs input from you on how to neatly handle
an ACPI registered ARA.


ACPI is really not my field. Try asking the I2C ACPI maintainers or
Benjamin Tissoires  who did work on SMBus
interrupts recently.
  

Hi Mika, Benjamin,

So we've lost most of the context in this thread, but the basic question
is how to handle smbus ARA support with ACPI.

https://patchwork.kernel.org/patch/10030309/

Has the proposal made in this driver which is really not terribly nice
(as it registers the ARA device by messing with the address and registering
a second device).

As I understood it the ARA device registration should be handled by the
i2c master, but there are very few examples.

Phil pointed out that equivalent OF support recently got taken from him..
https://www.spinics.net/lists/devicetree/msg191947.html
https://www.spinics.net/lists/linux-i2c/msg31173.html

Any thoughts on the right way to do this?


There does not seem to be any way in ACPI to tell which "connection" is
used to describe ARA so that part currently is something each driver
needs to handle as they know the device the best. I don't think we have
any means to handle it in generic way in I2C core except to provide some
helpers that work on top of i2c_setup_smbus_alert() but understand ACPI
resources. Say provide function like this:

int acpi_i2c_setup_smbus_alert(struct i2c_adapter *adapter, int index);

Which then extracts automatically I2cSerialBus connection from "index"
and calls i2c_setup_smbus_alert() accordingly.

In the long run we could introduce _DSD property that can be used to
name the connection in the same way DT does;

  Name (_CRS, ResourceTemplate () {
  I2cSerialBus () { ... } // ARA
  I2cSerialBus () { ... } // normal device address
  })

  Name (_DSD, Package () {
  ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
  Package () {
  Package () {"smbus_alert", 0} // Where 0 means the first 
I2cSerialBus
  ...
  }
  })
   


I wonder if it's worth involving the smbus_alert driver in this case.
The cm3218 driver doesn't appear to be using the alert callback in strcut 
i2c_driver.


True - though it really should be..


So the smbus_alert driver is not going to notifiy the cm3218 driver.
Are there more than one alert/ara capable devices on the bus?


I'm not keen on taking a work around for one board on the basis that
it only has this one device on the bus - we will probably get a different
one down the line where this isn't true - then we end up ripping up what
has been done so far and starting again.

I don't mind having some ACPI matching code in the driver but it needs
to use the ARA infrastructure to actually handle things...


I'd agree, I only suggested the approach as the driver didn't seem to be using 
the alert
callback. Without using that callback there seems little point using it IMO.

As you say the better approach is to have some generic ACPI code for the alert.



If we then get nice generic ACPI code via some other means in the future
we can move the driver over to that.

Sometimes I wonder if some people just write ACPI tables with no though
to generalization at all, or that the code running might be shared across
different machines. (sometimes that assumption is valid, but not that
often... oh well - usual case of if we all work together everyone wins
but not worth one company trying to do things right...)


Perhaps a workaround in this case is if that acpi entry is defined the cm3218 
driver
handles that ara request directly to clear the interrupt.



Jonathan





--
Regards
Phil Reid

Re: [PATCH v4] iio : Add cm3218 smbus ara and acpi support

[Phil Reid]

On 20/11/2017 18:57, Mika Westerberg wrote:

+Jarkko

On Sun, Nov 19, 2017 at 04:35:51PM +, Jonathan Cameron wrote:

On Thu, 2 Nov 2017 16:04:07 +0100
Wolfram Sang <w...@the-dreams.de> wrote:


On Thu, Nov 02, 2017 at 02:35:50PM +, Jonathan Cameron wrote:

On Fri, 27 Oct 2017 18:27:02 +0200
Marc CAPDEVILLE <m.capdevi...@no-log.org> wrote:
   

On asus T100, Capella cm3218 chip is implemented as ambiant light
sensor. This chip expose an smbus ARA protocol device on standard
address 0x0c. The chip is not functional before all alerts are
acknowledged.
On asus T100, this device is enumerated on ACPI bus and the
description give tow I2C connection. The first is the connection to
the ARA device and the second gives the real address of the device.

So, on device probe,If the i2c address is the ARA address and the
device is enumerated via acpi, we lookup for the real address in
the ACPI resource list and change it in the client structure.
if an interrupt resource is given, and only for cm3218 chip,
we declare an smbus_alert device.

Signed-off-by: Marc CAPDEVILLE <m.capdevi...@no-log.org>


Wolfram - this needs input from you on how to neatly handle
an ACPI registered ARA.


ACPI is really not my field. Try asking the I2C ACPI maintainers or
Benjamin Tissoires <benjamin.tissoi...@redhat.com> who did work on SMBus
interrupts recently.


Hi Mika, Benjamin,

So we've lost most of the context in this thread, but the basic question
is how to handle smbus ARA support with ACPI.

https://patchwork.kernel.org/patch/10030309/

Has the proposal made in this driver which is really not terribly nice
(as it registers the ARA device by messing with the address and registering
a second device).

As I understood it the ARA device registration should be handled by the
i2c master, but there are very few examples.

Phil pointed out that equivalent OF support recently got taken from him..
https://www.spinics.net/lists/devicetree/msg191947.html
https://www.spinics.net/lists/linux-i2c/msg31173.html

Any thoughts on the right way to do this?


There does not seem to be any way in ACPI to tell which "connection" is
used to describe ARA so that part currently is something each driver
needs to handle as they know the device the best. I don't think we have
any means to handle it in generic way in I2C core except to provide some
helpers that work on top of i2c_setup_smbus_alert() but understand ACPI
resources. Say provide function like this:

   int acpi_i2c_setup_smbus_alert(struct i2c_adapter *adapter, int index);

Which then extracts automatically I2cSerialBus connection from "index"
and calls i2c_setup_smbus_alert() accordingly.

In the long run we could introduce _DSD property that can be used to
name the connection in the same way DT does;

 Name (_CRS, ResourceTemplate () {
 I2cSerialBus () { ... } // ARA
 I2cSerialBus () { ... } // normal device address
 })

 Name (_DSD, Package () {
 ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
 Package () {
 Package () {"smbus_alert", 0} // Where 0 means the first 
I2cSerialBus
 ...
 }
 })



I wonder if it's worth involving the smbus_alert driver in this case.
The cm3218 driver doesn't appear to be using the alert callback in strcut 
i2c_driver.
So the smbus_alert driver is not going to notifiy the cm3218 driver.
Are there more than one alert/ara capable devices on the bus?
Perhaps a workaround in this case is if that acpi entry is defined the cm3218 
driver
handles that ara request directly to clear the interrupt.


--
Regards
Phil Reid

Re: [PATCH v4] iio : Add cm3218 smbus ara and acpi support

[Phil Reid]

On 20/11/2017 18:57, Mika Westerberg wrote:

+Jarkko

On Sun, Nov 19, 2017 at 04:35:51PM +, Jonathan Cameron wrote:

On Thu, 2 Nov 2017 16:04:07 +0100
Wolfram Sang  wrote:


On Thu, Nov 02, 2017 at 02:35:50PM +, Jonathan Cameron wrote:

On Fri, 27 Oct 2017 18:27:02 +0200
Marc CAPDEVILLE  wrote:
   

On asus T100, Capella cm3218 chip is implemented as ambiant light
sensor. This chip expose an smbus ARA protocol device on standard
address 0x0c. The chip is not functional before all alerts are
acknowledged.
On asus T100, this device is enumerated on ACPI bus and the
description give tow I2C connection. The first is the connection to
the ARA device and the second gives the real address of the device.

So, on device probe,If the i2c address is the ARA address and the
device is enumerated via acpi, we lookup for the real address in
the ACPI resource list and change it in the client structure.
if an interrupt resource is given, and only for cm3218 chip,
we declare an smbus_alert device.

Signed-off-by: Marc CAPDEVILLE 


Wolfram - this needs input from you on how to neatly handle
an ACPI registered ARA.


ACPI is really not my field. Try asking the I2C ACPI maintainers or
Benjamin Tissoires  who did work on SMBus
interrupts recently.


Hi Mika, Benjamin,

So we've lost most of the context in this thread, but the basic question
is how to handle smbus ARA support with ACPI.

https://patchwork.kernel.org/patch/10030309/

Has the proposal made in this driver which is really not terribly nice
(as it registers the ARA device by messing with the address and registering
a second device).

As I understood it the ARA device registration should be handled by the
i2c master, but there are very few examples.

Phil pointed out that equivalent OF support recently got taken from him..
https://www.spinics.net/lists/devicetree/msg191947.html
https://www.spinics.net/lists/linux-i2c/msg31173.html

Any thoughts on the right way to do this?


There does not seem to be any way in ACPI to tell which "connection" is
used to describe ARA so that part currently is something each driver
needs to handle as they know the device the best. I don't think we have
any means to handle it in generic way in I2C core except to provide some
helpers that work on top of i2c_setup_smbus_alert() but understand ACPI
resources. Say provide function like this:

   int acpi_i2c_setup_smbus_alert(struct i2c_adapter *adapter, int index);

Which then extracts automatically I2cSerialBus connection from "index"
and calls i2c_setup_smbus_alert() accordingly.

In the long run we could introduce _DSD property that can be used to
name the connection in the same way DT does;

 Name (_CRS, ResourceTemplate () {
 I2cSerialBus () { ... } // ARA
 I2cSerialBus () { ... } // normal device address
 })

 Name (_DSD, Package () {
 ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
 Package () {
 Package () {"smbus_alert", 0} // Where 0 means the first 
I2cSerialBus
 ...
 }
 })



I wonder if it's worth involving the smbus_alert driver in this case.
The cm3218 driver doesn't appear to be using the alert callback in strcut 
i2c_driver.
So the smbus_alert driver is not going to notifiy the cm3218 driver.
Are there more than one alert/ara capable devices on the bus?
Perhaps a workaround in this case is if that acpi entry is defined the cm3218 
driver
handles that ara request directly to clear the interrupt.


--
Regards
Phil Reid

[PATCH 2/2] net: dsa: lan9303: check error value from devm_gpiod_get_optional()

[Phil Reid]

devm_gpiod_get_optional() can return an error in addition to a NULL ptr.
Check for error and propagate that to the probe function. Check return
value in probe. This will now handle EPROBE_DEFER for the reset gpio.

Signed-off-by: Phil Reid <pr...@electromag.com.au>
---
 drivers/net/dsa/lan9303-core.c | 13 ++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/drivers/net/dsa/lan9303-core.c b/drivers/net/dsa/lan9303-core.c
index cc00308..b63173c 100644
--- a/drivers/net/dsa/lan9303-core.c
+++ b/drivers/net/dsa/lan9303-core.c
@@ -820,15 +820,18 @@ static int lan9303_register_switch(struct lan9303 *chip)
return dsa_register_switch(chip->ds);
 }
 
-static void lan9303_probe_reset_gpio(struct lan9303 *chip,
+static int lan9303_probe_reset_gpio(struct lan9303 *chip,
 struct device_node *np)
 {
chip->reset_gpio = devm_gpiod_get_optional(chip->dev, "reset",
   GPIOD_OUT_LOW);
 
+   if (IS_ERR(chip->reset_gpio))
+   return PTR_ERR(chip->reset_gpio);
+
if (!chip->reset_gpio) {
dev_dbg(chip->dev, "No reset GPIO defined\n");
-   return;
+   return 0;
}
 
chip->reset_duration = 200;
@@ -843,6 +846,8 @@ static void lan9303_probe_reset_gpio(struct lan9303 *chip,
/* A sane reset duration should not be longer than 1s */
if (chip->reset_duration > 1000)
chip->reset_duration = 1000;
+
+   return 0;
 }
 
 int lan9303_probe(struct lan9303 *chip, struct device_node *np)
@@ -851,7 +856,9 @@ int lan9303_probe(struct lan9303 *chip, struct device_node 
*np)
 
mutex_init(>indirect_mutex);
 
-   lan9303_probe_reset_gpio(chip, np);
+   ret = lan9303_probe_reset_gpio(chip, np);
+   if (ret)
+   return ret;
 
lan9303_handle_reset(chip);
 
-- 
1.8.3.1

[PATCH 2/2] net: dsa: lan9303: check error value from devm_gpiod_get_optional()

[Phil Reid]

devm_gpiod_get_optional() can return an error in addition to a NULL ptr.
Check for error and propagate that to the probe function. Check return
value in probe. This will now handle EPROBE_DEFER for the reset gpio.

Signed-off-by: Phil Reid 
---
 drivers/net/dsa/lan9303-core.c | 13 ++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/drivers/net/dsa/lan9303-core.c b/drivers/net/dsa/lan9303-core.c
index cc00308..b63173c 100644
--- a/drivers/net/dsa/lan9303-core.c
+++ b/drivers/net/dsa/lan9303-core.c
@@ -820,15 +820,18 @@ static int lan9303_register_switch(struct lan9303 *chip)
return dsa_register_switch(chip->ds);
 }
 
-static void lan9303_probe_reset_gpio(struct lan9303 *chip,
+static int lan9303_probe_reset_gpio(struct lan9303 *chip,
 struct device_node *np)
 {
chip->reset_gpio = devm_gpiod_get_optional(chip->dev, "reset",
   GPIOD_OUT_LOW);
 
+   if (IS_ERR(chip->reset_gpio))
+   return PTR_ERR(chip->reset_gpio);
+
if (!chip->reset_gpio) {
dev_dbg(chip->dev, "No reset GPIO defined\n");
-   return;
+   return 0;
}
 
chip->reset_duration = 200;
@@ -843,6 +846,8 @@ static void lan9303_probe_reset_gpio(struct lan9303 *chip,
/* A sane reset duration should not be longer than 1s */
if (chip->reset_duration > 1000)
chip->reset_duration = 1000;
+
+   return 0;
 }
 
 int lan9303_probe(struct lan9303 *chip, struct device_node *np)
@@ -851,7 +856,9 @@ int lan9303_probe(struct lan9303 *chip, struct device_node 
*np)
 
mutex_init(>indirect_mutex);
 
-   lan9303_probe_reset_gpio(chip, np);
+   ret = lan9303_probe_reset_gpio(chip, np);
+   if (ret)
+   return ret;
 
lan9303_handle_reset(chip);
 
-- 
1.8.3.1

[PATCH 1/2] net: dsa: lan9303: make lan9303_handle_reset() a void function

[Phil Reid]

lan9303_handle_reset never returns anything other than success.
So there's not need for it to return an error code.

Signed-off-by: Phil Reid <pr...@electromag.com.au>
---
 drivers/net/dsa/lan9303-core.c | 10 +++---
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/drivers/net/dsa/lan9303-core.c b/drivers/net/dsa/lan9303-core.c
index b471413..cc00308 100644
--- a/drivers/net/dsa/lan9303-core.c
+++ b/drivers/net/dsa/lan9303-core.c
@@ -550,18 +550,16 @@ static int lan9303_separate_ports(struct lan9303 *chip)
LAN9303_SWE_PORT_STATE_BLOCKING_PORT2);
 }
 
-static int lan9303_handle_reset(struct lan9303 *chip)
+static void lan9303_handle_reset(struct lan9303 *chip)
 {
if (!chip->reset_gpio)
-   return 0;
+   return;
 
if (chip->reset_duration != 0)
msleep(chip->reset_duration);
 
/* release (deassert) reset and activate the device */
gpiod_set_value_cansleep(chip->reset_gpio, 0);
-
-   return 0;
 }
 
 /* stop processing packets for all ports */
@@ -855,9 +853,7 @@ int lan9303_probe(struct lan9303 *chip, struct device_node 
*np)
 
lan9303_probe_reset_gpio(chip, np);
 
-   ret = lan9303_handle_reset(chip);
-   if (ret)
-   return ret;
+   lan9303_handle_reset(chip);
 
ret = lan9303_check_device(chip);
if (ret)
-- 
1.8.3.1

[PATCH 1/2] net: dsa: lan9303: make lan9303_handle_reset() a void function

[Phil Reid]

lan9303_handle_reset never returns anything other than success.
So there's not need for it to return an error code.

Signed-off-by: Phil Reid 
---
 drivers/net/dsa/lan9303-core.c | 10 +++---
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/drivers/net/dsa/lan9303-core.c b/drivers/net/dsa/lan9303-core.c
index b471413..cc00308 100644
--- a/drivers/net/dsa/lan9303-core.c
+++ b/drivers/net/dsa/lan9303-core.c
@@ -550,18 +550,16 @@ static int lan9303_separate_ports(struct lan9303 *chip)
LAN9303_SWE_PORT_STATE_BLOCKING_PORT2);
 }
 
-static int lan9303_handle_reset(struct lan9303 *chip)
+static void lan9303_handle_reset(struct lan9303 *chip)
 {
if (!chip->reset_gpio)
-   return 0;
+   return;
 
if (chip->reset_duration != 0)
msleep(chip->reset_duration);
 
/* release (deassert) reset and activate the device */
gpiod_set_value_cansleep(chip->reset_gpio, 0);
-
-   return 0;
 }
 
 /* stop processing packets for all ports */
@@ -855,9 +853,7 @@ int lan9303_probe(struct lan9303 *chip, struct device_node 
*np)
 
lan9303_probe_reset_gpio(chip, np);
 
-   ret = lan9303_handle_reset(chip);
-   if (ret)
-   return ret;
+   lan9303_handle_reset(chip);
 
ret = lan9303_check_device(chip);
if (ret)
-- 
1.8.3.1

[PATCH 0/2] net: dsa: lan9303: check error value from devm_gpiod_get_optional()

[Phil Reid]

Replaces Pan Bian <bianpan2...@163.com> patch 
"net: dsa: lan9303: correctly check return value of devm_gpiod_get_optional"

Errors need to be prograted back from probe.

Note: I have only compile tested the code as I don't have the hardware.

Phil Reid (2):
  net: dsa: lan9303: make lan9303_handle_reset() a void function
  net: dsa: lan9303: check error value from devm_gpiod_get_optional()

 drivers/net/dsa/lan9303-core.c | 21 -
 1 file changed, 12 insertions(+), 9 deletions(-)

-- 
1.8.3.1

[PATCH 0/2] net: dsa: lan9303: check error value from devm_gpiod_get_optional()

[Phil Reid]

Replaces Pan Bian  patch 
"net: dsa: lan9303: correctly check return value of devm_gpiod_get_optional"

Errors need to be prograted back from probe.

Note: I have only compile tested the code as I don't have the hardware.

Phil Reid (2):
  net: dsa: lan9303: make lan9303_handle_reset() a void function
  net: dsa: lan9303: check error value from devm_gpiod_get_optional()

 drivers/net/dsa/lan9303-core.c | 21 -
 1 file changed, 12 insertions(+), 9 deletions(-)

-- 
1.8.3.1

Re: [PATCH] net: dsa: lan9303: correctly check return value of devm_gpiod_get_optional

[Phil Reid]

On 12/11/2017 23:38, Pan Bian wrote:

Function devm_gpiod_get_optional() returns an ERR_PTR on failure. Its
return value should not be validated by a NULL check. Instead, use IS_ERR.

Signed-off-by: Pan Bian <bianpan2...@163.com>
---
  drivers/net/dsa/lan9303-core.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/dsa/lan9303-core.c b/drivers/net/dsa/lan9303-core.c
index b471413..6d3fc8f 100644
--- a/drivers/net/dsa/lan9303-core.c
+++ b/drivers/net/dsa/lan9303-core.c
@@ -828,7 +828,7 @@ static void lan9303_probe_reset_gpio(struct lan9303 *chip,
chip->reset_gpio = devm_gpiod_get_optional(chip->dev, "reset",
   GPIOD_OUT_LOW);
  
-	if (!chip->reset_gpio) {

+   if (IS_ERR(chip->reset_gpio)) {
dev_dbg(chip->dev, "No reset GPIO defined\n");
return;
}


Should not an error actually report the error and error out (ie fail probe).
But a null is the optional return and ok. (ie when -ENOENT return from sub 
gpiod_get call).

IS_ERR should be a separate condition check I think.

related lan9303_handle_reset() always returns 0.

lan9303_probe checks  lan9303_handle_reset() return value.

Probably should be checking lan9303_probe_reset_gpio() instead.

--
Regards
Phil Reid

Re: [PATCH] net: dsa: lan9303: correctly check return value of devm_gpiod_get_optional

[Phil Reid]

On 12/11/2017 23:38, Pan Bian wrote:

Function devm_gpiod_get_optional() returns an ERR_PTR on failure. Its
return value should not be validated by a NULL check. Instead, use IS_ERR.

Signed-off-by: Pan Bian 
---
  drivers/net/dsa/lan9303-core.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/dsa/lan9303-core.c b/drivers/net/dsa/lan9303-core.c
index b471413..6d3fc8f 100644
--- a/drivers/net/dsa/lan9303-core.c
+++ b/drivers/net/dsa/lan9303-core.c
@@ -828,7 +828,7 @@ static void lan9303_probe_reset_gpio(struct lan9303 *chip,
chip->reset_gpio = devm_gpiod_get_optional(chip->dev, "reset",
   GPIOD_OUT_LOW);
  
-	if (!chip->reset_gpio) {

+   if (IS_ERR(chip->reset_gpio)) {
dev_dbg(chip->dev, "No reset GPIO defined\n");
return;
}


Should not an error actually report the error and error out (ie fail probe).
But a null is the optional return and ok. (ie when -ENOENT return from sub 
gpiod_get call).

IS_ERR should be a separate condition check I think.

related lan9303_handle_reset() always returns 0.

lan9303_probe checks  lan9303_handle_reset() return value.

Probably should be checking lan9303_probe_reset_gpio() instead.

--
Regards
Phil Reid

Re: [PATCH v4] iio : Add cm3218 smbus ara and acpi support

[Phil Reid]

On 2/11/2017 22:49, Srinivas Pandruvada wrote:

On Thu, 2017-11-02 at 14:35 +, Jonathan Cameron wrote:

On Fri, 27 Oct 2017 18:27:02 +0200
Marc CAPDEVILLE <m.capdevi...@no-log.org> wrote:



On asus T100, Capella cm3218 chip is implemented as ambiant light
sensor. This chip expose an smbus ARA protocol device on standard
address 0x0c. The chip is not functional before all alerts are
acknowledged.
On asus T100, this device is enumerated on ACPI bus and the
description give tow I2C connection. The first is the connection to
the ARA device and the second gives the real address of the device.

So, on device probe,If the i2c address is the ARA address and the
device is enumerated via acpi, we lookup for the real address in
the ACPI resource list and change it in the client structure.
if an interrupt resource is given, and only for cm3218 chip,
we declare an smbus_alert device.

Signed-off-by: Marc CAPDEVILLE <m.capdevi...@no-log.org>

Wolfram - this needs input from you on how to neatly handle
an ACPI registered ARA.

There was another RFC from Alan cox
https://patchwork.ozlabs.org/patch/381773/



Wolfram just merged this from me:
[PATCH v11 00/10] Add sbs-manager with smbalert support
https://www.spinics.net/lists/devicetree/msg191947.html

Cleans up the smbus_alert driver a bit.
note: alert_edge_triggered was removed.

And for OF systems core creates the ara device.


--
Regards
Phil Reid

Re: [PATCH v4] iio : Add cm3218 smbus ara and acpi support

[Phil Reid]

On 2/11/2017 22:49, Srinivas Pandruvada wrote:

On Thu, 2017-11-02 at 14:35 +, Jonathan Cameron wrote:

On Fri, 27 Oct 2017 18:27:02 +0200
Marc CAPDEVILLE  wrote:



On asus T100, Capella cm3218 chip is implemented as ambiant light
sensor. This chip expose an smbus ARA protocol device on standard
address 0x0c. The chip is not functional before all alerts are
acknowledged.
On asus T100, this device is enumerated on ACPI bus and the
description give tow I2C connection. The first is the connection to
the ARA device and the second gives the real address of the device.

So, on device probe,If the i2c address is the ARA address and the
device is enumerated via acpi, we lookup for the real address in
the ACPI resource list and change it in the client structure.
if an interrupt resource is given, and only for cm3218 chip,
we declare an smbus_alert device.

Signed-off-by: Marc CAPDEVILLE 

Wolfram - this needs input from you on how to neatly handle
an ACPI registered ARA.

There was another RFC from Alan cox
https://patchwork.ozlabs.org/patch/381773/



Wolfram just merged this from me:
[PATCH v11 00/10] Add sbs-manager with smbalert support
https://www.spinics.net/lists/devicetree/msg191947.html

Cleans up the smbus_alert driver a bit.
note: alert_edge_triggered was removed.

And for OF systems core creates the ara device.


--
Regards
Phil Reid

Re: [PATCH] i2c-designware: add i2c gpio recovery option

[Phil Reid]

On 11/05/2017 21:53, Andy Shevchenko wrote:

+static int i2c_dw_init_recovery_info(struct dw_i2c_dev *dev,
+struct i2c_adapter *adap)
+{
+   struct i2c_bus_recovery_info *rinfo = >rinfo;
+
+   dev->gpio_scl = devm_gpiod_get_optional(dev->dev,
+   "scl",
+   GPIOD_OUT_HIGH);
+   if (IS_ERR_OR_NULL(dev->gpio_scl))

This is wrong. You should not use this macro in most cases. And
especially it breaks the logic behind _optional().

My logic here was that if the gpio is optional return null we return
0.

Why?!

_optional()*implies*  that all rest calls will go fine and do nothing.


which is an okay status.
But this breaks if !CONFIG_GPIOLIB, which I keep forgetting. I've
never
quite wrapped my head around why that's the case.

But the probe function only bails out if this returns EPROBE_DEFER.
Not sure that's the best approach

You need something like

desc = devm_gpiod_get_optional(...);
if (IS_ERR(desc))
  return PTR_ERR(desc);


I found that continuing without the check on null results in a kernel panic for 
a dereferenced null pointer.
So something in the gpiolib doesn't treat a null desc as optional.

It was probably this code:
int desc_to_gpio(const struct gpio_desc *desc)
{
return desc->gdev->base + (desc - >gdev->descs[0]);
}

So perhaps this should return an invalid gpio number when desc == null

I don't know what the intents are, so don't know if its a "bug" or  by design.

--
Regards
Phil Reid

Re: [PATCH] i2c-designware: add i2c gpio recovery option

[Phil Reid]

On 11/05/2017 21:53, Andy Shevchenko wrote:

+static int i2c_dw_init_recovery_info(struct dw_i2c_dev *dev,
+struct i2c_adapter *adap)
+{
+   struct i2c_bus_recovery_info *rinfo = >rinfo;
+
+   dev->gpio_scl = devm_gpiod_get_optional(dev->dev,
+   "scl",
+   GPIOD_OUT_HIGH);
+   if (IS_ERR_OR_NULL(dev->gpio_scl))

This is wrong. You should not use this macro in most cases. And
especially it breaks the logic behind _optional().

My logic here was that if the gpio is optional return null we return
0.

Why?!

_optional()*implies*  that all rest calls will go fine and do nothing.


which is an okay status.
But this breaks if !CONFIG_GPIOLIB, which I keep forgetting. I've
never
quite wrapped my head around why that's the case.

But the probe function only bails out if this returns EPROBE_DEFER.
Not sure that's the best approach

You need something like

desc = devm_gpiod_get_optional(...);
if (IS_ERR(desc))
  return PTR_ERR(desc);


I found that continuing without the check on null results in a kernel panic for 
a dereferenced null pointer.
So something in the gpiolib doesn't treat a null desc as optional.

It was probably this code:
int desc_to_gpio(const struct gpio_desc *desc)
{
return desc->gdev->base + (desc - >gdev->descs[0]);
}

So perhaps this should return an invalid gpio number when desc == null

I don't know what the intents are, so don't know if its a "bug" or  by design.

--
Regards
Phil Reid

Re: [PATCH] i2c-designware: add i2c gpio recovery option

[Phil Reid]

G'day Andy,

Thanks for the review.

On 10/05/2017 21:13, Andy Shevchenko wrote:

On Wed, 2017-05-10 at 13:57 +0200, Tim Sander wrote:

This patch contains much input from Phil Reid and has been tested
on Intel/Altera Cyclone V SOC Hardware with Altera GPIO's for the
SCL and SDA GPIO's. I am still a little unsure about the recover
in the timeout case (i2c-designware-core.c:770) as i could not
test this codepath.


Since it's not an RFC anymore let me do some comments on the below.


@@ -317,6 +317,7 @@ static void i2c_dw_release_lock(struct dw_i2c_dev
*dev)
 dev->release_lock(dev);
  }
  
+

  /**
   * i2c_dw_init() - initialize the designware i2c master hardware
   * @dev: device private data


This doesn't belong to the change.


@@ -463,7 +464,12 @@ static int i2c_dw_wait_bus_not_busy(struct
dw_i2c_dev *dev)
 while (dw_readl(dev, DW_IC_STATUS) & DW_IC_STATUS_ACTIVITY) {



 if (timeout <= 0) {
 dev_warn(dev->dev, "timeout waiting for bus
ready\n");
-   return -ETIMEDOUT;
+   i2c_recover_bus(>adapter);
+
+   if (dw_readl(dev, DW_IC_STATUS) &
DW_IC_STATUS_ACTIVITY)
+   return -ETIMEDOUT;



+   else


Redundant.


+   return 0;
 }


Actually I would rather refactor first above function:
1) to be do {} while();
2) to have invariant condition out of the loop.


 timeout--;
 usleep_range(1000, 1100);



@@ -719,9 +725,10 @@ static int i2c_dw_handle_tx_abort(struct
dw_i2c_dev *dev)
 for_each_set_bit(i, _source, ARRAY_SIZE(abort_sources))
 dev_err(dev->dev, "%s: %s\n", __func__,
abort_sources[i]);
  
-   if (abort_source & DW_IC_TX_ARB_LOST)

+   if (abort_source & DW_IC_TX_ARB_LOST) {
+   i2c_recover_bus(>adapter);
 return -EAGAIN;



-   else if (abort_source & DW_IC_TX_ABRT_GCALL_READ)
+   } else if (abort_source & DW_IC_TX_ABRT_GCALL_READ)
 return -EINVAL; /* wrong msgs[] data */
 else


Both else:s are redundant.

if (abort_source & DW_IC_TX_ARB_LOST) {
i2c_recover_bus(>adapter);
 return -EAGAIN;
}

 if (abort_source & DW_IC_TX_ABRT_GCALL_READ)
...

Though I may agree on leaving them here for sake of keeping less lines
of code.


 return -EIO;



+#include 


I think it should be

#include 


--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -41,6 +41,7 @@
  #include 
  #include 
  #include 



+#include 


No, please don't.

In recent code we try to avoid OF/ACPI/platform specific bits if there
is a common resource provider (and API) for that. GPIO is the case.


+void i2c_dw_unprepare_recovery(struct i2c_adapter *adap)
+{



+}
+
+


Remove extra line.


+static int i2c_dw_get_scl(struct i2c_adapter *adap)
+{
+   struct platform_device *pdev = to_platform_device(>dev);
+   struct dw_i2c_dev *dev = platform_get_drvdata(pdev);


struct dw_i2c_dev *dev = i2c_get_adapdata(adap); ?

Ditto for all occurrences in the code.


+
+   return gpiod_get_value_cansleep(dev->gpio_scl);
+}



+static int i2c_dw_init_recovery_info(struct dw_i2c_dev *dev,
+struct i2c_adapter *adap)
+{
+   struct i2c_bus_recovery_info *rinfo = >rinfo;
+
+   dev->gpio_scl = devm_gpiod_get_optional(dev->dev,
+   "scl",
+   GPIOD_OUT_HIGH);



+   if (IS_ERR_OR_NULL(dev->gpio_scl))


This is wrong. You should not use this macro in most cases. And
especially it breaks the logic behind _optional().

My logic here was that if the gpio is optional return null we return 0.
which is an okay status.
But this breaks if !CONFIG_GPIOLIB, which I keep forgetting. I've never
quite wrapped my head around why that's the case.

But the probe function only bails out if this returns EPROBE_DEFER.
Not sure that's the best approach




+   return PTR_ERR(dev->gpio_scl);
+
+   dev->gpio_sda = devm_gpiod_get_optional(dev->dev, "sda",
GPIOD_IN);



+   if (IS_ERR_OR_NULL(dev->gpio_sda))


Ditto.


+   return PTR_ERR(dev->gpio_sda);



+   rinfo->scl_gpio = desc_to_gpio(dev->gpio_scl);
+   rinfo->sda_gpio = desc_to_gpio(dev->gpio_sda);


Why?!

From my first attempt, didn't remove it from the example I sent.

We could change i2c_init_recovery to something like the following
then the gpio set / getter could use the default functions.
Not sure the code is completely correct but hopefully you get the concept.

static void i2c_init_recovery(struct i2c_adapter *adap)
{
struct

Re: [PATCH] i2c-designware: add i2c gpio recovery option

[Phil Reid]

G'day Andy,

Thanks for the review.

On 10/05/2017 21:13, Andy Shevchenko wrote:

On Wed, 2017-05-10 at 13:57 +0200, Tim Sander wrote:

This patch contains much input from Phil Reid and has been tested
on Intel/Altera Cyclone V SOC Hardware with Altera GPIO's for the
SCL and SDA GPIO's. I am still a little unsure about the recover
in the timeout case (i2c-designware-core.c:770) as i could not
test this codepath.


Since it's not an RFC anymore let me do some comments on the below.


@@ -317,6 +317,7 @@ static void i2c_dw_release_lock(struct dw_i2c_dev
*dev)
 dev->release_lock(dev);
  }
  
+

  /**
   * i2c_dw_init() - initialize the designware i2c master hardware
   * @dev: device private data


This doesn't belong to the change.


@@ -463,7 +464,12 @@ static int i2c_dw_wait_bus_not_busy(struct
dw_i2c_dev *dev)
 while (dw_readl(dev, DW_IC_STATUS) & DW_IC_STATUS_ACTIVITY) {



 if (timeout <= 0) {
 dev_warn(dev->dev, "timeout waiting for bus
ready\n");
-   return -ETIMEDOUT;
+   i2c_recover_bus(>adapter);
+
+   if (dw_readl(dev, DW_IC_STATUS) &
DW_IC_STATUS_ACTIVITY)
+   return -ETIMEDOUT;



+   else


Redundant.


+   return 0;
 }


Actually I would rather refactor first above function:
1) to be do {} while();
2) to have invariant condition out of the loop.


 timeout--;
 usleep_range(1000, 1100);



@@ -719,9 +725,10 @@ static int i2c_dw_handle_tx_abort(struct
dw_i2c_dev *dev)
 for_each_set_bit(i, _source, ARRAY_SIZE(abort_sources))
 dev_err(dev->dev, "%s: %s\n", __func__,
abort_sources[i]);
  
-   if (abort_source & DW_IC_TX_ARB_LOST)

+   if (abort_source & DW_IC_TX_ARB_LOST) {
+   i2c_recover_bus(>adapter);
 return -EAGAIN;



-   else if (abort_source & DW_IC_TX_ABRT_GCALL_READ)
+   } else if (abort_source & DW_IC_TX_ABRT_GCALL_READ)
 return -EINVAL; /* wrong msgs[] data */
 else


Both else:s are redundant.

if (abort_source & DW_IC_TX_ARB_LOST) {
i2c_recover_bus(>adapter);
 return -EAGAIN;
}

 if (abort_source & DW_IC_TX_ABRT_GCALL_READ)
...

Though I may agree on leaving them here for sake of keeping less lines
of code.


 return -EIO;



+#include 


I think it should be

#include 


--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -41,6 +41,7 @@
  #include 
  #include 
  #include 



+#include 


No, please don't.

In recent code we try to avoid OF/ACPI/platform specific bits if there
is a common resource provider (and API) for that. GPIO is the case.


+void i2c_dw_unprepare_recovery(struct i2c_adapter *adap)
+{



+}
+
+


Remove extra line.


+static int i2c_dw_get_scl(struct i2c_adapter *adap)
+{
+   struct platform_device *pdev = to_platform_device(>dev);
+   struct dw_i2c_dev *dev = platform_get_drvdata(pdev);


struct dw_i2c_dev *dev = i2c_get_adapdata(adap); ?

Ditto for all occurrences in the code.


+
+   return gpiod_get_value_cansleep(dev->gpio_scl);
+}



+static int i2c_dw_init_recovery_info(struct dw_i2c_dev *dev,
+struct i2c_adapter *adap)
+{
+   struct i2c_bus_recovery_info *rinfo = >rinfo;
+
+   dev->gpio_scl = devm_gpiod_get_optional(dev->dev,
+   "scl",
+   GPIOD_OUT_HIGH);



+   if (IS_ERR_OR_NULL(dev->gpio_scl))


This is wrong. You should not use this macro in most cases. And
especially it breaks the logic behind _optional().

My logic here was that if the gpio is optional return null we return 0.
which is an okay status.
But this breaks if !CONFIG_GPIOLIB, which I keep forgetting. I've never
quite wrapped my head around why that's the case.

But the probe function only bails out if this returns EPROBE_DEFER.
Not sure that's the best approach




+   return PTR_ERR(dev->gpio_scl);
+
+   dev->gpio_sda = devm_gpiod_get_optional(dev->dev, "sda",
GPIOD_IN);



+   if (IS_ERR_OR_NULL(dev->gpio_sda))


Ditto.


+   return PTR_ERR(dev->gpio_sda);



+   rinfo->scl_gpio = desc_to_gpio(dev->gpio_scl);
+   rinfo->sda_gpio = desc_to_gpio(dev->gpio_sda);


Why?!

From my first attempt, didn't remove it from the example I sent.

We could change i2c_init_recovery to something like the following
then the gpio set / getter could use the default functions.
Not sure the code is completely correct but hopefully you get the concept.

static void i2c_init_recovery(struct i2c_adapter *adap)
{
struct

Re: RFC: i2c designware gpio recovery

[Phil Reid]

G'day Tim,

Sorry for the delay in looking at this.
My device is currently running a 4.9 kernel and I had to backport the cahnges 
to the driver
to get things running with your patch.

In general the code works and the bus recovers now.
I've been using the i2c gpio bus driver because the dw wouldn't do recovery.
But this looks alot nicer.


On 4/05/2017 03:04, Tim Sander wrote:

So i took a look into the device tree file socfpga.dtsi and found that
the
reset lines where not defined (although available in the corresponding
reset manager). Is there a reason for this? Other components are
connected.


There's a few thing like that where the bootloader has been expected to
setup the resets etc.


Yes, but if the resets are not connected in the device tree, the linux
drivers are not going to use them?


Yes, so they should be added. I don't think we should assume the bootloader
sets things up. But that doesn't seem to have been the assumption with the
Alter SOC's.

I will prepare a patch for this.


However with the patch below my previously sent patch works!

If there is interest in would cleanup the patch and send it in for
mainlining. I think the most unacceptable part would be this line:
+   ret = gpio_request_one(bri->scl_gpio, //GPIOF_OPEN_DRAIN |
My gpio drivers refuse to work as output as they have no open drain
mode.
So i wonder how to get this solved in a clean manner.


I thought the gpio system would emulate open drain by switching the pin
between an input and output driven low in this case. How are you
configuring the GPIO's in the FPGA?


I don't switch to GPIO mode. As the I2C logic is only pulling active low,
i only do a wired and with the gpio (implemented in the fpga) port output
on the output enable line for the SCL output.  SDA is only an additional
input for the second in fpga gpio port.

A picture should make it a clearer:

gpio scl--\
i2c   scl --&---i2c mode output pin (configured as fpga loan)

In my case the original i2c pins where occupied by some other logic
conflicting so the i2c pins had to be shifted to some other pins using
fpga logic. So it was just a matter of adding a two port gpio port.


I think I understand. What soft core gpio controller are you using?

I am using the standard altera fpga gpios.




I dug into things a little and found the following init function works without 
requiring modification to the core.
The GPIO config (open drain or not etc) can be put in the device tree.

static int i2c_dw_get_scl(struct i2c_adapter *adap)
{
struct platform_device *pdev = to_platform_device(>dev);
struct dw_i2c_dev *dev = platform_get_drvdata(pdev);
return gpiod_get_value_cansleep(dev->gpio_scl);
}

static void i2c_dw_set_scl(struct i2c_adapter *adap, int val)
{
struct platform_device *pdev = to_platform_device(>dev);
struct dw_i2c_dev *dev = platform_get_drvdata(pdev);
gpiod_set_value_cansleep(dev->gpio_scl, val);
}

static int i2c_dw_get_sda(struct i2c_adapter *adap)
{
struct platform_device *pdev = to_platform_device(>dev);
struct dw_i2c_dev *dev = platform_get_drvdata(pdev);
return gpiod_get_value_cansleep(dev->gpio_sda);
}

static int i2c_dw_init_recovery_info(struct dw_i2c_dev *dev, struct i2c_adapter 
*adap)
{
struct i2c_bus_recovery_info *rinfo = >rinfo;

dev->gpio_scl = devm_gpiod_get_optional(dev->dev, "scl", 
GPIOD_OUT_HIGH);
if (IS_ERR_OR_NULL(dev->gpio_scl))
return PTR_ERR(dev->gpio_scl);

dev->gpio_sda = devm_gpiod_get_optional(dev->dev, "sda", GPIOD_IN);
if (IS_ERR_OR_NULL(dev->gpio_sda))
return PTR_ERR(dev->gpio_sda);

rinfo->scl_gpio  = desc_to_gpio(dev->gpio_scl);
rinfo->sda_gpio = desc_to_gpio(dev->gpio_sda);
rinfo->set_scl  = i2c_dw_set_scl;
rinfo->get_scl  = i2c_dw_get_scl;
rinfo->get_sda  = i2c_dw_get_sda;

rinfo->recover_bus = i2c_generic_scl_recovery;
rinfo->prepare_recovery = i2c_dw_prepare_recovery;
rinfo->unprepare_recovery = i2c_dw_unprepare_recovery;
adap->bus_recovery_info = rinfo;

dev_info(dev->dev,
"adapter: %s running with gpio recovery mode! scl:%i sda:%i \n",
adap->name, rinfo->scl_gpio, rinfo->sda_gpio);

return 0;
};

A small modification to the i2c-core could be done in i2c_init_recovery to 
allow:
rinfo->recover_bus == i2c_generic_scl_recovery
when scl_gpio is also set and fallback to using the core set / get scl / sda 
calls
Which would remove the need for the above i2c_dw_* functions.
I wouldn't think that would cause any problems.



--
Regards
Phil Reid

ElectroMagnetic Imaging Technology Pty Ltd
Development of Geophysical Instrumentation & Software
www.electromag.com.au

3 The Avenue, Midland WA 6056, AUSTRALIA
Ph: +61 8 9250 8100
Fax: +61 8 9250 7100
Email: pr...@electromag.com.au

Re: RFC: i2c designware gpio recovery

[Phil Reid]

G'day Tim,

Sorry for the delay in looking at this.
My device is currently running a 4.9 kernel and I had to backport the cahnges 
to the driver
to get things running with your patch.

In general the code works and the bus recovers now.
I've been using the i2c gpio bus driver because the dw wouldn't do recovery.
But this looks alot nicer.


On 4/05/2017 03:04, Tim Sander wrote:

So i took a look into the device tree file socfpga.dtsi and found that
the
reset lines where not defined (although available in the corresponding
reset manager). Is there a reason for this? Other components are
connected.


There's a few thing like that where the bootloader has been expected to
setup the resets etc.


Yes, but if the resets are not connected in the device tree, the linux
drivers are not going to use them?


Yes, so they should be added. I don't think we should assume the bootloader
sets things up. But that doesn't seem to have been the assumption with the
Alter SOC's.

I will prepare a patch for this.


However with the patch below my previously sent patch works!

If there is interest in would cleanup the patch and send it in for
mainlining. I think the most unacceptable part would be this line:
+   ret = gpio_request_one(bri->scl_gpio, //GPIOF_OPEN_DRAIN |
My gpio drivers refuse to work as output as they have no open drain
mode.
So i wonder how to get this solved in a clean manner.


I thought the gpio system would emulate open drain by switching the pin
between an input and output driven low in this case. How are you
configuring the GPIO's in the FPGA?


I don't switch to GPIO mode. As the I2C logic is only pulling active low,
i only do a wired and with the gpio (implemented in the fpga) port output
on the output enable line for the SCL output.  SDA is only an additional
input for the second in fpga gpio port.

A picture should make it a clearer:

gpio scl--\
i2c   scl --&---i2c mode output pin (configured as fpga loan)

In my case the original i2c pins where occupied by some other logic
conflicting so the i2c pins had to be shifted to some other pins using
fpga logic. So it was just a matter of adding a two port gpio port.


I think I understand. What soft core gpio controller are you using?

I am using the standard altera fpga gpios.




I dug into things a little and found the following init function works without 
requiring modification to the core.
The GPIO config (open drain or not etc) can be put in the device tree.

static int i2c_dw_get_scl(struct i2c_adapter *adap)
{
struct platform_device *pdev = to_platform_device(>dev);
struct dw_i2c_dev *dev = platform_get_drvdata(pdev);
return gpiod_get_value_cansleep(dev->gpio_scl);
}

static void i2c_dw_set_scl(struct i2c_adapter *adap, int val)
{
struct platform_device *pdev = to_platform_device(>dev);
struct dw_i2c_dev *dev = platform_get_drvdata(pdev);
gpiod_set_value_cansleep(dev->gpio_scl, val);
}

static int i2c_dw_get_sda(struct i2c_adapter *adap)
{
struct platform_device *pdev = to_platform_device(>dev);
struct dw_i2c_dev *dev = platform_get_drvdata(pdev);
return gpiod_get_value_cansleep(dev->gpio_sda);
}

static int i2c_dw_init_recovery_info(struct dw_i2c_dev *dev, struct i2c_adapter 
*adap)
{
struct i2c_bus_recovery_info *rinfo = >rinfo;

dev->gpio_scl = devm_gpiod_get_optional(dev->dev, "scl", 
GPIOD_OUT_HIGH);
if (IS_ERR_OR_NULL(dev->gpio_scl))
return PTR_ERR(dev->gpio_scl);

dev->gpio_sda = devm_gpiod_get_optional(dev->dev, "sda", GPIOD_IN);
if (IS_ERR_OR_NULL(dev->gpio_sda))
return PTR_ERR(dev->gpio_sda);

rinfo->scl_gpio  = desc_to_gpio(dev->gpio_scl);
rinfo->sda_gpio = desc_to_gpio(dev->gpio_sda);
rinfo->set_scl  = i2c_dw_set_scl;
rinfo->get_scl  = i2c_dw_get_scl;
rinfo->get_sda  = i2c_dw_get_sda;

rinfo->recover_bus = i2c_generic_scl_recovery;
rinfo->prepare_recovery = i2c_dw_prepare_recovery;
rinfo->unprepare_recovery = i2c_dw_unprepare_recovery;
adap->bus_recovery_info = rinfo;

dev_info(dev->dev,
"adapter: %s running with gpio recovery mode! scl:%i sda:%i \n",
adap->name, rinfo->scl_gpio, rinfo->sda_gpio);

return 0;
};

A small modification to the i2c-core could be done in i2c_init_recovery to 
allow:
rinfo->recover_bus == i2c_generic_scl_recovery
when scl_gpio is also set and fallback to using the core set / get scl / sda 
calls
Which would remove the need for the above i2c_dw_* functions.
I wouldn't think that would cause any problems.



--
Regards
Phil Reid

ElectroMagnetic Imaging Technology Pty Ltd
Development of Geophysical Instrumentation & Software
www.electromag.com.au

3 The Avenue, Midland WA 6056, AUSTRALIA
Ph: +61 8 9250 8100
Fax: +61 8 9250 7100
Email: pr...@electromag.com.au