| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <zh2cvvhvdklwnrnhmzsgajk5ryk7gwd5sayde656ddysi53d7b@frw2ph3opmoe>
Date: Wed, 17 Sep 2025 13:06:53 -0700
From: Dmitry Torokhov <dmitry.torokhov@...il.com>
To: Ivaylo Ivanov <ivo.ivanov.ivanov1@...il.com>
Cc: Krzysztof Kozlowski <krzk@...nel.org>, Rob Herring <robh@...nel.org>,
Conor Dooley <conor+dt@...nel.org>, Henrik Rydberg <rydberg@...math.org>,
linux-samsung-soc@...r.kernel.org, linux-input@...r.kernel.org, devicetree@...r.kernel.org,
linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v1 2/2] Input: s6sa552 - add a driver for the Samsung
A552 touchscreen controller
Hi Ivaylo,
On Sun, Sep 14, 2025 at 04:44:57PM +0300, Ivaylo Ivanov wrote:
> The S6SA552 touchscreen is a capacitive multi-touch controller for
> mobile use. It connects via i2c at the address 0x48.
>
> Introduce a basic driver, which can handle initialization, touch events
> and power states.
>
> At least the firmware for this IC on Galaxy S7 differs from S6SY761
> in register layout and bits, as well as some missing registers/functions,
> for example for retrieving the max X/Y coordinates and the amount
> of TX/RX channels.
I am not sure why you are using runtime PM in the driver, given that you
enable it on probe and disable it in remove and otherwise do not touch.
If you want to use it then you should probably call runtime_pm_get() and
runtime_pm_put() from open()/close() methods instead of toggling power
directly.
[...]
> +
> +static void s6sa552_input_close(struct input_dev *dev)
> +{
> + struct s6sa552_data *sdata = input_get_drvdata(dev);
> + int ret;
int error;
> +
> + ret = i2c_smbus_write_byte(sdata->client, S6SA552_SENSE_OFF);
> + if (ret)
> + dev_err(&sdata->client->dev, "failed to turn off sensing\n");
> +}
> +
> +static ssize_t s6sa552_sysfs_devid(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct s6sa552_data *sdata = dev_get_drvdata(dev);
> +
> + return sprintf(buf, "%#x\n", sdata->devid);
> +}
> +
> +static DEVICE_ATTR(devid, 0444, s6sa552_sysfs_devid, NULL);
> +
> +static struct attribute *s6sa552_sysfs_attrs[] = {
> + &dev_attr_devid.attr,
> + NULL
> +};
> +ATTRIBUTE_GROUPS(s6sa552_sysfs);
> +
> +static int s6sa552_power_on(struct s6sa552_data *sdata)
> +{
> + u8 buffer[S6SA552_EVENT_SIZE];
> + int ret;
int error;
Use "error" for storing error values from APIs that return negative or
0. For APIs that also return real values "ret" is fine.
> +
> + ret = regulator_bulk_enable(ARRAY_SIZE(sdata->regulators),
> + sdata->regulators);
> + if (ret)
> + return ret;
> +
> + msleep(140);
> +
> + /* double check whether the touch is functional */
> + ret = i2c_smbus_read_i2c_block_data(sdata->client,
> + S6SA552_READ_ONE_EVENT,
> + S6SA552_EVENT_SIZE,
> + buffer);
> + if (ret < 0)
> + return ret;
> +
> + if (buffer[0] != S6SA552_EVENT_TYPE_ACK ||
> + buffer[1] != S6SA552_EVENT_ACK_BOOT) {
> + return -ENODEV;
> + }
> +
> + ret = i2c_smbus_read_byte_data(sdata->client, S6SA552_BOOT_STATUS);
> + if (ret < 0)
> + return ret;
> +
> + /* for some reasons the device might be stuck in the bootloader */
> + if (ret != S6SA552_BS_APPLICATION)
> + return -ENODEV;
> +
> + /* enable touch functionality */
> + ret = i2c_smbus_write_byte_data(sdata->client,
> + S6SA552_TOUCH_FUNCTION, 0x01);
> + if (ret)
> + return ret;
> +
> + mdelay(20); /* make sure everything is up */
> +
> + return 0;
> +}
> +
> +static int s6sa552_hw_init(struct s6sa552_data *sdata)
> +{
> + u8 buffer[S6SA552_DEVID_SIZE];
> + int ret;
> +
> + ret = s6sa552_power_on(sdata);
> + if (ret)
> + return ret;
> +
> + ret = i2c_smbus_read_i2c_block_data(sdata->client,
> + S6SA552_DEVICE_ID,
> + S6SA552_DEVID_SIZE,
> + buffer);
> + if (ret < 0)
> + return ret;
> +
> + sdata->devid = get_unaligned_be16(buffer + 1);
> +
> + return 0;
> +}
> +
> +static void s6sa552_power_off(void *data)
> +{
> + struct s6sa552_data *sdata = data;
> +
> + disable_irq(sdata->client->irq);
> + regulator_bulk_disable(ARRAY_SIZE(sdata->regulators),
> + sdata->regulators);
> +}
> +
> +static int s6sa552_probe(struct i2c_client *client)
> +{
> + struct s6sa552_data *sdata;
> + int err;
> +
> + if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C |
> + I2C_FUNC_SMBUS_BYTE_DATA |
> + I2C_FUNC_SMBUS_I2C_BLOCK))
> + return -ENODEV;
> +
> + sdata = devm_kzalloc(&client->dev, sizeof(*sdata), GFP_KERNEL);
> + if (!sdata)
> + return -ENOMEM;
> +
> + i2c_set_clientdata(client, sdata);
> + sdata->client = client;
> +
> + sdata->regulators[S6SA552_REGULATOR_VDD].supply = "vdd";
> + sdata->regulators[S6SA552_REGULATOR_AVDD].supply = "avdd";
> + err = devm_regulator_bulk_get(&client->dev,
> + ARRAY_SIZE(sdata->regulators),
> + sdata->regulators);
> + if (err)
> + return err;
> +
> + err = devm_add_action_or_reset(&client->dev, s6sa552_power_off, sdata);
> + if (err)
> + return err;
> +
> + err = s6sa552_hw_init(sdata);
> + if (err)
> + return err;
> +
> + sdata->input = devm_input_allocate_device(&client->dev);
> + if (!sdata->input)
> + return -ENOMEM;
> +
> + sdata->input->name = S6SA552_DEV_NAME;
> + sdata->input->id.bustype = BUS_I2C;
> + sdata->input->open = s6sa552_input_open;
> + sdata->input->close = s6sa552_input_close;
> +
> + input_set_abs_params(sdata->input, ABS_MT_POSITION_X, 0, S6SA552_MAX_X,
> + 0, 0);
> + input_set_abs_params(sdata->input, ABS_MT_POSITION_Y, 0, S6SA552_MAX_Y,
> + 0, 0);
> + input_set_abs_params(sdata->input, ABS_MT_TOUCH_MAJOR, 0, 255, 0, 0);
> + input_set_abs_params(sdata->input, ABS_MT_TOUCH_MINOR, 0, 255, 0, 0);
> + input_set_abs_params(sdata->input, ABS_MT_PRESSURE, 0, 255, 0, 0);
> +
> + touchscreen_parse_properties(sdata->input, true, &sdata->prop);
> +
> + if (!input_abs_get_max(sdata->input, ABS_X) ||
> + !input_abs_get_max(sdata->input, ABS_Y)) {
> + dev_warn(&client->dev, "the axis have not been set\n");
> + }
> +
> + err = input_mt_init_slots(sdata->input, S6SA552_TX_CHANNELS,
> + INPUT_MT_DIRECT);
> + if (err)
> + return err;
> +
> + input_set_drvdata(sdata->input, sdata);
> +
> + err = input_register_device(sdata->input);
> + if (err)
> + return err;
> +
> + err = devm_request_threaded_irq(&client->dev, client->irq, NULL,
> + s6sa552_irq_handler,
> + IRQF_TRIGGER_LOW | IRQF_ONESHOT,
Do not hardcode trigger type, just use IRQF_ONESHOT.
> + "s6sa552_irq", sdata);
> + if (err)
> + return err;
> +
> + pm_runtime_enable(&client->dev);
> +
> + return 0;
> +}
Thanks.
--
Dmitry
Powered by blists - more mailing lists