Hello,
this is v2 of this series, it's goal is to fix struct device lifetime issues as
pointed out in patch #13. The patches up to patch #12 are only prepatory and
cleanup patches. Patch #13 provides the needed functions to fix the issues in
all drivers (patches #15 to #22). The last patch removes the then unused API
calls.
The changes compared to v1 is only build fixes that I missed to include in v1,
they were only in my working copy. Additionally I changed:
diff --git a/drivers/counter/counter-core.c b/drivers/counter/counter-core.c
index cdc6004a7e77..3f7dc5718423 100644
--- a/drivers/counter/counter-core.c
+++ b/drivers/counter/counter-core.c
@@ -27,7 +27,7 @@ static DEFINE_IDA(counter_ida);
struct counter_device_allochelper {
struct counter_device counter;
- unsigned long privdata[0];
+ unsigned long privdata[];
};
static void counter_device_release(struct device *dev)
The stm32-timer-cnt driver was used to test
this series, the other drivers are only compile tested.
To complete the information from the v1 thread: There are a few more
issues I found while working on this patch set:
- 104_QUAD_8 depends on X86, but compiles fine on ARCH=arm. Maybe
adding support for COMPILE_TEST would be a good idea.
- 104-quad-8.c uses devm_request_irq() and (now) devm_counter_add(). On
unbind an irq might be pending which results in quad8_irq_handler()
calling counter_push_event() for a counter that is already
unregistered. (The issue exists also without my changes.)
- I think intel-qep.c makes the counter unfunctional in
intel_qep_remove before the counter is unregistered.
- I wonder why counter is a bus and not a class device type. There is
no driver that would ever bind a counter device, is there? So
/sys/bus/counter/driver is always empty.
Do whatever you want with this list, I won't address these in the near
future.
Uwe Kleine-König (23):
counter: Use container_of instead of drvdata to track counter_device
counter: ftm-quaddec: Drop unused platform_set_drvdata()
counter: microchip-tcb-capture: Drop unused platform_set_drvdata()
counter: Provide a wrapper to access device private data
counter: 104-quad-8: Convert to counter_priv() wrapper
counter: interrupt-cnt: Convert to counter_priv() wrapper
counter: microchip-tcb-capture: Convert to counter_priv() wrapper
counter: intel-qep: Convert to counter_priv() wrapper
counter: ftm-quaddec: Convert to counter_priv() wrapper
counter: ti-eqep: Convert to counter_priv() wrapper
counter: stm32-lptimer-cnt: Convert to counter_priv() wrapper
counter: stm32-timer-cnt: Convert to counter_priv() wrapper
counter: Provide alternative counter registration functions
counter: Update documentation for new counter registration functions
counter: 104-quad-8: Convert to new counter registration
counter: interrupt-cnt: Convert to new counter registration
counter: intel-qep: Convert to new counter registration
counter: ftm-quaddec: Convert to new counter registration
counter: microchip-tcb-capture: Convert to new counter registration
counter: stm32-timer-cnt: Convert to new counter registration
counter: stm32-lptimer-cnt: Convert to new counter registration
counter: ti-eqep: Convert to new counter registration
counter: remove old and now unused registration API
Documentation/driver-api/generic-counter.rst | 10 +-
drivers/counter/104-quad-8.c | 93 +++++-----
drivers/counter/counter-core.c | 168 +++++++++++++------
drivers/counter/ftm-quaddec.c | 37 ++--
drivers/counter/intel-qep.c | 46 ++---
drivers/counter/interrupt-cnt.c | 38 +++--
drivers/counter/microchip-tcb-capture.c | 44 ++---
drivers/counter/stm32-lptimer-cnt.c | 51 +++---
drivers/counter/stm32-timer-cnt.c | 48 +++---
drivers/counter/ti-eqep.c | 47 +++---
include/linux/counter.h | 15 +-
11 files changed, 348 insertions(+), 249 deletions(-)
base-commit: a7904a538933c525096ca2ccde1e60d0ee62c08e
--
2.33.0
This is a straight forward conversion to the new counter_priv() wrapper.
Signed-off-by: Uwe Kleine-König <[email protected]>
---
drivers/counter/interrupt-cnt.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/counter/interrupt-cnt.c b/drivers/counter/interrupt-cnt.c
index 8514a87fcbee..4bf706ef46e2 100644
--- a/drivers/counter/interrupt-cnt.c
+++ b/drivers/counter/interrupt-cnt.c
@@ -37,7 +37,7 @@ static irqreturn_t interrupt_cnt_isr(int irq, void *dev_id)
static int interrupt_cnt_enable_read(struct counter_device *counter,
struct counter_count *count, u8 *enable)
{
- struct interrupt_cnt_priv *priv = counter->priv;
+ struct interrupt_cnt_priv *priv = counter_priv(counter);
*enable = priv->enabled;
@@ -47,7 +47,7 @@ static int interrupt_cnt_enable_read(struct counter_device *counter,
static int interrupt_cnt_enable_write(struct counter_device *counter,
struct counter_count *count, u8 enable)
{
- struct interrupt_cnt_priv *priv = counter->priv;
+ struct interrupt_cnt_priv *priv = counter_priv(counter);
if (priv->enabled == enable)
return 0;
@@ -85,7 +85,7 @@ static int interrupt_cnt_action_read(struct counter_device *counter,
static int interrupt_cnt_read(struct counter_device *counter,
struct counter_count *count, u64 *val)
{
- struct interrupt_cnt_priv *priv = counter->priv;
+ struct interrupt_cnt_priv *priv = counter_priv(counter);
*val = atomic_read(&priv->count);
@@ -95,7 +95,7 @@ static int interrupt_cnt_read(struct counter_device *counter,
static int interrupt_cnt_write(struct counter_device *counter,
struct counter_count *count, const u64 val)
{
- struct interrupt_cnt_priv *priv = counter->priv;
+ struct interrupt_cnt_priv *priv = counter_priv(counter);
if (val != (typeof(priv->count.counter))val)
return -ERANGE;
@@ -122,7 +122,7 @@ static int interrupt_cnt_signal_read(struct counter_device *counter,
struct counter_signal *signal,
enum counter_signal_level *level)
{
- struct interrupt_cnt_priv *priv = counter->priv;
+ struct interrupt_cnt_priv *priv = counter_priv(counter);
int ret;
if (!priv->gpio)
--
2.33.0
This is a straight forward conversion to the new counter_priv() wrapper.
Signed-off-by: Uwe Kleine-König <[email protected]>
---
drivers/counter/ftm-quaddec.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/counter/ftm-quaddec.c b/drivers/counter/ftm-quaddec.c
index 9272f7b58beb..f5d92df6a611 100644
--- a/drivers/counter/ftm-quaddec.c
+++ b/drivers/counter/ftm-quaddec.c
@@ -118,7 +118,7 @@ static void ftm_quaddec_disable(void *ftm)
static int ftm_quaddec_get_prescaler(struct counter_device *counter,
struct counter_count *count, u32 *cnt_mode)
{
- struct ftm_quaddec *ftm = counter->priv;
+ struct ftm_quaddec *ftm = counter_priv(counter);
uint32_t scflags;
ftm_read(ftm, FTM_SC, &scflags);
@@ -131,7 +131,7 @@ static int ftm_quaddec_get_prescaler(struct counter_device *counter,
static int ftm_quaddec_set_prescaler(struct counter_device *counter,
struct counter_count *count, u32 cnt_mode)
{
- struct ftm_quaddec *ftm = counter->priv;
+ struct ftm_quaddec *ftm = counter_priv(counter);
mutex_lock(&ftm->ftm_quaddec_mutex);
@@ -162,7 +162,7 @@ static int ftm_quaddec_count_read(struct counter_device *counter,
struct counter_count *count,
u64 *val)
{
- struct ftm_quaddec *const ftm = counter->priv;
+ struct ftm_quaddec *const ftm = counter_priv(counter);
uint32_t cntval;
ftm_read(ftm, FTM_CNT, &cntval);
@@ -176,7 +176,7 @@ static int ftm_quaddec_count_write(struct counter_device *counter,
struct counter_count *count,
const u64 val)
{
- struct ftm_quaddec *const ftm = counter->priv;
+ struct ftm_quaddec *const ftm = counter_priv(counter);
if (val != 0) {
dev_warn(&ftm->pdev->dev, "Can only accept '0' as new counter value\n");
--
2.33.0
For now this just wraps accessing struct counter_device::priv. However
this is about to change and converting drivers to this helper
individually makes fixing device lifetime issues result in easier to
review patches.
Signed-off-by: Uwe Kleine-König <[email protected]>
---
drivers/counter/counter-core.c | 12 ++++++++++++
include/linux/counter.h | 2 ++
2 files changed, 14 insertions(+)
diff --git a/drivers/counter/counter-core.c b/drivers/counter/counter-core.c
index f053a43c6c04..00c41f28c101 100644
--- a/drivers/counter/counter-core.c
+++ b/drivers/counter/counter-core.c
@@ -45,6 +45,18 @@ static struct bus_type counter_bus_type = {
static dev_t counter_devt;
+/**
+ * counter_priv - access counter device private data
+ * @counter: counter device
+ *
+ * Get the counter device private data
+ */
+void *counter_priv(const struct counter_device *const counter)
+{
+ return counter->priv;
+}
+EXPORT_SYMBOL_GPL(counter_priv);
+
/**
* counter_register - register Counter to the system
* @counter: pointer to Counter to register
diff --git a/include/linux/counter.h b/include/linux/counter.h
index b7d0a00a61cf..8daaa38c71d8 100644
--- a/include/linux/counter.h
+++ b/include/linux/counter.h
@@ -329,6 +329,8 @@ struct counter_device {
struct mutex ops_exist_lock;
};
+void *counter_priv(const struct counter_device *const counter);
+
int counter_register(struct counter_device *const counter);
void counter_unregister(struct counter_device *const counter);
int devm_counter_register(struct device *dev,
--
2.33.0
On Mon, Dec 27, 2021 at 10:45:07AM +0100, Uwe Kleine-K?nig wrote:
> For now this just wraps accessing struct counter_device::priv. However
> this is about to change and converting drivers to this helper
> individually makes fixing device lifetime issues result in easier to
> review patches.
>
> Signed-off-by: Uwe Kleine-K?nig <[email protected]>
> ---
> drivers/counter/counter-core.c | 12 ++++++++++++
> include/linux/counter.h | 2 ++
> 2 files changed, 14 insertions(+)
>
> diff --git a/drivers/counter/counter-core.c b/drivers/counter/counter-core.c
> index f053a43c6c04..00c41f28c101 100644
> --- a/drivers/counter/counter-core.c
> +++ b/drivers/counter/counter-core.c
> @@ -45,6 +45,18 @@ static struct bus_type counter_bus_type = {
>
> static dev_t counter_devt;
>
> +/**
> + * counter_priv - access counter device private data
> + * @counter: counter device
> + *
> + * Get the counter device private data
> + */
> +void *counter_priv(const struct counter_device *const counter)
> +{
> + return counter->priv;
> +}
> +EXPORT_SYMBOL_GPL(counter_priv);
Shouldn't this be usin gdev_get_drvdata() and using the private data
pointer that is already on the struct device structure itself? The void
*priv; should be dropped from struct counter_device entirely.
Oh ick, I just now looked at 'struct counter_device', there are other
reference counting issues in there (hint, struct cdev has a reference
count...) But that's independent of this patch series...
thanks,
greg k-h
On 12/27/21 12:02 PM, Greg Kroah-Hartman wrote:
> On Mon, Dec 27, 2021 at 10:45:07AM +0100, Uwe Kleine-König wrote:
>> For now this just wraps accessing struct counter_device::priv. However
>> this is about to change and converting drivers to this helper
>> individually makes fixing device lifetime issues result in easier to
>> review patches.
>>
>> Signed-off-by: Uwe Kleine-König <[email protected]>
>> ---
>> drivers/counter/counter-core.c | 12 ++++++++++++
>> include/linux/counter.h | 2 ++
>> 2 files changed, 14 insertions(+)
>>
>> diff --git a/drivers/counter/counter-core.c b/drivers/counter/counter-core.c
>> index f053a43c6c04..00c41f28c101 100644
>> --- a/drivers/counter/counter-core.c
>> +++ b/drivers/counter/counter-core.c
>> @@ -45,6 +45,18 @@ static struct bus_type counter_bus_type = {
>>
>> static dev_t counter_devt;
>>
>> +/**
>> + * counter_priv - access counter device private data
>> + * @counter: counter device
>> + *
>> + * Get the counter device private data
>> + */
>> +void *counter_priv(const struct counter_device *const counter)
>> +{
>> + return counter->priv;
>> +}
>> +EXPORT_SYMBOL_GPL(counter_priv);
> Shouldn't this be usin gdev_get_drvdata() and using the private data
> pointer that is already on the struct device structure itself? The void
> *priv; should be dropped from struct counter_device entirely.
>
> Oh ick, I just now looked at 'struct counter_device', there are other
> reference counting issues in there (hint, struct cdev has a reference
> count...) But that's independent of this patch series...
This is not a problem. The struct cdev holds a reference to the struct
dev. This allows them to use the same allocation. As long as there is a
reference to the cdev there will be a reference to the dev and the
memory will be kept alive.
On Mon, Dec 27, 2021 at 12:34:28PM +0100, Lars-Peter Clausen wrote:
> On 12/27/21 12:02 PM, Greg Kroah-Hartman wrote:
> > On Mon, Dec 27, 2021 at 10:45:07AM +0100, Uwe Kleine-K?nig wrote:
> > > For now this just wraps accessing struct counter_device::priv. However
> > > this is about to change and converting drivers to this helper
> > > individually makes fixing device lifetime issues result in easier to
> > > review patches.
> > >
> > > Signed-off-by: Uwe Kleine-K?nig <[email protected]>
> > > ---
> > > drivers/counter/counter-core.c | 12 ++++++++++++
> > > include/linux/counter.h | 2 ++
> > > 2 files changed, 14 insertions(+)
> > >
> > > diff --git a/drivers/counter/counter-core.c b/drivers/counter/counter-core.c
> > > index f053a43c6c04..00c41f28c101 100644
> > > --- a/drivers/counter/counter-core.c
> > > +++ b/drivers/counter/counter-core.c
> > > @@ -45,6 +45,18 @@ static struct bus_type counter_bus_type = {
> > > static dev_t counter_devt;
> > > +/**
> > > + * counter_priv - access counter device private data
> > > + * @counter: counter device
> > > + *
> > > + * Get the counter device private data
> > > + */
> > > +void *counter_priv(const struct counter_device *const counter)
> > > +{
> > > + return counter->priv;
> > > +}
> > > +EXPORT_SYMBOL_GPL(counter_priv);
> > Shouldn't this be usin gdev_get_drvdata() and using the private data
> > pointer that is already on the struct device structure itself? The void
> > *priv; should be dropped from struct counter_device entirely.
> >
> > Oh ick, I just now looked at 'struct counter_device', there are other
> > reference counting issues in there (hint, struct cdev has a reference
> > count...) But that's independent of this patch series...
> This is not a problem. The struct cdev holds a reference to the struct dev.
> This allows them to use the same allocation. As long as there is a reference
> to the cdev there will be a reference to the dev and the memory will be kept
> alive.
Ick, a cdev shouldn't be doing stuff like that, but I see how people
like to use it that way :(
Ok, it's fine for now, but yet-another-reaason why the cdev api is a
mess in places...
thanks,
greg k-h
On 12/27/21 10:45 AM, Uwe Kleine-König wrote:
> [...]
>
> - I wonder why counter is a bus and not a class device type. There is
> no driver that would ever bind a counter device, is there? So
> /sys/bus/counter/driver is always empty.
>
There used to be a time when GKH said that we do not want new driver
classes. And all new subsystems should use bus since bus is a superset
of class. This restriction has been eased since then.
But it was around when the IIO subsystem was merged and since the
counter subsystem originated from the IIO subsystem I assume it just
copied this.
On 12/27/21 12:52 PM, Greg Kroah-Hartman wrote:
> On Mon, Dec 27, 2021 at 12:34:28PM +0100, Lars-Peter Clausen wrote:
>> On 12/27/21 12:02 PM, Greg Kroah-Hartman wrote:
>>> On Mon, Dec 27, 2021 at 10:45:07AM +0100, Uwe Kleine-König wrote:
>>>> For now this just wraps accessing struct counter_device::priv. However
>>>> this is about to change and converting drivers to this helper
>>>> individually makes fixing device lifetime issues result in easier to
>>>> review patches.
>>>>
>>>> Signed-off-by: Uwe Kleine-König <[email protected]>
>>>> ---
>>>> drivers/counter/counter-core.c | 12 ++++++++++++
>>>> include/linux/counter.h | 2 ++
>>>> 2 files changed, 14 insertions(+)
>>>>
>>>> diff --git a/drivers/counter/counter-core.c b/drivers/counter/counter-core.c
>>>> index f053a43c6c04..00c41f28c101 100644
>>>> --- a/drivers/counter/counter-core.c
>>>> +++ b/drivers/counter/counter-core.c
>>>> @@ -45,6 +45,18 @@ static struct bus_type counter_bus_type = {
>>>> static dev_t counter_devt;
>>>> +/**
>>>> + * counter_priv - access counter device private data
>>>> + * @counter: counter device
>>>> + *
>>>> + * Get the counter device private data
>>>> + */
>>>> +void *counter_priv(const struct counter_device *const counter)
>>>> +{
>>>> + return counter->priv;
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(counter_priv);
>>> Shouldn't this be usin gdev_get_drvdata() and using the private data
>>> pointer that is already on the struct device structure itself? The void
>>> *priv; should be dropped from struct counter_device entirely.
>>>
>>> Oh ick, I just now looked at 'struct counter_device', there are other
>>> reference counting issues in there (hint, struct cdev has a reference
>>> count...) But that's independent of this patch series...
>> This is not a problem. The struct cdev holds a reference to the struct dev.
>> This allows them to use the same allocation. As long as there is a reference
>> to the cdev there will be a reference to the dev and the memory will be kept
>> alive.
> Ick, a cdev shouldn't be doing stuff like that, but I see how people
> like to use it that way :(
>
Can you explain why cdev shouldn't be doing that? This commit has some
backstory why this is done
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=233ed09d7fdacf592ee91e6c97ce5f4364fbe7c0
On Mon, 27 Dec 2021 13:25:25 +0100
Lars-Peter Clausen <[email protected]> wrote:
> On 12/27/21 10:45 AM, Uwe Kleine-König wrote:
> > [...]
> >
> > - I wonder why counter is a bus and not a class device type. There is
> > no driver that would ever bind a counter device, is there? So
> > /sys/bus/counter/driver is always empty.
> >
> There used to be a time when GKH said that we do not want new driver
> classes. And all new subsystems should use bus since bus is a superset
> of class. This restriction has been eased since then.
>
> But it was around when the IIO subsystem was merged and since the
> counter subsystem originated from the IIO subsystem I assume it just
> copied this.
>
Yup. Discussion about this back then with one view being there
should never have been class in the first place.
https://lore.kernel.org/lkml/[email protected]/
For anyone who loves the history of these things...
FWIW I think Greg suggested IIO should be a bus because we were hanging
a bunch of different types of device off a class and it was getting messy.
Kay then gave some history on class vs bus and suggested no new
subsystem should use class.
Ah well, opinions change over time!
Also interesting to see we were discussing a bridge to input all that
time ago and it's still not gone beyond various prototypes (with
exception of touch screens).
Jonathan
On Mon, 27 Dec 2021 10:45:07 +0100
Uwe Kleine-König <[email protected]> wrote:
> For now this just wraps accessing struct counter_device::priv. However
> this is about to change and converting drivers to this helper
> individually makes fixing device lifetime issues result in easier to
> review patches.
>
> Signed-off-by: Uwe Kleine-König <[email protected]>
Reviewed-by: Jonathan Cameron <[email protected]>
> ---
> drivers/counter/counter-core.c | 12 ++++++++++++
> include/linux/counter.h | 2 ++
> 2 files changed, 14 insertions(+)
>
> diff --git a/drivers/counter/counter-core.c b/drivers/counter/counter-core.c
> index f053a43c6c04..00c41f28c101 100644
> --- a/drivers/counter/counter-core.c
> +++ b/drivers/counter/counter-core.c
> @@ -45,6 +45,18 @@ static struct bus_type counter_bus_type = {
>
> static dev_t counter_devt;
>
> +/**
> + * counter_priv - access counter device private data
> + * @counter: counter device
> + *
> + * Get the counter device private data
> + */
> +void *counter_priv(const struct counter_device *const counter)
> +{
> + return counter->priv;
> +}
> +EXPORT_SYMBOL_GPL(counter_priv);
> +
> /**
> * counter_register - register Counter to the system
> * @counter: pointer to Counter to register
> diff --git a/include/linux/counter.h b/include/linux/counter.h
> index b7d0a00a61cf..8daaa38c71d8 100644
> --- a/include/linux/counter.h
> +++ b/include/linux/counter.h
> @@ -329,6 +329,8 @@ struct counter_device {
> struct mutex ops_exist_lock;
> };
>
> +void *counter_priv(const struct counter_device *const counter);
> +
> int counter_register(struct counter_device *const counter);
> void counter_unregister(struct counter_device *const counter);
> int devm_counter_register(struct device *dev,
On Mon, 27 Dec 2021 10:45:09 +0100
Uwe Kleine-König <[email protected]> wrote:
> This is a straight forward conversion to the new counter_priv() wrapper.
>
> Signed-off-by: Uwe Kleine-König <[email protected]>
Acked-by: Jonathan Cameron <[email protected]>
> ---
> drivers/counter/interrupt-cnt.c | 10 +++++-----
> 1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/counter/interrupt-cnt.c b/drivers/counter/interrupt-cnt.c
> index 8514a87fcbee..4bf706ef46e2 100644
> --- a/drivers/counter/interrupt-cnt.c
> +++ b/drivers/counter/interrupt-cnt.c
> @@ -37,7 +37,7 @@ static irqreturn_t interrupt_cnt_isr(int irq, void *dev_id)
> static int interrupt_cnt_enable_read(struct counter_device *counter,
> struct counter_count *count, u8 *enable)
> {
> - struct interrupt_cnt_priv *priv = counter->priv;
> + struct interrupt_cnt_priv *priv = counter_priv(counter);
>
> *enable = priv->enabled;
>
> @@ -47,7 +47,7 @@ static int interrupt_cnt_enable_read(struct counter_device *counter,
> static int interrupt_cnt_enable_write(struct counter_device *counter,
> struct counter_count *count, u8 enable)
> {
> - struct interrupt_cnt_priv *priv = counter->priv;
> + struct interrupt_cnt_priv *priv = counter_priv(counter);
>
> if (priv->enabled == enable)
> return 0;
> @@ -85,7 +85,7 @@ static int interrupt_cnt_action_read(struct counter_device *counter,
> static int interrupt_cnt_read(struct counter_device *counter,
> struct counter_count *count, u64 *val)
> {
> - struct interrupt_cnt_priv *priv = counter->priv;
> + struct interrupt_cnt_priv *priv = counter_priv(counter);
>
> *val = atomic_read(&priv->count);
>
> @@ -95,7 +95,7 @@ static int interrupt_cnt_read(struct counter_device *counter,
> static int interrupt_cnt_write(struct counter_device *counter,
> struct counter_count *count, const u64 val)
> {
> - struct interrupt_cnt_priv *priv = counter->priv;
> + struct interrupt_cnt_priv *priv = counter_priv(counter);
>
> if (val != (typeof(priv->count.counter))val)
> return -ERANGE;
> @@ -122,7 +122,7 @@ static int interrupt_cnt_signal_read(struct counter_device *counter,
> struct counter_signal *signal,
> enum counter_signal_level *level)
> {
> - struct interrupt_cnt_priv *priv = counter->priv;
> + struct interrupt_cnt_priv *priv = counter_priv(counter);
> int ret;
>
> if (!priv->gpio)
On Mon, 27 Dec 2021 10:45:12 +0100
Uwe Kleine-König <[email protected]> wrote:
> This is a straight forward conversion to the new counter_priv() wrapper.
>
> Signed-off-by: Uwe Kleine-König <[email protected]>
Acked-by: Jonathan Cameron <[email protected]>
> ---
> drivers/counter/ftm-quaddec.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/counter/ftm-quaddec.c b/drivers/counter/ftm-quaddec.c
> index 9272f7b58beb..f5d92df6a611 100644
> --- a/drivers/counter/ftm-quaddec.c
> +++ b/drivers/counter/ftm-quaddec.c
> @@ -118,7 +118,7 @@ static void ftm_quaddec_disable(void *ftm)
> static int ftm_quaddec_get_prescaler(struct counter_device *counter,
> struct counter_count *count, u32 *cnt_mode)
> {
> - struct ftm_quaddec *ftm = counter->priv;
> + struct ftm_quaddec *ftm = counter_priv(counter);
> uint32_t scflags;
>
> ftm_read(ftm, FTM_SC, &scflags);
> @@ -131,7 +131,7 @@ static int ftm_quaddec_get_prescaler(struct counter_device *counter,
> static int ftm_quaddec_set_prescaler(struct counter_device *counter,
> struct counter_count *count, u32 cnt_mode)
> {
> - struct ftm_quaddec *ftm = counter->priv;
> + struct ftm_quaddec *ftm = counter_priv(counter);
>
> mutex_lock(&ftm->ftm_quaddec_mutex);
>
> @@ -162,7 +162,7 @@ static int ftm_quaddec_count_read(struct counter_device *counter,
> struct counter_count *count,
> u64 *val)
> {
> - struct ftm_quaddec *const ftm = counter->priv;
> + struct ftm_quaddec *const ftm = counter_priv(counter);
> uint32_t cntval;
>
> ftm_read(ftm, FTM_CNT, &cntval);
> @@ -176,7 +176,7 @@ static int ftm_quaddec_count_write(struct counter_device *counter,
> struct counter_count *count,
> const u64 val)
> {
> - struct ftm_quaddec *const ftm = counter->priv;
> + struct ftm_quaddec *const ftm = counter_priv(counter);
>
> if (val != 0) {
> dev_warn(&ftm->pdev->dev, "Can only accept '0' as new counter value\n");
On Mon, Dec 27, 2021 at 10:45:07AM +0100, Uwe Kleine-König wrote:
> For now this just wraps accessing struct counter_device::priv. However
> this is about to change and converting drivers to this helper
> individually makes fixing device lifetime issues result in easier to
> review patches.
>
> Signed-off-by: Uwe Kleine-König <[email protected]>
Acked-by: William Breathitt Gray <[email protected]>
> ---
> drivers/counter/counter-core.c | 12 ++++++++++++
> include/linux/counter.h | 2 ++
> 2 files changed, 14 insertions(+)
>
> diff --git a/drivers/counter/counter-core.c b/drivers/counter/counter-core.c
> index f053a43c6c04..00c41f28c101 100644
> --- a/drivers/counter/counter-core.c
> +++ b/drivers/counter/counter-core.c
> @@ -45,6 +45,18 @@ static struct bus_type counter_bus_type = {
>
> static dev_t counter_devt;
>
> +/**
> + * counter_priv - access counter device private data
> + * @counter: counter device
> + *
> + * Get the counter device private data
> + */
> +void *counter_priv(const struct counter_device *const counter)
> +{
> + return counter->priv;
> +}
> +EXPORT_SYMBOL_GPL(counter_priv);
> +
> /**
> * counter_register - register Counter to the system
> * @counter: pointer to Counter to register
> diff --git a/include/linux/counter.h b/include/linux/counter.h
> index b7d0a00a61cf..8daaa38c71d8 100644
> --- a/include/linux/counter.h
> +++ b/include/linux/counter.h
> @@ -329,6 +329,8 @@ struct counter_device {
> struct mutex ops_exist_lock;
> };
>
> +void *counter_priv(const struct counter_device *const counter);
> +
> int counter_register(struct counter_device *const counter);
> void counter_unregister(struct counter_device *const counter);
> int devm_counter_register(struct device *dev,
> --
> 2.33.0
>
On Mon, Dec 27, 2021 at 10:45:09AM +0100, Uwe Kleine-König wrote:
> This is a straight forward conversion to the new counter_priv() wrapper.
>
> Signed-off-by: Uwe Kleine-König <[email protected]>
Acked-by: William Breathitt Gray <[email protected]>
> ---
> drivers/counter/interrupt-cnt.c | 10 +++++-----
> 1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/counter/interrupt-cnt.c b/drivers/counter/interrupt-cnt.c
> index 8514a87fcbee..4bf706ef46e2 100644
> --- a/drivers/counter/interrupt-cnt.c
> +++ b/drivers/counter/interrupt-cnt.c
> @@ -37,7 +37,7 @@ static irqreturn_t interrupt_cnt_isr(int irq, void *dev_id)
> static int interrupt_cnt_enable_read(struct counter_device *counter,
> struct counter_count *count, u8 *enable)
> {
> - struct interrupt_cnt_priv *priv = counter->priv;
> + struct interrupt_cnt_priv *priv = counter_priv(counter);
>
> *enable = priv->enabled;
>
> @@ -47,7 +47,7 @@ static int interrupt_cnt_enable_read(struct counter_device *counter,
> static int interrupt_cnt_enable_write(struct counter_device *counter,
> struct counter_count *count, u8 enable)
> {
> - struct interrupt_cnt_priv *priv = counter->priv;
> + struct interrupt_cnt_priv *priv = counter_priv(counter);
>
> if (priv->enabled == enable)
> return 0;
> @@ -85,7 +85,7 @@ static int interrupt_cnt_action_read(struct counter_device *counter,
> static int interrupt_cnt_read(struct counter_device *counter,
> struct counter_count *count, u64 *val)
> {
> - struct interrupt_cnt_priv *priv = counter->priv;
> + struct interrupt_cnt_priv *priv = counter_priv(counter);
>
> *val = atomic_read(&priv->count);
>
> @@ -95,7 +95,7 @@ static int interrupt_cnt_read(struct counter_device *counter,
> static int interrupt_cnt_write(struct counter_device *counter,
> struct counter_count *count, const u64 val)
> {
> - struct interrupt_cnt_priv *priv = counter->priv;
> + struct interrupt_cnt_priv *priv = counter_priv(counter);
>
> if (val != (typeof(priv->count.counter))val)
> return -ERANGE;
> @@ -122,7 +122,7 @@ static int interrupt_cnt_signal_read(struct counter_device *counter,
> struct counter_signal *signal,
> enum counter_signal_level *level)
> {
> - struct interrupt_cnt_priv *priv = counter->priv;
> + struct interrupt_cnt_priv *priv = counter_priv(counter);
> int ret;
>
> if (!priv->gpio)
> --
> 2.33.0
>
On Mon, Dec 27, 2021 at 10:45:12AM +0100, Uwe Kleine-König wrote:
> This is a straight forward conversion to the new counter_priv() wrapper.
>
> Signed-off-by: Uwe Kleine-König <[email protected]>
Acked-by: William Breathitt Gray <[email protected]>
> ---
> drivers/counter/ftm-quaddec.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/counter/ftm-quaddec.c b/drivers/counter/ftm-quaddec.c
> index 9272f7b58beb..f5d92df6a611 100644
> --- a/drivers/counter/ftm-quaddec.c
> +++ b/drivers/counter/ftm-quaddec.c
> @@ -118,7 +118,7 @@ static void ftm_quaddec_disable(void *ftm)
> static int ftm_quaddec_get_prescaler(struct counter_device *counter,
> struct counter_count *count, u32 *cnt_mode)
> {
> - struct ftm_quaddec *ftm = counter->priv;
> + struct ftm_quaddec *ftm = counter_priv(counter);
> uint32_t scflags;
>
> ftm_read(ftm, FTM_SC, &scflags);
> @@ -131,7 +131,7 @@ static int ftm_quaddec_get_prescaler(struct counter_device *counter,
> static int ftm_quaddec_set_prescaler(struct counter_device *counter,
> struct counter_count *count, u32 cnt_mode)
> {
> - struct ftm_quaddec *ftm = counter->priv;
> + struct ftm_quaddec *ftm = counter_priv(counter);
>
> mutex_lock(&ftm->ftm_quaddec_mutex);
>
> @@ -162,7 +162,7 @@ static int ftm_quaddec_count_read(struct counter_device *counter,
> struct counter_count *count,
> u64 *val)
> {
> - struct ftm_quaddec *const ftm = counter->priv;
> + struct ftm_quaddec *const ftm = counter_priv(counter);
> uint32_t cntval;
>
> ftm_read(ftm, FTM_CNT, &cntval);
> @@ -176,7 +176,7 @@ static int ftm_quaddec_count_write(struct counter_device *counter,
> struct counter_count *count,
> const u64 val)
> {
> - struct ftm_quaddec *const ftm = counter->priv;
> + struct ftm_quaddec *const ftm = counter_priv(counter);
>
> if (val != 0) {
> dev_warn(&ftm->pdev->dev, "Can only accept '0' as new counter value\n");
> --
> 2.33.0
>
On Tue, Dec 28, 2021 at 05:35:58PM +0000, Jonathan Cameron wrote:
> On Mon, 27 Dec 2021 13:25:25 +0100
> Lars-Peter Clausen <[email protected]> wrote:
>
> > On 12/27/21 10:45 AM, Uwe Kleine-König wrote:
> > > [...]
> > >
> > > - I wonder why counter is a bus and not a class device type. There is
> > > no driver that would ever bind a counter device, is there? So
> > > /sys/bus/counter/driver is always empty.
> > >
> > There used to be a time when GKH said that we do not want new driver
> > classes. And all new subsystems should use bus since bus is a superset
> > of class. This restriction has been eased since then.
> >
> > But it was around when the IIO subsystem was merged and since the
> > counter subsystem originated from the IIO subsystem I assume it just
> > copied this.
> >
>
> Yup. Discussion about this back then with one view being there
> should never have been class in the first place.
>
> https://lore.kernel.org/lkml/[email protected]/
>
> For anyone who loves the history of these things...
>
> FWIW I think Greg suggested IIO should be a bus because we were hanging
> a bunch of different types of device off a class and it was getting messy.
> Kay then gave some history on class vs bus and suggested no new
> subsystem should use class.
>
> Ah well, opinions change over time!
>
> Also interesting to see we were discussing a bridge to input all that
> time ago and it's still not gone beyond various prototypes (with
> exception of touch screens).
>
> Jonathan
Yes this is the reason: Counter subsystem just followed the structure of
the IIO subsystem originally which is how it ended up as a bus; changing
it to a class now would break userspace expectations so that is why it
remains a bus still.
William Breathitt Gray