Re: [greybus-dev] [PATCH v2] Staging: greybus: Cleanup in greybus driver
On 04/17 :41, Alex Elder wrote:
> On 4/16/19 5:13 PM, Madhumitha Prabakaran wrote:
> > Fix a blank line after structure declarations. Also, convert
> > macros into inline functions in order to maintain Linux kernel
> > coding style based on which the inline function is
> > preferable over the macro.
>
> Madhumitha, here is my explanation for why *not* to convert these
> container_of() macros to inline functions. It's not just because
> "we do this all over the kernel." The reason is that it actually
> does not improve the code.
>
> Inline functions are often better than macros because they are
> explicit about types, allowing the compiler to tell you if you
> are passing parameters of the right type, and possibly assigning
> results to objects of the right type.
This makes more sense now.
>
> Here is the definition of the container_of() macro in :
>
> #define container_of(ptr, type, member) ({ \
> const typeof(((type *)0)->member) * __mptr = (ptr); \
> (type *)((char *)__mptr - offsetof(type, member)); })
>
> You see that ptr is explicitly assigned to a local variable
> of the type of the member field, so the compiler is able to
> check that assignment. And the return value is similarly
> cast to the type of the containing structure, so the type
> compatibility of the assignment can also be checked. It is
> assumed that where this macro is used, the caller knows it
> is passing an appropriate address.
>
> There is another thing about this particular definition make
> it just as good as an inline function. A macro expansion can
> result in one of its parameters being used more than once, and
> that allows those parameters to be *evaluated* more than once
> in a single statement, which can produce incorrect code.
>
> This macro is defined using the "statement expression"
> extension to C--where a compound statement is enclosed in
> parentheses--({ ... }). This allows a local variable to be
> used in the macro expansion, which avoids any reuse of the
> macro parameters which might cause side-effects.
>
> So anyway, I don't think there is any benefit to replacing
> macros like this that do container_of() with inline functions.
>
> -Alex
Thanks for taking time to explain it in detailed way.
>
> > Blank line fixes are suggested by checkpatch.pl
> >
> > Signed-off-by: Madhumitha Prabakaran
> >
> > Changes in v2:
> > - To maintain consistency in driver greybus, all the instances of macro
> > with container_of are fixed in a single patch.
> > ---
> > drivers/staging/greybus/bundle.h| 6 +-
> > drivers/staging/greybus/control.h | 6 +-
> > drivers/staging/greybus/gbphy.h | 12 ++--
> > drivers/staging/greybus/greybus.h | 6 +-
> > drivers/staging/greybus/hd.h| 6 +-
> > drivers/staging/greybus/interface.h | 6 +-
> > drivers/staging/greybus/module.h| 6 +-
> > drivers/staging/greybus/svc.h | 6 +-
> > 8 files changed, 45 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/staging/greybus/bundle.h
> > b/drivers/staging/greybus/bundle.h
> > index 8734d2055657..84956eedb1c4 100644
> > --- a/drivers/staging/greybus/bundle.h
> > +++ b/drivers/staging/greybus/bundle.h
> > @@ -31,7 +31,11 @@ struct gb_bundle {
> >
> > struct list_headlinks; /* interface->bundles */
> > };
> > -#define to_gb_bundle(d) container_of(d, struct gb_bundle, dev)
> > +
> > +static inline struct gb_bundle *to_gb_bundle(struct device *d)
> > +{
> > + return container_of(d, struct gb_bundle, dev);
> > +}
> >
> > /* Greybus "private" definitions" */
> > struct gb_bundle *gb_bundle_create(struct gb_interface *intf, u8 bundle_id,
> > diff --git a/drivers/staging/greybus/control.h
> > b/drivers/staging/greybus/control.h
> > index 3a29ec05f631..a681ef74e7fe 100644
> > --- a/drivers/staging/greybus/control.h
> > +++ b/drivers/staging/greybus/control.h
> > @@ -24,7 +24,11 @@ struct gb_control {
> > char *vendor_string;
> > char *product_string;
> > };
> > -#define to_gb_control(d) container_of(d, struct gb_control, dev)
> > +
> > +static inline struct gb_control *to_gb_control(struct device *d)
> > +{
> > + return container_of(d, struct gb_control, dev);
> > +}
> >
> > struct gb_control *gb_control_create(struct gb_interface *intf);
> > int gb_control_enable(struct gb_control *control);
> > diff --git a/drivers/staging/greybus/gbphy.h
> > b/drivers/staging/greybus/gbphy.h
> > index 99463489d7d6..20307f6dfcb9 100644
> > --- a/drivers/staging/greybus/gbphy.h
> > +++ b/drivers/staging/greybus/gbphy.h
> > @@ -15,7 +15,11 @@ struct gbphy_device {
> > struct list_head list;
> > struct device dev;
> > };
> > -#define to_gbphy_dev(d) container_of(d, struct gbphy_device, dev)
> > +
> > +static inline struct gbphy_device *to_gbphy_dev(struct device *d)
> > +{
> > + return container_of(d, struct gbphy_device, dev);
> > +}
> >
> >
Re: [PATCH v2] Staging: greybus: Cleanup in greybus driver
On 04/17 :25, Greg KH wrote:
> On Tue, Apr 16, 2019 at 05:13:18PM -0500, Madhumitha Prabakaran wrote:
> > Fix a blank line after structure declarations. Also, convert
> > macros into inline functions in order to maintain Linux kernel
> > coding style based on which the inline function is
> > preferable over the macro.
> >
> > Blank line fixes are suggested by checkpatch.pl
> >
> > Signed-off-by: Madhumitha Prabakaran
> >
> > Changes in v2:
> > - To maintain consistency in driver greybus, all the instances of macro
> > with container_of are fixed in a single patch.
> > ---
> > drivers/staging/greybus/bundle.h| 6 +-
> > drivers/staging/greybus/control.h | 6 +-
> > drivers/staging/greybus/gbphy.h | 12 ++--
> > drivers/staging/greybus/greybus.h | 6 +-
> > drivers/staging/greybus/hd.h| 6 +-
> > drivers/staging/greybus/interface.h | 6 +-
> > drivers/staging/greybus/module.h| 6 +-
> > drivers/staging/greybus/svc.h | 6 +-
> > 8 files changed, 45 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/staging/greybus/bundle.h
> > b/drivers/staging/greybus/bundle.h
> > index 8734d2055657..84956eedb1c4 100644
> > --- a/drivers/staging/greybus/bundle.h
> > +++ b/drivers/staging/greybus/bundle.h
> > @@ -31,7 +31,11 @@ struct gb_bundle {
> >
> > struct list_headlinks; /* interface->bundles */
> > };
> > -#define to_gb_bundle(d) container_of(d, struct gb_bundle, dev)
> > +
> > +static inline struct gb_bundle *to_gb_bundle(struct device *d)
> > +{
> > + return container_of(d, struct gb_bundle, dev);
> > +}
>
> Why are we changing this to an inline function? The #define is fine,
> and how we "normally" do this type of container_of conversion.
>
> I understand the objection of the "no blank line", but this was the
> "original" style that we used to create these #defines from the very
> beginning of the driver model work a decade ago. Changing that
> muscle-memory is going to be hard for some of us. Look at
> drivers/base/base.h for other examples of this.
Thanks for explaining it.
>
> thanks,
>
> greg k-h
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v2] staging: greybus: power_supply: use struct_size() helper
Make use of the struct_size() helper instead of an open-coded version
in order to avoid any potential type mistakes, in particular in the
context in which this code is being used.
So, replace code of the following form:
sizeof(*resp) + props_count * sizeof(struct gb_power_supply_props_desc)
with:
struct_size(resp, props, props_count)
This code was detected with the help of Coccinelle.
Signed-off-by: Gustavo A. R. Silva
---
Changes in v2:
- Rebase on top of 47830c1127ef ("staging: greybus: power_supply: fix
prop-descriptor request size")
drivers/staging/greybus/power_supply.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/staging/greybus/power_supply.c
b/drivers/staging/greybus/power_supply.c
index ae5c0285a942..34b40a409ea3 100644
--- a/drivers/staging/greybus/power_supply.c
+++ b/drivers/staging/greybus/power_supply.c
@@ -520,8 +520,8 @@ static int gb_power_supply_prop_descriptors_get(struct
gb_power_supply *gbpsy)
op = gb_operation_create(connection,
GB_POWER_SUPPLY_TYPE_GET_PROP_DESCRIPTORS,
-sizeof(*req), sizeof(*resp) + props_count *
-sizeof(struct gb_power_supply_props_desc),
+sizeof(*req),
+struct_size(resp, props, props_count),
GFP_KERNEL);
if (!op)
return -ENOMEM;
--
2.21.0
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v4 12/12] staging: most: Documentation: update driver documentation
Hi Chris, We really appreciate your feedback. Thanks for that. For us going the vanilla way roughly means backporting to v4.14: - v5.1-rc5 MOST state (easy/straightforward) - MOST patches from either https://github.com/CogentEmbedded/meta-rcar/ or https://github.com/microchip-ais/meta-medialb-rcar - A number of patches from mld-1.8.0 >From the above list, the last step looks a bit more painful, but we will try to deal with that. If there are any mainline-relevant fixes developed on the way, we hope to find some time to submit them to you and get feedback. Best regards, Eugeniu. On Wed, Apr 17, 2019 at 01:45:41PM +, christian.gr...@microchip.com wrote: > On Mo, 2019-04-15 at 23:21 +0200, Eugeniu Rosca wrote: > > External E-Mail > > > > > > Hello Christian, hello Andrey, > > cc: Vladimir Barinov > > cc: linux-renesas-soc > > > > Our team currently has the requirement of adding MOST support to > > the v4.14-based R-Car3 kernel [1], matching the level of features > > and interfaces of mld-1.8.0 [2] release. > > > > Based on a quick check [3], the mld-1.8.0 release contains 221 non- > > merge > > non-mainline commits. It seems like at least some (most?) of them > > have > > been reworked during upstreaming and are now part of vanilla, thanks > > to > > your efforts. > > > > Since you've actively participated in pushing the out-of-tree drivers > > to mainline, could you please share your gut feeling whether the > > current mainline state of the MOST subsystem matches the mld-1.8.0 > > release in terms of features and interfaces? > > > > No, it doesn't. The version upstream does not have all the bells > and whistles the mld-1.8.0 version has, yet. Focus of the latest > mainline commits was on the driver model and the switch to configfs. > > > At first glance, such mld-1.8.0 functionalities like "flexible PCM > > rate support" [4], as well as a number of dim2 sysfs entries [5-8] > > appear to be missing in v5.1-rc5. Could you please feedback if these > > have been deliberately dropped or didn't make their way for another > > reason? What would be your recommendation between going the mld-1.8.0 > > and going the v5.1-rc5 way for MOST? > > > > The functionalities you're referring to have _not_ intentionally been > dropped. They will find their way into mainline. If there are urgent > feature requests, we could prioritize them. > > Bottom line is, the upstream version is the one you should be using, > as it is going to replace the mld-1.x branch. This is exactly what we > will soon be doing for AGL by the way. > > thanks, > Chris ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH] staging: comedi: usbduxsigma: Call mutex_destroy() on private mutex
`usbduxsigma_detach()` is the Comedi "detach" handler for the
usbduxsigma driver. When it is called, the private data for the device
is about to be freed. The private date contains a mutex `devpriv->mut`
that was initialized when the private data was allocated. Call
`mutex_destroy()` to mark it as invalid.
The calls to `mutex_lock()` and `mutex_unlock()` in
`usbduxsigma_detach()` are probably not required, especially as the
mutex is about to be destroyed, but leave them alone for now.
Signed-off-by: Ian Abbott
---
drivers/staging/comedi/drivers/usbduxsigma.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/staging/comedi/drivers/usbduxsigma.c
b/drivers/staging/comedi/drivers/usbduxsigma.c
index af5605a875e2..3cc40d2544be 100644
--- a/drivers/staging/comedi/drivers/usbduxsigma.c
+++ b/drivers/staging/comedi/drivers/usbduxsigma.c
@@ -1577,6 +1577,8 @@ static void usbduxsigma_detach(struct comedi_device *dev)
usbduxsigma_free_usb_buffers(dev);
mutex_unlock(>mut);
+
+ mutex_destroy(>mut);
}
static struct comedi_driver usbduxsigma_driver = {
--
2.20.1
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH] staging: comedi: usbduxfast: Call mutex_destroy() on private mutex
`usbduxfast_detach()` is the Comedi "detach" handler for the usbduxfast
driver. When it is called, the private data for the device is about to
be freed. The private date contains a mutex `devpriv->mut` that was
initialized when the private data was allocated. Call `mutex_destroy()`
to mark it as invalid.
The calls to `mutex_lock()` and `mutex_unlock()` in
`usbduxfast_detach()` are probably not required, especially as the mutex
is about to be destroyed, but leave them alone for now.
Signed-off-by: Ian Abbott
---
drivers/staging/comedi/drivers/usbduxfast.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/staging/comedi/drivers/usbduxfast.c
b/drivers/staging/comedi/drivers/usbduxfast.c
index 0d54f394dbd2..04bc488385e6 100644
--- a/drivers/staging/comedi/drivers/usbduxfast.c
+++ b/drivers/staging/comedi/drivers/usbduxfast.c
@@ -993,6 +993,8 @@ static void usbduxfast_detach(struct comedi_device *dev)
kfree(devpriv->duxbuf);
mutex_unlock(>mut);
+
+ mutex_destroy(>mut);
}
static struct comedi_driver usbduxfast_driver = {
--
2.20.1
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH] staging: comedi: usbdux: Call mutex_destroy() on private mutex
`usbdux_detach()` is the Comedi "detach" handler for the usbdux driver.
When it is called, the private data for the device is about to be freed.
The private date contains a mutex `devpriv->mut` that was initialized
when the private data was allocated. Call `mutex_destroy()` to mark it
as invalid.
The calls to `mutex_lock()` and `mutex_unlock()` are probably not
required, especially as the mutex is about to be destroyed, but leave
them alone for now.
Signed-off-by: Ian Abbott
---
drivers/staging/comedi/drivers/usbdux.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/staging/comedi/drivers/usbdux.c
b/drivers/staging/comedi/drivers/usbdux.c
index de177418190f..b8f54b7fb34a 100644
--- a/drivers/staging/comedi/drivers/usbdux.c
+++ b/drivers/staging/comedi/drivers/usbdux.c
@@ -1691,6 +1691,8 @@ static void usbdux_detach(struct comedi_device *dev)
usbdux_free_usb_buffers(dev);
mutex_unlock(>mut);
+
+ mutex_destroy(>mut);
}
static struct comedi_driver usbdux_driver = {
--
2.20.1
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH] staging: comedi: ni_usb6501: Call mutex_destroy() on private mutex
`ni6501_detach()` is the Comedi "detach" handler for the ni_usb6501
driver. It is called when the private data for the device is about to
be freed. The private data contains a mutex `devpriv->mut` that was
initialized when the private data was allocated. Call `mutex_destroy()`
to mark it as invalid.
Also remove the calls to `mutex_lock()` and `mutex_unlock()` from
`ni6501_detach()`. The only other locks of the mutex are by some of the
Comedi instruction handlers that cannot contend with the "detach"
handler for this mutex.
Signed-off-by: Ian Abbott
---
drivers/staging/comedi/drivers/ni_usb6501.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/staging/comedi/drivers/ni_usb6501.c
b/drivers/staging/comedi/drivers/ni_usb6501.c
index 808ed92ed66f..0e38f69aa8b6 100644
--- a/drivers/staging/comedi/drivers/ni_usb6501.c
+++ b/drivers/staging/comedi/drivers/ni_usb6501.c
@@ -564,14 +564,12 @@ static void ni6501_detach(struct comedi_device *dev)
if (!devpriv)
return;
- mutex_lock(>mut);
+ mutex_destroy(>mut);
usb_set_intfdata(intf, NULL);
kfree(devpriv->usb_rx_buf);
kfree(devpriv->usb_tx_buf);
-
- mutex_unlock(>mut);
}
static struct comedi_driver ni6501_driver = {
--
2.20.1
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH] staging: comedi: dt9812: Call mutex_destroy() on private mutex
`dt9812_detach()` is the Comedi "detach" handler for the dt9812 driver.
When it is called, the private data for the device is about to be freed.
The private data contains a mutex `devpriv->mut` that was initialized
when the private data was allocated. Call `mutex_destroy()` to mark it
as invalid.
Also remove the calls to `mutex_lock()` and `mutex_unlock()` from
`dt9812_detach()` as the mutex is only being used around a call to
`usb_set_intfdata()` to clear the USB interface's driver data pointer.
The mutex lock seems redundant here, especially as it is about to be
destroyed.
Signed-off-by: Ian Abbott
---
drivers/staging/comedi/drivers/dt9812.c | 5 +
1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/drivers/staging/comedi/drivers/dt9812.c
b/drivers/staging/comedi/drivers/dt9812.c
index 9f165f1cefa5..634f57730c1e 100644
--- a/drivers/staging/comedi/drivers/dt9812.c
+++ b/drivers/staging/comedi/drivers/dt9812.c
@@ -835,11 +835,8 @@ static void dt9812_detach(struct comedi_device *dev)
if (!devpriv)
return;
- mutex_lock(>mut);
-
+ mutex_destroy(>mut);
usb_set_intfdata(intf, NULL);
-
- mutex_unlock(>mut);
}
static struct comedi_driver dt9812_driver = {
--
2.20.1
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH -next] staging: most: configfs: Make mdev_link_list static
From: YueHaibing
Fix sparse warning:
drivers/staging/most/configfs.c:34:18: warning:
symbol 'mdev_link_list' was not declared. Should it be static?
Reported-by: Hulk Robot
Signed-off-by: YueHaibing
---
drivers/staging/most/configfs.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/staging/most/configfs.c b/drivers/staging/most/configfs.c
index 934fb6d..1d8bf29 100644
--- a/drivers/staging/most/configfs.c
+++ b/drivers/staging/most/configfs.c
@@ -31,7 +31,7 @@ struct mdev_link {
char comp_params[PAGE_SIZE];
};
-struct list_head mdev_link_list;
+static struct list_head mdev_link_list;
static int set_cfg_buffer_size(struct mdev_link *link)
{
--
2.7.4
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 1/2] staging: comedi: Add lockdep_assert_held() calls for dev->mutex
Lots of functions in the core comedi module expect the mutex in `struct
comedi_device` to be held, so add calls to `lockdep_assert_held()` to
check and document that. An unusual case is the calls to
`lockdep_assert_held()` after successful return from
`comedi_alloc_board_minor()` which allocates a `struct comedi_device`
and returns with its mutex locked.
Signed-off-by: Ian Abbott
---
drivers/staging/comedi/comedi_buf.c | 2 ++
drivers/staging/comedi/comedi_fops.c | 32
drivers/staging/comedi/drivers.c | 7 ++
3 files changed, 41 insertions(+)
diff --git a/drivers/staging/comedi/comedi_buf.c
b/drivers/staging/comedi/comedi_buf.c
index f693c2c0bec3..d2c8cc72a99d 100644
--- a/drivers/staging/comedi/comedi_buf.c
+++ b/drivers/staging/comedi/comedi_buf.c
@@ -211,6 +211,8 @@ int comedi_buf_alloc(struct comedi_device *dev, struct
comedi_subdevice *s,
{
struct comedi_async *async = s->async;
+ lockdep_assert_held(>mutex);
+
/* Round up new_size to multiple of PAGE_SIZE */
new_size = (new_size + PAGE_SIZE - 1) & PAGE_MASK;
diff --git a/drivers/staging/comedi/comedi_fops.c
b/drivers/staging/comedi/comedi_fops.c
index 0caae4a5c471..f6d1287c7b83 100644
--- a/drivers/staging/comedi/comedi_fops.c
+++ b/drivers/staging/comedi/comedi_fops.c
@@ -164,6 +164,7 @@ static bool comedi_clear_board_dev(struct comedi_device
*dev)
unsigned int i = dev->minor;
bool cleared = false;
+ lockdep_assert_held(>mutex);
mutex_lock(_board_minor_table_lock);
if (dev == comedi_board_minor_table[i]) {
comedi_board_minor_table[i] = NULL;
@@ -260,6 +261,7 @@ comedi_read_subdevice(const struct comedi_device *dev,
unsigned int minor)
{
struct comedi_subdevice *s;
+ lockdep_assert_held(>mutex);
if (minor >= COMEDI_NUM_BOARD_MINORS) {
s = comedi_subdevice_from_minor(dev, minor);
if (!s || (s->subdev_flags & SDF_CMD_READ))
@@ -273,6 +275,7 @@ comedi_write_subdevice(const struct comedi_device *dev,
unsigned int minor)
{
struct comedi_subdevice *s;
+ lockdep_assert_held(>mutex);
if (minor >= COMEDI_NUM_BOARD_MINORS) {
s = comedi_subdevice_from_minor(dev, minor);
if (!s || (s->subdev_flags & SDF_CMD_WRITE))
@@ -336,6 +339,8 @@ static int resize_async_buffer(struct comedi_device *dev,
struct comedi_async *async = s->async;
int retval;
+ lockdep_assert_held(>mutex);
+
if (new_size > async->max_bufsize)
return -EPERM;
@@ -726,6 +731,7 @@ static void do_become_nonbusy(struct comedi_device *dev,
{
struct comedi_async *async = s->async;
+ lockdep_assert_held(>mutex);
comedi_update_subdevice_runflags(s, COMEDI_SRF_RUNNING, 0);
if (async) {
comedi_buf_reset(s);
@@ -745,6 +751,7 @@ static int do_cancel(struct comedi_device *dev, struct
comedi_subdevice *s)
{
int ret = 0;
+ lockdep_assert_held(>mutex);
if (comedi_is_subdevice_running(s) && s->cancel)
ret = s->cancel(dev, s);
@@ -758,6 +765,7 @@ void comedi_device_cancel_all(struct comedi_device *dev)
struct comedi_subdevice *s;
int i;
+ lockdep_assert_held(>mutex);
if (!dev->attached)
return;
@@ -773,6 +781,7 @@ static int is_device_busy(struct comedi_device *dev)
struct comedi_subdevice *s;
int i;
+ lockdep_assert_held(>mutex);
if (!dev->attached)
return 0;
@@ -805,6 +814,7 @@ static int do_devconfig_ioctl(struct comedi_device *dev,
{
struct comedi_devconfig it;
+ lockdep_assert_held(>mutex);
if (!capable(CAP_SYS_ADMIN))
return -EPERM;
@@ -860,6 +870,7 @@ static int do_bufconfig_ioctl(struct comedi_device *dev,
struct comedi_subdevice *s;
int retval = 0;
+ lockdep_assert_held(>mutex);
if (copy_from_user(, arg, sizeof(bc)))
return -EFAULT;
@@ -920,6 +931,7 @@ static int do_devinfo_ioctl(struct comedi_device *dev,
struct comedi_subdevice *s;
struct comedi_devinfo devinfo;
+ lockdep_assert_held(>mutex);
memset(, 0, sizeof(devinfo));
/* fill devinfo structure */
@@ -966,6 +978,7 @@ static int do_subdinfo_ioctl(struct comedi_device *dev,
struct comedi_subdinfo *tmp, *us;
struct comedi_subdevice *s;
+ lockdep_assert_held(>mutex);
tmp = kcalloc(dev->n_subdevices, sizeof(*tmp), GFP_KERNEL);
if (!tmp)
return -ENOMEM;
@@ -1039,6 +1052,7 @@ static int do_chaninfo_ioctl(struct comedi_device *dev,
struct comedi_subdevice *s;
struct comedi_chaninfo it;
+ lockdep_assert_held(>mutex);
if (copy_from_user(, arg, sizeof(it)))
return -EFAULT;
@@ -1098,6 +1112,7 @@ static int do_bufinfo_ioctl(struct
[PATCH 0/2] staging: comedi: Add lockdep_assert_held() calls
Add some lockdep_assert_held() calls to the Comedi core. 1) staging: comedi: Add lockdep_assert_held() calls for dev->mutex 2) staging: comedi: Add lockdep_assert_held() calls for dev->attach_lock drivers/staging/comedi/comedi_buf.c | 2 ++ drivers/staging/comedi/comedi_fops.c | 32 drivers/staging/comedi/drivers.c | 8 3 files changed, 42 insertions(+) ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 2/2] staging: comedi: Add lockdep_assert_held() calls for dev->attach_lock
There are not a lot of functions in the core comedi module that require
the R/W semaphore `attach_lock` in `struct comedi_device` to be locked
(although there are a few functions that require at least one of
`attach_lock` and `mutex` to be locked). One function that requires the
caller to lock `attach_lock` is `comedi_device_detach_cleanup()` so add
a call to `lockdep_assert_held()` to check and document that.
Signed-off-by: Ian Abbott
---
drivers/staging/comedi/drivers.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/staging/comedi/drivers.c b/drivers/staging/comedi/drivers.c
index f845f95ca765..31223e5a8755 100644
--- a/drivers/staging/comedi/drivers.c
+++ b/drivers/staging/comedi/drivers.c
@@ -159,6 +159,7 @@ static void comedi_device_detach_cleanup(struct
comedi_device *dev)
int i;
struct comedi_subdevice *s;
+ lockdep_assert_held(>attach_lock);
lockdep_assert_held(>mutex);
if (dev->subdevices) {
for (i = 0; i < dev->n_subdevices; i++) {
--
2.20.1
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH] staging: comedi: don't release mutex too early in comedi_auto_config()
`comedi_auto_config()` uses `dev->class_dev` for logging a kernel
message after releasing `dev->mutex`. There is an unlikely possibility
that the Comedi device `dev` will have been removed by the
`COMEDI_DEVCONFIG` ioctl() command. Keep hold of the mutex until the
kernel message has been sent to prevent that. The function can call
`comedi_release_hardware_device()` on error. In that case, the mutex
must be unlocked before that.
Signed-off-by: Ian Abbott
---
drivers/staging/comedi/drivers.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/staging/comedi/drivers.c b/drivers/staging/comedi/drivers.c
index 5a32b8fc000e..b7b9e48d4303 100644
--- a/drivers/staging/comedi/drivers.c
+++ b/drivers/staging/comedi/drivers.c
@@ -1059,12 +1059,12 @@ int comedi_auto_config(struct device *hardware_device,
ret = driver->auto_attach(dev, context);
if (ret >= 0)
ret = comedi_device_postconfig(dev);
- mutex_unlock(>mutex);
if (ret < 0) {
dev_warn(hardware_device,
"driver '%s' failed to auto-configure device.\n",
driver->driver_name);
+ mutex_unlock(>mutex);
comedi_release_hardware_device(hardware_device);
} else {
/*
@@ -1074,6 +1074,7 @@ int comedi_auto_config(struct device *hardware_device,
dev_info(dev->class_dev,
"driver '%s' has successfully auto-configured '%s'.\n",
driver->driver_name, dev->board_name);
+ mutex_unlock(>mutex);
}
return ret;
}
--
2.20.1
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v4 12/12] staging: most: Documentation: update driver documentation
On Mo, 2019-04-15 at 23:21 +0200, Eugeniu Rosca wrote: > External E-Mail > > > Hello Christian, hello Andrey, > cc: Vladimir Barinov > cc: linux-renesas-soc > > Our team currently has the requirement of adding MOST support to > the v4.14-based R-Car3 kernel [1], matching the level of features > and interfaces of mld-1.8.0 [2] release. > > Based on a quick check [3], the mld-1.8.0 release contains 221 non- > merge > non-mainline commits. It seems like at least some (most?) of them > have > been reworked during upstreaming and are now part of vanilla, thanks > to > your efforts. > > Since you've actively participated in pushing the out-of-tree drivers > to mainline, could you please share your gut feeling whether the > current mainline state of the MOST subsystem matches the mld-1.8.0 > release in terms of features and interfaces? > No, it doesn't. The version upstream does not have all the bells and whistles the mld-1.8.0 version has, yet. Focus of the latest mainline commits was on the driver model and the switch to configfs. > At first glance, such mld-1.8.0 functionalities like "flexible PCM > rate support" [4], as well as a number of dim2 sysfs entries [5-8] > appear to be missing in v5.1-rc5. Could you please feedback if these > have been deliberately dropped or didn't make their way for another > reason? What would be your recommendation between going the mld-1.8.0 > and going the v5.1-rc5 way for MOST? > The functionalities you're referring to have _not_ intentionally been dropped. They will find their way into mainline. If there are urgent feature requests, we could prioritize them. Bottom line is, the upstream version is the one you should be using, as it is going to replace the mld-1.x branch. This is exactly what we will soon be doing for AGL by the way. thanks, Chris ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: rtl8723bs: fix spelling mistake: "nonprintabl" -> "non-printable"
On 4/17/2019 5:30 PM, Colin King wrote:
From: Colin Ian King
There is a spelling mistake in an RT_TRACE message, fix it.
Signed-off-by: Colin Ian King
Reviewed-by: Mukesh Ojha
Cheers,
-Mukesh
---
drivers/staging/rtl8723bs/core/rtw_ioctl_set.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/staging/rtl8723bs/core/rtw_ioctl_set.c
b/drivers/staging/rtl8723bs/core/rtw_ioctl_set.c
index 0c8a050b2a81..bd75bca1ac6e 100644
--- a/drivers/staging/rtl8723bs/core/rtw_ioctl_set.c
+++ b/drivers/staging/rtl8723bs/core/rtw_ioctl_set.c
@@ -44,7 +44,7 @@ u8 rtw_validate_ssid(struct ndis_802_11_ssid *ssid)
for (i = 0; i < ssid->SsidLength; i++) {
/* wifi, printable ascii code must be supported */
if (!((ssid->Ssid[i] >= 0x20) && (ssid->Ssid[i] <= 0x7e))) {
- RT_TRACE(_module_rtl871x_ioctl_set_c_, _drv_err_, ("ssid has
nonprintabl ascii\n"));
+ RT_TRACE(_module_rtl871x_ioctl_set_c_, _drv_err_, ("ssid has
non-printable ascii\n"));
ret = false;
break;
}
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: rtlwifi: fix spelling mistake "notity" -> "notify"
On 4/17/2019 5:38 PM, Colin King wrote:
From: Colin Ian King
There are two spelling mistake in RT_TRACE messages. Fix them.
Signed-off-by: Colin Ian King
Reviewed-by: Mukesh Ojha
Cheers,
-Mukesh
---
drivers/staging/rtlwifi/btcoexist/halbtc8822b1ant.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/staging/rtlwifi/btcoexist/halbtc8822b1ant.c
b/drivers/staging/rtlwifi/btcoexist/halbtc8822b1ant.c
index ade271cb4aab..32c797430512 100644
--- a/drivers/staging/rtlwifi/btcoexist/halbtc8822b1ant.c
+++ b/drivers/staging/rtlwifi/btcoexist/halbtc8822b1ant.c
@@ -4730,7 +4730,7 @@ void ex_btc8822b1ant_media_status_notify(struct
btc_coexist *btcoexist, u8 type)
if (wifi_under_b_mode) {
RT_TRACE(
rtlpriv, COMP_BT_COEXIST, DBG_LOUD,
- "[BTCoex], ** (media status notity under b
mode) **\n");
+ "[BTCoex], ** (media status notify under b
mode) **\n");
btcoexist->btc_write_1byte(btcoexist, 0x6cd,
0x00); /* CCK Tx */
btcoexist->btc_write_1byte(btcoexist, 0x6cf,
@@ -4738,7 +4738,7 @@ void ex_btc8822b1ant_media_status_notify(struct
btc_coexist *btcoexist, u8 type)
} else {
RT_TRACE(
rtlpriv, COMP_BT_COEXIST, DBG_LOUD,
- "[BTCoex], ** (media status notity not under
b mode) **\n");
+ "[BTCoex], ** (media status notify not under
b mode) **\n");
btcoexist->btc_write_1byte(btcoexist, 0x6cd,
0x00); /* CCK Tx */
btcoexist->btc_write_1byte(btcoexist, 0x6cf,
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v2 2/2] staging: speakup: refactor to use existing code in vt
This patch replaces speakup's implementations with calls to functions
in tty/vt/selection.c. Those functions are:
cancel_selection()
set_selection_kernel()
paste_selection()
Currently setting selection is done in interrupt context. However,
set_selection_kernel() can sleep - for instance, it requires console_lock
which can sleep. Therefore we offload that work to a work_struct thread,
following the same pattern which was already set for paste_selection().
When setting selection, we also get a reference to tty and make sure to
release the reference before returning.
struct speakup_paste_work has been renamed to the more generic
speakup_selection_work because it is now used for both pasting as well
as setting selection. When paste work is cancelled, the code wasn't
setting tty to NULL. This patch also fixes that by setting tty to NULL
so that in case of failure we don't get EBUSY forever.
Signed-off-by: Okash Khawaja
Reviewed-by: Samuel Thibault
Tested-by: Gregory Nowak
---
drivers/staging/speakup/main.c | 1 +
drivers/staging/speakup/selection.c | 212 +++-
drivers/staging/speakup/speakup.h | 1 +
3 files changed, 88 insertions(+), 126 deletions(-)
diff --git a/drivers/staging/speakup/main.c b/drivers/staging/speakup/main.c
index b6a65b0..488f253 100644
--- a/drivers/staging/speakup/main.c
+++ b/drivers/staging/speakup/main.c
@@ -2319,6 +2319,7 @@ static void __exit speakup_exit(void)
unregister_keyboard_notifier(_notifier_block);
unregister_vt_notifier(_notifier_block);
speakup_unregister_devsynth();
+ speakup_cancel_selection();
speakup_cancel_paste();
del_timer_sync(_timer);
kthread_stop(speakup_task);
diff --git a/drivers/staging/speakup/selection.c
b/drivers/staging/speakup/selection.c
index 0ed1fef..a8b4d0c 100644
--- a/drivers/staging/speakup/selection.c
+++ b/drivers/staging/speakup/selection.c
@@ -9,178 +9,138 @@
#include
#include
#include
+#include
#include "speakup.h"
-/* -- cut and paste - */
-/* Don't take this from : 011-015 on the screen aren't spaces */
-#define ishardspace(c) ((c) == ' ')
-
unsigned short spk_xs, spk_ys, spk_xe, spk_ye; /* our region points */
-
-/* Variables for selection control. */
-/* must not be deallocated */
struct vc_data *spk_sel_cons;
-/* cleared by clear_selection */
-static int sel_start = -1;
-static int sel_end;
-static int sel_buffer_lth;
-static char *sel_buffer;
-static unsigned char sel_pos(int n)
-{
- return inverse_translate(spk_sel_cons,
- screen_glyph(spk_sel_cons, n), 0);
-}
+struct speakup_selection_work {
+ struct work_struct work;
+ struct tiocl_selection sel;
+ struct tty_struct *tty;
+};
void speakup_clear_selection(void)
{
- sel_start = -1;
+ console_lock();
+ clear_selection();
+ console_unlock();
}
-/* does screen address p correspond to character at LH/RH edge of screen? */
-static int atedge(const int p, int size_row)
+static void __speakup_set_selection(struct work_struct *work)
{
- return !(p % size_row) || !((p + 2) % size_row);
-}
+ struct speakup_selection_work *ssw =
+ container_of(work, struct speakup_selection_work, work);
-/* constrain v such that v <= u */
-static unsigned short limit(const unsigned short v, const unsigned short u)
-{
- return (v > u) ? u : v;
-}
+ struct tty_struct *tty;
+ struct tiocl_selection sel;
-int speakup_set_selection(struct tty_struct *tty)
-{
- int new_sel_start, new_sel_end;
- char *bp, *obp;
- int i, ps, pe;
- struct vc_data *vc = vc_cons[fg_console].d;
+ sel = ssw->sel;
- spk_xs = limit(spk_xs, vc->vc_cols - 1);
- spk_ys = limit(spk_ys, vc->vc_rows - 1);
- spk_xe = limit(spk_xe, vc->vc_cols - 1);
- spk_ye = limit(spk_ye, vc->vc_rows - 1);
- ps = spk_ys * vc->vc_size_row + (spk_xs << 1);
- pe = spk_ye * vc->vc_size_row + (spk_xe << 1);
+ /* this ensures we copy sel before releasing the lock below */
+ rmb();
- if (ps > pe)/* make sel_start <= sel_end */
- swap(ps, pe);
+ /* release the lock by setting tty of the struct to NULL */
+ tty = xchg(>tty, NULL);
if (spk_sel_cons != vc_cons[fg_console].d) {
- speakup_clear_selection();
spk_sel_cons = vc_cons[fg_console].d;
- dev_warn(tty->dev,
-"Selection: mark console not the same as cut\n");
- return -EINVAL;
+ pr_warn("Selection: mark console not the same as cut\n");
+ goto unref;
}
- new_sel_start = ps;
- new_sel_end = pe;
-
- /* select to end of line if on trailing space */
- if (new_sel_end > new_sel_start &&
- !atedge(new_sel_end, vc->vc_size_row) &&
- ishardspace(sel_pos(new_sel_end))) {
- for (pe =
[PATCH v2 1/2] vt: selection: allow functions to be called from inside kernel
This patch breaks set_selection() into two functions so that when
called from kernel, copy_from_user() can be avoided. The two functions
are called set_selection_user() and set_selection_kernel() in order to
be explicit about their purposes. This also means updating any
references to set_selection() and fixing for name change. It also
exports set_selection_kernel() and paste_selection().
These changes are used the following patch where speakup's selection
functionality calls into the above functions, thereby doing away with
parallel implementation.
Signed-off-by: Okash Khawaja
Reviewed-by: Samuel Thibault
Tested-by: Gregory Nowak
---
drivers/tty/vt/selection.c | 46 ++
drivers/tty/vt/vt.c| 7 ---
include/linux/selection.h | 7 ---
3 files changed, 38 insertions(+), 22 deletions(-)
diff --git a/drivers/tty/vt/selection.c b/drivers/tty/vt/selection.c
index 07496c7..78732fe 100644
--- a/drivers/tty/vt/selection.c
+++ b/drivers/tty/vt/selection.c
@@ -2,7 +2,9 @@
/*
* This module exports the functions:
*
- * 'int set_selection(struct tiocl_selection __user *, struct tty_struct
*)'
+ * 'int set_selection_user(struct tiocl_selection __user *,
+ *struct tty_struct *)'
+ * 'int set_selection_kernel(struct tiocl_selection *, struct tty_struct
*)'
* 'void clear_selection(void)'
* 'int paste_selection(struct tty_struct *)'
* 'int sel_loadlut(char __user *)'
@@ -80,6 +82,7 @@ void clear_selection(void)
sel_start = -1;
}
}
+EXPORT_SYMBOL_GPL(clear_selection);
/*
* User settable table: what characters are to be considered alphabetic?
@@ -154,7 +157,7 @@ static int store_utf8(u32 c, char *p)
}
/**
- * set_selection - set the current selection.
+ * set_selection_user - set the current selection.
* @sel: user selection info
* @tty: the console tty
*
@@ -163,35 +166,44 @@ static int store_utf8(u32 c, char *p)
* The entire selection process is managed under the console_lock. It's
* a lot under the lock but its hardly a performance path
*/
-int set_selection(const struct tiocl_selection __user *sel, struct tty_struct
*tty)
+int set_selection_user(const struct tiocl_selection __user *sel,
+ struct tty_struct *tty)
+{
+ struct tiocl_selection v;
+
+ if (copy_from_user(, sel, sizeof(*sel)))
+ return -EFAULT;
+
+ return set_selection_kernel(, tty);
+}
+
+int set_selection_kernel(struct tiocl_selection *v, struct tty_struct *tty)
{
struct vc_data *vc = vc_cons[fg_console].d;
int new_sel_start, new_sel_end, spc;
- struct tiocl_selection v;
char *bp, *obp;
int i, ps, pe, multiplier;
u32 c;
int mode;
poke_blanked_console();
- if (copy_from_user(, sel, sizeof(*sel)))
- return -EFAULT;
- v.xs = min_t(u16, v.xs - 1, vc->vc_cols - 1);
- v.ys = min_t(u16, v.ys - 1, vc->vc_rows - 1);
- v.xe = min_t(u16, v.xe - 1, vc->vc_cols - 1);
- v.ye = min_t(u16, v.ye - 1, vc->vc_rows - 1);
- ps = v.ys * vc->vc_size_row + (v.xs << 1);
- pe = v.ye * vc->vc_size_row + (v.xe << 1);
+ v->xs = min_t(u16, v->xs - 1, vc->vc_cols - 1);
+ v->ys = min_t(u16, v->ys - 1, vc->vc_rows - 1);
+ v->xe = min_t(u16, v->xe - 1, vc->vc_cols - 1);
+ v->ye = min_t(u16, v->ye - 1, vc->vc_rows - 1);
+ ps = v->ys * vc->vc_size_row + (v->xs << 1);
+ pe = v->ye * vc->vc_size_row + (v->xe << 1);
- if (v.sel_mode == TIOCL_SELCLEAR) {
+ if (v->sel_mode == TIOCL_SELCLEAR) {
/* useful for screendump without selection highlights */
clear_selection();
return 0;
}
- if (mouse_reporting() && (v.sel_mode & TIOCL_SELMOUSEREPORT)) {
- mouse_report(tty, v.sel_mode & TIOCL_SELBUTTONMASK, v.xs, v.ys);
+ if (mouse_reporting() && (v->sel_mode & TIOCL_SELMOUSEREPORT)) {
+ mouse_report(tty, v->sel_mode & TIOCL_SELBUTTONMASK, v->xs,
+v->ys);
return 0;
}
@@ -208,7 +220,7 @@ int set_selection(const struct tiocl_selection __user *sel,
struct tty_struct *t
else
use_unicode = 0;
- switch (v.sel_mode)
+ switch (v->sel_mode)
{
case TIOCL_SELCHAR: /* character-by-character selection */
new_sel_start = ps;
@@ -322,6 +334,7 @@ int set_selection(const struct tiocl_selection __user *sel,
struct tty_struct *t
sel_buffer_lth = bp - sel_buffer;
return 0;
}
+EXPORT_SYMBOL_GPL(set_selection_kernel);
/* Insert the contents of the selection buffer into the
* queue of the tty associated with the current console.
@@ -367,3 +380,4 @@ int paste_selection(struct tty_struct *tty)
tty_ldisc_deref(ld);
[PATCH v2 0/2] staging: speakup: factor out selection code
Hi, The v2 renames set_selection() and do_set_selection() to following more explicit names: set_selection_user() /* includes copying data from user space */ set_selection_kernel() /* no copying from user space */ The patches also update references to set_selection() to be set_selection_user(). Original intro: Speakup's selection functionality parallels that of drivers/tty/vt/selection.c. This patch set replaces speakup's implementation with calls to vt's selection code. This is one of the remaining items in our TODO file and it's needed for moving speakup out of staging. Please note that in speakup selection is set inside interrupt context of keyboard handler. Set selection code in vt happens in process context and hence expects ability to sleep. To address this, there were two options: farm out speakup's set selection into a work_struct thread, or create atomic version of vt's set_selection. These patches implement the former option. Here's a summary: Patch 1 re-arranges code in vt and exports some functions. Patch 2 replaces speakup's selection code with calls to vt's functions exported in patch 1. Thanks, Okash ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH] staging: rtlwifi: fix spelling mistake "notity" -> "notify"
From: Colin Ian King
There are two spelling mistake in RT_TRACE messages. Fix them.
Signed-off-by: Colin Ian King
---
drivers/staging/rtlwifi/btcoexist/halbtc8822b1ant.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/staging/rtlwifi/btcoexist/halbtc8822b1ant.c
b/drivers/staging/rtlwifi/btcoexist/halbtc8822b1ant.c
index ade271cb4aab..32c797430512 100644
--- a/drivers/staging/rtlwifi/btcoexist/halbtc8822b1ant.c
+++ b/drivers/staging/rtlwifi/btcoexist/halbtc8822b1ant.c
@@ -4730,7 +4730,7 @@ void ex_btc8822b1ant_media_status_notify(struct
btc_coexist *btcoexist, u8 type)
if (wifi_under_b_mode) {
RT_TRACE(
rtlpriv, COMP_BT_COEXIST, DBG_LOUD,
- "[BTCoex], ** (media status notity
under b mode) **\n");
+ "[BTCoex], ** (media status notify
under b mode) **\n");
btcoexist->btc_write_1byte(btcoexist, 0x6cd,
0x00); /* CCK Tx */
btcoexist->btc_write_1byte(btcoexist, 0x6cf,
@@ -4738,7 +4738,7 @@ void ex_btc8822b1ant_media_status_notify(struct
btc_coexist *btcoexist, u8 type)
} else {
RT_TRACE(
rtlpriv, COMP_BT_COEXIST, DBG_LOUD,
- "[BTCoex], ** (media status notity not
under b mode) **\n");
+ "[BTCoex], ** (media status notify not
under b mode) **\n");
btcoexist->btc_write_1byte(btcoexist, 0x6cd,
0x00); /* CCK Tx */
btcoexist->btc_write_1byte(btcoexist, 0x6cf,
--
2.20.1
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH] staging: rtl8723bs: fix spelling mistake: "nonprintabl" -> "non-printable"
From: Colin Ian King
There is a spelling mistake in an RT_TRACE message, fix it.
Signed-off-by: Colin Ian King
---
drivers/staging/rtl8723bs/core/rtw_ioctl_set.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/staging/rtl8723bs/core/rtw_ioctl_set.c
b/drivers/staging/rtl8723bs/core/rtw_ioctl_set.c
index 0c8a050b2a81..bd75bca1ac6e 100644
--- a/drivers/staging/rtl8723bs/core/rtw_ioctl_set.c
+++ b/drivers/staging/rtl8723bs/core/rtw_ioctl_set.c
@@ -44,7 +44,7 @@ u8 rtw_validate_ssid(struct ndis_802_11_ssid *ssid)
for (i = 0; i < ssid->SsidLength; i++) {
/* wifi, printable ascii code must be supported */
if (!((ssid->Ssid[i] >= 0x20) && (ssid->Ssid[i] <= 0x7e))) {
- RT_TRACE(_module_rtl871x_ioctl_set_c_, _drv_err_,
("ssid has nonprintabl ascii\n"));
+ RT_TRACE(_module_rtl871x_ioctl_set_c_, _drv_err_,
("ssid has non-printable ascii\n"));
ret = false;
break;
}
--
2.20.1
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v2 4/4] staging: mt7621-pci-phy: convert driver to use kernel regmap API's
Instead of using writel and readl use regmap API which makes
the driver maintainability easier.
Signed-off-by: Sergio Paracuellos
---
.../staging/mt7621-pci-phy/pci-mt7621-phy.c | 88 ---
1 file changed, 54 insertions(+), 34 deletions(-)
diff --git a/drivers/staging/mt7621-pci-phy/pci-mt7621-phy.c
b/drivers/staging/mt7621-pci-phy/pci-mt7621-phy.c
index f762369d6792..2576f179e30a 100644
--- a/drivers/staging/mt7621-pci-phy/pci-mt7621-phy.c
+++ b/drivers/staging/mt7621-pci-phy/pci-mt7621-phy.c
@@ -11,6 +11,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -95,6 +96,7 @@ struct mt7621_pci_phy_instance {
/**
* struct mt7621_pci_phy - Mt7621 Pcie PHY core
* @dev: pointer to device
+ * @regmap: kernel regmap pointer
* @phys: pointer to Mt7621 PHY device
* @nphys: number of PHY devices for this core
* @bypass_pipe_rst: mark if 'mt7621_bypass_pipe_rst'
@@ -102,20 +104,24 @@ struct mt7621_pci_phy_instance {
*/
struct mt7621_pci_phy {
struct device *dev;
+ struct regmap *regmap;
struct mt7621_pci_phy_instance **phys;
int nphys;
bool bypass_pipe_rst;
};
-static inline u32 phy_read(struct mt7621_pci_phy_instance *instance, u32 reg)
+static inline u32 phy_read(struct mt7621_pci_phy *phy, u32 reg)
{
- return readl(instance->port_base + reg);
+ u32 val;
+
+ regmap_read(phy->regmap, reg, );
+
+ return val;
}
-static inline void phy_write(struct mt7621_pci_phy_instance *instance,
-u32 val, u32 reg)
+static inline void phy_write(struct mt7621_pci_phy *phy, u32 val, u32 reg)
{
- writel(val, instance->port_base + reg);
+ regmap_write(phy->regmap, reg, val);
}
static void mt7621_bypass_pipe_rst(struct mt7621_pci_phy *phy,
@@ -125,10 +131,10 @@ static void mt7621_bypass_pipe_rst(struct mt7621_pci_phy
*phy,
RG_PE1_PIPE_REG : RG_PE1_PIPE_REG + RG_P0_TO_P1_WIDTH;
u32 reg;
- reg = phy_read(instance, offset);
+ reg = phy_read(phy, offset);
reg &= ~(RG_PE1_PIPE_RST | RG_PE1_PIPE_CMD_FRC);
reg |= (RG_PE1_PIPE_RST | RG_PE1_PIPE_CMD_FRC);
- phy_write(instance, reg, offset);
+ phy_write(phy, reg, offset);
}
static void mt7621_set_phy_for_ssc(struct mt7621_pci_phy *phy,
@@ -142,72 +148,72 @@ static void mt7621_set_phy_for_ssc(struct mt7621_pci_phy
*phy,
reg = (reg >> 6) & 0x7;
/* Set PCIe Port PHY to disable SSC */
/* Debug Xtal Type */
- val = phy_read(instance, RG_PE1_FRC_H_XTAL_REG);
+ val = phy_read(phy, RG_PE1_FRC_H_XTAL_REG);
val &= ~(RG_PE1_FRC_H_XTAL_TYPE | RG_PE1_H_XTAL_TYPE);
val |= RG_PE1_FRC_H_XTAL_TYPE;
val |= RG_PE1_H_XTAL_TYPE_VAL(0x00);
- phy_write(instance, val, RG_PE1_FRC_H_XTAL_REG);
+ phy_write(phy, val, RG_PE1_FRC_H_XTAL_REG);
/* disable port */
offset = (instance->index != 1) ?
RG_PE1_FRC_PHY_REG : RG_PE1_FRC_PHY_REG + RG_P0_TO_P1_WIDTH;
- val = phy_read(instance, offset);
+ val = phy_read(phy, offset);
val &= ~(RG_PE1_FRC_PHY_EN | RG_PE1_PHY_EN);
val |= RG_PE1_FRC_PHY_EN;
- phy_write(instance, val, offset);
+ phy_write(phy, val, offset);
/* Set Pre-divider ratio (for host mode) */
- val = phy_read(instance, RG_PE1_H_PLL_REG);
+ val = phy_read(phy, RG_PE1_H_PLL_REG);
val &= ~(RG_PE1_H_PLL_PREDIV);
if (reg <= 5 && reg >= 3) { /* 40MHz Xtal */
val |= RG_PE1_H_PLL_PREDIV_VAL(0x01);
- phy_write(instance, val, RG_PE1_H_PLL_REG);
+ phy_write(phy, val, RG_PE1_H_PLL_REG);
dev_info(dev, "Xtal is 40MHz\n");
} else { /* 25MHz | 20MHz Xtal */
val |= RG_PE1_H_PLL_PREDIV_VAL(0x00);
- phy_write(instance, val, RG_PE1_H_PLL_REG);
+ phy_write(phy, val, RG_PE1_H_PLL_REG);
if (reg >= 6) {
dev_info(dev, "Xtal is 25MHz\n");
/* Select feedback clock */
- val = phy_read(instance, RG_PE1_H_PLL_FBKSEL_REG);
+ val = phy_read(phy, RG_PE1_H_PLL_FBKSEL_REG);
val &= ~(RG_PE1_H_PLL_FBKSEL);
val |= RG_PE1_H_PLL_FBKSEL_VAL(0x01);
- phy_write(instance, val, RG_PE1_H_PLL_FBKSEL_REG);
+ phy_write(phy, val, RG_PE1_H_PLL_FBKSEL_REG);
/* DDS NCPO PCW (for host mode) */
- val = phy_read(instance, RG_PE1_H_LCDDS_SSC_PRD_REG);
+ val = phy_read(phy, RG_PE1_H_LCDDS_SSC_PRD_REG);
val &= ~(RG_PE1_H_LCDDS_SSC_PRD);
val |= RG_PE1_H_LCDDS_SSC_PRD_VAL(0x1800);
- phy_write(instance, val, RG_PE1_H_LCDDS_SSC_PRD_REG);
+ phy_write(phy, val,
[PATCH v2 1/4] staging: mt7621-pci-phy: use 'platform_get_resource'
Driver is using 'of_address_to_resource' to get memory resources.
Make use of 'platform_get_resource' instead which is more accurate
for a platform driver. This also makes possible to delete a local
variable which is not needed anymore.
Signed-off-by: Sergio Paracuellos
---
drivers/staging/mt7621-pci-phy/pci-mt7621-phy.c | 11 +--
1 file changed, 5 insertions(+), 6 deletions(-)
diff --git a/drivers/staging/mt7621-pci-phy/pci-mt7621-phy.c
b/drivers/staging/mt7621-pci-phy/pci-mt7621-phy.c
index aa3ae632..bac188f00f4e 100644
--- a/drivers/staging/mt7621-pci-phy/pci-mt7621-phy.c
+++ b/drivers/staging/mt7621-pci-phy/pci-mt7621-phy.c
@@ -308,11 +308,10 @@ static struct phy *mt7621_pcie_phy_of_xlate(struct device
*dev,
static int mt7621_pci_phy_probe(struct platform_device *pdev)
{
struct device *dev = >dev;
- struct device_node *np = dev->of_node;
struct device_node *child_np;
struct phy_provider *provider;
struct mt7621_pci_phy *phy;
- struct resource res;
+ struct resource *res;
int port, ret;
void __iomem *port_base;
@@ -329,13 +328,13 @@ static int mt7621_pci_phy_probe(struct platform_device
*pdev)
phy->dev = dev;
platform_set_drvdata(pdev, phy);
- ret = of_address_to_resource(np, 0, );
- if (ret) {
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ if (!res) {
dev_err(dev, "failed to get address resource\n");
- return ret;
+ return -ENXIO;
}
- port_base = devm_ioremap_resource(dev, );
+ port_base = devm_ioremap_resource(dev, res);
if (IS_ERR(port_base)) {
dev_err(dev, "failed to remap phy regs\n");
return PTR_ERR(port_base);
--
2.19.1
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v2 2/4] staging: mt7621-pci-phy: remove some unnecessary local variables
Device tree is not using child nodes anymore so the 'child_np' variable
can safely removed. This also simplifies the error path to be able to
directly return errors removing also the 'ret' variable.
Signed-off-by: Sergio Paracuellos
---
drivers/staging/mt7621-pci-phy/pci-mt7621-phy.c | 16
1 file changed, 4 insertions(+), 12 deletions(-)
diff --git a/drivers/staging/mt7621-pci-phy/pci-mt7621-phy.c
b/drivers/staging/mt7621-pci-phy/pci-mt7621-phy.c
index bac188f00f4e..21f980cc2d8f 100644
--- a/drivers/staging/mt7621-pci-phy/pci-mt7621-phy.c
+++ b/drivers/staging/mt7621-pci-phy/pci-mt7621-phy.c
@@ -308,11 +308,10 @@ static struct phy *mt7621_pcie_phy_of_xlate(struct device
*dev,
static int mt7621_pci_phy_probe(struct platform_device *pdev)
{
struct device *dev = >dev;
- struct device_node *child_np;
struct phy_provider *provider;
struct mt7621_pci_phy *phy;
struct resource *res;
- int port, ret;
+ int port;
void __iomem *port_base;
phy = devm_kzalloc(dev, sizeof(*phy), GFP_KERNEL);
@@ -345,18 +344,15 @@ static int mt7621_pci_phy_probe(struct platform_device
*pdev)
struct phy *pphy;
instance = devm_kzalloc(dev, sizeof(*instance), GFP_KERNEL);
- if (!instance) {
- ret = -ENOMEM;
- goto put_child;
- }
+ if (!instance)
+ return -ENOMEM;
phy->phys[port] = instance;
pphy = devm_phy_create(dev, dev->of_node, _pci_phy_ops);
if (IS_ERR(phy)) {
dev_err(dev, "failed to create phy\n");
- ret = PTR_ERR(phy);
- goto put_child;
+ return PTR_ERR(phy);
}
instance->port_base = port_base;
@@ -368,10 +364,6 @@ static int mt7621_pci_phy_probe(struct platform_device
*pdev)
provider = devm_of_phy_provider_register(dev, mt7621_pcie_phy_of_xlate);
return PTR_ERR_OR_ZERO(provider);
-
-put_child:
- of_node_put(child_np);
- return ret;
}
static const struct of_device_id mt7621_pci_phy_ids[] = {
--
2.19.1
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v2 3/4] staging: mt7621-pci-phy: add quirks for 'E2' revision using 'soc_device_attribute'
Depending on revision of the chip, 'mt7621_bypass_pipe_rst' function
must be executed. Add better support for this using 'soc_device_match'
in driver probe function.
Signed-off-by: Sergio Paracuellos
---
drivers/staging/mt7621-pci-phy/pci-mt7621-phy.c | 17 ++---
1 file changed, 14 insertions(+), 3 deletions(-)
diff --git a/drivers/staging/mt7621-pci-phy/pci-mt7621-phy.c
b/drivers/staging/mt7621-pci-phy/pci-mt7621-phy.c
index 21f980cc2d8f..f762369d6792 100644
--- a/drivers/staging/mt7621-pci-phy/pci-mt7621-phy.c
+++ b/drivers/staging/mt7621-pci-phy/pci-mt7621-phy.c
@@ -11,11 +11,11 @@
#include
#include
#include
+#include
#include
#include
#define RALINK_CLKCFG1 0x30
-#define CHIP_REV_MT7621_E2 0x0101
#define PCIE_PORT_CLK_EN(x)BIT(24 + (x))
@@ -97,11 +97,14 @@ struct mt7621_pci_phy_instance {
* @dev: pointer to device
* @phys: pointer to Mt7621 PHY device
* @nphys: number of PHY devices for this core
+ * @bypass_pipe_rst: mark if 'mt7621_bypass_pipe_rst'
+ * needs to be executed. Depends on chip revision.
*/
struct mt7621_pci_phy {
struct device *dev;
struct mt7621_pci_phy_instance **phys;
int nphys;
+ bool bypass_pipe_rst;
};
static inline u32 phy_read(struct mt7621_pci_phy_instance *instance, u32 reg)
@@ -232,9 +235,8 @@ static int mt7621_pci_phy_init(struct phy *phy)
{
struct mt7621_pci_phy_instance *instance = phy_get_drvdata(phy);
struct mt7621_pci_phy *mphy = dev_get_drvdata(phy->dev.parent);
- u32 chip_rev_id = rt_sysc_r32(SYSC_REG_CHIP_REV);
- if ((chip_rev_id & 0x) == CHIP_REV_MT7621_E2)
+ if (mphy->bypass_pipe_rst)
mt7621_bypass_pipe_rst(mphy, instance);
mt7621_set_phy_for_ssc(mphy, instance);
@@ -305,9 +307,14 @@ static struct phy *mt7621_pcie_phy_of_xlate(struct device
*dev,
return mt7621_phy->phys[args->args[0]]->phy;
}
+static const struct soc_device_attribute mt7621_pci_quirks_match[] = {
+ { .soc_id = "mt7621", .revision = "E2" }
+};
+
static int mt7621_pci_phy_probe(struct platform_device *pdev)
{
struct device *dev = >dev;
+ const struct soc_device_attribute *attr;
struct phy_provider *provider;
struct mt7621_pci_phy *phy;
struct resource *res;
@@ -324,6 +331,10 @@ static int mt7621_pci_phy_probe(struct platform_device
*pdev)
if (!phy->phys)
return -ENOMEM;
+ attr = soc_device_match(mt7621_pci_quirks_match);
+ if (attr)
+ phy->bypass_pipe_rst = true;
+
phy->dev = dev;
platform_set_drvdata(pdev, phy);
--
2.19.1
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v2 0/4] staging: mt7621-pci-phy: driver cleanups
This series makes some cleanups pointed out here from Kishon Vijay Abraham I: * https://lkml.org/lkml/2019/4/17/53 Changes in v2: - add patch to make use of regmap API instead of readl and writel. Other changes: - make use of platform_get_resource. - make use of soc_device_match. - clean a bit error paths and not needed locals in 'probe' function. This changes have been only compile-tested. Hope this helps. Best regards, Sergio Paracuellos Sergio Paracuellos (4): staging: mt7621-pci-phy: use 'platform_get_resource' staging: mt7621-pci-phy: remove some unnecessary local variables staging: mt7621-pci-phy: add quirks for 'E2' revision using 'soc_device_attribute' staging: mt7621-pci-phy: convert driver to use kernel regmap API's .../staging/mt7621-pci-phy/pci-mt7621-phy.c | 132 ++ 1 file changed, 77 insertions(+), 55 deletions(-) -- 2.19.1 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [greybus-dev] [PATCH v2] Staging: greybus: Cleanup in greybus driver
On 4/16/19 5:13 PM, Madhumitha Prabakaran wrote:
> Fix a blank line after structure declarations. Also, convert
> macros into inline functions in order to maintain Linux kernel
> coding style based on which the inline function is
> preferable over the macro.
Madhumitha, here is my explanation for why *not* to convert these
container_of() macros to inline functions. It's not just because
"we do this all over the kernel." The reason is that it actually
does not improve the code.
Inline functions are often better than macros because they are
explicit about types, allowing the compiler to tell you if you
are passing parameters of the right type, and possibly assigning
results to objects of the right type.
Here is the definition of the container_of() macro in :
#define container_of(ptr, type, member) ({ \
const typeof(((type *)0)->member) * __mptr = (ptr); \
(type *)((char *)__mptr - offsetof(type, member)); })
You see that ptr is explicitly assigned to a local variable
of the type of the member field, so the compiler is able to
check that assignment. And the return value is similarly
cast to the type of the containing structure, so the type
compatibility of the assignment can also be checked. It is
assumed that where this macro is used, the caller knows it
is passing an appropriate address.
There is another thing about this particular definition make
it just as good as an inline function. A macro expansion can
result in one of its parameters being used more than once, and
that allows those parameters to be *evaluated* more than once
in a single statement, which can produce incorrect code.
This macro is defined using the "statement expression"
extension to C--where a compound statement is enclosed in
parentheses--({ ... }). This allows a local variable to be
used in the macro expansion, which avoids any reuse of the
macro parameters which might cause side-effects.
So anyway, I don't think there is any benefit to replacing
macros like this that do container_of() with inline functions.
-Alex
> Blank line fixes are suggested by checkpatch.pl
>
> Signed-off-by: Madhumitha Prabakaran
>
> Changes in v2:
> - To maintain consistency in driver greybus, all the instances of macro
> with container_of are fixed in a single patch.
> ---
> drivers/staging/greybus/bundle.h| 6 +-
> drivers/staging/greybus/control.h | 6 +-
> drivers/staging/greybus/gbphy.h | 12 ++--
> drivers/staging/greybus/greybus.h | 6 +-
> drivers/staging/greybus/hd.h| 6 +-
> drivers/staging/greybus/interface.h | 6 +-
> drivers/staging/greybus/module.h| 6 +-
> drivers/staging/greybus/svc.h | 6 +-
> 8 files changed, 45 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/staging/greybus/bundle.h
> b/drivers/staging/greybus/bundle.h
> index 8734d2055657..84956eedb1c4 100644
> --- a/drivers/staging/greybus/bundle.h
> +++ b/drivers/staging/greybus/bundle.h
> @@ -31,7 +31,11 @@ struct gb_bundle {
>
> struct list_headlinks; /* interface->bundles */
> };
> -#define to_gb_bundle(d) container_of(d, struct gb_bundle, dev)
> +
> +static inline struct gb_bundle *to_gb_bundle(struct device *d)
> +{
> + return container_of(d, struct gb_bundle, dev);
> +}
>
> /* Greybus "private" definitions" */
> struct gb_bundle *gb_bundle_create(struct gb_interface *intf, u8 bundle_id,
> diff --git a/drivers/staging/greybus/control.h
> b/drivers/staging/greybus/control.h
> index 3a29ec05f631..a681ef74e7fe 100644
> --- a/drivers/staging/greybus/control.h
> +++ b/drivers/staging/greybus/control.h
> @@ -24,7 +24,11 @@ struct gb_control {
> char *vendor_string;
> char *product_string;
> };
> -#define to_gb_control(d) container_of(d, struct gb_control, dev)
> +
> +static inline struct gb_control *to_gb_control(struct device *d)
> +{
> + return container_of(d, struct gb_control, dev);
> +}
>
> struct gb_control *gb_control_create(struct gb_interface *intf);
> int gb_control_enable(struct gb_control *control);
> diff --git a/drivers/staging/greybus/gbphy.h b/drivers/staging/greybus/gbphy.h
> index 99463489d7d6..20307f6dfcb9 100644
> --- a/drivers/staging/greybus/gbphy.h
> +++ b/drivers/staging/greybus/gbphy.h
> @@ -15,7 +15,11 @@ struct gbphy_device {
> struct list_head list;
> struct device dev;
> };
> -#define to_gbphy_dev(d) container_of(d, struct gbphy_device, dev)
> +
> +static inline struct gbphy_device *to_gbphy_dev(struct device *d)
> +{
> + return container_of(d, struct gbphy_device, dev);
> +}
>
> static inline void *gb_gbphy_get_data(struct gbphy_device *gdev)
> {
> @@ -43,7 +47,11 @@ struct gbphy_driver {
>
> struct device_driver driver;
> };
> -#define to_gbphy_driver(d) container_of(d, struct gbphy_driver, driver)
> +
> +static inline struct gbphy_driver *to_gbphy_driver(struct device_driver *d)
> +{
> + return
Re: [greybus-dev] [PATCH v2] Staging: greybus: Cleanup in greybus driver
On Wed, Apr 17, 2019 at 06:19:50AM -0500, Alex Elder wrote: > I'm not completely sure about the inline function, but on the no blank > lines thing (and many other minor issues) "checkpatch.pl" is to blame. > There are lots of examples of issues that checkpatch points out that are > matters of opinion and not hardened kernel style rules. > > We try to encourage people to get involved with kernel development by > fixing minor problems, and we tell them a good way to find them is > by running checkpatch and "fixing" what it reports. Unfortunately, > it is often things of this type, and reviewers balk and say "no, > please leave it," and the poor new person has a bad first experience. > > I *like* "checkpatch.pl". And the fact that it can point out some > of these sorts of things is great. But it would be nice if certain > types of problems (like multiple blank lines, or lines that are 81 > characters wide for example) would only be reported when a "--strict" > option or something were supplied. The problem is that --strict is enabled by default when running checkpatch on code in staging (and net). And it has all sorts of weird tests (prefixed as "CHECK" rather than "WARNING") to catch everyone and their mom's pet peeve. I don't think the intention ever was that all those should be "fixed", but this appears to be where this checkpatch mission creep comes from (and we're now seeing --strict being used on code outside of staging too). IMO we're setting a bad example for new contributers by accepting such changes by default. Blindly trusting a tool is not how kernel development works, but that seems to be the message currently sent. Johan ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [greybus-dev] [PATCH v2] Staging: greybus: Cleanup in greybus driver
On 4/17/19 1:25 AM, Greg KH wrote:
> On Tue, Apr 16, 2019 at 05:13:18PM -0500, Madhumitha Prabakaran wrote:
>> Fix a blank line after structure declarations. Also, convert
>> macros into inline functions in order to maintain Linux kernel
>> coding style based on which the inline function is
>> preferable over the macro.
>>
>> Blank line fixes are suggested by checkpatch.pl
>>
>> Signed-off-by: Madhumitha Prabakaran
>>
>> Changes in v2:
>> - To maintain consistency in driver greybus, all the instances of macro
>> with container_of are fixed in a single patch.
>> ---
>> drivers/staging/greybus/bundle.h| 6 +-
>> drivers/staging/greybus/control.h | 6 +-
>> drivers/staging/greybus/gbphy.h | 12 ++--
>> drivers/staging/greybus/greybus.h | 6 +-
>> drivers/staging/greybus/hd.h| 6 +-
>> drivers/staging/greybus/interface.h | 6 +-
>> drivers/staging/greybus/module.h| 6 +-
>> drivers/staging/greybus/svc.h | 6 +-
>> 8 files changed, 45 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/staging/greybus/bundle.h
>> b/drivers/staging/greybus/bundle.h
>> index 8734d2055657..84956eedb1c4 100644
>> --- a/drivers/staging/greybus/bundle.h
>> +++ b/drivers/staging/greybus/bundle.h
>> @@ -31,7 +31,11 @@ struct gb_bundle {
>>
>> struct list_headlinks; /* interface->bundles */
>> };
>> -#define to_gb_bundle(d) container_of(d, struct gb_bundle, dev)
>> +
>> +static inline struct gb_bundle *to_gb_bundle(struct device *d)
>> +{
>> +return container_of(d, struct gb_bundle, dev);
>> +}
>
> Why are we changing this to an inline function? The #define is fine,
> and how we "normally" do this type of container_of conversion.
I'm not completely sure about the inline function, but on the no blank
lines thing (and many other minor issues) "checkpatch.pl" is to blame.
There are lots of examples of issues that checkpatch points out that are
matters of opinion and not hardened kernel style rules.
We try to encourage people to get involved with kernel development by
fixing minor problems, and we tell them a good way to find them is
by running checkpatch and "fixing" what it reports. Unfortunately,
it is often things of this type, and reviewers balk and say "no,
please leave it," and the poor new person has a bad first experience.
I *like* "checkpatch.pl". And the fact that it can point out some
of these sorts of things is great. But it would be nice if certain
types of problems (like multiple blank lines, or lines that are 81
characters wide for example) would only be reported when a "--strict"
option or something were supplied.
-Alex
> I understand the objection of the "no blank line", but this was the
> "original" style that we used to create these #defines from the very
> beginning of the driver model work a decade ago. Changing that
> muscle-memory is going to be hard for some of us. Look at
> drivers/base/base.h for other examples of this.
>
> thanks,
>
> greg k-h
> ___
> greybus-dev mailing list
> greybus-...@lists.linaro.org
> https://lists.linaro.org/mailman/listinfo/greybus-dev
>
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 0/3] staging: mt7621-pci-phy: driver cleanups
This series makes some cleanups pointed out here from Kishon Vijay Abraham I: * https://lkml.org/lkml/2019/4/17/53 Changes: - make use of platform_get_resource. - make use of soc_device_match. - clean a bit error paths and not needed locals in 'probe' function. Regmap api is not being introduced yet... should we? I understand this simplifies code for spi or i2c drivers but I am not sure in this case. Neil, what do you think? This changes have been only compile-tested. Hope this helps. Best regards, Sergio Paracuellos Sergio Paracuellos (3): staging: mt7621-pci-phy: use 'platform_get_resource' staging: mt7621-pci-phy: remove some unnecessary local variables staging: mt7621-pci-phy: add quirks for 'E2' revision using 'soc_device_attribute' .../staging/mt7621-pci-phy/pci-mt7621-phy.c | 44 ++- 1 file changed, 23 insertions(+), 21 deletions(-) -- 2.19.1 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 3/3] staging: mt7621-pci-phy: add quirks for 'E2' revision using 'soc_device_attribute'
Depending on revision of the chip, 'mt7621_bypass_pipe_rst' function
must be executed. Add better support for this using 'soc_device_match'
in driver probe function.
Signed-off-by: Sergio Paracuellos
---
drivers/staging/mt7621-pci-phy/pci-mt7621-phy.c | 17 ++---
1 file changed, 14 insertions(+), 3 deletions(-)
diff --git a/drivers/staging/mt7621-pci-phy/pci-mt7621-phy.c
b/drivers/staging/mt7621-pci-phy/pci-mt7621-phy.c
index 21f980cc2d8f..f762369d6792 100644
--- a/drivers/staging/mt7621-pci-phy/pci-mt7621-phy.c
+++ b/drivers/staging/mt7621-pci-phy/pci-mt7621-phy.c
@@ -11,11 +11,11 @@
#include
#include
#include
+#include
#include
#include
#define RALINK_CLKCFG1 0x30
-#define CHIP_REV_MT7621_E2 0x0101
#define PCIE_PORT_CLK_EN(x)BIT(24 + (x))
@@ -97,11 +97,14 @@ struct mt7621_pci_phy_instance {
* @dev: pointer to device
* @phys: pointer to Mt7621 PHY device
* @nphys: number of PHY devices for this core
+ * @bypass_pipe_rst: mark if 'mt7621_bypass_pipe_rst'
+ * needs to be executed. Depends on chip revision.
*/
struct mt7621_pci_phy {
struct device *dev;
struct mt7621_pci_phy_instance **phys;
int nphys;
+ bool bypass_pipe_rst;
};
static inline u32 phy_read(struct mt7621_pci_phy_instance *instance, u32 reg)
@@ -232,9 +235,8 @@ static int mt7621_pci_phy_init(struct phy *phy)
{
struct mt7621_pci_phy_instance *instance = phy_get_drvdata(phy);
struct mt7621_pci_phy *mphy = dev_get_drvdata(phy->dev.parent);
- u32 chip_rev_id = rt_sysc_r32(SYSC_REG_CHIP_REV);
- if ((chip_rev_id & 0x) == CHIP_REV_MT7621_E2)
+ if (mphy->bypass_pipe_rst)
mt7621_bypass_pipe_rst(mphy, instance);
mt7621_set_phy_for_ssc(mphy, instance);
@@ -305,9 +307,14 @@ static struct phy *mt7621_pcie_phy_of_xlate(struct device
*dev,
return mt7621_phy->phys[args->args[0]]->phy;
}
+static const struct soc_device_attribute mt7621_pci_quirks_match[] = {
+ { .soc_id = "mt7621", .revision = "E2" }
+};
+
static int mt7621_pci_phy_probe(struct platform_device *pdev)
{
struct device *dev = >dev;
+ const struct soc_device_attribute *attr;
struct phy_provider *provider;
struct mt7621_pci_phy *phy;
struct resource *res;
@@ -324,6 +331,10 @@ static int mt7621_pci_phy_probe(struct platform_device
*pdev)
if (!phy->phys)
return -ENOMEM;
+ attr = soc_device_match(mt7621_pci_quirks_match);
+ if (attr)
+ phy->bypass_pipe_rst = true;
+
phy->dev = dev;
platform_set_drvdata(pdev, phy);
--
2.19.1
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 2/3] staging: mt7621-pci-phy: remove some unnecessary local variables
Device tree is not using child nodes anymore so the 'child_np' variable
can safely removed. This also simplifies the error path to be able to
directly return errors removing also the 'ret' variable.
Signed-off-by: Sergio Paracuellos
---
drivers/staging/mt7621-pci-phy/pci-mt7621-phy.c | 16
1 file changed, 4 insertions(+), 12 deletions(-)
diff --git a/drivers/staging/mt7621-pci-phy/pci-mt7621-phy.c
b/drivers/staging/mt7621-pci-phy/pci-mt7621-phy.c
index bac188f00f4e..21f980cc2d8f 100644
--- a/drivers/staging/mt7621-pci-phy/pci-mt7621-phy.c
+++ b/drivers/staging/mt7621-pci-phy/pci-mt7621-phy.c
@@ -308,11 +308,10 @@ static struct phy *mt7621_pcie_phy_of_xlate(struct device
*dev,
static int mt7621_pci_phy_probe(struct platform_device *pdev)
{
struct device *dev = >dev;
- struct device_node *child_np;
struct phy_provider *provider;
struct mt7621_pci_phy *phy;
struct resource *res;
- int port, ret;
+ int port;
void __iomem *port_base;
phy = devm_kzalloc(dev, sizeof(*phy), GFP_KERNEL);
@@ -345,18 +344,15 @@ static int mt7621_pci_phy_probe(struct platform_device
*pdev)
struct phy *pphy;
instance = devm_kzalloc(dev, sizeof(*instance), GFP_KERNEL);
- if (!instance) {
- ret = -ENOMEM;
- goto put_child;
- }
+ if (!instance)
+ return -ENOMEM;
phy->phys[port] = instance;
pphy = devm_phy_create(dev, dev->of_node, _pci_phy_ops);
if (IS_ERR(phy)) {
dev_err(dev, "failed to create phy\n");
- ret = PTR_ERR(phy);
- goto put_child;
+ return PTR_ERR(phy);
}
instance->port_base = port_base;
@@ -368,10 +364,6 @@ static int mt7621_pci_phy_probe(struct platform_device
*pdev)
provider = devm_of_phy_provider_register(dev, mt7621_pcie_phy_of_xlate);
return PTR_ERR_OR_ZERO(provider);
-
-put_child:
- of_node_put(child_np);
- return ret;
}
static const struct of_device_id mt7621_pci_phy_ids[] = {
--
2.19.1
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 1/3] staging: mt7621-pci-phy: use 'platform_get_resource'
Driver is using 'of_address_to_resource' to get memory resources.
Make use of 'platform_get_resource' instead which is more accurate
for a platform driver. This also makes possible to delete a local
variable which is not needed anymore.
Signed-off-by: Sergio Paracuellos
---
drivers/staging/mt7621-pci-phy/pci-mt7621-phy.c | 11 +--
1 file changed, 5 insertions(+), 6 deletions(-)
diff --git a/drivers/staging/mt7621-pci-phy/pci-mt7621-phy.c
b/drivers/staging/mt7621-pci-phy/pci-mt7621-phy.c
index aa3ae632..bac188f00f4e 100644
--- a/drivers/staging/mt7621-pci-phy/pci-mt7621-phy.c
+++ b/drivers/staging/mt7621-pci-phy/pci-mt7621-phy.c
@@ -308,11 +308,10 @@ static struct phy *mt7621_pcie_phy_of_xlate(struct device
*dev,
static int mt7621_pci_phy_probe(struct platform_device *pdev)
{
struct device *dev = >dev;
- struct device_node *np = dev->of_node;
struct device_node *child_np;
struct phy_provider *provider;
struct mt7621_pci_phy *phy;
- struct resource res;
+ struct resource *res;
int port, ret;
void __iomem *port_base;
@@ -329,13 +328,13 @@ static int mt7621_pci_phy_probe(struct platform_device
*pdev)
phy->dev = dev;
platform_set_drvdata(pdev, phy);
- ret = of_address_to_resource(np, 0, );
- if (ret) {
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ if (!res) {
dev_err(dev, "failed to get address resource\n");
- return ret;
+ return -ENXIO;
}
- port_base = devm_ioremap_resource(dev, );
+ port_base = devm_ioremap_resource(dev, res);
if (IS_ERR(port_base)) {
dev_err(dev, "failed to remap phy regs\n");
return PTR_ERR(port_base);
--
2.19.1
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3 00/26] compat_ioctl: cleanups
On Wed, Apr 17, 2019 at 12:33 AM Douglas Gilbert wrote:
>
> On 2019-04-16 4:19 p.m., Arnd Bergmann wrote:
> > Hi Al,
> >
> > It took me way longer than I had hoped to revisit this series, see
> > https://lore.kernel.org/lkml/20180912150142.157913-1-a...@arndb.de/
> > for the previously posted version.
> >
> > I've come to the point where all conversion handlers and most
> > COMPATIBLE_IOCTL() entries are gone from this file, but for
> > now, this series only has the parts that have either been reviewed
> > previously, or that are simple enough to include.
> >
> > The main missing piece is the SG_IO/SG_GET_REQUEST_TABLE conversion.
> > I'll post the patches I made for that later, as they need more
> > testing and review from the scsi maintainers.
>
> Perhaps you could look at the document in this url:
> http://sg.danny.cz/sg/sg_v40.html
>
> It is work-in-progress to modernize the SCSI generic driver. It
> extends ioctl(sg_fd, SG_IO, _obj) to additionally accept the sg v4
> interface as defined in include/uapi/linux/bsg.h . Currently only the
> bsg driver uses the sg v4 interface. Since struct sg_io_v4 is all
> explicitly sized integers, I'm guessing it is immune "compat" problems.
> [I can see no reference to bsg nor struct sg_io_v4 in the current
> fs/compat_ioctl.c file.]
Ok, I've taken a brief look at your series now. Unfortunately it clashes
quite hard with my series, but it's probably for the better to have your
stuff get merged first.
A few (unsorted) comments from going through your patches:
- the added ioctls are all compatible when using the v4 structures
and mostly don't need handlers for compat mode, but they need to be
called from .compat_ioctl to actually be usable in compat mode.
With my patches you get that.
- One exception for the v4 layout is the use of iovec pointers, as
'struct iovec' is incompatible. We should probably merge the
generic compat_import_iovec() into import_iovec() with a
'in_compat_syscall()' check, which would be helpful in general.
bsg.c does not iovec, so it is not affected by this at the moment,
maybe it would be better to stay compatible with that and also
not support them in sg.c?
- Is there a need for the new sg_ioctl_iosubmit/sg_ioctl_ioreceive
to support the v3 structures? Those are /not/ compatible, so
you need extra code to handle the v3-compat layout as well.
Supporting only v4 would simplify this.
- the lack of changeset descriptions is a bit irritating and makes
it much harder to understand what you are doing.
- try to keep patches that move code around separate from those
that change it in any other way, for better reviewing.
- in "sg: preparation for request sharing", you seem to inadvertently
change the size of "struct sg_extended_info", making it 4 bytes
longer by adding two members.
- You should never use IS_ERR_OR_NULL() in normal code, that
is just a sign of a bad API. Make each function have consistent
error behavior.
- The "#if 0 /* temporary to shorten big patch */" trick breaks
bisection, that is probably worse than the larger patch.
- The split access_ok()/__copy_from_user() has fallen out of
favor because it has caused too many bugs in the past, just
use the combined copy_from_user() instead.
- ktime_to_ns(ktime_get_with_offset(TK_OFFS_BOOT)) followed
by a 64-bit division won't work on 32-bit machines, use
ktime_get_boottime_ts64() instead.
> Other additions described in the that document are these new ioctls:
>- SG_IOSUBMITultimately to replace write(sg_fd, ...)
>- SG_IORECEIVE to replace read(sg_fd, ...)
>- SG_IOABORT abort SCSI cmd in progress; new functionality
>- SG_SET_GET_EXTENDED has associated struct sg_extended_info
>
> The first three take a pointer to a struct sg_io_hdr (v3 interface) or
> a struct sg_io_v4 object. Both objects start with a 32 bit integer:
> 'S' identifies the v3 interface while 'Q' identifies the v4 interface.
I think the magic character was a mistake in the original design,
just like versioned interfaces in general. If you are extending an
interface in an incompatible way, the normal way would be to
have separate command codes, like SG_IORECEIVE_V3
and SG_IORECEIVE_V4, if you absolutely have to maintain
compatiblity with the old interface (which I think you don't in
case of SG_IORECEIVE).
For SG_IO, I can see why you want to support both the v3
and v4 structures plus the compat-v3 version, but I'd try to keep
them as separate as possible, and do something like
static int sg_ctl_sg_io(struct file *filp, struct sg_device *sdp,
struct sg_fd *sfp,
void __user *p)
{
int ret;
ret = sg_io_v4(filp, sdp, sfp, (struct sg_io_v4 __user *)p);
if (ret != -ENOIOCTLCMD || !S_ENABLED(CONFIG_SG_IO_V3))
return ret;
if (in_compat_syscall())
ret = sg_io_compat_(filp, sdp, sfp, (struct
compat_sg_io_hdr __user *)p);
else
ret = sg_io_v3(filp,
Re: [PATCH] staging: comedi: use help instead of ---help--- in Kconfig
On 16/04/2019 18:08, Moses Christopher wrote: - Resolve the following warning from the Kconfig, "WARNING: prefer 'help' over '---help---' for new help texts" Signed-off-by: Moses Christopher --- drivers/staging/comedi/Kconfig | 254 - 1 file changed, 127 insertions(+), 127 deletions(-) Looks good, thanks! Reviewed-by: Ian Abbott -- -=( Ian Abbott || Web: www.mev.co.uk )=- -=( MEV Ltd. is a company registered in England & Wales. )=- -=( Registered number: 02862268. Registered address:)=- -=( 15 West Park Road, Bramhall, STOCKPORT, SK7 3JZ, UK. )=- ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 2/2] phy: ralink: Add PHY driver for MT7621 PCIe PHY
Hi Kishon, On Wed, Apr 17, 2019 at 7:58 AM Kishon Vijay Abraham I wrote: > > Hi, > > On 30/03/19 11:20 AM, Sergio Paracuellos wrote: > > This patch adds a driver for the PCIe PHY of MT7621 SoC. > > > > Signed-off-by: Sergio Paracuellos > > --- > > drivers/phy/ralink/Kconfig | 7 + > > drivers/phy/ralink/Makefile | 1 + > > drivers/phy/ralink/phy-mt7621-pci.c | 401 > > 3 files changed, 409 insertions(+) > > create mode 100644 drivers/phy/ralink/phy-mt7621-pci.c > > > > diff --git a/drivers/phy/ralink/Kconfig b/drivers/phy/ralink/Kconfig > > index 14fd219535ef..87943a10f210 100644 > > --- a/drivers/phy/ralink/Kconfig > > +++ b/drivers/phy/ralink/Kconfig > > @@ -1,6 +1,13 @@ > > # > > # PHY drivers for Ralink platforms. > > # > > +config PHY_MT7621_PCI > > + tristate "MediaTek MT7621 PCI PHY Driver" > > Shouldn't this be in drivers/phy/mediatek/ then? I am not sure about this, it is mediatek but it only appears on ralink so, I don't know where it should be placed. Maybe Neil has an opinion about this. > > + depends on RALINK && OF > > Add depends on COMPILE_TEST. Will do. > > + select GENERIC_PHY > > + help > > + Say 'Y' here to add support for MediaTek MT7621 PCI PHY driver, > > + > > config PHY_RALINK_USB > > tristate "Ralink USB PHY driver" > > depends on RALINK || COMPILE_TEST > > diff --git a/drivers/phy/ralink/Makefile b/drivers/phy/ralink/Makefile > > index 5c9e326e8757..2052d5649863 100644 > > --- a/drivers/phy/ralink/Makefile > > +++ b/drivers/phy/ralink/Makefile > > @@ -1 +1,2 @@ > > +obj-$(CONFIG_PHY_MT7621_PCI) += phy-mt7621-pci.o > > obj-$(CONFIG_PHY_RALINK_USB) += phy-ralink-usb.o > > diff --git a/drivers/phy/ralink/phy-mt7621-pci.c > > b/drivers/phy/ralink/phy-mt7621-pci.c > > new file mode 100644 > > index ..118302c122ee > > --- /dev/null > > +++ b/drivers/phy/ralink/phy-mt7621-pci.c > > @@ -0,0 +1,401 @@ > > +// SPDX-License-Identifier: GPL-2.0+ > > +/* > > + * Mediatek MT7621 PCI PHY Driver > > + * Author: Sergio Paracuellos > > + */ > > + > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > + > > +#define RALINK_CLKCFG1 0x30 > > +#define CHIP_REV_MT7621_E2 0x0101 > > Is this PHY specific or SoC revision? If it is SoC, we should use > soc_device_attribute and soc_device_match. Depending on revision reset lines are inverted and somebypasses need to be performed to properly initialize the phy. I'll take a look of how this can be done better using soc_device_attribute and soc_device_match as you are pointing out here. > > + > > +#define PCIE_PORT_CLK_EN(x) BIT(24 + (x)) > > + > > +#define RG_PE1_PIPE_REG 0x02c > > +#define RG_PE1_PIPE_RST BIT(12) > > +#define RG_PE1_PIPE_CMD_FRC BIT(4) > > + > > +#define RG_P0_TO_P1_WIDTH0x100 > > +#define RG_PE1_H_LCDDS_REG 0x49c > > +#define RG_PE1_H_LCDDS_PCW GENMASK(30, 0) > > +#define RG_PE1_H_LCDDS_PCW_VAL(x)((0x7fff & (x)) << 0) > > + > > +#define RG_PE1_FRC_H_XTAL_REG0x400 > > +#define RG_PE1_FRC_H_XTAL_TYPE BIT(8) > > +#define RG_PE1_H_XTAL_TYPE GENMASK(10, 9) > > +#define RG_PE1_H_XTAL_TYPE_VAL(x)((0x3 & (x)) << 9) > > + > > +#define RG_PE1_FRC_PHY_REG 0x000 > > +#define RG_PE1_FRC_PHY_ENBIT(4) > > +#define RG_PE1_PHY_ENBIT(5) > > + > > +#define RG_PE1_H_PLL_REG 0x490 > > +#define RG_PE1_H_PLL_BC GENMASK(23, 22) > > +#define RG_PE1_H_PLL_BC_VAL(x) ((0x3 & (x)) << 22) > > +#define RG_PE1_H_PLL_BP GENMASK(21, 18) > > +#define RG_PE1_H_PLL_BP_VAL(x) ((0xf & (x)) << 18) > > +#define RG_PE1_H_PLL_IR GENMASK(15, 12) > > +#define RG_PE1_H_PLL_IR_VAL(x) ((0xf & (x)) << 12) > > +#define RG_PE1_H_PLL_IC GENMASK(11, 8) > > +#define RG_PE1_H_PLL_IC_VAL(x) ((0xf & (x)) << 8) > > +#define RG_PE1_H_PLL_PREDIV GENMASK(7, 6) > > +#define RG_PE1_H_PLL_PREDIV_VAL(x) ((0x3 & (x)) << 6) > > +#define RG_PE1_PLL_DIVEN GENMASK(3, 1) > > +#define RG_PE1_PLL_DIVEN_VAL(x) ((0x7 & (x)) << 1) > > + > > +#define RG_PE1_H_PLL_FBKSEL_REG 0x4bc > > +#define RG_PE1_H_PLL_FBKSEL GENMASK(5, 4) > > +#define RG_PE1_H_PLL_FBKSEL_VAL(x) ((0x3 & (x)) << 4) > > + > > +#define RG_PE1_H_LCDDS_SSC_PRD_REG 0x4a4 > > +#define RG_PE1_H_LCDDS_SSC_PRD GENMASK(15, 0) > > +#define
Re: [PATCH v2] Staging: greybus: Cleanup in greybus driver
On Tue, Apr 16, 2019 at 05:13:18PM -0500, Madhumitha Prabakaran wrote:
> Fix a blank line after structure declarations. Also, convert
> macros into inline functions in order to maintain Linux kernel
> coding style based on which the inline function is
> preferable over the macro.
>
> Blank line fixes are suggested by checkpatch.pl
>
> Signed-off-by: Madhumitha Prabakaran
>
> Changes in v2:
> - To maintain consistency in driver greybus, all the instances of macro
> with container_of are fixed in a single patch.
> ---
> drivers/staging/greybus/bundle.h| 6 +-
> drivers/staging/greybus/control.h | 6 +-
> drivers/staging/greybus/gbphy.h | 12 ++--
> drivers/staging/greybus/greybus.h | 6 +-
> drivers/staging/greybus/hd.h| 6 +-
> drivers/staging/greybus/interface.h | 6 +-
> drivers/staging/greybus/module.h| 6 +-
> drivers/staging/greybus/svc.h | 6 +-
> 8 files changed, 45 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/staging/greybus/bundle.h
> b/drivers/staging/greybus/bundle.h
> index 8734d2055657..84956eedb1c4 100644
> --- a/drivers/staging/greybus/bundle.h
> +++ b/drivers/staging/greybus/bundle.h
> @@ -31,7 +31,11 @@ struct gb_bundle {
>
> struct list_headlinks; /* interface->bundles */
> };
> -#define to_gb_bundle(d) container_of(d, struct gb_bundle, dev)
> +
> +static inline struct gb_bundle *to_gb_bundle(struct device *d)
> +{
> + return container_of(d, struct gb_bundle, dev);
> +}
Why are we changing this to an inline function? The #define is fine,
and how we "normally" do this type of container_of conversion.
I understand the objection of the "no blank line", but this was the
"original" style that we used to create these #defines from the very
beginning of the driver model work a decade ago. Changing that
muscle-memory is going to be hard for some of us. Look at
drivers/base/base.h for other examples of this.
thanks,
greg k-h
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 2/2] phy: ralink: Add PHY driver for MT7621 PCIe PHY
Hi, On 30/03/19 11:20 AM, Sergio Paracuellos wrote: > This patch adds a driver for the PCIe PHY of MT7621 SoC. > > Signed-off-by: Sergio Paracuellos > --- > drivers/phy/ralink/Kconfig | 7 + > drivers/phy/ralink/Makefile | 1 + > drivers/phy/ralink/phy-mt7621-pci.c | 401 > 3 files changed, 409 insertions(+) > create mode 100644 drivers/phy/ralink/phy-mt7621-pci.c > > diff --git a/drivers/phy/ralink/Kconfig b/drivers/phy/ralink/Kconfig > index 14fd219535ef..87943a10f210 100644 > --- a/drivers/phy/ralink/Kconfig > +++ b/drivers/phy/ralink/Kconfig > @@ -1,6 +1,13 @@ > # > # PHY drivers for Ralink platforms. > # > +config PHY_MT7621_PCI > + tristate "MediaTek MT7621 PCI PHY Driver" Shouldn't this be in drivers/phy/mediatek/ then? > + depends on RALINK && OF Add depends on COMPILE_TEST. > + select GENERIC_PHY > + help > + Say 'Y' here to add support for MediaTek MT7621 PCI PHY driver, > + > config PHY_RALINK_USB > tristate "Ralink USB PHY driver" > depends on RALINK || COMPILE_TEST > diff --git a/drivers/phy/ralink/Makefile b/drivers/phy/ralink/Makefile > index 5c9e326e8757..2052d5649863 100644 > --- a/drivers/phy/ralink/Makefile > +++ b/drivers/phy/ralink/Makefile > @@ -1 +1,2 @@ > +obj-$(CONFIG_PHY_MT7621_PCI) += phy-mt7621-pci.o > obj-$(CONFIG_PHY_RALINK_USB) += phy-ralink-usb.o > diff --git a/drivers/phy/ralink/phy-mt7621-pci.c > b/drivers/phy/ralink/phy-mt7621-pci.c > new file mode 100644 > index ..118302c122ee > --- /dev/null > +++ b/drivers/phy/ralink/phy-mt7621-pci.c > @@ -0,0 +1,401 @@ > +// SPDX-License-Identifier: GPL-2.0+ > +/* > + * Mediatek MT7621 PCI PHY Driver > + * Author: Sergio Paracuellos > + */ > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#define RALINK_CLKCFG1 0x30 > +#define CHIP_REV_MT7621_E2 0x0101 Is this PHY specific or SoC revision? If it is SoC, we should use soc_device_attribute and soc_device_match. > + > +#define PCIE_PORT_CLK_EN(x) BIT(24 + (x)) > + > +#define RG_PE1_PIPE_REG 0x02c > +#define RG_PE1_PIPE_RST BIT(12) > +#define RG_PE1_PIPE_CMD_FRC BIT(4) > + > +#define RG_P0_TO_P1_WIDTH0x100 > +#define RG_PE1_H_LCDDS_REG 0x49c > +#define RG_PE1_H_LCDDS_PCW GENMASK(30, 0) > +#define RG_PE1_H_LCDDS_PCW_VAL(x)((0x7fff & (x)) << 0) > + > +#define RG_PE1_FRC_H_XTAL_REG0x400 > +#define RG_PE1_FRC_H_XTAL_TYPE BIT(8) > +#define RG_PE1_H_XTAL_TYPE GENMASK(10, 9) > +#define RG_PE1_H_XTAL_TYPE_VAL(x)((0x3 & (x)) << 9) > + > +#define RG_PE1_FRC_PHY_REG 0x000 > +#define RG_PE1_FRC_PHY_ENBIT(4) > +#define RG_PE1_PHY_ENBIT(5) > + > +#define RG_PE1_H_PLL_REG 0x490 > +#define RG_PE1_H_PLL_BC GENMASK(23, 22) > +#define RG_PE1_H_PLL_BC_VAL(x) ((0x3 & (x)) << 22) > +#define RG_PE1_H_PLL_BP GENMASK(21, 18) > +#define RG_PE1_H_PLL_BP_VAL(x) ((0xf & (x)) << 18) > +#define RG_PE1_H_PLL_IR GENMASK(15, 12) > +#define RG_PE1_H_PLL_IR_VAL(x) ((0xf & (x)) << 12) > +#define RG_PE1_H_PLL_IC GENMASK(11, 8) > +#define RG_PE1_H_PLL_IC_VAL(x) ((0xf & (x)) << 8) > +#define RG_PE1_H_PLL_PREDIV GENMASK(7, 6) > +#define RG_PE1_H_PLL_PREDIV_VAL(x) ((0x3 & (x)) << 6) > +#define RG_PE1_PLL_DIVEN GENMASK(3, 1) > +#define RG_PE1_PLL_DIVEN_VAL(x) ((0x7 & (x)) << 1) > + > +#define RG_PE1_H_PLL_FBKSEL_REG 0x4bc > +#define RG_PE1_H_PLL_FBKSEL GENMASK(5, 4) > +#define RG_PE1_H_PLL_FBKSEL_VAL(x) ((0x3 & (x)) << 4) > + > +#define RG_PE1_H_LCDDS_SSC_PRD_REG 0x4a4 > +#define RG_PE1_H_LCDDS_SSC_PRD GENMASK(15, 0) > +#define RG_PE1_H_LCDDS_SSC_PRD_VAL(x)((0x & (x)) << 0) > + > +#define RG_PE1_H_LCDDS_SSC_DELTA_REG 0x4a8 > +#define RG_PE1_H_LCDDS_SSC_DELTA GENMASK(11, 0) > +#define RG_PE1_H_LCDDS_SSC_DELTA_VAL(x) ((0xfff & (x)) << 0) > +#define RG_PE1_H_LCDDS_SSC_DELTA1GENMASK(27, 16) > +#define RG_PE1_H_LCDDS_SSC_DELTA1_VAL(x) ((0xff & (x)) << 16) > + > +#define RG_PE1_LCDDS_CLK_PH_INV_REG 0x4a0 > +#define RG_PE1_LCDDS_CLK_PH_INV BIT(5) > + > +#define RG_PE1_H_PLL_BR_REG 0x4ac > +#define RG_PE1_H_PLL_BR GENMASK(18, 16) > +#define RG_PE1_H_PLL_BR_VAL(x)
