Page MenuHomeFreeBSD
Differential D35701 Diff 107761 sys/dev/iicbus/mux/pca954x.c

Changeset View

Standalone View

View Options

sys/dev/iicbus/mux/pca954x.c

/*- /*-
* SPDX-License-Identifier: BSD-2-Clause * SPDX-License-Identifier: BSD-2-Clause
* *
* Copyright (c) Andriy Gapon * Copyright (c) 2019 Ian Lepore <ian@freebsd.org>
* Copyright (c) 2020-2021 Andriy Gapon
* Copyright (c) 2022 Bjoern A. Zeeb
* *
* Redistribution and use in source and binary forms, with or without * Redistribution and use in source and binary forms, with or without
* modification, are permitted provided that the following conditions * modification, are permitted provided that the following conditions
* are met: * are met:
* 1. Redistributions of source code must retain the above copyright * 1. Redistributions of source code must retain the above copyright
* notice, this list of conditions, and the following disclaimer. * notice, this list of conditions and the following disclaimer.
impUnsubmitted
Done
Inline Actions

this drops a comma.

imp: this drops a comma.
bzAuthorUnsubmitted
Done
Inline Actions

Given this got merged from two files, ... also the comma is not in /usr/share/etc/bsd-style-copyright . I can add this back is needed but eventually we'll need to align this "templates" (also with the one for the SPDX tag)

bz: Given this got merged from two files, ... also the comma is not in /usr/share/etc/bsd-style…
* 2. Redistributions in binary form must reproduce the above copyright * 2. Redistributions in binary form must reproduce the above copyright
* notice, this list of conditions and the following disclaimer in the * notice, this list of conditions and the following disclaimer in the
* documentation and/or other materials provided with the distribution. * documentation and/or other materials provided with the distribution.
* *
* THIS SOFTWARE IS PROVIDED BY THE AUTHOR AND CONTRIBUTORS ``AS IS'' AND * THIS SOFTWARE IS PROVIDED BY THE AUTHOR AND CONTRIBUTORS ``AS IS'' AND
* ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
* IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
* ARE DISCLAIMED. IN NO EVENT SHALL THE AUTHOR OR CONTRIBUTORS BE LIABLE FOR * ARE DISCLAIMED. IN NO EVENT SHALL THE AUTHOR OR CONTRIBUTORS BE LIABLE
* ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL * FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
* DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS
* OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) * OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION)
* HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT * HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT
* LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY * LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY
* OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF * OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
* SUCH DAMAGE. * SUCH DAMAGE.
*
*/ */
#include <sys/cdefs.h> #include <sys/cdefs.h>
__FBSDID("$FreeBSD$"); __FBSDID("$FreeBSD$");
#include "opt_platform.h" #include "opt_platform.h"
#include <sys/param.h> #include <sys/param.h>
Show All 9 Lines
#endif #endif
#include <dev/iicbus/iicbus.h> #include <dev/iicbus/iicbus.h>
#include <dev/iicbus/iiconf.h> #include <dev/iicbus/iiconf.h>
#include "iicbus_if.h" #include "iicbus_if.h"
#include "iicmux_if.h" #include "iicmux_if.h"
#include <dev/iicbus/mux/iicmux.h> #include <dev/iicbus/mux/iicmux.h>
enum pca954x_type {
PCA954X_MUX,
PCA954X_SW,
};
struct pca954x_descr { struct pca954x_descr {
const char *partname; const char *partname;
const char *description; const char *description;
int numchannels; enum pca954x_type type;
uint8_t numchannels;
uint8_t enable;
}; };
static struct pca954x_descr pca9540_descr = {
.partname = "pca9540",
.description = "PCA9540B I2C Mux",
.type = PCA954X_MUX,
.numchannels = 2,
.enable = 0x04,
};
static struct pca954x_descr pca9547_descr = {
.partname = "pca9547",
.description = "PCA9547 I2C Mux",
.type = PCA954X_MUX,
.numchannels = 8,
.enable = 0x08,
};
static struct pca954x_descr pca9548_descr = { static struct pca954x_descr pca9548_descr = {
.partname = "pca9548", .partname = "pca9548",
.description = "PCA9548A I2C Mux", .description = "PCA9548A I2C Switch",
.type = PCA954X_SW,
.numchannels = 8, .numchannels = 8,
}; };
#ifdef FDT #ifdef FDT
static struct ofw_compat_data compat_data[] = { static struct ofw_compat_data compat_data[] = {
{ "nxp,pca9540", (uintptr_t)&pca9540_descr },
{ "nxp,pca9547", (uintptr_t)&pca9547_descr },
{ "nxp,pca9548", (uintptr_t)&pca9548_descr }, { "nxp,pca9548", (uintptr_t)&pca9548_descr },
{ NULL, 0 }, { NULL, 0 },
}; };
#else #else
static struct pca954x_descr *part_descrs[] = { static struct pca954x_descr *part_descrs[] = {
&pca9540_descr,
&pca9547_descr,
&pca9548_descr, &pca9548_descr,
}; };
#endif #endif
struct pca954x_softc { struct pca954x_softc {
struct iicmux_softc mux; struct iicmux_softc mux;
const struct pca954x_descr *descr;
uint8_t addr; uint8_t addr;
bool idle_disconnect; bool idle_disconnect;
}; };
static int static int
pca954x_bus_select(device_t dev, int busidx, struct iic_reqbus_data *rd) pca954x_bus_select(device_t dev, int busidx, struct iic_reqbus_data *rd)
{ {
struct pca954x_softc *sc;
struct iic_msg msg; struct iic_msg msg;
struct pca954x_softc *sc = device_get_softc(dev);
uint8_t busbits;
int error; int error;
uint8_t busbits;
sc = device_get_softc(dev);
/* /*
avgUnsubmitted
Done
Inline Actions

Style nit: I'd add an empty line before the multi-line comment.

avg: Style nit: I'd add an empty line before the multi-line comment.
* The iicmux caller ensures busidx is between 0 and the number of buses * The iicmux caller ensures busidx is between 0 and the number of buses
* we passed to iicmux_init_softc(), no need for validation here. If * we passed to iicmux_init_softc(), no need for validation here. If
* the fdt data has the idle_disconnect property we idle the bus by * the fdt data has the idle_disconnect property we idle the bus by
* selecting no downstream buses, otherwise we just leave the current * selecting no downstream buses, otherwise we just leave the current
* bus active. * bus active.
*/ */
if (busidx == IICMUX_SELECT_IDLE) { if (busidx == IICMUX_SELECT_IDLE) {
if (sc->idle_disconnect) if (sc->idle_disconnect)
busbits = 0; busbits = 0;
else else
return (0); return (0);
} else { } else if (sc->descr->type == PCA954X_MUX) {
uint8_t en;
en = sc->descr->enable;
KASSERT(en > 0 && powerof2(en), ("%s: %s enable %#x "
"invalid\n", __func__, sc->descr->partname, en));
busbits = en | (busidx & (en - 1));
} else if (sc->descr->type == PCA954X_SW) {
bzAuthorUnsubmitted
Done
Inline Actions

Added { } here and below for consistency.

bz: Added { } here and below for consistency.
busbits = 1u << busidx; busbits = 1u << busidx;
} else {
panic("%s: %s: unsupported type %d\n",
__func__, sc->descr->partname, sc->descr->type);
avgUnsubmitted
Done
Inline Actions

Can this be "collapsed" into 'else if'?
Also, there is no default /catch-all else clause... maybe it's worthwhile adding it with some error and diagnostic? Or panic?

avg: Can this be "collapsed" into 'else if'? Also, there is no default /catch-all else clause...
} }
avgUnsubmitted
Done
Inline Actions

I think that what should be asserted here is that en is a power of 2, not just an even number.
Also, if en == 0 is allowed here, then this clause would cover the next clause as well.

avg: I think that what should be asserted here is that en is a power of 2, not just an even number.
bzAuthorUnsubmitted
Done
Inline Actions

I cannot follow the en == 0 logic as

busidx != (1 << busdix);

I am probably misunderstanding here.

I'll do the power2 check though.

bz: I cannot follow the en == 0 logic as ``` busidx != (1 << busdix); ``` I am probably…
avgUnsubmitted
Done
Inline Actions

Apologies, I misread the code.

avg: Apologies, I misread the code.
msg.slave = sc->addr; msg.slave = sc->addr;
msg.flags = IIC_M_WR; msg.flags = IIC_M_WR;
avgUnsubmitted
Done
Inline Actions

What does the XXX comment suggest here?
It wasn't clear to me.
I do not see how anything from iic_reqbus_data::flags could affect iic_msg::flags.

avg: What does the XXX comment suggest here? It wasn't clear to me. I do not see how anything from…
msg.len = 1; msg.len = 1;
msg.buf = &busbits; msg.buf = &busbits;
error = iicbus_transfer(dev, &msg, 1); error = iicbus_transfer(dev, &msg, 1);
return (error); return (error);
} }
static const struct pca954x_descr * static const struct pca954x_descr *
pca954x_find_chip(device_t dev) pca954x_find_chip(device_t dev)
{ {
#ifdef FDT #ifdef FDT
const struct ofw_compat_data *compat; const struct ofw_compat_data *compat;
if (!ofw_bus_status_okay(dev))
mmelUnsubmitted
Done
Inline Actions

I don't think it should be #ifdefed. The "okay" test is obviously missing in the original probe method.

mmel: I don't think it should be #ifdefed. The "okay" test is obviously missing in the original…
bzAuthorUnsubmitted
Done
Inline Actions

Is that status inherited? I never saw a '''status = "okay"''' inside the '''i2c-switch''' blocks, only in the upper '''i2c'' node.

bz: Is that status inherited? I never saw a '''status = "okay"''' inside the '''i2c-switch'''…
impUnsubmitted
Done
Inline Actions

Generally only direct children of ofwbus/fdtbus need to check this.

Also, I have a huge patch set I've been rebasing forward for years that hoists this check into the bus code and now seems like a good time to solicit comments on that :)

imp: Generally only direct children of ofwbus/fdtbus need to check this. Also, I have a huge patch…
bzAuthorUnsubmitted
Done
Inline Actions

Right but please on a separate review... I'll just remove the #if 0 block..., right?

bz: Right but please on a separate review... I'll just remove the #if 0 block..., right?
avgUnsubmitted
Done
Inline Actions

I think that adding the okay status was a correct change.
A mux / switch is a device tree node, so it can be disabled via its status property.
This was an oversight in my code.

avg: I think that adding the okay status was a correct change. A mux / switch is a device tree node…
return (NULL);
bzAuthorUnsubmitted
Done
Inline Actions

and this has become a return (NULL); c&p error.

bz: and this has become a return (NULL); c&p error.
compat = ofw_bus_search_compatible(dev, compat_data); compat = ofw_bus_search_compatible(dev, compat_data);
if (compat == NULL) if (compat == NULL)
return (NULL); return (NULL);
return ((const struct pca954x_descr *)compat->ocd_data); return ((const struct pca954x_descr *)compat->ocd_data);
#else #else
const char *type; const char *type;
int i; int i;
Show All 19 Linespca954x_probe(device_t dev)
device_set_desc(dev, descr->description); device_set_desc(dev, descr->description);
return (BUS_PROBE_DEFAULT); return (BUS_PROBE_DEFAULT);
} }
static int static int
pca954x_attach(device_t dev) pca954x_attach(device_t dev)
{ {
#ifdef FDT
phandle_t node;
#endif
struct pca954x_softc *sc; struct pca954x_softc *sc;
const struct pca954x_descr *descr; const struct pca954x_descr *descr;
int err; int error;
sc = device_get_softc(dev); sc = device_get_softc(dev);
sc->addr = iicbus_get_addr(dev); sc->addr = iicbus_get_addr(dev);
#ifdef FDT sc->idle_disconnect = device_has_property(dev, "i2c-mux-idle-disconnect");
node = ofw_bus_get_node(dev);
sc->idle_disconnect = OF_hasprop(node, "i2c-mux-idle-disconnect");
#endif
descr = pca954x_find_chip(dev); sc->descr = descr = pca954x_find_chip(dev);
err = iicmux_attach(dev, device_get_parent(dev), descr->numchannels); error = iicmux_attach(dev, device_get_parent(dev), descr->numchannels);
if (err == 0) if (error == 0)
bus_generic_attach(dev); bus_generic_attach(dev);
return (err);
return (error);
} }
static int static int
pca954x_detach(device_t dev) pca954x_detach(device_t dev)
{ {
int err; int error;
err = iicmux_detach(dev); error = iicmux_detach(dev);
return (err); return (error);
} }
static device_method_t pca954x_methods[] = { static device_method_t pca954x_methods[] = {
/* device methods */ /* device methods */
DEVMETHOD(device_probe, pca954x_probe), DEVMETHOD(device_probe, pca954x_probe),
DEVMETHOD(device_attach, pca954x_attach), DEVMETHOD(device_attach, pca954x_attach),
DEVMETHOD(device_detach, pca954x_detach), DEVMETHOD(device_detach, pca954x_detach),
/* iicmux methods */ /* iicmux methods */
DEVMETHOD(iicmux_bus_select, pca954x_bus_select), DEVMETHOD(iicmux_bus_select, pca954x_bus_select),
DEVMETHOD_END DEVMETHOD_END
}; };
DEFINE_CLASS_1(pca9548, pca954x_driver, pca954x_methods, DEFINE_CLASS_1(pca954x, pca954x_driver, pca954x_methods,
sizeof(struct pca954x_softc), iicmux_driver); sizeof(struct pca954x_softc), iicmux_driver);
DRIVER_MODULE(pca9548, iicbus, pca954x_driver, 0, 0); DRIVER_MODULE(pca954x, iicbus, pca954x_driver, 0, 0);
#ifdef FDT #ifdef FDT
DRIVER_MODULE(ofw_iicbus, pca9548, ofw_iicbus_driver, 0, 0); DRIVER_MODULE(ofw_iicbus, pca954x, ofw_iicbus_driver, 0, 0);
#else #else
DRIVER_MODULE(iicbus, pca9548, iicbus_driver, 0, 0); DRIVER_MODULE(iicbus, pca954x, iicbus_driver, 0, 0);
#endif #endif
MODULE_DEPEND(pca9548, iicmux, 1, 1, 1); MODULE_DEPEND(pca954x, iicmux, 1, 1, 1);
MODULE_DEPEND(pca9548, iicbus, IICBUS_MINVER, IICBUS_PREFVER, IICBUS_MAXVER); MODULE_DEPEND(pca954x, iicbus, IICBUS_MINVER, IICBUS_PREFVER, IICBUS_MAXVER);
MODULE_VERSION(pca9548, 1); MODULE_VERSION(pca954x, 1);
#ifdef FDT #ifdef FDT
IICBUS_FDT_PNP_INFO(compat_data); IICBUS_FDT_PNP_INFO(compat_data);
#endif #endif