[v2] at86rf230: convert to gpio descriptors
Commit Message
From: Arnd Bergmann <arnd@arndb.de>
There are no remaining in-tree users of the platform_data,
so this driver can be converted to using the simpler gpiod
interfaces.
Any out-of-tree users that rely on the platform data can
provide the data using the device_property and gpio_lookup
interfaces instead.
Reviewed-by: Miquel Raynal <miquel.raynal@bootlin.com>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
drivers/net/ieee802154/at86rf230.c | 82 +++++++++---------------------
include/linux/spi/at86rf230.h | 20 --------
2 files changed, 25 insertions(+), 77 deletions(-)
delete mode 100644 include/linux/spi/at86rf230.h
Comments
Hello Arnd.
On 26.01.23 17:22, Arnd Bergmann wrote:
> From: Arnd Bergmann <arnd@arndb.de>
>
> There are no remaining in-tree users of the platform_data,
> so this driver can be converted to using the simpler gpiod
> interfaces.
>
> Any out-of-tree users that rely on the platform data can
> provide the data using the device_property and gpio_lookup
> interfaces instead.
>
> Reviewed-by: Miquel Raynal <miquel.raynal@bootlin.com>
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
> drivers/net/ieee802154/at86rf230.c | 82 +++++++++---------------------
> include/linux/spi/at86rf230.h | 20 --------
> 2 files changed, 25 insertions(+), 77 deletions(-)
> delete mode 100644 include/linux/spi/at86rf230.h
>
This patch has been applied to the wpan-next tree and will be
part of the next pull request to net-next. Thanks!
regards
Stefan Schmidt
Hi Arnd,
On Thu, Jan 26, 2023 at 8:32 AM Arnd Bergmann <arnd@kernel.org> wrote:
>
> /* Reset */
> - if (gpio_is_valid(rstn)) {
> + if (rstn) {
> udelay(1);
> - gpio_set_value_cansleep(rstn, 0);
> + gpiod_set_value_cansleep(rstn, 0);
> udelay(1);
> - gpio_set_value_cansleep(rstn, 1);
> + gpiod_set_value_cansleep(rstn, 1);
For gpiod conversions, if we are not willing to chase whether existing
DTSes specify polarities
properly and create workarounds in case they are wrong, we should use
gpiod_set_raw_value*()
(my preference would be to do the work and not use "raw" variants).
In this particular case, arch/arm/boot/dts/vf610-zii-dev-rev-c.dts
defines reset line as active low,
so you are leaving the device in reset state.
Please review your other conversion patches.
On Tue, Jan 31, 2023 at 3:52 PM Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
>
> Hi Arnd,
>
> On Thu, Jan 26, 2023 at 8:32 AM Arnd Bergmann <arnd@kernel.org> wrote:
> >
> > /* Reset */
> > - if (gpio_is_valid(rstn)) {
> > + if (rstn) {
> > udelay(1);
> > - gpio_set_value_cansleep(rstn, 0);
> > + gpiod_set_value_cansleep(rstn, 0);
> > udelay(1);
> > - gpio_set_value_cansleep(rstn, 1);
> > + gpiod_set_value_cansleep(rstn, 1);
>
> For gpiod conversions, if we are not willing to chase whether existing
> DTSes specify polarities
> properly and create workarounds in case they are wrong, we should use
> gpiod_set_raw_value*()
> (my preference would be to do the work and not use "raw" variants).
>
> In this particular case, arch/arm/boot/dts/vf610-zii-dev-rev-c.dts
> defines reset line as active low,
> so you are leaving the device in reset state.
>
> Please review your other conversion patches.
We also can not change the names of requested GPIOs from "reset-gpio"
to "rstn-gpios" and expect
this to work.
Stefan, please consider reverting this and applying a couple of
patches I will send out shortly.
Thanks.
Hi Dmitry,
dmitry.torokhov@gmail.com wrote on Tue, 31 Jan 2023 16:50:07 -0800:
> On Tue, Jan 31, 2023 at 3:52 PM Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
> >
> > Hi Arnd,
> >
> > On Thu, Jan 26, 2023 at 8:32 AM Arnd Bergmann <arnd@kernel.org> wrote:
> > >
> > > /* Reset */
> > > - if (gpio_is_valid(rstn)) {
> > > + if (rstn) {
> > > udelay(1);
> > > - gpio_set_value_cansleep(rstn, 0);
> > > + gpiod_set_value_cansleep(rstn, 0);
> > > udelay(1);
> > > - gpio_set_value_cansleep(rstn, 1);
> > > + gpiod_set_value_cansleep(rstn, 1);
> >
> > For gpiod conversions, if we are not willing to chase whether existing
> > DTSes specify polarities
> > properly and create workarounds in case they are wrong, we should use
> > gpiod_set_raw_value*()
> > (my preference would be to do the work and not use "raw" variants).
> >
> > In this particular case, arch/arm/boot/dts/vf610-zii-dev-rev-c.dts
> > defines reset line as active low,
> > so you are leaving the device in reset state.
You mean the semantics of gpio_set_value() gpiod_set_value() are
different? Looking at your patch it looks like gpio_set_value() asserts
a physical line state (high or low) while gpiod_set_value() would
actually try to assert a logical state (enabled or disabled) with the
meaning of those being possibly inverted thanks to the DT polarities.
Am I getting this right?
> > Please review your other conversion patches.
>
> We also can not change the names of requested GPIOs from "reset-gpio"
> to "rstn-gpios" and expect
> this to work.
Yep, missed that indeed.
>
> Stefan, please consider reverting this and applying a couple of
> patches I will send out shortly.
If my above understanding is right, then, yeah, the current
patches need to be fixed.
Thanks,
Miquèl
Hello Dmitry.
On 01.02.23 01:50, Dmitry Torokhov wrote:
> On Tue, Jan 31, 2023 at 3:52 PM Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
>>
>> Hi Arnd,
>>
>> On Thu, Jan 26, 2023 at 8:32 AM Arnd Bergmann <arnd@kernel.org> wrote:
>>>
>>> /* Reset */
>>> - if (gpio_is_valid(rstn)) {
>>> + if (rstn) {
>>> udelay(1);
>>> - gpio_set_value_cansleep(rstn, 0);
>>> + gpiod_set_value_cansleep(rstn, 0);
>>> udelay(1);
>>> - gpio_set_value_cansleep(rstn, 1);
>>> + gpiod_set_value_cansleep(rstn, 1);
>>
>> For gpiod conversions, if we are not willing to chase whether existing
>> DTSes specify polarities
>> properly and create workarounds in case they are wrong, we should use
>> gpiod_set_raw_value*()
>> (my preference would be to do the work and not use "raw" variants).
>>
>> In this particular case, arch/arm/boot/dts/vf610-zii-dev-rev-c.dts
>> defines reset line as active low,
>> so you are leaving the device in reset state.
>>
>> Please review your other conversion patches.
>
> We also can not change the names of requested GPIOs from "reset-gpio"
> to "rstn-gpios" and expect
> this to work.
>
> Stefan, please consider reverting this and applying a couple of
> patches I will send out shortly.
Thanks for having another look at these patches. Do you have the same
concern for the convesion patch to cc2520 that has been posted and
applied as well?
Arnd, if you have any concerns about the revert please speak up soon as
I am going to revert your patch and get these patches into my tree later
today.
regards
Stefan Schmidt
On Wed, Feb 01, 2023 at 01:42:37PM +0100, Stefan Schmidt wrote:
> Hello Dmitry.
>
> On 01.02.23 01:50, Dmitry Torokhov wrote:
> > On Tue, Jan 31, 2023 at 3:52 PM Dmitry Torokhov
> > <dmitry.torokhov@gmail.com> wrote:
> > >
> > > Hi Arnd,
> > >
> > > On Thu, Jan 26, 2023 at 8:32 AM Arnd Bergmann <arnd@kernel.org> wrote:
> > > >
> > > > /* Reset */
> > > > - if (gpio_is_valid(rstn)) {
> > > > + if (rstn) {
> > > > udelay(1);
> > > > - gpio_set_value_cansleep(rstn, 0);
> > > > + gpiod_set_value_cansleep(rstn, 0);
> > > > udelay(1);
> > > > - gpio_set_value_cansleep(rstn, 1);
> > > > + gpiod_set_value_cansleep(rstn, 1);
> > >
> > > For gpiod conversions, if we are not willing to chase whether existing
> > > DTSes specify polarities
> > > properly and create workarounds in case they are wrong, we should use
> > > gpiod_set_raw_value*()
> > > (my preference would be to do the work and not use "raw" variants).
> > >
> > > In this particular case, arch/arm/boot/dts/vf610-zii-dev-rev-c.dts
> > > defines reset line as active low,
> > > so you are leaving the device in reset state.
> > >
> > > Please review your other conversion patches.
> >
> > We also can not change the names of requested GPIOs from "reset-gpio"
> > to "rstn-gpios" and expect
> > this to work.
> >
> > Stefan, please consider reverting this and applying a couple of
> > patches I will send out shortly.
>
> Thanks for having another look at these patches. Do you have the same
> concern for the convesion patch to cc2520 that has been posted and applied
> as well?
There are no DT users of cc2520 in the tree, so while ideally reset line
should not be left in "logical active" state at the end of the probe, we
can deal with this in a follow up patch, I doubt it will lead to
regressions as it is.
If I were really nitpicky I would adjust error messages when we fail to
get GPIOs, but again, can be done as a followup.
>
> Arnd, if you have any concerns about the revert please speak up soon as I am
> going to revert your patch and get these patches into my tree later today.
>
Thanks.
Hi Miquel,
On Wed, Feb 01, 2023 at 09:33:32AM +0100, Miquel Raynal wrote:
> Hi Dmitry,
>
> dmitry.torokhov@gmail.com wrote on Tue, 31 Jan 2023 16:50:07 -0800:
>
> > On Tue, Jan 31, 2023 at 3:52 PM Dmitry Torokhov
> > <dmitry.torokhov@gmail.com> wrote:
> > >
> > > Hi Arnd,
> > >
> > > On Thu, Jan 26, 2023 at 8:32 AM Arnd Bergmann <arnd@kernel.org> wrote:
> > > >
> > > > /* Reset */
> > > > - if (gpio_is_valid(rstn)) {
> > > > + if (rstn) {
> > > > udelay(1);
> > > > - gpio_set_value_cansleep(rstn, 0);
> > > > + gpiod_set_value_cansleep(rstn, 0);
> > > > udelay(1);
> > > > - gpio_set_value_cansleep(rstn, 1);
> > > > + gpiod_set_value_cansleep(rstn, 1);
> > >
> > > For gpiod conversions, if we are not willing to chase whether existing
> > > DTSes specify polarities
> > > properly and create workarounds in case they are wrong, we should use
> > > gpiod_set_raw_value*()
> > > (my preference would be to do the work and not use "raw" variants).
> > >
> > > In this particular case, arch/arm/boot/dts/vf610-zii-dev-rev-c.dts
> > > defines reset line as active low,
> > > so you are leaving the device in reset state.
>
> You mean the semantics of gpio_set_value() gpiod_set_value() are
> different? Looking at your patch it looks like gpio_set_value() asserts
> a physical line state (high or low) while gpiod_set_value() would
> actually try to assert a logical state (enabled or disabled) with the
> meaning of those being possibly inverted thanks to the DT polarities.
> Am I getting this right?
Right. If one wants to do physical levels, they need to use gpiod "raw"
APIs.
Thanks.
On Wed, Feb 01, 2023 at 01:42:37PM +0100, Stefan Schmidt wrote:
> On 01.02.23 01:50, Dmitry Torokhov wrote:
...
> Thanks for having another look at these patches.
+1 here. I dunno where I was when reviewing these changes...
Hello.
On 01.02.23 17:19, Dmitry Torokhov wrote:
> On Wed, Feb 01, 2023 at 01:42:37PM +0100, Stefan Schmidt wrote:
>> Hello Dmitry.
>>
>> On 01.02.23 01:50, Dmitry Torokhov wrote:
>>> On Tue, Jan 31, 2023 at 3:52 PM Dmitry Torokhov
>>> <dmitry.torokhov@gmail.com> wrote:
>>>>
>>>> Hi Arnd,
>>>>
>>>> On Thu, Jan 26, 2023 at 8:32 AM Arnd Bergmann <arnd@kernel.org> wrote:
>>>>>
>>>>> /* Reset */
>>>>> - if (gpio_is_valid(rstn)) {
>>>>> + if (rstn) {
>>>>> udelay(1);
>>>>> - gpio_set_value_cansleep(rstn, 0);
>>>>> + gpiod_set_value_cansleep(rstn, 0);
>>>>> udelay(1);
>>>>> - gpio_set_value_cansleep(rstn, 1);
>>>>> + gpiod_set_value_cansleep(rstn, 1);
>>>>
>>>> For gpiod conversions, if we are not willing to chase whether existing
>>>> DTSes specify polarities
>>>> properly and create workarounds in case they are wrong, we should use
>>>> gpiod_set_raw_value*()
>>>> (my preference would be to do the work and not use "raw" variants).
>>>>
>>>> In this particular case, arch/arm/boot/dts/vf610-zii-dev-rev-c.dts
>>>> defines reset line as active low,
>>>> so you are leaving the device in reset state.
>>>>
>>>> Please review your other conversion patches.
>>>
>>> We also can not change the names of requested GPIOs from "reset-gpio"
>>> to "rstn-gpios" and expect
>>> this to work.
>>>
>>> Stefan, please consider reverting this and applying a couple of
>>> patches I will send out shortly.
>>
>> Thanks for having another look at these patches. Do you have the same
>> concern for the convesion patch to cc2520 that has been posted and applied
>> as well?
>
> There are no DT users of cc2520 in the tree, so while ideally reset line
> should not be left in "logical active" state at the end of the probe, we
> can deal with this in a follow up patch, I doubt it will lead to
> regressions as it is.
>
> If I were really nitpicky I would adjust error messages when we fail to
> get GPIOs, but again, can be done as a followup.
Feel free to send patches if you are in the mood on fixing this as well. :-)
>> Arnd, if you have any concerns about the revert please speak up soon as I am
>> going to revert your patch and get these patches into my tree later today.
>>
>
> Thanks.
Reverted and pushed now. Your patches are applied as well. Thanks again
for catching this early on.
regards
Stefan Schmidt
@@ -15,13 +15,12 @@
#include <linux/jiffies.h>
#include <linux/interrupt.h>
#include <linux/irq.h>
-#include <linux/gpio.h>
#include <linux/delay.h>
+#include <linux/gpio/consumer.h>
#include <linux/spi/spi.h>
-#include <linux/spi/at86rf230.h>
+#include <linux/property.h>
#include <linux/regmap.h>
#include <linux/skbuff.h>
-#include <linux/of_gpio.h>
#include <linux/ieee802154.h>
#include <net/mac802154.h>
@@ -82,7 +81,7 @@ struct at86rf230_local {
struct ieee802154_hw *hw;
struct at86rf2xx_chip_data *data;
struct regmap *regmap;
- int slp_tr;
+ struct gpio_desc *slp_tr;
bool sleep;
struct completion state_complete;
@@ -107,8 +106,8 @@ at86rf230_async_state_change(struct at86rf230_local *lp,
static inline void
at86rf230_sleep(struct at86rf230_local *lp)
{
- if (gpio_is_valid(lp->slp_tr)) {
- gpio_set_value(lp->slp_tr, 1);
+ if (lp->slp_tr) {
+ gpiod_set_value(lp->slp_tr, 1);
usleep_range(lp->data->t_off_to_sleep,
lp->data->t_off_to_sleep + 10);
lp->sleep = true;
@@ -118,8 +117,8 @@ at86rf230_sleep(struct at86rf230_local *lp)
static inline void
at86rf230_awake(struct at86rf230_local *lp)
{
- if (gpio_is_valid(lp->slp_tr)) {
- gpio_set_value(lp->slp_tr, 0);
+ if (lp->slp_tr) {
+ gpiod_set_value(lp->slp_tr, 0);
usleep_range(lp->data->t_sleep_to_off,
lp->data->t_sleep_to_off + 100);
lp->sleep = false;
@@ -204,9 +203,9 @@ at86rf230_write_subreg(struct at86rf230_local *lp,
static inline void
at86rf230_slp_tr_rising_edge(struct at86rf230_local *lp)
{
- gpio_set_value(lp->slp_tr, 1);
+ gpiod_set_value(lp->slp_tr, 1);
udelay(1);
- gpio_set_value(lp->slp_tr, 0);
+ gpiod_set_value(lp->slp_tr, 0);
}
static bool
@@ -819,7 +818,7 @@ at86rf230_write_frame_complete(void *context)
ctx->trx.len = 2;
- if (gpio_is_valid(lp->slp_tr))
+ if (lp->slp_tr)
at86rf230_slp_tr_rising_edge(lp);
else
at86rf230_async_write_reg(lp, RG_TRX_STATE, STATE_BUSY_TX, ctx,
@@ -1415,32 +1414,6 @@ static int at86rf230_hw_init(struct at86rf230_local *lp, u8 xtal_trim)
return at86rf230_write_subreg(lp, SR_SLOTTED_OPERATION, 0);
}
-static int
-at86rf230_get_pdata(struct spi_device *spi, int *rstn, int *slp_tr,
- u8 *xtal_trim)
-{
- struct at86rf230_platform_data *pdata = spi->dev.platform_data;
- int ret;
-
- if (!IS_ENABLED(CONFIG_OF) || !spi->dev.of_node) {
- if (!pdata)
- return -ENOENT;
-
- *rstn = pdata->rstn;
- *slp_tr = pdata->slp_tr;
- *xtal_trim = pdata->xtal_trim;
- return 0;
- }
-
- *rstn = of_get_named_gpio(spi->dev.of_node, "reset-gpio", 0);
- *slp_tr = of_get_named_gpio(spi->dev.of_node, "sleep-gpio", 0);
- ret = of_property_read_u8(spi->dev.of_node, "xtal-trim", xtal_trim);
- if (ret < 0 && ret != -EINVAL)
- return ret;
-
- return 0;
-}
-
static int
at86rf230_detect_device(struct at86rf230_local *lp)
{
@@ -1547,7 +1520,8 @@ static int at86rf230_probe(struct spi_device *spi)
struct ieee802154_hw *hw;
struct at86rf230_local *lp;
unsigned int status;
- int rc, irq_type, rstn, slp_tr;
+ int rc, irq_type;
+ struct gpio_desc *rstn, *slp_tr;
u8 xtal_trim = 0;
if (!spi->irq) {
@@ -1555,32 +1529,26 @@ static int at86rf230_probe(struct spi_device *spi)
return -EINVAL;
}
- rc = at86rf230_get_pdata(spi, &rstn, &slp_tr, &xtal_trim);
- if (rc < 0) {
- dev_err(&spi->dev, "failed to parse platform_data: %d\n", rc);
+ rc = device_property_read_u8(&spi->dev, "xtal-trim", &xtal_trim);
+ if (rc < 0 && rc != -EINVAL) {
+ dev_err(&spi->dev, "failed to parse xtal-trim: %d\n", rc);
return rc;
}
- if (gpio_is_valid(rstn)) {
- rc = devm_gpio_request_one(&spi->dev, rstn,
- GPIOF_OUT_INIT_HIGH, "rstn");
- if (rc)
- return rc;
- }
+ rstn = devm_gpiod_get_optional(&spi->dev, "rstn", GPIOD_OUT_HIGH);
+ if (IS_ERR(rstn))
+ return PTR_ERR(rstn);
- if (gpio_is_valid(slp_tr)) {
- rc = devm_gpio_request_one(&spi->dev, slp_tr,
- GPIOF_OUT_INIT_LOW, "slp_tr");
- if (rc)
- return rc;
- }
+ slp_tr = devm_gpiod_get_optional(&spi->dev, "slp_tr", GPIOD_OUT_LOW);
+ if (IS_ERR(slp_tr))
+ return PTR_ERR(slp_tr);
/* Reset */
- if (gpio_is_valid(rstn)) {
+ if (rstn) {
udelay(1);
- gpio_set_value_cansleep(rstn, 0);
+ gpiod_set_value_cansleep(rstn, 0);
udelay(1);
- gpio_set_value_cansleep(rstn, 1);
+ gpiod_set_value_cansleep(rstn, 1);
usleep_range(120, 240);
}
@@ -1682,7 +1650,7 @@ MODULE_DEVICE_TABLE(spi, at86rf230_device_id);
static struct spi_driver at86rf230_driver = {
.id_table = at86rf230_device_id,
.driver = {
- .of_match_table = of_match_ptr(at86rf230_of_match),
+ .of_match_table = at86rf230_of_match,
.name = "at86rf230",
},
.probe = at86rf230_probe,
deleted file mode 100644
@@ -1,20 +0,0 @@
-/* SPDX-License-Identifier: GPL-2.0-only */
-/*
- * AT86RF230/RF231 driver
- *
- * Copyright (C) 2009-2012 Siemens AG
- *
- * Written by:
- * Dmitry Eremin-Solenikov <dmitry.baryshkov@siemens.com>
- */
-#ifndef AT86RF230_H
-#define AT86RF230_H
-
-struct at86rf230_platform_data {
- int rstn;
- int slp_tr;
- int dig2;
- u8 xtal_trim;
-};
-
-#endif