Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1272781 > unrolled thread
| Started by | Cory Tusar <cory.tusar@pid1solutions.com> |
|---|---|
| First post | 2015-11-19 04:40 +0100 |
| Last post | 2015-11-25 06:00 +0100 |
| Articles | 2 on this page of 22 — 5 participants |
Back to article view | Back to linux.kernel
[PATCH v2 0/5] Devicetree support for misc/eeprom/eeprom_93xx46. Cory Tusar <cory.tusar@pid1solutions.com> - 2015-11-19 04:40 +0100
[PATCH v2 1/5] misc: eeprom_93xx46: Fix 16-bit read and write accesses. Cory Tusar <cory.tusar@pid1solutions.com> - 2015-11-19 04:40 +0100
[PATCH v2 2/5] Documentation: devicetree: Add DT bindings to eeprom_93xx46 driver. Cory Tusar <cory.tusar@pid1solutions.com> - 2015-11-19 04:40 +0100
Re: [PATCH v2 2/5] Documentation: devicetree: Add DT bindings to eeprom_93xx46 driver. Rob Herring <robh@kernel.org> - 2015-11-19 16:00 +0100
Re: [PATCH v2 2/5] Documentation: devicetree: Add DT bindings to eeprom_93xx46 driver. Cory Tusar <cory.tusar@pid1solutions.com> - 2015-11-19 18:40 +0100
[PATCH v2 3/5] misc: eeprom_93xx46: Implement eeprom_93xx46 DT bindings. Cory Tusar <cory.tusar@pid1solutions.com> - 2015-11-19 04:40 +0100
Re: [PATCH v2 3/5] misc: eeprom_93xx46: Implement eeprom_93xx46 DT bindings. Vladimir Zapolskiy <vz@mleia.com> - 2015-11-19 07:00 +0100
Re: [PATCH v2 3/5] misc: eeprom_93xx46: Implement eeprom_93xx46 DT bindings. "Andrew F. Davis" <afd@ti.com> - 2015-11-19 15:10 +0100
Re: [PATCH v2 3/5] misc: eeprom_93xx46: Implement eeprom_93xx46 DT bindings. Vladimir Zapolskiy <vz@mleia.com> - 2015-11-19 17:20 +0100
Re: [PATCH v2 3/5] misc: eeprom_93xx46: Implement eeprom_93xx46 DT bindings. Cory Tusar <cory.tusar@pid1solutions.com> - 2015-11-21 06:00 +0100
Re: [PATCH v2 3/5] misc: eeprom_93xx46: Implement eeprom_93xx46 DT bindings. Cory Tusar <cory.tusar@pid1solutions.com> - 2015-11-21 05:50 +0100
Re: [PATCH v2 3/5] misc: eeprom_93xx46: Implement eeprom_93xx46 DT bindings. Vladimir Zapolskiy <vz@mleia.com> - 2015-11-21 19:40 +0100
Re: [PATCH v2 3/5] misc: eeprom_93xx46: Implement eeprom_93xx46 DT bindings. Cory Tusar <cory.tusar@pid1solutions.com> - 2015-11-23 19:30 +0100
[PATCH v2 4/5] misc: eeprom_93xx46: Add quirks to support Atmel AT93C46D device. Cory Tusar <cory.tusar@pid1solutions.com> - 2015-11-19 04:40 +0100
Re: [PATCH v2 4/5] misc: eeprom_93xx46: Add quirks to support Atmel AT93C46D device. Vladimir Zapolskiy <vz@mleia.com> - 2015-11-19 07:00 +0100
Re: [PATCH v2 4/5] misc: eeprom_93xx46: Add quirks to support Atmel AT93C46D device. Cory Tusar <cory.tusar@pid1solutions.com> - 2015-11-23 18:50 +0100
[PATCH v2 5/5] misc: eeprom_93xx46: Add support for a GPIO 'select' line. Cory Tusar <cory.tusar@pid1solutions.com> - 2015-11-19 04:40 +0100
Re: [PATCH v2 5/5] misc: eeprom_93xx46: Add support for a GPIO 'select' line. Vladimir Zapolskiy <vz@mleia.com> - 2015-11-19 07:10 +0100
Re: [PATCH v2 5/5] misc: eeprom_93xx46: Add support for a GPIO 'select' line. Andrew Lunn <andrew@lunn.ch> - 2015-11-19 15:20 +0100
Re: [PATCH v2 5/5] misc: eeprom_93xx46: Add support for a GPIO 'select' line. Vladimir Zapolskiy <vz@mleia.com> - 2015-11-19 18:00 +0100
Re: [PATCH v2 5/5] misc: eeprom_93xx46: Add support for a GPIO 'select' line. Andrew Lunn <andrew@lunn.ch> - 2015-11-19 18:20 +0100
Re: [PATCH v2 5/5] misc: eeprom_93xx46: Add support for a GPIO 'select' line. Cory Tusar <cory.tusar@pid1solutions.com> - 2015-11-25 06:00 +0100
Page 2 of 2 — ← Prev page 1 [2]
| From | Andrew Lunn <andrew@lunn.ch> |
|---|---|
| Date | 2015-11-19 18:20 +0100 |
| Subject | Re: [PATCH v2 5/5] misc: eeprom_93xx46: Add support for a GPIO 'select' line. |
| Message-ID | <qwB2h-3PG-1@gated-at.bofh.it> |
| In reply to | #1273290 |
On Thu, Nov 19, 2015 at 06:52:57PM +0200, Vladimir Zapolskiy wrote:
> On 19.11.2015 16:18, Andrew Lunn wrote:
> >>> #ifdef CONFIG_OF
> >>> +static void select_assert(void *context)
> >>> +{
> >>> + struct eeprom_93xx46_dev *edev = context;
> >>> +
> >>> + gpiod_set_value_cansleep(gpio_to_desc(edev->pdata->select_gpio), 1);
> >>
> >> I would suggest to use gpio_set_value()
> >
> > Could you explain why?
> >
> > Maybe this gpio is on an SPI GPIO expander?
>
> My point is that gpio_*() interface, gpio_set_value() or
> gpio_set_value_cansleep(), might be preferred is this particular case.
Ah, O.K, yes, avoid the gpio_to_desc() call.
Agreed.
Andrew
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Cory Tusar <cory.tusar@pid1solutions.com> |
|---|---|
| Date | 2015-11-25 06:00 +0100 |
| Subject | Re: [PATCH v2 5/5] misc: eeprom_93xx46: Add support for a GPIO 'select' line. |
| Message-ID | <qyAlr-1hq-3@gated-at.bofh.it> |
| In reply to | #1272866 |
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
On 11/19/2015 01:05 AM, Vladimir Zapolskiy wrote:
> On 19.11.2015 05:29, Cory Tusar wrote:
>> This commit adds support to the eeprom_93x46 driver allowing a GPIO line
>> to function as a 'select' or 'enable' signal prior to accessing the
>> EEPROM.
>>
>> Signed-off-by: Cory Tusar <cory.tusar@pid1solutions.com>
>> ---
>> drivers/misc/eeprom/eeprom_93xx46.c | 26 ++++++++++++++++++++++++++
>> include/linux/eeprom_93xx46.h | 1 +
>> 2 files changed, 27 insertions(+)
>>
>> diff --git a/drivers/misc/eeprom/eeprom_93xx46.c b/drivers/misc/eeprom/eeprom_93xx46.c
>> index 0386b03..375951f 100644
>> --- a/drivers/misc/eeprom/eeprom_93xx46.c
>> +++ b/drivers/misc/eeprom/eeprom_93xx46.c
>> @@ -10,11 +10,14 @@
>>
>> #include <linux/delay.h>
>> #include <linux/device.h>
>> +#include <linux/gpio.h>
>> +#include <linux/gpio/consumer.h>
>> #include <linux/kernel.h>
>> #include <linux/module.h>
>> #include <linux/mutex.h>
>> #include <linux/of.h>
>> #include <linux/of_device.h>
>> +#include <linux/of_gpio.h>
>
> Please double check, adding only linux/of_gpio.h header should work,
> linux/gpio.h and linux/gpio/consumer.h are redundant.
There was an error which turned up on a 0-day build related to this:
tree: https://github.com/lunn/linux.git asl_v4.3-rc2-zii-stable-dsa-reset
head: c91bad95b39a98e0d06809c4c70c9c26747c874a
commit: a3e1b85039c722799102366b527b6bab9543e4ac [4/41] misc: eeprom: 93xx46: Add support for a GPIO 'select' line.
config: x86_64-randconfig-x007-11010710 (attached as .config)
reproduce:
git checkout a3e1b85039c722799102366b527b6bab9543e4ac
# save the attached .config to linux build tree
make ARCH=x86_64
All errors (new ones prefixed by >>):
drivers/misc/eeprom/eeprom_93xx46.c: In function 'select_assert':
>> drivers/misc/eeprom/eeprom_93xx46.c:342:2: error: implicit declaration of function 'gpiod_set_value_cansleep' [-Werror=implicit-function-declaration]
gpiod_set_value_cansleep(gpio_to_desc(edev->pdata->select_gpio), 1);
^
>> drivers/misc/eeprom/eeprom_93xx46.c:342:27: error: implicit declaration of function 'gpio_to_desc' [-Werror=implicit-function-declaration]
gpiod_set_value_cansleep(gpio_to_desc(edev->pdata->select_gpio), 1);
I'll re-check with v3 (where everything uses the gpiod_*() interface) to
see if this can be eliminated...
>> #include <linux/slab.h>
>> #include <linux/spi/spi.h>
>> #include <linux/sysfs.h>
>> @@ -344,6 +347,20 @@ static ssize_t eeprom_93xx46_store_erase(struct device *dev,
>> static DEVICE_ATTR(erase, S_IWUSR, NULL, eeprom_93xx46_store_erase);
>>
>> #ifdef CONFIG_OF
>> +static void select_assert(void *context)
>> +{
>> + struct eeprom_93xx46_dev *edev = context;
>> +
>> + gpiod_set_value_cansleep(gpio_to_desc(edev->pdata->select_gpio), 1);
>
> I would suggest to use gpio_set_value()
v3 uses gpiod_*() throughout. This also addresses an issue where flags
were not being tracked and used properly...
>> +}
>> +
>> +static void select_deassert(void *context)
>> +{
>> + struct eeprom_93xx46_dev *edev = context;
>> +
>> + gpiod_set_value_cansleep(gpio_to_desc(edev->pdata->select_gpio), 0);
>
> Same here.
As above.
>> +}
>> +
>> static const struct of_device_id eeprom_93xx46_of_table[] = {
>> { .compatible = "eeprom-93xx46", },
>> { .compatible = "atmel,at93c46d", .data = &atmel_at93c46d_data, },
>> @@ -385,6 +402,15 @@ static int eeprom_93xx46_probe_dt(struct spi_device *spi)
>> if (of_property_read_bool(np, "read-only"))
>> pd->flags |= EE_READONLY;
>>
>> + ret = of_get_named_gpio(np, "select-gpios", 0);
>
> gpios or gpio? I see only one requested gpio.
gpios - for consistency.
>> + if (ret < 0) {
>> + pd->select_gpio = -1;
>> + } else {
>> + pd->select_gpio = ret;
>> + pd->prepare = select_assert;
>> + pd->finish = select_deassert;
>> + }
>> +
>> if (of_id->data) {
>> const struct eeprom_93xx46_devtype_data *data = of_id->data;
>>
>> diff --git a/include/linux/eeprom_93xx46.h b/include/linux/eeprom_93xx46.h
>> index 92fa4c3..aa472c7 100644
>> --- a/include/linux/eeprom_93xx46.h
>> +++ b/include/linux/eeprom_93xx46.h
>> @@ -21,4 +21,5 @@ struct eeprom_93xx46_platform_data {
>> */
>> void (*prepare)(void *);
>> void (*finish)(void *);
>> + unsigned int select_gpio;
>
> Same questions as in v2 4/5.
I simply see it as more straightforward to keep all platform-specific
data together, rather than mix-and-match between eeprom_93xx46_dev and
eeprom_93xx46_platform_data...
Also, the private eeprom_93xx46_dev structure has not been allocated
prior to parsing for DT bindings (without additional restructuring of
.probe() logic).
>> };
>>
>
> --
> With best wishes,
> Vladimir
>
- --
Cory Tusar
Principal
PID 1 Solutions, Inc.
"There are two ways of constructing a software design. One way is to
make it so simple that there are obviously no deficiencies, and the
other way is to make it so complicated that there are no obvious
deficiencies." --Sir Charles Anthony Richard Hoare
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2
iEUEARECAAYFAlZVPrUACgkQHT1tsfGwHJ8qxQCdEc01RKpHTX2aQepam4J9AweJ
ODsAmKMxPN+ljW/4vBQ7dr9ZXHcj3HQ=
=XmRI
-----END PGP SIGNATURE-----
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [standalone]
Page 2 of 2 — ← Prev page 1 [2]
Back to top | Article view | linux.kernel
csiph-web