On Thu, Sep 17, 2020 at 11:52:28AM +0100, Dan Scally wrote:
> On 17/09/2020 11:33, Sakari Ailus wrote:
I will do better review for next version, assuming you will Cc reviewers and
TWIMC people. Below is like small part of comments I may give to the code.
...
> > The ones I know require PMIC control done in software (not even
> > sensors are accessible without that).
> So far we've just been getting the sensor drivers themselves to toggle
> the gpio pins that turn the PMIC on (those pins are listed against the
> PMIC's _CRS, and we've been finding those by evaluating the sensor's
> _DEP) - once that's done the cameras show up on i2c and,with the bridge
> driver installed, you can use libcamera to take photos.
Do I understand correctly that you are able to get pictures from the camera
hardware?
...
> > a module and not enlarge everyone's kernel, and the initialisation would at
> > the same time take place before the rest of what the CIO2 driver does in
> > probe.
> I thought of that as well, but wasn't sure which was preferable. I can
> compress it into the CIO2 driver though sure.
Sakari, I tend to agree with Dan and have the board file separated from the
driver and even framework.
...
> > Cc Andy, too.
Thanks!
...
> >> I wanted to raise this as an RFC as although I don't think it's ready for
> >> integration it has some things that I'd like feedback on, in particular the
> >> method I chose to make the module be auto-inserted. A more ideal method would
> >> have been to have the driver be an ACPI driver for the INT343E device, but each
> > What do you think this device does represent? Devices whose status is
> > always zero may exist in the table even if they would not be actually
> > present.
> >
> > CIO2 is a PCI device and it has no ACPI (or PNP) ID, or at least should not
> > have one.
> This is the ACPI entry I mean:
>
> Device (CIO2)
> {
> Method (_STA, 0, NotSerialized) // _STA: Status
> {
> If ((CIOE == One))
> {
> Return (0x0F)
> }
> Else
> {
> Return (Zero)
> }
> }
>
> Name (_HID, "INT343E") // _HID: Hardware ID
> Method (_CRS, 0, NotSerialized) // _CRS: Current Resource Settings
> {
> Name (CBUF, ResourceTemplate ()
> {
> Interrupt (ResourceConsumer, Level, ActiveLow, Shared, ,, _Y15)
> {
> 0x00000010,
> }
> Memory32Fixed (ReadWrite,
> 0xFE400000, // Address Base
> 0x00010000, // Address Length
> )
> })
> CreateDWordField (CBUF, \_SB.PCI0.CIO2._CRS._Y15._INT, CIOV) // _INT: Interrupts
> CIOV = CIOI /* \CIOI */
> Return (CBUF) /* \_SB_.PCI0.CIO2._CRS.CBUF */
> }
> }
Ah, I think you misinterpreted the meaning of above. The above is a switch how
camera device appears either as PCI or an ACPI. So, it effectively means you
should *not* have any relation for this HID until you find a platform where the
device is for real enumerated via ACPI.
...
> >> +static int cio2_probe_can_progress(struct pci_dev *pci_dev)
> >> +{
> >> + void *sensor;
Why void?
> >> + /*
> >> + * On ACPI platforms, we need to probe _after_ sensors wishing to connect
> >> + * to cio2 have added a device link. If there are no consumers yet, then
> >> + * we need to defer. The .sync_state() callback will then be called after
> >> + * all linked sensors have probed
> >> + */
> >> +
> >> + if (IS_ENABLED(CONFIG_ACPI)) {
> >> + sensor = (struct device *)list_first_entry_or_null(
Besides the fact that castings from or to void * are implicit in C, the proper
use of list API should have pretty well defined type of lvalue.
> >> + &pci_dev->dev.links.consumers,
> >> + struct dev_links_info,
> >> + consumers);
> > Please wrap so it's under 80.
> >
> Will do
> >> +
> >> + if (!sensor)
> >> + return -EPROBE_DEFER;
> >> + }
> >> +
> >> + return 0;
> >> +}
...
> >> + if (!IS_ENABLED(CONFIG_ACPI)) {
> >> + r = cio2_parse_firmware(cio2);
> >> + if (r)
> >> + goto fail_clean_notifier;
> >> + }
How comes?
...
> >> \ No newline at end of file
???
Be sure you are using good editor.
...
> >> +#include <acpi/acpi_bus.h>
Redundant. ACPI headers are designed the way that you are using a single header
in Linux kernel for all. It might be different in drivers/acpi stuff, but not
in general.
> >> +#include <linux/device.h>
> >> +#include <linux/i2c.h>
> >> +#include <linux/kernel.h>
> >> +#include <linux/module.h>
> >> +#include <linux/pci.h>
> >> +#include <media/v4l2-subdev.h>
> >> +
> >> +#include <linux/fwnode.h>
> >> +#include <linux/kref.h>
Please, keep them sorted. And since it's for media, the media inclusion may be
placed last in a separate group.
...
> >> +#define PROPERTY_ENTRY_NULL \
> >> +((const struct property_entry) { })
> > Alignment. Same appears to apply to other macros (please indent).
> Yep
> >
> >> +#define SOFTWARE_NODE_NULL \
> >> +((const struct software_node) { })
Why?!
...
> >> +struct software_node cio2_hid_node = { CIO2_HID, };
static ?
Same for other global variables.
...
> >> +struct cio2_bridge bridge = { 0, };
When define as static the assignment will not be needed.
...
> >> +static int read_acpi_block(struct device *dev, char *id, void *data, u32 size)
> >> +{
> >> + union acpi_object *obj;
> >> + struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
> >> + struct acpi_handle *dev_handle = ACPI_HANDLE(dev);
Usually we use simple handle if there is no ambiguous reading.
> >> + int status;
Should be acpi_status
> >> + u32 buffer_length;
> >> +
> >> + status = acpi_evaluate_object(dev_handle, id, NULL, &buffer);
> >> + if (!ACPI_SUCCESS(status))
ACPI_FAILURE()
> >> + return -ENODEV;
> >> +
> >> + obj = (union acpi_object *)buffer.pointer;
Why explicit casting?
> >> + if (!obj || obj->type != ACPI_TYPE_BUFFER) {
> >> + dev_err(dev, "Could't read acpi buffer\n");
> >> + status = -ENODEV;
Should have different int type variable for that.
> >> + goto err;
If there is no obj, you may return directly without freeing.
> >> + }
> >> +
> >> + if (obj->buffer.length > size) {
> >> + dev_err(dev, "Given buffer is too small\n");
> >> + status = -ENODEV;
> >> + goto err;
> >> + }
> >> +
> >> + memcpy(data, obj->buffer.pointer, min(size, obj->buffer.length));
Does type of size and length the same? Otherwise you need min_t().
> >> + buffer_length = obj->buffer.length;
> >> + kfree(buffer.pointer);
> >> +
> >> + return buffer_length;
> >> +err:
Consider naming labels by what they are about to do. Like
err_free:
here.
> >> + kfree(buffer.pointer);
> >> + return status;
> >> +}
> >> +static int get_acpi_ssdb_sensor_data(struct device *dev,
> >> + struct sensor_bios_data *sensor)
> >> +{
> >> + struct sensor_bios_data_packed sensor_data;
> >> + int ret = read_acpi_block(dev, "SSDB", &sensor_data,
> >> + sizeof(sensor_data));
Please, split declaration and assignment especially in the cases where it
requires long lines.
> >> + if (ret < 0) {
> >> + dev_err(dev, "Failed to fetch SSDB data\n");
> >> + return ret;
> >> + }
> >> +
> >> + sensor->link = sensor_data.link;
> >> + sensor->lanes = sensor_data.lanes;
> >> + sensor->mclkspeed = sensor_data.mclkspeed;
> >> +
> >> + return 0;
> >> +}
...
> >> + if (!dev->driver_data) {
> >> + pr_info("ACPI match for %s, but it has no driver\n",
> >> + supported_devices[i]);
> >> + continue;
> >> + } else {
> >> + pr_info("Found supported device %s\n",
> >> + supported_devices[i]);
> >> + }
Positive conditions are easier to read, but on the other hand 'else' is
redundant in such conditionals (where if branch bails out from the flow).
--
With Best Regards,
Andy Shevchenko
