Hi,
On Fri, Jan 24, 2020 at 10:07 AM Ladislav Michl <
address@hidden> wrote:
> On Fri, Jan 24, 2020 at 09:48:25AM +0100, Pawel Kot wrote:
> > On Thu, Jan 23, 2020 at 2:02 AM Ladislav Michl <
address@hidden> wrote:
> > > +const static gn_device_ops _bluetooth_ops = {
> > > + .open = _bluetooth_open,
> >
> > Why just open with _ prefix? Overall question for other places as well.
>
> It would be more convenient to have all device functions with the same
> prototype. It is not the case (yet) and I wanted to have plugin change
> isolated without touching device drivers.
>
> So I just introduced _ prefixed variants to keep change in one place.
> Later we can unify driver functions to use the same prototype and
> this will go away. So it can have any name, feel free to suggest
> something better.
Fair enough, but let me review it over the weekend more thoroughly.
> > > + .close = bluetooth_close,
> > > + .select = bluetooth_select,
> > > + .read = bluetooth_read,
> > > + .write = bluetooth_write,
> > > +};
> >
> > > gn_error device_nreceived(int *n, struct gn_statemachine *state)
> > [...]
> > > + return state->device.ops->nrcvd(state->device.fd, n, state);
> >
> > I would leave naming consistent like nreceived everywhere. It applies not
> > just here.
>
> It was there, but it is too long to have device_ops above nicely aligned.
> This way all ops names are about the same lenght. If you prefer longer
> variant, let's change that.
I do not mind longer or shorter. I would only prefer have the same naming in function names and in structure fields.
> > OK, so this part is just being moved.
>
> Snipets are shuffled around to have all device checks in one place
> and the rest at another. No problem dropping that change or having
> separate patch for it.
Separate patch would be nice.
> > > -void device_reset(struct gn_statemachine *state);
> >
> > Why?
>
> This function is not used anywhere. Perhaps base device change on some
> preparation cleanup patches would make you more happy :)
OK.
> > > typedef struct {
> > > int fd;
> > > - gn_connection_type type;
> >
> > Won't we need it anymore?
>
> No, we have device_ops now. Also it would allow us to remove all stuff like
> (fbus_serial_open):
> if (state->config.connection_type == GN_CT_TCP)
> type = GN_CT_TCP;
> else
> type = GN_CT_Serial;
> as we handle all devices consistently now.
Sounds good, thanks.
Cheers,
--
Pawel Kot