Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1571941 > unrolled thread

Re: [PATCH 4/6] bus: add driver for the Technologic Systems NBUS

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 4/6] bus: add driver for the Technologic Systems NBUS Sebastien Bourdelin <sebastien.bourdelin@savoirfairelinux.com> - 2017-02-01 21:00 +0100
    Re: [PATCH 4/6] bus: add driver for the Technologic Systems NBUS Linus Walleij <linus.walleij@linaro.org> - 2017-02-03 15:00 +0100
      Re: [PATCH 4/6] bus: add driver for the Technologic Systems NBUS Sebastien Bourdelin <sebastien.bourdelin@savoirfairelinux.com> - 2017-02-03 17:00 +0100

#1571941 — Re: [PATCH 4/6] bus: add driver for the Technologic Systems NBUS

Hi Linus,

Thanks for your feedback.
I have a question regarding your recommendation, see below.

On 12/30/2016 02:58 AM, Linus Walleij wrote:

>> +
>> +static DEFINE_MUTEX(ts_nbus_lock);
>> +static bool ts_nbus_ready;
> 
> Why not move this to the struct ts_nbus state container?
> 
> It seems to be per-bus not per-system.
> 
>> +#define TS_NBUS_READ_MODE  0
>> +#define TS_NBUS_WRITE_MODE 1
>> +#define TS_NBUS_DIRECTION_IN  0
>> +#define TS_NBUS_DIRECTION_OUT 1
>> +#define TS_NBUS_WRITE_ADR 0
>> +#define TS_NBUS_WRITE_VAL 1
>> +
>> +struct ts_nbus {
>> +       struct pwm_device *pwm;
>> +       int num_data;
>> +       int *data;
>> +       int csn;
>> +       int txrx;
>> +       int strobe;
>> +       int ale;
>> +       int rdy;
>> +};
>> +
>> +static struct ts_nbus *ts_nbus;
> 
> Nopes. No singletons please.
> 
> Use the state container pattern:
> Documentation/driver-model/design-patterns.txt
> 

I understand the idea but have problem to find a good way to implement it.

Other drivers using the NBUS which are child nodes in the device tree
will use the ts_nbus_write() and ts_nbus_read() functions, it means these
drivers should have a pointer to the allocated ts_nbus and pass it to
the write() and read() functions as an argument if i'm not using a
singleton here.
But i'm lacking knowledge on how to properly share this pointer when
initializing the NBUS driver with the child nodes.

Perhaps my design is not appropriate for what i'm doing, if someone can
point me on a similar problematic it will be really helpful.

Best Regards,
Sebastien.

[toc] | [next] | [standalone]

#1573105

On Wed, Feb 1, 2017 at 8:56 PM, Sebastien Bourdelin
<sebastien.bourdelin@savoirfairelinux.com> wrote:
> On 12/30/2016 02:58 AM, Linus Walleij wrote:

>>> +static struct ts_nbus *ts_nbus;
>>
>> Nopes. No singletons please.
>>
>> Use the state container pattern:
>> Documentation/driver-model/design-patterns.txt
>
> I understand the idea but have problem to find a good way to implement it.
>
> Other drivers using the NBUS which are child nodes in the device tree
> will use the ts_nbus_write() and ts_nbus_read() functions, it means these
> drivers should have a pointer to the allocated ts_nbus and pass it to
> the write() and read() functions as an argument if i'm not using a
> singleton here.
> But i'm lacking knowledge on how to properly share this pointer when
> initializing the NBUS driver with the child nodes.

Your child nodes I guess will be instatiated as devices as well.

These devices will have the NBUS driver as .parent in their
struct device I guess. Else the design of this bus is tilted.

If the NBUS driver use dev_set_drvdata(dev, state_container_cookie)
the children can use dev_get_drvdata(dev->parent); to get a pointer to
the same cookie.

The subdrivers don't even need to know the members of the state
container as long as you're just passing a pointer to it. It's
enough if you forward-declare it as a "pointer to some struct":

struct foo;

{
   struct foo *fooptr = dev_get_drvdata(dev->parent);
   write(fooptr, 0x10);

etc

Yours,
Linus Walleij

[toc] | [prev] | [next] | [standalone]

#1573203

Hi Linus,

On 02/03/2017 08:51 AM, Linus Walleij wrote:
> Your child nodes I guess will be instatiated as devices as well.
> 
> These devices will have the NBUS driver as .parent in their
> struct device I guess. Else the design of this bus is tilted.
> 
> If the NBUS driver use dev_set_drvdata(dev, state_container_cookie)
> the children can use dev_get_drvdata(dev->parent); to get a pointer to
> the same cookie.
> 
> The subdrivers don't even need to know the members of the state
> container as long as you're just passing a pointer to it. It's
> enough if you forward-declare it as a "pointer to some struct":
> 
> struct foo;
> 
> {
>    struct foo *fooptr = dev_get_drvdata(dev->parent);
>    write(fooptr, 0x10);
> 
> etc

Thanks a lot, this is perfect!

Best Regards,
Sebastien.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web